diff --git a/.changeset/501-flat-phase-details-milestone-leak.md b/.changeset/501-flat-phase-details-milestone-leak.md new file mode 100644 index 000000000..97b0bfc08 --- /dev/null +++ b/.changeset/501-flat-phase-details-milestone-leak.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 513 +--- +**Milestone phase counts no longer leak across a flat `## Phase Details` section** — when a ROADMAP listed per-phase details in a single flat `## Phase Details` section before the milestone headings, `extractCurrentMilestone` folded every milestone's `### Phase N:` entries into the active-milestone scope, so `state json` over-counted `total_phases`/`total_plans` (the whole project instead of the active milestone). The preamble now strips flat phase-detail blocks, and `validate consistency` / `validate health` (W007) compare on-disk phase dirs against the full roadmap so shipped-milestone dirs are not flagged as orphans once the scope is correctly narrowed. (#501) diff --git a/get-shit-done/bin/lib/core.cjs b/get-shit-done/bin/lib/core.cjs index 404d4ae5f..549a0db2c 100644 --- a/get-shit-done/bin/lib/core.cjs +++ b/get-shit-done/bin/lib/core.cjs @@ -1056,8 +1056,18 @@ function extractCurrentMilestone(content, cwd) { const currentSection = content.slice(sectionStart, sectionEnd); // Also include any content before the first milestone heading (title, overview, etc.) - // but strip any
blocks in it (these are definitely shipped) - const preamble = beforeMilestones.replace(/
[\s\S]*?<\/details>/gi, ''); + // but strip any
blocks in it (these are definitely shipped) and any + // flat phase-detail blocks. A "## Phase Details"-style section before the first + // milestone heading lists `### Phase N:` entries spanning ALL milestones; left + // in the preamble they leak into the active-milestone scope and over-count + // total_phases / total_plans (#501). The active milestone's own phase content + // lives in currentSection, so stripping phase blocks from the preamble is safe. + const preamble = beforeMilestones + .replace(/
[\s\S]*?<\/details>/gi, '') + // Drop each `### Phase N:` heading and its body up to the next heading. + .replace(/^#{2,4}\s*Phase\s+[\w][\w.-]*\s*:[^\n]*(?:\n(?!#{1,6}\s)[^\n]*)*\n?/gim, '') + // Drop a now-empty flat phase-details section heading, if present. + .replace(/^#{1,4}\s*Phase Details\b[^\n]*\n?/gim, ''); return preamble + currentSection; } diff --git a/get-shit-done/bin/lib/verify.cjs b/get-shit-done/bin/lib/verify.cjs index 59f332a6c..53ae80a43 100644 --- a/get-shit-done/bin/lib/verify.cjs +++ b/get-shit-done/bin/lib/verify.cjs @@ -522,7 +522,9 @@ function cmdValidateConsistency(cwd, raw) { const roadmapContentRaw = fs.readFileSync(roadmapPath, 'utf-8'); const roadmapContent = extractCurrentMilestone(roadmapContentRaw, cwd); - // Extract phases from ROADMAP (archived milestones already stripped) + // Extract phases from the ACTIVE-milestone scope (archived milestones already + // stripped). Used for the "in ROADMAP but not on disk" check — we only require + // disk dirs for the active milestone's phases. const roadmapPhases = new Set(); const phasePattern = /#{2,4}\s*Phase\s+(\d+[A-Z]?(?:\.\d+)*)\s*:/gi; let m; @@ -530,20 +532,33 @@ function cmdValidateConsistency(cwd, raw) { roadmapPhases.add(m[1]); } + // Extract phases from the FULL ROADMAP (every milestone). Used for the + // "on disk but not in ROADMAP" orphan check: a phase dir belonging to a + // shipped milestone is expected to exist on disk and is NOT an orphan, even + // though it is absent from the active-milestone scope. Without this, narrowing + // the scope (#501) would flag every shipped phase dir as a spurious orphan. + const fullRoadmapPhases = new Set(); + const fullPhasePattern = /#{2,4}\s*Phase\s+(\d+[A-Z]?(?:\.\d+)*)\s*:/gi; + let fm; + while ((fm = fullPhasePattern.exec(roadmapContentRaw)) !== null) { + fullRoadmapPhases.add(fm[1]); + } + // Get phases on disk (flat layout + milestone-archive layout) const diskPhases = collectDiskPhases(planBase); - // Check: phases in ROADMAP but not on disk + // Check: phases in ROADMAP but not on disk (active-milestone scope) for (const p of roadmapPhases) { if (!diskPhases.has(p) && !diskPhases.has(normalizePhaseName(p))) { warnings.push(`Phase ${p} in ROADMAP.md but no directory on disk`); } } - // Check: phases on disk but not in ROADMAP + // Check: phases on disk but not in ROADMAP (compared against the FULL roadmap + // so shipped-milestone phase dirs are not flagged as orphans — #501) for (const p of diskPhases) { const unpadded = String(parseInt(p, 10)); - if (!roadmapPhases.has(p) && !roadmapPhases.has(unpadded)) { + if (!fullRoadmapPhases.has(p) && !fullRoadmapPhases.has(unpadded)) { warnings.push(`Phase ${p} exists on disk but not in ROADMAP.md`); } } @@ -883,10 +898,17 @@ function cmdValidateHealth(cwd, options, raw) { const roadmapContentRaw = fs.readFileSync(roadmapPath, 'utf-8'); const roadmapContent = extractCurrentMilestone(roadmapContentRaw, cwd); - // Build roadmapPhases (raw) and roadmapPhaseVariants (all normalized variants). - // roadmapPhases: used for W006 disk-existence check (preserve original for message). - // roadmapPhaseVariants: used for W007 roadmap-membership check. - const { roadmapPhases, roadmapPhaseVariants } = buildRoadmapPhaseVariants(roadmapContent); + // roadmapPhases (active-milestone scope): used for the W006 disk-existence + // check (preserve original for message). W006 stays scoped to the active + // milestone — we only require disk dirs for the current milestone's phases. + const { roadmapPhases } = buildRoadmapPhaseVariants(roadmapContent); + + // W007 (on-disk-but-not-in-roadmap) must check membership against the FULL + // roadmap (every milestone), not just the active-milestone scope. A phase dir + // belonging to a shipped milestone is expected on disk; flagging it as a W007 + // orphan once the scope is narrowed (#501) would be spurious noise. + const { roadmapPhaseVariants: fullRoadmapPhaseVariants } = + buildRoadmapPhaseVariants(roadmapContentRaw); // diskPhases: active phasesDir + archived milestone dirs (for W006 — archived phases // are valid on-disk locations for historical ROADMAP phases). @@ -925,7 +947,7 @@ function cmdValidateHealth(cwd, options, raw) { // so neither archived phases nor padding-mismatch phases trigger false W007. for (const p of activeDiskPhases) { const variants = phaseVariants(p); - if (![...variants].some((v) => roadmapPhaseVariants.has(v))) { + if (![...variants].some((v) => fullRoadmapPhaseVariants.has(v))) { addIssue('warning', 'W007', `Phase ${p} exists on disk but not in ROADMAP.md`, 'Add to roadmap or remove directory'); } } diff --git a/tests/bug-501-flat-phase-details-milestone-leak.test.cjs b/tests/bug-501-flat-phase-details-milestone-leak.test.cjs new file mode 100644 index 000000000..acfb4bc9a --- /dev/null +++ b/tests/bug-501-flat-phase-details-milestone-leak.test.cjs @@ -0,0 +1,147 @@ +/** + * Bug #501: extractCurrentMilestone leaks prior-milestone phases when the + * ROADMAP uses a flat shared "## Phase Details" section. + * + * extractCurrentMilestone returns `preamble + currentSection`, where the + * preamble is everything before the first milestone heading (only
+ * blocks stripped). A flat "## Phase Details" section listing every phase + * across all milestones therefore leaks its `### Phase N:` headings into the + * active-milestone scope, so getMilestonePhaseFilter / buildStateFrontmatter + * count the whole project instead of just the active milestone. + * + * Maintainer direction (triage of #501): fix in code AND make + * `validate consistency` milestone-aware so it does not flag shipped phase + * dirs as orphans once the scope is correctly narrowed. + * + * Layout under test (mirrors the real repro): + * # Roadmap + * ## Phase Details <- flat, BEFORE the first milestone heading + * ### Phase 1..3 <- shipped phases + * ## ✅ v2.0 <- shipped milestone + * ## 🚧 v3.0 (active) + * ### Phase 4..5 <- active-milestone phases + * STATE.md milestone: v3.0 → state json must report total_phases: 2. + */ + +const { test, describe, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); +const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); + +const ROADMAP = `# Roadmap + +Project overview prose that legitimately lives before the milestones. + +## Phase Details + +### Phase 1: Shipped One +Did a thing. + +### Phase 2: Shipped Two +Did another thing. + +### Phase 3: Shipped Three +Did a third thing. + +## ✅ v2.0: Foundation (shipped) + +Summary of the shipped milestone. + +## 🚧 v3.0: Active Milestone + +### Phase 4: Active One +Doing a thing. + +### Phase 5: Active Two +Doing another thing. +`; + +const STATE = `--- +gsd_state_version: 1.0 +milestone: v3.0 +milestone_name: Active Milestone +status: in_progress +progress: + total_phases: 2 + completed_phases: 0 + total_plans: 0 + completed_plans: 0 + percent: 0 +--- + +# Project State + +## Current Position + +Phase: 4 (Active One) +`; + +describe('flat "## Phase Details" milestone leak (#501)', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + const planning = path.join(tmpDir, '.planning'); + fs.writeFileSync(path.join(planning, 'ROADMAP.md'), ROADMAP, 'utf-8'); + fs.writeFileSync(path.join(planning, 'STATE.md'), STATE, 'utf-8'); + fs.writeFileSync(path.join(planning, 'config.json'), '{}', 'utf-8'); + // All five phase dirs exist on disk (the flat layout retains shipped dirs). + const phaseDirs = ['01-shipped-one', '02-shipped-two', '03-shipped-three', '04-active-one', '05-active-two']; + for (const d of phaseDirs) { + const dir = path.join(planning, 'phases', d); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, '01-PLAN.md'), '# Plan\n', 'utf-8'); + } + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('state json counts only the active milestone phases, not the flat Phase Details list', () => { + const result = runGsdTools(['state', 'json'], tmpDir); + assert.equal(result.success, true, result.error || result.output); + const state = JSON.parse(result.output); + assert.equal( + state.progress.total_phases, + 2, + `active milestone v3.0 has 2 phases (4,5); flat Phase Details (1-3) must not leak. Got total_phases=${state.progress.total_phases}` + ); + }); + + test('validate consistency does not flag shipped phase dirs as not-in-ROADMAP', () => { + // Once milestone scope is correctly narrowed (Test A), the shipped phase + // dirs (1-3) are no longer in the SCOPED roadmap. They are, however, real + // phases listed in the FULL roadmap, so they must NOT be reported as + // "exists on disk but not in ROADMAP" orphans. (#501 — validate must be + // milestone-aware.) + const result = runGsdTools(['validate', 'consistency'], tmpDir); + const payload = JSON.parse(result.output); + const warnings = payload.warnings || []; + const orphanWarnings = warnings.filter((w) => /exists on disk but not in ROADMAP/i.test(w)); + assert.deepEqual( + orphanWarnings, + [], + `shipped phase dirs (1-3) are in the full ROADMAP and must not be flagged as orphans. Got: ${JSON.stringify(orphanWarnings)}` + ); + }); + + test('validate health (W007) does not flag shipped phase dirs as not-in-ROADMAP', () => { + // cmdValidateHealth's Check 8 has the same coupling: its W007 membership + // check compared active disk phases against the active-milestone scope. + // Shipped phase dirs in the active phases/ dir must be checked against the + // FULL roadmap so they are not false W007 orphans. (#501) + const result = runGsdTools(['validate', 'health'], tmpDir); + const payload = JSON.parse(result.output); + const warnings = payload.warnings || []; + const w007Orphans = warnings.filter( + (w) => w.code === 'W007' && /exists on disk but not in ROADMAP/i.test(w.message) + ); + assert.deepEqual( + w007Orphans, + [], + `shipped phase dirs (1-3) must not produce W007. Got: ${JSON.stringify(w007Orphans.map((w) => w.message))}` + ); + }); +});