From 50ff7a87078a2bf0ce0b49d70370f6fe6da09d82 Mon Sep 17 00:00:00 2001 From: Behruz Nassre Esfahani Date: Tue, 30 Jun 2026 10:16:40 -0700 Subject: [PATCH] fix(#1776): scope prune phase resolution to ## Current Position (#1832) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#1776): scope prune phase resolution to ## Current Position cmdStatePrune resolved the current phase by extracting the `Phase` field over the WHOLE STATE.md body. stateExtractField's fallback chain ends in a pipe-table match (`| Phase | N |`), so a STATE.md lacking a `Current Phase` field and a prose `Phase:` line — but carrying an unrelated `Phase`-labelled table row (e.g. a historical verification table) — resolved that stale table cell as the current phase and computed a wrong cutoff (bailing "Only N phases" or pruning at a stale boundary). Resolve the phase via the same canonical chain buildStateFrontmatter uses — frontmatter `current_phase` → `Current Phase` field → prose `Phase: X of Y` — but scope ONLY the prose term to the `## Current Position` section via the fence-aware locateCurrentPosition seam (new exported sliceCurrentPositionSection). Frontmatter and the explicit `Current Phase` field stay document-wide (they are unambiguous); the shared stateExtractField is not narrowed for any other caller. Tests (folded into tests/state-prune.test.cjs): a stray `| Phase | 2 |` table with the real phase in frontmatter no longer drives the cutoff (fail-first on base); template-conformant STATE.md is unchanged; and a fast-check boundary-containment property that a `| Phase | N |` row outside Current Position never leaks into the scoped resolution. Co-Authored-By: Claude Opus 4.8 (1M context) * chore(#1776): add changeset for prune Current Position scoping Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) Co-authored-by: Tom Boucher --- .changeset/gallant-wasps-wave.md | 5 ++ gsd-core/bin/lib/state-transition.cjs | 15 ++++ src/state-transition.cts | 15 ++++ src/state.cts | 31 ++++++-- tests/state-prune.test.cjs | 101 ++++++++++++++++++++++++++ 5 files changed, 161 insertions(+), 6 deletions(-) create mode 100644 .changeset/gallant-wasps-wave.md diff --git a/.changeset/gallant-wasps-wave.md b/.changeset/gallant-wasps-wave.md new file mode 100644 index 000000000..118c6ddc1 --- /dev/null +++ b/.changeset/gallant-wasps-wave.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1832 +--- +state prune now resolves the current phase from the canonical location — frontmatter current_phase, the Current Phase field, or the prose Phase: line scoped to the ## Current Position section — instead of extracting Phase over the whole document, where stateExtractField's pipe-table fallback could latch onto an unrelated | Phase | N | row (e.g. a historical verification table) and compute a wrong prune cutoff. diff --git a/gsd-core/bin/lib/state-transition.cjs b/gsd-core/bin/lib/state-transition.cjs index 3104b9121..e56900497 100644 --- a/gsd-core/bin/lib/state-transition.cjs +++ b/gsd-core/bin/lib/state-transition.cjs @@ -19,6 +19,7 @@ exports.STATE_MD_SECTIONS = exports.FIELD_CLASSIFICATION = void 0; exports.getFieldClassification = getFieldClassification; exports.applyStatePreservation = applyStatePreservation; exports.transitionCore = transitionCore; +exports.sliceCurrentPositionSection = sliceCurrentPositionSection; // eslint-disable-next-line @typescript-eslint/no-require-imports const frontmatter = require("./frontmatter.cjs"); const state_document_cjs_1 = require("./state-document.cjs"); @@ -319,6 +320,20 @@ function locateCurrentPosition(body) { } return { start, end }; } +/** + * Return the body text of the `## Current Position` section, or `null` when it + * is absent. Reuses the fence-aware `locateCurrentPosition` locator (ADR-1372). + * + * Exposed so callers that must read a position field (e.g. `cmdStatePrune`, + * #1776) can scope extraction to the canonical section instead of the whole + * document — where `stateExtractField`'s pipe-table fallback could otherwise + * latch onto an unrelated `| Phase | N |` row elsewhere in STATE.md. This + * scopes the *caller*; the shared extractor is left broad for every other use. + */ +function sliceCurrentPositionSection(body) { + const span = locateCurrentPosition(body); + return span === null ? null : body.slice(span.start, span.end); +} /** * First-time ## Current Position mutation: update Phase / Plan / Status / * Last activity lines. Mirrors state.cts:2261-2324 byte-for-behaviour diff --git a/src/state-transition.cts b/src/state-transition.cts index f7051d6ab..d796aac10 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -528,6 +528,21 @@ function locateCurrentPosition(body: string): { start: number; end: number } | n return { start, end }; } +/** + * Return the body text of the `## Current Position` section, or `null` when it + * is absent. Reuses the fence-aware `locateCurrentPosition` locator (ADR-1372). + * + * Exposed so callers that must read a position field (e.g. `cmdStatePrune`, + * #1776) can scope extraction to the canonical section instead of the whole + * document — where `stateExtractField`'s pipe-table fallback could otherwise + * latch onto an unrelated `| Phase | N |` row elsewhere in STATE.md. This + * scopes the *caller*; the shared extractor is left broad for every other use. + */ +export function sliceCurrentPositionSection(body: string): string | null { + const span = locateCurrentPosition(body); + return span === null ? null : body.slice(span.start, span.end); +} + /** * First-time ## Current Position mutation: update Phase / Plan / Status / * Last activity lines. Mirrors state.cts:2261-2324 byte-for-behaviour diff --git a/src/state.cts b/src/state.cts index c61952aae..05779b201 100644 --- a/src/state.cts +++ b/src/state.cts @@ -32,7 +32,7 @@ const { extractFrontmatter, reconstructFrontmatter } = frontmatter; import scanPhasePlans = require('./plan-scan.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports import stateTransitionMod = require('./state-transition.cjs'); -const { transitionCore, applyStatePreservation } = stateTransitionMod; +const { transitionCore, applyStatePreservation, sliceCurrentPositionSection } = stateTransitionMod; type StateTransitionIntent = stateTransitionMod.StateTransitionIntent; type StateTransitionDeps = stateTransitionMod.StateTransitionDeps; type PhaseInventoryRecord = stateTransitionMod.PhaseInventoryRecord; @@ -2501,12 +2501,31 @@ function cmdStatePrune(cwd: string, options: StatePruneOptions, raw: boolean): v const keepRecent = parseInt(String(options.keepRecent), 10) || 3; const dryRun = !!options.dryRun; - // #1760: the canonical STATE.md template emits `Phase: [X] of [Y]`, not - // `Current Phase:`. Read both (mirroring buildStateFrontmatter / - // resolvePhaseIdForCompletePhase) so prune engages on template-conformant - // STATE.md instead of bailing with "Only 0 phases — nothing to prune". + // 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. const rawState = fs.readFileSync(statePath, 'utf-8'); - const currentPhaseRaw = stateExtractField(rawState, 'Current Phase') || stateExtractField(rawState, 'Phase'); + const fm = extractFrontmatter(rawState) as Record; + const body = stripFrontmatter(rawState); + // Mirror buildStateFrontmatter's fmScalar: only string/number/boolean + // frontmatter scalars are usable (an object/array `current_phase` is ignored, + // which also avoids a base-to-string on a non-primitive). + const fmRawPhase = fm.current_phase; + const fmCurrentPhase = + typeof fmRawPhase === 'string' ? (fmRawPhase.trim() || null) + : typeof fmRawPhase === 'number' || typeof fmRawPhase === 'boolean' ? String(fmRawPhase) + : null; + const positionSection = sliceCurrentPositionSection(body); + const prosePhase = + positionSection !== null ? parseProsePhaseField(stateExtractField(positionSection, 'Phase')).phase : null; + const currentPhaseRaw = fmCurrentPhase ?? stateExtractField(body, 'Current Phase') ?? prosePhase; const currentPhase = parseInt(String(currentPhaseRaw), 10) || 0; const cutoff = currentPhase - keepRecent; diff --git a/tests/state-prune.test.cjs b/tests/state-prune.test.cjs index a8e80d879..211d83add 100644 --- a/tests/state-prune.test.cjs +++ b/tests/state-prune.test.cjs @@ -8,7 +8,10 @@ const { test, describe, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); +const fc = require('./helpers/fast-check-setup.cjs'); const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); +const { sliceCurrentPositionSection } = require('../gsd-core/bin/lib/state-transition.cjs'); +const { stateExtractField } = require('../gsd-core/bin/lib/state-document.cjs'); function writeStateMd(tmpDir, content) { fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), content); @@ -275,3 +278,101 @@ describe('state prune (#1970)', () => { }); }); }); + +// #1776 — `cmdStatePrune` resolved the current phase by extracting the `Phase` +// field over the WHOLE STATE.md; `stateExtractField`'s pipe-table fallback then +// matched any `| Phase | N |` row anywhere (e.g. a historical verification +// table), so a stale table cell drove the cutoff. The fix scopes the prose +// `Phase:` lookup to the canonical `## Current Position` section. +describe('#1776: prune reads the current phase only from ## Current Position', () => { + let tmpDir; + beforeEach(() => { tmpDir = createTempProject(); }); + afterEach(() => { cleanup(tmpDir); }); + + // hiSandog's fixture shape: an unrelated `| Phase | 2 |` verification table, + // with the real position only in frontmatter (`current_phase: 12`) and NO + // `Current Phase` body field / prose `Phase:` line. Pre-fix, prune ignored + // frontmatter and `stateExtractField(body, 'Phase')` fell to the table cell → + // currentPhase=2 → cutoff -1 → "Only 2 phases" bail. Post-fix it reads the + // frontmatter phase and the table cell is never consulted. + test('a stray | Phase | N | table does not override the canonical (frontmatter) phase', () => { + writeStateMd(tmpDir, [ + '---', + 'gsd_state_version: 1.0', + 'current_phase: 12', + 'status: executing', + '---', + '', + '# GSD State', + '', + '## Verification History', + '', + '| Field | Value |', + '| --- | --- |', + '| Phase | 2 |', + '| Result | passed |', + '', + '## Decisions', + '', + '- [Phase 1]: Old decision', + '- [Phase 4]: Mid decision', + '- [Phase 11]: Recent decision', + '', + ].join('\n')); + + const result = runGsdTools('state prune --keep-recent 3', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + const out = JSON.parse(result.output); + + // Phase 12, keep-recent 3 → cutoff 9. If the table cell (2) had leaked, the + // cutoff would be -1 and prune would bail "Only 2 phases — nothing to prune". + assert.strictEqual(out.pruned, true, `expected prune to engage, got: ${JSON.stringify(out)}`); + assert.strictEqual(out.cutoff_phase, 9); + }); + + // AC2 — template-conformant STATE.md (prose Phase under Current Position, no + // stray table) is unchanged: prune still engages off the real phase. + test('template-conformant Current Position (no stray table) still prunes', () => { + writeStateMd(tmpDir, [ + '# GSD State', + '', + '## Current Position', + '', + 'Phase: 10 of 15', + '', + '## Decisions', + '', + '- [Phase 1]: Old', + '- [Phase 9]: Recent', + '', + ].join('\n')); + + const result = runGsdTools('state prune --keep-recent 3', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + const out = JSON.parse(result.output); + assert.strictEqual(out.cutoff_phase, 7); + }); + + // Boundary-containment property — the core scoping invariant: a `| Phase | N |` + // row OUTSIDE the `## Current Position` section is never visible to extraction + // scoped to that section. The section here carries NO `Phase:` line, so a + // correct slice yields `null`; a whole-document leak would instead surface the + // stray table cell (`stateExtractField`'s pipe-table fallback). The table is + // placed both before and after the section to exercise both span boundaries. + test('property: a | Phase | N | table outside Current Position is excluded from the scoped slice', () => { + fc.assert( + fc.property( + fc.integer({ min: 0, max: 998 }), // stray table phase + fc.boolean(), // stray table before (true) or after (false) the section + (stray, before) => { + const table = ['## History', '', '| Field | Value |', '| --- | --- |', `| Phase | ${stray} |`, '']; + const position = ['## Current Position', '', '**Status:** Executing', '']; // deliberately no `Phase:` line + const body = ['# GSD State', '', ...(before ? [...table, ...position] : [...position, ...table])].join('\n'); + const section = sliceCurrentPositionSection(body); + // Slice must exist and must NOT see the out-of-section table cell. + return section !== null && stateExtractField(section, 'Phase') === null; + } + ) + ); + }); +});