fix(memory): refuse mutations when on-disk file changed since last read (#26045)#30993
Open
bradhallett wants to merge 1 commit into
Open
fix(memory): refuse mutations when on-disk file changed since last read (#26045)#30993bradhallett wants to merge 1 commit into
bradhallett wants to merge 1 commit into
Conversation
…e last read (NousResearch#26045) Add a content-hash guard to the replace, add, and remove mutation paths. Before flushing, the tool re-reads the on-disk file and compares its SHA-256 hash to the snapshot taken at lock-acquire time. If the file was modified externally (concurrent session, patch tool edit, shell append) between those two points, the mutation is refused with a clear error message directing the caller to re-read and retry. This is the same contract the patch tool already enforces ('file was modified since you last read it on disk'). It supplements the existing drift guard (round-trip mismatch / oversized-entry detection) and would alone have prevented the original ~8KB data-loss incident. New tests: - test_replace_refuses_when_file_changed_between_read_and_write - test_replace_succeeds_when_file_unchanged - test_remove_refuses_when_file_changed_between_read_and_write - test_add_refuses_when_file_changed_between_read_and_write - test_on_disk_guard_does_not_trigger_when_file_is_new - test_on_disk_guard_and_drift_guard_are_independent
jsboige
approved these changes
May 23, 2026
jsboige
left a comment
There was a problem hiding this comment.
Reviewed this carefully. The on-disk change guard is well-designed:
What it does: SHA256 snapshot of file content before lock acquisition, then verification before write in add/replace/remove. Refuses mutation if content changed between read and write — prevents data loss from concurrent sessions (#26045).
Strengths:
- Static methods (
_snapshot_on_disk,_check_on_disk_unchanged) — clean separation, testable - Guard fires AFTER drift check — correct ordering (drift is structural, on-disk is temporal)
- 7 tests covering all 3 mutation paths + edge cases (new file, drift vs on-disk independence)
- Error messages reference issue #26045 and instruct user to re-read + retry
One observation (non-blocking): The pre_hash is taken just before _reload_target() which re-reads from disk. If the file changes between _snapshot_on_disk() and _reload_target(), the drift guard would likely catch it (unparseable content → backup). If it changes between _reload_target() and the check, the on-disk guard catches it. This covers the full window — good.
LGTM.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #26045 (commit 6855d17). The drift guard detects structurally un-roundtrippable content, but jrhouston-trilogy recommended an additional defense: refuse mutations when the on-disk file has changed since last read — the same contract the
patchtool already enforces.This alone would have prevented the original ~8KB data-loss incident.
Changes
tools/memory_tool.py: Added SHA-256 content-hash snapshot at lock acquisition. Beforesave_to_disk(), re-reads the file and refuses if the hash differs. Applied to all three mutation paths (add,replace,remove).tests/tools/test_memory_tool.py: 6 new tests inTestOnDiskChangeGuardRelationship to existing drift guard
Both defenses are active and independent:
Test plan
Refs #26045