MCP extra passthrough: tighten to explicit `extra:`-only (fail-closed) with deprecation path

[danielmeppiel]

Context

Follow-up to #1670 / PR #1765, which added a harness-specific extra passthrough to MCPDependency so keys like Claude Code's remote-MCP oauth block (clientId / callbackPort) survive apm.yml -> generated .mcp.json rendering instead of being silently dropped.

PR #1765 ships the bare-key ergonomics: any unknown top-level key on a self-defined MCP dependency is auto-captured into extra (matching the documented oauth: examples). It also adds a modeled-field-name denylist (a passthrough value can never shadow/redirect name/transport/command/url/headers/env/... or the Codex http_headers alias) so the concrete injection edge is closed.

The deferred tightening

The supply-chain-security review lens flagged that auto-capturing bare unknown keys is fail-open: a transitive dependency package's apm.yml is parsed straight through from_dict, and the prior contract dropped unknown keys (fail-closed) whereas the new contract preserves them. The denylist closes the high-severity shadow/redirect edge, but the residual posture question remains: should passthrough require an explicit extra: block and drop bare unknown keys with a warning (fail-closed), so only keys the author deliberately nests under extra: flow through?

This was deliberately sequenced out of #1765 by maintainer decision (Option B + denylist) because it is a user-facing authoring-contract change (the documented examples use bare top-level oauth:), i.e. a breaking change that needs a deprecation path rather than a silent flip.

Proposed scope (this issue)

1. Introduce explicit extra: as the only sanctioned passthrough surface.
2. Deprecation window: keep capturing bare unknown keys, but emit a deprecation warning instructing authors to nest them under extra:. After the window, drop bare unknown keys (restore fail-closed drop-and-warn).
3. Rewrite the three doc examples (install-mcp-servers.md, reference/manifest-schema.md, apm-usage/dependencies.md) to nest oauth: under extra:.
4. Update test_mcp_from_dict_unknown_keys.py expectations accordingly.

Also captured here (deferred from #1765, non-blocking)

• Per-harness scoping: extra: { copilot: {...}, codex: {...} } so a Claude-specific block is not broadcast to every target.
• Secret-shaped-value scan: warn when an extra value looks like a token/secret being written to a world-readable config.
• to_dict collision debug-warn: emit a debug log when an extra key shadows a known field during re-serialization.

Why not now

Per the PR #1765 maintainer decision, #1765 keeps bare-key ergonomics + denylist (closes the concrete trust-boundary edge) and defers this fail-closed contract tightening here so the posture change is explicit, deprecation-gated, and reviewable on its own.

/cc the #1670 thread.

[github-actions]

Triage decision

needs-design

Proposed labels

theme/security
area/mcp-config
area/mcp-trust
type/feature
status/needs-design
status/triaged
breaking-change

Milestone

null

Suggested next action

Write a mini-design doc (or inline comments on this issue) covering: (1) deprecation window timeline and warning UX, (2) the per-harness extra: { copilot: {...} } schema definition, and (3) the secret-detection heuristic, then link it back here.

Suggested issue comment

Thanks for the detailed write-up on the deprecation path for the `extra:` tightening!

The direction is well-grounded: moving to explicit `extra:`-only (fail-closed) is the right supply-chain posture, and the deprecation window approach balances the authoring-contract change against a smooth migration for authors currently using bare `oauth:` syntax.

Before implementation lands, three design decisions need to be documented:

1. **Deprecation window and warning UX**: Which apm versions carry the warning-only mode? What does the deprecation warning message say, and where does it surface (`apm install` output, `apm audit`, or both)?
2. **Per-harness `extra:` schema**: `extra: { copilot: {...}, codex: {...} }` needs a formal schema decision -- is the harness key a string matching a known target name, or is it freeform? How does `to_dict` handle unknown harness names?
3. **Secret-detection heuristic**: What patterns trigger the "looks like a token" warning? This needs a clear definition to avoid false positives on legitimate API-key-like values.

Once those three are documented (a short design note or inline comments on this issue work fine), the implementation scope becomes deterministic and a PR can land safely. The doc rewrites (`install-mcp-servers.md`, `manifest-schema.md`, `apm-usage/dependencies.md`) should be part of the acceptance criteria for that PR.

Per-lens notes (collapsed)

DevX UX Expert -- User-Need Reviewer

The request maps directly to the area/mcp-config authoring surface: authors who write oauth: as a bare top-level key in an MCP dependency block need a clear migration path when the contract tightens. The deprecation window (warn -> drop) matches the npm deprecation and pip removed-feature patterns. The extra: nesting is a one-line change per apm.yml file; the UX cost is low given the security benefit. The three doc files named in the body are the canonical surfaces that must be updated before any user encounters the new behavior.

Supply Chain Security Expert -- Risk-Surface Reviewer

This issue tightens a fail-open surface introduced in #1765: bare unknown top-level keys on MCPDependency being auto-captured into passthrough. Moving to explicit extra:-only restores the fail-closed default, preventing transitive dependency packages from silently passing through arbitrary unknown keys to MCP server config. The existing denylist (#1765) blocks shadow/redirect of known fields; this change blocks silent passthrough of arbitrary unknowns from transitive sources. theme/security and area/mcp-trust are the right labels. The per-harness scoping sub-item and secret-shaped-value scan are additive improvements. No escalation beyond existing labels needed.

OSS Growth Hacker -- Contributor-Tone Reviewer

Not activated -- danielmeppiel is a COLLABORATOR with deep context on the issue history; standard maintainer-to-maintainer tone is appropriate with no first-timer adjustment.

Python Architect -- Architecture Reviewer

The core tightening (fail-open -> fail-closed with deprecation warning) is bounded to MCPDependency.from_dict() plus the three doc files and test updates. The per-harness scoping sub-item is an additive schema change that needs a formal field-shape decision before implementation. The secret-detection heuristic is also design-dependent (false-positive risk on legitimate values). The current status/needs-design is correct: the core tightening is independently implementable once the three sub-item designs are documented. The issue could be split for faster progress on the core tightening, but that is a maintainer call.

Doc Writer -- Documentation Reviewer

Three doc files are explicitly named in the acceptance criteria: install-mcp-servers.md, reference/manifest-schema.md, and apm-usage/dependencies.md. All three will be factually wrong the moment the deprecation warning appears. The implementing PR MUST update all three in the same diff. An area/docs-site secondary label would be appropriate but is omitted here to stay within the 6-label limit (existing maintainer-applied labels already fill it); the doc requirement is noted in the suggested comment instead.

APM CEO -- Triage Arbiter

The issue is correctly labeled by the maintainer; status/needs-design holds because the deprecation window timeline, per-harness schema, and secret heuristic are unresolved design decisions. Adding status/triaged closes the sweep loop. The suggested comment asks for exactly the three documented design decisions needed before implementation, which is the right unblocking action. No milestone (needs-design issues are milestone-free per rubric). No priority per rubric.

{
  "decision": "needs-design",
  "decision_detail": "Three sub-items require design documentation: deprecation window timeline and warning UX, per-harness extra: schema definition, and secret-detection heuristic.",
  "theme": "theme/security",
  "areas": ["area/mcp-config", "area/mcp-trust"],
  "type": "type/feature",
  "status": "status/needs-design",
  "priority": null,
  "preserved_labels": ["breaking-change"],
  "milestone": null,
  "next_action": "Write a mini-design doc covering the deprecation window timeline, per-harness extra: schema, and secret-detection heuristic, then link it back here.",
  "comment_markdown": "Thanks for the detailed write-up on the deprecation path for the `extra:` tightening! The direction is well-grounded: moving to explicit `extra:`-only (fail-closed) is the right supply-chain posture, and the deprecation window approach balances the authoring-contract change against a smooth migration for authors currently using bare `oauth:` syntax. Before implementation lands, three design decisions need to be documented: (1) Deprecation window and warning UX -- which apm versions carry the warning-only mode and what does the message say? (2) Per-harness `extra:` schema -- is the harness key a known target name or freeform? (3) Secret-detection heuristic -- what patterns trigger the warning to avoid false positives? Once those three are documented, the implementation scope is deterministic. Doc rewrites for install-mcp-servers.md, manifest-schema.md, and apm-usage/dependencies.md are part of the acceptance criteria."
}

---

Triage status: agentic proposal pending human ratification.
Silence is approval. Maintainers can:

• Override any label or milestone above by editing it directly --
  human edits are authoritative and will not be reverted on
  subsequent runs.
• Re-trigger triage by applying the status/needs-triage label, or
  by removing status/triaged to enroll the issue in the next
  daily sweep.

Posted by the Triage Panel workflow. See .apm/skills/apm-triage-panel for the panel skill.

Generated by Triage Panel · sonnet46 3.8M · ◷