Only rewrite action refs to tags at the resolved commit - #117
Only rewrite action refs to tags at the resolved commit#117nodeselector wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Exact-SHA tag lookup still bypasses the family restriction and can reproduce issue #113.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
internal/tag/tags_test.go — These mocks only register compare responses for candidates expected to be accepted. Because… |
 |
internal/tag/tagging.go — The family restriction is applied only in the ancestor fallback, but every narrowing path first… |
What changed in this PR
Restricts ancestor-tag fallback to the requested semantic-version family.
Changes:
- Filters ancestor tags by major or minor family.
- Preserves unrestricted fallback for bare SHAs.
- Adds family-selection unit tests.
| File | Description |
|---|---|
internal/tag/tagging.go |
Adds family-aware ancestor filtering. |
internal/pin/plan.go |
Passes original refs into ancestor lookup. |
internal/tag/tags_test.go |
Tests family filtering behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
nodeselector
changed the title
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Regression coverage does not fully reproduce the ancestor relationship or issue #113’s required update flow.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
cmd/gh-actions-lock/command_test.go — The fixture labels v3.1.4 with ancestorSHA but never makes the Compare API report that it is an… |
|
internal/pin/plan_test.go — This helper-level test does not cover issue #113's acceptance path: an existing vOld lock entry,… |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
internal/tag/tagging.go — The family restriction is applied only in the ancestor fallback, but every narrowing path first… View resolved comment |
|
internal/tag/tags_test.go — These mocks only register compare responses for candidates expected to be accepted. Because… View resolved comment |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
NWO-wide narrowing state can prevent a bare SHA from being normalized when another ref for the same repository is narrowed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
internal/pin/plan.go — Delegating bare SHAs to ReverseLookup breaks normalization when the workflow also uses the same… |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
internal/pin/plan_test.go — This helper-level test does not cover issue #113's acceptance path: an existing vOld lock entry,… View resolved comment |
|
cmd/gh-actions-lock/command_test.go — The fixture labels v3.1.4 with ancestorSHA but never makes the Compare API report that it is an… View resolved comment |
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/pin/plan.go:350
- The preceding comment is now stale: this branch no longer finds a tag; it only preserves a SHA under
--no-narrow, while exact-tag or branch selection happens later inReverseLookup.
continue
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation matches the stated requirements and includes focused unit and command-level regression coverage.
Review tier: Balanced
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
internal/pin/plan.go — Delegating bare SHAs to ReverseLookup breaks normalization when the workflow also uses the same… View resolved comment |
What
Only rewrite action refs to tags that point exactly at the resolved commit.
Why
Ancestor tags do not contain descendant commits. Rewriting a ref to an ancestor tag while retaining the newer resolved commit creates an incoherent lock entry that immediately fails verification.
This affected both:
dawidd6/action-download-artifact@<SHA>, which became@v3.1.4while retaining the newerv21commit.dawidd6/action-send-mail@v18, which could become the unrelated ancestor@v3.12.0.Fixes #109
Fixes #113
How
v21, or to a containing branch.v4andv4.2narrowing limited to exact same-commit full tags in the requested family.Testing
go test ./...v21while a valid ancestor alternative exists, checks lock ref/commit coherence, then runs--verify-local.v3.12.0lock entry to workflow refv18at the same commit, checks rekey and stale-entry garbage collection, then runs--verify-local.