diff --git a/README.md b/README.md index d45a6c4a..da3857ed 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: @@ -117,6 +118,34 @@ 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 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 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 +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. diff --git a/cmd/gh-actions-lock/migrate_test.go b/cmd/gh-actions-lock/migrate_test.go index 3c0096b3..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,11 +12,131 @@ import ( "github.com/stretchr/testify/require" ) +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: + %s: %s + - uses: $/.github/actions/local +`, 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") + }) + } + }) + } +} + +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. // 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/cmd/gh-actions-lock/run.go b/cmd/gh-actions-lock/run.go index ebd6d51f..27db478a 100644 --- a/cmd/gh-actions-lock/run.go +++ b/cmd/gh-actions-lock/run.go @@ -634,16 +634,17 @@ 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 { return 0, undo, err } if root != "" { + repoRoot = root actionFiles, err := workflowfile.DiscoverCompositeActionFiles(root) if err != nil { return 0, undo, err @@ -691,8 +692,35 @@ 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 != "" { + 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 + } + } + + // 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..8eff934a --- /dev/null +++ b/internal/workflowfile/checkout.go @@ -0,0 +1,199 @@ +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 checkoutOverride struct { + path string + name 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]*checkoutOverride +} + +type checkoutContext struct { + steps *yaml.Node + input *yaml.Node +} + +func (f *File) validateCheckoutForMigration(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, 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 override, nil + } + context := checkoutContext{steps: steps} + if override != nil { + context.input = override.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]*checkoutOverride) + } + 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") { + 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 overrides: a later checkout can be conditional. + } + 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 { + 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") + nextOverride, err := v.steps(child, runs, v.replacements(child), override, depth+1) + if err != nil { + return nil, err + } + override = nextOverride + } + v.results[context] = override + return override, nil +} + +func isContextExpression(value, context string) bool { + if !strings.HasPrefix(value, "${{") || !strings.HasSuffix(value, "}}") { + return false + } + return strings.TrimSpace(value[3:len(value)-2]) == context +} diff --git a/internal/workflowfile/checkout_test.go b/internal/workflowfile/checkout_test.go new file mode 100644 index 00000000..6684f85e --- /dev/null +++ b/internal/workflowfile/checkout_test.go @@ -0,0 +1,198 @@ +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 + source string + }{ + { + 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", + }, + { + 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() + 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") + 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)) + 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 1d24a21e..de438ea8 100644 --- a/internal/workflowfile/rewrite.go +++ b/internal/workflowfile/rewrite.go @@ -136,38 +136,37 @@ 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 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 { 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.validateCheckoutForMigration(replacements); err != nil { + return append([]byte(nil), f.Content...), 0, err } return f.RewriteActionRefs(replacements) } +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, "./") + } + } + return replacements +} + 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..12243290 100644 --- a/internal/workflowfile/rewrite_test.go +++ b/internal/workflowfile/rewrite_test.go @@ -9,6 +9,149 @@ import ( "github.com/stretchr/testify/require" ) +func TestValidateCheckoutForMigration(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: "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, + }, + { + 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.validateCheckoutForMigration(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.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) } diff --git a/internal/workflowfile/workflowfile_test.go b/internal/workflowfile/workflowfile_test.go index fe6251ef..5789e603 100644 --- a/internal/workflowfile/workflowfile_test.go +++ b/internal/workflowfile/workflowfile_test.go @@ -222,6 +222,80 @@ func TestMigrateLocalActionsToSelfRepository_NoLocalPaths(t *testing.T) { assert.Equal(t, content, out) } +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)) + 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 + repository 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}, + {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 += " 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)) + require.NoError(t, err) + + out, changed, err := f.MigrateLocalActionsToSelfRepository() + if tt.blocked { + require.Error(t, err) + assert.Contains(t, err.Error(), "actions/checkout") + 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)) + 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))