fix(marketplace): record the commit an annotated tag points to - #3161
Rodion Kazennov (nefayran) wants to merge 2 commits into
Conversation
`git ls-remote` lists an annotated or signed tag twice: the tag object
under `refs/tags/<name>` and its commit under `refs/tags/<name>^{}`.
`_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 microsoft#3048
96c2f84 to
8debe84
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Peeled-tag interpretation duplicates an existing durable decision instead of routing through one canonical owner.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Fixes marketplace packing so annotated and signed tags record the checked-out commit SHA rather than the tag-object SHA.
Changes:
- Handles peeled
^{}tag references. - Adds parser and local-repository regression tests.
- Documents the fix in the changelog.
| File | Description |
|---|---|
src/apm_cli/marketplace/ref_resolver.py |
Applies peeled commit SHAs to tags. |
tests/unit/marketplace/test_ref_resolver.py |
Tests parser edge cases. |
tests/unit/marketplace/test_annotated_tag_source_sha.py |
Tests real tags and packed output. |
CHANGELOG.md |
Records the fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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] |
There was a problem hiding this comment.
Done in f8859f2. tag_commit_shas in deps/git_remote_ops.py now decides which commit a tag record names, and the annotated-tag security note moved with it. parse_ls_remote_output and the marketplace parser both read tags through it, so marketplace/ref_resolver.py no longer handles ^{} itself.
The owner is registered as ls-remote-tag-commits in transport-auth-platform.json, guarded by transport-platform-ls-remote-tag-commits (one definition of tag_commit_shas, no "^{}" literal in src/apm_cli outside the owner), with its mutation case. On 20,000 random ls-remote outputs, parse_ls_remote_output returns the same refs before and after the change.
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.
Description
git ls-remotelists an annotated or signed tag twice: the tag object underrefs/tags/<name>and the commit it points to underrefs/tags/<name>^{}._parse_ls_remote_outputinsrc/apm_cli/marketplace/ref_resolver.pydropped the^{}line, soapm packwrote the tag object assource.sha. A checkout of the tag lands on the commit, and installers that compare the two reject the pin, as in thecavemancase in the issue.Both parsers now read tags through
tag_commit_shasinsrc/apm_cli/deps/git_remote_ops.py, which gives a tag the SHA from its^{}line; that line adds no ref of its own. It is registered as the ownerls-remote-tag-commits, with the static guardtransport-platform-ls-remote-tag-commits. Lightweight tags and branches keep their only SHA. Aref:that is a full SHA never reaches this parser, so exact pins are unchanged.resolve_ref_shauses the marketplace parser and is only called withHEAD, which has no^{}line.Issue and approved scope
Issue: #3048
Human scope-approval comment: #3048 (comment)
This PR completes the issue. A
marketplace.jsonpacked before the fix keeps the tag object SHA untilapm packruns again.Type of change
Testing
tests/unit/marketplace/test_ref_resolver.py:test_peeled_tag_skippedis nowtest_annotated_tag_takes_peeled_commit_shaand expects the commit SHA. New cases cover a^{}line that comes before its tag line, a branch with a lightweight and an annotated tag, and a^{}line with no tag line.tests/unit/marketplace/test_annotated_tag_source_sha.pycreates the tags with git in a local bare repository (LocalGitRepositoryFactory): annotated, lightweight, and SSH-signed (skipped whenssh-keygenis missing). It packs a marketplace withref: v1.0.0,version: "~1.0.0"andref: v1.1.0entries, then clones the repository, checks out eachsource.refand comparesHEADwithsource.sha, the same check an installer makes. With the old parser, six of the new tests fail.tests/unit/deps/test_git_remote_ops.pycoverstag_commit_shasdirectly, and the guard has its case intests/integration/test_architecture_owner_rule_mutations.py. On 20,000 randomls-remoteoutputsparse_ls_remote_outputreturns the same refs before and after the change.Locally
uv run pytest tests/unit tests/test_console.pypasses (23585 passed, 42 skipped, 21 xfailed). Ruff check and format, the pylint R0801 duplication check,scripts/lint-auth-signals.shandscripts/lint-architecture-boundaries.share clean.Spec conformance (OpenAPM v0.1)
docs/src/content/docs/specs/openapm-v0.1.mdupdated(new/changed
<a id="req-XXX"></a>anchor + prose + Appendix Crow).
docs/src/content/docs/specs/manifests/openapm-v0.1.requirements.ymlupdated.
@pytest.mark.req("req-XXX")test undertests/spec_conformance/added or extended.CONFORMANCE.{md,json}regenerated viauv run --extra dev python -m tests.spec_conformance.gen_statementand committed.