From 82ca13f5a57b35eee1febf943d3396de0ce4f949 Mon Sep 17 00:00:00 2001 From: 0xdhx Date: Thu, 30 Jul 2026 12:53:39 -0500 Subject: [PATCH] fix(#2736): write current_phase_name from the transition intent, not the lossy prose round-trip (#2821) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#2736): intent-first current_phase_name on transitions; dash-first prose precedence Primary: completePhase (adapter) and beginPhase (via readModifyWriteStateMd options) pass the intent-held display name to syncStateFrontmatter as an authoritative override, applied after every derive/preserve/carry-forward step — so the lossy prose round-trip can never destroy a name the transition just resolved. Names containing a parenthetical (`Closer-ruling measurement (D1a)`) now land in frontmatter verbatim instead of collapsing to the parenthetical (`D1a`). Secondary (#1695 AC #3 residual): parsePhaseFromProse prefers the em-dash name when it is a genuine name (not a status keyword, not a `Milestone:` tail), else falls back to the parenthetical — satisfying both first-party writer shapes (`N — Name (aside)` and `N (Name) — EXECUTING`). Still lossy for paren-containing names, which is why the intent-first override is the primary fix. plannedPhase carries no name in its intent, so it is naturally out of scope. Fixes #2736 * docs(changeset): backfill PR number for #2736 fragment * fix(#2736): drop an unnecessary type assertion on result.data StateTransitionResult.data is already `Record | undefined`, so the cast was a no-op and tripped @typescript-eslint/no-unnecessary-type- assertion (CI lint-tests red on the first push; every test lane was green). --------- Co-authored-by: Tom Boucher --- .changeset/2736-phase-name-intent-first.md | 5 + gsd-core/bin/lib/state-transition.cjs | 6 +- src/phase-id.cts | 35 ++- src/phase.cts | 10 +- src/state-transition.cts | 6 +- src/state.cts | 61 ++++- tests/frontmatter.test.cjs | 279 +++++++++++++++++++++ tests/phase-id.test.cjs | 50 +++- 8 files changed, 439 insertions(+), 13 deletions(-) create mode 100644 .changeset/2736-phase-name-intent-first.md diff --git a/.changeset/2736-phase-name-intent-first.md b/.changeset/2736-phase-name-intent-first.md new file mode 100644 index 000000000..9386c5050 --- /dev/null +++ b/.changeset/2736-phase-name-intent-first.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2821 +--- +**`phase complete` and `state begin-phase` no longer rewrite `current_phase_name` to the name's own parenthetical** — transitions that already hold the exact display name now pass it to `syncStateFrontmatter` as an authoritative override, so the lossy body-prose re-derivation never runs the final word on a field the transition just resolved. Previously, completing into a phase named `Closer-ruling measurement (D1a)` wrote `current_phase_name: D1a` (the prose parser's paren-over-dash preference harvested the name's own parenthetical), and every downstream consumer of the scalar inherited the mangled name. `parsePhaseFromProse` also gains status-keyword-aware precedence (the #1695 AC #3 residual) for genuinely unknown prose: the em-dash name wins when it is not a status keyword or `Milestone:` tail, so `48 — Closer-ruling measurement (D1a)` now parses to `Closer-ruling measurement` instead of `D1a`. (#2736) diff --git a/gsd-core/bin/lib/state-transition.cjs b/gsd-core/bin/lib/state-transition.cjs index ff6701300..badc4e820 100644 --- a/gsd-core/bin/lib/state-transition.cjs +++ b/gsd-core/bin/lib/state-transition.cjs @@ -314,7 +314,11 @@ function beginPhaseCore(content, intent, deps) { // (do not touch Plan:, Phase:, Status:, stopped_at, progress.percent). body = mutateCurrentPositionResume(body, intent, today, updated); } - return { content: reassemble(body), updated }; + // #2736: surface the #3127 resume decision so the adapter can drop its + // intent-first current_phase_name override on a resume — the core just + // preserved the mid-flight name, and an override would drift frontmatter + // away from the preserved body value. + return { content: reassemble(body), updated, data: { resumed: isAlreadyExecuting } }; } /** * Find the `## Current Position` section, return its `{start, end}` byte diff --git a/src/phase-id.cts b/src/phase-id.cts index b2e3abda1..67d6e102d 100644 --- a/src/phase-id.cts +++ b/src/phase-id.cts @@ -595,9 +595,38 @@ function parsePhaseFromProse(value: string | null): { phase: string | null; name // cannot drive O(n^2) regex backtracking (CPU-exhaustion DoS). A real phase // name is far shorter than the cap. const parenName = str.match(/\(([^)]{1,200})\)/); - const dashName = str.match(/—\s*([^(\n]{1,200}?)(?:\s*\(|$)/); - const rawName = parenName?.[1] ?? dashName?.[1] ?? null; - const name = rawName && !/^(?:complete|executing|not started)$/i.test(rawName.trim()) + // #2736 (the #1695 AC #3 residual): status-keyword-aware precedence. The + // first-party writer shapes are `N — Name (aside)` (completePhaseCore), + // `N (Name) — EXECUTING` (beginPhaseCore), `N — COMPLETE`, and the + // gsd2-import `N (slug) — Milestone: Title`. A blind paren-first read + // harvests the aside as the name on the first shape; a blind dash-first + // read harvests the status keyword on the others. Prefer the em-dash name + // when it is a genuine name, else fall back to the parenthetical. Still + // lossy for names that themselves contain a parenthetical — transitions + // that hold the exact name bypass this parser entirely via the + // syncStateFrontmatter authoritative override. + // + // The em-dash separator is searched on a paren-stripped copy, so an em-dash + // INSIDE a parenthetical name (`16 (Native — Global Hotkey) — EXECUTING`) + // can never be mistaken for the name separator. + const strNoParens = str.replace(/\([^)\n]{0,200}\)/g, ' '); + const dashName = strNoParens.match(/—\s*([^(\n]{1,200}?)\s*$/); + // The precedence-decision vocabulary is deliberately broader than the final + // name-nulling filter below: a dash tail that merely LOOKS like a status + // annotation should lose to a parenthetical name, without changing which + // extracted names are nulled (that set stays the long-standing three). + const STATUS_WORD_RE = /^(?:complete|executing|not started)$/i; + const STATUSY_TAIL_RE = /^(?:completed?|executing|not started|planning|planned|ready(?:\s+to\s+\S.{0,50})?|done|in progress|blocked|paused|verifying)$/i; + const dashRaw = dashName?.[1]?.trim() ?? null; + const dashIsName = dashRaw !== null && dashRaw.length > 0 + && !STATUSY_TAIL_RE.test(dashRaw) + && !/^milestone\s*:/i.test(dashRaw) + // A lone ALL-CAPS token after the dash reads as a status marker whenever a + // parenthetical name exists to prefer (the beginPhase writer's systematic + // `(Name) — STATUS` shape); with no parenthetical it stays the best guess. + && !(parenName && /^[A-Z][A-Z0-9_-]*$/.test(dashRaw)); + const rawName = dashIsName ? dashRaw : (parenName?.[1] ?? dashRaw ?? null); + const name = rawName && !STATUS_WORD_RE.test(rawName.trim()) ? rawName.trim() : null; return { diff --git a/src/phase.cts b/src/phase.cts index 5089447f3..9d80b29b7 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -2456,7 +2456,15 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { planCount, summaryCount, ); - stateContent = syncStateFrontmatter(stateContent, cwd); + // #2736: the transition holds the next phase's exact display name in + // the intent; pass it as authoritative so the sync's prose + // re-derivation cannot rewrite current_phase_name to the name's own + // parenthetical (`Closer-ruling measurement (D1a)` → `D1a`). + stateContent = syncStateFrontmatter( + stateContent, + cwd, + nextPhaseDisplayName ? { current_phase_name: nextPhaseDisplayName } : undefined, + ); writes.push({ filePath: statePath, before: originalStateContent, after: stateContent }); } diff --git a/src/state-transition.cts b/src/state-transition.cts index b26c89be7..9312f4909 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -534,7 +534,11 @@ function beginPhaseCore( body = mutateCurrentPositionResume(body, intent, today, updated); } - return { content: reassemble(body), updated }; + // #2736: surface the #3127 resume decision so the adapter can drop its + // intent-first current_phase_name override on a resume — the core just + // preserved the mid-flight name, and an override would drift frontmatter + // away from the preserved body value. + return { content: reassemble(body), updated, data: { resumed: isAlreadyExecuting } }; } // Local frontmatter type aliases matching frontmatter.cts so we can call diff --git a/src/state.cts b/src/state.cts index b919347d4..59e218521 100644 --- a/src/state.cts +++ b/src/state.cts @@ -66,6 +66,13 @@ interface ReadModifyWriteOptions { resync?: boolean; /** #2440: when true, total_plans/total_phases take derived values even under !resync. */ deriveProgressKeys?: boolean; + /** + * #2736: intent-first frontmatter values forwarded to syncStateFrontmatter. + * Transition adapters that already hold the exact value (e.g. beginPhase's + * display name) pass it here so the lossy body-prose re-derivation can never + * destroy information the transition just resolved. + */ + authoritativeFm?: Record; } interface StateRecordMetricOptions { @@ -1767,7 +1774,7 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined): Re return fm; } -function syncStateFrontmatter(content: string, cwd: string | undefined): string { +function syncStateFrontmatter(content: string, cwd: string | undefined, authoritativeFm?: Record): string { // Read existing frontmatter BEFORE stripping — it may contain values // that the body no longer has (e.g., Status field removed by an agent). // `cwd` already identifies the workspace this content came from, so the STATE.md path is @@ -1870,6 +1877,21 @@ function syncStateFrontmatter(content: string, cwd: string | undefined): string // "Last activity:" line must not overwrite a newer frontmatter value. preferNewerLastActivity(existingFm, derivedFm); + // #2736: intent-first override, applied last. A transition adapter that + // already holds the exact value (completePhase's next-phase display name, + // beginPhase's phase name) passes it here, so the body-prose re-derivation + // above — which is lossy by construction for names containing a + // parenthetical (`Closer-ruling measurement (D1a)` → `D1a`) — never runs + // the final word on a field the transition just resolved. The prose parser + // remains the fallback for genuinely unknown prose only. + if (authoritativeFm) { + for (const [key, value] of Object.entries(authoritativeFm)) { + if (typeof value === 'string' && value.trim().length > 0) { + derivedFm[key] = value; + } + } + } + const yamlStr = reconstructFrontmatter(derivedFm as unknown as Frontmatter); return `---\n${yamlStr}\n---\n\n${body}`; } @@ -2188,7 +2210,7 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string return; } - let synced = syncStateFrontmatter(modified, cwd); + let synced = syncStateFrontmatter(modified, cwd, options?.authoritativeFm); // Post-transform body source fields used for the delta comparison (#1230). // Use `modified` (not `synced`): syncStateFrontmatter only rewrites the frontmatter block, so the body is identical in both — and we need the body the transform produced. @@ -2219,7 +2241,22 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string preBodyStoppedAt, postBodyStoppedAt, preBodyPhaseSource, postBodyPhaseSource, }); - if (preservation.mutated) { + // #2736: re-assert the intent-first values AFTER preservation. On STATE.md + // layouts with no body `Phase:` line, both phase-source snapshots are null + // (equal), so the #1695 restore fires and would put the stale pre-transition + // name back over the authoritative one. Intent beats both the prose + // re-derivation and the curated restore — the transition just resolved it. + let authoritativeReasserted = false; + if (options?.authoritativeFm) { + for (const [key, value] of Object.entries(options.authoritativeFm)) { + if (typeof value === 'string' && value.trim().length > 0 && preservation.postFm[key] !== value) { + preservation.postFm[key] = value; + authoritativeReasserted = true; + } + } + } + + if (preservation.mutated || authoritativeReasserted) { const yamlStr = reconstructFrontmatter(preservation.postFm as unknown as Frontmatter); const body = stripFrontmatter(synced); synced = `---\n${yamlStr}\n---\n\n${body}`; @@ -2315,12 +2352,28 @@ function cmdStateBeginPhase(cwd: string, phaseNumber: string | number, phaseName sourcePath: statePath, }; + // #2736: the transition holds the exact display name; without this the + // post-transform sync re-derives current_phase_name from the freshly + // written `Phase: N (Name) — EXECUTING` line, which truncates any name + // that itself contains a parenthetical. The #1695 delta-gate preservation + // still runs after the sync; the override is re-asserted after it inside + // readModifyWriteStateMd for layouts with no body `Phase:` line. + const rmwOptions: ReadModifyWriteOptions = { + authoritativeFm: intent.phaseName ? { current_phase_name: intent.phaseName } : undefined, + }; let updated: string[] = []; readModifyWriteStateMd(statePath, (content) => { const result = transitionCore(content, intent, deps); updated = result.updated; + // #3127 resume: the core preserved the mid-flight Current Phase Name, so + // the intent-first override must not fire — it would drift frontmatter + // away from the preserved body value. Dropping it here is safe because + // readModifyWriteStateMd consults options only after this callback returns. + if (result.data?.['resumed']) { + delete rmwOptions.authoritativeFm; + } return result.content; - }, cwd); + }, cwd, rmwOptions); output({ updated, phase: phaseNumber, phase_name: phaseName || null, plan_count: planCount || null }, raw, updated.length > 0 ? 'true' : 'false'); } diff --git a/tests/frontmatter.test.cjs b/tests/frontmatter.test.cjs index dc8aefc4d..fa46f8d57 100644 --- a/tests/frontmatter.test.cjs +++ b/tests/frontmatter.test.cjs @@ -1761,3 +1761,282 @@ test('extractFrontmatter handles large frontmatter blocks without body bleed', ( }); }); } + +// ──────────────────────────────────────────────────────────────────────── +// #2736 regressions — `phase complete` / `state begin-phase` rewrite +// current_phase_name to the name's own parenthetical. Placed here beside the +// #1695 delta-gate suite (the same defect family): the transition holds the +// exact display name, then the post-transform syncStateFrontmatter re-derives +// the scalar from body prose via the lossy parsePhaseFromProse. The fix is +// intent-first: adapters pass the intent-held name as an authoritative +// override (completePhase directly, beginPhase via readModifyWriteStateMd +// options), re-asserted after the #1695 preservation so neither the prose +// re-derivation nor the curated restore can destroy it. Parser-precedence +// cases live in tests/phase-id.test.cjs. +// ──────────────────────────────────────────────────────────────────────── +{ + const { describe: __d2736, test: __t2736, beforeEach: __be2736, afterEach: __ae2736 } = require('node:test'); + const __assert2736 = require('node:assert/strict'); + const __fs2736 = require('node:fs'); + const __path2736 = require('node:path'); + const { runGsdTools: __run2736, createTempProject: __mk2736, cleanup: __rm2736 } = require('./helpers.cjs'); + const __state2736 = require('../gsd-core/bin/lib/state.cjs'); + + const PAREN_NAME_2736 = 'Closer-ruling measurement (D1a)'; + + // The issue's repro (a) fixture: curated frontmatter name present, body + // `Phase:` line exactly as completePhaseCore writes it, and no + // `Current Phase Name:` body field (the common STATE.md shape, where the + // replace-only body-field write is a no-op). + function postAdvanceState2736() { + return [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.10', + 'current_phase: 48', + 'current_phase_name: Harness debt clearance', + 'status: planning', + '---', + '', + '# Project State', + '', + '**Status:** Ready to plan', + '', + '## Current Position', + '', + `Phase: 48 — ${PAREN_NAME_2736}`, + 'Plan: Not started', + '', + ].join('\n'); + } + + __d2736('#2736: syncStateFrontmatter authoritative override (unit)', () => { + __t2736('an intent-first override survives the prose round-trip verbatim', () => { + const out = __state2736.syncStateFrontmatter(postAdvanceState2736(), undefined, { + current_phase_name: PAREN_NAME_2736, + }); + const fm = extractFrontmatter(out); + __assert2736.strictEqual( + fm.current_phase_name, + PAREN_NAME_2736, + `authoritative current_phase_name must win over the prose re-derivation; got ${JSON.stringify(fm.current_phase_name)}`, + ); + }); + + __t2736('without an override, the dash name wins (secondary fix), but the paren aside is still dropped', () => { + // Documents the residual lossiness that makes the intent-first override + // necessary: for `N — Name (aside)` prose where the aside IS part of the + // name, no precedence can recover the full name from prose alone. + const out = __state2736.syncStateFrontmatter(postAdvanceState2736(), undefined); + const fm = extractFrontmatter(out); + __assert2736.strictEqual( + fm.current_phase_name, + 'Closer-ruling measurement', + `the paren-over-dash harvest ('D1a') must be gone; got ${JSON.stringify(fm.current_phase_name)}`, + ); + }); + + __t2736('an empty/blank override entry is ignored (no clearing an existing value)', () => { + const out = __state2736.syncStateFrontmatter(postAdvanceState2736(), undefined, { + current_phase_name: ' ', + }); + const fm = extractFrontmatter(out); + __assert2736.notStrictEqual(fm.current_phase_name, ' '); + }); + }); + + __d2736('#2736: phase complete preserves a paren-containing next-phase name (e2e)', () => { + let tmpDir; + __be2736(() => { tmpDir = __mk2736(); }); + __ae2736(() => { __rm2736(tmpDir); }); + + __t2736('current_phase_name lands as the exact roadmap display name, not its parenthetical', () => { + const planningDir = __path2736.join(tmpDir, '.planning'); + const phase1Dir = __path2736.join(planningDir, 'phases', '01-foundation'); + __fs2736.mkdirSync(phase1Dir, { recursive: true }); + + __fs2736.writeFileSync( + __path2736.join(planningDir, 'ROADMAP.md'), + [ + '# Roadmap', + '', + '- [ ] Phase 1: Foundation', + `- [ ] Phase 2: ${PAREN_NAME_2736}`, + '', + '### Phase 1: Foundation', + '**Goal:** Setup', + '**Plans:** 1 plans', + '', + `### Phase 2: ${PAREN_NAME_2736}`, + '**Goal:** Measure closer rulings', + '', + ].join('\n'), + ); + + // The in-the-wild STATE.md shape: frontmatter + ## Current Position with + // a `Phase:` line, and NO `Current Phase Name:` body field. + __fs2736.writeFileSync( + __path2736.join(planningDir, 'STATE.md'), + [ + '---', + 'gsd_state_version: 1.0', + 'current_phase: 1', + 'current_phase_name: Foundation', + 'status: executing', + '---', + '', + '# Project State', + '', + '## Current Position', + '', + 'Phase: 1 — Foundation', + 'Plan: 1 of 1', + 'Status: Executing Phase 1', + 'Last activity: 2026-07-01 — mid-flight', + '', + ].join('\n'), + ); + + __fs2736.writeFileSync(__path2736.join(phase1Dir, '01-01-PLAN.md'), '# Plan\n'); + __fs2736.writeFileSync(__path2736.join(phase1Dir, '01-01-SUMMARY.md'), '# Summary\n'); + __fs2736.writeFileSync( + __path2736.join(phase1Dir, '01-VERIFICATION.md'), + ['---', 'status: passed', '---', '', '# Verification', ''].join('\n'), + ); + + const result = __run2736(['phase', 'complete', '1'], tmpDir); + __assert2736.ok(result.success, `phase complete failed: ${result.error}`); + + const stateContent = __fs2736.readFileSync(__path2736.join(planningDir, 'STATE.md'), 'utf-8'); + const fm = extractFrontmatter(stateContent); + __assert2736.strictEqual( + fm.current_phase_name, + PAREN_NAME_2736, + `current_phase_name must be the exact next-phase display name; got ${JSON.stringify(fm.current_phase_name)} ` + + '(the prose round-trip harvested the parenthetical — #2736)', + ); + // The body prose the transition wrote stays as designed. + __assert2736.match(stateContent, /Phase: 2 — Closer-ruling measurement \(D1a\)/); + }); + }); + + __d2736('#2736 sibling: state begin-phase preserves a paren-containing name (e2e)', () => { + let tmpDir; + __be2736(() => { tmpDir = __mk2736(); }); + __ae2736(() => { __rm2736(tmpDir); }); + + function writeBeginFixture2736(lines) { + __fs2736.writeFileSync(__path2736.join(tmpDir, '.planning', 'STATE.md'), lines.join('\n')); + } + + __t2736('the intent-held name survives the begin-phase sync verbatim', () => { + writeBeginFixture2736([ + '---', + 'gsd_state_version: 1.0', + 'current_phase: 1', + 'current_phase_name: Foundation', + 'status: planning', + '---', + '', + '# Project State', + '', + '## Current Position', + '', + 'Phase: 1 — Foundation', + 'Plan: Not started', + 'Status: Ready to execute', + 'Last activity: 2026-07-01 — planned', + '', + ]); + + const result = __run2736( + ['state', 'begin-phase', '--phase', '2', '--name', PAREN_NAME_2736, '--plans', '1'], + tmpDir, + ); + __assert2736.ok(result.success, `state begin-phase failed: ${result.error}`); + + const fm = extractFrontmatter(__fs2736.readFileSync(__path2736.join(tmpDir, '.planning', 'STATE.md'), 'utf-8')); + __assert2736.strictEqual( + fm.current_phase_name, + PAREN_NAME_2736, + `current_phase_name must be the exact intent-held name; got ${JSON.stringify(fm.current_phase_name)} ` + + '(the `N (Name) — EXECUTING` round-trip truncates paren-containing names — #2736)', + ); + }); + + __t2736('the override outlives the #1695 preservation restore when no body Phase: line exists', () => { + // Cross-AI review finding (P4.6 round 1): with no `Phase:` body line the + // pre/post phase-source snapshots are both null (equal), so the #1695 + // restore fires after the sync and used to put the stale pre-transition + // name back over the authoritative one. The re-assert after + // applyStatePreservation is what this pins. + writeBeginFixture2736([ + '---', + 'gsd_state_version: 1.0', + 'current_phase: 1', + 'current_phase_name: Foundation', + 'status: planning', + '---', + '', + '# Project State', + '', + '**Current Phase:** 1', + '**Status:** Ready to execute', + '**Last Activity:** 2026-07-01', + '', + ]); + + const result = __run2736( + ['state', 'begin-phase', '--phase', '2', '--name', PAREN_NAME_2736, '--plans', '1'], + tmpDir, + ); + __assert2736.ok(result.success, `state begin-phase failed: ${result.error}`); + + const fm = extractFrontmatter(__fs2736.readFileSync(__path2736.join(tmpDir, '.planning', 'STATE.md'), 'utf-8')); + __assert2736.strictEqual( + fm.current_phase_name, + PAREN_NAME_2736, + `the intent-held name must outlive the preservation restore; got ${JSON.stringify(fm.current_phase_name)}`, + ); + }); + + __t2736('a #3127 resume does NOT override the preserved mid-flight name', () => { + // Cross-AI review finding (P4.6 round 2): on a resume (Status already + // `Executing Phase N`), beginPhaseCore deliberately preserves the + // mid-flight Current Phase Name — the adapter must drop the intent-first + // override so frontmatter tracks the preserved body value instead of the + // resume invocation's --name. + writeBeginFixture2736([ + '---', + 'gsd_state_version: 1.0', + 'current_phase: 2', + `current_phase_name: ${PAREN_NAME_2736}`, + 'status: executing', + '---', + '', + '# Project State', + '', + '## Current Position', + '', + `Phase: 2 — ${PAREN_NAME_2736}`, + 'Plan: 1 of 1', + 'Status: Executing Phase 2', + 'Last activity: 2026-07-01 — mid-flight', + '', + ]); + + const result = __run2736( + ['state', 'begin-phase', '--phase', '2', '--name', 'A Different Name', '--plans', '1'], + tmpDir, + ); + __assert2736.ok(result.success, `state begin-phase (resume) failed: ${result.error}`); + + const fm = extractFrontmatter(__fs2736.readFileSync(__path2736.join(tmpDir, '.planning', 'STATE.md'), 'utf-8')); + __assert2736.strictEqual( + fm.current_phase_name, + PAREN_NAME_2736, + `a resume must keep the preserved mid-flight name, not the resume's --name; got ${JSON.stringify(fm.current_phase_name)}`, + ); + }); + }); +} diff --git a/tests/phase-id.test.cjs b/tests/phase-id.test.cjs index b37c45e43..59a6fc715 100644 --- a/tests/phase-id.test.cjs +++ b/tests/phase-id.test.cjs @@ -538,12 +538,56 @@ describe('parsePhaseFromProse', () => { assert.equal(phaseId.parsePhaseFromProse('Phase 3A — Delta').phase, '3A'); }); - test('a status-word parenthetical is filtered from the name (preserved behavior)', () => { - // parenName wins over the em-dash tail; "executing" is a status word → name null. - assert.deepEqual(phaseId.parsePhaseFromProse('3A — Delta (executing)'), { phase: '3A', name: null }); + test('a status-word parenthetical is filtered from the name', () => { + // #2736 precedence change (the #1695 AC #3 residual): the em-dash name now + // wins when it is a genuine name, so `3A — Delta (executing)` yields + // 'Delta' (previously null — paren-priority harvested the status aside and + // the status filter nulled it, losing the real name). + assert.deepEqual(phaseId.parsePhaseFromProse('3A — Delta (executing)'), { phase: '3A', name: 'Delta' }); assert.equal(phaseId.parsePhaseFromProse('3 (complete)').name, null); }); + test('#2736: status-keyword-aware precedence across the first-party writer shapes', () => { + // completePhaseCore shape `N — Name (aside)`: the dash name wins; the + // name's own parenthetical is no longer harvested as the whole name. + assert.deepEqual( + phaseId.parsePhaseFromProse('48 — Closer-ruling measurement (D1a)'), + { phase: '48', name: 'Closer-ruling measurement' }, + ); + // beginPhaseCore shape `N (Name) — EXECUTING`: the dash tail is a status + // keyword, so the parenthetical name still wins. + assert.deepEqual( + phaseId.parsePhaseFromProse('16 (Native Global Hotkey) — EXECUTING'), + { phase: '16', name: 'Native Global Hotkey' }, + ); + // `N — COMPLETE` (state.cts phase-complete body line): status keyword on + // the dash, no paren → no name. + assert.deepEqual(phaseId.parsePhaseFromProse('5 — COMPLETE'), { phase: '5', name: null }); + // gsd2-import shape `N (slug) — Milestone: Title`: the dash tail is a + // milestone label, not a name → the parenthetical still wins. + assert.deepEqual( + phaseId.parsePhaseFromProse('06 (setup) — Milestone: Foundation'), + { phase: '06', name: 'setup' }, + ); + // Cross-AI review round 1: an em-dash INSIDE a parenthetical name must not + // be mistaken for the name separator (the dash search runs on a + // paren-stripped copy). + assert.deepEqual( + phaseId.parsePhaseFromProse('16 (Native — Global Hotkey) — EXECUTING'), + { phase: '16', name: 'Native — Global Hotkey' }, + ); + // Cross-AI review round 1: status-LIKE dash tails beyond the canonical + // three lose to a parenthetical name (broader precedence vocabulary + + // the lone-ALL-CAPS-token heuristic), without changing which extracted + // names are nulled. + assert.equal(phaseId.parsePhaseFromProse('3 (Foundation) — COMPLETED').name, 'Foundation'); + assert.equal(phaseId.parsePhaseFromProse('3 (Name) — In progress').name, 'Name'); + assert.equal(phaseId.parsePhaseFromProse('3 (Name) — READY').name, 'Name'); + assert.equal(phaseId.parsePhaseFromProse('3 (Name) — WIP').name, 'Name'); + // With no parenthetical to prefer, an unknown dash tail stays the best guess. + assert.equal(phaseId.parsePhaseFromProse('3 — WIP').name, 'WIP'); + }); + test('#2124 review: name quantifiers are length-bounded (ReDoS guard)', () => { // A parenthetical within the bound extracts; one longer than the bound is // NOT matched — the cap is what prevents O(n^2) backtracking on a crafted