fix(#3583): one percent per write — route update-progress through the shared computation (#3634)

* test(3583): failing-first coverage for one percent per write

state update-progress computes plan throughput (summaries/plans) for stdout and the
body Progress bar, while the same write re-derives frontmatter progress.percent as
min(planFraction, phaseFraction). Neither consults the other, so mid-phase the file
contradicts itself and state json disagrees with the verb that just wrote it.

These tests fail on that: equality across stdout, body bar, frontmatter and state json
on fixtures where the two fractions differ, plus a derivation-parity test that fails if
completedPhases is ever derived by summary parity instead of verification-passed status.

Also updates three pre-existing tests that pinned stdout to the plan-throughput value
(50->0, 50->0, 100->0). Those fixtures have summarized-but-unverified phases, so the old
expectations encoded the bug; changing them IS the fix, as the issue states explicitly.

* fix(3583): one percent per write — route the verb through the shared computation

RED proven at 7dbbb2d2: 9 failures — the new cross-surface equality tests, the withhold
test, and the pre-existing tests whose expectations encoded the bug.

state update-progress computed plan throughput (summaries/plans) for stdout and the body
Progress bar, while the SAME write re-derived frontmatter progress.percent as
min(planFraction, phaseFraction) through a separate path. Neither consulted the other, so
on any project where plan throughput ran ahead of phase completion — the normal mid-phase
state — the file contradicted itself and state json disagreed with the verb that had just
written it. Exit 0, no signal.

This is not a dispute about which metric is right. The min cap is deliberate (#3242 Bug B)
and is untouched; the fix aligns the printed and body values WITH it. Verified by diff:
computeProgressPercent's definition and cmdStateSync are both unmodified.

The verb now takes its percent from buildStateFrontmatter — the single owner of the
isPhaseComplete-based completedPhases count and the ROADMAP-union totalPhases logic that
the frontmatter sync later uses inside the same read-modify-write. Both calls hit the same
disk-scan cache against the same on-disk state, so they cannot disagree. Reusing that
owner, rather than re-deriving completedPhases locally, is the point: a second
almost-identical derivation is the very defect class being fixed, and a parity test now
fails if anyone swaps it for summary parity.

The first cut fell back to plan throughput when the shared computation withheld. That
reintroduced the defect in a rarer case — stdout would print a number the frontmatter
deliberately did not contain — so it is gone. The verb now withholds in the same shape as
its existing #3217 and #3233 guards. That path is reachable, not theoretical: a bare vX.Y
token in ROADMAP prose with no versioned heading leaves the milestone unbounded while both
existing guards see a COMPLETE scope. Covered by a test that also asserts state json omits
the percent, proving it is the same withhold rather than a divergent local computation.

Three pre-existing tests pinned stdout to plan throughput (50->0, 50->0, 100->0); their
fixtures have summarized-but-unverified phases, so those expectations encoded the bug.
Updating them is the fix, as the issue states.

Fixes #3583

* fix(3583): source the reported counts from the same milestone window as the percent

The adversarial pass found the first cut left the SAME defect one field over.

cmdStateUpdateProgress still reported completed/total from the top-of-function scan,
which calls listMilestonePhaseDirs with NO versionOverride — the auto-derived current
milestone — while percent now came from buildStateFrontmatter, whose scan scopes by
versionOverride: storedMilestone. getMilestonePhaseFilter shows those can select
different milestone windows, and #3017's own comment warns about exactly that mis-bind.
So a single JSON object could report a percent inconsistent with its own counts: the
self-contradiction this issue was filed to close, relocated rather than removed.

Counts now come from the same buildStateFrontmatter result as the percent. Proven on a
real divergent-milestone fixture where a preamble phase leaks into the auto-derived scan
but is excluded from the stored-milestone-scoped one: with the fix stashed the verb emits
{percent:0, completed:1, total:2}; with it applied, {percent:0, completed:1, total:1}.
The guard scan remains, gating only the #3217/#3233 withholds.

Also corrected a comment that overstated caching. Only the phase/plan disk scan is shared
between the two buildStateFrontmatter calls; getMilestoneInfo re-reads and re-parses
ROADMAP.md and readGitHeadSha spawns a bounded git rev-parse, and both now run twice per
invocation. Threading a precomputed frontmatter through the write seam to avoid it was
rejected: that seam is the shared ADR-3408 §8.3 composition with three other callers and
heavily-documented invariants, and this is not the change to renegotiate it. The comment
now says what is and is not cached instead of implying the second call is free.

Standards: six new assertions matched raw STATE.md body text the code under test had just
produced — the pattern CONTRIBUTING bans by name. They now extract the body Progress field
with the repo's own field extractor and assert the parsed percent, so the check survives
rewording of the rendered bar. The acceptance criterion still verifies the bar; only what
it asserts on moved.

Also trimmed ~50 lines of narration around a ~15-line change into a named helper, and
fixed a stale test comment that still claimed 100% next to assertions expecting 0%.

* chore(3583): add changeset fragment

* chore(3583): backfill changeset PR number (#3634)

---------

Co-authored-by: sim <sim@local>

src/state.cts

@@ -80,7 +80,6 @@ import { tokenizeHeadings, collectSection, replaceSection } from './markdown-sec
 import type { HeadingToken } from './markdown-sectionizer.cjs';
 import { parseMarkdownTable, updateTableCell, deleteTableRow, insertTableRow, splitTableRow, isDelimiterRow } from './markdown-table.cjs';
 import { textEncodingError } from './validate.cjs';
-import { clampPercent } from './phase-lifecycle.cjs';
 // eslint-disable-next-line @typescript-eslint/no-require-imports
 import healthDiagnosticTypesMod = require('./health-diagnostic-types.cjs');
 const { SEVERITY, adviseRemedy } = healthDiagnosticTypesMod;
@@ -874,14 +873,66 @@ function cmdStateRecordMetric(cwd: string, options: StateRecordMetricOptions, ra
   output(result, raw, 'true');
 }
 
+type UpdateProgressPreview =
+  | { withheld: true; reason: string }
+  | { withheld: false; percent: number; completedPlans: number; totalPlans: number };
+
+/**
+ * #3583: computes the write-path percent AND the completed/total plan counts
+ * reported alongside it from ONE `buildStateFrontmatter` call, so
+ * `cmdStateUpdateProgress`'s JSON output cannot report a `percent` that
+ * disagrees with its own `completed`/`total` (`buildStateFrontmatter`'s
+ * `progress.{percent,completed_plans,total_plans}` all come from the same
+ * disk scan, scoped to the STORED `milestone:` frontmatter value — #3017).
+ * Re-deriving completed/total from a second, differently-scoped scan (the
+ * auto-derived one `cmdStateUpdateProgress` still runs for its own #3217/
+ * #3233 withhold gates) is what let the two disagree when the auto-derived
+ * "current" milestone differs from the stored one.
+ *
+ * Perf note: this duplicates buildStateFrontmatter's own `getMilestoneInfo`
+ * (re-reads/re-parses ROADMAP.md) and `readGitHeadSha` (a `git rev-parse`
+ * subprocess spawn) — neither is memoized, unlike the phase/plan disk scan
+ * (`_diskScanCache`), which IS shared with the second `buildStateFrontmatter`
+ * call `readModifyWriteStateMd` makes below. Both non-cached calls therefore
+ * run twice per `state update-progress`.
+ */
+function computeUpdateProgressPreview(statePath: string, cwd: string): UpdateProgressPreview {
+  const preContent = fs.readFileSync(statePath, 'utf-8');
+  const existingFm = extractFrontmatter(preContent, statePath) as Record<string, unknown>;
+  const preBody = stripFrontmatter(preContent);
+  const storedMilestone = typeof existingFm['milestone'] === 'string' ? existingFm['milestone'] : null;
+  const builtFm = buildStateFrontmatter(preBody, cwd, storedMilestone, readStoredTotalPhases(existingFm));
+  const progress = builtFm['progress'] as Record<string, unknown> | undefined;
+  const percent = progress && typeof progress['percent'] === 'number' ? progress['percent'] : null;
+  const completedPlans = progress && typeof progress['completed_plans'] === 'number' ? progress['completed_plans'] : null;
+  const totalPlans = progress && typeof progress['total_plans'] === 'number' ? progress['total_plans'] : null;
+  // A null percent is REACHABLE beyond the #3217/#3233 withholds the caller
+  // already applies — buildStateFrontmatter also nulls it via its own #1761
+  // milestone-unbounded guard, evaluated from `assertedMilestoneVersion`
+  // (an independent derivation, including a bare-version-token-in-prose
+  // fallback) rather than from `storedMilestone`/diskScope, so a STATE.md
+  // with no explicit `milestone:` field but a bare vX.Y token mentioned in
+  // ROADMAP prose can pass both of the caller's guards and still land here.
+  // Falling back to a locally-computed percent would reintroduce the exact
+  // #3583 defect for that case, so withhold instead — same shape as the
+  // caller's own no-op guards.
+  if (percent === null || completedPlans === null || totalPlans === null) {
+    return { withheld: true, reason: 'progress percent withheld by buildStateFrontmatter — STATE.md left unchanged' };
+  }
+  return { withheld: false, percent, completedPlans, totalPlans };
+}
+
 function cmdStateUpdateProgress(cwd: string, raw: boolean): void {
   const statePath = planningPaths(cwd).state;
   if (!fs.existsSync(statePath)) { output({ error: 'STATE.md not found' }, raw, undefined); return; }
 
-  // Count summaries across current milestone phases only (outside lock — read-only)
+  // Auto-derived scan across current-milestone phases (outside lock — read-only).
+  // Gates the #3217/#3233 withholds below ONLY — the reported completed/total
+  // counts come from computeUpdateProgressPreview's differently-scoped
+  // (stored-milestone) scan instead, so percent and completed/total can never
+  // disagree (#3583, finding 1).
   const phasesDir = planningPaths(cwd).phases;
   let totalPlans = 0;
-  let totalSummaries = 0;
   let phaseScope: Scope = SCOPE.UNREADABLE;
 
   {
@@ -894,9 +945,8 @@ function cmdStateUpdateProgress(cwd: string, raw: boolean): void {
     const { value: phaseDirs, scope } = listMilestonePhaseDirs(phasesDir, { cwd });
     phaseScope = scope;
     for (const dir of phaseDirs) {
-      const { planCount, summaryCount } = scanPhasePlans(path.join(phasesDir, dir));
+      const { planCount } = scanPhasePlans(path.join(phasesDir, dir));
       totalPlans += planCount;
-      totalSummaries += summaryCount;
     }
   }
 
@@ -940,15 +990,25 @@ function cmdStateUpdateProgress(cwd: string, raw: boolean): void {
     return;
   }
 
-  const percent = clampPercent(totalSummaries, totalPlans);
+  // #3583: percent AND the completed/total counts reported alongside it both
+  // come from the SAME buildStateFrontmatter call (computeUpdateProgressPreview)
+  // — never from the auto-derived scan above, which exists only to gate the
+  // #3217/#3233 withholds and is scoped differently (no stored-milestone
+  // override), so reusing its counts here could report a percent that
+  // disagrees with its own completed/total.
+  const preview = computeUpdateProgressPreview(statePath, cwd);
+  if (preview.withheld) {
+    process.stderr.write(`[gsd-tools] WARNING: state update-progress skipped — ${preview.reason}\n`);
+    output({ updated: false, reason: preview.reason }, raw, 'false');
+    return;
+  }
+  const { percent, completedPlans: fmCompletedPlans, totalPlans: fmTotalPlans } = preview;
   const barWidth = 10;
   const filled = Math.round(percent / 100 * barWidth);
   const bar = '█'.repeat(filled) + '░'.repeat(barWidth - filled);
   const progressStr = `[${bar}] ${percent}%`;
 
   let updated = false;
-  const _totalPlans = totalPlans;
-  const _totalSummaries = totalSummaries;
 
   readModifyWriteStateMd(statePath, (content) => {
     // #2177: match against the BODY only. With /i the patterns below would
@@ -981,7 +1041,7 @@ function cmdStateUpdateProgress(cwd: string, raw: boolean): void {
   }, cwd);
 
   if (updated) {
-    output({ updated: true, percent, completed: _totalSummaries, total: _totalPlans, bar: progressStr }, raw, progressStr);
+    output({ updated: true, percent, completed: fmCompletedPlans, total: fmTotalPlans, bar: progressStr }, raw, progressStr);
   } else {
     output({ updated: false, reason: 'Progress field not found in STATE.md' }, raw, 'false');
   }