* fix(#501): stop flat "## Phase Details" leaking phases into active milestone extractCurrentMilestone returned `preamble + currentSection`, where the preamble (everything before the first milestone heading, only <details> stripped) could carry a flat "## Phase Details" section listing `### Phase N:` entries for ALL milestones. Those leaked into the active-milestone scope, so getMilestonePhaseFilter / buildStateFrontmatter counted the whole project (e.g. total_phases: 18) instead of the active milestone (14-18). Fix (maintainer direction: code fix, count + validate-aware): 1. core.cjs extractCurrentMilestone — strip flat phase-detail blocks (`### Phase N:` heading + body, and a "## Phase Details" heading) from the preamble. The active milestone's own phases live in currentSection, so this is safe. Fixes the count with no ROADMAP edits. 2. verify.cjs cmdValidateConsistency + cmdValidateHealth — the "phases on disk but not in ROADMAP" / W007 checks now compare disk dirs against the FULL roadmap (every milestone), not the active-milestone scope. Without this, narrowing the scope would flag every shipped phase dir as a spurious orphan (the documented side effect of the <details> workaround). Tests reproduce the real layout (flat Phase Details before milestones) and assert: state json total_phases counts only active phases; validate consistency and validate health (W007) do not flag shipped phase dirs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#501): add changeset for flat Phase Details milestone leak fix Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
5
.changeset/501-flat-phase-details-milestone-leak.md
Normal file
5
.changeset/501-flat-phase-details-milestone-leak.md
Normal file
@@ -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)
|
||||
@@ -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 <details> blocks in it (these are definitely shipped)
|
||||
const preamble = beforeMilestones.replace(/<details>[\s\S]*?<\/details>/gi, '');
|
||||
// but strip any <details> 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(/<details>[\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;
|
||||
}
|
||||
|
||||
@@ -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');
|
||||
}
|
||||
}
|
||||
|
||||
147
tests/bug-501-flat-phase-details-milestone-leak.test.cjs
Normal file
147
tests/bug-501-flat-phase-details-milestone-leak.test.cjs
Normal file
@@ -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 <details>
|
||||
* 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))}`
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user