From bcefffc13224a6485a78725d9e5ba1c8d4ad3f60 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 18 Aug 2026 08:33:46 -0400 Subject: [PATCH] fix(#3578): derive milestone status from phase counters, not phase-completion prose (#3614) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(3578): failing-first coverage for milestone status on partial completion Completing phase 2 of a 4-phase milestone sets frontmatter status: completed while the same call correctly writes completed_phases: 2 / total_phases: 4. These tests fail on that conflation and pin the boundary either side of it (3-of-4 must not complete, 4-of-4 must), plus milestone_name byte-identity and the 1-of-1 case that legitimately does complete. * fix(3578): derive milestone status from phase counters, not phase-completion prose RED proven at 253843b4 (tests-only): the 2-of-4 and 3-of-4 cases failed while the 4-of-4, milestone_name and 1-of-1 controls passed — the conflation, and nothing else. state complete-phase writes body prose `Phase N complete`. normalizeStateStatus matches 'complete' as a case-insensitive SUBSTRING, so phase-level prose collapsed into milestone-level frontmatter status: completed — even while the same call correctly derived completed_phases: 2 / total_phases: 4 / percent: 50. Check ORDER is why the sibling surface stays correct: completePhaseCore writes 'Ready to plan' for non-final phases, hitting the 'planning' arm before 'complete'. The two phase-completion surfaces disagreed and this was the conflated one — a violation of ADR-2207, which gives milestone termination solely to milestoneCompleteCore. buildStateFrontmatter now honors a 'completed' normalization from phase-completion prose only when the counters it already derived agree. Scoped deliberately: - anchored to bare `Phase complete`, so 'All phases complete' and ' milestone complete' are untouched (both out of scope). Verified by executing the guard's own regex from source against both forms. - gated on counter trustworthiness (COMPLETE disk scope, finite counts, positive denominator) so an unknown scope withholds rather than guessing 'not complete', which would be the mirror-image bug - normalizeStateStatus itself is NOT modified — it feeds every state.* write and the read path A 1-of-1 milestone still yields 'completed' by the rule, not by exemption, so the #1255 pinning test stays green on its merits. Fixes #3578 * fix(3578): gate the guard on milestone boundedness and close the review gaps Review findings from two orthogonal passes, all fixed inline. GUARD (correctness, from the standards pass): the guard omitted `milestoneUnbounded`, which is the established trust authority for these very counters in this same function — it nulls progressPercent at :2286 and gates the prose fallback at :2294. An unbounded milestone yields a conflated/understated total, so `completedPhases < totalPhases` could be an artifact of a bad denominator and demote a genuinely-complete milestone. Now gated. TESTS: - Prose/guard parity assertion. The guard regex-matches prose emitted from a DIFFERENT file; if that prose drifts the guard silently stops firing and the bug returns undetected. Per the repo's generative-fix-divergence rule, a test now asserts the emitted body Status still matches the guard's pattern — asserting the emitted value against the pattern rather than duplicating the string. - limit+1: completedPhases > totalPhases must NOT fire; inconsistent counters fall through rather than guessing. - Untrustworthy counters (no phases dir → totalPhases null) must NOT fire. - AC4: MCP invoke-command dispatch parity via handleMessage, the criterion both reviewers independently flagged as asserted-but-untested. - Hand-rolled STATE.md writes routed through the existing writeState fixture helper. The adversarial pass independently verified, by reading rather than trusting the diff's own comments, that: paused/stopped short-circuit before 'completed' so a paused milestone can never be clobbered; only cmdStateCompletePhase emits the targeted prose, so no sibling caller over-fires; the counters come from a fresh disk scan independent of this write, so there is no pre/post off-by-one; and the #1255 pinning fixture creates no phases dir, leaving completedPhases null and the guard inert — so that test is provably unaffected rather than assumed to be. * chore(3578): add changeset fragment * chore(3578): backfill changeset PR number (#3614) --------- Co-authored-by: sim --- .changeset/fierce-lemurs-forage.md | 5 + src/state.cts | 34 ++- tests/state.test.cjs | 372 ++++++++++++++++++++++++++++- 3 files changed, 409 insertions(+), 2 deletions(-) create mode 100644 .changeset/fierce-lemurs-forage.md diff --git a/.changeset/fierce-lemurs-forage.md b/.changeset/fierce-lemurs-forage.md new file mode 100644 index 000000000..ad508f108 --- /dev/null +++ b/.changeset/fierce-lemurs-forage.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3614 +--- +**Completing one phase no longer marks the whole milestone done** — `state complete-phase` wrote the body prose `Phase N complete`, and the status normalizer matches `complete` as a substring, so finishing phase 2 of 4 collapsed the milestone-level STATE.md frontmatter to `status: completed` while the very same call correctly recorded `completed_phases: 2` of `total_phases: 4`. Downstream automation that gates on milestone status — auto-advance, archival, ship gating — was told a half-open milestone was finished. Milestone status is now derived from those counters instead of from phase-level prose. (#3578) diff --git a/src/state.cts b/src/state.cts index 6a24a1210..e47486b1a 100644 --- a/src/state.cts +++ b/src/state.cts @@ -2296,7 +2296,39 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto if (pctMatch) progressPercent = parseInt(pctMatch[1], 10); } - const normalizedStatus = normalizeStateStatus(status, pausedAt); + let normalizedStatus = normalizeStateStatus(status, pausedAt); + // #3578: normalizeStateStatus matches 'complete' as a case-insensitive + // SUBSTRING, so the phase-completion prose cmdStateCompletePhase writes to + // the body (`Phase ${N} complete`) collapses to the milestone-level + // 'completed' status even when other phases remain open. Phase-level + // prose must never decide milestone-level status — completedPhases / + // totalPhases / diskScope, already derived above from a disk scan, are + // the authority on whether the MILESTONE is actually done. Only override + // when: (a) normalizeStateStatus actually landed on 'completed'; (b) the + // raw prose is UNAMBIGUOUSLY phase-completion prose — the anchored + // pattern below deliberately excludes "All phases complete" (no `\S+` + // phase token) and milestone-close prose like "v1.0 milestone complete" + // (no leading "phase"); and (c) the counters are trustworthy (a COMPLETE + // disk scope, both counts are finite numbers, and a positive + // denominator) and affirmatively disagree with 'completed'. In every + // other case normalizedStatus is left exactly as normalizeStateStatus + // returned it. + if ( + normalizedStatus === 'completed' && + typeof status === 'string' && + /^\s*phase\s+\S+\s+complete\s*$/i.test(status) && + diskScope === SCOPE.COMPLETE && + // #1761: an unbounded milestone yields a conflated/understated total — the + // same authority that nulls progressPercent above. Without this, a bad + // denominator could demote a genuinely-complete milestone. + !milestoneUnbounded && + typeof completedPhases === 'number' && Number.isFinite(completedPhases) && + typeof totalPhases === 'number' && Number.isFinite(totalPhases) && + totalPhases > 0 && + completedPhases < totalPhases + ) { + normalizedStatus = 'executing'; + } const fm: Record = { gsd_state_version: '1.0' }; diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 04c6414fa..c7ca28f81 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -12,7 +12,12 @@ const fs = require('fs'); const os = require('os'); const path = require('path'); const { runGsdTools, createTempDir, createTempProject, cleanup } = require('./helpers.cjs'); -const { createFixture, seedWorkstream } = require('./fixtures/index.cjs'); +const { createFixture, seedWorkstream, writeState } = require('./fixtures/index.cjs'); +// #3578 AC4 (MCP dispatch parity): drives the same `state complete-phase` +// command through the gsd_invoke_command MCP tool route instead of the CLI, +// mirroring the gsd-mcp-server.test.cjs `tools/call gsd_invoke_command` +// pattern (family/subcommand/args -> dispatchGsdCommand -> real subprocess). +const { handleMessage } = require('../gsd-core/bin/lib/mcp-server.cjs'); // ADR-3408 §8.3 Matrix A2/A3 (#3469): required fast-check property test — the // composed cmdPhaseComplete/readModifyWriteStateMd write-seam identity. const fc = require('fast-check'); @@ -8808,6 +8813,371 @@ describe('T6 section-splice characterization — complete-phase', () => { }); }); +// ───────────────────────────────────────────────────────────────────────────── +// #3578 — state complete-phase must not overwrite milestone status when other +// phases remain open. `normalizeStateStatus` matches 'complete' as a +// substring, so the phase-completion prose `Phase ${N} complete` collapses to +// the milestone-level 'completed' status even though completed_phases / +// total_phases (computed by the same buildStateFrontmatter call) correctly +// show the milestone is not yet done. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3578: complete-phase does not overwrite milestone status when phases remain open', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createFixture(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + const ROADMAP_4_PHASE = [ + '## Milestone v1.0: Test Milestone', + '', + '### Phase 01: Alpha', + '**Goal:** first', + '', + '### Phase 02: Beta', + '**Goal:** second', + '', + '### Phase 03: Gamma', + '**Goal:** third', + '', + '### Phase 04: Delta', + '**Goal:** fourth', + ].join('\n'); + + const PHASE_DIRS_4 = ['01-alpha', '02-beta', '03-gamma', '04-delta']; + + /** + * Seed `.planning/phases/` for phases 1..4. Every phase gets a + * PLAN.md; phases numbered <= completeThrough additionally get a + * SUMMARY.md and a passing VERIFICATION.md (disk-strict completion, + * ADR-3180 §7.4 / #3186), so isPhaseComplete reports them done. + */ + function seed4PhaseMilestone(completeThrough) { + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), ROADMAP_4_PHASE); + PHASE_DIRS_4.forEach((dirName, idx) => { + const n = idx + 1; + const padded = String(n).padStart(2, '0'); + const phaseDir = path.join(tmpDir, '.planning', 'phases', dirName); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, `${padded}-01-PLAN.md`), '# Plan\n'); + if (n <= completeThrough) { + fs.writeFileSync(path.join(phaseDir, `${padded}-01-SUMMARY.md`), '# Summary\n'); + writePassedVerification(tmpDir, dirName, padded); + } + }); + } + + function writeStateAtPhase(phase, extraBodyLines = []) { + writeState( + tmpDir, + [ + '---', + "gsd_state_version: '1.0'", + 'milestone: v1.0', + 'milestone_name: Test Milestone', + 'status: executing', + '---', + '', + '# GSD State', + '', + '## Configuration', + `Current Phase: ${phase}`, + `Status: Executing Phase ${phase}`, + 'Last Activity: 2026-01-01', + ...extraBodyLines, + '', + ].join('\n'), + ); + } + + function frontmatterStatus(after) { + const fm = frontmatterLib.extractFrontmatter(after); + return fm.status; + } + + test('2 of 4 phases complete on disk: --phase 2 must not set frontmatter status completed', () => { + // On disk, phases 1-2 are already complete (matches the #3578 repro: + // completed_phases/total_phases are correct while status wrongly collapses). + seed4PhaseMilestone(2); + writeStateAtPhase(2); + + const result = runGsdTools(['state', 'complete-phase', '--phase', '2'], tmpDir); + assert.ok(result.success, `complete-phase failed: ${result.error || result.output}`); + + const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.notEqual( + frontmatterStatus(after), + 'completed', + `status must not be 'completed' while phases remain open; got frontmatter:\n${after}`, + ); + + const jsonResult = runGsdTools('state json', tmpDir); + assert.ok(jsonResult.success, `state json failed: ${jsonResult.error}`); + const output = JSON.parse(jsonResult.output); + assert.strictEqual(Number(output.progress.completed_phases), 2, 'completed_phases must still be 2'); + assert.strictEqual(Number(output.progress.total_phases), 4, 'total_phases must still be 4'); + }); + + test('3 of 4 phases complete on disk (limit-1): --phase 3 must not set frontmatter status completed', () => { + seed4PhaseMilestone(3); + writeStateAtPhase(3); + + const result = runGsdTools(['state', 'complete-phase', '--phase', '3'], tmpDir); + assert.ok(result.success, `complete-phase failed: ${result.error || result.output}`); + + const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.notEqual( + frontmatterStatus(after), + 'completed', + `status must not be 'completed' with 3 of 4 phases done; got frontmatter:\n${after}`, + ); + }); + + test('4 of 4 phases complete on disk (limit): --phase 4 sets frontmatter status completed', () => { + seed4PhaseMilestone(4); + writeStateAtPhase(4); + + const result = runGsdTools(['state', 'complete-phase', '--phase', '4'], tmpDir); + assert.ok(result.success, `complete-phase failed: ${result.error || result.output}`); + + const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.strictEqual( + frontmatterStatus(after), + 'completed', + `status must be 'completed' once all 4 phases are done; got frontmatter:\n${after}`, + ); + }); + + test('milestone_name is byte-identical before and after complete-phase (2 of 4 case)', () => { + seed4PhaseMilestone(2); + writeStateAtPhase(2); + + const before = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + const beforeName = frontmatterLib.extractFrontmatter(before).milestone_name; + + const result = runGsdTools(['state', 'complete-phase', '--phase', '2'], tmpDir); + assert.ok(result.success, `complete-phase failed: ${result.error || result.output}`); + + const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + const afterName = frontmatterLib.extractFrontmatter(after).milestone_name; + + assert.strictEqual(beforeName, 'Test Milestone', 'precondition: milestone_name must start as Test Milestone'); + assert.strictEqual( + afterName, + beforeName, + `milestone_name must be byte-identical before/after complete-phase; before=${beforeName}, after=${afterName}`, + ); + }); + + test('1-of-1 milestone: --phase 1 still sets frontmatter status completed (counter rule allows it)', () => { + const ROADMAP_1_PHASE = [ + '## Milestone v2.0: Solo Milestone', + '', + '### Phase 01: Only', + '**Goal:** the only phase', + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), ROADMAP_1_PHASE); + + const phaseDir = path.join(tmpDir, '.planning', 'phases', '01-only'); + 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'); + writePassedVerification(tmpDir, '01-only', '01'); + + writeState( + tmpDir, + [ + '---', + "gsd_state_version: '1.0'", + 'milestone: v2.0', + 'milestone_name: Solo Milestone', + 'status: executing', + '---', + '', + '# GSD State', + '', + '## Configuration', + 'Current Phase: 1', + 'Status: Executing Phase 1', + 'Last Activity: 2026-01-01', + '', + ].join('\n'), + ); + + const result = runGsdTools(['state', 'complete-phase', '--phase', '1'], tmpDir); + assert.ok(result.success, `complete-phase failed: ${result.error || result.output}`); + + const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.strictEqual( + frontmatterStatus(after), + 'completed', + `a genuinely 1-of-1-complete milestone must still report 'completed'; got frontmatter:\n${after}`, + ); + }); + + /** + * Body Status field the guard's regex actually inspects (never the + * frontmatter `status:` scalar the guard *writes*). Reads via the same + * `stateFieldValue` fallback-chain owner (state-document.cjs) the guard's + * caller (buildStateFrontmatter) is built on, scoped to the body only + * (fmKey null) by stripping frontmatter first. + */ + function bodyStatus(after) { + const body = frontmatterLib.stripFrontmatter(after); + return stateDocument.stateFieldValue({}, body, null, 'Status').value; + } + + /** + * Seed `.planning/phases/` for phases 1..4 exactly like + * `seed4PhaseMilestone`, but WITHOUT writing ROADMAP.md at all. Used to + * drive `buildStateFrontmatter`'s roadmap-absent withhold path (#3573), + * which is the only deterministic way to detach `totalPhases` from the + * live disk-scanned total from a fixture. + */ + function seed4PhaseDirsNoRoadmap(completeThrough) { + PHASE_DIRS_4.forEach((dirName, idx) => { + const n = idx + 1; + const padded = String(n).padStart(2, '0'); + const phaseDir = path.join(tmpDir, '.planning', 'phases', dirName); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, `${padded}-01-PLAN.md`), '# Plan\n'); + if (n <= completeThrough) { + fs.writeFileSync(path.join(phaseDir, `${padded}-01-SUMMARY.md`), '# Summary\n'); + writePassedVerification(tmpDir, dirName, padded); + } + }); + } + + // Parity assertion (repo rule: "Generative Fix Divergence" — a shared + // constant/pattern between parallel surfaces needs a test that fails if + // they diverge). The guard's anchored regex + // /^\s*phase\s+\S+\s+complete\s*$/i lives in src/state.cts and hand-copies + // the SHAPE of the prose cmdStateCompletePhase writes to the body Status + // field (`Phase ${N} complete`, gsd-core/bin/lib/state.cjs) rather than + // sharing a constant with it. If that prose is ever reworded, the guard + // silently stops matching and the #3578 regression returns undetected by + // every other test in this block (they only assert the guard's downstream + // EFFECT on frontmatter status, never that its input pattern still fires). + test("#3578 parity: complete-phase's emitted body Status prose still matches the guard's phase-complete pattern", () => { + seed4PhaseMilestone(2); + writeStateAtPhase(2); + + const result = runGsdTools(['state', 'complete-phase', '--phase', '2'], tmpDir); + assert.ok(result.success, `complete-phase failed: ${result.error || result.output}`); + + const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + const status = bodyStatus(after); + assert.match( + status, + /^\s*phase\s+\S+\s+complete\s*$/i, + `emitted body Status prose ("${status}") no longer matches the #3578 guard's pattern in src/state.cts — the guard would silently stop firing`, + ); + }); + + test('completed phase dirs on disk exceed a stale declared total (limit+1 on the completedPhases < totalPhases comparison): guard must not fire', () => { + // No ROADMAP.md at all + a stale "Total Phases: 2" body annotation drives + // the #3573 roadmap-absent withhold path: totalPhases stays pinned at the + // stale body-declared value (2) instead of being replaced by the live + // disk-scanned total, while completedPhases is UNCONDITIONALLY set from + // the disk scan (buildStateFrontmatter) regardless of that withhold — so + // completedPhases (4) ends up greater than totalPhases (2), making the + // guard's `completedPhases < totalPhases` conjunct false (verified by + // direct probe: status lands 'completed' with progress + // {total_phases:2, completed_phases:4}). Note this fixture necessarily + // also drives listMilestonePhaseDirs' own ROADMAP-absent scope to + // non-COMPLETE (same missing file, independent read), so it does not + // purely isolate the counter conjunct from `diskScope === SCOPE.COMPLETE` + // — src/state.cts's withhold-with-a-stale-numeric-total path is only + // reachable via ROADMAP absence, which always drags that second conjunct + // along with it; no fixture can decouple the two under the current + // implementation. Inconsistent counters deliberately fall through to + // normalizeStateStatus's answer rather than guessing which of the two + // disagreeing numbers is correct. + seed4PhaseDirsNoRoadmap(4); + writeStateAtPhase(4, ['Total Phases: 2']); + + const result = runGsdTools(['state', 'complete-phase', '--phase', '4'], tmpDir); + assert.ok(result.success, `complete-phase failed: ${result.error || result.output}`); + + const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.strictEqual( + frontmatterStatus(after), + 'completed', + `guard must not demote when completedPhases > totalPhases; got frontmatter:\n${after}`, + ); + + const jsonResult = runGsdTools('state json', tmpDir); + assert.ok(jsonResult.success, `state json failed: ${jsonResult.error}`); + const output = JSON.parse(jsonResult.output); + assert.strictEqual(Number(output.progress.completed_phases), 4, 'completed_phases must reflect disk truth (4)'); + assert.strictEqual(Number(output.progress.total_phases), 2, 'total_phases must stay pinned at the stale declared value (2)'); + }); + + test('untrustworthy counters (no ROADMAP.md, no derivable total) must not demote status', () => { + // ROADMAP.md absent entirely + an asserted milestone + no body "Total + // Phases" annotation: buildStateFrontmatter's #3573 withhold path leaves + // totalPhases at null (never a number) because there is nothing on disk + // or in the body to derive a denominator from. The guard's + // `typeof totalPhases === 'number' && Number.isFinite(totalPhases)` + // conjunct fails, so it cannot fire regardless of the true completion + // state — the counters are not trustworthy enough to demote on. + seed4PhaseDirsNoRoadmap(2); + writeStateAtPhase(2); + + const result = runGsdTools(['state', 'complete-phase', '--phase', '2'], tmpDir); + assert.ok(result.success, `complete-phase failed: ${result.error || result.output}`); + + const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.strictEqual( + frontmatterStatus(after), + 'completed', + `guard must not fire without a trustworthy totalPhases; got frontmatter:\n${after}`, + ); + + const jsonResult = runGsdTools('state json', tmpDir); + assert.ok(jsonResult.success, `state json failed: ${jsonResult.error}`); + const output = JSON.parse(jsonResult.output); + assert.strictEqual( + output.progress.total_phases, + undefined, + 'total_phases must be withheld (no ROADMAP to derive it from), proving the guard truly had no denominator to compare against', + ); + assert.strictEqual(Number(output.progress.completed_phases), 2, 'completed_phases is still disk truth even when total_phases is withheld'); + }); + + test('#3578 AC4: gsd_invoke_command (MCP dispatch) yields the same non-completed status as the CLI route (2 of 4 case)', () => { + seed4PhaseMilestone(2); + writeStateAtPhase(2); + + const res = handleMessage( + { + jsonrpc: '2.0', + id: 100, + method: 'tools/call', + params: { name: 'gsd_invoke_command', arguments: { family: 'state', subcommand: 'complete-phase', args: ['--phase', '2'] } }, + }, + { cwd: tmpDir }, + ); + assert.notStrictEqual(res.result.isError, true, `MCP dispatch must succeed: ${JSON.stringify(res.result)}`); + // Same command, reached through a different dispatch surface (real + // subprocess spawn via dispatchGsdCommand -> gsd-tools.cjs), must produce + // the same on-disk effect as the CLI route above. + JSON.parse(res.result.content[0].text); + + const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.notEqual( + frontmatterStatus(after), + 'completed', + `MCP-dispatched complete-phase must not set status completed while phases remain open; got frontmatter:\n${after}`, + ); + }); +}); + describe('T6 section-splice characterization — milestone-switch', () => { const STATE_INLINE_MS = [ '---',