fix: skip already-configured self-defined MCP servers on install#191
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes repeated reinstall/reconfigure of self-defined (registry: false) MCP servers during apm install by adding a name-based “already configured” check (aligning behavior with the existing registry/UUID-based skip logic).
Changes:
- Added
_check_self_defined_servers_needing_installation()to detect self-defined servers missing from at least one target runtime config. - Updated
_install_mcp_dependencies()to skip already-configured self-defined servers and print✓ (already configured)instead of reinstalling. - Added unit tests covering the new helper and the skip behavior in the install flow.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/apm_cli/cli.py |
Adds helper for self-defined server presence checks and integrates skip/printing logic into MCP installation. |
tests/unit/test_transitive_mcp.py |
Adds tests for the new helper and for skipping already-configured self-defined servers during install. |
You can also share your feedback on Copilot code review. Take the survey.
|
What if the configuration changes for a self defined MCP server? The server could be found as already installed, but we would still need to update its settings like env vars etc. Is this scenario covered? @sergio-sisternes-epam |
|
During my tests I've been using --force when I wanted to override any existing config. This can be targeted to a specific package to avoid overwriting everything |
|
I have the feeling the manifest should be the source of truth to improve UX. I'm not sure imposing a |
|
Good point @danielmeppiel! I agree the manifest should be the source of truth long-term, but today the behaviour is inconsistent in the opposite direction: MCP servers are silently overwritten on every A smarter "diff-aware" install that detects manifest changes and only re-applies what actually changed would be a great improvement. But it should apply across all Primitives. I'd suggest we track that as a separate enhancement and keep this PR focused on creating a consistent behaviour, if that is the route we want it to take. |
danielmeppiel
left a comment
There was a problem hiding this comment.
Approving — the consistency argument is compelling, and this matches how registry servers already work. The --force flag doesn't bypass the registry-server skip either, so this PR is truly aligned with existing behavior.
Code quality is solid: the helper is clean, error handling is conservative (installs if config read fails), output formatting matches the existing pattern, and tests cover the key scenarios well.
I've opened a follow-up issue for diff-aware install (detect manifest changes → re-apply without --force) covering all primitive types.
danielmeppiel
left a comment
There was a problem hiding this comment.
Thanks for the contribution, Sergio! The fix for #189 itself looks good — the self-defined MCP server skip logic is the right approach.
However, this PR needs some cleanup before we can merge it:
1. Rebase onto main
The branch currently carries 71 commits, but only 2-3 are yours — the rest are commits from main that were pulled in via git merge. This makes the diff show 50 changed files / 5,048 additions when the actual fix is much smaller.
Please rebase your branch onto main to clean this up:
git fetch upstream
git rebase upstream/main
git push --force-with-leaseThis will reduce the PR to just your actual commits and make the diff reviewable.
2. Scope
Once the rebase is done, please ensure the PR only contains changes related to the #189 fix (skipping already-configured self-defined MCP servers). The current diff includes unrelated changes that were already merged to main separately (emoji removal from CLI help, plugin hardening, stale MCP cleanup, changelog updates, etc.) — those should all disappear after the rebase.
3. Test fixtures
The test fixtures (mock-marketplace-plugin, mock-claude-plugin, mock-plugin) are near-duplicates of each other. If your fix needs test fixtures, consider parametrizing a single fixture instead of duplicating directories. But first — let's see what the diff actually looks like after the rebase, since most of those fixtures may belong to the plugin feature that's already in main.
Looking forward to the cleaned-up version!
63faaf3 to
917463b
Compare
…rosoft#189) Adapt the fix to the extracted MCPIntegrator on upstream/main and retain the review follow-up behavior: cache runtime config reads for self-defined checks and emit non-console skip feedback.
917463b to
1c5d4c9
Compare
|
@danielmeppie I’ve cleaned the branch up and rebased it again onto the latest The PR is now reduced to the
Current diff is limited to:
I also reran the focused MCP unit tests locally:
On the fixture point: after cleaning up the branch, this PR no longer includes any plugin fixture changes, so I’ve kept that out of scope for |
Description
Self-defined (
registry: false) MCP servers were reinstalled/reconfigured on everyapm installeven when already present in the target runtime's configuration. This adds a name-based "already configured" check to the self-defined server install path, mirroring the existing ID-based check used for registry servers.What changed:
_check_self_defined_servers_needing_installation()helper that checks each target runtime's config for existing server entries by name_install_mcp_dependencies()to skip servers that are already configured, printing✓ (already configured)instead of reinstallingFixes #189
Type of change
Testing
Local integration test output: