From 13bf56477a84c36fb2b718ff6e564126f83b59fa Mon Sep 17 00:00:00 2001 From: Ben Lamm Date: Sun, 3 May 2026 12:19:38 -0400 Subject: [PATCH] fix(#2641): symmetric attribute tolerance in stripShippedMilestones + lockdown tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address CodeRabbit follow-up review on PR #3046. One real bug + two lockdown gaps + one defensive assertion. REAL BUG — sibling-asymmetry in
attribute tolerance: extractCurrentMilestone's
-aware fallback uses ]*> to tolerate attributes (#2641 hardening commit). stripShippedMilestones still used literal
, so shipped content wrapped in `
` (or any attributed tag) leaked through the strip. This is the failure mode trek-e's review almost caught with the "
" / extended-attribute test gap I deferred — CodeRabbit caught the deeper issue: it's not just a test gap, it's an actual asymmetry between the two functions that handle
blocks. Fix: align stripShippedMilestones's regex with extractCurrentMilestone's ]*> form. Comment explicitly notes the symmetry contract so a future change to either function flags the other. Tests added in stripShippedMilestones describe block: - removes
blocks - removes
blocks LOCKDOWN — leading-# strip in synthesized heading: My existing inline-HTML test exercised tag-stripping but didn't directly exercise the leading-# strip path (`.replace(/^#+\s*/, '')`). Added a dedicated test with `# v0.9 Hash-Prefixed` so a future refactor that drops the strip would fail loudly instead of producing `## # v0.9 …` (which downstream `#{2,4}` regex parses as a 4-hash header). DEFENSIVE — toBeDefined guard in roadmapAnalyze regression test: Added `expect(data.milestones).toBeDefined()` before casting and calling `.some()`. Failure now reports "expected undefined to be defined" instead of TypeError. META: my prior adversarial pass missed the sibling-asymmetry because the checklist's "sibling consistency" item only audited PARSERS for the same INPUT field (STATE.md's `milestone:`), not ADJACENT FUNCTIONS that process the same DATA SHAPE (
blocks). The latter is a wider audit — every adjacent function that touches the data shape my new code relies on. Will refine the learned rule. Verification: 51/51 roadmap.test.ts pass (was 48; +3 tests). FAMP smoke unchanged: roadmap.get-phase 3 returns active milestone phase. --- sdk/src/query/roadmap.test.ts | 43 +++++++++++++++++++++++++++++++++++ sdk/src/query/roadmap.ts | 9 ++++++-- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/sdk/src/query/roadmap.test.ts b/sdk/src/query/roadmap.test.ts index 64208b2cc..05b844ebc 100644 --- a/sdk/src/query/roadmap.test.ts +++ b/sdk/src/query/roadmap.test.ts @@ -99,6 +99,20 @@ describe('stripShippedMilestones', () => { expect(stripShippedMilestones(content)).toBe('middleend'); }); + // Bug #2641 (symmetry): tolerate attributes on
tag, matching + // extractCurrentMilestone's attribute-tolerant fallback. Without this, + // shipped content wrapped in `
` (a common GitHub pattern for + // sections that should default to expanded) would leak through the strip. + it('removes
blocks (attribute-bearing tags)', () => { + const content = 'before\n
\nshipped content\n
\nafter'; + expect(stripShippedMilestones(content)).toBe('before\n\nafter'); + }); + + it('removes
blocks (attribute-bearing tags)', () => { + const content = 'a
x
b'; + expect(stripShippedMilestones(content)).toBe('ab'); + }); + it('returns content unchanged when no details blocks', () => { expect(stripShippedMilestones('no details here')).toBe('no details here'); }); @@ -594,6 +608,32 @@ Detail expect(result).not.toMatch(/^##\s+v0\.9/m); }); + // ─── Bug #2641 (lockdown): leading `#` in stripped from synthesized heading ─── + it('bug-2641: strips leading # from text in synthesized heading', async () => { + // Prevents a `# v0.9 …` from producing `## # v0.9 …`, + // which downstream `#{2,4}` heading regexes would parse as a 4-hash + // header. The implementation uses `.replace(/^#+\s*/, '')` on the captured + // summary; this test pins that path so a future refactor doesn't drop it. + const roadmap = `# Roadmap + +
+# v0.9 Hash-Prefixed + +### Phase 1: Test +**Goal:** Works. +
+`; + const state = `---\nmilestone: v0.9\n---\n# State\n`; + await writeFile(join(tmpDir, '.planning', 'STATE.md'), state); + await writeFile(join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + + const result = await extractCurrentMilestone(roadmap, tmpDir); + + // Synthesized heading must be `## v0.9 …`, not `## # v0.9 …` + expect(result).toMatch(/^##\s+v0\.9 Hash-Prefixed/m); + expect(result).not.toMatch(/^##\s+#+/m); + }); + // ─── Bug #2641 (review hardening): inline HTML in + leading # ─── it('bug-2641: tolerates inline HTML in and strips it from synthesized heading', async () => { // GitHub-rendered summaries commonly contain inline tags like @@ -937,6 +977,9 @@ describe('roadmapAnalyze', () => { const result = await roadmapAnalyze([], tmpDir); const data = result.data as Record; + // Defensive guard: fail with a clear message if roadmapAnalyze didn't + // populate data.milestones, rather than throwing TypeError on `.some()`. + expect(data.milestones).toBeDefined(); const milestones = data.milestones as Array<{ heading: string; version: string }>; // Active milestone surfaces with correct version diff --git a/sdk/src/query/roadmap.ts b/sdk/src/query/roadmap.ts index cce4f0967..013af8846 100644 --- a/sdk/src/query/roadmap.ts +++ b/sdk/src/query/roadmap.ts @@ -50,8 +50,13 @@ interface PhaseSection { * Port of stripShippedMilestones from core.cjs line 1082-1084. */ export function stripShippedMilestones(content: string): string { - // Pattern 1:
...
blocks (explicit collapse) - let result = content.replace(/
[\s\S]*?<\/details>/gi, ''); + // Pattern 1:
...
blocks (explicit collapse). + // ]*> tolerates attributes (e.g.
,
). + // Symmetry with extractCurrentMilestone()'s
-aware fallback (#2641): + // both functions must agree on what counts as a
opening tag, or + // shipped content wrapped in attributed tags would leak through here while + // the active-milestone anchor in extractCurrentMilestone() correctly fires. + let result = content.replace(/]*>[\s\S]*?<\/details>/gi, ''); // Pattern 2: inline milestone headings marked as shipped. // Keep aligned with heading levels accepted by extractCurrentMilestone() (## and ###). const sections = result.split(/(?=^#{2,3}\s)/m);