diff --git a/tests/agent/test_learning_mutations.py b/tests/agent/test_learning_mutations.py index f04a3f842f..f78b7beed4 100644 --- a/tests/agent/test_learning_mutations.py +++ b/tests/agent/test_learning_mutations.py @@ -186,13 +186,13 @@ def test_edit_memory_keeps_concurrent_memory_tool_add(home, monkeypatch): lambda: lm.edit_node("memory:memory:0", "alpha rewritten")], ids=["delete", "edit"]) def test_memory_drift_is_refused_with_backup_like_memory_tool(home, mutate): - """Hand-edited content that wouldn't round-trip through the § parser is what - the memory tool's drift guard exists for: snapshot to .bak, refuse, leave the - file untouched. A Journey mutation must not reformat it silently.""" + """Free-form text appended without § delimiters (one "entry" past the whole-store + limit) is what the memory tool's drift guard exists for: snapshot to .bak, refuse, + leave the file untouched. A Journey mutation must not flatten it silently.""" from tools.memory_tool_store import _drift_error path = home / "memories" / "MEMORY.md" - raw = "alpha note\n§\n\n§\nbeta note\n" + raw = "alpha note\n§\nbeta note\n\n## Vendor Master\n" + "x" * 2300 + "\n" path.write_text(raw, encoding="utf-8") res = mutate() diff --git a/tests/tools/test_memory_tool.py b/tests/tools/test_memory_tool.py index be0eaa3e64..b94af3c045 100644 --- a/tests/tools/test_memory_tool.py +++ b/tests/tools/test_memory_tool.py @@ -253,12 +253,26 @@ class TestMemoryStoreReplace: store.add("memory", 'User runs a fleet (\u201cOmarchy\u201d \u2014 Trinity)') store.add("memory", "Tests: run \u2018make test\u2019 (needs\n docker up)") assert store.replace("memory", "fleet (\"Omarchy\" - Trinity)", "fleet (Zeus hub)")["success"] is True - assert store.apply_batch("memory", [{"action": "replace", "old_text": "run 'make test' (needs docker up)", + assert store.apply_batch("memory", [{"action": "replace", "old_text": "run 'make test' (needs\\n docker up)", "content": "Tests: make test-fast"}])["success"] is True assert store.memory_entries == ["fleet (Zeus hub)", "Tests: make test-fast"] result = store.replace("memory", "fleet 'on-prem'", "x") assert result["success"] is False and "No entry matched" in result["error"] +class TestSelfCorrectingFailures: + def test_failures_name_the_fix(self, store): + """A refused write tells the model exactly what to change: the chars to free, and + for an old_text that matches nothing, the entries it most resembles.""" + store.add("memory", "Deploys go through GitHub Actions to Fly.io with manual approval.") + store.add("memory", "x" * 380) # 65 + 3 + 380 = 448 of 500 + full = store.add("memory", "y" * 54) # 448 + 3 + 54 = 505 + assert "by 5 chars" in full["error"] and "free at least 5 chars" in full["error"] + miss = store.apply_batch("memory", [{"action": "replace", "old_text": "Deploys via GitHub Actions to Fly", + "content": "Deploys: Fly.io + canary."}]) + assert miss["closest_entries"] == ["Deploys go through GitHub Actions to Fly.io with manual approval."] + assert "current_entries" not in miss # batch aborts still never echo the store (#97316) + + class TestMemoryStoreRemove: def test_remove_entry(self, store): store.add("memory", "temporary note") @@ -676,6 +690,16 @@ class TestExternalDriftGuard: result = store.replace("memory", "Entry two", "Entry two replaced.") assert result["success"] is True + def test_hand_formatted_layout_is_not_drift(self, store): + """#107270: blank lines around § and an empty entry lose nothing on a flush (parsing + only strips whitespace), so replace/remove must work and leave no .bak behind.""" + path = store._path_for("memory") + path.write_text("Deploys: Fly.io, manual approval.\n\n§\n\nHost: forge\n§\n\n§\nRepo: acme", encoding="utf-8") + assert store.replace("memory", "Deploys:", "Deploys: Fly.io, 30m canary, manual approval.")["success"] is True + assert store.remove("memory", "Repo: acme")["success"] is True + assert path.read_text(encoding="utf-8") == "Deploys: Fly.io, 30m canary, manual approval.\n§\nHost: forge" + assert not list(path.parent.glob("MEMORY.md.bak.*")) + def test_drift_guard_also_protects_user_target(self, store): """USER.md gets the same guarantee as MEMORY.md.""" store.add("user", "Some preference.") diff --git a/tools/memory_tool_store.py b/tools/memory_tool_store.py index 28463815a8..2957598a1d 100644 --- a/tools/memory_tool_store.py +++ b/tools/memory_tool_store.py @@ -5,6 +5,7 @@ in ``tools.memory_tool`` and is read lazily.""" import logging import os +import re import time from contextlib import contextmanager, suppress from pathlib import Path @@ -36,9 +37,9 @@ def _error(message: str, **extra) -> Dict[str, Any]: def _drift_error(path: Path, bak_path: str) -> Dict[str, Any]: """External drift: the file wouldn't round-trip, so flushing would discard content.""" return _error(( - f"Refusing to write {path.name}: file on disk has content that wouldn't round-trip " - f"through the memory tool (likely added by the patch tool, a shell append, a manual edit, " - f"or a concurrent session). A snapshot was saved to {bak_path}. Resolve the drift first — " + f"Refusing to write {path.name}: it holds an entry larger than the whole store's char limit, " + f"i.e. free-form text written without § delimiters (the patch tool, a shell append, a manual " + f"edit). Rewriting it would lose that text. A snapshot was saved to {bak_path}. Resolve the drift first — " f"either rewrite the file as a clean §-delimited list of entries, or move the extra " f"content out — then retry. This guard exists to prevent silent data loss (issue #26045)." ), drift_backup=bak_path, remediation=( @@ -63,9 +64,10 @@ _MATCH_FOLD = str.maketrans({**dict.fromkeys("\"`\u2018\u2019\u201a\u201b\u201c\ def _normalize_for_match(text: str) -> str: """Quote/dash/whitespace-folded form for tolerant old_text matching. Models re-emit an - entry with straight quotes for curly ones, '-' for an em dash, or unwrapped lines, and a - byte-exact substring test then reports "No entry matched" for an unambiguous target.""" - return " ".join(str(text).translate(_MATCH_FOLD).split()) + entry with straight quotes for curly ones, '-' for an em dash, unwrapped lines, or a + newline over-escaped to a literal backslash-n, and a byte-exact substring test then + reports "No entry matched" for an unambiguous target.""" + return " ".join(str(text).replace("\\n", " ").translate(_MATCH_FOLD).split()) def _substring_matches(entries: List[str], old_text: str) -> List[int]: @@ -92,6 +94,27 @@ def _find_unique_match(entries: List[str], old_text: str) -> Tuple[Optional[int] return (matches[0] if matches else None), False +def _no_match_error(entries: List[str], old_text: str, verb: str, **extra) -> Dict[str, Any]: + """Zero-match error that names the fix: the entries sharing the most words with + *old_text* (a paraphrased or stale old_text usually still shares most of them), and a + pointer when old_text spans several entries joined by the delimiter.""" + def _words(text: str) -> set: + return set(re.findall(r"\w+", text.lower())) + words = _words(old_text) + scored = sorted(((len(words & _words(e)) / len(words), i) for i, e in enumerate(entries)) if words else [], + key=lambda p: (-p[0], p[1])) + closest = [entries[i] for score, i in scored[:3] if score >= 0.3] + hint = (" old_text must come from ONE entry ('§' separates entries); use one op per entry." + if "§" in old_text else "") + return _error(f"No entry matched '{old_text}'.{hint} Retry the {verb} with old_text copied verbatim " + f"from the intended entry" + (" (closest_entries below)." if closest else "."), + **({"closest_entries": closest} if closest else {}), **extra) + + +def _over_limit(new_total: int, limit: int) -> str: + return f"would put memory at {new_total:,}/{limit:,} chars: free at least {new_total - limit:,} chars" + + def _pinned_index(entries: List[str], matched_entry: str) -> Optional[int]: """Index of the exact entry a staged write was reviewed against; None once it is gone (stale).""" return entries.index(matched_entry) if matched_entry in entries else None @@ -258,12 +281,14 @@ class MemoryStore: return self._consolidation_failure( _error(message, current_entries=self._entries_for(target), usage=self._usage(target))) - def _batch_failure(self, target: str, message: str) -> Dict[str, Any]: + def _batch_failure(self, target: str, message: str, closest_entries: Optional[List[str]] = None) -> Dict[str, Any]: """Batch-abort failure WITHOUT ``current_entries``: the store did not change and the caller already holds the inventory, so echoing it made each consolidation retry - grow the context it was invoked to shrink (#97316).""" + grow the context it was invoked to shrink (#97316). A zero-match op carries only + its few ``closest_entries``.""" return self._consolidation_failure( - _error(message + " No operations were applied (batch is all-or-nothing).", usage=self._usage(target))) + _error(message + " No operations were applied (batch is all-or-nothing).", usage=self._usage(target), + **({"closest_entries": closest_entries} if closest_entries else {}))) def _mutate(self, target: str, mutate, *, skip_drift: bool = False) -> Dict[str, Any]: """Lock, re-read from disk, run ``mutate(entries, limit)`` -> ``(new_entries, message)`` @@ -308,11 +333,12 @@ class MemoryStore: if content in entries: return self._success_response(target, "Entry already exists (no duplicate added).") if len(ENTRY_DELIMITER.join(entries + [content])) > limit: + new_total = len(ENTRY_DELIMITER.join(entries + [content])) return self._failure_with_entries(target, ( - f"Memory at {self._char_count(target):,}/{limit:,} chars. Adding this entry " - f"({len(content)} chars) would exceed the limit. Consolidate now: use 'replace' to merge " - f"overlapping entries into shorter ones or 'remove' stale or less important entries (see " - f"current_entries below), then retry this add — all in this turn.")) + f"Memory at {self._char_count(target):,}/{limit:,} chars; adding this entry ({len(content)} chars) " + f"would exceed the limit by {new_total - limit:,} chars. Retry as ONE 'operations' batch that " + f"removes or shortens (replace) stale entries from current_entries below to free at least " + f"{new_total - limit:,} chars AND adds this entry — the limit is checked only on the batch result.")) return entries + [content], "Entry added." # Append-only: skip the drift guard (appending never clobbers foreign # content) but still refuse a failed read — add rewrites the WHOLE file. @@ -352,9 +378,7 @@ class MemoryStore: matches=[entries[i][:80] + ("..." if len(entries[i]) > 80 else "") for i in _substring_matches(entries, old_text)]) if idx is None: - return self._consolidation_failure(_error( - f"No entry matched '{old_text}'. Check current_entries below and retry with the exact text " - f"of the entry you want to {verb}.", current_entries=entries)) + return self._consolidation_failure(_no_match_error(entries, old_text, verb, current_entries=entries)) return idx def resolve_entry(self, target: str, old_text: str, verb: str) -> Dict[str, Any]: @@ -378,16 +402,17 @@ class MemoryStore: new_total = len(ENTRY_DELIMITER.join(replaced)) if new_total > limit: return self._failure_with_entries(target, ( - f"Replacement would put memory at {new_total:,}/{limit:,} chars. Shorten the new content, " - f"or 'remove' other stale or less important entries to make room (see current_entries " - f"below), then retry — all in this turn.")) + f"Replacement {_over_limit(new_total, limit)}. Shorten the new content, or retry as ONE " + f"'operations' batch with this replace plus removes/shortenings of other stale entries " + f"(see current_entries below) — the limit is checked only on the batch result.")) return replaced, "Entry replaced.", {"replaced_entry": entries[idx]} return self._mutate(target, _apply) @staticmethod def _apply_batch_op(working: List[str], act: str, content: str, old_text: str, pos: str, matched_entry: Optional[str] = None) -> Tuple[Optional[str], Optional[str]]: - """Apply one batch op to *working*; return ``(error message, previous content)``. + """Apply one batch op to *working*; return ``(error message, previous content)`` — + on a zero-match error the second value is that op's ``closest_entries`` instead. Previous content is captured before each replace/remove, under the store lock. It is published only after the entire batch has been validated and persisted. A staged op's *matched_entry* selects exactly that entry, as in ``_locate``. @@ -403,7 +428,8 @@ class MemoryStore: if not old_text: return f"{pos}: old_text is required.", None if act == "replace" and not content: - return f"{pos}: content is required (use action='remove' to delete).", None + return (f"{pos}: content is required — set this operation object's own 'content' to the COMPLETE " + f"new entry (or use action='remove' to delete)."), None if matched_entry is not None: idx = _pinned_index(working, matched_entry) if idx is None: @@ -413,7 +439,8 @@ class MemoryStore: if ambiguous: return f"{pos}: '{old_text}' matched multiple distinct entries -- be more specific.", None if idx is None: - return f"{pos}: no entry matched '{old_text}'.", None + miss = _no_match_error(working, old_text, act) + return f"{pos}: {miss['error']}", miss.get("closest_entries") previous_content = working[idx] working[idx:idx + 1] = [content] if act == "replace" else [] return None, previous_content @@ -451,8 +478,8 @@ class MemoryStore: working, act, (op.get("content") or op.get("new_text") or "").strip(), (op.get("old_text") or "").strip(), f"Operation {i + 1} ({act or 'unknown'})", op.get("matched_entry")) - if msg: - return self._batch_failure(target, msg) + if msg: # a failed op's second value is its closest_entries (or None) + return self._batch_failure(target, msg, closest_entries=previous_content) matched.append(previous_content) if entries and not working: # #103419: a consolidation batch that removes the last entry would @@ -467,9 +494,8 @@ class MemoryStore: new_total = len(ENTRY_DELIMITER.join(working)) # budget check against the FINAL state only if new_total > limit: return self._batch_failure(target, ( - f"After applying all {len(operations)} operations, memory would be at " - f"{new_total:,}/{limit:,} chars -- over the limit. Remove or shorten more " - f"entries in the same batch, then retry.")) + f"Applying all {len(operations)} operations {_over_limit(new_total, limit)} more. Add " + f"removes or shorter replacements of other stale entries to the same batch, then retry.")) if not commit: return {"success": True, "matched_entries": matched} # op index -> full entry text its replace/remove selected (#117952), 1-based to @@ -552,12 +578,13 @@ class MemoryStore: raise RuntimeError(f"Failed to write memory file {path}: {e}") def _detect_external_drift(self, target: str, raw: str) -> Optional[str]: - """``.bak.`` snapshot path if *raw* shows external drift, else None. Signals: - round-trip mismatch, or one entry over the whole-file limit (no tool-written - entry can be — an external writer appended free-form text).""" - parsed = self._parse_entries(raw) - if not raw.strip() or (raw.strip() == ENTRY_DELIMITER.join(parsed) - and max(map(len, parsed), default=0) <= self._char_limit(target)): + """``.bak.`` snapshot path if *raw* shows external drift, else None. The one + signal is an entry over the whole-file limit: no tool-written entry can be, so it is + free-form text appended without delimiters, and replacing that "entry" would discard + it. Layout differences (blank lines around ``§``, empty entries, padding) are NOT + drift: parsing only strips whitespace, so the flush loses nothing but whitespace, and + refusing there blocked every replace/remove on a hand-formatted file for good (#107270).""" + if max(map(len, self._parse_entries(raw)), default=0) <= self._char_limit(target): return None path = self._path_for(target) bak_path = path.with_suffix(path.suffix + f".bak.{int(time.time())}") diff --git a/website/docs/user-guide/features/memory.md b/website/docs/user-guide/features/memory.md index 6ce818afec..67d529647a 100644 --- a/website/docs/user-guide/features/memory.md +++ b/website/docs/user-guide/features/memory.md @@ -171,17 +171,16 @@ When you try to add an entry that would exceed the limit, the tool returns an er ```json { "success": false, - "error": "Memory at 2,100/2,200 chars. Adding this entry (250 chars) would exceed the limit. Consolidate now: use 'replace' to merge overlapping entries into shorter ones or 'remove' stale or less important entries (see current_entries below), then retry this add — all in this turn.", + "error": "Memory at 2,100/2,200 chars; adding this entry (250 chars) would exceed the limit by 153 chars. Retry as ONE 'operations' batch that removes or shortens (replace) stale entries from current_entries below to free at least 153 chars AND adds this entry — the limit is checked only on the batch result.", "current_entries": ["..."], "usage": "2,100/2,200" } ``` -The agent should then: -1. Read the current entries (shown in the error response) -2. Identify entries that can be removed or consolidated -3. Use `replace` to merge related entries into shorter versions -4. Then `add` the new entry +The agent then reissues one `operations` batch that frees at least the stated number of +characters (removes, or `replace`s with shorter versions) and adds the new entry; the limit +is checked only on the batch's final result. A `replace`/`remove` whose `old_text` matches no +entry fails the same way, with the entries it most resembles under `closest_entries`. **Best practice:** When memory is above 80% capacity (visible in the system prompt header), consolidate entries before adding new ones. For example, merge three separate "project uses X" entries into one comprehensive project description entry.