From 9044c5b079dca1c9718e23d105b4b906b2a16731 Mon Sep 17 00:00:00 2001 From: abrichr Date: Mon, 19 Oct 2026 14:16:00 -0400 Subject: [PATCH] fix(flow): refuse an API step whose span also wrote Review of #1269: an earlier POST inside the span (for example /reserve.php) was only listed in other_writes_seen, and the API step still replaced the whole span, so a run skipped that write. A proposal is refused when any POST, PUT, PATCH or DELETE other than the save was sent while the replaced steps ran. Admission refuses such a binding, and a binding admitted before this rule runs on the GUI with the reason. On the OpenEMR referral this refuses the API step: OpenEMR's background POST /library/ajax/execute_background_services.php lands inside the span. Co-Authored-By: Claude Opus 5.5 --- packages/flow/docs/API_TIER.md | 4 + .../openadapt_flow/compiler/api_proposal.py | 17 ++++ packages/flow/openadapt_flow/ir.py | 5 +- .../flow/openadapt_flow/runtime/replayer.py | 7 ++ .../tests/test_api_proposal_other_writes.py | 98 +++++++++++++++++++ 5 files changed, 129 insertions(+), 2 deletions(-) create mode 100644 packages/flow/tests/test_api_proposal_other_writes.py diff --git a/packages/flow/docs/API_TIER.md b/packages/flow/docs/API_TIER.md index 06185a23a..a1f2b5e66 100644 --- a/packages/flow/docs/API_TIER.md +++ b/packages/flow/docs/API_TIER.md @@ -78,6 +78,10 @@ doesn't propose an API step in these cases: - the save sends a secret field; - the form page wasn't captured; - the save isn't the program's last step; +- the steps the API step would replace also sent another write (a POST, + PUT, PATCH or DELETE other than the save, such as a reservation, an + autosave or a background call). Admission and runs refuse such a binding + too; - the program has branches; - a URL value equals two inputs' values (ambiguous), or the save or a pre-check is on another origin than the form; diff --git a/packages/flow/openadapt_flow/compiler/api_proposal.py b/packages/flow/openadapt_flow/compiler/api_proposal.py index 704d5fdb4..1d50a4c5d 100644 --- a/packages/flow/openadapt_flow/compiler/api_proposal.py +++ b/packages/flow/openadapt_flow/compiler/api_proposal.py @@ -564,6 +564,18 @@ def carried(line: dict[str, Any]) -> int: and form_t < float(ln["t"]) < float(post["t"]) } ) + if others: + # The steps the API step would replace also wrote (a reservation, an + # autosave, a background call): the one request can't stand in for + # them, and skipping them would change what the program does. + return ( + workflow, + None, + [ + "the steps the API step would replace also sent " + f"{', '.join(others)}; one request can't stand in for them" + ], + ) spec = WebFormSpec( form_path=form_path, fields=fields, @@ -681,6 +693,11 @@ def admit_api_step( and b.kind == "web_form" and (step_id is None or step.id == step_id) ): + if b.web_form is not None and b.web_form.other_writes_seen: + raise ApiProposalError( + f"{step.id}: the steps the API step would replace also sent " + f"{', '.join(b.web_form.other_writes_seen)}; it can't be admitted" + ) if not step.effects: raise ApiProposalError( f"{step.id} has no record check; an API step must be confirmed by one " diff --git a/packages/flow/openadapt_flow/ir.py b/packages/flow/openadapt_flow/ir.py index 1dcce099a..ddae6a1ca 100644 --- a/packages/flow/openadapt_flow/ir.py +++ b/packages/flow/openadapt_flow/ir.py @@ -605,8 +605,9 @@ class WebFormSpec(BaseModel): success_pattern: Optional[str] = None #: GUI steps the API step stands in for, in order. The last one is the save. replaces_steps: list[str] = Field(min_length=1) - #: Other non-GET requests seen while the recorded steps ran. They are not - #: repeated; they are listed so the person admitting the step can judge. + #: Other non-GET requests seen while the recorded steps ran. A proposal + #: with any is refused, and so are its admission and its run (a binding + #: proposed before that rule existed may still list some). other_writes_seen: list[str] = Field(default_factory=list) #: On-screen checks of folded steps that caused no request (screen-only, #: not repeated), listed for the person admitting the step. diff --git a/packages/flow/openadapt_flow/runtime/replayer.py b/packages/flow/openadapt_flow/runtime/replayer.py index 6dcfdf5fe..7f26df4fc 100644 --- a/packages/flow/openadapt_flow/runtime/replayer.py +++ b/packages/flow/openadapt_flow/runtime/replayer.py @@ -2421,6 +2421,13 @@ def _web_form_span_at( reason = "a governed run uses its qualified GUI path" elif not binding.admitted(): reason = "the API step is not admitted (openadapt-flow api admit)" + elif binding.web_form.other_writes_seen: + # Proposed before such spans were refused: the steps it would + # replace also wrote, and one request can't stand in for them. + reason = ( + "the steps the API step would replace also sent other writes " + f"({', '.join(binding.web_form.other_writes_seen)})" + ) elif list(binding.effects) != list(save.effects): reason = ( "the save step's record check differs from the one admitted " diff --git a/packages/flow/tests/test_api_proposal_other_writes.py b/packages/flow/tests/test_api_proposal_other_writes.py new file mode 100644 index 000000000..41036e78f --- /dev/null +++ b/packages/flow/tests/test_api_proposal_other_writes.py @@ -0,0 +1,98 @@ +"""An API step never replaces steps that also wrote. + +The reviewer's case for #1269: an earlier POST (/reserve.php) inside the span +was only listed in other_writes_seen, and the API step still replaced the +whole span, so a run skipped the reservation. Synthetic values only. +""" + +from __future__ import annotations + +import json + +import pytest + +from openadapt_flow import network_capture as nc +from openadapt_flow.compiler.api_proposal import ( + ApiProposalError, + admit_api_step, + propose_api_step, +) +from openadapt_flow.runtime.replayer import Replayer +from tests.test_api_tier_web_form import ( + _recording, + _span_workflow, + _workflow, +) +from tests.test_replayer import FakeBackend, FakeVision + + +def _with_write(rec, *, t=4.5, method="POST", url="http://app/app/reserve.php"): + rows = [json.loads(x) for x in (rec / nc.NETWORK_FILE).read_text().splitlines()] + rows.append( + {"t": t, "method": method, "url": url, "status": 200, "resource_type": "fetch"} + ) + (rec / nc.NETWORK_FILE).write_text("".join(json.dumps(r) + "\n" for r in rows)) + return rec + + +@pytest.mark.parametrize("method", ["POST", "PUT", "DELETE", "PATCH"]) +def test_a_write_inside_the_span_refuses_the_proposal(tmp_path, method): + rec = _with_write(_recording(tmp_path), method=method) + wf_in = _workflow() + wf, step, reasons = propose_api_step(wf_in, rec) + assert step is None and wf == wf_in + assert f"{method} /app/reserve.php" in " ".join(reasons) + + +def test_a_read_inside_the_span_is_still_fine(tmp_path): + rec = _with_write(_recording(tmp_path), method="GET") + _wf, step, reasons = propose_api_step(_workflow(), rec) + assert step is not None, reasons + + +def _old_binding(): + """A binding proposed before such spans were refused.""" + + wf = _span_workflow(admitted=False) + save = wf.steps[-1] + spec = save.api_binding.web_form.model_copy( + update={"other_writes_seen": ["POST /reserve.php"]} + ) + binding = save.api_binding.model_copy(update={"web_form": spec}) + return wf.model_copy( + update={ + "steps": [*wf.steps[:-1], save.model_copy(update={"api_binding": binding})] + } + ) + + +def test_such_a_binding_can_not_be_admitted(): + with pytest.raises(ApiProposalError, match="POST /reserve.php"): + admit_api_step(_old_binding(), admitted_by="reviewer") + + +def test_an_admitted_older_binding_runs_on_the_gui(): + from openadapt_flow.ir import ApiAdmission + + # Admitted before such spans were refused: its admission still matches. + wf = _old_binding() + save = wf.steps[-1] + clean = save.api_binding.model_copy(update={"admission": None}) + binding = clean.model_copy( + update={ + "admission": ApiAdmission( + admitted_by="reviewer", + admitted_at="2026-10-18T00:00:00Z", + spec_digest=clean.admission_digest(), + ) + } + ) + assert binding.admitted() + wf = wf.model_copy( + update={ + "steps": [*wf.steps[:-1], save.model_copy(update={"api_binding": binding})] + } + ) + replayer = Replayer(FakeBackend(), vision=FakeVision(), web_form_actuator=object()) + _end, _save, why = replayer._web_form_span_at(wf, 0, durable=False) + assert why is not None and "POST /reserve.php" in why