diff --git a/.changeset/bold-lynx-dart.md b/.changeset/bold-lynx-dart.md new file mode 100644 index 000000000..79c2ac593 --- /dev/null +++ b/.changeset/bold-lynx-dart.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3281 +--- +**Two GSD workflows told agents that a Claude Code `Agent()` spawn blocks until the subagent finishes** — Claude Code backgrounds subagents by default, so `/gsd-execute-phase` could treat a wave as returned when it had not, and `/gsd-debug` lost its session-manager handoff in exactly the way #2196 was filed to fix. The dispatch notes now match this package's own shipped capability matrix, and both debug spawns carry the `run_in_background: false` opt-out they always needed. (#3177) diff --git a/gsd-core/workflows/debug.md b/gsd-core/workflows/debug.md index 497c24653..fff78c87c 100644 --- a/gsd-core/workflows/debug.md +++ b/gsd-core/workflows/debug.md @@ -140,7 +140,8 @@ specialist_dispatch_enabled: true """, subagent_type="gsd-debug-session-manager", model="{debugger_model}", - description="Continue debug session {SLUG}" + description="Continue debug session {SLUG}", + run_in_background=false ) ``` @@ -203,7 +204,7 @@ Create `.planning/debug/{slug}.md` with initial state using the Write tool (neve After initial context setup, spawn the session manager to handle the full checkpoint/continuation loop. The session manager handles specialist_hint dispatch internally: when gsd-debugger returns ROOT CAUSE FOUND it extracts the specialist_hint field and invokes the matching skill (e.g. typescript-expert, swift-concurrency) before offering fix options. -> **Foreground, blocking spawn — #2196.** The `Agent(subagent_type="gsd-debug-session-manager", …)` call below is FOREGROUND and BLOCKING — it returns the compact session summary directly. Wait for it; do not background it, and do not poll for it. Never pass an agent or session identifier to `TaskOutput` — an agent ID is NOT a task ID, so `TaskOutput ` always returns `No task found with ID`. If the spawn returns no usable result (the handoff is lost), do NOT claim the session is still running: preserve the checkpoint at `.planning/debug/{slug}.md`, report the failed handoff plainly, and resume by re-spawning the session manager or via `/gsd:debug continue {slug}`. +> **Foreground, blocking spawn — #2196.** The `Agent(subagent_type="gsd-debug-session-manager", …)` call below MUST carry `run_in_background: false` — Claude Code backgrounds subagents by default, and only that flag makes the spawn return the compact session summary directly. Wait for it; do not background it, and do not poll for it. Never pass an agent or session identifier to `TaskOutput` — an agent ID is NOT a task ID, so `TaskOutput ` always returns `No task found with ID`. If the spawn returns no usable result (the handoff is lost), do NOT claim the session is still running: preserve the checkpoint at `.planning/debug/{slug}.md`, report the failed handoff plainly, and resume by re-spawning the session manager or via `/gsd:debug continue {slug}`. Print before spawning (runs in a subagent — no output until it returns, ~1–5 min; expected, not a freeze): ``` @@ -229,7 +230,8 @@ specialist_dispatch_enabled: true """, subagent_type="gsd-debug-session-manager", model="{debugger_model}", - description="Debug session {slug}" + description="Debug session {slug}", + run_in_background=false ) ``` diff --git a/gsd-core/workflows/execute-phase.md b/gsd-core/workflows/execute-phase.md index f92e1117e..0420b0cdc 100644 --- a/gsd-core/workflows/execute-phase.md +++ b/gsd-core/workflows/execute-phase.md @@ -19,7 +19,7 @@ Orchestrator coordinates, not executes. Each subagent loads the full execute-pla **Subagent spawning is runtime-specific:** -- **Claude Code:** Uses `Agent(subagent_type="gsd-executor", ...)` — blocks until complete, returns result +- **Claude Code:** Uses `Agent(subagent_type="gsd-executor", ...)` — backgrounded by default; verify completion - **Copilot:** Subagent spawning does not reliably return completion signals. **Default to sequential inline execution**: read and follow execute-plan.md directly for each plan instead of spawning parallel agents. Only attempt parallel spawning if the user @@ -852,8 +852,8 @@ increases monotonically across waves. `{status}` is `complete` (success), If the stalled executor ran in an isolated worktree, `kill and switch to inline execution` edits the primary checkout — see worktree recovery policy (`execute-phase/steps/worktree-recovery-policy.md`). Prefer `kill and retry` in a fresh worktree; inline execution requires explicit confirmation, never the default. - **This fallback applies automatically to all runtimes.** Claude Code's Agent() normally - returns synchronously, but the fallback ensures resilience if it doesn't. + **This fallback applies to all runtimes.** Claude Code's Agent() backgrounds by + default: the completion signal may never arrive. Verify, never wait. 5. **Post-wave hook validation (parallel mode only):** Hooks run on every executor commit by default (#2924); this post-wave run only fires when `workflow.worktree_skip_hooks=true` opted out of per-commit hooks: ```bash diff --git a/tests/emitted-drift-acks/3149-init-debug-entry-point.json b/tests/emitted-drift-acks/3149-init-debug-entry-point.json index 3612e2f6f..32df977f0 100644 --- a/tests/emitted-drift-acks/3149-init-debug-entry-point.json +++ b/tests/emitted-drift-acks/3149-init-debug-entry-point.json @@ -1,6 +1,6 @@ { "version": 1, "paths": { - "debug.md": "#3149 (prerequisite for #3128, ADR-1671 admission gate 2): debug.md gains a dedicated `init.debug` entry point (cmdInitDebug) and its Step 0 collapses THREE separate `gsd_run` round-trips into one. Removed: `gsd_run query state.load` (line 20, replaced in place), the `resolve-model gsd-debugger --pick model` block, and the `config-get workflow.tdd_mode --raw` block — 2 prose lead-ins and 2 fenced code blocks in total. Added: a 6-bullet extraction list documenting the bundle's fields (`commit_docs`, the now TOP-LEVEL `response_language`, `debug_dir`, `debugger_model`, `tdd_mode`, `section_manifest`) plus the `section_manifest: null` -> read-everything rule and the null-vs-empty-included distinction. Net SOURCE growth is +618 bytes (20,555 -> 21,173): the bullets that document one bundle cost more bytes than the two shell round-trips they replace, which is the intended trade — the round-trips cost three subprocess spawns at RUN time on every /gsd:debug invocation. No applicability-section marker is added and WHEN_VOCABULARY is unchanged at 29, so the composeWorkflow emission path is byte-identical in shape to before; only this file's own content moved. The `{TDD_MODE}` and `{debugger_model}` placeholders in the session-parameter blocks (lines ~137-145, ~226-234) are deliberately left byte-identical — they now resolve from the init bundle instead of shell variables, and rewording them would ripple into tests/fix-2257-debug-nonterminal-resume.test.cjs and tests/debug-session-manager-commit.test.cjs for no behavioral gain." + "debug.md": "#3149 (prerequisite for #3128, ADR-1671 admission gate 2): debug.md gains a dedicated `init.debug` entry point (cmdInitDebug) and its Step 0 collapses THREE separate `gsd_run` round-trips into one. Removed: `gsd_run query state.load` (line 20, replaced in place), the `resolve-model gsd-debugger --pick model` block, and the `config-get workflow.tdd_mode --raw` block — 2 prose lead-ins and 2 fenced code blocks in total. Added: a 6-bullet extraction list documenting the bundle's fields (`commit_docs`, the now TOP-LEVEL `response_language`, `debug_dir`, `debugger_model`, `tdd_mode`, `section_manifest`) plus the `section_manifest: null` -> read-everything rule and the null-vs-empty-included distinction. Net SOURCE growth is +618 bytes (20,555 -> 21,173): the bullets that document one bundle cost more bytes than the two shell round-trips they replace, which is the intended trade — the round-trips cost three subprocess spawns at RUN time on every /gsd:debug invocation. No applicability-section marker is added and WHEN_VOCABULARY is unchanged at 29, so the composeWorkflow emission path is byte-identical in shape to before; only this file's own content moved. The `{TDD_MODE}` and `{debugger_model}` placeholders in the session-parameter blocks (lines ~137-145, ~226-234) are deliberately left byte-identical — they now resolve from the init bundle instead of shell variables, and rewording them would ripple into tests/fix-2257-debug-nonterminal-resume.test.cjs and tests/debug-session-manager-commit.test.cjs for no behavioral gain. #3177: the \"Foreground, blocking spawn — #2196\" note asserted that the gsd-debug-session-manager dispatch \"is FOREGROUND and BLOCKING\" as an inherent property of the call, but NEITHER of the two real Agent(...) blocks — the new-session path and the continue path — passed run_in_background: false. Claude Code backgrounds subagents by DEFAULT as of v2.1.198, so both calls actually backgrounded and the compact session summary never returned inline, silently reinstating the exact lost-handoff failure #2196 was filed to fix (the file's own contingency for \"the handoff is lost\" became the normal path). The note now states the opt-out as a requirement and both dispatches carry run_in_background=false, so the mechanism matches the recorded intent. Growth is that clause plus the two added argument lines: +142 bytes (21,173 -> 21,315), far under the 40,960 DEFAULT tier cap. No other content changed." } } diff --git a/tests/fix-3177-execute-phase-dispatch-claim.test.cjs b/tests/fix-3177-execute-phase-dispatch-claim.test.cjs new file mode 100644 index 000000000..e4ae9a5bb --- /dev/null +++ b/tests/fix-3177-execute-phase-dispatch-claim.test.cjs @@ -0,0 +1,278 @@ +/** + * #3177 — `gsd-core/workflows/execute-phase.md` asserted that Claude Code's + * `Agent(...)` "blocks until complete, returns result", and restated the same + * claim further down as "Claude Code's Agent() normally returns synchronously". + * + * Both were stale. Claude Code runs subagents in the BACKGROUND by default as of + * v2.1.198; `run_in_background: false` is the explicit opt-out when an immediate + * result is needed (code.claude.com/docs/en/sub-agents, + * /agent-sdk/subagents, /tools-reference). + * + * This package already recorded the true value: `capabilities/claude/capability.json` + * declares `dispatch.background: true` and `docs/reference/host-integration-capability-matrix.md` + * renders it. So the SHIPPED PROSE and the SHIPPED MATRIX disagreed about the same + * runtime — and the prose is the copy loaded verbatim into the orchestrator's context + * on every `/gsd-execute-phase` run, where it is the premise a spawn-safety conclusion + * leans on (#3159 documents a host where that difference cost a wave of executor work). + * + * The durable guard is therefore NOT a snapshot of today's wording: it is a PARITY + * assertion (tests 3 and 4) that reds whenever the prose, the matrix, and the descriptor + * stop agreeing about `dispatch.background` — in either direction. Tests 5 and 6 pin the + * negative space so a future correction cannot over-sweep: Codex dispatch genuinely IS + * synchronous, and its orchestrator rules must survive a fix aimed at Claude Code. + */ + +// allow-test-rule: runtime-contract-is-the-product #3177 — the workflow markdown is loaded +// verbatim into the agent's context and the matrix/descriptor ARE the negotiated host +// contract; asserting agreement between those documents is behavioral, not source-grep. + +'use strict'; + +process.env.GSD_TEST_MODE = '1'; + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const fc = require('fast-check'); + +const ROOT = path.join(__dirname, '..'); +const WORKFLOW = path.join(ROOT, 'gsd-core', 'workflows', 'execute-phase.md'); +const MATRIX = path.join(ROOT, 'docs', 'reference', 'host-integration-capability-matrix.md'); +const DESCRIPTOR = path.join(ROOT, 'capabilities', 'claude', 'capability.json'); + +const workflowText = () => fs.readFileSync(WORKFLOW, 'utf8'); + +/** + * Value of `| | | …` inside the `## ` section of the matrix. + * + * Anchored on a whole heading LINE, not a substring: `## claude` must never match + * `## claude-local`, and the section must end at the next `## ` heading so a field + * absent from this host can never be answered from the next host's table. + * + * @param {string} matrix - full matrix document text + * @param {string} host - section name, e.g. `claude` + * @param {string} field - row label, e.g. `dispatch.background` + * @returns {string|null} trimmed cell value, or null when the section or row is absent + */ +function matrixField(matrix, host, field) { + const lines = matrix.split('\n'); + const start = lines.findIndex((l) => l.trim() === `## ${host}`); + if (start === -1) return null; + let end = lines.length; + for (let i = start + 1; i < lines.length; i += 1) { + if (lines[i].startsWith('## ')) { end = i; break; } + } + const row = lines.slice(start + 1, end).find((l) => l.startsWith(`| ${field} |`)); + if (!row) return null; + return row.split('|')[2].trim(); +} + +describe('#3177: execute-phase.md states Claude Code dispatch truthfully', () => { + test('execute-phase.md never claims Claude Code Agent() blocks or returns synchronously', () => { + // Row 1 — the failing-first regression. Both stale sentences, by their own text. + const text = workflowText(); + const stale = ['blocks until complete', 'returns synchronously']; + const present = stale.filter((phrase) => text.includes(phrase)); + assert.deepEqual( + present, [], + 'execute-phase.md still asserts synchronous Claude Code dispatch. Claude Code backgrounds ' + + 'subagents by default (v2.1.198+); `run_in_background: false` is the opt-out.', + ); + }); + + test('the Claude Code dispatch bullet states the background-by-default model', () => { + // Row 2 — the corrected sentence must actually SAY the true thing, not merely + // omit the false one. A deletion would pass row 1 while teaching nothing. + const bullet = workflowText() + .split('\n') + .find((l) => l.startsWith('- **Claude Code:**')); + assert.ok(bullet, 'the Claude Code bullet must exist'); + assert.match( + bullet, /backgrounded by default/, + 'the bullet must state that dispatch is backgrounded by default', + ); + assert.match( + bullet, /verify completion/, + 'the bullet must point at completion verification. This workflow deliberately backgrounds ' + + 'its executors (the multi-plan path prescribes run_in_background: true), so the blocking ' + + 'opt-out is not the guidance here — confirming completion is.', + ); + assert.ok( + bullet.includes('Agent(subagent_type="gsd-executor"'), + 'the bullet must still carry the dispatch mechanism the rest of the file depends on', + ); + }); + + test('the workflow prose and the capability matrix agree on claude dispatch.background', () => { + // Row 3 — the parity guard. This is the assertion that outlives the wording: + // flip the matrix to `false` without touching the prose (or vice versa) and this reds. + const declared = matrixField(fs.readFileSync(MATRIX, 'utf8'), 'claude', 'dispatch.background'); + assert.equal(declared, 'true', 'matrix must document claude dispatch.background'); + + const bullet = workflowText() + .split('\n') + .find((l) => l.startsWith('- **Claude Code:**')); + const proseSaysBackground = /backgrounded by default/.test(bullet ?? ''); + assert.equal( + proseSaysBackground, declared === 'true', + 'execute-phase.md and the host-integration matrix disagree about whether claude backgrounds ' + + 'its subagent dispatch. They describe the same runtime; exactly one of them is wrong (#3177).', + ); + }); + + test('the claude descriptor and the matrix agree on dispatch.background', () => { + // Row 4 — the other half of the divergence class. The matrix is generated from + // the descriptor, so this pins the generator's output to its input. + const descriptor = JSON.parse(fs.readFileSync(DESCRIPTOR, 'utf8')); + const declared = matrixField(fs.readFileSync(MATRIX, 'utf8'), 'claude', 'dispatch.background'); + assert.equal( + String(descriptor.runtime.hostIntegration.dispatch.background), declared, + 'capabilities/claude/capability.json and the rendered matrix disagree', + ); + }); + + test('the Codex orchestrator rule is not swept by the Claude Code correction', () => { + // Row 5 — negative space. Codex dispatch IS synchronous. A regex sweep for + // "return its result" would introduce a NEW falsehood here; this catches that. + const text = workflowText(); + const codexRules = text + .split('\n') + .filter((l) => l.includes('ORCHESTRATOR RULE — CODEX RUNTIME')); + assert.ok(codexRules.length >= 2, 'both Codex orchestrator rules must survive'); + for (const rule of codexRules) { + assert.ok( + rule.includes('Wait for the subagent to return its result'), + 'Codex dispatch is genuinely synchronous — its wait rule must not be corrected away', + ); + } + }); + + test('the Copilot and multi-plan dispatch rules survive the correction', () => { + // Row 6 — negative space. These were already true and are adjacent to the edit. + const text = workflowText(); + assert.ok( + text.includes('- **Copilot:** Subagent spawning does not reliably return completion signals.'), + 'the Copilot bullet must survive verbatim', + ); + assert.ok( + text.includes('one at a time with `run_in_background: true`'), + 'the multi-plan wave prescription must survive verbatim', + ); + assert.ok( + text.includes('If `Agent` IS available (top-level Claude'), + 'the spawn mandate is derived from TOOL AVAILABILITY, not from blocking — it must survive', + ); + }); +}); + +describe('#3177: matrix section extraction is bounded by its heading', () => { + const matrix = () => fs.readFileSync(MATRIX, 'utf8'); + + test('section extraction — first row of the section', () => { + // limit-1: the row immediately after the `## claude` heading is INSIDE. + assert.equal(matrixField(matrix(), 'claude', 'embeddingMode'), 'imperative'); + }); + + test('section extraction — last row before the next heading', () => { + // limit: the final row of `## claude` is still INSIDE. + assert.ok(matrixField(matrix(), 'claude', 'dispatch.isolation').startsWith('harness-worktree')); + }); + + test('section extraction — a row in the next section never leaks in', () => { + // limit+1: a field absent from claude must be null rather than silently + // resolved from `## codex` below it, and two hosts with different values + // must never resolve to the same cell. + const doc = matrix(); + assert.equal(matrixField(doc, 'claude', 'dispatch.background'), 'true'); + assert.equal(matrixField(doc, 'claude', '__definitely_not_a_field__'), null); + assert.notEqual( + matrixField(doc, 'claude', 'effortSurface'), + matrixField(doc, 'kilo', 'effortSurface'), + 'two hosts with different values must not resolve to the same cell', + ); + }); + + test('fc property: field extraction never leaks across ## boundaries', () => { + const hostArb = fc.stringMatching(/^[a-z][a-z0-9-]{0,12}$/); + const valueArb = fc.stringMatching(/^[a-z0-9]{1,10}$/); + fc.assert( + fc.property( + fc.uniqueArray(fc.tuple(hostArb, valueArb), { + minLength: 2, maxLength: 6, selector: ([h]) => h, + }), + fc.nat(), + (sections, pick) => { + const doc = sections + .map(([host, value]) => `## ${host}\n\n| axis | value |\n| f | ${value} |\n`) + .join('\n'); + const [host, value] = sections[pick % sections.length]; + // Exactly the requested section's value, never a neighbor's. + assert.equal(matrixField(doc, host, 'f'), value); + // A longer name that merely EXTENDS a real heading resolves to nothing. + // `_` is outside hostArb's alphabet, so this probe can NEVER collide with + // another generated section — the `-local` form could, and did. + assert.equal(matrixField(doc, `${host}_x`, 'f'), null); + }, + ), + { numRuns: 200 }, + ); + }); +}); + +describe('#3177: debug.md dispatches its session manager in the foreground', () => { + const DEBUG_WF = path.join(ROOT, 'gsd-core', 'workflows', 'debug.md'); + const debugText = () => fs.readFileSync(DEBUG_WF, 'utf8'); + + /** + * Every fenced `Agent( … )` block in debug.md that dispatches the session manager. + * + * Anchored on a line that is exactly `Agent(` so the PROSE mention of + * `Agent(subagent_type="gsd-debug-session-manager", …)` inside the blockquote at + * :206 is not mistaken for a dispatch. Positional selection (first match wins) was + * the original bug here: it silently checked the continue path while the + * new-session path went unexamined. + */ + function sessionManagerDispatches(text) { + const blocks = []; + for (const m of text.matchAll(/^Agent\($/gm)) { + const close = text.indexOf('\n)', m.index); + if (close === -1) continue; + const block = text.slice(m.index, close); + if (block.includes('subagent_type="gsd-debug-session-manager"')) blocks.push(block); + } + return blocks; + } + + test('every session-manager spawn carries the run_in_background: false opt-out', () => { + // #2196 required this dispatch be foreground and blocking so the orchestrator + // receives the session summary inline; debug.md still says "Wait for it; do not + // background it" and "Display the compact summary returned by the session + // manager". Claude Code backgrounds subagents by DEFAULT, so that intent only + // holds if each call states the opt-out explicitly — prose alone silently + // reinstated the exact lost-handoff failure #2196 was filed to fix. + const blocks = sessionManagerDispatches(debugText()); + assert.equal( + blocks.length, 2, + 'debug.md dispatches the session manager on BOTH the new-session and continue paths; ' + + 'a change to that count means a dispatch was added or removed and must be re-checked.', + ); + for (const block of blocks) { + assert.match( + block, /run_in_background\s*=\s*false/, + 'every gsd-debug-session-manager dispatch must pass run_in_background=false — without ' + + 'it Claude Code backgrounds the spawn and the compact summary never returns (#2196).', + ); + } + }); + + test('debug.md does not assert the spawn is inherently foreground', () => { + // The old premise ("is FOREGROUND and BLOCKING") was a property claim about the + // host, not an instruction — and it was false for the same reason as #3177. + assert.ok( + !debugText().includes('is FOREGROUND and BLOCKING'), + 'debug.md must not claim the Agent() call is inherently foreground; it must name the ' + + 'run_in_background: false opt-out that actually makes it so.', + ); + }); +});