Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 additions & 8 deletions tests/tools/test_memory_tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -486,16 +486,30 @@ def test_replace_refuses_on_drift(self, store):
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.")
Expand Down Expand Up @@ -550,12 +564,16 @@ def test_drift_backup_filename_is_unique_per_invocation(self, store):
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
Expand Down
20 changes: 12 additions & 8 deletions tools/memory_tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ def _path_for(target: str) -> Path:
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.
Expand All @@ -259,9 +259,13 @@ def _reload_target(self, target: str) -> Optional[str]:
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)
Expand Down Expand Up @@ -307,12 +311,12 @@ def add(self, target: str, content: str) -> Dict[str, Any]:

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.<ts>
# β€” 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)
Expand Down
Loading