fix(3691): address review — drop no-op Bug 2 change, anchor Plans regex, guard leading-dot IDs
Addresses gsd-code-reviewer (Bug 2 no-op proven empirically; unanchored Plans regex) and sonnet adversarial (leading-dot silent wave-1 default; multi-decimal + bare-bold test gaps). - F1: Drop "Bug 2" nextPhaseOffset regex change (\d[\d.]* → \d): confirmed no-op by reverting and verifying all 7 existing tests still pass — phase headings always start with a digit so \d already matches decimal phases like 02.3. - F2: Anchor plansBlockMatch to start-of-line via (?:^|\n) prefix so mid-line occurrences like `***Plans:***` in prose or `OpenPlans:` prefixes do not produce false matches. - F3: Add leading-dot plan ID validation guard before planData.find() — malformed IDs that fail /^\w[\w.-]*$/ are skipped rather than silently defaulting to wave 1. - F4: Add adversarial test cases for 001.10-PLAN.md (multi-decimal leading-zero ID) and **Plans:** bare-bold (no trailing text); delete vacuous Bug 2 describe block. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -541,9 +541,7 @@ function cmdRoadmapAnnotateDependencies(cwd, phaseNum, raw) {
|
||||
|
||||
const phaseStart = phaseMatch.index;
|
||||
const restAfterHeader = content.slice(phaseStart);
|
||||
// #3691 Bug 2: `\d` only matches a single digit, missing decimal phase headings
|
||||
// like `### Phase 02.3:`. Use `\d[\d.]*` to match integers and decimals alike.
|
||||
const nextPhaseOffset = restAfterHeader.slice(1).search(/\n#{2,4}\s+Phase\s+\d[\d.]*/i);
|
||||
const nextPhaseOffset = restAfterHeader.slice(1).search(/\n#{2,4}\s+Phase\s+\d/i);
|
||||
const phaseEnd = nextPhaseOffset >= 0 ? phaseStart + 1 + nextPhaseOffset : content.length;
|
||||
const phaseSection = content.slice(phaseStart, phaseEnd);
|
||||
|
||||
@@ -562,7 +560,10 @@ function cmdRoadmapAnnotateDependencies(cwd, phaseNum, raw) {
|
||||
// line with no immediately-following checklist items (e.g. a summary line above a
|
||||
// separate bare `Plans:` block) does not consume the match and prevent the actual
|
||||
// list from being found.
|
||||
const plansBlockMatch = phaseSection.match(/(\*{0,2}Plans\*{0,2}:[^\n]*\n)((?:\s*-\s*\[[ x]\][^\n]*\n?)+)/i);
|
||||
// Review fix (F2): `(?:^|\n)` anchors the match to start-of-line so mid-line
|
||||
// occurrences like `***Plans:***` embedded in a sentence or `OpenPlans: foo`
|
||||
// do not trigger a false match. Groups 1 and 2 retain the same semantics.
|
||||
const plansBlockMatch = phaseSection.match(/(?:^|\n)(\*{0,2}Plans\*{0,2}:[^\n]*\n)((?:\s*-\s*\[[ x]\][^\n]*\n?)+)/i);
|
||||
if (!plansBlockMatch) return;
|
||||
|
||||
const plansHeader = plansBlockMatch[1];
|
||||
@@ -580,6 +581,11 @@ function cmdRoadmapAnnotateDependencies(cwd, phaseNum, raw) {
|
||||
// terminating alternation (`-PLAN.md|.md|:|\s—`) as the boundary anchor.
|
||||
const idMatch = line.match(/\[\s*[x ]\s*\]\s*([\w.-]+?)(?:-PLAN\.md|\.md|:|\s—)/i);
|
||||
const planId = idMatch ? idMatch[1] : null;
|
||||
// Review fix (F3): reject malformed IDs that start with `.`, contain consecutive
|
||||
// dots, or otherwise violate the `^\w[\w.-]*$` contract. A leading-dot ID
|
||||
// (e.g. `.invalid-PLAN.md`) would silently default to wave 1 — defensively
|
||||
// skip the line instead so corrupted ROADMAP entries don't corrupt wave layout.
|
||||
if (planId && !/^\w[\w.-]*$/.test(planId)) continue;
|
||||
const planEntry = planId ? planData.find(p => p.planId === planId) : null;
|
||||
const wave = planEntry ? planEntry.wave : 1;
|
||||
if (!linesByWave.has(wave)) linesByWave.set(wave, []);
|
||||
|
||||
@@ -7,20 +7,29 @@
|
||||
/**
|
||||
* Regression — issue #3691
|
||||
*
|
||||
* Three regex defects in `roadmap.cjs` function `cmdRoadmapAnnotateDependencies`:
|
||||
* Two regex defects fixed in `roadmap.cjs` function `cmdRoadmapAnnotateDependencies`,
|
||||
* plus two review-cycle additions (F3 defensive guard, F4 adversarial gaps):
|
||||
*
|
||||
* Bug 1 (line ~553) — Plans-block detection regex `/(Plans:\s*\n)/i` requires no text
|
||||
* after the colon. Headers like `Plans: 3 plans across 2 waves\n` or
|
||||
* `**Plans:** 3 plans\n` are silently skipped and the function early-returns.
|
||||
*
|
||||
* Bug 2 (line ~542) — Phase-section boundary regex `/\n#{2,4}\s+Phase\s+\d/i` uses
|
||||
* `\d` (single digit) so decimal phase headings like `### Phase 02.3:` are not
|
||||
* recognized as boundaries. Content from an adjacent decimal phase may bleed into
|
||||
* the section being annotated.
|
||||
* Fix: `(?:^|\n)(\*{0,2}Plans\*{0,2}:[^\n]*\n)` anchors to start-of-line and
|
||||
* accepts optional bold wrappers and any trailing text on the header line.
|
||||
*
|
||||
* Bug 3 (line ~566) — Plan-ID extraction regex `/([\w-]+?)/` excludes `.`, so
|
||||
* decimal plan IDs like `02.3-01` are captured as `02` only, never match
|
||||
* the planData entry, and every plan defaults to wave 1.
|
||||
* Fix: `[\w.-]+?` includes `.` so decimal IDs are captured in full.
|
||||
*
|
||||
* Note: "Bug 2" (phase-section boundary `\d` → `\d[\d.]*`) was confirmed empirically
|
||||
* to be a no-op: any phase heading starts with a digit, so `\d` already matches.
|
||||
* The no-op change was dropped from the PR; this file has no Bug 2 describe block.
|
||||
*
|
||||
* Review additions:
|
||||
* F3 — Leading-dot plan ID guard: malformed IDs like `.invalid` are rejected
|
||||
* before planData.find() rather than silently defaulting to wave 1.
|
||||
* F4 — Adversarial gaps: multi-decimal leading-zero IDs (001.10-PLAN.md) and
|
||||
* bare-bold `**Plans:**` (no trailing text) are explicitly covered.
|
||||
*/
|
||||
|
||||
const { test, describe, afterEach } = require('node:test');
|
||||
@@ -264,62 +273,117 @@ describe('bug #3691 — Bug 3: decimal plan IDs (e.g. 02.3-01-PLAN.md) parse cor
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Bug 2 — Phase boundary: decimal phase headings as section terminators
|
||||
// F3 review fix — Leading-dot ID validation guard (defensive, malformed ROADMAP)
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('bug #3691 — Bug 2: decimal phase heading used as section boundary', () => {
|
||||
describe('review fix F3 — leading-dot plan ID is rejected (defensive guard)', () => {
|
||||
let tmpDir;
|
||||
afterEach(() => cleanup(tmpDir));
|
||||
|
||||
test('adjacent decimal phase heading terminates current phase section', (t) => {
|
||||
// Pre-fix: `/\n#{2,4}\s+Phase\s+\d/i` uses bare `\d` → doesn't match `### Phase 02.3:`
|
||||
// → phaseEnd = content.length → section includes all subsequent phases → plans from
|
||||
// phase 02.3 are mistakenly processed when annotating phase 02.2.
|
||||
// Post-fix: `/\n#{2,4}\s+Phase\s+\d[\d.]*/i` → matches decimal headings too
|
||||
//
|
||||
// Setup: phase 02.2 has 2 plans at different waves (so wave annotation is written).
|
||||
// phase 02.3 has 1 unannotated plan. We annotate phase 02.2 only.
|
||||
// Post-fix: phase 02.3 section must remain untouched.
|
||||
test('checklist line with leading-dot plan ID is skipped and does not silently default to wave 1', (t) => {
|
||||
// Guards: `.invalid-PLAN.md` would be captured as `.invalid` by the `[\w.-]+?` regex
|
||||
// (since `.` is now included), which starts with a dot — an invalid ID.
|
||||
// Without the guard, planData.find() misses it and wave defaults to 1, silently
|
||||
// polluting the output. With the guard, the line is skipped entirely.
|
||||
// We verify this by having TWO real plans with known waves (1 and 2), plus one
|
||||
// malformed line. The malformed line must not appear in the wave-annotated output.
|
||||
const roadmap = [
|
||||
'# Roadmap',
|
||||
'',
|
||||
'### Phase 02.2: First phase',
|
||||
'### Phase 1: Foundation',
|
||||
'',
|
||||
'Plans: 2 plans',
|
||||
'- [ ] 02.2-01-PLAN.md — First task',
|
||||
'- [ ] 02.2-02-PLAN.md — Second task',
|
||||
'',
|
||||
'### Phase 02.3: Surgical edit ops',
|
||||
'',
|
||||
'Plans: 2 plans',
|
||||
'- [ ] 02.3-01-PLAN.md — Path resolver',
|
||||
'- [ ] 02.3-02-PLAN.md — Op handlers',
|
||||
'Plans: 3 items',
|
||||
'- [ ] 01-01-PLAN.md — Task A',
|
||||
'- [ ] .invalid-PLAN.md — Corrupted entry',
|
||||
'- [ ] 01-02-PLAN.md — Task B',
|
||||
'',
|
||||
].join('\n');
|
||||
|
||||
tmpDir = makePlanProject({
|
||||
'.planning/ROADMAP.md': roadmap,
|
||||
'.planning/phases/02.2-first/02.2-01-PLAN.md': makePlan({ phase: '02.2', plan: '02.2-01', wave: 1 }),
|
||||
'.planning/phases/02.2-first/02.2-02-PLAN.md': makePlan({ phase: '02.2', plan: '02.2-02', wave: 2, dependsOn: ['02.2-01'] }),
|
||||
'.planning/phases/02.3-surgical-edit-ops/02.3-01-PLAN.md': makePlan({ phase: '02.3', plan: '02.3-01', wave: 1 }),
|
||||
'.planning/phases/02.3-surgical-edit-ops/02.3-02-PLAN.md': makePlan({ phase: '02.3', plan: '02.3-02', wave: 1 }),
|
||||
'.planning/phases/01-foundation/01-01-PLAN.md': makePlan({ phase: '1', plan: '01-01', wave: 1 }),
|
||||
'.planning/phases/01-foundation/01-02-PLAN.md': makePlan({ phase: '1', plan: '01-02', wave: 2, dependsOn: ['01-01'] }),
|
||||
});
|
||||
|
||||
// Annotate phase 02.2 only
|
||||
const result = runGsdTools('roadmap annotate-dependencies 02.2', tmpDir);
|
||||
const result = runGsdTools('roadmap annotate-dependencies 1', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
const out = JSON.parse(result.output);
|
||||
assert.strictEqual(out.updated, true, 'phase 02.2 must be annotated (2 plans across 2 waves)');
|
||||
assert.strictEqual(out.waves, 2, 'phase 02.2 must show 2 waves');
|
||||
// Two valid plans resolve to 2 waves — annotation must still proceed
|
||||
assert.strictEqual(out.updated, true, 'annotation must proceed despite malformed line');
|
||||
assert.strictEqual(out.waves, 2, 'two distinct waves from the two valid plans');
|
||||
|
||||
const written = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8');
|
||||
// Wave headers must appear in phase 02.2 section
|
||||
const phase022Section = written.split('### Phase 02.3:')[0];
|
||||
assert.ok(/\*\*Wave/.test(phase022Section),
|
||||
'wave headers must be written into the phase 02.2 section');
|
||||
// Phase 02.3 section must NOT contain wave headers (boundary must stop at Phase 02.3 heading)
|
||||
const phase023Section = written.split('### Phase 02.3:')[1] ?? '';
|
||||
assert.ok(!/\*\*Wave/.test(phase023Section),
|
||||
'phase 02.3 section must not contain wave headers when only 02.2 was annotated');
|
||||
// The malformed line should not appear in the written output (it was skipped)
|
||||
assert.ok(!written.includes('.invalid-PLAN.md'),
|
||||
'malformed leading-dot entry must be dropped from the annotated output');
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// F4 review additions — adversarial test gaps
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('review fix F4 — adversarial test gaps', () => {
|
||||
let tmpDir;
|
||||
afterEach(() => cleanup(tmpDir));
|
||||
|
||||
test('001.10-PLAN.md multi-decimal leading-zero ID is captured fully and wave-assigned correctly', (t) => {
|
||||
// Guards regression of Bug 3: `[\w-]+?` would stop at the first `.` and
|
||||
// capture `001` instead of `001.10`, which never matches any planData entry.
|
||||
// Post-fix `[\w.-]+?` must capture `001.10` in full.
|
||||
const roadmap = [
|
||||
'# Roadmap',
|
||||
'',
|
||||
'### Phase 001.10: Extended decimal phase',
|
||||
'',
|
||||
'Plans: 2 plans across 2 waves',
|
||||
'- [ ] 001.10-01-PLAN.md — First task',
|
||||
'- [ ] 001.10-02-PLAN.md — Second task',
|
||||
'',
|
||||
].join('\n');
|
||||
|
||||
tmpDir = makePlanProject({
|
||||
'.planning/ROADMAP.md': roadmap,
|
||||
'.planning/phases/001.10-extended/001.10-01-PLAN.md': makePlan({ phase: '001.10', plan: '001.10-01', wave: 1 }),
|
||||
'.planning/phases/001.10-extended/001.10-02-PLAN.md': makePlan({ phase: '001.10', plan: '001.10-02', wave: 2, dependsOn: ['001.10-01'] }),
|
||||
});
|
||||
|
||||
const result = runGsdTools('roadmap annotate-dependencies 001.10', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
const out = JSON.parse(result.output);
|
||||
assert.strictEqual(out.updated, true,
|
||||
'multi-decimal leading-zero plan ID must be captured fully and annotated');
|
||||
assert.strictEqual(out.waves, 2,
|
||||
'wave 2 dependency must be resolved from full 001.10-02 ID (not truncated to 001)');
|
||||
});
|
||||
|
||||
test('**Plans:** (bold, no trailing text) is matched and checklist is processed', (t) => {
|
||||
// Guards the bare-bold variant: `**Plans:**` with nothing after the colon.
|
||||
// The `[^\n]*` quantifier accepts zero chars so this should already work,
|
||||
// but this test would fail if `\*{0,2}Plans\*{0,2}` regressed to require no stars.
|
||||
const roadmap = [
|
||||
'# Roadmap',
|
||||
'',
|
||||
'### Phase 1: Foundation',
|
||||
'',
|
||||
'**Plans:**',
|
||||
'- [ ] 01-01-PLAN.md — Task A',
|
||||
'- [ ] 01-02-PLAN.md — Task B',
|
||||
'',
|
||||
].join('\n');
|
||||
|
||||
tmpDir = makePlanProject({
|
||||
'.planning/ROADMAP.md': roadmap,
|
||||
'.planning/phases/01-foundation/01-01-PLAN.md': makePlan({ phase: '1', plan: '01-01', wave: 1 }),
|
||||
'.planning/phases/01-foundation/01-02-PLAN.md': makePlan({ phase: '1', plan: '01-02', wave: 2, dependsOn: ['01-01'] }),
|
||||
});
|
||||
|
||||
const result = runGsdTools('roadmap annotate-dependencies 1', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
const out = JSON.parse(result.output);
|
||||
assert.strictEqual(out.updated, true,
|
||||
'**Plans:** bare-bold variant (no trailing text) must be detected and annotated');
|
||||
assert.strictEqual(out.waves, 2,
|
||||
'wave assignment must resolve correctly from planData for bare-bold variant');
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user