From 21f0b4316f5390b7bfe7484f82308055aa350943 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 27 Jun 2026 22:49:52 -0400 Subject: [PATCH] =?UTF-8?q?refactor(#1796):=20ADR-1769=20Path=20A=20?= =?UTF-8?q?=E2=80=94=20finish=20STATE.md=20preservation=20consolidation=20?= =?UTF-8?q?(#1799)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract readModifyWriteStateMd's post-sync preservation block into a pure, field-classification-table-driven applyStatePreservation in the STATE.md Transition Module. progress / status / stopped_at now join current_phase_name as table-governed (getFieldClassification), so a preservation-policy change is a one-row table edit instead of a per-call-site patch. This realizes the consolidation ADR-1769 / CONTEXT.md already claimed shipped ('Absorbs readModifyWriteStateMd post-sync preservation block') and routes the #1264 preservation policy through the single field-classification table — the bug class is now structurally guarded by the table, not just the call-site shouldResync flag. Behavior is byte-identical to the pre-amendment inline block (Hyrum-safe — the 15 readModifyWriteStateMd callers' observable preservation is unchanged): - state/frontmatter/transition + bug regression suite: 847 pass - phase/milestone/verify (other RMW consumers): 582 pass - codex (gpt-5.5/high) adversarial review: CLEAN (58,564-case equiv sweep) ADR-1769 amendment appended documenting #1796. Closes #1796 --- docs/adr/1769-state-md-transition-module.md | 35 +++++++ gsd-core/bin/lib/state-transition.cjs | 59 +++++++++++ src/state-transition.cts | 108 ++++++++++++++++++++ src/state.cts | 85 +++------------ tests/state-transition.test.cjs | 103 +++++++++++++++++++ 5 files changed, 322 insertions(+), 68 deletions(-) diff --git a/docs/adr/1769-state-md-transition-module.md b/docs/adr/1769-state-md-transition-module.md index da6c3c002..429bb55eb 100644 --- a/docs/adr/1769-state-md-transition-module.md +++ b/docs/adr/1769-state-md-transition-module.md @@ -215,3 +215,38 @@ alongside, leave callbacks — parallel worlds don't converge (ADR-857's failure | 5 | `milestoneComplete` + `milestone.cts:352` | #1789 | — | | 6 | `patch` | #1791 | #1743, #1695 | | 7 | `sync`, `prune`, `update` | #1793 | #1760, #1761 | + +## Amendments + +### #1796 — Finish the preservation consolidation (Path A) + +**Date:** 2026-06-28 · **Status:** Accepted + +Surfaced by an `/adr-phase-coverage` audit of this ADR (issue #1796): the +Consequences claim that the module *"Absorbs … `readModifyWriteStateMd`'s +post-sync preservation block"* was **not** realized by Phases 0–7. The block +stayed inline in `readModifyWriteStateMd` (`state.cts`); only +`current_phase_name`'s preservation was table-driven. `#1264` was fixed by a +call-site `shouldResync` guard rather than the field-classification table, so the +bug *class* was not "killed structurally" as the Consequences (line 161) claimed. + +**Resolution — Path A ("finish the consolidation"):** the post-sync preservation +block is now the pure, field-classification-table-driven `applyStatePreservation` +in `src/state-transition.cts`, consulted via `getFieldClassification` for **all +four** preserved fields — `progress`, `status`, `stopped_at`, `current_phase_name` +(previously only the last was table-driven). `readModifyWriteStateMd` calls it. +This makes the CONTEXT.md "Absorbs … post-sync preservation block" claim accurate +and routes the `#1264` preservation policy through the single field-classification +table (one policy source, not three drifting encodings). + +Behavior is byte-identical to the pre-amendment inline block (Hyrum-safe — 15 +callers' observable preservation is unchanged); the full state / frontmatter / +transition regression suite (847 tests, including the `#1264`, `#1743`, `#1695`, +`#1760`, `#1761`, `#3242`, `#1230` characterization blocks) passes unmodified. +A codex (gpt-5.5 / high) adversarial review returned CLEAN — no behavior drift +across a 58,564-case equivalence sweep, no security surface introduced. + +The `shouldResync` call-site guard remains — it is the transition's declaration +of "am I re-deriving from disk?" What changed is that the *preservation policy* +it feeds is now centralized and table-driven rather than re-encoded per writer, +which is the consolidation this ADR originally specified. diff --git a/gsd-core/bin/lib/state-transition.cjs b/gsd-core/bin/lib/state-transition.cjs index a79ccc7c5..3aca769ab 100644 --- a/gsd-core/bin/lib/state-transition.cjs +++ b/gsd-core/bin/lib/state-transition.cjs @@ -17,6 +17,7 @@ Object.defineProperty(exports, "__esModule", { value: true }); exports.STATE_MD_SECTIONS = exports.FIELD_CLASSIFICATION = void 0; exports.getFieldClassification = getFieldClassification; +exports.applyStatePreservation = applyStatePreservation; exports.transitionCore = transitionCore; // eslint-disable-next-line @typescript-eslint/no-require-imports const frontmatter = require("./frontmatter.cjs"); @@ -79,6 +80,64 @@ function getFieldClassification(field) { return null; return exports.FIELD_CLASSIFICATION[field]; } +/** + * Pure, table-driven post-sync preservation. Mutates `postFm` in place to + * mirror the pre-consolidation inline block (which also mutated in place) and + * returns whether any field was restored. + */ +function applyStatePreservation(input) { + const { preFm, postFm, preFmSnapshot, resync } = input; + let mutated = false; + // Curated progress ratchet (#3242/#1446; closes the #1264 class by routing + // the policy through the table). Restored only when the table says preserve- + // always AND this transition is not re-deriving from disk (!resync). sync and + // the lifecycle transitions pass resync=true and recompute; patch/update and + // body-only writes pass resync=false and keep the curated counters. + const progressCls = getFieldClassification('progress'); + if (progressCls !== null && + progressCls.preservation === 'preserve-always' && + !resync && + preFm && + preFm['progress']) { + postFm['progress'] = preFm['progress']; + mutated = true; + } + // status — #1230 body-delta heuristic. Table: preserve-when-unchanged. + const statusCls = getFieldClassification('status'); + if (statusCls !== null && + statusCls.preservation === 'preserve-when-unchanged' && + input.postBodyStatus === input.preBodyStatus && + typeof preFmSnapshot['status'] === 'string' && + preFmSnapshot['status'].length > 0 && + preFmSnapshot['status'] !== 'unknown' && + postFm['status'] !== preFmSnapshot['status']) { + postFm['status'] = preFmSnapshot['status']; + mutated = true; + } + // stopped_at — same #1230 body-delta heuristic. Table: preserve-when-unchanged. + const stoppedCls = getFieldClassification('stopped_at'); + if (stoppedCls !== null && + stoppedCls.preservation === 'preserve-when-unchanged' && + input.postBodyStoppedAt === input.preBodyStoppedAt && + typeof preFmSnapshot['stopped_at'] === 'string' && + preFmSnapshot['stopped_at'].length > 0 && + postFm['stopped_at'] !== preFmSnapshot['stopped_at']) { + postFm['stopped_at'] = preFmSnapshot['stopped_at']; + mutated = true; + } + // current_phase_name — curated (#1743/#1695). Table: preserve-always. + const phaseNameCls = getFieldClassification('current_phase_name'); + if (phaseNameCls !== null && + phaseNameCls.preservation === 'preserve-always' && + input.postBodyPhaseSource === input.preBodyPhaseSource && + typeof preFmSnapshot['current_phase_name'] === 'string' && + preFmSnapshot['current_phase_name'].length > 0 && + postFm['current_phase_name'] !== preFmSnapshot['current_phase_name']) { + postFm['current_phase_name'] = preFmSnapshot['current_phase_name']; + mutated = true; + } + return { postFm, mutated }; +} // ---------------------------------------------------------------------------- // Body section constants (ADR-1769 §6 — single writer after migration) // ---------------------------------------------------------------------------- diff --git a/src/state-transition.cts b/src/state-transition.cts index 13862d365..966aea056 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -117,6 +117,114 @@ export function getFieldClassification(field: string): FieldClassification | nul return FIELD_CLASSIFICATION[field]; } +// ---------------------------------------------------------------------------- +// applyStatePreservation — table-driven post-sync preservation (ADR-1769 #1796) +// ---------------------------------------------------------------------------- +// +// Absorbs the post-sync preservation block that previously lived inline in +// `readModifyWriteStateMd` (state.cts). One pure, field-classification-table- +// driven implementation replaces the three drifting encodings (the RMW post-sync +// block, `syncStateFrontmatter`, and `cmdStateBuildFrontmatter`'s read-path +// copy). Every preserved field — progress, status, stopped_at, current_phase_name +// — is governed by its FIELD_CLASSIFICATION row, so a policy change is a one-row +// table edit rather than a per-call-site patch. Behavior is byte-identical to +// the pre-#1796 inline block; this is the consolidation ADR-1769 / CONTEXT.md +// already claimed shipped. See issue #1796 (Path A: finish the consolidation). + +export type StatePreservationInput = { + /** Pre-transform frontmatter; `null` when the transition re-derives from disk (resync=true). */ + preFm: Record | null; + /** Post-`syncStateFrontmatter` frontmatter (the freshly recomputed one). */ + postFm: Record; + /** Always-present pre-transform frontmatter snapshot (drives the #1230 deltas). */ + preFmSnapshot: Record; + /** True when the caller asked for a full disk re-derivation (sync / advancePlan / completePhase). */ + resync: boolean; + preBodyStatus: string | null; + postBodyStatus: string | null; + preBodyStoppedAt: string | null; + postBodyStoppedAt: string | null; + preBodyPhaseSource: string | null; + postBodyPhaseSource: string | null; +}; + +export type StatePreservationResult = { + postFm: Record; + mutated: boolean; +}; + +/** + * Pure, table-driven post-sync preservation. Mutates `postFm` in place to + * mirror the pre-consolidation inline block (which also mutated in place) and + * returns whether any field was restored. + */ +export function applyStatePreservation(input: StatePreservationInput): StatePreservationResult { + const { preFm, postFm, preFmSnapshot, resync } = input; + let mutated = false; + + // Curated progress ratchet (#3242/#1446; closes the #1264 class by routing + // the policy through the table). Restored only when the table says preserve- + // always AND this transition is not re-deriving from disk (!resync). sync and + // the lifecycle transitions pass resync=true and recompute; patch/update and + // body-only writes pass resync=false and keep the curated counters. + const progressCls = getFieldClassification('progress'); + if ( + progressCls !== null && + progressCls.preservation === 'preserve-always' && + !resync && + preFm && + preFm['progress'] + ) { + postFm['progress'] = preFm['progress']; + mutated = true; + } + + // status — #1230 body-delta heuristic. Table: preserve-when-unchanged. + const statusCls = getFieldClassification('status'); + if ( + statusCls !== null && + statusCls.preservation === 'preserve-when-unchanged' && + input.postBodyStatus === input.preBodyStatus && + typeof preFmSnapshot['status'] === 'string' && + preFmSnapshot['status'].length > 0 && + preFmSnapshot['status'] !== 'unknown' && + postFm['status'] !== preFmSnapshot['status'] + ) { + postFm['status'] = preFmSnapshot['status']; + mutated = true; + } + + // stopped_at — same #1230 body-delta heuristic. Table: preserve-when-unchanged. + const stoppedCls = getFieldClassification('stopped_at'); + if ( + stoppedCls !== null && + stoppedCls.preservation === 'preserve-when-unchanged' && + input.postBodyStoppedAt === input.preBodyStoppedAt && + typeof preFmSnapshot['stopped_at'] === 'string' && + preFmSnapshot['stopped_at'].length > 0 && + postFm['stopped_at'] !== preFmSnapshot['stopped_at'] + ) { + postFm['stopped_at'] = preFmSnapshot['stopped_at']; + mutated = true; + } + + // current_phase_name — curated (#1743/#1695). Table: preserve-always. + const phaseNameCls = getFieldClassification('current_phase_name'); + if ( + phaseNameCls !== null && + phaseNameCls.preservation === 'preserve-always' && + input.postBodyPhaseSource === input.preBodyPhaseSource && + typeof preFmSnapshot['current_phase_name'] === 'string' && + preFmSnapshot['current_phase_name'].length > 0 && + postFm['current_phase_name'] !== preFmSnapshot['current_phase_name'] + ) { + postFm['current_phase_name'] = preFmSnapshot['current_phase_name']; + mutated = true; + } + + return { postFm, mutated }; +} + // ---------------------------------------------------------------------------- // Body section constants (ADR-1769 §6 — single writer after migration) // ---------------------------------------------------------------------------- diff --git a/src/state.cts b/src/state.cts index 08cc0fbad..ac9df0f5e 100644 --- a/src/state.cts +++ b/src/state.cts @@ -32,7 +32,7 @@ const { extractFrontmatter, reconstructFrontmatter } = frontmatter; import scanPhasePlans = require('./plan-scan.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports import stateTransitionMod = require('./state-transition.cjs'); -const { transitionCore, getFieldClassification } = stateTransitionMod; +const { transitionCore, applyStatePreservation } = stateTransitionMod; type StateTransitionIntent = stateTransitionMod.StateTransitionIntent; type StateTransitionDeps = stateTransitionMod.StateTransitionDeps; import { @@ -1913,14 +1913,9 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string let synced = syncStateFrontmatter(modified, cwd); - // Compute postFm once and apply BOTH the progress-restore (when !resync) - // AND the status/stopped_at preservation (#1230) before reconstructing. - // This avoids double-wrapping the frontmatter block. - const needsProgressRestore = !resync && preFm && preFm['progress']; - // Post-transform body source fields used for the delta comparison (#1230). // Use `modified` (not `synced`): syncStateFrontmatter only rewrites the frontmatter block, so the body is identical in both — and we need the body the transform produced. - // Strip frontmatter so the YAML status key cannot shadow the body field. + // Strip frontmatter so the YAML status key cannot shadow the body field we are tracking. const postBody = stripFrontmatter(modified); const postBodyStatus = stateExtractField(postBody, 'Status'); // Bug #1230 / Change B: scope stopped_at delta to the ## Session section, @@ -1932,68 +1927,22 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string // current_phase_name delta comparison. const postBodyPhaseSource = stateExtractField(postBody, 'Phase'); - let mutated = false; + // ADR-1769 #1796 (Path A — finish the consolidation): the post-sync + // preservation block is now the pure, table-driven `applyStatePreservation` + // in the STATE.md Transition Module. progress / status / stopped_at / + // current_phase_name are all governed by their FIELD_CLASSIFICATION row — + // one policy source, not three drifting encodings. Behavior-identical to + // the pre-#1796 inline block; this is the absorption ADR-1769 / CONTEXT.md + // already claimed shipped. const postFm = extractFrontmatter(synced) as Record; - - if (needsProgressRestore) { - // Re-apply the curated progress block that syncStateFrontmatter just - // overwrote with disk-derived values. Only restore keys that were present - // in the snapshot — this preserves any new non-progress frontmatter fields - // (e.g., status, current_phase) that syncStateFrontmatter legitimately - // derived from the updated body. - postFm['progress'] = preFm['progress']; - mutated = true; - } - - // Bug #1230: preserve existing frontmatter status when this write did NOT - // change the body's Status field. A write that doesn't touch Status must - // not silently revert a hand-set frontmatter status (e.g. 'completed') to - // whatever the stale body Status happens to derive (e.g. 'verifying'). - // Only apply when the existing frontmatter held a real, non-unknown status. - if ( - postBodyStatus === preBodyStatus && - typeof preFmSnapshot['status'] === 'string' && - preFmSnapshot['status'].length > 0 && - preFmSnapshot['status'] !== 'unknown' && - postFm['status'] !== preFmSnapshot['status'] - ) { - postFm['status'] = preFmSnapshot['status']; - mutated = true; - } - - // Bug #1230: same delta heuristic for stopped_at. - if ( - postBodyStoppedAt === preBodyStoppedAt && - typeof preFmSnapshot['stopped_at'] === 'string' && - preFmSnapshot['stopped_at'].length > 0 && - postFm['stopped_at'] !== preFmSnapshot['stopped_at'] - ) { - postFm['stopped_at'] = preFmSnapshot['stopped_at']; - mutated = true; - } - - // ADR-1769 Phase 6 / #1743 / #1695: same delta heuristic for the curated - // current_phase_name. Gated by the field-classification table (preserve-always). - // When this write did NOT change the body `Phase:` source line, the curated - // frontmatter current_phase_name wins over syncStateFrontmatter's body - // re-derivation (parseProsePhaseField can harvest a wrong parenthetical - // aside — #1695). begin/planned/complete-phase rewrite their body Phase line, - // so the delta does not fire for them and current_phase_name still advances. - const phaseNameCls = getFieldClassification('current_phase_name'); - if ( - phaseNameCls !== null && - phaseNameCls.preservation === 'preserve-always' && - postBodyPhaseSource === preBodyPhaseSource && - typeof preFmSnapshot['current_phase_name'] === 'string' && - preFmSnapshot['current_phase_name'].length > 0 && - postFm['current_phase_name'] !== preFmSnapshot['current_phase_name'] - ) { - postFm['current_phase_name'] = preFmSnapshot['current_phase_name']; - mutated = true; - } - - if (mutated) { - const yamlStr = reconstructFrontmatter(postFm as unknown as Frontmatter); + const preservation = applyStatePreservation({ + preFm, postFm, preFmSnapshot, resync, + preBodyStatus, postBodyStatus, + preBodyStoppedAt, postBodyStoppedAt, + preBodyPhaseSource, postBodyPhaseSource, + }); + if (preservation.mutated) { + const yamlStr = reconstructFrontmatter(preservation.postFm as unknown as Frontmatter); const body = stripFrontmatter(synced); synced = `---\n${yamlStr}\n---\n\n${body}`; } diff --git a/tests/state-transition.test.cjs b/tests/state-transition.test.cjs index 3e6b102b5..eda13abd7 100644 --- a/tests/state-transition.test.cjs +++ b/tests/state-transition.test.cjs @@ -13,6 +13,7 @@ const fc = require('fast-check'); const { transitionCore, + applyStatePreservation, FIELD_CLASSIFICATION, getFieldClassification, STATE_MD_SECTIONS, @@ -1210,3 +1211,105 @@ describe('ADR-1769 Phase 7: sync transition — body writes + #1761', () => { assert.strictEqual(stateExtractField(result.content, 'Total Plans in Phase'), '2'); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// ADR-1769 #1796: applyStatePreservation — table-driven post-sync consolidation +// +// Path A ("finish the consolidation"): the post-sync preservation block that +// lived inline in readModifyWriteStateMd (state.cts) is absorbed into the +// Transition Module as a pure, field-classification-table-driven function. +// Every preserved field (progress, status, stopped_at, current_phase_name) is +// governed by its FIELD_CLASSIFICATION row — one policy source, not three +// drifting encodings. Behavior is identical to the pre-consolidation block; +// these tests pin the table-driven contract. See issue #1796. +// ───────────────────────────────────────────────────────────────────────────── + +describe('ADR-1769 #1796: applyStatePreservation — table-driven post-sync consolidation', () => { + // Shared no-op deltas for tests that only exercise one field. + const untouched = { + preBodyStatus: null, postBodyStatus: null, + preBodyStoppedAt: null, postBodyStoppedAt: null, + preBodyPhaseSource: null, postBodyPhaseSource: null, + }; + + test('progress: restores curated block when table=preserve-always and transition is not re-deriving (!resync)', () => { + const curated = { progress: { total_phases: 4, completed_phases: 3, percent: 75 } }; + const r = applyStatePreservation({ + preFm: curated, + preFmSnapshot: curated, + postFm: { progress: { total_phases: 5, completed_phases: 0, percent: 0 } }, // disk-derived clobber + resync: false, + ...untouched, + }); + assert.deepEqual(r.postFm.progress, { total_phases: 4, completed_phases: 3, percent: 75 }); + assert.equal(r.mutated, true); + }); + + test('progress: NOT restored when transition re-derives from disk (resync=true) — sync/advancePlan/completePhase path', () => { + const recomputed = { progress: { total_phases: 5, completed_phases: 1, percent: 20 } }; + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: {}, + postFm: { ...recomputed }, + resync: true, + ...untouched, + }); + assert.deepEqual(r.postFm.progress, { total_phases: 5, completed_phases: 1, percent: 20 }); + assert.equal(r.mutated, false); + }); + + test('status: preserves when body Status source is unchanged (preserve-when-unchanged) and snapshot holds a real status', () => { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { status: 'completed' }, + postFm: { status: 'verifying' }, + resync: true, + preBodyStatus: 'Executing Phase 3', postBodyStatus: 'Executing Phase 3', + preBodyStoppedAt: null, postBodyStoppedAt: null, + preBodyPhaseSource: null, postBodyPhaseSource: null, + }); + assert.equal(r.postFm.status, 'completed'); + assert.equal(r.mutated, true); + }); + + test('status: does NOT preserve when the body Status source line changed this write', () => { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { status: 'completed' }, + postFm: { status: 'verifying' }, + resync: true, + preBodyStatus: 'Executing Phase 3', postBodyStatus: 'Completed Phase 3', // changed + preBodyStoppedAt: null, postBodyStoppedAt: null, + preBodyPhaseSource: null, postBodyPhaseSource: null, + }); + assert.equal(r.postFm.status, 'verifying'); + assert.equal(r.mutated, false); + }); + + test('current_phase_name: preserves curated value when body Phase source unchanged (preserve-always)', () => { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { current_phase_name: 'curated-name' }, + postFm: { current_phase_name: 'wrong-parenthetical-harvest' }, + resync: true, + preBodyStatus: null, postBodyStatus: null, + preBodyStoppedAt: null, postBodyStoppedAt: null, + preBodyPhaseSource: '3', postBodyPhaseSource: '3', + }); + assert.equal(r.postFm.current_phase_name, 'curated-name'); + assert.equal(r.mutated, true); + }); + + test('returns mutated=false and untouched postFm when no preservation rule applies', () => { + const postFm = { status: 'executing', progress: { percent: 10 } }; + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: {}, + postFm, + resync: true, + ...untouched, + }); + assert.equal(r.mutated, false); + assert.deepEqual(r.postFm, { status: 'executing', progress: { percent: 10 } }); + }); +});