fix(mcp): resolve self-defined stdio env ${VAR} placeholders at install time (closes #1963)#1966
fix(mcp): resolve self-defined stdio env ${VAR} placeholders at install time (closes #1963)#1966danielmeppiel wants to merge 3 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes install-time resolution of ${VAR} placeholders for self-defined stdio MCP server env blocks, ensuring Claude Code and Codex receive literal env values rather than ${VAR} strings caused by env_overrides shadowing os.environ.
Changes:
- Adjusts self-defined MCP install flow so stdio
envis resolved via the adapter pipeline (not reused asenv_overrides). - Adds a regression test covering both Claude (
.mcp.json) and Codex (.codex/config.toml) output. - Updates documentation/guide text and adds a changelog entry describing the behavior.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/install/test_mcp_integrator_install_flow.py | Adds regression test asserting ${MY_TOKEN} resolves for self-defined stdio servers in Claude + Codex. |
| src/apm_cli/integration/mcp_integrator_install.py | Stops passing self-defined stdio dep.env as env_overrides to avoid shadowing os.environ. |
| packages/apm-guide/.apm/skills/apm-usage/dependencies.md | Updates placeholder behavior notes (currently inconsistent with manifest-schema docs; see comments). |
| docs/src/content/docs/producer/author-primitives/mcp-as-primitive.md | Updates narrative on placeholder handling; suggested wording tweak to reflect prompting behavior. |
| CHANGELOG.md | Adds Unreleased entry (needs PR number reference update; see comment). |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Low
| # Cursor/Windsurf/OpenCode/Claude/Gemini: resolved at install time. | ||
| # Codex: passed through unchanged. | ||
| # Codex remote headers: passed through unchanged. | ||
| # Codex self-defined stdio env: resolved at install time. |
| Headers and env values are never shell-expanded by APM. For harnesses | ||
| that support runtime env placeholders, APM preserves the placeholder so | ||
| the harness resolves it when the server starts or the request is made. | ||
| For harnesses that require literal values, APM resolves `${VAR}` from | ||
| the install process environment and leaves unresolved placeholders | ||
| unchanged. Keep the real secret in the consumer's environment (or their | ||
| secret manager). |
|
|
||
| ### Fixed | ||
|
|
||
| - Self-defined stdio MCP env placeholders now resolve from the install process environment for Claude Code and Codex, matching remote header handling. (#1963) |
Fold review follow-ups for PR #1966 by clarifying the stdio-env docs and preserving the non-stdio self-defined env-overrides branch with a regression-trap test. The test prevents future changes from collapsing the branch that remote transports still rely on. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel: ship_now
cc @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review. This PR resolves a real-world bug where Panel signals converge cleanly: python-architect finds the change structurally sound, supply-chain-security and auth see no new surface, CLI logging is unaffected, and test coverage now validates both the stdio and non-stdio branches with mutation-break evidence. No specialist raised a remaining in-scope finding after the fold. Aligned with: portable_by_manifest: Ensures MCP server definitions in apm.yml resolve env placeholders consistently across adapters; multi_harness_multi_host: Fixes adapter-specific resolution for Claude Code and Codex; pragmatic_as_npm: Users expect env vars to just work in server definitions. Growth signal. This removes a first-run trust issue for Claude Code and Codex MCP authors. Panel summary
RecommendationAll in-scope findings have been folded, tests cover both branches, and mutation-break gates passed. No follow-ups remain. Merge at will after maintainer review. Folded in this run
Copilot signals reviewed
Regression-trap evidence (mutation-break gate)
Lint contract
CI
Mergeability statusCaptured from
Convergence1 outer iteration; 2 Copilot fetch rounds. Final panel stance: Ready for maintainer review. |
Fold Copilot's changelog reference signal by using the PR number in the Unreleased entry while keeping the issue link in the PR body. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Why
Self-defined MCP transports were inconsistent: HTTP headers already resolved
${VAR}placeholders from the install process environment for literal-only harnesses, but self-defined stdioenvvalues were passed back asenv_overrides, causing the authored placeholder to shadow the real process environment. That made Claude Code and Codex write${VAR}verbatim and break authenticated servers, a P6 reliability issue.What changed
envblocks asenv_overrides; the adapter now resolves_raw_stdio.envthrough the same placeholder pipeline used by headers.${MY_TOKEN}resolves while literal env values remain unchanged.How to test
Exact repro:
Validation run:
Mutation-break gate: removed
self_defined_env = {} if transport_label == "stdio" else dep.env or {}and restoredself_defined_env = dep.env or {}; the new regression test failed for both Claude and Codex with${MY_TOKEN}written verbatim (failed_without_guard=true).Closes #1963