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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
A phase whose slug's first word is a bare single digit (dir
`46-6-rs-pipeline-orchestrator`, roadmap phase "6 Rs Pipeline Orchestrator" →
slug `6-rs-…`) had its phase token over-collected as `46-6` instead of `46`, so
`gsd-tools` phase-by-number lookups (init.plan-phase, init.phase-op, and
downstream execute/verify/ship) resolved phase_dir=null / has_context=false.
Root cause: an over-broad "looks numeric" test (`/^\d/`, `\d+(?:-\d+)*`)
classified token segments and could not distinguish a legitimate zero-padded
sub-phase segment (`01`, `02`) from a single-digit slug word (`6`). Zero-padded
phase/sub-phase segments are always ≥2 digits, so requiring ≥2 digits is the
structural distinguisher. Applied consistently across every same-class
implementation the triage identified (fixing extractPhaseToken alone leaves the
health-check and milestone-filter subsystems exposed):
- src/phase-id.cts extractPhaseToken — a pure-numeric leading segment continues
only with ≥2-digit segments; a letter-prefixed milestone id (`M1`) still
admits its single-digit sub-phase (`M1-2`).
- src/validate.cts PHASE_TOKEN_FROM_DIR_RE (W005/W006/W007 health checks) and
canonicalPlanStem (I001 plan/summary pairing).
- src/roadmap-parser.cts isDirInMilestone numericRe (getMilestonePhaseFilter —
highest blast radius: a false negative silently excludes a phase from the
milestone).
- src/core-utils.cts + its verbatim duplicate src/phase.cts extractCanonicalPlanId.
Regression tests added across tests/{phase-id,health-validation,core-utils,
roadmap-parser}.test.cjs using the shared `46-6-rs-…` fixture, asserting the
token resolves to `46` (not `46-6`) while legit multi-segment tokens
(`01-02`, `02-03-04`, `M1-2`, `68-01`, `02-01`) are unchanged. The
roadmap-parser test exercises the real getMilestonePhaseFilter end-to-end.
The residual ≥2-digit-leading-slug case (e.g. `NN-2024-roadmap`) stays out of
scope — subsumed by the #612 / #565 phase-ID convention work, per the issue.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The convergence lint only scanned src/ + gsd-core/bin, so ~35 test files
still imported core.cjs. Repoint all 33 behaviour importers to the leaf
modules directly (same symbol->leaf map as the src migration; leaves are the
objects core re-exported by reference), delete the now-meaningless
shim-identity describe blocks in the 8 leaf tests, and delete tests/core.test.cjs
(forwarded-behaviour coverage now lives at the leaves; resolveWorktreeRoot
test relocated to worktree-safety in T0) and tests/lint-core-spine-imports.test.cjs
(the lint is removed in T-final). Dropped the stale core.test.cjs entries from
the allow-test-rule-refs allowlist; eslint-rules RuleTester fixture path
pointed at io.cjs.
After T6: ZERO test imports core.cjs. core.cts still builds (now fully unused);
T-final deletes it. No behaviour change.
Closes#1291
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
ADR-857 rollout phase 2a — the first cut of the core.cts decomposition.
Move the 9 pure phase-id parsing/matching helpers (escapeRegex,
normalizePhaseName, comparePhaseNum, extractPhaseToken, phaseTokenMatches,
phaseMarkdownRegexSource/Exact, getMilestoneFromPhaseId, getPhaseDirFromPhaseId)
out of core.cts into a new leaf module src/phase-id.cts. core.cts re-exports
them (behavior-preserving); its internal callers resolve the destructured
bindings.
Cycle-safe by design: phase-id depends on nothing in core, so re-export creates
no circular require (the property that made phase-1 io.cts clean). This is the
leaf-first ordering — it unblocks the roadmap-parser extraction (2b), which
imports phaseMarkdownRegexSource.
New-CLI-module checklist: .gitignore, eslint.config.mjs ignores, INVENTORY.md
count 91->92 + row, INVENTORY-MANIFEST.json, ARCHITECTURE.md row, CONTEXT.md
"Phase Id Module" glossary entry. Adds tests/phase-id.test.cjs (63 behavioral
tests incl. shim-identity + adversarial inputs).
Gates: lint, code-review, security-review, codex adversarial-review (all 0
findings), and gsd-test-both (14814 pass on Mac + Linux Docker, 0 fail).
Closes#865
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
