fix(#3395): own the phase line in planned-phase and persist --name (#3490)

* fix(#3395): own the phase line in planned-phase and persist --name * fix(#3395): backfill changeset pr 3490 --------- Co-authored-by: sim <sim@local>
2026-08-14 12:26:47 -04:00
parent 8a718f3385
commit 8bead8b0ff
5 changed files with 225 additions and 9 deletions
--- a/.changeset/serene-birds-sing.md
+++ 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.
--- 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: () => {
--- 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`,
    },
--- 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.).' }
--- 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
 // ─────────────────────────────────────────────────────────────────────────────