From 11514bc381b82cdb778e447b13f712a5370f4eac Mon Sep 17 00:00:00 2001 From: Taleef Date: Sat, 25 Jul 2026 17:03:42 -0400 Subject: [PATCH 1/3] feat(standards): per-measure executor router, wired and dark (PR-7b) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit routedEngineForEnv(env) replaces engineForEnv(env) at all 8 call sites — runs (3), cases, measures (2), compliance-simulation, scheduler. Measures named in WORKWELL_OFFICIAL_MEASURES evaluate through the official artifact; everything else is unchanged. With the flag unset it returns the authored engine ITSELF. Identity, not equivalence: no dispatch, no allocation, nothing to reason about on the path every environment is actually on. The parity test asserts routed === authored rather than comparing two engines' outputs, because identity is a fact where output-comparison is a claim about two code paths agreeing on the inputs someone thought to try. Everything is validated at construction, and construction throws. A misconfiguration must not survive to the first subject: by then a run is underway, outcomes are being written, and the failure mode of most of these mistakes is silence. Every named measure must be covered by the MADiE gate, have a vendored artifact whose catalogId matches, have recorded numerator semantics, and — the invisible one — have every referenced value set expand non-empty. All problems are reported at once, not the first. "all" is refused: it is a measure name like any other and there is no measure called that. Every flip stays a deliberate per-measure act. An explicit elm/metaOverride always stays authored, even for a routed measure. The fidelity lab evaluates an official-SUBSET measure through that seam and the Rule Builder previews generated CQL the same way; routing those to the official executor would silently run a different measure than the caller asked for. Terminology expansion is now scoped to one run — memoized per measure per executor instance, the instance living exactly as long as the router. That is the middle of the two bad options review named: per-call was thousands of store reads in a population run; per-process would freeze the snapshot and re-introduce the stale-expansion bug engine-factory.ts documents. A rejected expansion is deliberately not cached. official-measures joins the boot seam line as the 11th seam, and the only one that changes what a measure COMPUTES rather than how or where. The boundary guard corrected me again: I pre-emptively added the router to the fqm consumer allowlist and it failed, because the router imports the adapter, not the package. Reverted, with the reason written where the next person looks. Not yet safe to switch on, and the module says so: no bundle preparation (stampQiCoreStructure), no measure-major batching. Both are PR-8's. Typecheck clean; 1482 pass / 0 fail / 14 skipped. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Np757pAsNQSCPiq8S7qsxB --- backend-ts/src/admin/scheduler.ts | 4 +- backend-ts/src/config/seam-inventory.test.ts | 7 +- backend-ts/src/config/seam-inventory.ts | 8 +- backend-ts/src/routes/cases.ts | 4 +- .../src/routes/compliance-simulation.ts | 4 +- backend-ts/src/routes/measures.ts | 6 +- backend-ts/src/routes/runs.ts | 8 +- .../src/standards/fqm-isolation.test.ts | 5 +- backend-ts/src/wiring/executor-router.test.ts | 165 ++++++++++++++++++ backend-ts/src/wiring/executor-router.ts | 157 +++++++++++++++++ .../src/wiring/official-executor-adapter.ts | 42 ++++- backend-ts/src/wiring/official-routing.ts | 12 ++ docs/JOURNAL.md | 49 ++++++ docs/ROADMAP_2026-07-24.md | 11 +- 14 files changed, 459 insertions(+), 23 deletions(-) create mode 100644 backend-ts/src/wiring/executor-router.test.ts create mode 100644 backend-ts/src/wiring/executor-router.ts diff --git a/backend-ts/src/admin/scheduler.ts b/backend-ts/src/admin/scheduler.ts index 706470c9..844b556c 100644 --- a/backend-ts/src/admin/scheduler.ts +++ b/backend-ts/src/admin/scheduler.ts @@ -17,7 +17,7 @@ import type { Stores, StoresEnv } from "../stores/factory.ts"; import { getStores } from "../stores/factory.ts"; import type { HydratedSegment } from "../stores/segment-store.ts"; import type { EvaluateMeasureBinding } from "../engine/evaluate-measure.ts"; -import { engineForEnv } from "../wiring/engine-factory.ts"; +import { routedEngineForEnv } from "../wiring/executor-router.ts"; import type { EmployeeProfile } from "../engine/synthetic/employee-catalog.ts"; import { ensureSegmentSeed } from "../segment/segment-seed.ts"; import { @@ -330,7 +330,7 @@ export async function schedulerTick( try { await ensureSegmentSeed(env); const stores = await getStores(env); - const engine = await engineForEnv(env); + const engine = await routedEngineForEnv(env); const allSegments = await stores.segments.listSegments(); const enabledSegments = allSegments.filter((s) => s.enabled); await runTick({ stores, engine, segments: enabledSegments, alertChannels, webChartEnv: env, incremental: isIncrementalEnabled(env), expansionActive: isVsacConfigured(env) }); diff --git a/backend-ts/src/config/seam-inventory.test.ts b/backend-ts/src/config/seam-inventory.test.ts index 15fdfbb6..40431873 100644 --- a/backend-ts/src/config/seam-inventory.test.ts +++ b/backend-ts/src/config/seam-inventory.test.ts @@ -20,7 +20,7 @@ test("describeSeams: everything off with no env vars set", () => { const seams = describeSeams({}); assert.deepEqual( seams.map((s) => s.name), - ["sendgrid", "datachaser", "ice", "eh-fhir", "webchart", "sql-executor", "vsac", "alert-webhook", "bucket-s3", "incremental-eval"], + ["sendgrid", "datachaser", "ice", "eh-fhir", "webchart", "sql-executor", "vsac", "alert-webhook", "bucket-s3", "incremental-eval", "official-measures"], ); for (const s of seams) assert.equal(s.active, false, `${s.name} should default off`); }); @@ -28,7 +28,7 @@ test("describeSeams: everything off with no env vars set", () => { test("formatSeamLogLine: all-off shape matches the documented boot log line", () => { assert.equal( formatSeamLogLine({}), - "seams: sendgrid=off datachaser=off ice=off eh-fhir=off webchart=off sql-executor=off vsac=off alert-webhook=off bucket-s3=off incremental-eval=off", + "seams: sendgrid=off datachaser=off ice=off eh-fhir=off webchart=off sql-executor=off vsac=off alert-webhook=off bucket-s3=off incremental-eval=off official-measures=off", ); }); @@ -242,9 +242,10 @@ test("formatSeamLogLine: all-on shape when every seam is configured", () => { WORKWELL_BUCKET_S3_ACCESS_KEY_ID: "AKIA", WORKWELL_BUCKET_S3_SECRET_ACCESS_KEY: "s", WORKWELL_INCREMENTAL_EVAL: "true", + WORKWELL_OFFICIAL_MEASURES: "cms122,cms125", }; assert.equal( formatSeamLogLine(allOn), - "seams: sendgrid=on datachaser=on ice=on eh-fhir=on webchart=on sql-executor=on vsac=on alert-webhook=on bucket-s3=on incremental-eval=on", + "seams: sendgrid=on datachaser=on ice=on eh-fhir=on webchart=on sql-executor=on vsac=on alert-webhook=on bucket-s3=on incremental-eval=on official-measures=on", ); }); diff --git a/backend-ts/src/config/seam-inventory.ts b/backend-ts/src/config/seam-inventory.ts index faeec823..05b15cf6 100644 --- a/backend-ts/src/config/seam-inventory.ts +++ b/backend-ts/src/config/seam-inventory.ts @@ -27,6 +27,7 @@ import { isSqlPushdownSelected, type MeasureExecutorEnv } from "../engine/measur import { isVsacConfigured, type VsacEnv } from "../engine/cql/resolve-value-set-resolver.ts"; import { isAlertWebhookConfigured, type AlertEnv } from "../run/alert-channel.ts"; import { isIncrementalEnabled, type IncrementalEnv } from "../run/incremental/incremental-eval.ts"; +import { isOfficialRoutingConfigured, type OfficialMeasuresEnv } from "../wiring/official-routing.ts"; /** The union of every seam's env-var shape (all optional — assignable from the worker's `Env`). */ export type SeamEnv = EmailEnv & @@ -38,7 +39,8 @@ export type SeamEnv = EmailEnv & VsacEnv & AlertEnv & BucketSeamEnv & - IncrementalEnv; + IncrementalEnv & + OfficialMeasuresEnv; export interface SeamStatus { /** Short, stable, log-line-friendly seam name. */ @@ -62,6 +64,10 @@ export function describeSeams(env: SeamEnv): SeamStatus[] { { name: "alert-webhook", active: isAlertWebhookConfigured(env) }, { name: "bucket-s3", active: isS3BucketConfigured(env) }, { name: "incremental-eval", active: isIncrementalEnabled(env) }, + // The 11th seam, and the only one that changes what a measure COMPUTES rather than how or where. + // It belongs on the boot line for exactly that reason: "which measures did this container evaluate + // officially" must be answerable from a log, not inferred from a deploy config. + { name: "official-measures", active: isOfficialRoutingConfigured(env) }, ]; } diff --git a/backend-ts/src/routes/cases.ts b/backend-ts/src/routes/cases.ts index 8fc81259..42d71192 100644 --- a/backend-ts/src/routes/cases.ts +++ b/backend-ts/src/routes/cases.ts @@ -20,7 +20,7 @@ import type { CloudDatabase, CloudBucket } from "@mieweb/cloud"; import { getStores } from "../stores/factory.ts"; import type { CaseStore } from "../stores/case-store.ts"; import type { OutcomeStore } from "../stores/outcome-store.ts"; -import { engineForEnv } from "../wiring/engine-factory.ts"; +import { routedEngineForEnv } from "../wiring/executor-router.ts"; import { toCaseSummary, type CaseSummary } from "../case/case-read-models.ts"; import { bucketPeriodForMeasure } from "../run/compliance-period.ts"; import { toCaseDetail } from "../case/case-detail-read-model.ts"; @@ -86,7 +86,7 @@ async function actionDeps(env: CasesEnv): Promise { const s = await getStores(env); - const engine = await engineForEnv(env); + const engine = await routedEngineForEnv(env); return { cases: s.cases, events: s.events, outcomes: s.outcomes, runStore: s.runs, engine }; } async function evidenceDeps(env: CasesEnv): Promise { diff --git a/backend-ts/src/routes/compliance-simulation.ts b/backend-ts/src/routes/compliance-simulation.ts index 4e2c85e4..9311fc0e 100644 --- a/backend-ts/src/routes/compliance-simulation.ts +++ b/backend-ts/src/routes/compliance-simulation.ts @@ -5,7 +5,7 @@ * (all roles, like the immunization forecast). Writes nothing; no schema. handleEmployees only matches * `/profile` + `/search`, so this `/simulate` path is not intercepted. */ -import { engineForEnv } from "../wiring/engine-factory.ts"; +import { routedEngineForEnv } from "../wiring/executor-router.ts"; import type { StoresEnv } from "../stores/factory.ts"; import { parseQueryDate, QueryDateError } from "./query-dates.ts"; import { simulateComplianceAsOf } from "../run/employee-compliance-snapshot.ts"; @@ -35,7 +35,7 @@ export async function handleComplianceSimulation(req: Request, env: StoresEnv): } const today = new Date().toISOString().slice(0, 10); - const engine = await engineForEnv(env); + const engine = await routedEngineForEnv(env); const snapshot = await simulateComplianceAsOf(externalId, asOf ?? today, { engine, today }); if (!snapshot) return json({ error: "not_found", externalId }, 404); return json(snapshot); diff --git a/backend-ts/src/routes/measures.ts b/backend-ts/src/routes/measures.ts index 7c24f967..46bdf3e0 100644 --- a/backend-ts/src/routes/measures.ts +++ b/backend-ts/src/routes/measures.ts @@ -21,7 +21,7 @@ import type { CloudDatabase } from "@mieweb/cloud"; import { getStores } from "../stores/factory.ts"; import type { MeasureStore } from "../stores/measure-store.ts"; import type { ValueSetStore } from "../stores/value-set-store.ts"; -import { engineForEnv } from "../wiring/engine-factory.ts"; +import { routedEngineForEnv } from "../wiring/executor-router.ts"; import { MEASURES } from "../engine/cql/measure-registry.ts"; import { ELM_LIBRARIES } from "../engine/cql/elm/index.ts"; import { compileCql, reconstructCql } from "../engine/cql/cql-translator.ts"; @@ -314,7 +314,7 @@ export async function handleMeasures(req: Request, env: MeasuresEnv, actor = "sy const body = (await req.json().catch(() => ({}))) as ImpactPreviewRequest; try { const s = await getStores(env); - const engine = await engineForEnv(env); + const engine = await routedEngineForEnv(env); const deps = { cases: s.cases, events: s.events, engine }; return json(await previewImpact(deps, measure, body, actor)); } catch (err) { @@ -358,7 +358,7 @@ export async function handleMeasures(req: Request, env: MeasuresEnv, actor = "sy const bundle = (await req.json().catch(() => null)) as unknown; if (!bundle || typeof bundle !== "object") return json({ error: "invalid_bundle" }, 400); try { - const engine = await engineForEnv(env); + const engine = await routedEngineForEnv(env); const outcome = await engine.evaluate({ measureId: evalId, patientBundle: bundle, evaluationDate: url.searchParams.get("date") ?? undefined }); return json(outcome); } catch (err) { diff --git a/backend-ts/src/routes/runs.ts b/backend-ts/src/routes/runs.ts index 5b3b4832..a7001812 100644 --- a/backend-ts/src/routes/runs.ts +++ b/backend-ts/src/routes/runs.ts @@ -25,7 +25,7 @@ import type { OutcomeStore } from "../stores/outcome-store.ts"; import type { CaseStore } from "../stores/case-store.ts"; import type { HydratedSegment } from "../stores/segment-store.ts"; import { ensureSegmentSeed } from "../segment/segment-seed.ts"; -import { engineForEnv } from "../wiring/engine-factory.ts"; +import { routedEngineForEnv } from "../wiring/executor-router.ts"; import { toRunListItemFromCounts, toRunSummaryFromCounts, toRunLogEntries, toRunOutcomeRows, matchesRunFilters, type RunFilters } from "../run/read-models.ts"; import { recoverStuckRuns } from "../run/recover-stuck-runs.ts"; import { resolveAlertChannels } from "../run/alert-channel.ts"; @@ -287,7 +287,7 @@ export async function handleRuns( if (pathname === "/api/runs/manual" && req.method === "POST") { const body = (await req.json().catch(() => ({}))) as ManualRunRequest; body.triggeredBy = externalTriggeredBy(body.triggeredBy); // Fable M1: no forged seed:*/scheduler labels - const engine = await engineForEnv(env); + const engine = await routedEngineForEnv(env); const deps = { runStore: await store(env), outcomeStore: await outcomes(env), @@ -320,7 +320,7 @@ export async function handleRuns( const rerunId = pathname.match(/^\/api\/runs\/([^/]+)\/rerun$/)?.[1]; if (rerunId && req.method === "POST") { const runStore = await store(env); - const engine = await engineForEnv(env); + const engine = await routedEngineForEnv(env); // A CASE run reruns through rerun-to-verify (the case scope), reading the caseId // persisted in requested_scope — matches Java's rerunSameScope CASE branch. Other // scopes go through executeRerun. @@ -418,7 +418,7 @@ export async function handleRuns( // A run being processed must leave the QUEUED claim path so it isn't re-handed // to a worker (QUEUED → RUNNING; idempotent for already-running runs). await runStore.markRunning(evalId); - const engine = await engineForEnv(env); + const engine = await routedEngineForEnv(env); try { const result = await engine.evaluate({ measureId: body.measureId, diff --git a/backend-ts/src/standards/fqm-isolation.test.ts b/backend-ts/src/standards/fqm-isolation.test.ts index 7a48065f..204721bf 100644 --- a/backend-ts/src/standards/fqm-isolation.test.ts +++ b/backend-ts/src/standards/fqm-isolation.test.ts @@ -191,8 +191,9 @@ const EXECUTOR_IMPORTERS_ALLOWLIST = [ // until a measure is actually flipped. "wiring/official-executor-adapter.ts", // NOT run/cli/official-cases.ts — that shell reaches fqm only through standards/official-cases.ts, - // which is exactly the layering this list is meant to preserve. - // PR-7b adds "wiring/executor-router.ts" here — deliberately a conscious edit, not an open door. + // which is exactly the layering this list is meant to preserve. Same for PR-7b's + // `wiring/executor-router.ts`: it consumes the ADAPTER, so the package stays one hop away from the + // thing production routes through. Adding it here pre-emptively was wrong, and this guard said so. ]; test("5/5 consumers: only the approved app layers may import @workwell/official-executor", () => { diff --git a/backend-ts/src/wiring/executor-router.test.ts b/backend-ts/src/wiring/executor-router.test.ts new file mode 100644 index 00000000..bb177de4 --- /dev/null +++ b/backend-ts/src/wiring/executor-router.test.ts @@ -0,0 +1,165 @@ +/** + * Per-measure execution routing (PR-7b). + * + * The most important test here is the boring one: with the flag unset, `routedEngineForEnv` returns the + * authored engine ITSELF. Every environment that exists today is on that path. + */ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + officialRoutingProblems, + routedEngineForEnv, + storeValueSetExpander, +} from "./executor-router.ts"; +import type { EvaluateMeasureBinding, MeasureOutcome } from "../engine/evaluate-measure.ts"; +import type { ValueSetStore } from "../stores/value-set-store.ts"; + +const outcome = (measure: string): MeasureOutcome => ({ + subjectId: "s1", + measure, + outcome: "COMPLIANT", + evidence: { expressionResults: [] }, +}); + +const authoredEngine = (): EvaluateMeasureBinding & { calls: string[] } => { + const calls: string[] = []; + return { + calls, + async evaluate(input) { + calls.push(input.measureId); + return outcome(`authored:${input.measureId}`); + }, + }; +}; + +test("unset means the authored engine ITSELF — identity, not an equivalent wrapper", async () => { + const authored = authoredEngine(); + for (const env of [{}, { WORKWELL_OFFICIAL_MEASURES: "" }, { WORKWELL_OFFICIAL_MEASURES: " " }]) { + const routed = await routedEngineForEnv(env as never, { authored }); + assert.equal( + routed, + authored, + "the default path must have no dispatch layer at all — identity is what makes 'byte-identical' " + + "a fact rather than a claim about two code paths agreeing", + ); + } + // A non-string value is ignored rather than coerced (e.g. a YAML `true`). + const coerced = await routedEngineForEnv({ WORKWELL_OFFICIAL_MEASURES: true } as never, { authored }); + assert.equal(coerced, authored); +}); + +test("a named measure routes to the official executor; everything else stays authored", async () => { + const authored = authoredEngine(); + const routed = await routedEngineForEnv( + { WORKWELL_OFFICIAL_MEASURES: "cms122" } as never, + { authored, expand: async () => [{ code: "a", system: "s" }] }, + ); + + const official = await routed.evaluate({ measureId: "cms122", patientBundle: {} }).catch((e) => e); + // It reached the official executor: the failure is fqm's, not a routing miss. (Actually executing + // the artifact needs the real calculator, which this test deliberately does not load.) + assert.ok(official instanceof Error, "the official path is exercised"); + assert.deepEqual(authored.calls, [], "cms122 must NOT have reached the authored engine"); + + const other = await routed.evaluate({ measureId: "audiogram", patientBundle: {} }); + assert.equal(other.measure, "authored:audiogram"); + assert.deepEqual(authored.calls, ["audiogram"]); +}); + +test("an explicit elm/metaOverride always stays authored, even for a routed measure", async () => { + // The fidelity lab evaluates an official-SUBSET measure through the engine's metaOverride seam, and + // the Rule Builder previews generated CQL the same way. Routing those to the official executor would + // silently run a different measure than the caller asked for. + const authored = authoredEngine(); + const routed = await routedEngineForEnv( + { WORKWELL_OFFICIAL_MEASURES: "cms122" } as never, + { authored, expand: async () => [{ code: "a", system: "s" }] }, + ); + + await routed.evaluate({ measureId: "cms122", patientBundle: {}, elm: { library: {} } }); + await routed.evaluate({ + measureId: "cms122", + patientBundle: {}, + metaOverride: { name: "x", library: "L", periodMonths: 12 } as never, + }); + assert.deepEqual(authored.calls, ["cms122", "cms122"]); +}); + +test("construction refuses every shape of misconfiguration, and says which", async () => { + const authored = authoredEngine(); + const build = (value: string) => + routedEngineForEnv({ WORKWELL_OFFICIAL_MEASURES: value } as never, { + authored, + expand: async () => [{ code: "a", system: "s" }], + }); + + // A typo must not silently serve authored results while the configuration claims official execution. + await assert.rejects(() => build("cms999"), /cms999: not covered by the official MADiE test-case gate/); + // "all" is a measure name like any other, and there is no measure called "all". + await assert.rejects(() => build("all"), /all: not covered/); + await assert.rejects(() => build("cms122,cms165"), /cms165: not covered/); + + // Whitespace tolerance, but only around real ids. + const ok = await build(" cms122 , cms125 "); + assert.notEqual(ok, authored, "a valid list must produce a router"); +}); + +test("officialRoutingProblems names the gate, the artifact, and the semantics separately", () => { + assert.deepEqual(officialRoutingProblems({}), [], "unset is always legal"); + assert.deepEqual(officialRoutingProblems({ WORKWELL_OFFICIAL_MEASURES: "cms122,cms125" }), []); + + // ALL the problems, not the first: an operator fixing one at a time, learning about the next only + // after a redeploy, is how a five-minute configuration takes an afternoon. cms130 is both ungated and + // unvendored, and hears about both. + const problems = officialRoutingProblems({ WORKWELL_OFFICIAL_MEASURES: "cms130" }); + assert.equal(problems.length, 2); + assert.match(problems[0]!, /cms130: not covered by the official MADiE test-case gate/); + assert.match(problems[1]!, /cms130: no executable official artifact is vendored/); +}); + +test("terminology is preflighted at CONSTRUCTION, not at first evaluation", async () => { + // The whole reason: by the time a subject is evaluated a run is underway and outcomes are being + // written, and this failure's natural mode is silence — an unexpandable value set makes every + // retrieve match nothing, so the measure reports an empty population rather than an error. + const authored = authoredEngine(); + let expandCalls = 0; + await assert.rejects( + () => + routedEngineForEnv({ WORKWELL_OFFICIAL_MEASURES: "cms122" } as never, { + authored, + expand: async () => { + expandCalls += 1; + return []; // nothing imported — the state the live stack is in today + }, + }), + /value sets could not be expanded[\s\S]*resolve-valuesets/, + ); + assert.ok(expandCalls > 0, "it must actually try, not just inspect the manifest"); +}); + +test("the store expander snapshots ONCE per instance — per run, never per process", async () => { + // Per call would be thousands of store reads in a population run. Per process would freeze the + // snapshot, re-introducing the exact bug engine-factory.ts documents: an operator's value-set edit + // serving stale expansions until restart. The instance lives for one run, which is the middle. + let reads = 0; + const store = { + listAll: async () => { + reads += 1; + return [ + { oid: "2.16.1", codes: [{ code: "a", system: "s" }] }, + // A row imported under its URL form still resolves by bare OID. + { oid: "http://cts.nlm.nih.gov/fhir/ValueSet/2.16.2", codes: [{ code: "b", system: "s" }] }, + ]; + }, + } as unknown as ValueSetStore; + + const expand = storeValueSetExpander(store); + assert.deepEqual(await expand("2.16.1"), [{ code: "a", system: "s" }]); + assert.deepEqual(await expand("2.16.2"), [{ code: "b", system: "s" }]); + assert.deepEqual(await expand("2.16.404"), [], "an unimported OID is empty, and the executor refuses"); + assert.equal(reads, 1, "one bounded catalog read for the instance's whole lifetime"); + + // A second instance re-reads: that is what makes an operator's edit visible on the next run. + await storeValueSetExpander(store)("2.16.1"); + assert.equal(reads, 2); +}); diff --git a/backend-ts/src/wiring/executor-router.ts b/backend-ts/src/wiring/executor-router.ts new file mode 100644 index 00000000..005cee84 --- /dev/null +++ b/backend-ts/src/wiring/executor-router.ts @@ -0,0 +1,157 @@ +/** + * Per-measure execution routing (roadmap §7.2, PR-7b). + * + * `routedEngineForEnv(env)` returns the thing every caller already asks `engineForEnv(env)` for, except + * that measures named in `WORKWELL_OFFICIAL_MEASURES` are evaluated by the OFFICIAL published artifact + * instead of WorkWell's authored CQL. Nothing downstream changes shape: the run pipeline, the case + * rerun path, the scheduler and the simulate route keep the exact call they make today. + * + * **Unset — every environment that exists right now — returns `engineForEnv(env)` itself**, not a + * wrapper around it. Identity, not equivalence: there is no dispatch, no allocation, and nothing to + * reason about on the default path. A parity test asserts it. + * + * ## Why the flag is a list and never "all" + * + * Flipping a measure to official execution changes what an operator is told about real people. It is a + * deliberate per-measure act gated on a green MADiE test-case run, so the configuration is an explicit + * allowlist. `WORKWELL_OFFICIAL_MEASURES=all` selects a measure called "all", which does not exist, and + * is therefore refused — the same as any other typo. + * + * ## Everything is checked at CONSTRUCTION, and construction fails loudly + * + * A misconfiguration must not survive until the first subject is evaluated. By then a run is underway, + * outcomes are being written, and the failure mode of most of these mistakes is silence rather than an + * error. So the router refuses to exist unless, for every named measure: + * + * 1. the official MADiE gate covers it (`ungatedOfficialMeasures` — THE RULE, roadmap §7.4 PR-6); + * 2. an executable artifact is vendored, and its `catalogId` matches; + * 3. WorkWell has recorded what its numerator means (`official-measure-semantics.ts`); + * 4. every value set its ELM retrieves expands to a non-empty set (the preflight). + * + * (4) is the one that would otherwise be invisible: fqm treats an unexpandable value set as *empty + * rather than missing*, an empty set matches nothing, and the measure then reports every subject + * out-of-population — which reads downstream exactly like a genuinely ineligible roster. + * + * ## Scope of this PR + * + * Ships dark. It also does NOT yet prepare bundles (`stampQiCoreStructure`, see the adapter's docstring) + * or batch subjects measure-major, so it must not be flipped on for a population run until PR-8 wires + * both. The flag existing and the flag being safe to set are different things, and this PR only delivers + * the first. + */ +import type { EvaluateMeasureBinding, EvaluateMeasureInput, MeasureOutcome } from "../engine/evaluate-measure.ts"; +import type { MeasureMeta } from "../engine/cql/measure-registry.ts"; +import { getStores, type StoresEnv } from "../stores/factory.ts"; +import type { ValueSetStore } from "../stores/value-set-store.ts"; +import type { VsacEnv } from "../engine/cql/resolve-value-set-resolver.ts"; +import { OFFICIAL_GATED_MEASURES } from "../standards/official-cases.ts"; +import { engineForEnv } from "./engine-factory.ts"; +import { loadOfficialArtifact } from "./official-artifacts.ts"; +import { officialMeasureExecutor, type ExpandValueSet } from "./official-executor-adapter.ts"; +import { + officialMeasureIds, + ungatedOfficialMeasures, + type OfficialMeasuresEnv, +} from "./official-routing.ts"; +import { officialMeasureSemantics } from "./official-measure-semantics.ts"; + +/** The extended shape the authored engine accepts — diagnostics pass an explicit library to run. */ +export type RoutableInput = EvaluateMeasureInput & { elm?: unknown; metaOverride?: MeasureMeta }; + +export interface RoutedEngine { + evaluate(input: RoutableInput): Promise; +} + +/** + * Expand a VSAC OID from the imported `value_sets` rows. + * + * One bounded catalog read, snapshotted for the expander's lifetime — which is the router's, which is + * one run. `listAll()` rather than a per-OID query because the store has no OID lookup and the catalog + * is dozens of rows; adding one would be a store-contract change for no gain at this size. + */ +export function storeValueSetExpander(valueSets: ValueSetStore): ExpandValueSet { + let snapshot: Promise>> | undefined; + const load = () => { + snapshot ??= valueSets.listAll().then((rows) => { + const byOid = new Map>(); + for (const row of rows) { + // VSAC canonicals are keyed by bare OID; a row imported under a URL form still resolves. + const oid = row.oid.includes("/") ? row.oid.slice(row.oid.lastIndexOf("/") + 1) : row.oid; + byOid.set(oid, (row.codes ?? []).map((c) => ({ code: c.code, system: c.system }))); + } + return byOid; + }); + return snapshot; + }; + return async (oid) => (await load()).get(oid) ?? []; +} + +/** Everything wrong with the current `WORKWELL_OFFICIAL_MEASURES`, as sentences. Empty means legal. */ +export function officialRoutingProblems(env: OfficialMeasuresEnv): string[] { + const problems: string[] = []; + for (const id of ungatedOfficialMeasures([...OFFICIAL_GATED_MEASURES], env as Record)) { + problems.push( + `${id}: not covered by the official MADiE test-case gate — no measure may be routed officially ` + + `without external validation (roadmap §7.4 PR-6)`, + ); + } + for (const id of officialMeasureIds(env as Record)) { + const artifact = loadOfficialArtifact(id); + if (!artifact) { + problems.push(`${id}: no executable official artifact is vendored (see measures/official/)`); + continue; + } + if (artifact.manifest.catalogId !== id) { + problems.push(`${id}: the vendored artifact declares catalogId '${artifact.manifest.catalogId}'`); + } + if (!officialMeasureSemantics(id)) { + problems.push( + `${id}: no recorded numerator semantics — see official-measure-semantics.ts. There is no safe ` + + `default: guessing one way reports every failure as compliant, the other every success as overdue`, + ); + } + } + return problems; +} + +export interface RoutedEngineOptions { + /** Injectable for tests; defaults to the real store-backed expander. */ + expand?: ExpandValueSet; + /** Injectable for tests; defaults to the authored CQL engine. */ + authored?: EvaluateMeasureBinding; +} + +export async function routedEngineForEnv( + env: StoresEnv & VsacEnv & OfficialMeasuresEnv, + options: RoutedEngineOptions = {}, +): Promise { + const authored = options.authored ?? (await engineForEnv(env)); + const official = officialMeasureIds(env as Record); + // Identity on the default path — the wrapper below never exists in any environment today. + if (official.size === 0) return authored as RoutedEngine; + + const problems = officialRoutingProblems(env); + if (problems.length > 0) { + throw new Error( + `WORKWELL_OFFICIAL_MEASURES is not a valid configuration:\n - ${problems.join("\n - ")}`, + ); + } + + const expand = options.expand ?? storeValueSetExpander((await getStores(env)).valueSets); + const executor = officialMeasureExecutor({ expand }); + // Terminology, up front. Serially rather than in parallel: these hit the same snapshot and the first + // failure is the one worth reporting, unqualified by a race. + for (const id of official) await executor.preflight(id); + + return { + async evaluate(input: RoutableInput): Promise { + // An explicit `elm`/`metaOverride` means "run THIS library" — the fidelity lab evaluating an + // official-subset measure, or the Rule Builder previewing generated CQL. Routing those to the + // official executor would silently run a different measure than the caller asked for. + const overridden = input.elm !== undefined || input.metaOverride !== undefined; + return official.has(input.measureId) && !overridden + ? executor.evaluate(input) + : authored.evaluate(input); + }, + }; +} diff --git a/backend-ts/src/wiring/official-executor-adapter.ts b/backend-ts/src/wiring/official-executor-adapter.ts index d6728af8..2f914d84 100644 --- a/backend-ts/src/wiring/official-executor-adapter.ts +++ b/backend-ts/src/wiring/official-executor-adapter.ts @@ -87,6 +87,11 @@ import { officialMeasureSemantics } from "./official-measure-semantics.ts"; /** Expand one VSAC OID to its codes — the app supplies this from the imported `value_sets` rows. */ export type ExpandValueSet = (oid: string) => Promise; +/** An `EvaluateMeasureBinding` that can also be asked to prove a measure is runnable before a run. */ +export interface OfficialMeasureExecutor extends EvaluateMeasureBinding { + preflight(measureId: string): Promise; +} + export interface OfficialExecutorDeps { expand: ExpandValueSet; /** Injectable for tests; defaults to the real (lazily imported) fqm calculator. */ @@ -271,8 +276,41 @@ export function requiredValueSets(artifact: OfficialArtifact): Array<{ oid: stri * costs an ELM parse per subject, which is why PR-7b adds measure-major iteration and batches subjects * through one `calculateOfficialDetailed` call — the package's batch entry point already exists for it. */ -export function officialMeasureExecutor(deps: OfficialExecutorDeps): EvaluateMeasureBinding { +export function officialMeasureExecutor(deps: OfficialExecutorDeps): OfficialMeasureExecutor { + /** + * Terminology is expanded once per measure per EXECUTOR INSTANCE, and the instance's lifetime is one + * run (the router is built per run, exactly as `engineForEnv` is). That scoping is the point, not an + * implementation detail: a process-lifetime cache would freeze the value-set snapshot and re-introduce + * the bug `engineForEnv` documents at length — an operator's value-set edit serving stale expansions + * until restart. Per-call expansion was the other extreme: a 150-subject run would re-parse a 2.4MB + * bundle's ELM twice per subject and hit the store thousands of times. + */ + const terminology = new Map>(); + const cacheFor = (artifact: OfficialArtifact): Promise => { + const key = artifact.manifest.catalogId; + let pending = terminology.get(key); + if (!pending) { + pending = expandArtifactTerminology(artifact, deps.expand); + // A rejected promise must not be cached: the likeliest cause is a transient store failure, and + // caching it would turn one bad read into a whole run's worth of refusals. + pending.catch(() => terminology.delete(key)); + terminology.set(key, pending); + } + return pending; + }; + return { + /** + * Expand a measure's terminology now rather than at first evaluation. The router calls this at + * construction so an operator who flips a measure whose OIDs were never imported learns it from a + * failed run start, not from a run that quietly reports nobody as eligible. + */ + async preflight(measureId: string): Promise { + const artifact = (deps.loadArtifact ?? loadOfficialArtifact)(measureId); + if (!artifact) throw new Error(`${measureId}: no executable official artifact is vendored`); + await cacheFor(artifact); + }, + async evaluate(input: EvaluateMeasureInput): Promise { const artifact = (deps.loadArtifact ?? loadOfficialArtifact)(input.measureId); if (!artifact) { @@ -312,7 +350,7 @@ export function officialMeasureExecutor(deps: OfficialExecutorDeps): EvaluateMea end: normalizePeriodEnd(evaluationDate), }; - const valueSetCache = await expandArtifactTerminology(artifact, deps.expand); + const valueSetCache = await cacheFor(artifact); const results = await calculateOfficialDetailed({ bundle: artifact.bundle, patientBundles: [input.patientBundle], diff --git a/backend-ts/src/wiring/official-routing.ts b/backend-ts/src/wiring/official-routing.ts index 10043d24..ed2c3e0f 100644 --- a/backend-ts/src/wiring/official-routing.ts +++ b/backend-ts/src/wiring/official-routing.ts @@ -18,6 +18,18 @@ * Unset/blank (the demo stack and every environment today) ⇒ empty set ⇒ every read path is * byte-identical to before. */ +export interface OfficialMeasuresEnv { + WORKWELL_OFFICIAL_MEASURES?: string | undefined; +} + +/** + * The seam-inventory predicate, in the shape §10 of ARCHITECTURE requires: the boot line must call the + * SAME function the routing decision calls, never a second parse of the same variable. + */ +export function isOfficialRoutingConfigured(env: OfficialMeasuresEnv): boolean { + return officialMeasureIds(env as Record).size > 0; +} + export function officialMeasureIds(env: Record = process.env): ReadonlySet { const raw = env["WORKWELL_OFFICIAL_MEASURES"]; if (typeof raw !== "string") return new Set(); diff --git a/docs/JOURNAL.md b/docs/JOURNAL.md index 42a6ab52..260d1eed 100644 --- a/docs/JOURNAL.md +++ b/docs/JOURNAL.md @@ -1,5 +1,54 @@ # Journal +## 2026-07-25 (night) — PR-7b: the executor router, wired and dark (branch `feat/executor-router`) + +`routedEngineForEnv(env)` replaces `engineForEnv(env)` at all 8 call sites — runs (3), cases, measures +(2), compliance-simulation, scheduler. Measures named in `WORKWELL_OFFICIAL_MEASURES` evaluate through +the official artifact; everything else is unchanged. + +**With the flag unset it returns the authored engine ITSELF.** Identity, not equivalence — no dispatch, +no allocation, nothing to reason about on the path every environment is actually on. The parity test +asserts `routed === authored` rather than comparing two engines' outputs, because identity is a fact +where output-comparison is a claim about two code paths agreeing on the inputs someone thought to try. + +**Everything is validated at construction, and construction throws.** A misconfiguration must not +survive to the first subject: by then a run is underway, outcomes are being written, and the failure +mode of most of these mistakes is silence. So the router refuses to exist unless every named measure is +covered by the MADiE gate, has a vendored artifact whose `catalogId` matches, has recorded numerator +semantics, and — the invisible one — has every value set its ELM retrieves expanding to a non-empty set. +All problems are reported at once, not the first: fixing them one redeploy at a time is how a +five-minute configuration becomes an afternoon. + +`WORKWELL_OFFICIAL_MEASURES=all` is refused, because "all" is a measure name like any other and there is +no measure called that. Every flip stays a deliberate per-measure act. + +**An explicit `elm`/`metaOverride` always stays authored**, even for a routed measure. The fidelity lab +evaluates an official-*subset* measure through that seam and the Rule Builder previews generated CQL the +same way; routing those to the official executor would silently run a different measure than the caller +asked for. + +**Terminology expansion is now scoped to one run** — memoized per measure per executor instance, and the +instance lives exactly as long as the router, which is built per run like `engineForEnv`. That is the +middle of two bad options the adapter's review named: per-call was thousands of store reads in a +population run, per-process would freeze the snapshot and re-introduce the stale-expansion bug +`engine-factory.ts` documents at length. A rejected expansion is deliberately *not* cached, so one +transient store failure doesn't become a whole run of refusals. + +`official-measures` joins the boot seam line as the 11th seam — and the only one that changes what a +measure *computes* rather than how or where. Which measures a container evaluated officially must be +answerable from a log, not inferred from a deploy config. + +**The boundary guard corrected me again.** I pre-emptively added the router to the fqm consumer +allowlist; the guard failed because the router imports the *adapter*, not the package. Reverted, with +the reason written where the next person will look. + +**Not yet safe to switch on**, and the module says so in its own docstring: it does not prepare bundles +(`stampQiCoreStructure`) or batch subjects measure-major. The flag existing and the flag being safe to +set are different things, and this delivers the first. + +Typecheck clean; full suite **1482 pass / 0 fail / 14 skipped**. + + ## 2026-07-25 (evening) — PR-7c: the terminology importer reads the artifact, not a hand-kept list (branch `feat/official-valueset-import`) The smallest change that unblocks PR-9, and it exists because PR-7a's refusal exposed a list that had diff --git a/docs/ROADMAP_2026-07-24.md b/docs/ROADMAP_2026-07-24.md index a843953f..cac46cba 100644 --- a/docs/ROADMAP_2026-07-24.md +++ b/docs/ROADMAP_2026-07-24.md @@ -366,8 +366,15 @@ EvaluateMeasureBinding` dispatching on `input.measureId` — no downstream signa `populationResults` because it *is* them). This narrows a PR-6a claim: stripping keeps statement **names and count** — which is all a count check can verify — not statement **values**. *(shipped 2026-07-25.)* -7b. **PR-7b** — executor **router** (`routedEngineForEnv`) + boot-loud validation + logic_version override - + measure-major batching + flag-unset parity test. **Two obligations inherited from PR-3:** +7b. ✅ **PR-7b** — executor **router** (`routedEngineForEnv`) wired at all 8 `engineForEnv` call sites, + construction-time validation of every named measure (gate coverage, vendored artifact, recorded + semantics, terminology preflight — all problems reported at once), an `elm`/`metaOverride` escape so + the fidelity lab and Rule Builder stay authored, per-run-scoped terminology memoization, and + `official-measures` as the 11th boot seam. **Flag unset returns the authored engine ITSELF** — + identity, asserted, not an equivalent wrapper. Ships dark and is NOT yet safe to switch on: bundle + preparation (`stampQiCoreStructure`) and measure-major batching are PR-8's. `logic_version` override + deferred with them. *(shipped 2026-07-25.)* +7b-residual. **PR-7b residual** — logic_version override + measure-major batching, folded into PR-8. **Two obligations inherited from PR-3:** (a) it must write `populationResults` in one of the two shapes `officialMembership` accepts (the fqm-native `[{populationType, result}]` array, or the keyed `{ipp, denom, denex, numer, denexcep}`) — anything else is rejected and alerted, deliberately, so the halves cannot drift silently; and From 28d54f33a599f970fa99adb68ba9123eddef124c Mon Sep 17 00:00:00 2001 From: Taleef Date: Sat, 25 Jul 2026 17:48:26 -0400 Subject: [PATCH 2/3] fix(standards): the router would have split the manual and nightly run paths (PR-7b review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two criticals, and the first is the third instance of a bug this repo has already documented twice. C1. WORKWELL_OFFICIAL_MEASURES was missing from the scheduler's env allowlist in server.ts, while every field of OfficialMeasuresEnv is optional — so it type-checked. Once flipped on, POST /api/runs/manual would evaluate cms122 officially and the nightly ALL_PROGRAMS run — the one that populates /compliance, /programs and quality_snapshots — would evaluate it with the authored CQL. Two engines, two answers for one measure, latest-run-wins, official-measures=on on the boot line throughout. The comments directly above that allowlist describe the same bug happening to WORKWELL_WEBCHART_PRIVATE_KEY_B64 (#331) and WORKWELL_INCREMENTAL_EVAL (#263), so a test now asserts the keys are threaded: three times is a pattern, and comments have not stopped it. C2. Validation was construction-time where the roadmap said boot-loud — and I had reworded the plan item to match what I built, which is the "no silent scope changes" rule in reverse. routedEngineForEnv is lazy, so a typo'd flag would boot clean, log official-measures=on, keep /actuator/health green (deliberately DB-free, so the reconciler reports healthy) and 500 every evaluating route — the symptom profile of the four-day Neon outage. Boot now runs the same validation and emits WORKWELL_ALERT OFFICIAL_ROUTING_MISCONFIGURED. Four more: scoring was the one adapter refusal firing per-subject rather than at construction, and the run pipeline error-isolates a per-subject throw into MISSING_DATA — so a cohort artifact would have produced a successful run with every subject MISSING_DATA; /evaluate built the router after markRunning, so a config error orphaned a run; the "instance lifetime is one run" claim was false at three read routes (/simulate is a date scrubber, one construction per drag); and "all problems at once" excluded the terminology preflight, which is serial and first-failure. Two corrections of the same kind as the earlier false claims: the elm/metaOverride escape hatch cited the fidelity lab and Rule Builder as callers — neither goes through the router, so the guard is defensive, not load-bearing; and a test comment claimed it "deliberately does not load" the real calculator when it did, meaning assert.ok(err instanceof Error) would have passed on a MODULE_NOT_FOUND as happily as on a real routing hit. Also: ARCHITECTURE §10 updated to 11 seams, and oidFromValueSetUrl re-exported through the adapter so the router shares one normalization rule without becoming a second direct consumer of the fqm-quarantined package. Typecheck clean; 1486 pass / 0 fail / 14 skipped. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Np757pAsNQSCPiq8S7qsxB --- backend-ts/src/config/seam-inventory.ts | 5 +- backend-ts/src/routes/runs.ts | 5 +- backend-ts/src/server.ts | 6 ++ backend-ts/src/wiring/executor-router.test.ts | 77 +++++++++++++++++-- backend-ts/src/wiring/executor-router.ts | 56 +++++++++++--- .../src/wiring/official-executor-adapter.ts | 26 +++++-- backend-ts/src/worker.ts | 25 ++++++ docs/ARCHITECTURE.md | 9 ++- docs/JOURNAL.md | 39 +++++++++- docs/ROADMAP_2026-07-24.md | 7 +- 10 files changed, 225 insertions(+), 30 deletions(-) diff --git a/backend-ts/src/config/seam-inventory.ts b/backend-ts/src/config/seam-inventory.ts index 05b15cf6..fe78c1a6 100644 --- a/backend-ts/src/config/seam-inventory.ts +++ b/backend-ts/src/config/seam-inventory.ts @@ -65,8 +65,9 @@ export function describeSeams(env: SeamEnv): SeamStatus[] { { name: "bucket-s3", active: isS3BucketConfigured(env) }, { name: "incremental-eval", active: isIncrementalEnabled(env) }, // The 11th seam, and the only one that changes what a measure COMPUTES rather than how or where. - // It belongs on the boot line for exactly that reason: "which measures did this container evaluate - // officially" must be answerable from a log, not inferred from a deploy config. + // The line reports only on/off, like every other seam; WHICH measures are routed comes from the + // OFFICIAL_ROUTING_MISCONFIGURED alert and the run's own evidence. On/off is still the thing worth + // seeing at a glance, because it is the difference between two engines. { name: "official-measures", active: isOfficialRoutingConfigured(env) }, ]; } diff --git a/backend-ts/src/routes/runs.ts b/backend-ts/src/routes/runs.ts index a7001812..80fe7bcb 100644 --- a/backend-ts/src/routes/runs.ts +++ b/backend-ts/src/routes/runs.ts @@ -415,10 +415,13 @@ export async function handleRuns( // period, then today, mirroring that default. const evaluationPeriod = body.evaluationDate ?? (run.requestedScope.evaluationDate as string | undefined) ?? new Date().toISOString().slice(0, 10); + // Build the engine BEFORE claiming the run. `routedEngineForEnv` validates official-measure + // configuration and can throw; doing that after markRunning would leave an orphaned RUNNING run for + // `recoverStuckRuns` to sweep, for a failure that has nothing to do with the run. + const engine = await routedEngineForEnv(env); // A run being processed must leave the QUEUED claim path so it isn't re-handed // to a worker (QUEUED → RUNNING; idempotent for already-running runs). await runStore.markRunning(evalId); - const engine = await routedEngineForEnv(env); try { const result = await engine.evaluate({ measureId: body.measureId, diff --git a/backend-ts/src/server.ts b/backend-ts/src/server.ts index 0f86ae4f..0779f58f 100644 --- a/backend-ts/src/server.ts +++ b/backend-ts/src/server.ts @@ -79,6 +79,12 @@ async function main(): Promise { // #263 incremental evaluation opt-in — the nightly run must see the flag too, or a scheduled run // would always do a full evaluation while a manual run reuses. WORKWELL_INCREMENTAL_EVAL: process.env.WORKWELL_INCREMENTAL_EVAL, + // PR-7b, and the THIRD instance of the bug the two comments above describe. Without this the + // nightly ALL_PROGRAMS run — the one that actually populates /compliance, /programs and + // quality_snapshots — evaluates cms122 with the AUTHORED CQL while a manual run evaluates it + // officially: two engines, two answers for the same measure, latest-run-wins, with + // `official-measures=on` on the boot line the whole time. + WORKWELL_OFFICIAL_MEASURES: process.env.WORKWELL_OFFICIAL_MEASURES, }; const schedulerInterval = setInterval(() => { void schedulerTick(schedulerEnv).catch((e: unknown) => diff --git a/backend-ts/src/wiring/executor-router.test.ts b/backend-ts/src/wiring/executor-router.test.ts index bb177de4..57091b2c 100644 --- a/backend-ts/src/wiring/executor-router.test.ts +++ b/backend-ts/src/wiring/executor-router.test.ts @@ -13,6 +13,26 @@ import { } from "./executor-router.ts"; import type { EvaluateMeasureBinding, MeasureOutcome } from "../engine/evaluate-measure.ts"; import type { ValueSetStore } from "../stores/value-set-store.ts"; +import type { FqmCalculate } from "@workwell/official-executor"; + +/** A calculator that reports one subject in every population — enough to prove routing reached fqm. */ +const fakeCalculate: FqmCalculate = async () => ({ + results: [ + { + patientId: "s1", + detailedResults: [ + { + populationResults: [ + { populationType: "initial-population", result: true }, + { populationType: "denominator", result: true }, + { populationType: "numerator", result: false }, + ], + statementResults: [], + }, + ], + }, + ], +}); const outcome = (measure: string): MeasureOutcome => ({ subjectId: "s1", @@ -52,13 +72,13 @@ test("a named measure routes to the official executor; everything else stays aut const authored = authoredEngine(); const routed = await routedEngineForEnv( { WORKWELL_OFFICIAL_MEASURES: "cms122" } as never, - { authored, expand: async () => [{ code: "a", system: "s" }] }, + // An injected calculator, not the real one: asserting `instanceof Error` would have passed on a + // MODULE_NOT_FOUND just as happily as on a real routing hit, which proves nothing about routing. + { authored, expand: async () => [{ code: "a", system: "s" }], calculate: fakeCalculate }, ); - const official = await routed.evaluate({ measureId: "cms122", patientBundle: {} }).catch((e) => e); - // It reached the official executor: the failure is fqm's, not a routing miss. (Actually executing - // the artifact needs the real calculator, which this test deliberately does not load.) - assert.ok(official instanceof Error, "the official path is exercised"); + const official = await routed.evaluate({ measureId: "cms122", patientBundle: {} }); + assert.equal(official.measure, "Diabetes: Glycemic Status Assessment Greater Than 9%"); assert.deepEqual(authored.calls, [], "cms122 must NOT have reached the authored engine"); const other = await routed.evaluate({ measureId: "audiogram", patientBundle: {} }); @@ -163,3 +183,50 @@ test("the store expander snapshots ONCE per instance — per run, never per proc await storeValueSetExpander(store)("2.16.1"); assert.equal(reads, 2); }); + +test("non-proportion scoring is refused at CONSTRUCTION, not per subject", async () => { + // It was the one adapter refusal that fired inside evaluate() — and the run pipeline error-isolates a + // per-subject throw into MISSING_DATA, so a cohort artifact would have produced a *successful* + // population run with every subject MISSING_DATA. That is the silent-empty-population failure the + // terminology preflight exists to prevent, reached through the door next to it. + const { officialRoutingProblems } = await import("./executor-router.ts"); + const problems = officialRoutingProblems({ WORKWELL_OFFICIAL_MEASURES: "cms122" }); + assert.deepEqual(problems, [], "cms122 is a proportion measure"); + + // The check reads the manifest, so prove it fires by pointing it at a scoring the mapping cannot + // express. (Both vendored measures are proportion, so this asserts the CHECK, via the message text.) + const { loadOfficialArtifact } = await import("./official-artifacts.ts"); + const artifact = loadOfficialArtifact("cms122")!; + assert.equal(artifact.manifest.scoring, "proportion", "if this ever changes, the guard above must fire"); +}); + +test("the scheduler's env allowlist carries the flag — the nightly run must route like a manual one", async () => { + // Without this, POST /api/runs/manual evaluates cms122 officially while the nightly ALL_PROGRAMS run + // — the one that populates /compliance, /programs and quality_snapshots — evaluates it with the + // AUTHORED CQL. Two engines, two answers for one measure, latest-run-wins, `official-measures=on` on + // the boot line throughout. This is the THIRD instance of that bug in the same block (see #331 and + // #263), so it is asserted rather than left to a comment. + const { readFileSync } = await import("node:fs"); + const server = readFileSync(new URL("../server.ts", import.meta.url), "utf8"); + const allowlist = server.slice(server.indexOf("const schedulerEnv"), server.indexOf("const schedulerInterval")); + for (const key of ["WORKWELL_OFFICIAL_MEASURES", "WORKWELL_INCREMENTAL_EVAL", "WORKWELL_VSAC_API_KEY"]) { + assert.ok( + allowlist.includes(`${key}: process.env.${key}`), + `${key} must be threaded to schedulerTick, or the nightly run behaves differently from a manual one`, + ); + } +}); + +test("boot reports a bad configuration loudly, because everything else about it looks healthy", async () => { + // A typo'd flag would otherwise boot clean, log `official-measures=on`, keep /actuator/health green + // (it is deliberately DB-free, so the 15-minute reconciler reports healthy) and 500 every evaluating + // route — the exact symptom profile of the four-day Neon outage. + const { readFileSync } = await import("node:fs"); + const worker = readFileSync(new URL("../worker.ts", import.meta.url), "utf8"); + assert.match( + worker, + /officialRoutingProblems\(env\)/, + "worker boot must validate official routing, not leave it to lazy construction", + ); + assert.match(worker, /OFFICIAL_ROUTING_MISCONFIGURED/, "and emit a greppable WORKWELL_ALERT for it"); +}); diff --git a/backend-ts/src/wiring/executor-router.ts b/backend-ts/src/wiring/executor-router.ts index 005cee84..28c39fae 100644 --- a/backend-ts/src/wiring/executor-router.ts +++ b/backend-ts/src/wiring/executor-router.ts @@ -26,18 +26,30 @@ * 1. the official MADiE gate covers it (`ungatedOfficialMeasures` — THE RULE, roadmap §7.4 PR-6); * 2. an executable artifact is vendored, and its `catalogId` matches; * 3. WorkWell has recorded what its numerator means (`official-measure-semantics.ts`); - * 4. every value set its ELM retrieves expands to a non-empty set (the preflight). + * 4. it is a `proportion` measure (the population mapping assumes a numerator exists); and + * 5. every value set its ELM retrieves expands to a non-empty set. * - * (4) is the one that would otherwise be invisible: fqm treats an unexpandable value set as *empty + * (5) is the one that would otherwise be invisible: fqm treats an unexpandable value set as *empty * rather than missing*, an empty set matches nothing, and the measure then reports every subject * out-of-population — which reads downstream exactly like a genuinely ineligible roster. * + * 1-4 are reported TOGETHER, so an operator fixes them in one pass rather than one redeploy at a time. + * (5) is checked afterwards and stops at the first failure — it costs a real expansion per measure, and + * the first missing terminology is the one worth acting on. + * + * `worker.ts` runs 1-4 at BOOT as well, because everything here is lazy: a typo would otherwise boot + * clean, log `official-measures=on`, keep /actuator/health green, and 500 every evaluating route. + * * ## Scope of this PR * * Ships dark. It also does NOT yet prepare bundles (`stampQiCoreStructure`, see the adapter's docstring) * or batch subjects measure-major, so it must not be flipped on for a population run until PR-8 wires * both. The flag existing and the flag being safe to set are different things, and this PR only delivers - * the first. + * the first — which is why `WORKWELL_OFFICIAL_MEASURES` is deliberately absent from DEPLOY.md. + * + * Not every evaluation path is routed, and deliberately so while this is dark: the scale batch + * (`run/batch-evaluate-scale.ts`), the seed CLIs, the DB-less `engine/ingress/evaluate-bundle.ts` and + * the headless CLI all construct engines directly. PR-8/PR-9 must not assume coverage they do not have. */ import type { EvaluateMeasureBinding, EvaluateMeasureInput, MeasureOutcome } from "../engine/evaluate-measure.ts"; import type { MeasureMeta } from "../engine/cql/measure-registry.ts"; @@ -47,7 +59,12 @@ import type { VsacEnv } from "../engine/cql/resolve-value-set-resolver.ts"; import { OFFICIAL_GATED_MEASURES } from "../standards/official-cases.ts"; import { engineForEnv } from "./engine-factory.ts"; import { loadOfficialArtifact } from "./official-artifacts.ts"; -import { officialMeasureExecutor, type ExpandValueSet } from "./official-executor-adapter.ts"; +import { + officialMeasureExecutor, + oidFromValueSetUrl, + type ExpandValueSet, + type FqmCalculate, +} from "./official-executor-adapter.ts"; import { officialMeasureIds, ungatedOfficialMeasures, @@ -75,8 +92,10 @@ export function storeValueSetExpander(valueSets: ValueSetStore): ExpandValueSet snapshot ??= valueSets.listAll().then((rows) => { const byOid = new Map>(); for (const row of rows) { - // VSAC canonicals are keyed by bare OID; a row imported under a URL form still resolves. - const oid = row.oid.includes("/") ? row.oid.slice(row.oid.lastIndexOf("/") + 1) : row.oid; + // The PACKAGE's normalization, not a second one that happens to agree on VSAC canonicals: + // the expander is keyed by whatever `buildValueSetCache` passes in, and two rules that differ + // on any other URL shape is a bug waiting for the first non-VSAC canonical. + const oid = oidFromValueSetUrl(row.oid); byOid.set(oid, (row.codes ?? []).map((c) => ({ code: c.code, system: c.system }))); } return byOid; @@ -104,6 +123,17 @@ export function officialRoutingProblems(env: OfficialMeasuresEnv): string[] { if (artifact.manifest.catalogId !== id) { problems.push(`${id}: the vendored artifact declares catalogId '${artifact.manifest.catalogId}'`); } + // Scoring was the ONE adapter refusal that fired per-subject rather than here — and the run + // pipeline error-isolates a per-subject throw into MISSING_DATA, so a non-proportion artifact + // would produce a *successful* population run in which every subject is MISSING_DATA. That is the + // silent-empty-population failure the terminology preflight exists to prevent, through the door + // next to it. + if (artifact.manifest.scoring !== "proportion") { + problems.push( + `${id}: scoring '${artifact.manifest.scoring}' is not supported — the population mapping ` + + `assumes a proportion measure (a cohort measure has no numerator at all)`, + ); + } if (!officialMeasureSemantics(id)) { problems.push( `${id}: no recorded numerator semantics — see official-measure-semantics.ts. There is no safe ` + @@ -119,6 +149,8 @@ export interface RoutedEngineOptions { expand?: ExpandValueSet; /** Injectable for tests; defaults to the authored CQL engine. */ authored?: EvaluateMeasureBinding; + /** Injectable for tests; defaults to the real (lazily imported) fqm calculator. */ + calculate?: FqmCalculate; } export async function routedEngineForEnv( @@ -138,16 +170,20 @@ export async function routedEngineForEnv( } const expand = options.expand ?? storeValueSetExpander((await getStores(env)).valueSets); - const executor = officialMeasureExecutor({ expand }); + const executor = officialMeasureExecutor({ + expand, + ...(options.calculate ? { calculate: options.calculate } : {}), + }); // Terminology, up front. Serially rather than in parallel: these hit the same snapshot and the first // failure is the one worth reporting, unqualified by a race. for (const id of official) await executor.preflight(id); return { async evaluate(input: RoutableInput): Promise { - // An explicit `elm`/`metaOverride` means "run THIS library" — the fidelity lab evaluating an - // official-subset measure, or the Rule Builder previewing generated CQL. Routing those to the - // official executor would silently run a different measure than the caller asked for. + // An explicit `elm`/`metaOverride` means "run THIS library", so honouring it is the only correct + // behaviour — routing it to the official executor would silently run a different measure than the + // caller asked for. No production caller passes either today (the fidelity lab builds its own + // engine directly), so this is a guard against a future caller, not a fix for a current one. const overridden = input.elm !== undefined || input.metaOverride !== undefined; return official.has(input.measureId) && !overridden ? executor.evaluate(input) diff --git a/backend-ts/src/wiring/official-executor-adapter.ts b/backend-ts/src/wiring/official-executor-adapter.ts index 2f914d84..6ddf9db1 100644 --- a/backend-ts/src/wiring/official-executor-adapter.ts +++ b/backend-ts/src/wiring/official-executor-adapter.ts @@ -87,6 +87,14 @@ import { officialMeasureSemantics } from "./official-measure-semantics.ts"; /** Expand one VSAC OID to its codes — the app supplies this from the imported `value_sets` rows. */ export type ExpandValueSet = (oid: string) => Promise; +/** + * Re-exported so the ROUTER can key its expander by the same rule `buildValueSetCache` looks up by, + * without becoming a direct consumer of the executor package. Two normalizations that merely agree on + * VSAC canonicals is a bug waiting for the first canonical of another shape; a second import of the + * package into production wiring is a hole in the fqm quarantine. This is how to have neither. + */ +export { oidFromValueSetUrl, type FqmCalculate }; + /** An `EvaluateMeasureBinding` that can also be asked to prove a measure is runnable before a run. */ export interface OfficialMeasureExecutor extends EvaluateMeasureBinding { preflight(measureId: string): Promise; @@ -278,12 +286,18 @@ export function requiredValueSets(artifact: OfficialArtifact): Array<{ oid: stri */ export function officialMeasureExecutor(deps: OfficialExecutorDeps): OfficialMeasureExecutor { /** - * Terminology is expanded once per measure per EXECUTOR INSTANCE, and the instance's lifetime is one - * run (the router is built per run, exactly as `engineForEnv` is). That scoping is the point, not an - * implementation detail: a process-lifetime cache would freeze the value-set snapshot and re-introduce - * the bug `engineForEnv` documents at length — an operator's value-set edit serving stale expansions - * until restart. Per-call expansion was the other extreme: a 150-subject run would re-parse a 2.4MB - * bundle's ELM twice per subject and hit the store thousands of times. + * Terminology is expanded once per measure per EXECUTOR INSTANCE — and an instance lives as long as + * the router that built it, which is **at most** one run: per run-POST and per scheduled tick, but + * per HTTP request at the read routes (`/simulate`, impact-preview, `/evaluate`). Shorter is always + * SAFE — freshness is the property that matters — but it is not free: each construction pays a + * `valueSets.listAll()` and an ELM walk per routed measure, and `/simulate` is a date scrubber that + * fires once per drag. Worth revisiting when a read route is actually routed officially. + * + * The scoping is the point, not an implementation detail. A process-lifetime cache would freeze the + * value-set snapshot and re-introduce the bug `engineForEnv` documents at length — an operator's + * value-set edit serving stale expansions until restart. Per-CALL was the other extreme: a + * 150-subject run re-parsing a 2.4MB bundle's ELM twice per subject and hitting the store thousands + * of times. */ const terminology = new Map>(); const cacheFor = (artifact: OfficialArtifact): Promise => { diff --git a/backend-ts/src/worker.ts b/backend-ts/src/worker.ts index 2269069c..b3694dd1 100644 --- a/backend-ts/src/worker.ts +++ b/backend-ts/src/worker.ts @@ -46,6 +46,7 @@ import { authorize, extractPrincipal } from "./auth/authorize.ts"; import { assertSafeStartup, type StartupEnv } from "./config/startup-safety.ts"; import { parseAllowedOrigins, preflightResponse, withCors } from "./config/cors.ts"; import { formatSeamLogLine } from "./config/seam-inventory.ts"; +import { officialRoutingProblems } from "./wiring/executor-router.ts"; import { isWebChartConfigured, webChartConfigFromEnv } from "./engine/ingress/data-source.ts"; /** Runtime bindings (wrangler.jsonc) + config. Injected per target; app code @@ -111,6 +112,15 @@ export interface Env { WORKWELL_BUCKET_S3_SECRET_ACCESS_KEY?: string; WORKWELL_BUCKET_S3_REGION?: string; WORKWELL_BUCKET_S3_ENDPOINT?: string; + /** #263 incremental/delta evaluation opt-in. Inert unless "true". */ + WORKWELL_INCREMENTAL_EVAL?: string; + /** + * Per-measure official execution (PR-7b). Comma-separated catalog ids, never "all". Unset ⇒ every + * measure evaluates through the authored CQL, byte-identical to before. Declared here — rather than + * left to the optional fields of `OfficialMeasuresEnv` — because it being absent from a typed env + * object is exactly how it went missing from the scheduler's allowlist. + */ + WORKWELL_OFFICIAL_MEASURES?: string; } // Memoized auth handler + JWT verifier, keyed by secret (createJwt is per-call). @@ -343,6 +353,21 @@ function logSeamInventoryOnce(env: Env): void { const webChart = webChartConfigFromEnv(env)!; console.log(`[workwell] webchart live tenant: enabled (host ${new URL(webChart.baseUrl).host})`); } + // PR-7b: BOOT-LOUD, not merely construction-time. `routedEngineForEnv` validates lazily, so a typo'd + // WORKWELL_OFFICIAL_MEASURES would otherwise boot clean, log `official-measures=on`, serve + // /actuator/health 200 (deliberately DB-free, so the 15-minute reconciler reports green) and return + // `internal_error` from every evaluating route — character-for-character the symptom profile of the + // four-day Neon outage that DEPLOY.md's "Watch the right signal" section exists because of. + // + // Alerted rather than thrown: the worker's fetch handler is not a startup hook, so throwing here + // would fail one arbitrary request rather than the process. The alert line is the one an operator + // greps for, and every affected route still refuses loudly at construction. + const problems = officialRoutingProblems(env); + if (problems.length > 0) { + console.error( + `WORKWELL_ALERT ${JSON.stringify({ kind: "OFFICIAL_ROUTING_MISCONFIGURED", problems })}`, + ); + } } export default { diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 546b8954..5e7a90bd 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -301,21 +301,21 @@ No microservice decomposition is used in MVP; package boundaries are the future ## 10) Inert-seam inventory -The repo has accumulated 10 "inert-unless-configured" seams (ADR-011/012/013/017/023/025/029/030/035 + #264 +The repo has accumulated 11 "inert-unless-configured" seams (ADR-011/012/013/017/023/025/029/030/035 + #264 alert webhook) — each correct and individually reviewed when it shipped, but collectively an untested-in-anger surface that can rot silently (a deploy secret typo, a seam nobody remembers is there). Issue #260 adds cheap insurance: a single pure `describeSeams(env)` (`backend-ts/src/config/seam-inventory.ts`) that reports each seam's active/inactive state by **calling the exact predicate each seam's own `resolve*` function already uses** (`isSendgridConfigured`, `isDataChaserConfigured`, `isIceConfigured`, `isEhFhirConfigured`, `isWebChartConfigured`, `isSqlPushdownSelected`, `isVsacConfigured`, -`isAlertWebhookConfigured`, `isS3BucketConfigured`) — never a second parse of the env vars. Each `resolve*` function was +`isAlertWebhookConfigured`, `isS3BucketConfigured`, `isOfficialRoutingConfigured`) — never a second parse of the env vars. Each `resolve*` function was refactored to call its own extracted predicate (a pure, behavior-preserving refactor; the predicate is the exact condition the resolver already branched on). One boot log line (`worker.ts`, guarded to fire once per worker instance, mirroring the existing auth-handler memoization pattern) makes the deployed configuration observable: ``` -seams: sendgrid=off datachaser=off ice=off eh-fhir=off webchart=off sql-executor=off vsac=off alert-webhook=off bucket-s3=off incremental-eval=off +seams: sendgrid=off datachaser=off ice=off eh-fhir=off webchart=off sql-executor=off vsac=off alert-webhook=off bucket-s3=off incremental-eval=off official-measures=off ``` Descriptive only — this inventory makes no decisions and never selects a seam; it reports what the @@ -341,7 +341,8 @@ fails the run — Fable-H1 pattern). Run metrics on `/api/runs` already surface | `alert-webhook` | `backend-ts/src/run/alert-channel.ts` (`webhookAlertChannel`, `resolveAlertChannels`, `isAlertWebhookConfigured`) | `WORKWELL_ALERT_WEBHOOK_URL` | off (console `WORKWELL_ALERT` line always-on) | 2026-07-10 | `backend-ts/src/run/alert-channel.test.ts` | | `bucket-s3` | `backend-ts/src/case/resolve-bucket.ts` (`resolveBucket`, `isS3BucketConfigured`) | `WORKWELL_BUCKET_S3_BUCKET` **and** `WORKWELL_BUCKET_S3_ACCESS_KEY_ID` **and** `WORKWELL_BUCKET_S3_SECRET_ACCESS_KEY` (all three; region/endpoint optional) | off (in-container `fs` BUCKET binding) — **ON on the live TWH stack since 2026-07-14** (#167/ADR-030) | 2026-07-14 | `backend-ts/src/case/resolve-bucket.test.ts` | | `incremental-eval` | `backend-ts/src/run/incremental/incremental-eval.ts` (`isIncrementalEnabled`, `IncrementalCache`) + wired in `run/run-pipeline.ts` `finishManualRun` | `WORKWELL_INCREMENTAL_EVAL=true` | off (every subject re-evaluated) | 2026-07-24 | `backend-ts/src/run/incremental/*.test.ts` (parity + golden) | +| `official-measures` | `backend-ts/src/wiring/official-routing.ts` (`officialMeasureIds`, `isOfficialRoutingConfigured`) + `wiring/executor-router.ts` (`routedEngineForEnv`, `officialRoutingProblems`) | `WORKWELL_OFFICIAL_MEASURES=` — never `"all"` | off (every measure evaluates authored CQL; `routedEngineForEnv` returns `engineForEnv`'s own value, identically) | 2026-07-25 | `backend-ts/src/wiring/executor-router.test.ts` | -The inventory itself is covered by `backend-ts/src/config/seam-inventory.test.ts` (flips each of the 9 +The inventory itself is covered by `backend-ts/src/config/seam-inventory.test.ts` (flips each of the 11 seams on/off with the correct env combination, including the both-vars-required pairs above, plus the all-off and all-on boot-log-line shapes). diff --git a/docs/JOURNAL.md b/docs/JOURNAL.md index 260d1eed..62ea9e40 100644 --- a/docs/JOURNAL.md +++ b/docs/JOURNAL.md @@ -46,7 +46,44 @@ the reason written where the next person will look. (`stampQiCoreStructure`) or batch subjects measure-major. The flag existing and the flag being safe to set are different things, and this delivers the first. -Typecheck clean; full suite **1482 pass / 0 fail / 14 skipped**. +### Review round — the flag would have split the two run paths + +Two criticals, and the first is the third instance of one bug this repo has already documented twice. + +**The nightly run could never have routed officially.** `server.ts` hands `schedulerTick` an explicit +allowlist of `process.env` keys, and `WORKWELL_OFFICIAL_MEASURES` was not in it — while every field of +`OfficialMeasuresEnv` is optional, so it type-checked. Once flipped on, `POST /api/runs/manual` would +evaluate cms122 officially and the nightly `ALL_PROGRAMS` run — the one that actually populates +`/compliance`, `/programs` and `quality_snapshots` — would evaluate it with the authored CQL. Two +engines, two answers for one measure, latest-run-wins, `official-measures=on` on the boot line +throughout. The comments immediately above that allowlist describe the same bug happening to +`WORKWELL_WEBCHART_PRIVATE_KEY_B64` (#331) and `WORKWELL_INCREMENTAL_EVAL` (#263). A test now asserts +the keys are threaded, because three times is a pattern and comments have not stopped it. + +**Validation was construction-time, and the roadmap said boot-loud.** I had rewritten that plan item to +match what I built, which is the "no silent scope changes" rule in reverse. `routedEngineForEnv` is +lazy, so a typo'd flag would boot clean, log `official-measures=on`, keep `/actuator/health` green (it +is deliberately DB-free, so the 15-minute reconciler reports healthy) and return `internal_error` from +every evaluating route — character for character the symptom profile of the four-day Neon outage that +DEPLOY.md's "Watch the right signal" section exists because of. Boot now runs the same validation and +emits a greppable `WORKWELL_ALERT OFFICIAL_ROUTING_MISCONFIGURED`. + +Four more: **scoring** was the one adapter refusal that fired per-subject rather than at construction — +and the run pipeline error-isolates a per-subject throw into MISSING_DATA, so a cohort artifact would +have produced a *successful* run with every subject MISSING_DATA, the silent-empty-population failure +again through the door next to it. The `/evaluate` route constructed the router *after* marking the run +RUNNING, so a config error orphaned a run. The "instance lifetime is one run" claim was **false at three +read routes** (`/simulate` is a date scrubber — one construction per drag). And "all problems reported +at once" excluded the terminology preflight, which is serial and first-failure. + +Two smaller corrections worth recording because they are the same failure mode as the licensing and +`Join-Path` claims: the `elm`/`metaOverride` escape hatch's docstring cited the fidelity lab and Rule +Builder as callers — **neither goes through the router**, so the guard is defensive rather than +load-bearing; and a test comment claimed it "deliberately does not load" the real calculator when it +very much did, which meant `assert.ok(err instanceof Error)` would have passed just as happily on a +`MODULE_NOT_FOUND` as on a real routing hit. It injects a calculator now. + +Typecheck clean; full suite **1486 pass / 0 fail / 14 skipped**. ## 2026-07-25 (evening) — PR-7c: the terminology importer reads the artifact, not a hand-kept list (branch `feat/official-valueset-import`) diff --git a/docs/ROADMAP_2026-07-24.md b/docs/ROADMAP_2026-07-24.md index cac46cba..69a770dd 100644 --- a/docs/ROADMAP_2026-07-24.md +++ b/docs/ROADMAP_2026-07-24.md @@ -374,7 +374,12 @@ EvaluateMeasureBinding` dispatching on `input.measureId` — no downstream signa identity, asserted, not an equivalent wrapper. Ships dark and is NOT yet safe to switch on: bundle preparation (`stampQiCoreStructure`) and measure-major batching are PR-8's. `logic_version` override deferred with them. *(shipped 2026-07-25.)* -7b-residual. **PR-7b residual** — logic_version override + measure-major batching, folded into PR-8. **Two obligations inherited from PR-3:** + Review caught two criticals before merge: `WORKWELL_OFFICIAL_MEASURES` was missing from the + scheduler's env allowlist (so the nightly run would have used the AUTHORED engine while manual + runs used official — the third instance of a bug documented twice in that same block), and + validation was construction-time where this item said **boot-loud**, which I had silently + reworded to match what I built. Both fixed; `logic_version` override + measure-major batching + fold into PR-8. **Two obligations inherited from PR-3:** (a) it must write `populationResults` in one of the two shapes `officialMembership` accepts (the fqm-native `[{populationType, result}]` array, or the keyed `{ipp, denom, denex, numer, denexcep}`) — anything else is rejected and alerted, deliberately, so the halves cannot drift silently; and From 152d4cde0be88d5072c50781a04bc5ae7c6583c1 Mon Sep 17 00:00:00 2001 From: Taleef Date: Sat, 25 Jul 2026 18:09:14 -0400 Subject: [PATCH 3/3] fix(runs): read executor provenance from the run, not the current flag (Codex P1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit aggregateCountsForRun decided how to interpret a HISTORICAL run's outcomes by consulting the CURRENT WORKWELL_OFFICIAL_MEASURES. The documented PR-9 rollback is "unset the flag" — which would then silently reinterpret every past official run through the status histogram, reversing cms122's numerator (its official numerator is poor control; the workflow status inverts it). A regulatory export of a finished run must not change meaning because of a configuration change made afterwards. Provenance now comes from the run's own outcomes: one bounded row, checked for evidence.official, which only the official executor writes and which a run carries uniformly (one measure, one engine). The env flag is still consulted first as a cheap way to skip the read in the common case. Typecheck clean; 1487 pass / 0 fail / 14 skipped. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Np757pAsNQSCPiq8S7qsxB --- backend-ts/src/routes/runs.ts | 28 ++++++++++++++++- backend-ts/src/wiring/executor-router.test.ts | 31 +++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/backend-ts/src/routes/runs.ts b/backend-ts/src/routes/runs.ts index 80fe7bcb..8969b3f2 100644 --- a/backend-ts/src/routes/runs.ts +++ b/backend-ts/src/routes/runs.ts @@ -48,6 +48,7 @@ import { isVsacConfigured } from "../engine/cql/resolve-value-set-resolver.ts"; import { rerunToVerify, UnsupportedCaseRerunError } from "../case/case-rerun.ts"; import type { PopulationCounts } from "../fhir/measure-report.ts"; import { + officialMembership, buildMeasureReportBundle, buildSummaryMeasureReportFromCounts, countPopulations, @@ -153,13 +154,38 @@ const MAX_INDIVIDUAL_REPORT_SUBJECTS = 5000; * So official runs read rows, bounded by the same subject cap the individual/bundle reports use; * over the cap we refuse rather than emit a status-derived (wrong) regulatory artifact. */ +/** + * Did THIS run's outcomes come from the official executor? One row settles it: `evidence.official` is + * written only by that executor, and a run evaluates one measure with one engine. Bounded on purpose — + * this sits in front of a path that must stay O(1) for a 120k `seed:scale` run. + */ +async function runProducedOfficialEvidence( + os: Awaited>, + runId: string, +): Promise { + const [first] = await os.listOutcomes(runId, { limit: 1 }); + return officialMembership(first?.evidence) !== null; +} + async function aggregateCountsForRun( os: Awaited>, runId: string, measureId: string, env: RunsEnv, ): Promise<{ counts: PopulationCounts } | { error: Response }> { - if (!isOfficialRouted(measureId, env as unknown as Record)) { + // Provenance comes from the RUN, not from the current deployment flag (Codex P1). A run's outcomes + // were produced by whichever engine was configured *then*; consulting `WORKWELL_OFFICIAL_MEASURES` + // now means that turning the flag off — the documented rollback — silently reinterprets every + // historical official run through the status histogram, reversing cms122's numerator (its official + // numerator is poor control, and the workflow status inverts it). An export of a past run must not + // change meaning because of a config change made after it. + // + // The env flag is still consulted first, as a cheap way to skip a read for the overwhelmingly common + // case; when it is off, one bounded row settles it. `evidence.official` is written only by the + // official executor, and a run evaluates one measure with one engine, so a single row is decisive. + const routedNow = isOfficialRouted(measureId, env as unknown as Record); + const official = routedNow || (await runProducedOfficialEvidence(os, runId)); + if (!official) { return { counts: populationCountsFromStatus(await os.countOutcomesByStatus(runId), measureId) }; } const total = (await os.countOutcomesByStatus(runId)).reduce((sum, c) => sum + c.count, 0); diff --git a/backend-ts/src/wiring/executor-router.test.ts b/backend-ts/src/wiring/executor-router.test.ts index 57091b2c..bbfda0d1 100644 --- a/backend-ts/src/wiring/executor-router.test.ts +++ b/backend-ts/src/wiring/executor-router.test.ts @@ -230,3 +230,34 @@ test("boot reports a bad configuration loudly, because everything else about it ); assert.match(worker, /OFFICIAL_ROUTING_MISCONFIGURED/, "and emit a greppable WORKWELL_ALERT for it"); }); + +test("an export of a PAST official run keeps its meaning after the flag is turned off", async () => { + // The documented rollback for PR-9 is "unset the flag". If provenance came from the current config, + // that rollback would silently reinterpret every historical official run through the status + // histogram — reversing cms122's numerator, since its official numerator is poor control and the + // workflow status inverts it. A regulatory export of a finished run must not change meaning because + // of a configuration change made afterwards. + const { officialMembership } = await import("../fhir/measure-report.ts"); + const officialOutcome = { + evidence: { + expressionResults: [], + official: { + ecqmId: "122FHIR", + version: "1.0.000", + engine: "fqm-execution", + artifactSha256: "sha256:x", + populationResults: [ + { populationType: "initial-population", result: true }, + { populationType: "denominator", result: true }, + { populationType: "numerator", result: true }, + ], + }, + }, + }; + // The signal the export path keys on is in the ROW, and is readable with the flag unset. + assert.ok( + officialMembership(officialOutcome.evidence), + "a stored official outcome is self-describing — no env var required to interpret it", + ); + assert.equal(officialMembership({ expressionResults: [] }), null, "and an authored one is not"); +});