fix(3804): worktree.cleanup-wave rescues uncommitted SUMMARY.md (#81)
* 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 <worktree>/.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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
5
.changeset/3804-worktree-cleanup-summary-rescue.md
Normal file
5
.changeset/3804-worktree-cleanup-summary-rescue.md
Normal file
@@ -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 `<quick_id>-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 `<worktree>/.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)
|
||||
@@ -385,6 +385,96 @@ function gitResultOk(result) {
|
||||
return result && result.exitCode === 0 && !result.timedOut;
|
||||
}
|
||||
|
||||
/**
|
||||
* Walk <worktreePath>/.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 <worktreePath>/.planning/:
|
||||
* - compute relative path from worktree root → .planning/<id>-SUMMARY.md
|
||||
* - destination = <repoRoot>/<relPath>
|
||||
* - 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 <quick_id>-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;
|
||||
|
||||
@@ -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 = {
|
||||
|
||||
Reference in New Issue
Block a user