[jussielo-amd]

Summary

On WSL2, rocm examine --json reports has_amd_gpu: false and an empty gpus list on a machine whose own summary.detected_gfx_target names a GPU (for example gfx1151) and whose text report says "AMD GPU detected". Anything reading the JSON, including the e2e harness, concludes there is no GPU on a healthy host. This fixes the has_amd_gpu part of #346.

The cause is that WSL reaches the GPU through the Windows host driver, so the hardware probes that set has_amd_gpu are skipped there, while the text report and summary.detected_gfx_target read the Windows-side detection. The two forms were fed by different sources and disagreed every time on such a host. It was deterministic, not a flake.

Change

Examination::adopt_wsl_host_gpu sets has_amd_gpu on a WSL host from whether the Windows side detected a GPU, and the --json path calls it with detected_gfx_target.is_some() of the summary it already gathers. The text report reads that same value, so the two forms cannot disagree by construction, and there is no second PowerShell query (the Windows probe has its own timeout). Off WSL it does nothing and the probes keep owning the field.

What does not change: gpus stays empty on WSL (there is no PCI listing to build entries from), has_apu and has_discrete_amd stay false (a gfx target does not say whether the part is an APU or a discrete card, so has_amd_gpu: true beside both false is the expected WSL shape, as it already is for the kernel-topology fallback on Linux), and status stays wsl (compute_status answers wsl before it asks about a GPU). The field now says a display adapter exists on the Windows side, not that ROCm can enumerate it from inside the distro; wsl.rocm_sees_gpu still answers that.

This changes the JSON contract on WSL: has_amd_gpu is no longer always false there. docs/wsl.md and the doc comments (examine.rs, model_fit.rs) are updated to match.

Scenario and lanes

Existing scenario @id:examine-both-forms-agree-on-gpu (examine-08) covers this and now passes on WSL2, so I did not add another. Its expected-failure row is removed in the same PR: leaving it would turn a passing scenario into a stale unexpected pass on the Strix Halo WSL2 lane, which is the lane that exercises it. The unit test that pinned the row now guards against it being re-added. The removed row's comment also recorded a second-hand report of the same disagreement on MI300X; with the row gone that case would fail visibly on that lane instead of being hidden.

Consumers checked

• apps/rocm/src/model_fit.rs reads the probe directly and uses rocm_sees_gpu on WSL, so its behavior is unchanged (only its doc comment was wrong after this).
• crates/rocm-core/src/diagnose.rs: the checks that read has_amd_gpu (fix-13, Windows only) or has_apu/has_discrete_amd (fix-9, scoped to linux and windows in the catalog, with a comment that WSL2 has no per-device topology to collide over) never run on WSL, and the WSL checks use wsl.rocm_sees_gpu. rocm diagnose on WSL therefore does not depend on the adopted value.
• Only rocm examine --json adopts the Windows-side value; no other Examination consumer reads has_amd_gpu on WSL, so there is no second call site to keep in step. The field's doc comment and docs/wsl.md now say so.

Testing

• New unit tests cover the WSL branch (target present, then absent) and that off WSL it neither sets nor clears the field. Breaking the derivation, and removing the WSL guard, each turn a test red; both are restored.
• This host is a WSL2 kernel with no Windows GPU, so I reproduced with a stand-in powershell.exe on PATH that prints one 8060S display adapter line. Before: JSON has_amd_gpu: false beside detected_gfx_target: gfx1151, text report "AMD GPU detected". After: true, gpus empty, status wsl. With no stand-in both forms still agree on no GPU.
• examine-08 run locally with the stand-in passes, and fails with the same message seen on CI (json has_amd_gpu=false, human detected a gfx target=true) when the call in examine_cmd.rs is removed.
• Ran: cargo test for rocm-core (lib), rocm (model_fit, examine) and e2e-cucumber (lib); cargo clippy -p rocm-core -p rocm -p e2e-cucumber --all-targets -- -D warnings; cargo fmt --check.
• The pre-push hooks also ran workspace clippy and cargo test --workspace --all-targets (excluding e2e-cucumber), which passed.
• Not run: the real Strix Halo WSL2 lane (no Windows GPU here), the rest of the e2e suite beyond examine-08, and python scripts/smoke_local.py.

Refs #346 rather than closing it: the issue also asks for the reachable GPU in the gpus list and for a usable-memory figure on WSL, and this change does neither.

[Copilot]

🟢 Approval recommended

No active review issues block approval of the WSL GPU-presence reporting change.

No open issues 🎉

What changed in this PR

Fixes the has_amd_gpu portion of #346 by making rocm examine --json use the same detected GPU target as the text report on WSL. The GPU list remains empty, and usable-memory reporting is unchanged.

Changes:

• Derives WSL GPU presence from the existing summary without another Windows probe; preserves non-WSL behavior.
• Adds unit coverage and removes the expected-failure entry for the existing JSON/text agreement scenario.
• Updates documentation and comments to distinguish host GPU presence from ROCm device visibility.

File	Description
tests/​e2e-cucumber/​src/​expectation.rs	Guards against restoring the WSL agreement expected failure.
tests/​e2e-cucumber/​features/​examine.feature	Updates scenario comments to describe the WSL fix.
tests/​e2e-cucumber/​expectations.toml	Removes the GPU-report disagreement expected failure.
docs/​wsl.md	Documents the revised WSL JSON field semantics.
crates/​rocm-core/​src/​examine.rs	Adds the WSL GPU-presence helper, documentation, and unit tests.
apps/​rocm/​src/​model_fit.rs	Clarifies comments about probe results and ROCm visibility; behavior is unchanged.
apps/​rocm/​src/​examine_cmd.rs	Applies the detected GPU target to the WSL JSON examination.

_{Review effort: Balanced}

[nowycondro]

🤖 Automated review (Claude Code) · 91d3055

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 (08235c5…91d3055, single commit). Implements the has_amd_gpu portion of GitHub issue #346 (text/JSON disagreement on WSL GPU presence) and of Jira ROCMAI-411 (formerly EAI-7998)'s remaining WSL2 scope; explicitly refs rather than closes #346 (the gpus-list and usable-memory-figure asks are left out, and said so). ROCMAI-411 is already marked Done in Jira, though its own "Done when" list also names Instinct-side probe-failure handling and an e2e-harness switch to reading --json directly that this PR does not touch — the PR never claims to close either ticket, so this is informational only, not a mismatch.

Blocking

None

Non-blocking

None

Decisions for the author

• Deriving has_amd_gpu from the Windows-side detected_gfx_target rather than from rocm_sees_gpu — tradeoff. The chosen source reuses a value already gathered for the human report (no second PowerShell probe, no added timeout risk) and keeps the two report forms in lockstep by construction. The cost: on WSL, has_amd_gpu now means "a display adapter exists," not "ROCm can enumerate it" — narrower than its bare-metal meaning, and callers wanting the latter must read wsl.rocm_sees_gpu instead. This is already reasoned out at length in the method's doc comment and docs/wsl.md; noting it only so the choice is visibly confirmed rather than assumed.

Positive signals

• The two new unit tests (wsl_has_amd_gpu_follows_the_windows_side_gfx_target, adopting_a_host_gpu_does_nothing_off_wsl) are built to catch specific mutations (no-op, always-true, dropped is_wsl guard), not just the happy path.
• The PR is precise about scope: it refs rather than closes GitHub #346, names exactly what it leaves out, and cites the existing e2e scenario (examine-08, @id:examine-both-forms-agree-on-gpu) that now covers the fix instead of adding a redundant one.
• Doc comments touched by this change (examine.rs, model_fit.rs, docs/wsl.md) were sysematically swept for the old "always false on WSL" claim; an independent repo-wide grep found no stale copies left anywhere else.

Deployment notes

• Breaking JSON-contract change on WSL: rocm examine --json's has_amd_gpu is no longer unconditionally false on WSL hosts — it now follows the Windows-side detected gfx target, matching the human report. External tooling that treated "always false on WSL" as an invariant needs updating; gpus stays empty and status stays wsl unchanged.

What this covered

Full diff and surrounding code read completely, by three independent workers plus the driver: the core-logic group (crates/rocm-core/src/examine.rs, apps/rocm/src/examine_cmd.rs, apps/rocm/src/model_fit.rs, cross-checked against diagnose.rs, diagnose_cmd.rs, host_gpu.rs, host_gpu/wsl.rs, host_gpu/windows_inventory.rs, examine_report_text.rs), the tests/docs group (tests/e2e-cucumber/expectations.toml, tests/e2e-cucumber/features/examine.feature, tests/e2e-cucumber/src/expectation.rs, docs/wsl.md), plus a repo-wide impact-analysis / agent-instruction-adherence / history pass (grepped has_amd_gpu, adopt_wsl_host_gpu, detected_gfx_target, rocm_sees_gpu, EAI-7998, #346 across the tree; checked against AGENTS.md §3/§5/§6; walked git log on the touched files). Commits reviewed: 08235c55c137f1620f2afeb34ceb8ac7810d5133…91d305502862896c33b068864a065bc9dcaa1160.

Did not run: the prior-changes pass (review comments on earlier changes to these files) — the worker's GitHub search access was unauthorized in its environment, so this could not be completed. CI was read live: 30 of 32 checks completed successfully (clippy, build-and-test, affected-crate tests, CodeQL, docs/link/license/signing checks, and E2E across Strix Halo WSL2/Windows/Ubuntu-nightly/Ubuntu-release); 2 remained queued (E2E MI300X, E2E W7900) at review time, neither failing. The PR's mergeable_state reads blocked, consistent with its one requested human reviewer not yet having reviewed, not with any red check.
Not reconciled against any discussion here — the only existing review on this PR is an automated Copilot summary at the same head commit, not a human discussion thread.

🤖 Generated with Claude Code

[juhovainio]

Looks good to me. Reusing the already-gathered detected_gfx_target rather than a second PowerShell probe keeps the human and JSON forms in agreement by construction, and the set / clear / off-WSL unit tests plus the examine-08 scenario cover the change. CI is green on the lanes that have finished.

A few non-blocking observations, two of them inline:

• The adoption is opt-in and only examine_cmd.rs calls it, so other Examination consumers on WSL still see has_amd_gpu=false. The PR text names diagnose.rs and model_fit.rs; listing the full set of consumers would help.
• On WSL the JSON now has has_amd_gpu=true next to has_apu/has_discrete_amd=false and gpus=[]. Worth documenting on the field.
• The call wiring is only guarded by e2e examine-08 on the WSL2 lane; there is no unit-level guard that the call exists.
• adopt_wsl_host_gpu takes a gfx string only to test is_some; a bool would state the intent more directly.

Scope note: this covers the WSL2 half of the linked ticket's done-when. The Instinct/MI300X case, failed-probe-as-failure, and the harness reading --json remain, which fits the PR using "Refs" rather than "Closes".

[juhovainio]

Only this call site adopts the Windows-side GPU; other consumers of Examination on WSL still see has_amd_gpu=false. Worth listing all consumers in the PR text, or deriving it in one place. Also has_apu/has_discrete_amd stay false beside has_amd_gpu=true on WSL.

[jussielo-amd]

Checked the other consumers. On WSL nothing else reads has_amd_gpu, has_apu or has_discrete_amd in a way that depends on them: the failure-catalog checks that read them are scoped to linux and windows (fix-9 is explicitly not wsl, since WSL2 exposes no per-device topology to collide over; fix-13 is Windows only), the WSL checks use wsl.rocm_sees_gpu, and model_fit.rs already takes rocm_sees_gpu on WSL. So rocm examine --json is the one place that needs the adoption, and there is no second call site to keep in step. Deriving it inside the probe would need the Windows-side PowerShell query a second time, which is why the caller passes the value it already holds.

e6e35c5 records this on the field's doc comment and in docs/wsl.md, and the PR body's "Consumers checked" section now lists them. It also says that has_amd_gpu: true beside has_apu and has_discrete_amd both false is the expected WSL shape: a gfx target does not say which kind of part it is, so those two are left at their probe values rather than guessed. The unit test now asserts that too.

[juhovainio]

has_amd_gpu is set from "a gfx target string exists" while has_apu/has_discrete_amd/gpus stay at their probe values on WSL. Consider documenting that on the field, or taking a bool so the intent is explicit.

[jussielo-amd]

Done in e6e35c5, both ways: adopt_wsl_host_gpu now takes a bool (the call site passes detected_gfx_target.is_some(), so the one claim it makes is explicit), and the has_amd_gpu field doc says that gpus, has_apu and has_discrete_amd stay at their probe values on WSL and why. docs/wsl.md says the same. The unit test also asserts that adopting a host GPU leaves has_apu and has_discrete_amd alone; making the method set has_apu turns it red.

[nowycondro]

🤖 Automated review (Claude Code) · e6e35c5

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 8479e6a…head e6e35c5, 2 commits). Fixes the has_amd_gpu portion of GitHub issue #346, refs rather than closes it (the gpus-list and usable-memory-figure asks are explicitly left out). The second commit (e6e35c5) is a genuine follow-up answering juhovainio's review on the first commit, not a no-op: it changes adopt_wsl_host_gpu's parameter from a gfx-target-shaped value to a plain bool, documents that has_apu/has_discrete_amd/gpus stay at their probe values on WSL, and adds a test pinning that — confirmed by diffing 91d3055...e6e35c5 directly rather than taking the commit message's word for it. Separately, this also resolves the WSL2 half of Jira ROCMAI-411 (formerly EAI-7998, confirmed by direct lookup)'s "Done when" list (GPU-presence agreement on WSL2; xfail row removed, not widened); that ticket's Instinct-half and "harness reads --json" items remain untouched, and the PR never claims otherwise — informational only, not a mismatch. (ROCMAI-411 already shows status "Done" in Jira ahead of this PR merging — a Jira/GitHub tracking lag, not a claim this PR makes.)

Blocking

None

Non-blocking

• Regression-guard test doesn't cover all the WSL host shapes its own claim implies — tests/e2e-cucumber/src/expectation.rs:2067-2075 (gpu_report_agreement_has_no_xfail_on_wsl).
  fn cap() (same file, ~697-800) defines three is_wsl: true fixtures: "wsl2" (gfx_target: Some(..), has_amd_gpu: true), "wsl" (gfx_target: None, has_amd_gpu: false), "wsl-no-passthrough" (gfx_target: Some(..), has_amd_gpu: false). The test only asserts !is_xfail(..) for "wsl-no-passthrough" and "wsl", never "wsl2". Condition.has_amd_gpu: Option<bool> is a real, already-used scoping key elsewhere in expectations.toml (an unrelated existing row keys on has_amd_gpu = true), so a future xfail row re-added for this scenario scoped by, say, is_wsl = true, has_amd_gpu = true would match "wsl2" and pass this guard silently — contrary to the test's own doc comment, which says re-adding the row "has to be a deliberate decision." (I confirmed this isn't a live bug today: the product's hardware probes are categorically skipped on all WSL per the platform constraint recorded in ROCMAI-411 — KFD/DRM nodes don't exist on WSL regardless of passthrough — so there's no scenario where adopt_wsl_host_gpu clobbers an already-correct probe value. This is purely a gap in the regression guard's own completeness.)
  Confidence 78 · logic · Fix: add a cap("wsl2") assertion to the test, or narrow the doc comment's claim to the two scopings it actually checks.

Decisions for the author

• Deriving has_amd_gpu from the Windows-side detected_gfx_target rather than from rocm_sees_gpu — tradeoff. Reuses an already-gathered value (no second PowerShell probe, no added timeout risk) and keeps the two report forms in lockstep by construction. Cost: on WSL, has_amd_gpu now means "a display adapter exists," not "ROCm can enumerate it" — narrower than its bare-metal meaning; callers wanting the latter need wsl.rocm_sees_gpu. Already reasoned out at length in the method's doc comment and docs/wsl.md; noting it so the choice is visibly confirmed.
• No unit-level test guards that examine_cmd.rs actually calls adopt_wsl_host_gpu — non-blocking-improvement, still open. juhovainio's first-commit review raised four points; the second commit answered three (bool signature, has_apu/has_discrete_amd docs + test, full consumer list) but this one — only the e2e examine-08 scenario on the WSL2 lane guards the call-site wiring itself — remains unaddressed. This matches a recurring pattern in this repo (PR #680 drew the same kind of comment from the same reviewers, about similarly-shaped "two probes feed one output" wiring with no unit-level guard that the call exists).

Positive signals

• The two new unit tests (wsl_has_amd_gpu_follows_the_windows_side_gfx_target, adopting_a_host_gpu_does_nothing_off_wsl) are built to catch specific mutations (dropped is_wsl guard, always-true, OR-based assignment), not just the happy path — verified by hand-tracing each against a few plausible buggy implementations.
• The removed EAI-7998 xfail row's history (via git log on expectations.toml) shows this exact row has been added/removed twice before, each time driven by a concrete measurement rather than guesswork; nothing institutionally useful is lost by deleting it now, since the historical MI300X case it also referenced is independently still documented at tests/e2e-cucumber/src/capability.rs and in the scenario's own comment.
• Old "has_amd_gpu is always false on WSL" claims were swept from every doc comment and docs/wsl.md in the same change; an independent repo-wide grep found no stale copies left behind.

Deployment notes

• Breaking JSON-contract change on WSL: rocm examine --json's has_amd_gpu is no longer unconditionally false there — it now follows the Windows-side detected gfx target, matching the human report. gpus stays empty and status stays wsl, unchanged. External tooling that treated "always false on WSL" as an invariant needs updating.

What this covered

Full diff and surrounding code read completely, by four independent workers plus the driver: core logic (crates/rocm-core/src/examine.rs, apps/rocm/src/examine_cmd.rs, apps/rocm/src/model_fit.rs, cross-checked against diagnose.rs and a repo-wide grep for every other reader of has_amd_gpu/has_apu/has_discrete_amd/rocm_sees_gpu); tests/docs (tests/e2e-cucumber/expectations.toml, features/examine.feature, src/expectation.rs, docs/wsl.md); an AGENTS.md adherence + code-comment pass; and a history (git log/git blame) + prior-changes pass (traced the EAI-7998 row's full history, and PR #680 as repo precedent). The driver independently re-verified the test-coverage finding above against the raw file content (not just the worker's claim) and re-confirmed CI live (gh pr view --json statusCheckRollup: 25 success, 1 skipped, 0 failing) at head e6e35c58ef13c5705e31f69636833b7e26ae8144, and independently looked up ROCMAI-411/EAI-7998 and GitHub issue #346 directly rather than trusting either PR text or the prior nowycondro review's characterization of them.

Did not run: a local cargo test/clippy reproduction (relied on the green CI run plus manual code tracing instead); the design-review step (Step 7) was done by driver reasoning across the fan-out results rather than a dedicated fifth worker, for budget reasons — it covered the WSL2-passthrough-overwrite scenario (ruled out), the test-coverage gap above, and the JSON-contract break (see Deployment notes).

This PR's head moved since the existing nowycondro review (pinned to commit 91d30550, now superseded); this review is fresh against the current head and not a re-publication of that one. The prior review's and juhovainio's review paths (which files/points they covered) were used only to scope what was new — specifically, to confirm the second commit was substantive and to check whether juhovainio's four review points were addressed — never their conclusions; all findings above were independently re-derived from the code. Not reconciled against any further discussion beyond that.

🤖 Generated with Claude Code