diff --git a/tests/tools/test_memory_tool.py b/tests/tools/test_memory_tool.py index 43f0bf27b3b..06e8cf0c612 100644 --- a/tests/tools/test_memory_tool.py +++ b/tests/tools/test_memory_tool.py @@ -604,16 +604,30 @@ class TestExternalDriftGuard: assert Path(bak).exists() assert "Vendor Master" in Path(bak).read_text() - def test_add_refuses_on_drift(self, store): - store.add("memory", "Existing.") - path = self._plant_drift(store) - original = path.read_text() + def test_add_succeeds_despite_drift(self, store): + """Add (append) should succeed even when on-disk content shows drift. + + The drift guard protects replace/remove from clobbering un-roundtrippable + content, but add only appends — it never overwrites existing entries. + Issue #42874: prior-session add() writes shift the byte count, causing + the round-trip check to fire on subsequent adds in the same session. + """ + store.add("memory", "Existing entry.") + # Plant a mild drift: append content that won't round-trip but stays + # under the char limit (500 chars in test fixture). + path = store._path_for("memory") + path.write_text( + path.read_text(encoding="utf-8") + "\nextra content no delimiter", + encoding="utf-8", + ) result = store.add("memory", "New entry under drift.") - assert result["success"] is False - assert "drift_backup" in result - assert path.read_text() == original # untouched + assert result["success"] is True + # The new entry is appended — existing drift content is preserved. + updated = path.read_text(encoding="utf-8") + assert "New entry under drift." in updated + assert "extra content no delimiter" in updated def test_remove_refuses_on_drift(self, store): store.add("memory", "Target entry to remove.") @@ -668,12 +682,16 @@ class TestExternalDriftGuard: overwrite the first .bak. The current implementation accepts that — both files describe the same on-disk state — but pin the path format here so any future change has to think about it. + + Note: add() no longer triggers drift detection (issue #42874) — + only replace/remove do. Both r1 and r2 use replace/remove. """ store.add("memory", "Initial.") + store.add("memory", "Second entry.") self._plant_drift(store) r1 = store.replace("memory", "Initial", "Replacement.") - r2 = store.add("memory", "Another.") + r2 = store.remove("memory", "Second entry") assert r1.get("drift_backup") assert r2.get("drift_backup") # Same epoch second is the expected collision case — both point diff --git a/tools/memory_tool.py b/tools/memory_tool.py index 47d9d2c9922..96a8a1a0a18 100644 --- a/tools/memory_tool.py +++ b/tools/memory_tool.py @@ -248,7 +248,7 @@ class MemoryStore: return mem_dir / "USER.md" return mem_dir / "MEMORY.md" - def _reload_target(self, target: str) -> Optional[str]: + def _reload_target(self, target: str, *, skip_drift: bool = False) -> Optional[str]: """Re-read entries from disk into in-memory state. Called under file lock to get the latest state before mutating. @@ -258,9 +258,13 @@ class MemoryStore: When drift is detected the caller must abort the mutation — flushing would discard the un-roundtrippable content. Returns None on clean reload. + + When *skip_drift* is True the round-trip / entry-size check is + bypassed. Used by the ``add`` action which appends without + rewriting, so existing content is never clobbered. """ path = self._path_for(target) - bak = self._detect_external_drift(target) + bak = None if skip_drift else self._detect_external_drift(target) fresh = self._read_file(path) fresh = list(dict.fromkeys(fresh)) # deduplicate self._set_entries(target, fresh) @@ -306,12 +310,12 @@ class MemoryStore: with self._file_lock(self._path_for(target)): # Re-read from disk under lock to pick up writes from other sessions. - # If external drift was detected, the file was backed up to .bak. - # — refuse the mutation so we don't clobber the un-roundtrippable - # content the patch tool / shell append / sister session wrote. - bak = self._reload_target(target) - if bak: - return _drift_error(self._path_for(target), bak) + # For add (append-only), we skip the drift guard — appending never + # clobbers existing content, so round-trip mismatches from prior + # tool-written entries in the same session are harmless. The drift + # guard remains active for replace/remove where full-file rewrite + # would discard un-roundtrippable content (issue #26045). + self._reload_target(target, skip_drift=True) entries = self._entries_for(target) limit = self._char_limit(target)