* test(#3872): failing-first regressions for what a command reports it wrote Pins ADR-3473 §8.7 at the consumer's output. state planned-phase advances current_phase on disk and never reports it, and reports progress.total_plans which reconcileReportedFields silently drops because it cannot resolve a dotted key against nested frontmatter. Both directions of #3818's own before/after diff, reproduced against the real CLI. Also pins the two properties the change must not break: a fully-failed patch still reports an empty updated array, which is what state.cts:607's success boolean depends on; and two content-identical writes differ in last_updated alone. That second one measured state_head NOT to be ambient — it is recomputed every write but only changes when git HEAD moved — so the provenance exclusion is a one-element set, with a companion test pinning that state_head does change when HEAD moves. Refs #3872 * feat(#3872): derive what a command reports from the transaction diff ADR-3473 §8.7. reconcileReportedFields compared the transform's own output against persisted bytes and then filtered what preservation had restored by its FIELD_CLASSIFICATION policy. Both are replaced by one comparison of persisted against the pre-write state the transaction already holds, surfaced to the command through the same caller-allocates out-param idiom divergedFields established. Both of the old directions fall out of that single comparison: a field the transform reported but the pipeline discarded is persisted-equals-snapshot and drops out, and a field nobody reported but the write moved is different and appears. The classification filter is deleted, not relocated — no policy test remains anywhere 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 defect: plannedPhaseCore already pushed progress.total_plans and reconcileReportedFields silently dropped it, because a flat hasOwnProperty cannot resolve a dotted key against nested frontmatter. Current Position was lost the same way and is fixed in the same place. The exclusion is one field, last_updated, and it is by provenance rather than by classification: it is the only field measured to change on every write regardless of content. state_head was measured NOT to qualify — it is recomputed every write but only changes when git HEAD moved. Without that exclusion state.patch's success boolean, which is updated.length > 0, would be permanently true and a fully-failed patch would report success. Refs #3872 * fix(#3872): cover the matrix, and close a prototype-chain read the coverage found Review found 20 of 29 test-matrix rows uncovered. Covering them found two real defects rather than merely documenting the intended behavior. bodyLabelFor read FRONTMATTER_KEY_TO_BODY_LABEL with a bare bracket index on a plain object literal, so a field named __proto__, constructor or toString resolved to the inherited prototype member and leaked a non-string value into the updated array. Fixed with an own-property check, mirroring the discipline resolveFrontmatterPath already had. The security-relevant matrix row proved it before the fix. applyPostSyncPreservation still carried its own inline copy of the value comparison alongside the new stateFieldValuesDiffer, which is two live copies of one rule introduced by the epic that exists to remove them. Routed through the single owner. Adds the fast-check property that a field appears iff its persisted value differs from the snapshot, the string-versus-number representation boundary, dotted paths into missing parents and into scalars, deleted and added keys, and the preserve-if-placeholder pair that proves no classification test survives in the reporting path. Refs #3872 * docs(#3872): document the transaction diff on the write path The updated array's contract belongs where the write path is described. States the iff rule, leaf granularity, the single provenance exclusion and why state_head is deliberately not one, and closes with the consequence a reader actually needs: these arrays are longer than they used to be, because they used to under-report. Refs #3872 * fix(#3872): a derived leaf materializing is not a change the caller made The remote matrix caught 17 failures with two causes. The substantive one is that progress is source: disk, and the disk cannot change during a STATE.md write — the write only touches STATE.md. So a progress block appearing where the snapshot had none is the scanner populating a document that had never been synced. The bytes moved; nothing the caller did moved them. That is the same shape as last_updated one level up, so the provenance rule is generalized rather than special-cased: a field appears iff its persisted value changed for a reason attributable to this write's action, and two cases are not attributable — a field stamped unconditionally on every save, and a declared derived leaf materializing from a source that did not change. Crucially this does not consult the preservation policy, so the filter §8.7 deleted stays deleted; it uses the declared leaf set to know which keys are derived. This had a second production consumer the earlier review concluded did not exist: cmdStatePlannedPhase gates publishStateContract on updated.length, and its own inline comment predicts exactly this failure. A no-op call was publishing state.json. advancePlanNoOpDoesNotPublish genuinely encoded pre-§8.7 behavior and moves. E2 and E6 had carved out total_plans as reportable-on-materialization, an error introduced earlier on this branch rather than a pre-existing pin, and are corrected with it. Refs #3872 * chore(#3872): backfill changeset PR number --------- Co-authored-by: sim <sim@local>
This commit is contained in:
5
.changeset/sharp-cranes-wander.md
Normal file
5
.changeset/sharp-cranes-wander.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Changed
|
||||
pr: 3878
|
||||
---
|
||||
**`state.*` commands now report every field they actually changed, and only those** — the `updated` array is derived by diffing what was persisted against the pre-write state, so a counter the write genuinely moved is no longer suppressed, a field the write merely preserved is no longer claimed as an update, and a changed sub-counter is named at leaf granularity (`progress.total_plans`) instead of being dropped. Callers that compared the array exactly will see more, and truer, entries. (#3872)
|
||||
File diff suppressed because one or more lines are too long
@@ -557,6 +557,14 @@ STATE.md carries the same fact in two places — YAML frontmatter and the docume
|
||||
|
||||
`rebuildStateTransaction()` is the typed form of ADR-3408 §8.3's closed exception list: `state sync`, which exists to let the body win, and `/gsd-health --repair`'s factory reset. Both are deliberate and permanent, not debt — and because the type names them, the write-path drift guard no longer has to track them as strings in a ratcheted baseline.
|
||||
|
||||
**What a command reports it wrote is the same snapshot, read back (ADR-3473 §8.7).** Every `state.*` command returns an `updated` array. That array is now derived by comparing what was actually persisted against the transaction's pre-write snapshot: **a field appears if, and only if, its persisted value changed.** One comparison answers both of the questions that used to need separate machinery — a field the caller asked for that the pipeline then discarded is persisted-equals-snapshot and drops out, and a field nobody asked for that the write moved anyway is different and appears. Nothing is filtered by its preservation policy.
|
||||
|
||||
Reporting is at **leaf granularity**: when a single counter moves you are told `progress.total_plans`, not `progress`. The leaves are the ones the field-classification table already declares, so the report is bounded by a schema rather than by walking the document.
|
||||
|
||||
One field is excluded, and it is excluded for its *provenance* rather than its policy: `last_updated` is stamped on every save regardless of what you changed, so admitting it would make `state patch`'s success signal — which is simply whether `updated` is non-empty — permanently true, and a patch in which every field failed would report success. `state_head` is deliberately **not** excluded: it is recomputed on every save but only *changes* when the commit it records actually moved, so reporting it tells you something true.
|
||||
|
||||
A practical consequence worth knowing: these arrays are now longer than they used to be, because they used to under-report. If you compare one exactly, expect more entries — and expect them to be the ones that really changed.
|
||||
|
||||
**[ADR-3408](adr/3408-state-write-path-preservation.md) is the normative contract** for that path: one executor per declared policy, one write seam, and reports computed from what was actually persisted rather than from what the caller intended to write. Where the contract and the code disagree, the code is the defect. It is the write-side counterpart of [ADR-3180](adr/3180-planning-semantic-model-single-owner.md), which gave each read-side derivation a single owner.
|
||||
|
||||
**[ADR-3473](adr/3473-enforcement-by-construction.md) owns the invariants that sit outside that contract.** ADR-3408 governs what survives a write; it does not govern the pipeline's precondition, what a command reports it wrote, or where the set of STATE.md keys, types and enums is declared. ADR-3473 owns those, alongside document parsing, enumeration, and the return contract of every routine that can fail. It is the third application of ADR-3180's mechanism and the first whose success metric requires the guard surface to *shrink* as each seam lands.
|
||||
|
||||
565
src/state.cts
565
src/state.cts
@@ -67,6 +67,9 @@ const { transitionCore, applyStatePreservation, sliceCurrentPositionSection } =
|
||||
// #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;
|
||||
// ADR-3473 §8.7 (#3872): the declared dotted-leaf enumeration `reconcileReportedFields`
|
||||
// diffs against — see `declaredLeavesOf` below.
|
||||
const { FIELD_CLASSIFICATION } = stateTransitionMod;
|
||||
type StateTransitionIntent = stateTransitionMod.StateTransitionIntent;
|
||||
type StateTransitionDeps = stateTransitionMod.StateTransitionDeps;
|
||||
type PhaseInventoryRecord = stateTransitionMod.PhaseInventoryRecord;
|
||||
@@ -115,6 +118,25 @@ interface StateLockClock {
|
||||
sleep(ms: number): void;
|
||||
}
|
||||
|
||||
/**
|
||||
* ADR-3473 §8.7 (#3872): the transaction's pre-write snapshot, threaded from
|
||||
* `applyPostSyncPreservation` to `reconcileReportedFields` via
|
||||
* `ReadModifyWriteOptions.preWriteState` (see that field's own docstring for
|
||||
* the out-param idiom). `bodyDeltas` is the SAME pre/post body-source map
|
||||
* `applyPostSyncPreservation` builds for preservation — reused (not
|
||||
* re-derived) as the "did THIS write's own transform actually change the
|
||||
* body source" signal for every `FRONTMATTER_BODY_SOURCE` key, since those
|
||||
* keys are re-derived into frontmatter from the body on EVERY write
|
||||
* regardless of whether this write touched them (see
|
||||
* `computeChangedFrontmatterFields`'s docstring for why a raw frontmatter
|
||||
* diff over-reports for this key set).
|
||||
*/
|
||||
interface StatePreWriteSnapshot {
|
||||
fm?: Record<string, unknown>;
|
||||
body?: string;
|
||||
bodyDeltas?: Record<string, { pre: string | null; post: string | null }>;
|
||||
}
|
||||
|
||||
interface ReadModifyWriteOptions {
|
||||
resync?: boolean;
|
||||
/** #2440: when true, total_plans/total_phases take derived values even under !resync. */
|
||||
@@ -156,6 +178,24 @@ interface ReadModifyWriteOptions {
|
||||
* `resync` is driven by `shouldResyncStateProgress` rather than defaulting.
|
||||
*/
|
||||
explicitProgressField?: boolean;
|
||||
/**
|
||||
* ADR-3473 §8.7 (issue #3872): caller-allocated out-param, following the
|
||||
* SAME idiom as `divergedFields` above (caller allocates an empty box,
|
||||
* the pipeline fills it) — `applyPostSyncPreservation` populates `.fm`
|
||||
* with the transaction's OWN pre-write snapshot (`transaction.snapshot`,
|
||||
* the exact object `openStateTransaction`/`rebuildStateTransaction` was
|
||||
* constructed with — never a second `extractFrontmatter` derivation of
|
||||
* `originalContent`, which is the divergence this epic exists to remove)
|
||||
* and `.body` with the pre-write BODY (`stripFrontmatter(originalContent)`)
|
||||
* that the body-label comparison in `reconcileReportedFields` needs and
|
||||
* the frontmatter snapshot does not carry. Left entirely `undefined` when
|
||||
* `readModifyWriteStateMd`'s own #948 no-op guard fires (the transform's
|
||||
* raw output was byte-identical to the input, so nothing was ever
|
||||
* synced/preserved/written) — a consumer must treat "unset" as "nothing
|
||||
* changed", never coerce it to an empty snapshot (an empty `{}` would
|
||||
* make every already-persisted key look newly ADDED per §8.7's diff).
|
||||
*/
|
||||
preWriteState?: StatePreWriteSnapshot;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -576,14 +616,15 @@ function cmdStatePatch(cwd: string, patches: Record<string, string>, raw: boolea
|
||||
// delta (table-driven) that this phase adds. Field-name validation (security)
|
||||
// and the resync-progress decision stay in this adapter.
|
||||
let precomputed: { updated: string[]; failed: string[] } = { updated: [], failed: [] };
|
||||
let preSyncContent = '';
|
||||
const divergedFields: string[] = [];
|
||||
// ADR-3473 §8.7 (#3872): caller-allocated out-param, filled with the
|
||||
// transaction's own pre-write snapshot + body by `applyPostSyncPreservation`.
|
||||
const preWriteState: StatePreWriteSnapshot = {};
|
||||
readModifyWriteStateMd(statePath, (content) => {
|
||||
const result = transitionCore(content, { kind: 'patch', patches }, { clock: realClock });
|
||||
precomputed = (result.data as { updated: string[]; failed: string[] }) ?? precomputed;
|
||||
preSyncContent = result.content;
|
||||
return result.content;
|
||||
}, cwd, { resync: shouldResync, divergedFields, explicitProgressField: shouldResync });
|
||||
}, cwd, { resync: shouldResync, divergedFields, explicitProgressField: shouldResync, preWriteState });
|
||||
|
||||
// ADR-3408 §8.4 (D4, fix(#3351) generalized — see `reconcileReportedFields`):
|
||||
// patchCore's bookkeeping says whether the stateReplaceField text-replace
|
||||
@@ -599,7 +640,7 @@ function cmdStatePatch(cwd: string, patches: Record<string, string>, raw: boolea
|
||||
// `applyStatePreservation` restored that this patch never named at all
|
||||
// (#3345's direction), a case the pre-#3471 version of this command never
|
||||
// covered.
|
||||
const updated = reconcileReportedFields(statePath, preSyncContent, precomputed.updated, divergedFields);
|
||||
const updated = reconcileReportedFields(statePath, preWriteState, precomputed.updated, divergedFields);
|
||||
const updatedSet = new Set(updated);
|
||||
const failed = Object.keys(patches).filter((field) => !updatedSet.has(field));
|
||||
const results = { updated, failed };
|
||||
@@ -674,7 +715,9 @@ function cmdStateUpdate(cwd: string, field: string | undefined, value: string |
|
||||
const statePath = planningPaths(cwd).state;
|
||||
try {
|
||||
let updated = false;
|
||||
let preSyncContent = '';
|
||||
// ADR-3473 §8.7 (#3872): caller-allocated out-param, filled with the
|
||||
// transaction's own pre-write snapshot + body by `applyPostSyncPreservation`.
|
||||
const preWriteState: StatePreWriteSnapshot = {};
|
||||
let transitionData: Record<string, unknown> | undefined;
|
||||
// #3699 case D: when `updateCore` falls back to writing the frontmatter key
|
||||
// directly, the value must survive the post-sync pass — otherwise the write
|
||||
@@ -715,9 +758,8 @@ function cmdStateUpdate(cwd: string, field: string | undefined, value: string |
|
||||
if (transitionData?.wroteFrontmatter === true) {
|
||||
authoritativeFm[field as string] = value;
|
||||
}
|
||||
preSyncContent = result.content;
|
||||
return result.content;
|
||||
}, cwd, { resync: shouldResync, divergedFields, authoritativeFm, explicitProgressField: shouldResync });
|
||||
}, cwd, { resync: shouldResync, divergedFields, authoritativeFm, explicitProgressField: shouldResync, preWriteState });
|
||||
|
||||
// ADR-3408 §8.4 (D4): reconcile against the bytes actually persisted —
|
||||
// `updateCore`'s own match does not know whether sync/preservation later
|
||||
@@ -726,7 +768,7 @@ function cmdStateUpdate(cwd: string, field: string | undefined, value: string |
|
||||
// restored during this write that this command never touched at all
|
||||
// (#3345's direction) — reported separately from `updated` because this
|
||||
// command's contract is a single-field boolean, not a per-field array.
|
||||
const reconciled = reconcileReportedFields(statePath, preSyncContent, updated ? [field as string] : [], divergedFields);
|
||||
const reconciled = reconcileReportedFields(statePath, preWriteState, updated ? [field as string] : [], divergedFields);
|
||||
updated = reconciled.includes(field as string);
|
||||
const preserved = reconciled.filter((f) => f !== field);
|
||||
|
||||
@@ -803,8 +845,10 @@ function cmdStateAdvancePlan(cwd: string, raw: boolean): void {
|
||||
|
||||
let resultData: Record<string, unknown> | undefined;
|
||||
let precomputedUpdated: string[] = [];
|
||||
let preSyncContent = '';
|
||||
const divergedFields: string[] = [];
|
||||
// ADR-3473 §8.7 (#3872): caller-allocated out-param, filled with the
|
||||
// transaction's own pre-write snapshot + body by `applyPostSyncPreservation`.
|
||||
const preWriteState: StatePreWriteSnapshot = {};
|
||||
// #3311: the milestone (phase + session) claim is consulted INSIDE the
|
||||
// STATE.md lock, so the position read and the claim read cannot interleave
|
||||
// with another session's Current Position write.
|
||||
@@ -827,9 +871,8 @@ function cmdStateAdvancePlan(cwd: string, raw: boolean): void {
|
||||
const result = transitionCore(content, intent, deps);
|
||||
resultData = result.data;
|
||||
precomputedUpdated = result.updated;
|
||||
preSyncContent = result.content;
|
||||
return result.content;
|
||||
}, cwd, { divergedFields });
|
||||
}, cwd, { divergedFields, preWriteState });
|
||||
|
||||
if (!resultData || resultData['error']) {
|
||||
output({ error: 'Cannot parse Current Plan or Total Plans in Phase from STATE.md' }, raw, undefined);
|
||||
@@ -842,7 +885,7 @@ function cmdStateAdvancePlan(cwd: string, raw: boolean): void {
|
||||
// Generalizes fix(#3351) (closes #3351's direction) and folds in any field
|
||||
// preservation restored that this transform never touched (#3345's
|
||||
// direction).
|
||||
const updated = reconcileReportedFields(statePath, preSyncContent, precomputedUpdated, divergedFields);
|
||||
const updated = reconcileReportedFields(statePath, preWriteState, precomputedUpdated, divergedFields);
|
||||
|
||||
if (resultData['advanced'] === false) {
|
||||
output({ ...resultData, updated, milestone_conflict: milestoneConflict }, raw, 'false');
|
||||
@@ -1531,8 +1574,10 @@ function cmdStateRecordSession(cwd: string, options: StateRecordSessionOptions,
|
||||
const now = realClock.nowIso();
|
||||
const updated: string[] = [];
|
||||
let sessionCreated = false;
|
||||
let preSyncContent = '';
|
||||
const divergedFields: string[] = [];
|
||||
// ADR-3473 §8.7 (#3872): caller-allocated out-param, filled with the
|
||||
// transaction's own pre-write snapshot + body by `applyPostSyncPreservation`.
|
||||
const preWriteState: StatePreWriteSnapshot = {};
|
||||
|
||||
readModifyWriteStateMd(statePath, (content) => {
|
||||
// Update Last session / Last Date
|
||||
@@ -1738,15 +1783,14 @@ function cmdStateRecordSession(cwd: string, options: StateRecordSessionOptions,
|
||||
}
|
||||
}
|
||||
|
||||
preSyncContent = content;
|
||||
return content;
|
||||
}, cwd, { divergedFields });
|
||||
}, cwd, { divergedFields, preWriteState });
|
||||
|
||||
// ADR-3408 §8.4 (D4): reconcile this command's own success list against the
|
||||
// bytes actually persisted (fix(#3351) generalized) and fold in any field
|
||||
// preservation restored that this transform never touched (#3345's
|
||||
// direction).
|
||||
const reconciledUpdated = reconcileReportedFields(statePath, preSyncContent, updated, divergedFields);
|
||||
const reconciledUpdated = reconcileReportedFields(statePath, preWriteState, updated, divergedFields);
|
||||
|
||||
if (reconciledUpdated.length > 0) {
|
||||
const result: Record<string, unknown> = { recorded: true, updated: reconciledUpdated };
|
||||
@@ -3325,7 +3369,7 @@ function applyPostSyncPreservation(
|
||||
options: StatePreservationOptions,
|
||||
): string {
|
||||
assertStatePreservationOptions(options, 'applyPostSyncPreservation');
|
||||
const { resync, authoritativeFm, deriveProgressKeys, divergedFields, explicitProgressField } = options;
|
||||
const { resync, authoritativeFm, deriveProgressKeys, divergedFields, explicitProgressField, preWriteState } = options;
|
||||
|
||||
// Bug #1230: delta heuristic — snapshot pre-transform body source fields so
|
||||
// we can detect whether THIS write changed them. syncStateFrontmatter
|
||||
@@ -3421,6 +3465,16 @@ function applyPostSyncPreservation(
|
||||
status: { pre: preBodyStatus, post: postBodyStatus },
|
||||
stopped_at: { pre: preBodyStoppedAt, post: postBodyStoppedAt },
|
||||
current_phase_name: { pre: preBodyPhaseSource, post: postBodyPhaseSource },
|
||||
// ADR-3473 §8.7 (#3872): `last_activity` is the one `FRONTMATTER_BODY_SOURCE`
|
||||
// key that is NOT `preserve-when-unchanged` (it is `derive` — always
|
||||
// re-stamped from the body) and so was never part of this map before.
|
||||
// Added ONLY for `reconcileReportedFields`'s consumption below (via
|
||||
// `preWriteState.bodyDeltas`) — harmless here, since
|
||||
// `applyPreserveWhenUnchanged` is dispatched by
|
||||
// `getPreserveWhenUnchangedFields()`, never by iterating this object's
|
||||
// keys, so an extra non-preserve-when-unchanged entry changes no
|
||||
// preservation behavior.
|
||||
last_activity: { pre: preBodyLastActivityRaw, post: postBodyLastActivityRaw },
|
||||
};
|
||||
|
||||
// ADR-1769 #1796 (Path A — finish the consolidation): the post-sync
|
||||
@@ -3454,6 +3508,20 @@ function applyPostSyncPreservation(
|
||||
bodyDeltas,
|
||||
explicitProgressField: explicitProgressField === true,
|
||||
});
|
||||
// ADR-3473 §8.7 (#3872): fill the caller's out-param with the TRANSACTION'S
|
||||
// OWN snapshot object (not a second `extractFrontmatter(originalContent)`
|
||||
// derivation — `transaction.snapshot === preFmSnapshot`, reusing it is the
|
||||
// whole point) plus the pre-write body, so `reconcileReportedFields` can
|
||||
// diff persisted-vs-pre-write instead of re-deriving either side itself.
|
||||
if (preWriteState) {
|
||||
preWriteState.fm = transaction.snapshot;
|
||||
preWriteState.body = preBody;
|
||||
// ADR-3473 §8.7 (#3872): the pre/post body-source delta for every
|
||||
// FRONTMATTER_BODY_SOURCE key — see `StatePreWriteSnapshot`'s docstring
|
||||
// for why `reconcileReportedFields` needs this instead of a raw
|
||||
// frontmatter diff for these specific keys.
|
||||
preWriteState.bodyDeltas = bodyDeltas;
|
||||
}
|
||||
const preservation = applyStatePreservation({ transaction, postFm });
|
||||
if (divergedFields && preservationInputSnapshot) {
|
||||
// §8.5's "liberal but visible": every field whose value actually
|
||||
@@ -3466,10 +3534,13 @@ function applyPostSyncPreservation(
|
||||
for (const key of Object.keys(preservation.postFm)) {
|
||||
const before = preservationInputSnapshot[key];
|
||||
const after = preservation.postFm[key];
|
||||
const changed = (typeof before === 'object' || typeof after === 'object')
|
||||
? JSON.stringify(before) !== JSON.stringify(after)
|
||||
: before !== after;
|
||||
if (changed) divergedFields.push(key);
|
||||
// ADR-3473 §8.7 (#3872 standards-axis finding): route through the ONE
|
||||
// owner of this comparison rule (`stateFieldValuesDiffer`, defined
|
||||
// below) instead of carrying a second inline `JSON.stringify`-vs-`!==`
|
||||
// copy — this is exactly the duplicated-rule shape this epic exists to
|
||||
// remove. `stateFieldValuesDiffer` is a function declaration (hoisted),
|
||||
// so calling it here, above its textual definition, is safe.
|
||||
if (stateFieldValuesDiffer(before, after)) divergedFields.push(key);
|
||||
}
|
||||
// ADR-3408 §8.5 Row 2 (D1's actual bug, the reason the guards had to be
|
||||
// deleted rather than merely relocated): the loop above can only see a
|
||||
@@ -3619,6 +3690,11 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string
|
||||
deriveProgressKeys: options?.deriveProgressKeys === true,
|
||||
divergedFields: options?.divergedFields,
|
||||
explicitProgressField: options?.explicitProgressField === true,
|
||||
// ADR-3473 §8.7 (#3872): forwarded so `applyPostSyncPreservation` can
|
||||
// fill it — an unenumerated option here is silently dropped
|
||||
// (Phase 1's commit message; #3871), which is exactly how a prior cut
|
||||
// of this option would have gone missing.
|
||||
preWriteState: options?.preWriteState,
|
||||
},
|
||||
);
|
||||
|
||||
@@ -3676,8 +3752,19 @@ const FRONTMATTER_KEY_TO_BODY_LABEL: Readonly<Record<string, string>> = Object.f
|
||||
* (e.g. `progress`), not a silent degrade.
|
||||
*/
|
||||
function bodyLabelFor(field: string): string {
|
||||
const label = FRONTMATTER_KEY_TO_BODY_LABEL[field];
|
||||
if (label !== undefined) return label;
|
||||
// ADR-3473 §8.7 (#3872 review): an OWN-PROPERTY check, never a bare
|
||||
// bracket read — `FRONTMATTER_KEY_TO_BODY_LABEL` is a plain object literal
|
||||
// (real `Object.prototype` in its chain), so `[field]` for a hostile field
|
||||
// named `__proto__`/`constructor`/`toString` returns the INHERITED
|
||||
// prototype-chain member (`Object.prototype` itself, the `Object`
|
||||
// constructor function, `Object.prototype.toString`) instead of
|
||||
// `undefined` — which would then be returned as the "label" and leak a
|
||||
// non-string value into the caller's `updated` array. Proven by
|
||||
// `dottedResolutionDoesNotPollutePrototypes` (test matrix row 25) before
|
||||
// this fix. Mirrors `resolveFrontmatterPath`'s own-property discipline.
|
||||
if (Object.prototype.hasOwnProperty.call(FRONTMATTER_KEY_TO_BODY_LABEL, field)) {
|
||||
return FRONTMATTER_KEY_TO_BODY_LABEL[field];
|
||||
}
|
||||
const cls = stateTransitionMod.getFieldClassification(field);
|
||||
if (cls && cls.preservation === 'preserve-when-unchanged') {
|
||||
const err = new Error(
|
||||
@@ -3693,102 +3780,347 @@ function bodyLabelFor(field: string): string {
|
||||
}
|
||||
|
||||
/**
|
||||
* ADR-3408 §8.4 (D4): shared persisted-bytes reconciliation, generalized
|
||||
* from fix(#3351)'s `cmdStatePatch`-only version so every RMW-based command
|
||||
* that reports a per-field `updated` array shares ONE comparison instead of
|
||||
* re-deriving it per call site — duplicated policy is exactly what this
|
||||
* epic exists to remove.
|
||||
* ADR-3473 §8.7 (issue #3872): the provenance exclusion — the ONLY
|
||||
* frontmatter key measured to change on EVERY write, regardless of content.
|
||||
* Verified at the CLI (`40-design.md` "Two corrections from reproducing it"):
|
||||
* two content-identical writes to a git-backed fixture differ in exactly
|
||||
* this one key. `state_head` was deliberately measured OUT of this set —
|
||||
* it restamps every write but its PERSISTED VALUE changes only when git HEAD
|
||||
* actually moved, so it tracks a real fact and does not flood.
|
||||
*
|
||||
* Closes BOTH directions:
|
||||
* - **#3351's** (reported-but-discarded): a field the transform's own
|
||||
* return value held a value for, that sync/preservation then discarded
|
||||
* or overwrote before the file was saved, must NOT be reported.
|
||||
* - **#3345's** (persisted-but-unreported): a field `applyStatePreservation`
|
||||
* restored that the transform never touched at all must still be
|
||||
* reported — preservation can mutate a field the pre-sync intent never
|
||||
* knew about.
|
||||
* A CLOSED, ENUMERATED set — not a predicate or a callback (Greenspun's
|
||||
* Tenth Rule, ADR-3473 §8.7's Laws section: "the moment it takes a callback
|
||||
* it has become the classification table again under a new name"). It
|
||||
* exists to protect `src/state.cts:607` — `state.patch`'s ENTIRE
|
||||
* success/failure signal is `results.updated.length > 0` — admitting an
|
||||
* always-changing key here would make that boolean permanently `true`, so a
|
||||
* fully-failed patch would report success.
|
||||
*/
|
||||
const STATE_UPDATED_PROVENANCE_EXCLUSION: readonly string[] = Object.freeze(['last_updated']);
|
||||
|
||||
/** Sentinel: "this dotted path did not resolve to any value" — distinct from every real value including `undefined`/`null`, so absence and an explicit null are never confused. */
|
||||
const STATE_FIELD_ABSENT: unique symbol = Symbol('state-field-absent');
|
||||
|
||||
/**
|
||||
* ADR-3473 §8.7 (#3872): resolve `path` against a parsed frontmatter object.
|
||||
* Pure, never throws.
|
||||
*
|
||||
* @param preSyncContent The transformFn's OWN return value — the content
|
||||
* BEFORE `syncAndPreserveStateMd` ran this write — captured by the caller
|
||||
* inside its own `readModifyWriteStateMd` callback. Comparing that against
|
||||
* the actual bytes on disk after the full write pipeline settled is what
|
||||
* makes the report reflect what POST-sync bytes hold (ADR-3408 §8.4),
|
||||
* not what the pre-sync intent merely hoped for.
|
||||
* @param reported The candidate field names — the transform's OWN
|
||||
* success list (e.g. `beginPhaseCore`'s `updated`), never a raw intent
|
||||
* list the transform might not have actually matched. Body Title-Case
|
||||
* labels (`Status`, `Current Plan`) and frontmatter keys are both valid;
|
||||
* each is looked up as a frontmatter key first, else as a body field —
|
||||
* the same fallback chain `cmdStatePatch` used before this generalization.
|
||||
* @param divergedFields Frontmatter field names `applyStatePreservation`
|
||||
* actually restored during this write (ADR-3408 §8.5's out-param).
|
||||
* Order is pinned (test matrix row 26, `literalDottedKeyResolvesBeforePathTraversal`):
|
||||
* a LITERAL flat key wins first — a field name that happens to contain a `.`
|
||||
* but is stored as one flat key must not be shadowed by path traversal —
|
||||
* and only when no literal key exists does `path` get split and walked as a
|
||||
* dotted path.
|
||||
*
|
||||
* Hostile-input rows (23-25 of the test matrix) all resolve to
|
||||
* `STATE_FIELD_ABSENT` rather than throwing: a missing parent, a scalar
|
||||
* parent (`typeof cursor !== 'object'`), and — the prototype-pollution
|
||||
* case — a `__proto__`/`constructor`/`toString` segment. The own-property
|
||||
* check (`Object.prototype.hasOwnProperty.call`, never a bare `in` or
|
||||
* bracket read) is what makes the last one safe: an inherited
|
||||
* `Object.prototype` member is never mistaken for an own data key, and
|
||||
* because this function only ever READS a segment (never assigns one),
|
||||
* no prototype can be polluted by walking it.
|
||||
*/
|
||||
function resolveFrontmatterPath(fm: Record<string, unknown>, path: string): unknown {
|
||||
if (Object.prototype.hasOwnProperty.call(fm, path)) return fm[path];
|
||||
if (!path.includes('.')) return STATE_FIELD_ABSENT;
|
||||
let cursor: unknown = fm;
|
||||
for (const segment of path.split('.')) {
|
||||
if (typeof cursor !== 'object' || cursor === null || Array.isArray(cursor)) return STATE_FIELD_ABSENT;
|
||||
if (!Object.prototype.hasOwnProperty.call(cursor, segment)) return STATE_FIELD_ABSENT;
|
||||
cursor = (cursor as Record<string, unknown>)[segment];
|
||||
}
|
||||
return cursor;
|
||||
}
|
||||
|
||||
/**
|
||||
* ADR-3473 §8.7 (#3872): representation-insensitive equality for a
|
||||
* persisted-vs-snapshot leaf value (test matrix rows 21/22). Frontmatter
|
||||
* scalars round-trip as STRINGS (`extractFrontmatter`, §8.1's open type
|
||||
* question) while an in-memory derivation can hold a real number or boolean
|
||||
* — a naive `!==` would report every numeric/boolean field changed on every
|
||||
* write. Mirrors the existing `divergedFields` diff's typeof-object branch
|
||||
* in `applyPostSyncPreservation` (JSON.stringify for objects, else a
|
||||
* normalized scalar compare) rather than inventing a second comparison.
|
||||
* Presence-vs-absence (`STATE_FIELD_ABSENT` on exactly one side) is always a
|
||||
* change — a deleted or newly-added key (test matrix rows 16/17) — never
|
||||
* folded into the scalar branch below it.
|
||||
*/
|
||||
/**
|
||||
* ADR-3473 §8.7 (#3872): `String(v)` on an `unknown` is unsafe (a hostile
|
||||
* object could carry a custom, throwing, or `[object Object]`-degrading
|
||||
* `toString`) — narrowed per-branch here so each `String()` call below only
|
||||
* ever runs on a primitive TypeScript itself knows is safe to stringify.
|
||||
*/
|
||||
function stateScalarString(v: unknown): string {
|
||||
if (v === null || v === undefined) return '';
|
||||
if (typeof v === 'string') return v;
|
||||
if (typeof v === 'number' || typeof v === 'boolean' || typeof v === 'bigint') return String(v);
|
||||
return JSON.stringify(v) ?? '';
|
||||
}
|
||||
|
||||
function stateFieldValuesDiffer(before: unknown, after: unknown): boolean {
|
||||
if (before === STATE_FIELD_ABSENT && after === STATE_FIELD_ABSENT) return false;
|
||||
if (before === STATE_FIELD_ABSENT || after === STATE_FIELD_ABSENT) return true;
|
||||
if (typeof before === 'object' || typeof after === 'object') {
|
||||
return JSON.stringify(before) !== JSON.stringify(after);
|
||||
}
|
||||
return stateScalarString(before).trim() !== stateScalarString(after).trim();
|
||||
}
|
||||
|
||||
/**
|
||||
* ADR-3473 §8.7 (#3872): the declared dotted-leaf children of a frontmatter
|
||||
* key, read off `FIELD_CLASSIFICATION` (`progress` -> its five
|
||||
* `progress.*` rows) rather than walked from arbitrary nesting depth of a
|
||||
* user-authored document. A BOUNDED, DECLARED enumeration — the design
|
||||
* doc's Rejected #5 and the "Emit dotted leaves, not the parent" rule both
|
||||
* depend on this staying a closed set the schema names, not unbounded
|
||||
* traversal of whatever object shape happens to be on disk.
|
||||
*/
|
||||
function declaredLeavesOf(key: string): string[] {
|
||||
const prefix = `${key}.`;
|
||||
return Object.keys(FIELD_CLASSIFICATION).filter((k) => k.startsWith(prefix));
|
||||
}
|
||||
|
||||
/**
|
||||
* ADR-3473 §8.7 (#3872): every frontmatter key — resolved at DOTTED-LEAF
|
||||
* granularity for a key with declared leaves (`progress` -> only the
|
||||
* `progress.*` leaves that actually moved, never bare `progress` itself;
|
||||
* design doc rule 4/Rejected #5) — whose PERSISTED value differs from the
|
||||
* transaction's pre-write SNAPSHOT. Pure: no I/O, no `FIELD_CLASSIFICATION`
|
||||
* preservation-policy consultation (that filter is exactly what this rule
|
||||
* deletes — ADR-3473 §8.7 "no field is excluded by classification").
|
||||
* `last_updated` is the one-element provenance exclusion; every other key,
|
||||
* including `state_head`, is a candidate.
|
||||
*
|
||||
* **A `FRONTMATTER_BODY_SOURCE` key is diffed via `bodyDeltas`, never via a
|
||||
* raw frontmatter compare.** Found while driving the #1264 regression check
|
||||
* through this rewrite at the CLI: `syncStateFrontmatter` re-derives EVERY
|
||||
* body-sourced key into frontmatter on EVERY write, independent of whether
|
||||
* this write's own transform touched it. A hand-authored (or day-1
|
||||
* bootstrap) STATE.md whose frontmatter has not yet caught up to an
|
||||
* already-stable body value — e.g. `current_phase_name` present in the body
|
||||
* but absent from a pre-write frontmatter block that only ever recorded
|
||||
* `status`/`progress` — makes that key look newly ADDED under a raw diff
|
||||
* (rows 15/17) even though nothing changed. The real "did THIS write change
|
||||
* it" signal for these keys is whether their BODY SOURCE moved, which is
|
||||
* exactly what `bodyDeltas` (built once, in `applyPostSyncPreservation`,
|
||||
* from `originalContent` vs `transformedContent`) already answers — reused
|
||||
* here rather than re-derived, and it is what correctly REPORTS #3818's
|
||||
* `current_phase` (the body source did move) while staying SILENT on a
|
||||
* merely-backfilled, body-unchanged key (the #1264 false positive this
|
||||
* function's first cut produced).
|
||||
*
|
||||
* **A declared dotted-leaf (`declaredLeavesOf`, e.g. every `progress.*` row)
|
||||
* absent from the snapshot and present in persisted is materialization, not
|
||||
* a change.** Found the same way as the paragraph above, one layer down:
|
||||
* `progress` is `source: 'disk'` (state-transition.cts), re-derived by
|
||||
* `buildStateFrontmatter`'s phase-directory scan on every write regardless
|
||||
* of whether the caller's own action touched it — and the phases directory
|
||||
* cannot move during a STATE.md write, so a fresh `progress` block appearing
|
||||
* where the snapshot had none is the scanner catching a never-synced
|
||||
* document up, not the caller changing anything. This is the SAME
|
||||
* provenance principle `STATE_UPDATED_PROVENANCE_EXCLUSION` applies to
|
||||
* `last_updated` (a field stamped by the write's occurrence, not its
|
||||
* action) — generalized to the declared-leaf case, deliberately NOT a
|
||||
* second classification-based exclusion: `progress`'s `preserve-always`
|
||||
* policy plays no part in the check below, and a leaf already PRESENT in
|
||||
* the snapshot is diffed exactly as every other field is, including
|
||||
* reporting its outright disappearance (row 16) — only the absent-in-
|
||||
* snapshot-but-materialized-in-persisted transition is suppressed.
|
||||
*/
|
||||
function computeChangedFrontmatterFields(
|
||||
snapshotFm: Record<string, unknown>,
|
||||
persistedFm: Record<string, unknown>,
|
||||
bodyDeltas: Record<string, { pre: string | null; post: string | null }> | undefined,
|
||||
): string[] {
|
||||
const changed: string[] = [];
|
||||
const topKeys = new Set([...Object.keys(snapshotFm), ...Object.keys(persistedFm)]);
|
||||
for (const key of topKeys) {
|
||||
if (STATE_UPDATED_PROVENANCE_EXCLUSION.includes(key)) continue;
|
||||
|
||||
if (stateTransitionMod.getFrontmatterBodySource(key) !== null) {
|
||||
const delta = bodyDeltas ? bodyDeltas[key] : undefined;
|
||||
if (delta && stateFieldValuesDiffer(delta.pre ?? STATE_FIELD_ABSENT, delta.post ?? STATE_FIELD_ABSENT)) {
|
||||
changed.push(key);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
|
||||
const leaves = declaredLeavesOf(key);
|
||||
if (leaves.length > 0) {
|
||||
for (const leaf of leaves) {
|
||||
const before = resolveFrontmatterPath(snapshotFm, leaf);
|
||||
const after = resolveFrontmatterPath(persistedFm, leaf);
|
||||
// Generalizes the SAME provenance principle STATE_UPDATED_PROVENANCE_EXCLUSION
|
||||
// applies to `last_updated` one level up — this is NOT a classification-based
|
||||
// exclusion (progress's `preserve-always` policy plays no part here; that filter
|
||||
// stays deleted per §8.7). It is a fact about the DECLARED LEAF SET: every key
|
||||
// enumerated by `declaredLeavesOf` is `source: 'disk'` (state-transition.cts),
|
||||
// re-derived from a scan that cannot move during a STATE.md write (the write only
|
||||
// touches STATE.md, never the phases directory). So a leaf ABSENT from the
|
||||
// pre-write snapshot and PRESENT in persisted is the scanner catching a document
|
||||
// up to a derivation it had never synced before — the write's own OCCURRENCE
|
||||
// produced the bytes, not the caller's ACTION, exactly the `last_updated` shape.
|
||||
// A leaf already PRESENT in the snapshot behaves normally: any difference
|
||||
// (including disappearing entirely, row 16) is reported, because there the
|
||||
// snapshot proves the derivation had already run once, so a new persisted value
|
||||
// can only come from something genuinely moving (#3743/#3818).
|
||||
if (before === STATE_FIELD_ABSENT && after !== STATE_FIELD_ABSENT) continue;
|
||||
if (stateFieldValuesDiffer(before, after)) changed.push(leaf);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
const before = resolveFrontmatterPath(snapshotFm, key);
|
||||
const after = resolveFrontmatterPath(persistedFm, key);
|
||||
if (stateFieldValuesDiffer(before, after)) changed.push(key);
|
||||
}
|
||||
return changed;
|
||||
}
|
||||
|
||||
/**
|
||||
* ADR-3473 §8.7 (issue #3872): the transaction diff. `updated` is derived
|
||||
* by comparing PERSISTED frontmatter against the transaction's pre-write
|
||||
* SNAPSHOT — replacing the prior comparison of the transform's own OUTPUT
|
||||
* against persisted bytes, which answered a different question ("did the
|
||||
* transform's write survive to disk", #3351) from the one §8.7 asks ("what
|
||||
* did this write actually change" — both #3351's direction and #3345/#3818's
|
||||
* fall out of ONE comparison against the pre-write state; see the design
|
||||
* doc's "ambiguity in §8.7" section for why the transform-output comparison
|
||||
* was rejected).
|
||||
*
|
||||
* No field is excluded by classification — `getFieldClassification` /
|
||||
* `preservation !== 'preserve-when-unchanged'` is gone, not relocated. The
|
||||
* ONLY exclusion is `STATE_UPDATED_PROVENANCE_EXCLUSION` (provenance, not
|
||||
* classification): an unchanged `progress` no longer needs a special filter
|
||||
* to stay unreported (#1264) because the diff itself says "unchanged" —
|
||||
* and a GENUINELY changed `progress.*` leaf (#3743, #3818) is no longer
|
||||
* suppressed by the same filter.
|
||||
*
|
||||
* @param preWriteState The transaction's pre-write snapshot + body — the
|
||||
* `preWriteState` out-param `applyPostSyncPreservation` filled during
|
||||
* THIS write (see `ReadModifyWriteOptions.preWriteState`'s docstring).
|
||||
* `.fm`/`.body` are `undefined` only when `readModifyWriteStateMd`'s own
|
||||
* #948 no-op guard fired (transform output was byte-identical to input),
|
||||
* in which case nothing was ever written and `[]` is the correct,
|
||||
* short-circuited answer — never a diff against a synthesized empty `{}`
|
||||
* snapshot, which would read every already-persisted key as newly ADDED.
|
||||
* @param reported The candidate field names — the transform's OWN success
|
||||
* list. Body Title-Case labels (`Status`, `Current Plan`, `Current
|
||||
* Position`) and frontmatter keys (including dotted leaves like
|
||||
* `progress.total_plans`) are both valid; each is resolved via the same
|
||||
* `valueOf` fallback chain used for the inclusion test below.
|
||||
* @param divergedFields Kept for signature/out-param stability (ADR-3408
|
||||
* §8.5) — populated exactly as before by `applyPostSyncPreservation` and
|
||||
* still read directly by other code and `tests/state.test.cjs`'s A2f case
|
||||
* — but no longer consulted here as a candidate SOURCE (design doc row
|
||||
* 18): the frontmatter diff subsumes what it used to contribute, and it
|
||||
* sees only what *preservation* changed, never what *sync* changed
|
||||
* (#3818's own direction), which is why keeping it as the candidate
|
||||
* source was rejected (design doc, Rejected #1).
|
||||
*/
|
||||
function reconcileReportedFields(
|
||||
statePath: string,
|
||||
preSyncContent: string,
|
||||
preWriteState: StatePreWriteSnapshot,
|
||||
reported: string[],
|
||||
divergedFields: string[],
|
||||
): string[] {
|
||||
void divergedFields; // ADR-3473 §8.7 D18: out-param only, not a candidate source here.
|
||||
if (preWriteState.fm === undefined || preWriteState.body === undefined) return [];
|
||||
|
||||
const persisted = platformReadSync(statePath) || '';
|
||||
const persistedFm = extractFrontmatter(persisted, statePath) as Record<string, unknown>;
|
||||
const persistedBody = stripFrontmatter(persisted);
|
||||
const preFm = extractFrontmatter(preSyncContent, statePath) as Record<string, unknown>;
|
||||
const preBody = stripFrontmatter(preSyncContent);
|
||||
const snapshotFm = preWriteState.fm;
|
||||
const snapshotBody = preWriteState.body;
|
||||
|
||||
// #3471 review: body-FIRST, frontmatter-fallback — mirrors the actual write
|
||||
// precedence `patchCore`/`updateCore` apply (their own docstrings: "a key
|
||||
// that resolves against the STRIPPED body ... wins deterministically even
|
||||
// when the same key also happens to exist as a parsed frontmatter key").
|
||||
// The prior frontmatter-first order was silently correct for every
|
||||
// Title-Case body label (`Status`, `Current Plan`, ...) only because those
|
||||
// never case-exact-match a frontmatter key (frontmatter keys are always
|
||||
// lowercase snake_case) — so `hasOwnProperty` always missed and it fell
|
||||
// through to the body anyway. It broke the one case where `field` IS
|
||||
// lowercase and DOES exact-match a frontmatter key: a table-format
|
||||
// STATE.md with a lowercase field name (e.g. `state update status ...`
|
||||
// against `| status | ... |`). There, `preFm` (extracted from the
|
||||
// transform's own pre-sync output) never has a `status` key yet — but
|
||||
// `persistedFm` (extracted after `syncStateFrontmatter` ran) always does,
|
||||
// since `status` is a schema-owned frontmatter key re-derived on every
|
||||
// write. Reading frontmatter first made `intended` (body text) and
|
||||
// `persistedValue` (frontmatter-derived enum) compare two different
|
||||
// representations of the same field, and the write was never reconciled
|
||||
// (regression: #1162's "state update is case-insensitive for table field
|
||||
// names").
|
||||
// #3471 review (unchanged by this rewrite): body-FIRST, frontmatter-key-
|
||||
// FLAT-fallback, dotted-PATH-fallback last. Body-first mirrors the actual
|
||||
// write precedence `patchCore`/`updateCore` apply (#1162's fix — a
|
||||
// lowercase body label that happens to case-exact-match a frontmatter key
|
||||
// must still resolve against the body). `field` a literal flat key (even
|
||||
// one containing a `.`) is tried before it is split and walked as a
|
||||
// dotted path (test matrix row 26) — `resolveFrontmatterPath` pins that
|
||||
// same order for the frontmatter side alone.
|
||||
//
|
||||
// `Current Position` is special-cased: it names the WHOLE `## Current
|
||||
// Position` section, not a single `Label: value` line, so
|
||||
// `stateExtractField` can never resolve it (this is the root cause of the
|
||||
// "Current Position undercount" — a transform can correctly push
|
||||
// `'Current Position'` into its own `updated` list, and this function
|
||||
// still silently dropped it, because `valueOf` returned `null` for BOTH
|
||||
// sides and `null === null` failed the old `intended !== null` guard).
|
||||
// `sliceCurrentPositionSection` is the existing fence-aware section
|
||||
// locator (state-transition.cts) — reused rather than re-derived.
|
||||
const valueOf = (fm: Record<string, unknown>, body: string, field: string): string | null => {
|
||||
if (field === 'Current Position') {
|
||||
const section = stateTransitionMod.sliceCurrentPositionSection(body);
|
||||
return section !== null ? section.trim() : null;
|
||||
}
|
||||
const bodyValue = stateExtractField(body, field);
|
||||
if (bodyValue !== null) return bodyValue;
|
||||
return Object.prototype.hasOwnProperty.call(fm, field) ? String(fm[field]) : null;
|
||||
if (Object.prototype.hasOwnProperty.call(fm, field)) return String(fm[field]);
|
||||
if (field.includes('.')) {
|
||||
const resolved = resolveFrontmatterPath(fm, field);
|
||||
if (resolved !== STATE_FIELD_ABSENT) {
|
||||
return stateScalarString(resolved);
|
||||
}
|
||||
}
|
||||
return null;
|
||||
};
|
||||
|
||||
// A field in `reported` can itself be a declared derived leaf (e.g.
|
||||
// `plannedPhaseCore` pushing `'progress.total_plans'` — state-
|
||||
// transition.cts:1752). `valueOf`'s null-vs-string convention cannot tell
|
||||
// "absent from the frontmatter" apart from "resolved to the literal string
|
||||
// 'null'/''", so it cannot carry the same materialization rule
|
||||
// `computeChangedFrontmatterFields` applies below. Route these fields
|
||||
// through the SAME primitives (`resolveFrontmatterPath` + the
|
||||
// `STATE_FIELD_ABSENT` sentinel + `stateFieldValuesDiffer`) instead of a
|
||||
// second, parallel absence convention — one rule, reused, not duplicated.
|
||||
const isDeclaredDerivedLeaf = (candidate: string): boolean =>
|
||||
candidate.includes('.') && Object.prototype.hasOwnProperty.call(FIELD_CLASSIFICATION, candidate);
|
||||
|
||||
const changed = (field: string): boolean => {
|
||||
if (isDeclaredDerivedLeaf(field)) {
|
||||
const before = resolveFrontmatterPath(snapshotFm, field);
|
||||
const after = resolveFrontmatterPath(persistedFm, field);
|
||||
// Same generalized provenance rule as computeChangedFrontmatterFields:
|
||||
// absent-in-snapshot-materializing-in-persisted is the disk scan
|
||||
// catching a never-synced document up, not this write's own action.
|
||||
if (before === STATE_FIELD_ABSENT && after !== STATE_FIELD_ABSENT) return false;
|
||||
return stateFieldValuesDiffer(before, after);
|
||||
}
|
||||
const before = valueOf(snapshotFm, snapshotBody, field);
|
||||
const after = valueOf(persistedFm, persistedBody, field);
|
||||
if (before === null && after === null) return false;
|
||||
if (before === null || after === null) return true;
|
||||
return before.trim() !== after.trim();
|
||||
};
|
||||
|
||||
// Candidate set = `reported` ∪ every frontmatter key (dotted-leaf
|
||||
// granularity) whose persisted value differs from the snapshot, minus the
|
||||
// provenance exclusion. A frontmatter-diff-discovered field is mapped
|
||||
// through `bodyLabelFor` so it lands in the SAME output vocabulary a
|
||||
// transform would have used (`status` -> `'Status'`; `progress.total_plans`
|
||||
// has no body-line label and falls through to its raw dotted key, same as
|
||||
// today's `progress`/`milestone*` fall-through).
|
||||
const changedFrontmatterFields = computeChangedFrontmatterFields(snapshotFm, persistedFm, preWriteState.bodyDeltas);
|
||||
const mappedFrontmatterFields = changedFrontmatterFields.map((field) => bodyLabelFor(field));
|
||||
|
||||
const seen = new Set<string>();
|
||||
const reconciled: string[] = [];
|
||||
for (const field of reported) {
|
||||
const intended = valueOf(preFm, preBody, field);
|
||||
const persistedValue = valueOf(persistedFm, persistedBody, field);
|
||||
if (intended !== null && intended.trim() === (persistedValue ?? '').trim()) {
|
||||
if (STATE_UPDATED_PROVENANCE_EXCLUSION.includes(field) || seen.has(field)) continue;
|
||||
if (changed(field)) {
|
||||
seen.add(field);
|
||||
reconciled.push(field);
|
||||
}
|
||||
}
|
||||
// #3471 review: only fold a `divergedFields` entry into the reported array
|
||||
// when it is a `preserve-when-unchanged` row (has a genuine body-line
|
||||
// label — Status, Current Plan, Current Phase, ...). #3345's direction
|
||||
// ("preservation restored a field the intent never named") is about a
|
||||
// caller-visible BODY field the transform could plausibly have named —
|
||||
// never about `progress` (`preserve-always`) or `milestone`/`milestone_name`
|
||||
// (`preserve-if-placeholder`), which are structured/paired fields no
|
||||
// caller ever names via a per-field body label and whose restoration is
|
||||
// the long-standing, silent #3242/#948 protection, not a caller-visible
|
||||
// "update". Folding them in unconditionally reported `progress` as
|
||||
// `updated` on every `state.patch`/`state.update` write that happened to
|
||||
// preserve it — even when the call never touched Current Phase's
|
||||
// curated-progress-preserving field at all (regression: #1264's
|
||||
// `state.patch` of `Current Phase` reporting `updated: ['Current Phase',
|
||||
// 'progress']` instead of `['Current Phase']`).
|
||||
for (const field of divergedFields) {
|
||||
const cls = stateTransitionMod.getFieldClassification(field);
|
||||
if (!cls || cls.preservation !== 'preserve-when-unchanged') continue;
|
||||
const label = bodyLabelFor(field);
|
||||
if (!reconciled.includes(label)) reconciled.push(label);
|
||||
for (const field of mappedFrontmatterFields) {
|
||||
if (STATE_UPDATED_PROVENANCE_EXCLUSION.includes(field) || seen.has(field)) continue;
|
||||
seen.add(field);
|
||||
reconciled.push(field);
|
||||
}
|
||||
return reconciled;
|
||||
}
|
||||
@@ -3937,12 +4269,15 @@ function cmdStateBeginPhase(cwd: string, phaseNumber: string | number, phaseName
|
||||
// still runs after the sync; the override is re-asserted after it inside
|
||||
// readModifyWriteStateMd for layouts with no body `Phase:` line.
|
||||
const divergedFields: string[] = [];
|
||||
// ADR-3473 §8.7 (#3872): caller-allocated out-param, filled with the
|
||||
// transaction's own pre-write snapshot + body by `applyPostSyncPreservation`.
|
||||
const preWriteState: StatePreWriteSnapshot = {};
|
||||
const rmwOptions: ReadModifyWriteOptions = {
|
||||
authoritativeFm: intent.phaseName ? { current_phase_name: intent.phaseName } : undefined,
|
||||
divergedFields,
|
||||
preWriteState,
|
||||
};
|
||||
let precomputedUpdated: string[] = [];
|
||||
let preSyncContent = '';
|
||||
// #3311: begin-phase is the claim point — it is the one Current Position
|
||||
// transition that explicitly names its phase, so it both records this
|
||||
// session's claim and detects a conflicting live claim for a different
|
||||
@@ -3956,7 +4291,6 @@ function cmdStateBeginPhase(cwd: string, phaseNumber: string | number, phaseName
|
||||
}
|
||||
const result = transitionCore(content, intent, deps);
|
||||
precomputedUpdated = result.updated;
|
||||
preSyncContent = result.content;
|
||||
// #3127 resume: the core preserved the mid-flight Current Phase Name, so
|
||||
// the intent-first override must not fire — it would drift frontmatter
|
||||
// away from the preserved body value. Dropping it here is safe because
|
||||
@@ -3971,7 +4305,7 @@ function cmdStateBeginPhase(cwd: string, phaseNumber: string | number, phaseName
|
||||
// the bytes actually persisted (fix(#3351) generalized) and fold in any
|
||||
// field preservation restored that this transform never touched (#3345's
|
||||
// direction).
|
||||
const updated = reconcileReportedFields(statePath, preSyncContent, precomputedUpdated, divergedFields);
|
||||
const updated = reconcileReportedFields(statePath, preWriteState, precomputedUpdated, divergedFields);
|
||||
|
||||
output(
|
||||
{ updated, phase: phaseNumber, phase_name: phaseName || null, plan_count: planCount || null, milestone_conflict: milestoneConflict },
|
||||
@@ -4268,15 +4602,18 @@ function cmdStatePlannedPhase(cwd: string, phaseNumber: string | number, phaseNa
|
||||
// inside the RMW callback, before this write's own body mutation) rather
|
||||
// than leaving the field to a delta heuristic this exact transition defeats.
|
||||
const divergedFields: string[] = [];
|
||||
// ADR-3473 §8.7 (#3872): caller-allocated out-param, filled with the
|
||||
// transaction's own pre-write snapshot + body by `applyPostSyncPreservation`.
|
||||
const preWriteState: StatePreWriteSnapshot = {};
|
||||
const rmwOptions: ReadModifyWriteOptions = {
|
||||
resync: false,
|
||||
deriveProgressKeys: true,
|
||||
authoritativeFm: intent.phaseName ? { current_phase_name: intent.phaseName } : undefined,
|
||||
divergedFields,
|
||||
preWriteState,
|
||||
};
|
||||
|
||||
let precomputedUpdated: string[] = [];
|
||||
let preSyncContent = '';
|
||||
readModifyWriteStateMd(statePath, (content) => {
|
||||
if (!intent.phaseName) {
|
||||
const preFm = extractFrontmatter(content, statePath) as Record<string, unknown>;
|
||||
@@ -4287,7 +4624,6 @@ function cmdStatePlannedPhase(cwd: string, phaseNumber: string | number, phaseNa
|
||||
}
|
||||
const result = transitionCore(content, intent, deps);
|
||||
precomputedUpdated = result.updated;
|
||||
preSyncContent = result.content;
|
||||
return result.content;
|
||||
}, cwd, rmwOptions);
|
||||
|
||||
@@ -4297,7 +4633,7 @@ function cmdStatePlannedPhase(cwd: string, phaseNumber: string | number, phaseNa
|
||||
// (#3345's direction) — traced for this phase (design doc: "not traced in
|
||||
// the analysis pass") and found to need exactly the same treatment as
|
||||
// `cmdStateBeginPhase`.
|
||||
const updated = reconcileReportedFields(statePath, preSyncContent, precomputedUpdated, divergedFields);
|
||||
const updated = reconcileReportedFields(statePath, preWriteState, precomputedUpdated, divergedFields);
|
||||
|
||||
const result = updated.length === 0
|
||||
? { updated, phase: phaseNumber, plan_count: planCount, warning: 'STATE.md Current Position has no recognized labels — transition was a no-op. Verify STATE.md uses the canonical labeled format (Status:, Total Plans in Phase:, etc.).' }
|
||||
@@ -5305,8 +5641,10 @@ function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string
|
||||
// flat `string[]` (the command's OUTPUT CONTRACT — unchanged) happens once
|
||||
// below, right before `output()`.
|
||||
const updated: StateCompletePhaseUpdateEntry[] = [];
|
||||
let preSyncContent = '';
|
||||
const divergedFields: string[] = [];
|
||||
// ADR-3473 §8.7 (#3872): caller-allocated out-param, filled with the
|
||||
// transaction's own pre-write snapshot + body by `applyPostSyncPreservation`.
|
||||
const preWriteState: StatePreWriteSnapshot = {};
|
||||
// #3835: complete-phase unconditionally rewrites the body `Phase:` line to
|
||||
// `N — COMPLETE` below. That defeats current_phase_name's
|
||||
// preserve-when-unchanged delta rule the same way #3834's no-`--name`
|
||||
@@ -5317,7 +5655,7 @@ function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string
|
||||
// "an absent name does NOT clear an existing curated value" for the body
|
||||
// (`Current Phase Name` section below) — this reasserts the same rule for
|
||||
// the frontmatter key, mirroring cmdStatePlannedPhase's fix.
|
||||
const rmwOptions: ReadModifyWriteOptions = { divergedFields };
|
||||
const rmwOptions: ReadModifyWriteOptions = { divergedFields, preWriteState };
|
||||
|
||||
const wrote = readModifyWriteStateMd(statePath, (content) => {
|
||||
const currentPhase = resolvedPhase;
|
||||
@@ -5405,9 +5743,7 @@ function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string
|
||||
}
|
||||
}
|
||||
|
||||
const out = reassemble(body);
|
||||
preSyncContent = out;
|
||||
return out;
|
||||
return reassemble(body);
|
||||
}, cwd, rmwOptions);
|
||||
|
||||
// ADR-3408 §8.4 (D4): traced for this phase (design doc: "not traced in
|
||||
@@ -5425,7 +5761,7 @@ function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string
|
||||
// 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)];
|
||||
const reconciled = [...sectionEntries, ...reconcileReportedFields(statePath, preWriteState, fieldEntries, divergedFields)];
|
||||
|
||||
output(
|
||||
{ updated: reconciled, phase: resolvedPhase },
|
||||
@@ -5495,6 +5831,17 @@ export = {
|
||||
// FIELD_CLASSIFICATION, exposed so a parity test can pin that every
|
||||
// `preserve-when-unchanged` row has a label here.
|
||||
_FRONTMATTER_KEY_TO_BODY_LABEL: FRONTMATTER_KEY_TO_BODY_LABEL,
|
||||
// Test seam (ADR-3473 §8.7, #3872): the transaction diff and its pure
|
||||
// building blocks, exposed so the ~15 boundary/hostile/property rows in
|
||||
// the test matrix (dotted-path resolution, prototype-pollution safety,
|
||||
// string/number representation insensitivity, the provenance exclusion)
|
||||
// can be driven directly with fabricated snapshot/persisted objects
|
||||
// instead of round-tripping every case through a full RMW write.
|
||||
_reconcileReportedFields: reconcileReportedFields,
|
||||
_computeChangedFrontmatterFields: computeChangedFrontmatterFields,
|
||||
_resolveFrontmatterPath: resolveFrontmatterPath,
|
||||
_stateFieldValuesDiffer: stateFieldValuesDiffer,
|
||||
_STATE_UPDATED_PROVENANCE_EXCLUSION: STATE_UPDATED_PROVENANCE_EXCLUSION,
|
||||
// Test seam (audit M1): inject a deterministic isPidAlive so the liveness-gated
|
||||
// steal decision is exercised without real pids. Mirrors capability-lock.cts.
|
||||
_setLockProbes(probes: Partial<{ isPidAlive: (pid: number) => boolean }>): void {
|
||||
|
||||
@@ -1472,7 +1472,22 @@ describe('#1264: state.patch preserves curated progress frontmatter for non-prog
|
||||
assert.ok(patchResult.success, `state patch failed: ${patchResult.error}`);
|
||||
|
||||
const output = JSON.parse(patchResult.output);
|
||||
assert.deepEqual(output.updated, ['Total Plans in Phase']);
|
||||
// MOVED under ADR-3473 §8.7 (#3872): every `progress.*` leaf here
|
||||
// genuinely changed on disk (the curated 22/6/12/50 block is
|
||||
// disk-derived-resynced to match the 8 real plan dirs this fixture
|
||||
// creates) — this is exactly the #3743/#3818 direction §8.7 closes
|
||||
// (design doc row 4): a field the OLD `reconcileReportedFields`
|
||||
// suppressed via its `progress`-is-`preserve-always` classification
|
||||
// filter, regardless of whether the value actually changed, is now
|
||||
// reported at DOTTED-LEAF granularity because it actually did.
|
||||
assert.deepEqual(output.updated.slice().sort(), [
|
||||
'Total Plans in Phase',
|
||||
'progress.completed_phases',
|
||||
'progress.completed_plans',
|
||||
'progress.percent',
|
||||
'progress.total_phases',
|
||||
'progress.total_plans',
|
||||
].sort());
|
||||
|
||||
const progress = readPersistedProgress(statePath);
|
||||
assert.strictEqual(progress.total_plans, 8);
|
||||
|
||||
@@ -328,7 +328,16 @@ describe('state contract — call-site wiring (integration, real CLI)', () => {
|
||||
'| 1. A | 0/1 | Not started | - |',
|
||||
'',
|
||||
]);
|
||||
writeState(tmpDir, ['status: planning'], [
|
||||
// ADR-3473 §8.7 (#3872) / test-matrix row 17 (D15): `gsd_state_version`
|
||||
// is pre-seeded here so this fixture isolates the ONE thing it is meant
|
||||
// to prove (zero recognized Current Position labels is a genuine no-op)
|
||||
// from an UNRELATED, already-correct §8.7 behavior — a frontmatter block
|
||||
// synthesizing `gsd_state_version` for the first time on a document that
|
||||
// never had one IS a real, reportable addition (row 17/D15, pinned
|
||||
// separately by "state patch accepts JSON object input from workflows"
|
||||
// above). Omitting it here would make THIS no-op test fail for a reason
|
||||
// that has nothing to do with recognized labels.
|
||||
writeState(tmpDir, ['gsd_state_version: 1.0', 'status: planning'], [
|
||||
'# Project State', '',
|
||||
'(no recognized labels here)',
|
||||
]);
|
||||
@@ -358,12 +367,19 @@ describe('state contract — call-site wiring (integration, real CLI)', () => {
|
||||
// Additional no-op path found while auditing `state advance-plan`
|
||||
// (cmdStateAdvancePlan, src/state.cts): re-invoking advance-plan while
|
||||
// already parked at "last plan, ready for verification" reproduces
|
||||
// byte-identical STATE.md content on the SECOND call — `updated` still
|
||||
// reports `Status`/`Last Activity` (reconcileReportedFields matches
|
||||
// against the value ALREADY persisted from the first call), but nothing
|
||||
// is written on the second call, so publishing on `updated.length > 0`
|
||||
// alone would be wrong here; the fix gates on
|
||||
// `readModifyWriteStateMd`'s own write-happened return value instead.
|
||||
// byte-identical STATE.md content on the SECOND call. MOVED under
|
||||
// ADR-3473 §8.7 (#3872): before that change, `reconcileReportedFields`
|
||||
// compared the transform's OWN reported fields against the FINAL
|
||||
// persisted bytes, so `Status`/`Last Activity` (values the transform
|
||||
// always names, which trivially still matched the byte-identical file)
|
||||
// came back non-empty even though nothing was written. §8.7 replaced
|
||||
// that with a diff against the transaction's PRE-WRITE snapshot, so a
|
||||
// genuinely byte-identical second call now correctly reconciles to
|
||||
// `updated: []` — a true no-op is reported as a true no-op. Publishing
|
||||
// still does not fire on this call: `cmdStateAdvancePlan` gates on
|
||||
// `readModifyWriteStateMd`'s own write-happened return value, never on
|
||||
// `updated.length`, so this remains a no-op-does-not-publish regression
|
||||
// test — only the expected shape of the (now-correct) `updated` moved.
|
||||
test('advancePlanNoOpDoesNotPublish', (t) => {
|
||||
const tmpDir = createTempProject();
|
||||
t.after(() => cleanup(tmpDir));
|
||||
@@ -385,7 +401,7 @@ describe('state contract — call-site wiring (integration, real CLI)', () => {
|
||||
const second = runGsdTools(['state', 'advance-plan'], tmpDir);
|
||||
assert.ok(second.success, `second (no-op) advance-plan call must genuinely succeed: ${second.error}`);
|
||||
const output = JSON.parse(second.output);
|
||||
assert.ok(Array.isArray(output.updated) && output.updated.length > 0, 'reconciled updated[] is non-empty even though nothing was written this call');
|
||||
assert.deepStrictEqual(output.updated, [], 'ADR-3473 §8.7: a byte-identical second call is a genuine no-op — `updated` must reconcile to empty, not merely reflect fields the transform always names');
|
||||
assert.strictEqual(fs.existsSync(statePathOf(tmpDir)), false, 'the second, no-op advance-plan call must not publish state.json');
|
||||
});
|
||||
|
||||
|
||||
1205
tests/state.test.cjs
1205
tests/state.test.cjs
File diff suppressed because it is too large
Load Diff
Reference in New Issue
Block a user