Skip to content

Commit 4cf118d

Browse files
Lock refs another workflow rewrites in a shared $/ action (#231)
* Lock refs another workflow rewrites in a shared $/ action Workflows that reach the same in-repo $/ action share its rewrites. When one workflow kept a full-SHA ref (because a composite it uses declares that SHA) and another narrowed it to a tag, the shared action file was rewritten but the first workflow's lockfile list still named only the SHA, so --verify-local reported ref-changed right after a successful fix. Commit now records, under each workflow, the pins that other workflows' rewrites give its shared action files. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Demote roots a shared $/ rewrite leaves unused When another workflow's rewrite of a shared $/ action removed a workflow's only direct use of a ref, the ref stayed a workflow root and --verify-local reported it stale. Commit now drops it from the workflow's roots unless the workflow, after its own rewrites, or one of its $/ actions, after the merged rewrites, still uses it. The pin and the composite edge that requires it stay. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent de08e90 commit 4cf118d

4 files changed

Lines changed: 233 additions & 4 deletions

File tree

‎cmd/gh-actions-lock/run_test.go‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -817,6 +817,75 @@ func TestRun_SelfRepositoryLocalActionNamesItsParent(t *testing.T) {
817817
assert.Contains(t, stderr, "./helper (in $/actions/build), ./helper (in $/actions/test)")
818818
}
819819

820+
// TestRun_SharedSelfActionRewriteIsLockedForEveryWorkflow: ci.yml and
821+
// deploy.yml share $/.github/actions/restore, which uses
822+
// octo/cache/restore by SHA. ci.yml keeps that SHA, since octo/apt
823+
// declares it too; deploy.yml rewrites it to v2.0.0 in the shared file.
824+
// ci.yml then runs v2.0.0, so the lockfile must cover it. The SHA stays a
825+
// ci.yml root only while ci.yml still uses it directly; otherwise it is
826+
// locked only as octo/apt's child.
827+
func TestRun_SharedSelfActionRewriteIsLockedForEveryWorkflow(t *testing.T) {
828+
const cacheSHA, aptSHA = "cccccccccccccccccccccccccccccccccccccccc", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
829+
byCommit := "octo/cache/restore@" + strings.ToUpper(cacheSHA)
830+
for name, tc := range map[string]struct {
831+
steps string
832+
roots []string
833+
}{
834+
"used directly too": {
835+
steps: " - uses: " + byCommit + "\n",
836+
roots: []string{"octo/apt@v1.0.0", "octo/cache@" + cacheSHA, "octo/cache@v2.0.0"},
837+
},
838+
"used only in the shared action": {
839+
roots: []string{"octo/apt@v1.0.0", "octo/cache@v2.0.0"},
840+
},
841+
} {
842+
t.Run(name, func(t *testing.T) {
843+
manifests := map[string]string{"octo/apt": composite("octo/cache@" + cacheSHA)}
844+
commits := map[string]string{"octo/cache": cacheSHA, "octo/apt": aptSHA}
845+
reg := &httpmock.Registry{}
846+
registerLockedManifests(reg, manifests)
847+
for nwo, tags := range map[string]any{
848+
"octo/cache": httpmock.TagListResponse("v2.0.0", cacheSHA, "v2", cacheSHA),
849+
"octo/apt": httpmock.TagListResponse("v1.0.0", aptSHA, "v1", aptSHA),
850+
} {
851+
reg.RegisterReusable(httpmock.REST("GET", `repos/`+nwo+`/tags`), httpmock.JSONResponse(tags))
852+
reg.RegisterReusable(httpmock.REST("GET", `repos/`+nwo+`$`), httpmock.JSONResponse(map[string]any{"id": 1, "owner": map[string]any{"id": 1}, "default_branch": "main"}))
853+
reg.RegisterReusable(httpmock.REST("GET", `repos/`+nwo+`/branches`), httpmock.JSONResponse(httpmock.BranchListResponse("main", commits[nwo])))
854+
owner, repo, _ := strings.Cut(nwo, "/")
855+
httpmock.NoTagsNamed(reg, owner, repo)
856+
}
857+
registerLive(reg, manifests, commits)
858+
stubGraph(t, reg, nil, commits, nil)
859+
useDefaultTransport(t, reg)
860+
steps := "on: push\njobs:\n test:\n runs-on: ubuntu-latest\n steps:\n"
861+
ci := writeTempWorkflow(t, steps+" - uses: octo/apt@v1.0.0\n"+tc.steps+" - uses: $/.github/actions/restore\n")
862+
require.NoError(t, os.Mkdir(".git", 0o755))
863+
deploy := filepath.Join(".github", "workflows", "deploy.yml")
864+
require.NoError(t, os.WriteFile(deploy, []byte(steps+" - uses: $/.github/actions/restore\n"), 0o600))
865+
restore := filepath.Join(".github", "actions", "restore", "action.yml")
866+
require.NoError(t, os.MkdirAll(filepath.Dir(restore), 0o755))
867+
require.NoError(t, os.WriteFile(restore, []byte(composite(byCommit)), 0o600))
868+
869+
_, stderr, err := runCommandWithHTTP(t, reg, "--no-interactive")
870+
require.NoError(t, err, stderr)
871+
lock := readTempLockfilePins(t)
872+
action, err := os.ReadFile(restore)
873+
require.NoError(t, err)
874+
assert.Contains(t, string(action), "uses: octo/cache/restore@v2.0.0\n")
875+
want := "'" + filepath.ToSlash(ci) + "':\n"
876+
for _, root := range tc.roots {
877+
want += " - '" + root + "'\n"
878+
}
879+
assert.Contains(t, lock, want+"dependencies:")
880+
assert.Contains(t, lock, " uses:\n - 'octo/cache@"+cacheSHA+"'\n")
881+
for _, mode := range []string{"--verify-local", "--verify"} {
882+
_, stderr, err := runCommandWithHTTP(t, reg, mode)
883+
require.NoError(t, err, "%s: %s\n%s", mode, stderr, lock)
884+
}
885+
})
886+
}
887+
}
888+
820889
// TestRun_LocalActionUsingRemoteActionsBlocks: a `./…` composite in the
821890
// checkout that uses remote actions fails every mode, since the runner would
822891
// reject its unlocked uses; --ignore-local-actions (or the old

‎internal/pin/commit.go‎

Lines changed: 147 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"bytes"
55
"context"
66
"fmt"
7+
"maps"
78
"os"
89
"runtime"
910
"slices"
@@ -37,6 +38,10 @@ func Commit(ctx context.Context, rec *Record, store *lockfile.State, copts *Comm
3738
if err := validateRequiredRewrites(rec.Workflows); err != nil {
3839
return err
3940
}
41+
selfRewrites := mergeSelfActionRewrites(rec.Workflows)
42+
if err := adoptSharedRewrites(rec, selfRewrites); err != nil {
43+
return err
44+
}
4045

4146
// Phase 1: Update lockfile entries in memory. It runs before any file is
4247
// 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
7176

7277
// Action files can be shared by several workflows, so merge their
7378
// rewrites and write each file once, serially.
74-
if err := rewriteSelfActionFiles(rec.Workflows); err != nil {
79+
if err := rewriteSelfActionFiles(selfRewrites); err != nil {
7580
return err
7681
}
7782

@@ -167,9 +172,9 @@ func rewriteWorkflow(wp WorkflowPlan) error {
167172
return os.WriteFile(wp.Path, content, 0o644)
168173
}
169174

170-
// rewriteSelfActionFiles applies each workflow's rewrites to the in-repo
171-
// action files it reaches via `$/…`.
172-
func rewriteSelfActionFiles(plans []WorkflowPlan) error {
175+
// mergeSelfActionRewrites merges, per in-repo action file reached via `$/…`,
176+
// the rewrites of every workflow that reaches it.
177+
func mergeSelfActionRewrites(plans []WorkflowPlan) map[string]map[string]string {
173178
merged := make(map[string]map[string]string)
174179
for _, wp := range plans {
175180
if len(wp.Rewrites) == 0 {
@@ -184,6 +189,144 @@ func rewriteSelfActionFiles(plans []WorkflowPlan) error {
184189
}
185190
}
186191
}
192+
return merged
193+
}
194+
195+
// adoptSharedRewrites records, under each workflow, the pins another
196+
// workflow's rewrites give the action files they share. A workflow can keep a
197+
// ref another rewrites (a composite it uses declares that ref), and the
198+
// runner looks up each `uses:` in a `$/…` action as written, so the workflow
199+
// must lock both. Rewriting both to one ref would rename the composite's
200+
// declared edge instead.
201+
func adoptSharedRewrites(rec *Record, merged map[string]map[string]string) error {
202+
type pinKey struct{ wf, key string }
203+
adopted := map[pinKey]bool{}
204+
displaced := map[string][]*parserlock.ActionRef{}
205+
for _, wp := range rec.Workflows {
206+
for _, path := range wp.SelfActionFiles {
207+
foreign := map[string]string{}
208+
for oldUses, newUses := range merged[path] {
209+
if own, ok := workflowfile.LookupRewrite(oldUses, wp.Rewrites); !ok || own != newUses {
210+
foreign[oldUses] = newUses
211+
}
212+
}
213+
if len(foreign) == 0 {
214+
continue
215+
}
216+
wf, err := workflowfile.Load(path)
217+
if err != nil {
218+
return fmt.Errorf("loading %s: %w", path, err)
219+
}
220+
for _, oldUses := range slices.Sorted(maps.Keys(foreign)) {
221+
newUses := foreign[oldUses]
222+
if _, changed, err := wf.RewriteActionRefs(map[string]string{oldUses: newUses}); err != nil {
223+
return fmt.Errorf("rewriting %s: %w", path, err)
224+
} else if changed == 0 {
225+
continue
226+
}
227+
if old := parserlock.ParseActionRef(oldUses); old != nil {
228+
displaced[wp.Path] = append(displaced[wp.Path], old)
229+
}
230+
ref := parserlock.ParseActionRef(newUses)
231+
if ref == nil {
232+
continue
233+
}
234+
k := pinKey{wp.Path, strings.ToLower(ref.NWO()) + "@" + lockfile.CanonicalRef(ref.Ref)}
235+
if adopted[k] {
236+
continue
237+
}
238+
e, ok := pinnedEntry(rec, ref.NWO(), ref.Ref)
239+
if !ok {
240+
return fmt.Errorf("rewriting %s for %s: no planned pin for %s", path, wp.Path, ref.NWO()+"@"+ref.Ref)
241+
}
242+
adopted[k] = true
243+
e.Workflows = []string{wp.Path}
244+
e.Direct = true
245+
e.RequiredBy = nil
246+
rec.Entries = append(rec.Entries, e)
247+
}
248+
}
249+
}
250+
for _, wp := range rec.Workflows {
251+
if err := demoteDisplacedRoots(rec, wp, merged, displaced[wp.Path]); err != nil {
252+
return err
253+
}
254+
}
255+
return nil
256+
}
257+
258+
// demoteDisplacedRoots stops recording as a root of wp each displaced ref
259+
// that wp no longer uses directly once every rewrite is applied. Its pin
260+
// stays, with the edges of the composites that require it.
261+
func demoteDisplacedRoots(rec *Record, wp WorkflowPlan, merged map[string]map[string]string, displaced []*parserlock.ActionRef) error {
262+
if len(displaced) == 0 {
263+
return nil
264+
}
265+
var used []*parserlock.ActionRef
266+
files := map[string]map[string]string{wp.Path: wp.Rewrites}
267+
for _, path := range wp.SelfActionFiles {
268+
files[path] = merged[path]
269+
}
270+
for path, rewrites := range files {
271+
wf, err := workflowfile.Load(path)
272+
if err != nil {
273+
return fmt.Errorf("loading %s: %w", path, err)
274+
}
275+
content, _, err := wf.RewriteActionRefs(rewrites)
276+
if err != nil {
277+
return fmt.Errorf("rewriting %s: %w", path, err)
278+
}
279+
if wf, err = workflowfile.Parse(path, content); err != nil {
280+
return fmt.Errorf("parsing %s: %w", path, err)
281+
}
282+
for _, v := range wf.UsesValues() {
283+
if ref := parserlock.ParseActionRef(v); ref != nil {
284+
used = append(used, ref)
285+
}
286+
}
287+
}
288+
for _, old := range displaced {
289+
if slices.ContainsFunc(used, func(u *parserlock.ActionRef) bool { return sameRef(u.NWO(), u.Ref, old.NWO(), old.Ref) }) {
290+
continue
291+
}
292+
for i := range rec.Entries {
293+
e := &rec.Entries[i]
294+
if !e.Direct || !slices.Contains(e.Workflows, wp.Path) ||
295+
!sameRef(e.NWO, e.Ref, old.NWO(), old.Ref) {
296+
continue
297+
}
298+
if len(e.Workflows) == 1 {
299+
e.Direct = false
300+
continue
301+
}
302+
e.Workflows = slices.DeleteFunc(slices.Clone(e.Workflows), func(w string) bool { return w == wp.Path })
303+
demoted := *e
304+
demoted.Workflows = []string{wp.Path}
305+
demoted.Direct = false
306+
rec.Entries = append(rec.Entries, demoted)
307+
}
308+
}
309+
return nil
310+
}
311+
312+
// sameRef reports whether two refs name one lockfile key: owner/repo and
313+
// full SHAs compare case-insensitively.
314+
func sameRef(nwoA, refA, nwoB, refB string) bool {
315+
return strings.EqualFold(nwoA, nwoB) && lockfile.CanonicalRef(refA) == lockfile.CanonicalRef(refB)
316+
}
317+
318+
func pinnedEntry(rec *Record, nwo, ref string) (Entry, bool) {
319+
for _, e := range rec.Entries {
320+
if isPinOrVerified(e.Resolution) && e.SHA != "" && sameRef(e.NWO, e.Ref, nwo, ref) {
321+
return e, true
322+
}
323+
}
324+
return Entry{}, false
325+
}
326+
327+
// rewriteSelfActionFiles applies the merged rewrites to the in-repo action
328+
// files workflows reach via `$/…`, writing each file once.
329+
func rewriteSelfActionFiles(merged map[string]map[string]string) error {
187330
for path, rewrites := range merged {
188331
wf, err := workflowfile.Load(path)
189332
if err != nil {

‎internal/workflowfile/rewrite.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,3 +246,14 @@ func stripTrailingComment(tail string) string {
246246
}
247247
return strings.TrimRight(tail[:idx], " \t")
248248
}
249+
250+
// UsesValues returns every scalar `uses:` value in the file, as written.
251+
func (f *File) UsesValues() []string {
252+
var out []string
253+
walkYAMLNodes(&f.root, func(keyNode, valueNode *yaml.Node) {
254+
if keyNode != nil && valueNode != nil && keyNode.Value == "uses" && valueNode.Kind == yaml.ScalarNode {
255+
out = append(out, strings.TrimSpace(valueNode.Value))
256+
}
257+
})
258+
return out
259+
}

‎internal/workflowfile/rewrite_test.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,3 +235,9 @@ func TestRewriteActionRefs_SubPathActions(t *testing.T) {
235235
assert.Contains(t, out, "actions/cache@v5.0.5")
236236
assert.Contains(t, out, "actions/checkout@v3")
237237
}
238+
239+
func TestUsesValuesListsActionFileSteps(t *testing.T) {
240+
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"))
241+
require.NoError(t, err)
242+
assert.Equal(t, []string{"octo/cache/restore@v2", "$/actions/setup"}, f.UsesValues())
243+
}

0 commit comments

Comments
 (0)