Merge pull request #2129 from open-gsd/chore/2124-phase-id-canonical-surface

chore(#2124): build canonical phase-id.cts surface (Phase 1 of #2121)
2026-07-09 17:42:00 -04:00
parent d8782f772a 80923244b1
commit 960c3eaa15
4 changed files with 271 additions and 21 deletions
--- a/src/phase-id.cts
+++ b/src/phase-id.cts
@@ -255,6 +255,97 @@ 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 };
+  // 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()
+    : null;
+  return {
+    phase: phaseMatch ? phaseMatch[1] : null,
+    name,
+  };
+}
+
+/**
+ * Config-AWARE project-code prefix strip. Unlike the config-blind
+ * `stripProjectCodePrefix` (which strips ANY `<CODE>-` shape), this strips the
+ * leading `<CODE>-` ONLY when `<CODE>` 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 +359,8 @@ export = {
  comparePhaseNum,
  extractPhaseToken,
  phaseTokenMatches,
+  parsePhaseFromProse,
+  stripConfiguredProjectCodePrefix,
+  isForeignPrefixedPhaseQuery,
+  roadmapPhaseLookupSources,
 };
--- 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);
--- 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()
--- 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,169 @@ 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);
+  });
+
+  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) ───────────
+
+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))
+        );
+      }),
+    );
+  });
+});