fix(memory): accept new_text as an alias for content (#86642)
memory replace/remove target an entry by old_text but supply the replacement via content — an asymmetric pairing. Callers naturally reach for new_text to mirror old_text (it's exactly the patch tool's old_string/new_string shape), which left content empty and errored 'content is required'. The failure also rendered tersely, making the cause easy to miss and costing a retry. Accept new_text as an alias for content on both shapes: - single-op: coalesce content = content or new_text in memory_tool() + a new_text param wired through the registry handler. - batch ops: content = op.content or op.new_text (and in the approval-gate preview builder). - schema: document the alias on content and add a new_text property to the single-op params and batch item props so strict validators accept it. content wins if both are set. Tests: new_text alias on single add/replace, batch add/replace, and content-wins-over-new_text. 43/43 across memory tool + schema suites. Co-authored-by: Teknium <teknium1@users.noreply.github.com>
This commit is contained in:
Teknium
GitHub
@@ -306,6 +306,30 @@ class TestMemoryToolDispatcher:
|
||||
assert "content is required" in result["error"]
|
||||
assert "current_entries" not in result
|
||||
|
||||
def test_new_text_alias_for_content_on_replace(self, store):
|
||||
# A caller mirroring old_text with new_text (the patch tool's shape)
|
||||
# must succeed instead of erroring 'content is required'.
|
||||
store.add("memory", "fact A")
|
||||
result = json.loads(
|
||||
memory_tool(action="replace", old_text="fact A", new_text="fact A refined", store=store)
|
||||
)
|
||||
assert result["success"] is True
|
||||
assert "fact A refined" in store.memory_entries
|
||||
assert "fact A" not in [e for e in store.memory_entries if e == "fact A"]
|
||||
|
||||
def test_new_text_alias_for_content_on_add(self, store):
|
||||
result = json.loads(memory_tool(action="add", new_text="added via new_text", store=store))
|
||||
assert result["success"] is True
|
||||
assert "added via new_text" in store.memory_entries
|
||||
|
||||
def test_content_wins_when_both_content_and_new_text_set(self, store):
|
||||
result = json.loads(
|
||||
memory_tool(action="add", content="the real one", new_text="ignored", store=store)
|
||||
)
|
||||
assert result["success"] is True
|
||||
assert "the real one" in store.memory_entries
|
||||
assert "ignored" not in store.memory_entries
|
||||
|
||||
|
||||
class TestMemoryBatch:
|
||||
"""The 'operations' batch shape: atomic, all-or-nothing, final-budget."""
|
||||
@@ -330,6 +354,23 @@ class TestMemoryBatch:
|
||||
assert "usage" in result
|
||||
|
||||
|
||||
def test_batch_new_text_alias_for_content(self, store):
|
||||
# new_text works inside batch ops too (both add and replace).
|
||||
store.add("memory", "old entry")
|
||||
result = json.loads(memory_tool(
|
||||
target="memory",
|
||||
operations=[
|
||||
{"action": "replace", "old_text": "old entry", "new_text": "updated entry"},
|
||||
{"action": "add", "new_text": "batched via new_text"},
|
||||
],
|
||||
store=store,
|
||||
))
|
||||
assert result["success"] is True
|
||||
assert "updated entry" in store.memory_entries
|
||||
assert "batched via new_text" in store.memory_entries
|
||||
assert "old entry" not in store.memory_entries
|
||||
|
||||
|
||||
def test_batch_duplicate_add_is_noop_not_failure(self, store):
|
||||
store.add("memory", "already here")
|
||||
result = json.loads(memory_tool(
|
||||
|
||||
@@ -599,7 +599,7 @@ class MemoryStore:
|
||||
for i, op in enumerate(operations):
|
||||
op = op or {}
|
||||
act = op.get("action")
|
||||
content = (op.get("content") or "").strip()
|
||||
content = (op.get("content") or op.get("new_text") or "").strip()
|
||||
old_text = (op.get("old_text") or "").strip()
|
||||
pos = f"Operation {i + 1} ({act or 'unknown'})"
|
||||
|
||||
@@ -991,12 +991,13 @@ def _apply_batch_write_gate(target: str, operations: List[Dict[str, Any]]) -> Op
|
||||
for op in operations:
|
||||
op = op or {}
|
||||
act = op.get("action", "?")
|
||||
_op_content = op.get("content") or op.get("new_text") or ""
|
||||
if act == "remove":
|
||||
detail_lines.append(f"- remove: {op.get('old_text', '')}")
|
||||
elif act == "replace":
|
||||
detail_lines.append(f"- replace: {op.get('old_text', '')} -> {op.get('content', '')}")
|
||||
detail_lines.append(f"- replace: {op.get('old_text', '')} -> {_op_content}")
|
||||
else:
|
||||
detail_lines.append(f"- {act}: {op.get('content', '')}")
|
||||
detail_lines.append(f"- {act}: {_op_content}")
|
||||
detail = "\n".join(detail_lines)
|
||||
|
||||
decision = wa.evaluate_gate(wa.MEMORY, inline_summary=summary, inline_detail=detail)
|
||||
@@ -1057,6 +1058,7 @@ def memory_tool(
|
||||
target: str = "memory",
|
||||
content: str = None,
|
||||
old_text: str = None,
|
||||
new_text: str = None,
|
||||
operations: Optional[List[Dict[str, Any]]] = None,
|
||||
store: Optional[MemoryStore] = None,
|
||||
) -> str:
|
||||
@@ -1068,11 +1070,22 @@ def memory_tool(
|
||||
- Batch: operations=[{action, content?, old_text?}, ...] applied
|
||||
atomically against the final char budget in ONE call.
|
||||
|
||||
``new_text`` is accepted as an alias for ``content`` on both shapes. The
|
||||
replace/remove ops target by ``old_text`` and supply the replacement via
|
||||
``content``; callers naturally reach for ``new_text`` to mirror
|
||||
``old_text`` (it's the patch tool's ``old_string``/``new_string`` shape),
|
||||
which silently left ``content`` empty and errored. Coalescing here removes
|
||||
that trap.
|
||||
|
||||
Returns JSON string with results.
|
||||
"""
|
||||
if store is None:
|
||||
return tool_error("Memory is not available. It may be disabled in config or this environment.", success=False)
|
||||
|
||||
# Accept new_text as an alias for content (single-op path). See docstring.
|
||||
if content is None and new_text is not None:
|
||||
content = new_text
|
||||
|
||||
# Some strict providers fill optional schema fields with JSON null rather
|
||||
# than omitting them. Treat ``target: null`` as omitted so memory writes
|
||||
# still use the documented default store instead of failing validation.
|
||||
@@ -1196,12 +1209,16 @@ MEMORY_SCHEMA = {
|
||||
},
|
||||
"content": {
|
||||
"type": "string",
|
||||
"description": "The entry content. Required for 'add' and 'replace' (single-op shape)."
|
||||
"description": "The entry content. Required for 'add' and 'replace' (single-op shape). Alias: 'new_text' is also accepted (mirrors old_text)."
|
||||
},
|
||||
"old_text": {
|
||||
"type": "string",
|
||||
"description": "REQUIRED for 'replace' and 'remove' (single-op shape): a short unique substring identifying the existing entry to modify. Omit only for 'add'."
|
||||
},
|
||||
"new_text": {
|
||||
"type": "string",
|
||||
"description": "Alias for 'content' (single-op shape). Provided so the replace/remove old_text/new_text pairing works; if both are set, 'content' wins."
|
||||
},
|
||||
"operations": {
|
||||
"type": "array",
|
||||
"description": (
|
||||
@@ -1213,7 +1230,8 @@ MEMORY_SCHEMA = {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"action": {"type": "string", "enum": ["add", "replace", "remove"]},
|
||||
"content": {"type": "string", "description": "Entry content for add/replace."},
|
||||
"content": {"type": "string", "description": "Entry content for add/replace. Alias: 'new_text'."},
|
||||
"new_text": {"type": "string", "description": "Alias for 'content' in a batch op."},
|
||||
"old_text": {"type": "string", "description": "Substring identifying the entry for replace/remove."},
|
||||
},
|
||||
"required": ["action"],
|
||||
@@ -1237,6 +1255,7 @@ registry.register(
|
||||
target=args.get("target", "memory"),
|
||||
content=args.get("content"),
|
||||
old_text=args.get("old_text"),
|
||||
new_text=args.get("new_text"),
|
||||
operations=args.get("operations"),
|
||||
store=kw.get("store")),
|
||||
check_fn=check_memory_requirements,
|
||||
|
||||
Reference in New Issue
Block a user