From 4719b36d4165317805860b1f079d058b27f599bf Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 20 Jun 2026 13:37:32 -0400 Subject: [PATCH] fix(#1445,#1446): exclude 999.x backlog from milestone totals; allow total_phases downward correction (#1490) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#1445,#1446): exclude 999.x backlog phases from milestone totals; allow total_phases downward correction #1445: deriveProgressFromRoadmap (phase-lifecycle.cts), the roadmapPhaseCount loop (state.cts), and getMilestonePhaseFilter (roadmap-parser.cts) all now skip phase tokens matching /^999\b/ — consistent with the existing init.cts filter. 999.x backlog dirs are consequently excluded from phaseDirs too. #1446: shouldPreserveExistingProgress (state-document.cts) no longer includes total_phases in its ratchet check. total_phases always takes the freshly derived value; only completed_phases, total_plans, and completed_plans retain ratchet behaviour. Regression tests added for both bugs. Co-Authored-By: Claude Sonnet 4.6 * chore: add changeset for #1445/#1446 progress-backlog-exclusion-and-ratchet Co-Authored-By: Claude Sonnet 4.6 * fix(#1445,#1446): rename test files to fix-NNN convention; fix changeset pr: null Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- ...-progress-backlog-exclusion-and-ratchet.md | 6 + src/phase-lifecycle.cts | 14 +- src/roadmap-parser.cts | 3 +- src/state-document.cts | 6 +- src/state.cts | 3 +- ...acklog-excluded-from-total-phases.test.cjs | 174 ++++++++++++++++++ ...46-total-phases-corrects-downward.test.cjs | 156 ++++++++++++++++ 7 files changed, 354 insertions(+), 8 deletions(-) create mode 100644 .changeset/fix-1445-1446-progress-backlog-exclusion-and-ratchet.md create mode 100644 tests/fix-1445-999x-backlog-excluded-from-total-phases.test.cjs create mode 100644 tests/fix-1446-total-phases-corrects-downward.test.cjs diff --git a/.changeset/fix-1445-1446-progress-backlog-exclusion-and-ratchet.md b/.changeset/fix-1445-1446-progress-backlog-exclusion-and-ratchet.md new file mode 100644 index 000000000..9f4d8f62d --- /dev/null +++ b/.changeset/fix-1445-1446-progress-backlog-exclusion-and-ratchet.md @@ -0,0 +1,6 @@ +--- +type: Fixed +pr: 1490 +--- + +**999.x backlog phases are now excluded from `total_phases`, and `total_phases` can correct downward** — `deriveProgressFromRoadmap` counted all progress-table rows whose phase cell started with a digit, so a `999.1 Backlog` row inflated `total_phases` by one per entry (#1445). The same overcounting occurred in `getMilestonePhaseFilter` (which feeds `isDirInMilestone` and `phaseDirs`) and in the `roadmapPhaseCount` loop in `buildStateFrontmatter`. All three sites now filter phase tokens matching `/^999\b/`, consistent with the existing exclusion in `init.cts`. Additionally, `shouldPreserveExistingProgress` included `total_phases` in its ratchet check, preventing the counter from decreasing once set too high — e.g. after a 999.x fix or a ROADMAP correction (#1446). `total_phases` is now always taken from the freshly derived value; only `completed_phases`, `total_plans`, and `completed_plans` retain ratchet behaviour. diff --git a/src/phase-lifecycle.cts b/src/phase-lifecycle.cts index 93cc8997a..2175cd400 100644 --- a/src/phase-lifecycle.cts +++ b/src/phase-lifecycle.cts @@ -54,10 +54,16 @@ export function deriveProgressFromRoadmap(roadmapContent: string): RoadmapProgre ); if (progressTableMatch) { const tableText = progressTableMatch[0]; - // Count data rows (rows starting with pipe then a phase number) - const dataRowPattern = /^\|\s*\d+/gm; - const dataRows = tableText.match(dataRowPattern); - totalPhases = dataRows ? dataRows.length : null; + // Count data rows (rows starting with pipe then a phase number), + // excluding 999.x backlog phases. Mirrors init.cts /^999(?:\.|$)/ filter. + const dataRowPattern = /^\|\s*(\d+[^|]*)\|/gm; + let dataRowCount = 0; + let drm: RegExpExecArray | null; + while ((drm = dataRowPattern.exec(tableText)) !== null) { + if (/^999\b/.test(drm[1].trim())) continue; + dataRowCount++; + } + totalPhases = dataRowCount > 0 ? dataRowCount : null; } // Sum plan counts from M/N columns in progress table diff --git a/src/roadmap-parser.cts b/src/roadmap-parser.cts index f6a17fea8..76e96b3ba 100644 --- a/src/roadmap-parser.cts +++ b/src/roadmap-parser.cts @@ -408,7 +408,8 @@ function getMilestonePhaseFilter(cwd: string, versionOverride?: string | null, p for (const h of tokenizeHeadings(roadmap)) { if (h.level < 2 || h.level > 4) continue; const pm = phaseHeadingPattern.exec(h.text); - if (pm) milestonePhaseNums.add(pm[1]); + // Exclude 999.x backlog phases from milestone phase set. Mirrors init.cts filter. + if (pm && !/^999\b/.test(pm[1])) milestonePhaseNums.add(pm[1]); } } catch { /* intentionally empty */ } diff --git a/src/state-document.cts b/src/state-document.cts index cb9accd20..e7f0e114b 100644 --- a/src/state-document.cts +++ b/src/state-document.cts @@ -171,8 +171,10 @@ export function shouldPreserveExistingProgress(existingProgress: unknown, derive return false; const existing = existingProgress as ProgressRecord; const derived = derivedProgress as ProgressRecord; - return (existingProgressExceedsDerived(existing, derived, 'total_phases') || - existingProgressExceedsDerived(existing, derived, 'completed_phases') || + // total_phases is intentionally excluded from the ratchet: it must always + // take the freshly derived value so it can correct downward (#1446). + // Only completed_phases, total_plans, and completed_plans keep ratchet behaviour. + return (existingProgressExceedsDerived(existing, derived, 'completed_phases') || existingProgressExceedsDerived(existing, derived, 'total_plans') || existingProgressExceedsDerived(existing, derived, 'completed_plans')); } diff --git a/src/state.cts b/src/state.cts index 448ef41fe..14d819ffa 100644 --- a/src/state.cts +++ b/src/state.cts @@ -1402,7 +1402,8 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined): Re // Only count tokens that contain at least one digit — excludes // pure-word section headings (Overview, Details) while keeping // numeric phases (01, 05.1) and project-code IDs (PROJ-42). - if (/\d/.test(m[1])) roadmapPhaseCount++; + // Also exclude 999.x backlog phases. Mirrors init.cts filter. + if (/\d/.test(m[1]) && !/^999\b/.test(m[1])) roadmapPhaseCount++; } } } catch { /* fall through: phaseDirs.length used as sole count */ } diff --git a/tests/fix-1445-999x-backlog-excluded-from-total-phases.test.cjs b/tests/fix-1445-999x-backlog-excluded-from-total-phases.test.cjs new file mode 100644 index 000000000..1490db251 --- /dev/null +++ b/tests/fix-1445-999x-backlog-excluded-from-total-phases.test.cjs @@ -0,0 +1,174 @@ +'use strict'; +/** + * Regression test for bug #1445: + * 999.x backlog phases must not be counted toward total_phases. + * + * Root cause: + * deriveProgressFromRoadmap (phase-lifecycle.cts) counted ALL data rows + * matching /^\|\s*\d+/ in the progress table, including 999.x backlog rows. + * Similarly, state.cts's roadmapPhaseCount loop (via extractCurrentMilestone) + * counted 999.x phase headings because it only checked /\d/.test(m[1]). + * + * Fix: + * Both sites now test /^999(?:\.|$)/.test(token) and skip matching rows. + * Mirrors the existing init.cts /^999(?:\.|$)/ filter. + * + * Scenarios: + * A. deriveProgressFromRoadmap with a progress table containing a 999.x row. + * B. state json total_phases via extractCurrentMilestone / roadmapPhaseCount. + */ + +const { describe, test, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); +const { deriveProgressFromRoadmap } = require('../gsd-core/bin/lib/phase-lifecycle.cjs'); + +// ─── Scenario A: deriveProgressFromRoadmap unit test ──────────────────────── + +describe('bug #1445 — deriveProgressFromRoadmap excludes 999.x rows', () => { + test('3 real phases + 1 999.x backlog row → total_phases: 3, not 4', () => { + const roadmap = [ + '## Milestone v1.0: Test', + '', + '| Phase | Plans | Status | Completed |', + '| --- | --- | --- | --- |', + '| 1. Alpha | 2/2 | Complete | ✅ |', + '| 2. Beta | 1/2 | In Progress | |', + '| 3. Gamma | 0/1 | Planned | |', + '| 999.1 Backlog: Future Idea | 0/0 | Backlog | |', + ].join('\n'); + + const result = deriveProgressFromRoadmap(roadmap); + assert.equal( + result.totalPhases, + 3, + `total_phases must be 3 (not 4) — 999.1 backlog row must be excluded. Got ${result.totalPhases}`, + ); + assert.equal( + result.completedPhases, + 1, + `completed_phases must be 1. Got ${result.completedPhases}`, + ); + }); + + test('999 exact (no dot) row is also excluded', () => { + const roadmap = [ + '## Milestone v1.0: Test', + '', + '| Phase | Plans | Status | Completed |', + '| --- | --- | --- | --- |', + '| 1. Alpha | 1/1 | Complete | ✅ |', + '| 2. Beta | 1/1 | Complete | ✅ |', + '| 999 Backlog | 0/0 | Backlog | |', + ].join('\n'); + + const result = deriveProgressFromRoadmap(roadmap); + assert.equal( + result.totalPhases, + 2, + `total_phases must be 2 (not 3) — 999 row must be excluded. Got ${result.totalPhases}`, + ); + assert.equal( + result.completedPhases, + 2, + `completed_phases must be 2. Got ${result.completedPhases}`, + ); + }); + + test('all-backlog table yields null total_phases (no real phases)', () => { + const roadmap = [ + '## Milestone v1.0: Test', + '', + '| Phase | Plans | Status | Completed |', + '| --- | --- | --- | --- |', + '| 999.1 Future A | 0/0 | Backlog | |', + '| 999.2 Future B | 0/0 | Backlog | |', + ].join('\n'); + + const result = deriveProgressFromRoadmap(roadmap); + assert.equal( + result.totalPhases, + null, + `total_phases must be null when the only rows are 999.x backlog. Got ${result.totalPhases}`, + ); + }); +}); + +// ─── Scenario B: state json total_phases via roadmapPhaseCount ─────────────── + +describe('bug #1445 — state json excludes 999.x phase headings from total_phases', () => { + let tmpDir; + + const ROADMAP = [ + '## Milestone v1.0: Test Milestone', + '', + '### Phase 01: Alpha', + '**Goal:** first', + '', + '### Phase 02: Beta', + '**Goal:** second', + '', + '### Phase 03: Gamma', + '**Goal:** third', + '', + '### Phase 999.1: Backlog Item A', + '**Goal:** future idea, not counted', + '', + '### Phase 999.2: Backlog Item B', + '**Goal:** another future idea', + ].join('\n'); + + beforeEach(() => { + tmpDir = createTempProject('bug-1445-'); + const planning = path.join(tmpDir, '.planning'); + fs.writeFileSync(path.join(planning, 'ROADMAP.md'), ROADMAP, 'utf-8'); + fs.writeFileSync( + path.join(planning, 'STATE.md'), + [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'status: executing', + '---', + '', + '# GSD State', + '', + '## Configuration', + 'Current Phase: 1', + 'Status: Executing Phase 1', + 'Last Activity: 2026-01-01', + ].join('\n'), + 'utf-8', + ); + fs.writeFileSync(path.join(planning, 'config.json'), '{}', 'utf-8'); + + for (const d of ['01-alpha', '02-beta', '03-gamma']) { + const dir = path.join(planning, 'phases', d); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, 'PLAN.md'), '# Plan\n', 'utf-8'); + } + // 999.x dirs should exist on disk but must not inflate total_phases + for (const d of ['999.1-backlog-a', '999.2-backlog-b']) { + fs.mkdirSync(path.join(planning, 'phases', d), { recursive: true }); + } + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('state json total_phases is 3, not 5 (999.x dirs and headings excluded)', () => { + const result = runGsdTools(['state', 'json'], tmpDir); + assert.ok(result.success, `state json failed: ${result.error}`); + const state = JSON.parse(result.output); + assert.ok(state.progress, 'state json must return a progress block'); + assert.equal( + state.progress.total_phases, + 3, + `total_phases must be 3 (not 5). 999.x backlog phases must be excluded. Got ${state.progress.total_phases}`, + ); + }); +}); diff --git a/tests/fix-1446-total-phases-corrects-downward.test.cjs b/tests/fix-1446-total-phases-corrects-downward.test.cjs new file mode 100644 index 000000000..ae0c72bf8 --- /dev/null +++ b/tests/fix-1446-total-phases-corrects-downward.test.cjs @@ -0,0 +1,156 @@ +'use strict'; +/** + * Regression test for bug #1446: + * total_phases must correct downward when re-derived; shouldPreserveExistingProgress + * must NOT include total_phases in its ratchet check. + * + * Root cause: + * shouldPreserveExistingProgress (state-document.cts) returned true when + * existingProgress.total_phases > derivedProgress.total_phases, making the + * stored value sticky even when it was wrong (e.g. counted backlog phases). + * + * Fix: + * total_phases is removed from the "existing exceeds derived" check. + * Only completed_phases, total_plans, and completed_plans keep ratchet behaviour. + * + * Scenarios: + * A. shouldPreserveExistingProgress unit test — returns false when only total_phases differs. + * B. state sync re-derives a lower total_phases and writes the new value. + */ + +const { describe, test, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); +const { shouldPreserveExistingProgress } = require('../gsd-core/bin/lib/state-document.cjs'); + +// ─── Scenario A: unit test ─────────────────────────────────────────────────── + +describe('bug #1446 — shouldPreserveExistingProgress does not ratchet total_phases', () => { + test('existing total_phases:10 > derived total_phases:7 → returns false (no ratchet)', () => { + const existing = { total_phases: 10, completed_phases: 3, total_plans: 6, completed_plans: 3 }; + const derived = { total_phases: 7, completed_phases: 3, total_plans: 6, completed_plans: 3 }; + assert.equal( + shouldPreserveExistingProgress(existing, derived), + false, + 'total_phases downward correction must NOT trigger shouldPreserveExistingProgress', + ); + }); + + test('existing completed_phases:5 > derived completed_phases:2 → returns true (ratchet still active)', () => { + const existing = { total_phases: 7, completed_phases: 5, total_plans: 6, completed_plans: 3 }; + const derived = { total_phases: 7, completed_phases: 2, total_plans: 6, completed_plans: 3 }; + assert.equal( + shouldPreserveExistingProgress(existing, derived), + true, + 'completed_phases ratchet must still work', + ); + }); + + test('existing total_phases:10 > derived:7 AND completed_phases matches → false (total_phases alone does not preserve)', () => { + const existing = { total_phases: 10, completed_phases: 3 }; + const derived = { total_phases: 7, completed_phases: 3 }; + assert.equal( + shouldPreserveExistingProgress(existing, derived), + false, + 'only-total_phases discrepancy must not trigger preservation', + ); + }); + + test('all derived values equal existing → returns false', () => { + const existing = { total_phases: 7, completed_phases: 3, total_plans: 6, completed_plans: 3 }; + const derived = { total_phases: 7, completed_phases: 3, total_plans: 6, completed_plans: 3 }; + assert.equal(shouldPreserveExistingProgress(existing, derived), false); + }); +}); + +// ─── Scenario B: end-to-end state sync overwrites inflated total_phases ────── + +describe('bug #1446 — state sync writes corrected (lower) total_phases', () => { + let tmpDir; + + // ROADMAP has 3 real phases only (no 999.x). + const ROADMAP = [ + '## Milestone v1.0: Test', + '', + '### Phase 01: Alpha', + '**Goal:** alpha', + '', + '### Phase 02: Beta', + '**Goal:** beta', + '', + '### Phase 03: Gamma', + '**Goal:** gamma', + ].join('\n'); + + beforeEach(() => { + tmpDir = createTempProject('bug-1446-'); + const planning = path.join(tmpDir, '.planning'); + fs.writeFileSync(path.join(planning, 'ROADMAP.md'), ROADMAP, 'utf-8'); + + // STATE.md has a stale inflated total_phases:10 in frontmatter. + fs.writeFileSync( + path.join(planning, 'STATE.md'), + [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'status: executing', + 'progress:', + ' total_phases: 10', + ' completed_phases: 2', + ' total_plans: 6', + ' completed_plans: 4', + ' percent: 40', + '---', + '', + '# GSD State', + '', + '## Configuration', + 'Current Phase: 3', + 'Status: Executing Phase 3', + 'Last Activity: 2026-01-01', + 'Progress: [████░░░░░░] 40%', + ].join('\n'), + 'utf-8', + ); + fs.writeFileSync(path.join(planning, 'config.json'), '{}', 'utf-8'); + + for (const d of ['01-alpha', '02-beta', '03-gamma']) { + const dir = path.join(planning, 'phases', d); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, 'PLAN.md'), '# Plan\n', 'utf-8'); + // Mark 01 and 02 as complete (2 summaries) + if (d !== '03-gamma') { + fs.writeFileSync(path.join(dir, 'PLAN-SUMMARY.md'), '# Summary\n', 'utf-8'); + } + } + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('state sync corrects total_phases from 10 to 3', () => { + const syncResult = runGsdTools(['state', 'sync'], tmpDir); + assert.ok(syncResult.success, `state sync failed: ${syncResult.error}`); + + const jsonResult = runGsdTools(['state', 'json'], tmpDir); + assert.ok(jsonResult.success, `state json failed: ${jsonResult.error}`); + const state = JSON.parse(jsonResult.output); + + assert.ok(state.progress, 'state json must return a progress block'); + assert.equal( + state.progress.total_phases, + 3, + `total_phases must be corrected to 3 (derived), not kept at 10 (stale). Got ${state.progress.total_phases}`, + ); + // completed_phases ratchet still works: existing 2 ≥ disk-derived → keep 2 + assert.ok( + state.progress.completed_phases >= 2, + `completed_phases must be at least 2 (ratchet). Got ${state.progress.completed_phases}`, + ); + }); +});