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

src/mcp_manager.py

@@ -184,13 +184,27 @@ async def _connect_stdio(self, server_id: str, name: str, command: str, args: Li
         """Connect to an MCP server via stdio transport."""
         try:
             from mcp import ClientSession, StdioServerParameters
-            from mcp.client.stdio import stdio_client
+            from mcp.client.stdio import stdio_client, get_default_environment
             from contextlib import AsyncExitStack
 
+            # Passing env=None does not mean "inherit the parent environment" —
+            # the MCP SDK substitutes a minimal allowlist (HOME, PATH, …). The
+            # built-in NPX browser server passed nothing and so lost
+            # PLAYWRIGHT_BROWSERS_PATH, then reported `Browser "firefox" is not
+            # installed` with the browser sitting one directory away.
+            #
+            # Only the built-ins get the full environment. User-added servers
+            # are third-party processes: this one file's environment carries
+            # ODYSSEUS_INTERNAL_TOKEN (an admin bypass) among other secrets, so
+            # handing them os.environ would leak credentials to an arbitrary
+            # command. They keep the SDK's filtered default, plus whatever the
+            # server's own configuration explicitly sets.
+            base = os.environ if self.is_builtin(server_id) else get_default_environment()
+
             server_params = StdioServerParameters(
                 command=command,
                 args=args,
-                env={**os.environ, **env} if env else None,
+                env={**base, **env},
             )
 
             stack = AsyncExitStack()
@@ -477,6 +491,14 @@ async def call_tool(self, qualified_name: str, arguments: Dict) -> Dict:
         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 +559,30 @@ async def _do_call(self, session, tool_name: str, arguments: Dict) -> Dict:
     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

tests/test_mcp_stdio_env_scope.py

@@ -0,0 +1,90 @@
+"""Only built-in MCP servers may inherit the full process environment.
+
+`_connect_stdio` has to widen the environment for the built-in NPX browser
+server, which otherwise loses PLAYWRIGHT_BROWSERS_PATH and reports
+`Browser "firefox" is not installed`. Widening it for *every* stdio server
+would bypass the MCP SDK's allowlist and hand the whole environment — which
+carries ODYSSEUS_INTERNAL_TOKEN, an admin bypass, among other secrets — to
+user-added servers, i.e. to an arbitrary third-party command.
+
+These pin the split: full inheritance for built-ins, the SDK's filtered
+default for everything else, with a server's own explicit env still applied
+on top in both cases.
+"""
+
+import asyncio
+from unittest.mock import patch
+
+import pytest
+
+from src.mcp_manager import McpManager
+
+
+CANARY = "ODYSSEUS_TEST_CANARY_9931"
+
+
+class _Captured(Exception):
+    """Raised by the stub to stop _connect_stdio once params are captured."""
+
+
+def _connect_and_capture(server_id, env=None):
+    """Run _connect_stdio far enough to capture its StdioServerParameters."""
+    seen = {}
+
+    def _stub(params):
+        seen["params"] = params
+        raise _Captured
+
+    mgr = McpManager()
+    with patch("mcp.client.stdio.stdio_client", _stub):
+        try:
+            asyncio.run(
+                mgr._connect_stdio(
+                    server_id, "Test Server", "echo", ["hi"], env or {}
+                )
+            )
+        except _Captured:
+            pass  # expected: the stub aborts once it has the params
+    assert "params" in seen, "stdio_client was never reached"
+    return seen["params"].env
+
+
+@pytest.fixture(autouse=True)
+def _secrets(monkeypatch):
+    monkeypatch.setenv(CANARY, "canary-value")
+    monkeypatch.setenv("ODYSSEUS_INTERNAL_TOKEN", "admin-bypass-token")
+
+
+def test_builtin_server_inherits_full_environment():
+    """Built-ins are our own code — they need the deployment's environment."""
+    env = _connect_and_capture("builtin_browser")
+
+    assert env[CANARY] == "canary-value"
+    assert env["ODYSSEUS_INTERNAL_TOKEN"] == "admin-bypass-token"
+
+
+def test_user_server_does_not_receive_unfiltered_environment():
+    """A user-added server is a third-party process: no secrets by default."""
+    env = _connect_and_capture("user_added_server")
+
+    assert CANARY not in env
+    assert "ODYSSEUS_INTERNAL_TOKEN" not in env
+
+
+def test_user_server_still_gets_the_sdk_default_and_its_own_env():
+    """Filtering must not starve the server of what it legitimately needs."""
+    from mcp.client.stdio import get_default_environment
+
+    env = _connect_and_capture("user_added_server", {"MY_SERVER_KEY": "abc"})
+
+    for key in get_default_environment():
+        assert key in env, f"SDK default {key} was dropped"
+    assert env["MY_SERVER_KEY"] == "abc"
+    assert CANARY not in env
+
+
+def test_explicit_env_overrides_inherited_value_for_builtins():
+    """An explicit per-server value still wins over the inherited one."""
+    env = _connect_and_capture("builtin_browser", {CANARY: "explicit"})
+
+    assert env[CANARY] == "explicit"