From 56f19a6e6e0cf304338aa9f9efa421dd176eb183 Mon Sep 17 00:00:00 2001 From: Taleef Date: Sat, 25 Jul 2026 16:44:42 -0400 Subject: [PATCH 1/2] feat(standards): resolve-valuesets --official derives targets from the artifact (PR-7c) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. The importer has always chosen targets from a hand-kept 21-OID table. The vendored artifacts need more: CMS122 references 26 canonicals, CMS125 references 32, 35 distinct across both. The official executor refuses both measures on terminology today, and no amount of re-running the importer as configured would have fixed it. --official reads the target list from the artifact's own compiled ELM, so 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 rather than bare OIDs, and the 23 canonicals the two measures share are expanded once. It imports everything referenced, including the four SupplementalDataElements sets calculateSDEs:false never retrieves. They are cheap, and an importer that second-guesses which references really matter is how the two lists drifted apart in the first place. The test caught my arithmetic: I asserted the union at 40 (26 + 32 - 18 shared). It is 35 — the 18 was a different quantity (how many of CMS125's 32 are covered by the 21 already imported), carried across from the PR-7a review into a place it did not belong. Running the import still needs the UMLS key, so that stays an owner action; DEPLOY.md carries the exact command. Typecheck clean; 1475 pass / 0 fail / 14 skipped. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Np757pAsNQSCPiq8S7qsxB --- .../packages/official-executor/src/index.ts | 21 +++++-- .../src/run/cli/resolve-valuesets.test.ts | 34 ++++++++++++ backend-ts/src/run/cli/resolve-valuesets.ts | 55 +++++++++++++++++-- .../src/wiring/official-executor-adapter.ts | 17 ++++++ docs/DEPLOY.md | 18 ++++++ docs/JOURNAL.md | 30 ++++++++++ 6 files changed, 167 insertions(+), 8 deletions(-) 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..8605a31a 100644 --- a/backend-ts/src/run/cli/resolve-valuesets.test.ts +++ b/backend-ts/src/run/cli/resolve-valuesets.test.ts @@ -259,3 +259,37 @@ 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/); +}); diff --git a/backend-ts/src/run/cli/resolve-valuesets.ts b/backend-ts/src/run/cli/resolve-valuesets.ts index 8a1a1e7a..6de9c202 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, @@ -274,6 +320,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** From cf0d43a28c4e0db099cd606ccc43e095de4b5848 Mon Sep 17 00:00:00 2001 From: Taleef Date: Sat, 25 Jul 2026 17:05:07 -0400 Subject: [PATCH 2/2] fix(cli): keep the artifact alias on a failed expansion (Codex P2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ERROR upsert used the CMS122 name table only, so an artifact-derived OID that failed to expand was written with its bare OID — overwriting a good alias. Partial failure is an explicitly supported outcome of this bulk import, so the error branch is not a rare corner: one transient VSAC blip would leave an operator looking at a catalog row named 2.16.840.1.113883.3.464... with no way to know it ever had a name. Same fallback chain as the success path now. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Np757pAsNQSCPiq8S7qsxB --- .../src/run/cli/resolve-valuesets.test.ts | 27 +++++++++++++++++++ backend-ts/src/run/cli/resolve-valuesets.ts | 7 ++++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/backend-ts/src/run/cli/resolve-valuesets.test.ts b/backend-ts/src/run/cli/resolve-valuesets.test.ts index 8605a31a..5c8b9464 100644 --- a/backend-ts/src/run/cli/resolve-valuesets.test.ts +++ b/backend-ts/src/run/cli/resolve-valuesets.test.ts @@ -293,3 +293,30 @@ test("--official derives the import target from the artifact the executor actual 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 6de9c202..ba03887e 100644 --- a/backend-ts/src/run/cli/resolve-valuesets.ts +++ b/backend-ts/src/run/cli/resolve-valuesets.ts @@ -251,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: [],