fix(*): key browser grants on the page a call lands on and prune owner state - #839
Merged
Merged
Conversation
… 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:<uid>`, 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]) <noreply@anthropic.com>
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]) <noreply@anthropic.com>
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]) <noreply@anthropic.com>
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]) <noreply@anthropic.com>
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]) <noreply@anthropic.com>
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]) <noreply@anthropic.com>
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]) <noreply@anthropic.com>
…lper 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]) <noreply@anthropic.com>
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]) <noreply@anthropic.com>
gloryfromca
reviewed
Oct 2, 2026
gloryfromca
left a comment
Member
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the refreshed github/main...HEAD diff and found no plain error. I covered the repository rules in AGENTS.md and the Browser/Permission Gate vocabulary and layer constraints in CONTEXT-MAP.md/CONTEXT.md; the full diff; the permission-key, browser-owner, tab, relaunch, and approval callers; the relevant history; backward compatibility; and whether tests were removed or weakened. The shared landing rule keeps url_for aligned with _page_for, owner removal reaches the external stamp state on each binding-ending path, and the test changes add coverage rather than relaxing existing assertions.
Verification:
uv run pytest tests/test_browser_cache.py tests/test_browser_driver.py tests/test_browser_tools.py tests/test_browser_policy.py- 160 passed.- The same run plus both real-web suites - 160 passed, 12 failed because the cached Chromium on this host cannot load
libatk-1.0.so.0; the failures occur at browser launch, before the reviewed behavior. uv run ruff check ...- passed.uv run ruff format --check ...- 9 files already formatted.uv run python scripts/check_source_language.py github/main...HEAD- passed.git diff --check github/main...HEAD- passed.
0xKT
approved these changes
Oct 3, 2026
7 of 12 tasks
0xKT
pushed a commit
that referenced
this pull request
Oct 3, 2026
… the test profile (#848) ## Summary Follow-ups to #839 in the browser driver, the browser tools and the real-browser tests. **The touch note survives the end of an owner's binding (regression from #839).** #839 had the driver tell the tools' `_acted` store whenever it dropped an owner's tab binding, and the store then deleted that owner's last-act stamp. The readback promises `note: the user interacted with the browser since your last action` until the owner acts again, and the end of a binding is not the end of the owner: `Browser.release()` has no production caller, so the listener fired only on a reap after `OWNER_IDLE_S`, on the owner's tab closing and on the browser closing. When the reader closed an agent's tab from the panel, or the binding idled out, the agent's next read lost the note. The listener is removed and the driver's owner removal is back to its pre-#839 form; `_ACTED_MAX` still bounds the store. **A host missing a system library is told so.** The driver now answers `Chromium is installed but cannot start here: the system library libatk-1.0.so.0 is missing. Installing system libraries needs root, so it is the user's step: on Debian or Ubuntu, <python> -m playwright install-deps chromium`. Before, on x64 the loader's refusal came back as Playwright's whole launch log, after a throwaway relaunch that could not help; where Playwright's own dependency check runs (its linux-arm64 Chromium builds), its report suggests `playwright install-deps`, which the driver read as "Chromium is not installed". The message says whose step it is because tool results reach the model too. **Only failures a fresh profile cannot fix skip the throwaway relaunch.** The relaunch was gated on the substring "profile", and every Playwright launch failure quotes the command line, `--user-data-dir=<home>/.raven/browser-profile` included, so every failure relaunched and logged "profile is busy". Every failure is still retried once except a missing system library, Playwright's host check and a missing executable, and the warning says the launch failed rather than that the profile is busy. Matching Chromium's lock words instead would be narrower and wrong under another locale: with `LANG=zh_CN.UTF-8` a second headful launch on a held profile prints its line in Chinese, Playwright's English rewrite of it does not fire, and the error says only "Target page, context or browser has been closed". **The profile path is read at launch.** `PROFILE_DIR` was computed at import, before `tests/conftest.py` gives each test its own HOME, so the real-browser tests used the developer's own `~/.raven/browser-profile`, with parallel workers opening it at once; the cookie `test_browser_real_web.py` sets turned up in a developer's own cookie jar. `_profile_dir()` now resolves the path on each launch. **The real-browser suites skip on a host that cannot load Chromium.** `tests/_browser_cache.py` gains `chromium_launch_failure()`: one bare headless launch per process, with Playwright alone. When it fails for a missing system library, both `tests/integration/test_browser_*real_web.py` modules skip with the library named: the twelve `libatk-1.0.so.0` launch failures reported on #839 become twelve skips. Any other launch failure, such as an older build left in the cache or a timeout under load, is not a reason to skip, and the suites fail on it as before. `docs/browser-and-desktop.md` said a second process would get a throwaway profile. The headless shell takes no profile lock, so a second headless Chromium opens the same profile; the sentence now says so. Not in this PR: - `url_for` still predicts the landing page before the permission prompt, so whatever moves while a person decides moves the call (the binding-generation idea in the #600 discussion). - Two headless Ravens share one profile with no lock; documented above, not changed. - `tests/integration/test_browser_tools_real_web.py::test_two_owners_two_tabs_and_neither_takes_the_other` hung in 2 of 73 runs of both real-browser files on main's tree -- both times with one profile shared by every worker, never in the 24 runs with a profile per test -- and in none of 78 runs on this branch or 240 runs of that test alone. The cause is unknown, and counts this small cannot credit any change here with a fix. One of the first 45 branch runs, made before the profile change, also failed one test outright; it was not seen again and is not explained. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification Python 3.12, the repo environment with all extras, run from this tree. - `python -m pytest tests/test_browser_driver.py tests/test_browser_tools.py tests/test_browser_cache.py -q`: 129 passed. With `raven/browser/driver.py` and `raven/agent/tools/browser.py` taken from origin/main, 15 of them fail: the touch-note test for a reader's close and for an idle binding, the launch-time profile test, the four host-library cases, the empty-message retry, and the seven probe tests, which call a helper main lacks. - Each guarded construct was mutated on its own (14 mutations, among them: no stamp after a close, a stamp pruned on a tab close, each of the three no-retry clauses dropped, the host-library check moved after the not-installed one, the profile path fixed at import, the probe skipping on any failure, the probe launching on the caller's loop); each turns at least one test red. - Full suite, `python -m pytest -q`: 27482 passed, 116 skipped, 2 failed. Both failures are local to this box and touch nothing this branch changes: `tests/test_install_script.py::test_resolve_node_dir_answers_each_case_it_exists_for` (an npm elsewhere on PATH leaks past the fixture) and `tests/test_subagent_node_runtime.py::test_what_cannot_be_read_names_nothing` (the suite runs as root, which reads the file the test makes unreadable). - `python -m pytest tests/integration/test_browser_real_web.py tests/integration/test_browser_tools_real_web.py -q` with a real Chromium: 12 passed in each of 3 runs. A guard around `_profile_dir()` logged 39 resolutions, none under the real home, and no file under `~/.raven/browser-profile` changed during the runs. - Report shapes: the host-library fixtures follow real launches against a binary built to need an absent library, with Playwright's dependency check both skipped (the loader's refusal) and forced (both of its Linux reports). Two headless launches on one profile: the second starts. Two headful launches on one profile under `xvfb-run`: with `LANG=en_US.UTF-8` the error says the profile "is already in use"; with `LANG=zh_CN.UTF-8` it carries none of Chromium's English lock words. - `ruff check`, `ruff format --check`, `lint-imports` (10 kept), `ty check raven`, `scripts/check_source_language.py`, `scripts/check_large_files.py`: all pass. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [x] User-facing docs or screenshots are updated when needed ## Risk User-visible: the touch note is reported again after the reader closes an agent's tab or the binding idles out; a launch that fails for a missing system library reads as one line naming it, with no throwaway relaunch; every other launch failure still gets one throwaway relaunch, under a different warning. - [x] Security impact considered: the new message names a root command, and it is worded as the user's step because tool results reach the model. - [x] Backward compatibility considered: `Browser.on_owner_released` (added in #839) and the module constant `PROFILE_DIR` are removed; `git grep` finds no reader of either outside the two files changed here. - [x] Rollback path is clear for risky changes: revert the squash commit; nothing persisted changes shape. ## Related Issues Follows up #839; refs #600. --------- Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the three defects #600 reports in the browser tools, and the test
fixture that failed six real-browser tests on Linux.
A permission key could name a site the call would not act on. The
acting tools write the site into the call before the gate reads it, and
took it from
url_for(owner), which fell back to the front tab for anowner with no live binding. When another owner held that tab, the call
opened a blank tab of its own instead: a person approved
bank.testfora key press that landed on
about:blank, and banked a grant that anylater click, type or press on
bank.testpresents, since the threeverbs share one key per site.
url_fornow reports the page_page_forwill bind, through one ruleboth use (
_landing): the owner's live tab, else the front tab unlessanother live owner holds it, else nothing. In the last case the call
opens a tab of its own, which no site describes yet, so no site is
written and the grant is keyed on the call itself.
The issue offered two directions, and neither is taken as written. An
empty answer for every unbound owner fixes the reported case but breaks
one that worked: with the front tab the reader's own, the call lands on
it while the prompt names no site, and its per-call grant can then be
presented by an identical call on whatever front tab comes next.
Resolving the site after binding would make
cast_paramsasynchronous,and that hook is part of the contract every tool shares.
_BrowserTool._actedwas never pruned. Keyed byrun:<uid>, itkept one entry per delegated run for the life of the process: 50,000
runs left 50,000 entries and 6.6 MiB after
gc.collect(). The drivernow ends every binding through one
_drop_owners(a reap, a release,the owner's tab closing, the browser closing) and tells a listener the
tools register on their first stamp. The map also keeps only the 512
owners that acted most recently, for owners whose binding is never
dropped. The same 50,000 runs now leave 512 entries.
browser_tabsdid not say that close refuses a tab nobody holds.Such a tab is the user's and may hold a login they were asked to
finish, and the listing marks it neither
yoursnorheld. Thedescription now says so, as does the browser guide in both languages.
test_browser_real_web.pyaimed Playwright at the macOS cache onevery platform. The suite redirects
HOME, and Playwright resolvesits cache from the home directory, so the fixture points it back at the
login's cache, but only at
~/Library/Caches. The look-up now lives intests/_browser_cache.py, shared by both real-browser suites, andmirrors playwright-core's
registryDirectory: XDG_CACHE_HOME on Linux,LOCALAPPDATA on Windows,
PLAYWRIGHT_BROWSERS_PATH=0for an installinside the package, and a relative path. Both suites skipped only when
the package was missing, so a missing browser failed them; they now
check for the browser. The driver called that failure "not installed"
even when the browser sat where the process did not look; the message
now quotes the path it looked for and names PLAYWRIGHT_BROWSERS_PATH,
on one line.
Known gaps, left as they are:
url_foris a prediction made before the prompt. Anything that moveswhile a person decides (the reader switches tabs, another owner takes
the front tab, the binding idles past ten minutes) moves the call with
it. Closing that needs the gate to see the binding the call will use;
the issue thread sketches one shape.
Playwright needs. A cache holding only an older build still fails the
launch, and the message then names the path it looked for.
browser_tabsstamps an owner after closing that owner's own tab;that stamp leaves through the cap rather than through a release.
Type
Verification
Run with the project environment's interpreter (all extras installed):
python -m pytestovertests/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.pyandtests/integration/test_browser_tools_real_web.py: 172 passed. CIdoes not collect
tests/integration/, so those two files ran hereonly.
With PLAYWRIGHT_BROWSERS_PATH pointed at a directory that does not
exist, all 12 skip with the new reason instead of failing.
The
url_fortable, one case per branch of the landing rule, failsagainst main in the two cases where another owner holds the front
tab, and against the empty-answer direction in the three where the
front tab is free.
Deleting each of the 22 constructs that carry this change, one at a
time, turns at least one test red.
python -m pytest -q -p no:randomly: 27132 passed, 7 failed, 116skipped. The same 7 IDs fail on a complete tree of origin/main: five
provider-proxy tests that read this machine's proxy settings, and two
Node-runtime tests that depend on its node install and on root
ignoring permission bits.
ruff checkandruff format --checkon the changed files,lint-imports(10 kept, 0 broken), andty check raven evolver agents plugins-dist scripts(themake lint-typesset): all pass.scripts/check_source_language.pyandscripts/check_large_files.pyover origin/main...HEAD, and
scripts/check_commit_messages.pyandcommitlint over origin/main..HEAD: all pass.
Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Risk
An acting call that would open a tab of its own is no longer keyed on
the front tab's site: its prompt names no site, and a session grant for
it covers only an identical call, so that state prompts more often. A
call landing on the user's front tab keeps the site key it had. The
browser-unavailable message the panel shows is reworded; nothing parses
it. No config, schema or stored state changes, and reverting the squash
commit restores the previous behaviour.
Related Issues
Fixes #600