Spleen data-enrichment (labels) depends on a private repo — can't run the spleen e2e outside the org or in CI

[atriaybagur]

Description

Data enrichment — adding the labels FLIP cannot pull from PACS — was documented only in the abstract and tooled only in a private repo (londonaicentre/flip_project_spleen_segmentation, itself depending on the private, unlicensed londonaicentre/xnat_utils). This ships the tooling in-tree and documents the stage end to end.

No private repo or private package is required any more, and no data artefact is committed to do it.

What's here

flip.xnat — new subpackage in flip-utils

• XnatClient built on requests (zero new dependencies — requests is already a core dep), mirroring the REST shapes imaging-api already uses. Resolves the XNAT project by secondary_ID, lists scans, uploads scan resource files.
• upload_enrichment_files() — manifest-driven (accession_id,file_path[,target_filename]) and project-agnostic. It derives each target filename from the image already in the scan's resource (input_ → label_), which is the pairing the apps rely on, rather than guessing it. Skip-if-exists by default; refuses equal rename prefixes, which would overwrite the image.
• A flip-xnat console script, also runnable as python -m flip.xnat.
• Runs on the model developer's workstation inside the Trust network — never in the FL client, which holds no XNAT credentials. Kept import-light (no pyxnat) since flip ships inside every FL image.

Spleen tutorial — the private-repo replacement

• …/3d_spleen_segmentation/utils/upload_spleen_labels_to_xnat.py, alongside the existing spleen data helpers.
• The accession→MSD-case mapping is fetched at run time from the public aicentreflip/trust-data OMOP export, not committed. Verified it reproduces the private tables/dicom_metadata.csv join exactly (41/41 rows, identical case for every accession) and additionally carries source_trust — 21 trust1 / 20 trust2, matching the published Orthanc volumes with zero misplacements — so each label is uploaded only to the Trust that actually holds that study, rather than attempting both.
• upload-spleen-labels target in both backend Makefiles. Enrichment is a one-time, backend-agnostic step (the labels live in XNAT; both backends read them), so Flower delegates to the single copy of the script with its own --labels-dir rather than duplicating it.
• flip-api's e2e_smoke_spleen / e2e_smoke_spleen_evaluation targets and the demo-video docs repoint at it.

Fail loudly

The five spleen train/eval entry points now raise No image/label pairs found: N image(s) …, none with a matching label_*.nii.gz naming the likely cause, instead of letting an empty dataset surface as torch's opaque num_samples=0 after a full pull cycle.

Documentation

• New user guide: docs/source/user-guides/user-data-enrichment.rst, covering both the manual XNAT-UI route and the scripted one, the ordering constraint, the placement contract, and troubleshooting.
• It leads with which route a label should take: labels already in OMOP (a lab result, a coded report finding) belong in the cohort query and need no enrichment at all — as the chest X-ray tutorial demonstrates, projecting Effusion/Edema/Normal as dataframe columns. Enrichment is for labels with nowhere to live in OMOP, chiefly segmentation masks.
• Cross-links from user-common, flip-workflow and component-xnat; a flip.xnat section in the flip-utils overview (also the PyPI long description); both spleen tutorial READMEs.

Note for reviewers

The accession mapping is read from the OMOP CSV export for the data version this checkout deploys — the URL is built from trust/omop-db/.data_version (currently 20260729), so the mapping and the pgdata the trusts are seeded from share a single pin and cannot drift apart. If that file is missing or empty the script stops rather than guessing a version and silently fetching a mapping for different mock data.

Linked Issues

Closes #776

Checklist

• Follows the project's coding conventions and style guide
• Updates documentation
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published

Type of Change

• Non-breaking change (fix or new feature that would not break existing functionality).
• Breaking change (fix or new feature that would cause existing functionality to change).
• New tests added to cover the changes.
• In-line docstrings updated.
• Documentation updated, tested make -C docs/ docs.

Verification performed

• flip-utils: 729 tests pass (688 pre-existing + 41 new for flip.xnat), ruff clean, flip/xnat mypy-clean (the 6 reported errors are the pre-existing flip/core baseline, unchanged).
• make -C docs docs — build succeeded, no new warnings; the new page registers in the User guides toctree.
• fl-tutorials lint unchanged at 18 pre-existing errors (verified the one in a file I touched, client.py, exists on develop); scripts/check_tutorial_sync.sh green.
• The full Make chain exercised end to end: builds the in-tree flip-utils, fetches the mapping from HF (41 accessions, 21/20 split), pairs labels against the real MSD download, and stops at the credentials check. Missing-argument guard fires on both backends including through the Flower delegation.
• $FLIP_PROJECT_ID escaping through the new Makefile indirection confirmed by inspecting the expanded recipe.

Verified live against the dev trust stack. Brought up GSTT + KCH, created a spleen project (cf497b94-…), waited for the pull, then exercised the tool end to end:

Check	Result
resolve_project_by_flip_project_id	✅ resolved via secondary_ID on both trusts
list_scans / list_resource_files	✅ real scans + NIFTI/DICOM resources enumerated
Target-name derivation	✅ local label_spleen_63.nii.gz → label_CT_Spleen_20001201230811_1871960997.nii.gz, derived from the converted image in XNAT, not the local filename
GSTT dry run	✅ would upload 21, 0 skipped, 0 failed — exactly the count source_trust predicts
GSTT live upload	✅ 21 uploaded, 0 failed
Idempotency	✅ re-run reports 21 skipped (already present), nothing re-uploaded
Placement	✅ each scan's NIFTI resource now holds input_CT_Spleen_….nii.gz and label_CT_Spleen_….nii.gz
KCH dry run	✅ would upload 17, 3 correctly reported as skipped (no matching scan)

The KCH shortfall is a trust-side pull stall, not this change: imaging-api reports Processing=3, Successful=17, Failed=0 on a freshly-booted XNAT (the known directArchive/DQR stall), Orthanc logs no C-STORE aborts, and a permissions fault would have failed all 20 rather than 3. The tool reports those three honestly as skips rather than failing or inventing a match.

Also confirmed live: a DICOM resource (no input_ prefix) derives no target name, so it is skipped rather than risking an overwrite of imaging data.

---

Update — merged develop (5772968)

Merged 129 commits of develop, chiefly the client-API migration (#983) and Flower differential privacy (#833). What that changes about the diff above:

• "The five spleen train/eval entry points" is now three in this PR. #983 deleted two of them outright (3d_spleen_segmentation/client.py, 3d_spleen_segmentation_evaluation_client_api/) and its rewritten 3d_spleen_segmentation/app_files/trainer.py already raises on an empty datalist — so that entry point is covered on develop and needs nothing here. The remaining three (both Flower data_loading.py files and the NVFLARE 3d_spleen_segmentation_evaluation evaluator) carry the change; the evaluator edit landed byte-identically on the file that replaced the deleted _client_api copy.
  • Note the two surviving messages differ in wording: develop's trainer raises ValueError with its own phrasing, this PR's three raise RuntimeError and point at the new Data Enrichment guide. Worth unifying, but deliberately left alone here rather than rewriting merged code in a merge commit.
• make -C fl-tutorials test-template is gone — removed on develop. The three Makefiles keep that removal and add upload-spleen-labels.
• e2e_smoke_spleen uploads the Flower tutorial paths (develop's change) while the enrichment step runs out of the NVFLARE tree (backend-agnostic, this PR's design). CLAUDE.md/AGENTS.md now state both.
• Both spleen READMEs keep both sides' new sections (develop's simulator + differential-privacy sections, this PR's data-enrichment section).

Verification re-run post-merge

• fl-tutorials: ruff clean (the 18 pre-existing errors noted above were fixed on develop), 90 passed / 12 skipped.
• flip-utils: ruff clean, 748 passed (46 in flip/xnat), uv lock --check in sync.
• scripts/check_tutorial_sync.sh green; make -C docs docs build succeeded with 1 warning, pre-existing on develop (component-fl-nodes.rst, untouched here).
• upload-spleen-labels expands and its missing-argument guard fires on both backends, including through the Flower delegation.
• CI: 34/34 green on the merge commit (1 skip: the PR-to-main guard, correct for a develop-targeted PR).

---

Update — review round 2 (a12f9e5)

garciadias's review found three blocking gaps and five smaller ones. All eight are addressed; each thread carries its own reply. Two turned out worse than the review had them, and verifying them surfaced two further defects.

The three blocking ones

• Enrichment covered one trust. The uploader talked to one XNAT per run while e2e_smoke_spleen invoked it once, so the second trust got no labels and its client died at the new zero-pairs guard minutes into training — silently, because skipped_no_scan is not failed, so the step exited 0. --credentials-file is now repeatable and --xnat-url added, so one invocation covers the roster; flip-api/Makefile gains SPLEEN_XNAT_URLS (both dev trusts, credentials from the environment — no secrets in the Makefile). The manifest is self-selecting: an accession exists at exactly one trust, and the others report it as no matching scan. A trust that never pulled the project is a soft skip with a roster, still fatal alone.
• A run resolving zero destinations exited 0. ok ("nothing failed") is unchanged — it is a public property with an accurate name. A new resolved_any drives a shared EnrichmentReport.exit_code(), aggregate rather than per-server. skipped_no_resource deliberately does not count as resolved, so the ran-before-conversion case now fails too. --allow-no-op opts out; --dry-run is not special-cased, so a rehearsal predicts the real exit code.
• overwrite never reached the request — and worse than reported: on a 409 both branches raised, so --overwrite could not overwrite, it only made the error opaque. XNAT has no overwrite parameter, so it is now pre-checked in the client, mirroring imaging_api/services/upload.py:285-287.

The five smaller ones

target_filename is rejected at parse time unless bare, and percent-encoded at the point of use (verified against requests 2.34.2: parse_url splits on ?/# before quoting, so a # silently dropped inbody=true); HF_TRUST_DATA_REVISION is env-overridable, following update_omop_data.sh:38, and the mapping is cached on disk so a re-run needs no huggingface.co egress; coverage is reported with the correct post-filter denominator, at WARNING, naming the missing cases; --trust is validated against the fetched mapping instead of hardcoded choices; and the kit-name → XNAT_HOST correspondence is documented.

Two suggestions not taken, argued in-thread: no sha256 on the mapping (no download in this repo is checksummed, and three other readers hit the same dataset at main — worth doing across all four as its own change), and coverage warns rather than fails by default (fail-by-default would break e2e_smoke_spleen, which sets no NUM_CASES, and is unsatisfiable through the Flower delegation; --require-full-coverage opts in).

Found while verifying, not in the review

• A command-line EXTRA_ARGS silently erased enrichment. A command-line variable beats a target-specific one, so make -C flip-api e2e_smoke_spleen EXTRA_ARGS="--project-id <uuid>" — the documented fast-iterate flow — ran with no enrichment and died at the zero-pairs guard. Enrichment flags now ride in a separate ENRICHMENT_ARGS that composes.
• cli.py had no tests at all (0%; codecov's 39 missing lines), so no exit code in the subpackage was pinned. New test_cli.py, 13 cases, cli.py → 97%.
• The Flower delegation's tree holds 6 of 41 cases and has no NUM_CASES knob, so its remediation message pointed at a variable that does nothing there. The text now names both backends unconditionally, and the Flower README says so.

Verification

flip-utils 776 passed (748 → +28), ruff clean, flip/xnat mypy-clean. fl-tutorials 90 passed / 12 skipped, ruff clean, check_tutorial_sync.sh green. make -C docs docs succeeded with no Sphinx warnings. uv lock --check in sync. develop merged.

Checked directly, not only by unit test: the cache makes one network call for two reads and re-fetches on a version bump; HF_TRUST_DATA_REVISION reaches the URL; --trust 7 is rejected naming the trusts present; --xnat-url without credentials fails clearly; and make -n e2e_smoke_spleen EXTRA_ARGS="--project-id 1234" expands with both --xnat-url flags and the --project-id. A two-trust live dry run against the dev stack is the remaining check before merge.

---

Update — review round 3 (ee74d1c, 4dc7c94)

Three threads, all accepted.

• A roster where no trust holds the project exited 0. With two or more servers a per-server XnatProjectNotFound is a legitimate skip, so when every server raised it no EnrichmentSummary was produced at all: requested fell back to 0 and the no-op guard never fired — the exact silent no-labels case round 2 added it for. exit_code() now fails when there are outcomes but no summaries (fatal ones are caught above it, so the only way to reach that state is every server erroring), and does so regardless of --allow-no-op: a project existing at no trust means the image pull never ran there. Covered at library level and through the real CLI with --allow-no-op set.
• --require-full-coverage was blind to an un-pulled trust. Its denominator sums only the summaries that exist, so an un-pulled trust contributed zero scans and one fully-enriched trust vouched for the whole roster. fully_covered now also requires every visited server to have produced a summary. Covered by a one-holder/one-absent roster asserting exit_code() == 0 but exit_code(require_full_coverage=True) == 1.
• Exception type unified across the four empty-pairs guards. The NVFLARE spleen trainer raised ValueError where the two Flower apps and the NVFLARE evaluator raise RuntimeError; it now raises RuntimeError too. That ValueError was pre-existing code from f651b9b, not one of this PR's guards, and nothing in-tree catches either type on that path — so this is consistency, not a behaviour fix. The missing-accession_id-column ValueError in the same file is left alone; that one genuinely is a bad-value condition.

The --allow-no-op and --require-full-coverage rows in the user guide, its "resolves nothing" note, its secondary_ID troubleshooting row, and the flip-utils overview all now state these contracts.

Verification

flip-utils 789 passed (776 → +13, of which +2 for these fixes and the rest from the round-2 follow-up commit), ruff clean, flip/xnat mypy-clean (the 60 reported errors are the pre-existing flip/core, flip/flower and flip/nvflare baseline — none in flip/xnat). make -C fl-tutorials test: ruff clean, 90 passed / 12 skipped. Both edited .rst files parse without error.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[garciadias]

This is a substantial and largely well-built replacement. The contract from the e2e-smoke section of the instructions file is honoured on all four points I checked: enrichment runs after the pull and after DICOM-to-NIfTI conversion, it resolves each trust's XNAT project by secondary_ID, and it writes each label into the scan's existing NIFTI resource with input_ renamed to label_. Apache headers are present on all eleven new source files, there are zero new dependencies (only [project.scripts]), so no cooldown or lock concern arises, the new Make targets are documented, and the AGENTS mirror is exact. The CLAUDE.md text about the private repo, .xnat1.cfg/.xnat2.cfg, and "tracked in FLIP#776" is all correctly removed.

I also want to record two things explicitly, since both were prominent risks going in. There are no hardcoded credentials or hostnames anywhere in this change — every example is a placeholder. And the labels are deliberately not republished, so the MSD CC-BY-SA attribution obligations are unchanged; only the accession-to-case mapping is new.

Both earlier review points are properly resolved at this head. not_found_ok with 404-only handling is in place with all four claimed tests, and I swept the rest of the enrichment path for other swallowed exceptions and found none. The per-scan versus per-accession decision is sound as argued — the contract fix is the right call over deduplication, and the rendered output is unambiguous. Neither needs revisiting.

Three things should change before this merges.

Enrichment only covers one trust. e2e_smoke_spleen invokes the command once, but the new script talks to exactly one XNAT per run, whereas the private script it replaces was multi-trust in a single invocation. On the standard two-trust dev roster the second trust gets no labels and its client dies on the new zero-pairs guard. The user guide already says to run it once per trust, and the PR's own verification table drove GSTT and KCH separately, so this looks like an oversight — but the instructions file currently reads as though the target is complete, which will cost someone a full pull cycle.

An all-skipped run still exits 0. ok is defined as "nothing failed", so a run that resolved zero destinations reports success and returns 0 from both entry points. run_data_enrichment only raises on a non-zero exit, so the smoke treats a no-op enrichment as done and proceeds into training. The loud, actionable summary genuinely fixes the interactive case — this is the scripted case, where the signal the automation consumes is unchanged. Since removing that opaque num_samples=0 failure is the point of the issue, closing the automated half seems worth doing here.

The overwrite flag never reaches the request. The URL always carries just inbody=true; overwrite only selects between two error messages. Either --overwrite cannot overwrite, or overwrite=False protects nothing — which of the two depends on how XNAT responds to an existing file, and imaging-api's own pre-check suggests the latter. The product path is safe today because the enrichment layer pre-checks, so this is about the public client method and its docstring promise.

The five minor comments cover the mutable main revision with no checksum or cache on the mapping fetch, the partial-label-set case passing quietly when NUM_CASES is 10 against a 41-accession mapping, the --trust choices being pinned to the two-trust mock roster, an unescaped manifest target_filename reaching URL construction, and the new XNAT_HOST/XNAT_USER/XNAT_PASS triple not lining up with the names the kit files already use.

Two notes that are not change requests:

On closing #776. No CI workflow is added, and none is claimed, so this is not a checklist mismatch — but the "in CI" half of the issue is enabled rather than delivered, since a run still needs a live stack, XNAT credentials and egress to huggingface.co. Worth deciding whether #776 closes here or keeps a CI-enablement tail.

One thing I could not verify. Whether the Flower delegation's data/spleen/accession-resources tree actually uses the subject_N/scans/ names that build_manifest requires. The dataset is not local, and the verification in the description covers the NVFLARE labels directory and argument expansion. Could you confirm the Flower path end to end, or point me at a run that did?

[atriaybagur]

Thanks for the depth here — the three blocking items were all real, and two turned out worse than the review had them. All eight threads are addressed in a12f9e5 and answered individually; this covers the two questions and the places I did not do what was suggested.

The Flower path you could not verify — confirmed, with a caveat

data/spleen/accession-resources does use the layout build_manifest requires. Verified against the live HF tree (aicentreflip/flip-fl-base-test-data), not inferred: .../accession-resources/subject_2/scans/{input,label}_spleen_2.nii.gz, byte-for-byte what line 157 constructs. The Flower Makefile comment at :43-44 is accurate. It is also structurally forced — in LOCAL_DEV, flip/core/standard.py:634 keys the directory on accession_id, and for the spleen dev data those ids are the subject_N strings.

But checking it surfaced something else: that tree holds 6 of 41 cases (3 per trust), so the Flower delegation silently produced ~14% enrichment, and the error text it emitted told the user to run download-spleen-data NUM_CASES=41 — a variable fl-tutorials/flower/Makefile does not have. Both fixed: the remediation text now names both backends unconditionally, and the Flower README says plainly that its snapshot is a fixed 6-case subset and points at the NVFLARE download for full coverage.

#776

Keeping Closes #776. The issue's own Suggested directions accept "Port the uploader in-tree … Even if the fixtures stayed external, the step would be discoverable, reviewable and CI-able", and this delivers that plus the "fail loudly" direction. You are right that the "in CI" half is enabled rather than delivered, so I will file a follow-up for actually wiring a spleen e2e into CI rather than let it disappear — there is no existing issue for it (#63, the integration-tests epic, is the nearest home).

Where I did not do what was suggested

• No sha256 on the mapping. Revision pinning and caching are done — HF_TRUST_DATA_REVISION now follows the idiom update_omop_data.sh:38 and update_orthanc_data.sh:49 already use, which I had missed. The digest I have argued against on the thread: no download in this repo is checksummed, and omop-db/Makefile:100 and dataset.py:59 read the same dataset at main, so it would be a new house rule at one of four call sites plus a second thing to bump per data version. Worth doing across all four readers as its own change.
• Coverage warns rather than fails. Fail-by-default would break e2e_smoke_spleen (it sets no NUM_CASES) and is unsatisfiable through the Flower delegation. --require-full-coverage opts in.
• XNAT_HOST not renamed to XNAT_URL. Documented the correspondence instead; renaming would break the container-job path the naming is chosen for. Being straight though: nothing in this repo exercises Container Service injection today — dcm2niix_command.json declares "environment-variables": {} — so that justification is forward-looking, not demonstrated.

Two defects found while verifying, not in the review

• A command-line EXTRA_ARGS silently erased enrichment. A command-line variable beats a target-specific one, so make -C flip-api e2e_smoke_spleen EXTRA_ARGS="--project-id <uuid>" — the documented fast-iterate flow — ran the spleen smoke with no enrichment and died at the zero-pairs guard minutes later. Same time-wasting shape as the multi-trust bug. Enrichment flags now ride in a separate ENRICHMENT_ARGS that composes instead of being clobbered.
• cli.py had no tests at all (0%, codecov's 39 missing lines), so no exit code anywhere in the subpackage was pinned — which is exactly what your second thread was about. New test_cli.py: 13 cases, cli.py to 97%.

Verification

flip-utils 776 passed (748 → +28), ruff clean, flip/xnat mypy-clean (the 2 remaining errors are the pre-existing flip/core baseline). fl-tutorials 90 passed / 12 skipped, ruff clean, check_tutorial_sync.sh green. make -C docs docs build succeeded, no Sphinx warnings. uv lock --check in sync. develop merged (was 3 behind).

Behaviour checked directly rather than only by unit test: the mapping cache makes one network call for two reads and re-fetches on a version bump; HF_TRUST_DATA_REVISION=abc123 reaches the URL; --trust 7 is rejected naming the trusts present; --xnat-url without credentials fails with a clear message; and make -n e2e_smoke_spleen EXTRA_ARGS="--project-id 1234" still expands with both --xnat-url flags and the --project-id. The remaining live check is a two-trust upload-spleen-labels dry run against the dev stack, which I will run before this merges.

[atriaybagur]

Live verification of the new code, run against the dev stack. CI is green on a12f9e5 (34 pass, 1 skip — the PR-to-main guard).

One caveat up front: I could not run the spleen target with its own roster defaults. xnat1 (:8104) is serving 404 on every REST path — its webapp is not up, a pre-existing dev-stack condition unrelated to this change — and the MSD labels are not downloaded locally. So instead of the spleen driver I exercised the same library paths through the generic flip-xnat CLI against the two XNATs that are live: :8106 (holds project 92b55959-…, 300 experiments) and :9104 (does not hold it). That covers every line the spleen driver now goes through, since both entry points run the same run_enrichment / exit_code code.

Multi-trust in one invocation — two --credentials-files, one run:

Resolved FLIP project 92b55959-… to XNAT project 8e20be11-…
Found 300 scan(s) across 300 experiment(s)
FAK29433102: would upload mask.nii.gz as label_Chest_HCT_20021205063306_61823.nii.gz
FAK14926162: would upload mask.nii.gz as label_Chest_HCT_20141024075720_924270.nii.gz
FAK72607527: would upload mask.nii.gz as label_Chest_HCT_20180905064014_301239.nii.gz
NOT_AT_ANY_TRUST: no matching scan in project 8e20be11-…

XNAT project 8e20be11-… (http://127.0.0.1:8106):
  would upload: 3 file(s)
  skipped (no matching scan): 1 accession(s)
  coverage: 3/300 scan(s) in the project now enriched

↷ http://127.0.0.1:9104: No XNAT project at http://127.0.0.1:9104 has secondary_ID=... Has the image pull for this project run at this Trust?

Across 2 server(s): 3/300 scan(s) enriched

Both servers visited; target names derived from the real converted images in XNAT, not from the local filename; the trust that does not hold the project soft-skipped with ↷ rather than failing the run.

Exit codes, same live servers:

Case	Exit	Before
Normal run, 3 destinations resolved	0	0
Manifest matching nothing anywhere	1	0 ← the bug
Same, --allow-no-op	0	—
3 of 300 scans, --require-full-coverage	1	—

The second row is the one that mattered: that run prints would upload: 0 file(s) and ⚠️ Nothing was resolved: 2 item(s) requested, no destination found, and now exits non-zero, so run_data_enrichment raises instead of logging ✅ data enrichment complete and walking into training.

Also confirmed the SPLEEN_XNAT_URLS default is right: docker service ls shows xnat1_xnat-web *:8104->8080 and xnat2_xnat-web *:8106->8080, which is exactly what the Makefile now passes.

Everything was --dry-run — I did not write into a live dev project. A real two-trust upload still wants xnat1 healthy and NUM_CASES=41 downloaded; happy to run that once the stack is, if you would like it before merge.

Follow-up filed as #998 for the CI-enablement tail of #776, and it also picks up the lexicographic NUM_CASES selection in download_spleen_dataset.py:78 that I left out of this PR.

[atriaybagur]

Addressed the Codecov patch-coverage report in 6c90374. Covering those 10 lines turned up two real defects, so this is worth more than the number moving.

Network failures were reported as local-file errors. requests.RequestException subclasses OSError, so in upload_scan_resource_file the except OSError clause listed first caught every transport failure and raised Could not read <local file>: <err> — pointing the operator at a file that is perfectly fine while the real cause was the network or the server. It also made the except requests.RequestException branch dead code, which is exactly why coverage flagged lines 299-302 as unreachable. Handlers reordered, specific before general, with a comment saying why the order is load-bearing.

My own bare-filename guard missed ? and # — the two characters that motivated it, and the ones I called the likelier real-world bite on the thread above. Both are legal in POSIX filenames, so Path(name).name happily accepts them; only the client-side quote(..., safe="") was stopping them severing the URL. The manifest check now rejects path separators, ./.., and the URL-significant set (? # \ %), so it gives the clear parse-time error it promised rather than leaning on the escaping downstream. Four rejection cases plus the accepted one are parametrised.

flip.xnat is now at 100% line coverage (321 statements). New tests: the unreadable-file and transport-failure upload paths; manifest validation; the failed-upload and missing-project-id exit codes; and python -m flip.xnat propagating main()s exit code through runpy — worth pinning because that exit code is the contract e2e_smokes enrichment hook consumes. FakeSession gains put_error mirroring its existing get_error.

The single remaining exclusion is cli.pys if __name__ == "__main__" guard, marked # pragma: no cover with a reason: __main__.py covers the same call, and the repo already uses that pragma sparingly for genuinely unreachable lines.

flip-utils: 786 passed (748 at the start of this review round), ruff + format clean, flip/xnat mypy-clean, uv lock --check in sync.

[garciadias]

Good in-tree replacement for the private-repo label upload. One correctness gap blocks merge: with a multi-trust roster where no server resolved the project, run_enrichment exits 0 silently, defeating the fail-loud goal. Please handle the all-missing case and add a test.