[tisankanj]

Closes #1148.

Reachability, stated up front

This is defense-in-depth, not a live bug. No production caller passes modifiedFiles, and the git-derived list (git diff --name-only HEAD run with cwd: workspaceRoot) cannot produce a .. path. It is reachable only through the optional parameter. An earlier version of this description implied otherwise, which was wrong.

The gap

Snapshot keys are the file's path relative to the workspace:

const relativePath = path.relative(this.workspaceRoot, absolutePath);
const normalizedPath = relativePath.split(path.sep).join('/');

captureFiles never required the file to be inside the workspace, so a file above it is keyed ../name. Joined onto a checkpoint's files directory, that writes outside the checkpoint.

file-snapshot.ts already refused this on the way back out, in restoreFiles and deleteFile. Only the capture side was open.

The change

The rule now has one home, isInsideWorkspace, and three callers instead of two open-coded copies:

• capture drops the path and reports it through skipped, so it reaches the skippedFiles metadata and an incomplete checkpoint still says so at restore time.
• restore and delete keep their existing throw, unchanged in behaviour.

Capture reports rather than throws because it is best-effort, which is how the cap and unreadable-file cases already behave.

Test plan

Two tests in file-snapshot.spec.ts. Confirmed the first fails without the guard:

captureFiles refuses a path outside the workspace and reports it as skipped
  the file must not be captured
  Difference (- actual, + expected):
  - 1
  + 0

That 1 is the outside-workspace file being captured. With the guard, file-snapshot.spec.ts passes 39 and checkpoint-manager.spec.ts passes 33, which covers the restore and delete paths the shared rule now serves. tsc --noEmit and biome check are clean.

Three cli-integration tests fail in the full suite. They fail identically on upstream/main with this branch's changes stashed, re-checked against the current base.

Not included

timeline-manager.ts reads entry.filesChanged off the on-disk index and joins it onto filesDir without revalidating, which is the same argument. Left as a follow-up as suggested rather than widening this PR.

[tisankanj]

The red check is Package Audit, and it is a registry timeout rather than an advisory:

POST https://registry.npmjs.org/-/npm/v1/security/advisories/bulk error (23). Will retry in 10 seconds.
TimeoutError: The operation was aborted due to timeout
##[error]Process completed with exit code 1

This branch changes two source files and adds a changeset, no dependency or lockfile changes, so there is nothing here for pnpm audit to have found. Every other check passes, including unit tests, type checks, lint, format, Semgrep and CodeQL. A re-run should clear it; I cannot trigger one from a fork.

[will-lamerton]

Thanks for this - the gap is real and still unfixed on main, and the guard itself is correct (normalizes first, handles Windows separators, rejects absolute). A few things before it can land.

Needs a rebase. The branch is conflicting. Main has since landed the binary-snapshot fix, which rewrote the exact lines this touches: captureFiles now returns {snapshots, skipped} instead of a bare Map, contents are Buffer rather than string, and saveCheckpoint records skippedFiles / truncatedFileCount. The save-path hunk won't apply or compile as written, and the green CI here is from the old base.

Put the guard in captureFiles instead. FileSnapshotService.captureFiles is the single place these keys are minted, it already normalizes separators, and file-snapshot.ts already owns this identical rule twice (restoreFiles, deleteFile). Guarding there covers both consumers at once rather than adding a third copy of the rule in a second class. timeline-manager.ts only avoids the same bug on save because toRelativePath open-codes the same check.

Fold dropped paths into skipped. A path the guard filters out is only logged, so it never reaches the skippedFiles metadata that the binary-snapshot change added specifically so an incomplete checkpoint says so at restore time. It should be reported the same way.

Also worth doing: timeline-manager.ts reads entry.filesChanged off the on-disk index and joins it onto filesDir with no revalidation - the same "metadata.json is on-disk state" argument you make for loadCheckpoint. Happy for that to be a follow-up, but it shouldn't be left behind.

One note on framing: no production caller passes modifiedFiles, and the git-derived list (git diff --name-only HEAD with cwd: workspaceRoot) can't produce a .. path, so this is defense-in-depth on an optional parameter rather than something currently triggerable. Still worth having, but the description reads as if it were live.

Tests look reasonable otherwise - they'll need the Buffer and destructuring updates after the rebase.

[tisankanj]

All taken, rebased and reworked in ef15e4f. Point by point.

Rebase. Done onto current main. Rather than resolve a conflict in checkpoint-manager.ts for code your second point removes anyway, I reset the branch onto main and rebuilt it in the new place. The old checkpoint-manager.ts hunk is gone entirely, so the Buffer and {snapshots, skipped} changes are simply the base now rather than something I patched around.

Guard in captureFiles. You are right, and it is better than what I had. file-snapshot.ts was open-coding the identical rule twice, so I extracted isInsideWorkspace and pointed all three at it. That is one home and three callers, instead of two copies plus a third in another class:

private isInsideWorkspace(absolutePath: string): boolean {
  const relative = path.relative(this.workspaceRoot, absolutePath);
  return !relative.startsWith('..') && !path.isAbsolute(relative);
}

Deliberately byte-equivalent to what restoreFiles and deleteFile already did, so their behaviour is unchanged. checkpoint-manager.spec.ts still passes 33, which is what covers those two.

Fold into skipped. Done:

skipped.push({path: normalizedPath, reason: 'Outside the workspace'});

Reported rather than thrown, matching how the cap and unreadable files already behave on that path, so it lands in skippedFiles and an incomplete checkpoint says so at restore time.

timeline-manager.ts. Agreed it is the same argument, and I have left it as the follow-up you suggested rather than widening this PR. Noted in the description so it is not lost.

Framing. Fair, and my description was overstated. I have rewritten it to open with the reachability: no production caller passes modifiedFiles, and git diff --name-only HEAD with cwd: workspaceRoot cannot yield a .. path, so this is defense-in-depth on an optional parameter. That should have been stated the first time.

Tests moved to file-snapshot.spec.ts with the Buffer and destructuring updates. Confirmed the guard is what makes them pass:

captureFiles refuses a path outside the workspace and reports it as skipped
  the file must not be captured
  - 1
  + 0

39 pass in file-snapshot.spec.ts, 33 in checkpoint-manager.spec.ts, tsc and biome clean. The three cli-integration failures reproduce identically on main with this branch stashed, re-checked against the new base.

[will-lamerton]

All four points addressed, and the rework is better than what I asked for. Extracting isInsideWorkspace and pointing all three call sites at it removes the two open-coded copies that were already there, so the rule now has one home instead of gaining a third. Keeping it byte-equivalent to what restoreFiles and deleteFile did was the right call for a change that is meant to be behaviour-neutral on those two paths.

I verified against a merge with current main rather than this branch's base, since it is a good way behind. main has since rewritten getModifiedFilesResult in this same file, but in lines this PR does not touch, so the merge is clean and only the three intended files land. On that merged tree: file-snapshot.spec.ts 39 and checkpoint-manager.spec.ts 33, tsc --noEmit and biome check clean. I also removed just the new guard and re-ran, and captureFiles refuses a path outside the workspace fails, so the test is load-bearing rather than passing either way. filesChanged is built from fileSnapshots.keys(), so a refused path never reaches it and restore will not try to write ../x; the drop surfaces through skippedFiles into describeCheckpointGaps, which is what the third point was after.

I pushed one commit rather than send it back for a round trip. The new method landed between captureFiles' docblock and captureFiles itself, which swapped the two comments: hovering captureFiles showed the containment rule and hovering isInsideWorkspace showed the read-as-bytes explanation. Moved the method above the docblock. No behaviour change.

One thing for the follow-up, alongside timeline-manager.ts. The shared predicate uses relative.startsWith('..'), which false-positives on a file legitimately named ..hidden.txt at the workspace root. I confirmed it: that file is now dropped and reported as ..hidden.txt (Outside the workspace). Not a regression, since restoreFiles had the same bug and it was worse there (one such file made the whole restore throw), so moving the decision to capture time is already an improvement. But now that the rule lives in one place, relative === '..' || relative.startsWith('..' + path.sep) fixes it everywhere at once, which is exactly the payoff of having extracted it.

Thanks for the careful rework, and for correcting the framing in the description.