fix(tools): stop unusable browser tools from crowding out browser_nav…

[MummIndia]

Summary

Tool retrieval returns a top-K (8 by default), and @playwright/mcp alone exposes
30 tools. Measured on three typical browsing requests, the agent was handed
browser_drop, browser_handle_dialog, browser_close, browser_console_messages
and the whole xy-mouse family — while browser_navigate, the only tool that opens a
page, was missing in two cases out of three. On "click the login button", six of the
eight slots went to mouse primitives.

Without the tool that opens a page, every other browser tool is dead weight, so the
model answers that it cannot reach the internet. That answer is correct, but it reads
as a refusal or a hallucination, which is what makes it expensive to diagnose: the
browser is installed, the MCP server reports 30 tools, and calling browser_navigate
by hand works fine.

This adds two guards in src/tool_index.py. MCP_INDEX_DENIED keeps 18 unusable
tools out of the retrieval index — pointer primitives (browser_click and
browser_hover work off the accessibility snapshot and need no coordinates),
debugging aids, session plumbing, and browser_run_code_unsafe, which runs arbitrary
JavaScript in the page. They stay connected and callable; they just no longer compete
for a slot. MCP_COMPANIONS pulls browser_navigate and browser_snapshot in
whenever any browser tool is retrieved. Matching is on the bare tool name, after the
server prefix, so neither guard depends on the browser being registered as
builtin_browser — verified with a second prefix.

Target branch

• This PR targets dev, not main.

Linked Issue

Fixes #5763

Type of Change

• Bug fix (non-breaking — fixes a confirmed issue)

Checklist

• I searched open issues and open PRs — this is not a duplicate.
• This PR targets dev
• My changes are limited to the scope described above — no unrelated refactors or whitespace changes mixed in.
• I actually ran the app (docker compose up) and verified the change works end-to-end.

How to Test

1. Run the app with the built-in browser MCP server connected. Startup logs should
   show MCP server connected: Built-in: Browser (builtin_browser) - 30 tools via stdio.
2. In agent mode, send: Open https://example.com and summarise the page.
3. Before the change — check the [tool-rag] Retrieved tools for query: log line.
   mcp__builtin_browser__browser_navigate is absent, while entries such as
   browser_drop, browser_close or browser_console_messages occupy the slots.
   The agent then reports that it cannot access the internet.
4. After the change — the same log line contains browser_navigate and
   browser_snapshot, and startup reports Indexed 12 MCP tools instead of 30.
5. The agent completes the request, emitting
   Tool executed: mcp__builtin_browser__browser_navigate -> exit_code=0, then
   follows up with browser_snapshot to read the page.

Measured on three requests after the change: browser_navigate present 3/3, no
denied tool leaking through, and every slot filled with something usable —
navigate, snapshot, find, click, type, press_key, select_option.

Visual / UI changes

None. This PR only touches src/tool_index.py, which has no rendering path — no
HTML, CSS, SVG or static/js/ module is modified.

[o3LL]

Reviewed 830f7cd6d7cc1344f3c52346f6d6b810eb157cab against dev @ f9235ebb.

Short version: the bug is real, the mechanism is sound, and the tool surface behind it was clearly enumerated rather than guessed. But on the browser server Odysseus actually ships, neither guard changes what the model receives — I drove the running app to check, and the tool list is byte-identical before and after. The change is load-bearing for a browser MCP server registered by hand under a different server id, which is not the configuration the reproduction describes.

Findings

P1 — issue: both guards are cancelled out by _expand_browser_mcp_tools for the built-in browser server

• Problem: MCP_INDEX_DENIED filters at index time and MCP_COMPANIONS fires inside get_tools_for_query(). Immediately after that call returns, src/agent_loop.py:3894 runs _expand_browser_mcp_tools(_relevant_tools, mcp_mgr), which rebuilds the browser toolset from mcp_mgr.get_all_tools() — the MCP manager, not the retrieval index. Every denied tool goes straight back into _relevant_tools, before _tool_schemas_for_route filters schemas by it. _expand_browser_mcp_tools landed in d8a2059d on 2026-07-23, which is this branch's own merge base, so it is present in the tree the PR was written against.

• Impact: I ran the app on current dev with the built-in browser connected (Built-in: Browser (builtin_browser) - 30 tools via stdio), pointed it at a local OpenAI-compatible endpoint that records the tool schemas it is handed, and sent Open https://example.com and summarise the page in agent mode. Before and after applying this PR:

  	before	after
  Indexed N MCP tools at startup	30	12
  tool schemas sent to the model	34	34
  browser tools among them	30	30
  browser_navigate sent	yes	yes
  denied tools sent	18	18

  The index shrinks. The toolset does not. The Indexed 12 MCP tools line in step 4 of How to Test is real, but it describes the index, not what reaches the model, and the two are no longer the same thing once the expansion runs.

  The expansion is visible in the logs the issue tells reviewers to compare. On unmodified dev, retrieval returned six browser tools:

  [tool-rag] Retrieved tools for query: ['mcp__builtin_browser__browser_close',
    'mcp__builtin_browser__browser_find', 'mcp__builtin_browser__browser_navigate',
    'mcp__builtin_browser__browser_network_request',
    'mcp__builtin_browser__browser_network_requests',
    'mcp__builtin_browser__browser_tabs', 'web_fetch', 'web_search']
  
  [agent-debug] round=1 model=... _is_api_model=True tools_sent=34
    relevant_tools=[... 'mcp__builtin_browser__browser_click',
    'mcp__builtin_browser__browser_drag', 'mcp__builtin_browser__browser_mouse_click_xy',
    'mcp__builtin_browser__browser_mouse_down' ...]
  

  browser_click, browser_drag and the mouse primitives are in relevant_tools without ever appearing in the retrieval line. That gap is the expansion.

  Two consequences. The context-bloat win the ROADMAP item is about does not happen — 30 browser schemas still go out. And the security intent does not either: browser_run_code_unsafe, whose own description reads "Unsafe: executes arbitrary JavaScript in the Playwright server process and is RCE-equivalent", is still in the model's tool list, despite the comment saying it is deliberately out of reach.

• Ask: Decide where the guard belongs and put it in one place. Either filter the expansion's output through MCP_INDEX_DENIED as well, or drop the index-time deny from this PR and apply the same set at src/agent_loop.py:3894. Right now the two halves are in different files and undo each other.

• Location: src/tool_index.py:64-96 and src/tool_index.py:305, against src/agent_loop.py:56-77 and src/agent_loop.py:3894

P1 — issue (scope): the reported symptom does not reproduce on the configuration in the reproduction steps

• Problem: Because the expansion pulls in every connected builtin_browser tool whenever any one of them is retrieved, browser_navigate is already guaranteed on that path. The same end-to-end run above shows it present on unmodified dev, in the schemas actually sent to the model.

• Impact: #5763 explains the gap, and does so honestly — it records that the behaviour was observed "on a fork branched from dev on 2026-06-01", and that the check against current dev was done "by reading the source", naming src/tool_index.py. The expansion lives in src/agent_loop.py, so a source read scoped to tool_index.py would not surface it. The tools_sent=17 figure in the issue's log excerpt is consistent with a baseline where it was not yet running. Meanwhile the PR checklist ticks "I actually ran the app and verified the change works end-to-end", which is hard to square with that.

  The underlying bug is real for a browser MCP server added by hand under any other server id — src/agent_loop.py:67 matches builtin_browser specifically, so nothing rescues that case. Same measurement at the tool-selection layer, server_id=playwright, on click the login button: before, four browser tools selected and browser_navigate not among them, with three of the eight retrieval slots spent on browser_mouse_*; after, browser_navigate present. So the fix works. It just fixes a case the write-up does not describe.

• Ask: Re-measure against current dev and re-scope both the PR and #5763 to user-added browser MCP servers. That is a defensible change on its own and much easier to argue than the current framing.

• Location: src/tool_index.py:563-572, src/agent_loop.py:67

P2 — issue (api): where the deny list does take effect, denied tools are withheld from the model, and two of them strand the browser

• Problem: The body and the comment at src/tool_index.py:74 both say denied tools "stay connected and callable; they just no longer compete for a slot". On the path where the deny list has any effect — a browser server that is not builtin_browser, so the expansion never fires — that is not what happens. _tool_schemas_for_route filters MCP schemas by route_relevant_tools at src/agent_loop.py:4357-4360, so a tool that never enters the index is never offered and the model cannot call it. Measured: 0 of the 18 reach the model on that path. Denying is removal, not de-ranking.

• Impact: browser_handle_dialog is the one that bites. Playwright MCP refuses tools that do not handle modal state while a dialog is open, and names that tool as the way out. Driving the real server against a page that fires alert() on load, browser_snapshot returns:

  ### Error
  Error: Tool "browser_snapshot" does not handle the modal state.
  ### Modal state
  - ["alert" dialog with message "hi"]: can be handled by browser_handle_dialog
  

  With that tool withheld the model is told, on every subsequent call, to use something it was never offered, and the tab does not recover for the rest of the turn. A cookie banner or age gate implemented as a real dialog turns "the agent browses it" into "the agent is stuck" — the same flow this PR exists to repair. browser_file_upload clears the file-chooser modal by the same mechanism; I did not reproduce that one specifically.

• Ask: Move browser_handle_dialog and browser_file_upload into MCP_COMPANIONS so they return whenever a browser tool is in play, and correct the comment to say denied tools are withheld from the model rather than merely de-ranked.

• Location: src/tool_index.py:88, src/agent_loop.py:4357-4360

P2 — issue: the companion rule cannot fire on the query shape most likely to show the bug

• Problem: Companions are conditional on some browser tool already surfacing in the top-K. When none does, nothing is injected.

• Impact: go to news.ycombinator.com and tell me the top story retrieves no browser tool at all — before and after, on both server ids — so zero browser tools are selected either way. That is exactly the "the agent says it cannot browse" complaint the PR is written against, and the patch does not move it. Separately, note that the deny list alone cannot solve this: it takes the indexable browser surface from 30 to 12, still above the top-K of 8, so the companion rule is the load-bearing half and it has this hole in it.

• Ask: Either say so plainly in the body, or seed on browse intent structurally. _WEB_RE at src/tool_index.py:380 already detects this class of query for web_search/web_fetch and is the natural hook.

• Location: src/tool_index.py:563-572, src/tool_index.py:380

P3 — suggestion (test): neither new behaviour is covered

• Problem: Both guards are pure functions of a name list, in a tree with 793 test files and a documented testing standard, and nothing pins either.

• Impact: Both fail silently. If the companion rule regresses, no test goes red — the agent just goes back to reporting it cannot browse, which is the bug this PR exists to fix.

• Ask: tests/test_tool_rag_keyword_hints.py:23 already builds a ToolIndex with retrieval stubbed and needs no ChromaDB. Three asserts on that pattern cover it: a denied name never reaches the index; companions are injected using the retrieved hit's own prefix; nothing is injected when no browser tool is retrieved.

• Location: src/tool_index.py:64-96, tests/test_tool_rag_keyword_hints.py:23

P3 — suggestion: a static list against a floating upstream

• Problem: src/builtin_mcp.py:84 launches @playwright/mcp@latest, so the tool surface moves independently of this list. A tool added upstream arrives undenied; a renamed one silently drops out. The docstring on _expand_browser_mcp_tools makes exactly this point about release-to-release renames.

• Impact: Slow drift back toward the original problem with no signal when it happens. Nothing wrong today — I enumerated the live surface and all 18 names exist.

• Ask: Non-blocking. Consider inverting to an allow-list over the browser_ namespace, which fails closed as upstream grows; otherwise a comment noting the list tracks @latest would do.

• Location: src/tool_index.py:75-89, src/builtin_mcp.py:84

P3 — nit: the PR title is truncated, and merges here are squashes

• Problem: The title field ends ...crowding out browser_nav…. The commit subject itself is fine.

• Impact: A squash merge takes the PR title, so dev would carry the ellipsis in its history.

• Ask: Retype the title in full.

Open Questions

• question (scope, non-blocking): None of this reaches local models. src/agent_loop.py:4377-4378 returns route_mcp_schemas unfiltered by route_relevant_tools on the non-API path, so every connected MCP schema goes out regardless. The ROADMAP item being served here is specifically about prompts being too heavy for smaller local models. Deliberate scoping, or a follow-up?

• question (non-blocking): Was browser_take_screenshot kept on purpose? It survives the deny list and overlaps browser_snapshot, whose own description says the snapshot "is better than screenshot", so it is a plausible extra competitor for a top-8 slot.

• question (non-blocking): The body says six of the eight slots went to mouse primitives on "click the login button". I measured three of eight, on the FastEmbed lane against @playwright/mcp@0.0.79. Same direction, different number — a different @latest build, or were you on a custom embedding endpoint?

Validation

• Ran: macOS, Python 3.11, dev @ f9235ebb; the diff applies clean. python -m compileall src/tool_index.py clean. Full suite with the patch merged onto current dev: 2 failed, 5306 passed, 4 skipped in 127.86s; both failures (test_workspace_confine.py::test_glob_confined_e2e, test_integration_api_call_ssrf.py::test_real_socket_falls_back_from_dead_first_to_live_second) reproduce identically on unmodified dev and are environment-dependent, not caused by this change. Booted uvicorn app:app twice — once on unmodified dev, once with the patch — with the built-in browser MCP server connected and a local OpenAI-compatible endpoint recording the tool schemas it received, then sent a real agent-mode turn through /api/chat_stream and compared what the model was handed; that is where the table in the first finding comes from. Enumerated the live tool surface over a real MCP tools/list handshake: 24 tools by default and 30 with --caps vision, the six extra being the browser_mouse_* family that src/builtin_mcp.py:84 enables, and all 18 denied names exist. Reproduced the dialog modal-state refusal against the real server and a real browser. Ran retrieval against a real ChromaDB and real FastEmbed over the real tool descriptions, before and after, for builtin_browser and for a second server id, across four queries. Checked prefix handling for a bare builtin, an unprefixed browser name, mcp__builtin_browser__… and a custom prefix — rsplit("__", 1)[-1] at index time and rpartition("__") at retrieval agree on the bare name and the unprefixed case reconstructs correctly, so that is not a bug. Test-merged #5999 alongside this one: no conflict.

• Not run: I did not drive the non-builtin_browser case through the full app; that half is measured at the tool-selection layer with the real manager and the real tool list, not end to end. Only the FastEmbed lane — a configured custom embedding endpoint could rank differently. No browser_file_upload modal reproduction; I am inferring it from the dialog case. Nothing on Windows, which is where #5763 was observed, and nothing under Docker.

• Residual risk: Low on what the diff does mechanically. The retrieval slot counts are one embedding model on one machine and are indicative rather than exact; the before/after comparison is the part I would stand behind. The behavioural removal is wider than the headline suggests — on the path where the deny list does take effect it withdraws page console, network inspection, tab control, dialog handling and file upload, and only the last two look load-bearing, but a diagnosis flow that relied on console or network output would lose it silently.

PR Hygiene

• Target/template/checks: Targets dev. One focused change, a single file, 48 added lines, no drive-by edits. Mergeable, ready for review, and the latest run of every check is green — the red Check PR description in the rollup is a stale earlier run. Fixes #5763, which is open and describes this failure, so auto-close on merge is the right link. No screenshot needed; nothing renders.

• Branch state worth knowing: the branch is based on d8a2059d, three weeks back. On that base src/agent_loop.py cannot be imported — NameError: name 'Any' is not defined, from a dev bug fixed since — so the test suite cannot collect on the branch as it stands. GitHub still reports the merge into current dev as clean, which is what I tested, so no action is needed; but if you try to run anything locally and it fails at import, that is why.

• Related context: #5999 touches _KEYWORD_HINTS in the same file and merges cleanly alongside this. #5765 (Fixes #5764) is about the model treating browser_navigate as though it had read the page, which is the same browser_snapshot follow-up the companion rule here injects — the two are complementary and probably want looking at together. #5772 and #5817 touch tool selection in src/agent_loop.py without overlapping this diff.

[RaresKeY]

Thanks for the work on making browser MCP selection more reliable. I checked the latest head against current dev, focusing on retrieval, final schema selection, and supported browser actions, and found two correctness issues below.

issue (correctness): The denylist is bypassed before the final browser schema set is sent

• Problem: MCP_INDEX_DENIED only removes names while building the retrieval index. Once any browser tool is selected, the existing browser-expansion path can add the full connected builtin_browser set back into the selected set, and the MCP prompt-description path still includes the full connected tool list. On non-native/local model routes, a browser-keyword request also sends all MCP schemas instead of filtering them by the retrieved set.
• Impact: The change can still hand the model the low-value browser tools it is intended to keep out, so the reported context/tool-budget crowding can remain and the main acceptance behavior is not guaranteed.
• Ask: Apply the allow/deny or bounded browser-core selection at the final schema and prompt assembly boundaries as well as at indexing, preserving qualified names and disabled/policy filters, and add regressions that assert the final API and local-model tool sets.
• Location: src/tool_index.py:303-305; related final selection in src/agent_loop.py:75-96,2743-2751,4023-4024,4512-4514, src/mcp_manager.py:661-704, and routes/chat_routes.py:2286-2292.

issue (correctness): Keep browser drag reachable after denylisting low-level browser tools

• Problem: browser_drag is in MCP_INDEX_DENIED, and the new index filter removes it. The chat route still treats browser_drag as a supported browser tool, but its explicit browser-intent matching does not cover a drag-only request, while the companion logic adds browser_navigate and browser_snapshot only after another browser tool has already been retrieved. Explicit browser wording still forces the supported tool, so the gap is in drag-only retrieval paths without that explicit intent.
• Impact: A user asking only to drag an element can reach a turn with no browser schema for that supported action on the affected retrieval path.
• Ask: Remove browser_drag from the denylist if it is a useful high-level action, or explicitly cover drag-only intent and companion selection; add a regression that verifies the final schema set for a drag request.
• Location: src/tool_index.py:80,556-572; related browser routing in routes/chat_routes.py:285,1009-1013,2286-2292.

Validation

I reviewed the prepared current-dev source and the final retrieval, expansion, prompt-description, route, and schema-selection paths. The focused denylist/companion behavior is not covered by an automated regression test; dynamic validation was not run per review constraints.