From 52a6edec7eaf7f50ca2d4c4ef1f5c5af46bfdd95 Mon Sep 17 00:00:00 2001 From: Aaryan Dadu Date: Sat, 24 Oct 2026 14:45:48 +0530 Subject: [PATCH] fix(claude): keep ${VAR} references in project .mcp.json Claude Code expands ${VAR} and ${VAR:-default} in the project .mcp.json, so apm install keeps the references there instead of prompting for and writing the real values. Reinstall replaces values an earlier release wrote when APM can confirm it wrote them; otherwise it keeps the entry and names the fields to fix. User scope is unchanged. Adds an MCP variable compatibility page covering every harness. Closes #3252 --- CHANGELOG.md | 4 + docs/astro.config.mjs | 1 + .../docs/consumer/install-mcp-servers.md | 15 +- .../author-primitives/mcp-as-primitive.md | 11 +- .../content/docs/reference/manifest-schema.md | 1 + .../reference/mcp-variable-compatibility.md | 98 ++++++ .../.apm/skills/apm-usage/dependencies.md | 7 +- src/apm_cli/adapters/client/base.py | 14 + src/apm_cli/adapters/client/claude.py | 226 +++++++++++- src/apm_cli/adapters/client/copilot.py | 109 +++--- src/apm_cli/install/mcp/ownership.py | 14 + src/apm_cli/integration/mcp_integrator.py | 3 +- .../integration/mcp_integrator_install.py | 64 +++- .../test_claude_mcp_env_references_e2e.py | 325 ++++++++++++++++++ .../test_mcp_stdio_env_resolution_e2e.py | 20 +- .../test_mcp_integrator_install_flow.py | 12 +- tests/unit/install/test_mcp_ownership.py | 96 ++++++ tests/unit/test_claude_mcp.py | 299 ++++++++++++++++ tests/unit/test_copilot_adapter.py | 20 +- tests/unit/test_github_mcp_auth_policy.py | 3 + 20 files changed, 1259 insertions(+), 83 deletions(-) create mode 100644 docs/src/content/docs/reference/mcp-variable-compatibility.md create mode 100644 tests/integration/test_claude_mcp_env_references_e2e.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 82960dc779..eb26481b89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- `apm install --target claude` keeps `${VAR}` and `${VAR:-default}` references in the project's `.mcp.json` (`command`, `args`, `env`, `url`, `headers`) instead of prompting for and writing the real values, and reports missing variables by name for every server. Reinstall replaces values an earlier release wrote when APM can confirm it wrote them; otherwise it keeps the entry and names the fields to fix. User scope (`~/.claude.json`) is unchanged. The new MCP variable compatibility reference records every harness. -- by @Aaryan-Dadu (closes #3252) + ## [0.35.0] - 2026-10-22 ### Added diff --git a/docs/astro.config.mjs b/docs/astro.config.mjs index 265f2df38c..0dd7381dc9 100644 --- a/docs/astro.config.mjs +++ b/docs/astro.config.mjs @@ -280,6 +280,7 @@ export default defineConfig({ { label: 'Lockfile spec', slug: 'reference/lockfile-spec' }, { label: 'Policy schema', slug: 'reference/policy-schema' }, { label: 'Targets matrix', slug: 'reference/targets-matrix' }, + { label: 'MCP variable compatibility', slug: 'reference/mcp-variable-compatibility' }, { label: 'Primitive types', slug: 'reference/primitive-types' }, { label: 'Package types', slug: 'reference/package-types' }, { label: 'Baseline checks', slug: 'reference/baseline-checks' }, diff --git a/docs/src/content/docs/consumer/install-mcp-servers.md b/docs/src/content/docs/consumer/install-mcp-servers.md index 074a4ba70b..4b78ab03a5 100644 --- a/docs/src/content/docs/consumer/install-mcp-servers.md +++ b/docs/src/content/docs/consumer/install-mcp-servers.md @@ -428,8 +428,9 @@ credential value is not written into the generated config. Literal-only targets keep their existing automatic-token behavior. If none of the listed variables is set, no automatic header is added. -Reinstalling the same declaration does not automatically repair credentials -already written to runtime config. Follow +Except for Claude Code's project `.mcp.json` (see below), reinstalling the +same declaration does not automatically repair credentials already written to +runtime config. Follow [Repairing existing credentials](#repairing-existing-credentials) to preserve custom settings and rotate exposed credentials. For other authenticated remote servers, set headers explicitly with @@ -467,6 +468,16 @@ and make the variable available to Cursor. Keep all other fields and servers, and inspect the diff. An ordinary reinstall of the same declaration does not perform this repair. +For Claude Code at project scope, the next `apm install` replaces values in +`.mcp.json` that APM wrote earlier, either because APM recorded writing that +exact entry or because the entry matches the declaration expanded from the +current environment. It prints the affected variable names. An entry APM +cannot confirm it wrote is kept, and install reports which fields hold values. +Replace those yourself with `${NAME}` references, for example +`Bearer ${GITHUB_TOKEN}`. Export the variables before starting `claude`. +Claude Code still asks you to approve project servers; APM does not change +that approval. + If you instead regenerate an entry, save its custom fields first, remove only that server entry, reinstall, and restore those fields without restoring the old credential. Do not delete the whole config file. Rotate any credential diff --git a/docs/src/content/docs/producer/author-primitives/mcp-as-primitive.md b/docs/src/content/docs/producer/author-primitives/mcp-as-primitive.md index 6f44f2c89b..7d5c6e2913 100644 --- a/docs/src/content/docs/producer/author-primitives/mcp-as-primitive.md +++ b/docs/src/content/docs/producer/author-primitives/mcp-as-primitive.md @@ -115,11 +115,12 @@ shipped. Do not embed tokens. Two patterns work: ``` Headers and env values are never shell-expanded by APM. For harnesses -that support runtime env placeholders (for example VS Code and Kiro), -APM preserves the placeholder so the harness resolves it when the -server starts or the request is made. For harnesses that require -literal values (for example Claude Code and Codex self-defined stdio -env), APM resolves `${VAR}` from the install process environment, +that support runtime env placeholders (for example VS Code, Kiro, and +Claude Code's project `.mcp.json`), APM preserves the placeholder so the +harness resolves it when the server starts or the request is made. For +harnesses that require literal values (for example Codex self-defined +stdio env) and for Claude Code's user-scope `~/.claude.json`, APM +resolves `${VAR}` from the install process environment, prompts interactively when the terminal is attached, and leaves unresolved placeholders unchanged in non-interactive installs. Keep the real secret in the consumer's environment (or their secret manager). diff --git a/docs/src/content/docs/reference/manifest-schema.md b/docs/src/content/docs/reference/manifest-schema.md index f87244d751..27c9367f09 100644 --- a/docs/src/content/docs/reference/manifest-schema.md +++ b/docs/src/content/docs/reference/manifest-schema.md @@ -756,6 +756,7 @@ configurations. - **JetBrains Copilot** has native `${env:VAR}` interpolation in `mcp.json`; APM normalizes `${VAR}` and legacy `` to `${env:VAR}`. - **Copilot CLI and Kiro** have native `${VAR}` interpolation in their MCP config files; APM normalizes `${env:VAR}` and legacy `` to `${VAR}`. - **Codex and Gemini** have no runtime interpolation for `env`, so APM resolves `${VAR}`, `${env:VAR}`, and the legacy `` there at install time using `os.environ` (or an interactive prompt when missing). Resolved values are not re-scanned, so a value containing literal `${...}` text is preserved. +- **Claude Code** expands `${VAR}` and `${VAR:-default}` in `command`, `args`, `env`, `url` and `headers` of the project-scoped `.mcp.json` when it starts a server. An unset variable without a default stays as literal text, and `claude mcp list` warns about it. At project scope APM keeps these references, normalizing `${env:VAR}` and legacy `` to `${VAR}`, keeps static values as authored, and never writes referenced values into `.mcp.json` or `apm.lock.yaml`. Reinstall replaces values an earlier APM release expanded into `.mcp.json` when APM can confirm it wrote them; otherwise the entry is kept and install names the affected fields. User-scope installs (`-g`, `~/.claude.json`) keep resolving references at install time. See [MCP variable compatibility](../mcp-variable-compatibility/) for every harness. - **Cursor** resolves `${env:VAR}` references at runtime. APM translates `${VAR}` and legacy `` into that native form in `env`, `args`, and `headers`, so referenced values are not written into the project-local Cursor config. This normalization does not cover `command` or `url`; their existing behavior is unchanged. - **Codex `headers` are the exception.** Codex does read a remote server's headers from the environment, through dedicated fields rather than from inside `http_headers`, which it documents as static values. APM writes `Authorization: "Bearer ${VAR}"` as `bearer_token_env_var` and a value that is exactly `${VAR}` as `env_http_headers`; Codex resolves both at server start, so no token is written to `config.toml`. A value Codex cannot express that way -- literal text around the reference, or a shell-style default such as `${VAR:-}` -- is skipped with a warning instead of being written as static text. - **OpenCode** translates `${VAR}`, `${env:VAR}`, and legacy `` to native `{env:VAR}` in remote `headers`, local `environment` (manifest `env`), and command arguments. Already-native references, authored static values, command executables, and URLs stay unchanged. Registry `env` overlays preserve authored values only for declared, emitted keys. Export referenced variables when launching OpenCode: APM writes references, not their resolved values. Keep secrets out of authored static values. See [OpenCode config substitution](https://opencode.ai/docs/config/#env-vars). diff --git a/docs/src/content/docs/reference/mcp-variable-compatibility.md b/docs/src/content/docs/reference/mcp-variable-compatibility.md new file mode 100644 index 0000000000..49a82fc99a --- /dev/null +++ b/docs/src/content/docs/reference/mcp-variable-compatibility.md @@ -0,0 +1,98 @@ +--- +title: MCP variable compatibility +description: How each supported harness reads environment references in MCP configuration, what APM writes for it, and the gaps that remain. +sidebar: + order: 7 +--- + +`apm.yml` can reference environment variables in MCP `env`, `args`, `command`, +`url` and `headers` (see [Variable references](../manifest-schema/#424-variable-references-in-headers-and-env)). +Every harness reads references differently, so APM renders them per harness. +This page records, for each supported harness, the native contract, what APM +writes, and the gaps that remain. + +The results were checked on 2026-10-24 with the change for +[#3252](https://github.com/microsoft/apm/issues/3252). Evidence is marked as: + +- **client**: checked against the native client. +- **docs**: taken from the harness's official documentation only. +- **unverified**: the documentation does not say and no client was available. + +A harness that is only documented or unverified is not counted as passing. + +## Native contract + +| Harness (client checked) | APM writes to | Native syntax and fields | `${VAR:-default}` | Unset, no default | Approval before servers run | Evidence | +|---|---|---|---|---|---|---| +| Claude Code (2.1.310) | `.mcp.json` (project), `~/.claude.json` (user) | `${VAR}` in `command`, `args`, `env`, `url`, `headers` | Supported | Left as literal text; `claude mcp list` warns | Project servers need workspace trust and approval; user servers do not | client | +| GitHub Copilot CLI (1.0.44) | `.github/mcp.json` (project), `~/.copilot/mcp-config.json` (user) | `${NAME}` in `command`, `args`, `env`, header values (portable format) | Supported (portable format) | Not documented | Project servers load only after folder trust | docs; client for file locations | +| VS Code | `.vscode/mcp.json` (project) | `${env:NAME}`, `${input:id}` in `env`, `envFile`, `cwd`, `headers` | Only in the portable format | Not documented | Workspace servers follow Workspace Trust | docs | +| Cursor | `.cursor/mcp.json` (project), `~/.cursor/mcp.json` (user) | `${env:NAME}` in `command`, `args`, `env`, `url`, `headers` | Not documented | Not documented | Tool calls ask for approval by default | docs | +| Codex CLI | `.codex/config.toml` (project), `~/.codex/config.toml` (user) | No interpolation. `env_vars` forwards variables; `bearer_token_env_var` and `env_http_headers` name variables for HTTP | Not applicable | Not applicable | Project config only in trusted projects | docs | +| Gemini CLI | `.gemini/settings.json` (project), `~/.gemini/settings.json` (user) | `$VAR` or `${VAR}` in `env` | Not documented | Empty string | stdio servers connect only in trusted folders | docs | +| Antigravity | `.agents/mcp_config.json` (project), `~/.gemini/config/mcp_config.json` (user) | No interpolation documented; `env` is literal | Not documented | Not documented | Tool calls run in Ask mode by default | docs | +| Hermes Agent | `~/.hermes/config.yaml` (user) | `${VAR}` documented for catalog entries (`command`, `args`, `url`, `headers`); `config.yaml` entries not documented | Not documented | Not documented | `enabled: false` skips a server | unverified | +| OpenCode | `opencode.json` (project), `~/.config/opencode/opencode.json` (user) | `{env:VAR}` in config values | Not documented | Empty string | Not documented | docs | +| Windsurf | `~/.codeium/windsurf/mcp_config.json` (user) | `${env:VAR}` in `command`, `args`, `env`, `serverUrl`, `url`, `headers` | Not documented | Empty string | Team allowlists only | docs (Cascade documentation) | +| Kiro | `.kiro/settings/mcp.json` (project), `~/.kiro/settings/mcp.json` (user) | `${VAR}` (documented in `env` and `headers`) | Not documented | Not documented | Only variables listed in "Mcp Approved Env Vars" are expanded | docs | +| JetBrains Copilot | `$XDG_CONFIG_HOME/github-copilot/intellij/mcp.json` (user) | Not documented | Not documented | Not documented | Not documented | unverified | + +## What APM writes + +For one self-defined stdio server and one self-defined HTTP server that use a +set variable (`${DEMO_TOKEN}`), a defaulted one (`${DEMO_UNSET:-fallback}`), +an unset one (`${DEMO_MISSING}`) and a static value (`debug`): + +| Harness, scope | Set reference | Defaulted reference | Static value | Value written to the file? | +|---|---|---|---|---| +| Claude Code, project | `${DEMO_TOKEN}` in every field | Kept | Kept | No | +| Claude Code, user | `env`, `headers`: resolved at install time (unchanged); `command`, `args`, `url` copied | Kept | Kept | Yes (file is `0o600`, outside the repo) | +| Copilot CLI, project and user | `${DEMO_TOKEN}` | `env`: replaced by `${KEY}`; other fields kept | `env`: replaced by `${KEY}` | No | +| VS Code, project | `env`, `headers`: `${env:DEMO_TOKEN}`; `args`: bare `${DEMO_TOKEN}` | Kept | Kept | No | +| Cursor, project and user | `${env:DEMO_TOKEN}` in `env`, `args`, `headers`; `command` and `url` copied | Kept | Kept | No | +| Codex CLI, project and user | `env`: resolved; `Authorization`: `bearer_token_env_var` | Kept | Kept | Yes | +| Gemini CLI, project and user | Resolved at install time | Kept | Kept | Yes | +| Antigravity, project and user | Resolved at install time | Kept | Kept | Yes | +| Hermes Agent, user | Resolved at install time | Kept | Kept | Yes | +| OpenCode, project and user | `{env:DEMO_TOKEN}` in `env`, `command`, `headers`; `url` copied | Kept as `${...}` | Kept | No | +| Windsurf, user | Resolved at install time | Kept | Kept | Yes | +| Kiro, project and user | `${DEMO_TOKEN}` | `env`: replaced by `${KEY}` | `env`: replaced by `${KEY}` | No | +| JetBrains Copilot, user | `${env:DEMO_TOKEN}` | `env`: replaced by `${env:KEY}` | `env`: replaced by `${env:KEY}` | No | + +## Known gaps + +These existed before #3252 and are recorded here rather than changed by it: + +- **Codex CLI, Gemini CLI, Antigravity, Hermes Agent and Windsurf** write the + resolved value into their configuration. +- **Claude Code user scope** resolves at install time although Claude Code + expands references in `~/.claude.json` too. +- **Targets that resolve at install time** (Claude Code user scope, Codex CLI, + Gemini CLI, Antigravity, Hermes Agent, Windsurf) resolve `env` and `headers` + but copy references in `command`, `args` and `url` unchanged, including + spellings such as `${env:VAR}` that the harness may not read. +- **Cursor and OpenCode** copy references in `url` (and Cursor in `command`) + unchanged. +- **Copilot CLI, Kiro and JetBrains Copilot** replace static `env` values and + defaulted `env` references with a reference named after the key. +- **VS Code** leaves bare `${VAR}` in `args`, which the VS Code format does not + expand. +- **Cursor, VS Code, OpenCode, Codex, Gemini CLI, Antigravity, Hermes Agent and + Windsurf** copy `${VAR:-default}` unchanged, although their documented syntax + has no defaults. +- **Copilot CLI 1.0.44** reads project servers from `.mcp.json` only, not the + `.github/mcp.json` that APM writes. Current GitHub documentation lists both. +- **Every harness**: an MCP `url` must start with a literal `http://` or + `https://`, so the scheme itself cannot be a reference. + +## How this was checked + +- APM's output: the same declarations were installed for every harness and + scope with the installed CLI in an isolated home, using synthetic values, + and each generated file and the CLI output were searched for those values. +- Claude Code: a project `.mcp.json` and a user `~/.claude.json` using + `${VAR}`, `${VAR:-default}` and an unset variable in every field were loaded + by Claude Code 2.1.310 in an isolated configuration directory. A recording + stdio command and a local HTTP endpoint captured the expanded values. +- Copilot CLI: `copilot mcp list` (1.0.44) in an isolated home showed which + project files it reads. diff --git a/packages/apm-guide/.apm/skills/apm-usage/dependencies.md b/packages/apm-guide/.apm/skills/apm-usage/dependencies.md index 21a4fbba6b..1fcc1f47bf 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/dependencies.md +++ b/packages/apm-guide/.apm/skills/apm-usage/dependencies.md @@ -577,7 +577,12 @@ dependencies: # Kiro: preserved as ${VAR} and resolved at runtime. # Cursor: translated to ${env:VAR} and resolved at runtime. # OpenCode: translated to {env:VAR}; export variables at runtime. - # Windsurf/Claude/Gemini: resolved at install time. + # Claude Code project .mcp.json: kept as ${VAR} + # (${VAR:-default} too) in command, args, env, + # url and headers; Claude Code expands them. + # Reinstall replaces values APM wrote earlier. + # Claude user scope (-g)/Windsurf/Gemini: + # resolved at install time. # Codex: env resolved at install time; a remote # server's headers are written as # bearer_token_env_var / env_http_headers and diff --git a/src/apm_cli/adapters/client/base.py b/src/apm_cli/adapters/client/base.py index 7646bca296..e88c9c1f08 100644 --- a/src/apm_cli/adapters/client/base.py +++ b/src/apm_cli/adapters/client/base.py @@ -391,6 +391,20 @@ def matches_managed_server_config(self, native: object, expected: dict) -> bool: """Compare the native connection with the adapter's rendered baseline.""" return native == expected + def matches_legacy_managed_config( + self, native: object, expected: dict, *, native_hash: str | None = None + ) -> bool: + """Whether *native* is an older APM rendering of *expected*. + + Adapters whose native representation changed override this with the + evidence that proves an existing entry was APM-generated. + """ + return False + + def native_conflict_guidance(self, name: str, expected: dict) -> str | None: + """Extra target-specific guidance for a preserved conflicting entry.""" + return None + def managed_server_config(self, native: dict) -> dict: """Return connection data for an actual-write fingerprint.""" return dict(native) diff --git a/src/apm_cli/adapters/client/claude.py b/src/apm_cli/adapters/client/claude.py index 89650d467b..068b0d8ae5 100644 --- a/src/apm_cli/adapters/client/claude.py +++ b/src/apm_cli/adapters/client/claude.py @@ -22,15 +22,91 @@ import json import logging import os +import re +from collections.abc import Iterator, Mapping from pathlib import Path +from typing import ClassVar from ...utils.atomic_io import atomic_write_text from ...utils.console import _rich_error, _rich_success, _rich_warning from ...utils.path_security import PathTraversalError, validate_path_segments +from .base import _ENV_VAR_RE, _translate_env_placeholder from .copilot import CopilotClientAdapter _log = logging.getLogger(__name__) +# Claude Code's reference syntax: ${VAR} or ${VAR:-default}. +_CLAUDE_REFERENCE_RE = re.compile(r"\$\{([A-Za-z_][A-Za-z0-9_]*)(?::-[^}]*)?\}") + + +def _string_leaves(value: object) -> Iterator[str]: + """Yield every string inside a rendered server entry.""" + if isinstance(value, str): + yield value + elif isinstance(value, Mapping): + for item in value.values(): + yield from _string_leaves(item) + elif isinstance(value, list): + for item in value: + yield from _string_leaves(item) + + +def _paired_leaves(old: object, new: object, path: str = "") -> Iterator[tuple[str, str, str]]: + """Yield ``(path, old, new)`` for string leaves present in both entries.""" + if isinstance(old, str) and isinstance(new, str): + yield path, old, new + elif isinstance(old, Mapping) and isinstance(new, Mapping): + for key in new.keys() & old.keys(): + yield from _paired_leaves(old[key], new[key], f"{path}.{key}" if path else str(key)) + elif isinstance(old, list) and isinstance(new, list) and len(old) == len(new): + for index, (old_item, new_item) in enumerate(zip(old, new, strict=True)): + yield from _paired_leaves(old_item, new_item, f"{path}[{index}]") + + +def _expanded_leaves(old: object, new: object) -> Iterator[tuple[str, str]]: + """Yield ``(path, new)`` where *new* holds a reference and *old* a value.""" + for path, old_text, new_text in _paired_leaves(old, new): + if old_text != new_text and "${" not in old_text and _CLAUDE_REFERENCE_RE.search(new_text): + yield path, new_text + + +def _expand_like_install_time(text: str, environ: Mapping[str, str]) -> str: + """Expand bare ``${VAR}`` references the way pre-#3252 installs did.""" + return _ENV_VAR_RE.sub(lambda m: environ.get(m.group(1)) or m.group(0), text) + + +def _legacy_equivalent(native: object, expected: object, environ: Mapping[str, str]) -> bool: + """Whether *native* is *expected* with its references expanded from *environ*. + + Pre-#3252 releases expanded each recognized reference at install time and + left unresolved ones in their authored spelling (``${env:VAR}``, ````) + or, for registry variables, as an empty string. + """ + if isinstance(expected, Mapping): + return ( + isinstance(native, Mapping) + and native.keys() == expected.keys() + and all(_legacy_equivalent(native[key], expected[key], environ) for key in expected) + ) + if isinstance(expected, list): + return ( + isinstance(native, list) + and len(native) == len(expected) + and all( + _legacy_equivalent(item, ref, environ) + for item, ref in zip(native, expected, strict=True) + ) + ) + if isinstance(expected, str) and isinstance(native, str): + if native == expected: + return True + expanded = _expand_like_install_time(expected, environ) + if _translate_env_placeholder(native) == expanded: + return True + unset_reference = _ENV_VAR_RE.fullmatch(expected) + return native == "" and bool(unset_reference) and not environ.get(unset_reference.group(1)) + return native == expected + class ClaudeClientAdapter(CopilotClientAdapter): """MCP configuration for Claude Code (``mcpServers`` schema). @@ -44,14 +120,45 @@ class ClaudeClientAdapter(CopilotClientAdapter): """ supports_user_scope: bool = True + _client_label: str = "Claude Code" target_name: str = "claude" mcp_servers_key: str = "mcpServers" - # Claude Desktop / Code's mcp config does NOT support runtime env-var - # substitution -- the value in ``env`` must be a literal string. This - # adapter MUST keep the legacy install-time resolution behaviour. - # See #1152 supply-chain analysis. - _supports_runtime_env_substitution: bool = False + _runtime_config_label: ClassVar[str] = ".mcp.json" + _runtime_launch_command: ClassVar[str] = "claude" + + # Own post-install aggregation buckets: the parent's mutable ClassVars + # are shared by reference, so without these Claude's diagnostics would + # be reported (and worded) as Copilot CLI's. + _legacy_angle_offenders_by_server: ClassVar[dict] = {} + _security_upgraded_keys: ClassVar[set] = set() + _unset_env_keys_by_server: ClassVar[dict] = {} + _install_run_summary_emitted: ClassVar[bool] = False + + # Extra guidance appended to the security-upgrade notice: replacing a + # committed value does not revoke it or remove it from Git history. + _runtime_upgrade_guidance: ClassVar[str] = ( + "If .mcp.json was committed, rotate these credentials: replacing the " + "values does not remove them from Git history." + ) + + @property + def _supports_runtime_env_substitution(self) -> bool: + """Keep ``${VAR}`` references in the project-scoped ``.mcp.json``. + + Claude Code expands ``${VAR}`` and ``${VAR:-default}`` in a project + ``.mcp.json`` in ``command``, ``args``, ``env``, ``url`` and + ``headers`` when it starts a server (checked with Claude Code + 2.1.310), so secrets stay in each developer's environment instead of + being copied into a file teams commit. User scope (``~/.claude.json``, + outside the repo, written ``0o600``) keeps install-time resolution. + See #1152 and #3252. + """ + return not self._is_user_scope() + + def _format_runtime_env_placeholder(self, name: str) -> str: + """Return Claude Code's native reference syntax, ``${NAME}``.""" + return "${" + name + "}" # Skill packages declare self-defined MCP launchers using the cross-tool # converged path ``.agents/skills//...`` (see @@ -81,6 +188,12 @@ def _format_server_config(self, server_info, env_overrides=None, runtime_vars=No config = super()._format_server_config(server_info, env_overrides, runtime_vars) if isinstance(config, dict) and "command" in config: config["command"] = self._rewrite_self_defined_skill_command(config["command"]) + if isinstance(config, dict) and self._supports_runtime_env_substitution: + # The shared pipeline copies command and url verbatim. Claude Code + # only reads ${VAR}, so normalize ${env:VAR} and there too. + for field in ("command", "url"): + if isinstance(config.get(field), str): + config[field] = self._resolve_env_variable(field, config[field]) return config def render_server_config( @@ -128,6 +241,107 @@ def project_managed_server_config( return native return {key: value for key, value in native.items() if key != "cwd"} + def _resolve_environment_variables(self, env_vars, env_overrides=None): + """Keep static ``env`` values authored in apm.yml as written. + + Copilot's translate mode also rewrites static values to ``${NAME}``. + Claude Code passes an unset variable without a default through as + the literal ``${NAME}`` text, so a server configured with a plain + setting such as ``LOG_LEVEL: debug`` would silently lose it. + """ + if isinstance(env_vars, dict) and self._supports_runtime_env_substitution: + return self._resolve_explicit_environment_values(env_vars, env_overrides) + return super()._resolve_environment_variables(env_vars, env_overrides) + + # ------------------------------------------------------------------ # + # Post-install diagnostics (names only, never values) + # ------------------------------------------------------------------ # + + def _begin_env_tracking(self, server_url, server_name): + """Snapshot the current native entry for the security-upgrade notice.""" + self._last_env_placeholder_keys = set() + self._last_legacy_angle_vars = set() + if not self._supports_runtime_env_substitution: + return None + key = self._determine_config_key(server_url, server_name) + previous = (self.get_current_config().get(self.mcp_servers_key) or {}).get(key) + return previous if isinstance(previous, dict) else None + + def _record_env_diagnostics(self, config_key, server_config, previously_baked): + """Record replaced values and missing variables for the run summary.""" + if self._supports_runtime_env_substitution: + if self._last_legacy_angle_vars: + self._legacy_angle_offenders_by_server[config_key] = set( + self._last_legacy_angle_vars + ) + if isinstance(previously_baked, dict): + for _path, text in _expanded_leaves(previously_baked, server_config): + self._security_upgraded_keys.update( + m.group(1) for m in _CLAUDE_REFERENCE_RE.finditer(text) + ) + self._emit_install_summary(config_key, server_config) + + def _emit_install_summary(self, config_key, server_config): + """Track unset references without a default in every expanded field.""" + if not self._supports_runtime_env_substitution: + return + names = { + m.group(1) for text in _string_leaves(server_config) for m in _ENV_VAR_RE.finditer(text) + } + unset = sorted(name for name in names if not os.environ.get(name)) + if unset: + bucket = self.__class__._unset_env_keys_by_server.setdefault(config_key, []) + bucket.extend(name for name in unset if name not in bucket) + + # ------------------------------------------------------------------ # + # Entries written before references were kept (#3252) + # ------------------------------------------------------------------ # + + def matches_legacy_managed_config( + self, native: object, expected: dict, *, native_hash: str | None = None + ) -> bool: + """Whether *native* is the expanded form APM wrote for *expected*. + + Earlier releases expanded references at install time. Such an entry is + treated as APM-generated only when it is unchanged since APM recorded + its write hash, or when expanding the references in *expected* (the + current rendering of the same declaration) from this environment + reproduces it exactly. Anything else may hold user edits. + """ + if self._is_user_scope() or not isinstance(native, dict): + return False + if not any(_CLAUDE_REFERENCE_RE.search(text) for text in _string_leaves(expected)): + return False + comparable = self.managed_server_config(native) + if native_hash is not None: + from apm_cli.install.mcp.ownership import mcp_native_hash + + if mcp_native_hash(comparable) == native_hash: + return True + return _legacy_equivalent(comparable, expected, os.environ) + + def native_conflict_guidance(self, name: str, expected: dict) -> str | None: + """Explain a preserved entry that holds values where apm.yml has references.""" + if self._is_user_scope(): + return None + key = self._determine_config_key(name, None) + native = (self.get_current_config().get(self.mcp_servers_key) or {}).get(key) + fields = sorted(path for path, _text in _expanded_leaves(native, expected)) + if not fields: + return None + where = ( + f"Field {fields[0]} holds a value" + if len(fields) == 1 + else f"Fields {', '.join(fields)} hold values" + ) + return ( + f"{where} where apm.yml declares environment references, and APM cannot " + "confirm it wrote them. Put the ${VAR} references declared in apm.yml " + "back in those places yourself, then retry. If .mcp.json was committed, " + "rotate those credentials: editing the file does not erase them in Git " + "history." + ) + @classmethod def _is_remote_mcp_entry(cls, entry: dict) -> bool: """Return whether *entry* describes a remote (URL-addressed) server.""" @@ -371,6 +585,7 @@ def configure_mcp_server( config_key = self._determine_config_key(server_url, server_name) + previously_baked = self._begin_env_tracking(server_url, server_name) server_config = self._format_server_config(server_info, env_overrides, runtime_vars) from apm_cli.install.mcp.ownership import mcp_native_hash @@ -382,6 +597,7 @@ def configure_mcp_server( _rich_error(f"Failed to write MCP config for '{config_key}' to Claude Code") return False self.written_mcp_hashes[server_url] = fingerprint + self._record_env_diagnostics(config_key, server_config, previously_baked) _rich_success(f"Successfully configured MCP server '{config_key}' for Claude Code") return True diff --git a/src/apm_cli/adapters/client/copilot.py b/src/apm_cli/adapters/client/copilot.py index 1ac5572ca4..cd280d1f8f 100644 --- a/src/apm_cli/adapters/client/copilot.py +++ b/src/apm_cli/adapters/client/copilot.py @@ -46,6 +46,12 @@ class CopilotClientAdapter(MCPClientAdapter): target_name: str = "copilot" mcp_servers_key: str = "mcpServers" + # Wording for the post-install env-var diagnostics, so subclasses that + # reuse them (Claude Code) name their own config file and launch command. + _runtime_config_label: ClassVar[str] = "the Copilot config" + _runtime_launch_command: ClassVar[str] = "gh copilot" + _runtime_upgrade_guidance: ClassVar[str] = "" + # When True, env-var placeholders (``${VAR}``, ``${env:VAR}``, legacy # ````) are translated to Copilot CLI's native runtime-substitution # syntax (``${VAR}``) and emitted into mcp-config.json verbatim. The @@ -55,12 +61,11 @@ class CopilotClientAdapter(MCPClientAdapter): # environment and the literal value is baked into the config file # (legacy pre-#1152 behaviour). # - # Subclasses (Windsurf / OpenCode / Claude / Gemini) override this to - # ``False`` until their respective config formats are individually audited - # for runtime-substitution support. Cursor has audited native - # ``${env:VAR}`` support. Critically, Claude Desktop's config format does - # NOT support runtime substitution -- it MUST keep resolving at install - # time. + # Subclasses (Windsurf / OpenCode / Gemini) override this to ``False`` + # until their respective config formats are individually audited for + # runtime-substitution support. Cursor has audited native ``${env:VAR}`` + # support. Claude enables it for the project-scoped ``.mcp.json`` (see + # ``ClaudeClientAdapter``). _supports_runtime_env_substitution: bool = True # Process-wide aggregation of legacy ```` offenders, keyed by @@ -199,22 +204,7 @@ def configure_mcp_server( if server_info is None: return False - # Reset per-server tracking before formatting (so the per-server - # summary line and aggregated diagnostics reflect this server only). - self._last_env_placeholder_keys = set() - self._last_legacy_angle_vars = set() - - # Detect security upgrade: was the previous on-disk config for - # this server holding literal (resolved) values for env keys - # we are about to replace with ${KEY} placeholders? If so, - # remember the affected keys for the post-install notice. We - # snapshot BEFORE writing the new config. - previously_baked_keys = set() - previously_baked_headers = False - if self._supports_runtime_env_substitution: - previously_baked_keys, previously_baked_headers = ( - self._collect_previously_baked_keys(server_url, server_name) - ) + previously_baked = self._begin_env_tracking(server_url, server_name) # Generate server configuration with environment and runtime variable resolution server_config = self._format_server_config(server_info, env_overrides, runtime_vars) @@ -224,31 +214,54 @@ def configure_mcp_server( # Update configuration using the chosen key self.update_config({config_key: server_config}) - # Aggregate diagnostics for the post-install summary. - if self._supports_runtime_env_substitution: - if self._last_legacy_angle_vars: - self._legacy_angle_offenders_by_server[config_key] = set( - self._last_legacy_angle_vars - ) - # Only flag a security upgrade when the previously baked keys - # actually overlap with what we are now placeholderizing -- OR - # when the previous on-disk state had baked HTTP header - # literals (which don't expose env-var names directly, so we - # surface every newly-placeholderised key for this server). - upgraded = previously_baked_keys & self._last_env_placeholder_keys - if previously_baked_headers and self._last_env_placeholder_keys: - upgraded = upgraded | self._last_env_placeholder_keys - if upgraded: - self._security_upgraded_keys.update(upgraded) - - # Per-server install line with env-var summary parenthetical. - self._emit_install_summary(config_key, server_config) + self._record_env_diagnostics(config_key, server_config, previously_baked) return True except Exception as e: print(f"Error configuring MCP server: {e}") return False + def _begin_env_tracking(self, server_url, server_name): + """Reset per-server env tracking and snapshot baked on-disk values. + + Must run BEFORE the new config is formatted and written. Returns the + snapshot that :meth:`_record_env_diagnostics` consumes. + """ + # Reset per-server tracking before formatting (so the per-server + # summary line and aggregated diagnostics reflect this server only). + self._last_env_placeholder_keys = set() + self._last_legacy_angle_vars = set() + + # Detect security upgrade: was the previous on-disk config for + # this server holding literal (resolved) values for env keys + # we are about to replace with ${KEY} placeholders? If so, + # remember the affected keys for the post-install notice. + if not self._supports_runtime_env_substitution: + return set(), False + return self._collect_previously_baked_keys(server_url, server_name) + + def _record_env_diagnostics(self, config_key, server_config, previously_baked): + """Aggregate this server's env diagnostics for the post-install summary.""" + previously_baked_keys, previously_baked_headers = previously_baked + if self._supports_runtime_env_substitution: + if self._last_legacy_angle_vars: + self._legacy_angle_offenders_by_server[config_key] = set( + self._last_legacy_angle_vars + ) + # Only flag a security upgrade when the previously baked keys + # actually overlap with what we are now placeholderizing -- OR + # when the previous on-disk state had baked HTTP header + # literals (which don't expose env-var names directly, so we + # surface every newly-placeholderised key for this server). + upgraded = previously_baked_keys & self._last_env_placeholder_keys + if previously_baked_headers and self._last_env_placeholder_keys: + upgraded = upgraded | self._last_env_placeholder_keys + if upgraded: + self._security_upgraded_keys.update(upgraded) + + # Per-server install line with env-var summary parenthetical. + self._emit_install_summary(config_key, server_config) + def _collect_previously_baked_keys(self, server_url, server_name): """Return ``(env_keys, headers_were_baked)`` for the existing on-disk entry: the set of env-block keys whose values are literal @@ -357,9 +370,15 @@ def _emit_separator_once(): _emit_separator_once() _rich_warning( f"Security improvement: {count} environment {noun} previously stored as " - f"plaintext in the Copilot config are now resolved at runtime.\n" + f"plaintext in {cls._runtime_config_label} are now resolved at runtime.\n" f" Affected: {affected}\n" - f" Ensure these are exported in your shell before running 'gh copilot'", + f" Ensure these are exported in your shell before running " + f"'{cls._runtime_launch_command}'" + + ( + f"\n {cls._runtime_upgrade_guidance}" + if cls._runtime_upgrade_guidance + else "" + ), symbol="warning", ) if cls._unset_env_keys_by_server: @@ -372,11 +391,11 @@ def _emit_separator_once(): noun = "variable" if count == 1 else "variables" _emit_separator_once() _rich_warning( - f"Copilot CLI will resolve {count} environment {noun} at runtime " + f"{cls._client_label} will resolve {count} environment {noun} at runtime " f"that {'is' if count == 1 else 'are'} not currently set: " f"{', '.join(sorted_unset)}.\n" f" Export {'it' if count == 1 else 'them'} in your shell before " - f"running 'gh copilot', e.g.:\n" + f"running '{cls._runtime_launch_command}', e.g.:\n" f" export {export_hint}", symbol="warning", ) diff --git a/src/apm_cli/install/mcp/ownership.py b/src/apm_cli/install/mcp/ownership.py index 6ad7eea000..97fc9830c0 100644 --- a/src/apm_cli/install/mcp/ownership.py +++ b/src/apm_cli/install/mcp/ownership.py @@ -71,10 +71,12 @@ def compare_native_mcp_connection( if managed and isinstance(stored_config, dict): try: dependency = MCPDependency.from_dict(stored_config) + legacy_baseline = requested if dependency.is_self_defined: expected = client.render_server_config( MCPIntegrator._build_self_defined_info(dependency) ) + legacy_baseline = expected if requested is not None: comparison = client.project_managed_server_config( native, baseline=expected, requested=requested @@ -87,6 +89,18 @@ def compare_native_mcp_connection( matches_baseline = mcp_native_hash(comparable) == native_hash if matches_baseline and managed_fields is not None: managed_fields.update(comparable) + # An entry an earlier APM release rendered differently (e.g. with + # expanded references) is still managed when the adapter can prove it. + if ( + not matches_baseline + and legacy_baseline is not None + and client.matches_legacy_managed_config( + native, legacy_baseline, native_hash=native_hash + ) + ): + matches_baseline = True + if managed_fields is not None: + managed_fields.update(client.managed_server_config(native)) except (TypeError, ValueError): logger.debug("Unusable MCP baseline for %s on %s", name, client.target_name) if requested is not None: diff --git a/src/apm_cli/integration/mcp_integrator.py b/src/apm_cli/integration/mcp_integrator.py index 805e5452f5..3a6ae4ec67 100644 --- a/src/apm_cli/integration/mcp_integrator.py +++ b/src/apm_cli/integration/mcp_integrator.py @@ -1307,7 +1307,8 @@ def _install_for_runtime( all_ok = False # Emit aggregated post-install diagnostics for runtimes that - # support runtime env-var substitution (currently Copilot CLI). + # support runtime env-var substitution (currently Copilot CLI; + # Claude Code's are emitted once per run by run_mcp_install). # Safe no-op for runtimes whose adapter doesn't aggregate state. try: if runtime == "copilot": diff --git a/src/apm_cli/integration/mcp_integrator_install.py b/src/apm_cli/integration/mcp_integrator_install.py index fb17e06a33..b2c503443c 100644 --- a/src/apm_cli/integration/mcp_integrator_install.py +++ b/src/apm_cli/integration/mcp_integrator_install.py @@ -7,6 +7,7 @@ from __future__ import annotations import builtins +import logging from enum import Enum from pathlib import Path from typing import TYPE_CHECKING, Any @@ -34,6 +35,8 @@ from apm_cli.core.scope import InstallScope from apm_cli.core.target_detection import EffectiveTargetDecision +_log = logging.getLogger(__name__) + # IntelliJ owns a fail-closed atomic user-config contract. Other adapters retain # their existing best-effort behavior when a composed target set includes an # unavailable optional runtime. @@ -359,7 +362,18 @@ def _install_registry_group( # noqa: PLR0913 if native_hashes is not None: native_hashes.get(rt, {}).pop(dep, None) if state is NativeMcpConnectionState.CONFLICT: - _report_native_conflict(dep, rt, client, logger, native_conflicts) + _report_native_conflict( + dep, + rt, + client, + logger, + native_conflicts, + render=lambda client=client, dep=dep: client.render_server_config( + server_info_cache[dep], + env_overrides=shared_env_vars, + runtime_vars=shared_runtime_vars, + ), + ) continue if state in { NativeMcpConnectionState.MATCHING_MANAGED, @@ -433,10 +447,32 @@ def _install_registry_group( # noqa: PLR0913 def _report_native_conflict( - name: str, runtime: str, client: Any, logger: Any, conflicts: list[str] + name: str, + runtime: str, + client: Any, + logger: Any, + conflicts: list[str], + render: Any = None, ) -> None: - """Report a preserved native collision identically for both dependency sources.""" + """Report a preserved native collision identically for both dependency sources. + + *render* returns the requested native entry, letting the adapter add + target-specific guidance (names only) about the preserved connection. + """ conflicts.append(f"{name} ({runtime})") + guidance = None + if render is not None: + try: + guidance = client.native_conflict_guidance(name, render()) + except (TypeError, ValueError): + _log.debug("No conflict guidance for %s on %s", name, runtime, exc_info=True) + if guidance: + logger.error( + f"MCP server '{name}' conflicts with the existing {runtime} " + f"connection in {client.get_config_path()}. Preserved that connection. " + f"{guidance} --force does not take ownership of user connections." + ) + return logger.error( f"MCP server '{name}' conflicts with the existing {runtime} " f"connection in {client.get_config_path()}. Preserved that connection. " @@ -1184,7 +1220,16 @@ def _install_self_defined_deps( if native_hashes is not None: native_hashes.get(rt, {}).pop(dep.name, None) if state is NativeMcpConnectionState.CONFLICT: - _report_native_conflict(dep.name, rt, client, logger, native_conflicts) + _report_native_conflict( + dep.name, + rt, + client, + logger, + native_conflicts, + render=lambda client=client, info=synthetic_info: ( + client.render_server_config(info) + ), + ) continue if state in { NativeMcpConnectionState.MATCHING_MANAGED, @@ -1410,6 +1455,13 @@ def run_mcp_install( # noqa: PLR0913 console=console, logger=logger, ) + claude_targeted = "claude" in target_runtimes + if claude_targeted: + # Claude's diagnostics cover every server of this run, so they are + # emitted once at the end rather than after the first server. + from apm_cli.adapters.client.claude import ClaudeClientAdapter + + ClaudeClientAdapter.reset_install_run_state() if managed_target_servers is not None: from apm_cli.install.mcp.ownership import migrate_legacy_project_target_servers @@ -1575,11 +1627,15 @@ def run_mcp_install( # noqa: PLR0913 native_conflicts.append(str(exc)) if native_conflicts: + if claude_targeted: + ClaudeClientAdapter.emit_install_run_summary() raise MCPNativeConflictError( "\n".join(native_conflicts), wrote_connections=bool(written_connections) ) # Close the panel _print_mcp_summary(console, configured_count, successful_updates) + if claude_targeted: + ClaudeClientAdapter.emit_install_run_summary() return configured_count diff --git a/tests/integration/test_claude_mcp_env_references_e2e.py b/tests/integration/test_claude_mcp_env_references_e2e.py new file mode 100644 index 0000000000..0dd465a669 --- /dev/null +++ b/tests/integration/test_claude_mcp_env_references_e2e.py @@ -0,0 +1,325 @@ +"""Installed-CLI coverage for #3252: Claude project ``.mcp.json`` keeps references. + +Claude Code expands ``${VAR}`` and ``${VAR:-default}`` in a project ``.mcp.json`` +(``command``, ``args``, ``env``, ``url``, ``headers``) when it starts a server, so +``apm install`` must keep references there instead of collecting or copying the +values. User-scope ``~/.claude.json`` keeps install-time resolution. All values +are synthetic sentinels; every test asserts they reach no file or output. +""" + +from __future__ import annotations + +import json +import os +from pathlib import Path +from urllib.parse import urlparse + +import pytest +import yaml + +from tests.utils.apm_lifecycle_runner import ApmLifecycleRunner +from tests.utils.isolated_apm_environment import IsolatedApmEnvironment +from tests.utils.local_mcp_registry import LocalMcpRegistryFactory + +pytestmark = [pytest.mark.e2e, pytest.mark.requires_apm_binary] + +SECRET = "claude-e2e-secret-sentinel" +OTHER_SECRET = "claude-e2e-other-sentinel" + +SELF_DEFINED_STDIO = { + "name": "refs-stdio", + "registry": False, + "transport": "stdio", + "command": "${E2E_CMD:-uvx}", + "args": ["refs-server", "--token=${E2E_TOKEN}", "--mode=${E2E_MODE:-safe}", ""], + "env": { + "TOKEN": "${E2E_TOKEN}", + "PREFIXED": "${env:E2E_TOKEN}", + "DEFAULTED": "${E2E_UNSET:-fallback}", + "MISSING": "${E2E_MISSING}", + "LOG_LEVEL": "debug", + }, +} + +SELF_DEFINED_HTTP = { + "name": "refs-http", + "registry": False, + "transport": "http", + "url": "https://${E2E_HOST:-mcp.example.invalid}/mcp?key=${E2E_TOKEN}", + "headers": { + "Authorization": "Bearer ${E2E_TOKEN}", + "X-Region": "${E2E_REGION:-eu}", + "X-Missing": "${E2E_HTTP_MISSING}", + }, +} + +EXPECTED_STDIO = { + "type": "stdio", + "command": "${E2E_CMD:-uvx}", + "args": ["refs-server", "--token=${E2E_TOKEN}", "--mode=${E2E_MODE:-safe}", "${E2E_TOKEN}"], + "env": { + "TOKEN": "${E2E_TOKEN}", + "PREFIXED": "${E2E_TOKEN}", + "DEFAULTED": "${E2E_UNSET:-fallback}", + "MISSING": "${E2E_MISSING}", + "LOG_LEVEL": "debug", + }, +} + +EXPECTED_HTTP = { + "type": "http", + "url": "https://${E2E_HOST:-mcp.example.invalid}/mcp?key=${E2E_TOKEN}", + "headers": { + "Authorization": "Bearer ${E2E_TOKEN}", + "X-Region": "${E2E_REGION:-eu}", + "X-Missing": "${E2E_HTTP_MISSING}", + }, +} + +UNSET_NAMES = ("E2E_UNSET", "E2E_MISSING", "E2E_HTTP_MISSING", "E2E_CMD", "E2E_MODE", "E2E_HOST") + +Scenario = tuple[Path, dict[str, str], IsolatedApmEnvironment] + + +@pytest.fixture +def claude_project(tmp_path: Path) -> Scenario: + """A scrubbed environment with a Claude-enabled project and one set variable.""" + isolated = IsolatedApmEnvironment.create(tmp_path / "claude", base_env=dict(os.environ)) + project = isolated.work_root / "project" + (project / ".claude").mkdir(parents=True) + env = isolated.subprocess_env() + env["E2E_TOKEN"] = SECRET + env["APM_NON_INTERACTIVE"] = "1" + for name in UNSET_NAMES: + env.pop(name, None) + return project, env, isolated + + +def _write_apm_yml(directory: Path, servers: list[dict]) -> None: + directory.mkdir(parents=True, exist_ok=True) + manifest = { + "name": "claude-refs-e2e", + "version": "1.0.0", + "dependencies": {"apm": [], "mcp": servers}, + } + (directory / "apm.yml").write_text(yaml.safe_dump(manifest), encoding="utf-8") + + +def _install(binary: Path, project: Path, env: dict[str, str], *extra: str): + runner = ApmLifecycleRunner((str(binary),), timeout_seconds=120) + return runner.run(("install", "--only", "mcp", "--no-policy", *extra), cwd=project, env=env) + + +def _servers(project: Path) -> dict: + return json.loads((project / ".mcp.json").read_text(encoding="utf-8"))["mcpServers"] + + +def _flat(text: str) -> str: + """Join the CLI's terminal-width line wrapping back into single spaces.""" + return " ".join(text.split()) + + +def _assert_no_value(texts: list[str], *values: str) -> None: + for text in texts: + for value in values: + assert value not in text + + +def test_project_config_keeps_references_in_every_field( + claude_project: Scenario, apm_binary_path: Path +) -> None: + project, env, _isolated = claude_project + _write_apm_yml(project, [SELF_DEFINED_STDIO, SELF_DEFINED_HTTP]) + + result = _install(apm_binary_path, project, env, "--target", "claude") + + output = result.stdout + result.stderr + assert result.returncode == 0, output + servers = _servers(project) + assert servers["refs-stdio"] == EXPECTED_STDIO + assert servers["refs-http"] == EXPECTED_HTTP + _assert_no_value( + [(project / ".mcp.json").read_text(), (project / "apm.lock.yaml").read_text(), output], + SECRET, + ) + # Names only, once per run, covering both servers; defaulted names are not missing. + assert output.count("Claude Code will resolve") == 1 + assert "E2E_HTTP_MISSING, E2E_MISSING" in _flat(output) + for defaulted in ("E2E_UNSET", "E2E_CMD", "E2E_MODE", "E2E_HOST", "E2E_REGION"): + assert defaulted not in output + assert "Enter value for" not in output + + +def test_registry_server_keeps_references(claude_project: Scenario, apm_binary_path: Path) -> None: + project, env, isolated = claude_project + document = { + "name": "io.github.apm/claude-refs-registry", + "description": "Claude reference fixture", + "version": "1.0.0", + "packages": [ + { + "registryType": "npm", + "identifier": "@apm/claude-refs-registry", + "transport": {"type": "stdio"}, + "environmentVariables": [ + {"name": "E2E_REQUIRED", "required": True}, + {"name": "E2E_OPTIONAL", "required": False}, + ], + "packageArguments": [ + { + "type": "named", + "name": "--api-key", + "value": "{api_key}", + "variables": {"api_key": {"isSecret": True}}, + } + ], + } + ], + } + env["E2E_REQUIRED"] = SECRET + env["api_key"] = OTHER_SECRET + with LocalMcpRegistryFactory(isolated.root / "registries").start(document) as registry: + env["APM_TEST_LOOPBACK_PORTS"] = str(urlparse(registry.url).port) + env["MCP_REGISTRY_ALLOW_HTTP"] = "1" + _write_apm_yml(project, [{"name": document["name"], "registry": registry.url}]) + result = _install(apm_binary_path, project, env, "--target", "claude") + + output = result.stdout + result.stderr + assert result.returncode == 0, output + server = _servers(project)["claude-refs-registry"] + assert server["env"] == {"E2E_REQUIRED": "${E2E_REQUIRED}"} + assert server["args"][-2:] == ["--api-key", "${api_key}"] + _assert_no_value( + [(project / ".mcp.json").read_text(), (project / "apm.lock.yaml").read_text(), output], + SECRET, + OTHER_SECRET, + ) + + +def test_user_scope_keeps_install_time_resolution( + claude_project: Scenario, apm_binary_path: Path +) -> None: + project, env, isolated = claude_project + user_config = isolated.home / ".claude.json" + user_config.write_text(json.dumps({"numStartups": 3}), encoding="utf-8") + user_config.chmod(0o600) + _write_apm_yml( + isolated.config_root, + [ + { + "name": "user-refs", + "registry": False, + "transport": "stdio", + "command": "uvx", + "env": {"TOKEN": "${E2E_TOKEN}"}, + } + ], + ) + + result = _install(apm_binary_path, project, env, "--global", "--target", "claude") + + assert result.returncode == 0, result.stdout + result.stderr + data = json.loads(user_config.read_text(encoding="utf-8")) + assert data["numStartups"] == 3 + assert data["mcpServers"]["user-refs"]["env"] == {"TOKEN": SECRET} + assert user_config.stat().st_mode & 0o077 == 0 + assert not (project / ".mcp.json").exists() + + +def test_mixed_targets_use_their_own_native_representation( + claude_project: Scenario, apm_binary_path: Path +) -> None: + project, env, _isolated = claude_project + for signal in (".cursor", ".github", ".vscode"): + (project / signal).mkdir() + _write_apm_yml(project, [SELF_DEFINED_STDIO]) + + result = _install(apm_binary_path, project, env, "--target", "claude,cursor,copilot,vscode") + + output = result.stdout + result.stderr + assert result.returncode == 0, output + configs = { + "claude": project / ".mcp.json", + "cursor": project / ".cursor" / "mcp.json", + "copilot": project / ".github" / "mcp.json", + "vscode": project / ".vscode" / "mcp.json", + } + loaded = {name: json.loads(path.read_text(encoding="utf-8")) for name, path in configs.items()} + assert loaded["claude"]["mcpServers"]["refs-stdio"]["env"]["TOKEN"] == "${E2E_TOKEN}" + assert loaded["cursor"]["mcpServers"]["refs-stdio"]["env"]["TOKEN"] == "${env:E2E_TOKEN}" + assert loaded["copilot"]["mcpServers"]["refs-stdio"]["env"]["TOKEN"] == "${E2E_TOKEN}" + assert loaded["vscode"]["servers"]["refs-stdio"]["env"]["TOKEN"] == "${env:E2E_TOKEN}" + _assert_no_value([path.read_text() for path in configs.values()] + [output], SECRET) + + +def test_unrelated_entries_and_native_consent_are_untouched( + claude_project: Scenario, apm_binary_path: Path +) -> None: + project, env, _isolated = claude_project + user_entry = {"type": "stdio", "command": "node", "env": {"KEY": "user-authored-value"}} + (project / ".mcp.json").write_text( + json.dumps({"mcpServers": {"user-kept": user_entry}}), encoding="utf-8" + ) + consent = project / ".claude" / "settings.local.json" + consent.write_text(json.dumps({"enabledMcpjsonServers": ["user-kept"]}), encoding="utf-8") + consent_before = consent.read_bytes() + _write_apm_yml(project, [SELF_DEFINED_STDIO]) + + result = _install(apm_binary_path, project, env, "--target", "claude") + + assert result.returncode == 0, result.stdout + result.stderr + servers = _servers(project) + assert servers["user-kept"] == user_entry + assert servers["refs-stdio"] == EXPECTED_STDIO + assert consent.read_bytes() == consent_before + + +def _expand_like_earlier_releases(project: Path) -> None: + """Rewrite .mcp.json the way releases before #3252 wrote it.""" + path = project / ".mcp.json" + path.write_text( + path.read_text(encoding="utf-8").replace("${E2E_TOKEN}", SECRET), encoding="utf-8" + ) + + +def test_reinstall_replaces_values_apm_wrote_earlier( + claude_project: Scenario, apm_binary_path: Path +) -> None: + project, env, _isolated = claude_project + _write_apm_yml(project, [SELF_DEFINED_STDIO, SELF_DEFINED_HTTP]) + assert _install(apm_binary_path, project, env, "--target", "claude").returncode == 0 + _expand_like_earlier_releases(project) + assert SECRET in (project / ".mcp.json").read_text(encoding="utf-8") + + result = _install(apm_binary_path, project, env, "--target", "claude") + + output = result.stdout + result.stderr + assert result.returncode == 0, output + assert _servers(project) == {"refs-stdio": EXPECTED_STDIO, "refs-http": EXPECTED_HTTP} + assert "previously stored as plaintext in .mcp.json" in _flat(output) + assert "Affected: E2E_TOKEN" in _flat(output) + assert "rotate these credentials" in _flat(output) + _assert_no_value([output, (project / "apm.lock.yaml").read_text()], SECRET) + + +def test_reinstall_preserves_values_it_cannot_prove_it_wrote( + claude_project: Scenario, apm_binary_path: Path +) -> None: + project, env, _isolated = claude_project + _write_apm_yml(project, [SELF_DEFINED_STDIO]) + assert _install(apm_binary_path, project, env, "--target", "claude").returncode == 0 + path = project / ".mcp.json" + path.write_text( + path.read_text(encoding="utf-8").replace("${E2E_TOKEN}", OTHER_SECRET), encoding="utf-8" + ) + before = path.read_bytes() + + result = _install(apm_binary_path, project, env, "--target", "claude") + + output = result.stdout + result.stderr + assert result.returncode != 0 + assert path.read_bytes() == before + assert "Preserved that connection" in _flat(output) + assert "Fields args[1], args[3], env.PREFIXED, env.TOKEN hold values" in _flat(output) + assert "rotate those credentials" in _flat(output) + _assert_no_value([output], SECRET, OTHER_SECRET) diff --git a/tests/integration/test_mcp_stdio_env_resolution_e2e.py b/tests/integration/test_mcp_stdio_env_resolution_e2e.py index 7584c50c52..a550795fd7 100644 --- a/tests/integration/test_mcp_stdio_env_resolution_e2e.py +++ b/tests/integration/test_mcp_stdio_env_resolution_e2e.py @@ -1,4 +1,8 @@ -"""E2E guard for self-defined stdio MCP env placeholder resolution.""" +"""E2E guard for self-defined stdio MCP env placeholder resolution. + +Codex resolves references at install time; Claude Code expands them in the +project ``.mcp.json`` at server start, so APM keeps them there (#3252). +""" from __future__ import annotations @@ -75,7 +79,7 @@ def _env_block(project_dir: Path, runtime: str) -> dict[str, str]: ], ) @pytest.mark.parametrize("verbose", [False, True], ids=["default-output", "verbose-output"]) -def test_self_defined_stdio_env_placeholders_resolve_from_process_env_e2e( +def test_self_defined_stdio_env_placeholders_e2e( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, apm_binary_path: str, @@ -83,7 +87,7 @@ def test_self_defined_stdio_env_placeholders_resolve_from_process_env_e2e( signal_dir: str, verbose: bool, ) -> None: - """Real install writes process-env values for self-defined stdio env.""" + """Real install keeps or resolves self-defined stdio env per target.""" project_dir = tmp_path / "project" project_dir.mkdir() (project_dir / signal_dir).mkdir() @@ -101,10 +105,16 @@ def test_self_defined_stdio_env_placeholders_resolve_from_process_env_e2e( ) env_block = _env_block(project_dir, runtime) - assert env_block["APM_STDIO_E2E_TOKEN"] == secret_value - assert env_block["APM_STDIO_E2E_TOKEN"] != "${APM_STDIO_E2E_TOKEN}" assert env_block["APM_STDIO_E2E_UNSET_TOKEN"] == "${APM_STDIO_E2E_UNSET_TOKEN}" assert env_block["LITERAL_VALUE"] == "literal-value" observed_output = f"{result.stdout}\n{result.stderr}" assert secret_value not in observed_output + + if runtime == "claude": + assert env_block["APM_STDIO_E2E_TOKEN"] == "${APM_STDIO_E2E_TOKEN}" + assert secret_value not in (project_dir / ".mcp.json").read_text(encoding="utf-8") + assert "Claude Code will resolve 1 environment variable" in observed_output + assert "APM_STDIO_E2E_UNSET_TOKEN" in observed_output + else: + assert env_block["APM_STDIO_E2E_TOKEN"] == secret_value diff --git a/tests/unit/install/test_mcp_integrator_install_flow.py b/tests/unit/install/test_mcp_integrator_install_flow.py index a419dc47f5..42338b5b22 100644 --- a/tests/unit/install/test_mcp_integrator_install_flow.py +++ b/tests/unit/install/test_mcp_integrator_install_flow.py @@ -453,19 +453,21 @@ def test_self_defined_dep_separated_correctly(self, tmp_path: Path) -> None: assert install.call_args.kwargs["self_defined_deps"] == [sd_dep] @pytest.mark.parametrize( - ("runtime", "signal_dir", "config_relpath"), + ("runtime", "signal_dir", "config_relpath", "expected_token"), [ - ("claude", ".claude", ".mcp.json"), - ("codex", ".codex", ".codex/config.toml"), + # Claude Code expands ${VAR} in .mcp.json at server start (#3252). + ("claude", ".claude", ".mcp.json", "${MY_TOKEN}"), + ("codex", ".codex", ".codex/config.toml", "repro-secret-not-a-real-token"), ], ) - def test_self_defined_stdio_env_placeholders_resolve_from_process_env( + def test_self_defined_stdio_env_placeholders_follow_target_support( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, runtime: str, signal_dir: str, config_relpath: str, + expected_token: str, ) -> None: from apm_cli.integration.mcp_integrator_install import run_mcp_install from apm_cli.models.dependency.mcp import MCPDependency @@ -499,7 +501,7 @@ def test_self_defined_stdio_env_placeholders_resolve_from_process_env( ]["env"] else: env_block = toml.load(config_path)["mcp_servers"]["env-demo"]["env"] - assert env_block["MY_TOKEN"] == "repro-secret-not-a-real-token" + assert env_block["MY_TOKEN"] == expected_token assert env_block["LITERAL_VALUE"] == "literal-value" def test_self_defined_non_stdio_env_remains_env_overrides(self) -> None: diff --git a/tests/unit/install/test_mcp_ownership.py b/tests/unit/install/test_mcp_ownership.py index 8f4d4c9043..625fc57bb4 100644 --- a/tests/unit/install/test_mcp_ownership.py +++ b/tests/unit/install/test_mcp_ownership.py @@ -427,3 +427,99 @@ def test_legacy_adoption_inspects_intellij_obsolete_config(tmp_path) -> None: ) assert adopted == {"intellij": {"managed"}} + + +# --- Claude entries expanded by releases before #3252 ----------------------- + +_EXPANDED_STORED = { + "name": "managed", + "registry": False, + "transport": "stdio", + "command": "uvx", + "args": ["fixture-server", "--token=${OWN_TOKEN}"], + "env": {"TOKEN": "${OWN_TOKEN}", "MODE": "${OWN_MODE:-safe}", "STATIC": "debug"}, +} + + +def _expanded_native(client: ClaudeClientAdapter, value: str) -> dict: + """Render the declaration, then expand ${OWN_TOKEN} like earlier releases did.""" + rendered = client.render_server_config( + MCPIntegrator._build_self_defined_info(MCPDependency.from_dict(_EXPANDED_STORED)) + ) + return json.loads(json.dumps(rendered).replace("${OWN_TOKEN}", value)) + + +def test_expanded_entry_from_earlier_release_is_managed(tmp_path, monkeypatch) -> None: + monkeypatch.setenv("OWN_TOKEN", "own-sentinel") + client = ClaudeClientAdapter(project_root=tmp_path) + native = _expanded_native(client, "own-sentinel") + + result = _compare_managed_claude(client, _EXPANDED_STORED, native, _EXPANDED_STORED) + + assert result is NativeMcpConnectionState.UPDATE_MANAGED + + +@pytest.mark.parametrize("environment_value", [None, "rotated-sentinel"]) +def test_expanded_entry_without_proof_is_a_conflict( + tmp_path, monkeypatch, environment_value: str | None +) -> None: + monkeypatch.delenv("OWN_TOKEN", raising=False) + if environment_value is not None: + monkeypatch.setenv("OWN_TOKEN", environment_value) + client = ClaudeClientAdapter(project_root=tmp_path) + native = _expanded_native(client, "own-sentinel") + + result = _compare_managed_claude(client, _EXPANDED_STORED, native, _EXPANDED_STORED) + + assert result is NativeMcpConnectionState.CONFLICT + + +def test_recorded_write_hash_proves_expanded_entry(tmp_path, monkeypatch) -> None: + from apm_cli.install.mcp.ownership import mcp_native_hash + + monkeypatch.delenv("OWN_TOKEN", raising=False) + client = ClaudeClientAdapter(project_root=tmp_path) + native = _expanded_native(client, "typed-at-a-prompt") + Path(client.get_config_path()).write_text( + json.dumps({"mcpServers": {"managed": native}}), encoding="utf-8" + ) + + result = compare_native_mcp_connection( + client, + "managed", + requested_info=MCPIntegrator._build_self_defined_info( + MCPDependency.from_dict(_EXPANDED_STORED) + ), + stored_config=_EXPANDED_STORED, + managed=True, + native_hash=mcp_native_hash(native), + ) + + assert result is NativeMcpConnectionState.UPDATE_MANAGED + + +def test_unmanaged_expanded_entry_is_never_adopted(tmp_path, monkeypatch) -> None: + monkeypatch.setenv("OWN_TOKEN", "own-sentinel") + client = ClaudeClientAdapter(project_root=tmp_path) + native = _expanded_native(client, "own-sentinel") + + result = _compare_managed_claude( + client, _EXPANDED_STORED, native, _EXPANDED_STORED, managed=False + ) + + assert result is NativeMcpConnectionState.CONFLICT + + +def test_user_scope_expanded_entry_is_not_migrated(tmp_path, monkeypatch) -> None: + monkeypatch.setenv("OWN_TOKEN", "own-sentinel") + project = ClaudeClientAdapter(project_root=tmp_path) + expected = project.render_server_config( + MCPIntegrator._build_self_defined_info(MCPDependency.from_dict(_EXPANDED_STORED)) + ) + native = _expanded_native(project, "own-sentinel") + + assert project.matches_legacy_managed_config(native, expected) is True + assert ( + ClaudeClientAdapter(user_scope=True).matches_legacy_managed_config(native, expected) + is False + ) diff --git a/tests/unit/test_claude_mcp.py b/tests/unit/test_claude_mcp.py index 5493082cac..6069508e33 100644 --- a/tests/unit/test_claude_mcp.py +++ b/tests/unit/test_claude_mcp.py @@ -12,6 +12,7 @@ import pytest from apm_cli.adapters.client.claude import ClaudeClientAdapter +from apm_cli.adapters.client.copilot import CopilotClientAdapter from apm_cli.core.scope import InstallScope from apm_cli.factory import ClientFactory from apm_cli.utils.path_security import PathTraversalError @@ -253,6 +254,304 @@ def test_configure_self_defined_stdio_preserves_env(self): ) +class TestClaudeProjectEnvPlaceholders(unittest.TestCase): + """#3252: the committed ``.mcp.json`` keeps ``${VAR}`` references. + + Claude Code expands them when it starts the server, so APM must not + resolve secrets at install time and bake them into the project. + """ + + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.root = Path(self.tmp.name) + (self.root / ".claude").mkdir() + self.mcp_path = self.root / ".mcp.json" + ClaudeClientAdapter.reset_install_run_state() + CopilotClientAdapter.reset_install_run_state() + + def tearDown(self): + ClaudeClientAdapter.reset_install_run_state() + CopilotClientAdapter.reset_install_run_state() + self.tmp.cleanup() + + def _configure(self, adapter, server_info): + with patch.object(adapter, "registry_client") as mock_registry: + mock_registry.find_server_by_reference.return_value = server_info + self.assertTrue(adapter.configure_mcp_server(server_info["name"])) + + def _configure_project(self, server_info): + self._configure(ClaudeClientAdapter(project_root=self.root), server_info) + return self.mcp_path.read_text(encoding="utf-8") + + @staticmethod + def _stdio_info(name, env): + return {"name": name, "_raw_stdio": {"command": "uvx", "args": [name], "env": env}} + + def test_self_defined_env_references_stay_placeholders(self): + env = { + "JIRA_TOKEN": "${JIRA_TOKEN}", + "JIRA_URL": "${env:JIRA_URL}", + "LEGACY": "", + "LOG_LEVEL": "debug", + } + secrets = { + "JIRA_TOKEN": "token-sentinel", + "JIRA_URL": "https://jira.invalid", + "LEGACY_TOKEN": "legacy-sentinel", + } + with patch.dict(os.environ, secrets): + text = self._configure_project(self._stdio_info("atlassian", env)) + + self.assertEqual( + json.loads(text)["mcpServers"]["atlassian"]["env"], + { + "JIRA_TOKEN": "${JIRA_TOKEN}", + "JIRA_URL": "${JIRA_URL}", + "LEGACY": "${LEGACY_TOKEN}", + "LOG_LEVEL": "debug", + }, + ) + for value in secrets.values(): + self.assertNotIn(value, text) + + def test_registry_required_env_becomes_placeholder(self): + info = { + "name": "npm-srv", + "packages": [ + { + "registry_name": "npm", + "name": "@scope/tool", + "environment_variables": [{"name": "API_KEY", "required": True}], + } + ], + } + with patch.dict(os.environ, {"API_KEY": "token-sentinel"}): + text = self._configure_project(info) + + self.assertEqual(json.loads(text)["mcpServers"]["npm-srv"]["env"]["API_KEY"], "${API_KEY}") + self.assertNotIn("token-sentinel", text) + + def test_reinstall_over_baked_secret_reports_claude_diagnostics(self): + self.mcp_path.write_text( + json.dumps( + { + "mcpServers": { + "atlassian": { + "type": "stdio", + "command": "uvx", + "env": {"JIRA_TOKEN": "baked-sentinel"}, + } + } + } + ), + encoding="utf-8", + ) + with patch.dict(os.environ): + os.environ.pop("JIRA_TOKEN", None) + text = self._configure_project( + self._stdio_info("atlassian", {"JIRA_TOKEN": "${JIRA_TOKEN}"}) + ) + with patch("apm_cli.adapters.client.copilot._rich_warning") as mock_warn: + ClaudeClientAdapter.emit_install_run_summary() + + self.assertNotIn("baked-sentinel", text) + joined = "\n".join(call.args[0] for call in mock_warn.call_args_list) + self.assertIn("previously stored as plaintext in .mcp.json", joined) + self.assertIn("Claude Code will resolve 1 environment variable", joined) + self.assertIn("before running 'claude'", joined) + self.assertIn("JIRA_TOKEN", joined) + # Claude's diagnostics must not leak into Copilot CLI's summary. + self.assertEqual(CopilotClientAdapter._security_upgraded_keys, set()) + self.assertEqual(CopilotClientAdapter._unset_env_keys_by_server, {}) + + def test_user_scope_keeps_install_time_resolution(self): + home = self.root / "home" + home.mkdir() + with ( + patch.object(Path, "home", return_value=home), + patch.dict(os.environ, {"CLAUDE_CONFIG_DIR": "", "TOK": "user-secret"}), + ): + self._configure( + ClaudeClientAdapter(user_scope=True), self._stdio_info("srv", {"TOK": "${TOK}"}) + ) + + data = json.loads((home / ".claude.json").read_text(encoding="utf-8")) + self.assertEqual(data["mcpServers"]["srv"]["env"], {"TOK": "user-secret"}) + + def test_command_and_url_references_use_claude_syntax(self): + stdio = { + "name": "cmd-demo", + "_raw_stdio": {"command": "${env:DEMO_CMD}", "args": [], "env": {}}, + } + remote = { + "name": "url-demo", + "remotes": [{"url": "https:///${env:DEMO_PATH}", "transport_type": "http"}], + } + self._configure_project(stdio) + data = json.loads(self._configure_project(remote))["mcpServers"] + + self.assertEqual(data["cmd-demo"]["command"], "${DEMO_CMD}") + self.assertEqual(data["url-demo"]["url"], "https://${DEMO_HOST}/${DEMO_PATH}") + + def test_defaults_are_kept_in_every_field(self): + stdio = { + "name": "defaults", + "_raw_stdio": { + "command": "${DEMO_CMD:-uvx}", + "args": ["--mode=${DEMO_MODE:-safe}"], + "env": {"LEVEL": "${DEMO_LEVEL:-info}"}, + }, + } + remote = { + "name": "remote-defaults", + "remotes": [ + { + "url": "https://${DEMO_HOST:-mcp.example.invalid}/mcp", + "transport_type": "http", + "headers": [{"name": "X-Region", "value": "${DEMO_REGION:-eu}"}], + } + ], + } + self._configure_project(stdio) + data = json.loads(self._configure_project(remote))["mcpServers"] + + self.assertEqual(data["defaults"]["command"], "${DEMO_CMD:-uvx}") + self.assertEqual(data["defaults"]["args"], ["--mode=${DEMO_MODE:-safe}"]) + self.assertEqual(data["defaults"]["env"], {"LEVEL": "${DEMO_LEVEL:-info}"}) + self.assertEqual( + data["remote-defaults"]["url"], "https://${DEMO_HOST:-mcp.example.invalid}/mcp" + ) + self.assertEqual(data["remote-defaults"]["headers"], {"X-Region": "${DEMO_REGION:-eu}"}) + + def test_missing_variable_summary_covers_every_field_but_skips_defaults(self): + info = { + "name": "missing", + "_raw_stdio": { + "command": "${MISSING_CMD}", + "args": ["--token=${MISSING_ARG}", "--mode=${DEFAULTED_ARG:-safe}"], + "env": {"TOKEN": "${MISSING_ENV}"}, + }, + } + with patch.dict(os.environ): + for name in ("MISSING_CMD", "MISSING_ARG", "DEFAULTED_ARG", "MISSING_ENV"): + os.environ.pop(name, None) + self._configure_project(info) + with patch("apm_cli.adapters.client.copilot._rich_warning") as mock_warn: + ClaudeClientAdapter.emit_install_run_summary() + + joined = "\n".join(call.args[0] for call in mock_warn.call_args_list) + self.assertIn("MISSING_ARG, MISSING_CMD, MISSING_ENV", joined) + self.assertNotIn("DEFAULTED_ARG", joined) + + def test_upgrade_notice_names_values_replaced_in_args_and_headers(self): + self.mcp_path.write_text( + json.dumps( + { + "mcpServers": { + "remote": { + "type": "http", + "url": "https://mcp.example.invalid/mcp", + "headers": {"Authorization": "Bearer baked-sentinel"}, + } + } + } + ), + encoding="utf-8", + ) + remote = { + "name": "remote", + "remotes": [ + { + "url": "https://mcp.example.invalid/mcp", + "transport_type": "http", + "headers": [{"name": "Authorization", "value": "Bearer ${REMOTE_TOKEN}"}], + } + ], + } + with patch.dict(os.environ, {"REMOTE_TOKEN": "baked-sentinel"}): + text = self._configure_project(remote) + with patch("apm_cli.adapters.client.copilot._rich_warning") as mock_warn: + ClaudeClientAdapter.emit_install_run_summary() + + self.assertNotIn("baked-sentinel", text) + joined = "\n".join(call.args[0] for call in mock_warn.call_args_list) + self.assertIn("Affected: REMOTE_TOKEN", joined) + self.assertIn("rotate these credentials", joined) + self.assertNotIn("baked-sentinel", joined) + + +_LEGACY_EXPECTED = { + "type": "stdio", + "command": "uvx", + "args": ["--token=${OLD_TOKEN}"], + "env": {"TOKEN": "${OLD_TOKEN}", "MODE": "${OLD_MODE:-safe}", "STATIC": "debug"}, +} + + +class TestClaudeLegacyEntries(unittest.TestCase): + """Entries written before #3252 kept expanded values in ``.mcp.json``.""" + + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.root = Path(self.tmp.name) + (self.root / ".claude").mkdir() + self.adapter = ClaudeClientAdapter(project_root=self.root) + + def tearDown(self): + self.tmp.cleanup() + + def _native(self, value): + return json.loads(json.dumps(_LEGACY_EXPECTED).replace("${OLD_TOKEN}", value)) + + def test_expansion_from_current_environment_is_recognized(self): + with patch.dict(os.environ, {"OLD_TOKEN": "old-sentinel"}): + self.assertTrue( + self.adapter.matches_legacy_managed_config( + self._native("old-sentinel"), _LEGACY_EXPECTED + ) + ) + + def test_other_values_are_not_recognized(self): + with patch.dict(os.environ, {"OLD_TOKEN": "rotated"}): + self.assertFalse( + self.adapter.matches_legacy_managed_config( + self._native("old-sentinel"), _LEGACY_EXPECTED + ) + ) + + def test_unresolved_reference_in_old_spelling_is_recognized(self): + with patch.dict(os.environ): + os.environ.pop("OLD_TOKEN", None) + native = self._native("${env:OLD_TOKEN}") + self.assertTrue(self.adapter.matches_legacy_managed_config(native, _LEGACY_EXPECTED)) + + def test_unresolved_registry_variable_written_empty_is_recognized(self): + expected = {"type": "stdio", "command": "npx", "args": [], "env": {"API_KEY": "${API_KEY}"}} + native = {"type": "stdio", "command": "npx", "args": [], "env": {"API_KEY": ""}} + with patch.dict(os.environ): + os.environ.pop("API_KEY", None) + self.assertTrue(self.adapter.matches_legacy_managed_config(native, expected)) + + def test_extra_or_missing_keys_are_not_recognized(self): + native = self._native("old-sentinel") + native["env"]["EXTRA"] = "user-added" + with patch.dict(os.environ, {"OLD_TOKEN": "old-sentinel"}): + self.assertFalse(self.adapter.matches_legacy_managed_config(native, _LEGACY_EXPECTED)) + + def test_conflict_guidance_names_fields_without_values(self): + native = self._native("someone-elses-value") + self.adapter.update_config({"legacy": native}) + guidance = self.adapter.native_conflict_guidance("legacy", _LEGACY_EXPECTED) + + self.assertIn("Fields args[0], env.TOKEN hold values", guidance) + self.assertIn("rotate those credentials", guidance) + self.assertNotIn("someone-elses-value", guidance) + + def test_conflict_guidance_is_silent_without_expanded_fields(self): + self.adapter.update_config({"legacy": dict(_LEGACY_EXPECTED, command="other")}) + self.assertIsNone(self.adapter.native_conflict_guidance("legacy", _LEGACY_EXPECTED)) + + class TestClaudeClientAdapterUser(unittest.TestCase): """User scope: ``~/.claude.json`` top-level ``mcpServers``.""" diff --git a/tests/unit/test_copilot_adapter.py b/tests/unit/test_copilot_adapter.py index 1a3b5f1302..569b12b44c 100644 --- a/tests/unit/test_copilot_adapter.py +++ b/tests/unit/test_copilot_adapter.py @@ -534,19 +534,19 @@ class TestSiblingAdaptersUnchanged(unittest.TestCase): behaviour. Runtime-capable targets are tested separately after an individual audit.""" - def _adapter(self, cls): + def _adapter(self, cls, **kwargs): with ( patch("apm_cli.adapters.client.copilot.SimpleRegistryClient"), patch("apm_cli.adapters.client.copilot.RegistryIntegration"), ): - return cls() + return cls(**kwargs) - def test_claude_still_resolves_to_literal(self): - """Claude Desktop config does NOT support runtime substitution -- - this adapter MUST keep resolving.""" + def test_claude_user_scope_still_resolves_to_literal(self): + """Only Claude's project ``.mcp.json`` keeps placeholders (#3252); + user-scope ``~/.claude.json`` keeps install-time resolution.""" from apm_cli.adapters.client.claude import ClaudeClientAdapter - adapter = self._adapter(ClaudeClientAdapter) + adapter = self._adapter(ClaudeClientAdapter, user_scope=True) self.assertFalse(adapter._supports_runtime_env_substitution) with patch.dict(os.environ, {"MY_TOKEN": "literal-value"}, clear=False): result = adapter._resolve_env_variable( @@ -704,9 +704,9 @@ class TestLegacyModeStdioEnvBlock(unittest.TestCase): pre-fix latent bug returns ``{}`` and every self-defined stdio MCP server silently loses its env block. - Claude is exercised as the representative legacy-mode subclass; the - method now lives on ``MCPClientAdapter`` so the contract is shared - with every adapter that inherits the base. + Claude at user scope is exercised as the representative legacy-mode + subclass; the method now lives on ``MCPClientAdapter`` so the contract + is shared with every adapter that inherits the base. """ def _adapter(self): @@ -716,7 +716,7 @@ def _adapter(self): patch("apm_cli.adapters.client.copilot.SimpleRegistryClient"), patch("apm_cli.adapters.client.copilot.RegistryIntegration"), ): - return ClaudeClientAdapter() + return ClaudeClientAdapter(user_scope=True) def test_dict_shape_resolves_all_three_placeholder_syntaxes(self): adapter = self._adapter() diff --git a/tests/unit/test_github_mcp_auth_policy.py b/tests/unit/test_github_mcp_auth_policy.py index 638acda09d..221b7cf6aa 100644 --- a/tests/unit/test_github_mcp_auth_policy.py +++ b/tests/unit/test_github_mcp_auth_policy.py @@ -9,6 +9,7 @@ import pytest +from apm_cli.adapters.client.claude import ClaudeClientAdapter from apm_cli.adapters.client.copilot import CopilotClientAdapter from apm_cli.adapters.client.cursor import CursorClientAdapter from apm_cli.adapters.client.intellij import IntelliJClientAdapter @@ -23,6 +24,7 @@ "adapter_class,automatic", [ (CopilotClientAdapter, "Bearer ${GITHUB_TOKEN}"), + (ClaudeClientAdapter, "Bearer ${GITHUB_TOKEN}"), (IntelliJClientAdapter, "Bearer ${env:GITHUB_TOKEN}"), (WindsurfClientAdapter, "Bearer ambient-sentinel"), ], @@ -87,6 +89,7 @@ def test_shared_policy_preserves_registry_fallback_and_manifest_precedence( elif explicit: expected = { CopilotClientAdapter: "${USER_PAT}", + ClaudeClientAdapter: "${USER_PAT}", IntelliJClientAdapter: "${env:USER_PAT}", WindsurfClientAdapter: "user-sentinel", }[adapter_class]