Skip to content

Commit a43e2cb

Browse files
Kirill Turanskiyclaude
andcommitted
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>
1 parent 834cfd8 commit a43e2cb

6 files changed

Lines changed: 23 additions & 23 deletions

File tree

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 = DEFAULT_CONFIG_DIR / "certificate.pem"
57-
5855

5956
class Tunnel:
6057
def __init__(

src/mcpm/global_config.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,9 @@ def __init__(
3434
self.config_path = Path(config_path)
3535
self.metadata_path = Path(metadata_path)
3636
self.config_dir = self.config_path.parent
37+
self._ensure_dirs()
3738
self._servers: Dict[str, ServerConfig] = self._load_servers()
3839
self._profile_metadata: Dict[str, ProfileMetadata] = self._load_profile_metadata()
39-
self._ensure_dirs()
4040

4141
def _ensure_dirs(self) -> None:
4242
"""Ensure all configuration directories exist"""

src/mcpm/utils/config.py

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
import json
66
import logging
7+
from pathlib import Path
78
from typing import Any, Dict
89

910
from mcpm.utils.platform import get_config_directory
@@ -30,8 +31,6 @@ class ConfigManager:
3031
"""
3132

3233
def __init__(self, config_path=DEFAULT_CONFIG_FILE, auth_path=DEFAULT_AUTH_FILE):
33-
from pathlib import Path
34-
3534
# Normalize paths to Path objects for consistent handling
3635
self.config_path = Path(config_path)
3736
self.auth_path = Path(auth_path)

src/mcpm/utils/platform.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,10 @@ def get_config_directory(app_name: str = "mcpm") -> Path:
8080
Uses ~/.config/mcpm on all platforms for consistency.
8181
Path is returned as a Path object with correct separators for the current OS.
8282
83+
Note: This intentionally differs from get_pid_directory() and get_frpc_directory()
84+
which use platform-specific paths. The config directory uses a unified location
85+
to simplify user configuration and documentation across all platforms.
86+
8387
Args:
8488
app_name: The name of the application, used in the path
8589
@@ -96,6 +100,10 @@ def get_data_directory(app_name: str = "mcpm") -> Path:
96100
Uses ~/.mcpm on all platforms for consistency (stores server metadata, etc.).
97101
Path is returned as a Path object with correct separators for the current OS.
98102
103+
Note: This intentionally differs from get_pid_directory() and get_frpc_directory()
104+
which use platform-specific paths. The data directory uses a unified location
105+
to simplify user configuration and documentation across all platforms.
106+
99107
Args:
100108
app_name: The name of the application, used in the path
101109

tests/test_clients/test_qwen_cli.py

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -50,23 +50,23 @@ def test_qwen_cli_manager_is_client_installed():
5050

5151

5252
def test_qwen_cli_manager_is_client_installed_windows():
53-
"""Test QwenCliManager is_client_installed method on Windows
53+
"""Test QwenCliManager is_client_installed method
5454
5555
Note: shutil.which() handles Windows PATHEXT automatically, so we always
5656
search for "qwen" without extension. This finds qwen.cmd, qwen.ps1, qwen.exe, etc.
57+
The test is OS-agnostic as shutil.which handles platform differences.
5758
"""
5859
manager = QwenCliManager()
5960

60-
with patch.object(manager, "_system", "Windows"):
61-
# Mock shutil.which to return a path (simulating installed client via npm .cmd)
62-
with patch("shutil.which", return_value="C:\\Users\\user\\AppData\\Roaming\\npm\\qwen.cmd") as mock_which:
63-
assert manager.is_client_installed() is True
64-
mock_which.assert_called_with("qwen") # No .exe - shutil.which handles PATHEXT
61+
# Mock shutil.which to return a path (simulating installed client via npm .cmd)
62+
with patch("shutil.which", return_value="C:\\Users\\user\\AppData\\Roaming\\npm\\qwen.cmd") as mock_which:
63+
assert manager.is_client_installed() is True
64+
mock_which.assert_called_with("qwen") # No .exe - shutil.which handles PATHEXT
6565

66-
# Mock shutil.which to return None (simulating uninstalled client)
67-
with patch("shutil.which", return_value=None) as mock_which:
68-
assert manager.is_client_installed() is False
69-
mock_which.assert_called_with("qwen")
66+
# Mock shutil.which to return None (simulating uninstalled client)
67+
with patch("shutil.which", return_value=None) as mock_which:
68+
assert manager.is_client_installed() is False
69+
mock_which.assert_called_with("qwen")
7070

7171

7272
def test_qwen_cli_manager_get_empty_config_structure():

tests/test_profile.py

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,14 @@
55
import json
66
import os
77
import tempfile
8+
from pathlib import Path
89

910
import pytest
1011

11-
from mcpm.core.schema import STDIOServerConfig
12+
from mcpm.core.schema import ProfileMetadata, STDIOServerConfig
1213
from mcpm.global_config import GlobalConfigManager
1314
from mcpm.profile.profile_config import ProfileConfigManager
15+
from mcpm.utils.platform import get_config_directory
1416

1517

1618
@pytest.fixture
@@ -57,17 +59,13 @@ def profile_manager_with_legacy(temp_dirs):
5759

5860
def test_profile_manager_init_default_path():
5961
"""Test that the profile manager initializes with default path"""
60-
from mcpm.utils.platform import get_config_directory
61-
6262
manager = ProfileConfigManager()
6363
expected_path = get_config_directory() / "profiles.json"
6464
assert manager.profile_path == expected_path
6565

6666

6767
def test_profile_manager_init_custom_path(profile_manager_clean, temp_dirs):
6868
"""Test that the profile manager initializes with a custom path"""
69-
from pathlib import Path
70-
7169
temp_dir, servers_path, metadata_path, legacy_path = temp_dirs
7270
manager = profile_manager_clean
7371
assert manager.profile_path == Path(legacy_path)
@@ -341,8 +339,6 @@ def test_profile_metadata(profile_manager_clean):
341339
assert metadata.api_key is None
342340

343341
# Update metadata
344-
from mcpm.core.schema import ProfileMetadata
345-
346342
new_metadata = ProfileMetadata(name="api_profile", api_key="sk-test-123", description="Test profile")
347343
manager.update_profile_metadata(new_metadata)
348344

0 commit comments

Comments
 (0)