Skip to content

Only rewrite action refs to tags at the resolved commit - #117

Merged
nodeselector merged 8 commits into
mainfrom
nodeselector-fix-cross-family-tags
Sep 1, 2026
Merged

Only rewrite action refs to tags at the resolved commit#117
nodeselector merged 8 commits into
mainfrom
nodeselector-fix-cross-family-tags

Conversation

@nodeselector

@nodeselectornodeselector commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

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.4 while retaining the newer v21 commit.
  • dawidd6/action-send-mail@v18, which could become the unrelated ancestor @v3.12.0.

Fixes#109
Fixes#113

How

  • Remove ancestor-tag fallback from automatic narrowing and verified-entry updates.
  • Let existing reverse lookup normalize bare SHAs to exact tags, including major-only tags such as v21, or to a containing branch.
  • Keep symbolic v4 and v4.2 narrowing limited to exact same-commit full tags in the requested family.

Testing

  • go test ./...
  • Command regression updates the issue Strange behaviour when running the command #109 SHA to v21 while a valid ancestor alternative exists, checks lock ref/commit coherence, then runs --verify-local.
  • Command regression changes an existing v3.12.0 lock entry to workflow ref v18 at the same commit, checks rekey and stale-entry garbage collection, then runs --verify-local.

@nodeselector
nodeselector requested a review from a team as a code ownerAugust 31, 2026 18:58
CopilotAI balanced review requested due to automatic review settings August 31, 2026 18:58

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Medium severity · 1 Low severity

New issues introduced by this change (2)
SeverityFinding
Low severityinternal/​tag/​tags_test.go — These mocks only register compare responses for candidates expected to be accepted. Because…
Medium severityinternal/​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.
FileDescription
internal/​tag/​tagging.goAdds family-aware ancestor filtering.
internal/​pin/​plan.goPasses original refs into ancestor lookup.
internal/​tag/​tags_test.goTests family filtering behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment threadinternal/tag/tags_test.go Outdated
Comment threadinternal/tag/tagging.go Outdated
@nodeselectornodeselector changed the title Keep ancestor tag fallback in the requested version familyOnly rewrite action refs to tags at the resolved commitAug 31, 2026
@nodeselector
nodeselector requested a balanced review from CopilotAugust 31, 2026 21:50

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Low severity

New issues introduced by this change (2)
SeverityFinding
Low severitycmd/​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…
Low severityinternal/​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)
SeverityFinding
Medium severityinternal/​tag/​tagging.go — The family restriction is applied only in the ancestor fallback, but every narrowing path first… View resolved comment
Low severityinternal/​tag/​tags_test.go — These mocks only register compare responses for candidates expected to be accepted. Because… View resolved comment

Comment threadcmd/gh-actions-lock/command_test.go
Comment threadinternal/pin/plan_test.go

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 High severity

New issues introduced by this change (1)
SeverityFinding
High severityinternal/​pin/​plan.go — Delegating bare SHAs to ReverseLookup breaks normalization when the workflow also uses the same…
Issues resolved since last review (2)
SeverityFinding
Low severityinternal/​pin/​plan_test.go — This helper-level test does not cover issue #113's acceptance path: an existing vOld lock entry,… View resolved comment
Low severitycmd/​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 in ReverseLookup.
 continue

Comment threadinternal/pin/plan.go

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
SeverityFinding
High severityinternal/​pin/​plan.go — Delegating bare SHAs to ReverseLookup breaks normalization when the workflow also uses the same… View resolved comment

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The implementation consistently enforces ref/commit coherence and includes focused regression coverage.

Review tier: Balanced
Findings: None

@nodeselector
nodeselector merged commit 634cc11 into mainSep 1, 2026
11 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Updater rewrites a changed ref to another tag on the same commit Strange behaviour when running the command

3 participants

@nodeselector@heavymachinery