Skip to content

Commit 7515b77

Browse files
Address panel review recommendations for #1266
- Fix manifest-schema.md: update resolution table and bullets to include Gemini and Cursor alongside Copilot CLI and Codex (blocking finding) - Remove dead _stringify_env_literal from copilot.py (canonical copy in base.py) - Replace bare print() with _rich_warning() in _warn_input_variables - Add os.chmod(0o600) after config writes in codex, gemini, cursor adapters to protect resolved secrets from world-readable default permissions - Track legacy <VAR> keys in _last_env_placeholder_keys during translate mode so deprecation summaries include all placeholder types - Add _should_skip_env_prompts unit tests covering managed/CI/TTY branches - Add CHANGELOG entry for env-var placeholder resolution fix (#1266) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 433e650 commit 7515b77

8 files changed

Lines changed: 88 additions & 18 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1010
### Fixed
1111

1212
- 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 `<project_root>/.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)
13+
- Codex, Gemini, and Cursor adapters now resolve `${VAR}`, `${env:VAR}`, and legacy `<VAR>` environment placeholders in stdio server arguments, matching the behavior already present in the Copilot CLI adapter. (#1266)
1314
- `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)
1415
- 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)
1516
- `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)

docs/src/content/docs/reference/manifest-schema.md

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -442,19 +442,18 @@ dependencies:
442442

443443
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.
444444

445-
| Syntax | Source | VS Code | Copilot CLI / Codex |
445+
| Syntax | Source | VS Code | Copilot CLI / Codex / Gemini / Cursor |
446446
|---|---|---|---|
447447
| `${VAR}` | host environment | Translated to `${env:VAR}` (resolved at server-start by VS Code) | Resolved at install time from env (or interactive prompt) |
448448
| `${env:VAR}` | host environment | Native; passed through verbatim | Resolved at install time from env (or interactive prompt) |
449449
| `${input:<id>}` | user prompt | Native; VS Code prompts at runtime | Not supported; use `${VAR}` or `${env:VAR}` instead |
450450
| `<VAR>` (legacy) | host environment | Not recognized | Resolved at install time (kept for back-compat) |
451451

452452
- **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.
453-
- **Copilot CLI** has no runtime interpolation, so APM resolves `${VAR}`, `${env:VAR}`, and the legacy `<VAR>` 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.
454-
- **Codex** currently resolves only the legacy `<VAR>` placeholder at install time; `${VAR}` / `${env:VAR}` are passed through verbatim.
455-
- **Recommended:** Use `${VAR}` or `${env:VAR}` in all new manifests. They work on every target that supports remote MCP servers. `<VAR>` is legacy and only resolved by Copilot CLI and Codex; in VS Code it would silently render as literal text.
456-
- **Registry-backed servers**: APM auto-generates input prompts from registry metadata for `${input:...}`.
457-
- **Self-defined servers**: APM detects `${input:...}` patterns in `apm.yml` and generates matching input definitions automatically.
453+
- **Copilot CLI, Codex, Gemini, and Cursor** have no runtime interpolation, so APM resolves `${VAR}`, `${env:VAR}`, and the legacy `<VAR>` 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.
454+
- **Recommended:** Use `${VAR}` or `${env:VAR}` in all new manifests — they work on every target that supports remote MCP servers. `<VAR>` is legacy; in VS Code it would silently render as literal text in the generated config.
455+
- **Registry-backed servers** — APM auto-generates input prompts from registry metadata for `${input:...}`.
456+
- **Self-defined servers** — APM detects `${input:...}` patterns in `apm.yml` and generates matching input definitions automatically.
458457

459458
GitHub Actions templates (`${{ ... }}`) are intentionally left untouched.
460459

src/apm_cli/adapters/client/base.py

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
from pathlib import Path
77
from typing import ClassVar
88

9+
from ...utils.console import _rich_warning
10+
911
_INPUT_VAR_RE = re.compile(r"\$\{input:([^}]+)\}")
1012

1113
# Matches ${VAR} and ${env:VAR}, capturing VAR. Intentionally does NOT match
@@ -83,6 +85,13 @@ def _has_env_placeholder(value):
8385
return bool(_ENV_PLACEHOLDER_RE.search(value))
8486

8587

88+
def _stringify_env_literal(value):
89+
"""Return MCP env literal values in the manifest ``map<string, string>`` shape."""
90+
if isinstance(value, bool):
91+
return str(value).lower()
92+
return str(value)
93+
94+
8695
class MCPClientAdapter(ABC):
8796
"""Base adapter for MCP clients."""
8897

@@ -263,8 +272,8 @@ def _warn_input_variables(mapping, server_name, runtime_label):
263272
if var_id in seen:
264273
continue
265274
seen.add(var_id)
266-
print(
267-
f"[!] Warning: ${{input:{var_id}}} in server "
275+
_rich_warning(
276+
f"${{input:{var_id}}} in server "
268277
f"'{server_name}' will not be resolved -- "
269278
f"{runtime_label} does not support input variable prompts"
270279
)
@@ -456,7 +465,9 @@ def _resolve_env_variable(self, name, value, env_overrides=None):
456465
env_overrides: Pre-collected overrides (ignored in translate mode).
457466
"""
458467
if self._supports_runtime_env_substitution:
459-
self._last_legacy_angle_vars.update(_extract_legacy_angle_vars(value))
468+
legacy_keys = _extract_legacy_angle_vars(value)
469+
self._last_legacy_angle_vars.update(legacy_keys)
470+
self._last_env_placeholder_keys.update(legacy_keys)
460471
for match in _ENV_VAR_RE.finditer(value):
461472
self._last_env_placeholder_keys.add(match.group(1))
462473
return _translate_env_placeholder(value)

src/apm_cli/adapters/client/codex.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ def update_config(self, config_updates):
8484

8585
with open(config_path, "w", encoding="utf-8") as f:
8686
toml.dump(current_config, f)
87+
os.chmod(config_path, 0o600)
8788
_log.debug("Codex config written to %s", config_path)
8889
return True
8990

src/apm_cli/adapters/client/copilot.py

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,15 @@
1818
from ...registry.integration import RegistryIntegration
1919
from ...utils.console import _rich_warning
2020
from ...utils.github_host import is_github_hostname
21-
from .base import _ENV_VAR_RE, MCPClientAdapter, _has_env_placeholder
22-
23-
24-
def _stringify_env_literal(value):
25-
"""Return MCP env literal values in the manifest ``map<string, string>`` shape."""
26-
if isinstance(value, bool):
27-
return str(value).lower()
28-
return str(value)
21+
from .base import (
22+
_ENV_PLACEHOLDER_RE,
23+
_ENV_VAR_RE,
24+
MCPClientAdapter,
25+
_extract_legacy_angle_vars,
26+
_has_env_placeholder,
27+
_stringify_env_literal,
28+
_translate_env_placeholder,
29+
)
2930

3031

3132
class CopilotClientAdapter(MCPClientAdapter):
@@ -869,7 +870,7 @@ def _replace(match):
869870
)
870871
return env_value if env_value else match.group(0)
871872

872-
return _COPILOT_ENV_RE.sub(_replace, value)
873+
return _ENV_PLACEHOLDER_RE.sub(_replace, value)
873874

874875
def _inject_env_vars_into_docker_args(self, docker_args, env_vars):
875876
"""Inject environment variables into Docker arguments following registry template.

src/apm_cli/adapters/client/cursor.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ def update_config(self, config_updates):
7474

7575
with open(config_path, "w", encoding="utf-8") as f:
7676
json.dump(current_config, f, indent=2)
77+
os.chmod(config_path, 0o600)
7778

7879
def get_current_config(self):
7980
"""Read the current ``.cursor/mcp.json`` contents."""

src/apm_cli/adapters/client/gemini.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828

2929
import json
3030
import logging
31+
import os
3132
from pathlib import Path
3233

3334
from ...core.docker_args import DockerArgsProcessor
@@ -100,6 +101,7 @@ def update_config(self, config_updates):
100101
config_path.parent.mkdir(parents=True, exist_ok=True)
101102
with open(config_path, "w", encoding="utf-8") as f:
102103
json.dump(current_config, f, indent=2)
104+
os.chmod(config_path, 0o600)
103105

104106
def get_current_config(self):
105107
"""Read the current ``settings.json`` contents for the active scope."""
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
"""Unit tests for MCPClientAdapter._should_skip_env_prompts."""
2+
3+
import os
4+
import unittest
5+
from unittest.mock import patch
6+
7+
from apm_cli.adapters.client.base import MCPClientAdapter
8+
9+
10+
class TestShouldSkipEnvPrompts(unittest.TestCase):
11+
"""Verify the three-branch TTY/CI/managed-mode policy."""
12+
13+
def test_returns_true_when_env_overrides_provided(self):
14+
"""Managed mode: caller already collected env vars."""
15+
self.assertTrue(MCPClientAdapter._should_skip_env_prompts({"TOKEN": "val"}))
16+
17+
@patch.dict(os.environ, {"APM_E2E_TESTS": "1"})
18+
def test_returns_true_when_e2e_tests_flag_set(self):
19+
"""CI mode: APM_E2E_TESTS=1 disables prompts."""
20+
self.assertTrue(MCPClientAdapter._should_skip_env_prompts({}))
21+
22+
@patch.dict(os.environ, {}, clear=True)
23+
def test_returns_true_when_stdin_not_tty(self):
24+
"""Non-interactive: stdin is not a TTY."""
25+
with patch("sys.stdin") as mock_stdin, patch("sys.stdout") as mock_stdout:
26+
mock_stdin.isatty.return_value = False
27+
mock_stdout.isatty.return_value = True
28+
self.assertTrue(MCPClientAdapter._should_skip_env_prompts({}))
29+
30+
@patch.dict(os.environ, {}, clear=True)
31+
def test_returns_true_when_stdout_not_tty(self):
32+
"""Non-interactive: stdout is not a TTY."""
33+
with patch("sys.stdin") as mock_stdin, patch("sys.stdout") as mock_stdout:
34+
mock_stdin.isatty.return_value = True
35+
mock_stdout.isatty.return_value = False
36+
self.assertTrue(MCPClientAdapter._should_skip_env_prompts({}))
37+
38+
@patch.dict(os.environ, {}, clear=True)
39+
def test_returns_false_when_interactive_tty(self):
40+
"""Interactive: both stdin and stdout are TTYs, no overrides, no CI flag."""
41+
with patch("sys.stdin") as mock_stdin, patch("sys.stdout") as mock_stdout:
42+
mock_stdin.isatty.return_value = True
43+
mock_stdout.isatty.return_value = True
44+
self.assertFalse(MCPClientAdapter._should_skip_env_prompts({}))
45+
46+
def test_returns_true_with_empty_overrides_is_false(self):
47+
"""Empty dict is falsy — should NOT skip on overrides alone."""
48+
# With an empty dict, only TTY/CI determines the result.
49+
# This test just confirms {} is treated as "no overrides".
50+
with patch("sys.stdin") as mock_stdin, patch("sys.stdout") as mock_stdout:
51+
mock_stdin.isatty.return_value = True
52+
mock_stdout.isatty.return_value = True
53+
with patch.dict(os.environ, {}, clear=True):
54+
self.assertFalse(MCPClientAdapter._should_skip_env_prompts({}))

0 commit comments

Comments
 (0)