From 2726af12460f0692e1e57337f5d59e1dedf720a0 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 2 Jun 2026 16:14:28 -0400 Subject: [PATCH] fix(#245): surface worktree.cleanup-wave SUMMARY rescue copy failure (#616) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#245): surface worktree.cleanup-wave SUMMARY rescue copy failure rescueSummaryArtifacts recorded each path in the rescued set before the copyFileSync attempt; a thrown (and swallowed) copy left the path marked rescued, so the dirty-block filter excluded it and the worktree was merged + removed despite the SUMMARY never being written — silent data loss. Now a path is recorded only after a successful copy (or verified identical dest), and a write failure is surfaced as a blocked entry with reason 'summary_rescue_failed', failing closed instead of removing the worktree. Co-Authored-By: Claude Opus 4.8 * chore(#245): set changeset pr to 616 --------- Co-authored-by: Claude Opus 4.8 --- .changeset/245-summary-rescue-copy-failure.md | 5 ++ src/worktree-safety.cts | 45 ++++++++++--- tests/worktree-safety.test.cjs | 64 +++++++++++++++++++ 3 files changed, 104 insertions(+), 10 deletions(-) create mode 100644 .changeset/245-summary-rescue-copy-failure.md diff --git a/.changeset/245-summary-rescue-copy-failure.md b/.changeset/245-summary-rescue-copy-failure.md new file mode 100644 index 000000000..ad9cedb27 --- /dev/null +++ b/.changeset/245-summary-rescue-copy-failure.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 616 +--- +**`worktree.cleanup-wave` no longer silently loses a SUMMARY.md when the rescue copy fails** — a failed `*SUMMARY.md` rescue now blocks cleanup with `summary_rescue_failed` instead of merging and removing the worktree, preventing silent data loss. diff --git a/src/worktree-safety.cts b/src/worktree-safety.cts index 56d469cde..b61a634a4 100644 --- a/src/worktree-safety.cts +++ b/src/worktree-safety.cts @@ -546,12 +546,20 @@ function defaultFindSummaryFiles(worktreePath: string): string[] { * - destination = / * - copy when dest is absent or content differs * - * Returns a Set of worktree-relative paths (e.g. ".planning/q1-SUMMARY.md") - * that were eligible for rescue (regardless of whether a copy was needed). - * These paths are filtered out of the git-status porcelain output so a - * SUMMARY-only dirty worktree does not block cleanup. + * Returns `{ rescuedRelPaths, failures }`: + * - `rescuedRelPaths`: Set of worktree-relative paths that were successfully rescued + * (copy not needed because dest already matches, or copy succeeded). Only paths + * where the rescue genuinely succeeded are included so the dirty-block filter does + * not suppress paths that were silently lost. + * - `failures`: array of `{ relPath, error }` for any path where mkdirSync or + * copyFileSync threw. A read failure during content comparison is NOT a rescue + * failure — it sets needsCopy=true and the copy is attempted normally. */ -function rescueSummaryArtifacts(worktreePath: string, repoRoot: string, deps: WorktreeDeps): Set { +function rescueSummaryArtifacts( + worktreePath: string, + repoRoot: string, + deps: WorktreeDeps, +): { rescuedRelPaths: Set; failures: Array<{ relPath: string; error: string }> } { const findSummaryFiles = deps.findSummaryFiles || defaultFindSummaryFiles; const existsSync = deps.existsSync || fs.existsSync; const readFileSync = deps.readFileSync || ((p: string) => fs.readFileSync(p, 'utf8')); @@ -560,13 +568,13 @@ function rescueSummaryArtifacts(worktreePath: string, repoRoot: string, deps: Wo const summaryPaths = findSummaryFiles(worktreePath); const rescuedRelPaths = new Set(); + const failures: Array<{ relPath: string; error: string }> = []; for (const absPath of summaryPaths) { // relPath is the path relative to the worktree root (e.g. ".planning/q1-SUMMARY.md") // Normalize to forward slashes so the Set comparison against `git status --porcelain` // output works on Windows too (git always emits forward slashes in porcelain output). const relPath = absPath.slice(worktreePath.length).replace(/^[/\\]/, '').replace(/\\/g, '/'); - rescuedRelPaths.add(relPath); const dest = path.join(repoRoot, relPath); let needsCopy = !existsSync(dest); @@ -576,6 +584,7 @@ function rescueSummaryArtifacts(worktreePath: string, repoRoot: string, deps: Wo const destContent = readFileSync(dest); needsCopy = srcContent !== destContent; } catch { + // Read failure during comparison is not a rescue failure — force a copy attempt. needsCopy = true; } } @@ -583,13 +592,20 @@ function rescueSummaryArtifacts(worktreePath: string, repoRoot: string, deps: Wo try { mkdirSync(path.dirname(dest), { recursive: true }); copyFileSync(absPath, dest); - } catch { - // Best-effort rescue — if it fails the dirty check below will decide fate + // Copy succeeded — the SUMMARY is now safe in the main tree. + rescuedRelPaths.add(relPath); + } catch (err) { + // Write failure: the SUMMARY was NOT rescued. Record it so the caller can + // block cleanup instead of silently losing data. + failures.push({ relPath, error: (err as Error).message }); } + } else { + // dest already exists with identical content — SUMMARY is already safe. + rescuedRelPaths.add(relPath); } } - return rescuedRelPaths; + return { rescuedRelPaths, failures }; } interface WaveCleanupEntryResult extends CleanupManifestEntry { @@ -677,7 +693,16 @@ function executeWorktreeWaveCleanupPlan(plan: WaveCleanupPlan | null, deps: Work // Safety net: rescue uncommitted SUMMARY.md artifacts before the dirty check. // The executor leaves -SUMMARY.md uncommitted by contract — the // orchestrator commits it. Mirrors quick.md shell fallback (#2296, #2070, #2838, #3804). - const rescuedRelPaths = rescueSummaryArtifacts(entry.worktree_path, plan.repoRoot, deps); + const { rescuedRelPaths, failures: rescueFailures } = rescueSummaryArtifacts(entry.worktree_path, plan.repoRoot, deps); + if (rescueFailures.length > 0) { + result.status = 'blocked'; + result.reason = 'summary_rescue_failed'; + result.stderr = rescueFailures.map((f) => `${f.relPath}: ${f.error}`).join('; '); + results.push(result); + pending.push(...entries.slice(i + 1)); + ok = false; + break; + } const worktreeStatus = execGit(['-C', entry.worktree_path, 'status', '--porcelain', '--untracked-files=all'], { cwd: plan.repoRoot }); if (!gitResultOk(worktreeStatus)) { diff --git a/tests/worktree-safety.test.cjs b/tests/worktree-safety.test.cjs index d7bebe987..d2056206c 100644 --- a/tests/worktree-safety.test.cjs +++ b/tests/worktree-safety.test.cjs @@ -772,6 +772,70 @@ describe('executeWorktreeWaveCleanupPlan', () => { assert.equal(result.entries[0].reason, 'worktree_dirty'); }); + test('#245: blocks with summary_rescue_failed when copyFileSync throws during rescue', () => { + // Fixture: the only dirty file is .planning/q1-SUMMARY.md, but copyFileSync throws + // (simulating ENOSPC / permission error). The path must NOT be added to rescuedRelPaths, + // so the entry must be blocked with status='blocked', reason='summary_rescue_failed', + // and the worktree must NOT be merged or removed. + const calls = []; + 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: '' }; + } + 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: '' }; + } + // Any merge or worktree-remove call proves we failed to block — throw to surface it + if (key.startsWith('merge worktree-agent-a1') || key.startsWith('worktree remove')) { + throw new Error(`worktree was not blocked before merge/remove: ${key}`); + } + 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: (p) => { + if (p === '/repo/.claude/worktrees/agent-a1/.planning/q1-SUMMARY.md') return 'summary content'; + return ''; + }, + existsSync: () => false, + mkdirSync: () => {}, + copyFileSync: () => { throw new Error('ENOSPC: no space left on device'); }, + }); + + assert.equal(result.ok, false, 'result.ok must be false when rescue copy fails'); + assert.equal(result.entries[0].status, 'blocked', 'entry status must be blocked'); + assert.equal(result.entries[0].reason, 'summary_rescue_failed', 'entry reason must be summary_rescue_failed'); + // Verify no merge or worktree-remove call was made (the execGit throw above would have surfaced it) + const mergeCalls = calls.filter((c) => c.startsWith('merge worktree-agent-a1') || c.startsWith('worktree remove')); + assert.equal(mergeCalls.length, 0, 'no merge or worktree-remove git call must have been made'); + }); + test('blocks dirty worktrees before merge/remove/delete', () => { const calls = []; const plan = {