fix(#4094): withhold all four STATE.md progress counters under the milestone-unbounded guard (#4322)
* test(#4094): failing-first matrix for withholding all four progress counters * fix(#4094): withhold all four progress counters under the milestone-unbounded guard completed_phases/total_plans/completed_plans are accumulated from the same phaseDirs walk as total_phases, so the #3354/#3573 withhold condition makes them equally untrustworthy — yet only total_phases was withheld, and every resyncing state.* write silently clobbered the three stored siblings with the under-scoped disk numbers. Extend the withhold-then-fall-back-to-stored pattern to all three siblings: null sentinels in the disk-scan cache value, three new stored-counter readers threaded through all three buildStateFrontmatter call sites, and the same cached-else-stored consumer fallback. Milestone-bounded projects are untouched (gate-conditional). * fix(#4094): scope-requires for the new test block, keep the (#3573) warning token, and update two #3578 rows to the withheld-counter contract - the #4094 describe sat after the closing brace of the section that owned the module-level beforeEach destructure, so it needs its own local requires (mirroring the #3642 block); - the #3573 warning keeps its literal '(#3573)' tag (asserted by an existing test) with '#4094' appended as a separate token; - two #3578 status-guard rows in tests/state.test.cjs asserted the pre-#4094 unconditional disk-scan assignment of completed_phases under the roadmap-absent withhold — exactly the silent clobber #4094 removes; the status-guard conclusion (must not fire) is unchanged, the counter-value assertions now pin the withheld contract. * test(#4094): lint conformance — splitLines for the persisted-progress parser, local seeder, scoped rmSync disable * changeset(#4094) * changeset(#4094): backfill PR number --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
@@ -1787,6 +1787,337 @@ describe('#3573 total_phases — roadmap absent with an asserted milestone', ()
|
||||
});
|
||||
}
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// #4094 — the #3354 milestone-unbounded withhold only protects
|
||||
// progress.total_phases. completed_phases / total_plans / completed_plans are
|
||||
// accumulated from the SAME phaseDirs walk (same IIFE, same loop) but were
|
||||
// returned unconditionally and assigned straight from cached.* on every
|
||||
// resyncing write, silently clobbering stored values under the exact
|
||||
// condition #3354 established the scan is untrustworthy for. Extend the
|
||||
// withhold-then-fall-back-to-stored pattern to all three siblings.
|
||||
// Matrix: .gsd/bug/fix-4094-milestone-withhold-all-counters/50-test-matrix.md
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe('#4094 milestone-unbounded withhold — all four progress counters', () => {
|
||||
// Local requires: this block sits after the closing brace of the section
|
||||
// that owned the module-level beforeEach/afterEach destructure above, so
|
||||
// (mirroring the #3642 block below) everything it needs is required here.
|
||||
const { test, 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 { runNode } = require('./helpers/process-seam.cjs');
|
||||
const { TOOLS_PATH, TEST_ENV_BASE } = require('./helpers.cjs');
|
||||
const { splitLines } = require('../gsd-core/bin/lib/text-lines.cjs');
|
||||
|
||||
/** Seed `.planning/phases/<padded>-phase-<n>` — local copy (the block-scoped seeder at the top of this file is not reachable from here). */
|
||||
function seedPhaseDirs(dir, nums) {
|
||||
for (const n of nums) {
|
||||
const padded = String(n).padStart(2, '0');
|
||||
const phaseDir = path.join(dir, '.planning', 'phases', `${padded}-phase-${n}`);
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(phaseDir, `${padded}-01-PLAN.md`), '# Plan\n');
|
||||
}
|
||||
}
|
||||
|
||||
let tmpDir;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
/** The #3354/#4094 crux fixture: two milestone sections, asserted milestone matches neither. */
|
||||
function buildSectionedRoadmap() {
|
||||
const lines = ['# Roadmap', ''];
|
||||
lines.push('## Milestone v2.0 — Alpha', '');
|
||||
for (let i = 1; i <= 12; i++) lines.push(`### Phase ${i}: alpha-${i}`);
|
||||
lines.push('');
|
||||
lines.push('## Milestone v3.0 — Beta', '');
|
||||
for (let i = 13; i <= 25; i++) lines.push(`### Phase ${i}: beta-${i}`);
|
||||
lines.push('');
|
||||
return lines.join('\n');
|
||||
}
|
||||
|
||||
/**
|
||||
* STATE.md fixture with an explicit stored progress block. `counters`
|
||||
* entries that are null/undefined are OMITTED from the frontmatter block
|
||||
* (the partial-stored rows rely on that).
|
||||
*/
|
||||
function buildStateMdWithCounters({ milestone = 'v1.0', counters = {} }) {
|
||||
const { totalPhases, completedPhases, totalPlans, completedPlans } = counters;
|
||||
const fm = [
|
||||
'---',
|
||||
'gsd_state_version: 1.0',
|
||||
`milestone: ${milestone}`,
|
||||
'milestone_name: Unbounded',
|
||||
'current_phase: "01"',
|
||||
'status: executing',
|
||||
];
|
||||
const rows = [
|
||||
['total_phases', totalPhases],
|
||||
['completed_phases', completedPhases],
|
||||
['total_plans', totalPlans],
|
||||
['completed_plans', completedPlans],
|
||||
].filter(([, v]) => v !== null && v !== undefined);
|
||||
if (rows.length > 0) {
|
||||
fm.push('progress:');
|
||||
for (const [k, v] of rows) fm.push(` ${k}: ${v}`);
|
||||
}
|
||||
fm.push('---', '', '# GSD State', '', '## Current Position', '', '**Current Phase:** 01', '**Status:** Executing', '');
|
||||
return fm.join('\n');
|
||||
}
|
||||
|
||||
/** Read the PERSISTED progress block values straight out of STATE.md. */
|
||||
function persistedProgress(dir) {
|
||||
const raw = fs.readFileSync(path.join(dir, '.planning', 'STATE.md'), 'utf8');
|
||||
const lines = splitLines(raw);
|
||||
const out = {};
|
||||
let inProgress = false;
|
||||
for (const line of lines) {
|
||||
if (/^progress:\s*$/.test(line)) { inProgress = true; continue; }
|
||||
if (!inProgress) continue;
|
||||
const kv = line.match(/^ {2}(\w+): (.+)$/);
|
||||
if (!kv) break; // progress block ended
|
||||
out[kv[1]] = kv[2].trim();
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
function recordSession(dir, stoppedAt = 'Phase 1, Plan 1') {
|
||||
return runNode(
|
||||
[TOOLS_PATH, 'state', 'record-session', '--stopped-at', stoppedAt, '--resume-file', 'none'],
|
||||
{ cwd: dir, env: { ...process.env, ...TEST_ENV_BASE }, timeoutMs: 60000 },
|
||||
);
|
||||
}
|
||||
|
||||
function stateJson(dir) {
|
||||
const jsonResult = runGsdTools(['state', 'json', '--raw'], dir);
|
||||
assert.ok(jsonResult.success, `state json --raw failed: ${jsonResult.error}`);
|
||||
return JSON.parse(jsonResult.output);
|
||||
}
|
||||
|
||||
// Row 1 — the failing-first regression: all four counters preserved.
|
||||
test('#4094 stored completed_phases/total_plans/completed_plans are preserved alongside total_phases (milestoned-but-unbounded)', () => {
|
||||
fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), buildSectionedRoadmap());
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'STATE.md'),
|
||||
buildStateMdWithCounters({ counters: { totalPhases: 25, completedPhases: 9, totalPlans: 60, completedPlans: 40 } }),
|
||||
);
|
||||
seedPhaseDirs(tmpDir, [1, 2, 3, 4]);
|
||||
|
||||
const rec = recordSession(tmpDir);
|
||||
assert.ok(rec.exitCode === 0, `state record-session failed: ${rec.stderr}`);
|
||||
|
||||
const p = persistedProgress(tmpDir);
|
||||
assert.strictEqual(p.total_phases, '25', `#4094: total_phases must keep the stored 25 (existing #3354 guard)`);
|
||||
assert.strictEqual(p.completed_phases, '9', `#4094: completed_phases must keep the stored 9, not the disk-scan 0. Got ${p.completed_phases}`);
|
||||
assert.strictEqual(p.total_plans, '60', `#4094: total_plans must keep the stored 60, not the disk-scan 4. Got ${p.total_plans}`);
|
||||
assert.strictEqual(p.completed_plans, '40', `#4094: completed_plans must keep the stored 40, not the disk-scan 0. Got ${p.completed_plans}`);
|
||||
});
|
||||
|
||||
// Row 2 — the #3573 roadmap-absent sibling of the same withhold.
|
||||
test('#4094 roadmap-absent withhold preserves the three sibling counters too', () => {
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'STATE.md'),
|
||||
buildStateMdWithCounters({ counters: { totalPhases: 5, completedPhases: 2, totalPlans: 11, completedPlans: 7 } }),
|
||||
);
|
||||
seedPhaseDirs(tmpDir, [1]);
|
||||
|
||||
const rec = recordSession(tmpDir);
|
||||
assert.ok(rec.exitCode === 0, `state record-session failed: ${rec.stderr}`);
|
||||
|
||||
const p = persistedProgress(tmpDir);
|
||||
assert.strictEqual(p.total_phases, '5', `#4094: total_phases must keep the stored 5`);
|
||||
assert.strictEqual(p.completed_phases, '2', `#4094: completed_phases must keep the stored 2, not the disk-scan 0. Got ${p.completed_phases}`);
|
||||
assert.strictEqual(p.total_plans, '11', `#4094: total_plans must keep the stored 11, not the disk-scan 1. Got ${p.total_plans}`);
|
||||
assert.strictEqual(p.completed_plans, '7', `#4094: completed_plans must keep the stored 7, not the disk-scan 0. Got ${p.completed_plans}`);
|
||||
});
|
||||
|
||||
// Row 3 — nothing stored: all four keys omitted, never written from the disk scan.
|
||||
test('#4094 with nothing stored, all four counter keys are omitted', () => {
|
||||
fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), buildSectionedRoadmap());
|
||||
fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), buildStateMdWithCounters({}));
|
||||
seedPhaseDirs(tmpDir, [1, 2, 3, 4]);
|
||||
|
||||
const rec = recordSession(tmpDir);
|
||||
assert.ok(rec.exitCode === 0, `state record-session failed: ${rec.stderr}`);
|
||||
|
||||
const p = persistedProgress(tmpDir);
|
||||
for (const key of ['total_phases', 'completed_phases', 'total_plans', 'completed_plans']) {
|
||||
assert.ok(!(key in p), `#4094: with nothing stored, '${key}' must be omitted — never written from the disk scan. Got ${JSON.stringify(p)}`);
|
||||
}
|
||||
});
|
||||
|
||||
// Row 4 — partial stored block: stored ones preserved, unstated ones omitted.
|
||||
test('#4094 partial stored block: stored counters preserved, absent counters omitted', () => {
|
||||
fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), buildSectionedRoadmap());
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'STATE.md'),
|
||||
buildStateMdWithCounters({ counters: { totalPhases: 25, totalPlans: 60 } }),
|
||||
);
|
||||
seedPhaseDirs(tmpDir, [1, 2, 3, 4]);
|
||||
|
||||
const rec = recordSession(tmpDir);
|
||||
assert.ok(rec.exitCode === 0, `state record-session failed: ${rec.stderr}`);
|
||||
|
||||
const p = persistedProgress(tmpDir);
|
||||
assert.strictEqual(p.total_phases, '25', `#4094: stored total_phases preserved`);
|
||||
assert.strictEqual(p.total_plans, '60', `#4094: stored total_plans preserved. Got ${p.total_plans}`);
|
||||
assert.ok(!('completed_phases' in p), `#4094: unstored completed_phases must be omitted. Got ${JSON.stringify(p)}`);
|
||||
assert.ok(!('completed_plans' in p), `#4094: unstored completed_plans must be omitted. Got ${JSON.stringify(p)}`);
|
||||
});
|
||||
|
||||
// Rows 5-8 — boundary hold-1/hold/hold+1 per counter: the gate is boolean,
|
||||
// so preservation must not depend on how the stored value relates to the
|
||||
// disk-scan value. Rows 5-7 are the three newly-protected counters; row 8
|
||||
// (total_phases) is the already-green control.
|
||||
const BOUNDARY_SPECS = [
|
||||
{ key: 'completed_phases', fixtureKey: 'completedPhases', disk: 0, name: 'completed_phases' },
|
||||
{ key: 'total_plans', fixtureKey: 'totalPlans', disk: 4, name: 'total_plans' },
|
||||
{ key: 'completed_plans', fixtureKey: 'completedPlans', disk: 0, name: 'completed_plans' },
|
||||
{ key: 'total_phases', fixtureKey: 'totalPhases', disk: 4, name: 'total_phases (control)' },
|
||||
];
|
||||
for (const spec of BOUNDARY_SPECS) {
|
||||
for (const delta of [-1, 0, 1]) {
|
||||
test(`#4094 boundary: ${spec.name} preserved at disk${delta >= 0 ? '+' : ''}${delta}`, () => {
|
||||
fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), buildSectionedRoadmap());
|
||||
const stored = spec.disk + delta;
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'STATE.md'),
|
||||
buildStateMdWithCounters({ counters: { [spec.fixtureKey]: stored } }),
|
||||
);
|
||||
seedPhaseDirs(tmpDir, [1, 2, 3, 4]);
|
||||
|
||||
const rec = recordSession(tmpDir);
|
||||
assert.ok(rec.exitCode === 0, `state record-session failed: ${rec.stderr}`);
|
||||
|
||||
const p = persistedProgress(tmpDir);
|
||||
assert.strictEqual(
|
||||
p[spec.key],
|
||||
String(stored),
|
||||
`#4094 boundary: ${spec.key} must keep the stored ${stored} regardless of the disk count ${spec.disk}. Got ${JSON.stringify(p)}`,
|
||||
);
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
// Row 9 — legit-decrease negative space: on a BOUNDED milestone the disk
|
||||
// scan stays authoritative and corrections still land downward.
|
||||
test('#4094 bounded milestone: disk-scan corrections still land (plan deletion)', () => {
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
['# Roadmap', '', '## Milestone v1.0', '', ...[1, 2].map((i) => `### Phase ${i}: p${i}`), ''].join('\n'),
|
||||
);
|
||||
seedPhaseDirs(tmpDir, [1, 2]);
|
||||
// One plan in phase 1, two plans in phase 2.
|
||||
fs.writeFileSync(path.join(tmpDir, '.planning', 'phases', '02-phase-2', '02-02-PLAN.md'), '# Plan\n');
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'STATE.md'),
|
||||
buildStateMdWithCounters({ counters: { totalPhases: 2, completedPhases: 0, totalPlans: 3, completedPlans: 1 } }),
|
||||
);
|
||||
|
||||
// Delete one of phase 2's plans — a legitimate decrease the scan must apply.
|
||||
fs.unlinkSync(path.join(tmpDir, '.planning', 'phases', '02-phase-2', '02-02-PLAN.md'));
|
||||
|
||||
const rec = recordSession(tmpDir);
|
||||
assert.ok(rec.exitCode === 0, `state record-session failed: ${rec.stderr}`);
|
||||
|
||||
const p = persistedProgress(tmpDir);
|
||||
assert.strictEqual(p.total_plans, '2', `#4094 negative space: bounded scan must correct total_plans 3 -> 2 after plan deletion. Got ${p.total_plans}`);
|
||||
});
|
||||
|
||||
// Row 10 — legit-decrease negative space: superseded plans still excluded on a bounded milestone.
|
||||
test('#4094 bounded milestone: superseded plans still excluded from counts', () => {
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
['# Roadmap', '', '## Milestone v1.0', '', ...[1, 2].map((i) => `### Phase ${i}: p${i}`), ''].join('\n'),
|
||||
);
|
||||
seedPhaseDirs(tmpDir, [1, 2]);
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'phases', '02-phase-2', '02-02-PLAN.md'),
|
||||
'---\nstatus: superseded\n---\n# Superseded plan\n',
|
||||
);
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'STATE.md'),
|
||||
buildStateMdWithCounters({ counters: { totalPhases: 2, completedPhases: 0, totalPlans: 3, completedPlans: 0 } }),
|
||||
);
|
||||
|
||||
const rec = recordSession(tmpDir);
|
||||
assert.ok(rec.exitCode === 0, `state record-session failed: ${rec.stderr}`);
|
||||
|
||||
const p = persistedProgress(tmpDir);
|
||||
assert.strictEqual(p.total_plans, '2', `#4094 negative space: superseded plan must drop from total_plans on a bounded milestone. Got ${p.total_plans}`);
|
||||
});
|
||||
|
||||
// Row 11 — legit-decrease negative space: phase-directory removal still corrects counters when bounded.
|
||||
test('#4094 bounded milestone: phase-directory removal still corrects counters', () => {
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
['# Roadmap', '', '## Milestone v1.0', '', ...[1, 2].map((i) => `### Phase ${i}: p${i}`), ''].join('\n'),
|
||||
);
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'STATE.md'),
|
||||
buildStateMdWithCounters({ counters: { totalPhases: 2, completedPhases: 0, totalPlans: 2, completedPlans: 0 } }),
|
||||
);
|
||||
seedPhaseDirs(tmpDir, [1, 2]);
|
||||
// Removing a FIXTURE subdirectory (a phase dir), not the temp dir itself.
|
||||
// eslint-disable-next-line local/no-raw-rmsync-in-tests -- deliberate fixture mutation inside tmpDir, not a temp-dir teardown
|
||||
fs.rmSync(path.join(tmpDir, '.planning', 'phases', '02-phase-2'), { recursive: true });
|
||||
|
||||
const rec = recordSession(tmpDir);
|
||||
assert.ok(rec.exitCode === 0, `state record-session failed: ${rec.stderr}`);
|
||||
|
||||
const p = persistedProgress(tmpDir);
|
||||
assert.strictEqual(p.total_phases, '2', `#4094 negative space: bounded roadmap keeps heading-derived total 2. Got ${p.total_phases}`);
|
||||
assert.strictEqual(p.total_plans, '1', `#4094 negative space: bounded scan must correct total_plans 2 -> 1 after phase-dir removal. Got ${p.total_plans}`);
|
||||
});
|
||||
|
||||
// Row 12 — read-surface parity: `state json` reports the preserved values.
|
||||
test('#4094 state json reports preserved counters under the withhold', () => {
|
||||
fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), buildSectionedRoadmap());
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'STATE.md'),
|
||||
buildStateMdWithCounters({ counters: { totalPhases: 25, completedPhases: 9, totalPlans: 60, completedPlans: 40 } }),
|
||||
);
|
||||
seedPhaseDirs(tmpDir, [1, 2, 3, 4]);
|
||||
|
||||
const rec = recordSession(tmpDir);
|
||||
assert.ok(rec.exitCode === 0, `state record-session failed: ${rec.stderr}`);
|
||||
|
||||
const out = stateJson(tmpDir);
|
||||
assert.strictEqual(Number(out.progress && out.progress.total_phases), 25, `#4094 json parity: total_phases 25`);
|
||||
assert.strictEqual(Number(out.progress && out.progress.completed_phases), 9, `#4094 json parity: completed_phases must report the preserved 9. Got ${JSON.stringify(out.progress)}`);
|
||||
assert.strictEqual(Number(out.progress && out.progress.total_plans), 60, `#4094 json parity: total_plans must report the preserved 60. Got ${JSON.stringify(out.progress)}`);
|
||||
assert.strictEqual(Number(out.progress && out.progress.completed_plans), 40, `#4094 json parity: completed_plans must report the preserved 40. Got ${JSON.stringify(out.progress)}`);
|
||||
});
|
||||
|
||||
// Row 13 — the `state sync` special write path keeps the stored counters too
|
||||
// (same withhold, same gate — #3573 already threads the stored total there).
|
||||
test('#4094 state sync keeps stored counters under the withhold', () => {
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'STATE.md'),
|
||||
buildStateMdWithCounters({ counters: { totalPhases: 5, completedPhases: 2, totalPlans: 11, completedPlans: 7 } }),
|
||||
);
|
||||
seedPhaseDirs(tmpDir, [1]);
|
||||
|
||||
const rec = runNode(
|
||||
[TOOLS_PATH, 'state', 'sync'],
|
||||
{ cwd: tmpDir, env: { ...process.env, ...TEST_ENV_BASE }, timeoutMs: 60000 },
|
||||
);
|
||||
assert.ok(rec.exitCode === 0, `state sync failed: ${rec.stderr}`);
|
||||
|
||||
const p = persistedProgress(tmpDir);
|
||||
assert.strictEqual(p.completed_phases, '2', `#4094: state sync must keep stored completed_phases 2. Got ${p.completed_phases}`);
|
||||
assert.strictEqual(p.total_plans, '11', `#4094: state sync must keep stored total_plans 11. Got ${p.total_plans}`);
|
||||
assert.strictEqual(p.completed_plans, '7', `#4094: state sync must keep stored completed_plans 7. Got ${p.completed_plans}`);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// #3642: hasMilestoneSectioning's >=2 threshold let a single non-matching
|
||||
|
||||
Reference in New Issue
Block a user