Lock refs another workflow rewrites in a shared $/ action

cmd/gh-actions-lock/run_test.go

@@ -817,6 +817,75 @@ func TestRun_SelfRepositoryLocalActionNamesItsParent(t *testing.T) {
 	assert.Contains(t, stderr, "./helper (in $/actions/build), ./helper (in $/actions/test)")
 }
 
+// TestRun_SharedSelfActionRewriteIsLockedForEveryWorkflow: ci.yml and
+// deploy.yml share $/.github/actions/restore, which uses
+// octo/cache/restore by SHA. ci.yml keeps that SHA, since octo/apt
+// declares it too; deploy.yml rewrites it to v2.0.0 in the shared file.
+// ci.yml then runs v2.0.0, so the lockfile must cover it. The SHA stays a
+// ci.yml root only while ci.yml still uses it directly; otherwise it is
+// locked only as octo/apt's child.
+func TestRun_SharedSelfActionRewriteIsLockedForEveryWorkflow(t *testing.T) {
+	const cacheSHA, aptSHA = "cccccccccccccccccccccccccccccccccccccccc", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+	byCommit := "octo/cache/restore@" + strings.ToUpper(cacheSHA)
+	for name, tc := range map[string]struct {
+		steps string
+		roots []string
+	}{
+		"used directly too": {
+			steps: "      - uses: " + byCommit + "\n",
+			roots: []string{"octo/apt@v1.0.0", "octo/cache@" + cacheSHA, "octo/cache@v2.0.0"},
+		},
+		"used only in the shared action": {
+			roots: []string{"octo/apt@v1.0.0", "octo/cache@v2.0.0"},
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			manifests := map[string]string{"octo/apt": composite("octo/cache@" + cacheSHA)}
+			commits := map[string]string{"octo/cache": cacheSHA, "octo/apt": aptSHA}
+			reg := &httpmock.Registry{}
+			registerLockedManifests(reg, manifests)
+			for nwo, tags := range map[string]any{
+				"octo/cache": httpmock.TagListResponse("v2.0.0", cacheSHA, "v2", cacheSHA),
+				"octo/apt":   httpmock.TagListResponse("v1.0.0", aptSHA, "v1", aptSHA),
+			} {
+				reg.RegisterReusable(httpmock.REST("GET", `repos/`+nwo+`/tags`), httpmock.JSONResponse(tags))
+				reg.RegisterReusable(httpmock.REST("GET", `repos/`+nwo+`$`), httpmock.JSONResponse(map[string]any{"id": 1, "owner": map[string]any{"id": 1}, "default_branch": "main"}))
+				reg.RegisterReusable(httpmock.REST("GET", `repos/`+nwo+`/branches`), httpmock.JSONResponse(httpmock.BranchListResponse("main", commits[nwo])))
+				owner, repo, _ := strings.Cut(nwo, "/")
+				httpmock.NoTagsNamed(reg, owner, repo)
+			}
+			registerLive(reg, manifests, commits)
+			stubGraph(t, reg, nil, commits, nil)
+			useDefaultTransport(t, reg)
+			steps := "on: push\njobs:\n  test:\n    runs-on: ubuntu-latest\n    steps:\n"
+			ci := writeTempWorkflow(t, steps+"      - uses: octo/apt@v1.0.0\n"+tc.steps+"      - uses: $/.github/actions/restore\n")
+			require.NoError(t, os.Mkdir(".git", 0o755))
+			deploy := filepath.Join(".github", "workflows", "deploy.yml")
+			require.NoError(t, os.WriteFile(deploy, []byte(steps+"      - uses: $/.github/actions/restore\n"), 0o600))
+			restore := filepath.Join(".github", "actions", "restore", "action.yml")
+			require.NoError(t, os.MkdirAll(filepath.Dir(restore), 0o755))
+			require.NoError(t, os.WriteFile(restore, []byte(composite(byCommit)), 0o600))
+
+			_, stderr, err := runCommandWithHTTP(t, reg, "--no-interactive")
+			require.NoError(t, err, stderr)
+			lock := readTempLockfilePins(t)
+			action, err := os.ReadFile(restore)
+			require.NoError(t, err)
+			assert.Contains(t, string(action), "uses: octo/cache/restore@v2.0.0\n")
+			want := "'" + filepath.ToSlash(ci) + "':\n"
+			for _, root := range tc.roots {
+				want += "        - '" + root + "'\n"
+			}
+			assert.Contains(t, lock, want+"dependencies:")
+			assert.Contains(t, lock, "        uses:\n            - 'octo/cache@"+cacheSHA+"'\n")
+			for _, mode := range []string{"--verify-local", "--verify"} {
+				_, stderr, err := runCommandWithHTTP(t, reg, mode)
+				require.NoError(t, err, "%s: %s\n%s", mode, stderr, lock)
+			}
+		})
+	}
+}
+
 // TestRun_LocalActionUsingRemoteActionsBlocks: a `./…` composite in the
 // checkout that uses remote actions fails every mode, since the runner would
 // reject its unlocked uses; --ignore-local-actions (or the old

internal/pin/commit.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"maps"
 	"os"
 	"runtime"
 	"slices"
@@ -37,6 +38,10 @@ func Commit(ctx context.Context, rec *Record, store *lockfile.State, copts *Comm
 	if err := validateRequiredRewrites(rec.Workflows); err != nil {
 		return err
 	}
+	selfRewrites := mergeSelfActionRewrites(rec.Workflows)
+	if err := adoptSharedRewrites(rec, selfRewrites); err != nil {
+		return err
+	}
 
 	// Phase 1: Update lockfile entries in memory. It runs before any file is
 	// rewritten, so an entry the lockfile refuses leaves every file as it was.
@@ -71,7 +76,7 @@ func Commit(ctx context.Context, rec *Record, store *lockfile.State, copts *Comm
 
 	// Action files can be shared by several workflows, so merge their
 	// rewrites and write each file once, serially.
-	if err := rewriteSelfActionFiles(rec.Workflows); err != nil {
+	if err := rewriteSelfActionFiles(selfRewrites); err != nil {
 		return err
 	}
 
@@ -167,9 +172,9 @@ func rewriteWorkflow(wp WorkflowPlan) error {
 	return os.WriteFile(wp.Path, content, 0o644)
 }
 
-// rewriteSelfActionFiles applies each workflow's rewrites to the in-repo
-// action files it reaches via `$/…`.
-func rewriteSelfActionFiles(plans []WorkflowPlan) error {
+// mergeSelfActionRewrites merges, per in-repo action file reached via `$/…`,
+// the rewrites of every workflow that reaches it.
+func mergeSelfActionRewrites(plans []WorkflowPlan) map[string]map[string]string {
 	merged := make(map[string]map[string]string)
 	for _, wp := range plans {
 		if len(wp.Rewrites) == 0 {
@@ -184,6 +189,144 @@ func rewriteSelfActionFiles(plans []WorkflowPlan) error {
 			}
 		}
 	}
+	return merged
+}
+
+// adoptSharedRewrites records, under each workflow, the pins another
+// workflow's rewrites give the action files they share. A workflow can keep a
+// ref another rewrites (a composite it uses declares that ref), and the
+// runner looks up each `uses:` in a `$/…` action as written, so the workflow
+// must lock both. Rewriting both to one ref would rename the composite's
+// declared edge instead.
+func adoptSharedRewrites(rec *Record, merged map[string]map[string]string) error {
+	type pinKey struct{ wf, key string }
+	adopted := map[pinKey]bool{}
+	displaced := map[string][]*parserlock.ActionRef{}
+	for _, wp := range rec.Workflows {
+		for _, path := range wp.SelfActionFiles {
+			foreign := map[string]string{}
+			for oldUses, newUses := range merged[path] {
+				if own, ok := workflowfile.LookupRewrite(oldUses, wp.Rewrites); !ok || own != newUses {
+					foreign[oldUses] = newUses
+				}
+			}
+			if len(foreign) == 0 {
+				continue
+			}
+			wf, err := workflowfile.Load(path)
+			if err != nil {
+				return fmt.Errorf("loading %s: %w", path, err)
+			}
+			for _, oldUses := range slices.Sorted(maps.Keys(foreign)) {
+				newUses := foreign[oldUses]
+				if _, changed, err := wf.RewriteActionRefs(map[string]string{oldUses: newUses}); err != nil {
+					return fmt.Errorf("rewriting %s: %w", path, err)
+				} else if changed == 0 {
+					continue
+				}
+				if old := parserlock.ParseActionRef(oldUses); old != nil {
+					displaced[wp.Path] = append(displaced[wp.Path], old)
+				}
+				ref := parserlock.ParseActionRef(newUses)
+				if ref == nil {
+					continue
+				}
+				k := pinKey{wp.Path, strings.ToLower(ref.NWO()) + "@" + lockfile.CanonicalRef(ref.Ref)}
+				if adopted[k] {
+					continue
+				}
+				e, ok := pinnedEntry(rec, ref.NWO(), ref.Ref)
+				if !ok {
+					return fmt.Errorf("rewriting %s for %s: no planned pin for %s", path, wp.Path, ref.NWO()+"@"+ref.Ref)
+				}
+				adopted[k] = true
+				e.Workflows = []string{wp.Path}
+				e.Direct = true
+				e.RequiredBy = nil
+				rec.Entries = append(rec.Entries, e)
+			}
+		}
+	}
+	for _, wp := range rec.Workflows {
+		if err := demoteDisplacedRoots(rec, wp, merged, displaced[wp.Path]); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+// demoteDisplacedRoots stops recording as a root of wp each displaced ref
+// that wp no longer uses directly once every rewrite is applied. Its pin
+// stays, with the edges of the composites that require it.
+func demoteDisplacedRoots(rec *Record, wp WorkflowPlan, merged map[string]map[string]string, displaced []*parserlock.ActionRef) error {
+	if len(displaced) == 0 {
+		return nil
+	}
+	var used []*parserlock.ActionRef
+	files := map[string]map[string]string{wp.Path: wp.Rewrites}
+	for _, path := range wp.SelfActionFiles {
+		files[path] = merged[path]
+	}
+	for path, rewrites := range files {
+		wf, err := workflowfile.Load(path)
+		if err != nil {
+			return fmt.Errorf("loading %s: %w", path, err)
+		}
+		content, _, err := wf.RewriteActionRefs(rewrites)
+		if err != nil {
+			return fmt.Errorf("rewriting %s: %w", path, err)
+		}
+		if wf, err = workflowfile.Parse(path, content); err != nil {
+			return fmt.Errorf("parsing %s: %w", path, err)
+		}
+		for _, v := range wf.UsesValues() {
+			if ref := parserlock.ParseActionRef(v); ref != nil {
+				used = append(used, ref)
+			}
+		}
+	}
+	for _, old := range displaced {
+		if slices.ContainsFunc(used, func(u *parserlock.ActionRef) bool { return sameRef(u.NWO(), u.Ref, old.NWO(), old.Ref) }) {
+			continue
+		}
+		for i := range rec.Entries {
+			e := &rec.Entries[i]
+			if !e.Direct || !slices.Contains(e.Workflows, wp.Path) ||
+				!sameRef(e.NWO, e.Ref, old.NWO(), old.Ref) {
+				continue
+			}
+			if len(e.Workflows) == 1 {
+				e.Direct = false
+				continue
+			}
+			e.Workflows = slices.DeleteFunc(slices.Clone(e.Workflows), func(w string) bool { return w == wp.Path })
+			demoted := *e
+			demoted.Workflows = []string{wp.Path}
+			demoted.Direct = false
+			rec.Entries = append(rec.Entries, demoted)
+		}
+	}
+	return nil
+}
+
+// sameRef reports whether two refs name one lockfile key: owner/repo and
+// full SHAs compare case-insensitively.
+func sameRef(nwoA, refA, nwoB, refB string) bool {
+	return strings.EqualFold(nwoA, nwoB) && lockfile.CanonicalRef(refA) == lockfile.CanonicalRef(refB)
+}
+
+func pinnedEntry(rec *Record, nwo, ref string) (Entry, bool) {
+	for _, e := range rec.Entries {
+		if isPinOrVerified(e.Resolution) && e.SHA != "" && sameRef(e.NWO, e.Ref, nwo, ref) {
+			return e, true
+		}
+	}
+	return Entry{}, false
+}
+
+// rewriteSelfActionFiles applies the merged rewrites to the in-repo action
+// files workflows reach via `$/…`, writing each file once.
+func rewriteSelfActionFiles(merged map[string]map[string]string) error {
 	for path, rewrites := range merged {
 		wf, err := workflowfile.Load(path)
 		if err != nil {

internal/workflowfile/rewrite.go

@@ -246,3 +246,14 @@ func stripTrailingComment(tail string) string {
 	}
 	return strings.TrimRight(tail[:idx], " \t")
 }
+
+// UsesValues returns every scalar `uses:` value in the file, as written.
+func (f *File) UsesValues() []string {
+	var out []string
+	walkYAMLNodes(&f.root, func(keyNode, valueNode *yaml.Node) {
+		if keyNode != nil && valueNode != nil && keyNode.Value == "uses" && valueNode.Kind == yaml.ScalarNode {
+			out = append(out, strings.TrimSpace(valueNode.Value))
+		}
+	})
+	return out
+}

internal/workflowfile/rewrite_test.go

@@ -235,3 +235,9 @@ func TestRewriteActionRefs_SubPathActions(t *testing.T) {
 	assert.Contains(t, out, "actions/cache@v5.0.5")
 	assert.Contains(t, out, "actions/checkout@v3")
 }
+
+func TestUsesValuesListsActionFileSteps(t *testing.T) {
+	f, err := Parse("action.yml", []byte("runs:\n  using: composite\n  steps:\n    - uses: octo/cache/restore@v2 # pinned\n    - run: echo\n    - uses: $/actions/setup\n"))
+	require.NoError(t, err)
+	assert.Equal(t, []string{"octo/cache/restore@v2", "$/actions/setup"}, f.UsesValues())
+}