fix: make worktree cleanup fail closed (#3385)
* fix: make worktree cleanup fail closed * chore: add changeset for worktree cleanup safety * fix: address worktree cleanup review findings * docs: label remove-workspace failure block * fix: initialize worktree manifest before dispatch
This commit is contained in:
Tom Boucher
59
tests/bug-3384-secondary-defects.test.cjs
Normal file
59
tests/bug-3384-secondary-defects.test.cjs
Normal file
@@ -0,0 +1,59 @@
|
||||
// allow-test-rule: source-text-is-the-product
|
||||
const { describe, test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
|
||||
const repoRoot = path.resolve(__dirname, '..');
|
||||
|
||||
function read(relPath) {
|
||||
return fs.readFileSync(path.join(repoRoot, relPath), 'utf8');
|
||||
}
|
||||
|
||||
describe('bug #3384: adjacent worktree data-loss guards', () => {
|
||||
test('worktree cleanup CLI preserves caller cwd instead of resolving project root', () => {
|
||||
const source = read('get-shit-done/bin/gsd-tools.cjs');
|
||||
const skipSet = source.slice(
|
||||
source.indexOf('const SKIP_ROOT_RESOLUTION = new Set(['),
|
||||
source.indexOf('if (!SKIP_ROOT_RESOLUTION.has(command))'),
|
||||
);
|
||||
|
||||
assert.match(skipSet, /'worktree'/);
|
||||
});
|
||||
|
||||
test('diagnose-issues agents assert disposable worktree branch before reset --hard', () => {
|
||||
const source = read('get-shit-done/workflows/diagnose-issues.md');
|
||||
const branchCheck = source.indexOf('HEAD_REF=$(git symbolic-ref --quiet HEAD || echo');
|
||||
const namespaceCheck = source.indexOf('worktree-agent-* namespace');
|
||||
const reset = source.indexOf('git reset --hard {EXPECTED_BASE}');
|
||||
|
||||
assert.ok(branchCheck > 0, 'diagnose prompt must assert HEAD before repair');
|
||||
assert.ok(namespaceCheck > branchCheck, 'diagnose prompt must require disposable worktree-agent branch');
|
||||
assert.ok(reset > namespaceCheck, 'reset --hard must come only after branch namespace check');
|
||||
});
|
||||
|
||||
test('remove-workspace fails closed when git worktree remove fails', () => {
|
||||
const source = read('get-shit-done/workflows/remove-workspace.md');
|
||||
const init = source.indexOf('REMOVE_FAILED=false');
|
||||
const loop = source.indexOf('For each repo in the workspace');
|
||||
const remove = source.indexOf('git worktree remove "$WORKSPACE_PATH/$REPO_NAME"');
|
||||
|
||||
assert.doesNotMatch(
|
||||
source,
|
||||
/git worktree remove "\$WORKSPACE_PATH\/\$REPO_NAME" 2>&1 \|\| true/,
|
||||
'worktree removal failures must not be swallowed',
|
||||
);
|
||||
assert.ok(init > 0 && init < loop, 'REMOVE_FAILED must initialize once before the per-repo loop');
|
||||
assert.ok(remove > loop, 'worktree removal should remain inside the per-repo loop');
|
||||
assert.match(source, /Refusing to delete "\$WORKSPACE_PATH"/);
|
||||
});
|
||||
|
||||
test('validate health warns when worktree inventory cannot be listed', () => {
|
||||
const source = read('get-shit-done/bin/lib/verify.cjs');
|
||||
const failureBranch = source.indexOf("worktreeHealth.reason === 'git_list_failed'");
|
||||
const warning = source.indexOf("addIssue('warning', 'W020'", failureBranch);
|
||||
|
||||
assert.ok(failureBranch > 0, 'verify health should branch on git_list_failed');
|
||||
assert.ok(warning > failureBranch, 'git_list_failed should emit W020 degraded-health warning');
|
||||
});
|
||||
});
|
||||
235
tests/bug-3384-worktree-cleanup-manifest.test.cjs
Normal file
235
tests/bug-3384-worktree-cleanup-manifest.test.cjs
Normal file
@@ -0,0 +1,235 @@
|
||||
// allow-test-rule: source-text-is-the-product
|
||||
// Workflow markdown is the installed orchestration contract, and the CJS policy
|
||||
// module is the callable safety seam for worktree cleanup.
|
||||
|
||||
'use strict';
|
||||
|
||||
const { describe, test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
|
||||
const {
|
||||
planWorktreeWaveCleanup,
|
||||
executeWorktreeWaveCleanupPlan,
|
||||
} = require('../get-shit-done/bin/lib/worktree-safety.cjs');
|
||||
|
||||
const EXECUTE_PHASE_PATH = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'execute-phase.md');
|
||||
const QUICK_PATH = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'quick.md');
|
||||
|
||||
function readWorkflow(filePath) {
|
||||
return fs.readFileSync(filePath, 'utf8');
|
||||
}
|
||||
|
||||
describe('bug #3384: worktree cleanup is manifest-scoped and fail-closed', () => {
|
||||
test('cleanup plan includes only manifest entries and never discovers global agent worktrees', () => {
|
||||
const plan = planWorktreeWaveCleanup('/repo/main', {
|
||||
worktrees: [
|
||||
{
|
||||
agent_id: 'a1',
|
||||
worktree_path: '/repo/.claude/worktrees/agent-a1',
|
||||
branch: 'worktree-agent-a1',
|
||||
expected_base: 'abc123',
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
assert.equal(plan.ok, true);
|
||||
assert.deepEqual(plan.entries.map((entry) => ({
|
||||
agent_id: entry.agent_id,
|
||||
worktree_path: entry.worktree_path,
|
||||
branch: entry.branch,
|
||||
expected_base: entry.expected_base,
|
||||
})), [{
|
||||
agent_id: 'a1',
|
||||
worktree_path: '/repo/.claude/worktrees/agent-a1',
|
||||
branch: 'worktree-agent-a1',
|
||||
expected_base: 'abc123',
|
||||
}]);
|
||||
assert.equal(plan.discovery, 'manifest');
|
||||
});
|
||||
|
||||
test('cleanup plan rejects entries without expected base or disposable branch namespace', () => {
|
||||
const plan = planWorktreeWaveCleanup('/repo/main', {
|
||||
worktrees: [
|
||||
{
|
||||
agent_id: 'missing-base',
|
||||
worktree_path: '/repo/.claude/worktrees/agent-missing-base',
|
||||
branch: 'worktree-agent-missing-base',
|
||||
},
|
||||
{
|
||||
agent_id: 'feature-branch',
|
||||
worktree_path: '/repo/.claude/worktrees/agent-feature',
|
||||
branch: 'feature/user-work',
|
||||
expected_base: 'abc123',
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
assert.equal(plan.ok, false);
|
||||
assert.equal(plan.reason, 'empty_manifest');
|
||||
assert.deepEqual(plan.entries, []);
|
||||
});
|
||||
|
||||
test('cleanup executor does not delete a branch when worktree removal fails', () => {
|
||||
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: (cwd, args) => {
|
||||
calls.push({ cwd, 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.startsWith('merge worktree-agent-a1')) {
|
||||
return { exitCode: 0, stdout: '', stderr: '' };
|
||||
}
|
||||
if (key === 'worktree remove /repo/.claude/worktrees/agent-a1 --force') {
|
||||
return { exitCode: 1, stdout: '', stderr: 'locked' };
|
||||
}
|
||||
if (key === 'branch -D worktree-agent-a1') {
|
||||
throw new Error('branch deletion must not run after remove failure');
|
||||
}
|
||||
return { exitCode: 0, stdout: '', stderr: '' };
|
||||
},
|
||||
});
|
||||
|
||||
assert.equal(result.ok, false);
|
||||
assert.equal(result.entries[0].status, 'blocked');
|
||||
assert.equal(result.entries[0].reason, 'worktree_remove_failed');
|
||||
assert.equal(calls.some((call) => call.args.join(' ') === 'branch -D worktree-agent-a1'), false);
|
||||
});
|
||||
|
||||
test('cleanup executor stops on merge conflict and records remaining manifest entries', () => {
|
||||
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',
|
||||
},
|
||||
{
|
||||
agent_id: 'a2',
|
||||
worktree_path: '/repo/.claude/worktrees/agent-a2',
|
||||
branch: 'worktree-agent-a2',
|
||||
expected_base: 'abc123',
|
||||
},
|
||||
],
|
||||
};
|
||||
|
||||
const result = executeWorktreeWaveCleanupPlan(plan, {
|
||||
execGit: (_cwd, 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') {
|
||||
return { exitCode: 0, stdout: '', stderr: '' };
|
||||
}
|
||||
if (key.startsWith('merge worktree-agent-a1')) {
|
||||
return { exitCode: 1, stdout: '', stderr: 'CONFLICT' };
|
||||
}
|
||||
throw new Error(`unexpected git call after conflict: ${key}`);
|
||||
},
|
||||
});
|
||||
|
||||
assert.equal(result.ok, false);
|
||||
assert.equal(result.entries[0].status, 'blocked');
|
||||
assert.equal(result.entries[0].reason, 'merge_failed');
|
||||
assert.deepEqual(result.pending.map((entry) => entry.branch), ['worktree-agent-a2']);
|
||||
});
|
||||
|
||||
test('cleanup executor blocks dirty worktrees before merge/remove/delete', () => {
|
||||
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: (_cwd, 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') {
|
||||
return { exitCode: 0, stdout: '?? scratch.txt', stderr: '' };
|
||||
}
|
||||
throw new Error(`unexpected git call after dirty check: ${key}`);
|
||||
},
|
||||
});
|
||||
|
||||
assert.equal(result.ok, false);
|
||||
assert.equal(result.entries[0].reason, 'worktree_dirty');
|
||||
assert.equal(calls.some((call) => call.startsWith('merge worktree-agent-a1')), false);
|
||||
assert.equal(calls.some((call) => call === 'worktree remove /repo/.claude/worktrees/agent-a1 --force'), false);
|
||||
assert.equal(calls.some((call) => call === 'branch -D worktree-agent-a1'), false);
|
||||
});
|
||||
|
||||
test('execute-phase contract requires a cleanup manifest instead of global worktree discovery', () => {
|
||||
const content = readWorkflow(EXECUTE_PHASE_PATH);
|
||||
assert.match(content, /WAVE_WORKTREE_MANIFEST/);
|
||||
assert.match(content, /worktree\.cleanup-wave/);
|
||||
assert.match(content, /atomically append `\{agent_id, worktree_path, branch, expected_base\}`/);
|
||||
assert.match(content, /try\{if\(!p\)throw new Error\("WAVE_WORKTREE_MANIFEST is unset"\)/);
|
||||
assert.match(content, /WT_PATHS_FILE=.*gsd-worktree-paths-/);
|
||||
assert.doesNotMatch(content, /done < <\(node -e 'const fs=require\("fs"\);const p=process\.env\.WAVE_WORKTREE_MANIFEST/);
|
||||
assert.doesNotMatch(content, /done < <\(git worktree list --porcelain \| grep "\^worktree " \| grep "\\\.claude\/worktrees\/agent-"/);
|
||||
});
|
||||
|
||||
test('quick contract requires a cleanup manifest instead of global worktree discovery', () => {
|
||||
const content = readWorkflow(QUICK_PATH);
|
||||
assert.match(content, /WAVE_WORKTREE_MANIFEST|QUICK_WORKTREE_MANIFEST/);
|
||||
assert.match(content, /worktree\.cleanup-wave/);
|
||||
assert.match(content, /mktemp "\$\{TMPDIR:-\/tmp\}\/gsd-quick-worktree-/);
|
||||
assert.match(content, /append its returned `\{agent_id, worktree_path, branch, expected_base\}`/);
|
||||
assert.match(content, /try\{if\(!p\)throw new Error\("QUICK_WORKTREE_MANIFEST is unset"\)/);
|
||||
assert.match(content, /WT_PATHS_FILE=.*gsd-worktree-paths-/);
|
||||
assert.doesNotMatch(content, /done < <\(node -e 'const fs=require\("fs"\);const p=process\.env\.QUICK_WORKTREE_MANIFEST/);
|
||||
assert.doesNotMatch(content, /done < <\(git worktree list --porcelain \| grep "\^worktree " \| grep "\\\.claude\/worktrees\/agent-"/);
|
||||
});
|
||||
});
|
||||
@@ -39,7 +39,13 @@ function extractStep55Block(content) {
|
||||
}
|
||||
|
||||
describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3264)', () => {
|
||||
const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8');
|
||||
function readWorkflow() {
|
||||
try {
|
||||
return fs.readFileSync(WORKFLOW_PATH, 'utf-8');
|
||||
} catch (err) {
|
||||
throw new Error(`failed to read workflow fixture at ${WORKFLOW_PATH}: ${err.message}`);
|
||||
}
|
||||
}
|
||||
|
||||
test('workflow file exists', () => {
|
||||
assert.ok(fs.existsSync(WORKFLOW_PATH), 'workflows/execute-phase.md should exist');
|
||||
@@ -47,11 +53,13 @@ describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3
|
||||
|
||||
test('step 5.5 block exists and is bounded', () => {
|
||||
// extractStep55Block throws on failure — this test validates the helper itself
|
||||
const content = readWorkflow();
|
||||
const block = extractStep55Block(content);
|
||||
assert.ok(block.length > 0, 'step 5.5 block must be non-empty');
|
||||
});
|
||||
|
||||
test('step 5.5 documents the standard wave contract', () => {
|
||||
const content = readWorkflow();
|
||||
const block = extractStep55Block(content);
|
||||
assert.ok(
|
||||
block.includes('Standard wave contract'),
|
||||
@@ -60,6 +68,7 @@ describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3
|
||||
});
|
||||
|
||||
test('step 5.5 names cross-wave dependency deviation as a supported execution mode', () => {
|
||||
const content = readWorkflow();
|
||||
const block = extractStep55Block(content);
|
||||
assert.ok(
|
||||
block.includes('Cross-wave dependency deviation'),
|
||||
@@ -68,6 +77,7 @@ describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3
|
||||
});
|
||||
|
||||
test('cleanup-tail snippet contains git worktree prune', () => {
|
||||
const content = readWorkflow();
|
||||
const block = extractStep55Block(content);
|
||||
assert.ok(
|
||||
block.includes('git worktree prune'),
|
||||
@@ -76,6 +86,7 @@ describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3
|
||||
});
|
||||
|
||||
test('cleanup-tail snippet contains git worktree remove --force', () => {
|
||||
const content = readWorkflow();
|
||||
const block = extractStep55Block(content);
|
||||
assert.ok(
|
||||
block.includes('git worktree remove') && block.includes('--force'),
|
||||
@@ -84,6 +95,7 @@ describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3
|
||||
});
|
||||
|
||||
test('cleanup-tail snippet contains git worktree unlock', () => {
|
||||
const content = readWorkflow();
|
||||
const block = extractStep55Block(content);
|
||||
assert.ok(
|
||||
block.includes('git worktree unlock'),
|
||||
@@ -92,6 +104,7 @@ describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3
|
||||
});
|
||||
|
||||
test('cleanup-tail snippet contains git branch -D', () => {
|
||||
const content = readWorkflow();
|
||||
const block = extractStep55Block(content);
|
||||
assert.ok(
|
||||
block.includes('git branch -D'),
|
||||
@@ -100,6 +113,7 @@ describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3
|
||||
});
|
||||
|
||||
test('skip conditions enumerate empty-WAVE_WORKTREE_PLANS case', () => {
|
||||
const content = readWorkflow();
|
||||
const block = extractStep55Block(content);
|
||||
assert.ok(
|
||||
block.includes('WAVE_WORKTREE_PLANS'),
|
||||
@@ -108,6 +122,7 @@ describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3
|
||||
});
|
||||
|
||||
test('skip conditions enumerate custom-merge-deviation case', () => {
|
||||
const content = readWorkflow();
|
||||
const block = extractStep55Block(content);
|
||||
// The deviation skip condition must reference the cleanup-tail as the alternative
|
||||
assert.ok(
|
||||
@@ -116,25 +131,30 @@ describe('execute-phase step 5.5: cross-wave-deviation cleanup documentation (#3
|
||||
);
|
||||
});
|
||||
|
||||
test('cleanup-tail uses inclusion-based filter for agent namespace', () => {
|
||||
test('cleanup-tail uses wave manifest instead of agent namespace discovery', () => {
|
||||
const content = readWorkflow();
|
||||
const block = extractStep55Block(content);
|
||||
// Must use .claude/worktrees/agent- inclusion filter, not exclusion (per #2774 precedent)
|
||||
assert.ok(
|
||||
block.includes('.claude/worktrees/agent-'),
|
||||
'cleanup-tail must use inclusion-based filter matching .claude/worktrees/agent- namespace',
|
||||
block.includes('WAVE_WORKTREE_MANIFEST'),
|
||||
'cleanup-tail must consume the current wave manifest',
|
||||
);
|
||||
assert.ok(
|
||||
block.includes('avoid touching unrelated active agents'),
|
||||
'cleanup-tail must document why manifest-scoped cleanup is required',
|
||||
);
|
||||
});
|
||||
|
||||
test('cleanup-tail reads git worktree list --porcelain line-by-line', () => {
|
||||
test('cleanup-tail does not rediscover global agent worktrees', () => {
|
||||
const content = readWorkflow();
|
||||
const block = extractStep55Block(content);
|
||||
assert.ok(
|
||||
block.includes('git worktree list --porcelain'),
|
||||
'cleanup-tail must parse git worktree list --porcelain output',
|
||||
assert.doesNotMatch(
|
||||
block,
|
||||
/git worktree list --porcelain.*\.claude\/worktrees\/agent-/s,
|
||||
'cleanup-tail must not parse global git worktree list output for agent worktrees',
|
||||
);
|
||||
// Line-by-line reading requires IFS= read -r pattern
|
||||
assert.ok(
|
||||
block.includes('IFS= read -r'),
|
||||
'cleanup-tail must read line-by-line to preserve paths with whitespace',
|
||||
'cleanup-tail still reads manifest paths line-by-line to preserve paths with whitespace',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user