fix(#2641): harden <details> fallback per trek-e review
Address trek-e's adversarial review on PR #3046. Two critical merge-blockers plus four hardening items, all now covered with tests. CRITICAL #1 — substring-version trap: `[^<]*${escapedVersion}[^<]*` did substring containment, so `milestone: v0.1` matched <summary>v0.10 …</summary> and returned the v0.10 block's body as the active milestone — confidently-wrong content worse than the pre-PR fall-through. Add `(?![\d.])` non-version-character lookahead, mirroring the same boundary protection used by the existing `currentVersionStr` logic on the heading path. Test asserts v0.1 active with v0.10 sibling block returns v0.1's phases, not v0.10's. CRITICAL #2 — nested <details> silent truncation: The lazy `[\s\S]*?</details>` terminates on the FIRST </details>, which is the inner closer when nesting is present. Prior comment claimed "would mis-anchor (acceptable; falls through)" — factually wrong: the match succeeds with truncated body and is returned with a confident `## ${summary}` heading. Future maintainer investigating a "missing phase" report would be misled. Add `!detailsMatch[2].includes('<details')` guard so nesting falls through to stripShippedMilestones (loud failure) instead of returning truncated content (silent failure). Test locks the contract: no synthesized v0.9 heading anchored to truncated body. HARDENING: - Empty-body guard: `<details><summary>v0.9</summary></details>` would synthesize `## v0.9\n` (phantom milestone, zero phases, no error signal). Treat as no-match. - Inline-HTML in <summary>: rejected by `[^<]*` capture. Widen to `(?:(?!</summary>).)*?` (non-greedy until close tag) and strip tags + leading `#` from the captured summary before promoting to a `##` heading. Covers GitHub-rendered <em>(active)</em>, <code>v0.9</code>, <strong>...</strong> patterns. - JSDoc: rewrote to describe both anchoring strategies and the synthesized-heading contract; demoted stale "Port of core.cjs lines 1102-1170" to historical context with the divergence list. - Comment block: rewrote in contract style ("any consumer scanning /##\s*.*vX.Y/ sees the active milestone") instead of coupling to specific call sites (roadmapAnalyze, "later in this file"). Adds explicit regex anatomy + hardening-guards section so future readers can audit each guard. OUT OF SCOPE (per trek-e's "Recommended action" tier): - Debug logging on fall-through paths (Suggestion #10) — adds tracing surface to a function that doesn't currently use logger; appropriate for a follow-up if/when other extraction bugs surface. - Uppercase <DETAILS>/<SUMMARY> + extended attribute coverage (Test gap #7 last two rows) — already covered by the documented `i` flag and the existing <details open> test; adding redundant cases inflates the test set without locking new contracts. Verification: 45/45 roadmap.test.ts tests pass (was 41/41; added 4 hardening tests). FAMP end-to-end smoke unchanged: roadmap.get-phase 3 returns "Claude Code integration polish", roadmap.analyze surfaces v0.9 Local-First Bus in data.milestones with phase_count: 4.
This commit is contained in:
Ben Lamm
@@ -495,6 +495,133 @@ describe('extractCurrentMilestone', () => {
|
||||
expect(result).toMatch(/^##\s+v0\.9 Local-First Bus/m);
|
||||
});
|
||||
|
||||
// ─── Bug #2641 (review hardening): substring-version trap ───
|
||||
it('bug-2641: v0.1 must not substring-match <summary>v0.10 …</summary>', async () => {
|
||||
// The fallback regex anchors on `escapedVersion` inside `<summary>` text.
|
||||
// Without a non-version-character lookahead, `v0.1` matches inside `v0.10`,
|
||||
// and the function returns the v0.10 block's body as the active milestone
|
||||
// — confidently-wrong content (worse than the pre-fix fall-through, which
|
||||
// returned known-incomplete content). The synthesized `## v0.10 …` heading
|
||||
// would then mask the bug from downstream debugging. Lock the boundary.
|
||||
const roadmap = `# Roadmap
|
||||
|
||||
<details>
|
||||
<summary>v0.10 Future Milestone — Phase Details</summary>
|
||||
|
||||
### Phase 7: Wrong Phase
|
||||
**Goal:** This is from v0.10, not v0.1.
|
||||
</details>
|
||||
|
||||
<details>
|
||||
<summary>v0.1 Active — Phase Details</summary>
|
||||
|
||||
### Phase 1: Right Phase
|
||||
**Goal:** This is the active milestone.
|
||||
</details>
|
||||
`;
|
||||
const state = `---\nmilestone: v0.1\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);
|
||||
|
||||
expect(result).toContain('### Phase 1: Right Phase');
|
||||
expect(result).toContain('This is the active milestone');
|
||||
expect(result).not.toContain('Phase 7: Wrong Phase');
|
||||
expect(result).not.toContain('This is from v0.10');
|
||||
});
|
||||
|
||||
// ─── Bug #2641 (review hardening): nested <details> guard ───
|
||||
it('bug-2641: nested <details> falls through (does not silently truncate)', async () => {
|
||||
// The lazy [\s\S]*?</details> terminates on the FIRST </details>, which
|
||||
// is the inner closer when nesting is present. Without a guard, the
|
||||
// function returns truncated body and silently loses everything after the
|
||||
// inner </details>. Detect nesting and fall through to the existing
|
||||
// stripShippedMilestones path so the failure mode is loud (no match) not
|
||||
// silent (truncated content).
|
||||
const roadmap = `# Roadmap
|
||||
|
||||
<details>
|
||||
<summary>v0.9 Local-First Bus — Phase Details</summary>
|
||||
|
||||
### Phase 1: Library
|
||||
<details>
|
||||
<summary>Implementation notes</summary>
|
||||
Detail
|
||||
</details>
|
||||
|
||||
### Phase 2: Polish — would be silently lost without the guard
|
||||
**Goal:** Add polish.
|
||||
</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);
|
||||
|
||||
// The critical contract: must NOT return a synthesized `## v0.9` heading
|
||||
// anchored to truncated body. The truncation case (without the nested-
|
||||
// guard) would emit `## v0.9 Local-First Bus\n\n### Phase 1: Library\n
|
||||
// <details><summary>Implementation notes</summary>\nDetail` and silently
|
||||
// lose Phase 2 — confidently-wrong content. Falling through to
|
||||
// stripShippedMilestones() may leak unrelated content but doesn't claim
|
||||
// to be the active milestone. Loud failure > silent truncation.
|
||||
expect(result).not.toMatch(/^##\s+v0\.9 Local-First Bus/m);
|
||||
// The Phase 1 detail block (which sits between the outer <details> open
|
||||
// and the inner </details>) must not appear under a v0.9 heading.
|
||||
expect(result).not.toMatch(/##\s+v0\.9[\s\S]*Phase 1: Library/);
|
||||
});
|
||||
|
||||
// ─── Bug #2641 (review hardening): empty <details> body guard ───
|
||||
it('bug-2641: empty <details> body falls through (no phantom milestone)', async () => {
|
||||
// <details><summary>v0.9</summary></details> with no body would synthesize
|
||||
// `## v0.9\n` — a phantom milestone with zero phases. roadmapAnalyze would
|
||||
// then return {phases: []} with no error signal. Treat as no-match.
|
||||
const roadmap = `# Roadmap
|
||||
|
||||
<details>
|
||||
<summary>v0.9 Empty</summary>
|
||||
</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);
|
||||
|
||||
// Must not synthesize a phantom heading
|
||||
expect(result).not.toMatch(/^##\s+v0\.9/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
|
||||
// <em>(active)</em> or <code>v0.9</code>. The summary capture must allow
|
||||
// them through and the synthesized `## ` heading must strip the tags so
|
||||
// the result is clean markdown (no `## <em>...</em>`).
|
||||
const roadmap = `# Roadmap
|
||||
|
||||
<details open>
|
||||
<summary><strong>v0.9 Local-First Bus</strong> <em>(active)</em></summary>
|
||||
|
||||
### Phase 3: Polish
|
||||
**Goal:** Add polish.
|
||||
</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);
|
||||
|
||||
expect(result).toContain('### Phase 3: Polish');
|
||||
expect(result).toMatch(/^##\s+v0\.9 Local-First Bus\s+\(active\)/m);
|
||||
// Tags must be stripped from the synthesized heading
|
||||
expect(result).not.toMatch(/^##.*<strong>/m);
|
||||
expect(result).not.toMatch(/^##.*<em>/m);
|
||||
});
|
||||
|
||||
// ─── Bug #2422: same-version sub-heading truncation ───────────────────
|
||||
it('bug-2422: does not truncate at same-version sub-heading (## v2.0 Phase Details)', async () => {
|
||||
const roadmapWithDetails = `# ROADMAP
|
||||
|
||||
Reference in New Issue
Block a user