Skip to content

fix(compiler): stop a parameter's demonstrated value becoming a pixel invariant - #285

Merged
abrichr merged 2 commits into
mainfrom
fix/postcondition-param-leakage
Jul 27, 2026
Merged

fix(compiler): stop a parameter's demonstrated value becoming a pixel invariant#285
abrichr merged 2 commits into
mainfrom
fix/postcondition-param-leakage

Conversation

@abrichr

@abrichr abrichr commented Jul 27, 2026

Copy link
Copy Markdown
Member

The defect

lint_param_leakage screens text postconditions. include_region_stable=False screens the parameterized TYPE step whose own diff is the typed glyphs. Nothing screened a downstream CLICK whose changed region renders that parameter — so a REGION_STABLE postcondition could silently encode run-specific data as if it were a stable property of the application.

The cost is asymmetric and badly diagnosable: the bundle false-halts on every run whose parameter value differs, and each run's verdict is decided by the glyphs of that run's value.

Independent confirmation of the root cause

This is the defect behind the clean-baseline over-halt at step_010 Save Encounter, region (0, 228, 696, 223), in the published Flow 1.24.0 evidence. Compiling the bundled MockMed triage demo on unmodified main reproduces that region exactly:

step_010 click "click 'Save Encounter'"
     region_stable (0, 228, 696, 223)

and the after-frame OCR inside it is:

(24, 257, 113, 23) 'Encounters'
(25, 291, 305, 17) '·Triage—E2Etriagebooking threemonths'   <- the `note` param

It is not an engine regression. MockMed ships inside the Flow wheel, and c416b7d added a #patient-banner to renderEncounter(). The New Encounter form previously opened with <h1>New Encounter</h1>, so the save click's diff covered the top band; with the banner, that band is identical before and after the click and _largest_changed_region moves down onto the saved-encounter row. The published methodology claim is corrected in OpenAdaptAI/openadapt-evals#273.

The fix, and why it fails safe

In _postconditions:

  1. Screen_param_text_in_region tests the mined region against the run's parameter values, over the region's OCR lines joined (OCR splits a rendered value across lines, so no single fragment need carry it). Detection is forward-only coverage, deliberately asymmetric to _contains_excluded: that predicate also accepts value contains text, and an application's own literal is frequently a substring of a longer demonstrated value — MockMed's Triage inside the note E2E triage booking three months. Screening with the symmetric predicate disarmed step_006 and step_007, which are perfectly stable. Over-suppression is a real failure mode here and is tested.

  2. Narrow before dropping_param_free_band carves the value's row out and keeps the tallest remaining slice, preferring the slice with the most readable text (a slice holding the destination screen's heading discriminates; a taller slice of blank page matches any blank screen). For the MockMed save this turns (0, 228, 696, 223) into (0, 228, 696, 57) — the Encounters heading, no note.

    This step is load-bearing for safety, not polish. Dropping outright measurably breaks a real gate: tests/e2e/test_fault_model.py::test_fault_outcome[timeout-FALSE-ABORT] — commit-then-hang, where the client aborts and the app never leaves the form — flips to SUCCESS, because the surviving text_present 'Encounters' matches the New Encounter / Save Encounter labels on the wrong screen. With narrowing, all 27 fault-model e2e cases pass unchanged.

  3. Drop only as a last resort — when no usable slice survives. That is the fail-safe direction: an unverified step costs one missing check; a contaminated one false-halts every differing run.

Reported, never silent

A bundle that quietly lost a postcondition is undiagnosable. compile_recording now logs each drop and writes param_hygiene.json:

{
  "note": "REGION_STABLE postconditions dropped because the changed region rendered a demonstrated parameter value. ...",
  "dropped_region_stable": [
    {"step_id": "step_010", "region": [0, 228, 696, 223], "matched_text": "En****...**hs"}
  ]
}

Matched text is masked (a parameter value can be PHI), and a clean recompile removes a stale sidecar. It is not fatal — dropping is the correct outcome, not a compilation error, so it is a notice next to the lint rather than part of it. lint_param_leakage gains a scope note explaining that it can only see the text encoding of this class and pointing at where the pixel encoding is handled.

No safety gate is weakened. Nothing is made more permissive: a contaminated region is narrowed to a param-free subset or removed, and the fault-model, drift-oracle, policy, and postcondition-fuzz suites all pass unchanged.

Regression tests, proven against baseline

TestParamRegionHygiene (7) and tests/test_effect_verifier_transport.py (7). Run against a detached origin/main checkout (PYTHONPATH forced at the baseline worktree, same interpreter):

tests/test_compiler.py -k ParamRegionHygiene           4 failed, 3 passed
tests/test_effect_verifier_transport.py                5 failed, 2 passed

All 14 pass on this branch. The 5 that pass on both are the deliberate no-over-suppression guards — they assert behavior that must not change.

The end-to-end case is the real one: compile the synthetic recording, assert the save step carries no note-rendering REGION_STABLE, and assert param_hygiene.json names the step without reprinting the value.

Also: fhir.py and graphql.py carried the #281 requests bug

requests is a dev dependency (plus a windows extra); httpx is core. Verified by building the wheel and installing it into a venv holding only its declared dependencies, then exercising each verifier against a stdlib HTTP server:

=== BASELINE (origin/main) wheel ===        === THIS BRANCH ===
ok: requests is absent                      ok: requests is absent
rest     reachable=True   <- fixed in #281  rest     reachable=True
fhir     reachable=False                    fhir     reachable=True
graphql  reachable=False                    graphql  reachable=True

reachable=False means every verify raised, was read as unreadable, and HALTed — fail-safe, but effects.kind: fhir / graphql were unusable out of the box. Both now fall back to httpx. FHIR additionally passes TLS verification per request, which httpx binds at client construction instead; the fallback binds it there and omits the per-request kwarg (pinned by a test, since passing it would raise inside the blanket except and read as an unreadable system of record).

Not fixed here, reported: RestActuator (runtime/actuators/api.py) and MockMedCompensator (runtime/effects/compensation.py) carry the same assumption. api.py classifies delivery uncertainty off requests.exceptions.ConnectionError / ConnectTimeout, so its fallback needs its own exception mapping and safety review — not a drive-by change in this PR.

Scope

Verification

  • uvx ruff==0.15.22 check openadapt_flow — clean; ruff format --check openadapt_flow tests — 434 formatted
  • mypy (whole-package) — clean; strict safety-path mypy on the four touched modules — clean
  • scripts/check_consistency.py — passes
  • tests/e2e/test_fault_model.py — 27 passed
  • tests/test_compiler.py tests/test_postcondition_fuzz.py tests/test_field_label_params.py tests/test_secret_params.py tests/test_drift_oracle.py — 96 passed
  • effects/rest/fhir/graphql selection — 410 passed, 10 skipped

🤖 Generated with Claude Code

https://claude.ai/code/session_01NyCHrzA1psrKMFfroYbzaM

… invariant

`lint_param_leakage` screens TEXT postconditions, and `include_region_stable`
screens the parameterized TYPE step whose own diff is the typed glyphs. Nothing
screened a LATER step whose changed region RENDERS that parameter, so a
postcondition could silently encode run-specific data as if it were a stable
property of the application -- a false halt on every run whose value differs,
decided by the glyphs of that run's value, and very hard to diagnose.

This is the defect behind the `clean` 2/3 over-halt on `step_010 Save
Encounter`, region `(0, 228, 696, 223)`, in the published Flow 1.24.0 evidence.
Reproduced exactly: compiling the bundled MockMed triage demo on `main` mines
that same region, and it contains `Encounter saved - <note>` plus the new
`Triage - <note>` row. It is not an engine regression -- MockMed ships inside
the wheel and `c416b7d` added a patient banner to the New Encounter form, which
made the previously-mined top band identical across the click and pushed the
search down onto the note. openadapt-evals#273 corrects the published
methodology claim.

What the fix does, in `_postconditions`:

1. `_param_text_in_region` screens the mined region against the run's
   parameter values, over the region's OCR lines JOINED (OCR splits a rendered
   value across lines). Detection is forward-only `coverage`, deliberately
   asymmetric to `_contains_excluded`: an app's own literal is often a
   SUBSTRING of a longer value (MockMed's `Triage` inside the note `E2E triage
   booking three months`) and must not disqualify a stable region. Screening
   with the symmetric predicate disarmed `step_006`/`step_007` for exactly that
   reason.
2. A contaminated region is NARROWED, not immediately dropped
   (`_param_free_band`): the value's row is carved out and the tallest
   remaining slice is kept, preferring the slice with the most readable text
   (the destination screen's heading discriminates; a taller slice of blank
   page matches any blank screen). Dropping outright would have removed the
   step's only screen-state gate -- `tests/e2e/test_fault_model.py`'s
   commit-then-timeout case stops being detected. All 27 fault-model e2e cases
   pass unchanged.
3. Only when no usable slice survives is the postcondition dropped -- the
   fail-safe direction: an unverified step costs one missing check, a
   contaminated one false-halts every differing run.

Reported, not silent: `compile_recording` logs each drop and writes
`param_hygiene.json` (step id, region, MASKED matched text -- a parameter value
can be PHI), removing a stale sidecar on a clean recompile. Not fatal; dropping
is the correct outcome, not a compilation error. `lint_param_leakage` gains a
scope note pointing at it.

Also, the same class as #281 in two more verifiers: `FhirEffectVerifier` and
`GraphQLRecordVerifier` imported `requests`, a dev/`windows`-extra dependency.
Verified in a clean venv holding only the wheel's declared dependencies: on
`main` both report `reachable=False` -- every verify raised, was read as
*unreadable*, and HALTed. Fail-safe but unusable out of the box. Both now fall
back to `httpx` (already core). FHIR additionally passes TLS verification per
request, which `httpx` binds at client construction instead, so the fallback
binds it there and omits the per-request kwarg.

`RestActuator` (`runtime/actuators/api.py`) and `MockMedCompensator`
(`runtime/effects/compensation.py`) carry the same assumption and are NOT fixed
here: `api.py` classifies delivery uncertainty off `requests.exceptions`, so its
fallback needs its own exception mapping and safety review rather than a
drive-by change.

Tests: `TestParamRegionHygiene` (7 cases) and `tests/test_effect_verifier_transport.py`
(7 cases). Verified against a detached `origin/main` checkout: 4 of the 7 region
cases and 5 of the 7 transport cases fail there and pass here. The 5 that pass
on both are deliberate no-over-suppression guards.

No published benchmark number changes: this changes future compiles only.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NyCHrzA1psrKMFfroYbzaM
@abrichr
abrichr force-pushed the fix/postcondition-param-leakage branch from 4c2ec34 to 60a011a Compare July 27, 2026 18:04
@abrichr

abrichr commented Jul 27, 2026

Copy link
Copy Markdown
Member Author

Independent exact-head review found one pre-merge artifact-hygiene issue at 60a011a: recompiling into an existing bundle after parameter hygiene drops REGION_STABLE can leave the obsolete templates/<step>_expect.png. Workflow.save inventories/hashes all template files, so the unreferenced orphan remains in the sealed manifest and makes the bundle digest depend on output-directory history. Reproduction: stale file exists, manifest includes it, while the compiled step has no expect postcondition. Please remove the obsolete expected template on that transition and add a fresh-vs-reused output determinism regression. No runtime false-success was observed. Nonblocking follow-up: expose param_hygiene.json in the qualification/console JSON inventory.

@abrichr
abrichr marked this pull request as draft July 27, 2026 18:30
@abrichr

abrichr commented Jul 27, 2026

Copy link
Copy Markdown
Member Author

Focused fix is prepared locally at e03d19771426 (parent exact head 60a011a): removes stale plaintext/encrypted expected-template forms before remine and proves fresh-vs-reused manifest hash equality. Compiler/postcondition suites, Ruff, mypy, and diff checks pass. Marking draft only to prevent merge of the known artifact-history defect while current main/receipt hardening settles; the patch will be rebased, pushed, re-reviewed, and returned to ready.

@abrichr
abrichr merged commit c068554 into main Jul 27, 2026
17 checks passed
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