ci migrate: edit workflows in place instead of re-encoding them

[robstolarz]

Closes DEP-5306. Based on main — this was previously stacked on #554, and since that PR was closed (the .github/ → .depot/ rewrite stays) the branch has been rebased --onto main to drop that commit. The rewrite's behavior is unchanged by this PR.

depot ci migrate parsed each workflow with yaml.v3 and re-encoded the node tree. That reformats the whole file, so a user reviewing the migration diff sees the encoder's opinions mixed in with the actual changes:

• Every blank line disappears. A customer reported this directly: their migrated workflows came back as one unbroken wall of YAML, and they want the blank lines preserved natively rather than added back by hand.
• run: | blocks collapse. yaml.v3 will not emit a literal block whose lines end in whitespace — one stray trailing space anywhere in a script and the whole block comes back as a single \n-escaped double-quoted string. Shell that was readable is now not.
• Quoting style, key order, and indentation width all get normalized to the encoder's preferences.

Approach

Parse to locate, not to rewrite. The node tree is now used only for the line and column of the things migration actually changes; each change is spliced into the original bytes as a text edit. What isn't edited isn't touched — blank lines, block scalars, quoting, key order, and indentation width all survive byte for byte, and a workflow that needs no changes comes back byte-identical under its header.

Two things make that safe rather than merely plausible:

• Every extracted scalar token is re-parsed and compared against the value the node reported. A mis-delimited extent fails closed instead of writing a mangled file. The spliced result is re-parsed too.
• When any single change can't be expressed as an edit, the whole file falls back to re-encoding. An anchored label, a flow mapping of triggers, a scalar whose source can't be delimited — the file is reformatted, but the change is never skipped or half-applied. Correctness first; fidelity where the shape of the file allows it.

Also fixes a latent bug this surfaced: the pass that comments out uncorrectable jobs searched for a job key at exactly two spaces of indent. That only ever held because the encoder had already normalized the file it was reading — on the original text, a workflow indented with four spaces would have had its disabled job silently left live. It now uses each job's own indentation.

The one place the path rewrite had to move

The diff deletes transformGitHubPaths, and that is not the removal #554 proposed — the rewriting still happens, with the same guards and the same ChangePathRewritten record. It changed where it runs.

It was a walk over the node tree, mutating scalar values. Under in-place editing that would have forced the re-encoding fallback for any workflow carrying a local .github/ reference — nearly all of them — so the fidelity fix above would have applied to almost nothing. It is the same pure text substitution RewriteGitHubPathsInDir already applies to copied action files, so it now runs over the source ahead of the parse. The parsed text is the rewritten text, so no edit offset needs adjusting, and a workflow whose paths are rewritten keeps its formatting.

transform_test.go's existing path-rewrite assertions are unmodified and pass, which is the check that behavior didn't drift.

A note could be broken out of (found in review)

Cursor's security reviewer flagged the generated # was: <label> note as unsanitized, and it was right. Any label that isn't a Depot label, a standard GitHub runner, or an expression is remapped as "nonstandard" — so the quoted text is arbitrary workflow content, not one of a known set. A comment ends at its line's end, so runs-on: "self-hosted\ninjected: pwned" ended the note and left the rest of it standing as a live YAML line in the migrated workflow.

The re-parse guard does not catch this: the result is still valid YAML, just no longer the author's workflow. Notes are now flattened before splicing — content kept, line breaks become spaces. Both a unit test on the splice and an end-to-end test asserting the migrated document has exactly its original three keys; the unit test fails without the fix.

Tests

The fidelity tests are written as byte-exact comparisons against the input rather than Contains checks, which is the only way to catch an unintended reformat.

• Blank lines, a run: | block with a trailing space, four-space indentation, quoting, and comments all survive a runs-on remap.
• A workflow needing no changes comes back byte-identical.
• Trigger removal from a block mapping, a block sequence, a flow sequence (kept items keep their quoting), and the all-unsupported case that collapses to on: {} — none of them disturb surrounding blank lines or leave a double gap.
• A disabled job at four-space indent is commented out at its own indent, and its live siblings are still remapped.
• A quoted label stays quoted in the style the author used; a collapsed on: keeps a trailing comment on its own line; an author's existing line comment is never overwritten (the note goes above instead).
• Columns are byte-correct after a multibyte character earlier on the same line — yaml.v3 counts columns in runes.
• CRLF input isn't corrupted.
• Rewrite/fidelity interaction: a workflow with a local action reference and blank lines and a trailing-space run: | block comes back with only its two local paths rewritten, byte-exact otherwise, with a remote org/repo/.github/actions/... reference untouched. Run against main this fails on all three counts (blank lines dropped, block collapsed to "…build.sh \n\nmake test\n", indentation normalized) — it is the fail-first proof for the combined behavior.
• Positive/negative control pair: the fallback test runs the same fixture behind an anchored label and asserts the two deficiencies re-encoding does have. If the in-place path silently stopped being taken, the fidelity tests would fail; if the fallback stopped being reachable, this one would.

Shapes that route to the re-encode fallback

Anchored or aliased scalars; block or folded scalars where one is being replaced; multi-line plain scalars; a flow mapping of triggers containing an unsupported one; an entry that doesn't begin its own line; any scalar whose extracted token doesn't re-parse to the node's value; overlapping planned edits; a spliced result that doesn't parse.

All of these produce today's output, so this change is never-worse-than-before rather than best-effort.

Why the other two approaches were rejected

Sentinel comments (insert a marker comment per blank line, strip after encoding): yaml.v3 reflows comments to its own idea of placement, and a sentinel can't represent a blank line inside a block scalar — which is where they matter most in a run: script.

Trimming trailing whitespace from run: blocks so the encoder keeps the block style: this fixes the symptom by editing the user's shell text, and does nothing for blank lines outside block scalars.

Deferred / known-open

Shipping with these documented rather than fixed, so the reported problem lands now.

• DEP-5306 items 1–3 are not addressed here. Uncopied sibling files, paths embedded in regexes, and sparse-checkout paths rewritten while scripts weren't are all ways the .github/ → .depot/ rewrite goes wrong. They would have been resolved by construction had ci migrate: stop rewriting .github/ references to .depot/ #554 landed; with the rewrite retained they remain open, and they are ci migrate: resolve referenced paths and preserve YAML readability #533's territory. This PR covers item 4 (the run: block collapse) and the blank-line report.
• P2 — escape sequences in double-quoted scalars. The rewrite now judges the character before .github/ on the raw source text. In "\t.github/actions/x" that character is the literal t, not a tab, so the reference is left alone where the node-tree pass would have rewritten it. It fails closed (path untouched, never mangled), and needs a contrived fixture to hit.
• Workflows that route to the fallback still get today's formatting. The shapes are listed above; they are unchanged, not newly broken.
• Unrelated pre-existing flake: TestRunShellCommandCancellationAllowsTermCleanup in pkg/cmd/tests fails intermittently (1/5 on this branch, 1/6 on a clean origin/main worktree). This stack touches no file in that package.

History

The review-fix and narration-cleanup commits are folded into this feature commit. The final tree is unchanged; no behavior or rollout change is included. The consolidated history was verified with per-commit Go builds, go vet ./..., and go test ./....

---

Note

Medium Risk
Changes how migrated workflows are produced (correctness and diff fidelity); fallback limits blast radius, but wrong in-place edits could still break customer CI YAML.

Overview
depot ci migrate no longer re-encodes whole workflow files through yaml.v3 for the common case. It parses YAML only to find line/column positions, then splices text edits into the original bytes so blank lines, run: | blocks, quoting, and indentation stay byte-identical except where migration actually changes something.

TransformWorkflow now runs the existing .github/ → .depot/ path rewrite on raw text before parse (same rules as before), then tries transformInPlace for trigger removal, runs-on remapping with notes, and commenting out uncorrectable jobs at each job’s real indent. If any edit is unsafe (anchors, bad scalar extents, flow trigger maps, etc.) or the result fails to parse, it falls back to the previous re-encode path.

Adds textedit helpers (line indexing, scalar extent with re-parse checks, applyEdits) and commentSafe so generated # was: notes cannot inject newlines into YAML. Large byte-exact fidelity tests cover triggers, CRLF, path rewrite + formatting, and the re-encode fallback.

^{Reviewed by Cursor Bugbot for commit 4dbd2a6. Bugbot is set up for automated code reviews on this repo. Configure here.}

[linear-code]

DEP-5306

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit ce034bd. Configure here.}

[cursor]

Stale comment

Risk: medium. Left a non-blocking comment; did not approve, and reviewers were not assigned. Cursor Bugbot was present but completed as skipped and reported two unresolved findings, so human review is needed.

  

_{Sent by Cursor Approval Agent: Pull Request Router and Approver}

[cursor]

Stale comment

[cursor]

Stale comment

Risk: medium. Left a non-blocking comment; did not approve, and reviewers were not assigned. Cursor Bugbot was present but did not reach a terminal state within the wait window, so human review is needed.

  

_{Sent by Cursor Approval Agent: Pull Request Router and Approver}

[cursor]

Risk: medium. Approved; reviewers were not assigned. Cursor Bugbot and Cursor Security Agent both completed successfully with no unresolved findings that need human review.

  

_{Sent by Cursor Approval Agent: Pull Request Router and Approver}