fix(mcp): unique cursor_hash for write/summarize toggles (#238) - #245
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.
Closes #238.
Problem
MemoryWrite/MemorySummarize(viaemitNodeChange) derived a snapshot'scursor_hashfrom pure post-state content (diff.CursorHash(roots), which hashes onlyid:content_hash). Re-writing a node with a previously-used(anchor, payload)reproduced an earlier hash;snapshots.cursor_hashisCHAR(16) NOT NULL UNIQUE, so the INSERT failed and the wholeTxrolled back. Awrite A → write B → write Atoggle broke with an opaqueUNIQUE constraint failed: snapshots.cursor_hash, and the write was lost.Fix
Add
diff.CursorHashForChange(prevHeadID, deltas)which folds the monotonic parent snapshot id (snapshots.idisAUTOINCREMENT, never reused) into the digest, making the hash unique per snapshot even when the content/transition repeats.emitNodeChangenow readsGetHeadSnapshotID(safe under theStore.OpMualready held by both callers) and uses the new hash. This matches howMemoryRollback(CursorHashForRollback) andMemoryForget(CursorHashForDeltas) already avoid the same collision.Kept the
UNIQUEconstraint (no migration);cursor_hashbecomes a per-snapshot token rather than a content digest.Docs
docs/versioning.mdandskills/remind/references/snapshots-diffs.mdcorrected:cursor_hashis an opaque, unique-per-snapshot fingerprint — not a content digest (the prior wording implied identical content reproduces the hash).Tests
pkg/diff/hash_test.go:CursorHashForChangegives distinct hashes for a repeated transition under different parent ids, and is order-stable.pkg/mcp/tools/tools_test.go:TestHandleWrite_TogglePingPong(A→B→A→B→A) andTestHandleSummarize_RevertSucceeds(s1→s2→s1) — both assert one snapshot per call and the correct final content. Both fail on the old code.Verification
go build ./...,go test ./...(1181 passed),go vet,golangci-lint run ./...,make check-skills— all green.Follow-up
#244 tracks the same latent collision class in
MemoryCompile/MemoryForget(content/delta-only hashes that don't fold the parent id; rarer, compile mostly shielded by the zero-delta short-circuit).🤖 Generated with Claude Code