[jussielo-amd]

Closes #893.

Why

Since #875 build-and-test no longer waits for clippy, and since #718 the E2E gate opens once build-and-test has finished with success or failure. A run where clippy fails therefore still spends the whole E2E matrix (one job per Ubuntu image) on a change that the required clippy check blocks regardless. In the sample #875 used, clippy failed in 10 of 192 completed pull request runs (about 5%), and the issue measured 5 to 7.5 minutes of runner time per E2E job, so the saving is roughly 10 to 15 runner-minutes on a lint-red run and a few tenths of a minute averaged over all runs. The run history since #875 merged (below) points the same way, with a lower lint-red rate.

What changes

The e2e job now declares needs: [changes, clippy, build-and-test], and its "Decide whether to run" step opens the scenarios only when clippy succeeded, on top of the existing conditions (build-and-test reached a verdict, the change is heavy). A manual dispatch skips clippy and still opens the gate, because that branch is evaluated first.

On a heavy run where clippy is not green the gate prints a ::notice:: naming the clippy result and writes run=false. The required E2E check then reports success without having run a scenario, which is the same green-but-empty shape the gate's own comment already describes for a skipped build. That is harmless only while clippy is itself required, which is the first point below. The notice says so on the run page instead of leaving a silent pass. Failing the job instead was rejected: it would add a second, misleading red to a lint error.

The gate requires clippy to be exactly success rather than the issue's "success or skipped", and a one-line comment at the gate now says the difference from pr-linux-binaries is deliberate. The two are equivalent today. clippy has if: github.event_name != 'workflow_dispatch' and needs: changes, so it is skipped only on a dispatch (handled by the gate's first branch) or when changes did not succeed (in which case the e2e job's own if: keeps it from running at all). The stricter form fails closed if that ever changes.

Review points

Is clippy required in the merge queue? Read from the required-check flags on this pull request's check contexts (GraphQL isRequired(pullRequestNumber: 894)), the required contexts include clippy, E2E tests and E2E consolidated report. So the skip makes the required E2E tests check report green only while the required clippy check itself blocks the merge. This is evidence from those flags, not from the ruleset settings page, which I have not read, so a maintainer with access can confirm it there. This replaces the earlier wording in this description, which only said the pull request check list showed clippy as required.

The post-merge measurement promised in #875. I could not find it: #875, #893 and #871 have no comments, and I found no other issue or pull request that posts it. I did not invent one. What I can read from the Actions API for the CI workflow since #875 merged (2026-11-04 16:36 UTC), looked at on 2026-11-05, is below, and it is only a slice of what #875 proposed. The part about build-and-test's start time and whole-run wall-clock is not recounted here, because it does not bear on whether this change is safe or worth its cost.

• Push runs on main: 18 (the merge commit's own run and the 17 after it), which is fewer than the 20 ci: start build-and-test without waiting for clippy #875 asked for. clippy succeeded in all 18. In the 16 where build-and-test ran, clippy finished before the later of the two build-and-test legs every time (by at least 1.7 minutes, median 2.9), and E2E started at least 2.0 minutes after clippy finished. So making E2E wait for clippy would have added no wall-clock time in any of them, which is the "no added time on a green run" claim in ci: skip the E2E work on a clippy-red run #893. The two runs where build-and-test did not run failed elsewhere, not in clippy.
• Pull request runs since the merge: 112 listed, of which 101 completed without being cancelled (83 success, 18 failure), 5 were cancelled and 6 had not finished. clippy failed in 2 of the 101 (about 2%, against about 5% in the sample ci: start build-and-test without waiting for clippy #875 used). In both of those, E2E ran in full: run 41997828713 (E2E jobs of 8.4 and 6.7 minutes) and run 42029667439 (4.7 and 8.7 minutes), so 15.1 and 13.4 runner-minutes of job time that this change would not have spent. Cancelled and unfinished runs are not counted and could hide further lint-red runs, and two runs is a small sample, so this confirms the direction and the order of magnitude and nothing more.
• Averaged over those 101 runs the saving is roughly 0.3 runner-minutes per run, which is small. It is worth having only because the change is a few lines and cannot slow a green run, and the issue already named that as the condition.

Ordering against #871. #871 is an open issue and nothing for it has landed, so this change does not depend on it and it does not depend on this change. They interact only in one direction: today E2E waits for build-and-test (about 8.5 minutes on the green runs #875 sampled) and clippy finishes in 1.5 to 4 minutes, so waiting for clippy adds nothing. If #871 later makes build-and-test much shorter, E2E could become ready before clippy finishes on some runs, and this gate would then add at most the remaining clippy time (never more than clippy's own duration). That is the point to re-run the measurement from #875 on main, and it is not a reason to hold this change.

Clippy gate versus the sibling gate. Covered above and by the comment at the gate.

Verification

The workflow contract tests feed the clippy result into the gate script and run it under bash: e2e_gate_skips_scenarios_unless_clippy_passed covers failure, cancelled and skipped against both build verdicts on push, pull request and merge group, the passing case, a non-heavy change, and a dispatch, and asserts the notice and the run=false together. The needs pin and the other two gate tests were updated for the new input. In response to review, the gate test's notice assertions now live in one assert_e2e_gate helper and run_e2e_gate returns a named E2eGateOutcome instead of a (String, String). The rows, expected values and test names are unchanged.

I broke the target and each break made the test fail: narrowing the skip to failure only, dropping the heavy guard from the new branch, removing clippy from needs, and replacing the notice with a non-annotation line. After the helper refactor I re-ran three of them against the refactored test (narrowing the skip, dropping the heavy guard, and breaking the notice) and each still failed it; the needs pin is a separate test that the refactor did not touch. cargo test -p xtask passes (399 tests, the same count as before the refactor), as do cargo clippy -p xtask --all-targets -- -D warnings, cargo fmt --all --check, cargo xtask manifest --check, and the commit hooks (YAML check, license headers, links, fmt).

What CI covers and what it does not. The skip path is exercised only by the xtask contract test, which runs the gate's real shell script under bash with each input fed in; no CI run drives a real lint-red pull request through the live gate step and needs wiring. As far as I read the workflow, the contract tests run in CI through the Test (affected crates) job (nextest over the affected crates, the whole workspace on push), on both Ubuntu images; they are cfg(unix), so the Windows job does not run them. The notice's rendering on the run page and the live needs.clippy.result value can only be observed on a real lint-red run after merge, and the run-history figures above are the only real-run evidence.

actionlint is not installed here and the repository does not configure it; the workflow files are checked by the YAML hook and by the contract tests.

No Gherkin scenario is added: this is CI plumbing and no CLI behaviour changes.

The Strix Halo hardware E2E lane that failed in a separate self-hosted run is the known serve-40 expected failure tracked in #898 and is not touched here.

Scope

build-and-test still runs on a lint-red run; only E2E is skipped. e2e-report is unchanged: with no scenario report it says none were found, and the reason is in the notice on the E2E job.

• 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. (Not applicable: not a bug fix.)
• If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md. (Not applicable.)
• Every new or changed user-facing message was read against the code path that runs after it, and its test asserts the resulting state, not only the wording. (The only new message is the CI notice: its claim that no scenario ran is asserted as run=false in the same test.)

[Copilot]

🔵 Needs a closer look

Required E2E checks can now pass without running scenarios when clippy is not green, while clippy’s independent requirement for merge groups remains unverified and needs maintainer confirmation.

No open issues 🎉

What changed in this PR

This PR reduces unnecessary CI work by skipping E2E scenarios when clippy has not succeeded, while keeping build-and-test running independently of lint.

Changes:

• Makes E2E wait for clippy and emits a notice when a heavy change skips scenarios because lint did not pass.
• Preserves manual-dispatch behavior and E2E execution after either a successful or failed build when clippy passes.
• Extends workflow contract tests to cover clippy outcomes, skip notices, and the updated job dependencies.

File	Description
xtask/​src/​workflow_contract.rs	Adds clippy-result and notice assertions and updates E2E dependency checks.
.github/​workflows/​ci.yml	Adds clippy to E2E dependencies, gates scenarios on its result, and updates explanatory comments.

_{Review effort: Balanced}

[juhovainio]

Reviewed at 9798dcc. No correctness bugs found in the gate or the new xtask test; the requests below are to settle before merge.

Requested changes

• Confirm clippy is a required check in the merge-queue ruleset. The skip makes the required E2E check report green without running a scenario, which is only safe while clippy itself blocks the merge; the PR says this was not verified.
• #893 asked for this to follow the post-merge measurement promised in #875. The PR cites the #875 sample but not that measurement; please link it or note why it is unnecessary.
• Clippy is fully gated on success here but on success || skipped at the sibling gate (ci.yml ~L1129). Equivalent today (clippy is skipped only on dispatch, which is handled first), but see the inline note.

Notes

• A clippy infra flake now turns the required E2E check green with only a ::notice. Acceptable given clippy is required.
• Test nit: the notice assertions are duplicated across the loops, and run_e2e_gate returns an unnamed (String, String) tuple.
• CI: all checks in the main run pass. The Strix Halo hardware E2E lane failed in a separate self-hosted run and its logs were unavailable while it was in progress; it looks unrelated to a gate-only diff but is not proven. The new skip path is exercised only by the xtask test, not in CI.

[juhovainio]

Nit: the sibling gate at ~L1129 accepts success || skipped for clippy, while this one requires success. The rationale is in the comment above, but a one-line pointer here saying the difference is deliberate would stop a future reader from "aligning" them.

[jussielo-amd]

Added a one-line comment at the new gate saying the difference is deliberate (5ea5f28): clippy has if: github.event_name != 'workflow_dispatch', so it is skipped only on a dispatch, which the gate's first branch handles, and a skip anywhere else must not open the gate.

[jussielo-amd]

Pushed 5ea5f28 and 111bf78 and updated the PR body, point by point on your review.

1. Required check: I read it from the required-check flags on this PR's check contexts (isRequired for PR 894, not the ruleset settings page): the required contexts include clippy, E2E tests and E2E consolidated report. So the skip turns the required E2E tests check green only while the required clippy check itself blocks the merge. The body now says this and that it comes from those flags.
2. The ci: start build-and-test without waiting for clippy #875 measurement: it was never posted (ci: start build-and-test without waiting for clippy #875, ci: skip the E2E work on a clippy-red run #893 and ci: move the install lifecycle and self-test steps out of build-and-test so E2E starts sooner #871 have no comments, and no other issue or PR has it), so the body now says that and gives what I could read from the run history since ci: start build-and-test without waiting for clippy #875 merged. Of 18 push runs on main, clippy was green in all 18 and, in the 16 where build-and-test ran, it ended at least 1.7 minutes (median 2.9) before the later build-and-test leg, so waiting for it would have added no time. On PR runs clippy failed in 2 of 101 completed, non-cancelled runs (about 2%), and E2E ran in full in both. I did not count 5 cancelled and 6 unfinished PR runs, so the sample is small and the body says so.
3. success versus success || skipped: confirmed against the workflow (clippy is skipped only on workflow_dispatch, which the first branch handles) and now commented at the gate, see the inline reply.
4. Test nits: run_e2e_gate returns a named E2eGateOutcome and the notice assertions live in one assert_e2e_gate helper; rows, expected values and test names are unchanged. I re-ran the mutations after the refactor (narrowing the skip to failure, dropping the heavy guard, breaking the ::notice annotation) and each turns e2e_gate_skips_scenarios_unless_clippy_passed red. As you noted, the skip path is exercised only by the xtask test, which CI runs through Test (affected crates) on the Ubuntu images (the Windows job does not, since the test is unix-only); no real lint-red run goes through the live gate, and the body says so.

The Strix Halo hardware lane failure is the known serve-40 xfail issue (#898) and not related to this change. cargo test -p xtask passes (399) and clippy and fmt are clean. Could you take another look?

[Copilot]

🟢 Approval recommended

No active review issues are recorded for the change gating Linux E2E scenarios on clippy success.

No open issues 🎉

_{Review effort: Balanced}

fixed

[juhovainio]

I went through this against the saved diff and found nothing blocking. The gate logic holds up: the dispatch branch is checked first, the heavy guard is kept, and clippy also runs on merge_group, so the merge queue isn't stalled. The contract tests assert the notice and run=false together, and a scan of the diff for internal names came back clean.

A few things for you to weigh, none of them blockers:

• Issue #893 asked for this to land after the post-merge measurement from #875. That measurement doesn't exist yet, and the PR doesn't say how it is ordered against #871.
• Gating on success only (rather than success || skipped) is documented, so I'm fine with it.
• A skipped required E2E check reports green without running a scenario. That is accepted explicitly, but it only holds while clippy stays a required check.
• CI: Test (affected crates) (ubuntu-26.04) hit the 20m timeout while 24.04 passed in 6m. It looks unrelated to a workflow/xtask-only change, but a re-run would settle it. The Strix Halo E2E failures are attributed to serve-40 / #898; I didn't verify that.

Two small inline suggestions below. This is based on a read of the diff, not on CI results or a local run.

[juhovainio]

The notice says the required clippy check blocks the merge, but this branch also fires when clippy is cancelled, where nothing is red. Consider wording that doesn't assert merge-blocking, or one that says to re-run clippy and this job.

[jussielo-amd]

Good catch, you are right: the branch fires for every clippy result other than success, and a cancelled clippy is not red, so "the required clippy check blocks the merge" was not true for it. Fixed in b6b7829. The notice now reads "clippy did not succeed (result: X), so no scenario ran and this check reports success without one. Fix or re-run clippy, then re-run this job to run the scenarios." That holds for failure, cancelled and skipped alike and makes no claim about the merge. The contract test asserts this wording together with run=false for failure, cancelled and skipped, and I confirmed it goes red if the notice goes back to saying the merge is blocked or if that branch writes run=true.

[juhovainio]

This test hand-rolls the ${{ }} substitution that run_e2e_gate also does, so every new context the gate reads has to be added in two places. Consider calling run_e2e_gate here instead.

[jussielo-amd]

Agreed, fixed in 20858e3. The test now calls assert_e2e_gate, which goes through run_e2e_gate, so the expression substitution exists in one place and a new context the gate reads fails every gate test with "the E2E gate reads X, which this test does not feed" instead of needing a second edit here. The cases are unchanged, and each one now also checks that no clippy notice is printed when clippy passed. I checked that adding a new context to the gate makes the tests fail, and that changing which build result opens the gate does too. This was the only remaining hand-rolled copy of the substitution; the outcome struct you asked about already exists as E2eGateOutcome.

[Copilot]

🟢 Approval recommended

No active review issues remain with the clippy-dependent E2E gating change.

No open issues 🎉

_{Review effort: Balanced}

[juhovainio]

Re-reviewed at the current head. I found nothing blocking: the gate logic, the test that runs the real script (asserting run=false together with the notice), and the needs pin covering clippy all look right, and every check is green on this commit. Two small nits are left inline.

One thing for the author to keep in mind: the required E2E check now reports green without running scenarios when clippy is red. That is only safe while clippy stays a required check, and the PR notes the ruleset page was not read, so it is worth confirming that in the repo settings.

[juhovainio]

Nit: this comment is one very long line inside the script; wrap it like the surrounding comments.

[jussielo-amd]

Wrapped in 8300dc9. The comment is now three comment lines inside the run: | block, at the width of the neighbouring comments, with the words unchanged and the shell the step runs unchanged.

[juhovainio]

The notice says to re-run this job after fixing clippy. Worth confirming (on the first real lint-red run) that a re-run of only the E2E job reads the fresh needs.clippy.result; otherwise suggest re-running the workflow.

[jussielo-amd]

Good catch, and re-running only this job would not read a fresh result: a single-job re-run reuses the results the earlier attempt recorded for the jobs it needs, so it would see the old needs.clippy.result and skip the scenarios again. I could not check this against GitHub's documentation from here, so the wording is chosen to hold under either reading. Fixed in 8300dc9: the notice now says "Fix clippy if it is red, then re-run the workflow so that clippy and this job run again", keeping the (result: ...) part and the "no scenario ran and this check reports success without one" part. The contract test now requires "re-run the workflow" and rejects both "re-run this job" and "blocks the merge", together with run=false, for failure, cancelled and skipped; restoring the old wording turns e2e_gate_skips_scenarios_unless_clippy_passed red.

[jussielo-amd]

On the point for the author about the required checks: I read GitHub's own per-check required flags for this pull request (GraphQL isRequired) rather than the ruleset page, and the required contexts include clippy, E2E tests and E2E consolidated report, among others. So the required E2E tests check reports green without running scenarios only while the required clippy check is itself failing or missing, which still blocks the merge. A maintainer with access to the ruleset page can confirm the same there.

[Copilot]

🟢 Approval recommended

No active review issues remain for the change that skips E2E scenarios when clippy does not succeed.

No open issues 🎉

_{Review effort: Balanced}