[jussielo-amd]

Problem

Since #810 split the Windows job, the critical path of a push run on main is changes, clippy, build-and-test, E2E tests, E2E consolidated report. build-and-test declares needs: [changes, clippy, prek] with the comment "Don't spend a Linux build/test cycle until the cheap lint gate is green", so it does not start until clippy has finished. In 20 post-merge push runs on main, build-and-test started at a median of 2.9 minutes after run creation. Almost all of that is waiting for clippy, which ends at about 2.6 minutes, while prek ends at about 0.6 minutes.

Change

build-and-test now declares needs: [changes, prek], so it starts once prek is green and runs alongside clippy instead of behind it. The comment above needs: is rewritten to say so, and to explain why that is acceptable. The needs of test, pr-linux-binaries, remote-ssh, the e2e jobs and the Windows jobs are unchanged. I also corrected the comments that described the old ordering (on test, remote-ssh, the Windows lane and the clippy/build-and-test sccache sharing note), and added a workflow contract test, ci_build_and_test_waits_for_prek_but_not_clippy in xtask/src/workflow_contract.rs, that pins the needs list. The existing pins there cover only the Windows lane; this is a small pin for the one deliberate ordering decision on the Linux critical path, so a later edit cannot quietly put clippy's runtime back in front of the build. I checked it fails with clippy added back to needs (it reports ["changes", "clippy", "prek"] against ["changes", "prek"]) and passes again once restored. Because a clippy-red run now reaches the E2E job, two more contract tests pin its side of the ordering: ci_e2e_steps_after_the_gate_follow_its_decision (the job needs build-and-test, and every step after the gate step follows the gate's decision) and ci_e2e_gate_opens_only_once_build_and_test_ran_to_a_verdict, which runs the gate's own script over each event and build-and-test result and asserts that the E2E work opens only once build-and-test ran to a verdict (success or failure, as build-and-test is now a matrix over both images and a failure on one must not skip the other's E2E), or on a manual dispatch, and never for a skipped or cancelled build. I checked each of them goes red when the gate's &&, its else branch, its dispatch branch, its success comparison, a post-gate step's if: or the job's needs is broken.

What is kept

clippy is unchanged and still runs on every non-dispatch event, so a lint failure still shows up as a red clippy check. prek still gates build-and-test. Nothing treats a skipped build-and-test as a pass: the e2e job's gate step only runs the E2E work when needs.build-and-test.result is success or failure, that is, when it ran to a verdict, and e2e-report depends only on changes and e2e. What changes is a clippy-red run: before, build-and-test (and so the real E2E work) was skipped; now it runs. A prek-red run still skips build-and-test as before.

Trade-off and its limits

The figures below were measured before #718 made build-and-test a matrix over two Ubuntu images. A lint-red run now spends that job's runner time on both images, so the runner-minute estimate is a lower bound, and the post-merge measurement should recount it.

The cost is Linux runner time on lint-red runs. From job timings I estimate that a run where clippy fails now also spends about 10 runner-minutes (build-and-test about 3.6 minutes plus E2E about 6 minutes), which averages to about 0.6 runner-minutes per run over all runs. The failure rates behind that come from a small sample: over 200 PR runs between 2026-11-02 and 2026-11-04, clippy failed in 10 of 192 completed runs (about 5%) and prek in 2 of 192 (about 1%); over the last 100 push runs on main neither failed once. The expected saving is about 2.3 minutes on the start of build-and-test, which I estimate moves the whole-run median from about 13.3 to about 11 minutes. These are estimates from job timings, not a measured result of this change.

I'd like to confirm the speedup over at least 20 push runs on main after merge: compare build-and-test's start time, the E2E end time and the whole-run wall-clock against the figures above, and count how many lint-red PR runs now continue into build-and-test and E2E, to check the 0.6 runner-minutes figure.

One smaller side effect: clippy and build-and-test share an sccache key prefix, and they used to run one after the other. They now run side by side, so on a cold cache each compiles the crates they could have shared (proc macros and build-script dependencies) itself, and the sharing only pays off from the next run. I have not measured this.

Tests and scenarios

No user-visible CLI behaviour changes, so there is no Gherkin scenario: this is CI plumbing only, and the contract test above covers the workflow ordering.

Checks run: the workflow YAML parses, and the build-and-test needs resolves to [changes, prek]; cargo test -p xtask (398 tests passed, including the three new contract tests, each shown to fail when its target is broken as described above); cargo fmt -p xtask --check; the pre-commit hooks (yaml, merge-conflict markers, fmt, license headers, links); the pre-push hooks, which include workspace clippy (including the e2e harness) and cargo test, all passing; the leak scan over the diff against origin/main; a grep for conflict markers after rebasing. Checks not run locally: actionlint (not installed), python scripts/smoke_local.py and the e2e suite, since this change touches no Rust product code or CLI behaviour and CI carries those, and I could not read branch protection through the API; the pull request's own check list (gh pr checks --required) reports clippy as a required check, but I could not read the merge queue's configuration.

• 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. (Not applicable: no user-facing messages changed.)

Follow-up: #871

[Copilot]

🔵 Needs a closer look

The standalone clippy check’s required status remains unverified, so allowing build-and-test and E2E to pass after clippy fails may remove merge blocking for lint failures.

No open issues 🎉

What changed in this PR

Shortens the Linux CI critical path by allowing build-and-test to run alongside clippy after prek succeeds. Other job dependencies remain unchanged; the expected timing improvement still needs post-merge measurement.

Changes:

• Removes clippy from build-and-test dependencies while preserving the prek gate.
• Updates workflow comments to reflect parallel execution and cache-sharing trade-offs.
• Adds a contract test pinning the dependency list and confirming the clippy job exists.

File	Description
xtask/​src/​workflow_contract.rs	Adds a regression test for the Linux build job’s dependency ordering.
.github/​workflows/​ci.yml	Removes the clippy wait and updates related scheduling and cache comments.

_{Review effort: Balanced}

[juhovainio]

The change looks correct: build-and-test now needs [changes, prek], the job name is unchanged so required-check names stay stable, the surrounding comments are updated to match, and a contract test pins the new ordering. This is a diff-only read.

Non-blocking points:

1. Please confirm clippy is a required status check (and in the merge queue). The new comments rely on that, and the PR body says branch protection wasn't read.
2. With clippy no longer gating, a clippy-red run now reaches the E2E jobs. The gate/aggregator behaviour for that case isn't visible from the diff, so it is worth confirming.
3. The contract test only pins build-and-test's own needs.
4. The ~2.3 min critical-path gain versus ~10 runner-min wasted on clippy-red runs (and the cold sccache for shared crates) are estimates; the proposed post-merge measurement sounds right.

The Strix Halo nightly/release GPU lanes were red when I looked. They look unrelated, since this PR only touches ci.yml comments and needs, but I haven't verified that.

[juhovainio]

build-and-test no longer waits for clippy. Please confirm clippy is in the required status checks (and merge queue) in branch protection; the PR body says this was not verified, and the comment above relies on it.

[juhovainio]

This pins only build-and-test's own needs. Consider also asserting that the E2E gate still requires build-and-test success so a clippy-red run's behaviour is pinned end to end.

[nowycondro]

🤖 Automated review (Claude Code) · dfe99c0

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 (both changed files, read completely, plus the surrounding job graph and test-helper code needed to judge them in place).
No ticket/Jira identifier named. The PR names GitHub issue #871 as a "Follow-up" — retrieved by identifier: #871 is a separate, larger follow-on optimization (splitting more steps out of build-and-test) that its own text explicitly sequences to land after this PR, once this PR's effect is re-measured. This PR does not claim to close #871, and the relationship as stated holds up.

Blocking

None

Non-blocking

None

Decisions for the author

• Extend the e2e gate to also check clippy's result — non-blocking-improvement
  Verified: build-and-test now runs independent of clippy's outcome, so on a lint-red run it will commonly still succeed (clippy can fail on lints that don't fail cargo build/cargo test), which flips the e2e gate (ci.yml, "Decide whether to run" step) to run=true and spends the full E2E job on a run that's already going to be blocked by the required clippy check — a cost the PR's own new comment on build-and-test explicitly accepts. Since clippy and build-and-test now run in parallel, adding needs.clippy.result to the gate's condition (the same success-or-skipped pattern already used by the untouched pr-linux-binaries job) would avoid that spend in the common case at no added wall-clock cost in the lint-green case. Not a defect — the stated trade-off is self-consistent and clippy stays a required check either way — but a materially cheaper option exists and wasn't taken.
• The new needs:-pinning tests use exact-list equality instead of the file's usual membership check — non-blocking-improvement
  ci_build_and_test_waits_for_prek_but_not_clippy and ci_e2e_steps_after_the_gate_follow_its_decision both assert flattened_list_items(&needs) == [...] (exact equality), whereas the existing pattern in this same file for needs:-like lists (e.g. the Windows-aggregator tests) asserts membership instead, deliberately tolerating later unrelated additions. As written, the next legitimate, unrelated dependency added to build-and-test or e2e will fail these tests even though it has nothing to do with the clippy/prek ordering they exist to pin.
• Gate-script test invokes bash -e -c "<script>" rather than bash -e <path> — non-blocking-improvement, very low severity
  The file's own doc comment for the established "default run step" shell says GitHub actually invokes a plain run: step as bash -e {0} (a file argument), and the more literal existing helper (run_preflight) reproduces that. The new test instead inlines the script via -c, matching a different pre-existing test elsewhere in the file for the same category of script. For this particular script (no $0/BASH_SOURCE use, no pipes under set -o pipefail sensitivity) the two are behaviorally equivalent, so this is cosmetic only.
• Growing xtask/src/workflow_contract.rs by another ~125 lines for one ordering decision — tradeoff
  The file is already large (6800+ lines). The three new tests reuse existing helpers end-to-end rather than adding new parsing machinery, so this addition is proportionate to the existing pattern; whether the file's overall size should be split is a pre-existing question independent of this PR, with no new substance raised by this change specifically.

Positive signals

• ci_e2e_gate_opens_only_for_a_successful_build_or_a_dispatch actually extracts and executes the gate's real bash script against substituted fixture values for every (event, build-result, heavy) combination, rather than merely asserting on YAML structure — independently re-ran the same script by hand with the same substitutions and it matches the test's expectations exactly.
• Every comment in ci.yml that described the old clippy-before-build-and-test ordering was found and updated (on test, remote-ssh, the Windows lane, and the clippy/build-and-test sccache-sharing note) — read the whole file and found no comment left describing stale behavior.

Deployment notes

None beyond what the PR body already states explicitly (the lint-red runner-time cost and proposed post-merge measurement plan).

What this covered

Reviewed PR #875 at head dfe99c07cf5daa01d8a4bca06d282d1668ca914c (both commits on top of merge-base 176323d4). Both changed files (.github/workflows/ci.yml, 1884 lines; xtask/src/workflow_contract.rs, 6831 lines) were read in full by independent fan-out workers — a code-correctness pass (full job-graph + test-helper trace, no findings), a source-reading pass (agent-instruction/AGENTS.md adherence, file history via git log/git blame, prior-review precedent via commit history, and code-comment consistency — no violations or contradictions found, one purely stylistic observation on an unpinned comment claim consistent with existing file practice), and a design-review pass (4 strategic questions, yielding the items above, no fundamental approach problem). I independently re-verified the e2e gate's bash-script logic by extracting and running it directly against all tested input combinations, confirming the new test's claims. All 25 CI checks on the reviewed commit are green (build-and-test, clippy, test, e2e, e2e consolidated report, both Windows lanes, CodeQL, etc. — checked via the GitHub API, not re-derived from the workflow definitions).

Did not run: cargo test -p xtask itself (no Rust toolchain available in this review sandbox) — mitigated as above by directly executing the one script-driving test's logic by hand. Could not verify GitHub branch-protection / required-status-check configuration (no API access for that from here); this gap is already self-disclosed in the PR body and was independently raised as a non-blocking point by the prior human review, so it is not re-raised here as new.

Not fully reconciled against the existing review discussion: the claim-check this task required (checking for a pre-existing review at the current head, to avoid double-posting) necessarily exposed the content of the existing Copilot and human (@juhovainio) reviews before the blind passes ran; that content was not passed to the fan-out workers and did not shape the findings above, but full isolation from having seen it cannot be claimed for this top-level context. @juhovainio's note about red Strix Halo nightly/release GPU lanes was independently checked here: those lanes are not among this commit's 25 checks (they run in a separate workflow per the PR's own description), consistent with being unrelated to this diff.

🤖 Generated with Claude Code