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>

Author: Kirill Turanskiy

src/mcpm/clients/managers/qwen_cli.py

@@ -3,8 +3,8 @@
 """
 
 import logging
-import os
 import shutil
+from pathlib import Path
 from typing import Any, Dict
 
 from mcpm.clients.base import JSONClientManager
@@ -27,7 +27,7 @@ def __init__(self, config_path_override: str | None = None):
             config_path_override: Optional path to override the default config file location
         """
         # Qwen CLI stores its settings in ~/.qwen/settings.json
-        self.config_path = os.path.expanduser("~/.qwen/settings.json")
+        self.config_path = str(Path.home() / ".qwen" / "settings.json")
         super().__init__(config_path_override=config_path_override)
 
     def _get_empty_config(self) -> Dict[str, Any]:

src/mcpm/commands/doctor.py

@@ -49,32 +49,27 @@ def doctor():
     console.print(f"  ✅ Python version: {sys.version.split()[0]}")
     console.print(f"  ✅ Python executable: {sys.executable}")
 
+    # Helper function to check CLI tools
+    def _check_cli_tool(tool_name: str, display_name: str, not_found_msg: str) -> int:
+        """Checks for a CLI tool, prints status, and returns 1 if an issue is found."""
+        tool_path = shutil.which(tool_name)
+        if tool_path:
+            try:
+                version = subprocess.check_output([tool_path, "--version"], stderr=subprocess.DEVNULL).decode().strip()
+                console.print(f"  ✅ {display_name} version: {version}")
+                return 0
+            except (subprocess.CalledProcessError, OSError):
+                console.print(f"  ⚠️  {display_name} found but failed to get version")
+                return 1
+        else:
+            console.print(f"  ⚠️  {not_found_msg}")
+            return 1
+
     # 3. Check Node.js (for npx servers)
     # Use shutil.which() to find executables - handles Windows .cmd/.bat files via PATHEXT
     console.print("[bold cyan]📊 Node.js Environment[/]")
-    node_path = shutil.which("node")
-    if node_path:
-        try:
-            node_version = subprocess.check_output([node_path, "--version"], stderr=subprocess.DEVNULL).decode().strip()
-            console.print(f"  ✅ Node.js version: {node_version}")
-        except (subprocess.CalledProcessError, OSError):
-            console.print("  ⚠️  Node.js found but failed to get version")
-            issues_found += 1
-    else:
-        console.print("  ⚠️  Node.js not found - npx servers will not work")
-        issues_found += 1
-
-    npm_path = shutil.which("npm")
-    if npm_path:
-        try:
-            npm_version = subprocess.check_output([npm_path, "--version"], stderr=subprocess.DEVNULL).decode().strip()
-            console.print(f"  ✅ npm version: {npm_version}")
-        except (subprocess.CalledProcessError, OSError):
-            console.print("  ⚠️  npm found but failed to get version")
-            issues_found += 1
-    else:
-        console.print("  ⚠️  npm not found - package installation may fail")
-        issues_found += 1
+    issues_found += _check_cli_tool("node", "Node.js", "Node.js not found - npx servers will not work")
+    issues_found += _check_cli_tool("npm", "npm", "npm not found - package installation may fail")
 
     # 4. Check MCPM configuration
     console.print("[bold cyan]⚙️  MCPM Configuration[/]")

src/mcpm/global_config.py

@@ -8,7 +8,7 @@
 import json
 import logging
 from pathlib import Path
-from typing import Dict, List, Optional
+from typing import Any, Dict, List, Optional
 
 from pydantic import TypeAdapter
 
@@ -69,8 +69,11 @@ def _save_servers(self) -> None:
         self._ensure_dirs()
         servers_data = {name: config.model_dump() for name, config in self._servers.items()}
 
-        with open(self.config_path, "w", encoding="utf-8") as f:
-            json.dump(servers_data, f, indent=2)
+        try:
+            with open(self.config_path, "w", encoding="utf-8") as f:
+                json.dump(servers_data, f, indent=2)
+        except OSError as e:
+            logger.error(f"Error saving servers to {self.config_path}: {e}")
 
     def _load_profile_metadata(self) -> Dict[str, ProfileMetadata]:
         """Load profile metadata from the metadata configuration file."""
@@ -99,8 +102,11 @@ def _save_profile_metadata(self) -> None:
         self._ensure_dirs()
         metadata_data = {name: meta.model_dump() for name, meta in self._profile_metadata.items()}
 
-        with open(self.metadata_path, "w", encoding="utf-8") as f:
-            json.dump(metadata_data, f, indent=2)
+        try:
+            with open(self.metadata_path, "w", encoding="utf-8") as f:
+                json.dump(metadata_data, f, indent=2)
+        except OSError as e:
+            logger.error(f"Error saving profile metadata to {self.metadata_path}: {e}")
 
     def add_server(self, server_config: ServerConfig, force: bool = False) -> bool:
         """Add a server to the global configuration.
@@ -359,7 +365,7 @@ def list_profile_metadata(self) -> Dict[str, ProfileMetadata]:
         """
         return self._profile_metadata.copy()
 
-    def get_complete_profile(self, name: str) -> Optional[Dict[str, any]]:
+    def get_complete_profile(self, name: str) -> Optional[Dict[str, Any]]:
         """Get complete profile information including metadata and servers.
 
         Args:

src/mcpm/utils/config.py

@@ -30,7 +30,7 @@ class ConfigManager:
     Client-specific configurations are managed by ClientConfigManager.
     """
 
-    def __init__(self, config_path=DEFAULT_CONFIG_FILE, auth_path=DEFAULT_AUTH_FILE):
+    def __init__(self, config_path: Path | str = DEFAULT_CONFIG_FILE, auth_path: Path | str = DEFAULT_AUTH_FILE):
         # Normalize paths to Path objects for consistent handling
         self.config_path = Path(config_path)
         self.auth_path = Path(auth_path)
@@ -78,13 +78,19 @@ def _default_config(self) -> Dict[str, Any]:
 
     def _save_config(self) -> None:
         """Save current configuration to file"""
-        with open(self.config_path, "w", encoding="utf-8") as f:
-            json.dump(self._config, f, indent=2)
+        try:
+            with open(self.config_path, "w", encoding="utf-8") as f:
+                json.dump(self._config, f, indent=2)
+        except OSError as e:
+            logger.error(f"Error saving config file: {self.config_path} - {e}")
 
     def _save_auth_config(self) -> None:
         """Save current auth configuration to file"""
-        with open(self.auth_path, "w", encoding="utf-8") as f:
-            json.dump(self._auth_config, f, indent=2)
+        try:
+            with open(self.auth_path, "w", encoding="utf-8") as f:
+                json.dump(self._auth_config, f, indent=2)
+        except OSError as e:
+            logger.error(f"Error saving auth file: {self.auth_path} - {e}")
 
     def get_config(self) -> Dict[str, Any]:
         """Get the complete configuration"""

tests/test_clients/test_qwen_cli.py

@@ -2,8 +2,8 @@
 Test for Qwen CLI manager
 """
 
-import os
 import tempfile
+from pathlib import Path
 from unittest.mock import patch
 
 from mcpm.clients.managers.qwen_cli import QwenCliManager
@@ -16,7 +16,7 @@ def test_qwen_cli_manager_initialization():
     assert manager.client_key == "qwen-cli"
     assert manager.display_name == "Qwen CLI"
     assert manager.download_url == "https://github.com/QwenLM/qwen-code"
-    assert manager.config_path == os.path.expanduser("~/.qwen/settings.json")
+    assert manager.config_path == str(Path.home() / ".qwen" / "settings.json")
 
     # Test with custom config path
     custom_path = "/tmp/custom_settings.json"
@@ -91,5 +91,5 @@ def test_qwen_cli_manager_get_client_info():
     info = manager.get_client_info()
     assert info["name"] == "Qwen CLI"
     assert info["download_url"] == "https://github.com/QwenLM/qwen-code"
-    assert info["config_file"] == os.path.expanduser("~/.qwen/settings.json")
+    assert info["config_file"] == str(Path.home() / ".qwen" / "settings.json")
     assert info["description"] == "Alibaba's Qwen CLI tool"