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

fix(#630): pin wave-cleanup to orchestrator root via manifest, not worktree-list first-entry (#643)

Browse Source 
* fix(#630): pin wave-cleanup to orchestrator root via manifest, not list first-entry

Follow-up to #590. #590 fixed the dispatch-side orchestrator cwd anchor
(ORCHESTRATOR_WT via git rev-parse --show-toplevel) but the two
wave-cleanup guards still resolved PRIMARY_WT from `git worktree list
--porcelain`'s first entry — always the main checkout. An orchestrator
running from a non-primary (per-phase lane) worktree was therefore cd'd
off its own lane at cleanup, tripping the #3174 branch-drift assertion
(ORCH_BRANCH != EXPECTED_BRANCH) and refusing merge-back — the same
failure #590 set out to fix, surviving on the cleanup side.

Persist the dispatch-time orchestrator root (show-toplevel, captured from
the lane the orchestrator dispatches from) into WAVE_WORKTREE_MANIFEST as
`orchestrator_root`, and resolve PRIMARY_WT from it at both cleanup sites.
The git-worktree-list first entry survives only as a guarded fallback for
pre-#630 manifests. Byte-identical for a primary orchestrator (its root
IS the first entry); unblocks the non-primary-orchestrator topology.

Regression test (bug-630-...): behaviorally proves the pivot by running
the shipped manifest-reader one-liner against a real non-primary-worktree
git topology — it resolves to the lane while first-entry resolves to main
— plus contract assertions. Updates the #3425 worktree-cleanup contract
tests to the new manifest-based resolution.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* chore(#630): add changeset for wave-cleanup orchestrator-root fix

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test(#630): canonicalize paths with realpathSync.native for Windows 8.3 parity

On Windows the CI runner's os.tmpdir() yields an 8.3 short name (RUNNER~1)
while `git worktree list` reports the long form (runneradmin); plain
realpathSync preserved each input's form, so the first-entry/main sanity
comparison mismatched. Canonicalize both sides (and the reader output)
via fs.realpathSync.native, which reconciles 8.3 and long forms. Test-only;
the shipped manifest reader is unaffected.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-06-03 08:07:16 -04:00
committed by GitHub
parent 56a815c722
commit c89197972e
4 changed files with 207 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/plucky-herons-sing.md Normal file
View File

@@ -0,0 +1,5 @@
---
type: Fixed
pr: 643
---
**Wave-cleanup no longer refuses merge-back for an orchestrator running from a non-primary worktree** — the two `execute-phase` wave-cleanup guards now pin to the dispatch-time orchestrator root persisted in `WAVE_WORKTREE_MANIFEST`, instead of `git worktree list`'s first entry (always the main checkout). A per-phase-lane orchestrator with `workflow.use_worktrees: true` is no longer cd'd off its own branch into the #3174 branch-drift assertion at cleanup. Byte-identical for a primary-worktree orchestrator. Follow-up to #590.

25
gsd-core/workflows/execute-phase.md
View File

@@ -553,7 +553,12 @@ increases monotonically across waves. `{status}` is `complete` (success),
EXPECTED_BRANCH=$(git rev-parse --abbrev-ref HEAD)
if [ "${USE_WORKTREES_FOR_PLAN:-true}" != "false" ] && [ -z "${WAVE_WORKTREE_MANIFEST:-}" ]; then
WAVE_WORKTREE_MANIFEST=$(mktemp "${TMPDIR:-/tmp}/gsd-worktree-wave-XXXXXX.json")
printf '{"worktrees":[]}\n' > "$WAVE_WORKTREE_MANIFEST"
# Persist the dispatch-time orchestrator worktree root so wave-cleanup can pin back to the
# orchestrator's OWN worktree — NOT `git worktree list`'s first entry (always the main
# checkout), which pins a non-primary (per-phase lane) orchestrator off its branch (#630).
# Dispatch runs from the orchestrator's lane, so show-toplevel here is the correct root.
ORCH_ROOT=$(git rev-parse --show-toplevel)
ORCH_ROOT="$ORCH_ROOT" MANIFEST="$WAVE_WORKTREE_MANIFEST" node -e 'const fs=require("fs");fs.writeFileSync(process.env.MANIFEST,JSON.stringify({orchestrator_root:process.env.ORCH_ROOT||null,worktrees:[]})+"\n")'
export WAVE_WORKTREE_MANIFEST
fi
```
@@ -761,10 +766,15 @@ increases monotonically across waves. `{status}` is `complete` (success),
exit 1
}
# Guard: pin cleanup back to the primary worktree and fail on branch drift (#3174).
PRIMARY_WT=$(git worktree list --porcelain | awk '/^worktree /{print substr($0,10); exit}')
# Guard: pin cleanup back to the orchestrator's OWN worktree and fail on branch drift (#3174, #630).
# Resolve from the dispatch-time orchestrator root persisted in the manifest — NOT `git worktree
# list`'s first entry, which is always the main checkout and would pin a non-primary (per-phase
# lane) orchestrator off its own branch, tripping the #3174 assertion below (#630). Byte-identical
# for a primary orchestrator (its root IS the first entry); the fallback covers pre-#630 manifests.
PRIMARY_WT=$(MANIFEST="$WAVE_WORKTREE_MANIFEST" node -e 'const fs=require("fs");try{const j=JSON.parse(fs.readFileSync(process.env.MANIFEST,"utf8"));if(j&&j.orchestrator_root)process.stdout.write(String(j.orchestrator_root))}catch(e){}')
[ -n "$PRIMARY_WT" ] || PRIMARY_WT=$(git worktree list --porcelain | awk '/^worktree /{print substr($0,10); exit}')
if [ -z "$PRIMARY_WT" ]; then
echo "FATAL: could not resolve primary worktree before cleanup" >&2
echo "FATAL: could not resolve orchestrator worktree before cleanup" >&2
exit 1
fi
if [ -n "$PRIMARY_WT" ] && [ "$(pwd -P 2>/dev/null)" != "$(cd "$PRIMARY_WT" 2>/dev/null && pwd -P)" ]; then echo "⚠ Orchestrator CWD drifted to $(pwd) — pinning to $PRIMARY_WT before worktree cleanup (#3174)"; cd "$PRIMARY_WT" || { echo "FATAL: cannot cd to primary worktree $PRIMARY_WT" >&2; exit 1; }; fi
@@ -780,8 +790,11 @@ increases monotonically across waves. `{status}` is `complete` (success),
If the orchestrator deviated from the standard wave merge path (e.g., custom inter-worktree base-update merges with `merge: bring …` style messages), run this snippet after the custom merges are complete. It reads only `WAVE_WORKTREE_MANIFEST`; do not discover unrelated `worktree-agent-*` worktrees.
```bash
# Cleanup-tail: pin orchestrator CWD to primary worktree before cleanup-tail (#3174).
PRIMARY_WT=$(git worktree list --porcelain | awk '/^worktree /{print substr($0,10); exit}')
# Cleanup-tail: pin orchestrator CWD to its OWN worktree before cleanup-tail (#3174, #630).
# Same fix as the templated path: resolve the dispatch-time orchestrator root from the manifest,
# not `git worktree list`'s first entry (always the main checkout — wrong for a lane orchestrator).
PRIMARY_WT=$(MANIFEST="$WAVE_WORKTREE_MANIFEST" node -e 'const fs=require("fs");try{const j=JSON.parse(fs.readFileSync(process.env.MANIFEST,"utf8"));if(j&&j.orchestrator_root)process.stdout.write(String(j.orchestrator_root))}catch(e){}')
[ -n "$PRIMARY_WT" ] || PRIMARY_WT=$(git worktree list --porcelain | awk '/^worktree /{print substr($0,10); exit}')
if [ -n "$PRIMARY_WT" ] && [ "$(pwd -P 2>/dev/null)" != "$(cd "$PRIMARY_WT" 2>/dev/null && pwd -P)" ]; then echo "⚠ Orchestrator CWD drifted to $(pwd) — pinning to $PRIMARY_WT before cleanup-tail (#3174)"; cd "$PRIMARY_WT" || { echo "FATAL: cannot cd to primary worktree $PRIMARY_WT" >&2; exit 1; }; fi
# Cleanup-tail: remove residual agent worktrees after a cross-wave-dependency deviation.
# Uses only the current wave manifest to avoid touching unrelated active agents (#3384).

175
tests/bug-630-wave-cleanup-orchestrator-root.test.cjs Normal file
View File

@@ -0,0 +1,175 @@
// allow-test-rule: source-text-is-the-product
// execute-phase.md is the shipped orchestration contract for wave execution and
// cleanup. Bug #630: the two wave-cleanup guards resolved PRIMARY_WT from
// `git worktree list --porcelain`'s first entry — always the main checkout —
// so an orchestrator running from a non-primary (per-phase lane) worktree was
// cd'd off its own lane and tripped the #3174 branch-drift assertion at cleanup,
// refusing merge-back. The fix persists the dispatch-time orchestrator root in
// WAVE_WORKTREE_MANIFEST and pins cleanup to that, falling back to first-entry
// only for pre-#630 manifests.
//
// This file locks the source contract (the .md is the product) AND behaviorally
// proves the pivot by running the shipped manifest-reader one-liner against a
// real non-primary-worktree git topology.
'use strict';
const { test, describe } = require('node:test');
const assert = require('node:assert/strict');
const fs = require('node:fs');
const os = require('node:os');
const path = require('node:path');
const { execFileSync } = require('node:child_process');
const { cleanup } = require('./helpers.cjs');
const EXECUTE_PHASE_MD = path.join(__dirname, '..', 'gsd-core', 'workflows', 'execute-phase.md');
function readMd() {
return fs.readFileSync(EXECUTE_PHASE_MD, 'utf8');
}
// Pull the exact `node -e '...'` manifest-reader script shipped in the cleanup
// guard, so the behavioral test exercises the real shipped code, not a copy.
function extractManifestReaderScript() {
const content = readMd();
// Anchor on `PRIMARY_WT=$(MANIFEST=...` so we grab the cleanup READER, not the
// dispatch-time writer one-liner (which shares the `MANIFEST="..." node -e` prefix).
const m = content.match(/PRIMARY_WT=\$\(MANIFEST="\$WAVE_WORKTREE_MANIFEST" node -e '([^']*)'\)/);
assert.ok(m, 'expected a `PRIMARY_WT=$(MANIFEST="$WAVE_WORKTREE_MANIFEST" node -e \'...\')` reader in execute-phase.md');
return m[1];
}
function git(cwd, args) {
return execFileSync('git', args, {
cwd,
encoding: 'utf8',
stdio: ['ignore', 'pipe', 'pipe'],
}).trim();
}
// Canonicalize a path the way the OS does. On Windows, os.tmpdir() can yield an 8.3
// short name (RUNNER~1) while `git worktree list` reports the long form (runneradmin);
// realpathSync.native reconciles both to the true canonical path so comparisons are stable.
function canon(p) {
return fs.realpathSync.native(p);
}
describe('bug #630 — wave-cleanup pins to the orchestrator root, not git-worktree-list first entry', () => {
test('execute-phase.md is readable', () => {
assert.ok(readMd().length > 0, 'execute-phase.md must not be empty');
});
// ── Source contract (the .md is the product) ──────────────────────────────
test('dispatch persists the orchestrator root into the manifest (#630)', () => {
const content = readMd();
assert.match(
content,
/ORCH_ROOT=\$\(git rev-parse --show-toplevel\)/,
'manifest init must capture the dispatch-time orchestrator root via show-toplevel',
);
assert.match(
content,
/orchestrator_root:\s*process\.env\.ORCH_ROOT/,
'manifest init must write orchestrator_root into WAVE_WORKTREE_MANIFEST',
);
});
test('both cleanup guards resolve PRIMARY_WT from the manifest orchestrator_root (#630)', () => {
const content = readMd();
const readers = content.match(
/PRIMARY_WT=\$\(MANIFEST="\$WAVE_WORKTREE_MANIFEST" node -e '[^']*orchestrator_root[^']*'\)/g,
);
assert.ok(
readers && readers.length >= 2,
`both wave-cleanup guards (templated + cleanup-tail) must read orchestrator_root from the manifest; found ${readers ? readers.length : 0}`,
);
});
test('first-entry resolution survives only as a guarded fallback, never the sole resolver (#630)', () => {
const content = readMd();
// Every remaining first-entry resolution must be preceded by the `[ -n "$PRIMARY_WT" ] ||`
// guard, i.e. it only runs when the manifest lookup produced nothing.
const firstEntryLines = content.match(/^.*git worktree list --porcelain \| awk '\/\^worktree \/.*$/gm) || [];
for (const line of firstEntryLines) {
assert.match(
line,
/\[ -n "\$PRIMARY_WT" \] \|\|/,
`first-entry resolution must be a guarded fallback, not the primary resolver: ${line.trim()}`,
);
}
assert.ok(firstEntryLines.length >= 2, 'expected the fallback in both cleanup guards');
});
// ── Behavioral proof of the pivot ─────────────────────────────────────────
test('shipped manifest reader resolves to the lane worktree, while first-entry resolves to main (#630)', () => {
const tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-630-'));
try {
const mainDir = path.join(tmpRoot, 'main');
fs.mkdirSync(mainDir);
git(mainDir, ['-c', 'init.defaultBranch=main', 'init', '-q']);
git(mainDir, ['config', 'user.email', 'test@example.com']);
git(mainDir, ['config', 'user.name', 'Test']);
fs.writeFileSync(path.join(mainDir, 'f.txt'), 'x\n');
git(mainDir, ['add', '.']);
git(mainDir, ['commit', '-q', '-m', 'init']);
// Non-primary worktree on a per-phase lane branch.
const laneDir = path.join(tmpRoot, 'lane');
git(mainDir, ['worktree', 'add', '-q', '-b', 'feat/lane', laneDir]);
const realMain = canon(mainDir);
const realLane = canon(laneDir);
// Manifest as written at dispatch: orchestrator_root is the lane (the orchestrator runs there).
const manifest = path.join(tmpRoot, 'wave.json');
fs.writeFileSync(manifest, JSON.stringify({ orchestrator_root: realLane, worktrees: [] }) + '\n');
// Run the EXACT shipped reader one-liner.
const script = extractManifestReaderScript();
const resolved = execFileSync('node', ['-e', script], {
cwd: laneDir,
env: { ...process.env, MANIFEST: manifest },
encoding: 'utf8',
}).trim();
// The buggy first-entry resolution (run from the lane) yields the MAIN checkout.
const firstEntry = canon(
git(laneDir, ['worktree', 'list', '--porcelain'])
.split('\n')
.find(l => l.startsWith('worktree '))
.slice('worktree '.length),
);
assert.equal(canon(resolved), realLane, 'manifest reader must resolve to the orchestrator lane worktree');
assert.equal(firstEntry, realMain, 'sanity: first-entry resolution points at the main checkout (the #630 bug target)');
assert.notEqual(canon(resolved), firstEntry, 'the fix must diverge from the old first-entry behavior for a lane orchestrator');
// The #3174 branch assertion now passes (pinned to lane → branch matches EXPECTED_BRANCH);
// pinning to first-entry (main) would have failed it.
const expectedBranch = 'feat/lane';
assert.equal(git(resolved, ['rev-parse', '--abbrev-ref', 'HEAD']), expectedBranch, 'lane pin satisfies the #3174 branch check');
assert.notEqual(git(firstEntry, ['rev-parse', '--abbrev-ref', 'HEAD']), expectedBranch, 'first-entry pin would have tripped the #3174 branch check');
} finally {
cleanup(tmpRoot);
}
});
test('manifest reader falls through (empty output) when orchestrator_root is absent — fallback engages (#630)', () => {
const tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-630-fb-'));
try {
const manifest = path.join(tmpRoot, 'legacy.json');
// Pre-#630 manifest shape: no orchestrator_root.
fs.writeFileSync(manifest, JSON.stringify({ worktrees: [] }) + '\n');
const script = extractManifestReaderScript();
const out = execFileSync('node', ['-e', script], {
env: { ...process.env, MANIFEST: manifest },
encoding: 'utf8',
});
assert.equal(out, '', 'reader must emit nothing for a manifest without orchestrator_root so the first-entry fallback engages');
} finally {
cleanup(tmpRoot);
}
});
});

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

@@ -706,8 +706,11 @@ describe('bug #3384: worktree cleanup workflow contracts', () => {
test('#3425: helper cleanup path pins orchestrator CWD to primary worktree and checks EXPECTED_BRANCH', () => {
const content = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf8');
assert.match(content, /PRIMARY_WT=\$\(git worktree list --porcelain \| awk '\/\^worktree \/\{print substr\(\$0,10\); exit\}'\)/);
assert.match(content, /if \[ -z "\$PRIMARY_WT" \]; then\s+echo "FATAL: could not resolve primary worktree before cleanup" >&2\s+exit 1\s+fi/);
// #630: the orchestrator root is now resolved from the manifest's orchestrator_root; the
// git-worktree-list first entry survives only as a guarded fallback for pre-#630 manifests.
assert.match(content, /PRIMARY_WT=\$\(MANIFEST="\$WAVE_WORKTREE_MANIFEST" node -e '[^']*orchestrator_root[^']*'\)/);
assert.match(content, /\[ -n "\$PRIMARY_WT" \] \|\| PRIMARY_WT=\$\(git worktree list --porcelain \| awk '\/\^worktree \/\{print substr\(\$0,10\); exit\}'\)/);
assert.match(content, /if \[ -z "\$PRIMARY_WT" \]; then\s+echo "FATAL: could not resolve orchestrator worktree before cleanup" >&2\s+exit 1\s+fi/);
assert.match(content, /cd "\$PRIMARY_WT" \|\| \{ echo "FATAL: cannot cd to primary worktree \$PRIMARY_WT" >&2; exit 1; \}/);
assert.match(content, /ORCH_BRANCH=\$\(git rev-parse --abbrev-ref HEAD\)/);
assert.match(content, /FATAL: orchestrator on '\$ORCH_BRANCH' but expected '\$EXPECTED_BRANCH' before worktree cleanup — refusing to merge \(#3174-class drift\)/);
@@ -718,7 +721,9 @@ test('#3425: helper cleanup path pins orchestrator CWD to primary worktree and c
test('#3425: cleanup-tail snippet carries the same primary-worktree pin before removal', () => {
const content = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf8');
assert.match(content, /Cleanup-tail: pin orchestrator CWD to primary worktree before cleanup-tail \(#3174\)\./);
assert.match(content, /Cleanup-tail: pin orchestrator CWD to its OWN worktree before cleanup-tail \(#3174, #630\)\./);
// #630: cleanup-tail resolves the orchestrator root from the manifest, with first-entry fallback.
assert.match(content, /PRIMARY_WT=\$\(MANIFEST="\$WAVE_WORKTREE_MANIFEST" node -e '[^']*orchestrator_root[^']*'\)/);
assert.match(content, /FATAL: cannot cd to primary worktree \$PRIMARY_WT/);
assert.match(content, /# Cleanup-tail: remove residual agent worktrees after a cross-wave-dependency deviation\./);
});