diff --git a/.apm/architecture/owners/install-deployment.json b/.apm/architecture/owners/install-deployment.json index 248ef22d37..f348745afa 100644 --- a/.apm/architecture/owners/install-deployment.json +++ b/.apm/architecture/owners/install-deployment.json @@ -68,12 +68,16 @@ "id": "install-invocation-option-defaults", "decision": "Install invocation option defaults", "owner": "install/request.py (InstallRequest)", - "selectors": [ - "src/apm_cli/install/request.py", - "src/apm_cli/commands/install.py" - ], + "selectors": ["src/apm_cli/install/request.py"], "guards": ["install-deployment-request-defaults"] }, + { + "id": "install-command-scope-selection", + "decision": "Install command scope selection", + "owner": "commands/install.py (install computes scope once)", + "selectors": ["src/apm_cli/commands/install.py"], + "guards": ["install-deployment-install-scope-selection"] + }, { "id": "file-level-deploy-sync-cleanup", "decision": "File-level deploy / sync / cleanup", diff --git a/CHANGELOG.md b/CHANGELOG.md index 60e9340d8b..22c6f56e26 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,6 +68,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 semver tag while continuing unrelated updates; malformed remote tag records still fail before writes. The contract is recorded in `openapm-v0.1.md`. (#2667) +- `apm install -g --mcp NAME` now creates or updates the user manifest and + deploys only to global-capable runtimes instead of rejecting `--global`. + Registry identities are validated before user-state writes, fail closed when + any registry is unreachable, and reject ambiguous bare names. Registry URLs + reject embedded credentials, queries, and fragments; saved or + environment-provided HTTP endpoints require explicit opt-in. + `openapm-v0.1.md` now codifies the user-scope target-selection and + pre-mutation contract. (#2734) - Distributed `apm compile` now reconciles existing managed-section `AGENTS.md` files without overwriting hand-authored content, generates new placements safely, and never discovers, writes, or cleans content across diff --git a/CONFORMANCE.json b/CONFORMANCE.json index d5f9cfe801..ef7f42cc78 100644 --- a/CONFORMANCE.json +++ b/CONFORMANCE.json @@ -1,4 +1,9 @@ { + "consumer_user_scope": { + "lockfile_location": "~/.apm/apm.lock.yaml", + "manifest_location": "~/.apm/apm.yml", + "target_capability_declaration": "MCPClientAdapter.supports_user_scope (OpenAPM Target Registry v0.1 implementation profile)" + }, "generator": "gen_statement.py v1", "requirements": [ { @@ -1389,12 +1394,23 @@ "tests/spec_conformance/test_copilot_plugins_reqs.py::test_plugin_name_precedence_and_owner_identity_are_deterministic", "tests/spec_conformance/test_copilot_plugins_reqs.py::test_removal_is_exact_and_unowned_entries_are_refused" ] + }, + { + "conformance_class": "consumer", + "id": "req-tg-014", + "keyword": "MUST", + "section": "8.5.8", + "status": "active", + "test_count": 1, + "tests": [ + "tests/spec_conformance/test_manifest_reqs.py::test_user_scoped_mcp_target_selection_ignores_project_signals" + ] } ], "spec_version": "v0.1.1", "summary_by_class": { "consumer": { - "active": 89, + "active": 90, "skipped": 1, "unbound": 0, "xfail": 0 @@ -1418,5 +1434,5 @@ "xfail": 0 } }, - "total_requirements": 120 + "total_requirements": 121 } diff --git a/CONFORMANCE.md b/CONFORMANCE.md index 21ebec7639..355f215af0 100644 --- a/CONFORMANCE.md +++ b/CONFORMANCE.md @@ -14,12 +14,18 @@ There is NO automated CI detector for spec-vs-behaviour drift beyond the four se All four conformance classes (Producer, Consumer, Registry, Governance) carry active coverage in this statement. The Registry class is exercised via the trust-anchor invariant test in `tests/spec_conformance/test_registry_reqs.py`, which hashes the committed Registry-archive fixture and asserts equality with the digest the paired lockfile advertises (sec.11.3.3, req-rg-001). +## Consumer user-scope disclosure + +- Manifest: `~/.apm/apm.yml` +- Lockfile: `~/.apm/apm.lock.yaml` +- Target capability declaration: `MCPClientAdapter.supports_user_scope (OpenAPM Target Registry v0.1 implementation profile)` + ## Coverage summary | Class | Active | Skipped | Xfail | Unbound | |-------|-------:|--------:|------:|--------:| | Producer | 12 | 0 | 0 | 0 | -| Consumer | 89 | 1 | 0 | 0 | +| Consumer | 90 | 1 | 0 | 0 | | Registry | 1 | 0 | 0 | 0 | | Governance | 17 | 0 | 0 | 0 | @@ -147,6 +153,7 @@ All four conformance classes (Producer, Consumer, Registry, Governance) carry ac | [req-tg-011](docs/src/content/docs/specs/openapm-v0.1.md#req-tg-011) | MUST | 8.5.5 | consumer | active | 2 | - | | [req-tg-012](docs/src/content/docs/specs/openapm-v0.1.md#req-tg-012) | MUST | 8.5.6 | consumer | active | 1 | - | | [req-tg-013](docs/src/content/docs/specs/openapm-v0.1.md#req-tg-013) | MUST | 8.5.7 | consumer | active | 7 | - | +| [req-tg-014](docs/src/content/docs/specs/openapm-v0.1.md#req-tg-014) | MUST | 8.5.8 | consumer | active | 1 | - | ## Waivers diff --git a/docs/public/specs/manifests/openapm-v0.1.requirements.yml b/docs/public/specs/manifests/openapm-v0.1.requirements.yml index 115d861542..8e27cf8939 100644 --- a/docs/public/specs/manifests/openapm-v0.1.requirements.yml +++ b/docs/public/specs/manifests/openapm-v0.1.requirements.yml @@ -438,6 +438,11 @@ requirements: section: "8.5.7" conformance_class: consumer notes: "schema, effective-target, integrity, security, and executable admission drives one in-place aggregate registration opaque to legacy projection without lifecycle host-binary probing; only admitted dependencies participate in claimant selection and registration, direct dependencies win plugin-name collisions over transitive dependencies, equal-precedence collisions fail during admission, advisory cleanup omits ambiguous or changed-owner entries, and ledger-primary ownership permits exact-entry recovery of a reserved namespace while rejecting foreign collisions and invalid JSON, preserving unrelated JSON semantics, and rolling back catalog, ledger, and settings together" + - id: req-tg-014 + keyword: MUST + section: "8.5.8" + conformance_class: consumer + notes: "user-scoped MCP target selection uses a disclosed versioned capability contract, ignores project-only detection signals, applies first-source precedence without fallback, filters mixed sets, and refuses zero-supported selections before user manifest, lockfile, or target-config mutation" - id: req-pr-006 keyword: MUST section: "8.1" diff --git a/docs/src/content/docs/consumer/install-mcp-servers.md b/docs/src/content/docs/consumer/install-mcp-servers.md index ccf02a65ad..16942d6cea 100644 --- a/docs/src/content/docs/consumer/install-mcp-servers.md +++ b/docs/src/content/docs/consumer/install-mcp-servers.md @@ -88,6 +88,12 @@ apm install --mcp filesystem -- npx -y @modelcontextprotocol/server-filesystem / # Remote apm install --mcp linear --transport http --url https://mcp.linear.app/sse + +# Global stdio install (writes user APM + runtime config only) +apm install -g --target claude --mcp fetch -- npx -y @modelcontextprotocol/server-fetch + +# Global registry install (requires one unique registry match) +apm install -g --target claude --mcp io.github.github/github-mcp-server ``` `apm mcp install NAME ...` is an alias that forwards to the same code @@ -220,12 +226,16 @@ non-whitespace absolute path. Unset or blank values use `~/.claude.json`; relative values are rejected. Codex CLI writes to `$CODEX_HOME/config.toml` when `CODEX_HOME` is set to a non-whitespace value or `~/.codex/config.toml` otherwise, Gemini CLI to `~/.gemini/settings.json`, Antigravity CLI to `~/.gemini/config/mcp_config.json`, Windsurf to `~/.codeium/windsurf/mcp_config.json`, Kiro to `~/.kiro/settings/mcp.json`, -and JetBrains Copilot to its OS-specific user config). When the -package declares a `targets:` field (or the CLI passes `--target`), -only the matching runtimes receive the config write. When neither -restricts targets, all detected user-scope-capable runtimes are -configured. Workspace-only runtimes (VS Code, Cursor, OpenCode) are -skipped at user scope. +JetBrains Copilot to its OS-specific user config, and Hermes when enabled). +When the user-scope manifest declares a `targets:` field (or the CLI passes `--target`), +only the matching runtimes receive the config write. When no CLI target, +user-scope manifest target, or saved `apm config target` restricts targets, +all detected user-scope-capable runtimes are configured. Workspace-only +runtimes (VS Code, Cursor, OpenCode) are +skipped with a warning when a mixed target set also contains a global-capable +runtime. If none of the selected targets supports user scope, the command exits +`2` before changing the user manifest, lockfile, or runtime configuration. The direct command creates or updates +`~/.apm/apm.yml`; it does not fall back to the current project's manifest. ## stdio vs HTTP servers diff --git a/docs/src/content/docs/enterprise/security.md b/docs/src/content/docs/enterprise/security.md index f4651a5bad..8cfa761205 100644 --- a/docs/src/content/docs/enterprise/security.md +++ b/docs/src/content/docs/enterprise/security.md @@ -482,6 +482,17 @@ APM integrates MCP (Model Context Protocol) server configurations from packages. For Codex remote transport requirements, see [stdio vs HTTP servers](../../consumer/install-mcp-servers/#stdio-vs-http-servers). +### Direct registry installs at user scope + +`apm install -g --mcp NAME` can use an MCP registry endpoint to update +account-wide runtime configuration. APM requires HTTPS for registry URLs from +the environment or saved config unless `MCP_REGISTRY_ALLOW_HTTP=1` is set, and +rejects embedded credentials, query strings, and fragments. When a non-default +registry supplies the entry, APM saves that registry URL for reproducible +replay, but registry metadata is not signed or content-hash verified. Treat a +custom registry as a trusted source with access to every global runtime selected +for the install. + ### Direct dependencies MCP servers declared by your direct dependencies (packages listed in your `apm.yml`) are auto-trusted. You explicitly chose to depend on these packages, so their MCP server declarations are accepted. diff --git a/docs/src/content/docs/reference/cli/config.md b/docs/src/content/docs/reference/cli/config.md index eb5528e3d1..767a00ae8e 100644 --- a/docs/src/content/docs/reference/cli/config.md +++ b/docs/src/content/docs/reference/cli/config.md @@ -53,7 +53,7 @@ Write `KEY` to `~/.apm/config.json`. Validates the value before writing: - `self-update.channel` must be `stable` or `prerelease`. - `self-update.install-dir` is expanded and stored as an absolute path. It becomes the default `APM_INSTALL_DIR` for `apm self-update` when the env var is not set. - `copilot-cowork-skills-dir` must be absolute after expansion; the directory itself does not need to exist. -- `mcp-registry-url` must be an `http://` or `https://` URL with a valid host. All other schemes are rejected. +- `mcp-registry-url` must be an `http://` or `https://` URL with a valid host. Embedded credentials, invalid ports, query strings, fragments, and other schemes are rejected. Configured `http://` endpoints require `MCP_REGISTRY_ALLOW_HTTP=1` when used. - Boolean keys reject anything outside the accepted truthy/falsy strings. ### `apm config unset KEY` @@ -75,7 +75,7 @@ Remove `KEY` from `~/.apm/config.json`. No-op if the key is not set. Supported u | `audit-on-install` | enum | `off` | Default content-audit mode for `apm install`: `off` / `warn` / `block`. `warn` records findings in the install summary; `block` halts on critical findings. Overridable per-install with `--audit` / `--no-audit`; an org policy `security.audit.on_install` floor can raise it. Requires the `external-scanners` experimental flag for `set`. | | `external..llm` | boolean | unset | Opt a SARIF scanner into LLM-powered analysis (`` validated against supported scanners). SkillSpector default is offline. LLM mode makes outbound API calls and needs `OPENAI_API_KEY` or `NVIDIA_INFERENCE_KEY`. Overridable per-run with `--external-llm` / `--no-external-llm`. Requires the `external-scanners` experimental flag. | | `external..args` | string | unset | Extra scanner CLI flags, stored shlex-split as a list (e.g. `"--model gpt-4o"`). Allowlist-validated per adapter at run time. Overridable per-run with `--external-args`. Requires the `external-scanners` experimental flag. | -| `mcp-registry-url` | URL | public registry | Persist a private MCP registry endpoint. Accepts `http://` or `https://` URLs. Sits between `MCP_REGISTRY_URL` env and the built-in default in the resolution chain. Equivalent to exporting `MCP_REGISTRY_URL` permanently. | +| `mcp-registry-url` | URL | public registry | Persist a private MCP registry endpoint. Accepts `http://` or `https://` URLs. Configured HTTP endpoints require `MCP_REGISTRY_ALLOW_HTTP=1` when used; an explicit `--registry http://...` is the per-invocation opt-in. Sits between `MCP_REGISTRY_URL` env and the built-in default in the resolution chain. | | `registry..url` | URL | unset | Base URL for registry ``. Requires `registries` experimental flag. | | `registry..token` | string | unset | Bearer token for registry ``. Stored in `~/.apm/config.json`; never in repo-tracked files. Requires `registries` experimental flag. | | `registry..default` | boolean | `false` | Mark `` as the user-scoped default registry. Only one registry may be default at a time; setting `true` clears any previous default. Requires `registries` experimental flag. | diff --git a/docs/src/content/docs/reference/cli/install.md b/docs/src/content/docs/reference/cli/install.md index 470274b3a8..deba5ab09d 100644 --- a/docs/src/content/docs/reference/cli/install.md +++ b/docs/src/content/docs/reference/cli/install.md @@ -31,7 +31,7 @@ With no arguments it installs everything from `apm.yml`. With one or more `PACKA |---|---|---| | `--update` | off | Re-resolve dependencies to the latest version or Git ref allowed by `apm.yml` and rewrite `apm.lock.yaml`. Mutable Git refs must resolve against upstream; APM does not fall back to stale refs from the local bare Git cache. Mutually exclusive with `--frozen`. For interactive use with a confirmation prompt, use [`apm update`](../update/) instead. | | `--frozen` | off | Lockfile-only install: refuse to resolve anything new and fail before any project, config, deployment, or cache write if `apm.lock.yaml` is missing or out of sync with `apm.yml`, including MCP state. Mirrors `npm ci`. Mutually exclusive with `--update`, positional package additions, and `--mcp`. | -| `--dry-run` | off | Print the install plan without deployment writes. Positional packages and ref changes appear in the preview after validation but do not change an existing `apm.yml`; auto-bootstrap still keeps its new manifest and any explicit `--target` selection for the next run. | +| `--dry-run` | off | Print the install plan without deployment writes. Positional packages and ref changes appear in the preview after validation but do not change an existing `apm.yml`; auto-bootstrap still keeps its new manifest and any explicit `--target` selection for the next run. The `-g --mcp` path creates no user manifest, lockfile, or runtime configuration. | | `--force` | off | Overwrite locally-authored files on collision **and** bypass the security scan's critical-finding block. Does **not** suppress general install errors (any reported error still exits `1`, matching npm / pip / cargo) or select ref freshness. Add `--update` or `--refresh` to resolve mutable refs upstream; [`apm update`](../update/) does so with or without `--force`. Use only after independent verification. | | `--verbose`, `-v` | off | Show per-file paths and full error context in the diagnostic summary. | | `--dev` | off | Add new packages to `devDependencies`. Dev deps install locally but are excluded from `apm pack` output. | @@ -46,11 +46,11 @@ With no arguments it installs everything from `apm.yml`. With one or more `PACKA | Flag | Default | Description | |---|---|---| -| `--target`, `-t VALUE` | auto-detect | Force deployment targets. Comma-separated for multiple (`-t claude,cursor`). Values: `copilot`, `claude`, `grok-build`, `cursor`, `opencode`, `codex`, `gemini`, `antigravity`, `windsurf`, `kiro`, `intellij`, `vscode`, `agent-skills`, `all`; experimental `copilot-cowork`, `copilot-app`, and `grok-cloud` (skills only) are also accepted when enabled. IntelliJ-specific integration is MCP-only and writes JetBrains Copilot's user-scope MCP config; package file primitives use the Copilot profile. `all` excludes `agent-skills`, `antigravity`, `intellij`, and all experimental targets; combine them explicitly to add them, for example `all,intellij`. Explicit MCP target lists are exact: `intellij,claude` writes only those two MCP configs. See the precedence note below. With nothing to detect, install exits `2` with a teaching message. | +| `--target`, `-t VALUE` | auto-detect | Force deployment targets. Comma-separated for multiple (`-t claude,cursor`). Values: `copilot`, `claude`, `grok-build`, `cursor`, `opencode`, `codex`, `gemini`, `antigravity`, `windsurf`, `kiro`, `intellij`, `vscode`, `agent-skills`, `all`; experimental `copilot-cowork`, `copilot-app`, `grok-cloud` (skills only), and `hermes` are also accepted when enabled. IntelliJ-specific integration is MCP-only and writes JetBrains Copilot's user-scope MCP config; package file primitives use the Copilot profile. `all` excludes `agent-skills`, `antigravity`, `intellij`, and all experimental targets; combine them explicitly to add them, for example `all,intellij`. Explicit MCP target lists are exact: `intellij,claude` writes only those two MCP configs. See the precedence note below. With nothing to detect, install exits `2` with a teaching message. | | `--runtime VALUE` | unset | Legacy alias for `--target` (single value only). Still accepted; prefer `--target`. | | `--exclude VALUE` | unset | Skip one runtime from the resolved MCP/LSP target set (explicit selection, manifest, saved config, or auto-detection). | | `--only apm\|mcp` | both | Install only APM packages or only MCP servers. | -| `-g`, `--global` | off | Install to user scope (`~/.apm/`) instead of the current project. MCP servers deploy only to global-capable runtimes, such as Copilot CLI, Claude Code, Codex CLI, Gemini CLI, Antigravity CLI, Kiro, Windsurf, and JetBrains Copilot. | +| `-g`, `--global` | off | Install to user scope (`~/.apm/`) instead of the current project. `apm install -g --mcp NAME` creates or updates `~/.apm/apm.yml`, then deploys only to global-capable runtimes, such as Copilot CLI, Claude Code, Codex CLI, Gemini CLI, Antigravity CLI, Kiro, Windsurf, JetBrains Copilot, and Hermes when enabled. Mixed selections skip workspace-only targets with a warning. A selection with no global-capable target exits `2` before changing the user manifest, lockfile, or runtime configuration. | | `--legacy-skill-paths` | off | Deploy skills to per-client paths (`.cursor/skills/`, `.github/skills/`, ...) instead of the converged `.agents/skills/`. Env: `APM_LEGACY_SKILL_PATHS=1`. | File primitives resolve targets in this order: `--target`, manifest @@ -120,6 +120,7 @@ in `apm.yml`, then run `apm install` again. - **Target persistence on bootstrap.** When `--target` maps to recognized manifest targets, those target(s) are persisted to the new manifest's `targets:` field so a later bare `apm update` redeploys to the same targets without re-specifying `--target`. - **One effective target.** Package primitives, MCP servers, and LSP servers consume one target decision per invocation: `--target` > `apm.yml targets:` > `apm config set target ...` > auto-detect. A saved target therefore applies to `apm install`, `apm install --mcp`, and later `apm update` runs without another flag. - **Required service writes fail loudly.** If MCP or LSP work is declared but no target can be resolved, install exits non-zero before changing the manifest, package deployment, or native service config. A native MCP/LSP config write failure also exits non-zero with the failed target and a permissions/path next step. A successful direct `--mcp` add never reports `Install interrupted`. +- **Direct registry lookup fails closed.** Registry-form MCP entries (`apm install --mcp NAME` with no `--url` and no post-`--` command) resolve one unique registry identity before writing the manifest or user config. An unreachable registry, missing identity, or ambiguous bare server name exits non-zero without changing state. - **Diff-aware.** Packages whose ref or version changed in `apm.yml` are re-downloaded automatically. MCP servers with matching config are skipped (`already configured`); changed config is re-applied (`updated`). - **Transactional replacement.** `--update` and `--refresh` download package replacements to isolated staging paths and validate them before publication. diff --git a/docs/src/content/docs/reference/cli/mcp.md b/docs/src/content/docs/reference/cli/mcp.md index 5173e55694..478cc707d2 100644 --- a/docs/src/content/docs/reference/cli/mcp.md +++ b/docs/src/content/docs/reference/cli/mcp.md @@ -25,8 +25,10 @@ plus a thin install alias. The canonical install path for MCP servers is [`apm install --mcp NAME`](../install/#mcp-server-entry-use-only-with---mcp). It edits `apm.yml`, resolves the registry entry, and writes the resulting -`mcpServers` block to your project. `apm mcp install` is a forwarder -that calls the same code path -- use whichever spelling you prefer. +`mcpServers` block to your project. With `-g` or `--global`, it creates or +updates `~/.apm/apm.yml` and writes only to global-capable runtime +configurations. `apm mcp install` is a forwarder that calls the same code path +-- use whichever spelling you prefer. For an end-to-end consumer walkthrough (declaring an MCP server in `apm.yml`, configuring transport and credentials, deploying to a @@ -103,6 +105,7 @@ list): | `--dev` | Add to `devDependencies`. | | `--dry-run` | Resolve and print without writing `apm.yml`. | | `--force` | Overwrite an existing entry. | +| `-g`, `--global` | Install through `~/.apm/apm.yml` into global-capable runtimes. | | `--no-policy` | Skip policy checks. | | `--verbose`, `-v` | Verbose output. | @@ -113,8 +116,8 @@ list): | `MCP_REGISTRY_URL` | Override the registry endpoint used by `list`, `search`, `show`, and `install`. When set, every command prints a one-line `Registry: ` diagnostic so the override is visible. Unset: the public default registry is used silently. | Network failures against an overridden registry surface an explicit -hint pointing at `MCP_REGISTRY_URL` so misconfigurations are easy to -spot in CI logs. +hint pointing at `MCP_REGISTRY_URL`. Direct `apm install --mcp` lookup +also fails closed against the public default before writing user state. Registry URL resolution order (first set value wins): @@ -123,6 +126,10 @@ Registry URL resolution order (first set value wins): 3. `mcp-registry-url` in `~/.apm/config.json` (set via `apm config set mcp-registry-url`) -- prints `Registry (config): ` diagnostic 4. Built-in public default (silent) +Configured or environment-selected `http://` endpoints require +`MCP_REGISTRY_ALLOW_HTTP=1` when used. An explicit `--registry http://...` flag +is the per-invocation opt-in. + ## Examples Discover and inspect: @@ -138,6 +145,15 @@ Install a stdio server with a runtime command: apm mcp install fetch -- npx -y @modelcontextprotocol/server-fetch ``` +Install the same server at user scope: + +```bash +apm mcp install fetch -g --target claude -- npx -y @modelcontextprotocol/server-fetch +``` + +This creates or updates `~/.apm/apm.yml` and the selected runtime's user +configuration. + Install a remote HTTP server: ```bash diff --git a/docs/src/content/docs/specs/openapm-v0.1.md b/docs/src/content/docs/specs/openapm-v0.1.md index 1291e38ecf..c1e6e616bb 100644 --- a/docs/src/content/docs/specs/openapm-v0.1.md +++ b/docs/src/content/docs/specs/openapm-v0.1.md @@ -136,7 +136,7 @@ between the companion corpus and the implementation. ### 1.3 Document conventions -- OpenAPM v0.1 carries **120 normative statements** indexed in +- OpenAPM v0.1 carries **121 normative statements** indexed in [Appendix C](#appendix-c-index-of-normative-statements). - All on-disk files defined by this specification are **YAML 1.2** parsed under the safe subset defined in @@ -218,8 +218,9 @@ type"), the definition section is cross-linked. | Term | Definition | |---|---| -| **Manifest** | The `apm.yml` file at the root of a package. Defined in [Section 4](#4-manifest-format-apmyml). | -| **Lockfile** | The `apm.lock.yaml` file at the root of a project. Defined in [Section 5](#5-lockfile-format-apmlockyaml). | +| **Manifest** | The `apm.yml` file for one package or installation scope. Defined in [Section 4](#4-manifest-format-apmyml). | +| **Lockfile** | The `apm.lock.yaml` file recording one installation scope's resolved state. Defined in [Section 5](#5-lockfile-format-apmlockyaml). | +| **Installation scope** | The isolated manifest, lockfile, and target-configuration boundary for an install. A project scope is rooted at the consumer project. A user scope is independent of any project root and uses an implementation-defined user location disclosed by the consumer's conformance statement. | | **Policy** | An `apm-policy.yml` file evaluated by a Governance implementation. Defined in [Section 6](#6-policy-format-apm-policyyml). | | **Package** | A unit identified by a manifest (`apm.yml`) or by a recognised package layout (see [Section 8.1](#81-primitive-types)). | | **Primitive** | A typed unit of agent configuration (instruction, prompt, agent, skill, command, hook, or mcp server). Defined in [Section 8.1](#81-primitive-types). | @@ -250,8 +251,9 @@ type"), the definition section is cross-linked. ### 4.1 Document structure and required fields -The manifest is a single YAML 1.2 document located at the project root, -filename `apm.yml`. +The project-scope manifest is a single YAML 1.2 document located at the +project root, filename `apm.yml`. A user-scope manifest, when supported, +uses the user location declared under [req-tg-014](#req-tg-014). **[req-mf-001]** A conforming **producer** implementation MUST emit a @@ -798,10 +800,12 @@ This section's normative statements are: ### 5.1 Top-level structure -The lockfile is a single YAML 1.2 document at the project root, -filename `apm.lock.yaml`. It records the pinned resolved state of -every dependency the consumer has resolved from the manifest, plus -the set of files the consumer itself contributes (the self-entry). +The project-scope lockfile is a single YAML 1.2 document at the project +root, filename `apm.lock.yaml`. A user-scope lockfile, when supported, +uses the user location declared under [req-tg-014](#req-tg-014). A +lockfile records the pinned resolved state of every dependency the +consumer has resolved from the manifest, plus the set of files the +consumer itself contributes (the self-entry). **[req-lk-001]** A conforming **consumer** implementation MUST emit a @@ -2707,6 +2711,37 @@ suffix. > the materialized dependency directory; it does not copy package content into > the host's private plugin state. +#### 8.5.8 User-scoped MCP target selection + + +**[req-tg-014]** A conforming **consumer** implementation that supports a user +installation scope MUST disclose in its conformance statement the user-scope +manifest and lockfile locations and the versioned target-capability declaration +it uses to determine user-scope MCP support. The consumer MUST treat a target +without that declared capability as unsupported. + +When the consumer installs an MCP server into a user scope, it MUST resolve the +effective target selection from the first applicable source in this order: an +explicit target selection; a non-empty user-scope manifest restriction that +does not contain the literal no-restriction sentinel `all`; a configured user +default; then user-scope runtime discovery. Once a source selects one or more +targets, the consumer MUST NOT consult a lower-precedence source. Project-scoped +target-detection signals outside the user scope MUST NOT constrain the discovery +step. A manifest `all` token is treated as no restriction and therefore does +not suppress lower-precedence user-scope defaults or discovery. + +Before creating or modifying the user-scope manifest, lockfile, or target +configuration for the attempted MCP entry, the consumer MUST partition the +selected targets by the declared user-scope MCP capability. If no supported +target remains, it MUST emit an actionable diagnostic and MUST NOT make a +persistent mutation or fall back to discovery. For a mixed set, the supported +subset MUST become the effective target set; the consumer MUST diagnose every +unsupported target, MUST NOT write its target configuration, and MUST NOT fall +back to discovery. If it persists an explicit mixed selection as a user-scope +manifest restriction, it MUST serialize only the supported subset using target +identifiers whose replay selects the same runtimes; it MUST NOT persist an +unsupported member or remap one to a different supported runtime. + ### 8.6 Per-target primitive support (informational) The matrix of which primitive types each target supports is @@ -2724,7 +2759,8 @@ without a spec revision. The current matrix is in the companion [req-tg-008](#req-tg-008), [req-tg-009](#req-tg-009), [req-tg-010](#req-tg-010), [req-tg-011](#req-tg-011), [req-tg-012](#req-tg-012), [req-tg-013](#req-tg-013), - [req-pr-006](#req-pr-006), [req-pr-007](#req-pr-007). + [req-tg-014](#req-tg-014), [req-pr-006](#req-pr-006), + [req-pr-007](#req-pr-007). --- @@ -3333,6 +3369,7 @@ conformance statement identifying: [req-tg-008](#req-tg-008), [req-tg-009](#req-tg-009), [req-tg-010](#req-tg-010), [req-tg-011](#req-tg-011), [req-tg-012](#req-tg-012), [req-tg-013](#req-tg-013), +[req-tg-014](#req-tg-014), [req-sc-001](#req-sc-001), [req-sc-002](#req-sc-002), [req-sc-003](#req-sc-003), [req-sc-004](#req-sc-004), [req-sc-005](#req-sc-005), @@ -3782,6 +3819,7 @@ renumbering of conformance classes. | [req-tg-011](#req-tg-011) | MUST | 8.5.5 | consumer | | [req-tg-012](#req-tg-012) | MUST | 8.5.6 | consumer | | [req-tg-013](#req-tg-013) | MUST | 8.5.7 | consumer | +| [req-tg-014](#req-tg-014) | MUST | 8.5.8 | consumer | | [req-sc-001](#req-sc-001) | MUST | 10.4 | consumer | | [req-sc-002](#req-sc-002) | MUST | 10.9 | consumer | | [req-sc-003](#req-sc-003) | MUST | 10.3 | consumer | @@ -3801,7 +3839,7 @@ renumbering of conformance classes. | [req-cf-001](#req-cf-001) | MUST | 12.5 | consumer | | [req-cf-002](#req-cf-002) | MUST | 12.3 | consumer | -**Total normative statements: 120** (115 MUST, 5 SHOULD). +**Total normative statements: 121** (116 MUST, 5 SHOULD). --- @@ -3849,6 +3887,7 @@ renumbering of conformance classes. | 0.1.36 | 2026-08-29 | Editorial and defensive alignment for [req-tg-011] and [req-tg-013]. Named the [req-tg-008] result as the effective target intersection; scoped aggregate registration and plugin-name claimant selection to dependencies that passed admission; required target contraction to retire consumer-owned native registration; required advisory uninstall, prune, and restore reconciliation to omit ambiguous or changed-owner plugin entries without blocking cleanup; restored exact removal boundaries; defined directory-marketplace entries; and added reserved namespace disclosure to Section 11.2. Added conformance coverage for direct-owner promotion, advisory collision cleanup, and transitive owner-repoint refusal. Statement count remains 119 (114 MUST, 5 SHOULD). | | 0.1.37 | 2026-09-01 | Spec-citation fold for safe full-SHA revision-pin updates (closes #2511 Mode-B silent-extension gate). Added [req-rs-017] (Section 7.7, consumer MUST): a consumer extension may replace a full commit pin only with the peeled commit of the highest eligible non-prerelease annotated tag, including 0.x; no eligible tag retains the current commit and allows unrelated updates to continue; malformed, ambiguous, or failed remote tag resolution stops before manifest or lockfile writes. Revised [req-rs-011], [req-rs-012], and [req-rs-015] for bounded manifest rewrite, scoped operation, advisory tag provenance, and network-free replay. Section 5.2, Section 5.6, Section 7.11, Section 11.3.2, Appendix C, and conformance coverage updated. Statement count: 119 -> 120 (115 MUST, 5 SHOULD). | | 0.1.38 | 2026-09-01 | Defensive amendment of [req-lk-005] (no new normative statement; count remains 120 (115 MUST, 5 SHOULD)): `generated_at` is optional advisory metadata, new lockfiles omit it by default, and later writes preserve an existing omission unless explicitly configured otherwise. | +| 0.1.39 | 2026-09-01 | Spec-citation fold for user-scoped direct MCP target selection (closes #2548 Mode-B silent-extension gate). Added [req-tg-014] (Section 8.5.8, consumer MUST): explicit selection, the user-scope manifest, configured user default, and user-scope runtime discovery form one precedence chain; project-only signals cannot constrain final discovery; and a selected set with no user-capable runtime fails before user manifest, lockfile, or target-config mutation. Section 8.7, Section 11.3.2, and Appendix C updated. Statement count: 120 -> 121 (116 MUST, 5 SHOULD). | Errata (none at publication). diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index b809e9d214..53003dbea6 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -25,6 +25,15 @@ | `apm deps clean` | Clean dependency cache | `--dry-run`, `-y` skip confirm | | `apm deps update [PKGS...]` | Deprecated -- use `apm update` instead (now a strict superset). Update specific packages | `--verbose`, `--force`, `--target` (comma-separated), `--parallel-downloads N`, `-g/--global`, `--legacy-skill-paths` | +For `apm install -g --mcp`, mixed target selections warn and skip +workspace-only runtimes. If no selected target supports user scope, the command +exits `2` before creating or changing the user manifest, lockfile, or runtime +configuration. + +Positional-package dry-runs retain any bootstrapped manifest for inspection. +`apm install -g --mcp ... --dry-run` does not create a user manifest, lockfile, +or runtime configuration. + `apm update`, `apm install --update`, and `apm install --refresh` stage and validate replacement packages before publication. A failed download, validation, or activation leaves the previous package and lockfile active; @@ -277,14 +286,14 @@ To build the marketplace, run `apm pack` (it reads `apm.yml` and writes `.claude | Command | Purpose | Key flags | |---------|---------|-----------| -| `apm mcp install NAME [-- CMD...]` | Add an MCP server (alias for `apm install --mcp`) | `--transport`, `--url`, `--env`, `--header`, `--mcp-version`, `--registry URL`, `--dev`, `--force`, `--dry-run` | +| `apm mcp install NAME [-- CMD...]` | Add an MCP server (alias for `apm install --mcp`) | `-g`/`--global`, `--target`, `--transport`, `--url`, `--env`, `--header`, `--mcp-version`, `--registry URL`, `--dev`, `--force`, `--dry-run` | | `apm mcp list` | List MCP servers in project | `--limit N` | | `apm mcp search QUERY` | Search MCP registry | `--limit N` | | `apm mcp show SERVER` | Show server details | -- | Self-defined stdio MCP entries declared in `apm.yml` (`env:` / `args:`) have their placeholders resolved at install time on Codex, Gemini, Antigravity, and Cursor, which have no runtime interpolation. Copilot CLI preserves env references as `${VAR}`; VS Code and JetBrains preserve them as `${env:VAR}`. All three env syntaxes are accepted: `${VAR}`, `${env:VAR}`, and the legacy ``. Missing variables fall back to an interactive prompt on install-time targets (suppressed in non-TTY contexts). See [Manifest schema -- MCP placeholder syntaxes](https://microsoft.github.io/apm/reference/manifest-schema/) for the per-target matrix. -Set `MCP_REGISTRY_URL` (default `https://api.mcp.github.com`) to point all `apm mcp` commands and `apm install --mcp` at a custom MCP registry. The URL is validated at startup and must use `https://`; set `MCP_REGISTRY_ALLOW_HTTP=1` to opt in to plaintext `http://` for development. The registry must implement the [MCP Registry v0.1 spec](https://github.com/modelcontextprotocol/registry) (apm calls `/v0.1/servers/...`); legacy `/v0/`-only registries will return 404. When the override is set and the registry is unreachable during install pre-flight, APM fails closed. +Set `MCP_REGISTRY_URL` (default `https://api.mcp.github.com`) to point all `apm mcp` commands and `apm install --mcp` at a custom MCP registry. The URL is validated at startup and must use `https://`; set `MCP_REGISTRY_ALLOW_HTTP=1` to opt in to plaintext `http://` for development. The registry must implement the [MCP Registry v0.1 spec](https://github.com/modelcontextprotocol/registry) (apm calls `/v0.1/servers/...`); legacy `/v0/`-only registries will return 404. Direct install pre-flight fails closed when any registry is unreachable and accepts a bare server name only when it has one unique registry match. For a v0.1 package with `registryType: oci`, install generates a `docker` launcher automatically. Docker run options stay before the image and @@ -368,7 +377,7 @@ Experimental flags MUST NOT gate security-critical behaviour (content scanning, `apm config set external..llm true|false` and `apm config set external..args -- ""` persist per-scanner external-scanner defaults to `~/.apm/config.json` (JSON section `external_scanners..{llm,args}`), behind `apm experimental enable external-scanners`. `` is validated against the supported scanners (e.g. `skillspector`). `.args` is shlex-split and stored as a list; use the `--` separator so Click does not parse a leading `--flag` as an option. `apm config get external..{llm,args}`, `apm config unset external..{llm,args}`, and `apm config unset external.` (removes both) round out the surface. These keys are reachable only when the flag is enabled; bare `apm config get` lists set external keys when the flag is on. CLI flags (`--external-llm`, `--external-args`) override these values for a single run. -`apm config set mcp-registry-url https://mcp.internal.example.com` persists a private MCP registry URL so users do not need to export `MCP_REGISTRY_URL` every session. Accepts `http://` or `https://` URLs; all other schemes are rejected. Resolution order: `--registry ` flag on `apm mcp install` / `apm install --mcp` > `MCP_REGISTRY_URL` env var > `mcp-registry-url` in `~/.apm/config.json` > built-in public default. When the config layer is active, `apm mcp search` prints a `Registry (config): ` diagnostic. `apm config unset mcp-registry-url` removes the persisted URL. +`apm config set mcp-registry-url https://mcp.internal.example.com` persists a private MCP registry URL so users do not need to export `MCP_REGISTRY_URL` every session. Accepts `http://` or `https://` URLs; all other schemes are rejected. A configured `http://` endpoint also requires `MCP_REGISTRY_ALLOW_HTTP=1` when used; an explicit `--registry http://...` flag is the per-invocation opt-in. Resolution order: `--registry ` flag on `apm mcp install` / `apm install --mcp` > `MCP_REGISTRY_URL` env var > `mcp-registry-url` in `~/.apm/config.json` > built-in public default. When the config layer is active, `apm mcp search` prints a `Registry (config): ` diagnostic. `apm config unset mcp-registry-url` removes the persisted URL. `apm approve [PACKAGE_REF...]` grants a package permission to deploy executable primitives (hooks, `bin/`, self-defined MCP servers, canvas extensions). By default it writes the project `apm.yml` `executables.allow` block -- committed to source control, so the whole team inherits the trust decision. `apm approve --user` records a personal grant in `~/.apm/config.json` instead (machine-local, never committed, lowest authority -- it can only narrow trust). diff --git a/scripts/architecture_linter/checks/install_deployment_analyzers.py b/scripts/architecture_linter/checks/install_deployment_analyzers.py index 1f3b53ca4c..421848c8aa 100644 --- a/scripts/architecture_linter/checks/install_deployment_analyzers.py +++ b/scripts/architecture_linter/checks/install_deployment_analyzers.py @@ -42,9 +42,11 @@ ) from scripts.architecture_linter.checks.install_policy_intent import EXTRA_RULES from scripts.architecture_linter.checks.install_request_and_source import ( + _GUARD_INSTALL_SCOPE, _GUARD_OUTCOME, _GUARD_REQUEST_DEFAULTS, _GUARD_SOURCE_PLAN, + check_install_scope_selection, check_outcome, check_request_defaults, check_source_plan, @@ -108,6 +110,11 @@ "Install invocation option defaults stay owned by install/request.py.", check_request_defaults, ), + _rule( + _GUARD_INSTALL_SCOPE, + "Direct MCP installs consume the install command's single scope decision.", + check_install_scope_selection, + ), _rule( _GUARD_BASE_INTEGRATOR, "File-level deploy/sync/cleanup stays owned by BaseIntegrator.", diff --git a/scripts/architecture_linter/checks/install_request_and_source.py b/scripts/architecture_linter/checks/install_request_and_source.py index 0155c08a3c..f667979e50 100644 --- a/scripts/architecture_linter/checks/install_request_and_source.py +++ b/scripts/architecture_linter/checks/install_request_and_source.py @@ -1,15 +1,18 @@ -"""Request-default, source-plan, and outcome-routing install analyzers. +"""Request-default, scope-selection, source-plan, and outcome install analyzers. -Ports three owner guards recorded in +Ports four owner guards recorded in ``.apm/architecture/owners/install-deployment.json``: -``install-deployment-request-defaults``, ``install-deployment-source-plan``, -and ``install-deployment-outcome``. RULES assembly (with the guard-id -constants and the ``_rule`` factory) lives in the thin catalog module -:mod:`install_deployment_analyzers`, which imports each check function below. +``install-deployment-request-defaults``, +``install-deployment-install-scope-selection``, +``install-deployment-source-plan``, and ``install-deployment-outcome``. +RULES assembly (with the guard-id constants and the ``_rule`` factory) lives in +the thin catalog module :mod:`install_deployment_analyzers`, which imports each +check function below. """ from __future__ import annotations +import ast import re from scripts.architecture_linter.checks.install_deployment_shared import ( @@ -37,7 +40,11 @@ _GUARD_REQUEST_DEFAULTS = "install-deployment-request-defaults" +_GUARD_INSTALL_SCOPE = "install-deployment-install-scope-selection" + + _REQUEST_OWNER = "src/apm_cli/install/request.py" +_MCP_COMMAND = "src/apm_cli/install/mcp/command.py" _ALLOWED_WRAPPER_DEFAULTS = frozenset({"update_refs", "verbose", "only_packages"}) @@ -89,6 +96,221 @@ def check_request_defaults(provider: FactsProvider) -> tuple[Violation, ...]: return () +_MCP_CONFLICTS = "src/apm_cli/install/mcp/conflicts.py" + + +def _named_calls(nodes: tuple[ast.AST, ...], name: str) -> list[ast.Call]: + """Return direct name calls from one precomputed function scope.""" + return [ + node + for node in nodes + if isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id == name + ] + + +def _attribute_calls(nodes: tuple[ast.AST, ...], name: str) -> list[ast.Call]: + """Return attribute calls with the requested method name.""" + return [ + node + for node in nodes + if isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == name + ] + + +def _has_name_keyword(call: ast.Call, keyword: str, name: str) -> bool: + """Return whether a call forwards one keyword from the named local.""" + return any( + item.arg == keyword and isinstance(item.value, ast.Name) and item.value.id == name + for item in call.keywords + ) + + +def _has_scope_projection_keyword(call: ast.Call, keyword: str) -> bool: + """Return whether a keyword projects user scope through the canonical helper.""" + for item in call.keywords: + value = item.value + if ( + item.arg == keyword + and isinstance(value, ast.Call) + and isinstance(value.func, ast.Name) + and value.func.id == "is_user_scope" + and len(value.args) == 1 + and isinstance(value.args[0], ast.Name) + and value.args[0].id == "scope" + ): + return True + return False + + +def _canonical_scope_assignment(node: ast.Assign) -> bool: + """Return whether an assignment owns the global-to-install-scope mapping.""" + value = node.value + return ( + len(node.targets) == 1 + and isinstance(node.targets[0], ast.Name) + and node.targets[0].id == "scope" + and isinstance(value, ast.IfExp) + and isinstance(value.test, ast.Name) + and value.test.id == "global_" + and isinstance(value.body, ast.Attribute) + and isinstance(value.body.value, ast.Name) + and value.body.value.id == "InstallScope" + and value.body.attr == "USER" + and isinstance(value.orelse, ast.Attribute) + and isinstance(value.orelse.value, ast.Name) + and value.orelse.value.id == "InstallScope" + and value.orelse.attr == "PROJECT" + ) + + +def _takes_argument(function: ast.FunctionDef | ast.AsyncFunctionDef, name: str) -> bool: + """Return whether a function declares the named positional or keyword argument.""" + arguments = ( + *function.args.posonlyargs, + *function.args.args, + *function.args.kwonlyargs, + ) + return any(argument.arg == name for argument in arguments) + + +def _takes_scope(call: ast.Call) -> bool: + """Return whether a scope helper consumes the canonical local value.""" + return ( + len(call.args) == 1 + and isinstance(call.args[0], ast.Name) + and call.args[0].id == "scope" + and not call.keywords + ) + + +def _takes_deploy_root(call: ast.Call) -> bool: + """Return whether discovery consumes the canonical deploy-root projection.""" + if len(call.args) != 1: + return False + value = call.args[0] + return ( + isinstance(value, ast.Call) + and isinstance(value.func, ast.Name) + and value.func.id == "get_deploy_root" + and len(value.args) == 1 + and isinstance(value.args[0], ast.Name) + and value.args[0].id == "scope" + and _has_name_keyword(call, "exclude", "exclude") + ) + + +def check_install_scope_selection(provider: FactsProvider) -> tuple[Violation, ...]: + """Direct MCP installs must consume the install command's scope decision.""" + rule_id = _GUARD_INSTALL_SCOPE + _adapter, adapter_fail = _facts_for(provider, _INSTALL_ADAPTER, rule_id) + _conflicts, conflicts_fail = _facts_for(provider, _MCP_CONFLICTS, rule_id) + _command, command_fail = _facts_for(provider, _MCP_COMMAND, rule_id) + failures = list(adapter_fail) + list(conflicts_fail) + list(command_fail) + if failures: + return tuple(failures) + + adapter_index = provider.tree_index(_INSTALL_ADAPTER) + conflicts_index = provider.tree_index(_MCP_CONFLICTS) + command_index = provider.tree_index(_MCP_COMMAND) + if adapter_index is None or conflicts_index is None or command_index is None: + return ( + _summary( + rule_id, + _INSTALL_ADAPTER, + "Direct MCP installs must consume the install command scope", + ), + ) + + install = adapter_index.function("install") + handler = adapter_index.function("_handle_mcp_install") + validator = conflicts_index.function("validate_mcp_conflicts") + command = command_index.function("run_mcp_install") + if ( + not isinstance(install, (ast.FunctionDef, ast.AsyncFunctionDef)) + or not isinstance(handler, (ast.FunctionDef, ast.AsyncFunctionDef)) + or not isinstance(validator, (ast.FunctionDef, ast.AsyncFunctionDef)) + or not isinstance(command, (ast.FunctionDef, ast.AsyncFunctionDef)) + ): + return ( + _summary( + rule_id, + _INSTALL_ADAPTER, + "Direct MCP installs must consume the install command scope", + ), + ) + + install_nodes = adapter_index.own_scope(install) + handler_nodes = adapter_index.own_scope(handler) + command_nodes = command_index.own_scope(command) + assignments = [ + node + for node in install_nodes + if isinstance(node, ast.Assign) + and any(isinstance(target, ast.Name) and target.id == "scope" for target in node.targets) + ] + handler_calls = _named_calls(install_nodes, "_handle_mcp_install") + run_calls = _named_calls(handler_nodes, "_run_mcp_install") + target_calls = _named_calls(handler_nodes, "resolve_manifest_target_decision") + scope_partition_calls = _named_calls(handler_nodes, "partition_user_scope_runtimes") + scope_discovery_calls = _named_calls(handler_nodes, "discover_user_scope_mcp_runtimes") + exclusion_calls = _named_calls(handler_nodes, "filter_excluded_mcp_runtimes") + dry_run_validation_calls = _named_calls(handler_nodes, "_validate_mcp_dry_run_entry") + registry_validation_calls = _attribute_calls(command_nodes, "prevalidate_registry_dependencies") + bootstrap_calls = _named_calls(command_nodes, "_create_minimal_apm_yml") + manifest_write_calls = _named_calls(command_nodes, "add_mcp_to_apm_yml") + manifest_calls = _named_calls(handler_nodes, "get_manifest_path") + apm_dir_calls = _named_calls(handler_nodes, "get_apm_dir") + validator_args = ( + *validator.args.posonlyargs, + *validator.args.args, + *validator.args.kwonlyargs, + ) + + valid = ( + len(assignments) == 1 + and _canonical_scope_assignment(assignments[0]) + and _takes_argument(handler, "scope") + and len(handler_calls) == 1 + and _has_name_keyword(handler_calls[0], "scope", "scope") + and len(run_calls) == 1 + and _has_name_keyword(run_calls[0], "scope", "scope") + and _has_name_keyword(run_calls[0], "initial_manifest_config", "initial_manifest_config") + and len(target_calls) == 1 + and _has_scope_projection_keyword(target_calls[0], "user_scope") + and len(scope_partition_calls) == 1 + and len(scope_discovery_calls) == 1 + and _takes_deploy_root(scope_discovery_calls[0]) + and len(exclusion_calls) == 1 + and len(registry_validation_calls) == 1 + and len(bootstrap_calls) == 1 + and len(manifest_write_calls) == 1 + and len(dry_run_validation_calls) == 1 + and target_calls[0].lineno < run_calls[0].lineno + and scope_partition_calls[0].lineno < run_calls[0].lineno + and scope_discovery_calls[0].lineno < run_calls[0].lineno + and exclusion_calls[0].lineno < run_calls[0].lineno + and dry_run_validation_calls[0].lineno < run_calls[0].lineno + and registry_validation_calls[0].lineno < bootstrap_calls[0].lineno + and bootstrap_calls[0].lineno < manifest_write_calls[0].lineno + and len(manifest_calls) == 1 + and _takes_scope(manifest_calls[0]) + and len(apm_dir_calls) == 1 + and _takes_scope(apm_dir_calls[0]) + and all(argument.arg != "global_" for argument in validator_args) + ) + if valid: + return () + return ( + _summary( + rule_id, + _INSTALL_ADAPTER, + "Direct MCP installs must consume the install command scope", + ), + ) + + _SOURCE_PLAN_OWNER = "src/apm_cli/install/deployable_source_plan.py" diff --git a/src/apm_cli/commands/config.py b/src/apm_cli/commands/config.py index 29580462d2..fc401286ef 100644 --- a/src/apm_cli/commands/config.py +++ b/src/apm_cli/commands/config.py @@ -492,7 +492,12 @@ def set(key, value): # noqa: F811 try: set_mcp_registry_url(value) - logger.success(f"MCP registry URL set to: {get_mcp_registry_url()}") + configured_url = get_mcp_registry_url() + logger.success(f"MCP registry URL set to: {configured_url}") + if configured_url and configured_url.startswith("http://"): + logger.info( + "Set MCP_REGISTRY_ALLOW_HTTP=1 when using this trusted plaintext endpoint." + ) except ValueError as exc: logger.error(str(exc)) sys.exit(1) diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index 9185b1e9ea..5f0daa7d65 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -119,10 +119,17 @@ ) from ..core.target_catalog import target_help_fragment from ..core.target_detection import TargetParamType, manifest_targets_from_target_option +from ..install.mcp.args import parse_env_pairs as _parse_mcp_env_pairs +from ..install.mcp.args import parse_header_pairs as _parse_mcp_header_pairs # MCP --mcp helpers (module-level re-exports for test patches); must stay at # import time per comments in the original mid-file block. -from ..install.mcp.command import run_mcp_install as _run_mcp_install +from ..install.mcp.command import ( + run_mcp_install as _run_mcp_install, +) +from ..install.mcp.command import ( + run_mcp_policy_preflight as _run_mcp_policy_preflight, +) from ..install.mcp.conflicts import ( validate_mcp_conflicts as _validate_mcp_conflicts, ) @@ -739,13 +746,6 @@ def _validate_and_add_packages_to_apm_yml( # per LOC budget. Re-bind module-level names for back-compat with tests # that still patch ``apm_cli.commands.install._warn_*``. -# MCP registry / dry-run helpers are imported at module top (see -# ``..install.mcp.*`` imports above) so test patches keep working. - -# --------------------------------------------------------------------------- -# install() decomposition: extracted flow helpers -# --------------------------------------------------------------------------- - def _handle_mcp_install( # noqa: PLR0913 *, @@ -765,91 +765,92 @@ def _handle_mcp_install( # noqa: PLR0913 logger, no_policy, validated_registry_url, + scope, ): - """Execute the ``--mcp`` install path (MCP server add). + """Resolve and execute the direct ``--mcp`` install path.""" + from ..core.scope import get_apm_dir, get_deploy_root, get_manifest_path, is_user_scope - Resolves registry URL, runs policy preflight, handles dry-run, - and delegates to :func:`_run_mcp_install` for the actual installation. - Called from :func:`install` when ``--mcp`` is specified; the caller - returns immediately after this function completes. - """ - from ..core.scope import ( - InstallScope, - get_apm_dir, - get_manifest_path, - ) - - # Apply CLI > env > default precedence; emit override diagnostic. - resolved_registry_url, _registry_source = _resolve_registry_url( + resolved_registry_url, registry_source = _resolve_registry_url( validated_registry_url, logger=logger, ) - mcp_scope = InstallScope.PROJECT - mcp_manifest_path = get_manifest_path(mcp_scope) - mcp_apm_dir = get_apm_dir(mcp_scope) + integration_registry_url = resolved_registry_url + mcp_manifest_path = get_manifest_path(scope) + mcp_apm_dir = get_apm_dir(scope) from ..core.target_detection import resolve_manifest_target_decision target_decision = resolve_manifest_target_decision( Path.cwd(), manifest_path=mcp_manifest_path, explicit_target=target or runtime, + user_scope=is_user_scope(scope), ) + if is_user_scope(scope): + from ..core.target_detection import EffectiveTargetDecision + from ..integration.mcp_integrator_install import ( + discover_user_scope_mcp_runtimes, + filter_excluded_mcp_runtimes, + partition_user_scope_runtimes, + unavailable_user_scope_targets_message, + ) - # -- W2-mcp-preflight: policy enforcement before MCP install -- - # Build a lightweight MCPDependency for policy evaluation. - # This mirrors _build_mcp_entry routing but we only need the - # fields that policy checks inspect (name, transport, registry). - from ..models.dependency.mcp import MCPDependency as _MCPDep - from ..policy.install_preflight import ( - PolicyBlockError, - run_policy_preflight, - ) - - _is_self_defined = bool(url or command_argv) - _preflight_transport = transport - if _preflight_transport is None: - if command_argv: - _preflight_transport = "stdio" - elif url: - _preflight_transport = "http" - _preflight_dep = _MCPDep( - name=mcp_name, - transport=_preflight_transport, - registry=False if _is_self_defined else None, + scoped_runtime_targets = target_decision.runtime_targets_for_scope(user_scope=True) + if scoped_runtime_targets is None: + supported_runtimes, skipped_runtimes = discover_user_scope_mcp_runtimes( + get_deploy_root(scope), exclude=exclude + ) + else: + scoped_runtime_targets = filter_excluded_mcp_runtimes( + list(scoped_runtime_targets), exclude + ) + supported_runtimes, skipped_runtimes = partition_user_scope_runtimes( + list(scoped_runtime_targets) + ) + if skipped_runtimes and supported_runtimes: + logger.warning( + "Skipped workspace-only runtimes at user scope: " + f"{', '.join(sorted(skipped_runtimes))} -- omit --global to install these" + ) + if not supported_runtimes: + if exclude: + raise click.UsageError( + f"All selected MCP runtimes were removed by --exclude {exclude}; " + "choose another target or remove the exclusion" + ) + raise click.UsageError( + unavailable_user_scope_targets_message( + target_decision, scoped_runtime_targets, skipped_runtimes + ) + ) + target_decision = EffectiveTargetDecision(supported_runtimes, target_decision.source) + _run_mcp_policy_preflight( + mcp_name=mcp_name, + transport=transport, url=url, + command_argv=command_argv, + no_policy=no_policy, + logger=logger, + target_decision=target_decision, ) - from ..core.target_detection import normalize_policy_targets - - policy_targets = normalize_policy_targets(target_decision.value) - - try: - _pf_result, _pf_active = run_policy_preflight( - project_root=Path.cwd(), - mcp_deps=[_preflight_dep], - no_policy=no_policy, - logger=logger, - dry_run=logger.dry_run, - effective_target=policy_targets, - ) - except PolicyBlockError: - # Diagnostics already emitted by the helper + logger. - logger.render_summary() - sys.exit(1) if logger.dry_run: - # C1: validate eagerly so dry-run rejects what real install would. _validate_mcp_dry_run_entry( mcp_name, transport=transport, url=url, - env=env_pairs, - headers=header_pairs, + env=_parse_mcp_env_pairs(env_pairs), + headers=_parse_mcp_header_pairs(header_pairs), version=mcp_version, command_argv=command_argv, registry_url=resolved_registry_url, ) - logger.dry_run_notice(f"would add MCP server '{mcp_name}' to {mcp_manifest_path}") - return + + initial_manifest_config = None + if is_user_scope(scope) and not mcp_manifest_path.exists(): + project_name = _resolve_bootstrap_project_name(Path.home().name) + initial_manifest_config = _get_default_config(project_name) + if target is not None or runtime is not None: + initial_manifest_config["targets"] = supported_runtimes _run_mcp_install( mcp_name=mcp_name, transport=transport, @@ -866,8 +867,11 @@ def _handle_mcp_install( # noqa: PLR0913 exclude=exclude, logger=logger, apm_dir=mcp_apm_dir, - scope=mcp_scope, - registry_url=validated_registry_url, + scope=scope, + registry_url=integration_registry_url, + registry_allow_http=registry_source == "flag", + registry_source=registry_source, + initial_manifest_config=initial_manifest_config, ) @@ -965,7 +969,7 @@ def _handle_mcp_install( # noqa: PLR0913 "global_", is_flag=True, default=False, - help="Install to user scope (~/.apm/) instead of the current project. MCP servers target global-capable runtimes only (Copilot CLI, Claude Code, Codex CLI, Gemini CLI, Antigravity CLI, Kiro, Windsurf, JetBrains Copilot).", + help="Install to user scope (~/.apm/) instead of the current project. Direct MCP installs create or update ~/.apm/apm.yml. Mixed selections warn and skip workspace-only runtimes; selections with no global-capable runtime exit 2 before changing the user manifest, lockfile, or runtime configuration. Supported runtimes include Copilot CLI, Claude Code, Codex CLI, Gemini CLI, Antigravity CLI, Kiro, Windsurf, JetBrains Copilot, and Hermes when enabled.", ) @click.option( "--ssh", @@ -1365,6 +1369,9 @@ def install( # noqa: C901, PLR0913 # Validate --registry (raises UsageError on a bad URL). validated_registry_url = _validate_registry_url(registry_url) + from ..core.scope import InstallScope + + scope = InstallScope.USER if global_ else InstallScope.PROJECT _validate_mcp_conflicts( mcp_name=mcp_name, @@ -1376,7 +1383,6 @@ def install( # noqa: C901, PLR0913 headers=header_pairs, mcp_version=mcp_version, command_argv=command_argv, - global_=global_, only=only, update=update, any_transport_flag=use_ssh or use_https or allow_protocol_fallback, @@ -1405,6 +1411,7 @@ def install( # noqa: C901, PLR0913 logger=logger, no_policy=no_policy, validated_registry_url=validated_registry_url, + scope=scope, ) summary_rendered = True return @@ -1435,7 +1442,6 @@ def install( # noqa: C901, PLR0913 # Resolve scope from ..core.scope import ( - InstallScope, ensure_user_dirs, get_apm_dir, get_manifest_path, @@ -1443,8 +1449,6 @@ def install( # noqa: C901, PLR0913 warn_unsupported_user_scope, ) - scope = InstallScope.USER if global_ else InstallScope.PROJECT - if scope is InstallScope.USER: ensure_user_dirs() logger.progress("Installing to user scope (~/.apm/)") @@ -1717,12 +1721,10 @@ def _install_apm_packages(ctx, outcome): Parses ``apm.yml``, installs APM dependencies, collects and installs transitive MCP servers, and handles lockfile updates. - Args: ctx: :class:`InstallContext` with configuration and environment. outcome: ``_ValidationOutcome`` from package validation (may be ``None`` when no explicit packages were passed). - Returns: Tuple of ``(apm_count, mcp_count, lsp_count, apm_diagnostics)``. """ diff --git a/src/apm_cli/config.py b/src/apm_cli/config.py index 89cb50347c..3b3e94911b 100644 --- a/src/apm_cli/config.py +++ b/src/apm_cli/config.py @@ -761,9 +761,12 @@ def _validate_mcp_registry_url(url: str) -> str: f"({len(normalized)} > {_MCP_REGISTRY_URL_MAX_LENGTH} characters)" ) parsed = urlparse(normalized) + from apm_cli.install.mcp.registry import _redact_url_credentials + + safe_url = _redact_url_credentials(normalized) if not parsed.scheme: raise ValueError( - f"mcp-registry-url: Invalid URL '{normalized}': expected scheme://host " + f"mcp-registry-url: Invalid URL '{safe_url}': expected scheme://host " f"(e.g. https://mcp.internal.example.com)" ) scheme = parsed.scheme.lower() @@ -774,13 +777,16 @@ def _validate_mcp_registry_url(url: str) -> str: f"WebSocket URLs (ws/wss) and file:// paths are rejected for security." ) if parsed.username is not None: - raise ValueError( - "mcp-registry-url: URL must not contain credentials; " - "use the MCP_REGISTRY_URL environment variable or a credential helper instead." - ) + raise ValueError("mcp-registry-url: embedded credentials are not supported") + try: + _ = parsed.port + except ValueError as exc: + raise ValueError("mcp-registry-url: URL has an invalid port") from exc + if parsed.query or parsed.fragment: + raise ValueError("mcp-registry-url: base URL must not contain a query or fragment") if not parsed.hostname: raise ValueError( - f"mcp-registry-url: Invalid URL '{normalized}': expected scheme://host " + f"mcp-registry-url: Invalid URL '{safe_url}': expected scheme://host " f"(e.g. https://mcp.internal.example.com)" ) return normalized diff --git a/src/apm_cli/core/target_detection.py b/src/apm_cli/core/target_detection.py index 535b10e298..f70e56d627 100644 --- a/src/apm_cli/core/target_detection.py +++ b/src/apm_cli/core/target_detection.py @@ -872,8 +872,19 @@ def runtime_targets(self) -> tuple[str, ...] | None: def runtime_targets_for_scope(self, *, user_scope: bool) -> tuple[str, ...] | None: """Return MCP runtime identifiers adjusted for project or user scope.""" - del user_scope - return self.runtime_targets + targets = self.runtime_targets + if not user_scope or targets is None: + return targets + + from apm_cli.core.experimental import is_enabled + + eligible: list[str] = [] + for runtime in targets: + capability = get_target_capability(runtime) + if capability.experimental_flag and not is_enabled(capability.experimental_flag): + continue + eligible.append(runtime) + return tuple(eligible) @cached_property def runtime_equivalents(self) -> tuple[str, ...] | None: @@ -964,6 +975,7 @@ def resolve_manifest_target_decision( *, manifest_path: Path, explicit_target: str | list[str] | None, + user_scope: bool = False, ) -> EffectiveTargetDecision: """Resolve one effective target decision from an optional manifest path.""" package = None @@ -975,6 +987,7 @@ def resolve_manifest_target_decision( project_root, package=package, explicit_target=explicit_target, + user_scope=user_scope, ) diff --git a/src/apm_cli/install/mcp/command.py b/src/apm_cli/install/mcp/command.py index 1dc554c5da..17e8e9fa53 100644 --- a/src/apm_cli/install/mcp/command.py +++ b/src/apm_cli/install/mcp/command.py @@ -19,6 +19,7 @@ from ..errors import InstallFailureAlreadyRendered if TYPE_CHECKING: + from apm_cli.core.scope import InstallScope from apm_cli.core.target_detection import EffectiveTargetDecision from .args import parse_env_pairs, parse_header_pairs @@ -41,6 +42,47 @@ pass +def run_mcp_policy_preflight( + *, + mcp_name: str, + transport: str | None, + url: str | None, + command_argv: Sequence[str] | None, + no_policy: bool, + logger, + target_decision: EffectiveTargetDecision, +) -> None: + """Run policy checks for a direct MCP install before any persistent write.""" + from ...core.target_detection import normalize_policy_targets + from ...models.dependency.mcp import MCPDependency + from ...policy.install_preflight import PolicyBlockError, run_policy_preflight + + resolved_transport = transport + if resolved_transport is None: + if command_argv: + resolved_transport = "stdio" + elif url: + resolved_transport = "http" + dependency = MCPDependency( + name=mcp_name, + transport=resolved_transport, + registry=False if url or command_argv else None, + url=url, + ) + try: + run_policy_preflight( + project_root=Path.cwd(), + mcp_deps=[dependency], + no_policy=no_policy, + logger=logger, + dry_run=logger.dry_run, + effective_target=normalize_policy_targets(target_decision.value), + ) + except PolicyBlockError: + logger.render_summary() + raise click.exceptions.Exit(1) from None + + def run_mcp_install( # noqa: PLR0913 *, mcp_name: str, @@ -56,10 +98,13 @@ def run_mcp_install( # noqa: PLR0913 exclude: str | None, logger, apm_dir: Path, - scope: str | None, + scope: InstallScope | None, target: str | list[str] | None = None, target_decision: EffectiveTargetDecision | None = None, registry_url: str | None = None, + registry_allow_http: bool = True, + registry_source: str | None = None, + initial_manifest_config: dict | None = None, ) -> None: """Execute the --mcp install path. ``registry_url`` is the validated --registry value; the caller resolved precedence vs MCP_REGISTRY_URL. @@ -76,7 +121,7 @@ def run_mcp_install( # noqa: PLR0913 # Build entry (validates through MCPDependency). Convert ValueError # to UsageError so the CLI exits 2 with the model wording. try: - entry, _is_self_defined = build_mcp_entry( + entry, is_self_defined = build_mcp_entry( mcp_name, transport=transport, url=url, @@ -97,7 +142,57 @@ def run_mcp_install( # noqa: PLR0913 stdio_command = command_argv[0] if command_argv else None warn_shell_metachars(env, logger, command=stdio_command) - # Write to apm.yml. + # Build MCPDependency for install. ``entry`` may be a bare string. + if isinstance(entry, str): + dep = MCPDependency.from_string(entry) + else: + dep = MCPDependency.from_dict(entry) + + prevalidated_registry_servers = None + if not is_self_defined: + if not APM_DEPS_AVAILABLE: + raise click.ClickException( + f"MCP registry validation is unavailable for '{mcp_name}'. " + "Install the package dependencies and retry; no state was changed" + ) + with registry_env_override( + registry_url, + allow_http=registry_allow_http, + source=registry_source, + ): + try: + prevalidated_registry_servers = MCPIntegrator.prevalidate_registry_dependencies( + [dep], + registry_url=registry_url, + registry_source=registry_source, + verbose=verbose, + logger=logger, + ) + except Exception as exc: + detail = str(exc) + if detail.startswith("Could not reach MCP registry"): + logger.error(f"{detail} No state was changed.") + raise InstallFailureAlreadyRendered(detail) from None + if detail.startswith("Cannot install ") and "missing server" in detail: + raise InstallFailureAlreadyRendered(detail) from None + logger.verbose_detail(f"MCP registry validation error: {detail}") + raise click.ClickException( + f"MCP registry validation failed for '{mcp_name}'. Check the server " + "name and registry reachability/configuration, then retry; " + "no state was changed" + ) from None + + if getattr(logger, "dry_run", False) is True: + logger.dry_run_notice(f"would add MCP server '{mcp_name}' to {manifest_path}") + return + + if initial_manifest_config is not None: + from ...commands._helpers import _create_minimal_apm_yml + + apm_dir.mkdir(parents=True, exist_ok=True) + _create_minimal_apm_yml(initial_manifest_config, target_path=manifest_path) + logger.success(f"Created {manifest_path}") + status, _diff = add_mcp_to_apm_yml( mcp_name, entry, @@ -106,17 +201,8 @@ def run_mcp_install( # noqa: PLR0913 manifest_path=manifest_path, logger=logger, ) - if status == "skipped": logger.progress(f"MCP server '{mcp_name}' already declared; checking integrations") - # Fall through intentionally: unchanged entries still need legacy - # IntelliJ migration and per-target ownership repair. - - # Build MCPDependency for install. ``entry`` may be a bare string. - if isinstance(entry, str): - dep = MCPDependency.from_string(entry) - else: - dep = MCPDependency.from_dict(entry) # Install just this MCP via the integrator and update lockfile. # ``registry_env_override`` exports MCP_REGISTRY_URL for THIS call so @@ -128,7 +214,11 @@ def run_mcp_install( # noqa: PLR0913 if target is not None and logger: rendered_target = target if isinstance(target, str) else ", ".join(target) logger.verbose_detail(f"Target: {rendered_target}") - with registry_env_override(registry_url): + with registry_env_override( + registry_url, + allow_http=registry_allow_http, + source=registry_source, + ): try: # Migrate before creating apm.lock.yaml so legacy state is not shadowed. migrate_lockfile_if_needed(apm_dir) @@ -182,6 +272,7 @@ def run_mcp_install( # noqa: PLR0913 project_root=apm_dir, user_scope=user_scope, managed_target_servers=requested_target_servers, + prevalidated_registry_servers=prevalidated_registry_servers, ) new_names = MCPIntegrator.get_server_names([dep]) new_configs = MCPIntegrator.get_server_configs([dep]) diff --git a/src/apm_cli/install/mcp/conflicts.py b/src/apm_cli/install/mcp/conflicts.py index b677500589..c69686915c 100644 --- a/src/apm_cli/install/mcp/conflicts.py +++ b/src/apm_cli/install/mcp/conflicts.py @@ -1,4 +1,4 @@ -"""MCP CLI flag-conflict matrix (E1-E15). +"""MCP CLI flag-conflict matrix. Extracted from ``commands/install.py`` per the architecture-invariants LOC budget. ``validate_mcp_conflicts`` is the single chokepoint that @@ -34,13 +34,12 @@ def validate_mcp_conflicts( headers: Mapping[str, str], mcp_version: str | None, command_argv: Sequence[str] | None, - global_: bool, only: str | None, update: bool, any_transport_flag: bool, registry_url: str | None = None, ) -> None: - """Apply conflict matrix E1-E15. Raises ``click.UsageError`` on hit. + """Apply the conflict matrix. Raises ``click.UsageError`` on a conflict. ``any_transport_flag`` should be ``use_ssh or use_https or allow_protocol_fallback`` (pre-evaluated by the caller). @@ -74,12 +73,6 @@ def validate_mcp_conflicts( if pre_dash_packages: raise click.UsageError("cannot mix --mcp with positional packages") - # E2: --global not supported for MCP entries. - if global_: - raise click.UsageError( - "MCP servers are project-scoped; --global is not supported for MCP entries" - ) - # E3: --only apm conflicts with --mcp. if only == "apm": raise click.UsageError("cannot use --only apm with --mcp") diff --git a/src/apm_cli/install/mcp/registry.py b/src/apm_cli/install/mcp/registry.py index 8815c391f7..b745d320a9 100644 --- a/src/apm_cli/install/mcp/registry.py +++ b/src/apm_cli/install/mcp/registry.py @@ -35,27 +35,33 @@ def _redact_url_credentials(url: str) -> str: - """Strip ``user:password@`` from a URL before logging it. + """Redact URL userinfo, query, and fragment data before logging. - Registry URLs may legitimately carry credentials for private mirrors - (``https://user:token@registry.internal/``); we accept them at the - flag layer but never echo them back to the terminal where they could - leak via shell history, CI logs, or screenshots. + Potentially credential-bearing input is redacted before diagnostics. + Embedded credentials are rejected during validation and never echoed to the + terminal where they could leak via shell history, CI logs, or screenshots. Falls back to the original string on any parse error so a misformed URL still surfaces in the error message rather than being swallowed. """ try: parsed = urlparse(url) - if not parsed.netloc or "@" not in parsed.netloc: - return url - host = parsed.hostname or "" - if parsed.port is not None: - host = f"{host}:{parsed.port}" - sanitized = parsed._replace(netloc=host) + if not parsed.scheme or not parsed.netloc: + return "" + parsed_port = parsed.port + netloc = parsed.netloc + if "@" in netloc: + netloc = parsed.hostname or "" + if parsed_port is not None: + netloc = f"{netloc}:{parsed_port}" + sanitized = parsed._replace( + netloc=netloc, + query="" if parsed.query else "", + fragment="" if parsed.fragment else "", + ) return urlunparse(sanitized) except (ValueError, TypeError): - return url + return "" def _is_local_or_metadata_host(host: str | None) -> bool: @@ -77,6 +83,15 @@ def _is_local_or_metadata_host(host: str | None) -> bool: return addr.is_link_local or addr.is_private or addr.is_multicast or addr.is_unspecified +def _require_ambient_http_opt_in(url: str, source: str) -> None: + """Require the existing explicit opt-in for ambient plaintext endpoints.""" + if urlparse(url).scheme.lower() == "http" and os.environ.get("MCP_REGISTRY_ALLOW_HTTP") != "1": + raise click.UsageError( + f"{source} uses plaintext HTTP; set MCP_REGISTRY_ALLOW_HTTP=1 " + "only if this endpoint is trusted" + ) + + def validate_registry_url(value: str | None) -> str | None: """Validate a ``--registry`` URL value. Return the normalized URL. @@ -116,12 +131,20 @@ def validate_registry_url(value: str | None) -> str | None: f"(e.g. https://mcp.internal.example.com)" ) scheme = parsed.scheme.lower() + try: + _ = parsed.port + except ValueError as exc: + raise click.UsageError(f"--registry: Invalid URL '{safe_value}': invalid port") from exc if scheme not in _ALLOWED_URL_SCHEMES: raise click.UsageError( f"--registry: Invalid URL '{safe_value}': scheme '{scheme}' is not " f"supported; use http:// or https://. WebSocket URLs (ws/wss) " f"and file:// paths are rejected for security." ) + if parsed.username is not None: + raise click.UsageError("--registry: embedded credentials are not supported") + if parsed.query or parsed.fragment: + raise click.UsageError("--registry: base URL must not contain a query or fragment") return normalized @@ -159,6 +182,12 @@ def resolve_registry_url( # Defaults are quiet, overrides are visible: surface the env-driven # registry redirect so a poisoned MCP_REGISTRY_URL cannot silently # change package resolution. Always emitted (not verbose-gated). + try: + env_value = validate_registry_url(env_value) + except click.UsageError as exc: + detail = exc.message.removeprefix("--registry: ") + raise click.UsageError(f"MCP_REGISTRY_URL is invalid: {detail}") from exc + _require_ambient_http_opt_in(env_value, "MCP_REGISTRY_URL") if logger is not None: logger.progress( f"Using MCP registry: {_redact_url_credentials(env_value)} (from MCP_REGISTRY_URL)", @@ -172,6 +201,15 @@ def resolve_registry_url( config_value = _get_mcp_registry_url() if config_value: + try: + config_value = validate_registry_url(config_value) + except click.UsageError as exc: + detail = exc.message.removeprefix("--registry: ") + raise click.UsageError( + "Configured mcp-registry-url is invalid; run " + f"'apm config unset mcp-registry-url' and retry: {detail}" + ) from exc + _require_ambient_http_opt_in(config_value, "Configured mcp-registry-url") if logger is not None: logger.progress( f"Using MCP registry: {_redact_url_credentials(config_value)} (from apm config)", @@ -202,11 +240,17 @@ def _maybe_warn_local_host(url: str, logger) -> None: ) -_REGISTRY_ENV_KEYS = ("MCP_REGISTRY_URL", "MCP_REGISTRY_ALLOW_HTTP") +_REGISTRY_SOURCE_ENV_KEY = "APM_MCP_REGISTRY_SOURCE" +_REGISTRY_ENV_KEYS = ("MCP_REGISTRY_URL", "MCP_REGISTRY_ALLOW_HTTP", _REGISTRY_SOURCE_ENV_KEY) @contextlib.contextmanager -def registry_env_override(registry_url: str | None) -> Iterator[None]: +def registry_env_override( + registry_url: str | None, + *, + allow_http: bool = True, + source: str | None = None, +) -> Iterator[None]: """Temporarily export ``MCP_REGISTRY_URL`` for the duration of a call. ``MCPIntegrator.install`` constructs ``MCPServerOperations()`` deep in @@ -230,7 +274,9 @@ def registry_env_override(registry_url: str | None) -> Iterator[None]: saved = {k: os.environ.get(k) for k in _REGISTRY_ENV_KEYS} try: os.environ["MCP_REGISTRY_URL"] = registry_url - if urlparse(registry_url).scheme.lower() == "http": + if source in {"flag", "env", "config"}: + os.environ[_REGISTRY_SOURCE_ENV_KEY] = source + if allow_http and urlparse(registry_url).scheme.lower() == "http": os.environ["MCP_REGISTRY_ALLOW_HTTP"] = "1" yield finally: diff --git a/src/apm_cli/integration/mcp_integrator.py b/src/apm_cli/integration/mcp_integrator.py index 4ca51b916c..9692844052 100644 --- a/src/apm_cli/integration/mcp_integrator.py +++ b/src/apm_cli/integration/mcp_integrator.py @@ -239,6 +239,28 @@ class MCPIntegrator: # Dependency resolution # ------------------------------------------------------------------ + @staticmethod + def prevalidate_registry_dependencies( + mcp_deps: list, + *, + registry_url: str | None, + verbose: bool, + logger, + registry_source: str | None = None, + ) -> builtins.dict[str, builtins.dict]: + """Validate direct-install registry identities before any write.""" + from apm_cli.integration.mcp_integrator_install import ( + prevalidate_registry_dependencies, + ) + + return prevalidate_registry_dependencies( + mcp_deps, + registry_url=registry_url, + verbose=verbose, + logger=logger, + registry_source=registry_source, + ) + @staticmethod def collect_transitive( apm_modules_dir: Path, @@ -1254,7 +1276,7 @@ def _gate_project_scoped_runtimes( return out @staticmethod - def install( + def install( # noqa: PLR0913 mcp_deps: list, runtime: str = None, # noqa: RUF013 exclude: str = None, # noqa: RUF013 @@ -1269,6 +1291,7 @@ def install( diagnostics=None, scope=None, managed_target_servers: builtins.dict | None = None, + prevalidated_registry_servers: builtins.dict[str, builtins.dict] | None = None, fail_on_write_error: bool = False, ) -> int: """Install MCP dependencies. @@ -1314,5 +1337,6 @@ def install( diagnostics=diagnostics, scope=scope, managed_target_servers=managed_target_servers, + prevalidated_registry_servers=prevalidated_registry_servers, fail_on_write_error=fail_on_write_error, ) diff --git a/src/apm_cli/integration/mcp_integrator_install.py b/src/apm_cli/integration/mcp_integrator_install.py index c023c2bd61..7d7d6957a7 100644 --- a/src/apm_cli/integration/mcp_integrator_install.py +++ b/src/apm_cli/integration/mcp_integrator_install.py @@ -36,6 +36,64 @@ _STRICT_CONFIG_FAILURE_RUNTIMES = frozenset({"intellij"}) +def _validate_registry_servers( + operations: Any, + server_names: list[str], + *, + dependency_count: int, + verbose: bool, + logger: Any, + fail_closed: bool = False, + server_info_cache: dict[str, dict] | None = None, +) -> list[str]: + """Validate registry identities and raise before any target write.""" + logger.mcp_lookup_heartbeat(len(server_names)) + if verbose: + logger.verbose_detail(f"Validating {dependency_count} registry servers...") + if fail_closed: + valid_servers, invalid_servers = operations.validate_servers_exist( + server_names, + fail_closed=True, + server_info_cache=server_info_cache, + ) + else: + valid_servers, invalid_servers = operations.validate_servers_exist( + server_names, + server_info_cache=server_info_cache, + ) + if invalid_servers: + logger.error(f"Server(s) not found in registry: {', '.join(invalid_servers)}") + logger.progress("Run 'apm mcp search ' to find available servers") + raise RuntimeError(f"Cannot install {len(invalid_servers)} missing server(s)") + return valid_servers + + +def prevalidate_registry_dependencies( + mcp_deps: list, + *, + registry_url: str | None, + verbose: bool, + logger: Any, + registry_source: str | None = None, +) -> dict[str, dict]: + """Resolve direct-install registry identities before persistent writes.""" + from apm_cli.registry.operations import MCPServerOperations + + server_names = [dep.name if hasattr(dep, "name") else dep for dep in mcp_deps] + operations = MCPServerOperations(registry_url=registry_url, registry_source=registry_source) + server_info_cache: dict[str, dict] = {} + valid_servers = _validate_registry_servers( + operations, + server_names, + dependency_count=len(mcp_deps), + verbose=verbose, + logger=logger, + fail_closed=True, + server_info_cache=server_info_cache, + ) + return {name: server_info_cache[name] for name in valid_servers} + + class _TargetSelectionSource(StrEnum): """Source that supplied the MCP target set before compatibility gates.""" @@ -47,7 +105,7 @@ class _TargetSelectionSource(StrEnum): INVALID_MANIFEST = "invalid-manifest" -def _install_registry_group( +def _install_registry_group( # noqa: PLR0913 operations: Any, group_dep_names: list, group_dep_map: dict, @@ -62,6 +120,7 @@ def _install_registry_group( console: Any, logger: Any, managed_target_servers: dict[str, set[str]] | None, + prevalidated_servers: dict[str, dict] | None = None, fail_on_write_error: bool = False, ) -> int: """Process one group of registry deps through a single ``MCPServerOperations`` instance. @@ -76,19 +135,19 @@ def _install_registry_group( configured_count = 0 failed_installations: list[str] = [] + registry_server_cache: dict[str, dict] = prevalidated_servers or {} - # Early validation: check all servers exist in registry (fail-fast). - # F4 (#1116): emit a single batch heartbeat so users see the - # registry round-trip in progress instead of silent stall. - logger.mcp_lookup_heartbeat(len(group_dep_names)) - if verbose: - logger.verbose_detail(f"Validating {len(group_deps)} registry servers...") - valid_servers, invalid_servers = operations.validate_servers_exist(group_dep_names) - - if invalid_servers: - logger.error(f"Server(s) not found in registry: {', '.join(invalid_servers)}") - logger.progress("Run 'apm mcp search ' to find available servers") - raise RuntimeError(f"Cannot install {len(invalid_servers)} missing server(s)") + if prevalidated_servers is not None and set(group_dep_names) <= prevalidated_servers.keys(): + valid_servers = group_dep_names + else: + valid_servers = _validate_registry_servers( + operations, + group_dep_names, + dependency_count=len(group_deps), + verbose=verbose, + logger=logger, + server_info_cache=registry_server_cache, + ) if valid_servers: servers_to_install = operations.check_servers_needing_installation( @@ -96,6 +155,7 @@ def _install_registry_group( valid_servers, project_root=project_root, user_scope=user_scope, + server_info_cache=registry_server_cache, ) already_configured_candidates = [ dep for dep in valid_servers if dep not in servers_to_install @@ -143,7 +203,16 @@ def _install_registry_group( # Batch fetch server info once if verbose: logger.verbose_detail(f"Installing {len(servers_to_install)} servers...") - server_info_cache = operations.batch_fetch_server_info(servers_to_install) + server_info_cache = { + name: registry_server_cache[name] + for name in servers_to_install + if name in registry_server_cache + } + unresolved_servers = [ + name for name in servers_to_install if name not in server_info_cache + ] + if unresolved_servers: + server_info_cache.update(operations.batch_fetch_server_info(unresolved_servers)) # Apply overlays for server_name in servers_to_install: @@ -469,6 +538,70 @@ def _declared_manifest_target_runtimes( return list(projected or ()), True +def partition_user_scope_runtimes( + target_runtimes: list[str], +) -> tuple[list[str], list[str]]: + """Partition runtime names by their adapter's user-scope capability.""" + from apm_cli.factory import ClientFactory + + supported: list[str] = [] + skipped: list[str] = [] + for runtime in target_runtimes: + try: + client = ClientFactory.create_client(runtime) + except ValueError: + skipped.append(runtime) + continue + destination = supported if client.supports_user_scope else skipped + destination.append(runtime) + return supported, skipped + + +def unavailable_user_scope_targets_message( + target_decision: EffectiveTargetDecision, + scoped_targets: list[str] | None, + skipped_targets: list[str], +) -> str: + """Render recovery that distinguishes disabled from workspace-only targets.""" + original_targets = set(target_decision.runtime_targets or []) + disabled_targets = original_targets - set(scoped_targets or []) + experimental_hint = "enable selected experimental targets, " if disabled_targets else "" + rendered_targets = ", ".join(sorted(original_targets or set(skipped_targets))) + return ( + "Selected targets are unavailable for user-scope MCP installation " + f"({rendered_targets}; source: {target_decision.source}); " + f"{experimental_hint}choose a global-capable --target or omit --global" + ) + + +def discover_user_scope_mcp_runtimes( + project_root: Path, + *, + exclude: str | None = None, +) -> tuple[list[str], list[str]]: + """Discover installed MCP runtimes and partition them for user scope.""" + discovered = _discover_installed_runtimes(project_root, user_scope=True) + discovered = filter_excluded_mcp_runtimes(discovered, exclude) + return partition_user_scope_runtimes(discovered) + + +def filter_excluded_mcp_runtimes( + target_runtimes: list[str], + exclude: str | None, +) -> list[str]: + """Apply one canonical runtime exclusion, including target aliases.""" + if not exclude: + return list(target_runtimes) + exclusions = {exclude} + try: + from apm_cli.core.target_detection import EffectiveTargetDecision + + exclusions.update(EffectiveTargetDecision(exclude, "--exclude").runtime_equivalents or ()) + except KeyError: + pass + return [runtime for runtime in target_runtimes if runtime not in exclusions] + + def _resolve_target_runtimes( runtime: str | None, exclude: str | None, @@ -638,18 +771,7 @@ def _resolve_target_runtimes( # Exclusion narrows every selected source, including explicit CLI choices. # Apply it before progress output so the message names the narrowed set. if exclude: - exclusions = {exclude} - try: - from apm_cli.core.target_detection import EffectiveTargetDecision - - exclusions.update( - EffectiveTargetDecision(exclude, "--exclude").runtime_equivalents or () - ) - except KeyError: - pass - target_runtimes = [ - candidate for candidate in target_runtimes if candidate not in exclusions - ] + target_runtimes = filter_excluded_mcp_runtimes(target_runtimes, exclude) # Invalid manifests continue to the shared gate for canonical rendering. if not target_runtimes and selection_source is not _TargetSelectionSource.INVALID_MANIFEST: logger.warning( @@ -725,19 +847,7 @@ def _resolve_target_runtimes( from apm_cli.core.scope import InstallScope as _IS if scope is _IS.USER: - from apm_cli.factory import ClientFactory as _CF - - pre_filter = list(target_runtimes) - filtered_runtimes = [] - for rt in target_runtimes: - try: - client = _CF.create_client(rt) - except ValueError: - continue - if client.supports_user_scope: - filtered_runtimes.append(rt) - target_runtimes = filtered_runtimes - skipped = set(pre_filter) - set(target_runtimes) + target_runtimes, skipped = partition_user_scope_runtimes(target_runtimes) if skipped: msg = ( f"Skipped workspace-only runtimes at user scope: " @@ -918,7 +1028,7 @@ def _print_mcp_summary( console.print(f"[green]{STATUS_SYMBOLS['success']} All servers up to date[/green]") -def run_mcp_install( +def run_mcp_install( # noqa: PLR0913 mcp_deps: list, runtime: str | None = None, exclude: str | None = None, @@ -933,6 +1043,7 @@ def run_mcp_install( diagnostics=None, scope: InstallScope | None = None, managed_target_servers: dict[str, set[str]] | None = None, + prevalidated_registry_servers: dict[str, dict] | None = None, fail_on_write_error: bool = False, ) -> int: """Install MCP dependencies. @@ -1110,6 +1221,7 @@ def run_mcp_install( console=console, logger=logger, managed_target_servers=managed_target_servers, + prevalidated_servers=prevalidated_registry_servers, fail_on_write_error=fail_on_write_error, ) diff --git a/src/apm_cli/registry/client.py b/src/apm_cli/registry/client.py index 2cc6e21021..c29fea0751 100644 --- a/src/apm_cli/registry/client.py +++ b/src/apm_cli/registry/client.py @@ -99,6 +99,25 @@ def _decode_registry_json(raw: bytes) -> Any: _SERVER_NAME_RE = re.compile(r"^[A-Za-z0-9._~-]+(/[A-Za-z0-9._~-]+)?$") +def _safe_registry_url_for_display(value: str) -> str: + """Return a credential-free registry URL for diagnostics.""" + try: + parsed = urlparse(value) + _ = parsed.port + netloc = parsed.netloc + if "@" in netloc: + netloc = parsed.hostname or "" + if parsed.port is not None: + netloc = f"{netloc}:{parsed.port}" + return parsed._replace( + netloc=netloc, + query="" if parsed.query else "", + fragment="" if parsed.fragment else "", + ).geturl() + except (TypeError, ValueError): + return "" + + class ServerNotFoundError(ValueError): """Raised when a server lookup against the registry returns 404. @@ -152,7 +171,7 @@ def _read_float(env_key: str, default: float) -> float: class SimpleRegistryClient: """Simple client for querying MCP registries for server discovery.""" - def __init__(self, registry_url: str | None = None): + def __init__(self, registry_url: str | None = None, registry_source: str | None = None): """Initialize the registry client. Args: @@ -176,40 +195,55 @@ def __init__(self, registry_url: str | None = None): resolved = resolved.strip().rstrip("/") parsed = urlparse(resolved) + safe_resolved = _safe_registry_url_for_display(resolved) if not parsed.scheme or not parsed.netloc: raise ValueError( - f"Invalid MCP registry URL {resolved!r}: expected scheme://host " + f"Invalid MCP registry URL {safe_resolved!r}: expected scheme://host " f"(e.g. https://mcp.example.com). Check MCP_REGISTRY_URL if set." ) + try: + _ = parsed.port + except ValueError as exc: + raise ValueError( + f"Invalid MCP registry URL {safe_resolved!r}: invalid port. " + "Check MCP_REGISTRY_URL if set." + ) from exc + if parsed.username or parsed.password: + raise ValueError("MCP registry base URLs do not support embedded credentials.") + if parsed.query or parsed.fragment: + raise ValueError( + f"Invalid MCP registry base URL {safe_resolved!r}: " + "query strings and fragments are not supported." + ) if parsed.scheme not in ("http", "https"): raise ValueError( f"Unsupported scheme {parsed.scheme!r} in MCP registry URL " - f"{resolved!r}: only https:// is supported (http:// requires " + f"{safe_resolved!r}: only https:// is supported (http:// requires " f"MCP_REGISTRY_ALLOW_HTTP=1). Check MCP_REGISTRY_URL if set." ) - if parsed.scheme == "http" and not os.environ.get("MCP_REGISTRY_ALLOW_HTTP"): + if parsed.scheme == "http" and os.environ.get("MCP_REGISTRY_ALLOW_HTTP") != "1": raise ValueError( - f"Insecure MCP registry URL {resolved!r}: http:// is not allowed " + f"Insecure MCP registry URL {safe_resolved!r}: http:// is not allowed " f"by default. Set MCP_REGISTRY_ALLOW_HTTP=1 to opt in to plaintext " f"HTTP (not recommended for production). " f"Check MCP_REGISTRY_URL if set." ) - # Strip any embedded userinfo (``user:pass@``) before storing the URL so - # ``ServerNotFoundError`` and other diagnostics cannot leak credentials - # into terminal output or CI logs. Enterprise users sometimes set - # ``MCP_REGISTRY_URL=https://token:x-oauth@registry.corp/`` -- we still - # accept the URL (the credentials are passed via Authorization headers - # elsewhere), but we never echo them back. - if parsed.username or parsed.password: - host = parsed.hostname or "" - sanitized_netloc = host + (f":{parsed.port}" if parsed.port else "") - resolved = parsed._replace(netloc=sanitized_netloc).geturl().rstrip("/") - self.registry_url = resolved # True when the URL came from an explicit caller arg or MCP_REGISTRY_URL env var. # Consumed by validate_servers_exist() to fail-closed on overrides. self._is_custom_url = registry_url is not None or env_override is not None + source_override = os.environ.get("APM_MCP_REGISTRY_SOURCE") + if registry_source in {"flag", "env", "config"}: + self.registry_source = registry_source + elif registry_url is not None: + self.registry_source = "argument" + elif env_override is not None and source_override in {"flag", "env", "config"}: + self.registry_source = source_override + elif env_override is not None: + self.registry_source = "env" + else: + self.registry_source = "default" self.session = requests.Session() self._timeout = _resolve_timeout() self._http_cache = self._init_http_cache() @@ -373,13 +407,41 @@ def search_servers(self, query: str) -> list[dict[str, Any]]: Raises: requests.RequestException: If the request fails. """ + servers, _next_cursor = self._search_servers_page(query) + return servers + + def _search_servers_page( + self, query: str, cursor: str | None = None + ) -> tuple[list[dict[str, Any]], str | None]: + """Return one page from the spec search endpoint.""" url = f"{self.registry_url}{_V0_1_PREFIX}/servers" params = {"search": query} + if cursor is not None: + params["cursor"] = cursor data, _hdrs = self._cached_get_json(url, params=params) data = data or {} + metadata = data.get("metadata", {}) - return self._unwrap_server_list(data) + return self._unwrap_server_list(data), metadata.get("nextCursor") + + def _search_servers_all_pages(self, query: str) -> list[dict[str, Any]]: + """Search all pages so bare-name uniqueness is checked globally.""" + all_servers: list[dict[str, Any]] = [] + cursor: str | None = None + seen_cursors: set[str] = set() + while True: + servers, next_cursor = self._search_servers_page(query, cursor) + all_servers.extend(servers) + if not next_cursor: + return all_servers + if next_cursor in seen_cursors: + raise requests.RequestException( + "MCP registry returned a repeated search cursor; " + "cannot prove bare server-name uniqueness" + ) + seen_cursors.add(next_cursor) + cursor = next_cursor @staticmethod def _unwrap_server_list(data: dict[str, Any]) -> list[dict[str, Any]]: @@ -570,8 +632,10 @@ def find_server_by_reference(self, reference: str) -> dict[str, Any] | None: Raises: requests.RequestException: If the registry API request fails. """ - # Use search API to find by name - search_results = self.search_servers(reference) + # Use search API to find by name. Fetch every page before applying + # the bare-slug uniqueness guard; selecting from only page one would + # make dependency-confusion possible when another match sits later. + search_results = self._search_servers_all_pages(reference) # Pass 1: exact full-name match (prevents slug collisions) for server in search_results: @@ -582,14 +646,17 @@ def find_server_by_reference(self, reference: str) -> dict[str, Any] | None: except ValueError: continue - # Pass 2: fuzzy slug match (only when reference has no namespace) - for server in search_results: - server_name = server.get("name", "") - if self._is_server_match(reference, server_name): - try: - return self.get_server(server_name) - except ValueError: - continue + # Pass 2: an unqualified slug is safe only when it has one match. + slug_matches = [ + server.get("name", "") + for server in search_results + if self._is_server_match(reference, server.get("name", "")) + ] + if len(slug_matches) == 1: + try: + return self.get_server(slug_matches[0]) + except ValueError: + pass # If not found by name, server is not in registry return None diff --git a/src/apm_cli/registry/operations.py b/src/apm_cli/registry/operations.py index 3a8c9ab209..1319936740 100644 --- a/src/apm_cli/registry/operations.py +++ b/src/apm_cli/registry/operations.py @@ -13,16 +13,29 @@ logger = logging.getLogger(__name__) +def _registry_recovery_hint(registry_client: SimpleRegistryClient) -> str: + """Return source-specific recovery guidance for custom registry failures.""" + source = getattr(registry_client, "registry_source", None) + if source == "flag": + return "verify the --registry URL and registry reachability" + if source == "env": + return "verify MCP_REGISTRY_URL and registry reachability" + if source == "config": + return "verify the configured registry URL and registry reachability" + return "verify MCP_REGISTRY_URL or the configured registry URL and registry reachability" + + class MCPServerOperations: """Handles MCP server operations like conflict detection and installation status.""" - def __init__(self, registry_url: str | None = None): + def __init__(self, registry_url: str | None = None, registry_source: str | None = None): """Initialize MCP server operations. Args: - registry_url: Optional registry URL override + registry_url: Optional registry URL override. + registry_source: Optional source label for recovery text. """ - self.registry_client = SimpleRegistryClient(registry_url) + self.registry_client = SimpleRegistryClient(registry_url, registry_source=registry_source) def check_servers_needing_installation( self, @@ -31,6 +44,7 @@ def check_servers_needing_installation( project_root: Path | str | None = None, user_scope: bool = False, max_workers: int = 4, + server_info_cache: dict[str, dict] | None = None, ) -> list[str]: """Check which MCP servers actually need installation across target runtimes. @@ -67,7 +81,11 @@ def check_servers_needing_installation( def _check_one(server_ref: str) -> tuple[str, bool]: """Return (server_ref, needs_install).""" try: - server_info = self.registry_client.find_server_by_reference(server_ref) + server_info = None + if server_info_cache is not None: + server_info = server_info_cache.get(server_ref) + if server_info is None: + server_info = self.registry_client.find_server_by_reference(server_ref) if not server_info: return (server_ref, True) server_id = server_info.get("id") @@ -177,7 +195,12 @@ def _get_installed_server_ids( return installed_ids def validate_servers_exist( - self, server_references: list[str], max_workers: int = 4 + self, + server_references: list[str], + max_workers: int = 4, + *, + fail_closed: bool = False, + server_info_cache: dict[str, dict] | None = None, ) -> tuple[list[str], list[str]]: """Validate that all servers exist in the registry before attempting installation. @@ -192,40 +215,65 @@ def validate_servers_exist( Args: server_references: List of MCP server references to validate max_workers: Max parallel HTTP lookups (default 4). + fail_closed: Raise on registry network errors instead of assuming + the reference is valid. + server_info_cache: Optional destination for successfully resolved + server documents. Returns: Tuple of (valid_servers, invalid_servers) """ - from concurrent.futures import ThreadPoolExecutor + from concurrent.futures import ThreadPoolExecutor, as_completed valid_servers: list[str] = [] invalid_servers: list[str] = [] - def _validate_one(server_ref: str) -> tuple[str, bool]: - """Return (server_ref, is_valid).""" + def _validate_one(server_ref: str) -> tuple[str, bool, dict | None]: + """Return the reference, validity, and resolved server document.""" try: server_info = self.registry_client.find_server_by_reference(server_ref) - return (server_ref, server_info is not None) + return (server_ref, server_info is not None, server_info) except requests.RequestException: - if getattr(self.registry_client, "_is_custom_url", False): + if fail_closed or getattr(self.registry_client, "_is_custom_url", False): + if getattr(self.registry_client, "_is_custom_url", False): + recovery = _registry_recovery_hint(self.registry_client) + else: + recovery = "verify network connectivity and registry reachability" raise RuntimeError( # noqa: B904 f"Could not reach MCP registry at " f"{self.registry_client.registry_url} while validating " - f"server '{server_ref}'. MCP_REGISTRY_URL is set -- " - f"verify the URL is correct and reachable." + f"server '{server_ref}'; {recovery}." ) logger.debug( "Registry lookup failed for %s, assuming valid (transient error)", server_ref, exc_info=True, ) - return (server_ref, True) + return (server_ref, True, None) workers = min(max_workers, len(server_references)) if server_references else 1 - with ThreadPoolExecutor(max_workers=workers, thread_name_prefix="mcp-validate") as executor: - for ref, is_valid in executor.map(_validate_one, server_references): + results: dict[str, tuple[bool, dict | None]] = {} + executor = ThreadPoolExecutor(max_workers=workers, thread_name_prefix="mcp-validate") + futures = {executor.submit(_validate_one, ref): ref for ref in server_references} + try: + for future in as_completed(futures): + ref, is_valid, server_info = future.result() + results[ref] = (is_valid, server_info) + except Exception: + for future in futures: + future.cancel() + executor.shutdown(wait=True, cancel_futures=True) + raise + else: + executor.shutdown(wait=True) + + for ref in server_references: + if ref in results: + is_valid, server_info = results[ref] if is_valid: valid_servers.append(ref) + if server_info_cache is not None and server_info is not None: + server_info_cache[ref] = server_info else: invalid_servers.append(ref) diff --git a/tests/integration/test_architecture_owner_rule_mutations.py b/tests/integration/test_architecture_owner_rule_mutations.py index 05d2476c93..6abb589efe 100644 --- a/tests/integration/test_architecture_owner_rule_mutations.py +++ b/tests/integration/test_architecture_owner_rule_mutations.py @@ -269,6 +269,14 @@ class MutationCase: new=" def enforce_frozen_disabled(", intent="InstallService stops owning the frozen-install mutation preflight.", ), + MutationCase( + guard_id="install-deployment-install-scope-selection", + rule_id="install-deployment-install-scope-selection", + path="src/apm_cli/commands/install.py", + old="user_scope=is_user_scope(scope)", + new="user_scope=False", + intent="Direct MCP target resolution stops consuming the command's scope decision.", + ), MutationCase( guard_id="install-deployment-mcp-ownership-migration", rule_id="install-deployment-mcp-ownership-migration", diff --git a/tests/integration/test_config_surface_lifecycle_contract.py b/tests/integration/test_config_surface_lifecycle_contract.py index 59715d82a9..c25c3179cc 100644 --- a/tests/integration/test_config_surface_lifecycle_contract.py +++ b/tests/integration/test_config_surface_lifecycle_contract.py @@ -8,6 +8,7 @@ import sys from dataclasses import dataclass from pathlib import Path, PurePosixPath +from urllib.parse import urlparse import pytest @@ -22,6 +23,7 @@ LocalGitRepository, LocalGitRepositoryFactory, ) +from tests.utils.local_mcp_registry import LocalMcpRegistryFactory from tests.utils.local_package import LocalPackageFactory pytestmark = [ @@ -1193,6 +1195,593 @@ def test_saved_target_drives_direct_mcp_without_target_flag( assert "saved-direct-mcp" in repaired["mcpServers"] +def test_global_direct_mcp_uses_user_manifest_and_runtime_config( + tmp_path: Path, + apm_binary_path: Path, +) -> None: + """Global direct MCP install must avoid project-scoped state.""" + isolated = IsolatedApmEnvironment.create( + tmp_path / "global-direct-mcp", + base_env=dict(os.environ), + ) + user_manifest = isolated.home / ".apm" / "apm.yml" + (isolated.home / ".gemini").mkdir() + project = isolated.work_root / "consumer" + project.mkdir() + (project / ".cursor").mkdir() + project_manifest = project / "apm.yml" + dump_yaml( + { + "name": "project-direct", + "version": "0.1.0", + "targets": ["cursor"], + "dependencies": {"mcp": []}, + }, + project_manifest, + ) + + result = _runner(apm_binary_path).run_sequence( + ( + ( + "install", + "-g", + "--mcp", + "global-direct-server", + "--no-policy", + "--", + "echo", + "ready", + ), + ), + expected_returncodes=(0,), + scenario_id="global-direct-mcp", + cwd=project, + env=isolated.subprocess_env(), + )[0] + + user_config = load_yaml(user_manifest) + assert user_config["dependencies"]["mcp"][0]["name"] == "global-direct-server" + project_config = load_yaml(project_manifest) + assert project_config["dependencies"]["mcp"] == [] + gemini_config = json.loads( + (isolated.home / ".gemini" / "settings.json").read_text(encoding="utf-8") + ) + assert gemini_config["mcpServers"]["global-direct-server"] == { + "args": ["ready"], + "command": "echo", + } + assert "Install interrupted" not in result.stdout + result.stderr + + +def test_configured_mcp_registry_drives_global_direct_install( + tmp_path: Path, + apm_binary_path: Path, +) -> None: + """Persisted registry config must reach a real direct MCP integration.""" + isolated = IsolatedApmEnvironment.create( + tmp_path / "configured-direct-registry", + base_env=dict(os.environ), + ) + project = isolated.work_root / "consumer" + project.mkdir() + server_name = "io.github.apm/configured-registry" + document = { + "name": server_name, + "description": "Configured direct registry fixture", + "version": "1.0.0", + "packages": [ + { + "registryType": "npm", + "identifier": "@apm/configured-registry", + "runtimeHint": "npx", + "transport": {"type": "stdio"}, + "runtimeArguments": [], + } + ], + } + registry_factory = LocalMcpRegistryFactory(isolated.root / "registries") + + with registry_factory.start(document) as registry: + parsed_registry = urlparse(registry.url) + assert parsed_registry.port is not None + environment = isolated.subprocess_env() + environment["APM_TEST_LOOPBACK_PORTS"] = str(parsed_registry.port) + environment["MCP_REGISTRY_ALLOW_HTTP"] = "0" + runner = _runner(apm_binary_path) + config_result = runner.run( + ("config", "set", "mcp-registry-url", registry.url), + scenario_id="configured-global-direct-registry-config", + cwd=project, + env=environment, + ) + denied = runner.run( + ( + "install", + "-g", + "--mcp", + server_name, + "--target", + "claude", + "--no-policy", + "--verbose", + ), + scenario_id="configured-global-direct-registry-denied", + cwd=project, + env=environment, + ) + assert config_result.returncode == 0 + assert denied.returncode == 2 + assert "MCP_REGISTRY_ALLOW_HTTP=1" in denied.stdout + denied.stderr + assert list(registry.request_paths) == [] + + (isolated.home / ".apm" / "apm.yml").unlink(missing_ok=True) + (isolated.home / ".apm" / "apm.lock.yaml").unlink(missing_ok=True) + environment["MCP_REGISTRY_ALLOW_HTTP"] = "1" + runner.run_sequence( + ( + ( + "install", + "-g", + "--mcp", + server_name, + "--target", + "claude", + "--no-policy", + ), + ), + expected_returncodes=(0,), + scenario_id="configured-global-direct-registry", + cwd=project, + env=environment, + ) + + assert any(path.startswith("/v0.1/servers?") for path in registry.request_paths) + assert any(path.endswith("/versions/latest") for path in registry.request_paths) + + user_manifest = load_yaml(isolated.home / ".apm" / "apm.yml") + entry = user_manifest["dependencies"]["mcp"][0] + stored_registry = urlparse(entry["registry"]) + assert ( + stored_registry.scheme, + stored_registry.hostname, + stored_registry.port, + ) == ( + parsed_registry.scheme, + parsed_registry.hostname, + parsed_registry.port, + ) + claude_config = json.loads((isolated.home / ".claude.json").read_text(encoding="utf-8")) + assert "configured-registry" in claude_config["mcpServers"] + + +def test_unknown_global_registry_server_changes_no_user_state( + tmp_path: Path, + apm_binary_path: Path, +) -> None: + """Registry identity validation must precede every user-scope write.""" + isolated = IsolatedApmEnvironment.create( + tmp_path / "missing-direct-registry", + base_env=dict(os.environ), + ) + project = isolated.work_root / "consumer" + project.mkdir() + document = { + "name": "io.github.apm/known-server", + "description": "Known registry fixture", + "version": "1.0.0", + "packages": [], + } + registry_factory = LocalMcpRegistryFactory(isolated.root / "registries") + + with registry_factory.start(document) as registry: + parsed_registry = urlparse(registry.url) + assert parsed_registry.port is not None + environment = isolated.subprocess_env() + environment["APM_TEST_LOOPBACK_PORTS"] = str(parsed_registry.port) + result = _runner(apm_binary_path).run( + ( + "install", + "-g", + "--mcp", + "io.github.apm/missing-server", + "--target", + "claude", + "--registry", + registry.url, + "--no-policy", + ), + scenario_id="missing-global-direct-registry", + cwd=project, + env=environment, + ) + + assert result.returncode == 1, (result.stdout, result.stderr) + output = result.stdout + result.stderr + assert "Check the server name" in output + assert "then retry" in output + assert "no state was changed" in output + assert not (isolated.home / ".apm" / "apm.yml").exists() + assert not (isolated.home / ".apm" / "apm.lock.yaml").exists() + assert not (isolated.home / ".claude.json").exists() + + +def test_unreachable_global_registry_changes_no_user_state( + tmp_path: Path, + apm_binary_path: Path, +) -> None: + """A connection failure must precede every user-scope write.""" + isolated = IsolatedApmEnvironment.create( + tmp_path / "unreachable-direct-registry", + base_env=dict(os.environ), + ) + project = isolated.work_root / "consumer" + project.mkdir() + document = { + "name": "io.github.apm/known-server", + "description": "Closed registry fixture", + "version": "1.0.0", + "packages": [], + } + registry_factory = LocalMcpRegistryFactory(isolated.root / "registries") + with registry_factory.start(document) as registry: + registry_url = registry.url + registry_port = urlparse(registry_url).port + assert registry_port is not None + + environment = isolated.subprocess_env() + environment["APM_TEST_LOOPBACK_PORTS"] = str(registry_port) + environment["MCP_REGISTRY_ALLOW_HTTP"] = "1" + result = _runner(apm_binary_path).run( + ( + "install", + "-g", + "--mcp", + document["name"], + "--target", + "claude", + "--registry", + registry_url, + "--no-policy", + ), + scenario_id="unreachable-global-direct-registry", + cwd=project, + env=environment, + ) + + assert result.returncode == 1, (result.stdout, result.stderr) + output = result.stdout + result.stderr + assert "Could not reach MCP registry" in output + assert "verify the --registry URL" in output + assert "reachability" in output + assert "No state was changed." in output + assert not (isolated.home / ".apm" / "apm.yml").exists() + assert not (isolated.home / ".apm" / "apm.lock.yaml").exists() + assert not (isolated.home / ".claude.json").exists() + + +def test_ambient_registry_source_is_pinned_for_replay( + tmp_path: Path, + apm_binary_path: Path, +) -> None: + """An ambient private registry becomes credential-free manifest identity.""" + isolated = IsolatedApmEnvironment.create( + tmp_path / "ambient-registry-replay", + base_env=dict(os.environ), + ) + project = isolated.work_root / "consumer" + project.mkdir() + server_name = "io.github.apm/ambient-replay" + document = { + "name": server_name, + "description": "Ambient registry replay fixture", + "version": "1.0.0", + "packages": [ + { + "registryType": "npm", + "identifier": "@apm/ambient-replay", + "runtimeHint": "npx", + "transport": {"type": "stdio"}, + "runtimeArguments": [], + } + ], + } + registry_factory = LocalMcpRegistryFactory(isolated.root / "registries") + + with registry_factory.start(document) as registry: + parsed_registry = urlparse(registry.url) + assert parsed_registry.port is not None + environment = isolated.subprocess_env() + environment["APM_TEST_LOOPBACK_PORTS"] = str(parsed_registry.port) + environment["MCP_REGISTRY_ALLOW_HTTP"] = "1" + environment["MCP_REGISTRY_URL"] = registry.url + runner = _runner(apm_binary_path) + runner.run_sequence( + ( + ( + "install", + "-g", + "--mcp", + server_name, + "--target", + "claude", + "--no-policy", + ), + ), + expected_returncodes=(0,), + scenario_id="ambient-registry-initial", + cwd=project, + env=environment, + ) + initial_request_count = len(registry.request_paths) + environment.pop("MCP_REGISTRY_URL") + runner.run_sequence( + (("install", "-g", "--only", "mcp", "--target", "claude", "--no-policy"),), + expected_returncodes=(0,), + scenario_id="ambient-registry-replay", + cwd=project, + env=environment, + ) + assert len(registry.request_paths) == initial_request_count + + entry = load_yaml(isolated.home / ".apm" / "apm.yml")["dependencies"]["mcp"][0] + stored_registry = urlparse(entry["registry"]) + assert ( + stored_registry.scheme, + stored_registry.hostname, + stored_registry.port, + ) == ( + parsed_registry.scheme, + parsed_registry.hostname, + parsed_registry.port, + ) + + +def test_global_direct_mcp_filters_mixed_and_rejects_zero_supported_targets( + tmp_path: Path, + apm_binary_path: Path, +) -> None: + """User-scope capability filtering precedes state and survives replay.""" + isolated = IsolatedApmEnvironment.create( + tmp_path / "global-target-capability", + base_env=dict(os.environ), + ) + project = isolated.work_root / "consumer" + project.mkdir() + environment = isolated.subprocess_env() + runner = _runner(apm_binary_path) + user_manifest = isolated.home / ".apm" / "apm.yml" + + rejected = runner.run( + ( + "install", + "-g", + "--target", + "vscode", + "--mcp", + "rejected-server", + "--no-policy", + "--", + "echo", + "rejected", + ), + scenario_id="global-zero-supported-target", + cwd=project, + env=environment, + ) + + assert rejected.returncode == 2 + assert not user_manifest.exists() + + excluded = runner.run( + ( + "install", + "-g", + "--target", + "claude", + "--exclude", + "claude", + "--mcp", + "excluded-server", + "--no-policy", + "--", + "echo", + "excluded", + ), + scenario_id="global-all-targets-excluded", + cwd=project, + env=environment, + ) + assert excluded.returncode == 2 + excluded_output = excluded.stdout + excluded.stderr + assert "removed by --exclude" in excluded_output + assert "remove the exclusion" in excluded_output + assert not user_manifest.exists() + + hermes = runner.run( + ( + "install", + "-g", + "--target", + "hermes", + "--mcp", + "disabled-hermes-server", + "--no-policy", + "--", + "echo", + "disabled", + ), + scenario_id="global-disabled-hermes", + cwd=project, + env=environment, + ) + assert hermes.returncode == 2 + assert not user_manifest.exists() + + mixed, replay = runner.run_sequence( + ( + ( + "install", + "-g", + "--target", + "vscode,claude", + "--mcp", + "mixed-server", + "--no-policy", + "--", + "echo", + "mixed", + ), + ( + "install", + "-g", + "--mcp", + "replay-server", + "--no-policy", + "--", + "echo", + "replay", + ), + ), + expected_returncodes=(0, 0), + scenario_id="global-mixed-target-replay", + cwd=project, + env=environment, + ) + + assert "Skipped workspace-only runtimes at user scope: vscode" in (mixed.stdout + mixed.stderr) + assert "Skipped workspace-only runtimes" not in replay.stdout + replay.stderr + user_config = load_yaml(user_manifest) + assert user_config["targets"] == ["claude"] + claude_config = json.loads((isolated.home / ".claude.json").read_text(encoding="utf-8")) + assert set(claude_config["mcpServers"]) == {"mixed-server", "replay-server"} + + explicitly_excluded = runner.run( + ( + "install", + "-g", + "--target", + "vscode,claude", + "--exclude", + "vscode", + "--mcp", + "explicit-exclusion-server", + "--no-policy", + "--", + "echo", + "excluded", + ), + scenario_id="global-workspace-target-explicitly-excluded", + cwd=project, + env=environment, + ) + assert explicitly_excluded.returncode == 0 + assert "Skipped workspace-only runtimes" not in ( + explicitly_excluded.stdout + explicitly_excluded.stderr + ) + + +def test_global_direct_mcp_dry_run_creates_no_user_state( + tmp_path: Path, + apm_binary_path: Path, +) -> None: + """A user-scope preview must not bootstrap any persistent state.""" + isolated = IsolatedApmEnvironment.create( + tmp_path / "global-direct-dry-run", + base_env=dict(os.environ), + ) + project = isolated.work_root / "consumer" + project.mkdir() + + result = _runner(apm_binary_path).run( + ( + "install", + "-g", + "--target", + "claude", + "--mcp", + "dry-run-server", + "--dry-run", + "--no-policy", + "--", + "echo", + "ready", + ), + scenario_id="global-direct-mcp-dry-run", + cwd=project, + env=isolated.subprocess_env(), + ) + + assert result.returncode == 0 + assert not (isolated.home / ".apm" / "apm.yml").exists() + assert not (isolated.home / ".apm" / "apm.lock.yaml").exists() + assert not (isolated.home / ".claude.json").exists() + + +@pytest.mark.parametrize( + ("ambient_url", "secrets"), + ( + ( + "https://user:userinfo-secret@registry.example.invalid" + "?token=query-secret#fragment-secret", + ("userinfo-secret", "query-secret", "fragment-secret"), + ), + ( + "https://user:port-secret@registry.example.invalid:notaport", + ("port-secret",), + ), + ( + "https://registry.example.invalid:bare-port-secret", + ("bare-port-secret",), + ), + ( + "REGISTRY_SECRET_SENTINEL", + ("REGISTRY_SECRET_SENTINEL",), + ), + ), +) +def test_ambient_registry_credentials_never_reach_manifest_or_output( + tmp_path: Path, + apm_binary_path: Path, + ambient_url: str, + secrets: tuple[str, ...], +) -> None: + """Ambient registry parse failures redact every credential-bearing component.""" + isolated = IsolatedApmEnvironment.create( + tmp_path / f"ambient-registry-{len(secrets)}", + base_env=dict(os.environ), + ) + project = isolated.work_root / "consumer" + project.mkdir() + environment = isolated.subprocess_env() + environment["MCP_REGISTRY_URL"] = ambient_url + + result = _runner(apm_binary_path).run( + ( + "install", + "-g", + "--target", + "claude", + "--mcp", + "ambient-registry-server", + "--no-policy", + "--verbose", + ), + scenario_id="ambient-registry-redaction", + cwd=project, + env=environment, + ) + + assert result.returncode == 2 + combined_output = result.stdout + result.stderr + user_manifest_path = isolated.home / ".apm" / "apm.yml" + user_manifest = ( + user_manifest_path.read_text(encoding="utf-8") if user_manifest_path.is_file() else "" + ) + for secret in secrets: + assert secret not in combined_output + assert secret not in user_manifest + + def test_saved_target_drives_declared_mcp_and_lsp_without_package( tmp_path: Path, apm_binary_path: Path, diff --git a/tests/integration/test_coverage_phase4.py b/tests/integration/test_coverage_phase4.py index e9a333145e..cdf8a10e83 100644 --- a/tests/integration/test_coverage_phase4.py +++ b/tests/integration/test_coverage_phase4.py @@ -25,6 +25,7 @@ from typing import Any from unittest.mock import MagicMock, patch +import click import pytest from click.testing import CliRunner @@ -634,6 +635,10 @@ def test_skipped_status_verifies_integration(self, tmp_path: Path) -> None: "apm_cli.install.mcp.command.MCPIntegrator.install", return_value=0, ) as install_mcp, + patch( + "apm_cli.install.mcp.command.MCPIntegrator.prevalidate_registry_dependencies", + return_value={"my-server": {"id": "my-server-id"}}, + ), patch("apm_cli.install.mcp.command.MCPIntegrator.update_lockfile"), ): run_mcp_install( @@ -674,30 +679,133 @@ def test_added_string_entry_no_deps_available(self, tmp_path: Path) -> None: patch( "apm_cli.install.mcp.command.add_mcp_to_apm_yml", return_value=("added", None), - ), + ) as add_mcp, patch("apm_cli.install.mcp.command.warn_ssrf_url"), patch("apm_cli.install.mcp.command.warn_shell_metachars"), patch("apm_cli.install.mcp.command.APM_DEPS_AVAILABLE", False), ): - run_mcp_install( - mcp_name="registry-server", - transport=None, - url=None, - env_pairs=None, - header_pairs=None, - mcp_version=None, - command_argv=None, - dev=False, - force=False, - runtime=None, - exclude=None, - logger=logger, - apm_dir=tmp_path, - scope=None, - ) + with pytest.raises(click.ClickException, match="validation is unavailable"): + run_mcp_install( + mcp_name="registry-server", + transport=None, + url=None, + env_pairs=None, + header_pairs=None, + mcp_version=None, + command_argv=None, + dev=False, + force=False, + runtime=None, + exclude=None, + logger=logger, + apm_dir=tmp_path, + scope=None, + ) - logger.success.assert_called_once() - assert "Added" in logger.success.call_args[0][0] + add_mcp.assert_not_called() + logger.success.assert_not_called() + + def test_direct_registry_prevalidation_failure_is_already_rendered( + self, tmp_path: Path + ) -> None: + from apm_cli.install.errors import InstallFailureAlreadyRendered + from apm_cli.install.mcp.command import run_mcp_install + + apm_yml = tmp_path / "apm.yml" + apm_yml.write_text(_APM_YML_MINIMAL, encoding="utf-8") + logger = self._make_logger() + + with ( + patch( + "apm_cli.install.mcp.command.build_mcp_entry", + return_value=("registry-server", False), + ), + patch( + "apm_cli.install.mcp.command.add_mcp_to_apm_yml", + return_value=("added", None), + ) as add_mcp, + patch("apm_cli.install.mcp.command.warn_ssrf_url"), + patch("apm_cli.install.mcp.command.warn_shell_metachars"), + patch("apm_cli.install.mcp.command.APM_DEPS_AVAILABLE", True), + patch( + "apm_cli.install.mcp.command.MCPIntegrator.prevalidate_registry_dependencies", + side_effect=RuntimeError( + "Could not reach MCP registry at https://registry.example " + "while validating server 'registry-server'; verify reachability." + ), + ), + ): + with pytest.raises(InstallFailureAlreadyRendered): + run_mcp_install( + mcp_name="registry-server", + transport=None, + url=None, + env_pairs=None, + header_pairs=None, + mcp_version=None, + command_argv=None, + dev=False, + force=False, + runtime=None, + exclude=None, + logger=logger, + apm_dir=tmp_path, + scope=None, + ) + + add_mcp.assert_not_called() + logger.error.assert_called_once() + assert "No state was changed" in logger.error.call_args[0][0] + logger.success.assert_not_called() + + def test_direct_registry_missing_server_failure_is_already_rendered( + self, tmp_path: Path + ) -> None: + from apm_cli.install.errors import InstallFailureAlreadyRendered + from apm_cli.install.mcp.command import run_mcp_install + + apm_yml = tmp_path / "apm.yml" + apm_yml.write_text(_APM_YML_MINIMAL, encoding="utf-8") + logger = self._make_logger() + + with ( + patch( + "apm_cli.install.mcp.command.build_mcp_entry", + return_value=("missing-server", False), + ), + patch( + "apm_cli.install.mcp.command.add_mcp_to_apm_yml", + return_value=("added", None), + ) as add_mcp, + patch("apm_cli.install.mcp.command.warn_ssrf_url"), + patch("apm_cli.install.mcp.command.warn_shell_metachars"), + patch("apm_cli.install.mcp.command.APM_DEPS_AVAILABLE", True), + patch( + "apm_cli.install.mcp.command.MCPIntegrator.prevalidate_registry_dependencies", + side_effect=RuntimeError("Cannot install 1 missing server(s)"), + ), + ): + with pytest.raises(InstallFailureAlreadyRendered): + run_mcp_install( + mcp_name="missing-server", + transport=None, + url=None, + env_pairs=None, + header_pairs=None, + mcp_version=None, + command_argv=None, + dev=False, + force=False, + runtime=None, + exclude=None, + logger=logger, + apm_dir=tmp_path, + scope=None, + ) + + add_mcp.assert_not_called() + logger.verbose_detail.assert_not_called() + logger.success.assert_not_called() def test_replaced_dict_entry_no_deps_available(self, tmp_path: Path) -> None: from apm_cli.install.mcp.command import run_mcp_install diff --git a/tests/integration/test_wave2_adapters_coverage.py b/tests/integration/test_wave2_adapters_coverage.py index 14d88e82b2..a983e967bd 100644 --- a/tests/integration/test_wave2_adapters_coverage.py +++ b/tests/integration/test_wave2_adapters_coverage.py @@ -4016,7 +4016,7 @@ def test_remote_ref_dataclass(self) -> None: class TestMCPConflictMatrix: - """Tests for MCP flag conflict validation (E1-E15).""" + """Tests for MCP flag conflict validation.""" def _base_kwargs(self, **overrides) -> dict: """Return base valid kwargs for validate_mcp_conflicts.""" @@ -4030,7 +4030,6 @@ def _base_kwargs(self, **overrides) -> dict: "headers": {}, "mcp_version": None, "command_argv": None, - "global_": False, "only": None, "update": False, "any_transport_flag": False, @@ -4102,15 +4101,6 @@ def test_e1_positional_packages_with_mcp(self) -> None: with pytest.raises(click.UsageError, match="cannot mix"): validate_mcp_conflicts(**self._base_kwargs(pre_dash_packages=["owner/repo"])) - def test_e2_global_with_mcp(self) -> None: - """--global with --mcp raises UsageError.""" - import click - - from apm_cli.install.mcp.conflicts import validate_mcp_conflicts - - with pytest.raises(click.UsageError, match="--global is not supported"): - validate_mcp_conflicts(**self._base_kwargs(global_=True)) - def test_e3_only_apm_with_mcp(self) -> None: """--only apm with --mcp raises UsageError.""" import click diff --git a/tests/integration/test_wave7_policy_registry_coverage.py b/tests/integration/test_wave7_policy_registry_coverage.py index a7bd44f7e7..f9d2b784c5 100644 --- a/tests/integration/test_wave7_policy_registry_coverage.py +++ b/tests/integration/test_wave7_policy_registry_coverage.py @@ -1038,6 +1038,19 @@ def test_network_error_on_non_custom_url_assumes_valid(self, _no_cache_env, monk assert "io.github.acme/server" in valid + def test_network_error_on_non_custom_url_fail_closed_raises(self, _no_cache_env, monkeypatch): + import requests as _req + + from apm_cli.registry.operations import MCPServerOperations + + monkeypatch.delenv("MCP_REGISTRY_URL", raising=False) + ops = MCPServerOperations() + + with patch.object(ops.registry_client.session, "get") as mock_get: + mock_get.side_effect = _req.ConnectionError("network down") + with pytest.raises(RuntimeError, match="Could not reach MCP registry"): + ops.validate_servers_exist(["io.github.acme/server"], fail_closed=True) + def test_network_error_on_custom_url_raises(self, _no_cache_env): import requests as _req diff --git a/tests/spec_conformance/gen_statement.py b/tests/spec_conformance/gen_statement.py index 24f5fb300e..0a917456c6 100644 --- a/tests/spec_conformance/gen_statement.py +++ b/tests/spec_conformance/gen_statement.py @@ -34,6 +34,13 @@ SPEC_VERSION = "v0.1.1" GENERATOR = "gen_statement.py v1" +USER_SCOPE_DISCLOSURE = { + "manifest_location": "~/.apm/apm.yml", + "lockfile_location": "~/.apm/apm.lock.yaml", + "target_capability_declaration": ( + "MCPClientAdapter.supports_user_scope (OpenAPM Target Registry v0.1 implementation profile)" + ), +} def _ensure_coverage() -> dict[str, list[dict[str, str]]]: @@ -147,6 +154,7 @@ def build_json() -> dict: "generator": GENERATOR, "total_requirements": len(entries), "summary_by_class": summary, + "consumer_user_scope": USER_SCOPE_DISCLOSURE, "requirements": entries, } @@ -191,6 +199,14 @@ def build_md(doc: dict) -> str: summary_section = ( "## Coverage summary\n\n" + _md_class_summary(doc["summary_by_class"]) + "\n\n" ) + user_scope = doc["consumer_user_scope"] + scope_section = ( + "## Consumer user-scope disclosure\n\n" + f"- Manifest: `{user_scope['manifest_location']}`\n" + f"- Lockfile: `{user_scope['lockfile_location']}`\n" + "- Target capability declaration: " + f"`{user_scope['target_capability_declaration']}`\n\n" + ) rows = [ "## Per-requirement coverage\n", "| Req ID | Keyword | Sec | Class | Status | Tests | Oracle |", @@ -211,7 +227,7 @@ def build_md(doc: dict) -> str: waivers_section.append(f"- {w}") waivers_section.append("") waivers_md = "\n".join(waivers_section) + "\n" - return preamble + summary_section + table + waivers_md + return preamble + scope_section + summary_section + table + waivers_md def _is_ascii(text: str) -> bool: diff --git a/tests/spec_conformance/test_gen_statement.py b/tests/spec_conformance/test_gen_statement.py index 7a450e45d5..a624981ca0 100644 --- a/tests/spec_conformance/test_gen_statement.py +++ b/tests/spec_conformance/test_gen_statement.py @@ -2,6 +2,7 @@ from __future__ import annotations +import json import subprocess import sys @@ -11,6 +12,7 @@ CONFORMANCE_MD, GENERATOR, SPEC_VERSION, + USER_SCOPE_DISCLOSURE, ) @@ -60,3 +62,13 @@ def test_gen_statement_md_contains_honesty_phrase(): assert "NO automated CI detector" in md, ( "CONFORMANCE.md MUST carry the literal phrase 'NO automated CI detector' (honesty contract)" ) + + +def test_gen_statement_publishes_user_scope_disclosure(): + """req-tg-014 locations and capability contract stay generated.""" + _run_gen() + document = json.loads(CONFORMANCE_JSON.read_text(encoding="ascii")) + assert document["consumer_user_scope"] == USER_SCOPE_DISCLOSURE + markdown = CONFORMANCE_MD.read_text(encoding="ascii") + for value in USER_SCOPE_DISCLOSURE.values(): + assert value in markdown diff --git a/tests/spec_conformance/test_manifest_reqs.py b/tests/spec_conformance/test_manifest_reqs.py index da39115d82..09af537264 100644 --- a/tests/spec_conformance/test_manifest_reqs.py +++ b/tests/spec_conformance/test_manifest_reqs.py @@ -904,6 +904,76 @@ def test_dependency_package_targets_are_restriction_only() -> None: ) +@pytest.mark.req("req-tg-014") +def test_user_scoped_mcp_target_selection_ignores_project_signals( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """User-scope MCP discovery cannot inherit project-only target signals.""" + from apm_cli.core.target_detection import resolve_manifest_target_decision + from apm_cli.integration.mcp_integrator_install import partition_user_scope_runtimes + + project = tmp_path / "project" + project.mkdir() + (project / ".cursor").mkdir() + user_manifest = tmp_path / "home" / ".apm" / "apm.yml" + user_manifest.parent.mkdir(parents=True) + user_manifest.write_text( + "name: user-scope\nversion: 0.1.0\ndependencies:\n mcp: []\n", + encoding="ascii", + ) + monkeypatch.setattr("apm_cli.config.get_install_target", lambda: None) + + discovery = resolve_manifest_target_decision( + project, + manifest_path=user_manifest, + explicit_target=None, + user_scope=True, + ) + explicit = resolve_manifest_target_decision( + project, + manifest_path=user_manifest, + explicit_target="codex", + user_scope=True, + ) + manifest_target_path = user_manifest.with_name("manifest-target.yml") + manifest_target_path.write_text( + "name: user-scope\nversion: 0.1.0\ntargets: [claude]\n", + encoding="ascii", + ) + manifest = resolve_manifest_target_decision( + project, + manifest_path=manifest_target_path, + explicit_target=None, + user_scope=True, + ) + configured_path = user_manifest.with_name("configured-target.yml") + configured_path.write_text( + "name: user-scope\nversion: 0.1.0\n", + encoding="ascii", + ) + monkeypatch.setattr("apm_cli.config.get_install_target", lambda: "vscode") + configured = resolve_manifest_target_decision( + project, + manifest_path=configured_path, + explicit_target=None, + user_scope=True, + ) + + assert (discovery.value, discovery.source) == (None, "auto-detect") + assert (explicit.value, explicit.source) == ("codex", "--target flag") + assert (manifest.value, manifest.source) == (["claude"], "apm.yml") + assert (configured.value, configured.source) == ("vscode", "apm config target") + assert partition_user_scope_runtimes(["vscode", "claude"]) == (["claude"], ["vscode"]) + assert_spec_contains( + "User-scoped MCP target selection", + "Once a source selects one or more\ntargets, the consumer MUST NOT consult", + "Project-scoped\ntarget-detection signals outside the user scope MUST NOT", + "Before creating or modifying the user-scope manifest, lockfile, or target", + "For a mixed set, the supported\nsubset MUST become the effective target set", + ) + + @pytest.mark.req("req-tg-009") def test_kiro_agent_tools_gate_fails_closed_before_adopt(tmp_path: Path) -> None: """Fail-closed evaluation precedes any content-identity adoption fast-path. diff --git a/tests/unit/install/test_mcp_conflicts.py b/tests/unit/install/test_mcp_conflicts.py index 1fe785a8f9..c2ab49931a 100644 --- a/tests/unit/install/test_mcp_conflicts.py +++ b/tests/unit/install/test_mcp_conflicts.py @@ -1,4 +1,4 @@ -"""Unit tests for the MCP flag-conflict matrix (E1-E15). +"""Unit tests for the MCP flag-conflict matrix. Covers ``apm_cli.install.mcp.conflicts.validate_mcp_conflicts`` and the ``MCP_REQUIRED_FLAGS`` constant. All tests are pure-Python with no I/O. @@ -31,7 +31,6 @@ def _call(**overrides) -> None: headers={}, mcp_version=None, command_argv=None, - global_=False, only=None, update=False, any_transport_flag=False, @@ -160,20 +159,6 @@ def test_empty_pre_dash_packages_ok(self) -> None: _call(pre_dash_packages=[]) -# --------------------------------------------------------------------------- -# E2 - --global not supported for MCP entries -# --------------------------------------------------------------------------- - - -class TestE2GlobalNotSupportedForMcp: - def test_global_raises(self) -> None: - with pytest.raises(click.UsageError, match=r"--global is not supported"): - _call(global_=True) - - def test_not_global_ok(self) -> None: - _call(global_=False) - - # --------------------------------------------------------------------------- # E3 - --only apm conflicts with --mcp # --------------------------------------------------------------------------- diff --git a/tests/unit/install/test_mcp_registry_module.py b/tests/unit/install/test_mcp_registry_module.py index c3ff811aa8..de2bb5971e 100644 --- a/tests/unit/install/test_mcp_registry_module.py +++ b/tests/unit/install/test_mcp_registry_module.py @@ -11,6 +11,7 @@ import typing from unittest.mock import MagicMock +import click import pytest from apm_cli.install.mcp.registry import ( @@ -217,6 +218,14 @@ def test_http_url_sets_allow_http(self, monkeypatch): assert os.environ.get("MCP_REGISTRY_ALLOW_HTTP") == "1" assert "MCP_REGISTRY_ALLOW_HTTP" not in os.environ + def test_http_url_preserves_separate_opt_in_when_disabled(self, monkeypatch): + monkeypatch.delenv("MCP_REGISTRY_ALLOW_HTTP", raising=False) + import os + + with registry_env_override("http://intranet.example.com", allow_http=False): + assert "MCP_REGISTRY_ALLOW_HTTP" not in os.environ + assert "MCP_REGISTRY_ALLOW_HTTP" not in os.environ + def test_none_is_no_op(self, monkeypatch): monkeypatch.delenv("MCP_REGISTRY_URL", raising=False) import os @@ -232,6 +241,21 @@ class TestValidateRegistryUrl: def test_https_accepted(self): validate_registry_url("https://mcp.example.com") + def test_credentials_rejected(self): + with pytest.raises(click.UsageError, match="embedded credentials are not supported"): + validate_registry_url("https://user:secret@mcp.example.com") + + @pytest.mark.parametrize( + "url", + ( + "https://mcp.example.com?token=secret", + "https://mcp.example.com#token=secret", + ), + ) + def test_query_and_fragment_rejected(self, url): + with pytest.raises(click.UsageError, match="must not contain a query or fragment"): + validate_registry_url(url) + def test_http_accepted(self): validate_registry_url("http://intranet.example.com") @@ -332,10 +356,33 @@ def test_diagnostic_does_not_leak_credentials(self, monkeypatch): """End-to-end: B3 env-source diagnostic must redact creds.""" monkeypatch.setenv("MCP_REGISTRY_URL", "https://u:topsecret@x.example.com/") logger = MagicMock() - resolve_registry_url(None, logger=logger) - msg = logger.progress.call_args.args[0] - assert "topsecret" not in msg - assert "u:" not in msg + with pytest.raises(click.UsageError) as raised: + resolve_registry_url(None, logger=logger) + assert "topsecret" not in str(raised.value) + logger.progress.assert_not_called() + + def test_malformed_ambient_value_is_redacted_from_override_diagnostic(self, monkeypatch): + secret = "REGISTRY_SECRET_SENTINEL" + monkeypatch.setenv("MCP_REGISTRY_URL", secret) + logger = MagicMock() + + resolved, source = resolve_registry_url("https://registry.example.com", logger=logger) + + assert resolved == "https://registry.example.com" + assert source == "flag" + message = logger.progress.call_args.args[0] + assert secret not in message + assert "" in message + + def test_malformed_ambient_value_is_redacted_from_validation_error(self, monkeypatch): + secret = "REGISTRY_SECRET_SENTINEL" + monkeypatch.setenv("MCP_REGISTRY_URL", secret) + + with pytest.raises(click.UsageError) as raised: + resolve_registry_url(None) + + assert secret not in str(raised.value) + assert "" in str(raised.value) class TestSsrfWarning: diff --git a/tests/unit/scripts/test_architecture_runner.py b/tests/unit/scripts/test_architecture_runner.py index 3c9fedce91..e243ae82d5 100644 --- a/tests/unit/scripts/test_architecture_runner.py +++ b/tests/unit/scripts/test_architecture_runner.py @@ -623,6 +623,7 @@ def exiting_import( install-deployment-gitlab-facade-orchestration install-deployment-gitlab-policy-adapter install-deployment-incomplete-chain-routing +install-deployment-install-scope-selection install-deployment-local-bundle-policy-preflight install-deployment-local-identity-anchor install-deployment-locked-skill-subset-reconstruction diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index 34459d8a20..b95c412a30 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -223,6 +223,17 @@ def test_set_and_get_round_trip(self, isolated_config): config_mod._invalidate_config_cache() assert config_mod.get_mcp_registry_url() == "https://corp.mcp.example.com" + @pytest.mark.parametrize( + "url", + ( + "https://corp.mcp.example.com?token=secret", + "https://corp.mcp.example.com#token=secret", + ), + ) + def test_set_rejects_query_and_fragment(self, isolated_config, url): + with pytest.raises(ValueError, match="must not contain a query or fragment"): + config_mod.set_mcp_registry_url(url) + def test_set_strips_trailing_slash(self, isolated_config): config_mod.set_mcp_registry_url("https://corp.mcp.example.com/") assert config_mod.get_mcp_registry_url() == "https://corp.mcp.example.com" @@ -248,7 +259,7 @@ def test_set_rejects_missing_netloc(self, isolated_config): config_mod.set_mcp_registry_url("https://") def test_set_rejects_embedded_credentials(self, isolated_config): - with pytest.raises(ValueError, match="must not contain credentials"): + with pytest.raises(ValueError, match="embedded credentials are not supported"): config_mod.set_mcp_registry_url("https://user:token@corp.mcp.example.com") def test_unset_removes_key(self, isolated_config): @@ -259,3 +270,16 @@ def test_unset_removes_key(self, isolated_config): def test_unset_is_noop_when_absent(self, isolated_config): config_mod.unset_mcp_registry_url() assert config_mod.get_mcp_registry_url() is None + + def test_set_redacts_credentials_from_missing_scheme_error(self, isolated_config): + secret = "REGISTRY_SECRET_SENTINEL" + with pytest.raises(ValueError) as raised: + config_mod.set_mcp_registry_url(f"//user:{secret}@corp.mcp.example.com") + assert secret not in str(raised.value) + assert "" in str(raised.value) + + def test_set_rejects_invalid_port_without_echoing_value(self, isolated_config): + secret = "config-port-secret" + with pytest.raises(ValueError) as raised: + config_mod.set_mcp_registry_url(f"https://corp.mcp.example.com:{secret}") + assert secret not in str(raised.value) diff --git a/tests/unit/test_config_command.py b/tests/unit/test_config_command.py index 9456978521..c9040e64ab 100644 --- a/tests/unit/test_config_command.py +++ b/tests/unit/test_config_command.py @@ -1741,6 +1741,7 @@ def test_set_valid_http_url(self): ) assert result.exit_code == 0 mock_set.assert_called_once_with("http://internal.corp/mcp") + assert "MCP_REGISTRY_ALLOW_HTTP=1" in result.output def test_set_invalid_scheme_rejected(self): with patch( diff --git a/tests/unit/test_install_command.py b/tests/unit/test_install_command.py index 842864d605..617f679e7c 100644 --- a/tests/unit/test_install_command.py +++ b/tests/unit/test_install_command.py @@ -13,6 +13,7 @@ from click.testing import CliRunner from apm_cli.cli import cli +from apm_cli.core.scope import InstallScope from apm_cli.models.results import InstallResult @@ -2168,11 +2169,365 @@ def test_e1_mcp_with_positional_packages(self): assert result.exit_code == 2 assert "cannot mix --mcp with positional packages" in result.output - def test_e2_mcp_with_global(self): - with self._chdir_with_apm_yml(): - result = self.runner.invoke(cli, ["install", "--mcp", "foo", "--global"]) - assert result.exit_code == 2 - assert "project-scoped" in result.output + def test_global_mcp_uses_user_manifest_scope(self, tmp_path, monkeypatch): + fake_home = tmp_path / "home" + user_apm_dir = fake_home / ".apm" + user_apm_dir.mkdir(parents=True) + user_manifest = user_apm_dir / "apm.yml" + user_manifest.write_text( + yaml.safe_dump( + { + "name": "user-scope", + "version": "0.1.0", + "dependencies": {"mcp": []}, + } + ), + encoding="utf-8", + ) + project = tmp_path / "project" + project.mkdir() + project_manifest = project / "apm.yml" + project_manifest.write_text( + yaml.safe_dump( + { + "name": "project-scope", + "version": "0.1.0", + "targets": ["cursor"], + "dependencies": {"mcp": []}, + } + ), + encoding="utf-8", + ) + monkeypatch.chdir(project) + argv = [ + "apm", + "install", + "-g", + "--mcp", + "probe", + "--no-policy", + "--", + "echo", + "ready", + ] + + with ( + patch.object(Path, "home", return_value=fake_home), + patch("apm_cli.commands.install._get_invocation_argv", return_value=argv), + patch( + "apm_cli.integration.mcp_integrator_install.discover_user_scope_mcp_runtimes", + return_value=(["claude"], []), + ), + patch("apm_cli.install.mcp.command.MCPIntegrator") as integrator, + ): + result = self.runner.invoke(cli, argv[1:]) + + assert result.exit_code == 0, result.output + assert yaml.safe_load(user_manifest.read_text(encoding="utf-8"))["dependencies"]["mcp"] == [ + { + "args": ["ready"], + "command": "echo", + "name": "probe", + "registry": False, + "transport": "stdio", + } + ] + assert ( + yaml.safe_load(project_manifest.read_text(encoding="utf-8"))["dependencies"]["mcp"] + == [] + ) + install_call = integrator.install.call_args + assert install_call.kwargs["scope"] is InstallScope.USER + assert install_call.kwargs["user_scope"] is True + assert install_call.kwargs["project_root"] == user_apm_dir + assert install_call.kwargs["target_decision"].value == ["claude"] + assert install_call.kwargs["target_decision"].source == "auto-detect" + + def test_global_mcp_rejects_workspace_only_target_before_manifest_write( + self, tmp_path, monkeypatch + ): + fake_home = tmp_path / "home" + user_apm_dir = fake_home / ".apm" + user_manifest = user_apm_dir / "apm.yml" + project = tmp_path / "project" + project.mkdir() + monkeypatch.chdir(project) + argv = [ + "apm", + "install", + "-g", + "--target", + "vscode", + "--mcp", + "probe", + "--no-policy", + "--", + "echo", + "ready", + ] + + with ( + patch.object(Path, "home", return_value=fake_home), + patch("apm_cli.commands.install._get_invocation_argv", return_value=argv), + ): + result = self.runner.invoke(cli, argv[1:]) + + assert result.exit_code == 2, (result.output, result.exception) + assert "enable selected experimental targets" not in result.output + assert "vscode; source: --target flag" in result.output + assert "choose a global-capable --target" in result.output + assert "Skipped workspace-only runtimes" not in result.output + assert not user_manifest.exists() + + def test_global_mcp_names_disabled_experimental_target(self, tmp_path, monkeypatch): + fake_home = tmp_path / "home" + project = tmp_path / "project" + project.mkdir() + monkeypatch.chdir(project) + argv = [ + "apm", + "install", + "-g", + "--target", + "hermes", + "--mcp", + "probe", + "--no-policy", + "--", + "echo", + "ready", + ] + + with ( + patch.object(Path, "home", return_value=fake_home), + patch("apm_cli.commands.install._get_invocation_argv", return_value=argv), + ): + result = self.runner.invoke(cli, argv[1:]) + + assert result.exit_code == 2, (result.output, result.exception) + assert "hermes; source: --target flag" in result.output + assert "enable selected experimental targets" in result.output + + def test_global_mcp_rejects_unsupported_saved_target_without_fallback( + self, tmp_path, monkeypatch + ): + fake_home = tmp_path / "home" + user_manifest = fake_home / ".apm" / "apm.yml" + project = tmp_path / "project" + project.mkdir() + (project / ".claude").mkdir() + monkeypatch.chdir(project) + argv = [ + "apm", + "install", + "-g", + "--mcp", + "probe", + "--no-policy", + "--", + "echo", + "ready", + ] + + with ( + patch.object(Path, "home", return_value=fake_home), + patch("apm_cli.config.get_install_target", return_value="vscode"), + patch("apm_cli.commands.install._get_invocation_argv", return_value=argv), + ): + result = self.runner.invoke(cli, argv[1:]) + + assert result.exit_code == 2 + assert "enable selected experimental targets" not in result.output + assert "vscode; source: apm config target" in result.output + assert "choose a global-capable --target" in result.output + assert "Skipped workspace-only runtimes" not in result.output + assert not user_manifest.exists() + + def test_global_mcp_filters_mixed_target_set_before_dispatch(self, tmp_path, monkeypatch): + fake_home = tmp_path / "home" + project = tmp_path / "project" + project.mkdir() + monkeypatch.chdir(project) + argv = [ + "apm", + "install", + "-g", + "--target", + "vscode,claude", + "--mcp", + "probe", + "--no-policy", + "--", + "echo", + "ready", + ] + + with ( + patch.object(Path, "home", return_value=fake_home), + patch("apm_cli.commands.install._get_invocation_argv", return_value=argv) as invocation, + patch("apm_cli.commands.install._run_mcp_install") as run_mcp_install, + ): + result = self.runner.invoke(cli, argv[1:]) + excluded_argv = [ + "apm", + "install", + "-g", + "--target", + "vscode,claude", + "--exclude", + "vscode", + "--mcp", + "probe-three", + "--no-policy", + "--", + "echo", + "ready", + ] + invocation.return_value = excluded_argv + excluded = self.runner.invoke(cli, excluded_argv[1:]) + + assert result.exit_code == 0, result.output + assert excluded.exit_code == 0, excluded.output + first_call, excluded_call = run_mcp_install.call_args_list + decision = first_call.kwargs["target_decision"] + assert decision.value == ["claude"] + assert first_call.kwargs["initial_manifest_config"]["targets"] == ["claude"] + assert excluded_call.kwargs["target_decision"].value == ["claude"] + assert "Skipped workspace-only runtimes at user scope: vscode" in result.output + assert "Skipped workspace-only runtimes" not in excluded.output + + def test_global_mcp_dry_run_creates_no_user_state(self, tmp_path, monkeypatch): + fake_home = tmp_path / "home" + project = tmp_path / "project" + project.mkdir() + monkeypatch.chdir(project) + argv = [ + "apm", + "install", + "-g", + "--target", + "claude", + "--mcp", + "probe", + "--dry-run", + "--no-policy", + "--", + "echo", + "ready", + ] + + with ( + patch.object(Path, "home", return_value=fake_home), + patch("apm_cli.commands.install._get_invocation_argv", return_value=argv), + ): + result = self.runner.invoke(cli, argv[1:]) + + assert result.exit_code == 0, result.output + assert "would add MCP server" in result.output + assert not (fake_home / ".apm" / "apm.yml").exists() + + def test_mcp_dry_run_env_prevalidation_uses_parsed_pairs(self, tmp_path, monkeypatch): + fake_home = tmp_path / "home" + project = tmp_path / "project" + project.mkdir() + monkeypatch.chdir(project) + argv = [ + "apm", + "install", + "--target", + "claude", + "--mcp", + "probe", + "--env", + "FOO=bar", + "--dry-run", + "--no-policy", + "--", + "echo", + "ready", + ] + + with ( + patch.object(Path, "home", return_value=fake_home), + patch("apm_cli.commands.install._get_invocation_argv", return_value=argv), + ): + result = self.runner.invoke(cli, argv[1:]) + + assert result.exit_code == 0, result.output + assert "would add MCP server" in result.output + assert "dictionary update sequence" not in result.output + assert not (project / "apm.yml").exists() + + def test_mcp_dry_run_header_prevalidation_uses_parsed_pairs(self, tmp_path, monkeypatch): + fake_home = tmp_path / "home" + project = tmp_path / "project" + project.mkdir() + monkeypatch.chdir(project) + argv = [ + "apm", + "install", + "--target", + "claude", + "--mcp", + "probe", + "--url", + "https://example.com/mcp", + "--header", + "X-Test=1", + "--dry-run", + "--no-policy", + ] + + with ( + patch.object(Path, "home", return_value=fake_home), + patch("apm_cli.commands.install._get_invocation_argv", return_value=argv), + ): + result = self.runner.invoke(cli, argv[1:]) + + assert result.exit_code == 0, result.output + assert "would add MCP server" in result.output + assert "dictionary update sequence" not in result.output + assert not (project / "apm.yml").exists() + + def test_global_registry_mcp_dry_run_prevalidates_before_preview(self, tmp_path, monkeypatch): + fake_home = tmp_path / "home" + project = tmp_path / "project" + project.mkdir() + monkeypatch.chdir(project) + argv = [ + "apm", + "install", + "-g", + "--target", + "claude", + "--mcp", + "io.github.test/srv", + "--registry", + "https://custom.test", + "--dry-run", + "--no-policy", + ] + failure = ( + "Could not reach MCP registry at https://custom.test while validating " + "server 'io.github.test/srv'; verify the --registry URL and registry reachability." + ) + + with ( + patch.object(Path, "home", return_value=fake_home), + patch("apm_cli.commands.install._get_invocation_argv", return_value=argv), + patch("apm_cli.install.mcp.command.APM_DEPS_AVAILABLE", True), + patch( + "apm_cli.install.mcp.command.MCPIntegrator.prevalidate_registry_dependencies", + side_effect=RuntimeError(failure), + ), + ): + result = self.runner.invoke(cli, argv[1:]) + + assert result.exit_code == 1, result.output + assert "Could not reach MCP registry" in result.output + assert "verify the --registry URL" in result.output + assert "would add MCP server" not in result.output + assert not (fake_home / ".apm" / "apm.yml").exists() def test_e3_mcp_with_only_apm(self): with self._chdir_with_apm_yml(): @@ -2606,6 +2961,62 @@ def test_registry_flag_overrides_env_var(self): data = yaml.safe_load((tmp / "apm.yml").read_text()) assert data["dependencies"]["mcp"][0]["registry"] == "https://flag.example.com" + def test_registry_config_url_reaches_install_integration(self, monkeypatch): + """The persisted registry override must reach the integration call.""" + configured_url = "https://config.example.com" + monkeypatch.delenv("MCP_REGISTRY_URL", raising=False) + argv = ["apm", "install", "--mcp", "srv", "--no-policy"] + with ( + self._chdir_with_apm_yml(), + patch("apm_cli.commands.install._get_invocation_argv", return_value=argv), + patch("apm_cli.config.get_mcp_registry_url", return_value=configured_url), + patch("apm_cli.commands.install._run_mcp_install") as run_mcp_install, + ): + result = self.runner.invoke(cli, argv[1:]) + + assert result.exit_code == 0, result.output + run_mcp_install.assert_called_once() + assert run_mcp_install.call_args.kwargs["registry_url"] == configured_url + assert run_mcp_install.call_args.kwargs["registry_allow_http"] is False + assert "(from apm config)" in result.output + + def test_registry_env_url_pins_source_without_http_opt_in(self, monkeypatch): + """A valid ambient endpoint is persisted without relaxing HTTP policy.""" + configured_url = "https://env.example.com" + monkeypatch.setenv("MCP_REGISTRY_URL", configured_url) + argv = ["apm", "install", "--mcp", "srv", "--no-policy"] + with ( + self._chdir_with_apm_yml(), + patch("apm_cli.commands.install._get_invocation_argv", return_value=argv), + patch("apm_cli.commands.install._run_mcp_install") as run_mcp_install, + ): + result = self.runner.invoke(cli, argv[1:]) + + assert result.exit_code == 0, result.output + run_mcp_install.assert_called_once() + assert run_mcp_install.call_args.kwargs["registry_url"] == configured_url + assert run_mcp_install.call_args.kwargs["registry_allow_http"] is False + + def test_invalid_configured_registry_url_fails_before_dispatch(self, monkeypatch): + """Stale persisted URLs must pass current validation before use.""" + monkeypatch.delenv("MCP_REGISTRY_URL", raising=False) + argv = ["apm", "install", "--mcp", "srv", "--no-policy"] + with ( + self._chdir_with_apm_yml(), + patch("apm_cli.commands.install._get_invocation_argv", return_value=argv), + patch( + "apm_cli.config.get_mcp_registry_url", + return_value="https://config.example.com?token=secret", + ), + patch("apm_cli.commands.install._run_mcp_install") as run_mcp_install, + ): + result = self.runner.invoke(cli, argv[1:]) + + assert result.exit_code == 2 + assert "Configured mcp-registry-url is invalid" in result.output + assert "secret" not in result.output + run_mcp_install.assert_not_called() + def test_registry_with_version_overlay_persists_both(self): with ( self._chdir_with_apm_yml() as tmp, diff --git a/tests/unit/test_mcp_integrator_install_hermetic.py b/tests/unit/test_mcp_integrator_install_hermetic.py index 049280606b..2451663d6a 100644 --- a/tests/unit/test_mcp_integrator_install_hermetic.py +++ b/tests/unit/test_mcp_integrator_install_hermetic.py @@ -120,9 +120,13 @@ def test_single_runtime_targets_only_that_runtime(self, tmp_path): logger.mcp_lookup_heartbeat = MagicMock() mock_ops = MagicMock() - mock_ops.validate_servers_exist.return_value = (["my-server"], []) + + def validate(_names, **kwargs): + kwargs["server_info_cache"]["my-server"] = {"packages": []} + return ["my-server"], [] + + mock_ops.validate_servers_exist.side_effect = validate mock_ops.check_servers_needing_installation.return_value = ["my-server"] - mock_ops.batch_fetch_server_info.return_value = {"my-server": {"packages": []}} mock_ops.collect_environment_variables.return_value = {} mock_ops.collect_runtime_variables.return_value = {} @@ -147,6 +151,71 @@ def test_single_runtime_targets_only_that_runtime(self, tmp_path): logger=logger, ) assert result >= 0 + mock_ops.batch_fetch_server_info.assert_not_called() + + def test_prevalidated_server_is_not_validated_twice(self, tmp_path): + from apm_cli.integration.mcp_integrator_install import run_mcp_install + + dep = self._make_reg_dep("my-server") + logger = MagicMock() + mock_ops = MagicMock() + mock_ops.check_servers_needing_installation.return_value = ["my-server"] + mock_ops.collect_environment_variables.return_value = {} + mock_ops.collect_runtime_variables.return_value = {} + + with ( + patch( + "apm_cli.registry.operations.MCPServerOperations", + return_value=mock_ops, + ), + patch( + "apm_cli.integration.mcp_integrator.MCPIntegrator._gate_project_scoped_runtimes", + side_effect=lambda runtimes, **_kwargs: runtimes, + ), + patch( + "apm_cli.integration.mcp_integrator.MCPIntegrator._install_for_runtime", + return_value=True, + ), + ): + run_mcp_install( + mcp_deps=[dep], + runtime="copilot", + project_root=tmp_path, + logger=logger, + prevalidated_registry_servers={"my-server": {"packages": []}}, + ) + + mock_ops.validate_servers_exist.assert_not_called() + mock_ops.batch_fetch_server_info.assert_not_called() + + +def test_direct_registry_prevalidation_fails_closed(): + from apm_cli.integration.mcp_integrator_install import prevalidate_registry_dependencies + + dep = MagicMock() + dep.name = "my-server" + operations = MagicMock() + + def validate(_names, **kwargs): + kwargs["server_info_cache"]["my-server"] = {"packages": []} + return ["my-server"], [] + + operations.validate_servers_exist.side_effect = validate + + with patch( + "apm_cli.registry.operations.MCPServerOperations", + return_value=operations, + ): + result = prevalidate_registry_dependencies( + [dep], + registry_url=None, + verbose=False, + logger=MagicMock(), + ) + + operations.validate_servers_exist.assert_called_once() + assert operations.validate_servers_exist.call_args.kwargs["fail_closed"] is True + assert result == {"my-server": {"packages": []}} # --------------------------------------------------------------------------- diff --git a/tests/unit/test_mcp_overlays.py b/tests/unit/test_mcp_overlays.py index 7ffa654e1b..3a0225af6a 100644 --- a/tests/unit/test_mcp_overlays.py +++ b/tests/unit/test_mcp_overlays.py @@ -1,7 +1,7 @@ """Tests for MCP overlay functionality: MCPDependency model, self-defined server info building, overlay application, and install flow integration.""" -from unittest.mock import MagicMock, patch # noqa: F401 +from unittest.mock import ANY, MagicMock, patch # noqa: F401 import pytest @@ -847,7 +847,7 @@ def test_registry_deps_use_dep_names(self, mock_ops_cls, _console, mock_install_ count = MCPIntegrator.install([dep], runtime="vscode", explicit_target="vscode") mock_ops.validate_servers_exist.assert_called_once_with( - ["io.github.github/github-mcp-server"] + ["io.github.github/github-mcp-server"], server_info_cache=ANY ) assert count == 1 @@ -878,7 +878,7 @@ def test_mixed_deps_both_paths(self, mock_ops_cls, _console, mock_install_runtim # Registry dep goes through validation mock_ops.validate_servers_exist.assert_called_once_with( - ["io.github.github/github-mcp-server"] + ["io.github.github/github-mcp-server"], server_info_cache=ANY ) # Both deps result in _install_for_runtime calls (1 registry + 1 self-defined) assert mock_install_runtime.call_count == 2 diff --git a/tests/unit/test_registry_client.py b/tests/unit/test_registry_client.py index e1412ba37c..24fc594103 100644 --- a/tests/unit/test_registry_client.py +++ b/tests/unit/test_registry_client.py @@ -312,19 +312,58 @@ def test_environment_variable_override(self): client = SimpleRegistryClient("https://explicit-url.example.com") self.assertEqual(client.registry_url, "https://explicit-url.example.com") - @mock.patch("apm_cli.registry.client.SimpleRegistryClient.search_servers") - def test_find_server_by_reference_uuid_input_returns_none(self, mock_search_servers): + def test_environment_query_and_fragment_are_rejected_without_leaking(self): + secret = "registry-query-secret" + with ( + mock.patch.dict( + os.environ, + {"MCP_REGISTRY_URL": f"https://registry.example.com?token={secret}#{secret}"}, + clear=False, + ), + self.assertRaises(ValueError) as raised, + ): + SimpleRegistryClient() + + self.assertNotIn(secret, str(raised.exception)) + + def test_malformed_port_is_rejected_without_leaking_userinfo(self): + secret = "registry-port-secret" + with ( + mock.patch.dict( + os.environ, + {"MCP_REGISTRY_URL": f"https://user:{secret}@registry.example.com:notaport"}, + clear=False, + ), + self.assertRaises(ValueError) as raised, + ): + SimpleRegistryClient() + + self.assertNotIn(secret, str(raised.exception)) + + def test_malformed_port_is_rejected_without_leaking_netloc(self): + secret = "PORT_SECRET_SENTINEL" + with self.assertRaises(ValueError) as raised: + SimpleRegistryClient(f"https://registry.example.com:{secret}") + + self.assertNotIn(secret, str(raised.exception)) + + @mock.patch("apm_cli.registry.client.SimpleRegistryClient._search_servers_all_pages") + def test_find_server_by_reference_uuid_input_returns_none(self, mock_search_servers_all_pages): """The legacy UUID strategy is removed; UUID-shaped refs route to search and miss.""" - mock_search_servers.return_value = [] + mock_search_servers_all_pages.return_value = [] result = self.client.find_server_by_reference("123e4567-e89b-12d3-a456-426614174000") self.assertIsNone(result) - mock_search_servers.assert_called_once_with("123e4567-e89b-12d3-a456-426614174000") + mock_search_servers_all_pages.assert_called_once_with( + "123e4567-e89b-12d3-a456-426614174000" + ) @mock.patch("apm_cli.registry.client.SimpleRegistryClient.get_server") - @mock.patch("apm_cli.registry.client.SimpleRegistryClient.search_servers") - def test_find_server_by_reference_name_match(self, mock_search_servers, mock_get_server): + @mock.patch("apm_cli.registry.client.SimpleRegistryClient._search_servers_all_pages") + def test_find_server_by_reference_name_match( + self, mock_search_servers_all_pages, mock_get_server + ): """Test finding a server by exact name match (v0.1 shape).""" - mock_search_servers.return_value = [ + mock_search_servers_all_pages.return_value = [ {"name": "io.github.owner/repo-name"}, {"name": "other-server"}, ] @@ -334,13 +373,13 @@ def test_find_server_by_reference_name_match(self, mock_search_servers, mock_get result = self.client.find_server_by_reference("io.github.owner/repo-name") self.assertEqual(result, server_data) - mock_search_servers.assert_called_once_with("io.github.owner/repo-name") + mock_search_servers_all_pages.assert_called_once_with("io.github.owner/repo-name") mock_get_server.assert_called_once_with("io.github.owner/repo-name") - @mock.patch("apm_cli.registry.client.SimpleRegistryClient.search_servers") - def test_find_server_by_reference_name_not_found(self, mock_search_servers): + @mock.patch("apm_cli.registry.client.SimpleRegistryClient._search_servers_all_pages") + def test_find_server_by_reference_name_not_found(self, mock_search_servers_all_pages): """Test finding a server by name that doesn't exist in registry.""" - mock_search_servers.return_value = [ + mock_search_servers_all_pages.return_value = [ {"name": "io.github.owner/different-repo"}, {"name": "other-server"}, ] @@ -348,38 +387,38 @@ def test_find_server_by_reference_name_not_found(self, mock_search_servers): result = self.client.find_server_by_reference("ghcr.io/github/github-mcp-server") self.assertIsNone(result) - mock_search_servers.assert_called_once_with("ghcr.io/github/github-mcp-server") + mock_search_servers_all_pages.assert_called_once_with("ghcr.io/github/github-mcp-server") @mock.patch("apm_cli.registry.client.SimpleRegistryClient.get_server") - @mock.patch("apm_cli.registry.client.SimpleRegistryClient.search_servers") + @mock.patch("apm_cli.registry.client.SimpleRegistryClient._search_servers_all_pages") def test_find_server_by_reference_name_match_get_server_fails( - self, mock_search_servers, mock_get_server + self, mock_search_servers_all_pages, mock_get_server ): """When get_server raises ValueError (e.g. ServerNotFoundError), return None.""" - mock_search_servers.return_value = [{"name": "test-server"}] + mock_search_servers_all_pages.return_value = [{"name": "test-server"}] mock_get_server.side_effect = ValueError("Server not found") result = self.client.find_server_by_reference("test-server") self.assertIsNone(result) - mock_search_servers.assert_called_once_with("test-server") + mock_search_servers_all_pages.assert_called_once_with("test-server") @mock.patch("apm_cli.registry.client.SimpleRegistryClient.get_server") - @mock.patch("apm_cli.registry.client.SimpleRegistryClient.search_servers") + @mock.patch("apm_cli.registry.client.SimpleRegistryClient._search_servers_all_pages") def test_find_server_by_reference_name_match_network_error_propagates( - self, mock_search_servers, mock_get_server + self, mock_search_servers_all_pages, mock_get_server ): """Test that network errors in get_server propagate to the caller.""" - mock_search_servers.return_value = [{"name": "test-server"}] + mock_search_servers_all_pages.return_value = [{"name": "test-server"}] mock_get_server.side_effect = requests.ConnectionError("Network error") with self.assertRaises(requests.ConnectionError): self.client.find_server_by_reference("test-server") - @mock.patch("apm_cli.registry.client.SimpleRegistryClient.search_servers") - def test_find_server_by_reference_invalid_format(self, mock_search_servers): + @mock.patch("apm_cli.registry.client.SimpleRegistryClient._search_servers_all_pages") + def test_find_server_by_reference_invalid_format(self, mock_search_servers_all_pages): """Test finding a server with various invalid/edge case formats.""" - mock_search_servers.return_value = [] + mock_search_servers_all_pages.return_value = [] test_cases = [ "", @@ -395,10 +434,12 @@ def test_find_server_by_reference_invalid_format(self, mock_search_servers): self.assertIsNone(result) @mock.patch("apm_cli.registry.client.SimpleRegistryClient.get_server") - @mock.patch("apm_cli.registry.client.SimpleRegistryClient.search_servers") - def test_find_server_by_reference_no_slug_collision(self, mock_search_servers, mock_get_server): + @mock.patch("apm_cli.registry.client.SimpleRegistryClient._search_servers_all_pages") + def test_find_server_by_reference_no_slug_collision( + self, mock_search_servers_all_pages, mock_get_server + ): """Test that qualified names don't collide on shared slugs (bug #165).""" - mock_search_servers.return_value = [ + mock_search_servers_all_pages.return_value = [ {"name": "com.supabase/mcp"}, {"name": "microsoftdocs/mcp"}, ] @@ -411,12 +452,66 @@ def test_find_server_by_reference_no_slug_collision(self, mock_search_servers, m mock_get_server.assert_called_once_with("microsoftdocs/mcp") @mock.patch("apm_cli.registry.client.SimpleRegistryClient.get_server") - @mock.patch("apm_cli.registry.client.SimpleRegistryClient.search_servers") + @mock.patch("apm_cli.registry.client.SimpleRegistryClient._search_servers_all_pages") + def test_find_server_by_reference_rejects_ambiguous_slug( + self, mock_search_servers_all_pages, mock_get_server + ): + """An unqualified slug must not select the registry's first match.""" + mock_search_servers_all_pages.return_value = [ + {"name": "com.attacker/tool"}, + {"name": "org.trusted/tool"}, + ] + + result = self.client.find_server_by_reference("tool") + + self.assertIsNone(result) + mock_get_server.assert_not_called() + + @mock.patch("apm_cli.registry.client.SimpleRegistryClient.get_server") + def test_find_server_by_reference_rejects_ambiguous_slug_across_pages(self, mock_get_server): + """An unqualified slug must inspect all search pages before selection.""" + self.client._cached_get_json = mock.Mock( + side_effect=[ + ( + { + "servers": [{"server": {"name": "com.attacker/tool"}}], + "metadata": {"nextCursor": "page-2"}, + }, + {}, + ), + ( + { + "servers": [{"server": {"name": "org.trusted/tool"}}], + "metadata": {}, + }, + {}, + ), + ] + ) + + result = self.client.find_server_by_reference("tool") + + self.assertIsNone(result) + mock_get_server.assert_not_called() + self.assertEqual( + [call.kwargs["params"] for call in self.client._cached_get_json.call_args_list], + [{"search": "tool"}, {"search": "tool", "cursor": "page-2"}], + ) + + def test_constructor_rejects_registry_url_with_embedded_credentials(self): + credential = ":".join(("registry-user", "registry-password")) + registry_url = f"https://{credential}@registry.example.com" + + with self.assertRaisesRegex(ValueError, "embedded credentials"): + SimpleRegistryClient(registry_url) + + @mock.patch("apm_cli.registry.client.SimpleRegistryClient.get_server") + @mock.patch("apm_cli.registry.client.SimpleRegistryClient._search_servers_all_pages") def test_find_server_by_reference_qualified_no_match( - self, mock_search_servers, mock_get_server + self, mock_search_servers_all_pages, mock_get_server ): """Test that a qualified name with no exact match returns None.""" - mock_search_servers.return_value = [ + mock_search_servers_all_pages.return_value = [ {"name": "com.supabase/mcp"}, ] @@ -538,6 +633,7 @@ def test_env_var_override_marks_custom(self): c = SimpleRegistryClient() self.assertEqual(c.registry_url, "https://internal.example.com") self.assertTrue(c._is_custom_url) + self.assertEqual(c.registry_source, "env") def test_env_var_invalid_rejected(self): os.environ["MCP_REGISTRY_URL"] = "not-a-url" @@ -545,28 +641,37 @@ def test_env_var_invalid_rejected(self): SimpleRegistryClient() self.assertIn("MCP_REGISTRY_URL", str(cm.exception)) - def test_userinfo_stripped_from_registry_url(self): - """SimpleRegistryClient must strip user:pass@ from the stored URL. - - Regression trap for the credential-leak path: if userinfo survives - into ``self.registry_url``, ``ServerNotFoundError`` interpolates it - into terminal output and CI logs. - """ - c = SimpleRegistryClient("https://token:x-oauth@registry.corp.example.com/") - parsed = urlparse(c.registry_url) - self.assertEqual(parsed.scheme, "https") - self.assertEqual(parsed.hostname, "registry.corp.example.com") - self.assertIsNone(parsed.username) - self.assertIsNone(parsed.password) - self.assertEqual(c.registry_url, "https://registry.corp.example.com") - - def test_userinfo_stripped_preserves_explicit_port(self): - c = SimpleRegistryClient("https://user:pass@registry.corp.example.com:8443/") - parsed = urlparse(c.registry_url) - self.assertEqual(parsed.hostname, "registry.corp.example.com") - self.assertEqual(parsed.port, 8443) - self.assertIsNone(parsed.username) - self.assertIsNone(parsed.password) + def test_explicit_userinfo_is_rejected_without_leaking(self): + secret = "explicit-userinfo-secret" + with self.assertRaises(ValueError) as raised: + SimpleRegistryClient(f"https://user:{secret}@registry.corp.example.com/") + self.assertNotIn(secret, str(raised.exception)) + + def test_environment_userinfo_is_rejected_without_leaking(self): + secret = "environment-userinfo-secret" + with ( + mock.patch.dict( + os.environ, + {"MCP_REGISTRY_URL": f"https://user:{secret}@registry.corp.example.com:8443/"}, + clear=False, + ), + self.assertRaises(ValueError) as raised, + ): + SimpleRegistryClient() + self.assertNotIn(secret, str(raised.exception)) + + def test_false_like_http_opt_in_values_are_rejected(self): + for value in ("0", "false", "no"): + with ( + self.subTest(value=value), + mock.patch.dict( + os.environ, + {"MCP_REGISTRY_ALLOW_HTTP": value}, + clear=False, + ), + self.assertRaises(ValueError), + ): + SimpleRegistryClient("http://mcp.example.com") class TestNormalizeV01Package(unittest.TestCase): diff --git a/tests/unit/test_registry_operations_phase3.py b/tests/unit/test_registry_operations_phase3.py index 8fd038ab1f..348eb744fe 100644 --- a/tests/unit/test_registry_operations_phase3.py +++ b/tests/unit/test_registry_operations_phase3.py @@ -110,6 +110,20 @@ def test_server_info_no_id_marks_as_needing_install(self) -> None: result = ops.check_servers_needing_installation(["copilot"], ["server-x"]) assert "server-x" in result + def test_prevalidated_server_cache_avoids_registry_lookup(self) -> None: + ops = _make_ops() + ops._get_installed_server_ids = MagicMock(return_value=set()) + ops.registry_client.find_server_by_reference = MagicMock() + + result = ops.check_servers_needing_installation( + ["copilot"], + ["server-a"], + server_info_cache={"server-a": {"id": "uuid-a"}}, + ) + + assert result == ["server-a"] + ops.registry_client.find_server_by_reference.assert_not_called() + # --------------------------------------------------------------------------- # MCPServerOperations._get_installed_server_ids @@ -263,10 +277,11 @@ def test_network_error_with_custom_url_raises(self, monkeypatch: pytest.MonkeyPa monkeypatch.setenv("MCP_REGISTRY_ALLOW_HTTP", "1") ops = _make_ops("http://custom.test") ops.registry_client._is_custom_url = True + ops.registry_client.registry_source = "flag" ops.registry_client.find_server_by_reference = MagicMock( side_effect=requests.RequestException("timeout") ) - with pytest.raises(RuntimeError, match="MCP_REGISTRY_URL"): + with pytest.raises(RuntimeError, match="--registry URL"): ops.validate_servers_exist(["server-a"]) def test_empty_list_returns_empty(self) -> None: diff --git a/tests/unit/test_registry_operations_state.py b/tests/unit/test_registry_operations_state.py index 18e0292064..aff5695548 100644 --- a/tests/unit/test_registry_operations_state.py +++ b/tests/unit/test_registry_operations_state.py @@ -110,6 +110,20 @@ def test_server_info_no_id_marks_as_needing_install(self) -> None: result = ops.check_servers_needing_installation(["copilot"], ["server-x"]) assert "server-x" in result + def test_prevalidated_server_cache_avoids_registry_lookup(self) -> None: + ops = _make_ops() + ops._get_installed_server_ids = MagicMock(return_value=set()) + ops.registry_client.find_server_by_reference = MagicMock() + + result = ops.check_servers_needing_installation( + ["copilot"], + ["server-a"], + server_info_cache={"server-a": {"id": "uuid-a"}}, + ) + + assert result == ["server-a"] + ops.registry_client.find_server_by_reference.assert_not_called() + # --------------------------------------------------------------------------- # MCPServerOperations._get_installed_server_ids @@ -263,10 +277,11 @@ def test_network_error_with_custom_url_raises(self, monkeypatch: pytest.MonkeyPa monkeypatch.setenv("MCP_REGISTRY_ALLOW_HTTP", "1") ops = _make_ops("http://custom.test") ops.registry_client._is_custom_url = True + ops.registry_client.registry_source = "flag" ops.registry_client.find_server_by_reference = MagicMock( side_effect=requests.RequestException("timeout") ) - with pytest.raises(RuntimeError, match="MCP_REGISTRY_URL"): + with pytest.raises(RuntimeError, match="--registry URL"): ops.validate_servers_exist(["server-a"]) def test_empty_list_returns_empty(self) -> None: