fix(#2640): report truthful state_updated + keep progress frontmatter in sync after phase remove (#2974)

* 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 <sim@local>

.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)

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 =
+          modified =
-            stateReplaceField(stateContent, 'Total Phases', String(parseInt(totalRaw, 10) - 1)) ||
+            stateReplaceField(modified, 'Total Phases', String(parseInt(totalRaw, 10) - 1)) ||
-            stateContent;
+            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,
   );

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);
   }

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');
+  });
 });
 
 // ─────────────────────────────────────────────────────────────────────────────