diff --git a/cmd/gh-actions-lock/migrate_test.go b/cmd/gh-actions-lock/migrate_test.go index a6b7513e..ebbecf67 100644 --- a/cmd/gh-actions-lock/migrate_test.go +++ b/cmd/gh-actions-lock/migrate_test.go @@ -56,6 +56,34 @@ func TestMigrateLocalActions_CompositeActionFiles(t *testing.T) { assert.Contains(t, read("my-action/action.yml"), "uses: ./nonexistent") } +// TestMigrateLocalActions_RepoRootEnvAboveCheckout covers a RepoRootEnv root +// without .git that holds the real checkout in a subdirectory, as with +// actions/checkout `path: src`. That checkout must not be skipped as nested. +func TestMigrateLocalActions_RepoRootEnvAboveCheckout(t *testing.T) { + workspace := t.TempDir() + write := func(rel, content string) { + full := filepath.Join(workspace, rel) + require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o755)) + require.NoError(t, os.WriteFile(full, []byte(content), 0o644)) + } + require.NoError(t, os.MkdirAll(filepath.Join(workspace, "src", ".git"), 0o755)) + write("src/.github/workflows/ci.yml", + "name: CI\non: push\njobs:\n b:\n runs-on: ubuntu-latest\n steps:\n - uses: actions/checkout@v4\n") + write("src/my-action/action.yml", + "name: My Action\nruns:\n using: composite\n steps:\n - uses: ./src/helper\n") + write("src/helper/action.yml", + "name: Helper\nruns:\n using: composite\n steps:\n - run: echo hi\n shell: bash\n") + t.Setenv("GH_ACTIONS_LOCK_REPO_ROOT", workspace) + + total, _, err := migrateLocalActions([]string{filepath.Join(workspace, "src", ".github", "workflows", "ci.yml")}) + + require.NoError(t, err) + assert.Equal(t, 1, total) + b, err := os.ReadFile(filepath.Join(workspace, "src", "my-action", "action.yml")) + require.NoError(t, err) + assert.Contains(t, string(b), "uses: $/src/helper") +} + func TestMigrateLocalActions_StructuralErrorPreventsAllWrites(t *testing.T) { dir := t.TempDir() write := func(rel, content string) string { diff --git a/internal/workflowfile/rewrite.go b/internal/workflowfile/rewrite.go index 69037633..cbc72a9f 100644 --- a/internal/workflowfile/rewrite.go +++ b/internal/workflowfile/rewrite.go @@ -179,7 +179,10 @@ func (f *File) validateSelfRepositoryMigration(scan RefScan) error { } // localActionExists reports whether a `./…` path resolves to an action file -// (action.yml or action.yaml) within the repo root. +// (action.yml or action.yaml) within the repo root. An action inside a +// submodule or other nested checkout doesn't count: it isn't part of this +// repository, so a `$/` ref can't reach it, and DiscoverCompositeActionFiles +// doesn't migrate its own `./…` steps. func localActionExists(repoRoot, localPath string) bool { relPath := strings.TrimPrefix(localPath, "./") actionDir := filepath.Join(repoRoot, relPath) @@ -191,6 +194,9 @@ func localActionExists(repoRoot, localPath string) bool { if err := ValidatePathWithinRoot(repoRoot, actionPath); err != nil { continue } + if inNestedCheckout(repoRoot, actionPath) { + continue + } info, err := os.Stat(actionPath) if err == nil && info.Mode().IsRegular() { return true diff --git a/internal/workflowfile/workflowfile.go b/internal/workflowfile/workflowfile.go index dd887678..2c7b90eb 100644 --- a/internal/workflowfile/workflowfile.go +++ b/internal/workflowfile/workflowfile.go @@ -351,26 +351,53 @@ func FindRepoRoot(startPath string) (string, error) { // DiscoverCompositeActionFiles walks the repository rooted at root and returns // the paths of all action definition files (action.yml / action.yaml). The -// .git directory is skipped. Non-composite action files are included; callers -// migrate them with MigrateLocalActionsToSelfRepository, which no-ops when a file has -// no local `./…` steps. +// .git directory is skipped. When root is itself a checkout, so is any +// subdirectory that is a separate checkout (worktree, submodule or nested +// clone); see inNestedCheckout. A RepoRootEnv root without .git may hold the +// real checkout below it, so nothing is skipped there. Non-composite action +// files are included; callers migrate them with +// MigrateLocalActionsToSelfRepository, which no-ops when a file has no local +// `./…` steps. func DiscoverCompositeActionFiles(root string) ([]string, error) { if root == "" { return nil, nil } - var paths []string - err := filepath.WalkDir(root, func(p string, d fs.DirEntry, walkErr error) error { - if walkErr != nil { - return walkErr - } - if d.IsDir() { - if d.Name() == ".git" { - return filepath.SkipDir + w := actionFileWalker{root: root, skipNested: isCheckout(root)} + if err := w.walk(root); err != nil { + return nil, fmt.Errorf("discovering action files under %s: %w", root, err) + } + sort.Strings(w.paths) + return w.paths, nil +} + +type actionFileWalker struct { + root string + skipNested bool + paths []string +} + +// walk reads each directory once and checks its listing for a .git entry, +// so detecting nested checkouts costs no extra syscall per directory. +func (w *actionFileWalker) walk(dir string) error { + entries, err := os.ReadDir(dir) + if err != nil { + return err + } + if w.skipNested && dir != w.root && hasEntry(entries, ".git") && isCheckout(dir) { + return nil + } + for _, e := range entries { + p := filepath.Join(dir, e.Name()) + switch { + case e.IsDir(): + if e.Name() == ".git" { + continue } - return nil - } - if d.Name() == "action.yml" || d.Name() == "action.yaml" { - if err := ValidatePathWithinRoot(root, p); err != nil { + if err := w.walk(p); err != nil { + return err + } + case e.Name() == "action.yml" || e.Name() == "action.yaml": + if err := ValidatePathWithinRoot(w.root, p); err != nil { return err } info, err := os.Stat(p) @@ -380,15 +407,69 @@ func DiscoverCompositeActionFiles(root string) ([]string, error) { if !info.Mode().IsRegular() { return fmt.Errorf("action definition %s is not a regular file", p) } - paths = append(paths, p) + // The walk already skipped nested checkouts, so only a symlinked + // action file can still point into one. + if w.skipNested && e.Type()&fs.ModeSymlink != 0 && inNestedCheckout(w.root, p) { + continue + } + w.paths = append(w.paths, p) } - return nil - }) + } + return nil +} + +func hasEntry(entries []fs.DirEntry, name string) bool { + return slices.ContainsFunc(entries, func(e fs.DirEntry) bool { return e.Name() == name }) +} + +// isCheckout reports whether dir is the top of a git checkout. A worktree or +// submodule has a .git file and a clone has a .git directory. Stat follows +// symlinks, so a dangling .git link does not count. +func isCheckout(dir string) bool { + _, err := os.Stat(filepath.Join(dir, ".git")) + return err == nil +} + +// inNestedCheckout reports whether file, after resolving symlinks, lies in a +// separate checkout below root. Resolving matters because a symlink outside +// the nested checkout can point into it. As in DiscoverCompositeActionFiles, +// this only applies when root is itself a checkout. Callers have already +// validated file with ValidatePathWithinRoot, so a resolution error here +// means the tree changed underneath us; treat it as nested to stay safe. +// +// EvalSymlinks doesn't resolve Windows junctions (Go reports them as +// ModeIrregular), so a junction on the path hides where it leads. Those +// count as nested too, at the cost of not migrating a junction that stays +// inside this checkout. +func inNestedCheckout(root, file string) bool { + if !isCheckout(root) { + return false + } + resolvedRoot, err := filepath.EvalSymlinks(root) if err != nil { - return nil, fmt.Errorf("discovering action files under %s: %w", root, err) + return true } - sort.Strings(paths) - return paths, nil + resolvedFile, err := filepath.EvalSymlinks(file) + if err != nil { + return true + } + rel, err := filepath.Rel(resolvedRoot, resolvedFile) + if err != nil || !filepath.IsLocal(rel) { + return true + } + for dir := filepath.Dir(rel); dir != "."; dir = filepath.Dir(dir) { + p := filepath.Join(resolvedRoot, dir) + if !isPlainDir(p) || isCheckout(p) { + return true + } + } + return false +} + +// isPlainDir uses Lstat so a junction, which Stat would follow, doesn't count. +func isPlainDir(p string) bool { + info, err := os.Lstat(p) + return err == nil && info.IsDir() } // walkUses visits `uses:` values at the only places GitHub Actions honors @@ -539,7 +620,7 @@ func findRepoRoot(startPath string) (string, error) { return overrideRepoRoot(root, absPath) } for { - if _, err := os.Stat(filepath.Join(absPath, ".git")); err == nil { + if isCheckout(absPath) { return absPath, nil } parent := filepath.Dir(absPath) diff --git a/internal/workflowfile/workflowfile_test.go b/internal/workflowfile/workflowfile_test.go index a6343d92..e1bd8d80 100644 --- a/internal/workflowfile/workflowfile_test.go +++ b/internal/workflowfile/workflowfile_test.go @@ -3,7 +3,10 @@ package workflowfile import ( "errors" "os" + "os/exec" "path/filepath" + "runtime" + "slices" "strings" "testing" @@ -642,6 +645,65 @@ func TestMigrateLocalActionsToSelfRepository(t *testing.T) { assert.Contains(t, string(out), "uses: ./missing-action") } +func TestMigrateLocalActionsToSelfRepository_NestedCheckout(t *testing.T) { + tests := []struct { + name string + uses string + setup func(t *testing.T, repoRoot string) + }{ + {name: "leaves action inside submodule", uses: "./vendor/sub/action"}, + { + name: "leaves symlinked dir pointing into submodule", + uses: "./alias", + setup: func(t *testing.T, repoRoot string) { + symlinkOrSkip(t, filepath.Join("vendor", "sub", "action"), filepath.Join(repoRoot, "alias")) + }, + }, + { + name: "leaves action.yml symlinked into submodule", + uses: "./shim", + setup: func(t *testing.T, repoRoot string) { + shim := filepath.Join(repoRoot, "shim") + require.NoError(t, os.Mkdir(shim, 0o755)) + symlinkOrSkip(t, filepath.Join("..", "vendor", "sub", "action", "action.yml"), filepath.Join(shim, "action.yml")) + }, + }, + { + name: "leaves junction pointing into submodule", + uses: "./alias", + setup: func(t *testing.T, repoRoot string) { + junction(t, filepath.Join(repoRoot, "vendor", "sub", "action"), filepath.Join(repoRoot, "alias")) + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + repoRoot := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(repoRoot, ".git"), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Join(repoRoot, ".github", "workflows"), 0o755)) + subAction := filepath.Join(repoRoot, "vendor", "sub", "action") + require.NoError(t, os.MkdirAll(subAction, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(repoRoot, "vendor", "sub", ".git"), []byte("gitdir: ../../.git/modules/sub\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(subAction, "action.yml"), []byte("runs:\n using: composite\n"), 0o644)) + if tt.setup != nil { + tt.setup(t, repoRoot) + } + + workflowPath := filepath.Join(repoRoot, ".github", "workflows", "ci.yml") + content := []byte("jobs:\n build:\n steps:\n - uses: " + tt.uses + "\n") + require.NoError(t, os.WriteFile(workflowPath, content, 0o644)) + f, err := Load(workflowPath) + require.NoError(t, err) + + out, changed, err := f.MigrateLocalActionsToSelfRepository() + + require.NoError(t, err) + assert.Zero(t, changed, "a submodule action isn't part of this repository") + assert.Equal(t, content, out) + }) + } +} + func TestMigrateLocalActionsToSelfRepository_NoLocalPaths(t *testing.T) { content := []byte("jobs:\n build:\n steps:\n - uses: actions/checkout@v4\n") f, err := Parse("ci.yml", content) @@ -713,6 +775,98 @@ func TestDiscoverCompositeActionFiles(t *testing.T) { assert.Equal(t, want, got) } +func TestDiscoverCompositeActionFiles_NestedCheckouts(t *testing.T) { + gitFile := func(t *testing.T, dir string) { + require.NoError(t, os.WriteFile(filepath.Join(dir, ".git"), []byte("gitdir: /elsewhere\n"), 0o644)) + } + gitDir := func(t *testing.T, dir string) { + require.NoError(t, os.Mkdir(filepath.Join(dir, ".git"), 0o755)) + } + danglingGit := func(t *testing.T, dir string) { + symlinkOrSkip(t, "missing", filepath.Join(dir, ".git")) + } + tests := []struct { + name string + markRoot func(*testing.T, string) + nested string + markDir func(*testing.T, string) + wantSeen bool + }{ + {name: "skips nested worktree", markRoot: gitDir, nested: ".gwt/some-wt", markDir: gitFile}, + {name: "skips nested clone", markRoot: gitDir, nested: "vendor/clone", markDir: gitDir}, + {name: "walks root that is itself a worktree", markRoot: gitFile, nested: "sub", wantSeen: true}, + {name: "walks checkout below a root without .git", nested: "src", markDir: gitDir, wantSeen: true}, + {name: "walks dir whose .git is a dangling symlink", markRoot: gitDir, nested: "fixture", markDir: danglingGit, wantSeen: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + root := t.TempDir() + if tt.markRoot != nil { + tt.markRoot(t, root) + } + dir := filepath.Join(root, tt.nested) + require.NoError(t, os.MkdirAll(dir, 0o755)) + if tt.markDir != nil { + tt.markDir(t, dir) + } + action := filepath.Join(dir, "action.yml") + require.NoError(t, os.WriteFile(action, []byte("runs:\n using: composite\n"), 0o644)) + + got, err := DiscoverCompositeActionFiles(root) + + require.NoError(t, err) + assert.Equal(t, tt.wantSeen, slices.Contains(got, action)) + }) + } +} + +func TestDiscoverCompositeActionFiles_SymlinkIntoNestedCheckout(t *testing.T) { + root := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(root, ".git"), 0o755)) + sub := filepath.Join(root, "vendor", "sub") + require.NoError(t, os.MkdirAll(sub, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(sub, ".git"), []byte("gitdir: /elsewhere\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(sub, "action.yml"), []byte("runs:\n using: composite\n"), 0o644)) + own := filepath.Join(root, "own") + require.NoError(t, os.Mkdir(own, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(own, "action.yml"), []byte("runs:\n using: composite\n"), 0o644)) + shim := filepath.Join(root, "shim") + require.NoError(t, os.Mkdir(shim, 0o755)) + symlinkOrSkip(t, filepath.Join("..", "vendor", "sub", "action.yml"), filepath.Join(shim, "action.yml")) + inRepo := filepath.Join(root, "alias") + require.NoError(t, os.Mkdir(inRepo, 0o755)) + symlinkOrSkip(t, filepath.Join("..", "own", "action.yml"), filepath.Join(inRepo, "action.yml")) + + got, err := DiscoverCompositeActionFiles(root) + + require.NoError(t, err) + assert.NotContains(t, got, filepath.Join(shim, "action.yml"), "symlink into a nested checkout") + assert.Contains(t, got, filepath.Join(inRepo, "action.yml"), "symlink within this checkout") +} + +// symlinkOrSkip skips on Windows, where creating a symlink needs a privilege +// the user may lack (Developer Mode or admin). Elsewhere a failure is a bug in +// the test setup. +func symlinkOrSkip(t *testing.T, oldname, newname string) { + t.Helper() + err := os.Symlink(oldname, newname) + if err != nil && runtime.GOOS == "windows" { + t.Skipf("creating symlink: %v", err) + } + require.NoError(t, err) +} + +// junction creates a Windows directory junction: unlike a symlink it needs no +// privilege, and filepath.EvalSymlinks doesn't resolve it. +func junction(t *testing.T, target, link string) { + t.Helper() + if runtime.GOOS != "windows" { + t.Skip("junctions are Windows-only") + } + out, err := exec.Command("cmd", "/c", "mklink", "/J", link, target).CombinedOutput() + require.NoError(t, err, string(out)) +} + func TestDiscoverCompositeActionFiles_EmptyRoot(t *testing.T) { got, err := DiscoverCompositeActionFiles("") require.NoError(t, err)