-
Notifications
You must be signed in to change notification settings - Fork 233
fix: respect targets: whitelist for all runtimes during MCP install #1336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
3b4c5e0
82fe862
430bd9f
3a5bc4e
727ce8d
b68e512
260a144
9265b61
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -953,25 +953,56 @@ def _gate_project_scoped_runtimes( | |
| apm_config: dict | None, | ||
| explicit_target: str | None, | ||
| ) -> list[str]: | ||
|
Comment on lines
948
to
952
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 430bd9f (annotation widened to |
||
| """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 | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 430bd9f (em/en dashes replaced with ASCII -- across the docstring and inline comments). |
||
| 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") | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stale -- this was already addressed in commit 82fe862 (one commit before your review landed). The except is now narrowed to |
||
|
|
||
| config_target = explicit_target or explicit_from_config or None | ||
| has_explicit_targets = bool(explicit_target or explicit_from_config) | ||
|
|
||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch -- legit regression. Fixed in 430bd9f by normalizing CSV strings to a list before calling |
||
| 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: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 430bd9f (em dashes replaced with ASCII -- across the file). |
||
| # =========================================================================== | ||
|
|
||
|
|
||
| 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 | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 430bd9f (right-arrow replaced with ->). |
||
| 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 | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Declined -- repo convention is to reference the issue number in CHANGELOG entries, not the PR number. Every recent entry follows this: (#1313), (#1289), (#1222), (#1299), (#1317). Keeping (#1335) for consistency.