From cda3d7a5abebfcfc68a2e8133ae630098494fe73 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 22 May 2026 11:23:34 -0400 Subject: [PATCH] fix(3804): worktree.cleanup-wave rescues uncommitted SUMMARY.md (#81) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(3804): rescue uncommitted SUMMARY.md in executeWorktreeWaveCleanupPlan Ports the shell-fallback SUMMARY rescue logic from quick.md into executeWorktreeWaveCleanupPlan. Before the dirty-state check, all *SUMMARY.md files under /.planning/ are copied to the main tree (if absent or divergent), then filtered out of the git-status porcelain output. A worktree whose only dirty file is the executor's uncommitted SUMMARY.md now proceeds to merge+remove instead of returning cleanup_blocked/worktree_dirty. Adds two TDD tests (#3804): - Rescue-only dirty state (SUMMARY.md alone) → cleanup succeeds - SUMMARY + non-SUMMARY dirty files → cleanup still blocks Refs: #2296, #2070, #2838, #3804 Co-Authored-By: Claude Sonnet 4.6 * fix(3804): normalize relPath to forward slashes for Windows porcelain match On Windows, `path.join` produces backslash separators while `git status --porcelain` always emits forward slashes. The rescued-paths Set would never match porcelain output, causing the dirty-check filter to ignore SUMMARY rescue and block cleanup on Windows. Also normalize the test assertion for `rescued[0].dest` to use forward slashes so the test passes on both platforms. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- .../3804-worktree-cleanup-summary-rescue.md | 5 + get-shit-done/bin/lib/worktree-safety.cjs | 119 ++++++++++++++++- tests/worktree-safety.test.cjs | 121 ++++++++++++++++++ 3 files changed, 243 insertions(+), 2 deletions(-) create mode 100644 .changeset/3804-worktree-cleanup-summary-rescue.md diff --git a/.changeset/3804-worktree-cleanup-summary-rescue.md b/.changeset/3804-worktree-cleanup-summary-rescue.md new file mode 100644 index 000000000..6d9e94198 --- /dev/null +++ b/.changeset/3804-worktree-cleanup-summary-rescue.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3804 +--- +**`worktree.cleanup-wave` no longer blocks on executor's uncommitted SUMMARY.md** — `executeWorktreeWaveCleanupPlan` previously returned `cleanup_blocked` / `worktree_dirty` when the executor left `-SUMMARY.md` uncommitted in the worktree's `.planning/` directory (the documented contract — the orchestrator commits it). The fix ports the shell-fallback rescue logic from `quick.md` into the CJS function: before the dirty-state check, all `*SUMMARY.md` files under `/.planning/` are copied to the main tree (if absent or divergent), then filtered out of the porcelain output. Only non-SUMMARY dirty files now block cleanup. (#3804, mirrors #2296/#2070/#2838) diff --git a/get-shit-done/bin/lib/worktree-safety.cjs b/get-shit-done/bin/lib/worktree-safety.cjs index f24828c1f..84537cb2b 100644 --- a/get-shit-done/bin/lib/worktree-safety.cjs +++ b/get-shit-done/bin/lib/worktree-safety.cjs @@ -385,6 +385,96 @@ function gitResultOk(result) { return result && result.exitCode === 0 && !result.timedOut; } +/** + * Walk /.planning/ recursively and collect absolute paths of + * all files whose names match *SUMMARY.md. Returns [] when the directory + * does not exist or cannot be read. + * + * Mirrors the shell fallback in quick.md (#2296, #2070, #2838): + * find "$WT/.planning" -name "*SUMMARY.md" + */ +function defaultFindSummaryFiles(worktreePath) { + const planningDir = path.join(worktreePath, '.planning'); + const results = []; + function walk(dir) { + let entries; + try { entries = fs.readdirSync(dir, { withFileTypes: true }); } catch { return; } + for (const entry of entries) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + walk(full); + } else if (entry.isFile() && entry.name.endsWith('SUMMARY.md')) { + results.push(full); + } + } + } + walk(planningDir); + return results; +} + +/** + * Rescue uncommitted SUMMARY.md artifacts from a worktree into the main repo + * tree before the dirty-state check. Mirrors the shell-fallback rescue block + * in quick.md (lines 878–891, #2296/#2070/#2838). + * + * For each *SUMMARY.md found under /.planning/: + * - compute relative path from worktree root → .planning/-SUMMARY.md + * - 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. + * + * Injected deps (all optional — falls back to real FS): + * findSummaryFiles(worktreePath) → string[] + * existsSync(path) → boolean + * readFileSync(path) → string + * mkdirSync(dir, opts) + * copyFileSync(src, dest) + */ +function rescueSummaryArtifacts(worktreePath, repoRoot, deps) { + const findSummaryFiles = deps.findSummaryFiles || defaultFindSummaryFiles; + const existsSync = deps.existsSync || fs.existsSync; + const readFileSync = deps.readFileSync || ((p) => fs.readFileSync(p, 'utf8')); + const mkdirSync = deps.mkdirSync || ((d, o) => fs.mkdirSync(d, o)); + const copyFileSync = deps.copyFileSync || fs.copyFileSync; + + const summaryPaths = findSummaryFiles(worktreePath); + const rescuedRelPaths = new Set(); + + 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); + if (!needsCopy) { + try { + const srcContent = readFileSync(absPath); + const destContent = readFileSync(dest); + needsCopy = srcContent !== destContent; + } catch { + needsCopy = true; + } + } + if (needsCopy) { + try { + mkdirSync(path.dirname(dest), { recursive: true }); + copyFileSync(absPath, dest); + } catch { + // Best-effort rescue — if it fails the dirty check below will decide fate + } + } + } + + return rescuedRelPaths; +} + function executeWorktreeWaveCleanupPlan(plan, deps = {}) { const execGit = deps.execGit || execGitDefault; const entries = Array.isArray(plan?.entries) ? plan.entries : []; @@ -453,11 +543,36 @@ function executeWorktreeWaveCleanupPlan(plan, deps = {}) { break; } + // 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 worktreeStatus = execGit(['-C', entry.worktree_path, 'status', '--porcelain', '--untracked-files=all'], { cwd: plan.repoRoot }); - if (!gitResultOk(worktreeStatus) || worktreeStatus.stdout) { + if (!gitResultOk(worktreeStatus)) { result.status = 'blocked'; result.reason = 'worktree_dirty'; - result.stderr = worktreeStatus?.stdout || worktreeStatus?.stderr || ''; + result.stderr = worktreeStatus?.stderr || ''; + results.push(result); + pending.push(...entries.slice(i + 1)); + ok = false; + break; + } + // Filter rescued SUMMARY paths out of the porcelain output before deciding dirty. + // A line like "?? .planning/q1-SUMMARY.md" should not block when the SUMMARY + // has already been rescued into the main tree. + const dirtyLines = (worktreeStatus.stdout || '') + .split('\n') + .filter((line) => { + if (!line.trim()) return false; + // porcelain v1 format: "XY path" (3-char prefix + space + path) + const filePath = line.slice(3).trim(); + return !rescuedRelPaths.has(filePath); + }); + if (dirtyLines.length > 0) { + result.status = 'blocked'; + result.reason = 'worktree_dirty'; + result.stderr = dirtyLines.join('\n'); results.push(result); pending.push(...entries.slice(i + 1)); ok = false; diff --git a/tests/worktree-safety.test.cjs b/tests/worktree-safety.test.cjs index da53fc647..d7bebe987 100644 --- a/tests/worktree-safety.test.cjs +++ b/tests/worktree-safety.test.cjs @@ -651,6 +651,127 @@ describe('executeWorktreeWaveCleanupPlan', () => { assert.deepEqual(result.pending.map((entry) => entry.branch), ['worktree-agent-a2']); }); + test('#3804: rescues uncommitted SUMMARY.md from worktree .planning/ before dirty check', () => { + // Fixture: the only dirty file is .planning/q1-SUMMARY.md (executor left it uncommitted + // per documented contract — orchestrator commits it). cleanup-wave MUST rescue it + // (copy to main tree) and succeed, not return worktree_dirty. + 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: '' }; + } + 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: '' }; + } + 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: '' }; + }, + // Inject FS deps so tests don't touch the real filesystem + 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: (_p) => false, + mkdirSync: () => {}, + copyFileSync: (src, dest) => { rescued.push({ src, dest }); }, + }); + + // SUMMARY was rescued into the main tree + assert.equal(rescued.length, 1, 'SUMMARY.md must be rescued (copied) to main tree'); + assert.equal(rescued[0].src, '/repo/.claude/worktrees/agent-a1/.planning/q1-SUMMARY.md'); + // Normalize to forward slashes for cross-platform assertion (path.join uses \ on Windows) + assert.equal(rescued[0].dest.replace(/\\/g, '/'), '/repo/main/.planning/q1-SUMMARY.md'); + + // Cleanup succeeded — SUMMARY-only dirty state must not block + assert.equal(result.ok, true, 'cleanup must succeed when only SUMMARY.md is dirty'); + assert.equal(result.entries[0].status, 'merged_removed'); + assert.equal(result.entries[0].reason, 'ok'); + }); + + test('#3804: still blocks when worktree has non-SUMMARY dirty files alongside SUMMARY', () => { + // If there are OTHER dirty files (not SUMMARY), cleanup must still 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: '' }; + } + 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: '' }; + } + 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('blocks dirty worktrees before merge/remove/delete', () => { const calls = []; const plan = {