Memory tool parameter naming and schema validation bugs

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #21238 fix(memory): require schema fields by action (salvage #19180)

  merged to main

  files: tests/tools/test_memory_tool_schema.py, tools/memory_tool.py

  Show the diff

  • tests/tools/test_memory_tool_schema.py

    @@ -0,0 +1,39 @@
    +import json
    +from tools.memory_tool import MEMORY_SCHEMA
    +
    +
    +def test_memory_schema_requires_content_and_old_text_for_replace_action():
    +    schema = MEMORY_SCHEMA["parameters"]
    +    assert schema["required"] == ["action", "target"]
    +
    +    all_of = schema.get("allOf")
    +    assert all_of, "memory schema should use conditional requirements"
    +
    +    replace_requirements = [
    +        branch["then"].get("required", [])
    +        for branch in all_of
    +        if branch.get("if", {}).get("properties", {}).get("action", {}).get("const") == "replace"
    +    ]
    +    assert replace_requirements == [["old_text", "content"]]
    +
    +
    +def test_memory_schema_requires_content_for_add_action():
    +    add_requirements = [
    +        branch["then"].get("required", [])
    +        for branch in MEMORY_SCHEMA["parameters"].get("allOf", [])
    +        if branch.get("if", {}).get("properties", {}).get("action", {}).get("const") == "add"
    +    ]
    +    assert add_requirements == [["content"]]
    +
    +
    +def test_memory_schema_requires_old_text_for_remove_action():
    +    remove_requirements = [
    +        branch["then"].get("required", [])
    +        for branch in MEMORY_SCHEMA["parameters"].get("allOf", [])
    +        if branch.get("if", {}).get("properties", {}).get("action", {}).get("const") == "remove"
    +    ]
    +    assert remove_requirements == [["old_text"]]
    +
    +
    +def test_memory_schema_is_json_serializable():
    +    json.dumps(MEMORY_SCHEMA)

  • tools/memory_tool.py

    @@ -560,6 +560,29 @@ def check_memory_requirements() -> bool:
                 },
             },
             "required": ["action", "target"],
    +        "allOf": [
    +            {
    +                "if": {
    +                    "properties": {"action": {"const": "add"}},
    +                    "required": ["action"],
    +                },
    +                "then": {"required": ["content"]},
    +            },
    +            {
    +                "if": {
    +                    "properties": {"action": {"const": "replace"}},
    +                    "required": ["action"],
    +                },
    +                "then": {"required": ["old_text", "content"]},
    +            },
    +            {
    +                "if": {
    +                    "properties": {"action": {"const": "remove"}},
    +                    "required": ["action"],
    +                },
    +                "then": {"required": ["old_text"]},
    +            },
    +        ],
         },
     }
     

• #49997 fix(memory): recover from missing old_text on single-op replace/remove

  merged to main

  files: tests/tools/test_memory_tool.py, tools/memory_tool.py

  Show the diff

  • tests/tools/test_memory_tool.py

    @@ -435,12 +435,33 @@ def test_add_via_tool(self, store):
             assert result["success"] is True
     
         def test_replace_requires_old_text(self, store):
    +        # Missing old_text on a single-op replace is recoverable, not a dead-end:
    +        # return the current inventory + a retry instruction so the model can
    +        # reissue with old_text set. (issues #43412, #49466)
    +        store.add("memory", "fact A")
    +        store.add("memory", "fact B")
             result = json.loads(memory_tool(action="replace", content="new", store=store))
             assert result["success"] is False
    +        assert "old_text" in result["error"]
    +        assert result["current_entries"] == ["fact A", "fact B"]
    +        assert "usage" in result
     
         def test_remove_requires_old_text(self, store):
    +        store.add("memory", "fact A")
             result = json.loads(memory_tool(action="remove", store=store))
             assert result["success"] is False
    +        assert "old_text" in result["error"]
    +        assert result["current_entries"] == ["fact A"]
    +        assert "usage" in result
    +
    +    def test_replace_missing_content_still_distinct_error(self, store):
    +        # When old_text IS present but content is missing, keep the original
    +        # content-specific error (don't route through the old_text recovery path).
    +        store.add("memory", "fact A")
    +        result = json.loads(memory_tool(action="replace", old_text="fact A", store=store))
    +        assert result["success"] is False
    +        assert "content is required" in result["error"]
    +        assert "current_entries" not in result
     
     
     class TestMemoryBatch:

  • tools/memory_tool.py

    @@ -835,6 +835,38 @@ def _apply_batch_write_gate(target: str, operations: List[Dict[str, Any]]) -> Op
         )
     
     
    +def _missing_old_text_error(store: "MemoryStore", target: str, action: str) -> str:
    +    """Build a recoverable error for a replace/remove call that arrived without
    +    ``old_text``.
    +
    +    ``replace``/``remove`` are inherently targeted -- without ``old_text`` there
    +    is no entry to act on, so we cannot fulfil the call. But returning a bare
    +    "old_text is required" is a dead-end: some structured-output clients omit the
    +    optional ``old_text`` field (it isn't, and can't be, schema-required without
    +    a top-level combinator the Codex backend rejects -- see
    +    tests/tools/test_memory_tool_schema.py). So instead we return the current
    +    entry inventory plus an explicit retry instruction, letting the model reissue
    +    the call with ``old_text`` set to a unique substring of the entry it means.
    +    Mirrors the batch path's ``_batch_error`` shape. (issues #43412, #49466)
    +    """
    +    entries = store._entries_for(target)
    +    current = store._char_count(target)
    +    limit = store._char_limit(target)
    +    return json.dumps(
    +        {
    +            "success": False,
    +            "error": (
    +                f"'{action}' needs old_text -- a short unique substring of the entry "
    +                f"to {action}. None was provided. Reissue the {action} with old_text "
    +                f"set to part of one of the current_entries below."
    +            ),
    +            "current_entries": entries,
    +            "usage": f"{current:,}/{limit:,}",
    +        },
    +        ensure_ascii=False,
    +    )
    +
    +
     def memory_tool(
         action: str = None,
         target: str = "memory",
    @@ -876,9 +908,15 @@ def memory_tool(
             return tool_error("Content is required for 'add' action.", success=False)
         if action == "replace" and (not old_text or not content):
             missing = "old_text" if not old_text else "content"
    +        if not old_text:
    +            # The client/model omitted old_text. Replace is inherently targeted
    +            # -- we can't guess which entry. Return the current inventory plus a
    +            # retry instruction so the model can reissue with old_text set,
    +            # instead of hitting a dead-end error. (issues #43412, #49466)
    +            return _missing_old_text_error(store, target, "replace")
             return tool_error(f"{missing} is required for 'replace' action.", success=False)
         if action == "remove" and not old_text:
    -        return tool_error("old_text is required for 'remove' action.", success=False)
    +        return _missing_old_text_error(store, target, "remove")
     
         # Approval gate: when on, stages the write (background/gateway) or prompts
         # inline (interactive CLI); when off (default) passes straight through.
    @@ -971,7 +1009,7 @@ def apply_memory_pending(payload: Dict[str, Any], store: "MemoryStore") -> Dict[
                 },
                 "old_text": {
                     "type": "string",
    -                "description": "Short unique substring identifying the entry to replace or remove (single-op shape)."
    +                "description": "REQUIRED for 'replace' and 'remove' (single-op shape): a short unique substring identifying the existing entry to modify. Omit only for 'add'."
                 },
                 "operations": {
                     "type": "array",