[#86] Sync → Refuse to overwrite local edits made mid sync#111
Merged
Conversation
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
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.
Resolves #86
Problem
pullorpullDeletewrote over or deleted the local file without re checking it, silently discarding any edit made in that window, exactly the loss the conflict copy machinery exists to preventChanges
changed locally mid sync; sync again to reconcile, the pass fails, andstate.jsonnever advancesWhy
Greptile Summary
This PR closes the silent-data-loss window in #86: a sync pass snapshot was taken once at the start but destructive local writes (
pull,pullDelete, and conflict restores onto locally-deleted paths) executed later without re-verifying the file hadn't changed in the interim. The fix addscheckLocalDrift— a pre-write guard that re-reads and hashes the live file, failing the action when the content no longer matches the snapshot baseline, so the next pass sees both sides changed and routes the path through the existing conflict-copy machinery instead of discarding the edit silently.checkLocalDriftcorrectly distinguishes four states: absent (safe to proceed), unreadable-but-present (fail with the read's own error, never treat as absent), in snapshot with matching hash (safe), and anything else (DRIFT_MESSAGE); the previously flagged hole where apullDeletecould silently delete an unreadable file is closed.takeSnapshotintoexecuteSyncPlanviasync.ts; the same stat-gated-hash baseline the planner uses now drives the drift check, so no extra hashing work is needed for files that haven't changed.conflictwithdeletedSide "none"/"remote"— intentionally omitted because those branches read and preserve whatever local content is currently present before overwriting, so no data can be lost even if the file was edited mid-sync.Confidence Score: 5/5
Safe to merge; the change only adds conservative guards that fail actions rather than silently discard data, and the previously flagged read-error hole in
pullDeleteis explicitly tested and closed.Every destructive local write now re-verifies the file against the snapshot baseline before proceeding. The logic correctly separates the four states (absent, unreadable, hash-matched, drifted), the E2E retry test confirms the conflict-copy machinery takes over on the next pass, and the specific read-error scenario that was flagged in a prior review now has a dedicated regression test.
No files require special attention.
Important Files Changed
checkLocalDriftguard before every destructive local write (pull,pullDelete,conflictwithdeletedSide: "local"). Logic correctly distinguishes absent (safe), unreadable (fail with read error), unknown (DRIFT_MESSAGE), and hash-matched (safe) cases.pull,pullDelete,conflictrestore; read-error-on-present-file is refused; passing snapshot hash matches proceed; previously-flaggedpullDeleteread-error hole is explicitly exercised.fileExiststo theReaderinterface and exportshashBytes, givingcheckLocalDriftthe two primitives it needs.fileExistsseparates "absent" from "unreadable" so read failures on present files are never treated as safe.fileExistsviavault.getFileByPath(path) !== null, consistent with howreadFilealready resolves files; correctly returnsfalsefor paths outside the vault index.localsnapshot intoexecuteSyncPlanso the drift check has a baseline to compare against.fileExiststofakeReaderclosing over the same mutablefilesobject asreadFile, so mid-sync mutations in tests are visible to both methods — correctly mirroring a live filesystem.fakeReaderfixtures to satisfy the expandedReaderinterface; no logic changes.Reviews (2): Last reviewed commit: "Address" | Re-trigger Greptile