diff --git a/packages/flow/docs/API_TIER.md b/packages/flow/docs/API_TIER.md index 152d04921..bad6d2615 100644 --- a/packages/flow/docs/API_TIER.md +++ b/packages/flow/docs/API_TIER.md @@ -1,9 +1,14 @@ # API steps from a recording's network traffic -A web form save that OpenAdapt can repeat as one HTTP request runs as one -request instead of as the clicks and keystrokes that filled the form. The -right-record check before the save and the record check after it stay the -same. If anything the API step depends on has changed, the recorded GUI +**Scope: a single HTML form submission.** An API step repeats one +`application/x-www-form-urlencoded` POST of one HTML form, read again from +the live page, in place of the clicks and keystrokes that filled it. It is +not a general API client: JSON or script-built requests, more than one +write, multi-page server state, and forms whose script validates or rewrites +values all stay on the GUI. + +The right-record check before the save and the record check after it stay +the same. If anything the API step depends on has changed, the recorded GUI steps run instead. Its speed is measured on the local OpenEMR referral as arm `program_api` of @@ -40,9 +45,11 @@ openadapt-flow run bundle/ ... first of those steps carries `api_fallback` with the reason, and the GUI steps run as recorded. -If you change a check on one of the covered steps after compiling (for -example, you add a required on-screen text), the API step stops running -because it would skip that check. Run `openadapt-flow api propose bundle/ +If you change any of the covered steps after compiling (an action, a typed +value or parameter, a target or its identity evidence, a guard, an on-screen +check, or the save's record check), the API step stops running: its +admission is bound to a digest of the complete definition of every step it +replaces, the save included. Run `openadapt-flow api propose bundle/ --recording rec/` to propose it again, then admit it. ## What the compiler checks diff --git a/packages/flow/openadapt_flow/compiler/api_proposal.py b/packages/flow/openadapt_flow/compiler/api_proposal.py index 622802c85..4ba38dbad 100644 --- a/packages/flow/openadapt_flow/compiler/api_proposal.py +++ b/packages/flow/openadapt_flow/compiler/api_proposal.py @@ -613,7 +613,7 @@ def carried(line: dict[str, Any]) -> int: replaces_steps=[s.id for s in folded], other_writes_seen=others, screen_only_checks=screen_only, - folded_checks_digest=folded_checks_digest(folded[:-1]), + folded_checks_digest=folded_checks_digest(folded), ) binding = ApiBinding( kind="web_form", diff --git a/packages/flow/openadapt_flow/ir.py b/packages/flow/openadapt_flow/ir.py index 971898c99..61e3a63f5 100644 --- a/packages/flow/openadapt_flow/ir.py +++ b/packages/flow/openadapt_flow/ir.py @@ -612,9 +612,12 @@ class WebFormSpec(BaseModel): #: On-screen checks of folded steps that caused no request (screen-only, #: not repeated), listed for the person admitting the step. screen_only_checks: list[str] = Field(default_factory=list) - #: Digest of the folded steps' on-screen checks when this was proposed. - #: If a check is added or changed later, the API step does not run (it - #: would skip that check) until it is proposed again. + #: Digest of the complete definition of every replaced step, the save + #: included (minus its API binding), when this was proposed + #: (``folded_checks_digest`` in runtime.actuators.web_form). Any change to + #: those steps keeps the API step from running until it is proposed and + #: admitted again. Bindings proposed before it covered more than the + #: on-screen checks need proposing again. folded_checks_digest: str = Field(pattern=r"^sha256:[0-9a-f]{64}$") diff --git a/packages/flow/openadapt_flow/runtime/actuators/web_form.py b/packages/flow/openadapt_flow/runtime/actuators/web_form.py index 60547a231..74dced3a5 100644 --- a/packages/flow/openadapt_flow/runtime/actuators/web_form.py +++ b/packages/flow/openadapt_flow/runtime/actuators/web_form.py @@ -348,11 +348,19 @@ def option_for_label(control: dict[str, Any], label: str) -> Optional[str]: def folded_checks_digest(steps: Any) -> str: - """SHA-256 over the on-screen checks (``expect``) of the folded steps.""" + """SHA-256 over the complete definition of the steps an API step replaces. + + Every replaced step, the save included: its action and values, its + parameters, its target and identity evidence, its guards and waits, its + on-screen checks, its record check and its risk. Only the save's own API + binding is left out (it carries this digest). Any change to what the GUI + steps do or check changes the digest, and the API step doesn't run until + it is proposed and admitted again. + """ import hashlib - payload = [[s.id, [p.model_dump(mode="json") for p in s.expect]] for s in steps] + payload = [s.model_dump(mode="json", exclude={"api_binding"}) for s in steps] canonical = json.dumps(payload, sort_keys=True, separators=(",", ":")) return "sha256:" + hashlib.sha256(canonical.encode("utf-8")).hexdigest() diff --git a/packages/flow/openadapt_flow/runtime/authorization.py b/packages/flow/openadapt_flow/runtime/authorization.py index 5748c7b77..2cbd94623 100644 --- a/packages/flow/openadapt_flow/runtime/authorization.py +++ b/packages/flow/openadapt_flow/runtime/authorization.py @@ -343,7 +343,7 @@ class AdmittedApiStep(BaseModel): """One API step (``web_form`` binding) admitted into a governed run. Binds the exact binding a person admitted (``admission_digest``), the - on-screen checks of the GUI steps it stands for (``folded_checks_digest``) + complete definition of the GUI steps it stands for (``folded_checks_digest``) and its record check (``effect_contract_hashes``). The runtime runs an API step under a governed authorization only when all three still match. """ @@ -545,7 +545,7 @@ def admitted_api_step(self, step: Any, workflow: Any) -> Optional[str]: row.admission_digest != binding.admission_digest() or row.folded_checks_digest != binding.web_form.folded_checks_digest or row.folded_checks_digest - != folded_checks_digest(workflow.steps[start:end]) + != folded_checks_digest(workflow.steps[start : end + 1]) or list(row.effect_contract_hashes) != [effect.contract_hash() for effect in step.effects] ): diff --git a/packages/flow/openadapt_flow/runtime/replayer.py b/packages/flow/openadapt_flow/runtime/replayer.py index f762b353b..f7a51f711 100644 --- a/packages/flow/openadapt_flow/runtime/replayer.py +++ b/packages/flow/openadapt_flow/runtime/replayer.py @@ -2523,12 +2523,13 @@ def _web_form_span_at( elif ids != list(binding.web_form.replaces_steps): reason = "the program's steps no longer match the API step" elif ( - folded_checks_digest(workflow.steps[step_index:end_index]) + folded_checks_digest(workflow.steps[step_index : end_index + 1]) != binding.web_form.folded_checks_digest ): reason = ( - "an on-screen check of the GUI steps changed after the API " - "step was proposed, and the API step would skip it " + "the steps the API step replaces changed after it was " + "proposed (an action, value, target, check, guard or record " + "check), so the API step no longer stands for them " "(openadapt-flow api propose)" ) elif self.governed_authorization is not None and ( diff --git a/packages/flow/tests/test_api_step_durable.py b/packages/flow/tests/test_api_step_durable.py index 82b80900d..322c1c090 100644 --- a/packages/flow/tests/test_api_step_durable.py +++ b/packages/flow/tests/test_api_step_durable.py @@ -79,6 +79,28 @@ def actuate(self, binding, params): ) +def _reproposed(wf: Workflow) -> Workflow: + """The span program after an edit to its steps, its API step proposed again. + + The API step's admission binds the complete definition of the steps it + replaces, so an edited save needs the binding's digest recomputed, as + ``openadapt-flow api propose`` does, before it is admitted again. + """ + + from openadapt_flow.runtime.actuators.web_form import folded_checks_digest + + save = wf.steps[-1] + spec = save.api_binding.web_form.model_copy( + update={"folded_checks_digest": folded_checks_digest(wf.steps)} + ) + 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 _absolute_check_workflow() -> Workflow: """The span program with a record check that needs no before-snapshot. @@ -91,7 +113,7 @@ def _absolute_check_workflow() -> Workflow: effect = save.effects[0].model_copy(update={"forbid_collateral_loss": False}) binding = save.api_binding.model_copy(update={"effects": [effect]}) save = save.model_copy(update={"effects": [effect], "api_binding": binding}) - wf = wf.model_copy(update={"steps": [*wf.steps[:-1], save]}) + wf = _reproposed(wf.model_copy(update={"steps": [*wf.steps[:-1], save]})) wf, _ = admit_api_step(wf, admitted_by="reviewer") return wf @@ -330,7 +352,7 @@ def test_an_identity_checked_save_with_a_right_record_contract_runs_durably(tmp_ update={"steps": [*wf.steps[:-1], save], "params": {**wf.params, "note": "api"}} ) wf, _ = admit_api_step( - wf, admitted_by="reviewer", identity={"subject_name": "note"} + _reproposed(wf), admitted_by="reviewer", identity={"subject_name": "note"} ) fx = Fixture(tmp_path, wf) try: diff --git a/packages/flow/tests/test_api_step_governed.py b/packages/flow/tests/test_api_step_governed.py index d26917e7b..2aeb259eb 100644 --- a/packages/flow/tests/test_api_step_governed.py +++ b/packages/flow/tests/test_api_step_governed.py @@ -96,7 +96,7 @@ def _workflow(*, admitted: bool = True, identity: bool = True) -> Workflow: ], controls=["csrf", "lname"], replaces_steps=[typing.id, submit.id], - folded_checks_digest=folded_checks_digest([typing]), + folded_checks_digest=folded_checks_digest([typing, submit]), ) submit = submit.model_copy( update={ diff --git a/packages/flow/tests/test_api_step_replaced_steps_digest.py b/packages/flow/tests/test_api_step_replaced_steps_digest.py new file mode 100644 index 000000000..b80ac338e --- /dev/null +++ b/packages/flow/tests/test_api_step_replaced_steps_digest.py @@ -0,0 +1,70 @@ +"""An API step stands for the exact steps it was admitted for. + +The reviewer's case for #1269: folded_checks_digest hashed only the folded +steps' on-screen checks, so changing what a replaced step does (its action, +its value, its parameter, its target or identity, a guard, the save's record +check) left the admitted API step running in its place. +""" + +from __future__ import annotations + +import pytest + +from openadapt_flow.ir import ActionKind, Anchor, Guard, Predicate, PredicateKind +from openadapt_flow.runtime.replayer import Replayer +from tests.test_api_tier_web_form import _span_workflow +from tests.test_replayer import FakeBackend, FakeVision + + +def _edit(wf, index, **update): + steps = list(wf.steps) + steps[index] = steps[index].model_copy(update=update) + return wf.model_copy(update={"steps": steps}) + + +def _why(wf): + replayer = Replayer(FakeBackend(), vision=FakeVision(), web_form_actuator=object()) + _end, _save, why = replayer._web_form_span_at(wf, 0, durable=False) + return why + + +def test_the_admitted_steps_run_as_the_api_step(): + assert _why(_span_workflow()) is None + + +@pytest.mark.parametrize( + "index, update", + [ + (0, {"action": ActionKind.CLICK, "key": None}), + (0, {"key": "Shift+Tab"}), + (0, {"text": "{lname}", "action": ActionKind.TYPE, "key": None}), + (0, {"param": "lname"}), + ( + 0, + { + "anchor": Anchor( + template="templates/x.png", + region=(0, 0, 10, 10), + click_point=(5, 5), + context_text="Another record", + ) + }, + ), + ( + 0, + { + "guard": Guard( + predicate=Predicate( + kind=PredicateKind.PARAM_EQUALS, param="lname", value="x" + ), + on_unmet="halt", + ) + }, + ), + (1, {"key": "Ctrl+Enter"}), + (1, {"risk": "irreversible"}), + ], +) +def test_any_change_to_a_replaced_step_keeps_it_on_the_gui(index, update): + why = _why(_edit(_span_workflow(), index, **update)) + assert why is not None and "replaces changed" in why diff --git a/packages/flow/tests/test_api_tier_web_form.py b/packages/flow/tests/test_api_tier_web_form.py index c632a4c5b..c45f0f664 100644 --- a/packages/flow/tests/test_api_tier_web_form.py +++ b/packages/flow/tests/test_api_tier_web_form.py @@ -561,7 +561,7 @@ def _span_workflow(*, admitted=True) -> Workflow: WebFormPrecheck(path="/dup.php", require_text=["No matches were found."]) ], replaces_steps=["step_000", "step_001"], - folded_checks_digest=folded_checks_digest(steps[:1]), + folded_checks_digest=folded_checks_digest(steps), ) binding = ApiBinding( kind="web_form", url_template="/save.php", effects=[effect], web_form=spec @@ -666,7 +666,7 @@ def test_a_rejected_save_halts_without_gui_retry(tmp_path): } ), {}, - "on-screen check", + "replaces changed", ), ], )