fix: add should remove optional argument placeholder

Author: niechen

src/mcpm/commands/server_operations/add.py

@@ -345,40 +345,11 @@ def add(server_name, force=False, alias=None, target: str | None = 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)
+                # Extract the variable name from ${VAR_NAME}
+                env_var_name = value[2:-1]
+                # Use empty string as default when key not found
+                processed_env[key] = processed_variables.get(env_var_name, "")
             else:
                 processed_env[key] = value
 
@@ -389,11 +360,8 @@ def add(server_name, force=False, alias=None, target: str | None = None):
         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))
+            # Use empty string as default when key not found
+            replaced_arg = arg.replace(original, processed_variables.get(key, ""))
             processed_args.append(replaced_arg)
         else:
             processed_args.append(arg)

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
+    }