golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 4 Packages Projects Releases Wiki Activity

refactor(#3408): close the two known limits instead of recording them (#3524)

Browse Source 
* 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 <sim@local>
This commit is contained in:
Tom Boucher
2026-08-14 23:43:56 -04:00
committed by GitHub
parent 8fc88f663d
commit d922469613
5 changed files with 149 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
src/milestone.cts
View File

@@ -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) {

9
src/phase.cts
View File

@@ -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' });

117
src/state.cts
View File

@@ -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<ReadModifyWriteOptions, 'resync'> & { 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<string, unknown>,
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<string, unknown>;
@@ -3207,21 +3246,16 @@ function syncAndPreserveStateMd(
transformedContent: string,
statePath: string,
cwd: string | undefined,
resync: boolean,
authoritativeFm?: Record<string, unknown>,
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<string, unknown>, 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(

9
tests/state-write-path-drift-guard.test.cjs
View File

@@ -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');

54
tests/state.test.cjs
View File

@@ -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,