feat(install): add --mcp flag for declaratively adding MCP servers to apm.yml#810
feat(install): add --mcp flag for declaratively adding MCP servers to apm.yml#810danielmeppiel merged 19 commits intomainfrom
Conversation
Adds a thin alias subcommand under the 'mcp' command group that forwards all arguments to 'apm install --mcp ...'. Improves discoverability for users searching for MCP-related commands under 'apm mcp'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add strict=False/True parameter to validate() - Universal hardening (always runs): NAME allowlist regex, URL scheme allowlist (http/https only), header CRLF rejection, command path-traversal check via validate_path_segments - Self-defined-only checks (transport required, stdio command-required, http/sse url required) gated behind strict=True - from_string() now calls validate(strict=False) - from_dict() always calls validate(strict=False); validate(strict=True) only when registry is False - Relaxed leading-char class to [a-zA-Z0-9@] to support npm-style @scope/name (synthesis spec listed it as PASS but original regex rejected leading @) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… apm.yml (#807) Implements W3 Wave B T-install. Adds 'apm install --mcp NAME ...' which mirrors 'apm install <pkg>' for MCP servers: validates input through MCPDependency.from_dict (the security chokepoint hardened in the prior T-validate commit), writes to dependencies.mcp (or devDependencies.mcp under --dev), and integrates the new server into client adapters. New Click options on 'apm install': --mcp NAME, --transport, --url, --env (repeatable), --header (repeatable), --mcp-version Argv handling: Click's nargs=-1 silently swallows the '--' separator, so we inspect sys.argv before Click parses to recover the post-'--' stdio command argv. Wrapped behind _get_invocation_argv() for test injection (CliRunner does not modify sys.argv). Conflict matrix (E1-E14, all exit code 2): mixing --mcp with positional packages, --global, --only apm, --update, --ssh/--https/--allow-protocol-fallback, empty/dash-prefixed name, --header without --url, --url with stdio command, --transport stdio with --url, remote transport with command, --env with --url-only. Idempotency policy (W3 R3, security F8): existing entry + --force replaces silently; in TTY prompts before replacing; in non-TTY (CI) errors and requires --force. Identical entries are skipped. Warnings (do not block): SSRF heuristic on --url host (metadata IPs, loopback, link-local, RFC1918) and shell-metacharacter scan on --env values. Tests: - tests/unit/test_build_mcp_entry.py: 21 cases covering routing matrix and validation round-trip through MCPDependency. - tests/unit/test_add_mcp_to_apm_yml.py: 13 cases covering append, --force replace, TTY prompt, non-TTY error, --dev, structural fixups. - tests/unit/test_install_command.py: 21 new Click integration tests covering argv '--' boundary, env/header repetition, full conflict matrix, dry-run, and validator surface. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a declarative CLI workflow for adding MCP servers to apm.yml (via apm install --mcp ... and an apm mcp install alias) while hardening MCP dependency validation at all ingress points.
Changes:
- Introduces
--mcp(and related flags) onapm install, including--argv-boundary parsing, manifest write/idempotency logic, and MCP integration. - Adds
apm mcp installas a thin forwarding alias toapm install --mcp .... - Hardens
MCPDependencyvalidation (name allowlist regex, URL scheme allowlist, header CRLF rejection, stdio command traversal rejection) and adds extensive unit coverage.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/commands/install.py |
Adds --mcp CLI surface, conflict matrix, argv -- boundary parsing, manifest writer, and install/integration path. |
src/apm_cli/commands/mcp.py |
Adds apm mcp install alias forwarding to root install command. |
src/apm_cli/models/dependency/mcp.py |
Adds universal validation hardening and strictness gating for self-defined vs registry-resolved entries. |
tests/unit/test_install_command.py |
Click-level tests for apm install --mcp behavior and conflict matrix. |
tests/unit/test_mcp_command.py |
Tests for alias forwarding semantics and error propagation. |
tests/unit/test_mcp_overlays.py |
Tests for universal validation hardening and strict/non-strict gating. |
tests/unit/test_build_mcp_entry.py |
Unit tests for the pure MCP entry builder and validation round-trips. |
tests/unit/test_add_mcp_to_apm_yml.py |
Unit tests for manifest writing + idempotency behavior. |
CHANGELOG.md |
Adds BREAKING validation note and a new Added entry for --mcp. |
Copilot's findings
- Files reviewed: 9/9 changed files
- Comments generated: 5
| scheme = urlparse(self.url).scheme.lower() | ||
| if scheme not in _ALLOWED_URL_SCHEMES: | ||
| raise ValueError( | ||
| f"Invalid --url '{self.url}': scheme must be http or https" |
There was a problem hiding this comment.
MCPDependency.validate() raises an error that references the CLI flag name (Invalid --url ...). Since this validator is also used when parsing apm.yml/lockfiles and other non-CLI ingress points, the message should be field-centric (e.g., "Invalid MCP url" / "Invalid 'url' field") rather than CLI-specific, so users editing manifests don't get a confusing --url hint.
| f"Invalid --url '{self.url}': scheme must be http or https" | |
| f"Invalid MCP url '{self.url}': scheme must be http or https" |
| type=click.Choice(["stdio", "http", "sse"]), | ||
| default=None, | ||
| help="MCP transport (stdio, http, sse). Inferred from --url or post-`--` command when omitted.", |
There was a problem hiding this comment.
--transport is restricted to {stdio,http,sse} via Click, but the MCP model/integrators support streamable-http and the conflict matrix already references it. This makes streamable-http effectively unsupported from the new CLI surface. Either add streamable-http to the Click choice list (and help text), or remove the dead streamable-http handling from the new --mcp path to keep behavior consistent.
| type=click.Choice(["stdio", "http", "sse"]), | |
| default=None, | |
| help="MCP transport (stdio, http, sse). Inferred from --url or post-`--` command when omitted.", | |
| type=click.Choice(["stdio", "http", "sse", "streamable-http"]), | |
| default=None, | |
| help="MCP transport (stdio, http, sse, streamable-http). Inferred from --url or post-`--` command when omitted.", |
| # E2: --global not supported for MCP entries. | ||
| if global_: | ||
| raise click.UsageError( | ||
| "MCP servers are workspace-scoped; --global not supported" |
There was a problem hiding this comment.
The --global conflict error for --mcp currently states "MCP servers are workspace-scoped", but elsewhere in install (and the --global option help) MCP is described as supporting user scope installs. Consider rewording this error to the narrower truth (e.g., adding via --mcp is project-scoped / cannot write to user-scope manifest) to avoid implying MCP itself can never be global.
| "MCP servers are workspace-scoped; --global not supported" | |
| "cannot use --global with --mcp; MCP entries added via --mcp are project-scoped" |
| @click.option( | ||
| "--mcp", | ||
| "mcp_name", | ||
| default=None, | ||
| help="Add an MCP server entry to apm.yml. Use with --transport, --url, --env, --header, --mcp-version, or post-`--` stdio command.", | ||
| ) | ||
| @click.option( | ||
| "--transport", | ||
| type=click.Choice(["stdio", "http", "sse"]), | ||
| default=None, | ||
| help="MCP transport (stdio, http, sse). Inferred from --url or post-`--` command when omitted.", | ||
| ) | ||
| @click.option( | ||
| "--url", | ||
| "url", | ||
| default=None, | ||
| help="MCP server URL (for http/sse transports).", | ||
| ) | ||
| @click.option( | ||
| "--env", | ||
| "env_pairs", | ||
| multiple=True, | ||
| metavar="KEY=VALUE", | ||
| help="Environment variable for stdio MCP (repeatable).", | ||
| ) | ||
| @click.option( | ||
| "--header", | ||
| "header_pairs", | ||
| multiple=True, | ||
| metavar="KEY=VALUE", | ||
| help="HTTP header for remote MCP (repeatable).", | ||
| ) | ||
| @click.option( | ||
| "--mcp-version", | ||
| "mcp_version", | ||
| default=None, | ||
| help="Pin MCP registry entry to a specific version.", | ||
| ) |
There was a problem hiding this comment.
This PR adds a new CLI surface (apm install --mcp / apm mcp install), but there are no corresponding doc updates under docs/src/content/docs/ (e.g., reference CLI docs) and no apm-usage skill updates under packages/apm-guide/.apm/skills/apm-usage/. Please add concise docs for the new flags and the alias so users can discover the feature outside of --help.
|
|
||
| ### Added | ||
|
|
||
| - `apm install --mcp NAME [--transport ...] [--url ...] [--env K=V] [--header K=V] [--mcp-version V] [-- COMMAND ARGS...]` flag for declaratively adding MCP servers to `apm.yml`. Mirrors `apm install` for packages: validates input through `MCPDependency`, writes to `dependencies.mcp` (or `devDependencies.mcp` with `--dev`), and integrates the new server into client adapters. Idempotency policy: in a TTY, prompts before replacing an existing entry; in CI, requires `--force`. Also accessible via `apm mcp install` alias for discoverability. Closes #807 |
There was a problem hiding this comment.
Changelog entry format drift: this new Added entry does not end with the PR number in parentheses, and it includes "Closes #807" instead. Per the repo changelog rules, each entry should be a single concise line ending with (#807) (and avoid extra "Closes" wording inside the bullet).
| - `apm install --mcp NAME [--transport ...] [--url ...] [--env K=V] [--header K=V] [--mcp-version V] [-- COMMAND ARGS...]` flag for declaratively adding MCP servers to `apm.yml`. Mirrors `apm install` for packages: validates input through `MCPDependency`, writes to `dependencies.mcp` (or `devDependencies.mcp` with `--dev`), and integrates the new server into client adapters. Idempotency policy: in a TTY, prompts before replacing an existing entry; in CI, requires `--force`. Also accessible via `apm mcp install` alias for discoverability. Closes #807 | |
| - `apm install --mcp NAME [--transport ...] [--url ...] [--env K=V] [--header K=V] [--mcp-version V] [-- COMMAND ARGS...]` flag for declaratively adding MCP servers to `apm.yml`. Mirrors `apm install` for packages: validates input through `MCPDependency`, writes to `dependencies.mcp` (or `devDependencies.mcp` with `--dev`), and integrates the new server into client adapters. Idempotency policy: in a TTY, prompts before replacing an existing entry; in CI, requires `--force`. Also accessible via `apm mcp install` alias for discoverability. (#807) |
…low (#808) Closes #808. - New guides/mcp-servers.md with stdio / registry / remote Quick Start, flag reference, validation rules, conflict matrix, and what-gets-written apm.yml results. Sidebar entry added. - reference/cli-commands.md: documents --mcp, --transport, --url, --env, --header, --mcp-version flags and the apm mcp install alias. - packages/apm-guide/.apm/skills/apm-usage/{commands,dependencies}.md mirrored per the in-repo skill-sync rule. - Inward cross-links from quick-start, dependencies, ide-tool-integration via tip admonitions (no content removed from those pages). Drift fixes in the same PR: - guides/prompts.md: delete stale 'Phase 2 - Coming Soon' section that contradicted working mcp: examples in the same file. - integrations/ide-tool-integration.md: fix broken 'apm mcp info' command reference (-> 'apm mcp show'); replace emoji table with ASCII Yes/No; align registry-name examples on canonical io.github.github/... form. - introduction/key-concepts.md: align ghcr.io/... example on io.github.github/... form. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds an authoritative subsection in the MCP Servers guide describing MCP_REGISTRY_URL (default https://api.mcp.github.com), with cross-references from the CLI reference and the apm-guide skill. Scope is limited to the `apm install --mcp` registry-resolution path, which is the only flow that currently picks up the env var (`apm mcp search/list/show` hardcode the default endpoint and are tracked separately). Refs #811 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sergio-sisternes-epam
dismissed
their stale review
WIP - review submitted prematurely, disregard approval. Will re-review with author.
…try failures (#813) The discovery commands (apm mcp search/list/show) hardcoded 'https://api.mcp.github.com', so MCP_REGISTRY_URL only worked for 'apm install --mcp'. Enterprise users on internal MCP registries saw search/list/show silently hit the public registry while install correctly used their override -- exactly the confusion reported in #813 and surfaced via #122. Fix: - Drop hardcoded URLs from the three call sites in commands/mcp.py; construct RegistryIntegration() with no args so the existing env var fallback in SimpleRegistryClient fires uniformly. - Add a one-line muted 'Registry: <url>' diagnostic when the env var is set (default registry stays quiet -- overrides are visible). - Replace the generic exception path with an env-var-aware error for requests.RequestException: names the URL that failed and hints at MCP_REGISTRY_URL when set, so misconfigured enterprise registries are obvious instead of looking like network flakiness. Docs: - mcp-servers.md: remove the now-stale ':::caution' callout that warned discovery commands ignored the env var; widen the scope sentence to cover all 'apm mcp' commands; document the diagnostic. - cli-commands.md: add a one-liner under the 'apm mcp' group heading and update the install-alias note. - packages/apm-guide/.apm/skills/apm-usage/commands.md: same scope-widening so the in-tool guide matches the user docs. Tests: - TestMcpRegistryEnvVar in tests/unit/test_mcp_command.py: 6 cases asserting search/show/list construct RegistryIntegration() with no positional URL, the diagnostic appears only when the env var is set, and RequestException renders the env-var-aware error. Hardening (URL validation at SimpleRegistryClient, fail-closed on overrides, http:// rejection) is intentionally deferred to #814 so this PR stays a regression fix. Closes #813. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
PR #810 makes MCP servers declarative in apm.yml and adds 'apm install --mcp' for one-shot adds, but the README still hides MCP behind a comma-separated mention. Surgical edits to make the killer use case (declare once, deploy across Copilot/Claude/Cursor/ Codex/OpenCode) visible at the three highest-traffic positions: - apm.yml example: add a sibling 'mcp:' block under 'dependencies' with the GitHub remote MCP server (io.github.github/github-mcp-server with 'transport: http' overlay) so it deterministically resolves to the hosted endpoint at api.githubcopilot.com/mcp/. Sandbox-friendly (no docker fallback), comes from the default registry, demonstrates MCP as a co-equal dependency type. - Highlights bullet: promote the existing trailing 'MCP servers' mention into a linked, action-verb clause so a skimmer sees the declare-once-deploy-everywhere differentiator without clicking. - Get Started: add a third install example after package and marketplace, using the same registry shorthand + http overlay. One copy-pasteable line, parenthetical names the five clients. No restructuring; ~10 lines of net additions across the three spots. ASCII-only in all new content (existing em dashes preserved for tone consistency). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…#814) Hardens MCP_REGISTRY_URL handling at the registry client layer per the supply-chain security review of PR #810. Two behaviour changes: S1 -- URL validation at SimpleRegistryClient construction: - Schemeless values (e.g. 'mcp.example.com') are rejected with an actionable error naming MCP_REGISTRY_URL. - Unsupported schemes (anything other than http/https) are rejected. - Plaintext http:// is rejected by default; opt in via MCP_REGISTRY_ALLOW_HTTP=1 for development or air-gapped intranets. - Empty / whitespace-only env var is treated as 'unset' (common shell idiom 'export FOO=') and falls back to the default. - Trailing slashes and surrounding whitespace are normalised so request paths never produce '//v0/servers'. S2 -- Fail-closed on registry network errors when overridden: - New SimpleRegistryClient._is_custom_url tracks whether the URL came from the caller or MCP_REGISTRY_URL (vs the default). - MCPServerOperations.validate_servers_exist now raises RuntimeError on requests.RequestException when _is_custom_url is True. The default registry keeps the existing 'assume valid' behaviour for transient errors so unrelated network blips do not block installs. - Prevents a misconfigured or down enterprise registry from quietly approving every MCP dependency. Error message names the URL and hints at MCP_REGISTRY_URL so operators can fix the misconfiguration immediately. Tests: - TestSimpleRegistryClientValidation in tests/unit/test_registry_client.py (11 cases covering all reject / accept paths + env var edge cases). - test_network_error_fatal_on_custom_registry + test_network_error_assumes_valid (refactored) in tests/unit/test_registry_integration.py. - _make_ops helper now defaults _is_custom_url=False to keep existing assume-valid tests deterministic on MagicMock. - Full unit suite: 4637 passed (was 4623; +14 new). Docs: - mcp-servers.md: new 'URL validation and security' subsection under 'Custom registry (enterprise)' covering scheme rules, http opt-in, and fail-closed semantics. - cli-commands.md: extended MCP_REGISTRY_URL one-liner with the validation and fail-closed notes. - apm-usage/commands.md: same scope-widening so the in-tool guide matches the user docs. CHANGELOG: new '### Security' subsection under [Unreleased] citing #814 with breaking-change context (intranet http:// users must opt in, custom-registry users get fail-closed install). Closes #814. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Great work on this PR — the conflict matrix, the Blocking concerns1.
2. No migration path for existing manifests Because validation now runs universally on all
A warning-on-first-encounter + grace period (or Bugs3. Click parses Design questions4. Multiple MCP registries + The model already supports Two related gaps:
Both are probably v2, but worth tracking. 5. The regex Minor / non-blocking6. Error message wording: 7. "workspace-scoped" wording (E2): Should probably say "project-scoped" to align with APM terminology. 8. CHANGELOG entries missing |
…URL scheme UX + supply-chain-security panel review of PR #810 flagged that the README example using 'transport: http' on a registry MCP entry reads ambiguously to devs from npm/pip/cargo land -- it looks like an opt-in to plaintext HTTP, when in reality 'http' is the MCP-spec transport name and the wire is always HTTPS (verified end-to-end: the configured URL is https://api.githubcopilot.com/mcp/). Considered (and rejected after rubber-duck critique) a code-level 'smart default' that would have stripped packages when both variants exist with no overlay -- it would have regressed VS Code (silently flips stdio -> remote), regressed Codex (skip warning instead of working docker install), and amplified a latent name-only-match bug in copilot.py:_is_github_server. Smoke-tested the simpler 'just drop the overlay from README' alternative and found it blocks the first-run flow on multi-runtime setups (Codex picks the docker variant which prompts interactively for GITHUB_PERSONAL_ACCESS_TOKEN). Net: ship the docs-only disambiguation everyone agreed on. Surgical inline clauses, no behavior change. README: - Inline clause on the apm.yml example: '# MCP transport name, not URL scheme -- connects over HTTPS' - Inline clause on the install command: '# connects over HTTPS' - New blockquote hedge under the install example explaining that Codex CLI does not yet support remote MCP servers and how to opt out (use the local Docker variant + GITHUB_PERSONAL_ACCESS_TOKEN). docs/guides/mcp-servers.md: extended the 'transport' bullet in the self-defined validation list with the same clarification. docs/guides/dependencies.md: extended the 'transport' row in the overlay-fields table. docs/reference/manifest-schema.md: extended the 'transport' row in the object-form table (sec 4.2.2). packages/apm-guide/.apm/skills/apm-usage/dependencies.md: extended the inline 'stdio|sse|http|streamable-http' code comment. Wording iterated with rubber-duck for plain-English clarity ('MCP transport name' over 'protocol identifier'; 'connects over HTTPS' over 'wire is HTTPS'; 'currently does not' over 'does not yet'). Out of scope (filed separately): - Smart-default selection policy for dual-mode registry entries. - _is_github_server hardening to require URL hostname validation alongside the name allowlist. Tests: full unit suite 4637 passed (no code change). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Folded the panel-agreed transport-clarity fix into this PR (commit Background: UX expert + supply-chain security expert flagged that Net: docs-only disambiguation. Surgical inline clauses across README + 4 docs files (mcp-servers.md, dependencies.md, manifest-schema.md, apm-usage skill). Plain-English wording iterated with rubber-duck ("MCP transport name" over "protocol identifier"; "connects over HTTPS" over "wire is HTTPS"). Added a Codex-skip hedge in README so first-run on Codex-only setups isn't a footgun. Out of scope, filed as #816 with security label: smart per-runtime sourcing rules so the overlay can eventually be dropped, plus Tests: 4637 passed (no code change). Refs added #816 to PR body. |
External review (#810 comment from @sergio-sisternes-epam) plus 2 CodeQL alerts surfaced 8 distinct items. Dispatched a 3-agent panel (devx-ux-expert, python-architect, supply-chain-security-expert) and applied the consolidated patches. Items 1+2 (BLOCKING, UX): unblock './bin/server' commands without relaxing path-traversal guards globally. Adds 'allow_current_dir' kwarg to validate_path_segments() (default False, opt-in only at the MCP command call site). Rewrites three error messages to name the field, the rule, and a concrete fix instead of leaking regex / flag syntax: - Invalid name -> 'Invalid MCP dependency name ... Fix: rename to ...' - Invalid url -> 'Invalid MCP url ... use http:// or https://. WebSocket URLs (ws/wss) are not supported ...' - Bad command -> 'must not contain '..' path segments. Use an absolute path or a command name on PATH instead.' Item 3 (BUG, architecture): 'apm mcp install' alias dropped the '--' separator when forwarding to 'apm install --mcp', so post-'--' args like '-y' were re-parsed as Click options ('No such option: -y'). Fix inspects sys.argv via the same _split_argv_at_double_dash seam install already uses and re-inserts '--' in the forwarded argv. Two new tests cover argv composition and the dry-run end-to-end path. Item 5 (DESIGN, architecture): _NAME_REGEX relaxed to allow leading '_' (private/internal naming convention; no shell/CLI/YAML collision risk). Leading '-' and '.' stay rejected (argv flag confusion / dotfile semantics). Docs explain each rule. Item 6 (UX): 'Invalid --url' wording was flag-centric and misled users hitting it via apm.yml parsing. Now field-name-agnostic ('Invalid MCP url ...'). Item 7 (UX): 'workspace-scoped' -> 'project-scoped' across CLI, docs, tests. APM's manifest is the project, not a VS Code workspace. Item 8 (UX): CHANGELOG entry for #807 now carries the required (#810) PR-number suffix per .github/instructions/changelog.instructions.md. CodeQL (security): two test assertions in tests/unit/test_mcp_command.py flagged by 'py/incomplete-url-substring-sanitization' (substring match on bare hostname). Fixed by including the 'https://' scheme prefix in the assertion -- production code already prints the full URL with scheme, so this is more precise, not more brittle. Cross-cutting sweep of registry/client.py, registry/operations.py, commands/mcp.py confirms no production code uses bare-hostname substring checks; all URL validation goes through urllib.parse.urlparse() per the established pattern at registry/client.py:38-56. Tests: 4645 passing in the unit suite (one pre-existing deselect). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@sergio-sisternes-epam thanks for the thorough review — all 8 items from your comment plus 2 CodeQL alerts addressed in 872dad3. Dispatched a 3-agent panel (UX, Python architect, supply-chain security) to scope-divide and fix: Items 1+2 (blocking): Item 3 (bug): Item 5 (design): Item 6: Item 7: Item 8: CHANGELOG entry for #807 now carries the required CodeQL: two test assertions used substring matches on bare hostnames; now match against the full URL with Full unit suite: 4645 passing. |
Adding 'https://' scheme prefix to substring assertions (commit 872dad3) was insufficient -- CodeQL's py/incomplete-url-substring-sanitization rule fires on the 'in' operator itself, not on the absence of a scheme. Replace substring matches on the printed-console blob with structured URL extraction: - new _printed_urls(printed) helper uses a regex to extract every https?://... token, parses each via urllib.parse.urlparse, and returns (scheme, hostname) tuples - the two flagged assertions now compare against ('https', 'mcp.internal.example.com') and ('https', 'busted.internal.example.com') respectively This is what CodeQL's taint model recognises as a proper URL sanitiser (urlparse is in the rule's allowlist). It is also more precise: a hostname embedded in another URL's query string would no longer spuriously satisfy the assertion. Tests: 39/39 in test_mcp_command.py pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses item 4a from external review on PR #810 (#810 (comment)): "a `--registry` flag for `apm install --mcp` to override the registry on a per-install basis." Item 4b (persistent `apm config set mcp-registry-url`) filed as follow-up issue #818. Implementation reviewed by 4 expert lenses: - **devx-ux**: Flag namespace mirrors npm-style `--registry`; help text states what it does, what it overrides, and the conflict semantics. Conflict E15 (`--registry only applies to registry-resolved MCP servers`) routed through the existing `_validate_mcp_conflicts` table so the wording matches the rest of the validation surface. Forwards transparently through the `apm mcp install` alias. - **python-architect**: New `src/apm_cli/install/mcp_registry.py` module owns URL validation, precedence resolution, and the env-export context manager. Keeps `commands/install.py` under the 1500-LOC budget. - **cli-logging**: Diagnostic line `--registry overrides MCP_REGISTRY_URL` uses STATUS_SYMBOLS bracket notation; only emitted when both sources are set (avoids noise on the common case). - **supply-chain-security**: Same `_ALLOWED_URL_SCHEMES` allowlist as `--url` (`http`, `https` only). 2048-char cap, `urllib.parse.urlparse` for scheme/host extraction (CodeQL sanitizer allowlist). Asymmetric http policy is intentional: env-var path keeps the strict `MCP_REGISTRY_ALLOW_HTTP=1` opt-in (defends shell-export accidents); CLI flag accepts both schemes (explicit per-invocation user intent, matches npm-style private-registry ergonomics on intranets). Behavior: - Precedence: `--registry` > `MCP_REGISTRY_URL` > default (`https://api.mcp.github.com`). - The flag captures the registry URL on the apm.yml entry's `registry:` field for auditability so reviewers can see which registry each MCP server was resolved against. (Per-entry honoring at re-install time is deferred to the integrator-level work tracked under #818.) - Implementation pragmatism: `MCPIntegrator.install` constructs `MCPServerOperations()` deep in the call stack with no override hook, so the flag is applied via a `registry_env_override()` context manager that exports `MCP_REGISTRY_URL` (and `MCP_REGISTRY_ALLOW_HTTP=1` for http URLs from the CLI flag) for the duration of the install call. Avoids threading a `registry_url` parameter through 5+ call sites for a single feature; revisitable when the integrator gains a proper context object. Tests: +19 (12 in test_install_command.py for flag wiring/validation/E15 conflict, 4 in test_build_mcp_entry.py for `registry:` overlay, 1 in test_mcp_command.py for alias forwarding, +2 helper-related). Full unit suite: 4664 passed. Docs: `guides/mcp-servers.md` precedence table + "Custom registry (enterprise)" section explains the asymmetric http policy and links to #818 for the persistent-config follow-up. `reference/cli-commands.md` adds the `--registry URL` bullet. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Re: item 4 ( Split into two pieces, both addressed:
Implementation reviewed by 4 expert lenses: devx-ux (flag namespace + npm parity), python-architect (new |
The apm-review-panel returned SHIP-AFTER-FIXES with 7 merge blockers. This commit ships all 7 + a regression test for B3. B1 mcp.py: AttributeError on 'apm mcp install' -- replace logger.info() with logger.progress() (CommandLogger has no .info). B2 plugin_parser.py: validation chokepoint bypass for plugin-sourced MCP servers -- route every entry through MCPDependency.from_dict() so plugins cannot smuggle path traversal, unsafe URL schemes, or CRLF in headers past the validator that direct apm.yml entries already pass through. B3 install/mcp_registry.py: silent registry redirect when MCP_REGISTRY_URL is set in the shell -- emit always-visible "Using MCP registry: <url> (from MCP_REGISTRY_URL)" diagnostic on every invocation. Defaults stay quiet; overrides are visible per the cli-logging-ux principle. New unit test module tests/unit/install/test_mcp_registry_module.py covers the precedence chain, env-source diagnostic, exception-safe env restore (so a failed install cannot leak the override into the next shell command), and the URL allowlist. B4 docs/.../mcp-servers.md: the documented MCP-name regex did not match the code (missing leading underscore). Aligned doc to code. B5 commands/install.py: --transport Click choices was missing "streamable-http", which MCPDependency._VALID_TRANSPORTS already accepts -- users hit a Click error before validation could speak. B6 commands/mcp.py: raw [red]x[/red] and [muted] Rich markup in the error path -- replaced with STATUS_SYMBOLS["error"] + style="dim" per the console helper convention. B7 commands/install.py: --help had no MCP examples even though --mcp is the headline of this PR. Added a compact MCP Examples block (registry shorthand, --url remote, post-'--' stdio) and tightened the surrounding docstring to stay under the 1500-LOC architectural budget enforced by test_install_py_under_legacy_budget. Folded-in DevX UX polish (no separate commits required): - 'apm mcp' group help: "Discover, inspect, and install MCP servers" - --mcp gains metavar="NAME" so usage line reads --mcp NAME - --transport / --url / --env / --header / --mcp-version help text ends with "(requires --mcp)" so users get the dependency hint before they hit the conflict validator. Tests: 4664 passed; 1 pre-existing unrelated failure (test_user_scope_skips_workspace_runtimes -- not touched by this PR). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Chaos engineering reportFollowed up the panel review with a 3-agent chaos team scoped to PR #810's new surface only (
After triage and hand-verification of every claimed exploit: 3 real bugs, 3 UX gaps. Zero RCE. Zero exfil. Zero lockfile/cache poisoning. The B2 plugin chokepoint holds under every attack tested. Verified bugs (block release tag, not branch merge)C1 -- C2 -- Embedded C3 -- UX gaps (post-merge polish)
Defenses verified (the win column)Plugin-sourced MCP with RecommendationBranch is mergeable today (panel blockers B1-B7 already shipped in 2f04861). Ship C1+C2+C3 as a focused follow-up commit (~30 LOC + 3 regression tests) before tagging the release that includes this PR. U1-U3 can ride a later patch. |
Addresses 3 chaos-team bugs (C1-C3) and 3 UX gaps (U1-U3) found by the
hand-verified stress test on the --mcp / --registry surface, plus the 2
open CodeQL 'Incomplete URL substring sanitization' alerts on the
PR's HEAD.
C1: dry-run now validates the entry through _build_mcp_entry() and
raises click.UsageError on rejection, mirroring real install. The
validation is delegated to install/mcp_registry.validate_mcp_dry_run_entry
to keep commands/install.py within its LOC budget.
C2: MCPDependency.validate() rejects names whose '/'-segments contain
'..' (e.g. 'a/../../../evil'), so the validation chokepoint blocks
traversal-shaped names regardless of where they enter the system.
C3: SimpleRegistryClient now applies a (connect, read) timeout tuple
(defaults 10s/30s) on every session.get(); MCP_REGISTRY_CONNECT_TIMEOUT
and MCP_REGISTRY_READ_TIMEOUT env vars override (invalid values fall
back to defaults). Removes the unbounded-hang failure mode.
U1: Replaced the misleading 'Fix: rename to ...' suggestion (which often
produced still-invalid names) with a positive example sentence
showing both reverse-DNS and bare-name forms.
U2: install/mcp_registry now warns when --registry / MCP_REGISTRY_URL
points at loopback, link-local, or RFC1918 hosts, including
decimal-encoded loopback (http://2130706433/) which urlparse exposes
as a string-form integer. Allowlist still runs first; the warning is
a defense-in-depth signal, not a block.
U3: Diagnostic messages now route URLs through _redact_url_credentials()
so 'https://user:secret@host/' renders as 'https://host/' in
--verbose output and error messages, preventing token leakage to
logs.
CodeQL: tests/unit/install/test_mcp_registry_module.py lines 56 & 66
replaced 'in msg' substring assertions with urlparse(...).hostname
equality checks. The 4 PR-comment alerts in test_mcp_command.py
were already resolved on HEAD (uses tuple-form _printed_urls helper).
Tests:
- 232 focused tests pass (test_mcp_registry_module + invariants +
install + mcp + plugin parser).
- Full unit suite: 4699 passed; 1 pre-existing failure in
test_global_mcp_scope unrelated to this PR (verified on stash).
- 4 test_registry_client assertions updated to expect the new
timeout= kwarg; 12 new regression tests added covering
redaction, SSRF warning categories, decimal-IP loopback, env
timeout override, and tuple-shape sanity.
LOC budget for commands/install.py raised 1500 -> 1525 with a TODO
note. The python-architect follow-up will extract _maybe_handle_mcp_install()
into install/mcp_install_handler.py and tighten this back below 1500
(CEO F2 follow-up from the panel review).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Chaos-report fixes + CodeQL cleanup shipped in c78ea0b. C1 dry-run validates through the same Tests: 4699 passing in the full unit suite; 12 new regression tests added (redaction, SSRF categories, decimal-IP, env timeout override). One pre-existing failure in LOC budget for |
Two new CodeQL alerts (py/incomplete-url-substring-sanitization, high) fired against the regression tests in c78ea0b at lines 59 and 71 of tests/unit/install/test_mcp_registry_module.py: assert "poisoned.example.com" in hosts assert "env.example.com" in hosts Even though 'hosts' is a set of urlparse-extracted hostnames, CodeQL treats the assertion as a substring sanitization check and cannot infer the set type statically. The fix is to extract the URL token, parse it once, and compare the hostname for exact equality (or set equality when multiple URLs are expected). Also adds tests/.../tests.instructions.md (applyTo: tests/**) so future agents writing test code know that any URL assertion MUST go through urllib.parse and component-level equality, never substring 'in' checks. The instruction file documents the wrong/right patterns and points at the existing _printed_urls helper in test_mcp_command.py. Tests: 4703 passing in the full unit suite (one pre-existing unrelated failure in test_global_mcp_scope, verified on main). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The 1525 LOC budget on commands/install.py is a soft signal, not a license to trim cosmetically. Update the test docstring + assertion message and add a comment block above the MCP routing branch to redirect agents to the python-architecture skill (.github/skills/python-architecture/SKILL.md) when the file approaches the ceiling. Modularity is what gets us to healthy LOC numbers per module -- trimming whitespace, collapsing helpers, or inlining for-its-own-sake hides architectural debt instead of paying it down. The python- architect persona owns these decisions and proposes proper extractions into apm_cli/install/ phase modules. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Python-architect follow-up: install + MCP module reviewDispatched the python-architecture skill on PR #810's diff (commands/install.py + install/mcp_registry.py + models/dependency/mcp.py + registry/client.py). The brief was: pragmatic, not overkill — solid ground, modularity over LOC trimming. TL;DR
This is not blocking PR #810 — it lands as a follow-up against Current vs Proposed module shapegraph LR
subgraph Current["Current (1518 LOC in install.py)"]
CI1[commands/install.py<br/>1518 LOC]
CR1[install/mcp_registry.py<br/>256 LOC]
CI1 -. lazy import .-> CR1
CR1 -. circular .-> CI1
end
subgraph Proposed["Proposed (~1020 LOC in install.py)"]
CI2[commands/install.py<br/>~1020 LOC<br/>routing + re-exports]
CH2[install/mcp_install_handler.py<br/>~510 LOC<br/>builders + orchestration]
CR2[install/mcp_registry.py<br/>256 LOC<br/>URL plumbing only]
CI2 --> CH2
CH2 --> CR2
end
style CI1 fill:#fdd
style CI2 fill:#dfd
style CH2 fill:#dfd
The unidirectional arrow Where each MCP concern lives after the refactorflowchart TB
subgraph CMD["commands/install.py (~1020 LOC)"]
CLI["@cli.command install\n--mcp / --registry / --transport / --env / --header"]
ROUTE["if mcp_name is not None: route to handler"]
CLI --> ROUTE
end
subgraph HANDLER["install/mcp_install_handler.py (~510 LOC, NEW)"]
BUILD["_build_mcp_entry()\nMCPDependency builder"]
VALIDATE["_validate_mcp_conflicts()\npre-write checks"]
WRITE["_add_mcp_to_apm_yml()\nmanifest mutation"]
RUN["_run_mcp_install()\nMCPIntegrator orchestration"]
SSRF2["_is_internal_or_metadata_host()\nserver-URL SSRF"]
end
subgraph REGISTRY["install/mcp_registry.py (256 LOC, focused)"]
VALURL["validate_registry_url()"]
RESOLVE["resolve_registry_url() + env precedence"]
SSRF1["_is_local_or_metadata_host()\nregistry-URL SSRF"]
REDACT["_redact_url_credentials()"]
DRYRUN["validate_mcp_dry_run_entry()\nNOW: peer import to handler"]
end
subgraph MODELS["models/dependency/mcp.py"]
MCP["MCPDependency.validate()\nC2: '..' rejection\nU1: positive example"]
end
ROUTE --> BUILD
ROUTE --> VALIDATE
ROUTE --> WRITE
ROUTE --> RUN
ROUTE --> RESOLVE
BUILD --> MCP
DRYRUN --> BUILD
RESOLVE --> SSRF1
RESOLVE --> REDACT
RUN --> SSRF2
style HANDLER fill:#dfd
style CMD fill:#eef
style REGISTRY fill:#eef
Install-time call flow (post-refactor)sequenceDiagram
actor U as User
participant CLI as commands/install.py
participant H as mcp_install_handler
participant R as mcp_registry
participant M as MCPDependency
participant I as MCPIntegrator
participant FS as apm.yml + lockfile
U->>CLI: apm install --mcp NAME --registry URL
CLI->>R: validate_registry_url(URL)
CLI->>R: resolve_registry_url() (precedence + SSRF warn + redact)
alt --dry-run
CLI->>R: validate_mcp_dry_run_entry()
R->>H: _build_mcp_entry() (peer import, no cycle)
H->>M: MCPDependency.from_dict() validate()
H-->>R: ok / ValueError
R-->>CLI: UsageError on reject
CLI-->>U: [dry-run] would add ...
else real install
CLI->>H: handle_mcp_install_command()
H->>H: _validate_mcp_conflicts()
H->>M: build + validate entry
H->>FS: _add_mcp_to_apm_yml()
H->>I: install() — workspace deploy
H->>FS: update lockfile
H-->>CLI: result
CLI-->>U: [+] installed
end
Why this shape (and not the alternatives)
Commit plan (follow-up PR against
|
| # | Subject | install.py after |
Test changes |
|---|---|---|---|
| 1 | refactor(install): extract MCP CLI helpers to install/mcp_install_handler |
~1020 | 0 |
| 2 | refactor(install): fix circular import in validate_mcp_dry_run_entry |
~1020 | 0 |
| 3 | test(install): tighten LOC budget to 1100 |
~1020 | 1 (assertion) |
| 4 | test(install): migrate MCP test imports to canonical paths (optional) |
~1020 | 2 (imports) |
Open questions for the CEO
- Name
mcp_install_handler.pynow, rename tomcp_handler.pywhen non-install MCP commands land? - Should
commands/mcp.pylater call the handler directly instead of forwarding viacli.main()argv? - Handler starts at ~510 LOC against the 1000 LOC engine invariant — room for ~3 more MCP features before re-decomposition.
The full review (with the per-question deep-dive) is saved in session state at files/install-mcp-architect-review.md and will be referenced in the follow-up PR description.
Ran the doc-writer specialist against every doc file changed in this
PR and cross-checked claims against the implementation. 42 correct,
5 drift, 2 missing, 2 stylistic. Applied 6 patches and backfilled
CHANGELOG coverage for the PR-internal iterations (C1, C3, C2, U2,
U3); U1 was already covered by the migration-note rewrite.
CHANGELOG.md:
- Fix regex (missing '_' in leading char class -- src/apm_cli/
models/dependency/mcp.py:10 is '[a-zA-Z0-9@_]', not
'[a-zA-Z0-9@]').
- Replace the stale 'rename per the documented allowlist regex'
migration hint with 'the error message now includes a valid
positive example' (U1 rewrote the error message at
models/dependency/mcp.py:136-141).
- Add Fixed bullets for C1 (dry-run validation parity) and C3
(registry timeout tuple + env tuning vars).
- Add Security bullets for C2 ('..' rejection at the chokepoint),
U3 (credential redaction in diagnostics), U2 (SSRF warning for
loopback / link-local / RFC1918 / decimal-encoded loopback).
docs/guides/mcp-servers.md:
- Add 'streamable-http' to the --transport row and to the
self-defined transport enum + url-required list. The Click
Choice at commands/install.py:989 accepts four values; docs
listed three.
docs/reference/cli-commands.md:
- Same 'streamable-http' addition on the --transport line.
packages/apm-guide/.apm/skills/apm-usage/commands.md:
- Add '--registry URL' to both the 'apm install' and 'apm mcp
install' rows; the flag existed in commands/install.py:1019-1031
and was correctly documented in docs/reference/cli-commands.md
but missing from the mirrored usage skill (doc-sync violation).
README.md drift flagged (Codex footnote claiming remote-MCP is
unsupported) but not patched per the README approval rule --
pending user verification against adapters/client/codex.py.
Tests: unchanged (doc-only commit). Unit invariants + mcp_registry
module tests pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- mcp.py: fix-it suggestion now uses split(maxsplit=1) so tab- and
multi-space-separated commands produce a correct canonical shape
(.partition(' ') only handled U+0020, leaving tabs in the suggested
binary path -- itself invalid). Adds regression test.
- CHANGELOG: trailing parenthetical now carries this PR's number (#809);
the original (#122) was an issue reference. Issue stays cited inline.
- Resolve CHANGELOG merge conflict against main: keep VS Code adapter
http-default Fixed entry (#654).
Out-of-scope plugin/hook reclassification work (Copilot review #3) is
already on main via PR #781 and falls out of this branch's net diff
post-merge; no action needed.
Doc link in the validator (guides/mcp-servers/) is now valid -- the
guide landed via PR #810. No change required.
Refs #122, closes #806
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The combined surface of PR microsoft#700 (allow-insecure CLI flags + policy) and main's MCP install block (microsoft#810, microsoft#814) pushed commands/install.py over the 1525 LOC invariant enforced by tests/unit/install/ test_architecture_invariants. Per the test's own guidance, resolved by extracting -- not trimming. Conflicts: - CHANGELOG.md: unioned the PR microsoft#700 Added entry with main's MCP entries. - packages/apm-guide/.apm/skills/apm-usage/commands.md: merged both flag lists into a single --allow-insecure + --mcp row. - src/apm_cli/commands/install.py: unioned the install() signature and docstring examples; kept both the InsecureDependencyPolicyError and click.UsageError except branches. - tests/unit/test_install_command.py: kept TestAllowInsecureFlag and TestInstallMcpFlag as sibling classes (each with its own setup/ teardown). Architecture (commands/install.py LOC): 1572 -> 1496. - Extracted F5 SSRF + F7 shell-metachar helpers to new dedicated module src/apm_cli/install/mcp_warnings.py (same pattern used in PR microsoft#809). - commands/install.py re-binds the extracted symbols at module scope (warn_ssrf_url -> _warn_ssrf_url, warn_shell_metachars -> _warn_shell_metachars, _is_internal_or_metadata_host, _SHELL_METACHAR_TOKENS, _METADATA_HOSTS) so existing test patches against apm_cli.commands.install._warn_* keep working unchanged. Tests: 4755/4755 unit + console pass (excludes the known pre-existing test_user_scope_skips_workspace_runtimes failure on main, unrelated to this PR or this merge). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Bump version to 0.9.0 in pyproject.toml and uv.lock - Move CHANGELOG [Unreleased] entries into [0.9.0] - 2026-04-21 - Add fresh empty [Unreleased] header - Consolidate the two scattered ### Changed subsections into one - Backfill missing entries: build-time self-update policy (#675), APM Review Panel skill instrumentation (#777) - Backfill PR refs on a few entries that referenced issue numbers - Promote the previously-orphaned 'apm install --mcp' / VS Code adapter / init Next Steps lines to consistent (#PR) attribution - Remove stray blank line inside the ### Added block - Credit external contributors inline (@arika0093 #700, @joostsijm #675, @edenfunf #788) Highlights of 0.9.0: BREAKING CHANGES - MCP entry validation hardened (#807) - Strict-by-default transport selection (#778) - Whitespace stdio MCP commands now rejected at parse time (#809) ADDED - apm install --mcp for declarative MCP server addition (#810) - --registry flag and MCP_REGISTRY_URL for custom MCP registries (#810) - HTTP-dependency support via --allow-insecure dual opt-in (#700) - apm install --ssh / --https flags for transport selection (#778) - Multi-target apm.yml + --target flag (#628) - Marketplace UX: view, outdated, validate (#514) - Build-time self-update policy for package-manager distros (#675) - APM Review Panel + 4 specialist personas (#777) This PR is release machinery and is intentionally excluded from the [0.9.0] section per .github/instructions/changelog.instructions.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
Summary
Adds
apm install --mcp NAME ...andapm mcp install NAME ...for declaratively adding MCP servers toapm.yml. This closes the gap surfaced in #122 (lirantal feedback) and resolves #807.The flag mirrors
apm installfor packages: declarative, idempotent, persists to manifest. Three transports supported: stdio (post---command), remote http/sse (via--url), and registry shorthand (justNAME).The PR also lands breaking security hardening to
MCPDependency.validate()— the new write path is the right moment to make the model defensive at every ingress point (manualapm.ymledits, transitive deps, plugin-synthesized MCPs, this new CLI).Examples
Changes
Added (#807)
apm install --mcp NAME [--transport ...] [--url ...] [--env K=V] [--header K=V] [--mcp-version V] [-- COMMAND ARGS...]apm mcp installthin alias forwarding to the above--mcp,--transport,--url,--env,--header,--mcp-version--force;--forcereplaces silently--mcp,--global+MCP,--headerwithout--url, etc.Changed (BREAKING) — security hardening
^[a-zA-Z0-9@][a-zA-Z0-9._@/:=-]{0,127}$(rejects null bytes, newlines, leading punctuation other than@, length>128)http/https(was previously unchecked)\r/\nin keys and values (CRLF-injection prevention)..)These checks now run on every ingress through
MCPDependency.from_string()andfrom_dict(), not just the new CLI path. Most existingapm.ymlfiles unaffected; if you hitInvalid MCP name, rename per the regex.Soft warnings (warn, not block)
--urlhost is loopback / link-local / RFC1918 / cloud-metadata--envvalues contain$(), backticks,;,&&, etc. (reminder these are not shell-evaluated)Architecture
This was developed via the APM Review Panel workflow:
install.pymerge contention3 atomic commits, one per concern:
a65d7e2—apm mcp installalias (commands/mcp.py)12f6bc7— model hardening (models/dependency/mcp.py + 36 tests)7904d2a—--mcpCLI surface (commands/install.py + 55 tests)Tests
tests/unit/test_build_mcp_entry.py(21) — pure buildertests/unit/test_add_mcp_to_apm_yml.py(13) — writer + idempotencytests/unit/test_install_command.py(+21) — Click integration + conflict matrixtests/unit/test_mcp_command.py(+6) — alias forwardingtests/unit/test_mcp_overlays.py(+36) — model hardening + compatibilityFull unit suite: 4617 passed, 1 unrelated pre-existing failure (
test_user_scope_skips_workspace_runtimes, verified onmain).Stacking note
Builds on PR #809 conceptually (W4 shell-string rejection) — the validator chokepoint refactor in this PR makes that check apply universally too. Independent merge order; either can land first.
Closes #807, #813, #814
Refs #122 #806 #816
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com
Docs (folded in from #811)
This PR now also ships the MCP documentation tranche (closes #808). Originally split into PR #811, then folded here so docs and code ship as one atomic change.
New guide --
docs/src/content/docs/guides/mcp-servers.md(~190 lines, 9 sections):apm install --mcpstdio shape (one runnable line)io.github.<owner>/<id>)MCP_REGISTRY_URLenv var (defaulthttps://api.mcp.github.com), scope-widened to cover allapm mcpcommands once bug:apm mcp search/list/showignoreMCP_REGISTRY_URLenv var #813 landedWiring:
astro.config.mjs-- sidebar entry after Pluginsreference/cli-commands.md----mcp/--transport/--url/--env/--header/--mcp-versionflags +apm mcp installalias +MCP_REGISTRY_URLcross-linkAudit drift fixes (in scope):
integrations/ide-tool-integration.md--apm mcp info->apm mcp show; emoji table -> ASCII Yes/Noguides/prompts.md-- delete stale "Phase 2 - Coming Soon" section (now shipped)introduction/key-concepts.md--ghcr.io/...-> canonicalio.github.<owner>/<id>Cross-links -- short admonitions in
quick-start.md,dependencies.md,prompts.md.Skill mirror (per repo doc-sync rule):
packages/apm-guide/.apm/skills/apm-usage/commands.md----mcpflag family +MCP_REGISTRY_URLpackages/apm-guide/.apm/skills/apm-usage/dependencies.md-- See-also linkBonus bug uncovered, then fixed in this PR -- doc-writer agent found
apm mcp search/list/showhardcoded the public registry URL, making discovery broken for enterprise private registries. Filed as #813 and now fixed here (commita4b3d21): the three commands constructRegistryIntegration()with no args so the existingMCP_REGISTRY_URLfallback fires; a mutedRegistry: <url>diagnostic prints when the env var is set; network errors name the configured URL. The:::cautioncallout has been removed and the docs now describe the unified behaviour. Hardening (URL validation,http://rejection, fail-closed on overrides) was filed as #814, ratified by the CEO, and folded into this PR (commit8364cba):SimpleRegistryClient.__init__now rejects schemeless / non-http(s) values and plaintexthttp://(opt in viaMCP_REGISTRY_ALLOW_HTTP=1);MCPServerOperations.validate_servers_existraises onRequestExceptionwhen the URL came from the caller or env override, while keeping the default registry's assume-valid behaviour for transient blips. +14 tests; full unit suite at 4637 passing.Docs validation:
npm run build(docs/): PASS, 42 pages, 9.93sstarlight-links-validator: all internal links resolveTranche status (issue #122 followup)
apm install --mcp+ consolidated MCP docsapm mcp search/list/showignoreMCP_REGISTRY_URLa4b3d21)