fix: add should remove optional argument placeholder

src/mcpm/commands/server_operations/add.py

@@ -273,7 +273,6 @@ def add(server_name, force=False, alias=None, target: str | None = None):
         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
         progress.stop()
@@ -343,60 +342,15 @@ def add(server_name, force=False, alias=None, target: str | None = None):
             progress.start()
             progress.add_task(f"Configuring {server_name}...", total=None)
 
-        # Now process the replacement of environment variables
-        for key, value in env_vars.items():
-            processed_env[key] = value
-
-            if isinstance(value, str) and value.startswith("${") and value.endswith("}"):
-                # For required arguments, prompt the user if not in environment
-                env_value = os.environ.get(key, "")
-
-                if not env_value and key in required_args and key not in processed_variables:
-                    progress.stop()
-                    console.print(f"[yellow]Warning:[/] Required argument {key} is not set in environment")
-
-                    # Prompt for the value
-                    arg_info = required_args.get(key, {})
-                    description = arg_info.get("description", "")
-                    try:
-                        user_value = prompt_with_default(
-                            f"Enter value for {key} ({description})",
-                            default="",
-                            hide_input=_should_hide_input(key),
-                            required=True,
-                        )
-                        processed_env[key] = user_value
-                    except click.Abort:
-                        console.print("[yellow]Will store the reference to environment variable instead.[/]")
-                        processed_env[key] = value  # Store the reference as-is
-
-                    # Resume progress
-                    progress = Progress(
-                        SpinnerColumn(), TextColumn("[bold green]{task.description}[/]"), console=console
-                    )
-                    progress.start()
-                    progress.add_task(f"Configuring {server_name}...", total=None)
-                else:
-                    # Store reference to environment variable
-                    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:
-        # check if the argument contains a variable
-        matched = re.search(r"\$\{([^}]+)\}", arg)
-        if matched:
-            original, key = matched.group(0), matched.group(1)
-            if key not in processed_variables:
-                # Keep the original argument if variable not found
-                processed_args.append(arg)
-                continue
-            replaced_arg = arg.replace(original, processed_variables.get(key, arg))
-            processed_args.append(replaced_arg)
-        else:
-            processed_args.append(arg)
+        processed_args.append(_replace_variables(arg, processed_variables))
+
+    # process environment variables
+    processed_env = {}
+    for key, value in env_vars.items():
+        processed_env[key] = _replace_variables(value, processed_variables)
 
     # 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
@@ -457,3 +411,25 @@ def _should_hide_input(arg_name: str) -> bool:
         bool: True if input should be hidden, False otherwise
     """
     return "token" in arg_name.lower() or "key" in arg_name.lower() or "secret" in arg_name.lower()
+
+
+def _replace_variables(value: str, variables: dict) -> str:
+    """Replace ${VAR} patterns in a string with values from variables dict.
+
+    Args:
+        value: String that may contain ${VAR} patterns
+        variables: Dictionary of variable names to values
+
+    Returns:
+        String with all variables replaced (empty string for missing variables)
+    """
+    if not isinstance(value, str):
+        return value
+
+    # check if the value contains a variable
+    matched = re.search(r"\$\{([^}]+)\}", value)
+    if matched:
+        original, var_name = matched.group(0), matched.group(1)
+        # Use empty string as default when key not found
+        return value.replace(original, variables.get(var_name, ""))
+    return value

src/mcpm/schemas/server_config.py

@@ -34,8 +34,8 @@ def get_filtered_env_vars(self, env: Dict[str, str]) -> Dict[str, str]:
         # Use provided environment without falling back to os.environ
         environment = env
 
-        # Filter out empty environment variables
-        non_empty_env = {}
+        # Keep all environment variables, including empty strings
+        filtered_env = {}
         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.
@@ -44,14 +44,13 @@ def get_filtered_env_vars(self, env: Dict[str, str]) -> Dict[str, str]:
                     # Extract the variable name from ${VAR_NAME}
                     env_var_name = value[2:-1]
                     env_value = environment.get(env_var_name, "")
-                    # Only include if the variable has a value in the environment
-                    if env_value.strip() != "":
-                        non_empty_env[key] = value
-                # For regular values, only include if they're not empty
-                elif value.strip() != "":
-                    non_empty_env[key] = value
-
-        return non_empty_env
+                    # Include all values, even empty ones
+                    filtered_env[key] = env_value
+                else:
+                    # Include all values, even empty ones
+                    filtered_env[key] = value
+
+        return filtered_env
 
 
 class SSEServerConfig(BaseServerConfig):

tests/test_add.py

@@ -50,7 +50,7 @@ def test_add_server(windsurf_manager, monkeypatch):
 
 
 def test_add_server_with_missing_arg(windsurf_manager, monkeypatch):
-    """Test add server with a missing argument that should remain in the args"""
+    """Test add server with a missing argument that should be replaced with empty string"""
     monkeypatch.setattr(ClientRegistry, "get_active_client", Mock(return_value="windsurf"))
     monkeypatch.setattr(ClientRegistry, "get_active_client_manager", Mock(return_value=windsurf_manager))
     monkeypatch.setattr(ClientRegistry, "get_client_manager", Mock(return_value=windsurf_manager))
@@ -78,7 +78,7 @@ def test_add_server_with_missing_arg(windsurf_manager, monkeypatch):
                     "arguments": {
                         "fmt": {"type": "string", "description": "Output format", "required": True},
                         "API_KEY": {"type": "string", "description": "API key", "required": True},
-                        # Deliberately not including TZ to test the bug fix
+                        # Deliberately not including TZ to test empty string replacement
                     },
                 }
             }
@@ -104,10 +104,84 @@ def test_add_server_with_missing_arg(windsurf_manager, monkeypatch):
 
         assert result.exit_code == 0
 
-    # Check that the server was added with alias and the missing argument is preserved
+    # Check that the server was added with alias and the missing argument is replaced with empty string
     server = windsurf_manager.get_server("test-missing-arg")
     assert server is not None
     assert server.command == "npx"
-    # The ${TZ} argument should remain intact since it's not in the processed variables
-    assert server.args == ["-y", "@modelcontextprotocol/server-test", "--fmt", "json", "--timezone", "${TZ}"]
+    # The ${TZ} argument should be replaced with empty string since it's not in processed variables
+    assert server.args == ["-y", "@modelcontextprotocol/server-test", "--fmt", "json", "--timezone", ""]
     assert server.env["API_KEY"] == "test-api-key"
+
+
+def test_add_server_with_empty_args(windsurf_manager, monkeypatch):
+    """Test add server with missing arguments that should be replaced with empty strings"""
+    monkeypatch.setattr(ClientRegistry, "get_active_client", Mock(return_value="windsurf"))
+    monkeypatch.setattr(ClientRegistry, "get_active_client_manager", Mock(return_value=windsurf_manager))
+    monkeypatch.setattr(ClientRegistry, "get_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}",
+                                "--optional",
+                                "${OPTIONAL}",  # Optional arg not in arguments list
+                                "--api-key",
+                                "${API_KEY}",
+                            ],
+                            "env": {
+                                "API_KEY": "${API_KEY}",
+                                "OPTIONAL_ENV": "${OPTIONAL}",  # Optional env var
+                            },
+                        }
+                    },
+                    "arguments": {
+                        "fmt": {"type": "string", "description": "Output format", "required": True},
+                        "API_KEY": {"type": "string", "description": "API key", "required": True},
+                        # OPTIONAL is not listed in arguments
+                    },
+                }
+            }
+        ),
+    )
+
+    # Mock prompt responses for required arguments only
+    with (
+        patch("prompt_toolkit.PromptSession.prompt", side_effect=["json", "test-api-key"]),
+        patch("rich.progress.Progress.start"),
+        patch("rich.progress.Progress.stop"),
+        patch("rich.progress.Progress.add_task"),
+    ):
+        runner = CliRunner(mix_stderr=False)
+        result = runner.invoke(add, ["server-test", "--force", "--alias", "test-empty-args"])
+
+        assert result.exit_code == 0
+
+    # Check that the server was added and optional arguments are empty
+    server = windsurf_manager.get_server("test-empty-args")
+    assert server is not None
+    assert server.command == "npx"
+    # Optional arguments should be replaced with empty strings
+    assert server.args == [
+        "-y",
+        "@modelcontextprotocol/server-test",
+        "--fmt",
+        "json",
+        "--optional",
+        "",  # ${OPTIONAL} replaced with empty string
+        "--api-key",
+        "test-api-key",
+    ]
+    assert server.env == {
+        "API_KEY": "test-api-key",
+        "OPTIONAL_ENV": "",  # Optional env var should be empty string
+    }