diff --git a/src/mcp/cli/cli.py b/src/mcp/cli/cli.py index 62334a4a2..a91d1dfda 100644 --- a/src/mcp/cli/cli.py +++ b/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) diff --git a/tests/cli/test_utils.py b/tests/cli/test_utils.py index 44f4ab4d3..dda7cb62f 100644 --- a/tests/cli/test_utils.py +++ b/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