fix(rest): render batch entry failures through the single-resource error mapping

[aacruzgon]

Summary

Every error a batch entry could produce was built by one helper that hardcoded its OperationOutcome
issue code, so a scope denial, a missing resource, a malformed entry and an unsupported method all
reached the client as code: "processing" — distinguishable only by response.status and free-text
English. OperationOutcome.issue.code is bound required to http://hl7.org/fhir/ValueSet/issue-type
in all four bundled versions, and the ElementDefinition adds an unqualified SHALL: "The system that
creates an OperationOutcome SHALL choose the most applicable code from the IssueType value set."
Nineteen call sites emitting one code is the absence of a choice.

The issue proposes an issue-code argument on create_error_result. That treats the symptom. The
defect is that create_error_result existed at all: a second renderer of failure-to-OperationOutcome,
written beside the one IntoResponse already uses. entry_error (batch.rs:1533) is the proof —

let (status, _code, message) = RestError::from(err).client_response();

— it computed the correct code and bound it to _code, after which the wrapper stamped processing
over the result. And client_response's own doc comment already claims the parity that was missing:
"the single source of truth for how a RestError is surfaced to callers, shared by IntoResponse
and the batch/transaction handler so both sanitize identically." That sentence was false.

So the second renderer is deleted rather than parameterised. Giving it an argument keeps two mappings
agreeing by hand — the arrangement #502 died of.

This branch now carries #481

The diff below is #504's four commits plus #481's one. #481 (fix/478-bundle-get-search, "execute
search-style GET bundle entries as searches") was merged into this branch on 2026-08-07 and its branch
deleted. A later rebase onto current main flattened that merge, so its work is the top commit here —
+393/-16 across 5 files, including handlers/search.rs, which nothing else in this PR touches.

#481 reads MERGED on GitHub, but it was merged into this branch, not into main. Its code reaches
main through this PR and the four below it. Reviewing #516 means reviewing that commit too.

The conflict the Notes below anticipated resolved exactly as predicted: both of #481's _
code-discard call sites became entry_failure(e), and deleting create_error_result made that a
compile error rather than a silent reintroduction of processing. #518 pins both with tests.

Changes

One funnel. RestError::client_outcome() is now the only place a RestError becomes an
OperationOutcome. IntoResponse renders the pair as an HTTP body; handlers::batch renders it as a
Bundle.entry.response.outcome. Neither builds its own. Four functions deleted —
create_error_result, entry_error, validation_failure_message, EntryMethodRefusal::status() —
two added. Each call site constructs the same RestError its transaction twin already constructs,
so the arms cannot report different codes because there is no second table to disagree with.

Two new RestError variants, children of BadRequest's invalid in the issue-type hierarchy
rather than alternatives to it: MissingElement (400 + required) and InvalidElementValue
(400 + value). The crate could say neither before. MissingElement is used only where the SD gives
min=1; deliberately not for an absent Bundle.entry.resource, which is 0..1 with only R5/R6's
bdl-3c requiring it — a call site serving four versions must not assert a rule two of them lack.
Invariant keys stay in doc comments and off the wire, because bdl-3 does not exist in R5 or R6.

Entry failure	Status	Was	Now
request absent	400	processing	required
request.method absent	400	processing	required
request.method not an http-verb code	400	processing	value
request.url absent	400	processing	required
request.url names nothing	400	processing	value
resource absent on POST/PUT	400	processing	invalid
PUT/DELETE URL names no instance	400	processing	value
conditional criteria in the URL	400	processing	not-supported
insufficient scope	403	processing	forbidden
target not found	404	processing	not-found
HEAD	405	processing	not-supported
write validation failed	422	processing	the validator's own issues
PATCH	501	processing	not-supported
storage errors (×5 sites)	unchanged	processing	whatever client_response computed
ifMatch failed	412	conflict	untouched — already correct

The 403 is the sharpest row: forbidden is a child of security, and processing is not an ancestor
of it in any version, so a client filtering code is-a security to trigger re-auth got a false
negative. Four other emitters in this codebase already say forbidden for the identical denial.

The 422 is the lossiest case. check_write returns a fully-formed multi-issue outcome — per-issue
code, severity and expression (the FHIRPath location). The batch arm joined issue[].details.text
with "; " and re-wrapped it under processing. The transaction arm never did: it propagates with a
bare ?, reaching the branch whose comment already states the rule. So an identical bundle returned
typed codes and FHIRPath expressions as a transaction and one English sentence as a batch, decided
purely by Bundle.type. The interception now lives in client_outcome above client_response,
which is load-bearing: client_response's own ValidationFailed arm returns (422, "processing", "Resource validation failed"), so routing the 422 through the code table would reproduce the defect
with a shorter message.

Two exposed inconsistencies (commit 4): status_text had no arm for 413/429/503/504, all reachable,
so an exhausted pool rendered "503 Unknown" beside a correct transient; and
extract_outcome_description read only details.text, while the 412 gate writes diagnostics — so a
failed ifMatch produced an AuditEvent with no outcomeDesc at all.

On the element this writes into

Bundle.entry.response.outcome carries a comment, byte-identical in all four bundled versions and
generated into the model (crates/fhir/src/r4.rs:10011):

This outcome is not used for error responses in batch/transaction, only for hints and warnings. In a
batch operation, the error will be in Bundle.entry.response, and for transaction, there will be a
single OperationOutcome instead of a bundle in the case of an error.

Four things. It is a comment, not an invariant — no bdl-* constrains that element, verified by
enumerating every Bundle constraint in all four versions. HFS has placed error outcomes there since
before this stack, pinned by test_batch_error_outcome_in_response_not_resource, so this changes the
code and not the placement. "The error will be in Bundle.entry.response" holds: response.status
still carries it and outcome is a sibling under the same response. And the SHALL applies to any
OperationOutcome the system creates — if HFS creates one here it must code it correctly regardless of
whether it was obliged to create one.

Testing

cargo test -p helios-rest   →  1103 → 1109 passed, 0 failed
cargo fmt --all --check     →  clean

Six net-new tests, four compile-forced rewrites, five assertion-only extensions.

Verified non-vacuous, with the two disable runs isolated from each other:

• Disable 1 — entry_failure reverted to a hardcoded processing outcome, ValidationFailed
  short-circuit kept: 8 unit + 3 integration tests fail, e.g. left: String("processing"), right: "forbidden" / "not-found" / "required" / "exception". The 422 leg stays green (7 passed),
  which is what makes the two runs independent.
• Disable 2 — validation_failure_message restored at both sites: exactly one test fails,
  left: [("processing", "")], right: [("structure", "Patient.bogusElement")].
• The refusal tests keep their teeth for free: DelayStorage's writes are unimplemented!() and
  peak() == 0 is still asserted, so a refusal moved after dispatch panics rather than reporting a
  different code.

batch_if_match.rs:152 passing untouched is the evidence of restraint — the 412's conflict was
already correct and was deliberately not routed through the funnel.

Clippy: CI's invocation (ci.yml:497, which carries -A collapsible_if and seven sibling allow-flags) passes clean — exit 0, zero errors. Worth correcting explicitly, because this PR previously repeated the parent PRs' claim that the gate "still exits 101 on pre-existing lints tracked in #513": that is true of the bare cargo clippy --all-targets --all-features -- -D warnings, but not of what CI runs. All 42 crates/rest warnings and all six files that fail the bare form sit inside the allowed lint families, so the Linting job was never going to fail. Separately measured: crates/rest lint set is 42 → 42, zero introduced, zero removed against the base with forced recompilation.

Notes

• Messages that change, enumerated rather than counted. Three: the GET miss ("Resource not found"
  → "Resource Patient/ghost not found", which is what makes byte-identical parity with the resource
  endpoint possible), HEAD (both arms now print the guidance the batch arm printed and the transaction
  arm dropped), and PATCH (NotImplemented wraps its feature string). Every other message is
  byte-identical, including all of fix(rest): parse bundle entry URLs without their query string #512's. Nothing anywhere asserts on entry prose.
• All three deferrals discharged. fix(rest): run batch bundle entries with backend-bounded concurrency #505 filed it; fix(rest): parse bundle entry URLs without their query string #512 promised "batch: every per-entry error carries OperationOutcome code processing — a 403, 404 and 405 are indistinguishable without parsing English #504 fixes uniformly"; fix(rest): parse bundle entry methods through one shared matcher #515
  promised "batch: every per-entry error carries OperationOutcome code processing — a 403, 404 and 405 are indistinguishable without parsing English #504's stated fix, and landing it here would take that PR's mechanism early."
• One finding was pushed down the stack rather than carried here. Grounding this work turned up
  that a_transaction_get_with_a_query_is_not_declined_by_the_query_guard cannot fail: its third
  conjunct reads issue[0].diagnostics, which RestError never emits, so the negated assert holds
  even when the guard does fire. That test belongs to fix(rest): parse bundle entry URLs without their query string #512's f7a87938c, so the one-field fix landed
  there as a follow-up commit instead of here — blame stays with the PR that owns the test, and fix(rest): parse bundle entry URLs without their query string #512
  stays a fast-forward with its in-flight review intact. fix(rest): parse bundle entry methods through one shared matcher #515 and this branch were restacked on top.
• fix(rest): execute search-style GET bundle entries as searches #481 conflict is real, not avoided — now discharged; see "This branch now carries fix(rest): execute search-style GET bundle entries as searches #481" above.
  It inserts a search branch immediately above the GET arm's
  refusal at :751 — the one expression this PR changes on that arm — and adds two call sites with
  the _ code-discard, one of them on the transaction arm. That second one falsifies "no transaction
  per-entry outcome is reachable", which is true of the current tree only. Both become
  entry_failure(e) on rebase, and deleting create_error_result makes that a compile error
  rather than a silent reintroduction of processing. fix(transaction): resolve conditional references before execution #467 lands after status_text, which commit 4
  also edits. Integration coverage went in a new file so batch: every per-entry error carries OperationOutcome code processing — a 403, 404 and 405 are indistinguishable without parsing English #504 is not a fourth claimant on
  batch_conformance.rs's append anchor.
• Known amplification: validation_outcome is uncapped, so a 422 entry now carries N issues where
  it carried one. Exact parity with POST [base]/[type], unbounded since enforce mode shipped. Any cap
  belongs there, covering both surfaces; a batch-only cap would re-create the divergence this closes.
• Still broken, documented not fixed: a write entry failing after dispatch inside a transaction is
  collapsed to 400 processing by TransactionError::BundleError, which carries neither status nor
  code — the backends discard the entry result at their >= 400 guard.
  Both halves of this bullet were narrowed once fix(rest): execute search-style GET bundle entries as searches #481 landed on this branch. It no longer holds for
  search entries: fix(rest): execute search-style GET bundle entries as searches #481's search loop bypasses the backend executor, so a failed search surfaces as that
  entry's own outcome — the first reachable per-entry outcome on the transaction arm, which test(rest): pin the issue codes on #478's two search-entry failure paths #518 asserts.
  And the bare type-level GET Patient → "Resource Patient/ not found" claim is simply false here now;
  that arm is removed by the top commit.
• Deliberately not done: retyping client_response to return IssueType. That enum lacks
  Forbidden, Exception, Timeout, Throttled and TooLong, all of which client_response returns
  today, so the typed path would start by admitting the type is incomplete. Separate change.
• R4B-only + sqlite does not compile the integration targets, on this branch or its base — the test
  harnesses hardcode FhirVersion::R4 in seeding. The new file follows the same convention as the file
  it mirrors; the lib itself has 38 pre-existing errors in that configuration, identical before and
  after.

Closes #504

[codecov]

Codecov Report

❌ Patch coverage is 90.61224% with 46 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/rest/src/handlers/batch.rs	91.38%	38 Missing ⚠️
crates/rest/src/error.rs	80.95%	4 Missing ⚠️
crates/rest/src/handlers/search.rs	77.77%	4 Missing ⚠️

📢 Thoughts on this report? Let us know!

[smunini]

Review

The core refactor is genuinely good work. RestError::client_outcome() as the single funnel is the right fix, not the issue's suggested "add an issue-code argument" — the let (status, _code, message) discard in entry_error was the actual defect, and deleting it corrects five call sites by construction. The 403 processing → 403 forbidden row matters most (forbidden is-a security; processing is not an ancestor of it in any version). Threading real codes also surfaced two latent bugs the PR fixes: status_text had no arm for 413/429/503/504, and extract_outcome_description never read diagnostics, so a failed ifMatch produced an AuditEvent with no outcomeDesc at all. The disable-runs proving the tests non-vacuous are the right discipline.

However, I'd hold the merge on one finding — and it's in the #481 commit this branch carries, not in the #504 work.

Verification I ran

• cargo fmt --all --check → clean (verified on 55f6fd3a1, not on main).
• cargo test -p helios-rest --all-features → 971 passed, 1 failed. The one failure is environmental: sof_conformance_mongodb_tests::test_sof_v2_conformance_in_db_mongodb panics with SocketNotFoundError("/var/run/docker.sock") — no Docker daemon locally. Every new batch/issue-code and search-entry test passes.

---

Blocking: bundle GET search entries are authorized with the READ scope, not SEARCH

bundle_method_to_fhir_operation (crates/rest/src/handlers/batch.rs:1797) maps every BundleMethod::Get to FhirOperation::Read, and both scope gates use it — the transaction arm at :459 and the batch arm at :809.

Before the 01929ed56 commit this was harmless: a type-level GET Patient?family=X entry was a read of an empty id and answered 404. It now executes a real type-level search.

• crates/auth/src/policy/mod.rs:31 maps Read → SmartPermissions::READ and Search → SEARCH, and these are distinct bits — SMART v2 .r does not imply .s (crates/auth/src/scope/permissions.rs:41).
• crates/rest/src/middleware/auth.rs:416 classifies GET /Patient?... (one path segment) as FhirOperation::Search.
• middleware/auth.rs explicitly defers bundle authorization to the handler ("the same 'defer to handler' pattern batch and $export already use"), so this per-entry check is the only gate.

Concrete scenario: a SMART token with user/Patient.r (read, no search) is denied at GET [base]/Patient?family=X, but gets the full searchset by wrapping that exact URL in a batch entry. Scope escalation. The inverse also breaks — a .s-only token gets a spurious 403 on a bundle search entry.

This is precisely the batch-vs-single divergence class this PR stack exists to close, so it seems in-scope to fix here rather than defer. No existing test asserts that a bundle search entry is gated on the same permission as the equivalent HTTP request.

---

Should fix

parse_search_entry_url accepts any single path segment as a resource type (batch.rs:1498). The match arm is [resource_type] => Some(...) with no validity check, and nothing on the search path validates the type either. GET metadata (an entry form the spec sanctions), GET $export, or a typo'd GET Patinet now run a search on that string and return 200 OK with an empty searchset. Previously they answered 404. A false 200 is strictly worse for a client — it can't distinguish "nothing matched" from "no such endpoint".

The README limitation added by commit 4 is falsified by commit 5 of the same PR. crates/rest/README.md:579 still reads:

Bare type-level GET - GET Patient in a batch entry is read as an instance read with an empty id and answers 404 not-found … Executing it as a search is #478

01929ed56 implements exactly that. The PR body acknowledges the claim "is simply false here now", but the line wasn't removed.

Prefer: handling=strict is ignored for bundle search entries. Both execute_search_bundle call sites (batch.rs:600, :862) hardcode strict = false, while batch_handler already has the parsed PreferHeader in hand. GET Patient?famly=X (typo) in a strict-handling bundle returns 200 OK with an unfiltered searchset; the identical GET [base]/Patient?famly=X with the same header returns 400. New divergence in the same family this PR closes.

Audit events for search entries name the wrong entity type. emit_entry_audit derives resource_type from the entry URL, then unconditionally overrides it from result.resource["resourceType"] (batch.rs:1190). A searchset serializes as "resourceType": "Bundle", so an AuditEvent for GET Patient?family=X inside a Bundle records the accessed type as Bundle instead of Patient. extract_patient_from_resource("Bundle", …) also returns None, so the patient-compartment linkage is lost. Relevant given the BALP profiles.

---

Nits / discussion

• batch.rs:522 — the transaction search pre-validation does map_err(|e| RestError::BadRequest { message: format!("… {}", e.client_response().2) }), discarding the status and issue code the inner error computed and re-coding everything as 400 invalid. That is the exact _code-discard pattern this PR deletes in five other places.
• batch.rs:600 — a search entry failing after the transaction commits surfaces as a per-entry outcome inside a 200 OK transaction-response. Defensible (the writes already committed; there's no rollback available) and the code comment argues it well, but a client checking only the HTTP status — the normal transaction contract — silently drops the failure. Worth stating in the README's limitations rather than only in a comment.
• batch.rs:1854 — return=minimal omits entry.resource, which now strips searchset bodies. Pre-existing for instance reads, but writes + searches in one bundle is exactly the case where a client sends return=minimal.
• The search-param registry read lock is re-acquired per entry inside the transaction pre-validation loop; it could be hoisted.
• Untested edge: a transaction whose entries are all searches passes an empty entry list to execute_bundle after the partition. No test covers it.

---

Process

Two scope observations, offered as a question rather than a request:

1. The PR body is admirably honest that this branch carries #481 — but a PR titled fix(rest): render batch entry failures through the single-resource error mapping containing a +393/-16 behavioral feature, which is also where the one blocking finding lives, is hard to review as a unit. That the search commit's two _-discard sites became compile errors is a nice validation of the refactor, but it doesn't require shipping both together. Would splitting #481 back out be feasible?
2. Commit 55f6fd3a1 "style: rustfmt" reformats crates/subscriptions/* and crates/ui/*, unrelated to either PR. Those files already pass rustfmt 1.9.0-stable on main, so this looks like output from a different rustfmt version — worth pinning down before it churns back the other way.