From e40b5a9ad1954141fb75c124d73c2d3cdde80536 Mon Sep 17 00:00:00 2001 From: abrichr Date: Sat, 10 Oct 2026 00:06:11 -0400 Subject: [PATCH 1/3] fix: keep the --url authoring browser on one owner thread `openadapt-agent serve --authoring --url --headed` exited with "serve: Already running asyncio in this thread" before the MCP server started. Playwright's sync API was launched on the main thread before anyio.run, and it leaves its own event loop marked running on the thread that starts it. Even past startup, every MCP tool call ran on an anyio worker thread, and sync Playwright objects fail when called from a thread other than the one that created them. open_authoring_session now launches the --url browser, builds the Flow authoring session, serves every call, and closes the browser on one dedicated daemon thread. Callers get a ThreadOwnedSession that forwards attribute reads, method calls and attribute writes to that thread, so the mailbox Continue guard that swaps out type_text still reaches the real session. A session that fails to start now closes its browser instead of leaving it running. Tests cover the stdio serve path and the MCP server under anyio with a fake browser bound to its launch thread, the Continue guard through the owner thread, and (when Playwright Chromium is installed) a real headless browser through the MCP tools. Ledger: B076 Co-Authored-By: Claude Opus 5.5 --- src/openadapt_agent/authoring.py | 163 ++++++++++++++++- tests/test_authoring.py | 293 ++++++++++++++++++++++++++++++- tests/test_mailbox.py | 43 +++++ 3 files changed, 497 insertions(+), 2 deletions(-) diff --git a/src/openadapt_agent/authoring.py b/src/openadapt_agent/authoring.py index 02db8fa..f3ff9c7 100644 --- a/src/openadapt_agent/authoring.py +++ b/src/openadapt_agent/authoring.py @@ -20,11 +20,15 @@ from __future__ import annotations +import contextlib import json +import queue import re import sys +import threading +from concurrent.futures import Future from pathlib import Path -from typing import Any, Mapping, Optional +from typing import Any, Callable, Mapping, Optional from openadapt_agent.bridge import BridgeError, ToolSpec @@ -36,6 +40,7 @@ "MAX_AUTHORING_WIRE_BYTES", "NODE_ID_RE", "OBSERVE_SCHEMA_VERSION", + "ThreadOwnedSession", "discover_desktop_authoring_ipc", "open_authoring_session", "pin_local_backend", @@ -266,6 +271,123 @@ def halt(self) -> dict[str, Any]: return {"status": "halted"} +_OWNER_STOP = object() +_OWNER_STOP_TIMEOUT_S = 30.0 + + +class _BackendOwnerThread: + """One daemon thread that launches, drives, and closes a browser backend. + + Playwright's sync API binds its objects to the thread that started it and + leaves its own event loop marked running on that thread. Keeping the + browser on this thread lets ``anyio.run`` start on the caller's thread, + and lets MCP worker threads use the session through :meth:`call`. + """ + + def __init__(self) -> None: + self._jobs: queue.SimpleQueue = queue.SimpleQueue() + self._lock = threading.Lock() + self._stopped = False + self._thread = threading.Thread( + target=self._run, name="openadapt-authoring-backend", daemon=True + ) + self._thread.start() + + @property + def stopped(self) -> bool: + return self._stopped + + def _run(self) -> None: + while True: + job = self._jobs.get() + if job is _OWNER_STOP: + return + future, fn, args, kwargs = job + if not future.set_running_or_notify_cancel(): + continue + try: + result = fn(*args, **kwargs) + except BaseException as exc: + future.set_exception(exc) + else: + future.set_result(result) + + def call(self, fn: Callable[..., Any], /, *args: Any, **kwargs: Any) -> Any: + if threading.current_thread() is self._thread: + return fn(*args, **kwargs) + future: Future = Future() + with self._lock: + if self._stopped: + raise AuthoringError("the authoring session is closed") + self._jobs.put((future, fn, args, kwargs)) + return future.result() + + def stop(self) -> None: + with self._lock: + if self._stopped: + return + self._stopped = True + self._jobs.put(_OWNER_STOP) + if threading.current_thread() is not self._thread: + self._thread.join(timeout=_OWNER_STOP_TIMEOUT_S) + + +class _OwnedCall: + """A session method that runs on the backend owner thread.""" + + __slots__ = ("_owner", "target") + + def __init__(self, owner: _BackendOwnerThread, target: Callable[..., Any]): + self._owner = owner + self.target = target + + def __call__(self, *args: Any, **kwargs: Any) -> Any: + return self._owner.call(self.target, *args, **kwargs) + + +class ThreadOwnedSession: + """Authoring session whose every call runs on its backend owner thread. + + Attribute reads, method calls, and attribute writes (such as the mailbox + guard that swaps out ``type_text`` during Continue) all reach the wrapped + session on the thread that owns its browser. + """ + + __slots__ = ("_owner", "_session") + + def __init__(self, owner: _BackendOwnerThread, session: object): + object.__setattr__(self, "_owner", owner) + object.__setattr__(self, "_session", session) + + def __getattr__(self, name: str) -> Any: + if name in ThreadOwnedSession.__slots__: + raise AttributeError(name) + value = self._owner.call(getattr, self._session, name) + if callable(value): + return _OwnedCall(self._owner, value) + return value + + def __setattr__(self, name: str, value: Any) -> None: + if isinstance(value, _OwnedCall): + value = value.target + self._owner.call(setattr, self._session, name, value) + + def __delattr__(self, name: str) -> None: + self._owner.call(delattr, self._session, name) + + def close(self) -> None: + """Close the browser on its owner thread, then stop that thread.""" + + if self._owner.stopped: + return + try: + closer = self._owner.call(getattr, self._session, "close", None) + if callable(closer): + self._owner.call(closer) + finally: + self._owner.stop() + + def discover_desktop_authoring_ipc(*, home: Optional[Path] = None) -> Optional[dict[str, Any]]: """Return Desktop authoring IPC discovery when D2 has advertised it. @@ -427,6 +549,41 @@ def open_authoring_session( ) from exc discover_desktop_authoring_ipc() work_dir = Path(out_dir) if out_dir is not None else Path("runs") / "authoring" + options = dict( + work_dir=work_dir, + url=url, + headed=headed, + backend=backend, + backend_kind=backend_kind, + platform=platform, + **kwargs, + ) + if backend is None and backend_kind is None and url: + # --url pins sync Playwright. Launch, use, and close it on one owner + # thread: it leaves an event loop marked running on the thread that + # started it (anyio.run then refuses to start), and its objects fail + # when MCP worker threads call them. + owner = _BackendOwnerThread() + try: + session = owner.call(_open_pinned_session, flow_authoring, **options) + except BaseException: + owner.stop() + raise + return ThreadOwnedSession(owner, session) + return _open_pinned_session(flow_authoring, **options) + + +def _open_pinned_session( + flow_authoring: Any, + *, + work_dir: Path, + url: Optional[str], + headed: bool, + backend: Any, + backend_kind: Optional[str], + platform: Optional[str], + **kwargs: Any, +) -> object: close = None kind = backend_kind pinned = backend @@ -479,6 +636,10 @@ def open_authoring_session( if code == "COACH_ONLY" or type(exc).__name__ == "CoachOnlyError": session = CoachOnlySession(kind) else: + if close is not None: + # Do not leave the pinned browser running behind a refusal. + with contextlib.suppress(Exception): + close() if isinstance(exc, AuthoringError): raise raise AuthoringError(str(exc), code=code if isinstance(code, str) else None) from exc diff --git a/tests/test_authoring.py b/tests/test_authoring.py index dcba8a5..937070a 100644 --- a/tests/test_authoring.py +++ b/tests/test_authoring.py @@ -2,7 +2,9 @@ from __future__ import annotations +import asyncio import json +import threading from pathlib import Path import anyio @@ -14,6 +16,7 @@ AuthoringError, CoachOnlySession, MAX_AUTHORING_WIRE_BYTES, + ThreadOwnedSession, discover_desktop_authoring_ipc, open_authoring_session, pin_local_backend, @@ -759,6 +762,7 @@ def __init__(self, backend, out_dir, *, backend_kind, app_url=None, **kwargs): captured["out_dir"] = Path(out_dir) captured["backend_kind"] = backend_kind captured["app_url"] = app_url + captured["thread"] = threading.get_ident() import types import sys @@ -773,7 +777,10 @@ def __init__(self, backend, out_dir, *, backend_kind, app_url=None, **kwargs): lambda **kwargs: ("backend-obj", "web", None), ) session = open_authoring_session(out_dir=tmp_path, url="https://example.invalid/") - assert isinstance(session, Session) + # A --url session is built and driven on its browser owner thread. + assert isinstance(session, ThreadOwnedSession) + assert captured["thread"] != threading.get_ident() + session.close() assert captured["backend"] == "backend-obj" assert captured["backend_kind"] == "web" assert captured["app_url"] == "https://example.invalid/" @@ -822,3 +829,287 @@ def __init__(self, *args, **kwargs): out_dir=tmp_path, backend=object(), backend_kind="web" ) assert isinstance(session, CoachOnlySession) + + +class _ThreadBoundBackend: + """Stands in for sync Playwright: every call must come from its own thread.""" + + def __init__(self, owner: int): + self.owner = owner + self.calls: list[str] = [] + + def check(self, name: str) -> None: + if threading.get_ident() != self.owner: + raise AssertionError(f"{name} ran off the thread that launched the browser") + self.calls.append(name) + + +def install_fake_playwright_flow(monkeypatch) -> dict: + """Fake Flow whose browser launch behaves like sync_playwright().start(). + + Like the real launch, it leaves an event loop marked running on the + calling thread and returns objects bound to that thread. + """ + + import sys + import types + + launched: dict = {} + + class PlaywrightBackend: + @classmethod + def launch(cls, url, headless=True): + loop = asyncio.new_event_loop() + asyncio._set_running_loop(loop) + owner = threading.get_ident() + backend = _ThreadBoundBackend(owner) + launched.update(url=url, headless=headless, thread=owner, loop=loop, backend=backend) + + def close(): + backend.check("close") + asyncio._set_running_loop(None) + loop.close() + launched["closed"] = True + + return backend, close + + class Session: + backend_kind = "web" + + def __init__(self, backend): + self._backend = backend + + def observe(self): + self._backend.check("observe") + return {"backend": "web", "provider": "playwright_ax", "tree": []} + + def start_record(self): + self._backend.check("start_record") + return None + + def halt(self): + self._backend.check("halt") + return None + + def open_session(backend, out_dir, *, backend_kind, app_url=None): + backend.check("open_session") + return Session(backend) + + fake_flow = types.ModuleType("openadapt_flow") + fake_authoring = types.ModuleType("openadapt_flow.authoring") + fake_authoring.open_session = open_session + fake_backends = types.ModuleType("openadapt_flow.backends") + fake_playwright = types.ModuleType("openadapt_flow.backends.playwright_backend") + fake_playwright.PlaywrightBackend = PlaywrightBackend + monkeypatch.setitem(sys.modules, "openadapt_flow", fake_flow) + monkeypatch.setitem(sys.modules, "openadapt_flow.authoring", fake_authoring) + monkeypatch.setitem(sys.modules, "openadapt_flow.backends", fake_backends) + monkeypatch.setitem( + sys.modules, "openadapt_flow.backends.playwright_backend", fake_playwright + ) + return launched + + +def tool_is_error(result) -> bool: + """Read isError the same way on both MCP SDK generations.""" + + return result.model_dump(by_alias=True, mode="json")["isError"] is True + + +def release_main_thread_loop(launched: dict) -> None: + """Undo a launch that ran on this thread so later tests can start anyio.""" + + if launched.get("thread") == threading.get_ident() and not launched.get("closed"): + asyncio._set_running_loop(None) + launched["loop"].close() + + +def test_url_session_keeps_playwright_on_one_owner_thread_under_anyio( + monkeypatch, tmp_path, mcp_client +): + launched = install_fake_playwright_flow(monkeypatch) + try: + session = open_authoring_session( + out_dir=tmp_path, url="http://127.0.0.1:9/", headed=True + ) + server = build_server(authoring=AuthoringBridge(session, out_dir=tmp_path)) + + async def drive(): + async with mcp_client(server) as client: + return [ + await client.call_tool(name, {}) + for name in ("observe", "start_record", "halt") + ] + + # anyio refuses to start on a thread where Playwright left its loop. + results = anyio.run(drive) + assert [tool_is_error(result) for result in results] == [False, False, False], [ + result.content[0].text for result in results + ] + assert launched["thread"] != threading.get_ident() + assert launched["headless"] is False + with pytest.raises(RuntimeError): + asyncio.get_running_loop() + session.close() + assert launched["closed"] is True + assert launched["backend"].calls == [ + "open_session", + "observe", + "start_record", + "halt", + "close", + ] + with pytest.raises(AuthoringError, match="closed"): + session.observe() + session.close() + finally: + release_main_thread_loop(launched) + + +def test_authoring_url_headed_serve_starts_and_dispatches_tools( + monkeypatch, tmp_path, capsys, mcp_client +): + from openadapt_agent.cli import main + + launched = install_fake_playwright_flow(monkeypatch) + captured: dict = {} + + async def fake_run_stdio(bridge, authoring=None): + # Same anyio.run entry as stdio serve, with an in-memory client. + server = build_server(bridge, authoring=authoring) + async with mcp_client(server) as client: + captured["observe"] = await client.call_tool("observe", {}) + captured["start_record"] = await client.call_tool("start_record", {}) + + monkeypatch.setattr("openadapt_agent.mcp._run_stdio", fake_run_stdio) + try: + result = main( + [ + "serve", + "--authoring", + "--url", + "http://127.0.0.1:9/", + "--headed", + "--runs-dir", + str(tmp_path / "runs"), + ] + ) + err = capsys.readouterr().err + assert result == 0, err + assert "Already running asyncio" not in err + assert tool_is_error(captured["observe"]) is False + assert tool_is_error(captured["start_record"]) is False + assert launched["closed"] is True + finally: + release_main_thread_loop(launched) + + +def test_url_session_refusal_stops_the_owner_thread(monkeypatch, tmp_path): + install_fake_playwright_flow(monkeypatch) + before = {thread.name for thread in threading.enumerate()} + with pytest.raises(AuthoringError, match="empty cookies"): + open_authoring_session(out_dir=tmp_path, url="http://127.0.0.1:9/", headed=False) + assert {thread.name for thread in threading.enumerate()} <= before + + + +def test_url_session_construction_failure_closes_the_browser(monkeypatch, tmp_path): + import sys + + launched = install_fake_playwright_flow(monkeypatch) + + def broken_open_session(backend, out_dir, *, backend_kind, app_url=None): + raise RuntimeError("recorder unavailable") + + monkeypatch.setattr( + sys.modules["openadapt_flow.authoring"], "open_session", broken_open_session + ) + before = {thread.name for thread in threading.enumerate()} + try: + with pytest.raises(AuthoringError, match="recorder unavailable"): + open_authoring_session(out_dir=tmp_path, url="http://127.0.0.1:9/", headed=True) + assert launched["closed"] is True + assert {thread.name for thread in threading.enumerate()} <= before + finally: + release_main_thread_loop(launched) + +_DEMO_PAGE = b"""Demo + +""" + + +def test_real_url_session_serves_tools_under_anyio(monkeypatch, tmp_path, mcp_client): + """Real Playwright Chromium behind serve --authoring --url --headed. + + Skips when the browser extra or Chromium is missing. Never downloads a + browser. Chromium runs headless here so the test needs no display. + """ + + from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer + + pytest.importorskip("playwright") + try: + from openadapt_flow import authoring as _flow_authoring # noqa: F401 + from openadapt_flow._browser_setup import NO_AUTO_INSTALL_ENV, _chromium_present + from openadapt_flow.backends.playwright_backend import PlaywrightBackend + except ImportError: + pytest.skip("Flow authoring with the browser extra is not installed") + monkeypatch.setenv(NO_AUTO_INSTALL_ENV, "1") + try: + present = _chromium_present() + except Exception: + present = False + if not present: + pytest.skip("Playwright Chromium is not installed") + + launch = PlaywrightBackend.launch.__func__ + + def headless_launch(cls, url, headless=True, **kwargs): + return launch(cls, url, headless=True, **kwargs) + + monkeypatch.setattr(PlaywrightBackend, "launch", classmethod(headless_launch)) + + class DemoPage(BaseHTTPRequestHandler): + def do_GET(self): + self.send_response(200) + self.send_header("Content-Type", "text/html") + self.end_headers() + self.wfile.write(_DEMO_PAGE) + + def log_message(self, *args): + return None + + site = ThreadingHTTPServer(("127.0.0.1", 0), DemoPage) + threading.Thread(target=site.serve_forever, daemon=True).start() + try: + session = open_authoring_session( + out_dir=tmp_path, + url=f"http://127.0.0.1:{site.server_port}/", + headed=True, + ) + try: + server = build_server(authoring=AuthoringBridge(session, out_dir=tmp_path)) + + async def drive(): + async with mcp_client(server) as client: + observed = await client.call_tool("observe", {}) + tree = json.loads(observed.content[0].text)["tree"] + button = next(node for node in tree if node["role"] == "button") + results = [ + observed, + await client.call_tool("start_record", {}), + await client.call_tool("click", {"node_id": button["node_id"]}), + await client.call_tool("halt", {}), + ] + return tree, results + + tree, results = anyio.run(drive) + finally: + session.close() + finally: + site.shutdown() + site.server_close() + assert [tool_is_error(result) for result in results] == [False] * 4, [ + result.content[0].text for result in results + ] + assert {node["role"] for node in tree} >= {"button", "text_input"} diff --git a/tests/test_mailbox.py b/tests/test_mailbox.py index afb3fc7..b8f63a0 100644 --- a/tests/test_mailbox.py +++ b/tests/test_mailbox.py @@ -316,6 +316,49 @@ def test_continue_records_observed_never_type_text() -> None: assert "value" not in last["result"] +def test_continue_guard_reaches_a_browser_owner_thread_session() -> None: + """A --url session lives on its browser thread; Continue still cannot type.""" + + import threading + + from openadapt_agent.authoring import ThreadOwnedSession, _BackendOwnerThread + + class TypingContinue: + backend_kind = "web" + + def __init__(self) -> None: + self.typed: list[str] = [] + self.continue_thread: int | None = None + + def pause_for_input(self, *, param, secret=False, node_id=None): + return None + + def type_text(self, text, param=None): + self.typed.append(text) + + def continue_input(self): + self.continue_thread = threading.get_ident() + self.type_text("must not be typed") + return {"recorded": True, "param": "note"} + + owner = _BackendOwnerThread() + inner = owner.call(TypingContinue) + session = ThreadOwnedSession(owner, inner) + try: + mock = MockMailbox() + client, _out = _client(mock, session=session) + client.claim(PACK, BIND) + client.handle_envelope(_envelope("bind_pack")) + result = client.handle_envelope(_envelope("pause_for_input", args={"param": "note"})) + assert result == {"status": "error", "error": "error"} + assert inner.typed == [] + assert inner.continue_thread is not None + assert inner.continue_thread != threading.get_ident() + assert inner.type_text.__func__ is TypingContinue.type_text + finally: + session.close() + + def test_secret_continue_has_no_text() -> None: mock = MockMailbox() recorder = FakeRecorder() From 181432a94854077414878606f65bf7be784c49d2 Mon Sep 17 00:00:00 2001 From: abrichr Date: Sat, 10 Oct 2026 00:08:25 -0400 Subject: [PATCH 2/3] fix: refuse authoring connect --url before claiming the runner link `openadapt-agent authoring connect '' --url ` without --headed, which is the command docs/MAILBOX_CLI.md showed, could not open its browser. The connect path swallowed that refusal, claimed the one-use runner link anyway, asked the person to Allow the chat, and then answered every command with a bare "error". The person needed a new runner link and had no idea why. The same happened with --headed when the browser extra was missing. connect_mailbox now refuses --url without --headed before it opens a browser or contacts the mailbox, and says to add --headed. Any other failure to open the --url browser is reported with its reason before the claim. Both messages say the runner link was not used. A mailbox client that has a --url but no open session now answers as coach-only instead of "error". The docs example now passes --headed, and a test checks every documented `authoring connect --url` command does. Ledger: B085 Co-Authored-By: Claude Opus 5.5 --- docs/MAILBOX_CLI.md | 6 +-- src/openadapt_agent/mailbox.py | 16 ++++++- tests/test_mailbox.py | 78 ++++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 5 deletions(-) diff --git a/docs/MAILBOX_CLI.md b/docs/MAILBOX_CLI.md index e0b9261..8beffa1 100644 --- a/docs/MAILBOX_CLI.md +++ b/docs/MAILBOX_CLI.md @@ -12,10 +12,10 @@ Canonical in this package: openadapt-agent authoring connect '' ``` -Playwright web (fresh Chromium, empty cookies): +Playwright web (fresh Chromium, empty cookies, visible so you can sign in): ```bash -openadapt-agent authoring connect '' --url https://example.invalid/app +openadapt-agent authoring connect '' --url https://example.invalid/app --headed ``` Rejected alternative: `openadapt-agent serve --authoring --mailbox`. `serve` is stdio MCP. Mixing it with an outbound poll loop would look like this package grew a hosted transport. The mailbox client is a separate verb. @@ -52,7 +52,7 @@ Flow `AuthoringSession` / `Recorder` is the actuation engine when importable. Ov - Print Allow (`Allow ChatGPT to drive this job?` / replace-account copy). stdin `y/n`. - Pause: print `Sign in in the app, then press Enter`. Do not ask for a password. Do not `type_text` the secret. - Continue → `record_observed` on the pause-target node. -- `--url` pins Playwright Chromium with **empty cookies**. No debug-port attach. +- `--url` pins Playwright Chromium with **empty cookies**. No debug-port attach. It needs `--headed` so the person can sign in in that window. Without `--headed`, or when the browser can't open, the command stops before it claims the runner link, so the same link still works. - Unique-window fail-closed: macOS / Linux without a unique frontmost title is coach-only. Windows native / Citrix / RDP are coach-only. Never spawn `win_agent`. - Allow-per-`sub` before observe / click / halt. - GET handshake is not actuation (the CLI does not GET the pack page to click). diff --git a/src/openadapt_agent/mailbox.py b/src/openadapt_agent/mailbox.py index 11a954a..23da871 100644 --- a/src/openadapt_agent/mailbox.py +++ b/src/openadapt_agent/mailbox.py @@ -44,6 +44,11 @@ BOUND_SENTENCE = ( "OpenAdapt is installed on this computer, so an agent can drive only through OpenAdapt." ) +RUNNER_LINK_UNUSED = "The runner link was not used, so you can run the command again." +URL_NEEDS_HEADED = ( + "--url opens a new Chromium window with no saved sign-in. Add --headed so " + "you can sign in to the app in that window. " + RUNNER_LINK_UNUSED +) ENQUEUE_REQUIRING_ALLOW = frozenset( { "observe", @@ -381,7 +386,8 @@ def __init__( def _pin(self) -> tuple[str, bool]: if self._url: - return "web", False + # Without an open browser session nothing can be driven here. + return "web", self.session is None plat = self._platform if plat == "win32" or plat.startswith("win"): return "windows", True @@ -723,7 +729,11 @@ def _open_session(*, url: Optional[str], headed: bool, platform: Optional[str]) return CoachOnlySession("windows") try: return open_authoring_session(url=url, headed=headed, platform=platform) - except AuthoringError: + except AuthoringError as exc: + if url: + # Refuse before the one-use runner link is claimed. + detail = str(exc).rstrip(". ") + raise MailboxError(f"{detail}. {RUNNER_LINK_UNUSED}") from exc if (plat == "win32" or plat.startswith("win")) and not url: return CoachOnlySession("windows") return None @@ -758,6 +768,8 @@ def connect_mailbox( "this pack URL is not a runner link; paste the Connect this computer " "command (openadapt://runner?pack=…&bind=oab_…)" ) + if url and not headed and session is None: + raise MailboxError(URL_NEEDS_HEADED) transport = open_mailbox_transport(origin=parsed["origin"], post=post) opened = session if opened is None: diff --git a/tests/test_mailbox.py b/tests/test_mailbox.py index b8f63a0..13fcdc7 100644 --- a/tests/test_mailbox.py +++ b/tests/test_mailbox.py @@ -563,3 +563,81 @@ def test_connect_help_does_not_name_mockmed(capsys) -> None: assert "Allow" in out assert "MockMed" not in out assert "password" not in out.lower() + + +def _connect_url(mock: MockMailbox, *, headed: bool) -> int: + return connect_mailbox( + URI, + url="https://example.invalid/app", + headed=headed, + post=mock.post, + prompt=lambda _message: True, + pause_wait=lambda: None, + sleep=lambda _seconds: None, + max_polls=3, + stdout=io.StringIO(), + platform="darwin", + ) + + +def test_connect_url_without_headed_refuses_before_claim(monkeypatch) -> None: + opened: list[dict[str, Any]] = [] + monkeypatch.setattr( + "openadapt_agent.mailbox.open_authoring_session", + lambda **kwargs: opened.append(kwargs), + ) + mock = MockMailbox(poll_bodies=[_envelope("bind_pack"), _envelope("observe")]) + with pytest.raises(MailboxError, match="--headed"): + _connect_url(mock, headed=False) + # The one-use bind is still unclaimed, so the same runner link works again. + assert mock.requests == [] + assert opened == [] + + +def test_connect_url_refuses_before_claim_when_the_browser_cannot_open(monkeypatch) -> None: + from openadapt_agent.authoring import AuthoringError + + def no_browser(**kwargs): + raise AuthoringError( + "Playwright web pin requires openadapt-flow with the browser extra " + "(openadapt-agent[tutorial])" + ) + + monkeypatch.setattr("openadapt_agent.mailbox.open_authoring_session", no_browser) + mock = MockMailbox(poll_bodies=[_envelope("bind_pack"), _envelope("observe")]) + with pytest.raises(MailboxError, match="browser extra"): + _connect_url(mock, headed=True) + assert not any(path.endswith("/runner/claim") for path, _body, _headers in mock.requests) + + +def test_url_client_without_a_session_is_coach_only_not_error() -> None: + mock = MockMailbox() + client = MailboxClient( + MailboxTransport(origin=ORIGIN, post=mock.post), + session=None, + prompt=lambda _message: True, + sleep=lambda _seconds: None, + stdout=io.StringIO(), + url="https://example.invalid/app", + platform="darwin", + ) + client.claim(PACK, BIND) + client.handle_envelope(_envelope("bind_pack")) + observed = client.handle_envelope(_envelope("observe")) + assert observed["coach_only"] is True + assert observed["agent_drive"] is False + client.handle_envelope(_envelope("start_record")) + callback = [item[1] for item in mock.requests if item[0].endswith("/callback")][-1] + assert callback["result"]["error"] == "COACH_ONLY" + + +def test_documented_url_connect_commands_pass_headed() -> None: + root = Path(__file__).resolve().parents[1] + commands = [] + for doc in [root / "README.md", root / "llms.txt", *sorted((root / "docs").glob("*.md"))]: + text = doc.read_text(encoding="utf-8").replace("\\\n", " ") + for line in text.splitlines(): + if "authoring connect" in line and "--url" in line: + commands.append((doc.name, line.strip())) + assert commands, "expected a documented authoring connect --url example" + assert [item for item in commands if "--headed" not in item[1]] == [] From cc4bafd9d72ee25e4d22e588144cf7f7f94f920a Mon Sep 17 00:00:00 2001 From: abrichr Date: Sat, 10 Oct 2026 00:22:39 -0400 Subject: [PATCH 3/3] fix: keep a coach-only --url session coach-only behind its owner thread When `serve --authoring --url` cannot pin a browser on Windows (no --headed, or no browser extra), the session falls back to the coach-only stand-in. The owner-thread change wraps that stand-in in a ThreadOwnedSession, so AuthoringBridge's isinstance check no longer saw it as coach-only. A `type` call then answered {"status": "ok", "recorded": true} although nothing was typed or recorded. AuthoringBridge now looks through the owner-thread wrapper when it decides whether a session is coach-only, so `type` and `click` refuse with COACH_ONLY again. The new test fails without this change. Ledger: B076 Co-Authored-By: Claude Opus 5.5 --- src/openadapt_agent/authoring.py | 10 +++++++++- tests/test_authoring.py | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/openadapt_agent/authoring.py b/src/openadapt_agent/authoring.py index f3ff9c7..767fc88 100644 --- a/src/openadapt_agent/authoring.py +++ b/src/openadapt_agent/authoring.py @@ -388,6 +388,14 @@ def close(self) -> None: self._owner.stop() +def _is_coach_only_session(session: object) -> bool: + """True for the coach-only stand-in, even behind its owner thread.""" + + if isinstance(session, ThreadOwnedSession): + session = object.__getattribute__(session, "_session") + return isinstance(session, CoachOnlySession) + + def discover_desktop_authoring_ipc(*, home: Optional[Path] = None) -> Optional[dict[str, Any]]: """Return Desktop authoring IPC discovery when D2 has advertised it. @@ -915,7 +923,7 @@ def __init__(self, session: object, *, out_dir: Optional[Path | str] = None): self._out_dir = Path(out_dir) if out_dir is not None else None self._last_tool: Optional[str] = None self._last_result: Optional[dict[str, Any]] = None - self._coach_only = isinstance(session, CoachOnlySession) + self._coach_only = _is_coach_only_session(session) self._nodes: dict[str, dict[str, Any]] = {} self._coach_hint: Optional[str] = None diff --git a/tests/test_authoring.py b/tests/test_authoring.py index 937070a..6d612b6 100644 --- a/tests/test_authoring.py +++ b/tests/test_authoring.py @@ -1012,6 +1012,26 @@ def test_url_session_refusal_stops_the_owner_thread(monkeypatch, tmp_path): assert {thread.name for thread in threading.enumerate()} <= before +def test_url_coach_only_fallback_still_refuses_type_behind_owner_thread( + monkeypatch, tmp_path +): + """Windows --url that cannot pin a browser stays coach-only, not "recorded".""" + + install_fake_playwright_flow(monkeypatch) + session = open_authoring_session( + out_dir=tmp_path, url="http://127.0.0.1:9/", headed=False, platform="win32" + ) + try: + bridge = AuthoringBridge(session, out_dir=tmp_path) + with pytest.raises(AuthoringError, match="COACH_ONLY"): + bridge.dispatch("type", {"text": "hello"}) + with pytest.raises(AuthoringError, match="COACH_ONLY"): + bridge.dispatch("click", {"x": 1, "y": 2}) + finally: + closer = getattr(session, "close", None) + if callable(closer): + closer() + def test_url_session_construction_failure_closes_the_browser(monkeypatch, tmp_path): import sys @@ -1033,6 +1053,7 @@ def broken_open_session(backend, out_dir, *, backend_kind, app_url=None): finally: release_main_thread_loop(launched) + _DEMO_PAGE = b"""Demo """