fix(#3699): report why a derived frontmatter key was not written, and repair a missing body source (#3846)
* test(#3699): failing-first coverage for derived-key reporting and the case-D fallback * fix(#3699): report why a derived frontmatter key was not written, and repair a missing body source * fix(#3699): scope session-field writes to ## Session so an archived line cannot absorb the update * fix(#3699): resolve the session writer from body labels only, so a frontmatter key never writes the body * chore(#3699): backfill changeset PR number --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
@@ -142,6 +142,115 @@ export const FIELD_CLASSIFICATION: Readonly<Record<string, FieldClassification>>
|
||||
),
|
||||
);
|
||||
|
||||
/**
|
||||
* Which BODY field feeds each frontmatter key.
|
||||
*
|
||||
* `FIELD_CLASSIFICATION` above answers "who wins when frontmatter and body
|
||||
* disagree"; this answers "and what is the body one called". They are separate
|
||||
* questions and this one is display/routing knowledge, not preservation policy,
|
||||
* so it does not widen the ADR-3408-governed table.
|
||||
*
|
||||
* #3699: `state update stopped_at …` reported `Field "stopped_at" not found in
|
||||
* STATE.md` — byte-identical to what a genuinely absent field reports. The key
|
||||
* IS present; it is a projection of a body field, and the message pointed away
|
||||
* from the route that works. Naming the source is what makes the two cases
|
||||
* distinguishable.
|
||||
*
|
||||
* Transcribed from `buildStateFrontmatter` (`state.cts`), which is the real
|
||||
* deriver. That makes this a SECOND copy of knowledge that already exists, so it
|
||||
* ships with a parity test asserting this key set equals the body-derived key set
|
||||
* the builder actually emits (CLAUDE.md → Generative Fix Divergence). Keys the
|
||||
* builder derives from disk, an external file, or the clock have no body source
|
||||
* and are deliberately ABSENT here rather than mapped to a lie.
|
||||
*/
|
||||
export const FRONTMATTER_BODY_SOURCE: Readonly<Record<string, readonly string[]>> = Object.freeze(
|
||||
Object.assign(Object.create(null) as Record<string, readonly string[]>, {
|
||||
current_phase: Object.freeze(['Current Phase']),
|
||||
current_phase_name: Object.freeze(['Current Phase Name']),
|
||||
current_plan: Object.freeze(['Current Plan']),
|
||||
status: Object.freeze(['Status']),
|
||||
// Scoped to `## Session` by the builder; see the presence check in
|
||||
// `updateCore` for why the lookup here is deliberately unscoped.
|
||||
stopped_at: Object.freeze(['Stopped At', 'Stopped at']),
|
||||
paused_at: Object.freeze(['Paused At']),
|
||||
last_activity: Object.freeze(['Last Activity', 'Last activity']),
|
||||
last_activity_desc: Object.freeze(['Last Activity Description']),
|
||||
} satisfies Record<string, readonly string[]>),
|
||||
);
|
||||
|
||||
/**
|
||||
* The frontmatter keys whose body source lives inside `## Session`.
|
||||
*
|
||||
* #3374 established that these fields must be written where the reader reads
|
||||
* them: `buildStateFrontmatter` harvests `Stopped At` / `Paused At` from the
|
||||
* session section only, so a whole-body replace "lets a decoy `**Stopped at:**`
|
||||
* line in an unrelated (e.g. archive) section absorb the refresh while the
|
||||
* harvested session value stays stale" (`stateReplaceFieldInSession`'s own
|
||||
* docstring). `updateCore` was still doing the whole-body replace.
|
||||
*/
|
||||
const SESSION_SCOPED_KEYS: ReadonlySet<string> = new Set(['stopped_at', 'paused_at']);
|
||||
|
||||
/**
|
||||
* The `(primary, fallback)` label pair for a session-scoped frontmatter KEY.
|
||||
*/
|
||||
function sessionLabelsForKey(key: string): { primary: string; fallback: string | null } | null {
|
||||
if (!SESSION_SCOPED_KEYS.has(key)) return null;
|
||||
const labels = FRONTMATTER_BODY_SOURCE[key];
|
||||
return { primary: labels[0], fallback: labels[1] ?? null };
|
||||
}
|
||||
|
||||
/**
|
||||
* The same pair, resolved from a BODY LABEL the caller named (`Stopped At`,
|
||||
* `Stopped at`, `Paused At`). `null` for anything else.
|
||||
*
|
||||
* Deliberately does NOT accept a frontmatter key. An earlier cut resolved both
|
||||
* spellings through one function and used it for the write, which made
|
||||
* `state update stopped_at …` write the BODY line through the session writer —
|
||||
* silently defeating the "frontmatter keys are not directly writable" contract
|
||||
* this whole change exists to state, and reporting `updated: false` while having
|
||||
* written. The write may only ever be reached by naming a body field.
|
||||
*/
|
||||
function sessionLabelsForBodyField(field: string): { primary: string; fallback: string | null } | null {
|
||||
const key = frontmatterKeyForBodyField(field);
|
||||
return key === null ? null : sessionLabelsForKey(key);
|
||||
}
|
||||
|
||||
/**
|
||||
* Would a session-scoped write actually land? Asks by attempting the real write
|
||||
* with a throwaway value and seeing whether anything moved.
|
||||
*
|
||||
* Deliberately reuses the writer rather than re-deriving "where is the session
|
||||
* section" — a separate scope check could disagree with the writer, and a
|
||||
* presence check that disagrees with the write it guards is the whole bug class
|
||||
* here. `stateReplaceFieldInSession` is replace-only and pure, so probing costs
|
||||
* nothing and the result is discarded.
|
||||
*/
|
||||
function sessionSourceExists(body: string, labels: { primary: string; fallback: string | null }): boolean {
|
||||
return stateReplaceFieldInSession(body, labels.primary, labels.fallback, '\u0000probe') !== body;
|
||||
}
|
||||
|
||||
/**
|
||||
* Own-property body-source lookup. `null` for a key with no body source (a
|
||||
* disk/external/clock-derived key) and for anything not a frontmatter key.
|
||||
*/
|
||||
export function getFrontmatterBodySource(field: string): readonly string[] | null {
|
||||
if (!Object.prototype.hasOwnProperty.call(FRONTMATTER_BODY_SOURCE, field)) return null;
|
||||
return FRONTMATTER_BODY_SOURCE[field];
|
||||
}
|
||||
|
||||
/**
|
||||
* Reverse lookup: the frontmatter key a body field feeds, or `null`.
|
||||
* Lets a failed body-field update name the frontmatter key that still carries a
|
||||
* value (#3699 case D), instead of reporting a bare absence.
|
||||
*/
|
||||
export function frontmatterKeyForBodyField(bodyField: string): string | null {
|
||||
const wanted = bodyField.trim().toLowerCase();
|
||||
for (const key of Object.keys(FRONTMATTER_BODY_SOURCE)) {
|
||||
if (FRONTMATTER_BODY_SOURCE[key].some((f) => f.toLowerCase() === wanted)) return key;
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Own-property classification lookup. Returns `null` for unknown fields
|
||||
* (including inherited prototype methods like `toString`/`valueOf`).
|
||||
@@ -1834,8 +1943,69 @@ function updateCore(
|
||||
const existingFm = extractFrontmatter(content) as Record<string, unknown>;
|
||||
const hasFrontmatter = Object.keys(existingFm).length > 0;
|
||||
const body = stripFrontmatter(content);
|
||||
const result = stateReplaceField(body, intent.field, intent.value);
|
||||
// #3699 review: session-scoped fields are written through the session-scoped
|
||||
// writer. A whole-body `stateReplaceField` matches the FIRST occurrence
|
||||
// anywhere, so with no `Stopped At:` line in `## Session` but a stale one in
|
||||
// `## Session Continuity Archive`, `state update "Stopped At" …` reported
|
||||
// `updated: true` while rewriting the ARCHIVE line and leaving both the session
|
||||
// section and the `stopped_at` frontmatter key untouched — a silent corruption
|
||||
// of a historical record reported as success. #3374 already established this
|
||||
// rule for the other writer; this one had not adopted it.
|
||||
const sessionWriteLabels = sessionLabelsForBodyField(intent.field);
|
||||
let result: string | null;
|
||||
if (sessionWriteLabels) {
|
||||
// Replace-only by contract: unchanged content means the field is not in the
|
||||
// session section, which is a miss, not a write.
|
||||
const replaced = stateReplaceFieldInSession(body, sessionWriteLabels.primary, sessionWriteLabels.fallback, intent.value);
|
||||
result = replaced === body ? null : replaced;
|
||||
} else {
|
||||
result = stateReplaceField(body, intent.field, intent.value);
|
||||
}
|
||||
if (result === null) {
|
||||
// #3699 case D — the frontmatter fallback.
|
||||
//
|
||||
// Normally frontmatter keys are NOT writable here: they are projections, and
|
||||
// `buildStateFrontmatter` re-derives them from the body on every write, so a
|
||||
// direct frontmatter write would be discarded. But when the body source line
|
||||
// is absent entirely, there is nothing to derive FROM: the key's existing
|
||||
// value survives on `preserve-when-unchanged`, and neither the frontmatter
|
||||
// key nor the body field can be updated by any route. That document is
|
||||
// unrepairable through `state update`, which is the gap this closes.
|
||||
//
|
||||
// Deliberately narrow — all three must hold:
|
||||
// (1) the field is a frontmatter key with a known body source,
|
||||
// (2) NO body source line exists, so the body route is genuinely unavailable
|
||||
// (this is what keeps case A, where the body route works, routing to the
|
||||
// body as before), and
|
||||
// (3) the frontmatter already carries the key, so this updates a value that
|
||||
// is really there rather than inventing one.
|
||||
//
|
||||
// The presence check in (2) is UNSCOPED on purpose, unlike the builder's
|
||||
// `## Session` scoping for stopped_at/paused_at. The asymmetry is the safe
|
||||
// direction: any `Stopped at:` line anywhere in the body — including one in an
|
||||
// archive section — suppresses the fallback, so this never writes frontmatter
|
||||
// while a body line the user could edit still exists.
|
||||
const bodySource = getFrontmatterBodySource(intent.field);
|
||||
const frontmatterCarriesKey =
|
||||
hasFrontmatter && Object.prototype.hasOwnProperty.call(existingFm, intent.field);
|
||||
// The presence check asks the same question the WRITE asks, in the same
|
||||
// scope. An earlier cut checked the whole body on the reasoning that any
|
||||
// editable line should suppress the repair — but a line the reader never
|
||||
// reads is not a source, and suppressing on it left the document
|
||||
// unrepairable while pointing the user at a command that would rewrite the
|
||||
// wrong line. Same scope for read, write and probe, or they disagree.
|
||||
const sessionProbeLabels = sessionLabelsForKey(intent.field);
|
||||
const bodySourceExists = sessionProbeLabels
|
||||
? sessionSourceExists(body, sessionProbeLabels)
|
||||
: (bodySource ?? []).some((f) => stateExtractField(body, f) !== null);
|
||||
if (bodySource && frontmatterCarriesKey && !bodySourceExists) {
|
||||
const nextFm = { ...existingFm, [intent.field]: intent.value };
|
||||
return {
|
||||
content: `---\n${reconstructFrontmatter(nextFm as unknown as Frontmatter)}\n---\n\n${body}`,
|
||||
updated: [intent.field],
|
||||
data: { updated: true, wroteFrontmatter: true },
|
||||
};
|
||||
}
|
||||
return { content, updated: [], data: { updated: false } };
|
||||
}
|
||||
const reassembled = hasFrontmatter
|
||||
|
||||
104
src/state.cts
104
src/state.cts
@@ -64,6 +64,9 @@ import { findProjectRoot } from './project-root.cjs';
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports
|
||||
import milestoneLockMod = require('./milestone-lock.cjs');
|
||||
const { transitionCore, applyStatePreservation, sliceCurrentPositionSection } = stateTransitionMod;
|
||||
// #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;
|
||||
type StateTransitionIntent = stateTransitionMod.StateTransitionIntent;
|
||||
type StateTransitionDeps = stateTransitionMod.StateTransitionDeps;
|
||||
type PhaseInventoryRecord = stateTransitionMod.PhaseInventoryRecord;
|
||||
@@ -584,6 +587,54 @@ function cmdStatePatch(cwd: string, patches: Record<string, string>, raw: boolea
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Why did `state update <field>` not write anything?
|
||||
*
|
||||
* #3699: this used to be one sentence — `Field "X" not found in STATE.md` — for
|
||||
* every falsy outcome, so a PRESENT-but-derived frontmatter key and a genuinely
|
||||
* absent field produced byte-identical output apart from the name. The classifier
|
||||
* already knew the difference; the message threw it away, and worse, pointed away
|
||||
* from the route that works.
|
||||
*
|
||||
* Four distinct answers, because there are four distinct situations:
|
||||
* 1. a body-derived frontmatter key whose body source EXISTS → name that source
|
||||
* 2. a frontmatter key with no body source at all (disk/external/clock-derived)
|
||||
* → say what derives it, and do not invent a body field to blame
|
||||
* 3. a body field that feeds a frontmatter key still carrying a value
|
||||
* → name the key, so case D is diagnosable rather than a bare absence
|
||||
* 4. genuinely unknown → unchanged
|
||||
*/
|
||||
function explainUpdateFailure(field: string): string {
|
||||
const bodySource = getFrontmatterBodySource(field);
|
||||
if (bodySource) {
|
||||
// (1) The fallback in `updateCore` did not fire, so a body source line
|
||||
// exists — that is the writable route.
|
||||
const [primary] = bodySource;
|
||||
return `Field "${field}" is a body-derived frontmatter key and is not directly writable. `
|
||||
+ `Update its body source instead: state update "${primary}" <value>.`;
|
||||
}
|
||||
const classification = getFieldClassification(field);
|
||||
if (classification) {
|
||||
// (2) Known key, no body source: disk/external/free-derived.
|
||||
const derivedFrom: Record<string, string> = {
|
||||
disk: 'derived from a scan of .planning/phases/ and is not directly writable',
|
||||
external: 'derived from ROADMAP.md and is not directly writable',
|
||||
free: 'recomputed on every write and is not directly writable',
|
||||
curated: 'maintained by the write path and is not directly writable through this command',
|
||||
body: 'body-derived and is not directly writable',
|
||||
};
|
||||
return `Field "${field}" is a frontmatter key that is ${derivedFrom[classification.source]}.`;
|
||||
}
|
||||
const owningKey = frontmatterKeyForBodyField(field);
|
||||
if (owningKey) {
|
||||
// (3) Case D from the body-field side.
|
||||
return `Field "${field}" not found in STATE.md. It is the body source for frontmatter key `
|
||||
+ `"${owningKey}" — add the "${field}:" line to the body, or update "${owningKey}" directly `
|
||||
+ `to repair a document whose body source is missing.`;
|
||||
}
|
||||
return `Field "${field}" not found in STATE.md`; // (4) genuinely unknown
|
||||
}
|
||||
|
||||
function cmdStateUpdate(cwd: string, field: string | undefined, value: string | undefined): void {
|
||||
if (!field || value === undefined) {
|
||||
error('field and value required for state update');
|
||||
@@ -601,6 +652,27 @@ function cmdStateUpdate(cwd: string, field: string | undefined, value: string |
|
||||
try {
|
||||
let updated = false;
|
||||
let preSyncContent = '';
|
||||
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
|
||||
// is silently undone. `buildStateFrontmatter` re-derives `stopped_at` from
|
||||
// the body, finds no source, and emits nothing; `applyPreserveWhenUnchanged`
|
||||
// then sees an unchanged (absent) body source and restores the PRE-write
|
||||
// snapshot over the value just written. Verified: without this the command
|
||||
// reported `updated:false` with `preserved:["Stopped At"]` and the old value
|
||||
// stood.
|
||||
//
|
||||
// `authoritativeFm` is the seam built for exactly this (#2736 — "intent-first
|
||||
// frontmatter values … so the lossy body-prose re-derivation can never
|
||||
// destroy information the transition just resolved"), and it is re-applied
|
||||
// AFTER preservation (`applyPostSyncPreservation`), so it wins the restore.
|
||||
//
|
||||
// Populated by the transform below rather than up front, because only the
|
||||
// transition knows whether the fallback fired. Safe: `readModifyWriteStateMd`
|
||||
// dereferences `options.authoritativeFm` after running the transform. Left
|
||||
// empty when the fallback does not fire — an empty object iterates zero
|
||||
// entries and is a no-op at both application sites.
|
||||
const authoritativeFm: Record<string, unknown> = {};
|
||||
const divergedFields: string[] = [];
|
||||
const shouldResync = shouldResyncStateProgress([field as string]);
|
||||
// ADR-1769 Phase 7: dispatches to the STATE.md Transition Module. The
|
||||
@@ -616,9 +688,13 @@ function cmdStateUpdate(cwd: string, field: string | undefined, value: string |
|
||||
{ clock: realClock },
|
||||
);
|
||||
updated = (result.data as { updated: boolean } | undefined)?.updated === true;
|
||||
transitionData = result.data;
|
||||
if (transitionData?.wroteFrontmatter === true) {
|
||||
authoritativeFm[field as string] = value;
|
||||
}
|
||||
preSyncContent = result.content;
|
||||
return result.content;
|
||||
}, cwd, { resync: shouldResync, divergedFields });
|
||||
}, cwd, { resync: shouldResync, divergedFields, authoritativeFm });
|
||||
|
||||
// ADR-3408 §8.4 (D4): reconcile against the bytes actually persisted —
|
||||
// `updateCore`'s own match does not know whether sync/preservation later
|
||||
@@ -632,9 +708,31 @@ function cmdStateUpdate(cwd: string, field: string | undefined, value: string |
|
||||
const preserved = reconciled.filter((f) => f !== field);
|
||||
|
||||
if (updated) {
|
||||
output({ updated: true, preserved }, false, undefined);
|
||||
// #3699 case D: surfaced so a caller can tell "wrote the body source" from
|
||||
// "wrote the frontmatter key because no body source existed" — the second
|
||||
// is a repair, and silently reporting it as an ordinary update is the same
|
||||
// class of unfalsifiable success this issue is about.
|
||||
const wroteFrontmatter = (transitionData as { wroteFrontmatter?: boolean } | undefined)?.wroteFrontmatter === true;
|
||||
if (!wroteFrontmatter) {
|
||||
output({ updated: true, preserved }, false, undefined);
|
||||
} else {
|
||||
// `preserved` reports the BODY LABEL of each field preservation restored
|
||||
// (`bodyLabelFor`, the #3345 direction). In the case-D fallback that
|
||||
// reading is stale by one step: preservation DID restore this field's
|
||||
// snapshot, and `authoritativeFm` then overrode it, so the value on disk
|
||||
// is the one just written. Reporting it as preserved would claim a
|
||||
// restore that did not survive — the same unfalsifiable-success shape
|
||||
// #3699 is about, one field over. Drop this field's own labels; other
|
||||
// fields' preservation is untouched and still reported.
|
||||
const ownLabels = new Set((getFrontmatterBodySource(field as string) ?? []).map((l) => l.toLowerCase()));
|
||||
output({
|
||||
updated: true,
|
||||
wrote: 'frontmatter',
|
||||
preserved: preserved.filter((p) => !ownLabels.has(p.toLowerCase())),
|
||||
}, false, undefined);
|
||||
}
|
||||
} else {
|
||||
output({ updated: false, reason: `Field "${field as string}" not found in STATE.md`, preserved }, false, undefined);
|
||||
output({ updated: false, reason: explainUpdateFailure(field as string), preserved }, false, undefined);
|
||||
}
|
||||
} catch {
|
||||
output({ updated: false, reason: 'STATE.md not found' }, false, undefined);
|
||||
|
||||
Reference in New Issue
Block a user