fix(mcp): give stdio servers the environment, and let builtins reconnect

[MummIndia]

Summary

Two defects that combine to make the built-in browser MCP server unusable, each of
which hides the other.

_connect_stdio built env={**os.environ, **env} if env else None. None does not
mean "inherit the parent environment" — the MCP SDK substitutes a minimal default one.
Callers that pass an env inherit everything, which is why the Python builtins work:
they pass builtin_python_env(base_dir). The NPX browser server passes nothing, so it
loses the entire container environment including PLAYWRIGHT_BROWSERS_PATH. It then
looks for browsers in the default cache and reports Browser "firefox" is not installed — with the browser sitting one directory away, which is what makes this
hard to place.

Second, a stdio session can disappear without the process dying: the teardown races
across asyncio tasks. call_tool returned early on a missing session, and the existing
recovery only ran when a call raised — which presupposes a session. A missing one was
therefore terminal, even though reconnecting would have fixed it. Reconnection is now
attempted in that case too.

_reconnect_builtin also excluded the browser outright: it tested membership against
_BUILTIN_SERVERS, the Python-server dict, while is_builtin() counts the NPX servers
as builtins as well. It now handles both kinds.

Target branch

• This PR targets dev, not main.

Linked Issue

Fixes #5769

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 in a deployment where PLAYWRIGHT_BROWSERS_PATH points somewhere other than the
   default cache, with the built-in browser server enabled.
2. Before — the server starts but reports Browser "firefox" is not installed.
   Confirm the env is the cause by printing os.environ inside the spawned server.
3. After — the server connects and reports its 30 tools, and
   browser_navigate against a real URL returns exit_code=0.
4. For the reconnect path: drop the session (kill the npx child), then call a browser
   tool. Before, every subsequent call returns
   MCP server not connected: builtin_browser for the process lifetime. After, the
   log shows No session for builtin builtin_browser; attempting reconnect followed by
   Reconnected builtin MCP server: Built-in: Browser, and the call succeeds.

Visual / UI changes

None. src/mcp_manager.py only — transport and lifecycle, no rendering path.

[o3LL]

Reviewed 32bfcb16.

Findings

P1 issue (security): every stdio MCP server now receives the full process environment, overriding the SDK's allow-list

• Problem: env={**os.environ, **(env or {})} removes the last case in which a stdio server got a filtered environment. The SDK's None is not an oversight to route around: in the pinned mcp 1.29, mcp/client/stdio.py::get_default_environment() is documented as returning "only environment variables deemed safe to inherit" — DEFAULT_INHERITED_ENV_VARS = ['HOME', 'LOGNAME', 'PATH', 'SHELL', 'TERM', 'USER'] — and it drops values beginning with () with the comment "Skip functions, which are a security risk". Worth knowing that the SDK already merges that safe set under whatever env you pass ({**get_default_environment(), **server.env}), so an empty env was never a starved environment; it was the filtered one. Measured through the running app, adding a probe server via POST /api/mcp/servers with an empty env field:

  before: env_var_count=7  canary=<absent>              ssh_agent=no
  after : env_var_count=44 canary=sk-canary-do-not-ship ssh_agent=yes
  

• Impact: Odysseus reads 13 secret-bearing variables from the environment — OPENAI_API_KEY (src/constants.py:96), GOOGLE_API_KEY, GOOGLE_OAUTH_CLIENT_SECRET, HF_TOKEN, HUGGING_FACE_HUB_TOKEN, IMAP_PASSWORD, SMTP_PASSWORD, CARDDAV_PASSWORD, TAVILY_API_KEY, SERPER_API_KEY, DATA_BRAVE_API_KEY, EMBEDDING_API_KEY and ODYSSEUS_INTERNAL_TOKEN. After this change all of them go to npx -y @playwright/mcp@latest — a floating third-party package re-resolved on every start — and to any stdio server a user adds from Settings. ODYSSEUS_INTERNAL_TOKEN is the sharpest edge: core/middleware.py:31 treats it as an admin bypass on every admin route, so it isn't a leaked credential, it's authority over the app's own API — including registering another stdio server, which that route's docstring correctly calls equivalent to executing arbitrary binaries on the host. No shipped compose file sets it, so that part is limited to deployments that set it deliberately. In fairness this widens an existing hole rather than opening one: servers configured with a custom env already took the {**os.environ, **env} branch. But it removes the only configuration where the SDK's protection still applied.

• Ask: keep the widening where it already exists and stop there — full inheritance for the builtins, the SDK default for user-added servers:

  from mcp.client.stdio import get_default_environment
  base = os.environ if self.is_builtin(server_id) else get_default_environment()
  server_params = StdioServerParameters(command=command, args=args, env={**base, **env})

  If servers genuinely need more, make it an opt-in per-server "inherit host environment" toggle in the MCP settings UI so the blast radius is a deliberate choice. If full inheritance for everything is the intended contract, say so in the PR — it then needs a line in that UI, because "add MCP server" would mean "hand this binary every credential in the process".

• Location: src/mcp_manager.py:201

P1 issue: the new NPX reconnect drops the browser's args and env — it reintroduces the failure this PR cites

• Problem: startup and reconnect build the launch two different ways. register_builtin_servers passes _browser_mcp_args(cfg["args"]) — which appends --executable-path <resolved browser>, --isolated and --no-sandbox — plus env={"XDG_CACHE_HOME": ..., "PLAYWRIGHT_BROWSERS_PATH": ...}. The reconnect branch passes raw cfg["args"] and no env= at all. Captured on this head:

  STARTUP   args=['-y','@playwright/mcp@latest','--headless','--caps','vision',
                  '--executable-path','/opt/homebrew/bin/chromium','--isolated','--no-sandbox']
  STARTUP   env ={'XDG_CACHE_HOME': '<data>/local/playwright-mcp-cache',
                  'PLAYWRIGHT_BROWSERS_PATH': '<data>/local/playwright-mcp-cache/browsers'}
  RECONNECT args=['-y','@playwright/mcp@latest','--headless','--caps','vision']
  RECONNECT env =None
  

  End to end: start the browser as startup does, kill the npx child, call browser_navigate twice. The first call reaches the configured binary (<launching> /opt/homebrew/bin/chromium ...); after the reconnect the second fails with Chromium distribution 'chrome' is not found at /Applications/Google Chrome.app/.... Different browser, because --executable-path is gone.

• Impact: a reconnected browser server looks for browsers in the default cache instead of Odysseus's — precisely the Browser "..." is not installed class of failure this PR quotes as the bug being fixed, on the path added to recover from a crash. Losing --no-sandbox is worse in Docker, where Chromium refuses to start as root, so the reconnect silently downgrades to a browser that cannot launch at all. Losing --isolated changes profile persistence, and losing XDG_CACHE_HOME means npx -y ...@latest re-resolves into a different cache and can bring up a different package version than the one that was running. The reconnect also skips the ODYSSEUS_BROWSER_MCP_REQUIRE_CACHE guard, so an install that opted out of network installs at startup gets one anyway.

• Ask: factor the per-server launch configuration out of register_builtin_servers into one function — builtin_npx_launch(server_id) -> (command, args, env) — and call it from both places so they can't drift again. tests/test_mcp_reconnect_args.py already encodes this rule for the admin-tool reconnect: a reconnect passes the full server config.

• Location: src/mcp_manager.py:563

P2 issue (docs): the stated diagnosis doesn't hold for the browser server on this branch's own base

• Problem: the Summary puts the NPX browser server among "callers that pass none" and concludes it "loses PLAYWRIGHT_BROWSERS_PATH". It doesn't. src/builtin_mcp.py:226 — present in d8a2059d, this branch's parent — builds a truthy env for builtin_browser containing that exact variable, so the old ternary always took the {**os.environ, **env} branch. For the builtin browser at startup this change is a no-op. The linked issue says it was observed on a fork branched from dev on 2026-06-01; the browser env landed 2026-07-23. "Verified against current dev by reading the source — unchanged" is true of _connect_stdio but not of its caller, and the caller decides which branch runs. I booted unmodified dev: Built-in: Browser (builtin_browser) - 30 tools via stdio, launched with --executable-path. Also worth noting the shipped args carry no --browser flag (src/builtin_mcp.py:84) and Playwright MCP defaults to the chrome channel — I hit that default directly — so a Browser "firefox" is not installed message implies a non-default configuration and probably a different root cause again.

• Impact: the "Before" step in How to Test won't reproduce, so a reviewer can't confirm the fix by the route the PR gives them, and whatever really produced the original error stays unfixed — my best candidate is the reconnect gap above. The callers actually affected by env=None are user-added stdio servers with an empty env field, and, after this PR, the reconnect path itself. That population is the one carrying the security question, so the framing matters.

• Ask: re-run the repro against this branch's base and rewrite the Summary, the code comment and How to Test around whoever is actually affected, or point at the deployment where builtin_browser reaches env=None.

• Location: src/builtin_mcp.py:226

P2 issue: reconnect-on-every-call turns a fast error into an unbounded retry inside the agent turn

• Problem: a missing session used to return immediately. It now runs a full teardown plus npx -y @playwright/mcp@latest on every tool call, with no timeout, no backoff and no cap. _connect_stdio has no timeout of its own — the 20s guard exists only in _connect_with_timeout, on the startup path. Five calls against a down builtin produce five connect attempts and five times the latency.

• Impact: an agent turn against a server that is down and stays down stalls once per tool call instead of erroring instantly, and a hung npx — no network, slow registry — hangs the turn with nothing to cancel it.

• Ask: bound it. asyncio.wait_for(self._reconnect_builtin(server_id), timeout=20) plus a per-server cooldown so a dead server isn't respawned on every call.

• Location: src/mcp_manager.py:488

P2 issue: reconnects are unsynchronised

• Problem: call_tool now triggers _reconnect_builtin whenever a session is missing, and there is not a single lock in src/mcp_manager.py. N concurrent tool calls against a dropped server each run disconnect_server + connect_server for the same server_id.

• Impact: a tool-call burst after a crash spawns overlapping npx processes and interleaves teardown with setup — the same class of race the PR blames for losing the session in the first place.

• Ask: guard reconnects with a per-server asyncio.Lock, and have waiters re-check self._sessions after acquiring it.

• Location: src/mcp_manager.py:493

P2 issue: a failed teardown orphans the old subprocess before the new one starts

• Problem: disconnect_server catches the stack.aclose() failure, logs a warning and drops the reference anyway. The PR's own premise is that teardown races across asyncio tasks — that race is exactly what makes aclose() raise (Attempted to exit cancel scope in a different task), so the reconnect path is the one most likely to hit it.

• Impact: each reconnect can leave an orphaned npx + Chromium behind. Combined with the retry above, a down browser server leaks a process per tool call.

• Ask: treat a failed close as a failed teardown — log at error, and terminate the child rather than dropping the handle.

• Location: src/mcp_manager.py:407

P3 issue (test): no test, and both halves are cheap to test

• Problem: neither change has coverage. The env construction is testable by patching StdioServerParameters and asserting on the dict; the reconnect branch is testable with a fake manager, asserting that a reconnect produces the same command, args and env as the initial connect.

• Ask: one test per half. The reconnect one would have caught the first P1 above, which is the argument for writing it.

• Location: src/mcp_manager.py:556

P3 nit: (env or {}) is dead

• connect_server already normalizes with env or {} before calling, and the parameter is typed Dict[str, str]. {**os.environ, **env} is enough.

• Location: src/mcp_manager.py:201

Open Questions

• question (security): is full-environment inheritance the intended contract for user-added MCP servers, or only for the shipped builtins? That single answer decides whether the fix is one line or three, and it is the only thing keeping me from approving the transport half.

• question (nit, non-blocking): _BUILTIN_NPX_SERVERS["builtin_browser"]["command"] = "npx" is now unused — both call sites resolve _find_npx() instead. Drop the key or use it?

Validation

• Ran: 32bfcb16 merged into dev at f9235ebb (clean merge). Full suite: 5306 passed, 2 failed, 4 skipped in 142.5s — both failures the known macOS environment ones (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), reproducing on unmodified dev. python -m compileall clean on the touched files. Booted the app on :7099: all five builtins register, browser included, 30 tools. Added a probe stdio MCP server through POST /api/mcp/servers with an empty env field and read back what it received, on this branch and on unmodified dev. Drove the builtin browser through kill-and-reconnect and captured what the reconnect spawns. Read get_default_environment and DEFAULT_INHERITED_ENV_VARS out of the pinned mcp 1.29.0 rather than trusting the docs. Enumerated the secret-bearing variables the app reads from the environment.

• Not run: Docker, and Windows — the linked issue reports both, and the container is where both the --no-sandbox consequence and the credential exposure land hardest. No live agent turn against a real model, so the reconnect is verified at the manager layer, not through the agent loop.

• Residual risk: the P1 on reconnect args is verified on macOS against a Homebrew Chromium; the exact error text in a container will differ, but the launch-config divergence is structural and platform-independent. The env change is the kind that looks harmless in testing and only shows up in someone else's threat model — nothing in the suite would fail if it shipped.

PR Hygiene

• Targets dev, title matches the Conventional Commits check, template complete, one file, +41/−2. Fixes #5769 is open and is exactly this change, so auto-close is correct. Nothing renders, so no screenshot needed. Two independent changes in one PR — defensible since they share a file and a story, but the env change is security-relevant and would be easier to accept or reject on its own.

• The checks are not green in any useful sense. Seven workflows on this head — CI, Secret scan, CodeQL, Dependency review, Container scan (Trivy), Container scan, Workflow security — sit at action_required, waiting on approval. The only run that executed is ci / PR checks via pull_request_target: title, description and the mergeable flag. compileall, node --check, gitleaks and pytest have never run against 32bfcb16.

• No other open PR touches src/mcp_manager.py or src/builtin_mcp.py — checked all 117 open. No duplicate, nothing superseding. Same-author batch as #5762/#5765/#5767/#5772/#5774/#5776, all opened the same day.

• Base d8a2059d is three weeks behind; merges clean into current dev, which is what I tested.

• The PR body and the issue body both start with a UTF-8 BOM and every em dash renders as —. Worth re-pasting the body as UTF-8.