Pinning: guard SHA metadata repairs

Author: nodeselector

internal/pin/plan.go

@@ -130,7 +130,6 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption
 	if rewriteRefs == nil {
 		rewriteRefs = wr.ActionRefs
 	}
-	rewriteRefKeys := actionRefKeys(rewriteRefs)
 	selfActionRefKeys := actionRefKeys(wr.SelfActionRefs)
 	if opts.PartialScan && !opts.NoNarrow && opts.Tagger != nil {
 		// ponytail: refuse any local-action repair in a partial scan; track
@@ -150,7 +149,7 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption
 
 	if !wr.NeedsAttention() && !repinMoved {
 		entries = verifiedEntries(inventory, wr.Path)
-		rw := narrowVerifiedEntries(ctx, entries, opts, rewriteRefKeys)
+		rw := narrowVerifiedEntries(ctx, entries, opts, rewriteRefs)
 		wplans = append(wplans, WorkflowPlan{Path: wr.Path, Rewrites: rw, SelfActionFiles: wr.SelfActionFiles})
 		return planResult{entries: entries, wplans: wplans}, nil
 	}
@@ -184,7 +183,7 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption
 	}
 
 	if len(unrecordedRefs) == 0 {
-		rw := narrowVerifiedEntries(ctx, entries, opts, rewriteRefKeys)
+		rw := narrowVerifiedEntries(ctx, entries, opts, rewriteRefs)
 		wplans = append(wplans, WorkflowPlan{Path: wr.Path, Rewrites: rw, SelfActionFiles: wr.SelfActionFiles})
 		return planResult{entries: entries, wplans: wplans}, nil
 	}
@@ -274,7 +273,7 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption
 
 	// Record workflow plan if there are rewrites.
 	// Also narrow any verified (already-recorded) entries that have imprecise refs.
-	if verifiedRW := narrowVerifiedEntries(ctx, entries, opts, rewriteRefKeys); len(verifiedRW) > 0 {
+	if verifiedRW := narrowVerifiedEntries(ctx, entries, opts, rewriteRefs); len(verifiedRW) > 0 {
 		for k, v := range verifiedRW {
 			rewrites[k] = v
 		}
@@ -643,10 +642,11 @@ func verifiedEntries(inventory []checks.InventoryEntry, path string) []Entry {
 // narrowVerifiedEntries upgrades already-recorded direct deps to full semver
 // tags when possible, returning the workflow-YAML rewrites. Skipped for
 // --no-narrow, transitive deps, and refs the user kept imprecise (sticky v4).
-func narrowVerifiedEntries(ctx context.Context, entries []Entry, opts PlanOptions, rewriteRefKeys map[string]bool) map[string]string {
+func narrowVerifiedEntries(ctx context.Context, entries []Entry, opts PlanOptions, rewriteRefs []parserlock.ActionRef) map[string]string {
 	if opts.NoNarrow || opts.Tagger == nil {
 		return nil
 	}
+	rewriteRefKeys := actionRefKeys(rewriteRefs)
 	rewrites := make(map[string]string)
 	for i := range entries {
 		e := &entries[i]
@@ -664,8 +664,15 @@ func narrowVerifiedEntries(ctx context.Context, entries []Entry, opts PlanOption
 			if newRef == "" {
 				continue
 			}
+			if parserlock.IsFullSha(newRef) || hasConflictingLockTarget(opts.Store, e.NWO, newRef, e.SHA) {
+				continue
+			}
 			oldRef := e.Ref
-			rewrites[e.NWO+"@"+oldRef] = e.NWO + "@" + newRef
+			for _, ref := range rewriteRefs {
+				if strings.EqualFold(ref.Owner+"/"+ref.Repo, e.NWO) && ref.Ref == oldRef {
+					rewrites[ref.FullName()+"@"+oldRef] = ref.FullName() + "@" + newRef
+				}
+			}
 			e.Ref = newRef
 			e.AutoFixedRef = oldRef
 			continue
@@ -716,6 +723,18 @@ func narrowVerifiedEntries(ctx context.Context, entries []Entry, opts PlanOption
 	return rewrites
 }
 
+func hasConflictingLockTarget(store *lockfile.State, nwo, ref, sha string) bool {
+	if store == nil {
+		return false
+	}
+	action, ok := store.File().Dependencies[nwo+"@"+ref]
+	if !ok {
+		return false
+	}
+	_, targetSHA, ok := strings.Cut(action.Commit, "-")
+	return ok && !strings.EqualFold(targetSHA, sha)
+}
+
 func actionRefKeys(refs []parserlock.ActionRef) map[string]bool {
 	keys := make(map[string]bool, len(refs))
 	for _, ref := range refs {

internal/pin/plan_test.go

@@ -5,6 +5,7 @@ import (
 	"testing"
 
 	"github.com/github/gh-actions-lock/internal/dep"
+	"github.com/github/gh-actions-lock/internal/lockfile"
 	"github.com/github/gh-actions-lock/internal/pipeline/checks"
 
 	parserlock "github.com/github/actions-lockfile/go/pkg/lockfile"
@@ -324,6 +325,76 @@ func TestNarrowVerifiedEntries_StickyPrecision(t *testing.T) {
 		)
 	})
 
+	t.Run("SHA metadata does not rewrite to itself", func(t *testing.T) {
+		tagger, _ := newTagger(t)
+		report := fastPathReport(sha)
+		report.Inventory[0].Dep.Branch = sha
+		report.ActionRefs = []parserlock.ActionRef{{
+			Owner: "actions",
+			Repo:  "checkout",
+			Ref:   sha,
+		}}
+
+		result, err := planWorkflow(context.Background(), report, PlanOptions{Tagger: tagger}, func(string) {})
+		require.NoError(t, err)
+
+		require.Len(t, result.entries, 1)
+		assert.Equal(t, sha, result.entries[0].Ref)
+		assert.Empty(t, result.entries[0].AutoFixedRef)
+		assert.Empty(t, result.wplans[0].Rewrites)
+	})
+
+	t.Run("repair preserves source NWO spelling", func(t *testing.T) {
+		tagger, _ := newTagger(t)
+		report := fastPathReport(sha)
+		report.Inventory[0].Dep.Tag = "v4.2.1"
+		report.ActionRefs = []parserlock.ActionRef{{
+			Owner: "Actions",
+			Repo:  "Checkout",
+			Ref:   sha,
+		}}
+
+		result, err := planWorkflow(context.Background(), report, PlanOptions{Tagger: tagger}, func(string) {})
+		require.NoError(t, err)
+
+		require.Len(t, result.entries, 1)
+		assert.Equal(t, "v4.2.1", result.entries[0].Ref)
+		assert.Equal(t,
+			map[string]string{"Actions/Checkout@" + sha: "Actions/Checkout@v4.2.1"},
+			result.wplans[0].Rewrites,
+		)
+	})
+
+	t.Run("repair declines conflicting symbolic target", func(t *testing.T) {
+		tagger, _ := newTagger(t)
+		store, err := lockfile.LoadState(t.TempDir(), fakeMeta{})
+		require.NoError(t, err)
+		target := dep.Dependency{
+			NWO:      "actions/checkout",
+			Ref:      "v4.2.1",
+			SHA:      "def4560000000000000000000000000000000000",
+			HashAlgo: "sha1",
+		}
+		require.NoError(t, store.Set(context.Background(), "other.yml",
+			[]dep.Dependency{target}, nil, map[string]bool{target.Key(): true}))
+
+		report := fastPathReport(sha)
+		report.Inventory[0].Dep.Tag = target.Ref
+		report.ActionRefs = []parserlock.ActionRef{{
+			Owner: "actions",
+			Repo:  "checkout",
+			Ref:   sha,
+		}}
+
+		result, err := planWorkflow(context.Background(), report, PlanOptions{Tagger: tagger, Store: store}, func(string) {})
+		require.NoError(t, err)
+
+		require.Len(t, result.entries, 1)
+		assert.Equal(t, sha, result.entries[0].Ref)
+		assert.Empty(t, result.entries[0].AutoFixedRef)
+		assert.Empty(t, result.wplans[0].Rewrites)
+	})
+
 	t.Run("branch ref main is NOT narrowed", func(t *testing.T) {
 		// main is not version-shaped, so narrowing must not touch it.
 		// Non-version refs are intentional choices (e.g. vercel/next.js@canary).