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

fix: allow quick worktree parent plan base (#1347)

Browse Source
This commit is contained in:
Tom Boucher
2026-06-16 14:00:06 -04:00
committed by GitHub
parent 80014109c1
commit 6e242bd76a
9 changed files with 172 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/eager-ravens-dart.md Normal file
View File

@@ -0,0 +1,5 @@
---
type: Fixed
pr: 1347
---
**Quick worktree execution now accepts parent-or-plan bases for pre-dispatch plan commits** — quick mode records the parent and plan commit around the pre-dispatch PLAN.md commit, lets the worktree guard accept either approved base, materializes the plan from git objects when a runtime forks from the parent, and teaches cleanup to validate the same allowed-base set. (#1265)

16
gsd-core/references/worktree-branch-check.md
View File

@@ -6,9 +6,13 @@ block — do not inline a copy elsewhere. History of coordinated edits: #2924, #
**Contract for orchestrators:** before dispatch, capture `EXPECTED_BASE=$(git rev-parse HEAD)`,
then embed the block below into the sub-agent prompt verbatim, substituting `{EXPECTED_BASE}`
with that captured SHA. The sub-agent only *verifies* and fails closed; the orchestrator
(the worktree lifecycle owner) performs any base recovery — the sub-agent never rewrites a
worktree it did not create (#48).
with that captured SHA. Orchestrators that intentionally create a docs-only pre-dispatch
plan commit may also substitute `{EXPECTED_BASE_ALTERNATE}` with that commit's immediate
parent so runtimes that fork from either side of the docs-only commit pass the same
fail-closed guard (#1265). Otherwise substitute `{EXPECTED_BASE_ALTERNATE}` with an empty
string. The sub-agent only *verifies* and fails closed; the orchestrator (the worktree
lifecycle owner) performs any base recovery — the sub-agent never rewrites a worktree it
did not create (#48).
<worktree_branch_check>
FIRST ACTION: HEAD assertion MUST run before anything else, and this block is
@@ -30,8 +34,10 @@ if ! echo "$ACTUAL_BRANCH" | grep -Eq '^worktree-agent-[A-Za-z0-9._/-]+$'; then
echo "FATAL: worktree HEAD '$ACTUAL_BRANCH' is not in the worktree-agent-* namespace; refusing to commit (#2924)." >&2
exit 42
fi
if [ "$(git rev-parse HEAD)" != "{EXPECTED_BASE}" ]; then
echo "FATAL: worktree base mismatch — HEAD is $(git rev-parse HEAD), expected {EXPECTED_BASE}. Orchestrator owns recovery; sub-agent refuses to rewrite the worktree (#48)." >&2
ACTUAL_BASE=$(git rev-parse HEAD)
EXPECTED_BASE_ALTERNATE="{EXPECTED_BASE_ALTERNATE}"
if [ "$ACTUAL_BASE" != "{EXPECTED_BASE}" ] && { [ -z "$EXPECTED_BASE_ALTERNATE" ] || [ "$ACTUAL_BASE" != "$EXPECTED_BASE_ALTERNATE" ]; }; then
echo "FATAL: worktree base mismatch — HEAD is $ACTUAL_BASE, expected {EXPECTED_BASE}${EXPECTED_BASE_ALTERNATE:+ or $EXPECTED_BASE_ALTERNATE}. Orchestrator owns recovery; sub-agent refuses to rewrite the worktree (#48)." >&2
exit 42
fi
```

25
gsd-core/workflows/quick.md
View File

@@ -632,7 +632,10 @@ When `USE_WORKTREES !== "false"`, commit PLAN.md to the current branch **before*
Skip this step entirely if `USE_WORKTREES === "false"` (non-worktree mode: PLAN.md is committed in Step 8 as usual).
```bash
QUICK_PLAN_PARENT=""
QUICK_PLAN_COMMIT=""
if [ "${USE_WORKTREES}" != "false" ]; then
QUICK_PLAN_PARENT=$(git rev-parse HEAD)
COMMIT_DOCS=$(gsd_run query config-get commit_docs 2>/dev/null || echo "true")
if [ "$COMMIT_DOCS" != "false" ]; then
git add "${QUICK_DIR}/${quick_id}-PLAN.md"
@@ -650,8 +653,12 @@ if [ "${USE_WORKTREES}" != "false" ]; then
git commit -m "docs(${quick_id}): pre-dispatch plan for ${DESCRIPTION}" -- "${QUICK_DIR}/${quick_id}-PLAN.md" \
|| { echo "ERROR: pre-dispatch PLAN.md commit failed — likely a pre-commit hook failure. Fix the hook output above (or set workflow.worktree_skip_hooks=true to bypass) and re-run." >&2; exit 1; }
fi
QUICK_PLAN_COMMIT=$(git rev-parse HEAD)
fi
fi
if [ -z "$QUICK_PLAN_COMMIT" ]; then
QUICK_PLAN_COMMIT=$(git rev-parse HEAD)
fi
fi
```
@@ -678,8 +685,22 @@ Execute quick task ${quick_id}.
${USE_WORKTREES !== "false" ? `
<worktree_branch_check>
ORCHESTRATOR build-time embed (NOT a sub-agent runtime step): before this dispatch, read \`gsd-core/references/worktree-branch-check.md\`, substitute \`{EXPECTED_BASE}\` with the base SHA captured above (${EXPECTED_BASE}), and replace this note with that fragment's \`<worktree_branch_check>\` block so the dispatched prompt carries the runnable guard verbatim — do not pass this instruction through in its place.
ORCHESTRATOR build-time embed (NOT a sub-agent runtime step): before this dispatch, read \`gsd-core/references/worktree-branch-check.md\`, substitute \`{EXPECTED_BASE}\` with the base SHA captured above (${EXPECTED_BASE}), substitute \`{EXPECTED_BASE_ALTERNATE}\` with \`${QUICK_PLAN_PARENT}\` when it differs from \`${EXPECTED_BASE}\` (otherwise empty), and replace this note with that fragment's \`<worktree_branch_check>\` block so the dispatched prompt carries the runnable guard verbatim — do not pass this instruction through in its place.
</worktree_branch_check>
FIRST ACTION after the worktree branch check: ensure the quick PLAN.md exists at a worktree-rooted relative path before any Read/Edit/Write path can be primed. If \`${QUICK_DIR}/${quick_id}-PLAN.md\` is absent, materialize it from the shared git object store:
\`\`\`bash
QUICK_PLAN_COMMIT="${QUICK_PLAN_COMMIT}"
QUICK_PLAN_PATH="${QUICK_DIR}/${quick_id}-PLAN.md"
if [ ! -f "$QUICK_PLAN_PATH" ]; then
mkdir -p "$(dirname "$QUICK_PLAN_PATH")"
git show "${QUICK_PLAN_COMMIT}:${QUICK_PLAN_PATH}" > "$QUICK_PLAN_PATH" || {
echo "FATAL: unable to materialize quick plan from ${QUICK_PLAN_COMMIT}:${QUICK_PLAN_PATH}; refusing to continue." >&2
exit 42
}
fi
\`\`\`
` : ''}
<files_to_read>
@@ -743,7 +764,7 @@ SUMMARY.md and stop — the user must rerun with worktrees disabled.
> **ORCHESTRATOR RULE — CODEX RUNTIME**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available.
If the executor ran with `isolation="worktree"`, append its returned `{agent_id, worktree_path, branch, expected_base}` metadata to `QUICK_WORKTREE_MANIFEST` before cleanup. If any field is unavailable, stop and ask for recovery; do not discover global worktrees.
If the executor ran with `isolation="worktree"`, append its returned `{agent_id, worktree_path, branch, expected_base, allowed_bases}` metadata to `QUICK_WORKTREE_MANIFEST` before cleanup. Set `expected_base` to `${EXPECTED_BASE}` and `allowed_bases` to `["${EXPECTED_BASE}", "${QUICK_PLAN_PARENT}"]` with duplicates removed. If any required field is unavailable, stop and ask for recovery; do not discover global worktrees.
After executor returns:
1. **Worktree cleanup:** If the executor ran with `isolation="worktree"`, merge the worktree branch back and clean up:

11
src/worktree-safety.cts
View File

@@ -416,6 +416,7 @@ interface CleanupManifestEntry {
worktree_path: string;
branch: string;
expected_base: string;
allowed_bases?: string[];
}
function normalizeCleanupManifestEntry(entry: unknown): CleanupManifestEntry | null {
@@ -428,11 +429,16 @@ function normalizeCleanupManifestEntry(entry: unknown): CleanupManifestEntry | n
const expectedBase = typeof e.expected_base === 'string' ? e.expected_base : '';
if (!worktreePath || !branch || !expectedBase) return null;
if (!/^worktree-agent-[A-Za-z0-9._/-]+$/.test(branch)) return null;
const rawAllowedBases = Array.isArray(e.allowed_bases) ? e.allowed_bases : [];
const allowedBases = Array.from(new Set(
[expectedBase, ...rawAllowedBases.filter((base): base is string => typeof base === 'string' && base.length > 0)]
));
return {
agent_id: typeof e.agent_id === 'string' ? e.agent_id : null,
worktree_path: worktreePath,
branch,
expected_base: expectedBase,
allowed_bases: allowedBases,
};
}
@@ -693,7 +699,10 @@ function executeWorktreeWaveCleanupPlan(plan: WaveCleanupPlan | null, deps: Work
}
const mergeBase = execGit(['merge-base', 'HEAD', entry.branch], { cwd: plan.repoRoot });
if (!gitResultOk(mergeBase) || mergeBase.stdout.trim() !== entry.expected_base) {
const allowedBases = Array.isArray(entry.allowed_bases) && entry.allowed_bases.length > 0
? entry.allowed_bases
: [entry.expected_base];
if (!gitResultOk(mergeBase) || !allowedBases.includes(mergeBase.stdout.trim())) {
result.status = 'blocked';
result.reason = 'base_mismatch';
result.stderr = mergeBase?.stderr || '';

37
tests/bug-2432-quick-plan-predispatch-commit.test.cjs
View File

@@ -99,4 +99,41 @@ describe('quick.md pre-dispatch PLAN.md commit (#2432)', () => {
'executor files_to_read must NOT contain hardcoded absolute paths'
);
});
test('#1265 records both parent base and plan commit for worktree dispatch', () => {
const step56Start = content.indexOf('Step 5.6');
const step6Start = content.indexOf('Step 6:', step56Start);
const step56Block = content.slice(step56Start, step6Start);
const step6Block = content.slice(step6Start, content.indexOf('After executor returns:', step6Start));
assert.ok(
step56Block.includes('QUICK_PLAN_PARENT'),
'quick worktree mode must record the pre-dispatch parent SHA before committing PLAN.md (#1265)'
);
assert.ok(
step56Block.includes('QUICK_PLAN_COMMIT'),
'quick worktree mode must record the PLAN.md commit SHA after the pre-dispatch commit (#1265)'
);
assert.ok(
step6Block.includes('EXPECTED_BASE_ALTERNATE'),
'executor guard embed must carry the alternate accepted base for parent-vs-plan worktree forks (#1265)'
);
});
test('#1265 executor materializes PLAN.md from the plan commit when worktree starts at parent', () => {
const executorTask = content.indexOf('subagent_type="gsd-executor"');
assert.ok(executorTask !== -1, 'executor Task() spawn must exist');
const promptStart = content.lastIndexOf('prompt="', executorTask);
const promptEnd = content.indexOf('subagent_type="gsd-executor"', promptStart);
const executorPrompt = content.slice(promptStart, promptEnd);
assert.ok(
executorPrompt.includes('git show') && executorPrompt.includes('QUICK_PLAN_COMMIT'),
'executor prompt must materialize PLAN.md from git show <plan-commit>:<plan-path> when absent (#1265)'
);
assert.ok(
executorPrompt.indexOf('git show') < executorPrompt.indexOf('<files_to_read>'),
'PLAN.md materialization must be a first action before files_to_read can prime paths (#1265)'
);
});
});

2
tests/workflow-size-baseline.json
View File

@@ -57,7 +57,7 @@
"pr-branch.md": 9561,
"profile-user.md": 20650,
"progress.md": 29387,
"quick.md": 47251,
"quick.md": 48435,
"reapply-patches.md": 20393,
"remove-phase.md": 8469,
"remove-workspace.md": 7507,

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

@@ -689,7 +689,7 @@ describe('bug #3384: worktree cleanup workflow contracts', () => {
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, /append its returned `\{agent_id, worktree_path, branch, expected_base, allowed_bases\}`/);
// After #3797 architectural fix: quick.md delegates entirely to the SDK's cleanup-wave
// command (which handles manifest parsing internally). The shell fallback with manual
// QUICK_WORKTREE_MANIFEST node-e code is removed — the gsd_run call with || exit 1 is the

81
tests/worktree-safety.test.cjs
View File

@@ -518,6 +518,7 @@ describe('planWorktreeWaveCleanup', () => {
worktree_path: '/repo/.claude/worktrees/agent-a1',
branch: 'worktree-agent-a1',
expected_base: 'abc123',
allowed_bases: ['abc123', 'def456'],
},
],
});
@@ -527,11 +528,13 @@ describe('planWorktreeWaveCleanup', () => {
worktree_path: entry.worktree_path,
branch: entry.branch,
expected_base: entry.expected_base,
allowed_bases: entry.allowed_bases,
})), [{
agent_id: 'a1',
worktree_path: '/repo/.claude/worktrees/agent-a1',
branch: 'worktree-agent-a1',
expected_base: 'abc123',
allowed_bases: ['abc123', 'def456'],
}]);
assert.equal(plan.discovery, 'manifest');
});
@@ -562,6 +565,84 @@ describe('planWorktreeWaveCleanup', () => {
// ─── executeWorktreeWaveCleanupPlan ───────────────────────────────────────────
describe('executeWorktreeWaveCleanupPlan', () => {
test('#1265 accepts a merge-base listed in allowed_bases even when expected_base is the plan commit', () => {
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: 'plancommit',
allowed_bases: ['plancommit', 'parentcommit'],
}],
};
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: 'parentcommit', 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: 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: '' };
},
});
assert.equal(result.ok, true);
assert.equal(result.entries[0].status, 'merged_removed');
});
test('#1265 still blocks a merge-base outside expected_base and allowed_bases', () => {
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: 'plancommit',
allowed_bases: ['plancommit', 'parentcommit'],
}],
};
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: 'unrelatedbase', stderr: '' };
}
throw new Error(`unexpected git call after rejected base: ${key}`);
},
});
assert.equal(result.ok, false);
assert.equal(result.entries[0].status, 'blocked');
assert.equal(result.entries[0].reason, 'base_mismatch');
});
test('does not delete a branch when worktree removal fails', () => {
const calls = [];
const plan = {

6
tests/worktree.test.cjs
View File

@@ -176,8 +176,9 @@ describe('canonical worktree-branch-check fragment is the single source of truth
assert.ok(block.includes('update-ref'), 'fragment block must reference update-ref prohibition');
});
test('fragment block asserts exact base and fails closed with exit 42 (#48)', () => {
assert.ok(block.includes('git rev-parse HEAD') && block.includes('{EXPECTED_BASE}'), 'fragment must assert HEAD equals {EXPECTED_BASE} exactly (#48)');
test('fragment block asserts an allowed base set and fails closed with exit 42 (#48, #1265)', () => {
assert.ok(block.includes('git rev-parse HEAD') && block.includes('{EXPECTED_BASE}'), 'fragment must assert HEAD against {EXPECTED_BASE} (#48)');
assert.ok(block.includes('{EXPECTED_BASE_ALTERNATE}'), 'fragment must support one orchestrator-approved alternate base for quick parent/plan forks (#1265)');
assert.ok(/exit 42/.test(block), 'fragment must fail closed with exit 42 on mismatch (#48)');
});
});
@@ -848,4 +849,3 @@ done < <(${DISCOVERY_PIPELINE})
});
});
});