feat: support transitive MCP dependency propagation by sergio-sisternes-epam · Pull Request #123 · microsoft/apm

sergio-sisternes-epam · 2026-02-27T18:26:17Z

🚀 New Feature

Description

Adds support for transitive MCP dependency propagation across APM packages. Previously, MCP dependencies defined in transitive (non-root) packages were silently dropped during apm install. This PR ensures that MCP server configurations declared anywhere in the dependency tree are collected, deduplicated, and written to both VS Code (.vscode/mcp.json) and Copilot (~/.copilot/mcp-config.json) integration files.

This also fixes MCP dependency parsing to correctly handle both registry string format (e.g., ghcr.io/github/github-mcp-server) and inline dict format (e.g., {name: my-server, type: http, url: https://...}), which were previously dropped by a isinstance(dep, str) filter.

Changes Made

src/apm_cli/models/apm_package.py — Fixed get_mcp_dependencies() to preserve both string and dict MCP entries using isinstance(dep, (str, dict))
src/apm_cli/cli.py — Added transitive MCP collection pipeline:
- _collect_transitive_mcp_deps() — Scans apm_modules/ for MCP deps in installed packages (scoped to apm.lock entries)
- _deduplicate_mcp_deps() — Deduplicates by name (uses builtins.set() to avoid Click command collision)
- _install_inline_mcp_deps() — Orchestrates writing inline MCP deps to integration files
- _write_inline_mcp_vscode() — Writes to .vscode/mcp.json (servers key)
- _write_inline_mcp_copilot() — Writes to ~/.copilot/mcp-config.json (mcpServers key)
- Restructured _install_mcp_dependencies() to separate registry deps from inline deps
tests/unit/test_transitive_mcp.py — 25 new unit tests across 6 test classes covering parsing, collection, deduplication, and file writing
docs/dependencies.md — Documented inline dict MCP dependency format
docs/integrations.md — Documented transitive MCP collection behavior

Testing

Manual E2E testing completed (multi-level dependency tree with transitive MCP servers correctly propagated)
All existing tests pass: 1212 passed, 32 skipped, 0 failures
25 new unit tests added and passing

Checklist

LABEL: Apply enhancement label to this PR
Code follows project style guidelines
Documentation updated
CHANGELOG.md updated (for significant features)

Fixes #121

- Fix MCP dependency parsing to preserve both string and dict entries - Add transitive MCP collection from apm_modules/ dependency tree - Add deduplication, inline writing to .vscode/mcp.json and ~/.copilot/mcp-config.json - Add 25 unit tests covering parsing, collection, dedup, and file writing - Update docs/dependencies.md and docs/integrations.md

Copilot

Pull request overview

Adds support for propagating MCP dependencies declared in transitive APM packages during apm install, and extends MCP dependency parsing to preserve both registry-string and inline-dict formats so they can be deduplicated and installed into VS Code / Copilot configs.

Changes:

Preserve inline dict MCP entries in APMPackage.from_apm_yml() / get_mcp_dependencies().
Collect + deduplicate transitive MCP deps from installed packages and install inline MCP configs into .vscode/mcp.json and ~/.copilot/mcp-config.json.
Add unit tests and update docs to document inline MCP dict format + transitive behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
`src/apm_cli/models/apm_package.py`	Keeps both string and dict MCP deps when parsing `apm.yml`; updates MCP accessor return type accordingly.
`src/apm_cli/cli.py`	Implements transitive MCP collection, deduplication, and inline MCP writers for VS Code and Copilot; restructures MCP install flow.
`tests/unit/test_transitive_mcp.py`	Adds unit coverage for parsing, collection, deduplication, and file writing for transitive/inline MCP deps.
`docs/dependencies.md`	Documents MCP dependency formats (registry vs inline dict), required fields, optional headers, and transitive collection.
`docs/integrations.md`	Adds examples and notes about inline MCP config and transitive MCP propagation behavior.

src/apm_cli/cli.py

tests/unit/test_transitive_mcp.py

- Scope _collect_transitive_mcp_deps to apm.lock entries instead of full rglob - Gate transitive MCP on should_install_mcp + apm_modules existence - Add _KNOWN_MCP_TYPES validation with warning for unknown types - Emit warnings on JSON parse failures in vscode/copilot config writers - Fix success counting: only count when at least one runtime write succeeds - Remove unused imports in cli.py and test file - Update test_continues_on_write_failure to assert count==0

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

src/apm_cli/cli.py

The correct MCP transport types are 'sse' and 'http'. Updated docs and test fixtures that incorrectly used 'streamable-http' to align with _KNOWN_MCP_TYPES validation.

…MCP collection _collect_transitive_mcp_deps() included the lock-file 'host' field (e.g. github.com) in the apm_modules path, but packages are stored without the host segment. All locked paths resolved to non-existent directories, so zero transitive MCP deps were collected and .vscode/mcp.json was never generated. - Remove host from path construction to match get_install_path() behavior - Add tests for lock-file-scoped collection (regular and virtual_path)

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

src/apm_cli/cli.py

danielmeppiel

Thanks for this contribution, @sergio-sisternes-epam! The bug is real and well-researched. However, this PR bundles two distinct concerns that should be separated:

Bug fix (accept with changes): Transitive MCP propagation for registry string deps — this directly addresses #121 and the core logic is solid.
Feature (defer): Inline dict MCP format support — this is a schema design decision that needs deliberate, separate discussion given the MCP Registry spec evolution (#122 )

Summary of Required Changes

Blocking issues that must be fixed before this can merge:

_collect_transitive_mcp_deps bypasses the LockFile class — raw yaml.safe_load() instead of LockFile.from_yaml() creates maintenance coupling risk
get_mcp_dependencies() crashes on mcp: null — for dep in None raises TypeError
Codex runtime bug — writes inline configs to ~/.copilot/mcp-config.json (JSON) but Codex uses ~/.codex/config.toml (TOML) with key mcp_servers and doesn't support remote servers
Inline writers duplicate adapter logic — _write_inline_mcp_vscode() and _write_inline_mcp_copilot() reimplement path resolution, JSON I/O, and key structure already centralized in VSCodeClientAdapter.update_config() and CopilotClientAdapter.update_config()
No URL scheme validation — inline deps from transitive packages can inject arbitrary URLs (file://, data:, etc.) into MCP configs

Recommended Path

This PR (scoped down): Transitive collection + dedup for registry string deps only, using the LockFile class, with the mcp: null guard fix. The from_apm_yml() fix accepting dicts is also safe to include.
Separate discussion: Inline dict format.

The transitive collection + dedup logic for strings is clean, well-tested, and directly fixes the reported bug. I'd be happy to approve once scoped down.

src/apm_cli/cli.py

src/apm_cli/models/apm_package.py

src/apm_cli/cli.py

tests/unit/test_transitive_mcp.py

src/apm_cli/cli.py

- Use LockFile.read() instead of raw yaml.safe_load() in _collect_transitive_mcp_deps (#1) - Guard against mcp:null in get_mcp_dependencies() (#2) - Remove inline MCP installation pipeline, defer to follow-up PR (microsoft#3/microsoft#7) - Remove redundant import builtins in _deduplicate_mcp_deps (microsoft#10) - Add tests for mcp:null, mcp:[], root-over-transitive dedup order (microsoft#9) - Remove tests for deleted inline pipeline functions

…microsoft#5/microsoft#6) - Add _validate_inline_url() with https/http scheme allowlist - Add _install_inline_mcp_deps() delegating to ClientFactory adapters - VSCode: read-merge-write via adapter (full-overwrite API) - Copilot/Codex: pass merge dict via adapter update_config() - 15 new tests covering adapter delegation, URL validation, error cases - All 866 tests pass

…osoft#8) - Drop unittest.TestCase from all 6 test classes - Replace self.assert* with bare assert statements - Replace tempfile.TemporaryDirectory with tmp_path fixture - Remove unused imports (tempfile, unittest)

danielmeppiel

@sergio-sisternes-epam — All 5 technical issues from my previous review are fixed. Nice work.

However, the inline dict MCP format (name/type/url in apm.yml) contradicts the direction set in #132 — registry-first config overlays, no inline server definitions. #122 (full inline definitions) was closed as won't-fix for this reason.

To merge, please remove the inline dict installation pipeline:

Remove _install_inline_mcp_deps(), _validate_inline_url(), and the inline paths in _install_mcp_dependencies()
Remove doc updates describing inline dict format (dependencies.md, integrations.md)
Remove inline installation tests

Keep:

Transitive collection + dedup for registry string deps
LockFile-based collection
mcp: null guard in get_mcp_dependencies()
from_apm_yml() preserving dicts during parsing (data fidelity fix)

Ready to approve once scoped to the bug fix.

Scope down to registry-only MCP propagation per review feedback. Inline dict format contradicts microsoft#132 (registry-first) and microsoft#122 (closed as won't-fix). Removed: - _validate_inline_url() and _install_inline_mcp_deps() from cli.py - TestValidateInlineUrl (5 tests) and TestInstallInlineMCPDeps (10 tests) - Inline dict examples and descriptions from dependencies.md and integrations.md Kept (bug-fix scope): - _collect_transitive_mcp_deps() with LockFile-based collection - _deduplicate_mcp_deps() for registry string dedup - mcp: null guard in get_mcp_dependencies() - from_apm_yml() preserving dicts for data fidelity

sergio-sisternes-epam · 2026-03-03T12:42:15Z

@danielmeppiel — Done. Scoped down in 940c9e4:

Removed:

_install_inline_mcp_deps(), _validate_inline_url(), and the inline split in _install_mcp_dependencies()
Inline dict examples/descriptions from dependencies.md and integrations.md
TestValidateInlineUrl (5 tests) and TestInstallInlineMCPDeps (10 tests)

Kept (bug-fix scope):

Transitive collection + dedup for registry string deps
LockFile-based collection
mcp: null guard in get_mcp_dependencies()
from_apm_yml() preserving dicts (data fidelity)

Full suite passes (1211 passed, 32 skipped). Ready for re-review.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/apm_cli/cli.py:2327

Because the function accepts mixed mcp_deps but only acts on registry_deps, the user-facing output/summary risks counting or describing “MCP dependencies” that are never processed (e.g., inline dict entries). Consider either filtering non-string entries before computing any counts/messages, or emitting a distinct message indicating how many inline vs registry entries will be handled.

    # Filter to registry strings only (inline dicts are preserved during
    # parsing for data fidelity but not installed — see #132).
    registry_deps = [d for d in mcp_deps if isinstance(d, str)]

    console = _get_console()

    # Start MCP section with clean header

Copilot · 2026-03-03T14:46:35Z

src/apm_cli/cli.py

+    # Filter to registry strings only (inline dicts are preserved during
+    # parsing for data fidelity but not installed — see #132).
+    registry_deps = [d for d in mcp_deps if isinstance(d, str)]
+


_install_mcp_dependencies now accepts mixed MCP entries (str/dict), but immediately filters to registry_deps (strings only) and does not handle dict (inline) entries anywhere else in this module. As a result, projects with only inline MCP dicts (or transitive inline dicts) will report an MCP section but perform no configuration. Either add an inline-dict installation path here, or explicitly reject/skip dict entries with a clear warning so users don’t think they were applied.

This issue also appears on line 2321 of the same file.

danielmeppiel

Review: Scope reduction incomplete — dict MCP artifacts remain

The transitive MCP propagation bug fix is solid and well-engineered. However, the descoping to remove inline dict support was only partially applied. The inline writers were removed, but the entire intake pipeline for dict deps remains: type widening, parser preservation, collection, deduplication, tests, and terminology.

What happens today: Dict MCP deps are parsed → collected → deduplicated → then silently dropped at install time by registry_deps = [d for d in mcp_deps if isinstance(d, str)]. This is worse UX than rejecting at parse time (the original behavior), and leaves ~130 lines of dead code + 7 dead tests.

What's needed: One cleanup commit that strips dict handling throughout, leaving a focused bug fix for transitive registry string deps + the mcp: null guard. See inline comments for exact locations.

The core value delivery (transitive collection, lockfile-scoped scanning, dedup, mcp: null fix, already_configured separation) is all good to ship — this is purely about removing the leftover scaffolding for a feature that hasn't been designed yet (#132).

danielmeppiel · 2026-03-03T19:39:30Z

src/apm_cli/models/apm_package.py

    source: Optional[str] = None  # Source location (for dependencies)
    resolved_commit: Optional[str] = None  # Resolved commit SHA (for dependencies)
-    dependencies: Optional[Dict[str, List[Union[DependencyReference, str]]]] = None  # Mixed types for APM/MCP
+    dependencies: Optional[Dict[str, List[Union[DependencyReference, str, dict]]]] = None  # Mixed types for APM/MCP/inline


Revert type widening — descoped to #132.

This adds dict to the union type but dict MCP deps were explicitly descoped. No consumer in this PR uses dict deps — they get parsed, collected, deduplicated, then silently dropped at install time by registry_deps = [d for d in mcp_deps if isinstance(d, str)]. That's worse UX than rejecting at parse time.

Revert to Union[DependencyReference, str].

danielmeppiel · 2026-03-03T19:39:38Z

src/apm_cli/models/apm_package.py

                    else:
-                        # Other dependencies (like MCP) remain as strings
-                        dependencies[dep_type] = [str(dep) for dep in dep_list if isinstance(dep, str)]
+                        # Other dependencies (like MCP): keep strings and dicts


Revert parser change — descoped to #132.

isinstance(dep, (str, dict)) preserves dicts that flow through the entire pipeline (collection → dedup → install) only to be filtered out at install time. This creates a silent data loss path: dicts are "collected" and "deduplicated" but never installed, with no warning to the user.

Revert to the original isinstance(dep, str) filter. Comment should remain # Other dependencies (like MCP) remain as strings.

danielmeppiel · 2026-03-03T19:39:47Z

src/apm_cli/models/apm_package.py

+    def get_mcp_dependencies(self) -> List[Union[str, dict]]:
+        """Get list of MCP dependencies (strings for registry, dicts for inline configs)."""
        if not self.dependencies or 'mcp' not in self.dependencies:
            return []


Revert return type to List[str]; keep the or [] null guard.

The return type widening to List[Union[str, dict]] and the isinstance(dep, (str, dict)) filter are dict scope creep. The or [] fix for mcp: null is a real bug fix — keep that.

Suggested change

return []

def get_mcp_dependencies(self) -> List[str]:

"""Get list of MCP dependencies (as strings for compatibility)."""

if not self.dependencies or 'mcp' not in self.dependencies:

return []

return [dep for dep in (self.dependencies.get('mcp') or [])

if isinstance(dep, str)]

danielmeppiel · 2026-03-03T19:39:57Z

src/apm_cli/cli.py

+    Falls back to scanning all apm.yml files if no lock file is available.
+
+    Args:
+        apm_modules_dir: Path to the apm_modules directory.


Fix return type annotation.

-> list (bare) loses all type information and is inconsistent with the rest of the codebase. Should be -> List[str].

danielmeppiel · 2026-03-03T19:40:08Z

src/apm_cli/cli.py

+    Strings are deduped by value; dicts are deduped by their 'name' key.
+
+    Args:
+        deps: Mixed list of str and dict MCP entries.


Strip dict logic from _deduplicate_mcp_deps — descoped to #132.

The seen_names / dict-by-name branch (~lines 2293-2308) handles a type that never enters this function if the model layer filters at parse time (comments #1-#3 above). Simplify to string-only dedup:

def _deduplicate_mcp_deps(deps: List[str]) -> List[str]: """Deduplicate MCP dependency strings preserving insertion order.""" seen: builtins.set = builtins.set() result = [] for dep in deps: if dep not in seen: seen.add(dep) result.append(dep) return result

Also: deps: list and -> list should be List[str].

danielmeppiel · 2026-03-03T19:40:20Z

src/apm_cli/cli.py

+        mcp_deps: List of MCP dependency entries (registry strings)
        runtime: Target specific runtime only
        exclude: Exclude specific runtime from installation
        verbose: Show detailed installation information


Remove registry_deps filter, fix type annotation, clean up terminology.

Three things here:

Type annotation: mcp_deps: list → mcp_deps: List[str]

Dead filter (line ~2327): registry_deps = [d for d in mcp_deps if isinstance(d, str)] is only needed because dicts leak through the pipeline. Once the model layer strips them at parse time (comments Why do we need a GitHub token? #1-Will there be MCP coverage? #3), all deps are strings. Remove this filter and the # Filter to registry strings only...see #132 comment. Use mcp_deps directly throughout.

"Registry" terminology leak: Four user-facing strings reference "registry servers" / "registry MCP servers" — this split terminology only makes sense alongside inline deps, which were descoped. Locations:

"Validating {len(registry_deps)} registry servers..." → "Validating {len(mcp_deps)} servers..."

"All registry MCP servers already configured" → "All MCP servers already configured"

"Already configured registry MCP servers: " → "Already configured MCP servers: "

# --- Registry-based deps (strings) --- → # --- Install MCP servers ---

danielmeppiel · 2026-03-03T19:40:32Z

tests/unit/test_transitive_mcp.py

+
+    def test_parse_dict_mcp_deps(self, tmp_path):
+        """Inline dict MCP deps are preserved."""
+        inline = {"name": "my-srv", "type": "sse", "url": "https://example.com"}


Remove 7 dead-path dict tests; fix 1 fixture; update docstring.

Tests covering descoped dict functionality (#132) that should be removed:

test_parse_dict_mcp_deps (line 37)

test_parse_mixed_mcp_deps (line 51)

test_collects_dict_deps (line 115)

test_deduplicates_dicts_by_name (line 230)

test_mixed_dedup (line 237)

test_dict_without_name_kept (line 247)

test_root_deps_take_precedence_over_transitive (line 252)

Additionally:

Line 170 (test_lockfile_with_virtual_path): Change the dict MCP dep fixture {"name": "learn", ...} to a registry string dep so the test exercises the actual supported code path.

Line 20 (class docstring): Change "preserves both string and dict MCP entries" → "parses MCP entries correctly".

danielmeppiel · 2026-03-03T19:40:40Z

docs/dependencies.md

    - github/awesome-copilot/skills/review-and-refactor  # Code review skill
  mcp:
-    - io.github.github/github-mcp-server
+    - io.github.github/github-mcp-server          # Registry reference


Remove # Registry reference qualifier — misleading after descoping.

The # Registry reference inline comment implies the existence of a non-registry format. Since inline dict deps were descoped to #132, this qualifier is misleading. Remove the comment or use something neutral like # MCP server.

danielmeppiel

Given discussion in #132 , let's merge this with the inline related code for now

Copilot AI review requested due to automatic review settings February 27, 2026 18:26

sergio-sisternes-epam requested a review from danielmeppiel as a code owner February 27, 2026 18:26

Copilot started reviewing on behalf of sergio-sisternes-epam February 27, 2026 18:26 View session

Copilot AI reviewed Feb 27, 2026

View reviewed changes

sergio-sisternes-epam requested a review from Copilot February 27, 2026 18:53

Copilot started reviewing on behalf of sergio-sisternes-epam February 27, 2026 18:54 View session

Copilot AI reviewed Feb 27, 2026

View reviewed changes

src/apm_cli/cli.py Outdated Show resolved Hide resolved

sergio-sisternes-epam added 2 commits February 27, 2026 19:07

fix: replace streamable-http with http in docs and tests

f334098

The correct MCP transport types are 'sse' and 'http'. Updated docs and test fixtures that incorrectly used 'streamable-http' to align with _KNOWN_MCP_TYPES validation.

sergio-sisternes-epam requested a review from Copilot February 28, 2026 15:15

Copilot started reviewing on behalf of sergio-sisternes-epam February 28, 2026 15:15 View session

Copilot AI reviewed Feb 28, 2026

View reviewed changes

src/apm_cli/cli.py Outdated Show resolved Hide resolved

src/apm_cli/cli.py Outdated Show resolved Hide resolved

src/apm_cli/cli.py Outdated Show resolved Hide resolved

danielmeppiel added the needs-triage New issue, not yet reviewed by maintainers label Mar 2, 2026

fix: address copilot feedback for transitive MCP install

7f80e89

danielmeppiel mentioned this pull request Mar 2, 2026

[Discussion] Design inline MCP dependency format vs MCP Registry alignment #131

Closed

danielmeppiel requested changes Mar 2, 2026

View reviewed changes

Merge branch 'main' into feat/transitive-mcp-dependencies

f746291

danielmeppiel removed the needs-triage New issue, not yet reviewed by maintainers label Mar 2, 2026

sergio-sisternes-epam added 3 commits March 2, 2026 22:40

danielmeppiel requested changes Mar 3, 2026

View reviewed changes

danielmeppiel and others added 2 commits March 3, 2026 04:27

Merge branch 'main' into feat/transitive-mcp-dependencies

46e0ce0

Merge branch 'main' into feat/transitive-mcp-dependencies

c4969c5

danielmeppiel requested a review from Copilot March 3, 2026 14:40

Copilot started reviewing on behalf of danielmeppiel March 3, 2026 14:40 View session

Copilot AI reviewed Mar 3, 2026

View reviewed changes

Merge branch 'main' into feat/transitive-mcp-dependencies

b45122f

sergio-sisternes-epam added this to the 0.8.0 milestone Mar 3, 2026

danielmeppiel requested changes Mar 3, 2026

View reviewed changes

sergio-sisternes-epam mentioned this pull request Mar 3, 2026

[FEATURE] MCP dependency config overlays in apm.yml #132

Closed

Merge branch 'main' into feat/transitive-mcp-dependencies

720d040

danielmeppiel approved these changes Mar 4, 2026

View reviewed changes

danielmeppiel merged commit 0126597 into microsoft:main Mar 4, 2026
6 checks passed

Conversation

sergio-sisternes-epam commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🚀 New Feature

Description

Changes Made

Testing

Checklist

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!

Uh oh!

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danielmeppiel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Summary of Required Changes

Recommended Path

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danielmeppiel left a comment

Choose a reason for hiding this comment

Uh oh!

sergio-sisternes-epam commented Mar 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

danielmeppiel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Review: Scope reduction incomplete — dict MCP artifacts remain

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sergio-sisternes-epam commented Feb 27, 2026 •

edited

Loading

danielmeppiel left a comment •

edited

Loading

danielmeppiel left a comment •

edited

Loading