fix(memory): skip drift guard for add (append-only) action (#42874)

The drift guard (introduced for #26045) correctly protects replace/remove
from clobbering un-roundtrippable content, but it also fires on the add
path. Since add only appends and never overwrites, the guard is
unnecessary and causes false positives when prior add() calls in the same
session shift the byte count of the on-disk file.

Add skip_drift parameter to _reload_target() and pass True from add().
Replace/remove continue to use the drift guard unchanged.

Salvaged from #42880 by @liuhao1024.

Closes #42874
This commit is contained in:
liuhao1024 2026-06-25 00:22:14 +05:30 committed by kshitijk4poor
parent b13e2fd694
commit 25e2312230
2 changed files with 38 additions and 16 deletions

View file

@ -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

View file

@ -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.<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)