golem15/msd-core
4
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(#588): consolidate duplicated worktree_branch_check into one canonical fragment (#589)

Browse Source 
Extracts the fail-closed worktree branch-check guard into a single canonical fragment (get-shit-done/references/worktree-branch-check.md) and repoints all five sites at it; orchestrator embeds the runnable block at dispatch. All safety invariants preserved; adversarially reviewed; full matrix green. Closes #588.
This commit is contained in:
Tom Boucher
2026-06-01 18:08:10 -04:00
committed by GitHub
parent f069331737
commit b88d6d6ef1
12 changed files with 222 additions and 126 deletions
Download Patch File Download Diff File Expand all files Collapse all files

25
tests/bug-3384-secondary-defects.test.cjs
View File

@@ -5,6 +5,7 @@ const fs = require('node:fs');
const path = require('node:path');
const repoRoot = path.resolve(__dirname, '..');
const WORKTREE_BRANCH_CHECK_FRAGMENT = path.join(repoRoot, 'get-shit-done', 'references', 'worktree-branch-check.md');
function read(relPath) {
return fs.readFileSync(path.join(repoRoot, relPath), 'utf8');
@@ -22,14 +23,24 @@ describe('bug #3384: adjacent worktree data-loss guards', () => {
});
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}');
// diagnose-issues.md now references the canonical fragment rather than
// inlining the block. Verify (a) it references the fragment and (b) the
// fragment itself has the correct ordering: symbolic-ref/HEAD assertion and
// ^worktree-agent- allow-list appear BEFORE git reset --hard.
const diagnoseSource = read('get-shit-done/workflows/diagnose-issues.md');
assert.ok(
diagnoseSource.includes('worktree-branch-check.md'),
'diagnose-issues.md must reference the canonical worktree-branch-check.md fragment'
);
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');
const fragmentSource = fs.readFileSync(WORKTREE_BRANCH_CHECK_FRAGMENT, 'utf8');
const branchCheck = fragmentSource.indexOf('HEAD_REF=$(git symbolic-ref --quiet HEAD || echo');
const namespaceCheck = fragmentSource.indexOf('^worktree-agent-');
const reset = fragmentSource.indexOf('git reset --hard {EXPECTED_BASE}');
assert.ok(branchCheck > 0, 'canonical fragment must assert HEAD before repair');
assert.ok(namespaceCheck > branchCheck, 'canonical fragment must require disposable worktree-agent branch before reset');
assert.ok(reset > namespaceCheck, 'reset --hard must come only after branch namespace check in canonical fragment');
});
test('remove-workspace fails closed when git worktree remove fails', () => {

60
tests/worktree-cleanup.test.cjs
View File

@@ -33,6 +33,7 @@ const EXECUTE_PLAN_PATH = path.join(REPO_ROOT, 'get-shit-done', 'workflows', 'ex
const QUICK_PATH = path.join(REPO_ROOT, 'get-shit-done', 'workflows', 'quick.md');
const EXECUTOR_AGENT_PATH = path.join(REPO_ROOT, 'agents', 'gsd-executor.md');
const GIT_INTEGRATION_PATH = path.join(REPO_ROOT, 'get-shit-done', 'references', 'git-integration.md');
const WORKTREE_BRANCH_CHECK_FRAGMENT = path.join(REPO_ROOT, 'get-shit-done', 'references', 'worktree-branch-check.md');
// ─── Helpers ──────────────────────────────────────────────────────────────────
@@ -118,11 +119,19 @@ function findCommandIndex(statements, predicate) {
describe('bug #2924: worktree HEAD attachment + destructive recovery', () => {
describe('execute-phase.md worktree_branch_check', () => {
const content = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf-8');
const block = extractNamedBlock(content, 'worktree_branch_check');
const executePhaseContent = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf-8');
const fragmentContent = fs.readFileSync(WORKTREE_BRANCH_CHECK_FRAGMENT, 'utf-8');
const block = extractNamedBlock(fragmentContent, 'worktree_branch_check');
test('block exists', () => {
assert.ok(block, 'execute-phase.md must contain a <worktree_branch_check> block');
test('execute-phase.md references the canonical fragment', () => {
assert.ok(
executePhaseContent.includes('worktree-branch-check.md'),
'execute-phase.md must reference the canonical worktree-branch-check.md fragment'
);
});
test('block exists in canonical fragment', () => {
assert.ok(block, 'worktree-branch-check.md must contain a <worktree_branch_check> block');
});
test('block invokes `git symbolic-ref` to inspect HEAD attachment', () => {
@@ -269,18 +278,26 @@ describe('bug #2924: worktree HEAD attachment + destructive recovery', () => {
});
describe('quick.md worktree_branch_check', () => {
const content = fs.readFileSync(QUICK_PATH, 'utf-8');
const block = extractNamedBlock(content, 'worktree_branch_check');
const quickContent = fs.readFileSync(QUICK_PATH, 'utf-8');
const fragmentContent = fs.readFileSync(WORKTREE_BRANCH_CHECK_FRAGMENT, 'utf-8');
const block = extractNamedBlock(fragmentContent, 'worktree_branch_check');
test('block exists', () => {
assert.ok(block, 'quick.md must contain a <worktree_branch_check> block');
test('quick.md references the canonical fragment', () => {
assert.ok(
quickContent.includes('worktree-branch-check.md'),
'quick.md must reference the canonical worktree-branch-check.md fragment'
);
});
test('block exists in canonical fragment', () => {
assert.ok(block, 'worktree-branch-check.md must contain a <worktree_branch_check> block');
});
test('block references `git symbolic-ref` for HEAD attachment assertion', () => {
// quick.md uses inline `git symbolic-ref ... HEAD` rather than a fenced
// block, so search the block as a token stream of statements.
const statements = shellStatements(block);
const idx = findCommandIndex(statements, (cmd) =>
// Search the block from the canonical fragment as a token stream of statements.
const codeBlocks = extractFencedCodeBlocks(block);
const allStatements = codeBlocks.flatMap(({ body }) => shellStatements(body));
const idx = findCommandIndex(allStatements, (cmd) =>
cmd[0] === 'git' && cmd[1] === 'symbolic-ref' && cmd.includes('HEAD')
);
assert.notStrictEqual(
@@ -290,12 +307,20 @@ describe('bug #2924: worktree HEAD attachment + destructive recovery', () => {
});
test('HEAD assertion precedes `git reset --hard`', () => {
const symbolicRefByteIdx = block.indexOf('symbolic-ref');
const resetHardByteIdx = block.indexOf('reset --hard');
assert.notStrictEqual(symbolicRefByteIdx, -1);
assert.notStrictEqual(resetHardByteIdx, -1);
// Use shell-statement ordering on the fenced code block to avoid false matches
// from the preamble text that mentions `git reset --hard` for context.
const codeBlocks = extractFencedCodeBlocks(block);
const allStatements = codeBlocks.flatMap(({ body }) => shellStatements(body));
const symbolicRefIdx = findCommandIndex(allStatements, (cmd) =>
cmd[0] === 'git' && cmd[1] === 'symbolic-ref' && cmd.includes('HEAD')
);
const resetHardIdx = findCommandIndex(allStatements, (cmd) =>
cmd[0] === 'git' && cmd[1] === 'reset' && cmd.includes('--hard')
);
assert.notStrictEqual(symbolicRefIdx, -1, 'symbolic-ref check must exist');
assert.notStrictEqual(resetHardIdx, -1, 'reset --hard must exist');
assert.ok(
symbolicRefByteIdx < resetHardByteIdx,
symbolicRefIdx < resetHardIdx,
'symbolic-ref HEAD assertion must appear before `git reset --hard` in quick.md worktree_branch_check'
);
});
@@ -582,6 +607,7 @@ describe('worktree commit safety hardening (#1977)', () => {
test('execute-plan worktree_branch_check has no Windows-only platform qualifier', () => {
const content = fs.readFileSync(EXECUTE_PLAN_PATH, 'utf-8');
assert.ok(content.includes('worktree_branch_check'), 'execute-plan.md must contain a worktree_branch_check block');
assert.ok(content.includes('worktree-branch-check.md'), 'execute-plan.md must reference the canonical worktree-branch-check.md fragment');
const hasWindowsOnlyQualifier = (
/Windows.only/i.test(content) ||
/affects Windows only/i.test(content) ||

131
tests/worktree.test.cjs
View File

@@ -35,6 +35,7 @@ const QUICK_PATH = path.join(REPO_ROOT, 'get-shit-done', 'workflows', 'quick.md'
const EXECUTOR_AGENT_PATH = path.join(REPO_ROOT, 'agents', 'gsd-executor.md');
const DIAGNOSE_PATH = path.join(REPO_ROOT, 'get-shit-done', 'workflows', 'diagnose-issues.md');
const GIT_INTEGRATION_PATH = path.join(REPO_ROOT, 'get-shit-done', 'references', 'git-integration.md');
const WORKTREE_BRANCH_CHECK_FRAGMENT = path.join(REPO_ROOT, 'get-shit-done', 'references', 'worktree-branch-check.md');
const isWindows = process.platform === 'win32';
@@ -127,6 +128,59 @@ function findCommandIndex(statements, predicate) {
}
// ─── Canonical fragment: single source of truth ─────────────────────────────
describe('canonical worktree-branch-check fragment is the single source of truth', () => {
const fragmentContent = fs.readFileSync(WORKTREE_BRANCH_CHECK_FRAGMENT, 'utf-8');
const blockMatch = fragmentContent.match(/<worktree_branch_check>([\s\S]*?)<\/worktree_branch_check>/);
const block = blockMatch ? blockMatch[1] : '';
test('fragment file exists and contains a <worktree_branch_check> block', () => {
assert.ok(blockMatch, 'worktree-branch-check.md must contain a <worktree_branch_check> block');
});
test('fragment block contains reset --hard', () => {
assert.ok(block.includes('reset --hard'), 'fragment block must use reset --hard');
});
test('fragment block does NOT contain reset --soft', () => {
assert.ok(!block.includes('reset --soft'), 'fragment block must not use reset --soft');
});
test('fragment block protected-ref alternation contains main', () => {
assert.ok(/\bmain\b/.test(block), 'fragment protected-ref alternation must include main');
});
test('fragment block protected-ref alternation contains master', () => {
assert.ok(/\bmaster\b/.test(block), 'fragment protected-ref alternation must include master');
});
test('fragment block protected-ref alternation contains develop', () => {
assert.ok(/\bdevelop\b/.test(block), 'fragment protected-ref alternation must include develop');
});
test('fragment block protected-ref alternation contains trunk', () => {
assert.ok(/\btrunk\b/.test(block), 'fragment protected-ref alternation must include trunk');
});
test('fragment block protected-ref alternation contains release', () => {
assert.ok(/\brelease\b/.test(block), 'fragment protected-ref alternation must include release');
});
test('fragment block positive allow-list matches ^worktree-agent- pattern', () => {
const allowListRe = /grep\s+-Eq?\s+'\^worktree-agent-/;
assert.ok(allowListRe.test(block), 'fragment block must enforce a positive allow-list matching ^worktree-agent-');
});
test('fragment block contains update-ref prohibition text', () => {
assert.ok(block.includes('update-ref'), 'fragment block must reference update-ref prohibition');
});
test('fragment block contains merge-base HEAD {EXPECTED_BASE}', () => {
assert.ok(block.includes('merge-base HEAD {EXPECTED_BASE}'), 'fragment block must contain git merge-base HEAD {EXPECTED_BASE}');
});
});
const DISCOVERY_PIPELINE =
'grep "^worktree " | grep "\\.claude/worktrees/agent-" | sed \'s/^worktree //\'';
@@ -163,11 +217,11 @@ function makeTempUpstreamRepo(prefix) {
describe('worktree_branch_check must use reset --hard not reset --soft (#2015)', () => {
test('execute-phase.md worktree_branch_check does not use reset --soft', () => {
const content = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf-8');
const fragmentContent = fs.readFileSync(WORKTREE_BRANCH_CHECK_FRAGMENT, 'utf-8');
// Extract the worktree_branch_check block
const blockMatch = content.match(/<worktree_branch_check>([\s\S]*?)<\/worktree_branch_check>/);
assert.ok(blockMatch, 'execute-phase.md must contain a <worktree_branch_check> block');
// Extract the worktree_branch_check block from the canonical fragment
const blockMatch = fragmentContent.match(/<worktree_branch_check>([\s\S]*?)<\/worktree_branch_check>/);
assert.ok(blockMatch, 'worktree-branch-check.md must contain a <worktree_branch_check> block');
const block = blockMatch[1];
assert.ok(
@@ -177,9 +231,9 @@ describe('worktree_branch_check must use reset --hard not reset --soft (#2015)',
});
test('execute-phase.md worktree_branch_check uses reset --hard for base correction', () => {
const content = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf-8');
const blockMatch = content.match(/<worktree_branch_check>([\s\S]*?)<\/worktree_branch_check>/);
assert.ok(blockMatch, 'execute-phase.md must contain a <worktree_branch_check> block');
const fragmentContent = fs.readFileSync(WORKTREE_BRANCH_CHECK_FRAGMENT, 'utf-8');
const blockMatch = fragmentContent.match(/<worktree_branch_check>([\s\S]*?)<\/worktree_branch_check>/);
assert.ok(blockMatch, 'worktree-branch-check.md must contain a <worktree_branch_check> block');
const block = blockMatch[1];
assert.ok(
@@ -189,9 +243,9 @@ describe('worktree_branch_check must use reset --hard not reset --soft (#2015)',
});
test('quick.md worktree_branch_check does not use reset --soft', () => {
const content = fs.readFileSync(QUICK_PATH, 'utf-8');
const blockMatch = content.match(/<worktree_branch_check>([\s\S]*?)<\/worktree_branch_check>/);
assert.ok(blockMatch, 'quick.md must contain a <worktree_branch_check> block');
const fragmentContent = fs.readFileSync(WORKTREE_BRANCH_CHECK_FRAGMENT, 'utf-8');
const blockMatch = fragmentContent.match(/<worktree_branch_check>([\s\S]*?)<\/worktree_branch_check>/);
assert.ok(blockMatch, 'worktree-branch-check.md must contain a <worktree_branch_check> block');
const block = blockMatch[1];
assert.ok(
@@ -201,9 +255,9 @@ describe('worktree_branch_check must use reset --hard not reset --soft (#2015)',
});
test('quick.md worktree_branch_check uses reset --hard for base correction', () => {
const content = fs.readFileSync(QUICK_PATH, 'utf-8');
const blockMatch = content.match(/<worktree_branch_check>([\s\S]*?)<\/worktree_branch_check>/);
assert.ok(blockMatch, 'quick.md must contain a <worktree_branch_check> block');
const fragmentContent = fs.readFileSync(WORKTREE_BRANCH_CHECK_FRAGMENT, 'utf-8');
const blockMatch = fragmentContent.match(/<worktree_branch_check>([\s\S]*?)<\/worktree_branch_check>/);
assert.ok(blockMatch, 'worktree-branch-check.md must contain a <worktree_branch_check> block');
const block = blockMatch[1];
assert.ok(
@@ -211,6 +265,22 @@ describe('worktree_branch_check must use reset --hard not reset --soft (#2015)',
'quick.md worktree_branch_check must use reset --hard to correctly reset both HEAD and working tree'
);
});
test('execute-phase.md references the canonical fragment', () => {
const content = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf-8');
assert.ok(
content.includes('worktree-branch-check.md'),
'execute-phase.md must reference the canonical worktree-branch-check.md fragment'
);
});
test('quick.md references the canonical fragment', () => {
const content = fs.readFileSync(QUICK_PATH, 'utf-8');
assert.ok(
content.includes('worktree-branch-check.md'),
'quick.md must reference the canonical worktree-branch-check.md fragment'
);
});
});
// ─── #2075: worktree deletion safeguards ────────────────────────────────────
@@ -271,12 +341,17 @@ describe('bug-2075: worktree deletion safeguards', () => {
describe('Failure Mode A: worktree_branch_check audit across all worktree-spawning workflows', () => {
test('execute-phase.md has worktree_branch_check block with --hard reset', () => {
const content = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf-8');
const executePhaseContent = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf-8');
assert.ok(
executePhaseContent.includes('worktree-branch-check.md'),
'execute-phase.md must reference the canonical worktree-branch-check.md fragment'
);
const blockMatch = content.match(/<worktree_branch_check>([\s\S]*?)<\/worktree_branch_check>/);
const fragmentContent = fs.readFileSync(WORKTREE_BRANCH_CHECK_FRAGMENT, 'utf-8');
const blockMatch = fragmentContent.match(/<worktree_branch_check>([\s\S]*?)<\/worktree_branch_check>/);
assert.ok(
blockMatch,
'execute-phase.md must contain a <worktree_branch_check> block'
'worktree-branch-check.md must contain a <worktree_branch_check> block'
);
const block = blockMatch[1];
@@ -291,12 +366,17 @@ describe('bug-2075: worktree deletion safeguards', () => {
});
test('quick.md has worktree_branch_check block with --hard reset', () => {
const content = fs.readFileSync(QUICK_PATH, 'utf-8');
const quickContent = fs.readFileSync(QUICK_PATH, 'utf-8');
assert.ok(
quickContent.includes('worktree-branch-check.md'),
'quick.md must reference the canonical worktree-branch-check.md fragment'
);
const blockMatch = content.match(/<worktree_branch_check>([\s\S]*?)<\/worktree_branch_check>/);
const fragmentContent = fs.readFileSync(WORKTREE_BRANCH_CHECK_FRAGMENT, 'utf-8');
const blockMatch = fragmentContent.match(/<worktree_branch_check>([\s\S]*?)<\/worktree_branch_check>/);
assert.ok(
blockMatch,
'quick.md must contain a <worktree_branch_check> block'
'worktree-branch-check.md must contain a <worktree_branch_check> block'
);
const block = blockMatch[1];
@@ -311,15 +391,18 @@ describe('bug-2075: worktree deletion safeguards', () => {
});
test('diagnose-issues.md has worktree_branch_check instruction for spawned agents', () => {
const content = fs.readFileSync(DIAGNOSE_PATH, 'utf-8');
const diagnoseContent = fs.readFileSync(DIAGNOSE_PATH, 'utf-8');
assert.ok(
content.includes('worktree_branch_check'),
'diagnose-issues.md must include worktree_branch_check instruction for spawned debug agents'
diagnoseContent.includes('worktree-branch-check.md'),
'diagnose-issues.md must reference the canonical worktree-branch-check.md fragment for spawned debug agents'
);
const fragmentContent = fs.readFileSync(WORKTREE_BRANCH_CHECK_FRAGMENT, 'utf-8');
const blockMatch = fragmentContent.match(/<worktree_branch_check>([\s\S]*?)<\/worktree_branch_check>/);
assert.ok(blockMatch, 'worktree-branch-check.md must contain a <worktree_branch_check> block');
const block = blockMatch[1];
assert.ok(
content.includes('reset --hard'),
block.includes('reset --hard'),
'diagnose-issues.md worktree_branch_check must instruct agents to use git reset --hard'
);
});