diff --git a/.changeset/patient-sloths-glide.md b/.changeset/patient-sloths-glide.md new file mode 100644 index 000000000..9176a7553 --- /dev/null +++ b/.changeset/patient-sloths-glide.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4028 +--- +**`state advance-plan` refuses an ambiguous Current Position instead of silently advancing the first entry** — when the section carries more than one `Phase:` line (the wave-log style), the command now returns a typed `ambiguous_position_phase` error naming every candidate and leaves STATE.md byte-identical, instead of silently advancing the first entry's plan counter (in the reporting incident, a hard-gated final plan 7→8 of 8) with `advanced: true` and no ambiguity signal. (#3807) diff --git a/src/state-transition.cts b/src/state-transition.cts index 1822e762e..140e7f527 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -16,7 +16,7 @@ // eslint-disable-next-line @typescript-eslint/no-require-imports import frontmatter = require('./frontmatter.cjs'); -import { stateReplaceField, stateExtractField, stateReplaceFieldIfTemplate, stateReplaceFieldWithFallback, stateReplaceFieldInSession } from './state-document.cjs'; +import { stateReplaceField, stateExtractField, stateReplaceFieldIfTemplate, stateReplaceFieldWithFallback, stateReplaceFieldInSession, stateCurrentPositionSlice } from './state-document.cjs'; import { KNOWN_TEMPLATE_DEFAULTS, toFiniteNumber } from './state-document.cjs'; import { tokenizeHeadings } from './markdown-sectionizer.cjs'; import type { HeadingToken } from './markdown-sectionizer.cjs'; @@ -1382,6 +1382,34 @@ function advancePlanCore(content: string, deps: StateTransitionDeps): StateTrans const { body: initialBody, reassemble } = beginFrontmatterReassembly(content, deps.sourcePath); let body = initialBody; + // #3807: refuse a Current Position section carrying more than one `Phase:` + // entry BEFORE mutating. The plan fields below come from document-wide + // first-match extraction, so in a wave-log style section (one entry per + // completed wave) the FIRST entry's plan counter silently advanced — in the + // reporting incident, a hard-gated final plan 7→8 of 8 — while the entry + // the caller meant sat untouched below it, with advanced:true and no + // ambiguity signal. advance-plan now refuses before acting. Scoped via the + // #2956 canonical locator (stateCurrentPositionSlice — H2 or H3 heading, + // the same one cmdStateAdvancePlan's own milestone read uses); NO whole-body + // fallback — a legacy-format document with unrelated `Phase:` history lines + // elsewhere has no section to disambiguate and must keep its current + // behavior rather than be falsely refused. + const positionScope = stateCurrentPositionSlice(body); + if (positionScope !== null) { + const phaseCandidates = (positionScope.match(/^Phase:.*$/gm) || []); + if (phaseCandidates.length > 1) { + return { + content, + updated: [], + data: { + error: true, + reason: 'ambiguous_position_phase', + phase_candidates: phaseCandidates.map((l) => l.trim()), + }, + }; + } + } + // Parse plan number — legacy first, then compound. const legacyPlan = stateExtractField(content, 'Current Plan'); const legacyTotal = stateExtractField(content, 'Total Plans in Phase'); diff --git a/src/state.cts b/src/state.cts index 953d65029..7c4d5d796 100644 --- a/src/state.cts +++ b/src/state.cts @@ -894,6 +894,16 @@ function cmdStateAdvancePlan(cwd: string, raw: boolean): void { }, cwd, { divergedFields, preWriteState }); if (!resultData || resultData['error']) { + // #3807: a multi-`Phase:` Current Position section carries its own cause + // and its own remedy (name the candidates; the caller resolves them). + if (resultData && resultData['reason'] === 'ambiguous_position_phase') { + output({ + error: 'Current Position section contains more than one Phase: entry — refusing to silently advance the first. Resolve the section to a single current entry and re-run.', + reason: resultData['reason'], + phase_candidates: resultData['phase_candidates'], + }, raw, undefined); + return; + } output({ error: 'Cannot parse Current Plan or Total Plans in Phase from STATE.md' }, raw, undefined); return; } diff --git a/tests/advance-plan-ambiguous-phase.test.cjs b/tests/advance-plan-ambiguous-phase.test.cjs new file mode 100644 index 000000000..907ba3eb5 --- /dev/null +++ b/tests/advance-plan-ambiguous-phase.test.cjs @@ -0,0 +1,100 @@ +'use strict'; + +// ───────────────────────────────────────────────────────────────────────────── +// #3807 — advance-plan must refuse a Current Position section carrying +// more than one `Phase:` entry instead of silently advancing the first. +// +// The #2956 fix scoped the milestone-conflict Phase read to the Current +// Position section, but advancePlanCore's plan fields still came from +// document-wide first-match stateExtractField — so a wave-log style section +// (one Phase: entry per completed wave, all under Current Position) had its +// FIRST entry's plan counter silently advanced — in the reporter's incident, +// a hard-gated final plan 7→8 of 8 — with advanced:true and +// milestone_conflict:null, no error, no ambiguity signal. +// ───────────────────────────────────────────────────────────────────────────── + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const { createTempProject, cleanup, runGsdTools } = require('./helpers.cjs'); + +const TWO_ENTRY_BODY = [ + '## Current Position', + '', + 'Phase: 03.1 of 8 (some-phase)', + 'Plan: 7 of 8 in current phase', + 'Status: In progress', + 'Last activity: 2026-08-24 — working', + '', + 'Phase: 04 of 15 (other-phase)', + 'Plan: 7 of 15 in current phase', + 'Status: Phase complete', + 'Last activity: 2026-08-24 — wave 4 done', + '', +].join('\n'); + +function writeState(tmpDir, positionBody) { + const content = [ + '---', + 'gsd_state_version: 1.0', + 'current_phase: 03', + 'status: executing', + 'progress:', + ' total_phases: 2', + '---', + '', + positionBody, + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), content); +} + +function runAdvance(cwd) { + return runGsdTools(['state', 'advance-plan'], cwd); +} + +describe('#3807: advance-plan refuses an ambiguous multi-entry Current Position', () => { + test('#3807: two Phase: entries under Current Position → ambiguous error, no mutation', (t) => { + const tmpDir = createTempProject('gsd-3807-amb-'); + t.after(() => cleanup(tmpDir)); + writeState(tmpDir, TWO_ENTRY_BODY); + const before = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf8'); + + const r = runAdvance(tmpDir); + // The CLI reports an error payload (exit success shape is the command's + // own convention for parse errors — assert on the payload, not the code). + const out = JSON.parse(r.output); + assert.ok( + out.error && out.reason === 'ambiguous_position_phase' && /more than one Phase/i.test(String(out.error)), + `#3807: the error must name the multi-Phase condition with the typed reason; got ${r.output}`, + ); + assert.ok( + Array.isArray(out.phase_candidates) && out.phase_candidates.length === 2, + `#3807: both Phase: candidates must be named; got ${JSON.stringify(out.phase_candidates)}`, + ); + const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf8'); + assert.equal(after, before, '#3807: refusing must leave STATE.md byte-identical'); + assert.ok(!/Plan: 8 of 8/.test(after), 'the first entry\'s plan counter must NOT advance'); + }); + + test('#3807 control: a single-entry section advances exactly as before', (t) => { + const tmpDir = createTempProject('gsd-3807-ctl-'); + t.after(() => cleanup(tmpDir)); + writeState(tmpDir, [ + '## Current Position', + '', + 'Phase: 03 of 8 (some-phase)', + 'Plan: 3 of 8 in current phase', + 'Status: In progress', + 'Last activity: 2026-08-24 — working', + '', + ].join('\n')); + + const r = runAdvance(tmpDir); + const out = JSON.parse(r.output); + assert.equal(out.advanced, true, `single-entry advance still works; got ${r.output}`); + const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf8'); + assert.match(after, /Plan: 4 of 8/, 'the plan counter advanced'); + }); +}); diff --git a/tests/io.test.cjs b/tests/io.test.cjs index 8da2a2889..52e8c5094 100644 --- a/tests/io.test.cjs +++ b/tests/io.test.cjs @@ -841,11 +841,11 @@ describe('#3912 A3-A5: output({error}) records DEGRADED — shape-exhaustive plu perFile, { 'commands.cts': 5, 'frontmatter.cts': 7, 'gsd2-import.cts': 2, 'phase.cts': 4, - 'roadmap.cts': 3, 'state.cts': 25, 'template.cts': 3, 'verify.cts': 8, 'workstream.cts': 7, + 'roadmap.cts': 3, 'state.cts': 26, 'template.cts': 3, 'verify.cts': 8, 'workstream.cts': 7, // +1 #3807: advance-plan's ambiguous-position error }, `per-file output({error}) census drifted: ${JSON.stringify(perFile)}`, ); - assert.strictEqual(total, 64, `enumerated output({error}) population drifted from the measured 64: got ${total}`); + assert.strictEqual(total, 65, `enumerated output({error}) population drifted from the measured 65 (64 + #3807's ambiguous-position error): got ${total}`); }); });