[jussielo-amd]

Summary

The Windows build-and-test job is the longest pole of every CI run: about 16 minutes on main and 20+ on pull requests from forks, which get no shared compiler cache. It also starts about 3 minutes late because it waits for the Linux clippy and prek jobs. This splits it into two parallel jobs and removes that wait. By step timings of recent runs, main should drop from about 16 minutes to about 10 (an estimate, not yet measured).

What changed

• windows-debug: Windows clippy, dev build, workspace tests, local smoke, and the harness self-tests.
• windows-release: release build, install lifecycle e2e, release-readiness self-test, installer syntax check, pinned-key check, and PR bundle packaging. None of these consume the debug side's output.
• windows-build-and-test keeps its name as the required check and becomes an aggregator that needs changes, windows-debug and windows-release, and requires each to have finished exactly success. A skip is accepted only for a manual dispatch that did not ask for binaries.
• Whether the Windows jobs run is decided once, as a new changes.windows output. Both jobs gate on it and the aggregator derives its expected skip from it, so the two cannot disagree.
• Neither Windows job waits for clippy or prek any more. Both remain required checks of their own, so a lint failure still blocks the merge.
• Every step of the old job is carried over unchanged into exactly one new job (sccache stats runs in both). I checked this by diffing the parsed steps before and after.
• Comments and docs that described the old single job are updated.

Why the gate is not weakened

A skipped required check reads as passing, so the aggregator compares each result for exact equality instead of testing for failure: failure, cancelled and an unexpected skipped all fail it, and a failed changes fails it even when the work jobs are meant to be skipped. The new contract tests in xtask/src/workflow_contract.rs pin this: the aggregator must await every job in the Windows lane (found by runner, with an explicit allowlist for powershell-lint), its script is executed over every combination of job results, the work jobs' needs, runner and if: are pinned exactly with no job-level continue-on-error, the two jobs' env and setup steps cannot drift apart, and each gating step is pinned by name with its command and exact if:. I applied 41 deliberate breakages to ci.yml (dropped steps, continue-on-error, if: false, wrong needs, fork-skipping conditions, a Windows job the aggregator does not await, and so on) and each one fails a Windows contract test.

Trade-offs

• Dropping the clippy/prek wait buys about 2.5 minutes of wall clock on every run, paid for with Windows runner time on pushes that turn out lint-red. A middle option is needs: [changes, prek], which costs about 20 seconds; happy to switch if preferred.
• The rust-cache key includes the job id, so both new jobs start cold on their first runs, and the old windows-build-and-test cache entry is orphaned until it ages out. The sccache prefix is unchanged, so the compiler cache stays warm.
• cargo xtask e2e builds the e2e harness in the dev profile, which now happens in windows-release instead of being warmed by the debug build. sccache should absorb this after the first run.
• Fork pull requests still get no shared compiler cache; that is a separate change.

Verification

• cargo test -p xtask (346 tests), cargo fmt --check and cargo clippy -p xtask --all-targets -- -D warnings pass locally, and the leak scan and conflict-marker grep are clean.
• The workspace tests were not run locally for this push (the local pre-push hook was skipped); the diff touches only ci.yml, a few comments and docs, and xtask test code, and CI runs the workspace.
• The new jobs and the aggregator have never run on GitHub; this pull request's own run is the first real check of the wiring. actionlint was not available locally.

This is CI plumbing and changes no CLI behaviour, so there is no Gherkin scenario.

[Copilot]

🔵 Needs a closer look

The rewired merge-required Windows CI gate lacks a live GitHub run, so a maintainer must confirm debug/release execution, aggregator results, and manual-dispatch skip behavior.

No open issues 🎉

What changed in this PR

This PR aims to shorten CI runs by splitting Windows debug and release work into parallel jobs, while preserving windows-build-and-test as the required check.

Changes:

• Separates debug validation from release builds, lifecycle checks, and packaging; removes both jobs’ wait for Linux lint checks.
• Adds a shared Windows execution condition and an aggregator that checks dependency results.
• Adds workflow contract tests and updates documentation and comments to reflect the split.

File	Description
xtask/​src/​workflow_contract.rs	Adds contracts for Windows job gating, aggregation, setup consistency, and retained validation steps.
xtask/​src/​env_mutation_contract.rs	Updates comments identifying the job that runs Windows workspace tests.
tests/​e2e-cucumber/​features/​comfyui.feature	Updates comments identifying the Windows lifecycle job.
docs/​testing.md	Clarifies which Windows job runs workspace tests and lifecycle scenarios.
.github/​workflows/​e2e-selfhosted.yml	Updates comments distinguishing hosted Windows jobs from GPU coverage.
.github/​workflows/​ci.yml	Splits Windows work into parallel jobs and adds the required-check aggregator.

_{Review effort: Balanced}

[juhovainio]

I reviewed this PR and found nothing blocking. Two small notes are left inline.

What I checked:

• The aggregator job windows-build-and-test keeps its name, so the required-check match still holds. It needs changes, windows-debug and windows-release, uses !cancelled(), and requires an exact result from each.
• Every old step now lives in exactly one of the new jobs, and the release job keeps its AWS_LC_SYS_USE_SYSTEM env.
• Action pins are unchanged, no stale references to the old single job remain in docs, features or xtask, and I saw no leaks.

Notes:

• On this PR's cold-cache run windows-release took 20m31s against 14m46s for windows-debug, so the lanes are unbalanced and the gain is still unmeasured.
• The two new work jobs are not required checks themselves, so the aggregator needs to stay in the branch-protection ruleset.
• Dropping the wait on clippy/prek is a documented, deliberate tradeoff.

Not verified: the "41 mutation checks" claim, and I did not run the xtask tests. Two Strix Halo Ubuntu lanes (nightly, release) failed at 45m25s on files this PR does not touch, likely runner infra, but I did not confirm the cause. This reflects a diff read, not a full CI result.

[juhovainio]

On this PR's run windows-release took 20m31s vs windows-debug 14m46s, so the two are not about the same length yet (cold caches on a PR). Worth re-measuring on a main push and rebalancing if the release side stays the long pole.

[juhovainio]

Nit: the rewrap leaves "folds in the hermetic" as an orphan line; reflow the paragraph.

[nowycondro]

🤖 Automated review (Claude Code) · ce854ce

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 (base df1548a2…head ce854ce9). Purely CI plumbing: splits the Windows GitHub Actions lane into two parallel jobs behind a pinned aggregator; no ticket named in the description, and none appears needed for a CI-only change.

Blocking

None

Non-blocking

• ci_jobs()'s comment-attachment differs from the pre-existing job_block()'s, which is a latent fragility in the new contract-test helpers — xtask/src/workflow_contract.rs:4252-4277 (new ci_jobs/ci_job) vs xtask/src/workflow_contract.rs:303-320 (pre-existing job_block).
  job_block stops a job's extracted text at the first indent-2 line, comment or not, so a trailing comment before the next job is excluded from both jobs. ci_jobs only starts a new job at an indent-2, non-comment line, so a trailing comment block that precedes a job header (e.g. the rationale comment right before windows-build-and-test:, which itself contains the literal strings !cancelled() and always()) is folded into the previous job's text instead of being excluded or attached to the job it actually describes. ci_windows_aggregator_requires_every_windows_job then does code.contains("!cancelled()") && !code.contains("always()") over ci_job(&ci, "windows-build-and-test"); today this still evaluates correctly only because the job's own if: line repeats !cancelled() and the misattributed comment's always() mention lands in windows-release's captured block instead, outside what the assertion checks. A later edit to that comment, or a new trailing comment before any job this helper is also used on, could make the assertion pass or fail for the wrong reason without anyone noticing, which runs against the rest of this test file's evident care about not trusting a check for the wrong reason.
  Confidence 70 · architectural · Fix: make ci_jobs's job-boundary rule match job_block's (stop at any indent-2 line), or have it drop a job's leading comment lines instead of attaching them to whichever job happened to be parsed immediately before.

Decisions for the author

• Dropping windows-debug/windows-release's needs: [changes, clippy, prek] relies on clippy and prek already being independently-required branch-protection checks — tradeoff. ci.yml's own comment states "Those stay required checks in their own right, so a lint failure still blocks the merge," which is a claim about branch-protection configuration that isn't visible from the files in this repo. It is very likely already true independent of this change — dozens of other jobs in the same file (e.g. test, build-and-test) still need: [changes, clippy, prek] purely as a compute-saving gate, a pattern that only makes sense if clippy/prek are already required on their own — and the pre-split job's if: treated a clippy/prek skip the same as success, so this PR does not newly introduce reliance on "skipped reads as passing" for the Windows lane; if anything it now produces a real success/failure for windows-debug/windows-release instead of skipping them outright when lint is red. Worth a quick confirmation against the actual ruleset before merge, since nothing in-repo proves it.
• Running two parallel windows-latest jobs instead of one serial job duplicates per-job fixed overhead (checkout, sccache install, the cache-backend probe, toolchain setup) that was previously paid once — tradeoff. The change's own comments already weigh the clippy/prek-wait removal's cost in wall-clock terms, but not this one in Windows-runner-minutes: total billed Windows compute time may rise somewhat even as end-to-end latency falls. Worth confirming that's an accepted cost, alongside the already-documented cold-cache effect on both jobs' first runs.

Positive signals

• ci_windows_aggregator_passes_only_when_every_job_succeeded directly executes the aggregator's extracted bash script under bash -e -c against every result/skip combination (success/failure/cancelled/skipped × expected-skip true/false) rather than only asserting on source text — real behavioral verification of the merge gate, independently re-run and confirmed correct for this review.
• Every step from the pre-split monolithic job lands in exactly one of windows-debug/windows-release, confirmed by diffing the old single job against the new pair step-by-step; nothing was silently dropped or duplicated.
• The four supporting files (docs/testing.md, tests/e2e-cucumber/features/comfyui.feature, xtask/src/env_mutation_contract.rs, .github/workflows/e2e-selfhosted.yml) were each updated to name the correct new job for the behavior they describe — including the non-obvious case that the install-lifecycle/E2E_ALSO_RUN_IDS work moved to windows-release while the single-process cargo test --workspace moved to windows-debug, which would have been an easy pair to swap by mistake.
• ci_windows_jobs_share_one_cache_and_toolchain_setup guards the two duplicated jobs' env and setup steps against silent drift from each other, and ci_windows_jobs_keep_every_gating_step_enabled pins every gating step's exact command, if:, and absence of continue-on-error — both checked by direct reading against the literal YAML, including the trickier >- block-scalar steps.
• All uses: lines are pinned to full commit SHAs with trailing version comments, consistent with this repo's own convention; single, clearly-written, DCO-signed commit whose message matches the diff.

Deployment notes

• At head commit ce854ce9, two self-hosted GPU jobs in the separate "E2E self-hosted" workflow — "E2E tests (Strix Halo, Ubuntu, nightly, current)" and "...release, current)" — show cancelled, and two more ("E2E tests (MI300X, current)", "E2E tests (W7900, current)") remain queued. This PR touches that workflow with only a verified-accurate comment edit, the job logs show an artifact upload immediately followed by orphan-process termination (consistent with a runner-side cancellation rather than a failing assertion), and this repo's own AGENTS.md documents that self-hosted GPU hardware availability can differ from a clean run. All checks this change can actually affect — the windows-build-and-test aggregator, windows-debug, windows-release, clippy, prek, and the affected-crates test job that compiles/runs the new contract tests — are green. Recommend confirming the self-hosted lanes are unrelated infrastructure flakiness before treating the run as fully clean.

What this covered

Every one of the 6 changed files read completely at the PR's head content (fetched via git show refs/pr810head:<path>, never checked out), plus unchanged repository context (module wiring in xtask/src/main.rs, release.yml/nightly.yml, README.md, other docs) for impact analysis — via two independent blind fan-out workers (one on ci.yml + xtask/src/workflow_contract.rs, one on the four smaller doc/comment files), each scoped to files only, with no access to this PR's description or comments, plus my own consolidation, impact-analysis and design-review pass. Range reviewed: df1548a211ed5a53d37d2e5df8f83a7746a13239…ce854ce901021150f06ba765d8286e0e197a5e9e.

Procedural note: as the orchestrating context I necessarily read this PR's title/body once during the mandatory pre-review duplicate-review check (the API call that returns the head SHA also returns the body); neither fan-out worker saw it, and its content was not used for judgment until after the findings above were settled.

Live CI was read at the exact head commit (gh pr checks): all Windows- and lint-relevant jobs are SUCCESS, as detailed under Deployment notes. "Prior changes" — no earlier review threads exist on this PR to reconcile against (confirmed zero review-comment threads). "History" — compared the pre-split job definition on origin/main against the new pair directly via git show/git diff. Could not execute cargo test -p xtask directly (no Rust toolchain available in this environment); relied on the live CI's own passing "Test (affected crates)" job plus direct re-execution of the aggregator's bash script and manual tracing of the YAML-parsing helpers against the literal file content. Not reconciled against any discussion on this PR.