diff --git a/packages/flow/benchmark/scoreboard/PROTOCOL.md b/packages/flow/benchmark/scoreboard/PROTOCOL.md index 37437f3337..7961f691eb 100644 --- a/packages/flow/benchmark/scoreboard/PROTOCOL.md +++ b/packages/flow/benchmark/scoreboard/PROTOCOL.md @@ -171,6 +171,17 @@ imports) and, for agent arms, one ledger per item with every model call's tokens and cost. Screenshots and videos are not committed. Raw data is committed before any number is quoted. +## Note on amendment numbers + +Amendments 11 to 14 were written as Amendments 10 to 13 on their branch. They +were renumbered when the branch merged after Amendment 10 (lifecycle +economics). Old Amendment N is now Amendment N+1. Files committed before the +merge keep the old numbers: +`raw/openemr_referral/code_agent/2026-10-25-rb/meta.json`, +`raw/openemr_referral/code_agent/2026-10-25-rb-admit/meta.json` and +`raw/mockmed/code_agent/2026-10-25-rb/HAND_REVIEW.md`. Their "Amendment 11" is +Amendment 12 here. + ## Amendment 1 (2026-10-18, after a code review, before any figure was quoted) Arms 1 and 3 on MockMed and arm 1 on openIMIS were running when these changes @@ -381,11 +392,6 @@ that has a REPORT.md. ## Amendment 11 (2026-10-25): arm 7 with a read-back on every save, and the program it compiles -Amendments 11 to 14 were written as Amendments 10 to 13 on their branch and -renumbered when the branch merged after Amendment 10 (lifecycle economics). -Run notes committed under `raw/` before the merge use the old numbers: their -"Amendment 11" is Amendment 12 here. - Written while arm 7 was on item 25, before any result was summed. Code as action became the default first-run agent, and run 1 now reads every save back from the system of record before the diff --git a/packages/flow/openadapt_flow/agent/browser_teach.py b/packages/flow/openadapt_flow/agent/browser_teach.py index 998520bc6e..56c94e7bcc 100644 --- a/packages/flow/openadapt_flow/agent/browser_teach.py +++ b/packages/flow/openadapt_flow/agent/browser_teach.py @@ -946,7 +946,9 @@ class BrowserTeachBridge: Duck-types :class:`~openadapt_flow.agent.teach.TeachBridge`: the MCP layer calls ``handles``, ``list_tool_specs``, ``hold_needed`` and ``dispatch``. - No action here needs a person's approval, because none can change data. + No action here needs a person's approval. Without a write contract + nothing can change data. With one, the wire check lets only the + contract's writes leave the browser. """ @property @@ -1014,7 +1016,7 @@ def handles(self, name: str) -> bool: def hold_needed( self, arguments: Mapping[str, Any], tool: str = "teach_act" ) -> None: - """The MCP layer passes the tool name (``teach_act`` or ``teach_run``).""" + """Always None: this bridge has no person holds. ``tool`` is ignored.""" return None diff --git a/packages/flow/openadapt_flow/agent/mcp.py b/packages/flow/openadapt_flow/agent/mcp.py index e84f4ccfed..1c405173dc 100644 --- a/packages/flow/openadapt_flow/agent/mcp.py +++ b/packages/flow/openadapt_flow/agent/mcp.py @@ -288,7 +288,8 @@ def _server_instructions( elif teach is not None: text += ( " To add a workflow, teach it: call teach_start with the task, then " - "teach_observe and teach_act until it is done, then teach_finish. " + "teach_observe and teach_run (web apps) or teach_act until it is " + "done, then teach_finish. " "Run 1 makes a real entry in the app; use test records. A person " "confirms the record check (teach_confirm_checks shows them a form) " "and teach_admit admits it only under the operator's signed policy. " diff --git a/packages/flow/openadapt_flow/agent/verify_tools.py b/packages/flow/openadapt_flow/agent/verify_tools.py index 2a411b123d..1ed0587d8f 100644 --- a/packages/flow/openadapt_flow/agent/verify_tools.py +++ b/packages/flow/openadapt_flow/agent/verify_tools.py @@ -128,7 +128,7 @@ def handles(self, name: str) -> bool: def hold_needed( self, arguments: Mapping[str, Any], tool: str = "teach_act" ) -> None: - """The MCP layer passes the tool name (``teach_act`` or ``teach_run``).""" + """Always None: this bridge has no person holds. ``tool`` is ignored.""" return None diff --git a/packages/flow/tests/test_mcp_teach_bridge_contract.py b/packages/flow/tests/test_mcp_teach_bridge_contract.py new file mode 100644 index 0000000000..990b9779d3 --- /dev/null +++ b/packages/flow/tests/test_mcp_teach_bridge_contract.py @@ -0,0 +1,119 @@ +"""Every teach bridge takes the MCP layer's ``hold_needed(arguments, tool=...)``. + +``build_server`` duck-types its ``teach`` bridge. When the MCP layer started +passing ``tool=``, one bridge lacked the keyword, and only an e2e test found +the TypeError. This builds each real bridge into the real server, calls its +teach tools through an in-memory MCP client, and checks that ``hold_needed`` +was called with ``tool=`` and that no call failed inside the server. +No app, browser or model is used. +""" + +from __future__ import annotations + +import inspect +import json +from pathlib import Path +from typing import Any + +import pytest +import yaml + +pytest.importorskip("mcp") + +import anyio # noqa: E402 +from mcp import ClientSession # noqa: E402 +from mcp.shared.memory import create_client_server_memory_streams # noqa: E402 + +from openadapt_flow.agent.browser_teach import BrowserTeachBridge # noqa: E402 +from openadapt_flow.agent.mcp import build_server # noqa: E402 +from openadapt_flow.agent.teach import TeachBridge, TeachConfig # noqa: E402 +from openadapt_flow.agent.verify_tools import VerifyBridge # noqa: E402 +from openadapt_flow.guard_demo import verify_yaml # noqa: E402 + +BASE = "http://127.0.0.1:9" +# The MCP layer's answer when a call raised something other than BridgeError, +# such as a TypeError from a hold_needed without ``tool``. +FAILED = "Local tool execution failed safely" +# Schema-valid arguments; each call fails cleanly because no run is going. +ARGS = { + "teach_act": {"action": "click", "ref": "e1"}, + "teach_run": {"script": "tree"}, +} + + +def _teach(tmp_path: Path) -> TeachBridge: + verify = tmp_path / "verify.yaml" + verify.write_text( + "verifier:\n kind: rest\n" + f" base_url: {BASE}\n records_path: /api/db\n records_key: records\n" + ) + return TeachBridge( + TeachConfig(verify=verify, cache_dir=tmp_path / "teach", url=f"{BASE}/") + ) + + +def _browser(tmp_path: Path) -> BrowserTeachBridge: + return BrowserTeachBridge(tmp_path / "tools", tmp_path / "files") + + +def _verify(tmp_path: Path) -> VerifyBridge: + contracts = tmp_path / "contracts" + contracts.mkdir() + (contracts / "referral.yaml").write_text(yaml.safe_dump(verify_yaml(BASE))) + return VerifyBridge(contracts, tmp_path / "state") + + +BRIDGES = {"teach": _teach, "browser_teach": _browser, "verify": _verify} + + +async def _calls(server: Any, names: list[str]) -> dict[str, dict]: + out: dict[str, dict] = {} + async with create_client_server_memory_streams() as (client, transport): + async with anyio.create_task_group() as tasks: + tasks.start_soon( + server.run, *transport, server.create_initialization_options() + ) + async with ClientSession(*client) as session: + await session.initialize() + listed = {tool.name for tool in (await session.list_tools()).tools} + for name in names: + if name not in listed: + continue + result = await session.call_tool(name, ARGS[name]) + out[name] = json.loads(result.content[0].text) + tasks.cancel_scope.cancel() + return out + + +@pytest.mark.parametrize("kind", sorted(BRIDGES)) +def test_hold_needed_takes_the_tool_name( + kind: str, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + bridge = BRIDGES[kind](tmp_path) + # The signature the MCP layer calls, for both tool names. + for tool in ARGS: + inspect.signature(bridge.hold_needed).bind({}, tool=tool) + + # Record the real method's calls; it still runs as written. + seen: list[dict[str, Any]] = [] + real = bridge.hold_needed + + def recording(arguments: Any, **kwargs: Any) -> Any: + seen.append(kwargs) + return real(arguments, **kwargs) + + monkeypatch.setattr(bridge, "hold_needed", recording) + server = build_server(teach=bridge) + results = anyio.run(_calls, server, list(ARGS)) + + for name, payload in results.items(): + assert FAILED not in str(payload.get("error")), (name, payload) + held = [name for name in ARGS if name in results and bridge.handles(name)] + assert seen == [{"tool": name} for name in held] + if kind == "teach": + # The web teach bridge offers both, and the MCP layer asks it to hold both. + assert held == ["teach_act", "teach_run"] + elif kind == "browser_teach": + assert held == ["teach_act"] and "teach_run" not in results + else: + assert results == {} diff --git a/packages/flow/tests/test_readback_volatile_fields.py b/packages/flow/tests/test_readback_volatile_fields.py new file mode 100644 index 0000000000..8934282433 --- /dev/null +++ b/packages/flow/tests/test_readback_volatile_fields.py @@ -0,0 +1,158 @@ +"""Run 1's read-back on a FHIR store whose resources change on every read. + +``RecordReadBack`` compares whole records. Some FHIR servers set a +resource's ``meta.lastUpdated`` (or ``meta.versionId``) when it is read, not +when it is written. This pins what the read-back does then, on the real +``load_observer`` path over HTTP: a field the operator's ``field_paths`` does +not map is never seen; a mapped one stops run 1 as ``wrong_record`` although +only a read changed; ``records_drop_fields`` removes it. The whole-database +snapshot (``db_snapshot``) is not a read-back source: ``load_observer`` has no +kind for it, so it is checked only by the contract frame. +""" + +from __future__ import annotations + +import itertools +import json +import threading +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer +from pathlib import Path +from typing import Any, Iterator + +import pytest +import yaml + +from openadapt_flow.compile_agent.harness import record_readback +from openadapt_flow.record_sor import RecordSorError, load_observer + +NOTE = "Synthetic follow-up in two weeks" + + +class _Store: + def __init__(self) -> None: + self.resources = [ + {"id": "1", "subject": {"reference": "Patient/8"}, "valueString": "old"} + ] + self.reads = itertools.count() + + def bundle(self) -> dict[str, Any]: + n = next(self.reads) + entries = [ + { + "resource": { + "resourceType": "Observation", + "status": "final", + # Changes on every read, as on servers that stamp it then. + "meta": { + "lastUpdated": f"2026-11-03T12:00:{n:02d}Z", + "versionId": str(n), + }, + **r, + } + } + for r in self.resources + ] + return { + "resourceType": "Bundle", + "type": "searchset", + "total": len(entries), + "entry": entries, + } + + +@pytest.fixture +def fhir() -> Iterator[tuple[str, _Store]]: + store = _Store() + + class Handler(BaseHTTPRequestHandler): + def do_GET(self) -> None: # noqa: N802 - http.server API + body = json.dumps(store.bundle()).encode() + self.send_response(200) + self.send_header("Content-Type", "application/fhir+json") + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + def log_message(self, *args: Any) -> None: + pass + + server = ThreadingHTTPServer(("127.0.0.1", 0), Handler) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + try: + yield f"http://127.0.0.1:{server.server_port}/fhir", store + finally: + server.shutdown() + server.server_close() + + +def _run1_save(tmp_path: Path, base: str, store: _Store, **extra: Any) -> str: + """One save in run 1, read back as the pipeline does; the outcome.""" + + del store.resources[1:] + verify = tmp_path / "verify.yaml" + verify.write_text( + yaml.safe_dump( + { + "verifier": { + "kind": "fhir", + "base_url": base, + "resource_type": "Observation", + "timeout_s": 2.0, + **extra, + } + } + ) + ) + observer, _ = load_observer(verify) + baseline = observer.system_of_record + readback = record_readback( + lambda: observer.system_of_record, + {"patient": "Patient/9"}, + lambda: baseline, + values={"note": NOTE}, + ) + readback.start() + readback.mark() + store.resources.append( + {"id": "2", "subject": {"reference": "Patient/9"}, "valueString": NOTE} + ) + return readback().outcome + + +def test_an_unmapped_volatile_field_is_never_seen(fhir, tmp_path: Path) -> None: + base, store = fhir + assert _run1_save(tmp_path, base, store) == "landed" + + +@pytest.mark.parametrize("path", ["meta.lastUpdated", "meta.versionId"]) +def test_a_mapped_volatile_field_stops_run_1(fhir, tmp_path: Path, path: str) -> None: + # A false stop: only a read changed the starting record. It fails closed. + base, store = fhir + paths = { + "id": "id", + "patient": "subject.reference", + "note": "valueString", + "updated": path, + } + assert _run1_save(tmp_path, base, store, field_paths=paths) == "wrong_record" + assert ( + _run1_save( + tmp_path, + base, + store, + field_paths=paths, + records_drop_fields=["updated"], + ) + == "landed" + ) + + +@pytest.mark.parametrize("kind", ["db_snapshot", "database"]) +def test_the_database_snapshot_is_not_a_readback_source( + tmp_path: Path, kind: str +) -> None: + verify = tmp_path / "verify.yaml" + verify.write_text(yaml.safe_dump({"verifier": {"kind": kind}})) + with pytest.raises(RecordSorError): + load_observer(verify)