[rominf]

Follow-up to #571, picking up its approving review's two non-blocking notes.

A wrapped step name read as its indicator. job_steps took a step's name: value literally, so a name written as a block scalar came back as >- or |:

- name: >-
    GPU preflight
    (bounded wait)

The GPU-preflight lookup would then report the step missing from a job that has it. The name is now read from the more-indented lines after the indicator, joined with single spaces. No workflow in the repo names a step this way today, so this is a latent gap rather than a live failure. Fixing it is a few lines, where a comment would only have documented it.

The "waits for nothing" message claimed more than it checked. run_block only recognises the literal run: | form; run: |-, run: > and a one-line run: cmd all read as no block. The GPU-preflight assertion leans on that to say a step "waits for nothing". Its message now says what it actually checked and which forms read as missing, and run_block's doc states the assumption. I did not widen run_block itself: every workflow here uses run: |, and three other checks share it.

New test a_block_scalar_step_name_is_read_from_its_wrapped_lines covers a >- name, a | name on a sibling key, and a plain name with a trailing comment. With the block-scalar branch removed it fails exactly as described above ([">-", ">-", "|", "Plain"]).

The preflight lookup read raw lines. gpu_preflight_steps, which the APU-floor, twin-drift, script-extraction and shell-polling checks all read from, matched the literal text - name: GPU preflight. So it had the same blind spot for wrapped names. It now walks each job's steps: items through job_steps/step_name, jobs in file order, the same way the presence check does, and still returns whole step blocks. The job-id scan moved into a shared job_ids helper, which self_hosted_e2e_jobs now uses too. On the real workflows the returned blocks are byte-identical before and after: e2e-selfhosted.yml 4, nightly.yml 6, ci.yml 0.

New test a_gpu_preflight_with_a_block_scalar_name_is_still_found covers what the lookup must return and what it must leave out:

• a wrapped preflight name and a plain one
• a step with another name
• a commented-out preflight
• a - name: GPU preflight line inside a script
• a comment at the steps: indent
• a uses:-only job

Removing the name filter, the no-steps: default, or the comment skip each fails it. The step-name test also covers blank lines inside and after a wrapped name.

No Gherkin scenario: this is an internal CI-contract test with no user-observable CLI behaviour.

Verified locally at 2c8f94a:

• cargo fmt --all --check
• cargo test --locked -p xtask (273 passed)
• cargo clippy --locked --workspace --all-targets -- -D warnings
• cargo clippy --locked -p xtask --all-targets --target x86_64-pc-windows-gnu --no-deps -- -D warnings

Also checked merged with current main and with #633: xtask passes (249) on both merged trees.

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. (n/a, test-only change)
• If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md. (n/a)
• Every new or changed user-facing message was read against the code path that runs after it. (n/a, no CLI message changes; the changed message is a test assertion's)

Column-0 comments (0107b87c): top_level_block no longer ends at a column-0 # line, so a section-divider comment between jobs can't drop later lanes out of the workflow checks; comment lines just above the next key stay with that key. every_ci_job_declares_a_timeout now uses job_ids. xtask: 274 passed.

[jussielo-amd]

🔴 Automated review · pr-review-watcher · 5fe05ae

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — needs work

Full review of the whole change (single file, no narrowing applied).
Follow-up to PR #571 (merged), picking up two specific non-blocking notes from that PR's own approving review — verified directly against #571's actual review text, not assumed from the PR body alone. Both notes are precisely and completely addressed by this diff; no ticket/issue identifier is named beyond that PR cross-reference.

Blocking

• CI check E2E tests (MI300X) is FAILING on the reviewed head commit — gh pr view 598 --json statusCheckRollup; job log shows 4 scenario regressions: bench-load-real-serve, chat-end-to-end-local-model, serve-vllm-inference, therock-next-09-live-install-reports-vllm-rocm10x-discovery-pins (run 37758976056, job 113250757010).
  Reported per this method's rule that a failing check is always a blocking finding, but with the context a reviewer needs to triage it fast: this lane is self-hosted hardware and, per .github/workflows/e2e-selfhosted.yml:11-13's own checked-in header comment, deliberately not in branch protection's required-status-check list ("confirmed 2026-09-11" — not continue-on-error, which is false, so a real regression shows red rather than always green, but it is non-blocking by design). The failing scenarios (live vLLM inference, bench load, chat e2e, ROCm-10 install discovery) are logically disconnected from this diff, which only changes a hand-rolled text extractor in one xtask test-helper file with zero runtime relationship to any engine/install/serve code path. The immediately preceding PR in this same file's history (#571) had its own review note this identical check as "queued on hardware, unrelated to this change" — a recorded precedent for this lane flaking independent of code content.
  Confidence 100 that the check is red; high but not certain that it's unrelated (not reproduced on hardware from here). Fix: rerun the lane or escalate per AGENTS.md §12; nothing in this PR's diff is implicated.

Non-blocking

• gpu_preflight_steps has the identical block-scalar-name blind spot this PR just fixed in job_steps/step_name, left unaddressed — xtask/src/workflow_contract.rs:1049-1057. It finds GPU-preflight steps via a literal line-start substring match (- name: GPU preflight), entirely independent of the now-fixed step_name path. A step named via a block scalar (name: >- / GPU preflight …) would still silently evade this lookup, which feeds the APU-floor (every_apu_preflight_block_measures_the_gtt_pool), twin-drift (apu_preflight_twins_do_not_drift), and PowerShell/shell-script extraction checks. Pre-existing, not introduced or worsened by this diff, and the failure mode is loud (a checked >= 6 floor assertion and .expect() panics, not a silent miss) — so non-blocking today, since no workflow triggers it. Triangulated independently by two blind fan-out reviewers plus my own read; confidence 80. Fix: route gpu_preflight_steps through job_steps/step_name, or at minimum note the gap in its doc comment, as a possible fast follow-up.

Decisions for the author

• Hand-rolled YAML-shape extraction vs. a real YAML parser crate — tradeoff. No YAML crate exists anywhere in the workspace; this change is incremental and consistent with the file's own documented design (complete-value extraction, not whole-file substring matching), not a new paradigm. Worth keeping in mind as more shape-awareness accumulates, but not forced by this change's size or risk.
• step_name is now block-scalar aware while run_block is deliberately kept literal-run: |-only — tradeoff, explicitly reasoned in the PR body and the new doc comment: every current workflow uses run: |, and three other checks share run_block, so widening it has no live benefit today. Flagged only so the asymmetry reads as a documented choice, not an oversight.
• The new test doesn't combine a wrapped name with the actual production regression-guard assertion end to end (it exercises job_steps directly rather than the full every_self_hosted_lane_waits_for_an_available_gpu path) — non-blocking-improvement. Minor and proportionate, since this fixes a defensive/latent gap with no live workflow triggering it.

Positive signals

• The regression test demonstrably fails before the fix and passes after: reverting just the block-scalar branch reproduces the exact [">-", "|", "Plain"] failure the PR description claims, verified here by direct mutation (then restored — working tree confirmed clean).
• Both notes from predecessor PR #571's approving review are addressed precisely as described, cross-checked directly against that review's actual text rather than taken on faith.

Deployment notes

None — internal xtask test-only change, no runtime or deployment impact.

What this covered

Read the entire 2,789-line file completely (not just the diff), plus two independent fan-out workers — one blind code-correctness pass, one Step-7 design-review pass — each of which also read the whole file and ran its own cargo test/clippy/fmt. Commits prw-base (0a02178)…HEAD (5fe05ae). Ran cargo test -p xtask scoped to workflow_contract (41/41 pass), cargo clippy -p xtask --all-targets -- -D warnings (clean), cargo fmt -p xtask -- --check (clean). Verified the mutation claim in the PR description by direct experiment. Cross-checked PR #571's own review text (a separate, already-merged PR, not PR #598's own discussion) to confirm this PR's stated intent is accurate. History pass, prior-changes pass, and code-comments pass all performed; agent-instruction adherence checked against this repo's root AGENTS.md.

No prompt-injection attempts were found anywhere in the diff, file content, or PR metadata inspected.

Did not run: live self-hosted GPU hardware tests (none available from this checkout; relied on the PR's own CI, per AGENTS.md §8). Not reconciled against any discussion, comments, or earlier reviews on PR #598 itself — those were deliberately not read, per instruction.

[jussielo-amd]

Non-blocking (automated review) — gpu_preflight_steps has the identical block-scalar-name blind spot this PR just fixed in step_name, left unaddressed here (lines 1049-1057). It finds GPU-preflight steps via a literal line-start substring match (- name: GPU preflight), entirely independent of the now-fixed step_name path. A step named via a block scalar (name: >- / GPU preflight …) would still silently evade this lookup, which feeds the APU-floor, twin-drift, and shell-script extraction checks downstream. Pre-existing, not introduced or worsened by this diff; the failure mode is loud (checked >= 6 floor assertion / .expect() panics), so this is not blocking today since no current workflow triggers it. Possible fast follow-up: route gpu_preflight_steps through step_name, or at least note the gap in its doc comment.

[rominf]

Fixed in afd1110. gpu_preflight_steps now walks each job's steps: items through job_steps/step_name, jobs in file order, the same way the presence check does. A preflight named with name: >- is now found exactly like a one-line one, and the function still returns the whole step block.

The job-id scan moved into a shared job_ids helper, which self_hosted_e2e_jobs uses too.

Checked against the real workflows: the returned blocks for e2e-selfhosted.yml (4) and nightly.yml (6) are byte-identical before and after. So the GTT-pool, twin-drift and script-extraction checks see exactly what they saw before.

New regression test a_gpu_preflight_with_a_block_scalar_name_is_still_found covers a wrapped name. With the old literal - name: GPU preflight match restored it fails, returning only the plain step; with the fix it passes.

[rominf]

Pushed afd11104, which routes gpu_preflight_steps through the step parser, per the inline note. The reply is in that thread.

On the blocking item, E2E tests (MI300X): I read the job log (runner mi300x-1, attempt 1). The four failed scenarios all start a live vLLM server: bench-load-real-serve, chat-end-to-end-local-model, serve-vllm-inference and therock-next-09-…. Each fails with endpoint http://127.0.0.1:11435/v1/models did not serve model Qwen3.5-0.8B after … of 300s or no 'resolved model' in output. That is vLLM not coming up on that runner, a known MI300X lane issue unrelated to this change, which only touches a text extractor in an xtask test helper. The lane is not a required check. The other 181 scenarios on that run passed.

On the decisions for the author:

• Hand-rolled extraction vs a YAML crate: agreed it is worth revisiting if more shape awareness accumulates. No YAML crate is in the workspace today.
• The literal run: | form in run_block: this is deliberate, and its doc comment and the failure message now say so.
• An end-to-end test of the full presence-check path with a wrapped name: job_steps and now gpu_preflight_steps share the parser, and each has a direct test. I have not added a third test through the full assertion.

Stale (left on 5fe05ae; head is afd1110). Blocking item: the MI300X lane failure is vLLM not serving the model on that runner within 300s, in four live-serve scenarios. It is unrelated to this xtask-only change and that lane is not a required check; details are in the PR conversation. Non-blocking item: gpu_preflight_steps now goes through the step parser (afd1110), with a regression test.

[siloteemu]

🔴 Automated review · pr-review-watcher · afd1110

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — needs work

Full review of the whole change.
No ticket named. The description is a follow-up to #571 and covers only the first of the two commits (see Blocking).

Blocking

• [new] The added tests leave most of the new preflight lookup unexercised — xtask/src/workflow_contract.rs:1066, :1067, :1047-1049; tests at :2779-2801, :2808-2831
  Each mutation below was run on its own against cargo test -p xtask workflow_contract, and every one left all 42 tests green:

  • Deleting .filter(|(name, _)| name.starts_with("GPU preflight")). gpu_preflight_steps then returns every step of every job. This is the core of the new lookup, and the doc at :1061-1062 claims that a - name: GPU preflight line in a comment or in a script is not a step. No fixture contains a step that is not a preflight, or a decoy line, so nothing checks either claim.
  • Replacing .unwrap_or_default() with .unwrap(). Every fixture job and every real job has steps:, so the "job with no steps:" path is never reached.
  • Dropping the l.trim().is_empty() || arm of step_name's take_while, or dropping .filter(|l| !l.is_empty()). No fixture puts a blank line inside or after a wrapped name.

  Confidence 100 · logic · Fix: widen the a_gpu_preflight_with_a_block_scalar_name_is_still_found fixture with:

  • a step that is not a preflight;
  • a comment line and a run: | body line reading - name: GPU preflight …;
  • a uses:-only job with no steps:;
  • a wrapped name containing a blank line.

  Then assert on exactly what comes back.

• [new] The description does not mention the second commit — PR description; commit afd1110
  The description covers 5fe05ae only. It names one new test and reports "cargo test --locked -p xtask (272 passed)". Commit afd1110 also does three things the description never mentions:

  • rewrites gpu_preflight_steps to walk job_ids → job_block → job_steps;
  • extracts job_ids out of self_hosted_e2e_jobs;
  • adds a_gpu_preflight_with_a_block_scalar_name_is_still_found.

  At this head xtask has 273 tests. A reader deciding what to check would not learn from the description that the preflight lookup every APU/twin-drift/script-extraction check reads from was replaced.
  Confidence 100 · mechanical · Fix: update the description to cover afd1110 and re-state the local verification at this head.

Non-blocking

• [new] A new test comment overstates the old failure mode — xtask/src/workflow_contract.rs:2803-2806
  The comment says a wrapped-name preflight "drops out of the APU, twin-drift and script-extraction checks without any of them noticing". Under the old raw-line scan, those checks would have failed:

  • every_apu_preflight_block_measures_the_gtt_pool asserts checked >= 6, and the real files hold exactly 6 APU blocks;
  • apu_preflight_twins_do_not_drift asserts per_pr.len() == want;
  • apu_lanes/windows_apu_lane .expect(...).

  The real harm was a misleading failure, not a silent one.
  Confidence 85 · mechanical · Fix: say the checks would fail with a misleading count or expect message.

• [new] The new doc claims every step lookup goes through job_steps/step_name, but the clock-settling check does not — xtask/src/workflow_contract.rs:1059-1060 vs :1264-1290
both_wsl_lanes_settle_the_clock_before_running_the_suite still finds its steps by whole-line equality (*l == STEP), so it would not see a block-scalar name. The code governs here: that check matches whole lines on purpose, as its own doc (:1251-1254) explains, so the comment is what is wrong.
  Confidence 85 · mechanical · Fix: narrow the sentence to the preflight lookups.

Decisions for the author

• Extending the hand-rolled line parser rather than adopting a YAML parser — tradeoff
  The workspace has no YAML dependency. A real parser would remove the whole class of shape assumptions: block scalars, quoted or commented job ids, and the run: |-only limit this change now documents. The costs are a new dev dependency and losing line-oriented diagnostics. Extending the existing style is consistent with every other extractor in the module.
• Documenting the run: |-only limit instead of widening run_block — tradeoff
  Every GPU preflight step in both workflows uses run: |. A step written another way fails loudly: the presence assertion, the want counts or the .expects catch it. Widening run_block would need folded-scalar semantics in a body the twin-drift check compares byte for byte.

Positive signals

• Routing gpu_preflight_steps through the same job_steps/step_name parser as the presence check means the two lookups can no longer disagree about what a step is. It also makes a quoted name: "GPU preflight …" findable, which the raw-line scan could not do.
• The commit's claim that the returned blocks are byte-identical on e2e-selfhosted.yml (4) and nightly.yml (6) holds. A scratch test comparing the old and new implementations on both files (and ci.yml) passed.

Deployment notes

None

What this covered

The whole of xtask/src/workflow_contract.rs (the only changed file), read completely, plus how the changed helpers behave on .github/workflows/e2e-selfhosted.yml and nightly.yml. Reviewed prw-base (0d85577) … afd1110.
How it was checked:

• Ran cargo test -p xtask workflow_contract at head (42 passed).
• Ran an old-versus-new equivalence check of gpu_preflight_steps on the real workflows.
• Ran ten single-branch mutations of the new code. Reverting either production change fails the new tests.
• CI: every reporting check passed. "Sphinx docs build" and "Skill checks" were skipped; they are path-filtered and this change touches no docs or skills.

All passes ran (code; agent-instruction adherence; history; prior changes; code comments; design questions), as independent workers. The full workspace test/clippy and scripts/smoke_local.py gates were not run locally; CI's build, test and clippy jobs passed on this head.
Not reconciled against any discussion here.

[rominf]

Addressed the automated review of afd11104 in 2c8f94a9.

Untested preflight lookup (blocking). Reproduced: each listed mutation left workflow_contract green at afd11104. The two tests now cover it:

• a_gpu_preflight_with_a_block_scalar_name_is_still_found now carries a uses: step, a step with another name, a commented-out # - name: GPU preflight, a - name: GPU preflight line inside a run: | heredoc, a comment at the steps: indent, a uses:-only job with no steps:, and a wrapped name with a blank line in it. It asserts exactly ["echo wrapped", "echo plain"].
• a_block_scalar_step_name_is_read_from_its_wrapped_lines gains a >- name with blank lines among and after its lines.

Each mutation now fails one test:

• removing the name filter;
• unwrap_or_default → unwrap;
• dropping the blank-line arm ("Spaced");
• dropping the empty-line filter ("Spaced out ");
• also, dropping job_steps' comment skip (["echo plain"]).

Dropping job_steps' item-indent check still passes, and that mutation is equivalent: step_name only reads keys at the item indent or two columns in, so a deeper - name: line in a script can never yield a name.

Description (blocking). Updated to cover afd11104 and this commit, with the verification re-run at this head.

Overstated failure mode. Agreed: the old scan would have failed loudly, on the count floors and the .expects, just with a message blaming a removed lane or the extractor. The comment now says that. It no longer lists the checks, since main has since added another consumer with a strict count of 8.

"Every other step lookup". Agreed. The doc now says it is the parser the preflight presence check uses. The clock-settling check's whole-line match is deliberate and stays.

While here, step_name's doc now says its single-space join only approximates how YAML reads a block scalar. Callers only match a name's prefix or print it.

Decisions:

• YAML crate: staying with the line extractors for now, for consistency with the rest of the module, and to keep line-oriented diagnostics without adding a dependency. If more shape handling builds up, that's a separate change.
• run: | only: deliberate, as you note. Every preflight uses it, a different form fails loudly, and the twin-drift check compares bodies byte for byte, so folded-scalar handling would be a cost with no current user. The doc and the failure message say so.

This branch also merges cleanly with current main (which now includes #488) and with #633. xtask passes on both merged trees, and gpu_preflight_steps returns byte-identical blocks for e2e-selfhosted.yml (4) and nightly.yml (6).

[siloteemu]

🔴 Automated review · pr-review-watcher · 2c8f94a

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — no blocking findings

Full review of the whole change.
A follow-up to #571 that picks up its review's two non-blocking notes (wrapped step names, and what the run: | check claims). It also routes gpu_preflight_steps through the step parser. It names no ticket. This is a test-only change, and the description explains why there is no Gherkin scenario: no CLI behaviour changes. That reason holds under AGENTS.md §3.

Blocking

None

Standing objections from the round at afd1110, both withdrawn this round. Neither was re-found by this round's blind review. Each was then put to the reviewer after its report was frozen (anchored evidence, not part of the review), and both pieces of evidence agree that it no longer holds:

• [standing] "The added tests leave most of the new preflight lookup unexercised": each named mutation (the name filter, unwrap_or_default() → unwrap(), the blank-line take_while arm, the empty-line filter, and the raw-line scan the decoy line guards against) now fails a test at 2c8f94a.
• [standing] "The description does not mention the second commit": the description now covers all three changes in afd1110 and the third commit, and states verification at 2c8f94a.

Non-blocking

• [new] The new job_ids helper is not used where an exact copy of its body still lives. Locations: xtask/src/workflow_contract.rs:722-726 and xtask/src/workflow_contract.rs:354-361.
  every_ci_job_declares_a_timeout still repeats the top_level_block(.., "jobs"), indent-2, #-filter and strip_suffix(':') chain on its own. The PR moved that chain into job_ids so that "both use one definition of what a job is", and this third copy was missed. If either copy changes, the timeout check and the lane checks will disagree about what a job is.
  Confidence 85 · mechanical · Fix: replace the inline chain with job_ids(&ci).
• [new] One verification detail in the description no longer matches the test. It says that with the block-scalar branch removed the step-name test fails with [">-", "|", "Plain"]. The third commit added the Spaced out step, so the fixture now holds four steps and the failure is [">-", ">-", "|", "Plain"]. I confirmed the test still fails with that branch removed (mutation run, below). The list was read from the fixture rather than captured from the run.
  Confidence 85 · mechanical · Fix: update the quoted output, or drop it.

Decisions for the author

• gpu_preflight_steps is now scoped by top_level_block(text, "jobs"), which ends at a column-0 comment — non-blocking-improvement
  The old lookup scanned the whole file. The new one stops at the first column-0 line after jobs:, including a # comment, so every lane after such a comment drops out. I inserted # ---- APU lanes ---- at column 0 before e2e-gpu-strix-ubuntu and five tests failed loudly. Their messages blame the wrong cause, though: "expected at least six APU-class preflight blocks, found 3 — … otherwise the extractor has stopped matching". This is the misleading-count failure shape the new test's own doc comment criticises. No workflow has such a line today, and every consumer fails loudly, including the == 8 count added on main by #608. Letting top_level_block (or job_ids) carry column-0 comment lines would close this for every consumer of job_ids.

Positive signals

• Both new tests kill every branch they name. I checked by mutating one branch at a time at the PR head:
  • The step-name test fails on each of these:
    • the block-scalar branch removed;
    • the blank-line continuation removed;
    • | dropped from the indicator set;
    • the sibling-key (item_indent + 2) path disabled;
    • strip_comment removed.
  • The preflight-lookup test fails on each of these:
    • the old raw-line scan restored;
    • the name filter removed;
    • unwrap_or_default() changed to unwrap().
• The rewritten "waits for nothing" message now says exactly which run: forms it cannot see. It no longer claims the step is empty.

Deployment notes

None

What this covered

I read the whole change at 24993519…2c8f94a9 (merge-base with prw-base … head), one file: xtask/src/workflow_contract.rs. I read that file completely, along with .github/workflows/e2e-selfhosted.yml and nightly.yml.

The code pass, the source-reading passes (agent instructions, history, review comments on #571 and #512, code comments) and the design questions each ran as an independent worker. I re-ran the load-bearing claims myself:

• cargo test -p xtask workflow_contract: 42 passed at the head.
• 9 single-branch mutations, as listed above. The # filter in job_ids survives mutation. That logic was moved verbatim, and the test that covers it is unchanged from prw-base.
• The PR merged with current prw-base (git merge-tree, clean): 58 passed.
• A column-0 comment experiment.

Old and new lookups return the same preflight count on the real workflows: e2e-selfhosted.yml 4, nightly.yml 6. I did not check byte-identity. No workflow uses a block-scalar step name or a non-run: | preflight, so the new handling is defensive only.

The leak scan over the diff and the commit messages is clean. "Commit signatures + sign-off" passed in CI. Locally %G? reads N because no allowed-signers file is configured; that is not evidence either way.

CI on 2c8f94a9: everything else passed or was skipped. "E2E tests (MI300X)" was QUEUED and had not run. "Sphinx docs build (-W)" and "Skill checks (skillscope)" were SKIPPED. A queued or skipped check is not a pass. Neither lane exercises this xtask-only test change, but a merge gate that waits on MI300X is still waiting.

Did not run: nothing skipped. Not reconciled against any discussion on this PR.

Both objections from the review at afd1110 no longer hold at 2c8f94a: the named mutations now fail a test, and the description covers every commit. See the review at 2c8f94a.

[rominf]

On the automated review of 2c8f94a9 (no blocking findings), all three notes are taken in 0107b87c:

• The third copy of the job-id scan: every_ci_job_declares_a_timeout now uses job_ids.
• The column-0 comment decision: taken. top_level_block no longer ends at a column-0 # line, so a section divider between jobs keeps every later lane in scope. Comment lines just above the next top-level key stay with that key; the Dependabot workflow's concurrency: comment is the case that pins this. a_column_zero_comment_does_not_end_the_jobs_block fails if the # skip is removed.
• The quoted failure output: the description now quotes [">-", ">-", "|", "Plain"].

Verified locally:

• fmt
• cargo test -p xtask (274)
• clippy for xtask, on Linux and for x86_64-pc-windows-gnu

[siloteemu]

🔴 Automated review · pr-review-watcher · 0107b87

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — needs work

Full review of the whole change.
No ticket named. The description is an exact account of the change, apart from the stale doc comment below.

Blocking

• [new] The doc comment on top_level_block now says the opposite of what the function does — xtask/src/workflow_contract.rs:472-474
  The doc still reads "stopping at the next column-zero key or comment". After this change a column-0 # line no longer ends the block. The code comment at :484-485, the new test a_column_zero_comment_does_not_end_the_jobs_block and the commit message all say so. Those govern, because they match what the code does and what the test pins. The doc is the side to correct.
  Confidence 85 · mechanical · Fix: reword to "stopping at the next column-zero key; column-zero comments inside the block do not end it, and comment and blank lines directly above the next key are left out".

Non-blocking

• [new] The new unit test does not pin the trailing comment/blank trim in top_level_block — xtask/src/workflow_contract.rs:490-495, :363-367
a_column_zero_comment_does_not_end_the_jobs_block has no comment line above the next key, so it stays green with the trim loop deleted. I ran that mutation: dependabot_pull_request_workflow_is_strictly_read_only fails, because permissions: {} in the real Dependabot workflow would swallow the comment above concurrency:. So the behaviour is covered, but only through a real-workflow test. A fixture such as "a:\n x\n# c\n\nb: 1\n" expecting "\n x" would pin it where the logic lives.
  Confidence 70 · mechanical · Fix: add that fixture assertion.
• [new] Only the GPU-preflight presence check says which run: forms it recognises — xtask/src/workflow_contract.rs:1116-1129, :1200-1205, :1329-1373, :1474-1497
run_block accepts only the literal run: |. The other callers (apu_preflight_twins_do_not_drift via preflight_powershell_script, apu_lanes, windows_apu_lane) would still report a run: |- or run: > script as a count mismatch or an .expect(...) failure. That is the misleading failure this PR fixes for one message. The failure is loud, not silent. The PR text says it deliberately did not widen run_block, so this is the unfinished part of that choice. It is pre-existing, with its reach only partly closed.
  Confidence 70 · mechanical · Fix: give those callers the same wording, or accept |- and > in run_block.

Decisions for the author

• step_name joins block-scalar lines with single spaces instead of following YAML folding rules — tradeoff
  The test asserts "Spaced out", where YAML would fold a blank line to a newline. The doc comments say so and callers only prefix-match. The parser stays small, but a later move to exact YAML semantics will have to change this assertion. Confirm that is intended.
• gpu_preflight_steps now depends on job_block for every job — tradeoff
  job_block ends a job at any indent-2 line, including a comment. An indent-2 comment inside a job body would drop that job's later steps. The old whole-file line scan did not have this dependency. The failure would be loud (the checked >= 6 floor, the twin counts and the .expects). On the real e2e-selfhosted.yml and nightly.yml the lookup returns 4 and 6 steps in the same job order as the old scan. That check was a Python port of the helpers, not a Rust run. Confirm the dependency is acceptable.
• run_block still reads only run: | — non-blocking-improvement
  No workflow in the repo uses another form, and the PR says it left this alone on purpose. Widening it would also remove the second non-blocking item above.

Positive signals

• gpu_preflight_steps is rebuilt on job_ids → job_block → job_steps → step_name. The preflight lookup and the presence check now share one definition of a step. The test also excludes a step inside a heredoc, a commented-out step and a uses:-only job.
• The new tests were mutation-checked. Each of the following turns a test red:
  • removing the column-0 # skip
  • disabling the block-scalar branch
  • dropping blank-line handling inside a wrapped name
  • removing strip_comment
  • reverting gpu_preflight_steps to the raw-line scan
• The "waits for nothing" assertion message now says what the check can and cannot see.

Deployment notes

None. The change is test-module-only, in xtask.

What this covered

All of xtask/src/workflow_contract.rs was read completely, in four commits. The helpers and their callers were checked against .github/workflows/. Diff scope: prw-base...HEAD, ee3131be…0107b87c, one file, all inside mod tests. The review fanned out to three workers, and all returned:

• a code pass over the whole file
• the source-reading passes (instruction adherence, history, prior changes on earlier PRs, comment accuracy)
• a design pass

Runs I did:

• cargo test -p xtask at HEAD: 274 passed.
• Five single-edit mutations, one at a time with cargo test -q -p xtask workflow_contract (results above):
  • remove the # skip → the new column-0 test fails
  • disable the trim loop → the Dependabot read-only test fails
  • make the block-scalar branch unconditional → both new step tests fail
  • narrow the blank-line handling in a wrapped name → the wrapped-name test fails
  • remove strip_comment → the wrapped-name test fails
• One further mutation, reverting gpu_preflight_steps to the raw-line scan → the block-scalar preflight test fails.

Not run: cargo clippy, which CI reports green on this head, and the Windows-target clippy. The mutations proposed by the workers that survive by reading (strip_quotes removal, starts_with → contains) were not run. The survivors would change nothing today, and no fixture exercises them (below the reporting bar).

CI at this head, from statusCheckRollup:

• Green: clippy, build-and-test, Windows build-and-test, prek, commit signatures and sign-off, Strix Halo Ubuntu/Windows/WSL2 E2E, CodeQL, and the rest.
• "E2E tests (MI300X)" is QUEUED and has not reported.
• "Sphinx docs build (-W)" and "Skill checks" are SKIPPED, not failed.

No Gherkin scenario is needed: the PR text says why (internal CI-contract test, no user-observable CLI behaviour), and the change confirms that. The leak scan over the diff found nothing, and all four commits carry DCO sign-off. The history and prior-changes passes ran. The PR's own discussion and earlier reviews were not read here and nothing was reconciled against them.

[rominf]

On the automated review of 0107b87c, all addressed in 60ea4631:

• The doc comment on top_level_block (blocking): reworded to what it does. It stops at the next column-zero key; column-zero comments inside the block do not end it; and comment and blank lines directly above the next key are left out.
• The trim had no test: a_column_zero_comment_does_not_end_the_jobs_block now also asserts that a comment directly above env: stays out of the jobs block. Removing the trim turns it red.
• Only the presence check named the run: form it reads: the preflight-script doc and the clock-settling check's failure message now say the same thing (only the literal run: | form is read; run: |-, run: > and one-line run: are not).
• Decisions:
  • The single-space join in step_name stays, since callers only match a prefix or print the name.
  • Building gpu_preflight_steps on job_block is deliberate: it is what gives the lookup and the presence check one definition of a step.
  • run_block stays run: |-only, as discussed in the earlier round.

Verified locally:

• fmt
• cargo test -p xtask (274)
• clippy for xtask, on Linux and for x86_64-pc-windows-gnu

This reviewed 0107b87. The head is now 60ea463: top_level_block's doc now matches its behaviour, the column-zero test pins the trailing-comment trim (removing the trim turns it red), and the other run: callers name the one form they read. All required checks are green on 60ea463.

[rominf]

Rebased onto main af315e0a (#454 and #672); the head is now 0ba2cc6e.

• Conflict: one textual conflict, at the point where step_name is inserted. Both sides are kept, and the PR's diff is line-for-line unchanged.
• New lanes: job_ids and top_level_block read the new W7900 lanes in e2e-selfhosted.yml and nightly.yml. Both lanes declare timeout-minutes and have one GPU preflight step. gpu_preflight_steps finds the same steps as a raw line count.
• Checks: cargo test -p xtask passes (264), and workspace clippy -D warnings is clean.
• Overlap: this overlaps test(workflows): fail a pwsh step that can mask a native failure #658's one-argument step_name(step). If test(workflows): fail a pwsh step that can mask a native failure #658 merges first, the wrapped-name handling will move behind that signature.

[rominf]

Rebased onto main after #658 merged; the head is now 55b657ce.

#658 and this PR each added a step_name helper, and they are now one:

• step_key(step, key) keeps test(workflows): fail a pwsh step that can mask a native failure #658's one-line lookup.
• step_name(step) keeps test(workflows): fail a pwsh step that can mask a native failure #658's one-argument signature and reads wrapped names (name: >- and the like), as this PR intended.
• job_steps and pwsh_steps both use step_name. A new test pins that pwsh_steps reads a wrapped name, since its allow-list is keyed on step names.
• step_key(step, "run") deliberately does not wrap, so a script's lines are not joined with spaces.

Checks:

• Step names unchanged: the step names extracted from every real workflow are identical before and after; no workflow uses a wrapped name today.
• The rest of the PR is unchanged: three of its five commits are identical apart from context. The other two differ only in the helper region.
• Mutation checks: all six mutations of the wrapped-name handling fail a test.
• Gate: cargo test -p xtask passes (321, also on a merge with current main), and clippy -D warnings passes for both invocations.