Skip to content

Compare typed FHIR references by target identity (partial #1855) - #1952

Draft
abrichr wants to merge 6 commits into
mainfrom
agent/1855
Draft

abrichr wants to merge 6 commits into
mainfrom
agent/1855

Conversation

@abrichr

@abrichr abrichr commented Nov 5, 2026 •

Copy link
Copy Markdown
Member

Refs #1855.

Treat version-specific FHIR references as the same logical target when finding existing records, while keeping the requested version separate for read-back checks. Send local references in canonical Type/id search form and refuse a search that omits a freshly known matching record. Typed presentation fields still cannot identify a target on their own.

Validation: 389 passed, one opt-in live-service test skipped; package static checks and generality, release and source-boundary guards passed. The final formatting-only commit preserves module ASTs and all test assertions. Regression controls cover historical/current identity, canonical query forms, wrong-target display values, missing known matches and unreadable nested references.

A pinned HAPI FHIR R4 run passed five native identifying-search assertions and one wrong-target assertion. Four query-only observations accepted both absolute and relative forms. Its duplicate control failed closed: after the duplicate was saved and independently read, the native search omitted it, so the guard returned unreadable before the pure judge. That run does not establish a successful native duplicate-judge control or full governed workflow qualification.

Coverage here is scalar Observation.subject and nested CareTeam references. Broader Reference paths, SQL relationship metadata, OpenAPI identities and full real-application qualification remain separate work. This draft does not close #1855.

@abrichr

abrichr commented Nov 5, 2026

Copy link
Copy Markdown
Member Author

Independent adversarial review: BLOCK

Head reviewed: 9382ae1fe81bd9341e0f6d92a3623422dd644633. This PR is partial scope for #1855 (FHIR only; SQL label membership and OpenAPI are not implemented). I judged it on that scope.

What I ran

  • pytest tests/test_fhir_reference.py tests/test_generate_ensure_contract.py tests/test_shadow_fhir.py tests/test_ensure_record*.py in the flow package's locked environment: 281 passed.
  • ruff check and ruff format --check on every changed file: clean. Package-wide ruff check reports 103 errors in files this PR doesn't touch.
  • gh pr checks 1952: ci, packages-gate and run-ci fail on the draft gate, as expected. Everything else is skipped.
  • Probes that drive find_matches against the PR's own Store fake. I didn't run a real FHIR server.

Findings

P1: a version-specific reference to the same resource can hide an existing record from duplicate detection. fhir_reference.py:210-212 keeps Patient/a/_history/1 as a separate identity from Patient/a, and same() (ensure_record.py:557) compares identities as exact strings. That strictness is right in wrong_fields, where a mismatch fails closed. In is_person it fails open, because _find_matches (ensure_record.py:1293) uses is_person to decide whether the person's record already exists. Probe: the search returns an Observation whose subject.reference is Patient/a/_history/1, the task gives Patient/a, and the result is ({}, 1). The record is seen but not counted. The baseline then shows no existing record, the agent creates a second one, and judge can return created_verified. The reverse case fails the same way (versioned task value, unversioned stored value; the search sends .../Patient/a/_history/1). Fix options: compare the version-stripped logical identity when matching, or make a version-only difference raise ReferenceUnreadable so the verdict becomes needs_review. Either needs a test that drives find_matches. The raw string comparison on main misses the same case. I'm blocking anyway because this PR defines target identity, and version-specific references are an explicit case it claims to handle.

P2: the identity search now sends an absolute URL without validation. query_value (ensure_record.py:1262) sends the resolved identity, for example patient=https://.../fhir/R4/Patient/a, where main sent the task's literal Patient/a. Some servers, such as HAPI unless the base is configured as local, treat an absolute reference as external and return no rows. No rows would again hide an existing record. No test drives find_matches with a typed reference, and no real FHIR run has happened (the PR body says so). Please add a fake-based query assertion and run at least one real HAPI control. Sending the relative Type/id form also avoids the problem.

P3: a closed contract accepts a display that names someone else. _expected_leaves (ensure_record.py:~779) always allows display, type and identifier.* on a typed Reference. That follows the ruling that display never decides identity. I'm noting it only so nobody reads created_verified as "the display was checked".

P3: the late recheck can raise. In shadow.py:822, is_person can raise ReferenceUnreadable for a multi-valued Reference path, because the wrong_fields pre-resolution checks only the first value. The outer handler (shadow.py:694) turns that into UNCERTAIN, so the result fails closed. The cost is a noisier reason string.

Checked and found sound

  • Display text decides no identity. reference_value drops display before it builds the key, an identity rule can't name a display field, and a display parameter without a bound target refuses.
  • Unresolvable targets fail closed: missing, deleted (404 or 410), wrong type, a read that returns a different id or version, references outside the base or carrying credentials, queries or fragments, contained #x references, and identifier searches that return zero or several matches.
  • Relative and absolute references to the same target agree. A literal reference and an identifier that point to different targets refuse.
  • The merge keeps ensure_record: bind the read-back to the last version written and name version authors (#1854 a+b) #1873 intact. read_back_unbound and the version checks are unchanged, and the interaction tests cover a reference in every corrected version and the last-written version.
  • The code is generic. Reference typing comes from a hash-pinned StructureDefinition, and nothing is app-specific.

This branch has not been deployed

No deployments
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.

Verifier: compare references by identity, never by display text

1 participant