golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

chore(#3235): hoist the preamble-strip conditional out of the replace operand (#3236)

Browse Source 
* chore(#3235): hoist the preamble-strip conditional out of the replace operand

CodeQL js/identity-replacement (alert 53, medium, CWE-116) fired on
src/roadmap-parser.cts:637. The #2947 fix (2bc53baa0) made the preamble
phase-detail strip conditional by swapping the REGEX OPERAND rather than the
operation, using `/$/` as a "match nothing" sentinel:

  .replace(currentSectionHasPhaseDetails ? /^#{2,4}\s*Phase\s+.../gim : /$/, '')

`str.replace(/$/, '')` substitutes the zero-width end-of-input match with the
empty string, so the branch is inert. Behavior was correct; the hazard is that
both branches shared ONE replacement argument, so a later change of `''` to a
non-empty string would silently give the no-op branch a real effect -- on a seam
whose blast radius is CRITICAL (200+ affected symbols, 45 files, 22 processes).

Hoist the conditional around the .replace() call instead. The do-not-strip
branch now performs no replacement at all. The `Phase Details` heading strip
stays UNCONDITIONAL in both branches, which is what #730 depends on.

Equivalence is not asserted, it is measured: a deterministic-seed differential
probe compared the old and new formulations over 320 generated inputs x both
branch values -- 640 comparisons, 0 mismatches. Corpus covered empty string,
CRLF, heading depths 1-5, the #1729 pre-colon tag form, decimal phase IDs,
`## Phase Details`, fenced blocks and horizontal rules.

Six characterization tests pin both branches, including the one case no existing
#2947 fixture covers: that the unconditional `Phase Details` strip still runs on
the do-not-strip branch. Boundary rows cover the strip regex's #{2,4} bounds at
limit-1 (h1), limit (h2/h4) and limit+1 (h5).

Refs #3235

* test(#3235): add parser property test; kill two weak regression tests

Review findings from the orthogonal passes, all fixed inline.

Standards axis (hard violation, CLAUDE.md -> TEST RULES & CONVENTIONS):
"Parsers, budget limits, and bijective contracts must include at least one
fast-check (fc) property test." roadmap-parser is a parser module and the six
new tests were all example-based. Adds one property test over generated
preambles, asserting four invariants across BOTH branches:
  1. no `Phase Details` heading ever survives (the unconditional strip)
  2. headings outside the strip regex's #{2,4} bound always survive
  3. hasOwnDetails=true  -> preamble phase headings are gone
  4. hasOwnDetails=false -> preamble phase headings are all retained
Generator is document-shaped -- a fixed alphabet of literal line shapes, NOT
derived from the parser's own regexes (CONTRIBUTING.md #2371 fixture
provenance: a writer-seeded generator can only confirm what the author already
believed). Seed pinned to 20260809, numRuns bounded at 300, verbose replay on
failure, per the determinism rule.

Isolated adversarial review ran mutation testing against the compiled lib and
found two weak tests:

- CRLF test killed no mutant the LF-only sibling did not already kill. Its
  fixture now carries a `## Phase Details` heading, which exercises the
  `[^\n]*` / `\n?` tail of the Phase Details regex under CRLF -- a region no
  LF-only fixture can reach -- and asserts no orphaned CR is left behind.
- "no preamble does not throw" caught none of five targeted mutants.
  `.replace()` cannot throw on any string, so no mutation confined to this
  change could fail it. Deleted rather than propped up: the empty-preamble case
  is now genuinely covered by the property test's minLength:0 generator, which
  produces empty preambles on both branches. Strictly subsumed.

Refs #3235

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-08-09 10:32:46 -04:00
committed by GitHub
parent 2a73f53cb3
commit 70e50eb1b2
2 changed files with 249 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
src/roadmap-parser.cts
View File

@@ -632,10 +632,18 @@ function extractCurrentMilestoneScoped(content: string, cwd?: string, ws?: strin
// the selected milestone section actually contains its own — otherwise the
// preamble phases ARE this milestone's phases and must be preserved.
const currentSectionHasPhaseDetails = /^#{2,4}\s*Phase\s+\S/im.test(currentSection);
const preamble = stripTaggedBlocks(beforeMilestones, 'details')
// #1729: `(?:\s*\([^)\n]{0,200}\))?` tolerates a pre-colon ( ) tag (literal mirror of OPTIONAL_PHASE_TAG_SOURCE).
.replace(currentSectionHasPhaseDetails ? /^#{2,4}\s*Phase\s+[\w][\w.-]*(?:\s*\([^)\n]{0,200}\))?\s*:[^\n]*(?:\n(?!#{1,6}\s)[^\n]*)*\n?/gim : /$/, '')
.replace(/^#{1,4}\s*Phase Details\b[^\n]*\n?/gim, '');
const preambleBase = stripTaggedBlocks(beforeMilestones, 'details');
// #3235: the conditional wraps the REPLACE, not the pattern. This used to select between the
// strip regex and a `/$/` sentinel, which made the do-not-strip branch an identity replacement
// (CodeQL js/identity-replacement, alert 53) -- correct, but it left both branches sharing one
// replacement argument, so changing `''` would silently give the no-op branch a real effect.
// #1729: `(?:\s*\([^)\n]{0,200}\))?` tolerates a pre-colon ( ) tag (literal mirror of OPTIONAL_PHASE_TAG_SOURCE).
const preambleWithoutPhaseDetails = currentSectionHasPhaseDetails
? preambleBase.replace(/^#{2,4}\s*Phase\s+[\w][\w.-]*(?:\s*\([^)\n]{0,200}\))?\s*:[^\n]*(?:\n(?!#{1,6}\s)[^\n]*)*\n?/gim, '')
: preambleBase;
// Unconditional in BOTH branches -- the #730 `Phase Details` heading strip is independent of
// whether the selected milestone section carries phase details of its own.
const preamble = preambleWithoutPhaseDetails.replace(/^#{1,4}\s*Phase Details\b[^\n]*\n?/gim, '');
const value = detailsSection
? preamble + currentSection + '\n' + detailsSection

237
tests/roadmap-parser.test.cjs
View File

@@ -21,6 +21,7 @@ const { describe, test, beforeEach, afterEach } = require('node:test');
const assert = require('node:assert/strict');
const fs = require('node:fs');
const path = require('node:path');
const fc = require('fast-check');
const roadmapParser = require('../gsd-core/bin/lib/roadmap-parser.cjs');
const { SCOPE } = require('../gsd-core/bin/lib/planning-scope.cjs');
@@ -358,6 +359,242 @@ describe('roadmap-parser: extractCurrentMilestone', () => {
const payload = JSON.parse(result.output);
assert.strictEqual(payload.phase_count, 2, `expected phase_count 2, got ${payload.phase_count} (phases dropped — #2947)`);
});
// ─── #3235: the preamble strip's conditional wraps the REPLACE, not the pattern.
// The previous form selected between the strip regex and a `/$/` sentinel, making
// the do-not-strip branch an identity replacement (CodeQL js/identity-replacement,
// alert 53). These pin BOTH branches so the restructure cannot move behavior. ──────
test('#3235 — Phase Details heading is stripped even when preamble phase details are preserved', () => {
// The do-not-strip branch must leave `### Phase N:` blocks alone WITHOUT also
// disabling the unconditional `Phase Details` heading strip. Pulling that second
// replace inside the conditional would regress #730 invisibly: no existing #2947
// fixture carries a `Phase Details` heading, so the suite would stay green.
writeState(tmpDir, { milestone: 'v9.0' });
const content = [
'# ROADMAP',
'',
'## Milestones',
'',
'- 🚧 **v9.0 Test Milestone** — Phases 1-2 (in progress)',
'',
'## Phase Details',
'',
'## Phases',
'',
'### Phase 1: Alpha',
'',
'**Goal:** do alpha',
'',
'### Phase 2: Beta',
'',
'**Goal:** do beta',
'',
'## Progress',
'',
'### v9.0 phase progress',
'',
'| Phase | Status |',
].join('\n');
writeRoadmap(tmpDir, content);
const roadmap = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8');
const result = extractCurrentMilestone(roadmap, tmpDir);
assert.ok(result.includes('Phase 1: Alpha'), 'do-not-strip branch preserves preamble phase details');
assert.ok(result.includes('Phase 2: Beta'), 'do-not-strip branch preserves every preamble phase detail');
assert.ok(!result.includes('## Phase Details'), 'the Phase Details heading strip is unconditional and must still run');
});
test('#3235 — preamble phase details are still stripped when the milestone section has its own', () => {
writeState(tmpDir, { milestone: 'v9.0' });
const content = [
'# ROADMAP',
'',
'## Preamble',
'',
'### Phase 7: PreambleGhost',
'',
'**Goal:** should be stripped',
'',
'## 🚧 v9.0 Current',
'',
'### Phase 1: Alpha',
'',
'**Goal:** do alpha',
'',
].join('\n');
writeRoadmap(tmpDir, content);
const roadmap = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8');
const result = extractCurrentMilestone(roadmap, tmpDir);
assert.ok(result.includes('Phase 1: Alpha'), 'selected milestone phases retained');
assert.ok(!result.includes('PreambleGhost'), 'preamble phase-detail heading stripped on the strip branch');
assert.ok(!result.includes('should be stripped'), 'the stripped heading takes its body with it');
assert.ok(result.includes('## Preamble'), 'a non-Phase preamble heading is untouched');
});
test('#3235 — preamble strip honors the #{2,4} heading-depth bounds', () => {
// Boundary coverage: limit-1 (h1) and limit+1 (h5) survive; h2 and h4 are stripped.
writeState(tmpDir, { milestone: 'v9.0' });
const content = [
'# ROADMAP',
'',
'# Phase 90: DepthOne',
'',
'## Phase 91: DepthTwo',
'',
'#### Phase 93: DepthFour',
'',
'##### Phase 94: DepthFive',
'',
'## 🚧 v9.0 Current',
'',
'### Phase 1: Alpha',
'',
'**Goal:** do alpha',
'',
].join('\n');
writeRoadmap(tmpDir, content);
const roadmap = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8');
const result = extractCurrentMilestone(roadmap, tmpDir);
assert.ok(result.includes('DepthOne'), 'h1 is below the #{2,4} floor and survives');
assert.ok(!result.includes('DepthTwo'), 'h2 is at the floor and is stripped');
assert.ok(!result.includes('DepthFour'), 'h4 is at the ceiling and is stripped');
assert.ok(result.includes('DepthFive'), 'h5 is above the #{2,4} ceiling and survives');
});
test('#3235 — #1729 pre-colon tag tolerance survives in the hoisted strip', () => {
writeState(tmpDir, { milestone: 'v9.0' });
const content = [
'# ROADMAP',
'',
'## Preamble',
'',
'### Phase 8 (deferred): TaggedGhost',
'',
'**Goal:** should be stripped',
'',
'## 🚧 v9.0 Current',
'',
'### Phase 1: Alpha',
'',
'**Goal:** do alpha',
'',
].join('\n');
writeRoadmap(tmpDir, content);
const roadmap = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8');
const result = extractCurrentMilestone(roadmap, tmpDir);
assert.ok(!result.includes('TaggedGhost'), '`### Phase 8 (deferred):` still matches the strip (#1729)');
assert.ok(result.includes('Phase 1: Alpha'), 'selected milestone phases retained');
});
// The fixture below carries its own `## Phase Details` heading in the preamble.
// The LF-only sibling test above can't catch a CRLF-specific regression in the
// `[^\n]*` / `\n?` tail of the Phase Details strip regex — those tail tokens are
// LF-anchored, so only a CRLF document can prove the strip still consumes the
// heading (and only the heading, leaving no orphaned `\r`) when line endings are
// `\r\n` throughout.
test('#3235 — CRLF roadmap preserves preamble phases on the do-not-strip branch', () => {
writeState(tmpDir, { milestone: 'v9.0' });
const content = [
'# ROADMAP',
'',
'## Milestones',
'',
'- 🚧 **v9.0 Test Milestone**',
'',
'## Phase Details',
'',
'## Phases',
'',
'### Phase 1: Alpha',
'',
'**Goal:** do alpha',
'',
'## Progress',
'',
'### v9.0 phase progress',
'',
'| Phase | Status |',
].join('\n').replace(/\n/g, '\r\n');
writeRoadmap(tmpDir, content);
const roadmap = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8');
const result = extractCurrentMilestone(roadmap, tmpDir);
assert.ok(result.includes('Phase 1: Alpha'), 'CRLF preamble phases preserved');
assert.ok(result.includes('\r\n'), 'CRLF line endings preserved in the extracted section');
assert.ok(!/^#{1,4}[ \t]*Phase Details\b/m.test(result), 'the unconditional Phase Details strip also runs under CRLF');
assert.ok(!/\r(?!\n)/.test(result), 'the CRLF strip leaves no orphaned CR behind');
});
test('#3235 — property: Phase Details strip is unconditional and #{2,4} bounds hold across generated preambles', () => {
writeState(tmpDir, { milestone: 'v9.0' });
// The alphabet below is deliberately a fixed list of literal line shapes, NOT
// derived from the parser's own regexes (CONTRIBUTING.md #2371: document-shaped,
// not writer-seeded).
const PREAMBLE_LINE = fc.constantFrom(
'## Phase 11: PreTwo',
'### Phase 12: PreThree',
'#### Phase 13: PreFour',
'# Phase 14: PreOne',
'##### Phase 15: PreFive',
'## Phase Details',
'#### Phase Details — trailing',
'prose line',
'**Goal:** something',
'',
'---',
'| Phase | Status |',
);
for (const hasOwnDetails of [true, false]) {
const prop = fc.property(
fc.array(PREAMBLE_LINE, { minLength: 0, maxLength: 12 }),
(preambleLines) => {
const doc = [
'# ROADMAP',
'',
...preambleLines,
'',
'## 🚧 v9.0 Current',
'',
...(hasOwnDetails
? ['### Phase 1: OwnPhase', '', '**Goal:** own goal']
: ['just prose, no phase headings']),
].join('\n');
writeRoadmap(tmpDir, doc);
const roadmap = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8');
const result = extractCurrentMilestone(roadmap, tmpDir);
// Invariant 1 (ALWAYS): no Phase Details heading survives, on either branch.
if (/^#{1,4}[ \t]*Phase Details\b/m.test(result)) return false;
// Invariant 2 (ALWAYS): markers outside the strip regex's #{2,4} bound
// survive if they were present in the input.
if (preambleLines.includes('# Phase 14: PreOne') && !result.includes('PreOne')) return false;
if (preambleLines.includes('##### Phase 15: PreFive') && !result.includes('PreFive')) return false;
if (hasOwnDetails) {
// Invariant 3: the milestone section has its own Phase headings, so
// every preamble Phase heading (#{2,4}) must be stripped.
if (result.includes('PreTwo') || result.includes('PreThree') || result.includes('PreFour')) {
return false;
}
} else {
// Invariant 4: the milestone section has no Phase headings of its own,
// so preamble Phase headings (#{2,4}) are preserved on the do-not-strip branch.
for (const marker of ['PreTwo', 'PreThree', 'PreFour']) {
const appeared = preambleLines.some((line) => line.includes(marker));
if (appeared && !result.includes(marker)) return false;
}
}
return true;
},
);
fc.assert(prop, { seed: 20260809, numRuns: 300, verbose: true });
}
});
});
// ─── replaceInCurrentMilestone ────────────────────────────────────────────────