fix(#1445,#1446): exclude 999.x backlog from milestone totals; allow total_phases downward correction (#1490)
* 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 <noreply@anthropic.com> * chore: add changeset for #1445/#1446 progress-backlog-exclusion-and-ratchet Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(#1445,#1446): rename test files to fix-NNN convention; fix changeset pr: null Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -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.
|
||||
@@ -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
|
||||
|
||||
@@ -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 */ }
|
||||
|
||||
|
||||
@@ -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'));
|
||||
}
|
||||
|
||||
@@ -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 */ }
|
||||
|
||||
174
tests/fix-1445-999x-backlog-excluded-from-total-phases.test.cjs
Normal file
174
tests/fix-1445-999x-backlog-excluded-from-total-phases.test.cjs
Normal file
@@ -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}`,
|
||||
);
|
||||
});
|
||||
});
|
||||
156
tests/fix-1446-total-phases-corrects-downward.test.cjs
Normal file
156
tests/fix-1446-total-phases-corrects-downward.test.cjs
Normal file
@@ -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}`,
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user