From 70f22e4643940b12bc7a2c9d4b3cacf8a463fa19 Mon Sep 17 00:00:00 2001 From: Atirna Date: Sat, 5 Sep 2026 17:26:06 +0530 Subject: [PATCH] fix(#4213): keep STATE.md progress surfaces synchronized (#4231) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#4213): keep STATE.md progress surfaces synchronized * fix(#4213): clamp the shared progress bar and keep bold-first priority, changeset + property tests - formatProgressMachineSegment clamps through clampPercentFromFraction (ADR-3180 Decision 7 kernel) with a 0 floor, so a hand-edited out-of-range persisted percent renders a clamped bar instead of throwing RangeError on repeat() inside the write seam - stateReplaceProgressPercent restores the #2177 bold-first priority: **Progress:** anywhere in the body wins; a plain ^Progress: line is the fallback, so free text starting with Progress: cannot capture the rewrite ahead of the real status line - cross-reference comment names the three consumers and the cmdStateSync sanctioned exception (ADR-3408 §8.3) - CONTEXT.md: applyPostSyncPreservation reconciliation documented in the STATE.md Transition Module entry - property tests (never-throws/well-formed, idempotency, round-trip, bold-first) + two regression rows through the CLI --------- Co-authored-by: Tom Boucher --- .changeset/4213-progress-surfaces-sync.md | 5 + CONTEXT.md | 2 +- src/state-transition.cts | 56 ++++++++-- src/state.cts | 50 +++------ tests/4213-progress-helpers.property.test.cjs | 90 ++++++++++++++++ tests/state.test.cjs | 100 ++++++++++++++++++ 6 files changed, 261 insertions(+), 42 deletions(-) create mode 100644 .changeset/4213-progress-surfaces-sync.md create mode 100644 tests/4213-progress-helpers.property.test.cjs diff --git a/.changeset/4213-progress-surfaces-sync.md b/.changeset/4213-progress-surfaces-sync.md new file mode 100644 index 000000000..227e020cb --- /dev/null +++ b/.changeset/4213-progress-surfaces-sync.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4231 +--- +**`state` verbs keep the STATE.md body Progress bar in sync with frontmatter `progress.percent`** — 13 of 15 verbs rewrote the frontmatter percent while the body bar stayed stale (issue #4213: frontmatter 75, body bar still 50), so the two surfaces silently diverged on every record-session, add-decision and milestone switch. The bar is now rewritten through one shared helper on the write seam, keeping the bold `**Progress:**` status line the target even when a free-text line above it starts with `Progress:`, and an out-of-range persisted percent renders a clamped 0-100 bar instead of crashing the write. (#4213) diff --git a/CONTEXT.md b/CONTEXT.md index 1b3c56afe..f463c4c69 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -85,7 +85,7 @@ Module owning projection from dispatch results/errors to CLI `{ exitCode, stdout Module owning STATE.md parse, field extraction, field replacement, status normalization, frontmatter reconstruction, and `## Current Position` section scoping (`stateCurrentPositionSlice`, #1956 — the one owner of that scope for the read path; `state.cts`'s `matchCurrentPositionSection` is a thin alias over it, and the `drift-guard phase-status` seam consumes it, so the #2956 archive-shadowing fix cannot be re-derived into a second copy — the byte-exact mutation path served by `state-transition.cts`'s `locateCurrentPosition`/`sliceCurrentPositionSection` is a deliberately separate, un-consolidated locator, #3187). `stateFieldValue` (#3187) is the single owner of the #1760 frontmatter-then-body field fallback chain, consolidating the 14 re-derivations of that ladder onto one scope-carrying (`complete`/`truncated`/`unscoped`/`unreadable`) primitive. `isRealCalendarDate` (#3696) is the single owner of "does this y/m/d exist on the calendar" — moved here from a private copy in `smart-entry.cts` so that reader and `state validate`'s S008 cannot drift into disagreeing about whether a STATE.md is usable; it enforces ADR-227's shape-AND-value rule, rejecting `2026-02-30` rather than letting `Date.parse` roll it forward to `2026-03-02`. `stateFieldContinuation` (#3696) reports the prose a wrapped single-line field leaves behind, which `stateExtractField`'s newline-excluding `(.+)` silently drops (S009); it is additive beside `stateExtractField` rather than a widening of it, because ADR-3180 §7.7 Rejected #1 pins that function as untouchable (20 direct callers, CRITICAL blast radius) and a continuation join there would apply to every field — `Status:` would swallow the line beneath it. It does not scan `.planning/phases` and does not own persistence or locking; phase/plan/summary counts arrive from inventory/progress Modules as inputs, and read-modify-write paths remain Adapters. Source of truth: `gsd-core/bin/lib/state-document.cjs`. **Commit provenance (#2573):** `state_head` records the full sha STATE.md was written against, stamped by `syncStateFrontmatter` and omitted entirely outside a git repo. `readStateHeadFreshness(cwd, stateHead)` (`src/state.cts`) is the single derivation consumed by both `validate.health` (W024) and smart-entry — it returns `{ state_head, current_commit, commits_behind, commit_stale }` with **tri-state** `commit_stale`: `null` = unknown (no stamp, no git, or a stamp that is not an ancestor of HEAD after a history rewrite), `false` = known fresh, `true` = the codebase has moved. Mirrors the graphify commit-staleness contract deliberately. It is a freshness PROXY, never a drift measurement: `rev-list` counts unrelated commits and the stamp restamps on every state write, so a low count means STATE.md was written recently, not that its contents are accurate — it must never gate. `toFiniteNumber` (#3871) is now EXPORTED as the single owner of "coerce this frontmatter scalar to a finite number, or say it is not one" — the STATE.md Transition Module's progress-ratchet consults it to decide whether a derived total is a real measurement, and a private second copy there would have diverged immediately, because frontmatter scalars arrive as STRINGS (`"0"`, not `0`) and a `=== 0` test is wrong at both call sites. ### STATE.md Transition Module -Module owning STATE.md lifecycle/maintenance transitions as intent-based methods (`beginPhase`, `advancePlan`, `completePhase`, `plannedPhase`, `milestoneSwitch`, `milestoneComplete`, `patch`, `sync`, `prune`, `update`, `rebuild`). Pure core `(content, intent, deps) → newContent` with injected I/O (file read/write, lock, disk scan); consults a field-classification table that names each STATE.md field's class (`derived-from-body` | `derived-from-disk` | `derived-from-external` | `curated` | `free`) and its preservation policy. Supersedes the 14 scattered RMW callbacks in `state.cts` (phase.cts's former direct caller has since been migrated away). `syncAndPreserveStateMd` (`state.cts`, #3469) is the single composition of `syncStateFrontmatter` + `applyPostSyncPreservation`; `readModifyWriteStateMd` and `cmdPhaseComplete` both **call** it rather than assembling the two steps, because assembling them at a call site is a re-derivation even when every step calls an owner. **Exactly two direct `writeStateMd` callers remain, and both are SANCTIONED PERMANENT exceptions under ADR-3408 §8.3 (as amended): `cmdStateSync` and the `REGENERATE_STATE` remedy (`health-diagnostic.cts`).** Neither is debt — `state sync` exists to re-derive frontmatter *from* the body (#905), and `REGENERATE_STATE` is a factory reset; preservation on either would re-lock precisely what the command was invoked to replace. `cmdMilestoneComplete` was the third and is now routed through the composition (#3469). Phase 1's whole-repo drift guard, not this line, is the authoritative count. *(Location correction: this entry previously placed the factory-reset primitive at `verify.cts:1925`. It moved to `health-diagnostic.cts` when `cmdValidateHealth` migrated onto the rule table (#3309); `verify.cts` now contains no `writeStateMd` call. The design intent was unchanged — only the address was stale.)* Absorbs `syncStateFrontmatter` + `readModifyWriteStateMd`'s post-sync preservation block; Encoding 3 (`cmdStateBuildFrontmatter`) stays separate — read path concern. Sibling/super-module of the STATE.md Document Module; consumes its `stateReplaceField`/`stateExtractField` primitives. Body section structure (`## Current Position`, `## Session`, etc.) lives as a constants block inside the Module. Append-only transitions (`addDecision`, `addBlocker`, etc.) stay on today's RMW seam for now. Targets the #1760/#1761/#1743/#1695/#1264/#1255/#1257/#3242 bug cluster. Migration per ADR-1372 §T6 sequenced as substrate + `beginPhase` first (PR1), then transition-by-transition with characterization tests first per transition. **ADR-1817 adds `rebuild` as the capstone 11th transition — the body-structure derivability contract.** Re-derives `## Current Position` prose from frontmatter and `## By-Phase Progress` table from phase dirs on disk; preserves `## Session` / `## Decisions` / unknown sections verbatim; de-duplicates `## Session Continuity Archive` (keep most-recent N, default 3); appends a structured audit entry to `## Rebuild Log` (`timestamp`, `kind`, `section`, `before`, `after`, `reason`) for every mutation. Hard idempotency guarantee: a no-mutation rebuild appends no log entry, so two successive invocations on a clean file are byte-identical. Non-overlapping with `sync` (3 lightweight frontmatter fields, auto-triggered) and orthogonal to `auto_prune_state` (age-based removal) — `rebuild` reconciles with current canonical sources, `prune` removes by retention policy, the two compose (rebuild first, then prune). Section ordering is invariant: rebuild rewrites content in place, never reorders. Targets the #1776/#1761/#1591 body-drift cluster that survived ADR-1769's per-field transitions. Phased per ADR-1817: Phase 0 = this ADR + predicates (closes #1817), Phase 1 = `rebuildCore` body + `rebuild` dispatch case + drift-class unit tests (#1827), Phase 2 = `cmdStateRebuild` CLI + `--dry-run`/`--verbose` + integration tests + docs + changeset (#1826). Source of truth: `gsd-core/bin/lib/state-transition.cjs` (generated from `src/state-transition.cts`). `state_head` (#2573) is classified `{ source: 'free', preservation: 'derive' }` — an ambient git read recomputed on every write, like `last_updated`; never preserved, because a stale stamp would claim STATE.md was written against a commit it wasn't. **Phase 4 (#3471):** `syncStateFrontmatter`'s six empty-only guards (D1) are now GATED — active only for the §8.3 sanctioned-permanent exceptions (`cmdStateSync`, `REGENERATE_STATE`), which have no preservation executor downstream and for which body-beats-frontmatter is the deliberate contract; OFF on the write seam, where `applyStatePreservation` owns the empty case via the exported `applyPreserveWhenUnchanged` executor. `reconcileReportedFields` (`state.cts`, §8.4/D4) is the single owner of reconciling a command's reported `updated`/`failed` field array against what was actually persisted, closing both #3351's (reported-but-discarded) and #3345's (persisted-but-unreported) directions; used by seven commands rather than seven re-derivations. `cmdStateJson` (§8.5/D3) no longer carries a private copy of the empty-only guards; it routes through `applyPreserveWhenUnchanged` directly, scoped to the same six fields. This entry has now needed correcting three times inside this one epic; Phase 1's whole-repo drift guard, not this line, remains the authoritative count. **ADR-3473 §8.7 (#3872) — the transaction diff.** `reconcileReportedFields` no longer compares the transform's own output against persisted bytes, nor filters `divergedFields` by policy class. It compares **persisted against the pre-write state the transaction already holds**, surfaced to the command through the same caller-allocates out-param idiom `divergedFields` established (and which `readModifyWriteStateMd`'s hand-enumerated option forwarding must list, or the field is silently dropped). Both prior directions fall out of that one comparison: a field the transform reported but the pipeline discarded is persisted-equals-snapshot and drops out (#3351), and a field nobody reported but the write moved is different and appears (#3345's `total_phases` 7→4, #3818's `current_phase` 203→204). The classification filter is **deleted, not relocated** — no `getFieldClassification` test survives in the reporting path. Reporting is at **dotted-leaf** granularity, enumerated from the `progress.*` rows `FIELD_CLASSIFICATION` already declares rather than by walking user data to arbitrary depth; that closes a live drop, because `plannedPhaseCore` already pushed `progress.total_plans` and a flat `hasOwnProperty` could never resolve it against nested frontmatter (`Current Position` was lost the same way). The one exclusion is `last_updated`, and it is by **provenance, not classification**: it is the only field measured to change on every write regardless of content. `state_head` was measured NOT to qualify — recomputed every write, but its value moves only when git HEAD moved. Without that single exclusion, `state.patch`'s success signal (`updated.length > 0`, `state.cts`) would be permanently true and a fully-failed patch would report success. **ADR-3473 §8.6 (#3871) — the state transaction.** `StateTransaction` is the Module's pre-write policy value, constructed only by `openStateTransaction` (preservation applies) or `rebuildStateTransaction` (it does not); both carry a MANDATORY `snapshot`, and an absent one is a **construction failure** (`STATE_TRANSACTION_SNAPSHOT_REQUIRED`), never a runtime no-op. This replaces the `preFm`/`preFmSnapshot` pair, which were the same `extractFrontmatter` call with one copy nulled on `resync` — a policy flag encoded as a missing input, which is why a declared `preserve-always` row silently skipped on the default write path (#3756). An EMPTY snapshot (`{}`) stays legal: that is the honest snapshot of a document with no parseable frontmatter, and `/gsd-health --repair` runs precisely on such documents. `rebuildStateTransaction` is the typed expression of ADR-3408 §8.3's closed exception list — `cmdStateSync` (#905) and `REGENERATE_STATE` — so `writeStateMd` takes a `rebuild` transaction and rejects anything else, and the two `sanctioned-permanent` entries the write-path drift guard used to ratchet as strings are retired with their baseline file. Within `applyPreserveAlways`, an all-zero or absent set of derived `progress` TOTALS is an **unmeasured scan, not a measurement** (the convention #3233 established, and why `computeProgressPercent` already returns `null` on an empty denominator), so the curated block stands rather than being overwritten with zeros; `completed_*` being zero is normal and does not decide it, and a measured scan still corrects totals downward (#1446/#2440). The rule carries a second, non-negotiable condition: it does **not** fire when the caller NAMED a progress field, because `preserve-always` has always meant "never overwrite unless the caller explicitly names this field" and `state update Progress` exists precisely to re-derive the block from the body just rewritten. `explicitProgressField` carries that and is **derived, never hand-set** — it comes from `shouldResyncStateProgress(fields)` (true iff the field set contains `Progress`, `Total Plans in Phase` or `Total Phases`), so it cannot drift from what the caller actually asked for. Omitting it silently discarded an explicitly-requested resync (#3242 / #1972), caught by the remote matrix, not by review. `getPreserveWhenUnchangedFields()` projects the `preserve-when-unchanged` rows out of `FIELD_CLASSIFICATION` so `cmdStateJson` consults the declaration instead of the hand-maintained parallel list that had drifted from it (#3836). +Module owning STATE.md lifecycle/maintenance transitions as intent-based methods (`beginPhase`, `advancePlan`, `completePhase`, `plannedPhase`, `milestoneSwitch`, `milestoneComplete`, `patch`, `sync`, `prune`, `update`, `rebuild`). Pure core `(content, intent, deps) → newContent` with injected I/O (file read/write, lock, disk scan); consults a field-classification table that names each STATE.md field's class (`derived-from-body` | `derived-from-disk` | `derived-from-external` | `curated` | `free`) and its preservation policy. Supersedes the 14 scattered RMW callbacks in `state.cts` (phase.cts's former direct caller has since been migrated away). `syncAndPreserveStateMd` (`state.cts`, #3469) is the single composition of `syncStateFrontmatter` + `applyPostSyncPreservation`; `readModifyWriteStateMd` and `cmdPhaseComplete` both **call** it rather than assembling the two steps, because assembling them at a call site is a re-derivation even when every step calls an owner. **Exactly two direct `writeStateMd` callers remain, and both are SANCTIONED PERMANENT exceptions under ADR-3408 §8.3 (as amended): `cmdStateSync` and the `REGENERATE_STATE` remedy (`health-diagnostic.cts`).** Neither is debt — `state sync` exists to re-derive frontmatter *from* the body (#905), and `REGENERATE_STATE` is a factory reset; preservation on either would re-lock precisely what the command was invoked to replace. `cmdMilestoneComplete` was the third and is now routed through the composition (#3469). Phase 1's whole-repo drift guard, not this line, is the authoritative count. *(Location correction: this entry previously placed the factory-reset primitive at `verify.cts:1925`. It moved to `health-diagnostic.cts` when `cmdValidateHealth` migrated onto the rule table (#3309); `verify.cts` now contains no `writeStateMd` call. The design intent was unchanged — only the address was stale.)* Absorbs `syncStateFrontmatter` + `readModifyWriteStateMd`'s post-sync preservation block; Encoding 3 (`cmdStateBuildFrontmatter`) stays separate — read path concern. Sibling/super-module of the STATE.md Document Module; consumes its `stateReplaceField`/`stateExtractField` primitives. Body section structure (`## Current Position`, `## Session`, etc.) lives as a constants block inside the Module. Append-only transitions (`addDecision`, `addBlocker`, etc.) stay on today's RMW seam for now. Targets the #1760/#1761/#1743/#1695/#1264/#1255/#1257/#3242 bug cluster. Migration per ADR-1372 §T6 sequenced as substrate + `beginPhase` first (PR1), then transition-by-transition with characterization tests first per transition. **ADR-1817 adds `rebuild` as the capstone 11th transition — the body-structure derivability contract.** Re-derives `## Current Position` prose from frontmatter and `## By-Phase Progress` table from phase dirs on disk; preserves `## Session` / `## Decisions` / unknown sections verbatim; de-duplicates `## Session Continuity Archive` (keep most-recent N, default 3); appends a structured audit entry to `## Rebuild Log` (`timestamp`, `kind`, `section`, `before`, `after`, `reason`) for every mutation. Hard idempotency guarantee: a no-mutation rebuild appends no log entry, so two successive invocations on a clean file are byte-identical. Non-overlapping with `sync` (3 lightweight frontmatter fields, auto-triggered) and orthogonal to `auto_prune_state` (age-based removal) — `rebuild` reconciles with current canonical sources, `prune` removes by retention policy, the two compose (rebuild first, then prune). Section ordering is invariant: rebuild rewrites content in place, never reorders. Targets the #1776/#1761/#1591 body-drift cluster that survived ADR-1769's per-field transitions. Phased per ADR-1817: Phase 0 = this ADR + predicates (closes #1817), Phase 1 = `rebuildCore` body + `rebuild` dispatch case + drift-class unit tests (#1827), Phase 2 = `cmdStateRebuild` CLI + `--dry-run`/`--verbose` + integration tests + docs + changeset (#1826). Source of truth: `gsd-core/bin/lib/state-transition.cjs` (generated from `src/state-transition.cts`). `state_head` (#2573) is classified `{ source: 'free', preservation: 'derive' }` — an ambient git read recomputed on every write, like `last_updated`; never preserved, because a stale stamp would claim STATE.md was written against a commit it wasn't. **Phase 4 (#3471):** `syncStateFrontmatter`'s six empty-only guards (D1) are now GATED — active only for the §8.3 sanctioned-permanent exceptions (`cmdStateSync`, `REGENERATE_STATE`), which have no preservation executor downstream and for which body-beats-frontmatter is the deliberate contract; OFF on the write seam, where `applyStatePreservation` owns the empty case via the exported `applyPreserveWhenUnchanged` executor. `reconcileReportedFields` (`state.cts`, §8.4/D4) is the single owner of reconciling a command's reported `updated`/`failed` field array against what was actually persisted, closing both #3351's (reported-but-discarded) and #3345's (persisted-but-unreported) directions; used by seven commands rather than seven re-derivations. `cmdStateJson` (§8.5/D3) no longer carries a private copy of the empty-only guards; it routes through `applyPreserveWhenUnchanged` directly, scoped to the same six fields. This entry has now needed correcting three times inside this one epic; Phase 1's whole-repo drift guard, not this line, remains the authoritative count. **ADR-3473 §8.7 (#3872) — the transaction diff.** `reconcileReportedFields` no longer compares the transform's own output against persisted bytes, nor filters `divergedFields` by policy class. It compares **persisted against the pre-write state the transaction already holds**, surfaced to the command through the same caller-allocates out-param idiom `divergedFields` established (and which `readModifyWriteStateMd`'s hand-enumerated option forwarding must list, or the field is silently dropped). Both prior directions fall out of that one comparison: a field the transform reported but the pipeline discarded is persisted-equals-snapshot and drops out (#3351), and a field nobody reported but the write moved is different and appears (#3345's `total_phases` 7→4, #3818's `current_phase` 203→204). The classification filter is **deleted, not relocated** — no `getFieldClassification` test survives in the reporting path. Reporting is at **dotted-leaf** granularity, enumerated from the `progress.*` rows `FIELD_CLASSIFICATION` already declares rather than by walking user data to arbitrary depth; that closes a live drop, because `plannedPhaseCore` already pushed `progress.total_plans` and a flat `hasOwnProperty` could never resolve it against nested frontmatter (`Current Position` was lost the same way). The one exclusion is `last_updated`, and it is by **provenance, not classification**: it is the only field measured to change on every write regardless of content. `state_head` was measured NOT to qualify — recomputed every write, but its value moves only when git HEAD moved. Without that single exclusion, `state.patch`'s success signal (`updated.length > 0`, `state.cts`) would be permanently true and a fully-failed patch would report success. **ADR-3473 §8.6 (#3871) — the state transaction.** `StateTransaction` is the Module's pre-write policy value, constructed only by `openStateTransaction` (preservation applies) or `rebuildStateTransaction` (it does not); both carry a MANDATORY `snapshot`, and an absent one is a **construction failure** (`STATE_TRANSACTION_SNAPSHOT_REQUIRED`), never a runtime no-op. This replaces the `preFm`/`preFmSnapshot` pair, which were the same `extractFrontmatter` call with one copy nulled on `resync` — a policy flag encoded as a missing input, which is why a declared `preserve-always` row silently skipped on the default write path (#3756). An EMPTY snapshot (`{}`) stays legal: that is the honest snapshot of a document with no parseable frontmatter, and `/gsd-health --repair` runs precisely on such documents. `rebuildStateTransaction` is the typed expression of ADR-3408 §8.3's closed exception list — `cmdStateSync` (#905) and `REGENERATE_STATE` — so `writeStateMd` takes a `rebuild` transaction and rejects anything else, and the two `sanctioned-permanent` entries the write-path drift guard used to ratchet as strings are retired with their baseline file. Within `applyPreserveAlways`, an all-zero or absent set of derived `progress` TOTALS is an **unmeasured scan, not a measurement** (the convention #3233 established, and why `computeProgressPercent` already returns `null` on an empty denominator), so the curated block stands rather than being overwritten with zeros; `completed_*` being zero is normal and does not decide it, and a measured scan still corrects totals downward (#1446/#2440). The rule carries a second, non-negotiable condition: it does **not** fire when the caller NAMED a progress field, because `preserve-always` has always meant "never overwrite unless the caller explicitly names this field" and `state update Progress` exists precisely to re-derive the block from the body just rewritten. `explicitProgressField` carries that and is **derived, never hand-set** — it comes from `shouldResyncStateProgress(fields)` (true iff the field set contains `Progress`, `Total Plans in Phase` or `Total Phases`), so it cannot drift from what the caller actually asked for. Omitting it silently discarded an explicitly-requested resync (#3242 / #1972), caught by the remote matrix, not by review. `getPreserveWhenUnchangedFields()` projects the `preserve-when-unchanged` rows out of `FIELD_CLASSIFICATION` so `cmdStateJson` consults the declaration instead of the hand-maintained parallel list that had drifted from it (#3836). **ADR-3408 §8.3 body-progress reconciliation (#4213):** `applyPostSyncPreservation` now closes by reconciling the body `Progress:` line against the persisted frontmatter `progress.percent` through `stateReplaceProgressPercent` (STATE.md Transition Module) — the #4213 fix: 13 of 15 `state.*` verbs rewrote `progress.percent` without touching the body bar, so the two surfaces silently diverged. The helper owns the #2177 priority (bold `**Progress:**` anywhere beats a plain `^Progress:` line, so a free-text line starting with `Progress:` cannot capture the rewrite) and clamps its bar through the completion-ratio kernel, so a hand-edited out-of-range persisted percent renders `[██████████] 100%` instead of throwing. It runs on every write through this seam, preserved-branch or not. Three named consumers keep the surfaces equal: this reconciliation, `cmdStateUpdateProgress`, and `syncCore`'s progress intent — `cmdStateSync` is the §8.3 sanctioned exception whose body-wins contract means it never reaches preservation, and its correctness comes from the `syncCore` call. A future writer that builds STATE.md content without one of those three reintroduces the divergence class. ### STATE.md Field Schema Module The one declaration (ADR-3473 §8.8, #3873) for "which STATE.md keys exist and what they carry", replacing three hand-maintained tables that were already observed to disagree: `FIELD_CLASSIFICATION` and `FRONTMATTER_BODY_SOURCE` (STATE.md Transition Module) and `FRONTMATTER_KEY_TO_BODY_LABEL` (STATE.md Document Module's `bodyLabelFor`, `state.cts`). One frozen, null-prototype `STATE_FIELD_SCHEMA` row per key carries `type`, `cardinality`, `source`, `preservation`, `guard`/`mergeStrategy` (ADR-3408's closed vocabularies, whose type declarations moved here), `bodySource`/`bodyLabel`, `acceptedShapes` (declared value shapes a hand-written parser accepts — e.g. `current_plan`'s `N` / `N of M`, #3784 — never a predicate; Greenspun's Tenth Rule still applies), and `emitted` (mirrors `buildStateFrontmatter`'s null-guards). The three original tables are now PROJECTIONS derived from this schema at module load, byte-identical in shape/key-order/frozen-and-null-prototype-ness to what they were before #3873 — every existing consumer (the preservation dispatch loop, `getFieldClassification`, `getPreserveWhenUnchangedFields`, `bodyLabelFor`, #3872's `declaredLeavesOf`) is unaffected. **The `last_activity` disagreement is resolved by declaration, not by picking the table that "looks right":** it carries a `bodySource` (it IS body-derived) but deliberately no `bodyLabel`, matching what ships today — its `preservation` is `derive`, so it can never reach `bodyLabelFor`'s `STATE_BODY_LABEL_UNWIRED_ROW` throw, pinned by `tests/state.test.cjs`'s `lastActivityLabelResolutionMatchesShippedBehavior`. Leaf module: imports from neither `state-transition.cts` nor `state.cts` (both import it), avoiding the CJS require-cycle `src/health-diagnostic-types.cts` was split out to break. `scripts/lint-state-field-drift.cjs` is UNCHANGED and retained — it guards the #3187 STATE.md field-extraction fallback-chain re-derivation, an orthogonal concern this schema does not make unrepresentable (§8.8's own claim that the guard is deleted here was verified false). Source of truth: `gsd-core/bin/lib/state-md-schema.cjs` (generated from `src/state-md-schema.cts`). diff --git a/src/state-transition.cts b/src/state-transition.cts index b64853f5d..a652b04ab 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -20,7 +20,7 @@ import { stateReplaceField, stateExtractField, stateReplaceFieldIfTemplate, stat import { KNOWN_TEMPLATE_DEFAULTS, toFiniteNumber } from './state-document.cjs'; import { tokenizeHeadings } from './markdown-sectionizer.cjs'; import type { HeadingToken } from './markdown-sectionizer.cjs'; -import { deriveProgressFromRoadmap, clampPercent } from './phase-lifecycle.cjs'; +import { deriveProgressFromRoadmap, clampPercent, clampPercentFromFraction } from './phase-lifecycle.cjs'; import { escapeRegex } from './pattern.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import stateMdSchemaMod = require('./state-md-schema.cjs'); @@ -29,6 +29,47 @@ type StateFieldSchema = stateMdSchemaMod.StateFieldSchema; const { extractFrontmatter, reconstructFrontmatter, stripFrontmatter, FRONTMATTER_UNPARSEABLE } = frontmatter; +export function formatProgressMachineSegment(percent: number): string { + // ADR-3180 Decision 7: rounding and the 100 ceiling belong to the + // completion-ratio kernel. The floor is added here because this helper is + // also fed persisted frontmatter values (hand-editable, unlike the + // count-shaped entries into that kernel), and `'░'.repeat` throws on a + // negative count. Bar and printed percent use the clamped value so the two + // halves of the segment can never disagree. + const clamped = Math.max(0, clampPercentFromFraction(percent / 100)); + const filled = Math.round(clamped / 10); + return `[${'█'.repeat(filled)}${'░'.repeat(10 - filled)}] ${clamped}%`; +} + +// Consumers (a future STATE.md writer that bypasses all three reintroduces the +// #4213 divergence class): `cmdStateUpdateProgress` and `syncCore`'s progress +// intent (both in this module) plus the post-sync body reconciliation in +// `applyPostSyncPreservation` (src/state.cts). `cmdStateSync` never reaches +// that reconciliation — ADR-3408 §8.3: `state sync` lets the body win, so +// preservation must NOT run — which is why its correctness comes from +// `syncCore`'s call here. +export function stateReplaceProgressPercent(content: string, percent: number): string | null { + const body = stripFrontmatter(content); + // #2177: bold `**Progress:**` anywhere in the body wins outright; the plain + // `^Progress:` form is the fallback only when no bold line exists, so an + // earlier free-text line starting with `Progress:` cannot capture the + // rewrite ahead of the real status line. + const boldProgressPattern = /(\*\*Progress:\*\*[ \t]*)([^\r\n]*)/i; + const plainProgressPattern = /^(Progress:[ \t]*)([^\r\n]*)/im; + const pattern = boldProgressPattern.test(body) + ? boldProgressPattern + : plainProgressPattern.test(body) + ? plainProgressPattern + : null; + if (!pattern) return null; + const machineSegment = /(?:\[[^\]\r\n]*\][ \t]*)?\d{1,3}%/; + const progress = formatProgressMachineSegment(percent); + const updatedBody = body.replace(pattern, (_match: string, prefix: string, value: string) => ( + `${prefix}${machineSegment.test(value) ? value.replace(machineSegment, progress) : progress}` + )); + return content.slice(0, content.length - body.length) + updatedBody; +} + /** * ADR-3473 §8.1 (#3881, consequence 2 wiring): does `existingFm` carry the * `FRONTMATTER_UNPARSEABLE` marker `extractFrontmatter` sets when a @@ -2771,13 +2812,12 @@ function syncCore( if (currentProgress) { const currentPercent = parseInt(currentProgress.replace(/[^\d]/g, ''), 10); if (currentPercent !== intent.percent) { - const barWidth = 10; - const filled = Math.round((intent.percent / 100) * barWidth); - const bar = '█'.repeat(filled) + '░'.repeat(barWidth - filled); - const progressStr = `[${bar}] ${intent.percent}%`; - changes.push(`Progress: ${currentProgress} -> ${progressStr}`); - const result = stateReplaceField(modified, 'Progress', progressStr); - if (result) { modified = result; updated.push('Progress'); } + const result = stateReplaceProgressPercent(modified, intent.percent); + if (result) { + const progressStr = formatProgressMachineSegment(intent.percent); + changes.push(`Progress: ${currentProgress} -> ${progressStr}`); + modified = result; updated.push('Progress'); + } } } } diff --git a/src/state.cts b/src/state.cts index 1b8ac74ce..17cbb799b 100644 --- a/src/state.cts +++ b/src/state.cts @@ -91,7 +91,7 @@ import { findProjectRoot } from './project-root.cjs'; // it introduces no cycle on this path. // eslint-disable-next-line @typescript-eslint/no-require-imports import milestoneLockMod = require('./milestone-lock.cjs'); -const { transitionCore, applyStatePreservation, sliceCurrentPositionSection } = stateTransitionMod; +const { transitionCore, applyStatePreservation, sliceCurrentPositionSection, stateReplaceProgressPercent, formatProgressMachineSegment } = stateTransitionMod; // #3699: the frontmatter-key <-> body-field routing behind `state update`'s // failure explanation, and the classification table it falls back to. const { getFieldClassification, getFrontmatterBodySource, frontmatterKeyForBodyField } = stateTransitionMod; @@ -112,6 +112,7 @@ import { shouldPreserveExistingProgress, stateExtractField, stateFieldValue, + toFiniteNumber, // #3696: the `last_activity` invariant that `state validate` (S008/S009) now // asserts. Both live in the field-semantics owner, not here, so `smart-entry` // and `state validate` cannot drift apart about the same field. @@ -1445,41 +1446,15 @@ function cmdStateUpdateProgress(cwd: string, raw: boolean): void { 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}%`; + const progressStr = formatProgressMachineSegment(percent); let updated = false; readModifyWriteStateMd(statePath, (content) => { - // #2177: match against the BODY only. With /i the patterns below would - // otherwise hit the YAML frontmatter `progress:` key first (and `\s*` would - // eat its newline, mangling the nested block), while the body Progress: line - // — which frontmatter `percent` is re-derived from on every write — stays - // stale and silently reverts the update. - const body = stripFrontmatter(content); - const fmPrefix = content.slice(0, content.length - body.length); - - // Swap only the machine segment ("[bar] NN%" or bare "NN%"), preserving any - // descriptive suffix an agent authored, e.g. "(2/4 plans done; blocked on…)". - const machineSegment = /(?:\[[^\]\r\n]*\][ \t]*)?\d{1,3}%/; - const replaceValue = (value: string) => machineSegment.test(value) - ? value.replace(machineSegment, progressStr) - : progressStr; - - // Try **Progress:** bold format first, then plain Progress: format. - const boldProgressPattern = /(\*\*Progress:\*\*[ \t]*)([^\r\n]*)/i; - const plainProgressPattern = /^(Progress:[ \t]*)([^\r\n]*)/im; - const pattern = boldProgressPattern.test(body) - ? boldProgressPattern - : plainProgressPattern.test(body) - ? plainProgressPattern - : null; - if (!pattern) return content; - + const result = stateReplaceProgressPercent(content, percent); + if (result === null) return content; updated = true; - return fmPrefix + body.replace(pattern, (_match, prefix: string, value: string) => `${prefix}${replaceValue(value)}`); + return result; }, cwd); if (updated) { @@ -4090,6 +4065,8 @@ function applyPostSyncPreservation( } } + let finalContent = syncedContent; + if (preservation.mutated || authoritativeReasserted) { // #3742: preservation RESTORES frontmatter keys the body-derived rebuild // could not produce (e.g. `current_phase` on a layout with no body @@ -4110,9 +4087,16 @@ function applyPostSyncPreservation( } const yamlStr = reconstructFrontmatter(preservation.postFm as unknown as Frontmatter); const body = stripFrontmatter(syncedContent); - return `---\n${yamlStr}\n---\n\n${body}`; + finalContent = `---\n${yamlStr}\n---\n\n${body}`; } - return syncedContent; + const persistedPercent = toFiniteNumber( + preservation.postFm['progress'] && (preservation.postFm['progress'] as Record)['percent'], + ); + if (persistedPercent !== null) { + const reconciled = stateReplaceProgressPercent(finalContent, persistedPercent); + if (reconciled !== null) finalContent = reconciled; + } + return finalContent; } /** diff --git a/tests/4213-progress-helpers.property.test.cjs b/tests/4213-progress-helpers.property.test.cjs new file mode 100644 index 000000000..c3b7a3583 --- /dev/null +++ b/tests/4213-progress-helpers.property.test.cjs @@ -0,0 +1,90 @@ +'use strict'; + +/** + * Property-based tests for the #4213 progress-surface helpers + * + * Module: gsd-core/bin/lib/state-transition.cjs + * Exported: formatProgressMachineSegment(percent), + * stateReplaceProgressPercent(content, percent) + * + * Properties tested: + * (a) formatProgressMachineSegment: never throws on any finite input + * (the review's RangeError case — persisted frontmatter values are + * hand-editable and only finiteness-checked upstream) + * (b) formatProgressMachineSegment: always returns a well-formed + * `[bar] NN%` segment with an exactly-10-glyph bar, 0-100 percent + * (c) stateReplaceProgressPercent: idempotent — a second application with + * the same percent is a no-op + * (d) stateReplaceProgressPercent: round-trip — the written bar re-parses + * to the same (clamped) percent for any 0-100 input + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fc = require('./helpers/fast-check-setup.cjs'); + +const { + formatProgressMachineSegment, + stateReplaceProgressPercent, +} = require('../gsd-core/bin/lib/state-transition.cjs'); + +const SEGMENT_RE = /^\[(█{0,10}░{0,10})\] (\d{1,3})%$/; + +function clampToPercentRange(n) { + return Math.max(0, Math.min(100, Math.round(n))); +} + +describe('formatProgressMachineSegment properties (#4213 review finding)', () => { + // (a) + (b) never throws, always well-formed, for ANY finite number. + test('property: never throws and always yields a well-formed segment for any finite percent', () => { + fc.assert( + fc.property(fc.double({ noDefaultInfinity: true, noNaN: true }), (percent) => { + const segment = formatProgressMachineSegment(percent); + const match = SEGMENT_RE.exec(segment); + assert.ok(match, `segment must match [bar] NN%, got ${segment}`); + assert.equal(match[1].length, 10, 'bar must be exactly 10 glyphs'); + const printed = Number(match[2]); + assert.ok(printed >= 0 && printed <= 100, `printed percent must be clamped, got ${printed}`); + }), + ); + }); + + // Bar fill agrees with the printed percent at the 0-100 boundaries reviewers read. + test('boundary literals: 0, 100, 105, -30', () => { + assert.equal(formatProgressMachineSegment(0), '[░░░░░░░░░░] 0%'); + assert.equal(formatProgressMachineSegment(100), '[██████████] 100%'); + assert.equal(formatProgressMachineSegment(105), '[██████████] 100%'); + assert.equal(formatProgressMachineSegment(-30), '[░░░░░░░░░░] 0%'); + }); +}); + +describe('stateReplaceProgressPercent properties (#4213 review finding)', () => { + // Any percent the writer can be handed, applied to a representative body. + const anyPercent = fc.integer({ min: -1000, max: 1000 }); + + // (c) idempotency: applying twice with the same percent changes nothing more. + test('property: idempotent on a second application with the same percent', () => { + fc.assert( + fc.property(anyPercent, (percent) => { + const content = '# Project State\n\n**Progress:** [█████░░░░░] 50% (2/4 plans done)\n'; + const once = stateReplaceProgressPercent(content, percent); + const twice = stateReplaceProgressPercent(once, percent); + assert.equal(twice, once); + }), + ); + }); + + // (d) round-trip: the segment the helper writes re-parses to the same + // clamped percent it printed for any in-range input. + test('property: written segment re-parses to the clamped percent (0-100)', () => { + fc.assert( + fc.property(fc.integer({ min: 0, max: 100 }), (percent) => { + const content = 'Progress: [██░░░░░░░░] 20%\n'; + const updated = stateReplaceProgressPercent(content, percent); + const match = /\[(█{0,10}░{0,10})\] (\d{1,3})%/.exec(updated); + assert.ok(match, `rewritten body must carry a machine segment, got ${updated}`); + assert.equal(Number(match[2]), clampToPercentRange(percent)); + }), + ); + }); +}); diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 5e38d1f7e..0fb0f3808 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -2857,6 +2857,106 @@ describe('cmdStateUpdateProgress (state update-progress)', () => { }); }); +describe('#4213: resyncing state verbs keep body Progress bar equal to frontmatter progress.percent', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createFixture(); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); + for (const num of ['01', '02', '03', '04']) { + const dir = path.join(tmpDir, '.planning', 'phases', num); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, `${num}-PLAN.md`), '# Plan\n'); + } + }); + + afterEach(() => cleanup(tmpDir)); + + function seedState(seededPercent = 50, withProgress = true) { + const progressLine = withProgress + ? `Progress: [█████░░░░░] ${seededPercent}% (2/4 plans done)` + : '**Status:** Executing'; + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + `---\ngsd_state_version: "1.0"\nstatus: executing\nprogress:\n total_phases: 4\n completed_phases: ${seededPercent / 25}\n total_plans: 4\n completed_plans: ${seededPercent / 25}\n percent: ${seededPercent}\n---\n\n# Project State\n\n${progressLine}\n` + ); + } + + function completePhasesOnDisk(count) { + for (const num of ['01', '02', '03', '04'].slice(0, count)) { + const dir = path.join(tmpDir, '.planning', 'phases', num); + fs.writeFileSync(path.join(dir, `${num}-PLAN-SUMMARY.md`), '# Summary\n'); + writePassedVerification(tmpDir, num, num); + } + } + function assertProgress(expected, suffix = false) { + const state = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.strictEqual(bodyProgressPercent(state), expected); + assert.strictEqual(Number(JSON.parse(runGsdTools('state json', tmpDir).output).progress.percent), expected); + if (suffix) assert.match(stateDocument.stateExtractField(state, 'Progress'), /\(2\/4 plans done\)/); + } + test('resyncing verbs repair drift-up and preserve the body suffix', () => { + for (const command of [['state', 'record-session', '--stopped-at', '2.3'], 'state sync']) { + seedState(); + completePhasesOnDisk(3); + assert.ok(runGsdTools(command, tmpDir).success, `${command} failed`); + assertProgress(75, true); + } + }); + test('a no-drift write keeps both surfaces at the existing percent', () => { + seedState(); completePhasesOnDisk(2); + assert.ok(runGsdTools(['state', 'record-session', '--stopped-at', '2.3'], tmpDir).success); + assertProgress(50); + }); + test('resyncing verbs repair drift-down without inserting a missing bar', () => { + seedState(); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), + '# Roadmap\n\n### Phase 01: A\n### Phase 02: B\n### Phase 03: C\n### Phase 04: D\n### Phase 05: E\n### Phase 06: F\n'); + completePhasesOnDisk(2); + assert.ok(runGsdTools(['state', 'add-decision', '--phase', '3', '--summary', 's'], tmpDir).success); + assertProgress(33); + seedState(50, false); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); + completePhasesOnDisk(3); + assert.ok(runGsdTools(['state', 'record-session', '--stopped-at', '2.3'], tmpDir).success); + const state = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.strictEqual(bodyProgressPercent(state), null); + assert.strictEqual(Number(JSON.parse(runGsdTools('state json', tmpDir).output).progress.percent), 75); + }); + test('a free-text plain Progress: line above the status line cannot capture the rewrite, and an out-of-range percent clamps', () => { + // The #2177 bold-first priority restated for the shared helper (an earlier + // free-text line starting with `Progress:` must stay byte-identical while + // the bold status line is rewritten — a leftmost-match alternation got + // this wrong), and the clamp case: a hand-edited body percent (105%) with + // an unmeasured scan (no plans on disk, so the curated block stands) + // reaches the helper through applyPostSyncPreservation and must render the + // clamped 100% bar instead of throwing RangeError on repeat(-1). + const freeText = 'Progress: tracked in the weekly thread, do not edit this line by hand'; + seedState(50); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), + `---\ngsd_state_version: "1.0"\nstatus: executing\nprogress:\n total_phases: 4\n completed_phases: 2\n total_plans: 4\n completed_plans: 2\n percent: 50\n---\n\n# Project State\n\n${freeText}\n\n**Progress:** [█████░░░░░] 50%\n`); + completePhasesOnDisk(3); + assert.ok(runGsdTools(['state', 'record-session', '--stopped-at', '2.3'], tmpDir).success); + let state = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.ok(state.includes(freeText), 'the free-text plain line must stay byte-identical'); + assert.match(stateDocument.stateExtractField(state, 'Progress'), /^\[████████░░\] 75%$/, 'the bold status line is the one rewritten (extractor returns its value)'); + assert.strictEqual(bodyProgressPercent(state), 75); + assert.strictEqual(Number(JSON.parse(runGsdTools('state json', tmpDir).output).progress.percent), 75); + + seedState(50); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), + '---\ngsd_state_version: "1.0"\nstatus: executing\nprogress:\n total_phases: 0\n completed_phases: 0\n total_plans: 0\n completed_plans: 0\n percent: 105\n---\n\n# Project State\n\nProgress: [██████████░] 105% (2/4 plans done)\n'); + for (let n = 1; n <= 4; n++) { + // eslint-disable-next-line local/no-raw-rmsync-in-tests -- removing fixture phase dirs beforeEach created; helpers.cleanup owns the tmp root itself + fs.rmSync(path.join(tmpDir, '.planning', 'phases', String(n).padStart(2, '0')), { recursive: true, force: true }); + } + assert.ok(runGsdTools(['state', 'add-decision', '--phase', '3', '--summary', 's'], tmpDir).success); + state = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.strictEqual(bodyProgressPercent(state), 100, 'bar renders the clamped 100%'); + assert.match(stateDocument.stateExtractField(state, 'Progress'), /\(2\/4 plans done\)/, 'suffix survives'); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // cmdStateResolveBlocker, cmdStateRecordSession // ─────────────────────────────────────────────────────────────────────────────