fix(cli): avoid Windows shell for mcp dev

Author: Genmin

src/mcp/cli/cli.py

@@ -3,6 +3,7 @@
 import importlib.metadata
 import importlib.util
 import os
+import shutil
 import subprocess
 import sys
 from pathlib import Path
@@ -44,11 +45,8 @@ def _get_npx_command():
     if sys.platform == "win32":
         # Try both npx.cmd and npx.exe on Windows
         for cmd in ["npx.cmd", "npx.exe", "npx"]:
-            try:
-                subprocess.run([cmd, "--version"], check=True, capture_output=True, shell=True)
-                return cmd
-            except subprocess.CalledProcessError:
-                continue
+            if resolved := shutil.which(cmd):
+                return resolved
         return None
     return "npx"  # On Unix-like systems, just use npx
 
@@ -241,7 +239,7 @@ def dev(
             help="Additional packages to install",
         ),
     ] = [],
-) -> None:  # pragma: no cover
+) -> None:
     """Run an MCP server with the MCP Inspector."""
     file, server_object = _parse_file_path(file_spec)
 
@@ -271,12 +269,10 @@ def dev(
             )
             sys.exit(1)
 
-        # Run the MCP Inspector command with shell=True on Windows
-        shell = sys.platform == "win32"
+        # Run the MCP Inspector command.
         process = subprocess.run(
             [npx_cmd, "@modelcontextprotocol/inspector"] + uv_cmd,
             check=True,
-            shell=shell,
             env=dict(os.environ.items()),  # Convert to list of tuples for env update
         )
         sys.exit(process.returncode)

tests/cli/test_utils.py

@@ -5,6 +5,7 @@
 
 import pytest
 
+from mcp.cli import cli
 from mcp.cli.cli import _build_uv_command, _get_npx_command, _parse_file_path  # type: ignore[reportPrivateUsage]
 
 
@@ -79,23 +80,154 @@ def test_get_npx_windows(monkeypatch: pytest.MonkeyPatch):
     """Should return one of the npx candidates on Windows."""
     candidates = ["npx.cmd", "npx.exe", "npx"]
 
-    def fake_run(cmd: list[str], **kw: Any) -> subprocess.CompletedProcess[bytes]:
-        if cmd[0] in candidates:
-            return subprocess.CompletedProcess(cmd, 0)
-        else:  # pragma: no cover
-            raise subprocess.CalledProcessError(1, cmd[0])
+    def fake_which(cmd: str) -> str | None:
+        return f"C:\\node\\{cmd}" if cmd in candidates else None
 
     monkeypatch.setattr(sys, "platform", "win32")
-    monkeypatch.setattr(subprocess, "run", fake_run)
-    assert _get_npx_command() in candidates
+    monkeypatch.setattr(cli.shutil, "which", fake_which)
+    assert _get_npx_command() == "C:\\node\\npx.cmd"
 
 
 def test_get_npx_returns_none_when_npx_missing(monkeypatch: pytest.MonkeyPatch):
     """Should give None if every candidate fails."""
     monkeypatch.setattr(sys, "platform", "win32", raising=False)
 
-    def always_fail(*args: Any, **kwargs: Any) -> subprocess.CompletedProcess[bytes]:
-        raise subprocess.CalledProcessError(1, args[0])
+    def no_npx(_cmd: str) -> str | None:
+        return None
 
-    monkeypatch.setattr(subprocess, "run", always_fail)
+    monkeypatch.setattr(cli.shutil, "which", no_npx)
     assert _get_npx_command() is None
+
+
+def test_dev_uses_arg_list_without_shell_on_windows(monkeypatch: pytest.MonkeyPatch, tmp_path: Path):
+    """Should not route user-controlled dev paths through cmd.exe on Windows."""
+    server_path = tmp_path / "server&calc.py"
+    captured: dict[str, Any] = {}
+
+    def fake_run(cmd: list[str], **kwargs: Any) -> subprocess.CompletedProcess[bytes]:
+        captured["cmd"] = cmd
+        captured["kwargs"] = kwargs
+        return subprocess.CompletedProcess(cmd, 0)
+
+    def fake_parse_file_path(_file_spec: str) -> tuple[Path, str | None]:
+        return server_path, None
+
+    def fake_import_server(_file: Path, _server_object: str | None) -> object:
+        return object()
+
+    def fake_get_npx_command() -> str:
+        return "C:\\node\\npx.cmd"
+
+    monkeypatch.setattr(sys, "platform", "win32")
+    monkeypatch.setattr(cli, "_parse_file_path", fake_parse_file_path)
+    monkeypatch.setattr(cli, "_import_server", fake_import_server)
+    monkeypatch.setattr(cli, "_get_npx_command", fake_get_npx_command)
+    monkeypatch.setattr(cli.subprocess, "run", fake_run)
+
+    with pytest.raises(SystemExit) as exc_info:
+        cli.dev(str(server_path))
+
+    assert exc_info.value.code == 0
+    assert captured["cmd"][-1] == str(server_path)
+    assert captured["kwargs"].get("shell") is not True
+
+
+def test_dev_adds_server_dependencies(monkeypatch: pytest.MonkeyPatch, tmp_path: Path):
+    """Should include dependencies declared by the imported server."""
+    server_path = tmp_path / "server.py"
+    captured: dict[str, Any] = {}
+
+    class Server:
+        dependencies = ["server-extra"]
+
+    def fake_run(cmd: list[str], **kwargs: Any) -> subprocess.CompletedProcess[bytes]:
+        captured["cmd"] = cmd
+        captured["kwargs"] = kwargs
+        return subprocess.CompletedProcess(cmd, 0)
+
+    def fake_parse_file_path(_file_spec: str) -> tuple[Path, str | None]:
+        return server_path, None
+
+    def fake_import_server(_file: Path, _server_object: str | None) -> object:
+        return Server()
+
+    monkeypatch.setattr(cli, "_parse_file_path", fake_parse_file_path)
+    monkeypatch.setattr(cli, "_import_server", fake_import_server)
+    monkeypatch.setattr(cli, "_get_npx_command", lambda: "npx")
+    monkeypatch.setattr(cli.subprocess, "run", fake_run)
+
+    with pytest.raises(SystemExit) as exc_info:
+        cli.dev(str(server_path), with_packages=["direct-extra"])
+
+    assert exc_info.value.code == 0
+    with_values = [captured["cmd"][index + 1] for index, value in enumerate(captured["cmd"]) if value == "--with"]
+    assert with_values[0] == "mcp"
+    assert set(with_values[1:]) == {"direct-extra", "server-extra"}
+
+
+def test_dev_exits_when_npx_missing(monkeypatch: pytest.MonkeyPatch, tmp_path: Path):
+    """Should exit before spawning the inspector if npx cannot be found."""
+    server_path = tmp_path / "server.py"
+
+    def fake_parse_file_path(_file_spec: str) -> tuple[Path, str | None]:
+        return server_path, None
+
+    def fake_import_server(_file: Path, _server_object: str | None) -> object:
+        return object()
+
+    monkeypatch.setattr(cli, "_parse_file_path", fake_parse_file_path)
+    monkeypatch.setattr(cli, "_import_server", fake_import_server)
+    monkeypatch.setattr(cli, "_get_npx_command", lambda: None)
+
+    with pytest.raises(SystemExit) as exc_info:
+        cli.dev(str(server_path))
+
+    assert exc_info.value.code == 1
+
+
+def test_dev_exits_with_process_returncode(monkeypatch: pytest.MonkeyPatch, tmp_path: Path):
+    """Should propagate inspector process failures."""
+    server_path = tmp_path / "server.py"
+
+    def fake_run(cmd: list[str], **_kwargs: Any) -> subprocess.CompletedProcess[bytes]:
+        raise subprocess.CalledProcessError(7, cmd)
+
+    def fake_parse_file_path(_file_spec: str) -> tuple[Path, str | None]:
+        return server_path, None
+
+    def fake_import_server(_file: Path, _server_object: str | None) -> object:
+        return object()
+
+    monkeypatch.setattr(cli, "_parse_file_path", fake_parse_file_path)
+    monkeypatch.setattr(cli, "_import_server", fake_import_server)
+    monkeypatch.setattr(cli, "_get_npx_command", lambda: "npx")
+    monkeypatch.setattr(cli.subprocess, "run", fake_run)
+
+    with pytest.raises(SystemExit) as exc_info:
+        cli.dev(str(server_path))
+
+    assert exc_info.value.code == 7
+
+
+def test_dev_exits_when_subprocess_cannot_find_npx(monkeypatch: pytest.MonkeyPatch, tmp_path: Path):
+    """Should handle npx disappearing after discovery."""
+    server_path = tmp_path / "server.py"
+
+    def fake_run(_cmd: list[str], **_kwargs: Any) -> subprocess.CompletedProcess[bytes]:
+        raise FileNotFoundError()
+
+    def fake_parse_file_path(_file_spec: str) -> tuple[Path, str | None]:
+        return server_path, None
+
+    def fake_import_server(_file: Path, _server_object: str | None) -> object:
+        return object()
+
+    monkeypatch.setattr(cli, "_parse_file_path", fake_parse_file_path)
+    monkeypatch.setattr(cli, "_import_server", fake_import_server)
+    monkeypatch.setattr(cli, "_get_npx_command", lambda: "npx")
+    monkeypatch.setattr(cli.subprocess, "run", fake_run)
+
+    with pytest.raises(SystemExit) as exc_info:
+        cli.dev(str(server_path))
+
+    assert exc_info.value.code == 1