diff --git a/.changeset/serene-birds-sing.md b/.changeset/serene-birds-sing.md new file mode 100644 index 000000000..7940084c5 --- /dev/null +++ b/.changeset/serene-birds-sing.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3490 +--- +state planned-phase now refreshes the Current Position Phase: line (the body source current_phase is re-derived from) instead of leaving a stale previous-phase line behind, so STATE.md frontmatter, body prose, and state json stay coherent; the --name argument is persisted into the Phase line and current_phase_name instead of being silently dropped. diff --git a/src/state-command-router.cts b/src/state-command-router.cts index 764a13fba..975cb79e2 100644 --- a/src/state-command-router.cts +++ b/src/state-command-router.cts @@ -46,7 +46,7 @@ interface StateModule { cmdStateBeginPhase(cwd: string, phase: string | null | undefined, name: string | null | undefined, plans: number | null, raw: boolean): void; cmdSignalWaiting(cwd: string, type: string | null | undefined, question: string | null | undefined, options: string | null | undefined, phase: string | null | undefined, raw: boolean): void; cmdSignalResume(cwd: string, raw: boolean): void; - cmdStatePlannedPhase(cwd: string, phase: string | null | undefined, plans: number | null, raw: boolean): void; + cmdStatePlannedPhase(cwd: string, phase: string | null | undefined, name: string | null | undefined, plans: number | null, raw: boolean): void; cmdStateValidate(cwd: string, raw: boolean): void; cmdStateSync(cwd: string, opts: { verify: string | boolean | null | undefined }, raw: boolean): void; cmdStatePrune(cwd: string, opts: { keepRecent: string; dryRun: boolean }, raw: boolean): void; @@ -180,7 +180,11 @@ function routeStateCommand({ state, args, cwd, raw, error }: RouteStateCommandOp 'signal-resume': () => state.cmdSignalResume(cwd, raw), 'planned-phase': () => { const a = parseNamedArgs(args, ['phase', 'name', 'plans']); - state.cmdStatePlannedPhase(cwd, strArg(a, 'phase'), parsePlans(strArg(a, 'plans')), raw); + // #3395: --name was parsed here but never forwarded (the StateModule + // signature had no channel for it), so the argument was silently + // dropped. It now persists into the Current Position `Phase:` line and + // the authoritative current_phase_name, mirroring begin-phase. + state.cmdStatePlannedPhase(cwd, strArg(a, 'phase'), strArg(a, 'name'), parsePlans(strArg(a, 'plans')), raw); }, validate: () => state.cmdStateValidate(cwd, raw), sync: () => { diff --git a/src/state-transition.cts b/src/state-transition.cts index 330c7ebda..d694c4c44 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -462,7 +462,7 @@ export type StateTransitionIntent = planCount: number; summaryCount: number; } - | { kind: 'plannedPhase'; phaseNumber: string | number; planCount: number | null } + | { kind: 'plannedPhase'; phaseNumber: string | number; phaseName: string | null; planCount: number | null } | { kind: 'milestoneSwitch'; version: string; name: string } | { kind: 'milestoneComplete'; @@ -845,7 +845,7 @@ function mutateCurrentPositionResume( */ function mutateCurrentPositionForAdvance( content: string, - fields: { status?: string; lastActivity?: string; plan?: string }, + fields: { phase?: string; status?: string; lastActivity?: string; plan?: string }, statusDefaults: string[] | null | undefined, lastActivityDefaults: string[] | null | undefined, ): string { @@ -854,6 +854,22 @@ function mutateCurrentPositionForAdvance( let sectionBody = content.slice(span.start, span.end); let mutated = false; + // #3395: Phase is always replaced when a caller passes it — system-derived, + // not executor-authored (same rule as Plan below). plannedPhaseCore uses + // this so the transition that declares phase N planned also owns the `Phase:` + // line the frontmatter resync and `state json` re-derive current_phase from; + // before, the line survived stale from a previous phase and every + // body-derived consumer kept reading it (#948 class). + if (fields.phase) { + if (/^Phase:/m.test(sectionBody)) { + sectionBody = sectionBody.replace(/^Phase:.*$/m, `Phase: ${fields.phase}`); + mutated = true; + } else { + const replaced = stateReplaceField(sectionBody, 'Phase', fields.phase); + if (replaced !== null) { sectionBody = replaced; mutated = true; } + } + } + if (fields.status) { const replaced = stateReplaceFieldIfTemplate(sectionBody, 'Status', statusDefaults, fields.status); if (replaced !== null && replaced !== sectionBody) { sectionBody = replaced; mutated = true; } @@ -1198,7 +1214,10 @@ function completePhaseCore( * per-phase body fields after plan-phase runs: Status (template-aware — only * replaces handler-generated values, preserving executor-authored ones), * Total Plans in Phase, Last Activity (template-aware), Last Activity - * Description, and the ## Current Position section. The adapter wraps this in + * Description, and the ## Current Position section — including its `Phase:` + * line, which this transition owns (#3395: the line is the body source + * `current_phase` re-derives from, so it must not survive stale from a + * previous phase). The adapter wraps this in * `readModifyWriteStateMd({ resync: false })` so the milestone-wide progress.* * frontmatter is NOT re-derived from a half-planned disk snapshot (#500 RC1). * @@ -1208,7 +1227,7 @@ function completePhaseCore( */ function plannedPhaseCore( content: string, - intent: { kind: 'plannedPhase'; phaseNumber: string | number; planCount: number | null }, + intent: { kind: 'plannedPhase'; phaseNumber: string | number; phaseName: string | null; planCount: number | null }, deps: StateTransitionDeps, ): StateTransitionResult { const updated: string[] = []; @@ -1270,11 +1289,22 @@ function plannedPhaseCore( updated.push('Last Activity Description'); } - // ## Current Position section — Status + Last activity (template-aware). + // ## Current Position section — Phase + Status + Last activity. + // #3395: plannedPhaseCore owns the `Phase:` line for the same reason + // beginPhaseCore/completePhaseCore do — it is the body source the frontmatter + // resync and `state json` re-derive `current_phase` from. Before, a stale + // line from a previous phase survived this transition and every + // body-derived consumer kept reading it (the write path was already + // protected by the #3258 preserve-when-unchanged row; the source itself was + // never refreshed). The label mirrors beginPhaseCore's `N (Name) — EXECUTING` + // convention with this transition's status vocabulary ("Ready to execute"). + // Phase is system-derived, always replaced (Knuth invariant does not apply); + // Status / Last activity stay template-aware. const beforePos = body; body = mutateCurrentPositionForAdvance( body, { + phase: `${intent.phaseNumber}${intent.phaseName ? ` (${intent.phaseName})` : ''} — READY TO EXECUTE`, status: 'Ready to execute', lastActivity: `${today} — Phase ${intent.phaseNumber} planning complete`, }, diff --git a/src/state.cts b/src/state.cts index 5a1b8bb96..855dc0928 100644 --- a/src/state.cts +++ b/src/state.cts @@ -3268,7 +3268,7 @@ function updatePerformanceMetricsSection(content: string, cwd: string, phaseNum: * Gate 3a: Record state after plan-phase completes. * Updates Status to "Ready to execute", Total Plans, Last Activity. */ -function cmdStatePlannedPhase(cwd: string, phaseNumber: string | number, planCount: number | null | undefined, raw: boolean): void { +function cmdStatePlannedPhase(cwd: string, phaseNumber: string | number, phaseName: string | null | undefined, planCount: number | null | undefined, raw: boolean): void { const statePath = planningPaths(cwd).state; if (!fs.existsSync(statePath)) { output({ error: 'STATE.md not found' }, raw, undefined); @@ -3286,6 +3286,7 @@ function cmdStatePlannedPhase(cwd: string, phaseNumber: string | number, planCou const intent: StateTransitionIntent = { kind: 'plannedPhase', phaseNumber, + phaseName: phaseName ?? null, planCount: planCount ?? null, }; const deps: StateTransitionDeps = { @@ -3293,12 +3294,23 @@ function cmdStatePlannedPhase(cwd: string, phaseNumber: string | number, planCou sourcePath: statePath, }; + // #3395 / #2736: the transition holds the exact display name. plannedPhaseCore + // writes it into the Current Position `Phase: N (Name) — READY TO EXECUTE` + // line, and the prose re-derivation of current_phase_name truncates names + // that themselves contain a parenthetical — the authoritative override keeps + // the exact value, exactly as cmdStateBeginPhase does for its EXECUTING line. + const rmwOptions: ReadModifyWriteOptions = { + resync: false, + deriveProgressKeys: true, + authoritativeFm: intent.phaseName ? { current_phase_name: intent.phaseName } : undefined, + }; + let updated: string[] = []; readModifyWriteStateMd(statePath, (content) => { const result = transitionCore(content, intent, deps); updated = result.updated; return result.content; - }, cwd, { resync: false, deriveProgressKeys: true }); + }, cwd, rmwOptions); const result = updated.length === 0 ? { updated, phase: phaseNumber, plan_count: planCount, warning: 'STATE.md Current Position has no recognized labels — transition was a no-op. Verify STATE.md uses the canonical labeled format (Status:, Total Plans in Phase:, etc.).' } diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 33b9e7aac..855101d9f 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -3214,6 +3214,171 @@ describe('#3052: planned-phase preserves same-date last_activity_desc', () => { }); }); +// ───────────────────────────────────────────────────────────────────────────── +// #3395: state planned-phase must refresh the Current Position `Phase:` line +// (the body source the frontmatter resync and `state json` re-derive +// current_phase from) instead of leaving a stale one behind, and must persist +// its --name argument instead of silently dropping it. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3395: planned-phase refreshes the stale Phase line and persists --name', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createFixture(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + const PINNED_ENV = { GSD_NOW_MS: String(Date.parse('2026-08-14T15:00:00.000Z')) }; + + function frontmatterBlock(stateContent) { + const m = stateContent.match(/^---\r?\n([\s\S]*?)\r?\n---/); + return m ? m[1] : ''; + } + + // The issue's repro shape: frontmatter already carries the correct decimal + // sub-phase, but the body's `## Current Position` still describes the + // PREVIOUS phase's completion prose. + function writeStalePhaseLineFixture() { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '---', + "current_phase: '35.3'", + 'current_phase_name: unattended-launch-prerequisites', + 'status: planning', + '---', + '', + '# Project State', + '', + '## Current Position', + '', + 'Phase: 35.1 (unattended-launch-prerequisites) — COMPLETE (4/4 plans)', + 'Status: Planning', + 'Total Plans in Phase: 4', + 'Last Activity: 2026-08-01', + '', + ].join('\n'), + ); + } + + test('issue repro: stale body Phase line is refreshed and current_phase stays coherent end to end', () => { + writeStalePhaseLineFixture(); + const result = runGsdTools(['state', 'planned-phase', '--phase', '35.3', '--plans', '3'], tmpDir, PINNED_ENV); + assert.ok(result.success, `Command failed: ${result.error}`); + + const stateContent = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + const fm = frontmatterBlock(stateContent); + // AC #1 outcome: the correct frontmatter value survives the write. + assert.ok(/current_phase:[^\n]*35\.3/.test(fm), + `frontmatter current_phase must stay 35.3; frontmatter was:\n${fm}`); + // The stale body source must not survive the transition that just + // declared 35.3 planned — it is the source every body-derived consumer + // (state json included) re-reads. + assert.ok(!stateContent.includes('35.1'), + `the stale 35.1 phase prose must be refreshed away; STATE.md was:\n${stateContent}`); + assert.ok(/Phase: 35\.3 — READY TO EXECUTE/m.test(stateContent), + `Current Position Phase line must read "Phase: 35.3 — READY TO EXECUTE"; STATE.md was:\n${stateContent}`); + // The read path must agree with the write path. + const json = JSON.parse(runGsdTools(['state', 'json', '--raw'], tmpDir, PINNED_ENV).output); + assert.strictEqual(json.current_phase, '35.3', + `state json must report the refreshed phase, got: ${json.current_phase}`); + }); + + test('--name is persisted into the Phase line and frontmatter, not silently dropped', () => { + writeStalePhaseLineFixture(); + const result = runGsdTools( + ['state', 'planned-phase', '--phase', '36', '--name', 'Core Foundation', '--plans', '5'], + tmpDir, + PINNED_ENV, + ); + assert.ok(result.success, `Command failed: ${result.error}`); + + const stateContent = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.ok(/Phase: 36 \(Core Foundation\) — READY TO EXECUTE/m.test(stateContent), + `Current Position Phase line must carry the passed name; STATE.md was:\n${stateContent}`); + const fm = frontmatterBlock(stateContent); + // AC #2: the body phase source genuinely changed this transition, so + // current_phase re-derives from the refreshed line. + assert.ok(/current_phase:[^\n]*36/.test(fm), + `current_phase must follow the genuinely changed body phase source (36); frontmatter was:\n${fm}`); + assert.ok(/current_phase_name:[^\n]*Core Foundation/.test(fm), + `current_phase_name must persist the passed name; frontmatter was:\n${fm}`); + }); + + test('--name containing a parenthetical survives intact in frontmatter (#2736 mirror)', () => { + writeStalePhaseLineFixture(); + const result = runGsdTools( + ['state', 'planned-phase', '--phase', '36', '--name', 'auth (oauth) refresh', '--plans', '5'], + tmpDir, + PINNED_ENV, + ); + assert.ok(result.success, `Command failed: ${result.error}`); + + const fm = frontmatterBlock(fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8')); + assert.ok(/current_phase_name:[^\n]*auth \(oauth\) refresh/.test(fm), + `current_phase_name must carry the exact authoritative name (prose re-derivation is lossy for nested parens); frontmatter was:\n${fm}`); + }); + + test('canonical labeled fixture without frontmatter current_phase: Phase line still refreshed', () => { + // No YAML frontmatter disagreement here — pins that the Phase-line refresh + // also applies to the plain template shape (fields only, Current Position + // `Phase: 1 of 5 (setup)` template form). + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '# Project State', + '', + '**Current Phase:** 1', + '**Total Plans in Phase:** 0', + '**Status:** Planning', + '**Last Activity:** 2026-03-20', + '', + '## Current Position', + 'Phase: 1 of 5 (setup)', + 'Plan: 0 of 5 in current phase', + 'Status: Planning', + 'Last activity: 2026-03-20 -- Phase 1 complete', + '', + ].join('\n'), + ); + const result = runGsdTools( + ['state', 'planned-phase', '--phase', '2', '--name', 'Core', '--plans', '5'], + tmpDir, + PINNED_ENV, + ); + assert.ok(result.success, `Command failed: ${result.error}`); + + const stateContent = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.ok(/Phase: 2 \(Core\) — READY TO EXECUTE/m.test(stateContent), + `Current Position Phase line must be refreshed from the template form; STATE.md was:\n${stateContent}`); + }); + + test('no Current Position section: command still succeeds and body Current Phase field is untouched', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '# Project State', + '', + '**Status:** Planning', + '**Total Plans in Phase:** 0', + '**Last Activity:** 2024-01-01', + '**Current Phase:** 3', + '', + ].join('\n'), + ); + const result = runGsdTools(['state', 'planned-phase', '--phase', '3', '--plans', '5'], tmpDir, PINNED_ENV); + assert.ok(result.success, `Command failed: ${result.error}`); + + const stateContent = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.ok(/\*\*Current Phase:\*\* 3/.test(stateContent), + `body **Current Phase:** field must be untouched when no Current Position section exists; STATE.md was:\n${stateContent}`); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // bug #1070 regression: "Complete ✓" terminal status must yield to planned-phase // ─────────────────────────────────────────────────────────────────────────────