diff --git a/src/apm_cli/commands/marketplace.py b/src/apm_cli/commands/marketplace.py index 1c62c0c61..1ea8a7ccd 100644 --- a/src/apm_cli/commands/marketplace.py +++ b/src/apm_cli/commands/marketplace.py @@ -7,6 +7,7 @@ import builtins import json import os +import re import subprocess import sys import traceback @@ -44,6 +45,16 @@ from ._helpers import _get_console, _is_interactive +# Marketplace alias must satisfy this pattern so it can appear on the right of +# ``@`` in ``apm install @`` syntax. +_ALIAS_PATTERN = re.compile(r"^[a-zA-Z0-9._-]+$") + + +def _is_valid_alias(value: str) -> bool: + """Return True when ``value`` is a legal marketplace alias.""" + return bool(value) and _ALIAS_PATTERN.match(value) is not None + + # --------------------------------------------------------------------------- # Custom group for organised --help output # --------------------------------------------------------------------------- @@ -336,28 +347,22 @@ def add(repo, name, branch, host, verbose): resolved_host = normalized_host else: resolved_host = default_host() - display_name = name or repo_name - - # Validate name is identifier-compatible for NAME@MARKETPLACE syntax - import re - if not re.match(r"^[a-zA-Z0-9._-]+$", display_name): + # Hard-fail if the user-supplied --name flag is malformed; the + # manifest's name is validated softly below (publisher mistakes + # shouldn't break a successful add). + if name is not None and not _is_valid_alias(name): logger.error( - f"Invalid marketplace name: '{display_name}'. " + f"Invalid marketplace name: '{name}'. " f"Names must only contain letters, digits, '.', '_', and '-' " f"(required for 'apm install plugin@marketplace' syntax)." ) sys.exit(1) - logger.start(f"Registering marketplace '{display_name}'...", symbol="gear") - logger.verbose_detail(f" Repository: {owner}/{repo_name}") - logger.verbose_detail(f" Branch: {branch}") - if resolved_host != "github.com": - logger.verbose_detail(f" Host: {resolved_host}") - - # Auto-detect marketplace.json location + # Probe for the marketplace.json location. The probe source's name + # is a placeholder -- _auto_detect_path only consults host/owner/repo. probe_source = MarketplaceSource( - name=display_name, + name=name or repo_name, owner=owner, repo=repo_name, branch=branch, @@ -373,9 +378,56 @@ def add(repo, name, branch, host, verbose): ) sys.exit(1) + # Fetch and validate the manifest before logging start, so that the + # success/start lines display the *final* alias the user must use. + fetch_source = MarketplaceSource( + name=name or repo_name, + owner=owner, + repo=repo_name, + branch=branch, + host=resolved_host, + path=detected_path, + ) + manifest = fetch_marketplace(fetch_source, force_refresh=True) + plugin_count = len(manifest.plugins) + + # Resolve final alias: --name flag > manifest.name (if valid) > repo name. + # Track which tier won so we can report it in verbose mode and emit a + # warning when a publisher-declared name had to be rejected. + manifest_name = (manifest.name or "").strip() + if name is not None: + display_name = name + alias_source = "--name flag" + elif manifest_name and _is_valid_alias(manifest_name): + display_name = manifest_name + alias_source = f"manifest.name ('{manifest_name}')" + else: + display_name = repo_name + if manifest_name and not _is_valid_alias(manifest_name): + logger.warning( + f"Manifest declares name '{manifest_name}' which is not a " + f"valid alias (must match [a-zA-Z0-9._-]+). " + f"Falling back to repo name." + ) + alias_source = f"repo name (manifest.name '{manifest_name}' invalid)" + else: + alias_source = "repo name (manifest.name missing)" + + # Defense-in-depth: repo names from GitHub already satisfy the alias + # regex, so this invariant should always hold by the time we register. + assert _is_valid_alias(display_name), ( + f"Resolved marketplace alias '{display_name}' failed validation" + ) + + logger.start(f"Registering marketplace '{display_name}'...", symbol="gear") + logger.verbose_detail(f" Repository: {owner}/{repo_name}") + logger.verbose_detail(f" Branch: {branch}") + if resolved_host != "github.com": + logger.verbose_detail(f" Host: {resolved_host}") logger.verbose_detail(f" Detected path: {detected_path}") + logger.verbose_detail(f" Alias source: {alias_source}") - # Create source with detected path + # Persist with the final alias. source = MarketplaceSource( name=display_name, owner=owner, @@ -384,12 +436,6 @@ def add(repo, name, branch, host, verbose): host=resolved_host, path=detected_path, ) - - # Fetch and validate - manifest = fetch_marketplace(source, force_refresh=True) - plugin_count = len(manifest.plugins) - - # Register add_marketplace(source) logger.success( @@ -399,6 +445,14 @@ def add(repo, name, branch, host, verbose): if manifest.description: logger.verbose_detail(f" {manifest.description}") + # Surface the install syntax only when the alias is something the user + # could not have predicted from OWNER/REPO. Silence is fine otherwise. + if name is None and display_name != repo_name: + logger.progress( + f"Install plugins with: apm install @{display_name}", + symbol="info", + ) + except Exception as e: # noqa: BLE001 -- top-level command catch-all logger.error(f"Failed to register marketplace: {e}") if verbose: diff --git a/tests/unit/marketplace/test_marketplace_commands.py b/tests/unit/marketplace/test_marketplace_commands.py index bd96bd331..b5757206a 100644 --- a/tests/unit/marketplace/test_marketplace_commands.py +++ b/tests/unit/marketplace/test_marketplace_commands.py @@ -38,6 +38,154 @@ def test_invalid_format_no_slash(self, runner): assert result.exit_code != 0 assert "OWNER/REPO" in result.output + @patch("apm_cli.marketplace.registry.add_marketplace") + @patch("apm_cli.marketplace.client.fetch_marketplace") + @patch("apm_cli.marketplace.client._auto_detect_path") + def test_add_uses_manifest_name_when_available( + self, mock_detect, mock_fetch, mock_add, runner + ): + """Manifest's `name` field becomes the registered alias.""" + from apm_cli.commands.marketplace import marketplace + + mock_detect.return_value = ".claude-plugin/marketplace.json" + mock_fetch.return_value = MarketplaceManifest( + name="addy-agent-skills", + plugins=(MarketplacePlugin(name="agent-skills"),), + ) + + result = runner.invoke(marketplace, ["add", "addyosmani/agent-skills"]) + assert result.exit_code == 0 + # Registered source carries the manifest's name, not the repo name. + registered_source = mock_add.call_args[0][0] + assert registered_source.name == "addy-agent-skills" + assert registered_source.repo == "agent-skills" + # Install hint surfaces the alias the user must use next. + assert "apm install @addy-agent-skills" in result.output + + @patch("apm_cli.marketplace.registry.add_marketplace") + @patch("apm_cli.marketplace.client.fetch_marketplace") + @patch("apm_cli.marketplace.client._auto_detect_path") + def test_add_cli_name_overrides_manifest( + self, mock_detect, mock_fetch, mock_add, runner + ): + """An explicit --name flag wins over the manifest's name.""" + from apm_cli.commands.marketplace import marketplace + + mock_detect.return_value = "marketplace.json" + mock_fetch.return_value = MarketplaceManifest( + name="manifest-alias", + plugins=(MarketplacePlugin(name="p1"),), + ) + + result = runner.invoke( + marketplace, ["add", "acme/plugins", "--name", "custom-alias"] + ) + assert result.exit_code == 0 + registered_source = mock_add.call_args[0][0] + assert registered_source.name == "custom-alias" + # No install hint when the user explicitly chose the alias. + assert "Install plugins with" not in result.output + + @patch("apm_cli.marketplace.registry.add_marketplace") + @patch("apm_cli.marketplace.client.fetch_marketplace") + @patch("apm_cli.marketplace.client._auto_detect_path") + def test_add_falls_back_when_manifest_name_invalid( + self, mock_detect, mock_fetch, mock_add, runner + ): + """Invalid manifest.name triggers a soft fallback to the repo name.""" + from apm_cli.commands.marketplace import marketplace + + mock_detect.return_value = "marketplace.json" + mock_fetch.return_value = MarketplaceManifest( + name="has spaces!", + plugins=(MarketplacePlugin(name="p1"),), + ) + + result = runner.invoke(marketplace, ["add", "acme/plugins"]) + # Soft fallback: the command still succeeds. + assert result.exit_code == 0 + registered_source = mock_add.call_args[0][0] + assert registered_source.name == "plugins" + # User sees a warning quoting the offending value. + assert "has spaces!" in result.output + assert "Falling back to repo name" in result.output + + @patch("apm_cli.marketplace.registry.add_marketplace") + @patch("apm_cli.marketplace.client.fetch_marketplace") + @patch("apm_cli.marketplace.client._auto_detect_path") + def test_add_falls_back_when_manifest_name_missing( + self, mock_detect, mock_fetch, mock_add, runner + ): + """Empty manifest.name silently falls back to the repo name.""" + from apm_cli.commands.marketplace import marketplace + + mock_detect.return_value = "marketplace.json" + mock_fetch.return_value = MarketplaceManifest( + name="", + plugins=(MarketplacePlugin(name="p1"),), + ) + + result = runner.invoke(marketplace, ["add", "acme/plugins"]) + assert result.exit_code == 0 + registered_source = mock_add.call_args[0][0] + assert registered_source.name == "plugins" + # No warning when the publisher simply omitted the field. + assert "Falling back" not in result.output + # No install hint either: alias matches the repo name -- predictable. + assert "Install plugins with" not in result.output + + def test_add_rejects_invalid_cli_name(self, runner): + """An invalid --name flag is a user error and hard-fails.""" + from apm_cli.commands.marketplace import marketplace + + result = runner.invoke( + marketplace, ["add", "acme/plugins", "--name", "bad name"] + ) + assert result.exit_code != 0 + assert "Invalid marketplace name" in result.output + + @patch("apm_cli.marketplace.registry.add_marketplace") + @patch("apm_cli.marketplace.client.fetch_marketplace") + @patch("apm_cli.marketplace.client._auto_detect_path") + def test_add_awesome_copilot_pattern_unchanged( + self, mock_detect, mock_fetch, mock_add, runner + ): + """Regression: github/awesome-copilot manifest name == repo name -> no behaviour change.""" + from apm_cli.commands.marketplace import marketplace + + mock_detect.return_value = ".github/plugin/marketplace.json" + mock_fetch.return_value = MarketplaceManifest( + name="awesome-copilot", + plugins=(MarketplacePlugin(name="azure-cloud-development"),), + ) + + result = runner.invoke(marketplace, ["add", "github/awesome-copilot"]) + assert result.exit_code == 0 + registered_source = mock_add.call_args[0][0] + assert registered_source.name == "awesome-copilot" + # Alias matches the repo name, so the install hint is suppressed. + assert "Install plugins with" not in result.output + + @patch("apm_cli.marketplace.client.fetch_marketplace") + @patch("apm_cli.marketplace.client._auto_detect_path") + def test_add_verbose_shows_alias_source( + self, mock_detect, mock_fetch, runner + ): + """Verbose mode reports which precedence tier picked the alias.""" + from apm_cli.commands.marketplace import marketplace + + mock_detect.return_value = "marketplace.json" + mock_fetch.return_value = MarketplaceManifest( + name="acme-tools", + plugins=(MarketplacePlugin(name="p1"),), + ) + + result = runner.invoke( + marketplace, ["add", "acme/plugins", "--verbose"] + ) + assert result.exit_code == 0 + assert "Alias source: manifest.name" in result.output + @patch("apm_cli.marketplace.client.fetch_marketplace") @patch("apm_cli.marketplace.client._auto_detect_path") def test_successful_add(self, mock_detect, mock_fetch, runner):