Only rewrite action refs to tags at the resolved commit

cmd/gh-actions-lock/command_test.go

@@ -67,6 +67,123 @@ jobs:
 	assert.Empty(t, payload.Findings)
 }
 
+func TestCheck_BareSHAUsesExactMajorTagAndVerifiesLocally(t *testing.T) {
+	const (
+		sha         = "b6e2e70617bc3265edd6dab6c906732b2f1ae151"
+		ancestorSHA = "09f2f74827fd0000000000000000000000000000"
+	)
+
+	reg := &httpmock.Registry{}
+	reg.Register(
+		httpmock.GraphQLForRepo("dawidd6", "action-download-artifact"),
+		httpmock.JSONResponse(map[string]any{
+			"data": map[string]any{
+				"a0": testRepoResponse("dawidd6/action-download-artifact", sha, nodeActionYAML),
+			},
+		}),
+	)
+	reg.Register(
+		httpmock.REST("GET", `repos/dawidd6/action-download-artifact$`),
+		httpmock.JSONResponse(map[string]any{
+			"default_branch": "main",
+			"id":             2,
+			"owner":          map[string]any{"id": 1},
+		}),
+	)
+	reg.Register(
+		httpmock.REST("GET", `repos/dawidd6/action-download-artifact/git/ref/heads/main`),
+		httpmock.JSONResponse(map[string]any{
+			"ref": "refs/heads/main", "object": map[string]any{"sha": sha, "type": "commit"},
+		}),
+	)
+	reg.Register(
+		httpmock.REST("GET", `repos/dawidd6/action-download-artifact/tags`),
+		httpmock.JSONResponse(httpmock.TagListResponse(
+			"v21", sha,
+			"v3.1.4", ancestorSHA,
+		)),
+	)
+	reg.Register(
+		httpmock.REST("GET", `repos/dawidd6/action-download-artifact/releases`),
+		httpmock.JSONResponse([]map[string]any{}),
+	)
+	reg.Register(
+		httpmock.REST("GET", `repos/dawidd6/action-download-artifact/compare/09f2f74827fd0000000000000000000000000000\.\.\.b6e2e70617bc3265edd6dab6c906732b2f1ae151`),
+		httpmock.JSONResponse(httpmock.CompareAncestorResponse(ancestorSHA)),
+	)
+
+	workflowPath := writeTempWorkflow(t, `
+name: ci
+on: push
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: dawidd6/action-download-artifact@b6e2e70617bc3265edd6dab6c906732b2f1ae151
+`)
+
+	_, _, err := runCommandWithHTTP(t, reg, workflowPath)
+	require.NoError(t, err)
+
+	workflow, err := os.ReadFile(workflowPath)
+	require.NoError(t, err)
+	assert.Contains(t, string(workflow), "dawidd6/action-download-artifact@v21")
+	assert.NotContains(t, string(workflow), "@v3.1.4")
+
+	lock := readTempLockfilePins(t)
+	assert.Contains(t, lock, "'dawidd6/action-download-artifact@v21':")
+	assert.Contains(t, lock, "ref: 'v21'")
+	assert.Contains(t, lock, "commit: 'sha1-"+sha+"'")
+	assert.NotContains(t, lock, "v3.1.4")
+
+	_, _, err = runCommandWithHTTP(t, &httpmock.Registry{}, "--verify-local", workflowPath)
+	require.NoError(t, err)
+}
+
+func TestCheck_ChangedTagAtSameCommitRekeysAndVerifiesLocally(t *testing.T) {
+	const sha = "94de994a9f6fffee200243214e17002e2920bb59"
+
+	reg := &httpmock.Registry{}
+	reg.Register(
+		httpmock.GraphQLForRepo("dawidd6", "action-send-mail"),
+		httpmock.JSONResponse(map[string]any{
+			"data": map[string]any{
+				"a0": testRepoResponse("dawidd6/action-send-mail", sha, nodeActionYAML),
+			},
+		}),
+	)
+	reg.Register(
+		httpmock.REST("GET", `repos/dawidd6/action-send-mail/tags`),
+		httpmock.JSONResponse(httpmock.TagListResponse("v18", sha, "v3.12.0", sha)),
+	)
+	reg.Register(
+		httpmock.REST("GET", `repos/dawidd6/action-send-mail/releases`),
+		httpmock.JSONResponse([]map[string]any{}),
+	)
+
+	workflowPath := writeTempWorkflow(t, `
+name: ci
+on: push
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: dawidd6/action-send-mail@v18
+`, "dawidd6/action-send-mail@v3.12.0=sha1-"+sha)
+
+	_, _, err := runCommandWithHTTP(t, reg, workflowPath)
+	require.NoError(t, err)
+
+	lock := readTempLockfilePins(t)
+	assert.Contains(t, lock, "'dawidd6/action-send-mail@v18':")
+	assert.Contains(t, lock, "ref: 'v18'")
+	assert.Contains(t, lock, "commit: 'sha1-"+sha+"'")
+	assert.NotContains(t, lock, "v3.12.0")
+
+	_, _, err = runCommandWithHTTP(t, &httpmock.Registry{}, "--verify-local", workflowPath)
+	require.NoError(t, err)
+}
+
 const nodeActionYAML = "name: Test Action\nruns:\n  using: node20\n"
 
 func testRepoResponse(nameWithOwner, oid, actionYAML string) map[string]any {

internal/pin/plan.go

@@ -195,17 +195,16 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption
 	rootTracker := lockfile.NewDirectTracker(unrecordedRefs, deps)
 	rewriteTracker := lockfile.NewDirectTracker(rewriteRefs, deps)
 
-	// Narrow mutable version tags to patch tags, and resolve bare-SHA refs
-	// to a symbolic tag when one exists.
+	// Narrow mutable version tags to exact patch tags.
 	status("pinning " + wr.Path)
 	rewrites := make(map[string]string)
-	narrowedNWOs := make(map[string]bool) // NWOs where narrowing chose a tag
+	preservedDeps := make(map[int]bool)
 
-	narrowDirectDeps(ctx, opts, deps, rewriteTracker, rewrites, narrowedNWOs)
+	narrowDirectDeps(ctx, opts, deps, rewriteTracker, rewrites, preservedDeps)
 
 	// ReverseLookup canonicalizes each dep's ref while preserving the tags
 	// narrowing chose and transitive deps' declared refs.
-	rlRewrites, lookupIssues, err := reverseLookupRewrites(ctx, opts, wr, deps, rewriteTracker, narrowedNWOs)
+	rlRewrites, lookupIssues, err := reverseLookupRewrites(ctx, opts, wr, deps, rewriteTracker, preservedDeps)
 	if err != nil {
 		return planResult{}, err
 	}
@@ -319,10 +318,9 @@ func unresolvedEntries(wr checks.WorkflowReport, unrecordedRefs []parserlock.Act
 	return out
 }
 
-// narrowDirectDeps rewrites direct deps' mutable refs to precise tags (bare SHA
-// or partial/non-semver ref -> full patch tag), leaving transitive deps verbatim.
-// Each rewrite mutates deps[i].Ref and records the old->new uses and narrowed NWO.
-func narrowDirectDeps(ctx context.Context, opts PlanOptions, deps []dep.Dependency, directTracker lockfile.DirectTracker, rewrites map[string]string, narrowedNWOs map[string]bool) {
+// narrowDirectDeps rewrites direct partial semver refs to exact patch tags,
+// leaving bare SHA and transitive refs for reverse lookup.
+func narrowDirectDeps(ctx context.Context, opts PlanOptions, deps []dep.Dependency, directTracker lockfile.DirectTracker, rewrites map[string]string, preservedDeps map[int]bool) {
 	if opts.Tagger == nil {
 		return
 	}
@@ -340,29 +338,11 @@ func narrowDirectDeps(ctx context.Context, opts PlanOptions, deps []dep.Dependen
 			continue
 		}
 
-		// Bare-SHA refs: find a tag pointing at the same commit.
-		// Skip if --no-narrow — the user wants to keep their commit SHA as-is.
-		// Mark narrowedNWOs so ReverseLookup also preserves the SHA ref.
+		// ReverseLookup owns bare-SHA normalization unless --no-narrow protects it.
 		if parserlock.IsFullSha(dep.Ref) {
 			if opts.NoNarrow {
-				narrowedNWOs[strings.ToLower(dep.NWO)] = true
-				continue
-			}
-			patchTag, err := opts.Tagger.BestPatchTagForSHA(ctx, owner, repo, dep.SHA)
-			if err != nil {
-				continue
+				preservedDeps[i] = true
 			}
-			if patchTag == "" {
-				patchTag, err = opts.Tagger.BestAncestorTag(ctx, owner, repo, dep.SHA)
-				if err != nil || patchTag == "" {
-					continue
-				}
-			}
-			oldUses := dep.NWO + "@" + dep.Ref
-			newUses := dep.NWO + "@" + patchTag
-			rewrites[oldUses] = newUses
-			dep.Ref = patchTag
-			narrowedNWOs[strings.ToLower(dep.NWO)] = true
 			continue
 		}
 
@@ -384,37 +364,30 @@ func narrowDirectDeps(ctx context.Context, opts PlanOptions, deps []dep.Dependen
 			continue
 		}
 
-		patchTag, err := opts.Tagger.BestPatchTagForSHA(ctx, owner, repo, dep.SHA)
-		if err != nil {
+		patchTag, err := opts.Tagger.BestPatchTagForSHA(ctx, owner, repo, dep.SHA, dep.Ref)
+		if err != nil || patchTag == "" {
 			continue
 		}
-		// No exact tag match - if the repo publishes semver releases,
-		// walk back to the latest tag that's an ancestor of this SHA.
-		if patchTag == "" {
-			patchTag, err = opts.Tagger.BestAncestorTag(ctx, owner, repo, dep.SHA)
-			if err != nil || patchTag == "" {
-				continue
-			}
-		}
 		oldUses := dep.NWO + "@" + dep.Ref
 		newUses := dep.NWO + "@" + patchTag
 		rewrites[oldUses] = newUses
 		dep.Ref = patchTag
-		narrowedNWOs[nwoLower] = true
+		preservedDeps[i] = true
 	}
 }
 
 // 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.
-func reverseLookupRewrites(ctx context.Context, opts PlanOptions, wr checks.WorkflowReport, deps []dep.Dependency, directTracker lockfile.DirectTracker, narrowedNWOs map[string]bool) (map[string]string, []resolve.LookupIssue, 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.
-	narrowedRefs := make(map[int]string)
+	preservedRefs := make(map[int]string)
+	preservedKeys := make(map[string]bool)
 	for i := range deps {
-		nwo := strings.ToLower(deps[i].NWO)
-		if narrowedNWOs[nwo] {
-			narrowedRefs[i] = deps[i].Ref
+		if preservedDeps[i] {
+			preservedRefs[i] = deps[i].Ref
+			preservedKeys[deps[i].Key()] = true
 		}
 	}
 
@@ -434,7 +407,7 @@ func reverseLookupRewrites(ctx context.Context, opts PlanOptions, wr checks.Work
 		return nil, nil, fmt.Errorf("reverse lookup: %w", err)
 	}
 	// Restore narrowed refs that ReverseLookup may have overwritten.
-	for i, ref := range narrowedRefs {
+	for i, ref := range preservedRefs {
 		deps[i].Ref = ref
 	}
 	// Restore transitive deps' declared refs — we don't own the composite's
@@ -455,11 +428,8 @@ func reverseLookupRewrites(ctx context.Context, opts PlanOptions, wr checks.Work
 		if transitiveRewriteKeys[k] {
 			continue
 		}
-		if at := strings.Index(k, "@"); at > 0 {
-			nwo := strings.ToLower(k[:at])
-			if narrowedNWOs[nwo] {
-				continue
-			}
+		if preservedKeys[k] {
+			continue
 		}
 		rewrites[k] = v
 	}
@@ -662,17 +632,10 @@ func narrowVerifiedEntries(ctx context.Context, entries []Entry, opts PlanOption
 		if sv.IsFull() {
 			continue
 		}
-		// Try exact tag match, then ancestor fallback.
-		patchTag, err := opts.Tagger.BestPatchTagForSHA(ctx, owner, repo, e.SHA)
-		if err != nil {
+		patchTag, err := opts.Tagger.BestPatchTagForSHA(ctx, owner, repo, e.SHA, e.Ref)
+		if err != nil || patchTag == "" {
 			continue
 		}
-		if patchTag == "" {
-			patchTag, err = opts.Tagger.BestAncestorTag(ctx, owner, repo, e.SHA)
-			if err != nil || patchTag == "" {
-				continue
-			}
-		}
 		oldRef := e.Ref
 		oldUses := e.NWO + "@" + oldRef
 		newUses := e.NWO + "@" + patchTag