[Feature]: Merge flip-omop-db into the monorepo as the trust/omop-db build source

[atriaybagur]

Closes #834
Implements #842 and #843 (folded in by request — each keeps one residual org-admin action, tracked there).

What

Merges the private flip-omop-db repo into the monorepo as trust/omop-db/ — and takes the opportunity to make every published OMOP artifact licence-clean: the image, the pgdata tarballs and the HF dataset now carry no licensed vocabulary content at all.

Snapshot import, not a history merge: one verbatim commit at flip-omop-db main (5cf75d4); the private history (personal identifiers, private-repo references, no DCO) stays archived in the private repo.

The vocab-free architecture (#842 + #843)

The core OMOP vocabulary (an OHDSI Athena export: SNOMED CT Int/US/UK, LOINC 2.77, NHS Read v2, dm+d, … — 59-vocabulary roster table in the README) is licensed material, and it was 99.8% of the published payload: the old image carried a 3.6 GB baked bundle and each 2.2 GB pgdata tarball was ~8 GB of vocab tables around ~15 MB of synthetic cohort. Now:

• Image (Dockerfile): schema init chain + read-only roles + seed-time helpers only. No data inputs, no credentials → CI-published via the new test-gated docker_build_omop_db.yml (same workflow_run pattern as the other services), and ~4.3 GB → ~450 MB.
• pgdata tarballs: vocab-free — schema + synthetic cohort + the freely-redistributable DICOM vocab. 2.2 GB → ~11 MB each; published to the public HF dataset as .data_version 20260729 (built via make populate CORE_VOCAB=0 + make export-pgdata).
• One uniform seeding step (files/load_core_vocab.sh): streams the licensed bundle into a running database over TCP (COPY FROM STDIN — no mounts, no server-side files, no image layers) and applies the FK constraints; core-aware idempotency guards tolerate the DICOM vocab already present in the tarballs. Wired into all three environments: dev make load-omop-vocab, EC2 via a new Ansible play inside seed-trust-data (throwaway container on loopback, kit credentials passed by the AWS Makefile), Kubernetes via the chart's omop-vocab-load post-install/upgrade hook Job (omopDb.vocabLoad values + schema).
• Vocab acquisition — two licence-proper paths only (README "The core vocabulary bundle"): org members make fetch-vocab-core from s3://flipdev-aicentre/vocab/ (VOCAB_S3_BUCKET parameterised, temp+rename robustness); everyone else self-serves an equivalent export from OHDSI Athena under their own licences (NHS TRUD pointer for UK editions). The image-extraction fallback is removed.
• The DICOM bundle is a byte-for-byte copy of four files from DICOM2OMOP's files/OMOP CDM Staging/ at upstream commit 1ef3354 (verified by content diff; Apache 2.0; pinned in THIRD_PARTY_NOTICES), fetched anonymously from HF with the upstream pickle converted to CSV — no pickle deserialisation anywhere.

Also in this PR

• Producer + consumer merged: OMOP CDM 5.4 + MI-CDM DDL/PK/index/constraint SQL, readonly-role SQL, build compose and populate tooling join the existing harness; .data_version path untouched (AWS Makefile hardcodes it).
• Canonical mock dataset + N-trust split (omop_db_tools.dataset): the synthetic per-trust CSVs (verified: FAK… accession IDs, fake NHS numbers) live as ONE dataset on HF (omop-csv/20260729/, source_trust provenance). legacy mode reproduces the original two-trust membership exactly (frame-equal, keeps accession IDs consistent with the mock Orthanc volumes); modulo partitions person_id % N. Two-trust build stack; N>2 fails fast until a compose service + port exist.
• Security: no baked build-arg credentials; SQL identifiers validated; sqlalchemy.URL.create escaping; build compose carries the runtime service's hardening (no-new-privileges, cap_drop: ALL + five caps — fresh initdb verified under it); OCI source labels re-link the GHCR package to this repo; data_analyst_reader rotation paths documented (password lives in pgdata, not the image).
• Multi-agent review round applied on-branch (4 specialist agents): ON_ERROR_STOP on constraints, temp+rename fetches, NUM_TRUSTS-derived fan-out with port fail-fast, bounded first-boot readiness wait, gapped-source_trust and NaN-person_id guards, per-source asymmetry rejection in build_canonical, validate-before-destructive-clean, .env.build.example un-swallowed from trust/.gitignore, radiology_occurrence→image_occurrence across data-access-api OpenAPI examples and trust docs. Unit tests 26 → 46.
• CI: new Trust - OMOP DB CI (lint+mypy+unit) and docker_build_omop_db.yml; omop-db in root unit_test; uv-lock hook + UV_PROJECTS.
• Docs: component-omop-database.rst expanded; README documents both vocab bundles (roster, provenance, acquisition) and the full build/populate/export/seed workflows; CLAUDE/AGENTS pairs throughout.

Verification (all live on this branch)

• Phase A — vocab-free export build: populate CORE_VOCAB=0 + export-pgdata → 11.4 MB tarballs.
• Phase B — runtime-seed simulation: seeded a DB from the published tarball, ran load_core_vocab.sh + constraints → 4,187 persons, 6,108,439 concepts (SNOMED 1,084,286 + DICOM 8,811 coexisting), 173 FKs, the user-guide's example JOIN omop.concept query returns 2,479 rows, data_analyst_reader write denied, loader re-run is a clean no-op.
• Phase C — standard full populate: both trusts exact counts (4,187/4,186 persons, full SNOMED, 173 FKs).
• Kubernetes: chart lints + renders (job present; absent under omopDb.enabled=false), and the kind E2E does a real cluster install — the StatefulSet + tarball init path are cluster-verified, and the vocab-load hook's machinery was exercised for real (it ran and failed on S3 auth in credential-less CI, as designed; it is disabled in the CI values). Not yet run to completion with real S3 credentials in a cluster — the loader script itself is identical to the host-side-proven one; verify the credentialed Job on the first real k8s deploy. Ansible play: live-verified end-to-end on a stag trust EC2 (targeted terraform create → provisioning → 11 MB tarball restore → instance-role S3 fetch → loader + constraints; on-box verification identical to the local numbers: 4,187 persons / 6.1 M concepts / 173 FKs / roles enforced). Found and fixed en route: per-env vocab bucket wiring, and bundles must be uploaded --sse AES256 (trust role lacks kms:Decrypt on bucket KMS keys — documented in the README).
• pre-commit clean on the branch diff; omop-db tools 46 tests + data-access-api 151 tests green.
• HF anonymous fetches verified: new tarballs, canonical CSVs, DICOM bundle.

Review round 3 (@7492916c) — the approving review's four items

All four were flagged non-blocking on the approval; each is fixed, and the same defect is swept where it recurred elsewhere.

1. Attribution on the imported CDM SQL. The four OMOPCDM_postgresql_5.4_*.sql files carried a sole Copyright 2024 Guy's and St Thomas' header while THIRD_PARTY_NOTICES.md (added by this PR) states they derive from OHDSI CommonDataModel — so the header contradicted our own notices file, which makes this an attribution fix rather than a stale-year one. Rewritten in the dual-attribution form already used for vendored SQL at trust/xnat/postgres/XNAT.sql: upstream OHDSI copyright + SPDX, then FLIP modifications. Also drops the U+2019 smart quotes these four alone carried. Headers only — no SQL changed.
2. OMOP_DB_TAG=latest. Correct — and this PR is what made it wrong, since the new CI publishing redefines :latest as the production image, so a dev kit ran omop-db from prod while its four siblings ran :stag. The composes now resolve ${OMOP_DB_TAG:-${DOCKER_TAG}}, keeping the per-service override for exactly the case XNAT_TAG exists for (omop-db changes rarely, so its branch tag is the one most likely to be absent). No trust/Makefile export needed — the kit reaches compose via --env-file and DOCKER_TAG is already exported for dev. Swept the same :latest default out of the Helm chart (omopDb.image.tag → stag, matching all eight siblings; the tag is now load-bearing because the vocab-load Job runs the image's own loader) and the Ansible play, which now receives the env's DOCKER_TAG from the Makefile.
3. Helm vocabLoad default. Gated the Job on s3Bucket and defaulted it empty, mirroring initJob's own empty-bucket idiom — no bucket ⇒ no Job ⇒ clean install. A public fallback is not legally available here (SNOMED CT / LOINC / Read v2 / dm+d), which is what makes the initJob pattern the right shape rather than a mirror. This also unhardcodes flipdev-aicentre, which a stag/prod EKS install could never read. Since "no vocabulary" is otherwise silent — cohort queries joining omop.concept return nothing while every probe stays green — the skip is announced in a new NOTES.txt, documented in the chart README (+ a troubleshooting entry), and sync_k8s_kit.py now emits the bucket from the kit's AICENTRE_BUCKET_NAME so the org path stays zero-touch.
4. Ansible cleanup on failure. Wrapped the play in block/always. This also clears a retry trap the flat list had: the creates: guards key on the unpacked directory, so a re-run after a partial failure skipped both the download and the unzip and reused a possibly-truncated bundle. docker rm -f gets failed_when: false, since in an always: it runs even when the container never started and would otherwise fail the play it exists to clean up after.

Behaviour in the wild is unchanged by item 2: OMOP_DB_TAG is not in HUB_SHARED_KEYS, so sync-trust-kit never strips it — existing kits keep their literal OMOP_DB_TAG=latest line and resolve exactly as before. Only kits newly scaffolded by make new-trust pick up the new default.

Verified on the branch: helm lint; the template gate matrix (no bucket / bucket set / enabled=false / CI values); the NOTES warning fires only when the vocabulary is genuinely absent; compose resolves omop-db:stag beside its four siblings and still honours a pinned OMOP_DB_TAG; ansible-playbook --syntax-check + --list-tasks; prettier (the one hard CI gate on deploy/providers/AWS/*.yml); k8s sync tests 13 passed; omop-db ruff + mypy + 47 unit tests. The Ansible path is not re-run live — the stag trust EC2 used for the original verification was destroyed after that round; the changed play is otherwise identical to the live-verified one.

Post-merge admin actions (kept open on #842/#843)

1. [Task]: Rebuild omop-db image vocab-free; enable CI publish; purge vocab-bearing package versions #842: delete the old vocab-bearing GHCR package versions once the first CI-published vocab-free image is live (:latest is replaced on the first main publish; historical sha-pinned versions need manual deletion). Note the currently published tags are latest, trust1, trust2, stag — all hand-pushed, all predating the vocab-free rework, so all still carry the licensed bundle.
2. [Task]: Decide licensing posture of the public HF pgdata tarballs (licensed vocabularies inside) #843: delete the old trust<N>_pgdata_20260129.tar (2.2 GB, vocab-bearing) files from the public HF dataset once deployments are on 20260729.
3. Archive londonaicentre/flip-omop-db with a pointer README; org-admin confirm the GHCR package re-links to FLIP after the first labelled push.
4. Seed a vocab-free :stag: gh workflow run docker_build_omop_db.yml --ref develop once after merge. CI has never published this image, and the workflow_run gate does not fire retroactively on the merge that introduces it (workflow_dispatch bypasses the gate). Until then :stag is the old hand-pushed image, which lacks load_core_vocab.sh. :prod does not exist yet and is created on the first main promotion.

Note for reviewers: existing dev environments will download the new 11 MB volumes on their next update-omop-data (version bump) and must then run make load-omop-vocab once per trust — called out in the README.

develop merged in (@8323dba7) + review round 4 (@f64446ea)

Merge. origin/develop (147 commits) merged in; 6 conflicts. Only one had substance:
trust/data-access-api/tests/routers/test_cohort.py — #839 deleted the _parse_and_emit helper this
branch's side still called, so the resolution takes develop's validate_query shape carrying this
branch's radiology_occurrence→image_occurrence rename (that test held the last such reference in
the tree; image_occurrence was already canonical at the merge-base, in the trust-api integration
fixtures). The other five were unions: test_helm_chart.yml (this branch's two vocab-load gates +
develop's FLIP-PT-091 orthanc gate) and both CLAUDE/AGENTS pairs. No test was lost — verified by
enumerating every def test_* across both parents and the merge.

Review round 4. A full-diff review of this PR found 11 defects; each was reproduced locally
before being fixed, and four were downgraded from their initial severity after re-checking.

Deployment

1. make ansible-init — and so full-deploy — failed outright. The new vocab-load play asserts
   omop_postgres_password, which only seed-trust-data passes. ansible-init runs before
   register-trusts in every chain, has no KIT in scope (hence its trust_num fallback to 1) and
   structurally cannot supply it, so passing the vars there is not an option — the play is now
   excluded via the omop-vocab tag it already carries. Verified: --list-tasks --skip-tags omop-vocab selects zero tasks from that play, --tags data still selects all nine.
2. Licensed content could be left on the host. A failing task in Ansible's always: aborts the
   remaining ones (verified), so with the container removal ahead of it a docker rm against a dead
   daemon skipped the ~3.6 GB bundle deletion. Deletion now runs first.
3. creates: guards reused a truncated bundle — both keyed on the unpacked directory, so an
   interrupted unzip made the next run skip download and unzip. Both stages now stage into a temp
   path and rename on success, matching fetch-vocab-core.

CI
4. The uv-lock cooldown gate never checked trust/omop-db — secret-scanning.yml hardcodes a
second copy of the project list that this PR didn't update, so the supply-chain backstop CLAUDE.md
names was silently absent for the new project.
5. Neither vocab-load render gate could fail. ! grep -q is exempt from set -e and worked only
by being the last line (appending any line disarms it — verified), and the absence assertion also
passed on an empty render. Both are now explicit if + ::error::, with a positive control.
6. test_trust_omop_db.yml lacked environment: flip, so CODECOV_TOKEN resolved empty and the
upload failed silently. (Unverified from the CLI — repo secret scoping isn't readable; harmless if
the token turns out to be repo-level.)

omop-db
7. load_core_vocab.sh swallowed a failed guard. A command substitution inside an if condition
is exempt from set -e, so a failed psql probe yielded "", compared unequal to "t" and fell
through to the COPY — silently duplicating rows in the four tables with no primary key. Now a
plain assignment plus a case that treats anything but t/f as fatal.
8. A degenerate NUM_TRUSTS (0, 00, empty, non-numeric) made populate /apply-constraints /
export-pgdata fan out to zero targets and exit 0 — populate would fetch the bundles and load
nothing. Guarded.
9. constraints.sql guard 18 reused guard 17's sentinel, so PAYER_PLAN_PERIOD's 10 FKs were
never applied on any run. Impact is ~nil (the table is unused), but the DO/IF NOT EXISTS
wrapper is a local addition — upstream OHDSI ships a flat list of plain ALTER TABLE
statements — so the typo is ours.

Docs — 10/11: the README claimed the vocabulary bundle was needed "not for running the trust
stacks", contradicting its own mandatory make load-omop-vocab step; gave those commands an explicit
-C; corrected a workflow comment still calling the vocabulary "baked".

Verified on the branch: omop-db ruff + mypy + 47 tests; bash -n; the NUM_TRUSTS guard across
9 edge cases; ansible-playbook --syntax-check + both tag selections; helm lint + every
helm-template gate, including proof the rewritten gates now fail when the Job is wrongly present
and when the render is empty; prettier on the gated AWS scope; full pre-commit on the diff.

Filed separately (all develop-side, none introduced here): #879 (cohort schema-allowlist
bypassable by dropping the qualifier — the one security finding, demonstrated), #880, #881, #882, #883.

Acceptance Criteria

Imported from issue #834

• flip-omop-db source (OMOP CDM 5.4 DDL/PK/index/constraint SQL, readonly-role SQL, Dockerfile, populate tooling) lives under trust/omop-db/, snapshot-imported with FLIP conventions applied (Apache headers, ruff/mypy config, uv 72h cooldown, Makefile target contract, CLAUDE.md/AGENTS.md pair, uv-lock pre-commit entry, UV_PROJECTS)
• Third-party attribution added (OHDSI CommonDataModel, DICOM2OMOP) and internal S3/private-repo references scrubbed
• Mock CSVs published to the public HF dataset as one canonical set; populate tooling fetches it and deterministically partitions by person_id across N trusts, preserving referential integrity
• Trust compose files use ${OMOP_DB_TAG} instead of hardcoded :latest; "built from" comments point at the in-repo path
• Vocabularies remain outside git; manual image build documented (vocab licensing)
• Docs updated: component-omop-database.rst expanded, stale radiology_occurrence references reconciled to image_occurrence, data-access-api cross-references point in-repo
• Unit tests cover the split tooling; pre-commit and service lint/mypy/tests green

[github-actions]

✅ Acceptance criteria have been automatically imported from the linked issue(s) and added to the PR description.

[github-actions]

✅ Acceptance criteria have been automatically imported from the linked issue(s) and added to the PR description.

[github-actions]

✅ Acceptance criteria have been automatically imported from the linked issue(s) and added to the PR description.

[garciadias]

Thorough migration - all seven acceptance-criteria items from #834 verified satisfied in substance, 37+ new unit tests, and a real reduction in what gets published (image 4.3GB→450MB, pgdata tarballs 2.2GB→11MB). Left one comment on a concrete gap in the new k8s vocab-load Job's local/kind-cluster credential path, which the PR body already flags as not yet run to completion with real credentials - this confirms that caveat with a specific fix.

Two minor items, not blocking on their own:

• deploy/providers/AWS/Makefile:488 passes -e omop_postgres_password=... as a plaintext ansible-playbook CLI argument - the only secret-shaped -e in this file; every other credential is handled via env/unset rather than argv, which is visible to other local users via ps aux for the process lifetime. Worth passing it via environment: in the task or a cleaned-up temp extra-vars file instead.
• trust/omop-db/files/OMOPCDM_postgresql_5.4_ddl.sql, _indices.sql, _primary_keys.sql, _constraints.sql all open with a copyright header in a different format from the rest of the tree (missing "(c)", missing "& King's College London", dated 2024 not 2026) - cosmetic, but worth aligning with the template the rest of this PR uses.

[codecov]

Codecov Report

❌ Patch coverage is 70.16129% with 74 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...rust/omop-db/src/omop_db_tools/load_dicom_vocab.py	55.00%	36 Missing ⚠️
trust/omop-db/src/omop_db_tools/import_tables.py	63.15%	21 Missing ⚠️
trust/omop-db/src/omop_db_tools/dataset.py	84.21%	15 Missing ⚠️
trust/omop-db/src/omop_db_tools/config.py	87.50%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[garciadias]

Thorough, well-tested vendoring PR. I rebuilt the image from scratch, ran the full init chain (schema → PKs → indices → readonly-role creation), confirmed the read-only role can SELECT but not INSERT (defence-in-depth against the cohort-query docstring claims), ran the new vocab loader twice against a synthetic bundle to confirm its idempotency guards actually work, ran the full unit suite (47 passing, ruff/mypy clean), and rendered the Helm chart both with and without vocab-load enabled. Everything the PR claims held up under independent verification, and all CI checks are green.

One real gap worth a follow-up (doesn't need to block this merge): the dev/trust kit-file examples (trust/.env.example, trust/.env.GSTT.development.example, trust/.env.KCH.development.example) still default OMOP_DB_TAG=latest. Before this PR that was fine (omop-db had no CI publishing, so :latest just meant "whatever was last manually pushed"), but this PR adds the same :stag-on-develop/:prod-on-main CI tagging every other repo-built service uses — and dev environments elsewhere in the project track :stag. Left unchanged, freshly-scaffolded dev/trust stacks will now silently pull the prod-pinned image instead of tracking develop.

Left three minor inline comments (stale copyright header on the imported CDM SQL files, the vocab-load Helm default failing without org S3 credentials, and an Ansible cleanup task that isn't wrapped to run on failure). None of these block. Approving.

[atriaybagur]

Thanks for the thorough pass — replies are in-thread on the three inline comments. This is for the fourth item from your review body, which has no thread to reply in: the OMOP_DB_TAG=latest defaults in the kit examples. Fixed in 7492916 along with the rest.

You're right, and it's sharper than "fine before": this PR is what makes it wrong. docker_build_omop_db.yml publishes :prod and :latest from main, so latest now is the production image — a dev kit was running omop-db from prod while its four siblings ran :stag.

Rather than swapping the literal to stag, the composes now resolve ${OMOP_DB_TAG:-${DOCKER_TAG}} and the examples comment the variable out. That keeps a per-service override for the case trust/xnat/Makefile's XNAT_TAG exists for — omop-db changes rarely, so it's precisely the service whose branch tag will be absent when the rest of the stack runs a feature-branch DOCKER_TAG (branch builds are workflow_dispatch-only). A bare ${DOCKER_TAG} would turn those deploys into a manifest unknown on omop-db. No trust/Makefile export was needed: the kit already reaches compose via --env-file (trust/Makefile:90) and DOCKER_TAG is exported for dev at :55-60 — adding a make-level default would in fact clobber a kit's own value, since shell env beats --env-file.

Nothing in the wild changes. OMOP_DB_TAG isn't in HUB_SHARED_KEYS, so sync-trust-kit never strips it — existing kits keep their literal OMOP_DB_TAG=latest line and resolve exactly as they do today. Only kits newly scaffolded by make new-trust pick up the new default.

I swept the same :latest default out of the two other places it had spread to, on your reasoning:

• Helm omopDb.image.tag → stag, matching all eight siblings in values.yaml. This one had become a correctness issue rather than cosmetics: the new omop-vocab-load Job execs /flip/omop/load_core_vocab.sh from the image.
• Ansible omop_db_tag | default('latest') — nothing ever set it, so it always resolved latest. make seed-trust-data now passes -e omop_db_tag=$(DOCKER_TAG), exactly as it already does for the bucket, with the fallback moved to stag.

One thing worth flagging that fell out of checking this. The published omop-db tags today are latest, trust1, trust2, stag — no :prod, no sha-*. CI has never published this image; every tag was hand-pushed via make push, and they all predate the vocab-free rework, so they still bake the licensed bundle (COPY ./data/vocab_aicentre_core_20240916 /flip/omop/vocab) and none of them contains load_core_vocab.sh. That's the exposure #842 already tracks, but it means a one-time gh workflow run docker_build_omop_db.yml --ref develop is needed right after merge — the workflow_run gate doesn't fire retroactively on the merge that introduces it. I've added that as item 4 under post-merge admin actions in the PR description.

[atriaybagur]

Ready for another look. CI is green (33 pass, 1 skip) and every review thread is resolved.

Your approval was dismissed by two pushes since, so for the delta:

• 8323dba7 — merge of develop.
• f64446ea — eleven defects found by a full-diff self-review, each reproduced locally before being accepted (four were downgraded from their initial severity after re-checking). The two that would have bitten in deployment: make ansible-init (and so full-deploy) failed outright because the new vocab-load play asserts omop_postgres_password, which only seed-trust-data passes — it now skips there via the omop-vocab tag it already carries, with the vocabulary still seeded later by seed-trust-data; and the always: cleanup removed the container before deleting the bundle, so a failing task against an unreachable daemon left ~3.6 GB of licensed vocabulary at rest on the host — deletion now runs first.

Nothing else has changed since you approved.