From 532f5222a1c7587225b13f19103d2aeeba339520 Mon Sep 17 00:00:00 2001 From: kuma0128 Date: Wed, 14 Oct 2026 13:21:46 +0900 Subject: [PATCH 1/5] Guard local action migration after checkout of a custom ref --- README.md | 9 ++ cmd/gh-actions-lock/migrate_test.go | 41 ++++++- internal/workflowfile/rewrite.go | 61 +++++++++- internal/workflowfile/rewrite_test.go | 130 +++++++++++++++++++++ internal/workflowfile/workflowfile_test.go | 50 ++++++++ 5 files changed, 285 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index d45a6c4a..2ce47303 100644 --- a/README.md +++ b/README.md @@ -117,6 +117,15 @@ leave `./…` refs untouched on fix runs, opt out with gh actions-lock --no-migrate-local-actions ``` +Migration stops before writing files if a local action follows an +`actions/checkout` step with a non-empty `ref` other than `${{ github.sha }}` +in the same job or composite action. A custom ref can make `./…` load different +code from `$/…`. The error identifies the checkout ref and its line. Review +the checkout and action references together, or use `--no-migrate-local-actions` +to preserve the local references; workflows that keep `./…` remain ineligible +for onboarding. This check covers explicit checkout steps, not workspace +changes made by scripts or other actions. + ## How it works A repo gets a lockfile (located at [`.github/workflows/actions.lock`](https://github.com/github/gh-actions-lock/blob/main/.github/workflows/actions.lock)) and workflows are onboarded to the lockfile on a per-workflow basis. diff --git a/cmd/gh-actions-lock/migrate_test.go b/cmd/gh-actions-lock/migrate_test.go index 3c0096b3..b59a6482 100644 --- a/cmd/gh-actions-lock/migrate_test.go +++ b/cmd/gh-actions-lock/migrate_test.go @@ -11,11 +11,50 @@ import ( "github.com/stretchr/testify/require" ) +func TestMigrateLocalActions_CustomCheckoutRefWritesNothing(t *testing.T) { + for _, args := range [][]string{nil, {"--json"}} { + t.Run(strings.Join(args, " "), func(t *testing.T) { + t.Chdir(t.TempDir()) + require.NoError(t, os.Mkdir(".git", 0o755)) + files := map[string]string{ + ".github/workflows/first.yml": "jobs:\n build:\n steps:\n - uses: ./.github/actions/local\n", + ".github/workflows/second.yml": `jobs: + refresh: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + ref: ${{ env.TARGET_BRANCH }} + - uses: ./.github/actions/local +`, + ".github/actions/local/action.yml": "runs:\n using: composite\n steps:\n - uses: ./.github/actions/helper\n", + ".github/actions/helper/action.yml": "runs:\n using: composite\n steps:\n - run: echo hello\n shell: bash\n", + ".github/workflows/actions.lock": "version: 'v0.0.3'\nworkflows: {}\ndependencies: {}\n", + } + for path, content := range files { + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o755)) + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) + } + before := snapshotWorkflows(t) + transport := &requestCountingTransport{} + + _, _, err := runCommandWithHTTP(t, transport, args...) + + require.Error(t, err) + assert.Contains(t, err.Error(), "second.yml") + assert.Contains(t, err.Error(), "${{ env.TARGET_BRANCH }}") + assert.Contains(t, err.Error(), "--no-migrate-local-actions") + assert.Equal(t, before, snapshotWorkflows(t)) + assert.Zero(t, transport.calls.Load(), "unsafe migration stops before resolving dependencies") + }) + } +} + // TestMigrateLocalActions_CompositeActionFiles verifies that migration // rewrites `./…` not just in workflow files but also inside in-repo composite // action definitions (action.yml), including ones nested a few directories deep. // A `./…` whose target does not resolve to an in-repo action file is left -// untouched — that is the equivalence guard that keeps `$/` a safe replacement. +// untouched. func TestMigrateLocalActions_CompositeActionFiles(t *testing.T) { dir := t.TempDir() write := func(rel, content string) { diff --git a/internal/workflowfile/rewrite.go b/internal/workflowfile/rewrite.go index 1d24a21e..6b81e482 100644 --- a/internal/workflowfile/rewrite.go +++ b/internal/workflowfile/rewrite.go @@ -136,11 +136,10 @@ func (f *File) ValidateRequiredActionRefRewrites(replacements map[string]string) // MigrateLocalActionsToSelfRepository rewrites same-repo `./…` composite action // references to the inherently-pinned `$/…` form. Only local paths that -// resolve to an in-repo action file are rewritten — that in-repo existence is -// the same-repo equivalence guard that makes `$/` a safe replacement for -// `./`. Local reusable workflows are never candidates (ExtractActionRefs -// excludes them). Returns the new content and the number of `uses:` lines -// changed. +// resolve to an in-repo action file are candidates. A preceding checkout of a +// custom ref prevents migration. Local reusable workflows are never candidates +// (ExtractActionRefs excludes them). Returns the new content and the number of +// `uses:` lines changed. func (f *File) MigrateLocalActionsToSelfRepository() ([]byte, int, error) { scan := f.ExtractActionRefs() if err := f.validateSelfRepositoryMigration(scan); err != nil { @@ -164,10 +163,62 @@ func (f *File) MigrateLocalActionsToSelfRepository() ([]byte, int, error) { if len(replacements) == 0 { return append([]byte(nil), f.Content...), 0, nil } + if err := f.validateCheckoutRefsForMigration(replacements); err != nil { + return append([]byte(nil), f.Content...), 0, err + } return f.RewriteActionRefs(replacements) } +func (f *File) validateCheckoutRefsForMigration(replacements map[string]string) error { + root := resolveAlias(documentRoot(&f.root)) + if err := validateStepCheckoutRefs(mapValue(root, "runs"), replacements); err != nil { + return err + } + jobs := mapValue(root, "jobs") + if jobs == nil || jobs.Kind != yaml.MappingNode { + return nil + } + for i := 1; i < len(jobs.Content); i += 2 { + if err := validateStepCheckoutRefs(resolveAlias(jobs.Content[i]), replacements); err != nil { + return err + } + } + return nil +} + +func validateStepCheckoutRefs(owner *yaml.Node, replacements map[string]string) error { + steps := mapValue(owner, "steps") + if steps == nil || steps.Kind != yaml.SequenceNode { + return nil + } + var customRef *yaml.Node + for _, stepNode := range steps.Content { + step := resolveAlias(stepNode) + uses := strings.TrimSpace(scalarValue(mapValue(step, "uses"))) + action, _, hasRef := strings.Cut(uses, "@") + if hasRef && strings.EqualFold(action, "actions/checkout") { + ref := mapValue(mapValue(step, "with"), "ref") + value := strings.TrimSpace(scalarValue(ref)) + if value != "" && !isRunningCommitRef(value) { + customRef = ref + } + // Keep earlier custom refs: a later checkout can be conditional. + } + if replacement, ok := replacements[uses]; ok && customRef != nil { + return fmt.Errorf("cannot migrate %s to %s: preceding actions/checkout uses ref %q at line %d; $/ resolves to the running commit, not the checked-out ref; use --no-migrate-local-actions to keep local references", uses, replacement, customRef.Value, customRef.Line) + } + } + return nil +} + +func isRunningCommitRef(value string) bool { + if !strings.HasPrefix(value, "${{") || !strings.HasSuffix(value, "}}") { + return false + } + return strings.TrimSpace(value[3:len(value)-2]) == "github.sha" +} + func (f *File) validateSelfRepositoryMigration(scan RefScan) error { if len(scan.SelfRepositoryRefErrs) > 0 { return fmt.Errorf("invalid self repository reference %q", scan.SelfRepositoryRefErrs[0]) diff --git a/internal/workflowfile/rewrite_test.go b/internal/workflowfile/rewrite_test.go index 1286eaec..e6eb1326 100644 --- a/internal/workflowfile/rewrite_test.go +++ b/internal/workflowfile/rewrite_test.go @@ -9,6 +9,136 @@ import ( "github.com/stretchr/testify/require" ) +func TestValidateCheckoutRefsForMigration(t *testing.T) { + for _, tt := range []struct { + name string + content string + blocked bool + }{ + { + name: "checkout in another job does not block migration", + content: `jobs: + other: + steps: + - uses: actions/checkout@v4 + with: {ref: other} + build: + steps: + - uses: ./local +`, + }, + { + name: "checkout after the action does not block migration", + content: `jobs: + build: + steps: + - uses: ./local + - uses: actions/checkout@v4 + with: {ref: other} +`, + }, + { + name: "one unsafe use blocks a shared replacement", + content: `jobs: + first: + steps: + - uses: ./local + second: + steps: + - uses: actions/checkout@v4 + with: {ref: other} + - uses: ./local +`, + blocked: true, + }, + { + name: "checkout in composite steps blocks migration", + content: `runs: + using: composite + steps: + - uses: actions/checkout@v4 + with: {ref: other} + - uses: ./local +`, + blocked: true, + }, + { + name: "aliased checkout step blocks migration", + content: `jobs: + other: + steps: + - &checkout + uses: actions/checkout@v4 + with: {ref: other} + build: + steps: + - *checkout + - uses: ./local +`, + blocked: true, + }, + { + name: "aliased inputs block migration", + content: `env: + inputs: &inputs {ref: other} +jobs: + build: + steps: + - uses: actions/checkout@v4 + with: *inputs + - uses: ./local +`, + blocked: true, + }, + { + name: "conditional checkout cannot clear an earlier custom ref", + content: `jobs: + build: + steps: + - uses: actions/checkout@v4 + with: {ref: other} + - uses: actions/checkout@v4 + if: inputs.reset + - uses: ./local +`, + blocked: true, + }, + { + name: "checkout name is case insensitive and can be SHA pinned", + content: `jobs: + build: + steps: + - uses: Actions/Checkout@0123456789012345678901234567890123456789 + with: {ref: other} + - uses: ./local +`, + blocked: true, + }, + { + name: "noncandidate local reference is left to diagnosis", + content: `jobs: + build: + steps: + - uses: actions/checkout@v4 + with: {ref: other} + - uses: ./missing +`, + }, + } { + t.Run(tt.name, func(t *testing.T) { + f, err := Parse("ci.yml", []byte(tt.content)) + require.NoError(t, err) + err = f.validateCheckoutRefsForMigration(map[string]string{"./local": "$/local"}) + if tt.blocked { + require.Error(t, err) + assert.Contains(t, err.Error(), "preceding actions/checkout") + return + } + require.NoError(t, err) + }) + } +} + func TestSubpathRewriteLookup(t *testing.T) { replacements := map[string]string{ "actions/cache@27d5ce7": "actions/cache@v5.0.5", diff --git a/internal/workflowfile/workflowfile_test.go b/internal/workflowfile/workflowfile_test.go index fe6251ef..4c1cdc94 100644 --- a/internal/workflowfile/workflowfile_test.go +++ b/internal/workflowfile/workflowfile_test.go @@ -222,6 +222,56 @@ func TestMigrateLocalActionsToSelfRepository_NoLocalPaths(t *testing.T) { assert.Equal(t, content, out) } +func TestMigrateLocalActionsToSelfRepository_CheckoutRef(t *testing.T) { + repoRoot := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(repoRoot, ".git"), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Join(repoRoot, "local-action"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(repoRoot, "local-action", "action.yml"), + []byte("runs:\n using: composite\n"), 0o644)) + + for _, tt := range []struct { + name string + ref string + blocked bool + }{ + {name: "default ref"}, + {name: "empty ref", ref: "''"}, + {name: "running commit", ref: "${{ github.sha }}"}, + {name: "running commit without spaces", ref: "${{github.sha}}"}, + {name: "branch", ref: "develop2", blocked: true}, + {name: "tag", ref: "v1.0.0", blocked: true}, + {name: "literal commit", ref: "0123456789012345678901234567890123456789", blocked: true}, + {name: "workflow input", ref: "${{ inputs.ref || github.ref }}", blocked: true}, + {name: "environment variable", ref: "${{ env.TARGET_BRANCH }}", blocked: true}, + {name: "pull request head", ref: "${{ github.event.pull_request.head.sha }}", blocked: true}, + {name: "running branch can move", ref: "${{ github.ref }}", blocked: true}, + } { + t.Run(tt.name, func(t *testing.T) { + content := "jobs:\n build:\n steps:\n - uses: actions/checkout@v4\n" + if tt.ref != "" { + content += " with:\n ref: " + tt.ref + "\n" + } + content += " - uses: ./local-action\n" + f, err := Parse(filepath.Join(repoRoot, ".github", "workflows", "ci.yml"), []byte(content)) + require.NoError(t, err) + + out, changed, err := f.MigrateLocalActionsToSelfRepository() + if tt.blocked { + require.Error(t, err) + assert.Contains(t, err.Error(), "actions/checkout") + assert.Contains(t, err.Error(), tt.ref) + assert.Contains(t, err.Error(), "--no-migrate-local-actions") + assert.Zero(t, changed) + assert.Equal(t, content, string(out)) + return + } + require.NoError(t, err) + assert.Equal(t, 1, changed) + assert.Contains(t, string(out), "uses: $/local-action") + }) + } +} + func TestMigrateLocalActionsToSelfRepository_RejectsStructuralErrors(t *testing.T) { repoRoot := t.TempDir() require.NoError(t, os.Mkdir(filepath.Join(repoRoot, ".git"), 0o755)) From 70c32f87dde175d8ec1a526b7c09b473ba60371a Mon Sep 17 00:00:00 2001 From: kuma0128 Date: Wed, 14 Oct 2026 13:40:18 +0900 Subject: [PATCH 2/5] workflowfile: propagate checkout context through composite migrations --- README.md | 14 +- cmd/gh-actions-lock/migrate_test.go | 4 +- cmd/gh-actions-lock/run.go | 25 +++- internal/workflowfile/checkout.go | 192 +++++++++++++++++++++++++ internal/workflowfile/checkout_test.go | 173 ++++++++++++++++++++++ internal/workflowfile/rewrite.go | 66 +-------- internal/workflowfile/workflowfile.go | 18 ++- 7 files changed, 419 insertions(+), 73 deletions(-) create mode 100644 internal/workflowfile/checkout.go create mode 100644 internal/workflowfile/checkout_test.go diff --git a/README.md b/README.md index 2ce47303..86ac4d55 100644 --- a/README.md +++ b/README.md @@ -119,12 +119,16 @@ gh actions-lock --no-migrate-local-actions Migration stops before writing files if a local action follows an `actions/checkout` step with a non-empty `ref` other than `${{ github.sha }}` -in the same job or composite action. A custom ref can make `./…` load different -code from `$/…`. The error identifies the checkout ref and its line. Review -the checkout and action references together, or use `--no-migrate-local-actions` +in the same job, including checkouts and local references reached through +in-repo composite actions. Checkout state follows nested `./…` and `$/…` +calls and carries back to subsequent caller steps. Since composite definitions +are migrated repository-wide, their callers are checked even in workflows +outside a scoped run. A custom ref can make `./…` load different code from +`$/…`. The error identifies the action file and the checkout's file and line. +Review the checkout and action references together, or use `--no-migrate-local-actions` to preserve the local references; workflows that keep `./…` remain ineligible -for onboarding. This check covers explicit checkout steps, not workspace -changes made by scripts or other actions. +for onboarding. This check covers explicit checkout steps in workflows and +in-repo composite actions, not workspace changes made by scripts or remote actions. ## How it works diff --git a/cmd/gh-actions-lock/migrate_test.go b/cmd/gh-actions-lock/migrate_test.go index b59a6482..8c4f0a15 100644 --- a/cmd/gh-actions-lock/migrate_test.go +++ b/cmd/gh-actions-lock/migrate_test.go @@ -12,7 +12,7 @@ import ( ) func TestMigrateLocalActions_CustomCheckoutRefWritesNothing(t *testing.T) { - for _, args := range [][]string{nil, {"--json"}} { + for _, args := range [][]string{nil, {"--json"}, {".github/workflows/first.yml"}} { t.Run(strings.Join(args, " "), func(t *testing.T) { t.Chdir(t.TempDir()) require.NoError(t, os.Mkdir(".git", 0o755)) @@ -25,7 +25,7 @@ func TestMigrateLocalActions_CustomCheckoutRefWritesNothing(t *testing.T) { - uses: actions/checkout@v4 with: ref: ${{ env.TARGET_BRANCH }} - - uses: ./.github/actions/local + - uses: $/.github/actions/local `, ".github/actions/local/action.yml": "runs:\n using: composite\n steps:\n - uses: ./.github/actions/helper\n", ".github/actions/helper/action.yml": "runs:\n using: composite\n steps:\n - run: echo hello\n shell: bash\n", diff --git a/cmd/gh-actions-lock/run.go b/cmd/gh-actions-lock/run.go index ebd6d51f..a2efb107 100644 --- a/cmd/gh-actions-lock/run.go +++ b/cmd/gh-actions-lock/run.go @@ -634,6 +634,7 @@ func migrateLocalActions(paths []string) (int, func(), error) { } } files := append([]string(nil), paths...) + var repoRoot string // Also sweep in-repo composite action files. The repo root is derived from // a workflow path (they live under .github/workflows/); onboarding only @@ -644,6 +645,7 @@ func migrateLocalActions(paths []string) (int, func(), error) { return 0, undo, err } if root != "" { + repoRoot = root actionFiles, err := workflowfile.DiscoverCompositeActionFiles(root) if err != nil { return 0, undo, err @@ -691,8 +693,27 @@ func migrateLocalActions(paths []string) (int, func(), error) { migrations = append(migrations, plannedMigration{path: path, original: original, content: content, changed: changed}) } - // Do not write anything until every candidate has passed structural and - // repository-boundary validation. + // Composite definitions are shared even when the command names only one + // workflow. Check all callers before writing changes to those definitions. + var migrationPaths []string + compositeChanged := false + for _, plan := range migrations { + migrationPaths = append(migrationPaths, plan.path) + name := filepath.Base(plan.path) + compositeChanged = compositeChanged || name == "action.yml" || name == "action.yaml" + } + if compositeChanged && repoRoot != "" { + workflowPaths, err := workflowfile.DiscoverWorkflowsIn(filepath.Join(repoRoot, ".github", "workflows")) + if err != nil { + return 0, undo, err + } + if err := workflowfile.ValidateLocalActionMigrations(workflowPaths, migrationPaths); err != nil { + return 0, undo, err + } + } + + // Do not write anything until every candidate and its callers have passed + // validation. total := 0 for _, plan := range migrations { written = append(written, plan) diff --git a/internal/workflowfile/checkout.go b/internal/workflowfile/checkout.go new file mode 100644 index 00000000..bb48a984 --- /dev/null +++ b/internal/workflowfile/checkout.go @@ -0,0 +1,192 @@ +package workflowfile + +import ( + "fmt" + "path/filepath" + "strings" + + "gopkg.in/yaml.v3" +) + +// ValidateLocalActionMigrations checks caller checkout context for the files +// being migrated. workflowPaths must include every workflow in the repository: +// shared composite actions can also be called by workflows outside a scoped run. +func ValidateLocalActionMigrations(workflowPaths, migrationPaths []string) error { + migrations := make(map[string]bool, len(migrationPaths)) + for _, path := range migrationPaths { + absPath, err := filepath.Abs(path) + if err != nil { + return err + } + resolvedPath, err := filepath.EvalSymlinks(absPath) + if err != nil { + return err + } + migrations[resolvedPath] = true + } + for _, path := range workflowPaths { + absPath, err := filepath.Abs(path) + if err != nil { + return err + } + root, err := findRepoRoot(absPath) + if err != nil { + return err + } + if err := ValidatePathWithinRoot(root, absPath); err != nil { + return err + } + resolvedPath, err := filepath.EvalSymlinks(absPath) + if err != nil { + return err + } + f, err := Load(resolvedPath) + if err != nil { + return err + } + v := checkoutValidator{repoRoot: root, migrations: migrations, files: make(map[string]*File)} + if err := v.validate(f, v.replacements(f)); err != nil { + return fmt.Errorf("checking checkout context in %s: %w", path, err) + } + } + return nil +} + +type checkoutRef struct { + path string + node *yaml.Node +} + +type checkoutValidator struct { + repoRoot string + migrations map[string]bool // nil means all local action references are candidates + files map[string]*File + active map[checkoutContext]bool + results map[checkoutContext]*checkoutRef +} + +type checkoutContext struct { + steps *yaml.Node + ref *yaml.Node +} + +func (f *File) validateCheckoutRefsForMigration(replacements map[string]string) error { + root, err := findRepoRoot(f.Path) + if err != nil { + return err + } + v := checkoutValidator{repoRoot: root, files: make(map[string]*File)} + return v.validate(f, replacements) +} + +func (v *checkoutValidator) replacements(f *File) map[string]string { + if v.migrations != nil && !v.migrations[f.Path] { + return nil + } + return f.localActionReplacements(v.repoRoot) +} + +func (v *checkoutValidator) validate(f *File, replacements map[string]string) error { + root := resolveAlias(documentRoot(&f.root)) + if _, err := v.steps(f, mapValue(root, "runs"), replacements, nil, 0); err != nil { + return err + } + jobs := mapValue(root, "jobs") + if jobs == nil || jobs.Kind != yaml.MappingNode { + return nil + } + for i := 1; i < len(jobs.Content); i += 2 { + if _, err := v.steps(f, resolveAlias(jobs.Content[i]), replacements, nil, 0); err != nil { + return err + } + } + return nil +} + +func (v *checkoutValidator) steps(f *File, owner *yaml.Node, replacements map[string]string, customRef *checkoutRef, depth int) (*checkoutRef, error) { + if depth > maxYAMLWalkDepth { + return nil, fmt.Errorf("checking checkout context in %s: composite action nesting exceeds %d", f.Path, maxYAMLWalkDepth) + } + steps := mapValue(owner, "steps") + if steps == nil || steps.Kind != yaml.SequenceNode { + return customRef, nil + } + context := checkoutContext{steps: steps} + if customRef != nil { + context.ref = customRef.node + } + if result, ok := v.results[context]; ok { + return result, nil + } + if v.active[context] { + return nil, fmt.Errorf("checking checkout context in %s: composite action cycle", f.Path) + } + if v.active == nil { + v.active = make(map[checkoutContext]bool) + v.results = make(map[checkoutContext]*checkoutRef) + } + v.active[context] = true + defer delete(v.active, context) + for _, stepNode := range steps.Content { + step := resolveAlias(stepNode) + uses := strings.TrimSpace(scalarValue(mapValue(step, "uses"))) + action, _, hasRef := strings.Cut(uses, "@") + if hasRef && strings.EqualFold(action, "actions/checkout") { + ref := mapValue(mapValue(step, "with"), "ref") + value := strings.TrimSpace(scalarValue(ref)) + if value != "" && !isRunningCommitRef(value) { + customRef = &checkoutRef{path: f.Path, node: ref} + } + // Keep earlier custom refs: a later checkout can be conditional. + } + if replacement, ok := replacements[uses]; ok && customRef != nil { + return nil, fmt.Errorf("cannot migrate %s to %s in %s: preceding actions/checkout uses ref %q at %s:%d; $/ resolves to the running commit, not the checked-out ref; use --no-migrate-local-actions to keep local references", uses, replacement, f.Path, customRef.node.Value, customRef.path, customRef.node.Line) + } + var localPath string + switch { + case IsSelfRepositoryAction(uses): + localPath = "./" + strings.TrimPrefix(uses, selfRepositoryPrefix) + case strings.HasPrefix(uses, "./"): + localPath = uses + default: + continue + } + if v.repoRoot == "" || !localActionExists(v.repoRoot, localPath) { + continue + } + actionDir := filepath.Join(v.repoRoot, strings.TrimPrefix(localPath, "./")) + child, ok := v.files[actionDir] + if !ok { + content, path, err := readActionFile(v.repoRoot, actionDir) + if err != nil { + return nil, err + } + path, err = filepath.EvalSymlinks(path) + if err != nil { + return nil, err + } + child, err = Parse(path, content) + if err != nil { + return nil, err + } + v.files[actionDir] = child + } + // A composite inherits the caller's checkout state and can change the + // workspace seen by subsequent steps, even if its invocation is conditional. + runs := mapValue(documentRoot(&child.root), "runs") + nextRef, err := v.steps(child, runs, v.replacements(child), customRef, depth+1) + if err != nil { + return nil, err + } + customRef = nextRef + } + v.results[context] = customRef + return customRef, nil +} + +func isRunningCommitRef(value string) bool { + if !strings.HasPrefix(value, "${{") || !strings.HasSuffix(value, "}}") { + return false + } + return strings.TrimSpace(value[3:len(value)-2]) == "github.sha" +} diff --git a/internal/workflowfile/checkout_test.go b/internal/workflowfile/checkout_test.go new file mode 100644 index 00000000..1617f1ca --- /dev/null +++ b/internal/workflowfile/checkout_test.go @@ -0,0 +1,173 @@ +package workflowfile + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMigrateLocalActionsToSelfRepository_CheckoutContext(t *testing.T) { + for _, tt := range []struct { + name string + workflow string + outer string + blocked bool + }{ + { + name: "custom ref reaches an already migrated composite", + workflow: " - uses: actions/checkout@v4\n with: {ref: other}\n - uses: $/outer\n", + outer: " - uses: ./leaf\n", + blocked: true, + }, + { + name: "custom ref reaches a nested composite", + workflow: " - uses: actions/checkout@v4\n with: {ref: other}\n - uses: $/outer\n", + outer: " - uses: $/middle\n", + blocked: true, + }, + { + name: "a safe call does not hide a later unsafe call", + workflow: " - uses: $/outer\n - uses: actions/checkout@v4\n with: {ref: other}\n - uses: $/outer\n", + outer: " - uses: ./leaf\n", + blocked: true, + }, + { + name: "checkout in a composite reaches subsequent caller steps", + workflow: " - uses: $/outer\n - uses: ./leaf\n", + outer: " - uses: actions/checkout@v4\n with: {ref: other}\n", + blocked: true, + }, + { + name: "checkout in a local composite reaches subsequent caller steps", + workflow: " - uses: ./outer\n - uses: ./leaf\n", + outer: " - uses: actions/checkout@v4\n with: {ref: other}\n", + blocked: true, + }, + { + name: "checkout in a composite reaches a sibling composite", + workflow: " - uses: $/outer\n - uses: $/middle\n", + outer: " - uses: actions/checkout@v4\n with: {ref: other}\n", + blocked: true, + }, + { + name: "conditional composite checkout remains blocking", + workflow: " - uses: $/outer\n if: github.event_name == 'push'\n - uses: ./leaf\n", + outer: " - uses: actions/checkout@v4\n with: {ref: other}\n", + blocked: true, + }, + { + name: "composite checkout does not affect another job", + workflow: " - uses: $/outer\n second:\n steps:\n - uses: $/middle\n", + outer: " - uses: actions/checkout@v4\n with: {ref: other}\n", + }, + { + name: "checkout after a nested local action allows migration", + workflow: " - uses: $/outer\n", + outer: " - uses: ./leaf\n - uses: actions/checkout@v4\n with: {ref: other}\n", + }, + { + name: "running commit checkout allows nested migration", + workflow: " - uses: actions/checkout@v4\n with: {ref: '${{ github.sha }}'}\n - uses: $/outer\n", + outer: " - uses: $/middle\n", + }, + { + name: "custom checkout with only self repository references needs no migration", + workflow: " - uses: actions/checkout@v4\n with: {ref: other}\n - uses: $/outer\n", + outer: " - uses: $/leaf\n", + }, + } { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(dir, ".git"), 0o755)) + workflow := "jobs:\n build:\n steps:\n" + tt.workflow + for path, content := range map[string]string{ + ".github/workflows/ci.yml": workflow, + "outer/action.yml": "runs:\n using: composite\n steps:\n" + tt.outer, + "middle/action.yaml": "runs:\n using: composite\n steps:\n - uses: ./leaf\n", + "leaf/action.yml": "runs:\n using: node24\n main: index.js\n", + } { + full := filepath.Join(dir, path) + require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o755)) + require.NoError(t, os.WriteFile(full, []byte(content), 0o644)) + } + f, err := Load(filepath.Join(dir, ".github/workflows/ci.yml")) + require.NoError(t, err) + content, changed, err := f.MigrateLocalActionsToSelfRepository() + if tt.blocked { + require.Error(t, err) + assert.Contains(t, err.Error(), "preceding actions/checkout") + assert.Contains(t, err.Error(), `ref "other"`) + assert.Contains(t, err.Error(), ".yml:") + assert.Zero(t, changed) + assert.Equal(t, workflow, string(content)) + return + } + require.NoError(t, err) + }) + } +} + +func TestValidateLocalActionMigrations_ScopedRun(t *testing.T) { + for _, tt := range []struct { + name string + uses string + blocked bool + }{ + {name: "unchanged workflow reference does not block a scoped run", uses: "./leaf"}, + {name: "shared composite reference blocks a scoped run", uses: "$/outer", blocked: true}, + {name: "symlinked composite reference blocks a scoped run", uses: "$/alias", blocked: true}, + } { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(dir, ".git"), 0o755)) + for path, content := range map[string]string{ + ".github/workflows/outside.yml": "jobs:\n build:\n steps:\n - uses: actions/checkout@v4\n with: {ref: other}\n - uses: " + tt.uses + "\n", + "outer/action.yml": "runs:\n using: composite\n steps:\n - uses: ./leaf\n", + "leaf/action.yml": "runs:\n using: node24\n main: index.js\n", + } { + full := filepath.Join(dir, path) + require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o755)) + require.NoError(t, os.WriteFile(full, []byte(content), 0o644)) + } + if tt.uses == "$/alias" { + if err := os.Symlink(filepath.Join(dir, "outer"), filepath.Join(dir, "alias")); err != nil { + t.Skipf("creating symlink: %v", err) + } + } + err := ValidateLocalActionMigrations( + []string{filepath.Join(dir, ".github/workflows/outside.yml")}, + []string{filepath.Join(dir, "outer/action.yml")}, + ) + if tt.blocked { + require.Error(t, err) + assert.Contains(t, err.Error(), "outer/action.yml") + assert.Contains(t, err.Error(), "outside.yml:5") + return + } + require.NoError(t, err) + }) + } +} + +func TestMigrateLocalActionsToSelfRepository_CompositeCycle(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(dir, ".git"), 0o755)) + for path, content := range map[string]string{ + "ci.yml": "jobs:\n build:\n steps:\n - uses: $/outer\n", + "outer/action.yml": "runs:\n using: composite\n steps:\n - uses: $/inner\n", + "inner/action.yml": "runs:\n using: composite\n steps:\n - uses: $/outer\n", + } { + full := filepath.Join(dir, path) + require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o755)) + require.NoError(t, os.WriteFile(full, []byte(content), 0o644)) + } + f, err := Load(filepath.Join(dir, "ci.yml")) + require.NoError(t, err) + _, changed, err := f.MigrateLocalActionsToSelfRepository() + require.Error(t, err) + assert.Contains(t, err.Error(), "composite action cycle") + assert.Zero(t, changed) +} diff --git a/internal/workflowfile/rewrite.go b/internal/workflowfile/rewrite.go index 6b81e482..2338e9e2 100644 --- a/internal/workflowfile/rewrite.go +++ b/internal/workflowfile/rewrite.go @@ -145,24 +145,11 @@ func (f *File) MigrateLocalActionsToSelfRepository() ([]byte, int, error) { if err := f.validateSelfRepositoryMigration(scan); err != nil { return append([]byte(nil), f.Content...), 0, err } - if len(scan.LocalPaths) == 0 { - return append([]byte(nil), f.Content...), 0, nil - } - repoRoot, err := findRepoRoot(f.Path) if err != nil { return append([]byte(nil), f.Content...), 0, err } - replacements := make(map[string]string, len(scan.LocalPaths)) - for _, localPath := range scan.LocalPaths { - if !localActionExists(repoRoot, localPath) { - continue - } - replacements[localPath] = selfRepositoryPrefix + strings.TrimPrefix(localPath, "./") - } - if len(replacements) == 0 { - return append([]byte(nil), f.Content...), 0, nil - } + replacements := f.localActionReplacements(repoRoot) if err := f.validateCheckoutRefsForMigration(replacements); err != nil { return append([]byte(nil), f.Content...), 0, err } @@ -170,53 +157,14 @@ func (f *File) MigrateLocalActionsToSelfRepository() ([]byte, int, error) { return f.RewriteActionRefs(replacements) } -func (f *File) validateCheckoutRefsForMigration(replacements map[string]string) error { - root := resolveAlias(documentRoot(&f.root)) - if err := validateStepCheckoutRefs(mapValue(root, "runs"), replacements); err != nil { - return err - } - jobs := mapValue(root, "jobs") - if jobs == nil || jobs.Kind != yaml.MappingNode { - return nil - } - for i := 1; i < len(jobs.Content); i += 2 { - if err := validateStepCheckoutRefs(resolveAlias(jobs.Content[i]), replacements); err != nil { - return err - } - } - return nil -} - -func validateStepCheckoutRefs(owner *yaml.Node, replacements map[string]string) error { - steps := mapValue(owner, "steps") - if steps == nil || steps.Kind != yaml.SequenceNode { - return nil - } - var customRef *yaml.Node - for _, stepNode := range steps.Content { - step := resolveAlias(stepNode) - uses := strings.TrimSpace(scalarValue(mapValue(step, "uses"))) - action, _, hasRef := strings.Cut(uses, "@") - if hasRef && strings.EqualFold(action, "actions/checkout") { - ref := mapValue(mapValue(step, "with"), "ref") - value := strings.TrimSpace(scalarValue(ref)) - if value != "" && !isRunningCommitRef(value) { - customRef = ref - } - // Keep earlier custom refs: a later checkout can be conditional. +func (f *File) localActionReplacements(repoRoot string) map[string]string { + replacements := make(map[string]string) + for _, localPath := range f.ExtractActionRefs().LocalPaths { + if localActionExists(repoRoot, localPath) { + replacements[localPath] = selfRepositoryPrefix + strings.TrimPrefix(localPath, "./") } - if replacement, ok := replacements[uses]; ok && customRef != nil { - return fmt.Errorf("cannot migrate %s to %s: preceding actions/checkout uses ref %q at line %d; $/ resolves to the running commit, not the checked-out ref; use --no-migrate-local-actions to keep local references", uses, replacement, customRef.Value, customRef.Line) - } - } - return nil -} - -func isRunningCommitRef(value string) bool { - if !strings.HasPrefix(value, "${{") || !strings.HasSuffix(value, "}}") { - return false } - return strings.TrimSpace(value[3:len(value)-2]) == "github.sha" + return replacements } func (f *File) validateSelfRepositoryMigration(scan RefScan) error { diff --git a/internal/workflowfile/workflowfile.go b/internal/workflowfile/workflowfile.go index 32aa8a64..564d13b1 100644 --- a/internal/workflowfile/workflowfile.go +++ b/internal/workflowfile/workflowfile.go @@ -663,6 +663,18 @@ func parseActionYAMLForUses(content []byte) ([]string, error) { // readActionUses returns the `uses:` values of an in-repo action definition // along with the file they were read from, so callers can rewrite that file. func readActionUses(repoRoot, actionDir string) ([]string, string, error) { + content, actionPath, err := readActionFile(repoRoot, actionDir) + if err != nil { + return nil, "", err + } + uses, err := parseActionYAMLForUses(content) + if err != nil { + return nil, "", fmt.Errorf("parsing %s: %w", filepath.Base(actionPath), err) + } + return uses, actionPath, nil +} + +func readActionFile(repoRoot, actionDir string) ([]byte, string, error) { var lastErr error for _, name := range []string{"action.yml", "action.yaml"} { actionPath := filepath.Join(actionDir, name) @@ -678,11 +690,7 @@ func readActionUses(repoRoot, actionDir string) ([]string, string, error) { lastErr = err continue } - uses, err := parseActionYAMLForUses(content) - if err != nil { - return nil, "", fmt.Errorf("parsing %s: %w", name, err) - } - return uses, actionPath, nil + return content, actionPath, nil } return nil, "", fmt.Errorf("reading action.yml or action.yaml: %w", lastErr) } From 09c7c98dc0026a039d9932a910bdd8f59591f7c4 Mon Sep 17 00:00:00 2001 From: kuma0128 Date: Fri, 16 Oct 2026 15:25:50 +0900 Subject: [PATCH 3/5] docs: clarify self repository refs for pull_request_target --- README.md | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 86ac4d55..06e2f136 100644 --- a/README.md +++ b/README.md @@ -90,9 +90,10 @@ have read access to the tenant repositories used by your workflows. ### Self repository actions (`$/…`) `uses: $/…` references an action or reusable workflow in the **same repository** as -the defining file, resolved at the **running commit**. Because it always resolves to -that repository's running SHA it is **inherently pinned** — no lockfile entry is -required, and it is valid anywhere a relative `./…` reference is: +the defining file, resolved at the **running commit** of that workflow or action. +This is independent of the workspace populated by `actions/checkout`. The reference +is **inherently pinned** and needs no lockfile entry. It is valid anywhere a relative +`./…` reference is: ```yaml steps: @@ -130,6 +131,13 @@ to preserve the local references; workflows that keep `./…` remain ineligible for onboarding. This check covers explicit checkout steps in workflows and in-repo composite actions, not workspace changes made by scripts or remote actions. +For a workflow triggered by `pull_request_target`, `$/…` in that workflow uses +the default-branch commit selected for the run (`github.sha`), regardless of the +PR's base branch (see [GitHub's event semantics](https://github.blog/changelog/2025-11-07-actions-pull_request_target-and-environment-branch-protections-changes/)). +A checkout of the same repository with no `ref`, or with `ref: ${{ github.sha }}`, +uses that commit too. Checking out `ref: ${{ github.event.pull_request.head.sha }}` +would make `./…` use the PR head instead, so the same guard rejects migration. + ## How it works A repo gets a lockfile (located at [`.github/workflows/actions.lock`](https://github.com/github/gh-actions-lock/blob/main/.github/workflows/actions.lock)) and workflows are onboarded to the lockfile on a per-workflow basis. From f7bce6c2679e4828559f71ed583764cec38e2671 Mon Sep 17 00:00:00 2001 From: kuma0128 Date: Sat, 17 Oct 2026 11:59:07 +0900 Subject: [PATCH 4/5] Check configured workflow callers before composite migration --- README.md | 6 ++- cmd/gh-actions-lock/migrate_test.go | 64 +++++++++++++++++++++++++++++ cmd/gh-actions-lock/run.go | 19 ++++++--- 3 files changed, 81 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 06e2f136..18429f4d 100644 --- a/README.md +++ b/README.md @@ -124,8 +124,10 @@ in the same job, including checkouts and local references reached through in-repo composite actions. Checkout state follows nested `./…` and `$/…` calls and carries back to subsequent caller steps. Since composite definitions are migrated repository-wide, their callers are checked even in workflows -outside a scoped run. A custom ref can make `./…` load different code from -`$/…`. The error identifies the action file and the checkout's file and line. +outside a scoped run. This includes workflows in both `.github/workflows` and +the directory configured by `GH_ACTIONS_LOCK_WORKFLOWS_DIR`. A custom ref can +make `./…` load different code from `$/…`. The error identifies the action file +and the checkout's file and line. Review the checkout and action references together, or use `--no-migrate-local-actions` to preserve the local references; workflows that keep `./…` remain ineligible for onboarding. This check covers explicit checkout steps in workflows and diff --git a/cmd/gh-actions-lock/migrate_test.go b/cmd/gh-actions-lock/migrate_test.go index 8c4f0a15..5c632a06 100644 --- a/cmd/gh-actions-lock/migrate_test.go +++ b/cmd/gh-actions-lock/migrate_test.go @@ -50,6 +50,70 @@ func TestMigrateLocalActions_CustomCheckoutRefWritesNothing(t *testing.T) { } } +func TestMigrateLocalActions_ConfiguredWorkflowCallers(t *testing.T) { + for _, tt := range []struct { + name string + callerDir string + absoluteDir bool + blocked bool + }{ + {name: "relative configured directory blocks unsafe migration", callerDir: ".github/custom-workflows", blocked: true}, + {name: "absolute configured directory blocks unsafe migration", callerDir: ".github/custom-workflows", absoluteDir: true, blocked: true}, + {name: "default directory remains checked with an override", callerDir: ".github/workflows", blocked: true}, + {name: "running commit checkout allows migration", callerDir: ".github/custom-workflows"}, + } { + t.Run(tt.name, func(t *testing.T) { + t.Chdir(t.TempDir()) + require.NoError(t, os.Mkdir(".git", 0o755)) + workflowsDir := filepath.Join(".github", "custom-workflows") + configuredDir := workflowsDir + if tt.absoluteDir { + absDir, err := filepath.Abs(workflowsDir) + require.NoError(t, err) + configuredDir = absDir + } + t.Setenv("GH_ACTIONS_LOCK_WORKFLOWS_DIR", configuredDir) + ref := "${{ github.sha }}" + if tt.blocked { + ref = "other" + } + firstPath := filepath.Join(workflowsDir, "first.yml") + secondPath := filepath.Join(tt.callerDir, "second.yml") + files := map[string]string{ + firstPath: "jobs:\n build:\n steps:\n - uses: $/.github/actions/local\n", + secondPath: "jobs:\n build:\n steps:\n - uses: actions/checkout@v4\n with: {ref: '" + ref + "'}\n - uses: $/.github/actions/local\n", + ".github/actions/local/action.yml": "runs:\n using: composite\n steps:\n - uses: ./.github/actions/helper\n", + ".github/actions/helper/action.yml": "runs:\n using: composite\n steps:\n - run: echo hello\n shell: bash\n", + filepath.Join(workflowsDir, "actions.lock"): "version: 'v0.0.3'\nworkflows: {}\ndependencies: {}\n", + } + for path, content := range files { + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o755)) + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) + } + before := snapshotWorkflows(t) + transport := &requestCountingTransport{} + + _, _, err := runCommandWithHTTP(t, transport, firstPath) + + if tt.blocked { + require.Error(t, err) + assert.Contains(t, err.Error(), secondPath) + assert.Contains(t, err.Error(), `ref "other"`) + assert.Equal(t, before, snapshotWorkflows(t)) + assert.Zero(t, transport.calls.Load(), "unsafe migration stops before resolving dependencies") + return + } + require.NoError(t, err) + content, err := os.ReadFile(".github/actions/local/action.yml") + require.NoError(t, err) + assert.Contains(t, string(content), "uses: $/.github/actions/helper") + caller, err := os.ReadFile(secondPath) + require.NoError(t, err) + assert.Equal(t, files[secondPath], string(caller)) + }) + } +} + // TestMigrateLocalActions_CompositeActionFiles verifies that migration // rewrites `./…` not just in workflow files but also inside in-repo composite // action definitions (action.yml), including ones nested a few directories deep. diff --git a/cmd/gh-actions-lock/run.go b/cmd/gh-actions-lock/run.go index a2efb107..27db478a 100644 --- a/cmd/gh-actions-lock/run.go +++ b/cmd/gh-actions-lock/run.go @@ -636,9 +636,8 @@ func migrateLocalActions(paths []string) (int, func(), error) { files := append([]string(nil), paths...) var repoRoot string - // Also sweep in-repo composite action files. The repo root is derived from - // a workflow path (they live under .github/workflows/); onboarding only - // runs when workflows exist, so an empty paths list means nothing to do. + // Also sweep in-repo composite action files. Derive the repo root from a + // workflow path; an empty paths list means nothing to do. if len(paths) > 0 { root, err := workflowfile.FindRepoRoot(paths[0]) if err != nil { @@ -703,9 +702,17 @@ func migrateLocalActions(paths []string) (int, func(), error) { compositeChanged = compositeChanged || name == "action.yml" || name == "action.yaml" } if compositeChanged && repoRoot != "" { - workflowPaths, err := workflowfile.DiscoverWorkflowsIn(filepath.Join(repoRoot, ".github", "workflows")) - if err != nil { - return 0, undo, err + workflowDirs := []string{filepath.Join(repoRoot, ".github", "workflows")} + if dir := os.Getenv("GH_ACTIONS_LOCK_WORKFLOWS_DIR"); dir != "" { + workflowDirs = append(workflowDirs, dir) + } + var workflowPaths []string + for _, dir := range workflowDirs { + callers, err := workflowfile.DiscoverWorkflowsIn(dir) + if err != nil { + return 0, undo, err + } + workflowPaths = append(workflowPaths, callers...) } if err := workflowfile.ValidateLocalActionMigrations(workflowPaths, migrationPaths); err != nil { return 0, undo, err From 008d7194765ae0599847678ae3d4eec06f101760 Mon Sep 17 00:00:00 2001 From: kuma0128 Date: Wed, 21 Oct 2026 09:54:37 +0900 Subject: [PATCH 5/5] Guard local action migration after repository checkout overrides --- README.md | 20 ++++-- cmd/gh-actions-lock/migrate_test.go | 74 ++++++++++++++-------- internal/workflowfile/checkout.go | 51 ++++++++------- internal/workflowfile/checkout_test.go | 27 +++++++- internal/workflowfile/rewrite.go | 8 +-- internal/workflowfile/rewrite_test.go | 17 ++++- internal/workflowfile/workflowfile_test.go | 36 +++++++++-- 7 files changed, 163 insertions(+), 70 deletions(-) diff --git a/README.md b/README.md index 18429f4d..da3857ed 100644 --- a/README.md +++ b/README.md @@ -119,15 +119,21 @@ gh actions-lock --no-migrate-local-actions ``` Migration stops before writing files if a local action follows an -`actions/checkout` step with a non-empty `ref` other than `${{ github.sha }}` -in the same job, including checkouts and local references reached through -in-repo composite actions. Checkout state follows nested `./…` and `$/…` -calls and carries back to subsequent caller steps. Since composite definitions +`actions/checkout` step in the same job with either: + +- A non-empty `ref` other than `${{ github.sha }}`. +- A non-empty `repository` other than `${{ github.repository }}`. Literal + repository names and other expressions are rejected because the CLI cannot + establish that they name the same repository at runtime. + +This includes checkouts and local references reached through in-repo composite +actions. Checkout state follows nested `./…` and `$/…` calls and carries back +to subsequent caller steps. Since composite definitions are migrated repository-wide, their callers are checked even in workflows outside a scoped run. This includes workflows in both `.github/workflows` and -the directory configured by `GH_ACTIONS_LOCK_WORKFLOWS_DIR`. A custom ref can -make `./…` load different code from `$/…`. The error identifies the action file -and the checkout's file and line. +the directory configured by `GH_ACTIONS_LOCK_WORKFLOWS_DIR`. A custom repository +or ref can make `./…` load different code from `$/…`. The error identifies the +action file and the checkout input's file and line. Review the checkout and action references together, or use `--no-migrate-local-actions` to preserve the local references; workflows that keep `./…` remain ineligible for onboarding. This check covers explicit checkout steps in workflows and diff --git a/cmd/gh-actions-lock/migrate_test.go b/cmd/gh-actions-lock/migrate_test.go index 5c632a06..27b4af55 100644 --- a/cmd/gh-actions-lock/migrate_test.go +++ b/cmd/gh-actions-lock/migrate_test.go @@ -1,6 +1,7 @@ package main import ( + "fmt" "os" "path/filepath" "strings" @@ -11,41 +12,58 @@ import ( "github.com/stretchr/testify/require" ) -func TestMigrateLocalActions_CustomCheckoutRefWritesNothing(t *testing.T) { - for _, args := range [][]string{nil, {"--json"}, {".github/workflows/first.yml"}} { - t.Run(strings.Join(args, " "), func(t *testing.T) { - t.Chdir(t.TempDir()) - require.NoError(t, os.Mkdir(".git", 0o755)) - files := map[string]string{ - ".github/workflows/first.yml": "jobs:\n build:\n steps:\n - uses: ./.github/actions/local\n", - ".github/workflows/second.yml": `jobs: +func TestMigrateLocalActions_CustomCheckoutWritesNothing(t *testing.T) { + for _, input := range []struct { + name string + value string + }{ + {name: "ref", value: "${{ env.TARGET_BRANCH }}"}, + {name: "repository", value: "other/project"}, + } { + t.Run(input.name, func(t *testing.T) { + for _, mode := range []struct { + name string + args []string + }{ + {name: "full run leaves files unchanged"}, + {name: "JSON run leaves files unchanged", args: []string{"--json"}}, + {name: "scoped run checks unselected callers", args: []string{".github/workflows/first.yml"}}, + } { + t.Run(mode.name, func(t *testing.T) { + t.Chdir(t.TempDir()) + require.NoError(t, os.Mkdir(".git", 0o755)) + files := map[string]string{ + ".github/workflows/first.yml": "jobs:\n build:\n steps:\n - uses: ./.github/actions/local\n", + ".github/workflows/second.yml": fmt.Sprintf(`jobs: refresh: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 with: - ref: ${{ env.TARGET_BRANCH }} + %s: %s - uses: $/.github/actions/local -`, - ".github/actions/local/action.yml": "runs:\n using: composite\n steps:\n - uses: ./.github/actions/helper\n", - ".github/actions/helper/action.yml": "runs:\n using: composite\n steps:\n - run: echo hello\n shell: bash\n", - ".github/workflows/actions.lock": "version: 'v0.0.3'\nworkflows: {}\ndependencies: {}\n", - } - for path, content := range files { - require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o755)) - require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) +`, input.name, input.value), + ".github/actions/local/action.yml": "runs:\n using: composite\n steps:\n - uses: ./.github/actions/helper\n", + ".github/actions/helper/action.yml": "runs:\n using: composite\n steps:\n - run: echo hello\n shell: bash\n", + ".github/workflows/actions.lock": "version: 'v0.0.3'\nworkflows: {}\ndependencies: {}\n", + } + for path, content := range files { + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o755)) + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) + } + before := snapshotWorkflows(t) + transport := &requestCountingTransport{} + + _, _, err := runCommandWithHTTP(t, transport, mode.args...) + + require.Error(t, err) + assert.Contains(t, err.Error(), "second.yml:7") + assert.Contains(t, err.Error(), fmt.Sprintf("%s %q", input.name, input.value)) + assert.Contains(t, err.Error(), "--no-migrate-local-actions") + assert.Equal(t, before, snapshotWorkflows(t)) + assert.Zero(t, transport.calls.Load(), "unsafe migration stops before resolving dependencies") + }) } - before := snapshotWorkflows(t) - transport := &requestCountingTransport{} - - _, _, err := runCommandWithHTTP(t, transport, args...) - - require.Error(t, err) - assert.Contains(t, err.Error(), "second.yml") - assert.Contains(t, err.Error(), "${{ env.TARGET_BRANCH }}") - assert.Contains(t, err.Error(), "--no-migrate-local-actions") - assert.Equal(t, before, snapshotWorkflows(t)) - assert.Zero(t, transport.calls.Load(), "unsafe migration stops before resolving dependencies") }) } } diff --git a/internal/workflowfile/checkout.go b/internal/workflowfile/checkout.go index bb48a984..8eff934a 100644 --- a/internal/workflowfile/checkout.go +++ b/internal/workflowfile/checkout.go @@ -52,8 +52,9 @@ func ValidateLocalActionMigrations(workflowPaths, migrationPaths []string) error return nil } -type checkoutRef struct { +type checkoutOverride struct { path string + name string node *yaml.Node } @@ -62,15 +63,15 @@ type checkoutValidator struct { migrations map[string]bool // nil means all local action references are candidates files map[string]*File active map[checkoutContext]bool - results map[checkoutContext]*checkoutRef + results map[checkoutContext]*checkoutOverride } type checkoutContext struct { steps *yaml.Node - ref *yaml.Node + input *yaml.Node } -func (f *File) validateCheckoutRefsForMigration(replacements map[string]string) error { +func (f *File) validateCheckoutForMigration(replacements map[string]string) error { root, err := findRepoRoot(f.Path) if err != nil { return err @@ -103,17 +104,17 @@ func (v *checkoutValidator) validate(f *File, replacements map[string]string) er return nil } -func (v *checkoutValidator) steps(f *File, owner *yaml.Node, replacements map[string]string, customRef *checkoutRef, depth int) (*checkoutRef, error) { +func (v *checkoutValidator) steps(f *File, owner *yaml.Node, replacements map[string]string, override *checkoutOverride, depth int) (*checkoutOverride, error) { if depth > maxYAMLWalkDepth { return nil, fmt.Errorf("checking checkout context in %s: composite action nesting exceeds %d", f.Path, maxYAMLWalkDepth) } steps := mapValue(owner, "steps") if steps == nil || steps.Kind != yaml.SequenceNode { - return customRef, nil + return override, nil } context := checkoutContext{steps: steps} - if customRef != nil { - context.ref = customRef.node + if override != nil { + context.input = override.node } if result, ok := v.results[context]; ok { return result, nil @@ -123,7 +124,7 @@ func (v *checkoutValidator) steps(f *File, owner *yaml.Node, replacements map[st } if v.active == nil { v.active = make(map[checkoutContext]bool) - v.results = make(map[checkoutContext]*checkoutRef) + v.results = make(map[checkoutContext]*checkoutOverride) } v.active[context] = true defer delete(v.active, context) @@ -132,15 +133,21 @@ func (v *checkoutValidator) steps(f *File, owner *yaml.Node, replacements map[st uses := strings.TrimSpace(scalarValue(mapValue(step, "uses"))) action, _, hasRef := strings.Cut(uses, "@") if hasRef && strings.EqualFold(action, "actions/checkout") { - ref := mapValue(mapValue(step, "with"), "ref") - value := strings.TrimSpace(scalarValue(ref)) - if value != "" && !isRunningCommitRef(value) { - customRef = &checkoutRef{path: f.Path, node: ref} + for _, input := range []struct{ name, context string }{ + {"repository", "github.repository"}, + {"ref", "github.sha"}, + } { + node := mapValue(mapValue(step, "with"), input.name) + value := strings.TrimSpace(scalarValue(node)) + if value != "" && !isContextExpression(value, input.context) { + override = &checkoutOverride{path: f.Path, name: input.name, node: node} + break + } } - // Keep earlier custom refs: a later checkout can be conditional. + // Keep earlier overrides: a later checkout can be conditional. } - if replacement, ok := replacements[uses]; ok && customRef != nil { - return nil, fmt.Errorf("cannot migrate %s to %s in %s: preceding actions/checkout uses ref %q at %s:%d; $/ resolves to the running commit, not the checked-out ref; use --no-migrate-local-actions to keep local references", uses, replacement, f.Path, customRef.node.Value, customRef.path, customRef.node.Line) + if replacement, ok := replacements[uses]; ok && override != nil { + return nil, fmt.Errorf("cannot migrate %s to %s in %s: preceding actions/checkout uses %s %q at %s:%d; $/ resolves to the defining repository's running commit, not the checked-out workspace; use --no-migrate-local-actions to keep local references", uses, replacement, f.Path, override.name, override.node.Value, override.path, override.node.Line) } var localPath string switch { @@ -174,19 +181,19 @@ func (v *checkoutValidator) steps(f *File, owner *yaml.Node, replacements map[st // A composite inherits the caller's checkout state and can change the // workspace seen by subsequent steps, even if its invocation is conditional. runs := mapValue(documentRoot(&child.root), "runs") - nextRef, err := v.steps(child, runs, v.replacements(child), customRef, depth+1) + nextOverride, err := v.steps(child, runs, v.replacements(child), override, depth+1) if err != nil { return nil, err } - customRef = nextRef + override = nextOverride } - v.results[context] = customRef - return customRef, nil + v.results[context] = override + return override, nil } -func isRunningCommitRef(value string) bool { +func isContextExpression(value, context string) bool { if !strings.HasPrefix(value, "${{") || !strings.HasSuffix(value, "}}") { return false } - return strings.TrimSpace(value[3:len(value)-2]) == "github.sha" + return strings.TrimSpace(value[3:len(value)-2]) == context } diff --git a/internal/workflowfile/checkout_test.go b/internal/workflowfile/checkout_test.go index 1617f1ca..6684f85e 100644 --- a/internal/workflowfile/checkout_test.go +++ b/internal/workflowfile/checkout_test.go @@ -15,6 +15,7 @@ func TestMigrateLocalActionsToSelfRepository_CheckoutContext(t *testing.T) { workflow string outer string blocked bool + source string }{ { name: "custom ref reaches an already migrated composite", @@ -78,6 +79,26 @@ func TestMigrateLocalActionsToSelfRepository_CheckoutContext(t *testing.T) { workflow: " - uses: actions/checkout@v4\n with: {ref: other}\n - uses: $/outer\n", outer: " - uses: $/leaf\n", }, + { + name: "other repository reaches a nested composite after a safe call", + workflow: " - uses: $/outer\n - uses: actions/checkout@v4\n with: {repository: other/project}\n - uses: $/outer\n", + outer: " - uses: $/middle\n", + blocked: true, + source: `repository "other/project"`, + }, + { + name: "repository checkout in a composite reaches subsequent caller steps", + workflow: " - uses: $/outer\n - uses: ./leaf\n", + outer: " - uses: actions/checkout@v4\n with: {repository: other/project}\n", + blocked: true, + source: `repository "other/project"`, + }, + { + name: "default checkout cannot clear an earlier repository override", + workflow: " - uses: actions/checkout@v4\n with: {repository: other/project}\n - uses: actions/checkout@v4\n if: inputs.reset\n with: {repository: '${{ github.repository }}'}\n - uses: ./leaf\n", + blocked: true, + source: `repository "other/project"`, + }, } { t.Run(tt.name, func(t *testing.T) { dir := t.TempDir() @@ -99,7 +120,11 @@ func TestMigrateLocalActionsToSelfRepository_CheckoutContext(t *testing.T) { if tt.blocked { require.Error(t, err) assert.Contains(t, err.Error(), "preceding actions/checkout") - assert.Contains(t, err.Error(), `ref "other"`) + source := tt.source + if source == "" { + source = `ref "other"` + } + assert.Contains(t, err.Error(), source) assert.Contains(t, err.Error(), ".yml:") assert.Zero(t, changed) assert.Equal(t, workflow, string(content)) diff --git a/internal/workflowfile/rewrite.go b/internal/workflowfile/rewrite.go index 2338e9e2..de438ea8 100644 --- a/internal/workflowfile/rewrite.go +++ b/internal/workflowfile/rewrite.go @@ -137,9 +137,9 @@ func (f *File) ValidateRequiredActionRefRewrites(replacements map[string]string) // MigrateLocalActionsToSelfRepository rewrites same-repo `./…` composite action // references to the inherently-pinned `$/…` form. Only local paths that // resolve to an in-repo action file are candidates. A preceding checkout of a -// custom ref prevents migration. Local reusable workflows are never candidates -// (ExtractActionRefs excludes them). Returns the new content and the number of -// `uses:` lines changed. +// custom repository or ref prevents migration. Local reusable workflows are +// never candidates (ExtractActionRefs excludes them). Returns the new content +// and the number of `uses:` lines changed. func (f *File) MigrateLocalActionsToSelfRepository() ([]byte, int, error) { scan := f.ExtractActionRefs() if err := f.validateSelfRepositoryMigration(scan); err != nil { @@ -150,7 +150,7 @@ func (f *File) MigrateLocalActionsToSelfRepository() ([]byte, int, error) { return append([]byte(nil), f.Content...), 0, err } replacements := f.localActionReplacements(repoRoot) - if err := f.validateCheckoutRefsForMigration(replacements); err != nil { + if err := f.validateCheckoutForMigration(replacements); err != nil { return append([]byte(nil), f.Content...), 0, err } diff --git a/internal/workflowfile/rewrite_test.go b/internal/workflowfile/rewrite_test.go index e6eb1326..12243290 100644 --- a/internal/workflowfile/rewrite_test.go +++ b/internal/workflowfile/rewrite_test.go @@ -9,7 +9,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestValidateCheckoutRefsForMigration(t *testing.T) { +func TestValidateCheckoutForMigration(t *testing.T) { for _, tt := range []struct { name string content string @@ -87,6 +87,19 @@ jobs: - uses: actions/checkout@v4 with: *inputs - uses: ./local +`, + blocked: true, + }, + { + name: "aliased repository input blocks migration", + content: `env: + inputs: &inputs {repository: other/project} +jobs: + build: + steps: + - uses: actions/checkout@v4 + with: *inputs + - uses: ./local `, blocked: true, }, @@ -128,7 +141,7 @@ jobs: t.Run(tt.name, func(t *testing.T) { f, err := Parse("ci.yml", []byte(tt.content)) require.NoError(t, err) - err = f.validateCheckoutRefsForMigration(map[string]string{"./local": "$/local"}) + err = f.validateCheckoutForMigration(map[string]string{"./local": "$/local"}) if tt.blocked { require.Error(t, err) assert.Contains(t, err.Error(), "preceding actions/checkout") diff --git a/internal/workflowfile/workflowfile_test.go b/internal/workflowfile/workflowfile_test.go index 4c1cdc94..5789e603 100644 --- a/internal/workflowfile/workflowfile_test.go +++ b/internal/workflowfile/workflowfile_test.go @@ -222,7 +222,7 @@ func TestMigrateLocalActionsToSelfRepository_NoLocalPaths(t *testing.T) { assert.Equal(t, content, out) } -func TestMigrateLocalActionsToSelfRepository_CheckoutRef(t *testing.T) { +func TestMigrateLocalActionsToSelfRepository_CheckoutInputs(t *testing.T) { repoRoot := t.TempDir() require.NoError(t, os.Mkdir(filepath.Join(repoRoot, ".git"), 0o755)) require.NoError(t, os.MkdirAll(filepath.Join(repoRoot, "local-action"), 0o755)) @@ -230,9 +230,10 @@ func TestMigrateLocalActionsToSelfRepository_CheckoutRef(t *testing.T) { []byte("runs:\n using: composite\n"), 0o644)) for _, tt := range []struct { - name string - ref string - blocked bool + name string + ref string + repository string + blocked bool }{ {name: "default ref"}, {name: "empty ref", ref: "''"}, @@ -245,11 +246,29 @@ func TestMigrateLocalActionsToSelfRepository_CheckoutRef(t *testing.T) { {name: "environment variable", ref: "${{ env.TARGET_BRANCH }}", blocked: true}, {name: "pull request head", ref: "${{ github.event.pull_request.head.sha }}", blocked: true}, {name: "running branch can move", ref: "${{ github.ref }}", blocked: true}, + {name: "empty repository", repository: "''"}, + {name: "whitespace repository", repository: "' '"}, + {name: "current repository", repository: "${{ github.repository }}"}, + {name: "current repository without spaces", repository: "${{github.repository}}"}, + {name: "current repository with surrounding whitespace", repository: "' ${{ github.repository }} '"}, + {name: "current repository and running commit", repository: "${{ github.repository }}", ref: "${{ github.sha }}"}, + {name: "current repository with custom ref", repository: "${{ github.repository }}", ref: "other", blocked: true}, + {name: "other repository with default ref", repository: "other/project", blocked: true}, + {name: "other repository with running commit", repository: "other/project", ref: "${{ github.sha }}", blocked: true}, + {name: "repository from workflow input", repository: "${{ inputs.repository }}", blocked: true}, + {name: "pull request head repository", repository: "${{ github.event.pull_request.head.repo.full_name }}", blocked: true}, + {name: "repository expression with fallback", repository: "${{ inputs.repository || github.repository }}", blocked: true}, } { t.Run(tt.name, func(t *testing.T) { content := "jobs:\n build:\n steps:\n - uses: actions/checkout@v4\n" + if tt.ref != "" || tt.repository != "" { + content += " with:\n" + } if tt.ref != "" { - content += " with:\n ref: " + tt.ref + "\n" + content += " ref: " + tt.ref + "\n" + } + if tt.repository != "" { + content += " repository: " + tt.repository + "\n" } content += " - uses: ./local-action\n" f, err := Parse(filepath.Join(repoRoot, ".github", "workflows", "ci.yml"), []byte(content)) @@ -259,7 +278,12 @@ func TestMigrateLocalActionsToSelfRepository_CheckoutRef(t *testing.T) { if tt.blocked { require.Error(t, err) assert.Contains(t, err.Error(), "actions/checkout") - assert.Contains(t, err.Error(), tt.ref) + if tt.repository != "" && tt.repository != "${{ github.repository }}" { + assert.Contains(t, err.Error(), "repository") + assert.Contains(t, err.Error(), tt.repository) + } else { + assert.Contains(t, err.Error(), tt.ref) + } assert.Contains(t, err.Error(), "--no-migrate-local-actions") assert.Zero(t, changed) assert.Equal(t, content, string(out))