From c90ae479f96bffd3d424350f82c450e17e2e7afb Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 14 Aug 2026 11:30:39 -0400 Subject: [PATCH] fix(#3350): prefer lowest outstanding phase over positional next in phase complete (#3482) * fix(#3350): prefer lowest outstanding phase over positional next in phase complete * chore(#3350): add changeset fragment * chore(#3350): backfill changeset pr field --------- Co-authored-by: sim --- .changeset/sturdy-hawks-munch.md | 5 + src/phase.cts | 38 +++- tests/phase.test.cjs | 296 ++++++++++++++++++++++++++++++- 3 files changed, 331 insertions(+), 8 deletions(-) create mode 100644 .changeset/sturdy-hawks-munch.md diff --git a/.changeset/sturdy-hawks-munch.md b/.changeset/sturdy-hawks-munch.md new file mode 100644 index 000000000..e1e42fbd8 --- /dev/null +++ b/.changeset/sturdy-hawks-munch.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3482 +--- +phase complete now selects the lowest genuinely-outstanding lower-numbered phase as next_phase instead of a merely-positionally-next higher phase heading, and keeps STATE.md frontmatter current_phase and current_phase_name paired (both describe the same phase) even for narrative-prose STATE.md files diff --git a/src/phase.cts b/src/phase.cts index 5e7c9abe6..f745f1a5d 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -2899,7 +2899,17 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { // pattern mirrors the sibling phasePattern's anchoring (only whitespace/bold // between the box and "Phase", a required `:`) so unrelated checklist lines // that merely mention "Phase N" don't match. - if (isLastPhase && roadmapContent !== null) { + // #3350: this stage answers a DIFFERENT question than stages 1-2 ("what is + // the next actionable phase?" vs "is this the last phase?"), so it must not + // be gated on their answer. Gating on isLastPhase let a merely-positionally + // next higher heading (stage 2) permanently mask a genuinely-outstanding + // lower phase — stage 2 cleared isLastPhase and this scan never ran. The + // scan already refuses anything not strictly lower than the completed phase + // (plus sentinels, #2949), so running it unconditionally cannot manufacture + // a wrong answer: when no lower phase is outstanding it finds nothing and + // stages 1-2's pick stands unchanged; in the masking case isLastPhase is + // already false, so the last-phase signal has no reachable regression. + if (roadmapContent !== null) { try { const milestoneScope = extractCurrentMilestone(roadmapContent, cwd); const cbPattern = new RegExp( @@ -2988,11 +2998,27 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { // the intent; pass it as authoritative so the sync's prose // re-derivation cannot rewrite current_phase_name to the name's own // parenthetical (`Closer-ruling measurement (D1a)` → `D1a`). - stateContent = syncStateFrontmatter( - stateContent, - cwd, - nextPhaseDisplayName ? { current_phase_name: nextPhaseDisplayName } : undefined, - ); + // #3350: PAIR the override. When STATE.md's body carries no Current + // Phase / Phase field to re-derive from (narrative prose), the #905 + // preserve guard in syncStateFrontmatter keeps the OLD frontmatter + // current_phase while the authoritative current_phase_name advances — + // leaving the two fields describing different phases. Pin BOTH to the + // resolved next phase in that case. When the body DOES carry the field + // (completePhaseCore just rewrote it), stay name-only so the body's + // richer `N of T (name)` derived shape survives the sync. + const fmBody = frontmatterMod.stripFrontmatter(stateContent); + const bodyHasPhaseField = + stateExtractField(fmBody, 'Current Phase') != null || + stateExtractField(fmBody, 'Phase') != null; + const authoritativeFm: Record | undefined = nextPhaseDisplayName + ? bodyHasPhaseField || !nextPhaseNum + ? { current_phase_name: nextPhaseDisplayName } + : { + current_phase: String(nextPhaseNum), + current_phase_name: nextPhaseDisplayName, + } + : undefined; + stateContent = syncStateFrontmatter(stateContent, cwd, authoritativeFm); writes.push({ filePath: statePath, before: originalStateContent, after: stateContent }); } diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index d787e30ae..b2f22dd79 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -5232,8 +5232,14 @@ describe('phase complete excludes 999.x backlog from next-phase (#2129)', () => assert.ok(result.success, `Command failed: ${result.error}`); const output = JSON.parse(result.output); - // Should find phase 3 from roadmap, NOT 999.1 from filesystem - assert.strictEqual(output.next_phase, '3', 'next_phase should be 3, not 999.1'); + // #3350: with stage 3 (#2028 lowest-outstanding) no longer gated behind + // stages 1-2 missing, the unchecked Phase 1 row outranks the positionally- + // next Phase 3 heading — a phase is complete iff its roadmap checkbox is + // `[x]` (#2028), and Phase 1's row is unchecked despite its dir on disk + // (same drift shape #2949's realLowerOutstandingPhaseStillSelected pins). + // The #2129 contract itself is unchanged: 999.x is NEVER selected. + assert.notEqual(output.next_phase, '999.1', '999.x backlog must never be next_phase'); + assert.strictEqual(output.next_phase, '1', 'lowest unchecked phase (1) wins over positional 3 (#3350); never 999.1'); assert.strictEqual(output.is_last_phase, false, 'should not be last phase'); }); }); @@ -10967,3 +10973,289 @@ describe('phase complete stage-3 sentinel filter (#2949)', () => { }); } })(); + +// #3350 — phase complete: a merely-positionally-next higher phase heading +// (stage 2) must not mask a genuinely-outstanding lower phase (stage 3), and +// STATE.md frontmatter's current_phase/current_phase_name must stay paired. +// Matrix: .gsd/bug/fix-3350-phase-complete-name-desync/50-test-matrix.md +describe('phase complete lowest-outstanding vs positional-next (#3350)', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject('gsd-3350-'); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + /** Scaffold a phase dir with one executed plan (PLAN + SUMMARY). */ + function scaffoldPhase(slug, planNum) { + const dir = path.join(tmpDir, '.planning', 'phases', slug); + fs.mkdirSync(dir, { recursive: true }); + const padded = String(planNum).padStart(2, '0'); + fs.writeFileSync(path.join(dir, `${padded}-01-PLAN.md`), '# Plan'); + fs.writeFileSync(path.join(dir, `${padded}-01-SUMMARY.md`), '# Summary'); + } + + const statePath = () => path.join(tmpDir, '.planning', 'STATE.md'); + + /** + * Row-4 roadmap: phase 5 is completed (dir on disk); phases 3 and 4 are + * outstanding (unchecked, never executed, no dirs — optional); higher + * headings 6 and 7 are pre-declared with no dirs (stage-1 miss, stage-2 hit). + */ + function writeRow4Roadmap({ withLowerDirs = false, withHigherDir = false } = {}) { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap + +- [ ] **Phase 3: Attestation Freeze** +- [ ] **Phase 4: Corpus Maturation** +- [ ] **Phase 5: Harness Mechanization** +- [ ] **Phase 6: Oracle Re-Hardening** +- [ ] **Phase 7: Recall Monitoring** + +### Phase 3: Attestation Freeze +**Goal:** three +**Plans:** 1 plans + +### Phase 4: Corpus Maturation +**Goal:** four +**Plans:** 1 plans + +### Phase 5: Harness Mechanization +**Goal:** five +**Plans:** 1 plans + +### Phase 6: Oracle Re-Hardening +**Goal:** six +**Plans:** 1 plans + +### Phase 7: Recall Monitoring +**Goal:** seven +**Plans:** 1 plans +`, + ); + scaffoldPhase('05-harness-mechanization', 5); + if (withLowerDirs) { + scaffoldPhase('03-attestation-freeze', 3); + scaffoldPhase('04-corpus-maturation', 4); + } + if (withHigherDir) { + scaffoldPhase('06-oracle-re-hardening', 6); + } + } + + /** Explicit-field STATE.md (body fields present — the 0xdhx fixture shape). */ + function writeExplicitState() { + fs.writeFileSync( + statePath(), + `# State + +**Current Phase:** 5 +**Current Phase Name:** Harness Mechanization +**Status:** In progress +**Current Plan:** 05-01 +**Last Activity:** 2025-01-01 +**Last Activity Description:** Working on phase 5 +`, + ); + } + + /** + * Narrative-prose STATE.md (NO body fields) with frontmatter parked on the + * just-completed phase — the filed #3350 shape: without the pairing override + * current_phase stays at 5 while current_phase_name advances. + */ + function writeNarrativeState() { + fs.writeFileSync( + statePath(), + `--- +current_phase: 5 +current_phase_name: Harness Mechanization +--- + +# State + +We are wrapping up phase 5 of the milestone; the next actionable phase is +still to be determined by the roadmap. +`, + ); + } + + function parseFrontmatterField(content, key) { + const m = content.match(new RegExp(`^${key}:\\s*(.*)$`, 'm')); + return m ? m[1].trim().replace(/^["']|["']$/g, '') : null; + } + + test('lowerOutstandingBeatsHigherHeading', () => { + // Row 4 (failing-first): completing 5 with outstanding 3/4 AND pre-declared + // higher headings 6/7 must pick the lowest outstanding phase (3), not the + // positionally-next heading (6). + writeRow4Roadmap(); + writeExplicitState(); + + const result = runVerifiedPhaseComplete('phase complete 5', tmpDir); + assert.ok(result.success, `Command failed: ${result.error || result.output}`); + const output = JSON.parse(result.output); + + assert.strictEqual(output.completed_phase, '5'); + assert.strictEqual(output.is_last_phase, false, 'a higher phase exists — not last'); + assert.strictEqual( + parseInt(String(output.next_phase), 10), + 3, + `lowest outstanding phase 3 must be next_phase (got ${output.next_phase})`, + ); + assert.strictEqual(output.next_phase_name, 'attestation-freeze'); + }); + + test('narrativeStateFrontmatterPairing', () => { + // Row 4 + acceptance #2/#5 (failing-first): with a narrative STATE.md and + // frontmatter parked on the completed phase, BOTH frontmatter fields must + // describe the resolved next phase (3) after the write — never a split. + writeRow4Roadmap(); + writeNarrativeState(); + + const result = runVerifiedPhaseComplete('phase complete 5', tmpDir); + assert.ok(result.success, `Command failed: ${result.error || result.output}`); + const output = JSON.parse(result.output); + assert.strictEqual(parseInt(String(output.next_phase), 10), 3); + + const state = fs.readFileSync(statePath(), 'utf-8'); + const fmPhase = parseFrontmatterField(state, 'current_phase'); + const fmName = parseFrontmatterField(state, 'current_phase_name'); + assert.ok(fmPhase !== null, 'frontmatter current_phase must exist'); + assert.ok(fmName !== null, 'frontmatter current_phase_name must exist'); + assert.strictEqual( + parseInt(fmPhase, 10), + 3, + `current_phase must advance to the resolved next phase 3 (got ${fmPhase})`, + ); + assert.match(fmName, /attestation freeze/i, `current_phase_name must name phase 3 (got ${fmName})`); + }); + + test('higherStillWinsWhenNoLowerOutstanding', () => { + // Row 2 non-regression (acceptance #4): N+k really is the correct next phase + // when no lower phase is genuinely outstanding. + writeRow4Roadmap(); + writeExplicitState(); + // Check phases 3 and 4 off — the higher heading is then the right answer. + const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md'); + fs.writeFileSync( + roadmapPath, + fs.readFileSync(roadmapPath, 'utf-8') + .replace('- [ ] **Phase 3: Attestation Freeze**', '- [x] **Phase 3: Attestation Freeze**') + .replace('- [ ] **Phase 4: Corpus Maturation**', '- [x] **Phase 4: Corpus Maturation**'), + ); + + const result = runVerifiedPhaseComplete('phase complete 5', tmpDir); + assert.ok(result.success, `Command failed: ${result.error || result.output}`); + const output = JSON.parse(result.output); + assert.strictEqual(parseInt(String(output.next_phase), 10), 6, 'positionally-next 6 wins when no lower phase is outstanding'); + assert.strictEqual(output.is_last_phase, false); + }); + + test('lowerOnlyStillSelected', () => { + // Row 3 non-regression (#2028 original shape): lower outstanding, no higher. + writeRow4Roadmap(); + writeExplicitState(); + const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md'); + let roadmap = fs.readFileSync(roadmapPath, 'utf-8'); + roadmap = roadmap + .replace('- [ ] **Phase 6: Oracle Re-Hardening**\n', '') + .replace('- [ ] **Phase 7: Recall Monitoring**\n', '') + .replace('### Phase 6: Oracle Re-Hardening\n**Goal:** six\n**Plans:** 1 plans\n\n', '') + .replace('### Phase 7: Recall Monitoring\n**Goal:** seven\n**Plans:** 1 plans\n', ''); + + fs.writeFileSync(roadmapPath, roadmap); + + const result = runVerifiedPhaseComplete('phase complete 5', tmpDir); + assert.ok(result.success, `Command failed: ${result.error || result.output}`); + const output = JSON.parse(result.output); + assert.strictEqual(parseInt(String(output.next_phase), 10), 3, '#2028 behavior preserved'); + assert.strictEqual(output.is_last_phase, false); + }); + + test('nothingOutstandingStillCompletesMilestone', () => { + // Row 1 non-regression: no higher phase (heading or dir) and no unchecked + // lower phase → is_last_phase stays true. Higher HEADINGS alone keep + // is_last_phase false (stage 2 matches headings regardless of checkbox + // state), so this fixture drops them entirely. + writeRow4Roadmap(); + writeExplicitState(); + const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md'); + let roadmap = fs.readFileSync(roadmapPath, 'utf-8'); + roadmap = roadmap + .replace('- [ ] **Phase 3: Attestation Freeze**', '- [x] **Phase 3: Attestation Freeze**') + .replace('- [ ] **Phase 4: Corpus Maturation**', '- [x] **Phase 4: Corpus Maturation**') + .replace('- [ ] **Phase 6: Oracle Re-Hardening**\n', '') + .replace('- [ ] **Phase 7: Recall Monitoring**\n', '') + .replace('### Phase 6: Oracle Re-Hardening\n**Goal:** six\n**Plans:** 1 plans\n\n', '') + .replace('### Phase 7: Recall Monitoring\n**Goal:** seven\n**Plans:** 1 plans\n', ''); + fs.writeFileSync(roadmapPath, roadmap); + + const result = runVerifiedPhaseComplete('phase complete 5', tmpDir); + assert.ok(result.success, `Command failed: ${result.error || result.output}`); + const output = JSON.parse(result.output); + assert.strictEqual(output.is_last_phase, true, 'nothing outstanding — milestone completes'); + assert.strictEqual(output.next_phase, null); + }); + + test('sentinelStillExcludedWithHigherPresent', () => { + // Acceptance #3: the #2949 sentinel filter keeps working alongside the + // ungate — an unchecked 0.x backlog row never becomes next_phase. + writeRow4Roadmap(); + writeExplicitState(); + const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md'); + fs.writeFileSync( + roadmapPath, + '- [ ] **Phase 0.1: Backlog sentinel item**\n' + fs.readFileSync(roadmapPath, 'utf-8'), + ); + + const result = runVerifiedPhaseComplete('phase complete 5', tmpDir); + assert.ok(result.success, `Command failed: ${result.error || result.output}`); + const output = JSON.parse(result.output); + assert.strictEqual( + parseInt(String(output.next_phase), 10), + 3, + `real phase 3 (not the 0.1 sentinel) is next_phase (got ${output.next_phase})`, + ); + }); + + test('explicitBodyFieldsMoveTogether', () => { + // Row 4 explicit-field variant (0xdhx fixture A): the body fields AND the + // frontmatter must move to phase 3 together — never both-wrong on 6. + writeRow4Roadmap(); + writeExplicitState(); + + const result = runVerifiedPhaseComplete('phase complete 5', tmpDir); + assert.ok(result.success, `Command failed: ${result.error || result.output}`); + const output = JSON.parse(result.output); + assert.strictEqual(parseInt(String(output.next_phase), 10), 3); + + const state = fs.readFileSync(statePath(), 'utf-8'); + assert.ok(/\*\*Current Phase:\*\*\s*3\b/.test(state), `body Current Phase must be 3, got: ${state.match(/\*\*Current Phase:\*\*.*/)?.[0]}`); + assert.match(state, /\*\*Current Phase Name:\*\*\s*Attestation Freeze/); + const fmPhase = parseFrontmatterField(state, 'current_phase'); + const fmName = parseFrontmatterField(state, 'current_phase_name'); + assert.strictEqual(parseInt(fmPhase, 10), 3, `frontmatter current_phase must derive to 3 (got ${fmPhase})`); + assert.match(fmName, /attestation freeze/i, `frontmatter current_phase_name must name phase 3 (got ${fmName})`); + }); + + test('diskPresentHigherStillLosesToLowerOutstanding', () => { + // Row 4 stage-1 variant (failing-first): a higher phase directory ON DISK + // (stage-1 hit) also must not mask an outstanding lower phase. + writeRow4Roadmap({ withHigherDir: true }); + writeExplicitState(); + + const result = runVerifiedPhaseComplete('phase complete 5', tmpDir); + assert.ok(result.success, `Command failed: ${result.error || result.output}`); + const output = JSON.parse(result.output); + assert.strictEqual( + parseInt(String(output.next_phase), 10), + 3, + `lowest outstanding phase 3 beats the on-disk higher phase 6 (got ${output.next_phase})`, + ); + }); +});