diff --git a/.changeset/plucky-herons-sing.md b/.changeset/plucky-herons-sing.md new file mode 100644 index 000000000..efbeebbd8 --- /dev/null +++ b/.changeset/plucky-herons-sing.md @@ -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. diff --git a/gsd-core/workflows/execute-phase.md b/gsd-core/workflows/execute-phase.md index a6f75f33c..96ca1d190 100644 --- a/gsd-core/workflows/execute-phase.md +++ b/gsd-core/workflows/execute-phase.md @@ -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). diff --git a/tests/bug-630-wave-cleanup-orchestrator-root.test.cjs b/tests/bug-630-wave-cleanup-orchestrator-root.test.cjs new file mode 100644 index 000000000..28f357cad --- /dev/null +++ b/tests/bug-630-wave-cleanup-orchestrator-root.test.cjs @@ -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); + } + }); +}); diff --git a/tests/worktree-cleanup.test.cjs b/tests/worktree-cleanup.test.cjs index df4d7ea35..79c327f3f 100644 --- a/tests/worktree-cleanup.test.cjs +++ b/tests/worktree-cleanup.test.cjs @@ -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\./); });