diff --git a/backend-ts/packages/official-executor/src/index.ts b/backend-ts/packages/official-executor/src/index.ts index 403c64e1..3634bc3b 100644 --- a/backend-ts/packages/official-executor/src/index.ts +++ b/backend-ts/packages/official-executor/src/index.ts @@ -111,7 +111,16 @@ export function isExecutableMeasureBundle(bundle: unknown): bundle is MeasureBun * Read from the compiled ELM rather than the CQL text, so it reflects what will actually execute. */ export function referencedValueSetUrls(bundle: MeasureBundle): string[] { - const urls = new Set(); + return referencedValueSets(bundle).map((v) => v.url); +} + +/** + * As `referencedValueSetUrls`, but keeps the CQL alias the ELM declares alongside each canonical + * ("Hospice Encounter", "Diabetes"). A terminology importer wants that: without it every row it writes + * is named by its bare OID, and a human reading the value-set catalog has nothing to go on. + */ +export function referencedValueSets(bundle: MeasureBundle): Array<{ url: string; name?: string }> { + const byUrl = new Map(); for (const entry of bundle.entry) { const resource = entry.resource as { resourceType?: string; @@ -125,11 +134,15 @@ export function referencedValueSetUrls(bundle: MeasureBundle): string[] { // on an unresolvable value set anyway - so swallowing this would only trade a precise parse error // for an opaque failure deep inside the batch. const elm = JSON.parse(Buffer.from(data, "base64").toString("utf8")) as { - library?: { valueSets?: { def?: Array<{ id: string }> } }; + library?: { valueSets?: { def?: Array<{ id: string; name?: string }> } }; }; - for (const def of elm.library?.valueSets?.def ?? []) urls.add(def.id); + for (const def of elm.library?.valueSets?.def ?? []) { + // First name wins: the same canonical is declared in several libraries, and the first is as good + // as any. Keeping a set keyed by url preserves the previous de-duplication exactly. + if (!byUrl.has(def.id)) byUrl.set(def.id, { url: def.id, ...(def.name ? { name: def.name } : {}) }); + } } - return [...urls]; + return [...byUrl.values()]; } /** `http://cts.nlm.nih.gov/fhir/ValueSet/2.16.840...` → `2.16.840...` (expanders are keyed by bare OID). */ diff --git a/backend-ts/src/run/cli/resolve-valuesets.test.ts b/backend-ts/src/run/cli/resolve-valuesets.test.ts index 7c2feae1..5c8b9464 100644 --- a/backend-ts/src/run/cli/resolve-valuesets.test.ts +++ b/backend-ts/src/run/cli/resolve-valuesets.test.ts @@ -259,3 +259,64 @@ test("a first-ever import is not drift (no prior row to compare)", async () => { assert.deepEqual(res.changed, []); assert.equal(audits.filter((a) => a.eventType === "VALUE_SET_EXPANSION_CHANGED").length, 0); }); + +test("--official derives the import target from the artifact the executor actually refuses on", async () => { + const { parseArgs, officialArtifactOids } = await import("./resolve-valuesets.ts"); + const { loadOfficialArtifact } = await import("../../wiring/official-artifacts.ts"); + const { requiredOids } = await import("../../wiring/official-executor-adapter.ts"); + + // The point of the flag: the SAME ELM that makes the executor refuse a measure decides what gets + // fetched. A hand-kept list is how CMS122 came to be "imported" while 5 of its 26 canonicals were + // missing — so assert the two agree by construction, not by coincidence. + for (const catalogId of ["cms122", "cms125"]) { + const artifact = loadOfficialArtifact(catalogId)!; + const parsed = parseArgs(["--official", catalogId]); + assert.deepEqual( + [...(parsed.oids ?? [])].sort(), + [...new Set(requiredOids(artifact))].sort(), + `${catalogId}: the import target must be exactly what the executor requires`, + ); + // Names come from the ELM's CQL aliases, so the value-set catalog is readable rather than a + // column of bare OIDs. + assert.ok( + Object.values(parsed.names ?? {}).some((n) => /[a-z]/i.test(n)), + `${catalogId}: artifact-derived rows must carry their CQL alias`, + ); + } + + // The two measures overlap heavily (shared hospice / palliative / frailty / encounter sets), so + // expanding a shared canonical twice would double-write the row and double-count the summary. + const both = parseArgs(["--official", "cms122", "--official", "cms125"]); + assert.equal(both.oids!.length, new Set(both.oids).size, "targets must be de-duplicated"); + assert.equal(both.oids!.length, 35, "26 + 32 with 23 shared — one import covers both measures"); + + assert.throws(() => officialArtifactOids("cms999"), /no vendored artifact/); + assert.throws(() => parseArgs(["--official"]), /needs a catalog id/); +}); + +test("a failed expansion keeps the artifact's alias — an ERROR row must not be renamed to a bare OID", async () => { + const { runResolve } = await import("./resolve-valuesets.ts"); + const f = fakes(); + const client = { + expand: async (oid: string) => { + if (oid === "2.16.999") throw new Error("VSAC timeout"); + return { contains: [{ code: "a", system: "s", display: "A" }], version: "1" } as VsacExpansion; + }, + }; + const res = await runResolve({ + oids: ["2.16.111", "2.16.999"], + names: { "2.16.111": "Hospice Encounter", "2.16.999": "Palliative Care Intervention" }, + client: client as never, + valueSets: f.valueSets, + events: f.events, + now: "2026-07-25T00:00:00Z", + }); + + assert.equal(res.resolved, 1); + assert.equal(res.errors, 1); + // Partial failure is a supported outcome of a bulk import, so the error branch is not a rare corner: + // writing the bare OID would overwrite a good alias on any transient VSAC blip, leaving an operator + // looking at a row named 2.16.999 with no way to know it ever had a name. + const errorRow = f.upserts.find((u) => u.resolutionStatus === "ERROR"); + assert.equal(errorRow?.name, "Palliative Care Intervention"); +}); diff --git a/backend-ts/src/run/cli/resolve-valuesets.ts b/backend-ts/src/run/cli/resolve-valuesets.ts index 8a1a1e7a..ba03887e 100644 --- a/backend-ts/src/run/cli/resolve-valuesets.ts +++ b/backend-ts/src/run/cli/resolve-valuesets.ts @@ -4,7 +4,11 @@ * A failed expand writes an ERROR row and continues. Owner-run ON DEMAND, NOT on deploy. Local (SQLite * floor) or Neon (export DATABASE_URL + WORKWELL_VSAC_API_KEY). Default target = the CMS122 reference set. * - * pnpm resolve-valuesets [--oid ...] [--measure cms122] + * pnpm resolve-valuesets [--oid ...] [--measure cms122] [--official ] + * + * `--official cms122 --official cms125` imports exactly what the vendored official artifacts retrieve + * (35 canonicals, de-duplicated) — the list the official executor refuses to run without. Prefer it to + * the hand-kept default, which covers 21 of the 26 CMS122 needs and 18 of the 32 CMS125 ones. * * ROLLBACK (reversible; schema-qualify on Postgres): DELETE FROM workwell_spike.value_sets WHERE source = 'VSAC'; * @@ -13,20 +17,49 @@ import { getStores, type StoresEnv } from "../../stores/factory.ts"; import { httpVsacClient, type VsacClient } from "../../engine/cql/vsac-client.ts"; import { CMS122V14 } from "../../standards/references/cms122v14.ts"; +import { loadOfficialArtifact } from "../../wiring/official-artifacts.ts"; +import { requiredValueSets } from "../../wiring/official-executor-adapter.ts"; import type { CaseEventStore } from "../../stores/case-event-store.ts"; import type { ValueSetStore } from "../../stores/value-set-store.ts"; export const USAGE = - "Usage: pnpm resolve-valuesets [--oid ...] [--measure cms122] [--manifest | --expansion ]"; + "Usage: pnpm resolve-valuesets [--oid ...] [--measure cms122] [--official ] " + + "[--manifest | --expansion ]"; export const DEFAULT_OIDS: string[] = [...new Set(CMS122V14.valueSets.map((v) => v.oid))]; const NAME_BY_OID: Record = Object.fromEntries(CMS122V14.valueSets.map((v) => [v.oid, v.name])); +/** + * `--official `: import exactly the value sets the VENDORED OFFICIAL ARTIFACT retrieves, + * read from its compiled ELM. + * + * The hand-kept `CMS122V14.valueSets` table (21 OIDs) is how this CLI has always chosen targets, and it + * was wrong in a way nobody could see: CMS122's own artifact references **26** canonicals and CMS125's + * references **32**, so the official executor refuses both measures on terminology today. Deriving the + * list from the artifact puts the importer and the executor's refusal in agreement by construction — + * the same ELM decides what must be present and what gets fetched. + * + * Deliberately imports everything referenced, including the four SupplementalDataElements sets that + * `calculateSDEs: false` never retrieves. They are cheap, and an importer that second-guesses which + * references "really" matter is how the two lists drift apart again. + */ +export function officialArtifactOids(catalogId: string): Array<{ oid: string; name?: string }> { + const artifact = loadOfficialArtifact(catalogId); + if (!artifact) { + throw new ResolveCliUsageError( + `--official ${catalogId}: no vendored artifact (see measures/official/)\n${USAGE}`, + ); + } + return requiredValueSets(artifact); +} + export class ResolveCliUsageError extends Error { override readonly name = "ResolveCliUsageError"; } export interface ResolveArgs { oids?: string[]; + /** Display names for artifact-derived OIDs, so imported rows are not named by bare OID. */ + names?: Record; /** Release pin (#295) — forwarded to $expand; mutually exclusive. */ manifest?: string; expansion?: string; @@ -34,6 +67,7 @@ export interface ResolveArgs { export function parseArgs(args: string[]): ResolveArgs { const oids: string[] = []; + const names: Record = {}; let manifest: string | undefined; let expansion: string | undefined; for (let i = 0; i < args.length; i++) { @@ -42,6 +76,13 @@ export function parseArgs(args: string[]): ResolveArgs { const v = args[++i]; if (!v) throw new ResolveCliUsageError(`--oid needs a value\n${USAGE}`); oids.push(v); + } else if (a === "--official") { + const catalogId = args[++i]; + if (!catalogId) throw new ResolveCliUsageError(`--official needs a catalog id\n${USAGE}`); + for (const vs of officialArtifactOids(catalogId)) { + oids.push(vs.oid); + if (vs.name) names[vs.oid] = vs.name; + } } else if (a === "--measure") { const m = args[++i]; if (m !== "cms122") throw new ResolveCliUsageError(`--measure only supports 'cms122' today\n${USAGE}`); @@ -63,7 +104,10 @@ export function parseArgs(args: string[]): ResolveArgs { if (manifest && expansion) throw new ResolveCliUsageError(`--manifest and --expansion are mutually exclusive\n${USAGE}`); return { - ...(oids.length ? { oids } : {}), + // De-duplicated: `--official cms122 --official cms125` shares 18 canonicals, and expanding one + // twice would write the row twice and count it twice in the summary. + ...(oids.length ? { oids: [...new Set(oids)] } : {}), + ...(Object.keys(names).length ? { names } : {}), ...(manifest ? { manifest } : {}), ...(expansion ? { expansion } : {}), }; @@ -71,6 +115,8 @@ export function parseArgs(args: string[]): ResolveArgs { export interface RunResolveDeps { oids: string[]; + /** Display names by OID (artifact-derived); falls back to the CMS122 table, then the bare OID. */ + names?: Record; client: VsacClient; valueSets: ValueSetStore; events: CaseEventStore; @@ -142,7 +188,7 @@ export async function runResolve(deps: RunResolveDeps): Promise { const drifted = before !== undefined && before !== hash; await deps.valueSets.upsertResolvedValueSet({ oid, - name: NAME_BY_OID[oid] ?? oid, + name: deps.names?.[oid] ?? NAME_BY_OID[oid] ?? oid, version: exp.version ?? null, source: "VSAC", codes, @@ -205,7 +251,12 @@ export async function runResolve(deps: RunResolveDeps): Promise { // resolution_status flags it, so retaining the prior hash as a baseline is sound. await deps.valueSets.upsertResolvedValueSet({ oid, - name: NAME_BY_OID[oid] ?? oid, + // The SAME name fallback as the success path. Partial failure is an explicitly supported outcome + // of this bulk import, so the error branch is not a rare corner: writing the bare OID here would + // overwrite a good alias with a worse one on any transient VSAC failure, and the operator would + // be looking at a catalog row named `2.16.840.1.113883.3.464...` with no way to know it once had + // a name. + name: deps.names?.[oid] ?? NAME_BY_OID[oid] ?? oid, version: null, source: "VSAC", codes: [], @@ -274,6 +325,7 @@ export async function main(argv: string[]): Promise { valueSets: stores.valueSets, events: stores.events, now: new Date().toISOString(), + ...(parsed.names ? { names: parsed.names } : {}), ...(parsed.manifest ? { manifest: parsed.manifest } : {}), ...(parsed.expansion ? { expansion: parsed.expansion } : {}), }); diff --git a/backend-ts/src/wiring/official-executor-adapter.ts b/backend-ts/src/wiring/official-executor-adapter.ts index 695ccd30..d6728af8 100644 --- a/backend-ts/src/wiring/official-executor-adapter.ts +++ b/backend-ts/src/wiring/official-executor-adapter.ts @@ -65,6 +65,7 @@ import { buildValueSetCache, calculateOfficialDetailed, normalizePeriodEnd, + referencedValueSets, referencedValueSetUrls, oidFromValueSetUrl, type ExpandedCode, @@ -247,6 +248,22 @@ export function requiredOids(artifact: OfficialArtifact): string[] { return referencedValueSetUrls(artifact.bundle).map(oidFromValueSetUrl); } +/** + * The value sets an artifact needs, with the CQL alias each is declared under. + * + * This is what `pnpm resolve-valuesets --official ` imports. Deriving the target list from + * the artifact — rather than a hand-kept OID table — is what keeps the importer and the executor's + * refusal in agreement: the same ELM that decides "this measure cannot run without these" decides what + * gets fetched. A hand-kept list is exactly how CMS122 ended up importable while 5 of its 26 canonicals + * were missing. + */ +export function requiredValueSets(artifact: OfficialArtifact): Array<{ oid: string; name?: string }> { + return referencedValueSets(artifact.bundle).map((v) => ({ + oid: oidFromValueSetUrl(v.url), + ...(v.name ? { name: v.name } : {}), + })); +} + /** * An `EvaluateMeasureBinding` that runs the official artifact for whichever measure it is asked about. * diff --git a/docs/DEPLOY.md b/docs/DEPLOY.md index 486ad12c..a5ccf76c 100644 --- a/docs/DEPLOY.md +++ b/docs/DEPLOY.md @@ -334,6 +334,24 @@ DATABASE_URL= WORKWELL_VSAC_API_KEY= \ pnpm resolve-valuesets --manifest Library/ecqm-update-2025-05-08 ``` +> **`--official ` — required before any measure is routed to official execution (PR-7a).** +> The default target is a hand-kept 21-OID table, and the vendored official artifacts need more than +> that: **CMS122 references 26 canonicals and CMS125 references 32** (35 distinct across both). The +> official executor refuses to run a measure when any referenced value set expands to zero codes — +> deliberately, because fqm treats an unexpandable set as *empty rather than missing*, and an empty set +> matches nothing, so the measure would report every subject out-of-population with no error anywhere. +> `--official` derives the target list from the artifact's own compiled ELM, so the importer and that +> refusal cannot disagree: +> +> ```bash +> cd backend-ts +> DATABASE_URL= WORKWELL_VSAC_API_KEY= \ +> pnpm resolve-valuesets --official cms122 --official cms125 --manifest +> ``` +> +> Repeatable and idempotent per OID; the 23 canonicals both measures share are expanded once. Rows are +> named from the ELM's CQL aliases rather than bare OIDs. + > **Release pinning + drift detection (#295).** Without `--manifest ` (or `--expansion > `; the two are mutually exclusive) VSAC serves **latest-active** semantics — a republish > silently changes our expansions and therefore the CMS122/CMS125 literal-diff results. The CLI now diff --git a/docs/JOURNAL.md b/docs/JOURNAL.md index 06bdd23e..42a6ab52 100644 --- a/docs/JOURNAL.md +++ b/docs/JOURNAL.md @@ -1,5 +1,35 @@ # Journal +## 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 +been quietly wrong for months. `pnpm resolve-valuesets` has always chosen its targets from +`CMS122V14.valueSets` — 21 OIDs, maintained by hand. The vendored artifacts need more: **CMS122 +references 26 canonicals, CMS125 references 32**, 35 distinct across both. So the official executor +refuses both measures on terminology today, and no amount of re-running the importer as configured would +have fixed it. + +`--official ` derives the target list from the artifact's own compiled ELM, which makes the +importer and the executor's refusal agree *by construction* — the same ELM that decides "this measure +cannot run without these" decides what gets fetched. A test asserts that equality for both measures +rather than trusting it. Rows are named from the ELM's CQL aliases ("Hospice Encounter", "Diabetes") +instead of bare OIDs, and the 23 canonicals the two measures share are expanded once. + +It deliberately imports *everything* referenced, including the four SupplementalDataElements sets that +`calculateSDEs: false` never retrieves. They are cheap, and an importer that second-guesses which +references "really" matter is precisely how the two lists drifted apart in the first place. + +Arithmetic worth recording because the test caught me: I asserted the union at 40 (26 + 32 − 18 shared). +It is **35**. The 18 was a different quantity entirely — how many of CMS125's 32 happen to be covered by +the 21 already imported — and I had carried it across from the PR-7a review into a place it did not +belong. 23 are shared. + +The import itself still needs the UMLS key, which lives only as a GitHub secret, so running it stays an +owner action. `docs/DEPLOY.md` now carries the exact command. + +Typecheck clean; full suite **1475 pass / 0 fail / 14 skipped**. + + ## 2026-07-25 (later still) — PR-7a: the official executor adapter, and the failure mode it refuses (branch `feat/official-executor-adapter`) Roadmap §7.2/§7.3. The adapter that runs a measure by executing the **official published artifact**