Skip to content

getMilestonePhaseFilter: greedy custom-ID regex excludes every letter-named phase directory, silently fabricating milestone counts #3213

Description

@bmv002

Summary

getMilestonePhaseFilter's custom-ID branch in src/roadmap-parser.cts uses a greedy capture that swallows the whole phase-directory name. For any project whose phase directories are letter-named (A-tool-output-contract), every such directory falls out of the milestone, and the milestone progress/plan counts are then computed over whatever numeric directory happens to survive.

The number is not an error — it is well-formed, plausible, and at a phase boundary sometimes even correct. That is what makes it hard to notice. In one project it recurred 26 times across eight phases before anyone checked it against the tree.

The line

src/roadmap-parser.cts:1035

const customMatch = dirName.match(/^([A-Za-z][A-Za-z0-9]*(?:-[A-Za-z0-9]+)*)/);
if (customMatch && normalized.has(customMatch[1].toLowerCase())) return true;

The regex exists to turn PROJ-42-description into PROJ-42. But (?:-[A-Za-z0-9]+)* keeps consuming hyphenated segments, so on A-tool-output-contract it captures A-tool-output-contract, not A. The set membership test then fails and the directory is excluded.

The numeric branch above it is unaffected, which is why this only shows up on letter/custom-ID phase IDs.

Reproduction

Measured against open-gsd/gsd-core @ next, by extracting the real bytes of isDirInMilestone + normalizePhaseIdSegments out of src/roadmap-parser.cts and stripProjectCodePrefix out of src/phase-id.cts and running a real 13-phase tree through them (ROADMAP.md declares Phase 00: and Phase A:Phase L:; .planning/phases/ holds the matching 13 directories):

  IN   00-inventory-approval-gate
  OUT  A-tool-output-contract
  OUT  B-evidence-artifact-contract
  OUT  C-attention-triage
  OUT  D-async-runner-hardening
  OUT  E-persona-driven-adversarial-e2e
  OUT  F-measurement-bench
  OUT  G-memory-skill-hygiene
  OUT  H-evidence-schema-enum-closure
  OUT  I-runner-fail-open-webhook-debt
  OUT  J-artifact-format-hygiene
  OUT  K-installed-runtime-fail-open-hotfix
  OUT  L-framework-distribution

RESULT (gsd-core HEAD): 1 of 13 phase dirs in-milestone
TRUE totals across all 13 dirs : 89/89
What the filter yields         : 5/5

5/5 is 00-inventory-approval-gate's own counts. The tool reports 5/5 = 100% for a tree holding 89 PLAN.md and 89 SUMMARY.md across 13 phase directories.

Minimal check, no harness needed:

> 'A-tool-output-contract'.match(/^([A-Za-z][A-Za-z0-9]*(?:-[A-Za-z0-9]+)*)/)[1]
'A-tool-output-contract'      // expected 'A'

Why letter-named phases are not an exotic input

They are GSD's own convention. Phase A:Phase L: headings in ROADMAP.md are produced by GSD's normal roadmap flow, and docs/adr/612-bracket-phase-id-convention.md treats non-numeric phase IDs as first-class. Any project that uses them gets silently wrong milestone progress.

Suggested fix

Replace the capture with a segment-boundary membership test: a directory belongs if its lowercased name equals a declared phase ID, or begins with that ID followed by -.

const lower = dirName.toLowerCase();
for (const id of idsLongestFirst) {
  if (lower === id || lower.startsWith(id + '-')) return true;
}

Two things worth preserving in whatever shape you pick:

  1. Try IDs longest-first, so a short ID cannot shadow a longer one (A must not win over A-1 on A-1-foo).
  2. Do not use a bare startsWithALPHA-x must not match phase A. That would trade this bug for a fail-open in the other direction, and it deserves its own test.

PROJ-42-descriptionPROJ-42 still matches, which is the case the original capture existed to serve.

Scope of this report — what I ran vs. what I only read

  • Reproduced by execution against next HEAD: the filter defect above.
  • Read, not executed: syncStateFrontmatter (src/state.cts:1868) rebuilds frontmatter from the parsed object returned by buildStateFrontmatter, and YAML comments are not part of that object, so full-line # comments in .planning/STATE.md's frontmatter are dropped on every write. I did not build an end-to-end repro for this one, so treat it as an observation rather than a confirmed report — happy to open it separately with a proper repro if useful.
  • Explicitly NOT reported: the /^(Progress:\s*).*/im pattern whose \s* spans a newline. That one is already fixed at HEAD (src/state.cts:790-791 now uses [ \t]* / [^\r\n]*). Noting it so this report isn't read as broader than it is.

Found while hardening a downstream project against exactly this class of defect — a tool asserting a result it did not compute. Happy to send a PR with the fix and tests if you'd like.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingconfirmed-bugVerified reproducible bug

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions