From 3b4c5e0e9d19b386a41a13efd92b35f5e1855673 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Fri, 15 May 2026 07:44:46 +0200 Subject: [PATCH 1/7] fix: respect targets: whitelist for all runtimes during MCP install (#1335) Two defects caused the targets: field in apm.yml to be silently ignored during MCP server installation: 1. _gate_project_scoped_runtimes read apm_config.get('target') (singular) instead of using parse_targets_field() which handles both target: and targets: keys. 2. Only codex and claude were in _PROJECT_SCOPED_RUNTIMES; all other runtimes (copilot, vscode, cursor, opencode, gemini, windsurf) were never filtered against the user's whitelist. Fix: when an explicit targets field is present (or --target CLI flag), gate ALL runtimes against the active target set. When no targets field exists, preserve backward-compat by gating only project-scoped runtimes. Adds 10 unit tests covering both singular/plural keys, user_scope bypass, explicit_target override, backward-compat auto-detect, and edge cases. Closes #1335 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 1 + src/apm_cli/integration/mcp_integrator.py | 51 ++++-- tests/unit/integration/test_mcp_integrator.py | 160 ++++++++++++++++++ 3 files changed, 202 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea197f037..2b65d5539 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `targets:` (plural) in `apm.yml` is now respected during MCP server installation; previously only the singular `target:` key was read, and only Codex / Claude were gated. All runtimes are now filtered when an explicit whitelist is present. (#1335) - 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) - `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) diff --git a/src/apm_cli/integration/mcp_integrator.py b/src/apm_cli/integration/mcp_integrator.py index f1df337ca..2ada5f4cd 100644 --- a/src/apm_cli/integration/mcp_integrator.py +++ b/src/apm_cli/integration/mcp_integrator.py @@ -953,25 +953,56 @@ def _gate_project_scoped_runtimes( apm_config: dict | None, explicit_target: str | None, ) -> list[str]: - """Drop project-scoped runtimes that are not active project targets. + """Filter *target_runtimes* against the project's active targets. - Codex and Claude Code both write project-scoped MCP config files - (``.codex/config.toml`` and ``.mcp.json``) whose creation should be - opt-in. When auto-detection brought one of them in but the project's - own targets do not include it, we silently strip it -- mirroring the - Cursor/OpenCode/Gemini directory-presence convention. + Two gating modes: + + 1. **Explicit targets** (``targets:`` / ``target:`` in *apm.yml*, or + the ``--target`` CLI flag): ALL runtimes not in the whitelist are + dropped. This is the contract users expect – see #1335. + 2. **Auto-detect** (no ``targets`` field): only project-scoped + runtimes (Codex, Claude Code) are gated, preserving backward + compatibility for repos that rely on directory-presence discovery. """ if user_scope: return target_runtimes - gated = [rt for rt in MCPIntegrator._PROJECT_SCOPED_RUNTIMES if rt in target_runtimes] - if not gated: - return target_runtimes + from apm_cli.core.apm_yml import parse_targets_field from apm_cli.integration.targets import active_targets + # --- resolve explicit targets from config ------------------------- + explicit_from_config: list[str] = [] + if apm_config: + try: + explicit_from_config = parse_targets_field(apm_config) + except Exception: + # ConflictingTargetsError / EmptyTargetsListError — validation + # should have caught this earlier; fall through to auto-detect. + _log.debug("parse_targets_field failed; falling back to auto-detect") + + config_target = explicit_target or explicit_from_config or None + has_explicit_targets = bool(explicit_target or explicit_from_config) + root = project_root or Path.cwd() - config_target = explicit_target or (apm_config.get("target") if apm_config else None) active = {t.name for t in active_targets(root, config_target)} + + if has_explicit_targets: + # Explicit whitelist: gate every runtime not in the active set. + out = [rt for rt in target_runtimes if rt in active] + dropped = set(target_runtimes) - set(out) + if dropped: + _log.debug( + "Targets whitelist gated out: %s (active=%s)", + sorted(dropped), + sorted(active), + ) + return out + + # No explicit targets — backward-compat: only gate project-scoped + # runtimes whose directory marker was auto-detected. + gated = [rt for rt in MCPIntegrator._PROJECT_SCOPED_RUNTIMES if rt in target_runtimes] + if not gated: + return target_runtimes out = list(target_runtimes) for rt in gated: if rt not in active: diff --git a/tests/unit/integration/test_mcp_integrator.py b/tests/unit/integration/test_mcp_integrator.py index 94d63866a..f2be6a8ee 100644 --- a/tests/unit/integration/test_mcp_integrator.py +++ b/tests/unit/integration/test_mcp_integrator.py @@ -731,3 +731,163 @@ def test_returns_empty_when_dir_missing(self, tmp_path): def test_returns_empty_when_dir_empty(self, tmp_path): result = MCPIntegrator.collect_transitive(tmp_path) assert result == [] + + +# =========================================================================== +# _gate_project_scoped_runtimes — issue #1335 +# =========================================================================== + + +class _FakeTarget: + """Minimal stand-in for TargetProfile.""" + + def __init__(self, name: str): + self.name = name + + +def _fake_active_targets(names: list[str]): + """Return a mock for active_targets that yields *names*.""" + + def _inner(_root, _explicit=None): + return [_FakeTarget(n) for n in names] + + return _inner + + +class TestGateProjectScopedRuntimes: + """Tests for MCPIntegrator._gate_project_scoped_runtimes (issue #1335).""" + + _gate = staticmethod(MCPIntegrator._gate_project_scoped_runtimes) + + # -- user_scope bypass -------------------------------------------------- + + def test_user_scope_bypasses_all_gating(self, tmp_path): + result = self._gate( + ["claude", "copilot", "vscode", "codex"], + user_scope=True, + project_root=tmp_path, + apm_config={"targets": ["claude"]}, + explicit_target=None, + ) + assert result == ["claude", "copilot", "vscode", "codex"] + + # -- explicit targets: (plural) gates all runtimes --------------------- + + @patch("apm_cli.integration.targets.active_targets") + def test_targets_plural_filters_unlisted_runtimes(self, mock_at, tmp_path): + mock_at.side_effect = _fake_active_targets(["claude"]) + result = self._gate( + ["claude", "copilot", "vscode", "codex"], + user_scope=False, + project_root=tmp_path, + apm_config={"targets": ["claude"]}, + explicit_target=None, + ) + assert result == ["claude"] + + @patch("apm_cli.integration.targets.active_targets") + def test_target_singular_filters_unlisted_runtimes(self, mock_at, tmp_path): + mock_at.side_effect = _fake_active_targets(["claude"]) + result = self._gate( + ["claude", "copilot", "vscode"], + user_scope=False, + project_root=tmp_path, + apm_config={"target": "claude"}, + explicit_target=None, + ) + assert result == ["claude"] + + @patch("apm_cli.integration.targets.active_targets") + def test_targets_multiple_values_keeps_all_listed(self, mock_at, tmp_path): + mock_at.side_effect = _fake_active_targets(["claude", "copilot"]) + result = self._gate( + ["claude", "copilot", "vscode", "codex", "cursor"], + user_scope=False, + project_root=tmp_path, + apm_config={"targets": ["claude", "copilot"]}, + explicit_target=None, + ) + assert result == ["claude", "copilot"] + + # -- no targets field: backward-compat (gate only project-scoped) ------ + + @patch("apm_cli.integration.targets.active_targets") + def test_no_targets_gates_only_codex_claude(self, mock_at, tmp_path): + # active_targets returns copilot only — codex/claude should be gated + mock_at.side_effect = _fake_active_targets(["copilot"]) + result = self._gate( + ["copilot", "vscode", "codex", "claude", "cursor"], + user_scope=False, + project_root=tmp_path, + apm_config={}, + explicit_target=None, + ) + assert "copilot" in result + assert "vscode" in result + assert "cursor" in result + assert "codex" not in result + assert "claude" not in result + + @patch("apm_cli.integration.targets.active_targets") + def test_no_targets_no_project_scoped_returns_all(self, mock_at, tmp_path): + # No codex/claude in list → nothing to gate, return all + mock_at.side_effect = _fake_active_targets(["copilot"]) + result = self._gate( + ["copilot", "vscode", "cursor"], + user_scope=False, + project_root=tmp_path, + apm_config={}, + explicit_target=None, + ) + assert result == ["copilot", "vscode", "cursor"] + + # -- explicit_target CLI flag overrides config ------------------------- + + @patch("apm_cli.integration.targets.active_targets") + def test_explicit_target_overrides_config(self, mock_at, tmp_path): + mock_at.side_effect = _fake_active_targets(["vscode"]) + result = self._gate( + ["claude", "copilot", "vscode", "codex"], + user_scope=False, + project_root=tmp_path, + apm_config={"targets": ["claude", "copilot"]}, + explicit_target="vscode", + ) + assert result == ["vscode"] + + @patch("apm_cli.integration.targets.active_targets") + def test_explicit_target_without_config(self, mock_at, tmp_path): + mock_at.side_effect = _fake_active_targets(["cursor"]) + result = self._gate( + ["claude", "copilot", "cursor", "codex"], + user_scope=False, + project_root=tmp_path, + apm_config={}, + explicit_target="cursor", + ) + assert result == ["cursor"] + + # -- edge cases -------------------------------------------------------- + + def test_empty_target_runtimes_returns_empty(self, tmp_path): + result = self._gate( + [], + user_scope=False, + project_root=tmp_path, + apm_config={"targets": ["claude"]}, + explicit_target=None, + ) + assert result == [] + + @patch("apm_cli.integration.targets.active_targets") + def test_apm_config_none_falls_through_to_auto_detect(self, mock_at, tmp_path): + mock_at.side_effect = _fake_active_targets(["copilot"]) + result = self._gate( + ["copilot", "codex"], + user_scope=False, + project_root=tmp_path, + apm_config=None, + explicit_target=None, + ) + assert "copilot" in result + assert "codex" not in result From 82fe8624d58d79ccbd2b2ecaad0a004fcc043b06 Mon Sep 17 00:00:00 2001 From: Daniel Meppiel Date: Fri, 15 May 2026 08:29:20 +0200 Subject: [PATCH 2/7] fix: act on panel review for #1336 -- fail-closed + visible whitelist gating Implements all five panel follow-ups in-PR rather than deferring: 1. Supply-chain + py-architect: narrow except to (ConflictingTargetsError, EmptyTargetsListError) and fail closed with _rich_warning + return []. Malformed apm.yml targets no longer silently widen the MCP write surface via auto-detect fallback. 2. Test-coverage: two new tests cover the fail-closed path (test_conflicting_targets_field_fails_closed, test_empty_targets_list_fails_closed). 12/12 TestGateProjectScopedRuntimes pass. 3. CLI-logging + DevX UX: promote whitelist gating from _log.debug to _rich_info so users see confirmation that their declared targets: filter took effect ("Targets whitelist active: skipped MCP config for X"). The auto-detect debug log on the backward-compat path is unchanged. 4. Doc-writer: docs/consumer/install-mcp-servers.md now documents the targets: whitelist semantics and fail-closed behavior in the canonical 'What apm install writes to disk' section. 5. OSS growth: CHANGELOG entry rewritten to lead with the user promise ('MCP server installation now respects the explicit targets: whitelist for all runtimes'), name the user-facing symptom, and call out the backward-compat split + fail-closed semantics. Plus the supply-chain user_scope security comment and the py-architect falsy-list rationale comment on config_target. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 2 +- .../docs/consumer/install-mcp-servers.md | 11 ++++++ src/apm_cli/integration/mcp_integrator.py | 39 +++++++++++++++---- tests/unit/integration/test_mcp_integrator.py | 29 ++++++++++++++ 4 files changed, 73 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b65d5539..0af277630 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- `targets:` (plural) in `apm.yml` is now respected during MCP server installation; previously only the singular `target:` key was read, and only Codex / Claude were gated. All runtimes are now filtered when an explicit whitelist is present. (#1335) +- MCP server installation now respects the explicit `targets:` whitelist for all runtimes; previously only Codex and Claude Code were gated, and only when the singular `target:` key was used (e.g. `targets: [copilot]` no longer leaks an MCP write to `~/.codex/config.toml`). Repos without a `targets:` field retain the previous auto-detect semantics for backward compatibility. A malformed `targets:` field (conflicting `target:` + `targets:`, or `targets: []`) now fails closed -- no MCP files are written -- with a warning naming the field. (#1335) - 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) - `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) diff --git a/docs/src/content/docs/consumer/install-mcp-servers.md b/docs/src/content/docs/consumer/install-mcp-servers.md index df0f60a30..0c5acfabf 100644 --- a/docs/src/content/docs/consumer/install-mcp-servers.md +++ b/docs/src/content/docs/consumer/install-mcp-servers.md @@ -93,6 +93,17 @@ This avoids creating runtime artifacts for tools you do not use. Gemini's user-scope path (`~/.gemini/settings.json`, selected with `-g`) is unconditional and creates `~/.gemini/` if needed. +When `apm.yml` declares `targets:` (or `--target` is passed on the +command line), MCP install treats that list as a hard whitelist -- +runtimes outside the list are skipped even if their harness +directory is present, and APM emits an info line naming the skipped +runtimes so you can confirm the whitelist took effect. With no +`targets:` field, only project-scoped runtimes (Codex, Claude Code) +are gated for backward compatibility (#1335). A malformed `targets:` +field (e.g. both `target:` and `targets:` set, or `targets: []`) +fails closed: no MCP files are written and a warning names the field +to fix. + `apm install -g --mcp NAME` routes the write to each runtime's user-scope MCP config when that runtime supports user scope -- for example Copilot CLI writes to `~/.copilot/mcp-config.json`, Codex diff --git a/src/apm_cli/integration/mcp_integrator.py b/src/apm_cli/integration/mcp_integrator.py index 2ada5f4cd..38fcdae14 100644 --- a/src/apm_cli/integration/mcp_integrator.py +++ b/src/apm_cli/integration/mcp_integrator.py @@ -24,9 +24,9 @@ from apm_cli.utils.console import ( _get_console, # noqa: F401 -- module attribute; patched by tests and used via re-export _rich_error, # noqa: F401 - _rich_info, # noqa: F401 + _rich_info, _rich_success, - _rich_warning, # noqa: F401 + _rich_warning, ) _log = logging.getLogger(__name__) @@ -964,10 +964,17 @@ def _gate_project_scoped_runtimes( runtimes (Codex, Claude Code) are gated, preserving backward compatibility for repos that rely on directory-presence discovery. """ + # Security note: user-scope writes target ~/.config paths the user owns + # globally; gating only applies to project-scoped writes that could + # affect shared repos. See enterprise/security model. if user_scope: return target_runtimes - from apm_cli.core.apm_yml import parse_targets_field + from apm_cli.core.apm_yml import ( + ConflictingTargetsError, + EmptyTargetsListError, + parse_targets_field, + ) from apm_cli.integration.targets import active_targets # --- resolve explicit targets from config ------------------------- @@ -975,11 +982,23 @@ def _gate_project_scoped_runtimes( if apm_config: try: explicit_from_config = parse_targets_field(apm_config) - except Exception: - # ConflictingTargetsError / EmptyTargetsListError — validation - # should have caught this earlier; fall through to auto-detect. - _log.debug("parse_targets_field failed; falling back to auto-detect") + except (ConflictingTargetsError, EmptyTargetsListError) as exc: + # Manifest declared targets but the declaration is unparseable. + # Fail closed: write nothing rather than fall back to permissive + # auto-detect, which would widen the MCP write surface beyond + # the user's stated intent. + _rich_warning( + f"apm.yml targets field is invalid: {exc}. Skipping MCP " + "install for all runtimes. Fix the manifest and re-run." + ) + _log.debug( + "parse_targets_field failed; failing closed (no MCP writes)", + exc_info=True, + ) + return [] + # `parse_targets_field` returns [] when neither key is present, so the + # falsy chain below correctly falls through to auto-detect in that case. config_target = explicit_target or explicit_from_config or None has_explicit_targets = bool(explicit_target or explicit_from_config) @@ -991,6 +1010,12 @@ def _gate_project_scoped_runtimes( out = [rt for rt in target_runtimes if rt in active] dropped = set(target_runtimes) - set(out) if dropped: + # User declared a whitelist; surface the consequence so they + # can confirm their intent took effect (or correct it). + _rich_info( + f"Targets whitelist active: skipped MCP config for " + f"{', '.join(sorted(dropped))} (not in targets)." + ) _log.debug( "Targets whitelist gated out: %s (active=%s)", sorted(dropped), diff --git a/tests/unit/integration/test_mcp_integrator.py b/tests/unit/integration/test_mcp_integrator.py index f2be6a8ee..2ebb6ddc6 100644 --- a/tests/unit/integration/test_mcp_integrator.py +++ b/tests/unit/integration/test_mcp_integrator.py @@ -891,3 +891,32 @@ def test_apm_config_none_falls_through_to_auto_detect(self, mock_at, tmp_path): ) assert "copilot" in result assert "codex" not in result + + # -- malformed targets field: fail-closed (issue #1335 follow-up) ------ + + @patch("apm_cli.integration.targets.active_targets") + def test_conflicting_targets_field_fails_closed(self, mock_at, tmp_path): + # Both `target` and `targets` set -> ConflictingTargetsError. + # Fail-closed contract: write nothing rather than widen via auto-detect. + mock_at.side_effect = _fake_active_targets(["copilot", "claude", "codex"]) + result = self._gate( + ["claude", "copilot", "vscode", "codex"], + user_scope=False, + project_root=tmp_path, + apm_config={"target": "claude", "targets": ["copilot"]}, + explicit_target=None, + ) + assert result == [] + + @patch("apm_cli.integration.targets.active_targets") + def test_empty_targets_list_fails_closed(self, mock_at, tmp_path): + # `targets: []` -> EmptyTargetsListError. Same fail-closed contract. + mock_at.side_effect = _fake_active_targets(["copilot", "claude", "codex"]) + result = self._gate( + ["claude", "copilot", "codex"], + user_scope=False, + project_root=tmp_path, + apm_config={"targets": []}, + explicit_target=None, + ) + assert result == [] From 430bd9fe133e6e840d3cdcf593f0a9aa0339cd91 Mon Sep 17 00:00:00 2001 From: Daniel Meppiel Date: Fri, 15 May 2026 08:34:23 +0200 Subject: [PATCH 3/7] fix: address Copilot PR review on #1336 Triage of 7 Copilot comments: LEGIT and applied: - Replace non-ASCII em/en dashes and right-arrows in mcp_integrator.py and test_mcp_integrator.py with ASCII equivalents (encoding rule). - Update explicit_target type hint from `str | None` to `str | list[str] | None`; document CSV-string acceptance via the _wire_bundle_mcp_servers caller. - Normalize CSV `explicit_target` ("claude,copilot") to a list before calling active_targets(), which would otherwise treat the CSV as one unknown token and drop every runtime. Adds test_explicit_target_csv_string_normalized as regression guard. Already addressed in 82fe8624 (no-op): - Broad `except Exception` was already narrowed to (ConflictingTargetsError, EmptyTargetsListError) with fail-closed semantics in the prior follow-up commit; comment was stale. Rejected: - Suggestion to reference PR # in CHANGELOG: declined. Every recent CHANGELOG entry references the issue # (#1313, #1289, #1222, #1299, etc.) -- this is the established repo convention. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/integration/mcp_integrator.py | 25 ++++++++++--- tests/unit/integration/test_mcp_integrator.py | 37 +++++++++++++++++-- 2 files changed, 54 insertions(+), 8 deletions(-) diff --git a/src/apm_cli/integration/mcp_integrator.py b/src/apm_cli/integration/mcp_integrator.py index 38fcdae14..b08a665c6 100644 --- a/src/apm_cli/integration/mcp_integrator.py +++ b/src/apm_cli/integration/mcp_integrator.py @@ -951,7 +951,7 @@ def _gate_project_scoped_runtimes( user_scope: bool, project_root, apm_config: dict | None, - explicit_target: str | None, + explicit_target: str | list[str] | None, ) -> list[str]: """Filter *target_runtimes* against the project's active targets. @@ -959,10 +959,17 @@ def _gate_project_scoped_runtimes( 1. **Explicit targets** (``targets:`` / ``target:`` in *apm.yml*, or the ``--target`` CLI flag): ALL runtimes not in the whitelist are - dropped. This is the contract users expect – see #1335. + dropped. This is the contract users expect -- see #1335. 2. **Auto-detect** (no ``targets`` field): only project-scoped runtimes (Codex, Claude Code) are gated, preserving backward compatibility for repos that rely on directory-presence discovery. + + ``explicit_target`` accepts ``str`` (single token), ``list[str]``, + or a CSV string (``"claude,copilot"``) -- the latter is produced by + ``install/local_bundle_handler.py::_wire_bundle_mcp_servers`` when + wiring multi-target bundles. CSV strings are normalized to a list + before they reach :func:`active_targets`, which only understands + single tokens or list input. """ # Security note: user-scope writes target ~/.config paths the user owns # globally; gating only applies to project-scoped writes that could @@ -999,8 +1006,16 @@ def _gate_project_scoped_runtimes( # `parse_targets_field` returns [] when neither key is present, so the # falsy chain below correctly falls through to auto-detect in that case. - config_target = explicit_target or explicit_from_config or None - has_explicit_targets = bool(explicit_target or explicit_from_config) + # Normalize CSV strings ("claude,copilot") to a list before active_targets, + # which treats a CSV string as a single unknown token (returns []). + normalized_explicit: str | list[str] | None + if isinstance(explicit_target, str) and "," in explicit_target: + normalized_explicit = [t.strip() for t in explicit_target.split(",") if t.strip()] + else: + normalized_explicit = explicit_target + + config_target = normalized_explicit or explicit_from_config or None + has_explicit_targets = bool(normalized_explicit or explicit_from_config) root = project_root or Path.cwd() active = {t.name for t in active_targets(root, config_target)} @@ -1023,7 +1038,7 @@ def _gate_project_scoped_runtimes( ) return out - # No explicit targets — backward-compat: only gate project-scoped + # No explicit targets -- backward-compat: only gate project-scoped # runtimes whose directory marker was auto-detected. gated = [rt for rt in MCPIntegrator._PROJECT_SCOPED_RUNTIMES if rt in target_runtimes] if not gated: diff --git a/tests/unit/integration/test_mcp_integrator.py b/tests/unit/integration/test_mcp_integrator.py index 2ebb6ddc6..ec40c5242 100644 --- a/tests/unit/integration/test_mcp_integrator.py +++ b/tests/unit/integration/test_mcp_integrator.py @@ -734,7 +734,7 @@ def test_returns_empty_when_dir_empty(self, tmp_path): # =========================================================================== -# _gate_project_scoped_runtimes — issue #1335 +# _gate_project_scoped_runtimes -- issue #1335 # =========================================================================== @@ -813,7 +813,7 @@ def test_targets_multiple_values_keeps_all_listed(self, mock_at, tmp_path): @patch("apm_cli.integration.targets.active_targets") def test_no_targets_gates_only_codex_claude(self, mock_at, tmp_path): - # active_targets returns copilot only — codex/claude should be gated + # active_targets returns copilot only -- codex/claude should be gated mock_at.side_effect = _fake_active_targets(["copilot"]) result = self._gate( ["copilot", "vscode", "codex", "claude", "cursor"], @@ -830,7 +830,7 @@ def test_no_targets_gates_only_codex_claude(self, mock_at, tmp_path): @patch("apm_cli.integration.targets.active_targets") def test_no_targets_no_project_scoped_returns_all(self, mock_at, tmp_path): - # No codex/claude in list → nothing to gate, return all + # No codex/claude in list -> nothing to gate, return all mock_at.side_effect = _fake_active_targets(["copilot"]) result = self._gate( ["copilot", "vscode", "cursor"], @@ -920,3 +920,34 @@ def test_empty_targets_list_fails_closed(self, mock_at, tmp_path): explicit_target=None, ) assert result == [] + + @patch("apm_cli.integration.targets.active_targets") + def test_explicit_target_csv_string_normalized(self, mock_at, tmp_path): + # `_wire_bundle_mcp_servers` passes `target_csv = "claude,copilot"`. + # active_targets() does not split CSV; the gate must normalize first + # or active_targets sees one unknown token and returns []. Regression + # guard for the Copilot review on PR #1336. + seen_explicit: list = [] + + def _resolve(_root, explicit=None): + seen_explicit.append(explicit) + # Real active_targets returns profiles for each canonical name in + # the (now-list) explicit input. + tokens = ( + [explicit] if isinstance(explicit, str) else (list(explicit) if explicit else []) + ) + return [_FakeTarget(t) for t in tokens] + + mock_at.side_effect = _resolve + result = self._gate( + ["claude", "copilot", "codex"], + user_scope=False, + project_root=tmp_path, + apm_config=None, + explicit_target="claude,copilot", + ) + # Gate must have normalized the CSV to a list before calling active_targets. + assert seen_explicit == [["claude", "copilot"]] + assert "claude" in result + assert "copilot" in result + assert "codex" not in result From 3a5bc4e697b4658991dbcc4d5b81ae0a2fae37c4 Mon Sep 17 00:00:00 2001 From: Daniel Meppiel Date: Fri, 15 May 2026 08:43:15 +0200 Subject: [PATCH 4/7] refactor: align MCP gate with apm-install targets engine UX Drop the codex/claude backward-compat special case. The gate now mirrors active_targets() resolution (explicit > directory detection > [copilot] fallback) for every runtime, exactly the same model 'apm install' uses for skills, agents, and instructions. A user running 'apm install' on a project with no .codex/ directory no longer leaks an MCP write to ~/.codex/config.toml -- no matter whether 'targets:' is declared. Drops _PROJECT_SCOPED_RUNTIMES constant. Aliases the 'vscode' runtime to the 'copilot' canonical target inside the gate (mirrors the alias active_targets honors for explicit_target). Updates docs, CHANGELOG, and the two backward-compat-specific tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 2 +- .../docs/consumer/install-mcp-servers.md | 19 ++--- src/apm_cli/integration/mcp_integrator.py | 70 ++++++++----------- tests/unit/integration/test_mcp_integrator.py | 32 +++++---- 4 files changed, 59 insertions(+), 64 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0af277630..cf7e4cf2a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- MCP server installation now respects the explicit `targets:` whitelist for all runtimes; previously only Codex and Claude Code were gated, and only when the singular `target:` key was used (e.g. `targets: [copilot]` no longer leaks an MCP write to `~/.codex/config.toml`). Repos without a `targets:` field retain the previous auto-detect semantics for backward compatibility. A malformed `targets:` field (conflicting `target:` + `targets:`, or `targets: []`) now fails closed -- no MCP files are written -- with a warning naming the field. (#1335) +- MCP server installation now mirrors the `apm install` targets engine: the active target set (explicit `--target` / `targets:`, else directory detection, else `[copilot]` fallback) is the whitelist for every runtime. Previously only Codex and Claude Code were gated when `targets:` was omitted, and `targets: [copilot]` only became a hard whitelist when the singular `target:` key was used (so a stray `~/.codex/config.toml` write could still leak). A malformed `targets:` field (conflicting `target:` + `targets:`, or `targets: []`) now fails closed -- no MCP files are written -- with a warning naming the field. (#1335) - 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) - `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) diff --git a/docs/src/content/docs/consumer/install-mcp-servers.md b/docs/src/content/docs/consumer/install-mcp-servers.md index 0c5acfabf..9a6e16cdd 100644 --- a/docs/src/content/docs/consumer/install-mcp-servers.md +++ b/docs/src/content/docs/consumer/install-mcp-servers.md @@ -95,14 +95,17 @@ Gemini's user-scope path (`~/.gemini/settings.json`, selected with When `apm.yml` declares `targets:` (or `--target` is passed on the command line), MCP install treats that list as a hard whitelist -- -runtimes outside the list are skipped even if their harness -directory is present, and APM emits an info line naming the skipped -runtimes so you can confirm the whitelist took effect. With no -`targets:` field, only project-scoped runtimes (Codex, Claude Code) -are gated for backward compatibility (#1335). A malformed `targets:` -field (e.g. both `target:` and `targets:` set, or `targets: []`) -fails closed: no MCP files are written and a warning names the field -to fix. +runtimes outside the list are skipped even if their harness directory +is present. With no explicit targets, MCP install falls back to the +same directory-detection rule `apm install` uses for skills and other +APM dependencies: a runtime is configured only when its harness +directory (`.github/`, `.claude/`, `.cursor/`, `.codex/`, +`.opencode/`, `.gemini/`) is present in the project, with greenfield +projects defaulting to Copilot. Either way, APM emits an info line +naming the skipped runtimes so you can confirm the gate took effect. +A malformed `targets:` field (e.g. both `target:` and `targets:` set, +or `targets: []`) fails closed: no MCP files are written and a +warning names the field to fix. (#1335) `apm install -g --mcp NAME` routes the write to each runtime's user-scope MCP config when that runtime supports user scope -- for diff --git a/src/apm_cli/integration/mcp_integrator.py b/src/apm_cli/integration/mcp_integrator.py index b08a665c6..69f550279 100644 --- a/src/apm_cli/integration/mcp_integrator.py +++ b/src/apm_cli/integration/mcp_integrator.py @@ -942,8 +942,6 @@ def _install_for_runtime( # Main orchestrator # ------------------------------------------------------------------ - _PROJECT_SCOPED_RUNTIMES: tuple[str, ...] = ("codex", "claude") - @staticmethod def _gate_project_scoped_runtimes( target_runtimes: list[str], @@ -955,14 +953,12 @@ def _gate_project_scoped_runtimes( ) -> list[str]: """Filter *target_runtimes* against the project's active targets. - Two gating modes: - - 1. **Explicit targets** (``targets:`` / ``target:`` in *apm.yml*, or - the ``--target`` CLI flag): ALL runtimes not in the whitelist are - dropped. This is the contract users expect -- see #1335. - 2. **Auto-detect** (no ``targets`` field): only project-scoped - runtimes (Codex, Claude Code) are gated, preserving backward - compatibility for repos that rely on directory-presence discovery. + Mirrors the UX of ``apm install`` for apm dependencies: the active + target set (explicit ``--target`` / ``targets:`` field, else + directory detection, else fallback ``[copilot]``) is the whitelist + for MCP writes too. Runtimes outside that set are skipped with an + info line naming them, rather than leaking config files into + projects that are not configured for them (#1335). ``explicit_target`` accepts ``str`` (single token), ``list[str]``, or a CSV string (``"claude,copilot"``) -- the latter is produced by @@ -970,6 +966,9 @@ def _gate_project_scoped_runtimes( wiring multi-target bundles. CSV strings are normalized to a list before they reach :func:`active_targets`, which only understands single tokens or list input. + + A malformed ``targets:`` field (conflicting ``target:`` + + ``targets:``, or ``targets: []``) fails closed: nothing is written. """ # Security note: user-scope writes target ~/.config paths the user owns # globally; gating only applies to project-scoped writes that could @@ -1004,8 +1003,6 @@ def _gate_project_scoped_runtimes( ) return [] - # `parse_targets_field` returns [] when neither key is present, so the - # falsy chain below correctly falls through to auto-detect in that case. # Normalize CSV strings ("claude,copilot") to a list before active_targets, # which treats a CSV string as a single unknown token (returns []). normalized_explicit: str | list[str] | None @@ -1014,40 +1011,31 @@ def _gate_project_scoped_runtimes( else: normalized_explicit = explicit_target + # `parse_targets_field` returns [] when neither key is present, so the + # falsy chain below correctly falls through to auto-detect in that case. config_target = normalized_explicit or explicit_from_config or None - has_explicit_targets = bool(normalized_explicit or explicit_from_config) root = project_root or Path.cwd() active = {t.name for t in active_targets(root, config_target)} - if has_explicit_targets: - # Explicit whitelist: gate every runtime not in the active set. - out = [rt for rt in target_runtimes if rt in active] - dropped = set(target_runtimes) - set(out) - if dropped: - # User declared a whitelist; surface the consequence so they - # can confirm their intent took effect (or correct it). - _rich_info( - f"Targets whitelist active: skipped MCP config for " - f"{', '.join(sorted(dropped))} (not in targets)." - ) - _log.debug( - "Targets whitelist gated out: %s (active=%s)", - sorted(dropped), - sorted(active), - ) - return out - - # No explicit targets -- backward-compat: only gate project-scoped - # runtimes whose directory marker was auto-detected. - gated = [rt for rt in MCPIntegrator._PROJECT_SCOPED_RUNTIMES if rt in target_runtimes] - if not gated: - return target_runtimes - out = list(target_runtimes) - for rt in gated: - if rt not in active: - _log.debug("%s gated out: active_targets=%s", rt.capitalize(), sorted(active)) - out = [r for r in out if r != rt] + # Runtime name "vscode" maps to canonical target "copilot" (the same + # alias active_targets honors for explicit_target). Without this map + # a project with `.github/` set but no `.copilot/` would correctly + # detect copilot but still gate the vscode runtime. + def _canonical(rt: str) -> str: + return "copilot" if rt in ("vscode", "agents") else rt + + out = [rt for rt in target_runtimes if _canonical(rt) in active] + dropped = sorted(set(target_runtimes) - set(out)) + if dropped: + # Surface the consequence so users can confirm their targets + # whitelist (explicit or directory-detected) took effect. + _rich_info(f"Skipped MCP config for {', '.join(dropped)} (not in active targets).") + _log.debug( + "Active-targets gate dropped: %s (active=%s)", + dropped, + sorted(active), + ) return out @staticmethod diff --git a/tests/unit/integration/test_mcp_integrator.py b/tests/unit/integration/test_mcp_integrator.py index ec40c5242..ecb648334 100644 --- a/tests/unit/integration/test_mcp_integrator.py +++ b/tests/unit/integration/test_mcp_integrator.py @@ -665,6 +665,9 @@ def test_install_uses_explicit_project_root_for_workspace_runtime_detection( (nested / ".cursor").mkdir(parents=True) (nested / ".opencode").mkdir() (nested / ".vscode").mkdir() + # Copilot's project profile detects on `.github/` (targets.py:330); + # without it the active-targets gate would drop vscode here. + (nested / ".github").mkdir() mock_mgr = mock_mgr_cls.return_value mock_mgr.is_runtime_available.return_value = False @@ -799,6 +802,8 @@ def test_target_singular_filters_unlisted_runtimes(self, mock_at, tmp_path): @patch("apm_cli.integration.targets.active_targets") def test_targets_multiple_values_keeps_all_listed(self, mock_at, tmp_path): + # `vscode` runtime canonicalizes to `copilot`, so when copilot is + # in active targets both `copilot` and `vscode` runtime writes pass. mock_at.side_effect = _fake_active_targets(["claude", "copilot"]) result = self._gate( ["claude", "copilot", "vscode", "codex", "cursor"], @@ -807,13 +812,15 @@ def test_targets_multiple_values_keeps_all_listed(self, mock_at, tmp_path): apm_config={"targets": ["claude", "copilot"]}, explicit_target=None, ) - assert result == ["claude", "copilot"] + assert result == ["claude", "copilot", "vscode"] - # -- no targets field: backward-compat (gate only project-scoped) ------ + # -- no targets field: directory-detection acts as the whitelist ------ @patch("apm_cli.integration.targets.active_targets") - def test_no_targets_gates_only_codex_claude(self, mock_at, tmp_path): - # active_targets returns copilot only -- codex/claude should be gated + def test_no_targets_uses_directory_detection_for_all_runtimes(self, mock_at, tmp_path): + # active_targets returns copilot only -- every other runtime gates, + # not just codex/claude. Mirrors `apm install` UX. Note: `vscode` + # canonicalizes to `copilot` so both pass when copilot is active. mock_at.side_effect = _fake_active_targets(["copilot"]) result = self._gate( ["copilot", "vscode", "codex", "claude", "cursor"], @@ -822,16 +829,11 @@ def test_no_targets_gates_only_codex_claude(self, mock_at, tmp_path): apm_config={}, explicit_target=None, ) - assert "copilot" in result - assert "vscode" in result - assert "cursor" in result - assert "codex" not in result - assert "claude" not in result + assert result == ["copilot", "vscode"] @patch("apm_cli.integration.targets.active_targets") - def test_no_targets_no_project_scoped_returns_all(self, mock_at, tmp_path): - # No codex/claude in list -> nothing to gate, return all - mock_at.side_effect = _fake_active_targets(["copilot"]) + def test_no_targets_keeps_all_when_all_directories_present(self, mock_at, tmp_path): + mock_at.side_effect = _fake_active_targets(["copilot", "vscode", "cursor"]) result = self._gate( ["copilot", "vscode", "cursor"], user_scope=False, @@ -845,7 +847,9 @@ def test_no_targets_no_project_scoped_returns_all(self, mock_at, tmp_path): @patch("apm_cli.integration.targets.active_targets") def test_explicit_target_overrides_config(self, mock_at, tmp_path): - mock_at.side_effect = _fake_active_targets(["vscode"]) + # Real active_targets normalizes "vscode" -> "copilot"; mirror that + # in the fake so the gate's vscode->copilot alias check sees a hit. + mock_at.side_effect = _fake_active_targets(["copilot"]) result = self._gate( ["claude", "copilot", "vscode", "codex"], user_scope=False, @@ -853,7 +857,7 @@ def test_explicit_target_overrides_config(self, mock_at, tmp_path): apm_config={"targets": ["claude", "copilot"]}, explicit_target="vscode", ) - assert result == ["vscode"] + assert result == ["copilot", "vscode"] @patch("apm_cli.integration.targets.active_targets") def test_explicit_target_without_config(self, mock_at, tmp_path): From 727ce8dd6bd2b09494093c8a56c29266876c98f0 Mon Sep 17 00:00:00 2001 From: Daniel Meppiel Date: Fri, 15 May 2026 09:02:24 +0200 Subject: [PATCH 5/7] fix: honor APM-install UX in MCP gate (audit-caught call-site bypass) Paired CLI-logging + DevX-UX advisory review caught five UX divergences between MCP install gating and the canonical 'apm install' targets engine. The most critical was a hidden call-site bypass that silently broke the PR's own promise. Audit-caught fix (B1, devx-ux blocking) commands/install.py:1795 built mcp_apm_config from APMPackage.target (singular legacy field) only. APMPackage.from_apm_yml never read 'targets:' plural, so for any user on the canonical syntax apm_package.target = None, the gate saw an empty config dict, and fell back to permissive directory detection. The whitelist was silently bypassed. - APMPackage now exposes a 'targets: list[str] | None' field and parses it in from_apm_yml. - The call site builds mcp_apm_config conditionally, forwarding only the key the user actually declared (avoids tripping the gate's target/targets mutex check with a None placeholder). - Regression test test_apm_package_targets_plural_forwards_through_call_site locks in the full apm.yml -> APMPackage -> mcp_apm_config path. CLI-logging UX parity (B1+B2+B3) - Malformed-targets branch now emits two _rich_error lines (red [x]) instead of a single _rich_warning that glued the multi-line EmptyTargetsListError body mid-sentence into a yellow [!]. Mirrors install/phases/targets.py:213's voice. No SystemExit because the gate runs mid-bundle in local_bundle_handler callers. - Drop branch now passes symbol='info' explicitly and includes '(active targets: ...)' in the same ' ' double-space shape as format_provenance's canonical 'Targets: X (source: Y)' line. DevX-UX parity (B3+B4) - apm install --mcp NAME help text mentions the gate. - docs/consumer/install-mcp-servers.md cross-links to install-packages 'Where files land' instead of duplicating the rule. Closes the PR's own promise (#1335) end-to-end. The gate was correct; the call site dropped 'targets:' plural before reaching it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 2 +- .../docs/consumer/install-mcp-servers.md | 29 +++++---- src/apm_cli/commands/install.py | 18 ++++-- src/apm_cli/integration/mcp_integrator.py | 37 +++++++++--- src/apm_cli/models/apm_package.py | 26 +++++++- tests/unit/integration/test_mcp_integrator.py | 60 +++++++++++++++++++ 6 files changed, 145 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cf7e4cf2a..c0d2f8877 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- MCP server installation now mirrors the `apm install` targets engine: the active target set (explicit `--target` / `targets:`, else directory detection, else `[copilot]` fallback) is the whitelist for every runtime. Previously only Codex and Claude Code were gated when `targets:` was omitted, and `targets: [copilot]` only became a hard whitelist when the singular `target:` key was used (so a stray `~/.codex/config.toml` write could still leak). A malformed `targets:` field (conflicting `target:` + `targets:`, or `targets: []`) now fails closed -- no MCP files are written -- with a warning naming the field. (#1335) +- MCP server installation now mirrors the `apm install` targets engine end-to-end: the active target set (explicit `--target` / `targets:`, else directory detection, else `[copilot]` fallback) is the whitelist for every runtime, and the user-facing logging follows the same `[i]` / `[x]` symbol contract. Previously only Codex and Claude Code were gated when `targets:` was omitted, and the call site silently dropped the modern `targets:` plural form before reaching the gate so `targets: [copilot, claude]` could still leak `~/.codex/config.toml`. Drops now emit `[i] Skipped MCP config for X (active targets: Y)`, mirroring the canonical `Targets: X (source: Y)` provenance line. A malformed `targets:` field (conflicting `target:` + `targets:`, or `targets: []`) fails closed and renders the same `[x]` red error voice + remediation block as `apm install`. (#1335) - 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) - `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) diff --git a/docs/src/content/docs/consumer/install-mcp-servers.md b/docs/src/content/docs/consumer/install-mcp-servers.md index 9a6e16cdd..c4a8fce62 100644 --- a/docs/src/content/docs/consumer/install-mcp-servers.md +++ b/docs/src/content/docs/consumer/install-mcp-servers.md @@ -93,19 +93,22 @@ This avoids creating runtime artifacts for tools you do not use. Gemini's user-scope path (`~/.gemini/settings.json`, selected with `-g`) is unconditional and creates `~/.gemini/` if needed. -When `apm.yml` declares `targets:` (or `--target` is passed on the -command line), MCP install treats that list as a hard whitelist -- -runtimes outside the list are skipped even if their harness directory -is present. With no explicit targets, MCP install falls back to the -same directory-detection rule `apm install` uses for skills and other -APM dependencies: a runtime is configured only when its harness -directory (`.github/`, `.claude/`, `.cursor/`, `.codex/`, -`.opencode/`, `.gemini/`) is present in the project, with greenfield -projects defaulting to Copilot. Either way, APM emits an info line -naming the skipped runtimes so you can confirm the gate took effect. -A malformed `targets:` field (e.g. both `target:` and `targets:` set, -or `targets: []`) fails closed: no MCP files are written and a -warning names the field to fix. (#1335) +MCP install honors the same target resolution chain as `apm install` +for any other dependency: see [Where files land](/consumer/install-packages/#where-files-land). +In short: `--target` wins, then `apm.yml`'s `targets:`, then +auto-detect from harness directories. Runtimes outside the active +set are skipped silently from a config-write perspective, but APM +emits an `[i]` line naming what got skipped and which targets are +active so you can confirm the gate took effect: + +```text +[i] Skipped MCP config for claude, codex (active targets: copilot) +``` + +A malformed `targets:` field (both `target:` and `targets:` set, or +`targets: []`) fails closed: no MCP files are written and an `[x]` +error names the field to fix. The voice and exit semantics mirror +the canonical `apm install` skills phase. (#1335) `apm install -g --mcp NAME` routes the write to each runtime's user-scope MCP config when that runtime supports user scope -- for diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index e63da3e15..fcf6896d1 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -978,7 +978,7 @@ def _handle_mcp_install( "mcp_name", default=None, metavar="NAME", - help="Add an MCP server entry to apm.yml. Use with --transport, --url, --env, --header, --mcp-version, or post-- stdio command.", + help="Add an MCP server entry to apm.yml. Use with --transport, --url, --env, --header, --mcp-version, or post-- stdio command. Honors the same target resolution chain as 'apm install' (--target > apm.yml targets: > auto-detect); runtimes outside the active set are skipped with an [i] line.", ) @click.option( "--transport", @@ -1792,10 +1792,18 @@ def _install_apm_packages(ctx, outcome): # Continue with MCP installation (existing logic) mcp_count = 0 new_mcp_servers: builtins.set = builtins.set() - mcp_apm_config = { - "target": apm_package.target, - "scripts": apm_package.scripts or {}, - } + # Forward only the targets-key the user actually declared so parse_targets_field + # in the gate sees the same dict shape it sees from raw apm.yml. Including a + # `targets: None` placeholder when the user wrote `target:` (singular) would + # falsely trip the conflict-mutex check (see core.apm_yml.parse_targets_field). + # This restores parity with `apm install` for users on the modern `targets:` + # plural form -- without this, `targets:` was silently dropped at the call + # site and the gate fell back to permissive directory detection (#1335). + mcp_apm_config: dict = {"scripts": apm_package.scripts or {}} + if apm_package.targets is not None: + mcp_apm_config["targets"] = apm_package.targets + elif apm_package.target is not None: + mcp_apm_config["target"] = apm_package.target if should_install_mcp and mcp_deps: mcp_count = MCPIntegrator.install( mcp_deps, diff --git a/src/apm_cli/integration/mcp_integrator.py b/src/apm_cli/integration/mcp_integrator.py index 69f550279..13d420f63 100644 --- a/src/apm_cli/integration/mcp_integrator.py +++ b/src/apm_cli/integration/mcp_integrator.py @@ -23,10 +23,9 @@ from apm_cli.deps.lockfile import LockFile, get_lockfile_path from apm_cli.utils.console import ( _get_console, # noqa: F401 -- module attribute; patched by tests and used via re-export - _rich_error, # noqa: F401 + _rich_error, _rich_info, _rich_success, - _rich_warning, ) _log = logging.getLogger(__name__) @@ -993,10 +992,23 @@ def _gate_project_scoped_runtimes( # Fail closed: write nothing rather than fall back to permissive # auto-detect, which would widen the MCP write surface beyond # the user's stated intent. - _rich_warning( - f"apm.yml targets field is invalid: {exc}. Skipping MCP " - "install for all runtimes. Fix the manifest and re-run." + # + # Voice mirrors the canonical `apm install` skills phase, which + # surfaces the same exception as a red [x] error block via + # logger.error(..., symbol="") (install/phases/targets.py:213). + # We render the body with symbol="" because the exception text + # already begins with "[x] ..." (see core/errors.py and + # core/apm_yml.EmptyTargetsListError). symbol="" suppresses the + # auto-prefix so the renderer doesn't double-stamp the marker. + # Exit semantics differ deliberately: the gate may be invoked + # mid-bundle-install (local_bundle_handler) where SystemExit(2) + # would corrupt partial state, so we fail closed and continue. + _rich_error( + "MCP install: apm.yml targets field is invalid. " + "Skipping MCP config writes for all runtimes.", + symbol="error", ) + _rich_error(str(exc), symbol="") _log.debug( "parse_targets_field failed; failing closed (no MCP writes)", exc_info=True, @@ -1029,8 +1041,19 @@ def _canonical(rt: str) -> str: dropped = sorted(set(target_runtimes) - set(out)) if dropped: # Surface the consequence so users can confirm their targets - # whitelist (explicit or directory-detected) took effect. - _rich_info(f"Skipped MCP config for {', '.join(dropped)} (not in active targets).") + # whitelist (explicit or directory-detected) took effect. Mirror + # the shape of the canonical `Targets: X (source: Y)` provenance + # line emitted by the skills phase (target_detection.format_provenance): + # name the dropped runtimes AND the active set on a single line so + # the user can audit the decision input without re-reading apm.yml. + # Double-space before "(active targets:" matches the canonical + # provenance line spacing (install/phases/targets.py:265 + + # core/target_detection.py:777). + _rich_info( + f"Skipped MCP config for {', '.join(dropped)} " + f"(active targets: {', '.join(sorted(active))})", + symbol="info", + ) _log.debug( "Active-targets gate dropped: %s (active=%s)", dropped, diff --git a/src/apm_cli/models/apm_package.py b/src/apm_cli/models/apm_package.py index 84ed9f79f..bd458b4ac 100644 --- a/src/apm_cli/models/apm_package.py +++ b/src/apm_cli/models/apm_package.py @@ -92,7 +92,15 @@ class APMPackage: # project root. source_path: Path | None = None target: str | list[str] | None = ( - None # Target agent(s): single string or list (applies to compile and install) + None # Singular 'target:' field (legacy/CSV form). May coexist with `targets` + # being None in apm.yml, but never both populated -- ConflictingTargetsError + # is raised at install time. Read by callers that only need a single value. + ) + targets: list[str] | None = ( + None # Plural 'targets:' field (canonical YAML-list form, #1335). Stored raw + # so the install gate (mcp_integrator._gate_project_scoped_runtimes) can + # re-validate via parse_targets_field with the same dict shape it sees from + # raw apm.yml. None means the user did not declare 'targets:' at all. ) type: PackageContentType | None = ( None # Package content type: instructions, skill, hybrid, or prompts @@ -267,6 +275,21 @@ def from_apm_yml( source_path=apm_yml_path, ) + # Plural 'targets:' field is stored raw (no canonical validation here) + # so the MCP install gate at mcp_integrator._gate_project_scoped_runtimes + # can re-run parse_targets_field on a dict that mirrors apm.yml shape + # and surface the same conflict / empty-list errors uniformly. Without + # this passthrough, the call site at commands/install.py would silently + # bypass the targets whitelist for any user on the modern plural form + # (#1335 regression caught in PR #1336 audit). + targets_value: list[str] | None = None + if "targets" in data and data["targets"] is not None: + raw_targets = data["targets"] + if isinstance(raw_targets, list): + targets_value = [str(t).strip() for t in raw_targets if str(t).strip()] + else: + targets_value = [str(raw_targets).strip()] + result = cls( name=data["name"], version=data["version"], @@ -279,6 +302,7 @@ def from_apm_yml( package_path=apm_yml_path.parent, source_path=resolved_source, target=target_value, + targets=targets_value, type=pkg_type, includes=includes, ) diff --git a/tests/unit/integration/test_mcp_integrator.py b/tests/unit/integration/test_mcp_integrator.py index ecb648334..92c9bd947 100644 --- a/tests/unit/integration/test_mcp_integrator.py +++ b/tests/unit/integration/test_mcp_integrator.py @@ -955,3 +955,63 @@ def _resolve(_root, explicit=None): assert "claude" in result assert "copilot" in result assert "codex" not in result + + # -- call-site forwarding regression (PR #1336 audit, devx-ux B1) ------ + + def test_apm_package_targets_plural_forwards_through_call_site(self, tmp_path): + """Regression: `targets:` plural in apm.yml must reach the gate. + + Before the audit fix, ``commands/install.py`` built + ``mcp_apm_config = {"target": apm_package.target, ...}`` -- which is + always None for users on the modern ``targets:`` form. The gate then + saw an empty config dict and fell back to permissive directory + detection, silently bypassing the whitelist the user explicitly set. + + This test exercises the full call-site path: load real apm.yml -> + APMPackage -> build mcp_apm_config the way commands/install.py does + -> assert the gate sees the plural list. + """ + from apm_cli.core.apm_yml import parse_targets_field + from apm_cli.models.apm_package import APMPackage + + apm_yml = tmp_path / "apm.yml" + apm_yml.write_text( + "name: gate-regression\nversion: 0.0.1\ntargets:\n - copilot\n - claude\n" + ) + pkg = APMPackage.from_apm_yml(apm_yml) + assert pkg.targets == ["copilot", "claude"], ( + "APMPackage must expose 'targets:' plural; otherwise the call site " + "cannot forward it to the gate." + ) + + # Mirror the exact dict construction in commands/install.py:1795. + mcp_apm_config: dict = {"scripts": pkg.scripts or {}} + if pkg.targets is not None: + mcp_apm_config["targets"] = pkg.targets + elif pkg.target is not None: + mcp_apm_config["target"] = pkg.target + + # The gate's parse_targets_field must now see the plural list. + assert parse_targets_field(mcp_apm_config) == ["copilot", "claude"] + + @patch("apm_cli.integration.targets.active_targets") + def test_dropped_runtime_message_includes_active_targets(self, mock_at, tmp_path, capsys): + """Negative-case message must name the active set (cli-logging B3). + + Without "(active targets: ...)" the user has to grep apm.yml to + confirm what the gate did. Mirrors the canonical provenance line + shape ``Targets: X (source: Y)``. + """ + mock_at.side_effect = _fake_active_targets(["copilot"]) + self._gate( + ["copilot", "claude", "codex"], + user_scope=False, + project_root=tmp_path, + apm_config={"targets": ["copilot"]}, + explicit_target=None, + ) + out = capsys.readouterr().out + assert "Skipped MCP config for claude, codex" in out + assert "active targets: copilot" in out + # Symbol prefix asserts the gate honors the [+]/[!]/[i]/[x] contract. + assert "[i]" in out From b68e512a1af365cfcfab246bdb12160f7317933b Mon Sep 17 00:00:00 2001 From: Daniel Meppiel Date: Fri, 15 May 2026 09:28:37 +0200 Subject: [PATCH 6/7] fix: extend MCP-gate strictness to greenfield + apply panel docs/UX polish Closes the last asymmetry vs 'apm install': greenfield projects (no targets:, no --target, no detected signals) now fail closed with the same NoHarnessError voice as the canonical install path, instead of silently falling back to [copilot] for the MCP write. Apply apm-review-panel recommendations: - scsec R2: catch UnknownTargetError in the gate so malformed --target value renders an [x] error instead of a Python traceback. - scsec N1: local_bundle_handler builds plural 'targets:' list shape (was a CSV string -- pre-empted dependency-shape drift). - cli-log N1, N2: drop-line wording tightened ('Skipping all MCP config writes' for hard-fail; double-space lock between message and parens). - py-arch nit3: shared RUNTIME_TO_CANONICAL_TARGET dict in integration/targets.py used by both alias-canonicalization in the gate and the runtime adapter resolver. - devux N1 + growth N1: --mcp NAME help text wordsmithed to land the 'gated by targets:' contract in one line. - doc-writer R1: packages/apm-guide commands.md --mcp row spells out the gate behavior + the -g carve-out. - doc-writer R2: docs/reference/targets-matrix.md MCP section no longer claims unconditional writes 'for every target with an adapter'. - doc-writer N1, N2 + scsec R1: docs/consumer/install-mcp-servers.md consolidates per-runtime opt-in + project-gate paragraphs into one rule, mentions greenfield NoHarnessError parity, and frames the -g user-scope carve-out explicitly. - tc R1: end-to-end gating integration test covers whitelist suppression, multi-target allow, and greenfield fail-closed. - growth R1: CHANGELOG leads with the user-visible delta, then audit detail, then the strictness extension and -g carve-out. 8480 unit tests + 3 new integration tests pass; ruff check + format both silent. Refs #1335 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 2 +- .../docs/consumer/install-mcp-servers.md | 54 +++-- .../content/docs/reference/targets-matrix.md | 14 +- .../.apm/skills/apm-usage/commands.md | 2 +- src/apm_cli/commands/install.py | 7 +- src/apm_cli/install/local_bundle_handler.py | 14 +- src/apm_cli/integration/mcp_integrator.py | 179 +++++++++------ src/apm_cli/integration/targets.py | 22 +- .../test_mcp_targets_gating_e2e.py | 209 ++++++++++++++++++ .../test_install_local_bundle_issue1207.py | 8 +- tests/unit/integration/test_mcp_integrator.py | 195 ++++++++++------ 11 files changed, 535 insertions(+), 171 deletions(-) create mode 100644 tests/integration/test_mcp_targets_gating_e2e.py diff --git a/CHANGELOG.md b/CHANGELOG.md index c0d2f8877..8541b0e06 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- MCP server installation now mirrors the `apm install` targets engine end-to-end: the active target set (explicit `--target` / `targets:`, else directory detection, else `[copilot]` fallback) is the whitelist for every runtime, and the user-facing logging follows the same `[i]` / `[x]` symbol contract. Previously only Codex and Claude Code were gated when `targets:` was omitted, and the call site silently dropped the modern `targets:` plural form before reaching the gate so `targets: [copilot, claude]` could still leak `~/.codex/config.toml`. Drops now emit `[i] Skipped MCP config for X (active targets: Y)`, mirroring the canonical `Targets: X (source: Y)` provenance line. A malformed `targets:` field (conflicting `target:` + `targets:`, or `targets: []`) fails closed and renders the same `[x]` red error voice + remediation block as `apm install`. (#1335) +- MCP server installation now respects the `targets:` whitelist exactly like `apm install`: drop a non-listed runtime even when its `.cursor/`, `.codex/`, or other on-disk signal exists. Previously the MCP install path called `active_targets()` reading the singular `target:` key only, so projects whitelisting `targets: [copilot]` could still receive `~/.codex/config.toml` and `.cursor/mcp.json` writes from foreign signals. The fix audits both paths: (a) the call site at `local_bundle_handler.py` now forwards the canonical plural list; (b) the gate now delegates to the same `resolve_targets` resolver that backs `apm install` skills, so a malformed `targets:` field (conflicting `target:` + `targets:`, `targets: []`, or unknown target name) fails closed with the same `[x]` red error voice + remediation block. The same delegation closes a related asymmetry: a greenfield project (no `targets:`, no `--target` flag, no detected signals) used to silently fall back to `[copilot]` for MCP-only invocations, while `apm install` raised `NoHarnessError` on the same input -- both surfaces now error consistently. Drop lines now use the `[i] Skipped MCP config for X (active targets: Y)` format mirroring the canonical `Targets: X (source: Y)` provenance line. The `-g`/`--global` carve-out is unchanged: `apm install -g --mcp NAME` writes to user-scope (`~/.config/...`, `~/.codex/`, etc.) bypassing the project-scope gate by design. (#1335) - 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) - `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) diff --git a/docs/src/content/docs/consumer/install-mcp-servers.md b/docs/src/content/docs/consumer/install-mcp-servers.md index c4a8fce62..d832d9741 100644 --- a/docs/src/content/docs/consumer/install-mcp-servers.md +++ b/docs/src/content/docs/consumer/install-mcp-servers.md @@ -86,36 +86,46 @@ writes a runtime-specific MCP config file. The schemas differ; the | OpenCode | `opencode.json` | project (only if `.opencode/` exists) | JSON `mcp` | | Windsurf | `~/.codeium/windsurf/mcp_config.json` | global | JSON `mcpServers` | -Cursor, Gemini, and OpenCode are opt-in by directory in project scope: -APM only writes their config when the corresponding `.cursor/`, -`.gemini/`, or `.opencode/` directory already exists in the project. -This avoids creating runtime artifacts for tools you do not use. -Gemini's user-scope path (`~/.gemini/settings.json`, selected with -`-g`) is unconditional and creates `~/.gemini/` if needed. +## How `targets:` gates which configs get written MCP install honors the same target resolution chain as `apm install` -for any other dependency: see [Where files land](/consumer/install-packages/#where-files-land). +for any other dependency: see +[Where files land](/consumer/install-packages/#where-files-land). In short: `--target` wins, then `apm.yml`'s `targets:`, then -auto-detect from harness directories. Runtimes outside the active -set are skipped silently from a config-write perspective, but APM -emits an `[i]` line naming what got skipped and which targets are -active so you can confirm the gate took effect: +auto-detect from harness directories. + +When a runtime is outside the active target set, APM does NOT write +its MCP config -- and announces the drop on stdout so you can confirm +the gate took effect: ```text [i] Skipped MCP config for claude, codex (active targets: copilot) ``` -A malformed `targets:` field (both `target:` and `targets:` set, or -`targets: []`) fails closed: no MCP files are written and an `[x]` -error names the field to fix. The voice and exit semantics mirror -the canonical `apm install` skills phase. (#1335) - -`apm install -g --mcp NAME` routes the write to each runtime's -user-scope MCP config when that runtime supports user scope -- for -example Copilot CLI writes to `~/.copilot/mcp-config.json`, Codex -CLI to `~/.codex/config.toml`, and Gemini CLI to -`~/.gemini/settings.json`. Workspace-only runtimes (VS Code, Cursor, -OpenCode) are skipped at user scope. +This single rule replaces two older ones that used to coexist: + +- A "directory opt-in" carve-out for Cursor / Gemini / OpenCode -- now + redundant, because `targets:` (or auto-detection) drives the gate + for those runtimes too. +- The pre-#1335 silent skip path, which dropped non-listed runtimes + without telling you. + +A malformed `targets:` field (both `target:` and `targets:` set, +`targets: []`, or an unknown target name) fails closed: no MCP files +are written and an `[x]` error names the field to fix. A greenfield +project with no `targets:`, no `--target` flag, AND no detected +signals (`.github/copilot-instructions.md`, `.cursor/`, etc.) also +fails closed with the same `[x]` voice -- consistent with how +`apm install` treats the same input. Pin a target with `--target` or +declare one in `apm.yml`. (#1335) + +`apm install -g --mcp NAME` is a deliberate carve-out: it routes the +write to each runtime's user-scope MCP config (Copilot CLI to +`~/.copilot/mcp-config.json`, Codex CLI to `~/.codex/config.toml`, +Gemini CLI to `~/.gemini/settings.json`) and does not consult the +project-scope `targets:` whitelist -- user-scope writes are by +definition not project-bound. Workspace-only runtimes (VS Code, +Cursor, OpenCode) are skipped at user scope. ## stdio vs HTTP servers diff --git a/docs/src/content/docs/reference/targets-matrix.md b/docs/src/content/docs/reference/targets-matrix.md index 8c2566292..8abd468f0 100644 --- a/docs/src/content/docs/reference/targets-matrix.md +++ b/docs/src/content/docs/reference/targets-matrix.md @@ -182,7 +182,19 @@ To restore the pre-convergence per-target layout (skills land under each target' ## MCP servers -MCP is not a `TargetProfile` primitive; it is wired by a separate integrator that writes per-client config files (e.g. `.vscode/mcp.json`, `.cursor/mcp.json`, `.claude.json`) for every target with an MCP client adapter. The matrix above marks `mcp` supported when an adapter exists. See [`apm mcp`](../cli/mcp/) for the runtime surface. +MCP is not a `TargetProfile` primitive; it is wired by a separate +integrator that writes per-client config files (e.g. +`.vscode/mcp.json`, `.cursor/mcp.json`, `.claude.json`) for every +target in the active set that has an MCP client adapter. Active set +follows the same `--target` > `targets:` > auto-detect chain as +`apm install`: a runtime with an adapter but outside the active set +is skipped and APM emits an `[i] Skipped MCP config for X (active +targets: Y)` line so the gate decision is observable. The matrix +above marks `mcp` supported when an adapter exists; whether the +config gets written on a given install is a function of the active +target set, not just adapter availability. See +[Install MCP servers](../../consumer/install-mcp-servers/) for the +gate behavior and [`apm mcp`](../cli/mcp/) for the runtime surface. ## See also diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index 32b3e3af0..1e311ff51 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -10,7 +10,7 @@ | Command | Purpose | Key flags | |---------|---------|-----------| -| `apm install [PKGS...]` | Install APM and MCP dependencies (supports APM packages, Claude skills (SKILL.md), and plugin collections (plugin.json)) | `--update` (deprecated; prefer `apm update`) refresh refs, `--force` overwrite (does NOT refresh refs; use `apm update` for that), `--frozen` CI-safe install that fails fast when `apm.lock.yaml` is missing or out of sync with `apm.yml` (mutually exclusive with `--update`; structural presence check only -- use `apm audit` for SHA integrity), `--dry-run`, `--verbose`, `--only [apm\|mcp]`, `--target` (comma-separated, e.g. `--target claude,cursor`; highest-priority entry in the resolution chain `--target` > apm.yml `targets:` > auto-detect; `--target all` deprecated, see `apm compile --all`; use `copilot-cowork` with `--global` after `apm experimental enable copilot-cowork`), `--dev`, `-g` global, `--trust-transitive-mcp`, `--parallel-downloads N`, `--allow-insecure`, `--allow-insecure-host HOSTNAME`, `--skill NAME` install named skill(s) from SKILL_BUNDLE (repeatable; persisted in apm.yml; `'*'` resets to all), `--legacy-skill-paths` restore per-client skill dirs, `--mcp NAME` add MCP entry, `--transport`, `--url`, `--env KEY=VAL`, `--header KEY=VAL`, `--mcp-version`, `--registry URL` custom MCP registry | +| `apm install [PKGS...]` | Install APM and MCP dependencies (supports APM packages, Claude skills (SKILL.md), and plugin collections (plugin.json)) | `--update` (deprecated; prefer `apm update`) refresh refs, `--force` overwrite (does NOT refresh refs; use `apm update` for that), `--frozen` CI-safe install that fails fast when `apm.lock.yaml` is missing or out of sync with `apm.yml` (mutually exclusive with `--update`; structural presence check only -- use `apm audit` for SHA integrity), `--dry-run`, `--verbose`, `--only [apm\|mcp]`, `--target` (comma-separated, e.g. `--target claude,cursor`; highest-priority entry in the resolution chain `--target` > apm.yml `targets:` > auto-detect; `--target all` deprecated, see `apm compile --all`; use `copilot-cowork` with `--global` after `apm experimental enable copilot-cowork`), `--dev`, `-g` global, `--trust-transitive-mcp`, `--parallel-downloads N`, `--allow-insecure`, `--allow-insecure-host HOSTNAME`, `--skill NAME` install named skill(s) from SKILL_BUNDLE (repeatable; persisted in apm.yml; `'*'` resets to all), `--legacy-skill-paths` restore per-client skill dirs, `--mcp NAME` add MCP entry (NAME goes through the same `--target` > `targets:` > auto-detect resolver as APM packages, so a project whitelisting `targets: [copilot]` will not write `.cursor/mcp.json` even if `.cursor/` exists; `apm install -g --mcp NAME` writes user-scope and bypasses the project-scope gate by design), `--transport`, `--url`, `--env KEY=VAL`, `--header KEY=VAL`, `--mcp-version`, `--registry URL` custom MCP registry | | `apm targets` | Show resolved deployment targets for the current project (Click group; reads filesystem signals; works with or without `apm.yml`) | `--all` also include the `agent-skills` meta-target (only meaningful with `--json`), `--json` machine-readable output. No provenance line is printed (the table is the provenance). | | `apm uninstall PKGS...` | Remove packages | `--dry-run`, `-g` global | | `apm prune` | Remove orphaned packages | `--dry-run` | diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index fcf6896d1..653c52cc6 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -978,7 +978,12 @@ def _handle_mcp_install( "mcp_name", default=None, metavar="NAME", - help="Add an MCP server entry to apm.yml. Use with --transport, --url, --env, --header, --mcp-version, or post-- stdio command. Honors the same target resolution chain as 'apm install' (--target > apm.yml targets: > auto-detect); runtimes outside the active set are skipped with an [i] line.", + help=( + "Add an MCP server entry to apm.yml. Use with --transport, --url, --env, " + "--header, --mcp-version, or a stdio command after `--`. Resolves active " + "targets the same way `apm install` does (--target > apm.yml targets: > " + "auto-detect); writes only for active targets, skips others with [i]." + ), ) @click.option( "--transport", diff --git a/src/apm_cli/install/local_bundle_handler.py b/src/apm_cli/install/local_bundle_handler.py index 505115d74..ae5d42a49 100644 --- a/src/apm_cli/install/local_bundle_handler.py +++ b/src/apm_cli/install/local_bundle_handler.py @@ -364,8 +364,8 @@ def _wire_bundle_mcp_servers( from apm_cli.integration.mcp_integrator import MCPIntegrator - target_csv = ",".join(t.name for t in targets) - apm_config = {"target": target_csv, "scripts": {}} + target_names = [t.name for t in targets] + apm_config = {"targets": target_names, "scripts": {}} try: count = MCPIntegrator.install( deps, @@ -373,7 +373,7 @@ def _wire_bundle_mcp_servers( apm_config=apm_config, project_root=project_root, user_scope=user_scope, - explicit_target=target_csv, + explicit_target=target_names, logger=logger, ) except Exception as exc: @@ -385,15 +385,15 @@ def _wire_bundle_mcp_servers( return 0 if count: - logger.success( - f"Wired {count} MCP server(s) from bundle .mcp.json (target(s): {target_csv})" - ) + joined = ", ".join(target_names) + logger.success(f"Wired {count} MCP server(s) from bundle .mcp.json (target(s): {joined})") elif deps: # Bundle declared servers but none applied (e.g. resolved targets # all gated out, or all servers already configured). Emit an info # line so users have a paper-trail. + joined = ", ".join(target_names) logger.info( f"Bundle .mcp.json declared {len(deps)} server(s); " - f"no new MCP config changes for target(s): {target_csv}" + f"no new MCP config changes for target(s): {joined}" ) return count diff --git a/src/apm_cli/integration/mcp_integrator.py b/src/apm_cli/integration/mcp_integrator.py index 13d420f63..d9ba7c7d4 100644 --- a/src/apm_cli/integration/mcp_integrator.py +++ b/src/apm_cli/integration/mcp_integrator.py @@ -952,60 +952,80 @@ def _gate_project_scoped_runtimes( ) -> list[str]: """Filter *target_runtimes* against the project's active targets. - Mirrors the UX of ``apm install`` for apm dependencies: the active - target set (explicit ``--target`` / ``targets:`` field, else - directory detection, else fallback ``[copilot]``) is the whitelist - for MCP writes too. Runtimes outside that set are skipped with an - info line naming them, rather than leaking config files into - projects that are not configured for them (#1335). - - ``explicit_target`` accepts ``str`` (single token), ``list[str]``, - or a CSV string (``"claude,copilot"``) -- the latter is produced by - ``install/local_bundle_handler.py::_wire_bundle_mcp_servers`` when - wiring multi-target bundles. CSV strings are normalized to a list - before they reach :func:`active_targets`, which only understands - single tokens or list input. + UX parity with ``apm install`` for apm dependencies: the active + target set (explicit ``--target`` > ``targets:`` field > + directory-signal detection) is the whitelist for MCP writes. Any + runtime outside that set is skipped with an info line naming both + what was dropped and the active set, so users can audit the + decision input without re-reading apm.yml (#1335). + + Strict resolution model -- mirrors :func:`resolve_targets`, + the same call ``apm install`` uses + (``install/phases/targets.py:233``): + + - flag > yaml-targets > directory signals (no permissive + "fallback to copilot" greenfield default); + - no flag, no ``targets:``, and no harness-signal directory -> + :class:`NoHarnessError` (red ``[x]``, write nothing); + - multiple ambiguous signals with no disambiguation -> + :class:`AmbiguousHarnessError` (same fail-closed shape). + + ``explicit_target`` accepts ``str``, ``list[str]``, or a CSV + string (``"claude,copilot"``) -- the latter is produced by + legacy callers; it is normalized to a list before the resolver + is invoked so the canonical-name validator does not reject it as + one unknown token. A malformed ``targets:`` field (conflicting ``target:`` + - ``targets:``, or ``targets: []``) fails closed: nothing is written. + ``targets:``, ``targets: []``, or unknown canonical name) likewise + fails closed: nothing is written. + + Exit semantics differ deliberately from ``install/phases/targets.py``: + the canonical install phase calls ``raise SystemExit(2)`` when + resolution fails; this gate may be invoked mid-bundle (see + ``install/local_bundle_handler``) where a hard exit would corrupt + partial state, so we render the same red ``[x]`` voice and return + an empty list (fail-closed-continue). + + ``user_scope=True`` is a deliberate carve-out: user-scope writes + target ``~/.config`` paths the user owns globally, so the + project-level whitelist is irrelevant. Documented in the + consumer install-mcp-servers guide. """ - # Security note: user-scope writes target ~/.config paths the user owns - # globally; gating only applies to project-scoped writes that could - # affect shared repos. See enterprise/security model. if user_scope: return target_runtimes from apm_cli.core.apm_yml import ( ConflictingTargetsError, EmptyTargetsListError, + UnknownTargetError, parse_targets_field, ) - from apm_cli.integration.targets import active_targets + from apm_cli.core.errors import ( + AmbiguousHarnessError, + NoHarnessError, + ) + from apm_cli.core.target_detection import resolve_targets + from apm_cli.integration.targets import RUNTIME_TO_CANONICAL_TARGET - # --- resolve explicit targets from config ------------------------- - explicit_from_config: list[str] = [] + # --- step 1: parse declared targets (fail-closed on any invalid form) + yaml_targets: list[str] | None = None if apm_config: try: - explicit_from_config = parse_targets_field(apm_config) - except (ConflictingTargetsError, EmptyTargetsListError) as exc: - # Manifest declared targets but the declaration is unparseable. - # Fail closed: write nothing rather than fall back to permissive - # auto-detect, which would widen the MCP write surface beyond - # the user's stated intent. - # - # Voice mirrors the canonical `apm install` skills phase, which - # surfaces the same exception as a red [x] error block via - # logger.error(..., symbol="") (install/phases/targets.py:213). - # We render the body with symbol="" because the exception text - # already begins with "[x] ..." (see core/errors.py and - # core/apm_yml.EmptyTargetsListError). symbol="" suppresses the - # auto-prefix so the renderer doesn't double-stamp the marker. - # Exit semantics differ deliberately: the gate may be invoked - # mid-bundle-install (local_bundle_handler) where SystemExit(2) - # would corrupt partial state, so we fail closed and continue. + parsed = parse_targets_field(apm_config) + yaml_targets = parsed if parsed else None + except ( + ConflictingTargetsError, + EmptyTargetsListError, + UnknownTargetError, + ) as exc: + # Voice mirrors the canonical `apm install` skills phase + # (install/phases/targets.py:213): red [x] lead-with-outcome, + # then the structured error body. symbol="" suppresses the + # auto-prefix on the body because the exception text already + # begins with "[x] ..." (see core/errors.py). _rich_error( - "MCP install: apm.yml targets field is invalid. " - "Skipping MCP config writes for all runtimes.", + "Skipping all MCP config writes -- apm.yml 'targets' field is invalid.", symbol="error", ) _rich_error(str(exc), symbol="") @@ -1015,43 +1035,66 @@ def _gate_project_scoped_runtimes( ) return [] - # Normalize CSV strings ("claude,copilot") to a list before active_targets, - # which treats a CSV string as a single unknown token (returns []). - normalized_explicit: str | list[str] | None + # --- step 2: normalize CSV explicit_target sugar to a list ----- + # `_wire_bundle_mcp_servers` historically passes a CSV string; the + # canonical-name validator inside _resolve_targets_v2 would reject + # the whole CSV as one unknown token. Normalize first. + flag: str | list[str] | None if isinstance(explicit_target, str) and "," in explicit_target: - normalized_explicit = [t.strip() for t in explicit_target.split(",") if t.strip()] + flag = [t.strip() for t in explicit_target.split(",") if t.strip()] else: - normalized_explicit = explicit_target - - # `parse_targets_field` returns [] when neither key is present, so the - # falsy chain below correctly falls through to auto-detect in that case. - config_target = normalized_explicit or explicit_from_config or None - + flag = explicit_target + + # Apply the runtime->canonical-target alias BEFORE passing the flag + # to resolve_targets. The canonical-name validator inside the + # resolver only knows about CANONICAL_TARGETS (claude/copilot/...); + # it rejects runtime aliases (vscode/agents) as unknown tokens. + # The MCP gate, however, must accept those aliases because users + # naturally type `--target vscode` for the VS Code Copilot runtime. + if flag is not None: + tokens = [flag] if isinstance(flag, str) else list(flag) + flag = [RUNTIME_TO_CANONICAL_TARGET.get(t, t) for t in tokens] + + # --- step 3: delegate to the canonical v2 resolver ------------- + # This is the same call the `apm install` skills phase makes at + # install/phases/targets.py:233. It enforces the strict + # flag > yaml > signals chain and raises NoHarnessError / + # AmbiguousHarnessError on greenfield / under-disambiguated + # projects -- the ASYMMETRY closed by this PR is that the gate + # used to silently fall back to [copilot] in those cases. root = project_root or Path.cwd() - active = {t.name for t in active_targets(root, config_target)} + try: + resolved = resolve_targets(root, flag=flag, yaml_targets=yaml_targets) + except (NoHarnessError, AmbiguousHarnessError) as exc: + _rich_error( + "Skipping all MCP config writes -- could not resolve active targets.", + symbol="error", + ) + _rich_error(str(exc), symbol="") + _log.debug( + "resolve_targets failed; failing closed (no MCP writes)", + exc_info=True, + ) + return [] - # Runtime name "vscode" maps to canonical target "copilot" (the same - # alias active_targets honors for explicit_target). Without this map - # a project with `.github/` set but no `.copilot/` would correctly - # detect copilot but still gate the vscode runtime. - def _canonical(rt: str) -> str: - return "copilot" if rt in ("vscode", "agents") else rt + active = set(resolved.targets) - out = [rt for rt in target_runtimes if _canonical(rt) in active] + # Runtime name "vscode" maps to canonical target "copilot" (same + # alias active_targets honors); shared table prevents drift with + # the alias resolution in integration/targets.py. + out = [rt for rt in target_runtimes if RUNTIME_TO_CANONICAL_TARGET.get(rt, rt) in active] dropped = sorted(set(target_runtimes) - set(out)) if dropped: - # Surface the consequence so users can confirm their targets - # whitelist (explicit or directory-detected) took effect. Mirror - # the shape of the canonical `Targets: X (source: Y)` provenance - # line emitted by the skills phase (target_detection.format_provenance): - # name the dropped runtimes AND the active set on a single line so - # the user can audit the decision input without re-reading apm.yml. - # Double-space before "(active targets:" matches the canonical - # provenance line spacing (install/phases/targets.py:265 + - # core/target_detection.py:777). + # Mirror the canonical `Targets: X (source: Y)` provenance shape + # (install/phases/targets.py:265, core/target_detection.py:777): + # double-space before the parenthetical. The "or ''" guard is + # defensive -- an empty active set is unreachable when + # _resolve_targets_v2 succeeded, but if a future contract change + # widens that contract we surface "" rather than render + # "(active targets: )" which reads as a renderer bug. + active_csv = ", ".join(sorted(active)) or "" _rich_info( - f"Skipped MCP config for {', '.join(dropped)} " - f"(active targets: {', '.join(sorted(active))})", + f"Skipped MCP config for {', '.join(dropped)} (active targets: {active_csv})", symbol="info", ) _log.debug( diff --git a/src/apm_cli/integration/targets.py b/src/apm_cli/integration/targets.py index 15ac88060..fc6662162 100644 --- a/src/apm_cli/integration/targets.py +++ b/src/apm_cli/integration/targets.py @@ -317,6 +317,24 @@ def for_scope(self, user_scope: bool = False) -> TargetProfile | None: return replace(self, root_dir=new_root, primitives=filtered) +# ------------------------------------------------------------------ +# Runtime -> canonical target alias map +# ------------------------------------------------------------------ +# +# Several runtime identifiers used at the MCP-config layer (e.g. ``vscode``, +# ``agents``) emit configuration that lands inside the ``copilot`` target's +# tree. The MCP gate (``mcp_integrator._gate_project_scoped_runtimes``) and +# the explicit-target resolution branch in :func:`active_targets` both need +# to map runtime -> canonical-target name in the same way. Hold the table +# in one place to prevent the two sites drifting -- a silent drift would +# strip a runtime even when its canonical target is active (the same class +# of bug as #1335). +RUNTIME_TO_CANONICAL_TARGET: dict[str, str] = { + "vscode": "copilot", + "agents": "copilot", +} + + # ------------------------------------------------------------------ # Known targets # ------------------------------------------------------------------ @@ -700,7 +718,7 @@ def active_targets_user_scope( profiles: list = [] seen: set = set() for t in raw: - canonical = "copilot" if t in ("copilot", "vscode", "agents") else t + canonical = RUNTIME_TO_CANONICAL_TARGET.get(t, t) if canonical == "all": from apm_cli.core.target_detection import EXPLICIT_ONLY_TARGETS @@ -773,7 +791,7 @@ def active_targets( profiles: list = [] seen: set = set() for t in raw: - canonical = "copilot" if t in ("copilot", "vscode", "agents") else t + canonical = RUNTIME_TO_CANONICAL_TARGET.get(t, t) if canonical == "all": # Exclude explicit-only targets (agent-skills) -- they must # be requested individually. diff --git a/tests/integration/test_mcp_targets_gating_e2e.py b/tests/integration/test_mcp_targets_gating_e2e.py new file mode 100644 index 000000000..7c01a7193 --- /dev/null +++ b/tests/integration/test_mcp_targets_gating_e2e.py @@ -0,0 +1,209 @@ +"""E2E integration tests for the MCP gate honoring ``targets:``. + +Closes the test gap surfaced in PR #1336 (issue #1335): the MCP install +path used to call ``active_targets()`` with the singular ``target:`` key +only, so a project whitelisting ``targets: [copilot]`` would still write +``.cursor/mcp.json`` and ``.codex/config.toml`` if a foreign signal +existed on disk. + +These tests exercise the real ``MCPIntegrator.install`` against on-disk +project layouts -- no mocks of the gate, no mocks of the resolver -- to +prove: + +1. A ``targets: [copilot]`` whitelist suppresses every non-copilot + per-runtime config write even when foreign signals exist. +2. A ``targets: [copilot, cursor]`` whitelist allows both writes. +3. The greenfield strictness contract holds: no ``targets:``, no + ``--target`` flag, no detectable signals -> NO MCP writes happen + anywhere (the gate fails closed; the asymmetry between MCP install + and ``apm install`` is closed). +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from apm_cli.integration.mcp_integrator import MCPIntegrator +from apm_cli.models.dependency.mcp import MCPDependency + +pytestmark = pytest.mark.integration + + +def _make_stdio_dep(name: str = "test-srv") -> MCPDependency: + """Build a synthetic stdio MCP dependency that needs no network.""" + return MCPDependency.from_dict( + { + "name": name, + "registry": False, + "transport": "stdio", + "command": "echo", + "args": ["mcp-targets-gate-e2e"], + } + ) + + +def _seed_signal(project: Path, target: str) -> None: + """Seed an on-disk signal that ``detect_signals`` recognizes.""" + if target == "copilot": + gh = project / ".github" + gh.mkdir(parents=True, exist_ok=True) + (gh / "copilot-instructions.md").write_text("# test\n", encoding="utf-8") + elif target == "cursor": + (project / ".cursor").mkdir(parents=True, exist_ok=True) + elif target == "codex": + (project / ".codex").mkdir(parents=True, exist_ok=True) + elif target == "claude": + (project / ".claude").mkdir(parents=True, exist_ok=True) + (project / "CLAUDE.md").write_text("# test\n", encoding="utf-8") + elif target == "gemini": + (project / ".gemini").mkdir(parents=True, exist_ok=True) + else: + raise ValueError(f"unsupported signal target: {target}") + + +def _copilot_mcp_path(project: Path) -> Path: + """Project-scoped copilot MCP config path.""" + return project / ".vscode" / "mcp.json" + + +def _cursor_mcp_path(project: Path) -> Path: + return project / ".cursor" / "mcp.json" + + +def _codex_mcp_path(project: Path) -> Path: + return project / ".codex" / "config.toml" + + +class TestMCPTargetsGatingE2E: + def test_targets_whitelist_copilot_suppresses_foreign_writes( + self, tmp_path, capsys, monkeypatch + ): + """``targets: [copilot]`` + on-disk cursor/codex signals -> only + copilot survives the gate; foreign-runtime config files MUST NOT + be written. This is the core regression described in #1335: + pre-fix, foreign-signal directories silently received MCP writes + despite the explicit whitelist. + + The copilot adapter writes to user-scope ``~/.copilot/`` (not the + project), so we monkeypatch ``HOME`` to a tmp path to keep the + test hermetic. The load-bearing assertions are: + + * ``.cursor/mcp.json`` is NOT written + * ``.codex/config.toml`` is NOT written + * the gate emits a ``[i] Skipped MCP config for ...`` drop line + """ + project = tmp_path / "proj-whitelist-copilot" + project.mkdir() + fake_home = tmp_path / "home" + fake_home.mkdir() + monkeypatch.setenv("HOME", str(fake_home)) + + # Seed BOTH whitelisted and foreign signals; the gate must drop + # cursor and codex even though their on-disk markers are present. + _seed_signal(project, "copilot") + _seed_signal(project, "cursor") + _seed_signal(project, "codex") + + MCPIntegrator.install( + [_make_stdio_dep("e2e-copilot-only")], + project_root=project, + apm_config={"targets": ["copilot"]}, + ) + + captured = capsys.readouterr() + assert "Skipped MCP config" in captured.out, ( + "Gate must announce the dropped runtimes via the drop line " + "so users can see why their foreign-signal directories did " + "not receive writes." + ) + # Honor the cli-log N1 lead-with-outcome contract: outcome FIRST. + assert "Skipped MCP config" in captured.out.split("\n")[0] or any( + line.lstrip().startswith("[i] Skipped MCP config") for line in captured.out.splitlines() + ) + + assert not _cursor_mcp_path(project).exists(), ( + "cursor MCP config MUST NOT be written when cursor is absent " + "from targets: -- the foreign .cursor/ signal does not grant " + "an implicit license to write." + ) + assert not _codex_mcp_path(project).exists(), ( + "codex MCP config MUST NOT be written when codex is absent from targets:." + ) + + def test_targets_whitelist_multi_allows_listed_runtimes(self, tmp_path, monkeypatch): + """``targets: [copilot, cursor]`` -> cursor MCP config IS written + (positive control); codex (declared on disk but not whitelisted) + is dropped. + """ + project = tmp_path / "proj-whitelist-multi" + project.mkdir() + fake_home = tmp_path / "home" + fake_home.mkdir() + monkeypatch.setenv("HOME", str(fake_home)) + + _seed_signal(project, "copilot") + _seed_signal(project, "cursor") + _seed_signal(project, "codex") + + MCPIntegrator.install( + [_make_stdio_dep("e2e-multi")], + project_root=project, + apm_config={"targets": ["copilot", "cursor"]}, + ) + + assert _cursor_mcp_path(project).exists(), ( + "cursor MCP config MUST be written when cursor IS in targets: " + "-- proves the gate is not over-restricting." + ) + cursor_data = json.loads(_cursor_mcp_path(project).read_text(encoding="utf-8")) + assert "e2e-multi" in cursor_data.get("mcpServers", {}) + + assert not _codex_mcp_path(project).exists(), ( + "codex must remain unwritten -- the targets: list is the " + "single source of truth, not the on-disk signal." + ) + + def test_greenfield_no_targets_no_signals_no_flag_writes_nothing( + self, tmp_path, capsys, monkeypatch + ): + """Greenfield strictness contract (PR #1336): no ``targets:`` in + apm_config, no per-runtime signals on disk, no ``--target`` flag + passed -> NO MCP writes happen anywhere AND the gate emits a + red ``[x]`` error explaining why. + + Pre-fix the gate fell back to ``[copilot]`` and silently wrote + ``.vscode/mcp.json`` even on a fully greenfield project. Post-fix + the gate delegates to ``resolve_targets``, which raises + ``NoHarnessError``; the gate fails closed and returns ``[]`` -- + matching the UX of ``apm install`` on the same input. + """ + project = tmp_path / "proj-greenfield" + project.mkdir() + fake_home = tmp_path / "home" + fake_home.mkdir() + monkeypatch.setenv("HOME", str(fake_home)) + # Intentionally NO signal markers and NO targets: in apm_config. + + MCPIntegrator.install( + [_make_stdio_dep("e2e-greenfield")], + project_root=project, + apm_config={}, + ) + + captured = capsys.readouterr() + assert "Skipping all MCP config writes" in captured.out, ( + "Greenfield project must surface the closed-gate decision " + "with a red [x] error -- silent no-op is exactly the " + "asymmetry vs `apm install` that PR #1336 closes." + ) + + assert not _cursor_mcp_path(project).exists() + assert not _codex_mcp_path(project).exists() + assert not (project / ".vscode" / "mcp.json").exists(), ( + "Greenfield project (no targets:, no signals, no flag) must " + "not receive a silent copilot-vscode MCP write -- the " + "pre-#1336 fallback is gone." + ) diff --git a/tests/unit/install/test_install_local_bundle_issue1207.py b/tests/unit/install/test_install_local_bundle_issue1207.py index 12f239163..c91114883 100644 --- a/tests/unit/install/test_install_local_bundle_issue1207.py +++ b/tests/unit/install/test_install_local_bundle_issue1207.py @@ -538,8 +538,12 @@ def fake_install(deps, **kwargs): logger=logger, ) assert count == 1 - assert captured["kwargs"]["explicit_target"] == "copilot,opencode" - assert captured["kwargs"]["apm_config"]["target"] == "copilot,opencode" + # Per scsec N1 (PR #1336): the bundle wiring forwards canonical + # plural shape -- explicit_target as list, apm_config["targets"] + # as list -- so the gate's CSV-normalization branch becomes pure + # backward-compat instead of load-bearing for this path. + assert captured["kwargs"]["explicit_target"] == ["copilot", "opencode"] + assert captured["kwargs"]["apm_config"]["targets"] == ["copilot", "opencode"] assert captured["kwargs"]["user_scope"] is False assert {d.name for d in captured["deps"]} == {"a"} diff --git a/tests/unit/integration/test_mcp_integrator.py b/tests/unit/integration/test_mcp_integrator.py index 92c9bd947..e46c084e7 100644 --- a/tests/unit/integration/test_mcp_integrator.py +++ b/tests/unit/integration/test_mcp_integrator.py @@ -684,7 +684,13 @@ def test_install_uses_explicit_project_root_for_workspace_runtime_detection( MCPIntegrator.install( mcp_deps=["test/server"], project_root=nested, - apm_config={}, + # Declare every target the multi-signal nested project + # supports. Without this, the strict resolver raises + # AmbiguousHarnessError on >=2 signals (cursor, opencode, + # copilot from .github/) and the gate fails closed -- which + # is correct UX in production but obscures what THIS test + # is asserting (workspace project_root resolution). + apm_config={"targets": ["copilot", "cursor", "opencode"]}, ) called_runtimes = {call.args[0] for call in mock_install_rt.call_args_list} @@ -742,14 +748,14 @@ def test_returns_empty_when_dir_empty(self, tmp_path): class _FakeTarget: - """Minimal stand-in for TargetProfile.""" + """Minimal stand-in for TargetProfile (legacy active_targets shape).""" def __init__(self, name: str): self.name = name def _fake_active_targets(names: list[str]): - """Return a mock for active_targets that yields *names*.""" + """Return a mock for the legacy active_targets that yields *names*.""" def _inner(_root, _explicit=None): return [_FakeTarget(n) for n in names] @@ -757,8 +763,43 @@ def _inner(_root, _explicit=None): return _inner +def _make_signal_dir(root: Path, target: str) -> None: + """Create a directory marker that detect_signals() recognises. + + Lets us exercise the gate's third-priority signals path without + mocking resolve_targets. Maps the canonical target name to the + minimal on-disk artifact detect_signals scans for; mirrors the + fixture-style setup in tests/integration/conftest.py:make_copilot_project + rather than duplicating private detection rules in test code. + """ + if target == "copilot": + gh = root / ".github" + gh.mkdir(parents=True, exist_ok=True) + (gh / "copilot-instructions.md").write_text("placeholder\n") + elif target == "claude": + (root / ".claude").mkdir(parents=True, exist_ok=True) + (root / "CLAUDE.md").write_text("placeholder\n") + elif target == "codex": + (root / ".codex").mkdir(parents=True, exist_ok=True) + elif target == "cursor": + (root / ".cursor").mkdir(parents=True, exist_ok=True) + elif target == "gemini": + (root / ".gemini").mkdir(parents=True, exist_ok=True) + else: + raise AssertionError(f"unknown target signal in test: {target}") + + class TestGateProjectScopedRuntimes: - """Tests for MCPIntegrator._gate_project_scoped_runtimes (issue #1335).""" + """Tests for MCPIntegrator._gate_project_scoped_runtimes (issue #1335). + + Behavior model: the gate delegates to + :func:`apm_cli.core.target_detection.resolve_targets`, which enforces + the strict flag > yaml > signals chain (no permissive + "fallback to copilot" greenfield default). Tests that exercise the + signals path use real on-disk markers via :func:`_make_signal_dir` + rather than mocking the resolver -- it keeps test failures honest if + detect_signals' rules change. + """ _gate = staticmethod(MCPIntegrator._gate_project_scoped_runtimes) @@ -776,9 +817,7 @@ def test_user_scope_bypasses_all_gating(self, tmp_path): # -- explicit targets: (plural) gates all runtimes --------------------- - @patch("apm_cli.integration.targets.active_targets") - def test_targets_plural_filters_unlisted_runtimes(self, mock_at, tmp_path): - mock_at.side_effect = _fake_active_targets(["claude"]) + def test_targets_plural_filters_unlisted_runtimes(self, tmp_path): result = self._gate( ["claude", "copilot", "vscode", "codex"], user_scope=False, @@ -788,9 +827,7 @@ def test_targets_plural_filters_unlisted_runtimes(self, mock_at, tmp_path): ) assert result == ["claude"] - @patch("apm_cli.integration.targets.active_targets") - def test_target_singular_filters_unlisted_runtimes(self, mock_at, tmp_path): - mock_at.side_effect = _fake_active_targets(["claude"]) + def test_target_singular_filters_unlisted_runtimes(self, tmp_path): result = self._gate( ["claude", "copilot", "vscode"], user_scope=False, @@ -800,11 +837,9 @@ def test_target_singular_filters_unlisted_runtimes(self, mock_at, tmp_path): ) assert result == ["claude"] - @patch("apm_cli.integration.targets.active_targets") - def test_targets_multiple_values_keeps_all_listed(self, mock_at, tmp_path): + def test_targets_multiple_values_keeps_all_listed(self, tmp_path): # `vscode` runtime canonicalizes to `copilot`, so when copilot is # in active targets both `copilot` and `vscode` runtime writes pass. - mock_at.side_effect = _fake_active_targets(["claude", "copilot"]) result = self._gate( ["claude", "copilot", "vscode", "codex", "cursor"], user_scope=False, @@ -816,12 +851,10 @@ def test_targets_multiple_values_keeps_all_listed(self, mock_at, tmp_path): # -- no targets field: directory-detection acts as the whitelist ------ - @patch("apm_cli.integration.targets.active_targets") - def test_no_targets_uses_directory_detection_for_all_runtimes(self, mock_at, tmp_path): - # active_targets returns copilot only -- every other runtime gates, - # not just codex/claude. Mirrors `apm install` UX. Note: `vscode` - # canonicalizes to `copilot` so both pass when copilot is active. - mock_at.side_effect = _fake_active_targets(["copilot"]) + def test_no_targets_uses_directory_detection_for_all_runtimes(self, tmp_path): + # Single signal: .github/copilot-instructions.md -> copilot only. + # Every other runtime gates, mirroring `apm install`. + _make_signal_dir(tmp_path, "copilot") result = self._gate( ["copilot", "vscode", "codex", "claude", "cursor"], user_scope=False, @@ -831,25 +864,25 @@ def test_no_targets_uses_directory_detection_for_all_runtimes(self, mock_at, tmp ) assert result == ["copilot", "vscode"] - @patch("apm_cli.integration.targets.active_targets") - def test_no_targets_keeps_all_when_all_directories_present(self, mock_at, tmp_path): - mock_at.side_effect = _fake_active_targets(["copilot", "vscode", "cursor"]) + def test_no_targets_with_ambiguous_signals_fails_closed(self, tmp_path): + # >=2 signals + no flag + no targets: -> AmbiguousHarnessError. + # Strict v2 contract: gate writes nothing, surfaces red [x]. + _make_signal_dir(tmp_path, "copilot") + _make_signal_dir(tmp_path, "claude") result = self._gate( - ["copilot", "vscode", "cursor"], + ["copilot", "claude"], user_scope=False, project_root=tmp_path, apm_config={}, explicit_target=None, ) - assert result == ["copilot", "vscode", "cursor"] + assert result == [] # -- explicit_target CLI flag overrides config ------------------------- - @patch("apm_cli.integration.targets.active_targets") - def test_explicit_target_overrides_config(self, mock_at, tmp_path): - # Real active_targets normalizes "vscode" -> "copilot"; mirror that - # in the fake so the gate's vscode->copilot alias check sees a hit. - mock_at.side_effect = _fake_active_targets(["copilot"]) + def test_explicit_target_overrides_config(self, tmp_path): + # vscode runtime -> copilot canonical; gate keeps both vscode AND + # copilot when the flag resolves to copilot. result = self._gate( ["claude", "copilot", "vscode", "codex"], user_scope=False, @@ -859,9 +892,7 @@ def test_explicit_target_overrides_config(self, mock_at, tmp_path): ) assert result == ["copilot", "vscode"] - @patch("apm_cli.integration.targets.active_targets") - def test_explicit_target_without_config(self, mock_at, tmp_path): - mock_at.side_effect = _fake_active_targets(["cursor"]) + def test_explicit_target_without_config(self, tmp_path): result = self._gate( ["claude", "copilot", "cursor", "codex"], user_scope=False, @@ -883,9 +914,8 @@ def test_empty_target_runtimes_returns_empty(self, tmp_path): ) assert result == [] - @patch("apm_cli.integration.targets.active_targets") - def test_apm_config_none_falls_through_to_auto_detect(self, mock_at, tmp_path): - mock_at.side_effect = _fake_active_targets(["copilot"]) + def test_apm_config_none_with_signal_falls_through_to_auto_detect(self, tmp_path): + _make_signal_dir(tmp_path, "copilot") result = self._gate( ["copilot", "codex"], user_scope=False, @@ -896,13 +926,35 @@ def test_apm_config_none_falls_through_to_auto_detect(self, mock_at, tmp_path): assert "copilot" in result assert "codex" not in result + # -- strict-mode greenfield: no flag/yaml/signal -> NoHarnessError ---- + + def test_no_signal_no_targets_no_flag_fails_closed(self, tmp_path, capsys): + """Greenfield (no flag, no targets:, no harness dir) writes nothing. + + Closes the asymmetry the panel surfaced: canonical `apm install` + raises NoHarnessError on this state (install/phases/targets.py), + but the MCP gate previously fell back to permissive [copilot] + via active_targets -- the same class of silent-write bug as + #1335, just gated by greenfield rather than explicit-target + mismatch. + """ + result = self._gate( + ["copilot", "claude", "codex", "cursor"], + user_scope=False, + project_root=tmp_path, + apm_config={}, + explicit_target=None, + ) + assert result == [] + out = capsys.readouterr().out + # Canonical lead-with-outcome voice + structured error body. + assert "Skipping all MCP config writes" in out + assert "could not resolve active targets" in out + # -- malformed targets field: fail-closed (issue #1335 follow-up) ------ - @patch("apm_cli.integration.targets.active_targets") - def test_conflicting_targets_field_fails_closed(self, mock_at, tmp_path): + def test_conflicting_targets_field_fails_closed(self, tmp_path): # Both `target` and `targets` set -> ConflictingTargetsError. - # Fail-closed contract: write nothing rather than widen via auto-detect. - mock_at.side_effect = _fake_active_targets(["copilot", "claude", "codex"]) result = self._gate( ["claude", "copilot", "vscode", "codex"], user_scope=False, @@ -912,10 +964,8 @@ def test_conflicting_targets_field_fails_closed(self, mock_at, tmp_path): ) assert result == [] - @patch("apm_cli.integration.targets.active_targets") - def test_empty_targets_list_fails_closed(self, mock_at, tmp_path): - # `targets: []` -> EmptyTargetsListError. Same fail-closed contract. - mock_at.side_effect = _fake_active_targets(["copilot", "claude", "codex"]) + def test_empty_targets_list_fails_closed(self, tmp_path): + # `targets: []` -> EmptyTargetsListError. result = self._gate( ["claude", "copilot", "codex"], user_scope=False, @@ -925,24 +975,33 @@ def test_empty_targets_list_fails_closed(self, mock_at, tmp_path): ) assert result == [] - @patch("apm_cli.integration.targets.active_targets") - def test_explicit_target_csv_string_normalized(self, mock_at, tmp_path): - # `_wire_bundle_mcp_servers` passes `target_csv = "claude,copilot"`. - # active_targets() does not split CSV; the gate must normalize first - # or active_targets sees one unknown token and returns []. Regression - # guard for the Copilot review on PR #1336. - seen_explicit: list = [] - - def _resolve(_root, explicit=None): - seen_explicit.append(explicit) - # Real active_targets returns profiles for each canonical name in - # the (now-list) explicit input. - tokens = ( - [explicit] if isinstance(explicit, str) else (list(explicit) if explicit else []) - ) - return [_FakeTarget(t) for t in tokens] + def test_unknown_target_in_yaml_fails_closed(self, tmp_path, capsys): + """Non-canonical token in `targets:` -> UnknownTargetError. + + scsec R2: the previous catch only handled ConflictingTargets and + EmptyTargets; UnknownTargetError leaked uncaught past the gate + on entry paths that bypass the upstream manifest validator + (mcp_integrator_install.py, _wire_bundle_mcp_servers). + """ + result = self._gate( + ["copilot", "claude"], + user_scope=False, + project_root=tmp_path, + apm_config={"targets": ["copilot", "bogus"]}, + explicit_target=None, + ) + assert result == [] + out = capsys.readouterr().out + assert "Skipping all MCP config writes" in out + assert "apm.yml 'targets' field is invalid" in out - mock_at.side_effect = _resolve + def test_explicit_target_csv_string_normalized(self, tmp_path): + """Legacy `_wire_bundle_mcp_servers` CSV input must normalize first. + + The canonical-name validator inside resolve_targets would reject + the whole CSV "claude,copilot" as one unknown token -- the gate + normalizes to a list before the resolver sees it. + """ result = self._gate( ["claude", "copilot", "codex"], user_scope=False, @@ -950,8 +1009,6 @@ def _resolve(_root, explicit=None): apm_config=None, explicit_target="claude,copilot", ) - # Gate must have normalized the CSV to a list before calling active_targets. - assert seen_explicit == [["claude", "copilot"]] assert "claude" in result assert "copilot" in result assert "codex" not in result @@ -994,15 +1051,16 @@ def test_apm_package_targets_plural_forwards_through_call_site(self, tmp_path): # The gate's parse_targets_field must now see the plural list. assert parse_targets_field(mcp_apm_config) == ["copilot", "claude"] - @patch("apm_cli.integration.targets.active_targets") - def test_dropped_runtime_message_includes_active_targets(self, mock_at, tmp_path, capsys): + def test_dropped_runtime_message_includes_active_targets(self, tmp_path, capsys): """Negative-case message must name the active set (cli-logging B3). Without "(active targets: ...)" the user has to grep apm.yml to confirm what the gate did. Mirrors the canonical provenance line - shape ``Targets: X (source: Y)``. + shape ``Targets: X (source: Y)`` -- including the double-space + separator before the parenthetical (tc N1 contract lock). """ - mock_at.side_effect = _fake_active_targets(["copilot"]) + import re + self._gate( ["copilot", "claude", "codex"], user_scope=False, @@ -1015,3 +1073,8 @@ def test_dropped_runtime_message_includes_active_targets(self, mock_at, tmp_path assert "active targets: copilot" in out # Symbol prefix asserts the gate honors the [+]/[!]/[i]/[x] contract. assert "[i]" in out + # Lock the double-space provenance separator that mirrors + # canonical `Targets: X (source: Y)`. Without this, a future + # contributor collapsing to a single space would silently break + # provenance-line parity with the install phase. + assert re.search(r"Skipped MCP config for [^(]+ \(active targets:", out) From 260a144678a592e588bc3b80873cd7df36b3280b Mon Sep 17 00:00:00 2001 From: Daniel Meppiel Date: Fri, 15 May 2026 09:48:40 +0200 Subject: [PATCH 7/7] fix(tests,docs): pass explicit_target in MCP integrator tests; fix broken docs link The 5 failing CI tests in tests/unit/test_mcp_overlays.py and tests/unit/test_mcp_integrator_characterisation.py called MCPIntegrator.install(runtime="vscode") without an explicit_target. With the new strict targets gate (#1335), the gate falls back to cwd-based harness detection when explicit_target is None. The CI worker subprocess cwd has no harness markers visible, so the gate hits NoHarnessError and skips _install_for_runtime. Production callers always pass explicit_target alongside runtime; the tests now mirror that contract. Also fix a broken absolute link in docs/src/content/docs/consumer/install-mcp-servers.md (/consumer/install-packages/#where-files-land) that did not include the docs site base prefix /apm/. Converted to relative form (../install-packages/#where-files-land) to match the convention used elsewhere in the doc tree. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/src/content/docs/consumer/install-mcp-servers.md | 2 +- tests/unit/test_mcp_integrator_characterisation.py | 2 ++ tests/unit/test_mcp_overlays.py | 8 +++++--- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/docs/src/content/docs/consumer/install-mcp-servers.md b/docs/src/content/docs/consumer/install-mcp-servers.md index d832d9741..6839109d2 100644 --- a/docs/src/content/docs/consumer/install-mcp-servers.md +++ b/docs/src/content/docs/consumer/install-mcp-servers.md @@ -90,7 +90,7 @@ writes a runtime-specific MCP config file. The schemas differ; the MCP install honors the same target resolution chain as `apm install` for any other dependency: see -[Where files land](/consumer/install-packages/#where-files-land). +[Where files land](../install-packages/#where-files-land). In short: `--target` wins, then `apm.yml`'s `targets:`, then auto-detect from harness directories. diff --git a/tests/unit/test_mcp_integrator_characterisation.py b/tests/unit/test_mcp_integrator_characterisation.py index 33806ffa6..4b9368295 100644 --- a/tests/unit/test_mcp_integrator_characterisation.py +++ b/tests/unit/test_mcp_integrator_characterisation.py @@ -110,6 +110,7 @@ def test_install_exclude_filter(self, mock_mcp_deps): mcp_deps=mock_mcp_deps, runtime="vscode", exclude="cursor", + explicit_target="vscode", logger=logger, ) assert isinstance(result, int) @@ -130,6 +131,7 @@ def test_install_specific_runtime(self, mock_mcp_deps): MCPIntegrator.install( mcp_deps=mock_mcp_deps, runtime="vscode", + explicit_target="vscode", logger=logger, ) assert mock_install.called diff --git a/tests/unit/test_mcp_overlays.py b/tests/unit/test_mcp_overlays.py index 10eaac739..263443c88 100644 --- a/tests/unit/test_mcp_overlays.py +++ b/tests/unit/test_mcp_overlays.py @@ -810,7 +810,7 @@ def test_self_defined_deps_skip_registry_validation(self, _console, mock_install command="my-cmd", ) - count = MCPIntegrator.install([dep], runtime="vscode") + count = MCPIntegrator.install([dep], runtime="vscode", explicit_target="vscode") # Self-defined deps should NOT go through registry validation # (MCPServerOperations is never instantiated for self-defined-only lists) @@ -839,7 +839,7 @@ def test_registry_deps_use_dep_names(self, mock_ops_cls, _console, mock_install_ mock_ops.collect_runtime_variables.return_value = {} dep = MCPDependency.from_string("io.github.github/github-mcp-server") - count = MCPIntegrator.install([dep], runtime="vscode") + count = MCPIntegrator.install([dep], runtime="vscode", explicit_target="vscode") mock_ops.validate_servers_exist.assert_called_once_with( ["io.github.github/github-mcp-server"] @@ -867,7 +867,9 @@ def test_mixed_deps_both_paths(self, mock_ops_cls, _console, mock_install_runtim command="my-cmd", ) - count = MCPIntegrator.install([registry_dep, self_defined_dep], runtime="vscode") + count = MCPIntegrator.install( + [registry_dep, self_defined_dep], runtime="vscode", explicit_target="vscode" + ) # Registry dep goes through validation mock_ops.validate_servers_exist.assert_called_once_with(