diff --git a/.changeset/nimble-dogs-purr.md b/.changeset/nimble-dogs-purr.md new file mode 100644 index 000000000..4a9233663 --- /dev/null +++ b/.changeset/nimble-dogs-purr.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 709 +--- +**Worktree wave-cleanup no longer fails when the phase SUMMARY is committed** — `rescueSummaryArtifacts` no longer copies an already-committed SUMMARY into the main checkout, which previously caused `git merge --no-ff` to abort with a permanent `merge_failed` (#706). diff --git a/src/worktree-safety.cts b/src/worktree-safety.cts index b61a634a4..90db1b0d5 100644 --- a/src/worktree-safety.cts +++ b/src/worktree-safety.cts @@ -543,6 +543,12 @@ function defaultFindSummaryFiles(worktreePath: string): string[] { * * For each *SUMMARY.md found under /.planning/: * - compute relative path from worktree root → .planning/-SUMMARY.md + * - if the file is ALREADY COMMITTED on the worktree branch + * (`git cat-file -e HEAD:` returns exit 0), skip the copy entirely: + * the merge will carry it naturally and copying it as an untracked file would + * cause a "untracked working tree files would be overwritten by merge" collision. + * On timeout or fatal exit (128) the rescue is also skipped (fail-closed). + * (#706 — execute-phase committed-SUMMARY contract) * - destination = / * - copy when dest is absent or content differs * @@ -560,6 +566,7 @@ function rescueSummaryArtifacts( repoRoot: string, deps: WorktreeDeps, ): { rescuedRelPaths: Set; failures: Array<{ relPath: string; error: string }> } { + const execGit = deps.execGit || execGitDefault; const findSummaryFiles = deps.findSummaryFiles || defaultFindSummaryFiles; const existsSync = deps.existsSync || fs.existsSync; const readFileSync = deps.readFileSync || ((p: string) => fs.readFileSync(p, 'utf8')); @@ -576,6 +583,31 @@ function rescueSummaryArtifacts( // output works on Windows too (git always emits forward slashes in porcelain output). const relPath = absPath.slice(worktreePath.length).replace(/^[/\\]/, '').replace(/\\/g, '/'); + // #706: skip rescue when the SUMMARY is already committed on the branch. + // Use `git cat-file -e HEAD:` (not `ls-files --error-unmatch`) so + // the check is against the committed tree, not the index. ls-files also + // matches staged-but-uncommitted files, which would skip rescue when the + // file is staged but not yet committed — the merge wouldn't carry it, and + // the executor's content could be lost. cat-file -e HEAD: returns + // exit 0 only when the object exists in the committed HEAD tree. + // + // Fail-closed on timeout/fatal git errors: if we cannot determine whether + // the file is committed, do NOT rescue it (rescuing an actually-committed + // file would re-create the untracked collision; the merge will surface the + // issue). The cleanup will be blocked by merge_failed in the worst case, + // which is the observable behaviour before this fix and is recoverable. + const catFileResult = execGit(['-C', worktreePath, 'cat-file', '-e', `HEAD:${relPath}`], { cwd: repoRoot }); + if (catFileResult.exitCode !== 1) { + // Rescue only when cat-file definitively reports the object is absent (exit 1). + // exit 0 → object exists (committed on HEAD) — merge will carry it, skip. + // exit 128 → fatal git error (corrupt store, unborn HEAD, etc.) — uncertain, + // fail-closed: do NOT rescue to avoid recreating the #706 collision. + // timedOut / null / other → unreliable result — same fail-closed policy. + // In all non-1 cases the merge will either succeed naturally (0) or surface + // the problem safely (128/timeout), which is the recoverable pre-fix behaviour. + continue; + } + const dest = path.join(repoRoot, relPath); let needsCopy = !existsSync(dest); if (!needsCopy) { diff --git a/tests/worktree-safety.test.cjs b/tests/worktree-safety.test.cjs index ba0558e8c..74826e56a 100644 --- a/tests/worktree-safety.test.cjs +++ b/tests/worktree-safety.test.cjs @@ -682,6 +682,10 @@ describe('executeWorktreeWaveCleanupPlan', () => { if (key === 'diff --diff-filter=D --name-only HEAD...worktree-agent-a1') { return { exitCode: 0, stdout: '', stderr: '' }; } + // SUMMARY is NOT committed on the branch — cat-file -e HEAD: returns non-zero + if (key === '-C /repo/.claude/worktrees/agent-a1 cat-file -e HEAD:.planning/q1-SUMMARY.md') { + return { exitCode: 1, stdout: '', stderr: 'error: pathspec \'.planning/q1-SUMMARY.md\' did not match any file(s) known to git' }; + } if (key === '-C /repo/.claude/worktrees/agent-a1 status --porcelain --untracked-files=all') { // Only the SUMMARY is dirty — no other modified files return { exitCode: 0, stdout: '?? .planning/q1-SUMMARY.md', stderr: '' }; @@ -751,6 +755,10 @@ describe('executeWorktreeWaveCleanupPlan', () => { if (key === 'diff --diff-filter=D --name-only HEAD...worktree-agent-a1') { return { exitCode: 0, stdout: '', stderr: '' }; } + // SUMMARY is NOT committed on the branch (uncommitted, per quick.md contract) + if (key === '-C /repo/.claude/worktrees/agent-a1 cat-file -e HEAD:.planning/q1-SUMMARY.md') { + return { exitCode: 1, stdout: '', stderr: 'error: pathspec \'.planning/q1-SUMMARY.md\' did not match any file(s) known to git' }; + } if (key === '-C /repo/.claude/worktrees/agent-a1 status --porcelain --untracked-files=all') { // SUMMARY plus another dirty file return { exitCode: 0, stdout: '?? .planning/q1-SUMMARY.md\nM src/foo.js', stderr: '' }; @@ -803,6 +811,10 @@ describe('executeWorktreeWaveCleanupPlan', () => { if (key === 'diff --diff-filter=D --name-only HEAD...worktree-agent-a1') { return { exitCode: 0, stdout: '', stderr: '' }; } + // SUMMARY is NOT committed on the branch — rescue should proceed (and fail with ENOSPC) + if (key === '-C /repo/.claude/worktrees/agent-a1 cat-file -e HEAD:.planning/q1-SUMMARY.md') { + return { exitCode: 1, stdout: '', stderr: 'error: pathspec \'.planning/q1-SUMMARY.md\' did not match any file(s) known to git' }; + } if (key === '-C /repo/.claude/worktrees/agent-a1 status --porcelain --untracked-files=all') { // Only the SUMMARY is dirty return { exitCode: 0, stdout: '?? .planning/q1-SUMMARY.md', stderr: '' }; @@ -836,6 +848,361 @@ describe('executeWorktreeWaveCleanupPlan', () => { assert.equal(mergeCalls.length, 0, 'no merge or worktree-remove git call must have been made'); }); + test('#706: does NOT rescue SUMMARY when it is already committed on the branch (execute-phase contract)', () => { + // Regression for issue #706: when execute-phase commits SUMMARY.md on the + // worktree branch, the cleanup-wave helper must NOT copy it as an untracked + // file into the main tree. Doing so creates a collision that causes + // `git merge --no-ff` to abort with "untracked working tree files would be + // overwritten by merge". + // + // Fixture: SUMMARY.md is committed on the branch (git cat-file -e HEAD: + // returns exit 0). The worktree status shows the file as committed (not dirty). + // The rescue step must skip this file entirely. The merge must succeed. + const calls = []; + const rescued = []; + const plan = { + ok: true, + repoRoot: '/repo/main', + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ + agent_id: 'a1', + worktree_path: '/repo/.claude/worktrees/agent-a1', + branch: 'worktree-agent-a1', + expected_base: 'abc123', + }], + }; + const result = executeWorktreeWaveCleanupPlan(plan, { + execGit: (args) => { + calls.push(args.join(' ')); + const key = args.join(' '); + if (key === '-C /repo/.claude/worktrees/agent-a1 rev-parse --abbrev-ref HEAD') { + return { exitCode: 0, stdout: 'worktree-agent-a1', stderr: '' }; + } + if (key === 'merge-base HEAD worktree-agent-a1') { + return { exitCode: 0, stdout: 'abc123', stderr: '' }; + } + if (key === 'diff --diff-filter=D --name-only HEAD...worktree-agent-a1') { + return { exitCode: 0, stdout: '', stderr: '' }; + } + // SUMMARY is committed on the branch — cat-file -e HEAD: succeeds (exit 0) + if (key === '-C /repo/.claude/worktrees/agent-a1 cat-file -e HEAD:.planning/q1-SUMMARY.md') { + return { exitCode: 0, stdout: '.planning/q1-SUMMARY.md', stderr: '' }; + } + if (key === '-C /repo/.claude/worktrees/agent-a1 status --porcelain --untracked-files=all') { + // Worktree is clean — SUMMARY is committed, not dirty + return { exitCode: 0, stdout: '', stderr: '' }; + } + if (key.startsWith('merge worktree-agent-a1')) { + return { exitCode: 0, stdout: '', stderr: '' }; + } + if (key === 'worktree remove /repo/.claude/worktrees/agent-a1 --force') { + return { exitCode: 0, stdout: '', stderr: '' }; + } + if (key === 'branch -D worktree-agent-a1') { + return { exitCode: 0, stdout: '', stderr: '' }; + } + return { exitCode: 0, stdout: '', stderr: '' }; + }, + findSummaryFiles: (worktreePath) => { + if (worktreePath === '/repo/.claude/worktrees/agent-a1') { + return ['/repo/.claude/worktrees/agent-a1/.planning/q1-SUMMARY.md']; + } + return []; + }, + readFileSync: () => 'summary content', + existsSync: (_p) => false, + mkdirSync: () => {}, + copyFileSync: (src, dest) => { rescued.push({ src, dest }); }, + }); + + // The committed SUMMARY must NOT have been copied into the main tree as an untracked file + assert.equal(rescued.length, 0, + 'rescueSummaryArtifacts must NOT copy an already-committed SUMMARY into the main tree — ' + + 'doing so creates an untracked file that collides with the --no-ff merge'); + + // Cleanup must succeed + assert.equal(result.ok, true, 'cleanup must succeed when SUMMARY is committed on the branch'); + assert.equal(result.entries[0].status, 'merged_removed'); + assert.equal(result.entries[0].reason, 'ok'); + }); + + test('#706: SUMMARY committed on branch + untracked non-SUMMARY dirty file still blocks', () => { + // Even when SUMMARY is committed (no rescue needed), a non-SUMMARY dirty file must block. + const plan = { + ok: true, + repoRoot: '/repo/main', + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ + agent_id: 'a1', + worktree_path: '/repo/.claude/worktrees/agent-a1', + branch: 'worktree-agent-a1', + expected_base: 'abc123', + }], + }; + const result = executeWorktreeWaveCleanupPlan(plan, { + execGit: (args) => { + const key = args.join(' '); + if (key === '-C /repo/.claude/worktrees/agent-a1 rev-parse --abbrev-ref HEAD') { + return { exitCode: 0, stdout: 'worktree-agent-a1', stderr: '' }; + } + if (key === 'merge-base HEAD worktree-agent-a1') { + return { exitCode: 0, stdout: 'abc123', stderr: '' }; + } + if (key === 'diff --diff-filter=D --name-only HEAD...worktree-agent-a1') { + return { exitCode: 0, stdout: '', stderr: '' }; + } + // SUMMARY is committed on the branch + if (key === '-C /repo/.claude/worktrees/agent-a1 cat-file -e HEAD:.planning/q1-SUMMARY.md') { + return { exitCode: 0, stdout: '.planning/q1-SUMMARY.md', stderr: '' }; + } + if (key === '-C /repo/.claude/worktrees/agent-a1 status --porcelain --untracked-files=all') { + // Another untracked file exists alongside the committed SUMMARY + return { exitCode: 0, stdout: '?? scratch.txt', stderr: '' }; + } + throw new Error(`unexpected git call after dirty check: ${key}`); + }, + findSummaryFiles: (worktreePath) => { + if (worktreePath === '/repo/.claude/worktrees/agent-a1') { + return ['/repo/.claude/worktrees/agent-a1/.planning/q1-SUMMARY.md']; + } + return []; + }, + readFileSync: () => 'summary content', + existsSync: () => false, + mkdirSync: () => {}, + copyFileSync: () => {}, + }); + assert.equal(result.ok, false); + assert.equal(result.entries[0].reason, 'worktree_dirty'); + }); + + test('#706: SUMMARY staged-but-not-committed is rescued (cat-file -e HEAD only matches committed)', () => { + // Codex adversarial finding: git ls-files --error-unmatch would match staged + // files (added to index but not committed), causing rescue to be skipped for + // a file the merge would NOT carry. cat-file -e HEAD: only matches + // committed objects, so staged-but-not-committed SUMMARY is rescued correctly. + // + // Fixture: cat-file -e HEAD: returns exit 1 (not in committed tree), + // but git status shows 'A .planning/q1-SUMMARY.md' (staged). Rescue must + // copy it into the main tree and the cleanup must proceed. + const rescued = []; + const plan = { + ok: true, + repoRoot: '/repo/main', + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ + agent_id: 'a1', + worktree_path: '/repo/.claude/worktrees/agent-a1', + branch: 'worktree-agent-a1', + expected_base: 'abc123', + }], + }; + const result = executeWorktreeWaveCleanupPlan(plan, { + execGit: (args) => { + const key = args.join(' '); + if (key === '-C /repo/.claude/worktrees/agent-a1 rev-parse --abbrev-ref HEAD') { + return { exitCode: 0, stdout: 'worktree-agent-a1', stderr: '' }; + } + if (key === 'merge-base HEAD worktree-agent-a1') { + return { exitCode: 0, stdout: 'abc123', stderr: '' }; + } + if (key === 'diff --diff-filter=D --name-only HEAD...worktree-agent-a1') { + return { exitCode: 0, stdout: '', stderr: '' }; + } + // SUMMARY is staged but NOT committed — cat-file -e HEAD: returns non-zero + if (key === '-C /repo/.claude/worktrees/agent-a1 cat-file -e HEAD:.planning/q1-SUMMARY.md') { + return { exitCode: 1, stdout: '', stderr: 'fatal: Not a valid object name HEAD:.planning/q1-SUMMARY.md' }; + } + if (key === '-C /repo/.claude/worktrees/agent-a1 status --porcelain --untracked-files=all') { + // File is staged ('A .planning/q1-SUMMARY.md') + return { exitCode: 0, stdout: 'A .planning/q1-SUMMARY.md', stderr: '' }; + } + if (key.startsWith('merge worktree-agent-a1')) { + return { exitCode: 0, stdout: '', stderr: '' }; + } + if (key === 'worktree remove /repo/.claude/worktrees/agent-a1 --force') { + return { exitCode: 0, stdout: '', stderr: '' }; + } + if (key === 'branch -D worktree-agent-a1') { + return { exitCode: 0, stdout: '', stderr: '' }; + } + return { exitCode: 0, stdout: '', stderr: '' }; + }, + findSummaryFiles: (worktreePath) => { + if (worktreePath === '/repo/.claude/worktrees/agent-a1') { + return ['/repo/.claude/worktrees/agent-a1/.planning/q1-SUMMARY.md']; + } + return []; + }, + readFileSync: () => 'summary content', + existsSync: () => false, + mkdirSync: () => {}, + copyFileSync: (src, dest) => { rescued.push({ src, dest }); }, + }); + + // The staged-but-not-committed SUMMARY must be rescued into the main tree + assert.equal(rescued.length, 1, + 'staged-but-not-committed SUMMARY must be rescued — cat-file -e HEAD only skips truly committed files'); + // Cleanup succeeded: staged-file shows as 'A ..' which is in rescuedRelPaths filter + assert.equal(result.ok, true, 'cleanup must succeed when only staged SUMMARY is present'); + assert.equal(result.entries[0].status, 'merged_removed'); + }); + + test('#706: cat-file fatal exit 128 causes rescue to be skipped (fail-closed on uncertain git)', () => { + // Finding #1 (code-review): exit code 128 means a fatal git error (e.g. corrupt + // object store, unborn HEAD, missing repo). The guard must treat it as + // "uncertain — cannot determine committed status" and skip rescue, NOT proceed. + // Rescuing when status is uncertain would re-create the #706 merge collision if + // the file is actually already committed. + // + // Fixture: cat-file returns exitCode:128, timedOut:false. + // The cleanup must NOT rescue the SUMMARY (no copy into main tree). + const rescued = []; + const plan = { + ok: true, + repoRoot: '/repo/main', + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ + agent_id: 'a1', + worktree_path: '/repo/.claude/worktrees/agent-a1', + branch: 'worktree-agent-a1', + expected_base: 'abc123', + }], + }; + const result = executeWorktreeWaveCleanupPlan(plan, { + execGit: (args) => { + const key = args.join(' '); + if (key === '-C /repo/.claude/worktrees/agent-a1 rev-parse --abbrev-ref HEAD') { + return { exitCode: 0, stdout: 'worktree-agent-a1', stderr: '' }; + } + if (key === 'merge-base HEAD worktree-agent-a1') { + return { exitCode: 0, stdout: 'abc123', stderr: '' }; + } + if (key === 'diff --diff-filter=D --name-only HEAD...worktree-agent-a1') { + return { exitCode: 0, stdout: '', stderr: '' }; + } + // cat-file returns 128 — fatal git error (e.g. corrupt object store) + if (key === '-C /repo/.claude/worktrees/agent-a1 cat-file -e HEAD:.planning/q1-SUMMARY.md') { + return { exitCode: 128, stdout: '', stderr: 'fatal: not a git repository', timedOut: false }; + } + if (key === '-C /repo/.claude/worktrees/agent-a1 status --porcelain --untracked-files=all') { + // Worktree appears clean (SUMMARY is committed on branch) + return { exitCode: 0, stdout: '', stderr: '' }; + } + if (key.startsWith('merge worktree-agent-a1')) { + return { exitCode: 0, stdout: '', stderr: '' }; + } + if (key === 'worktree remove /repo/.claude/worktrees/agent-a1 --force') { + return { exitCode: 0, stdout: '', stderr: '' }; + } + if (key === 'branch -D worktree-agent-a1') { + return { exitCode: 0, stdout: '', stderr: '' }; + } + return { exitCode: 0, stdout: '', stderr: '' }; + }, + findSummaryFiles: (worktreePath) => { + if (worktreePath === '/repo/.claude/worktrees/agent-a1') { + return ['/repo/.claude/worktrees/agent-a1/.planning/q1-SUMMARY.md']; + } + return []; + }, + readFileSync: () => 'summary content', + existsSync: () => false, + mkdirSync: () => {}, + copyFileSync: (src, dest) => { rescued.push({ src, dest }); }, + }); + + // On fatal exit 128, rescue must be skipped (fail-closed) — no copy into main tree + assert.equal(rescued.length, 0, + 'cat-file exit 128 must NOT rescue the SUMMARY — uncertain git state, skip to avoid recreating #706 collision'); + // The rest of cleanup proceeds normally (merge/remove/delete succeed in this fixture) + assert.equal(result.ok, true, 'cleanup can still succeed when cat-file returns 128 and worktree is clean'); + }); + + test('#706: cat-file timeout causes rescue to be skipped (fail-closed on unreliable git)', () => { + // Codex adversarial finding: on cat-file timeout, rescuing an actually-committed + // file would re-create the untracked collision. The fix treats timeout as + // "cannot determine status — skip rescue" (fail-closed). This means the merge + // will fail with merge_failed, which is the observable pre-fix behaviour and + // is recoverable, rather than silently corrupting the main tree. + // + // Fixture: cat-file returns timedOut:true. The cleanup must NOT rescue the + // SUMMARY (no copy). The merge will then succeed normally (SUMMARY is on the + // branch) or fail safely if the worktree status check catches something. + const rescued = []; + const plan = { + ok: true, + repoRoot: '/repo/main', + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ + agent_id: 'a1', + worktree_path: '/repo/.claude/worktrees/agent-a1', + branch: 'worktree-agent-a1', + expected_base: 'abc123', + }], + }; + const result = executeWorktreeWaveCleanupPlan(plan, { + execGit: (args) => { + const key = args.join(' '); + if (key === '-C /repo/.claude/worktrees/agent-a1 rev-parse --abbrev-ref HEAD') { + return { exitCode: 0, stdout: 'worktree-agent-a1', stderr: '' }; + } + if (key === 'merge-base HEAD worktree-agent-a1') { + return { exitCode: 0, stdout: 'abc123', stderr: '' }; + } + if (key === 'diff --diff-filter=D --name-only HEAD...worktree-agent-a1') { + return { exitCode: 0, stdout: '', stderr: '' }; + } + // cat-file times out — cannot determine if SUMMARY is committed + if (key === '-C /repo/.claude/worktrees/agent-a1 cat-file -e HEAD:.planning/q1-SUMMARY.md') { + return { + exitCode: null, + stdout: '', + stderr: '', + timedOut: true, + signal: 'SIGTERM', + error: Object.assign(new Error('spawnSync git ETIMEDOUT'), { code: 'ETIMEDOUT' }), + }; + } + if (key === '-C /repo/.claude/worktrees/agent-a1 status --porcelain --untracked-files=all') { + // Worktree appears clean (SUMMARY is committed on branch) + return { exitCode: 0, stdout: '', stderr: '' }; + } + if (key.startsWith('merge worktree-agent-a1')) { + return { exitCode: 0, stdout: '', stderr: '' }; + } + if (key === 'worktree remove /repo/.claude/worktrees/agent-a1 --force') { + return { exitCode: 0, stdout: '', stderr: '' }; + } + if (key === 'branch -D worktree-agent-a1') { + return { exitCode: 0, stdout: '', stderr: '' }; + } + return { exitCode: 0, stdout: '', stderr: '' }; + }, + findSummaryFiles: (worktreePath) => { + if (worktreePath === '/repo/.claude/worktrees/agent-a1') { + return ['/repo/.claude/worktrees/agent-a1/.planning/q1-SUMMARY.md']; + } + return []; + }, + readFileSync: () => 'summary content', + existsSync: () => false, + mkdirSync: () => {}, + copyFileSync: (src, dest) => { rescued.push({ src, dest }); }, + }); + + // On timeout, rescue must be skipped (fail-closed) — no copy into main tree + assert.equal(rescued.length, 0, + 'cat-file timeout must NOT rescue the SUMMARY — copying a committed file as untracked would recreate the #706 merge collision'); + // The rest of cleanup proceeds normally (merge/remove/delete succeed in this fixture) + assert.equal(result.ok, true, 'cleanup can still succeed when cat-file times out and worktree is clean'); + }); + test('blocks dirty worktrees before merge/remove/delete', () => { const calls = []; const plan = {