From da79ccbdb9281c7de086f27a69c8accb172b3ce8 Mon Sep 17 00:00:00 2001 From: Leonardo Nascimento Date: Thu, 11 Jun 2026 23:19:34 +0100 Subject: [PATCH] fix(marketplace): resolve `check` against each entry's host + token `apm marketplace check` built a single default-host RefResolver with no token and called list_remote_refs(entry.source), ignoring the entry's effective host. After #1736 a relative source under `marketplace.sourceBase` parses as host=None with the bare relative `source`, and the base is composed only on the build (`pack`) path -- which `check` does not share. So `check` ran `git ls-remote https://github.com/.git` and failed with exit 128 for every `sourceBase` entry (and for the pre-existing #1288 host-prefixed form), even when `apm pack` against the same apm.yml succeeded. `check` now mirrors the builder's routing: it composes a host-less source onto `sourceBase` (via the shared `split_source_base`), honours per-entry host overrides, resolves a per-host token through the shared `marketplace/auth_helpers.resolve_token_for_host`, and short-circuits local `./` packages before any network call. Default-host entries are unchanged. Adds three `check` regressions: composed -> base host + token, host-prefixed override -> github, local -> zero ls-remote. Verified end-to-end against a real self-managed GitLab (the exit-128 case now resolves). Follows up #1736 / closes the `check` gap flagged there. --- CHANGELOG.md | 1 + src/apm_cli/commands/marketplace/check.py | 58 ++++++++- src/apm_cli/marketplace/auth_helpers.py | 50 +++++++ tests/unit/commands/test_marketplace_check.py | 122 ++++++++++++++++++ 4 files changed, 227 insertions(+), 4 deletions(-) create mode 100644 src/apm_cli/marketplace/auth_helpers.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 52cf18db1..a081f820a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,6 +62,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 preserving hand-authored files. Use `--no-dedup` / `--force-instructions` to write full `AGENTS.md` files anyway. (by @tillig; closes #1730, related: #1138, #1550) (#1742) +- `apm marketplace check` now resolves each package against its effective host with that host's token, matching `apm pack`. Entries on a non-default host -- a `host.tld/owner/repo` shorthand or a relative source composed onto `marketplace.sourceBase` -- previously failed with `git ls-remote` exit 128 because `check` always probed the default host with no token; local `./` packages now skip the network. Follows up #1736. ## [0.19.0] - 2026-06-09 diff --git a/src/apm_cli/commands/marketplace/check.py b/src/apm_cli/commands/marketplace/check.py index e83fb7f72..1e214c4ed 100644 --- a/src/apm_cli/commands/marketplace/check.py +++ b/src/apm_cli/commands/marketplace/check.py @@ -8,9 +8,11 @@ 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 split_source_base from . import ( _CheckResult, _extract_tag_versions, @@ -21,6 +23,24 @@ ) +def _entry_coordinates(entry, source_base): + """Return ``(host, owner_repo)`` for *entry*, mirroring the build-time + routing in ``MarketplaceBuilder._remote_source_coordinates`` so that + ``check`` and ``pack`` resolve every entry against the same host. + + - A per-entry host (``host.tld/owner/repo`` or full URL) is an override. + - Otherwise, when ``marketplace.sourceBase`` is set, a host-less source + composes onto the base. + - Otherwise the source stays a default-host ``owner/repo``. + """ + if entry.host: + return entry.host, entry.source + if source_base: + base_host, base_path = split_source_base(source_base) + return base_host, f"{base_path}/{entry.source}" + return None, entry.source + + @marketplace.command(help="Validate marketplace entries are resolvable") @click.option("--offline", is_flag=True, help="Schema + cached-ref checks only (no network)") @click.option("--verbose", "-v", is_flag=True, help="Show detailed output") @@ -40,15 +60,44 @@ def check(offline, verbose): symbol="info", ) - resolver = RefResolver(offline=offline) + # One resolver per effective host. 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 token, exactly like ``apm pack`` does. + # Default-host entries keep the bare ambient-credential path. + source_base = getattr(yml, "source_base", None) + resolvers: dict[str | None, RefResolver] = {} + + def _resolver_for(host: str | None) -> RefResolver: + if host not in resolvers: + if host is None: + resolvers[host] = RefResolver(offline=offline) + else: + token = resolve_token_for_host(host, offline=offline) + resolvers[host] = RefResolver(offline=offline, host=host, token=token) + return resolvers[host] + results = [] failure_count = 0 try: for entry in yml.packages: + # Local-path packages skip git resolution entirely. + if entry.is_local: + 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 = _entry_coordinates(entry, source_base) + refs = _resolver_for(host).list_remote_refs(owner_repo) # Check version/ref resolution ref_ok = False @@ -152,4 +201,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 000000000..9094ac763 --- /dev/null +++ b/src/apm_cli/marketplace/auth_helpers.py @@ -0,0 +1,50 @@ +"""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: + from ..core.auth import AuthResolver # lazy import to avoid cycles + + resolver = auth_resolver if auth_resolver is not None else AuthResolver() + 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/tests/unit/commands/test_marketplace_check.py b/tests/unit/commands/test_marketplace_check.py index c7511f62d..4ebe3baa7 100644 --- a/tests/unit/commands/test_marketplace_check.py +++ b/tests/unit/commands/test_marketplace_check.py @@ -538,3 +538,125 @@ 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, not github.com + mock_token.assert_called_once_with("gitlab.example.com", offline=False) + # 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 + mock_token.assert_called_once_with("github.com", offline=False) + 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()