Session checkpoints & safe rewind: content-addressed blobs, atomic restore, file locking#112
Conversation
…store, file locking Module 4 of the runtime-core split. Scope: internal/sessions only. 3-way merged cleanly onto main's #106 Tag/Depth work (preserved); no new deps (filelock uses stdlib syscall); build/vet/-race/windows-cross-compile/full-suite green. - CaptureToolCheckpoint snapshots a tool's target files into content-addressed (sha256) blobs before a mutating tool runs; SnapshotForCheckpoint records file mode; oversize files are skipped, identical content deduped. - RestoreToSequence reverts to a target sequence: applies only the closest-to-target checkpoint per path, verifies each blob's sha256 before writing, preserves file mode, and confines paths with EvalSymlinks (no symlink escape). ApplyRewind runs restore+truncate+prune+marker atomically under one session lock. - Cross-process file lock (flock) serializes session mutations; Fork copies checkpoint blobs so a fork can rewind; writeMetadata uses a unique tmp suffix. FORWARD: this is the persistence layer; the CLI 'zero sessions rewind' and the TUI '/rewind' + pre-tool checkpoint capture wiring land in later modules. Exercised here by checkpoint_test.go / rewind_test.go. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR implements per-session workspace checkpoints with content-addressed (sha256) blobs, gated capture, blob deduplication/integrity checks, orphan-blob pruning, cross-process session file locks, and restore/rewind flows that atomically restore files (with mode preservation) and truncate events. Extensive tests cover integrity, security, concurrency, and fork behavior. ChangesSession Checkpoint and Rewind Flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/sessions/store.go (1)
305-344:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftLock the parent snapshot while forking.
Forkfreezes the parent's event list first and copies whatever blobs still exist later. A concurrent parentApplyRewindcan truncate checkpoint events and prune their blobs in that gap, leaving the fork with copiedEventSessionCheckpointentries that already point at missing content. Snapshot the parent under its session lock and copy only the hashes referenced by that frozen event set before releasing it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sessions/store.go` around lines 305 - 344, The parent session must be locked while snapshotting its events and referenced checkpoint blob hashes to prevent a concurrent ApplyRewind from truncating events and pruning blobs; modify the fork flow in the code around ReadEvents/Create/AppendEvent/copyBlobs so you acquire the parent session lock (using the store's session lock API), read and freeze the events and collect the set of blob hashes referenced by EventSessionCheckpoint events while still holding the lock, then release the lock and proceed to Create the fork, AppendEvent to the fork, and call copyBlobs but only for the frozen set of hashes you captured under the lock (ensuring you reference the frozen event list rather than calling ReadEvents again after releasing the lock).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sessions/checkpoint_test.go`:
- Around line 51-52: The two os.WriteFile calls in
internal/sessions/checkpoint_test.go currently discard errors; change them to
check and fail the test on error (e.g., capture the error returned by
os.WriteFile for filepath.Join(ws, "a.txt") and filepath.Join(ws, "b.txt") and
call t.Fatalf or use require.NoError/require.NoErrorf) so test setup failures
surface instead of silently causing misleading pass/fail behavior.
- Around line 134-137: The test currently ignores the error returned by
os.ReadFile when reading path and then compares the content, which can produce a
misleading failure; update the code in the test (around the os.ReadFile call
that assigns to got) to capture the error (e.g., got, err := os.ReadFile(path)),
check if err != nil and call t.Fatalf with the error (or t.Fatalf("read failed:
%v", err)) before comparing the string content, and only then compare
string(got) to "original" using the existing t.Fatalf on mismatch.
- Around line 174-176: In the test where os.ReadFile(path) is used (same pattern
as TestRestoreToSequenceRevertsFileContent), capture and check the error
returned from os.ReadFile before comparing the contents: call os.ReadFile(path)
into variables (e.g. got, err), if err != nil call t.Fatalf with the error
(including context like "read file" and err) and only then compare string(got)
to "original" to fail with the correct message if content differs; reference the
existing use of os.ReadFile and the path variable in the test to locate where to
add this check.
- Around line 85-88: The test directly indexes p.Files[0] without verifying
p.Files is non-empty; update the Test (around decodeCk and the assertions) to
first check len(p.Files) > 0 and call t.Fatalf (or t.Fatalf with context) if
it's zero, then proceed to assert p.Files[0].Skipped and p.Files[0].Blob;
reference decodeCk and p.Files to locate where to add the bounds check
(mirroring the fix used in TestCaptureRecordsAbsentForNewFile).
- Line 94: The test setup silently ignores errors from the os.WriteFile call;
change the call in the test to check the returned error and fail the test on
error (e.g., capture err := os.WriteFile(...); if err != nil { t.Fatalf("failed
to write a.txt: %v", err) } or use your test helper like require.NoError(t,
err)); update the os.WriteFile invocation in the checkpoint test so setup
failures surface immediately.
- Around line 217-220: The test calls ReadEvents but ignores its error; change
the code to capture and check the error returned by store.ReadEvents (e.g.,
events, err := store.ReadEvents("s")), and if err != nil call t.Fatalf with the
error message (include context like "ReadEvents failed: %v"), then assert
len(events) == 0 as before; this ensures ReadEvents failures cause the test to
fail instead of being silently ignored.
- Line 80: The test currently discards the result of
os.WriteFile(filepath.Join(ws, "big.txt"), []byte("123456"), 0o644); change it
to check and fail on error (e.g. err := os.WriteFile(...); if err != nil {
t.Fatalf("failed to write big.txt: %v", err) } or use require.NoError(t, err))
so the test setup aborts immediately on write failure and does not proceed to
checkpoint a missing file.
- Around line 177-185: The test reads events via store.ReadEvents("s") but
doesn't check the returned error or ensure the slice has at least two elements
before indexing; update the test around the ReadEvents call to check the error
returned by ReadEvents (store.ReadEvents) and t.Fatalf on error, and then verify
len(events) >= 2 before accessing events[len(events)-1] and
events[len(events)-2]; keep the existing assertions for last.Type ==
EventSessionRewind and events[len(events)-2].Sequence == target.Sequence but
fail the test with clear messages if the error is non-nil or the events slice is
too short.
- Around line 114-117: Check the error returned by store.ReadEvents("s") and
fail the test if it is non-nil, and ensure you validate the events slice length
before indexing. Specifically, update the test around the call to ReadEvents to
capture (events, err) := store.ReadEvents("s"), call t.Fatalf or t.Fatal with
the err if err != nil, and then verify len(events) == 2 before accessing
events[len(events)-1] and comparing its Sequence to seqs[1]; reference the
ReadEvents function, the events variable and seqs slice to locate the change.
- Around line 71-74: The test reads p := decodeCk(t, ev) and then indexes
p.Files[0] without checking length; add a bounds check on p.Files (e.g., if
len(p.Files) == 0) before accessing index 0 and call t.Fatalf with a clear
message if it's empty, then proceed to assert p.Files[0].Absent and
p.Files[0].Blob; this change should be made in the failing test function around
the decodeCk usage to prevent panics when the Files slice is empty.
- Line 59: The test currently ignores the error from os.ReadDir when calling
entries, _ := os.ReadDir(store.blobsDir("s")), which can mask failures; update
the call to capture the error (entries, err := os.ReadDir(...)) and
assert/require that err is nil before using len(entries) (e.g., t.Fatalf or
require.NoError on err), referencing the same store.blobsDir("s") call and the
entries variable so the test fails when ReadDir errors instead of incorrectly
passing.
In `@internal/sessions/checkpoint.go`:
- Around line 86-143: SnapshotForCheckpoint currently writes blobs via writeBlob
without holding the session lock, allowing pruneOrphanBlobs or ApplyRewind to
delete those blobs before the caller can append EventSessionCheckpoint; fix by
making the operation atomic: either make SnapshotForCheckpoint unexported and
only call it from inside the session-locked path, or add a new Store method
(e.g. CaptureToolCheckpoint or WriteCheckpointAtomic) that acquires the session
lock, writes blobs (via writeBlob) and immediately appends the
EventSessionCheckpoint under the same lock so no pruneOrphanBlobs/ApplyRewind
race can remove them; update callers to use the new/locked API and remove any
external blob-writing sequences that assume the gap is safe.
- Around line 97-133: SnapshotForCheckpoint currently joins and stats rel
directly, allowing ../ or symlinked paths to escape workspace and treating all
stat failures as Absent; change the loop that builds abs to first resolve and
validate containment: compute abs := filepath.Join(workspaceRoot, rel), then
resolvedAbs := filepath.Clean(abs) and attempt
filepath.EvalSymlinks(resolvedAbs) (fall back to resolvedAbs on EvalSymlinks
errors if you still need to proceed), then ensure the resolved path is inside
workspaceRoot (compare with filepath.Clean(workspaceRoot) and require the
resolved path to have the workspace prefix plus separator); if the resolved path
is outside the workspace mark entry.Skipped and continue; when os.Stat(abs) or
os.ReadFile(abs) fail, map only os.IsNotExist(err) to entry.Absent and treat all
other errors (permission, symlink loop, IO) as entry.Skipped; keep setting
entry.Mode, entry.Skipped, entry.Absent and calling store.writeBlob(sessionID,
content) as before.
In `@internal/sessions/filelock_other.go`:
- Around line 5-8: The current acquireFileLock in Store is a no-op on Windows,
leaving cross-process/session operations unprotected; implement a real OS-level
file lock for Windows in acquireFileLock (and its unlock) so locks serialize
across processes and Store instances: open a dedicated lock file (e.g., based on
sessionID) and use the Windows API via golang.org/x/sys/windows
(LockFileEx/UnlockFileEx or CreateFile with exclusive/shared semantics) to
obtain an exclusive lock, return an unlock function that releases the lock and
closes the handle, and surface any errors from locking; keep the existing
in-memory mutex as well for intra-process safety.
In `@internal/sessions/rewind.go`:
- Around line 76-97: resolveWithinWorkspace currently validates paths but
subsequent os.Remove and store.writeFileAtomic operate on the validated string,
leaving a TOCTOU symlink-swap hole; fix by pinning the workspace root (open
workspaceRoot with os.Open to get a rootFD) and perform file removals and writes
relative to that descriptor using descriptor-based syscalls (e.g. unix.Unlinkat
for deletes and unix.Openat/O_CREAT|O_NOFOLLOW for creates) or by adapting
store.writeFileAtomic to accept a dir FD plus a relative path; change calls in
rewind.go that call os.Remove and store.writeFileAtomic (and any other file ops
after resolveWithinWorkspace like those around lines 113-157 and 253-277) to use
the rootFD + relative paths returned from resolveWithinWorkspace so mutations
cannot escape the pinned workspace.
---
Outside diff comments:
In `@internal/sessions/store.go`:
- Around line 305-344: The parent session must be locked while snapshotting its
events and referenced checkpoint blob hashes to prevent a concurrent ApplyRewind
from truncating events and pruning blobs; modify the fork flow in the code
around ReadEvents/Create/AppendEvent/copyBlobs so you acquire the parent session
lock (using the store's session lock API), read and freeze the events and
collect the set of blob hashes referenced by EventSessionCheckpoint events while
still holding the lock, then release the lock and proceed to Create the fork,
AppendEvent to the fork, and call copyBlobs but only for the frozen set of
hashes you captured under the lock (ensuring you reference the frozen event list
rather than calling ReadEvents again after releasing the lock).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c8b0e861-1360-479d-813b-b0a6726e4711
📒 Files selected for processing (7)
internal/sessions/checkpoint.gointernal/sessions/checkpoint_test.gointernal/sessions/filelock_other.gointernal/sessions/filelock_unix.gointernal/sessions/rewind.gointernal/sessions/rewind_test.gointernal/sessions/store.go
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
What's good
-
Content-addressed checkpoint blobs.
writeBlobstores content under its sha256, andStore.lookupsby hash naturally dedupe identical content.TestCaptureDedupsIdenticalContentconfirms that two files with the same bytes share a single blob. A rewind that captures the same state twice is free. -
Integrity-verified reads.
readBlobre-hashes the bytes on every read and returns an error on mismatch, so a corrupted or tampered blob never gets written as truth.TestReadBlobRejectsCorruptContentcorrupts a blob and assertsreadBloberrors.TestRestoreSkipsCorruptBlobasserts the restore reports the path inSkippedrather than writing attacker-controlled bytes to disk. This is the right threat model for a feature that may run with hostile workspace content. -
Defense-in-depth workspace confinement.
resolveWithinWorkspacerejects both lexical traversal (../) and an in-workspace symlink that points outside the root — itEvalSymlinksthe deepest existing ancestor, re-joins missing segments, and verifies the result is underEvalSymlinks(root).TestRestoreRejectsPathTraversalcovers the lexical case;TestRestoreRejectsInWorkspaceSymlinkEscapecovers the symlink case. Both pass and the outside file survives. The comment explicitly says "an in-workspace symlink that points outside the workspace must not let a restore write outside the root". -
Closest-to-target wins, with no double-count.
RestoreToSequenceiteratessortedCheckpointsAfterfrom oldest to newest, marks each path in arestoredmap, and skips any later entry.TestRestoreDoesNotDoubleCountFilesRestoredconfirmsFilesRestored == 1for two checkpoints on the same path.TestRestoreClosestToTargetSkippedWinsconfirms a closestSkippedentry is NOT overwritten by a newer blob andFilesRestored == 0. -
Per-mode file lock with build-tag split.
filelock_unix.gousesunix.Flock(LOCK_EX)on a per-sessionsession.lock;filelock_other.gois a no-op fallback for Windows.TestSessionFileLockSerializesAcrossStores(skipped on Windows) confirms storeB'sAppendEventblocks while storeA holds the OS lock and proceeds once released. The in-process mutex still serializes within one process, so the cross-process layer is a clean addition. -
Non-reentrant lock pattern with
*Lockedvariants.lockSessionreturns a single unlock closure that releases both the OS lock and the in-process mutex.ApplyRewindholds that lock once and callsrestoreToSequenceLocked/truncateEventsLocked/pruneOrphanBlobsLocked/appendEventLocked— the comments explicitly warn that re-locking would deadlock the non-reentrant in-process mutex.TestApplyRewindDoesNotDeadlockrunsApplyRewindin a goroutine with a 5s timeout and asserts completion. -
Atomic file writes with
Chmodto bypass umask.writeFileAtomicwrites via tmp+rename and thenChmods the tmp file to the exact perm bits captured by the checkpoint. The comment says "WriteFile only applies perm on creation and is subject to umask; force the exact bits so an executable script's mode is faithfully restored".TestRestorePreservesExecutableModewrites a0o755script, captures it, clobbers mode to0o644, and asserts the restored file is0o755with the executable bit set. -
Fork carries blobs, not just events.
copyBlobsruns inStore.Forkand copies every blob from the parent to the fork'sblobsDir.TestForkRewindRestoresFromCopiedBlobsis the regression test: it capturesoriginal, mutates, forks, thenApplyRewindon the fork and asserts the file is back tooriginal(and that every parent blob is present in the fork). The comment names the threat: "Fork must copy content-addressed checkpoint blobs into the fork, not just the checkpoint EVENTS. Otherwise an ApplyRewind on the fork reads a blob that does not exist in the fork's blobs dir and silently skips the file instead of restoring it." -
snapshotOrCapturesplit supports event batching.SnapshotForCheckpointreturns the payload without appending an event;CaptureToolCheckpointwraps it withappendEventLocked. The TUI/agent can callSnapshot(under the tool lock) then append the event after the mutation, so the checkpoint event is recorded next to the mutation it covers, not before it. -
pruneOrphanBlobsis best-effort and lock-aware. PublicpruneOrphanBlobsacquires the session lock; privatepruneOrphanBlobsLockedis the body for callers that already hold it.TestCaptureAndPruneConcurrentruns 20 capture/prune iterations in parallel under-raceand asserts every blob referenced by a surviving event is still on disk. The prune never deletes a blob that a concurrent capture has just referenced. -
writeMetadatanow uses a unique tmp suffix and cleans up on rename failure. The newtmp = fmt.Sprintf("%s.tmp-%d", path, store.idCounter.Add(1))plus_ = os.Remove(tmp)onRenamefailure closes a real leak where the previous fixed.tmpsuffix would collide and the failure path would leave garbage behind. The PR is small but the cleanup is real. -
sessionLocksmap is intentionally non-evicting. The doc comment explains why: removing a mutex while a goroutine is blocked on it would break mutual exclusion, and reference counting would be needed to do it safely. The cost of leaving them is one*sync.Mutexper session id ever touched, which is bounded. The comment makes the trade-off explicit so a future maintainer doesn't "fix" it. -
EventsJSONLintegrity preserved under truncate.truncateEventsLockedreads events, filters to<= keepThroughSequence, marshals back, and writes via tmp+rename. The order is preserved (events are in sequence order) and a leading newline is added only when there is at least one event. -
EventSessionCheckpointis added to the type list. Other types (EventProviderUsage,EventCompaction,EventSessionFork, etc.) follow the samekebab-caseconvention.session_checkpointandsession_rewindare parallel. -
CheckpointsEnabled()andmaxCheckpointBytes()are env-var configurable.ZERO_CHECKPOINTS=offdisables;ZERO_CHECKPOINT_MAX_BYTESoverrides the 5 MiB cap. Both are tested. The default-on behavior matches the description in the body. -
Test coverage is end-to-end and named after the findings.
TestRestoreClosestToTargetSkippedWins,TestRestoreDoesNotDoubleCountFilesRestored,TestReadBlobRejectsCorruptContent,TestRestoreRejectsInWorkspaceSymlinkEscape,TestForkRewindRestoresFromCopiedBlobs,TestRestorePreservesExecutableMode,TestApplyRewindDoesNotDeadlock,TestCaptureAndPruneConcurrent,TestSessionFileLockSerializesAcrossStores— every threat model in the PR body has a focused test. A future maintainer can grep "Finding" or "Audit finding" to see what each test guards. -
Setup helpers fail at the source.
mustWriteFile/mustCapture/mustAppendare documented as setup helpers that fail the test immediately, so a setup error is diagnosed at the line that produced it rather than surfacing as a confusing downstream assertion. Small but real maintainability win.
Observations (non-blocking)
-
writeBlob's tmp suffix uses the per-StoreidCounter, not a session-scoped namespace. ConcurrentwriteBlobcalls for different sessions under one Store could in theory collide on<blobPath>.tmp-<n>. In practice they are serialized by the session lock and the counter is unique per Store, so this is fine — but asessionID+hashprefix in the tmp name would be more bulletproof and would not need a per-Store counter. -
writeFileAtomicusespath.zero-restore-tmp-<n>for the same reason. The Store counter is per-Store not per-session, so two concurrent restores across different sessions under one Store share a counter. Again, the session lock serializes mutations, so it's fine in practice. Same bulletproofing idea applies. -
readBlobhas a TOCTOU window betweenos.ReadFileandsha256.Sum256. In the current design the session lock serializes all mutations, so the window is closed by construction. A real on-disk attacker who can race the read still has the read+hash window. For a rewind that holds the lock, the read+hash is effectively atomic. -
writeFileAtomicusesos.MkdirAllwith0o755. For workspaces that ship with a private root mode (0o700), the restored parent dir could end up more permissive than the rest of the workspace. Probably fine for0o700over0o755is the default for most workspaces, but worth a future consideration if a project ships with0o700. -
acquireFileLockopens withO_RDWR|O_CREATE. Could beO_RDONLY|O_CREATE(flock works on any descriptor). Not a bug; just a small overkill on the open mode. -
Lock order is in-process mutex first, then OS lock. A future code path that needs to acquire the OS lock first could deadlock against the current order. Worth a comment on
lockSession(it already has a comment, but the order is not explicit). The current design is correct; the comment could be clearer. -
Windows
acquireFileLockis a no-op. The doc comment is accurate: "the in-memory per-Store mutex still serializes mutations within a process". Cross-process safety does not apply on Windows. A future PR could addLockFileExon Windows for parity; not a blocker. -
RestoreToSequencewithtargetSeq == 0includes every checkpoint (sinceSequence > 0for all). That's a useful "rewind to origin" semantics; the doc comment could call it out as a feature. -
RestoreReport.Skippedaccumulates both honest (Skipped) and adversarial (traversal) paths. The caller can distinguish viaSkippedcontent. A future enrichment could split the list intoSkippedandRejectedfor cleaner reporting. -
pruneOrphanBlobsis best-effort and returns0, nilon a missing blobs dir. Correct for the case where a session has no checkpoints. A caller that calls prune before any checkpoints will not see an error. -
writeMetadata's unique tmp suffix is a small but real cleanup improvement. The oldpath + ".tmp"would collide if two writers raced; the newpath + ".tmp-" + idCounteris unique. Theos.Remove(tmp)on rename failure is a small leak fix. -
sessionLocksnever evicts. The doc comment explains the trade-off clearly. A future enhancement could add aClose()/Delete()lifecycle hook on the Store and reference-count mutex removal; the comment correctly says this is intentionally out of scope. -
referencedBlobsreads the full event log on every prune. For a session with many events, this is O(n). The session lock holds, so there's no concurrency concern. A future enhancement could maintain an in-memory set of referenced hashes; the current design is correct and simple. -
RestoreToSequenceiteratessortedCheckpointsAfter(newest-first) in reverse to get oldest-first. The iteration is a simplefor i := len-1; i >= 0; i--and the comment explains why. A future refactor could expose aoldestFirstvariant ofsortedCheckpointsAfterdirectly. -
Forkordering: events are copied first, then blobs. A concurrent reader between the two steps would see events pointing at missing blobs. In practice the read path (RestoreToSequence) is itself under the session lock, so this is fine. A comment could call this out. -
No benchmark for
RestoreToSequence/CaptureToolCheckpoint/pruneOrphanBlobs. Each is on a hot path (per-tool-call for capture, per-rewind for restore, per-prune for prune). A benchmark suite would catch future regressions. The current code is correct; the benchmark would be nice-to-have. -
SnapshotForCheckpointandCaptureToolCheckpointare exported. That's the right API for the TUI to call. The split is clean:Snapshotreturns a payload,Captureappends an event. -
EventSessionCheckpointandEventSessionRewindare siblings in the type list. Consistent naming (session_*) and both use kebab-case JSON. -
The PR cleanly handles the
maindrift onTag/Depth. The body says "this was 3-way merged onto the currentmain— #106's Tag/Depth is preserved,store.gomerged with no conflicts". Thestore.godiff shows the newsessionLocksdoc comment and theidCountercleanup addition — neither overwrites #106's fields. Clean merge. -
The forward-code (compiles now, wired later) pattern is preserved. The CLI
zero sessions rewindand TUI/rewind+ pre-tool checkpoint capture are in later modules, per the body. The persistence API is exercised by the unit tests in this PR. That's the right slicing for the runtime-core split.
Approving — this is a substantial persistence layer that closes real audit findings (fork blob copy, deadlock, capture/prune race, executable mode preservation) and finds (closest-to-target semantics, double-count, corrupt blob rejection, in-workspace symlink escape, cross-process file lock). The defense-in-depth path confinement (resolveWithinWorkspace with EvalSymlinks of the deepest existing ancestor) is the right call for a feature that may run with hostile workspace content. The non-reentrant lock pattern with *Locked variants is correct and the deadlock test is the proof. The cross-process flock + per-mode build-tag split is clean. The content-addressed sha256 + read-time integrity check is the right integrity model. The 4 audit findings and 9 numbered findings are all addressed with focused tests. The merge with #106's Tag/Depth drift is clean. The forward-code boundary is clear. The follow-up is the CLI zero sessions rewind and TUI /rewind + pre-tool capture wiring, which land in later modules.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: Request changes.
This is a strong, well-scoped sessions slice and the CI result is healthy. I also ran go test ./internal/sessions locally against the PR worktree and it passed.
Blocker:
internal/sessions/checkpoint.go:97-123snapshotsfilepath.Join(workspaceRoot, rel)directly before any workspace confinement. If a caller passes a traversal path such as../secret.txt, or another non-workspace-relative value, checkpoint capture can read and store content outside the workspace into the session blob store.RestoreToSequencehas good defense-in-depth viaresolveWithinWorkspace, but by then the sensitive content may already be captured. Please validate/resolve every capture target againstworkspaceRootbeforeos.Stat/os.ReadFile, skip invalid targets, and add a regression test for traversal at capture time.
Once capture has the same confinement guarantee as restore, I am comfortable approving this PR. Nice split overall.
… API, Windows lock, test hygiene - [critical/human blocker] snapshotForCheckpoint now confines every capture target with resolveWithinWorkspace (same EvalSymlinks/no-".." guard as restore) BEFORE stat/read, so a traversal/symlink target can't read files outside the workspace into a blob. os.Stat failures are classified correctly: only os.IsNotExist -> Absent (restore deletes); permission/IO/symlink-loop -> Skipped (restore leaves alone), instead of treating every failure as Absent. Regression test added (TestCaptureRejectsTraversalTargets). - [major] SnapshotForCheckpoint unexported -> snapshotForCheckpoint: the blob writes and the referencing EventSessionCheckpoint must be committed atomically under one session lock, which only CaptureToolCheckpoint guarantees. Removes the unsafe snapshot-only external entry point (blob/event gap). - [major] Windows: real LockFileEx/UnlockFileEx (filelock_windows.go) replacing the no-op, so cross-process/cross-Store session serialization holds on Windows too. No new deps (x/sys already required for the unix flock). - [minor] checkpoint_test hygiene: check os.WriteFile/ReadDir/ReadFile/ReadEvents errors via helpers, add bounds checks before p.Files[0]/events[...] so setup failures fail at the source instead of panicking or passing spuriously. - rewind restore: documented the residual symlink-swap TOCTOU (boundary re-resolved immediately before the mutation; full closure needs openat2 RESOLVE_BENEATH / per-component O_NOFOLLOW, tracked for the CLI/TUI rewind-wiring work). build/vet/-race/full-suite + GOOS=windows build all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@Vasanthdev2004 thanks — your blocker is fixed: capture now confines every target with the same @coderabbitai review — also addressed: unexported the snapshot-only API (atomicity is via |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch the PR details and files
gh pr view 112 --repo Gitlawb/zero --json title,state,files,headRefName
# Get the current diff
gh pr diff 112 --repo Gitlawb/zeroLength of output: 56030
1.
|
| Issue | Status |
|---|---|
Capture-side resolveWithinWorkspace confinement |
✅ |
os.Stat failure classification (IsNotExist→Absent, others→Skipped) |
✅ |
TestCaptureRejectsTraversalTargets |
✅ |
snapshotForCheckpoint unexported with atomicity rationale |
✅ |
Real Windows LockFileEx (replaces no-op) |
✅ |
writeMetadata unique tmp suffix + cleanup on rename failure |
✅ |
| Restore-side symlink-swap TOCTOU documented | ✅ |
ApplyRewind deadlock-free (single lock across sub-steps) |
✅ |
| Closest-to-target entry wins; no double-count | ✅ |
Blob integrity check in readBlob |
✅ |
Items 1–2 above are one-line comment fixes; item 3 is an optional clarifying comment. None are blockers if you want to ship as-is, but #1 in particular will confuse the next reader.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: Approved.
Re-reviewed the latest head 600337b. The prior blocker is fixed: checkpoint capture now resolves every target through resolveWithinWorkspace before stat/read, traversal targets are recorded as Skipped rather than captured/absent, and the snapshot-only API is package-private so blob writes stay atomic with the checkpoint event.
Validation:
- Local PR worktree:
go test ./internal/sessionspassed. - Local PR worktree:
go test ./...passed. - GitHub CI: ubuntu/macOS/windows smoke passed, Zero Review passed, Performance Smoke passed.
CodeRabbit was still running at review time, so handle any new actionable comments if it posts them, but I no longer see a blocker from my review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/sessions/rewind.go (2)
57-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail rewind on unreadable checkpoint payloads.
If the checkpoint closest to
targetSeqdoes not decode, thiscontinuelets a newer checkpoint win and can restore a later state than the caller asked for. Treat payload corruption as a hard rewind failure instead of silently skipping it.Suggested fix
var payload CheckpointPayload if err := json.Unmarshal(ev.Payload, &payload); err != nil { - continue + return report, fmt.Errorf("decode checkpoint payload seq %d: %w", ev.Sequence, err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sessions/rewind.go` around lines 57 - 59, The code currently ignores JSON unmarshal errors for CheckpointPayload (var payload CheckpointPayload; json.Unmarshal(ev.Payload, &payload)) and continues, which can let a corrupted checkpoint closer to targetSeq be skipped; instead, treat unreadable checkpoint payloads as a hard rewind failure: when json.Unmarshal on ev.Payload fails, return a descriptive error (including the sequence id/targetSeq and the underlying unmarshal error) from the containing function so the rewind aborts rather than skipping to a newer checkpoint; update the function's error return accordingly and ensure callers propagate/handle that error.
54-71:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDeduplicate restores by the resolved path, not the raw checkpoint string.
restored[f.Path]treats./foo,dir/../foo, or a symlink alias as different entries even whenresolveWithinWorkspacemaps them to the same file. In that case a newer checkpoint can overwrite the closest snapshot, which breaks rewind determinism.Also applies to: 87-90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sessions/rewind.go` around lines 54 - 71, The code currently deduplicates using the raw checkpoint path (restored[f.Path]) which fails to coalesce equivalent paths like "./foo", "dir/../foo" or symlink aliases; change the dedupe key to the workspace-resolved canonical path by calling resolveWithinWorkspace(ctx, f.Path) (or the existing resolver used elsewhere) and use the resolved string for both the existence check and the assignment to restored; if resolution returns an error or empty result, skip that file as before. Apply the same change where restored is consulted/updated later in the function (the other block handling payload.Files around the same logic) so all FilesRestored/FilesDeleted accounting uses the resolved path as the unique identifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/sessions/rewind.go`:
- Around line 57-59: The code currently ignores JSON unmarshal errors for
CheckpointPayload (var payload CheckpointPayload; json.Unmarshal(ev.Payload,
&payload)) and continues, which can let a corrupted checkpoint closer to
targetSeq be skipped; instead, treat unreadable checkpoint payloads as a hard
rewind failure: when json.Unmarshal on ev.Payload fails, return a descriptive
error (including the sequence id/targetSeq and the underlying unmarshal error)
from the containing function so the rewind aborts rather than skipping to a
newer checkpoint; update the function's error return accordingly and ensure
callers propagate/handle that error.
- Around line 54-71: The code currently deduplicates using the raw checkpoint
path (restored[f.Path]) which fails to coalesce equivalent paths like "./foo",
"dir/../foo" or symlink aliases; change the dedupe key to the workspace-resolved
canonical path by calling resolveWithinWorkspace(ctx, f.Path) (or the existing
resolver used elsewhere) and use the resolved string for both the existence
check and the assignment to restored; if resolution returns an error or empty
result, skip that file as before. Apply the same change where restored is
consulted/updated later in the function (the other block handling payload.Files
around the same logic) so all FilesRestored/FilesDeleted accounting uses the
resolved path as the unique identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b3b11dc9-d10a-4b28-8d24-0354d18ff783
📒 Files selected for processing (4)
internal/sessions/checkpoint.gointernal/sessions/checkpoint_test.gointernal/sessions/filelock_windows.gointernal/sessions/rewind.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/sessions/checkpoint_test.go
- internal/sessions/checkpoint.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict update: Request changes.
I approved after verifying the capture-side blocker was fixed, but CodeRabbit finished immediately after and surfaced two new rewind.go issues. I validated them against the current head and they are real correctness blockers for safe rewind:
-
internal/sessions/rewind.go:57-59silently continues when a checkpoint payload cannot be decoded. If the closest-to-target checkpoint is unreadable, rewind may skip it and let a newer checkpoint restore a later state than the caller requested. Please treat invalid checkpoint payloads as a hard rewind error with the checkpoint sequence in the message. -
internal/sessions/rewind.go:68-71dedupes by raw checkpoint path before workspace resolution. Equivalent paths such as./a.txt,dir/../a.txt, or a symlink alias can resolve to the same workspace file but bypass therestoredmap, allowing a newer checkpoint to overwrite the closest snapshot. Please resolve/constrain first, then use the resolved path as the dedupe key, and add a regression test for equivalent raw paths touching the same file.
The previous capture-side workspace escape issue is fixed, and validation is otherwise strong:
- local PR worktree
go test ./internal/sessions: pass - local PR worktree
go test ./...: pass - GitHub ubuntu/macOS/windows smoke, Zero Review, and Performance Smoke: pass
After these two rewind determinism issues are fixed, this should be ready to approve.
[major] restoreToSequenceLocked silently continued past a checkpoint whose payload failed to decode, which let a NEWER checkpoint win for that path and restore a later state than the caller requested. Treat corruption as a hard rewind error. Regression test TestRestoreFailsOnCorruptCheckpointPayload (fails-open before, errors now). Re-review note: the earlier capture-target-validation (critical) and test bounds (minor) were already addressed in 600337b (resolveWithinWorkspace guard + len checks); those re-anchored threads are stale. The restore-side symlink-swap TOCTOU remains a documented heavy-lift residual (needs openat2/descriptor traversal), tracked for the rewind-wiring work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai review — addressed the re-review:
build/vet/-race/full-suite + |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr diff 112 --repo Gitlawb/zeroLength of output: 55982
✅ Confirmed fixed this round
Still open from the prior round (comment-only, no logic change needed)
None of these are blockers; items 1 and 2 are one-line text edits. The implementation is solid — happy to approve once the 🧠 Learnings used |
[major] restoreToSequenceLocked deduped the per-path closest-to-target short-circuit on the RAW checkpoint path, so equivalent paths (./a.txt, dir/../a.txt, a symlink alias) keyed differently and a NEWER checkpoint could overwrite the closest-to-target snapshot for the same underlying file. Now resolve/confine FIRST and dedupe on the resolved workspace path (unresolvable targets dedupe on raw path). Regression test TestRestoreDedupesEquivalentRawPaths. This + the prior commit (24e9ba2, fail-hard on undecodable payload) address both correctness blockers @Vasanthdev2004 raised after CodeRabbit's pass. build/vet/-race/full-suite + GOOS=windows build green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@Vasanthdev2004 both blockers from your re-review are fixed:
build/vet/-race/full-suite + @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr diff 112 --repo Gitlawb/zeroLength of output: 57360
✅ Confirmed fixed this round
The ordering in Still open (comment-only, no logic change needed)These three items are unchanged from the last two rounds — all cosmetic, none are blockers:
The implementation is solid — happy to approve once item 1 is corrected. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: Approved.
Re-reviewed latest head 9898ad3. Both rewind determinism blockers from the previous review are fixed:
- corrupt/undecodable checkpoint payloads now fail the rewind with a sequence-specific error instead of silently continuing to a newer checkpoint;
- restore now resolves/constrains the workspace target first and dedupes on the resolved path, so equivalent raw paths like
a.txtand./a.txtcannot bypass closest-checkpoint-wins behavior.
Regression coverage is present for both cases (TestRestoreFailsOnCorruptCheckpointPayload and TestRestoreDedupesEquivalentRawPaths).
Validation:
- local PR worktree:
go test ./internal/sessionspassed; - local PR worktree:
go test ./...passed; - GitHub checks: ubuntu/macOS/windows smoke passed, Zero Review passed, Performance Smoke passed;
- CodeRabbit latest pass: no actionable comments.
This sessions checkpoint/rewind persistence slice is ready from my side.
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
@coderabbitai resolve All four remaining inline threads are stale re-anchors of the original (586559e) review, already addressed on the current head (9898ad3):
Plus the two correctness fixes from the re-review: hard-fail on undecodable checkpoint payload (24e9ba2) and resolved-path dedupe (9898ad3). @Vasanthdev2004 has re-approved. CI green across smoke/macOS/ubuntu/windows. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# List all review comments by coderabbitai on PR `#112`
gh api repos/Gitlawb/zero/pulls/112/comments --paginate --jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, path: .path, body: .body[0:80]}'Length of output: 2759 🏁 Script executed: #!/bin/bash
# Get the PR node ID and review threads
gh api graphql -f query='
{
repository(owner: "Gitlawb", name: "zero") {
pullRequest(number: 112) {
reviewThreads(first: 100) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
author { login }
body
}
}
}
}
}
}
}'Length of output: 32675 🏁 Script executed: #!/bin/bash
# Resolve the two remaining open threads
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOSqaaUs6Hofmh"}) {
thread { id isResolved }
}
}'
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOSqaaUs6Hofmi"}) {
thread { id isResolved }
}
}'Length of output: 565
The three comment-only items from the prior rounds ( [resolve] 🧠 Learnings used |
|
@CodeRabbit approve |
✅ Action performedComments resolved and changes approved. |
3 participants
Module 4 of N — Session checkpoints & safe rewind
Fourth slice of the runtime-core split. Scope:
internal/sessionsonly (7 files, +1346/−7). Unlike the earlier modules this package had drifted onmain(#106 addedTag/Depth), so this was 3-way merged onto the currentmain— #106's Tag/Depth is preserved,store.gomerged with no conflicts. No new deps (file locking uses stdlibsyscall).What's in it
CaptureToolCheckpointsnapshots a tool's target files into sha256 blobs before a mutating tool runs; records file mode; skips oversize files; dedupes identical content.RestoreToSequencereverts to a target sequence — applies only the closest-to-target checkpoint per path, verifies each blob's sha256 before writing, preserves file mode, and confines paths withEvalSymlinks(no symlink escape).ApplyRewindruns restore + truncate + prune + marker atomically under one session lock.Forkcopies checkpoint blobs so a fork can rewind;writeMetadatauses a unique tmp suffix.Forward code (compiles now, wired later)
This is the persistence layer. The CLI
zero sessions rewindand the TUI/rewind+ pre-tool checkpoint capture wiring land in later modules; the API is exercised here bycheckpoint_test.go/rewind_test.go.Testing
go build ./...,go vet ./...,go test ./...,go test -race ./internal/sessions/, andGOOS=windows go build ./internal/sessions/all green.Part of decomposing #101 (draft); subagents excluded.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Safety
Tests