From 7b470f2625b2b59233231e20097a7659cb6b1585 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 24 Apr 2026 18:11:12 -0400 Subject: [PATCH] fix(#2633): ROADMAP.md is the authority for current-milestone phase counts (#2665) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#2633): use ROADMAP.md as authority for current-milestone phase counts initMilestoneOp (SDK + CJS) derives phase_count and completed_phases from the current milestone section of ROADMAP.md instead of counting on-disk `.planning/phases/` directories. After `phases clear` at the start of a new milestone the on-disk set is a subset of the roadmap, causing premature `all_phases_complete: true`. validateHealth W002 now unions ROADMAP.md phase declarations (all milestones — current, shipped, backlog) with on-disk dirs when checking STATE.md phase refs. Eliminates false positives for future-phase refs in the current milestone and history-phase refs from shipped milestones. Falls back to legacy on-disk counting when ROADMAP.md is missing or unparseable so no-roadmap fixtures still work. Adds vitest regressions for both handlers; all 66 SDK + 118 CJS tests pass. * fix(#2633): preserve full phase tokens in W002 + completion lookup CodeRabbit flagged that the parseInt-based normalization collapses distinct phase IDs (3, 3A, 3.1) into the same integer bucket, masking real STATE/ROADMAP mismatches and miscounting completions in milestones with inserted/sub-phases. Index disk dirs and validate STATE.md refs by canonical full phase token — strip leading zeros from the integer head only, preserve [A-Z] suffix and dotted segments, and accept just the leading-zero variant of the integer prefix as a tolerated alias. 3A and 3 never share a bucket. Also widens the disk and STATE.md regexes to accept [A-Z]? suffix tokens. --- get-shit-done/bin/lib/init.cjs | 62 ++++++++++++++++++++++++++++--- get-shit-done/bin/lib/verify.cjs | 49 +++++++++++++++++++----- sdk/src/query/init.test.ts | 45 ++++++++++++++++++++++ sdk/src/query/init.ts | 64 +++++++++++++++++++++++++++++--- sdk/src/query/validate.test.ts | 43 +++++++++++++++++++++ sdk/src/query/validate.ts | 49 ++++++++++++++++++++---- 6 files changed, 282 insertions(+), 30 deletions(-) diff --git a/get-shit-done/bin/lib/init.cjs b/get-shit-done/bin/lib/init.cjs index 33bf6231..2570dcf3 100644 --- a/get-shit-done/bin/lib/init.cjs +++ b/get-shit-done/bin/lib/init.cjs @@ -827,20 +827,70 @@ function cmdInitMilestoneOp(cwd, raw) { let phaseCount = 0; let completedPhases = 0; const phasesDir = path.join(planningDir(cwd), 'phases'); + + // Bug #2633 — ROADMAP.md (current milestone section) is the authority for + // phase counts, NOT the on-disk `.planning/phases/` directory. After + // `phases clear` between milestones, on-disk dirs will be a subset of the + // roadmap until each phase is materialized; reading from disk causes + // `all_phases_complete: true` to fire prematurely. + let roadmapPhaseNumbers = []; + try { + const roadmapPath = path.join(planningDir(cwd), 'ROADMAP.md'); + const roadmapRaw = fs.readFileSync(roadmapPath, 'utf-8'); + const currentSection = extractCurrentMilestone(roadmapRaw, cwd); + const phasePattern = /#{2,4}\s*Phase\s+(\d+[A-Z]?(?:\.\d+)*)\s*:/gi; + let m; + while ((m = phasePattern.exec(currentSection)) !== null) { + roadmapPhaseNumbers.push(m[1]); + } + } catch { /* intentionally empty */ } + + // Canonicalize a phase token by stripping leading zeros from the integer + // head while preserving any [A-Z]? suffix and dotted segments. So "03" → + // "3", "03A" → "3A", "03.1" → "3.1", "3A" → "3A". Disk dirs that pad + // ("03-alpha") then match roadmap tokens ("Phase 3") without ever + // collapsing distinct tokens like "3" / "3A" / "3.1" into the same bucket. + const canonicalizePhase = (tok) => { + const m = tok.match(/^(\d+)([A-Z]?(?:\.\d+)*)$/); + return m ? String(parseInt(m[1], 10)) + m[2] : tok; + }; + const diskPhaseDirs = new Map(); try { const entries = fs.readdirSync(phasesDir, { withFileTypes: true }); - const dirs = entries.filter(e => e.isDirectory()).map(e => e.name); - phaseCount = dirs.length; + for (const e of entries) { + if (!e.isDirectory()) continue; + const m = e.name.match(/^(\d+[A-Z]?(?:\.\d+)*)/); + if (!m) continue; + diskPhaseDirs.set(canonicalizePhase(m[1]), e.name); + } + } catch { /* intentionally empty */ } - // Count phases with summaries (completed) - for (const dir of dirs) { + if (roadmapPhaseNumbers.length > 0) { + phaseCount = roadmapPhaseNumbers.length; + for (const num of roadmapPhaseNumbers) { + const dirName = diskPhaseDirs.get(canonicalizePhase(num)); + if (!dirName) continue; try { - const phaseFiles = fs.readdirSync(path.join(phasesDir, dir)); + const phaseFiles = fs.readdirSync(path.join(phasesDir, dirName)); const hasSummary = phaseFiles.some(f => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md'); if (hasSummary) completedPhases++; } catch { /* intentionally empty */ } } - } catch { /* intentionally empty */ } + } else { + // Fallback: no parseable ROADMAP — preserve legacy on-disk behavior. + try { + const entries = fs.readdirSync(phasesDir, { withFileTypes: true }); + const dirs = entries.filter(e => e.isDirectory()).map(e => e.name); + phaseCount = dirs.length; + for (const dir of dirs) { + try { + const phaseFiles = fs.readdirSync(path.join(phasesDir, dir)); + const hasSummary = phaseFiles.some(f => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md'); + if (hasSummary) completedPhases++; + } catch { /* intentionally empty */ } + } + } catch { /* intentionally empty */ } + } // Check archive const archiveDir = path.join(planningRoot(cwd), 'archive'); diff --git a/get-shit-done/bin/lib/verify.cjs b/get-shit-done/bin/lib/verify.cjs index 227223a1..e5d9dddc 100644 --- a/get-shit-done/bin/lib/verify.cjs +++ b/get-shit-done/bin/lib/verify.cjs @@ -591,28 +591,57 @@ function cmdValidateHealth(cwd, options, raw) { } else { const stateContent = fs.readFileSync(statePath, 'utf-8'); // Extract phase references from STATE.md - const phaseRefs = [...stateContent.matchAll(/[Pp]hase\s+(\d+(?:\.\d+)*)/g)].map(m => m[1]); - // Get disk phases - const diskPhases = new Set(); + const phaseRefs = [...stateContent.matchAll(/[Pp]hase\s+(\d+[A-Z]?(?:\.\d+)*)/g)].map(m => m[1]); + // Bug #2633 — ROADMAP.md is the authority for which phases are valid. + // STATE.md may legitimately reference current-milestone future phases + // (not yet materialized on disk) and shipped-milestone history phases + // (archived / cleared off disk). Matching only against on-disk dirs + // produces false W002 warnings in both cases. + const validPhases = new Set(); try { const entries = fs.readdirSync(phasesDir, { withFileTypes: true }); for (const e of entries) { if (e.isDirectory()) { - const m = e.name.match(/^(\d+(?:\.\d+)*)/); - if (m) diskPhases.add(m[1]); + const m = e.name.match(/^(\d+[A-Z]?(?:\.\d+)*)/); + if (m) validPhases.add(m[1]); } } } catch { /* intentionally empty */ } + // Union in every phase declared anywhere in ROADMAP.md (current + shipped + backlog). + try { + if (fs.existsSync(roadmapPath)) { + const roadmapRaw = fs.readFileSync(roadmapPath, 'utf-8'); + const all = [...roadmapRaw.matchAll(/#{2,4}\s*Phase\s+(\d+[A-Z]?(?:\.\d+)*)/gi)]; + for (const m of all) validPhases.add(m[1]); + } + } catch { /* intentionally empty */ } + // Compare canonical full phase tokens. Also accept a leading-zero variant + // on the integer prefix only (e.g. "03" matching "3", "03.1" matching + // "3.1") so historic STATE.md formatting still validates. Suffix tokens + // like "3A" must match exactly — never collapsed to "3". + const normalizedValid = new Set(); + for (const p of validPhases) { + normalizedValid.add(p); + const dotIdx = p.indexOf('.'); + const head = dotIdx === -1 ? p : p.slice(0, dotIdx); + const tail = dotIdx === -1 ? '' : p.slice(dotIdx); + if (/^\d+$/.test(head)) { + normalizedValid.add(head.padStart(2, '0') + tail); + } + } // Check for invalid references for (const ref of phaseRefs) { - const normalizedRef = String(parseInt(ref, 10)).padStart(2, '0'); - if (!diskPhases.has(ref) && !diskPhases.has(normalizedRef) && !diskPhases.has(String(parseInt(ref, 10)))) { - // Only warn if phases dir has any content (not just an empty project) - if (diskPhases.size > 0) { + const dotIdx = ref.indexOf('.'); + const head = dotIdx === -1 ? ref : ref.slice(0, dotIdx); + const tail = dotIdx === -1 ? '' : ref.slice(dotIdx); + const padded = /^\d+$/.test(head) ? head.padStart(2, '0') + tail : ref; + if (!normalizedValid.has(ref) && !normalizedValid.has(padded)) { + // Only warn if we know any valid phases (not just an empty project) + if (normalizedValid.size > 0) { addIssue( 'warning', 'W002', - `STATE.md references phase ${ref}, but only phases ${[...diskPhases].sort().join(', ')} exist`, + `STATE.md references phase ${ref}, but only phases ${[...validPhases].sort().join(', ')} are declared`, 'Review STATE.md manually before changing it; /gsd:health --repair will not overwrite an existing STATE.md for phase mismatches' ); } diff --git a/sdk/src/query/init.test.ts b/sdk/src/query/init.test.ts index 8973ef96..02b96374 100644 --- a/sdk/src/query/init.test.ts +++ b/sdk/src/query/init.test.ts @@ -447,6 +447,51 @@ describe('initMilestoneOp', () => { expect(data.completed_phases).toBeGreaterThanOrEqual(0); expect(data.project_root).toBe(tmpDir); }); + + // Regression: #2633 — ROADMAP.md is the authority for current-milestone + // phase count, not on-disk phase directories. After `phases clear` a new + // milestone's roadmap may list phases 3/4/5 while only 03 and 04 exist on + // disk yet. Deriving phase_count from disk yields 2 and falsely flags + // all_phases_complete=true once both on-disk phases have summaries. + it('derives phase_count from ROADMAP current milestone, not on-disk dirs (#2633)', async () => { + // Custom fixture overriding the shared beforeEach: simulate post-cleanup + // start of v1.1 where roadmap declares phases 3, 4, 5 but only 03 and 04 + // have been materialized on disk (both with summaries). + const fresh = await mkdtemp(join(tmpdir(), 'gsd-init-2633-')); + try { + await mkdir(join(fresh, '.planning', 'phases', '03-alpha'), { recursive: true }); + await mkdir(join(fresh, '.planning', 'phases', '04-beta'), { recursive: true }); + await writeFile(join(fresh, '.planning', 'config.json'), JSON.stringify({ + model_profile: 'balanced', + workflow: { nyquist_validation: true }, + })); + await writeFile(join(fresh, '.planning', 'STATE.md'), [ + '---', 'milestone: v1.1', 'milestone_name: Next', 'status: executing', '---', '', + ].join('\n')); + await writeFile(join(fresh, '.planning', 'ROADMAP.md'), [ + '# Roadmap', '', + '## v1.1: Next', + '', + '### Phase 3: Alpha', '**Goal:** A', '', + '### Phase 4: Beta', '**Goal:** B', '', + '### Phase 5: Gamma', '**Goal:** C', '', + ].join('\n')); + // Both on-disk phases have summaries (completed). + await writeFile(join(fresh, '.planning', 'phases', '03-alpha', '03-01-SUMMARY.md'), '# S'); + await writeFile(join(fresh, '.planning', 'phases', '04-beta', '04-01-SUMMARY.md'), '# S'); + + const result = await initMilestoneOp([], fresh); + const data = result.data as Record; + // Roadmap declares 3 phases for the current milestone. + expect(data.phase_count).toBe(3); + // Only 2 are materialized + summarized on disk. + expect(data.completed_phases).toBe(2); + // Therefore milestone is NOT complete — phase 5 is still outstanding. + expect(data.all_phases_complete).toBe(false); + } finally { + await rm(fresh, { recursive: true, force: true }); + } + }); }); describe('initMapCodebase', () => { diff --git a/sdk/src/query/init.ts b/sdk/src/query/init.ts index 33cee0bb..7fe2c945 100644 --- a/sdk/src/query/init.ts +++ b/sdk/src/query/init.ts @@ -26,7 +26,7 @@ import { homedir } from 'node:os'; import { loadConfig, type GSDConfig } from '../config.js'; import { resolveModel, MODEL_PROFILES } from './config-query.js'; import { findPhase } from './phase.js'; -import { roadmapGetPhase, getMilestoneInfo } from './roadmap.js'; +import { roadmapGetPhase, getMilestoneInfo, extractCurrentMilestone, extractPhasesFromSection } from './roadmap.js'; import { planningPaths, normalizePhaseName, toPosixPath, resolveAgentsDir, detectRuntime } from './helpers.js'; import { relPlanningPath } from '../workstream-utils.js'; import type { QueryHandler } from './utils.js'; @@ -780,19 +780,71 @@ export const initMilestoneOp: QueryHandler = async (_args, projectDir) => { let phaseCount = 0; let completedPhases = 0; + // Bug #2633 — ROADMAP.md (current milestone section) is the authority for + // phase counts, NOT the on-disk `.planning/phases/` directory. After + // `phases clear` between milestones, on-disk dirs will be a subset of the + // roadmap until each phase is materialized, and reading from disk causes + // `all_phases_complete: true` to fire as soon as the materialized subset + // gets summaries — even though the roadmap has phases still to do. + let roadmapPhaseNumbers: string[] = []; + try { + const { readFile } = await import('node:fs/promises'); + const roadmapRaw = await readFile(join(planningDir, 'ROADMAP.md'), 'utf-8'); + const currentSection = await extractCurrentMilestone(roadmapRaw, projectDir); + roadmapPhaseNumbers = extractPhasesFromSection(currentSection).map(p => p.number); + } catch { /* intentionally empty */ } + + // Build the on-disk index keyed by the canonical full phase token (e.g. + // "3", "3A", "3.1") so distinct tokens with the same integer prefix never + // collide. Roadmap writes "Phase 3", "Phase 3A", and "Phase 3.1" as + // distinct phases and disk dirs preserve those tokens. + // Canonicalize a phase token by stripping leading zeros from the integer + // head while preserving any [A-Z]? suffix and dotted segments. So "03" → + // "3", "03A" → "3A", "03.1" → "3.1", "3A" → "3A". This lets disk dirs that + // pad ("03-alpha") match roadmap tokens ("Phase 3") without ever collapsing + // distinct tokens like "3" / "3A" / "3.1" into the same bucket. + const canonicalizePhase = (tok: string): string => { + const m = tok.match(/^(\d+)([A-Z]?(?:\.\d+)*)$/); + return m ? String(parseInt(m[1], 10)) + m[2] : tok; + }; + const diskPhaseDirs: Map = new Map(); try { const entries = readdirSync(phasesDir, { withFileTypes: true }); - const dirs = entries.filter(e => e.isDirectory()).map(e => e.name); - phaseCount = dirs.length; + for (const e of entries) { + if (!e.isDirectory()) continue; + const m = e.name.match(/^(\d+[A-Z]?(?:\.\d+)*)/); + if (!m) continue; + diskPhaseDirs.set(canonicalizePhase(m[1]), e.name); + } + } catch { /* intentionally empty */ } - for (const dir of dirs) { + if (roadmapPhaseNumbers.length > 0) { + phaseCount = roadmapPhaseNumbers.length; + for (const num of roadmapPhaseNumbers) { + const dirName = diskPhaseDirs.get(canonicalizePhase(num)); + if (!dirName) continue; try { - const phaseFiles = readdirSync(join(phasesDir, dir)); + const phaseFiles = readdirSync(join(phasesDir, dirName)); const hasSummary = phaseFiles.some(f => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md'); if (hasSummary) completedPhases++; } catch { /* intentionally empty */ } } - } catch { /* intentionally empty */ } + } else { + // Fallback: no parseable ROADMAP (e.g. brand-new project). Preserve the + // legacy on-disk-count behavior so existing no-roadmap tests still pass. + try { + const entries = readdirSync(phasesDir, { withFileTypes: true }); + const dirs = entries.filter(e => e.isDirectory()).map(e => e.name); + phaseCount = dirs.length; + for (const dir of dirs) { + try { + const phaseFiles = readdirSync(join(phasesDir, dir)); + const hasSummary = phaseFiles.some(f => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md'); + if (hasSummary) completedPhases++; + } catch { /* intentionally empty */ } + } + } catch { /* intentionally empty */ } + } const archiveDir = join(projectDir, '.planning', 'archive'); let archivedMilestones: string[] = []; diff --git a/sdk/src/query/validate.test.ts b/sdk/src/query/validate.test.ts index bf601e4d..a233328f 100644 --- a/sdk/src/query/validate.test.ts +++ b/sdk/src/query/validate.test.ts @@ -550,6 +550,49 @@ describe('validateHealth', () => { expect(w003!.repairable).toBe(true); }); + // Regression: #2633 — W002 must consult ROADMAP.md (current + shipped + // milestones) for valid phase numbers, not only on-disk phase dirs. After + // `phases clear` at the start of a new milestone, STATE.md can legitimately + // reference future phases (current milestone) and history phases (shipped + // milestones) that no longer have a corresponding disk directory. + it('does not emit W002 for roadmap-valid future or history phase refs (#2633)', async () => { + const planning = join(tmpDir, '.planning'); + await mkdir(join(planning, 'phases', '03-alpha'), { recursive: true }); + await mkdir(join(planning, 'phases', '04-beta'), { recursive: true }); + + await writeFile(join(planning, 'PROJECT.md'), '# Project\n\n## What This Is\n\nA project.\n\n## Core Value\n\nValue here.\n\n## Requirements\n\n- Req 1\n'); + await writeFile(join(planning, 'ROADMAP.md'), [ + '# Roadmap', '', + '## v1.0: Shipped ✅ SHIPPED', '', + '### Phase 1: Origin', '**Goal:** O', '', + '### Phase 2: Continuation', '**Goal:** C', '', + '## v1.1: Current', '', + '### Phase 3: Alpha', '**Goal:** A', '', + '### Phase 4: Beta', '**Goal:** B', '', + '### Phase 5: Gamma', '**Goal:** C', '', + ].join('\n')); + await writeFile(join(planning, 'STATE.md'), [ + '---', 'milestone: v1.1', 'milestone_name: Current', 'status: executing', '---', '', + '# State', '', + '**Current Phase:** 4', + '**Next:** Phase 5', + '', + '## Accumulated Context', + '- Decision from Phase 1', + '- Follow-up from Phase 2', + ].join('\n')); + await writeFile(join(planning, 'config.json'), JSON.stringify({ + model_profile: 'balanced', + workflow: { nyquist_validation: true }, + }, null, 2)); + + const result = await validateHealth([], tmpDir); + const data = result.data as Record; + const warnings = data.warnings as Array>; + const w002s = warnings.filter(w => w.code === 'W002'); + expect(w002s).toEqual([]); + }); + it('returns warning W005 for bad phase directory naming', async () => { await createHealthyPlanning(); await mkdir(join(tmpDir, '.planning', 'phases', 'bad_name'), { recursive: true }); diff --git a/sdk/src/query/validate.ts b/sdk/src/query/validate.ts index a9a5bb54..c770dff9 100644 --- a/sdk/src/query/validate.ts +++ b/sdk/src/query/validate.ts @@ -432,24 +432,57 @@ export const validateHealth: QueryHandler = async (args, projectDir, _workstream } else { try { const stateContent = await readFile(statePath, 'utf-8'); - const phaseRefs = [...stateContent.matchAll(/[Pp]hase\s+(\d+(?:\.\d+)*)/g)].map(m => m[1]); - const diskPhases = new Set(); + const phaseRefs = [...stateContent.matchAll(/[Pp]hase\s+(\d+[A-Z]?(?:\.\d+)*)/g)].map(m => m[1]); + + // Bug #2633 — ROADMAP.md is the authority for which phases are valid. + // STATE.md may legitimately reference current-milestone future phases + // (not yet materialized on disk) and shipped-milestone history phases + // (archived / cleared off disk). Matching only against on-disk dirs + // produces false W002 warnings in both cases. + const validPhases = new Set(); try { const entries = await readdir(phasesDir, { withFileTypes: true }); for (const e of entries) { if (e.isDirectory()) { - const m = e.name.match(/^(\d+(?:\.\d+)*)/); - if (m) diskPhases.add(m[1]); + const m = e.name.match(/^(\d+[A-Z]?(?:\.\d+)*)/); + if (m) validPhases.add(m[1]); } } } catch { /* intentionally empty */ } + // Union in every phase declared anywhere in ROADMAP.md — current milestone, + // shipped milestones (inside
/ ✅ SHIPPED sections), and any + // preamble/Backlog. We deliberately do NOT filter by current milestone. + try { + const roadmapRaw = await readFile(roadmapPath, 'utf-8'); + const all = [...roadmapRaw.matchAll(/#{2,4}\s*Phase\s+(\d+[A-Z]?(?:\.\d+)*)/gi)]; + for (const m of all) validPhases.add(m[1]); + } catch { /* intentionally empty */ } + + // Compare canonical full phase tokens. Also accept a leading-zero + // variant on the integer prefix only (e.g. "03" → "3", "03.1" → "3.1") + // so historic STATE.md formatting still validates. Suffix tokens like + // "3A" must match exactly — never collapsed to "3". + const normalizedValid = new Set(); + for (const p of validPhases) { + normalizedValid.add(p); + const dotIdx = p.indexOf('.'); + const head = dotIdx === -1 ? p : p.slice(0, dotIdx); + const tail = dotIdx === -1 ? '' : p.slice(dotIdx); + if (/^\d+$/.test(head)) { + normalizedValid.add(head.padStart(2, '0') + tail); + } + } + for (const ref of phaseRefs) { - const normalizedRef = String(parseInt(ref, 10)).padStart(2, '0'); - if (!diskPhases.has(ref) && !diskPhases.has(normalizedRef) && !diskPhases.has(String(parseInt(ref, 10)))) { - if (diskPhases.size > 0) { + const dotIdx = ref.indexOf('.'); + const head = dotIdx === -1 ? ref : ref.slice(0, dotIdx); + const tail = dotIdx === -1 ? '' : ref.slice(dotIdx); + const padded = /^\d+$/.test(head) ? head.padStart(2, '0') + tail : ref; + if (!normalizedValid.has(ref) && !normalizedValid.has(padded)) { + if (normalizedValid.size > 0) { addIssue('warning', 'W002', - `STATE.md references phase ${ref}, but only phases ${[...diskPhases].sort().join(', ')} exist`, + `STATE.md references phase ${ref}, but only phases ${[...validPhases].sort().join(', ')} are declared`, 'Review STATE.md manually'); } }