diff --git a/CHANGELOG.md b/CHANGELOG.md index ea197f037..ca1bb4f1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Gemini CLI: `apm install -g --mcp NAME` now correctly writes to `~/.gemini/settings.json` (user scope) and `apm install` from outside the target project writes to `/.gemini/settings.json` instead of `cwd`. Previously `--global` had no effect on Gemini and project-scope writes silently landed in the wrong directory. The matching opt-in gate and cleanup paths in `MCPIntegrator` are aligned in the same change. (#1299) +- Codex, Gemini, and Cursor adapters now resolve `${VAR}`, `${env:VAR}`, and legacy `` environment placeholders in stdio server arguments, matching the behavior already present in the Copilot CLI adapter. (#1266) - `apm install --target claude` now preserves self-defined stdio MCP `env` values from `apm.yml` and writes non-string values such as `PORT: 3000` and `DEBUG: false` as MCP-compatible strings. (#1222) - Non-skill integrators (agent, instruction, prompt, command, hook script-copy) silently adopt byte-identical pre-existing files so a degraded `deployed_files=[]` lockfile no longer permanently blocks installs gated by `required-packages-deployed`. (#1313) - `apm audit` drift check now returns skip-with-info (`passed=True`) when the install cache is cold, instead of failing the audit; bare `apm audit` surfaces the skip reason on stderr so CI pipelines that have not yet run `apm install` are not incorrectly red-marked. (#1289) diff --git a/docs/src/content/docs/reference/manifest-schema.md b/docs/src/content/docs/reference/manifest-schema.md index 55f4f187d..254b33f59 100644 --- a/docs/src/content/docs/reference/manifest-schema.md +++ b/docs/src/content/docs/reference/manifest-schema.md @@ -442,7 +442,7 @@ dependencies: Values in `headers` and `env` may contain three placeholder syntaxes. APM resolves them per-target so secrets stay out of generated config files where possible. -| Syntax | Source | VS Code | Copilot CLI / Codex | +| Syntax | Source | VS Code | Copilot CLI / Codex / Gemini / Cursor | |---|---|---|---| | `${VAR}` | host environment | Translated to `${env:VAR}` (resolved at server-start by VS Code) | Resolved at install time from env (or interactive prompt) | | `${env:VAR}` | host environment | Native; passed through verbatim | Resolved at install time from env (or interactive prompt) | @@ -450,11 +450,10 @@ Values in `headers` and `env` may contain three placeholder syntaxes. APM resolv | `` (legacy) | host environment | Not recognized | Resolved at install time (kept for back-compat) | - **VS Code** has native `${env:VAR}` and `${input:VAR}` interpolation, so APM emits placeholders rather than baking secrets into `mcp.json`. Bare `${VAR}` is normalized to `${env:VAR}` for you. -- **Copilot CLI** has no runtime interpolation, so APM resolves `${VAR}`, `${env:VAR}`, and the legacy `` 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. -- **Codex** currently resolves only the legacy `` placeholder at install time; `${VAR}` / `${env:VAR}` are passed through verbatim. -- **Recommended:** Use `${VAR}` or `${env:VAR}` in all new manifests. They work on every target that supports remote MCP servers. `` is legacy and only resolved by Copilot CLI and Codex; in VS Code it would silently render as literal text. -- **Registry-backed servers**: APM auto-generates input prompts from registry metadata for `${input:...}`. -- **Self-defined servers**: APM detects `${input:...}` patterns in `apm.yml` and generates matching input definitions automatically. +- **Copilot CLI, Codex, Gemini, and Cursor** have no runtime interpolation, so APM resolves `${VAR}`, `${env:VAR}`, and the legacy `` 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. +- **Recommended:** Use `${VAR}` or `${env:VAR}` in all new manifests — they work on every target that supports remote MCP servers. `` is legacy; in VS Code it would silently render as literal text in the generated config. +- **Registry-backed servers** — APM auto-generates input prompts from registry metadata for `${input:...}`. +- **Self-defined servers** — APM detects `${input:...}` patterns in `apm.yml` and generates matching input definitions automatically. GitHub Actions templates (`${{ ... }}`) are intentionally left untouched. diff --git a/src/apm_cli/adapters/client/base.py b/src/apm_cli/adapters/client/base.py index 59cf18d2a..91c12f15f 100644 --- a/src/apm_cli/adapters/client/base.py +++ b/src/apm_cli/adapters/client/base.py @@ -4,6 +4,9 @@ import re from abc import ABC, abstractmethod from pathlib import Path +from typing import ClassVar + +from ...utils.console import _rich_warning _INPUT_VAR_RE = re.compile(r"\$\{input:([^}]+)\}") @@ -14,6 +17,80 @@ # variable handling, so existing _INPUT_VAR_RE call sites are unaffected. _ENV_VAR_RE = re.compile(r"\$\{(?:env:)?([A-Za-z_][A-Za-z0-9_]*)\}") +# Superset of _ENV_VAR_RE that also matches the legacy ```` syntax +# (uppercase identifier only). Used as the single-pass translation target so +# resolved values are NOT re-scanned -- a literal value whose text happens to +# contain ``${...}`` does not get recursively expanded. ``${input:...}`` is +# intentionally not matched here so input-variable handling stays disjoint. +_ENV_PLACEHOLDER_RE = re.compile(r"<([A-Z_][A-Z0-9_]*)>|" + _ENV_VAR_RE.pattern) + +# Detects the legacy ```` placeholder syntax only. Used to aggregate +# deprecation warnings across all servers in a single install run. +_LEGACY_ANGLE_VAR_RE = re.compile(r"<([A-Z_][A-Z0-9_]*)>") + + +def _translate_env_placeholder(value): + """Pure-textual translation of env-var placeholders to the canonical + ``${VAR}`` runtime-substitution syntax. + + Security-critical helper for issue #1152: MUST NOT read ``os.environ`` + and MUST NOT resolve placeholders to literal values. Runtimes that + support runtime substitution (Copilot CLI) resolve ``${VAR}`` from the + host environment at server-start, so APM emits placeholders verbatim + rather than baking secrets to disk. + + Translations: + ``${env:VAR}`` -> ``${VAR}`` (strip ``env:`` prefix) + ``${VAR}`` -> ``${VAR}`` (no-op) + ```` -> ``${VAR}`` (legacy syntax migration) + ``${VAR:-default}``-> passthrough (regex doesn't match) + ``$VAR`` (bare) -> passthrough (regex doesn't match) + ``${input:foo}`` -> passthrough (regex doesn't match) + non-string -> passthrough + + Idempotent: applying twice yields the same result as applying once. + """ + if not isinstance(value, str): + return value + + def _to_brace(match): + # group(1) = legacy ; group(2) = ${VAR} / ${env:VAR} + var_name = match.group(1) or match.group(2) + return "${" + var_name + "}" + + return _ENV_PLACEHOLDER_RE.sub(_to_brace, value) + + +def _extract_legacy_angle_vars(value): + """Return the set of legacy ```` names present in *value*. + + Used to aggregate deprecation warnings across all servers in a single + install run, so authors see one helpful list instead of one warning per + occurrence. + """ + if not isinstance(value, str): + return set() + return set(_LEGACY_ANGLE_VAR_RE.findall(value)) + + +def _has_env_placeholder(value): + """True if *value* is a string containing any recognised env-var + placeholder syntax (``${VAR}``, ``${env:VAR}``, or legacy ````). + + Used to distinguish placeholder-sourced env values (which translate) + from hardcoded literal defaults (which stay literal). + """ + if not isinstance(value, str): + return False + return bool(_ENV_PLACEHOLDER_RE.search(value)) + + +def _stringify_env_literal(value): + """Return MCP env literal values in the manifest ``map`` shape.""" + if isinstance(value, bool): + return str(value).lower() + return str(value) + class MCPClientAdapter(ABC): """Base adapter for MCP clients.""" @@ -42,6 +119,13 @@ class MCPClientAdapter(ABC): # so that ``apm install --global`` can install MCP servers to them. supports_user_scope: bool = False + # Whether the target runtime resolves ``${VAR}`` placeholders from the + # host environment at server-start time. Adapters that opt in (Copilot + # CLI) emit placeholders verbatim so secrets never touch disk; legacy + # adapters resolve to literal values at install time via env_overrides + # -> os.environ -> optional interactive prompt. See issue #1152. + _supports_runtime_env_substitution: bool = False + def __init__( self, project_root: Path | str | None = None, @@ -58,6 +142,13 @@ def __init__( """ self._project_root = Path(project_root) if project_root is not None else None self.user_scope = user_scope + # Per-server tracking populated by the env-resolution helpers and + # consumed by ``configure_mcp_server`` for the post-install summary + # and the aggregated legacy-syntax deprecation warning. Defined on + # the base so every adapter has the attributes regardless of which + # subclass path constructed it. + self._last_env_placeholder_keys: set[str] = set() + self._last_legacy_angle_vars: set[str] = set() @property def project_root(self) -> Path: @@ -181,8 +272,8 @@ def _warn_input_variables(mapping, server_name, runtime_label): if var_id in seen: continue seen.add(var_id) - print( - f"[!] Warning: ${{input:{var_id}}} in server " + _rich_warning( + f"${{input:{var_id}}} in server " f"'{server_name}' will not be resolved -- " f"{runtime_label} does not support input variable prompts" ) @@ -196,3 +287,257 @@ def normalize_project_arg(self, value): ): return "." return value + + # -- Env-var placeholder resolution ------------------------------------- + # GitHub MCP server defaults: not secrets, preserved literal in translate + # mode and used as fallbacks in legacy mode. The defaults apply regardless + # of which client CLI runs the server, so they live on the base. + _DEFAULT_GITHUB_ENV: ClassVar[dict[str, str]] = { + "GITHUB_TOOLSETS": "context", + "GITHUB_DYNAMIC_TOOLSETS": "1", + } + + @staticmethod + def _should_skip_env_prompts(env_overrides): + """True when the caller has already collected env vars (managed mode), + when APM_E2E_TESTS is set, or when stdin/stdout is not a TTY. + + Centralising this policy keeps the resolver paths consistent and + avoids subtle drift between ``_resolve_environment_variables`` and + ``_resolve_env_variable``. + """ + import sys + + if env_overrides: + return True + if os.getenv("APM_E2E_TESTS") == "1": + return True + return not (sys.stdin.isatty() and sys.stdout.isatty()) + + def _resolve_environment_variables(self, env_vars, env_overrides=None): + """Resolve (or translate) declared environment variables. + + Behaviour follows ``self._supports_runtime_env_substitution``: + translate-mode (Copilot CLI) emits ``${VAR}`` placeholders verbatim + so the runtime resolves them at server-start (see issue #1152); + legacy-mode resolves placeholders to literal values via env_overrides + -> os.environ -> optional interactive prompt. + + Args: + env_vars: Either a ``dict[name, value-or-placeholder]`` from a + self-defined stdio dep (``_raw_stdio["env"]``), or a + ``list[{name, description, required}]`` from the registry. + env_overrides: Pre-collected env-var overrides (ignored in + translate mode). + + Returns: + dict: ``{name: value}`` -- placeholder string in translate + mode, literal value in legacy mode. + """ + # ---- translate mode, dict shape (self-defined stdio in apm.yml) ---- + if isinstance(env_vars, dict) and self._supports_runtime_env_substitution: + # Value type is intentionally untyped: most entries are translated + # placeholder strings, but non-string values (e.g. an int/bool + # YAML scalar) are passed through verbatim and serialised by the + # adapter's config writer (JSON/TOML). + translated: dict = {} + placeholder_keys: list[str] = [] + for name, raw_value in env_vars.items(): + if not name: + continue + if not isinstance(raw_value, str): + translated[name] = raw_value + continue + if _has_env_placeholder(raw_value): + self._last_legacy_angle_vars.update(_extract_legacy_angle_vars(raw_value)) + translated[name] = _translate_env_placeholder(raw_value) + # Record every ${VAR} in the translated value (handles + # both ${env:VAR} -> ${VAR} and bare ${VAR} cases). + placeholder_keys.extend( + m.group(1) for m in _ENV_VAR_RE.finditer(translated[name]) + ) + elif ( + name in self._DEFAULT_GITHUB_ENV and raw_value == self._DEFAULT_GITHUB_ENV[name] + ): + translated[name] = raw_value + else: + # Literal value present in apm.yml -- replace with a + # runtime placeholder so the secret never touches disk. + translated[name] = "${" + name + "}" + placeholder_keys.append(name) + self._last_env_placeholder_keys = set(placeholder_keys) + return translated + + # ---- translate mode, registry list shape ---- + if self._supports_runtime_env_substitution: + resolved: dict[str, str] = {} + placeholder_keys: list[str] = [] + for env_var in env_vars: + if not isinstance(env_var, dict): + continue + name = env_var.get("name", "") + if not name: + continue + if name in self._DEFAULT_GITHUB_ENV: + resolved[name] = self._DEFAULT_GITHUB_ENV[name] + else: + resolved[name] = "${" + name + "}" + placeholder_keys.append(name) + self._last_env_placeholder_keys = set(placeholder_keys) + return resolved + + # ---- legacy mode, dict shape (self-defined stdio in apm.yml) ---- + # Issue #1266 / #1222: ``_raw_stdio["env"]`` is a plain dict. Each + # value is resolved via the same single-value pipeline used for + # header values so all three placeholder syntaxes (````, + # ``${VAR}``, ``${env:VAR}``) behave consistently across adapters. + # + # Note the deliberate semantic divergence from the legacy-list branch + # below: empty strings authored in apm.yml are preserved as-is and + # ``_DEFAULT_GITHUB_ENV`` fallbacks are NOT applied, because a value + # explicitly written by the user expresses intent, whereas an empty + # value coming from ``env_overrides`` / ``os.environ`` for a + # registry-declared schema entry means "no value supplied, use the + # default if one exists". + if isinstance(env_vars, dict): + resolved = {} + for name, value in env_vars.items(): + if not name: + continue + if isinstance(value, str): + resolved[name] = self._resolve_env_variable( + name, value, env_overrides=env_overrides + ) + elif value is not None: + resolved[name] = str(value) + return resolved + + # ---- legacy mode, registry list shape ---- + from rich.prompt import Prompt + + env_overrides = env_overrides or {} + skip_prompting = self._should_skip_env_prompts(env_overrides) + + # Variables explicitly provided with empty values mean "use the default". + empty_value_vars = {k for k, v in env_overrides.items() if not v or not v.strip()} + + resolved = {} + for env_var in env_vars: + if not isinstance(env_var, dict): + continue + name = env_var.get("name", "") + if not name: + continue + required = env_var.get("required", True) + + value = env_overrides.get(name) or os.getenv(name) + if not value and required and not skip_prompting: + prompt_text = f"Enter value for {name}" + if description := env_var.get("description", ""): + prompt_text += f" ({description})" + value = Prompt.ask( + prompt_text, + password="token" in name.lower() or "key" in name.lower(), + ) + + if value and value.strip(): + resolved[name] = value + elif name in self._DEFAULT_GITHUB_ENV and ( + name in empty_value_vars or not required or skip_prompting + ): + resolved[name] = self._DEFAULT_GITHUB_ENV[name] + + return resolved + + def _resolve_env_variable(self, name, value, env_overrides=None): + """Resolve (or translate) a single env-var value. + + Used for header values and for individual entries in dict-shape + env blocks. The ``name`` parameter is currently unused by the + method body but kept in the signature because every call site + (headers, dict iteration) already has the name in hand, and + passing it preserves call-site symmetry with future hooks that + may want to dispatch on it. + + Args: + name: Env-var name (currently unused, see above). + value: Env-var value possibly containing placeholders. + env_overrides: Pre-collected overrides (ignored in translate mode). + """ + if self._supports_runtime_env_substitution: + legacy_keys = _extract_legacy_angle_vars(value) + self._last_legacy_angle_vars.update(legacy_keys) + self._last_env_placeholder_keys.update(legacy_keys) + for match in _ENV_VAR_RE.finditer(value): + self._last_env_placeholder_keys.add(match.group(1)) + return _translate_env_placeholder(value) + + from rich.prompt import Prompt + + env_overrides = env_overrides or {} + skip_prompting = self._should_skip_env_prompts(env_overrides) + + # Three accepted placeholder syntaxes resolved against + # env_overrides -> os.environ -> optional interactive prompt. + # Single-pass substitution preserves the legacy ```` semantics: + # resolved values are NOT re-scanned for further expansion. + def _replace(match): + env_name = match.group(1) or match.group(2) + env_value = env_overrides.get(env_name) or os.getenv(env_name) + if not env_value and not skip_prompting: + env_value = Prompt.ask( + f"Enter value for {env_name}", + password="token" in env_name.lower() or "key" in env_name.lower(), + ) + return env_value if env_value else match.group(0) + + return _ENV_PLACEHOLDER_RE.sub(_replace, value) + + def _resolve_variable_placeholders(self, value, resolved_env, runtime_vars): + """Resolve env-var and APM template placeholders in argument strings. + + Translate mode rewrites all three env-var placeholder syntaxes to + ``${VAR}`` (so the runtime can resolve them at server-start); legacy + mode resolves only the legacy ```` form against ``resolved_env`` + and leaves the newer ``${VAR}`` / ``${env:VAR}`` syntaxes untouched + for backward compatibility. APM template variables (``{runtime_var}``) + are always resolved at install time because they are an APM-internal + concept the target runtime cannot interpret. + + Args: + value: String possibly containing placeholders. + resolved_env: Resolved env-var literals (legacy mode) or + placeholder strings (translate mode). + runtime_vars: Resolved APM template variables. + + Returns: + str: ``value`` with placeholders translated or resolved. + """ + if not value: + return value + + processed = str(value) + + if self._supports_runtime_env_substitution: + self._last_legacy_angle_vars.update(_extract_legacy_angle_vars(processed)) + processed = _translate_env_placeholder(processed) + else: + # Resolve only the legacy ```` form; newer syntaxes are + # preserved verbatim for backward compatibility. + def _replace_legacy_angle(match): + return resolved_env.get(match.group(1), match.group(0)) + + processed = _LEGACY_ANGLE_VAR_RE.sub(_replace_legacy_angle, processed) + + # Resolve APM ``{runtime_var}`` template variables. The negative + # lookbehind on ``$`` ensures we never accidentally match the brace + # of an already-translated ``${VAR}`` env placeholder. + if runtime_vars: + runtime_pattern = re.compile(r"(?``, ``${VAR}``, ``${env:VAR}``) are resolved at + # install time before being written to ~/.codex/config.toml. + # See issue #1266. raw = server_info.get("_raw_stdio") if raw: config["command"] = raw["command"] - config["args"] = [self.normalize_project_arg(arg) for arg in raw["args"]] + resolved_env_for_args: dict = {} if raw.get("env"): - config["env"] = raw["env"] + resolved_env_for_args = self._resolve_environment_variables( + raw["env"], env_overrides=env_overrides + ) + config["env"] = resolved_env_for_args self._warn_input_variables(raw["env"], server_info.get("name", ""), "Codex CLI") + + def _process_stdio_arg(arg): + if isinstance(arg, str): + arg = self._resolve_variable_placeholders( + arg, resolved_env_for_args, runtime_vars + ) + return self.normalize_project_arg(arg) + + config["args"] = [_process_stdio_arg(arg) for arg in raw.get("args") or []] return config # Note: Remote servers (SSE type) are handled in configure_mcp_server and rejected early @@ -461,48 +481,6 @@ def _process_environment_variables(self, env_vars, env_overrides=None): return resolved - def _resolve_variable_placeholders(self, value, resolved_env, runtime_vars): - """Resolve both environment and runtime variable placeholders in values. - - Args: - value (str): Value that may contain placeholders like or {runtime_var} - resolved_env (dict): Dictionary of resolved environment variables. - runtime_vars (dict): Dictionary of resolved runtime variables. - - Returns: - str: Processed value with actual variable values. - """ - import re - - if not value: - return value - - processed = str(value) - - # Replace with actual values from resolved_env (for Docker env vars) - env_pattern = r"<([A-Z_][A-Z0-9_]*)>" - - def replace_env_var(match): - env_name = match.group(1) - return resolved_env.get(env_name, match.group(0)) # Return original if not found - - processed = re.sub(env_pattern, replace_env_var, processed) - - # Replace {runtime_var} with actual values from runtime_vars - runtime_pattern = r"\{([a-zA-Z_][a-zA-Z0-9_]*)\}" - - def replace_runtime_var(match): - var_name = match.group(1) - return runtime_vars.get(var_name, match.group(0)) # Return original if not found - - processed = re.sub(runtime_pattern, replace_runtime_var, processed) - - return processed - - def _resolve_env_placeholders(self, value, resolved_env): - """Legacy method for backward compatibility. Use _resolve_variable_placeholders instead.""" - return self._resolve_variable_placeholders(value, resolved_env, {}) - def _ensure_docker_env_flags(self, base_args, env_vars): """Ensure all environment variables are represented as -e flags in Docker args. diff --git a/src/apm_cli/adapters/client/copilot.py b/src/apm_cli/adapters/client/copilot.py index ae527f7b5..0f06d07af 100644 --- a/src/apm_cli/adapters/client/copilot.py +++ b/src/apm_cli/adapters/client/copilot.py @@ -7,7 +7,6 @@ import json import os -import re from pathlib import Path from typing import ClassVar @@ -19,85 +18,15 @@ from ...registry.integration import RegistryIntegration from ...utils.console import _rich_warning from ...utils.github_host import is_github_hostname -from .base import _ENV_VAR_RE, MCPClientAdapter - -# Combined env-var placeholder regex covering all three syntaxes Copilot accepts: -# legacy APM (group 1, uppercase only) -# ${VARNAME} POSIX shell (group 2) -# ${env:VARNAME} VS Code-flavored (group 2) -# A single-pass substitution preserves the original ```` semantics: -# resolved values are NOT re-scanned, so a token whose literal text contains -# ``${...}`` does not get recursively expanded. Module-level compile avoids -# per-call cost. ``${input:...}`` is intentionally not matched here. -_COPILOT_ENV_RE = re.compile(r"<([A-Z_][A-Z0-9_]*)>|" + _ENV_VAR_RE.pattern) - -# Detects the legacy ```` placeholder syntax. Used both for translation -# and for emitting an aggregated deprecation warning, mirroring the analogous -# pattern in ``vscode.py``. -_LEGACY_ANGLE_VAR_RE = re.compile(r"<([A-Z_][A-Z0-9_]*)>") - - -def _translate_env_placeholder(value): - """Pure-textual translation of env-var placeholders to Copilot CLI's - native runtime substitution syntax (``${VAR}``). - - This is the security-critical helper for issue #1152: it MUST NOT read - ``os.environ`` and MUST NOT resolve placeholders to their literal values. - Copilot CLI resolves ``${VAR}`` from the host environment at server-start - time, so APM emits placeholders verbatim rather than baking secrets into - ``~/.copilot/mcp-config.json``. - - Translations: - ``${env:VAR}`` -> ``${VAR}`` (strip ``env:`` prefix) - ``${VAR}`` -> ``${VAR}`` (no-op) - ```` -> ``${VAR}`` (legacy syntax migration) - ``${VAR:-default}``-> passthrough (regex doesn't match) - ``$VAR`` (bare) -> passthrough (regex doesn't match) - ``${input:foo}`` -> passthrough (regex doesn't match) - non-string -> passthrough - - The translation is idempotent: applying it twice produces the same - result as applying it once. - """ - if not isinstance(value, str): - return value - - def _to_brace(match): - # group(1) = legacy ; group(2) = ${VAR} / ${env:VAR} - var_name = match.group(1) or match.group(2) - return "${" + var_name + "}" - - return _COPILOT_ENV_RE.sub(_to_brace, value) - - -def _extract_legacy_angle_vars(value): - """Return the set of legacy ```` names present in *value*. - - Used to aggregate deprecation warnings across all servers in a single - install run, so authors see one helpful list instead of one warning per - occurrence. - """ - if not isinstance(value, str): - return set() - return set(_LEGACY_ANGLE_VAR_RE.findall(value)) - - -def _has_env_placeholder(value): - """True if *value* is a string containing any recognised env-var - placeholder syntax (``${VAR}``, ``${env:VAR}``, or legacy ````). - Used to distinguish placeholder-sourced env values (which translate) - from hardcoded literal defaults (which stay literal). - """ - if not isinstance(value, str): - return False - return bool(_COPILOT_ENV_RE.search(value)) - - -def _stringify_env_literal(value): - """Return MCP env literal values in the manifest ``map`` shape.""" - if isinstance(value, bool): - return str(value).lower() - return str(value) +from .base import ( + _ENV_PLACEHOLDER_RE, + _ENV_VAR_RE, + MCPClientAdapter, + _extract_legacy_angle_vars, + _has_env_placeholder, + _stringify_env_literal, + _translate_env_placeholder, +) class CopilotClientAdapter(MCPClientAdapter): @@ -169,14 +98,6 @@ def __init__( super().__init__(project_root=project_root, user_scope=user_scope) self.registry_client = SimpleRegistryClient(registry_url) self.registry_integration = RegistryIntegration(registry_url) - # Per-server tracking of placeholder-sourced env-var keys, populated - # during ``_format_server_config`` and consumed by the post-install - # summary line. Keys: env-var names; never holds resolved values. - self._last_env_placeholder_keys = set() - # Per-server collection of legacy ```` offenders, populated by - # the resolution helpers and consumed by ``configure_mcp_server`` to - # feed the aggregated deprecation warning. - self._last_legacy_angle_vars = set() def get_config_path(self): """Get the path to the Copilot CLI MCP configuration file. @@ -949,7 +870,7 @@ def _replace(match): ) return env_value if env_value else match.group(0) - return _COPILOT_ENV_RE.sub(_replace, value) + return _ENV_PLACEHOLDER_RE.sub(_replace, value) def _inject_env_vars_into_docker_args(self, docker_args, env_vars): """Inject environment variables into Docker arguments following registry template. @@ -1120,74 +1041,6 @@ def _process_arguments(self, arguments, resolved_env=None, runtime_vars=None): return processed - def _resolve_variable_placeholders(self, value, resolved_env, runtime_vars): - """Resolve runtime template variables and translate or resolve env-var - placeholders in argument strings. - - Behaviour depends on ``self._supports_runtime_env_substitution``: - - - True (Copilot CLI default): env-var placeholders (````, - ``${VAR}``, ``${env:VAR}``) are translated to ``${VAR}`` for - runtime substitution by Copilot CLI. APM template variables - (``{runtime_var}``) are still resolved at install time because - they are an APM-internal concept Copilot cannot interpret. - - - False (legacy / sibling-adapter behaviour): legacy ```` - placeholders are resolved against ``resolved_env`` (the dict of - literal env-var values), and ``{runtime_var}`` against - ``runtime_vars``. Newer ``${VAR}`` / ``${env:VAR}`` syntaxes are - left as-is for backward compatibility. - - Args: - value (str): Value that may contain placeholders. - resolved_env (dict): Dictionary of resolved env vars (legacy - mode) or placeholder strings (translate mode). - runtime_vars (dict): Dictionary of resolved runtime variables. - - Returns: - str: Processed value with placeholders translated or resolved. - """ - import re - - if not value: - return value - - processed = str(value) - - if self._supports_runtime_env_substitution: - # Track legacy offenders before translating them away. - self._last_legacy_angle_vars.update(_extract_legacy_angle_vars(processed)) - # Translate all three env-var placeholder syntaxes to ${VAR}. - processed = _translate_env_placeholder(processed) - else: - # Replace with actual values from resolved_env (for Docker env vars) - env_pattern = r"<([A-Z_][A-Z0-9_]*)>" - - def replace_env_var(match): - env_name = match.group(1) - return resolved_env.get(env_name, match.group(0)) # Return original if not found - - processed = re.sub(env_pattern, replace_env_var, processed) - - # Replace {runtime_var} with actual values from runtime_vars (for NPM args). - # Negative lookbehind on `$` so we never re-substitute inside an already-translated - # ${VAR} env placeholder (the brace is part of a Copilot CLI runtime substitution, - # not an APM template variable). - if runtime_vars: - runtime_pattern = r"(?``, ``${VAR}``, ``${env:VAR}``) + # are resolved at install time before being written to + # ``.gemini/settings.json``. See issue #1266. raw = server_info.get("_raw_stdio") if raw: config["command"] = raw["command"] - config["args"] = raw["args"] + resolved_env_for_args: dict = {} if raw.get("env"): - config["env"] = raw["env"] + resolved_env_for_args = self._resolve_environment_variables( + raw["env"], env_overrides=env_overrides + ) + config["env"] = resolved_env_for_args self._warn_input_variables(raw["env"], server_info.get("name", ""), "Gemini CLI") + config["args"] = [ + self._resolve_variable_placeholders(arg, resolved_env_for_args, runtime_vars) + if isinstance(arg, str) + else arg + for arg in raw.get("args") or [] + ] return config # --- remote endpoints --- diff --git a/tests/test_codex_empty_string_and_defaults.py b/tests/test_codex_empty_string_and_defaults.py index f046387a5..d51342f30 100644 --- a/tests/test_codex_empty_string_and_defaults.py +++ b/tests/test_codex_empty_string_and_defaults.py @@ -10,6 +10,7 @@ import os import sys +from pathlib import Path from unittest.mock import Mock, patch # noqa: F401 import pytest @@ -219,5 +220,93 @@ def test_whitespace_only_treated_as_empty(self, github_mcp_server_data): assert env_section["GITHUB_DYNAMIC_TOOLSETS"] == "1" # Default +class TestCodexSelfDefinedStdioEnvResolution: + """Regression coverage for issue #1266. + + Self-defined stdio MCP servers declared in ``apm.yml`` pass their ``env`` + block through the adapter as a plain dict (the ``_raw_stdio["env"]`` + shape). Before #1266, the Codex adapter wrote that dict to disk verbatim, + so placeholders like ``${TOKEN}`` ended up as literal strings in + ``~/.codex/config.toml`` and authentication silently failed. The fix + routes the dict through ``_resolve_environment_variables`` in legacy + mode so all three placeholder syntaxes resolve to literal values from + ``env_overrides`` -> ``os.environ`` at install time. + """ + + @pytest.fixture + def adapter(self, tmp_path, monkeypatch): + """Codex adapter writing into an isolated ~/.codex under tmp_path.""" + monkeypatch.setattr(Path, "home", lambda: tmp_path) + return CodexClientAdapter(user_scope=True) + + @staticmethod + def _server_info_with_placeholders(): + return { + "name": "bitbucket", + "id": "", + "_raw_stdio": { + "command": "pnpx", + "args": ["@aashari/mcp-server-atlassian-bitbucket@3.1.0"], + "env": { + "TOKEN_DOLLAR": "${ATLASSIAN_API_TOKEN}", + "TOKEN_ENVPREFIX": "${env:ATLASSIAN_API_TOKEN}", + "TOKEN_ANGLE": "", + "LITERAL_EMAIL": "user@example.com", + }, + }, + } + + def test_all_three_placeholder_syntaxes_resolve_to_literal(self, adapter): + env_overrides = {"ATLASSIAN_API_TOKEN": "real-secret-xyz123"} + + with patch.object(adapter, "registry_client") as mock_registry: + mock_registry.find_server_by_reference.return_value = ( + self._server_info_with_placeholders() + ) + ok = adapter.configure_mcp_server("bitbucket", env_overrides=env_overrides) + + assert ok is True + env_block = adapter.get_current_config()["mcp_servers"]["bitbucket"]["env"] + assert env_block["TOKEN_DOLLAR"] == "real-secret-xyz123" + assert env_block["TOKEN_ENVPREFIX"] == "real-secret-xyz123" + assert env_block["TOKEN_ANGLE"] == "real-secret-xyz123" + assert env_block["LITERAL_EMAIL"] == "user@example.com" + + def test_unresolvable_placeholder_is_preserved(self, adapter, monkeypatch): + """When neither env_overrides nor os.environ provides the value, + the placeholder must round-trip unchanged rather than disappear.""" + monkeypatch.delenv("ATLASSIAN_API_TOKEN", raising=False) + + with patch.object(adapter, "registry_client") as mock_registry: + mock_registry.find_server_by_reference.return_value = ( + self._server_info_with_placeholders() + ) + adapter.configure_mcp_server("bitbucket") + + env_block = adapter.get_current_config()["mcp_servers"]["bitbucket"]["env"] + assert env_block["TOKEN_DOLLAR"] == "${ATLASSIAN_API_TOKEN}" + assert env_block["TOKEN_ENVPREFIX"] == "${env:ATLASSIAN_API_TOKEN}" + assert env_block["TOKEN_ANGLE"] == "" + + def test_placeholders_in_args_also_resolve(self, adapter): + server_info = { + "name": "demo", + "id": "", + "_raw_stdio": { + "command": "demo", + "args": ["--token", ""], + "env": {"API_TOKEN": ""}, + }, + } + + with patch.object(adapter, "registry_client") as mock_registry: + mock_registry.find_server_by_reference.return_value = server_info + adapter.configure_mcp_server("demo", env_overrides={"API_TOKEN": "tok-abc"}) + + srv = adapter.get_current_config()["mcp_servers"]["demo"] + assert srv["env"]["API_TOKEN"] == "tok-abc" + assert srv["args"] == ["--token", "tok-abc"] + + if __name__ == "__main__": pytest.main([__file__, "-v"]) diff --git a/tests/unit/test_copilot_adapter.py b/tests/unit/test_copilot_adapter.py index 83f2c1e32..5f64ade2e 100644 --- a/tests/unit/test_copilot_adapter.py +++ b/tests/unit/test_copilot_adapter.py @@ -570,6 +570,72 @@ def test_dict_shaped_env_block_stringifies_literals_and_omits_none(self): ) +class TestLegacyModeStdioEnvBlock(unittest.TestCase): + """Issue #1266 / #1222 regression trap. + + Legacy-mode adapters (Claude / Cursor / Codex / Gemini) receive + ``_raw_stdio["env"]`` as a plain dict from ``apm.yml``. The legacy + branch of ``_resolve_environment_variables`` must handle this shape + by resolving each value via ``_resolve_env_variable`` -- otherwise the + pre-fix latent bug returns ``{}`` and every self-defined stdio MCP + server silently loses its env block. + + Cursor 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): + from apm_cli.adapters.client.cursor import CursorClientAdapter + + with ( + patch("apm_cli.adapters.client.copilot.SimpleRegistryClient"), + patch("apm_cli.adapters.client.copilot.RegistryIntegration"), + ): + return CursorClientAdapter() + + def test_dict_shape_resolves_all_three_placeholder_syntaxes(self): + adapter = self._adapter() + self.assertFalse(adapter._supports_runtime_env_substitution) + env_overrides = {"MY_TOKEN": "real-secret"} + result = adapter._resolve_environment_variables( + { + "DOLLAR": "${MY_TOKEN}", + "ENVPREFIX": "${env:MY_TOKEN}", + "ANGLE": "", + "LITERAL": "plain-value", + }, + env_overrides=env_overrides, + ) + self.assertEqual(result["DOLLAR"], "real-secret") + self.assertEqual(result["ENVPREFIX"], "real-secret") + self.assertEqual(result["ANGLE"], "real-secret") + self.assertEqual(result["LITERAL"], "plain-value") + + def test_dict_shape_does_not_silently_drop_keys(self): + """The pre-fix bug: the legacy branch iterated the dict as KEYS + (each a string), every key failed ``isinstance(env_var, dict)``, + and the resolver returned ``{}``. This regression trap fails + immediately if that empty-drop behaviour ever returns.""" + adapter = self._adapter() + result = adapter._resolve_environment_variables( + {"FOO": "literal", "BAR": "${UNSET}"}, env_overrides={"FOO": "x"} + ) + self.assertEqual(set(result.keys()), {"FOO", "BAR"}) + + def test_dict_shape_unresolvable_placeholder_round_trips(self): + """When neither env_overrides nor os.environ provides a value, the + placeholder must round-trip verbatim so the author can see what + was missing rather than getting an empty string.""" + adapter = self._adapter() + # patch.dict snapshots os.environ on enter and restores on exit, so + # the pop is reverted automatically after the test. + with patch.dict(os.environ, {}, clear=False): + os.environ.pop("DEFINITELY_NOT_SET_xz123", None) + result = adapter._resolve_environment_variables({"X": "${DEFINITELY_NOT_SET_xz123}"}) + self.assertEqual(result["X"], "${DEFINITELY_NOT_SET_xz123}") + + class TestCopilotInstallRunSummary(unittest.TestCase): """Issue #1152: aggregated post-install diagnostics. diff --git a/tests/unit/test_cursor_mcp.py b/tests/unit/test_cursor_mcp.py index 19e296b5a..db36a350c 100644 --- a/tests/unit/test_cursor_mcp.py +++ b/tests/unit/test_cursor_mcp.py @@ -342,5 +342,62 @@ def test_unsupported_packages_raises_valueerror(self): self.assertIn("No supported package type", str(ctx.exception)) +class TestCursorSelfDefinedStdioEnvResolution(unittest.TestCase): + """Regression coverage for a latent partner-bug of issue #1266. + + Before #1266 the Cursor adapter routed `raw["env"]` (a dict) through + `_resolve_environment_variables`, but the legacy-mode branch of that + method only handled the registry list-of-dict shape -- the dict shape + was silently iterated as KEYS, every key failed the `isinstance(..., dict)` + check, and the env block came out empty. The fix adds a dedicated + dict-shape legacy branch to the resolver so the same call site now + correctly resolves all three placeholder syntaxes. + """ + + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.cursor_dir = Path(self.tmp.name) / ".cursor" + self.cursor_dir.mkdir() + self.mcp_json = self.cursor_dir / "mcp.json" + self._cwd_patcher = patch("os.getcwd", return_value=self.tmp.name) + self._cwd_patcher.start() + self.adapter = CursorClientAdapter() + + def tearDown(self): + self._cwd_patcher.stop() + self.tmp.cleanup() + + def test_dict_shape_env_does_not_silently_drop(self): + server_info = { + "name": "bitbucket", + "id": "", + "_raw_stdio": { + "command": "pnpx", + "args": ["@aashari/mcp-server-atlassian-bitbucket@3.1.0"], + "env": { + "TOKEN_DOLLAR": "${ATLASSIAN_API_TOKEN}", + "TOKEN_ENVPREFIX": "${env:ATLASSIAN_API_TOKEN}", + "TOKEN_ANGLE": "", + "LITERAL_EMAIL": "user@example.com", + }, + }, + } + env_overrides = {"ATLASSIAN_API_TOKEN": "real-secret-xyz123"} + + with patch.object(self.adapter, "registry_client") as mock_registry: + mock_registry.find_server_by_reference.return_value = server_info + self.adapter.configure_mcp_server("bitbucket", env_overrides=env_overrides) + + env_block = json.loads(self.mcp_json.read_text(encoding="utf-8"))["mcpServers"][ + "bitbucket" + ]["env"] + # The pre-fix latent bug returned {}; the fix returns the resolved + # literal values for every placeholder syntax. + self.assertEqual(env_block["TOKEN_DOLLAR"], "real-secret-xyz123") + self.assertEqual(env_block["TOKEN_ENVPREFIX"], "real-secret-xyz123") + self.assertEqual(env_block["TOKEN_ANGLE"], "real-secret-xyz123") + self.assertEqual(env_block["LITERAL_EMAIL"], "user@example.com") + + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/test_gemini_mcp.py b/tests/unit/test_gemini_mcp.py index 0be0b5deb..0754e21ac 100644 --- a/tests/unit/test_gemini_mcp.py +++ b/tests/unit/test_gemini_mcp.py @@ -1,6 +1,7 @@ """Tests for the Gemini CLI MCP client adapter.""" import json +import os import shutil import tempfile import unittest @@ -363,3 +364,96 @@ def test_remote_sse_uses_url(self): self.assertEqual(config["url"], "https://api.example.com/sse") self.assertNotIn("httpUrl", config) self.assertNotIn("type", config) + + +class TestGeminiSelfDefinedStdioEnvResolution(unittest.TestCase): + """Regression coverage for issue #1266. + + Self-defined stdio MCP servers declared in apm.yml pass their env block + through the adapter as a plain dict (the _raw_stdio["env"] shape). + Before #1266, the Gemini adapter wrote that dict to disk verbatim, so + placeholders like ${TOKEN} ended up as literal strings in + .gemini/settings.json. The fix routes the dict through + _resolve_environment_variables in legacy mode so all three placeholder + syntaxes resolve to literal values from env_overrides -> os.environ at + install time. + """ + + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.gemini_dir = Path(self.tmp.name) / ".gemini" + self.gemini_dir.mkdir() + self.settings_json = self.gemini_dir / "settings.json" + self._cwd_patcher = patch("os.getcwd", return_value=self.tmp.name) + self._cwd_patcher.start() + self.adapter = GeminiClientAdapter() + + def tearDown(self): + self._cwd_patcher.stop() + self.tmp.cleanup() + + @staticmethod + def _server_info_with_placeholders(): + return { + "name": "bitbucket", + "id": "", + "_raw_stdio": { + "command": "pnpx", + "args": ["@aashari/mcp-server-atlassian-bitbucket@3.1.0"], + "env": { + "TOKEN_DOLLAR": "${ATLASSIAN_API_TOKEN}", + "TOKEN_ENVPREFIX": "${env:ATLASSIAN_API_TOKEN}", + "TOKEN_ANGLE": "", + "LITERAL_EMAIL": "user@example.com", + }, + }, + } + + def test_all_three_placeholder_syntaxes_resolve_to_literal(self): + env_overrides = {"ATLASSIAN_API_TOKEN": "real-secret-xyz123"} + with patch.object(self.adapter, "registry_client") as mock_registry: + mock_registry.find_server_by_reference.return_value = ( + self._server_info_with_placeholders() + ) + ok = self.adapter.configure_mcp_server("bitbucket", env_overrides=env_overrides) + + self.assertTrue(ok) + env_block = json.loads(self.settings_json.read_text())["mcpServers"]["bitbucket"]["env"] + self.assertEqual(env_block["TOKEN_DOLLAR"], "real-secret-xyz123") + self.assertEqual(env_block["TOKEN_ENVPREFIX"], "real-secret-xyz123") + self.assertEqual(env_block["TOKEN_ANGLE"], "real-secret-xyz123") + self.assertEqual(env_block["LITERAL_EMAIL"], "user@example.com") + + def test_unresolvable_placeholder_is_preserved(self): + # patch.dict snapshots os.environ on enter and restores on exit, so + # the pop is reverted automatically after the test. + with patch.dict(os.environ, {}, clear=False): + os.environ.pop("ATLASSIAN_API_TOKEN", None) + with patch.object(self.adapter, "registry_client") as mock_registry: + mock_registry.find_server_by_reference.return_value = ( + self._server_info_with_placeholders() + ) + self.adapter.configure_mcp_server("bitbucket") + + env_block = json.loads(self.settings_json.read_text())["mcpServers"]["bitbucket"]["env"] + self.assertEqual(env_block["TOKEN_DOLLAR"], "${ATLASSIAN_API_TOKEN}") + self.assertEqual(env_block["TOKEN_ENVPREFIX"], "${env:ATLASSIAN_API_TOKEN}") + self.assertEqual(env_block["TOKEN_ANGLE"], "") + + def test_placeholders_in_args_also_resolve(self): + server_info = { + "name": "demo", + "id": "", + "_raw_stdio": { + "command": "demo", + "args": ["--token", ""], + "env": {"API_TOKEN": ""}, + }, + } + with patch.object(self.adapter, "registry_client") as mock_registry: + mock_registry.find_server_by_reference.return_value = server_info + self.adapter.configure_mcp_server("demo", env_overrides={"API_TOKEN": "tok-abc"}) + + srv = json.loads(self.settings_json.read_text())["mcpServers"]["demo"] + self.assertEqual(srv["env"]["API_TOKEN"], "tok-abc") + self.assertEqual(srv["args"], ["--token", "tok-abc"]) diff --git a/tests/unit/test_skip_env_prompts.py b/tests/unit/test_skip_env_prompts.py new file mode 100644 index 000000000..62fa21cba --- /dev/null +++ b/tests/unit/test_skip_env_prompts.py @@ -0,0 +1,54 @@ +"""Unit tests for MCPClientAdapter._should_skip_env_prompts.""" + +import os +import unittest +from unittest.mock import patch + +from apm_cli.adapters.client.base import MCPClientAdapter + + +class TestShouldSkipEnvPrompts(unittest.TestCase): + """Verify the three-branch TTY/CI/managed-mode policy.""" + + def test_returns_true_when_env_overrides_provided(self): + """Managed mode: caller already collected env vars.""" + self.assertTrue(MCPClientAdapter._should_skip_env_prompts({"TOKEN": "val"})) + + @patch.dict(os.environ, {"APM_E2E_TESTS": "1"}) + def test_returns_true_when_e2e_tests_flag_set(self): + """CI mode: APM_E2E_TESTS=1 disables prompts.""" + self.assertTrue(MCPClientAdapter._should_skip_env_prompts({})) + + @patch.dict(os.environ, {}, clear=True) + def test_returns_true_when_stdin_not_tty(self): + """Non-interactive: stdin is not a TTY.""" + with patch("sys.stdin") as mock_stdin, patch("sys.stdout") as mock_stdout: + mock_stdin.isatty.return_value = False + mock_stdout.isatty.return_value = True + self.assertTrue(MCPClientAdapter._should_skip_env_prompts({})) + + @patch.dict(os.environ, {}, clear=True) + def test_returns_true_when_stdout_not_tty(self): + """Non-interactive: stdout is not a TTY.""" + with patch("sys.stdin") as mock_stdin, patch("sys.stdout") as mock_stdout: + mock_stdin.isatty.return_value = True + mock_stdout.isatty.return_value = False + self.assertTrue(MCPClientAdapter._should_skip_env_prompts({})) + + @patch.dict(os.environ, {}, clear=True) + def test_returns_false_when_interactive_tty(self): + """Interactive: both stdin and stdout are TTYs, no overrides, no CI flag.""" + with patch("sys.stdin") as mock_stdin, patch("sys.stdout") as mock_stdout: + mock_stdin.isatty.return_value = True + mock_stdout.isatty.return_value = True + self.assertFalse(MCPClientAdapter._should_skip_env_prompts({})) + + def test_returns_true_with_empty_overrides_is_false(self): + """Empty dict is falsy — should NOT skip on overrides alone.""" + # With an empty dict, only TTY/CI determines the result. + # This test just confirms {} is treated as "no overrides". + with patch("sys.stdin") as mock_stdin, patch("sys.stdout") as mock_stdout: + mock_stdin.isatty.return_value = True + mock_stdout.isatty.return_value = True + with patch.dict(os.environ, {}, clear=True): + self.assertFalse(MCPClientAdapter._should_skip_env_prompts({})) diff --git a/tests/unit/test_vscode_adapter.py b/tests/unit/test_vscode_adapter.py index 598d21609..3e7f8d25c 100644 --- a/tests/unit/test_vscode_adapter.py +++ b/tests/unit/test_vscode_adapter.py @@ -1216,24 +1216,24 @@ def test_warning_emitted_for_input_reference( self, ): mapping = {"Authorization": "Bearer ${input:my-token}"} - with patch("builtins.print") as mock_print: + with patch("apm_cli.adapters.client.base._rich_warning") as mock_warn: MCPClientAdapter._warn_input_variables(mapping, "my-server", "Copilot CLI") - mock_print.assert_called_once() - msg = mock_print.call_args[0][0] + mock_warn.assert_called_once() + msg = mock_warn.call_args[0][0] assert "my-token" in msg assert "Copilot CLI" in msg def test_no_warning_for_plain_values(self): mapping = {"Content-Type": "application/json"} - with patch("builtins.print") as mock_print: + with patch("apm_cli.adapters.client.base._rich_warning") as mock_warn: MCPClientAdapter._warn_input_variables(mapping, "s", "Codex CLI") - mock_print.assert_not_called() + mock_warn.assert_not_called() def test_no_warning_for_empty_mapping(self): - with patch("builtins.print") as mock_print: + with patch("apm_cli.adapters.client.base._rich_warning") as mock_warn: MCPClientAdapter._warn_input_variables({}, "s", "Codex CLI") MCPClientAdapter._warn_input_variables(None, "s", "Codex CLI") - mock_print.assert_not_called() + mock_warn.assert_not_called() class TestWarnOnLegacyAngleVars(unittest.TestCase):