feat(standards): resolve-valuesets --official derives targets from the artifact (PR-7c)#340
Conversation
…e artifact (PR-7c) 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 <catalogId> 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) <[email protected]> Claude-Session: https://claude.ai/code/session_01Np757pAsNQSCPiq8S7qsxB
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56f19a6e6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await deps.valueSets.upsertResolvedValueSet({ | ||
| oid, | ||
| name: NAME_BY_OID[oid] ?? oid, | ||
| name: deps.names?.[oid] ?? NAME_BY_OID[oid] ?? oid, |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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.
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) <[email protected]> Claude-Session: https://claude.ai/code/session_01Np757pAsNQSCPiq8S7qsxB
1 participant
The smallest change that unblocks PR-9, and it exists because PR-7a's terminology refusal exposed a list that had been quietly wrong for months.
The problem
pnpm resolve-valuesetshas always chosen its targets fromCMS122V14.valueSets— 21 OIDs, maintained by hand. The vendored official artifacts need more:So the official executor refuses both measures on terminology today, and no amount of re-running the importer as configured would have fixed it. The refusal is deliberate: fqm treats an unexpandable value 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.
The fix
--official <catalogId>derives 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 (
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: falsenever 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 the test caught
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 — carried across from the PR-7a review into a place it did not belong. 23 are shared.
Still an owner action
Running the import needs the UMLS key, which lives only as a GitHub secret.
docs/DEPLOY.mdnow carries the exact command and says why it is required before any measure is routed officially.Verification
🤖 Generated with Claude Code
https://claude.ai/code/session_01Np757pAsNQSCPiq8S7qsxB