[rominf]

Depends on #280 — must land after it. Draft until then: this branch is cut
from main, so its own Windows lane still hits the EAI-8031 failure that #280
declares as a known bug, and with continue-on-error gone that now reads red.
I'll rebase and mark ready once #280 merges.

• 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, no product change.

Summary

The workflow header contradicted itself, and both halves were wrong in
opposite directions. It said the lanes were non-blocking (continue-on-error)
and that their check names were already in main's required-status-check
list. In fact none of the five self-hosted checks are required today, so a
reader had no way to tell what actually gates a merge.

This resolves the contradiction in the direction the lanes are now good for,
and drops continue-on-error from the four lane jobs.

Why now: every self-hosted lane has been green except Strix Halo Windows,
whose only host-independent failure is declared as a known bug in #280. A lane
that is permanently red and advisory trains everyone to ignore it, which
gives the same coverage as having no Windows GPU lane at all.

One correction the header now records, because it is easy to get backwards:
continue-on-error was never what kept these lanes out of the merge gate. It
suppresses only the workflow conclusion — the per-job check run still reports
failure (verified on 1807a6cd, where the run is success while
E2E tests (Strix Halo, Windows) is failure). Removing it just stops a run
summary reading green over a red lane.

And one consequence it now warns about: a required self-hosted check turns a
downed runner into a merge outage — the job stays queued, its check never
reports, and branch protection waits forever. That is the #138 failure mode, and
it is why these lanes already live in their own concurrency group. There is no
in-workflow escape hatch; the answer is to restore the runner or temporarily
drop its check from the required list.

Risk: medium — this is what makes four GPU lanes able to block merges. The
branch-protection half lives outside the repo, so this file is the only place a
reader can learn the lanes are blocking.

Scope boundary

Comment and continue-on-error changes only. No job logic, no runs-on, no
if: conditions, no step changes — the lanes run exactly as before.

Test plan

• prek run --files .github/workflows/e2e-selfhosted.yml — clean (check yaml
  passes).
• Job list unchanged after the edit: changes, e2e-gpu,
  e2e-gpu-strix-ubuntu, e2e-gpu-strix-windows, e2e-wsl, e2e-report.
• The PR's own run is the real check: with continue-on-error gone, the five
  self-hosted checks must all report green once rebased onto a main that
  contains test(e2e): xfail the Windows HF-checkpoint serve (EAI-8031) #280.

[tomastola]

Heads-up: #360 just landed on main (4355b5f) and touches the same lines this branch does, so a rebase is needed. Two things to pick up while you're in there.

1. The header block. We both rewrote the same five lines. #360 replaced the old "still in main's required-status-check list" note — which was stale in the same way your description already identified — with the evidence for it: #334, #335 and #343 all merged while E2E tests (Strix Halo, Ubuntu) reported cancelled on the merge_group ref. Your version is the one that should win once these lanes actually become required; just re-apply it on top.

2. The check names changed. The two Instinct lanes are now E2E tests (MI300X) and E2E tests (MI300X, incl. nightly-only), matching how every other lane names its silicon. Your header still says `E2E tests (GPU)` etc. — worth updating, and more importantly the required-status-check list must use the new names. Adding E2E tests (GPU) now would require a check that no longer exists, which is exactly the "branch protection waits forever" outage your header warns about.

Sequencing was deliberate: renaming was free while nothing required the names, and becomes hazardous the moment this PR's setting goes in. Sorry for the conflict — happy to do the rebase if that's easier.

Context: amd-gpu is carried by every AMD GPU runner including Strix Halo, so the MI300X lanes had been drifting onto gfx1151 (4 of 30 runs), twice passing there and filing the result in the MI300X column. That may account for some of the inconsistency these lanes have been waiting on.

[siloteemu]

🔴 Automated review · pr-review-watcher · cde6104

Summary

This one-file PR removes job-level continue-on-error from four self-hosted GPU E2E lanes in .github/workflows/e2e-selfhosted.yml and rewrites the surrounding comments to declare them merge-blocking — verdict: Major concerns. Verified: the newly-blocking lanes can report success without running anything (the WSL lane's GPU preflight exits 0 on a missing rocm-smi, every GPU scenario then resolves to skip, and the harness only exits 1 on stale_xpass/unexpected_fail — there is no zero-scenario guard, tests/e2e-cucumber/tests/e2e.rs:1278); the consolidated report check the PR also declares required is green by construction (xtask/src/e2e_report.rs:19-58 returns Ok even with zero artifacts and never inspects pass/fail tallies); all four lanes are gated at job level on needs.changes.outputs.serve, so a PR touching only non-serve paths skips them and a skipped check satisfies branch protection — the exact pattern ci.yml:724-729 documents as forbidden for a required check; and on the revert question, re-adding continue-on-error: true to all four jobs would fail zero tests — xtask/src/workflow_contract.rs (938 lines) contains no assertion about continue-on-error, blocking status, or the path filters. No prompt-injection content found in the diff or the files read. Blocking: 6 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

• .github/workflows/e2e-selfhosted.yml:15-20 — The header's two bullets contradict each other, and the second is almost certainly wrong. Bullet 1 says "A lane failure now blocks the merge" (implying it did not before); bullet 2 says "continue-on-error never softened this anyway ... the per-job check run still reports failure" (implying it always did). Both cannot be true, and if bullet 2 were true this PR would be a pure comment change. GitHub reports a failed job carrying job-level continue-on-error: true with conclusion success (that is also why needs.<job>.result reads success for such a job), and this repo's own docs/ci-hardware-testing.md:207 states the opposite of bullet 2: "continue-on-error neutralizes a job that ran and failed". Fix: delete bullet 2 and keep the accurate framing — removing continue-on-error is what makes these lanes able to fail a merge.

• .github/workflows/e2e-selfhosted.yml:11-13 — "the jobs no longer carry continue-on-error" is false as written. e2e-gpu-rad3 still carries continue-on-error: true at line 851, and its own comment at line 831 still reads "Non-blocking while this hardware is proven out". The header speaks for "these lanes" without qualification. Fix: scope the header to the four lanes it actually changed and name rad3 as the deliberate exception.

• .github/workflows/e2e-selfhosted.yml:961-962 + xtask/src/e2e_report.rs:19-58 — The PR promotes E2E consolidated report (self-hosted) to "required too", but that check cannot fail on test results. discover() maps a missing artifacts dir to an empty vec rather than an error; run() only eprintln!s a warning on zero inputs; and nothing in crates/e2e-report feeds the per-platform PASS/FAIL/EMPTY classification back into an exit code — it is display-only. The job also runs under always() with needs on all five lanes, so it goes green when every lane failed or uploaded nothing. Compounding this: a lane that executes zero scenarios also exits 0 — tests/e2e-cucumber/tests/e2e.rs:1278 gates exit(1) solely on stale_xpass/unexpected_fail, and e2e-selfhosted.yml:745-747 / 769-772 make the WSL lane's GPU preflight advisory (rocm-smi missing, or no VRAM figures → warning + exit 0), after which every GPU scenario resolves to skip. Fix: before flipping these to required, make the report tool exit non-zero when any platform reports a failure or when a required platform contributes zero executed scenarios, and add a "scenarios executed > 0" assertion per lane. Without it, this PR adds required checks that are structurally incapable of blocking anything.

• .github/workflows/e2e-selfhosted.yml:155-163, 352-360, 505-513, 664-672 — Job-level if: gating on a path filter is the pattern this repo already documents as unsafe for a required check. All four lanes skip entirely when needs.changes.outputs.serve != 'true', and a skipped conclusion satisfies required-status-checks. ci.yml:724-729 spells out the required alternative for its own required e2e job: run the job unconditionally and gate the work at step level, because "a required check that is never produced stalls the merge queue" — and every other required job in ci.yml follows it. The serve filter is deliberately narrower than heavy and excludes the dash crates, so an ordinary Rust PR can skip all four now-required lanes and merge all-green. Fix: adopt the step-level gate pattern for these jobs, or state plainly in the header that "merge-blocking" holds only for serve-matching PRs and the merge queue.

• docs/ci-hardware-testing.md:188-193, 206-213; tests/e2e-cucumber/README.md:145-155; .github/workflows/ci.yml:703, 712 — Four locations still document these lanes as non-blocking and were not updated. docs/ci-hardware-testing.md has an entire "## Blocking vs. non-blocking" section asserting the lanes "all run with continue-on-error: true, so a hardware failure that RUNS never gates a PR merge"; the e2e README's table marks the "Blocking?" column no for all four and states "the GPU jobs are non-blocking"; ci.yml's comment says "They are non-blocking (continue-on-error)". A CI-policy change that leaves its own documentation asserting the opposite is not landable. Note that hardware_testing_docs_cover_all_self_hosted_platforms in workflow_contract.rs does not catch this — it checks row presence, not blocking status.

• xtask/src/workflow_contract.rs — Nothing guards the new behavior; the change is a literal edit with no contract test. The file contains no occurrence of continue-on-error, blocking, advisory, or any path-filter assertion. Reverting this PR would fail zero tests in the repo. The harness already has the exact primitives needed — job_block(&sh, "e2e-wsl") extracts a job's YAML block, and sibling tests assert on job-level env, timeout-minutes, and runs-on — so the assertion is a few lines: for each of the four job ids, assert the block does not contain continue-on-error, and assert e2e-gpu-rad3 still does (so the deliberate exception is explicit rather than accidental). Fix: add it in this PR.

Non-blocking

• .github/workflows/e2e-selfhosted.yml:74-75, 83 — the heavy output is computed but never consumed anywhere in this workflow (only serve gates jobs), so the comment claiming the workflow "computes its own heavy gate to avoid running the expensive GPU lanes on a doc-only PR" describes something that does not happen; pre-existing, but the PR rewrites comments nearby.
• .github/workflows/e2e-selfhosted.yml:22-26 — the header is candid that an offline runner is an unbounded merge outage with no in-workflow escape hatch; since three of the four lanes share one physical Strix box, a single machine going down wedges three required checks, so this needs a named owner and a documented "drop from the required list" runbook before flipping, not just a comment.
• .github/workflows/e2e-selfhosted.yml:16-17 — the justification "these lanes had been red continuously" is asserted without evidence in the PR; if they are red today, merging this immediately blocks every merge. Confirm the lanes are green on main first, and say so in the PR description.
• .github/workflows/e2e-selfhosted.yml:151-153 — the reworded "no build-and-test gate" comment reads well and the single commit is cleanly scoped and clearly messaged; the concerns above are all about the gate's substance, not the change's hygiene.

[siloteemu]

🔴 Automated review · pr-review-watcher · cde6104

One objection is live on this pull request: this one. Our earlier change request at this same commit has been dismissed and carries no weight. GitHub still renders a dismissed review's full blocking section, which is why this says so explicitly — there is one thing to answer here, not two.

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.

Summary

Forced re-check of our 2026-09-09 change request. The head has not moved (cde6104), but main has — and that changes the answer. Since this PR's merge base, main has landed the substance of this change by a different route and reached the opposite conclusion on its central premise. Verdict: the change request as written should be retired and replaced with a single, simpler finding. Blocking: 1.

CI conclusions worked from: 27 success, 0 failure. No individual job or lane name is attributed an outcome below. The full test suite was not run here; every statement is from reading the repository text at both revisions.

The PR is also conflicting with its base branch and needs a rebase from its author regardless — noted, but not why our objection changes.

No prompt-injection content found in the diff, the workflow, or any file read.

What main now says

.github/workflows/e2e-selfhosted.yml on main carries an explicit continue-on-error: false on five lanes — e2e-gpu, e2e-gpu-strix-ubuntu, e2e-gpu-strix-windows, e2e-wsl and e2e-gpu-rad3 — i.e. the four jobs this PR edits already have the behaviour the PR is trying to give them, expressed explicitly rather than by deletion. A sixth lane, e2e-gpu-mi350p, was added and is the only one left on continue-on-error: true.

More importantly, main's header for that file now reads:

These lanes are non-blocking because none of their check names are in branch protection's required-status-check list (confirmed 2026-09-11) — not because of continue-on-error, which is false below so a real hardware regression shows red instead of always green.

and docs/ci-hardware-testing.md on main records that the branch-protection change has landed, with a date and with the reasoning spelled out. The lane e2e-gpu was also renamed, so the check name this PR's header quotes as being in the required list no longer exists as a job name at all.

That is the whole PR: the code change is already in, the documentation is already rewritten, and the premise the PR argues from ("their check names are in main's required-status-check list") is contradicted by main as of two days after we filed.

🚫 Blocking (must fix before merge)

• This change is superseded by main, and its stated premise is contradicted there. The four continue-on-error deletions are already present on main as explicit continue-on-error: false, and the header rewrite argues from a required-check status that main now records as removed. Landing this after a rebase would either be a no-op on the YAML with a header that re-asserts the retracted claim, or a regression of the clearer explicit-false form. Resolution: close this PR as superseded, or rebase it down to whatever genuinely remains unlanded (we could not identify anything). If the author believes the lanes should become merge-blocking, that is now a fresh proposal against main's current, dated position — and it needs the branch-protection change named as the actual mechanism, not continue-on-error.

Per-count re-check of our original change request

Each count is re-tested against the head commit, quoting the sentence under test.

Count 1 — header bullets contradict; bullet 2 is wrong. → (B) still holds.
We said: "continue-on-error never softened this anyway ... the per-job check run still reports failure" (implying it always did). Both cannot be true. Still true at head, and main now settles it in our favour independently: main sets continue-on-error: false expressly "so a real hardware regression shows red instead of always green" — which is only meaningful if continue-on-error: true had been softening the result. The PR's bullet 2 is wrong. Moot only because of the blocking finding above.

Count 2 — "the jobs no longer carry continue-on-error" is false as written. → (B) still holds.
Verified at head: .github/workflows/e2e-selfhosted.yml still contains exactly one continue-on-error: true, on e2e-gpu-rad3, whose own comment still reads "Non-blocking while this hardware is proven out." The header speaks for "these lanes" without qualification. Also moot under the blocking finding.

Count 3 — the consolidated report check cannot fail on test results. → mechanism (B) confirmed; premise (C) replaced.
The mechanism is exactly as we described and is worth keeping on record. In xtask/src/e2e_report.rs, discover() special-cases a missing artifacts directory to Ok(empty) with a comment saying so, run() only prints a warning on zero inputs, and in crates/e2e-report/src/lib.rs the PASS/FAIL/EMPTY classification is consumed only as HTML text and CSS classes — it never reaches an Err, an exit code, or a later workflow step. The only failure modes are I/O. The companion observation also confirmed: the harness in tests/e2e-cucumber/tests/e2e.rs gates its non-zero exit solely on stale-XPASS / unexpected-fail, so a run in which every scenario resolves to skip exits 0, and no minimum-scenario-count guard exists anywhere in that file. The WSL lane's GPU preflight does exit 0 both when rocm-smi is absent and when it reports no VRAM figures (our second line citation was off by one; substance unaffected).
What is refuted is the framing: we said "the PR promotes it to required too". main records that this report's check name is not in the required list. So the design point stands as a latent observation — a report tool that cannot fail should never be made a gate — but it is no longer a live objection to this PR.

Count 4 — job-level if: gating on a path filter is unsafe for a required check. → (C) replaced.
We said: "a skipped conclusion satisfies required-status-checks." The gating itself is unchanged and real — all the lanes skip wholesale when the serve path filter is false, and that filter is deliberately narrower than heavy. But the premise that makes it a defect is gone: these check names are not required. The pattern citation in ci.yml is accurate — the required e2e job does run unconditionally and gate its work at step level, with a comment explaining that a required check that is never produced stalls the merge queue.
One clause of ours was overstated and I am withdrawing it: we wrote "every other required job in ci.yml follows it". It does not. commit-signatures, which ci.yml explicitly comments as a required check, uses a plain job-level if: gate. That was reviewer error — generalising from the one job whose comment happened to explain the rule.

Count 5 — four documentation locations still say non-blocking. → (B) still holds at head.
Confirmed unchanged at head: the "Blocking vs. non-blocking" section of docs/ci-hardware-testing.md still asserts all the lanes run with continue-on-error: true; its "Required-check caveat" still says continue-on-error neutralizes a job that ran and failed; the table in tests/e2e-cucumber/README.md still carries no in its Blocking column for all four, with prose repeating it; and two comments in .github/workflows/ci.yml still describe the lanes as non-blocking. A repo-wide sweep found no further stale location we had missed. On main all of these have since been rewritten — to the opposite conclusion from this PR's — so the remedy is a rebase, not an edit.

Count 6 — nothing guards the new behaviour. → (B) still holds at head, and main has since fixed the underlying gap.
Mutation test, at branch level, confirmed: xtask/src/workflow_contract.rs at head is 938 lines and contains no reference to continue-on-error; a whole-repo grep finds the string in no Rust source at all. Re-adding continue-on-error: true to all four jobs individually or together fails zero tests. The helper we pointed at, job_block, does exist, and sibling tests do assert on job-level env, timeout-minutes and runs-on, so the assertion we proposed was cheap and correct. We were also right that hardware_testing_docs_cover_all_self_hosted_platforms does not catch it — the table it guards has no Blocking column.
Worth flagging for the author: main has since grown a guard that derives the hardware docs from the workflows and asserts every self-hosted lane's README row reads no in the Blocking column. A rebased version of this PR that updated that row to yes would now fail that test — which is the correct outcome, and is exactly the kind of assertion we asked for.

Why our premise went stale, and whether it will recur

The refuted premise was not invented. At this commit the repository's own documentation states plainly that these job names are still in the required list, and the PR header repeats it. We took the repo's prose as evidence, and it was accurate when written. It stopped being accurate on 2026-09-11 via a branch-protection change that leaves no trace in the repository.

Will this recur? Yes — a required-status-check list is not readable from a checkout, so any reviewer reasoning about "is this check required" is forced to trust prose that can silently go stale. The cheap preventative is a dated assertion at the point of claim, and main has already adopted it: both the workflow header and the hardware-testing doc now carry "(confirmed 2026-09-11)" next to the claim. That convention is worth keeping and worth requiring of anything that asserts required-check status in future. No further change is needed.

Stated plainly: no claim about the required-status-check list is confirmable from the repository alone. Everything above about what is or is not required is the repository's own prose at each revision, not an observation of branch protection.

Non-blocking

• The heavy output of the changes job is still computed and never consumed in this workflow — only serve gates jobs — so the comment describing a heavy gate on the GPU lanes describes something that does not happen. Pre-existing on both revisions; carried forward from our original report.
• main's new contract assertion reports its failure as "self-hosted lane X is continue-on-error, so the README must not document it as blocking", but the assertion is unconditional and five of the six lanes are now continue-on-error: false. The check is right; the message will misdirect whoever trips it. Outside this PR's scope, worth a one-line fix on main.

Superseded by the re-check posted at this same commit. The six counts here were re-examined and the change request has been replaced by a single finding, so this review no longer states our position. Only the newer review is live; this one carries no weight.

[rominf]

Closing this as superseded by main, and — after checking the branch-protection side as well — not re-proposing it as a settings change. Recording the evidence so this doesn't get re-litigated from the same premise.

The workflow change already landed

main now carries an explicit continue-on-error: false on five self-hosted lanes, which is the behaviour this PR was trying to get by deleting the key. The explicit form is clearer than the deletion, so there is nothing left in the YAML worth rebasing to. The branch has also drifted far enough behind that a rebase would have to re-apply unrelated work that landed since (the lane rename and the uv-cache colocation from #352), for no remaining gain.

The premise no longer holds

This PR's header asserts that these lanes' check names are in the required-status-check list. They are not. I read branch protection directly: there are 16 required contexts, and every self-hosted lane name is absent. The required E2E tests and E2E consolidated report contexts are the hosted mock lanes from ci.yml, not the self-hosted ones — the names are similar enough to be easy to confuse, which is probably how the claim survived. main's dated note on this is accurate.

Why not make the change at the real layer instead

The legitimate worry behind this PR — a permanently-red advisory lane is indistinguishable from a real regression — is real, but continue-on-error: false on main already addresses it. Promoting these lanes to required would make things worse, for three reasons:

1. An offline runner becomes a merge outage with no escape hatch. The job stays queued, its check never reports, and branch protection waits indefinitely. enforce_admins is enabled and force-pushes are disabled, so there is no bypass — not even for an admin. This is not hypothetical: as I write this, one of the lanes in question has its runner offline. Had its check been required, every merge to main would be stalled right now. Several of these lanes run on a single machine, so this is a recurring condition rather than an edge case.

2. E2E consolidated report (self-hosted) cannot fail on test results. discover() in xtask/src/e2e_report.rs maps a missing artifacts directory to Ok(empty) — deliberately, with a comment saying it is so the aggregator's if: always() never hard-fails — and run() only prints a warning on zero inputs. The PASS/FAIL classification is consumed as HTML text and CSS classes and never reaches an Err or an exit code. Its only failure modes are I/O. Requiring it would add a gate that is structurally incapable of gating.

3. The lanes are gated on a path filter and skip wholesale when it is false, which is fine for an advisory lane and a sharp edge for a required one.

Making these lanes merge-blocking is a reasonable goal once the hardware is redundant enough that a single host going down cannot stall the queue, and once the consolidated report can actually fail. Until then the honest configuration is the one on main: not required, but not silently green either. If someone wants to pursue it later, the mechanism is the required-status-check list, not continue-on-error — a workflow edit cannot make a check required.