diff --git a/.apm/instructions/tests.instructions.md b/.apm/instructions/tests.instructions.md index 8c7cd435f..1378efb63 100644 --- a/.apm/instructions/tests.instructions.md +++ b/.apm/instructions/tests.instructions.md @@ -83,8 +83,8 @@ URL string is the same code shape as a security-critical sanitizer check, and the analyzer cannot tell them apart. Treating every URL assertion uniformly through `urlparse` keeps CI green AND reinforces the security pattern that production code must follow (see -`src/apm_cli/install/mcp_registry.py::_redact_url_credentials` and -`src/apm_cli/install/mcp_registry.py::_is_local_or_metadata_host`). +`src/apm_cli/install/mcp/registry.py::_redact_url_credentials` and +`src/apm_cli/install/mcp/registry.py::_is_local_or_metadata_host`). ## Other rules diff --git a/.github/instructions/tests.instructions.md b/.github/instructions/tests.instructions.md index 8c7cd435f..1378efb63 100644 --- a/.github/instructions/tests.instructions.md +++ b/.github/instructions/tests.instructions.md @@ -83,8 +83,8 @@ URL string is the same code shape as a security-critical sanitizer check, and the analyzer cannot tell them apart. Treating every URL assertion uniformly through `urlparse` keeps CI green AND reinforces the security pattern that production code must follow (see -`src/apm_cli/install/mcp_registry.py::_redact_url_credentials` and -`src/apm_cli/install/mcp_registry.py::_is_local_or_metadata_host`). +`src/apm_cli/install/mcp/registry.py::_redact_url_credentials` and +`src/apm_cli/install/mcp/registry.py::_is_local_or_metadata_host`). ## Other rules diff --git a/CHANGELOG.md b/CHANGELOG.md index 38b11d843..200ad0a88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Marketplace `url:` sources now require `https://github.com/` or `http://github.com/` URLs; GHES, GitLab, and other non-GitHub hosts are no longer resolved. (#951) +- Grouped the seven `apm_cli/install/mcp_*.py` helper modules into an `apm_cli/install/mcp/` subpackage and stripped the redundant `mcp_` prefix from each filename, aligning with the unprefixed-noun naming convention used by the rest of `install/`. Pure refactor; behaviour and public API unchanged. (#951) - `apm marketplace init` and `apm init --marketplace` next-step hints now point at `apm pack` (was `apm marketplace build`). (#722) - **Manifest contract: invalid `target:` values now raise a parse error.** Previously, an unknown token (or a CSV string like `target: opencode,claude,copilot,agents` instead of the YAML list `target: [opencode, claude, copilot, agents]`) was silently ignored, leaving `apm install` and `apm compile` to exit 0 while deploying nothing. The shared parser used by `--target` now also validates `apm.yml`'s `target:`, so the same input resolves the same way at every entry point. **Migration:** three previously-silent inputs now fail loud -- (1) unknown tokens (`target: bogus` -> fix the typo), (2) empty values (`target: ""`, `target: []` -> remove the line if you meant auto-detect), (3) `all` mixed with other targets (`target: [all, claude]` -> use `all` alone). Omitting `target:` entirely still triggers auto-detection. (#820) - Rename `DownloadStrategyManager` to `DownloadDelegate` to better reflect Facade/Delegate pattern (#918) @@ -37,6 +39,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Fixed `AttributeError: 'str' object has no attribute 'get'` crash when running `apm install --mcp ` without `--transport`, `--url`, `--mcp-version`, `--registry`, or a post-`--` stdio command. The bare-string registry shorthand now exits 0 and persists correctly to `apm.yml`. Closes #938 (#951) - `apm install` and `apm compile` no longer exit 0 with success messages when `target:` in `apm.yml` is a CSV string -- the value now parses identically to the same input on `--target`, and zero-target resolution surfaces a warning instead of a silent no-op. (#820) - Remove redundant `seen` set from `_scan_patterns()` discovery walk (#918) - `apm pack` (marketplace producer) now respects `GITHUB_HOST` for GitHub Enterprise repos -- ref resolution, token lookup, and metadata fetch all use the configured host instead of hardcoded `github.com`. `git ls-remote` is authenticated so private repos work without separate credential setup. (#1008) diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index 2156f5ea3..032eea2a3 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -594,10 +594,10 @@ def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, lo # MCP CLI helpers (W3 --mcp flag) # --------------------------------------------------------------------------- -# F7 / F5 install-time MCP warnings live in apm_cli/install/mcp_warnings.py +# F7 / F5 install-time MCP warnings live in apm_cli/install/mcp/warnings.py # per LOC budget. Re-bind module-level names for back-compat with tests # that still patch ``apm_cli.commands.install._warn_*``. -from ..install.mcp_warnings import ( +from ..install.mcp.warnings import ( warn_ssrf_url as _warn_ssrf_url, warn_shell_metachars as _warn_shell_metachars, _SHELL_METACHAR_TOKENS, @@ -605,444 +605,33 @@ def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, lo _is_internal_or_metadata_host, ) -# --registry helpers live in apm_cli/install/mcp_registry.py per LOC budget. -from ..install.mcp_registry import ( +# --registry helpers live in apm_cli/install/mcp/registry.py per LOC budget. +from ..install.mcp.registry import ( validate_registry_url as _validate_registry_url, resolve_registry_url as _resolve_registry_url, validate_mcp_dry_run_entry as _validate_mcp_dry_run_entry, ) - -def _parse_kv_pairs(pairs, *, flag_name): - """Parse a tuple of ``KEY=VALUE`` strings into a dict. - - Empty input returns ``{}``. Raises :class:`click.UsageError` (exit - code 2) on a missing ``=`` separator or empty key. - """ - result: builtins.dict = {} - for raw in pairs or (): - if "=" not in raw: - raise click.UsageError( - f"Invalid {flag_name} '{raw}': expected KEY=VALUE" - ) - key, _, value = raw.partition("=") - if not key: - raise click.UsageError( - f"Invalid {flag_name} '{raw}': key cannot be empty" - ) - result[key] = value - return result - - -def _parse_env_pairs(pairs): - """Parse ``--env KEY=VAL`` repetitions into a dict.""" - return _parse_kv_pairs(pairs, flag_name="--env") - - -def _parse_header_pairs(pairs): - """Parse ``--header KEY=VAL`` repetitions into a dict.""" - return _parse_kv_pairs(pairs, flag_name="--header") - - -def _build_mcp_entry(name, *, transport, url, env, headers, version, command_argv, - registry_url=None): - """Pure builder. Return ``(entry, is_self_defined)``. - - Routing: - - ``command_argv`` non-empty -> stdio self-defined dict. - - ``url`` set -> remote self-defined dict (transport defaults to http). - - else -> registry shorthand (bare string when no overlays, dict when - ``version`` / ``transport`` / ``registry_url`` is set; the URL is - then persisted to the entry's ``registry:`` field for reproducible - installs). ``registry_url`` is incompatible with self-defined - entries; the CLI layer enforces that via E15. - - Round-trips through :class:`MCPDependency.from_dict` (or - :meth:`from_string`) for the validation chokepoint. Validation - failures surface as :class:`ValueError` from the model. - """ - from ..models.dependency.mcp import MCPDependency - - if command_argv: - # Self-defined stdio - argv = builtins.list(command_argv) - entry: builtins.dict = { - "name": name, - "registry": False, - "transport": "stdio", - "command": argv[0], - } - if len(argv) > 1: - entry["args"] = argv[1:] - if env: - entry["env"] = builtins.dict(env) - MCPDependency.from_dict(entry) - return entry, True - - if url: - # Self-defined remote - chosen_transport = transport or "http" - entry = { - "name": name, - "registry": False, - "transport": chosen_transport, - "url": url, - } - if headers: - entry["headers"] = builtins.dict(headers) - MCPDependency.from_dict(entry) - return entry, True - - # Registry shorthand - if version: - entry = {"name": name, "version": version} - if transport: - entry["transport"] = transport - if registry_url: - entry["registry"] = registry_url - MCPDependency.from_dict(entry) - return entry, False - - if transport: - entry = {"name": name, "transport": transport} - if registry_url: - entry["registry"] = registry_url - MCPDependency.from_dict(entry) - return entry, False - - if registry_url: - # No other overlays but a custom registry URL -- promote to dict - # form so the URL is captured in apm.yml. - entry = {"name": name, "registry": registry_url} - MCPDependency.from_dict(entry) - return entry, False - - # Bare string registry shorthand -- no overlays at all. - MCPDependency.from_string(name) - return name, False - - -def _diff_entry(old, new) -> builtins.list: - """Return a short list of ``key: old -> new`` strings for human display.""" - if isinstance(old, str) and isinstance(new, str): - if old == new: - return [] - return [f" {old} -> {new}"] - old_d = {"name": old} if isinstance(old, str) else (old or {}) - new_d = {"name": new} if isinstance(new, str) else (new or {}) - keys = builtins.list(old_d.keys()) + [k for k in new_d.keys() if k not in old_d] - diff: builtins.list = [] - for k in keys: - ov = old_d.get(k, "") - nv = new_d.get(k, "") - if ov != nv: - diff.append(f" {k}: {ov!r} -> {nv!r}") - return diff - - -def _add_mcp_to_apm_yml(name, entry, *, dev=False, force=False, project_root=None, - manifest_path=None, logger=None): - """Persist ``entry`` to ``apm.yml`` under ``dependencies.mcp`` (or - ``devDependencies.mcp`` when ``dev=True``). - - Idempotency policy (W3 R3, security F8): - - Existing entry + ``--force``: replace silently, return - ``("replaced", diff)``. - - Existing entry + interactive TTY: prompt, return - ``("replaced", diff)`` or ``("skipped", diff)``. - - Existing entry + non-TTY (CI): raise :class:`click.UsageError` so - the CLI exits with code 2. - - New entry: append, return ``("added", None)``. - """ - from ..utils.yaml_io import dump_yaml, load_yaml - - apm_yml_path = manifest_path or Path(APM_YML_FILENAME) - if not apm_yml_path.exists(): - raise click.UsageError( - f"{apm_yml_path}: no apm.yml found. Run 'apm init' first." - ) - data = load_yaml(apm_yml_path) or {} - - section_name = "devDependencies" if dev else "dependencies" - if section_name not in data or not isinstance(data[section_name], builtins.dict): - data[section_name] = {} - if "mcp" not in data[section_name] or data[section_name]["mcp"] is None: - data[section_name]["mcp"] = [] - mcp_list = data[section_name]["mcp"] - if not isinstance(mcp_list, builtins.list): - raise click.UsageError( - f"{apm_yml_path}: '{section_name}.mcp' must be a list" - ) - - existing_idx = None - existing_entry = None - for i, item in enumerate(mcp_list): - item_name = item if isinstance(item, str) else ( - item.get("name") if isinstance(item, builtins.dict) else None - ) - if item_name == name: - existing_idx = i - existing_entry = item - break - - status = "added" - diff = None - if existing_idx is not None: - diff = _diff_entry(existing_entry, entry) - if not diff: - return "skipped", [] - is_tty = sys.stdin.isatty() and sys.stdout.isatty() - if force: - mcp_list[existing_idx] = entry - status = "replaced" - elif is_tty: - if logger: - logger.warning( - f"MCP server '{name}' already exists. Replacement diff:" - ) - for line in diff: - logger.verbose_detail(line) - else: - _rich_warning( - f"MCP server '{name}' already exists. Replacement diff:" - ) - for line in diff: - _rich_echo(line, color="dim") - if not click.confirm(f"Replace MCP server '{name}'?", default=False): - return "skipped", diff - mcp_list[existing_idx] = entry - status = "replaced" - else: - raise click.UsageError( - f"MCP server '{name}' already exists in {apm_yml_path}. " - f"Use --force to replace (non-interactive)." - ) - else: - mcp_list.append(entry) - - data[section_name]["mcp"] = mcp_list - dump_yaml(data, apm_yml_path) - return status, diff - - -# Mapping for E10: which flags require --mcp. Keyed by attribute-style -# name so we can read directly from the Click handler locals. -_MCP_REQUIRED_FLAGS = ( - ("transport", "--transport"), - ("url", "--url"), - ("env", "--env"), - ("header", "--header"), - ("mcp_version", "--mcp-version"), +# MCP --mcp helpers live in apm_cli/install/mcp/*.py per LOC budget. +# Re-bind module-level ``_xxx`` names so existing test patches against +# ``apm_cli.commands.install._`` and direct imports +# (``from apm_cli.commands.install import _build_mcp_entry``) keep +# working without modification. +from ..install.mcp.args import ( + parse_env_pairs as _parse_env_pairs, + parse_header_pairs as _parse_header_pairs, + parse_kv_pairs as _parse_kv_pairs, ) - - -def _validate_mcp_conflicts( - *, - mcp_name, - packages, - pre_dash_packages, - transport, - url, - env, - headers, - mcp_version, - command_argv, - global_, - only, - update, - use_ssh, - use_https, - allow_protocol_fallback, - registry_url=None, -): - """Apply conflict matrix E1-E15. Raises ``click.UsageError`` on hit.""" - # E10: flags require --mcp -- run first so users get the right hint. - if mcp_name is None: - flag_values = { - "transport": transport, - "url": url, - "env": env, - "header": headers, - "mcp_version": mcp_version, - "registry": registry_url, - } - for attr, label in (*_MCP_REQUIRED_FLAGS, ("registry", "--registry")): - if flag_values.get(attr): - raise click.UsageError(f"{label} requires --mcp") - if command_argv: - # post-`--` stdio command without --mcp: silently allowed today - # (legacy install behaviour). Do not error. - pass - return - - # E7/E8: NAME shape. - if mcp_name == "": - raise click.UsageError("MCP name cannot be empty") - if mcp_name.startswith("-"): - raise click.UsageError( - f"MCP name cannot start with '-'; did you forget a value for --mcp?" - ) - - # E1: positional packages mixed with --mcp. - 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") - - # E4: transport selection flags do not apply. - if use_ssh or use_https or allow_protocol_fallback: - raise click.UsageError( - "transport selection flags (--ssh/--https/--allow-protocol-fallback) " - "don't apply to MCP entries" - ) - - # E5: --update is for refreshing, not adding. - if update: - raise click.UsageError("use 'apm update' instead to update MCP entries") - - # E9: --header without --url. - if headers and not url: - raise click.UsageError("--header requires --url") - - # E11: --url with stdio command. - if url and command_argv: - raise click.UsageError("cannot specify both --url and a stdio command") - - # E12: --transport stdio with --url. - if transport == "stdio" and url: - raise click.UsageError("stdio transport doesn't accept --url") - - # E13: remote transports with stdio command. - if transport in ("http", "sse", "streamable-http") and command_argv: - raise click.UsageError("remote transports don't accept stdio command") - - # E14: --env with --url and no command. - if env and url and not command_argv: - raise click.UsageError( - "--env applies to stdio MCPs; use --header for remote" - ) - - # E15: --registry only applies to registry-resolved entries. - if registry_url and (url or command_argv): - raise click.UsageError( - "--registry only applies to registry-resolved MCP servers; " - "remove --url or the post-`--` stdio command, or drop --registry" - ) - - -def _run_mcp_install( - *, - mcp_name, - transport, - url, - env_pairs, - header_pairs, - mcp_version, - command_argv, - dev, - force, - runtime, - exclude, - verbose, - logger, - manifest_path, - apm_dir, - scope, - registry_url=None, -): - """Execute the --mcp install path. ``registry_url`` is the validated - --registry value; the caller resolved precedence vs MCP_REGISTRY_URL.""" - from ..models.dependency.mcp import MCPDependency - - env = _parse_env_pairs(env_pairs) - headers = _parse_header_pairs(header_pairs) - - # 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( - mcp_name, - transport=transport, - url=url, - env=env, - headers=headers, - version=mcp_version, - command_argv=command_argv, - registry_url=registry_url, - ) - except ValueError as exc: - raise click.UsageError(str(exc)) - - # F5 + F7 warnings -- do not block. - _warn_ssrf_url(url, logger) - _warn_shell_metachars(env, logger, command=entry.get("command")) - - # Write to apm.yml. - status, _diff = _add_mcp_to_apm_yml( - mcp_name, - entry, - dev=dev, - force=force, - manifest_path=manifest_path, - logger=logger, - ) - - if status == "skipped": - logger.progress(f"MCP server '{mcp_name}' unchanged") - return - - # 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 - # MCPServerOperations() (constructed deep inside MCPIntegrator.install) - # picks up the override; prior env restored on exit. - if APM_DEPS_AVAILABLE: - from ..install.mcp_registry import registry_env_override - - if registry_url and logger and verbose: - logger.verbose_detail(f"Registry: {registry_url}") - with registry_env_override(registry_url): - try: - _existing_lock = LockFile.read(get_lockfile_path(apm_dir)) - old_servers = builtins.set(_existing_lock.mcp_servers) if _existing_lock else builtins.set() - old_configs = builtins.dict(_existing_lock.mcp_configs) if _existing_lock else {} - MCPIntegrator.install( - [dep], runtime, exclude, verbose, - stored_mcp_configs=old_configs, - scope=scope, - ) - new_names = MCPIntegrator.get_server_names([dep]) - new_configs = MCPIntegrator.get_server_configs([dep]) - merged_names = old_servers | new_names - merged_configs = builtins.dict(old_configs) - merged_configs.update(new_configs) - MCPIntegrator.update_lockfile(merged_names, mcp_configs=merged_configs) - except Exception as exc: # pragma: no cover -- defensive - logger.warning(f"MCP server written to apm.yml but integration failed: {exc}") - - verb = "Replaced" if status == "replaced" else "Added" - logger.success(f"{verb} MCP server '{mcp_name}'", symbol="check") - if isinstance(entry, builtins.dict): - chosen_transport = entry.get("transport") or "registry" - else: - chosen_transport = "registry" - logger.tree_item(f" transport: {chosen_transport}") - logger.tree_item(f" apm.yml: {manifest_path}") +from ..install.mcp.entry import build_mcp_entry as _build_mcp_entry +from ..install.mcp.writer import ( + _diff_entry, + add_mcp_to_apm_yml as _add_mcp_to_apm_yml, +) +from ..install.mcp.conflicts import ( + MCP_REQUIRED_FLAGS as _MCP_REQUIRED_FLAGS, + validate_mcp_conflicts as _validate_mcp_conflicts, +) +from ..install.mcp.command import run_mcp_install as _run_mcp_install # --------------------------------------------------------------------------- diff --git a/src/apm_cli/install/__init__.py b/src/apm_cli/install/__init__.py index 351c4c711..f18d8fbf5 100644 --- a/src/apm_cli/install/__init__.py +++ b/src/apm_cli/install/__init__.py @@ -17,6 +17,7 @@ phases/ one module per pipeline phase helpers/ cross-cutting helpers (security scan, gitignore) presentation/ dry-run preview + final result rendering + mcp/ ``apm install --mcp`` flow (parse / build / warn / write) The engine is import-safe (no Click decorators at top level) so phase modules can be unit-tested directly without invoking the CLI. diff --git a/src/apm_cli/install/mcp/__init__.py b/src/apm_cli/install/mcp/__init__.py new file mode 100644 index 000000000..54dc208ce --- /dev/null +++ b/src/apm_cli/install/mcp/__init__.py @@ -0,0 +1,18 @@ +"""MCP-specific helpers for the ``apm install --mcp`` code path. + +Modules in this subpackage compose the user-visible MCP install flow +(parse args -> build entry -> warn -> write apm.yml -> integrate). They +sit alongside the main install pipeline (``install/pipeline.py`` and +``install/phases/``) but form an independent flow that the +``commands/install.py`` Click handler delegates to when ``--mcp`` is set. + +Layout: + + args.py CLI ``--env`` / ``--header`` KEY=VAL parsers + command.py ``run_mcp_install`` orchestrator + conflicts.py ``--mcp`` flag conflict matrix (E1-E15) + entry.py Pure ``apm.yml`` MCP entry builder (str | dict union) + registry.py ``--registry`` validation, precedence, env override + warnings.py F5 SSRF + F7 shell-metachar install-time warnings + writer.py Idempotent ``apm.yml`` MCP entry persistence +""" diff --git a/src/apm_cli/install/mcp/args.py b/src/apm_cli/install/mcp/args.py new file mode 100644 index 000000000..789446bb9 --- /dev/null +++ b/src/apm_cli/install/mcp/args.py @@ -0,0 +1,46 @@ +"""MCP CLI argument parsing for ``--env`` and ``--header`` repetitions. + +Extracted from ``commands/install.py`` per the architecture-invariants +LOC budget (sibling to ``warnings.py`` / ``registry.py``). +""" + +from __future__ import annotations + +from typing import Dict, Iterable, Optional + +import click + + +def parse_kv_pairs( + pairs: Optional[Iterable[str]], + *, + flag_name: str, +) -> Dict[str, str]: + """Parse a tuple of ``KEY=VALUE`` strings into a dict. + + Empty input returns ``{}``. Raises :class:`click.UsageError` (exit + code 2) on a missing ``=`` separator or empty key. + """ + result: Dict[str, str] = {} + for raw in pairs or (): + if "=" not in raw: + raise click.UsageError( + f"Invalid {flag_name} '{raw}': expected KEY=VALUE" + ) + key, _, value = raw.partition("=") + if not key: + raise click.UsageError( + f"Invalid {flag_name} '{raw}': key cannot be empty" + ) + result[key] = value + return result + + +def parse_env_pairs(pairs: Optional[Iterable[str]]) -> Dict[str, str]: + """Parse ``--env KEY=VAL`` repetitions into a dict.""" + return parse_kv_pairs(pairs, flag_name="--env") + + +def parse_header_pairs(pairs: Optional[Iterable[str]]) -> Dict[str, str]: + """Parse ``--header KEY=VAL`` repetitions into a dict.""" + return parse_kv_pairs(pairs, flag_name="--header") diff --git a/src/apm_cli/install/mcp/command.py b/src/apm_cli/install/mcp/command.py new file mode 100644 index 000000000..f29927c24 --- /dev/null +++ b/src/apm_cli/install/mcp/command.py @@ -0,0 +1,143 @@ +"""Orchestrator for the ``apm install --mcp`` code path. + +Extracted from ``commands/install.py`` per the architecture-invariants +LOC budget. ``run_mcp_install`` composes the sibling MCP modules +(``args``, ``entry``, ``writer``, ``warnings``, ``registry``) into the +user-visible install flow: + + parse args -> build entry -> warn -> write apm.yml -> integrate +""" + +from __future__ import annotations + +from pathlib import Path +from typing import Optional, Sequence + +import click + +from .args import parse_env_pairs, parse_header_pairs +from .entry import build_mcp_entry +from .registry import registry_env_override +from .warnings import warn_shell_metachars, warn_ssrf_url +from .writer import add_mcp_to_apm_yml + + +# APM Dependencies (conditional import for graceful degradation). +# Mirrors the pattern in ``commands/install.py`` so the success/log +# behaviour around a missing optional dep is symmetric across the two +# code paths (package install vs. MCP install). +APM_DEPS_AVAILABLE = False +try: + from ...deps.lockfile import LockFile, get_lockfile_path + from ...integration.mcp_integrator import MCPIntegrator + + APM_DEPS_AVAILABLE = True +except ImportError: + pass + + +def run_mcp_install( + *, + mcp_name: str, + transport: Optional[str], + url: Optional[str], + env_pairs: Optional[Sequence[str]], + header_pairs: Optional[Sequence[str]], + mcp_version: Optional[str], + command_argv: Optional[Sequence[str]], + dev: bool, + force: bool, + runtime: Optional[str], + exclude: Optional[str], + verbose: bool, + logger, + manifest_path: Path, + apm_dir: Path, + scope: Optional[str], + registry_url: Optional[str] = None, +) -> None: + """Execute the --mcp install path. ``registry_url`` is the validated + --registry value; the caller resolved precedence vs MCP_REGISTRY_URL.""" + from ...models.dependency.mcp import MCPDependency + + env = parse_env_pairs(env_pairs) + headers = parse_header_pairs(header_pairs) + + # 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( + mcp_name, + transport=transport, + url=url, + env=env, + headers=headers, + version=mcp_version, + command_argv=command_argv, + registry_url=registry_url, + ) + except ValueError as exc: + raise click.UsageError(str(exc)) + + # F5 + F7 warnings -- do not block. Source the stdio command from the + # CLI input rather than the built ``entry``: ``entry`` is ``str`` for + # bare-string registry shorthand and ``dict`` otherwise, so ``entry.get`` + # is unsafe. + warn_ssrf_url(url, logger) + stdio_command = command_argv[0] if command_argv else None + warn_shell_metachars(env, logger, command=stdio_command) + + # Write to apm.yml. + status, _diff = add_mcp_to_apm_yml( + mcp_name, + entry, + dev=dev, + force=force, + manifest_path=manifest_path, + logger=logger, + ) + + if status == "skipped": + logger.progress(f"MCP server '{mcp_name}' unchanged") + return + + # 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 + # MCPServerOperations() (constructed deep inside MCPIntegrator.install) + # picks up the override; prior env restored on exit. + if APM_DEPS_AVAILABLE: + if registry_url and logger and verbose: + logger.verbose_detail(f"Registry: {registry_url}") + with registry_env_override(registry_url): + try: + _existing_lock = LockFile.read(get_lockfile_path(apm_dir)) + old_servers = set(_existing_lock.mcp_servers) if _existing_lock else set() + old_configs = dict(_existing_lock.mcp_configs) if _existing_lock else {} + MCPIntegrator.install( + [dep], runtime, exclude, verbose, + stored_mcp_configs=old_configs, + scope=scope, + ) + new_names = MCPIntegrator.get_server_names([dep]) + new_configs = MCPIntegrator.get_server_configs([dep]) + merged_names = old_servers | new_names + merged_configs = dict(old_configs) + merged_configs.update(new_configs) + MCPIntegrator.update_lockfile(merged_names, mcp_configs=merged_configs) + except Exception as exc: # pragma: no cover -- defensive + logger.warning(f"MCP server written to apm.yml but integration failed: {exc}") + + verb = "Replaced" if status == "replaced" else "Added" + logger.success(f"{verb} MCP server '{mcp_name}'", symbol="check") + if isinstance(entry, dict): + chosen_transport = entry.get("transport") or "registry" + else: + chosen_transport = "registry" + logger.tree_item(f" transport: {chosen_transport}") + logger.tree_item(f" apm.yml: {manifest_path}") diff --git a/src/apm_cli/install/mcp/conflicts.py b/src/apm_cli/install/mcp/conflicts.py new file mode 100644 index 000000000..0df07904e --- /dev/null +++ b/src/apm_cli/install/mcp/conflicts.py @@ -0,0 +1,128 @@ +"""MCP CLI flag-conflict matrix (E1-E15). + +Extracted from ``commands/install.py`` per the architecture-invariants +LOC budget. ``validate_mcp_conflicts`` is the single chokepoint that +turns invalid ``apm install --mcp`` flag combinations into +``click.UsageError`` (exit 2) before any side-effects fire. +""" + +from __future__ import annotations + +from typing import Mapping, Optional, Sequence, Tuple + +import click + + +# Mapping for E10: which flags require --mcp. Keyed by attribute-style +# name so we can read directly from the Click handler locals. +MCP_REQUIRED_FLAGS: Tuple[Tuple[str, str], ...] = ( + ("transport", "--transport"), + ("url", "--url"), + ("env", "--env"), + ("header", "--header"), + ("mcp_version", "--mcp-version"), +) + + +def validate_mcp_conflicts( + *, + mcp_name: Optional[str], + packages: Sequence[str], + pre_dash_packages: Sequence[str], + transport: Optional[str], + url: Optional[str], + env: Mapping[str, str], + headers: Mapping[str, str], + mcp_version: Optional[str], + command_argv: Optional[Sequence[str]], + global_: bool, + only: Optional[str], + update: bool, + use_ssh: bool, + use_https: bool, + allow_protocol_fallback: bool, + registry_url: Optional[str] = None, +) -> None: + """Apply conflict matrix E1-E15. Raises ``click.UsageError`` on hit.""" + # E10: flags require --mcp -- run first so users get the right hint. + if mcp_name is None: + flag_values = { + "transport": transport, + "url": url, + "env": env, + "header": headers, + "mcp_version": mcp_version, + "registry": registry_url, + } + for attr, label in (*MCP_REQUIRED_FLAGS, ("registry", "--registry")): + if flag_values.get(attr): + raise click.UsageError(f"{label} requires --mcp") + if command_argv: + # post-`--` stdio command without --mcp: silently allowed today + # (legacy install behaviour). Do not error. + pass + return + + # E7/E8: NAME shape. + if mcp_name == "": + raise click.UsageError("MCP name cannot be empty") + if mcp_name.startswith("-"): + raise click.UsageError( + f"MCP name cannot start with '-'; did you forget a value for --mcp?" + ) + + # E1: positional packages mixed with --mcp. + 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") + + # E4: transport selection flags do not apply. + if use_ssh or use_https or allow_protocol_fallback: + raise click.UsageError( + "transport selection flags (--ssh/--https/--allow-protocol-fallback) " + "don't apply to MCP entries" + ) + + # E5: --update is for refreshing, not adding. + if update: + raise click.UsageError("use 'apm update' instead to update MCP entries") + + # E9: --header without --url. + if headers and not url: + raise click.UsageError("--header requires --url") + + # E11: --url with stdio command. + if url and command_argv: + raise click.UsageError("cannot specify both --url and a stdio command") + + # E12: --transport stdio with --url. + if transport == "stdio" and url: + raise click.UsageError("stdio transport doesn't accept --url") + + # E13: remote transports with stdio command. + if transport in ("http", "sse", "streamable-http") and command_argv: + raise click.UsageError("remote transports don't accept stdio command") + + # E14: --env with --url and no command. + if env and url and not command_argv: + raise click.UsageError( + "--env applies to stdio MCPs; use --header for remote" + ) + + # E15: --registry only applies to registry-resolved entries. + if registry_url and (url or command_argv): + raise click.UsageError( + "--registry only applies to registry-resolved MCP servers; " + "remove --url or the post-`--` stdio command, or drop --registry" + ) diff --git a/src/apm_cli/install/mcp/entry.py b/src/apm_cli/install/mcp/entry.py new file mode 100644 index 000000000..beec85d49 --- /dev/null +++ b/src/apm_cli/install/mcp/entry.py @@ -0,0 +1,100 @@ +"""Pure builder for MCP ``apm.yml`` entries. + +Extracted from ``commands/install.py`` per the architecture-invariants +LOC budget. ``build_mcp_entry`` returns a tagged-union value -- a bare +string for the registry-shorthand-with-no-overlays path (preserving the +``mcp: [foo]`` ``apm.yml`` UX contract) and a dict otherwise. Callers +must dispatch with ``isinstance(entry, dict)`` or treat the result as +opaque; see #938 for the regression that motivates this rule. +""" + +from __future__ import annotations + +from typing import Any, Dict, Mapping, Optional, Sequence, Tuple, Union + + +def build_mcp_entry( + name: str, + *, + transport: Optional[str], + url: Optional[str], + env: Optional[Mapping[str, str]], + headers: Optional[Mapping[str, str]], + version: Optional[str], + command_argv: Optional[Sequence[str]], + registry_url: Optional[str] = None, +) -> Tuple[Union[str, Dict[str, Any]], bool]: + """Pure builder. Return ``(entry, is_self_defined)``. + + Routing: + - ``command_argv`` non-empty -> stdio self-defined dict. + - ``url`` set -> remote self-defined dict (transport defaults to http). + - else -> registry shorthand (bare string when no overlays, dict when + ``version`` / ``transport`` / ``registry_url`` is set; the URL is + then persisted to the entry's ``registry:`` field for reproducible + installs). ``registry_url`` is incompatible with self-defined + entries; the CLI layer enforces that via E15. + + Round-trips through :class:`MCPDependency.from_dict` (or + :meth:`from_string`) for the validation chokepoint. Validation + failures surface as :class:`ValueError` from the model. + """ + from ...models.dependency.mcp import MCPDependency + + if command_argv: + # Self-defined stdio + argv = list(command_argv) + entry: Dict[str, Any] = { + "name": name, + "registry": False, + "transport": "stdio", + "command": argv[0], + } + if len(argv) > 1: + entry["args"] = argv[1:] + if env: + entry["env"] = dict(env) + MCPDependency.from_dict(entry) + return entry, True + + if url: + # Self-defined remote + chosen_transport = transport or "http" + entry = { + "name": name, + "registry": False, + "transport": chosen_transport, + "url": url, + } + if headers: + entry["headers"] = dict(headers) + MCPDependency.from_dict(entry) + return entry, True + + # Registry shorthand + if version: + entry = {"name": name, "version": version} + if transport: + entry["transport"] = transport + if registry_url: + entry["registry"] = registry_url + MCPDependency.from_dict(entry) + return entry, False + + if transport: + entry = {"name": name, "transport": transport} + if registry_url: + entry["registry"] = registry_url + MCPDependency.from_dict(entry) + return entry, False + + if registry_url: + # No other overlays but a custom registry URL -- promote to dict + # form so the URL is captured in apm.yml. + entry = {"name": name, "registry": registry_url} + MCPDependency.from_dict(entry) + return entry, False + + # Bare string registry shorthand -- no overlays at all. + MCPDependency.from_string(name) + return name, False diff --git a/src/apm_cli/install/mcp_registry.py b/src/apm_cli/install/mcp/registry.py similarity index 95% rename from src/apm_cli/install/mcp_registry.py rename to src/apm_cli/install/mcp/registry.py index fb8730099..bee9594b8 100644 --- a/src/apm_cli/install/mcp_registry.py +++ b/src/apm_cli/install/mcp/registry.py @@ -27,7 +27,7 @@ import click -from ..models.dependency.mcp import _ALLOWED_URL_SCHEMES +from ...models.dependency.mcp import _ALLOWED_URL_SCHEMES # Defensive cap on registry URL length to keep apm.yml diffs reviewable @@ -241,16 +241,13 @@ def validate_mcp_dry_run_entry(name, **kwargs) -> None: """C1: validate the MCP entry that ``apm install --mcp ... --dry-run`` would persist, raising :class:`click.UsageError` on rejection. - Mirrors the validation that real install runs via ``_build_mcp_entry``, + Mirrors the validation that real install runs via ``build_mcp_entry``, so dry-run never previews "success" for an entry the real install would reject. Lives here (not in commands/install.py) per the LOC-budget invariant on that module. """ - # Local import: ``_build_mcp_entry`` lives in commands/install.py and - # imports from this module, so the import must be deferred to call time - # to avoid a circular import. - from ..commands.install import _build_mcp_entry + from .entry import build_mcp_entry try: - _build_mcp_entry(name, **kwargs) + build_mcp_entry(name, **kwargs) except ValueError as exc: raise click.UsageError(str(exc)) diff --git a/src/apm_cli/install/mcp_warnings.py b/src/apm_cli/install/mcp/warnings.py similarity index 100% rename from src/apm_cli/install/mcp_warnings.py rename to src/apm_cli/install/mcp/warnings.py diff --git a/src/apm_cli/install/mcp/writer.py b/src/apm_cli/install/mcp/writer.py new file mode 100644 index 000000000..f18c7ed4c --- /dev/null +++ b/src/apm_cli/install/mcp/writer.py @@ -0,0 +1,129 @@ +"""Persist MCP entries into ``apm.yml`` (idempotent W3 R3 / F8 contract). + +Extracted from ``commands/install.py`` per the architecture-invariants +LOC budget. ``add_mcp_to_apm_yml`` is the single chokepoint that mutates +``apm.yml`` for ``apm install --mcp``; the diff helper used to render +replacement previews is colocated as a private module-level helper. +""" + +from __future__ import annotations + +import sys +from pathlib import Path +from typing import Any, Dict, List, Optional, Tuple, Union + +import click + +from ...constants import APM_YML_FILENAME +from ...core.null_logger import NullCommandLogger + + +MCPEntry = Union[str, Dict[str, Any]] + + +def _diff_entry( + old: Optional[MCPEntry], + new: Optional[MCPEntry], +) -> List[str]: + """Return a short list of ``key: old -> new`` strings for human display.""" + if isinstance(old, str) and isinstance(new, str): + if old == new: + return [] + return [f" {old} -> {new}"] + old_d = {"name": old} if isinstance(old, str) else (old or {}) + new_d = {"name": new} if isinstance(new, str) else (new or {}) + keys = list(old_d.keys()) + [k for k in new_d.keys() if k not in old_d] + diff: List[str] = [] + for k in keys: + ov = old_d.get(k, "") + nv = new_d.get(k, "") + if ov != nv: + diff.append(f" {k}: {ov!r} -> {nv!r}") + return diff + + +def add_mcp_to_apm_yml( + name: str, + entry: MCPEntry, + *, + dev: bool = False, + force: bool = False, + project_root: Optional[Path] = None, + manifest_path: Optional[Path] = None, + logger=None, +) -> Tuple[str, Optional[List[str]]]: + """Persist ``entry`` to ``apm.yml`` under ``dependencies.mcp`` (or + ``devDependencies.mcp`` when ``dev=True``). + + Idempotency policy (W3 R3, security F8): + - Existing entry + ``--force``: replace silently, return + ``("replaced", diff)``. + - Existing entry + interactive TTY: prompt, return + ``("replaced", diff)`` or ``("skipped", diff)``. + - Existing entry + non-TTY (CI): raise :class:`click.UsageError` so + the CLI exits with code 2. + - New entry: append, return ``("added", None)``. + """ + from ...utils.yaml_io import dump_yaml, load_yaml + + log = logger if logger is not None else NullCommandLogger() + apm_yml_path = manifest_path or Path(APM_YML_FILENAME) + if not apm_yml_path.exists(): + raise click.UsageError( + f"{apm_yml_path}: no apm.yml found. Run 'apm init' first." + ) + data = load_yaml(apm_yml_path) or {} + + section_name = "devDependencies" if dev else "dependencies" + if section_name not in data or not isinstance(data[section_name], dict): + data[section_name] = {} + if "mcp" not in data[section_name] or data[section_name]["mcp"] is None: + data[section_name]["mcp"] = [] + mcp_list = data[section_name]["mcp"] + if not isinstance(mcp_list, list): + raise click.UsageError( + f"{apm_yml_path}: '{section_name}.mcp' must be a list" + ) + + existing_idx = None + existing_entry = None + for i, item in enumerate(mcp_list): + item_name = item if isinstance(item, str) else ( + item.get("name") if isinstance(item, dict) else None + ) + if item_name == name: + existing_idx = i + existing_entry = item + break + + status = "added" + diff = None + if existing_idx is not None: + diff = _diff_entry(existing_entry, entry) + if not diff: + return "skipped", [] + is_tty = sys.stdin.isatty() and sys.stdout.isatty() + if force: + mcp_list[existing_idx] = entry + status = "replaced" + elif is_tty: + log.warning( + f"MCP server '{name}' already exists. Replacement diff:" + ) + for line in diff: + log.verbose_detail(line) + if not click.confirm(f"Replace MCP server '{name}'?", default=False): + return "skipped", diff + mcp_list[existing_idx] = entry + status = "replaced" + else: + raise click.UsageError( + f"MCP server '{name}' already exists in {apm_yml_path}. " + f"Use --force to replace (non-interactive)." + ) + else: + mcp_list.append(entry) + + data[section_name]["mcp"] = mcp_list + dump_yaml(data, apm_yml_path) + return status, diff diff --git a/tests/unit/install/test_mcp_registry_module.py b/tests/unit/install/test_mcp_registry_module.py index d594aeddd..cd89c9ecb 100644 --- a/tests/unit/install/test_mcp_registry_module.py +++ b/tests/unit/install/test_mcp_registry_module.py @@ -1,4 +1,4 @@ -"""Unit tests for ``apm_cli.install.mcp_registry``. +"""Unit tests for ``apm_cli.install.mcp.registry``. Covers: - ``resolve_registry_url`` precedence chain and visibility of overrides. @@ -12,7 +12,7 @@ import pytest -from apm_cli.install.mcp_registry import ( +from apm_cli.install.mcp.registry import ( registry_env_override, resolve_registry_url, validate_registry_url, @@ -183,7 +183,7 @@ class TestRedactUrlCredentials: """U3 regression: never echo URL credentials in logger output.""" def test_strips_user_password(self): - from apm_cli.install.mcp_registry import _redact_url_credentials + from apm_cli.install.mcp.registry import _redact_url_credentials from urllib.parse import urlparse out = _redact_url_credentials("https://user:secret@registry.example.com/v0") assert "secret" not in out @@ -192,7 +192,7 @@ def test_strips_user_password(self): assert parsed.hostname == "registry.example.com" def test_keeps_port(self): - from apm_cli.install.mcp_registry import _redact_url_credentials + from apm_cli.install.mcp.registry import _redact_url_credentials from urllib.parse import urlparse out = _redact_url_credentials("https://u:p@registry.example.com:8443/x") parsed = urlparse(out) @@ -201,7 +201,7 @@ def test_keeps_port(self): assert "p" not in (parsed.password or "") def test_no_creds_passthrough(self): - from apm_cli.install.mcp_registry import _redact_url_credentials + from apm_cli.install.mcp.registry import _redact_url_credentials url = "https://registry.example.com/v0" assert _redact_url_credentials(url) == url diff --git a/tests/unit/test_install_command.py b/tests/unit/test_install_command.py index d104fa828..b8fa7b66a 100644 --- a/tests/unit/test_install_command.py +++ b/tests/unit/test_install_command.py @@ -1673,6 +1673,24 @@ def test_mcp_header_repeats_collect_into_dict(self): data = yaml.safe_load((tmp / "apm.yml").read_text()) assert data["dependencies"]["mcp"][0]["headers"] == {"X-A": "1", "X-B": "2"} + def test_mcp_registry_shorthand_no_overlays_persists_bare_string(self): + # Bare registry shorthand (no --transport, --url, --mcp-version, + # --registry, post-`--` argv) is a documented happy path; the + # builder returns ``str``, and the install path must not introspect + # the entry as a dict. + ref = "io.github.github/github-mcp-server" + with self._chdir_with_apm_yml() as tmp, \ + patch("apm_cli.commands.install._get_invocation_argv", + return_value=["apm", "install", "--mcp", ref]), \ + patch("apm_cli.install.mcp.command.MCPIntegrator"): + result = self.runner.invoke(cli, ["install", "--mcp", ref]) + assert result.exit_code == 0, result.output + assert "'str' object has no attribute" not in result.output + data = yaml.safe_load((tmp / "apm.yml").read_text()) + # Bare-string serialization is the apm.yml UX contract for + # shorthand-with-no-overlays; do not silently promote to a dict. + assert data["dependencies"]["mcp"] == [ref] + # --- Conflict matrix E1-E14 --- def test_e1_mcp_with_positional_packages(self):