diff --git a/pkg/ci/migrate/copy.go b/pkg/ci/migrate/copy.go index cd9fdb50..8088dda2 100644 --- a/pkg/ci/migrate/copy.go +++ b/pkg/ci/migrate/copy.go @@ -5,6 +5,7 @@ import ( "io" "os" "path/filepath" + "strings" ) // CopyMode controls behavior when destination already exists @@ -165,3 +166,82 @@ func CopyGitHubToDepot(repoRoot string, dirs []string, mode CopyMode) (*CopyResu return result, nil } + +// CopyWorkflowSiblings mirrors non-YAML files living under .github/workflows/ into +// .depot/workflows/, preserving relative layout and permissions. The workflow YAML +// itself is produced by the transform pipeline; this copies sibling assets (helper +// scripts, configs, templates) that workflows reference so those references resolve +// after migration. Symlinks are skipped. Returns the destination paths written. +func CopyWorkflowSiblings(srcWorkflowsDir, destWorkflowsDir string) ([]string, error) { + var copied []string + + err := filepath.WalkDir(srcWorkflowsDir, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + return nil + } + + ext := strings.ToLower(filepath.Ext(path)) + if ext == ".yml" || ext == ".yaml" { + return nil + } + + info, err := os.Lstat(path) + if err != nil { + return err + } + if info.Mode()&os.ModeSymlink != 0 { + return nil + } + + relPath, err := filepath.Rel(srcWorkflowsDir, path) + if err != nil { + return err + } + destPath := filepath.Join(destWorkflowsDir, relPath) + + if err := os.MkdirAll(filepath.Dir(destPath), 0755); err != nil { + return fmt.Errorf("failed to create directory %s: %w", filepath.Dir(destPath), err) + } + if err := copyFile(path, destPath, info.Mode().Perm()); err != nil { + return err + } + + copied = append(copied, destPath) + return nil + }) + if err != nil { + return nil, fmt.Errorf("failed to walk .github/workflows for sibling files: %w", err) + } + + return copied, nil +} + +// copyFile copies a regular file from src to dest, truncating dest if it exists. +func copyFile(src, dest string, perm os.FileMode) error { + srcFile, err := os.Open(src) + if err != nil { + return fmt.Errorf("failed to open source file %s: %w", src, err) + } + defer srcFile.Close() + + destFile, err := os.OpenFile(dest, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, perm) + if err != nil { + return fmt.Errorf("failed to create destination file %s: %w", dest, err) + } + defer destFile.Close() + + if _, err := io.Copy(destFile, srcFile); err != nil { + return fmt.Errorf("failed to copy file %s to %s: %w", src, dest, err) + } + + // OpenFile only applies perm when creating the file; when overwriting an + // existing sibling (a re-run over an already-migrated tree) the old mode + // sticks, so set it explicitly to keep the source's permissions. + if err := os.Chmod(dest, perm); err != nil { + return fmt.Errorf("failed to set permissions on %s: %w", dest, err) + } + return nil +} diff --git a/pkg/ci/migrate/copy_test.go b/pkg/ci/migrate/copy_test.go index 6ff7b0b7..7b2aa599 100644 --- a/pkg/ci/migrate/copy_test.go +++ b/pkg/ci/migrate/copy_test.go @@ -338,3 +338,93 @@ func TestCopyMissingSubDir(t *testing.T) { t.Errorf("workflows/ci.yml not found: %v", err) } } + +// TestCopyWorkflowSiblings verifies that non-YAML sibling files under +// .github/workflows/ are mirrored into .depot/workflows/ while YAML workflows and +// symlinks are skipped. +func TestCopyWorkflowSiblings(t *testing.T) { + tmpDir := t.TempDir() + srcDir := filepath.Join(tmpDir, ".github", "workflows") + if err := os.MkdirAll(filepath.Join(srcDir, "scripts"), 0755); err != nil { + t.Fatalf("failed to create source dir: %v", err) + } + + // YAML workflow — should NOT be copied by this helper. + if err := os.WriteFile(filepath.Join(srcDir, "ci.yml"), []byte("name: CI\n"), 0644); err != nil { + t.Fatal(err) + } + // Sibling script at the top level and in a subdirectory — should be copied. + if err := os.WriteFile(filepath.Join(srcDir, "helper.sh"), []byte("#!/bin/sh\necho hi\n"), 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(srcDir, "scripts", "deploy.sh"), []byte("#!/bin/sh\n"), 0644); err != nil { + t.Fatal(err) + } + + destDir := filepath.Join(tmpDir, ".depot", "workflows") + copied, err := CopyWorkflowSiblings(srcDir, destDir) + if err != nil { + t.Fatalf("CopyWorkflowSiblings failed: %v", err) + } + + if len(copied) != 2 { + t.Errorf("expected 2 sibling files copied, got %d: %v", len(copied), copied) + } + + // YAML must not be mirrored by this helper. + if _, err := os.Stat(filepath.Join(destDir, "ci.yml")); !os.IsNotExist(err) { + t.Errorf("expected ci.yml NOT to be copied, stat err: %v", err) + } + + // Siblings must exist with content and layout preserved. + helper := filepath.Join(destDir, "helper.sh") + if data, err := os.ReadFile(helper); err != nil { + t.Errorf("expected helper.sh copied: %v", err) + } else if !strings.Contains(string(data), "echo hi") { + t.Errorf("helper.sh content not preserved: %q", data) + } + // Executable bit preserved. + if info, err := os.Stat(helper); err != nil { + t.Errorf("stat helper.sh: %v", err) + } else if info.Mode().Perm()&0100 == 0 { + t.Errorf("expected helper.sh to stay executable, got %v", info.Mode().Perm()) + } + if _, err := os.Stat(filepath.Join(destDir, "scripts", "deploy.sh")); err != nil { + t.Errorf("expected scripts/deploy.sh copied preserving layout: %v", err) + } +} + +// TestCopyWorkflowSiblings_OverwritePreservesMode verifies that re-copying over an +// existing destination file resets its mode to match the source, since OpenFile only +// applies the mode when creating a new file. +func TestCopyWorkflowSiblings_OverwritePreservesMode(t *testing.T) { + tmpDir := t.TempDir() + srcDir := filepath.Join(tmpDir, ".github", "workflows") + destDir := filepath.Join(tmpDir, ".depot", "workflows") + if err := os.MkdirAll(srcDir, 0755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(destDir, 0755); err != nil { + t.Fatal(err) + } + + // Executable source, but a pre-existing non-executable destination. + if err := os.WriteFile(filepath.Join(srcDir, "helper.sh"), []byte("#!/bin/sh\n"), 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(destDir, "helper.sh"), []byte("stale\n"), 0600); err != nil { + t.Fatal(err) + } + + if _, err := CopyWorkflowSiblings(srcDir, destDir); err != nil { + t.Fatalf("CopyWorkflowSiblings failed: %v", err) + } + + info, err := os.Stat(filepath.Join(destDir, "helper.sh")) + if err != nil { + t.Fatal(err) + } + if info.Mode().Perm() != 0755 { + t.Errorf("expected overwritten sibling mode 0755, got %v", info.Mode().Perm()) + } +} diff --git a/pkg/ci/transform/transform.go b/pkg/ci/transform/transform.go index fe779afd..1e7b6376 100644 --- a/pkg/ci/transform/transform.go +++ b/pkg/ci/transform/transform.go @@ -27,6 +27,7 @@ const ( ChangeTriggerRemoved // Unsupported trigger was removed ChangeJobDisabled // Entire job was commented out ChangePathRewritten // .github/ path was rewritten to .depot/ + ChangeSparseCheckout // .depot/ paths added alongside .github/ in a checkout sparse-checkout ) // ChangeRecord describes a single change made during transformation. @@ -77,11 +78,22 @@ func TransformWorkflow(raw []byte, wf *migrate.WorkflowFile, report *compat.Comp runsOnChanges := transformRunsOn(root, disabledJobs) changes = append(changes, runsOnChanges...) - // 4. Rewrite .github/ path references to .depot/ - pathChanges := transformGitHubPaths(root, migratedWorkflows) + // 4. Handle actions/checkout sparse-checkout inputs before the generic path + // pass. These keep their .github/ entries and gain .depot/ siblings, so the + // generic pass must skip them (see transformSparseCheckout). + sparseSkip, sparseChanges := transformSparseCheckout(root, migratedWorkflows) + changes = append(changes, sparseChanges...) + + // 5. Rewrite .github/ path references to .depot/ + pathChanges := transformGitHubPaths(root, migratedWorkflows, sparseSkip) changes = append(changes, pathChanges...) - // 5. Marshal the node tree back to bytes + // 6. Strip trailing whitespace from POSIX-shell step `run:` block scalars so yaml.v3 + // keeps literal/folded style (e.g. `run: |`) instead of flattening to a quoted + // string. Non-shell inputs and non-POSIX shells are left byte-exact. + sanitizeRunBlockScalars(root) + + // 7. Marshal the node tree back to bytes var buf bytes.Buffer enc := yaml.NewEncoder(&buf) enc.SetIndent(2) @@ -92,7 +104,7 @@ func TransformWorkflow(raw []byte, wf *migrate.WorkflowFile, report *compat.Comp output := buf.Bytes() - // 6. Post-process: comment out disabled jobs in text + // 8. Post-process: comment out disabled jobs in text if len(disabledJobs) > 0 { var disableChanges []ChangeRecord output, disableChanges = commentOutDisabledJobs(output, disabledJobs) @@ -107,7 +119,7 @@ func TransformWorkflow(raw []byte, wf *migrate.WorkflowFile, report *compat.Comp } } - // 7. Prepend header comment + // 9. Prepend header comment header := buildHeaderComment(wf, changes) output = append([]byte(header), output...) @@ -328,7 +340,9 @@ func transformRunsOnNode(node *yaml.Node, jobName string) []ChangeRecord { // transformGitHubPaths walks all nodes and rewrites local .github/ references to .depot/ // in both scalar values and YAML comments (HeadComment, LineComment, FootComment). // Remote references like org/repo/.github/workflows/reusable.yml@ref are left untouched. -func transformGitHubPaths(node *yaml.Node, migratedWorkflows map[string]bool) []ChangeRecord { +// Nodes in skip are left entirely alone; sparse-checkout values handled by +// transformSparseCheckout live there so their .github/ entries are preserved. +func transformGitHubPaths(node *yaml.Node, migratedWorkflows map[string]bool, skip map[*yaml.Node]bool) []ChangeRecord { rewrote := false rewrite := func(s string) string { result, changed := rewriteGitHubPaths(s, migratedWorkflows) @@ -338,6 +352,9 @@ func transformGitHubPaths(node *yaml.Node, migratedWorkflows map[string]bool) [] return result } walkNodes(node, func(n *yaml.Node) { + if skip[n] { + return + } if n.Kind == yaml.ScalarNode { n.Value = rewrite(n.Value) } @@ -361,13 +378,21 @@ func transformGitHubPaths(node *yaml.Node, migratedWorkflows map[string]bool) [] } var ( - // githubPathRe matches .github/actions or .github/workflows references. - githubPathRe = regexp.MustCompile(`\.github/(actions|workflows)`) + // githubPathRe matches .github/actions or .github/workflows references. The + // separator group tolerates a backslash-escaped slash (\/) so paths embedded in + // regexes or escaped string literals inside action sources — e.g. \.github\/actions + // — are matched the same as plain paths in fixtures. The captured separator is + // reused in the replacement so the escaping style is preserved. + githubPathRe = regexp.MustCompile(`\.github(\\?/)(actions|workflows)`) // remoteRefRe matches owner/repo/.github/(actions|workflows) patterns. // The char class [a-zA-Z0-9_.-] naturally excludes expression characters ($, {, }), // so expression-expanded paths like "${{ workspace }}/.github/" won't match. - remoteRefRe = regexp.MustCompile(`[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+/\.github/(?:actions|workflows)`) + // Each separator, and the dot before "github", tolerates a backslash-escape (\/ + // and \.) so escaped remote refs embedded in code strings — e.g. + // org\/repo\/\.github\/actions — are still classified as remote and left alone, + // matching how githubPathRe treats a preceding backslash as a boundary. + remoteRefRe = regexp.MustCompile(`[a-zA-Z0-9_.-]+\\?/[a-zA-Z0-9_.-]+\\?/\\?\.github\\?/(?:actions|workflows)`) // pathTailRe captures a file path segment after a / delimiter, stopping at // whitespace or shell metacharacters. @@ -395,14 +420,15 @@ func rewriteGitHubPaths(s string, migratedWorkflows map[string]bool) (string, bo for _, m := range candidates { start, end := m[0], m[1] - subdir := s[m[2]:m[3]] + sep := s[m[2]:m[3]] + subdir := s[m[4]:m[5]] if !shouldRewrite(s, start, end, subdir, migratedWorkflows, remoteSpans) { continue } b.WriteString(s[last:start]) - b.WriteString(".depot/" + subdir) + b.WriteString(".depot" + sep + subdir) last = end changed = true } @@ -415,14 +441,28 @@ func rewriteGitHubPaths(s string, migratedWorkflows map[string]bool) (string, bo } // boundaryChars are characters that can validly precede ".github/" as a path reference. -// Prevents matching inside longer names like "myapp.github/actions". -const boundaryChars = "/ \t\n\"'();|&=" +// Prevents matching inside longer names like "myapp.github/actions". The backslash is +// included so regex/escaped-string forms in action sources (\.github/actions) are +// treated as references, matching how the same paths in fixtures are rewritten. +const boundaryChars = "/ \t\n\"'();|&=\\" // shouldRewrite decides whether a .github/(actions|workflows) match at [start:end] // should be replaced with .depot/. func shouldRewrite(s string, start, end int, subdir string, migratedWorkflows map[string]bool, remoteSpans [][]int) bool { - if start > 0 && !strings.ContainsRune(boundaryChars, rune(s[start-1])) { - return false + if start > 0 { + prev := s[start-1] + if prev == '\\' { + // An escaped dot (\.github) is only a path boundary when the backslash + // itself sits at a boundary. If a path character precedes the backslash — + // e.g. a regex like `myapp\.github/actions` — this is a longer token, not a + // path reference, and must be skipped just as the unescaped form + // (myapp.github/actions) is. + if start >= 2 && !strings.ContainsRune(boundaryChars, rune(s[start-2])) { + return false + } + } else if !strings.ContainsRune(boundaryChars, rune(prev)) { + return false + } } // Must not continue into a longer dir name (e.g., ".github/actions-custom") @@ -452,11 +492,27 @@ func shouldRewrite(s string, start, end int, subdir string, migratedWorkflows ma // Bare directory refs (e.g., "ls .github/workflows") are skipped when filtering // is active since the directory still partially lives at .github/. if subdir == "workflows" && migratedWorkflows != nil { - if end >= len(s) || s[end] != '/' { + // The separator after "workflows" may itself be escaped (\/) when the path + // is embedded in a regex or escaped string, so consume either form before + // reading the filename tail. + rest := s[end:] + switch { + case strings.HasPrefix(rest, "/"): + rest = rest[1:] + case strings.HasPrefix(rest, "\\/"): + rest = rest[2:] + default: + return false + } + tail := pathTailRe.FindString(rest) + if tail == "" { return false } - tail := pathTailRe.FindString(s[end+1:]) - if tail == "" || !migratedWorkflows[tail] { + // The tail itself may carry escaped separators (scripts\/build.sh) when the + // reference is embedded in a regex or escaped string. The allow-list is keyed + // by plain slash paths (scripts/build.sh), so unescape before the lookup — + // otherwise an escaped nested-sibling reference would be left at .github/. + if !migratedWorkflows[strings.ReplaceAll(tail, "\\/", "/")] { return false } } @@ -485,6 +541,13 @@ func isURL(s string, idx int) bool { if i >= 2 && s[i-2:i+1] == "://" { return true } + // Also recognize a backslash-escaped protocol separator (":\/"), which is how + // a URL appears when embedded in an escaped string literal or regex — e.g. + // "https:\/\/github.com\/org\/repo\/.github\/actions". Without this, the + // escaped-slash matching would rewrite .github inside such URLs. + if i >= 2 && s[i-2:i+1] == ":\\/" { + return true + } } return false } @@ -500,6 +563,403 @@ func walkNodes(node *yaml.Node, fn func(*yaml.Node)) { } } +// sanitizeRunBlockScalars strips trailing whitespace from the lines of shell-step `run:` +// block scalars. yaml.v3's emitter refuses block style for any scalar with a line ending +// in a space or tab and silently falls back to a double-quoted, single-line string — +// flattening a readable `run: |` block into "\n"-escaped text. In a POSIX shell that +// trailing whitespace is inert (modulo the quote and continuation cases +// trimTrailingWhitespace guards), so trimming it restores the block style, and with it +// review legibility, without changing what runs. +// +// It is deliberately narrow on two axes so it never touches data: +// - Only genuine step `run:` fields under jobs..steps[] are considered. A block +// scalar passed to an action input that happens to be named `run` (under `with:`) +// is not a shell command and is left byte-exact, as are all other block scalars +// (e.g. `with: body: |`, whose trailing spaces may be Markdown hard breaks). +// - Only steps whose effective shell is POSIX (bash/sh, including the unset default on +// the Linux runners Depot CI targets) are trimmed. Under cmd, PowerShell, python, +// and the like trailing spaces can be significant (e.g. cmd `set NAME=value `), so +// those run blocks are left byte-exact and yaml.v3 keeps them via its quoted fallback. +func sanitizeRunBlockScalars(root *yaml.Node) { + _, jobsVal := findMappingKey(root, "jobs") + if jobsVal == nil || jobsVal.Kind != yaml.MappingNode { + return + } + + workflowShell := defaultRunShell(root) + + for i := 1; i < len(jobsVal.Content); i += 2 { + job := jobsVal.Content[i] + if job.Kind != yaml.MappingNode { + continue + } + + jobShell := defaultRunShell(job) + if jobShell == "" { + jobShell = workflowShell + } + + _, steps := findMappingKey(job, "steps") + if steps == nil || steps.Kind != yaml.SequenceNode { + continue + } + for _, step := range steps.Content { + if step.Kind != yaml.MappingNode { + continue + } + _, runVal := findMappingKey(step, "run") + if runVal == nil || runVal.Kind != yaml.ScalarNode { + continue + } + if runVal.Style != yaml.LiteralStyle && runVal.Style != yaml.FoldedStyle { + continue + } + + shell := "" + if _, shellVal := findMappingKey(step, "shell"); shellVal != nil && shellVal.Kind == yaml.ScalarNode { + shell = shellVal.Value + } + if shell == "" { + shell = jobShell + } + if !isPOSIXShell(shell) { + continue + } + + trimTrailingWhitespace(runVal) + } + } +} + +// defaultRunShell returns the defaults.run.shell value declared on a workflow- or +// job-level mapping, or "" when unset. +func defaultRunShell(m *yaml.Node) string { + _, defaults := findMappingKey(m, "defaults") + if defaults == nil || defaults.Kind != yaml.MappingNode { + return "" + } + _, run := findMappingKey(defaults, "run") + if run == nil || run.Kind != yaml.MappingNode { + return "" + } + _, shell := findMappingKey(run, "shell") + if shell == nil || shell.Kind != yaml.ScalarNode { + return "" + } + return shell.Value +} + +// isPOSIXShell reports whether a run step's effective shell is one where trailing +// whitespace in the command text is inert. An empty value is the GitHub Actions default, +// which is bash (falling back to sh) on the Linux and macOS runners Depot CI targets. +// Anything else — pwsh, cmd, python, or a custom template like "perl {0}" — may attach +// meaning to trailing whitespace, so those run blocks are left byte-exact. +func isPOSIXShell(shell string) bool { + switch shell { + case "", "bash", "sh": + return true + default: + return false + } +} + +// trimTrailingWhitespace strips trailing spaces and tabs from each line of a block +// scalar's value, but only on lines where that whitespace is provably inert. It leaves +// bytes untouched in the three cases where trailing whitespace is load-bearing in shell: +// heredoc bodies, escaped line continuations, and text inside a quote that spans lines. +func trimTrailingWhitespace(n *yaml.Node) { + // A heredoc payload can legitimately depend on trailing whitespace, and we can't + // tell a heredoc body from ordinary commands at this layer, so leave any scalar + // containing a heredoc operator untouched — yaml.v3's quoted fallback keeps it + // byte-exact. Ordinary run blocks (the common case) have no heredoc and are still + // tidied back into readable block style. + if strings.Contains(n.Value, "<<") { + return + } + lines := strings.Split(n.Value, "\n") + inSingle, inDouble := false, false + for i := range lines { + // A line's trailing whitespace is only safe to trim when the line does not + // end inside an open single- or double-quoted string. If a quote is still + // open, those spaces are string data continuing onto the next line — e.g. + // `printf '%s' 'foo ` closing as `bar'` two lines down — and trimming them + // would change the value the shell sees. Track quote state across lines and + // skip any line that ends mid-quote; yaml.v3 then keeps that scalar quoted. + openAtEnd := quoteStateAtLineEnd(lines[i], &inSingle, &inDouble) + if openAtEnd { + continue + } + trimmed := strings.TrimRight(lines[i], " \t") + // Leave a line whose trimmed form ends in a line-continuation escape untouched. + // Trimming "foo \ " (a literal escaped space) down to "foo \" would splice + // this line onto the next, changing what the migrated workflow executes. The + // continuation character is shell-specific — backslash in POSIX sh, backtick in + // PowerShell, caret in cmd — so guard all three since a run step may set any of + // them via `shell:`. Such a line keeps its trailing whitespace, so yaml.v3 still + // quotes that one scalar; correctness wins over tidiness in this rare case. + if n := len(trimmed); n > 0 { + switch trimmed[n-1] { + case '\\', '`', '^': + continue + } + } + lines[i] = trimmed + } + n.Value = strings.Join(lines, "\n") +} + +// quoteStateAtLineEnd scans a single line of shell text, advancing the single- and +// double-quote state carried across lines, and reports whether a quote is still open +// at the end of the line. It models the quoting rules that matter here: single quotes +// are literal (no escapes, only another ' closes them); inside double quotes a backslash +// (POSIX sh) or backtick (PowerShell) escapes the next character; and a backslash outside +// quotes escapes the next character too. It is intentionally conservative — exotic forms +// like $'...' ANSI-C quoting are not modeled — but every simplification errs toward +// treating a quote as still open, which only ever leaves a scalar quoted (less tidy) and +// never trims whitespace that was actually inside a quote. +func quoteStateAtLineEnd(line string, inSingle, inDouble *bool) bool { + for i := 0; i < len(line); i++ { + c := line[i] + switch { + case *inSingle: + if c == '\'' { + *inSingle = false + } + case *inDouble: + switch c { + case '\\', '`': + i++ // \ (POSIX sh) or ` (PowerShell) escapes the next char inside double quotes + case '"': + *inDouble = false + } + default: + switch c { + case '\'': + *inSingle = true + case '"': + *inDouble = true + case '\\': + i++ // escaped char outside quotes + } + } + } + return *inSingle || *inDouble +} + +// isCheckoutAction reports whether a `uses` value refers to actions/checkout. +func isCheckoutAction(uses string) bool { + name := uses + if i := strings.IndexByte(name, '@'); i >= 0 { + name = name[:i] + } + return name == "actions/checkout" +} + +// transformSparseCheckout augments actions/checkout `sparse-checkout` inputs instead of +// letting the generic path pass rewrite them. Rewriting a sparse-checkout entry from +// .github/ to .depot/ stops git from materializing .github/, which breaks steps that +// still reference scripts living under .github/ — only .github/actions and the selected +// .github/workflows move to .depot/, so everything else legitimately stays. To keep both +// working, each .github/(actions|workflows) entry is preserved and a .depot/ sibling is +// added, making the checkout a superset. The returned set marks the handled value nodes +// (and their sequence items) so transformGitHubPaths leaves the .github/ entries alone. +func transformSparseCheckout(root *yaml.Node, migratedWorkflows map[string]bool) (map[*yaml.Node]bool, []ChangeRecord) { + skip := make(map[*yaml.Node]bool) + augmented := false + + _, jobsVal := findMappingKey(root, "jobs") + if jobsVal == nil || jobsVal.Kind != yaml.MappingNode { + return skip, nil + } + + for i := 1; i < len(jobsVal.Content); i += 2 { + job := jobsVal.Content[i] + if job.Kind != yaml.MappingNode { + continue + } + _, steps := findMappingKey(job, "steps") + if steps == nil || steps.Kind != yaml.SequenceNode { + continue + } + for _, step := range steps.Content { + if step.Kind != yaml.MappingNode { + continue + } + _, uses := findMappingKey(step, "uses") + if uses == nil || uses.Kind != yaml.ScalarNode || !isCheckoutAction(uses.Value) { + continue + } + _, with := findMappingKey(step, "with") + if with == nil || with.Kind != yaml.MappingNode { + continue + } + _, sparse := findMappingKey(with, "sparse-checkout") + if sparse == nil { + continue + } + // Always skip the sparse-checkout value so the generic pass never + // rewrites its .github/ entries, whether or not we add siblings. + skip[sparse] = true + for _, item := range sparse.Content { + skip[item] = true + } + if augmentSparseCheckout(sparse, migratedWorkflows) { + augmented = true + } + } + } + + if !augmented { + return skip, nil + } + return skip, []ChangeRecord{{ + Type: ChangeSparseCheckout, + Detail: "Added .depot/ paths alongside .github/ in checkout sparse-checkout (kept .github/ so steps that still reference it keep working)", + }} +} + +// rewriteSparseEntry computes the .depot/ sibling for a single sparse-checkout entry, +// preserving a leading "!" negation. In non-cone mode sparse-checkout accepts +// gitignore-style patterns, so `!.github/actions/cache` excludes that path; mirroring it +// to `!.depot/actions/cache` keeps the migrated checkout from re-including files the +// original explicitly excluded. +// +// The .depot/ path is computed with the full (nil) filter so that even a bare +// ".github/workflows" directory entry gains its .depot/workflows counterpart — a +// partial filter would decline that (correctly, for a run: reference), but sparse-checkout +// must still materialize the .depot/ tree the migration produced. To avoid the reverse +// mistake — pointing sparse-checkout at a .depot/workflows/ that was never copied — +// a specific workflow-file entry is only mirrored when that file is actually in the +// partial allow-list (see sparseWorkflowCovered). Returns whether a sibling exists. +func rewriteSparseEntry(entry string, migratedWorkflows map[string]bool) (string, bool) { + neg := "" + body := entry + if strings.HasPrefix(body, "!") { + neg = "!" + body = body[1:] + } + if migratedWorkflows != nil && !sparseWorkflowCovered(body, migratedWorkflows) { + return entry, false + } + depot, ok := rewriteGitHubPaths(body, nil) + if !ok { + return entry, false + } + return neg + depot, true +} + +// sparseWorkflowCovered reports whether a sparse-checkout entry's .depot/ counterpart +// will actually contain something after a partial migration. Actions always migrate, so +// any non-workflows entry maps. A bare ".github/workflows" directory maps because it +// holds the migrated workflows and their copied siblings. A glob under workflows +// (e.g. ".github/workflows/*.yml") maps because it is self-limiting — it includes only +// the migrated files that exist under .depot/workflows. A specific literal +// ".github/workflows/" maps only when is a migrated file or a directory +// prefix of one — otherwise .depot/workflows/ would not exist and the mirrored +// pattern would match nothing. +func sparseWorkflowCovered(entry string, migratedWorkflows map[string]bool) bool { + const wf = ".github/workflows" + idx := strings.Index(entry, wf) + if idx < 0 { + return true // not a workflows entry (e.g. .github/actions) — always maps + } + rest := strings.TrimPrefix(entry[idx+len(wf):], "/") + tail := strings.TrimSuffix(rest, "/") + if tail == "" { + return true // the bare .github/workflows directory + } + // A glob pattern (gitignore-style, as non-cone sparse-checkout accepts) is + // self-limiting: mirroring ".github/workflows/*.yml" to ".depot/workflows/*.yml" + // includes whatever migrated files landed under .depot/workflows and matches nothing + // otherwise. Mirroring it is therefore always safe — and necessary, since the glob is + // how the original checked those files out. Only a literal path asserts one specific + // file, so only a literal tail is gated on actual coverage below. + if strings.ContainsAny(tail, "*?[") { + return true + } + for k := range migratedWorkflows { + if k == tail || strings.HasPrefix(k, tail+"/") { + return true + } + } + return false +} + +// augmentSparseCheckout adds a .depot/ sibling for each .github/(actions|workflows) +// entry in a sparse-checkout value, leaving the original entries in place. See +// rewriteSparseEntry for how the .depot/ path is derived under a partial migration. +// Returns whether anything was added. +// +// The mirror is order-faithful: each .depot/ sibling keeps the relative position of its +// .github/ source (inserted right after it in a block scalar, appended in source order in +// a sequence). Non-cone sparse-checkout applies gitignore-style rules where the last +// matching pattern wins, so preserving order is what makes the .depot/ patterns exclude +// and include in the same sequence the author intended for .github/ — e.g. a +// `.depot/actions` include followed by a `!.depot/actions/cache` exclude drops the cache +// exactly as the original did. Because .github/ and .depot/ patterns live under different +// top-level directories they never cross-match, so a .github/ line interleaved among the +// .depot/ lines cannot change which .depot/ paths materialize; the outcome depends only on +// the order among the .depot/ patterns, which equals the order among their .github/ +// sources. Reordering the mirror (e.g. hoisting includes above excludes) would instead +// make the migrated checkout diverge from the source's own semantics. +func augmentSparseCheckout(node *yaml.Node, migratedWorkflows map[string]bool) bool { + switch node.Kind { + case yaml.ScalarNode: + lines := strings.Split(node.Value, "\n") + existing := make(map[string]bool, len(lines)) + for _, l := range lines { + existing[strings.TrimSpace(l)] = true + } + var out []string + changed := false + for _, l := range lines { + out = append(out, l) + trimmed := strings.TrimSpace(l) + if trimmed == "" { + continue + } + depot, ok := rewriteSparseEntry(trimmed, migratedWorkflows) + if !ok || depot == trimmed || existing[depot] { + continue + } + indent := l[:len(l)-len(strings.TrimLeft(l, " \t"))] + out = append(out, indent+depot) + existing[depot] = true + changed = true + } + if changed { + node.Value = strings.Join(out, "\n") + } + return changed + + case yaml.SequenceNode: + existing := make(map[string]bool, len(node.Content)) + for _, item := range node.Content { + if item.Kind == yaml.ScalarNode { + existing[item.Value] = true + } + } + var additions []*yaml.Node + for _, item := range node.Content { + if item.Kind != yaml.ScalarNode { + continue + } + depot, ok := rewriteSparseEntry(item.Value, migratedWorkflows) + if !ok || depot == item.Value || existing[depot] { + continue + } + additions = append(additions, &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: depot}) + existing[depot] = true + } + if len(additions) > 0 { + node.Content = append(node.Content, additions...) + return true + } + return false + } + return false +} + // RewriteGitHubPathsInDir walks a directory and rewrites .github/ → .depot/ references // in all text files. Binary files and symlinks are skipped. Original file permissions // are preserved. This is used for copied action files that aren't processed through @@ -518,35 +978,51 @@ func RewriteGitHubPathsInDir(dir string, migratedWorkflows map[string]bool) (int if d.IsDir() || d.Type()&os.ModeSymlink != 0 { return nil } - - info, err := d.Info() + changed, err := RewriteGitHubPathsInFile(path, migratedWorkflows) if err != nil { - return fmt.Errorf("failed to stat %s: %w", path, err) - } - - raw, err := os.ReadFile(path) - if err != nil { - return fmt.Errorf("failed to read %s: %w", path, err) - } - - if isBinary(raw) { - return nil - } - - result, changed := rewriteGitHubPaths(string(raw), migratedWorkflows) - if !changed { - return nil + return err } - - if err := os.WriteFile(path, []byte(result), info.Mode().Perm()); err != nil { - return fmt.Errorf("failed to write %s: %w", path, err) + if changed { + rewritten++ } - rewritten++ return nil }) return rewritten, err } +// RewriteGitHubPathsInFile rewrites .github/ → .depot/ references in a single text +// file, skipping symlinks and binary files and preserving the file's permissions. +// It reports whether the file was modified. Used for copied assets (action files and +// workflow sibling files) that don't pass through the YAML transform pipeline. +func RewriteGitHubPathsInFile(path string, migratedWorkflows map[string]bool) (bool, error) { + info, err := os.Lstat(path) + if err != nil { + return false, fmt.Errorf("failed to stat %s: %w", path, err) + } + if info.Mode()&os.ModeSymlink != 0 { + return false, nil + } + + raw, err := os.ReadFile(path) + if err != nil { + return false, fmt.Errorf("failed to read %s: %w", path, err) + } + + if isBinary(raw) { + return false, nil + } + + result, changed := rewriteGitHubPaths(string(raw), migratedWorkflows) + if !changed { + return false, nil + } + + if err := os.WriteFile(path, []byte(result), info.Mode().Perm()); err != nil { + return false, fmt.Errorf("failed to write %s: %w", path, err) + } + return true, nil +} + // commentOutDisabledJobs does a text-level pass to comment out entire job blocks. func commentOutDisabledJobs(content []byte, disabledJobs map[string]disabledJobInfo) ([]byte, []ChangeRecord) { if len(disabledJobs) == 0 { diff --git a/pkg/ci/transform/transform_test.go b/pkg/ci/transform/transform_test.go index 250e53ff..fad49b48 100644 --- a/pkg/ci/transform/transform_test.go +++ b/pkg/ci/transform/transform_test.go @@ -9,6 +9,7 @@ import ( "github.com/depot/cli/pkg/ci/compat" "github.com/depot/cli/pkg/ci/migrate" + "gopkg.in/yaml.v3" ) func TestTransformWorkflow_NoChanges(t *testing.T) { @@ -796,6 +797,68 @@ jobs: } } +func TestTransformWorkflow_PartialMigration_SiblingScriptPath(t *testing.T) { + // A migrated workflow references a sibling helper script that lives under + // .github/workflows/. Under a partial migration the rewrite is gated by the + // migratedWorkflows set, keyed by path relative to the workflows dir — so a nested + // script key like "scripts/build.sh" must gate its reference the same way a + // top-level workflow filename does. This is the membership check the command layer + // relies on after it adds copied siblings to the set. + raw := []byte(`name: CI +on: push +jobs: + build: + runs-on: depot-ubuntu-latest + steps: + - run: bash .github/workflows/scripts/build.sh + - run: bash .github/workflows/scripts/other.sh +`) + + wf := &migrate.WorkflowFile{ + Path: ".github/workflows/ci.yml", + Name: "CI", + Triggers: []string{"push"}, + Jobs: []migrate.JobInfo{{Name: "build", RunsOn: "depot-ubuntu-latest"}}, + } + report := compat.AnalyzeWorkflow(wf) + + // build.sh was copied (so it joins the set); other.sh was not. + migratedWorkflows := map[string]bool{ + "ci.yml": true, + "scripts/build.sh": true, + } + + result, err := TransformWorkflow(raw, wf, report, migratedWorkflows) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + content := string(result.Content) + if !strings.Contains(content, ".depot/workflows/scripts/build.sh") { + t.Errorf("expected copied sibling reference rewritten to .depot/, got:\n%s", content) + } + if !strings.Contains(content, ".github/workflows/scripts/other.sh") { + t.Errorf("expected un-copied sibling reference left at .github/, got:\n%s", content) + } +} + +func TestRewriteGitHubPaths_PartialMigration_EscapedSiblingTail(t *testing.T) { + // An escaped nested-sibling reference embedded in a code string keeps escaped + // separators in its tail (scripts\/build.sh). The allow-list is keyed by plain + // slash paths, so the tail must be unescaped before the lookup — otherwise the + // copied sibling reference would be left pointing at .github/. + set := map[string]bool{"scripts/build.sh": true} + input := `node -e "require('fs').existsSync('.github\/workflows\/scripts\/build.sh')"` + + got, changed := rewriteGitHubPaths(input, set) + if !changed { + t.Fatalf("expected escaped sibling reference rewritten, got unchanged: %q", got) + } + if !strings.Contains(got, `.depot\/workflows\/scripts\/build.sh`) { + t.Errorf("expected escaped tail rewritten to .depot/, got:\n%s", got) + } +} + func TestRewriteGitHubPaths(t *testing.T) { tests := []struct { name string @@ -1001,6 +1064,48 @@ func TestRewriteGitHubPaths(t *testing.T) { want: "(.depot/actions/setup/run.sh)", changed: true, }, + { + name: "regex escaped dot rewritten", + input: `const re = /\.github\/actions\//`, + want: `const re = /\.depot\/actions\//`, + changed: true, + }, + { + name: "regex escaped dot literal slash rewritten", + input: `\.github/actions/setup`, + want: `\.depot/actions/setup`, + changed: true, + }, + { + name: "escaped dot inside longer token not rewritten", + input: `myapp\.github/actions/setup`, + want: `myapp\.github/actions/setup`, + changed: false, + }, + { + name: "python raw regex workflows rewritten", + input: `re.compile(r"\.github\/workflows")`, + want: `re.compile(r"\.depot\/workflows")`, + changed: true, + }, + { + name: "escaped-slash url left untouched", + input: `"https:\/\/github.com\/org\/repo\/tree\/main\/.github\/actions"`, + want: `"https:\/\/github.com\/org\/repo\/tree\/main\/.github\/actions"`, + changed: false, + }, + { + name: "escaped-slash remote ref left untouched", + input: `uses: org\/repo\/.github\/workflows\/reusable.yml`, + want: `uses: org\/repo\/.github\/workflows\/reusable.yml`, + changed: false, + }, + { + name: "fully escaped remote ref left untouched", + input: `ref: org\/repo\/\.github\/actions\/setup`, + want: `ref: org\/repo\/\.github\/actions\/setup`, + changed: false, + }, } for _, tt := range tests { @@ -1234,3 +1339,453 @@ func TestBuildHeaderComment_WithNonstandardChanges(t *testing.T) { t.Errorf("expected nonstandard label detail in header, got: %s", header) } } + +func TestSanitizeBlockScalars_PreservesBackslashContinuation(t *testing.T) { + // A line ending in a backslash followed by whitespace is a literal escaped + // space in shell, not a line continuation. Trimming it down to a trailing + // backslash would change what the command does, so it must be left intact — + // while an ordinary trailing-whitespace line is still trimmed. + n := &yaml.Node{ + Kind: yaml.ScalarNode, + Style: yaml.LiteralStyle, + Value: "echo foo \\ \necho bar \n", + } + trimTrailingWhitespace(n) + + lines := strings.Split(n.Value, "\n") + if lines[0] != "echo foo \\ " { + t.Errorf("expected backslash-continuation line preserved, got %q", lines[0]) + } + if lines[1] != "echo bar" { + t.Errorf("expected ordinary trailing whitespace trimmed, got %q", lines[1]) + } +} + +func TestSanitizeBlockScalars_PreservesHeredoc(t *testing.T) { + // A heredoc payload may depend on trailing whitespace, so a scalar containing + // a heredoc operator is left entirely untouched rather than tidied. + original := "cat < inclLine { + t.Errorf("expected mirrored !.depot/actions/cache to precede .depot/actions (source order), got:\n%s", content) + } +} + +func TestTransformWorkflow_SparseCheckoutSuperset_PartialMigration(t *testing.T) { + // Under a partial migration the sparse-checkout superset must only point at + // .depot/ paths that actually exist post-migration. .github/actions always + // migrates, and a bare .github/workflows directory holds the migrated content, + // so both gain a .depot/ sibling. A specific workflow file that was NOT copied + // (other.yml) must not gain a .depot/workflows/other.yml sibling — that path + // would not exist — while a migrated one (ci.yml) must. + raw := []byte("name: CI\non: push\njobs:\n build:\n runs-on: depot-ubuntu-latest\n steps:\n - uses: actions/checkout@v4\n with:\n sparse-checkout: |\n .github/workflows\n .github/workflows/ci.yml\n .github/workflows/other.yml\n .github/actions\n") + + wf := &migrate.WorkflowFile{ + Path: ".github/workflows/ci.yml", + Name: "CI", + Triggers: []string{"push"}, + Jobs: []migrate.JobInfo{{Name: "build", RunsOn: "depot-ubuntu-latest"}}, + } + report := compat.AnalyzeWorkflow(wf) + + // Only ci.yml was migrated; other.yml stays at .github/. + migratedWorkflows := map[string]bool{"ci.yml": true} + + result, err := TransformWorkflow(raw, wf, report, migratedWorkflows) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + content := string(result.Content) + // The bare workflows directory, the migrated file, and actions all gain siblings. + if !strings.Contains(content, ".depot/workflows\n") && !strings.Contains(content, ".depot/workflows ") { + t.Errorf("expected bare .depot/workflows directory added, got:\n%s", content) + } + if !strings.Contains(content, ".depot/workflows/ci.yml") { + t.Errorf("expected migrated .depot/workflows/ci.yml added, got:\n%s", content) + } + if !strings.Contains(content, ".depot/actions") { + t.Errorf("expected .depot/actions added, got:\n%s", content) + } + // The non-migrated file must NOT gain a .depot/ sibling. + if strings.Contains(content, ".depot/workflows/other.yml") { + t.Errorf("expected NO .depot/workflows/other.yml sibling for un-migrated file, got:\n%s", content) + } + // Every original .github/ entry is preserved either way. + for _, orig := range []string{".github/workflows/ci.yml", ".github/workflows/other.yml", ".github/actions"} { + if !strings.Contains(content, orig) { + t.Errorf("expected original %q preserved, got:\n%s", orig, content) + } + } +} + +func TestTransformWorkflow_SparseCheckoutSuperset_PartialMigrationGlob(t *testing.T) { + // A glob sparse-checkout pattern is self-limiting: even under a partial migration it + // must gain its .depot/ sibling so the migrated workflow files that landed under + // .depot/workflows are still checked out. Unlike a literal un-migrated filename, a + // glob asserts no specific file, so it is not gated on the allow-list. + raw := []byte("name: CI\non: push\njobs:\n build:\n runs-on: depot-ubuntu-latest\n steps:\n - uses: actions/checkout@v4\n with:\n sparse-checkout-cone-mode: false\n sparse-checkout: |\n .github/workflows/*.yml\n") + + wf := &migrate.WorkflowFile{ + Path: ".github/workflows/ci.yml", + Name: "CI", + Triggers: []string{"push"}, + Jobs: []migrate.JobInfo{{Name: "build", RunsOn: "depot-ubuntu-latest"}}, + } + report := compat.AnalyzeWorkflow(wf) + + // Only ci.yml was migrated, yet the glob must still be mirrored. + migratedWorkflows := map[string]bool{"ci.yml": true} + + result, err := TransformWorkflow(raw, wf, report, migratedWorkflows) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + content := string(result.Content) + if !strings.Contains(content, ".depot/workflows/*.yml") { + t.Errorf("expected glob .depot/workflows/*.yml sibling added, got:\n%s", content) + } + if !strings.Contains(content, ".github/workflows/*.yml") { + t.Errorf("expected original glob preserved, got:\n%s", content) + } +} diff --git a/pkg/cmd/ci/migrate.go b/pkg/cmd/ci/migrate.go index c3a68d85..e917c97d 100644 --- a/pkg/cmd/ci/migrate.go +++ b/pkg/cmd/ci/migrate.go @@ -551,7 +551,38 @@ func workflows(opts migrateOptions) error { } } - // Rewrite .github/ references in copied action files + depotWorkflowsDir := filepath.Join(depotDir, "workflows") + if err := os.MkdirAll(depotWorkflowsDir, 0755); err != nil { + return fmt.Errorf("failed to create .depot/workflows: %w", err) + } + + // Mirror non-YAML sibling files living alongside workflows (helper scripts, + // configs) into .depot/workflows/ so references to them resolve there. This runs + // before the transform so that, under a partial migration, the copied siblings can + // join the rewrite allow-list below — otherwise a selected workflow's reference to + // a sibling script would keep pointing at .github/ even though the script was moved. + siblings, err := migrate.CopyWorkflowSiblings(workflowsDir, depotWorkflowsDir) + if err != nil { + return fmt.Errorf("failed to copy workflow sibling files: %w", err) + } + + // During a partial migration migratedWorkflows gates which .github/workflows/ + // references get rewritten (only selected workflows). The copied siblings now live + // under .depot/workflows/, so add their relative paths to the set; without this a + // reference like ".github/workflows/scripts/build.sh" inside a migrated workflow + // would be left pointing at .github/ and the copy would go unused. Full migrations + // (nil set) already rewrite every reference, so there is nothing to add there. + if migratedWorkflows != nil { + for _, sibling := range siblings { + rel, err := filepath.Rel(depotWorkflowsDir, sibling) + if err != nil { + return fmt.Errorf("failed to resolve relative path for %s: %w", sibling, err) + } + migratedWorkflows[filepath.ToSlash(rel)] = true + } + } + + // Rewrite .github/ references in copied action files. depotActionsDir := filepath.Join(depotDir, "actions") if info, err := os.Stat(depotActionsDir); err == nil && info.IsDir() { if _, err := transform.RewriteGitHubPathsInDir(depotActionsDir, migratedWorkflows); err != nil { @@ -559,12 +590,6 @@ func workflows(opts migrateOptions) error { } } - // Transform and write each workflow - depotWorkflowsDir := filepath.Join(depotDir, "workflows") - if err := os.MkdirAll(depotWorkflowsDir, 0755); err != nil { - return fmt.Errorf("failed to create .depot/workflows: %w", err) - } - type workflowResult struct { filename string result *transform.TransformResult @@ -605,6 +630,14 @@ func workflows(opts migrateOptions) error { }) } + // Now that migratedWorkflows includes the copied siblings, rewrite any .github/ + // paths inside those sibling files the same way action files are rewritten. + for _, sibling := range siblings { + if _, err := transform.RewriteGitHubPathsInFile(sibling, migratedWorkflows); err != nil { + return fmt.Errorf("failed to rewrite paths in %s: %w", sibling, err) + } + } + // Print summary skipped := len(workflows) - len(selectedWorkflows) if skipped > 0 { diff --git a/pkg/cmd/ci/migrate_test.go b/pkg/cmd/ci/migrate_test.go index 91c00366..aedec492 100644 --- a/pkg/cmd/ci/migrate_test.go +++ b/pkg/cmd/ci/migrate_test.go @@ -326,3 +326,45 @@ jobs: } } } + +func TestRunMigrate_CopiesWorkflowSiblings(t *testing.T) { + dir := t.TempDir() + workflowsDir := filepath.Join(dir, ".github", "workflows") + os.MkdirAll(filepath.Join(workflowsDir, "scripts"), 0755) + + workflow := `name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - run: bash .github/workflows/scripts/build.sh +` + os.WriteFile(filepath.Join(workflowsDir, "ci.yml"), []byte(workflow), 0644) + // Non-YAML sibling referenced by the workflow. + os.WriteFile(filepath.Join(workflowsDir, "scripts", "build.sh"), []byte("#!/bin/sh\necho building\n"), 0755) + + var buf bytes.Buffer + opts := migrateOptions{yes: true, dir: dir, stdout: &buf} + if err := workflows(opts); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // The sibling script should be mirrored into .depot/workflows/scripts/. + sibling := filepath.Join(dir, ".depot", "workflows", "scripts", "build.sh") + if data, err := os.ReadFile(sibling); err != nil { + t.Fatalf("expected sibling copied to %s: %v", sibling, err) + } else if !strings.Contains(string(data), "echo building") { + t.Errorf("sibling content not preserved: %q", data) + } + + // The workflow reference to the script should now point at .depot/. + ci, err := os.ReadFile(filepath.Join(dir, ".depot", "workflows", "ci.yml")) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(ci), ".depot/workflows/scripts/build.sh") { + t.Errorf("expected script reference rewritten to .depot/, got:\n%s", ci) + } +}