Merge main and preserve transfer rewrite coverage

Author: nodeselector

.github/workflows/dependency-review.yml

@@ -1,5 +1,3 @@
-# This workflow is managed by gh actions-lock.
-
 name: dependency-review
 
 on:

.github/workflows/release.yml

@@ -1,5 +1,3 @@
-# This workflow is managed by gh actions-lock.
-
 name: Release
 
 on:

.github/workflows/test.yml

@@ -1,5 +1,3 @@
-# This workflow is managed by gh actions-lock.
-
 name: test
 
 on:

cmd/gh-actions-lock/command_test.go

@@ -520,6 +520,123 @@ func TestCheckCommand_RejectsTransferredRecordedRemoteCompositeRef(t *testing.T)
 	assert.Equal(t, lockBefore, lockAfter)
 }
 
+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 {

cmd/gh-actions-lock/selfrepository_test.go

@@ -8,7 +8,6 @@ import (
 	"sync/atomic"
 	"testing"
 
-	"github.com/github/gh-actions-lock/internal/workflowfile"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -51,7 +50,6 @@ jobs:
 	gotWorkflow, err := os.ReadFile(workflowPath)
 	require.NoError(t, err)
 	assert.Equal(t, workflow, gotWorkflow)
-	assert.NotContains(t, string(gotWorkflow), workflowfile.SentinelComment)
 
 	lockPath := filepath.Join(dir, ".github", "workflows", "actions.lock")
 	if lockContent, readErr := os.ReadFile(lockPath); readErr == nil {
@@ -92,7 +90,6 @@ jobs:
 	gotWorkflow, readErr := os.ReadFile(workflowPath)
 	require.NoError(t, readErr)
 	assert.Equal(t, workflow, gotWorkflow)
-	assert.NotContains(t, string(gotWorkflow), workflowfile.SentinelComment)
 	lockPath := filepath.Join(dir, ".github", "workflows", "actions.lock")
 	_, readErr = os.Stat(lockPath)
 	assert.ErrorIs(t, readErr, os.ErrNotExist)

internal/pin/commit.go

@@ -133,16 +133,14 @@ func rewriteWorkflow(wp WorkflowPlan) error {
 		}
 	}
 
-	content = workflowfile.EnsureSentinel(content)
 	if bytes.Equal(content, wf.Content) {
 		return nil
 	}
 	return os.WriteFile(wp.Path, content, 0o644)
 }
 
 // rewriteSelfActionFiles applies each workflow's rewrites to the in-repo
-// action files it reaches via `$/…`. No sentinel comment: these are action
-// definitions, not managed workflows.
+// action files it reaches via `$/…`.
 func rewriteSelfActionFiles(plans []WorkflowPlan) error {
 	merged := make(map[string]map[string]string)
 	for _, wp := range plans {

internal/pin/commit_test.go

@@ -113,11 +113,12 @@ func TestCommitRemovesDependenciesDroppedFromWorkflow(t *testing.T) {
 	dir := t.TempDir()
 	workflowPath := filepath.Join(".github", "workflows", "ci.yml")
 	require.NoError(t, os.MkdirAll(filepath.Join(dir, filepath.Dir(workflowPath)), 0o755))
-	require.NoError(t, os.WriteFile(filepath.Join(dir, workflowPath), []byte(`on: push
+	workflow := []byte(`on: push
 jobs:
   lint:
     uses: owner/reusable/.github/workflows/lint.yml@main
-`), 0o644))
+`)
+	require.NoError(t, os.WriteFile(filepath.Join(dir, workflowPath), workflow, 0o644))
 	t.Chdir(dir)
 
 	store, err := lockfile.LoadState(dir, fakeMeta{})
@@ -144,6 +145,10 @@ jobs:
 	}
 	require.NoError(t, Commit(context.Background(), rec, store, nil))
 
+	gotWorkflow, err := os.ReadFile(filepath.Join(dir, workflowPath))
+	require.NoError(t, err)
+	assert.Equal(t, workflow, gotWorkflow)
+
 	got, err := os.ReadFile(filepath.Join(dir, ".github", "workflows", "actions.lock"))
 	require.NoError(t, err)
 	assert.Contains(t, string(got), "actions/checkout@v7")

internal/pin/plan.go

@@ -228,17 +228,16 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption
 	}
 	parentMap = dep.RekeyParentMap(parentMap, canonicalRekeys)
 
-	// 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
 	}
@@ -308,8 +307,7 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption
 			SelfActionFiles:  wr.SelfActionFiles,
 		})
 	} else if len(wplans) == 0 {
-		// No rewrites and no plan entry yet — still include the workflow
-		// so EnsureSentinel can be applied during commit.
+		// Keep the workflow in the plan so its lockfile entry is updated.
 		wplans = append(wplans, WorkflowPlan{Path: wr.Path, SelfActionFiles: wr.SelfActionFiles})
 	}
 
@@ -404,10 +402,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
 	}
@@ -425,29 +422,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
+				preservedDeps[i] = true
 			}
-			patchTag, err := opts.Tagger.BestPatchTagForSHA(ctx, owner, repo, dep.SHA)
-			if err != nil {
-				continue
-			}
-			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
 		}
 
@@ -469,37 +448,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
 		}
 	}
 
@@ -519,7 +491,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
@@ -540,11 +512,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
 	}
@@ -747,17 +716,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