From edd3c6c332fb6d414e44ed586a82bb979a464e0e Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 6 Jun 2026 22:13:20 -0400 Subject: [PATCH] fix(#730): scope current-milestone Phase Details section in roadmap parser (#748) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `extractCurrentMilestone()` scoped the current-milestone window to its `## Phases` checklist subsection and terminated at the milestone's own `## Milestone … (Phase Details)` heading, so the `### Phase N:` detail headers fell outside scope. Every parser-backed command — `init.phase-op` (and thus `/gsd:discuss-phase`, `/gsd:plan-phase`), `state`, `roadmap list`, and `validate health` (W006) — therefore could not resolve phases of any milestone after the first until a `.planning/phases/` directory already existed, blocking discuss/plan. The parser now additionally includes the current milestone's `(Phase Details)` section in scope, located via the already-computed version matches and anchored (boundary-aware) to the selected milestone's version token so sibling sub-milestones sharing a version prefix do not cross-pollinate. The existing heading selection and primary window are unchanged. Adds tests/bug-730-milestone-phase-details-scope.test.cjs covering the two-milestone reproduction, first-milestone non-regression, direct getRoadmapPhaseInternal resolution, validate-health W006 visibility, a three-milestone roadmap, and the closed-sibling sub-milestone case. Co-authored-by: Claude Opus 4.8 --- ...0-roadmap-milestone-phase-details-scope.md | 5 + scripts/lint-test-file-count.allowlist.json | 1 + src/core.cts | 97 ++++-- ...730-milestone-phase-details-scope.test.cjs | 275 ++++++++++++++++++ 4 files changed, 348 insertions(+), 30 deletions(-) create mode 100644 .changeset/730-roadmap-milestone-phase-details-scope.md create mode 100644 tests/bug-730-milestone-phase-details-scope.test.cjs diff --git a/.changeset/730-roadmap-milestone-phase-details-scope.md b/.changeset/730-roadmap-milestone-phase-details-scope.md new file mode 100644 index 000000000..ea32b54a1 --- /dev/null +++ b/.changeset/730-roadmap-milestone-phase-details-scope.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 748 +--- +The roadmap parser now resolves fresh phases of the current milestone in multi-milestone roadmaps. `extractCurrentMilestone()` scoped the current-milestone window to its `## Phases` checklist subsection and stopped at the milestone's own `## Milestone … (Phase Details)` heading, so the `### Phase N:` detail headers fell out of scope. Any command backed by the parser — `init.phase-op` (and therefore `/gsd:discuss-phase` and `/gsd:plan-phase`), `state`, `roadmap list`, and `validate health` (W006) — could not resolve phases of any milestone after the first until a `.planning/phases/` directory already existed, blocking discuss/plan. The parser now also includes the current milestone's `(Phase Details)` section in scope, anchored to the selected milestone's version token so sibling sub-milestones do not cross-pollinate. (#730) diff --git a/scripts/lint-test-file-count.allowlist.json b/scripts/lint-test-file-count.allowlist.json index f16983ace..abec29430 100644 --- a/scripts/lint-test-file-count.allowlist.json +++ b/scripts/lint-test-file-count.allowlist.json @@ -43,6 +43,7 @@ }, "milestone": { "files": [ + "bug-730-milestone-phase-details-scope.test.cjs", "milestone-archive.test.cjs", "milestone-helper.test.cjs", "milestone-prefixed-convention.test.cjs", diff --git a/src/core.cts b/src/core.cts index c031210a7..a62e1c38f 100644 --- a/src/core.cts +++ b/src/core.cts @@ -1085,37 +1085,40 @@ function extractCurrentMilestone(content: string, cwd?: string): string { const sectionStart = selected.index; - const sectionMatch = selected; - const headingLevel = (sectionMatch[1].match(/^(#{1,3})\s/) ?? ['', '#'])[1].length; - const restContent = content.slice(sectionStart + sectionMatch[0].length); - const nextMilestonePattern = new RegExp( - `^#{1,${headingLevel}}\\s+(?!Phase\\s+\\S)(?:.*v\\d+\\.\\d+|✅|📋|🚧)`, - 'i' - ); - - let sectionEnd = content.length; - let fenceChar: string | null = null; - let fenceLen = 0; - let charOffset = 0; - for (const line of restContent.split('\n')) { - const fenceMatch = line.match(/^\s{0,3}((?:`{3,}|~{3,}))(.*)/); - if (fenceMatch) { - const char = fenceMatch[1][0]; - const len = fenceMatch[1].length; - const trailing = fenceMatch[2] || ''; - if (!fenceChar) { - fenceChar = char; - fenceLen = len; - } else if (char === fenceChar && len >= fenceLen && /^\s*$/.test(trailing)) { - fenceChar = null; - fenceLen = 0; + const computeSectionEnd = (headingText: string, headingStart: number): number => { + const level = (headingText.match(/^(#{1,3})\s/) ?? ['', '#'])[1].length; + const rest = content.slice(headingStart + headingText.length); + const stopPattern = new RegExp( + `^#{1,${level}}\\s+(?!Phase\\s+\\S)(?:.*v\\d+\\.\\d+|✅|📋|🚧)`, + 'i', + ); + let end = content.length; + let fc: string | null = null; + let fl = 0; + let off = 0; + for (const line of rest.split('\n')) { + const fm = line.match(/^\s{0,3}((?:`{3,}|~{3,}))(.*)/); + if (fm) { + const ch = fm[1][0]; + const ln = fm[1].length; + const trailing = fm[2] || ''; + if (!fc) { + fc = ch; + fl = ln; + } else if (ch === fc && ln >= fl && /^\s*$/.test(trailing)) { + fc = null; + fl = 0; + } + } else if (!fc && stopPattern.test(line)) { + end = headingStart + headingText.length + off; + break; } - } else if (!fenceChar && nextMilestonePattern.test(line)) { - sectionEnd = sectionStart + sectionMatch[0].length + charOffset; - break; + off += line.length + 1; } - charOffset += line.length + 1; - } + return end; + }; + + const sectionEnd = computeSectionEnd(selected[0], sectionStart); const anyMilestonePattern = /^#{1,3}\s+(?!Phase\s+\S)(?:.*v\d+\.\d+|✅|📋|🚧)/im; const firstMilestoneMatch = content.match(anyMilestonePattern); @@ -1125,12 +1128,46 @@ function extractCurrentMilestone(content: string, cwd?: string): string { const beforeMilestones = content.slice(0, preambleCutoff); const currentSection = content.slice(sectionStart, sectionEnd); + // Multi-milestone roadmaps split each added milestone across two version-bearing + // headings: a `## Phases` checklist subsection (early) and a dedicated + // `## Milestone … (Phase Details)` section (late) holding the `### Phase N:` + // detail headers. The scope window above stops at the next version-bearing + // heading — the current milestone's OWN Phase Details heading — leaving those + // detail headers outside `currentSection`. Append that section so phase + // resolution and counting see the current milestone's phases. Anchor the lookup + // to the SELECTED heading's specific version token (boundary-aware, so a + // `v3.0` state does not match a `v3.0-A` sub-milestone) so sibling milestones + // that share a version prefix do not cross-pollinate. (#730) + const selectedVersionToken = selected[1].match( + /v\d+(?:\.\d+)+(?:[-.][A-Za-z0-9]+)*/i, + )?.[0]; + const detailsVersionBoundary = selectedVersionToken + ? new RegExp(`${escapeRegex(selectedVersionToken)}(?![\\w.-])`, 'i') + : null; + let detailsSection = ''; + const detailsMatch = allMatches.find( + (m) => + /\(Phase\s+Details\)/i.test(m[1]) && + !isClosed(m[1]) && + (!detailsVersionBoundary || detailsVersionBoundary.test(m[1])) && + (m.index ?? 0) >= sectionEnd, + ); + if (detailsMatch) { + const detailsStart = detailsMatch.index ?? 0; + detailsSection = content.slice( + detailsStart, + computeSectionEnd(detailsMatch[0], detailsStart), + ); + } + const preamble = beforeMilestones .replace(/
[\s\S]*?<\/details>/gi, '') .replace(/^#{2,4}\s*Phase\s+[\w][\w.-]*\s*:[^\n]*(?:\n(?!#{1,6}\s)[^\n]*)*\n?/gim, '') .replace(/^#{1,4}\s*Phase Details\b[^\n]*\n?/gim, ''); - return preamble + currentSection; + return detailsSection + ? preamble + currentSection + '\n' + detailsSection + : preamble + currentSection; } /** diff --git a/tests/bug-730-milestone-phase-details-scope.test.cjs b/tests/bug-730-milestone-phase-details-scope.test.cjs new file mode 100644 index 000000000..c44ef4f20 --- /dev/null +++ b/tests/bug-730-milestone-phase-details-scope.test.cjs @@ -0,0 +1,275 @@ +/** + * Regression test for bug #730: phase details defined under a milestone-scoped + * "## Milestone vX.Y — … (Phase Details)" section are invisible to phase + * resolution (getRoadmapPhaseInternal / init phase-op) when the flat shared + * "## Phase Details" section for an earlier milestone sits between the shared + * ## Phases checklist and the per-milestone Phase Details section. + * + * The bug manifests ONLY before any .planning/phases/ directory exists because + * findPhaseInternal masks it once the dir is created. RED step — tests 1 and 3 + * are expected to fail against current code. + */ + +const { test, describe, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); +const os = require('os'); +const { runGsdTools, cleanup } = require('./helpers.cjs'); + +// --------------------------------------------------------------------------- +// Shared fixture content +// --------------------------------------------------------------------------- + +const STATE_CONTENT = `--- +milestone: v1.1 +--- +`; + +const ROADMAP_CONTENT = `# Roadmap: Example + +## Phases + +- [x] **Phase 1: Setup** — initial scaffold + +### Milestone v1.1 — Second milestone (added 2026-01-01) + +- [ ] **Phase 2: Feature** — the new thing + +## Phase Details + +### Phase 1: Setup +**Goal:** scaffold the app. + +## Milestone v1.1 — Second milestone (Phase Details) + +### Phase 2: Feature +**Goal:** build the new thing. +`; + +// --------------------------------------------------------------------------- +// Helper: create a bare project with .planning/ but NO .planning/phases/ dir +// --------------------------------------------------------------------------- + +function createBareProject() { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-test-730-')); + fs.mkdirSync(path.join(tmpDir, '.planning'), { recursive: true }); + return tmpDir; +} + +// --------------------------------------------------------------------------- +// Suite +// --------------------------------------------------------------------------- + +describe('bug #730 — milestone (Phase Details) section scope resolution', () => { + let dir; + + beforeEach(() => { + dir = createBareProject(); + fs.writeFileSync(path.join(dir, '.planning', 'STATE.md'), STATE_CONTENT, 'utf-8'); + fs.writeFileSync(path.join(dir, '.planning', 'ROADMAP.md'), ROADMAP_CONTENT, 'utf-8'); + }); + + afterEach(() => { + cleanup(dir); + }); + + // ------------------------------------------------------------------------- + // Test 1 (AC1): init phase-op resolves phase defined only under its + // per-milestone "(Phase Details)" section + // ------------------------------------------------------------------------- + test('init phase-op resolves a current-milestone phase defined only under its (Phase Details) section', () => { + const r = runGsdTools('init phase-op 2', dir); + assert.ok(r.success, `init phase-op 2 failed: ${r.error}`); + + const out = JSON.parse(r.output); + assert.strictEqual(out.phase_found, true, `phase_found should be true; got phase_found=${out.phase_found}, expected_phase_dir=${out.expected_phase_dir}`); + assert.strictEqual(out.phase_name, 'Feature', `phase_name should be 'Feature'; got '${out.phase_name}'`); + assert.strictEqual(out.padded_phase, '02', `padded_phase should be '02'; got '${out.padded_phase}'`); + assert.strictEqual(out.expected_phase_dir, '.planning/phases/02-feature', `expected_phase_dir should be '.planning/phases/02-feature'; got '${out.expected_phase_dir}'`); + }); + + // ------------------------------------------------------------------------- + // Test 2 (AC4): first-milestone phase still resolves via the flat + // "## Phase Details" section — no regression + // ------------------------------------------------------------------------- + test('init phase-op still resolves a first-milestone phase (no regression on flat Phase Details)', () => { + const r = runGsdTools('init phase-op 1', dir); + assert.ok(r.success, `init phase-op 1 failed: ${r.error}`); + + const out = JSON.parse(r.output); + assert.strictEqual(out.phase_found, true, `phase_found should be true for phase 1; got ${out.phase_found}`); + assert.strictEqual(out.phase_name, 'Setup', `phase_name should be 'Setup'; got '${out.phase_name}'`); + }); + + // ------------------------------------------------------------------------- + // Test 3 (AC5): getRoadmapPhaseInternal resolves the current-milestone phase + // directly before any phases/ dir exists + // ------------------------------------------------------------------------- + test('getRoadmapPhaseInternal resolves the current-milestone phase directly before any dir exists', () => { + const core = require('../gsd-core/bin/lib/core.cjs'); + + const res = core.getRoadmapPhaseInternal(dir, '2'); + assert.ok(res !== null && res !== undefined, `getRoadmapPhaseInternal returned null/undefined for phase 2`); + assert.strictEqual(res.found, true, `res.found should be true; got ${JSON.stringify(res)}`); + assert.strictEqual(res.phase_name, 'Feature', `res.phase_name should be 'Feature'; got '${res.phase_name}'`); + }); + + // ------------------------------------------------------------------------- + // Test 4 (AC3): validate health raises W006 for a current-milestone phase + // defined under (Phase Details) with no directory on disk. + // + // Before the fix, extractCurrentMilestone's slice stopped before the + // "## Milestone v1.1 — … (Phase Details)" section, so phase 2's + // "### Phase 2: Feature" header was invisible and W006 was never raised. + // After the fix the slice includes that section and W006 is emitted. + // + // This test uses its OWN local fixture (separate tmpdir) so it does not + // disturb the shared beforeEach/afterEach fixture used by tests 1–3. + // ------------------------------------------------------------------------- + test('validate health raises W006 for a started current-milestone phase defined under (Phase Details) with no directory', () => { + const localDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-test-730-t4-')); + try { + const planning = path.join(localDir, '.planning'); + fs.mkdirSync(planning, { recursive: true }); + + // STATE.md — milestone: v1.1 + fs.writeFileSync( + path.join(planning, 'STATE.md'), + `---\nmilestone: v1.1\n---\n`, + 'utf-8', + ); + + // ROADMAP.md — phase 2 is [x] (started/complete) so the not-started + // guard does NOT suppress W006. Phase 2's details live exclusively in + // the per-milestone "(Phase Details)" section (the blind-spot pre-fix). + fs.writeFileSync( + path.join(planning, 'ROADMAP.md'), + `# Roadmap: Example\n\n## Phases\n\n- [x] **Phase 1: Setup** — initial scaffold\n\n### Milestone v1.1 — Second milestone (added 2026-01-01)\n\n- [x] **Phase 2: Feature** — the new thing\n\n## Phase Details\n\n### Phase 1: Setup\n**Goal:** scaffold the app.\n\n## Milestone v1.1 — Second milestone (Phase Details)\n\n### Phase 2: Feature\n**Goal:** build the new thing.\n`, + 'utf-8', + ); + + // Create the phase 1 directory so phase 1 does NOT trigger W006. + // Phase 2 has NO directory — that's the missing-dir condition under test. + fs.mkdirSync(path.join(planning, 'phases', '01-setup'), { recursive: true }); + + const result = runGsdTools(['validate', 'health'], localDir); + const payload = JSON.parse(result.output); + const warnings = payload.warnings || []; + + // Find a W006 entry whose message references phase 2 (by number or name). + const w006ForPhase2 = warnings.find( + (w) => + w.code === 'W006' && + (/\b2\b/.test(w.message) || /\b02\b/.test(w.message) || /Feature/i.test(w.message)), + ); + + assert.ok( + w006ForPhase2 != null, + `Expected a W006 warning referencing phase 2 (Feature) — phase 2 is started ([x]) and has no directory on disk, ` + + `but its ### Phase 2: header lives in the Milestone v1.1 (Phase Details) section which was invisible before the fix. ` + + `Got warnings: ${JSON.stringify(warnings)}`, + ); + } finally { + cleanup(localDir); + } + }); + + // ------------------------------------------------------------------------- + // Test 5: three-milestone roadmap, current = latest (v1.2) + // ------------------------------------------------------------------------- + test('init phase-op resolves the latest milestone phase in a 3-milestone roadmap', () => { + const localDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-test-730-t5-')); + try { + const planning = path.join(localDir, '.planning'); + fs.mkdirSync(planning, { recursive: true }); + + fs.writeFileSync( + path.join(planning, 'STATE.md'), + `---\nmilestone: v1.2\n---\n`, + 'utf-8', + ); + + fs.writeFileSync( + path.join(planning, 'ROADMAP.md'), + `# Roadmap: Example\n\n## Phases\n\n- [x] **Phase 1: Setup** — done\n\n### Milestone v1.1 — Second (added 2026-01-01)\n\n- [x] **Phase 2: Feature** — done\n\n### Milestone v1.2 — Third (added 2026-02-01)\n\n- [ ] **Phase 3: Polish** — current\n\n## Phase Details\n\n### Phase 1: Setup\n**Goal:** scaffold.\n\n## Milestone v1.1 — Second (Phase Details)\n\n### Phase 2: Feature\n**Goal:** build.\n\n## Milestone v1.2 — Third (Phase Details)\n\n### Phase 3: Polish\n**Goal:** refine.\n`, + 'utf-8', + ); + + const r = runGsdTools('init phase-op 3', localDir); + assert.ok(r.success, `init phase-op 3 failed: ${r.error}`); + + const out = JSON.parse(r.output); + assert.strictEqual(out.phase_found, true, `phase_found should be true; got phase_found=${out.phase_found}`); + assert.strictEqual(out.phase_name, 'Polish', `phase_name should be 'Polish'; got '${out.phase_name}'`); + assert.strictEqual(out.padded_phase, '03', `padded_phase should be '03'; got '${out.padded_phase}'`); + assert.strictEqual(out.expected_phase_dir, '.planning/phases/03-polish', `expected_phase_dir should be '.planning/phases/03-polish'; got '${out.expected_phase_dir}'`); + } finally { + cleanup(localDir); + } + }); + + // ------------------------------------------------------------------------- + // Test 6: sub-milestone sharing a version prefix — closed sibling must NOT + // cross-pollinate into the active milestone's Phase Details lookup (#730) + // ------------------------------------------------------------------------- + test('init phase-op anchors Phase Details to the selected sub-milestone, not a closed same-prefix sibling', () => { + const localDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-test-730-t6-')); + try { + const planning = path.join(localDir, '.planning'); + fs.mkdirSync(planning, { recursive: true }); + + // STATE.md — milestone: v3.0 (matches v3.0-B active slice) + fs.writeFileSync( + path.join(planning, 'STATE.md'), + `---\nmilestone: v3.0\n---\n`, + 'utf-8', + ); + + // ROADMAP.md — v3.0-A is SHIPPED (closed), v3.0-B is active. + // The Phase Details for v3.0-A comes FIRST — without version-boundary + // anchoring the old code would grab it (first non-closed (Phase Details) + // heading outside the window), returning phase_name='Alpha' instead of 'Beta'. + fs.writeFileSync( + path.join(planning, 'ROADMAP.md'), + [ + '# Roadmap: Example', + '', + '## Phases', + '', + '### Milestone v3.0-A — First slice (added 2026-01-01) ✅ SHIPPED', + '', + '- [x] **Phase 1: Alpha** — done', + '', + '### Milestone v3.0-B — Second slice (added 2026-02-01)', + '', + '- [ ] **Phase 2: Beta** — current', + '', + '## Phase Details', + '', + '## Milestone v3.0-A — First slice (Phase Details)', + '', + '### Phase 1: Alpha', + '**Goal:** alpha goal.', + '', + '## Milestone v3.0-B — Second slice (Phase Details)', + '', + '### Phase 2: Beta', + '**Goal:** beta goal.', + '', + ].join('\n'), + 'utf-8', + ); + + const r = runGsdTools('init phase-op 2', localDir); + assert.ok(r.success, `init phase-op 2 failed: ${r.error}`); + + const out = JSON.parse(r.output); + assert.strictEqual(out.phase_found, true, `phase_found should be true; got phase_found=${out.phase_found}, output=${JSON.stringify(out)}`); + assert.strictEqual(out.phase_name, 'Beta', `phase_name should be 'Beta' (v3.0-B section), not '${out.phase_name}' (would indicate v3.0-A cross-pollination)`); + assert.strictEqual(out.expected_phase_dir, '.planning/phases/02-beta', `expected_phase_dir should be '.planning/phases/02-beta'; got '${out.expected_phase_dir}'`); + } finally { + cleanup(localDir); + } + }); +});