diff --git a/docs-site/docs/browser-collaboration.md b/docs-site/docs/browser-collaboration.md index 47bc53dd7..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 @@ -98,6 +100,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..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 进程可能有 @@ -82,6 +84,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 9abc6e3f7..86bc02da4 100644 --- a/raven/agent/tools/browser.py +++ b/raven/agent/tools/browser.py @@ -37,6 +37,13 @@ SNAPSHOT_TEXT_CHARS = 8_000 MAX_REFS_SHOWN = 120 +# 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 # 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. @@ -50,7 +57,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(): @@ -175,7 +185,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 +201,36 @@ def _owner(self) -> str: return current_owner() def _mark(self, owner: str) -> None: - _BrowserTool._acted[owner] = time.monotonic() + 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() + while len(acted) > _ACTED_MAX: + acted.pop(next(iter(acted))) 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: + """Have the driver tell the stamp map when it drops an owner's binding. + + 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 + that event rather than on a second clock of its own. + """ + _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 +587,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..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 @@ -217,6 +218,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. 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 --------------------------------------------------------- @@ -318,7 +325,20 @@ 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 + # "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{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) return self._s.page @@ -485,9 +505,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.""" @@ -497,13 +522,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.""" - 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 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 + 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. @@ -518,18 +572,16 @@ 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 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() @@ -559,7 +611,25 @@ 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, 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: + 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 +740,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() @@ -733,6 +802,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/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/_browser_cache.py b/tests/_browser_cache.py new file mode 100644 index 000000000..6245ab30b --- /dev/null +++ b/tests/_browser_cache.py @@ -0,0 +1,108 @@ +"""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 + +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()``, 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: + pass + return os.path.expanduser("~") + + +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 | 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 build sits in the browser cache. + + 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 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: + return False + + +def point_at_login_cache(monkeypatch) -> None: + """Fixture body: aim Playwright at the login's cache, unless already aimed.""" + if os.environ.get("PLAYWRIGHT_BROWSERS_PATH"): + return + cache = playwright_cache(login_home()) + if cache is not None: + monkeypatch.setenv("PLAYWRIGHT_BROWSERS_PATH", cache) + + +__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"""