Fix `apm install --global` skipping MCP server installation by sergio-sisternes-epam · Pull Request #638 · microsoft/apm

sergio-sisternes-epam · 2026-04-09T12:46:08Z

Description

Add scope awareness to MCP server installation so that apm install --global writes MCP server entries to global/user-scoped runtime config paths (e.g. ~/.copilot/mcp-config.json for Copilot CLI) instead of unconditionally skipping all MCP installation at user scope.

Fixes #637

Type of change

Bug fix
New feature
Documentation
Maintenance / refactor

Testing

Tested locally
All existing tests pass
Added tests for new functionality (if applicable)

Copilot

Pull request overview

Fixes global-scope MCP installation so apm install --global installs MCP servers for runtimes that have user/global config paths (instead of blanket-skipping MCP at user scope), and threads scope through install/uninstall MCP cleanup.

Changes:

Add supports_user_scope to MCP client adapters and filter MCP install/cleanup targets based on InstallScope.
Remove the user-scope MCP blanket-disable in apm install and forward scope into MCP install/remove-stale paths.
Add unit tests plus doc/changelog updates describing the new --global MCP behavior.

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tests/unit/test_global_mcp_scope.py	Adds coverage for adapter scope flags, MCPIntegrator filtering, uninstall cleanup behavior, and install command integration.
src/apm_cli/integration/mcp_integrator.py	Adds `scope` to `install()` / `remove_stale()` and filters runtimes at user scope using adapter capability.
src/apm_cli/commands/install.py	Removes blanket user-scope MCP skip; forwards `scope` to MCPIntegrator install/remove_stale.
src/apm_cli/commands/uninstall/engine.py	Threads `scope` through stale MCP cleanup to avoid touching workspace configs in user scope.
src/apm_cli/commands/uninstall/cli.py	Passes uninstall scope into `_cleanup_stale_mcp(...)`.
src/apm_cli/adapters/client/base.py	Introduces `supports_user_scope: bool = False` default on MCP adapters.
src/apm_cli/adapters/client/copilot.py	Marks Copilot adapter as user-scope-capable (`supports_user_scope = True`).
src/apm_cli/adapters/client/codex.py	Marks Codex adapter as user-scope-capable (`supports_user_scope = True`).
src/apm_cli/adapters/client/cursor.py	Explicitly marks Cursor as workspace-only (`supports_user_scope = False`).
src/apm_cli/adapters/client/opencode.py	Explicitly marks OpenCode as workspace-only (`supports_user_scope = False`).
docs/src/content/docs/guides/dependencies.md	Updates docs to reflect partial user-scope MCP support (global-capable runtimes only).
CHANGELOG.md	Adds unreleased “Fixed” entries for global-scope MCP install and `--trust-transitive-mcp` behavior.

- Wrap ClientFactory.create_client() in try/except ValueError in both install() and remove_stale() scope filtering so unsupported runtimes are silently skipped instead of crashing - Replace brittle source-text assertion in TestInstallCommandMCPScope with a functional test that calls MCPIntegrator.install() with mocked dependencies and verifies scope filtering end-to-end - Consolidate CHANGELOG into single entry referencing PR microsoft#638 Co-authored-by: Copilot <[email protected]>

Copilot

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.

Co-authored-by: Copilot <[email protected]>

Remove the blanket should_install_mcp=False gate for InstallScope.USER in install.py. Instead, add scope awareness to MCPClientAdapter via a supports_user_scope class attribute that each adapter declares: - CopilotClientAdapter: True (~/.copilot/mcp-config.json) - CodexClientAdapter: True (~/.codex/config.toml) - VSCodeClientAdapter: False (.vscode/mcp.json) - CursorClientAdapter: False (.cursor/mcp.json) - OpenCodeClientAdapter: False (opencode.json) MCPIntegrator.install() and remove_stale() now accept a scope parameter and filter target_runtimes to only global-capable adapters at USER scope. The uninstall path (_cleanup_stale_mcp) also receives scope so global uninstall only cleans global configs. This also unblocks --trust-transitive-mcp at global scope, which was silently ignored when the blanket MCP gate was active. Fixes microsoft#637 Co-authored-by: Copilot <[email protected]>

- Wrap ClientFactory.create_client() in try/except ValueError in both install() and remove_stale() scope filtering so unsupported runtimes are silently skipped instead of crashing - Replace brittle source-text assertion in TestInstallCommandMCPScope with a functional test that calls MCPIntegrator.install() with mocked dependencies and verifies scope filtering end-to-end - Consolidate CHANGELOG into single entry referencing PR microsoft#638 Co-authored-by: Copilot <[email protected]>

danielmeppiel · 2026-04-20T19:26:54Z

APM Review Panel — PR #638

[>] Verdict: APPROVE-WITH-INLINE-FIXES. Land after a small polish pass; one substantive follow-up issue must be filed before the release that ships this PR.

@sergio-sisternes-epam — thank you, this is exactly the kind of silent-footgun fix that earns trust. Four specialists (python-architect, cli-logging-expert, devx-ux-expert, supply-chain-security) converged with no hard blockers; concerns are tight and absorbable.

What's excellent

Right structural fix. supports_user_scope capability flag beats abstract-method or registry alternatives. Inheritance lock-down test (test_vscode_does_not_support_user_scope) hardens the Cursor/OpenCode IS-A trap into a contract.
End-to-end correctness on --trust-transitive-mcp -g. Removing the blanket should_install_mcp guard restores transitive collection — structural, not surface-level.
Backward compat preserved (scope=None paths untouched). 339 lines of new tests. CHANGELOG entries parse clean.
Security gates intact. _pre_deploy_security_scan and the depth-1 transitive-trust gate are scope-agnostic — --global inherits the same trust model with no new bypass.

Findings

[x] BLOCKER (same PR) — Skip-message visibility. if logger: logger.verbose_detail(...) else: _rich_info(...) produces inconsistent visibility for the same event (cli-logging + devx agree). Promote to default visibility, add symbol="info", and reword to be actionable:
```
[i] Skipped workspace-only runtimes at user scope: vscode, cursor — omit --global to install these
```
Discoverability is the whole point of this fix.
[x] BLOCKER (same PR) — Docs + help text drift. Per cli.instructions.md ("If a CLI change is not reflected in cli-commands.md in the same PR, that change is incomplete by definition"), this PR needs a one-line note in cli-commands.md under apm install --global and a one-liner in the Click help= string for -g/--global. Non-negotiable on a behavior-changing flag.
[x] BLOCKER (same PR) — Status symbols missing. _rich_warning(...) / _rich_info(...) calls at mcp_integrator.py:261-281 of the diff are missing symbol= and render naked. Mechanical fix: symbol="warning" / symbol="info".
[!] HIGH (follow-up issue, must land before release shipping this PR) — Lockfile-path-at---global bug. MCPIntegrator.update_lockfile defaults lock_path to Path.cwd(); install-side callers at install.py:686/691/697 don't pass an explicit path. Pre-existing, but this PR exposes it — apm install --global pkg-with-mcp from a project directory will now write the MCP audit entry into the project lockfile instead of ~/.apm/apm.lock.yaml. Not a same-PR fix (plumbing + E2E test), but file the issue, reference it in the CHANGELOG line, and land it in the same release. Mirror the remove_stale(scope=) plumbing pattern.
[i] FOLLOW-UP — VS Code user-profile gap. Issue [BUG] apm install --global skips MCP server installation for all runtimes #637 named VS Code user-profile mcp.json as in-scope, but the PR explicitly marks VS Code as workspace-only. Acceptable scope cut. Open tracking issue and reword dependencies.md from "are skipped" to "tracked in #NNN" — turns a positive-assertion test into a transitional one.
[i] FOLLOW-UP — Hardcoded (supported: copilot, codex). Derive from ClientFactory introspection when a 3rd global-capable adapter lands. Not now.
[i] LOW — Test gap (architect + security). No assertion that _cleanup_stale_mcp forwards scope; no assertion that --trust-transitive-mcp -g triggers depth-2 trust warning. Add at author discretion.

Merge call

Land #1, #2, #3 in this PR (mechanical, ~30 min). Open issues for #4, #5, #6 before merge; reference #4 in the CHANGELOG line so users hitting the lockfile path know it's tracked. Then merge — don't re-queue specialists. Sergio has throughput; we honor that by being decisive on what's same-PR vs follow-up.

Suggested CHANGELOG augment:

apm install --global now installs MCP servers to global-capable runtimes (Copilot CLI, Codex CLI) instead of blanket-skipping all MCP installation at user scope (#638) — by @sergio-sisternes-epam. Note: lockfile-path behavior at --global tracked in #NNN.

[+] Ship it.

Growth angle

apm install -g finally earns the -g. With Copilot CLI and Codex CLI honoring global MCP at --global, there's a clean one-liner for the next release post and README beat: "apm install -g works the way npm -g taught you to expect — for AI agent infrastructure." Mental-model hook that travels on social and gives the CLI-runtime crowd a reason to repost.
Turn the VS Code gap into a contributor funnel. Frame the follow-up as "good first issue for the next layer of runtime coverage — VS Code user-profile MCP path wanted". Recent contributor momentum (Sergio, plus fix(auth): thread dep_ref.port into credential resolution (#785) #788/fix(install): warn when --allow-protocol-fallback reuses a custom port across schemes (#786) #789 authors) suggests the funnel is warm enough for this to land.

_{Reviewed via the apm-review-panel skill: python-architect, cli-logging-expert, devx-ux-expert, supply-chain-security; synthesized by apm-ceo with oss-growth-hacker side-channel.}

danielmeppiel

Blockers per last comment

- Wrap ClientFactory.create_client() in try/except ValueError in both install() and remove_stale() scope filtering so unsupported runtimes are silently skipped instead of crashing - Replace brittle source-text assertion in TestInstallCommandMCPScope with a functional test that calls MCPIntegrator.install() with mocked dependencies and verifies scope filtering end-to-end - Consolidate CHANGELOG into single entry referencing PR microsoft#638 Co-authored-by: Copilot <[email protected]>

The 0.9.0 section had 30 bullets where 9 PRs were over-split: #810 (5 bullets), #514 (5), #700 (2), #778 (3), #638 (2), #808 (paragraph listing every drift fix), plus one orphan ### Changed block holding a single sub-point of the #778 BREAKING entry. Per .github/instructions/changelog.instructions.md: 'One line per PR: concise description ending with (#PR_NUMBER). Combine related PRs into a single line when they form one logical change.' Result: 23 entries, one per PR, terse imperative summaries with the user-facing impact up front. Migration / 'Closes' / 'Previously...' prose moved off the changelog (it belongs in the PR body and CHANGELOG-linked docs). Section is now scannable as headlines instead of essays. No code changes; v0.9.0 tag already published on prior commit. Co-authored-by: Copilot <[email protected]>

sergio-sisternes-epam marked this pull request as ready for review April 9, 2026 13:58

sergio-sisternes-epam requested a review from danielmeppiel as a code owner April 9, 2026 13:58

Copilot AI review requested due to automatic review settings April 9, 2026 13:58

Copilot started reviewing on behalf of sergio-sisternes-epam April 9, 2026 13:59 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

Comment thread src/apm_cli/integration/mcp_integrator.py Outdated

Comment thread src/apm_cli/integration/mcp_integrator.py Outdated

Comment thread tests/unit/test_global_mcp_scope.py Outdated

Comment thread CHANGELOG.md Outdated

sergio-sisternes-epam requested a review from Copilot April 9, 2026 14:36

Copilot started reviewing on behalf of sergio-sisternes-epam April 9, 2026 14:37 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

Comment thread tests/unit/test_global_mcp_scope.py

Comment thread tests/unit/test_global_mcp_scope.py

Comment thread src/apm_cli/integration/mcp_integrator.py Outdated

Comment thread docs/src/content/docs/guides/dependencies.md

danielmeppiel added CI/CD Deprecated: use area/ci-cd. Kept for issue history; will be removed in milestone 0.10.0. and removed CI/CD Deprecated: use area/ci-cd. Kept for issue history; will be removed in milestone 0.10.0. labels Apr 19, 2026

Sergio Sisternes and others added 2 commits April 20, 2026 17:07

fix: scaffold for global MCP install support (microsoft#637)

bb065bd

Co-authored-by: Copilot <[email protected]>

sergio-sisternes-epam force-pushed the fix/637-global-mcp-install branch from 2b9390a to 52627c8 Compare April 20, 2026 16:11

sergio-sisternes-epam enabled auto-merge April 20, 2026 16:12

danielmeppiel requested changes Apr 20, 2026

View reviewed changes

danielmeppiel mentioned this pull request Apr 20, 2026

fix: VS Code adapter fails to configure HTTP/SSE remote MCP servers (#654) #656

Merged

7 tasks

This was referenced Apr 20, 2026

Bug: MCPIntegrator.update_lockfile uses Path.cwd() at --global scope, writing MCP audit entries to the wrong lockfile #794

Open

Feature: Support VS Code user-profile mcp.json for --global scope #793

Open

sergio-sisternes-epam force-pushed the fix/637-global-mcp-install branch from 52627c8 to ab95af3 Compare April 20, 2026 21:47

danielmeppiel self-requested a review April 20, 2026 23:31

danielmeppiel approved these changes Apr 20, 2026

View reviewed changes

sergio-sisternes-epam added this pull request to the merge queue Apr 20, 2026

Merged via the queue into microsoft:main with commit 118ad50 Apr 20, 2026
11 checks passed

sergio-sisternes-epam deleted the fix/637-global-mcp-install branch April 20, 2026 23:37

danielmeppiel mentioned this pull request Apr 21, 2026

[docs] Update MCP client documentation for global scope and missing runtimes #796

Closed

danielmeppiel mentioned this pull request Apr 25, 2026

docs(changelog): resolve 0.9.2 merge conflict, dedupe sections, add missing post-0.9.2 entries #932

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `apm install --global` skipping MCP server installation#638

Fix `apm install --global` skipping MCP server installation#638
sergio-sisternes-epam merged 3 commits intomicrosoft:mainfrom
sergio-sisternes-epam:fix/637-global-mcp-install

sergio-sisternes-epam commented Apr 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danielmeppiel commented Apr 20, 2026 •

edited

Loading

Uh oh!

danielmeppiel left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

sergio-sisternes-epam commented Apr 9, 2026

Description

Type of change

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danielmeppiel commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

APM Review Panel — PR #638

What's excellent

Findings

Merge call

Growth angle

Uh oh!

danielmeppiel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danielmeppiel commented Apr 20, 2026 •

edited

Loading