From aaf41c980f94857154419cfe1aae42102f7eca1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=A8=B1=E5=85=83=E8=B1=AA?= <146086744+edenfunf@users.noreply.github.com> Date: Sun, 26 Apr 2026 20:14:21 +0800 Subject: [PATCH 1/7] fix(install): handle bare-string MCP entry in shell-metachar warning `apm install --mcp ` without --transport, --url, --mcp-version, --registry, or post-`--` argv returns a bare string from the entry builder per the apm.yml shorthand contract. The F7 shell-metachar warning callsite then read `entry.get("command")` unguarded, raising `'str' object has no attribute 'get'` and aborting the install before apm.yml could be written. Source the stdio command from `command_argv` (the CLI input that would have populated `entry["command"]` in the dict-entry path) rather than introspecting the post-build entry. The two values are identical in the stdio path and both yield `None` elsewhere, so warning behaviour is preserved bit-for-bit while the str-vs-dict dispatch disappears from this callsite. Other already-guarded `isinstance(entry, dict)` callsites in `_run_mcp_install` are left as-is to keep the diff minimal. Closes #938 --- src/apm_cli/commands/install.py | 8 ++++++-- tests/unit/test_install_command.py | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index 61de53794..b4d67165c 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -849,9 +849,13 @@ def _run_mcp_install( except ValueError as exc: raise click.UsageError(str(exc)) - # F5 + F7 warnings -- do not block. + # 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) - _warn_shell_metachars(env, logger, command=entry.get("command")) + 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( diff --git a/tests/unit/test_install_command.py b/tests/unit/test_install_command.py index 728d723a4..511104732 100644 --- a/tests/unit/test_install_command.py +++ b/tests/unit/test_install_command.py @@ -1653,6 +1653,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.commands.install.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): From da306553cf939f4f08b18948c7288708846d90c9 Mon Sep 17 00:00:00 2001 From: xuyuanhao Date: Mon, 27 Apr 2026 14:42:44 +0800 Subject: [PATCH 2/7] refactor(install): extract --mcp install path into apm_cli/install/ commands/install.py grew to 1704 LOC, 4 over the 1700 budget enforced by tests/unit/install/test_architecture_invariants.py. Per the python-architecture skill, cosmetic trimming is the explicit anti-pattern; engage modular extraction instead. The --mcp install code path is the most cohesive contiguous block in install.py: parse args -> build entry -> validate conflicts -> warn -> write apm.yml -> integrate. Five sibling modules under install/, next to the existing mcp_warnings.py / mcp_registry.py, host the extraction: - install/mcp_args.py: parse_kv_pairs / parse_env_pairs / parse_header_pairs - install/mcp_entry.py: build_mcp_entry (str|dict tagged-union builder) - install/mcp_writer.py: add_mcp_to_apm_yml + _diff_entry - install/mcp_conflicts.py: validate_mcp_conflicts + MCP_REQUIRED_FLAGS - install/mcp_command.py: run_mcp_install orchestrator commands/install.py drops from 1704 to 1289 LOC (411 lines headroom). Back-compat preserved bit-for-bit: - All five helpers are re-exported from commands/install.py with the leading-underscore aliases the existing tests rely on, so direct imports (from apm_cli.commands.install import _build_mcp_entry) and patches (@patch("apm_cli.commands.install._run_mcp_install")) keep working unchanged. - mcp_command.py mirrors the APM_DEPS_AVAILABLE try/except pattern from commands/install.py so the success-log behaviour around a missing optional dep stays symmetric across both code paths. - mcp_registry.py's prior local-import-to-avoid-cycle of _build_mcp_entry is replaced with a sibling module-top import from mcp_entry. Cycle eliminated. The PR #951 fix (command_argv[0] if command_argv else None instead of entry.get("command")) moves intact to install/mcp_command.py:84-85. 5515 unit tests pass; test_architecture_invariants (the LOC gate) green. --- src/apm_cli/commands/install.py | 453 ++------------------------- src/apm_cli/install/mcp_args.py | 40 +++ src/apm_cli/install/mcp_command.py | 140 +++++++++ src/apm_cli/install/mcp_conflicts.py | 126 ++++++++ src/apm_cli/install/mcp_entry.py | 89 ++++++ src/apm_cli/install/mcp_registry.py | 9 +- src/apm_cli/install/mcp_writer.py | 120 +++++++ 7 files changed, 537 insertions(+), 440 deletions(-) create mode 100644 src/apm_cli/install/mcp_args.py create mode 100644 src/apm_cli/install/mcp_command.py create mode 100644 src/apm_cli/install/mcp_conflicts.py create mode 100644 src/apm_cli/install/mcp_entry.py create mode 100644 src/apm_cli/install/mcp_writer.py diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index b4d67165c..2e14750a7 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -478,441 +478,26 @@ def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, lo 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. 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: - 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 @click.command( diff --git a/src/apm_cli/install/mcp_args.py b/src/apm_cli/install/mcp_args.py new file mode 100644 index 000000000..0cf8bb948 --- /dev/null +++ b/src/apm_cli/install/mcp_args.py @@ -0,0 +1,40 @@ +"""MCP CLI argument parsing for ``--env`` and ``--header`` repetitions. + +Extracted from ``commands/install.py`` per the architecture-invariants +LOC budget (sibling to ``mcp_warnings.py`` / ``mcp_registry.py``). +""" + +from __future__ import annotations + +import click + + +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: 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") diff --git a/src/apm_cli/install/mcp_command.py b/src/apm_cli/install/mcp_command.py new file mode 100644 index 000000000..bf262925c --- /dev/null +++ b/src/apm_cli/install/mcp_command.py @@ -0,0 +1,140 @@ +"""Orchestrator for the ``apm install --mcp`` code path. + +Extracted from ``commands/install.py`` per the architecture-invariants +LOC budget. ``run_mcp_install`` composes the smaller MCP modules +(``mcp_args``, ``mcp_entry``, ``mcp_writer``, ``mcp_warnings``, +``mcp_registry``) into the user-visible install flow: + + parse args -> build entry -> warn -> write apm.yml -> integrate +""" + +from __future__ import annotations + +import click + +from .mcp_args import parse_env_pairs, parse_header_pairs +from .mcp_entry import build_mcp_entry +from .mcp_registry import registry_env_override +from .mcp_warnings import warn_shell_metachars, warn_ssrf_url +from .mcp_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, + 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. 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..03e35ddf1 --- /dev/null +++ b/src/apm_cli/install/mcp_conflicts.py @@ -0,0 +1,126 @@ +"""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 + +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 = ( + ("transport", "--transport"), + ("url", "--url"), + ("env", "--env"), + ("header", "--header"), + ("mcp_version", "--mcp-version"), +) + + +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" + ) diff --git a/src/apm_cli/install/mcp_entry.py b/src/apm_cli/install/mcp_entry.py new file mode 100644 index 000000000..ce7fc9688 --- /dev/null +++ b/src/apm_cli/install/mcp_entry.py @@ -0,0 +1,89 @@ +"""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 + + +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 = list(command_argv) + entry: dict = { + "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 index fb8730099..eb1a5e6f7 100644 --- a/src/apm_cli/install/mcp_registry.py +++ b/src/apm_cli/install/mcp_registry.py @@ -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 .mcp_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_writer.py b/src/apm_cli/install/mcp_writer.py new file mode 100644 index 000000000..d94662bce --- /dev/null +++ b/src/apm_cli/install/mcp_writer.py @@ -0,0 +1,120 @@ +"""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 + +import click + +from ..constants import APM_YML_FILENAME +from ..utils.console import _rich_echo, _rich_warning + + +def _diff_entry(old, new) -> 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 = list(old_d.keys()) + [k for k in new_d.keys() if k not in old_d] + diff: 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], 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: + 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 From a763e985ace0287e9c62c5f7e406891e59c4afc4 Mon Sep 17 00:00:00 2001 From: xuyuanhao Date: Mon, 27 Apr 2026 14:43:12 +0800 Subject: [PATCH 3/7] docs(changelog): add #938 fix entry under [Unreleased] > Fixed Required by the apm-review-panel verdict on PR #951. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e7c4a41b..b06a755fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,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) - Fixed TLS validation failure behind corporate TLS-intercepting proxies and firewalls: `install/validation.py` now uses `requests` (honouring `REQUESTS_CA_BUNDLE`) instead of stdlib `urllib`, and surfaces a single CA-trust hint at default verbosity instead of a misleading auth error. (#911) ## [0.9.3] - 2026-04-26 From ed663822b93b34a9db3e3d68eed45d9aa6c7bbce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=A8=B1=E5=85=83=E8=B1=AA?= <146086744+edenfunf@users.noreply.github.com> Date: Tue, 28 Apr 2026 18:22:41 +0800 Subject: [PATCH 4/7] refactor(install): group MCP modules under install/mcp/ subpackage Move the seven mcp_*.py helpers under apm_cli/install/ into a focused mcp/ subpackage and strip the redundant `mcp_` prefix from each filename. Aligns with the unprefixed-noun naming convention used by the rest of install/ (pipeline.py, context.py, service.py, ...). Pure refactor: behaviour, public symbol names, and the back-compat re-bound `_xxx` aliases in commands/install.py are unchanged. All install/ unit tests, the #938 regression, and the LOC budget invariant stay green. --- .apm/instructions/tests.instructions.md | 4 ++-- .github/instructions/tests.instructions.md | 4 ++-- CHANGELOG.md | 4 ++++ src/apm_cli/commands/install.py | 20 ++++++++--------- src/apm_cli/install/__init__.py | 1 + src/apm_cli/install/mcp/__init__.py | 18 +++++++++++++++ .../install/{mcp_args.py => mcp/args.py} | 2 +- .../{mcp_command.py => mcp/command.py} | 22 +++++++++---------- .../{mcp_conflicts.py => mcp/conflicts.py} | 0 .../install/{mcp_entry.py => mcp/entry.py} | 2 +- .../{mcp_registry.py => mcp/registry.py} | 4 ++-- .../{mcp_warnings.py => mcp/warnings.py} | 0 .../install/{mcp_writer.py => mcp/writer.py} | 6 ++--- .../unit/install/test_mcp_registry_module.py | 10 ++++----- 14 files changed, 60 insertions(+), 37 deletions(-) create mode 100644 src/apm_cli/install/mcp/__init__.py rename src/apm_cli/install/{mcp_args.py => mcp/args.py} (94%) rename src/apm_cli/install/{mcp_command.py => mcp/command.py} (87%) rename src/apm_cli/install/{mcp_conflicts.py => mcp/conflicts.py} (100%) rename src/apm_cli/install/{mcp_entry.py => mcp/entry.py} (98%) rename src/apm_cli/install/{mcp_registry.py => mcp/registry.py} (98%) rename src/apm_cli/install/{mcp_warnings.py => mcp/warnings.py} (100%) rename src/apm_cli/install/{mcp_writer.py => mcp/writer.py} (96%) 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 9a66911f6..904f4cc6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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) +### Changed + +- 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) + ## [0.10.0] - 2026-04-27 ### Added diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index 012dfdb47..b67cc1092 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -460,10 +460,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, @@ -471,33 +471,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, ) -# MCP --mcp helpers live in apm_cli/install/mcp_*.py per LOC budget. +# 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 ( +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, ) -from ..install.mcp_entry import build_mcp_entry as _build_mcp_entry -from ..install.mcp_writer import ( +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 ( +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 +from ..install.mcp.command import run_mcp_install as _run_mcp_install @click.command( 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 similarity index 94% rename from src/apm_cli/install/mcp_args.py rename to src/apm_cli/install/mcp/args.py index 0cf8bb948..32269a54b 100644 --- a/src/apm_cli/install/mcp_args.py +++ b/src/apm_cli/install/mcp/args.py @@ -1,7 +1,7 @@ """MCP CLI argument parsing for ``--env`` and ``--header`` repetitions. Extracted from ``commands/install.py`` per the architecture-invariants -LOC budget (sibling to ``mcp_warnings.py`` / ``mcp_registry.py``). +LOC budget (sibling to ``warnings.py`` / ``registry.py``). """ from __future__ import annotations diff --git a/src/apm_cli/install/mcp_command.py b/src/apm_cli/install/mcp/command.py similarity index 87% rename from src/apm_cli/install/mcp_command.py rename to src/apm_cli/install/mcp/command.py index bf262925c..77d54aae7 100644 --- a/src/apm_cli/install/mcp_command.py +++ b/src/apm_cli/install/mcp/command.py @@ -1,9 +1,9 @@ """Orchestrator for the ``apm install --mcp`` code path. Extracted from ``commands/install.py`` per the architecture-invariants -LOC budget. ``run_mcp_install`` composes the smaller MCP modules -(``mcp_args``, ``mcp_entry``, ``mcp_writer``, ``mcp_warnings``, -``mcp_registry``) into the user-visible install flow: +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 """ @@ -12,11 +12,11 @@ import click -from .mcp_args import parse_env_pairs, parse_header_pairs -from .mcp_entry import build_mcp_entry -from .mcp_registry import registry_env_override -from .mcp_warnings import warn_shell_metachars, warn_ssrf_url -from .mcp_writer import add_mcp_to_apm_yml +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). @@ -25,8 +25,8 @@ # 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 + from ...deps.lockfile import LockFile, get_lockfile_path + from ...integration.mcp_integrator import MCPIntegrator APM_DEPS_AVAILABLE = True except ImportError: @@ -55,7 +55,7 @@ def run_mcp_install( ): """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 + from ...models.dependency.mcp import MCPDependency env = parse_env_pairs(env_pairs) headers = parse_header_pairs(header_pairs) diff --git a/src/apm_cli/install/mcp_conflicts.py b/src/apm_cli/install/mcp/conflicts.py similarity index 100% rename from src/apm_cli/install/mcp_conflicts.py rename to src/apm_cli/install/mcp/conflicts.py diff --git a/src/apm_cli/install/mcp_entry.py b/src/apm_cli/install/mcp/entry.py similarity index 98% rename from src/apm_cli/install/mcp_entry.py rename to src/apm_cli/install/mcp/entry.py index ce7fc9688..06688f7b4 100644 --- a/src/apm_cli/install/mcp_entry.py +++ b/src/apm_cli/install/mcp/entry.py @@ -28,7 +28,7 @@ def build_mcp_entry(name, *, transport, url, env, headers, version, command_argv :meth:`from_string`) for the validation chokepoint. Validation failures surface as :class:`ValueError` from the model. """ - from ..models.dependency.mcp import MCPDependency + from ...models.dependency.mcp import MCPDependency if command_argv: # Self-defined stdio diff --git a/src/apm_cli/install/mcp_registry.py b/src/apm_cli/install/mcp/registry.py similarity index 98% rename from src/apm_cli/install/mcp_registry.py rename to src/apm_cli/install/mcp/registry.py index eb1a5e6f7..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 @@ -246,7 +246,7 @@ def validate_mcp_dry_run_entry(name, **kwargs) -> None: would reject. Lives here (not in commands/install.py) per the LOC-budget invariant on that module. """ - from .mcp_entry import build_mcp_entry + from .entry import build_mcp_entry try: build_mcp_entry(name, **kwargs) except ValueError as 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 similarity index 96% rename from src/apm_cli/install/mcp_writer.py rename to src/apm_cli/install/mcp/writer.py index d94662bce..6fa8e8fbb 100644 --- a/src/apm_cli/install/mcp_writer.py +++ b/src/apm_cli/install/mcp/writer.py @@ -13,8 +13,8 @@ import click -from ..constants import APM_YML_FILENAME -from ..utils.console import _rich_echo, _rich_warning +from ...constants import APM_YML_FILENAME +from ...utils.console import _rich_echo, _rich_warning def _diff_entry(old, new) -> list: @@ -49,7 +49,7 @@ def add_mcp_to_apm_yml(name, entry, *, dev=False, force=False, project_root=None the CLI exits with code 2. - New entry: append, return ``("added", None)``. """ - from ..utils.yaml_io import dump_yaml, load_yaml + 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(): 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 From bf3ae0773ab8e4949b718e649cac42ae84944c2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=A8=B1=E5=85=83=E8=B1=AA?= <146086744+edenfunf@users.noreply.github.com> Date: Wed, 29 Apr 2026 18:21:19 +0800 Subject: [PATCH 5/7] fix(951): address panel verdict required actions - Restore parse_target_field() in apm_package.from_apm_yml so apm.yml target: values go through the same CSV split / alias resolution / VALID_TARGET_VALUES check as the --target CLI flag. - Restore zero-target install warning in install/phases/targets.py so a misconfigured target: no longer silently deploys nothing. - Re-unify active_targets() / active_targets_user_scope() loops in integration/targets.py via a single normalization step. - Route writer.py replacement-diff output through the injected logger (defaulting to NullCommandLogger) instead of calling _rich_echo / _rich_warning directly, restoring the CommandLogger pattern. - CHANGELOG: document the marketplace url: scope narrowing to https://github.com/ / http://github.com/ under [Unreleased] > Changed. Tests aligned with the parse_target_field behaviour are pulled in alongside (alias resolution, single-element list collapse, unknown-token rejection). --- CHANGELOG.md | 1 + src/apm_cli/core/target_detection.py | 189 +++++++++++++++++------ src/apm_cli/install/mcp/writer.py | 20 +-- src/apm_cli/install/phases/targets.py | 35 +++-- src/apm_cli/integration/targets.py | 118 +++++++------- src/apm_cli/models/apm_package.py | 12 +- tests/unit/core/test_target_detection.py | 20 ++- tests/unit/integration/test_targets.py | 31 +++- tests/unit/test_apm_package.py | 15 +- 9 files changed, 283 insertions(+), 158 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e17a81a6a..01fe8ef33 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ 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) - Rename `DownloadStrategyManager` to `DownloadDelegate` to better reflect Facade/Delegate pattern (#918) - Fix incorrect double-checked locking in marketplace registry `_load()` -- hold lock across full check+read+set (#918) diff --git a/src/apm_cli/core/target_detection.py b/src/apm_cli/core/target_detection.py index 9b0878816..ac963afb3 100644 --- a/src/apm_cli/core/target_detection.py +++ b/src/apm_cli/core/target_detection.py @@ -261,12 +261,141 @@ def normalize_target_list( ) +def parse_target_field( + value: Union[str, List[str], None], + *, + source_path: Optional[Path] = None, +) -> Union[str, List[str], None]: + """Parse, validate, and normalize a target value from any entry point. + + Single source of truth for the ``target`` field, shared by the + ``--target`` CLI flag (via :class:`TargetParamType`) and ``apm.yml``'s + top-level ``target:`` (via :func:`APMPackage.from_apm_yml`). The + output may differ from the input in case (lowercased), order + (preserved but deduplicated), and shape (single-element multi-token + inputs collapse to ``str``). Aliases are resolved for multi-token + input only; see the *Returns* section below for the exact rules. + + Accepted input shapes: + + * ``None`` -> ``None`` (auto-detect at consumption time -- this is the + "field absent" path; an apm.yml without ``target:`` lands here). + * Single token (``"claude"``) -> the same lowercased token as ``str``. + Aliases are NOT resolved for solo input -- ``"copilot"`` returns + ``"copilot"`` (not the canonical ``"vscode"``) to match the + long-standing CLI contract; downstream consumers handle the alias + set explicitly. + * CSV string (``"claude,copilot"``) -> deduplicated ``List[str]`` with + aliases resolved to canonical names. Collapses to a bare ``str`` if + after dedup only one canonical token remains. + * List input (``["claude", "copilot"]``) goes through the same path as + the CSV form -- single-element lists collapse to ``str``. + * Literal ``"all"`` -> ``"all"`` (exclusive; cannot be combined). + + Args: + value: The raw value -- ``str``, ``List[str]``, or ``None``. + source_path: Optional path to the apm.yml that produced ``value``. + When supplied, ValueError messages name the file so users can + jump to it directly. + + Returns: + ``None`` for unset, a ``str`` for a single token (or ``"all"``), + or a deduplicated ``List[str]`` for multi-target input. + + Raises: + ValueError: When the value is an empty / whitespace-only / commas-only + string, an empty list, a non-string non-list type, contains a + token that is not in :data:`VALID_TARGET_VALUES`, or mixes + ``"all"`` with other targets. An empty *string* is treated as + user error (the "field absent" path is ``None``, supplied by + the YAML loader for a missing key). + """ + if value is None: + return None + + # ---- collect raw tokens ---- + if isinstance(value, str): + # Empty / whitespace-only / comma-only strings are user error -- a + # missing field comes through as ``None`` from the YAML loader, so + # an empty *string* means the user typed something invalid. + raw_parts = [v.strip().lower() for v in value.split(",") if v.strip()] + elif isinstance(value, list): + raw_parts = [] + for item in value: + if not isinstance(item, str): + raise ValueError(_target_error( + f"each entry must be a string, got {type(item).__name__}", + source_path, + )) + if item.strip(): + raw_parts.append(item.strip().lower()) + else: + raise ValueError(_target_error( + f"expected string or list of strings, got {type(value).__name__}", + source_path, + )) + + if not raw_parts: + raise ValueError(_target_error("target value must not be empty", source_path)) + + # ---- validate every token ---- + for p in raw_parts: + if p not in VALID_TARGET_VALUES: + raise ValueError(_target_error( + f"'{p}' is not a valid target. " + f"Choose from: {', '.join(sorted(VALID_TARGET_VALUES))}", + source_path, + )) + + # ---- "all" is exclusive ---- + if "all" in raw_parts: + if len(raw_parts) > 1: + raise ValueError(_target_error( + "'all' cannot be combined with other targets", + source_path, + )) + return "all" + + # Single-token input is returned as-is (no alias resolution). This + # preserves the long-standing CLI contract where ``--target copilot`` + # yields ``"copilot"`` rather than the canonical ``"vscode"``; every + # downstream consumer (active_targets, agents_compiler, + # _CROSS_TARGET_MAPS, _TARGET_PREFIXES) already accepts both alias + # spellings, so resolving here would be a visible behaviour change + # with zero functional benefit and would break the CLI test suite + # (~10 ``test_single_*`` cases). This is the one asymmetry #820's + # "shared normalization" intentionally leaves in place; collapsing it + # is an independent decision tracked separately from this fix. + if len(raw_parts) == 1: + return raw_parts[0] + + # Multi-token: resolve aliases + dedupe, preserving input order. + seen: set[str] = set() + result: List[str] = [] + for p in raw_parts: + canonical = TARGET_ALIASES.get(p, p) + if canonical not in seen: + seen.add(canonical) + result.append(canonical) + + if len(result) == 1: + return result[0] + return result + + +def _target_error(message: str, source_path: Optional[Path]) -> str: + """Format a target validation error, naming the source file when known.""" + if source_path is not None: + return f"Invalid 'target' in {source_path}: {message}" + return f"Invalid target: {message}" + + class TargetParamType(click.ParamType): """Click parameter type accepting comma-separated target values. - Single values and ``"all"`` are returned as plain strings for backward - compatibility with existing command handlers. Multiple comma-separated - targets are returned as a deduplicated ``list[str]`` of canonical names. + Delegates to :func:`parse_target_field`, which is the shared validator + used by ``apm.yml``'s ``target:`` field as well -- so ``--target X`` and + ``target: X`` always resolve identically and reject the same inputs. Examples:: @@ -284,50 +413,10 @@ def convert( param: Optional[click.Parameter], ctx: Optional[click.Context], ) -> Union[str, List[str], None]: - if value is None: - return None - # If already converted (e.g. from a default), pass through. - if isinstance(value, list): - return value - - # Split on comma, normalize whitespace & case, drop empty parts. - parts = [v.strip().lower() for v in value.split(",") if v.strip()] - if not parts: - self.fail("target value must not be empty", param, ctx) - - # Validate every token. - for p in parts: - if p not in VALID_TARGET_VALUES: - self.fail( - f"'{p}' is not a valid target. " - f"Choose from: {', '.join(sorted(VALID_TARGET_VALUES))}", - param, - ctx, - ) - - # "all" is exclusive -- reject combinations like "all,claude". - if "all" in parts: - if len(parts) > 1: - self.fail( - "'all' cannot be combined with other targets", - param, - ctx, - ) - return "all" - - # Single target -> plain string (backward compat). - if len(parts) == 1: - return parts[0] - - # Multi-target: resolve aliases and deduplicate. - seen: set[str] = set() - result: List[str] = [] - for p in parts: - canonical = TARGET_ALIASES.get(p, p) - if canonical not in seen: - seen.add(canonical) - result.append(canonical) - # If aliases collapsed everything to one target, return a string. - if len(result) == 1: - return result[0] - return result + try: + return parse_target_field(value) + except ValueError as e: + # Click idiom: route validation errors through self.fail so the + # user sees a clean "Invalid value for '--target': ..." message + # rather than a Python traceback. + self.fail(str(e), param, ctx) diff --git a/src/apm_cli/install/mcp/writer.py b/src/apm_cli/install/mcp/writer.py index 6fa8e8fbb..71df9c4a4 100644 --- a/src/apm_cli/install/mcp/writer.py +++ b/src/apm_cli/install/mcp/writer.py @@ -14,7 +14,7 @@ import click from ...constants import APM_YML_FILENAME -from ...utils.console import _rich_echo, _rich_warning +from ...core.null_logger import NullCommandLogger def _diff_entry(old, new) -> list: @@ -51,6 +51,7 @@ def add_mcp_to_apm_yml(name, entry, *, dev=False, force=False, project_root=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( @@ -91,18 +92,11 @@ def add_mcp_to_apm_yml(name, entry, *, dev=False, force=False, project_root=None 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") + 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 diff --git a/src/apm_cli/install/phases/targets.py b/src/apm_cli/install/phases/targets.py index 44f76f5ae..bfa63a5c7 100644 --- a/src/apm_cli/install/phases/targets.py +++ b/src/apm_cli/install/phases/targets.py @@ -115,21 +115,30 @@ def run(ctx: "InstallContext") -> None: ) raise SystemExit(1) - # Log target detection results - if ctx.logger and _targets: + # Log target detection results. The empty-targets branch is a defensive + # warning -- with parse_target_field as the upstream gatekeeper this + # state is unreachable in normal flow, but a silent zero-target install + # is the worst-case package-manager DX (see #820), so always emit. + if ctx.logger: _scope_label = "global" if _is_user else "project" - _target_names = ", ".join( - f"{t.name} (~/{t.root_dir}/)" if _is_user else t.name - for t in _targets - ) - ctx.logger.verbose_detail( - f"Active {_scope_label} targets: {_target_names}" - ) - if _is_user: - from apm_cli.deps.lockfile import get_lockfile_path - + if _targets: + _target_names = ", ".join( + f"{t.name} (~/{t.root_dir}/)" if _is_user else t.name + for t in _targets + ) ctx.logger.verbose_detail( - f"Lockfile: {get_lockfile_path(ctx.apm_dir)}" + f"Active {_scope_label} targets: {_target_names}" + ) + if _is_user: + from apm_cli.deps.lockfile import get_lockfile_path + + ctx.logger.verbose_detail( + f"Lockfile: {get_lockfile_path(ctx.apm_dir)}" + ) + else: + ctx.logger.warning( + f"No {_scope_label} targets resolved -- nothing will be " + f"deployed. Check 'target:' in apm.yml or use --target." ) for _t in _targets: diff --git a/src/apm_cli/integration/targets.py b/src/apm_cli/integration/targets.py index 6e70385fd..9c746dad8 100644 --- a/src/apm_cli/integration/targets.py +++ b/src/apm_cli/integration/targets.py @@ -3,6 +3,17 @@ Each target tool (Copilot, Claude, Cursor, ...) describes where APM primitives should land. Adding a new target means adding an entry to ``KNOWN_TARGETS`` -- no new classes required. + +Resolver invariant (#820): both :func:`active_targets` and +:func:`active_targets_user_scope` accept ``Union[str, List[str]]`` for +``explicit_target`` but treat the two shapes identically -- string inputs +are wrapped to a one-element list before the resolution loop. Validity +is enforced *upstream* by +:func:`apm_cli.core.target_detection.parse_target_field`, which is the +shared gatekeeper for both ``--target`` and ``apm.yml``'s ``target:`` +field. Unknown tokens never reach these functions in normal flow; if +one does, it falls through the loop without matching any profile and +the result is an empty list (no silent ``[copilot]`` fallback). """ from __future__ import annotations @@ -474,9 +485,11 @@ def active_targets_user_scope( Resolution order: - 1. **Explicit target** (``--target``): returns the matching profile - if it supports user scope. ``"all"`` returns every user-capable - target. A list of names returns all matching user-capable profiles. + 1. **Explicit target** (``--target``): returns the matching profile(s) + that support user scope. ``"all"`` returns every user-capable + target. Validity is enforced upstream by + :func:`apm_cli.core.target_detection.parse_target_field`; this + function does not silently fall back when given unknown tokens. 2. **Directory detection**: profiles whose ``effective_root(user_scope=True)`` directory exists under ``~/``. 3. **Fallback**: ``[copilot]`` -- same default as project scope. @@ -487,37 +500,22 @@ def active_targets_user_scope( # --- explicit target --- if explicit_target: - if isinstance(explicit_target, list): - profiles: list = [] - seen: set = set() - for t in explicit_target: - canonical = t - if canonical in ("copilot", "vscode", "agents"): - canonical = "copilot" - if canonical == "all": - return [ - p for p in KNOWN_TARGETS.values() - if p.user_supported and _flag_gated(p) - ] - profile = KNOWN_TARGETS.get(canonical) - if profile and profile.user_supported and _flag_gated(profile) and profile.name not in seen: - seen.add(profile.name) - profiles.append(profile) - return profiles if profiles else [] - - # single string (existing behavior) - canonical = explicit_target - if canonical in ("copilot", "vscode", "agents"): - canonical = "copilot" - if canonical == "all": - return [ - p for p in KNOWN_TARGETS.values() - if p.user_supported and _flag_gated(p) - ] - profile = KNOWN_TARGETS.get(canonical) - if profile and profile.user_supported and _flag_gated(profile): - return [profile] - return [] + # See module docstring on the parse_target_field gate-keeping contract. + raw = [explicit_target] if isinstance(explicit_target, str) else list(explicit_target) + profiles: list = [] + seen: set = set() + for t in raw: + canonical = "copilot" if t in ("copilot", "vscode", "agents") else t + if canonical == "all": + return [ + p for p in KNOWN_TARGETS.values() + if p.user_supported and _flag_gated(p) + ] + profile = KNOWN_TARGETS.get(canonical) + if profile and profile.user_supported and _flag_gated(profile) and profile.name not in seen: + seen.add(profile.name) + profiles.append(profile) + return profiles # --- auto-detect by directory presence at ~/ --- # Targets with detect_by_dir=False (cowork) are never auto-detected. @@ -544,8 +542,11 @@ def active_targets( Resolution order: 1. **Explicit target** (``--target`` flag or ``apm.yml target:``): - returns only the matching profile(s). ``"all"`` returns every - known target. A list of names returns all matching profiles. + returns the matching profile(s). ``"all"`` returns every known + target. Validity is enforced upstream by + :func:`apm_cli.core.target_detection.parse_target_field`; unknown + tokens never reach here, so this branch never silently falls back + to ``[copilot]``. 2. **Directory detection**: profiles whose ``root_dir`` already exists under *project_root*. 3. **Fallback**: when nothing is detected, returns ``[copilot]`` @@ -562,35 +563,22 @@ def active_targets( # --- explicit target --- if explicit_target: - if isinstance(explicit_target, list): - profiles: list = [] - seen: set = set() - for t in explicit_target: - canonical = t - if canonical in ("copilot", "vscode", "agents"): - canonical = "copilot" - if canonical == "all": - # Return all targets regardless of flag gating. - # The project-scope gate in phases/targets.py and - # for_scope() handle user-observable blocking. - return list(KNOWN_TARGETS.values()) - profile = KNOWN_TARGETS.get(canonical) - if profile and _flag_gated(profile) and profile.name not in seen: - seen.add(profile.name) - profiles.append(profile) - return profiles if profiles else [KNOWN_TARGETS["copilot"]] - - # single string (existing behavior) - canonical = explicit_target - if canonical in ("copilot", "vscode", "agents"): - canonical = "copilot" - if canonical == "all": - # Return all targets regardless of flag gating. - return list(KNOWN_TARGETS.values()) - profile = KNOWN_TARGETS.get(canonical) - if profile and _flag_gated(profile): - return [profile] - return [] + # See module docstring on the parse_target_field gate-keeping contract. + raw = [explicit_target] if isinstance(explicit_target, str) else list(explicit_target) + profiles: list = [] + seen: set = set() + for t in raw: + canonical = "copilot" if t in ("copilot", "vscode", "agents") else t + if canonical == "all": + # Return all targets regardless of flag gating. + # The project-scope gate in phases/targets.py and + # for_scope() handle user-observable blocking. + return list(KNOWN_TARGETS.values()) + profile = KNOWN_TARGETS.get(canonical) + if profile and _flag_gated(profile) and profile.name not in seen: + seen.add(profile.name) + profiles.append(profile) + return profiles # --- auto-detect by directory presence --- # Targets with detect_by_dir=False (cowork) are never auto-detected. diff --git a/src/apm_cli/models/apm_package.py b/src/apm_cli/models/apm_package.py index eee2df4fd..34f91b071 100644 --- a/src/apm_cli/models/apm_package.py +++ b/src/apm_cli/models/apm_package.py @@ -27,6 +27,7 @@ ValidationResult, validate_apm_package, ) +from ..core.target_detection import parse_target_field # Re-export all moved symbols so `from apm_cli.models.apm_package import X` keeps working __all__ = [ @@ -196,6 +197,15 @@ def from_apm_yml(cls, apm_yml_path: Path) -> "APMPackage": else: raise ValueError("'includes' must be 'auto' or a list of strings") + # Parse target field through the same validator as --target so a CSV + # string like ``target: "claude,copilot"`` resolves identically to + # ``--target claude,copilot`` and unknown tokens fail at parse time + # (see apm_cli.core.target_detection.parse_target_field). + target_value = parse_target_field( + data.get('target'), + source_path=apm_yml_path, + ) + result = cls( name=data['name'], version=data['version'], @@ -206,7 +216,7 @@ def from_apm_yml(cls, apm_yml_path: Path) -> "APMPackage": dev_dependencies=dev_dependencies, scripts=data.get('scripts'), package_path=apm_yml_path.parent, - target=data.get('target'), + target=target_value, type=pkg_type, includes=includes, ) diff --git a/tests/unit/core/test_target_detection.py b/tests/unit/core/test_target_detection.py index fcd68ac0e..04b0ee2a8 100644 --- a/tests/unit/core/test_target_detection.py +++ b/tests/unit/core/test_target_detection.py @@ -408,12 +408,22 @@ def test_none_returns_none(self): """None value passes through unchanged.""" assert self.tp.convert(None, None, None) is None - # -- Already-converted list passthrough ------------------------------- + # -- List input goes through the same validator as strings ----------- - def test_list_passthrough(self): - """A list value passes through unchanged.""" - lst = ["claude", "vscode"] - assert self.tp.convert(lst, None, None) is lst + def test_list_input_is_validated(self): + """List input flows through parse_target_field: validated + deduped. + + Returned list is a fresh canonical sequence, not the input list -- + identity is no longer preserved because list and string inputs share + a single normalization path. + """ + result = self.tp.convert(["claude", "vscode"], None, None) + assert result == ["claude", "vscode"] + + def test_list_input_collapses_aliases_to_string(self): + """Multi-element list whose entries all alias to one canonical + target collapses to that single canonical name (``"vscode"``).""" + assert self.tp.convert(["copilot", "agents"], None, None) == "vscode" # -- Single target (backward compat: returns string) ------------------ diff --git a/tests/unit/integration/test_targets.py b/tests/unit/integration/test_targets.py index 29b1b7fe5..75800dff1 100644 --- a/tests/unit/integration/test_targets.py +++ b/tests/unit/integration/test_targets.py @@ -94,9 +94,18 @@ def test_explicit_overrides_detection(self): targets = active_targets(self.root, explicit_target="claude") assert [t.name for t in targets] == ["claude"] - def test_explicit_unknown_returns_empty(self): - targets = active_targets(self.root, explicit_target="nonexistent") - assert targets == [] + def test_unknown_target_raises_at_parse_time(self): + """Unknown tokens in apm.yml or --target must fail at the parser. + + Replaces the previous ``test_explicit_unknown_returns_empty`` -- + the silent-empty contract was the root cause of #820 (apm install + and apm compile exited 0 while deploying nothing). + """ + import pytest + from apm_cli.core.target_detection import parse_target_field + + with pytest.raises(ValueError, match="not a valid target"): + parse_target_field("nonexistent") # -- codex detection -- @@ -171,12 +180,22 @@ def test_explicit_list_all_mixed_returns_every_known_target(self): targets = active_targets(self.root, explicit_target=["claude", "all"]) assert len(targets) == len(KNOWN_TARGETS) - def test_explicit_list_unknown_targets_falls_back_to_copilot(self): + def test_explicit_list_all_unknown_returns_empty(self): + """When the parser is bypassed and all tokens are unknown, the + result is an empty list -- the old asymmetric ``[copilot]`` fallback + was removed in #820 because the parser + (:func:`apm_cli.core.target_detection.parse_target_field`) now + rejects unknown tokens at the entry point.""" targets = active_targets(self.root, explicit_target=["nonexistent", "bogus"]) - assert [t.name for t in targets] == ["copilot"] + assert targets == [] def test_explicit_list_mixed_known_unknown(self): - """Known targets are included, unknown ones are silently skipped.""" + """Known targets are included, unknown ones are skipped (no fallback). + + In normal use the parser rejects this input upstream; this test + exercises the post-parser invariant that the loop only adds known + profiles. + """ targets = active_targets(self.root, explicit_target=["claude", "nonexistent"]) assert [t.name for t in targets] == ["claude"] diff --git a/tests/unit/test_apm_package.py b/tests/unit/test_apm_package.py index f7cc19fd5..889224c86 100644 --- a/tests/unit/test_apm_package.py +++ b/tests/unit/test_apm_package.py @@ -225,7 +225,9 @@ def test_target_string(self, tmp_path): assert isinstance(pkg.target, str) def test_target_list(self, tmp_path): - """target: [claude, copilot] → stored as list.""" + """``target: [claude, copilot]`` is now alias-resolved through the + shared parser -- ``copilot`` collapses to its canonical name + ``vscode`` (#820). Multi-target lists stay as lists.""" yml = _write_apm_yml(tmp_path, { "name": "test-pkg", "version": "1.0.0", @@ -234,7 +236,7 @@ def test_target_list(self, tmp_path): pkg = APMPackage.from_apm_yml(yml) - assert pkg.target == ["claude", "copilot"] + assert pkg.target == ["claude", "vscode"] assert isinstance(pkg.target, list) def test_target_missing(self, tmp_path): @@ -249,7 +251,10 @@ def test_target_missing(self, tmp_path): assert pkg.target is None def test_target_single_item_list(self, tmp_path): - """target: [copilot] → stored as single-element list.""" + """A single-element list (``target: [copilot]``) collapses to a + plain string -- the shared parser canonicalizes ``str`` and + ``[str]`` to the same shape so downstream code only ever sees one + ``Union[str, List[str]]`` form per cardinality (#820).""" yml = _write_apm_yml(tmp_path, { "name": "test-pkg", "version": "1.0.0", @@ -258,8 +263,8 @@ def test_target_single_item_list(self, tmp_path): pkg = APMPackage.from_apm_yml(yml) - assert pkg.target == ["copilot"] - assert isinstance(pkg.target, list) + assert pkg.target == "copilot" + assert isinstance(pkg.target, str) def test_target_direct_construction_string(self): """APMPackage can be constructed with target as string.""" From cd50a35cf5dd385419c401d8e88d81da48a77a1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=A8=B1=E5=85=83=E8=B1=AA?= <146086744+edenfunf@users.noreply.github.com> Date: Thu, 30 Apr 2026 00:34:31 +0800 Subject: [PATCH 6/7] chore(951): address copilot review comments Type-annotate the five mcp/ helper modules extracted from commands/install.py so they match the typing convention already established by their siblings (warnings.py, registry.py). No behavioural change -- annotations only, with from __future__ import annotations already in place so types are stringified at runtime. - args.py: parse_kv_pairs / parse_env_pairs / parse_header_pairs. - entry.py: build_mcp_entry returns the explicit Tuple[Union[str, Dict[str, Any]], bool] tagged-union shape that motivated #938; future call sites that try entry.get(...) on the bare-string branch will now surface as a type-checker error. - writer.py: introduce a module-local MCPEntry alias and fully type _diff_entry / add_mcp_to_apm_yml return shape. - conflicts.py: validate_mcp_conflicts and the MCP_REQUIRED_FLAGS matrix. - command.py: run_mcp_install signature with Path/Optional/Sequence shapes matching the Click handler that calls it. Also fix the patch target in the #938 regression test: test_mcp_registry_shorthand_no_overlays_persists_bare_string was patching apm_cli.commands.install.MCPIntegrator, but the --mcp path is now delegated to apm_cli.install.mcp.command.run_mcp_install, which imports MCPIntegrator from ...integration.mcp_integrator at module load. Patch the binding at the new lookup site so the test actually intercepts the integration call instead of relying on the defensive try/except in command.py to swallow real network failures. --- src/apm_cli/install/mcp/args.py | 14 +++++++--- src/apm_cli/install/mcp/command.py | 37 ++++++++++++++------------- src/apm_cli/install/mcp/conflicts.py | 38 +++++++++++++++------------- src/apm_cli/install/mcp/entry.py | 17 ++++++++++--- src/apm_cli/install/mcp/writer.py | 23 ++++++++++++++--- tests/unit/test_install_command.py | 2 +- 6 files changed, 84 insertions(+), 47 deletions(-) diff --git a/src/apm_cli/install/mcp/args.py b/src/apm_cli/install/mcp/args.py index 32269a54b..789446bb9 100644 --- a/src/apm_cli/install/mcp/args.py +++ b/src/apm_cli/install/mcp/args.py @@ -6,16 +6,22 @@ from __future__ import annotations +from typing import Dict, Iterable, Optional + import click -def parse_kv_pairs(pairs, *, flag_name): +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 = {} + result: Dict[str, str] = {} for raw in pairs or (): if "=" not in raw: raise click.UsageError( @@ -30,11 +36,11 @@ def parse_kv_pairs(pairs, *, flag_name): return result -def parse_env_pairs(pairs): +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): +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 index 77d54aae7..f29927c24 100644 --- a/src/apm_cli/install/mcp/command.py +++ b/src/apm_cli/install/mcp/command.py @@ -10,6 +10,9 @@ from __future__ import annotations +from pathlib import Path +from typing import Optional, Sequence + import click from .args import parse_env_pairs, parse_header_pairs @@ -35,24 +38,24 @@ def run_mcp_install( *, - mcp_name, - transport, - url, - env_pairs, - header_pairs, - mcp_version, - command_argv, - dev, - force, - runtime, - exclude, - verbose, + 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, - apm_dir, - scope, - registry_url=None, -): + 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 diff --git a/src/apm_cli/install/mcp/conflicts.py b/src/apm_cli/install/mcp/conflicts.py index 03e35ddf1..0df07904e 100644 --- a/src/apm_cli/install/mcp/conflicts.py +++ b/src/apm_cli/install/mcp/conflicts.py @@ -8,12 +8,14 @@ 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 = ( +MCP_REQUIRED_FLAGS: Tuple[Tuple[str, str], ...] = ( ("transport", "--transport"), ("url", "--url"), ("env", "--env"), @@ -24,23 +26,23 @@ 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, -): + 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: diff --git a/src/apm_cli/install/mcp/entry.py b/src/apm_cli/install/mcp/entry.py index 06688f7b4..beec85d49 100644 --- a/src/apm_cli/install/mcp/entry.py +++ b/src/apm_cli/install/mcp/entry.py @@ -10,9 +10,20 @@ from __future__ import annotations +from typing import Any, Dict, Mapping, Optional, Sequence, Tuple, Union -def build_mcp_entry(name, *, transport, url, env, headers, version, command_argv, - registry_url=None): + +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: @@ -33,7 +44,7 @@ def build_mcp_entry(name, *, transport, url, env, headers, version, command_argv if command_argv: # Self-defined stdio argv = list(command_argv) - entry: dict = { + entry: Dict[str, Any] = { "name": name, "registry": False, "transport": "stdio", diff --git a/src/apm_cli/install/mcp/writer.py b/src/apm_cli/install/mcp/writer.py index 71df9c4a4..f18c7ed4c 100644 --- a/src/apm_cli/install/mcp/writer.py +++ b/src/apm_cli/install/mcp/writer.py @@ -10,6 +10,7 @@ import sys from pathlib import Path +from typing import Any, Dict, List, Optional, Tuple, Union import click @@ -17,7 +18,13 @@ from ...core.null_logger import NullCommandLogger -def _diff_entry(old, new) -> list: +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: @@ -26,7 +33,7 @@ def _diff_entry(old, new) -> list: 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 = [] + diff: List[str] = [] for k in keys: ov = old_d.get(k, "") nv = new_d.get(k, "") @@ -35,8 +42,16 @@ def _diff_entry(old, new) -> list: return diff -def add_mcp_to_apm_yml(name, entry, *, dev=False, force=False, project_root=None, - manifest_path=None, logger=None): +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``). diff --git a/tests/unit/test_install_command.py b/tests/unit/test_install_command.py index 76eefd383..b8fa7b66a 100644 --- a/tests/unit/test_install_command.py +++ b/tests/unit/test_install_command.py @@ -1682,7 +1682,7 @@ def test_mcp_registry_shorthand_no_overlays_persists_bare_string(self): 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.commands.install.MCPIntegrator"): + 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 From 65ce78de466dbe429413724d4dcf080d5dceb3dd Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Thu, 30 Apr 2026 00:56:29 +0200 Subject: [PATCH 7/7] fix(951): address panel verdict -- redact creds, fail loud on integration error, fix dry-run signature, unhide diff Resolves the 5 required code findings from the review panel verdict (https://github.com/microsoft/apm/pull/951#issuecomment-4347463031): - registry.py: redact `user:password@` from --registry UsageError messages via the existing _redact_url_credentials helper. Three error paths (invalid scheme/host, unsupported scheme) previously echoed raw URLs into CI logs and shell history. - registry.py: replace validate_mcp_dry_run_entry(name, **kwargs) with an explicit keyword-only signature mirroring build_mcp_entry. Unknown kwargs now surface as TypeError at the boundary instead of being silently swallowed. - writer.py: emit the replacement diff via tree_item (always-on) instead of verbose_detail (suppressed unless --verbose). Asking for confirm() while hiding the diff defeated the prompt. - command.py: on MCPIntegrator.install failure, log raw exception at verbose level only, surface a fixed actionable error string, and raise ClickException so the CLI exits 1. Previously the partial-failure path (apm.yml mutated, integration crashed) exited 0 with a warning that forwarded an unredacted exception. - CHANGELOG.md: remove the false 'github.com-only marketplace url: restriction' entry. resolver.py was unchanged in this PR; the entry alarmed GHES/GitLab users with a non-existent breakage and made a security promise the code did not enforce. Tests: - 5 existing --mcp install tests now also patch apm_cli.install.mcp.command.MCPIntegrator (the integrator import moved during the refactor; tests previously masked it via the warning path). - New tests cover credential redaction in UsageError, the typed-kwarg contract on validate_mcp_dry_run_entry, and the exit-1 + redacted output contract on the partial-failure path. Full unit suite: 6712 passed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 1 - src/apm_cli/install/mcp/command.py | 17 ++++++++- src/apm_cli/install/mcp/registry.py | 37 ++++++++++++++++--- src/apm_cli/install/mcp/writer.py | 5 ++- .../unit/install/test_mcp_registry_module.py | 32 ++++++++++++++++ tests/unit/test_install_command.py | 33 +++++++++++++++-- 6 files changed, 111 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4999c9eff..44b19c53c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,7 +21,6 @@ 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) diff --git a/src/apm_cli/install/mcp/command.py b/src/apm_cli/install/mcp/command.py index f29927c24..c33a00d23 100644 --- a/src/apm_cli/install/mcp/command.py +++ b/src/apm_cli/install/mcp/command.py @@ -130,8 +130,21 @@ def run_mcp_install( 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}") + except Exception as exc: + # Keep the raw exception (which may contain internal paths, + # credentials, or stack-trace fragments) at verbose level + # only; surface a fixed actionable string to the user, then + # fail with exit 1 so CI does not see a green run on a + # partial-failure path (apm.yml mutated, integration didn't + # complete). + logger.verbose_detail(f"MCP integration error: {exc}") + logger.error( + "MCP server written to apm.yml but tool integration " + "failed. Run with --verbose for details." + ) + raise click.ClickException( + f"MCP integration failed for '{mcp_name}'" + ) verb = "Replaced" if status == "replaced" else "Added" logger.success(f"{verb} MCP server '{mcp_name}'", symbol="check") diff --git a/src/apm_cli/install/mcp/registry.py b/src/apm_cli/install/mcp/registry.py index bee9594b8..cfa93ff67 100644 --- a/src/apm_cli/install/mcp/registry.py +++ b/src/apm_cli/install/mcp/registry.py @@ -22,7 +22,7 @@ import contextlib import ipaddress import os -from typing import Iterator, Optional, Tuple +from typing import Any, Iterator, Mapping, Optional, Sequence, Tuple from urllib.parse import urlparse, urlunparse import click @@ -119,15 +119,19 @@ def validate_registry_url(value: Optional[str]) -> Optional[str]: f"{_MAX_REGISTRY_URL_LENGTH} characters)" ) parsed = urlparse(normalized) + # Redact credentials before echoing the URL back to the user: any + # ``user:password@`` segment in `value` would otherwise land in + # `UsageError` text and be captured by CI logs / shell history. + safe_value = _redact_url_credentials(value) if not parsed.scheme or not parsed.netloc: raise click.UsageError( - f"--registry: Invalid URL '{value}': expected scheme://host " + f"--registry: Invalid URL '{safe_value}': expected scheme://host " f"(e.g. https://mcp.internal.example.com)" ) scheme = parsed.scheme.lower() if scheme not in _ALLOWED_URL_SCHEMES: raise click.UsageError( - f"--registry: Invalid URL '{value}': scheme '{scheme}' is not " + 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." ) @@ -237,17 +241,38 @@ def registry_env_override(registry_url: Optional[str]) -> Iterator[None]: os.environ[k] = v -def validate_mcp_dry_run_entry(name, **kwargs) -> None: +def validate_mcp_dry_run_entry( + name: str, + *, + transport: Optional[str] = None, + url: Optional[str] = None, + env: Optional[Mapping[str, str]] = None, + headers: Optional[Mapping[str, str]] = None, + version: Optional[str] = None, + command_argv: Optional[Sequence[str]] = None, + registry_url: Optional[str] = None, +) -> 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``, 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. + invariant on that module. The keyword-only signature matches + :func:`build_mcp_entry` exactly so unknown kwargs surface as + ``TypeError`` at the boundary instead of being silently swallowed. """ from .entry import build_mcp_entry try: - build_mcp_entry(name, **kwargs) + build_mcp_entry( + name, + transport=transport, + url=url, + env=env, + headers=headers, + version=version, + command_argv=command_argv, + registry_url=registry_url, + ) except ValueError as exc: raise click.UsageError(str(exc)) diff --git a/src/apm_cli/install/mcp/writer.py b/src/apm_cli/install/mcp/writer.py index f18c7ed4c..0cf22b3a7 100644 --- a/src/apm_cli/install/mcp/writer.py +++ b/src/apm_cli/install/mcp/writer.py @@ -110,8 +110,11 @@ def add_mcp_to_apm_yml( log.warning( f"MCP server '{name}' already exists. Replacement diff:" ) + # Diff lines drive the confirm prompt below: emit unconditionally + # (tree_item is always-on, no --verbose gating) so users always + # see what they are about to confirm. for line in diff: - log.verbose_detail(line) + log.tree_item(line) if not click.confirm(f"Replace MCP server '{name}'?", default=False): return "skipped", diff mcp_list[existing_idx] = entry diff --git a/tests/unit/install/test_mcp_registry_module.py b/tests/unit/install/test_mcp_registry_module.py index cd89c9ecb..305605565 100644 --- a/tests/unit/install/test_mcp_registry_module.py +++ b/tests/unit/install/test_mcp_registry_module.py @@ -178,6 +178,38 @@ def test_empty_rejected(self): with pytest.raises(Exception): validate_registry_url("") + def test_credentials_redacted_in_invalid_url_message(self): + """UsageError text for an unparseable URL must not echo credentials.""" + import click + with pytest.raises(click.UsageError) as exc_info: + validate_registry_url("nothttp://user:topsecret@example.com") + msg = str(exc_info.value.message) + assert "topsecret" not in msg + assert "user:" not in msg + + def test_credentials_redacted_in_unsupported_scheme_message(self): + """UsageError text for an unsupported scheme must not echo credentials.""" + import click + with pytest.raises(click.UsageError) as exc_info: + validate_registry_url("ws://user:topsecret@example.com") + msg = str(exc_info.value.message) + assert "topsecret" not in msg + assert "user:" not in msg + + +class TestValidateMcpDryRunEntrySignature: + """Public-API contract: explicit typed kwargs, no silent **kwargs.""" + + def test_unknown_kwarg_raises_type_error(self): + from apm_cli.install.mcp.registry import validate_mcp_dry_run_entry + with pytest.raises(TypeError): + validate_mcp_dry_run_entry("srv", bogus_kwarg="x") + + def test_accepts_documented_kwargs(self): + from apm_cli.install.mcp.registry import validate_mcp_dry_run_entry + # Should not raise -- bare-string registry shorthand is valid. + validate_mcp_dry_run_entry("srv") + class TestRedactUrlCredentials: """U3 regression: never echo URL credentials in logger output.""" diff --git a/tests/unit/test_install_command.py b/tests/unit/test_install_command.py index b8fa7b66a..bde802e39 100644 --- a/tests/unit/test_install_command.py +++ b/tests/unit/test_install_command.py @@ -1691,6 +1691,26 @@ def test_mcp_registry_shorthand_no_overlays_persists_bare_string(self): # shorthand-with-no-overlays; do not silently promote to a dict. assert data["dependencies"]["mcp"] == [ref] + def test_mcp_integration_failure_exits_1_with_redacted_message(self): + """Partial-failure (apm.yml mutated, integrator raised) must exit 1 + with an actionable string -- not exit 0 with a warning that includes + a raw exception. CI must see a red run on this code path.""" + ref = "io.github.example/srv" + boom = RuntimeError("internal token=ghp_SECRET path=/tmp/x.yml") + 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") as mock_integ: + mock_integ.install.side_effect = boom + result = self.runner.invoke(cli, ["install", "--mcp", ref]) + assert result.exit_code != 0, result.output + # Raw exception details must NOT appear at default log level. + assert "ghp_SECRET" not in result.output + assert "/tmp/x.yml" not in result.output + # Actionable fixed string is shown instead. + assert "tool integration failed" in result.output + assert "--verbose" in result.output + # --- Conflict matrix E1-E14 --- def test_e1_mcp_with_positional_packages(self): @@ -1857,7 +1877,8 @@ def test_registry_https_url_persisted_to_apm_yml(self): patch("apm_cli.commands.install._get_invocation_argv", return_value=["apm", "install", "--mcp", "srv", "--registry", "https://mcp.internal.example.com"]), \ - patch("apm_cli.commands.install.MCPIntegrator"): + patch("apm_cli.commands.install.MCPIntegrator"), \ + patch("apm_cli.install.mcp.command.MCPIntegrator"): result = self.runner.invoke( cli, ["install", "--mcp", "srv", "--registry", "https://mcp.internal.example.com"], @@ -1875,7 +1896,8 @@ def test_registry_http_url_accepted_for_enterprise(self): patch("apm_cli.commands.install._get_invocation_argv", return_value=["apm", "install", "--mcp", "srv", "--registry", "http://mcp.internal.local"]), \ - patch("apm_cli.commands.install.MCPIntegrator"): + patch("apm_cli.commands.install.MCPIntegrator"), \ + patch("apm_cli.install.mcp.command.MCPIntegrator"): result = self.runner.invoke( cli, ["install", "--mcp", "srv", "--registry", "http://mcp.internal.local"], @@ -1890,7 +1912,8 @@ def test_registry_normalizes_trailing_slash(self): patch("apm_cli.commands.install._get_invocation_argv", return_value=["apm", "install", "--mcp", "srv", "--registry", "https://mcp.example.com/"]), \ - patch("apm_cli.commands.install.MCPIntegrator"): + patch("apm_cli.commands.install.MCPIntegrator"), \ + patch("apm_cli.install.mcp.command.MCPIntegrator"): result = self.runner.invoke( cli, ["install", "--mcp", "srv", "--registry", "https://mcp.example.com/"], @@ -2001,6 +2024,7 @@ def test_registry_flag_overrides_env_var(self): return_value=["apm", "install", "--mcp", "srv", "--registry", "https://flag.example.com"]), \ patch("apm_cli.commands.install.MCPIntegrator"), \ + patch("apm_cli.install.mcp.command.MCPIntegrator"), \ patch.dict(os.environ, {"MCP_REGISTRY_URL": "https://env.example.com"}): result = self.runner.invoke( cli, ["install", "--mcp", "srv", "--verbose", @@ -2017,7 +2041,8 @@ def test_registry_with_version_overlay_persists_both(self): return_value=["apm", "install", "--mcp", "srv", "--mcp-version", "1.2.3", "--registry", "https://mcp.example.com"]), \ - patch("apm_cli.commands.install.MCPIntegrator"): + patch("apm_cli.commands.install.MCPIntegrator"), \ + patch("apm_cli.install.mcp.command.MCPIntegrator"): result = self.runner.invoke( cli, ["install", "--mcp", "srv", "--mcp-version", "1.2.3",