From 32bfcb162fae67dfadffb97a66fc19018fecc9cd Mon Sep 17 00:00:00 2001
From: Matthieu <mummindia@gmail.com>
Date: Sun, 26 Jul 2026 21:22:07 +0200
Subject: [PATCH] fix(mcp): give stdio servers the environment, and let
 builtins reconnect
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 so 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 — so the one
server most likely to need a restart was the one that could never get
one. It now handles both kinds.

Together these mean a browser server that dropped mid-session stayed
down for the rest of the process lifetime, and a fresh one started
without the environment it needs. Verified on Docker/Windows: the
server reconnects on demand and reports its 30 tools, and
`browser_navigate` returns exit_code=0 against a real URL.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 src/mcp_manager.py | 43 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/src/mcp_manager.py b/src/mcp_manager.py
index 6f44e999a..5fb16969d 100644
--- a/src/mcp_manager.py
+++ b/src/mcp_manager.py
@@ -190,7 +190,15 @@ class McpManager:
             server_params = StdioServerParameters(
                 command=command,
                 args=args,
-                env={**os.environ, **env} if env else None,
+                # `None` is not "inherit the parent environment" — the MCP SDK
+                # substitutes a minimal default one. Callers that pass an env
+                # (the Python builtins, via builtin_python_env) inherit
+                # everything; callers that pass none lose the whole container
+                # environment. The NPX browser server is in the second group,
+                # so it loses PLAYWRIGHT_BROWSERS_PATH and then reports
+                # `Browser "firefox" is not installed` with the browser sitting
+                # one directory away.
+                env={**os.environ, **(env or {})},
             )
 
             stack = AsyncExitStack()
@@ -477,6 +485,14 @@ class McpManager:
         tool_name = parts[2]
 
         session = self._sessions.get(server_id)
+        if not session and self.is_builtin(server_id):
+            # A stdio session can disappear without the process dying — the
+            # teardown races across asyncio tasks. The recovery below only runs
+            # when a call raises, which presupposes a session, so a missing one
+            # was terminal even though reconnecting would have fixed it.
+            logger.warning(f"No session for builtin {server_id}; attempting reconnect")
+            if await self._reconnect_builtin(server_id):
+                session = self._sessions.get(server_id)
         if not session:
             return {"error": f"MCP server not connected: {server_id}", "exit_code": 1}
 
@@ -537,7 +553,30 @@ class McpManager:
     async def _reconnect_builtin(self, server_id: str) -> bool:
         """Tear down and reconnect a crashed builtin MCP server."""
         import sys
-        from src.builtin_mcp import _BUILTIN_SERVERS, builtin_python_env
+        from src.builtin_mcp import (
+            _BUILTIN_SERVERS, _BUILTIN_NPX_SERVERS, _find_npx, builtin_python_env,
+        )
+
+        # NPX-backed builtins (the browser) are builtins too — is_builtin()
+        # says so — but this membership test only knew about the Python ones,
+        # so the browser could never be reconnected.
+        if server_id in _BUILTIN_NPX_SERVERS:
+            cfg = _BUILTIN_NPX_SERVERS[server_id]
+            await self.disconnect_server(server_id)
+            try:
+                ok = await self.connect_server(
+                    server_id=server_id,
+                    name=cfg["name"],
+                    transport="stdio",
+                    command=_find_npx(),
+                    args=cfg["args"],
+                )
+                if ok:
+                    logger.info(f"Reconnected builtin MCP server: {cfg['name']}")
+                return ok
+            except Exception as e:
+                logger.error(f"Failed to reconnect builtin MCP server {cfg['name']}: {e}")
+                return False
 
         if server_id not in _BUILTIN_SERVERS:
             return False