test(xtask): make the WSL clock-step guard match lines and compare the twins

[rominf]

Summary

Two gaps in both_wsl_lanes_settle_the_clock_before_running_the_suite, the contract test #451 added to keep both WSL2 lanes settling the guest clock before the suite starts timing scenarios. Both were non-blocking review items on #451, which merged before they were addressed.

It matched a substring, while its doc comment said it matched the step name. The step was located with job_block.find(STEP). The surrounding comments discuss the clock at length, and a comment line that embeds the step's text keeps the check green after the step itself is deleted. I measured this: replacing the step line with # disabled: - name: Settle the clock ... passes the previous version of the test. Both the settle step and the suite step are now located by whole-line equality, so that mutant fails.

It pinned presence and order, not content. The two copies of the step's run: body (one per lane, kept in sync by hand until the lanes are deduplicated in #294) could drift silently. That is the same failure apu_preflight_twins_do_not_drift exists for, and its own comment records the drift being caught too late once. The bodies are now extracted with the existing run_block/dedent helpers and compared byte for byte. They are identical today (65 lines each), and a one-character edit to either fails the test.

Extracting a single step reuses a new step_block helper, which gpu_preflight_steps now calls instead of carrying its own copy of the loop.

The per-PR copy of the step also gains the pointer to its guarding test that only the nightly copy had, so the guard is discoverable from both sides.

Test plan

• Mutation-checked: the commented-out-step mutant passes the old test and fails the new one; a one-character drift in the nightly body fails the new test; the clean tree passes.
• apu_preflight_twins_do_not_drift still passes, confirming the gpu_preflight_steps refactor preserves behaviour.
• cargo fmt --check, cargo test -p xtask (251), cargo clippy -p xtask --all-targets -- -D warnings; both workflow files still parse as YAML (their edits are comment-only).

Note for whoever merges second: #454 inserts code directly above gpu_preflight_steps, a few lines from the body this refactors, so expect a small textual conflict there. No logic overlaps.

CI test plumbing only, so no user-observable behaviour changes and no scenario is needed (AGENTS.md §3).

• Not a bug fix in shipped code; no xfail rows apply.
• No new subcommand or subsystem.
• No user-facing message added or changed.

[volen-silo]

I reviewed this and re-ran the mutation checks from the description, plus a few of my own.

The two gaps this targets are genuinely closed. With the old substring match, a line reading # disabled: - name: Settle the clock ... keeps the test green with the step gone; whole-line equality fails it. Deleting the step outright, moving it after the suite, and turning run: | into run: > all fail loudly with the message they should. A one-character edit inside either copy's body fails the new comparison, and the two bodies are 65 real lines of PowerShell and shell, so the equality is not something that holds trivially. The gpu_preflight_steps extraction is behaviour-preserving, and clippy, fmt and all 251 xtask tests are clean.

Three things I would fix before merge, all in the new comparison, and one note. None of them are hard, and none undermine the approach -- it is the right change.

The body being compared is not quite the run: block. step_block and run_block both let blank lines through unconditionally, so the blank separator line that sits between the settle step and the next step ends up inside the extracted body. Both workflows happen to have exactly one blank line there today, which is the only reason this passes. Delete that blank line in one file and change nothing else, and the test fails claiming the clock-settling step has drifted -- for a formatting edit entirely outside the step. I reproduced this. That is a false failure that would block unrelated work, with a message pointing at the wrong thing.

The failure output is not readable. assert_eq! prints both bodies as single escaped strings of roughly four kilobytes. In the blank-line case they differ by one trailing newline; in the real one-character drift case they differ by one character inside 65 lines. Neither is findable by eye.

step_block's doc comment does not match what it does on blank lines, and that exemption is the mechanism behind the first point, so it is load-bearing rather than cosmetic.

The note, which is not a request to change anything: the guard still passes if both copies are gutted in lockstep. I replaced both run bodies with an identical echo hi and the test was happy. Since the copies are kept in sync by hand, "edit one and copy across" is the normal workflow, so a bad edit applied the normal way is invisible here. The doc comment is honest about what it checks, but one sentence saying it does not check that either body does anything would stop the next reader over-trusting it.

A few concerns I chased that did not hold up, so there are no comments on them: adding an unrelated step between the settle step and the suite step does not trip the ordering assertion; a uniform reindent of one copy is correctly absorbed by dedent while trailing whitespace is still caught; and step_block's unguarded index cannot be reached from either caller.

[volen-silo]

This does not extract only the run: block. step_block skips blank lines when looking for the end of the step (!body.trim().is_empty() && ...), and run_block's take_while admits blank lines unconditionally, so the blank line that separates this step from the next one is pulled into the body along with the step's actual content.

Right now both workflows have exactly one blank line in that position, so the two bodies still match and the test passes. That is the only reason it passes. I deleted just that blank line from .github/workflows/nightly.yml (the one between exit 0 and - name: Ensure native build deps) and ran the test:

assertion `left == right` failed: the clock-settling step has drifted between
e2e-selfhosted.yml and nightly.yml -- the copies are kept in sync by hand

Nothing inside either step changed. A whitespace-only reflow of one workflow now fails a test that says the clock-settling step drifted, which would block unrelated work and send whoever hits it looking in the wrong place.

Trimming trailing blank lines off the extracted body before comparing closes it -- either .map(|b| b.trim_end().to_string()) here, or dropping trailing blanks from the vector run_block returns so every caller benefits.

[rominf]

Fixed in a8aa739b. run_block now drops trailing blank lines from what it returns, so the separator that step_block lets through never reaches a comparison. That also covers the APU preflight twin check, which had the same exposure. I reproduced your case before the fix: deleting the blank line after exit 0 in nightly.yml failed the test. It passes now, and so does adding extra blank lines there.

[volen-silo]

When this fires, assert_eq! prints both bodies as single escaped strings of about four kilobytes each. For the trailing-blank-line case above the difference is one \n at the very end; for the genuine one-character drift the description mentions, it is one character somewhere inside 65 lines. In both cases the two blobs look identical in the terminal.

Since the inputs are already line-oriented, finding the first differing line and naming it would make the failure actionable, e.g. compare first_body.lines().zip(second_body.lines()).position(|(a, b)| a != b) and put the index and both lines in the message, falling back to a length mismatch. Without that, a real drift is detected but still has to be hunted down by hand.

[rominf]

Fixed in a8aa739b. The assert_eq! is replaced by a report of the first differing line. Changing sleep 1 to sleep 2 in nightly.yml now fails with first difference at body line 48: followed by both versions of the line. If every shared line matches, it reports the two line counts instead.

[volen-silo]

"until one is indented no deeper than it" is not what the loop does: the !body.trim().is_empty() guard means a blank line never terminates the block, whatever its indentation. That is the right behaviour for YAML, but it is also exactly why the inter-step blank line leaks into the compared body in the new test, so it is worth stating rather than leaving to be rediscovered -- something like "blank lines never end a block, however they are indented".

[rominf]

Fixed in a8aa739b. The comment now says a block ends at the first non-blank line indented no deeper than the step, that blank lines never end a block however they are indented, and that run_block is where the trailing separator gets dropped.

[volen-silo]

Worth being explicit here about what byte-for-byte equality between the copies does not buy. I replaced both run: bodies with an identical echo hi and the test still passes: presence, ordering and mutual agreement are all satisfied by a step that does nothing.

That is not a defect -- it is the stated scope -- but because the copies are synced by hand, the normal way to edit this step is to change one and paste it over the other, which keeps them in agreement by construction. The one workflow that is routine is also the one this check is blind to. A sentence saying the comparison pins that the lanes agree, not that either body is still doing the work, would keep the next reader from over-trusting it.

[rominf]

Added in a8aa739b: equality pins only that the two lanes agree, not that either body still does the work. Both gutted to an identical echo hi would still pass.

Dismissed as stale: this review is of abbb04c. All four findings are addressed in a8aa739 — trailing blank lines trimmed in run_block, a first-differing-line failure report, and the corrected step_block and test doc comments — with a reply in each thread. Please re-review the current head.

[volen-silo]

Re-reviewed at a8aa739b. Three of the four points from last round are closed — I checked each against the code and reproduced rather than taking the replies at face value. One is closed where I raised it but left standing right next to it.

Point 1, the separator leaking into the compared body, is genuinely fixed. I re-ran the case that exposed it: deleting the blank line between exit 0 and - name: Ensure native build deps in nightly.yml now passes, and so does adding two extra blank lines there. Deleting a blank line inside the body still fails and names it — first difference at body line 7: with both versions printed — so the trim stops at the script and does not reach into it.

I also looked for the obvious way a fix like this overshoots. The only drift the trim can now hide is a trailing whitespace-only line present in one copy and not the other: I appended one after exit 0 in nightly.yml and the test passed. That is inert in both shell and PowerShell, and YAML's | clips trailing empty lines before either copy is ever a string, so the pre-fix behaviour was the wrong one and this is not a loss.

Point 3 is closed. Reading the new step_block comment against the loop: "until a non-blank one is indented no deeper than it" is what !body.trim().is_empty() && indent_of(body) <= step_indent does, and the added paragraph states the blank-line behaviour that caused point 1 and points at run_block for where it is undone. run_block's own new comment matches its code too.

Point 4 is closed by the sentence added to the test's doc comment.

Because a fix for a false positive can quietly become a false negative, I re-ran the mutations that validated the original guard. Commenting out the step name, deleting the step, moving it after the suite step, a one-character sleep 1 → sleep 2 edit, and dropping the last body line all still fail, each with the message that names the right thing. cargo test -p xtask, clippy and cargo fmt --check are clean. I also walked every caller of run_block and step_block — they are all in this one module — and none of them changes behaviour except in the intended way; an emptied body still reaches the has no \run: |` body` panic or the pinned block counts.

The one thing left is point 2, which is fixed for the clock-settling comparison but not for the sibling drift check in the same module, where the failure output is worse. Details inline, plus a small note on the new helper's length-mismatch branch.

[volen-silo]

The line I actually mean is 1227, inside apu_preflight_twins_do_not_drift. GitHub only lets me anchor inside the diff, so this is attached to the new helper instead.

first_line_difference makes the clock-settling failure readable, which is exactly what I asked for. But the sibling drift check a few hundred lines further down still does

assert_eq!(
    a, b,
    "{language} APU preflight script #{i} has drifted between \
     e2e-selfhosted.yml and nightly.yml"
);

on whole dedented scripts, and those scripts are considerably longer than the clock-settling one. I changed ${GPU_PREFLIGHT_MIN_FREE_GIB:-8} to ${GPU_PREFLIGHT_MIN_FREE_GIB:-8 } in nightly.yml; the failure printed two escaped single-line blobs of roughly six kilobytes each, differing by one space near the start. Reading that terminal output, the drift is no more findable than it was before this PR.

That test is already in scope here — the commit message says the run_block trim covers it, and your reply to the separator point said the same. The remedy now exists twenty lines above it. Routing the body comparison in that loop through first_line_difference would finish the job; the two assert_eq!s on block counts just above are fine as they are, since they compare small integers.

[rominf]

Fixed in 8d6d9a42. The per-script comparison in apu_preflight_twins_do_not_drift now goes through first_line_difference, and the block-count assert_eq!s are unchanged. With your ${GPU_PREFLIGHT_MIN_FREE_GIB:-8 } edit in nightly.yml, the failure went from two ~7 KB blobs to first difference at body line 1: followed by that line from each file, labelled e2e-selfhosted.yml: and nightly.yml:.

[volen-silo]

Small one, on the branch rather than the fix as a whole.

This is the only outcome that does not tell the reader which file to open. The first-difference branch prints the two lines in first, second order, so they match the "drifted between e2e-selfhosted.yml and nightly.yml" sentence above and you can work out which is which. "every shared line matches, but one body has 65 lines and the other 64" does not — I hit it by deleting exit 0 from the nightly copy, and the message tells you a line went missing without saying where from.

Taking the two names as parameters, or phrasing it as "{a_name} has N lines, {b_name} has M", would close that.

[rominf]

Fixed in 8d6d9a42. first_line_difference now takes the two workflow names, so deleting exit 0 from the nightly copy reports every shared line matches, but e2e-selfhosted.yml has 65 lines and nightly.yml has 64. The first-difference lines carry their file names too.

Dismissed as stale: this review is of a8aa739. Both remaining points are addressed in 8d6d9a4 — the APU twin check now reports the first differing line, and the length-mismatch message names both files — with a reply in each thread. Please re-review the current head.

[volen-silo]

Both points from the last round are closed. I checked by rerunning the mutations rather than by reading the diff.

The APU twin check now routes its body comparison through the same helper as the clock-settling one. Changing ${GPU_PREFLIGHT_MIN_FREE_GIB:-8} to ${GPU_PREFLIGHT_MIN_FREE_GIB:-8 } in nightly.yml used to print two escaped blobs of about six kilobytes each; it now prints

shell APU preflight script #0 has drifted between e2e-selfhosted.yml and nightly.yml; first difference at body line 1:
  e2e-selfhosted.yml: "MIN_FREE_GIB=\"${GPU_PREFLIGHT_MIN_FREE_GIB:-8}\""
  nightly.yml: "MIN_FREE_GIB=\"${GPU_PREFLIGHT_MIN_FREE_GIB:-8 }\""

The two assertions on block counts just above it are untouched, which is right — they compare small integers and were already readable.

The length-mismatch branch names both files now. Deleting exit 0 from the nightly clock-settling body reports every shared line matches, but e2e-selfhosted.yml has 65 lines and nightly.yml has 64, so you can tell which copy lost the line without opening either.

Since this guard has been edited in three rounds running, I re-ran the whole battery against the current head to make sure nothing had gone quiet. Each mutation is in nightly.yml, applied one at a time against a clean tree:

mutation	result
rename the clock-settling step	fails — "has no clock-settling step"
delete the clock-settling step	fails — same message
move the step after the suite step	fails — "settles the clock AFTER starting the suite"
add one space inside the APU preflight script	fails — first difference at body line 1, both files named
delete exit 0 from the body	fails — line counts, both files named
delete an interior blank line from the body	fails — first difference at body line 16, both files named

On a clean tree the xtask suite passes (251 tests), and clippy and rustfmt are clean on that crate.

Nothing is left open from my side. This looks safe to approve once CI agrees — that reflects a read of the diff plus the local runs above, not the full pipeline or a maintainer's own pass, and it is not itself an approval.