diff --git a/docs/src/content/docs/reference/manifest-schema.md b/docs/src/content/docs/reference/manifest-schema.md index c75e7bd0b..e127cd879 100644 --- a/docs/src/content/docs/reference/manifest-schema.md +++ b/docs/src/content/docs/reference/manifest-schema.md @@ -432,7 +432,6 @@ Values in `headers` and `env` may contain three placeholder syntaxes. APM resolv - **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 in the Codex adapter today. - **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 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. diff --git a/src/apm_cli/adapters/client/base.py b/src/apm_cli/adapters/client/base.py index 59cf18d2a..6e150d1c1 100644 --- a/src/apm_cli/adapters/client/base.py +++ b/src/apm_cli/adapters/client/base.py @@ -4,6 +4,7 @@ import re from abc import ABC, abstractmethod from pathlib import Path +from typing import ClassVar _INPUT_VAR_RE = re.compile(r"\$\{input:([^}]+)\}") @@ -14,6 +15,73 @@ # 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)) + class MCPClientAdapter(ABC): """Base adapter for MCP clients.""" @@ -42,6 +110,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 +133,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: @@ -196,3 +278,255 @@ 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: + self._last_legacy_angle_vars.update(_extract_legacy_angle_vars(value)) + 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 +480,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 2751a30fe..32b756d6c 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,78 +18,7 @@ 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)) +from .base import _ENV_VAR_RE, MCPClientAdapter, _has_env_placeholder class CopilotClientAdapter(MCPClientAdapter): @@ -162,14 +90,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. @@ -700,235 +620,6 @@ def _format_server_config(self, server_info, env_overrides=None, runtime_vars=No return config - def _resolve_environment_variables(self, env_vars, env_overrides=None): - """Resolve (or translate) declared environment variables. - - Behaviour depends on ``self._supports_runtime_env_substitution``: - - - True (Copilot CLI default): each declared env var ``NAME`` gets a - ``${NAME}`` placeholder that Copilot CLI resolves at server-start - from the host environment. Hardcoded literal defaults - (``GITHUB_TOOLSETS``, ``GITHUB_DYNAMIC_TOOLSETS``) stay literal - because they are not secrets and provide essential server - configuration. The host environment is NOT read; secrets never - touch disk. See issue #1152 for context. - - - False (legacy / sibling-adapter behaviour): resolve each variable - to its literal value via ``env_overrides`` -> ``os.environ`` -> - optional interactive prompt, baking the result into the config. - - Args: - env_vars (list): List of environment variable definitions from - server info (each item is ``{name, description, required}``). - env_overrides (dict, optional): Pre-collected environment - variable overrides. Ignored in translate mode. - - Returns: - dict: ``{name: value}`` -- placeholder string in translate mode, - literal value in legacy mode. - """ - # Hardcoded literal defaults that supply essential server behaviour - # rather than secrets. These stay literal in translate mode so that - # tool-selection still works without a user export step. - default_github_env = {"GITHUB_TOOLSETS": "context", "GITHUB_DYNAMIC_TOOLSETS": "1"} - - # Self-defined stdio deps pass ``env`` as a plain dict - # ({NAME: value-or-placeholder}); registry-sourced deps pass a list - # of {name, description, required} dicts. Translate-mode handling - # for the dict shape: each value is either already a placeholder - # (translate it to the canonical ${VAR} form) or a literal (record - # the key as a placeholder reference and emit ${NAME} so the - # value never lands on disk). See issue #1152. - if isinstance(env_vars, dict) and self._supports_runtime_env_substitution: - translated = {} - placeholder_keys = [] - 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). - for match in _ENV_VAR_RE.finditer(translated[name]): - placeholder_keys.append(match.group(1)) - elif name in default_github_env and raw_value == 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 - - if self._supports_runtime_env_substitution: - resolved = {} - placeholder_keys = [] - 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 default_github_env: - # Non-secret literal default -- preserve as-is. - resolved[name] = default_github_env[name] - else: - # Emit a runtime-substitution placeholder; Copilot CLI - # resolves ``${NAME}`` from the host environment at - # server-start. APM never reads or stores the value. - resolved[name] = "${" + name + "}" - placeholder_keys.append(name) - # Record for the post-install summary line and the - # security-improvement notice. - self._last_env_placeholder_keys = set(placeholder_keys) - return resolved - - import os - import sys - - from rich.prompt import Prompt - - resolved = {} - env_overrides = env_overrides or {} - - # If env_overrides is provided, it means the CLI has already handled environment variable collection - # In this case, we should NEVER prompt for additional variables - skip_prompting = bool(env_overrides) - - # Check for CI/automated environment via APM_E2E_TESTS flag (more reliable than TTY detection) - if os.getenv("APM_E2E_TESTS") == "1": - skip_prompting = True - print(f" APM_E2E_TESTS detected, will skip environment variable prompts") # noqa: F541 - - # Also skip prompting if we're in a non-interactive environment (fallback) - is_interactive = sys.stdin.isatty() and sys.stdout.isatty() - if not is_interactive: - skip_prompting = True - - # Track which variables were explicitly provided with empty values (user wants defaults) - empty_value_vars = set() - if env_overrides: - for key, value in env_overrides.items(): - if key in env_overrides and (not value or not value.strip()): - empty_value_vars.add(key) - - for env_var in env_vars: - if isinstance(env_var, dict): - name = env_var.get("name", "") - description = env_var.get("description", "") - required = env_var.get("required", True) - - if name: - # First check overrides, then environment - value = env_overrides.get(name) or os.getenv(name) - - # Only prompt if not provided in overrides or environment AND it's required AND we're not in managed override mode - if not value and required and not skip_prompting: - prompt_text = f"Enter value for {name}" - if description: - prompt_text += f" ({description})" - value = Prompt.ask( - prompt_text, - password=True # noqa: SIM210 - if "token" in name.lower() or "key" in name.lower() - else False, - ) - - # Add variable if it has a value OR if user explicitly provided empty and we have a default - if value and value.strip(): - resolved[name] = value - elif name in empty_value_vars and name in default_github_env: - # User provided empty value and we have a default - use default - resolved[name] = default_github_env[name] - elif not required and name in default_github_env: - # Variable is optional and we have a default - use default - resolved[name] = default_github_env[name] - elif skip_prompting and name in default_github_env: - # Non-interactive environment and we have a default - use default - resolved[name] = default_github_env[name] - - return resolved - - def _resolve_env_variable(self, name, value, env_overrides=None): - """Resolve (or translate) a single environment variable value. - - Behaviour depends on ``self._supports_runtime_env_substitution``: - - - True (Copilot CLI default): translate placeholders to Copilot CLI's - native runtime substitution syntax (``${VAR}``). The host - environment is NOT read; the secret never touches disk. See issue - #1152 for context. Legacy ```` offenders are tracked for the - aggregated deprecation warning emitted by - ``configure_mcp_server``. - - - False (legacy / sibling-adapter behaviour): resolve placeholders - to literal values via ``env_overrides`` -> ``os.environ`` -> - optional interactive prompt, baking the result into the config. - - Args: - name (str): Environment variable name. - value (str): Environment variable value or placeholder. - env_overrides (dict, optional): Pre-collected environment - variable overrides. Ignored in translate mode. - - Returns: - str: Translated placeholder (translate mode) or resolved - literal value (legacy mode). - """ - if self._supports_runtime_env_substitution: - # Track legacy offenders for the aggregated deprecation - # warning. Translation itself is a pure-textual rewrite. - self._last_legacy_angle_vars.update(_extract_legacy_angle_vars(value)) - # Track env-var names referenced via this header/value so the - # security-upgrade detector and per-server summary can see - # them (the env-block path tracks via _resolve_environment_variables). - for match in _ENV_VAR_RE.finditer(value): - self._last_env_placeholder_keys.add(match.group(1)) - return _translate_env_placeholder(value) - - import sys - - from rich.prompt import Prompt - - env_overrides = env_overrides or {} - # If env_overrides is provided, it means we're in managed environment collection mode - skip_prompting = bool(env_overrides) - - # Check for CI/automated environment via APM_E2E_TESTS flag (more reliable than TTY detection) - if os.getenv("APM_E2E_TESTS") == "1": - skip_prompting = True - - # Also skip prompting if we're in a non-interactive environment (fallback) - is_interactive = sys.stdin.isatty() and sys.stdout.isatty() - if not is_interactive: - skip_prompting = True - - # Three accepted placeholder syntaxes (see _COPILOT_ENV_RE at module - # top), all resolved against env_overrides -> os.environ -> optional - # interactive prompt. Single-pass substitution preserves the legacy - # ```` semantics: resolved values are not re-scanned for further - # placeholder expansion. - def _replace(match): - # Group 1 = legacy ; group 2 = ${VAR} / ${env:VAR}. - 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: - prompt_text = f"Enter value for {env_name}" - env_value = Prompt.ask( - prompt_text, - password=True # noqa: SIM210 - if "token" in env_name.lower() or "key" in env_name.lower() - else False, - ) - return env_value if env_value else match.group(0) - - return _COPILOT_ENV_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. @@ -1098,74 +789,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 df087efe6..722136779 100644 --- a/tests/unit/test_copilot_adapter.py +++ b/tests/unit/test_copilot_adapter.py @@ -558,6 +558,72 @@ def test_dict_shaped_env_block_does_not_silently_drop(self): self.assertEqual(set(result.keys()), {"FOO", "BAR"}) +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 cb2b914e6..86e4bc6c1 100644 --- a/tests/unit/test_gemini_mcp.py +++ b/tests/unit/test_gemini_mcp.py @@ -1,7 +1,7 @@ """Tests for the Gemini CLI MCP client adapter.""" import json -import os # noqa: F401 +import os import shutil import tempfile import unittest @@ -262,3 +262,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"])