refactor: extract MCPIntegrator from cli.py#215
Merged
danielmeppiel merged 2 commits intomainfrom Mar 9, 2026
Merged
Conversation
Move 11 MCP lifecycle functions (~760 lines) from cli.py into a standalone MCPIntegrator class in integration/mcp_integrator.py. - collect_transitive, deduplicate, install, remove_stale, update_lockfile - _build_self_defined_info, _apply_overlay, get_server_names - _detect_runtimes, _filter_runtimes, _install_for_runtime - All methods are @staticmethod (namespace, not stateful object) - Hardened bare except blocks with logger.debug(exc_info=True) - Added cycle guard to _collect_descendants in cli.py - Updated 5 test files to use new import paths and patch targets cli.py: 5267 → 4508 lines (-759 net)
Contributor
There was a problem hiding this comment.
Pull request overview
Extracts MCP lifecycle orchestration out of the monolithic cli.py into a dedicated MCPIntegrator module under src/apm_cli/integration/, keeping CLI call sites and tests aligned with the new location.
Changes:
- Added
src/apm_cli/integration/mcp_integrator.pycontaining MCP dependency resolution, install orchestration, stale cleanup, lockfile updates, and runtime detection helpers. - Updated
cli.pyto callMCPIntegrator.*and added a cycle guard to dependency tree traversal. - Updated unit/integration tests to import/patch the new module paths.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/integration/mcp_integrator.py |
New standalone orchestrator for MCP lifecycle logic previously embedded in cli.py. |
src/apm_cli/cli.py |
Switched MCP call sites to MCPIntegrator and added a cycle guard to _collect_descendants. |
src/apm_cli/integration/__init__.py |
Exports MCPIntegrator from the integration package. |
tests/unit/test_transitive_mcp.py |
Updated imports and patch targets to the new MCP integrator module. |
tests/unit/test_runtime_detection.py |
Updated runtime detection tests to call MCPIntegrator helpers. |
tests/unit/test_mcp_overlays.py |
Updated overlay/self-defined server tests to the new integrator paths. |
tests/unit/test_mcp_lifecycle_e2e.py |
Updated end-to-end MCP lifecycle tests to use MCPIntegrator. |
tests/integration/test_selective_install_mcp.py |
Updated selective install integration tests to patch MCPIntegrator.install. |
Comments suppressed due to low confidence (1)
src/apm_cli/integration/mcp_integrator.py:896
- Same as above:
configured_countis incremented unconditionally for self-defined deps, even if runtime configuration fails. Consider aggregating per-runtime results and only counting the server as configured when at least one runtime update succeeds (or adjusting the docstring/return semantics).
if console:
console.print(
f"│ [green]✓[/green] {dep.name} → "
f"{', '.join([rt.title() for rt in target_runtimes])}"
)
configured_count += 1
- Early exit when all runtimes excluded via --exclude - Add encoding='utf-8' to apm.yml file open - Gate configured_count on _install_for_runtime success (returns bool) - Log MCP cleanup errors instead of silently swallowing
This was referenced Mar 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #209 — Extract MCPIntegrator: move MCP lifecycle logic out of cli.py
Moves 11 MCP bare functions (~760 lines) from
cli.pyinto a standaloneMCPIntegratorclass insrc/apm_cli/integration/mcp_integrator.py.What changed
New file:
src/apm_cli/integration/mcp_integrator.pyA static-method namespace class containing all MCP lifecycle orchestration:
_collect_transitive_mcp_depscollect_transitive_deduplicate_mcp_depsdeduplicate_build_self_defined_server_info_build_self_defined_info_apply_mcp_overlay_apply_overlay_get_mcp_dep_namesget_server_names_remove_stale_mcp_serversremove_stale_update_lockfile_mcp_serversupdate_lockfile_install_mcp_dependenciesinstall_detect_runtimes_from_scripts_detect_runtimes_filter_available_runtimes_filter_runtimes_install_for_runtime_install_for_runtimeDesign decisions
@staticmethod— the class is a logical namespace, not a stateful object. No instantiation needed.except: passblocks withlogger.debug(..., exc_info=True)for debuggability.visitedset to_collect_descendantsin cli.py to prevent infinite loops in circular dependency graphs.cli.py impact
5267 → 4508 lines (−759 net). Call sites in
install()anduninstall()updated to useMCPIntegrator.*.Test updates
5 test files updated to point imports and
@patchtargets to the new module:tests/unit/test_mcp_overlays.pytests/unit/test_mcp_lifecycle_e2e.pytests/unit/test_transitive_mcp.pytests/unit/test_runtime_detection.pytests/integration/test_selective_install_mcp.pyVerification
All 1729 tests pass (0 failures, 92 skipped).