fix(copilot): translate env-var placeholders instead of baking secrets to disk (#1152)

[danielmeppiel]

fix(copilot): translate env-var placeholders instead of baking secrets to disk (#1152)

TL;DR

apm install --target copilot previously resolved env-var placeholders (${env:VAR}, ${VAR}, legacy <VAR>) at install time and wrote the literal secret values into ~/.copilot/mcp-config.json. Copilot CLI natively supports runtime substitution, so this PR translates placeholders to Copilot's native ${VAR} form and lets Copilot resolve them at server-start. Secrets no longer touch disk. Sibling adapters (Cursor, Claude, Windsurf, OpenCode, Gemini) are explicitly opted out of the new behavior to keep this PR strictly scoped — they keep their current resolve-time semantics.

Important

Closes #1152. After this lands, anyone whose ~/.copilot/mcp-config.json was written by a prior APM version should rotate any secrets that were baked there and re-run apm install --target copilot. APM emits a one-shot security-improvement notice the first time it overwrites baked literals.

Problem (WHY)

• Secrets-on-disk vulnerability. _resolve_env_variable, _resolve_environment_variables, and _resolve_variable_placeholders all read os.environ and substituted literal values into the config file written to ~/.copilot/. A token in ${env:GITHUB_TOKEN} landed as Bearer ghp_... on disk.
• Unnecessary work. Copilot CLI's MCP config natively supports runtime substitution — ${VAR}, $VAR, ${VAR:-default} — per the Copilot CLI MCP docs. Resolving at install time duplicates the runtime's job and removes the operator's ability to rotate secrets without reinstalling.
• Regression introduced by [fix] translate bare ${VAR} env-var refs in self-defined MCP server headers (#944) #947. That PR was authored on the (incorrect) premise that "Copilot CLI mcp-config.json has no native interpolation". The Copilot docs contradict it.
• [!] Silent breakage risk. Five sibling adapters (Cursor, Windsurf, OpenCode, Claude, Gemini) inherit from CopilotClientAdapter. A naive change to the base methods would have flipped their behavior too — and Claude's claude_desktop_config.json does NOT support runtime substitution, so it MUST keep resolving.

Why these matter: APM's security model is "secure by default" — secrets are not stored on disk by APM. The previous behavior violated that contract.

Approach (WHAT)

#	Fix
1	Add a ClassVar flag _supports_runtime_env_substitution on CopilotClientAdapter (set to True for Copilot itself). Five sibling adapters (Cursor, Windsurf, OpenCode, Claude, Gemini) explicitly override the flag to False so this PR cannot silently change their semantics.
2	Translate placeholders via a pure-textual helper that never reads os.environ: ${env:VAR} -> ${VAR}, ${VAR} -> ${VAR}, <VAR> -> ${VAR} (with deprecation warning). ${VAR:-default}, ${input:foo}, $VAR, ${{ secrets.X }} pass through unchanged.
3	Apply translation across all three Copilot config surfaces: HTTP headers, stdio env block (both list-of-dicts AND dict shapes), and stdio args. APM template vars like {runtime_var} still resolve at install time (they're an APM concept, not env vars).
4	Aggregate post-install diagnostics into a single end-of-run summary: unset env-var names, security-upgrade notice (one-shot when overwriting baked literals), and <VAR> deprecation warnings - all keyed on names only, never values.
5	Remove the install-time interactive prompt for missing env vars on the Copilot path; with runtime substitution there is nothing to prompt for.

Implementation (HOW)

• src/apm_cli/adapters/client/copilot.py - Adds module-level helpers _translate_env_placeholder, _has_env_placeholder, _extract_legacy_angle_vars. CopilotClientAdapter sets _supports_runtime_env_substitution = True plus four ClassVar aggregation buckets (_legacy_angle_offenders_by_server, _security_upgraded_keys, _unset_env_keys_by_server, _install_run_summary_emitted). Refactors _resolve_env_variable, _resolve_environment_variables (now handles dict-shaped stdio env blocks too), and _resolve_variable_placeholders to branch on the flag. Adds _emit_install_summary, classmethod emit_install_run_summary, and reset_install_run_state. _collect_previously_baked_keys now returns (env_keys, headers_were_baked) so the security-upgrade detector picks up the case where the only baked literals were HTTP headers (which don't expose env-var names directly).

• src/apm_cli/adapters/client/{cursor,windsurf,opencode,claude,gemini}.py — Each sibling explicitly sets _supports_runtime_env_substitution: ClassVar[bool] = False. This freezes their existing behavior so this PR cannot silently change them. Per-adapter migration to the runtime-translation contract becomes a follow-up issue per adapter (each needs its own runtime-substitution research).

• src/apm_cli/integration/mcp_integrator.py — Calls CopilotClientAdapter.emit_install_run_summary() once after the per-server install loop, gated on runtime == "copilot". The aggregated summary appears between the per-server tree lines and the +- Configured N server(s) close.

• tests/unit/test_copilot_adapter.py — 39 unit tests across five classes: translation table, header path, stdio env-block dict shape (regression trap for the supply-chain finding), inheritance flag (siblings unchanged), and post-install summary aggregation. Each class has setUp/tearDown that calls reset_install_run_state() for isolation.

• tests/integration/test_mcp_env_var_copilot_e2e.py — Three end-to-end tests run real apm install --target copilot subprocesses with secrets exported to the environment, then assert the resulting ~/.copilot/mcp-config.json contains ${VAR} and not the secret literal: HTTP server, self-defined stdio (env + args), and a Cursor regression trap that confirms the sibling-adapter still resolves at install time.

• docs/src/content/docs/guides/dependencies.md, docs/src/content/docs/reference/cli-commands.md, packages/apm-guide/.apm/skills/apm-usage/dependencies.md, CHANGELOG.md — Document the translation contract, post-install diagnostics, and the security-improvement entry.

Diagrams

Legend: how the same apm.yml env-var placeholder flowed onto disk before vs after this PR. Highlighted region shows the new branch where APM stops reading the host environment.

sequenceDiagram
    participant U as User shell
    participant A as apm install
    participant E as os.environ
    participant F as ~/.copilot/mcp-config.json
    participant C as gh copilot

    Note over U,C: BEFORE this PR
    U->>A: apm install --target copilot
    A->>E: read GITHUB_TOKEN
    E-->>A: ghp_secret_value
    A->>F: write Bearer ghp_secret_value
    C->>F: read config at server-start
    F-->>C: Bearer ghp_secret_value

    rect rgb(255, 247, 200)
        Note over U,C: AFTER this PR
        U->>A: apm install --target copilot
        A->>F: write Bearer ${GITHUB_TOKEN}
        Note over A,E: APM never reads os.environ
        C->>F: read config at server-start
        F-->>C: Bearer ${GITHUB_TOKEN}
        C->>E: substitute ${GITHUB_TOKEN}
        E-->>C: ghp_secret_value
    end

Loading

Legend: which adapters opt in to the new translation behavior. Five siblings inherit from CopilotClientAdapter and explicitly opt out so this PR cannot silently change their semantics.

flowchart LR
    subgraph base[Base]
        MCP[MCPClientAdapter]
    end
    subgraph translate[Translate at install time]
        COP[CopilotClientAdapter<br/>_supports_runtime = True]
    end
    subgraph resolve[Resolve at install time, unchanged]
        CUR[CursorClientAdapter]
        WIN[WindsurfClientAdapter]
        OPE[OpenCodeClientAdapter]
        CLA[ClaudeClientAdapter]
        GEM[GeminiClientAdapter]
    end
    MCP --> COP
    MCP --> CDX[CodexClientAdapter<br/>not affected]
    COP --> CUR
    COP --> WIN
    COP --> OPE
    COP --> CLA
    COP --> GEM
    classDef new stroke-dasharray: 5 5,stroke:#0a7;
    class COP new;

Loading

Trade-offs

• Translate, do not resolve. Chose runtime substitution; rejected install-time resolution-with-better-warnings because it preserves the secrets-on-disk failure mode. The cost is a one-time UX shift: users now see ${VAR} in their config and must export the var before gh copilot.
• Strict scope to Copilot. Five sibling adapters keep current resolve-time behavior via the opt-out flag. Rejected fixing them in the same PR because each needs its own runtime-substitution research (Claude in particular MUST keep resolving — claude_desktop_config.json has no native interpolation). Follow-up issues per adapter are the right venue.
• Pre-existing is_github_server literal token baking left in place. The remote-GitHub branch at copilot.py:533-541 still reads a live token from GitHubTokenManager and writes Bearer {token} to disk. That predates this PR and is out of scope; supply-chain review explicitly flagged it as nit-not-blocker. Tracked as follow-up.
• reset_install_run_state only called from tests today. Long-lived processes that invoke install programmatically would accumulate env-var names (never values) in class-level state. Acceptable for the CLI's actual usage; documented as a follow-up.

Benefits

1. Zero secrets on disk in ~/.copilot/mcp-config.json for the env-var path — verified by grep -F over the file in the e2e test.
2. Operator can rotate secrets by export VAR=... and re-run gh copilot; no apm install re-run required.
3. One aggregated post-install summary instead of N per-server warning blobs: security-upgrade notice + unset-var aggregation + <VAR> deprecation, all in one place at end-of-run.
4. Sibling adapters frozen — explicit opt-out flag plus tests/unit/test_copilot_adapter.py::TestCopilotInheritedAdaptersUnchanged makes silent regression on Cursor/Claude/etc. impossible.
5. Legacy <VAR> migration path — translated automatically with a one-shot deprecation warning naming the offending servers, scheduled for removal in v1.0.

Validation

Lint (canonical CI-mirror commands per .apm/instructions/linting.instructions.md):

$ uv run --extra dev ruff check src/ tests/
All checks passed!
$ uv run --extra dev ruff format --check src/ tests/
705 files already formatted

Tests:

$ uv run --extra dev pytest tests/unit/test_copilot_adapter.py tests/integration/test_mcp_env_var_copilot_e2e.py
============================== 40 passed in 8.42s ==============================

Cross-adapter regression check (all 211 inherited adapter tests still green):

$ uv run --extra dev pytest tests/unit/test_copilot_adapter.py tests/unit/test_copilot_runtime.py tests/unit/test_vscode_adapter.py tests/unit/test_cursor_mcp.py tests/unit/test_claude_mcp.py tests/unit/test_windsurf_adapter.py tests/unit/test_opencode_mcp.py tests/unit/test_gemini_mcp.py
============================= 211 passed in 25.18s =============================

Smoke test: real apm install --target copilot with secrets exported, confirming no plaintext leak

$ HOME=/tmp/apm-stdio-debug/home MY_STDIO_TOKEN=secret MY_LEGACY_VAR=legacy \
    APM_NON_INTERACTIVE=1 apm install --target copilot
[>] Installing dependencies from apm.yml...
+- MCP Servers (1)
|  >  test-stdio-server (self-defined, stdio)
|     +- Configuring for Copilot...
  + test-stdio-server

[!] Deprecated: <VAR> placeholder syntax used in 1 server (test-stdio-server). Migrate to ${VAR} in apm.yml. <VAR> support will be removed in v1.0.
|  +  test-stdio-server -> Copilot (configured)
+- Configured 1 server

[*] Installed 1 MCP server in 0.4s.

$ cat ~/.copilot/mcp-config.json
{
  "mcpServers": {
    "test-stdio-server": {
      "type": "local",
      "tools": ["*"],
      "id": "",
      "command": "echo",
      "env": {
        "PRIMARY_TOKEN": "${MY_STDIO_TOKEN}",
        "PREFIXED_TOKEN": "${MY_STDIO_TOKEN}",
        "LEGACY_TOKEN": "${MY_LEGACY_VAR}"
      },
      "args": [
        "--token=${MY_STDIO_TOKEN}",
        "--legacy=${MY_LEGACY_VAR}"
      ]
    }
  }
}

$ grep -E "secret|legacy(-secret)?" ~/.copilot/mcp-config.json
# (no output: neither "secret" nor "legacy" appears as a literal value)

Scenario Evidence

#	Scenario (user promise)	Principle(s)	Test(s) proving it	Type
1	apm install --target copilot does NOT bake secret values into ~/.copilot/mcp-config.json for HTTP servers; runtime placeholders appear instead	Secure by default	tests/integration/test_mcp_env_var_copilot_e2e.py::TestMcpEnvVarHeadersCopilot::test_self_defined_http_server_translates_env_vars_not_resolves (regression-trap for #1152)	integration
2	apm install --target copilot does NOT bake secret values for self-defined stdio servers (env block AND args), across ${env:VAR}, ${VAR}, and legacy <VAR> syntaxes	Secure by default	tests/integration/test_mcp_env_var_copilot_e2e.py::TestMcpEnvVarHeadersCopilot::test_self_defined_stdio_server_translates_env_vars_in_args (regression-trap for the supply-chain dict-shape bug)	integration
3	apm install --target cursor continues to RESOLVE env vars at install time (sibling adapters are not silently affected)	Multi-harness support, Vendor-neutral	tests/integration/test_mcp_env_var_copilot_e2e.py::TestMcpEnvVarHeadersCursor::test_cursor_still_resolves_env_vars_to_literal	integration
4	Legacy <VAR> syntax in apm.yml is translated to ${VAR} for Copilot AND a deprecation warning names the offending servers	DevX	tests/unit/test_copilot_adapter.py::TestCopilotEnvVarTranslationInHeaders::test_legacy_angle_translates_with_warning	unit
5	When APM overwrites a Copilot config that previously contained baked literals, a one-shot security-improvement notice tells the user which vars to export	DevX, Secure by default	tests/unit/test_copilot_adapter.py::TestCopilotInstallRunSummary::test_security_upgrade_warning_when_baked_keys_detected	unit
6	Self-defined stdio dep with dict-shaped env block is not silently dropped to {} (regression trap for round-2 supply-chain finding)	Secure by default	tests/unit/test_copilot_adapter.py::TestCopilotEnvVarTranslationInStdioEnvBlock::test_dict_shaped_env_block_does_not_silently_drop	unit
7	When a Copilot HTTP-server config previously had baked header literals, the security-improvement notice still names the env vars to export (regression trap for the round-3 review-panel finding)	Secure by default, DevX	tests/unit/test_copilot_adapter.py::TestCopilotInstallRunSummary::test_security_upgrade_detects_baked_http_header_literals	unit

How to test

• Pull the branch and uv sync --extra dev in a clean checkout.
• Run the e2e test uv run --extra dev pytest tests/integration/test_mcp_env_var_copilot_e2e.py -v and confirm 3 passed.
• Manual smoke: in a scratch directory write an apm.yml with one self-defined stdio MCP server using ${env:MY_TOKEN} in args, then MY_TOKEN=plaintext APM_NON_INTERACTIVE=1 apm install --target copilot with HOME=/tmp/scratch. Confirm /tmp/scratch/.copilot/mcp-config.json contains ${MY_TOKEN} and does NOT contain plaintext.
• Sibling regression check: same scratch dir, apm install --target cursor instead. Confirm ~/.cursor/mcp.json (or equivalent) still contains the resolved literal — sibling adapters are unaffected by this PR.
• Post-install summary check: in the smoke test, confirm the end-of-run output includes [!] Copilot CLI will resolve N environment variable(s) at runtime that are not currently set if any referenced vars are missing from the shell.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[Copilot]

Pull request overview

This PR updates the Copilot MCP adapter to translate env-var placeholders into Copilot CLI runtime-substitution form (instead of resolving them at install time), so secrets are no longer written in plaintext to ~/.copilot/mcp-config.json.

Changes:

• Copilot target now translates ${env:VAR} / ${VAR} / legacy <VAR> into Copilot-native ${VAR} without reading os.environ, plus emits an aggregated end-of-run summary for security upgrades, unset vars, and legacy syntax.
• Sibling adapters inheriting from CopilotClientAdapter (Cursor/Windsurf/OpenCode/Claude/Gemini) are explicitly pinned to legacy install-time resolution via _supports_runtime_env_substitution = False.
• Adds unit + e2e regression coverage and updates docs + changelog to describe the new Copilot behavior.

Show a summary per file

File	Description
src/apm_cli/adapters/client/copilot.py	Implements placeholder translation mode, security-upgrade detection, and aggregated post-install diagnostics.
src/apm_cli/integration/mcp_integrator.py	Emits Copilot aggregated install-run summary once per runtime install.
src/apm_cli/adapters/client/cursor.py	Pins Cursor to legacy install-time env-var resolution.
src/apm_cli/adapters/client/windsurf.py	Pins Windsurf to legacy install-time env-var resolution.
src/apm_cli/adapters/client/opencode.py	Pins OpenCode to legacy install-time env-var resolution.
src/apm_cli/adapters/client/claude.py	Pins Claude to legacy resolution (no runtime substitution support).
src/apm_cli/adapters/client/gemini.py	Pins Gemini to legacy install-time env-var resolution.
tests/unit/test_copilot_adapter.py	Expands unit coverage for translation behavior, sibling pinning, and aggregated summary behavior.
tests/integration/test_mcp_env_var_copilot_e2e.py	Adds end-to-end guards ensuring Copilot configs contain placeholders (not secrets) and Cursor remains literal.
docs/src/content/docs/reference/cli-commands.md	Documents Copilot env-var placeholder translation and aggregated diagnostics.
docs/src/content/docs/guides/dependencies.md	Documents placeholder syntax and target-specific materialization behavior.
packages/apm-guide/.apm/skills/apm-usage/dependencies.md	Updates dependency authoring guidance to reflect Copilot translation semantics.
CHANGELOG.md	Adds an Unreleased Security entry describing the Copilot secrets-on-disk fix.

Copilot's findings

• Files reviewed: 13/13 changed files
• Comments generated: 4