From e6d047decc23dfc802a0ffcc032d4a23b863bf7f Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 5 Sep 2026 23:02:07 -0400 Subject: [PATCH] fix(#4129): derive completed_phases from the ROADMAP authority; honor the progress-ratchet on every state write (#4359) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#4129): failing-first regressions — completed_phases clobber on resyncing writes and phase-complete failure to increment * fix(#4129): completed_phases derives from the ROADMAP authority and the write path honors the progress-ratchet Three coordinated prongs (diagnosis in .gsd/bug/fix-4129-completed-phases-recompute/): P1 — buildStateFrontmatter's disk scan floors the completed-phases numerator at the milestone-scoped ROADMAP Complete-row count (deriveProgressFromRoadmap, the one owner), gated inside the same safeToUseRoadmapCount / not-withheld branch that owns the denominator. A completed phase whose verification routes stale (#2348 clean-commit-time drift) or is missing no longer under-counts forever. P2 — applyPreserveAlways's resync arm merges instead of wholesale-replacing on a measured scan: totals derived both directions (#2440), completed counters up-only (#2969 — the schema-declared progress-ratchet, now enforced on the write path like the read path always has), percent recomputed from the merged counters. The #3756 unmeasured guard and the #3242 explicit-progress contract are unchanged. P3 — phase complete's atomic 3-file commit passes the post-completion ROADMAP-derived counters through the #2736 authoritativeFm seam (new object direction for the progress key; completedOnlyRaise at the post-preservation re-assert), because the transaction's disk scan reads the pre-completion ROADMAP and failed to increment on the completing phase's own write. * fix(#4129): adversarial-review hardening — intent is a floor at BOTH authoritativeFm sites The pre-preservation merge could lower a correctly-higher disk-derived counter (a verification-passed phase whose ROADMAP table row drifted behind the disk signal). completedOnlyRaise now governs both application sites: the intent and the derivation agree on direction (up), never on subtraction. * fix(#4129): the ratchet merge keeps derived values verbatim when numerically equal The re-parsed derived block carries string scalars ("2") while the curated snapshot carries numbers (2); substituting the curated spelling over an equal derived one was a no-op in substance but a shape churn the ADR-3473 §8.7 reporting loop surfaced as a phantom preserved-over-disagreeing-derived warning on phase complete (ADR-3408 §8.5 Matrix B). Only a strictly-greater curated counter replaces the derived value now; percent gets the same verbatim rule. * changeset(#4129): backfill PR 4359 --------- Co-authored-by: sim --- .changeset/humble-koalas-swim.md | 5 + src/phase.cts | 65 +++++++-- src/state-transition.cts | 129 ++++++++++++++--- src/state.cts | 125 ++++++++++++++++- tests/phase.test.cjs | 161 +++++++++++++++++++++ tests/state-transition.test.cjs | 145 ++++++++++++++++++- tests/state.test.cjs | 231 +++++++++++++++++++++++++++++++ 7 files changed, 827 insertions(+), 34 deletions(-) create mode 100644 .changeset/humble-koalas-swim.md diff --git a/.changeset/humble-koalas-swim.md b/.changeset/humble-koalas-swim.md new file mode 100644 index 000000000..4434cf98c --- /dev/null +++ b/.changeset/humble-koalas-swim.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4359 +--- +**`progress.completed_phases` and `percent` are now derived from the ROADMAP's own milestone Complete rows and never move downward on a state write** — previously every default-resync verb (`state record-session`, `add-decision`, `begin-phase`, `phase complete` itself) recomputed the counter from a disk scan that drops any completed phase whose verification reads `stale` (a summary committed or edited after it) or is missing, so the stored value was silently reverted to the under-count on every write and hand-corrections never survived. The scan now floors the numerator at the milestone-scoped ROADMAP Complete-row count (same gate and scope as the denominator), the write path enforces the schema-declared `progress-ratchet` (totals correct both directions, completed counters up-only, percent recomputed from the surviving counters), and `phase complete` passes its post-completion ROADMAP-derived counters through the transition so the completing phase's own write increments. (#4129) diff --git a/src/phase.cts b/src/phase.cts index 47e784cc8..832f345bf 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -59,6 +59,12 @@ const { findPhaseInternal, getArchivedPhaseDirs, listMilestonePhaseDirs } = phas // eslint-disable-next-line @typescript-eslint/no-require-imports -- roadmap-parser.cjs is an export= CommonJS module import roadmapParserMod = require('./roadmap-parser.cjs'); const { stripShippedMilestones, extractCurrentMilestone, currentMilestoneRawRanges, withPhaseSection, findMilestoneScopeHeadingLines } = roadmapParserMod; +// #4129: the single owner of "count the ROADMAP's milestone Complete rows" +// (pure computation, no I/O — no cycle on this path) for the intent-first +// progress counters the phase-complete transaction passes downstream. +// eslint-disable-next-line @typescript-eslint/no-require-imports -- phase-lifecycle.cjs is an export= CommonJS module +import phaseLifecycleMod = require('./phase-lifecycle.cjs'); +const { deriveProgressFromRoadmap: deriveProgressFromRoadmapForIntent, clampPercent: clampPercentForIntent } = phaseLifecycleMod; // eslint-disable-next-line @typescript-eslint/no-require-imports -- planning-workspace.cjs is an export= CommonJS module import planningWorkspace = require('./planning-workspace.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports -- frontmatter.cjs is an export= CommonJS module @@ -4395,14 +4401,57 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { const bodyHasPhaseField = stateExtractField(fmBody, 'Current Phase') != null || stateExtractField(fmBody, 'Phase') != null; - const authoritativeFm: Record | undefined = nextPhaseDisplayName - ? bodyHasPhaseField || !nextPhaseNum - ? { current_phase_name: nextPhaseDisplayName } - : { - current_phase: String(nextPhaseNum), - current_phase_name: nextPhaseDisplayName, - } - : undefined; + // #4129: the POST-completion progress counters, derived from the very + // ROADMAP this transaction just mutated (still in memory — it hits disk + // only at writePlanningFileSet, AFTER this content was assembled). + // buildStateFrontmatter's disk scan inside syncAndPreserveStateMd + // reads the PRE-completion ROADMAP (and any stale-dated sibling + // verification), so without this intent the persisted counter failed + // to increment on the completing phase's own transaction. Routed + // through the #2736 authoritativeFm seam's object direction: the + // pre-preservation merge makes it the derived truth the ratchet + // compares, and the post-preservation re-assert (completedOnlyRaise) + // is a floor no preservation branch can drop below. clampPercent is + // completePhaseCore's own percent formula (state-transition.cts), + // reused so the frontmatter and the body `Progress:` line agree. + const postCompletionRoadmapScope = roadmapContent !== null + ? extractCurrentMilestone(roadmapContent, cwd) + : null; + const postCompletionRoadmapProgress = postCompletionRoadmapScope !== null + ? deriveProgressFromRoadmapForIntent(postCompletionRoadmapScope) + : null; + const authoritativeProgress: Record | undefined = + postCompletionRoadmapProgress && postCompletionRoadmapProgress.completedPhases !== null + ? postCompletionRoadmapProgress.totalPhases !== null && postCompletionRoadmapProgress.totalPhases > 0 + ? { + completed_phases: postCompletionRoadmapProgress.completedPhases, + percent: clampPercentForIntent( + postCompletionRoadmapProgress.completedPhases, + postCompletionRoadmapProgress.totalPhases, + ), + } + : { completed_phases: postCompletionRoadmapProgress.completedPhases } + : undefined; + const authoritativeFm: Record | undefined = authoritativeProgress + ? { + ...(nextPhaseDisplayName + ? bodyHasPhaseField || !nextPhaseNum + ? { current_phase_name: nextPhaseDisplayName } + : { + current_phase: String(nextPhaseNum), + current_phase_name: nextPhaseDisplayName, + } + : {}), + progress: authoritativeProgress, + } + : nextPhaseDisplayName + ? bodyHasPhaseField || !nextPhaseNum + ? { current_phase_name: nextPhaseDisplayName } + : { + current_phase: String(nextPhaseNum), + current_phase_name: nextPhaseDisplayName, + } + : undefined; // ADR-3408 §8.3 / #3469: this deliberately bypasses // readModifyWriteStateMd (STATE.md is committed atomically with // ROADMAP/REQUIREMENTS), so it calls the single write-seam diff --git a/src/state-transition.cts b/src/state-transition.cts index a652b04ab..e001c4d98 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -17,11 +17,16 @@ // eslint-disable-next-line @typescript-eslint/no-require-imports import frontmatter = require('./frontmatter.cjs'); import { stateReplaceField, stateExtractField, stateReplaceFieldIfTemplate, stateReplaceFieldWithFallback, stateReplaceFieldInSession, stateCurrentPositionSlice } from './state-document.cjs'; -import { KNOWN_TEMPLATE_DEFAULTS, toFiniteNumber } from './state-document.cjs'; +import { KNOWN_TEMPLATE_DEFAULTS, toFiniteNumber, computeProgressPercent } from './state-document.cjs'; import { tokenizeHeadings } from './markdown-sectionizer.cjs'; import type { HeadingToken } from './markdown-sectionizer.cjs'; import { deriveProgressFromRoadmap, clampPercent, clampPercentFromFraction } from './phase-lifecycle.cjs'; import { escapeRegex } from './pattern.cjs'; +// #4129: the completion-ratio kernel for the resync-arm ratchet's percent +// (planning-scope's SCOPE — state-document's own dependency, no cycle here: +// state-document never imports this module). +// eslint-disable-next-line @typescript-eslint/no-require-imports +import planningScopeMod = require('./planning-scope.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports import stateMdSchemaMod = require('./state-md-schema.cjs'); const { STATE_FIELD_SCHEMA } = stateMdSchemaMod; @@ -684,6 +689,78 @@ function preservedValuesEqual(a: unknown, b: unknown): boolean { return a === b; } +/** + * #4129: the resync-arm progress merge. A resyncing write whose scan MEASURED + * something no longer wholesale-replaces the curated block — the declared + * `progress-ratchet` mergeStrategy ("completed_plans/completed_phases only ever + * ratchet UP toward the derived value (#2969)", state-md-schema.cts) now holds + * on the write path too, matching what the read path (`shouldPreserveExistingProgress`) + * has always enforced. Rules, mirroring the `deriveProgressKeys` branch above: + * + * - total_plans / total_phases always take the derived value (#2440 — totals + * correct in BOTH directions). + * - completed_plans / completed_phases take the derived value only when it is + * strictly GREATER (#2969's `>` not `>=`); else the curated value survives + * (a hand-corrected or previously-correct counter can never be re-derived + * downward — the #4129 clobber). + * - any other key keeps the curated value (the existing branch's convention). + * - percent is RECOMPUTED from the merged counters through the single kernel + * (`computeProgressPercent`), because either side's stored percent was + * computed against that side's counters and the merged block may mix them + * (curated completed, derived totals). Recomputation runs ONLY when the + * derived block itself carried a percent — an upstream withhold + * (#1761 milestone-unbounded, #3217 scope) nulled percent deliberately and + * this merge must not resurrect it. + * + * Frontmatter scalars arrive as STRINGS ("2", not 2), so every comparison + * coerces through `toFiniteNumber` — never a `typeof === 'number'` test + * (scanMeasuredSomething's own convention). + */ +function mergeResyncProgressRatchet( + curatedRecord: Record, + derivedRecord: Record, +): Record { + const merged: Record = { ...derivedRecord }; + for (const [key, value] of Object.entries(curatedRecord)) { + if (key === 'total_plans' || key === 'total_phases' || key === 'percent') continue; + if (key === 'completed_plans' || key === 'completed_phases') { + const derivedNum = toFiniteNumber(derivedRecord[key]) ?? -Infinity; + const curatedNum = toFiniteNumber(value) ?? -Infinity; + // Ratchet up only (strictly greater, #2969); else keep curated. + if (derivedNum > curatedNum) continue; + // Numerically EQUAL keeps the derived value VERBATIM. The two sides + // arrive in different scalar shapes (the re-parsed derived block + // carries string totals "2" while the curated snapshot carries numbers + // 2), and substituting the curated spelling over an equal derived one + // is a no-op in substance but a shape churn the §8.7 reporting loop + // would surface as a phantom `preserved-over-disagreeing-derived` + // warning (it diffs structurally). Only a curated counter that is + // STRICTLY greater replaces the derived value. + if (derivedNum === curatedNum) continue; + merged[key] = value; + } else { + merged[key] = value; + } + } + if (toFiniteNumber(derivedRecord.percent) !== null) { + const recomputed = computeProgressPercent( + toFiniteNumber(merged.completed_plans), + toFiniteNumber(merged.total_plans), + toFiniteNumber(merged.completed_phases), + toFiniteNumber(merged.total_phases), + planningScopeMod.SCOPE.COMPLETE, + ); + // Same verbatim rule for percent: assign only when the recomputed value + // numerically differs, so a string-spelled derived percent ("67") is not + // churned into a number-spelled 67 (phantom-divergence noise, not a + // change). + if (recomputed !== null && recomputed !== toFiniteNumber(merged.percent)) { + merged.percent = recomputed; + } + } + return merged; +} + /** * Executor for `preservation: 'preserve-always'` (ADR-3408 §8.1). Only * `progress` carries this policy today. Preserves #3242/#1446/#2440/#2969 @@ -698,25 +775,41 @@ function applyPreserveAlways(field: string, cls: FieldClassification, ctx: Prese const derived = ctx.postFm[field]; const derivedMeasured = scanMeasuredSomething(cls, derived); const curatedMeasured = scanMeasuredSomething(cls, curated); - // On a resyncing write the fresh derivation is authoritative — UNLESS it - // measured nothing while the curated block did (#3756), AND the caller did - // not explicitly name a progress-affecting field this write. The - // unmeasured-scan guard exists to stop an INCIDENTAL resync (e.g. `state - // add-decision`, whose `resync` defaults true for reasons that have - // nothing to do with `progress`) from dropping a real curated block when a - // milestone-scoped disk scan measures nothing (#3756's archived-milestone - // case). It must not also block a write the user pointed AT `progress` on - // purpose: `preserve-always`'s own contract is "never overwrite unless the - // caller explicitly names this field" (FIELD_CLASSIFICATION doc comment), - // and `state update Progress` / `state patch Progress=...` are exactly - // that naming — the resync they trigger must win even when the disk scan - // it also drives (e.g. because there are no phase dirs at all) reads as - // "unmeasured" (tests/frontmatter.test.cjs: "state.update \"Progress\" - // resyncs progress frontmatter from the updated body", pre-existing, #3242). - if (ctx.resync && (derivedMeasured || !curatedMeasured || ctx.explicitProgressField)) return; + // On a resyncing write the fresh derivation is authoritative in two cases + // (#3756 / ADR-3473 §8.6, unchanged): when the caller EXPLICITLY named a + // progress-affecting field (`preserve-always`'s contract is "never + // overwrite unless the caller explicitly names this field" — `state update + // Progress` is exactly that naming, pre-existing #3242 behavior), and when + // the derivation measured something the curated block did not (an + // unmeasured CURATED block is not worth protecting). The unmeasured-DERIVED + // guard also stands: an incidental resync (e.g. `state add-decision`, whose + // `resync` defaults true for reasons that have nothing to do with + // `progress`) that measured nothing must not drop a real curated block + // (#3756's archived-milestone case) — that falls through to the wholesale + // restore below. + // + // #4129 narrows the remaining arm. A resyncing write whose scan MEASURED + // something while the curated block is also real previously wholesale- + // replaced the curated block with the derived one — no monotonic guard, so + // any under-counting derivation (a stale-dated verification, #2348) silently + // reverted every hand-correction and every correct value an earlier write + // had persisted, while the read path (`shouldPreserveExistingProgress`) + // kept reporting the higher stored counters. That arm now falls through to + // `mergeResyncProgressRatchet` — the declared `progress-ratchet` + // mergeStrategy, finally enforced on the write path: totals derived both + // directions (#2440), completed counters up-only (#2969), percent + // recomputed from the merged counters. + if (ctx.resync && (ctx.explicitProgressField || (derivedMeasured && !curatedMeasured))) return; let next: unknown; - if (cls.mergeStrategy === 'progress-ratchet' && ctx.deriveProgressKeys && derived && derivedMeasured) { + if (cls.mergeStrategy === 'progress-ratchet' && ctx.resync && derived && derivedMeasured && curatedMeasured) { + // #4129 resync arm — see mergeResyncProgressRatchet's doc. Reached only + // after the early-out above, so curatedMeasured is guaranteed true here. + next = mergeResyncProgressRatchet( + curated as Record, + (derived ?? {}) as Record, + ); + } else if (cls.mergeStrategy === 'progress-ratchet' && ctx.deriveProgressKeys && derived && derivedMeasured) { // #2440: total_plans and total_phases always take the derived (post-sync) // value even under !resync. This is used by cmdStatePlannedPhase where // total_plans must correct upward after plans are added. For body-only diff --git a/src/state.cts b/src/state.cts index 2ae2cdd61..b4ecedde0 100644 --- a/src/state.cts +++ b/src/state.cts @@ -69,6 +69,13 @@ import planDependencyGraphMod = require('./plan-dependency-graph.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports import verificationMod = require('./verification.cjs'); const { isPhaseComplete } = verificationMod; +// #4129: the single owner of "count the ROADMAP's milestone Complete rows" +// (phase-lifecycle.cts) — reused for the completed-phases numerator floor so +// this scan cannot grow a second ROADMAP parser. Pure computation module (no +// I/O), so it introduces no cycle on this path. +// eslint-disable-next-line @typescript-eslint/no-require-imports +import phaseLifecycleMod = require('./phase-lifecycle.cjs'); +const { deriveProgressFromRoadmap } = phaseLifecycleMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import planningScopeMod = require('./planning-scope.cjs'); const { SCOPE } = planningScopeMod; @@ -3014,6 +3021,34 @@ function buildStateFrontmatter( // write silently clobbered the three stored siblings with the // under-scoped disk numbers. const diskCountsWithheld = milestonedButUnbounded || roadmapAbsentWithAssertedMilestone; + // #4129: floor the completed-phases numerator at the ROADMAP's own + // milestone Complete-row count. The disk numerator counts ONLY + // phase dirs whose *-VERIFICATION.md routes `passed` (isPhaseComplete, + // #2957 disk-strict — the gate stays untouched), so a completed + // phase whose verification reads `stale` (a SUMMARY committed or + // edited after it, #2348 clean-commit-time clock) or `missing` + // (pre-verification era, hand-flipped ROADMAP row) drops out of the + // count forever — while every other surface (the ROADMAP row + // `phase complete` just flipped, the body `Completed Phases` field + // completePhaseCore derives from deriveProgressFromRoadmap) still + // asserts the phase complete. max(disk, ROADMAP) keeps the disk + // signal for gap detection (a verification-passed phase whose ROADMAP + // row is not yet flipped still counts) while never UNDER-counting + // what the ROADMAP asserts. Scoped exactly like the denominator: + // the same milestone window (roadmapScope), the same + // safeToUseRoadmapCount gate, and never under the #3354/#3573 + // withhold — a whole-document Complete-row count must not leak + // through an untrustworthy scope. Reuses deriveProgressFromRoadmap + // (phase-lifecycle.cts, the one owner of "read the Progress table") + // — no second ROADMAP parser here. A ROADMAP without a canonical + // `## Progress` table resolves no table → floor inert (disk count + // stands), the owner's own answer to "what is countable". + const roadmapCompletedPhases = roadmapScope !== null && safeToUseRoadmapCount && !diskCountsWithheld + ? deriveProgressFromRoadmap(roadmapScope).completedPhases + : null; + const flooredCompletedPhases = roadmapCompletedPhases !== null + ? Math.max(diskCompletedPhases, roadmapCompletedPhases) + : diskCompletedPhases; return { // The two WITHHOLD shapes (#3354 milestoned-but-unbounded, #3573 // roadmap-absent-with-asserted-milestone) must be evaluated BEFORE @@ -3024,7 +3059,7 @@ function buildStateFrontmatter( ? null : (safeToUseRoadmapCount ? Math.max(phaseDirs.length, roadmapPhaseCount) : phaseDirs.length), milestoneBounded, - completedPhases: diskCountsWithheld ? null : diskCompletedPhases, + completedPhases: diskCountsWithheld ? null : flooredCompletedPhases, totalPlans: diskCountsWithheld ? null : diskTotalPlans, completedPlans: diskCountsWithheld ? null : diskTotalSummaries, phaseDirScope, @@ -3395,6 +3430,76 @@ function readStoredCompletedPlans(existingFm: Record | null | u return readStoredProgressCounter(existingFm, 'completed_plans'); } +/** + * #4129: is this authoritativeFm value a PARTIAL progress intent? The #2736 + * seam was string-only (names); #4129 extends it with one object direction — + * the `progress` key carrying the sub-keys a transition resolved + * authoritatively (completePhase's ROADMAP-derived completed_phases/percent). + * Anything else keeps the seam's existing contract untouched. + */ +function isPartialProgressIntent(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + +/** + * #4129: merge a PARTIAL progress intent (see isPartialProgressIntent) into a + * frontmatter object's `progress` block. Sub-keys are accepted only when they + * are a declared `progress.*` row in FIELD_CLASSIFICATION — the single policy + * source (ADR-3408 §8.5) decides which leaves exist; an intent may not invent + * one. Returns whether anything changed. + * + * `completedOnlyRaise` (both application sites use it): completed counters + * apply only when strictly greater than what is already in the block, so no + * intent can LOWER a count another trustworthy signal already established — + * at the pre-preservation site the disk derivation's own count (a + * verification-passed phase whose ROADMAP row drifted behind), at the + * post-preservation re-assert the #2969 monotonic property preservation just + * enforced. `percent` follows its sibling: it is applied when a completed + * counter moved this call (the intent percent was computed from the intent + * counters and is coherent with them) or when the block has no percent to + * lose (a repair, never a regression of an upstream withhold — the withhold + * nulled percent upstream precisely so no write would re-assert one over + * untrustworthy counts; here the intent's own counts ARE the trustworthy + * source, the post-completion ROADMAP). + */ +function applyAuthoritativeProgressSubkeys( + fm: Record, + intent: Record, + opts: { completedOnlyRaise: boolean }, +): boolean { + const current = fm['progress']; + const base: Record = isPartialProgressIntent(current) + ? { ...current } + : {}; + let changed = false; + let completedMoved = false; + for (const [subkey, value] of Object.entries(intent)) { + if (typeof value !== 'number' || !Number.isFinite(value)) continue; + if (!getFieldClassification(`progress.${subkey}`)) continue; + const isCompletedCounter = subkey === 'completed_phases' || subkey === 'completed_plans'; + if (isCompletedCounter && opts.completedOnlyRaise) { + const currentNum = toFiniteNumber(base[subkey]); + if (currentNum !== null && currentNum >= value) continue; + } + if (isCompletedCounter && !Object.is(base[subkey], value)) completedMoved = true; + if (!Object.is(base[subkey], value)) { + base[subkey] = value; + changed = true; + } + } + // percent: applied only when a completed counter moved (coherent with the + // counters that just landed) or when no percent exists to contradict. + const intentPercent = intent['percent']; + if (typeof intentPercent === 'number' && Number.isFinite(intentPercent) && (completedMoved || toFiniteNumber(base['percent']) === null)) { + if (!Object.is(base['percent'], intentPercent)) { + base['percent'] = intentPercent; + changed = true; + } + } + if (changed) fm['progress'] = base; + return changed; +} + function syncStateFrontmatter( content: string, cwd: string | undefined, @@ -3620,10 +3725,20 @@ function syncStateFrontmatter( // parenthetical (`Closer-ruling measurement (D1a)` → `D1a`) — never runs // the final word on a field the transition just resolved. The prose parser // remains the fallback for genuinely unknown prose only. + // #4129: the `progress` key carries a PARTIAL block (the object direction of + // this seam — see applyAuthoritativeProgressSubkeys) for the same reason: + // completePhase holds the POST-completion ROADMAP, and the disk scan this + // function drives reads the PRE-completion one. The intent is applied as a + // FLOOR here too (completedOnlyRaise): a derivation that already counted + // MORE completed phases than the ROADMAP table asserts (verification-passed + // phases whose table rows drifted behind) must not be lowered by the intent + // — the two signals agree on direction (up), never on subtraction. if (authoritativeFm) { for (const [key, value] of Object.entries(authoritativeFm)) { if (typeof value === 'string' && value.trim().length > 0) { derivedFm[key] = value; + } else if (key === 'progress' && isPartialProgressIntent(value)) { + applyAuthoritativeProgressSubkeys(derivedFm, value, { completedOnlyRaise: true }); } } } @@ -4194,12 +4309,20 @@ function applyPostSyncPreservation( // (equal), so the #1695 restore fires and would put the stale pre-transition // name back over the authoritative one. Intent beats both the prose // re-derivation and the curated restore — the transition just resolved it. + // #4129: for the `progress` key the re-assert is a FLOOR, not an override — + // the #2969 monotonic property preservation just enforced (completed + // counters never move down) must not be undone by the intent, so completed + // sub-keys apply only-raise here (see applyAuthoritativeProgressSubkeys). let authoritativeReasserted = false; if (authoritativeFm) { for (const [key, value] of Object.entries(authoritativeFm)) { if (typeof value === 'string' && value.trim().length > 0 && preservation.postFm[key] !== value) { preservation.postFm[key] = value; authoritativeReasserted = true; + } else if (key === 'progress' && isPartialProgressIntent(value)) { + if (applyAuthoritativeProgressSubkeys(preservation.postFm, value, { completedOnlyRaise: true })) { + authoritativeReasserted = true; + } } } } diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index 8e6081efb..83c36d646 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -7380,6 +7380,167 @@ describe('bug-3287 — init plan-phase exposes expected_phase_dir with project_c }); }); + // ───────────────────────────────────────────────────────────────────────── + // #4129 row 1: `phase complete N` failed to increment completed_phases when + // a SIBLING completed phase's verification routes `stale` (its SUMMARY was + // touched after the verification — the issue's real-world drift). The + // transaction flips this phase's ROADMAP row and derives the BODY counters + // from the post-completion ROADMAP, but the frontmatter disk scan reads the + // PRE-completion ROADMAP and the stale-dated sibling, so the persisted + // counter stays pinned at the under-count. See .gsd/bug/ + // fix-4129-completed-phases-recompute/{10-diagnosis,50-test-matrix}.md. + // ───────────────────────────────────────────────────────────────────────── + describe('#4129: phase complete increments completed_phases to the ROADMAP truth', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4129-phase-')); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // Body Progress percent through the repo's own field extractor (never raw + // substring matching on rendered STATE.md — CONTRIBUTING.md prohibits it). + function bodyProgressPercentFromState(stateContent) { + const raw = stateExtractField(stateContent, 'Progress'); + if (raw === null) return null; + const match = raw.match(/(\d{1,3})%/); + return match ? Number(match[1]) : null; + } + + function setupStaleSiblingProject(tmpDir) { + const planningDir = path.join(tmpDir, '.planning'); + const phasesDir = path.join(planningDir, 'phases'); + fs.mkdirSync(phasesDir, { recursive: true }); + fs.writeFileSync(path.join(planningDir, 'config.json'), JSON.stringify({ project_code: 'REPRO' })); + + const roadmapLines = [ + '# Roadmap', + '', + '## Current Milestone: v1.0', + '', + '| Phase | Plans Complete | Status | Completed |', + '|-------|----------------|--------|-----------|', + '| 1. | 2/2 | Complete | 2026-01-01 |', + '| 2. | 2/2 | Complete | 2026-01-02 |', + '| 3. | 2/2 | In Progress | |', + ]; + for (let i = 4; i <= 18; i += 1) roadmapLines.push(`| ${i}. | 0/2 | Not Started | |`); + roadmapLines.push('', '- [x] Phase 1: Alpha (completed 2026-01-01)', '- [x] Phase 2: Beta (completed 2026-01-02)', '- [ ] Phase 3: Gamma'); + for (let i = 4; i <= 18; i += 1) roadmapLines.push(`- [ ] Phase ${i}: P${i}`); + for (let i = 1; i <= 18; i += 1) { + roadmapLines.push('', `### Phase ${i}: P${i}`, '', '**Goal:** goal', '**Plans:** 2 plans', ''); + } + fs.writeFileSync(path.join(planningDir, 'ROADMAP.md'), roadmapLines.join('\n')); + + fs.writeFileSync( + path.join(planningDir, 'STATE.md'), + [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'milestone_name: Programme', + 'status: executing', + 'current_phase: 3', + 'last_updated: 2026-01-02T10:00:00.000Z', + 'progress:', + ' total_phases: 18', + ' completed_phases: 2', + ' total_plans: 4', + ' completed_plans: 4', + ' percent: 11', + '---', + '', + '# Project State', + '', + '## Current Position', + '', + 'Phase: 3 of 18 (Gamma) — EXECUTING', + 'Plan: 2 of 2', + 'Status: Executing Phase 3', + 'Last activity: 2026-01-02', + '', + '## Progress', + '', + 'Progress: [█░░░░░░░░░] 11% (2/18 phases complete)', + '', + '## Session Continuity', + '', + 'Last session: 2026-01-02T10:00:00.000Z', + '', + ].join('\n'), + ); + + for (const p of [1, 2, 3]) { + const pp = String(p).padStart(2, '0'); + const dir = path.join(phasesDir, `${pp}-p${p}`); + fs.mkdirSync(dir, { recursive: true }); + for (const i of [1, 2]) { + fs.writeFileSync(path.join(dir, `${pp}-0${i}-PLAN.md`), '# Plan\n'); + fs.writeFileSync(path.join(dir, `${pp}-0${i}-SUMMARY.md`), '# Summary\n'); + } + fs.writeFileSync( + path.join(dir, `${pp}-VERIFICATION.md`), + ['---', 'status: passed', '---', '', '# Verification', ''].join('\n'), + ); + } + + // The drift: phase 1's summary touched after its verification. No git + // repo → the #2348 clock compares mtimes; the newer summary mtime + // routes phase 1's verification `stale` (verified via + // `verification status` in the diagnosis repro). + const older = new Date('2026-01-01T00:00:00Z'); + const newer = new Date('2026-03-01T00:00:00Z'); + fs.utimesSync(path.join(phasesDir, '01-p1', '01-VERIFICATION.md'), older, older); + fs.utimesSync(path.join(phasesDir, '01-p1', '01-01-SUMMARY.md'), newer, newer); + + return { planningDir }; + } + + test('phaseCompleteIncrementsCompletedPhasesPastStaleSibling', () => { + setupStaleSiblingProject(tmpDir); + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + + const r = runSdkQuery(['phase.complete', '3'], tmpDir); + assert.ok(r.success, `phase complete 3 failed: ${r.error}`); + + const state = fs.readFileSync(statePath, 'utf8'); + const fm = extractFrontmatter(state); + assert.ok(fm.progress, 'progress block must exist after phase complete'); + assert.equal( + Number(fm.progress.completed_phases), + 3, + `#4129: completing phase 3 must increment completed_phases to the ROADMAP truth 3 (phases 1-3 Complete), got ${fm.progress.completed_phases}`, + ); + assert.equal( + Number(fm.progress.percent), + 17, + `#4129: percent must follow the incremented counter (3/18), got ${fm.progress.percent}`, + ); + // The body bar must stay coherent with the persisted percent (#4129 AC7). + assert.equal(bodyProgressPercentFromState(state), 17, 'the body Progress bar must carry the same 17% as the frontmatter percent'); + // The ROADMAP row this very transaction flipped is the authority. + const roadmap = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf8'); + assert.ok(/^- \[x\] Phase 3: Gamma/m.test(roadmap), 'precondition: the transaction flipped the phase 3 ROADMAP checkbox'); + assert.ok(/^\| 3\.\s*\|\s*2\/2\s*\|\s*Complete\s*\|/m.test(roadmap), 'precondition: the transaction flipped the phase 3 table row'); + }); + + test('phaseCompleteIsIdempotentOnTheRoadmapTruth', () => { + setupStaleSiblingProject(tmpDir); + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + + const r1 = runSdkQuery(['phase.complete', '3'], tmpDir); + assert.ok(r1.success, `first call failed: ${r1.error}`); + const r2 = runSdkQuery(['phase.complete', '3'], tmpDir); + assert.ok(r2.success, `second call failed: ${r2.error}`); + + const fm = extractFrontmatter(fs.readFileSync(statePath, 'utf8')); + assert.equal(Number(fm.progress.completed_phases), 3, '#4129: double-complete stays at the ROADMAP truth 3 (idempotent)'); + }); + }); + // ───────────────────────────────────────────────────────────────────────── // ADR-3408 §8.3 Matrix A (#3469): cmdPhaseComplete now calls the ONE // write-seam composition (syncAndPreserveStateMd) directly instead of diff --git a/tests/state-transition.test.cjs b/tests/state-transition.test.cjs index 8eedd6053..0ad4996ba 100644 --- a/tests/state-transition.test.cjs +++ b/tests/state-transition.test.cjs @@ -4348,9 +4348,19 @@ describe('ADR-3473 §8.6 matrix rows 13-17 (+10/11 pinned): the measured-vs-unme }); test('stringNonZeroTotalIsMeasured', () => { + // #4129 superseded the observable: a measured resyncing write no longer + // lets the derived block wholesale-replace the curated one — the + // declared progress-ratchet now merges (totals derived both directions, + // completed counters up-only, #2969). The measured/unmeasured BOUNDARY + // this row pins is unchanged and still observable here: the derived + // TOTALS stand (a wholesale curated restore would have written 5/32). const r = restoreWithDerivedTotals({ total_phases: '1', total_plans: '0', completed_phases: '0', completed_plans: '0' }); - assert.strictEqual(r.mutated, false, 'a measured scan (total_phases:"1") must win — derived stands untouched'); - assert.deepStrictEqual(r.postFm.progress, { total_phases: '1', total_plans: '0', completed_phases: '0', completed_plans: '0' }); + assert.strictEqual(r.mutated, true, 'a measured scan (total_phases:"1") must not wholesale-restore the curated block — the #4129 ratchet merge runs'); + assert.deepStrictEqual( + r.postFm.progress, + { total_phases: '1', total_plans: '0', completed_phases: 5, completed_plans: 32 }, + 'measured: totals take the derived value (#2440 both directions); completed counters keep curated ("0" < 5/#2969 up-only); derived carried no percent so none is invented', + ); }); test('absentTotalsAreUnmeasured', () => { @@ -4379,15 +4389,18 @@ describe('ADR-3473 §8.6 matrix rows 13-17 (+10/11 pinned): the measured-vs-unme // Pinned mirror pair from the design's row 11/the existing matrix's row // 10 — grouped here so the boundary (only both-zero is unmeasured) reads - // legibly against the coercion cases above. + // legibly against the coercion cases above. #4129 superseded the observable + // to the ratchet merge (see stringNonZeroTotalIsMeasured above): "measured" + // is still decided by EITHER total being non-zero, and still observable + // because the derived TOTALS stand rather than a curated wholesale restore. test('oneNonZeroTotalCountsAsMeasuredEitherDirection', () => { const r1 = restoreWithDerivedTotals({ total_phases: 1, total_plans: 0, completed_phases: 0, completed_plans: 0 }); - assert.strictEqual(r1.mutated, false, 'total_phases:1, total_plans:0 must count as measured'); - assert.deepStrictEqual(r1.postFm.progress, { total_phases: 1, total_plans: 0, completed_phases: 0, completed_plans: 0 }); + assert.strictEqual(r1.mutated, true, 'total_phases:1, total_plans:0 must count as measured (#4129 ratchet merge ran, not a curated restore)'); + assert.deepStrictEqual(r1.postFm.progress, { total_phases: 1, total_plans: 0, completed_phases: 5, completed_plans: 32 }); const r2 = restoreWithDerivedTotals({ total_phases: 0, total_plans: 1, completed_phases: 0, completed_plans: 0 }); - assert.strictEqual(r2.mutated, false, 'total_phases:0, total_plans:1 must count as measured (the mirror) — only both-zero is unmeasured'); - assert.deepStrictEqual(r2.postFm.progress, { total_phases: 0, total_plans: 1, completed_phases: 0, completed_plans: 0 }); + assert.strictEqual(r2.mutated, true, 'total_phases:0, total_plans:1 must count as measured (the mirror) — only both-zero is unmeasured'); + assert.deepStrictEqual(r2.postFm.progress, { total_phases: 0, total_plans: 1, completed_phases: 5, completed_plans: 32 }); }); }); @@ -4441,3 +4454,121 @@ describe('ADR-3473 §8.6 matrix row 36: property — preservation never drops a ); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// #4129: the resync arm of `applyPreserveAlways` (rows 8-14 of .gsd/bug/ +// fix-4129-completed-phases-recompute/50-test-matrix.md). A resyncing write +// whose scan MEASURED something now runs the declared `progress-ratchet` +// merge instead of wholesale-replacing the curated block — the write path +// finally enforces the same monotonic property the read path +// (`shouldPreserveExistingProgress`) always has. The #3756 unmeasured guard +// and the #3242/#3871 explicit-progress contract are unchanged (pinned by the +// blocks above and by tests/frontmatter.test.cjs). +// ───────────────────────────────────────────────────────────────────────────── + +describe('#4129: resyncing measured write ratchets the progress block', () => { + function resyncMerge(curatedProgress, derivedProgress, extra = {}) { + const tx = openStateTransaction({ + snapshot: { progress: { ...curatedProgress } }, + resync: true, + bodyDeltas: neutralBodyDeltasForMatrix(), + ...extra, + }); + return applyStatePreservation({ transaction: tx, postFm: { progress: { ...derivedProgress } } }); + } + + test('resyncRatchetKeepsCuratedCompletedWhenDerivedUnderCounts', () => { + // The issue's exact shape: derived 2 (a stale-dated sibling verification) + // vs curated 3 (the ROADMAP truth a hand-fix or earlier correct write left). + const r = resyncMerge( + { total_phases: 18, completed_phases: 3, total_plans: 6, completed_plans: 6, percent: 17 }, + { total_phases: 18, completed_phases: 2, total_plans: 6, completed_plans: 6, percent: 11 }, + ); + assert.deepStrictEqual( + r.postFm.progress, + { total_phases: 18, completed_phases: 3, total_plans: 6, completed_plans: 6, percent: 17 }, + '#4129: a measured resync must never move completed counters DOWN — totals derived, completed curated, percent recomputed from the merged counters (3/18=17)', + ); + assert.strictEqual(r.mutated, true, 'the merge actually changed the derived block'); + }); + + test('resyncRatchetStillLetsCompletedMoveUp', () => { + // Genuine completion: derived ABOVE curated must ratchet up, and percent follows. + const r = resyncMerge( + { total_phases: 18, completed_phases: 2, total_plans: 6, completed_plans: 6, percent: 11 }, + { total_phases: 18, completed_phases: 3, total_plans: 6, completed_plans: 6, percent: 17 }, + ); + assert.deepStrictEqual( + r.postFm.progress, + { total_phases: 18, completed_phases: 3, total_plans: 6, completed_plans: 6, percent: 17 }, + '#4129: the ratchet is up-only, never a freeze — a genuine increment still lands', + ); + }); + + test('resyncRatchetMixedSidesRecomputePercentFromMergedCounters', () => { + // Mixed: derived completed_plans ratchets up, curated completed_phases + // survives — percent must be recomputed from the MERGED counters + // (min(54/54 plans, 5/? phases capped by plan fraction), never either + // side's stale stored percent. + const r = resyncMerge( + { total_plans: 54, completed_plans: 50, completed_phases: 5, percent: 93 }, + { total_plans: 54, completed_plans: 54, completed_phases: 1, percent: 50 }, + ); + assert.strictEqual(r.postFm.progress.completed_plans, 54, 'derived completed_plans (54 > 50) ratchets up'); + assert.strictEqual(r.postFm.progress.completed_phases, 5, 'curated completed_phases (5 > 1) survives — never down'); + // min(54/54, 5/5-with-no-total... completed_phases 5, total_phases absent) — + // computeProgressPercent with only plan data present: min(1.0, plan)=100? No: + // phase data absent → phaseFraction=1 → min(1, 1) = 100. + assert.strictEqual(r.postFm.progress.percent, 100, 'percent is recomputed from the merged counters through the completion-ratio kernel'); + }); + + test('resyncRatchetCoercesStringScalarsThroughToFiniteNumber', () => { + // Frontmatter scalars arrive as STRINGS ("2", not 2) — the comparison + // must coerce (scanMeasuredSomething's own convention), never `typeof`. + const r = resyncMerge( + { total_phases: 18, completed_phases: 3, percent: 17 }, + { total_phases: '18', total_plans: '6', completed_phases: '2', completed_plans: '6', percent: 11 }, + ); + assert.strictEqual(r.postFm.progress.completed_phases, 3, 'string "2" must compare numerically against curated 3 — curated survives'); + assert.strictEqual(r.postFm.progress.total_phases, '18', 'string totals take the derived value verbatim (both directions)'); + assert.strictEqual(r.postFm.progress.percent, 17, 'percent recomputed from merged counters (3/18)'); + }); + + test('resyncRatchetIsANoOpWhenDerivedEqualsCurated', () => { + // The #4094 withheld shape arrives here: the scan fell back to the stored + // counters, so derived == curated and the merge must not report a + // mutation (the #948 no-op-write family). + const block = { total_phases: 18, completed_phases: 3, total_plans: 6, completed_plans: 6, percent: 17 }; + const r = resyncMerge(block, { ...block }); + assert.strictEqual(r.mutated, false, 'derived === curated → no mutation (withheld shape is untouched)'); + assert.deepStrictEqual(r.postFm.progress, block); + }); + + test('resyncRatchetDoesNotResurrectAWithheldPercent', () => { + // Derived percent ABSENT (an upstream #1761/#3217 withhold nulled it) — + // the merge must not recompute one over counters it was withheld for. + // Percent follows the derived side (the deriveProgressKeys branch's own + // convention) and recomputation is gated on the derived block HAVING + // carried one — an absent percent stays absent, exactly as the + // pre-#4129 wholesale replace left it. + const r = resyncMerge( + { total_phases: 5, completed_phases: 5, percent: 100 }, + { total_phases: 5, completed_phases: 2 }, + ); + assert.strictEqual(r.postFm.progress.completed_phases, 5, 'curated completed counter survives'); + assert.ok(!('percent' in r.postFm.progress), 'no recomputed percent may appear when the derived block carried none'); + }); + + test('explicitProgressFieldStillWholesaleReplacesOnMeasuredScan', () => { + // #3242/#3871 contract, unchanged by #4129: when the caller NAMED a + // progress field, the resync they asked for wins outright — the escape + // hatch for deliberate downward correction stays open. + const r = resyncMerge( + { total_phases: 18, completed_phases: 3, percent: 17 }, + { total_phases: 18, completed_phases: 2, percent: 11 }, + { explicitProgressField: true }, + ); + assert.strictEqual(r.mutated, false, 'explicit progress write: the derived block stands untouched'); + assert.deepStrictEqual(r.postFm.progress, { total_phases: 18, completed_phases: 2, percent: 11 }); + }); +}); diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 31ea8fe14..8c843ed89 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -18163,6 +18163,237 @@ describe('#3871 / #3756: curated progress must survive a write on an archived mi }); }); +// ═════════════════════════════════════════════════════════════════════════ +// #4129: progress.completed_phases is recomputed to a WRONG value on every +// resyncing state write, and hand-fixes never survive (.gsd/bug/ +// fix-4129-completed-phases-recompute/{10-diagnosis,50-test-matrix}.md). +// +// The repro shape (issue rows 1/4): a completed phase whose SUMMARY was +// touched after its verification passed — a later reformat/re-run commit, or +// any dirty working-tree edit — permanently stale-dates that phase's +// verification under the #2348 clean-commit-time clock. isPhaseComplete +// (#2957 disk-strict, correctly) refuses to count it, so +// buildStateFrontmatter's disk numerator UNDER-counts vs the ROADMAP +// Complete rows that `phase complete` itself maintains, and the resync arm +// of applyPreserveAlways wholesale-replaces the stored block with the +// under-count on every default-resync write (record-session / add-decision / +// begin-phase / ...), clobbering any hand-corrected value. +// +// The fixture below is git-free: outside a repo the #2348 clock falls back +// to filesystem mtimes, so a newer-mtime SUMMARY reproduces the exact stale +// routing the real git clock produces (verified against the same +// `verification status` CLI the reporter used). +// ═════════════════════════════════════════════════════════════════════════ + +describe('#4129: completed_phases derives from the ROADMAP authority and survives resyncing writes', () => { + /** + * The #4129 STALE shape: milestone v1.0, 18 phases, phases 1-3 Complete in + * the ROADMAP (canonical 4-column Progress table + checklist), each with + * plans/summaries and a passing verification; phase 1's SUMMARY carries a + * NEWER mtime than its verification → `verification status` routes `stale` + * → disk numerator 2, ROADMAP truth 3. STATE.md starts at the truth (3). + * `includeStoredProgress: false` writes STATE.md with NO stored progress + * block, so the reported counters are exactly what the derivation computes + * (the read-path ratchet has no stored block to lean on). + */ + function buildStaleVerificationFixture(cwd, initialCompleted = 3, includeStoredProgress = true) { + const planningDir = path.join(cwd, '.planning'); + const phasesDir = path.join(planningDir, 'phases'); + fs.mkdirSync(phasesDir, { recursive: true }); + fs.writeFileSync(path.join(planningDir, 'config.json'), JSON.stringify({ project_code: 'REPRO' })); + + const roadmapLines = [ + '# Roadmap', + '', + '## Current Milestone: v1.0', + '', + '| Phase | Plans Complete | Status | Completed |', + '|-------|----------------|--------|-----------|', + '| 1. | 2/2 | Complete | 2026-01-01 |', + '| 2. | 2/2 | Complete | 2026-01-02 |', + '| 3. | 2/2 | Complete | 2026-01-03 |', + ]; + for (let i = 4; i <= 18; i += 1) roadmapLines.push(`| ${i}. | 0/2 | Not Started | |`); + roadmapLines.push('', '- [x] Phase 1: Alpha (completed 2026-01-01)', '- [x] Phase 2: Beta (completed 2026-01-02)', '- [x] Phase 3: Gamma (completed 2026-01-03)'); + for (let i = 4; i <= 18; i += 1) roadmapLines.push(`- [ ] Phase ${i}: P${i}`); + for (let i = 1; i <= 18; i += 1) { + roadmapLines.push('', `### Phase ${i}: P${i}`, '', '**Goal:** goal', '**Plans:** 2 plans', ''); + } + fs.writeFileSync(path.join(planningDir, 'ROADMAP.md'), roadmapLines.join('\n')); + + const stateLines = [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'milestone_name: Programme', + 'status: executing', + 'current_phase: 4', + 'last_updated: 2026-01-03T10:00:00.000Z', + ]; + if (includeStoredProgress) { + stateLines.push( + 'progress:', + ' total_phases: 18', + ` completed_phases: ${initialCompleted}`, + ' total_plans: 6', + ' completed_plans: 6', + ' percent: 17', + ); + } + stateLines.push( + '---', + '', + '# Project State', + '', + '## Current Position', + '', + 'Phase: 4 of 18 (P4) — EXECUTING', + 'Plan: 1 of 2', + 'Status: Executing Phase 4', + 'Last activity: 2026-01-03', + '', + '## Progress', + '', + `Progress: [██░░░░░░░░] 17% (${initialCompleted}/18 phases complete)`, + '', + '## Session', + '', + 'Last session: 2026-01-03T10:00:00.000Z', + 'Stopped at: Finished phase 3', + 'Resume file: None', + '', + ); + fs.writeFileSync(path.join(planningDir, 'STATE.md'), stateLines.join('\n')); + + for (const p of [1, 2, 3]) { + const pp = String(p).padStart(2, '0'); + const dir = path.join(phasesDir, `${pp}-p${p}`); + fs.mkdirSync(dir, { recursive: true }); + for (const i of [1, 2]) { + fs.writeFileSync(path.join(dir, `${pp}-0${i}-PLAN.md`), '# Plan\n'); + fs.writeFileSync(path.join(dir, `${pp}-0${i}-SUMMARY.md`), '# Summary\n'); + } + writePassedVerification(cwd, `${pp}-p${p}`, pp); + } + + // The drift: phase 1's summary edited AFTER the verification was written. + // No git repo → the #2348 clock compares mtimes; the newer summary mtime + // routes the phase-1 verification `stale` exactly as a later commit would. + const verificationPath = path.join(phasesDir, '01-p1', '01-VERIFICATION.md'); + const driftedSummary = path.join(phasesDir, '01-p1', '01-01-SUMMARY.md'); + const older = new Date('2026-01-01T00:00:00Z'); + const newer = new Date('2026-03-01T00:00:00Z'); + fs.utimesSync(verificationPath, older, older); + fs.utimesSync(driftedSummary, newer, newer); + + return { planningDir, phasesDir }; + } + + function readProgress(cwd) { + const content = fs.readFileSync(path.join(cwd, '.planning', 'STATE.md'), 'utf-8'); + const fm = frontmatterLib.extractFrontmatter(content); + assert.ok(fm && fm.progress, 'STATE.md frontmatter must carry a progress block'); + return { progress: fm.progress, content }; + } + + // Row 1 of the 50-test-matrix — the failing-first regression. On current + // `next` each of these verbs clobbers the stored 3 down to the + // stale-verification disk count 2 (percent 17 → 11), which is the issue's + // "any hand-correction is silently reverted by the next one". + test('handFixedCompletedPhasesSurvivesEveryResyncingWrite', (t) => { + const cwd = createTempDir('gsd-4129-handfix-'); + t.after(() => cleanup(cwd)); + buildStaleVerificationFixture(cwd, 3); + + // Precondition — the drift is really in place: phase 1 routes stale, so + // the disk numerator is 2 while ROADMAP + stored say 3. + const staleCheck = runGsdTools(['verification', 'status', path.join('.planning', 'phases', '01-p1')], cwd); + assert.ok(staleCheck.success, `verification status failed: ${staleCheck.error}`); + assert.strictEqual(JSON.parse(staleCheck.output).status, 'stale', 'fixture precondition: phase 1 verification must route stale (newer summary)'); + + for (const [label, args] of [ + ['state record-session', ['state', 'record-session', '--stopped-at', 'Finished phase 3 verification']], + ['state add-decision', ['state', 'add-decision', '--summary', 'Ship it']], + ['state begin-phase', ['state', 'begin-phase', '--phase', '4', '--name', 'P4']], + ]) { + const result = runGsdTools(args, cwd); + assert.ok(result.success, `${label} failed: ${result.error}`); + const { progress, content } = readProgress(cwd); + assert.strictEqual( + Number(progress.completed_phases), + 3, + `#4129: ${label} must not clobber the ROADMAP-correct completed_phases 3 down to the stale-verification disk count (${progress.completed_phases})`, + ); + assert.strictEqual(Number(progress.percent), 17, `#4129: ${label} — percent follows the surviving counters (3/18), not the discarded disk count`); + assert.strictEqual(bodyProgressPercent(content), 17, `#4129: ${label} — the body Progress bar must stay coherent with the persisted percent`); + } + }); + + // Row 2 — the DERIVED block itself must carry the ROADMAP-floored numerator. + // state json applies the read-path ratchet (shouldPreserveExistingProgress), + // which would mask a still-wrong derivation whenever a stored block exists — + // so this asserts on a STATE.md whose stored block is ABSENT: what json + // reports is then exactly what buildStateFrontmatter derived. + test('stateJsonDerivesCompletedPhasesFromRoadmapAuthority', (t) => { + const cwd = createTempDir('gsd-4129-jsonfloor-'); + t.after(() => cleanup(cwd)); + buildStaleVerificationFixture(cwd, 3, false); + + const result = runGsdTools(['state', 'json'], cwd); + assert.ok(result.success, `state json failed: ${result.error}`); + const reported = JSON.parse(result.output).progress; + assert.strictEqual( + Number(reported.completed_phases), + 3, + `#4129: the derived completed_phases must agree with the ROADMAP Complete rows (3), not the stale-verification disk count (${reported.completed_phases})`, + ); + assert.strictEqual(Number(reported.percent), 17, '#4129: percent derives from the floored numerator'); + }); + + // Row 3 — the flip side: a counter STUCK LOW (the issue's row-1 aftermath) + // must move UP to the ROADMAP truth on the next write, not stay pinned. + test('stuckLowCounterIncrementsToRoadmapTruthOnNextWrite', (t) => { + const cwd = createTempDir('gsd-4129-stucklow-'); + t.after(() => cleanup(cwd)); + buildStaleVerificationFixture(cwd, 2); + + const result = runGsdTools(['state', 'record-session', '--stopped-at', 'x'], cwd); + assert.ok(result.success, `record-session failed: ${result.error}`); + const { progress } = readProgress(cwd); + assert.strictEqual(Number(progress.completed_phases), 3, '#4129: a stored 2 below the ROADMAP truth must rise to 3, not be re-derived as 2'); + assert.strictEqual(Number(progress.percent), 17, '#4129: percent follows the corrected numerator'); + }); + + // Row 5 — negative space: no canonical Progress table (checklist-only + // ROADMAP) → deriveProgressFromRoadmap resolves no table → the floor is + // inert and the disk-verification count stands. The floor must not invent + // a parser for checklist bullets (one-owner rule). + test('floorIsInertWithoutCanonicalProgressTable', (t) => { + const cwd = createTempDir('gsd-4129-notable-'); + t.after(() => cleanup(cwd)); + buildStaleVerificationFixture(cwd, 2, false); + + // Rewrite the ROADMAP with the table stripped — checklist only. + // CRLF-tolerant line split (local/no-crlf-fragile-split). + const roadmapPath = path.join(cwd, '.planning', 'ROADMAP.md'); + const withoutTable = fs + .readFileSync(roadmapPath, 'utf-8') + .split(/\r?\n/) + .filter((line) => !line.trimStart().startsWith('|')) + .join('\n'); + fs.writeFileSync(roadmapPath, withoutTable); + + const result = runGsdTools(['state', 'json'], cwd); + assert.ok(result.success, `state json failed: ${result.error}`); + const reported = JSON.parse(result.output).progress; + assert.strictEqual( + Number(reported.completed_phases), + 2, + '#4129 negative space: without a canonical Progress table the completed count stays the disk-verification count (the floor reuses deriveProgressFromRoadmap, which reads only the table)', + ); + }); +}); + // ═════════════════════════════════════════════════════════════════════════ // #3872 / ADR-3473 §8.7: what a command reports it wrote // (.gsd/phase/feat-3872-transaction-diff-reporting/{40-design,50-test-matrix}.md)