diff --git a/tests/tools/test_memory_tool.py b/tests/tools/test_memory_tool.py index 50d28d8357a..43f0bf27b3b 100644 --- a/tests/tools/test_memory_tool.py +++ b/tests/tools/test_memory_tool.py @@ -435,12 +435,33 @@ class TestMemoryToolDispatcher: 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: diff --git a/tools/memory_tool.py b/tools/memory_tool.py index eed5742ef39..33d6ffff5e5 100644 --- a/tools/memory_tool.py +++ b/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 @@ MEMORY_SCHEMA = { }, "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",