Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions backend-ts/packages/official-executor/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>();
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<string, { url: string; name?: string }>();
for (const entry of bundle.entry) {
const resource = entry.resource as {
resourceType?: string;
Expand All @@ -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). */
Expand Down
61 changes: 61 additions & 0 deletions backend-ts/src/run/cli/resolve-valuesets.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
});
62 changes: 57 additions & 5 deletions backend-ts/src/run/cli/resolve-valuesets.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 <oid> ...] [--measure cms122]
* pnpm resolve-valuesets [--oid <oid> ...] [--measure cms122] [--official <catalogId>]
*
* `--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';
*
Expand All @@ -13,27 +17,57 @@
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 <oid> ...] [--measure cms122] [--manifest <canonical> | --expansion <name>]";
"Usage: pnpm resolve-valuesets [--oid <oid> ...] [--measure cms122] [--official <catalogId>] " +
"[--manifest <canonical> | --expansion <name>]";
export const DEFAULT_OIDS: string[] = [...new Set(CMS122V14.valueSets.map((v) => v.oid))];
const NAME_BY_OID: Record<string, string> = Object.fromEntries(CMS122V14.valueSets.map((v) => [v.oid, v.name]));

/**
* `--official <catalogId>`: 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<string, string>;
/** Release pin (#295) — forwarded to $expand; mutually exclusive. */
manifest?: string;
expansion?: string;
}

export function parseArgs(args: string[]): ResolveArgs {
const oids: string[] = [];
const names: Record<string, string> = {};
let manifest: string | undefined;
let expansion: string | undefined;
for (let i = 0; i < args.length; i++) {
Expand All @@ -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}`);
Expand All @@ -63,14 +104,19 @@ 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 } : {}),
};
}

export interface RunResolveDeps {
oids: string[];
/** Display names by OID (artifact-derived); falls back to the CMS122 table, then the bare OID. */
names?: Record<string, string>;
client: VsacClient;
valueSets: ValueSetStore;
events: CaseEventStore;
Expand Down Expand Up @@ -142,7 +188,7 @@ export async function runResolve(deps: RunResolveDeps): Promise<ResolveResult> {
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,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve artifact aliases on failed imports

When an artifact-derived OID fails to expand, the catch path does not apply deps.names?.[oid], so CMS125-only targets are written with the bare OID and an existing alias can be overwritten. Because partial failures are an explicitly supported outcome of this bulk import, apply the same artifact-name fallback to both the RESOLVED and ERROR upserts.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — the ERROR upsert now uses the same deps.names?.[oid] ?? NAME_BY_OID[oid] ?? oid chain as the success path, with a test that expands one OID and fails another and asserts the failed row keeps its alias.

Worth stating why it matters more than it looks: partial failure is an explicitly supported outcome of this bulk import (that is the whole point of writing an ERROR row and continuing), so the error branch is not a rare corner. One transient VSAC blip would have left 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.

version: exp.version ?? null,
source: "VSAC",
codes,
Expand Down Expand Up @@ -205,7 +251,12 @@ export async function runResolve(deps: RunResolveDeps): Promise<ResolveResult> {
// 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: [],
Expand Down Expand Up @@ -274,6 +325,7 @@ export async function main(argv: string[]): Promise<number> {
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 } : {}),
});
Expand Down
17 changes: 17 additions & 0 deletions backend-ts/src/wiring/official-executor-adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ import {
buildValueSetCache,
calculateOfficialDetailed,
normalizePeriodEnd,
referencedValueSets,
referencedValueSetUrls,
oidFromValueSetUrl,
type ExpandedCode,
Expand Down Expand Up @@ -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 <catalogId>` 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.
*
Expand Down
18 changes: 18 additions & 0 deletions docs/DEPLOY.md
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,24 @@ DATABASE_URL=<neon-pooled> WORKWELL_VSAC_API_KEY=<umls-api-key> \
pnpm resolve-valuesets --manifest Library/ecqm-update-2025-05-08
```

> **`--official <catalogId>` — 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=<neon-pooled> WORKWELL_VSAC_API_KEY=<umls-api-key> \
> pnpm resolve-valuesets --official cms122 --official cms125 --manifest <release-canonical>
> ```
>
> 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 <canonical>` (or `--expansion
> <name>`; 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
Expand Down
30 changes: 30 additions & 0 deletions docs/JOURNAL.md
Original file line number Diff line number Diff line change
@@ -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 <catalogId>` 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**
Expand Down
Loading