mirror of
https://github.com/glittercowboy/get-shit-done
synced 2026-05-08 16:22:14 +02:00
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
|
||||
|
||||
@@ -146,7 +146,21 @@ export async function getMilestoneInfo(projectDir: string, workstream?: string):
|
||||
/**
|
||||
* Extract the current milestone section from ROADMAP.md.
|
||||
*
|
||||
* Port of extractCurrentMilestone from core.cjs lines 1102-1170.
|
||||
* Two anchoring strategies, tried in order:
|
||||
* 1. Markdown heading containing the active version (`^#{1,3}\s+.*vX.Y…`).
|
||||
* 2. `<details><summary>vX.Y…</summary>…</details>` block (the GitHub-friendly
|
||||
* collapse pattern; see #2641). When this fallback fires, the captured
|
||||
* `<summary>` text is synthesized as a `##` heading prepended to the
|
||||
* returned slice so downstream consumers that scan for milestone headings
|
||||
* (e.g. the `data.milestones` loop in `roadmapAnalyze`) still see an
|
||||
* active-milestone anchor.
|
||||
*
|
||||
* If neither strategy matches the active version, falls through to
|
||||
* `stripShippedMilestones(content)`.
|
||||
*
|
||||
* Originally ported from core.cjs lines 1102-1170; the TS implementation has
|
||||
* since diverged (Backlog-leak fix #2422, phase-vX.Y truncation fix #2619,
|
||||
* fenced-code-block tracking #2787, `<details><summary>` fallback #2641).
|
||||
*
|
||||
* @param content - Full ROADMAP.md content
|
||||
* @param projectDir - Working directory for reading STATE.md
|
||||
@@ -207,26 +221,47 @@ export async function extractCurrentMilestone(content: string, projectDir: strin
|
||||
// active ROADMAP. The init.phase-op safety guard then misfires and can
|
||||
// route phase lookups into archived milestones.
|
||||
//
|
||||
// <details\b[^>]*> tolerates attributes like <details open> and
|
||||
// <details class="...">. <summary\b[^>]*> tolerates the same on the
|
||||
// <summary> tag. The lazy [\s\S]*? terminates on the first </details>;
|
||||
// nested <details> inside the active milestone are not expected and would
|
||||
// mis-anchor (acceptable; FAMP-style ROADMAPs do not nest, and any project
|
||||
// that does will fall through to the existing stripShippedMilestones path
|
||||
// with no regression vs. today's behavior).
|
||||
// Regex anatomy:
|
||||
// <details\b[^>]*> tolerate attributes (e.g. <details open>)
|
||||
// \s*<summary\b[^>]*> tolerate attributes on <summary>
|
||||
// ((?:(?!</summary>).)*? non-greedy summary capture; tolerates
|
||||
// ${escapedVersion} inline HTML in the summary text
|
||||
// (?![\d.]) non-version-character lookahead — prevents
|
||||
// `v0.1` from substring-matching `v0.10`
|
||||
// (?:(?!</summary>).)*)
|
||||
// </summary> end of summary
|
||||
// ([\s\S]*?)</details> lazy body capture to the FIRST </details>
|
||||
//
|
||||
// We capture the <summary> text and prepend it as a normalized `##`
|
||||
// milestone heading on the returned slice. This keeps downstream consumers
|
||||
// that scan for `##` milestone headings (e.g. roadmapAnalyze's
|
||||
// data.milestones loop later in this file) producing a meaningful entry
|
||||
// for the active milestone instead of seeing an unanchored body.
|
||||
// Contract: any consumer that scans the returned slice for milestone
|
||||
// headings (e.g. /##\s*.*vX.Y/) sees the active milestone's anchor. We
|
||||
// synthesize that heading from the captured <summary> text rather than
|
||||
// returning the body alone.
|
||||
//
|
||||
// Hardening guards:
|
||||
// - Nested <details>: the lazy quantifier truncates at the inner
|
||||
// </details>, silently losing trailing phases. Detect and fall through
|
||||
// to stripShippedMilestones() instead of returning truncated content.
|
||||
// - Empty body: a <details> block with no body would synthesize a heading
|
||||
// with nothing under it. Treat as no-match.
|
||||
// - Summary sanitization: strip inline HTML (e.g. <em>active</em>) and
|
||||
// leading `#` tokens before promoting to a `##` heading, so the result
|
||||
// is a single well-formed markdown heading.
|
||||
const detailsPattern = new RegExp(
|
||||
`<details\\b[^>]*>\\s*<summary\\b[^>]*>([^<]*${escapedVersion}[^<]*)</summary>([\\s\\S]*?)</details>`,
|
||||
`<details\\b[^>]*>\\s*<summary\\b[^>]*>` +
|
||||
`((?:(?!</summary>).)*?${escapedVersion}(?![\\d.])(?:(?!</summary>).)*)` +
|
||||
`</summary>([\\s\\S]*?)</details>`,
|
||||
'i'
|
||||
);
|
||||
const detailsMatch = content.match(detailsPattern);
|
||||
if (detailsMatch) {
|
||||
const summary = detailsMatch[1].trim();
|
||||
if (
|
||||
detailsMatch &&
|
||||
detailsMatch[2].trim() && // empty-body guard
|
||||
!detailsMatch[2].includes('<details') // nested-<details> guard
|
||||
) {
|
||||
const summary = detailsMatch[1]
|
||||
.replace(/<[^>]+>/g, '') // strip inline HTML
|
||||
.replace(/^#+\s*/, '') // strip leading `#`
|
||||
.trim();
|
||||
const body = detailsMatch[2];
|
||||
return `## ${summary}\n${body}`;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user