From 73418c516fd6a43e91357826914661eacc3c5745 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 1 Aug 2026 00:02:41 -0400 Subject: [PATCH] fix(#2956): scope Phase extraction to ## Current Position (3rd gen of #2444/#2567) (#2961) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#2956): fail-first regressions for Phase scoped to ## Current Position Third generation of #2444 / #2567. Stopped At / Paused At were scoped to ## Session; Phase (canonically in ## Current Position per templates/state.md) was left unscoped, so a historical Phase: / **Phase:** line in an archive section overwrites current_phase on every write. Since current_phase is routing input for gsd-progress / --next, the rewind routes work to the wrong phase. Six failing-first regressions + one round-trip: - shape B: bold **Phase:** 19 archive BELOW the section - shape C: plain archive Phase: 19 ABOVE the section - bootstrap h3 ### Current Position variant - CRLF variant - Phase token in decisions prose (over-broad-fix guard) - Paused At read-path parity with the write seam (## Session) - write-then-read round trip stays at 22 (read/write agreement) Folded into tests/state.test.cjs (lint:regression-test-names bans a new tests/bug-NNNN-*.test.cjs file). * fix(#2956): scope Phase extraction to ## Current Position at both seams Third generation of #2444 / #2567. Stopped At / Paused At were scoped to ## Session by those fixes; Phase (canonically in ## Current Position per templates/state.md) was left unscoped, so a historical Phase: / **Phase:** line in an archive section silently overwrote current_phase on every write. Because current_phase is routing input for gsd-progress / --next, the rewind routes work to the wrong phase. Fix mirrors the proven #2444 seam exactly: - new matchCurrentPositionSection helper (collectSection-based, CRLF-tolerant, level-flexible for the bootstrap ### Current Position h3 variant), sited next to matchSessionSection. - read path (cmdStateSnapshot): extract Phase from matchCurrentPositionSection ?? body. Also scope Paused At to matchSessionSection ?? body so the read seam agrees with the write seam (which already scoped Paused At to ## Session). - write path (buildStateFrontmatter): extract Phase from matchCurrentPositionSection ?? bodyContent. stateExtractField itself is untouched (its bold/plain precedence is load-bearing for other fields — the #3265 test depends on it), and preferNewerLastActivity is untouched (Last Activity has no canonical section; its date-direction guard is deliberate). Fall back to full-body when no ## Current Position section exists so files without the heading keep current behaviour. * chore(#2956): add changeset fragment (pr:0 placeholder, backfill after PR) * test(#2956): make round-trip test actually trigger the write-path resync The write-then-read round-trip test used 'state update Status "Executing"' on a fixture with no Status field, so the update was a no-op (updated:false) and no frontmatter resync ran through buildStateFrontmatter — the assertion on the written frontmatter then failed not because the fix is wrong, but because no write happened. Add a **Status:** field so the update performs a real field update (updated:true) and forces the resync. Verified locally: pre-fix this writes current_phase:19 (the archive value); post-fix it writes 22. The code fix is correct (5 of 7 RED tests passed; the 2 failures were this defective test). This is the 'fix the bad test' half of the TDD-loop rule. * chore(#2956): backfill changeset PR number 2961 --------- Co-authored-by: sim --- .changeset/curious-seals-zip.md | 5 + src/state.cts | 50 +++++++- tests/state.test.cjs | 216 ++++++++++++++++++++++++++++++++ 3 files changed, 268 insertions(+), 3 deletions(-) create mode 100644 .changeset/curious-seals-zip.md diff --git a/.changeset/curious-seals-zip.md b/.changeset/curious-seals-zip.md new file mode 100644 index 000000000..a2750c5db --- /dev/null +++ b/.changeset/curious-seals-zip.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2961 +--- +**`current_phase` no longer rewinds to an archived phase when STATE.md carries a historical `Phase:` line** — a stale `Phase:` or `**Phase:**` line in an archive section of a long-lived STATE.md silently overwrote `current_phase` on every state write, and because `current_phase` drives `gsd-progress` and `--next` routing the rewind sent work to the wrong phase. Phase extraction is now scoped to the `## Current Position` section (mirroring the existing `## Session` scoping for Stopped At / Paused At). (#2956) diff --git a/src/state.cts b/src/state.cts index 245135d85..6701c9ece 100644 --- a/src/state.cts +++ b/src/state.cts @@ -1307,6 +1307,31 @@ function matchSessionSection(body: string): string | null { return section ? section.body : null; } +/** + * Match the "Current Position" section body from a STATE.md body. #2956: this + * is the Phase analogue of matchSessionSection. `Phase` canonically lives under + * `## Current Position` (gsd-core/templates/state.md), so — like Stopped At / + * Paused At under `## Session` — it must be extracted from THAT section, not + * from the first `Phase:` / `**Phase:**` line anywhere in the body. Without the + * scope, a historical `Phase:` line in an archive section silently overwrites + * `current_phase` on every write, and because `current_phase` is routing input + * for gsd-progress / --next the rewind routes work to the wrong phase. + * + * Level-flexible: the canonical template uses an h2 `## Current Position`, the + * bootstrap template an h3 `### Current Position` (templates/state.md). Both + * must match — mirroring how matchSessionSection recognises `## Session` and + * `## Session Continuity`. Exact 'current position' text match (case-insensitive) + * excludes unrelated headings. Built on the same `collectSection` seam as + * matchSessionSection, so it inherits that seam's CRLF tolerance (#2444 fix). + * Returns the section body, or null (caller falls back to full-body search). + */ +function matchCurrentPositionSection(body: string): string | null { + const isCurrentPosition = (h: HeadingToken): boolean => + (h.level === 2 || h.level === 3) && h.text.trim().toLowerCase() === 'current position'; + const section = collectSection(body, isCurrentPosition, { levelBounded: true }); + return section ? section.body : null; +} + /** * #2567: prevent a stale archive "Last activity:" line from overwriting a * newer frontmatter value. `stateExtractField` matches the first body @@ -1392,7 +1417,14 @@ function cmdStateSnapshot(cwd: string, raw: boolean): void { }; // Extract basic fields — frontmatter keys take precedence over body - const prosePhase = parseProsePhaseField(stateExtractField(body, 'Phase')); + // #2956: scope `Phase` extraction to ## Current Position so a historical + // Phase: / **Phase:** line in an archive section cannot overwrite the current + // value. Phase canonically lives in ## Current Position (templates/state.md), + // so it is scopeable exactly like Stopped At under ## Session. Fall back to + // full-body search only when no ## Current Position section exists, so files + // with no section heading keep their current behaviour. + const currentPositionScope = matchCurrentPositionSection(body) ?? body; + const prosePhase = parseProsePhaseField(stateExtractField(currentPositionScope, 'Phase')); const currentPhase = fmScalar('current_phase') ?? stateExtractField(body, 'Current Phase') ?? prosePhase.phase; const currentPhaseName = fmScalar('current_phase_name') ?? stateExtractField(body, 'Current Phase Name') ?? prosePhase.name; const totalPhasesRaw = fmScalar('total_phases') ?? stateExtractField(body, 'Total Phases'); @@ -1404,7 +1436,12 @@ function cmdStateSnapshot(cwd: string, raw: boolean): void { const proseLastActivity = parseProseLastActivityField(rawLastActivity); const lastActivity = fmScalar('last_activity') ?? proseLastActivity.date ?? rawLastActivity; const lastActivityDesc = fmScalar('last_activity_desc') ?? stateExtractField(body, 'Last Activity Description') ?? proseLastActivity.description; - const pausedAt = fmScalar('paused_at') ?? stateExtractField(body, 'Paused At'); + // #2956: Paused At canonically lives in ## Session (see the comment above + // preferNewerLastActivity and the write seam in buildStateFrontmatter). The + // write seam already scopes it to ## Session; this read seam must agree, so a + // stale "Paused At:" in a Session Continuity Archive cannot win here either. + const sessionScope = matchSessionSection(body) ?? body; + const pausedAt = fmScalar('paused_at') ?? stateExtractField(sessionScope, 'Paused At'); // Parse numeric fields const totalPhases = totalPhasesRaw ? parseInt(totalPhasesRaw, 10) : null; @@ -1552,7 +1589,14 @@ function extractRetiredPhaseNumbers(scope: string): Set { * reliably via `state json` instead of fragile regex parsing. */ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined): Record { - const prosePhase = parseProsePhaseField(stateExtractField(bodyContent, 'Phase')); + // #2956: scope `Phase` extraction to ## Current Position (mirrors the read + // path in cmdStateSnapshot and the Stopped At / Paused At ## Session scoping + // below). Phase canonically lives in ## Current Position (templates/state.md); + // without the scope, a historical Phase: / **Phase:** line in an archive + // section overwrites current_phase here, and the next read surfaces it. Fall + // back to full-body search when no ## Current Position section exists. + const currentPositionScope = matchCurrentPositionSection(bodyContent) ?? bodyContent; + const prosePhase = parseProsePhaseField(stateExtractField(currentPositionScope, 'Phase')); const currentPhase = stateExtractField(bodyContent, 'Current Phase') ?? prosePhase.phase; const currentPhaseName = stateExtractField(bodyContent, 'Current Phase Name') ?? prosePhase.name; const currentPlan = stateExtractField(bodyContent, 'Current Plan'); diff --git a/tests/state.test.cjs b/tests/state.test.cjs index a68263427..8605438b5 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -162,6 +162,177 @@ describe('state-snapshot command', () => { assert.strictEqual(output.paused_at, 'Phase 3, Plan 1, Task 2 - mid-implementation', 'paused_at extracted'); }); + // ─── Regression: #2956 — Phase must be scoped to ## Current Position ────── + // Third generation of #2444 / #2567. Stopped At / Paused At were scoped to + // ## Session; Phase (which canonically lives in ## Current Position per + // gsd-core/templates/state.md) was left unscoped, so a historical Phase: / + // **Phase:** line in an archive section silently overwrote current_phase on + // every write. Because current_phase is routing input for gsd-progress / --next, + // the rewind routes work to the wrong phase — not merely a stale display. + + test('#2956 scopes Phase to ## Current Position — bold archive line below the section (shape B)', () => { + // Bold **Phase:** 19 in an archive BELOW ## Current Position. The unscoped + // extractor's bold pattern wins outright (it is tried first, unanchored), + // so the read returns 19 instead of 22. Scoping to the section fixes it. + const stateContent = [ + '# Project State', + '', + '## Current Position', + '', + 'Phase: 22 (Documentation hygiene) — COMPLETE', + '', + '## Archive — earlier milestones', + '', + '**Phase:** 19 **complete — shipped v2.43.0**', + '', + ].join('\n'); + + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), stateContent); + + const result = runGsdTools('state-snapshot', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.current_phase, '22', 'current_phase must come from ## Current Position, not the bold archive line'); + }); + + test('#2956 scopes Phase to ## Current Position — plain archive line above the section (shape C)', () => { + // Plain archive Phase: 19 ABOVE ## Current Position. Without scoping the + // plain pattern (^Phase:, /im) matches the first line-start occurrence in + // document order — the archive line — and returns 19 instead of 22. + const stateContent = [ + '# Project State', + '', + '## Archive — earlier milestones', + '', + 'Phase: 19 **complete — shipped v2.43.0**', + '', + '## Current Position', + '', + 'Phase: 22 (Documentation hygiene) — COMPLETE', + '', + ].join('\n'); + + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), stateContent); + + const result = runGsdTools('state-snapshot', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.current_phase, '22', 'current_phase must come from ## Current Position, not the plain archive line above it'); + }); + + test('#2956 scopes Phase to ### Current Position (bootstrap h3 variant)', () => { + // gsd-core/templates/state.md ships a bootstrap layout that uses a level-3 + // ### Current Position heading. The section matcher must recognise BOTH h2 + // and h3, mirroring how matchSessionSection recognises ## Session and + // ## Session Continuity — matching only h2 would silently drop the h3 shape. + const stateContent = [ + '# Project State', + '', + '### Current Position', + '', + 'Phase: 22 (Documentation hygiene)', + '', + '### Archive', + '', + '**Phase:** 19', + '', + ].join('\n'); + + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), stateContent); + + const result = runGsdTools('state-snapshot', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.current_phase, '22', 'current_phase must resolve from the h3 ### Current Position section'); + }); + + test('#2956 scopes Phase to ## Current Position under CRLF', () => { + // The #2444 seam was CRLF-fixed by migrating onto collectSection (which + // strips the trailing \r before heading-text extraction). The Phase scope + // must inherit that CRLF tolerance — a hand-rolled [ \t]*\n boundary would + // silently fail to match a ## Current Position\r\n heading. + const stateContent = [ + '# Project State', + '', + '## Current Position', + '', + 'Phase: 22 (Documentation hygiene)', + '', + '## Archive — earlier milestones', + '', + '**Phase:** 19', + '', + ].join('\r\n'); + + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), stateContent); + + const result = runGsdTools('state-snapshot', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.current_phase, '22', 'current_phase must come from ## Current Position under CRLF'); + }); + + test('#2956 ignores a Phase token in decisions prose outside ## Current Position', () => { + // A decisions-table row or prose mention of "Phase 19" elsewhere is NOT the + // current phase. Scoping it out is correct, not a regression — this is the + // over-broad-fix guard. + const stateContent = [ + '# Project State', + '', + '## Current Position', + '', + 'Phase: 22 (Documentation hygiene)', + '', + '### Decisions', + '', + '| Decided to defer Phase 19 to the next milestone | 2026-07-01 |', + '', + ].join('\n'); + + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), stateContent); + + const result = runGsdTools('state-snapshot', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.current_phase, '22', 'a Phase token in decisions prose must not leak into current_phase'); + }); + + test('#2956 scopes Paused At to ## Session on the read path (parity with the write seam)', () => { + // The WRITE seam (buildStateFrontmatter src/state.cts ~1576) already scopes + // Paused At to ## Session. The READ seam (cmdStateSnapshot) read it + // unscoped — a parity gap. A stale "Paused At:" in a Session Continuity + // Archive below the real ## Session must not win on the read path. + const stateContent = [ + '# Project State', + '', + '## Current Position', + '', + 'Phase: 22 (Documentation hygiene)', + '', + '## Session', + '', + '**Paused At:** Phase 22, Plan 1, Task 2 - mid-implementation', + '', + '## Session Continuity Archive', + '', + '**Paused At:** Phase 19, Plan 3 (stale)', + '', + ].join('\n'); + + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), stateContent); + + const result = runGsdTools('state-snapshot', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.paused_at, 'Phase 22, Plan 1, Task 2 - mid-implementation', 'paused_at must come from ## Session, not the archive'); + }); + describe('--cwd override', () => { let outsideDir; @@ -588,6 +759,51 @@ describe('STATE.md frontmatter sync', () => { assert.ok(content.includes('status: paused'), 'frontmatter should reflect latest status'); }); + test('#2956 write-then-read does not rewind current_phase past an archive Phase line', () => { + // The write seam (buildStateFrontmatter) and the read seam (cmdStateSnapshot) + // must agree: a state write that re-syncs frontmatter must not pick up the + // archive **Phase:** 19 line and write current_phase: 19, which the next + // state-snapshot read would then surface. Round-trip must stay at 22. + // + // The fixture carries a **Status:** field so `state update Status` performs + // a real field update (updated:true) and forces the frontmatter resync + // through buildStateFrontmatter — without an existing Status field the + // update is a no-op (updated:false) and no write occurs. + const stateContent = [ + '# Project State', + '', + '## Current Position', + '', + 'Phase: 22 (Documentation hygiene)', + '', + '**Status:** Ready', + '', + '## Archive — earlier milestones', + '', + '**Phase:** 19 **complete — shipped v2.43.0**', + '', + ].join('\n'); + + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), stateContent); + + // state update forces a frontmatter sync through buildStateFrontmatter. + const writeResult = runGsdTools('state update Status "Executing"', tmpDir); + assert.ok(writeResult.success, `write failed: ${writeResult.error}`); + const writeOutput = JSON.parse(writeResult.output); + assert.strictEqual(writeOutput.updated, true, 'state update must perform a real field update to force the resync'); + + // The persisted frontmatter must not have rewound to the archive phase. + const written = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.ok(/current_phase:\s*22\b/m.test(written), 'written frontmatter current_phase must be 22 (not the archive 19)'); + assert.ok(!/^current_phase:\s*19\b/m.test(written), 'written frontmatter must NOT carry the archive phase 19'); + + // And a fresh read must agree. + const readResult = runGsdTools('state-snapshot', tmpDir); + assert.ok(readResult.success, `read failed: ${readResult.error}`); + const output = JSON.parse(readResult.output); + assert.strictEqual(output.current_phase, '22', 'read-after-write current_phase must stay 22 (round-trip agreement)'); + }); + test('preserves frontmatter status when body Status field is missing', () => { // Simulate: frontmatter has status: executing, but body lost Status: field fs.writeFileSync(