From cdb2d09f17f7d91e9b97c90cea9025d80500975f Mon Sep 17 00:00:00 2001 From: Chen Nie Date: Wed, 16 Apr 2025 21:26:11 +0800 Subject: [PATCH 1/2] fix: add should remove optional argument placeholder --- src/mcpm/commands/server_operations/add.py | 44 ++---------- src/mcpm/schemas/server_config.py | 19 +++-- tests/test_add.py | 84 ++++++++++++++++++++-- 3 files changed, 94 insertions(+), 53 deletions(-) diff --git a/src/mcpm/commands/server_operations/add.py b/src/mcpm/commands/server_operations/add.py index 63bccebe..3fc7c783 100644 --- a/src/mcpm/commands/server_operations/add.py +++ b/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) diff --git a/src/mcpm/schemas/server_config.py b/src/mcpm/schemas/server_config.py index 83f8178c..61e981f8 100644 --- a/src/mcpm/schemas/server_config.py +++ b/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): diff --git a/tests/test_add.py b/tests/test_add.py index b65ca6ed..8b6d6177 100644 --- a/tests/test_add.py +++ b/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 + } From b13f216b40f231b106cbd6435e00567813d7e537 Mon Sep 17 00:00:00 2001 From: Chen Nie Date: Wed, 16 Apr 2025 21:36:51 +0800 Subject: [PATCH 2/2] Update add.py --- src/mcpm/commands/server_operations/add.py | 48 +++++++++++++--------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/src/mcpm/commands/server_operations/add.py b/src/mcpm/commands/server_operations/add.py index 3fc7c783..9cfda80c 100644 --- a/src/mcpm/commands/server_operations/add.py +++ b/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,28 +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(): - if isinstance(value, str) and value.startswith("${") and value.endswith("}"): - # 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 - # 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) - # 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) + 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 @@ -425,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