From 058a5b33887e0896c3f0b9ff5ee4dbe6a2695420 Mon Sep 17 00:00:00 2001 From: Max Isbey <224885523+maxisbey@users.noreply.github.com> Date: Thu, 19 Mar 2026 13:24:51 +0000 Subject: [PATCH 1/3] test: rewrite cli.claude config tests to assert JSON output directly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous test_command_execution shelled out to 'uv run --frozen --with mcp[cli] mcp run --help' with a 20s timeout to verify the generated command was executable. Appending --help only proves uv is installed; it doesn't validate the command would actually launch a server. update_claude_config is a config-file writer — its contract is producing correct JSON. Test that directly: - Assert exact command + args structure (replaces the subprocess smoke test) - Mock get_uv_path so tests don't depend on the host's uv install - Cover previously-untested option paths: with_packages (sorted/deduped), with_editable, env_vars, env merge-on-reinstall, file specs without :object, preserving other mcpServers entries - Add get_uv_path tests for shutil.which resolution and fallback Removes 7 now-stale '# pragma: no cover' annotations from claude.py. Moves the file to tests/cli/test_claude.py to mirror src/mcp/cli/claude.py. --- src/mcp/cli/claude.py | 14 ++--- tests/cli/test_claude.py | 117 ++++++++++++++++++++++++++++++++++++ tests/client/test_config.py | 75 ----------------------- 3 files changed, 124 insertions(+), 82 deletions(-) create mode 100644 tests/cli/test_claude.py delete mode 100644 tests/client/test_config.py diff --git a/src/mcp/cli/claude.py b/src/mcp/cli/claude.py index 071b4b6fb..4223d2e6d 100644 --- a/src/mcp/cli/claude.py +++ b/src/mcp/cli/claude.py @@ -33,7 +33,7 @@ def get_claude_config_path() -> Path | None: # pragma: no cover def get_uv_path() -> str: """Get the full path to the uv executable.""" uv_path = shutil.which("uv") - if not uv_path: # pragma: no cover + if not uv_path: logger.error( "uv executable not found in PATH, falling back to 'uv'. Please ensure uv is installed and in your PATH" ) @@ -65,7 +65,7 @@ def update_claude_config( """ config_dir = get_claude_config_path() uv_path = get_uv_path() - if not config_dir: # pragma: no cover + if not config_dir: raise RuntimeError( "Claude Desktop config directory not found. Please ensure Claude Desktop" " is installed and has been run at least once to initialize its config." @@ -90,7 +90,7 @@ def update_claude_config( config["mcpServers"] = {} # Always preserve existing env vars and merge with new ones - if server_name in config["mcpServers"] and "env" in config["mcpServers"][server_name]: # pragma: no cover + if server_name in config["mcpServers"] and "env" in config["mcpServers"][server_name]: existing_env = config["mcpServers"][server_name]["env"] if env_vars: # New vars take precedence over existing ones @@ -103,14 +103,14 @@ def update_claude_config( # Collect all packages in a set to deduplicate packages = {MCP_PACKAGE} - if with_packages: # pragma: no cover + if with_packages: packages.update(pkg for pkg in with_packages if pkg) # Add all packages with --with for pkg in sorted(packages): args.extend(["--with", pkg]) - if with_editable: # pragma: no cover + if with_editable: args.extend(["--with-editable", str(with_editable)]) # Convert file path to absolute before adding to command @@ -118,7 +118,7 @@ def update_claude_config( if ":" in file_spec: file_path, server_object = file_spec.rsplit(":", 1) file_spec = f"{Path(file_path).resolve()}:{server_object}" - else: # pragma: no cover + else: file_spec = str(Path(file_spec).resolve()) # Add mcp run command @@ -127,7 +127,7 @@ def update_claude_config( server_config: dict[str, Any] = {"command": uv_path, "args": args} # Add environment variables if specified - if env_vars: # pragma: no cover + if env_vars: server_config["env"] = env_vars config["mcpServers"][server_name] = server_config diff --git a/tests/cli/test_claude.py b/tests/cli/test_claude.py new file mode 100644 index 000000000..3b580541a --- /dev/null +++ b/tests/cli/test_claude.py @@ -0,0 +1,117 @@ +"""Tests for mcp.cli.claude — Claude Desktop config file generation.""" + +import json +from pathlib import Path +from typing import Any + +import pytest + +from mcp.cli.claude import get_uv_path, update_claude_config + + +@pytest.fixture +def config_dir(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """Temp Claude config dir with get_claude_config_path and get_uv_path mocked.""" + claude_dir = tmp_path / "Claude" + claude_dir.mkdir() + monkeypatch.setattr("mcp.cli.claude.get_claude_config_path", lambda: claude_dir) + monkeypatch.setattr("mcp.cli.claude.get_uv_path", lambda: "/fake/bin/uv") + return claude_dir + + +def _read_server(config_dir: Path, name: str) -> dict[str, Any]: + config = json.loads((config_dir / "claude_desktop_config.json").read_text()) + return config["mcpServers"][name] + + +def test_generates_uv_run_command(config_dir: Path): + """Should write a uv run command that invokes mcp run on the resolved file spec.""" + assert update_claude_config(file_spec="server.py:app", server_name="my_server") + + resolved = Path("server.py").resolve() + assert _read_server(config_dir, "my_server") == { + "command": "/fake/bin/uv", + "args": ["run", "--frozen", "--with", "mcp[cli]", "mcp", "run", f"{resolved}:app"], + } + + +def test_file_spec_without_object_suffix(config_dir: Path): + """File specs without :object should still resolve to an absolute path.""" + assert update_claude_config(file_spec="server.py", server_name="s") + + assert _read_server(config_dir, "s")["args"][-1] == str(Path("server.py").resolve()) + + +def test_with_packages_sorted_and_deduplicated(config_dir: Path): + """Extra packages should appear as --with flags, sorted and deduplicated with mcp[cli].""" + assert update_claude_config(file_spec="s.py:app", server_name="s", with_packages=["zebra", "aardvark", "zebra"]) + + args = _read_server(config_dir, "s")["args"] + assert args[:8] == ["run", "--frozen", "--with", "aardvark", "--with", "mcp[cli]", "--with", "zebra"] + + +def test_with_editable_adds_flag(config_dir: Path, tmp_path: Path): + """with_editable should add --with-editable after the --with flags.""" + editable = tmp_path / "project" + assert update_claude_config(file_spec="s.py:app", server_name="s", with_editable=editable) + + args = _read_server(config_dir, "s")["args"] + assert args[4:6] == ["--with-editable", str(editable)] + + +def test_env_vars_written(config_dir: Path): + """env_vars should be written under the server's env key.""" + assert update_claude_config(file_spec="s.py:app", server_name="s", env_vars={"KEY": "val"}) + + assert _read_server(config_dir, "s")["env"] == {"KEY": "val"} + + +def test_existing_env_vars_merged_new_wins(config_dir: Path): + """Re-installing should merge env vars, with new values overriding existing ones.""" + (config_dir / "claude_desktop_config.json").write_text( + json.dumps({"mcpServers": {"s": {"env": {"OLD": "keep", "KEY": "old"}}}}) + ) + + assert update_claude_config(file_spec="s.py:app", server_name="s", env_vars={"KEY": "new"}) + + assert _read_server(config_dir, "s")["env"] == {"OLD": "keep", "KEY": "new"} + + +def test_existing_env_vars_preserved_without_new(config_dir: Path): + """Re-installing without env_vars should keep the existing env block intact.""" + (config_dir / "claude_desktop_config.json").write_text(json.dumps({"mcpServers": {"s": {"env": {"KEEP": "me"}}}})) + + assert update_claude_config(file_spec="s.py:app", server_name="s") + + assert _read_server(config_dir, "s")["env"] == {"KEEP": "me"} + + +def test_other_servers_preserved(config_dir: Path): + """Installing a new server should not clobber existing mcpServers entries.""" + (config_dir / "claude_desktop_config.json").write_text(json.dumps({"mcpServers": {"other": {"command": "x"}}})) + + assert update_claude_config(file_spec="s.py:app", server_name="s") + + config = json.loads((config_dir / "claude_desktop_config.json").read_text()) + assert set(config["mcpServers"]) == {"other", "s"} + assert config["mcpServers"]["other"] == {"command": "x"} + + +def test_raises_when_config_dir_missing(monkeypatch: pytest.MonkeyPatch): + """Should raise RuntimeError when Claude Desktop config dir can't be found.""" + monkeypatch.setattr("mcp.cli.claude.get_claude_config_path", lambda: None) + monkeypatch.setattr("mcp.cli.claude.get_uv_path", lambda: "/fake/bin/uv") + + with pytest.raises(RuntimeError, match="Claude Desktop config directory not found"): + update_claude_config(file_spec="s.py:app", server_name="s") + + +@pytest.mark.parametrize("which_result, expected", [("/usr/local/bin/uv", "/usr/local/bin/uv"), (None, "uv")]) +def test_get_uv_path(monkeypatch: pytest.MonkeyPatch, which_result: str | None, expected: str): + """Should return shutil.which's result, or fall back to bare 'uv' when not on PATH.""" + + def fake_which(cmd: str) -> str | None: + return which_result + + monkeypatch.setattr("shutil.which", fake_which) + assert get_uv_path() == expected diff --git a/tests/client/test_config.py b/tests/client/test_config.py deleted file mode 100644 index d1a0576ff..000000000 --- a/tests/client/test_config.py +++ /dev/null @@ -1,75 +0,0 @@ -import json -import subprocess -from pathlib import Path -from unittest.mock import patch - -import pytest - -from mcp.cli.claude import update_claude_config - - -@pytest.fixture -def temp_config_dir(tmp_path: Path): - """Create a temporary Claude config directory.""" - config_dir = tmp_path / "Claude" - config_dir.mkdir() - return config_dir - - -@pytest.fixture -def mock_config_path(temp_config_dir: Path): - """Mock get_claude_config_path to return our temporary directory.""" - with patch("mcp.cli.claude.get_claude_config_path", return_value=temp_config_dir): - yield temp_config_dir - - -def test_command_execution(mock_config_path: Path): - """Test that the generated command can actually be executed.""" - # Setup - server_name = "test_server" - file_spec = "test_server.py:app" - - # Update config - success = update_claude_config(file_spec=file_spec, server_name=server_name) - assert success - - # Read the generated config - config_file = mock_config_path / "claude_desktop_config.json" - config = json.loads(config_file.read_text()) - - # Get the command and args - server_config = config["mcpServers"][server_name] - command = server_config["command"] - args = server_config["args"] - - test_args = [command] + args + ["--help"] - - result = subprocess.run(test_args, capture_output=True, text=True, timeout=20, check=False) - - assert result.returncode == 0 - assert "usage" in result.stdout.lower() - - -def test_absolute_uv_path(mock_config_path: Path): - """Test that the absolute path to uv is used when available.""" - # Mock the shutil.which function to return a fake path - mock_uv_path = "/usr/local/bin/uv" - - with patch("mcp.cli.claude.get_uv_path", return_value=mock_uv_path): - # Setup - server_name = "test_server" - file_spec = "test_server.py:app" - - # Update config - success = update_claude_config(file_spec=file_spec, server_name=server_name) - assert success - - # Read the generated config - config_file = mock_config_path / "claude_desktop_config.json" - config = json.loads(config_file.read_text()) - - # Verify the command is the absolute path - server_config = config["mcpServers"][server_name] - command = server_config["command"] - - assert command == mock_uv_path From 1b1d4044fd3bcc3f0d9f6ddd3f8fd144f6beff31 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 19 Mar 2026 13:39:27 +0000 Subject: [PATCH 2/3] fix: handle Windows drive letters in update_claude_config Before this fix, `update_claude_config` would incorrectly split Windows paths like `C:\Users\foo\server.py` on the drive letter colon, treating `\Users\foo\server.py` as an object suffix. This would generate broken config entries that made MCP servers unlaunchable from Claude Desktop on Windows. Apply the same Windows drive-letter detection logic already present in `_parse_file_path` (cli.py:97-102): check if position 1 is a colon, and if so, only consider colons after position 2 as object-suffix separators. Add test coverage for Windows paths with and without `:object` suffixes. Co-authored-by: Felix Weinberger --- src/mcp/cli/claude.py | 6 +++++- tests/cli/test_claude.py | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/mcp/cli/claude.py b/src/mcp/cli/claude.py index 4223d2e6d..93bf218fb 100644 --- a/src/mcp/cli/claude.py +++ b/src/mcp/cli/claude.py @@ -115,7 +115,11 @@ def update_claude_config( # Convert file path to absolute before adding to command # Split off any :object suffix first - if ":" in file_spec: + # First check if we have a Windows path (e.g., C:\...) + has_windows_drive = len(file_spec) > 1 and file_spec[1] == ":" + + # Split on the last colon, but only if it's not part of the Windows drive letter + if ":" in (file_spec[2:] if has_windows_drive else file_spec): file_path, server_object = file_spec.rsplit(":", 1) file_spec = f"{Path(file_path).resolve()}:{server_object}" else: diff --git a/tests/cli/test_claude.py b/tests/cli/test_claude.py index 3b580541a..c233c58da 100644 --- a/tests/cli/test_claude.py +++ b/tests/cli/test_claude.py @@ -115,3 +115,39 @@ def fake_which(cmd: str) -> str | None: monkeypatch.setattr("shutil.which", fake_which) assert get_uv_path() == expected + + +def test_windows_drive_letter_without_object(config_dir: Path, monkeypatch: pytest.MonkeyPatch): + """Windows paths like C:\\path\\server.py without :object should not split on drive letter.""" + # Mock Path.resolve to return a fake Windows-style path + original_resolve = Path.resolve + + def fake_resolve(self: Path) -> Path: + if str(self) == "C:\\Users\\foo\\server.py": + # Return the same path as if it were already resolved + return self + return original_resolve(self) + + monkeypatch.setattr(Path, "resolve", fake_resolve) + + assert update_claude_config(file_spec="C:\\Users\\foo\\server.py", server_name="s") + + # Should use the full path without splitting on the drive letter colon + assert _read_server(config_dir, "s")["args"][-1] == "C:\\Users\\foo\\server.py" + + +def test_windows_drive_letter_with_object(config_dir: Path, monkeypatch: pytest.MonkeyPatch): + """Windows paths like C:\\path\\server.py:app should only split on the :app suffix.""" + original_resolve = Path.resolve + + def fake_resolve(self: Path) -> Path: + if str(self) == "C:\\Users\\foo\\server.py": + return self + return original_resolve(self) + + monkeypatch.setattr(Path, "resolve", fake_resolve) + + assert update_claude_config(file_spec="C:\\Users\\foo\\server.py:app", server_name="s") + + # Should split on :app but not on C: + assert _read_server(config_dir, "s")["args"][-1] == "C:\\Users\\foo\\server.py:app" From b641234f99792005bfaec4c36e0515a2b9f065be Mon Sep 17 00:00:00 2001 From: Max Isbey <224885523+maxisbey@users.noreply.github.com> Date: Thu, 19 Mar 2026 13:45:07 +0000 Subject: [PATCH 3/3] test: simplify Windows drive-letter test to fix coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous tests mocked Path.resolve with a conditional that fell through to the original implementation — but with the fix in place, only the full path ever hits resolve(), so the fallthrough was dead code and missed coverage (lines 129, 146). Replace with a single parametrized test that records what resolve() was called with. Before the fix, a bare 'C:\path\server.py' would rsplit on the drive colon and call resolve(Path('C')); now it calls resolve on the full path. The recorder asserts that directly — no dead branches. Also covers the has_windows_drive=True + :object-suffix branch that was previously missed. --- tests/cli/test_claude.py | 49 +++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 28 deletions(-) diff --git a/tests/cli/test_claude.py b/tests/cli/test_claude.py index c233c58da..73d4f0eb5 100644 --- a/tests/cli/test_claude.py +++ b/tests/cli/test_claude.py @@ -117,37 +117,30 @@ def fake_which(cmd: str) -> str | None: assert get_uv_path() == expected -def test_windows_drive_letter_without_object(config_dir: Path, monkeypatch: pytest.MonkeyPatch): - """Windows paths like C:\\path\\server.py without :object should not split on drive letter.""" - # Mock Path.resolve to return a fake Windows-style path - original_resolve = Path.resolve +@pytest.mark.parametrize( + "file_spec, expected_last_arg", + [ + ("C:\\Users\\server.py", "C:\\Users\\server.py"), + ("C:\\Users\\server.py:app", "C:\\Users\\server.py:app"), + ], +) +def test_windows_drive_letter_not_split( + config_dir: Path, monkeypatch: pytest.MonkeyPatch, file_spec: str, expected_last_arg: str +): + """Drive-letter paths like 'C:\\server.py' must not be split on the drive colon. + + Before the fix, a bare 'C:\\path\\server.py' would hit rsplit(":", 1) and yield + ("C", "\\path\\server.py"), calling resolve() on Path("C") instead of the full path. + """ + seen: list[str] = [] def fake_resolve(self: Path) -> Path: - if str(self) == "C:\\Users\\foo\\server.py": - # Return the same path as if it were already resolved - return self - return original_resolve(self) + seen.append(str(self)) + return self monkeypatch.setattr(Path, "resolve", fake_resolve) - assert update_claude_config(file_spec="C:\\Users\\foo\\server.py", server_name="s") + assert update_claude_config(file_spec=file_spec, server_name="s") - # Should use the full path without splitting on the drive letter colon - assert _read_server(config_dir, "s")["args"][-1] == "C:\\Users\\foo\\server.py" - - -def test_windows_drive_letter_with_object(config_dir: Path, monkeypatch: pytest.MonkeyPatch): - """Windows paths like C:\\path\\server.py:app should only split on the :app suffix.""" - original_resolve = Path.resolve - - def fake_resolve(self: Path) -> Path: - if str(self) == "C:\\Users\\foo\\server.py": - return self - return original_resolve(self) - - monkeypatch.setattr(Path, "resolve", fake_resolve) - - assert update_claude_config(file_spec="C:\\Users\\foo\\server.py:app", server_name="s") - - # Should split on :app but not on C: - assert _read_server(config_dir, "s")["args"][-1] == "C:\\Users\\foo\\server.py:app" + assert seen == ["C:\\Users\\server.py"] + assert _read_server(config_dir, "s")["args"][-1] == expected_last_arg