diff --git a/.changeset/rapid-foxes-forage.md b/.changeset/rapid-foxes-forage.md new file mode 100644 index 000000000..bf31b23ce --- /dev/null +++ b/.changeset/rapid-foxes-forage.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3495 +--- +**STATE.md preservation now enforces every policy its own table declares** — a field could be declared `preserve-when-unchanged` and quietly go unenforced, because the executor branched on field names rather than on the declared policy, so four of eight rows were honored by a weaker mechanism elsewhere and two policies had no implementation at all. Preservation is now dispatched from the classification table, a declared row nothing enforces fails loudly instead of silently, and a whitespace-only curated value is no longer treated as a real one. (#3468) diff --git a/docs/adr/3408-state-write-path-preservation.md b/docs/adr/3408-state-write-path-preservation.md index dc0e0ce69..856abec18 100644 --- a/docs/adr/3408-state-write-path-preservation.md +++ b/docs/adr/3408-state-write-path-preservation.md @@ -267,4 +267,41 @@ One row per concern. A blank owner is a concern whose contract is locked (§8) b ## Amendments -*(None yet. Phase 1 records its `clear` resolution (§8.6) and its guard's measured baseline here; Phase 3 records the §8.4 bucket decision.)* +### Amendment 1 — Phase 1 (#3468) validation: the contract held; a policy had two implementations + +Decisions 1–5 needed **no change**. Every input class Phase 1 hit was expressible in the contract as written, and the closed guard vocabulary of Decision 1 proved sufficient at exactly the size predicted. What the contract did *not* anticipate was a divergence one level below the policy. + +**`preserve-always` had two divergent implementations, and one contradicted its own row.** Neither #3408 nor this ADR named it; it fell out of reading the code rather than the write-up. + +| Field | Declared | Implemented as | Gate | +|---|---|---|---| +| `progress` | `preserve-always` | preserve-always | `!resync`, no delta condition | +| `current_phase_name` | `preserve-always` | **preserve-when-unchanged** | body `Phase:` delta, no resync gate | + +**Resolution: the row was wrong, the code was right.** `current_phase_name` is reclassified to `preserve-when-unchanged`. Its delta-gated behavior is regression-bound by #1743, #1695 and #2736, so changing the code to match the label would have broken deliberate decisions; changing the label is behavior-preserving and makes the table honest, which is the point of the epic. Matrix test C2 pins its exact outputs as literals, because a behavior-preserving change is precisely where a silent mistake hides. + +**§8.6 resolved: `clear` is DELETED.** No row used it and no executor existed — verified as the single occurrence of the token in `src/state-transition.cts`, with zero dependents and a clean `tsc`. Speculative Generality: a policy invented for a need that never arrived. `FieldPreservation` now has four members, each with exactly one executor. + +**The closed guard vocabulary is real and small.** Decision 1 mandated named guards over a predicate slot. Implementation found exactly **one** true executor-side guard in the entire codebase — `status`'s `'unknown'` sentinel — because `stopped_at`'s `## Session` scoping turned out to be caller-side *extraction*, not an executor condition. Shipped as `FieldGuard = 'non-sentinel-unknown'` plus `FieldMergeStrategy = 'progress-ratchet'` for the #2440/#2969 merge. Two single-member unions is deliberate: the alternative is the open predicate slot Greenspun's Tenth Rule rejects, and adding a member remains an amendment here. + +**Copy count, per Amendment 3's standing rule — "N found by the guard", never "N per the epic":** + +| | #3408 scoped | Guard found | +|---|---|---| +| Write-seam bypasses (Axis 2) | 2 | **4** | +| Policy-dispatch violations (Axis 1) | not scoped | **7** — 5 field-name branches, plus `derive` and `clear` with no executor at all | + +The four are `cmdStateSync`, `cmdMilestoneComplete`, `cmdPhaseComplete`, and `REGENERATE_STATE`. **`patchCore` — one of the two this ADR named — is not among them**, because it bypasses via `stateReplaceField` rather than the seam calls. The epic's list was both short and partly wrong, a fourth consecutive confirmation that a hand-maintained list is a lower bound. + +**Two detectors were built and removed again, recorded because the guard is the instrument this epic trusts:** + +1. A prompt-layer detector reported **5 backticked prose mentions** as drift — ADR-3180 Amendment 3's recorded false-positive class. `CONTRIBUTING.md` already settles it: a backticked command reference is a mention. Now gated on inline-code spans. +2. A `stateReplaceField` co-occurrence detector for §8.3(b), measured at **29 false positives to 1 true positive** — it matched the function's own *definition* and ~20 calls on frontmatter-free body slices. Banking 29 non-defects to catch one is Decision 5's own "ratchet as a parking lot" route, so shipping it would have made the guard violate this ADR. + +**DECLARED KNOWN GAP, owned by Phase 2 (#3469).** §8.3(b) — `patchCore` writing over frontmatter — is **not detected**. It needs genuine dataflow ("is this argument the whole document or a body slice?"), not co-occurrence. Phase 2 both fixes the defect and makes detection tractable, because once the pure pipeline exists the invariant simplifies to "no transition core calls `stateReplaceField` on unstripped content". Stated here so the gap has a named owner rather than being a silence. + +**Decision 5's anti-gaming list earned itself twice, in one phase.** After the refactor the guard reported `policyDispatchViolations: 0` while `applyPreserveIfPlaceholder` still opened with `if (field !== 'milestone_name') return` — a branch selected by field name, which §8.1 forbids outright, in a syntactic form Axis 1 did not match. That is the "route it through a differently-named local" row of Decision 5's table, live. The executor was already idempotent, so the test bought nothing and was deleted; Axis 1 is widened to catch field-variable comparisons against literals, with its remaining evasion (renaming the loop variable) declared in the guard's header rather than left implicit. Separately, the guard's own `loadBaseline` conflated an *unreadable* baseline with an *absent* one — a diagnostic collapsing two states into one identical result, which is this epic's defining failure shape reproduced inside the tool built to detect it. Both fixed in Phase 1. + +**Complexity, measured before and after.** `applyStatePreservation` was 177 lines, cyclomatic 61, cognitive 107, `risk_level: critical`. It is now a 27-line dispatch loop over four executors of 28, 44, 26 and 3 lines. This was Kernighan's Law's contribution to the design — the justification for the refactor was debuggability, not line count, and the largest remaining unit is the one holding the genuinely intricate #2440/#2969 ratchet. + +*(Phase 3 records the §8.4 bucket decision here.)* diff --git a/package.json b/package.json index 42fe58be2..a78ad067e 100644 --- a/package.json +++ b/package.json @@ -116,7 +116,7 @@ "lint:table-schema-drift": "node scripts/lint-table-schema-drift.cjs", "lint:frontmatter-scalar-broad-grep": "node scripts/lint-frontmatter-scalar-broad-grep.cjs", "lint:removed-but-needed": "node scripts/lint-removed-but-needed.cjs", - "lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-emitted-drift-ack.cjs && node scripts/lint-portable-timeout.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-test.cjs && node scripts/lint-example-parser-parity.cjs && node scripts/lint-docs-command-form.cjs && node scripts/lint-plan-count-drift.cjs && node scripts/lint-milestone-window-drift.cjs && node scripts/lint-phase-enumeration-drift.cjs && node scripts/lint-planning-prompt-drift.cjs && node scripts/lint-completion-ratio-drift.cjs && node scripts/lint-state-field-drift.cjs && node scripts/lint-completion-predicate-drift.cjs && node scripts/lint-planning-snapshot-bypass-drift.cjs && node scripts/lint-health-diagnostic-rule-table.cjs && node scripts/lint-planning-artifact-writer-drift.cjs && node scripts/lint-frontmatter-scalar-broad-grep.cjs && node scripts/lint-removed-but-needed.cjs && node scripts/lint-no-adhoc-regex-escape.cjs", + "lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-emitted-drift-ack.cjs && node scripts/lint-portable-timeout.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-test.cjs && node scripts/lint-example-parser-parity.cjs && node scripts/lint-docs-command-form.cjs && node scripts/lint-plan-count-drift.cjs && node scripts/lint-milestone-window-drift.cjs && node scripts/lint-phase-enumeration-drift.cjs && node scripts/lint-planning-prompt-drift.cjs && node scripts/lint-completion-ratio-drift.cjs && node scripts/lint-state-field-drift.cjs && node scripts/lint-state-write-path-drift.cjs && node scripts/lint-completion-predicate-drift.cjs && node scripts/lint-planning-snapshot-bypass-drift.cjs && node scripts/lint-health-diagnostic-rule-table.cjs && node scripts/lint-planning-artifact-writer-drift.cjs && node scripts/lint-frontmatter-scalar-broad-grep.cjs && node scripts/lint-removed-but-needed.cjs && node scripts/lint-no-adhoc-regex-escape.cjs", "lint:allow-test-rule-refs": "node scripts/lint-allow-test-rule-refs.cjs", "lint:regression-names": "node scripts/lint-regression-test-names.cjs", "lint:descriptions": "node scripts/lint-descriptions.cjs", diff --git a/scripts/lint-state-write-path-drift.cjs b/scripts/lint-state-write-path-drift.cjs new file mode 100644 index 000000000..9ca016276 --- /dev/null +++ b/scripts/lint-state-write-path-drift.cjs @@ -0,0 +1,914 @@ +#!/usr/bin/env node +'use strict'; + +/** + * Anti-divergence drift guard for the STATE.md WRITE PATH — epic #3408, issue + * #3468, ADR-3408 Decision 5, contract §8.1/§8.2/§8.3 + * (`docs/adr/3408-state-write-path-preservation.md` is the contract this + * guard enforces; read it first). + * + * TWO AXES, ONE GUARD, because they fail together — a table that is + * bypassed on dispatch and a seam that is bypassed on write are the same + * failure mode ("policy declared, enforcement hand-rolled") applied to two + * different call shapes: + * + * AXIS 1 — POLICY DISPATCH (§8.1). `applyStatePreservation` must select + * its branch from a `FIELD_CLASSIFICATION` row's `preservation` value, + * never from a field NAME. Every `getFieldClassification('')` inside `src/state-transition.cts` is a field-name-keyed + * branch — the shape that let four declared rows go unimplemented until + * #3258, and that leaves `derive` and `clear` with no executor today. A + * VARIABLE argument (`getFieldClassification(field)`) is the CORRECT + * table-driven shape and is deliberately NOT matched. Also matched: a + * direct `field === ''` / `field !== ''` / `'' + * === field` comparison of the dispatch loop's own `field` variable — the + * same prohibited shape routed AROUND `getFieldClassification` instead of + * through it (#3468 found this exact form live in `applyPreserveIfPlaceholder`, + * undetected by the call-shape check alone). Scoped to the identifier + * `field` only; see `FIELD_VAR_EQ_LITERAL_RE`'s own comment for why. + * + * AXIS 2 — WRITE SEAM (§8.3), RATCHETED. `readModifyWriteStateMd` is the + * only path meant to write STATE.md. Every direct `writeStateMd(` or + * `syncStateFrontmatter(` call outside the owner's own definitions is a + * bypass that skips preservation and the #948 no-op guard — how #3374 and + * #3350 reproduce. This axis ships RATCHETED (see below), never a bare + * 0-or-fail check. + * + * DESIGN CONSTRAINTS (ADR-3180 Decision 4, adopted verbatim by ADR-3408): + * - 4(a) whole-repo scan, never an allowlist. ADR-3180's own phases found + * 26/5/54 copies where their epics scoped 3/3/4 — a scoped guard earns + * nothing. + * - 4(d) the scan surface is DECLARED and is NOT just `src/` — `src/` + * alone is itself an allowlist one directory wide; #1762 traced a wrong + * count to a shell snippet in `gsd-core/workflows/progress.md`. And + * inward: the OWNER FILE (`src/state.cts`) is not exempt, only its + * named canonical FUNCTIONS are (`SEAM_OWNER_EXEMPT_FUNCTIONS` below). + * - 4(e) Axis 2 ships ratcheted because Phase 1 (this file) cannot + * consolidate the write seam — that is Phase 2 (#3469). Landing a + * guard later, against an already-clean tree, is the "found it, wrote + * it down, moved on" posture the epic removes. + * + * GOODHART, PER ADR-3408 DECISION 5: "0 bypasses" is a LAGGING metric — a + * measure about to become a target. This guard's own `_comment` and its + * human-readable success message both say so: the zero this guard reports + * must NEVER be quoted alone; it is only meaningful beside the behavioral + * identity test's result (the consumer-output assertion Decision 5's gaming + * table names as the actual defense). + * + * String literals are matched, never parsed as an AST — deliberately, per + * `scripts/lint-state-field-drift.cjs`'s own precedent: over-reporting + * (flagging a comment or a string that merely looks like a call) is safe; + * under-reporting (missing a real bypass) is the failure this guard exists + * to prevent. `stripComments` does not track quoted strings for exactly + * this reason — see its own header. + * + * DECLARED KNOWN GAP — §8.3(b) `patchCore` frontmatter-write shape is NOT + * detected by this guard. `patchCore` runs `stateReplaceField(` over the + * WHOLE document (body + frontmatter) instead of stripping frontmatter + * first, the way `updateCore` does — a real defect, but this guard does not + * catch it. + * + * Why: catching it needs genuine DATAFLOW ("is this argument a variable + * holding the full document, or a body slice?"), not function-scoped + * co-occurrence. A co-occurrence approximation (does the enclosing function + * also call `stripFrontmatter(`?) was implemented and measured directly + * against this repo: 33 occurrences of `stateReplaceField(`, of which only + * 4 are genuine write-seam bypasses and 29 are noise — the definition of + * `stateReplaceField` itself (matched as a call), ~20 calls on `sectionBody` + * (a body slice that is frontmatter-free by construction), and several + * calls inside `readModifyWriteStateMd` callbacks (correct, because the RMW + * envelope applies preservation after the callback returns). 29 false + * positives to 1 true positive buries the signal and makes the ratchet's + * shrink-rate meaningless as a Phase 2 progress indicator — recorded here, + * with these numbers, so the next reader does not re-attempt the same + * approximation. + * + * Who owns closing it: Phase 2 (#3469), which also FIXES the defect by + * consolidating on the single write seam — after which detection becomes + * tractable, because once the pure pipeline exists the invariant simplifies + * to "no transition core calls `stateReplaceField` on unstripped content". + * + * This is a DECLARED gap with a named owner, not a silent omission — a + * guard that quietly does not look somewhere is the failure ADR-3180 + * Decision 4(d) records. + */ + +const fs = require('node:fs'); +const path = require('node:path'); +const { scanTree, sanitizeForReport } = require('./lib/drift-scan.cjs'); +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); + +const REPO_ROOT = path.resolve(__dirname, '..'); +const BASELINE_PATH = path.join(__dirname, 'state-write-path-drift-baseline.json'); + +// Frozen REASON enum — mirrors `lint-state-field-drift.cjs`'s house style of +// naming every failure shape explicitly rather than reusing one generic +// "violation" string, so a reader can `grep` a reason string straight back +// to the paragraph of this header (or of the ADR) that explains it. +const REASON = Object.freeze({ + FIELD_NAME_DISPATCH: 'field_name_dispatch', + UNIMPLEMENTED_POLICY: 'unimplemented_policy', + SEAM_BYPASS_UNRECORDED: 'seam_bypass_unrecorded', + SEAM_BYPASS_COUNT_GREW: 'seam_bypass_count_grew', + SEAM_BYPASS_COUNT_SHRANK: 'seam_bypass_count_shrank', + BASELINE_ENTRY_STALE: 'baseline_entry_stale', + BASELINE_UNREADABLE: 'baseline_unreadable', +}); + +// Scan surface — declared, per Decision 4(d), never inferred from `src/` +// alone. `src/` covers the executor and the write-seam owner; the prompt +// layer covers markdown that can shell out to `state.patch` / `phase.complete` +// and post-process the result outside any TypeScript this guard could see. +const SRC_DIRS = ['src']; +const SRC_EXT = new Set(['.cts']); +const PROMPT_DIRS = ['gsd-core/workflows', 'commands', 'agents', 'skills']; +const PROMPT_EXT = new Set(['.md']); + +// The executor (Axis 1) and the write-seam owner (Axis 2). Forward-slash +// literals: every `rel` this guard compares against them is unconditionally +// POSIX-normalized first (`toPosixRel` below) — never gated on +// `process.platform`. +const EXECUTOR_FILE = 'src/state-transition.cts'; +const SEAM_OWNER_FILE = 'src/state.cts'; + +// Per Decision 4(d)'s "owner FILE is not exempt, only its named canonical +// FUNCTIONS are": a `writeStateMd(`/`syncStateFrontmatter(` call inside one +// of these two functions, in `SEAM_OWNER_FILE` only, is the seam's own +// internal plumbing (the I/O wrapper calling the pure sync stage), not a +// bypass. Every OTHER function in `state.cts` — and every function in every +// OTHER file — is still scanned and still flagged. +const SEAM_OWNER_EXEMPT_FUNCTIONS = ['writeStateMd', 'readModifyWriteStateMd']; + +// Unconditional path-separator normalization (never gated on +// `process.platform` — a Windows-authored fork PR must be judged by the +// same POSIX-relative rule as everything else this guard reads). +function toPosixRel(rel) { + return rel.split(path.sep).join('/'); +} + +/** + * Strip `//` line comments and `/* ... *\/` block comments from `text`, + * returning one entry PER INPUT LINE so line numbers computed against the + * result stay correct against the original file. Block comments are tracked + * across lines (`inBlock`); line comments only ever affect their own line. + * + * Deliberately does NOT parse string/template literal contents — a `//` or + * `/*` embedded inside a quoted string is treated exactly like real source, + * which can occasionally UNDER-strip (leaving a would-be-comment's text + * live) but never OVER-strips real code into invisibility. Per this guard's + * header and `lint-state-field-drift.cjs`'s own precedent: over-reporting a + * documentation paragraph that merely DESCRIBES a call (ADR-3180 Amendment + * 3's exact false positive) is the failure this exists to prevent; a rare + * miss on an adversarial one-line string is an accepted, narrower risk in + * the opposite (safe) direction — under-reporting, never over-reporting. + */ +function stripComments(text) { + const lines = text.split('\n'); + const out = new Array(lines.length); + let inBlock = false; + for (let i = 0; i < lines.length; i++) { + const line = lines[i]; + let result = ''; + let j = 0; + while (j < line.length) { + if (inBlock) { + const close = line.indexOf('*/', j); + if (close === -1) { + j = line.length; + break; + } + j = close + 2; + inBlock = false; + continue; + } + if (line[j] === '/' && line[j + 1] === '/') { + j = line.length; // rest of line is a line comment + break; + } + if (line[j] === '/' && line[j + 1] === '*') { + inBlock = true; + j += 2; + continue; + } + result += line[j]; + j++; + } + out[i] = result; + } + return out; +} + +// A named function declaration, tolerating `export`/`async` prefixes — the +// SAME shape `enclosingFunction` looks backward for and `findSeamBypasses` +// uses to recognise (and skip) the seam functions' own definitions. +const FUNCTION_DECL_LINE_RE = /^\s*(?:export\s+)?(?:async\s+)?function\s+([A-Za-z_$][\w$]*)\s*\(/; + +/** + * Nearest preceding named-function declaration, scanning `lines` BACKWARD + * from `index`. Scopes an exemption to a FUNCTION, never a FILE — a whole- + * file owner exemption is precisely how `getMilestoneInfo` stayed invisible + * to an earlier drift guard (ADR-3180 Decision 4(d)'s own cautionary case, + * cross-referenced by this guard's header). + */ +function enclosingFunction(lines, index) { + for (let i = index; i >= 0; i--) { + const m = FUNCTION_DECL_LINE_RE.exec(lines[i]); + if (m) return m[1]; + } + return null; +} + +// One member of the `FieldPreservation` union, e.g. `'preserve-always'` — +// lowercase-with-dashes, single-quoted. +const POLICY_UNION_START_RE = /export\s+type\s+FieldPreservation\s*=/; +const POLICY_UNION_MEMBER_RE = /'([a-z][a-z-]*)'/g; + +/** + * Parse the members of `export type FieldPreservation = 'a' | 'b' | ...;` + * straight out of the executor's own source, so this guard cannot drift + * from the type it polices (a hardcoded copy of the union would be exactly + * the "declared here, enforced somewhere else" shape ADR-3408 exists to + * remove — this time inside the GUARD). In `src/state-transition.cts` the + * union is declared across several lines, each shaped like + * ` | 'clear'; // remove the field entirely`. Scans from the declaration + * line forward, collecting every quoted token on each line, and stops at + * the first line whose (comment-INCLUDING) text still carries a `;` — the + * statement terminator ends the union regardless of any trailing comment. + */ +function readPolicyUnion(text) { + const lines = text.split('\n'); + const members = []; + let inUnion = false; + for (const line of lines) { + if (!inUnion) { + if (!POLICY_UNION_START_RE.test(line)) continue; + inUnion = true; + } + POLICY_UNION_MEMBER_RE.lastIndex = 0; + let m; + while ((m = POLICY_UNION_MEMBER_RE.exec(line)) !== null) { + members.push(m[1]); + } + if (line.includes(';')) break; + } + return members; +} + +// `getFieldClassification(` called with a quoted string-literal argument — +// the field-name-keyed dispatch shape. `getFieldClassification(variable)` +// (a bare identifier, no quote) never matches this pattern, by construction +// (the quote-character backreference requires an opening quote immediately +// inside the parens) — the correct, table-driven shape is silent here. +const FIELD_NAME_DISPATCH_RE = /getFieldClassification\s*\(\s*(['"`])([^'"`]+)\1\s*\)/g; + +// `field === ''` / `field !== ''`, and the reversed +// `'' === field` — the field-name-keyed BRANCH shape (as opposed to +// `FIELD_NAME_DISPATCH_RE`'s field-name-keyed CALL shape above; both report +// the same `REASON.FIELD_NAME_DISPATCH`, since both are "a branch selected +// by field name", ADR-3408 §8.1's exact prohibition). This is the shape a +// bypass takes when it routes AROUND `getFieldClassification` entirely +// rather than through it — ADR-3408 Decision 5's "route the bypass through +// a wrapper or a differently-named local" gaming route. +// +// Deliberately scoped to ONLY an identifier literally named `field` — the +// dispatch loop's own loop variable declared at +// `for (const field of Object.keys(FIELD_CLASSIFICATION))` a few dozen lines +// below in this same file. This is a DECLARED, narrow limitation, not a +// silent one: a rename of the loop variable would evade this detector +// entirely, and an unrelated local elsewhere in this file that happens to +// also be named `field` would false-positive. Both risks are accepted +// in trade for avoiding a name-agnostic match, which would flag every +// unrelated `===`/`!==` string comparison in the file (there are many — +// e.g. `derivedName !== MILESTONE_PLACEHOLDER`-shaped guards) and bury the +// real signal in noise; per this guard's own header, over-reporting a +// comment is an accepted risk but over-reporting live code this broadly is +// not. +// +// The reversed `!==` form (`'' !== field`) is deliberately NOT +// matched — not observed anywhere in this codebase, and left out rather +// than speculatively widened past what was found in practice. +const FIELD_VAR_EQ_LITERAL_RE = /\bfield\s*(?:===|!==)\s*(['"`])([^'"`]+)\1/g; +const LITERAL_EQ_FIELD_VAR_RE = /(['"`])([^'"`]+)\1\s*===\s*\bfield\b/g; + +/** + * AXIS 1a: every `getFieldClassification('')` CALL, and every + * `field === ''` / `field !== ''` / `'' === + * field` BRANCH, inside the executor is a field-name-keyed branch (§8.1). + * Only ever called against `EXECUTOR_FILE` — `collect()` gates the call + * site, mirroring `findPolicyDispatchDrift`'s own "only when rel === + * EXECUTOR_FILE" rule from the spec this guard was authored against. + * `preservation === ''` comparisons — the CORRECT policy-dispatch + * shape `findUnimplementedPolicies` requires to exist — are unaffected: the + * identifier compared there is `preservation`, never `field`, so + * `FIELD_VAR_EQ_LITERAL_RE`'s `\bfield\b` anchor does not reach them. + */ +function findPolicyDispatchDrift(rel, text) { + const out = []; + const stripped = stripComments(text); + for (let i = 0; i < stripped.length; i++) { + const line = stripped[i]; + if (!line.trim()) continue; + // `file` is sanitized here, at construction, not just at the human + // formatter: `rel` is exactly as attacker-controlled as `source` on a + // fork PR (a tracked filename can legally carry C1 bytes or bidi + // overrides), and it reaches the committed baseline and `--json` stdout + // unfiltered otherwise — see `sanitizeForReport`'s own header. + FIELD_NAME_DISPATCH_RE.lastIndex = 0; + let m; + while ((m = FIELD_NAME_DISPATCH_RE.exec(line)) !== null) { + out.push({ + reason: REASON.FIELD_NAME_DISPATCH, + axis: 'policy-dispatch', + file: sanitizeForReport(rel), + line: i + 1, + // `field` is captured straight out of a quoted string literal in + // repo source — attacker-controlled on the same fork-PR basis as + // `file`/`source`, so sanitize it too rather than let it reach + // `--json` stdout / the baseline raw. + field: sanitizeForReport(m[2]), + source: sanitizeForReport(line.trim()), + }); + } + FIELD_VAR_EQ_LITERAL_RE.lastIndex = 0; + while ((m = FIELD_VAR_EQ_LITERAL_RE.exec(line)) !== null) { + out.push({ + reason: REASON.FIELD_NAME_DISPATCH, + axis: 'policy-dispatch', + file: sanitizeForReport(rel), + line: i + 1, + // `field` is captured straight out of a quoted string literal in + // repo source — attacker-controlled on the same fork-PR basis as + // `file`/`source`, so sanitize it too rather than let it reach + // `--json` stdout / the baseline raw. + field: sanitizeForReport(m[2]), + source: sanitizeForReport(line.trim()), + }); + } + LITERAL_EQ_FIELD_VAR_RE.lastIndex = 0; + while ((m = LITERAL_EQ_FIELD_VAR_RE.exec(line)) !== null) { + out.push({ + reason: REASON.FIELD_NAME_DISPATCH, + axis: 'policy-dispatch', + file: sanitizeForReport(rel), + line: i + 1, + // `field` is captured straight out of a quoted string literal in + // repo source — attacker-controlled on the same fork-PR basis as + // `file`/`source`, so sanitize it too rather than let it reach + // `--json` stdout / the baseline raw. + field: sanitizeForReport(m[2]), + source: sanitizeForReport(line.trim()), + }); + } + } + return out; +} + +/** + * AXIS 1b: every `FieldPreservation` member (read from `text` via + * `readPolicyUnion`, so the check cannot itself drift from the union) that + * has no `preservation === ''` comparison anywhere in the + * comment-stripped executor source is a declared policy with no executor — + * §8.1's mirror defect, one level up (a whole MEMBER unimplemented, not just + * one dispatch call keyed on a field name). `derive` and `clear` are the + * live instances ADR-3408 §8.6 names. + */ +function findUnimplementedPolicies(text, rel) { + const members = readPolicyUnion(text); + const strippedText = stripComments(text).join('\n'); + const out = []; + for (const member of members) { + const memberRe = new RegExp(`preservation\\s*===\\s*'${escapeRegex(member)}'`); + if (memberRe.test(strippedText)) continue; + // `file` and `policy` are sanitized here for the same reason as + // `findPolicyDispatchDrift` above: both `rel` and a `FieldPreservation` + // union member are attacker-controlled on a fork PR, exactly like + // `source`. + out.push({ + reason: REASON.UNIMPLEMENTED_POLICY, + axis: 'policy-dispatch', + file: sanitizeForReport(rel), + line: 0, + policy: sanitizeForReport(member), + source: sanitizeForReport(`FieldPreservation member '${member}' has no executor`), + }); + } + return out; +} + +// The two write-seam functions, matched only as CALLS (`\(` immediately +// after, modulo whitespace) — never as bare mentions of the name. +const SEAM_CALL_RE = /\b(writeStateMd|syncStateFrontmatter)\s*\(/g; +// A line that IS one of the two seam functions' own definitions — skipped +// outright, never counted as a call to itself. +const SEAM_DEF_LINE_RE = /^\s*(?:export\s+)?(?:async\s+)?function\s+(?:writeStateMd|syncStateFrontmatter)\b/; + +/** + * AXIS 2a: every direct `writeStateMd(`/`syncStateFrontmatter(` call in + * `text`, outside the two functions' own definitions and (only inside + * `SEAM_OWNER_FILE`) outside `SEAM_OWNER_EXEMPT_FUNCTIONS`'s own bodies. No + * `reason` on these findings — `applyRatchet` assigns one, since the same + * observed call site is a different failure shape depending on whether the + * baseline already knows about it. + */ +function findSeamBypasses(rel, text) { + const rawLines = text.split('\n'); + const stripped = stripComments(text); + const out = []; + for (let i = 0; i < stripped.length; i++) { + const line = stripped[i]; + if (!line.trim()) continue; + if (SEAM_DEF_LINE_RE.test(line)) continue; + SEAM_CALL_RE.lastIndex = 0; + let m; + while ((m = SEAM_CALL_RE.exec(line)) !== null) { + if (rel === SEAM_OWNER_FILE) { + const fn = enclosingFunction(stripped, i); + if (fn && SEAM_OWNER_EXEMPT_FUNCTIONS.includes(fn)) continue; + } + // `file` is sanitized here, at construction, not just at the human + // formatter: `rel` is exactly as attacker-controlled as `source` on a + // fork PR (a tracked filename can legally carry C1 bytes or bidi + // overrides), and it reaches the committed baseline and `--json` + // stdout unfiltered otherwise — see `sanitizeForReport`'s own header. + out.push({ + axis: 'write-seam', + file: sanitizeForReport(rel), + line: i + 1, + symbol: m[1], + source: sanitizeForReport(rawLines[i].trim()), + }); + } + } + return out; +} + +// Prose in the prompt layer shelling out to a write-side `gsd-tools` +// subcommand — the SAME write seam, expressed as markdown instructing an +// agent to run a command, rather than TypeScript calling a function +// directly (Decision 4(d)'s "the scan surface is declared, and is not just +// `src/`"). Matched on RAW lines — no comment stripping — because markdown +// carries no comment syntax this guard should be stripping in the first +// place. `g`-flagged so multiple candidate occurrences on one line are all +// checked against backtick spans below, rather than only the first. +const PROMPT_SEAM_RE = /gsd[-_]?tools[^\n]*\b(state\.patch|state\.planned-phase|state\.sync|phase\.complete)\b/g; + +/** + * Every `` `...` `` inline-code span on `line`, as `[start, end)` character + * ranges (end exclusive). Handles multiple spans on one line correctly by + * repeated `exec` over a global, non-overlapping backtick-pair pattern — + * unlike a naive "count backticks before the match" parity check, this does + * not get confused by a line that mixes code spans with unrelated literal + * backticks (e.g. an unmatched one in prose). + */ +const CODE_SPAN_RE = /`[^`\n]+`/g; +function codeSpanRanges(line) { + const ranges = []; + CODE_SPAN_RE.lastIndex = 0; + let m; + while ((m = CODE_SPAN_RE.exec(line)) !== null) { + ranges.push([m.index, m.index + m[0].length]); + } + return ranges; +} + +/** + * True when character offset `index` of `line` falls inside one of `line`'s + * inline-code spans. + */ +function isInsideCodeSpan(line, index) { + return codeSpanRanges(line).some(([start, end]) => index >= start && index < end); +} + +/** + * AXIS 2b: every prompt-layer line instructing an agent to shell out to a + * write-side `gsd-tools` subcommand. Same finding shape as + * `findSeamBypasses` (no `reason` — the ratchet assigns it), with a fixed + * `symbol` since there is no single function name to report for prose. + * + * A candidate occurrence enclosed in backticks is a MENTION, not an + * invocation, and is deliberately not reported — CONTRIBUTING.md's "Every + * `commit` invocation in shipped content must declare `--files`" section + * states the repo's settled convention verbatim: "Write the command + * reference in backticks — the repo's own convention — and it is correctly + * read as a mention." ADR-3180 Amendment 3 records the cost of getting this + * wrong: the first `lint-phase-enumeration-drift.cjs` flagged JSDoc that + * merely documented the canonical owner as drift, which trains readers to + * reflexively exempt documentation instead of trusting the guard — the + * opposite of Decision 4(a)'s intent. All 5 of this guard's original + * prompt-layer baseline entries were exactly this false positive. + */ +function findPromptSeamUses(rel, text) { + const lines = text.split('\n'); + const out = []; + for (let i = 0; i < lines.length; i++) { + const line = lines[i]; + PROMPT_SEAM_RE.lastIndex = 0; + let m; + while ((m = PROMPT_SEAM_RE.exec(line)) !== null) { + if (isInsideCodeSpan(line, m.index)) continue; + // `file` is sanitized here for the same reason as `findSeamBypasses` + // above: `rel` is attacker-controlled on a fork PR, exactly like + // `source`. + out.push({ + axis: 'write-seam', + file: sanitizeForReport(rel), + line: i + 1, + symbol: 'prompt-layer-state-write', + source: sanitizeForReport(line.trim()), + }); + } + } + return out; +} + +/** + * Ratchet key for one write-seam finding — `(file, TRIMMED source text)`, + * NEVER a line number, which churns on any unrelated edit to the same file + * (mirrors `qa-smell-ratchet.cjs`'s own key shape). `v.source` is already + * the trimmed, sanitized line text by the time it reaches this function. + */ +function ratchetKey(v) { + return `${v.file} ${v.source}`; +} + +/** + * Read `BASELINE_PATH`. Returns `{ entries: [] }` when the file is ABSENT + * (`ENOENT` — first run, or a fully-shrunk Phase 4 baseline that deleted the + * file — ADR-3408 §8.3's roster foresees exactly this end state); returns + * `{ entries: null, code }` when the file is present but could not be read + * OR could not be parsed/shaped (missing/malformed `entries` array) — `code` + * carries the underlying `fs` error code (e.g. `'EACCES'`) when the failure + * happened at the read step, `null` when it happened at the parse/shape + * step, so the caller can fail closed rather than silently ratcheting + * against nothing. Returns the parsed object when the read+parse succeed. + * + * Absent-vs-unreadable is deliberately NOT collapsed into one arm. This + * guard exists to catch write paths whose failure and success are + * output-identical (ADR-3180 / ADR-3408, "The failure mode that hides all + * of it") — a `catch { return { entries: [] } }` around the read would + * reproduce exactly that shape in the tool built to detect it: an + * unreadable baseline (EACCES, EISDIR, EIO, ...) would be silently + * indistinguishable from a legitimate absent one. Do not simplify this back + * into a single catch arm. + */ +function loadBaseline() { + let raw; + try { + raw = fs.readFileSync(BASELINE_PATH, 'utf8'); + } catch (err) { + if (err && err.code === 'ENOENT') return { entries: [] }; + return { entries: null, code: err && err.code ? err.code : 'UNKNOWN' }; + } + let doc; + try { + doc = JSON.parse(raw); + } catch { + return { entries: null, code: null }; + } + if (!doc || typeof doc !== 'object' || !Array.isArray(doc.entries)) return { entries: null, code: null }; + return doc; +} + +/** + * The ratchet — mirrors `scripts/qa-smell-ratchet.cjs`'s four invariants, + * applied to write-seam bypass COUNTS instead of QA-smell fingerprints: + * + * 1. An observed key absent from the baseline is a brand-new, + * unacknowledged bypass — `SEAM_BYPASS_UNRECORDED`. + * 2. An observed count greater than the acknowledged count is a NEW copy + * landing beside an already-acknowledged one — `SEAM_BYPASS_COUNT_GREW`. + * 3. An observed count less than the acknowledged count is a PARTIAL + * migration — some but not all call sites at this exact key were + * removed, and the baseline still claims the old, larger number — + * `SEAM_BYPASS_COUNT_SHRANK`. + * 4. A baseline key with zero current observations is a STALE + * acknowledgment: an entry may never outlive what it describes, and + * the baseline may only shrink (via `--baseline`, regenerated) — + * `BASELINE_ENTRY_STALE`. + * + * The occurrence COUNT (not just key presence) is what makes a partial + * migration visible at all: two byte-identical call sites in one file are + * otherwise a single indistinguishable key, so removing one of the two + * would silently vanish from a presence-only check while the acknowledgment + * still describes two. + * + * Every returned finding carries both `observed` and `acknowledged` counts. + */ +function applyRatchet(observed, baseline) { + const observedByKey = new Map(); + for (const finding of observed) { + const key = ratchetKey(finding); + let group = observedByKey.get(key); + if (!group) { + group = { count: 0, sample: finding }; + observedByKey.set(key, group); + } + group.count += 1; + } + + const baselineByKey = new Map(); + for (const entry of baseline.entries) { + baselineByKey.set(`${entry.file} ${entry.source}`, entry); + } + + const out = []; + for (const [key, group] of observedByKey) { + const entry = baselineByKey.get(key); + const acknowledged = entry && typeof entry.count === 'number' ? entry.count : 0; + let reason = null; + if (!entry) { + reason = REASON.SEAM_BYPASS_UNRECORDED; + } else if (group.count > acknowledged) { + reason = REASON.SEAM_BYPASS_COUNT_GREW; + } else if (group.count < acknowledged) { + reason = REASON.SEAM_BYPASS_COUNT_SHRANK; + } + if (!reason) continue; + out.push({ + reason, + axis: 'write-seam', + file: group.sample.file, + line: group.sample.line, + symbol: group.sample.symbol, + source: group.sample.source, + observed: group.count, + acknowledged, + }); + } + + for (const [key, entry] of baselineByKey) { + if (observedByKey.has(key)) continue; + out.push({ + reason: REASON.BASELINE_ENTRY_STALE, + axis: 'write-seam', + file: entry.file, + line: 0, + symbol: entry.symbol, + source: entry.source, + observed: 0, + acknowledged: typeof entry.count === 'number' ? entry.count : 0, + }); + } + + return out; +} + +/** + * Run both scan passes (the `src/` tree for Axis 1 + Axis 2a, the prompt + * layer for Axis 2b) and split the combined findings by `axis` into + * `{ policyFindings, seamFindings }`. `policyFindings` are already terminal + * (each carries its own `reason`); `seamFindings` are raw observations — + * `applyRatchet` is what turns them into (or clears them of) a finding. + */ +function collect() { + const srcFindings = scanTree({ + root: REPO_ROOT, + scanDirs: SRC_DIRS, + scanExt: SRC_EXT, + onFile(rel, text) { + const relPosix = toPosixRel(rel); + const found = []; + if (relPosix === EXECUTOR_FILE) { + found.push(...findPolicyDispatchDrift(relPosix, text)); + found.push(...findUnimplementedPolicies(text, relPosix)); + } + found.push(...findSeamBypasses(relPosix, text)); + return found; + }, + }); + + const promptFindings = scanTree({ + root: REPO_ROOT, + scanDirs: PROMPT_DIRS, + scanExt: PROMPT_EXT, + onFile(rel, text) { + return findPromptSeamUses(toPosixRel(rel), text); + }, + }); + + const all = [...srcFindings, ...promptFindings]; + return { + policyFindings: all.filter((f) => f.axis === 'policy-dispatch'), + seamFindings: all.filter((f) => f.axis === 'write-seam'), + }; +} + +/** + * Group `seamFindings` by `ratchetKey` into the baseline entry shape + * (`{file, source, symbol, count, owner}`), sorted by `file+source`. + * + * `owner` is NEVER invented by this mechanical regeneration — the guard can + * observe WHERE a bypass is and HOW MANY there are, but not which issue owns + * removing it; inventing one would violate the same "never guess" discipline + * `qa-smell-ratchet.cjs` applies to its own `issue` field (its `--update` + * never invents an issue number either). ADR-3408 §8.3 requires every + * shipped entry to carry "a named ratchet entry carrying the issue that owns + * its removal, never an unrecorded pass" — that owner is HUMAN-CURATED and + * must be recorded before the entry ships. + * + * Because `--baseline` overwrites `BASELINE_PATH` wholesale, a naive + * mechanical regeneration would silently re-null every curated `owner` on + * each run. To avoid that, `existingEntries` (the baseline as it stood + * BEFORE this regeneration, i.e. `loadBaseline().entries`) is optional and, + * when supplied, its `owner` values are merged forward onto matching new + * entries keyed on `(file, source)` — the same key `ratchetKey` / + * `applyRatchet` use to identify a bypass. A key with no prior entry (a + * brand-new bypass) still gets `owner: null`, exactly as before; only + * already-curated owners survive the regeneration. Re-running `--baseline` + * twice in a row is therefore idempotent with respect to `owner`. + */ +function buildBaselineEntries(seamFindings, existingEntries) { + const priorOwnerByKey = new Map(); + if (Array.isArray(existingEntries)) { + for (const entry of existingEntries) { + priorOwnerByKey.set(ratchetKey(entry), entry.owner); + } + } + + const groups = new Map(); + for (const finding of seamFindings) { + const key = ratchetKey(finding); + let group = groups.get(key); + if (!group) { + group = { + file: finding.file, + source: finding.source, + symbol: finding.symbol, + count: 0, + owner: priorOwnerByKey.has(key) ? priorOwnerByKey.get(key) : null, + }; + groups.set(key, group); + } + group.count += 1; + } + return [...groups.values()].sort((a, b) => ratchetKey(a).localeCompare(ratchetKey(b))); +} + +const BASELINE_COMMENT = + 'ADR-3408 Decision 5 write-seam ratchet baseline (issue #3468, Phase 1). Every entry here is a ' + + '`writeStateMd(`/`syncStateFrontmatter(` bypass this guard found by a whole-repo scan (Decision ' + + '4(a)) — it is ACKNOWLEDGED, not endorsed: acknowledgment is in writing (this file), with the ' + + 'issue owning its removal recorded in the entry\'s "owner" field. This baseline is SHRINK-ONLY — ' + + 'an entry that stops firing goes STALE and fails the plain run until `--baseline` is re-run to ' + + 'drop it (ADR-3180 Decision 4(e)\'s "the baseline may only shrink", adopted verbatim by ADR-3408). ' + + 'Phase 2 (#3469) removes the `cmdPhaseComplete` and `patchCore` entries when it lands the single ' + + 'write seam. Phase 4 (#3471) drives this baseline to empty and deletes this file. ' + + '`REGENERATE_STATE` (`src/health-diagnostic.cts`) is a SANCTIONED PERMANENT exception, not debt — ' + + 'it is `/gsd-health --repair`\'s factory reset, which rebuilds STATE.md from scratch, so ' + + 'preservation would restore exactly the values it was invoked to discard; do not "consolidate" ' + + 'its entry away.'; + +function writeBaseline(seamFindings) { + const priorBaseline = loadBaseline(); + const entries = buildBaselineEntries( + seamFindings, + Array.isArray(priorBaseline.entries) ? priorBaseline.entries : null, + ); + const doc = { _comment: BASELINE_COMMENT, entries }; + fs.writeFileSync(BASELINE_PATH, `${JSON.stringify(doc, null, 2)}\n`, 'utf8'); + return entries; +} + +const GOODHART_NOTE = + 'Goodhart note (ADR-3408 Decision 5): this "0 write-path bypasses" is a LAGGING metric — report ' + + 'it only alongside the behavioral identity test\'s result (asserted at the consumer\'s output), ' + + 'never alone.'; + +function printFindings(findings) { + for (const f of findings) { + process.stderr.write(`[${f.reason}] ${sanitizeForReport(f.file)}:${f.line}\n`); + process.stderr.write(` ${sanitizeForReport(f.source)}\n`); + } +} + +/** + * `argv`: `--baseline` regenerates `BASELINE_PATH` from a fresh scan and + * exits 0; `--json` (check mode only) prints the machine-readable finding + * set instead of the human-readable report. Exit codes: 0 clean, 1 drift + * (policy-dispatch violation, ratchet violation, or an unreadable + * baseline), 2 usage error. + */ +function main(argv) { + const args = argv || []; + const recognized = new Set(['--baseline', '--json']); + const unknown = args.filter((a) => !recognized.has(a)); + if (unknown.length > 0) { + process.stderr.write( + `lint-state-write-path-drift: unrecognized argument(s): ${unknown.map((a) => sanitizeForReport(a)).join(', ')} ` + + '(expected --baseline and/or --json)\n', + ); + process.exitCode = 2; + return; + } + + if (args.includes('--baseline')) { + const { seamFindings } = collect(); + const entries = writeBaseline(seamFindings); + process.stdout.write(`lint-state-write-path-drift: wrote ${entries.length} entries to ${BASELINE_PATH}\n`); + process.exitCode = 0; + return; + } + + const wantJson = args.includes('--json'); + const baseline = loadBaseline(); + + if (baseline.entries === null) { + const finding = { + reason: REASON.BASELINE_UNREADABLE, + axis: 'write-seam', + file: path.relative(REPO_ROOT, BASELINE_PATH), + line: 0, + symbol: null, + code: baseline.code, + source: sanitizeForReport( + baseline.code + ? `${BASELINE_PATH} is present but could not be read (${baseline.code}) — run \`node ${__filename} --baseline\` to regenerate it` + : `${BASELINE_PATH} is present but unparseable — run \`node ${__filename} --baseline\` to regenerate it`, + ), + }; + if (wantJson) { + process.stdout.write( + `${JSON.stringify( + { + ok: false, + findings: [finding], + summary: { policyDispatchViolations: 0, seamBypassesObserved: 0, seamBypassesAcknowledged: 0, ratchetViolations: 0 }, + }, + null, + 2, + )}\n`, + ); + } else { + printFindings([finding]); + } + process.exitCode = 1; + return; + } + + const { policyFindings, seamFindings } = collect(); + const ratchetFindings = applyRatchet(seamFindings, baseline); + const findings = [...policyFindings, ...ratchetFindings]; + const acknowledgedTotal = baseline.entries.reduce( + (sum, e) => sum + (typeof e.count === 'number' ? e.count : 0), + 0, + ); + const summary = { + policyDispatchViolations: policyFindings.length, + seamBypassesObserved: seamFindings.length, + seamBypassesAcknowledged: acknowledgedTotal, + ratchetViolations: ratchetFindings.length, + }; + + if (wantJson) { + process.stdout.write(`${JSON.stringify({ ok: findings.length === 0, findings, summary }, null, 2)}\n`); + process.exitCode = findings.length === 0 ? 0 : 1; + return; + } + + if (findings.length === 0) { + process.stdout.write( + 'ok state-write-path-drift: no policy-dispatch violations, no unrecorded/grown/shrunk/stale ' + + 'write-seam entries against the acknowledged baseline\n', + ); + process.stdout.write(`${GOODHART_NOTE}\n`); + process.exitCode = 0; + return; + } + + process.stderr.write( + 'state-write-path-drift: policy-dispatch and/or write-seam divergence found (ADR-3408 §8.1/§8.3). ' + + 'See docs/adr/3408-state-write-path-preservation.md for the contract:\n', + ); + printFindings(findings); + process.exitCode = 1; +} + +if (require.main === module) main(process.argv.slice(2)); + +module.exports = { + REASON, + REPO_ROOT, + BASELINE_PATH, + SRC_DIRS, + SRC_EXT, + PROMPT_DIRS, + PROMPT_EXT, + EXECUTOR_FILE, + SEAM_OWNER_FILE, + SEAM_OWNER_EXEMPT_FUNCTIONS, + toPosixRel, + stripComments, + enclosingFunction, + readPolicyUnion, + findPolicyDispatchDrift, + findUnimplementedPolicies, + findSeamBypasses, + findPromptSeamUses, + isInsideCodeSpan, + ratchetKey, + loadBaseline, + applyRatchet, + collect, + buildBaselineEntries, + main, +}; diff --git a/scripts/lint-test-file-count.allowlist.json b/scripts/lint-test-file-count.allowlist.json index 28f55910c..2140482aa 100644 --- a/scripts/lint-test-file-count.allowlist.json +++ b/scripts/lint-test-file-count.allowlist.json @@ -92,6 +92,7 @@ "state-prune.test.cjs", "state-rebuild-cli.test.cjs", "state-rebuild.test.cjs", + "state-write-path-drift-guard.test.cjs", "state.test.cjs" ], "issue": "180" diff --git a/scripts/state-write-path-drift-baseline.json b/scripts/state-write-path-drift-baseline.json new file mode 100644 index 000000000..4f6b4ae9d --- /dev/null +++ b/scripts/state-write-path-drift-baseline.json @@ -0,0 +1,33 @@ +{ + "_comment": "ADR-3408 Decision 5 write-seam ratchet baseline (issue #3468, Phase 1). Every entry here is a `writeStateMd(`/`syncStateFrontmatter(` bypass this guard found by a whole-repo scan (Decision 4(a)) — it is ACKNOWLEDGED, not endorsed: acknowledgment is in writing (this file), with the issue owning its removal recorded in the entry's \"owner\" field. This baseline is SHRINK-ONLY — an entry that stops firing goes STALE and fails the plain run until `--baseline` is re-run to drop it (ADR-3180 Decision 4(e)'s \"the baseline may only shrink\", adopted verbatim by ADR-3408). Phase 2 (#3469) removes the `cmdPhaseComplete` and `patchCore` entries when it lands the single write seam. Phase 4 (#3471) drives this baseline to empty and deletes this file. `REGENERATE_STATE` (`src/health-diagnostic.cts`) is a SANCTIONED PERMANENT exception, not debt — it is `/gsd-health --repair`'s factory reset, which rebuilds STATE.md from scratch, so preservation would restore exactly the values it was invoked to discard; do not \"consolidate\" its entry away.", + "entries": [ + { + "file": "src/health-diagnostic.cts", + "source": "writeStateMd(statePath, stateContent, cwd);", + "symbol": "writeStateMd", + "count": 1, + "owner": "sanctioned-permanent" + }, + { + "file": "src/milestone.cts", + "source": "writeStateMd(statePath, result.content, cwd);", + "symbol": "writeStateMd", + "count": 1, + "owner": "#3471" + }, + { + "file": "src/phase.cts", + "source": "const synced = syncStateFrontmatter(stateContent, cwd, authoritativeFm);", + "symbol": "syncStateFrontmatter", + "count": 1, + "owner": "#3469" + }, + { + "file": "src/state.cts", + "source": "writeStateMd(statePath, modified, cwd);", + "symbol": "writeStateMd", + "count": 1, + "owner": "#3471" + } + ] +} diff --git a/src/state-transition.cts b/src/state-transition.cts index d6c803587..12f8b8ae9 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -51,10 +51,31 @@ export type FieldPreservation = | 'derive' // always re-derive from source | 'preserve-when-unchanged' // #1230 delta heuristic: keep existing if body source field unchanged | 'preserve-always' // never overwrite unless the caller explicitly names this field - | 'preserve-if-placeholder' // overwrite only when derived value is a known placeholder (#948) - | 'clear'; // remove the field entirely + | 'preserve-if-placeholder'; // overwrite only when derived value is a known placeholder (#948) +// ADR-3408 §8.6 amendment: 'clear' was deleted (no row used it, no executor +// existed) rather than implemented — Speculative Generality, a policy +// invented for a need that never arrived. -export type FieldClassification = { source: FieldSource; preservation: FieldPreservation }; +/** + * ADR-3408 Decision 1 (Greenspun's Tenth Rule): guards and merge strategies + * are named members of a CLOSED vocabulary, never an open predicate slot. The + * table has already accreted five times (`preserve-always` #1743/#1695, + * `preserve-if-placeholder` #948/#2135, `state_head` #2573, `deriveProgressKeys` + * #2440, `bodyDeltas` #3258) — an open per-row predicate is what would turn + * this table into an interpreter. Adding a member to either union below is an + * amendment to ADR-3408, not a table edit. + */ +export type FieldGuard = 'non-sentinel-unknown'; +export type FieldMergeStrategy = 'progress-ratchet'; + +export type FieldClassification = { + source: FieldSource; + preservation: FieldPreservation; + /** Closed vocabulary (see the comment above). Adding a member is an ADR-3408 amendment, not a table edit. */ + guard?: FieldGuard; + /** Closed vocabulary. Same rule. */ + mergeStrategy?: FieldMergeStrategy; +}; /** * Single source of truth for "which fields win when frontmatter and body @@ -83,11 +104,17 @@ export const FIELD_CLASSIFICATION: Readonly> // Phase / plan position (body-derived) current_phase: { source: 'body', preservation: 'preserve-when-unchanged' } as FieldClassification, - current_phase_name: { source: 'curated', preservation: 'preserve-always' } as FieldClassification, // #1743, #1695 + // #1743, #1695. #3468: row corrected to match its long-standing behavior + // — was declared preserve-always, has always been delta-gated (only + // restores when the body `Phase:` source is unchanged this write). + current_phase_name: { source: 'curated', preservation: 'preserve-when-unchanged' } as FieldClassification, current_plan: { source: 'body', preservation: 'preserve-when-unchanged' } as FieldClassification, // Status / lifecycle (body-derived; #1230 delta heuristic applies) - status: { source: 'body', preservation: 'preserve-when-unchanged' } as FieldClassification, + // guard: the 'unknown' sentinel is the ONLY true executor-side guard in + // this table (stopped_at's `## Session` scoping is caller-side delta + // extraction, not an executor condition) — ADR-3408 Decision 1. + status: { source: 'body', preservation: 'preserve-when-unchanged', guard: 'non-sentinel-unknown' } as FieldClassification, stopped_at: { source: 'body', preservation: 'preserve-when-unchanged' } as FieldClassification, paused_at: { source: 'body', preservation: 'preserve-when-unchanged' } as FieldClassification, @@ -102,7 +129,10 @@ export const FIELD_CLASSIFICATION: Readonly> state_head: { source: 'free', preservation: 'derive' } as FieldClassification, // #2573 // Progress block (disk-derived, except the curated progress ratchet) - progress: { source: 'curated', preservation: 'preserve-always' } as FieldClassification, // #3242, #1446 + // mergeStrategy: 'progress-ratchet' — completed_plans/completed_phases + // only ever ratchet UP toward the derived value (#2969); everything + // else in the merge is either always-derived (#2440) or always-curated. + progress: { source: 'curated', preservation: 'preserve-always', mergeStrategy: 'progress-ratchet' } as FieldClassification, // #3242, #1446 'progress.total_phases': { source: 'disk', preservation: 'derive' } as FieldClassification, 'progress.completed_phases': { source: 'disk', preservation: 'derive' } as FieldClassification, 'progress.total_plans': { source: 'disk', preservation: 'derive' } as FieldClassification, @@ -144,12 +174,6 @@ export type StatePreservationInput = { preFmSnapshot: Record; /** True when the caller asked for a full disk re-derivation (sync / advancePlan / completePhase). */ resync: boolean; - preBodyStatus: string | null; - postBodyStatus: string | null; - preBodyStoppedAt: string | null; - postBodyStoppedAt: string | null; - preBodyPhaseSource: string | null; - postBodyPhaseSource: string | null; /** * #2440: when true, total_plans and total_phases take the derived (post-sync) * value even under !resync, instead of the wholesale curated restore. Used @@ -159,16 +183,18 @@ export type StatePreservationInput = { */ deriveProgressKeys?: boolean; /** - * #3258: pre/post body-source values for the preserve-when-unchanged (#1230 - * delta heuristic) family, keyed by frontmatter field. The caller snapshots - * each field's body source before/after the transform (mirroring how - * buildStateFrontmatter derives it); applyStatePreservation consults - * FIELD_CLASSIFICATION and, for every row whose policy is preserve-when- - * unchanged that is NOT already on a dedicated channel (status, stopped_at), + * #3258 / #3468: pre/post body-source values for every preserve-when-unchanged + * (#1230 delta heuristic) row, keyed by the FRONTMATTER field the policy + * guards — one channel for all seven rows (status, stopped_at, + * current_phase_name, current_phase, current_plan, paused_at, + * last_activity_desc), folded from the two channels (a bodyDeltas map plus + * three dedicated pre/post parameter pairs) #3468 replaced. The caller + * snapshots each field's body source before/after the transform (mirroring + * how buildStateFrontmatter derives it); `applyPreserveWhenUnchanged` * restores the pre-write frontmatter value when that body source was left - * unchanged by this write. Adding a preserve-when-unchanged row is a one-row - * table edit PLUS a bodyDeltas entry from the caller; the invariant test in - * tests/state-transition.test.cjs fails if either is missing. + * unchanged by this write. Adding a preserve-when-unchanged row is a + * one-row table edit PLUS a bodyDeltas entry from the caller — omitting the + * entry is not a silent no-op, it THROWS (ADR-3408 §8.2, `applyPreserveWhenUnchanged`). */ bodyDeltas?: Record; }; @@ -179,187 +205,225 @@ export type StatePreservationResult = { }; /** - * Pure, table-driven post-sync preservation. Mutates `postFm` in place to - * mirror the pre-consolidation inline block (which also mutated in place) and - * returns whether any field was restored. + * Mutable, single-write dispatch context threaded through every policy + * executor below. `mutated` accumulates across the whole field loop (ADR-3408 + * §8.1 — one executor per policy, sharing one result). */ -export function applyStatePreservation(input: StatePreservationInput): StatePreservationResult { - const { preFm, postFm, preFmSnapshot, resync } = input; - let mutated = false; +type PreservationCtx = { + preFm: Record | null; + postFm: Record; + preFmSnapshot: Record; + resync: boolean; + deriveProgressKeys: boolean; + bodyDeltas: Record | undefined; + mutated: boolean; +}; - // Curated progress ratchet (#3242/#1446; closes the #1264 class by routing - // the policy through the table). Restored only when the table says preserve- - // always AND this transition is not re-deriving from disk (!resync). sync and - // the lifecycle transitions pass resync=true and recompute; patch/update and - // body-only writes pass resync=false and keep the curated counters. - const progressCls = getFieldClassification('progress'); - if ( - progressCls !== null && - progressCls.preservation === 'preserve-always' && - !resync && - preFm && - preFm['progress'] - ) { - // #2440: when the caller opts in (deriveProgressKeys), total_plans and - // total_phases always take the derived (post-sync) value even under !resync. - // This is used by cmdStatePlannedPhase where total_plans must correct upward - // after plans are added. For body-only writes (state.update/patch without - // the flag), the wholesale restore preserves everything as before — the - // #3242 Bug A protection stays fully in force. - if (input.deriveProgressKeys && postFm['progress']) { - const curated = preFm['progress'] as Record | null; - const derived = (postFm['progress'] ?? {}) as Record; - const merged: Record = { ...derived }; - if (curated) { - // #2440: total_plans and total_phases always take the derived value. - // #2969: completed_plans and completed_phases take the derived value - // when it is GREATER than the curated value (gap-closure plans that - // completed after the plan count grew) — ratcheting UP only, never - // deriving downward (preserves the #3242 curated-progress protection - // for cases unrelated to plan-count growth, e.g. a deleted SUMMARY). - // percent also takes the derived value — the resync recomputed it from - // disk counts, and a stale curated percent would be incoherent against - // the ratcheted-up completed counts (e.g. 54/54 at 93%). - const ratchetUpKeys = new Set(['completed_plans', 'completed_phases']); - for (const [key, value] of Object.entries(curated)) { - if (key === 'total_plans' || key === 'total_phases' || key === 'percent') continue; - if (ratchetUpKeys.has(key)) { - const derivedNum = typeof derived[key] === 'number' ? derived[key] : -Infinity; - const curatedNum = typeof value === 'number' ? value : -Infinity; - // Take the derived value only when it ratchets up; else keep curated. - if (derivedNum > curatedNum) continue; - merged[key] = value; - } else { - merged[key] = value; - } +/** + * ADR-3408 §8.2: an unenforced `preserve-when-unchanged` row throws. Both + * ends of this invariant are gsd-core's own source — a declared row the + * *caller code* forgot to wire via `bodyDeltas` — so it is a programming + * error, unreachable from any user document. The bright line, stated because + * conflating its two sides would be severe: a drifted, malformed, or + * unparseable user STATE.md NEVER reaches this throw (§8.5 governs that case + * with preserve-and-warn); this fires only when the *caller* omitted a + * `bodyDeltas` entry for a row the table itself declares. Getting this + * backwards turns every desynced project's `phase.complete` into a hard + * failure. + */ +function throwUnwiredRow(field: string): never { + const err = new Error( + `applyStatePreservation: preserve-when-unchanged row ${JSON.stringify(field)} reached the ` + + 'executor with no wired ctx.bodyDeltas entry. This is an internal invariant violation (ADR-3408 ' + + '§8.2) — the caller (readModifyWriteStateMd) forgot to supply this field\'s body-source delta. ' + + 'Add a bodyDeltas entry for this field per ADR-3408 §8.3, or remove the row from ' + + 'FIELD_CLASSIFICATION if the field no longer needs this policy.', + ) as Error & { code: string; field: string }; + err.code = 'STATE_PRESERVATION_UNWIRED_ROW'; + err.field = field; + throw err; +} + +/** + * Executor for `preservation: 'preserve-when-unchanged'` (ADR-3408 §8.1). The + * #1230 delta heuristic: restore the pre-write frontmatter snapshot when this + * write did not change the field's body source, and the snapshot is a real + * (non-empty-after-trim) curated value the derived value should not clobber. + * + * Every row carrying this policy — status, stopped_at, current_phase_name, + * current_phase, current_plan, paused_at, last_activity_desc — is honored by + * this ONE executor; `cls.guard` is the only field-specific variation (the + * closed vocabulary of ADR-3408 Decision 1). + */ +function applyPreserveWhenUnchanged(field: string, cls: FieldClassification, ctx: PreservationCtx): void { + // 1. A declared row with no wired delta is an internal invariant violation + // — throw (ADR-3408 §8.2). Never reached for a user-document defect: the + // production caller (readModifyWriteStateMd) wires every preserve-when- + // unchanged row unconditionally. + const delta = ctx.bodyDeltas ? ctx.bodyDeltas[field] : undefined; + if (!delta) throwUnwiredRow(field); + + // 2. Only a real, non-whitespace-only curated string is worth restoring + // (#3468: tightened from `.length > 0` to a trimmed check — a whitespace- + // only snapshot is not a real curated value). + const snapshot = ctx.preFmSnapshot[field]; + if (typeof snapshot !== 'string' || snapshot.trim().length === 0) return; + + // 3. Closed-vocabulary guard: status's 'unknown' sentinel is never restored. + // Exact-match, case-sensitive — 'Unknown' is a real value and IS restored. + if (cls.guard === 'non-sentinel-unknown' && snapshot === 'unknown') return; + + // 4. The body source changed this write → the freshly-derived value wins. + if (delta.pre !== delta.post) return; + + // 5. Already correct → no-op (avoid a spurious `mutated=true`). + if (ctx.postFm[field] === snapshot) return; + + // 6. Restore. + ctx.postFm[field] = snapshot; + ctx.mutated = true; +} + +/** + * Executor for `preservation: 'preserve-always'` (ADR-3408 §8.1). Only + * `progress` carries this policy today. Preserves #3242/#1446/#2440/#2969 + * semantics byte-for-byte: gated on `!resync` and a truthy `preFm[field]`; + * the `mergeStrategy: 'progress-ratchet'` per-key merge only fires when the + * caller opts in via `deriveProgressKeys`, else the whole curated block wins + * wholesale. + */ +function applyPreserveAlways(field: string, cls: FieldClassification, ctx: PreservationCtx): void { + if (ctx.resync || !ctx.preFm || !ctx.preFm[field]) return; + + if (cls.mergeStrategy === 'progress-ratchet' && ctx.deriveProgressKeys && ctx.postFm[field]) { + // #2440: total_plans and total_phases always take the derived (post-sync) + // value even under !resync. This is used by cmdStatePlannedPhase where + // total_plans must correct upward after plans are added. For body-only + // writes (state.update/patch without the flag), the wholesale restore + // below preserves everything as before — the #3242 Bug A protection + // stays fully in force. + const curated = ctx.preFm[field] as Record | null; + const derived = (ctx.postFm[field] ?? {}) as Record; + const merged: Record = { ...derived }; + if (curated) { + // #2440: total_plans and total_phases always take the derived value. + // #2969: completed_plans and completed_phases take the derived value + // when it is GREATER than the curated value (gap-closure plans that + // completed after the plan count grew) — ratcheting UP only, never + // deriving downward (preserves the #3242 curated-progress protection + // for cases unrelated to plan-count growth, e.g. a deleted SUMMARY). + // percent also takes the derived value — the resync recomputed it from + // disk counts, and a stale curated percent would be incoherent against + // the ratcheted-up completed counts (e.g. 54/54 at 93%). + const ratchetUpKeys = new Set(['completed_plans', 'completed_phases']); + for (const [key, value] of Object.entries(curated)) { + if (key === 'total_plans' || key === 'total_phases' || key === 'percent') continue; + if (ratchetUpKeys.has(key)) { + const derivedNum = typeof derived[key] === 'number' ? derived[key] : -Infinity; + const curatedNum = typeof value === 'number' ? value : -Infinity; + // Take the derived value only when it ratchets up (strictly + // greater — #2969's `>` not `>=`); else keep curated. + if (derivedNum > curatedNum) continue; + merged[key] = value; + } else { + merged[key] = value; } } - postFm['progress'] = merged; - } else { - postFm['progress'] = preFm['progress']; } - mutated = true; + ctx.postFm[field] = merged; + } else { + ctx.postFm[field] = ctx.preFm[field]; } + ctx.mutated = true; +} - // status — #1230 body-delta heuristic. Table: preserve-when-unchanged. - const statusCls = getFieldClassification('status'); +/** + * Executor for `preservation: 'preserve-if-placeholder'` (ADR-3408 §8.1). + * `milestone` and `milestone_name` both carry this policy in the table, and + * both rows dispatch into this SAME executor body — no branch is selected by + * field name (ADR-3408 §8.1). The body always restores the name+version pair + * together (#948/#2135), ignoring which of the two rows triggered the call; + * this is deliberately safe because the executor is idempotent: whichever + * row fires first either performs the restore (after which the second row's + * call recomputes against already-restored state and finds nothing left to + * do) or finds no placeholder to restore (in which case the second row's + * call, seeing the same unchanged inputs, reaches the same conclusion). Two + * dispatches per write converge to the identical single-pass result, so the + * field argument itself is unused here — it exists only to satisfy the + * shared executor signature every policy branch in the dispatch loop shares. + */ +function applyPreserveIfPlaceholder(_field: string, _cls: FieldClassification, ctx: PreservationCtx): void { + const MILESTONE_PLACEHOLDER = 'milestone'; + const derivedName = ctx.postFm['milestone_name']; + const derivedLooksLikeName = typeof derivedName === 'string' + && derivedName.length > 0 + && derivedName !== MILESTONE_PLACEHOLDER + && !/^[\s—–:-]/.test(derivedName); + const snapshotName = ctx.preFmSnapshot['milestone_name']; + const snapshotNameIsReal = typeof snapshotName === 'string' + && snapshotName.length > 0 + && snapshotName !== MILESTONE_PLACEHOLDER; + if (derivedLooksLikeName || !snapshotNameIsReal) return; + + if (ctx.postFm['milestone_name'] !== snapshotName) { + ctx.postFm['milestone_name'] = snapshotName; + ctx.mutated = true; + } + const snapshotVersion = ctx.preFmSnapshot['milestone']; if ( - statusCls !== null && - statusCls.preservation === 'preserve-when-unchanged' && - input.postBodyStatus === input.preBodyStatus && - typeof preFmSnapshot['status'] === 'string' && - preFmSnapshot['status'].length > 0 && - preFmSnapshot['status'] !== 'unknown' && - postFm['status'] !== preFmSnapshot['status'] + typeof snapshotVersion === 'string' && snapshotVersion.length > 0 && + ctx.postFm['milestone'] !== snapshotVersion ) { - postFm['status'] = preFmSnapshot['status']; - mutated = true; + ctx.postFm['milestone'] = snapshotVersion; + ctx.mutated = true; } +} - // stopped_at — same #1230 body-delta heuristic. Table: preserve-when-unchanged. - const stoppedCls = getFieldClassification('stopped_at'); - if ( - stoppedCls !== null && - stoppedCls.preservation === 'preserve-when-unchanged' && - input.postBodyStoppedAt === input.preBodyStoppedAt && - typeof preFmSnapshot['stopped_at'] === 'string' && - preFmSnapshot['stopped_at'].length > 0 && - postFm['stopped_at'] !== preFmSnapshot['stopped_at'] - ) { - postFm['stopped_at'] = preFmSnapshot['stopped_at']; - mutated = true; - } +/** + * Executor for `preservation: 'derive'`. Explicit no-op — the sync's + * freshly-derived value stands untouched. Naming this executor (rather than + * skipping `derive` rows by omission) is what makes ADR-3408 §8.2's throw + * decidable: "policy says do nothing" is now distinguishable from "nobody + * wired this", because every member of `FieldPreservation` reaches an + * executor. + */ +function applyDerive(_field: string, _cls: FieldClassification, _ctx: PreservationCtx): void { + // No-op by design — see docstring. +} - // current_phase_name — curated (#1743/#1695). Table: preserve-always. - const phaseNameCls = getFieldClassification('current_phase_name'); - if ( - phaseNameCls !== null && - phaseNameCls.preservation === 'preserve-always' && - input.postBodyPhaseSource === input.preBodyPhaseSource && - typeof preFmSnapshot['current_phase_name'] === 'string' && - preFmSnapshot['current_phase_name'].length > 0 && - postFm['current_phase_name'] !== preFmSnapshot['current_phase_name'] - ) { - postFm['current_phase_name'] = preFmSnapshot['current_phase_name']; - mutated = true; - } +/** + * Pure, table-driven post-sync preservation (ADR-3408 §8.1). One loop over + * `FIELD_CLASSIFICATION`, dispatching on the row's `preservation` value — + * never on the field name. Mutates `postFm` in place to mirror the + * pre-#3468 inline block (which also mutated in place) and returns whether + * any field was restored. + */ +export function applyStatePreservation(input: StatePreservationInput): StatePreservationResult { + const ctx: PreservationCtx = { + preFm: input.preFm, + postFm: input.postFm, + preFmSnapshot: input.preFmSnapshot, + resync: input.resync, + deriveProgressKeys: input.deriveProgressKeys === true, + bodyDeltas: input.bodyDeltas, + mutated: false, + }; - // ─── #3258: the remaining preserve-when-unchanged rows ──────────────────── - // - // status and stopped_at (above) carry field-specific guards (the 'unknown' - // sentinel; ## Session scoping done by the caller) and stay on their - // dedicated blocks. The other preserve-when-unchanged rows — - // last_activity_desc, paused_at, current_phase, current_plan — share a - // uniform #1230 delta heuristic with no extra sentinel, so they are driven - // from the table in one loop. This is the reconciliation the maintainer - // rescope to #3258 asked for: the row is honored by the table-consuming pass - // itself, not only by a weaker absent-value fallback elsewhere. - // - // For last_activity_desc this replaces preferNewerLastActivity's date-based - // desc writes (removed from syncStateFrontmatter + cmdStateJson): the field - // is now governed by exactly one rule, this one. For paused_at / current_phase - // / current_plan the declared delta heuristic is strictly stronger than the - // #905 absent-fallback that syncStateFrontmatter still applies on the - // writeStateMd / cmdStateJson paths this pass does not reach — same policy, - // two enforcement points, they agree by construction (both restore the - // pre-write snapshot value when they fire). - const bodyDeltas = input.bodyDeltas; - if (bodyDeltas) { - for (const field of Object.keys(FIELD_CLASSIFICATION)) { - const cls = getFieldClassification(field); - if (!cls || cls.preservation !== 'preserve-when-unchanged') continue; - // status / stopped_at stay on their dedicated blocks above. - if (field === 'status' || field === 'stopped_at') continue; - const delta = bodyDeltas[field]; - if (!delta) continue; // caller did not wire this field's body source → cannot decide - const snapshot = preFmSnapshot[field]; - if (typeof snapshot !== 'string' || snapshot.length === 0) continue; - if (delta.pre !== delta.post) continue; // body source changed this write → derived wins - if (postFm[field] === snapshot) continue; // already correct, no-op - postFm[field] = snapshot; - mutated = true; + for (const field of Object.keys(FIELD_CLASSIFICATION)) { + const cls = getFieldClassification(field); + if (!cls) continue; + if (cls.preservation === 'preserve-when-unchanged') { + applyPreserveWhenUnchanged(field, cls, ctx); + } else if (cls.preservation === 'preserve-always') { + applyPreserveAlways(field, cls, ctx); + } else if (cls.preservation === 'preserve-if-placeholder') { + applyPreserveIfPlaceholder(field, cls, ctx); + } else if (cls.preservation === 'derive') { + applyDerive(field, cls, ctx); } } - // ─── #3258: preserve-if-placeholder — milestone / milestone_name ────────── - // - // The derived milestone_name (from ROADMAP.md via buildStateFrontmatter) must - // not clobber a curated name when it resolves to the template placeholder - // 'milestone' or a punctuation-led fragment. Mirrors the #948/#2135 guard in - // syncStateFrontmatter so the table pass enforces the same policy on the RMW - // path; syncStateFrontmatter remains the enforcement point on the writeStateMd - // / cmdStateJson paths this pass does not reach (same policy, two points). - // The milestone VERSION is restored alongside the name, exactly as the sync - // guard does, so the two stay consistent. - const milestoneNameCls = getFieldClassification('milestone_name'); - if (milestoneNameCls !== null && milestoneNameCls.preservation === 'preserve-if-placeholder') { - const MILESTONE_PLACEHOLDER = 'milestone'; - const derivedName = postFm['milestone_name']; - const derivedLooksLikeName = typeof derivedName === 'string' - && derivedName.length > 0 - && derivedName !== MILESTONE_PLACEHOLDER - && !/^[\s—–:-]/.test(derivedName); - const snapshotName = preFmSnapshot['milestone_name']; - const snapshotNameIsReal = typeof snapshotName === 'string' - && snapshotName.length > 0 - && snapshotName !== MILESTONE_PLACEHOLDER; - if (!derivedLooksLikeName && snapshotNameIsReal) { - if (postFm['milestone_name'] !== snapshotName) { - postFm['milestone_name'] = snapshotName; - mutated = true; - } - const snapshotVersion = preFmSnapshot['milestone']; - if ( - typeof snapshotVersion === 'string' && snapshotVersion.length > 0 && - postFm['milestone'] !== snapshotVersion - ) { - postFm['milestone'] = snapshotVersion; - mutated = true; - } - } - } - - return { postFm, mutated }; + return { postFm: ctx.postFm, mutated: ctx.mutated }; } // ---------------------------------------------------------------------------- @@ -1108,8 +1172,8 @@ function completePhaseCore( } // Current Phase Name — only written when a next-phase display name is known - // (#1743/#1695: classified curated/preserve-always, so an absent name does - // NOT clear an existing curated value). + // (#1743/#1695: classified curated/preserve-when-unchanged, so an absent + // name does NOT clear an existing curated value). if (nextPhaseDisplayName) { const after = stateReplaceField(body, 'Current Phase Name', nextPhaseDisplayName); if (after) { @@ -1369,10 +1433,11 @@ function plannedPhaseCore( * preserved. * * This is a destructive reset intent: it intentionally overwrites the curated - * `progress` / `current_phase_name` fields (classified preserve-always) because - * a new milestone starts from zero. That is the intent's contract, not a - * violation of the field-classification table — the table governs the steady- - * state RMW transitions; a milestone boundary is an explicit reset. + * `progress` (preserve-always) / `current_phase_name` (preserve-when-unchanged) + * fields because a new milestone starts from zero. That is the intent's + * contract, not a violation of the field-classification table — the table + * governs the steady-state RMW transitions; a milestone boundary is an + * explicit reset. * * The adapter wraps this in `acquireStateLock` + `platformWriteSync` (NOT * `readModifyWriteStateMd`) because milestoneSwitch rebuilds frontmatter @@ -1635,7 +1700,9 @@ function milestoneCompleteCore( * * The curated-field preservation that fixes #1743/#1695 is NOT in this core — * it lives in `readModifyWriteStateMd`'s post-sync delta (table-driven via - * `getFieldClassification('current_phase_name').preservation === 'preserve-always'`). + * `current_phase_name`'s `preserve-when-unchanged` row, ADR-3408 §8.1 — + * reclassified from `preserve-always` in #3468 to match its long-standing, + * delta-gated behavior). * `patch` consulting the table "refuses to overwrite" curated fields implicitly: * when the patch does not change a curated field's body source line, the * existing frontmatter value wins over the sync re-derivation. The adapter diff --git a/src/state.cts b/src/state.cts index 287f42c61..4fb8cd3c7 100644 --- a/src/state.cts +++ b/src/state.cts @@ -2880,11 +2880,27 @@ function applyPostSyncPreservation( ?? stateExtractField(postBody, 'Last activity'); const postBodyLastActivityDesc = stateExtractField(postBody, 'Last Activity Description') ?? parseProseLastActivityField(postBodyLastActivityRaw).description; + // #3468: single channel for every preserve-when-unchanged row. Before this + // change, seven body-source pre/post pairs travelled in two different + // shapes — this map for four fields, six dedicated parameters + // (preBodyStatus/postBodyStatus, preBodyStoppedAt/postBodyStoppedAt, + // preBodyPhaseSource/postBodyPhaseSource) for the other three — same data, + // same purpose, which is exactly why applyStatePreservation needed a + // hand-written branch per field instead of one loop over the table. Every + // row FIELD_CLASSIFICATION declares preserve-when-unchanged MUST appear + // here — an omission now throws (STATE_PRESERVATION_UNWIRED_ROW, ADR-3408 + // §8.2) at the first write rather than becoming a quiet preservation bug. + // Note current_phase_name's source is the body `Phase:` line, deliberately + // a DIFFERENT source from current_phase's: the key names the field the + // policy GUARDS, not the body field it reads. const bodyDeltas = { last_activity_desc: { pre: preBodyLastActivityDesc, post: postBodyLastActivityDesc }, paused_at: { pre: preBodyPausedAt, post: postBodyPausedAt }, current_phase: { pre: preBodyCurrentPhase, post: postBodyCurrentPhase }, current_plan: { pre: preBodyCurrentPlan, post: postBodyCurrentPlan }, + status: { pre: preBodyStatus, post: postBodyStatus }, + stopped_at: { pre: preBodyStoppedAt, post: postBodyStoppedAt }, + current_phase_name: { pre: preBodyPhaseSource, post: postBodyPhaseSource }, }; // ADR-1769 #1796 (Path A — finish the consolidation): the post-sync @@ -2902,9 +2918,6 @@ function applyPostSyncPreservation( preFm, postFm, preFmSnapshot, resync, deriveProgressKeys: deriveProgressKeys === true, bodyDeltas, - preBodyStatus, postBodyStatus, - preBodyStoppedAt, postBodyStoppedAt, - preBodyPhaseSource, postBodyPhaseSource, }); // #2736: re-assert the intent-first values AFTER preservation. On STATE.md // layouts with no body `Phase:` line, both phase-source snapshots are null diff --git a/tests/state-transition.test.cjs b/tests/state-transition.test.cjs index 0ee9f6143..372293f28 100644 --- a/tests/state-transition.test.cjs +++ b/tests/state-transition.test.cjs @@ -34,7 +34,6 @@ describe('ADR-1769 substrate: field-classification table', () => { 'preserve-when-unchanged', 'preserve-always', 'preserve-if-placeholder', - 'clear', ]); test('every classified field has a { source, preservation } row with known enum values', () => { @@ -50,10 +49,10 @@ describe('ADR-1769 substrate: field-classification table', () => { } }); - test('current_phase_name is curated / preserve-always (ADR-1769 §4 — kills #1743/#1695 by construction)', () => { + test('current_phase_name is curated / preserve-when-unchanged (ADR-1769 §4 — kills #1743/#1695 by construction; ADR-3408 #3468 reclassified from preserve-always to match its long-standing delta-gated behavior)', () => { const cls = getFieldClassification('current_phase_name'); assert.strictEqual(cls && cls.source, 'curated'); - assert.strictEqual(cls && cls.preservation, 'preserve-always'); + assert.strictEqual(cls && cls.preservation, 'preserve-when-unchanged'); }); test('progress is curated / preserve-always (ADR-1769 §4 — curated-progress ratchet)', () => { @@ -417,7 +416,6 @@ describe('ADR-1769 Phase 1: property tests (RULESET.TESTS.property-based-testing 'preserve-when-unchanged', 'preserve-always', 'preserve-if-placeholder', - 'clear', ]); fc.assert( fc.property(fc.string(), (s) => { @@ -1295,12 +1293,13 @@ describe('ADR-1769 Phase 7: sync transition — body writes + #1761', () => { // ───────────────────────────────────────────────────────────────────────────── describe('ADR-1769 #1796: applyStatePreservation — table-driven post-sync consolidation', () => { - // Shared no-op deltas for tests that only exercise one field. - const untouched = { - preBodyStatus: null, postBodyStatus: null, - preBodyStoppedAt: null, postBodyStoppedAt: null, - preBodyPhaseSource: null, postBodyPhaseSource: null, - }; + // Shared no-op deltas for tests that only exercise one field. #3468 folded + // the three dedicated status/stopped_at/current_phase_name parameter pairs + // into the single bodyDeltas channel every preserve-when-unchanged row now + // uses — `neutralBodyDeltas()` (defined below, hoisted) wires every such + // row to an "unchanged this write" delta so a test exercising ONE field + // (e.g. progress) never trips the §8.2 unwired-row throw for another. + const untouched = { bodyDeltas: neutralBodyDeltas() }; test('progress: restores curated block when table=preserve-always and transition is not re-deriving (!resync)', () => { // Default behavior: wholesale curated restore. #3242 Bug A protection. @@ -1426,9 +1425,7 @@ describe('ADR-1769 #1796: applyStatePreservation — table-driven post-sync cons preFmSnapshot: { status: 'completed' }, postFm: { status: 'verifying' }, resync: true, - preBodyStatus: 'Executing Phase 3', postBodyStatus: 'Executing Phase 3', - preBodyStoppedAt: null, postBodyStoppedAt: null, - preBodyPhaseSource: null, postBodyPhaseSource: null, + bodyDeltas: { ...neutralBodyDeltas(), status: { pre: 'Executing Phase 3', post: 'Executing Phase 3' } }, }); assert.equal(r.postFm.status, 'completed'); assert.equal(r.mutated, true); @@ -1440,23 +1437,19 @@ describe('ADR-1769 #1796: applyStatePreservation — table-driven post-sync cons preFmSnapshot: { status: 'completed' }, postFm: { status: 'verifying' }, resync: true, - preBodyStatus: 'Executing Phase 3', postBodyStatus: 'Completed Phase 3', // changed - preBodyStoppedAt: null, postBodyStoppedAt: null, - preBodyPhaseSource: null, postBodyPhaseSource: null, + bodyDeltas: { ...neutralBodyDeltas(), status: { pre: 'Executing Phase 3', post: 'Completed Phase 3' } }, // changed }); assert.equal(r.postFm.status, 'verifying'); assert.equal(r.mutated, false); }); - test('current_phase_name: preserves curated value when body Phase source unchanged (preserve-always)', () => { + test('current_phase_name: preserves curated value when body Phase source unchanged (preserve-when-unchanged, #3468 reclassified)', () => { const r = applyStatePreservation({ preFm: null, preFmSnapshot: { current_phase_name: 'curated-name' }, postFm: { current_phase_name: 'wrong-parenthetical-harvest' }, resync: true, - preBodyStatus: null, postBodyStatus: null, - preBodyStoppedAt: null, postBodyStoppedAt: null, - preBodyPhaseSource: '3', postBodyPhaseSource: '3', + bodyDeltas: { ...neutralBodyDeltas(), current_phase_name: { pre: '3', post: '3' } }, }); assert.equal(r.postFm.current_phase_name, 'curated-name'); assert.equal(r.mutated, true); @@ -1480,7 +1473,7 @@ describe('ADR-1769 #1796: applyStatePreservation — table-driven post-sync cons // #3258: every FIELD_CLASSIFICATION row declaring a preservation policy must be // honored by applyStatePreservation (the table-consuming pass). The table's own // docstring promises "a policy change is a one-row table edit" — this invariant -// proves it: for every non-`derive`/non-`clear` row, a minimal input where the +// proves it: for every non-`derive` row, a minimal input where the // declared policy would restore the snapshot value DOES restore it. Adding a // new preservation row without an implementation branch makes this fail. // @@ -1492,23 +1485,23 @@ describe('ADR-1769 #1796: applyStatePreservation — table-driven post-sync cons // ───────────────────────────────────────────────────────────────────────────── describe('#3258: applyStatePreservation honors every declared preservation row', () => { - const GOOD_PLACEHOLDER = 'preserved-by-table'; // Universal "body source unchanged this write" deltas. Every // preserve-when-unchanged probe reuses these so the delta condition (pre === // post) is satisfied and the only variable is whether the branch exists. + // #3468: current_phase_name folded into this same bodyDeltas channel + // (reclassified from preserve-always to preserve-when-unchanged) — every + // preserve-when-unchanged row must be present here or the §8.2 throw fires + // for whichever row a probe does not itself supply. const SAME = { pre: 'unchanged-source', post: 'unchanged-source' }; const unchangedBodyDeltas = { status: SAME, stopped_at: SAME, + current_phase_name: SAME, paused_at: SAME, current_phase: SAME, current_plan: SAME, last_activity_desc: SAME, }; - // Dedicated-channel deltas (the four pre-#3258 rows use these, unchanged). - const sameStatus = { preBodyStatus: 'x', postBodyStatus: 'x' }; - const sameStoppedAt = { preBodyStoppedAt: 'x', postBodyStoppedAt: 'x' }; - const samePhaseSource = { preBodyPhaseSource: 'x', postBodyPhaseSource: 'x' }; // Per-policy probe. Returns whether applyStatePreservation restored the // field's snapshot value under an input crafted so the declared policy fires. @@ -1516,36 +1509,27 @@ describe('#3258: applyStatePreservation honors every declared preservation row', const cls = getFieldClassification(field); if (!cls) return false; const policy = cls.preservation; - if (policy === 'derive' || policy === 'clear') return true; // not a preservation policy + if (policy === 'derive') return true; // not a preservation policy const GOOD = 'preserved-by-table'; const BAD = 'clobbered-by-derive'; if (policy === 'preserve-always') { - if (field === 'progress') { - const curated = { progress: { total_phases: 4, completed_phases: 3, percent: 75 } }; - const r = applyStatePreservation({ - preFm: curated, preFmSnapshot: curated, - postFm: { progress: { total_phases: 5, completed_phases: 0, percent: 0 } }, - resync: false, bodyDeltas: unchangedBodyDeltas, - ...sameStatus, ...sameStoppedAt, ...samePhaseSource, - }); - return JSON.stringify(r.postFm.progress) === JSON.stringify(curated.progress); - } - // current_phase_name: preserve-always, restored when body Phase source unchanged. + // Only `progress` carries this policy today (current_phase_name was + // reclassified to preserve-when-unchanged in #3468 — ADR-3408 §8.1). + const curated = { progress: { total_phases: 4, completed_phases: 3, percent: 75 } }; const r = applyStatePreservation({ - preFm: null, preFmSnapshot: { [field]: GOOD }, - postFm: { [field]: BAD }, resync: false, bodyDeltas: unchangedBodyDeltas, - ...sameStatus, ...sameStoppedAt, preBodyPhaseSource: '3', postBodyPhaseSource: '3', + preFm: curated, preFmSnapshot: curated, + postFm: { progress: { total_phases: 5, completed_phases: 0, percent: 0 } }, + resync: false, bodyDeltas: unchangedBodyDeltas, }); - return r.postFm[field] === GOOD; + return JSON.stringify(r.postFm.progress) === JSON.stringify(curated.progress); } if (policy === 'preserve-when-unchanged') { const r = applyStatePreservation({ preFm: null, preFmSnapshot: { [field]: GOOD }, postFm: { [field]: BAD }, resync: true, bodyDeltas: unchangedBodyDeltas, - ...sameStatus, ...sameStoppedAt, ...samePhaseSource, }); return r.postFm[field] === GOOD; } @@ -1559,7 +1543,6 @@ describe('#3258: applyStatePreservation honors every declared preservation row', preFmSnapshot: { milestone: GOOD, milestone_name: GOOD }, postFm: { milestone: 'derived-version', milestone_name: 'milestone' }, resync: true, bodyDeltas: unchangedBodyDeltas, - ...sameStatus, ...sameStoppedAt, ...samePhaseSource, }); return r.postFm[field] === GOOD; } @@ -1567,10 +1550,10 @@ describe('#3258: applyStatePreservation honors every declared preservation row', return false; } - test('every non-derive/non-clear preservation row is honored (the one-row-table-edit contract)', () => { + test('every non-derive preservation row is honored (the one-row-table-edit contract)', () => { const expected = []; for (const [field, cls] of Object.entries(FIELD_CLASSIFICATION)) { - if (cls.preservation !== 'derive' && cls.preservation !== 'clear') { + if (cls.preservation !== 'derive') { expected.push(field); } } @@ -1579,36 +1562,43 @@ describe('#3258: applyStatePreservation honors every declared preservation row', missing, [], `applyStatePreservation does not honor these declared preservation rows (expected every ` + - `non-derive/non-clear row to restore its snapshot value): ${JSON.stringify(missing)}. ` + + `non-derive row to restore its snapshot value): ${JSON.stringify(missing)}. ` + `Add a branch per ADR-1769 §4 so the table is the single policy source (#3258).`, ); }); - const unchchangedChanged = { + const lastActivityDescChangedDeltas = { status: { pre: 'x', post: 'x' }, stopped_at: { pre: 'x', post: 'x' }, + current_phase_name: { pre: 'x', post: 'x' }, paused_at: { pre: 'x', post: 'x' }, current_phase: { pre: 'x', post: 'x' }, current_plan: { pre: 'x', post: 'x' }, last_activity_desc: { pre: 'old description', post: 'new description from transition' }, // changed }; - test('a row declaring a preservation policy with no implementation would be caught (sentinel)', () => { - // Proves the invariant above actually catches a missing implementation: a - // field with a preservation policy but no body-delta wiring / branch does - // not get restored. We simulate this by probing a field whose body-source - // delta the caller forgot to supply (the realistic regression shape). - const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { current_plan: GOOD_PLACEHOLDER }, - postFm: { current_plan: 'derived' }, - resync: true, - bodyDeltas: {}, // caller forgot to wire current_plan's body-source delta - ...sameStatus, ...sameStoppedAt, ...samePhaseSource, - }); - assert.notEqual(r.postFm.current_plan, GOOD_PLACEHOLDER, - 'a preserve-when-unchanged row with no body-source delta wiring must NOT be restored ' + - '(proves the invariant would catch a new unimplemented row)'); + test('a preserve-when-unchanged row with no wired bodyDeltas entry THROWS (ADR-3408 §8.2 — #3468 tightened from the pre-#3468 silent skip)', () => { + // Pre-#3468 this row was a "sentinel" proving a missing implementation was + // merely NOT restored (a silent `continue`). ADR-3408 §8.2 requires the + // stronger invariant: an internal invariant violation — a declared row + // the caller forgot to wire via bodyDeltas — THROWS with a structured + // error, never a silent no-op indistinguishable from a correct skip. + assert.throws( + () => applyStatePreservation({ + preFm: null, + preFmSnapshot: { current_plan: 'preserved-by-table' }, + postFm: { current_plan: 'derived' }, + resync: true, + bodyDeltas: {}, // caller forgot to wire current_plan's body-source delta + }), + (err) => { + assert.strictEqual(err.code, 'STATE_PRESERVATION_UNWIRED_ROW'); + assert.strictEqual(err.field, 'current_phase', + 'current_phase is the first preserve-when-unchanged field in FIELD_CLASSIFICATION\'s ' + + 'iteration order, so it is the field named by the throw when bodyDeltas is empty'); + return true; + }, + ); }); // Per-field restore tests (clearer failure messages than the set-equality @@ -1621,7 +1611,6 @@ describe('#3258: applyStatePreservation honors every declared preservation row', postFm: { last_activity_desc: 'stale derived description' }, resync: true, bodyDeltas: { ...unchangedBodyDeltas }, - ...sameStatus, ...sameStoppedAt, ...samePhaseSource, }); assert.equal(r.postFm.last_activity_desc, 'authoritative description'); assert.equal(r.mutated, true); @@ -1633,8 +1622,7 @@ describe('#3258: applyStatePreservation honors every declared preservation row', preFmSnapshot: { last_activity_desc: 'old description' }, postFm: { last_activity_desc: 'new description from transition' }, resync: true, - bodyDeltas: { ...unchchangedChanged }, // body 'Last Activity Description' moved - ...sameStatus, ...sameStoppedAt, ...samePhaseSource, + bodyDeltas: { ...lastActivityDescChangedDeltas }, // body 'Last Activity Description' moved }); assert.equal(r.postFm.last_activity_desc, 'new description from transition'); assert.equal(r.mutated, false); @@ -1650,7 +1638,6 @@ describe('#3258: applyStatePreservation honors every declared preservation row', postFm: { paused_at: '2026-01-01' }, resync: true, bodyDeltas: { ...unchangedBodyDeltas }, - ...sameStatus, ...sameStoppedAt, ...samePhaseSource, }); assert.equal(r.postFm.paused_at, '2026-02-02'); assert.equal(r.mutated, true); @@ -1663,7 +1650,6 @@ describe('#3258: applyStatePreservation honors every declared preservation row', postFm: { current_phase: '2' }, resync: true, bodyDeltas: { ...unchangedBodyDeltas }, - ...sameStatus, ...sameStoppedAt, ...samePhaseSource, }); assert.equal(r.postFm.current_phase, '4'); assert.equal(r.mutated, true); @@ -1676,7 +1662,6 @@ describe('#3258: applyStatePreservation honors every declared preservation row', postFm: { current_plan: '3' }, resync: true, bodyDeltas: { ...unchangedBodyDeltas }, - ...sameStatus, ...sameStoppedAt, ...samePhaseSource, }); assert.equal(r.postFm.current_plan, '5'); assert.equal(r.mutated, true); @@ -1689,7 +1674,6 @@ describe('#3258: applyStatePreservation honors every declared preservation row', postFm: { milestone: '0.x', milestone_name: 'milestone' }, // placeholder derive resync: true, bodyDeltas: { ...unchangedBodyDeltas }, - ...sameStatus, ...sameStoppedAt, ...samePhaseSource, }); assert.equal(r.postFm.milestone_name, 'Real Curated Name', 'placeholder-derived milestone_name must yield to the curated snapshot (#948/#2135 contract)'); @@ -1698,6 +1682,612 @@ describe('#3258: applyStatePreservation honors every declared preservation row', }); }); +// ───────────────────────────────────────────────────────────────────────────── +// #3468 (ADR-3408, Phase 1 test matrix 50-test-matrix.md, sections A/B/C): +// policy dispatch, the §8.2 unenforced-row invariant, and behavior identity +// across the refactor. Failing-first: several rows below are EXPECTED to +// fail against the current tree until the refactor lands (see each test's +// comment for which). Everything else characterizes behavior the refactor +// must preserve byte-for-byte. +// ───────────────────────────────────────────────────────────────────────────── + +// Wires every currently-declared preserve-when-unchanged field with a neutral +// "unchanged this write" delta, so a test probing ONE field never trips over +// another field being unwired — forward-compatible with both today's tree +// (where an unwired row silently `continue`s) and the post-refactor tree +// (where an unwired row throws per §8.2 / matrix row B1). +function neutralBodyDeltas() { + const deltas = {}; + for (const [field, cls] of Object.entries(FIELD_CLASSIFICATION)) { + if (cls.preservation === 'preserve-when-unchanged') { + deltas[field] = { pre: 'unchanged-source', post: 'unchanged-source' }; + } + } + return deltas; +} + +// #3468 folded status/stopped_at/current_phase_name's three dedicated +// pre/post parameter pairs into the single bodyDeltas channel and deleted +// them from StatePreservationInput — applyStatePreservation no longer reads +// these properties at all. Kept (rather than stripped from every call site +// below) as an inert, harmless spread: StatePreservationInput is a TypeScript +// type these plain-JS tests are not checked against, so an extra own +// property is silently ignored at runtime, and removing it from ~20 call +// sites would be pure churn with no behavior change. neutralBodyDeltas() +// already supplies the real (bodyDeltas-based) no-op deltas these fields +// need post-refactor. +const dedicatedNoop = { + preBodyStatus: 'x', postBodyStatus: 'x', + preBodyStoppedAt: 'x', postBodyStoppedAt: 'x', + preBodyPhaseSource: 'x', postBodyPhaseSource: 'x', +}; + +describe('#3468 matrix A: executor policy dispatch (ADR-3408 §8.1) — new/boundary/hostile rows', () => { + test('A3: preserve-when-unchanged — an empty-string snapshot is not restored', () => { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { current_plan: '' }, + postFm: { current_plan: 'derived' }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + ...dedicatedNoop, + }); + assert.equal(r.postFm.current_plan, 'derived'); + assert.equal(r.mutated, false); + }); + + // A4 (matrix: "likely gap today"): a whitespace-only snapshot passes the + // current `.length > 0` check (3 > 0) and DOES get restored today. The + // required behavior is skip. This is expected to FAIL against the current + // tree until the refactor tightens the guard. + test('A4: preserve-when-unchanged — a whitespace-only snapshot is not restored (".length > 0" is not enough)', () => { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { current_plan: ' ' }, + postFm: { current_plan: 'derived' }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + ...dedicatedNoop, + }); + assert.equal(r.postFm.current_plan, 'derived', + 'a whitespace-only snapshot must not be treated as a real curated value'); + assert.equal(r.mutated, false); + }); + + test('A5: preserve-when-unchanged — a non-string snapshot is ignored (no throw)', () => { + for (const snapshot of [42, true, null, { nested: 1 }, undefined]) { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { current_plan: snapshot }, + postFm: { current_plan: 'derived' }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + ...dedicatedNoop, + }); + assert.equal(r.postFm.current_plan, 'derived', `snapshot=${JSON.stringify(snapshot)} must not be restored`); + } + }); + + test('A6: preserve-when-unchanged — no-op when postFm already equals the snapshot', () => { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { current_plan: 'same-value' }, + postFm: { current_plan: 'same-value' }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + ...dedicatedNoop, + }); + assert.equal(r.postFm.current_plan, 'same-value'); + assert.equal(r.mutated, false); + }); + + test('A7: preserve-when-unchanged — a postFm missing the key entirely is restored (undefined !== snapshot)', () => { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { current_plan: 'curated' }, + postFm: {}, // key absent entirely + resync: true, + bodyDeltas: neutralBodyDeltas(), + ...dedicatedNoop, + }); + assert.equal(r.postFm.current_plan, 'curated'); + assert.equal(r.mutated, true); + }); + + test('A8: status — the "unknown" sentinel snapshot is never restored', () => { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { status: 'unknown' }, + postFm: { status: 'verifying' }, + resync: true, + preBodyStatus: 'x', postBodyStatus: 'x', + preBodyStoppedAt: 'x', postBodyStoppedAt: 'x', + preBodyPhaseSource: 'x', postBodyPhaseSource: 'x', + bodyDeltas: neutralBodyDeltas(), + }); + assert.equal(r.postFm.status, 'verifying'); + assert.equal(r.mutated, false); + }); + + test('A9: status — the "unknown" sentinel guard is case-sensitive ("Unknown" is a real value, restored)', () => { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { status: 'Unknown' }, + postFm: { status: 'verifying' }, + resync: true, + preBodyStatus: 'x', postBodyStatus: 'x', + preBodyStoppedAt: 'x', postBodyStoppedAt: 'x', + preBodyPhaseSource: 'x', postBodyPhaseSource: 'x', + bodyDeltas: neutralBodyDeltas(), + }); + assert.equal(r.postFm.status, 'Unknown', + 'the sentinel is an exact-match on the lowercase literal "unknown" — a case variant is a real value'); + assert.equal(r.mutated, true); + }); + + test('A12: preserve-always progress — preFm===null under !resync does not throw (skip)', () => { + assert.doesNotThrow(() => { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: {}, + postFm: { progress: { total_phases: 5, completed_phases: 1, percent: 20 } }, + resync: false, + bodyDeltas: neutralBodyDeltas(), + ...dedicatedNoop, + }); + assert.deepEqual(r.postFm.progress, { total_phases: 5, completed_phases: 1, percent: 20 }); + assert.equal(r.mutated, false); + }); + }); + + test('A14: deriveProgressKeys — derived completed_plans === curated keeps curated (limit: ">" not ">=", #2969)', () => { + const curated = { progress: { total_plans: 10, completed_plans: 7, percent: 70 } }; + const r = applyStatePreservation({ + preFm: curated, + preFmSnapshot: curated, + postFm: { progress: { total_plans: 10, completed_plans: 7, percent: 70 } }, // derived === curated + resync: false, + deriveProgressKeys: true, + bodyDeltas: neutralBodyDeltas(), + ...dedicatedNoop, + }); + assert.equal(r.postFm.progress.completed_plans, 7, + 'equal counts (derived === curated) must keep curated — the ratchet is ">" not ">="'); + }); + + test('A18: preserve-if-placeholder — punctuation-led derived names are rejected for every delimiter', () => { + for (const derived of ['— Foo', ': Foo', '-Foo']) { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { milestone: '1.0', milestone_name: 'Real Curated Name' }, + postFm: { milestone: '1.1', milestone_name: derived }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + ...dedicatedNoop, + }); + assert.equal(r.postFm.milestone_name, 'Real Curated Name', + `punctuation-led derived name ${JSON.stringify(derived)} must be rejected`); + } + }); + + test('A19: preserve-if-placeholder — an empty-string derived name is rejected (restored)', () => { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { milestone: '1.0', milestone_name: 'Real Curated Name' }, + postFm: { milestone: '1.1', milestone_name: '' }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + ...dedicatedNoop, + }); + assert.equal(r.postFm.milestone_name, 'Real Curated Name'); + }); + + test('A20: preserve-if-placeholder — a placeholder snapshot is not restored over a placeholder derived value', () => { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { milestone: '1.0', milestone_name: 'milestone' }, // snapshot IS the placeholder + postFm: { milestone: '1.1', milestone_name: 'milestone' }, // derived is also the placeholder + resync: true, + bodyDeltas: neutralBodyDeltas(), + ...dedicatedNoop, + }); + assert.equal(r.postFm.milestone_name, 'milestone', 'nothing better to restore — value passes through unchanged'); + assert.equal(r.postFm.milestone, '1.1', 'milestone version passes through unchanged alongside it'); + }); + + test('A21: preserve-if-placeholder — a real, different derived name wins over the curated snapshot', () => { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { milestone: '1.0', milestone_name: 'Old Curated Name' }, + postFm: { milestone: '2.0', milestone_name: 'New Real Milestone Name' }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + ...dedicatedNoop, + }); + assert.equal(r.postFm.milestone_name, 'New Real Milestone Name'); + assert.equal(r.postFm.milestone, '2.0'); + }); + + // A22 (design 40-design.md row 17 / matrix "new — row 17"): a `derive` row + // must be an explicit no-op — untouched, no throw, mutated unaffected. + // NOTE: as a black-box behavioral probe against the public + // applyStatePreservation API (the only seam this test file can drive), + // this currently ALREADY PASSES — today's loop filters every field to + // `cls.preservation === 'preserve-when-unchanged'` before doing anything, + // so a `derive` row is skipped by omission rather than by an explicit + // branch. The matrix's "no branch exists" is a source-structure claim + // (§8.1's "exactly one executor per policy") that this black-box test + // cannot distinguish from "skipped by omission" — see report. + test('A22: derive rows (last_updated, state_head, gsd_state_version, last_activity) are an explicit no-op', () => { + for (const field of ['last_updated', 'state_head', 'gsd_state_version', 'last_activity']) { + assert.doesNotThrow(() => { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { [field]: 'curated-value' }, + postFm: { [field]: 'freshly-derived-value' }, + resync: true, + bodyDeltas: { ...neutralBodyDeltas(), [field]: { pre: 'old', post: 'new' } }, + ...dedicatedNoop, + }); + assert.equal(r.postFm[field], 'freshly-derived-value', + `${field}: a derive row always takes the freshly-derived (postFm) value`); + }); + } + }); + + test('A23: a field with no FIELD_CLASSIFICATION row passes through untouched', () => { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { totally_unclassified_field: 'curated' }, + postFm: { totally_unclassified_field: 'derived' }, + resync: true, + bodyDeltas: { ...neutralBodyDeltas(), totally_unclassified_field: { pre: 'x', post: 'y' } }, + ...dedicatedNoop, + }); + assert.equal(r.postFm.totally_unclassified_field, 'derived'); + assert.equal(r.mutated, false); + }); + + test('A24: prototype-pollution — __proto__ / constructor / toString never resolve to inherited classifications', () => { + for (const hostileKey of ['__proto__', 'constructor', 'toString']) { + assert.strictEqual(getFieldClassification(hostileKey), null, + `${hostileKey} must not resolve to an inherited Object member`); + } + // applyStatePreservation itself must not crash or misbehave when a + // hostile field name rides along in postFm/preFmSnapshot/bodyDeltas. + // Computed keys (not literal `__proto__:`) create genuine OWN properties + // instead of mutating the object's prototype. + assert.doesNotThrow(() => { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { ['__proto__']: 'x', ['constructor']: 'y', ['toString']: 'z' }, + postFm: { ['__proto__']: 'a', ['constructor']: 'b', ['toString']: 'c' }, + resync: true, + bodyDeltas: { + ...neutralBodyDeltas(), + ['__proto__']: { pre: 'x', post: 'y' }, + ['constructor']: { pre: 'x', post: 'y' }, + ['toString']: { pre: 'x', post: 'y' }, + }, + ...dedicatedNoop, + }); + assert.equal(typeof r.postFm, 'object'); + assert.equal(typeof r.mutated, 'boolean'); + }); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// #3468 matrix B: the §8.2 unenforced-row invariant. +// +// Structured error shape THIS TASK REQUIRES THE IMPLEMENTATION TO PROVIDE +// (CONTRIBUTING.md § Prohibited: Raw Text Matching — the thrown error must +// carry typed properties, never asserted via message-prose matching): +// err.code === 'STATE_PRESERVATION_UNWIRED_ROW' +// err.field === '' +// +// B1-B3 are EXPECTED TO FAIL against the current tree: today's loop at +// src/state-transition.cts:314 does `if (!delta) continue;` — a silent skip, +// not a throw. This is the exact defect §8.2 requires fixing. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3468 matrix B: an unenforced preserve-when-unchanged row (ADR-3408 §8.2)', () => { + test('B1: a declared preserve-when-unchanged row missing from bodyDeltas throws, naming the field', () => { + const bodyDeltas = neutralBodyDeltas(); + delete bodyDeltas.current_plan; // the ONLY unwired row + assert.throws( + () => applyStatePreservation({ + preFm: null, + preFmSnapshot: { current_plan: 'curated' }, + postFm: { current_plan: 'derived' }, + resync: true, + bodyDeltas, + ...dedicatedNoop, + }), + (err) => { + assert.strictEqual(err.code, 'STATE_PRESERVATION_UNWIRED_ROW'); + assert.strictEqual(err.field, 'current_plan'); + return true; + }, + ); + }); + + test('B2: bodyDeltas entirely absent throws, naming the first unwired row', () => { + assert.throws( + () => applyStatePreservation({ + preFm: null, + preFmSnapshot: {}, + postFm: {}, + resync: true, + // bodyDeltas omitted entirely + ...dedicatedNoop, + }), + (err) => { + assert.strictEqual(err.code, 'STATE_PRESERVATION_UNWIRED_ROW'); + // current_phase is the first preserve-when-unchanged field in + // FIELD_CLASSIFICATION's insertion order — before current_phase_name, + // current_plan, status, stopped_at, paused_at, last_activity_desc, + // all of which share the same loop post-#3468 (no field stays on a + // dedicated channel any more). + assert.strictEqual(err.field, 'current_phase'); + return true; + }, + ); + }); + + test('B3: bodyDeltas present but {} throws, naming the first unwired row', () => { + assert.throws( + () => applyStatePreservation({ + preFm: null, + preFmSnapshot: {}, + postFm: {}, + resync: true, + bodyDeltas: {}, + ...dedicatedNoop, + }), + (err) => { + assert.strictEqual(err.code, 'STATE_PRESERVATION_UNWIRED_ROW'); + assert.strictEqual(err.field, 'current_phase'); + return true; + }, + ); + }); + + test('B4: a delta shaped {pre:null,post:null} is treated as WIRED, not missing (the boundary that separates B1 from A1)', () => { + const bodyDeltas = { + ...neutralBodyDeltas(), + current_phase: { pre: null, post: null }, + }; + assert.doesNotThrow(() => { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: {}, // no snapshot — skip is a legitimate, non-throwing outcome + postFm: { current_phase: 'derived' }, + resync: true, + bodyDeltas, + ...dedicatedNoop, + }); + assert.equal(r.postFm.current_phase, 'derived'); + }); + }); + + test('B5: a partially-shaped delta ({post} with no pre key) does not throw — treated as wired', () => { + const bodyDeltas = { + ...neutralBodyDeltas(), + current_phase: { post: 'x' }, // `pre` key entirely absent + }; + assert.doesNotThrow(() => { + applyStatePreservation({ + preFm: null, + preFmSnapshot: { current_phase: 'curated' }, + postFm: { current_phase: 'derived' }, + resync: true, + bodyDeltas, + ...dedicatedNoop, + }); + }); + }); + + test('B6: only preserve-when-unchanged rows require wiring — derive/preserve-always/preserve-if-placeholder do not', () => { + // Every preserve-when-unchanged row IS wired here; last_updated (derive), + // progress (preserve-always), and milestone_name (preserve-if-placeholder) + // are deliberately absent from bodyDeltas and must not trigger the throw. + assert.doesNotThrow(() => { + applyStatePreservation({ + preFm: null, + preFmSnapshot: {}, + postFm: {}, + resync: true, + bodyDeltas: neutralBodyDeltas(), + ...dedicatedNoop, + }); + }); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// #3468 matrix C: behavior identity across the refactor (ADR-3408 §8.1). +// +// Literal, pinned expected values — not derived from a shared helper — per +// 40-design.md's "Known limits": the reclassification of current_phase_name +// is behavior-preserving by design, so the existing suite staying green +// proves nothing; only a pinned before/after comparison catches a silent +// drift (CONTRIBUTING.md § Fixture provenance #2371). +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3468 matrix C1/C2: identity across the refactor — pinned literal outputs', () => { + const GOOD = 'preserved-by-table'; + const BAD = 'clobbered-by-derive'; + + // Four of the seven preserve-when-unchanged rows, pinned generically here; + // status/stopped_at get their own pinned test above (their 'unknown' + // sentinel guard has no analogue in these four), and current_phase_name + // gets its own dedicated C2 test below (it needs BOTH an unchanged AND a + // changed delta for the same field, which this generic loop cannot express). + const LOOP_PWU_FIELDS = ['current_phase', 'current_plan', 'paused_at', 'last_activity_desc']; + + test('C1: shared preserve-when-unchanged loop — delta unchanged restores the snapshot (pinned)', () => { + for (const field of LOOP_PWU_FIELDS) { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { [field]: GOOD }, + postFm: { [field]: BAD }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + ...dedicatedNoop, + }); + assert.equal(r.postFm[field], GOOD, `${field}: pinned identity — restore-when-unchanged`); + } + }); + + test('C1: shared preserve-when-unchanged loop — delta changed lets derived win (pinned)', () => { + for (const field of LOOP_PWU_FIELDS) { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { [field]: GOOD }, + postFm: { [field]: BAD }, + resync: true, + bodyDeltas: { ...neutralBodyDeltas(), [field]: { pre: 'old', post: 'new' } }, + ...dedicatedNoop, + }); + assert.equal(r.postFm[field], BAD, `${field}: pinned identity — derived wins when body source changed`); + } + }); + + test('C1: status / stopped_at (pinned)', () => { + const rStatus = applyStatePreservation({ + preFm: null, preFmSnapshot: { status: GOOD }, postFm: { status: BAD }, resync: true, + bodyDeltas: neutralBodyDeltas(), + }); + assert.equal(rStatus.postFm.status, GOOD); + + const rStopped = applyStatePreservation({ + preFm: null, preFmSnapshot: { stopped_at: GOOD }, postFm: { stopped_at: BAD }, resync: true, + bodyDeltas: neutralBodyDeltas(), + }); + assert.equal(rStopped.postFm.stopped_at, GOOD); + }); + + test('C1: preserve-always progress and preserve-if-placeholder milestone/milestone_name (pinned)', () => { + const curated = { progress: { total_phases: 4, completed_phases: 3, percent: 75 } }; + const rProgress = applyStatePreservation({ + preFm: curated, preFmSnapshot: curated, + postFm: { progress: { total_phases: 5, completed_phases: 0, percent: 0 } }, + resync: false, bodyDeltas: neutralBodyDeltas(), ...dedicatedNoop, + }); + assert.deepEqual(rProgress.postFm.progress, curated.progress); + + const rMilestone = applyStatePreservation({ + preFm: null, + preFmSnapshot: { milestone: '1.0', milestone_name: GOOD }, + postFm: { milestone: '1.1', milestone_name: 'milestone' }, + resync: true, bodyDeltas: neutralBodyDeltas(), ...dedicatedNoop, + }); + assert.equal(rMilestone.postFm.milestone_name, GOOD); + }); + + test('C1: derive-classified fields pass through untouched regardless of snapshot (pinned)', () => { + for (const field of ['gsd_state_version', 'last_updated', 'last_activity', 'state_head']) { + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot: { [field]: GOOD }, + postFm: { [field]: BAD }, + resync: true, + bodyDeltas: { ...neutralBodyDeltas(), [field]: { pre: 'old', post: 'new' } }, + ...dedicatedNoop, + }); + assert.equal(r.postFm[field], BAD, `${field}: derive rows always take the derived (postFm) value`); + } + }); + + // C2: current_phase_name's row is reclassified preserve-always → + // preserve-when-unchanged (#3468, ADR-3408 §8.1 amendment) as a + // BEHAVIOR-PRESERVING change (40-design.md). Both outcomes are pinned + // literally so a post-refactor drift is caught even though the existing + // suite staying green would prove nothing. The delta now travels through + // bodyDeltas.current_phase_name (folded from the pre-#3468 dedicated + // preBodyPhaseSource/postBodyPhaseSource parameter pair, which #3468 + // deleted from StatePreservationInput) rather than through + // neutralBodyDeltas()'s generic "unchanged" default, since this test needs + // to drive both an unchanged AND a changed delta for the SAME field. + test('C2: current_phase_name (reclassified preserve-always → preserve-when-unchanged) — pinned outputs', () => { + const rEqual = applyStatePreservation({ + preFm: null, + preFmSnapshot: { current_phase_name: GOOD }, + postFm: { current_phase_name: BAD }, + resync: true, + bodyDeltas: { ...neutralBodyDeltas(), current_phase_name: { pre: '3', post: '3' } }, // body Phase: source unchanged this write + }); + assert.equal(rEqual.postFm.current_phase_name, GOOD, + 'reclassification must not change this: unchanged body Phase source still restores the curated name'); + assert.equal(rEqual.mutated, true); + + const rDiffer = applyStatePreservation({ + preFm: null, + preFmSnapshot: { current_phase_name: GOOD }, + postFm: { current_phase_name: BAD }, + resync: true, + bodyDeltas: { ...neutralBodyDeltas(), current_phase_name: { pre: '3', post: '4' } }, // body Phase: source changed this write + }); + assert.equal(rDiffer.postFm.current_phase_name, BAD, + 'reclassification must not change this: changed body Phase source still lets derived win'); + assert.equal(rDiffer.mutated, false); + }); +}); + +describe('#3468 matrix C3: executor dispatch is a pure function of the row policy (property)', () => { + // Every preserve-when-unchanged field EXCEPT status/stopped_at — excluded + // because status carries the 'unknown' sentinel guard (a field-specific + // exception this generic property does not model) and stopped_at is its + // paired dedicated-channel sibling in the pre-#3468 design this comment + // originally described; both get their own pinned coverage in C1 above. + const LOOP_PWU_FIELDS = Object.keys(FIELD_CLASSIFICATION).filter((f) => { + const cls = getFieldClassification(f); + return cls !== null && cls.preservation === 'preserve-when-unchanged' && f !== 'status' && f !== 'stopped_at'; + }); + + test('property: restore fires iff (wired AND non-empty snapshot AND unchanged delta AND postFm differs)', () => { + fc.assert( + fc.property( + fc.constantFrom(...LOOP_PWU_FIELDS), + fc.string({ maxLength: 20 }), + fc.boolean(), + fc.string({ maxLength: 20 }), + fc.boolean(), + fc.boolean(), + (field, snapshotValue, snapshotPresent, postFmValue, deltaChanged, resync) => { + const preFmSnapshot = snapshotPresent ? { [field]: snapshotValue } : {}; + const postFm = { [field]: postFmValue }; + const delta = { pre: 'source', post: deltaChanged ? 'source-changed' : 'source' }; + const r = applyStatePreservation({ + preFm: null, + preFmSnapshot, + postFm, + resync, + bodyDeltas: { ...neutralBodyDeltas(), [field]: delta }, + ...dedicatedNoop, + }); + + const snapshotUsable = snapshotPresent && snapshotValue.length > 0; + const alreadyCorrect = postFmValue === snapshotValue; + const expectRestore = snapshotUsable && !deltaChanged && !alreadyCorrect; + + const expectedValue = expectRestore ? snapshotValue : postFmValue; + if (r.postFm[field] !== expectedValue || r.mutated !== expectRestore) { + throw new Error( + `dispatch mismatch: field=${field} snapshotPresent=${snapshotPresent} ` + + `snapshotValue=${JSON.stringify(snapshotValue)} postFmValue=${JSON.stringify(postFmValue)} ` + + `deltaChanged=${deltaChanged} resync=${resync} expectRestore=${expectRestore} ` + + `got postFm[field]=${JSON.stringify(r.postFm[field])} mutated=${r.mutated}`, + ); + } + return true; + }, + ), + { seed: 3468, numRuns: 200 }, + ); + }); +}); // ──────────────────────────────────────────────────────────────────────── // Folded from tests/bug-21-state-md-template-frontmatter.test.cjs — consolidation epic #1969 (B8 #1977) diff --git a/tests/state-write-path-drift-guard.test.cjs b/tests/state-write-path-drift-guard.test.cjs new file mode 100644 index 000000000..e522f2fd2 --- /dev/null +++ b/tests/state-write-path-drift-guard.test.cjs @@ -0,0 +1,566 @@ +'use strict'; + +/** + * Tests for the STATE.md write-path anti-divergence drift guard + * (epic #3408, issue #3468, ADR-3408 Decision 5) — + * `scripts/lint-state-write-path-drift.cjs`. + * + * Design contract: docs/adr/3408-state-write-path-preservation.md (§8.1/§8.2/§8.3) + * Test matrix: .gsd/phase/refactor-3468-table-driven-preservation/50-test-matrix.md + * (section D, rows D1-D13 — this file covers section D only) + * + * Every row except D1 and D13 drives the guard's exported PURE functions + * (`findSeamBypasses`, `findPromptSeamUses`, `applyRatchet`, `loadBaseline`) + * directly with in-memory fixtures — no temp tree is needed, mirroring + * tests/state-field-drift.test.cjs's own house pattern for this class of + * guard. `REPO_ROOT` inside the guard module is a constant resolved from + * `__dirname` at require time, so it cannot be pointed at a synthetic tree + * without changing the guard's own interface — D1 (the real-tree contract) + * is therefore driven through the CLI's `--json` output instead, and D13 + * (an unreadable file) through an `fs.readFileSync` monkeypatch rather than + * a real synthetic tree. + * + * Fixtures use array `.join('\n')`, never an indented template literal — + * indentation bleed would shift every asserted line number. Per D2/D5/D7's + * matrix note ("guard fixtures come from outside the guard's own writer"), + * the write-seam call lines reused below are copied VERBATIM from real, + * pre-existing production call sites (the guard's own current baseline + * entries) rather than invented by this test file: + * - `writeStateMd(statePath, modified, cwd);` src/state.cts:3682 + * - `writeStateMd(statePath, result.content, cwd);` src/milestone.cts:865 + * - `writeStateMd(statePath, stateContent, cwd);` src/health-diagnostic.cts:337 + * + * Assertions compare the frozen `REASON` enum values and the `--json`/pure + * function return shapes only — never a substring/regex match on the human + * formatter's prose (CONTRIBUTING.md, "Prohibited: Raw Text Matching on Test + * Outputs"). + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const { runNode } = require('./helpers/process-seam.cjs'); +const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); + +const guard = require('../scripts/lint-state-write-path-drift.cjs'); +const { + REASON, + findSeamBypasses, + findPromptSeamUses, + findPolicyDispatchDrift, + applyRatchet, + loadBaseline, + buildBaselineEntries, + SEAM_OWNER_FILE, + SEAM_OWNER_EXEMPT_FUNCTIONS, + EXECUTOR_FILE, + REPO_ROOT, + BASELINE_PATH, +} = guard; + +const GUARD_PATH = path.join(REPO_ROOT, 'scripts', 'lint-state-write-path-drift.cjs'); + +// A synthetic, non-owner, non-executor consumer file — never a real repo +// path — used as the `rel` argument wherever a row does not specifically +// need EXECUTOR_FILE or SEAM_OWNER_FILE behavior. +const OTHER_FILE = 'src/example-consumer.cts'; +const OTHER_FILE_2 = 'src/example-consumer-2.cts'; +const OTHER_FILE_3 = 'src/example-consumer-3.cts'; + +// ─── D1: the real tree, through the CLI's --json contract ───────────────── + +describe('D1 — the real tree', () => { + test('guard: clean tree passes', () => { + // Expected RED until the sibling refactor of src/state-transition.cts + // (issue #3468 Phase 1, concurrent with this test file's own authorship) + // lands: at write time the executor still dispatches five fields by + // literal name and leaves `derive`/`clear` unimplemented, which this + // guard's policy-dispatch axis correctly reports as 7 findings. Mirrors + // tests/milestone-window-drift-guard.test.cjs's own precedent of an + // explicitly-documented real-tree row that is red until its companion + // consolidation lands. + const result = runNode([GUARD_PATH, '--json'], { cwd: REPO_ROOT, timeoutMs: PROBE_TIMEOUT_MS }); + assert.strictEqual(result.outcome, 'exited'); + const body = JSON.parse(result.stdout); + assert.strictEqual(body.ok, true); + assert.deepStrictEqual(body.findings, []); + assert.strictEqual(result.exitCode, 0); + }); +}); + +// ─── D2: the guard MUST be able to fail ──────────────────────────────────── + +describe('D2 — an unrecorded bypass fails', () => { + test('guard: an unrecorded bypass fails', () => { + const text = [ + 'function cmdSomethingElse(cwd) {', + ' const modified = deriveModifiedContent();', + ' writeStateMd(statePath, modified, cwd);', + '}', + ].join('\n'); + + const observed = findSeamBypasses(OTHER_FILE, text); + assert.strictEqual(observed.length, 1); + assert.strictEqual(observed[0].line, 3); + + const findings = applyRatchet(observed, { entries: [] }); + // A guard that cannot fail is worse than no guard: an unacknowledged + // bypass against an empty baseline MUST produce exactly one finding, + // reasoned, at the exact file and line — not merely "an array". + assert.strictEqual(findings.length, 1); + assert.strictEqual(findings[0].reason, REASON.SEAM_BYPASS_UNRECORDED); + assert.strictEqual(findings[0].file, OTHER_FILE); + assert.strictEqual(findings[0].line, 3); + assert.strictEqual(findings[0].source, 'writeStateMd(statePath, modified, cwd);'); + }); +}); + +// ─── D3: a recorded bypass is acknowledged ───────────────────────────────── + +describe('D3 — a recorded bypass is acknowledged', () => { + test('guard: a recorded bypass is acknowledged', () => { + const text = [ + 'function cmdSomethingElse(cwd) {', + ' const modified = deriveModifiedContent();', + ' writeStateMd(statePath, modified, cwd);', + '}', + ].join('\n'); + + const observed = findSeamBypasses(OTHER_FILE, text); + const baseline = { + entries: [{ file: OTHER_FILE, source: 'writeStateMd(statePath, modified, cwd);', symbol: 'writeStateMd', count: 1, owner: null }], + }; + assert.deepStrictEqual(applyRatchet(observed, baseline), []); + }); +}); + +// ─── D4: a stale acknowledgment fails ────────────────────────────────────── + +describe('D4 — a stale acknowledgment fails', () => { + test('guard: a stale acknowledgment fails', () => { + // The call site the baseline acknowledges no longer fires at all this + // scan — the acknowledgment has outlived what it describes. + const baseline = { + entries: [{ file: OTHER_FILE, source: 'writeStateMd(statePath, modified, cwd);', symbol: 'writeStateMd', count: 1, owner: null }], + }; + const findings = applyRatchet([], baseline); + assert.strictEqual(findings.length, 1); + assert.strictEqual(findings[0].reason, REASON.BASELINE_ENTRY_STALE); + assert.strictEqual(findings[0].file, OTHER_FILE); + assert.strictEqual(findings[0].observed, 0); + assert.strictEqual(findings[0].acknowledged, 1); + }); +}); + +// ─── D5/D6/D7: the occurrence-count boundary triple (limit-1/limit/limit+1) ─ +// The baseline acknowledges 2 occurrences throughout ("the limit"); only the +// OBSERVED count in the fixture source varies. This is why the ratchet keys +// entries on (file, trimmed source) instead of line number: two +// byte-identical call sites in one file are otherwise indistinguishable. + +describe('D5 — occurrence count catches partial migration (limit-1)', () => { + test('guard: occurrence count catches partial migration', () => { + const text = [ + 'function cmdSomethingElse(cwd) {', + ' writeStateMd(statePath, result.content, cwd);', + '}', + ].join('\n'); + + const observed = findSeamBypasses(OTHER_FILE_2, text); + assert.strictEqual(observed.length, 1); + + const baseline = { + entries: [{ file: OTHER_FILE_2, source: 'writeStateMd(statePath, result.content, cwd);', symbol: 'writeStateMd', count: 2, owner: null }], + }; + const findings = applyRatchet(observed, baseline); + // Only 1 of the 2 acknowledged call sites still fires — a genuine + // partial migration, not a clean removal — must fail, not silently pass. + assert.strictEqual(findings.length, 1); + assert.strictEqual(findings[0].reason, REASON.SEAM_BYPASS_COUNT_SHRANK); + assert.strictEqual(findings[0].observed, 1); + assert.strictEqual(findings[0].acknowledged, 2); + }); +}); + +describe('D6 — matching occurrence count passes (limit)', () => { + test('guard: matching occurrence count passes', () => { + const text = [ + 'function cmdSomethingElse(cwd) {', + ' writeStateMd(statePath, result.content, cwd);', + ' writeStateMd(statePath, result.content, cwd);', + '}', + ].join('\n'); + + const observed = findSeamBypasses(OTHER_FILE_2, text); + assert.strictEqual(observed.length, 2); + + const baseline = { + entries: [{ file: OTHER_FILE_2, source: 'writeStateMd(statePath, result.content, cwd);', symbol: 'writeStateMd', count: 2, owner: null }], + }; + assert.deepStrictEqual(applyRatchet(observed, baseline), []); + }); +}); + +describe('D7 — a new copy beside an acknowledged one fails (limit+1)', () => { + test('guard: a new copy beside an acknowledged one fails', () => { + const text = [ + 'function cmdA(cwd) {', + ' writeStateMd(statePath, stateContent, cwd);', + '}', + 'function cmdB(cwd) {', + ' writeStateMd(statePath, stateContent, cwd);', + '}', + 'function cmdC(cwd) {', + ' writeStateMd(statePath, stateContent, cwd);', + '}', + ].join('\n'); + + const observed = findSeamBypasses(OTHER_FILE_3, text); + assert.strictEqual(observed.length, 3); + + const baseline = { + entries: [{ file: OTHER_FILE_3, source: 'writeStateMd(statePath, stateContent, cwd);', symbol: 'writeStateMd', count: 2, owner: null }], + }; + const findings = applyRatchet(observed, baseline); + assert.strictEqual(findings.length, 1); + assert.strictEqual(findings[0].reason, REASON.SEAM_BYPASS_COUNT_GREW); + assert.strictEqual(findings[0].observed, 3); + assert.strictEqual(findings[0].acknowledged, 2); + }); +}); + +// ─── D8: comments are not drift ──────────────────────────────────────────── + +describe('D8 — comments are not drift', () => { + test('guard: comments are not drift', () => { + const text = [ + '// writeStateMd(statePath, modified, cwd);', + '/**', + ' * writeStateMd(statePath, modified, cwd);', + ' */', + 'function noop() {}', + ].join('\n'); + + // ADR-3180 Amendment 3's recorded false positive: a `//` line comment + // and a `/* */` block comment both carrying the exact call text must + // stay silent — both are blanked by `stripComments` before the seam-call + // regex ever runs. + assert.deepStrictEqual(findSeamBypasses(OTHER_FILE, text), []); + }); +}); + +// ─── D9/D10: the owner-file exemption is function-scoped, not file-scoped ── + +describe('D9 — owner functions are exempt', () => { + test('guard: owner functions are exempt', () => { + assert.ok(SEAM_OWNER_EXEMPT_FUNCTIONS.includes('readModifyWriteStateMd')); + + const text = [ + 'function readModifyWriteStateMd(cwd) {', + ' const modified = compute();', + ' writeStateMd(statePath, modified, cwd);', + ' return modified;', + '}', + ].join('\n'); + + // The seam's own internal plumbing (the I/O wrapper calling the pure + // sync stage) is not a bypass. + assert.deepStrictEqual(findSeamBypasses(SEAM_OWNER_FILE, text), []); + }); +}); + +describe('D10 — the owner file is not exempt', () => { + test('guard: the owner file is not exempt', () => { + // ADR-3408 Decision 5's named gaming route: a whole-FILE exemption on + // the owner is exactly how getMilestoneInfo stayed invisible to an + // earlier drift guard. A call inside any OTHER function in state.cts — + // not one of SEAM_OWNER_EXEMPT_FUNCTIONS — must still be caught. + const text = [ + 'function patchCore(cwd) {', + ' const modified = compute();', + ' writeStateMd(statePath, modified, cwd);', + ' return modified;', + '}', + ].join('\n'); + + const out = findSeamBypasses(SEAM_OWNER_FILE, text); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].line, 3); + }); +}); + +// ─── D11: the prompt layer is in the scan surface ────────────────────────── + +describe('D11 — the prompt layer is in the scan surface', () => { + const PROMPT_FILE = 'gsd-core/workflows/example-workflow.md'; + + test('guard: the prompt layer is in the scan surface', () => { + const text = [ + '# Example workflow', + '', + 'Run gsd-tools state.patch --field status --value done to record completion directly.', + ].join('\n'); + + const out = findPromptSeamUses(PROMPT_FILE, text); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].line, 3); + assert.strictEqual(out[0].symbol, 'prompt-layer-state-write'); + }); + + test('control: the same candidate wrapped in backticks is a mention, not an invocation, and is not reported', () => { + // Proves the detection above is genuinely exercising the code-span + // exclusion, not merely that the fixture happens to score zero. + const text = 'Documentation only: `gsd-tools state.patch --field status --value done`.'; + assert.deepStrictEqual(findPromptSeamUses(PROMPT_FILE, text), []); + }); +}); + +// ─── D12: CRLF is scanned identically to LF ──────────────────────────────── + +describe('D12 — CRLF is scanned identically', () => { + test('guard: CRLF is scanned identically', () => { + const lfText = [ + 'function cmdSomethingElse(cwd) {', + ' writeStateMd(statePath, modified, cwd);', + '}', + ].join('\n'); + const crlfText = lfText.split('\n').join('\r\n'); + + const lfOut = findSeamBypasses(OTHER_FILE, lfText); + const crlfOut = findSeamBypasses(OTHER_FILE, crlfText); + + assert.strictEqual(crlfOut.length, 1); + const strip = (arr) => arr.map(({ line, symbol, source }) => ({ line, symbol, source })); + assert.deepStrictEqual(strip(crlfOut), strip(lfOut)); + // A stray trailing \r surviving into the reported source (the repo's + // documented \n-only-regex bug class) would show up here as a + // sanitized `\x0d` escape — it must not. + assert.strictEqual(crlfOut[0].source, 'writeStateMd(statePath, modified, cwd);'); + }); +}); + +// ─── D13: an unreadable file degrades, never crashes ─────────────────────── + +describe('D13 — an unreadable file is reported, not fatal', () => { + test('guard: an unreadable file is reported, not fatal', (t) => { + const originalReadFileSync = fs.readFileSync; + t.after(() => { + fs.readFileSync = originalReadFileSync; + }); + + // Monkeypatch (never chmod 0o000, which root bypasses under Docker/CI + // and would leave this assertion covering nothing). Scoped to + // BASELINE_PATH only, so no other read in this process is disturbed. + fs.readFileSync = function patchedReadFileSync(target, ...rest) { + if (target === BASELINE_PATH) { + const err = new Error('simulated unreadable baseline file'); + err.code = 'EACCES'; + throw err; + } + return originalReadFileSync.call(fs, target, ...rest); + }; + + // loadBaseline() must not throw — it degrades to a returned value. + assert.doesNotThrow(() => loadBaseline()); + const result = loadBaseline(); + // An unreadable file (EACCES) is NOT the same state as an absent one + // (ENOENT) and must not degrade to the same "no baseline yet" shape — + // collapsing the two is the exact ADR-3180/ADR-3408 failure mode this + // guard exists to catch. loadBaseline() must surface a distinguishable + // `entries: null` result carrying the underlying fs error code. + assert.deepStrictEqual(result, { entries: null, code: 'EACCES' }); + }); + + test('CLI: an unreadable baseline reaches REASON.BASELINE_UNREADABLE with its error code, not the first-run shape', (t) => { + const originalReadFileSync = fs.readFileSync; + t.after(() => { + fs.readFileSync = originalReadFileSync; + }); + + fs.readFileSync = function patchedReadFileSync(target, ...rest) { + if (target === BASELINE_PATH) { + const err = new Error('simulated unreadable baseline file'); + err.code = 'EACCES'; + throw err; + } + return originalReadFileSync.call(fs, target, ...rest); + }; + + // Drive main() in-process (not via the CLI subprocess helper) so the + // monkeypatched fs.readFileSync is actually in effect for the call. + const originalArgv = process.argv; + const originalWrite = process.stdout.write; + t.after(() => { + process.stdout.write = originalWrite; + process.argv = originalArgv; + process.exitCode = 0; + }); + let captured = ''; + process.stdout.write = function patchedWrite(chunk) { + captured += chunk; + return true; + }; + process.argv = [originalArgv[0], GUARD_PATH, '--json']; + guard.main(['--json']); + const exitCode = process.exitCode; + + assert.strictEqual(exitCode, 1); + const parsed = JSON.parse(captured); + assert.strictEqual(parsed.ok, false); + assert.strictEqual(parsed.findings.length, 1); + assert.strictEqual(parsed.findings[0].reason, REASON.BASELINE_UNREADABLE); + assert.strictEqual(parsed.findings[0].code, 'EACCES'); + }); +}); + +// ─── D14: field-name-keyed BRANCH comparisons (not just CALLS) ──────────── +// #3468: `applyPreserveIfPlaceholder` shipped a `field !== 'milestone_name'` +// branch — a field-name-keyed dispatch that routed around +// `getFieldClassification` entirely, so `FIELD_NAME_DISPATCH_RE` (which only +// matches the CALL shape) reported zero violations while the branch shape +// sat in the executor undetected. This section drives the widened detector +// directly against in-memory fixtures, mirroring D2's own "guard: does the +// pure function itself report" pattern. + +describe('D14 — field-name-keyed branch comparisons are caught', () => { + test('guard: field !== literal is reported as FIELD_NAME_DISPATCH', () => { + const text = [ + 'function applyPreserveIfPlaceholder(field, cls, ctx) {', + " if (field !== 'milestone_name') return;", + '}', + ].join('\n'); + + const out = findPolicyDispatchDrift(EXECUTOR_FILE, text); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].reason, REASON.FIELD_NAME_DISPATCH); + assert.strictEqual(out[0].line, 2); + assert.strictEqual(out[0].field, 'milestone_name'); + }); + + test('guard: field === literal is reported as FIELD_NAME_DISPATCH', () => { + const text = [ + 'function applyPreserveWhenUnchanged(field, cls, ctx) {', + " if (field === 'status') return;", + '}', + ].join('\n'); + + const out = findPolicyDispatchDrift(EXECUTOR_FILE, text); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].reason, REASON.FIELD_NAME_DISPATCH); + assert.strictEqual(out[0].line, 2); + assert.strictEqual(out[0].field, 'status'); + }); + + test('guard: the reversed literal === field is reported as FIELD_NAME_DISPATCH', () => { + const text = [ + 'function applyPreserveIfPlaceholder(field, cls, ctx) {', + " if ('milestone' === field) return;", + '}', + ].join('\n'); + + const out = findPolicyDispatchDrift(EXECUTOR_FILE, text); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].reason, REASON.FIELD_NAME_DISPATCH); + assert.strictEqual(out[0].line, 2); + assert.strictEqual(out[0].field, 'milestone'); + }); + + test('control: preservation === literal (the CORRECT policy-dispatch shape) is NOT reported', () => { + const text = [ + 'function applyPreserveAlways(field, cls, ctx) {', + " if (cls.preservation === 'preserve-always') return;", + '}', + ].join('\n'); + + assert.deepStrictEqual(findPolicyDispatchDrift(EXECUTOR_FILE, text), []); + }); + + test('control: an unrelated variable compared to a literal is NOT reported', () => { + const text = [ + 'function helper(status, cls, ctx) {', + " if (status !== 'unknown') return;", + '}', + ].join('\n'); + + assert.deepStrictEqual(findPolicyDispatchDrift(EXECUTOR_FILE, text), []); + }); + + test('control: bracket-indexed field access (ctx.postFm[field]) compared to a non-literal is NOT reported', () => { + const text = [ + 'function applyPreserveWhenUnchanged(field, cls, ctx) {', + ' if (ctx.postFm[field] === snapshot) return;', + '}', + ].join('\n'); + + assert.deepStrictEqual(findPolicyDispatchDrift(EXECUTOR_FILE, text), []); + }); +}); + +// ─── D15: `file` (and other attacker-derived fields) are sanitized AT +// CONSTRUCTION, not just by the human formatter ───────────────────────────── +// Security review finding: a repo can legally track a filename containing C1 +// control bytes or bidi-override codepoints — exactly as attacker-controlled +// on a fork PR as the `source` fragment this guard already sanitized before +// this fix. Before this fix `file` reached `--json` stdout and the committed +// baseline (`scripts/state-write-path-drift-baseline.json`) unsanitized — +// only the human formatter wrapped it. A finding's `file` (and any other +// attacker-derived field, like `field`) must come back escaped from the +// FINDER itself, so every consumer (human, `--json`, baseline) inherits the +// sanitization uniformly. +// +// The two attack codepoints are built via `String.fromCharCode` rather than +// embedded as literal bytes, so this test file's own source never carries a +// live control/bidi codepoint on disk. + +describe('D15 — file (and field) values are sanitized at construction', () => { + const RLO = String.fromCharCode(0x202e); // RIGHT-TO-LEFT OVERRIDE (bidi) + const C1_CSI = String.fromCharCode(0x9b); // C1 CONTROL: CSI + const ATTACK_FILE = `src/evil${RLO}${C1_CSI}name.cts`; + const ESCAPED_FILE = 'src/evil\\u202e\\x9bname.cts'; + + test('findSeamBypasses: an attacker-controlled filename comes back escaped', () => { + const text = ['function cmdSomethingElse(cwd) {', ' writeStateMd(statePath, modified, cwd);', '}'].join('\n'); + + const out = findSeamBypasses(ATTACK_FILE, text); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].file, ESCAPED_FILE); + // Neither raw attack codepoint survives in the finding at all — this is + // exactly what reaches `--json` stdout verbatim (JSON.stringify + // neutralizes C0 but NOT C1 or bidi codepoints, which is why + // construction-time escaping — not JSON.stringify — is load-bearing). + assert.ok(!out[0].file.includes(RLO)); + assert.ok(!out[0].file.includes(C1_CSI)); + + // The SAME escaped value is what a regenerated baseline entry persists — + // proving the fix reaches the committed + // scripts/state-write-path-drift-baseline.json, not just the finding. + const entries = buildBaselineEntries(out, null); + assert.strictEqual(entries.length, 1); + assert.strictEqual(entries[0].file, ESCAPED_FILE); + assert.ok(!entries[0].file.includes(RLO)); + assert.ok(!entries[0].file.includes(C1_CSI)); + }); + + test('findPromptSeamUses: an attacker-controlled filename comes back escaped', () => { + const text = 'Run gsd-tools state.patch --field status --value done to record completion directly.'; + + const out = findPromptSeamUses(ATTACK_FILE, text); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].file, ESCAPED_FILE); + assert.ok(!out[0].file.includes(RLO)); + assert.ok(!out[0].file.includes(C1_CSI)); + }); + + test('findPolicyDispatchDrift: filename AND the field literal are both escaped', () => { + const text = [" if (field === 'status" + RLO + C1_CSI + "') return;"].join('\n'); + + const out = findPolicyDispatchDrift(ATTACK_FILE, text); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].file, ESCAPED_FILE); + assert.strictEqual(out[0].field, 'status\\u202e\\x9b'); + assert.ok(!out[0].field.includes(RLO)); + assert.ok(!out[0].field.includes(C1_CSI)); + }); +}); diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 9ddded1e4..7ec5daf05 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -25,6 +25,14 @@ const workstreamInventory = require('../gsd-core/bin/lib/workstream-inventory.cj // Phase 12 (#3310, ADR-3180 §8.4 rule 3): `cmdStateValidate`'s `warnings` are // now `Diagnostic[]` (S0NN codes), not bare strings, and `drift` is gone. const { SEVERITY } = require('../gsd-core/bin/lib/health-diagnostic-types.cjs'); +// #3468 matrix B8: distinguishes a controlled, structured CLI failure (an +// `error()` call, which always emits plain text even under --json-errors — +// see `#2979` in tests/cli-exit.test.cjs) from an uncaught internal crash +// (any non-ExitError throw, which under --json-errors emits a JSON envelope +// carrying `reason: ERROR_REASON.SDK_FAIL_FAST`). The §8.2 invariant throw is +// exactly the latter shape, so this is the structured, non-prose signal B8 +// asserts against. +const { ERROR_REASON } = require('../gsd-core/bin/lib/io.cjs'); /** First `Diagnostic` in `output.warnings` carrying the given S0NN code, or `undefined`. */ function findWarning(output, code) { @@ -13541,3 +13549,165 @@ const HEX_RE = /^[0-9a-f]{4,40}$/i; assert.strictEqual(r.commits_behind, 0); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// #3468 matrix section B, rows B7/B8 — ADR-3408 §8.2's bright line, exercised +// through the REAL production write path rather than a hand-built +// applyStatePreservation input (CONTRIBUTING.md § Fixture provenance #2371: +// a test constructing its own convenient input proves a property no shipping +// caller exercises). +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3468 B7: the production caller wires every declared preserve-when-unchanged row', () => { + let tmpDir; + let statePath; + + beforeEach(() => { + tmpDir = createTempProject('gsd-3468-b7-'); + statePath = path.join(tmpDir, '.planning', 'STATE.md'); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('B7: readModifyWriteStateMd never throws the §8.2 invariant on a normal write', () => { + // The real caller (state.cts:2744-2795, the ONLY call site of + // applyStatePreservation) computes and wires bodyDeltas for every + // declared preserve-when-unchanged row. Driving a normal transform + // through the real function — not a hand-built applyStatePreservation + // call — is what proves B1's throw can never fire in production. + assert.doesNotThrow(() => { + stateLib.readModifyWriteStateMd( + statePath, + (content) => `${content}\n\n`, + tmpDir, + ); + }); + // Also exercise the resync:false (body-only) branch — a second, + // independently-reachable call shape into the same pipeline. + assert.doesNotThrow(() => { + stateLib.readModifyWriteStateMd( + statePath, + (content) => `${content}\n\n`, + tmpDir, + { resync: false }, + ); + }); + }); +}); + +describe('#3468 B8: a drifted / malformed / unparseable STATE.md never reaches the invariant throw (§8.2 bright line)', () => { + let tmpDir; + let statePath; + + beforeEach(() => { + tmpDir = createTempProject('gsd-3468-b8-'); + statePath = path.join(tmpDir, '.planning', 'STATE.md'); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // Each fixture replaces the fixture-generated STATE.md with a document that + // is drifted, malformed, or unparseable in a distinct way. None of these + // are internal invariant violations — they are exactly the "ordinary, + // expected user-document state" ADR-3408 §8.5 names, so the command must + // never surface the §8.2 invariant (an uncaught internal Error). + const fixtures = [ + { + name: 'drifted: frontmatter current_phase disagrees with the body Phase: line', + content: [ + '---', + 'gsd_state_version: 1.0', + 'current_phase: "7"', + 'status: executing', + '---', + '', + '# Project State', + '', + '## Current Position', + '', + 'Phase: 2 (Stale Name) — EXECUTING', + 'Plan: 1 of 3', + 'Status: Executing Phase 2', + '', + ].join('\n'), + }, + { + name: 'malformed: frontmatter fields carry the wrong YAML type', + content: [ + '---', + 'gsd_state_version: 1.0', + 'status: 42', + 'progress: "not-an-object"', + 'current_phase_name: [Unexpected, Array]', + '---', + '', + '# Project State', + '', + '## Current Position', + '', + 'Phase: 2 (Some Phase) — EXECUTING', + 'Plan: 1 of 3', + 'Status: Executing Phase 2', + '', + ].join('\n'), + }, + { + name: 'unparseable: frontmatter fence opens but never closes', + content: [ + '---', + 'gsd_state_version: 1.0', + 'status: executing', + '', + '# Project State (frontmatter never terminated above)', + '', + '## Current Position', + '', + 'Phase: 2 (Some Phase) — EXECUTING', + 'Plan: 1 of 3', + 'Status: Executing Phase 2', + '', + ].join('\n'), + }, + ]; + + for (const fixture of fixtures) { + test(`B8: ${fixture.name}`, () => { + fs.writeFileSync(statePath, fixture.content); + + const result = runGsdTools(['state', 'sync', '--json-errors'], tmpDir); + + if (result.success) { + assert.doesNotThrow( + () => JSON.parse(result.output), + `${fixture.name}: success output must be well-formed JSON; got: ${result.output}`, + ); + return; + } + + // A non-zero exit is allowed (a drifted document may be a legitimate + // domain-level failure) — but it must be a CONTROLLED failure. Under + // --json-errors, an ExitError (a deliberate error() call) always + // writes PLAIN TEXT to stderr (never JSON — see tests/cli-exit.test.cjs + // #2979), while an uncaught internal Error (what the §8.2 invariant + // throw would be, since it is deliberately never caught by a discriminator + // per ADR-3408 §8.2's "Rejected: Return a ScopedResult") is the ONLY + // path that emits the JSON envelope with reason: SDK_FAIL_FAST. So: if + // stderr parses as that envelope, the command crashed on an uncaught + // internal error — the exact bright line B8 forbids. + let structured = null; + try { structured = JSON.parse(result.error); } catch { /* plain text — a controlled ExitError, expected */ } + if (structured) { + assert.notStrictEqual( + structured.reason, + ERROR_REASON.SDK_FAIL_FAST, + `${fixture.name}: command crashed on an uncaught internal error instead of a controlled failure — ` + + `this is the §8.2 invariant bright line: a user document must never trigger it. stderr: ${result.error}`, + ); + } + }); + } +});