From 1b027298dc6f9a1205fe1843576d4da126453659 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 14 Aug 2026 23:00:26 -0400 Subject: [PATCH] fix(#3481): resolve add-roadmap-evolution's phase from STATE.md, not a literal `?` (#3522) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#3481): resolve add-roadmap-evolution's phase from STATE.md, not a literal `?` `state add-roadmap-evolution` built its entry from the raw `--phase` flag alone, so omitting the flag persisted `- Phase ?` even when STATE.md's own frontmatter carried `current_phase` above the insertion point — the #3231 defect at a second call site. Roadmap-evolution entries are the permanent trail explaining why the roadmap changed shape; `Phase ?` makes that trail unattributable, and the command is mostly invoked from agents that do not know to pass `--phase`. The #3481 triage confirmed the #3231 sibling site (`add-decision`) was also still unfixed on next — both PRs that attempted it (#3232, #3347) were closed unmerged. This applies the #3347 treatment to both call sites: - Extracts the write-path phase-resolution ladder `cmdStatePrune` already ran — frontmatter `current_phase` → body `Current Phase` field → prose `Phase: X of Y` scoped to `## Current Position` — into a shared `resolveCurrentPhaseId`, and routes `cmdStateAddRoadmapEvolution`, `cmdStateAddDecision`, and `cmdStatePrune` through it. - Deliberately NOT routed through `resolveStatePhase` (#3208): its `matchCurrentPositionSection(body) ?? body` fallback widens the prose rung to the whole document when no `## Current Position` section exists, where the pipe-table fallback matches any historical `| Phase | N |` row (#1776). Read-path callers (snapshot/validate) report to a human; write-path callers persist durably, so they take the strict rung and render `?` instead of guessing. - The resolved id is returned as written, never parsed to a number (`11-01` and `04.1` are real ids). Prune still parses its own integer cutoff, so its behavior is byte-identical. - Explicit `--phase` still wins and its path is untouched — STATE.md is not even read. When no rung resolves, `?` is still written. Tests: per-call-site coverage for both commands — omitted `--phase` resolves (including a non-integer prose id), explicit `--phase` wins, and two counter-tests pinning the degraded verdict (nothing resolvable → `?`, and a historical `| Phase | 7 |` table row must NOT be adopted). Plus a static guard sweeping src/*.cts for the raw `phase || '?'` placeholder shape so a future call site cannot reintroduce the class. Fixes #3481 * chore(#3481): add changeset fragment for PR #3522 --------- Co-authored-by: sim --- .changeset/3481-state-phase-placeholder.md | 5 + src/state.cts | 115 ++++++++-- tests/state.test.cjs | 243 +++++++++++++++++++++ 3 files changed, 340 insertions(+), 23 deletions(-) create mode 100644 .changeset/3481-state-phase-placeholder.md diff --git a/.changeset/3481-state-phase-placeholder.md b/.changeset/3481-state-phase-placeholder.md new file mode 100644 index 000000000..f54fb753f --- /dev/null +++ b/.changeset/3481-state-phase-placeholder.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3522 +--- +**`state add-roadmap-evolution` and `state add-decision` no longer persist a literal `Phase ?` when `--phase` is omitted** — both commands built their entry from the raw CLI flag's `?` fallback instead of the phase already recorded in STATE.md, even with `current_phase: 3` present in frontmatter. Both now resolve the phase through a strict write-path ladder (frontmatter `current_phase` → body `Current Phase` → `Phase: X of Y` scoped strictly to `## Current Position`), leaving `?` only when genuinely unresolvable; an explicit `--phase` still wins. The resolver deliberately does not reuse the read-path `resolveStatePhase`, whose document-wide fallback could adopt a stale historical `| Phase | N |` table row. A guard test now sweeps `src/*.cts` for any new raw `phase || '?'` call site. (#3481) diff --git a/src/state.cts b/src/state.cts index 495894f85..e184faf4e 100644 --- a/src/state.cts +++ b/src/state.cts @@ -983,7 +983,21 @@ function cmdStateAddDecision(cwd: string, options: StateAddDecisionOptions, raw: if (!summaryText) { output({ error: 'summary required' }, raw, undefined); return; } - const entry = `- [Phase ${phase || '?'}]: ${summaryText}${rationaleText ? ` — ${rationaleText}` : ''}`; + // #3231/#3481: `--phase` omitted → resolve from the STATE.md being written, via + // the canonical ladder `state prune` uses. A decision entry is a permanent + // record, so a literal `[Phase ?]` written while `current_phase` sat three + // lines above the insertion point loses that decision's provenance for good. + // Explicit `--phase` still wins, and its path is untouched — the file is not + // even read. When no rung resolves, `?` is still written: an unknown phase + // stays visibly unknown rather than being guessed or defaulted to a number. + let phaseId: string | undefined = phase; + if (!phaseId) { + const rawState = fs.readFileSync(statePath, 'utf-8'); + const fm = extractFrontmatter(rawState, statePath) as Record; + phaseId = resolveCurrentPhaseId(fm, stripFrontmatter(rawState)) ?? undefined; + } + + const entry = `- [Phase ${phaseId || '?'}]: ${summaryText}${rationaleText ? ` — ${rationaleText}` : ''}`; let _added = false; let created = false; @@ -1126,7 +1140,21 @@ function cmdStateAddRoadmapEvolution(cwd: string, options: StateAddRoadmapEvolut const actionText = (action && action.trim()) || 'changed'; const afterText = after && after.trim() ? ` after Phase ${after.trim()}` : ''; const urgentText = urgent ? ' (URGENT)' : ''; - const entry = `- Phase ${phase || '?'} ${actionText}${afterText}: ${flatNote}${urgentText}`; + // #3481: same treatment as add-decision's #3231 fix — `--phase` omitted → + // resolve from the STATE.md being written via the shared write-path ladder. + // A roadmap-evolution entry is a permanent record of why the roadmap changed + // shape, so a literal `Phase ?` written while `current_phase` sat in the + // frontmatter above the insertion point makes that trail unattributable. + // Explicit `--phase` still wins (the file is not even read on that path), and + // `?` is still written when nothing resolves — never a guess. + let phaseId: string | undefined = phase; + if (!phaseId) { + const rawState = fs.readFileSync(statePath, 'utf-8'); + const fm = extractFrontmatter(rawState, statePath) as Record; + phaseId = resolveCurrentPhaseId(fm, stripFrontmatter(rawState)) ?? undefined; + } + + const entry = `- Phase ${phaseId || '?'} ${actionText}${afterText}: ${flatNote}${urgentText}`; let duplicate = false; let created = false; @@ -1630,6 +1658,59 @@ function resolveStatePhase(fm: Record, body: string): { }; } +/** + * Resolve a STATE.md's own current phase id from the document itself — the + * WRITE-PATH ladder shared by `cmdStateAddDecision` (#3231) and + * `cmdStateAddRoadmapEvolution` (#3481), extracted from the ladder + * `cmdStatePrune` already ran (#1760). + * + * The rungs are the canonical ones owned by state-document.cjs's + * `stateFieldValue` (#3187, ADR-3180 §7.7): frontmatter `current_phase` → body + * `Current Phase` field → prose `Phase: X of Y` scoped to `## Current + * Position`. + * + * #1776: the prose rung stays scoped to `## Current Position`. Over the whole + * body, `stateExtractField`'s pipe-table fallback matches any `| Phase | N |` + * row — e.g. a historical verification table — and would resolve a stale phase. + * Frontmatter and the explicit `Current Phase` field are unambiguous, so they + * stay document-wide. `cmdStateSnapshot` deliberately keeps the looser + * whole-body fallback for its own prose rung and is not routed through here. + * + * Returns the id exactly as written, NOT parsed to a number: phase ids are not + * always integers (`11-01` and `04.1` are both real). Callers needing an + * integer parse it themselves. Returns null when no rung carries a value — a + * genuinely absent phase is a real answer (§7.7 behavior table row 4), and + * callers must render it as unknown rather than guess one. + * + * NOT the same function as `resolveStatePhase` above (#3208), and deliberately + * not routed through it — the difference is one line and it is the whole point: + * + * resolveStatePhase: matchCurrentPositionSection(body) ?? body + * resolveCurrentPhaseId: null when the section is absent + * + * That `?? body` fallback is exactly the #1776 hazard. With no `## Current + * Position` section, the prose rung widens to the entire document, where + * `stateExtractField`'s pipe-table fallback matches any `| Phase | N |` row — + * a historical verification table included — and resolves a stale phase. + * + * `resolveStatePhase`'s callers (`cmdStateSnapshot`, `cmdStateValidate`) READ + * and report; a stale guess there is a wrong line in output a human is already + * looking at. This function's callers WRITE: `cmdStateAddDecision` and + * `cmdStateAddRoadmapEvolution` persist the result into records that outlive + * the session, and `cmdStatePrune` decides what to delete from it. A wrong + * phase there is durable and silent, so the write path takes the strict rung + * and renders `?` rather than guessing. + * + * Reconcile the two only by giving `resolveStatePhase` an explicit scope + * parameter — never by pointing this at it and dropping the difference. + */ +function resolveCurrentPhaseId(fm: Record, body: string): string | null { + const positionSection = sliceCurrentPositionSection(body); + const prosePhase = + positionSection !== null ? parseProsePhaseField(stateFieldValue(fm, positionSection, null, 'Phase').value).phase : null; + return stateFieldValue(fm, body, 'current_phase', 'Current Phase').value ?? prosePhase; +} + function parseProseLastActivityField(value: string | null): { date: string | null; description: string | null } { if (!value) return { date: null, description: null }; const match = value.match(/^(\d{4}-\d{2}-\d{2})(?:\s+[—-]{1,2}\s+(.+))?$/); @@ -4319,30 +4400,18 @@ function cmdStatePrune(cwd: string, options: StatePruneOptions, raw: boolean): v const keepRecent = parseInt(String(options.keepRecent), 10) || 3; const dryRun = !!options.dryRun; - // Resolve the current phase via the same canonical chain buildStateFrontmatter - // uses (frontmatter `current_phase` → `Current Phase` field → prose `Phase: X - // of Y`), so prune engages on template-conformant STATE.md instead of bailing - // "Only 0 phases" (#1760). - // #1776: scope ONLY the prose `Phase:` term to the canonical `## Current - // Position` section. Over the whole body, `stateExtractField`'s pipe-table - // fallback matches any `| Phase | N |` row (e.g. a historical verification - // table), resolving a stale phase and computing a wrong cutoff. Frontmatter and - // the explicit `Current Phase` field are unambiguous, so they stay document-wide; - // the shared extractor is not narrowed for any other caller. + // Resolve the current phase via `resolveCurrentPhaseId` — the shared owner of + // the canonical frontmatter → `Current Phase` field → scoped prose ladder + // (#1760 origin, #1776 scoping, #3187 ownership; see its doc comment). Prune + // engages on a template-conformant STATE.md instead of bailing "Only 0 + // phases" (#1760). #3231/#3481 routed the phase-labeled write commands + // through the same helper rather than leaving a second copy of the ladder here. const rawState = fs.readFileSync(statePath, 'utf-8'); const fm = extractFrontmatter(rawState, statePath) as Record; const body = stripFrontmatter(rawState); - // #3187: frontmatter-scalar-then-body-field precedence is owned by - // state-document.cjs's `stateFieldValue` (ADR-3180 §7.7). This comment - // previously claimed to mirror `buildStateFrontmatter`'s fmScalar — that - // attribution was stale: buildStateFrontmatter (state.cts:1620) reads body - // only and never consults frontmatter at all; this actually mirrored - // cmdStateSnapshot's now-removed closure instead. - const positionSection = sliceCurrentPositionSection(body); - const prosePhase = - positionSection !== null ? parseProsePhaseField(stateFieldValue(fm, positionSection, null, 'Phase').value).phase : null; - const currentPhaseRaw = stateFieldValue(fm, body, 'current_phase', 'Current Phase').value ?? prosePhase; - const currentPhase = parseInt(String(currentPhaseRaw), 10) || 0; + // Prune needs an integer cutoff, so it parses the resolved id itself; a + // non-numeric or absent id lands on 0 and prune bails, as before. + const currentPhase = parseInt(String(resolveCurrentPhaseId(fm, body)), 10) || 0; const cutoff = currentPhase - keepRecent; if (cutoff <= 0) { diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 94d09bd3d..4cac39367 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -14698,3 +14698,246 @@ describe('#3468 B8: a drifted / malformed / unparseable STATE.md never reaches t }); } }); + +// ───────────────────────────────────────────────────────────────────────────── +// #3481: `state add-roadmap-evolution` resolves the phase from STATE.md's own +// frontmatter/body when `--phase` is omitted, instead of persisting a literal +// `Phase ?`. This is the #3231 defect at a second call site — the sibling +// `add-decision` site (also currently unfixed on next, per the #3481 triage) +// is pinned here too, so the whole class is covered by one contract. +// ───────────────────────────────────────────────────────────────────────────── +{ + const { describe, test, beforeEach, afterEach } = require('node:test'); + const assert = require('node:assert/strict'); + const path = require('path'); + + describe('#3481 phase-labeled write commands resolve the phase from STATE.md when --phase is omitted', () => { + let tmpDir; + + beforeEach(() => { tmpDir = createTempProject(); }); + afterEach(() => { cleanup(tmpDir); }); + + const statePathIn = (d) => path.join(d, '.planning', 'STATE.md'); + + // Write a STATE.md verbatim, bypassing the project scaffold, so each test + // controls exactly which rung of the resolution ladder is populated. + const writeState = (d, content) => fs.writeFileSync(statePathIn(d), content); + + const run = (d, args) => { + const result = runGsdTools(args, d); + assert.ok(result.success, `command must succeed, got: ${result.error}`); + return JSON.parse(result.output); + }; + + // Assert on the persisted file, not only the JSON echo — the defect in + // #3481 is that the placeholder is written to disk permanently. + const persistedLines = (d, prefix) => + fs.readFileSync(statePathIn(d), 'utf-8') + .split(/\r?\n/) + .filter(l => l.startsWith(prefix)); + + const STATE_WITH_FM_PHASE = [ + '---', + 'gsd_state_version: 1.0', + 'current_phase: 3', + 'current_phase_name: Sourcing Coverage', + 'status: executing', + '---', + '', + '# Project State', + '', + '## Accumulated Context', + '', + '### Decisions', + '', + ].join('\n'); + + // ── add-roadmap-evolution (the site #3481 reports) ────────────────────── + + test('add-roadmap-evolution: frontmatter current_phase resolves the entry when --phase is omitted', () => { + writeState(tmpDir, STATE_WITH_FM_PHASE); + + const parsed = run(tmpDir, ['state', 'add-roadmap-evolution', '--action', 'added', '--note', 'Phase 4.2 inserted for enrichment reach']); + + assert.strictEqual( + parsed.entry, + '- Phase 3 added: Phase 4.2 inserted for enrichment reach', + 'omitted --phase must resolve from frontmatter current_phase, not render "?"', + ); + assert.deepStrictEqual( + persistedLines(tmpDir, '- Phase 3 added:'), + ['- Phase 3 added: Phase 4.2 inserted for enrichment reach'], + 'the resolved phase must be what lands in STATE.md', + ); + }); + + test('add-roadmap-evolution: explicit --phase wins over a resolvable frontmatter current_phase', () => { + writeState(tmpDir, STATE_WITH_FM_PHASE); + + const parsed = run(tmpDir, ['state', 'add-roadmap-evolution', '--phase', '7', '--action', 'verified', '--note', 'Explicit wins']); + + assert.strictEqual( + parsed.entry, + '- Phase 7 verified: Explicit wins', + 'an explicitly passed --phase must never be overridden by the resolved value', + ); + }); + + test('add-roadmap-evolution: prose "Phase:" under ## Current Position resolves, preserving a non-integer id', () => { + writeState(tmpDir, [ + '# Project State', + '', + '## Current Position', + '', + 'Phase: 04.1 of 7', + '', + '## Accumulated Context', + '', + ].join('\n')); + + const parsed = run(tmpDir, ['state', 'add-roadmap-evolution', '--action', 'edited', '--note', 'Prose rung']); + + // Phase ids are not always integers — 04.1 must survive verbatim. + assert.strictEqual(parsed.entry, '- Phase 04.1 edited: Prose rung'); + }); + + // Counter-test (TESTING-STANDARDS.md contract 6): assert the specific + // degraded verdict. When no rung resolves, the entry must still read "?" + // — an unknown phase stays visibly unknown and is never guessed. + test('add-roadmap-evolution CONTROL: no phase resolvable from any rung still writes a literal "?"', () => { + writeState(tmpDir, [ + '---', + 'gsd_state_version: 1.0', + 'status: executing', + '---', + '', + '# Project State', + '', + '## Accumulated Context', + '', + ].join('\n')); + + const parsed = run(tmpDir, ['state', 'add-roadmap-evolution', '--action', 'added', '--note', 'Unresolvable stays unknown']); + + assert.strictEqual( + parsed.entry, + '- Phase ? added: Unresolvable stays unknown', + 'with no frontmatter current_phase, no body "Current Phase" field and no scoped prose Phase line, the entry must degrade to "?" — not to a default, an empty string, or a guessed number', + ); + assert.deepStrictEqual(persistedLines(tmpDir, '- Phase ? added:'), ['- Phase ? added: Unresolvable stays unknown']); + }); + + // Counter-test pinning #1776: the write path must NOT adopt a phase-shaped + // token from outside the designated current-position location. A stale + // historical `| Phase | 7 |` table row is not this document's current phase. + test('add-roadmap-evolution CONTROL: a historical Phase row outside Current Position must not resolve (#1776)', () => { + writeState(tmpDir, [ + '---', + 'gsd_state_version: 1.0', + 'status: executing', + '---', + '', + '# Project State', + '', + '## Verification History', + '', + '| Field | Value |', + '|---|---|', + '| Phase | 7 |', + '', + '## Accumulated Context', + '', + ].join('\n')); + + const parsed = run(tmpDir, ['state', 'add-roadmap-evolution', '--action', 'added', '--note', 'Stale table must not win']); + + assert.strictEqual( + parsed.entry, + '- Phase ? added: Stale table must not win', + 'the prose rung is scoped to ## Current Position; a | Phase | N | row in a historical table must not be persisted as this entry\'s phase', + ); + }); + + // ── add-decision (the #3231 sibling site — same defect, same fix) ─────── + + test('add-decision: frontmatter current_phase resolves the entry when --phase is omitted', () => { + writeState(tmpDir, STATE_WITH_FM_PHASE); + + const parsed = run(tmpDir, ['state', 'add-decision', '--summary', 'Dedup threshold set to 0.85 by sweep']); + + assert.strictEqual( + parsed.decision, + '- [Phase 3]: Dedup threshold set to 0.85 by sweep', + 'omitted --phase must resolve from frontmatter current_phase, not render "?"', + ); + assert.deepStrictEqual( + persistedLines(tmpDir, '- [Phase 3]:'), + ['- [Phase 3]: Dedup threshold set to 0.85 by sweep'], + 'the resolved phase must be what lands in STATE.md', + ); + }); + + test('add-decision: explicit --phase wins over a resolvable frontmatter current_phase', () => { + writeState(tmpDir, STATE_WITH_FM_PHASE); + + const parsed = run(tmpDir, ['state', 'add-decision', '--phase', '7', '--summary', 'Explicit wins']); + + assert.strictEqual(parsed.decision, '- [Phase 7]: Explicit wins'); + }); + + test('add-decision CONTROL: a historical Phase row outside Current Position must not resolve (#1776)', () => { + writeState(tmpDir, [ + '---', + 'gsd_state_version: 1.0', + 'status: executing', + '---', + '', + '# Project State', + '', + '## Verification History', + '', + '| Field | Value |', + '|---|---|', + '| Phase | 7 |', + '', + '### Decisions', + '', + ].join('\n')); + + const parsed = run(tmpDir, ['state', 'add-decision', '--summary', 'Stale table must not win']); + + assert.strictEqual( + parsed.decision, + '- [Phase ?]: Stale table must not win', + 'the prose rung is scoped to ## Current Position; a | Phase | N | row in a historical table is not this document\'s current phase', + ); + }); + + // ── Guard (skill "Partial Fix Across Call Sites"): the #3481/#3231 defect + // class is "entry text built from the RAW CLI phase flag with a '?' fallback, + // never consulting the document being written". Behavioral tests above pin + // the two known sites; this static sweep catches any NEW site that + // regresses to the same shape — the fix routes every such site through the + // resolved id, so the raw-flag pattern must not reappear in src/. + test('GUARD: no phase-labeled entry is built from the raw CLI phase flag with a "?" fallback', () => { + const SRC_DIR = path.join(__dirname, '..', 'src'); + const offenders = fs.readdirSync(SRC_DIR) + .filter(f => f.endsWith('.cts')) + .flatMap(f => { + const rel = path.join('src', f); + const lines = fs.readFileSync(path.join(SRC_DIR, f), 'utf-8').split(/\r?\n/); + return lines + .map((line, i) => ({ rel, i, line })) + .filter(({ line }) => line.includes('Phase ${phase || \'?\'}')); + }) + .map(({ rel, i }) => `${rel}:${i + 1}`); + + assert.deepStrictEqual( + offenders, + [], + 'raw `phase || \'?\'` placeholder in a phase-labeled entry — resolve the phase from the STATE.md being written ' + + '(see resolveCurrentPhaseId, #3231/#3481): ' + offenders.join(', '), + ); + }); + }); +}