fix(#2641): symmetric attribute tolerance in stripShippedMilestones + lockdown tests
Address CodeRabbit follow-up review on PR #3046. One real bug + two lockdown gaps + one defensive assertion. REAL BUG — sibling-asymmetry in <details> attribute tolerance: extractCurrentMilestone's <details>-aware fallback uses <details\b[^>]*> to tolerate attributes (#2641 hardening commit). stripShippedMilestones still used literal <details>, so shipped content wrapped in `<details open>` (or any attributed tag) leaked through the strip. This is the failure mode trek-e's review almost caught with the "<details open>" / 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 <details> blocks. Fix: align stripShippedMilestones's regex with extractCurrentMilestone's <details\b[^>]*> form. Comment explicitly notes the symmetry contract so a future change to either function flags the other. Tests added in stripShippedMilestones describe block: - removes <details open> blocks - removes <details class="..." data-..."> 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 `<summary># v0.9 Hash-Prefixed</summary>` 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 (<details> 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.
This commit is contained in:
@@ -99,6 +99,20 @@ describe('stripShippedMilestones', () => {
|
||||
expect(stripShippedMilestones(content)).toBe('middleend');
|
||||
});
|
||||
|
||||
// Bug #2641 (symmetry): tolerate attributes on <details> tag, matching
|
||||
// extractCurrentMilestone's attribute-tolerant fallback. Without this,
|
||||
// shipped content wrapped in `<details open>` (a common GitHub pattern for
|
||||
// sections that should default to expanded) would leak through the strip.
|
||||
it('removes <details open> blocks (attribute-bearing tags)', () => {
|
||||
const content = 'before\n<details open>\nshipped content\n</details>\nafter';
|
||||
expect(stripShippedMilestones(content)).toBe('before\n\nafter');
|
||||
});
|
||||
|
||||
it('removes <details class="..."> blocks (attribute-bearing tags)', () => {
|
||||
const content = 'a<details class="milestone" data-version="v0.5">x</details>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 <summary> stripped from synthesized heading ───
|
||||
it('bug-2641: strips leading # from <summary> text in synthesized heading', async () => {
|
||||
// Prevents a `<summary># v0.9 …</summary>` 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
|
||||
|
||||
<details>
|
||||
<summary># v0.9 Hash-Prefixed</summary>
|
||||
|
||||
### Phase 1: Test
|
||||
**Goal:** Works.
|
||||
</details>
|
||||
`;
|
||||
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 <summary> + leading # ───
|
||||
it('bug-2641: tolerates inline HTML in <summary> 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<string, unknown>;
|
||||
// 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
|
||||
|
||||
@@ -50,8 +50,13 @@ interface PhaseSection {
|
||||
* Port of stripShippedMilestones from core.cjs line 1082-1084.
|
||||
*/
|
||||
export function stripShippedMilestones(content: string): string {
|
||||
// Pattern 1: <details>...</details> blocks (explicit collapse)
|
||||
let result = content.replace(/<details>[\s\S]*?<\/details>/gi, '');
|
||||
// Pattern 1: <details>...</details> blocks (explicit collapse).
|
||||
// <details\b[^>]*> tolerates attributes (e.g. <details open>, <details class="…">).
|
||||
// Symmetry with extractCurrentMilestone()'s <details>-aware fallback (#2641):
|
||||
// both functions must agree on what counts as a <details> 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(/<details\b[^>]*>[\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);
|
||||
|
||||
Reference in New Issue
Block a user