diff --git a/CHANGELOG.md b/CHANGELOG.md index ee300e2ea..373cf9f0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `apm update` against private Azure DevOps deps no longer fails on Windows with a misleading "az present but not logged in" diagnostic when the user IS signed in via `az login`. Root cause: Python's `subprocess.run(["az", ...])` -> `CreateProcessW` does not honor `PATHEXT` for non-`.exe` executables, so the Windows `az.cmd` wrapper could not be invoked even though `shutil.which("az")` resolved it. `AzureCliBearerProvider` now resolves the `az` binary via `shutil.which` once at construction and passes the absolute path to every subprocess call. As a defense-in-depth measure, the ADO `--update` preflight probe no longer strips `GIT_CONFIG_GLOBAL` / `GIT_CONFIG_NOSYSTEM` / `GIT_ASKPASS`, so Git Credential Manager can answer for Entra-cached ADO credentials whenever bearer acquisition is unavailable for any reason (sandbox, proxy, future PATH quirks). The actual clone path keeps its full gitconfig isolation. (#1430) - Root `.apm` hooks no longer duplicate after renaming the project directory or using git worktrees; Claude, Codex, Cursor, Gemini, and Windsurf hook configs stay idempotent across checkouts. The hook source-id is now derived from `apm.yml`'s `name` field instead of `install_path.name`, and `apm install` silently heals stale same-content entries from prior checkout basenames. Copilot is unaffected (its hooks live in per-file namespaces under `.github/hooks/`, not a shared merged config). (#1392, closes #1329) +- VS Code adapter now handles MCP registry v0.1 `runtimeArguments` entries with `variables` placeholders, fixing Docker-based MCP servers that require workspace mounts or other runtime parameters. (#1444, closes #1391) - `apm install --skill ` now persists the skill subset filter to `apm.yml` so subsequent `apm install` runs honour the selection. (#1442, closes #1395) - `apm audit` no longer false-reports non-skill primitives (instructions, agents, prompts, hooks, commands) as orphaned drift for targets with `auto_create=False` (windsurf, cursor, claude, codex, copilot, gemini, opencode). The drift replay engine now pre-creates target root directories in the ephemeral scratch space. (#1441, closes #1411) - Copilot harness auto-detection now recognises `.github/instructions/`, `.github/agents/`, `.github/prompts/`, and `.github/hooks/` as valid Copilot markers, and the "No harness detected" error message lists them. (#1440, closes #1435) diff --git a/src/apm_cli/adapters/client/vscode.py b/src/apm_cli/adapters/client/vscode.py index d129d47ac..6b21866b9 100644 --- a/src/apm_cli/adapters/client/vscode.py +++ b/src/apm_cli/adapters/client/vscode.py @@ -175,7 +175,9 @@ def configure_mcp_server( ) # Generate server configuration - server_config, input_vars = self._format_server_config(server_info) + server_config, input_vars = self._format_server_config( + server_info, runtime_vars=runtime_vars + ) if not server_config: if logger: @@ -228,11 +230,12 @@ def configure_mcp_server( print(f"Error configuring MCP server: {e}") return False - def _format_server_config(self, server_info): + def _format_server_config(self, server_info, runtime_vars=None): """Format server details into VSCode mcp.json compatible format. Args: server_info (dict): Server information from registry. + runtime_vars (dict, optional): Runtime variable substitutions. Returns: tuple: (server_config, input_vars) where: @@ -270,7 +273,9 @@ def _format_server_config(self, server_info): package = self._select_best_package(server_info["packages"]) runtime_hint = package.get("runtime_hint", "") if package else "" registry_name = self._infer_registry_name(package) if package else "" - pkg_args = self._extract_package_args(package) if package else [] + pkg_args = ( + self._extract_package_args(package, runtime_vars=runtime_vars) if package else [] + ) # Handle npm packages if runtime_hint == "npx" or registry_name == "npm": @@ -496,16 +501,25 @@ def _extract_input_variables(self, mapping, server_name): return result @staticmethod - def _extract_package_args(package): + def _extract_package_args(package, runtime_vars=None): """Extract positional arguments from a package entry. The MCP registry API uses ``package_arguments`` (with ``type``/``value`` pairs). Older or synthetic entries may use ``runtime_arguments`` - (with ``is_required``/``value_hint``). This method normalises both - formats into a flat list of argument strings. + (with ``is_required``/``value_hint``). v0.1 registry format uses + ``runtime_arguments`` where a ``variables`` dict is a *sibling* of + ``value_hint``; the ``value_hint`` string contains ``{var_name}`` + placeholders that are substituted at config-generation time. + This method normalises all formats into a flat list of argument strings. Args: package (dict): A single package entry. + runtime_vars (dict, optional): Runtime variable substitutions. + When a ``{var_name}`` placeholder is encountered in a v0.1 + ``value_hint``, the corresponding value from ``runtime_vars`` + is used. For VS Code, ``workspaceFolder`` defaults to the + native ``${workspaceFolder}`` interpolation token when not + supplied via ``runtime_vars``. Returns: list[str]: Ordered argument strings, may be empty. @@ -525,13 +539,32 @@ def _extract_package_args(package): if args: return args - # Fall back to runtime_arguments (legacy / synthetic format) + # Fall back to runtime_arguments (legacy / synthetic format and v0.1 variables shape) rt_args = package.get("runtime_arguments") or [] if rt_args: args = [] for arg in rt_args: if isinstance(arg, dict): - if arg.get("is_required", False) and arg.get("value_hint"): + if "variables" in arg and "value_hint" in arg: + # v0.1 format: variables is a sibling of value_hint; the + # value_hint string contains {var_name} placeholders. + value = arg["value_hint"] + for var_name in arg["variables"]: + if runtime_vars and var_name in runtime_vars: + replacement = runtime_vars[var_name] + elif var_name == "workspaceFolder": + # VS Code native variable substitution token + replacement = "${workspaceFolder}" + else: + replacement = f"${{{var_name}}}" + value = value.replace(f"{{{var_name}}}", replacement) + if value: + args.append(value) + elif arg.get("is_required", False) and arg.get("value_hint"): + # Legacy format: explicit is_required=True entries + args.append(arg["value_hint"]) + elif "value_hint" in arg and arg["value_hint"] and "is_required" not in arg: + # v0.1 format: plain value_hint entries without is_required args.append(arg["value_hint"]) if args: return args diff --git a/tests/unit/adapters/__init__.py b/tests/unit/adapters/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/unit/adapters/test_vscode_docker_runtime_args.py b/tests/unit/adapters/test_vscode_docker_runtime_args.py new file mode 100644 index 000000000..910804fda --- /dev/null +++ b/tests/unit/adapters/test_vscode_docker_runtime_args.py @@ -0,0 +1,282 @@ +"""Tests for VS Code adapter handling of Docker runtimeArguments with variables (MCP registry v0.1). + +Issue #1391: VS Code adapter drops Docker `runtimeArguments` with `variables`. +""" + +import unittest +from unittest.mock import patch + +from apm_cli.adapters.client.vscode import VSCodeClientAdapter + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +# v0.1 real shape: `variables` is a SIBLING of `value_hint`. +# The `value_hint` string contains {var_name} placeholders that get substituted. +V01_DOCKER_PACKAGE = { + "name": "ghcr.io/example/playwright-mcp:1.2.3", + "registry_name": "docker", + "runtime_hint": "docker", + "runtime_arguments": [ + {"value_hint": "run"}, + {"value_hint": "-i"}, + {"value_hint": "--rm"}, + {"value_hint": "-v"}, + { + "value_hint": "{workspaceFolder}:/workspace", + "variables": { + "workspaceFolder": { + "description": "Workspace folder path", + "is_required": True, + } + }, + }, + {"value_hint": "-w"}, + {"value_hint": "/workspace"}, + {"value_hint": "ghcr.io/example/playwright-mcp:1.2.3"}, + ], +} + +LEGACY_PACKAGE = { + "name": "some-server", + "registry_name": "npm", + "runtime_hint": "npx", + "runtime_arguments": [ + {"is_required": True, "value_hint": "server"}, + {"is_required": True, "value_hint": "start"}, + ], +} + +PACKAGE_ARGUMENTS_PACKAGE = { + "name": "@mcp/fetch", + "registry_name": "npm", + "runtime_hint": "npx", + "package_arguments": [ + {"type": "positional", "value": "--port"}, + {"type": "positional", "value": "3000"}, + ], +} + + +# --------------------------------------------------------------------------- +# Unit tests for _extract_package_args +# --------------------------------------------------------------------------- + + +class TestExtractPackageArgsVariables(unittest.TestCase): + """Tests for _extract_package_args with v0.1 variables shape.""" + + def test_variables_workspaceFolder_defaults_to_vscode_token(self): + """workspaceFolder placeholder in value_hint -> ${workspaceFolder}:/workspace.""" + result = VSCodeClientAdapter._extract_package_args(V01_DOCKER_PACKAGE) + self.assertIn("run", result) + self.assertIn("-i", result) + self.assertIn("--rm", result) + self.assertIn("-v", result) + self.assertIn("${workspaceFolder}:/workspace", result) + self.assertIn("-w", result) + self.assertIn("/workspace", result) + self.assertIn("ghcr.io/example/playwright-mcp:1.2.3", result) + + def test_variables_workspaceFolder_substituted_in_place(self): + """workspaceFolder placeholder produces a single combined mount arg.""" + result = VSCodeClientAdapter._extract_package_args(V01_DOCKER_PACKAGE) + mount_args = [a for a in result if "workspaceFolder" in a or ":/workspace" in a] + self.assertEqual(len(mount_args), 1) + self.assertEqual(mount_args[0], "${workspaceFolder}:/workspace") + + def test_variables_with_runtime_vars_substitution(self): + """Provided runtime_vars values are substituted into the value_hint string.""" + runtime_vars = {"workspaceFolder": "/home/user/project"} + result = VSCodeClientAdapter._extract_package_args( + V01_DOCKER_PACKAGE, runtime_vars=runtime_vars + ) + self.assertNotIn("${workspaceFolder}:/workspace", result) + self.assertIn("/home/user/project:/workspace", result) + + def test_variables_unknown_var_uses_placeholder(self): + """Unknown variable names get a ${varName} placeholder inside the value_hint.""" + pkg = { + "name": "some-image", + "registry_name": "docker", + "runtime_hint": "docker", + "runtime_arguments": [ + { + "value_hint": "{customVar}:/data", + "variables": { + "customVar": {"description": "Some custom var", "is_required": True} + }, + } + ], + } + result = VSCodeClientAdapter._extract_package_args(pkg) + self.assertEqual(result, ["${customVar}:/data"]) + + def test_variables_unknown_var_resolved_from_runtime_vars(self): + """Unknown variable is resolved from runtime_vars into the value_hint string.""" + pkg = { + "name": "some-image", + "registry_name": "docker", + "runtime_hint": "docker", + "runtime_arguments": [ + { + "value_hint": "{customVar}:/data", + "variables": {"customVar": {"description": "Custom", "is_required": True}}, + } + ], + } + result = VSCodeClientAdapter._extract_package_args( + pkg, runtime_vars={"customVar": "/custom/path"} + ) + self.assertEqual(result, ["/custom/path:/data"]) + + def test_legacy_is_required_still_works(self): + """Existing is_required/value_hint format still produces correct args (regression guard).""" + result = VSCodeClientAdapter._extract_package_args(LEGACY_PACKAGE) + self.assertEqual(result, ["server", "start"]) + + def test_package_arguments_still_take_priority(self): + """package_arguments format takes precedence over runtime_arguments.""" + pkg = { + "name": "something", + "package_arguments": [{"type": "positional", "value": "run"}], + "runtime_arguments": [{"is_required": True, "value_hint": "old"}], + } + result = VSCodeClientAdapter._extract_package_args(pkg) + self.assertEqual(result, ["run"]) + + def test_package_arguments_format(self): + """package_arguments values are extracted correctly.""" + result = VSCodeClientAdapter._extract_package_args(PACKAGE_ARGUMENTS_PACKAGE) + self.assertEqual(result, ["--port", "3000"]) + + def test_is_required_false_entries_skipped(self): + """Entries with is_required=False and value_hint are excluded (backward compat).""" + pkg = { + "name": "tool", + "runtime_arguments": [ + {"is_required": True, "value_hint": "run"}, + {"is_required": False, "value_hint": "--optional-flag"}, + ], + } + result = VSCodeClientAdapter._extract_package_args(pkg) + self.assertEqual(result, ["run"]) + + def test_empty_package(self): + self.assertEqual(VSCodeClientAdapter._extract_package_args({}), []) + + def test_none_package(self): + self.assertEqual(VSCodeClientAdapter._extract_package_args(None), []) + + +# --------------------------------------------------------------------------- +# Integration tests for _format_server_config with Docker + variables +# --------------------------------------------------------------------------- + + +def _make_adapter(): + """Create a VSCodeClientAdapter with mocked registry.""" + with ( + patch("apm_cli.adapters.client.vscode.SimpleRegistryClient"), + patch("apm_cli.adapters.client.vscode.RegistryIntegration"), + ): + adapter = VSCodeClientAdapter(project_root="/tmp/workspace") + return adapter + + +class TestFormatServerConfigDockerVariables(unittest.TestCase): + """End-to-end _format_server_config tests for Docker MCP servers with variables.""" + + def setUp(self): + self.adapter = _make_adapter() + self.server_info = { + "id": "playwright-mcp", + "name": "playwright-mcp", + "packages": [V01_DOCKER_PACKAGE], + } + + def test_docker_server_config_uses_workspaceFolder_token(self): + """Docker server config substitutes {workspaceFolder} -> ${workspaceFolder}:/workspace.""" + config, _input_vars = self.adapter._format_server_config(self.server_info) + self.assertEqual(config.get("command"), "docker") + self.assertIn("${workspaceFolder}:/workspace", config.get("args", [])) + + def test_docker_server_config_with_runtime_vars(self): + """Docker server config substitutes workspaceFolder from runtime_vars.""" + runtime_vars = {"workspaceFolder": "/workspace/myproject"} + config, _ = self.adapter._format_server_config(self.server_info, runtime_vars=runtime_vars) + self.assertEqual(config.get("command"), "docker") + args = config.get("args", []) + self.assertIn("/workspace/myproject:/workspace", args) + self.assertNotIn("${workspaceFolder}:/workspace", args) + + def test_docker_server_config_arg_order(self): + """Docker args preserve the order defined in runtime_arguments.""" + config, _ = self.adapter._format_server_config(self.server_info) + args = config.get("args", []) + run_idx = args.index("run") + rm_idx = args.index("--rm") + v_idx = args.index("-v") + self.assertLess(run_idx, rm_idx) + self.assertLess(rm_idx, v_idx) + + def test_format_server_config_threads_runtime_vars(self): + """Verify runtime_vars passed to _format_server_config reaches _extract_package_args.""" + runtime_vars = {"workspaceFolder": "/some/path"} + with patch.object( + VSCodeClientAdapter, + "_extract_package_args", + wraps=VSCodeClientAdapter._extract_package_args, + ) as mock_extract: + self.adapter._format_server_config(self.server_info, runtime_vars=runtime_vars) + mock_extract.assert_called_once() + _, kwargs = mock_extract.call_args + self.assertEqual(kwargs.get("runtime_vars"), runtime_vars) + + +# --------------------------------------------------------------------------- +# configure_mcp_server passes runtime_vars through to _format_server_config +# --------------------------------------------------------------------------- + + +class TestConfigureMcpServerPassesRuntimeVars(unittest.TestCase): + """Verify configure_mcp_server threads runtime_vars into _format_server_config.""" + + def setUp(self): + self.adapter = _make_adapter() + self.server_info = { + "id": "playwright-mcp", + "name": "playwright-mcp", + "packages": [V01_DOCKER_PACKAGE], + } + self.adapter.registry_client.find_server_by_reference.return_value = self.server_info + + def test_runtime_vars_forwarded_to_format_server_config(self): + runtime_vars = {"workspaceFolder": "/repo"} + + with ( + patch.object( + self.adapter, + "_format_server_config", + wraps=self.adapter._format_server_config, + ) as mock_fmt, + patch.object(self.adapter, "update_config", return_value=True), + patch.object( + self.adapter, + "get_current_config", + return_value={"servers": {}, "inputs": []}, + ), + ): + self.adapter.configure_mcp_server( + "playwright-mcp", + runtime_vars=runtime_vars, + ) + mock_fmt.assert_called_once_with( + self.server_info, + runtime_vars=runtime_vars, + ) + + +if __name__ == "__main__": + unittest.main()