diff --git a/nanobot/agent/tools/mcp.py b/nanobot/agent/tools/mcp.py index ab95b446..8db90a03 100644 --- a/nanobot/agent/tools/mcp.py +++ b/nanobot/agent/tools/mcp.py @@ -1,6 +1,8 @@ """MCP client: connects to MCP servers and wraps their tools as native nanobot tools.""" import asyncio +import os +import shutil from contextlib import AsyncExitStack from typing import Any @@ -24,12 +26,50 @@ _TRANSIENT_EXC_NAMES: frozenset[str] = frozenset(( "ConnectionError", )) +_WINDOWS_SHELL_LAUNCHERS: frozenset[str] = frozenset(("npx", "npm", "pnpm", "yarn", "bunx")) + def _is_transient(exc: BaseException) -> bool: """Check if an exception looks like a transient connection error.""" return type(exc).__name__ in _TRANSIENT_EXC_NAMES +def _windows_command_basename(command: str) -> str: + """Return the lowercase basename for a Windows command or path.""" + return command.replace("\\", "/").rsplit("/", maxsplit=1)[-1].lower() + + +def _normalize_windows_stdio_command( + command: str, + args: list[str] | None, + env: dict[str, str] | None, +) -> tuple[str, list[str], dict[str, str] | None]: + """Wrap Windows shell launchers so MCP stdio servers start reliably.""" + normalized_args = list(args or []) + if os.name != "nt": + return command, normalized_args, env + + basename = _windows_command_basename(command) + if basename in {"cmd", "cmd.exe", "powershell", "powershell.exe", "pwsh", "pwsh.exe"}: + return command, normalized_args, env + + if basename.endswith((".exe", ".com")): + return command, normalized_args, env + + resolved = shutil.which(command, path=(env or {}).get("PATH")) or command + resolved_basename = _windows_command_basename(resolved) + should_wrap = ( + basename in _WINDOWS_SHELL_LAUNCHERS + or basename.endswith((".cmd", ".bat")) + or resolved_basename.endswith((".cmd", ".bat")) + ) + if not should_wrap: + return command, normalized_args, env + + comspec = (env or {}).get("COMSPEC") or os.environ.get("COMSPEC") or "cmd.exe" + return comspec, ["/d", "/c", command, *normalized_args], env + + def _extract_nullable_branch(options: Any) -> tuple[dict[str, Any], bool] | None: """Return the single non-null branch for nullable unions.""" if not isinstance(options, list): @@ -416,8 +456,15 @@ async def connect_mcp_servers( return name, None if transport_type == "stdio": + command, args, env = _normalize_windows_stdio_command( + cfg.command, + cfg.args, + cfg.env or None, + ) params = StdioServerParameters( - command=cfg.command, args=cfg.args, env=cfg.env or None + command=command, + args=args, + env=env, ) read, write = await server_stack.enter_async_context(stdio_client(params)) elif transport_type == "sse": diff --git a/tests/tools/test_mcp_tool.py b/tests/tools/test_mcp_tool.py index a133f53d..7732f859 100644 --- a/tests/tools/test_mcp_tool.py +++ b/tests/tools/test_mcp_tool.py @@ -1,16 +1,18 @@ from __future__ import annotations import asyncio -from contextlib import AsyncExitStack, asynccontextmanager import sys +from contextlib import asynccontextmanager from types import ModuleType, SimpleNamespace import pytest +import nanobot.agent.tools.mcp as mcp_mod from nanobot.agent.tools.mcp import ( - MCPResourceWrapper, MCPPromptWrapper, + MCPResourceWrapper, MCPToolWrapper, + _normalize_windows_stdio_command, connect_mcp_servers, ) from nanobot.agent.tools.registry import ToolRegistry @@ -178,6 +180,99 @@ def test_wrapper_normalizes_nullable_property_anyof() -> None: } +def test_normalize_windows_stdio_command_is_noop_off_windows( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(mcp_mod.os, "name", "posix", raising=False) + + command, args, env = _normalize_windows_stdio_command( + "npx", + ["-y", "chrome-devtools-mcp@latest"], + {"FOO": "bar"}, + ) + + assert command == "npx" + assert args == ["-y", "chrome-devtools-mcp@latest"] + assert env == {"FOO": "bar"} + + +def test_normalize_windows_stdio_command_wraps_npx_on_windows( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(mcp_mod.os, "name", "nt", raising=False) + monkeypatch.setattr( + mcp_mod.shutil, + "which", + lambda command, path=None: r"C:\Program Files\nodejs\npx.cmd", + ) + monkeypatch.setenv("COMSPEC", r"C:\Windows\System32\cmd.exe") + + command, args, env = _normalize_windows_stdio_command( + "npx", + ["-y", "chrome-devtools-mcp@latest"], + None, + ) + + assert command == r"C:\Windows\System32\cmd.exe" + assert args == ["/d", "/c", "npx", "-y", "chrome-devtools-mcp@latest"] + assert env is None + + +def test_normalize_windows_stdio_command_wraps_resolved_cmd_launcher( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(mcp_mod.os, "name", "nt", raising=False) + + def _fake_which(command: str, path: str | None = None) -> str: + assert command == "custom-launcher" + assert path == r"C:\Tools" + return r"C:\Tools\custom-launcher.cmd" + + monkeypatch.setattr(mcp_mod.shutil, "which", _fake_which) + monkeypatch.setenv("COMSPEC", r"C:\Windows\System32\cmd.exe") + + command, args, _env = _normalize_windows_stdio_command( + "custom-launcher", + ["serve"], + {"PATH": r"C:\Tools"}, + ) + + assert command == r"C:\Windows\System32\cmd.exe" + assert args == ["/d", "/c", "custom-launcher", "serve"] + + +def test_normalize_windows_stdio_command_keeps_real_executables_unchanged( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(mcp_mod.os, "name", "nt", raising=False) + + command, args, env = _normalize_windows_stdio_command( + "python.exe", + ["-m", "http.server"], + {"FOO": "bar"}, + ) + + assert command == "python.exe" + assert args == ["-m", "http.server"] + assert env == {"FOO": "bar"} + + +def test_normalize_windows_stdio_command_skips_existing_shells( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(mcp_mod.os, "name", "nt", raising=False) + + command, args, env = _normalize_windows_stdio_command( + "cmd.exe", + ["/c", "echo", "hello"], + None, + ) + + assert command == "cmd.exe" + assert args == ["/c", "echo", "hello"] + assert env is None + + @pytest.mark.asyncio async def test_execute_returns_text_blocks() -> None: async def call_tool(_name: str, arguments: dict) -> object: @@ -423,6 +518,48 @@ async def test_connect_mcp_servers_one_failure_does_not_block_others( assert set(stacks) == {"good"} +@pytest.mark.asyncio +async def test_connect_mcp_servers_wraps_windows_stdio_launchers( + fake_mcp_runtime: dict[str, object | None], + monkeypatch: pytest.MonkeyPatch, +) -> None: + fake_mcp_runtime["session"] = _make_fake_session(["demo"]) + captured: dict[str, object] = {} + + @asynccontextmanager + async def _capturing_stdio_client(params: object): + captured["command"] = params.command + captured["args"] = params.args + captured["env"] = params.env + yield object(), object() + + monkeypatch.setattr(mcp_mod.os, "name", "nt", raising=False) + monkeypatch.setattr( + mcp_mod.shutil, + "which", + lambda command, path=None: r"C:\Program Files\nodejs\npx.cmd", + ) + monkeypatch.setenv("COMSPEC", r"C:\Windows\System32\cmd.exe") + monkeypatch.setattr(sys.modules["mcp.client.stdio"], "stdio_client", _capturing_stdio_client) + + registry = ToolRegistry() + stacks = await connect_mcp_servers( + { + "test": MCPServerConfig( + command="npx", + args=["-y", "chrome-devtools-mcp@latest"], + ) + }, + registry, + ) + for stack in stacks.values(): + await stack.aclose() + + assert captured["command"] == r"C:\Windows\System32\cmd.exe" + assert captured["args"] == ["/d", "/c", "npx", "-y", "chrome-devtools-mcp@latest"] + assert captured["env"] is None + + # --------------------------------------------------------------------------- # MCPResourceWrapper tests # ---------------------------------------------------------------------------