diff --git a/CLAUDE.md b/CLAUDE.md index 62f6030d7..8a5a80d44 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -29,7 +29,11 @@ sprint context — read them for background, never act on them. - Auth: user accounts remain hardcoded (no SSO, no real user directory). The JWT refresh token flow (HttpOnly cookie, token rotation, `/api/auth/refresh`) is approved and implemented. - Email: `WORKWELL_EMAIL_PROVIDER=simulated` is the default and must remain so on the demo stack. SendGrid wiring exists in the code but must not be activated unless `WORKWELL_EMAIL_SENDGRID_API_KEY` is explicitly set (with `WORKWELL_EMAIL_PROVIDER=sendgrid`) in a non-demo environment. - AI never decides compliance (see docs/AI_GUARDRAILS.md). CQL engine is sole source of truth. -- Every state change writes `audit_event` — no exceptions +- Every state change writes `audit_event` — the RULE, and it is not everywhere true today (#598). + **CASE actions audit first and cannot lose the event.** The run-created case transition, the measure + lifecycle, segment create and terminology-mapping create all MUTATE first and audit after, so each + can. Write new code audit-first; `DATA_MODEL_CONTRACTS` §4 carries the list and why the run's + ordering is deliberate. - No silent scope changes. If a stop condition triggers, document fallback in JOURNAL.md. - Schema migrations are owned by Taleef — never written or applied by an agent without explicit instruction diff --git a/backend-ts/src/measure/measure-read-models.ts b/backend-ts/src/measure/measure-read-models.ts index 417a9ebd9..f7eb0316d 100644 --- a/backend-ts/src/measure/measure-read-models.ts +++ b/backend-ts/src/measure/measure-read-models.ts @@ -171,7 +171,21 @@ export const compileAllowsActivation = (s: string) => s.toUpperCase() === "COMPI const OUTCOME_BUCKETS = new Set(["COMPLIANT", "DUE_SOON", "OVERDUE", "MISSING_DATA", "EXCLUDED"]); -/** Port of MeasureService.validateTests: a fixture set passes when non-empty and each fixture is well-formed. */ +/** + * Port of MeasureService.validateTests: a fixture set passes when non-empty and each fixture is + * well-formed. + * + * **It does NOT execute anything (#599), and the name is the whole problem.** "Tests passed" reads as + * "the fixtures ran and the measure produced the expected outcomes". What is checked is that the list + * is non-empty and that each entry has a name, a subject and an `expectedOutcome` in the allowed set. + * A fixture asserting an impossible outcome passes this gate, and so does one whose expected outcome + * contradicts the CQL — so activation was blocked by a control that could not fail on the thing its + * label implied. + * + * Studio's row now reads "Fixtures Well-Formed … not executed against the measure". Executing them is + * the real fix (the engine is right there, and a fixture is a subject plus an expected outcome) and + * belongs with the next Studio work; #599 carries it. + */ export function validateTests(fixtures: MeasureSpec["testFixtures"]): { passed: boolean; failures: string[] } { if (fixtures.length === 0) return { passed: false, failures: ["At least one test fixture is required before activation."] }; const failures: string[] = []; diff --git a/backend-ts/src/measure/validate-tests-does-not-execute.test.ts b/backend-ts/src/measure/validate-tests-does-not-execute.test.ts new file mode 100644 index 000000000..adfb51af3 --- /dev/null +++ b/backend-ts/src/measure/validate-tests-does-not-execute.test.ts @@ -0,0 +1,65 @@ +/** + * `validateTests` checks SHAPE, not outcomes (#599). + * + * Studio rendered a green tick on a row labelled "Test Fixtures" and blocked activation until it + * passed, which reads as *the fixtures were run and the measure produced the expected outcomes*. It + * never meant that: the function checks the list is non-empty and that each entry carries a name, a + * subject, and an `expectedOutcome` in the allowed set. So activation was gated by a control that + * could not fail on the thing its label implied — the vacuous-guard shape this repo keeps finding. + * + * These tests pin the limitation rather than the copy, and that is deliberate. The Studio row now + * says "Fixtures Well-Formed … not executed against the measure", but a label is easy to drift back. + * If someone later implements execution (issue #599's option 2, which is the real fix), the first + * test here FAILS — forcing the label and the behaviour to move together rather than apart. + * + * node --import tsx --test src/measure/validate-tests-does-not-execute.test.ts + */ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { validateTests } from "./measure-read-models.ts"; + +type Fixture = Parameters[0][number]; + +const fixture = (over: Partial = {}): Fixture => + ({ + fixtureName: "a well-formed fixture", + employeeExternalId: "emp-006", + expectedOutcome: "COMPLIANT", + ...over, + }) as Fixture; + +test("a fixture naming a subject that does not exist still PASSES — nothing is executed", () => { + // `nobody-at-all` is in no directory and no corpus. If this function ran the measure it could not + // report success here. When #599's option 2 lands, this is the test that tells you to move the + // label with it. + const result = validateTests([fixture({ employeeExternalId: "nobody-at-all" })]); + assert.equal(result.passed, true, "shape-only: the subject is never looked up"); + assert.deepEqual(result.failures, []); +}); + +test("a fixture whose expected outcome contradicts the measure still passes", () => { + // Two fixtures for the SAME subject asserting opposite outcomes. At most one can be right, and a + // gate that evaluated anything would say so. + const result = validateTests([ + fixture({ fixtureName: "compliant", expectedOutcome: "COMPLIANT" }), + fixture({ fixtureName: "overdue", expectedOutcome: "OVERDUE" }), + ]); + assert.equal(result.passed, true, "contradictory expectations are not detectable without executing"); +}); + +test("what it DOES check: presence, a name, a subject, and a known outcome bucket", () => { + // The other half of an honest label - the check is real, it is just narrower than it read. + assert.equal(validateTests([]).passed, false, "an empty set blocks activation"); + assert.match(validateTests([]).failures[0]!, /At least one test fixture/); + + assert.equal(validateTests([fixture({ fixtureName: " " })]).passed, false, "a blank name is not a name"); + assert.equal(validateTests([fixture({ employeeExternalId: "" })]).passed, false, "a subject is required"); + + const unknown = validateTests([fixture({ expectedOutcome: "WIBBLE" as Fixture["expectedOutcome"] })]); + assert.equal(unknown.passed, false); + assert.match(unknown.failures[0]!, /unsupported expectedOutcome/); + + // And every failure names WHICH fixture, since an author has to find it. + const second = validateTests([fixture(), fixture({ fixtureName: "" })]); + assert.match(second.failures[0]!, /Fixture 2/); +}); diff --git a/docs/DATA_MODEL_CONTRACTS.md b/docs/DATA_MODEL_CONTRACTS.md index 731590f9c..38ee2fcb2 100644 --- a/docs/DATA_MODEL_CONTRACTS.md +++ b/docs/DATA_MODEL_CONTRACTS.md @@ -121,6 +121,46 @@ SQLite floor and the Pg ceiling read the current row and apply the shared pure ` instead of being left stuck RUNNING / marked FAILED after the case was already mutated (mirrors the `RUN_COMPLETED` best-effort write). + > **So "every state change writes an `audit_event`" is the RULE, and it is not everywhere true + > today (#598).** CLAUDE.md stated it with "no exceptions", and the exceptions were discoverable + > only by reading the source — which is the shape of claim this file exists to stop. + > + > **This correction was itself too broad on its first cut** (Codex review), which is worth recording + > because it is the same failure one level up: it said "operator actions audit first and cannot lose + > the event", true of CASE actions and false of several other operator surfaces. The list below came + > from a sweep for the mutate-before-audit shape rather than from memory. + > + > **AUDIT FIRST — cannot produce an unaudited state change:** + > - every case action (`case/case-actions.ts`), where `recordCaseEvent` makes the action row and the + > audit row one transaction and the patch follows; + > - rerun-to-verify's case patch (`case/case-rerun.ts`), which says so at the call site; + > - bulk assign and panel backfill, through the batch `recordCaseEvents`. + > + > A failure between the two leaves an action **recorded but not applied** — recoverable, and never a + > silent state change. + > + > **MUTATE FIRST — can apply a change and lose the event:** + > - the **run-created case transition** (`run/run-pipeline.ts`), which audits best-effort after the + > upsert. **This one is deliberate**: the alternative strands an otherwise-complete run as RUNNING + > after the case was already mutated; + > - the **measure lifecycle** — create, approve, deprecate and the explicit status transition + > (`measure/measure-lifecycle.ts`); + > - **segment create** (`routes/segments.ts`) and **terminology-mapping create** + > (`measure/value-set-governance.ts`). + > + > Only the first is a considered trade. The rest are simply the order they were written in, and new + > code should audit first. + > + > **What is missing is the primitive, not the ordering** (for the run; for the others the ordering + > is missing too)**.** There is no `applyCaseAction({ patch, + > action, audit })` making all three one unit, and there cannot be one inside a single store: the + > action and audit rows belong to `CaseEventStore` (which already opens its own `BEGIN`/`COMMIT`) + > while the patch belongs to `CaseStore`, so a real fix needs a transaction seam spanning both. + > Until that exists, **do not build operational reliance on the ledger being complete for + > run-created transitions.** A reconciliation job is not a substitute: without durable operation + > identity, an expected version, a deadline and a visible failure state it is a second unreliable + > thing checking the first. + ### The work list is READ two ways, and they must answer the same question (#561, ADR-084) `/api/cases` takes ONE page and the exact total from a single statement (`CaseStore.listCasesPage`, diff --git a/docs/JOURNAL.md b/docs/JOURNAL.md index 910323d50..30e9799e3 100644 --- a/docs/JOURNAL.md +++ b/docs/JOURNAL.md @@ -1,5 +1,71 @@ # Journal +## 2026-09-21 (evening) — two controls that promised more than they did + +**#598** and **#599**, both from the 2026-09-07 review, both about a claim being wider than the thing +behind it. Neither is a behaviour change; both are the always-loaded contract and a label catching up +with the code. + +### #598 — "every state change writes an audit_event" is true of operator actions, and not of runs + +`CLAUDE.md` states the rule with "no exceptions". The run path is an exception, and it was +discoverable only by reading `run-pipeline.ts`. + +The two paths make **opposite trades on purpose**: + +- An **operator action** records the event first, then applies the patch (`recordCaseEvent` makes the + action row and the audit row one transaction). A failure between them leaves an action recorded but + not applied — never an unaudited state change. +- A **run** upserts the case first, then audits best-effort. A failure there leaves a state change + applied but unaudited, accepted because the alternative strands an otherwise-complete run as + RUNNING after the case was already mutated. + +Both orderings stay. What is missing is the **primitive**, and it cannot live in one store: the action +and audit rows belong to `CaseEventStore` (which already opens its own `BEGIN`/`COMMIT`) while the +patch belongs to `CaseStore`, so a real `applyCaseAction({ patch, action, audit })` needs a +transaction seam spanning both. That is deferred deliberately, not forgotten — it wants local +Postgres, since the SQLite floor cannot catch Pg-only SQL, and #598 carries it with the line that +until it exists **nothing should build operational reliance on the ledger being complete for +run-created transitions.** + +The correction is in `DATA_MODEL_CONTRACTS` §4 beside the best-effort note and in `CLAUDE.md`'s rule +itself, because a rule whose exception lives in a source file is a rule a session will contradict. + +**And the correction was itself too broad** (Codex, on the PR). It said "operator actions audit +first and cannot lose the event" — true of CASE actions, false of several other operator surfaces. +The same failure one level up, in a change whose entire subject is claims being wider than what is +behind them. + +So the paths were enumerated by a sweep for the shape rather than recalled. **Audit-first:** every +case action, rerun-to-verify's case patch, bulk assign and panel backfill. **Mutate-first, and so +able to lose the event:** the run-created case transition, the measure lifecycle (create, approve, +deprecate, transition), segment create, terminology-mapping create. + +`case-rerun.ts` looked like a violation to the sweep and is not — its first mutation creates a RUN +row, and the case patch is explicitly after an audit-first `recordCaseEvent`, with a comment saying +so. Worth the check: it would have been an easy thing to assert wrongly in the other direction. + +**Only the run's ordering is a considered trade.** The other six are the order they happened to be +written in, which splits #598 into a cheap half — flip them, no seam needed — and the primitive that +still needs a cross-store transaction. + +### #599 — an approval gate that could not fail on the thing its label implied + +Studio rendered "Test Fixtures ✅" and blocked activation until it passed, which reads as *the +fixtures ran and the measure produced the expected outcomes*. `validateTests` never executes +anything: it checks the list is non-empty and that each entry has a name, a subject, and an +`expectedOutcome` in the allowed set. + +The row now reads **"Fixtures Well-Formed — present and well-formed; not executed against the +measure"**, and the function's docstring says the same. + +**The guard went on the semantics, not the copy.** A render test asserting label text would need four +child panels mocked to check a string, and would pin the wording rather than the meaning. Instead +three backend tests pin the *limitation*: a fixture naming a subject that exists nowhere still +passes, and two fixtures asserting opposite outcomes for the same subject both pass. If someone +implements execution — #599's option 2, the real fix — **those tests fail**, which forces the label +and the behaviour to move together instead of drifting apart again. + ## 2026-09-21 (later) — the review's one high-priority defect, and it was worse than the review said **#594** and **#595** were accepted on 2026-09-08, named "the next two slices", and then sat for diff --git a/frontend/features/studio/components/ReleaseApprovalTab.tsx b/frontend/features/studio/components/ReleaseApprovalTab.tsx index 21931666c..a3b3f0d36 100644 --- a/frontend/features/studio/components/ReleaseApprovalTab.tsx +++ b/frontend/features/studio/components/ReleaseApprovalTab.tsx @@ -140,7 +140,26 @@ export function ReleaseApprovalTab({

Readiness Checklist

Compile Status: {compileReady ? "✅" : "❌"} {formatStatusLabel(activationReadiness?.compileStatus ?? "UNKNOWN")}

-

Test Fixtures: {testsReady ? "✅" : "❌"} {activationReadiness?.testFixtureCount ?? 0} fixtures

+ {/* + * "Test Fixtures ✅" read as "the fixtures ran and the measure produced the expected + * outcomes" (#599). It never meant that: `validateTests` checks the list is non-empty and + * each entry is well-formed - a name, a subject, an outcome in the allowed set - and never + * executes the measure. A fixture asserting an impossible outcome passes, and so does one + * that contradicts the CQL, so this was an approval gate that could not fail on the thing + * its name implied. + * + * The label now says what is checked. Actually running them is the real fix and belongs + * with the next Studio work; until then the row must not imply a verification nobody did. + */} +

+ Fixtures Well-Formed:{" "} + + {testsReady ? "✅" : "❌"} {activationReadiness?.testFixtureCount ?? 0} fixtures + + + — present and well-formed; not executed against the measure + +

Value Set Resolvability:{" "} {hasValueSets ? (