From d9224696137fdb39b0a7fcd1ccb1fa8f9c990e7c Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 14 Aug 2026 23:43:56 -0400 Subject: [PATCH] refactor(#3408): close the two known limits instead of recording them (#3524) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(#3408): close the two known limits instead of recording them Both of these were flagged in review and written down as 'known limits' in a PR body and an issue comment. CLAUDE.md is explicit that a note is not a fix and is not surfacing — it is a silent defer. Recording them while closing the epic was the pattern this epic exists to remove, performed on the epic itself. syncAndPreserveStateMd and applyPostSyncPreservation each took eight positional arguments, the last three optional, one of them an out-param. The review's own wording was that 'a third consumer should trigger an options-object refactor' — a deferral with a trigger condition nobody would notice firing. Content and path stay positional; resync, authoritativeFm, deriveProgressKeys and divergedFields move into a named StatePreservationOptions. Every call site updated, with tsc as the proof none was missed. cmdStateCompletePhase's updated array carried both field labels and a section name, worked around by a SECTION_ENTRIES Set that re-derived the distinction by string matching. The kinds are now typed where they are produced and flattened once at output. Output contract unchanged: updated is still a flat string array with the same entries in the same order. Behavior-preservation was proven rather than asserted — the compiled lib was built at 411196bc3 and post-fix, and the same fixtures run through each. Both byte-identical, modulo the clock-driven last_updated. * fix(#3408): update every non-typed call site, and make a wrong options call loud The previous commit claimed 'tsc is the proof a site was not missed'. That was wrong and I asserted it. tsc type-checks src/ only; the test call sites are plain .cjs and are not checked at all. Fourteen tests failed with divergedFields: [] because a positional resync boolean landed in the options slot and every option came through undefined. Nine stale call sites converted. Also caught: the drift guard suite's E2 fixture embedded the old call shape 'verbatim from src/milestone.cts' — a fixture that mirrors production and had silently drifted from it. The deeper defect is that the refactor itself introduced the failure shape this epic exists to remove. An options-object parameter is silently mis-consumable by any non-TypeScript caller: pass the wrong thing and the function proceeds with every option undefined, returning a well-formed, plausible, empty result. That is precisely ADR-3408's Context section, reintroduced by the change meant to tidy the code up. Both functions now assert their options argument is a non-null object and throw STATE_PRESERVATION_OPTIONS_INVALID carrying the offending type, mirroring throwUnwiredRow. A test pins that the guard fires on the exact mistake that produced these fourteen failures. Verified by probe rather than asserted: a correct options call returns the expected divergedFields; a legacy positional call throws with the structured code instead of silently returning empty. * chore(#3408): drop the changeset — this PR has no user-facing impact CONTRIBUTING.md: 'PRs with no user-facing impact (test refactors, lint config changes, CI tweaks, formatting-only changes) can add the no-changelog label.' Typing this Changed and then exempting it from the docs requirement would have been wrong twice: it publishes a CHANGELOG entry under Changed when no command output moves, and it uses a per-fragment exemption to paper over a type that was wrong to begin with. Both refactors are behavior-preserving, verified byte-identical against the pre-change compiled lib. The one new throw guards a module-private seam in src/state.cts that no external caller can reach. * refactor(#3408): derive StatePreservationOptions, narrow the guard message Two findings from the orthogonal reviews on the close-known-limits PR. StatePreservationOptions repeated all four ReadModifyWriteOptions fields, differing only in resync being required, and divergedFields carried a second independently-worded docstring. It is now derived via Omit so the shared fields have one definition and cannot drift out of hand-sync. assertStatePreservationOptions echoed JSON.stringify(options) into the thrown message text. The contract for that guard is a structured code plus the offending type, not the value; echoing the value would become a disclosure path if a caller ever passed user-derived data. Removed from the message; err.code and err.receivedType are unchanged, and the test asserts on those. --------- Co-authored-by: sim --- src/milestone.cts | 9 +- src/phase.cts | 9 +- src/state.cts | 117 +++++++++++++++----- tests/state-write-path-drift-guard.test.cjs | 9 +- tests/state.test.cjs | 54 +++++++-- 5 files changed, 149 insertions(+), 49 deletions(-) diff --git a/src/milestone.cts b/src/milestone.cts index 3a5b1aeca..f6f7c0309 100644 --- a/src/milestone.cts +++ b/src/milestone.cts @@ -935,10 +935,11 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo result.content, statePath, cwd, - true, - Object.keys(authoritativeFm).length > 0 ? authoritativeFm : undefined, - undefined, - divergedFields, + { + resync: true, + authoritativeFm: Object.keys(authoritativeFm).length > 0 ? authoritativeFm : undefined, + divergedFields, + }, ); platformWriteSync(statePath, finalContent); for (const field of divergedFields) { diff --git a/src/phase.cts b/src/phase.cts index 2bf461d85..d7f4d9c17 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -3059,10 +3059,11 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { stateContent, statePath, cwd, - true, - authoritativeFm, - undefined, - divergedFields, + { + resync: true, + authoritativeFm, + divergedFields, + }, ); for (const field of divergedFields) { preservationWarnings.push({ field, reason: 'preserved-over-disagreeing-derived' }); diff --git a/src/state.cts b/src/state.cts index e184faf4e..ce40b0053 100644 --- a/src/state.cts +++ b/src/state.cts @@ -114,6 +114,21 @@ interface ReadModifyWriteOptions { divergedFields?: string[]; } +/** + * #3408 review (close-known-limits): options for `applyPostSyncPreservation` + * and `syncAndPreserveStateMd` — replaces the 8-positional-parameter + * signatures (a Data Clump / out-param smell flagged in review and deferred + * pending "a third consumer"; `cmdMilestoneComplete` is that third consumer). + * Same shape as `ReadModifyWriteOptions` minus `resync` being required here + * (every existing call site already passes it explicitly) — derived below + * so the two interfaces cannot drift out of hand-sync. + * + * `divergedFields` (ADR-3408 §8.5 D4) stays an out-param (not a return + * value) deliberately: converting it ripples into every caller's control + * flow for no behavior change. + */ +type StatePreservationOptions = Omit & { resync: boolean }; + interface StateRecordMetricOptions { phase: string; plan: string; @@ -2978,16 +2993,40 @@ function writeStateMd(statePath: string, content: string, cwd?: string, clock?: * sync only rewrites the frontmatter block, so its body IS the post-write * body), and `syncedContent` is what `syncStateFrontmatter` produced. */ +/** + * #3471 Fix: `StatePreservationOptions` is silently mis-consumable by any + * non-TypeScript caller — `tsc` only type-checks src/, so a plain-.cjs test + * (or any future JS caller) can pass a boolean where this options object + * goes and both functions below would previously proceed with `resync`, + * `authoritativeFm`, `deriveProgressKeys`, and `divergedFields` all + * `undefined`, degrading to a well-formed-looking but silently-empty + * `divergedFields: []` — exactly the "stale but present" failure shape + * ADR-3408 exists to remove. This is a contract assertion (caller-shape + * only), not field-level validation — mirrors `throwUnwiredRow`'s + * structured-error shape in src/state-transition.cts. + */ +function assertStatePreservationOptions(options: unknown, caller: string): asserts options is StatePreservationOptions { + if (typeof options !== 'object' || options === null || Array.isArray(options)) { + const err = new Error( + `${caller}: options argument must be a StatePreservationOptions object, got ${typeof options === 'object' ? 'array/null' : typeof options}. ` + + 'This function takes a single options object as its final ' + + 'parameter, not positional resync/authoritativeFm/deriveProgressKeys/divergedFields arguments (#3471).', + ) as Error & { code: string; receivedType: string }; + err.code = 'STATE_PRESERVATION_OPTIONS_INVALID'; + err.receivedType = Array.isArray(options) ? 'array' : typeof options; + throw err; + } +} + function applyPostSyncPreservation( originalContent: string, transformedContent: string, syncedContent: string, statePath: string, - resync: boolean, - authoritativeFm?: Record, - deriveProgressKeys?: boolean, - divergedFields?: string[], + options: StatePreservationOptions, ): string { + assertStatePreservationOptions(options, 'applyPostSyncPreservation'); + const { resync, authoritativeFm, deriveProgressKeys, divergedFields } = options; // Snapshot the existing progress block BEFORE the transform so we can // restore it when resync is false. const preFm = resync ? null : extractFrontmatter(originalContent, statePath) as Record; @@ -3207,21 +3246,16 @@ function syncAndPreserveStateMd( transformedContent: string, statePath: string, cwd: string | undefined, - resync: boolean, - authoritativeFm?: Record, - deriveProgressKeys?: boolean, - divergedFields?: string[], + options: StatePreservationOptions, ): string { - const synced = syncStateFrontmatter(transformedContent, cwd, authoritativeFm); + assertStatePreservationOptions(options, 'syncAndPreserveStateMd'); + const synced = syncStateFrontmatter(transformedContent, cwd, options.authoritativeFm); return applyPostSyncPreservation( originalContent, transformedContent, synced, statePath, - resync, - authoritativeFm, - deriveProgressKeys, - divergedFields, + options, ); } @@ -3274,10 +3308,12 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string modified, statePath, cwd, - resync, - options?.authoritativeFm, - options?.deriveProgressKeys === true, - options?.divergedFields, + { + resync, + authoritativeFm: options?.authoritativeFm, + deriveProgressKeys: options?.deriveProgressKeys === true, + divergedFields: options?.divergedFields, + }, ); platformWriteSync(statePath, synced); @@ -4674,6 +4710,21 @@ function resolvePhaseIdForCompletePhase(fm: Record, body: strin return parsePhaseFromProse(candidate).phase; } +/** + * #3408 review (close-known-limits): `cmdStateCompletePhase`'s `updated` + * tracks two different kinds of thing — a single FIELD `reconcileReportedFields` + * can look up against the persisted bytes, or the whole `Current Position` + * SECTION block, which is not a field at all. Typing the distinction at the + * producer (each `updated.push(...)` site) means the reconciliation step + * below reads the kind directly instead of re-deriving it by matching the + * entry's `name` against a hardcoded Set of section names. The command's + * OUTPUT CONTRACT is unaffected: `updated` is still flattened to a flat + * `string[]` (same entries, same order) at the single `output()` call site. + */ +type StateCompletePhaseUpdateEntry = + | { kind: 'field'; name: string } + | { kind: 'section'; name: string }; + function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string): void { const statePath = planningPaths(cwd).state; if (!fs.existsSync(statePath)) { @@ -4740,7 +4791,16 @@ function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string } const today = realClock.localToday(); - const updated: string[] = []; + // #3408 review (close-known-limits): `updated` mixes two different kinds of + // thing — FIELD names (Status, Last Activity, ...), each reconcilable + // against the persisted bytes via `reconcileReportedFields`, and the + // SECTION name `Current Position` (the whole Current-Position block, not a + // single field `stateExtractField` can look up). Rather than re-deriving + // the distinction downstream by string-matching against a Set, each entry + // now carries its kind at the point it is PRODUCED; the flattening to a + // flat `string[]` (the command's OUTPUT CONTRACT — unchanged) happens once + // below, right before `output()`. + const updated: StateCompletePhaseUpdateEntry[] = []; let preSyncContent = ''; const divergedFields: string[] = []; @@ -4759,16 +4819,16 @@ function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string // Update Status field (body only — #1255) const statusValue = `Phase ${currentPhase} complete`; let result = stateReplaceField(body, 'Status', statusValue); - if (result) { body = result; updated.push('Status'); } + if (result) { body = result; updated.push({ kind: 'field', name: 'Status' }); } // Update Last Activity date result = stateReplaceField(body, 'Last Activity', today); - if (result) { body = result; updated.push('Last Activity'); } + if (result) { body = result; updated.push({ kind: 'field', name: 'Last Activity' }); } // Update Last Activity Description const activityDesc = `Phase ${currentPhase} marked complete`; result = stateReplaceField(body, 'Last Activity Description', activityDesc); - if (result) { body = result; updated.push('Last Activity Description'); } + if (result) { body = result; updated.push({ kind: 'field', name: 'Last Activity Description' }); } // Update ## Current Position section // ADR-1372 T6: positionPattern → tokenizeHeadings; stop at level ≥ 2. @@ -4822,7 +4882,7 @@ function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string } body = body.slice(0, cpBodyStart) + posBody + body.slice(cpBodyEnd); - updated.push('Current Position'); + updated.push({ kind: 'section', name: 'Current Position' }); } } @@ -4833,18 +4893,19 @@ function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string // ADR-3408 §8.4 (D4): traced for this phase (design doc: "not traced in // the analysis pass"). Unlike the transitionCore-based commands, this - // adapter's `updated` mixes FIELD names (Status, Last Activity, Last + // adapter's `updated` mixes FIELD entries (Status, Last Activity, Last // Activity Description — each reconcilable against the persisted bytes, - // same as every other command in this phase) with the SECTION name + // same as every other command in this phase) with the SECTION entry // `Current Position` (the whole Current-Position block, not a single // field `stateExtractField` can look up — reconciling it the same way as // a field would always drop it as a false negative). Reconcile only the // field-shaped entries (#3351's direction), pass the section entry // through unconditionally, and fold in any field preservation restored - // that this transform never touched (#3345's direction). - const SECTION_ENTRIES = new Set(['Current Position']); - const sectionEntries = updated.filter((f) => SECTION_ENTRIES.has(f)); - const fieldEntries = updated.filter((f) => !SECTION_ENTRIES.has(f)); + // that this transform never touched (#3345's direction). The kind was + // decided at PUSH time above (typed producer), not re-derived here by + // string-matching a name against a Set. + const sectionEntries = updated.filter((e) => e.kind === 'section').map((e) => e.name); + const fieldEntries = updated.filter((e) => e.kind === 'field').map((e) => e.name); const reconciled = [...sectionEntries, ...reconcileReportedFields(statePath, preSyncContent, fieldEntries, divergedFields)]; output( diff --git a/tests/state-write-path-drift-guard.test.cjs b/tests/state-write-path-drift-guard.test.cjs index 88beae586..45da2224c 100644 --- a/tests/state-write-path-drift-guard.test.cjs +++ b/tests/state-write-path-drift-guard.test.cjs @@ -621,10 +621,11 @@ describe('E2 — a legitimate single call to the composition is not detected', ( ' result.content,', ' statePath,', ' cwd,', - ' true,', - ' undefined,', - ' undefined,', - ' divergedFields,', + ' {', + ' resync: true,', + ' authoritativeFm: Object.keys(authoritativeFm).length > 0 ? authoritativeFm : undefined,', + ' divergedFields,', + ' },', ' );', ].join('\n'); diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 4cac39367..80ef1fe66 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -5115,7 +5115,7 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid const transformed = original.replace('Status: Executing', 'Status: Verifying'); const statePath = path.join(tmp, 'STATE.md'); const divergedFields = []; - const out = stateLib.syncAndPreserveStateMd(original, transformed, statePath, tmp, false, undefined, undefined, divergedFields); + const out = stateLib.syncAndPreserveStateMd(original, transformed, statePath, tmp, { resync: false, divergedFields }); const fm = frontmatterLib.extractFrontmatter(out); assert.strictEqual(fm.current_phase, '5', 'current_phase must be restored by the executor, not lost'); assert.strictEqual(fm.current_phase_name, 'Curated Name', 'current_phase_name must be restored by the executor, not lost'); @@ -5135,7 +5135,7 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid const tmp = createTempDir('gsd-3471-a2-'); const statePath = path.join(tmp, 'STATE.md'); const divergedFields = []; - const out = stateLib.syncAndPreserveStateMd(original, transformed, statePath, tmp, false, undefined, undefined, divergedFields); + const out = stateLib.syncAndPreserveStateMd(original, transformed, statePath, tmp, { resync: false, divergedFields }); cleanup(tmp); return { fm: frontmatterLib.extractFrontmatter(out), divergedFields }; } @@ -5205,7 +5205,7 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid const statePath = path.join(tmp, 'STATE.md'); const divergedFields = []; assert.doesNotThrow(() => { - const out = stateLib.syncAndPreserveStateMd(original, transformed, statePath, tmp, false, undefined, undefined, divergedFields); + const out = stateLib.syncAndPreserveStateMd(original, transformed, statePath, tmp, { resync: false, divergedFields }); const fm = frontmatterLib.extractFrontmatter(out); assert.strictEqual(fm.current_phase, undefined); assert.strictEqual(fm.current_phase_name, undefined); @@ -5235,7 +5235,7 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid ].join('\n'); const transformed = original.replace('Total Phases: 3', 'Total Phases: 10'); const statePath = path.join(tmp, 'STATE.md'); - const out = stateLib.syncAndPreserveStateMd(original, transformed, statePath, tmp, false, undefined, true); + const out = stateLib.syncAndPreserveStateMd(original, transformed, statePath, tmp, { resync: false, deriveProgressKeys: true }); const fm = frontmatterLib.extractFrontmatter(out); // #3471 review: extractFrontmatter's mini-YAML parser returns nested // `progress.*` scalars as raw strings — see `numericProgress` above @@ -5262,7 +5262,7 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid const statePath = path.join(tmp, 'STATE.md'); fs.writeFileSync(statePath, original); const divergedFields = []; - const written = stateLib.syncAndPreserveStateMd(original, transformed, statePath, tmp, false, undefined, undefined, divergedFields); + const written = stateLib.syncAndPreserveStateMd(original, transformed, statePath, tmp, { resync: false, divergedFields }); fs.writeFileSync(statePath, written); // Consumer-level: read the real file back, never compare the owner's @@ -5293,7 +5293,7 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid fs.writeFileSync(statePath, original); const divergedFields = []; // No transform this write — delta unchanged (transformedContent === originalContent). - const written = stateLib.syncAndPreserveStateMd(original, original, statePath, tmp, false, undefined, undefined, divergedFields); + const written = stateLib.syncAndPreserveStateMd(original, original, statePath, tmp, { resync: false, divergedFields }); fs.writeFileSync(statePath, written); const onDisk = fs.readFileSync(statePath, 'utf8'); @@ -5314,10 +5314,46 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid ].join('\n'); const transformed = original.replace('Executing', 'Verifying'); const statePath = path.join(tmp, 'STATE.md'); - const out = stateLib.syncAndPreserveStateMd(original, transformed, statePath, tmp, false); + const out = stateLib.syncAndPreserveStateMd(original, transformed, statePath, tmp, { resync: false }); const fm = frontmatterLib.extractFrontmatter(out); assert.strictEqual(fm.custom_unknown_key, 'preserved-value', 'an unknown/custom frontmatter key must still carry forward — a different mechanism from the six deleted guards'); }); + + // A8 (#3471): the actual defect the 14-failure incident exposed — a + // plain-.cjs caller passing the OLD positional `resync` boolean where the + // options object now goes. `tsc` never catches this (it only + // type-checks src/); before this guard the function silently proceeded + // with every option `undefined`, producing a well-formed-looking but + // empty `divergedFields: []` rather than failing loudly. Assert on the + // structured `code`/`receivedType`, never on prose (CONTRIBUTING.md, + // "Prohibited: Raw Text Matching on Test Outputs"). + test('A8: passing a boolean in the options slot throws a structured error instead of silently degrading', () => { + const tmp = createTempDir('gsd-3471-a8-'); + const original = ['---', 'gsd_state_version: 1.0', '---', '', '# Project State', '', '## Current Position', '', 'Status: Executing', ''].join('\n'); + const transformed = original.replace('Status: Executing', 'Status: Verifying'); + const statePath = path.join(tmp, 'STATE.md'); + cleanup(tmp); + + assert.throws( + () => stateLib.syncAndPreserveStateMd(original, transformed, statePath, tmp, false), + (err) => { + assert.strictEqual(err.code, 'STATE_PRESERVATION_OPTIONS_INVALID'); + assert.strictEqual(err.receivedType, 'boolean'); + return true; + }, + 'syncAndPreserveStateMd must throw a structured error, not silently return with every option undefined', + ); + + assert.throws( + () => stateLib.applyPostSyncPreservation(original, transformed, transformed, statePath, false), + (err) => { + assert.strictEqual(err.code, 'STATE_PRESERVATION_OPTIONS_INVALID'); + assert.strictEqual(err.receivedType, 'boolean'); + return true; + }, + 'applyPostSyncPreservation must throw a structured error, not silently return with every option undefined', + ); + }); }); // ─── Section C — cmdStateJson (D3) ───────────────────────────────────────── @@ -5731,7 +5767,7 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid // Write-side: syncAndPreserveStateMd with an UNCHANGED delta // (transformedContent === originalContent) — the same regime // cmdStateJson's synthetic {pre,post} delta represents. - const written = stateLib.syncAndPreserveStateMd(content, content, statePath, tmp, false); + const written = stateLib.syncAndPreserveStateMd(content, content, statePath, tmp, { resync: false }); const writeFm = frontmatterLib.extractFrontmatter(written); const writeVal = writeFm[field] !== undefined && writeFm[field] !== null ? String(writeFm[field]) : null; @@ -10889,7 +10925,7 @@ describe('ADR-3408 §8.3 Matrix A1/A2/A3 (property): the shared write-seam compo // syncAndPreserveStateMd, then the adapter's own write. const pathA = path.join(tmp, 'A.md'); fs.writeFileSync(pathA, original); - const composed = stateLib.syncAndPreserveStateMd(original, transformed, pathA, tmp, resync, authoritativeFm); + const composed = stateLib.syncAndPreserveStateMd(original, transformed, pathA, tmp, { resync, authoritativeFm }); fs.writeFileSync(pathA, composed); // Path B: readModifyWriteStateMd's owner shape — same composition,