diff --git a/CHANGELOG.md b/CHANGELOG.md index 114ff5439c..8a3c93a529 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `apm marketplace check` no longer fails with exit 128 for entries on + non-default hosts, including relative entries composed onto + `marketplace.sourceBase` (self-managed GitLab / GHES / Azure DevOps). It now + resolves each entry against its effective host and per-org token, matching + `apm pack`; local `./` packages skip the network. + (closes #1762, follows up #1736) - Policy inheritance no longer drops `fetch_failure`, `registry_source`, and `bin_deploy` when a policy `extends` another; these fields now carry and tighten through the merge like sibling sections. (closes #1778) (#1791) - `apm install` no longer silently ignores MCP servers declared in `devDependencies.mcp`; dev MCP configs and lockfile entries now stay in sync on fresh installs. (closes #1780) (#1787) - `apm compile` now honors `managed_section` mode on distributed root diff --git a/src/apm_cli/commands/marketplace/check.py b/src/apm_cli/commands/marketplace/check.py index e83fb7f728..fa19058632 100644 --- a/src/apm_cli/commands/marketplace/check.py +++ b/src/apm_cli/commands/marketplace/check.py @@ -4,13 +4,16 @@ import sys import traceback +from typing import TYPE_CHECKING import click from ...core.command_logger import CommandLogger +from ...marketplace.auth_helpers import resolve_token_for_host from ...marketplace.errors import GitLsRemoteError, OfflineMissError from ...marketplace.ref_resolver import RefResolver from ...marketplace.semver import satisfies_range +from ...marketplace.yml_schema import PackageEntry, split_source_base from . import ( _CheckResult, _extract_tag_versions, @@ -20,6 +23,34 @@ marketplace, ) +if TYPE_CHECKING: + from ...core.auth import AuthResolver + + +def _entry_coordinates( + entry: PackageEntry, source_base: str | None +) -> tuple[str | None, str, str | None]: + """Return ``(host, owner_repo, org)`` for *entry*, mirroring the build-time + routing in ``MarketplaceBuilder._remote_source_coordinates`` so that + ``check`` and ``pack`` resolve every entry against the same host AND the + same per-org auth hint. + + - A per-entry host (``host.tld/owner/repo`` or full URL) is an override + with no org hint, matching the builder. + - Otherwise, when ``marketplace.sourceBase`` is set, a host-less source + composes onto the base and the base's leading path segment becomes the + per-org auth hint (so ``GITHUB_APM_PAT_{ORG}`` resolves identically to + ``pack``). + - Otherwise the source stays a default-host ``owner/repo`` with no hint. + """ + if entry.host: + return entry.host, entry.source, None + if source_base: + base_host, base_path = split_source_base(source_base) + org = base_path.split("/", 1)[0] if base_path else None + return base_host, f"{base_path}/{entry.source}", org + return None, entry.source, None + @marketplace.command(help="Validate marketplace entries are resolvable") @click.option("--offline", is_flag=True, help="Schema + cached-ref checks only (no network)") @@ -40,15 +71,62 @@ def check(offline, verbose): symbol="info", ) - resolver = RefResolver(offline=offline) + # One resolver per effective (host, org) pair. An entry whose source + # named a non-default host -- a host-prefixed source or a relative source + # composed onto ``marketplace.sourceBase`` -- must be resolved against + # that host with the host's (and, for sourceBase, the org's) token, + # exactly like ``apm pack`` does. Default-host entries keep the bare + # ambient-credential path. + source_base = getattr(yml, "source_base", None) + resolvers: dict[tuple[str | None, str | None], RefResolver] = {} + auth_resolver: AuthResolver | None = None + + def _resolver_for(host: str | None, org: str | None) -> RefResolver: + nonlocal auth_resolver + key = (host, org) + if key not in resolvers: + if host is None: + resolvers[key] = RefResolver(offline=offline) + else: + if auth_resolver is None and not offline: + from ...core.auth import AuthResolver + + auth_resolver = AuthResolver() + token = resolve_token_for_host( + host, + offline=offline, + org=org, + auth_resolver=auth_resolver, + ) + resolvers[key] = RefResolver(offline=offline, host=host, token=token) + return resolvers[key] + results = [] failure_count = 0 try: for entry in yml.packages: + # Local-path packages skip git resolution entirely. + if entry.is_local: + logger.verbose_detail(f"Skipping {entry.name} -- local path, no network check") + results.append( + _CheckResult( + name=entry.name, + reachable=True, + version_found=True, + ref_ok=True, + error="", + ) + ) + continue try: - # Attempt to resolve each entry - refs = resolver.list_remote_refs(entry.source) + # Resolve each entry against its effective host + composed path. + host, owner_repo, org = _entry_coordinates(entry, source_base) + remote_label = f"https://{host}/{owner_repo}.git" if host else owner_repo + logger.verbose_detail( + f"Resolving {entry.name} via {host or 'default host'}: {remote_label}" + ) + refs = _resolver_for(host, org).list_remote_refs(owner_repo) # Check version/ref resolution ref_ok = False @@ -152,4 +230,5 @@ def check(offline, verbose): logger.success(f"All {total} entries OK", symbol="check") finally: - resolver.close() + for resolver in resolvers.values(): + resolver.close() diff --git a/src/apm_cli/marketplace/auth_helpers.py b/src/apm_cli/marketplace/auth_helpers.py new file mode 100644 index 0000000000..da9363c905 --- /dev/null +++ b/src/apm_cli/marketplace/auth_helpers.py @@ -0,0 +1,53 @@ +"""Shared auth helper for marketplace commands. + +Both ``apm pack`` (``MarketplaceBuilder``) and ``apm marketplace check`` need +to resolve a per-host token before running ``git ls-remote`` against a +non-default host (self-managed GitLab, GHES, ADO, Bitbucket DC). Keeping this +in one place stops the two paths from drifting -- the divergence that left +``check`` resolving every entry against the default host regardless of the +entry's real host (see #1519 follow-up). +""" + +from __future__ import annotations + +import logging +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from ..core.auth import AuthResolver + +logger = logging.getLogger(__name__) + + +def resolve_token_for_host( + host: str, + *, + offline: bool = False, + org: str | None = None, + auth_resolver: AuthResolver | None = None, +) -> str | None: + """Resolve an auth token for *host* via ``AuthResolver``. + + Returns ``None`` -- letting ``git`` fall back to ambient credentials -- + when offline, when no token is configured for the host, or when + ``AuthResolver`` raises. Never raises. + + Pass *auth_resolver* to reuse a cached resolver across many calls; + otherwise a fresh one is created per call. + """ + if offline: + return None + try: + if auth_resolver is None: + from ..core.auth import AuthResolver # lazy import to avoid cycles + + resolver = AuthResolver() + else: + resolver = auth_resolver + ctx = resolver.resolve(host) if org is None else resolver.resolve(host, org=org) + if ctx.token: + logger.debug("Resolved token for host %s (source=%s)", host, ctx.source) + return ctx.token + except Exception: + logger.debug("Could not resolve token for host %s", host, exc_info=True) + return None diff --git a/src/apm_cli/marketplace/builder.py b/src/apm_cli/marketplace/builder.py index b3c8880945..98a4d2c4b5 100644 --- a/src/apm_cli/marketplace/builder.py +++ b/src/apm_cli/marketplace/builder.py @@ -37,6 +37,7 @@ from ..utils.path_security import ensure_path_within from ._io import atomic_write from ._shared import iter_semver_tags +from .auth_helpers import resolve_token_for_host from .diagnostics import BuildDiagnostic from .errors import ( BuildError, @@ -454,28 +455,21 @@ def _get_resolver_for_host(self, host: str | None, *, org: str | None = None) -> return resolver def _resolve_token_for_host(self, host: str, *, org: str | None = None) -> str | None: - """Resolve an auth token for *host* via ``AuthResolver``. - - Returns ``None`` -- letting ``git`` fall back to ambient credentials - -- when offline, when no token is configured for the host, or when - ``AuthResolver`` raises. Never raises. - """ + """Resolve an auth token for *host* via the shared marketplace helper.""" if self._options.offline: return None - try: - from ..core.auth import AuthResolver # lazy import - - resolver = self._auth_resolver - if resolver is None: - resolver = AuthResolver() - self._auth_resolver = resolver - ctx = resolver.resolve(host) if org is None else resolver.resolve(host, org=org) - if ctx.token: - logger.debug("Resolved token for host %s (source=%s)", host, ctx.source) - return ctx.token - except Exception: - logger.debug("Could not resolve token for host %s", host, exc_info=True) - return None + from ..core.auth import AuthResolver # lazy import + + resolver = self._auth_resolver + if resolver is None: + resolver = AuthResolver() + self._auth_resolver = resolver + return resolve_token_for_host( + host, + offline=self._options.offline, + org=org, + auth_resolver=resolver, + ) def _ensure_auth(self) -> None: """Lazily resolve host classification and GitHub token. diff --git a/tests/unit/commands/test_marketplace_check.py b/tests/unit/commands/test_marketplace_check.py index c7511f62d5..db5acb8827 100644 --- a/tests/unit/commands/test_marketplace_check.py +++ b/tests/unit/commands/test_marketplace_check.py @@ -4,7 +4,7 @@ import textwrap from pathlib import Path # noqa: F401 -from unittest.mock import MagicMock, patch +from unittest.mock import ANY, MagicMock, patch import pytest from click.testing import CliRunner @@ -538,3 +538,130 @@ def test_heads_prefix_not_confused_with_tags(self, MockResolver, runner, tmp_pat # refs/tags/main stripped to "main" → tag_name == "main" == entry.ref → passes # (this also tests the tags branch hits and indirectly confirms the heads branch too) assert result.exit_code == 0 + + +# --------------------------------------------------------------------------- +# Per-host resolution (#1519 follow-up): check must resolve each entry against +# its effective host with that host's token, matching `apm pack`. A relative +# source composed onto `sourceBase`, or a host-prefixed source, targets a +# non-default host; bare `owner/repo` keeps the default-host path. +# --------------------------------------------------------------------------- + + +class TestCheckPerHostResolution: + @patch("apm_cli.commands.marketplace.check.resolve_token_for_host", return_value="glpat-xyz") + @patch("apm_cli.commands.marketplace.check.RefResolver") + @patch("apm_cli.commands.marketplace.check._load_config_or_exit") + def test_sourcebase_entry_composes_and_uses_host_token( + self, mock_load, MockResolver, mock_token, runner, tmp_path, monkeypatch + ): + monkeypatch.chdir(tmp_path) + (tmp_path / "marketplace.yml").write_text("---\n", encoding="utf-8") + mock_load.return_value = ( + tmp_path, + MarketplaceYml( + name="m", + description="d", + version="1.0.0", + owner=MarketplaceOwner(name="o"), + source_base="https://gitlab.example.com/group/sub/team/project", + packages=( + PackageEntry( + name="my-package", + source="my-package", # relative, host=None as parsed + version="^1.0.0", + ), + ), + ), + ) + mock_inst = MockResolver.return_value + mock_inst.list_remote_refs.return_value = _REFS_GOOD + mock_inst.close = MagicMock() + + result = runner.invoke(marketplace, ["check"]) + + assert result.exit_code == 0 + # token resolved for the GitLab base host, with the sourceBase org hint + # (leading path segment) -- identical to what ``apm pack`` resolves + mock_token.assert_called_once_with( + "gitlab.example.com", offline=False, org="group", auth_resolver=ANY + ) + # resolver bound to that host with that token + MockResolver.assert_called_once_with( + offline=False, host="gitlab.example.com", token="glpat-xyz" + ) + # ls-remote runs against the composed nested path + mock_inst.list_remote_refs.assert_called_once_with("group/sub/team/project/my-package") + + @patch("apm_cli.commands.marketplace.check.resolve_token_for_host", return_value="ghp-tok") + @patch("apm_cli.commands.marketplace.check.RefResolver") + @patch("apm_cli.commands.marketplace.check._load_config_or_exit") + def test_host_prefixed_override_uses_that_host( + self, mock_load, MockResolver, mock_token, runner, tmp_path, monkeypatch + ): + monkeypatch.chdir(tmp_path) + (tmp_path / "marketplace.yml").write_text("---\n", encoding="utf-8") + mock_load.return_value = ( + tmp_path, + MarketplaceYml( + name="m", + description="d", + version="1.0.0", + owner=MarketplaceOwner(name="o"), + source_base="https://gitlab.example.com/group/sub", + packages=( + PackageEntry( + name="helper", + source="owner/repo", + version="^1.0.0", + host="github.com", # host-prefixed override: base ignored + ), + ), + ), + ) + mock_inst = MockResolver.return_value + mock_inst.list_remote_refs.return_value = _REFS_GOOD + mock_inst.close = MagicMock() + + result = runner.invoke(marketplace, ["check"]) + + assert result.exit_code == 0 + # host-prefixed override carries no sourceBase org hint (org=None), + # matching the builder's _remote_source_coordinates + mock_token.assert_called_once_with("github.com", offline=False, org=None, auth_resolver=ANY) + MockResolver.assert_called_once_with(offline=False, host="github.com", token="ghp-tok") + mock_inst.list_remote_refs.assert_called_once_with("owner/repo") + + @patch("apm_cli.commands.marketplace.check.resolve_token_for_host") + @patch("apm_cli.commands.marketplace.check.RefResolver") + @patch("apm_cli.commands.marketplace.check._load_config_or_exit") + def test_local_entry_makes_zero_ls_remote_calls( + self, mock_load, MockResolver, mock_token, runner, tmp_path, monkeypatch + ): + monkeypatch.chdir(tmp_path) + (tmp_path / "marketplace.yml").write_text("---\n", encoding="utf-8") + mock_load.return_value = ( + tmp_path, + MarketplaceYml( + name="m", + description="d", + version="1.0.0", + owner=MarketplaceOwner(name="o"), + source_base="https://gitlab.example.com/group/sub", + packages=( + PackageEntry( + name="local-tool", + source="./packages/local-tool", + is_local=True, + ), + ), + ), + ) + + result = runner.invoke(marketplace, ["check"]) + + assert result.exit_code == 0 + assert "All 1 entries OK" in result.output + # local sources never touch the network or the token resolver + MockResolver.assert_not_called() + mock_token.assert_not_called() diff --git a/tests/unit/marketplace/test_auth_helpers.py b/tests/unit/marketplace/test_auth_helpers.py new file mode 100644 index 0000000000..4b05933556 --- /dev/null +++ b/tests/unit/marketplace/test_auth_helpers.py @@ -0,0 +1,54 @@ +"""Tests for marketplace auth helpers.""" + +from __future__ import annotations + +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +from apm_cli.marketplace.auth_helpers import resolve_token_for_host + + +def test_resolve_token_for_host_offline_returns_none_without_resolver() -> None: + """Offline checks must not touch AuthResolver.""" + with patch("apm_cli.core.auth.AuthResolver") as mock_resolver: + assert resolve_token_for_host("gitlab.example.com", offline=True) is None + + mock_resolver.assert_not_called() + + +def test_resolve_token_for_host_returns_token_from_supplied_resolver() -> None: + """A supplied resolver is used directly and its token is returned.""" + resolver = MagicMock() + resolver.resolve.return_value = SimpleNamespace(token="glpat-token", source="TEST") + + token = resolve_token_for_host( + "gitlab.example.com", + org="group", + auth_resolver=resolver, + ) + + assert token == "glpat-token" + resolver.resolve.assert_called_once_with("gitlab.example.com", org="group") + + +def test_resolve_token_for_host_returns_none_when_resolver_raises() -> None: + """Auth failures must fall back to ambient git credentials.""" + resolver = MagicMock() + resolver.resolve.side_effect = RuntimeError("auth unavailable") + + assert resolve_token_for_host("gitlab.example.com", auth_resolver=resolver) is None + + +def test_resolve_token_for_host_creates_resolver_when_not_supplied() -> None: + """Callers may let the helper create AuthResolver lazily.""" + with patch("apm_cli.core.auth.AuthResolver") as mock_resolver: + mock_resolver.return_value.resolve.return_value = SimpleNamespace( + token="ghp-token", + source="TEST", + ) + + token = resolve_token_for_host("github.example.com") + + assert token == "ghp-token" + mock_resolver.assert_called_once_with() + mock_resolver.return_value.resolve.assert_called_once_with("github.example.com")