fix: handle MCP v0.1 runtimeArguments.variables in copilot and codex adapters#1461
Conversation
…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>
There was a problem hiding this comment.
Pull request overview
This PR fixes MCP registry v0.1 runtimeArguments parsing in the non-VS Code MCP client adapters by teaching the Copilot and Codex adapters to accept the {value_hint, variables} argument shape and substitute {var} placeholders from runtime_vars, aligning behavior with the existing VS Code adapter so Docker-based servers generate correct argument lists.
Changes:
- Add a v0.1
value_hint/variablesparsing branch to_process_argumentsin the Copilot adapter. - Add the same v0.1 parsing/substitution branch to
_process_argumentsin the Codex adapter (benefiting inheritors of the Copilot adapter as noted in the PR description). - Add unit regressions tests covering plain
value_hint, variable substitution, unknown variables, and a full Docker arg fixture.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
tests/unit/adapters/test_v01_variables_process_arguments.py |
Adds regression coverage for v0.1 value_hint + variables runtime arguments in Copilot and Codex. |
src/apm_cli/adapters/client/copilot.py |
Extends _process_arguments to handle v0.1 value_hint args and substitute {var} placeholders. |
src/apm_cli/adapters/client/codex.py |
Mirrors the Copilot _process_arguments v0.1 handling in the Codex adapter. |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 1 | Correct fix with acceptable duplication; shared helper on MCPClientAdapter is the natural follow-up. |
| CLI Logging Expert | 0 | 0 | 1 | No CLI output surface touched; silent placeholder fallback matches VS Code parity. |
| DevX UX Expert | 0 | 0 | 1 | Cross-harness parity restored; inheritance claim verified across all 5 dependent adapters. |
| Supply Chain Security Expert | 0 | 0 | 0 | No new trust boundary; runtime_vars are user-supplied via interactive prompt. |
| OSS Growth Hacker | 0 | 1 | 0 | Named third-party tool unlock deserves a CHANGELOG line and release-note beat; contributor credit is a retention lever. |
| Doc Writer | 0 | 1 | 1 | CHANGELOG drift: [Unreleased] is empty; needs a Fixed entry symmetric to the 0.14.2 entry for #1444. |
| Test Coverage Expert | 0 | 0 | 0 | 8 new unit tests pass (4.37s); mutation-break holds; inheritance verified; no coverage gaps. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 4 follow-ups
- [Doc Writer] Add CHANGELOG Fixed entry under
[Unreleased]naming all 7 adapters and crediting the contributor. -- Process requirement perchangelog.instructions.md; symmetric to the 0.14.2 entry for fix(vscode): handle v0.1 runtimeArguments with variables for Docker MCP (#1391) #1444. Also the highest-signal growth lever (named-tool unlock visible to upgrading users). - [Python Architect] Extract v0.1 variable substitution to a shared static helper on
MCPClientAdapter. -- The 15-line elif block is now copy-pasted acrosscopilot.py,codex.py, andvscode.py(with a minor divergence). A shared helper DRYs the logic and makes substitution semantics unit-testable in isolation. Follow-up issue, not blocking. - [DevX UX Expert] Add inline comment explaining intentional omission of
workspaceFolderspecial-case in copilot/codex path. -- Prevents a future contributor from "fixing" a divergence that is intentional (non-VS-Code harnesses have no native${workspaceFolder}expansion). - [CLI Logging Expert] Emit a verbose-only diagnostic when a variable falls back to a literal placeholder. -- Aids user debugging when
runtime_varslacks a declared variable; parity with VS Code adapter (both currently silent) means this is a future improvement, not a regression.
Architecture
classDiagram
direction TB
class MCPClientAdapter {
<<Abstract>>
+_resolve_variable_placeholders(value, resolved_env, runtime_vars) str
}
class CopilotClientAdapter {
+_process_arguments(arguments, resolved_env, runtime_vars) list
}
class CodexClientAdapter {
+_process_arguments(arguments, resolved_env, runtime_vars) list
}
class VSCodeClientAdapter {
+_extract_package_args(package, runtime_vars) list
}
class ClaudeClientAdapter
class CursorClientAdapter
class GeminiClientAdapter
class OpenCodeClientAdapter
class WindsurfClientAdapter
MCPClientAdapter <|-- CopilotClientAdapter
MCPClientAdapter <|-- CodexClientAdapter
MCPClientAdapter <|-- VSCodeClientAdapter
CopilotClientAdapter <|-- ClaudeClientAdapter
CopilotClientAdapter <|-- CursorClientAdapter
CopilotClientAdapter <|-- GeminiClientAdapter
CopilotClientAdapter <|-- OpenCodeClientAdapter
CopilotClientAdapter <|-- WindsurfClientAdapter
class CopilotClientAdapter:::touched
class CodexClientAdapter:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A[apm install / add command] --> B[adapter._process_arguments runtime_arguments]
B --> C{isinstance arg dict?}
C -->|Yes| D{arg_type == positional?}
C -->|No| K[isinstance arg str: resolve placeholders]
D -->|Yes| E[_resolve_variable_placeholders value]
D -->|No| F{arg_type == named?}
F -->|Yes| G[append flag + optional value]
F -->|No| H{not arg_type and value_hint in arg?}
H -->|Yes| I[v0.1 path: substitute variables into value_hint]
H -->|No| L[arg silently skipped]
I --> J[_resolve_variable_placeholders substituted_value]
J --> M[processed.append result]
E --> M
G --> M
K --> M
M --> N[return processed list]
style I fill:#fff3b0,stroke:#d47600
style H fill:#fff3b0,stroke:#d47600
Recommendation
Merge once a CHANGELOG entry is added under [Unreleased] -> ### Fixed (process requirement, not optional). The fix is correct, fully tested, inheritance-verified, and symmetric to the already-shipped #1444. The shared-helper extraction and inline-comment nits are post-merge follow-ups the team should track in a separate issue. Recommend the maintainer request the CHANGELOG addition from the contributor or add it at merge to keep the PR moving.
Full per-persona findings
Python Architect
- [recommended] Extract v0.1 variable substitution to a shared helper on
MCPClientAdapter. atsrc/apm_cli/adapters/client/codex.py:422
The 15-line v0.1 elif block is now copy-pasted incopilot.py:L982andcodex.py:L422.VSCodeClientAdapter._extract_package_argshas a third near-identical variant with an extraworkspaceFolderspecial case. A static helper_substitute_v01_variables(value_hint, variables, runtime_vars) -> stronMCPClientAdapterwould DRY all three sites and make substitution semantics unit-testable in isolation. Not blocking because pylint disable is already in place and the fix is correct; this is a follow-up the team should track. - [nit] Redundant
str()cast on value already known to be a string. atsrc/apm_cli/adapters/client/copilot.py:994
arg['value_hint']is always astrfrom JSON deserialization; wrapping it instr()before passing to_resolve_variable_placeholdersis a no-op cast that adds visual noise.
CLI Logging Expert
- [nit] Consider a verbose-only diagnostic when an arg variable falls back to a literal placeholder. at
src/apm_cli/adapters/client/copilot.py
Whenruntime_varslacks a declared variable, the code emits a${varName}literal silently. ADiagnosticCollectorinfo-tier note (verbose-only) would let users debug why an arg kept a raw placeholder. The vscode adapter is also silent so parity is correct for now; this is purely a future-improvement nit.
DevX UX Expert
- [nit] Consider one-line comment explaining the intentional divergence from vscode's
workspaceFolderspecial-case. atsrc/apm_cli/adapters/client/copilot.py:992
The vscode adapter special-casesworkspaceFolderto emit the VS Code-native${workspaceFolder}token. The new copilot/codex branch emits a generic${varName}for all unknowns (correct for non-vscode harnesses since they have no native${workspaceFolder}expansion). A short inline comment would prevent a future contributor from "fixing" this divergence.
Supply Chain Security Expert
No findings.
OSS Growth Hacker
- [recommended] Add a CHANGELOG entry naming the third-party tool unlock and crediting the contributor. at
CHANGELOG.md
Microsoft Playwright MCP and other v0.1-shaped servers now work in Copilot, Codex, Gemini, Cursor, Claude, Windsurf, and OpenCode is a concrete repostable claim that drives word-of-mouth. Shipping without a CHANGELOG line makes the fix invisible to upgrading users and wastes the release-note surface. Named-tool unlocks compound recognition of the multi-harness-support promise. Crediting @sergio-sisternes-epam in the entry is the cheapest retention lever for the next community contributor.
Suggested: Under## [Unreleased]->### Fixed: "MCP v0.1 servers (including Microsoft Playwright MCP) now resolve correctly in Copilot, Codex, Gemini, Cursor, Claude, Windsurf, and OpenCode adapters, matching the VS Code fix from fix(vscode): handle v0.1 runtimeArguments with variables for Docker MCP (#1391) #1444. (fix: handle MCP v0.1 runtimeArguments.variables in copilot and codex adapters #1461, closes Extend MCP v0.1 runtimeArguments.variables handling to all client adapters (P3 follow-up to #1444) #1452, thanks @sergio-sisternes-epam)."
Auth Expert -- inactive
PR touches only MCP runtime-argument parsing in copilot.py and codex.py adapters with a matching test file; no auth, token, credential, or host-classification surface is affected.
Doc Writer
- [recommended] Missing CHANGELOG Fixed entry under
[Unreleased]. atCHANGELOG.md
Per.github/instructions/changelog.instructions.md, every merged PR that changes code must have a changelog entry. The[Unreleased]section is currently empty. This PR is the symmetric counterpart to 0.14.2's entry for fix(vscode): handle v0.1 runtimeArguments with variables for Docker MCP (#1391) #1444 (VS Code adapter MCP v0.1) and should follow the same wording shape so users grepping for MCP v0.1 find both halves of the fix.
Suggested: Add under## [Unreleased]->### Fixed: "Copilot, Codex, Cursor, Claude, Windsurf, OpenCode, and Gemini adapters handle MCP v0.1runtimeArguments/packageArgumentswithvariables(notypekey), matching the VS Code fix from fix(vscode): handle v0.1 runtimeArguments with variables for Docker MCP (#1391) #1444. (fix: handle MCP v0.1 runtimeArguments.variables in copilot and codex adapters #1461, closes Extend MCP v0.1 runtimeArguments.variables handling to all client adapters (P3 follow-up to #1444) #1452)". Naming the inheriting adapters avoids the reader assuming only copilot+codex were repaired. - [nit] Preserve the inheritance framing from the PR description in the CHANGELOG line.
Verified:src/apm_cli/adapters/client/{cursor,claude,windsurf,opencode,gemini}.pyall declareclass XClientAdapter(CopilotClientAdapter), none override_process_arguments, and Gemini's overridden_format_server_configstill routes throughself._process_arguments. The "inherits the fix automatically" framing is correct. Naming the inheriting adapters in the CHANGELOG prevents the reader from re-deriving that mapping from source.
Test Coverage Expert
No findings.
Evidence: tests/unit/adapters/test_v01_variables_process_arguments.py -- 8 unit tests passed in 4.37s; tests directly exercise the new v0.1 elif path in both adapters; mutation-break claim holds (removing the new branch would fail all 8). Inheritance verified: gemini/cursor/claude/windsurf/opencode extend CopilotClientAdapter without override.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
…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 <sergio-sisternes-epam@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Follow-ups from the apm-review-panel pass have landed. Summary:
No regression-trap tests were added in this completion pass; the PR's existing 8 unit tests already cover the new elif branch in both copilot and codex adapters with mutation-break-verified coverage (test-coverage-expert confirmed in the panel pass). Lint contract: CI: All 12 checks successful (0 failing, 0 pending) on head af4bee1. Ready for maintainer review. |
sergio-sisternes-epam
added
the
panel-review
- 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>
github-actions
Bot
removed
the
panel-review
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 2 | 1 | Fix is correct and safe; the duplicate _process_arguments across Copilot+Codex is the pre-existing debt this PR compounds -- lift to MCPClientAdapter base. |
| CLI Logging Expert | 0 | 2 | 1 | No logging in _process_arguments; missing verbose-mode trace for unresolved vars leaves agent operators blind to placeholder substitution fallback. |
| DevX UX Expert | 0 | 2 | 1 | Fix is correct and silently safe for the happy path; missing-required-variable fallback is a silent time-bomb that will confuse users at runtime. |
| Supply Chain Security Expert | 0 | 2 | 1 | No shell-injection vector; args go to exec not shell. Two recommended hardening gaps: no type-check on value_hint, no sanitization of user-supplied runtime_vars before Docker arg substitution. |
| OSS Growth Hacker | 0 | 2 | 1 | High-value silent-failure fix for Docker/MCP v0.1 packages across 7 adapters; CHANGELOG undersells the 'parity with VS Code' story. |
| Doc Writer | 0 | 1 | 2 | CHANGELOG adapter list is correct (Windsurf/OpenCode confirmed CopilotClientAdapter subclasses); CEO confirms packageArguments scope is also covered. |
| Test Coverage Expert | 0 | 1 | 2 | 8 unit regression tests cover the bug-fix path well; one tier-floor gap: the v0.1 branch has no integration-with-fixtures test through the full adapter download pipeline. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
-
[Test Coverage Expert] Add integration-with-fixtures test: build a fake v0.1 registry response with variables args, feed through full adapter download pipeline, assert resolved Docker mount arg appears in generated config. -- Missing evidence outcome on an install-pipeline surface. Unit tests pass even if upstream arg shape changes before _process_arguments; only an integration test catches that regression. Highest-signal gap in the PR.
-
[CLI Logging Expert] Emit _rich_warning when a required (is_required=True) variable is absent from runtime_vars and falls back to the ${varName} placeholder; emit logger.debug for optional variables and for silently dropped unknown arg formats. -- Three panelists (cli-logging, devx-ux, supply-chain) independently identified this silent-failure path. Docker bind-mount errors produced by unresolved placeholders are cryptic and hard to diagnose without an upstream warning. This is the highest-signal UX gap.
-
[OSS Growth Hacker] Rewrite CHANGELOG entry to lead with the user-trust story and the VS Code parity frame, name all seven adapters, and cite the closing issue. -- The current entry reads as an internal diff note. 'parity with VS Code' is a conversion hook for users choosing between editors and belongs in the headline of the entry, not implied by the code.
-
[Python Architect] Open a follow-up issue to audit the inverted name/value field mapping in codex.py _process_arguments named-arg branch vs copilot.py, add a regression test, and track the lift of _process_arguments to MCPClientAdapter base. -- The field swap (flag reads arg.get('value'), value reads arg.get('name') in codex.py vs the reverse in copilot.py) is a latent correctness bug. The duplication makes any future protocol version branch require two synchronized edits and risks further drift.
-
[Supply Chain Security Expert] Add isinstance(value, str) guard after value = arg['value_hint'] in both copilot.py and codex.py; evaluate path-traversal validation for runtime_vars values that resolve to filesystem paths. -- A malformed registry entry supplying a list or dict as value_hint causes an unguarded AttributeError. The test-coverage-expert separately confirmed value_hint=None with a sibling variables dict is also untested and triggers the same crash.
Architecture
classDiagram
direction LR
class MCPClientAdapter {
<<ABC>>
+install(server_info, env_overrides, runtime_vars)
+_resolve_variable_placeholders(value, env, vars) str
+_warn_input_variables(env, name, label)
}
class CopilotClientAdapter {
<<ConcreteAdapter>>
+_process_arguments(arguments, resolved_env, runtime_vars) list
+_format_server_config(server_info, env_overrides, runtime_vars)
+_resolve_environment_variables(env_vars, env_overrides) dict
}
class CodexClientAdapter {
<<ConcreteAdapter>>
+_process_arguments(arguments, resolved_env, runtime_vars) list
+_format_server_config(server_info, env_overrides, runtime_vars)
+_process_environment_variables(env_vars, env_overrides) dict
}
class VSCodeClientAdapter {
<<ConcreteAdapter>>
+_extract_package_args(package, runtime_vars) list
}
class GeminiClientAdapter {
<<ConcreteAdapter - inherits fix>>
}
class CursorClientAdapter {
<<ConcreteAdapter - inherits fix>>
}
class ClaudeClientAdapter {
<<ConcreteAdapter - inherits fix>>
}
class WindsurfClientAdapter {
<<ConcreteAdapter - inherits fix>>
}
class OpenCodeClientAdapter {
<<ConcreteAdapter - inherits fix>>
}
MCPClientAdapter <|-- CopilotClientAdapter
MCPClientAdapter <|-- CodexClientAdapter
MCPClientAdapter <|-- VSCodeClientAdapter
CopilotClientAdapter <|-- GeminiClientAdapter
CopilotClientAdapter <|-- CursorClientAdapter
CopilotClientAdapter <|-- ClaudeClientAdapter
CopilotClientAdapter <|-- WindsurfClientAdapter
CopilotClientAdapter <|-- OpenCodeClientAdapter
class CopilotClientAdapter:::touched
class CodexClientAdapter:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["apm install --client copilot or codex"] --> B["install() -- base.py MCPClientAdapter"]
B --> C["_format_server_config() -- copilot.py / codex.py"]
C --> D["[I/O] package registry fetch"]
D --> E{"arg format?"}
E -->|"type=positional"| F["arg.get value/default\n-> _resolve_variable_placeholders()"]
E -->|"type=named"| G["append flag name + optional value"]
E -->|"no type + value_hint\n[NEW BRANCH]"| H["value = arg[value_hint]\nfor var_name in variables:\n resolve from runtime_vars\n or fall back to placeholder\n-> _resolve_variable_placeholders()"]
E -->|"str"| I["_resolve_variable_placeholders() direct"]
F --> J["processed list append"]
G --> J
H --> J
I --> J
J --> K["[FS] write config JSON/TOML"]
Recommendation
Ship this PR. The bug fix is correct, the test coverage for the new branch is solid at the unit tier, and the VS Code parity story is genuinely valuable for the seven affected adapters. The CHANGELOG entry should be rewritten before merge (growth-hacker finding, five-minute change). The five follow-ups above should be filed as issues immediately after merge -- the integration-with-fixtures gap and the unresolved-required-variable warning are the two that will matter most to operators running Docker MCP packages in production.
Full per-persona findings
Python Architect
-
[recommended] _process_arguments is fully duplicated in copilot.py and codex.py; the pylint suppress comment admits it at
src/apm_cli/adapters/client/base.py:95
The PR correctly adds identical elif branches to both files, but the method is byte-for-byte duplicated (codex.py even carries a pylint disable=duplicate-code comment as a marker). Any future protocol version branch will require a third synchronized edit. Lifting _process_arguments to MCPClientAdapter eliminates the drift surface.
Suggested: Add _process_arguments to MCPClientAdapter with the full copilot.py implementation as the canonical body. Remove it from CopilotClientAdapter and CodexClientAdapter (audit the named-arg field delta before merging). -
[recommended] named-arg field mapping diverges between copilot.py and codex.py -- possible latent bug pre-dating this PR at
src/apm_cli/adapters/client/codex.py:409
In copilot.py the named branch reads flag from arg.get('name') and value from arg.get('value'); in codex.py it reads flag from arg.get('value') and value from arg.get('name'). The field swap is suspicious. One adapter is probably wrong.
Suggested: Add a regression test for a named-arg round-trip through both adapters, then audit which field mapping matches the registry schema and align both adapters. -
[nit] fallback placeholder uses shell syntax ${var} instead of registry syntax {var} -- needs a comment explaining intent at
src/apm_cli/adapters/client/copilot.py:992
Suggested: Add inline comment: # emit ${var} so unresolved placeholders are visible in the output config
CLI Logging Expert
-
[recommended] Unresolved v0.1 variables fall back to '${varName}' placeholder with no diagnostic output at
src/apm_cli/adapters/client/copilot.py
When a required variable (is_required: True) is missing from runtime_vars, the arg is passed downstream as a raw token. For Docker mount args this silently produces an invalid bind-mount path. Operators running apm with --verbose have no way to detect this at install/run time.
Suggested: After the else branch, emit a _rich_warning for required variables, or log at debug/verbose for optional ones. -
[recommended] Completely unrecognized dict args (no type, no value_hint) are silently dropped with no diagnostic at
src/apm_cli/adapters/client/copilot.py
An unknown arg format could indicate a registry schema version the adapter does not yet handle.
Suggested: Add an else branch: logger.debug('Skipping unrecognized arg format (keys: %s)', list(arg.keys())) -
[nit] Identical code block in codex.py and copilot.py -- no logging consistency risk now but will diverge at
src/apm_cli/adapters/client/codex.py
Suggested: Consider extracting the shared _process_arguments body into a mixin or base-class method so logging changes propagate automatically.
DevX UX Expert
-
[recommended] Required variables that are unresolved silently produce a ${varName} placeholder passed to Docker, causing a cryptic mount failure at runtime at
src/apm_cli/adapters/client/copilot.py
When is_required=true and the variable is absent from runtime_vars, the adapter emits '${customVar}:/data' as a literal Docker argument. Docker will fail with an obscure bind-mount error or mount a directory literally named '${customVar}'.
Suggested: After the substitution loop, check if any is_required variable was not resolved and emit a _rich_warning naming the variable and a hint ('pass --var workspaceFolder=(path) or set it in apm.yml'). Do the same in codex.py. -
[recommended] Silent drop of empty value_hint string is invisible to the user and can produce a wrong arg list without any signal at
src/apm_cli/adapters/client/copilot.py
The 'if value:' guard silently skips an arg whose value resolves to an empty string, potentially shifting positional Docker arguments with no warning.
Suggested: Log at debug/verbose level when value is empty after substitution, including the original value_hint template. -
[nit] Test name 'unknown var gets placeholder' frames a footgun as expected behavior at
tests/unit/adapters/test_v01_variables_process_arguments.py
Suggested: Rename to 'test_v01_required_var_missing_falls_back_to_literal_placeholder' and add a TODO comment noting that a warning should accompany this path.
Supply Chain Security Expert
-
[recommended] value_hint is not type-checked before string operations; a non-string registry value causes an unguarded AttributeError at
src/apm_cli/adapters/client/copilot.py
The code does value = arg['value_hint'] then value.replace(...) without asserting it is a str. A malformed or adversarial registry entry supplying a list or dict as value_hint would cause an AttributeError.
Suggested: Add isinstance(value, str) guard immediately after value = arg['value_hint'], before the replace loop. -
[recommended] User-supplied runtime_vars values flow into Docker volume mount args with no path-traversal sanitization at
src/apm_cli/adapters/client/copilot.py
runtime_vars is populated by _prompt_for_environment_variables, which reads raw user input. A user entering ../../etc is substituted verbatim and lands in config['args'] for Docker. The new branch widens an existing surface.
Suggested: After variable substitution, if the resolved value looks like a path, run it through path_security.ensure_path_within or validate_path_segments. -
[nit] ${varName} fallback string written to JSON config could mislead downstream tooling at
src/apm_cli/adapters/client/copilot.py
Shell-based launchers that later see this string in the JSON config file may attempt to expand it. Very low risk because shell=True is not used.
Suggested: Log a warning via existing _rich_warning helper when a variable falls back to the literal placeholder.
OSS Growth Hacker
-
[recommended] CHANGELOG entry buries the lead -- 'silently dropped args' is a user-trust story, not just a bug note
Docker-based MCP packages with workspaceFolder mounts are a high-growth pattern. Users who hit this saw empty arg lists with no error -- the worst kind of failure. The entry reads like an internal diff note.
Suggested: Rewrite as: 'Fixed: Docker MCP packages using v0.1 runtimeArguments.variables (e.g. {workspaceFolder} mounts) were silently dropped in Copilot, Codex, Gemini, Cursor, Claude, Windsurf, and OpenCode adapters -- args now resolve correctly, matching VS Code behavior. (fix: handle MCP v0.1 runtimeArguments.variables in copilot and codex adapters #1461, closes Extend MCP v0.1 runtimeArguments.variables handling to all client adapters (P3 follow-up to #1444) #1452, thanks @sergio-sisternes-epam)' -
[recommended] The 'parity with VS Code' frame is a growth asset -- missing from the CHANGELOG entry
The PR body and test docstring both say 'matching VS Code behavior'. Users choosing between editors now have a concrete guarantee APM treats them equally. That phrase is absent from the CHANGELOG entirely.
Suggested: Add: '(all adapters now match VS Code behavior introduced in fix(vscode): handle v0.1 runtimeArguments with variables for Docker MCP (#1391) #1444)' -
[nit] CHANGELOG adapter list (Windsurf, OpenCode) now confirmed accurate -- doc-writer verified both extend CopilotClientAdapter
Doc Writer
-
[recommended] CHANGELOG adds 'packageArguments' alongside 'runtimeArguments' -- CEO has confirmed this is accurate (_process_arguments is called for both at lines 623 and 626 of copilot.py). PR body should be updated to mention packageArguments for completeness. at
CHANGELOG.md
Suggested: Update PR body to note that the fix covers both runtimeArguments and packageArguments. -
[nit] Windsurf and OpenCode are correctly listed in CHANGELOG -- confirmed CopilotClientAdapter subclasses at
CHANGELOG.md
No action needed. -
[nit] Adapter enumeration order is arbitrary in the CHANGELOG entry at
CHANGELOG.md
Suggested: 'Copilot and Codex (and Cursor, Claude, Gemini, OpenCode, Windsurf adapters that inherit from CopilotClientAdapter) now handle ...'
Test Coverage Expert
-
[recommended] v0.1 _process_arguments branch has no integration-with-fixtures coverage through the adapter download pipeline at
tests/integration/test_download_copilot_phase3.py
The new elif branch is only tested via direct unit calls to _process_arguments. If upstream code changes its arg shape before reaching _process_arguments, the unit tests would still pass but users would again get silently dropped Docker args.
Suggested: Add a test class that builds a fake registry response with v0.1 value_hint+variables args, feeds it through the adapter full pipeline, and asserts the produced config contains the resolved Docker mount arg.
Proof (missing at integration-with-fixtures):tests/integration/test_download_copilot_phase3.py::TestCopilotAdapterV01RuntimeArgs::test_v01_docker_args_flow_through_full_pipeline-- proves: A Docker MCP server registered with v0.1 runtimeArguments.variables produces a correctly resolved config after apm install [multi-harness-support]
assert '/home/user/project:/workspace' in generated_config['servers']['srv']['args'] -
[nit] value_hint=None with a sibling variables dict causes unguarded AttributeError -- not tested at
tests/unit/adapters/test_v01_variables_process_arguments.py
value = arg['value_hint'] is assigned before the variables loop. If value_hint is None and variables is also present, value.replace(...) raises AttributeError before the if value: guard.
Suggested: Add test_v01_value_hint_none_with_variables_skipped, then guard copilot.py:987 with 'if not isinstance(value, str): continue'.
Proof (missing at unit):tests/unit/adapters/test_v01_variables_process_arguments.py::TestCopilotProcessArgumentsV01Variables::test_v01_value_hint_none_with_variables_skipped
self.assertEqual(adapter._process_arguments([{'value_hint': None, 'variables': {'x': {}}}], {}, {}), []) -
[nit] Inherited adapters (Claude, Cursor, Gemini) have no explicit smoke test -- acceptable given pure inheritance but worth noting at
tests/unit/adapters/test_v01_variables_process_arguments.py
grep confirms none of claude.py, cursor.py, gemini.py override _process_arguments. A parametrized test would make the inheritance assumption explicit.
Suggested:@pytest.mark.parametrize('adapter_cls', [CursorClientAdapter, ClaudeClientAdapter, GeminiClientAdapter]) on test_v01_plain_value_hint_args_extracted.
Proof (missing at unit):tests/unit/adapters/test_v01_variables_process_arguments.py::TestInheritedAdaptersV01Variables::test_cursor_v01_plain_value_hint
self.assertEqual(cursor_adapter._process_arguments([{'value_hint': 'run'}], {}, {}), ['run'])
Auth Expert -- inactive
PR only touches adapter argument-parsing logic (codex.py, copilot.py) and tests -- no auth, token, credential, or host-classification code is modified.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pypi.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "pypi.org"See Network Configuration for more information.
Generated by PR Review Panel for issue #1461 · ● 3M · ◷
Problem
The
_process_argumentsmethod incopilot.pyandcodex.pyonly handled arg dicts withtype=positional,type=named, or plain strings. Args using the v0.1 registry format (value_hint+variablesdict, notypekey) were silently dropped.This meant Docker mount arguments with
{workspaceFolder}placeholders produced empty arg lists in Copilot, Codex, Gemini, Cursor, and Claude adapters -- while VS Code handled them correctly.Fix
Add an
elifbranch in bothcopilot.pyandcodex.py_process_argumentsfor v0.1value_hintargs (with optionalvariablesdict) that performs{var_name}placeholder substitution, matching the existing logic inVSCodeClientAdapter._extract_package_args.Gemini, Cursor, and Claude inherit from
CopilotClientAdapterso they get the fix automatically.Tests
Fixes #1452