diff --git a/.changeset/lucky-lynx-wave.md b/.changeset/lucky-lynx-wave.md new file mode 100644 index 000000000..f6103b3bf --- /dev/null +++ b/.changeset/lucky-lynx-wave.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3655 +--- +**`/gsd:health` no longer fires W002 for archived phase numbers in STATE.md body (#3652)** — after `/gsd:complete-milestone`, phase directories move into `milestones/vX.Y-phases/` and their `#### Phase N:` headings in ROADMAP.md are collapsed inside `
` blocks. Neither the on-disk phases scan nor the ROADMAP heading scan picked them up, so W002 fired for every archived phase number referenced in STATE.md's historical narrative body (`## Recent`, `## Decisions`, `## Deferred Items`). Projects ran permanently `degraded` with W002 noise growing proportionally to lifetime phase count. The validity set now also unions phase directories from any `milestones/vX.Y-phases/` archive, mirroring the W006 archive-lookup. Uses the shared `PHASE_TOKEN_FROM_DIR_RE` / `MILESTONE_ARCHIVE_DIR_RE` constants so project-code-prefixed dirs (e.g. `CK-64-foo`) are recognised. diff --git a/get-shit-done/bin/lib/verify.cjs b/get-shit-done/bin/lib/verify.cjs index d50884aff..218ad857a 100644 --- a/get-shit-done/bin/lib/verify.cjs +++ b/get-shit-done/bin/lib/verify.cjs @@ -413,6 +413,26 @@ function listMilestoneArchiveDirs(planBase) { } } +/** + * Walk every milestone archive directory and call `onPhase` with the phase + * token (e.g. `64`, `64A`, `64.1`) extracted from each archived phase dir's + * name. Mirrors `forEachArchivedPhaseToken` in sdk/src/query/validate.ts so + * Check 4 (W002) on the CJS side has the same archive-walking primitive. + * Bug #3652. + */ +function forEachArchivedPhaseToken(planBase, onPhase) { + for (const archiveDir of listMilestoneArchiveDirs(planBase)) { + try { + const entries = fs.readdirSync(archiveDir, { withFileTypes: true }); + for (const e of entries) { + if (!e.isDirectory()) continue; + const m = e.name.match(PHASE_TOKEN_FROM_DIR_RE); + if (m) onPhase(m[1]); + } + } catch { /* archive dir absent/unreadable */ } + } +} + function getActiveMilestoneArchiveDir(planBase) { const archiveDirs = listMilestoneArchiveDirs(planBase); if (archiveDirs.length === 0) return null; @@ -665,6 +685,14 @@ function cmdValidateHealth(cwd, options, raw) { for (const m of all) validPhases.add(m[1]); } } catch { /* intentionally empty */ } + // Bug #3652 — also union phases from every milestone archive, not only + // the active one. After /gsd:complete-milestone, historical phase dirs + // live under milestones/vX.Y-phases/ and their `#### Phase N:` headings + // get collapsed inside
blocks (which the heading regex above + // misses). collectDiskPhases() only scans the active archive, so + // without this step STATE.md's narrative references to older shipped + // phases fire false W002. + forEachArchivedPhaseToken(planBase, (token) => validPhases.add(token)); // 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 @@ -691,7 +719,7 @@ function cmdValidateHealth(cwd, options, raw) { addIssue( 'warning', 'W002', - `STATE.md references phase ${ref}, but only phases ${[...validPhases].sort().join(', ')} are declared`, + `STATE.md references phase ${ref}, but only phases ${[...validPhases].sort((a, b) => a.localeCompare(b, undefined, { numeric: true })).join(', ')} are declared`, `Review STATE.md manually before changing it; ${slash('health')} --repair will not overwrite an existing STATE.md for phase mismatches` ); } @@ -803,7 +831,12 @@ function cmdValidateHealth(cwd, options, raw) { } catch { /* intentionally empty — agent check is non-blocking */ } // ─── Check 8: Run existing consistency checks ───────────────────────────── - // Inline subset of cmdValidateConsistency + // Inline subset of cmdValidateConsistency. Note: unlike Check 4 (W002), + // this check intentionally filters ROADMAP.md through extractCurrentMilestone + // first — shipped milestones (whether collapsed in
or not) are + // stripped before the heading scan, so archived phase numbers never reach + // `roadmapPhases` and W006/W007 cannot fire for them. That is why the + // #3652 archive-union added to Check 4 is NOT mirrored here. if (fs.existsSync(roadmapPath)) { const roadmapContentRaw = fs.readFileSync(roadmapPath, 'utf-8'); const roadmapContent = extractCurrentMilestone(roadmapContentRaw, cwd); diff --git a/sdk/src/query/validate.test.ts b/sdk/src/query/validate.test.ts index 06ca231d2..501262451 100644 --- a/sdk/src/query/validate.test.ts +++ b/sdk/src/query/validate.test.ts @@ -593,6 +593,99 @@ describe('validateHealth', () => { expect(w002s).toEqual([]); }); + // Regression: #3652 — after /gsd:complete-milestone, STATE.md body retains + // historical phase references across milestones while their `#### Phase N:` + // headings in ROADMAP.md are collapsed into
blocks and the phase + // dirs are moved to `milestones/vX.Y-phases/`. The heading-scan regex misses + // collapsed phases, so W002 used to fire for every archived phase mentioned + // in narrative prose. Cross-referencing the milestones archive suppresses it. + it('does not emit W002 for phase refs that live in milestones archive (#3652)', async () => { + const planning = join(tmpDir, '.planning'); + await mkdir(join(planning, 'phases', '23-current'), { recursive: true }); + await mkdir(join(planning, 'milestones', 'v1.3a-phases', '12-old-phase'), { recursive: true }); + for (const n of ['19-alpha', '20-beta', '21-gamma', '22-delta']) { + await mkdir(join(planning, 'milestones', 'v1.3b-phases', n), { 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.3a: Shipped', '', + '- Phase 12: archived', '', + '
', '', + '
v1.3b: Shipped', '', + '- Phase 19, 20, 21, 22: archived', '', + '
', '', + '## v1.4: Current', '', + '### Phase 23: Current work', '**Goal:** stuff', '', + ].join('\n')); + await writeFile(join(planning, 'STATE.md'), [ + '---', 'milestone: v1.4', 'milestone_name: Current', 'status: executing', '---', '', + '# State', '', + '**Current Phase:** 23', '', + '## Recent', '- Phase 19 shipped', '- Phase 20 shipped', '- Phase 21 shipped', '- Phase 22 shipped', + '', '## Decisions', '- Decision from Phase 12 still applies', + '', '## Deferred Items', '- Note from Phase 19', + ].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([]); + }); + + // Regression: #3652 — project-code-prefixed archive dirs (e.g. `CK-64-...`) + // must also be recognised as valid phase declarations. The shared + // PHASE_TOKEN_FROM_DIR_RE / MILESTONE_ARCHIVE_DIR_RE constants handle the + // prefix; an ad-hoc /^\d+/-style regex here would miss them. + it('recognises project-code-prefixed archive dirs as valid for W002 (#3652)', async () => { + const planning = join(tmpDir, '.planning'); + await mkdir(join(planning, 'phases', 'CK-65-current'), { recursive: true }); + await mkdir(join(planning, 'milestones', 'v2.0-phases', 'CK-64-prior-shipped'), { 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', '', + '
v2.0: Shipped', '', + // `#### Phase 64:` lives inside
so the heading-scan + // picks it up but the on-disk active phases scan does NOT. The W006 + // archive scan must recognise `CK-64-prior-shipped` to suppress the + // warning — proving the shared regex is in use. + '#### Phase 64: Prior shipped', '', + '
', '', + '## v2.1: Current', '', + '### Phase 65: Current', '**Goal:** stuff', '', + ].join('\n')); + await writeFile(join(planning, 'STATE.md'), [ + '---', 'milestone: v2.1', 'milestone_name: Current', 'status: executing', '---', '', + '# State', '', + '**Current Phase:** 65', '', + '## Recent', '- Phase 64 shipped', + ].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([]); + // Same prefixed-archive recognition must also suppress W006 for any + // ROADMAP heading that points at the CK-prefixed archived phase, so the + // pre-existing W006 archive scan doesn't regress to the old ad-hoc regex. + const w006sForArchived = warnings.filter( + w => w.code === 'W006' && String(w.message).includes('Phase 64'), + ); + expect(w006sForArchived).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 db21706fe..ea9eb09a4 100644 --- a/sdk/src/query/validate.ts +++ b/sdk/src/query/validate.ts @@ -16,7 +16,7 @@ import { readFile, readdir, writeFile } from 'node:fs/promises'; import { existsSync } from 'node:fs'; -import { dirname, join, resolve } from 'node:path'; +import { basename, dirname, join, resolve } from 'node:path'; import { homedir } from 'node:os'; import { MODEL_PROFILES } from './config-query.js'; @@ -44,11 +44,7 @@ async function listMilestoneArchiveDirs(planBase: string): Promise { return entries .filter((e) => e.isDirectory() && MILESTONE_ARCHIVE_DIR_RE.test(e.name)) .map((e) => join(milestonesDir, e.name)) - .sort((a, b) => { - const an = a.slice(a.lastIndexOf('/') + 1); - const bn = b.slice(b.lastIndexOf('/') + 1); - return an.localeCompare(bn, undefined, { numeric: true }); - }); + .sort((a, b) => basename(a).localeCompare(basename(b), undefined, { numeric: true })); } catch { return []; } @@ -80,6 +76,28 @@ async function getActiveMilestoneArchiveDir(planBase: string): Promise void, +): Promise { + for (const archiveDir of await listMilestoneArchiveDirs(planBase)) { + try { + const entries = await readdir(archiveDir, { withFileTypes: true }); + for (const e of entries) { + if (!e.isDirectory()) continue; + const m = e.name.match(PHASE_TOKEN_FROM_DIR_RE); + if (m) onPhase(m[1]); + } + } catch { /* archive dir absent/unreadable */ } + } +} + /** * Collect the active phase roots to validate against. When the flat * `.planning/phases/` directory exists, it counts. When an active @@ -539,7 +557,7 @@ export const validateHealth: QueryHandler = async (args, projectDir, workstream) const entries = await readdir(phasesDir, { withFileTypes: true }); for (const e of entries) { if (e.isDirectory()) { - const m = e.name.match(/^(\d+[A-Z]?(?:\.\d+)*)/); + const m = e.name.match(PHASE_TOKEN_FROM_DIR_RE); if (m) validPhases.add(m[1]); } } @@ -554,6 +572,15 @@ export const validateHealth: QueryHandler = async (args, projectDir, workstream) for (const m of all) validPhases.add(m[1]); } catch { /* intentionally empty */ } + // Bug #3652 — STATE.md body retains historical phase references across + // milestones. After /gsd:complete-milestone, phases are moved into + // `milestones/vX.Y-phases/` and their `#### Phase N:` headings in + // ROADMAP.md are collapsed (e.g. inside
blocks), so neither + // the on-disk phases dir nor the ROADMAP heading scan picks them up. + // Treat any phase directory present in any archived milestone as a + // valid phase reference. + await forEachArchivedPhaseToken(planBase, (token) => validPhases.add(token)); + // 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 @@ -577,7 +604,7 @@ export const validateHealth: QueryHandler = async (args, projectDir, workstream) if (!normalizedValid.has(ref) && !normalizedValid.has(padded)) { if (normalizedValid.size > 0) { addIssue('warning', 'W002', - `STATE.md references phase ${ref}, but only phases ${[...validPhases].sort().join(', ')} are declared`, + `STATE.md references phase ${ref}, but only phases ${[...validPhases].sort((a, b) => a.localeCompare(b, undefined, { numeric: true })).join(', ')} are declared`, 'Review STATE.md manually'); } } @@ -712,7 +739,7 @@ export const validateHealth: QueryHandler = async (args, projectDir, workstream) const entries = await readdir(phasesDir, { withFileTypes: true }); for (const e of entries) { if (e.isDirectory()) { - const dm = e.name.match(/^(\d+[A-Z]?(?:\.\d+)*)/i); + const dm = e.name.match(PHASE_TOKEN_FROM_DIR_RE); if (dm) { diskPhases.add(dm[1]); activeDiskPhases.add(dm[1]); @@ -720,20 +747,9 @@ export const validateHealth: QueryHandler = async (args, projectDir, workstream) } } } catch { /* intentionally empty */ } - // Include archived milestone phase directories as valid on-disk locations - // for historical ROADMAP phases. - try { - const milestoneEntries = await readdir(join(planBase, 'milestones'), { withFileTypes: true }); - for (const milestoneEntry of milestoneEntries) { - if (!milestoneEntry.isDirectory() || !/-phases$/i.test(milestoneEntry.name)) continue; - const archivedPhaseEntries = await readdir(join(planBase, 'milestones', milestoneEntry.name), { withFileTypes: true }); - for (const archivedPhase of archivedPhaseEntries) { - if (!archivedPhase.isDirectory()) continue; - const dm = archivedPhase.name.match(/^(\d+[A-Z]?(?:\.\d+)*)/i); - if (dm) diskPhases.add(dm[1]); - } - } - } catch { /* intentionally empty */ } + // Include archived milestone phase directories as valid on-disk + // locations for historical ROADMAP phases. + await forEachArchivedPhaseToken(planBase, (token) => diskPhases.add(token)); for (const p of roadmapPhases) { const variants = phaseVariants(p); diff --git a/tests/verify-health.test.cjs b/tests/verify-health.test.cjs index c36c1e9f8..8f89b9fc3 100644 --- a/tests/verify-health.test.cjs +++ b/tests/verify-health.test.cjs @@ -201,6 +201,63 @@ describe('validate health command', () => { assert.strictEqual(w002.repairable, false, 'W002 should not be auto-repairable'); }); + // Regression: #3652 — after /gsd:complete-milestone, phase dirs move into + // milestones/vX.Y-phases/ and their `#### Phase N:` headings in ROADMAP.md + // get collapsed inside
blocks. The heading-scan regex misses + // collapsed phases and collectDiskPhases() only walks the active archive, + // so W002 used to fire for every historical phase number mentioned in + // STATE.md's narrative body. Cross-referencing every milestone archive + // suppresses the false positive. + test('does not warn W002 for phase refs that live in any milestones archive (#3652)', () => { + writeMinimalProjectMd(tmpDir); + writeValidConfigJson(tmpDir); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '23-current'), { recursive: true }); + fs.mkdirSync(path.join(tmpDir, '.planning', 'milestones', 'v1.3a-phases', '12-old-phase'), { recursive: true }); + for (const n of ['19-alpha', '20-beta', '21-gamma', '22-delta']) { + fs.mkdirSync(path.join(tmpDir, '.planning', 'milestones', 'v1.3b-phases', n), { recursive: true }); + } + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + [ + '# Roadmap', '', + '
v1.3a: Shipped', '', + '- Phase 12: archived', '', + '
', '', + '
v1.3b: Shipped', '', + '- Phase 19, 20, 21, 22: archived', '', + '
', '', + '## v1.4: Current', '', + '### Phase 23: Current work', '', + ].join('\n') + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '---', 'milestone: v1.4', 'milestone_name: Current', 'status: executing', '---', '', + '# State', '', + '**Current Phase:** 23', '', + '## Recent', '- Phase 19 shipped', '- Phase 20 shipped', '- Phase 21 shipped', '- Phase 22 shipped', + '', '## Decisions', '- Decision from Phase 12 still applies', + ].join('\n') + ); + + const result = runGsdTools('validate health', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + const w002s = (output.warnings || []).filter(w => w.code === 'W002'); + assert.strictEqual(w002s.length, 0, `Did not expect W002s for archived phases: ${JSON.stringify(w002s)}`); + // Also no W006 for the archived phases — extractCurrentMilestone strips + // shipped milestones before the Check 8 heading scan in the CJS path, + // so archived phase numbers never reach `roadmapPhases`. Pins the + // assumption that drove the decision NOT to mirror the W002 archive + // union into Check 8 on the CJS side. + const w006s = (output.warnings || []).filter(w => + w.code === 'W006' && /Phase (?:12|19|20|21|22)\b/.test(String(w.message)), + ); + assert.strictEqual(w006s.length, 0, `Did not expect W006s for archived phases: ${JSON.stringify(w006s)}`); + }); + // ─── Check 5: config.json valid JSON + valid schema ─────────────────────── test('warns when config.json is missing with repairable true', () => {