[rominf]

What was wrong

The clippy job runs on Linux only. It therefore compiles the cfg(unix) side of every platform conditional and never sees the cfg(windows) side.

That is not "Windows code is under-linted" — it is outside the warnings-as-errors gate entirely. A lint could land in Windows-gated code and no check in the set could fail, because no check ever compiled it. windows-build-and-test builds and tests on Windows, so type errors are caught there; lints are not.

What this does

Adds both of the Linux job's clippy invocations to windows-build-and-test, before the build so a lint failure costs seconds rather than the full build/test/package cycle — the same reasoning behind that job's existing needs: clippy.

It goes in the existing job rather than a new one on purpose: windows-build-and-test is already a required status check, so the gate becomes load-bearing immediately with no branch-protection change. With the clippy steps added the job took 18 minutes on this PR's last run (clippy 5m23s, build 7m29s, test 1m43s), against a 45-minute timeout, so there is headroom. Clippy and build compile separately; clippy's output is metadata only, so the build cannot reuse it.

The 21 violations it surfaced

Most are mechanical and are fixed here:

• &mut x passed to a Win32 call → &raw mut x (also avoids materialising a reference just to take a pointer from it)
• if c { 1 } else { 0 } → i32::from(c)
• map(..).unwrap_or(..) → is_ok_and(..), map(..).unwrap_or_else(..) → map_or_else(..)
• use_self, uninlined_format_args

The rest are deliberately silenced rather than "fixed", and that is the part worth reviewing.

Sixteen are missing_const_for_fn (13) or unused_async (3) on platform stubs. Applying Clippy's suggestion there is wrong in two ways:

1. Making only the off-unix arm of a cfg-split pair const gives the two platforms different signatures. Two of these are pub, so a const-context call would compile on Windows and fail on Linux.
2. It cascades. Making process_start_ticks const pushed the same lint onto ProcessIdentity::capture; making probe_usable_amd_gpu_indices const pushed it onto usable_amd_gpu_indices. This is why the burn-down grew on each pass rather than shrinking.

The async stubs are the same shape: callers .await them on both platforms, so dropping async would not compile. Each site carries its reason inline. The one case where const is applied is WindowsExamineInventory::is_empty, which is cfg(windows)-only with no sibling to disagree with.

Verification, and its limit

Cross-compiled the whole workspace to x86_64-pc-windows-gnu with mingw. Both clippy invocations are clean for that target, both are clean for Linux (no regression), and cargo test --workspace --all-targets passes.

The msvc target could not be checked locally: ring and aws-lc-sys need a Windows C toolchain to build their build scripts, which does not exist off Windows. The cfg(windows) code is identical between the two targets, but an msvc-only lint would appear for the first time in this PR's own CI run — which is, conveniently, exactly the gate being added. If one shows up, it is a real finding, not a flake.

Clippy also stops at the first failing crate, so this converged over several passes (e2e-cucumber → rocm-core → rocmd → apps/rocm). The final pass reports zero.

Scope

CI config plus the lint fixes it requires — no behaviour change. No Gherkin scenario: nothing user-observable changes. The &raw mut rewrites are the only edits inside unsafe FFI blocks, and they are the compiler-suggested form for taking a raw pointer without an intervening reference.

[jussielo-amd]

Code review

Blocking

1. .github/workflows/ci.yml:590 — The new "Clippy (Windows targets)" step runs two cargo clippy commands in one pwsh script with no error handling. pwsh defaults to $ErrorActionPreference: Continue, and GitHub Actions reads the step's pass/fail from $LASTEXITCODE at the end of the script — which only reflects the last command. So if the first (broader) clippy invocation fails but the second (narrower, e2e-cucumber-only) one passes, the whole step reports green. The entire point of this PR is to lint-gate cfg(windows) code, and this bug means it can silently fail to do that. The same file's "Probe sccache cache backend" step already handles this correctly with explicit $LASTEXITCODE checks, so this isn't a tooling limitation — just a gap in the new step.

Worth raising

2. ci.yml:590 — Running cargo clippy --workspace --all-targets immediately before cargo build --workspace --all-targets in the same job causes Cargo to recompile the workspace twice (clippy's RUSTC_WORKSPACE_WRAPPER changes the build fingerprint, so build doesn't reuse clippy's artifacts). Risks eating into the job's ~13min/45min timeout headroom the PR itself cites.
3. ci.yml:590 — This new lint step runs serially inside windows-build-and-test (the repo's slowest job), rather than following the existing convention (see powershell-lint) of a separate parallel job — so it adds to critical-path time instead of overlapping with the Linux clippy job, and a Windows-lint failure surfaces buried inside a build/test job rather than its own check.
4. Cargo.toml:74 — The workspace already has a [workspace.lints.clippy] allow-list mechanism for exactly this kind of structural false-positive (nursery lints on cfg-split platform stubs), but the PR hand-applies #[allow(...)] with duplicated justification comments to 9+ individual functions instead of one workspace-level policy change. Future platform stubs will keep re-triggering this and require copy-pasting the same idiom.

Minor

5. crates/rocm-core/src/uv.rs:283 — The justification comment is copy-pasted near-verbatim across 9 files; one of them (make_executable) already describes a structurally different situation using near-identical wording, so drift has already started.
6. ci.yml:593 — The two clippy invocations run sequentially with no data dependency forcing that.
7. apps/rocm/src/main.rs:3492 — #[allow(clippy::missing_const_for_fn)] is applied unconditionally to a function that isn't actually cfg-split (just has one non-const branch), unlike every other suppression in this PR — if that branch ever becomes const-compatible, the suppression would stay in place with nothing forcing re-evaluation.

[rominf]

Thanks for the thorough review. Rebased onto 4ff6b821 and addressed below (head 06d809ff):

1. Two clippy commands in one pwsh step mask a failure — fixed in ce00f8e9, one step per invocation, the way the Linux clippy job does it. Reproduced before fixing: under pwsh 7.4, two commands where the first exits 3 leave the step at 0, while the failing command on its own exits 3. The Build step had the same two-command shape, so it's split the same way.
2. Clippy then build compiles twice — confirmed, and the description's "~13 minutes" was stale. The last run took 18 min (clippy 5m23s, build 7m29s, test 1m43s) against the 45-min timeout. The second compile can't be avoided by ordering: clippy only produces metadata, so the build can't reuse it. The description now has the measured numbers.
3. Separate parallel job — kept in windows-build-and-test on purpose. That job is already a required check, so the gate blocks merges from day one. A new job would need an admin to add its context to branch protection before a red run blocked anything. Happy to move it once someone can update protection in the same change.
4. [workspace.lints.clippy] instead of per-function allows — not done. Cargo's lints table can't be scoped by cfg, so allowing missing_const_for_fn workspace-wide would switch it off on Linux too, where it guards real const fns. A crate-root #![cfg_attr(not(unix), allow(clippy::missing_const_for_fn))] is possible, but it would also silence the lint on Windows-only code that should be const. Per-stub allows keep it scoped to exactly the cases that are wrong.
5. Repeated justification comment — the "Platform stub…" text is the same everywhere on purpose: it's the same reason each time, and identical wording keeps them greppable. The cited uv.rs:283 is a different function (make_executable, now around uv.rs:561), and its comment is distinct and accurate. No change.
6. Sequential clippy invocations — kept sequential. They share the target directory and the cargo lock, so running them in parallel would contend rather than save time, and the second takes about 44 s.
7. Unconditional allow on PrivilegeEscalation::detect — fixed in 06d809ff at the root. The off-Linux running_as_root() stub was a pub const fn, exactly the const-on-one-platform pattern this PR argues against, and that's what pushed the lint onto detect. The stub is now a plain fn with the same allow and reason as the other stubs, and the allow on detect is gone. Checked in a scratch crate under the workspace's nursery level for the Windows target: without the allow the stub fails clippy, with it clippy passes.

Also corrected in the description: the allow count is 16 (13 missing_const_for_fn, 3 unused_async), not eleven. Verified locally: cargo test -p xtask (255), and cargo clippy --workspace --all-targets -- -D warnings on Linux. I couldn't run a full Windows-target clippy here (ring needs a mingw C toolchain that isn't available), so this run's Windows clippy steps are the first full check against the rebased code.

Comments addresses.

[jussielo-amd]

Follow-up on the blocking finding and the two I'd flagged as worth a closer look.

Blocking — step masks a clippy failure (ci.yml): fixed in ce00f8e9, confirmed by reading the diff and the CI run. Each cargo clippy/cargo build invocation now has its own step, so pwsh's last-exit-code behavior can't hide a failing first command behind a passing second one anymore. The Windows job's actual run (37275489997/windows-build-and-test) shows all four steps executing and reporting independently, which is also the first clean full Windows-target clippy run against this code — nice to have that confirmation land in the same PR that adds the gate.

PrivilegeEscalation::detect's unconditional allow: also fixed, and fixed at the right layer — 06d809ff turns the off-Linux running_as_root stub from pub const fn back into a plain fn (matching the non-const Linux signature) instead of leaving detect with a blanket suppression. Checked the callers; none of them need running_as_root in a const context, so this doesn't cost anything.

On the rest — the double-compile, the in-job vs. separate-job placement, the per-function allows vs. workspace lint table, and sequential clippy invocations — I buy the reasoning in the write-up (clippy's wrapper really does invalidate the build's fingerprint, the lints table can't be scoped by cfg, and the two invocations do share the target dir/lock). None of those were blocking to begin with, so I'm fine leaving them as documented tradeoffs rather than holding up the PR over them.

Spot-checked the uv.rs comment claim too — make_executable is a single function with internal #[cfg] blocks, not a cfg-split pair, so its comment being worded differently from the "Platform stub" ones elsewhere is correct, not drift.

CI is green except E2E tests (MI350P), which timed out waiting 24h for a self-hosted runner — infra, not this change.

Good to go.

[rominf]

This was ejected from the merge queue by the gate it adds. 5ed93653 ("Tackle out of memory errors") landed on main with an off-Linux stub, fn gpu_vram_usage_sysfs() -> Option<Vec<GpuVramUsage>> { None }, which Windows clippy flags as missing_const_for_fn. Until this PR nothing could see that, because clippy ran only on Linux.

I rebased onto 4ee61616 (no conflicts) and added 82fa27b8. The stub stays a plain fn, with the same #[allow(clippy::missing_const_for_fn)] and "Platform stub" reason as the other stubs, so its signature matches the non-const Linux sibling.

A full Windows-target clippy cross-check from Linux is clean on the rebased head, so nothing else on main trips the gate. It used x86_64-pc-windows-gnu, ran both CI invocations, and touched the sources first so the cache could not hide anything. Linux fmt, clippy and the xtask/e2e-cucumber tests also pass. Your approval was on 06d809ff, so this needs another look.

[rominf]

Rebased onto b863f77a. The PR was dirty because #528 (830f3799) moved host/GPU detection out of crates/rocm-core/src/lib.rs into the new crates/rocm-core/src/host_gpu.rs, and four of this PR's fixes lived inside the moved region.

What the rebase changed:

• crates/rocm-core/src/lib.rs — resolved the delete/modify conflict in favour of main's extraction. The Win32 FFI fixes that stayed behind (&raw mut / &raw const in spawn_hidden_console_with_log, wait_for_process_exit, process_is_running, spawn_windows_no_inherit; i32::from; the detach_command_session stub allow) are unchanged.
• crates/rocm-core/src/host_gpu.rs — re-applied the four fixes at their new home: the missing_const_for_fn allow on the #[cfg(not(target_os = "linux"))] probe_usable_amd_gpu_indices stub, const fn is_empty (Windows-only, no sibling, so const is right here), merge_missing_from(… other: Self), and map_or_else in append_windows_probe_diagnostics.
• No other file's resolution changed. ROCMAI-83: common.rs follow-up, webhook.rs, cli.rs and sandbox.rs extraction from apps/rocmd/src/lib.rs #483's apps/rocmd extraction did not move the two stubs this PR annotates, so apps/rocmd/src/lib.rs merged cleanly.

Every one of the 12 #[allow(clippy::missing_const_for_fn)] sites this PR adds was re-checked to sit directly on its intended #[cfg(...)] stub after the move.

Re-ran the Windows cross-check from Linux on x86_64-pc-windows-gnu after the rebase, with all sources touched first: both invocations this PR adds are clean (exit 0, zero diagnostics), as is RUSTFLAGS="-D warnings" cargo check --all-targets for that target. To confirm the gate is not vacuous, a deliberate lint injected into a #[cfg(windows)] function fails it (exit 101, length comparison to zero).

On whether the new gate would be red on code this PR does not otherwise touch: measured rather than assumed. Restoring the pinned-main content of every file this PR edits and re-running the gate gives exit 101 with 21 diagnostics — 9 missing_const_for_fn, 6 implicit-raw-borrow, 2 unused_async, plus uninlined_format_args, map().unwrap_or_else(), a struct-name repetition and a bool-to-int if — across rocm-core, rocm-dash-daemon, rocm-dash-tui and e2e-cucumber, and more that --keep-going could not reach once rocm-core failed. Those are fixed in this PR rather than excluded, since narrowing the gate would reinstate exactly the blind spot it exists to close.

Caveat for reviewers: the local cross-check is the GNU target and CI is MSVC, so target-env-conditional differences would only show on the windows-build-and-test lane.

Local gates: fmt; workspace clippy and the e2e-target clippy, both -D warnings, clean; xtask 271; e2e-cucumber --lib 136; feature_naming 4; cargo check --test e2e; rocm-core --lib 582; rocm --bin rocm 1013.

One unrelated flake seen locally: comfyui::tests::status_reports_stopped_when_saved_comfyui_pid_is_gone failed once under load and passed on a clean re-run. It saves pid: 0 against an unused_local_port(), so another process can bind that port between allocation and assertion. This branch does not touch comfyui.rs.

[rominf]

Rebased onto 28f9ff7d (now 11069ab5). Since the last rebase, #487 moved descendant_pids_for_roots and force_terminate_remaining_processes, and their cfg(not(unix)) stubs, out of apps/rocmd/src/lib.rs into the new apps/rocmd/src/service.rs. The #[allow(clippy::missing_const_for_fn)] and "Platform stub" annotations on those two stubs now live in service.rs. git range-diff shows only that relocation; the other three commits are identical.

The Windows-target gate passes locally against the new base on x86_64-pc-windows-gnu, with all sources touched first (exit 0, zero diagnostics):

• cargo clippy --workspace --all-targets -- -D warnings
• cargo clippy -p e2e-cucumber --test e2e -- -D warnings
• RUSTFLAGS="-D warnings" cargo check --workspace --all-targets

Main's new code from #583 (uv.rs, engines/vllm/src/install.rs) and the cfg-gated code #487 moved produce no new findings. A deliberate #[cfg(windows)] lint still fails the gate, so it is live.

Heads-up for merge order: #499 deletes these same two helpers, so whichever of the two lands second will conflict at their stubs in service.rs. The resolution is to keep the deletion.

Stale: approved 06d809f; head is now 11069ab, so a fresh look is needed on the current head. Since the approval: rebased twice onto main. The Windows clippy gate ejected this from the merge queue on a new off-Linux stub main had landed, gpu_vram_usage_sysfs, fixed in 11069ab. #528 and #487 moved the stubs this PR annotates into host_gpu.rs and service.rs, where the allow-attributes now sit. The other commits (cfecb60, ddd0d74, 77d7498) are the reviewed ones, rebased. The Windows-target cross-check is clean.

[jussielo-amd]

Reviewed PR #488 (fetched head to a local branch, diffed against origin/main) with a full code-review pass at high effort. The change is CI-only: it adds clippy to the windows-build-and-test job (the same two invocations the Linux clippy job already runs), splits the Build step into Build and Build (release) so a single run: block no longer hides a failing first command behind a passing second one, and fixes the 21 lints that surfaced. Most of those fixes are #[allow(clippy::missing_const_for_fn)] or #[allow(clippy::unused_async)] on platform-stub functions, each with an inline comment explaining why applying the suggestion would either give the two cfg(unix)/cfg(not(unix)) arms different signatures or break a caller that awaits the stub on both platforms; the rest are mechanical (if/else to i32::from, map(..).unwrap_or_else(..) to map_or_else(..), map(..).unwrap_or(false) to is_ok_and(..), and &mut to &raw mut/&raw const for the Win32 FFI calls in crates/rocm-core/src/lib.rs around lines 1267, 1283, 1333, 1516, and 1659-1660). I checked each suppression's stated rationale against its sibling cfg arm and they hold up, and the &raw mut/&raw const rewrites are semantically equivalent to the references they replace for those FFI calls. No user-observable behavior changes here, so no Gherkin/BDD scenario is needed, and the PR description says so explicitly.

Two non-blocking notes for a possible follow-up:

1. .github/workflows/ci.yml:669-675 ("Acceptance harness self-tests" in windows-build-and-test) still runs three python --self-test invocations inside one shell: pwsh run: block, which is the exact failure-masking pattern this PR's own description calls out and fixes for the Clippy and Build steps just above it (pwsh continues past a failing native command, and the step's reported result is only the last command's exit code). It's pre-existing and outside this PR's stated scope, not something this diff introduces, but since the PR already established the one-command-per-step fix, it'd be worth applying it here too in a follow-up.
2. The "Platform stub..." justification comment plus #[allow(clippy::missing_const_for_fn)] is duplicated near-verbatim across roughly a dozen stub functions across apps/rocm, apps/rocmd, crates/rocm-core, and tests/e2e-cucumber (e.g. crates/rocm-core/src/openmpi.rs:184, crates/rocm-core/src/proc_lifecycle.rs:311 and :336, apps/rocmd/src/service.rs:104 and :254). That's a minor future maintenance cost if the underlying rationale ever needs correcting, but inline reasoning at each call site is a defensible tradeoff given Rust has no clean way to share an #[allow] across a cfg-split signature pair.

Approving: the lint fixes are correct and each is matched to the actual constraint on its platform stub, the CI change achieves its stated goal of gating cfg(windows) code behind clippy, and neither note above rises to a correctness or scope problem with this PR.

[rominf]

On the approval's two non-blocking notes:

1. The Windows "Acceptance harness self-tests" step: ci: run each Windows acceptance self-test in its own step #633 gives each of the three self-tests its own step. I reproduced the masking locally with GitHub's pwsh wrapper: a failing command followed by a passing one exits 0. The Linux copy of the step runs under bash -e, which already stops at the first failure, so it is unchanged.
2. The repeated missing_const_for_fn justification: left as is. Each #[allow] sits on a cfg-split stub whose sibling arm is what makes the suggestion wrong, and Rust has no way to attach one #[allow] to both arms of a cfg pair. A comment at each site keeps the reason next to the code it excuses. If the rationale ever changes, git grep "Platform stub" finds every copy.