Skip to content

fix(*): key browser grants on the page a call lands on and prune owner state - #839

Open
LivXue wants to merge 9 commits into
mainfrom
fix/browser_owner_state_site_key
Open

LivXue wants to merge 9 commits into
mainfrom
fix/browser_owner_state_site_key

Conversation

@LivXue

@LivXue LivXue commented Oct 2, 2026

Copy link
Copy Markdown
Member

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 an
owner with no live binding. When another owner held that tab, the call
opened a blank tab of its own instead: a person approved bank.test for
a key press that landed on about:blank, and banked a grant that any
later click, type or press on bank.test presents, since the three
verbs share one key per site.

url_for now reports the page _page_for will bind, through one rule
both use (_landing): the owner's live tab, else the front tab unless
another 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_params asynchronous,
and that hook is part of the contract every tool shares.

_BrowserTool._acted was never pruned. Keyed by run:<uid>, it
kept 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 driver
now 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_tabs did 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 yours nor held. The
description now says so, as does the browser guide in both languages.

test_browser_real_web.py aimed Playwright at the macOS cache on
every platform.
The suite redirects HOME, and Playwright resolves
its 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 in
tests/_browser_cache.py, shared by both real-browser suites, and
mirrors playwright-core's registryDirectory: XDG_CACHE_HOME on Linux,
LOCALAPPDATA on Windows, PLAYWRIGHT_BROWSERS_PATH=0 for an install
inside 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_for is a prediction made before the prompt. Anything that moves
    while 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.
  • The skip checks for a Chromium directory, not the revision this
    Playwright needs. A cache holding only an older build still fails the
    launch, and the message then names the path it looked for.
  • browser_tabs stamps an owner after closing that owner's own tab;
    that stamp leaves through the cap rather than through a release.

Type

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

Verification

Run with the project environment's interpreter (all extras installed):

  • python -m pytest over 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 and
    tests/integration/test_browser_tools_real_web.py: 172 passed. CI
    does not collect tests/integration/, so those two files ran here
    only.
    With PLAYWRIGHT_BROWSERS_PATH pointed at a directory that does not
    exist, all 12 skip with the new reason instead of failing.

  • The url_for table, one case per branch of the landing rule, fails
    against 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, 116
    skipped. 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 check and ruff format --check on the changed files,
    lint-imports (10 kept, 0 broken), and ty check raven evolver agents plugins-dist scripts (the make lint-types set): all pass.

  • scripts/check_source_language.py and scripts/check_large_files.py
    over origin/main...HEAD, and scripts/check_commit_messages.py and
    commitlint 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.

  • Security impact considered
  • Backward compatibility considered
  • Rollback path is clear for risky changes

Related Issues

Fixes #600

LivXue and others added 9 commits October 2, 2026 16:10
… 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>
@LivXue
LivXue requested review from 0xKT and gloryfromca October 2, 2026 17:57

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(agent): browser tools leak owner state, and can key a permission on the wrong site

2 participants