Add extra passthrough for harness-specific MCP keys (#1670)#1765
Add extra passthrough for harness-specific MCP keys (#1670)#1765sergio-sisternes-epam wants to merge 2 commits into
Conversation
MCPDependency now captures unknown keys from apm.yml into an 'extra' dict instead of silently dropping them. The extra fields round-trip through to_dict() and flow through _build_self_defined_info() into every client adapter's _format_server_config(), where _merge_extra() writes them into the generated target manifest without shadowing known keys. This enables harness-specific configuration such as Claude Code's oauth block for remote-MCP OAuth client config to be declared once in apm.yml and appear in the generated .mcp.json. Closes #1670 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a forward-compatible “extra passthrough” mechanism for MCP dependencies: unknown keys found in apm.yml MCP dependency objects are preserved on the model and then merged into the generated target manifests so harness-specific config (e.g., an oauth block) is not lost.
Changes:
- Extend
MCPDependencyto capture unknown keys intoextraand round-trip them viato_dict(). - Plumb
_extrathrough MCP integration and merge it into per-harness adapter configs. - Add/adjust unit tests and update docs to describe the passthrough behavior with examples.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/models/dependency/mcp.py |
Adds extra field and merges unknown keys into extra, then into to_dict() output. |
src/apm_cli/integration/mcp_integrator.py |
Adds _extra passthrough for adapter consumption (currently only in self-defined path; see comments). |
src/apm_cli/adapters/client/base.py |
Introduces _merge_extra() helper used by adapters to merge passthrough keys. |
src/apm_cli/adapters/client/copilot.py |
Calls _merge_extra() before returning formatted config. |
src/apm_cli/adapters/client/codex.py |
Calls _merge_extra() on some return paths (remote-only path still misses it; see comments). |
src/apm_cli/adapters/client/cursor.py |
Calls _merge_extra() before returning formatted config. |
src/apm_cli/adapters/client/gemini.py |
Calls _merge_extra() before returning formatted config. |
src/apm_cli/adapters/client/kiro.py |
Calls _merge_extra() before returning formatted config. |
src/apm_cli/adapters/client/vscode.py |
Calls _merge_extra() before returning formatted config. |
tests/unit/test_mcp_from_dict_unknown_keys.py |
Updates/adds tests for unknown-key preservation, round-tripping, and merge behavior. |
docs/src/content/docs/reference/manifest-schema.md |
Documents extra passthrough semantics and adds an example. |
docs/src/content/docs/consumer/install-mcp-servers.md |
Adds an example using a harness-specific extra key (oauth). |
packages/apm-guide/.apm/skills/apm-usage/dependencies.md |
Updates dependency docs with an extra-key example for MCP. |
…remote path - Add 'extra' to _KNOWN_DICT_KEYS so an explicit 'extra:' YAML block merges into dep.extra without nesting - Propagate dep.extra via _apply_overlay for registry-resolved deps - Add _merge_extra call in codex remote-only return path - Add 7 new tests covering all three fixes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 2 | Solid passthrough chain; two quality findings: hasattr guard on always-MCPDependency param, _merge_extra mutates in-place and returns (all callers ignore return). |
| CLI Logging Expert | 0 | 2 | 2 | Stale 'dropped' docstring contradicts new behavior; [!] symbol misfires for working-as-intended passthrough; message body leaves outcome unstated. |
| DevX UX Expert | 0 | 3 | 2 | Feature works; 3 UX gaps worth fixing: warning fires on documented pattern, message misleads about output shape, pack credential gap unacknowledged in docs. |
| Supply Chain Security Expert | 0 | 2 | 1 | Shadow prevention holds in both to_dict and _merge_extra; apm pack sanitizer leaves non-credential-named oauth sub-keys intact; HTTP server command injection via extra is possible (consumer-controlled only). |
| OSS Growth Hacker | 0 | 2 | 2 | Extra passthrough fixes a real retention-killer; two gaps undercut the trust story: missing CHANGELOG entry and no in-docs pack-safety advisory for extra keys. |
| Auth Expert | 0 | 2 | 2 | No auth bypass or AuthResolver interaction; two recommended findings on secret persistence and cross-adapter oauth leakage; one nit on env-via-extra. |
| Doc Writer | 0 | 4 | 1 | 5 gaps: stale form-count, no cross-link, undocumented explicit extra: key, warning asymmetry, duplicate example -- no blockers. |
| Test Coverage Expert | 0 | 3 | 0 | 25 unit tests cover each isolated segment well; missing integration-with-fixtures for full apm.yml->manifest pipeline; 6 adapters have 17 uncovered _merge_extra call sites. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [CLI Logging Expert] Replace
[!]with_rich_infoand rewrite warning body to name the target manifest output, not the internal dict name -- Warning fires on the documented top-leveloauth:example;[!]implies data loss where none exists; "preserved in extra" misleads about output shape (oauth appears at top-level in .mcp.json, not nested under an extra key). Convergent with devx-ux-expert. Pre-merge, one-line fix: suggested body"MCP dependency '{name}': unknown key(s) will appear verbatim in target manifests: {safe_keys}". - [OSS Growth Hacker] Add CHANGELOG [Unreleased] entry for the warn-and-drop to warn-and-preserve behavior change -- Behavior change is undocumented in the durable release record; upgrading users who grep CI logs for the old dropped-key warning text get no migration signal. Pre-merge, one line.
- [Supply Chain Security Expert] Add one-sentence pack safety advisory to manifest-schema.md: extra keys are written verbatim to packed bundles; do not place secrets in extra fields -- Four panelists flag this independently: the pack sanitizer does not strip 'oauth' or 'client_id', so a clientSecret in the extra block survives
apm packunredacted. Pre-merge, one sentence. - [Auth Expert] Add per-adapter passthrough_keys allowlist or document explicitly that extra broadcasts to all adapter configs -- oauth is Claude Code-specific per docs but
_merge_extrafires in all seven adapters; a future harness that interprets oauth differently receives the Claude Code client config. Post-merge issue. - [Test Coverage Expert] Add integration-with-fixtures test for full apm.yml -> MCPIntegrator -> adapter -> manifest pipeline, and per-adapter _merge_extra wiring tests -- Evidence outcome=missing: no test exercises the complete chain with real objects; 17 call sites across six adapters have no per-adapter wiring test. Post-merge issue.
Architecture
classDiagram
direction LR
class MCPDependency {
<<DataClass>>
+name str
+transport str|None
+extra dict|None
+from_dict(d) MCPDependency
+to_dict() dict
+validate(strict) None
}
note for MCPDependency "extra captures unknown apm.yml keys; explicit extra block wins over same-named auto-captured keys; known fields win in to_dict"
class MCPIntegrator {
<<Service>>
-_build_self_defined_info(dep) dict
-_apply_overlay(cache, dep) None
+get_server_configs(deps) dict
}
note for MCPIntegrator "dep untyped in both statics; hasattr guard always True for MCPDependency -- add dep: MCPDependency annotation"
class MCPClientAdapter {
<<AbstractBase>>
+target_name str
+mcp_servers_key str
+_merge_extra(config,server_info) dict
+configure_mcp_server()
}
note for MCPClientAdapter "_merge_extra mutates config in-place AND returns it; all callers ignore return -- remove return, annotate void"
class CopilotClientAdapter {
<<Template>>
#_format_server_config(server_info) dict
}
class CursorClientAdapter {
<<ConcreteAdapter>>
#_format_server_config(server_info) dict
}
class CodexClientAdapter {
<<ConcreteAdapter>>
#_format_server_config(server_info) dict
}
class GeminiClientAdapter {
<<ConcreteAdapter>>
#_format_server_config(server_info) dict
}
class KiroClientAdapter {
<<ConcreteAdapter>>
#_format_server_config(server_info) dict
}
class VSCodeClientAdapter {
<<ConcreteAdapter>>
#_format_server_config(server_info) tuple
}
class ClaudeClientAdapter {
<<ConcreteAdapter>>
#_format_server_config(server_info) dict
}
class WindsurfClientAdapter {
<<ConcreteAdapter>>
}
MCPIntegrator ..> MCPDependency : reads
MCPIntegrator ..> MCPClientAdapter : dispatches to
MCPClientAdapter <|-- CopilotClientAdapter
MCPClientAdapter <|-- CursorClientAdapter
MCPClientAdapter <|-- CodexClientAdapter
MCPClientAdapter <|-- GeminiClientAdapter
MCPClientAdapter <|-- KiroClientAdapter
MCPClientAdapter <|-- VSCodeClientAdapter
CopilotClientAdapter <|-- ClaudeClientAdapter
CopilotClientAdapter <|-- WindsurfClientAdapter
class MCPDependency:::touched
class MCPIntegrator:::touched
class MCPClientAdapter:::touched
class CopilotClientAdapter:::touched
class CursorClientAdapter:::touched
class CodexClientAdapter:::touched
class GeminiClientAdapter:::touched
class KiroClientAdapter:::touched
class VSCodeClientAdapter:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["apm.yml: MCP dep with unknown keys"] --> B
B["mcp.py: from_dict()\n_KNOWN_DICT_KEYS check\nunknown keys detected"] --> C
C{"explicit extra: block in input?"}
C -- Yes --> D["extra = unknown_keys merged with explicit_extra\nexplicit_extra wins on collision"]
C -- No --> E["extra = unknown keys only"]
D --> F
E --> F
F["MCPDependency.extra assigned\n_rich_warning emitted for unknown keys"] --> G
G{"dep.is_self_defined?"}
G -- "yes: registry=False" --> H["MCPIntegrator._build_self_defined_info(dep)\nbuild synthetic server_info dict"]
G -- "no: overlay dep" --> I["MCPIntegrator._apply_overlay(cache, dep)\nmutate cached server_info in-place"]
H --> J["if dep.extra:\n server_info['_extra'] = dict(dep.extra)\nnote: hasattr guard always True for MCPDependency"]
I --> J
J --> K["adapter._format_server_config(server_info, ...)"]
K --> L{"server_info branch"}
L -- "_raw_stdio" --> M["build command+args\n_merge_extra(config, server_info)\nreturn config"]
L -- "remotes" --> N["build url+headers\n_merge_extra(config, server_info)\nreturn config"]
L -- "packages" --> O["build command+args from package\n_merge_extra(config, server_info)\nreturn config"]
M --> P
N --> P
O --> P
P["base.py: _merge_extra()\nfor k,v in _extra.items():\n if k not in config: config[k] = v\nall callers ignore return value"] --> Q
Q["write target manifest\n.mcp.json: oauth block appears at top level"]
Recommendation
Ship after three pre-merge text changes land in this PR: (1) replace [!] with _rich_info and rewrite the warning body to state that keys will appear verbatim in target manifests; (2) add CHANGELOG [Unreleased] entry for the behavioral change; (3) add one-sentence pack safety advisory to manifest-schema.md. The two structural post-merge follow-ups -- per-adapter extra key allowlist for oauth broadcast and integration-with-fixtures pipeline test -- should be filed as issues before the next minor release. The feature is architecturally sound, closes a real user-facing regression, and is unit-tested at every pipeline link.
Full per-persona findings
Python Architect
-
[recommended] hasattr(dep, 'extra') guard is always True -- dep is always MCPDependency; replace with type annotation at
src/apm_cli/integration/mcp_integrator.py:350
_build_self_defined_info(dep) and _apply_overlay(cache, dep) both accept an untyped dep param. Every call site passes a concrete MCPDependency which always declares extra as a dataclass field. The hasattr guard is dead code and misleads future readers.
Suggested:def _build_self_defined_info(dep: MCPDependency) -> dict: ... if dep.extra: info['_extra'] = dict(dep.extra). Same for _apply_overlay. -
[nit] _merge_extra mutates config in-place AND returns the same dict; all call sites discard the return at
src/apm_cli/adapters/client/base.py:132
Returning a mutated argument signals fluent/functional style. Every adapter calls it as a void statement. Removereturn config; change return annotation to-> None. -
[nit] explicit extra: block silently wins over same-named auto-captured unknown keys in from_dict() at
src/apm_cli/models/dependency/mcp.py:83
from_dict() merges asextra = {**unknown_keys, **explicit_extra}. Undocumented. Add comment at merge site.
CLI Logging Expert
-
[recommended] from_dict docstring still says keys are 'dropped' -- directly contradicts the new preserved-in-extra behavior at
src/apm_cli/models/dependency/mcp.py
Line 71 reads "Unknown keys are dropped with a warning naming each discarded key." Developers reading this docstring get false information about the method contract.
Suggested: Update to: "Unknown keys are collected into dep.extra with a warning naming each preserved key; explicit extra: blocks are merged in without a warning." -
[recommended]
[!]warning symbol is now wrong -- passthrough is working-as-intended, not a data-loss alert atsrc/apm_cli/models/dependency/mcp.py
The old 'dropped' behavior justified[!]. Now keys are intentionally preserved. Keeping[!]means users copying the documented example see a yellow alarm on every command invocation.
Suggested: Replace_rich_warning(..., symbol='warning')with_rich_info(..., symbol='info')at line 87-90. Or gate behind--verbose. -
[nit] "preserved in extra" names the mechanism but not the user-visible outcome at
src/apm_cli/models/dependency/mcp.py
User cannot confirm from this message that their oauth block will appear in the generated .mcp.json.
Suggested: "MCP dependency '{safe_name}': unknown key(s) will pass through verbatim to the generated target manifest: {safe_keys}" -
[nit] Warning fires on every apm.yml parse, not only apm install -- repeated noise across all commands at
src/apm_cli/models/dependency/mcp.py
MCPDependency.from_dict is invoked whenever any command loads apm.yml (list, run, plan, etc.). A project with oauth keys sees the message on every APM invocation.
Suggested: Emit only during apm install via CommandLogger, or gate on--verbose.
DevX UX Expert
-
[recommended] Warning fires on the DOCUMENTED top-level oauth: pattern, implying user error where none exists at
docs/src/content/docs/consumer/install-mcp-servers.md
The install guide shows oauth: at top-level as supported config. Yet the code warns "unknown key(s) preserved in extra: oauth". A user copying the documented example gets a yellow alarm on first run.
Suggested: (a) Downgrade warning to debug/verbose-only now that passthrough is intentional, or (b) add a callout in docs noting the expected warning and confirming the key will appear in the output. -
[recommended] "preserved in extra" warning misleads user about the output manifest shape at
src/apm_cli/models/dependency/mcp.py
dep.extra is stored internally, then _merge_extra() flattens it back to top-level in .mcp.json -- oauth appears alongside name/transport/url, NOT nested under an 'extra' key. Users who see "preserved in extra" will search for extra.oauth in their output file.
Suggested: "MCP dependency '{safe_name}': unknown key(s) will appear verbatim in target manifests: {safe_keys}" -
[recommended] apm pack credential stripping for extra keys is noted as out-of-scope but undisclosed to users in docs at
docs/src/content/docs/reference/manifest-schema.md
A user who puts clientSecret in the oauth block will have that secret packed unredacted. The PR makes this more likely by documenting the feature.
Suggested: Add: "Note: extra keys are included verbatim in packed bundles. Do not place secrets or tokens in extra fields -- use env: with ${VAR} references instead." -
[nit] from_dict docstring still says "Unknown keys are dropped" -- now factually wrong at
src/apm_cli/models/dependency/mcp.py -
[nit] Explicit extra: block is a silent alternative path not documented or discoverable at
docs/src/content/docs/reference/manifest-schema.md
Users can write oauth: at top-level (warning fires) orextra: { oauth: ... }(no warning, keys preserved). The no-warning path and its different merge semantics are undocumented.
Supply Chain Security Expert
-
[recommended] apm pack credential sanitizer does not strip opaque extra blocks like oauth; non-credential-named sub-keys survive into plugin.json verbatim at
src/apm_cli/core/plugin_manifest.py
_sanitize_value() strips keys whose normalized names contain token/secret/password/etc. But 'oauth' normalizes to 'oauth' (no match) and 'client_id' normalizes to 'clientid' (no match). A short or non-standard secret value would survive pack.
Suggested: Add 'oauth' to _SENSITIVE_MCP_KEY_NAMES so the entire block is stripped from pack output. Or document that extra blocks are intentionally retained and add a pack-time warning. At minimum, add a regression test. -
[recommended] HTTP/SSE server configs do not set 'command';
extra: {command: evil_bin}would be injected into those configs atsrc/apm_cli/adapters/client/base.py
_merge_extra adds keys only when absent from config. HTTP/SSE adapters set url/headers but not command. Consumer-controlled only, but the injection is silent and undocumented.
Suggested: Warn in _merge_extra when extra contains transport-semantic fields (command, args, url) inconsistent with the adapter config's transport type. -
[nit] extra: dict[str, Any] has no JSON-serializability check; YAML timestamps produce opaque TypeError at JSON write time at
src/apm_cli/models/dependency/mcp.py
Suggested: Addtry: json.dumps(extra) except (TypeError, ValueError) as e: raise ValueError(...)after building extra in from_dict.
OSS Growth Hacker
-
[recommended] CHANGELOG.md has no entry for the warn-and-drop to warn-and-preserve behavior change at
CHANGELOG.md
PR feat: warn on unknown keys dropped in MCPDependency.from_dict() #1674 added the warning and is documented; PR Add extra passthrough for harness-specific MCP keys (#1670) #1765 changes the behavior but adds nothing to [Unreleased]. Upgrading users who rely on the old warning text in CI log greps will be silently broken with no changelog signal.
Suggested: Add to [Unreleased] Added: "Unknown MCP dependency keys in apm.yml are now preserved in an extra passthrough dict and round-tripped verbatim into generated target manifests (e.g. Claude Code oauth block for remote-MCP OAuth). Previously they were silently dropped. A parse-time warning names each preserved key. (Add extra passthrough for harness-specific MCP keys (#1670) #1765)" -
[recommended] No in-docs advisory that apm pack does not strip extra keys; users can silently bundle sensitive values at
docs/src/content/docs/reference/manifest-schema.md
The PR body correctly flags pack credential stripping as out of scope, but that note lives only in the PR body. A user who puts a bearer token in extra and runs apm pack will bundle it with zero warning.
Suggested: Append to the passthrough paragraph: "Note: apm pack does not strip extra keys from published artifacts -- do not place secrets, tokens, or client credentials in extra fields if you intend to publish the package." -
[nit] install-mcp-servers.md comment gives no Claude Code scent trail; users searching for Claude Code OAuth setup will not land here at
docs/src/content/docs/consumer/install-mcp-servers.md
Suggested: Change comment to "# Claude Code remote-MCP OAuth (harness-specific extra keys -- see manifest schema Section 4.2.2)" -
[nit] Placeholder
<pre-registered-client-id>has no companion link; users cannot act on this example without external research atdocs/src/content/docs/consumer/install-mcp-servers.md
Auth Expert
-
[recommended] oauth extra block written to .mcp.json in plaintext; client_secret would persist unredacted at
src/apm_cli/adapters/client/base.py
repr correctly redacts extra (shows count only). But _merge_extra writes the raw value dict straight into adapter config, which is JSON-serialised to .mcp.json. APM has no scan gate on the extra dict comparable to _ENV_ASSIGN_SECRET_RE in plugin_manifest.py.
Suggested: Document clearly in the warning emitted for unknown keys that extra values are written verbatim to generated manifests and are not treated as secrets. Optionally apply _ENV_ASSIGN_SECRET_RE / _AUTH_SCHEME_RE heuristics against serialized extra values and emit a warning when a credential-shaped value is detected. -
[recommended] oauth block (Claude Code-specific) propagates to every adapter via _merge_extra; other harnesses receive unknown config at
src/apm_cli/adapters/client/base.py
_merge_extra is called in copilot, claude, cursor, kiro, codex, gemini, and vscode. The oauth block is Claude Code-specific but all adapters write it. A future harness that interprets oauth differently would receive the Claude Code client config with no gate.
Suggested: Add per-adapter extra key allowlist or a passthrough_keys field on MCPClientAdapter (empty by default, overridden by ClaudeClientAdapter to include 'oauth'). At minimum, document the broadcast behaviour so users know oauth will appear in every generated manifest. -
[nit] Explicit extra: block allows injecting known auth keys (env, headers) bypassing CRLF/type validation at
src/apm_cli/models/dependency/mcp.py
env and headers cannot reach dep.extra from top-level apm.yml keys. But they CAN be nested inside an explicit extra: dict. _merge_extra then injects them if the key is absent from config. For a registry remote server, env is not set in config, soextra: {env: {FOO: value}}would succeed bypassing the dep.env CRLF-check path.
Suggested: Strip known auth/config keys (env, headers) from the explicit extra: block during from_dict parsing, or apply the same CRLF + type validation. -
[nit] AuthResolver and GitHubTokenManager have zero interaction with the extra dict -- no auth bypass at
src/apm_cli/core/auth.py
Confirming the auth surface is clean for reviewers. No action needed.
Doc Writer
-
[recommended] install-mcp-servers.md intro says "Three forms are valid" but now shows four numbered examples at
docs/src/content/docs/consumer/install-mcp-servers.md
Line 23 reads "Three forms are valid -- pick the one that matches the source you have:" but the code block has a 'Add ARM64 Linux support to CI/CD pipeline #4' numbered entry. Factual inconsistency.
Suggested: Change to "Three forms are valid; any self-defined server also accepts harness-specific extra keys (see manifest schema):" and fold example 4 into a note after example 3. -
[recommended] install-mcp-servers.md example 4 has no prose and no cross-link to the passthrough spec at
docs/src/content/docs/consumer/install-mcp-servers.md
The oauth example appears with only a YAML comment. No sentence explains that this works for any unknown key, that a warning fires, or where to find the full spec.
Suggested: Add 1-2 sentences after the code block: "Any key not in the field table above is preserved and written verbatim into generated target manifests. See the manifest schema reference for collision rules and the explicit extra: block alternative." -
[recommended] The explicit 'extra:' named YAML key is undocumented -- it is a separate, warning-free API at
docs/src/content/docs/reference/manifest-schema.md
'extra' is in _KNOWN_DICT_KEYS;extra: {oauth: {...}}is a supported first-class input that does NOT trigger a warning. Current docs show only top-level unknown keys (which DO trigger the warning). Users copying the example will see a warning on every install; users who know the explicit extra: form avoid it.
Suggested: Add 'extra' as a row in the MCP dependency field table with description: "Explicit passthrough block; contents merged with any unknown top-level keys and round-tripped verbatim. Does not emit a warning. Known fields always win on collision." -
[recommended] Warning behavior documented but no-warning path via explicit extra: is missing, creating asymmetric expectations at
docs/src/content/docs/reference/manifest-schema.md
manifest-schema.md states "A warning is emitted at parse time naming each non-standard key." This is only true for top-level unknown keys -- the explicit extra: block flows with zero warnings. Users cannot know (a) whether the warning is expected, (b) that it can be suppressed.
Suggested: Amend to: "When keys appear directly at the top level (not under an explicit extra: block) a warning is emitted at parse time. To suppress the warning, nest the keys under an explicit extra: block instead." -
[nit] Duplicate oauth example in packages/apm-guide dependencies.md violates state-once principle at
packages/apm-guide/.apm/skills/apm-usage/dependencies.md
Same slack/oauth block appears verbatim in both install-mcp-servers.md and dependencies.md.
Suggested: Replace the full slack/oauth block in dependencies.md with a one-line comment pointing to install-mcp-servers.md or manifest-schema.md.
Test Coverage Expert
-
[recommended] No integration-with-fixtures test for the full extra passthrough pipeline: apm.yml -> MCPIntegrator -> adapter -> manifest file at
tests/integration/test_mcp_integrator_install_flow.py
Unit tests exist for each link in isolation, but no test exercises the complete chain with real objects (no mocks). Floor tier for cross-module integration is integration-with-fixtures. Probed: grep -rn '_merge_extra|_extra|oauth' tests/integration/ -- zero matches related to this feature.
Suggested: Add a hermetic fixture test: construct MCPDependency(name='slack', transport='http', url='...', extra={'oauth': {'clientId': 'abc', 'callbackPort': 3118}}), call _build_self_defined_info(dep), call adapter._format_server_config(info), assertconfig['oauth'] == {'clientId': 'abc', 'callbackPort': 3118}.
Proof (missing at integration-with-fixtures):tests/integration/test_mcp_integrator_install_flow.py::test_self_defined_http_oauth_extra_survives_to_adapter_config-- proves: An oauth block declared in apm.yml survives the full MCPIntegrator->adapter pipeline [portability-by-manifest, devx]
assert config['oauth'] == {'clientId': 'abc', 'callbackPort': 3118} -
[recommended] Six adapter _format_server_config methods have 17 _merge_extra call sites with no per-adapter test passing _extra in server_info at
tests/unit/test_copilot_adapter.py
Probed: grep -rn '_extra' tests/unit/test_copilot_adapter.py tests/unit/test_cursor_mcp.py -- zero matches. TestAdapterMergeExtra proves the base class method works in isolation but does not prove each adapter calls it. A PR removing one call site would cause silent regression.
Suggested: In each adapter's test file, add one scenario that passes _extra in server_info and asserts the key appears in the returned config.
Proof (missing at unit):tests/unit/test_copilot_adapter.py::test_format_server_config_raw_stdio_includes_extra_keys-- proves: CopilotClientAdapter._format_server_config merges _extra into config on the raw_stdio path [devx, multi-harness-support]
assert config['oauth'] == {'clientId': 'abc'} -
[recommended] PR body lacks a Scenario Evidence table; no user-words scenario mapped to APM principle with verified test path
The PR has a well-written Validation section listing test counts and lint results but no scenario-to-principle mapping. Without this table the panel cannot audit whether all changed behavioral surfaces are covered.
Suggested: Add a Scenario Evidence table with rows: (1) User declares oauth: in apm.yml; manifest contains oauth -> DevX; (2) Extra key cannot override type or url -> Secure by default; (3) Registry-resolved dep with extra overlay: _apply_overlay sets _extra -> Portability.
Performance Expert -- inactive
All 13 changed files are in src/apm_cli/models/, src/apm_cli/integration/, src/apm_cli/adapters/client/, docs/, and tests/ -- none touch cache layout, dependency resolution, install phases, git transport, or any materialization hot path.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Generated by PR Review Panel for issue #1765 · sonnet46 20.9M · ◷
github-actions
Bot
removed
the
panel-review
TL;DR
MCPDependencynow captures unknown keys fromapm.ymlinto a genericextradict and round-trips them into generated target manifests, enabling harness-specific configuration (e.g. Claude Code'soauthblock) to survive the install pipeline.Problem
When a user declares harness-specific keys (like
oauthfor Claude Code remote-MCP OAuth) in anapm.ymlMCP dependency entry, those keys are silently dropped during parsing becauseMCPDependency.from_dict()only recognises a fixed set of known fields. The warning was already landed, but the keys were still discarded -- they never appeared in the generated.mcp.json.Approach
A generic
extra: dict[str, Any] | Nonefield onMCPDependencycaptures all unknown keys and passes them through the full data flow:Shadow prevention: extra keys never override known fields or adapter-set keys. In both
to_dict()and_merge_extra(), known keys take precedence.Changes
src/apm_cli/models/dependency/mcp.pyextrafield, updatedfrom_dict(),to_dict(),__repr__src/apm_cli/integration/mcp_integrator.py_extrapassthrough in_build_self_defined_info()src/apm_cli/adapters/client/base.py_merge_extra()static methodsrc/apm_cli/adapters/client/{copilot,codex,cursor,gemini,kiro,vscode}.py_merge_extra()calls at all return pointstests/unit/test_mcp_from_dict_unknown_keys.pydocs/src/content/docs/reference/manifest-schema.mddocs/src/content/docs/consumer/install-mcp-servers.mdoauthextra keypackages/apm-guide/.apm/skills/apm-usage/dependencies.mdoauthextra keyValidation
Out of scope
apm packcredential stripping forextrakeys (follow-up)claude:,vscode:) -- genericextrais simpler and sufficientCloses #1670