[Feature]: Support connecting FLIP XNAT to a real Trust PACS

[atriaybagur]

Closes #993.

FLIP could only ever talk to the mocked Orthanc. The PACS host, AE title and
query/retrieve port were literals, XNAT's own AE title was hardcoded in the three
places that have to agree, and on Kubernetes the chart blocked the inbound
C-STORE by design. This makes the whole connection configurable, defaulting to
today's mocked behaviour.

First target: a fresh single-node Kubernetes deployment at GSTT, NodePort, DICOM on 8104.

What changed

Configuration. PACS_HOST / PACS_AETITLE / PACS_QR_PORT / PACS_LABEL,
PACS_SUPPORTS_EXTENDED_NEGOTIATIONS and XNAT_AETITLE, all joining the existing fail-loud guard
block. The DQR retry settings and the PACS availability schedule are exposed, because a production
PACS may refuse further associations after a certain volume.

One PACS per XNAT. A trust XNAT retrieves from exactly one PACS, so the script owns the
registration list rather than tolerating several: the configured PACS is created or updated in
place, and any other registration is removed, logged per entry. That closes two orphan bugs —
changing XNAT_AETITLE used to strand the old SCP receiver on the same port, and changing
PACS_AETITLE stranded the old PACS registration, both of them claiming to be the default.
Receiver reclamation is scoped to what this script owns — the dqrObjectIdentifier it stamps on
its own receiver, plus XNAT's stock XNAT one — rather than to a port or a title, so a rename no
longer strands the old entry and a receiver an operator added for another local DICOM source is
left alone. imaging-api is
correspondingly simpler: PACS_AETITLE is gone from its settings and both compose files, and it
resolves "the registered PACS" rather than matching a title — still not assuming id 1, since XNAT
numbers registrations in creation order. trust-api's health probe no longer pins id 1 either: it
calls /imaging/ping_pacs with no id and lets imaging-api resolve it.

The port split. XNAT_PORT was doing four jobs. It is now the DICOM SCP receiver port
only; XNAT_WEB_PORT is the host-published web UI. That is why host 8104 served Tomcat while
the DICOM receiver's 8104 was an unpublished container port — a real PACS could not have
completed the C-STORE leg at all. The base compose now publishes the receiver unconditionally
(same number both sides of the mapping — DQR matches the C-MOVE destination by exact AE:port,
so no translation is possible on that leg), so a development deployment runs the same wiring a
real-PACS trust relies on. An earlier revision made this opt-in via a REAL_PACS=true overlay;
that overlay is retired for dev/prod parity. Both ports being host-published means every kit must
give them distinct values — the deploy guard refuses a collision, which is also what catches a
pre-split kit that still sets only XNAT_PORT. XNAT_PORT keeps the canonical 8104 (the number
the PACS dials, the firewall rule names, and the Helm chart pins); the web UI moves off it — dev
allocation 8104/8105 (GSTT), 8106/8107 (KCH) — and the dev tooling that dialled the web UI (demo
recorder, seg uploader, EC2 status probe/port-forward helper) follows it.

Kubernetes. Three copies of the XNAT configuration existed and had drifted — the script in
the image, ~390 lines inlined in the init Job, and a third in a ConfigMap. The inlined copy
never read the chart's own orthanc.dicomHost / dicomPort / dicomAet values, which is why
orthanc.enabled: false with an external host never actually redirected DQR. The init
container now runs the same configure-xnat.sh; the other two copies and the dead values are
deleted. xnat.web.dicomNodePort pins the NodePort and
networkPolicies.allowedIngressCIDRsWithPorts allows the return leg from the PACS CIDRs only.

Security posture. Retrieval is a pull, so the PACS opens a new association back to XNAT.
That is one inbound path into a trust. security.rst and governance-and-compliance.rst
previously claimed no inbound connections at all; they now state it precisely — nothing from
the internet or the hub, one path from the trust's own PACS, scoped to that PACS on the DICOM
port, and only where FLIP is connected to a real PACS. AWS-hosted trusts are mock trusts running
FLIP's bundled Orthanc over the container network, so they have no such path and their security
groups keep no ingress rules at all — true by design rather than by accident. Wording signed off.

Docs. New "Connecting to a Trust PACS" section covering the pull model, the inbound return
leg, the three firewalls, a worked Sectra example, and the mocked Orthanc used in development.
The XNAT credentials email figure is regenerated from the current SES template; the previous
one predated the rebrand.

Verification

• configure-xnat.sh run against a stub curl: with defaults the emitted payloads are
  byte-identical to the previous script, apart from the PACS id now being resolved.
• trust/xnat: 229 passed. test_configure_pacs.py asserts on the payloads XNAT would receive,
  against a stub that keeps the /xapi/pacs and /xapi/dicomscp collections in files so a GET
  reflects what the script actually did. It found a real bug: ${VAR:-default} silently
  substituted the default for an explicitly empty value, so PACS_HOST= in a kit file would have
  fallen back to the mocked PACS instead of failing. Now ${VAR-default}.
• Mutation-tested: 45 single-token changes to configure-xnat.sh, all 45 caught. The first run
  caught 20 — see the review round below.
• test_deploy_wiring.py covers the layer the script cannot see: the Makefile's variable
  resolution and port guards, and both deployment files' environment blocks. Every bug found while
  deploying this change lived there.
• trust/imaging-api: 287 passed. trust/trust-api: 112 passed. ruff and mypy clean.
• helm lint clean; default, flower, external-override, real-PACS and vocab-load all render.
  CI gains a real-PACS render asserting the NodePort and ingress policy, plus a companion
  asserting the default renders neither, and a third asserting a PACS configured with no egress
  rule is refused.
• Sphinx builds clean.

Verified on a dev deployment

One Compose trust and one Kubernetes trust, per the review ask.

Compose (KCH, slot 2). XNAT came up from this branch. With defaults it reproduced current
behaviour exactly — PACS ORTHANC@orthanc:4242 (id 1), SCP receiver XNAT on 8106 (the
trust's own port, not a hardcoded 8104), stock receiver removed, anonymisation on. Re-running
configuration with real-PACS values then replaced the receiver with FLIPXNAT, moved
dqrCallingAe with it, applied the throttle (25 attempts / 120s), and updated the PACS
registration in place — id stayed 1, host and port changed, nothing duplicated.

Kubernetes. Installed into an isolated namespace so the existing release was untouched. The
unified init job — now running configure-xnat.sh from the xnat-web image — succeeded, and
XNAT ended up correctly configured (PACS id 1, receiver XNAT:8104 with dqrObjectIdentifier,
DQR calling AE XNAT, allowAllUsersToUseDqr still false). curl and jq are both present in
that image, which was the dependency flagged as unverified.

Deploying found three defects the tests had not, fixed in eddb4117:

1. XNAT_WEB_PORT defaulted to a literal 8104, so any trust whose XNAT is not on 8104 would
   have had its web UI moved there — colliding with an already-running stack on a multi-trust
   host. KCH is on 8106 and failed to come up next to GSTT. Now defaults to XNAT_PORT.
2. docker stack deploy rejects nested variable defaults, so ${XNAT_WEB_PORT:-${XNAT_PORT}}
   failed the deploy outright.
3. XNAT_AETITLE exported empty from the Makefile.

One pre-existing limitation confirmed and now documented: DQR pre-creates the availability
intervals and rejects later writes to a day that already has one, including after a
re-registration, and it normalises 24:00 to 00:00. The configured window is therefore not
reliably in force, so the docs tell operators to verify what XNAT holds rather than assume. Making
it actually apply is separate work.

End-to-end smoke

make e2e_smoke FL_BACKEND=flower --trusts KCH, against the KCH trust running this branch.

The imaging path — the part this PR changes — passed end to end:

⏳ Waiting for image pull (>=80% across ["King's College Hospital"], timeout 900s)
  King's College Hospital: 0/300 (0%)
  King's College Hospital: 86/300 (29%)
  King's College Hospital: 205/300 (68%)
  King's College Hospital: 300/300 (100%)
  image pull threshold reached

300 studies resolved from a real cohort query and pulled into XNAT through the modified
imaging-api — resolve_pacs_id for the C-FIND, the settings-driven AE title as the C-MOVE
destination — and the reconfigured SCP receiver. Cohort query, model upload and file scanning all
passed on the way.

FL training did not run, for reasons unrelated to this branch. The job was submitted and
accepted (Submitted Flower job ... 200 OK), but the client had already joined a pre-existing run
that fails with 403 on /cohort/dataframe, and never picked up a slot for the new one. This PR
touches nothing in the FL path.

Two environment gaps found on the way, both pre-existing:

• KCH's OMOP had no licensed vocabulary — 8,814 concepts against GSTT's 6.1 million — so the cohort
  query returned nothing. This is the documented silent failure: every health check passes and the
  only symptom is an empty cohort.
• A fresh worktree defaults to FL_BACKEND=nvflare while the running stack is on Flower, so the
  clients crash on missing NVFLARE startup files until the backend is set to match.

Out of scope, tracked separately: DICOM TLS; multiple studies per accession and the unused
StudyQuery.modality filter; the redeploy that rm -rfs XNAT_DATA_DIR; PS3.15 profile
declaration and (0012,0064) coded values.

Review round

A full multi-agent review of the branch, then the findings fixed in three commits.

Five silent-failure bugs (426180ff), each with a regression test reproducing the reviewer's
case: the Kubernetes imaging-api never received XNAT_AETITLE; compose's ${VAR:-default}
cancelled the fail-loud guard the PR had just added — and the test I had cited as proof of that fix
bypassed compose entirely; operator values were spliced into JSON as text, so a crafted PACS_HOST
produced valid JSON whose injected defaultQueryRetrievePacs: false won; receiver reclamation
orphaned the old entry when port and title both changed; and helm upgrade could delete a
hand-registered real PACS.

Networking and honesty in the docs (fa870150). externalTrafficPolicy: Local, since under
the default Cluster policy kube-proxy rewrites the PACS source address before the pod sees it and
the ingress CIDR would never have matched. A NetworkPolicy ingress entry with no port now fails
the render rather than silently widening the one inbound path from DICOM to everything. A
configured PACS with no egress rule fails too — which correctly broke this PR's own CI reference
configuration, which had been rendering an unreachable PACS and asserting it was right.
resolve_pacs_id gained a timeout, selects on defaultQueryRetrievePacs rather than list order,
never caches a fallback, and clears its cache when XNAT reports the PACS missing. Four
documentation claims were simply wrong and are corrected, including the firewall table in
admin-platform-support.rst whose DICOM row had an empty Inbound column — the table an
operator uses to raise the request.

Reclamation scope and the return leg (764f4c0a). Receiver reclamation deleted every SCP
receiver XNAT reported, so an operator's second receiver for another local DICOM source was
collateral damage of every redeploy and helm upgrade — visible only in a Job pod log the
hook-delete-policy discards on success. It is now scoped to FLIP's own dqrObjectIdentifier plus
XNAT's stock receiver (GET /xapi/dicomscp does return identifier per receiver — verified against
a live XNAT). A foreign receiver squatting the configured port survives to the POST, which fails
loud, rather than being deleted quietly. On the chart side, a real PACS with no egress rule
already failed the render while the missing inbound return leg rendered clean, which is precisely
the queries-succeed-retrievals-time-out failure this PR keeps calling the hardest to diagnose; a
matching ingress guard now requires an allowedIngressCIDRsWithPorts entry on xnat.web.dicomPort,
with CI asserting both the omission and the near miss of listing dicomNodePort instead of the
container port.

Test gaps (6ea694f5). Mutation testing found that every security-relevant field in the
payloads could be flipped with the suite still green: allowAllUsersToUseDqr to true, site
anonymization uploaded but switched off, the guest account left enabled, the SCP identifier moved
off dqrObjectIdentifier. So could the settings that decide whether retrieval works at all. The
stub was part of the problem — it echoed the configured AE title back, so "the configured title
reached XNAT" held even when the script sent something else. Rebuilt as a stateful stub, nineteen
tests added, all 45 mutants now die, and the suite went from 121s to under 2s. Also in that commit:
PACS_SUPPORTS_EXTENDED_NEGOTIATIONS (documented as configurable, hardcoded true), trust-api's
pinned PACS id, check_local_status.py probing the web UI on what is now the DICOM port, and its
runner that enumerated tests by hand so a new one ran nowhere while the suite reported green.

Acceptance Criteria

Imported from issue #993

• A trust can point XNAT at a real PACS purely through configuration — PACS host, AE title and
  Q/R port — with no code change.
• XNAT's own AE title is configurable, and is applied consistently to the DICOM SCP receiver,
  dqrCallingAe, and the C-MOVE destination in the import request.
• The DICOM port is a single value end to end: bound by XNAT, registered in dicomscp,
  advertised as the C-MOVE destination, and published/NodePort-exposed. No translation at any
  layer.
• On Kubernetes, the XNAT DICOM SCP is reachable from a configured PACS CIDR — Service exposure
  and a NetworkPolicy ingress allowance — with default-deny preserved when unconfigured.
• Re-running configuration after changing PACS host/port updates the existing registration
  rather than silently skipping it.
• The PACS throttle is configurable: availability window, thread count, retry attempts and retry
  wait.
• make e2e_smoke passes unchanged against the mocked Orthanc, on both FL backends.

On the three left unticked:

• NodePort reachability is implemented and asserted at render time (Service exposure,
  externalTrafficPolicy: Local, NetworkPolicy ingress scoped to the PACS CIDR on the DICOM port,
  default-deny preserved when unconfigured, and a render that is refused if a PACS is configured
  with no egress rule). It has not been proven against a PACS outside the cluster, because there
  isn't one to test against yet — GSTT is the first.
• The throttle is configurable and the retry attempts / retry wait apply. The availability
  window does not reliably take effect: DQR pre-creates the intervals at registration and rejects
  later writes to a day that already has one, and XNAT normalises 24:00 to 00:00. Documented as
  a caveat; making it apply is separate work.
• make e2e_smoke on both backends has not had a clean start-to-finish run. The imaging path this
  PR changes passed (300/300 studies pulled through the modified imaging-api and the reconfigured
  SCP receiver); the runs that failed afterwards did so in the FL path, for reasons unrelated to
  this branch.

[github-actions]

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[atriaybagur]

Ready for review. CI green (33/34, the skip is the develop-target guard), mergeable, no unresolved threads. Full multi-agent review completed with all critical and important findings fixed and regression-tested; configure-xnat.sh is mutation-tested (45/45 mutants die) and the deployment wiring around it has its own suite. Verified on a dev deployment (Compose + Kubernetes trusts, 300/300 studies pulled through the modified imaging path) and the mocked-Orthanc defaults are byte-identical to pre-PR behaviour. Security-posture wording in security.rst / governance-and-compliance.rst is signed off. Copilot review requested. 4/7 acceptance criteria ticked — the PR body explains the honest state of the other three (NodePort unproven against a real external PACS until GSTT, the availability window caveat, and e2e smoke).

[Copilot]

Pull request overview

Adds full configurability and deployment support for connecting a trust XNAT (via DQR) to a real upstream PACS (AE title/host/ports, retry/throttle controls), while keeping the default behavior pointing at the bundled mocked Orthanc. This spans trust-side runtime configuration, Kubernetes Helm chart wiring + network policy exposure for the inbound C-STORE return leg, imaging-api runtime PACS-id resolution, and accompanying documentation/tests.

Changes:

• Make XNAT PACS/DQR configuration fully environment-driven (including XNAT AE title, PACS registration ownership/update-in-place, retries, and availability/throttle knobs) and add a Compose overlay to publish the DICOM receiver only when needed (REAL_PACS=true).
• Update imaging-api to use settings-driven XNAT AE title for C-MOVE destinations and to resolve the PACS id dynamically from XNAT (with caching and fallback), plus adjust trust-api health probing to stop pinning a PACS id.
• Extend Helm chart values/templates to support real-PACS connectivity (NodePort pinning + scoped ingress NetworkPolicy + required egress validation), and add/refresh docs and CI renders/tests to prevent drift.

Reviewed changes

Copilot reviewed 46 out of 47 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
trust/xnat/xnat/config/configure-xnat.sh	Parameterizes XNAT AE title + PACS registration + DQR settings; switches payload building to jq for injection safety; updates idempotency/ownership logic.
trust/xnat/tests/test_deploy_wiring.py	Adds tests to ensure Make/Compose/Helm wiring correctly passes every script knob and enforces port guards.
trust/xnat/tests/test_configure_pacs.py	Adds execution-style tests running the script against a stateful curl stub to validate payloads and safety properties.
trust/xnat/Makefile	Splits XNAT_PORT vs XNAT_WEB_PORT, adds REAL_PACS overlay handling, and adds port collision/numeric guards.
trust/xnat/docker-compose-stack.yml	Publishes only the web UI port by default; passes new PACS/XNAT env vars into xnat-web.
trust/xnat/docker-compose-stack.real-pacs.yml	New opt-in overlay to publish the DICOM SCP receiver port on the host for real PACS integrations.
trust/trust-api/trust_api/services/health_collector.py	Updates DICOM health probe to call imaging-api ping without a pinned PACS id.
trust/trust-api/trust_api/config.py	Removes PACS_ID from trust-api settings.
trust/trust-api/tests/services/test_health_collector.py	Updates assertions for the ping route change (no trailing PACS id).
trust/trust-api/README.md	Removes PACS_ID from documented env vars.
trust/imaging-api/tests/services/test_imaging.py	Adds tests for PACS-id resolution behavior, caching, fallbacks, and AE-title configuration.
trust/imaging-api/tests/routers/test_imaging.py	Adds router tests for /imaging/ping_pacs (with and without explicit id).
trust/imaging-api/imaging_api/services/imaging.py	Implements PACS-id resolution with caching/fallback and uses it in query/import paths; adds cache invalidation helper.
trust/imaging-api/imaging_api/routers/schemas.py	Makes DQR import request AE title follow settings (XNAT_AETITLE) instead of a literal.
trust/imaging-api/imaging_api/routers/imaging.py	Adds /imaging/ping_pacs (no-id) route and resolves PACS id when omitted.
trust/imaging-api/imaging_api/config.py	Adds XNAT_AETITLE setting and reframes PACS_ID as an unreachable-XNAT fallback only.
trust/deploy/compose_trust.production.yml	Threads XNAT_AETITLE into Orthanc modality config and imaging-api env.
trust/deploy/compose_trust.development.yml	Threads XNAT_AETITLE into Orthanc modality config and imaging-api env.
trust/CLAUDE.md	Updates trust service port notes to reflect the XNAT_PORT/XNAT_WEB_PORT split.
trust/AGENTS.md	Mirrors the trust/CLAUDE.md update per the repo’s CLAUDE↔AGENTS convention.
trust/.env.KCH.development.example	Adds XNAT_WEB_PORT and PACS-related configuration defaults/documentation.
trust/.env.GSTT.development.example	Adds XNAT_WEB_PORT and PACS-related configuration defaults/documentation.
trust/.env.example	Adds XNAT_WEB_PORT and PACS-related configuration defaults/documentation.
scripts/tests/test_check_local_status.py	Updates local status checks to probe XNAT web UI via XNAT_WEB_PORT and improves test discovery.
scripts/check_local_status.py	Uses XNAT_WEB_PORT (fallback to XNAT_PORT) when probing XNAT web UI locally.
docs/source/sys-admin/admin-platform-support.rst	Corrects inbound/outbound firewall table to reflect the PACS C-STORE return leg when applicable.
docs/source/security.rst	Refines network posture language: no inbound from internet/hub; PACS return leg inbound is the exception.
docs/source/governance-and-compliance.rst	Updates governance narrative to acknowledge the internal PACS→XNAT inbound return leg when configured.
docs/source/glossary.rst	Adds glossary entries for AE Title, SCU/SCP, DIMSE, C-FIND/C-MOVE/C-STORE, and DQR.
docs/source/deploy-flip/deploy-flip-node-on-prem.rst	Clarifies on-prem networking: no inbound from internet; PACS return leg is the exception.
docs/source/components/component-xnat.rst	Updates XNAT component docs to reference PACS retrieval behavior and link to PACS page.
docs/source/components/component-pacs.rst	New detailed documentation for PACS connectivity model, required settings, firewall rules, and verification.
docs/source/components.rst	Adds the new PACS component doc to the components index.
deploy/providers/kubernetes/values.yaml	Adds PACS configuration values and NodePort/NetworkPolicy knobs for real PACS connectivity.
deploy/providers/kubernetes/TROUBLESHOOTING.md	Updates PACS/XNAT port-map troubleshooting guidance to match new values and semantics.
deploy/providers/kubernetes/templates/xnat-web.yaml	Fixes XNAT_PORT meaning (DICOM), adds NodePort pin + externalTrafficPolicy: Local behavior when configured.
deploy/providers/kubernetes/templates/orthanc.yaml	Makes Orthanc modality config follow xnat.web.dicomAet/dicomPort values.
deploy/providers/kubernetes/templates/network-policy.yaml	Adds optional ingress policy for PACS CIDR+port and adds fail-fast validation for missing PACS egress.
deploy/providers/kubernetes/templates/imaging-api.yaml	Sources imaging-api XNAT_PORT and XNAT_AETITLE from the same xnat.web.* values used by the init job.
deploy/providers/kubernetes/README.md	Documents the opt-in NodePort exception for exposing the DICOM receiver.
deploy/providers/kubernetes/NETWORK-POLICY.md	Documents the new inbound PACS exception and scoping guidance.
CLAUDE.md	Documents the new XNAT/PACS-related environment variables and their semantics.
AGENTS.md	Mirrors the CLAUDE.md update per the repo’s CLAUDE↔AGENTS convention.
.github/workflows/test_trust_xnat.yml	Expands path filters so wiring-sensitive files trigger the trust/xnat CI suite.
.github/workflows/test_helm_chart.yml	Adds Helm render assertions for real-PACS configuration and guards default-deny behavior.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[garciadias]

Re: trust/deploy/compose_trust.development.yml (line 61) — flagging at PR level because this line is unchanged context just outside the diff hunks.

This commented-out mapping still references the retired PACS_DICOM_PORT variable, which no longer exists anywhere in the deploy path. It will mislead anyone grepping for the variable or copying the line into an active config. Same in compose_trust.production.yml.

Suggested change: remove the commented # - "${PACS_DICOM_PORT}:4242" lines; the Orthanc DICOM port is intentionally unpublished and the new component-pacs.rst already explains that.

[garciadias]

Very strong PR overall with excellent test coverage, but one destructive behaviour blocks merge: configure-xnat.sh now deletes every SCP receiver on each redeploy/upgrade (previously only aeTitle==XNAT), removing operator-added receivers. Please scope the deletion to FLIP-owned receivers or add a refuse guard.

[atriaybagur]

On the PR-level note about the commented-out ${PACS_DICOM_PORT} mappings — removed in 764f4c0.

Both were dead disabled config referring to a variable this PR retires, so they can only mislead: trust/deploy/compose_trust.development.yml:61 and compose_trust.production.yml:53. The surviving prose mentions of PACS_DICOM_PORT are deliberate and stay — configure-xnat.sh:118 and :408 explain why the variable was retired (it meant the host-published port, not the port XNAT must dial, which is how FLIP#822/#862 stayed hidden), and CLAUDE.md/AGENTS.md make the same point. Deleting those would lose the rationale for the retirement.

In passing: trust/xnat/Makefile:71 used ${PACS_DICOM_PORT} as an illustrative variable in a rationale comment about docker stack deploy having no --env-file. Swapped for ${PACS_UI_PORT}, which is still live in those composes, so the example stays true.

[garciadias]

Overall this is well-architected and unusually well-verified for infra work that can't be exercised against a real PACS in CI -- the fail-loud ${VAR-default} contract, the mutation-tested configure-xnat.sh rewrite (45/45 killed per the PR body), and the clean removal of AWS-credential handling from the Kubernetes chart are all solid. Left 5 inline comments: 2 Major (the new XNAT_AETITLE env var in both compose_trust.{development,production}.yml uses the silently-defaulting ${VAR:-default} form, which is exactly the pattern this PR's own fail-loud contract was written to eliminate elsewhere) and 3 Minor (the new wiring test doesn't cover those two compose files, an unguarded cache race in resolve_pacs_id, and a Helm NodePort/NetworkPolicy combination gap that can pass render-time validation while leaving no actual network path to the PACS).

Two clarifying questions, non-blocking: was the colon-dash XNAT_AETITLE a deliberate exception or missed when the bare-dash fix was applied elsewhere? And is the NodePort/NetworkPolicy gap an accepted risk folded into the already-disclosed "not yet tested against a PACS outside the cluster" limitation?

[garciadias]

Requesting changes: the PACS drift check compares only host and port, so label and supportsExtendedNegotiations changes are silently dropped on re-apply. See inline comment.

[atriaybagur]

The 2026-08-27 review point is addressed in 277ec6b — the in-place PACS update now compares every kit-managed field (host, queryRetrievePort, label, supportsExtendedNegotiations), with a regression test pinning the flag-only-drift case; details in the thread reply. The branch had also gone CONFLICTING against develop, so develop is merged in (3eab64b: trust/xnat Makefile instance-scoping + XNAT port-split comments combined, test_trust_xnat.yml comment blocks combined, check_local_status test runner kept on auto-discovery which picks up the three new develop tests) and CI is running again on the head.

[garciadias]

This uses the colon-dash form ${XNAT_AETITLE:-XNAT} in Orthanc's DICOM_MODALITIES AE title. Per this PR's own fail-loud contract (enforced elsewhere via test_compose_defaults_do_not_cancel_the_scripts_empty_check), an operator who sets XNAT_AETITLE= (empty) in a kit file should get a loud failure, not a silent fallback to "XNAT". The colon form substitutes the default on an explicitly-empty value, cancelling that guard here even though configure-xnat.sh itself (via trust/xnat/docker-compose-stack.yml's bare-dash ${XNAT_AETITLE-XNAT}) still fails loud. The wiring test only reads trust/xnat/docker-compose-stack.yml, so this occurrence isn't covered.

[garciadias]

Same issue as line 74: XNAT_AETITLE: ${XNAT_AETITLE:-XNAT} passed to imaging-api uses the colon-dash form, silently defaulting to "XNAT" on an explicitly-empty kit value instead of tripping the fail-loud guard. This becomes the C-MOVE destination AE title imaging-api hands to the PACS -- if it silently diverges from what's actually registered on XNAT (e.g. because the XNAT-stack deploy failed loud on the same empty value and kept an old AE title), retrieval fails silently exactly as this PR elsewhere prevents.

[garciadias]

Same colon-dash issue as compose_trust.development.yml:74 -- Orthanc's DICOM_MODALITIES AE title falls back to "XNAT" on an explicitly-empty XNAT_AETITLE, cancelling the fail-loud contract this PR established elsewhere for exactly this class of operator-configurable knob.

[garciadias]

Same colon-dash issue as compose_trust.development.yml:110 -- imaging-api's XNAT_AETITLE env var silently falls back to "XNAT" on an explicitly-empty value rather than failing loud, which can desync the C-MOVE destination AE title from whatever XNAT is actually configured with.

[garciadias]

This is an exceptionally well-engineered and well-tested PR (mutation-tested shell script, dedicated deploy-wiring tests, real Compose + Kubernetes deployment verification, five internal review rounds that already closed the obvious silent-failure bugs). But it left one instance of the exact bug class it set out to eliminate unfixed: trust/deploy/compose_trust.{development,production}.yml still use ${XNAT_AETITLE:-XNAT} (colon-dash) for Orthanc's mock modality config and imaging-api's AE title, which silently cancels the fail-loud guard on an explicitly-empty kit value -- and this pair of files falls outside the scope of the new deploy-wiring test suite, so it wasn't caught. Four inline comments detail each occurrence; the fix is a small, mechanical swap to bare-dash plus a test extension.