pin: preserve locked targets during SHA normalization

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)
+		})
+	}
+}

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
 	}