fix: respect package targets during user-scope (global) MCP install#1941
Conversation
_gate_project_scoped_runtimes() previously short-circuited on user_scope=True, returning all detected runtimes without filtering against the package's declared targets field. A package declaring 'targets: copilot' would create ~/.kiro/ (and other client folders) when installed globally. Now at user scope the gate parses the package's declared targets and any explicit --target flag, filtering runtimes accordingly. When neither the package nor the CLI restricts targets, all detected runtimes still pass through (backward-compatible). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes MCP runtime target-gating for apm install --global by removing the previous user_scope=True bypass so that user-scope installs still respect apm.yml targets: and any explicit --target flag, preventing unintended runtime config writes (e.g., creating ~/.kiro/ when a package targets only Copilot).
Changes:
- Update
MCPIntegrator._gate_project_scoped_runtimes()to apply target filtering at user scope (while skipping directory-signal detection there). - Add/adjust unit tests to cover user-scope gating scenarios (declared targets, explicit target, passthrough cases, malformed targets fail-closed).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/apm_cli/integration/mcp_integrator.py | Removes the user-scope bypass and implements user-scope target filtering consistent with declared targets: and --target. |
| tests/unit/integration/test_mcp_integrator.py | Replaces the old “user-scope bypass” test with a suite of user-scope filtering tests. |
Address review feedback: - Treat 'all' as a passthrough in the user-scope fast path, mirroring resolve_targets behavior. - Update install-mcp-servers.md to reflect that user-scope installs now respect the package's targets field and --target flag. - Add regression tests for explicit_target='all' at user scope. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
apm-spec-waiver: bug fix aligning user-scope MCP gating with existing targets contract -- no new observable behaviour Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ssage Fold two in-scope panel follow-ups: - doc-writer + oss-growth-hacker: missing CHANGELOG [Unreleased] Fixed entry for #1941 (user-scope MCP install target filtering). Adds contributor credit for @sergio-sisternes-epam consistent with the pattern in [Unreleased]. - cli-logging-expert: user-scope _rich_info message now includes '(scope: global)' to distinguish it from the project-scope gate message in verbose output, and removes the pre-existing double space before the parenthetical. Addresses panel follow-ups FU-1 (CHANGELOG) and FU-5 (scope label). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 2 | Filter + log logic repeated in user/project scope blocks; method name _gate_project_scoped_runtimes is now misleading. Both pre-existing; defer rename + extract. |
| CLI Logging Expert | 0 | 0 | 1 | User-scope info message now has scope label (folded); no test on log output (minor). |
| DevX UX Expert | 0 | 0 | 1 | Double space in info message fixed (folded); docs prose is dense but functional. |
| Supply-Chain Security Expert | 0 | 0 | 0 | Fix tightens a permissive bypass; no security concerns. |
| OSS Growth Hacker | 0 | 0 | 1 | CHANGELOG entry added (folded); contributor credit present. |
| Doc Writer | 0 | 0 | 1 | CHANGELOG entry added (folded); "when neither restricts targets" phrasing in docs is slightly ambiguous but clear in context. |
| Test Coverage Expert | 0 | 0 | 1 | 3 of 8 tests falsify old mutation; integration test for --global end-to-end is a nice-to-have (defer). |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 3 follow-ups (deferred)
- [Test Coverage Expert] Add integration test exercising
apm install -g --mcpwith a target-restricted package -- would catch future regressions at the CLI surface level rather than unit only. - [Python Architect] Rename
_gate_project_scoped_runtimes->_gate_runtimes_by_targetsnow that it handles both scopes -- avoids misleading new contributors. - [Python Architect] Extract shared
_filter_runtimes_by_active(runtimes, active_set)helper to eliminate the near-duplicate filter+log blocks in user-scope and project-scope paths.
Recommendation
Ship this fix. The core correctness change is small, well-tested, and addresses a real user-visible bug (spurious ~/.kiro/ and similar folders created on --global installs targeting only a subset of runtimes). All CI checks pass on head sha 0d5ffbce39d4db1aa1418a530ab16535b414e1b2. The three deferred items are post-merge quality-of-life improvements; none affect correctness of this fix.
Full per-persona findings
Python Architect
- [nit] Scope blocks near-duplicate: the user-scope and project-scope paths each contain a filter-then-log pattern. Extract a shared
_filter_runtimes_by_activehelper to reduce drift. - [nit]
_gate_project_scoped_runtimesnow gates both scopes; the name is misleading. Rename to_gate_runtimes_by_targetsin a follow-up (touches callers -- defer).
CLI Logging Expert
- [nit] User-scope
_rich_infomessage now carries(scope: global)label (folded in this pass). No test asserts the log line content, but that is a minor gap for a debug-level message.
DevX UX Expert
- [nit] Double space before
(active targets:in user-scope message fixed (folded). Codex docs row for target filtering is long; acceptable as-is.
Supply-Chain Security Expert
No findings. Fix removes a permissive early-return bypass; the change is strictly tightening.
OSS Growth Hacker
- [nit] CHANGELOG
[Unreleased] Fixedentry added with contributor credit (folded).
Auth Expert -- inactive
No auth surfaces touched.
Doc Writer
- [nit] CHANGELOG entry added (folded). Docs prose "When neither restricts targets" is slightly ambiguous but not wrong.
Test Coverage Expert
- [nit] 8 new unit tests present; 3 directly falsify the removed bypass. Integration test for
--globalat the CLI level is deferred.
Performance Expert -- inactive
No hot path or algorithmic complexity changes.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Description
When a package declares
targets: copilotand is installed globally (apm install --global),_gate_project_scoped_runtimes()previously short-circuited onuser_scope=True, returningall detected runtimes without filtering. This caused folders like
~/.kiro/to be createdeven though the package only targets Copilot.
The fix removes the blanket bypass and applies target filtering at user scope too: the
package's declared
targets:field and any explicit--targetflag are respected. Whenneither restricts targets, all detected runtimes still pass through (backward-compatible).
apm-spec-waiver: bug fix aligning user-scope MCP gating with existing targets contract -- no new observable behaviour
Type of change
Testing
8 new test cases cover user-scope filtering: declared targets, explicit target, no-targets
passthrough, config override, None config, malformed targets, and
--target allpassthrough(string and list forms). All 129 MCP integrator tests pass.
Spec conformance (OpenAPM v0.1)