CheckpointPerISC: don't stage a concurrent session's work - #1826
Closed
justinkatz94-glitch wants to merge 1 commit into
Closed
CheckpointPerISC: don't stage a concurrent session's work#1826justinkatz94-glitch wants to merge 1 commit into
justinkatz94-glitch wants to merge 1 commit into
Conversation
The hook already refuses `git add -A` and scopes staging by mtime, and its own
comment names the remaining gap:
"Residual: a file another session dirties mid-run still has a fresh mtime and
can be swept; this closes the common case, not every case."
I hit that residual. A checkpoint commit for one run captured four files written
by a different session running concurrently in the same tree. Content intact,
subject line describing someone else's work, which corrupts the record the
Algorithm treats as authoritative ("the changelog is git").
Root cause is that a timestamp carries no attribution. Two sessions writing one
tree in the same window are indistinguishable by mtime alone.
The attribution is already on disk and was simply never read: every hook receives
`transcript_path`, and a transcript records the file_path of every write tool
call. This adds `foreignClaimedPaths()`, which reads sibling transcripts modified
inside the run window and excludes paths another session provably wrote.
Deliberately exclusion-shaped rather than inclusion-shaped. I tried the obvious
version first (stage only what THIS session's transcript claims) and it was worse:
tested against a real session it recovered 14 of 33 paths, because edits made by
scripts rather than by Write/Edit tool calls are never named in a transcript. That
silently drops work, which beats a wrong subject line in the wrong direction.
Exclusion leaves unattributed files staging on mtime exactly as before.
Verified by symmetry across two real concurrent sessions: each session's hook
stages what it wrote and excludes what the other wrote, in both directions. A
first test run reported both files excluded, which is impossible; the transcript
path was relative while the scan built absolute paths, so self-exclusion silently
failed. Testing one direction could not have caught that.
No behaviour change when transcripts are absent or unreadable: the set is empty
and staging falls through to the existing mtime filter.
Contributor
Author
|
Withdrawing for now to verify CI and re-check against upstream properly before asking for review time. The fix and its test evidence are sound; I would rather resubmit clean. |
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.
The problem
CheckpointPerISCalready refusesgit add -Aand scopes staging by mtime. Its own comment names what is left:I hit that residual. A checkpoint commit for one run captured four files written by a different session running concurrently in the same tree. The content was intact but the subject line described someone else's work, which corrupts the record the Algorithm treats as authoritative ("the changelog is git", and
git log -- <path>is the provenance).Root cause: a timestamp carries no attribution. Two sessions writing one tree in the same window are indistinguishable by mtime alone.
The fix
The attribution is already on disk and was simply never read. Every hook receives
transcript_path, and a transcript records thefile_pathof every write tool call.foreignClaimedPaths()reads sibling transcripts modified inside the run window and excludes paths another session provably wrote. No new hook, no new state file, no new ledger.Why exclusion rather than inclusion
I built the obvious version first: stage only what THIS session's transcript claims. Tested against a real session it recovered 14 of 33 paths, because edits made by scripts rather than by
Write/Edittool calls are never named in a transcript. That silently drops real work, which is worse than a wrong subject line.Exclusion leaves anything unattributed staging on mtime exactly as before. Only another session's provable work is removed, which is precisely the reported defect and nothing more.
Verification
Symmetry across two real concurrent sessions:
Worth flagging: my first test run reported both files excluded, which is impossible. The transcript path was passed relative while the scan built absolute paths, so self-exclusion silently failed and the session's own transcript counted as foreign. Testing one direction could not have caught it, since a design that excludes everything looks identical to one that works if you only check what you expected to be excluded.
Compatibility
No behaviour change when transcripts are absent, unreadable, or yield nothing: the set is empty and staging falls through to the existing mtime filter. The set is computed once per invocation and bounded by filtering to transcripts modified within the run window.
Smoke tested on non-ISA paths, missing ISA paths, and with a real transcript supplied; all exit 0 with
{"continue":true}.