diff --git a/internal/pin/normalization_test.go b/internal/pin/normalization_test.go index d5fe85b4..8558ec1d 100644 --- a/internal/pin/normalization_test.go +++ b/internal/pin/normalization_test.go @@ -74,6 +74,10 @@ func TestPlanNormalizationPrunesStaleTargetsInOneRun(t *testing.T) { tags = httpmock.TagListResponse(tt.tag, headSHA) } reg.Register(httpmock.REST("GET", `repos/owner/action/tags`), httpmock.JSONResponse(tags)) + } + // Branch normalization requires the branch head to equal the pin, + // even when the lockfile already records the containing branch. + if !tt.recorded || tt.tag == "" { reg.Register(httpmock.REST("GET", `repos/owner/action/git/ref/heads/main`), httpmock.JSONResponse(map[string]any{ "ref": "refs/heads/main", "object": map[string]any{"sha": headSHA, "type": "commit"}, diff --git a/internal/pin/plan.go b/internal/pin/plan.go index f7091b46..6ce594eb 100644 --- a/internal/pin/plan.go +++ b/internal/pin/plan.go @@ -638,8 +638,12 @@ func narrowVerifiedEntries(ctx context.Context, entries []Entry, opts PlanOption } if parserlock.IsFullSha(e.Ref) { newRef := e.Tag - if newRef == "" { - newRef = e.OnBranch + if newRef == "" && e.OnBranch != "" && opts.Resolver != nil { + // A containing branch cannot identify an older pinned commit. + owner, repo := splitNWO(e.NWO) + if head, ok := opts.Resolver.GetBranchHead(ctx, owner, repo, e.OnBranch); ok && strings.EqualFold(head.SHA, e.SHA) { + newRef = e.OnBranch + } } if newRef == "" { continue diff --git a/internal/pin/plan_test.go b/internal/pin/plan_test.go index d18e7f9b..183ccdf7 100644 --- a/internal/pin/plan_test.go +++ b/internal/pin/plan_test.go @@ -418,7 +418,13 @@ func TestNarrowVerifiedEntries_StickyPrecision(t *testing.T) { }) t.Run("SHA metadata repairs to branch", func(t *testing.T) { - tagger, _ := newTagger(t) + tagger, reg := newTagger(t) + reg.Register(httpmock.REST("GET", `repos/actions/checkout/git/ref/heads/main`), + httpmock.JSONResponse(map[string]any{ + "ref": "refs/heads/main", "object": map[string]any{"sha": sha, "type": "commit"}, + })) + resolver, err := resolve.New("github.com", pinpool.New(2, nil), resolve.WithTransport(reg)) + require.NoError(t, err) report := fastPathReport(sha) report.Inventory[0].Dep.Branch = "main" report.ActionRefs = []parserlock.ActionRef{{ @@ -427,7 +433,7 @@ func TestNarrowVerifiedEntries_StickyPrecision(t *testing.T) { Ref: sha, }} - result, err := planWorkflow(context.Background(), report, PlanOptions{Tagger: tagger}, func(string) {}) + result, err := planWorkflow(context.Background(), report, PlanOptions{Tagger: tagger, Resolver: resolver}, func(string) {}) require.NoError(t, err) require.Len(t, result.entries, 1) @@ -506,6 +512,74 @@ func TestNarrowVerifiedEntries_StickyPrecision(t *testing.T) { }) } +func TestPlanPreservesDifferentHistoricalSHAsOnSameBranch(t *testing.T) { + const headSHA = "cccccccccccccccccccccccccccccccccccccccc" + shas := []string{ + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + } + for _, tt := range []struct { + name string + locked int + }{ + {name: "fresh lockfile"}, + {name: "existing lockfile", locked: 2}, + {name: "new workflow with existing lockfile", locked: 1}, + } { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register(httpmock.REST("GET", `repos/owner/action/git/ref/heads/main`), + httpmock.JSONResponse(map[string]any{ + "ref": "refs/heads/main", "object": map[string]any{"sha": headSHA, "type": "commit"}, + })) + if tt.locked < len(shas) { + reg.Register(httpmock.REST("GET", `repos/owner/action$`), + httpmock.JSONResponse(map[string]any{"default_branch": "main"})) + reg.Register(httpmock.REST("GET", `repos/owner/action/tags`), + httpmock.JSONResponse(httpmock.TagListResponse())) + for _, sha := range shas[tt.locked:] { + reg.Register(httpmock.REST("GET", `repos/owner/action/compare/`+sha+`\.\.\.`+headSHA), + httpmock.JSONResponse(httpmock.CompareAncestorResponse(sha))) + } + } + pool := pinpool.New(2, nil) + resolver, err := resolve.New("github.com", pool, resolve.WithTransport(reg)) + require.NoError(t, err) + report := &checks.Report{} + for i, sha := range shas { + d := dep.Dependency{Hostname: "github.com", NWO: "owner/action", Ref: sha, SHA: sha, Branch: "main"} + resolver.SeedFromLockfile([]dep.Dependency{d}) + wr := checks.WorkflowReport{ + Path: ".github/workflows/" + sha + ".yml", + ActionRefs: []parserlock.ActionRef{{Owner: "owner", Repo: "action", Ref: sha}}, + } + if i < tt.locked { + wr.Inventory = []checks.InventoryEntry{{Dep: d, File: wr.Path, Direct: true}} + } else { + wr.Findings = []checks.Finding{{Category: checks.NotPinned, Severity: checks.SeverityError}} + } + report.Workflows = append(report.Workflows, wr) + } + + record, err := Plan(context.Background(), report, PlanOptions{ + Resolver: resolver, Tagger: tag.NewListerForTest(t, reg), Pool: pool, + }) + require.NoError(t, err) + require.Len(t, record.Entries, len(shas)) + for i, entry := range record.Entries { + assert.Equal(t, shas[i], entry.Ref) + assert.Equal(t, shas[i], entry.SHA) + assert.Equal(t, "main", entry.OnBranch) + } + require.Len(t, record.Workflows, len(shas)) + for _, wp := range record.Workflows { + assert.Empty(t, wp.Rewrites) + } + }) + } +} + func TestPlanNormalizationRespectsLockedTarget(t *testing.T) { const ( oldSHA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" diff --git a/internal/resolve/reverse_lookup.go b/internal/resolve/reverse_lookup.go index 0541aeb5..2913b7a1 100644 --- a/internal/resolve/reverse_lookup.go +++ b/internal/resolve/reverse_lookup.go @@ -252,8 +252,9 @@ type LookupIssue struct { // ReverseLookup leaves symbolic refs unchanged and reverse-lookups bare SHA // refs via DiscoverContaining. For bare SHAs it populates dep.Tag and dep.Branch -// and computes the canonical @ref. The ref priority is: -// tag (semver-ish release) > protected branch > default branch > any branch. +// and computes the canonical @ref. Exact tags take precedence over branch +// heads. A containing branch remains metadata when its head differs from SHA, +// so historical pins on the same branch keep distinct refs. // When the canonical ref differs from dep.Ref the change is recorded in // the returned rewrites map and dep.Ref is updated in place. // @@ -300,7 +301,9 @@ func (r *Resolver) ReverseLookup(ctx context.Context, deps []dep.Dependency) (ma newRef = branch } if newRef == "" { - newRef = branch + if head, ok := r.GetBranchHead(ctx, owner, repo, branch); ok && strings.EqualFold(head.SHA, d.SHA) { + newRef = branch + } } if newRef == "" || newRef == d.Ref { continue diff --git a/internal/resolve/reverse_lookup_test.go b/internal/resolve/reverse_lookup_test.go index f2b61671..ba7ee03e 100644 --- a/internal/resolve/reverse_lookup_test.go +++ b/internal/resolve/reverse_lookup_test.go @@ -8,8 +8,62 @@ import ( "github.com/github/gh-actions-lock/internal/ghapi" "github.com/github/gh-actions-lock/internal/ghapi/httpmock" "github.com/github/gh-actions-lock/internal/pinpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +func TestReverseLookup_ContainingBranchDoesNotIdentifyHistoricalCommit(t *testing.T) { + const ( + oldSHA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + headSHA = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + ) + for _, tt := range []struct { + name string + sha string + tag string + wantRef string + }{ + {name: "historical SHA stays distinct", sha: oldSHA, wantRef: oldSHA}, + {name: "branch head can use branch name", sha: headSHA, wantRef: "main"}, + {name: "exact tag identifies historical commit", sha: oldSHA, tag: "v1.0.0", wantRef: "v1.0.0"}, + } { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register(httpmock.REST("GET", `repos/owner/action$`), + httpmock.JSONResponse(map[string]any{"default_branch": "main"})) + reg.Register(httpmock.REST("GET", `repos/owner/action/git/ref/heads/main`), + httpmock.JSONResponse(map[string]any{ + "ref": "refs/heads/main", "object": map[string]any{"sha": headSHA, "type": "commit"}, + })) + if tt.sha != headSHA { + reg.Register(httpmock.REST("GET", `repos/owner/action/compare/`+oldSHA+`\.\.\.`+headSHA), + httpmock.JSONResponse(httpmock.CompareAncestorResponse(oldSHA))) + } + tags := httpmock.TagListResponse() + if tt.tag != "" { + tags = httpmock.TagListResponse(tt.tag, tt.sha) + } + reg.Register(httpmock.REST("GET", `repos/owner/action/tags`), httpmock.JSONResponse(tags)) + resolver, err := New("github.com", pinpool.New(2, nil), WithTransport(reg)) + require.NoError(t, err) + deps := []dep.Dependency{{NWO: "owner/action", Ref: tt.sha, SHA: tt.sha}} + + rewrites, issues, err := resolver.ReverseLookup(context.Background(), deps) + require.NoError(t, err) + assert.Empty(t, issues) + assert.Equal(t, tt.wantRef, deps[0].Ref) + assert.Equal(t, "main", deps[0].Branch) + assert.Equal(t, tt.tag, deps[0].Tag) + if tt.wantRef == tt.sha { + assert.Empty(t, rewrites) + } else { + assert.Equal(t, map[string]string{"owner/action@" + tt.sha: "owner/action@" + tt.wantRef}, rewrites) + } + }) + } +} + func TestFindContainingBranch_ExactHeadMatch(t *testing.T) { reg := &httpmock.Registry{} r, err := New("test.com", pinpool.New(1, nil), WithTransport(reg))