fix: args placeholder is not replaced (#17)

Author: JoJoJoJoJoJoJo

pyproject.toml

@@ -49,3 +49,9 @@ fix = true
 [tool.ruff.lint]
 select = ["F", "E", "W", "I"]
 fixable = ["I", "F401"]
+
+[dependency-groups]
+dev = [
+    "ipython>=8.34.0",
+    "pytest>=8.3.5",
+]

src/mcpm/clients/base.py

@@ -306,7 +306,7 @@ def from_client_format(cls, server_name: str, client_config: Dict[str, Any]) ->
 
         # Add environment variables if present
         if "env" in client_config:
-            server_data["env_vars"] = client_config["env"]
+            server_data["env"] = client_config["env"]
 
         return ServerConfig.from_dict(server_data)
 

src/mcpm/commands/add.py

@@ -4,6 +4,7 @@
 
 import json
 import os
+import re
 
 import click
 from rich.console import Console
@@ -157,7 +158,6 @@ def add(server_name, force=False, alias=None):
             install_args = selected_method.get("args", install_args)
             package_name = selected_method.get("package", package_name)
             env_vars = selected_method.get("env", env_vars)
-            required_args = server_metadata.get("required_args", required_args)
 
         console.print(f"\n[green]Using [bold]{installation_method}[/] installation method[/]")
 
@@ -175,7 +175,9 @@ def add(server_name, force=False, alias=None):
         # Get all available arguments from the server metadata
         all_arguments = server_metadata.get("arguments", {})
 
-        # Process environment variables to store in config
+        required_args = {}
+        # Process variables to store in config
+        processed_variables = {}
         processed_env = {}
 
         # First, prompt for all defined arguments even if they're not in env_vars
@@ -196,6 +198,7 @@ def add(server_name, force=False, alias=None):
 
                 # Add required indicator
                 if is_required:
+                    required_args[arg_name] = arg_info
                     prompt_text += " (required)"
                 else:
                     prompt_text += " (optional, press Enter to skip)"
@@ -216,13 +219,12 @@ def add(server_name, force=False, alias=None):
                         )
                         if user_value != env_value:
                             # User provided a different value
-                            processed_env[arg_name] = user_value
+                            processed_variables[arg_name] = user_value
                         else:
-                            # User kept the environment value
-                            processed_env[arg_name] = f"${{{arg_name}}}"
+                            # User use environment value
+                            processed_variables[arg_name] = env_value
                     except click.Abort:
-                        # Keep environment reference on abort
-                        processed_env[arg_name] = f"${{{arg_name}}}"
+                        pass
                 else:
                     # No environment value
                     try:
@@ -234,7 +236,7 @@ def add(server_name, force=False, alias=None):
                                 or "key" in arg_name.lower()
                                 or "secret" in arg_name.lower(),
                             )
-                            processed_env[arg_name] = user_value
+                            processed_variables[arg_name] = user_value
                         else:
                             # Optional argument can be skipped
                             user_value = click.prompt(
@@ -247,43 +249,36 @@ def add(server_name, force=False, alias=None):
                             )
                             # Only add non-empty values to the environment
                             if user_value and user_value.strip():
-                                processed_env[arg_name] = user_value
+                                processed_variables[arg_name] = user_value
                             # Explicitly don't add anything if the user leaves it blank
                     except click.Abort:
                         if is_required:
                             console.print(f"[yellow]Warning: Required argument {arg_name} not provided.[/]")
-                            # Store as environment reference even if missing
-                            processed_env[arg_name] = f"${{{arg_name}}}"
 
             # Resume progress display
             progress = Progress(SpinnerColumn(), TextColumn("[bold green]{task.description}[/]"), console=console)
             progress.start()
             progress.add_task(f"Configuring {server_name}...", total=None)
 
-        # Now process any remaining environment variables from the installation method
+        # Now process the replacement of environment variables
         for key, value in env_vars.items():
-            # Skip if we already processed this key
-            if key in processed_env:
-                continue
+            processed_env[key] = value
 
             if isinstance(value, str) and value.startswith("${") and value.endswith("}"):
-                env_var_name = value[2:-1]  # Extract variable name from ${NAME}
-                is_required = env_var_name in required_args
-
                 # For required arguments, prompt the user if not in environment
-                env_value = os.environ.get(env_var_name, "")
+                env_value = os.environ.get(key, "")
 
-                if not env_value and is_required:
+                if not env_value and key in required_args and key not in processed_variables:
                     progress.stop()
-                    console.print(f"[yellow]Warning:[/] Required argument {env_var_name} is not set in environment")
+                    console.print(f"[yellow]Warning:[/] Required argument {key} is not set in environment")
 
                     # Prompt for the value
-                    arg_info = required_args.get(env_var_name, {})
+                    arg_info = required_args.get(key, {})
                     description = arg_info.get("description", "")
                     try:
                         user_value = click.prompt(
-                            f"Enter value for {env_var_name} ({description})",
-                            hide_input="token" in env_var_name.lower() or "key" in env_var_name.lower(),
+                            f"Enter value for {key} ({description})",
+                            hide_input="token" in key.lower() or "key" in key.lower(),
                         )
                         processed_env[key] = user_value
                     except click.Abort:
@@ -298,19 +293,33 @@ def add(server_name, force=False, alias=None):
                     progress.add_task(f"Configuring {server_name}...", total=None)
                 else:
                     # Store reference to environment variable
-                    processed_env[key] = value
+                    processed_env[key] = processed_variables.get(key, env_value)
             else:
                 processed_env[key] = value
 
+    # replace arguments with values
+    processed_args = []
+    for arg in install_args:
+        matched = re.match(r"\$\{(.*?)\}", arg)
+        if matched:
+            original, key = matched.group(0), matched.group(1)
+            if key not in processed_variables:
+                # not required, remove the argument from args
+                continue
+            replaced_arg = arg.replace(original, processed_variables.get(key, arg))
+            processed_args.append(replaced_arg)
+        else:
+            processed_args.append(arg)
+
     # Get actual MCP execution command, args, and env from the selected installation method
     # This ensures we use the actual server command information instead of placeholders
     if selected_method:
         mcp_command = selected_method.get("command", install_command)
-        mcp_args = selected_method.get("args", install_args)
+        mcp_args = processed_args
         # Env vars are already processed above
     else:
         mcp_command = install_command
-        mcp_args = install_args
+        mcp_args = processed_args
 
     # Create server configuration using ServerConfig
     server_config = ServerConfig(
@@ -319,7 +328,7 @@ def add(server_name, force=False, alias=None):
         description=description,
         command=mcp_command,  # Use the actual MCP server command
         args=mcp_args,  # Use the actual MCP server arguments
-        env_vars=processed_env,
+        env=processed_env,
         # Use the simplified installation method
         installation=installation_method,
     )

src/mcpm/utils/server_config.py

@@ -1,7 +1,6 @@
 """Server configuration utilities for MCPM"""
 
-import os
-from typing import Dict, List, Optional, ClassVar
+from typing import ClassVar, Dict, List, Optional
 
 from pydantic import BaseModel, Field, model_validator
 
@@ -20,7 +19,7 @@ class ServerConfig(BaseModel):
     args: List[str]
 
     # Optional fields
-    env_vars: Dict[str, str] = Field(default_factory=dict)
+    env: Dict[str, str] = Field(default_factory=dict)
     display_name: Optional[str] = None
     description: str = ""
     installation: Optional[str] = None
@@ -81,15 +80,15 @@ def get_filtered_env_vars(self, env: Dict[str, str]) -> Dict[str, str]:
         Returns:
             Dictionary of non-empty environment variables
         """
-        if not self.env_vars:
+        if not self.env:
             return {}
 
         # Use provided environment without falling back to os.environ
         environment = env
 
         # Filter out empty environment variables
         non_empty_env = {}
-        for key, value in self.env_vars.items():
+        for key, value in self.env.items():
             # For environment variable references like ${VAR_NAME}, check if the variable exists
             # and has a non-empty value. If it doesn't exist or is empty, exclude it.
             if value is not None and isinstance(value, str):

tests/conftest.py

@@ -2,8 +2,68 @@
 Pytest configuration for MCPM tests
 """
 
+import json
+import os
 import sys
+import tempfile
 from pathlib import Path
 
+import pytest
+
 # Add the src directory to the path for all tests
 sys.path.insert(0, str(Path(__file__).parent.parent))
+
+from mcpm.clients.managers.windsurf import WindsurfManager
+from mcpm.utils.config import ConfigManager
+
+
+@pytest.fixture
+def temp_config_file():
+    """Create a temporary Windsurf config file for testing"""
+    with tempfile.NamedTemporaryFile(delete=False, suffix=".json") as f:
+        # Create a basic config with a test server
+        config = {
+            "mcpServers": {
+                "test-server": {
+                    "command": "npx",
+                    "args": ["-y", "@modelcontextprotocol/server-test"],
+                    "version": "1.0.0",
+                    "path": "/path/to/server",
+                    "display_name": "Test Server",
+                }
+            }
+        }
+        f.write(json.dumps(config).encode("utf-8"))
+        temp_path = f.name
+
+    yield temp_path
+    # Clean up
+    os.unlink(temp_path)
+
+
+@pytest.fixture
+def config_manager():
+    """Create a ClientConfigManager with a temp config for testing"""
+    with tempfile.TemporaryDirectory() as temp_dir:
+        config_path = os.path.join(temp_dir, "config.json")
+        # Create ConfigManager with the temp path
+        config_mgr = ConfigManager(config_path=config_path)
+        # Create ClientConfigManager that will use this ConfigManager internally
+        from mcpm.clients.client_config import ClientConfigManager
+
+        client_mgr = ClientConfigManager()
+        # Override its internal config_manager with our temp one
+        client_mgr.config_manager = config_mgr
+        yield client_mgr
+
+
+@pytest.fixture
+def windsurf_manager(temp_config_file):
+    """Create a WindsurfManager instance using the temp config file"""
+    return WindsurfManager(config_path=temp_config_file)
+
+
+@pytest.fixture
+def empty_windsurf_manager(empty_config_file):
+    """Create a WindsurfManager instance with an empty config"""
+    return WindsurfManager(config_path=empty_config_file)

tests/test_add.py

@@ -0,0 +1,45 @@
+from unittest.mock import Mock
+
+from click.testing import CliRunner
+
+from mcpm.clients.client_registry import ClientRegistry
+from mcpm.commands.add import add
+from mcpm.utils.repository import RepositoryManager
+
+
+def test_add_server(windsurf_manager, monkeypatch):
+    """Test add server"""
+    monkeypatch.setattr(ClientRegistry, "get_active_client", Mock(return_value="windsurf"))
+    monkeypatch.setattr(ClientRegistry, "get_active_client_manager", Mock(return_value=windsurf_manager))
+    monkeypatch.setattr(
+        RepositoryManager,
+        "_fetch_servers",
+        Mock(
+            return_value={
+                "server-test": {
+                    "installations": {
+                        "npm": {
+                            "type": "npm",
+                            "command": "npx",
+                            "args": ["-y", "@modelcontextprotocol/server-test", "--fmt", "${fmt}"],
+                            "env": {"API_KEY": "${API_KEY}"},
+                        }
+                    },
+                    "arguments": {
+                        "fmt": {"type": "string", "description": "Output format", "required": True},
+                        "API_KEY": {"type": "string", "description": "API key", "required": True},
+                    },
+                }
+            }
+        ),
+    )
+    runner = CliRunner()
+    result = runner.invoke(add, ["server-test", "--force", "--alias", "test"], input="\njson\n\ntest-api-key\n")
+    assert result.exit_code == 0
+
+    # Check that the server was added
+    server = windsurf_manager.get_server("test")
+    assert server is not None
+    assert server.command == "npx"
+    assert server.args == ["-y", "@modelcontextprotocol/server-test", "--fmt", "json"]
+    assert server.env["API_KEY"] == "test-api-key"