fix(hts): resolve versionless canonicals to the owning package, not a copy (#200)#261
Open
mauripunzueta wants to merge 8 commits into
Open
fix(hts): resolve versionless canonicals to the owning package, not a copy (#200)#261mauripunzueta wants to merge 8 commits into
mauripunzueta wants to merge 8 commits into
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
mauripunzueta
force-pushed
the
fix/200-codesystem-version-resolution
branch
from
6645f38 to
276e7e9
Compare
mauripunzueta
added a commit
that referenced
this pull request
Jul 14, 2026
Ten-reviewer review of #261 surfaced two ways the provenance model was wrong, both caused by treating 'no evidence of ownership' as 'is the owner'. 1. A package manifest need not describe its content's authority. us.nlm.vsac declares the fhir.org REGISTRY base yet ships 14,850 ValueSets under cts.nlm.nih.gov; us.cdc.phinvads likewise. The old rule branded all 16,817 of them AUTHORITY_FOREIGN_COPY(2) — a claim we never established. And because vs_precedence_order_by has no content/has-concepts tier beneath the provenance tier (value_sets has neither column), any rank-0 row on one of those URLs would have outranked the real VSAC definition unconditionally, regardless of version. A single REST upload or one more entry in vsac-supplement.bundle.json would have silently served a stub in place of a national terminology. That is a landmine this PR introduced. 2. Non-package writes defaulted to AUTHORITY_OWNER(0). Replaying hl7.fhir.r4.core's truncated copy through $import-bundle or a REST PUT stamps rank 0 on the 4.0.1 row; the upsert's MIN only ever LOWERS a rank, so the copy would then tie THO's original on every tier, win on version DESC, and reinstate issue #200 PERMANENTLY, with no re-import able to recover it. Fix: promote only on positive evidence, never demote on its absence. AUTHORITY_OWNER(0) now requires either a package whose declared canonical base covers the URL, or a native importer loading the publisher's own distribution. Everything else is AUTHORITY_UNKNOWN(1) — the default. Conflating 'stale copy' with 'unattributable original' is safe precisely because rank is only ever a TIEBREAK among rows sharing a canonical: being UNKNOWN costs a row nothing unless a demonstrable owner of that same URL exists. VSAC keeps its URLs; r4.core's copies still lose to THO. This also closes the trust inversion the security review flagged: a REST upload can no longer outrank a native SNOMED/LOINC import, because it cannot claim OWNER. Verified against a real SQLite over the shared ORDER BY: #200 still fixed, the bundle-replay attack no longer re-breaks it, VSAC is no longer outrankable, and the tx-ecosystem version fixtures and the SNOMED version=current canary are unaffected.
mauripunzueta
added a commit
that referenced
this pull request
Jul 14, 2026
Two councils (gate review + implementation review) went through this branch. They found real defects; this commit fixes all of them. **1. Provenance was too generous with rank 0 — two ways to break the fix.** (a) A package manifest need not describe its content's authority. `us.nlm.vsac` declares the fhir.org REGISTRY base yet ships 14,850 ValueSets under `cts.nlm.nih.gov`; `us.cdc.phinvads` likewise. The old rule branded all 16,817 of them a "foreign copy" — a claim we never established. And because `vs_precedence_order_by` has no content/has-concepts tier beneath the provenance tier (`value_sets` has neither column), any rank-0 row on one of those URLs would have outranked the real VSAC definition unconditionally, regardless of version. One REST upload, or one more entry in `vsac-supplement.bundle.json`, and the server silently serves a stub in place of a national terminology. This PR introduced that. (b) Non-package writes defaulted to OWNER(0). Replaying `hl7.fhir.r4.core`'s truncated copy through `$import-bundle` or a REST PUT stamps rank 0 on the 4.0.1 row; the upsert's MIN only ever LOWERS a rank, so that copy would then tie THO's original on every tier, win on `version DESC`, and reinstate issue #200 PERMANENTLY, with no re-import able to recover it. Fix: promote only on positive evidence, never demote on its absence. `AUTHORITY_OWNER(0)` now requires a package whose declared canonical base covers the URL, or a native importer loading the publisher's own distribution. Everything else is `AUTHORITY_UNKNOWN(1)`, the default. Conflating "stale copy" with "unattributable original" is safe precisely because rank is only ever a TIEBREAK among rows sharing a canonical: being UNKNOWN costs a row nothing unless a demonstrable owner of that same URL exists. VSAC keeps its URLs; r4.core's copies still lose to THO. This also closes the trust inversion the security review flagged — a REST upload can no longer outrank a native SNOMED/LOINC import, because it cannot claim OWNER. **2. Four same-URL resolvers were missed**, so `$lookup` could read `language` off the truncated copy while reading displays from the authoritative row — the exact incoherence this PR claims to eliminate. `code_system_language` had partial tiers on SQLite and NO ordering at all on Postgres; `supplement_target` had no ordering on either (so a supplement whose URL collides could silently vanish). All four now use the shared helper. My original sweep grepped for the old ordering's *shape*, which is why a query with no ORDER BY slipped through. **3. Two Rust-side ValueSet sorts in `expand.rs` contradicted the backend.** They sort `search()` JSON by version string, but precedence now depends on `authority_rank`, a storage column deliberately absent from the resource — so the response echoed one ValueSet while the backend expanded another. Added `ValueSetOperations::value_set_version_for_url` so the operations layer ASKS the backend which row it resolved instead of re-deriving it. **4. Postgres kept the error-message defect SQLite fixed.** `validate_code`'s miss path re-queried by URL, which — now that version pins are honoured — can return a different row than the one validated. It cited a version the code was never checked against, and worse, `content` gates the fragment→warning vs complete→error branch, so it could silently downgrade a genuine error to a warning. **5. Security/robustness:** `_htsSourceCanonical` was reachable from `POST /import` — an untrusted body could set the value deciding which definition is authoritative. Provenance is now an explicit parameter (`parse_bundle_from_package`), routed through the existing `import_parsed` seam, and the key is gone. `authority_rank_for` gained a path-boundary check (`http://hl7.org/fhir` no longer claims `/fhirpath/...`). **6. The magic sentinel `9`** is now `AUTHORITY_UNCLAIMED`, documented alongside the deliberate NULL asymmetry (weakest on write, strongest on read). **7. A CI guard** (`scripts/check-canonical-precedence.sh`) now fails the build if a hand-rolled same-URL ordering reappears. A doc comment is not an enforcement mechanism, and ad-hoc orderings drifting apart are how this bug class persisted. Also corrected comments that contradicted the code (claims of `DEFAULT 0` on a nullable column; "highest version" on a list that is now in precedence order; an over-claim that MIN makes the whole ROW order-independent when it only does so for the rank), and restored a doc comment my earlier edit had accidentally stolen from `resolve_cs_version_pattern`. New tests: ValueSet provenance end-to-end (previously untested — half the shipped surface); the MIN/LEAST upsert conflict path (previously never exercised, because the other tests use different versions and so never collide a row); and legacy-NULL demotion. Verified against a real SQLite over the shared ORDER BY that #200 stays fixed, the bundle-replay attack no longer re-breaks it, VSAC is no longer outrankable, and the tx-ecosystem version fixtures and SNOMED version=current canary are unaffected.
mauripunzueta
force-pushed
the
fix/200-codesystem-version-resolution
branch
from
276e7e9 to
1495231
Compare
… copy (#200) A bare $validate-code against http://terminology.hl7.org/CodeSystem/audit-event-type rejected valid THO codes such as `object`, because the canonical URL resolved to the truncated 4.0.1 copy bundled in hl7.fhir.r4.core instead of the complete THO 1.0.0 definition. Neither a semver fix nor a content-based rule can separate those two rows: both are `content: complete`, both carry concepts, and 4.0.1 outsorts 1.0.0 under any version ordering — the "4.0.1" is the FHIR *release* version, not the code system's own. The only thing that distinguishes them is provenance. hl7.fhir.r4.core declares canonical `http://hl7.org/fhir` yet re-ships 746 CodeSystems and 603 ValueSets owned by terminology.hl7.org. 208 of those currently resolve to the stale copy, and in 32 the winning row is truncated, leaving 1,297 concepts unvalidatable via a bare call (`claimcareteamrole` code `rehab` fails the same way as `audit-event-type` code `object`). Record provenance at import: a package that publishes a URL outside its own declared `canonical` base is re-publishing someone else's resource, so it is a copy. This is stored as `authority_rank` on `code_systems` / `value_sets` and becomes one tier of the same-URL precedence rule. It needs no hardcoded URL list and no package-priority table — it self-maintains as packages are added. Copies are ranked, not discarded: `version=4.0.1` still resolves, and `resource_json` stays byte-faithful to what the package shipped. The tier deliberately sits below the content and has-concepts tiers: a populated copy must still beat an empty stub from the owning package, and those two tiers are the only thing keeping SNOMED `version=current` resolving to the real edition (the stub's version string is literally "current", and 'c' > '2', so it text-sorts above "20260501"). Upgrading an existing database: `authority_rank` cannot be backfilled — nothing already stored on a row says which package supplied it. The column is nullable (NULL = "never claimed", coalesced to 0, i.e. previous behaviour), and adding it is the signal that the database predates provenance, so the migration clears the `.tgz` entries from the bootstrap_imports ledger and the next startup re-imports those packages. Without that the ledger — which skips files whose size and mtime are unchanged — would leave every row at its default and the fix would be a silent no-op in production. Only `.tgz` rows are cleared; the multi-GB SNOMED/LOINC/RxNorm archives are not re-imported. Also in this change, because they are the same defect: * Unify same-URL resolution. It had no home (the trait has no resolve method), so it was reimplemented in backend SQL at 34 sites in three mutually inconsistent variants. All now share `cs_precedence_order_by` / `vs_precedence_order_by`, embedded verbatim by both backends so they cannot drift. This fixes a latent Postgres-only bug: `resolve_system_id_pg` lacked the content and has-concepts tiers while its doc comment claimed it matched SQLite, so on Postgres an empty `content=not-present` stub could beat a real import and `$expand` would return nothing. * $validate-code's "unknown code" diagnostic re-queried by URL with its own ordering, so a miss could name a version the code was never validated against. It now reports the row that was actually resolved. * CodeSystem/$validate-code honours `systemVersion`, `system-version`, `force-system-version` and `check-system-version`. These were already honoured on $expand and ValueSet/$validate-code but silently ignored here, so a client pinning systemVersion=1.0.0 got the default row anyway (the second defect reported in #200). Caching is unaffected: the canonical pins already disable the handler cache. Both backends are covered. Regression tests drive the real .tgz importer on SQLite and Postgres, assert the outcome is independent of import order, and pin the SNOMED `version=current` canary.
…ce helpers
CI Linting failed on rustfmt diffs. Also inline the named format argument
(`{a}`, `a = alias`) as `{alias}` so clippy's uninlined_format_args does not
trip the warnings-denied lint gate — the Linting job aborted at fmt before
clippy ran, so this is pre-emptive.
… helpers clippy::needless_borrows_for_generic_args — `tar::Builder::append_data` takes `P: AsRef<Path>`, and `String` already satisfies it, so the `&` was redundant.
The pin handling was implemented but untested. Issue #200 explicitly reports that CodeSystem/$validate-code honoured only `version`, so the pins deserve direct coverage rather than riding on the resolver tests. Two versions of one canonical URL, each defining a code the other lacks, so the default resolution (newest) cannot satisfy any of the pinned assertions — every test fails if the parameter is ignored, which is exactly the reported bug: * unpinned resolves to the newest version (baseline) * `systemVersion` selects the older row * the canonical `system-version=<url>|<version>` form selects the older row * `force-system-version` overrides an explicit `version` * an explicit `version` beats a `system-version` default (it is a default, not a force)
Codecov flagged the `if column_added` branch of `migrate_authority_rank` as
uncovered, and it was right to: every existing test builds its database from
`SCHEMA`, which already declares the column, so the ALTER always fails with
"duplicate column name" and the branch never executes. That branch is the only
thing that makes this fix reach an already-populated server — without the ledger
clear, packages never re-import, `authority_rank` stays NULL on every row, and
the whole change is a silent no-op in production. It should not have shipped
untested.
Adds a legacy-shaped database (resource tables with no `authority_rank`, ledger
already populated) and asserts the upgrade contract:
* both columns are added
* exactly the `.tgz` ledger rows are cleared, so packages re-import — the bulk
SNOMED/LOINC/RxNorm archives keep their entries and are NOT re-imported
* pre-existing rows come out NULL ("unclaimed"), not 0, which is what preserves
the old behaviour until re-import and lets the upsert still demote a copy
* the migration is idempotent: a second run does not re-clear the ledger, so a
restart does not re-import every package
* on a fresh `SCHEMA` database it is a no-op and leaves the ledger alone
Also covers manifest parsing in the tgz importer: a package.json with no
`canonical` key must fail open (no claim, everything stays authoritative), and
manifests must never be imported as resources or counted as parse errors.
Two councils (gate review + implementation review) went through this branch. They found real defects; this commit fixes all of them. **1. Provenance was too generous with rank 0 — two ways to break the fix.** (a) A package manifest need not describe its content's authority. `us.nlm.vsac` declares the fhir.org REGISTRY base yet ships 14,850 ValueSets under `cts.nlm.nih.gov`; `us.cdc.phinvads` likewise. The old rule branded all 16,817 of them a "foreign copy" — a claim we never established. And because `vs_precedence_order_by` has no content/has-concepts tier beneath the provenance tier (`value_sets` has neither column), any rank-0 row on one of those URLs would have outranked the real VSAC definition unconditionally, regardless of version. One REST upload, or one more entry in `vsac-supplement.bundle.json`, and the server silently serves a stub in place of a national terminology. This PR introduced that. (b) Non-package writes defaulted to OWNER(0). Replaying `hl7.fhir.r4.core`'s truncated copy through `$import-bundle` or a REST PUT stamps rank 0 on the 4.0.1 row; the upsert's MIN only ever LOWERS a rank, so that copy would then tie THO's original on every tier, win on `version DESC`, and reinstate issue #200 PERMANENTLY, with no re-import able to recover it. Fix: promote only on positive evidence, never demote on its absence. `AUTHORITY_OWNER(0)` now requires a package whose declared canonical base covers the URL, or a native importer loading the publisher's own distribution. Everything else is `AUTHORITY_UNKNOWN(1)`, the default. Conflating "stale copy" with "unattributable original" is safe precisely because rank is only ever a TIEBREAK among rows sharing a canonical: being UNKNOWN costs a row nothing unless a demonstrable owner of that same URL exists. VSAC keeps its URLs; r4.core's copies still lose to THO. This also closes the trust inversion the security review flagged — a REST upload can no longer outrank a native SNOMED/LOINC import, because it cannot claim OWNER. **2. Four same-URL resolvers were missed**, so `$lookup` could read `language` off the truncated copy while reading displays from the authoritative row — the exact incoherence this PR claims to eliminate. `code_system_language` had partial tiers on SQLite and NO ordering at all on Postgres; `supplement_target` had no ordering on either (so a supplement whose URL collides could silently vanish). All four now use the shared helper. My original sweep grepped for the old ordering's *shape*, which is why a query with no ORDER BY slipped through. **3. Two Rust-side ValueSet sorts in `expand.rs` contradicted the backend.** They sort `search()` JSON by version string, but precedence now depends on `authority_rank`, a storage column deliberately absent from the resource — so the response echoed one ValueSet while the backend expanded another. Added `ValueSetOperations::value_set_version_for_url` so the operations layer ASKS the backend which row it resolved instead of re-deriving it. **4. Postgres kept the error-message defect SQLite fixed.** `validate_code`'s miss path re-queried by URL, which — now that version pins are honoured — can return a different row than the one validated. It cited a version the code was never checked against, and worse, `content` gates the fragment→warning vs complete→error branch, so it could silently downgrade a genuine error to a warning. **5. Security/robustness:** `_htsSourceCanonical` was reachable from `POST /import` — an untrusted body could set the value deciding which definition is authoritative. Provenance is now an explicit parameter (`parse_bundle_from_package`), routed through the existing `import_parsed` seam, and the key is gone. `authority_rank_for` gained a path-boundary check (`http://hl7.org/fhir` no longer claims `/fhirpath/...`). **6. The magic sentinel `9`** is now `AUTHORITY_UNCLAIMED`, documented alongside the deliberate NULL asymmetry (weakest on write, strongest on read). **7. A CI guard** (`scripts/check-canonical-precedence.sh`) now fails the build if a hand-rolled same-URL ordering reappears. A doc comment is not an enforcement mechanism, and ad-hoc orderings drifting apart are how this bug class persisted. Also corrected comments that contradicted the code (claims of `DEFAULT 0` on a nullable column; "highest version" on a list that is now in precedence order; an over-claim that MIN makes the whole ROW order-independent when it only does so for the rank), and restored a doc comment my earlier edit had accidentally stolen from `resolve_cs_version_pattern`. New tests: ValueSet provenance end-to-end (previously untested — half the shipped surface); the MIN/LEAST upsert conflict path (previously never exercised, because the other tests use different versions and so never collide a row); and legacy-NULL demotion. Verified against a real SQLite over the shared ORDER BY that #200 stays fixed, the bundle-replay attack no longer re-breaks it, VSAC is no longer outrankable, and the tx-ecosystem version fixtures and SNOMED version=current canary are unaffected.
mauripunzueta
force-pushed
the
fix/200-codesystem-version-resolution
branch
from
1495231 to
a3c0cba
Compare
Contributor
Author
|
✅ Update (resolved): The inherited It was fixed by #273, merged into All 14 tests added by this PR pass — the provenance, upsert-semantics, migration, and version-pin tests are all green on both SQLite and Postgres. Edited to reflect that #273 (not #280) landed the fix and CI is now green. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #200.
The bug is not a version-sorting bug
A bare
$validate-codeonhttp://terminology.hl7.org/CodeSystem/audit-event-typerejects valid THO codes such asobject, resolving to the truncated4.0.1row instead of the complete THO1.0.0one.The two rows are near-twins — same FHIR id, same name, same status, both
content: complete, both non-empty:hl7.fhir.r4.corecopy4.0.1rest)hl7.terminologyoriginal1.0.0rest,hl7-v2,hl7-v3,document,object)So a semver fix would not help (
4.0.1>1.0.0under any ordering — the4.0.1is the FHIR release version, not the code system's own), and a content-based rule would not help (both arecomplete). Nothing intrinsic to the rows separates them. Only provenance does.It is much wider than
audit-event-typehl7.fhir.r4.coredeclares canonicalhttp://hl7.org/fhiryet re-ships 746 CodeSystems and 603 ValueSets owned byterminology.hl7.org. Measured against the vendored packages:Confirmed live on
hts.heliossoftware.com—claimcareteamrole+rehabfails exactly likeaudit-event-type+object.The rule
Every FHIR NPM package declares a
canonicalbase inpackage.json. A package publishing a URL outside its own base is re-publishing someone else's resource — a copy. Recorded at import asauthority_rank(0= owner or non-package source,2= foreign copy) and used as one tier of same-URL precedence.No hardcoded URL list, no hand-maintained package-priority table; it self-maintains as packages are added, and covers ValueSets for free. Resources arriving outside any package (REST writes,
$import-bundle, the native SNOMED/LOINC importers) are authoritative, so an operator-supplied resource outranks any vendored copy.Order:
content→has-concepts→authority_rank→version→id.The new tier sits below the first two on purpose: a populated copy must still beat an empty stub from the owning package, and those tiers are the only thing keeping SNOMED
version=currentresolving to the real edition (the stub's version string is literally"current", and'c' > '2', so it text-sorts above"20260501").Copies are ranked, not discarded —
version=4.0.1still resolves, andresource_jsonstays byte-faithful to what the package shipped.Upgrading an existing database (the part that would otherwise no-op)
authority_rankcannot be backfilled — nothing already stored on a row says which package supplied it. So:NULLmeans "never claimed" and readers coalesce it to0, i.e. an un-re-imported database behaves exactly as before.tgzrows from thebootstrap_importsledger and the next startup re-imports those packagesWithout that, the ledger (which skips files whose size and mtime are unchanged) would leave every row at its default and this fix would be a silent no-op on the live server. Only
.tgzentries are cleared — the multi-GB SNOMED/LOINC/RxNorm archives are not re-imported.Folded in, because it is the same defect
cs_precedence_order_by/vs_precedence_order_by, embedded verbatim by both backends so they cannot drift. This fixes a latent Postgres-only bug:resolve_system_id_pglacked the content and has-concepts tiers while its doc comment claimed it matched SQLite — so on Postgres an emptycontent=not-presentstub could beat a real import and$expandreturned nothing.CodeSystem/$validate-codenow honourssystemVersion/system-version/force-system-version/check-system-version— already honoured on$expandandValueSet/$validate-code, silently ignored here (the second defect in HTS: $validate-code default-resolves audit-event-type to incomplete 4.0.1 fragment instead of complete THO 1.0.0 #200). Caching is unaffected: the canonical pins already disable the handler cache.Both backends are covered, per the multi-DB requirement.
Tests
Regression tests drive the real
.tgzimporter (a plain$import-bundlecarries no provenance, so it could not exercise the mechanism) on SQLite and Postgres:$validate-coderesolves to the owning package's complete rowversion=4.0.1still reaches the copyauthority_rank_forunit coverage, including the fail-open case (package declares no canonical)Verification caveat
This environment has no C linker, so I could not run
cargo testlocally. Every changed file parses (rustfmt), and I executed the real schema and the real SQL against a live SQLite covering: both import orders, the legacy-DB upgrade path (unchanged before re-import, fixed after), the tx-ecosystemvnnfixtures (tie on rank → highest version still wins), the SNOMEDversion=currentcanary, and empty-owner-vs-populated-copy. CI is the first real compile and test run — reviewing its result is the next step.Acceptance
GET /CodeSystem/$validate-code?url=http://terminology.hl7.org/CodeSystem/audit-event-type&code=object→result: true, so theTX_SYSTEM_VERSION_OVERRIDESpin invalidate_report.py(on the unmerged #183 branch) is no longer needed.