Skip to content

Commit e2a41cb

Browse files
thebtfKirill Turanskiyclaude
authored
fix: correct Windows CLI detection for npm-installed tools (#294)
* fix: correct Windows CLI detection for npm-installed tools The CLI managers for Claude Code, Gemini CLI, Codex CLI, and Qwen CLI were incorrectly searching for `.exe` files on Windows. These tools are installed via npm which creates `.cmd` wrapper scripts, not `.exe` files. The fix removes the explicit `.exe` extension check because `shutil.which()` automatically handles Windows PATHEXT environment variable, which includes `.CMD`, `.BAT`, `.EXE`, and other executable extensions. Before: `shutil.which("claude.exe")` -> None (not found) After: `shutil.which("claude")` -> finds claude.cmd via PATHEXT Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: use Path objects for cross-platform path handling - Add get_config_directory() and get_data_directory() to platform.py - Update config.py, global_config.py, profile_config.py to use Path - Update install.py and tunnel.py to use Path for consistent separators - Fix test_profile.py to work with Path objects This fixes mixed path separators on Windows (e.g., C:\Users\btf/.config/mcpm) by using pathlib.Path throughout the codebase. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address PR review comments - Remove unused CERTIFICATE_PATH and DEFAULT_CONFIG_DIR import in tunnel.py - Fix initialization order in GlobalConfigManager (ensure_dirs before loading) - Move Path import to top-level in config.py - Add documentation for architectural decisions in platform.py - Clean up test imports in test_profile.py and test_qwen_cli.py - Remove unnecessary mock in test_qwen_cli_manager_is_client_installed_windows Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address Gemini and CodeRabbit review comments - Add type hints to ConfigManager constructor (config.py) - Add error handling for config save operations (config.py, global_config.py) - Fix type hint: `any` -> `Any` in global_config.py - Refactor CLI detection to reduce duplication (doctor.py) - Update qwen_cli.py to use shutil.which for cross-platform detection - Fix test imports in test_qwen_cli.py Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Kirill Turanskiy <kt@novamedia.ru> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 0eb2bbd commit e2a41cb

13 files changed

Lines changed: 164 additions & 98 deletions

File tree

src/mcpm/clients/managers/claude_code.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,8 @@ def is_client_installed(self) -> bool:
4242
Returns:
4343
bool: True if claude command is available, False otherwise
4444
"""
45-
claude_executable = "claude.exe" if self._system == "Windows" else "claude"
46-
return shutil.which(claude_executable) is not None
45+
# shutil.which() handles Windows PATHEXT automatically (.cmd, .bat, .exe, etc.)
46+
return shutil.which("claude") is not None
4747

4848
def get_client_info(self) -> Dict[str, str]:
4949
"""Get information about this client

src/mcpm/clients/managers/codex_cli.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,8 @@ def is_client_installed(self) -> bool:
4747
Returns:
4848
bool: True if codex command is available, False otherwise
4949
"""
50-
codex_executable = "codex.exe" if self._system == "Windows" else "codex"
51-
return shutil.which(codex_executable) is not None
50+
# shutil.which() handles Windows PATHEXT automatically (.cmd, .bat, .exe, etc.)
51+
return shutil.which("codex") is not None
5252

5353
def get_client_info(self) -> Dict[str, str]:
5454
"""Get information about this client

src/mcpm/clients/managers/gemini_cli.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,8 @@ def is_client_installed(self) -> bool:
4949
Returns:
5050
bool: True if gemini command is available, False otherwise
5151
"""
52-
gemini_executable = "gemini.exe" if self._system == "Windows" else "gemini"
53-
return shutil.which(gemini_executable) is not None
52+
# shutil.which() handles Windows PATHEXT automatically (.cmd, .bat, .exe, etc.)
53+
return shutil.which("gemini") is not None
5454

5555
def get_client_info(self) -> Dict[str, str]:
5656
"""Get information about this client

src/mcpm/clients/managers/qwen_cli.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@
33
"""
44

55
import logging
6-
import os
76
import shutil
7+
from pathlib import Path
88
from typing import Any, Dict
99

1010
from mcpm.clients.base import JSONClientManager
@@ -27,7 +27,7 @@ def __init__(self, config_path_override: str | None = None):
2727
config_path_override: Optional path to override the default config file location
2828
"""
2929
# Qwen CLI stores its settings in ~/.qwen/settings.json
30-
self.config_path = os.path.expanduser("~/.qwen/settings.json")
30+
self.config_path = str(Path.home() / ".qwen" / "settings.json")
3131
super().__init__(config_path_override=config_path_override)
3232

3333
def _get_empty_config(self) -> Dict[str, Any]:
@@ -44,8 +44,8 @@ def is_client_installed(self) -> bool:
4444
Returns:
4545
bool: True if qwen command is available, False otherwise
4646
"""
47-
qwen_executable = "qwen.exe" if self._system == "Windows" else "qwen"
48-
return shutil.which(qwen_executable) is not None
47+
# shutil.which() handles Windows PATHEXT automatically (.cmd, .bat, .exe, etc.)
48+
return shutil.which("qwen") is not None
4949

5050
def get_client_info(self) -> Dict[str, str]:
5151
"""Get information about this client

src/mcpm/commands/doctor.py

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
"""Doctor command for MCPM - System health check and diagnostics"""
22

33
import os
4+
import shutil
45
import subprocess
56
import sys
67
from pathlib import Path
@@ -48,21 +49,27 @@ def doctor():
4849
console.print(f" ✅ Python version: {sys.version.split()[0]}")
4950
console.print(f" ✅ Python executable: {sys.executable}")
5051

52+
# Helper function to check CLI tools
53+
def _check_cli_tool(tool_name: str, display_name: str, not_found_msg: str) -> int:
54+
"""Checks for a CLI tool, prints status, and returns 1 if an issue is found."""
55+
tool_path = shutil.which(tool_name)
56+
if tool_path:
57+
try:
58+
version = subprocess.check_output([tool_path, "--version"], stderr=subprocess.DEVNULL).decode().strip()
59+
console.print(f" ✅ {display_name} version: {version}")
60+
return 0
61+
except (subprocess.CalledProcessError, OSError):
62+
console.print(f" ⚠️ {display_name} found but failed to get version")
63+
return 1
64+
else:
65+
console.print(f" ⚠️ {not_found_msg}")
66+
return 1
67+
5168
# 3. Check Node.js (for npx servers)
69+
# Use shutil.which() to find executables - handles Windows .cmd/.bat files via PATHEXT
5270
console.print("[bold cyan]📊 Node.js Environment[/]")
53-
try:
54-
node_version = subprocess.check_output(["node", "--version"], stderr=subprocess.DEVNULL).decode().strip()
55-
console.print(f" ✅ Node.js version: {node_version}")
56-
except (subprocess.CalledProcessError, FileNotFoundError):
57-
console.print(" ⚠️ Node.js not found - npx servers will not work")
58-
issues_found += 1
59-
60-
try:
61-
npm_version = subprocess.check_output(["npm", "--version"], stderr=subprocess.DEVNULL).decode().strip()
62-
console.print(f" ✅ npm version: {npm_version}")
63-
except (subprocess.CalledProcessError, FileNotFoundError):
64-
console.print(" ⚠️ npm not found - package installation may fail")
65-
issues_found += 1
71+
issues_found += _check_cli_tool("node", "Node.js", "Node.js not found - npx servers will not work")
72+
issues_found += _check_cli_tool("npm", "npm", "npm not found - package installation may fail")
6673

6774
# 4. Check MCPM configuration
6875
console.print("[bold cyan]⚙️ MCPM Configuration[/]")

src/mcpm/commands/install.py

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
from mcpm.schemas.full_server_config import FullServerConfig
2222
from mcpm.utils.config import NODE_EXECUTABLES, ConfigManager
2323
from mcpm.utils.non_interactive import is_explicit_non_interactive, should_force_operation
24+
from mcpm.utils.platform import get_data_directory
2425
from mcpm.utils.repository import RepositoryManager
2526
from mcpm.utils.rich_click_config import click
2627

@@ -55,6 +56,7 @@ def _get_prompt_session() -> Optional[PromptSession]:
5556

5657
return prompt_session
5758

59+
5860
style = Style.from_dict(
5961
{
6062
"prompt": "ansicyan bold",
@@ -223,14 +225,14 @@ def install(server_name, force=False, alias=None):
223225
return
224226

225227
# Create server directory in the MCP directory
226-
base_dir = os.path.expanduser("~/.mcpm")
227-
os.makedirs(base_dir, exist_ok=True)
228+
base_dir = get_data_directory()
229+
base_dir.mkdir(parents=True, exist_ok=True)
228230

229-
servers_dir = os.path.join(base_dir, "servers")
230-
os.makedirs(servers_dir, exist_ok=True)
231+
servers_dir = base_dir / "servers"
232+
servers_dir.mkdir(parents=True, exist_ok=True)
231233

232-
server_dir = os.path.join(servers_dir, server_name)
233-
os.makedirs(server_dir, exist_ok=True)
234+
server_dir = servers_dir / server_name
235+
server_dir.mkdir(parents=True, exist_ok=True)
234236

235237
# Extract installation information
236238
installations = server_metadata.get("installations", {})
@@ -304,7 +306,7 @@ def install(server_name, force=False, alias=None):
304306
with Progress(SpinnerColumn(), TextColumn("[bold green]{task.description}[/]"), console=console) as progress:
305307
# Save metadata to server directory
306308
progress.add_task("Saving server metadata...", total=None)
307-
metadata_path = os.path.join(server_dir, "metadata.json")
309+
metadata_path = server_dir / "metadata.json"
308310
with open(metadata_path, "w", encoding="utf-8") as f:
309311
json.dump(server_metadata, f, indent=2)
310312

src/mcpm/core/tunnel.py

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010

1111
import httpx
1212

13-
from mcpm.utils.config import DEFAULT_CONFIG_DIR
1413
from mcpm.utils.platform import get_frpc_directory
1514

1615
VERSION = "0.3"
@@ -53,8 +52,6 @@
5352
TUNNEL_TIMEOUT_SECONDS = 30
5453
TUNNEL_ERROR_MESSAGE = "Could not create share URL. Please check the appended log from frpc for more information:"
5554

56-
CERTIFICATE_PATH = f"{DEFAULT_CONFIG_DIR}/certificate.pem"
57-
5855

5956
class Tunnel:
6057
def __init__(

src/mcpm/global_config.py

Lines changed: 24 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -7,15 +7,16 @@
77

88
import json
99
import logging
10-
import os
11-
from typing import Dict, List, Optional
10+
from pathlib import Path
11+
from typing import Any, Dict, List, Optional
1212

1313
from pydantic import TypeAdapter
1414

1515
from mcpm.core.schema import ProfileMetadata, ServerConfig
16+
from mcpm.utils.platform import get_config_directory
1617

17-
DEFAULT_GLOBAL_CONFIG_PATH = os.path.expanduser("~/.config/mcpm/servers.json")
18-
DEFAULT_PROFILE_METADATA_PATH = os.path.expanduser("~/.config/mcpm/profiles_metadata.json")
18+
DEFAULT_GLOBAL_CONFIG_PATH = get_config_directory() / "servers.json"
19+
DEFAULT_PROFILE_METADATA_PATH = get_config_directory() / "profiles_metadata.json"
1920

2021
logger = logging.getLogger(__name__)
2122

@@ -28,22 +29,22 @@ class GlobalConfigManager:
2829
"""
2930

3031
def __init__(
31-
self, config_path: str = DEFAULT_GLOBAL_CONFIG_PATH, metadata_path: str = DEFAULT_PROFILE_METADATA_PATH
32+
self, config_path: Path = DEFAULT_GLOBAL_CONFIG_PATH, metadata_path: Path = DEFAULT_PROFILE_METADATA_PATH
3233
):
33-
self.config_path = os.path.expanduser(config_path)
34-
self.metadata_path = os.path.expanduser(metadata_path)
35-
self.config_dir = os.path.dirname(self.config_path)
34+
self.config_path = Path(config_path)
35+
self.metadata_path = Path(metadata_path)
36+
self.config_dir = self.config_path.parent
37+
self._ensure_dirs()
3638
self._servers: Dict[str, ServerConfig] = self._load_servers()
3739
self._profile_metadata: Dict[str, ProfileMetadata] = self._load_profile_metadata()
38-
self._ensure_dirs()
3940

4041
def _ensure_dirs(self) -> None:
4142
"""Ensure all configuration directories exist"""
42-
os.makedirs(self.config_dir, exist_ok=True)
43+
self.config_dir.mkdir(parents=True, exist_ok=True)
4344

4445
def _load_servers(self) -> Dict[str, ServerConfig]:
4546
"""Load servers from the global configuration file."""
46-
if not os.path.exists(self.config_path):
47+
if not self.config_path.exists():
4748
return {}
4849

4950
try:
@@ -68,12 +69,15 @@ def _save_servers(self) -> None:
6869
self._ensure_dirs()
6970
servers_data = {name: config.model_dump() for name, config in self._servers.items()}
7071

71-
with open(self.config_path, "w", encoding="utf-8") as f:
72-
json.dump(servers_data, f, indent=2)
72+
try:
73+
with open(self.config_path, "w", encoding="utf-8") as f:
74+
json.dump(servers_data, f, indent=2)
75+
except OSError as e:
76+
logger.error(f"Error saving servers to {self.config_path}: {e}")
7377

7478
def _load_profile_metadata(self) -> Dict[str, ProfileMetadata]:
7579
"""Load profile metadata from the metadata configuration file."""
76-
if not os.path.exists(self.metadata_path):
80+
if not self.metadata_path.exists():
7781
return {}
7882

7983
try:
@@ -98,8 +102,11 @@ def _save_profile_metadata(self) -> None:
98102
self._ensure_dirs()
99103
metadata_data = {name: meta.model_dump() for name, meta in self._profile_metadata.items()}
100104

101-
with open(self.metadata_path, "w", encoding="utf-8") as f:
102-
json.dump(metadata_data, f, indent=2)
105+
try:
106+
with open(self.metadata_path, "w", encoding="utf-8") as f:
107+
json.dump(metadata_data, f, indent=2)
108+
except OSError as e:
109+
logger.error(f"Error saving profile metadata to {self.metadata_path}: {e}")
103110

104111
def add_server(self, server_config: ServerConfig, force: bool = False) -> bool:
105112
"""Add a server to the global configuration.
@@ -358,7 +365,7 @@ def list_profile_metadata(self) -> Dict[str, ProfileMetadata]:
358365
"""
359366
return self._profile_metadata.copy()
360367

361-
def get_complete_profile(self, name: str) -> Optional[Dict[str, any]]:
368+
def get_complete_profile(self, name: str) -> Optional[Dict[str, Any]]:
362369
"""Get complete profile information including metadata and servers.
363370
364371
Args:

src/mcpm/profile/profile_config.py

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
import logging
2-
import os
2+
from pathlib import Path
33
from typing import Dict, List, Optional
44

55
from mcpm.core.schema import ProfileMetadata, ServerConfig
66
from mcpm.global_config import GlobalConfigManager
7+
from mcpm.utils.platform import get_config_directory
78

8-
DEFAULT_PROFILE_PATH = os.path.expanduser("~/.config/mcpm/profiles.json")
9+
DEFAULT_PROFILE_PATH = get_config_directory() / "profiles.json"
910

1011
logger = logging.getLogger(__name__)
1112

@@ -22,9 +23,9 @@ class ProfileConfigManager:
2223
"""
2324

2425
def __init__(
25-
self, profile_path: str = DEFAULT_PROFILE_PATH, global_config_manager: Optional[GlobalConfigManager] = None
26+
self, profile_path: Path = DEFAULT_PROFILE_PATH, global_config_manager: Optional[GlobalConfigManager] = None
2627
):
27-
self.profile_path = os.path.expanduser(profile_path)
28+
self.profile_path = Path(profile_path)
2829
self.global_config = global_config_manager or GlobalConfigManager()
2930

3031
# Note: Legacy profile migration is now handled by V1ToV2Migrator

src/mcpm/utils/config.py

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,17 @@
44

55
import json
66
import logging
7-
import os
7+
from pathlib import Path
88
from typing import Any, Dict
99

10+
from mcpm.utils.platform import get_config_directory
11+
1012
logger = logging.getLogger(__name__)
1113

12-
# Default configuration paths
13-
DEFAULT_CONFIG_DIR = os.path.expanduser("~/.config/mcpm")
14-
DEFAULT_CONFIG_FILE = os.path.join(DEFAULT_CONFIG_DIR, "config.json")
15-
DEFAULT_AUTH_FILE = os.path.join(DEFAULT_CONFIG_DIR, "auth.json")
14+
# Default configuration paths using platform-specific directories
15+
DEFAULT_CONFIG_DIR = get_config_directory()
16+
DEFAULT_CONFIG_FILE = DEFAULT_CONFIG_DIR / "config.json"
17+
DEFAULT_AUTH_FILE = DEFAULT_CONFIG_DIR / "auth.json"
1618
# Default port for HTTP mode
1719
DEFAULT_PORT = 6276 # 6276 represents MCPM on a T9 keypad (6=M, 2=C, 7=P, 6=M)
1820
# Default share address
@@ -28,10 +30,11 @@ class ConfigManager:
2830
Client-specific configurations are managed by ClientConfigManager.
2931
"""
3032

31-
def __init__(self, config_path: str = DEFAULT_CONFIG_FILE, auth_path: str = DEFAULT_AUTH_FILE):
32-
self.config_path = config_path
33-
self.auth_path = auth_path
34-
self.config_dir = os.path.dirname(config_path)
33+
def __init__(self, config_path: Path | str = DEFAULT_CONFIG_FILE, auth_path: Path | str = DEFAULT_AUTH_FILE):
34+
# Normalize paths to Path objects for consistent handling
35+
self.config_path = Path(config_path)
36+
self.auth_path = Path(auth_path)
37+
self.config_dir = self.config_path.parent
3538
self._config = {}
3639
self._auth_config = {}
3740
self._ensure_dirs()
@@ -40,11 +43,11 @@ def __init__(self, config_path: str = DEFAULT_CONFIG_FILE, auth_path: str = DEFA
4043

4144
def _ensure_dirs(self) -> None:
4245
"""Ensure all configuration directories exist"""
43-
os.makedirs(self.config_dir, exist_ok=True)
46+
self.config_dir.mkdir(parents=True, exist_ok=True)
4447

4548
def _load_config(self) -> None:
4649
"""Load configuration from file or create default"""
47-
if os.path.exists(self.config_path):
50+
if self.config_path.exists():
4851
try:
4952
with open(self.config_path, "r", encoding="utf-8") as f:
5053
self._config = json.load(f)
@@ -57,7 +60,7 @@ def _load_config(self) -> None:
5760

5861
def _load_auth_config(self) -> None:
5962
"""Load auth configuration from file or create default"""
60-
if os.path.exists(self.auth_path):
63+
if self.auth_path.exists():
6164
try:
6265
with open(self.auth_path, "r", encoding="utf-8") as f:
6366
self._auth_config = json.load(f)
@@ -75,13 +78,19 @@ def _default_config(self) -> Dict[str, Any]:
7578

7679
def _save_config(self) -> None:
7780
"""Save current configuration to file"""
78-
with open(self.config_path, "w", encoding="utf-8") as f:
79-
json.dump(self._config, f, indent=2)
81+
try:
82+
with open(self.config_path, "w", encoding="utf-8") as f:
83+
json.dump(self._config, f, indent=2)
84+
except OSError as e:
85+
logger.error(f"Error saving config file: {self.config_path} - {e}")
8086

8187
def _save_auth_config(self) -> None:
8288
"""Save current auth configuration to file"""
83-
with open(self.auth_path, "w", encoding="utf-8") as f:
84-
json.dump(self._auth_config, f, indent=2)
89+
try:
90+
with open(self.auth_path, "w", encoding="utf-8") as f:
91+
json.dump(self._auth_config, f, indent=2)
92+
except OSError as e:
93+
logger.error(f"Error saving auth file: {self.auth_path} - {e}")
8594

8695
def get_config(self) -> Dict[str, Any]:
8796
"""Get the complete configuration"""

0 commit comments

Comments
 (0)