Skip to content

fix(mcp): resolve self-defined stdio env ${VAR} placeholders at install time (closes #1963)#1966

Open
danielmeppiel wants to merge 3 commits into
mainfrom
fix/mcp-stdio-env-resolution
Open

fix(mcp): resolve self-defined stdio env ${VAR} placeholders at install time (closes #1963)#1966
danielmeppiel wants to merge 3 commits into
mainfrom
fix/mcp-stdio-env-resolution

Conversation

@danielmeppiel

Copy link
Copy Markdown
Collaborator

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 stdio env values were passed back as env_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

  • Stopped passing self-defined stdio env blocks as env_overrides; the adapter now resolves _raw_stdio.env through the same placeholder pipeline used by headers.
  • Added a regression test that installs the same self-defined stdio server into Claude Code and Codex and asserts ${MY_TOKEN} resolves while literal env values remain unchanged.
  • Updated docs, the APM usage guide, and the changelog entry for the clarified behavior.

How to test

Exact repro:

rm -rf .scratch-1963
mkdir -p .scratch-1963/.claude .scratch-1963/.codex
cat > .scratch-1963/apm.yml <<'YAML'
name: repro-1963
version: 0.0.0
dependencies:
  mcp:
    - name: env-demo
      registry: false
      transport: stdio
      command: npx
      args: ["-y", "example-mcp"]
      env:
        MY_TOKEN: "${MY_TOKEN}"
        LITERAL_VALUE: "literal-value"
    - name: http-demo
      registry: false
      transport: http
      url: https://example.com/mcp
      headers:
        X-Token: "${MY_TOKEN}"
YAML
cd .scratch-1963
MY_TOKEN="repro-secret-not-a-real-token" uv run --project .. apm install --only mcp --target claude
cat .mcp.json
MY_TOKEN="repro-secret-not-a-real-token" uv run --project .. apm install --only mcp --target codex
cat .codex/config.toml

Validation run:

uv run --extra dev pytest tests/unit/install/test_mcp_integrator_install_flow.py::TestRunMcpInstallSelfDefined::test_self_defined_stdio_env_placeholders_resolve_from_process_env -q
uv run --extra dev pytest tests/unit/install/test_mcp_integrator_install_flow.py tests/unit/test_claude_mcp.py tests/unit/test_codex_adapter_phase3.py tests/test_codex_empty_string_and_defaults.py -q
uv run --extra dev ruff check src/ tests/
uv run --extra dev ruff format --check src/ tests/

Mutation-break gate: removed self_defined_env = {} if transport_label == "stdio" else dep.env or {} and restored self_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

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 30, 2026 13:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 env is resolved via the adapter pipeline (not reused as env_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
Comment on lines 371 to +373
# 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.
Comment on lines +116 to +122
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).
Comment thread CHANGELOG.md Outdated

### 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>
@danielmeppiel

danielmeppiel commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

APM Review Panel: ship_now

Fixes self-defined stdio MCP env placeholder resolution for Claude Code and Codex by eliminating env_overrides shadowing and leveraging existing adapter resolution.

cc @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

This PR resolves a real-world bug where ${VAR} placeholders in stdio MCP server definitions were not resolved when the adapter already handles environment variable expansion natively. The fix is minimal and precise: avoid injecting env_overrides that would shadow the adapter's own resolution logic. The shepherd cycle folded all panel and Copilot findings in commits 9ca597c57 and 122d237c: changelog entry clarified, code comment improved, a non-stdio branch test added, and the changelog now references the PR number.

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

Persona B R N Takeaway
Python architect 0 0 1 Minimal data-flow fix uses the existing adapter resolution path; comment clarity was folded.
CLI logging expert 0 0 0 No CLI output or logging surface touched.
DevX UX expert 0 0 1 Behavior now matches user expectation for ${VAR} stdio env indirection; docs wording was folded.
Supply chain security expert 0 0 0 The fix improves secret placeholder handling without adding a token leak or new trust boundary.
OSS growth hacker 0 0 0 Closes a sharp MCP authoring reliability gap for Claude Code and Codex users.
Auth expert 0 0 0 Correct auth-adjacent behavior: stdio token placeholders now resolve from process environment.
Doc writer 0 1 0 Docs are aligned after the folded changelog and harness-bucket wording changes.
Test coverage expert 0 0 1 Regression coverage now covers the stdio fix and the non-stdio else branch.

B = highest-signal correctness/security/architecture findings, R = recommended, N = nits. Counts are advisory; the maintainer ships.

Recommendation

All 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

  • review 4600591422 -- LEGIT: stale summary noted the changelog PR-number reference issue; folded by changing (#1963) to (#1966) in 122d237c.

Regression-trap evidence (mutation-break gate)

  • tests/unit/install/test_mcp_integrator_install_flow.py::TestRunMcpInstallSelfDefined::test_self_defined_stdio_env_placeholders_resolve_from_process_env -- deleted the stdio env-overrides guard; test FAILED as expected; guard restored.
  • tests/unit/install/test_mcp_integrator_install_flow.py::TestRunMcpInstallSelfDefined::test_self_defined_non_stdio_env_remains_env_overrides -- collapsed the non-stdio else branch to {}; test FAILED as expected; guard restored.

Lint contract

uv run --extra dev ruff check src/ tests/ and uv run --extra dev ruff format --check src/ tests/ both passed before each push. Full local lint mirror also passed before the code/test push: pylint R0801 and scripts/lint-auth-signals.sh.

CI

gh pr checks 1966 --repo microsoft/apm --watch observed all checks green on head 122d237c3320c0ba68f2a67c454ea37bf9dac40a (CI run 28447213285; Lint, Build & Test shards, Coverage Combine, APM Self-Check, PR Binary Smoke all passed; CodeQL, Merge Gate, NOTICE Drift Check, Spec conformance, docs build, and license/cla passed or skipped where expected). CI recovery iterations: 0.

Mergeability status

Captured from gh pr view 1966 --json mergeable,mergeStateStatus,statusCheckRollup immediately after the last push of this run.

PR head SHA CEO stance iters folds defers Copilot rounds CI mergeable mergeStateStatus notes
#1966 122d237 ship_now 1 5 0 2 green MERGEABLE BLOCKED pending required review

Convergence

1 outer iteration; 2 Copilot fetch rounds. Final panel stance: ship_now.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants