fix(state): preserve curated progress on body-only updates; correct percent formula (#3242) (#3252)

* test: reproduce state.update progress trampling and percent formula (#3242)

Two failing regression tests:
- Bug A: state.update "Last Activity" tramples curated progress.* frontmatter via readModifyWriteStateMd → syncStateFrontmatter
- Bug B: 12 declared ROADMAP phases / 6 realized / 6/6 plans done → percent: 100 instead of 50 (phase-fraction ignored)

* fix(state): preserve curated progress on body-only updates; correct percent formula (#3242)

Bug A: readModifyWriteStateMd now accepts { resync: false } to preserve existing
frontmatter progress.* when only body text changes. cmdStateUpdate passes this flag
since it only replaces a body field and must not trample manually-curated
cross-milestone counters.

Bug B: extract computeProgressPercent() helper — shared by buildStateFrontmatter and
cmdStateSync — that applies min(plan_fraction, phase_fraction). When ROADMAP declares
more phases than are realized on disk, phase_fraction caps percent so 22/22 plans
done with only 6/12 phases gives 50%, not a false 100%.

* changeset: pr=3252 for #3242

* fix(test): replace content.includes with structured state json assertion (#3242)

.changeset/gentle-bears-wave.md

@@ -0,0 +1,5 @@
+---
+type: Fixed
+pr: 3252
+---
+**`state.update <field>` no longer rebuilds the progress.* block from disk on body-only updates** — manually-curated cross-milestone counters are preserved. Also: progress.percent now reflects the lower of plan-fraction and phase-fraction so milestones with un-planned future phases don't show false 100%.

get-shit-done/bin/lib/state.cjs

@@ -13,6 +13,32 @@ const { extractFrontmatter, reconstructFrontmatter } = require('./frontmatter.cj
 // hasn't changed within the same gsd-tools invocation.
 const _diskScanCache = new Map();
 
+/**
+ * Compute the canonical progress percent for STATE.md frontmatter and body.
+ *
+ * Uses min(plan_fraction, phase_fraction) when both denominators are > 0.
+ * This prevents a false "100%" when ROADMAP declares future phases that have no
+ * disk dirs yet — all plans summarised only means 100% of *realized* work done,
+ * not 100% of the declared milestone (#3242 Bug B).
+ *
+ * @param {number|null} completedPlans
+ * @param {number|null} totalPlans
+ * @param {number|null} completedPhases
+ * @param {number|null} totalPhases  - ROADMAP-declared count (>= realised dirs)
+ * @returns {number|null}  0–100, or null when there is no data
+ */
+function computeProgressPercent(completedPlans, totalPlans, completedPhases, totalPhases) {
+  const hasPlanData = totalPlans !== null && totalPlans > 0 && completedPlans !== null;
+  const hasPhaseData = totalPhases !== null && totalPhases > 0 && completedPhases !== null;
+
+  if (!hasPlanData && !hasPhaseData) return null;
+
+  const planFraction = hasPlanData ? completedPlans / totalPlans : 1;
+  const phaseFraction = hasPhaseData ? completedPhases / totalPhases : 1;
+
+  return Math.min(100, Math.round(Math.min(planFraction, phaseFraction) * 100));
+}
+
 /** Shorthand — every state command needs this path */
 function getStatePath(cwd) {
   return planningPaths(cwd).state;
@@ -202,6 +228,9 @@ function cmdStateUpdate(cwd, field, value) {
   const statePath = planningPaths(cwd).state;
   try {
     let updated = false;
+    // resync: false — cmdStateUpdate only replaces a body text line.
+    // Triggering syncStateFrontmatter would rebuild progress.* from disk, trampling
+    // manually-curated cross-milestone counters stored in the frontmatter (#3242 Bug A).
     readModifyWriteStateMd(statePath, (content) => {
       const fieldEscaped = escapeRegex(field);
       // Try **Field:** bold format first, then plain Field: format
@@ -215,7 +244,7 @@ function cmdStateUpdate(cwd, field, value) {
         return content.replace(plainPattern, (_match, prefix) => `${prefix}${value}`);
       }
       return content;
-    }, cwd);
+    }, cwd, { resync: false });
     if (updated) {
       output({ updated: true });
     } else {
@@ -813,13 +842,12 @@ function buildStateFrontmatter(bodyContent, cwd) {
   }
 
   // Derive percent from disk counts when available (ground truth).
-  // Only falls back to the body Progress: field when no plan files exist on disk
-  // (phases directory empty or absent), which means disk has no authoritative data.
-  // This prevents a stale body "0%" from overriding the real 100% completion state.
-  let progressPercent = null;
-  if (totalPlans !== null && totalPlans > 0 && completedPlans !== null) {
-    progressPercent = Math.min(100, Math.round(completedPlans / totalPlans * 100));
-  } else if (progressRaw) {
+  // Uses min(plan_fraction, phase_fraction) via computeProgressPercent so that
+  // ROADMAP-declared-but-unrealized future phases cap the reported completion
+  // instead of a false 100% from plan-only coverage (#3242 Bug B).
+  // Falls back to the body Progress: field only when no plan files exist on disk.
+  let progressPercent = computeProgressPercent(completedPlans, totalPlans, completedPhases, totalPhases);
+  if (progressPercent === null && progressRaw) {
     const pctMatch = progressRaw.match(/(\d+)%/);
     if (pctMatch) progressPercent = parseInt(pctMatch[1], 10);
   }
@@ -971,13 +999,43 @@ function writeStateMd(statePath, content, cwd) {
  * Holds the lock across the entire read -> transform -> write cycle,
  * preventing the lost-update problem where two agents read the same
  * content and the second write clobbers the first.
+ *
+ * @param {string} statePath
+ * @param {function} transformFn - (content: string) => string
+ * @param {string} cwd
+ * @param {{ resync?: boolean }} [options]
+ *   resync: when true (default) rebuilds the entire frontmatter from disk after
+ *   the transform. Pass { resync: false } for body-only updates (e.g. state.update
+ *   on a single field) that must not trample manually-curated cross-milestone
+ *   progress.* counters in the frontmatter (#3242 Bug A).
+ *   When resync is false, syncStateFrontmatter still runs to maintain/create the
+ *   frontmatter block, but any existing progress.* sub-keys are preserved from
+ *   the pre-transform file rather than being rebuilt from disk.
  */
-function readModifyWriteStateMd(statePath, transformFn, cwd) {
+function readModifyWriteStateMd(statePath, transformFn, cwd, options) {
+  const resync = !options || options.resync !== false;
   const lockPath = acquireStateLock(statePath);
   try {
     const content = fs.existsSync(statePath) ? fs.readFileSync(statePath, 'utf-8') : '';
+    // Snapshot the existing progress block BEFORE the transform so we can
+    // restore it when resync is false.
+    const preFm = resync ? null : extractFrontmatter(content);
     const modified = transformFn(content);
-    const synced = syncStateFrontmatter(modified, cwd);
+    let synced = syncStateFrontmatter(modified, cwd);
+
+    if (!resync && preFm && preFm.progress) {
+      // Re-apply the curated progress block that syncStateFrontmatter just
+      // overwrote with disk-derived values.  Only restore keys that were present
+      // in the snapshot — this preserves any new non-progress frontmatter fields
+      // (e.g., status, current_phase) that syncStateFrontmatter legitimately
+      // derived from the updated body.
+      const postFm = extractFrontmatter(synced);
+      postFm.progress = preFm.progress;
+      const yamlStr = reconstructFrontmatter(postFm);
+      const body = stripFrontmatter(synced);
+      synced = `---\n${yamlStr}\n---\n\n${body}`;
+    }
+
     atomicWriteFileSync(statePath, normalizeMd(synced), 'utf-8');
   } finally {
     releaseStateLock(lockPath);
@@ -1449,6 +1507,7 @@ function cmdStateSync(cwd, options, raw) {
 
   let totalDiskPlans = 0;
   let totalDiskSummaries = 0;
+  let diskCompletedPhases = 0;
   let highestIncompletePhase = null;
   let highestIncompletePhaseNum = null;
   let highestIncompletePhaseplanCount = 0;
@@ -1461,6 +1520,7 @@ function cmdStateSync(cwd, options, raw) {
     const summaries = files.filter(f => f.match(/-SUMMARY\.md$/i)).length;
     totalDiskPlans += plans;
     totalDiskSummaries += summaries;
+    if (plans > 0 && summaries >= plans) diskCompletedPhases++;
 
     // Track the highest phase with incomplete plans (or any plans)
     const phaseMatch = dir.match(/^(\d+[A-Z]?(?:\.\d+)*)/i);
@@ -1481,6 +1541,18 @@ function cmdStateSync(cwd, options, raw) {
     }
   }
 
+  // Determine total phases from ROADMAP (may be larger than realized disk dirs).
+  // Mirrors the logic in buildStateFrontmatter so both report consistent percents (#3242 Bug B).
+  let syncTotalPhases = null;
+  try {
+    const isDirInMilestone = getMilestonePhaseFilter(cwd);
+    if (isDirInMilestone.phaseCount > 0) {
+      syncTotalPhases = Math.max(entries.length, isDirInMilestone.phaseCount);
+    } else {
+      syncTotalPhases = entries.length;
+    }
+  } catch { /* intentionally empty */ }
+
   // Sync Total Plans in Phase
   if (highestIncompletePhase) {
     const currentPlansField = stateExtractField(modified, 'Total Plans in Phase');
@@ -1491,8 +1563,13 @@ function cmdStateSync(cwd, options, raw) {
     }
   }
 
-  // Sync Progress
-  const percent = totalDiskPlans > 0 ? Math.min(100, Math.round(totalDiskSummaries / totalDiskPlans * 100)) : 0;
+  // Sync Progress — use shared helper so formula stays in one place (#3242 Bug B).
+  // computeProgressPercent applies min(plan_fraction, phase_fraction) so unrealised
+  // ROADMAP phases cap the reported percent rather than allowing a false 100%.
+  const percent = (() => {
+    const p = computeProgressPercent(totalDiskSummaries, totalDiskPlans, diskCompletedPhases, syncTotalPhases);
+    return p !== null ? p : 0;
+  })();
   const currentProgress = stateExtractField(modified, 'Progress');
   if (currentProgress) {
     const currentPercent = parseInt(currentProgress.replace(/[^\d]/g, ''), 10);

tests/bug-3242-state-update-progress-trample.test.cjs

@@ -0,0 +1,378 @@
+'use strict';
+// Regression tests for issue #3242 — two distinct bugs in state.cjs:
+//
+// Bug A: cmdStateUpdate("Last Activity", date) triggers a full disk-derived
+// progress.* block rebuild via readModifyWriteStateMd → syncStateFrontmatter →
+// buildStateFrontmatter, which tramples manually-curated cross-milestone counters
+// in STATE.md frontmatter. A body-only field update must not modify progress.*.
+//
+// Bug B: buildStateFrontmatter (and the duplicate in cmdStateSync) derives
+// progress.percent = completedPlans / totalPlans. When ROADMAP declares more
+// phases than have dirs on disk, all plans being summarised gives percent: 100
+// even though half the phases are unrealised. The formula must be
+// min(plan_fraction, phase_fraction) to reflect true completion.
+
+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');
+
+// ─────────────────────────────────────────────────────────────────────────────
+// Helpers
+// ─────────────────────────────────────────────────────────────────────────────
+
+/**
+ * Parse the STATE.md frontmatter `progress:` block into a numeric map.
+ * Returns { total_phases, completed_phases, total_plans, completed_plans, percent }
+ * sourced directly from the persisted frontmatter YAML (not rebuilt from disk).
+ *
+ * Only reads the --- block; throws if no closed --- block is found.
+ * Uses structural parsing (line splitting) rather than regex on the whole file.
+ */
+function parsePersistedProgress(content) {
+  const lines = content.split('\n');
+  let inFm = false;
+  let inProgress = false;
+  const result = {};
+  let fmStarted = false;
+
+  for (const line of lines) {
+    if (!fmStarted) {
+      if (line.trim() === '---') { inFm = true; fmStarted = true; }
+      continue;
+    }
+    if (line.trim() === '---') break; // end of frontmatter
+    if (line === 'progress:') { inProgress = true; continue; }
+    if (inProgress) {
+      const m = line.match(/^\s{2}([a-z_]+):\s*(\d+)/);
+      if (m) {
+        result[m[1]] = parseInt(m[2], 10);
+      } else if (line.trim() !== '' && !/^\s/.test(line)) {
+        inProgress = false; // left progress block
+      }
+    }
+  }
+  return result;
+}
+
+/**
+ * Build a minimal STATE.md body with frontmatter that has curated progress.*.
+ * The progress values are cross-milestone aggregates that must NOT be overwritten
+ * by a body-only field update.
+ */
+function buildStateWithCuratedProgress(opts) {
+  const {
+    completedPlans = 22,
+    totalPlans = 22,
+    completedPhases = 6,
+    totalPhases = 12,
+    percent = 50,
+    lastActivity = '2026-01-01',
+  } = opts || {};
+
+  return [
+    '---',
+    'gsd_state_version: 1.0',
+    'status: executing',
+    'progress:',
+    `  total_phases: ${totalPhases}`,
+    `  completed_phases: ${completedPhases}`,
+    `  total_plans: ${totalPlans}`,
+    `  completed_plans: ${completedPlans}`,
+    `  percent: ${percent}`,
+    '---',
+    '',
+    '# GSD State',
+    '',
+    '## Configuration',
+    'Current Phase: 6',
+    'Current Phase Name: test-phase',
+    'Total Plans in Phase: 4',
+    'Current Plan: 1',
+    'Status: Executing Phase 6',
+    `Last Activity: ${lastActivity}`,
+    '',
+  ].join('\n');
+}
+
+/**
+ * Write a ROADMAP.md with `numPhases` phase headings (matching `## Phase N:` pattern).
+ * Only `numRealizedDirs` phase dirs will have plan/summary files on disk.
+ */
+function buildRoadmap(numPhases) {
+  const lines = ['# ROADMAP', '', '## Milestone v1.0', ''];
+  for (let i = 1; i <= numPhases; i++) {
+    lines.push(`### Phase ${i}: phase-${i}`);
+    lines.push('');
+  }
+  return lines.join('\n');
+}
+
+/**
+ * Create phase dirs with full plan+summary coverage for the first `count` phases.
+ * Each dir gets 1 PLAN + 1 SUMMARY so the disk-scan treats them as complete.
+ */
+function createPhaseDirs(phasesDir, count) {
+  for (let i = 1; i <= count; i++) {
+    const dir = path.join(phasesDir, String(i).padStart(2, '0'));
+    fs.mkdirSync(dir, { recursive: true });
+    fs.writeFileSync(path.join(dir, `01-PLAN.md`), `# Plan\n`);
+    fs.writeFileSync(path.join(dir, `01-SUMMARY.md`), `# Summary\n`);
+  }
+}
+
+// ─────────────────────────────────────────────────────────────────────────────
+// Bug A: state.update must not trample curated progress.* frontmatter
+// ─────────────────────────────────────────────────────────────────────────────
+
+describe('#3242 Bug A: body-only state.update preserves curated progress frontmatter', () => {
+  let tmpDir;
+
+  beforeEach(() => {
+    tmpDir = createTempProject();
+  });
+
+  afterEach(() => {
+    cleanup(tmpDir);
+  });
+
+  test('state.update "Last Activity" does not overwrite progress.completed_plans', () => {
+    const statePath = path.join(tmpDir, '.planning', 'STATE.md');
+    fs.writeFileSync(statePath, buildStateWithCuratedProgress({
+      completedPlans: 22,
+      totalPlans: 22,
+      completedPhases: 6,
+      totalPhases: 12,
+      percent: 50,
+      lastActivity: '2026-01-01',
+    }));
+
+    // Write 6 phase dirs with full coverage — disk would report 6/6 phases done,
+    // 6/6 plans done (percent=100 from plans-only formula), but frontmatter says 50%.
+    const phasesDir = path.join(tmpDir, '.planning', 'phases');
+    createPhaseDirs(phasesDir, 6);
+
+    const updateResult = runGsdTools(
+      ['state', 'update', 'Last Activity', '2026-05-07'],
+      tmpDir,
+    );
+    assert.ok(updateResult.success, `state update failed: ${updateResult.error}`);
+
+    // Read back the STATE.md file and inspect the persisted frontmatter directly.
+    // Note: state json always rebuilds from disk (correct for freshness), so we
+    // must check the file on disk to assert that state.update did not trample
+    // the curated frontmatter values (#3242 Bug A).
+    const content = fs.readFileSync(statePath, 'utf-8');
+    const progress = parsePersistedProgress(content);
+
+    assert.ok(
+      Object.keys(progress).length > 0,
+      'STATE.md frontmatter must contain a progress: block after state.update',
+    );
+
+    // completed_plans must NOT have been trampled to 6 (disk reality) from the
+    // curated 22 that was stored in the frontmatter before the update.
+    assert.strictEqual(
+      progress.completed_plans,
+      22,
+      `state.update "Last Activity" must not overwrite curated progress.completed_plans ` +
+      `(was 22, got ${progress.completed_plans})`,
+    );
+
+    // total_phases must NOT have been trampled to 6 (disk dirs) from curated 12.
+    assert.strictEqual(
+      progress.total_phases,
+      12,
+      `state.update "Last Activity" must not overwrite curated progress.total_phases ` +
+      `(was 12, got ${progress.total_phases})`,
+    );
+
+    // percent must NOT have been trampled to 100 (plan-only formula on 6 realized dirs).
+    assert.strictEqual(
+      progress.percent,
+      50,
+      `state.update "Last Activity" must not overwrite curated progress.percent ` +
+      `(was 50, got ${progress.percent})`,
+    );
+  });
+
+  test('state.update "Last Activity" updates the body field itself', () => {
+    const statePath = path.join(tmpDir, '.planning', 'STATE.md');
+    fs.writeFileSync(statePath, buildStateWithCuratedProgress({ lastActivity: '2026-01-01' }));
+
+    const updateResult = runGsdTools(
+      ['state', 'update', 'Last Activity', '2026-05-07'],
+      tmpDir,
+    );
+    assert.ok(updateResult.success, `state update failed: ${updateResult.error}`);
+
+    // Confirm the body field was indeed updated via state json (structured output).
+    // state json reads last_activity from the body — if the field wasn't updated
+    // this will return the original '2026-01-01' value.
+    const jsonResult = runGsdTools('state json', tmpDir);
+    assert.ok(jsonResult.success, `state json failed: ${jsonResult.error}`);
+    const fm = JSON.parse(jsonResult.output);
+    assert.strictEqual(
+      fm.last_activity,
+      '2026-05-07',
+      `state.update should have written '2026-05-07' to the Last Activity body field`,
+    );
+  });
+});
+
+// ─────────────────────────────────────────────────────────────────────────────
+// Bug B: progress.percent must use min(plan_fraction, phase_fraction)
+// ─────────────────────────────────────────────────────────────────────────────
+
+describe('#3242 Bug B: progress.percent reflects phase fraction when ROADMAP declares future phases', () => {
+  let tmpDir;
+
+  beforeEach(() => {
+    tmpDir = createTempProject();
+  });
+
+  afterEach(() => {
+    cleanup(tmpDir);
+  });
+
+  test('12 declared phases / 6 realized / 6/6 plans done → percent is 50, not 100', () => {
+    const statePath = path.join(tmpDir, '.planning', 'STATE.md');
+
+    // Body: 6 realized phases visible to disk scan.
+    // Frontmatter: intentionally absent so buildStateFrontmatter runs fresh.
+    fs.writeFileSync(statePath, [
+      '# GSD State',
+      '',
+      '## Configuration',
+      'Current Phase: 6',
+      'Current Phase Name: test-phase-6',
+      'Total Plans in Phase: 1',
+      'Current Plan: 1',
+      'Status: Executing Phase 6',
+      'Last Activity: 2026-01-01',
+      '',
+    ].join('\n'));
+
+    // ROADMAP with 12 phase headings — only 6 will have dirs on disk
+    fs.writeFileSync(
+      path.join(tmpDir, '.planning', 'ROADMAP.md'),
+      buildRoadmap(12),
+    );
+
+    // 6 fully-realized phases (all plans have summaries)
+    const phasesDir = path.join(tmpDir, '.planning', 'phases');
+    createPhaseDirs(phasesDir, 6);
+
+    // state json rebuilds frontmatter from disk+body — this exercises buildStateFrontmatter
+    const jsonResult = runGsdTools('state json', tmpDir);
+    assert.ok(jsonResult.success, `state json failed: ${jsonResult.error}`);
+
+    const fm = JSON.parse(jsonResult.output);
+    assert.ok(fm.progress, 'frontmatter must have a progress block');
+
+    // ROADMAP declares 12 phases; only 6 exist on disk → totalPhases = 12
+    assert.strictEqual(
+      fm.progress.total_phases,
+      12,
+      `total_phases must reflect ROADMAP-declared count (12), got ${fm.progress.total_phases}`,
+    );
+
+    // 6 of 12 phases realized → phase_fraction = 50%
+    // 6/6 plans done → plan_fraction = 100%
+    // percent = min(100, 50) = 50
+    assert.strictEqual(
+      fm.progress.percent,
+      50,
+      `percent must be 50 (phase fraction), not 100 (plan fraction) — ` +
+      `6 of 12 ROADMAP phases realized. Got ${fm.progress.percent}`,
+    );
+  });
+
+  test('all phases realized: percent equals plan fraction (no artificial cap)', () => {
+    const statePath = path.join(tmpDir, '.planning', 'STATE.md');
+
+    fs.writeFileSync(statePath, [
+      '# GSD State',
+      '',
+      '## Configuration',
+      'Current Phase: 3',
+      'Current Phase Name: final-phase',
+      'Total Plans in Phase: 1',
+      'Current Plan: 1',
+      'Status: Executing Phase 3',
+      'Last Activity: 2026-01-01',
+      '',
+    ].join('\n'));
+
+    // ROADMAP declares 3 phases; all 3 have dirs and full plan+summary coverage
+    fs.writeFileSync(
+      path.join(tmpDir, '.planning', 'ROADMAP.md'),
+      buildRoadmap(3),
+    );
+
+    const phasesDir = path.join(tmpDir, '.planning', 'phases');
+    createPhaseDirs(phasesDir, 3);
+
+    const jsonResult = runGsdTools('state json', tmpDir);
+    assert.ok(jsonResult.success, `state json failed: ${jsonResult.error}`);
+
+    const fm = JSON.parse(jsonResult.output);
+    assert.ok(fm.progress, 'frontmatter must have progress block');
+
+    // 3/3 phases done → phase_fraction = 100%
+    // 3/3 plans done → plan_fraction = 100%
+    // percent = min(100, 100) = 100
+    assert.strictEqual(
+      fm.progress.percent,
+      100,
+      `percent must be 100 when all phases are realized and all plans summarized`,
+    );
+  });
+
+  test('state sync also reflects phase-fraction-capped percent in body Progress field', () => {
+    // state sync updates the body's Progress: field — it must use the same capped formula
+    const statePath = path.join(tmpDir, '.planning', 'STATE.md');
+
+    fs.writeFileSync(statePath, [
+      '# GSD State',
+      '',
+      '## Configuration',
+      'Current Phase: 6',
+      'Current Phase Name: phase-6',
+      'Total Plans in Phase: 1',
+      'Current Plan: 1',
+      'Status: Executing Phase 6',
+      'Last Activity: 2026-01-01',
+      'Progress: [░░░░░░░░░░] 0%',
+      '',
+    ].join('\n'));
+
+    fs.writeFileSync(
+      path.join(tmpDir, '.planning', 'ROADMAP.md'),
+      buildRoadmap(12),
+    );
+
+    const phasesDir = path.join(tmpDir, '.planning', 'phases');
+    createPhaseDirs(phasesDir, 6);
+
+    const syncResult = runGsdTools('state sync', tmpDir);
+    assert.ok(syncResult.success, `state sync failed: ${syncResult.error}`);
+
+    // Read the body's Progress field via state json (JSON output is authoritative)
+    const jsonResult = runGsdTools('state json', tmpDir);
+    assert.ok(jsonResult.success, `state json failed: ${jsonResult.error}`);
+
+    const fm = JSON.parse(jsonResult.output);
+    assert.ok(fm.progress, 'frontmatter must have progress block');
+
+    // state sync wrote a Progress: body field; state json re-derives percent from disk.
+    // Both must agree: 50%, not 100%.
+    assert.strictEqual(
+      fm.progress.percent,
+      50,
+      `state sync must cap percent at phase fraction (50%), got ${fm.progress.percent}`,
+    );
+  });
+});