From bf25e0b4358b5867442786267e6b0d48d5c1c0a5 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 9 Jul 2026 16:13:06 -0400 Subject: [PATCH 1/2] chore(#2124): build canonical phase-id.cts surface (Phase 1 of #2121) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the ADR-2121-locked canonical functions to src/phase-id.cts. No consumer behavior changes — Phases 2-4 migrate the divergent call sites against them. - parsePhaseFromProse: anchored prose parser. A phase is returned only when the STATE.md "Phase:" field VALUE begins with a phase token, so "Milestone v0.5 complete" yields { phase: null } instead of "5" (the #2111 root cause: the old unanchored \b(\d+..)\b mined the minor-version digit). Name extraction (parenthetical / em-dash tail, minus status words) unchanged. - stripConfiguredProjectCodePrefix / isForeignPrefixedPhaseQuery: config-aware prefix policy. A foreign prefix (MEM-01 when the configured code is LKML) is preserved rather than collapsed to a bare numeric phase — the #2104 fix's canonical home (consumed later, outside this epic's critical path). - roadmapPhaseLookupSources: moved from roadmap-parser.cts so phase-id.cts is the single owner of the exact -> numeric -> prefix-tolerant ordering. roadmap-parser.cts now imports it (behavior-identical); its two now-unused imports (phaseMarkdownRegexSourceExact, OPTIONAL_PROJECT_CODE_PREFIX_SOURCE) are dropped. Tests: subject-named suites in tests/phase-id.test.cjs covering the ADR boundary set (v0.5, v1.0, MEM-01, AB-29, bare 29, zero-padded 029) plus two fast-check properties: the #2111 "Milestone vX.Y complete never yields a phase" invariant and a parse/normalize property. Extend-never-mutate: the 12 pre-existing phase-id.cts exports are unchanged. Closes #2124 Refs #2121 Co-Authored-By: Claude Opus 4.8 --- src/phase-id.cts | 88 +++++++++++++++++++++++ src/roadmap-parser.cts | 22 +----- tests/phase-id.test.cjs | 151 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 242 insertions(+), 19 deletions(-) diff --git a/src/phase-id.cts b/src/phase-id.cts index 07d0799e6..68d6b4015 100644 --- a/src/phase-id.cts +++ b/src/phase-id.cts @@ -255,6 +255,90 @@ function phaseTokenMatches(dirName: string, normalized: string): boolean { return false; } +// ─── #2121 canonical surface (ADR-2121) ────────────────────────────────────── + +/** + * Parse a phase identifier from a STATE.md `Phase:` prose field VALUE — the text + * after the `Phase:` label (e.g. `"3 of 4 (Delta)"`, `"3A — Delta (executing)"`, + * or `"Milestone v0.5 complete"`). + * + * The token is anchored to the START of the value (after an optional literal + * `Phase ` label and an optional project-code prefix) so a phase is only + * returned when the value actually begins with one. This is the #2111 fix: the + * prior unanchored `/\b(\d+[A-Z]?(?:\.\d+)*)\b/i` mined the first numeral + * anywhere, so `"Milestone v0.5 complete"` collapsed to `"5"` (the minor-version + * digit) and `"v1.0"` to `"0"` (a reserved sentinel). Here both yield + * `{ phase: null }` because they do not begin with a phase token. The name + * extraction (parenthetical or em-dash tail, minus status words) is unchanged. + */ +function parsePhaseFromProse(value: string | null): { phase: string | null; name: string | null } { + if (!value) return { phase: null, name: null }; + const phaseMatch = value.match(/^\s*(?:Phase\s+)?(?:[A-Z][A-Z0-9_]*-)?(\d+[A-Z]?(?:\.\d+)*)\b/i); + const parenName = value.match(/\(([^)]+)\)/); + const dashName = value.match(/—\s*([^(\n]+?)(?:\s*\(|$)/); + const rawName = parenName?.[1] ?? dashName?.[1] ?? null; + const name = rawName && !/^(?:complete|executing|not started)$/i.test(rawName.trim()) + ? rawName.trim() + : null; + return { + phase: phaseMatch ? phaseMatch[1] : null, + name, + }; +} + +/** + * Config-AWARE project-code prefix strip. Unlike the config-blind + * `stripProjectCodePrefix` (which strips ANY `-` shape), this strips the + * leading `-` ONLY when `` case-insensitively equals the configured + * `projectCode`. A foreign prefix (`MEM-01` when the configured code is `LKML`) + * or an absent/empty `projectCode` is preserved verbatim — this is the #2104 + * fix: a foreign-prefixed id must not collapse to a bare numeric phase and + * collide with a real one. + */ +function stripConfiguredProjectCodePrefix(value: unknown, projectCode: string | null | undefined): string { + const input = String(value); + const configured = typeof projectCode === 'string' ? projectCode.trim() : ''; + if (!configured) return input; + const m = input.match(PROJECT_CODE_PREFIX_CAPTURE_RE_I); + if (!m) return input; + if (m[1].toUpperCase() !== configured.toUpperCase()) return input; + return m[2]; +} + +/** + * True when `phase` carries a project-code prefix that is NOT the configured + * `projectCode` (or when no `projectCode` is configured). The canonical + * predicate the init-command foreign-prefix guard (#2056 / PR #2105) delegates + * to, so every call site shares one foreign-prefix rule. + */ +function isForeignPrefixedPhaseQuery(phase: unknown, projectCode: unknown): boolean { + const m = String(phase).match(PROJECT_CODE_PREFIX_CAPTURE_RE_I); + if (!m) return false; + const configured = typeof projectCode === 'string' ? projectCode.trim() : ''; + return !configured || m[1].toUpperCase() !== configured.toUpperCase(); +} + +/** + * Canonical ROADMAP heading lookup-source list (moved here from + * roadmap-parser.cts so phase-id.cts is the single owner of the ordering). + * Sources are tried in a fixed, deduplicated order: exact (only when the query + * itself is project-code-prefixed) → bare numeric / padding-tolerant → + * prefix-tolerant fallback. The bare numeric source precedes the prefix-tolerant + * form so a canonical heading (`### Phase 117:`) is preferred over a drifted + * prefixed one (`### Phase MANIFOLD-117:`) when both exist in one ROADMAP. + */ +function roadmapPhaseLookupSources(phaseNum: unknown): string[] { + const sources: string[] = []; + const exactSource = phaseMarkdownRegexSourceExact(phaseNum); + if (exactSource) sources.push(exactSource); + + const numericSource = phaseMarkdownRegexSource(phaseNum); + sources.push(numericSource); + sources.push(`${OPTIONAL_PROJECT_CODE_PREFIX_SOURCE}${numericSource}`); + + return [...new Set(sources)]; +} + export = { escapeRegex, OPTIONAL_PROJECT_CODE_PREFIX_SOURCE, @@ -268,4 +352,8 @@ export = { comparePhaseNum, extractPhaseToken, phaseTokenMatches, + parsePhaseFromProse, + stripConfiguredProjectCodePrefix, + isForeignPrefixedPhaseQuery, + roadmapPhaseLookupSources, }; diff --git a/src/roadmap-parser.cts b/src/roadmap-parser.cts index 1dd7c1c99..f2ab3f648 100644 --- a/src/roadmap-parser.cts +++ b/src/roadmap-parser.cts @@ -22,10 +22,11 @@ import phaseIdModule = require('./phase-id.cjs'); const { escapeRegex, phaseMarkdownRegexSource, - phaseMarkdownRegexSourceExact, stripProjectCodePrefix, - OPTIONAL_PROJECT_CODE_PREFIX_SOURCE, OPTIONAL_PHASE_TAG_SOURCE, + // #2121: roadmapPhaseLookupSources now lives in phase-id.cjs (single owner of + // the lookup-source ordering); imported here rather than defined locally. + roadmapPhaseLookupSources, } = phaseIdModule; // eslint-disable-next-line @typescript-eslint/no-require-imports import planningWorkspace = require('./planning-workspace.cjs'); @@ -242,23 +243,6 @@ function findRoadmapPhaseInContent(content: string, phaseNum: unknown, phaseSour }; } -function roadmapPhaseLookupSources(phaseNum: unknown): string[] { - const sources: string[] = []; - const exactSource = phaseMarkdownRegexSourceExact(phaseNum); - if (exactSource) sources.push(exactSource); - - const numericSource = phaseMarkdownRegexSource(phaseNum); - // Source order matters: the bare numeric source is tried before the - // prefix-tolerant form so that a canonical bare heading ("Phase 117:") is - // preferred over a drifted prefixed heading ("Phase MANIFOLD-117:") when - // both exist in the same ROADMAP. The prefix-tolerant form is the fallback - // that handles the drifted-only case. - sources.push(numericSource); - sources.push(`${OPTIONAL_PROJECT_CODE_PREFIX_SOURCE}${numericSource}`); - - return [...new Set(sources)]; -} - function getRoadmapPhaseInternal(cwd: string, phaseNum: unknown): RoadmapPhaseResult | null { if (!phaseNum) return null; const normalizedPhase = stripProjectCodePrefix(phaseNum); diff --git a/tests/phase-id.test.cjs b/tests/phase-id.test.cjs index 94746235b..e56f1d56c 100644 --- a/tests/phase-id.test.cjs +++ b/tests/phase-id.test.cjs @@ -22,6 +22,7 @@ const { test, describe } = require('node:test'); const assert = require('node:assert/strict'); const phaseId = require('../gsd-core/bin/lib/phase-id.cjs'); +const fc = require('fast-check'); // ─── escapeRegex ───────────────────────────────────────────────────────────── @@ -450,3 +451,153 @@ describe('getPhaseDirFromPhaseId', () => { assert.ok(!result.endsWith('-')); }); }); + +// ─── parsePhaseFromProse (#2121, anchored — fixes #2111) ───────────────────── + +describe('parsePhaseFromProse', () => { + test('null / empty input yields null phase and name', () => { + assert.deepEqual(phaseId.parsePhaseFromProse(null), { phase: null, name: null }); + assert.deepEqual(phaseId.parsePhaseFromProse(''), { phase: null, name: null }); + }); + + test('#2111: a milestone-completion string carries no phase', () => { + assert.equal(phaseId.parsePhaseFromProse('Milestone v0.5 complete').phase, null); + assert.equal(phaseId.parsePhaseFromProse('Milestone v1.0 complete').phase, null); + assert.equal(phaseId.parsePhaseFromProse('Milestone v2.10 complete').phase, null); + }); + + test('#2111: a bare version token or stray numeral is not a phase', () => { + assert.equal(phaseId.parsePhaseFromProse('v0.5').phase, null); + assert.equal(phaseId.parsePhaseFromProse('v1.0').phase, null); + assert.equal(phaseId.parsePhaseFromProse('Fixed 12 bugs in v2.3').phase, null); + }); + + test('a genuine phase value (starting with the token) is parsed', () => { + assert.deepEqual(phaseId.parsePhaseFromProse('3 of 4 (Delta)'), { phase: '3', name: 'Delta' }); + assert.deepEqual(phaseId.parsePhaseFromProse('3A — Delta'), { phase: '3A', name: 'Delta' }); + assert.equal(phaseId.parsePhaseFromProse('12.1: Setup').phase, '12.1'); + assert.equal(phaseId.parsePhaseFromProse('29 of 30').phase, '29'); + assert.equal(phaseId.parsePhaseFromProse('029').phase, '029'); + }); + + test('a leading project-code prefix is tolerated but not captured (bare token)', () => { + assert.equal(phaseId.parsePhaseFromProse('MEM-01 — Foo').phase, '01'); + assert.equal(phaseId.parsePhaseFromProse('AB-29 of 30').phase, '29'); + }); + + test('an optional leading "Phase" label is tolerated', () => { + 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 }); + assert.equal(phaseId.parsePhaseFromProse('3 (complete)').name, null); + }); +}); + +// ─── stripConfiguredProjectCodePrefix (#2121 / #2104, config-aware) ─────────── + +describe('stripConfiguredProjectCodePrefix', () => { + test('#2104: a foreign prefix is preserved (not collapsed to a bare phase)', () => { + assert.equal(phaseId.stripConfiguredProjectCodePrefix('MEM-01', 'LKML'), 'MEM-01'); + }); + + test('the configured prefix is stripped (case-insensitive)', () => { + assert.equal(phaseId.stripConfiguredProjectCodePrefix('CK-01', 'CK'), '01'); + assert.equal(phaseId.stripConfiguredProjectCodePrefix('LKML-29', 'lkml'), '29'); + assert.equal(phaseId.stripConfiguredProjectCodePrefix('AB-29', 'AB'), '29'); + }); + + test('a value with no prefix is returned unchanged', () => { + assert.equal(phaseId.stripConfiguredProjectCodePrefix('01', 'CK'), '01'); + assert.equal(phaseId.stripConfiguredProjectCodePrefix('029', 'CK'), '029'); + }); + + test('an absent/empty projectCode preserves the value verbatim', () => { + assert.equal(phaseId.stripConfiguredProjectCodePrefix('MEM-01', ''), 'MEM-01'); + assert.equal(phaseId.stripConfiguredProjectCodePrefix('MEM-01', null), 'MEM-01'); + assert.equal(phaseId.stripConfiguredProjectCodePrefix('MEM-01', undefined), 'MEM-01'); + }); +}); + +// ─── isForeignPrefixedPhaseQuery (#2121 / #2056) ───────────────────────────── + +describe('isForeignPrefixedPhaseQuery', () => { + test('a prefix that is not the configured code is foreign', () => { + assert.equal(phaseId.isForeignPrefixedPhaseQuery('MEM-01', 'LKML'), true); + }); + + test('the configured prefix is not foreign (case-insensitive)', () => { + assert.equal(phaseId.isForeignPrefixedPhaseQuery('CK-01', 'CK'), false); + assert.equal(phaseId.isForeignPrefixedPhaseQuery('ck-01', 'CK'), false); + }); + + test('a value with no prefix is never foreign', () => { + assert.equal(phaseId.isForeignPrefixedPhaseQuery('01', 'CK'), false); + assert.equal(phaseId.isForeignPrefixedPhaseQuery('29', 'AB'), false); + }); + + test('a prefixed query with no configured code is foreign; a bare one is not', () => { + assert.equal(phaseId.isForeignPrefixedPhaseQuery('MEM-01', ''), true); + assert.equal(phaseId.isForeignPrefixedPhaseQuery('MEM-01', null), true); + assert.equal(phaseId.isForeignPrefixedPhaseQuery('01', ''), false); + }); +}); + +// ─── roadmapPhaseLookupSources (#2121, owned here after the move) ───────────── + +describe('roadmapPhaseLookupSources', () => { + const PREFIX_TOLERANT = `${phaseId.OPTIONAL_PROJECT_CODE_PREFIX_SOURCE}0*29`; + + test('a bare numeric query yields the numeric then prefix-tolerant sources', () => { + const sources = phaseId.roadmapPhaseLookupSources('29'); + assert.deepEqual(sources, ['0*29', PREFIX_TOLERANT]); + }); + + test('the bare numeric source precedes the prefix-tolerant fallback', () => { + const sources = phaseId.roadmapPhaseLookupSources('29'); + assert.ok(sources.indexOf('0*29') < sources.indexOf(PREFIX_TOLERANT)); + }); + + test('a project-code-prefixed query adds the exact source first (3 sources)', () => { + const sources = phaseId.roadmapPhaseLookupSources('AB-29'); + assert.equal(sources.length, 3); + assert.equal(sources[0], 'AB-29'); + assert.ok(sources.includes('0*29')); + assert.ok(sources.includes(PREFIX_TOLERANT)); + }); + + test('zero-padding is tolerated: 029 resolves the same sources as 29', () => { + assert.deepEqual(phaseId.roadmapPhaseLookupSources('029'), phaseId.roadmapPhaseLookupSources('29')); + }); + + test('sources are deduplicated', () => { + const sources = phaseId.roadmapPhaseLookupSources('29'); + assert.equal(sources.length, new Set(sources).size); + }); +}); + +// ─── #2121 property tests (fast-check) ─────────────────────────────────────── + +describe('phase-id canonical surface — properties', () => { + test('#2111 invariant: a "Milestone vX.Y complete" string never yields a phase', () => { + fc.assert( + fc.property(fc.nat(999), fc.nat(999), (major, minor) => { + return phaseId.parsePhaseFromProse(`Milestone v${major}.${minor} complete`).phase === null; + }), + ); + }); + + test('parse↔normalize: a "N of M" prose value extracts N, and it normalizes stably', () => { + fc.assert( + fc.property(fc.integer({ min: 1, max: 9999 }), fc.integer({ min: 1, max: 9999 }), (n, m) => { + const parsed = phaseId.parsePhaseFromProse(`${n} of ${m}`); + return ( + parsed.phase === String(n) && + phaseId.normalizePhaseName(parsed.phase) === phaseId.normalizePhaseName(String(n)) + ); + }), + ); + }); +}); From 80923244b1a93993c33db668ed2caa2e3b7471c1 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 9 Jul 2026 16:33:39 -0400 Subject: [PATCH 2/2] fix(#2124): harden parsePhaseFromProse per orthogonal review (ReDoS + coercion) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Orthogonal security review of the Phase 1 surface found two issues; both fixed and regression-tested: - MEDIUM ReDoS: the name-extraction regexes /\(([^)]+)\)/ and /—\s*([^(\n]+?).../ backtrack O(n^2) on a crafted STATE.md field value with a long unterminated "(" / "—" run (reviewer measured ~38s at 320k chars). Length-bound both quantifiers to {1,200} -> linear (320k now ~100ms). A real phase name is far shorter than the cap. - LOW: parsePhaseFromProse threw on non-string truthy input, unlike its three sibling #2121 functions. Coerce via String(value) up front. The identical ReDoS regexes are copied verbatim from the pre-existing state.cts:parseProsePhaseField; per the no-defer rule that surfaced defect is fixed inline there too (Phase 2 / #2125 later supersedes that function by delegating to the bounded phase-id.cts parser). Adds a behavioral bound-guard regression test (a >200-char parenthetical is not extracted) and a non-string-coercion test. Refs #2124, #2121 Co-Authored-By: Claude Opus 4.8 --- src/phase-id.cts | 13 ++++++++++--- src/state.cts | 8 ++++++-- tests/phase-id.test.cjs | 16 ++++++++++++++++ 3 files changed, 32 insertions(+), 5 deletions(-) diff --git a/src/phase-id.cts b/src/phase-id.cts index 68d6b4015..e08a4af43 100644 --- a/src/phase-id.cts +++ b/src/phase-id.cts @@ -273,9 +273,16 @@ function phaseTokenMatches(dirName: string, normalized: string): boolean { */ function parsePhaseFromProse(value: string | null): { phase: string | null; name: string | null } { if (!value) return { phase: null, name: null }; - const phaseMatch = value.match(/^\s*(?:Phase\s+)?(?:[A-Z][A-Z0-9_]*-)?(\d+[A-Z]?(?:\.\d+)*)\b/i); - const parenName = value.match(/\(([^)]+)\)/); - const dashName = value.match(/—\s*([^(\n]+?)(?:\s*\(|$)/); + // Coerce defensively so a non-string caller cannot throw on this canonical + // surface (mirrors the sibling #2121 functions' String(...) handling). + const str = String(value); + const phaseMatch = str.match(/^\s*(?:Phase\s+)?(?:[A-Z][A-Z0-9_]*-)?(\d+[A-Z]?(?:\.\d+)*)\b/i); + // The name-extraction quantifiers are length-bounded so a crafted long + // unterminated run (many `(` or `—`) in an untrusted STATE.md field value + // 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()) ? rawName.trim() diff --git a/src/state.cts b/src/state.cts index 02cfc8b59..0e8199266 100644 --- a/src/state.cts +++ b/src/state.cts @@ -1118,8 +1118,12 @@ function matchSessionSection(body: string): RegExpMatchArray | null { function parseProsePhaseField(value: string | null): { phase: string | null; name: string | null } { if (!value) return { phase: null, name: null }; const phaseMatch = value.match(/\b(\d+[A-Z]?(?:\.\d+)*)\b/i); - const parenName = value.match(/\(([^)]+)\)/); - const dashName = value.match(/—\s*([^(\n]+?)(?:\s*\(|$)/); + // #2124 review: length-bound the name quantifiers so a crafted long + // unterminated `(` / `—` run in an untrusted STATE.md field cannot drive + // O(n^2) backtracking (CPU DoS). (Phase 2 / #2125 supersedes this function + // by delegating to phase-id.cts:parsePhaseFromProse, which is bounded too.) + const parenName = value.match(/\(([^)]{1,200})\)/); + const dashName = value.match(/—\s*([^(\n]{1,200}?)(?:\s*\(|$)/); const rawName = parenName?.[1] ?? dashName?.[1] ?? null; const name = rawName && !/^(?:complete|executing|not started)$/i.test(rawName.trim()) ? rawName.trim() diff --git a/tests/phase-id.test.cjs b/tests/phase-id.test.cjs index e56f1d56c..286cf7a0c 100644 --- a/tests/phase-id.test.cjs +++ b/tests/phase-id.test.cjs @@ -494,6 +494,22 @@ describe('parsePhaseFromProse', () => { assert.deepEqual(phaseId.parsePhaseFromProse('3A — Delta (executing)'), { phase: '3A', name: null }); assert.equal(phaseId.parsePhaseFromProse('3 (complete)').name, null); }); + + 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 + // untrusted value. Removing the bound would extract the long name → fail. + assert.equal(phaseId.parsePhaseFromProse('3 (Delta)').name, 'Delta'); + assert.equal(phaseId.parsePhaseFromProse(`3 (${'x'.repeat(201)})`).name, null); + // A long unterminated "(" run yields no name and still parses the phase. + assert.deepEqual(phaseId.parsePhaseFromProse(`3 ${'('.repeat(5000)}`), { phase: '3', name: null }); + }); + + test('#2124 review: non-string input is coerced, never throws', () => { + assert.doesNotThrow(() => phaseId.parsePhaseFromProse(3)); + assert.equal(phaseId.parsePhaseFromProse(3).phase, '3'); + assert.deepEqual(phaseId.parsePhaseFromProse(true), { phase: null, name: null }); + }); }); // ─── stripConfiguredProjectCodePrefix (#2121 / #2104, config-aware) ───────────