From f0bb0787c9f550c19f7ccfbaf2cfb4db98218db3 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 1 Aug 2026 11:56:34 -0400 Subject: [PATCH] fix(#2640): report truthful state_updated + keep progress frontmatter in sync after phase remove (#2974) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#2640): add regression for state_updated false positive + stale progress Three cases: (1) state_updated reflects actual content change, not just file existence; (2) progress.total_phases resync'd even when the body lacks 'Total Phases:' (the no-op guard was skipping syncStateFrontmatter); (3) state_updated is false when STATE.md doesn't exist. * fix(#2640): report truthful state_updated + force frontmatter resync Two defects in cmdPhaseRemove: 1. state_updated was fs.existsSync(statePath) — trivially true, since the file existed before and readModifyWriteStateMd never deletes it. Now captures the boolean return from readModifyWriteStateMd (changed from void to boolean: true when content was written, false on no-op). 2. progress.* frontmatter stayed stale when the body lacked 'Total Phases:' or 'of N' — readModifyWriteStateMd's no-op guard (#948) skipped syncStateFrontmatter when the body transform was unchanged. Now the transform forces a body diff when a phase was actually removed, so the guard passes and syncStateFrontmatter rebuilds progress.* from the post-deletion disk/ROADMAP state. * fix(#2640): address review — gate forced-diff on targetDir, strengthen assertions Two MAJOR findings from isolated adversarial review: 1. Forced-diff injected a spurious 'Total Phases:' line even when no directory was removed (targetDir === null). Now gated on targetDir !== null. 2. Test #2 asserted 'not 3' instead of '2' — would pass for any wrong count. Now asserts exact value. Test #1 strengthened to assert body Total Phases and frontmatter total_phases both equal 1. * chore(#2640): add changeset fragment * chore(#2640): backfill changeset PR number 2974 --------- Co-authored-by: sim --- .changeset/steady-seals-swim.md | 5 +++ src/phase.cts | 50 ++++++++++++++++++---- src/state.cts | 5 ++- tests/phase.test.cjs | 75 +++++++++++++++++++++++++++++++++ 4 files changed, 124 insertions(+), 11 deletions(-) create mode 100644 .changeset/steady-seals-swim.md diff --git a/.changeset/steady-seals-swim.md b/.changeset/steady-seals-swim.md new file mode 100644 index 000000000..e4426c9f2 --- /dev/null +++ b/.changeset/steady-seals-swim.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2974 +--- +**`phase remove` now reports accurate state_updated and keeps STATE.md progress counters in sync** — the command reported `state_updated: true` based on file existence (always true) rather than actual content change, and the frontmatter `progress.total_phases`/`completed_phases`/`percent` counters went stale when the STATE.md body lacked a `Total Phases:` field (the no-op write guard skipped the frontmatter resync). (#2640) diff --git a/src/phase.cts b/src/phase.cts index d283a4bcc..2bb5a716b 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -1598,24 +1598,56 @@ function cmdPhaseRemove( ); const statePath = path.join(planningDir(cwd), 'STATE.md'); + let stateUpdated = false; if (fs.existsSync(statePath)) { - readModifyWriteStateMd( + // #2640: report whether STATE.md content actually changed, not just file + // existence (fs.existsSync was trivially true). Also ensure the body + // transform produces a diff so readModifyWriteStateMd's no-op guard + // (#948) doesn't skip the frontmatter resync — without that, the + // progress.* frontmatter block stays stale when the body has no + // 'Total Phases:' or 'of N' phrase. + stateUpdated = readModifyWriteStateMd( statePath, (stateContent: string) => { - const totalRaw = stateExtractField(stateContent, 'Total Phases'); + let modified = stateContent; + const totalRaw = stateExtractField(modified, 'Total Phases'); if (totalRaw) { - stateContent = - stateReplaceField(stateContent, 'Total Phases', String(parseInt(totalRaw, 10) - 1)) || - stateContent; + modified = + stateReplaceField(modified, 'Total Phases', String(parseInt(totalRaw, 10) - 1)) || + modified; } - const ofMatch = stateContent.match(/(\bof\s+)(\d+)(\s*(?:\(|phases?))/i); + const ofMatch = modified.match(/(\bof\s+)(\d+)(\s*(?:\(|phases?))/i); if (ofMatch) { - stateContent = stateContent.replace( + modified = modified.replace( /(\bof\s+)(\d+)(\s*(?:\(|phases?))/i, `$1${parseInt(ofMatch[2], 10) - 1}$3`, ); } - return stateContent; + // #2640: if neither body field was found, the transform is a no-op. + // readModifyWriteStateMd's no-op guard (#948) would then skip the + // frontmatter resync, leaving progress.* stale. Force a body diff + // ONLY when a phase directory was actually removed (targetDir !== null) + // so the guard passes and syncStateFrontmatter rebuilds the frontmatter + // from the post-deletion disk/ROADMAP state. Without the targetDir gate, + // a no-op removal (ROADMAP-only phase, no directory) would inject a + // spurious 'Total Phases:' line into a body that intentionally lacked one. + if (targetDir && modified === stateContent) { + // subdirs was read before the deletion; excluding the removed target + // gives the remaining count. Renumbering changes names but not count. + const remainingPhases = subdirs.filter( + (d) => phaseTokenMatches(d, normalized) === false, + ).length; + if (totalRaw) { + modified = + stateReplaceField(modified, 'Total Phases', String(remainingPhases)) || modified; + } else { + // No 'Total Phases:' field in the body — append one so the no-op + // guard sees a diff. syncStateFrontmatter will then rebuild the + // frontmatter progress.* block from the real disk/ROADMAP count. + modified = `Total Phases: ${remainingPhases}\n` + modified; + } + } + return modified; }, cwd, ); @@ -1628,7 +1660,7 @@ function cmdPhaseRemove( renamed_directories: renamedDirs, renamed_files: renamedFiles, roadmap_updated: true, - state_updated: fs.existsSync(statePath), + state_updated: stateUpdated, }, raw, ); diff --git a/src/state.cts b/src/state.cts index 6701c9ece..c6da78f46 100644 --- a/src/state.cts +++ b/src/state.cts @@ -2213,7 +2213,7 @@ function writeStateMd(statePath: string, content: string, cwd?: string, clock?: * @param clock * Optional clock seam; defaults to realClock. Passed through to acquireStateLock. */ -function readModifyWriteStateMd(statePath: string, transformFn: (content: string) => string, cwd: string, options?: ReadModifyWriteOptions, clock?: StateLockClock): void { +function readModifyWriteStateMd(statePath: string, transformFn: (content: string) => string, cwd: string, options?: ReadModifyWriteOptions, clock?: StateLockClock): boolean { const resync = !options || options.resync !== false; const lockPath = acquireStateLock(statePath, clock); try { @@ -2261,7 +2261,7 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string // content already returns the mutated string, and callers that detect a // no-op explicitly return the original content unchanged. if (modified === content) { - return; + return false; } let synced = syncStateFrontmatter(modified, cwd, options?.authoritativeFm); @@ -2317,6 +2317,7 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string } platformWriteSync(statePath, synced); + return true; } finally { releaseStateLock(lockPath); } diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index 2a4ddb81d..0a2a67f8a 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -2755,6 +2755,81 @@ Plans: "surviving row's Plans/Status/Completed cells stay byte-identical; only its leading ordinal renumbers 3->2", ); }); + + // ─── #2640: state_updated must reflect actual content change, and progress + // frontmatter must be resync'd even when the body lacks 'Total Phases:'. ── + + test('#2640 — state_updated reflects actual content change (not just file existence)', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap\n\n### Phase 1: A\n**Goal:** x\n\n### Phase 2: B\n**Goal:** y\n`, + ); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '01-a'), { recursive: true }); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '02-b'), { recursive: true }); + // STATE.md with 'Total Phases:' body field + progress frontmatter + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + `---\ngsd_state_version: 1.0\ncurrent_phase: 1\nprogress:\n total_phases: 2\n completed_phases: 0\n percent: 0\n---\n\n# State\n\nTotal Phases: 2\n`, + ); + + const result = runGsdTools('phase remove 2', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + const out = JSON.parse(result.output); + assert.strictEqual(out.state_updated, true, 'state_updated must be true when STATE.md content changed'); + // Body 'Total Phases:' must be decremented from 2 to 1. + const afterState = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + const bodyMatch = afterState.match(/^Total Phases:\s*(\d+)/m); + assert.ok(bodyMatch, 'body must have Total Phases field after remove'); + assert.strictEqual(bodyMatch[1], '1', `body 'Total Phases:' must be 1 after removing one of 2 phases; got ${bodyMatch[1]}`); + // Frontmatter progress.total_phases must agree. + const fmMatch = afterState.match(/total_phases:\s*(\d+)/); + assert.ok(fmMatch, 'frontmatter must have total_phases'); + assert.strictEqual(fmMatch[1], '1', `frontmatter progress.total_phases must be 1; got ${fmMatch[1]}`); + }); + + test('#2640 — progress.total_phases resync\'d even when body lacks Total Phases', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap\n\n### Phase 1: A\n**Goal:** x\n\n### Phase 2: B\n**Goal:** y\n\n### Phase 3: C\n**Goal:** z\n`, + ); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '01-a'), { recursive: true }); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '02-b'), { recursive: true }); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '03-c'), { recursive: true }); + // STATE.md with NO 'Total Phases:' body field, but with progress frontmatter + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + `---\ngsd_state_version: 1.0\ncurrent_phase: 1\nprogress:\n total_phases: 3\n completed_phases: 0\n percent: 0\n---\n\n# State\n\nNo body phase count here.\n`, + ); + + const beforeState = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + const beforeMatch = beforeState.match(/total_phases:\s*(\d+)/); + assert.ok(beforeMatch && beforeMatch[1] === '3', 'precondition: total_phases should be 3'); + + const result = runGsdTools('phase remove 2', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const afterState = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + const afterMatch = afterState.match(/total_phases:\s*(\d+)/); + assert.ok(afterMatch, `STATE.md frontmatter must still have total_phases after remove; got:\n${afterState}`); + // Must be exactly 2 — 3 phases minus 1 removed. Asserting the exact value + // catches a wrong count (not just "not 3"). + assert.strictEqual(afterMatch[1], '2', + `total_phases must be exactly 2 after removing one of 3 phases; got ${afterMatch[1]}`); + }); + + test('#2640 — state_updated is false when STATE.md does not exist', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap\n\n### Phase 1: A\n**Goal:** x\n`, + ); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '01-a'), { recursive: true }); + // No STATE.md + + const result = runGsdTools('phase remove 1', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + const out = JSON.parse(result.output); + assert.strictEqual(out.state_updated, false, 'state_updated must be false when no STATE.md exists'); + }); }); // ─────────────────────────────────────────────────────────────────────────────