From dc87f8fca5d64e74158cae38a49ef09b88bb187d Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Fri, 2 Oct 2026 16:09:44 +0000 Subject: [PATCH 1/9] fix(browser): key an acting call on its own site, and bound the owner stamps Three defects the browser tools carry, plus the test fixture that hid one of them from every Linux run. 1. The permission key could name a site the call would not act on. `_ActingTool.cast_params` read the site from `url_for(owner)`, which fell back to the panel's active page for an owner with no live binding. If that tab belonged to another owner, the action landed on a fresh blank tab instead, so the person was asked to approve a site the call never touched. The grant was also reusable: `session_keys` returns one digest for all three acting verbs on a site, so the key that call banked was identical to the one a later real click, type or press on that site presents. `url_for` now answers empty for an unbound owner rather than the panel, and `cast_params` writes no site. No site means `session_keys` falls through to the per-call digest, so a call that opens its own page banks nothing a real site can present. Empty is also the honest answer for the reader's path: `url_for(None)` still reads the panel. 2. `_BrowserTool._acted` was never pruned. Class-level, keyed by owner, with no path that removed an entry -- and the owner became `run:`, so every delegated run that acted on the browser left a key for the life of the process. Measured: 50000 runs retained 50000 entries and 6.6 MiB after `gc.collect()`. Two bounds now. The driver announces every binding it drops, through one `_drop_owners` that a reap, an explicit release and a closing tab all go through, and the tool forgets that owner's stamp when it hears. And the map keeps the newest 512 owners, so an owner whose binding was never explicitly ended cannot accumulate either. A stale binding is no longer handed the panel's tab: `_page_for` drops it and opens its own page. 3. `browser_tabs` did not say that `close` refuses an unheld tab. Its description named only the tab "marked held", but `tab_close` also refuses every tab with no owner, because an unowned tab is the reader's and may hold the login `HANDOFF_NOTE` asked them to complete. The listing marks such a tab with neither `yours` nor `held`, so nothing the model read told it apart from one it may close. 4. The real-page suite pinned Playwright's browser cache to `~/Library/Caches/ms-playwright`, which is the macOS location only. Every Linux run looked there, found nothing, and reported Chromium missing -- while suggesting an install that would write to `~/.cache` and change nothing. The skip guard asked only whether the `playwright` package imports, so the missing binary failed six tests instead of skipping them. The look-up moves to `tests/_browser_cache.py`: per-platform, past a sandboxed `HOME`, and the skip now checks for the binary it needs. The browser driver's own message stops claiming "not installed" when the truth is "not where this process looked" -- that reading cost a reviewer a false lead -- and quotes the path Playwright tried. Verification: - `uv run pytest tests/test_browser_cache.py tests/test_browser_driver.py tests/test_browser_tools.py tests/test_browser_policy.py tests/integration/test_browser_real_web.py tests/integration/test_browser_tools_real_web.py` -- 158 passed. - Reverting the source with the new tests kept turns five of them red, including the acting-tool key test. - 50000 `_mark` calls now retain 512 entries rather than 50000. - Full non-integration suite: 27110 passed, 7 failed. All 7 fail identically on a pristine `origin/main`, and are this box: an environment proxy that is listening, root ignoring permission bits, and a node runtime the fixtures expect to be unreadable. - `ruff check`, `ruff format --check`, `lint-imports` (10 kept), the source-language gate and the large-file gate all pass. Co-authored-by: Claude (claude-opus-5-5[1m]) --- raven/agent/tools/browser.py | 41 +++++++- raven/browser/driver.py | 76 ++++++++++++--- tests/_browser_cache.py | 94 ++++++++++++++++++ tests/integration/test_browser_real_web.py | 18 ++-- .../test_browser_tools_real_web.py | 42 ++------ tests/test_browser_cache.py | 76 +++++++++++++++ tests/test_browser_driver.py | 96 ++++++++++++++++++- tests/test_browser_tools.py | 28 +++++- 8 files changed, 409 insertions(+), 62 deletions(-) create mode 100644 tests/_browser_cache.py create mode 100644 tests/test_browser_cache.py diff --git a/raven/agent/tools/browser.py b/raven/agent/tools/browser.py index 9abc6e3f7..1f6a09762 100644 --- a/raven/agent/tools/browser.py +++ b/raven/agent/tools/browser.py @@ -37,6 +37,12 @@ SNAPSHOT_TEXT_CHARS = 8_000 MAX_REFS_SHOWN = 120 +# How many owners' stamps ``_acted`` keeps. The stamps only answer "did the +# reader touch the page since I last acted", which the newest few owners can +# answer as well as all of them; the cap is what stops one key per delegated run +# from accumulating for the life of the process. +_ACTED_MAX = 512 + # Carried by browser_navigate alone. Every tool's description is paid for on # every turn of every conversation, and the one place the model decides whether # a page is its business at all is when it opens one. @@ -175,7 +181,10 @@ class _BrowserTool(Tool): timeout_seconds = 90.0 # The last time this owner acted, so a readback can say whether a hand - # other than the model's touched the browser in between. + # other than the model's touched the browser in between. Bounded, and + # pruned when the driver drops the matching tab binding: the key is a + # delegated run's uid, and a run that has ended never acts again, so an + # unbounded map grows one entry per run for the life of the process. _acted: dict[str, float] = {} @staticmethod @@ -188,12 +197,37 @@ def _owner(self) -> str: return current_owner() def _mark(self, owner: str) -> None: - _BrowserTool._acted[owner] = time.monotonic() + acted = _BrowserTool._acted + acted[owner] = time.monotonic() + self._bind_to_driver() + if len(acted) > _ACTED_MAX: + # Oldest-first, and only ever to the cap: the keys are owner names + # that a finished delegated run never speaks again, so without this + # one key per run outlives the process. + for stale in sorted(acted, key=lambda k: acted[k])[: len(acted) - _ACTED_MAX]: + acted.pop(stale, None) def _touched(self, owner: str) -> bool: last = _BrowserTool._acted.get(owner) return last is not None and _browser().touched_since(last) + @classmethod + def _forget_owner(cls, owner: str) -> None: + cls._acted.pop(owner, None) + + def _bind_to_driver(self) -> None: + """Register this owner's store with the driver that ends it. + + Lazy and idempotent: the driver is the process-wide one, and reaching + for it is what builds it, so doing this at import would have the tools + module construct the shared browser as a side effect. The driver is the + one place that knows when an owner's tab binding ends, so the stamp map + is pruned from there rather than on a second clock of its own. + """ + browser = _browser() + if browser.on_owner_released is not _BrowserTool._forget_owner: + browser.on_owner_released = _BrowserTool._forget_owner + async def _readback(self, owner: str, state: dict[str, Any], *, acted: bool) -> ToolResult: """State plus a compact snapshot; the snapshot is skipped on an error so a failure reads as one and not as a page description.""" @@ -550,7 +584,8 @@ def description(self) -> str: return ( "List, open, switch or close tabs of the shared browser. Your calls always land on your own " "tab; a tab marked held is another agent's and cannot be taken. new opens a fresh tab (with " - "an optional url) and makes it yours; activate makes an unheld tab yours." + "an optional url) and makes it yours; activate makes an unheld tab yours. close takes only a " + "tab you hold: a tab with no mark at all is the user's, and closing it needs activate first." ) @property diff --git a/raven/browser/driver.py b/raven/browser/driver.py index 7b87a8012..147bfc348 100644 --- a/raven/browser/driver.py +++ b/raven/browser/driver.py @@ -217,6 +217,12 @@ def __init__(self) -> None: # moment two callers are most likely to collide. Separate from # `_s.lock`, which the navigation methods take after calling `_ensure`. self._launch_lock = asyncio.Lock() + # Told the name of every owner this driver stops binding to a page, so + # whatever a caller keeps per owner can drop its copy on the same event + # instead of on a second, independently drifting clock. An attribute + # rather than a state field because the bindings outlive a close(), and + # a listener registered before one would otherwise be dropped by it. + self.on_owner_released: Any = None # ---- lifecycle --------------------------------------------------------- @@ -318,7 +324,18 @@ async def _launch_once(self) -> Any: await self.close() hint = f"{exc}" if "Executable doesn't exist" in hint or "playwright install" in hint: - raise BrowserUnavailableError(f"Chromium is not installed. Run: {CHROMIUM_INSTALL_HINT}") from None + # A search failure, not only a missing install. This fires when + # the browser is on disk somewhere the launcher did not look -- + # Playwright resolves its cache under HOME, which a sandboxed + # test or a different user profile can point elsewhere -- and + # "not installed" then sends the reader to download something + # they already have. Playwright's own first line names the path + # it tried, which is the one fact that tells the two apart. + tried = hint.strip().splitlines()[0] if hint.strip() else "" + detail = f"\n({tried})" if tried else "" + raise BrowserUnavailableError( + f"Chromium is not installed where this process looks for it. Run: {CHROMIUM_INSTALL_HINT}{detail}" + ) from None raise BrowserUnavailableError(f"could not start Chromium: {hint}") from None logger.info("browser: chromium started ({}x{})", w, h) return self._s.page @@ -485,9 +502,14 @@ def _owner_live(self, rec: _Owner, now: float) -> bool: closed = True return not closed and now - rec.seen < OWNER_IDLE_S - def _reap_owners(self, now: float) -> None: - for key in [k for k, rec in self._s.owners.items() if not self._owner_live(rec, now)]: - del self._s.owners[key] + def _reap_owners(self, now: float) -> list[str]: + """The owners whose bindings have just died. Removal is the caller's. + + Names rather than removal, because every way a binding ends has to be + announced in one place: ``_drop_owners`` does the removing and the + telling, so a reap and an explicit release cannot drift apart. + """ + return [k for k, rec in self._s.owners.items() if not self._owner_live(rec, now)] def owner_of(self, page: Any) -> str | None: """Which live owner holds this page, if any.""" @@ -498,12 +520,22 @@ def owner_of(self, page: Any) -> str | None: return None def url_for(self, owner: str | None) -> str: - """Where an owner's tab is, without starting anything or rebinding.""" - if owner is not None: - rec = self._s.owners.get(owner) - if rec is not None and self._owner_live(rec, time.monotonic()): - return rec.page.url - return self.url + """Where an owner's tab is, without starting anything or rebinding. + + An owner already bound to a live page reads that page. An owner with no + live binding answers empty rather than the panel's active page: the + panel may be showing somebody else's tab, and a caller that keys an + action on this url -- the permission gate does -- would otherwise ask + about a site the call is not going to touch. Empty is the answer that + says "this owner has no page yet", and the caller decides what that + means. + """ + if owner is None: + return self.url + rec = self._s.owners.get(owner) + if rec is not None and self._owner_live(rec, time.monotonic()): + return rec.page.url + return "" async def _page_for(self, owner: str | None, *, act: bool = True) -> Any: """The page a caller works on, binding an owner on its first call. @@ -518,7 +550,7 @@ async def _page_for(self, owner: str | None, *, act: bool = True) -> Any: if act: self._s.touched = now return active - self._reap_owners(now) + self._drop_owners(self._reap_owners(now)) rec = self._s.owners.get(owner) if rec is not None: rec.seen = now @@ -559,7 +591,24 @@ def touched_since(self, when: float) -> bool: def release(self, owner: str) -> None: """Forget an owner's binding; its tab stays open for whoever claims it next.""" - self._s.owners.pop(owner, None) + self._drop_owners([owner]) + + def _drop_owners(self, owners: list[str]) -> None: + """Remove these bindings, and tell the listener about the ones there were. + + Whatever ends a binding -- a reap, an explicit release, the tab it was + on closing -- goes through here, so the listener is told once per owner + under one rule rather than at each call site. An owner that was never + bound is not announced: there is nothing to have lost. + """ + gone = [owner for owner in owners if self._s.owners.pop(owner, None) is not None] + if self.on_owner_released is None: + return + for owner in gone: + try: + self.on_owner_released(owner) + except Exception as exc: # noqa: BLE001 - a listener must not take a binding drop down + logger.debug("browser: owner-release listener failed for {}: {}", owner, exc) # ---- tabs ---------------------------------------------------------------- @@ -670,8 +719,7 @@ async def tab_close(self, index: int, *, owner: str | None = None) -> dict[str, await victim.close() except Exception: pass - for key in [k for k, rec in self._s.owners.items() if rec.page is victim]: - del self._s.owners[key] + self._drop_owners([k for k, rec in self._s.owners.items() if rec.page is victim]) rest = self._pages() if not rest: await self.close() diff --git a/tests/_browser_cache.py b/tests/_browser_cache.py new file mode 100644 index 000000000..76a249146 --- /dev/null +++ b/tests/_browser_cache.py @@ -0,0 +1,94 @@ +"""Where Playwright keeps the Chromium a real-page test needs, by platform. + +Two things about that look-up are easy to get wrong, and both were, in two +different files: + +* Playwright resolves its browser cache relative to ``HOME``, and this suite + redirects ``HOME`` at a per-test tmp dir (``tests/conftest.py``, so no test + reads the config of whoever is running it). A real-page test therefore has to + point the look-up back at the login's cache itself. +* The default location is per-OS -- ``~/Library/Caches/ms-playwright`` on + macOS, ``%LOCALAPPDATA%``-shaped on Windows, ``~/.cache/ms-playwright`` on + Linux -- and a fixture that hard-codes one of them is right on exactly one + platform and, worse, right for whoever wrote it. + +An explicit ``PLAYWRIGHT_BROWSERS_PATH`` always wins over both, so CI that +installs its browsers elsewhere is untouched by any of this. +""" + +from __future__ import annotations + +import os +import sys + +try: + import pwd +except ImportError: # pragma: no cover - Windows has no pwd module + pwd = None # type: ignore[assignment] + + +def login_home() -> str: + """The login directory, ignoring a ``HOME`` a fixture has sandboxed. + + ``pwd`` rather than ``Path.home()`` because the latter follows ``HOME``, + which is the value every caller here is trying to see past. + """ + if pwd is not None: + try: + return pwd.getpwuid(os.getuid()).pw_dir + except (KeyError, AttributeError): # pragma: no cover - no passwd entry + pass + return os.path.expanduser("~") + + +def playwright_cache(home: str) -> str: + """The directory ``playwright install chromium`` writes to under ``home``.""" + if sys.platform == "darwin": + return os.path.join(home, "Library", "Caches", "ms-playwright") + if sys.platform == "win32": + return os.path.join(home, "AppData", "Local", "ms-playwright") + return os.path.join(home, ".cache", "ms-playwright") + + +def browsers_dir() -> str: + """The browser cache this process resolves to. Safe to call at import. + + Not the same question as "is Chromium installed": this is where the + look-up goes, which depends on the environment and not on the disk. + """ + return os.environ.get("PLAYWRIGHT_BROWSERS_PATH") or playwright_cache(login_home()) + + +def chromium_installed(where: str | None = None) -> bool: + """Whether a Chromium the launcher would accept is under ``where``. + + A directory entry rather than ``Browser.probe()``, which only answers + whether the ``playwright`` package imports: those are different questions, + and skipping on the package alone turns a missing binary into six failures. + """ + target = where or browsers_dir() + try: + return any(name.startswith("chromium") for name in os.listdir(target)) + except OSError: + return False + + +def point_at_login_cache(monkeypatch) -> None: + """Fixture body: aim Playwright at the login's cache, unless already aimed. + + Set before the launch rather than after, because the sandboxed ``HOME`` is + already in place by the time a fixture runs and the first ``goto`` is what + starts Chromium. + """ + if os.environ.get("PLAYWRIGHT_BROWSERS_PATH"): + return + monkeypatch.setenv("PLAYWRIGHT_BROWSERS_PATH", playwright_cache(login_home())) + + +__all__ = [ + "browsers_dir", + "chromium_installed", + "login_home", + "playwright_cache", + "point_at_login_cache", +] diff --git a/tests/integration/test_browser_real_web.py b/tests/integration/test_browser_real_web.py index dcc969e21..965d15f13 100644 --- a/tests/integration/test_browser_real_web.py +++ b/tests/integration/test_browser_real_web.py @@ -12,15 +12,19 @@ from __future__ import annotations -import os -import pwd - import pytest from raven.browser import get_browser from raven.browser.driver import Browser +from tests._browser_cache import chromium_installed, point_at_login_cache -pytestmark = pytest.mark.skipif(not Browser.probe()[0], reason="browser extra not installed") +# Both halves matter: the package can import with no browser downloaded, and a +# skip that only asks about the package reports that as six failures rather +# than as a skip. The install line above is what the second half is about. +pytestmark = pytest.mark.skipif( + not Browser.probe()[0] or not chromium_installed(), + reason="playwright or its Chromium is not installed (see the module docstring)", +) @pytest.fixture @@ -136,9 +140,7 @@ async def test_chromium_starts_only_on_demand(browser) -> None: @pytest.fixture(autouse=True) -def _browsers_from_the_real_home(monkeypatch): +def _browsers_from_the_real_home(monkeypatch: pytest.MonkeyPatch) -> None: """The suite redirects HOME to a temp dir; playwright keeps its browsers under the real one. Point it there unless the caller already did.""" - if not os.environ.get("PLAYWRIGHT_BROWSERS_PATH"): - real_home = pwd.getpwuid(os.getuid()).pw_dir - monkeypatch.setenv("PLAYWRIGHT_BROWSERS_PATH", os.path.join(real_home, "Library", "Caches", "ms-playwright")) + point_at_login_cache(monkeypatch) diff --git a/tests/integration/test_browser_tools_real_web.py b/tests/integration/test_browser_tools_real_web.py index 215d85ee7..62b08591c 100644 --- a/tests/integration/test_browser_tools_real_web.py +++ b/tests/integration/test_browser_tools_real_web.py @@ -16,7 +16,6 @@ import http.server import os -import sys import threading from collections.abc import Iterator from pathlib import Path @@ -34,8 +33,15 @@ ) from raven.browser import get_browser from raven.browser.driver import Browser - -pytestmark = pytest.mark.skipif(not Browser.probe()[0], reason="browser extra not installed") +from tests._browser_cache import chromium_installed, point_at_login_cache + +# Both halves matter: the package can import with no browser downloaded, and a +# skip that only asks about the package reports that as failures rather than as +# a skip. +pytestmark = pytest.mark.skipif( + not Browser.probe()[0] or not chromium_installed(), + reason="playwright or its Chromium is not installed (see test_browser_real_web.py)", +) FORM = b"""Raven Form Test

Sign up

@@ -66,28 +72,6 @@ def log_message(self, *args: object) -> None: pass -try: - import pwd -except ImportError: # pragma: no cover - Windows has no pwd module - pwd = None # type: ignore[assignment] - - -def _playwright_cache(home: str) -> str: - """Where ``playwright install`` put the browsers, by platform. - - Playwright picks this directory by OS, and this fixture only has to name - the same one: hard-coded to macOS it sent a Linux run looking in - ``~/Library/Caches``, which the documented install never writes -- the - suite then reported Chromium missing and suggested an install that would - write somewhere else again. - """ - if sys.platform == "darwin": - return os.path.join(home, "Library", "Caches", "ms-playwright") - if sys.platform == "win32": - return os.path.join(home, "AppData", "Local", "ms-playwright") - return os.path.join(home, ".cache", "ms-playwright") - - @pytest.fixture(autouse=True) def _browsers_from_the_real_home(monkeypatch: pytest.MonkeyPatch) -> None: """Point Playwright at the human's cache rather than a sandboxed HOME. @@ -95,13 +79,7 @@ def _browsers_from_the_real_home(monkeypatch: pytest.MonkeyPatch) -> None: An explicit ``PLAYWRIGHT_BROWSERS_PATH`` always wins, so CI that installs browsers elsewhere is untouched. """ - if os.environ.get("PLAYWRIGHT_BROWSERS_PATH"): - return - try: - home = pwd.getpwuid(os.getuid()).pw_dir if pwd is not None else os.path.expanduser("~") - except (KeyError, AttributeError): - home = os.path.expanduser("~") - monkeypatch.setenv("PLAYWRIGHT_BROWSERS_PATH", _playwright_cache(home)) + point_at_login_cache(monkeypatch) @pytest.fixture diff --git a/tests/test_browser_cache.py b/tests/test_browser_cache.py new file mode 100644 index 000000000..866099a55 --- /dev/null +++ b/tests/test_browser_cache.py @@ -0,0 +1,76 @@ +"""The Playwright browser-cache look-up the real-page suites share. + +Both of those suites need the cache under the *login's* home, because the +suite redirects ``HOME`` at a per-test tmp dir and Playwright resolves the +cache from ``HOME``. Getting it wrong is not loud: the run reports Chromium +missing and suggests an install that would write somewhere else again. +""" + +from __future__ import annotations + +import os +import sys + +import pytest + +from tests._browser_cache import browsers_dir, chromium_installed, login_home, playwright_cache + + +def test_the_cache_is_named_per_platform() -> None: + """The three defaults Playwright documents. Hard-coding one of them is + right on exactly one OS, and right for whoever wrote it.""" + assert playwright_cache("/home/u") == os.path.join("/home/u", ".cache", "ms-playwright") + if sys.platform == "darwin": + assert playwright_cache("/Users/u") == "/Users/u/Library/Caches/ms-playwright" + pytest.skip("the other two branches are the two below, checked by the platform not taken") + if sys.platform == "win32": + assert playwright_cache("C:/Users/u") == os.path.join("C:/Users/u", "AppData", "Local", "ms-playwright") + + +@pytest.mark.parametrize( + ("platform", "expected"), + [ + ("darwin", "Library/Caches/ms-playwright"), + ("win32", "AppData/Local/ms-playwright"), + ("linux", ".cache/ms-playwright"), + ], +) +def test_every_branch_of_the_platform_switch_is_reachable(monkeypatch, platform, expected) -> None: + """Which branch runs is `sys.platform`, so exercise all three rather than + only the one this machine happens to be.""" + monkeypatch.setattr(sys, "platform", platform) + + assert playwright_cache("/h") == os.path.join("/h", expected) + + +def test_the_login_home_ignores_a_sandboxed_home(monkeypatch) -> None: + """`Path.home()` and `expanduser` both follow `HOME`, which is the value + every caller here is trying to see past.""" + monkeypatch.setenv("HOME", "/tmp/sandboxed") + + home = login_home() + + assert home != "/tmp/sandboxed" + assert os.path.isdir(home), "and it is a real directory, not a guess" + + +def test_an_explicit_cache_path_wins(monkeypatch) -> None: + monkeypatch.setenv("PLAYWRIGHT_BROWSERS_PATH", "/elsewhere/ms-playwright") + + assert browsers_dir() == "/elsewhere/ms-playwright" + + +def test_the_skip_only_fires_when_there_is_no_chromium(monkeypatch, tmp_path) -> None: + """The old skip asked `Browser.probe()`, which only tests the `playwright` + package -- so a missing binary failed six tests instead of skipping them.""" + empty = tmp_path / "ms-playwright" + empty.mkdir() + assert chromium_installed(str(empty)) is False + + (empty / "chromium-1234").mkdir() + assert chromium_installed(str(empty)) is True + + (empty / "chromium_headless_shell-1234").mkdir() + assert chromium_installed(str(empty)) is True + + assert chromium_installed(str(tmp_path / "absent")) is False, "a missing directory is not an install" diff --git a/tests/test_browser_driver.py b/tests/test_browser_driver.py index 49dd1fddc..1e9c22db8 100644 --- a/tests/test_browser_driver.py +++ b/tests/test_browser_driver.py @@ -349,8 +349,12 @@ async def start(self) -> None: await b._ensure() why = str(exc.value) - assert "Chromium is not installed" in why + assert "Chromium is not installed where this process looks for it" in why assert f"{shlex.quote(sys.executable)} -m playwright install chromium" in why + # The path Playwright searched, quoted back. Without it the message reads + # as "not installed" whichever of the two it was, and a sandboxed HOME + # sends the reader to download a browser they already have. + assert "/nowhere/chrome" in why # ── tabs ──────────────────────────────────────────────────────────────── @@ -380,6 +384,11 @@ class _FakeContext: def __init__(self, pages: list[Any]) -> None: self.pages = pages + async def new_page(self) -> _FakePage: + page = _FakePage("about:blank", f"tab{len(self.pages)}") + self.pages.append(page) + return page + def _with_pages(b: Browser, pages: list[_FakePage], active: int = 0) -> None: b._s.context = _FakeContext(pages) @@ -733,12 +742,12 @@ async def test_an_owner_keeps_its_tab_and_can_hand_it_back() -> None: first, second = _ActingPage("https://a.test", "A"), _ActingPage("https://b.test", "B") _driving(b, [first, second]) - assert b.url_for("run:a") == "https://a.test", "an unbound owner reads the active tab" + assert b.url_for("run:a") == "", "an owner with no binding has no page of its own" b._s.owners["run:a"] = _Owner(second, time.monotonic()) assert b.url_for("run:a") == "https://b.test" b.release("run:a") - assert b.url_for("run:a") == "https://a.test" + assert b.url_for("run:a") == "", "and releasing it hands back nothing, not the panel" assert not second.is_closed(), "releasing a binding leaves the tab open" @@ -947,3 +956,84 @@ async def new_page(self) -> Any: assert b._s.page is page and page is not held assert streams == ["restream"], "an act fronts the new tab exactly once" + + +async def test_an_unbound_owner_has_no_site_and_keeps_the_panel_out_of_it() -> None: + """A caller that keys an action on an owner's url must not be handed the + panel's page when that owner has no binding: the panel may be showing + another owner's tab, and the caller -- the permission gate -- would then ask + the person about a site the call is not going to touch.""" + b = get_browser() + other, panel = _FakePage("https://mine.test/"), _FakePage("https://bank.test/") + _with_pages(b, [other, panel], active=1) + b._s.owners["run:other"] = _Owner(other, time.monotonic()) + + assert b.url_for("run:absent") == "", "an owner with no binding has no site" + assert b.url_for(None) == "https://bank.test/", "the reader still reads the panel" + assert b.url_for("run:other") == "https://mine.test/" + + +async def test_a_stale_binding_is_dropped_rather_than_handed_the_panel() -> None: + """Past OWNER_IDLE_S a binding is dead, but it is still that owner's own. + Reaping it and then binding the caller to whatever the panel shows would + move the call onto somebody else's tab, and a sibling's live binding is + untouched by the reap either way.""" + b = get_browser() + stale, held = _FakePage("https://mine.test/"), _FakePage("https://theirs.test/") + _driving(b, [stale, held], active=1) + b._s.owners["run:x"] = _Owner(stale, time.monotonic() - driver_module.OWNER_IDLE_S - 1) + b._s.owners["run:other"] = _Owner(held, time.monotonic()) + b._wire = lambda page: None # type: ignore[method-assign] + + page = await b._page_for("run:x") + + assert page is not stale, "a stale binding is left behind" + assert page is not held, "and the panel's tab is not simply handed over" + assert b._s.owners["run:other"].page is held, "the sibling keeps its own" + assert b.url_for("run:x") == page.url + + +async def test_every_way_a_binding_ends_names_the_owner_to_the_listener() -> None: + """A per-owner store outside the driver is told on the same event the driver + drops the binding -- an explicit release, a reap, and the owner's tab + closing -- and is never told about an owner that was never bound.""" + b = get_browser() + first, second, third = (_FakePage(f"https://{n}.test/") for n in ("a", "b", "c")) + _driving(b, [first, second, third]) + b._wire = lambda page: None # type: ignore[method-assign] + seen: list[str] = [] + b.on_owner_released = seen.append + + b.release("never-bound") + assert seen == [], "nothing to announce" + + b._s.owners["run:a"] = _Owner(first, time.monotonic()) + b.release("run:a") + assert seen == ["run:a"] + + seen.clear() + b._s.owners["run:b"] = _Owner(second, time.monotonic() - driver_module.OWNER_IDLE_S - 1) + await b._page_for("run:c", act=False) + assert seen == ["run:b"], "a reaped binding is announced, and only it" + + seen.clear() + b._s.owners["run:c"] = _Owner(third, time.monotonic()) + await b.tab_close(2, owner=None) + assert seen == ["run:c"], "closing the tab an owner held announces it" + + +async def test_a_listener_that_raises_does_not_stop_the_binding_from_ending() -> None: + """The listener is a caller's bookkeeping; a bug in it must not leave the + driver holding a binding it has decided to drop.""" + b = get_browser() + page = _FakePage("https://a.test/") + _driving(b, [page]) + b._s.owners["run:a"] = _Owner(page, time.monotonic()) + + def explode(owner: str) -> None: + raise RuntimeError("listener is broken") + + b.on_owner_released = explode + b.release("run:a") + + assert "run:a" not in b._s.owners diff --git a/tests/test_browser_tools.py b/tests/test_browser_tools.py index e82d0c359..0aa5fd2f0 100644 --- a/tests/test_browser_tools.py +++ b/tests/test_browser_tools.py @@ -152,16 +152,40 @@ def test_a_site_grant_covers_click_and_type_alike() -> None: def test_acting_tools_write_the_site_into_the_call(monkeypatch: pytest.MonkeyPatch) -> None: """The gate only sees parameters, so the site rides in them; a site the - model wrote itself is replaced, never trusted.""" + model wrote itself is replaced, never trusted. The site is the owner's own + page, so an owner that has none contributes none: the panel may be showing + somebody else's tab, and keying the call on that site would ask the person + to approve a site the call is not going to act on.""" b = get_browser() p = _FakePage("https://shop.example.com/cart") _running(b, [p]) monkeypatch.setattr(tools_mod, "current_owner", lambda: "session:x") + b._s.owners["session:x"] = _Owner(p, time.monotonic()) out = BrowserClickTool().cast_params({"ref": "ref_2", "site": "attacker.test"}) assert out == {"ref": "ref_2", "site": "shop.example.com"} - assert BrowserClickTool().cast_params({"ref": "ref_2"})["site"] == "shop.example.com" + + +def test_an_unbound_owner_contributes_no_site_and_the_call_stands_alone( + monkeypatch: pytest.MonkeyPatch, +) -> None: + b = get_browser() + held = _FakePage("https://bank.test/login") + _running(b, [held]) + b._s.owners["session:parent"] = _Owner(held, time.monotonic()) + monkeypatch.setattr(tools_mod, "current_owner", lambda: "run:child") + + out = BrowserPressTool().cast_params({"key": "Enter", "site": "attacker.test"}) + + assert out == {"key": "Enter"}, "neither the panel's site nor the model's own" + from raven.permissions.builtin import BROWSER_SITE_KEYED_TOOLS, action_digest, session_keys + + assert "browser_press" in BROWSER_SITE_KEYED_TOOLS + assert session_keys("browser_press", out) == (action_digest("browser_press", out),), ( + "no site means the grant is this call's own, so a later click on bank.test " + "presents a different key and a site nobody approved cannot be banked" + ) def test_no_page_means_no_site() -> None: From adb9f3651ce31efd80d638af71d3adcc10894b90 Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Fri, 2 Oct 2026 17:17:26 +0000 Subject: [PATCH 2/9] fix(*): report the page an unbound owner's call would land on The previous commit made url_for answer empty for an owner with no live binding, so the permission gate was never shown the site of a front tab such a call would take. That fixed the reported case -- the front tab held by another owner, where the call opens a blank tab of its own -- and broke a case that already worked: with the front tab the reader's own, a delegated run's first acting call landed on it while the prompt named no site, and the per-call grant it banked could be presented by an identical call landing on whatever front tab came next. url_for now predicts the page _page_for binds, through one rule both use: the owner's live tab, else the front tab unless another live owner holds it, else empty for a tab the call would open itself. A table drives each branch of that choice and asserts the reported url is where the call lands; it fails on main in the two held-front-tab cases and on the previous commit in the three where the front tab is free. Two tests the previous commit rewrote for the empty answer are restored as they stand on main, since their assertions hold again. The comment beside the site injection, the session_keys docstring and the browser guide say what a call with no site keys. Co-authored-by: Claude (claude-opus-5-5[1m]) --- docs-site/docs/browser-collaboration.md | 2 + docs-site/docs/browser-collaboration.zh.md | 2 + raven/agent/tools/browser.py | 5 +- raven/browser/driver.py | 55 ++++++++++------ raven/permissions/builtin.py | 7 +- tests/test_browser_driver.py | 75 ++++++++++++---------- tests/test_browser_tools.py | 15 ++--- 7 files changed, 98 insertions(+), 63 deletions(-) diff --git a/docs-site/docs/browser-collaboration.md b/docs-site/docs/browser-collaboration.md index 47bc53dd7..c3dbea86b 100644 --- a/docs-site/docs/browser-collaboration.md +++ b/docs-site/docs/browser-collaboration.md @@ -98,6 +98,8 @@ that an external Claude Code session, for example, uses the panel's Chromium. Check the effective [permission mode](permissions.md): `full` skips ordinary ask-tier prompts. Site-scoped consent is broader than one button, and the approval may show a ref and site rather than a human-readable element label. +An action that would open a fresh tab has no site yet: its prompt names none, +and a session grant for it covers only an identical call. Disable tools through `tools.disabledTools` or explicit permission rules when they must not be available. diff --git a/docs-site/docs/browser-collaboration.zh.md b/docs-site/docs/browser-collaboration.zh.md index 1135a000c..9484f8e4a 100644 --- a/docs-site/docs/browser-collaboration.zh.md +++ b/docs-site/docs/browser-collaboration.zh.md @@ -82,6 +82,8 @@ Agent 的动作会将自己的页切到前台，单纯读取不会。其他 owne 检查实际[权限模式](permissions.md)：`full` 跳过普通 ask-tier 提示。 站点授权比单个按钮更宽；提示可能显示 ref 和站点，而不是元素的可读标签。 +会新开标签页的动作此时还没有站点：提示中不显示站点，对它的会话授权只覆盖参数 +完全相同的调用。 必须不可用的工具应通过 `tools.disabledTools` 或明确权限规则禁用。 导航接受 HTTP(S) 和空白页，拒绝危险 scheme 和字面 link-local 目标。 diff --git a/raven/agent/tools/browser.py b/raven/agent/tools/browser.py index 1f6a09762..649c6f712 100644 --- a/raven/agent/tools/browser.py +++ b/raven/agent/tools/browser.py @@ -56,7 +56,10 @@ # person approves for a click is the site, not the ref id -- so the site the # call will land on is written into the parameters before the gate reads them. # ``raven.permissions.builtin.session_keys`` keys a browser grant on it, and -# the approval prompt shows it. +# the approval prompt shows it. The driver's ``url_for`` predicts that page by +# the rule ``_page_for`` then binds by. When the call would open a tab of its +# own, no site describes it yet and none is written: the grant is keyed on the +# call itself, which no later call to a real site can present. def _browser(): diff --git a/raven/browser/driver.py b/raven/browser/driver.py index 147bfc348..655cc91a6 100644 --- a/raven/browser/driver.py +++ b/raven/browser/driver.py @@ -519,23 +519,42 @@ def owner_of(self, page: Any) -> str | None: return key return None + def _landing(self, owner: str, now: float) -> Any | None: + """The page a call for ``owner`` lands on now, or None for a new tab. + + The one rule both questions about it go through: ``_page_for`` binds + the answer and ``url_for`` reports it without binding. The permission + gate keys an acting call on that report before the call runs, so the + site a person is asked about is only the site acted on if the same + rule picks the page both times. Only live bindings count, which is the + view ``_page_for`` acts on once it has reaped. + """ + rec = self._s.owners.get(owner) + if rec is not None and self._owner_live(rec, now): + return rec.page + active = self._s.page + held = {id(r.page) for k, r in self._s.owners.items() if k != owner and self._owner_live(r, now)} + if id(active) not in held: + return active + return None + def url_for(self, owner: str | None) -> str: - """Where an owner's tab is, without starting anything or rebinding. - - An owner already bound to a live page reads that page. An owner with no - live binding answers empty rather than the panel's active page: the - panel may be showing somebody else's tab, and a caller that keys an - action on this url -- the permission gate does -- would otherwise ask - about a site the call is not going to touch. Empty is the answer that - says "this owner has no page yet", and the caller decides what that - means. + """Where a call for this owner would act, without starting or binding. + + The owner's own tab while it holds a live one; otherwise the page + ``_page_for`` would hand it -- the front tab, unless another live owner + holds that -- and empty when the call would have to open a tab of its + own, which no site describes yet, or when no browser is running and the + call would start one. A prediction, made before the call: + whatever moves in between (the reader switches tabs, another owner + takes the front one, the binding idles past ``OWNER_IDLE_S`` while a + person decides) moves the call with it. The reader, ``None``, reads + the front tab. """ if owner is None: return self.url - rec = self._s.owners.get(owner) - if rec is not None and self._owner_live(rec, time.monotonic()): - return rec.page.url - return "" + page = self._landing(owner, time.monotonic()) + return page.url if page is not None else "" async def _page_for(self, owner: str | None, *, act: bool = True) -> Any: """The page a caller works on, binding an owner on its first call. @@ -556,12 +575,10 @@ async def _page_for(self, owner: str | None, *, act: bool = True) -> Any: rec.seen = now page = rec.page else: - held = {id(r.page) for k, r in self._s.owners.items() if k != owner} - if id(active) not in held: - page = active - elif len(self._pages()) >= MAX_TABS: - raise BrowserBusyError(f"tab limit reached ({MAX_TABS}); close one before opening another") - else: + page = self._landing(owner, now) + if page is None: + if len(self._pages()) >= MAX_TABS: + raise BrowserBusyError(f"tab limit reached ({MAX_TABS}); close one before opening another") self._s.spawning += 1 try: page = await self._s.context.new_page() diff --git a/raven/permissions/builtin.py b/raven/permissions/builtin.py index 80ce65589..54fa3201c 100644 --- a/raven/permissions/builtin.py +++ b/raven/permissions/builtin.py @@ -110,8 +110,11 @@ def session_keys(tool_name: str, params: dict[str, Any], ask_segments: tuple[str path names another file once the directory is rebound. The browser's acting tools share one key per site: what the human approved was letting the agent work on that site, and a click and the typing that follows it are one such - piece of work, not two. Every other tool keys the exact call -- a `path` - field on an unknown tool says nothing about what its other fields do. + piece of work, not two. One that names no site -- the page it acts on has + none yet: a tab it is about to open, or one still blank -- keys the exact + call, so its grant is never presented by a call to a real site. Every other + tool keys the exact call -- a `path` field on an unknown tool says nothing + about what its other fields do. """ if tool_name == "exec" and ask_segments: working_dir = params.get("working_dir") diff --git a/tests/test_browser_driver.py b/tests/test_browser_driver.py index 1e9c22db8..6dc72171f 100644 --- a/tests/test_browser_driver.py +++ b/tests/test_browser_driver.py @@ -742,12 +742,12 @@ async def test_an_owner_keeps_its_tab_and_can_hand_it_back() -> None: first, second = _ActingPage("https://a.test", "A"), _ActingPage("https://b.test", "B") _driving(b, [first, second]) - assert b.url_for("run:a") == "", "an owner with no binding has no page of its own" + assert b.url_for("run:a") == "https://a.test", "an unbound owner reads the active tab" b._s.owners["run:a"] = _Owner(second, time.monotonic()) assert b.url_for("run:a") == "https://b.test" b.release("run:a") - assert b.url_for("run:a") == "", "and releasing it hands back nothing, not the panel" + assert b.url_for("run:a") == "https://a.test" assert not second.is_closed(), "releasing a binding leaves the tab open" @@ -958,39 +958,48 @@ async def new_page(self) -> Any: assert streams == ["restream"], "an act fronts the new tab exactly once" -async def test_an_unbound_owner_has_no_site_and_keeps_the_panel_out_of_it() -> None: - """A caller that keys an action on an owner's url must not be handed the - panel's page when that owner has no binding: the panel may be showing - another owner's tab, and the caller -- the permission gate -- would then ask - the person about a site the call is not going to touch.""" +@pytest.mark.parametrize( + ("bindings", "lands_on"), + [ + pytest.param({}, "front", id="front-tab-is-the-readers"), + pytest.param({"run:other": ("front", 0)}, "new", id="front-tab-held-by-another-owner"), + pytest.param({"run:x": ("mine", 0)}, "mine", id="caller-holds-its-own-tab"), + pytest.param({"run:x": ("mine", "idle")}, "front", id="caller-idled-out-front-tab-free"), + pytest.param( + {"run:x": ("mine", "idle"), "run:other": ("front", 0)}, "new", id="caller-idled-out-front-tab-held" + ), + pytest.param({"run:x": ("mine", "closed")}, "front", id="callers-tab-was-closed"), + pytest.param({"run:other": ("front", "idle")}, "front", id="front-tab-held-by-an-idled-out-owner"), + ], +) +async def test_the_url_reported_for_an_owner_is_where_its_call_lands( + bindings: dict[str, tuple[str, Any]], lands_on: str +) -> None: + """The permission gate keys an acting call on ``url_for`` before the call + runs, so the site a person approves is the site acted on only if this + report and ``_page_for`` pick the same page. One case per branch of that + choice; a call that must open a tab of its own is reported empty, since no + site describes a page nobody has opened yet.""" b = get_browser() - other, panel = _FakePage("https://mine.test/"), _FakePage("https://bank.test/") - _with_pages(b, [other, panel], active=1) - b._s.owners["run:other"] = _Owner(other, time.monotonic()) - - assert b.url_for("run:absent") == "", "an owner with no binding has no site" - assert b.url_for(None) == "https://bank.test/", "the reader still reads the panel" - assert b.url_for("run:other") == "https://mine.test/" - - -async def test_a_stale_binding_is_dropped_rather_than_handed_the_panel() -> None: - """Past OWNER_IDLE_S a binding is dead, but it is still that owner's own. - Reaping it and then binding the caller to whatever the panel shows would - move the call onto somebody else's tab, and a sibling's live binding is - untouched by the reap either way.""" - b = get_browser() - stale, held = _FakePage("https://mine.test/"), _FakePage("https://theirs.test/") - _driving(b, [stale, held], active=1) - b._s.owners["run:x"] = _Owner(stale, time.monotonic() - driver_module.OWNER_IDLE_S - 1) - b._s.owners["run:other"] = _Owner(held, time.monotonic()) + pages = {"mine": _FakePage("https://mine.test/"), "front": _FakePage("https://bank.test/")} + _driving(b, [pages["mine"], pages["front"]], active=1) b._wire = lambda page: None # type: ignore[method-assign] - - page = await b._page_for("run:x") - - assert page is not stale, "a stale binding is left behind" - assert page is not held, "and the panel's tab is not simply handed over" - assert b._s.owners["run:other"].page is held, "the sibling keeps its own" - assert b.url_for("run:x") == page.url + for owner, (name, age) in bindings.items(): + if age == "closed": + pages[name]._closed = True + seen = time.monotonic() - (driver_module.OWNER_IDLE_S + 1 if age == "idle" else 0) + b._s.owners[owner] = _Owner(pages[name], seen) + before = set(map(id, pages.values())) + + reported = b.url_for("run:x") + landed = await b._page_for("run:x") + + if lands_on == "new": + assert id(landed) not in before, "the call opened a tab of its own" + assert reported == "" + else: + assert landed is pages[lands_on] + assert reported == landed.url async def test_every_way_a_binding_ends_names_the_owner_to_the_listener() -> None: diff --git a/tests/test_browser_tools.py b/tests/test_browser_tools.py index 0aa5fd2f0..2fdfd3f06 100644 --- a/tests/test_browser_tools.py +++ b/tests/test_browser_tools.py @@ -152,24 +152,23 @@ def test_a_site_grant_covers_click_and_type_alike() -> None: def test_acting_tools_write_the_site_into_the_call(monkeypatch: pytest.MonkeyPatch) -> None: """The gate only sees parameters, so the site rides in them; a site the - model wrote itself is replaced, never trusted. The site is the owner's own - page, so an owner that has none contributes none: the panel may be showing - somebody else's tab, and keying the call on that site would ask the person - to approve a site the call is not going to act on.""" + model wrote itself is replaced, never trusted.""" b = get_browser() p = _FakePage("https://shop.example.com/cart") _running(b, [p]) monkeypatch.setattr(tools_mod, "current_owner", lambda: "session:x") - b._s.owners["session:x"] = _Owner(p, time.monotonic()) out = BrowserClickTool().cast_params({"ref": "ref_2", "site": "attacker.test"}) assert out == {"ref": "ref_2", "site": "shop.example.com"} + assert BrowserClickTool().cast_params({"ref": "ref_2"})["site"] == "shop.example.com" -def test_an_unbound_owner_contributes_no_site_and_the_call_stands_alone( - monkeypatch: pytest.MonkeyPatch, -) -> None: +def test_a_call_that_would_open_its_own_tab_carries_no_site(monkeypatch: pytest.MonkeyPatch) -> None: + """With the front tab another owner's, this owner's first call opens a tab + of its own, so no site describes where it acts yet. None is written -- not + the front tab's, and not one the model supplied -- and the grant is then + keyed on the call itself.""" b = get_browser() held = _FakePage("https://bank.test/login") _running(b, [held]) From 63030cd272cddd23ea478ef8b1d30d85934fd834 Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Fri, 2 Oct 2026 17:17:32 +0000 Subject: [PATCH 3/9] fix(browser): announce the bindings a browser close ends close() replaces the driver's state wholesale, so every owner binding it held ended without passing through _drop_owners, and the tools' stamp map kept those owners for the life of the process. The pop-out flow is a close and a relaunch, so this ran in ordinary use. The bindings are now dropped through _drop_owners before the state is replaced, and the comment on the listener gives the reason it lives on the driver. The listener test now covers four endings, taken from the driver's own removal sites -- a release, a reap, the owner's tab closing, the browser closing -- under a name that no longer claims every way. Co-authored-by: Claude (claude-opus-5-5[1m]) --- raven/browser/driver.py | 18 ++++++++++++------ tests/test_browser_driver.py | 13 ++++++++++--- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/raven/browser/driver.py b/raven/browser/driver.py index 655cc91a6..20042633e 100644 --- a/raven/browser/driver.py +++ b/raven/browser/driver.py @@ -219,9 +219,9 @@ def __init__(self) -> None: self._launch_lock = asyncio.Lock() # Told the name of every owner this driver stops binding to a page, so # whatever a caller keeps per owner can drop its copy on the same event - # instead of on a second, independently drifting clock. An attribute - # rather than a state field because the bindings outlive a close(), and - # a listener registered before one would otherwise be dropped by it. + # instead of on a second, independently drifting clock. Held here + # rather than in `_State`, which close() replaces: the store listening + # outlives any one browser session, as the launch lock above does. self.on_owner_released: Any = None # ---- lifecycle --------------------------------------------------------- @@ -614,9 +614,10 @@ def _drop_owners(self, owners: list[str]) -> None: """Remove these bindings, and tell the listener about the ones there were. Whatever ends a binding -- a reap, an explicit release, the tab it was - on closing -- goes through here, so the listener is told once per owner - under one rule rather than at each call site. An owner that was never - bound is not announced: there is nothing to have lost. + on closing, the browser closing -- goes through here, so the listener + is told once per owner under one rule rather than at each call site. An + owner that was never bound is not announced: there is nothing to have + lost. """ gone = [owner for owner in owners if self._s.owners.pop(owner, None) is not None] if self.on_owner_released is None: @@ -798,6 +799,11 @@ async def close(self) -> None: await s.playwright.stop() except Exception: pass + # Announced before the state is replaced: closing ends every binding it + # held, and a per-owner store keyed against them outlives the state -- + # the pop-out flow is close-and-relaunch, and a relaunch continues the + # same owners. A fresh `_State` would otherwise strand those keys. + self._drop_owners(list(s.owners)) self._s = _State() logger.info("browser: closed") diff --git a/tests/test_browser_driver.py b/tests/test_browser_driver.py index 6dc72171f..a352fb06e 100644 --- a/tests/test_browser_driver.py +++ b/tests/test_browser_driver.py @@ -1002,10 +1002,12 @@ async def test_the_url_reported_for_an_owner_is_where_its_call_lands( assert reported == landed.url -async def test_every_way_a_binding_ends_names_the_owner_to_the_listener() -> None: +async def test_a_binding_ends_names_the_owner_to_the_listener_however_it_ended() -> None: """A per-owner store outside the driver is told on the same event the driver - drops the binding -- an explicit release, a reap, and the owner's tab - closing -- and is never told about an owner that was never bound.""" + drops the binding. Four endings, and the set is the driver's own removal + sites rather than a list: an explicit release, a reap, the owner's tab + closing, and the whole browser closing. An owner that was never bound is + not announced -- there is nothing to have lost.""" b = get_browser() first, second, third = (_FakePage(f"https://{n}.test/") for n in ("a", "b", "c")) _driving(b, [first, second, third]) @@ -1030,6 +1032,11 @@ async def test_every_way_a_binding_ends_names_the_owner_to_the_listener() -> Non await b.tab_close(2, owner=None) assert seen == ["run:c"], "closing the tab an owner held announces it" + seen.clear() + b._s.owners["run:d"] = _Owner(first, time.monotonic()) + await b.close() + assert seen == ["run:d"], "closing the browser ends the bindings it still held" + async def test_a_listener_that_raises_does_not_stop_the_binding_from_ending() -> None: """The listener is a caller's bookkeeping; a bug in it must not leave the From 2a1c19fb98bde350cf18c1407edf4cba4ef0e64b Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Fri, 2 Oct 2026 17:17:38 +0000 Subject: [PATCH 4/9] fix(agent): order the stamp cap by recency and pin its driver wiring Deleting the call that registers the stamp map with the driver left every browser test green: nothing exercised the prune once the map held an entry. A test now marks an owner, releases its binding and asserts the stamp is gone. The cap evicted by sorting timestamps, which can tie for two stamps one clock tick apart -- the tick is about 15 ms on Windows -- and then left the victim to dict order. Each mark now re-inserts its owner, so the map's own order is recency and eviction pops from the front with no sort. The guard that skipped re-registering compared bound methods by identity, a new object on every access, so it never skipped; it is gone. Co-authored-by: Claude (claude-opus-5-5[1m]) --- raven/agent/tools/browser.py | 40 ++++++++++++++++++------------------ tests/test_browser_tools.py | 36 ++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 20 deletions(-) diff --git a/raven/agent/tools/browser.py b/raven/agent/tools/browser.py index 649c6f712..5a38ac49d 100644 --- a/raven/agent/tools/browser.py +++ b/raven/agent/tools/browser.py @@ -37,10 +37,11 @@ SNAPSHOT_TEXT_CHARS = 8_000 MAX_REFS_SHOWN = 120 -# How many owners' stamps ``_acted`` keeps. The stamps only answer "did the -# reader touch the page since I last acted", which the newest few owners can -# answer as well as all of them; the cap is what stops one key per delegated run -# from accumulating for the life of the process. +# How many owners' stamps ``_acted`` keeps, most recent first. The stamps only +# answer "did the reader touch the page since this owner last acted", which the +# owners acting lately can answer as well as all of them. The driver prunes an +# owner when it drops that owner's binding; the cap is for the owners it never +# drops, so one key per delegated run cannot accumulate for the process's life. _ACTED_MAX = 512 # Carried by browser_navigate alone. Every tool's description is paid for on @@ -201,14 +202,14 @@ def _owner(self) -> str: def _mark(self, owner: str) -> None: acted = _BrowserTool._acted + # Re-inserted rather than updated, so the dict's own order is the order + # owners last acted in and the cap can evict from the front: no sort, + # and no tie to break between two stamps one clock tick apart. + acted.pop(owner, None) acted[owner] = time.monotonic() self._bind_to_driver() - if len(acted) > _ACTED_MAX: - # Oldest-first, and only ever to the cap: the keys are owner names - # that a finished delegated run never speaks again, so without this - # one key per run outlives the process. - for stale in sorted(acted, key=lambda k: acted[k])[: len(acted) - _ACTED_MAX]: - acted.pop(stale, None) + while len(acted) > _ACTED_MAX: + acted.pop(next(iter(acted))) def _touched(self, owner: str) -> bool: last = _BrowserTool._acted.get(owner) @@ -219,17 +220,16 @@ def _forget_owner(cls, owner: str) -> None: cls._acted.pop(owner, None) def _bind_to_driver(self) -> None: - """Register this owner's store with the driver that ends it. - - Lazy and idempotent: the driver is the process-wide one, and reaching - for it is what builds it, so doing this at import would have the tools - module construct the shared browser as a side effect. The driver is the - one place that knows when an owner's tab binding ends, so the stamp map - is pruned from there rather than on a second clock of its own. + """Have the driver tell the stamp map when it drops an owner's binding. + + Called from ``_mark``, the map's only writer, so no owner can hold a + stamp before the driver knows to prune it. Not at import: reaching for + the driver is what builds the process-wide browser, and importing this + module must not construct one as a side effect. The driver is the one + place that knows when an owner's binding ends, so the map is pruned on + that event rather than on a second clock of its own. """ - browser = _browser() - if browser.on_owner_released is not _BrowserTool._forget_owner: - browser.on_owner_released = _BrowserTool._forget_owner + _browser().on_owner_released = _BrowserTool._forget_owner async def _readback(self, owner: str, state: dict[str, Any], *, acted: bool) -> ToolResult: """State plus a compact snapshot; the snapshot is skipped on an error diff --git a/tests/test_browser_tools.py b/tests/test_browser_tools.py index 2fdfd3f06..8a285dbe2 100644 --- a/tests/test_browser_tools.py +++ b/tests/test_browser_tools.py @@ -769,3 +769,39 @@ def test_site_keyed_permission_set_matches_the_acting_tool_hierarchy() -> None: acting = {cls().name for cls in tools_mod._ActingTool.__subclasses__()} assert acting == set(BROWSER_SITE_KEYED_TOOLS) + + +def test_the_stamp_store_is_pruned_by_the_driver_once_it_holds_an_entry( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The store registers with the driver from its only writer, so an owner + that holds a stamp is an owner the driver will prune -- and not at import, + where reaching for the driver would build the process-wide browser. This + drives the release a run's end would cause and asserts the stamp is gone; + an unregistered store would keep it for the life of the process.""" + b = get_browser() + page = _FakePage("https://a.test/") + _running(b, [page]) + assert b.on_owner_released is None, "nothing has reached the driver yet" + + BrowserPressTool()._mark("run:r1") + b._s.owners["run:r1"] = _Owner(page, time.monotonic()) + + assert b.on_owner_released == tools_mod._BrowserTool._forget_owner + b.release("run:r1") + assert "run:r1" not in tools_mod._BrowserTool._acted + + +def test_the_stamp_store_keeps_the_owners_that_acted_most_recently(monkeypatch: pytest.MonkeyPatch) -> None: + """An owner the driver never drops is still evicted once the store is + full, least recent first, and acting again moves an owner to the back.""" + monkeypatch.setattr(tools_mod, "_ACTED_MAX", 3) + _running(get_browser(), [_FakePage("https://a.test/")]) + tool = BrowserPressTool() + + for owner in ("run:a", "run:b", "run:c"): + tool._mark(owner) + tool._mark("run:a") + tool._mark("run:d") + + assert list(tools_mod._BrowserTool._acted) == ["run:c", "run:a", "run:d"] From 80259d65ab61ecc0daa6b0c259c7a198e9d8687a Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Fri, 2 Oct 2026 17:17:45 +0000 Subject: [PATCH 5/9] test(agent): pin the close rule the tabs description promises The previous commit told the model that close takes only a tab it holds and that an unmarked tab is the user's, and no test noticed when that sentence was deleted. The test takes the recovery from the driver's own refusal and asserts the description states the same rule. Co-authored-by: Claude (claude-opus-5-5[1m]) --- tests/test_browser_tools.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/test_browser_tools.py b/tests/test_browser_tools.py index 8a285dbe2..6e75c6f3e 100644 --- a/tests/test_browser_tools.py +++ b/tests/test_browser_tools.py @@ -805,3 +805,21 @@ def test_the_stamp_store_keeps_the_owners_that_acted_most_recently(monkeypatch: tool._mark("run:d") assert list(tools_mod._BrowserTool._acted) == ["run:c", "run:a", "run:d"] + + +async def test_the_tabs_description_names_the_close_rule_the_driver_enforces() -> None: + """The driver refuses an owner's close of a tab nobody holds -- the + reader's, which may carry a login the model just asked them to finish -- + and the listing marks that tab with neither ``yours`` nor ``held``. The + description is the only text the model reads before calling, so without + the rule there the refusal is the first it hears of it.""" + b = get_browser() + _running(b, [_FakePage("https://bank.test/")]) + + refused = await b.tab_close(0, owner="run:x") + + assert "activate it first" in refused["error"], "the driver's own recovery, which the description must match" + said = " ".join(BrowserTabsTool().description.split()) + assert "close takes only a tab you hold" in said + assert "a tab with no mark at all is the user's" in said + assert "closing it needs activate first" in said From d4f1252d997cdec339b84a553bf585d5cb49224c Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Fri, 2 Oct 2026 17:17:51 +0000 Subject: [PATCH 6/9] docs: say an agent activates the user's tab before closing it The browser guide said only that another owner's tab cannot be closed. A tab with no owner is refused too, because it is the user's and may hold a login they were asked to finish. Both language versions say so. Co-authored-by: Claude (claude-opus-5-5[1m]) --- docs-site/docs/browser-collaboration.md | 8 +++++--- docs-site/docs/browser-collaboration.zh.md | 4 +++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/docs-site/docs/browser-collaboration.md b/docs-site/docs/browser-collaboration.md index c3dbea86b..3d95c9b3d 100644 --- a/docs-site/docs/browser-collaboration.md +++ b/docs-site/docs/browser-collaboration.md @@ -80,9 +80,11 @@ It takes the current unowned tab or gets a fresh one. Subsequent calls stay bound to that tab even if the panel displays another one. An agent action brings its tab forward; a read alone does not. Another owner's -tab is marked held and cannot be activated or closed by that agent. Bindings -expire after ten idle minutes or when the tab closes. User panel interactions -are not restricted to a model's owner binding. +tab is marked held and cannot be activated or closed by that agent. A tab with +no owner is the user's and may hold a login they were asked to finish: an agent +closes it only after activating it, a switch the panel shows. Bindings expire +after ten idle minutes or when the tab closes. User panel interactions are not +restricted to a model's owner binding. This shares one process's browser, not every browser in the deployment. External ACP/CLI processes may have their own browser state. Do not promise diff --git a/docs-site/docs/browser-collaboration.zh.md b/docs-site/docs/browser-collaboration.zh.md index 9484f8e4a..5cb1d10a8 100644 --- a/docs-site/docs/browser-collaboration.zh.md +++ b/docs-site/docs/browser-collaboration.zh.md @@ -67,7 +67,9 @@ Owner 是当前进程内子 Agent 运行，否则是主对话。它会取得未 或新建标签页。后续调用绑定该页，即使面板显示另一页也不改变地址。 Agent 的动作会将自己的页切到前台，单纯读取不会。其他 owner 的页标为 held， -该 Agent 不能切换到它或关闭它。绑定在空闲十分钟后或标签页关闭时失效。 +该 Agent 不能切换到它或关闭它。没有 owner 的页属于用户，可能停在刚请用户完成的 +登录上：Agent 必须先切换到它（面板可见）才能关闭。绑定在空闲十分钟后或标签页 +关闭时失效。 用户面板交互不受模型 owner 绑定限制。 共享的是同一进程的浏览器，不是部署中的所有浏览器。外部 ACP/CLI 进程可能有 From faaec74dcc5afedf898c9b7381cc164edee050bb Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Fri, 2 Oct 2026 17:17:58 +0000 Subject: [PATCH 7/9] fix(browser): name the path and the knob when chromium is not found The launcher's "Executable doesn't exist" error became "Chromium is not installed", which is false when the browser is installed where this process does not look -- Playwright resolves its cache from HOME, which a sandboxed run or another account moves. The message now quotes the path it looked for, names PLAYWRIGHT_BROWSERS_PATH for a browser installed elsewhere and ends on the install command. It stays on one line, because the panel shows it in a single block. Co-authored-by: Claude (claude-opus-5-5[1m]) --- raven/browser/driver.py | 23 +++++++++++++---------- tests/test_browser_driver.py | 13 ++++++++----- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/raven/browser/driver.py b/raven/browser/driver.py index 20042633e..8c31725e1 100644 --- a/raven/browser/driver.py +++ b/raven/browser/driver.py @@ -26,6 +26,7 @@ import asyncio import base64 +import re import shlex import sys import time @@ -324,17 +325,19 @@ async def _launch_once(self) -> Any: await self.close() hint = f"{exc}" if "Executable doesn't exist" in hint or "playwright install" in hint: - # A search failure, not only a missing install. This fires when - # the browser is on disk somewhere the launcher did not look -- - # Playwright resolves its cache under HOME, which a sandboxed - # test or a different user profile can point elsewhere -- and - # "not installed" then sends the reader to download something - # they already have. Playwright's own first line names the path - # it tried, which is the one fact that tells the two apart. - tried = hint.strip().splitlines()[0] if hint.strip() else "" - detail = f"\n({tried})" if tried else "" + # "Not installed" is the right reading only when nothing is on + # disk. The same error comes back when a browser is installed + # where this process does not look -- Playwright resolves its + # cache from HOME, which a sandboxed run or another account + # moves -- and then the install line sends the reader to fetch + # what they already have. Playwright's error names the path it + # tried, which is the one fact that tells the two apart. + looked = re.search(r"Executable doesn't exist at (.+)", hint) + where = f" ({looked.group(1).strip()})" if looked else "" raise BrowserUnavailableError( - f"Chromium is not installed where this process looks for it. Run: {CHROMIUM_INSTALL_HINT}{detail}" + f"Chromium is not installed where this process looks for it{where}; if it is " + "installed elsewhere, set PLAYWRIGHT_BROWSERS_PATH to its ms-playwright directory. " + f"Otherwise run: {CHROMIUM_INSTALL_HINT}" ) from None raise BrowserUnavailableError(f"could not start Chromium: {hint}") from None logger.info("browser: chromium started ({}x{})", w, h) diff --git a/tests/test_browser_driver.py b/tests/test_browser_driver.py index a352fb06e..facaa785d 100644 --- a/tests/test_browser_driver.py +++ b/tests/test_browser_driver.py @@ -350,11 +350,14 @@ async def start(self) -> None: why = str(exc.value) assert "Chromium is not installed where this process looks for it" in why - assert f"{shlex.quote(sys.executable)} -m playwright install chromium" in why - # The path Playwright searched, quoted back. Without it the message reads - # as "not installed" whichever of the two it was, and a sandboxed HOME - # sends the reader to download a browser they already have. - assert "/nowhere/chrome" in why + assert why.endswith(f"{shlex.quote(sys.executable)} -m playwright install chromium"), "the fix closes the line" + # The path Playwright searched, quoted back, and the knob that moves the + # search: without them the message reads as "not installed" whichever of + # the two it was, and a sandboxed HOME sends the reader to download a + # browser they already have. + assert "(/nowhere/chrome)" in why + assert "PLAYWRIGHT_BROWSERS_PATH" in why + assert "\n" not in why, "the panel shows this in one block, where a newline collapses to a space" # ── tabs ──────────────────────────────────────────────────────────────── From 80a566f8eae845268a71ae89f1e202a15e225148 Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Fri, 2 Oct 2026 17:18:08 +0000 Subject: [PATCH 8/9] test(browser): follow playwright's own cache rule in the real-page helper The helper named the per-OS default and nothing else. Playwright's registryDirectory also reads XDG_CACHE_HOME on Linux and LOCALAPPDATA on Windows, takes PLAYWRIGHT_BROWSERS_PATH=0 as an install inside the package, and resolves a relative path from INIT_CWD or the working directory. Without the first two, the fixture overrode a cache Playwright would have found by itself; without the third, a hermetic install was skipped as missing. Each branch now has a case. Co-authored-by: Claude (claude-opus-5-5[1m]) --- tests/_browser_cache.py | 100 +++++++++++++++----------- tests/test_browser_cache.py | 137 ++++++++++++++++++++++++------------ 2 files changed, 150 insertions(+), 87 deletions(-) diff --git a/tests/_browser_cache.py b/tests/_browser_cache.py index 76a249146..ffe101ef3 100644 --- a/tests/_browser_cache.py +++ b/tests/_browser_cache.py @@ -1,19 +1,22 @@ -"""Where Playwright keeps the Chromium a real-page test needs, by platform. - -Two things about that look-up are easy to get wrong, and both were, in two -different files: - -* Playwright resolves its browser cache relative to ``HOME``, and this suite - redirects ``HOME`` at a per-test tmp dir (``tests/conftest.py``, so no test - reads the config of whoever is running it). A real-page test therefore has to - point the look-up back at the login's cache itself. -* The default location is per-OS -- ``~/Library/Caches/ms-playwright`` on - macOS, ``%LOCALAPPDATA%``-shaped on Windows, ``~/.cache/ms-playwright`` on - Linux -- and a fixture that hard-codes one of them is right on exactly one - platform and, worse, right for whoever wrote it. - -An explicit ``PLAYWRIGHT_BROWSERS_PATH`` always wins over both, so CI that -installs its browsers elsewhere is untouched by any of this. +"""Where Playwright keeps the Chromium a real-page test needs. + +This suite redirects ``HOME`` at a per-test tmp dir (``tests/conftest.py``, +so no test reads the config of whoever is running it), and Playwright looks +its browsers up from the home directory -- so a real-page test has to aim the +look-up back at the login's own cache before it launches anything. + +The rule mirrored here is ``registryDirectory`` in playwright-core's registry, +read from the driver bundled with playwright 1.62: + +* ``PLAYWRIGHT_BROWSERS_PATH=0`` -- a hermetic install, inside the package; +* any other value of it -- that directory, a relative one taken from + ``INIT_CWD`` or the working directory; +* otherwise ``ms-playwright`` under a per-OS cache root: ``XDG_CACHE_HOME`` + or ``~/.cache`` on Linux, ``~/Library/Caches`` on macOS, ``LOCALAPPDATA`` or + ``~/AppData/Local`` on Windows. Playwright refuses every other platform. + +Only the home directory needs correcting: the two environment variables are +not sandboxed, so where one is set Playwright already finds the cache. """ from __future__ import annotations @@ -30,43 +33,57 @@ def login_home() -> str: """The login directory, ignoring a ``HOME`` a fixture has sandboxed. - ``pwd`` rather than ``Path.home()`` because the latter follows ``HOME``, - which is the value every caller here is trying to see past. + ``pwd`` rather than ``Path.home()``, which follows ``HOME`` -- the value + every caller here is trying to see past. Without a passwd entry there is + nothing to see past it with, and ``HOME`` is the answer. """ if pwd is not None: try: return pwd.getpwuid(os.getuid()).pw_dir - except (KeyError, AttributeError): # pragma: no cover - no passwd entry + except KeyError: pass return os.path.expanduser("~") -def playwright_cache(home: str) -> str: - """The directory ``playwright install chromium`` writes to under ``home``.""" - if sys.platform == "darwin": - return os.path.join(home, "Library", "Caches", "ms-playwright") - if sys.platform == "win32": - return os.path.join(home, "AppData", "Local", "ms-playwright") - return os.path.join(home, ".cache", "ms-playwright") - +def playwright_cache(home: str) -> str | None: + """The default browser cache for ``home``, or None where Playwright has none.""" + if sys.platform == "linux": + root = os.environ.get("XDG_CACHE_HOME") or os.path.join(home, ".cache") + elif sys.platform == "darwin": + root = os.path.join(home, "Library", "Caches") + elif sys.platform == "win32": + root = os.environ.get("LOCALAPPDATA") or os.path.join(home, "AppData", "Local") + else: + return None + return os.path.join(root, "ms-playwright") -def browsers_dir() -> str: - """The browser cache this process resolves to. Safe to call at import. - Not the same question as "is Chromium installed": this is where the - look-up goes, which depends on the environment and not on the disk. - """ - return os.environ.get("PLAYWRIGHT_BROWSERS_PATH") or playwright_cache(login_home()) +def browsers_dir() -> str | None: + """The browser cache this process resolves to. Safe to call at import.""" + explicit = os.environ.get("PLAYWRIGHT_BROWSERS_PATH") + if explicit == "0": + try: + import playwright + except ImportError: + return None + return os.path.join(os.path.dirname(playwright.__file__), "driver", "package", ".local-browsers") + if explicit: + return os.path.abspath(os.path.join(os.environ.get("INIT_CWD") or os.getcwd(), explicit)) + return playwright_cache(login_home()) def chromium_installed(where: str | None = None) -> bool: - """Whether a Chromium the launcher would accept is under ``where``. + """Whether a Chromium build sits in the browser cache. A directory entry rather than ``Browser.probe()``, which only answers - whether the ``playwright`` package imports: those are different questions, - and skipping on the package alone turns a missing binary into six failures. + whether the ``playwright`` package imports: skipping on the package alone + turned a missing binary into six failures. It does not check the revision + -- a cache holding only an older build still fails the launch, loudly, and + the driver's error then names the path it looked for. """ target = where or browsers_dir() + if target is None: + return False try: return any(name.startswith("chromium") for name in os.listdir(target)) except OSError: @@ -74,15 +91,12 @@ def chromium_installed(where: str | None = None) -> bool: def point_at_login_cache(monkeypatch) -> None: - """Fixture body: aim Playwright at the login's cache, unless already aimed. - - Set before the launch rather than after, because the sandboxed ``HOME`` is - already in place by the time a fixture runs and the first ``goto`` is what - starts Chromium. - """ + """Fixture body: aim Playwright at the login's cache, unless already aimed.""" if os.environ.get("PLAYWRIGHT_BROWSERS_PATH"): return - monkeypatch.setenv("PLAYWRIGHT_BROWSERS_PATH", playwright_cache(login_home())) + cache = playwright_cache(login_home()) + if cache is not None: + monkeypatch.setenv("PLAYWRIGHT_BROWSERS_PATH", cache) __all__ = [ diff --git a/tests/test_browser_cache.py b/tests/test_browser_cache.py index 866099a55..99b9394d0 100644 --- a/tests/test_browser_cache.py +++ b/tests/test_browser_cache.py @@ -1,9 +1,11 @@ """The Playwright browser-cache look-up the real-page suites share. -Both of those suites need the cache under the *login's* home, because the -suite redirects ``HOME`` at a per-test tmp dir and Playwright resolves the -cache from ``HOME``. Getting it wrong is not loud: the run reports Chromium -missing and suggests an install that would write somewhere else again. +Those suites need the cache Playwright would use for the *login's* home: the +suite redirects ``HOME`` at a per-test tmp dir, and Playwright resolves the +cache from the home directory. Getting it wrong is quiet -- the run reports +Chromium missing and suggests an install that writes somewhere else again -- +so the cases below are the branches of Playwright's own rule, not a list of +the platforms someone happened to test on. """ from __future__ import annotations @@ -13,64 +15,111 @@ import pytest -from tests._browser_cache import browsers_dir, chromium_installed, login_home, playwright_cache - - -def test_the_cache_is_named_per_platform() -> None: - """The three defaults Playwright documents. Hard-coding one of them is - right on exactly one OS, and right for whoever wrote it.""" - assert playwright_cache("/home/u") == os.path.join("/home/u", ".cache", "ms-playwright") - if sys.platform == "darwin": - assert playwright_cache("/Users/u") == "/Users/u/Library/Caches/ms-playwright" - pytest.skip("the other two branches are the two below, checked by the platform not taken") - if sys.platform == "win32": - assert playwright_cache("C:/Users/u") == os.path.join("C:/Users/u", "AppData", "Local", "ms-playwright") +from tests._browser_cache import ( + browsers_dir, + chromium_installed, + login_home, + playwright_cache, + point_at_login_cache, +) @pytest.mark.parametrize( - ("platform", "expected"), + ("platform", "env", "expected"), [ - ("darwin", "Library/Caches/ms-playwright"), - ("win32", "AppData/Local/ms-playwright"), - ("linux", ".cache/ms-playwright"), + pytest.param("linux", {}, ("", ".cache", "ms-playwright"), id="linux"), + pytest.param("linux", {"XDG_CACHE_HOME": "/xdg"}, ("/xdg", "ms-playwright"), id="linux-xdg"), + pytest.param("darwin", {}, ("", "Library", "Caches", "ms-playwright"), id="darwin"), + pytest.param("win32", {}, ("", "AppData", "Local", "ms-playwright"), id="win32"), + pytest.param("win32", {"LOCALAPPDATA": "/lad"}, ("/lad", "ms-playwright"), id="win32-localappdata"), + pytest.param("sunos5", {}, None, id="a-platform-playwright-refuses"), ], ) -def test_every_branch_of_the_platform_switch_is_reachable(monkeypatch, platform, expected) -> None: - """Which branch runs is `sys.platform`, so exercise all three rather than - only the one this machine happens to be.""" +def test_the_default_cache_follows_playwright_s_rule( + monkeypatch: pytest.MonkeyPatch, platform: str, env: dict[str, str], expected: tuple[str, ...] | None +) -> None: monkeypatch.setattr(sys, "platform", platform) + for name in ("XDG_CACHE_HOME", "LOCALAPPDATA"): + monkeypatch.delenv(name, raising=False) + for name, value in env.items(): + monkeypatch.setenv(name, value) + + got = playwright_cache("/h") + + if expected is None: + assert got is None + else: + assert got == os.path.join(*("/h" if part == "" else part for part in expected)) + + +def test_an_explicit_cache_path_wins(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("PLAYWRIGHT_BROWSERS_PATH", "/elsewhere/ms-playwright") - assert playwright_cache("/h") == os.path.join("/h", expected) + assert browsers_dir() == os.path.abspath("/elsewhere/ms-playwright") -def test_the_login_home_ignores_a_sandboxed_home(monkeypatch) -> None: - """`Path.home()` and `expanduser` both follow `HOME`, which is the value - every caller here is trying to see past.""" +def test_a_relative_cache_path_is_taken_from_init_cwd_or_the_working_directory( + monkeypatch: pytest.MonkeyPatch, tmp_path +) -> None: + monkeypatch.setenv("PLAYWRIGHT_BROWSERS_PATH", os.path.join("rel", "pw")) + monkeypatch.delenv("INIT_CWD", raising=False) + monkeypatch.chdir(tmp_path) + assert browsers_dir() == str(tmp_path / "rel" / "pw") + + monkeypatch.setenv("INIT_CWD", str(tmp_path / "init")) + assert browsers_dir() == str(tmp_path / "init" / "rel" / "pw") + + +def test_a_hermetic_install_is_looked_for_inside_the_package(monkeypatch: pytest.MonkeyPatch) -> None: + playwright = pytest.importorskip("playwright") + monkeypatch.setenv("PLAYWRIGHT_BROWSERS_PATH", "0") + + assert browsers_dir() == os.path.join( + os.path.dirname(playwright.__file__), "driver", "package", ".local-browsers" + ), "a literal '0' is a directory name to nothing but Playwright" + + +def test_the_login_home_sees_past_a_sandboxed_home(monkeypatch: pytest.MonkeyPatch) -> None: + pwd = pytest.importorskip("pwd") + try: + real = pwd.getpwuid(os.getuid()).pw_dir + except KeyError: + pytest.skip("this account has no passwd entry, so HOME is all there is to read") monkeypatch.setenv("HOME", "/tmp/sandboxed") - home = login_home() + assert login_home() == real - assert home != "/tmp/sandboxed" - assert os.path.isdir(home), "and it is a real directory, not a guess" +def test_the_fixture_aims_playwright_at_the_login_cache_unless_already_aimed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.delenv("PLAYWRIGHT_BROWSERS_PATH", raising=False) + point_at_login_cache(monkeypatch) + assert os.environ["PLAYWRIGHT_BROWSERS_PATH"] == playwright_cache(login_home()) + + monkeypatch.setenv("PLAYWRIGHT_BROWSERS_PATH", "/chosen") + point_at_login_cache(monkeypatch) + assert os.environ["PLAYWRIGHT_BROWSERS_PATH"] == "/chosen", "a caller's own choice is left alone" -def test_an_explicit_cache_path_wins(monkeypatch) -> None: - monkeypatch.setenv("PLAYWRIGHT_BROWSERS_PATH", "/elsewhere/ms-playwright") - assert browsers_dir() == "/elsewhere/ms-playwright" +def test_a_chromium_directory_is_what_counts_as_installed(tmp_path) -> None: + """The old skip asked ``Browser.probe()``, which only tests the package, + so a missing binary failed six tests instead of skipping them.""" + cache = tmp_path / "ms-playwright" + cache.mkdir() + (cache / "ffmpeg-1011").mkdir() + assert chromium_installed(str(cache)) is False, "another download is not a browser" + (cache / "chromium_headless_shell-1234").mkdir() + assert chromium_installed(str(cache)) is True -def test_the_skip_only_fires_when_there_is_no_chromium(monkeypatch, tmp_path) -> None: - """The old skip asked `Browser.probe()`, which only tests the `playwright` - package -- so a missing binary failed six tests instead of skipping them.""" - empty = tmp_path / "ms-playwright" - empty.mkdir() - assert chromium_installed(str(empty)) is False + assert chromium_installed(str(tmp_path / "absent")) is False - (empty / "chromium-1234").mkdir() - assert chromium_installed(str(empty)) is True - (empty / "chromium_headless_shell-1234").mkdir() - assert chromium_installed(str(empty)) is True +def test_a_platform_playwright_refuses_counts_as_not_installed(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(sys, "platform", "sunos5") + monkeypatch.delenv("PLAYWRIGHT_BROWSERS_PATH", raising=False) - assert chromium_installed(str(tmp_path / "absent")) is False, "a missing directory is not an install" + assert chromium_installed() is False + point_at_login_cache(monkeypatch) + assert "PLAYWRIGHT_BROWSERS_PATH" not in os.environ From c20b173768a797d5b800037d1b24572b51cfdc57 Mon Sep 17 00:00:00 2001 From: Dizhan Xue Date: Fri, 2 Oct 2026 17:22:23 +0000 Subject: [PATCH 9/9] docs(*): correct four overstated claims in the browser change's text The shared cache helper and its test counted six failures for a missing browser, a number true of one suite and not of the two the helper serves. _mark was called the stamp map's only writer, though _forget_owner writes to it too; it is the only place that adds a stamp. And the tabs description was called the only text the model reads before a call, when the parameter schema and the system prompt are read too. Comments and docstrings only; both trees parse to the same AST with docstrings stripped. Co-authored-by: Claude (claude-opus-5-5[1m]) --- raven/agent/tools/browser.py | 4 ++-- tests/_browser_cache.py | 2 +- tests/test_browser_cache.py | 2 +- tests/test_browser_tools.py | 15 ++++++++------- 4 files changed, 12 insertions(+), 11 deletions(-) diff --git a/raven/agent/tools/browser.py b/raven/agent/tools/browser.py index 5a38ac49d..86bc02da4 100644 --- a/raven/agent/tools/browser.py +++ b/raven/agent/tools/browser.py @@ -222,8 +222,8 @@ def _forget_owner(cls, owner: str) -> None: def _bind_to_driver(self) -> None: """Have the driver tell the stamp map when it drops an owner's binding. - Called from ``_mark``, the map's only writer, so no owner can hold a - stamp before the driver knows to prune it. Not at import: reaching for + Called from ``_mark``, the only place that adds a stamp, so no owner + can hold one before the driver knows to prune it. Not at import: reaching for the driver is what builds the process-wide browser, and importing this module must not construct one as a side effect. The driver is the one place that knows when an owner's binding ends, so the map is pruned on diff --git a/tests/_browser_cache.py b/tests/_browser_cache.py index ffe101ef3..6245ab30b 100644 --- a/tests/_browser_cache.py +++ b/tests/_browser_cache.py @@ -77,7 +77,7 @@ def chromium_installed(where: str | None = None) -> bool: A directory entry rather than ``Browser.probe()``, which only answers whether the ``playwright`` package imports: skipping on the package alone - turned a missing binary into six failures. It does not check the revision + turned a missing binary into failures. It does not check the revision -- a cache holding only an older build still fails the launch, loudly, and the driver's error then names the path it looked for. """ diff --git a/tests/test_browser_cache.py b/tests/test_browser_cache.py index 99b9394d0..bc8082d94 100644 --- a/tests/test_browser_cache.py +++ b/tests/test_browser_cache.py @@ -104,7 +104,7 @@ def test_the_fixture_aims_playwright_at_the_login_cache_unless_already_aimed( def test_a_chromium_directory_is_what_counts_as_installed(tmp_path) -> None: """The old skip asked ``Browser.probe()``, which only tests the package, - so a missing binary failed six tests instead of skipping them.""" + so a missing binary failed the real-page tests instead of skipping them.""" cache = tmp_path / "ms-playwright" cache.mkdir() (cache / "ffmpeg-1011").mkdir() diff --git a/tests/test_browser_tools.py b/tests/test_browser_tools.py index 6e75c6f3e..f04737198 100644 --- a/tests/test_browser_tools.py +++ b/tests/test_browser_tools.py @@ -774,11 +774,12 @@ def test_site_keyed_permission_set_matches_the_acting_tool_hierarchy() -> None: def test_the_stamp_store_is_pruned_by_the_driver_once_it_holds_an_entry( monkeypatch: pytest.MonkeyPatch, ) -> None: - """The store registers with the driver from its only writer, so an owner - that holds a stamp is an owner the driver will prune -- and not at import, - where reaching for the driver would build the process-wide browser. This - drives the release a run's end would cause and asserts the stamp is gone; - an unregistered store would keep it for the life of the process.""" + """The store registers with the driver from the only place that adds a + stamp, so an owner that holds one is an owner the driver will prune -- and + not at import, where reaching for the driver would build the process-wide + browser. This drives the release a run's end would cause and asserts the + stamp is gone; an unregistered store would keep it for the life of the + process.""" b = get_browser() page = _FakePage("https://a.test/") _running(b, [page]) @@ -811,8 +812,8 @@ async def test_the_tabs_description_names_the_close_rule_the_driver_enforces() - """The driver refuses an owner's close of a tab nobody holds -- the reader's, which may carry a login the model just asked them to finish -- and the listing marks that tab with neither ``yours`` nor ``held``. The - description is the only text the model reads before calling, so without - the rule there the refusal is the first it hears of it.""" + description is what the model reads before it calls; without the rule + there, the refusal is the first it hears of it.""" b = get_browser() _running(b, [_FakePage("https://bank.test/")])