Address #112 review: capture-side confinement, atomic-only checkpoint 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) <[email protected]>

internal/sessions/checkpoint.go

@@ -76,19 +76,23 @@ func (store *Store) CaptureToolCheckpoint(sessionID, workspaceRoot, tool string,
 	}
 	defer unlock()
 
-	payload, ok := store.SnapshotForCheckpoint(sessionID, workspaceRoot, tool, paths)
+	payload, ok := store.snapshotForCheckpoint(sessionID, workspaceRoot, tool, paths)
 	if !ok {
 		return Event{}, nil
 	}
 	return store.appendEventLocked(sessionID, AppendEventInput{Type: EventSessionCheckpoint, Payload: payload})
 }
 
-// SnapshotForCheckpoint reads and stores the before-mutation blobs for paths and
-// returns the checkpoint payload WITHOUT appending an event. Callers that batch
-// session events (e.g. the TUI) snapshot here — before the mutation runs — then
-// append the EventSessionCheckpoint themselves. Returns ok=false when there is
-// nothing to record (disabled, no paths, or no capturable files).
-func (store *Store) SnapshotForCheckpoint(sessionID, workspaceRoot, tool string, paths []string) (CheckpointPayload, bool) {
+// snapshotForCheckpoint reads and stores the before-mutation blobs for paths and
+// returns the checkpoint payload WITHOUT appending an event. It is intentionally
+// package-private: the blob writes and the referencing EventSessionCheckpoint
+// must be committed atomically under the same session lock, which only its sole
+// caller CaptureToolCheckpoint guarantees. A snapshot-only entry point exposed to
+// external callers would let blobs be written that a concurrent
+// pruneOrphanBlobs/ApplyRewind could delete before the event is recorded.
+// Returns ok=false when there is nothing to record (disabled, no paths, or no
+// capturable files).
+func (store *Store) snapshotForCheckpoint(sessionID, workspaceRoot, tool string, paths []string) (CheckpointPayload, bool) {
 	if !CheckpointsEnabled() || len(paths) == 0 {
 		return CheckpointPayload{}, false
 	}
@@ -96,11 +100,26 @@ func (store *Store) SnapshotForCheckpoint(sessionID, workspaceRoot, tool string,
 	files := make([]CheckpointFile, 0, len(paths))
 	for _, rel := range paths {
 		entry := CheckpointFile{Path: rel}
-		abs := filepath.Join(workspaceRoot, rel)
+		// Confine every capture target to the workspace using the SAME guard the
+		// restore path uses (EvalSymlinks-resolved, no "../" escape). A target that
+		// does not resolve inside the workspace is Skipped — never read into a blob,
+		// and never recorded as Absent (which would delete it on rewind).
+		abs, ok := resolveWithinWorkspace(workspaceRoot, rel)
+		if !ok {
+			entry.Skipped = true
+			files = append(files, entry)
+			continue
+		}
 		info, statErr := os.Stat(abs)
 		if statErr != nil {
-			// Treat anything we cannot stat as absent (new file) — restore deletes it.
-			entry.Absent = true
+			if os.IsNotExist(statErr) {
+				// Genuinely new file — restore deletes it.
+				entry.Absent = true
+			} else {
+				// Permission / IO / symlink-loop errors must NOT be rewound as a
+				// delete; record them as skipped so restore leaves the file alone.
+				entry.Skipped = true
+			}
 			files = append(files, entry)
 			continue
 		}

internal/sessions/checkpoint_test.go

@@ -48,15 +48,18 @@ func TestCaptureToolCheckpointWritesBlobAndEvent(t *testing.T) {
 
 func TestCaptureDedupsIdenticalContent(t *testing.T) {
 	store, ws := newCkStore(t)
-	_ = os.WriteFile(filepath.Join(ws, "a.txt"), []byte("same"), 0o644)
-	_ = os.WriteFile(filepath.Join(ws, "b.txt"), []byte("same"), 0o644)
+	mustWriteFile(t, filepath.Join(ws, "a.txt"), "same")
+	mustWriteFile(t, filepath.Join(ws, "b.txt"), "same")
 	if _, err := store.CaptureToolCheckpoint("s", ws, "write_file", []string{"a.txt"}); err != nil {
 		t.Fatal(err)
 	}
 	if _, err := store.CaptureToolCheckpoint("s", ws, "write_file", []string{"b.txt"}); err != nil {
 		t.Fatal(err)
 	}
-	entries, _ := os.ReadDir(store.blobsDir("s"))
+	entries, err := os.ReadDir(store.blobsDir("s"))
+	if err != nil {
+		t.Fatalf("read blobs dir: %v", err)
+	}
 	if len(entries) != 1 {
 		t.Fatalf("expected 1 deduped blob, got %d", len(entries))
 	}
@@ -69,20 +72,49 @@ func TestCaptureRecordsAbsentForNewFile(t *testing.T) {
 		t.Fatal(err)
 	}
 	p := decodeCk(t, ev)
+	if len(p.Files) != 1 {
+		t.Fatalf("expected 1 file entry, got %+v", p.Files)
+	}
 	if !p.Files[0].Absent || p.Files[0].Blob != "" {
 		t.Fatalf("expected absent marker, got %+v", p.Files[0])
 	}
 }
 
+func TestCaptureRejectsTraversalTargets(t *testing.T) {
+	store, ws := newCkStore(t)
+	// A file outside the workspace must never be read into a blob or marked
+	// Absent (which would delete it on rewind). Regression for capture-side
+	// path-traversal confinement.
+	outside := filepath.Join(filepath.Dir(ws), "secret.txt")
+	mustWriteFile(t, outside, "top secret")
+	ev, err := store.CaptureToolCheckpoint("s", ws, "write_file", []string{"../secret.txt"})
+	if err != nil {
+		t.Fatal(err)
+	}
+	p := decodeCk(t, ev)
+	if len(p.Files) != 1 {
+		t.Fatalf("expected 1 file entry, got %+v", p.Files)
+	}
+	if f := p.Files[0]; !f.Skipped || f.Absent || f.Blob != "" {
+		t.Fatalf("traversal target must be skipped (not captured/absent), got %+v", f)
+	}
+	if got, rerr := os.ReadFile(outside); rerr != nil || string(got) != "top secret" {
+		t.Fatalf("outside file must remain untouched: got %q err %v", got, rerr)
+	}
+}
+
 func TestCaptureSkipsOversizeFiles(t *testing.T) {
 	t.Setenv("ZERO_CHECKPOINT_MAX_BYTES", "4")
 	store, ws := newCkStore(t)
-	_ = os.WriteFile(filepath.Join(ws, "big.txt"), []byte("123456"), 0o644)
+	mustWriteFile(t, filepath.Join(ws, "big.txt"), "123456")
 	ev, err := store.CaptureToolCheckpoint("s", ws, "write_file", []string{"big.txt"})
 	if err != nil {
 		t.Fatal(err)
 	}
 	p := decodeCk(t, ev)
+	if len(p.Files) != 1 {
+		t.Fatalf("expected 1 file entry, got %+v", p.Files)
+	}
 	if !p.Files[0].Skipped || p.Files[0].Blob != "" {
 		t.Fatalf("expected skipped, got %+v", p.Files[0])
 	}
@@ -91,7 +123,7 @@ func TestCaptureSkipsOversizeFiles(t *testing.T) {
 func TestCaptureDisabled(t *testing.T) {
 	t.Setenv("ZERO_CHECKPOINTS", "off")
 	store, ws := newCkStore(t)
-	_ = os.WriteFile(filepath.Join(ws, "a.txt"), []byte("v1"), 0o644)
+	mustWriteFile(t, filepath.Join(ws, "a.txt"), "v1")
 	ev, err := store.CaptureToolCheckpoint("s", ws, "write_file", []string{"a.txt"})
 	if err != nil || ev.Type != "" {
 		t.Fatalf("expected no-op when disabled, got ev=%+v err=%v", ev, err)
@@ -111,7 +143,10 @@ func TestTruncateEvents(t *testing.T) {
 	if err := store.TruncateEvents("s", seqs[1]); err != nil {
 		t.Fatal(err)
 	}
-	events, _ := store.ReadEvents("s")
+	events, err := store.ReadEvents("s")
+	if err != nil {
+		t.Fatalf("read events: %v", err)
+	}
 	if len(events) != 2 || events[len(events)-1].Sequence != seqs[1] {
 		t.Fatalf("expected 2 events through seq %d, got %d", seqs[1], len(events))
 	}
@@ -125,13 +160,16 @@ func TestRestoreToSequenceRevertsFileContent(t *testing.T) {
 	mustCapture(t, store, ws, "write_file", "a.txt") // captures "original"
 	mustWriteFile(t, path, "edited1")
 	mustCapture(t, store, ws, "edit_file", "a.txt") // captures "edited1"
-	_ = os.WriteFile(path, []byte("edited2"), 0o644)
+	mustWriteFile(t, path, "edited2")
 
 	report, err := store.RestoreToSequence("s", ws, target.Sequence)
 	if err != nil {
 		t.Fatal(err)
 	}
-	got, _ := os.ReadFile(path)
+	got, err := os.ReadFile(path)
+	if err != nil {
+		t.Fatalf("read restored file: %v", err)
+	}
 	if string(got) != "original" {
 		t.Fatalf("restore should yield 'original', got %q", got)
 	}
@@ -171,10 +209,20 @@ func TestApplyRewindRestoresAndTruncates(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if got, _ := os.ReadFile(path); string(got) != "original" {
+	got, rerr := os.ReadFile(path)
+	if rerr != nil {
+		t.Fatalf("read restored file: %v", rerr)
+	}
+	if string(got) != "original" {
 		t.Fatalf("file not restored: %q", got)
 	}
-	events, _ := store.ReadEvents("s")
+	events, err := store.ReadEvents("s")
+	if err != nil {
+		t.Fatalf("read events: %v", err)
+	}
+	if len(events) < 2 {
+		t.Fatalf("expected >=2 events, got %d", len(events))
+	}
 	last := events[len(events)-1]
 	if last.Type != EventSessionRewind {
 		t.Fatalf("expected trailing rewind marker, got %s", last.Type)
@@ -214,7 +262,10 @@ func TestTruncateToZeroProducesEmptyFile(t *testing.T) {
 	if err := store.TruncateEvents("s", 0); err != nil {
 		t.Fatal(err)
 	}
-	events, _ := store.ReadEvents("s")
+	events, err := store.ReadEvents("s")
+	if err != nil {
+		t.Fatalf("read events: %v", err)
+	}
 	if len(events) != 0 {
 		t.Fatalf("expected 0 events after truncate-to-0, got %d", len(events))
 	}

internal/sessions/filelock_other.go

@@ -1,9 +0,0 @@
-//go:build windows
-
-package sessions
-
-// acquireFileLock is a no-op fallback on platforms without flock; the in-memory
-// per-Store mutex still serializes mutations within a process.
-func (store *Store) acquireFileLock(sessionID string) (func(), error) {
-	return func() {}, nil
-}

internal/sessions/filelock_windows.go

@@ -0,0 +1,35 @@
+//go:build windows
+
+package sessions
+
+import (
+	"fmt"
+	"os"
+
+	"golang.org/x/sys/windows"
+)
+
+// acquireFileLock takes an exclusive OS lock (LockFileEx) on the session's .lock
+// file so concurrent processes — and separate Store instances within one process
+// — sharing the same RootDir serialize their session mutations, matching the
+// flock behavior on unix. It blocks until the lock is available and returns a
+// release function; closing the handle and unlocking the region releases it.
+func (store *Store) acquireFileLock(sessionID string) (func(), error) {
+	path := store.lockPath(sessionID)
+	file, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0o600)
+	if err != nil {
+		return nil, fmt.Errorf("open zero session lock: %w", err)
+	}
+	handle := windows.Handle(file.Fd())
+	overlapped := new(windows.Overlapped)
+	// Lock a fixed 1-byte region. A blocking exclusive lock (no
+	// LOCKFILE_FAIL_IMMEDIATELY) waits until any current holder releases.
+	if err := windows.LockFileEx(handle, windows.LOCKFILE_EXCLUSIVE_LOCK, 0, 1, 0, overlapped); err != nil {
+		_ = file.Close()
+		return nil, fmt.Errorf("lock zero session: %w", err)
+	}
+	return func() {
+		_ = windows.UnlockFileEx(handle, 0, 1, 0, overlapped)
+		_ = file.Close()
+	}, nil
+}

internal/sessions/rewind.go

@@ -72,7 +72,18 @@ func (store *Store) restoreToSequenceLocked(sessionID, workspaceRoot string, tar
 
 			// Defense in depth: never write/delete outside the workspace, even if
 			// a checkpoint event was tampered with (path traversal via "../") or
-			// an in-workspace symlink points outside the root.
+			// an in-workspace symlink points outside the root. The boundary is
+			// re-resolved here, immediately before the mutation, to keep the
+			// check-to-use window small.
+			//
+			// Residual: a symlink-swap TOCTOU remains — a concurrent process with
+			// workspace write access could replace a validated intermediate
+			// directory with a symlink between this check and the
+			// os.Remove/writeFileAtomic below. Fully closing it needs
+			// descriptor-based traversal (openat2 RESOLVE_BENEATH on Linux /
+			// per-component O_NOFOLLOW), which is platform-specific; tracked for
+			// the CLI/TUI rewind-wiring work. The narrow window plus the
+			// workspace-write-access precondition make this low-risk here.
 			abs, ok := resolveWithinWorkspace(workspaceRoot, f.Path)
 			if !ok {
 				report.Skipped = append(report.Skipped, f.Path)