From 1f9a723f8697e8b2a3f073b5efebd7f956bb355b Mon Sep 17 00:00:00 2001 From: Rodion Kazennov Date: Sat, 3 Oct 2026 12:04:56 +0000 Subject: [PATCH 1/2] fix(marketplace): record the commit an annotated tag points to `git ls-remote` lists an annotated or signed tag twice: the tag object under `refs/tags/` and its commit under `refs/tags/^{}`. `_parse_ls_remote_output` dropped the `^{}` line, so `apm pack` wrote the tag object as `source.sha`, and installers that check out the tag and compare the result rejected the pin. The tag now takes the peeled SHA, as `parse_ls_remote_output` in `deps/git_remote_ops.py` already does. Lightweight tags and branches keep their only SHA. Fixes #3048 --- CHANGELOG.md | 1 + src/apm_cli/marketplace/ref_resolver.py | 14 +- .../test_annotated_tag_source_sha.py | 188 ++++++++++++++++++ tests/unit/marketplace/test_ref_resolver.py | 36 +++- 4 files changed, 233 insertions(+), 6 deletions(-) create mode 100644 tests/unit/marketplace/test_annotated_tag_source_sha.py diff --git a/CHANGELOG.md b/CHANGELOG.md index beb4e53002..ed50a8422b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Codex agent conversion preserves native `model` and `model_reasoning_effort` settings and warns about dropped metadata instead of silently losing it. (#3150) - Cursor hooks use native v1 events and flat handlers; unsupported input and overlapping Claude imports now fail explicitly. Choose one hook route per dependency; see the [supported mappings](docs/src/content/docs/producer/author-primitives/hooks-and-commands.md#cursor-native-hooks-and-claude-import) and `openapm-v0.1.md` requirements `req-tg-016/017`. (#3149) +- `apm pack` records the commit an annotated or signed tag points to as `source.sha` instead of the tag object, so plugin installers that check out the tag accept the pin. (by @nefayran, closes #3048, #3161) ## [0.33.0] - 2026-10-02 diff --git a/src/apm_cli/marketplace/ref_resolver.py b/src/apm_cli/marketplace/ref_resolver.py index 04e94660c1..3cbe6816dd 100644 --- a/src/apm_cli/marketplace/ref_resolver.py +++ b/src/apm_cli/marketplace/ref_resolver.py @@ -170,8 +170,16 @@ def __len__(self) -> int: def _parse_ls_remote_output(output: str) -> list[RemoteRef]: - """Parse ``git ls-remote`` stdout into a list of ``RemoteRef``.""" + """Parse ``git ls-remote`` stdout into a list of ``RemoteRef``. + + An annotated or signed tag arrives as two lines: the tag object under + ``refs/tags/`` and the commit it points to under + ``refs/tags/^{}``. A checkout of the tag lands on that commit, so + the tag's ``RemoteRef`` carries the peeled SHA; the ``^{}`` line adds no + ref of its own. Lightweight tags and branches keep their only SHA. + """ refs: list[RemoteRef] = [] + peeled: dict[str, str] = {} for line in output.splitlines(): line = line.strip() if not line: @@ -182,11 +190,11 @@ def _parse_ls_remote_output(output: str) -> list[RemoteRef]: sha, refname = parts[0].strip(), parts[1].strip() if not _SHA_RE.match(sha): continue - # Skip peeled tag objects (^{}) if refname.endswith("^{}"): + peeled[refname[:-3]] = sha continue refs.append(RemoteRef(name=refname, sha=sha)) - return refs + return [RemoteRef(name=ref.name, sha=peeled.get(ref.name, ref.sha)) for ref in refs] class RefResolver: diff --git a/tests/unit/marketplace/test_annotated_tag_source_sha.py b/tests/unit/marketplace/test_annotated_tag_source_sha.py new file mode 100644 index 0000000000..26c8082e23 --- /dev/null +++ b/tests/unit/marketplace/test_annotated_tag_source_sha.py @@ -0,0 +1,188 @@ +"""Marketplace ``source.sha`` for annotated, signed and lightweight tags (#3048). + +Plugin installers check out the selected tag and compare the result with +``source.sha``. A checkout of an annotated or signed tag lands on the commit +the tag points to, so that commit, not the tag object, has to be recorded. +Git creates the tags in a local bare repository; the tests list them with +``git ls-remote`` and pack them into a marketplace. +""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess +import textwrap +from dataclasses import dataclass +from pathlib import Path + +import pytest + +from apm_cli.marketplace.builder import BuildOptions, MarketplaceBuilder +from apm_cli.marketplace.ref_resolver import RemoteRef, _parse_ls_remote_output +from tests.utils.isolated_apm_environment import IsolatedApmEnvironment +from tests.utils.local_git_repository import LocalGitRepository, LocalGitRepositoryFactory + +_SOURCE = "acme/tagged-package" + + +@dataclass(frozen=True) +class _TaggedRepository: + env: dict[str, str] + repository: LocalGitRepository + commits: dict[str, str] # tag name -> commit the tag points to + + +def _git(env: dict[str, str], cwd: Path, *args: str) -> str: + result = subprocess.run( + ("git", *args), + cwd=cwd, + env=env, + capture_output=True, + text=True, + check=True, + timeout=30, + ) + return result.stdout.strip() + + +def _remote_refs(tagged: _TaggedRepository) -> list[RemoteRef]: + output = _git( + tagged.env, + tagged.repository.worktree, + "ls-remote", + "--tags", + "--heads", + tagged.repository.file_url, + ) + return _parse_ls_remote_output(output) + + +@pytest.fixture +def tagged(tmp_path: Path) -> _TaggedRepository: + """v1.0.0 is annotated, v1.1.0 lightweight, each on its own commit.""" + isolated = IsolatedApmEnvironment.create(tmp_path / "fixture", base_env=dict(os.environ)) + env = isolated.subprocess_env() + repositories = LocalGitRepositoryFactory(isolated.repository_root, env=env) + repository = repositories.create("tagged-package") + readme = repository.worktree / "README.md" + readme.write_text("first\n", encoding="utf-8") + first = repositories.commit(repository, message="first") + repositories.tag(repository, "v1.0.0", first, annotated=True) + readme.write_text("second\n", encoding="utf-8") + second = repositories.commit(repository, message="second") + repositories.tag(repository, "v1.1.0", second) + return _TaggedRepository( + env=env, + repository=repository, + commits={"v1.0.0": first.sha, "v1.1.0": second.sha}, + ) + + +def test_tags_resolve_to_the_commit_a_checkout_lands_on(tagged: _TaggedRepository) -> None: + worktree = tagged.repository.worktree + tag_object = _git(tagged.env, worktree, "rev-parse", "v1.0.0") + assert tag_object != tagged.commits["v1.0.0"] # annotated: its object is not the commit + + shas = {ref.name: ref.sha for ref in _remote_refs(tagged)} + + assert shas["refs/tags/v1.0.0"] == tagged.commits["v1.0.0"] + assert shas["refs/tags/v1.1.0"] == tagged.commits["v1.1.0"] + assert shas["refs/heads/main"] == tagged.commits["v1.1.0"] + assert not any(name.endswith("^{}") for name in shas) + + +def test_ssh_signed_tag_resolves_to_its_commit(tagged: _TaggedRepository, tmp_path: Path) -> None: + if shutil.which("ssh-keygen") is None: + pytest.skip("ssh-keygen is needed to sign a tag") + key = tmp_path / "signing-key" + subprocess.run( + ("ssh-keygen", "-q", "-t", "ed25519", "-N", "", "-C", "apm-test", "-f", str(key)), + capture_output=True, + check=True, + timeout=30, + ) + worktree = tagged.repository.worktree + _git( + tagged.env, + worktree, + "-c", + "gpg.format=ssh", + "-c", + f"user.signingkey={key}", + "tag", + "-s", + "v2.0.0", + tagged.commits["v1.1.0"], + "-m", + "Release v2.0.0", + ) + _git(tagged.env, worktree, "push", "origin", "refs/tags/v2.0.0") + assert "BEGIN SSH SIGNATURE" in _git(tagged.env, worktree, "cat-file", "tag", "v2.0.0") + + shas = {ref.name: ref.sha for ref in _remote_refs(tagged)} + + assert shas["refs/tags/v2.0.0"] == tagged.commits["v1.1.0"] + + +class _ResolverFromRefs: + """Stand-in for RefResolver that serves refs already read from git.""" + + def __init__(self, refs: list[RemoteRef]) -> None: + self._refs = refs + + def list_remote_refs( + self, owner_repo: str, *, remote_url: str | None = None + ) -> list[RemoteRef]: + assert owner_repo == _SOURCE + return self._refs + + def close(self) -> None: + pass + + +def test_pack_records_the_commit_a_checkout_of_the_ref_lands_on( + tagged: _TaggedRepository, tmp_path: Path +) -> None: + yml = tmp_path / "marketplace.yml" + yml.write_text( + textwrap.dedent( + f"""\ + name: tag-marketplace + description: Annotated tag sources + version: 1.0.0 + owner: + name: Test + packages: + - name: annotated-ref + source: {_SOURCE} + ref: v1.0.0 + - name: annotated-range + source: {_SOURCE} + version: "~1.0.0" + - name: lightweight-ref + source: {_SOURCE} + ref: v1.1.0 + """ + ), + encoding="utf-8", + ) + builder = MarketplaceBuilder(yml, BuildOptions(offline=True)) + builder._resolver = _ResolverFromRefs(_remote_refs(tagged)) # type: ignore[assignment] + report = builder.build() + + plugins = json.loads(report.output_path.read_text("utf-8"))["plugins"] + sources = {plugin["name"]: plugin["source"] for plugin in plugins} + assert {name: (source["ref"], source["sha"]) for name, source in sources.items()} == { + "annotated-ref": ("v1.0.0", tagged.commits["v1.0.0"]), + "annotated-range": ("v1.0.0", tagged.commits["v1.0.0"]), + "lightweight-ref": ("v1.1.0", tagged.commits["v1.1.0"]), + } + + # What an installer does with the entry: clone, check out the ref, compare. + clone = tmp_path / "clone" + _git(tagged.env, tmp_path, "clone", "--quiet", tagged.repository.file_url, str(clone)) + for source in sources.values(): + _git(tagged.env, clone, "checkout", "--quiet", "--detach", source["ref"]) + assert _git(tagged.env, clone, "rev-parse", "HEAD") == source["sha"] diff --git a/tests/unit/marketplace/test_ref_resolver.py b/tests/unit/marketplace/test_ref_resolver.py index 79d4607534..46e69ed8c7 100644 --- a/tests/unit/marketplace/test_ref_resolver.py +++ b/tests/unit/marketplace/test_ref_resolver.py @@ -48,14 +48,44 @@ def test_multiple_refs(self) -> None: refs = _parse_ls_remote_output(output) assert len(refs) == 3 - def test_peeled_tag_skipped(self) -> None: + def test_annotated_tag_takes_peeled_commit_sha(self) -> None: + # Tag object first, then the commit it points to (#3048). output = ( "aaaa23456789abcdef1234567890abcdef123456\trefs/tags/v1.0.0\n" "bbbb23456789abcdef1234567890abcdef123456\trefs/tags/v1.0.0^{}\n" ) refs = _parse_ls_remote_output(output) - assert len(refs) == 1 - assert refs[0].name == "refs/tags/v1.0.0" + assert refs == [ + RemoteRef(name="refs/tags/v1.0.0", sha="bbbb23456789abcdef1234567890abcdef123456") + ] + + def test_peeled_line_before_tag_line(self) -> None: + output = ( + "bbbb23456789abcdef1234567890abcdef123456\trefs/tags/v1.0.0^{}\n" + "aaaa23456789abcdef1234567890abcdef123456\trefs/tags/v1.0.0\n" + ) + refs = _parse_ls_remote_output(output) + assert refs == [ + RemoteRef(name="refs/tags/v1.0.0", sha="bbbb23456789abcdef1234567890abcdef123456") + ] + + def test_only_annotated_tags_change(self) -> None: + output = ( + "1111111111111111111111111111111111111111\trefs/heads/main\n" + "2222222222222222222222222222222222222222\trefs/tags/v1.0.0\n" + "3333333333333333333333333333333333333333\trefs/tags/v2.0.0\n" + "4444444444444444444444444444444444444444\trefs/tags/v2.0.0^{}\n" + ) + refs = _parse_ls_remote_output(output) + assert refs == [ + RemoteRef(name="refs/heads/main", sha="1" * 40), + RemoteRef(name="refs/tags/v1.0.0", sha="2" * 40), + RemoteRef(name="refs/tags/v2.0.0", sha="4" * 40), + ] + + def test_peeled_line_without_tag_line_adds_nothing(self) -> None: + output = "bbbb23456789abcdef1234567890abcdef123456\trefs/tags/v1.0.0^{}\n" + assert _parse_ls_remote_output(output) == [] def test_invalid_sha_skipped(self) -> None: output = "not-a-sha\trefs/tags/v1.0.0\n" From efbfe6418664e7034959ede090e353eb5ef8c2f7 Mon Sep 17 00:00:00 2001 From: Rodion Kazennov Date: Sat, 3 Oct 2026 12:47:47 +0000 Subject: [PATCH 2/2] refactor(deps): one owner for ls-remote tag commits The marketplace parser had its own reading of peeled `^{}` records next to the one in `deps/git_remote_ops.py`. `tag_commit_shas` in `git_remote_ops.py` now decides which commit a tag record names, with the annotated-tag security note moved along; `parse_ls_remote_output` and the marketplace `_parse_ls_remote_output` both read tags through it, and the marketplace module no longer mentions `^{}`. The owner is registered as `ls-remote-tag-commits` with the guard `transport-platform-ls-remote-tag-commits`: one definition of `tag_commit_shas`, and no `"^{}"` literal in `src/apm_cli` outside the owner. The guard has its mutation case, and `tag_commit_shas` has direct unit tests. --- .../owners/transport-auth-platform.json | 7 ++ .../checks/transport_ls_remote_tags.py | 63 ++++++++++++ .../groups/transport_platform.py | 8 ++ src/apm_cli/deps/git_remote_ops.py | 97 ++++++++++++------- src/apm_cli/marketplace/ref_resolver.py | 26 ++--- .../test_architecture_owner_rule_mutations.py | 8 ++ tests/unit/deps/test_git_remote_ops.py | 29 ++++++ .../unit/scripts/test_architecture_runner.py | 1 + 8 files changed, 191 insertions(+), 48 deletions(-) create mode 100644 scripts/architecture_linter/checks/transport_ls_remote_tags.py diff --git a/.apm/architecture/owners/transport-auth-platform.json b/.apm/architecture/owners/transport-auth-platform.json index 5d6f9e5fae..50623340b4 100644 --- a/.apm/architecture/owners/transport-auth-platform.json +++ b/.apm/architecture/owners/transport-auth-platform.json @@ -150,6 +150,13 @@ "selectors": ["src/apm_cli/deps/revision_pins.py"], "guards": ["transport-platform-revision-pin-outcome"] }, + { + "id": "ls-remote-tag-commits", + "decision": "Which commit a git ls-remote tag record names: an annotated or signed tag resolves to its peeled ^{} commit, for dependency refs and marketplace source pins alike", + "owner": "deps/git_remote_ops.py (tag_commit_shas); consumers: parse_ls_remote_output and marketplace/ref_resolver.py", + "selectors": ["src/apm_cli/deps/git_remote_ops.py", "src/apm_cli/marketplace/ref_resolver.py"], + "guards": ["transport-platform-ls-remote-tag-commits"] + }, { "id": "git-semver-preflight-resolution", "decision": "Git semver preflight eligibility and resolution", diff --git a/scripts/architecture_linter/checks/transport_ls_remote_tags.py b/scripts/architecture_linter/checks/transport_ls_remote_tags.py new file mode 100644 index 0000000000..d66a6a6c8d --- /dev/null +++ b/scripts/architecture_linter/checks/transport_ls_remote_tags.py @@ -0,0 +1,63 @@ +"""Ownership check for how ``git ls-remote`` tag records resolve to commits.""" + +from __future__ import annotations + +import re + +from scripts.architecture_linter.checks.transport_platform_shared import ( + GROUP, + _count_checks, + _forbid_scan, + _src_python, +) +from scripts.architecture_linter.facts import FactsProvider +from scripts.architecture_linter.models import Rule, Violation + +_RULE_ID = "transport-platform-ls-remote-tag-commits" +_OWNER = "src/apm_cli/deps/git_remote_ops.py" +_OWNER_DEFINITION = re.compile(r"^def tag_commit_shas\(") +# A peeled ``refs/tags/^{}`` record is recognised only by the owner. +_PEELED_RECORD = re.compile(r"""["']\^\{\}["']""") + + +def _check_ls_remote_tag_commits(provider: FactsProvider) -> tuple[Violation, ...]: + """Keep the tag-to-commit decision for ls-remote records in one function.""" + inventory = frozenset(provider.inventory) + findings: list[Violation] = [] + findings.extend( + _count_checks( + provider, + inventory, + _RULE_ID, + _OWNER, + (("re", _OWNER_DEFINITION.pattern, 1, "eq"),), + "ls-remote tag-to-commit resolution must stay owned by tag_commit_shas", + ) + ) + findings.extend( + _forbid_scan( + provider, + inventory, + _RULE_ID, + _src_python(provider, exclude={_OWNER}), + _PEELED_RECORD, + "Only deps/git_remote_ops.py may interpret peeled ^{} ls-remote records", + exempt=True, + ) + ) + return tuple(findings) + + +RULES: tuple[Rule, ...] = ( + Rule( + id=_RULE_ID, + group=GROUP, + guard_ids=(_RULE_ID,), + description="ls-remote tag records resolve to commits only through tag_commit_shas.", + check=_check_ls_remote_tag_commits, + ), +) + +COLLECTORS: tuple[object, ...] = () + +__all__ = ["COLLECTORS", "RULES"] diff --git a/scripts/architecture_linter/groups/transport_platform.py b/scripts/architecture_linter/groups/transport_platform.py index 5dadc1dc68..e8a86d3cf8 100644 --- a/scripts/architecture_linter/groups/transport_platform.py +++ b/scripts/architecture_linter/groups/transport_platform.py @@ -12,6 +12,12 @@ from scripts.architecture_linter.checks.transport_gitlab_sparse import ( RULES as _GITLAB_SPARSE_RULES, ) +from scripts.architecture_linter.checks.transport_ls_remote_tags import ( + COLLECTORS as _LS_REMOTE_TAG_COLLECTORS, +) +from scripts.architecture_linter.checks.transport_ls_remote_tags import ( + RULES as _LS_REMOTE_TAG_RULES, +) from scripts.architecture_linter.checks.transport_network_and_runtime import ( COLLECTORS as _NETWORK_COLLECTORS, ) @@ -38,6 +44,7 @@ + _SPARSE_RULES + _GITLAB_SPARSE_RULES + _REVISION_PIN_RULES + + _LS_REMOTE_TAG_RULES + _NETWORK_RULES ) COLLECTORS = ( @@ -45,6 +52,7 @@ + _CACHE_COLLECTORS + _SPARSE_COLLECTORS + _REVISION_PIN_COLLECTORS + + _LS_REMOTE_TAG_COLLECTORS + _NETWORK_COLLECTORS ) diff --git a/src/apm_cli/deps/git_remote_ops.py b/src/apm_cli/deps/git_remote_ops.py index 93bea9ada2..e54020c0bd 100644 --- a/src/apm_cli/deps/git_remote_ops.py +++ b/src/apm_cli/deps/git_remote_ops.py @@ -6,6 +6,7 @@ """ import re +from collections.abc import Iterable from ..models.apm_package import GitReferenceType, RemoteRef @@ -76,18 +77,64 @@ def validate_ls_remote_tag_output(output: str) -> None: raise RemoteRefParseError("Malformed git ls-remote tag output.") -def parse_ls_remote_output(output: str) -> list[RemoteRef]: - """Parse ``git ls-remote --tags --heads`` output into RemoteRef objects. - - Format per line: ``\\t`` +def tag_commit_shas( + records: Iterable[tuple[str, str]], +) -> tuple[dict[str, str], frozenset[str]]: + """Resolve ``git ls-remote`` tag records to the commits they name. - For annotated tags git emits two lines:: + ``records`` are ``(sha, refname)`` pairs in output order. For an annotated + or signed tag git emits two records:: refs/tags/v1.0.0 refs/tags/v1.0.0^{} - We want the commit SHA (from the ``^{}`` line) and skip the - tag-object-only line. + A checkout of the tag lands on the commit, so the ``^{}`` record wins and + adds no refname of its own; a lightweight tag keeps its only SHA. + + Returns ``(commits, annotated)``: ``commits`` maps each ``refs/tags/`` + to its commit SHA in first-seen order, and ``annotated`` holds the + refnames that had a ``^{}`` record. Records outside ``refs/tags/`` are + ignored. This is the one place that interprets ``^{}`` records; the + dependency resolver and the marketplace builder both read tags through it. + """ + commits: dict[str, str] = {} + annotated: set[str] = set() + for sha, refname in records: + if not refname.startswith("refs/tags/"): + continue + if refname.endswith("^{}"): + # Dereferenced commit -- overwrite with the real commit SHA. + # + # SECURITY INVARIANT (load-bearing, do not weaken): only + # ANNOTATED tags emit this peeled ``^{}`` line, so the + # presence of a peeled ref is our sole signal for + # ``annotated=True``. The revision-pin resolver + # (find_latest_annotated_tag) accepts ONLY annotated tags and + # rejects branches and lightweight tags fail-closed, so a + # branch or lightweight tag named like a release can never + # masquerade as a SHA-pin update target. A transport that + # suppressed peeled refs would misclassify a genuine annotated + # tag as lightweight. Revision-pin updates then retain the + # current SHA rather than selecting an unverified target, which + # is the safe direction. Any future edit here that marks a + # non-peeled ref as annotated would break this anti-spoofing + # fence. + refname = refname[:-3] + commits[refname] = sha + annotated.add(refname) + else: + # Only store if we haven't seen the deref line yet. + commits.setdefault(refname, sha) + return commits, frozenset(annotated) + + +def parse_ls_remote_output(output: str) -> list[RemoteRef]: + """Parse ``git ls-remote --tags --heads`` output into RemoteRef objects. + + Format per line: ``\\t`` + + Tags take the commit SHA :func:`tag_commit_shas` resolves for them, so an + annotated tag carries its peeled commit and ``annotated=True``. Args: output: Raw stdout from ``git ls-remote``. @@ -95,8 +142,7 @@ def parse_ls_remote_output(output: str) -> list[RemoteRef]: Returns: Unsorted list of RemoteRef. """ - tags: dict[str, str] = {} # tag name -> commit sha - annotated_tags: set[str] = set() + tag_records: list[tuple[str, str]] = [] branches: list[RemoteRef] = [] for line in output.splitlines(): @@ -109,31 +155,7 @@ def parse_ls_remote_output(output: str) -> list[RemoteRef]: sha, refname = parts[0].strip(), parts[1].strip() if refname.startswith("refs/tags/"): - tag_name = refname[len("refs/tags/") :] - if tag_name.endswith("^{}"): - # Dereferenced commit -- overwrite with the real commit SHA. - # - # SECURITY INVARIANT (load-bearing, do not weaken): only - # ANNOTATED tags emit this peeled ``^{}`` line, so the - # presence of a peeled ref is our sole signal for - # ``annotated=True``. The revision-pin resolver - # (find_latest_annotated_tag) accepts ONLY annotated tags and - # rejects branches and lightweight tags fail-closed, so a - # branch or lightweight tag named like a release can never - # masquerade as a SHA-pin update target. A transport that - # suppressed peeled refs would misclassify a genuine annotated - # tag as lightweight. Revision-pin updates then retain the - # current SHA rather than selecting an unverified target, which - # is the safe direction. Any future edit here that marks a - # non-peeled ref as annotated would break this anti-spoofing - # fence. - tag_name = tag_name[:-3] - tags[tag_name] = sha - annotated_tags.add(tag_name) - else: - # Only store if we haven't seen the deref line yet. - tags.setdefault(tag_name, sha) - + tag_records.append((sha, refname)) elif refname.startswith("refs/heads/"): branch_name = refname[len("refs/heads/") :] branches.append( @@ -144,14 +166,15 @@ def parse_ls_remote_output(output: str) -> list[RemoteRef]: ) ) + commits, annotated_tags = tag_commit_shas(tag_records) tag_refs = [ RemoteRef( - name=name, + name=refname[len("refs/tags/") :], ref_type=GitReferenceType.TAG, commit_sha=sha, - annotated=name in annotated_tags, + annotated=refname in annotated_tags, ) - for name, sha in tags.items() + for refname, sha in commits.items() ] return tag_refs + branches diff --git a/src/apm_cli/marketplace/ref_resolver.py b/src/apm_cli/marketplace/ref_resolver.py index 3cbe6816dd..82d8b05381 100644 --- a/src/apm_cli/marketplace/ref_resolver.py +++ b/src/apm_cli/marketplace/ref_resolver.py @@ -22,6 +22,7 @@ import urllib.parse from dataclasses import dataclass +from ..deps.git_remote_ops import tag_commit_shas from ..utils.git_env import redact_git_diagnostic from ..utils.github_host import ( build_ado_https_clone_url, @@ -172,14 +173,12 @@ def __len__(self) -> int: def _parse_ls_remote_output(output: str) -> list[RemoteRef]: """Parse ``git ls-remote`` stdout into a list of ``RemoteRef``. - An annotated or signed tag arrives as two lines: the tag object under - ``refs/tags/`` and the commit it points to under - ``refs/tags/^{}``. A checkout of the tag lands on that commit, so - the tag's ``RemoteRef`` carries the peeled SHA; the ``^{}`` line adds no - ref of its own. Lightweight tags and branches keep their only SHA. + Tags carry the commit a checkout of them lands on, as resolved by + ``deps.git_remote_ops.tag_commit_shas``: an annotated or signed tag takes + the SHA of its peeled record, which adds no ref of its own. Branches and + other refs keep their only SHA. """ - refs: list[RemoteRef] = [] - peeled: dict[str, str] = {} + records: list[tuple[str, str]] = [] for line in output.splitlines(): line = line.strip() if not line: @@ -190,11 +189,16 @@ def _parse_ls_remote_output(output: str) -> list[RemoteRef]: sha, refname = parts[0].strip(), parts[1].strip() if not _SHA_RE.match(sha): continue - if refname.endswith("^{}"): - peeled[refname[:-3]] = sha - continue + records.append((sha, refname)) + commits, _annotated = tag_commit_shas(records) + refs: list[RemoteRef] = [] + for sha, refname in records: + if refname.startswith("refs/tags/"): + if refname not in commits: + continue # the peeled record of an annotated tag + sha = commits[refname] refs.append(RemoteRef(name=refname, sha=sha)) - return [RemoteRef(name=ref.name, sha=peeled.get(ref.name, ref.sha)) for ref in refs] + return refs class RefResolver: diff --git a/tests/integration/test_architecture_owner_rule_mutations.py b/tests/integration/test_architecture_owner_rule_mutations.py index 01007aa1f5..69af085006 100644 --- a/tests/integration/test_architecture_owner_rule_mutations.py +++ b/tests/integration/test_architecture_owner_rule_mutations.py @@ -1053,6 +1053,14 @@ class MutationCase: new="def parse_host_qualified_reference_disabled(", intent="Host-qualified reference parsing loses its canonical coordinate owner.", ), + MutationCase( + guard_id="transport-platform-ls-remote-tag-commits", + rule_id="transport-platform-ls-remote-tag-commits", + path="src/apm_cli/marketplace/ref_resolver.py", + old=" if refname not in commits:", + new=' if refname.endswith("^{}"):', + intent="The marketplace parser interprets peeled ^{} records outside tag_commit_shas.", + ), MutationCase( guard_id="transport-platform-marketplace-package-remote", rule_id="transport-platform-marketplace-package-remote", diff --git a/tests/unit/deps/test_git_remote_ops.py b/tests/unit/deps/test_git_remote_ops.py index eb2baf1e06..d80d360652 100644 --- a/tests/unit/deps/test_git_remote_ops.py +++ b/tests/unit/deps/test_git_remote_ops.py @@ -17,6 +17,7 @@ parse_ls_remote_output, semver_sort_key, sort_remote_refs, + tag_commit_shas, validate_ls_remote_tag_output, ) from apm_cli.models.apm_package import GitReferenceType, RemoteRef @@ -125,6 +126,34 @@ def test_whitespace_is_stripped_from_sha_and_refname(self) -> None: assert refs[0].name == "main" +class TestTagCommitShas: + """The one place that turns ls-remote tag records into commits (#3048).""" + + def test_annotated_tag_takes_its_peeled_commit(self) -> None: + commits, annotated = tag_commit_shas( + [("a" * 40, "refs/tags/v1.0.0"), ("b" * 40, "refs/tags/v1.0.0^{}")] + ) + assert commits == {"refs/tags/v1.0.0": "b" * 40} + assert annotated == frozenset({"refs/tags/v1.0.0"}) + + def test_peeled_record_first_still_wins(self) -> None: + commits, annotated = tag_commit_shas( + [("b" * 40, "refs/tags/v1.0.0^{}"), ("a" * 40, "refs/tags/v1.0.0")] + ) + assert commits == {"refs/tags/v1.0.0": "b" * 40} + assert annotated == frozenset({"refs/tags/v1.0.0"}) + + def test_lightweight_tag_keeps_its_sha_and_is_not_annotated(self) -> None: + commits, annotated = tag_commit_shas([("c" * 40, "refs/tags/v2.0.0")]) + assert commits == {"refs/tags/v2.0.0": "c" * 40} + assert annotated == frozenset() + + def test_records_outside_refs_tags_are_ignored(self) -> None: + commits, annotated = tag_commit_shas([("d" * 40, "refs/heads/main"), ("e" * 40, "HEAD")]) + assert commits == {} + assert annotated == frozenset() + + class TestValidateLsRemoteTagOutput: def test_empty_output_is_a_valid_no_tag_result(self) -> None: validate_ls_remote_tag_output("") diff --git a/tests/unit/scripts/test_architecture_runner.py b/tests/unit/scripts/test_architecture_runner.py index be3c76df6a..313e00e82b 100644 --- a/tests/unit/scripts/test_architecture_runner.py +++ b/tests/unit/scripts/test_architecture_runner.py @@ -729,6 +729,7 @@ def exiting_import( transport-platform-gitlab-sparse-plan transport-platform-host-credential-resolution transport-platform-host-reference-coordinates +transport-platform-ls-remote-tag-commits transport-platform-marketplace-package-remote transport-platform-network-host-parsing transport-platform-ref-freshness