From 525835649fff29ab38387faf40fbb713485e4eaf Mon Sep 17 00:00:00 2001 From: Taleef Date: Mon, 21 Sep 2026 16:30:06 -0400 Subject: [PATCH 1/3] =?UTF-8?q?fix(audit):=20finish=20#598's=20triage=20?= =?UTF-8?q?=E2=80=94=20five=20more=20audit-first,=20and=20the=20rule=20get?= =?UTF-8?q?s=20a=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The owner's #598 decision was that a path which CAN audit before it mutates should. #607/#608 took the plain reorders; this finishes the sweep's output, with every remaining candidate opened rather than named from the tool's summary. **Five flipped, each needing a one-field seam change:** `createMeasure`, segment create, segment UPDATE, segment delete, `uploadEvidence`. `CreateMeasureInput`, `CreateSegmentInput` and `InsertEvidenceInput` now accept the value the event keys on — optional, minted by the store when absent, so every other caller is unchanged. That was the whole obstacle: the store minted the id, or for evidence the `uploadedAt` the payload reports as `payload.timestamp`. Segment UPDATE needed three writes moved rather than one (`updateSegment`, `setMeasures`, `setOverrides`), so a failure after the first left a partly-updated segment with no event at all; its 404 became an explicit pre-read, because `updateSegment` returning null WAS the not-found signal, which is what made the old order unavoidable. `uploadEvidence` audits before the BUCKET write too — an object in storage the ledger never mentions is harder to notice than a missing row. **`src/audit/audit-order.test.ts` exists because nothing tested the rule.** Nine call sites had been flipped across three commits and no test could tell: every existing test asserts the event EXISTS after a SUCCEEDING operation, which is equally true in either order, so a reorder back was silent. Each case makes the MUTATION fail and requires the event anyway — the only externally visible difference between the orders. Mutation-checked on two. Still mutate-first, with the reason now at each call site: the run-created transition and the import-driven finalize (deliberate — the event is best-effort at the run boundary); `dispatchOutreach`, which dispatches a message before any ledger entry and builds its payload from the delivery result, so it needs ADR-073 d4's intent-then-completion pair and that adds an event type consumers read; the three identity-link writes, whose obstacle is sharper than "the store mints the id" — `upsertLink` returns the EXISTING row's id on conflict, so keying those events on the PAIR is the fix and it changes what `entity_id` means; and the two backfill scripts, which are seeding tools rather than operator surfaces. Checked and NOT violations, every one a matcher artifact: `audit-packet` (a hash), `materialize-run` and `backfill-trend-history` (reads), evidence download, `measure-seed` (itself audit-first), subject-list create (its audit is a `beforeComplete` callback that runs before the list becomes visible), and panel assignment, which audits before the mapping and records each per-case event before `assignCases`. The sweep is fully triaged, which is not #598 closing: what remains is the cross-store `applyCaseAction` primitive plus the outreach and identity decisions. §4 and CLAUDE.md say exactly that. Backend 2,856 tests: 2,832 pass, 23 skip, 1 pre-existing local failure (`corpus-membership`). --- CLAUDE.md | 9 +- backend-ts/src/audit/audit-order.test.ts | 259 ++++++++++++++++++ backend-ts/src/case/evidence-service.ts | 40 +-- backend-ts/src/measure/measure-lifecycle.ts | 26 +- backend-ts/src/routes/segments.test.ts | 58 ++++ backend-ts/src/routes/segments.ts | 52 ++-- backend-ts/src/stores/evidence-store.ts | 7 + backend-ts/src/stores/measure-store.ts | 8 + .../postgres/evidence-store-postgres.ts | 2 +- .../stores/postgres/measure-store-postgres.ts | 4 +- .../stores/postgres/segment-store-postgres.ts | 2 +- backend-ts/src/stores/segment-store.ts | 7 + .../stores/sqlite/evidence-store-sqlite.ts | 2 +- .../src/stores/sqlite/measure-store-sqlite.ts | 4 +- .../src/stores/sqlite/segment-store-sqlite.ts | 2 +- docs/DATA_MODEL_CONTRACTS.md | 71 +++-- docs/JOURNAL.md | 50 ++++ 17 files changed, 521 insertions(+), 82 deletions(-) create mode 100644 backend-ts/src/audit/audit-order.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 554031e9e..3bb859f61 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -31,10 +31,11 @@ sprint context — read them for background, never act on them. - AI never decides compliance (see docs/AI_GUARDRAILS.md). CQL engine is sole source of truth. - Every state change writes `audit_event` — the RULE, and **not yet everywhere true (#598)**. Write new code **audit-first**: the ledger errs toward an over-claim rather than a silent state change. - Case actions and several measure/value-set paths already do; the run-created case transition - deliberately does not, and an untriaged set remains. `DATA_MODEL_CONTRACTS` §4 has what is verified, - and `backend-ts/scripts/audit-order-sweep.py` finds candidates — **the inventory is not complete, so - #598 does not close on §4 alone.** + Case actions, the measure/segment/value-set/waiver/appointment/evidence paths already do; the + run-created case transition and the import-driven finalize deliberately do not, and outreach cannot + without a new event pair. `DATA_MODEL_CONTRACTS` §4 has the whole triage and + `backend-ts/scripts/audit-order-sweep.py` re-derives it — **but #598 does not close on §4, because + what is still missing is the cross-store `applyCaseAction` PRIMITIVE, not the ordering.** - 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/audit/audit-order.test.ts b/backend-ts/src/audit/audit-order.test.ts new file mode 100644 index 000000000..bde176476 --- /dev/null +++ b/backend-ts/src/audit/audit-order.test.ts @@ -0,0 +1,259 @@ +/** + * The audit-first rule, asserted rather than commented (#598). + * + * `CLAUDE.md` and `DATA_MODEL_CONTRACTS` §4 say: write new code audit-first, so the ledger errs toward + * an **over-claim** — an event for a change that then failed to commit — rather than toward a silent + * state change. Nine call sites were flipped to that order across #607/#608 and this change, and **not + * one test could tell.** Every existing test asserts that the event EXISTS after a successful + * operation, which is equally true in either order; a reorder back would have been silent. + * + * So each case here makes the MUTATION fail and requires the event to be there anyway. That is exactly + * the property the rule promises, it is the only externally visible difference between the two orders, + * and reversing any one call site fails the corresponding case. + * + * The route-level surfaces (segments, subject lists) resolve their stores from `env` rather than taking + * them injected, so their ordering is not reachable this way; `routes/segments.test.ts` pins the + * enabling half instead — that the audited entity id is the id the row is created under. + * + * node --import tsx --test src/audit/audit-order.test.ts + */ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import type { AppendAuditInput } from "../stores/case-event-store.ts"; +import { grantWaiver } from "../admin/waivers.ts"; +import { scheduleAppointment } from "../case/appointment-service.ts"; +import { uploadEvidence } from "../case/evidence-service.ts"; +import { createMeasure, approveMeasure, deprecateMeasure } from "../measure/measure-lifecycle.ts"; + +/** A ledger that records what it was asked to append, in order. */ +function ledger() { + const events: AppendAuditInput[] = []; + return { + events, + types: () => events.map((e) => e.eventType), + appendAudit: async (input: AppendAuditInput) => { + events.push(input); + }, + // `recordCaseEvent` is the action+audit transaction the case surfaces use; both halves land here. + recordCaseEvent: async (input: { action: unknown; audit: AppendAuditInput }) => { + events.push(input.audit); + }, + recordCaseEvents: async (inputs: Array<{ action: unknown; audit: AppendAuditInput }>) => { + for (const i of inputs) events.push(i.audit); + }, + }; +} + +const BOOM = new Error("the write failed"); +const explode = async (): Promise => { + throw BOOM; +}; + +// A record whose activation readiness PASSES, so `approveMeasure` reaches its write rather than +// throwing on a blocker — the fixture is what `validateTests` requires, not decoration. +const MEASURE = { + measureId: "audiogram", + versionId: "v-1", + name: "Audiogram", + policyRef: "OSHA 1910.95", + owner: "safety", + version: "v1.0", + status: "Draft", + compileStatus: "COMPILED", + tags: [], + spec: { + testFixtures: [{ fixtureName: "overdue welder", employeeExternalId: "emp-001", expectedOutcome: "OVERDUE" }], + }, +}; + +const CASE = { + id: "11111111-2222-3333-4444-555555555555", + employeeId: "emp-001", + measureId: "audiogram", + evaluationPeriod: "2026", + status: "OPEN", + priority: "HIGH", + assignee: null, + nextAction: null, + lastRunId: "run-1", + currentOutcomeStatus: "OVERDUE", + createdAt: "2026-01-01T00:00:00.000Z", + updatedAt: "2026-01-01T00:00:00.000Z", +}; + +test("grantWaiver: the WAIVER_GRANTED event survives a failed insert", async () => { + const log = ledger(); + await assert.rejects( + () => + grantWaiver( + { + waivers: { insert: explode } as never, + measures: { getLatest: async () => MEASURE, getByVersionId: async () => MEASURE } as never, + events: log as never, + }, + { employeeExternalId: "emp-001", measureId: "audiogram", exclusionReason: "Medically exempt", expiresAt: null, notes: null, active: true }, + "admin", + ), + /the write failed/, + ); + assert.deepEqual(log.types(), ["WAIVER_GRANTED"]); + // And the event describes the waiver the insert was about to write, not a row it read back. + const payload = log.events[0]!.payload as Record; + assert.equal(payload.measureId, "audiogram"); + assert.equal(payload.exclusionReason, "Medically exempt"); + assert.equal(payload.active, true); +}); + +test("scheduleAppointment: the case action + audit survive a failed insert", async () => { + const log = ledger(); + await assert.rejects( + () => + scheduleAppointment( + { + appointments: { insert: explode } as never, + cases: { getCase: async () => CASE, patchCase: explode } as never, + events: log as never, + outcomes: { listOutcomes: async () => [] } as never, + }, + CASE.id, + { appointmentType: "Audiometric test", scheduledAt: "2026-10-01T15:00:00.000Z", location: "Plant A", notes: null }, + "nurse", + ), + /the write failed/, + ); + assert.deepEqual(log.types(), ["APPOINTMENT_SCHEDULED"]); +}); + +test("uploadEvidence: the event survives BOTH a failed bucket write and a failed insert", async () => { + // Two separate cases, because the bucket write is the one whose exposure is wider than a row: an + // object in storage the ledger never mentions is harder to notice than a missing row. + const bytes = new TextEncoder().encode("name,role\nOmar,Welder\n"); + const upload = { bytes, fileName: "evidence.csv", description: "signed form" }; + + const failedBucket = ledger(); + await assert.rejects( + () => + uploadEvidence( + { + evidence: { insert: explode } as never, + cases: { getCase: async () => CASE } as never, + bucket: { put: explode } as never, + events: failedBucket as never, + }, + CASE.id, + upload, + "nurse", + ), + /the write failed/, + ); + assert.deepEqual(failedBucket.types(), ["EVIDENCE_UPLOADED"]); + + const failedRow = ledger(); + let putCalls = 0; + await assert.rejects( + () => + uploadEvidence( + { + evidence: { insert: explode } as never, + cases: { getCase: async () => CASE } as never, + bucket: { put: async () => { putCalls++; } } as never, + events: failedRow as never, + }, + CASE.id, + upload, + "nurse", + ), + /the write failed/, + ); + assert.deepEqual(failedRow.types(), ["EVIDENCE_UPLOADED"]); + assert.equal(putCalls, 1); + // `payload.timestamp` used to read `record.uploadedAt`, which the store minted — the reason this + // one needed a seam change rather than a reorder. It must now be a real stamp, not empty. + const payload = failedRow.events[0]!.payload as Record; + assert.match(String(payload.timestamp), /^\d{4}-\d{2}-\d{2}T/); +}); + +test("uploadEvidence: a successful upload stores the stamp the event reported", async () => { + // The other half of the seam change, and the way it could go quietly wrong: mint the stamp for the + // event and let the store mint a second one for the row, so the ledger and the attachment disagree + // about when the upload happened. + const log = ledger(); + let stored: Record | null = null; + const record = await uploadEvidence( + { + evidence: { + insert: async (input: Record) => { + stored = input; + return { ...input, uploadedAt: input.uploadedAt }; + }, + } as never, + cases: { getCase: async () => CASE } as never, + bucket: { put: async () => {} } as never, + events: log as never, + }, + CASE.id, + { bytes: new TextEncoder().encode("hello"), fileName: "note.txt", description: null }, + "nurse", + ); + const payload = log.events[0]!.payload as Record; + assert.equal(stored!.uploadedAt, payload.timestamp); + assert.equal(record.uploadedAt, payload.timestamp); +}); + +test("createMeasure: the MEASURE_CREATED event survives a failed insert, and names the version it would have made", async () => { + const log = ledger(); + await assert.rejects( + () => + createMeasure( + { measures: { createMeasure: explode } as never, events: log as never }, + { name: "Respirator fit test", policyRef: "OSHA 1910.134", owner: "safety" }, + "admin", + ), + /the write failed/, + ); + assert.deepEqual(log.types(), ["MEASURE_CREATED"]); + assert.match(String(log.events[0]!.entityId), /^[0-9a-f-]{36}$/, "a real version id, minted caller-side"); + assert.equal(log.events[0]!.entityId, log.events[0]!.refMeasureVersionId); +}); + +test("createMeasure: a successful create inserts under the ids the event named", async () => { + // The enabling change, and the way it could regress silently: mint ids for the event and let the + // store mint its own, leaving every MEASURE_CREATED event pointing at a version that never existed. + const log = ledger(); + let passed: Record | null = null; + const returned = await createMeasure( + { + measures: { + createMeasure: async (input: Record) => { + passed = input; + return { ...MEASURE, measureId: input.measureId, versionId: input.versionId }; + }, + } as never, + events: log as never, + }, + { name: "Respirator fit test", policyRef: "OSHA 1910.134", owner: "safety" }, + "admin", + ); + assert.equal(passed!.versionId, log.events[0]!.entityId); + assert.equal(passed!.measureId, returned); + assert.equal((log.events[0]!.payload as Record).measureId, returned); +}); + +test("approveMeasure / deprecateMeasure: the event survives a failed status write", async () => { + for (const [label, run] of [ + ["MEASURE_APPROVED", (deps: never) => approveMeasure(deps, "audiogram", "approver")], + ["MEASURE_DEPRECATED", (deps: never) => deprecateMeasure(deps, "audiogram", "retired", "admin")], + ] as const) { + const log = ledger(); + const status = label === "MEASURE_APPROVED" ? "Draft" : "Active"; + await assert.rejects( + () => + run({ + measures: { getLatest: async () => ({ ...MEASURE, status }), setVersionStatus: explode } as never, + events: log as never, + } as never), + /the write failed/, + ); + assert.deepEqual(log.types(), [label], `${label} is written before the status changes`); + } +}); diff --git a/backend-ts/src/case/evidence-service.ts b/backend-ts/src/case/evidence-service.ts index 1f38ae850..c58464a8b 100644 --- a/backend-ts/src/case/evidence-service.ts +++ b/backend-ts/src/case/evidence-service.ts @@ -127,25 +127,14 @@ export async function uploadEvidence(deps: EvidenceDeps, caseId: string, input: const storageKey = `${caseId}/${evidenceId}-${safeName}`; const description = input.description && input.description.trim() !== "" ? input.description.trim() : null; - // Does NOT audit first (#598), and cannot without a store change: the event's `payload.timestamp` - // reads `record.uploadedAt`, which `EvidenceStore.insert` mints — the same "the store mints the - // value" cause as `createMeasure` and segment create, one field over. Adding `uploadedAt` to - // `InsertEvidenceInput` would fix it on both stores; that is a seam change, not a reorder. + // AUDIT BEFORE MUTATE (#598), and before the BUCKET write as well as the row — the exposure here is + // wider than a row, because a failed audit could otherwise leave an object in storage the ledger + // never mentions. // - // Worth noting the exposure is wider here than a row: the BUCKET write lands first too, so a failed - // audit can leave an object in storage that the ledger never mentions. - await deps.bucket.put(storageKey, input.bytes, { httpMetadata: { contentType: mimeType } }); - const record = await deps.evidence.insert({ - id: evidenceId, - caseId, - uploadedBy: actor, - fileName: safeName, - fileSizeBytes: input.bytes.length, - mimeType, - storageKey, - description, - }); - + // `uploadedAt` is minted HERE rather than by the insert (the store now accepts one), which is what + // makes the order possible: the event's `payload.timestamp` IS this value, and reading it back off + // the record is what forced the old order. + const uploadedAt = new Date().toISOString(); await deps.events.appendAudit({ eventType: "EVIDENCE_UPLOADED", entityType: "evidence", @@ -160,9 +149,22 @@ export async function uploadEvidence(deps: EvidenceDeps, caseId: string, input: mimeType, fileSizeBytes: input.bytes.length, description: description ?? "", - timestamp: record.uploadedAt, + timestamp: uploadedAt, }, }); + + await deps.bucket.put(storageKey, input.bytes, { httpMetadata: { contentType: mimeType } }); + const record = await deps.evidence.insert({ + id: evidenceId, + caseId, + uploadedBy: actor, + fileName: safeName, + fileSizeBytes: input.bytes.length, + mimeType, + storageKey, + description, + uploadedAt, + }); return record; } diff --git a/backend-ts/src/measure/measure-lifecycle.ts b/backend-ts/src/measure/measure-lifecycle.ts index 18659d25b..4f01f273c 100644 --- a/backend-ts/src/measure/measure-lifecycle.ts +++ b/backend-ts/src/measure/measure-lifecycle.ts @@ -28,7 +28,15 @@ export class MeasureError extends Error {} // `/deprecate` route (reason required), not this APPROVER-reachable `/status` path (Fable M2). const ALLOWED_TRANSITIONS = new Set(["Draft->Approved", "Approved->Active"]); -async function audit(deps: MeasureLifecycleDeps, eventType: string, r: MeasureRecord, actor: string, payload: Record): Promise { +// `Pick<..., "versionId">` rather than the whole record, because that is the only field read — and +// since #598 `createMeasure` audits BEFORE the insert, where no record exists yet to hand over. +async function audit( + deps: MeasureLifecycleDeps, + eventType: string, + r: Pick, + actor: string, + payload: Record, +): Promise { await deps.events.appendAudit({ eventType, entityType: "measure_version", @@ -51,13 +59,15 @@ export async function createMeasure( const policyRef = input.policyRef?.trim(); const owner = input.owner?.trim(); if (!name || !policyRef || !owner) throw new MeasureError("name, policyRef and owner are required"); - // The one in this file that does NOT audit first (#598), and it cannot without a store change: the - // audit is keyed on `r.versionId`, which the insert mints. Minting the id caller-side (as - // `createTerminologyMapping` does) or an intent-then-completion pair (ADR-073 d4's compaction - // pattern) would both work; either is a real change rather than a reorder, so it stays on #598. - const r = await deps.measures.createMeasure({ name, policyRef, owner }); - await audit(deps, "MEASURE_CREATED", r, actor, { measureId: r.measureId, name, policyRef, owner }); - return r.measureId; + // AUDIT BEFORE MUTATE (#598). Both ids are minted HERE rather than by the insert — the store now + // accepts them — so the event can name the version before the row exists. `audit` takes the record + // it keys on, so it is handed the ids directly; every field in the payload is an input, not a + // read-back. + const measureId = crypto.randomUUID(); + const versionId = crypto.randomUUID(); + await audit(deps, "MEASURE_CREATED", { versionId }, actor, { measureId, name, policyRef, owner }); + await deps.measures.createMeasure({ name, policyRef, owner, measureId, versionId }); + return measureId; } /** POST /api/measures/:id/approve — Draft → Approved (gated on the activation readiness). */ diff --git a/backend-ts/src/routes/segments.test.ts b/backend-ts/src/routes/segments.test.ts index 159385c2e..19022a20e 100644 --- a/backend-ts/src/routes/segments.test.ts +++ b/backend-ts/src/routes/segments.test.ts @@ -188,3 +188,61 @@ test("POST /api/segments/preview → 400 on a malformed rule (op/value shape)", ); assert.equal(res?.status, 400); }); + +/** + * The audit-first half of #598 for this route. The ORDER is not reachable from outside — the route + * resolves its stores from `env` rather than taking them injected, so no test here can make the insert + * fail and watch the event survive (`src/audit/audit-order.test.ts` does that for the injectable + * services). What IS reachable is the enabling change, and the way it could regress silently: mint an + * id for the event and let the store mint its own, leaving every SEGMENT_CREATED event pointing at a + * segment that never existed. + */ +test("SEGMENT_CREATED names the id the segment is created under (#598)", async () => { + const res = await post({ name: "Audit-order welders", rule: welderRule, measureIds: ["audiogram"] }); + assert.equal(res?.status, 201); + const created = (await res!.json()) as { id: string; name: string }; + const row = await (env.DB as { prepare: (s: string) => { bind: (...a: unknown[]) => { first: () => Promise } } }) + .prepare("SELECT entity_id, entity_type, payload_json FROM audit_events WHERE event_type = 'SEGMENT_CREATED' AND entity_id = ?") + .bind(created.id) + .first<{ entity_id: string; entity_type: string; payload_json: string }>(); + assert.ok(row, "the event names the created segment, not an id the store discarded"); + assert.equal(row!.entity_type, "segment"); + assert.equal((JSON.parse(row!.payload_json) as { name: string }).name, "Audit-order welders"); +}); + +test("SEGMENT_UPDATED reports the post-state resolved from the pre-state and the request (#598)", async () => { + // The payload used to come from a re-read AFTER the three writes, which is what forced the audit to + // come second. It is now merged from the row as it was plus the fields the request carries — so a + // PARTIAL body must still report the unchanged fields, not drop them. + const created = (await (await post({ name: "Before", rule: welderRule, measureIds: ["audiogram"] }))!.json()) as { id: string }; + const res = await put(created.id, { name: "After" }); // name only: measureIds and enabled are untouched + assert.equal(res?.status, 200); + const row = await (env.DB as { prepare: (s: string) => { bind: (...a: unknown[]) => { first: () => Promise } } }) + .prepare("SELECT payload_json FROM audit_events WHERE event_type = 'SEGMENT_UPDATED' AND entity_id = ?") + .bind(created.id) + .first<{ payload_json: string }>(); + assert.ok(row); + const payload = JSON.parse(row!.payload_json) as { name: string; enabled: boolean; measureIds: string[] }; + assert.equal(payload.name, "After", "the requested change"); + assert.deepEqual(payload.measureIds, ["audiogram"], "and the fields the request did not mention"); + assert.equal(payload.enabled, true); +}); + +test("DELETE audits before the row goes, and a missing segment is still a 404 with no event (#598)", async () => { + const created = (await (await post({ name: "Doomed", rule: welderRule, measureIds: [] }))!.json()) as { id: string }; + assert.equal((await del(created.id))?.status, 204); + const q = (env.DB as { prepare: (s: string) => { bind: (...a: unknown[]) => { first: () => Promise } } }); + const row = await q + .prepare("SELECT payload_json FROM audit_events WHERE event_type = 'SEGMENT_DELETED' AND entity_id = ?") + .bind(created.id) + .first<{ payload_json: string }>(); + assert.equal((JSON.parse(row!.payload_json) as { name: string }).name, "Doomed", "the name is read off the row before it is deleted"); + // An unknown id refuses before it audits — an over-claim is the side the rule picks, but only for a + // change somebody actually asked for. + assert.equal((await del("00000000-0000-4000-8000-000000000000"))?.status, 404); + const none = await q + .prepare("SELECT COUNT(*) AS n FROM audit_events WHERE event_type = 'SEGMENT_DELETED' AND entity_id = ?") + .bind("00000000-0000-4000-8000-000000000000") + .first<{ n: number }>(); + assert.equal(Number(none!.n), 0); +}); diff --git a/backend-ts/src/routes/segments.ts b/backend-ts/src/routes/segments.ts index c761f6b88..bbc7c5eb4 100644 --- a/backend-ts/src/routes/segments.ts +++ b/backend-ts/src/routes/segments.ts @@ -169,19 +169,23 @@ export async function handleSegments(req: Request, env: SegmentsEnv, actor: stri const overrideErr = validateOverrides(body.overrides); if (overrideErr) return bad(overrideErr); - // Does NOT audit first (#598), and cannot without a store change: `createSegment` mints the id and - // returns it, so there is nothing to key an event on beforehand. The fix is the same one - // `createTerminologyMapping` already has — mint the id caller-side — or ADR-073 d4's - // intent-then-completion pair. Either is a real change rather than a reorder. + // AUDIT BEFORE MUTATE (#598). The id is minted HERE rather than by the insert — the store now + // accepts one — so the event can name the segment before it exists. The payload's two values come + // from the REQUEST rather than from the created row: they are the same values the insert is about + // to store, and reading them off the result is what forced the old order. + const id = crypto.randomUUID(); + const name = body.name as string; + const measureIds = body.measureIds as string[]; + await audit(stores.events, "SEGMENT_CREATED", id, actor, { name, measureIds }); const created = await store.createSegment({ - name: body.name, + id, + name, description: typeof body.description === "string" ? body.description : undefined, enabled: typeof body.enabled === "boolean" ? body.enabled : undefined, rule: body.rule as SegmentRule, - measureIds: body.measureIds as string[], + measureIds, overrides: body.overrides as SegmentOverride[] | undefined, }); - await audit(stores.events, "SEGMENT_CREATED", created.id, actor, { name: created.name, measureIds: created.measureIds }); return json(created, 201); } @@ -203,23 +207,33 @@ export async function handleSegments(req: Request, env: SegmentsEnv, actor: stri const overrideErr = validateOverrides(body.overrides); if (overrideErr) return bad(overrideErr); - const patched = await store.updateSegment(putId, { + // AUDIT BEFORE MUTATE (#598), and it needed THREE writes moved, not one: `updateSegment` was + // followed by `setMeasures` and `setOverrides`, so a failure after the first left a partly-updated + // segment with no event at all. + // + // The 404 moved to an explicit pre-read. `updateSegment` returning null WAS the not-found signal, + // which is exactly what made the old order unavoidable: the route could not know the segment + // existed until it had already tried to change it. The pre-read leaves a window in which the row + // could vanish between the check and the write — an event for a change that then did not happen, + // which is the side #598's rule deliberately picks. + const before = await store.getSegment(putId); + if (!before) return json({ error: "not_found", message: `Segment not found: ${putId}` }, 404); + // The post-state, resolved from the pre-state and the request rather than from a re-read: the + // payload describes the segment the three writes below are about to produce. + await audit(stores.events, "SEGMENT_UPDATED", putId, actor, { + name: (body.name as string | undefined) ?? before.name, + enabled: (body.enabled as boolean | undefined) ?? before.enabled, + measureIds: (body.measureIds as string[] | undefined) ?? before.measureIds, + }); + await store.updateSegment(putId, { name: body.name as string | undefined, description: body.description as string | undefined, enabled: body.enabled as boolean | undefined, rule: body.rule as SegmentRule | undefined, }); - if (!patched) return json({ error: "not_found", message: `Segment not found: ${putId}` }, 404); if (body.measureIds !== undefined) await store.setMeasures(putId, body.measureIds as string[]); if (body.overrides !== undefined) await store.setOverrides(putId, body.overrides as SegmentOverride[]); - - const hydrated = await store.getSegment(putId); - await audit(stores.events, "SEGMENT_UPDATED", putId, actor, { - name: hydrated?.name, - enabled: hydrated?.enabled, - measureIds: hydrated?.measureIds, - }); - return json(hydrated); + return json(await store.getSegment(putId)); } // DELETE /api/segments/:id @@ -227,8 +241,10 @@ export async function handleSegments(req: Request, env: SegmentsEnv, actor: stri if (delId) { const seg = await store.getSegment(delId); if (!seg) return json({ error: "not_found", message: `Segment not found: ${delId}` }, 404); - await store.deleteSegment(delId); + // AUDIT BEFORE MUTATE (#598) — a plain reorder here: the id and the name both come from the row + // already read, so nothing the event needs is minted by the delete. await audit(stores.events, "SEGMENT_DELETED", delId, actor, { name: seg.name }); + await store.deleteSegment(delId); return new Response(null, { status: 204 }); } diff --git a/backend-ts/src/stores/evidence-store.ts b/backend-ts/src/stores/evidence-store.ts index 4ba354c5f..ae451e746 100644 --- a/backend-ts/src/stores/evidence-store.ts +++ b/backend-ts/src/stores/evidence-store.ts @@ -24,6 +24,13 @@ export interface InsertEvidenceInput { mimeType: string; storageKey: string; description: string | null; + /** + * The upload stamp. Optional and minted by the store when absent, so every existing caller is + * unchanged — but `uploadEvidence` passes one, because its audit payload carries this exact value as + * `payload.timestamp` and #598's rule is audit-before-mutate. It was the last of the + * "the store mints something the event needs" cases that a seam change could reach. + */ + uploadedAt?: string; } export interface EvidenceStore { diff --git a/backend-ts/src/stores/measure-store.ts b/backend-ts/src/stores/measure-store.ts index 9ec9d6263..788c17a78 100644 --- a/backend-ts/src/stores/measure-store.ts +++ b/backend-ts/src/stores/measure-store.ts @@ -48,6 +48,14 @@ export interface CreateMeasureInput { name: string; policyRef: string; owner: string; + /** + * Ids to insert under. Optional and minted by the store when absent, so every existing caller is + * unchanged — but `createMeasure` (`measure/measure-lifecycle.ts`) passes both, because its audit + * event is keyed on the VERSION id and #598's rule is audit-before-mutate. The two are separate + * columns (`measures.id`, `measure_versions.id`) and the event names the version. + */ + measureId?: string; + versionId?: string; } /** A lifecycle status change on a version (+ optional approver / activation stamp). */ diff --git a/backend-ts/src/stores/postgres/evidence-store-postgres.ts b/backend-ts/src/stores/postgres/evidence-store-postgres.ts index 9302e7bb7..817f4ed10 100644 --- a/backend-ts/src/stores/postgres/evidence-store-postgres.ts +++ b/backend-ts/src/stores/postgres/evidence-store-postgres.ts @@ -37,7 +37,7 @@ export class PgEvidenceStore implements EvidenceStore { constructor(private readonly pool: PgPool) {} async insert(input: InsertEvidenceInput): Promise { - const uploadedAt = new Date().toISOString(); + const uploadedAt = input.uploadedAt ?? new Date().toISOString(); await this.pool.query( `INSERT INTO ${SPIKE_SCHEMA}.evidence_attachments (id, case_id, uploaded_by, file_name, file_size_bytes, mime_type, storage_key, description, uploaded_at) diff --git a/backend-ts/src/stores/postgres/measure-store-postgres.ts b/backend-ts/src/stores/postgres/measure-store-postgres.ts index bef420547..5a2a45e4a 100644 --- a/backend-ts/src/stores/postgres/measure-store-postgres.ts +++ b/backend-ts/src/stores/postgres/measure-store-postgres.ts @@ -106,8 +106,8 @@ export class PgMeasureStore implements MeasureStore { } async createMeasure(input: CreateMeasureInput): Promise { - const measureId = crypto.randomUUID(); - const versionId = crypto.randomUUID(); + const measureId = input.measureId ?? crypto.randomUUID(); + const versionId = input.versionId ?? crypto.randomUUID(); await this.seedMeasure({ measureId, name: input.name, diff --git a/backend-ts/src/stores/postgres/segment-store-postgres.ts b/backend-ts/src/stores/postgres/segment-store-postgres.ts index 6db9fc44a..0ed5439eb 100644 --- a/backend-ts/src/stores/postgres/segment-store-postgres.ts +++ b/backend-ts/src/stores/postgres/segment-store-postgres.ts @@ -49,7 +49,7 @@ export class PgSegmentStore implements SegmentStore { } async createSegment(input: CreateSegmentInput): Promise { - const id = crypto.randomUUID(); + const id = input.id ?? crypto.randomUUID(); const now = new Date().toISOString(); await this.pool.query( `INSERT INTO ${S}.segments (id, name, description, enabled, rule_json, created_by, created_at, updated_at) diff --git a/backend-ts/src/stores/segment-store.ts b/backend-ts/src/stores/segment-store.ts index 5f1cf4f80..cc1aac93e 100644 --- a/backend-ts/src/stores/segment-store.ts +++ b/backend-ts/src/stores/segment-store.ts @@ -36,6 +36,13 @@ export interface HydratedSegment { } export interface CreateSegmentInput { + /** + * The id to insert under. Optional and minted by the store when absent, so every existing caller is + * unchanged — but the ROUTE passes one, because an event cannot name a row the store has not yet + * returned and #598's rule is audit-before-mutate. Same shape as `SubjectListStore.createList` and + * `createTerminologyMapping`. + */ + id?: string; name: string; description?: string; enabled?: boolean; diff --git a/backend-ts/src/stores/sqlite/evidence-store-sqlite.ts b/backend-ts/src/stores/sqlite/evidence-store-sqlite.ts index 94d1e8171..0df359d43 100644 --- a/backend-ts/src/stores/sqlite/evidence-store-sqlite.ts +++ b/backend-ts/src/stores/sqlite/evidence-store-sqlite.ts @@ -36,7 +36,7 @@ export class SqliteEvidenceStore implements EvidenceStore { constructor(private readonly db: CloudDatabase) {} async insert(input: InsertEvidenceInput): Promise { - const uploadedAt = new Date().toISOString(); + const uploadedAt = input.uploadedAt ?? new Date().toISOString(); await this.db .prepare( `INSERT INTO evidence_attachments diff --git a/backend-ts/src/stores/sqlite/measure-store-sqlite.ts b/backend-ts/src/stores/sqlite/measure-store-sqlite.ts index 74654756d..059cd0752 100644 --- a/backend-ts/src/stores/sqlite/measure-store-sqlite.ts +++ b/backend-ts/src/stores/sqlite/measure-store-sqlite.ts @@ -108,8 +108,8 @@ export class SqliteMeasureStore implements MeasureStore { } async createMeasure(input: CreateMeasureInput): Promise { - const measureId = crypto.randomUUID(); - const versionId = crypto.randomUUID(); + const measureId = input.measureId ?? crypto.randomUUID(); + const versionId = input.versionId ?? crypto.randomUUID(); const now = new Date().toISOString(); await this.seedMeasure({ measureId, diff --git a/backend-ts/src/stores/sqlite/segment-store-sqlite.ts b/backend-ts/src/stores/sqlite/segment-store-sqlite.ts index 1eed68016..e855ef535 100644 --- a/backend-ts/src/stores/sqlite/segment-store-sqlite.ts +++ b/backend-ts/src/stores/sqlite/segment-store-sqlite.ts @@ -49,7 +49,7 @@ export class SqliteSegmentStore implements SegmentStore { } async createSegment(input: CreateSegmentInput): Promise { - const id = crypto.randomUUID(); + const id = input.id ?? crypto.randomUUID(); const now = new Date().toISOString(); await this.db .prepare("INSERT INTO segments (id, name, description, enabled, rule_json, created_by, created_at, updated_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?)") diff --git a/docs/DATA_MODEL_CONTRACTS.md b/docs/DATA_MODEL_CONTRACTS.md index ec952eda5..b13faa793 100644 --- a/docs/DATA_MODEL_CONTRACTS.md +++ b/docs/DATA_MODEL_CONTRACTS.md @@ -137,35 +137,56 @@ SQLite floor and the Pg ceiling read the current row and apply the shared pure ` > transaction, and the patch follows. > > **Verified and flipped (2026-09-21, owner decision on #598):** measure approve, deprecate and the - > explicit status transition; terminology-mapping create; value-set attach and detach; **`grantWaiver` - > and `scheduleAppointment`**. Every one of them could flip for the same reason — the id is minted - > caller-side rather than by the insert, so the event can name the row before it exists. + > explicit status transition; terminology-mapping create; value-set attach and detach; `grantWaiver` + > and `scheduleAppointment`; and — with a one-field seam change each — **`createMeasure`**, **segment + > create, update and delete**, and **`uploadEvidence`**. Every one could flip for the same reason: + > the value the event keys on is minted caller-side rather than by the insert. `CreateMeasureInput`, + > `CreateSegmentInput` and `InsertEvidenceInput` now ACCEPT that value (optional, minted by the store + > when absent, so every other caller is unchanged), which is the change that made a reorder possible. + > **`uploadEvidence` audits before the BUCKET write too** — an object in storage the ledger never + > mentions is harder to notice than a missing row. Segment UPDATE needed three writes moved rather + > than one (`updateSegment`, `setMeasures`, `setOverrides`), and its 404 became an explicit pre-read: + > `updateSegment` returning null WAS the not-found signal, which is what made the old order + > unavoidable. > - > **Known to still mutate first, with the reason at each call site:** - > - the **run-created case transition** — **deliberate**, because the alternative strands an - > otherwise-complete run as RUNNING after the case was already mutated; - > - **`createMeasure`**, **segment create**, the three **identity-link** writes, and - > **`uploadEvidence`** — all of which share one cause: the store mints something the event needs, - > so there is nothing to key it on beforehand. For the first three that is the entity id; for - > evidence it is `record.uploadedAt`, which the event carries as `payload.timestamp`. The fix is - > to mint it caller-side (as `createTerminologyMapping` and now `grantWaiver` do) or ADR-073 d4's - > intent-then-completion pair — a real change rather than a reorder. - > **`uploadEvidence`'s exposure is wider than a row**: the BUCKET write lands first too, so a - > failed audit can leave an object in storage the ledger never mentions. + > **`src/audit/audit-order.test.ts` is what holds this**, and it exists because nothing did: every + > other test asserts the event EXISTS after a SUCCEEDING operation, which is equally true in either + > order, so a reorder back was silent. Each case makes the MUTATION fail and requires the event + > anyway — the only externally visible difference between the two orders. A new audit-first path + > belongs in it. > - > **Checked and NOT a violation:** panel assignment already audits before `upsertPanelAssignment` — - > the sweep's hit there was `activeCasesForSubjects`, a read. + > **Still mutate-first, with the reason at each call site:** + > - the **run-created case transition** and the **import-driven finalize** (`routes/runs.ts`) — + > **deliberate and the same pattern**: the event is best-effort at the run boundary, because the + > alternative strands an otherwise-complete run after its rows were already written. A failed + > write logs a run `WARN`; + > - **`dispatchOutreach`** — the only one whose ordering puts something OUTSIDE the system before the + > ledger: `channel.send()` dispatches the message, and the event payload is built from the delivery + > result (`status`, `messageId`, `provider`, `sentAt`), so there is nothing to record beforehand and + > nothing to retract afterwards. It needs ADR-073 d4's **intent-then-completion pair**, which adds + > an event type consumers read — an owner decision, not a reorder; + > - the three **identity-link** writes — and the reason is sharper than "the store mints the id": + > `upsertLink` returns the EXISTING row's id on conflict, so a caller-minted id is not the id the + > event would name. Keying these events on the PAIR — which IS known beforehand — would work, and + > changes what `entity_id` means for a consumer; + > - **`backfill-scale`** and **`backfill-quality-history`** — one-shot seeding tools rather than + > operator surfaces, each writing a run and its rows before a single completion event. Left as they + > are on purpose, and said here so the sweep's output does not read as untriaged. > - > **This is NOT a complete inventory, and two earlier versions of this paragraph wrongly implied it - > was.** `backend-ts/scripts/audit-order-sweep.py` lists every `await` preceding an audit write. - > Its output on the current tree still contains untriaged candidates — the import-driven finalize in - > `routes/runs.ts` most of all, plus `materialize-run`, `measure-seed`, `case-outreach` and - > `audit-packet`, several of which are probably reads. **#598 owns that triage**, and nothing should - > read this section as licence to close it. + > **Checked and NOT violations** — every one a hit the matcher produced for a read, a pure + > computation, or a DIFFERENT write's audit: `audit-packet` (`sha256Hex`), `materialize-run` and + > `backfill-trend-history` (reads), `evidence-service`'s download (`arrayBuffer`), `measure-seed` + > (`repairHypertensionSeedRow`, itself audit-first), `subject-lists` create (its audit is a + > `beforeComplete` callback that runs before the list becomes visible), and **panel assignment**, + > which audits before `upsertPanelAssignment` AND records each per-case event before `assignCases` — + > the mapping-then-consequences order is by design. > - > **Keep this list in step with the code in the SAME change.** The commit that flipped waivers and - > appointments left them listed here as untriaged, which pointed the next reader at work already - > done — caught in review, and exactly the failure mode an always-loaded file has. + > **The sweep's output is now fully triaged, and that is NOT the same as #598 being closed.** What + > remains is the missing PRIMITIVE (below) plus the two decisions above. + > `backend-ts/scripts/audit-order-sweep.py` lists every `await` preceding an audit write; re-run it + > and **keep this list in step with the code in the SAME change** — the commit that flipped waivers + > and appointments left them listed here as untriaged, which pointed the next reader at work already + > done. > > Two things the corrections taught, both worth keeping: > - **A function can be on BOTH sides.** `rerunToVerify` records its action audit-first and then diff --git a/docs/JOURNAL.md b/docs/JOURNAL.md index 2235f0d0e..a34e511c6 100644 --- a/docs/JOURNAL.md +++ b/docs/JOURNAL.md @@ -1,5 +1,55 @@ # Journal +## 2026-09-21 (late, III) — the audit-order sweep is fully triaged, and the rule finally has a test + +The owner's #598 decision was: where a path CAN audit before it mutates, it should. #607/#608 flipped +the ones that were a plain reorder. This finishes the sweep's output — every candidate opened, none +named from the tool's summary. + +**Flipped, each needing a one-field seam change rather than a reorder:** `createMeasure`, segment +create, segment UPDATE, segment delete, and `uploadEvidence`. `CreateMeasureInput`, +`CreateSegmentInput` and `InsertEvidenceInput` now ACCEPT the value the event keys on — optional, minted +by the store when absent, so every other caller is unchanged. That is the whole obstacle these five +shared: the store minted the id (or, for evidence, the `uploadedAt` the payload reports as +`payload.timestamp`), so there was nothing to key an event on beforehand. + +Two details worth keeping. **Segment UPDATE needed three writes moved, not one** — `updateSegment`, +`setMeasures`, `setOverrides` — so a failure after the first left a partly-updated segment with no event +at all; and its 404 became an explicit pre-read, because `updateSegment` returning null WAS the +not-found signal, which is what made the old order unavoidable. **`uploadEvidence` audits before the +BUCKET write too**: an object in storage the ledger never mentions is harder to notice than a missing +row. + +**`src/audit/audit-order.test.ts` exists because nothing tested the rule.** Nine call sites had been +flipped across three commits and not one test could tell: every existing test asserts the event EXISTS +after a SUCCEEDING operation, which is equally true in either order, so a reorder back was silent. Each +case now makes the MUTATION fail and requires the event anyway — the only externally visible difference +between the two orders. Mutation-checked on two of them. + +**Still mutate-first, and now every one has a reason at its call site:** +- the run-created case transition and the import-driven finalize — the same deliberate pattern, an + event best-effort at the run boundary, because the alternative strands an otherwise-complete run; +- **`dispatchOutreach`** — the only one that puts something OUTSIDE the system before the ledger. + `channel.send()` dispatches the message and the payload is built from the delivery result, so there + is nothing to record beforehand and nothing to retract after. Needs ADR-073 d4's intent-then-completion + pair, which adds an event type consumers read: an owner decision, not a reorder; +- the three identity-link writes — and the reason is sharper than "the store mints the id": + `upsertLink` returns the EXISTING row's id on conflict, so a caller-minted id is not the id the event + would name. Keying those events on the PAIR would work and changes what `entity_id` means; +- `backfill-scale` and `backfill-quality-history` — one-shot seeding tools, not operator surfaces. + +**Checked and NOT violations**, every one a matcher artifact: `audit-packet` (a hash), `materialize-run` +and `backfill-trend-history` (reads), evidence DOWNLOAD (`arrayBuffer`), `measure-seed` (itself +audit-first, flagged against a different write's audit), subject-list create (its audit is a +`beforeComplete` callback that runs before the list becomes visible), and panel assignment, which audits +before the mapping AND records each per-case event before `assignCases`. + +So the sweep's output is fully triaged — which is NOT the same as #598 closing. What remains is the +missing cross-store `applyCaseAction` primitive, plus the two decisions above. `DATA_MODEL_CONTRACTS` +§4 and CLAUDE.md now say exactly that. + +Backend 2,856 tests, one pre-existing local failure (`corpus-membership`). + ## 2026-09-21 (night) — four paths flipped to audit-first, and the list was still wrong by three Owner decision on #598: where a path can audit before it mutates, it should — the ledger errs toward From 311622fedd5186b8c98cdf8c3d150da021f2d6d7 Mon Sep 17 00:00:00 2001 From: Taleef Date: Mon, 21 Sep 2026 17:10:17 -0400 Subject: [PATCH 2/3] =?UTF-8?q?fix(audit):=20the=20review's=20corrections?= =?UTF-8?q?=20=E2=80=94=20a=20404=20I=20dropped,=20and=20three=20tests=20t?= =?UTF-8?q?hat=20could=20not=20fail?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of this branch found seven things. One is a regression this branch introduced; three are tests asserting a weaker property than their titles claim. **The PUT's relocated 404 dropped a guard that also protected the two later writes.** `updateSegment` returning null was the not-found signal, and moving the 404 to an explicit pre-read discarded it — so a row vanishing between the check and the write gave either a 500 (`setMeasures` violating the `segment_measures` foreign key) or an HTTP 200 whose body is `null`, where the old order returned a clean 404 for both. Two concurrent admin requests reach it. The return value is checked again, before the child writes. **All three new segments tests passed against the pre-change code.** The reason was a false claim in the test file's own header: that the route's ordering is unreachable because it resolves its stores from `env`. It is reachable — the store is a class, and patching its prototype makes a write fail against the real fixture. The DELETE case asserted only that the payload name came from a pre-read, which was true before the change too. Both cases now make the write fail and require the event to survive, and the PUT gets one for the vanished-row 404. **The event reported a measure list the row would never hold.** `setMeasures` writes `[...new Set(...)]` and `hydrate` reads back ordered, so a payload built from the request array named something the segment never contained. Harmless while the audit came second; a payload-accuracy regression once it comes first, in the direction #598 exists to close. **None of the three new seams was exercised by the store contract**, so the deployed Postgres ceiling was asserted nowhere: deleting `input.id ??` from the SQLite adapter failed a test, and the identical edit to the Pg adapter failed nothing. Three contract cases now, on both stores. **Four existing audit-first paths had no ordering test** despite §4 saying one belongs — `transitionStatus`, `createTerminologyMapping`, value-set attach and detach. **§4's completeness claim was wrong for the third time.** `backfill-trend-history` was filed under "not a violation (reads)" on the strength of two of its four hits; the other two are writes. `recover-stuck-runs`, `resolve-valuesets` and `batch-evaluate-scale` were missing altogether, and the PUT's own writes now surface as matcher artifacts against the DELETE's audit. Each time the prose was plausible and the arithmetic was not done — so §4 now carries the count, 55 hits across 20 files, plus the one-line command that re-derives it. Recorded rather than fixed: EVIDENCE_UPLOADED now reaches the case TIMELINE (`audit_events WHERE ref_case_id`), so a failed bucket write leaves a permanent "Evidence uploaded — " row with nothing to download. The rule picks the over-claim side for the ledger; whether a clinical-ops read surface should inherit it for a named file is an owner call, and §4 says so now. Backend 2,863 tests: 2,839 pass, 23 skip, 1 pre-existing local failure. Mutations killed: reverting the PUT ordering, dropping the null check, reverting the dedupe, and re-minting each of the three seam values — each failing only its own case. --- backend-ts/src/audit/audit-order.test.ts | 71 ++++++++++++++- backend-ts/src/routes/segments.test.ts | 109 ++++++++++++++++++----- backend-ts/src/routes/segments.ts | 29 +++++- backend-ts/src/stores/store-contract.ts | 62 +++++++++++++ docs/DATA_MODEL_CONTRACTS.md | 63 +++++++++---- docs/JOURNAL.md | 42 +++++++++ 6 files changed, 333 insertions(+), 43 deletions(-) diff --git a/backend-ts/src/audit/audit-order.test.ts b/backend-ts/src/audit/audit-order.test.ts index bde176476..eb13208b9 100644 --- a/backend-ts/src/audit/audit-order.test.ts +++ b/backend-ts/src/audit/audit-order.test.ts @@ -11,9 +11,11 @@ * the property the rule promises, it is the only externally visible difference between the two orders, * and reversing any one call site fails the corresponding case. * - * The route-level surfaces (segments, subject lists) resolve their stores from `env` rather than taking - * them injected, so their ordering is not reachable this way; `routes/segments.test.ts` pins the - * enabling half instead — that the audited entity id is the id the row is created under. + * The route-level surfaces resolve their stores from `env` rather than taking them injected, so they + * are not reachable THIS way — but they are reachable (`routes/segments.test.ts` patches the store's + * prototype to make a write fail, and asserts the same property). An earlier version of this comment + * said their ordering was not testable at all, and three cases were written to a weaker property as a + * result (#612 review). * * node --import tsx --test src/audit/audit-order.test.ts */ @@ -23,7 +25,8 @@ import type { AppendAuditInput } from "../stores/case-event-store.ts"; import { grantWaiver } from "../admin/waivers.ts"; import { scheduleAppointment } from "../case/appointment-service.ts"; import { uploadEvidence } from "../case/evidence-service.ts"; -import { createMeasure, approveMeasure, deprecateMeasure } from "../measure/measure-lifecycle.ts"; +import { createMeasure, approveMeasure, deprecateMeasure, transitionStatus } from "../measure/measure-lifecycle.ts"; +import { attachValueSet, detachValueSet, createTerminologyMapping } from "../measure/value-set-governance.ts"; /** A ledger that records what it was asked to append, in order. */ function ledger() { @@ -257,3 +260,63 @@ test("approveMeasure / deprecateMeasure: the event survives a failed status writ assert.deepEqual(log.types(), [label], `${label} is written before the status changes`); } }); + +/** + * The four audit-first paths flipped in #607/#608 that this file did not cover (#612 review). + * + * §4 says "a new audit-first path belongs in it", and four existing ones did not — so a reorder back on + * any of them was as silent as the five this file was written for. They are cheap to add because all + * four take their deps injected. + */ +test("transitionStatus: the event survives a failed status write", async () => { + const log = ledger(); + await assert.rejects( + () => + transitionStatus( + { measures: { getLatest: async () => MEASURE, setVersionStatus: explode } as never, events: log as never }, + "audiogram", + "Approved", + "approver", + ), + /the write failed/, + ); + assert.deepEqual(log.types(), ["MEASURE_VERSION_STATUS_CHANGED"]); +}); + +test("createTerminologyMapping: the event survives a failed insert, and names the id it would have made", async () => { + const log = ledger(); + await assert.rejects( + () => + createTerminologyMapping( + { valueSets: { createTerminologyMapping: explode } as never, events: log as never }, + { + localCode: "L1", localSystem: "urn:local", standardCode: "S1", standardSystem: "http://loinc.org", + localDisplay: null, standardDisplay: null, mappingStatus: null, mappingConfidence: null, notes: null, + }, + "admin", + ), + /the write failed/, + ); + assert.deepEqual(log.types(), ["TERMINOLOGY_MAPPING_CREATED"]); + assert.match(String(log.events[0]!.entityId), /^[0-9a-f-]{36}$/, "minted caller-side — the property that let it flip"); +}); + +test("attachValueSet / detachValueSet: the event survives a failed link write", async () => { + for (const [label, run] of [ + ["MEASURE_VALUE_SET_LINKED", (deps: never) => attachValueSet(deps, "audiogram", "vs-1", "admin")], + ["MEASURE_VALUE_SET_UNLINKED", (deps: never) => detachValueSet(deps, "audiogram", "vs-1", "admin")], + ] as const) { + const log = ledger(); + await assert.rejects( + () => + run({ + measures: { getLatest: async () => MEASURE } as never, + valueSets: { link: explode, unlink: explode } as never, + events: log as never, + } as never), + /the write failed/, + ); + assert.deepEqual(log.types(), [label]); + assert.equal(log.events[0]!.entityId, MEASURE.versionId, "keyed on the version the link belongs to"); + } +}); diff --git a/backend-ts/src/routes/segments.test.ts b/backend-ts/src/routes/segments.test.ts index 19022a20e..e0dac9fbc 100644 --- a/backend-ts/src/routes/segments.test.ts +++ b/backend-ts/src/routes/segments.test.ts @@ -11,6 +11,9 @@ import { rmSync } from "node:fs"; import { createSqliteD1 } from "@mieweb/cloud-local"; import { RUN_STORE_FLOOR_DDL } from "../stores/sqlite/schema.ts"; import { handleSegments } from "./segments.ts"; +// Patched per case to make a WRITE fail: the route resolves its own stores, so the prototype is the +// only seam — and it is enough to assert the ordering the comments claim. +import { SqliteSegmentStore } from "../stores/sqlite/segment-store-sqlite.ts"; const dbPath = join(tmpdir(), `workwell-segroute-${crypto.randomUUID()}.sqlite`); let env: { DB: unknown }; @@ -27,6 +30,18 @@ const getPreview = (id: string) => handleSegments(new Request(`http://x/api/segm const welderRule = { match: "ANY", conditions: [{ attr: "role", op: "contains", value: "Welder" }] }; +/** The audit rows the #598 cases read, so each assertion is one line rather than five. */ +const dbq = () => env.DB as { + prepare: (sql: string) => { bind: (...a: unknown[]) => { first: () => Promise } }; +}; +const eventCount = async (eventType: string, entityId: string): Promise => + Number((await dbq().prepare("SELECT COUNT(*) AS n FROM audit_events WHERE event_type = ? AND entity_id = ?").bind(eventType, entityId).first<{ n: number }>())!.n); +const eventPayload = async (eventType: string, entityId: string): Promise => + JSON.parse((await dbq().prepare("SELECT payload_json FROM audit_events WHERE event_type = ? AND entity_id = ? ORDER BY id DESC LIMIT 1").bind(eventType, entityId).first<{ payload_json: string }>())!.payload_json); +const countSegments = async (id: string): Promise => + Number((await dbq().prepare("SELECT COUNT(*) AS n FROM segments WHERE id = ?").bind(id).first<{ n: number }>())!.n); + + before(async () => { const db = await createSqliteD1(dbPath); await db.exec(RUN_STORE_FLOOR_DDL.replace(/\n/g, " ")); @@ -190,12 +205,16 @@ test("POST /api/segments/preview → 400 on a malformed rule (op/value shape)", }); /** - * The audit-first half of #598 for this route. The ORDER is not reachable from outside — the route - * resolves its stores from `env` rather than taking them injected, so no test here can make the insert - * fail and watch the event survive (`src/audit/audit-order.test.ts` does that for the injectable - * services). What IS reachable is the enabling change, and the way it could regress silently: mint an - * id for the event and let the store mint its own, leaving every SEGMENT_CREATED event pointing at a - * segment that never existed. + * The audit-first half of #598 for this route (#612 review). + * + * **The order IS reachable from here, and the first cut of this file said it was not.** The route + * resolves its stores from `env`, so it cannot be handed a failing fake — but the store is a class, and + * patching its prototype makes a write fail against the real fixture. Every case below that names an + * order now asserts one: reverting the route to write-then-audit fails them. Before that correction all + * three passed against the pre-change code, which is the shape of test this project keeps finding. + * + * The seam itself is covered separately and differently: `store-contract.ts` asserts the caller-minted + * id is honoured on BOTH stores, because a route test on the SQLite floor cannot speak for the ceiling. */ test("SEGMENT_CREATED names the id the segment is created under (#598)", async () => { const res = await post({ name: "Audit-order welders", rule: welderRule, measureIds: ["audiogram"] }); @@ -226,23 +245,71 @@ test("SEGMENT_UPDATED reports the post-state resolved from the pre-state and the assert.equal(payload.name, "After", "the requested change"); assert.deepEqual(payload.measureIds, ["audiogram"], "and the fields the request did not mention"); assert.equal(payload.enabled, true); + + // DEDUPED and ORDERED, because that is what the row holds: `setMeasures` writes a Set and `hydrate` + // reads back ordered. Reporting the request array verbatim made the event name a list the segment + // never contained — harmless while the audit came second, a payload-accuracy regression once it + // comes first (#612 review). + const dupes = (await (await post({ name: "Dupes", rule: welderRule, measureIds: ["tb_surveillance", "audiogram", "audiogram"] }))!.json()) as { id: string; measureIds: string[] }; + assert.deepEqual( + (await eventPayload("SEGMENT_CREATED", dupes.id) as { measureIds: string[] }).measureIds, + dupes.measureIds, + "the event's list is the row's list", + ); }); -test("DELETE audits before the row goes, and a missing segment is still a 404 with no event (#598)", async () => { +test("DELETE audits BEFORE the row goes — the event survives a failed delete (#598)", async () => { + // The only externally visible difference between the two orders, and what the first cut of this case + // did not test: it asserted the payload name came from a pre-read, which was true before the change + // too, so it passed against the code it was written to guard (#612 review). const created = (await (await post({ name: "Doomed", rule: welderRule, measureIds: [] }))!.json()) as { id: string }; - assert.equal((await del(created.id))?.status, 204); - const q = (env.DB as { prepare: (s: string) => { bind: (...a: unknown[]) => { first: () => Promise } } }); - const row = await q - .prepare("SELECT payload_json FROM audit_events WHERE event_type = 'SEGMENT_DELETED' AND entity_id = ?") - .bind(created.id) - .first<{ payload_json: string }>(); - assert.equal((JSON.parse(row!.payload_json) as { name: string }).name, "Doomed", "the name is read off the row before it is deleted"); - // An unknown id refuses before it audits — an over-claim is the side the rule picks, but only for a - // change somebody actually asked for. + const real = SqliteSegmentStore.prototype.deleteSegment; + SqliteSegmentStore.prototype.deleteSegment = async () => { throw new Error("the delete failed"); }; + try { + await assert.rejects(() => del(created.id) as Promise, /the delete failed/); + } finally { + SqliteSegmentStore.prototype.deleteSegment = real; + } + assert.equal(await eventCount("SEGMENT_DELETED", created.id), 1, "the event is there although the row is not gone"); + assert.equal((await countSegments(created.id)), 1, "and the segment really did survive"); + const payload = await eventPayload("SEGMENT_DELETED", created.id); + assert.equal((payload as { name: string }).name, "Doomed"); + + // An unknown id refuses before it audits — the over-claim is for a change somebody actually asked for. assert.equal((await del("00000000-0000-4000-8000-000000000000"))?.status, 404); - const none = await q - .prepare("SELECT COUNT(*) AS n FROM audit_events WHERE event_type = 'SEGMENT_DELETED' AND entity_id = ?") - .bind("00000000-0000-4000-8000-000000000000") - .first<{ n: number }>(); - assert.equal(Number(none!.n), 0); + assert.equal(await eventCount("SEGMENT_DELETED", "00000000-0000-4000-8000-000000000000"), 0); +}); + +test("PUT audits BEFORE its three writes, and a row that vanishes mid-update is a 404 (#598)", async () => { + // The PR's largest behaviour change — three writes moved and a relocated 404 — and the part that had + // no test of either property (#612 review). + const created = (await (await post({ name: "Before", rule: welderRule, measureIds: ["audiogram"] }))!.json()) as { id: string }; + const real = SqliteSegmentStore.prototype.updateSegment; + + // (a) the audit survives a failed update + SqliteSegmentStore.prototype.updateSegment = async () => { throw new Error("the update failed"); }; + try { + await assert.rejects(() => put(created.id, { name: "After" }) as Promise, /the update failed/); + } finally { + SqliteSegmentStore.prototype.updateSegment = real; + } + assert.equal(await eventCount("SEGMENT_UPDATED", created.id), 1); + assert.equal(((await (await getList())!.json()) as Array<{ id: string; name: string }>).find((x) => x.id === created.id)?.name, "Before", + "the row is unchanged, so the event is an over-claim — which is the side the rule picks"); + + // (b) the row VANISHING between the pre-read and the write is a clean 404, not a 500 and not a + // 200-with-null. `updateSegment` returning null is what says so, and dropping that check was a + // real regression: `setMeasures` then violates the segment_measures foreign key. + SqliteSegmentStore.prototype.updateSegment = async function vanished(this: SqliteSegmentStore, id: string) { + await real.call(this, id, {}); // keep the timestamp behaviour honest + await this.deleteSegment(id); // ...and then the row goes, as a concurrent DELETE would + return null; + } as typeof real; + try { + const res = await put(created.id, { name: "Racing", measureIds: ["hazwoper"] }); + assert.equal(res?.status, 404, "a vanished row is 404 — never a 500 from the child insert"); + assert.equal(((await res!.json()) as { error: string }).error, "not_found"); + } finally { + SqliteSegmentStore.prototype.updateSegment = real; + } }); diff --git a/backend-ts/src/routes/segments.ts b/backend-ts/src/routes/segments.ts index bbc7c5eb4..551a514bc 100644 --- a/backend-ts/src/routes/segments.ts +++ b/backend-ts/src/routes/segments.ts @@ -34,6 +34,16 @@ const json = (data: unknown, status = 200): Response => const bad = (message: string): Response => json({ error: "invalid_request", message }, 400); +/** + * The measure list AS THE ROW WILL HOLD IT — deduped and ordered. + * + * `setMeasures` inserts `[...new Set(measureIds)]` and `hydrate` reads back `ORDER BY measure_id ASC`, + * so a payload built from the request array can name a list the segment never contained. That did not + * matter while the audit came second (it reported the hydrated value); it does now that the event is + * written first, and it is a payload-accuracy regression in the direction #598 exists to close. + */ +const storedMeasureIds = (measureIds: readonly string[]): string[] => [...new Set(measureIds)].sort(); + /** Shared membership-preview projection used by BOTH preview surfaces (GET :id/preview + POST /preview) * so they can't drift: filter the directory through the canonical matchesCohort, return { count, members }. */ const previewResponse = (seg: HydratedSegment): Response => { @@ -176,7 +186,12 @@ export async function handleSegments(req: Request, env: SegmentsEnv, actor: stri const id = crypto.randomUUID(); const name = body.name as string; const measureIds = body.measureIds as string[]; - await audit(stores.events, "SEGMENT_CREATED", id, actor, { name, measureIds }); + // DEDUPED, because that is what the row will hold: `setMeasures` writes `[...new Set(...)]` and + // `hydrate` reads back ordered by `measure_id`. Reporting the request array verbatim made the event + // describe something the segment never contained — which is a payload-accuracy regression in the + // direction this whole change exists to close (review of #612). `storedMeasureIds` is the one place + // the two agree. + await audit(stores.events, "SEGMENT_CREATED", id, actor, { name, measureIds: storedMeasureIds(measureIds) }); const created = await store.createSegment({ id, name, @@ -223,14 +238,22 @@ export async function handleSegments(req: Request, env: SegmentsEnv, actor: stri await audit(stores.events, "SEGMENT_UPDATED", putId, actor, { name: (body.name as string | undefined) ?? before.name, enabled: (body.enabled as boolean | undefined) ?? before.enabled, - measureIds: (body.measureIds as string[] | undefined) ?? before.measureIds, + // Deduped for the same reason as the create above; `before.measureIds` is already stored form. + measureIds: body.measureIds === undefined ? before.measureIds : storedMeasureIds(body.measureIds as string[]), }); - await store.updateSegment(putId, { + // **`patched` is still checked, and dropping that check was a real regression** (review of #612). + // The pre-read covers the ordinary not-found; this null covers the row VANISHING between the two, + // and it guarded the two writes below as well. Without it a concurrent delete gave either a 500 + // (`setMeasures` violating the `segment_measures` foreign key) or an HTTP 200 whose body is + // `null` — a client doing `(await res.json()).id` gets a TypeError on a success. The old order + // returned a clean 404 for both, and the audit-first reorder must not cost that. + const patched = await store.updateSegment(putId, { name: body.name as string | undefined, description: body.description as string | undefined, enabled: body.enabled as boolean | undefined, rule: body.rule as SegmentRule | undefined, }); + if (!patched) return json({ error: "not_found", message: `Segment not found: ${putId}` }, 404); if (body.measureIds !== undefined) await store.setMeasures(putId, body.measureIds as string[]); if (body.overrides !== undefined) await store.setOverrides(putId, body.overrides as SegmentOverride[]); return json(await store.getSegment(putId)); diff --git a/backend-ts/src/stores/store-contract.ts b/backend-ts/src/stores/store-contract.ts index 3ec032aeb..915a286eb 100644 --- a/backend-ts/src/stores/store-contract.ts +++ b/backend-ts/src/stores/store-contract.ts @@ -2575,6 +2575,21 @@ export function measureStoreContract(label: string, freshStore: () => Promise { + const store = await freshStore(); + const measureId = crypto.randomUUID(); + const versionId = crypto.randomUUID(); + const created = await store.createMeasure({ name: "Caller-minted", policyRef: "POL-1", owner: "safety", measureId, versionId }); + // BOTH ids, because the event is keyed on the VERSION and the response carries the MEASURE. + assert.equal(created.measureId, measureId); + assert.equal(created.versionId, versionId, "the audit event names this one"); + assert.equal((await store.getLatest(measureId))?.versionId, versionId, "and the row is readable under it"); + const auto = await store.createMeasure({ name: "Store-minted", policyRef: "POL-2", owner: "safety" }); + assert.ok(auto.measureId && auto.measureId !== measureId); + assert.ok(auto.versionId && auto.versionId !== versionId); + }); + } /** Registers the EvidenceStore contract — metadata insert/list/get (bytes live in the BUCKET). */ @@ -2619,6 +2634,32 @@ export function evidenceStoreContract(label: string, freshStore: () => Promise { + const store = await freshStore(); + // The event's `payload.timestamp` IS this value, so a store that re-stamped would put the ledger + // and the attachment on different clocks — silently, since both would look like real timestamps. + const uploadedAt = "2026-03-04T05:06:07.000Z"; + const record = await store.insert({ + id: crypto.randomUUID(), + caseId: "case-stamp", + uploadedBy: "cm@x", + fileName: "stamped.pdf", + fileSizeBytes: 9, + mimeType: "application/pdf", + storageKey: "case-stamp/stamped.pdf", + description: null, + uploadedAt, + }); + assert.equal(record.uploadedAt, uploadedAt, "the returned record"); + assert.equal((await store.getById(record.id))!.uploadedAt, uploadedAt, "and the persisted column"); + const auto = await store.insert({ + id: crypto.randomUUID(), caseId: "case-stamp", uploadedBy: "cm@x", fileName: "auto.pdf", + fileSizeBytes: 9, mimeType: "application/pdf", storageKey: "case-stamp/auto.pdf", description: null, + }); + assert.ok(auto.uploadedAt && auto.uploadedAt !== uploadedAt, "omitted still stamps now"); + }); + } /** Registers the AppointmentStore contract — insert + newest-first list. */ @@ -2984,6 +3025,27 @@ export function segmentStoreContract(label: string, freshStore: () => Promise { + const store = await freshStore(); + const id = crypto.randomUUID(); + const created = await store.createSegment({ + id, + name: "Caller-minted", + rule: { match: "ANY", conditions: [] }, + measureIds: ["audiogram"], + overrides: [{ externalId: "emp-001", mode: "INCLUDE" }], + }); + assert.equal(created.id, id, "the row is created under the id the event already named"); + // And the id reached the CHILD writes too, not just the parent row. + const back = (await store.getSegment(id))!; + assert.deepEqual(back.measureIds, ["audiogram"]); + assert.deepEqual(back.overrides, [{ externalId: "emp-001", mode: "INCLUDE" }]); + // Omitted still mints one, so every other caller is unchanged. + const auto = await store.createSegment({ name: "Store-minted", rule: { match: "ANY", conditions: [] }, measureIds: [] }); + assert.ok(auto.id && auto.id !== id); + }); + } /** Registers the QualitySnapshotStore contract for one backend (#E16). `freshStore` → isolated, empty. */ diff --git a/docs/DATA_MODEL_CONTRACTS.md b/docs/DATA_MODEL_CONTRACTS.md index b13faa793..f393e4b7d 100644 --- a/docs/DATA_MODEL_CONTRACTS.md +++ b/docs/DATA_MODEL_CONTRACTS.md @@ -144,7 +144,14 @@ SQLite floor and the Pg ceiling read the current row and apply the shared pure ` > `CreateSegmentInput` and `InsertEvidenceInput` now ACCEPT that value (optional, minted by the store > when absent, so every other caller is unchanged), which is the change that made a reorder possible. > **`uploadEvidence` audits before the BUCKET write too** — an object in storage the ledger never - > mentions is harder to notice than a missing row. Segment UPDATE needed three writes moved rather + > mentions is harder to notice than a missing row. + > + > **And that over-claim reaches an OPERATOR surface, not only the ledger** (review of #612). The case + > timeline is `audit_events WHERE ref_case_id = ?`, so a failed bucket write or a failed insert now + > leaves a permanent "Evidence uploaded — " row on case detail with nothing in + > `listEvidence` and nothing to download. The rule picks the over-claim side for the LEDGER; whether + > a clinical-ops read surface should inherit it for a named file is a different question, and an + > owner one. Recorded rather than assumed, because the code comment argues only the storage side. Segment UPDATE needed three writes moved rather > than one (`updateSegment`, `setMeasures`, `setOverrides`), and its 404 became an explicit pre-read: > `updateSegment` returning null WAS the not-found signal, which is what made the old order > unavoidable. @@ -169,24 +176,50 @@ SQLite floor and the Pg ceiling read the current row and apply the shared pure ` > `upsertLink` returns the EXISTING row's id on conflict, so a caller-minted id is not the id the > event would name. Keying these events on the PAIR — which IS known beforehand — would work, and > changes what `entity_id` means for a consumer; - > - **`backfill-scale`** and **`backfill-quality-history`** — one-shot seeding tools rather than - > operator surfaces, each writing a run and its rows before a single completion event. Left as they - > are on purpose, and said here so the sweep's output does not read as untriaged. + > - **`backfill-scale`**, **`backfill-quality-history`** and **`backfill-trend-history`** — one-shot + > seeding tools rather than operator surfaces, each writing a run and its rows before a single + > completion event. Left as they are on purpose, and said here so the sweep's output does not read + > as untriaged. (`backfill-trend-history` was listed under "not a violation" one revision of this + > paragraph ago, on the strength of its two READ hits; it has two WRITE hits as well.) + > - **`recover-stuck-runs`** — `failStuckRuns` flips RUNNING→FAILED before `RUN_RECOVERED`. Same + > class as the run boundary above and for the same reason: the sweep exists so a stuck run does not + > stay visible as RUNNING, and losing the sweep to a failed audit write would defeat it. + > - **`resolve-valuesets`** (the CLI) — `upsertResolvedValueSet` before its audit, twice. A + > build-time tool, not a served surface. + > - **`batch-evaluate-scale`** — `finalizeRun` before `SCALE_EVALUATED`, best-effort with a `WARN`, + > and the comment at the call site says so: the run is already COMPLETED, so aborting would strand + > every remaining measure unfinalized. The run-boundary class again. > > **Checked and NOT violations** — every one a hit the matcher produced for a read, a pure - > computation, or a DIFFERENT write's audit: `audit-packet` (`sha256Hex`), `materialize-run` and - > `backfill-trend-history` (reads), `evidence-service`'s download (`arrayBuffer`), `measure-seed` - > (`repairHypertensionSeedRow`, itself audit-first), `subject-lists` create (its audit is a - > `beforeComplete` callback that runs before the list becomes visible), and **panel assignment**, - > which audits before `upsertPanelAssignment` AND records each per-case event before `assignCases` — - > the mapping-then-consequences order is by design. + > computation, or a DIFFERENT write's audit: `audit-packet` (`sha256Hex`), `materialize-run` (a read), + > `evidence-service`'s download (`arrayBuffer`), `measure-seed` (`repairHypertensionSeedRow`, itself + > audit-first), `subject-lists` create (its audit is a `beforeComplete` callback that runs before the + > list becomes visible), and **panel assignment**, which audits before `upsertPanelAssignment` AND + > records each per-case event before `assignCases` — the mapping-then-consequences order is by design. + > + > Two whole FILES are artifacts and always will be: `stores/postgres/case-event-store-postgres.ts` + > (the audit writer itself — its own `pool.query` calls match against its own audit statement) and + > `stores/store-contract.ts` (the test that drives them). + > + > **Every hit the sweep reports is accounted for above, and that is NOT the same as #598 being + > closed.** What remains is the missing PRIMITIVE (below) plus the two decisions above. + > + > **Two things a re-run will show that are NOT new work.** `outcome-compaction`'s hit is the + > COMPLETION event of ADR-073 d4's intent/completion pair — the intent is written before the delete, + > which is the rule satisfied rather than broken. And `routes/segments.ts` still reports three hits + > for the PUT's writes, matched against the DELETE's audit further down the file and attributed to a + > local helper: that route is audit-first, and those are matcher artifacts. > - > **The sweep's output is now fully triaged, and that is NOT the same as #598 being closed.** What - > remains is the missing PRIMITIVE (below) plus the two decisions above. > `backend-ts/scripts/audit-order-sweep.py` lists every `await` preceding an audit write; re-run it - > and **keep this list in step with the code in the SAME change** — the commit that flipped waivers - > and appointments left them listed here as untriaged, which pointed the next reader at work already - > done. + > and **keep this list in step with the code in the SAME change**. + > + > **CHECK THE COUNT, NOT THE LABELS.** On 2026-09-21 the sweep reports **55 hits across 20 files**, + > and every file above is one of those 20. This paragraph has claimed completeness three times and + > been wrong three times: first by implying an inventory it did not have; then by filing two of + > `backfill-trend-history`'s four hits under "reads" because the other two were; then by leaving + > `batch-evaluate-scale` out altogether while listing its two siblings. Each time the prose was + > plausible and the arithmetic was not done. `python scripts/audit-order-sweep.py | sed 's/:.*//' | + > sort -u` is the check, and it takes a second. > > Two things the corrections taught, both worth keeping: > - **A function can be on BOTH sides.** `rerunToVerify` records its action audit-first and then diff --git a/docs/JOURNAL.md b/docs/JOURNAL.md index a34e511c6..9b3851643 100644 --- a/docs/JOURNAL.md +++ b/docs/JOURNAL.md @@ -50,6 +50,48 @@ missing cross-store `applyCaseAction` primitive, plus the two decisions above. ` Backend 2,856 tests, one pre-existing local failure (`corpus-membership`). +**Review round (#612).** Seven findings, one of them a regression I introduced and three of them tests +that could not fail. + +**The PUT's relocated 404 dropped a guard that also protected the two later writes.** `updateSegment` +returning null was the not-found signal, and moving the 404 to a pre-read discarded it — so a row +vanishing between the check and the write gave either a **500** (`setMeasures` violating the +`segment_measures` foreign key) or an **HTTP 200 whose body is `null`**, where the old order returned a +clean 404 for both. Reachable by two concurrent admin requests. The return value is checked again. + +**All three new segments tests passed against the pre-change code**, and the reason was a false claim in +my own header: that the route's ordering is unreachable because it resolves its stores from `env`. It is +reachable — the store is a class, and patching its prototype makes a write fail against the real +fixture. The DELETE case asserted only that the payload name came from a pre-read, which was true +before too. Both now make the write fail and require the event to survive, and the PUT gets a case for +the vanished-row 404. Mutation-confirmed: reverting the PUT to write-then-audit, dropping the null +check, and reverting the dedupe each fail exactly one case. + +**The event reported a measure list the row would never hold.** `setMeasures` writes `[...new Set(...)]` +and `hydrate` reads back ordered, so a payload built from the request array named something the segment +never contained — harmless while the audit came second, a payload-accuracy regression once it comes +first. One helper, `storedMeasureIds`. + +**None of the three new seams was exercised by the store contract**, so the deployed Postgres ceiling was +asserted nowhere: deleting `input.id ??` from the SQLite adapter failed a test, and the identical edit to +the Pg adapter failed nothing. Three contract cases now, which run on both stores. + +**Four existing audit-first paths had no ordering test** despite §4 saying one belongs — `transitionStatus`, +`createTerminologyMapping`, and value-set attach/detach. Added. + +**And §4's completeness claim was wrong for the third time.** `backfill-trend-history` was filed under +"not a violation (reads)" on the strength of two of its four hits; the other two are writes. +`recover-stuck-runs`, `resolve-valuesets` and `batch-evaluate-scale` were absent altogether, and the +PUT's own writes now appear as matcher artifacts against the DELETE's audit. Every time the prose was +plausible and the arithmetic was not done, so §4 now carries the **count** — 55 hits across 20 files — +and the one-line command that re-derives it. + +One thing recorded rather than fixed: **`EVIDENCE_UPLOADED` now reaches an operator surface.** The case +timeline is `audit_events WHERE ref_case_id = ?`, so a failed bucket write leaves a permanent "Evidence +uploaded — " row with nothing to download. The rule picks the over-claim side for the LEDGER; +whether a clinical-ops read should inherit it for a named file is an owner call. + + ## 2026-09-21 (night) — four paths flipped to audit-first, and the list was still wrong by three Owner decision on #598: where a path can audit before it mutates, it should — the ledger errs toward From a6a793e1b60ecd54d340c245a01812bc6f587b63 Mon Sep 17 00:00:00 2001 From: Taleef Date: Mon, 21 Sep 2026 17:35:35 -0400 Subject: [PATCH 3/3] fix(audit): SEGMENT_UPDATED reports what the request changes, not a guessed post-state (Codex, #612) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merging the request over a pre-read produced a post-state, and under concurrency that post-state is a guess: read `enabled: true`, let another admin set it false, change only the name, and `updateSegment` preserves the newer false while the event reports true. The post-write hydration this replaced could not be wrong about it, because it re-read — and an audit-first event cannot re-read. So the payload is now what THIS REQUEST changes: each field the body supplies, plus a `changed` list naming exactly that set, and nothing about the fields it does not set. `measureIds` stays deduped, because that is what the row will hold. A consumer wanting the resulting state reads the row; what the ledger is for is who changed what, and every value here is knowable before the write and true after it. Pinned by a deterministic race rather than an argument: another writer flips `enabled` between this request's pre-read and its write, and the assertions are that the event says nothing about `enabled` and that the other writer's value survives — which is what the merged form would have mis-reported. Codex's other two findings on this PR were already closed by the previous commit: rechecking `updateSegment`'s result, and §4's incomplete audit-order triage. Backend 2,864 tests: 2,840 pass, 23 skip, 1 pre-existing local failure. Mutation-checked: restoring the merged payload fails the race case. --- backend-ts/src/routes/segments.test.ts | 67 ++++++++++++++++++-------- backend-ts/src/routes/segments.ts | 30 ++++++++---- docs/JOURNAL.md | 13 +++++ 3 files changed, 79 insertions(+), 31 deletions(-) diff --git a/backend-ts/src/routes/segments.test.ts b/backend-ts/src/routes/segments.test.ts index e0dac9fbc..0b0c708ef 100644 --- a/backend-ts/src/routes/segments.test.ts +++ b/backend-ts/src/routes/segments.test.ts @@ -229,33 +229,58 @@ test("SEGMENT_CREATED names the id the segment is created under (#598)", async ( assert.equal((JSON.parse(row!.payload_json) as { name: string }).name, "Audit-order welders"); }); -test("SEGMENT_UPDATED reports the post-state resolved from the pre-state and the request (#598)", async () => { - // The payload used to come from a re-read AFTER the three writes, which is what forced the audit to - // come second. It is now merged from the row as it was plus the fields the request carries — so a - // PARTIAL body must still report the unchanged fields, not drop them. +test("SEGMENT_UPDATED reports what THIS REQUEST changes, not the resulting state (#598)", async () => { + // The payload came from a re-read AFTER the three writes, which is what forced the audit to come + // second. The first audit-first cut replaced that with a pre-read merged under the request — a + // post-state GUESS, and wrong under concurrency (Codex on #612): read `enabled: true`, let another + // admin set it false, change only the name, and `updateSegment` preserves the newer false while the + // event says true. An audit-first event cannot re-read, so it must not claim the parts it does not + // set. const created = (await (await post({ name: "Before", rule: welderRule, measureIds: ["audiogram"] }))!.json()) as { id: string }; - const res = await put(created.id, { name: "After" }); // name only: measureIds and enabled are untouched - assert.equal(res?.status, 200); - const row = await (env.DB as { prepare: (s: string) => { bind: (...a: unknown[]) => { first: () => Promise } } }) - .prepare("SELECT payload_json FROM audit_events WHERE event_type = 'SEGMENT_UPDATED' AND entity_id = ?") - .bind(created.id) - .first<{ payload_json: string }>(); - assert.ok(row); - const payload = JSON.parse(row!.payload_json) as { name: string; enabled: boolean; measureIds: string[] }; - assert.equal(payload.name, "After", "the requested change"); - assert.deepEqual(payload.measureIds, ["audiogram"], "and the fields the request did not mention"); - assert.equal(payload.enabled, true); - - // DEDUPED and ORDERED, because that is what the row holds: `setMeasures` writes a Set and `hydrate` - // reads back ordered. Reporting the request array verbatim made the event name a list the segment - // never contained — harmless while the audit came second, a payload-accuracy regression once it - // comes first (#612 review). + assert.equal((await put(created.id, { name: "After" }))?.status, 200); // name only + const payload = (await eventPayload("SEGMENT_UPDATED", created.id)) as Record; + assert.equal(payload.name, "After", "the field the request supplies"); + assert.deepEqual(payload.changed, ["name"], "and `changed` names exactly that set"); + for (const untouched of ["enabled", "measureIds", "rule", "description", "overrides"]) { + assert.ok(!(untouched in payload), `${untouched} is ABSENT — the request said nothing about it`); + } + + // The concurrency case itself, made deterministic: another writer flips `enabled` between this + // request's pre-read and its write. The event must not have claimed a value for it. + const raced = (await (await post({ name: "Raced", rule: welderRule, measureIds: [] }))!.json()) as { id: string }; + const realGet = SqliteSegmentStore.prototype.getSegment; + let flipped = false; + SqliteSegmentStore.prototype.getSegment = async function racy(this: SqliteSegmentStore, id: string) { + const row = await realGet.call(this, id); + if (!flipped && id === raced.id) { + flipped = true; + await realGet.call(this, id); // read-through, then the "other admin" writes + await SqliteSegmentStore.prototype.updateSegment.call(this, id, { enabled: false }); + } + return row; + } as typeof realGet; + try { + assert.equal((await put(raced.id, { name: "Renamed" }))?.status, 200); + } finally { + SqliteSegmentStore.prototype.getSegment = realGet; + } + const racedPayload = (await eventPayload("SEGMENT_UPDATED", raced.id)) as Record; + assert.ok(!("enabled" in racedPayload), "the event says nothing about a field it did not set"); + const after = ((await (await getList())!.json()) as Array<{ id: string; enabled: boolean }>).find((x) => x.id === raced.id); + assert.equal(after?.enabled, false, "and the other writer's value survived, which is what the old merge would have mis-reported"); +}); + +test("SEGMENT_CREATED reports the measure list the ROW will hold, deduped and ordered (#598)", async () => { + // `setMeasures` writes a Set and `hydrate` reads back ordered, so the request array can name a list + // the segment never contained. Harmless while the audit came second; a payload-accuracy regression + // once it comes first (#612 review). const dupes = (await (await post({ name: "Dupes", rule: welderRule, measureIds: ["tb_surveillance", "audiogram", "audiogram"] }))!.json()) as { id: string; measureIds: string[] }; assert.deepEqual( - (await eventPayload("SEGMENT_CREATED", dupes.id) as { measureIds: string[] }).measureIds, + ((await eventPayload("SEGMENT_CREATED", dupes.id)) as { measureIds: string[] }).measureIds, dupes.measureIds, "the event's list is the row's list", ); + assert.deepEqual(dupes.measureIds, ["audiogram", "tb_surveillance"], "and not vacuous: the request sent three, deduped"); }); test("DELETE audits BEFORE the row goes — the event survives a failed delete (#598)", async () => { diff --git a/backend-ts/src/routes/segments.ts b/backend-ts/src/routes/segments.ts index 551a514bc..57effde26 100644 --- a/backend-ts/src/routes/segments.ts +++ b/backend-ts/src/routes/segments.ts @@ -231,16 +231,26 @@ export async function handleSegments(req: Request, env: SegmentsEnv, actor: stri // existed until it had already tried to change it. The pre-read leaves a window in which the row // could vanish between the check and the write — an event for a change that then did not happen, // which is the side #598's rule deliberately picks. - const before = await store.getSegment(putId); - if (!before) return json({ error: "not_found", message: `Segment not found: ${putId}` }, 404); - // The post-state, resolved from the pre-state and the request rather than from a re-read: the - // payload describes the segment the three writes below are about to produce. - await audit(stores.events, "SEGMENT_UPDATED", putId, actor, { - name: (body.name as string | undefined) ?? before.name, - enabled: (body.enabled as boolean | undefined) ?? before.enabled, - // Deduped for the same reason as the create above; `before.measureIds` is already stored form. - measureIds: body.measureIds === undefined ? before.measureIds : storedMeasureIds(body.measureIds as string[]), - }); + if (!(await store.getSegment(putId))) return json({ error: "not_found", message: `Segment not found: ${putId}` }, 404); + // **The payload is what THIS REQUEST CHANGES, not the resulting state** (Codex, #612). + // + // The first cut merged the request over a pre-read, so it reported a post-state — and under two + // concurrent admins that post-state is a guess: read `enabled: true`, let the other request set it + // false, change only the name, and `updateSegment` preserves the newer false while the event says + // true. The old post-write hydration could not be wrong about that, because it re-read; an + // audit-first event cannot re-read, so it must not claim the parts it does not set. + // + // A consumer wanting the resulting state reads the row. What the ledger is FOR is who changed what, + // and every field here is one this request supplies — knowable before the write and true after it. + const changes: Record = {}; + if (body.name !== undefined) changes.name = body.name; + if (body.description !== undefined) changes.description = body.description; + if (body.enabled !== undefined) changes.enabled = body.enabled; + if (body.rule !== undefined) changes.rule = body.rule; + // Deduped for the same reason as the create above: it is what the row will hold. + if (body.measureIds !== undefined) changes.measureIds = storedMeasureIds(body.measureIds as string[]); + if (body.overrides !== undefined) changes.overrides = body.overrides; + await audit(stores.events, "SEGMENT_UPDATED", putId, actor, { changed: Object.keys(changes).sort(), ...changes }); // **`patched` is still checked, and dropping that check was a real regression** (review of #612). // The pre-read covers the ordinary not-found; this null covers the row VANISHING between the two, // and it guarded the two writes below as well. Without it a concurrent delete gave either a 500 diff --git a/docs/JOURNAL.md b/docs/JOURNAL.md index 9b3851643..c0e6123a0 100644 --- a/docs/JOURNAL.md +++ b/docs/JOURNAL.md @@ -91,6 +91,19 @@ timeline is `audit_events WHERE ref_case_id = ?`, so a failed bucket write leave uploaded — " row with nothing to download. The rule picks the over-claim side for the LEDGER; whether a clinical-ops read should inherit it for a named file is an owner call. +**Codex (#612).** Three findings; two were already closed by the round above (the recheck of +`updateSegment`'s result, and §4's incomplete triage). The third was not. + +**The audit payload was a post-state GUESS.** Merging the request over a pre-read is wrong under +concurrency: read `enabled: true`, let another admin set it false, change only the name, and +`updateSegment` preserves the newer false while the event says true. The old post-write hydration could +not be wrong about that because it re-read — and an audit-first event cannot re-read. So the payload is +now **what this request CHANGES**: every field the body supplies, plus a `changed` list naming them, and +nothing about the fields it does not set. A consumer wanting the resulting state reads the row; what the +ledger is for is who changed what. Pinned by a deterministic race — another writer flips `enabled` +between the pre-read and the write, and the event must say nothing about it. + + ## 2026-09-21 (night) — four paths flipped to audit-first, and the list was still wrong by three