From c7a0fcffd7ba339a2d0b7a569cab73a0e8f06a39 Mon Sep 17 00:00:00 2001 From: Alexander Bayandin <864213+bayandin@users.noreply.github.com> Date: Wed, 14 Oct 2026 12:10:27 +0100 Subject: [PATCH 1/2] Protect existing locked targets during SHA normalization Keep SHA refs when normalization would reuse a symbolic ref pinned to another commit. Exclude targets orphaned by stale workflow refs while retaining pins required by other workflows and composite actions. --- internal/pin/plan.go | 111 +++++++++++++++++++++++-------------------- 1 file changed, 60 insertions(+), 51 deletions(-) diff --git a/internal/pin/plan.go b/internal/pin/plan.go index ebcd23ca..f7091b46 100644 --- a/internal/pin/plan.go +++ b/internal/pin/plan.go @@ -50,6 +50,10 @@ type PlanOptions struct { // creating duplicate dep entries at different ref granularities. prevImpreciseNWO map[string]bool + // retainedLockTargets excludes pins orphaned by this run's stale-ref + // pruning, while retaining pins needed by unscanned or skipped workflows. + retainedLockTargets map[string]bool + // OnProgress is called at each phase boundary with a human-readable // label (e.g. "Resolving actions/checkout"). Nil means no progress. OnProgress func(phase string) @@ -79,6 +83,7 @@ func Plan(ctx context.Context, report *checks.Report, opts PlanOptions) (*Record if opts.prevImpreciseNWO == nil && opts.Store != nil { opts.prevImpreciseNWO = impreciseDirectNWOs(opts.Store) } + opts.retainedLockTargets = retainedLockTargets(report, opts) results := make([]planResult, len(report.Workflows)) var planErr error poolErr := pinpool.RunTyped(opts.Pool, ctx, "Planning pins", @@ -417,64 +422,29 @@ func narrowDirectDeps(ctx context.Context, opts PlanOptions, deps []dep.Dependen } } -// reverseLookupRewrites canonicalizes dep refs via ReverseLookup (SHA -> tag/ -// branch), restoring refs that narrowing or a transitive dep already fixed. -// Returns the rewrites map, indices of unresolvable deps, and any hard error. +// reverseLookupRewrites canonicalizes refs via ReverseLookup, preserving +// narrowed refs, transitive declarations, and existing locked targets. +// It returns rewrites, indices of unresolvable deps, and any hard error. func reverseLookupRewrites(ctx context.Context, opts PlanOptions, wr checks.WorkflowReport, deps []dep.Dependency, directTracker lockfile.DirectTracker, preservedDeps map[int]bool) (map[string]string, []resolve.LookupIssue, error) { - // Save narrowed refs before ReverseLookup - it may overwrite dep.Ref - // with a branch name, but we want to keep the semver tag narrowing chose. - preservedRefs := make(map[int]string) - preservedKeys := make(map[string]bool) + originalRefs := make([]string, len(deps)) for i := range deps { - if preservedDeps[i] { - preservedRefs[i] = deps[i].Ref - preservedKeys[deps[i].Key()] = true - } + originalRefs[i] = deps[i].Ref } - // Snapshot transitive deps' declared refs before ReverseLookup mutates - // them. We restore symbolic refs (tags/branches) afterward but let - // ReverseLookup's discovered ref stick when the original is a bare SHA. - transitiveOrigRefs := make(map[int]string) - for i := range deps { - if !directTracker.IsDirect(i) { - transitiveOrigRefs[i] = deps[i].Ref - } - } - - // ReverseLookup: SHA -> containing tag/branch. Rewrites refs to canonical form. normRewrites, lookupIssues, err := opts.Resolver.ReverseLookup(ctx, deps) if err != nil { return nil, nil, fmt.Errorf("reverse lookup: %w", err) } - // Restore narrowed refs that ReverseLookup may have overwritten. - for i, ref := range preservedRefs { - deps[i].Ref = ref - } - // Restore transitive deps' declared refs — we don't own the composite's - // action.yml so the lockfile key must match what it declares. The - // discovered tag/branch is preserved in dep.Tag/dep.Branch and used by - // state.Set for the lockfile ref: field. - transitiveRewriteKeys := make(map[string]bool) + // Composite refs must match their action.yml. A direct rewrite must not + // reuse a ref locked to a different commit. Keep the discovered metadata. for i := range deps { - if directTracker.IsDirect(i) { - continue + if preservedDeps[i] || !directTracker.IsDirect(i) || + hasConflictingLockTarget(opts, deps[i].NWO, deps[i].Ref, deps[i].SHA) { + deps[i].Ref = originalRefs[i] + delete(normRewrites, deps[i].Key()) } - origRef := transitiveOrigRefs[i] - deps[i].Ref = origRef - transitiveRewriteKeys[deps[i].NWO+"@"+origRef] = true } - rewrites := make(map[string]string) - for k, v := range normRewrites { - if transitiveRewriteKeys[k] { - continue - } - if preservedKeys[k] { - continue - } - rewrites[k] = v - } - return rewrites, lookupIssues, nil + return normRewrites, lookupIssues, nil } // buildPinnedEntries emits an entry for every resolved dep, marking it Verified @@ -674,7 +644,7 @@ func narrowVerifiedEntries(ctx context.Context, entries []Entry, opts PlanOption if newRef == "" { continue } - if parserlock.IsFullSha(newRef) || hasConflictingLockTarget(opts.Store, e.NWO, newRef, e.SHA) { + if parserlock.IsFullSha(newRef) || hasConflictingLockTarget(opts, e.NWO, newRef, e.SHA) { continue } oldRef := e.Ref @@ -726,11 +696,50 @@ func narrowVerifiedEntries(ctx context.Context, entries []Entry, opts PlanOption return rewrites } -func hasConflictingLockTarget(store *lockfile.State, nwo, ref, sha string) bool { - if store == nil { +func retainedLockTargets(report *checks.Report, opts PlanOptions) map[string]bool { + if opts.Store == nil { + return nil + } + staleByWorkflow := make(map[string]map[string]bool) + for _, wr := range report.Workflows { + if wr.SkipCommit || wr.BlockingResolverError || wr.CountByCategory(checks.InvalidSelfRepositoryRef) > 0 { + continue + } + if opts.Relock && !opts.AcceptMoved && wr.CountByCategory(checks.UnreachablePin) > 0 && wr.CountByCategory(checks.RefMoved) > 0 { + continue + } + stale := make(map[string]bool) + for _, f := range wr.Findings { + if f.Category == checks.Stale && f.Dependency != nil { + stale[strings.ToLower(f.Dependency.NWO)+"@"+f.Dependency.Ref] = true + } + } + staleByWorkflow[workflowfile.KeyFromPath(wr.Path)] = stale + } + + // A stale direct pin can still be required by another workflow or by a + // surviving composite. Only ignore targets absent from every closure. + retained := make(map[string]bool) + for _, wfKey := range opts.Store.WorkflowKeys() { + live := func(p parserlock.Pin) bool { + return !staleByWorkflow[wfKey][p.String()] + } + for _, locked := range opts.Store.Closure(wfKey, live) { + retained[locked.Pin.String()] = true + } + } + return retained +} + +func hasConflictingLockTarget(opts PlanOptions, nwo, ref, sha string) bool { + if opts.Store == nil { + return false + } + key := strings.ToLower(nwo) + "@" + ref + if opts.retainedLockTargets != nil && !opts.retainedLockTargets[key] { return false } - action, ok := store.File().Dependencies[nwo+"@"+ref] + action, ok := opts.Store.File().Dependencies[key] if !ok { return false } From 750e8e14195452889f7022471d8ddbeb84ac39ee Mon Sep 17 00:00:00 2001 From: Alexander Bayandin <864213+bayandin@users.noreply.github.com> Date: Wed, 14 Oct 2026 13:18:10 +0100 Subject: [PATCH 2/2] Test locked targets during SHA normalization Cover matching and occupied targets, repository-name casing, unchanged repeat runs, partial scans, and skipped workflows. --- internal/pin/normalization_test.go | 211 +++++++++++++++++++++++++++++ internal/pin/plan_test.go | 91 +++++++++++++ 2 files changed, 302 insertions(+) create mode 100644 internal/pin/normalization_test.go diff --git a/internal/pin/normalization_test.go b/internal/pin/normalization_test.go new file mode 100644 index 00000000..d5fe85b4 --- /dev/null +++ b/internal/pin/normalization_test.go @@ -0,0 +1,211 @@ +package pin + +import ( + "context" + "os" + "path/filepath" + "testing" + + parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" + "github.com/github/gh-actions-lock/internal/dep" + "github.com/github/gh-actions-lock/internal/ghapi/httpmock" + "github.com/github/gh-actions-lock/internal/lockfile" + "github.com/github/gh-actions-lock/internal/pinpool" + "github.com/github/gh-actions-lock/internal/pipeline" + "github.com/github/gh-actions-lock/internal/pipeline/checks" + "github.com/github/gh-actions-lock/internal/resolve" + "github.com/github/gh-actions-lock/internal/tag" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPlanNormalizationPrunesStaleTargetsInOneRun(t *testing.T) { + const ( + oldSHA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + headSHA = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + ) + for _, tt := range []struct { + name string + tag string + recorded bool + workflows []string + }{ + {name: "fresh branch head"}, + {name: "recorded branch head", recorded: true}, + {name: "fresh exact tag", tag: "v1.0.0"}, + {name: "recorded exact tag", tag: "v1.0.0", recorded: true}, + { + name: "stale in every workflow", + workflows: []string{".github/workflows/ci.yml", ".github/workflows/release.yml"}, + }, + } { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + dir := t.TempDir() + t.Chdir(dir) + paths := tt.workflows + if len(paths) == 0 { + paths = []string{".github/workflows/ci.yml"} + } + store, err := lockfile.LoadState(dir, fakeMeta{}) + require.NoError(t, err) + wantRef := "main" + if tt.tag != "" { + wantRef = tt.tag + } + for _, path := range paths { + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o755)) + require.NoError(t, os.WriteFile(path, []byte("name: CI\non: push\njobs:\n build:\n runs-on: ubuntu-latest\n steps:\n - uses: owner/action@"+headSHA+"\n"), 0o644)) + locked := []dep.Dependency{{Hostname: "github.com", NWO: "owner/action", Ref: wantRef, SHA: oldSHA, Branch: "main", Tag: tt.tag}} + if tt.recorded { + locked = append(locked, dep.Dependency{Hostname: "github.com", NWO: "owner/action", Ref: headSHA, SHA: headSHA, Branch: "main", Tag: tt.tag}) + } + require.NoError(t, store.Set(ctx, path, locked, nil, nil)) + } + require.NoError(t, store.Save()) + + reg := &httpmock.Registry{} + defer reg.Verify(t) + if !tt.recorded { + reg.Register(httpmock.REST("GET", `repos/owner/action$`), + httpmock.JSONResponse(map[string]any{"default_branch": "main"})) + tags := httpmock.TagListResponse() + if tt.tag != "" { + tags = httpmock.TagListResponse(tt.tag, headSHA) + } + reg.Register(httpmock.REST("GET", `repos/owner/action/tags`), httpmock.JSONResponse(tags)) + 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"}, + })) + } + pool := pinpool.New(2, nil) + resolver, err := resolve.New("github.com", pool, resolve.WithTransport(reg)) + require.NoError(t, err) + resolver.SeedFromLockfile([]dep.Dependency{{Hostname: "github.com", NWO: "owner/action", Ref: headSHA, SHA: headSHA}}) + + firstFiles := make(map[string][]byte) + files := append([]string{parserlock.Path}, paths...) + for pass := range 2 { + store, err := lockfile.LoadState(dir, fakeMeta{}) + require.NoError(t, err) + report := pipeline.DiagnoseParsed(ctx, pipeline.ParseAll(paths, store), nil, store, pool) + record, err := Plan(ctx, report, PlanOptions{ + Resolver: resolver, Tagger: tag.NewListerForTest(t, reg), Store: store, Pool: pool, + }) + require.NoError(t, err) + require.Len(t, record.Entries, len(paths)) + for _, entry := range record.Entries { + assert.Equal(t, wantRef, entry.Ref) + assert.Equal(t, headSHA, entry.SHA) + } + require.NoError(t, Commit(ctx, record, store, nil)) + assert.True(t, pipeline.VerifyLocalCoverage(pipeline.ParseAll(paths, store), store).IsValid()) + for _, path := range files { + content, err := os.ReadFile(path) + require.NoError(t, err) + if pass == 0 { + firstFiles[path] = content + } else { + assert.Equal(t, string(firstFiles[path]), string(content), "%s changed on an unchanged run", path) + } + } + } + }) + } +} + +func TestPlanNormalizationRetainsSharedTargets(t *testing.T) { + const ( + oldSHA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + headSHA = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + ) + for _, tt := range []struct { + name string + unscanned bool + transitive bool + blocked string + }{ + {name: "used by another scanned workflow"}, + {name: "used by an unscanned workflow", unscanned: true}, + {name: "used by a scanned composite", transitive: true}, + {name: "used by an unscanned composite", transitive: true, unscanned: true}, + {name: "stale in a skipped workflow", blocked: "skip"}, + {name: "stale in a workflow with a resolver error", blocked: "resolver"}, + {name: "stale in a workflow with an invalid self ref", blocked: "self"}, + {name: "stale in a workflow with a blocked relock", blocked: "relock"}, + } { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + store, err := lockfile.LoadState(t.TempDir(), fakeMeta{}) + require.NoError(t, err) + locked := dep.Dependency{Hostname: "github.com", NWO: "owner/action", Ref: "main", SHA: oldSHA} + changed := checks.WorkflowReport{ + Path: ".github/workflows/changed.yml", + ActionRefs: []parserlock.ActionRef{{Owner: "owner", Repo: "action", Ref: headSHA}}, + Inventory: []checks.InventoryEntry{{Dep: locked, Direct: true}}, + Findings: []checks.Finding{ + {Category: checks.Stale, Dependency: &locked}, + {Category: checks.NotPinned, Severity: checks.SeverityError}, + }, + } + require.NoError(t, store.Set(ctx, changed.Path, []dep.Dependency{locked}, nil, nil)) + protected := checks.WorkflowReport{Path: ".github/workflows/protected.yml"} + root := locked + deps := []dep.Dependency{locked} + var parents dep.ParentMap + if tt.transitive { + root = dep.Dependency{Hostname: "github.com", NWO: "owner/composite", Ref: "v1.0.0", SHA: "cccccccccccccccccccccccccccccccccccccccc"} + deps = append(deps, root) + parents = dep.ParentMap{locked.Key(): {root.Key()}} + } + require.NoError(t, store.Set(ctx, protected.Path, deps, parents, nil)) + protected.Inventory = []checks.InventoryEntry{{Dep: root, Direct: true}} + if tt.blocked != "" { + protected.Findings = []checks.Finding{{Category: checks.Stale, Dependency: &locked}} + } + switch tt.blocked { + case "skip": + protected.SkipCommit = true + case "resolver": + protected.BlockingResolverError = true + case "self": + protected.Findings = append(protected.Findings, checks.Finding{Category: checks.InvalidSelfRepositoryRef}) + case "relock": + protected.Findings = append(protected.Findings, + checks.Finding{Category: checks.RefMoved, Dependency: &locked}, + checks.Finding{Category: checks.UnreachablePin}, + ) + } + report := &checks.Report{Workflows: []checks.WorkflowReport{changed}} + if !tt.unscanned { + report.Workflows = append(report.Workflows, protected) + } + + 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/tags`), httpmock.JSONResponse(httpmock.TagListResponse())) + 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"}, + })) + pool := pinpool.New(2, nil) + resolver, err := resolve.New("github.com", pool, resolve.WithTransport(reg)) + require.NoError(t, err) + resolver.SeedFromLockfile([]dep.Dependency{{Hostname: "github.com", NWO: "owner/action", Ref: headSHA, SHA: headSHA}}) + record, err := Plan(ctx, report, PlanOptions{ + Resolver: resolver, Tagger: tag.NewListerForTest(t, reg), Store: store, Pool: pool, + PartialScan: tt.unscanned, Relock: tt.blocked == "relock", + }) + require.NoError(t, err) + require.NotEmpty(t, record.Entries) + assert.Equal(t, []string{changed.Path}, record.Entries[0].Workflows) + assert.Equal(t, headSHA, record.Entries[0].Ref) + assert.Equal(t, headSHA, record.Entries[0].SHA) + require.NotEmpty(t, record.Workflows) + assert.Empty(t, record.Workflows[0].Rewrites) + }) + } +} diff --git a/internal/pin/plan_test.go b/internal/pin/plan_test.go index 2996c136..d18e7f9b 100644 --- a/internal/pin/plan_test.go +++ b/internal/pin/plan_test.go @@ -506,6 +506,97 @@ func TestNarrowVerifiedEntries_StickyPrecision(t *testing.T) { }) } +func TestPlanNormalizationRespectsLockedTarget(t *testing.T) { + const ( + oldSHA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + headSHA = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + ) + for _, tt := range []struct { + name string + nwo string + lockedRef string + lockedSHA string + tag string + wantRef string + }{ + { + name: "branch pinned to another commit", nwo: "owner/action", + lockedRef: "main", lockedSHA: oldSHA, wantRef: headSHA, + }, + { + name: "tag pinned to another commit", nwo: "owner/action", + lockedRef: "v1.0.0", lockedSHA: oldSHA, tag: "v1.0.0", wantRef: headSHA, + }, + { + name: "matching branch pin permits normalization", nwo: "owner/action", + lockedRef: "main", lockedSHA: headSHA, wantRef: "main", + }, + { + name: "matching tag pin permits normalization", nwo: "owner/action", + lockedRef: "v1.0.0", lockedSHA: headSHA, tag: "v1.0.0", wantRef: "v1.0.0", + }, + { + name: "repository case does not bypass locked pin", nwo: "Owner/Action", + lockedRef: "main", lockedSHA: oldSHA, wantRef: headSHA, + }, + } { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register(httpmock.REST("GET", `repos/`+tt.nwo+`$`), + httpmock.JSONResponse(map[string]any{"default_branch": "main"})) + reg.Register(httpmock.REST("GET", `repos/`+tt.nwo+`/git/ref/heads/main`), + httpmock.JSONResponse(map[string]any{ + "ref": "refs/heads/main", "object": map[string]any{"sha": headSHA, "type": "commit"}, + })) + tags := httpmock.TagListResponse() + if tt.tag != "" { + tags = httpmock.TagListResponse(tt.tag, headSHA) + } + reg.Register(httpmock.REST("GET", `repos/`+tt.nwo+`/tags`), httpmock.JSONResponse(tags)) + store, err := lockfile.LoadState(t.TempDir(), fakeMeta{}) + require.NoError(t, err) + locked := dep.Dependency{Hostname: "github.com", NWO: "owner/action", Ref: tt.lockedRef, SHA: tt.lockedSHA} + require.NoError(t, store.Set(context.Background(), "existing.yml", []dep.Dependency{locked}, nil, nil)) + pool := pinpool.New(2, nil) + resolver, err := resolve.New("github.com", pool, resolve.WithTransport(reg)) + require.NoError(t, err) + resolver.SeedFromLockfile([]dep.Dependency{{Hostname: "github.com", NWO: tt.nwo, Ref: headSHA, SHA: headSHA}}) + owner, repo := splitNWO(tt.nwo) + report := &checks.Report{Workflows: []checks.WorkflowReport{ + { + Path: ".github/workflows/existing.yml", + Inventory: []checks.InventoryEntry{{Dep: locked, Direct: true}}, + }, + { + Path: ".github/workflows/new.yml", + ActionRefs: []parserlock.ActionRef{{Owner: owner, Repo: repo, Ref: headSHA}}, + Findings: []checks.Finding{{Category: checks.NotPinned, Severity: checks.SeverityError}}, + }, + }} + + record, err := Plan(context.Background(), report, PlanOptions{ + Resolver: resolver, Tagger: tag.NewListerForTest(t, reg), Store: store, Pool: pool, + }) + require.NoError(t, err) + require.Len(t, record.Entries, 2) + assert.Equal(t, tt.lockedRef, record.Entries[0].Ref) + assert.Equal(t, tt.lockedSHA, record.Entries[0].SHA) + assert.Equal(t, tt.wantRef, record.Entries[1].Ref) + assert.Equal(t, headSHA, record.Entries[1].SHA) + assert.Equal(t, "main", record.Entries[1].OnBranch) + assert.Equal(t, tt.tag, record.Entries[1].Tag) + require.Len(t, record.Workflows, 2) + assert.Empty(t, record.Workflows[0].Rewrites) + if tt.wantRef == headSHA { + assert.Empty(t, record.Workflows[1].Rewrites) + } else { + assert.Equal(t, map[string]string{tt.nwo + "@" + headSHA: tt.nwo + "@" + tt.wantRef}, record.Workflows[1].Rewrites) + } + }) + } +} + func TestPlanRejectsConflictingSameRunRepairs(t *testing.T) { const firstSHA = "abc1230000000000000000000000000000000000" const secondSHA = "def4560000000000000000000000000000000000"