From b146b82e3ce70899fcc7c020feedac471f5c160f Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 4 Aug 2026 08:38:31 -0400 Subject: [PATCH] fix(#2868): resume a phase stranded between its last plan and verification (#3041) * fix(#2868): resume a phase stranded between its last plan and verification discover_and_group_plans exited unconditionally once every plan was filtered out, conflating "no plan work left" with "phase fully done". Those differ once a run can be interrupted between the final wave's SUMMARY and the verify step -- most often by a checkpoint plan that is retired but still writes a SUMMARY. The result was a phase that looked healthy from every index yet had no VERIFICATION.md, and whose recommended recovery command provably no-opped, because the only step that produces the artifact sits ten steps past that exit. The exit is now conditional. When the verification report is genuinely missing and no filter is active, the run reports the situation by name and continues at the tail gates instead of stopping. Two guards keep the normal paths untouched: - A filtered run (--gaps-only, or an explicit wave) finding nothing left in its own slice says nothing about whether the phase as a whole is done, so it exits exactly as before. Without this, --wave 1 on a finished first wave would jump to verification with later waves still outstanding. - A phase that already has its report exits as before too. The recovered path deliberately keeps the code-review and regression gates. The manual workaround this replaces skipped both, and that gap is the reason a real route exists rather than telling users to spawn the verifier by hand. Also acknowledges the emitted growth of the workflow file. As with #2830 it is appended to the fragment that already owns that path, since the linter hard-fails when two acknowledgment sources name the same one. Co-Authored-By: Claude Opus 5 * fix(#2868): never treat a blocked-and-incomplete phase as finished Three findings from adversarial review, all fixed. BLOCKER -- the trigger conflated two different zero-runnable states. This step now has two skip rules: has_summary (the #2868 target) and, from #2830, a skip for plans whose blocked_by is non-empty. "All filtered" was therefore reachable with plans that never ran: plan A halts and is summarized, plan B is blocked by A and has no summary. The resume path fired, announced "All N plans are summarized" -- false -- skipped the wave steps so B was never dispatched, and jumped to the gates. B was silently abandoned, which is the same class of disappearance #2830 exists to prevent, reintroduced one layer up. The decision is now an explicit ordered three-way: a filtered run exits unchanged; any blocked-plan skip reports the phase as stuck on a halt and exits, routing to resolving the halt rather than to verification; only an all-summarized, unfiltered phase with a missing report resumes. MAJOR -- RESUME_TAIL_ONLY was set and never read anywhere in the workflow or its step fragments. Dead state implying enforcement that did not exist. Removed; the imperative at the decision point is what actually carries the control flow, so it now says so plainly. MAJOR -- the resume path skipped aggregate_results, which is the only step that runs the secure-phase threats-open gate. A phase with open threats would have advanced with no warning where a normal run always shows one. The path now enters at aggregate_results, verified to read only on-disk phase artifacts and independent queries, so it tolerates having executed no plans this run. Co-Authored-By: Claude Opus 5 * chore(#2868): backfill changeset pr number Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- .changeset/noble-deer-frolic.md | 5 + gsd-core/workflows/execute-phase.md | 43 +++++- ...930-fragmentize-execute-phase-markers.json | 2 +- tests/execute-phase-wave.test.cjs | 122 ++++++++++++++++++ tests/verification-status.test.cjs | 73 +++++++++++ 5 files changed, 243 insertions(+), 2 deletions(-) create mode 100644 .changeset/noble-deer-frolic.md diff --git a/.changeset/noble-deer-frolic.md b/.changeset/noble-deer-frolic.md new file mode 100644 index 000000000..79912a8e4 --- /dev/null +++ b/.changeset/noble-deer-frolic.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3041 +--- +**A phase stranded between its last plan and verification can now be recovered** — if every plan carried a SUMMARY but the run never reached the verify step (most often because a checkpoint plan was retired yet still summarized), re-running execute-phase exited immediately and could never produce the missing VERIFICATION.md, so the recommended recovery command silently did nothing. It now resumes at the phase gates instead, with the code-review and regression gates still running. (#2868) diff --git a/gsd-core/workflows/execute-phase.md b/gsd-core/workflows/execute-phase.md index 878f200ef..f40b0d78a 100644 --- a/gsd-core/workflows/execute-phase.md +++ b/gsd-core/workflows/execute-phase.md @@ -340,7 +340,48 @@ Parse JSON for: `phase`, `plans[]` (each with `id`, `wave`, `autonomous`, `objec **Wave safety check:** If `WAVE_FILTER` is set and there are still incomplete plans in any lower wave that match the current execution mode, STOP and tell the user to finish earlier waves first. Do not let Wave 2+ execute while prerequisite earlier-wave plans remain incomplete. -If all filtered: "No matching incomplete plans" → exit. +**If all filtered — do NOT exit unconditionally (#2868).** "No plan work left" and "phase fully +done" are different conditions: a run can be interrupted between the final wave's SUMMARY and +`verify_phase_goal` (most commonly by a checkpoint plan that is retired but still writes a SUMMARY), +leaving a phase that looks complete from every index yet never produced `*-VERIFICATION.md`. A +third condition looks identical to the first two by plan_count alone but is neither: some filtered +plans were filtered because they are **blocked** (non-empty `blocked_by`, #2830), not because they +are done. Blocked-and-incomplete must never be reported as finished. + +```bash +VERIFY_STATUS=$(gsd_run query verification status "${PHASE_DIR}" --pick status) +``` + +Evaluate in this exact order — the first matching condition decides the outcome; do not evaluate +later conditions once one matches: + +1. **A filter is active** (`--gaps-only`, or `WAVE_FILTER` set): report "No matching incomplete + plans" → exit, unchanged. A filtered run finding nothing left in ITS slice says nothing about + whether the phase as a whole is done, and must never jump to verification. +2. **No filter is active, and at least one filtered plan was skipped because of a non-empty + `blocked_by`** (irrespective of `VERIFY_STATUS`): the phase is NOT finished — it is **stuck on a + halt**. A plan with no SUMMARY and no dispatched work must never be treated as done merely + because nothing was left to filter. Report: + `"Phase stuck: {blocked plan ids} blocked by halted {their blocked_by ids} — resolve the halt, do not resume verification."` + → exit. Do not fall through to condition 3; this is not a completion state. +3. **No filter is active, and every filtered plan was filtered by `has_summary` alone** (no + blocked-plan skip occurred): + - **`VERIFY_STATUS` is anything other than `missing`**: the phase genuinely finished. Report + "No matching incomplete plans" → exit, unchanged. + - **`VERIFY_STATUS == missing`**: the plans are all summarized but the run never reached the + tail gates. Report: + `"All {plan_count} plans are summarized but no VERIFICATION.md exists — resuming at the phase gates (#2868)."` + SKIP `cross_ai_delegation`, `execute_waves` and `checkpoint_handling` — there is no wave work + to do — and continue directly at `aggregate_results`, NOT `code_review_gate`. `aggregate_results` + is the only step that runs the `SECURITY_FILE` / secure-phase threats-open gate, and it reads + exclusively from on-disk `${PHASE_DIR}` artifacts (`*-SUMMARY.md`, `*-SECURITY.md` via `ls`) and + independent `gsd_run` calls — nothing it reads is produced only by `execute_waves` or + `checkpoint_handling` — so it tolerates having executed no plans in this run. From there the + run proceeds exactly as a normal one: `aggregate_results` → `code_review_gate` → + `close_parent_artifacts` → `regression_gate` → `verify_phase_goal` → `update_roadmap`. Never + skip `aggregate_results`, `code_review_gate` or `regression_gate` on this path — the manual + workaround this replaces skipped all three, and that gap is the reason this route exists + rather than telling users to spawn the verifier by hand. Report: ``` diff --git a/tests/emitted-drift-acks/2930-fragmentize-execute-phase-markers.json b/tests/emitted-drift-acks/2930-fragmentize-execute-phase-markers.json index 056751ee0..b116a1cc1 100644 --- a/tests/emitted-drift-acks/2930-fragmentize-execute-phase-markers.json +++ b/tests/emitted-drift-acks/2930-fragmentize-execute-phase-markers.json @@ -1,6 +1,6 @@ { "version": 1, "paths": { - "execute-phase.md": "#2930 (epic #1671 Phase 3): pilots the in-file `` marker grammar by wrapping the --wave/gap-closure/regression-gate branch sections (partial-wave, gap-closure-artifacts, regression-gate) in marker pairs, proving the composeWorkflow seam runs at install time before per-runtime rewrites. Retargeted from plan-phase.md (chore/2930 review): plan-phase.md sits only 36 B under the ADR-857 Phase-6 PRE_PHASE6 gate (tests/phase6-capstone-conformance.test.cjs) and cannot absorb marker overhead, so the maintainer retargeted the pilot to execute-phase.md, which has 728 B of headroom under its own PRE_PHASE6 cap. SOURCE grows by exactly 275 marker bytes (6 marker lines); the EMITTED artifact composeWorkflow produces at install is byte-identical to the pre-#2930 file (markers are stripped, never shipped). See .gsd/phase/chore-2930-fragmentize-xl-workflow/40-design.md 'Known limits' item 5. #2639: handle_branching now warns when local is ahead of origin (+376 B condensed one-line WARNING + rev-list --count check). #2993 (epic #1671 Phase 6.2): fixes the sibling gap #2932 shipped — `flag:--wave` section gating was added to the WHEN_VOCABULARY and to init.execute-phase's section manifest, but execute-phase.md never actually parsed `--wave` out of `$ARGUMENTS` or forwarded it on the `gsd_run query init.execute-phase` line, so the flag could never fire. This diff adds a WAVE_PARAM extraction (`--wave ` via BASH_REMATCH) and appends it to the init call, growing the file 163 bytes (89,507 -> 89,670). #2830: the discover_and_group_plans step gained the halt-aware skip rule — plans whose blocked_by is non-empty are skipped in addition to the existing has_summary skip, and each is reported BY NAME with its cause (\"Skipping {id}: blocked by halted {…}\") rather than silently vanishing from the executable list. Growth is the added rule plus the blocked_by/runnable fields in the documented phase-plan-index parse contract, growing the file 518 bytes (89,670 -> 90,188); no other content changed." + "execute-phase.md": "#2930 (epic #1671 Phase 3): pilots the in-file `` marker grammar by wrapping the --wave/gap-closure/regression-gate branch sections (partial-wave, gap-closure-artifacts, regression-gate) in marker pairs, proving the composeWorkflow seam runs at install time before per-runtime rewrites. Retargeted from plan-phase.md (chore/2930 review): plan-phase.md sits only 36 B under the ADR-857 Phase-6 PRE_PHASE6 gate (tests/phase6-capstone-conformance.test.cjs) and cannot absorb marker overhead, so the maintainer retargeted the pilot to execute-phase.md, which has 728 B of headroom under its own PRE_PHASE6 cap. SOURCE grows by exactly 275 marker bytes (6 marker lines); the EMITTED artifact composeWorkflow produces at install is byte-identical to the pre-#2930 file (markers are stripped, never shipped). See .gsd/phase/chore-2930-fragmentize-xl-workflow/40-design.md 'Known limits' item 5. #2639: handle_branching now warns when local is ahead of origin (+376 B condensed one-line WARNING + rev-list --count check). #2993 (epic #1671 Phase 6.2): fixes the sibling gap #2932 shipped — `flag:--wave` section gating was added to the WHEN_VOCABULARY and to init.execute-phase's section manifest, but execute-phase.md never actually parsed `--wave` out of `$ARGUMENTS` or forwarded it on the `gsd_run query init.execute-phase` line, so the flag could never fire. This diff adds a WAVE_PARAM extraction (`--wave ` via BASH_REMATCH) and appends it to the init call, growing the file 163 bytes (89,507 -> 89,670). #2830: the discover_and_group_plans step gained the halt-aware skip rule — plans whose blocked_by is non-empty are skipped in addition to the existing has_summary skip, and each is reported BY NAME with its cause (\"Skipping {id}: blocked by halted {…}\") rather than silently vanishing from the executable list. Growth is the added rule plus the blocked_by/runnable fields in the documented phase-plan-index parse contract, growing the file 518 bytes (89,670 -> 90,188); no other content changed. #2868: the discover_and_group_plans step no longer exits unconditionally when every plan is filtered out — it now checks whether the phase ever produced a VERIFICATION.md and, when one is missing on an unfiltered run, resumes at the tail gates (code_review_gate -> close_parent_artifacts -> regression_gate -> verify_phase_goal) instead of stranding the phase. Growth is that rule plus its filter-active and already-verified guards; no other content changed." } } diff --git a/tests/execute-phase-wave.test.cjs b/tests/execute-phase-wave.test.cjs index b3ed639aa..38579dfa8 100644 --- a/tests/execute-phase-wave.test.cjs +++ b/tests/execute-phase-wave.test.cjs @@ -105,6 +105,128 @@ describe('execute-phase workflow: wave filtering', () => { }); }); +// #2868: a phase whose plans are ALL summarized but which never reached +// verify_phase_goal (most commonly a retired checkpoint plan that still wrote a +// SUMMARY) must resume at the phase gates instead of exiting unconditionally — +// the prior behavior made `code_review_gate`, `regression_gate`, and +// `verify_phase_goal` (the only producer of *-VERIFICATION.md) unreachable. +describe('execute-phase workflow: #2868 stranded-phase resume on discover_and_group_plans', () => { + test('W1: all-filtered outcome is no longer an unconditional exit; it consults verification status', () => { + const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + assert.ok( + !content.includes('If all filtered: "No matching incomplete plans" → exit.'), + 'the old unconditional all-filtered exit line must be gone (#2868)' + ); + assert.ok( + content.includes('VERIFY_STATUS'), + 'discover_and_group_plans should consult VERIFY_STATUS before exiting on all-filtered' + ); + assert.ok( + content.includes('verification status'), + 'discover_and_group_plans should call the verification status query' + ); + }); + + test('W2: the resume path names both code_review_gate and regression_gate', () => { + const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + const discoverIdx = content.indexOf(''); + const discoverEnd = content.indexOf('', discoverIdx) + ''.length; + assert.ok(discoverIdx >= 0, 'discover_and_group_plans step should exist'); + const discoverSection = content.substring(discoverIdx, discoverEnd); + + assert.ok( + discoverSection.includes('code_review_gate'), + 'discover_and_group_plans should name code_review_gate as the resume target' + ); + assert.ok( + discoverSection.includes('regression_gate'), + 'discover_and_group_plans should name regression_gate so a future rename breaks this test ' + + 'instead of silently orphaning the resume path' + ); + }); + + test('W3: the resume path is gated off when a filter is active (--gaps-only or WAVE_FILTER)', () => { + const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + const discoverIdx = content.indexOf(''); + const discoverEnd = content.indexOf('', discoverIdx) + ''.length; + assert.ok(discoverIdx >= 0, 'discover_and_group_plans step should exist'); + const discoverSection = content.substring(discoverIdx, discoverEnd); + + const filterIdx = discoverSection.indexOf('A filter is active'); + assert.ok(filterIdx >= 0, 'discover_and_group_plans should describe a filter-active branch'); + // Both flags must be mentioned near the filter-active branch, not merely + // anywhere in the step (e.g. in the pre-existing filtering prose above). + const filterClause = discoverSection.substring(filterIdx, filterIdx + 200); + assert.ok( + filterClause.includes('--gaps-only'), + 'filter-active branch should mention --gaps-only' + ); + assert.ok( + filterClause.includes('WAVE_FILTER'), + 'filter-active branch should mention WAVE_FILTER' + ); + }); + + test('W4: the resume decision is gated on the absence of blocked_by-skipped plans', () => { + const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + const discoverIdx = content.indexOf(''); + const discoverEnd = content.indexOf('', discoverIdx) + ''.length; + assert.ok(discoverIdx >= 0, 'discover_and_group_plans step should exist'); + const discoverSection = content.substring(discoverIdx, discoverEnd); + + // Scope to the resume-decision text specifically (from the "If all filtered" marker + // onward), not the pre-existing #2830 filtering prose above it that already mentions + // blocked_by unconditionally — otherwise this assertion would be vacuous. + const decisionIdx = discoverSection.indexOf('If all filtered'); + assert.ok(decisionIdx >= 0, 'discover_and_group_plans should have an all-filtered decision block'); + const decisionText = discoverSection.substring(decisionIdx); + + assert.ok( + decisionText.includes('blocked_by'), + 'the resume-decision text must reference blocked_by so an all-blocked phase is never ' + + 'reported as finished (#2868 finding 1)' + ); + assert.ok( + /stuck/i.test(decisionText), + 'the resume-decision text must call out the blocked-and-incomplete case as stuck, ' + + 'distinct from genuinely finished' + ); + }); + + test('W5: the resume path enters at aggregate_results, not code_review_gate', () => { + const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + const discoverIdx = content.indexOf(''); + const discoverEnd = content.indexOf('', discoverIdx) + ''.length; + assert.ok(discoverIdx >= 0, 'discover_and_group_plans step should exist'); + const discoverSection = content.substring(discoverIdx, discoverEnd); + + const continueMatch = discoverSection.match(/continue (?:directly )?at\s+`([a-zA-Z_]+)`/); + assert.ok(continueMatch, 'resume decision should state which step it continues at'); + assert.strictEqual( + continueMatch[1], + 'aggregate_results', + 'the resume path must enter at aggregate_results (the only step running the ' + + 'SECURITY_FILE / secure-phase threats-open gate), not code_review_gate — skipping ' + + 'aggregate_results silently drops the only security gate (#2868 finding 3)' + ); + assert.notStrictEqual( + continueMatch[1], + 'code_review_gate', + 'resume entry point must not be code_review_gate' + ); + }); + + test('W6: RESUME_TAIL_ONLY (dead, write-only state) must not appear anywhere in the workflow', () => { + const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + assert.ok( + !content.includes('RESUME_TAIL_ONLY'), + 'RESUME_TAIL_ONLY was set but never read anywhere in the workflow or its steps files ' + + '(#2868 finding 2) — remove it; the imperative instruction at the decision point is ' + + 'what actually carries control flow' + ); + }); +}); + describe('execute-phase docs: user-facing wave flag', () => { test('COMMANDS.md documents --wave usage', () => { const content = fs.readFileSync(COMMANDS_DOC_PATH, 'utf-8'); diff --git a/tests/verification-status.test.cjs b/tests/verification-status.test.cjs index 389df8851..f74de9521 100644 --- a/tests/verification-status.test.cjs +++ b/tests/verification-status.test.cjs @@ -1050,3 +1050,76 @@ describe('#2617: the phase-complete error path projects too', () => { }); } }); + +// ─── #2868: stranded-phase detection via `verification status` ──────────────── +// +// execute-phase's `discover_and_group_plans` step resumes at the phase gates +// when every plan is summarized but no *-VERIFICATION.md exists yet. That +// resume decision is driven by `gsd_run query verification status +// --pick status` reading `missing`. These tests pin the CLI query's behavior +// on the exact fixture shapes the workflow branches on, via the real CLI +// (runGsdTools), not the in-process readVerificationStatus() helper used above. +describe('#2868: verification status CLI drives the execute-phase stranded-phase resume', () => { + const { runGsdTools, createTempGitProject } = require('./helpers.cjs'); + + test('D1: all plans summarized, no *-VERIFICATION.md → status is missing', () => { + const projectDir = createTempGitProject(); + try { + const phaseDir = path.join(projectDir, '.planning', 'phases', '01-example'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '01-01-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(phaseDir, '01-01-SUMMARY.md'), '# Summary\n'); + + const res = runGsdTools(['verification', 'status', phaseDir, '--pick', 'status'], projectDir); + assert.equal(res.success, true, `verification status should succeed: ${res.error}`); + assert.equal(res.output, 'missing', 'no VERIFICATION.md at all → status must be missing'); + } finally { + cleanup(projectDir); + } + }); + + test('D2: same fixture plus a passed *-VERIFICATION.md → status is not missing', () => { + const projectDir = createTempGitProject(); + try { + const phaseDir = path.join(projectDir, '.planning', 'phases', '01-example'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '01-01-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(phaseDir, '01-01-SUMMARY.md'), '# Summary\n'); + fs.writeFileSync( + path.join(phaseDir, '01-VERIFICATION.md'), + '---\nstatus: passed\n---\n\n# Verification\n', + ); + + const res = runGsdTools(['verification', 'status', phaseDir, '--pick', 'status'], projectDir); + assert.equal(res.success, true, `verification status should succeed: ${res.error}`); + assert.notEqual(res.output, 'missing', 'a passed VERIFICATION.md must not read as missing'); + assert.equal(res.output, 'passed'); + } finally { + cleanup(projectDir); + } + }); + + test('D3: one plan lacking a SUMMARY and no verification → still missing (not conflated with "stranded")', () => { + const projectDir = createTempGitProject(); + try { + const phaseDir = path.join(projectDir, '.planning', 'phases', '01-example'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '01-01-PLAN.md'), '# Plan 1\n'); + fs.writeFileSync(path.join(phaseDir, '01-01-SUMMARY.md'), '# Summary 1\n'); + // 01-02 has a PLAN but no SUMMARY — plan work is still outstanding, which is + // a different condition from the phase being "stranded" (all plans done, + // verification never ran). The query must not conflate the two. + fs.writeFileSync(path.join(phaseDir, '01-02-PLAN.md'), '# Plan 2\n'); + + const res = runGsdTools(['verification', 'status', phaseDir, '--pick', 'status'], projectDir); + assert.equal(res.success, true, `verification status should succeed: ${res.error}`); + assert.equal( + res.output, + 'missing', + 'outstanding plan work must not change verification status away from missing', + ); + } finally { + cleanup(projectDir); + } + }); +});