fix: snapshot files in repositories without commits - #1015
fix: snapshot files in repositories without commits#1015ethanhawkes-gif wants to merge 1 commit into
Conversation
will-lamerton
left a comment
There was a problem hiding this comment.
Thanks for this - the diagnosis is right and the error handling degrades safely. I merged it into main locally and confirmed it merges cleanly, typechecks, and passes all 30 tests in file-snapshot.spec.ts. A few things before it can go in.
The fix misses staged files
On an unborn branch we set modifiedOutput = '' and rely only on git ls-files --others --exclude-standard. --others excludes anything in the index, so the fix stops working the moment a user stages. Running the merged code against git init && git add .:
{"files":[],"truncated":false,"available":true}
Zero files, for a repo where the whole tree is staged. git init then git add . is a very common opening move, and git checkout --orphan hits the same path with a fully populated index. That affects checkpoint-manager.ts:125 and both scans in acp-timeline.ts.
git diff handles an unborn HEAD natively, so one word covers it:
modifiedOutput = execSync('git diff --name-only --cached', {...}).trim();The show-ref probe is redundant
rev-parse --verify HEAD already answers "does HEAD exist". The show-ref probe only exists to work out which command in the inner try threw, and it only needs to because git diff is inside that same try. throw headError on line 131 is also thrown inside the try whose own catch (showRefError) then catches and re-inspects it. It happens to rethrow correctly today (both commands exit 128), but only by accident - any git command that ever exits 1 there would be silently treated as an unborn branch.
Moving git diff out removes the whole nested-catch structure:
let hasHead = true;
try {
execSync('git rev-parse --verify HEAD', {cwd: this.workspaceRoot, stdio: ['pipe', 'pipe', 'pipe']});
} catch {
hasHead = false;
}
// An unborn branch has no HEAD to diff against, but its index can still be
// full (`git init && git add .`), so diff the index instead of skipping.
const modifiedOutput = execSync(
hasHead ? 'git diff --name-only HEAD' : 'git diff --name-only --cached',
{cwd: this.workspaceRoot, encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe']},
).trim();A real git diff failure then falls straight to the outer catch, which is the old behaviour the comment says it wants to preserve.
Smaller points
git rev-parse --is-inside-work-treenever checks its output - in a bare repo it printsfalseand exits 0, so the comment overstates what it does. With the simplification above it is fully redundant with the existing error handling, and dropping it takes the per-scan subprocess count back from 4 to 2 (getModifiedFilesResultruns twice per opaque tool call inacp-timeline).- No changeset.
changeset-checkis deliberately non-blocking, so its green tick is not evidence one exists. This is a user-facing fix, so please runpnpm changeset. - The new test is a genuine regression test (it fails without the fix), but please add the staged-file case above, and call
getModifiedFilesResult()so it can assertavailable: true- that flag is whatacp-timeline.tsbranches on for unborn repos. - Nit:
import {execSync} from 'child_process'sits after thefsimport;file-snapshot.tsitself orderschild_processfirst.
|
Hi @ethanhawkes-gif, thanks for this PR! It looks like a codeowner has left feedback Whenever you get a chance, could you take a look at the open comments? |
`getModifiedFilesResult` diffed against HEAD, which does not exist on an unborn branch, so the whole scan fell to its catch and reported git as unavailable. Every file in a freshly `git init`ed workspace looked unmodified and a checkpoint taken there restored nothing. Check for HEAD first and diff the index with `git diff --name-only --cached` when there is none. `git ls-files --others` alone is not enough: it excludes anything already in the index, so `git init && git add .` (and `git checkout --orphan`, which starts with a fully populated index) would still have yielded zero files. A real `git diff` failure now falls straight to the outer catch, which is the pre-existing behaviour, instead of being routed through a nested catch that re-inspected the error's exit status. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d41ba41 to
1c72787
Compare
|
All six points addressed in Index-diff instead of the work-tree probe (1–3). Staged-file test (4). Added alongside the untracked one, asserting Changeset (5) is Full suite: 7520 passed, 1 skipped, 0 failed; On the two red checks — both were stale-base, not this PR. The Semgrep finding is |
Summary
HEADis absent, then collectnon-ignored untracked files with
git ls-files --others --exclude-standardsource/services/file-snapshot.tsand add the regression caseFileSnapshotService returns untracked files from a repository without commitsin
source/services/file-snapshot.spec.ts; the case initializes a temporaryrepository with no commits and verifies root, nested, and ignored files
Test plan
pnpm run test:ava -- source/services/file-snapshot.spec.tspnpm run test:typesCloses #1010