From c9e353782a44067176674e8a9d76e97903c86ea6 Mon Sep 17 00:00:00 2001 From: Taleef Date: Mon, 21 Sep 2026 11:24:15 -0400 Subject: [PATCH 1/6] =?UTF-8?q?fix(qicore):=20preparation=20supplies=20a?= =?UTF-8?q?=20system,=20never=20a=20code=20=E2=80=94=20and=20the=20importe?= =?UTF-8?q?r=20says=20what=20the=20document=20says?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #594. The 2026-09-07 review's one high-priority correctness defect, accepted 2026-09-08 and untouched since. prepareForQiCore filled four coded fields when it could not bind them: Condition.clinicalStatus, verificationStatus, category and Encounter.class. Its guard, unbindable(), is true of a MISSING field as well as a present-but-unbindable one, and both branches assigned a module default. Two consequences. First, an absent field was invented — live rather than latent, because the QRDA-I import emits none of those three on a Condition and no class on an Encounter, so preparation minted them on a third party's document, including stamping `active` on a Condition the importer had just given an abatementDateTime from a closed interval. Second, and worse than the issue recorded: a PRESENT code was discarded. unbindable() is as true of a system-less `resolved` as of a system-less `active`, so a corrected misdiagnosis was reported as an active, confirmed problem — that patient enters CMS122's denominator and, with no HbA1c, its numerator. The file's own docstring claimed this hole was closed. So: this layer supplies a SYSTEM, never a CODE. Normalize only when a value is present, cannot bind, and carries a code from that field's own value set, writing that same code back. Absent stays absent; bindable is untouched; an unrecognised code is left alone, because we cannot claim to know which system it came from. The mapping moves to where the source semantics are known. times() now reports three states rather than two, because collapsing them made a faithful mapping impossible however it was written: a with a value closes the interval (resolved, beside its own abatementDateTime), a is QDM open prevalence (active), and an absent is silence and emits nothing. Nothing the pilot reports moves: the ADR-075 corpus records all four fields itself, fully systemed, with category distinguishing an encounter diagnosis from a problem-list item — and cms122/125/2/137 still find real populations. Fixtures are adversarial per the review's bar: refuted, resolved, entered-in-error, an unrecognised code, an inpatient class, and the three interval shapes. Four mutations killed, including "code discarded, default substituted" — the first cut of this fix turned Encounter {code:"IMP"} into ambulatory and an existing test caught it. --- backend-ts/src/fhir/qrda1-import.test.ts | 78 +++++++++++ backend-ts/src/fhir/qrda1-import.ts | 47 ++++++- .../src/wiring/qicore-preparation.test.ts | 79 ++++++++++-- backend-ts/src/wiring/qicore-preparation.ts | 122 +++++++++++++++--- 4 files changed, 298 insertions(+), 28 deletions(-) diff --git a/backend-ts/src/fhir/qrda1-import.test.ts b/backend-ts/src/fhir/qrda1-import.test.ts index a41bb40b2..8b15cc635 100644 --- a/backend-ts/src/fhir/qrda1-import.test.ts +++ b/backend-ts/src/fhir/qrda1-import.test.ts @@ -832,3 +832,81 @@ test("import: a birthTime with an OFFSET keeps its own calendar day, not the UTC .find((r) => r.resourceType === "Patient")!; assert.equal(patient.birthDate, "2000-01-01", "the day the document states, not the UTC-normalized one"); }); + +// --------------------------------------------------------------------------------------------------- +// clinicalStatus comes from what the document SAYS about the interval (#594) +// +// Until 2026-09-21 this file emitted no `clinicalStatus` and `prepareForQiCore` minted `active` for +// every imported Condition - including one carrying an `abatementDateTime` that this importer had +// just written from a closed interval. Two files in one pipeline disagreed about identical bytes: +// `qdm-entries.ts` honours a system-less `entered-in-error` as a negation while preparation rewrote +// it to `confirmed`. +// +// The three cases below are the three things a QRDA-I `` can say, and they must not +// collapse into one. Each asserts a DIFFERENT result, which is the bar #594 set: a fixture that +// cannot change the answer cannot tell a correct mapping from the previous one. +// --------------------------------------------------------------------------------------------------- + +const diagnosisDocument = (effectiveTime: string) => ` + + +
+ + + + + ${effectiveTime} + + +
+`; + +const conditionFromDoc = (effectiveTime: string) => { + const entries = importQrda1Document(diagnosisDocument(effectiveTime)).bundle.entry; + return entries + .map((e) => e.resource as { resourceType?: string; clinicalStatus?: unknown; abatementDateTime?: string }) + .find((r) => r.resourceType === "Condition"); +}; + +const CLINICAL = "http://terminology.hl7.org/CodeSystem/condition-clinical"; + +test("import: a CLOSED interval is resolved, and says so beside its own abatement date", () => { + const condition = conditionFromDoc(``); + assert.ok(condition, "the diagnosis imported"); + assert.equal(condition!.abatementDateTime, "2024-06-30", "the interval ended, which this already recorded"); + // The contradiction #594 named: preparation used to stamp `active` on exactly this resource. + assert.deepEqual(condition!.clinicalStatus, { coding: [{ system: CLINICAL, code: "resolved" }] }); +}); + +test("import: high nullFlavor=UNK is open prevalence, so active is a faithful reading", () => { + const condition = conditionFromDoc( + ``, + ); + assert.ok(condition); + assert.equal(condition!.abatementDateTime, undefined, "no end date, because none is known"); + // The source made an explicit assertion - there is no known end - and that is what licenses this. + assert.deepEqual(condition!.clinicalStatus, { coding: [{ system: CLINICAL, code: "active" }] }); +}); + +test("import: no high element at all is SILENCE, and nothing is asserted", () => { + const condition = conditionFromDoc(``); + assert.ok(condition); + assert.equal(condition!.abatementDateTime, undefined); + // The whole of #594 in one assertion. An absent element and `nullFlavor="UNK"` both yield no end + // date; only one of them is a statement about the patient. The QI-Core profile goes unsatisfied and + // the condition is not retrieved, which is the honest outcome for a document that did not say. + assert.equal(condition!.clinicalStatus, undefined, "not active, not resolved - absent"); +}); + +test("import: the three interval shapes produce three DIFFERENT statuses", () => { + // Guards the collapse itself rather than each case: `times()` returned no end for both the + // nullFlavor and the absent form, so any mapping built on its output alone could not separate them + // however it was written. + const statuses = [ + ``, + ``, + ``, + ].map((t) => JSON.stringify(conditionFromDoc(t)?.clinicalStatus ?? null)); + assert.equal(new Set(statuses).size, 3, `expected three distinct statuses, got ${statuses.join(" | ")}`); +}); diff --git a/backend-ts/src/fhir/qrda1-import.ts b/backend-ts/src/fhir/qrda1-import.ts index c1cf0c277..af5ac41d5 100644 --- a/backend-ts/src/fhir/qrda1-import.ts +++ b/backend-ts/src/fhir/qrda1-import.ts @@ -191,11 +191,23 @@ function concept(node: CdaNode | undefined): { coding: Array<{ system: string; c } /** `` → `{ point, start, end }` in ISO, whichever the element expresses. */ -function times(node: CdaNode | undefined): { point?: string; start?: string; end?: string } { +function times(node: CdaNode | undefined): { point?: string; start?: string; end?: string; endUnknown?: boolean } { if (!node) return {}; const point = isoFromHl7(node.attrs.value); if (point) return { point }; - return { start: isoFromHl7(child(node, "low")?.attrs.value), end: isoFromHl7(child(node, "high")?.attrs.value) }; + const high = child(node, "high"); + const end = isoFromHl7(high?.attrs.value); + // THREE states, not two (#594). An absent `` and a `` both yield no + // end date, and collapsing them loses the only thing that says whether the source made an + // assertion: `nullFlavor="UNK"` is an explicit "this has not ended, and I do not know when it + // will" - QDM open prevalence - while an absent element is silence. One licenses reporting the + // condition as active; the other licenses nothing. + const endUnknown = high !== undefined && end === undefined; + return { + start: isoFromHl7(child(node, "low")?.attrs.value), + end, + ...(endUnknown ? { endUnknown: true } : {}), + }; } /** @@ -233,15 +245,46 @@ function encounterFrom(node: CdaNode, i: string): unknown { }; } +const CONDITION_CLINICAL = "http://terminology.hl7.org/CodeSystem/condition-clinical"; + +/** + * The clinical status this document actually asserts, or nothing (#594). + * + * Until 2026-09-21 no `clinicalStatus` was emitted here at all and `prepareForQiCore` minted `active` + * for every imported Condition - including one carrying an `abatementDateTime`, i.e. one this very + * function had just recorded as ended. Preparation may no longer invent it, so the mapping belongs + * where the source semantics are known, which is here. + * + * Derived from the effective time and nothing else, because that is the only thing a QRDA-I Diagnosis + * entry says about it: + * - a `` with a real value closes the interval, so the condition ENDED - `resolved`, matching + * the `abatementDateTime` emitted from the same value; + * - a `` is QDM open prevalence: the source states there is no known end, so + * `active` is a faithful reading of an explicit assertion; + * - no `` at all is SILENCE, and silence authorizes nothing. The field is omitted, the QI-Core + * profile is unsatisfied, and the condition is not retrieved - the honest outcome for a document + * that did not say. + * + * The system is written here rather than left for preparation to add, so the value this file emits is + * bindable on its own terms. + */ +function clinicalStatusFrom(t: { end?: string; endUnknown?: boolean }): unknown { + if (t.end) return { coding: [{ system: CONDITION_CLINICAL, code: "resolved" }] }; + if (t.endUnknown) return { coding: [{ system: CONDITION_CLINICAL, code: "active" }] }; + return undefined; +} + function conditionFrom(node: CdaNode, i: string): unknown { const t = times(child(node, "effectiveTime")); // The patient's condition is the VALUE; `` says only "this entry is a diagnosis". const code = concept(child(node, "value")); if (!code) return undefined; + const clinicalStatus = clinicalStatusFrom(t); return { resourceType: "Condition", id: idOf(node, `qrda1-condition-${i}`), verificationStatus: { coding: [{ code: "confirmed" }] }, + ...(clinicalStatus ? { clinicalStatus } : {}), code, ...(t.start ?? t.point ? { onsetDateTime: t.start ?? t.point } : {}), ...(t.end ? { abatementDateTime: t.end } : {}), diff --git a/backend-ts/src/wiring/qicore-preparation.test.ts b/backend-ts/src/wiring/qicore-preparation.test.ts index 120fa9152..bb923a513 100644 --- a/backend-ts/src/wiring/qicore-preparation.test.ts +++ b/backend-ts/src/wiring/qicore-preparation.test.ts @@ -24,19 +24,64 @@ const bundleWith = (...resources: Array>): PreparableBun entry: resources.map((resource) => ({ resource })), }); -test("a Condition gets the status QI-Core binds - and no invented onset", () => { +test("an ABSENT status stays absent - nothing is invented (#594)", () => { + // This test asserted the opposite until 2026-09-21: a Condition with no `clinicalStatus` was given + // `active`, no `verificationStatus` became `confirmed`, no `category` became a problem-list item. + // That is fabrication (ADR-037 forbids it here) and it was LIVE, not latent - the QRDA-I import + // emits none of those three, so preparation minted all three on a third party document. const bundle = bundleWith({ resourceType: "Condition", id: "c1" }); prepareForQiCore(bundle); const condition = bundle.entry[0]!.resource; - assert.deepEqual(condition.clinicalStatus, { - coding: [{ system: "http://terminology.hl7.org/CodeSystem/condition-clinical", code: "active" }], - }); + assert.equal(condition.clinicalStatus, undefined, "the source said nothing; so does the prepared bundle"); + assert.equal(condition.verificationStatus, undefined); + assert.equal(condition.category, undefined); // An onset date is the date of a real event. CMS165 gates denominator membership on hypertension // onset relative to the measurement period, so minting one here would decide who is in the measure. - // Measured, it also buys nothing: status alone already yields IPP=25/25 on the CMS122 artifact. assert.equal(condition.onsetDateTime, undefined, "onset is never fabricated"); }); +test("an absent Encounter class stays absent too", () => { + const bundle = bundleWith({ resourceType: "Encounter", id: "e1" }); + prepareForQiCore(bundle); + assert.equal(bundle.entry[0]!.resource.class, undefined, "not silently ambulatory"); +}); + +test("a system-less code keeps its CODE and gains the system - resolved does not become active", () => { + // The bug the old behaviour actually had, as opposed to the one its docstring claimed to have fixed. + // `unbindable()` is true of a system-less `resolved` exactly as it is of a system-less `active`, and + // both took the same module-level default - so a corrected misdiagnosis was reported as an active, + // confirmed problem. That patient enters the CMS122 denominator and, with no HbA1c, its numerator. + const bundle = bundleWith({ + resourceType: "Condition", + clinicalStatus: { coding: [{ code: "resolved" }] }, + verificationStatus: { coding: [{ code: "entered-in-error" }] }, + category: [{ coding: [{ code: "encounter-diagnosis" }] }], + }); + prepareForQiCore(bundle); + const condition = bundle.entry[0]!.resource; + assert.deepEqual(condition.clinicalStatus, { + coding: [{ system: "http://terminology.hl7.org/CodeSystem/condition-clinical", code: "resolved" }], + }); + assert.deepEqual(condition.verificationStatus, { + coding: [{ system: "http://terminology.hl7.org/CodeSystem/condition-ver-status", code: "entered-in-error" }], + }); + assert.deepEqual(condition.category, [ + { coding: [{ system: "http://terminology.hl7.org/CodeSystem/condition-category", code: "encounter-diagnosis" }] }, + ], "an encounter diagnosis is not relabelled a problem-list item"); +}); + +test("a code outside the value set for its field is left ALONE rather than guessed at", () => { + // We cannot claim to know which system an unrecognised code came from, and stamping one would assert + // a binding the source never made. It stays unbindable, which means it is not retrieved - the honest + // outcome for a source that wrote something we do not understand. + const bundle = bundleWith({ + resourceType: "Condition", + clinicalStatus: { coding: [{ code: "wibble" }] }, + }); + prepareForQiCore(bundle); + assert.deepEqual(bundle.entry[0]!.resource.clinicalStatus, { coding: [{ code: "wibble" }] }); +}); + test("a REAL clinicalStatus is preserved - resolved does not silently become active", () => { // The first cut overwrote unconditionally, which would have turned a corrected misdiagnosis into an // active confirmed problem: that patient enters CMS122's denominator and, with no HbA1c, its @@ -52,8 +97,10 @@ test("a REAL clinicalStatus is preserved - resolved does not silently become act test("prepared bundles never share a mutable object", () => { // Module-level constants assigned by reference would alias ONE object into every prepared bundle, // so a single downstream mutation would reach all of them at once. - const a = bundleWith({ resourceType: "Condition" }); - const b = bundleWith({ resourceType: "Condition" }); + // Present-but-unbindable, since an ABSENT status is now left absent and would give two `undefined` + // values - which are equal, so the test would pass for the wrong reason. + const a = bundleWith({ resourceType: "Condition", clinicalStatus: { coding: [{ code: "active" }] } }); + const b = bundleWith({ resourceType: "Condition", clinicalStatus: { coding: [{ code: "active" }] } }); prepareForQiCore(a); prepareForQiCore(b); assert.notEqual(a.entry[0]!.resource.clinicalStatus, b.entry[0]!.resource.clinicalStatus); @@ -76,11 +123,24 @@ test("data that already carries onset, category or Encounter class is left ALONE const bundle = bundleWith( { resourceType: "Condition", onsetDateTime: "2019-04-04", category: [{ text: "real" }] }, { resourceType: "Encounter", class: { code: "IMP" } }, + { resourceType: "Encounter", class: { system: "http://terminology.hl7.org/CodeSystem/v3-ActCode", code: "IMP" } }, ); prepareForQiCore(bundle); assert.equal(bundle.entry[0]!.resource.onsetDateTime, "2019-04-04"); + // Text with no code: nothing to recognise, so nothing is claimed about it. assert.deepEqual(bundle.entry[0]!.resource.category, [{ text: "real" }]); - assert.deepEqual(bundle.entry[1]!.resource.class, { code: "IMP" }); + // **INPATIENT STAYS INPATIENT.** The first cut of #594 replaced a present-but-unbindable value with + // the module default and turned this into `AMB` - substituting a different clinical fact while + // claiming to normalize one. The code is the source, only the system is ours. + assert.deepEqual(bundle.entry[1]!.resource.class, { + system: "http://terminology.hl7.org/CodeSystem/v3-ActCode", + code: "IMP", + }); + // Already bindable, so untouched rather than re-stamped. + assert.deepEqual(bundle.entry[2]!.resource.class, { + system: "http://terminology.hl7.org/CodeSystem/v3-ActCode", + code: "IMP", + }); }); test("it normalizes structure and never touches a clinical fact", () => { @@ -96,7 +156,8 @@ test("it normalizes structure and never touches a clinical fact", () => { test("the copying form leaves its input untouched", () => { // The runtime executor needs this: the authored engine may evaluate the same bundle, and ADR-008 // requires its outcome to be byte-identical whether or not official routing is on. - const bundle = bundleWith({ resourceType: "Condition", id: "c1" }); + // Something preparation actually changes, now that an absent field is left absent. + const bundle = bundleWith({ resourceType: "Condition", id: "c1", clinicalStatus: { coding: [{ code: "active" }] } }); const original = JSON.stringify(bundle); const prepared = preparedForQiCore(bundle); assert.equal(JSON.stringify(bundle), original, "the input must not be mutated"); diff --git a/backend-ts/src/wiring/qicore-preparation.ts b/backend-ts/src/wiring/qicore-preparation.ts index f2053cb99..98611aec0 100644 --- a/backend-ts/src/wiring/qicore-preparation.ts +++ b/backend-ts/src/wiring/qicore-preparation.ts @@ -66,24 +66,86 @@ export interface PreparableBundle { entry: Array<{ resource: Record }>; } -const clinicalActive = () => ({ - coding: [{ system: "http://terminology.hl7.org/CodeSystem/condition-clinical", code: "active" }], -}); -const verificationConfirmed = () => ({ - coding: [{ system: "http://terminology.hl7.org/CodeSystem/condition-ver-status", code: "confirmed" }], -}); -const problemCategory = () => [ - { coding: [{ system: "http://terminology.hl7.org/CodeSystem/condition-category", code: "problem-list-item" }] }, -]; -const ambulatoryClass = () => ({ system: "http://terminology.hl7.org/CodeSystem/v3-ActCode", code: "AMB" }); +// The systems these four fields bind. They are the ONLY thing this module supplies for a coded field: +// the code always comes from the source (#594). Constants rather than inline strings because +// `withSystem` takes the system and the value set as a pair and they must not drift apart. +const CONDITION_CLINICAL = "http://terminology.hl7.org/CodeSystem/condition-clinical"; +const CONDITION_VER_STATUS = "http://terminology.hl7.org/CodeSystem/condition-ver-status"; +const CONDITION_CATEGORY = "http://terminology.hl7.org/CodeSystem/condition-category"; +const V3_ACT_CODE = "http://terminology.hl7.org/CodeSystem/v3-ActCode"; -/** True when a CodeableConcept carries no coding that names a system — i.e. nothing that can bind. */ +/** + * True when a CodeableConcept carries no coding that names a system — i.e. nothing that can bind. + * + * ABSENT is not unbindable, and the distinction is the whole of #594. This returned `true` for a + * missing field too, so every caller below read "there is a code here that cannot bind" and + * "the source said nothing" as the same condition — and answered both by minting a value. + */ function unbindable(concept: unknown): boolean { const codings = (concept as { coding?: Array<{ system?: unknown }> } | undefined)?.coding; if (!Array.isArray(codings) || codings.length === 0) return true; return !codings.some((coding) => typeof coding?.system === "string" && coding.system.length > 0); } +/** + * Stamp the field's system onto a code that is ALREADY THERE, preserving the code (#594). + * + * This is the whole of what this module is allowed to do to a coded field, and the shape matters more + * than it looks. The previous form asked only "can this bind?" and, when it could not, assigned a + * module-level DEFAULT — so the code the source wrote was discarded. A system-less `resolved` became + * `active`; a system-less `refuted` or `entered-in-error` became `confirmed`. The docstring above + * claimed that hole was closed and it was not: `unbindable()` is true of a system-less `resolved` just + * as it is of a system-less `active`, and both took the same default. + * + * Returns the normalized concept, or `undefined` meaning "leave this alone", for three distinct + * reasons that must not be conflated: + * - **absent** — the source said nothing, and a required target field does not authorize inventing a + * value (ADR-037; the whole of #594); + * - **already bindable** — something names a system, so there is nothing to fix; + * - **a code we do not recognise** — we cannot claim to know which system it came from. `{text: "real"}` + * with no code at all is this case, and so is any code outside the field's own value set. + */ +function withSystem(value: unknown, system: string, allowed: ReadonlySet): { coding: Array<{ system: string; code: string }> } | undefined { + if (value === undefined || value === null) return undefined; + const codings = Array.isArray(value) + ? (value as Array<{ coding?: Array<{ system?: unknown; code?: unknown }> }>).flatMap((entry) => entry?.coding ?? []) + : ((value as { coding?: Array<{ system?: unknown; code?: unknown }> }).coding ?? []); + if (!Array.isArray(codings) || codings.length === 0) return undefined; + if (codings.some((coding) => typeof coding?.system === "string" && coding.system.length > 0)) return undefined; + const code = codings.find((coding) => typeof coding?.code === "string" && allowed.has(coding.code as string))?.code; + if (typeof code !== "string") return undefined; + return { coding: [{ system, code }] }; +} + +/** As `withSystem`, for a bare Coding (`Encounter.class`) rather than a CodeableConcept. */ +function codingWithSystem(value: unknown, system: string, allowed: ReadonlySet): { system: string; code: string } | undefined { + if (value === undefined || value === null) return undefined; + const coding = value as { system?: unknown; code?: unknown }; + if (typeof coding.system === "string" && coding.system.length > 0) return undefined; + if (typeof coding.code !== "string" || !allowed.has(coding.code)) return undefined; + return { system, code: coding.code }; +} + +/** + * The value sets these three fields bind, so a code can be recognised as belonging to one. + * + * Complete rather than "the ones the corpus emits": a set holding only `active` would silently decline + * to normalize a system-less `resolved` and leave it unbindable, which reads as caution and is really + * just the old bug wearing a different coat — the condition would drop out of every retrieve instead + * of being retrieved as resolved. + */ +const CONDITION_CLINICAL_CODES: ReadonlySet = new Set([ + "active", "recurrence", "relapse", "inactive", "remission", "resolved", +]); +const CONDITION_VERIFICATION_CODES: ReadonlySet = new Set([ + "unconfirmed", "provisional", "differential", "confirmed", "refuted", "entered-in-error", +]); +const CONDITION_CATEGORY_CODES: ReadonlySet = new Set(["problem-list-item", "encounter-diagnosis"]); +/** v3 ActCode's encounter classes — `IMP` must stay inpatient, which is what caught the first cut. */ +const ENCOUNTER_CLASS_CODES: ReadonlySet = new Set([ + "AMB", "IMP", "EMER", "FLD", "HH", "ACUTE", "NONAC", "OBSENC", "PRENC", "SS", "VR", +]); + const US_CORE_BLOOD_PRESSURE = "http://hl7.org/fhir/us/core/StructureDefinition/us-core-blood-pressure"; /** The two LOINC panel codes a blood pressure is recorded under. Same set `normalize.ts` verified * against the live WebChart export; kept local because these layers must be able to move apart. */ @@ -177,6 +239,25 @@ function stampProfile(resource: Record, profile: string): void * misdiagnosis was corrected would enter CMS122's denominator and, having no HbA1c, its numerator. * The defect being fixed is an unbindable coding, so that is what the condition tests. * + * **And an ABSENT field is left absent (#594).** This is the correction to the paragraph above, which + * closed half the hole and described the other half as out of scope. `unbindable()` returns true for a + * missing field as well as an unbindable one, so until now a Condition with no `clinicalStatus` at all + * was given `active`, no `verificationStatus` became `confirmed`, no `category` became a problem-list + * item, and an Encounter with no `class` became ambulatory. + * + * That is fabrication, which ADR-037 forbids this layer, and it was **live** rather than latent: the + * QRDA-I import path (`fhir/qrda1-import.ts`) emits none of those three fields, so preparation was + * minting them on a third party's document — including stamping `active` on a Condition that carries + * an `abatementDateTime`, i.e. one the source said had ended. It also put this file in direct + * contradiction with `engine/cql/qdm-entries.ts`, which honours a system-less `entered-in-error` as a + * negation while this rewrote the same bytes to `confirmed`. + * + * **Nothing about the pilot moves**, because the ADR-075 corpus records all four itself, fully + * systemed and with `category` correctly distinguishing an encounter diagnosis from a problem-list + * item (`corpus/corpus-bundle.ts`). That is the shape the onset rule below already prescribes: where a + * measure genuinely cannot retrieve without a field, the answer is a source that records it, not a + * value minted here. A source that records nothing now retrieves nothing, which is the honest result. + * * **Onset is NOT invented**, which is why this takes no evaluation date at all. An earlier cut * anchored a missing onset three years before the evaluation date, which * this module's own rule forbids: an onset date is the date of an actual event, and CMS165 — on the @@ -192,13 +273,20 @@ export function prepareForQiCore(bundle: PreparableBundle): void { const resource = entry?.resource; if (!resource) continue; if (resource.resourceType === "Condition") { - // Fresh objects per resource: a shared constant assigned by reference would alias one object into - // every prepared bundle, so a single downstream mutation would reach all of them at once. - if (unbindable(resource.clinicalStatus)) resource.clinicalStatus = clinicalActive(); - if (unbindable(resource.verificationStatus)) resource.verificationStatus = verificationConfirmed(); - if (!resource.category) resource.category = problemCategory(); + // PRESENT-BUT-UNBINDABLE only, never absent (#594). Fresh objects per resource: a shared + // constant assigned by reference would alias one object into every prepared bundle, so a single + // downstream mutation would reach all of them at once. + const clinical = withSystem(resource.clinicalStatus, CONDITION_CLINICAL, CONDITION_CLINICAL_CODES); + if (clinical) resource.clinicalStatus = clinical; + const verification = withSystem(resource.verificationStatus, CONDITION_VER_STATUS, CONDITION_VERIFICATION_CODES); + if (verification) resource.verificationStatus = verification; + const category = withSystem(resource.category, CONDITION_CATEGORY, CONDITION_CATEGORY_CODES); + // An ARRAY field: one normalized entry, because the input's codings were all unbindable and + // carried one recognisable code between them. + if (category) resource.category = [category]; } else if (resource.resourceType === "Encounter") { - if (!resource.class) resource.class = ambulatoryClass(); + const encounterClass = codingWithSystem(resource.class, V3_ACT_CODE, ENCOUNTER_CLASS_CODES); + if (encounterClass) resource.class = encounterClass; } else if (resource.resourceType === "Observation" && isBloodPressure(resource)) { // CMS165 identifies a blood pressure by PROFILE ALONE — it is the only Observation retrieve in // that artifact with no code filter — so it is the one measure the executor runs with From c265e8ea2630bf17d5deec28851707f012f9c267 Mon Sep 17 00:00:00 2001 From: Taleef Date: Mon, 21 Sep 2026 11:25:02 -0400 Subject: [PATCH 2/6] fix(corpus): a bundle carries only what was known by its as-of MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #595, and ADR-086 covering it with #594. A corpus bundle built as of 2026-09-07 carried 19 future-dated clinical events in the first 48 records — a PHQ-9 on 8 October, a blood pressure on 14 October, mammography in October and November. The bundle knew things that had not happened. Facts are generated for the calendar year the evaluation date falls in, which is correct (ADR-072 scores a calendar year), and nothing then filtered what was EMITTED by the as-of. The cutoff filters on the date each fact was RECORDED — the value already handed to provenanceFor — and not on "every date inside the resource is in the past": a medication order known today may legitimately carry a future intended end, and dropping it would be a different wrong answer. A resource and its Provenance are emitted together, so a filtered fact leaves no status, abatement, reference or provenance behind. No reported number moves, and that is asserted rather than argued: every official measurement is taken at 31 December, so the year-end cutoff is compared against an unbounded one and must be identical. cms122, cms125, cms2 and cms137 still find real populations. A separate test requires the mid-year cutoff to actually remove something, so the no-foreknowledge sweep cannot pass against a filter that does nothing. It stops being invisible the moment anything evaluates at another date: a mid-year rerun, a demo "as of today", an encounter-time evaluation (MM-4), or an acceptance cohort built around a timing boundary. --- .../synthetic/corpus/corpus-bundle.test.ts | 71 +++++++++++++++ .../engine/synthetic/corpus/corpus-bundle.ts | 27 +++++- docs/ADR_INDEX.md | 3 +- docs/DECISIONS.md | 90 +++++++++++++++++++ docs/JOURNAL.md | 64 +++++++++++++ 5 files changed, 253 insertions(+), 2 deletions(-) diff --git a/backend-ts/src/engine/synthetic/corpus/corpus-bundle.test.ts b/backend-ts/src/engine/synthetic/corpus/corpus-bundle.test.ts index c743f6cca..ce0d9f79b 100644 --- a/backend-ts/src/engine/synthetic/corpus/corpus-bundle.test.ts +++ b/backend-ts/src/engine/synthetic/corpus/corpus-bundle.test.ts @@ -398,3 +398,74 @@ test("an engaged patient carries at least two engagement services, so rate 2's n } } }); + +// --------------------------------------------------------------------------------------------------- +// The knowledge cutoff (#595) +// +// A bundle must not know things that have not happened yet. Facts are generated for the calendar year +// the evaluation date falls in, so before 31 December a bundle used to carry the rest of the year: +// reproduced as of 2026-09-07, 19 future-dated events in the first 48 records. +// --------------------------------------------------------------------------------------------------- + +/** Every date a Provenance records — i.e. when each fact in this bundle became known. */ +const recordedDays = (patient: Parameters[0], asOf: string): string[] => + bundleForPatient(patient, asOf) + .entry.map((e) => e.resource as { resourceType: string; recorded?: string }) + .filter((r) => r.resourceType === "Provenance") + .map((r) => String(r.recorded ?? "").slice(0, 10)); + +test("no fact is dated after the as-of — the bundle has no foreknowledge (#595)", () => { + const asOf = "2027-09-07"; + let checked = 0; + for (const patient of corpusPatients(DEFAULT_CORPUS_SEED, 48)) { + for (const day of recordedDays(patient, asOf)) { + assert.ok(day <= asOf, `${patient.externalId}: a fact recorded ${day} is after the as-of ${asOf}`); + checked += 1; + } + } + assert.ok(checked > 0, "the sweep saw facts at all — otherwise it proves nothing"); +}); + +test("the cutoff actually removes something at mid-year, so the test above is not vacuous", () => { + // Without this, a bundle that happened to generate everything in January would satisfy the sweep + // while the filter did nothing at all. Measured on the first 48: the September cutoff drops facts. + const midYear = corpusPatients(DEFAULT_CORPUS_SEED, 48) + .reduce((n, p) => n + bundleForPatient(p, "2027-09-07").entry.length, 0); + const yearEnd = corpusPatients(DEFAULT_CORPUS_SEED, 48) + .reduce((n, p) => n + bundleForPatient(p, EVAL_DATE).entry.length, 0); + assert.ok(midYear < yearEnd, `a September as-of must carry fewer entries than 31 December (${midYear} vs ${yearEnd})`); +}); + +test("at 31 December the cutoff excludes NOTHING — no reported number moves", () => { + // This is the claim that makes #595 safe to ship: every official measurement is taken at year end + // (ADR-072), so the filter must be a no-op there. Compared against a cutoff far in the future + // rather than against a golden file, which is the same assertion without a fixture to rot: if the + // two bundles are identical, 31 December excluded nothing. + for (const patient of corpusPatients(DEFAULT_CORPUS_SEED, 60)) { + assert.deepEqual( + bundleForPatient(patient, EVAL_DATE), + bundleForPatient(patient, "2027-12-31T23:59:59Z"), + `${patient.externalId}: a timestamped year-end cutoff must behave as the plain day`, + ); + const atYearEnd = bundleForPatient(patient, EVAL_DATE).entry.length; + const unbounded = bundleForPatient(patient, "2099-12-31").entry.length; + assert.equal(atYearEnd, unbounded, `${patient.externalId}: year end must exclude nothing`); + } +}); + +test("a dropped fact takes its Provenance with it — no back door (#595)", () => { + // The invariant the first test in this file asserts at year end, re-asserted at a mid-year cutoff: + // a resource and its Provenance are emitted together, so a filtered fact cannot leave a dangling + // record of itself behind. Status, abatement and references go the same way. + for (const patient of corpusPatients(DEFAULT_CORPUS_SEED, 48)) { + const entries = bundleForPatient(patient, "2027-09-07").entry.map((e) => e.resource as Res); + const ids = new Set(entries.map((r) => `${r.resourceType}/${r.id}`)); + const provenances = entries.filter((r) => r.resourceType === "Provenance"); + const clinical = entries.filter((r) => !ADMINISTRATIVE.has(r.resourceType)); + assert.equal(provenances.length, clinical.length, `${patient.externalId}: one Provenance per surviving fact`); + for (const prov of provenances) { + const target = (prov.target as Array<{ reference: string }>)[0]!.reference; + assert.ok(ids.has(target), `${patient.externalId}: Provenance targets ${target}, which was filtered out`); + } + } +}); diff --git a/backend-ts/src/engine/synthetic/corpus/corpus-bundle.ts b/backend-ts/src/engine/synthetic/corpus/corpus-bundle.ts index b31ff227a..ca40b0ff1 100644 --- a/backend-ts/src/engine/synthetic/corpus/corpus-bundle.ts +++ b/backend-ts/src/engine/synthetic/corpus/corpus-bundle.ts @@ -723,7 +723,32 @@ export function bundleForPatient( clinical.push({ resource, date: day, external: false }); } - for (const item of clinical) { + // **The knowledge cutoff (#595).** A bundle must not know things that have not happened yet. + // + // Facts are generated for the calendar year the evaluation date falls in (ADR-072, and see + // `corpus-bundle-source.ts`, which passes the YEAR for exactly that reason), so a run evaluated on + // any day before 31 December used to see the rest of the year: reproduced on a bundle built as of + // 2026-09-07, 19 future-dated events in the first 48 records - a PHQ-9 on 8 October, a blood + // pressure on 14 October, mammography in October and November. + // + // The filter is on `item.date`, which is the day the fact was RECORDED - it is the date this + // corpus hands to `provenanceFor` as the recording instant. That is deliberately not "every date + // inside the resource is in the past": a medication order known today may legitimately carry a + // future intended end, and dropping it would be a second kind of wrong answer. What is excluded is + // a fact nobody could have known on the cutoff date. + // + // The back doors close with it, because a resource and its Provenance are pushed together below: + // a dropped fact leaves no status, no abatement, no reference and no provenance behind. + // + // **Nothing that is currently reported moves.** Every official measurement is taken at 31 December + // (ADR-072), and every generated fact falls inside that calendar year, so at year end this excludes + // nothing - which is asserted rather than asserted-about in `corpus-bundle.test.ts`. It starts to + // matter the moment anything evaluates at another date: a mid-year rerun, a demo "as of today", an + // encounter-time evaluation (MM-4), or an acceptance cohort built around a timing boundary. + const cutoff = evaluationDate.slice(0, 10); + const known = clinical.filter((item) => item.date.slice(0, 10) <= cutoff); + + for (const item of known) { entry.push({ resource: item.resource }); entry.push({ resource: provenanceFor( diff --git a/docs/ADR_INDEX.md b/docs/ADR_INDEX.md index c5e9addf9..5050601ee 100644 --- a/docs/ADR_INDEX.md +++ b/docs/ADR_INDEX.md @@ -6,7 +6,7 @@ > `grep -o '^#\+ ADR-[0-9]*.*' docs/DECISIONS.md`, newest first. If the highest number here is lower > than the highest there, this file is stale. > -> **`·archived`** (14 of 85) means the BODY moved to `docs/archive/DECISIONS_ARCHIVE.md` — superseded, +> **`·archived`** (14 of 86) means the BODY moved to `docs/archive/DECISIONS_ARCHIVE.md` — superseded, > or a historical *finding* rather than a decision that governs. `DECISIONS.md` keeps every heading plus > a pointer, so every cross-reference still resolves. The 70 unmarked titles are the record that governs. > @@ -14,6 +14,7 @@ ## Titles (newest first) +- ADR-086: what the source did not say is not ours to supply — a code keeps its meaning, and a corpus keeps its knowledge cutoff - ADR-085: a long run yields the event loop between subjects, and a run too long for a request is scheduled rather than awaited - ADR-084: a statement timeout is a role default the pooler cannot strip — and a filter belongs in SQL only where the database can see what it filters on - ADR-083: an exception is data the measure reads, never a status WorkWell flips — and a case a person closed is still a gap the run counts diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index 733d8141e..bc1d0da13 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -18,6 +18,96 @@ > > **Sequence note:** ADR-033 does not exist — verified absent, and the number must not be reused. +## ADR-086: what the source did not say is not ours to supply — a code keeps its meaning, and a corpus keeps its knowledge cutoff + +**Date:** 2026-09-21. **Status:** accepted. Milestone M-M (#594, #595). Applies ADR-037 +("normalization only, never fabrication") to the two places that were quietly violating it, and +sharpens ADR-075 (the generated corpus). + +### Context + +The 2026-09-07 expert review named one **high-priority correctness defect**, and a second finding +beside it. Both were accepted on 2026-09-08, named "the next two slices", and then sat untouched for +twelve days with no issue number — which is why they stayed invisible rather than being deferred on +purpose. + +**#594.** `prepareForQiCore` filled four coded fields when it could not bind them: +`Condition.clinicalStatus`, `Condition.verificationStatus`, `Condition.category` and +`Encounter.class`. Its guard, `unbindable()`, returns true for a field that is **missing** as well as +one that is present and unbindable, and in both cases the module assigned a module-level DEFAULT. + +Two consequences, and the second is worse than the review recorded: + +1. An absent field was invented. That is live rather than latent, because the QRDA-I import path + emits none of those three on a Condition and no `class` on an Encounter — so preparation minted + them **on a third party's document**, including stamping `active` on a Condition that the importer + had just given an `abatementDateTime` from a closed interval. +2. **A present code was DISCARDED.** `unbindable()` is as true of a system-less `resolved` as of a + system-less `active`, and both took the same default — so a corrected misdiagnosis was reported as + an active, confirmed problem. The file's own docstring claimed this hole was closed; it was not. + The first cut of the fix reproduced the same mistake in a new place, turning an Encounter + `{code: "IMP"}` into ambulatory, and was caught by an existing test. + +It also put two files in one pipeline in direct contradiction: `engine/cql/qdm-entries.ts` honours a +system-less `entered-in-error` as a negation while preparation rewrote those bytes to `confirmed`. + +**#595.** The generated corpus emitted facts dated after the evaluation date — reproduced at +2026-09-07 as 19 future-dated events in the first 48 records. `corpus-bundle-source.ts` passes the +measurement YEAR to generation (correctly: ADR-072 scores a calendar year), and nothing then filtered +what was emitted by the as-of. Every official measurement is taken at 31 December, so the effect on +every number we report is zero — which is exactly why it would have been found by a demo rather than +by a test. + +### Decision + +**d1. This layer supplies a SYSTEM, never a CODE.** `prepareForQiCore` normalizes a coded field only +when a value is present, cannot bind, and carries a code belonging to that field's own value set — +and it writes that same code back with the system added. Absent stays absent; already-bindable is +untouched; an unrecognised code is left alone, because we cannot claim to know which system it came +from. The value sets are complete rather than "the codes the corpus emits": a set holding only +`active` would decline to normalize `resolved` and leave it unretrievable, which reads as caution and +is the old bug in a new coat. + +**d2. A required target field does not authorize inventing its value.** Where a profile needs a field +the source never supplied, the resource goes unretrieved. That is the honest outcome, and it is the +same rule the onset paragraph in that file already applied: if a measure genuinely cannot retrieve +without a field, the answer is a **source that records it**, not a value minted in preparation. + +**d3. The QRDA-I importer derives `clinicalStatus`, because that is where the source semantics are +known** — and `times()` now reports three states rather than two. A `` with a value closes the +interval (`resolved`, matching the `abatementDateTime` written from the same value); a +`` is QDM open prevalence, an explicit assertion of no known end (`active`); +an absent `` is silence and emits nothing. Collapsing the last two was what made a faithful +mapping impossible to write however the mapping itself was expressed. + +**d4. A corpus bundle carries only what was KNOWN BY its as-of.** The filter is on the date each fact +was recorded — the value already handed to `provenanceFor` — and not on "every date inside the +resource is in the past": a medication order known today may legitimately carry a future intended +end, and dropping it would be a different wrong answer. A resource and its Provenance are emitted +together, so a filtered fact leaves no status, abatement, reference or provenance behind. + +**d5. Nothing that is currently reported moves, and that is asserted rather than argued.** For d1–d3, +the ADR-075 corpus records all four fields itself, fully systemed and with `category` distinguishing +an encounter diagnosis from a problem-list item — so the pilot's bundles never took the invented +path, and cms122/125/2/137 still find real populations. For d4, the year-end cutoff is compared +against an unbounded one and must be identical. + +### Consequences + +- **The QRDA-I import path changes**, which is the point: an imported Condition now carries the + status its document implies, or none. A document that says nothing produces a Condition that does + not satisfy the QI-Core profile and is not retrieved — visible as a smaller population rather than + as a confident wrong one. +- **`Encounter.class` is no longer supplied for imported encounters.** QRDA-I carries an encounter + type, not a FHIR class, and asserting ambulatory would be the same defect this ADR removes. If a + measure turns out to need it, that is a gap to surface in the importer against the document's own + evidence. +- A mid-year evaluation now returns different — correct — numbers from what it would have returned + before d4. No reported number is among them, because every official measurement is at year end. +- The fixtures are deliberately adversarial per the review's own bar: refuted, resolved, + `entered-in-error`, an unrecognised code, an inpatient class, and the three interval shapes. A + fixture that cannot change the answer cannot distinguish a correct mapping from the previous one. + ## ADR-085: a long run yields the event loop between subjects, and a run too long for a request is scheduled rather than awaited **Date:** 2026-09-21. **Status:** accepted. Milestone M-M (#563, #590). Builds on ADR-075 (chunked diff --git a/docs/JOURNAL.md b/docs/JOURNAL.md index f0287c20d..910323d50 100644 --- a/docs/JOURNAL.md +++ b/docs/JOURNAL.md @@ -1,5 +1,69 @@ # Journal +## 2026-09-21 (later) — the review's one high-priority defect, and it was worse than the review said + +**#594** and **#595** were accepted on 2026-09-08, named "the next two slices", and then sat for +twelve days with no issue number — which is why they stayed invisible rather than deferred. ADR-086. + +### #594 — preparation was supplying codes, not just systems + +`prepareForQiCore` filled four coded fields when it could not bind them. Its guard, `unbindable()`, +is true of a **missing** field as well as a present-but-unbindable one, and both branches assigned a +module-level default. + +The review named the first consequence: an absent field was invented. That is **live**, not latent — +the QRDA-I import emits no `clinicalStatus`, no `category` and no `class`, so preparation minted them +on a third party's document, including stamping `active` on a Condition the importer had just given +an `abatementDateTime` from a closed interval. Two files in one pipeline disagreed about identical +bytes: `qdm-entries.ts` honours a system-less `entered-in-error` as a negation while preparation +rewrote it to `confirmed`. + +**The second consequence is worse and was not in the issue: a present code was DISCARDED.** +`unbindable()` is as true of a system-less `resolved` as of a system-less `active`, so a corrected +misdiagnosis became an active, confirmed problem — that patient enters CMS122's denominator and, with +no HbA1c, its numerator. The file's own docstring claimed that hole was closed. It was not. + +So the rule is now: **this layer supplies a SYSTEM, never a CODE.** Normalize only when a value is +present, cannot bind, and carries a code from that field's own value set — writing that same code +back. Absent stays absent, bindable is untouched, an unrecognised code is left alone. + +**My first cut reproduced the exact bug it was fixing**, one field over: it replaced a +present-but-unbindable value with the module default, turning an Encounter `{code: "IMP"}` into +ambulatory — substituting a different clinical fact while claiming to normalize one. An existing test +caught it, which is the argument for rewriting tests last. + +The mapping moved to where the source semantics live. `times()` now reports **three** states, because +collapsing two of them made a faithful mapping impossible however it was written: + +| CDA `` | means | Condition | +|---|---|---| +| `value="…"` | closed interval | `resolved`, beside its own `abatementDateTime` | +| `nullFlavor="UNK"` | explicit: no known end | `active` | +| absent | silence | **no `clinicalStatus`** | + +### #595 — a corpus that knew what had not happened + +A bundle built as of 2026-09-07 carried 19 future-dated events in the first 48 records. Facts are +generated for the calendar year (correctly — ADR-072 scores a year), and nothing filtered what was +emitted by the as-of. + +The cutoff filters on the date each fact was **recorded** — the value already handed to +`provenanceFor` — rather than on "every date inside the resource is past", so a medication order +known today keeps its future intended end. A resource and its Provenance are emitted together, so a +filtered fact leaves no status, abatement, reference or provenance behind. + +### Why both were safe to ship, asserted rather than argued + +The ADR-075 corpus records all four fields itself, fully systemed, with `category` distinguishing an +encounter diagnosis from a problem-list item — so the pilot's bundles never took the invented path. +**cms122, cms125, cms2 and cms137 still find real populations after the change.** And the year-end +cutoff is compared against an unbounded one and must be identical, which is the whole claim that no +reported number moves. + +Ten mutations, all killed — including "code discarded, default substituted", which is the one that +would have shipped if the tests had been rewritten to match the implementation instead of the other +way round. + ## 2026-09-21 — the microtask trap, and the profile that named a function I had not read Two issues, one PR: **#590** (a MEASURE run always 504'd and invited a retry that also ran) and From dc13c59184d58613276a5d2e5d30e7530a33d650 Mon Sep 17 00:00:00 2001 From: Taleef Date: Mon, 21 Sep 2026 11:44:19 -0400 Subject: [PATCH 3/6] fix(qicore): a parse failure is not an assertion, and normalizing an entry keeps its neighbours MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both Codex findings on #606, and both are the same class as the defect the PR removes — which is the part worth recording. A malformed `` was read as an open interval. The first cut asked "a exists and produced no date", which is equally true of a date the SOURCE asserted and this importer could not parse. It reported `active`: a status the document never made, about a condition whose end we simply failed to read, and one that can put the patient into a measure population. It is now keyed on the nullFlavor attribute, and on any spelling — NI, NA and ASKU all mean the source addressed the end and recorded no value for it, so keying on UNK alone would read the rest as silence. A parse failure is a third thing and says nothing. Normalizing one `category` entry discarded the others. The first cut flattened every entry's codings, picked one recognised code and assigned the result AS the whole array, so a Condition carrying two categories kept one and lost the other with its text and extensions. Each entry is now normalized on its own, everything else it carries is preserved, and an entry that cannot be normalized passes through unchanged — the same reason an unrecognised code is left alone one level up. Three separate cuts of this change have now re-committed the error it exists to remove: substituting a default code for IMP, reading a parse failure as an assertion, and replacing an array to normalize one of its entries. Each was caught by a test rather than by review of the idea, which is the argument for writing the adversarial fixture first. ADR-086 records it. Five tests added, three more mutations killed. --- backend-ts/src/fhir/qrda1-import.test.ts | 34 +++++++++++++++++ backend-ts/src/fhir/qrda1-import.ts | 9 ++++- .../src/wiring/qicore-preparation.test.ts | 38 +++++++++++++++++++ backend-ts/src/wiring/qicore-preparation.ts | 35 +++++++++++++---- docs/DECISIONS.md | 32 +++++++++++++--- 5 files changed, 135 insertions(+), 13 deletions(-) diff --git a/backend-ts/src/fhir/qrda1-import.test.ts b/backend-ts/src/fhir/qrda1-import.test.ts index 8b15cc635..a241256d3 100644 --- a/backend-ts/src/fhir/qrda1-import.test.ts +++ b/backend-ts/src/fhir/qrda1-import.test.ts @@ -910,3 +910,37 @@ test("import: the three interval shapes produce three DIFFERENT statuses", () => ].map((t) => JSON.stringify(conditionFromDoc(t)?.clinicalStatus ?? null)); assert.equal(new Set(statuses).size, 3, `expected three distinct statuses, got ${statuses.join(" | ")}`); }); + +test("import: a MALFORMED high value is a parse failure, not an open interval", () => { + // `20240230` is a date the source asserted and this importer cannot parse - 30 February. The first + // cut of #594 inferred `endUnknown` from "a exists and produced no date", which is true here + // too, so it reported the diagnosis as `active`: a status the document never asserted, on a + // condition whose end date we simply failed to understand, and one that can put the patient into a + // measure population. It is the nullFlavor that says "open", not our own failure to read a value. + const condition = conditionFromDoc(``); + assert.ok(condition); + assert.equal(condition!.abatementDateTime, undefined, "nothing parseable to record"); + assert.equal(condition!.clinicalStatus, undefined, "not active, not resolved - the document was not understood"); +}); + +test("import: an EMPTY high element asserts nothing either", () => { + // No value and no nullFlavor: the element is there but says nothing at all. + const condition = conditionFromDoc(``); + assert.ok(condition); + assert.equal(condition!.clinicalStatus, undefined); +}); + +test("import: other nullFlavor spellings are open intervals too, not just UNK", () => { + // `NI`, `NA`, `ASKU` all say the same thing for this purpose: the source addressed the end and + // recorded no value for it. Keying on UNK alone would read the rest as silence. + for (const flavor of ["UNK", "NI", "NA", "ASKU"]) { + const condition = conditionFromDoc( + ``, + ); + assert.deepEqual( + condition!.clinicalStatus, + { coding: [{ system: CLINICAL, code: "active" }] }, + `nullFlavor="${flavor}" is an explicit open interval`, + ); + } +}); diff --git a/backend-ts/src/fhir/qrda1-import.ts b/backend-ts/src/fhir/qrda1-import.ts index af5ac41d5..063b1b659 100644 --- a/backend-ts/src/fhir/qrda1-import.ts +++ b/backend-ts/src/fhir/qrda1-import.ts @@ -202,7 +202,14 @@ function times(node: CdaNode | undefined): { point?: string; start?: string; end // assertion: `nullFlavor="UNK"` is an explicit "this has not ended, and I do not know when it // will" - QDM open prevalence - while an absent element is silence. One licenses reporting the // condition as active; the other licenses nothing. - const endUnknown = high !== undefined && end === undefined; + // + // **It is the nullFlavor that says so, not the absence of a parsed date** (Codex review). The first + // cut asked `high !== undefined && end === undefined`, which is also true of `` - a value the source DID assert and this importer could not parse. Reading + // that as an open interval would report `active` for a diagnosis whose end date we simply failed to + // understand, and that status can put the condition into a measure population. A malformed value is + // neither a closed interval nor an open one: it is a parse failure, and it says nothing. + const endUnknown = end === undefined && typeof high?.attrs.nullFlavor === "string" && high.attrs.nullFlavor.length > 0; return { start: isoFromHl7(child(node, "low")?.attrs.value), end, diff --git a/backend-ts/src/wiring/qicore-preparation.test.ts b/backend-ts/src/wiring/qicore-preparation.test.ts index bb923a513..727ccaefd 100644 --- a/backend-ts/src/wiring/qicore-preparation.test.ts +++ b/backend-ts/src/wiring/qicore-preparation.test.ts @@ -397,3 +397,41 @@ test("stamping does not leak into the caller's bundle (ADR-008: the authored out preparedForQiCore(bundle as never); assert.equal((resource as { meta?: unknown }).meta, undefined, "the caller's own bundle is untouched"); }); + +test("normalizing one category entry does not discard the others (#594, review)", () => { + // The first cut flattened every entry's codings into one list, picked a single recognised code and + // assigned the result AS the whole array - so a Condition carrying two categories kept one and lost + // the other, with any text or extension on it. That is data loss dressed as normalization, which is + // the defect this change exists to remove. + const bundle = bundleWith({ + resourceType: "Condition", + category: [ + { coding: [{ code: "encounter-diagnosis" }] }, + { text: "the practice's own label" }, + { coding: [{ code: "wibble" }], text: "unrecognised, and kept" }, + ], + }); + prepareForQiCore(bundle); + const category = bundle.entry[0]!.resource.category as Array>; + assert.equal(category.length, 3, "every entry survives"); + assert.deepEqual(category[0], { + coding: [{ system: "http://terminology.hl7.org/CodeSystem/condition-category", code: "encounter-diagnosis" }], + }); + assert.deepEqual(category[1], { text: "the practice's own label" }, "text-only entries are untouched"); + assert.deepEqual( + category[2], + { coding: [{ code: "wibble" }], text: "unrecognised, and kept" }, + "an unrecognised code is left exactly as it came, alongside its text", + ); +}); + +test("a normalized category entry keeps its own text and extensions", () => { + const bundle = bundleWith({ + resourceType: "Condition", + category: [{ coding: [{ code: "problem-list-item" }], text: "Problem list", id: "cat-1" }], + }); + prepareForQiCore(bundle); + const entry = (bundle.entry[0]!.resource.category as Array>)[0]!; + assert.equal(entry.text, "Problem list", "only `coding` is replaced"); + assert.equal(entry.id, "cat-1"); +}); diff --git a/backend-ts/src/wiring/qicore-preparation.ts b/backend-ts/src/wiring/qicore-preparation.ts index 98611aec0..3cfcff5c1 100644 --- a/backend-ts/src/wiring/qicore-preparation.ts +++ b/backend-ts/src/wiring/qicore-preparation.ts @@ -107,9 +107,7 @@ function unbindable(concept: unknown): boolean { */ function withSystem(value: unknown, system: string, allowed: ReadonlySet): { coding: Array<{ system: string; code: string }> } | undefined { if (value === undefined || value === null) return undefined; - const codings = Array.isArray(value) - ? (value as Array<{ coding?: Array<{ system?: unknown; code?: unknown }> }>).flatMap((entry) => entry?.coding ?? []) - : ((value as { coding?: Array<{ system?: unknown; code?: unknown }> }).coding ?? []); + const codings = (value as { coding?: Array<{ system?: unknown; code?: unknown }> }).coding ?? []; if (!Array.isArray(codings) || codings.length === 0) return undefined; if (codings.some((coding) => typeof coding?.system === "string" && coding.system.length > 0)) return undefined; const code = codings.find((coding) => typeof coding?.code === "string" && allowed.has(coding.code as string))?.code; @@ -117,6 +115,30 @@ function withSystem(value: unknown, system: string, allowed: ReadonlySet return { coding: [{ system, code }] }; } +/** + * The same rule over an ARRAY field (`Condition.category`), entry by entry (Codex review). + * + * The first cut flattened every entry's codings into one list, picked a single recognised code and + * assigned the result as the whole array - so a Condition carrying two categories kept one and lost + * the other, along with any `text` or extension on it. That is data loss dressed as normalization, + * which is the defect this change exists to remove, so it must not appear inside the fix. + * + * An entry that cannot be normalized is passed through UNCHANGED rather than dropped: the same reason + * an unrecognised code is left alone, one level up. + */ +function eachWithSystem(value: unknown, system: string, allowed: ReadonlySet): unknown[] | undefined { + if (!Array.isArray(value) || value.length === 0) return undefined; + let changed = false; + const next = value.map((entry) => { + const normalized = withSystem(entry, system, allowed); + if (!normalized) return entry; + changed = true; + // Preserve everything else the entry carried; only its `coding` is replaced. + return { ...(entry as Record), coding: normalized.coding }; + }); + return changed ? next : undefined; +} + /** As `withSystem`, for a bare Coding (`Encounter.class`) rather than a CodeableConcept. */ function codingWithSystem(value: unknown, system: string, allowed: ReadonlySet): { system: string; code: string } | undefined { if (value === undefined || value === null) return undefined; @@ -280,10 +302,9 @@ export function prepareForQiCore(bundle: PreparableBundle): void { if (clinical) resource.clinicalStatus = clinical; const verification = withSystem(resource.verificationStatus, CONDITION_VER_STATUS, CONDITION_VERIFICATION_CODES); if (verification) resource.verificationStatus = verification; - const category = withSystem(resource.category, CONDITION_CATEGORY, CONDITION_CATEGORY_CODES); - // An ARRAY field: one normalized entry, because the input's codings were all unbindable and - // carried one recognisable code between them. - if (category) resource.category = [category]; + // An ARRAY field, normalized entry by entry so nothing else in it is lost. + const category = eachWithSystem(resource.category, CONDITION_CATEGORY, CONDITION_CATEGORY_CODES); + if (category) resource.category = category; } else if (resource.resourceType === "Encounter") { const encounterClass = codingWithSystem(resource.class, V3_ACT_CODE, ENCOUNTER_CLASS_CODES); if (encounterClass) resource.class = encounterClass; diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index bc1d0da13..4b2d0d6f8 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -68,6 +68,15 @@ from. The value sets are complete rather than "the codes the corpus emits": a se `active` would decline to normalize `resolved` and leave it unretrievable, which reads as caution and is the old bug in a new coat. +**d1a. Normalizing an entry may not discard its neighbours** (review). `Condition.category` is an +array, and the first cut flattened every entry's codings, chose one recognised code and assigned the +result as the whole array — so a Condition carrying two categories kept one and lost the other, along +with any `text` or extension on it. Each entry is normalized on its own and everything else it +carries is preserved; an entry that cannot be normalized passes through unchanged rather than being +dropped, for the same reason an unrecognised code is left alone. Data loss dressed as normalization +is the defect this ADR removes, and it must not appear inside the fix — which, twice now, is exactly +where it appeared. + **d2. A required target field does not authorize inventing its value.** Where a profile needs a field the source never supplied, the resource goes unretrieved. That is the honest outcome, and it is the same rule the onset paragraph in that file already applied: if a measure genuinely cannot retrieve @@ -75,11 +84,19 @@ without a field, the answer is a **source that records it**, not a value minted **d3. The QRDA-I importer derives `clinicalStatus`, because that is where the source semantics are known** — and `times()` now reports three states rather than two. A `` with a value closes the -interval (`resolved`, matching the `abatementDateTime` written from the same value); a -`` is QDM open prevalence, an explicit assertion of no known end (`active`); -an absent `` is silence and emits nothing. Collapsing the last two was what made a faithful +interval (`resolved`, matching the `abatementDateTime` written from the same value); a `` +carrying a `nullFlavor` is QDM open prevalence, an explicit assertion of no known end (`active`); an +absent `` is silence and emits nothing. Collapsing the last two was what made a faithful mapping impossible to write however the mapping itself was expressed. +**It is the `nullFlavor` that says "open", not our own failure to parse a value** (review). The first +cut asked "a `` exists and produced no date", which is equally true of `` — a date the source asserted and the importer could not read. Reporting that as +`active` would assert a status the document never made, about a condition whose end we simply failed +to understand, and that status can put the patient into a measure population. A parse failure is a +third thing, and it says nothing. Any `nullFlavor` counts, not `UNK` alone: `NI`, `NA` and `ASKU` all +mean the source addressed the end and recorded no value for it. + **d4. A corpus bundle carries only what was KNOWN BY its as-of.** The filter is on the date each fact was recorded — the value already handed to `provenanceFor` — and not on "every date inside the resource is in the past": a medication order known today may legitimately carry a future intended @@ -105,8 +122,13 @@ against an unbounded one and must be identical. - A mid-year evaluation now returns different — correct — numbers from what it would have returned before d4. No reported number is among them, because every official measurement is at year end. - The fixtures are deliberately adversarial per the review's own bar: refuted, resolved, - `entered-in-error`, an unrecognised code, an inpatient class, and the three interval shapes. A - fixture that cannot change the answer cannot distinguish a correct mapping from the previous one. + `entered-in-error`, an unrecognised code, an inpatient class, a malformed `20240230`, a + multi-entry category, and the three interval shapes. A fixture that cannot change the answer cannot + distinguish a correct mapping from the previous one. +- **Three separate cuts of this change re-committed the error it exists to remove** — substituting a + default code for `IMP`, reading a parse failure as an assertion, and replacing an array to + normalize one of its entries. Each was caught by a test rather than by review of the idea, which is + the argument for writing the adversarial fixture first and the convenient one never. ## ADR-085: a long run yields the event loop between subjects, and a run too long for a request is scheduled rather than awaited From 9d0f86848957afaa03d67dc239df4db209390ffb Mon Sep 17 00:00:00 2001 From: Taleef Date: Mon, 21 Sep 2026 11:52:22 -0400 Subject: [PATCH 4/6] docs(audit): the audit rule holds for operator actions, not for the run path (#598) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CLAUDE.md states "every state change writes audit_event — no exceptions". The run path is an exception, and it was discoverable only by reading run-pipeline.ts, which is the shape of claim DATA_MODEL_CONTRACTS exists to stop. The two paths make opposite trades on purpose and both stay. An operator action records the event FIRST and then applies the patch (recordCaseEvent makes the action row and the audit row one transaction), so a failure between them leaves an action recorded but not applied — never an unaudited state change. A run upserts the case FIRST and audits best-effort, so a failure there leaves a state change applied but unaudited, accepted because the alternative strands an otherwise-complete run as RUNNING after the case was already mutated. What is missing is the primitive, not the ordering — and it cannot live in one store: the action and audit rows belong to CaseEventStore, which already opens its own BEGIN/COMMIT, while the patch belongs to CaseStore, so applyCaseAction({patch, action, audit}) needs a transaction seam spanning both. Deferred deliberately: it wants local Postgres, since the SQLite floor cannot catch Pg-only SQL. Until it exists, nothing should build operational reliance on the ledger being complete for run-created transitions, and a reconciliation job is not a substitute without durable operation identity, an expected version, a deadline and a visible failure state. Also #599's backend half: validateTests checks SHAPE, not outcomes, and its docstring said so only by omission. Three tests pin the limitation rather than the copy — a fixture naming a subject that exists nowhere passes, and two fixtures asserting opposite outcomes for one subject both pass. When execution lands, those tests fail, which forces the label and the behaviour to move together. --- CLAUDE.md | 4 +- backend-ts/src/measure/measure-read-models.ts | 16 ++++- .../validate-tests-does-not-execute.test.ts | 65 +++++++++++++++++++ docs/DATA_MODEL_CONTRACTS.md | 23 +++++++ 4 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 backend-ts/src/measure/validate-tests-does-not-execute.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 62f6030d7..61c471a71 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -29,7 +29,9 @@ sprint context — read them for background, never act on them. - Auth: user accounts remain hardcoded (no SSO, no real user directory). The JWT refresh token flow (HttpOnly cookie, token rotation, `/api/auth/refresh`) is approved and implemented. - Email: `WORKWELL_EMAIL_PROVIDER=simulated` is the default and must remain so on the demo stack. SendGrid wiring exists in the code but must not be activated unless `WORKWELL_EMAIL_SENDGRID_API_KEY` is explicitly set (with `WORKWELL_EMAIL_PROVIDER=sendgrid`) in a non-demo environment. - AI never decides compliance (see docs/AI_GUARDRAILS.md). CQL engine is sole source of truth. -- Every state change writes `audit_event` — no exceptions +- Every state change writes `audit_event` — no exceptions. **One known gap, #598:** a run-created case + transition audits best-effort AFTER the upsert, so it can mutate and lose the event. Operator actions + audit first and cannot. `DATA_MODEL_CONTRACTS` §4 carries which is which and why. - No silent scope changes. If a stop condition triggers, document fallback in JOURNAL.md. - Schema migrations are owned by Taleef — never written or applied by an agent without explicit instruction diff --git a/backend-ts/src/measure/measure-read-models.ts b/backend-ts/src/measure/measure-read-models.ts index 417a9ebd9..f7eb0316d 100644 --- a/backend-ts/src/measure/measure-read-models.ts +++ b/backend-ts/src/measure/measure-read-models.ts @@ -171,7 +171,21 @@ export const compileAllowsActivation = (s: string) => s.toUpperCase() === "COMPI const OUTCOME_BUCKETS = new Set(["COMPLIANT", "DUE_SOON", "OVERDUE", "MISSING_DATA", "EXCLUDED"]); -/** Port of MeasureService.validateTests: a fixture set passes when non-empty and each fixture is well-formed. */ +/** + * Port of MeasureService.validateTests: a fixture set passes when non-empty and each fixture is + * well-formed. + * + * **It does NOT execute anything (#599), and the name is the whole problem.** "Tests passed" reads as + * "the fixtures ran and the measure produced the expected outcomes". What is checked is that the list + * is non-empty and that each entry has a name, a subject and an `expectedOutcome` in the allowed set. + * A fixture asserting an impossible outcome passes this gate, and so does one whose expected outcome + * contradicts the CQL — so activation was blocked by a control that could not fail on the thing its + * label implied. + * + * Studio's row now reads "Fixtures Well-Formed … not executed against the measure". Executing them is + * the real fix (the engine is right there, and a fixture is a subject plus an expected outcome) and + * belongs with the next Studio work; #599 carries it. + */ export function validateTests(fixtures: MeasureSpec["testFixtures"]): { passed: boolean; failures: string[] } { if (fixtures.length === 0) return { passed: false, failures: ["At least one test fixture is required before activation."] }; const failures: string[] = []; diff --git a/backend-ts/src/measure/validate-tests-does-not-execute.test.ts b/backend-ts/src/measure/validate-tests-does-not-execute.test.ts new file mode 100644 index 000000000..adfb51af3 --- /dev/null +++ b/backend-ts/src/measure/validate-tests-does-not-execute.test.ts @@ -0,0 +1,65 @@ +/** + * `validateTests` checks SHAPE, not outcomes (#599). + * + * Studio rendered a green tick on a row labelled "Test Fixtures" and blocked activation until it + * passed, which reads as *the fixtures were run and the measure produced the expected outcomes*. It + * never meant that: the function checks the list is non-empty and that each entry carries a name, a + * subject, and an `expectedOutcome` in the allowed set. So activation was gated by a control that + * could not fail on the thing its label implied — the vacuous-guard shape this repo keeps finding. + * + * These tests pin the limitation rather than the copy, and that is deliberate. The Studio row now + * says "Fixtures Well-Formed … not executed against the measure", but a label is easy to drift back. + * If someone later implements execution (issue #599's option 2, which is the real fix), the first + * test here FAILS — forcing the label and the behaviour to move together rather than apart. + * + * node --import tsx --test src/measure/validate-tests-does-not-execute.test.ts + */ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { validateTests } from "./measure-read-models.ts"; + +type Fixture = Parameters[0][number]; + +const fixture = (over: Partial = {}): Fixture => + ({ + fixtureName: "a well-formed fixture", + employeeExternalId: "emp-006", + expectedOutcome: "COMPLIANT", + ...over, + }) as Fixture; + +test("a fixture naming a subject that does not exist still PASSES — nothing is executed", () => { + // `nobody-at-all` is in no directory and no corpus. If this function ran the measure it could not + // report success here. When #599's option 2 lands, this is the test that tells you to move the + // label with it. + const result = validateTests([fixture({ employeeExternalId: "nobody-at-all" })]); + assert.equal(result.passed, true, "shape-only: the subject is never looked up"); + assert.deepEqual(result.failures, []); +}); + +test("a fixture whose expected outcome contradicts the measure still passes", () => { + // Two fixtures for the SAME subject asserting opposite outcomes. At most one can be right, and a + // gate that evaluated anything would say so. + const result = validateTests([ + fixture({ fixtureName: "compliant", expectedOutcome: "COMPLIANT" }), + fixture({ fixtureName: "overdue", expectedOutcome: "OVERDUE" }), + ]); + assert.equal(result.passed, true, "contradictory expectations are not detectable without executing"); +}); + +test("what it DOES check: presence, a name, a subject, and a known outcome bucket", () => { + // The other half of an honest label - the check is real, it is just narrower than it read. + assert.equal(validateTests([]).passed, false, "an empty set blocks activation"); + assert.match(validateTests([]).failures[0]!, /At least one test fixture/); + + assert.equal(validateTests([fixture({ fixtureName: " " })]).passed, false, "a blank name is not a name"); + assert.equal(validateTests([fixture({ employeeExternalId: "" })]).passed, false, "a subject is required"); + + const unknown = validateTests([fixture({ expectedOutcome: "WIBBLE" as Fixture["expectedOutcome"] })]); + assert.equal(unknown.passed, false); + assert.match(unknown.failures[0]!, /unsupported expectedOutcome/); + + // And every failure names WHICH fixture, since an author has to find it. + const second = validateTests([fixture(), fixture({ fixtureName: "" })]); + assert.match(second.failures[0]!, /Fixture 2/); +}); diff --git a/docs/DATA_MODEL_CONTRACTS.md b/docs/DATA_MODEL_CONTRACTS.md index 731590f9c..9638391ef 100644 --- a/docs/DATA_MODEL_CONTRACTS.md +++ b/docs/DATA_MODEL_CONTRACTS.md @@ -121,6 +121,29 @@ SQLite floor and the Pg ceiling read the current row and apply the shared pure ` instead of being left stuck RUNNING / marked FAILED after the case was already mutated (mirrors the `RUN_COMPLETED` best-effort write). + > **So "every state change writes an `audit_event`" is TRUE OF OPERATOR ACTIONS and NOT of the + > run-created case transition (#598).** CLAUDE.md states the rule without that qualification, and + > the qualification was discoverable only by reading `run-pipeline.ts` — which is the shape of + > claim this file exists to stop. The two paths make opposite trades ON PURPOSE and both are + > deliberate: + > + > - **An operator action records the event FIRST, then applies the patch** (`case/case-actions.ts`; + > `recordCaseEvent` makes the action row and the audit row one transaction). A failure between + > the two leaves an action **recorded but not applied** — never an unaudited state change. Bulk + > assign takes the same side, auditing before it mutates. + > - **A run upserts the case FIRST, then audits best-effort.** A failure there leaves a state change + > **applied but unaudited**, which is the violation — accepted because the alternative strands an + > otherwise-complete run as RUNNING after the case was already mutated. + > + > **What is missing is the primitive, not the ordering.** There is no `applyCaseAction({ patch, + > action, audit })` making all three one unit, and there cannot be one inside a single store: the + > action and audit rows belong to `CaseEventStore` (which already opens its own `BEGIN`/`COMMIT`) + > while the patch belongs to `CaseStore`, so a real fix needs a transaction seam spanning both. + > Until that exists, **do not build operational reliance on the ledger being complete for + > run-created transitions.** A reconciliation job is not a substitute: without durable operation + > identity, an expected version, a deadline and a visible failure state it is a second unreliable + > thing checking the first. + ### The work list is READ two ways, and they must answer the same question (#561, ADR-084) `/api/cases` takes ONE page and the exact total from a single statement (`CaseStore.listCasesPage`, From b2367187039c22f4c94696e25d022a738f96377e Mon Sep 17 00:00:00 2001 From: Taleef Date: Mon, 21 Sep 2026 11:52:31 -0400 Subject: [PATCH 5/6] fix(studio): the fixtures row says what it checks, not what it implied (#599) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Studio rendered "Test Fixtures ✅" and blocked activation until it passed, which reads as: the fixtures ran and the measure produced the expected outcomes. validateTests never executes anything. It checks the list is non-empty and that each entry has a name, a subject, and an expectedOutcome in the allowed set — so activation was gated by a control that could not fail on the thing its label implied, which is the vacuous-guard shape this repo keeps finding. The row now reads "Fixtures Well-Formed — present and well-formed; not executed against the measure". Relabel only. Executing them is the real fix and belongs with the next Studio work; #599 carries it, and the backend tests in the previous commit are what make the two move together — they pin the limitation, so implementing execution fails them. Blast radius was bounded: Studio authoring is hidden from the pilot's CASE_MANAGER seats, so the misleading tick was in front of engineering users only. That is why it was not urgent, not a reason it was fine. --- docs/JOURNAL.md | 48 +++++++++++++++++++ .../studio/components/ReleaseApprovalTab.tsx | 21 +++++++- 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/docs/JOURNAL.md b/docs/JOURNAL.md index 910323d50..5df2dca62 100644 --- a/docs/JOURNAL.md +++ b/docs/JOURNAL.md @@ -1,5 +1,53 @@ # Journal +## 2026-09-21 (evening) — two controls that promised more than they did + +**#598** and **#599**, both from the 2026-09-07 review, both about a claim being wider than the thing +behind it. Neither is a behaviour change; both are the always-loaded contract and a label catching up +with the code. + +### #598 — "every state change writes an audit_event" is true of operator actions, and not of runs + +`CLAUDE.md` states the rule with "no exceptions". The run path is an exception, and it was +discoverable only by reading `run-pipeline.ts`. + +The two paths make **opposite trades on purpose**: + +- An **operator action** records the event first, then applies the patch (`recordCaseEvent` makes the + action row and the audit row one transaction). A failure between them leaves an action recorded but + not applied — never an unaudited state change. +- A **run** upserts the case first, then audits best-effort. A failure there leaves a state change + applied but unaudited, accepted because the alternative strands an otherwise-complete run as + RUNNING after the case was already mutated. + +Both orderings stay. What is missing is the **primitive**, and it cannot live in one store: the action +and audit rows belong to `CaseEventStore` (which already opens its own `BEGIN`/`COMMIT`) while the +patch belongs to `CaseStore`, so a real `applyCaseAction({ patch, action, audit })` needs a +transaction seam spanning both. That is deferred deliberately, not forgotten — it wants local +Postgres, since the SQLite floor cannot catch Pg-only SQL, and #598 carries it with the line that +until it exists **nothing should build operational reliance on the ledger being complete for +run-created transitions.** + +The correction is in `DATA_MODEL_CONTRACTS` §4 beside the best-effort note and in `CLAUDE.md`'s rule +itself, because a rule whose exception lives in a source file is a rule a session will contradict. + +### #599 — an approval gate that could not fail on the thing its label implied + +Studio rendered "Test Fixtures ✅" and blocked activation until it passed, which reads as *the +fixtures ran and the measure produced the expected outcomes*. `validateTests` never executes +anything: it checks the list is non-empty and that each entry has a name, a subject, and an +`expectedOutcome` in the allowed set. + +The row now reads **"Fixtures Well-Formed — present and well-formed; not executed against the +measure"**, and the function's docstring says the same. + +**The guard went on the semantics, not the copy.** A render test asserting label text would need four +child panels mocked to check a string, and would pin the wording rather than the meaning. Instead +three backend tests pin the *limitation*: a fixture naming a subject that exists nowhere still +passes, and two fixtures asserting opposite outcomes for the same subject both pass. If someone +implements execution — #599's option 2, the real fix — **those tests fail**, which forces the label +and the behaviour to move together instead of drifting apart again. + ## 2026-09-21 (later) — the review's one high-priority defect, and it was worse than the review said **#594** and **#595** were accepted on 2026-09-08, named "the next two slices", and then sat for diff --git a/frontend/features/studio/components/ReleaseApprovalTab.tsx b/frontend/features/studio/components/ReleaseApprovalTab.tsx index 21931666c..a3b3f0d36 100644 --- a/frontend/features/studio/components/ReleaseApprovalTab.tsx +++ b/frontend/features/studio/components/ReleaseApprovalTab.tsx @@ -140,7 +140,26 @@ export function ReleaseApprovalTab({

Readiness Checklist

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

-

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

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

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

Value Set Resolvability:{" "} {hasValueSets ? ( From 3b54eafaeeff13e2cdfeb70943d5f0381ea2515a Mon Sep 17 00:00:00 2001 From: Taleef Date: Mon, 21 Sep 2026 12:02:16 -0400 Subject: [PATCH 6/6] =?UTF-8?q?docs(audit):=20the=20correction=20was=20too?= =?UTF-8?q?=20broad=20too=20=E2=80=94=20the=20paths,=20swept=20rather=20th?= =?UTF-8?q?an=20recalled?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex on #607. The first cut said "operator actions audit first and cannot lose the event": true of CASE actions, false of several other operator surfaces. That is the same failure one level up, inside a change whose entire subject is claims being wider than the thing behind them. Enumerated by sweeping for the mutate-before-audit shape instead of recalling it. AUDIT FIRST, cannot produce an unaudited state change: every case action (recordCaseEvent makes the action row and the audit row one transaction, patch follows), rerun-to-verify's case patch, bulk assign and panel backfill via the batch form. MUTATE FIRST, can apply a change and lose the event: the run-created case transition; the measure lifecycle (create :54, approve :69, deprecate :87, transition :118); segment create (routes/segments.ts:172); terminology-mapping create (value-set-governance.ts:335). Only the run's ordering is a considered trade — the alternative strands an otherwise-complete run as RUNNING after the case was already mutated. The other six are simply the order they were written in, which splits #598 into a cheap half (flip them; no seam needed) and the primitive that still wants a cross-store transaction. Recorded on the issue. case-rerun.ts looked like a violation and is not: its first mutation creates a RUN row, and the case patch is explicitly after an audit-first recordCaseEvent. Checked rather than assumed, in both directions. The sweep covered admin, case, measure, quality, program, compliance and routes and is a heuristic, so the list is what it found rather than proof of completeness — stated as such. --- CLAUDE.md | 8 ++++--- docs/DATA_MODEL_CONTRACTS.md | 43 +++++++++++++++++++++++++----------- docs/JOURNAL.md | 18 +++++++++++++++ 3 files changed, 53 insertions(+), 16 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 61c471a71..8a5a80d44 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -29,9 +29,11 @@ sprint context — read them for background, never act on them. - Auth: user accounts remain hardcoded (no SSO, no real user directory). The JWT refresh token flow (HttpOnly cookie, token rotation, `/api/auth/refresh`) is approved and implemented. - Email: `WORKWELL_EMAIL_PROVIDER=simulated` is the default and must remain so on the demo stack. SendGrid wiring exists in the code but must not be activated unless `WORKWELL_EMAIL_SENDGRID_API_KEY` is explicitly set (with `WORKWELL_EMAIL_PROVIDER=sendgrid`) in a non-demo environment. - AI never decides compliance (see docs/AI_GUARDRAILS.md). CQL engine is sole source of truth. -- Every state change writes `audit_event` — no exceptions. **One known gap, #598:** a run-created case - transition audits best-effort AFTER the upsert, so it can mutate and lose the event. Operator actions - audit first and cannot. `DATA_MODEL_CONTRACTS` §4 carries which is which and why. +- Every state change writes `audit_event` — the RULE, and it is not everywhere true today (#598). + **CASE actions audit first and cannot lose the event.** The run-created case transition, the measure + lifecycle, segment create and terminology-mapping create all MUTATE first and audit after, so each + can. Write new code audit-first; `DATA_MODEL_CONTRACTS` §4 carries the list and why the run's + ordering is deliberate. - No silent scope changes. If a stop condition triggers, document fallback in JOURNAL.md. - Schema migrations are owned by Taleef — never written or applied by an agent without explicit instruction diff --git a/docs/DATA_MODEL_CONTRACTS.md b/docs/DATA_MODEL_CONTRACTS.md index 9638391ef..38ee2fcb2 100644 --- a/docs/DATA_MODEL_CONTRACTS.md +++ b/docs/DATA_MODEL_CONTRACTS.md @@ -121,21 +121,38 @@ SQLite floor and the Pg ceiling read the current row and apply the shared pure ` instead of being left stuck RUNNING / marked FAILED after the case was already mutated (mirrors the `RUN_COMPLETED` best-effort write). - > **So "every state change writes an `audit_event`" is TRUE OF OPERATOR ACTIONS and NOT of the - > run-created case transition (#598).** CLAUDE.md states the rule without that qualification, and - > the qualification was discoverable only by reading `run-pipeline.ts` — which is the shape of - > claim this file exists to stop. The two paths make opposite trades ON PURPOSE and both are - > deliberate: + > **So "every state change writes an `audit_event`" is the RULE, and it is not everywhere true + > today (#598).** CLAUDE.md stated it with "no exceptions", and the exceptions were discoverable + > only by reading the source — which is the shape of claim this file exists to stop. > - > - **An operator action records the event FIRST, then applies the patch** (`case/case-actions.ts`; - > `recordCaseEvent` makes the action row and the audit row one transaction). A failure between - > the two leaves an action **recorded but not applied** — never an unaudited state change. Bulk - > assign takes the same side, auditing before it mutates. - > - **A run upserts the case FIRST, then audits best-effort.** A failure there leaves a state change - > **applied but unaudited**, which is the violation — accepted because the alternative strands an - > otherwise-complete run as RUNNING after the case was already mutated. + > **This correction was itself too broad on its first cut** (Codex review), which is worth recording + > because it is the same failure one level up: it said "operator actions audit first and cannot lose + > the event", true of CASE actions and false of several other operator surfaces. The list below came + > from a sweep for the mutate-before-audit shape rather than from memory. > - > **What is missing is the primitive, not the ordering.** There is no `applyCaseAction({ patch, + > **AUDIT FIRST — cannot produce an unaudited state change:** + > - every case action (`case/case-actions.ts`), where `recordCaseEvent` makes the action row and the + > audit row one transaction and the patch follows; + > - rerun-to-verify's case patch (`case/case-rerun.ts`), which says so at the call site; + > - bulk assign and panel backfill, through the batch `recordCaseEvents`. + > + > A failure between the two leaves an action **recorded but not applied** — recoverable, and never a + > silent state change. + > + > **MUTATE FIRST — can apply a change and lose the event:** + > - the **run-created case transition** (`run/run-pipeline.ts`), which audits best-effort after the + > upsert. **This one is deliberate**: the alternative strands an otherwise-complete run as RUNNING + > after the case was already mutated; + > - the **measure lifecycle** — create, approve, deprecate and the explicit status transition + > (`measure/measure-lifecycle.ts`); + > - **segment create** (`routes/segments.ts`) and **terminology-mapping create** + > (`measure/value-set-governance.ts`). + > + > Only the first is a considered trade. The rest are simply the order they were written in, and new + > code should audit first. + > + > **What is missing is the primitive, not the ordering** (for the run; for the others the ordering + > is missing too)**.** There is no `applyCaseAction({ patch, > action, audit })` making all three one unit, and there cannot be one inside a single store: the > action and audit rows belong to `CaseEventStore` (which already opens its own `BEGIN`/`COMMIT`) > while the patch belongs to `CaseStore`, so a real fix needs a transaction seam spanning both. diff --git a/docs/JOURNAL.md b/docs/JOURNAL.md index 5df2dca62..30e9799e3 100644 --- a/docs/JOURNAL.md +++ b/docs/JOURNAL.md @@ -31,6 +31,24 @@ run-created transitions.** The correction is in `DATA_MODEL_CONTRACTS` §4 beside the best-effort note and in `CLAUDE.md`'s rule itself, because a rule whose exception lives in a source file is a rule a session will contradict. +**And the correction was itself too broad** (Codex, on the PR). It said "operator actions audit +first and cannot lose the event" — true of CASE actions, false of several other operator surfaces. +The same failure one level up, in a change whose entire subject is claims being wider than what is +behind them. + +So the paths were enumerated by a sweep for the shape rather than recalled. **Audit-first:** every +case action, rerun-to-verify's case patch, bulk assign and panel backfill. **Mutate-first, and so +able to lose the event:** the run-created case transition, the measure lifecycle (create, approve, +deprecate, transition), segment create, terminology-mapping create. + +`case-rerun.ts` looked like a violation to the sweep and is not — its first mutation creates a RUN +row, and the case patch is explicitly after an audit-first `recordCaseEvent`, with a comment saying +so. Worth the check: it would have been an easy thing to assert wrongly in the other direction. + +**Only the run's ordering is a considered trade.** The other six are the order they happened to be +written in, which splits #598 into a cheap half — flip them, no seam needed — and the primitive that +still needs a cross-store transaction. + ### #599 — an approval gate that could not fail on the thing its label implied Studio rendered "Test Fixtures ✅" and blocked activation until it passed, which reads as *the