From c85d94547c4061bc2bda343afc2d5a378d23a96c Mon Sep 17 00:00:00 2001 From: Sergio Sisternes Date: Sat, 23 May 2026 13:20:07 +0100 Subject: [PATCH 1/3] fix: handle MCP v0.1 runtimeArguments.variables in copilot and codex adapters The _process_arguments method in copilot.py and codex.py only handled args with type=positional, type=named, or plain strings. Args using the v0.1 registry format (value_hint + variables dict, no type key) were silently dropped. This meant Docker mount arguments with {workspaceFolder} placeholders produced empty arg lists in Copilot, Codex, Gemini, Cursor, and Claude adapters. Add an elif branch for v0.1 value_hint args (with optional variables dict) that performs {var_name} placeholder substitution, matching the existing logic in VSCodeClientAdapter._extract_package_args. Gemini, Cursor, and Claude inherit from CopilotClientAdapter so they get the fix automatically. Fixes #1452 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/adapters/client/codex.py | 15 ++ src/apm_cli/adapters/client/copilot.py | 15 ++ .../test_v01_variables_process_arguments.py | 185 ++++++++++++++++++ 3 files changed, 215 insertions(+) create mode 100644 tests/unit/adapters/test_v01_variables_process_arguments.py diff --git a/src/apm_cli/adapters/client/codex.py b/src/apm_cli/adapters/client/codex.py index dc3f5293b..e057448da 100644 --- a/src/apm_cli/adapters/client/codex.py +++ b/src/apm_cli/adapters/client/codex.py @@ -422,6 +422,21 @@ def _process_arguments( # pylint: disable=duplicate-code # structural similari str(additional_value), resolved_env, runtime_vars ) processed.append(processed_value) + elif not arg_type and "value_hint" in arg: + # v0.1 registry format: value_hint with optional variables dict + value = arg["value_hint"] + if "variables" in arg: + for var_name in arg["variables"]: + if runtime_vars and var_name in runtime_vars: + replacement = runtime_vars[var_name] + else: + replacement = f"${{{var_name}}}" + value = value.replace(f"{{{var_name}}}", replacement) + if value: + processed_value = self._resolve_variable_placeholders( + str(value), resolved_env, runtime_vars + ) + processed.append(processed_value) elif isinstance(arg, str): # Already a string, use as-is but resolve variable placeholders processed_value = self._resolve_variable_placeholders( diff --git a/src/apm_cli/adapters/client/copilot.py b/src/apm_cli/adapters/client/copilot.py index 42a3ccb73..972c8f256 100644 --- a/src/apm_cli/adapters/client/copilot.py +++ b/src/apm_cli/adapters/client/copilot.py @@ -982,6 +982,21 @@ def _process_arguments(self, arguments, resolved_env=None, runtime_vars=None): str(value), resolved_env, runtime_vars ) processed.append(processed_value) + elif not arg_type and "value_hint" in arg: + # v0.1 registry format: value_hint with optional variables dict + value = arg["value_hint"] + if "variables" in arg: + for var_name in arg["variables"]: + if runtime_vars and var_name in runtime_vars: + replacement = runtime_vars[var_name] + else: + replacement = f"${{{var_name}}}" + value = value.replace(f"{{{var_name}}}", replacement) + if value: + processed_value = self._resolve_variable_placeholders( + str(value), resolved_env, runtime_vars + ) + processed.append(processed_value) elif isinstance(arg, str): # Already a string, use as-is but resolve variable placeholders processed_value = self._resolve_variable_placeholders( diff --git a/tests/unit/adapters/test_v01_variables_process_arguments.py b/tests/unit/adapters/test_v01_variables_process_arguments.py new file mode 100644 index 000000000..2e6937cf4 --- /dev/null +++ b/tests/unit/adapters/test_v01_variables_process_arguments.py @@ -0,0 +1,185 @@ +"""Regression tests for MCP v0.1 runtimeArguments.variables handling in non-vscode adapters. + +Issue #1452: copilot and codex adapters' _process_arguments do not handle +the v0.1 format where a ``variables`` dict is a sibling of ``value_hint`` +(no ``type`` key). This causes Docker mount args with {workspaceFolder} +placeholders to be silently dropped. + +gemini, cursor, and claude inherit from CopilotClientAdapter, so fixing +copilot fixes all three. +""" + +from __future__ import annotations + +import unittest +from pathlib import Path +from unittest.mock import patch + +from apm_cli.adapters.client.codex import CodexClientAdapter +from apm_cli.adapters.client.copilot import CopilotClientAdapter + +# --------------------------------------------------------------------------- +# Shared v0.1 Docker fixture (same shape as real registry data) +# --------------------------------------------------------------------------- + +V01_DOCKER_RUNTIME_ARGS = [ + {"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"}, +] + + +# --------------------------------------------------------------------------- +# Adapter factories +# --------------------------------------------------------------------------- + + +def _make_copilot(**kwargs) -> CopilotClientAdapter: + with ( + patch("apm_cli.adapters.client.copilot.SimpleRegistryClient"), + patch("apm_cli.adapters.client.copilot.RegistryIntegration"), + ): + return CopilotClientAdapter(**kwargs) + + +def _make_codex(tmp_path: Path | None = None) -> CodexClientAdapter: + with ( + patch("apm_cli.adapters.client.codex.SimpleRegistryClient"), + patch("apm_cli.adapters.client.codex.RegistryIntegration"), + ): + return CodexClientAdapter(project_root=tmp_path) + + +# --------------------------------------------------------------------------- +# Copilot adapter +# --------------------------------------------------------------------------- + + +class TestCopilotProcessArgumentsV01Variables(unittest.TestCase): + """_process_arguments must handle v0.1 value_hint + variables args.""" + + def _adapter(self) -> CopilotClientAdapter: + return _make_copilot() + + def test_v01_plain_value_hint_args_extracted(self): + """Plain value_hint args (no variables, no type) are extracted.""" + adapter = self._adapter() + result = adapter._process_arguments( + [{"value_hint": "run"}, {"value_hint": "--rm"}], + resolved_env={}, + runtime_vars={}, + ) + self.assertEqual(result, ["run", "--rm"]) + + def test_v01_variables_placeholder_resolved_from_runtime_vars(self): + """v0.1 arg with variables dict resolves {workspaceFolder} from runtime_vars.""" + adapter = self._adapter() + result = adapter._process_arguments( + V01_DOCKER_RUNTIME_ARGS, + resolved_env={}, + runtime_vars={"workspaceFolder": "/home/user/project"}, + ) + self.assertIn("/home/user/project:/workspace", result) + + def test_v01_variables_unknown_var_gets_placeholder(self): + """Unknown variable gets a ${varName} placeholder (same as vscode).""" + adapter = self._adapter() + args = [ + { + "value_hint": "{customVar}:/data", + "variables": {"customVar": {"description": "Custom path", "is_required": True}}, + } + ] + result = adapter._process_arguments(args, resolved_env={}, runtime_vars={}) + self.assertEqual(result, ["${customVar}:/data"]) + + def test_v01_full_docker_arg_set_preserved(self): + """All 8 args from the v0.1 Docker fixture are present.""" + adapter = self._adapter() + result = adapter._process_arguments( + V01_DOCKER_RUNTIME_ARGS, + resolved_env={}, + runtime_vars={"workspaceFolder": "/ws"}, + ) + self.assertEqual(len(result), 8) + self.assertEqual(result[0], "run") + self.assertEqual(result[1], "-i") + self.assertEqual(result[2], "--rm") + self.assertEqual(result[3], "-v") + self.assertEqual(result[4], "/ws:/workspace") + self.assertEqual(result[5], "-w") + self.assertEqual(result[6], "/workspace") + self.assertEqual(result[7], "ghcr.io/example/playwright-mcp:1.2.3") + + +# --------------------------------------------------------------------------- +# Codex adapter +# --------------------------------------------------------------------------- + + +class TestCodexProcessArgumentsV01Variables(unittest.TestCase): + """_process_arguments must handle v0.1 value_hint + variables args.""" + + def _adapter(self) -> CodexClientAdapter: + return _make_codex() + + def test_v01_plain_value_hint_args_extracted(self): + """Plain value_hint args (no variables, no type) are extracted.""" + adapter = self._adapter() + result = adapter._process_arguments( + [{"value_hint": "run"}, {"value_hint": "--rm"}], + resolved_env={}, + runtime_vars={}, + ) + self.assertEqual(result, ["run", "--rm"]) + + def test_v01_variables_placeholder_resolved_from_runtime_vars(self): + """v0.1 arg with variables dict resolves {workspaceFolder} from runtime_vars.""" + adapter = self._adapter() + result = adapter._process_arguments( + V01_DOCKER_RUNTIME_ARGS, + resolved_env={}, + runtime_vars={"workspaceFolder": "/home/user/project"}, + ) + self.assertIn("/home/user/project:/workspace", result) + + def test_v01_variables_unknown_var_gets_placeholder(self): + """Unknown variable gets a ${varName} placeholder (same as vscode).""" + adapter = self._adapter() + args = [ + { + "value_hint": "{customVar}:/data", + "variables": {"customVar": {"description": "Custom path", "is_required": True}}, + } + ] + result = adapter._process_arguments(args, resolved_env={}, runtime_vars={}) + self.assertEqual(result, ["${customVar}:/data"]) + + def test_v01_full_docker_arg_set_preserved(self): + """All 8 args from the v0.1 Docker fixture are present.""" + adapter = self._adapter() + result = adapter._process_arguments( + V01_DOCKER_RUNTIME_ARGS, + resolved_env={}, + runtime_vars={"workspaceFolder": "/ws"}, + ) + self.assertEqual(len(result), 8) + self.assertEqual(result[0], "run") + self.assertEqual(result[4], "/ws:/workspace") + + +if __name__ == "__main__": + unittest.main() From af4bee1932a6793cd086cbb3cc39bdba88a090ea Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Sat, 23 May 2026 15:24:54 +0200 Subject: [PATCH 2/3] docs(changelog): add Unreleased entry crediting @sergio-sisternes-epam for #1461 Per .github/instructions/changelog.instructions.md and the apm-review-panel recommendation. Names all 7 adapters that inherit the fix so users grepping for MCP v0.1 find both halves of the symmetric repair (vscode in 0.14.2, copilot/codex + inherited adapters here). Co-authored-by: sergio-sisternes-epam Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 32d7f56ed..1110f2352 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Copilot, Codex, Cursor, Claude, Windsurf, OpenCode, and Gemini adapters handle MCP v0.1 `runtimeArguments`/`packageArguments` with `variables` (no `type` key), matching the VS Code fix from #1444. (#1461, closes #1452, thanks @sergio-sisternes-epam) + ## [0.14.2] - 2026-05-22 ### Added From df8e8d1f9f67dd501176960731f3f0edcd1d240e Mon Sep 17 00:00:00 2001 From: Sergio Sisternes Date: Sat, 23 May 2026 14:57:10 +0100 Subject: [PATCH 3/3] fix: address Copilot review feedback on PR #1461 - Extract v0.1 value_hint processing into shared helper src/apm_cli/adapters/client/_mcp_runtime_args.py to eliminate the R0801 duplicate-code violation between copilot.py and codex.py - Add is_required guard in process_v01_value_hint_arg(): entries with is_required: False are legacy optional hints and must be skipped; absent is_required defaults to True (required) - Add test cases for legacy {is_required: False, value_hint: ...} shape in both Copilot and Codex test classes to prevent regression Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../adapters/client/_mcp_runtime_args.py | 61 +++++++++++++++++++ src/apm_cli/adapters/client/codex.py | 15 ++--- src/apm_cli/adapters/client/copilot.py | 15 ++--- .../test_v01_variables_process_arguments.py | 54 ++++++++++++++++ 4 files changed, 125 insertions(+), 20 deletions(-) create mode 100644 src/apm_cli/adapters/client/_mcp_runtime_args.py diff --git a/src/apm_cli/adapters/client/_mcp_runtime_args.py b/src/apm_cli/adapters/client/_mcp_runtime_args.py new file mode 100644 index 000000000..b80efb800 --- /dev/null +++ b/src/apm_cli/adapters/client/_mcp_runtime_args.py @@ -0,0 +1,61 @@ +"""Shared helper for MCP v0.1 runtimeArguments.variables handling. + +Extracted to avoid R0801 duplicate-code violations across adapter modules +that each implement a ``_process_arguments`` method but share identical v0.1 +``value_hint`` processing logic (copilot and codex). + +Usage in an adapter's ``_process_arguments`` loop:: + + from ._mcp_runtime_args import process_v01_value_hint_arg + + elif not arg_type and "value_hint" in arg: + value = process_v01_value_hint_arg(arg, runtime_vars) + if value: + processed.append(self._resolve_variable_placeholders( + value, resolved_env, runtime_vars + )) +""" + +from __future__ import annotations + + +def process_v01_value_hint_arg(arg: dict, runtime_vars: dict | None) -> str | None: + """Process a single v0.1-format runtimeArguments entry. + + A v0.1 entry has a ``value_hint`` key but no ``type`` key. An optional + ``variables`` dict maps placeholder names to their metadata. + + The ``is_required`` field on the *arg itself* controls whether the entry + participates in argument building. Entries with ``is_required: False`` + are optional hints that must be skipped -- VS Code's extractor only + includes entries with ``is_required: True``, so including them would + produce extra, unintended CLI args. When ``is_required`` is absent the + entry is treated as required (default True). + + Args: + arg: The argument dict to process. Must contain ``value_hint``. + runtime_vars: Resolved APM template variables (may be ``None`` or + empty). + + Returns: + The processed value string after ``{var_name}`` placeholder + substitution, or ``None`` if the entry should be skipped (optional + hint or empty value). + """ + # Skip optional legacy hints that are not required. + if not arg.get("is_required", True): + return None + + value: str = arg.get("value_hint", "") + if not value: + return None + + if "variables" in arg: + for var_name in arg["variables"]: + if runtime_vars and var_name in runtime_vars: + replacement = runtime_vars[var_name] + else: + replacement = f"${{{var_name}}}" + value = value.replace(f"{{{var_name}}}", replacement) + + return value or None diff --git a/src/apm_cli/adapters/client/codex.py b/src/apm_cli/adapters/client/codex.py index e057448da..cabe500be 100644 --- a/src/apm_cli/adapters/client/codex.py +++ b/src/apm_cli/adapters/client/codex.py @@ -10,6 +10,7 @@ from ...registry.client import SimpleRegistryClient from ...registry.integration import RegistryIntegration from ...utils.console import _rich_success, _rich_warning +from ._mcp_runtime_args import process_v01_value_hint_arg from .base import MCPClientAdapter _log = logging.getLogger(__name__) @@ -423,18 +424,12 @@ def _process_arguments( # pylint: disable=duplicate-code # structural similari ) processed.append(processed_value) elif not arg_type and "value_hint" in arg: - # v0.1 registry format: value_hint with optional variables dict - value = arg["value_hint"] - if "variables" in arg: - for var_name in arg["variables"]: - if runtime_vars and var_name in runtime_vars: - replacement = runtime_vars[var_name] - else: - replacement = f"${{{var_name}}}" - value = value.replace(f"{{{var_name}}}", replacement) + # v0.1 registry format: shared helper handles is_required + # guard and {var_name} placeholder substitution. + value = process_v01_value_hint_arg(arg, runtime_vars) if value: processed_value = self._resolve_variable_placeholders( - str(value), resolved_env, runtime_vars + value, resolved_env, runtime_vars ) processed.append(processed_value) elif isinstance(arg, str): diff --git a/src/apm_cli/adapters/client/copilot.py b/src/apm_cli/adapters/client/copilot.py index 972c8f256..9d683a0c4 100644 --- a/src/apm_cli/adapters/client/copilot.py +++ b/src/apm_cli/adapters/client/copilot.py @@ -18,6 +18,7 @@ from ...registry.integration import RegistryIntegration from ...utils.console import _rich_warning from ...utils.github_host import is_github_hostname +from ._mcp_runtime_args import process_v01_value_hint_arg from .base import ( _ENV_PLACEHOLDER_RE, _ENV_VAR_RE, @@ -983,18 +984,12 @@ def _process_arguments(self, arguments, resolved_env=None, runtime_vars=None): ) processed.append(processed_value) elif not arg_type and "value_hint" in arg: - # v0.1 registry format: value_hint with optional variables dict - value = arg["value_hint"] - if "variables" in arg: - for var_name in arg["variables"]: - if runtime_vars and var_name in runtime_vars: - replacement = runtime_vars[var_name] - else: - replacement = f"${{{var_name}}}" - value = value.replace(f"{{{var_name}}}", replacement) + # v0.1 registry format: shared helper handles is_required + # guard and {var_name} placeholder substitution. + value = process_v01_value_hint_arg(arg, runtime_vars) if value: processed_value = self._resolve_variable_placeholders( - str(value), resolved_env, runtime_vars + value, resolved_env, runtime_vars ) processed.append(processed_value) elif isinstance(arg, str): diff --git a/tests/unit/adapters/test_v01_variables_process_arguments.py b/tests/unit/adapters/test_v01_variables_process_arguments.py index 2e6937cf4..19daea189 100644 --- a/tests/unit/adapters/test_v01_variables_process_arguments.py +++ b/tests/unit/adapters/test_v01_variables_process_arguments.py @@ -124,6 +124,33 @@ def test_v01_full_docker_arg_set_preserved(self): self.assertEqual(result[6], "/workspace") self.assertEqual(result[7], "ghcr.io/example/playwright-mcp:1.2.3") + def test_legacy_optional_hint_skipped(self): + """Legacy entries with is_required: False must not be appended.""" + adapter = self._adapter() + args = [ + {"value_hint": "--optional-flag", "is_required": False}, + {"value_hint": "required-arg"}, + ] + result = adapter._process_arguments(args, resolved_env={}, runtime_vars={}) + self.assertEqual(result, ["required-arg"]) + self.assertNotIn("--optional-flag", result) + + def test_legacy_optional_hint_with_variables_skipped(self): + """Legacy entries with is_required: False and a variables dict are skipped.""" + adapter = self._adapter() + args = [ + { + "value_hint": "{optionalPath}:/data", + "is_required": False, + "variables": { + "optionalPath": {"description": "Optional mount", "is_required": False} + }, + }, + {"value_hint": "required-arg"}, + ] + result = adapter._process_arguments(args, resolved_env={}, runtime_vars={}) + self.assertEqual(result, ["required-arg"]) + # --------------------------------------------------------------------------- # Codex adapter @@ -180,6 +207,33 @@ def test_v01_full_docker_arg_set_preserved(self): self.assertEqual(result[0], "run") self.assertEqual(result[4], "/ws:/workspace") + def test_legacy_optional_hint_skipped(self): + """Legacy entries with is_required: False must not be appended.""" + adapter = self._adapter() + args = [ + {"value_hint": "--optional-flag", "is_required": False}, + {"value_hint": "required-arg"}, + ] + result = adapter._process_arguments(args, resolved_env={}, runtime_vars={}) + self.assertEqual(result, ["required-arg"]) + self.assertNotIn("--optional-flag", result) + + def test_legacy_optional_hint_with_variables_skipped(self): + """Legacy entries with is_required: False and a variables dict are skipped.""" + adapter = self._adapter() + args = [ + { + "value_hint": "{optionalPath}:/data", + "is_required": False, + "variables": { + "optionalPath": {"description": "Optional mount", "is_required": False} + }, + }, + {"value_hint": "required-arg"}, + ] + result = adapter._process_arguments(args, resolved_env={}, runtime_vars={}) + self.assertEqual(result, ["required-arg"]) + if __name__ == "__main__": unittest.main()