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.
This branch has not been deployed
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