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,