Skip to content

effects: read a paged FHIR or REST record list whole, or not at all - #1538

Merged
abrichr merged 3 commits into
mainfrom
claude/fhir-rest-paging
Oct 25, 2026
Merged

abrichr merged 3 commits into
mainfrom
claude/fhir-rest-paging

Conversation

@abrichr

@abrichr abrichr commented Oct 25, 2026 •

Copy link
Copy Markdown
Member

The FHIR record check read only the first page of a search, so a record on page two read as absent. This PR takes the pagination engine fix out of #1520 on its own.

Change

  • FHIR (runtime/effects/fhir.py): follows the Bundle's next links on the same origin and checks total.
  • REST (runtime/effects/rest.py): follows the next page's URL at records_next_path (a dotted path in the body, such as links.next) when it is set. A relative link resolves against the page it came from.
  • Fail closed: the read is unreadable (INDETERMINATE), never a confirmation or a refutation from the first pages, when:
    • a next page is on another origin;
    • a page fails;
    • the chain runs past 50 pages;
    • a FHIR total differs from the number of matches read (included resources don't count);
    • with records_total_path (REST), the first page's total differs from the records read, or is missing;
    • a response landed on another origin. A requests session doesn't follow redirects, so the configured auth headers never reach another host.
  • Why it's unreadable is in the verdict's reason and detail.read_problem, for example next link origin https://localhost:9300 != base_url http://localhost:9520.
  • OpenEMR builds REST next links from its configured site address. Read through a mapped port, the read is unreadable with that reason; EFFECT_KIT.md says to set base_url to the site address.
  • Config (deployment.py): records_next_path and records_total_path on effects:. When they are unset they drop out of the serialized config and the verifier identity, so existing digests hold. Without it, one REST response is the whole read, as before.
  • Docs: EFFECT_KIT.md configuration reference, a CAPABILITIES.md row, and llms-full.txt.

This changes when VERIFIED is issued: on a server that pages, a record past page one now confirms instead of reading as absent, and a partial read is INDETERMINATE.

Credit

FHIR/REST pagination also proposed by @yunusarda-sudo in #1511 (superseded by this PR). The commit carries their Co-authored-by trailer.

Tests

tests/test_effect_paging.py runs the FHIR and REST verifiers against local servers:

  • FHIR reads every page; one page without a next link is the whole read.
  • FHIR: a next link to another origin, a failed later page, a total above the read, or an endless chain is unreadable.
  • REST follows absolute and relative links, and reads one response without records_next_path.
  • REST: another origin, a failed later page, a non-text link, or an endless chain is unreadable.
  • records_next_path through EffectsConfig and build_effect_verifier; an unset value doesn't change the serialized config.
  • Review fixes: FHIR total below the read; included resources; the named origin; OpenEMR-style site-address next links; records_total_path equal, above, below and missing; a 302 that never reaches the other host (REST with an API key header, and FHIR); the reason in the verdict.

Run: test_effect_paging, test_effect_fhir, test_rest_verifier_transport, test_smart_backend_auth, test_effect_kit_config, test_effect_collateral_change_hashes, test_effect_verifier_transport, test_effect_verifier, test_cli_deployment and test_effect_fault_matrix. 159 passed, 1 skipped.

Not run against live OpenEMR FHIR; that needs the shared fixture slot (test_effect_fhir_live_openemr.py).

🤖 Generated with Claude Code

The FHIR verifier read only the first page of a search, so a record on page
two read as absent. It now follows the Bundle's next links on the same
origin and checks total. The REST verifier follows the next page's URL at
records_next_path when it is set (relative links resolve against the page).
A next page on another origin, a failed page, more than 50 pages, or a total
above what was read makes the read unreadable, never a confirmation or a
refutation from the first pages. Unset, records_next_path drops out of the
config so existing digests hold.

FHIR/REST pagination also proposed by @yunusarda-sudo in #1511 (superseded
by this change).

Co-authored-by: yunusarda-sudo <yunusarda-sudo@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@abrichr abrichr left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coordinator safety review (AI-assisted). Approved, with small fixes. Same-origin next links, the page cap, failed pages and the co-author trailer all check out.

  1. FHIR total check: make it total != len(out), not one-sided.
  2. REST total: add an optional records_total_path, or document that REST paging trusts the chain.
  3. Redirects: use allow_redirects=False, or re-check the origin, so custom auth headers can't follow a redirect to another host.
  4. OpenEMR REST next links: document the case where they name the configured site address, with a test and a distinct error ("next link origin X != base_url Y").
  5. CAPABILITIES row: add the PR number.

Review of #1538:
- FHIR: a total other than the number of matches read (included resources
  don't count) is unreadable, not only a total above it.
- REST: records_total_path checks the record count against a total in the
  first page; without it a REST read trusts the page chain (documented).
- A requests session doesn't follow redirects, so the configured auth
  headers never reach another host, and a response that landed on another
  origin is unreadable.
- Each unreadable read says why (verdict reason, detail.read_problem), such
  as "next link origin X != base_url Y". OpenEMR builds REST next links from
  its site address; EFFECT_KIT.md says to set base_url to it, with a test.
- The CAPABILITIES row names this PR.

Co-authored-by: yunusarda-sudo <yunusarda-sudo@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@abrichr

abrichr commented Oct 25, 2026

Copy link
Copy Markdown
Member Author

AI re-review (coordinator). All five items fixed. (1) FHIR checks total != matches and excludes include/outcome entries; the total-below test was added. (2) Optional records_total_path for REST, missing or wrong total is unreadable (parametrized test), and EFFECT_KIT says REST trusts the chain without it. (3) allow_redirects=False for requests sessions, plus a landed-origin re-check on every GET. (4) Distinct error next link origin X != base_url Y, OpenEMR site-address case documented and tested. (5) The CAPABILITIES row names #1538. Nit, not blocking: the module docstring of test_effect_paging.py still says "a total above what was read". Queuing for auto-merge.

@abrichr
abrichr enabled auto-merge October 25, 2026 05:28
@abrichr
abrichr merged commit 40a8387 into main Oct 25, 2026
40 checks passed
@abrichr
abrichr deleted the claude/fhir-rest-paging branch October 25, 2026 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant