[tomastola]

Fixes #448

Problem

rocm examine --json reported "amdkfd_loaded": false on machines where kfd works. The field was true only when a module named amdkfd appeared in lsmod or /proc/modules. kfd is not a module of its own: HSA_AMD is a bool Kconfig option and amdkfd is linked into amdgpu, so that name never shows up, whether amdgpu is built in or loaded as a module. A healthy system was reported as "kfd missing".

amdgpu_loaded had the same blind spot for a different reason: an amdgpu that is built into the kernel is absent from lsmod, so the field was false. rocm diagnose reads that as evidence of a DKMS failure (+30), "amdgpu module is not loaded" (+35) and a Secure Boot problem (+10), none of which is true on such a machine.

Change

This PR fixes both flags in probe_modules.

amdkfd_loaded is now true when any of these holds:

• amdkfd is in the module list (unchanged behaviour);
• /sys/class/kfd/kfd exists;
• /dev/kfd exists.

amdgpu_loaded is now true when any of these holds:

• amdgpu is in the module list (unchanged behaviour);
• /sys/bus/pci/drivers/amdgpu exists. The kernel creates it once amdgpu_init has registered the driver, for a loaded module and a built-in driver alike. /sys/module/amdgpu is not used: a built-in driver gets it at an early initcall even when amdgpu_init then bails out (nomodeset, amdgpu.modeset=0);
• kfd is present (/sys/class/kfd/kfd or /dev/kfd). kfd is linked into amdgpu, so kfd cannot be there without it, and a container that passes /dev/kfd through without /sys/bus/pci must not report amdkfd up and amdgpu down.

The module-list check and the host probes share one function, module_or_host_state, and the host probe only runs when the module list has not already answered. driver_states pairs the two fields' checks, so which host signal answers which field is testable apart from the real module list.

All paths are read through host_path, so the probes can be tested against a planted root; they are listed with the other simulated-host-root reads in hardware_root.rs and docs/release-trust.md, and the CI comment on planted-root coverage names their tests. If the module list cannot be read at all but the host shows the driver, the field is true rather than unknown; with no module list and nothing on the host it stays unknown. Both fields keep their names and types, so the JSON schema is unchanged. Each field has a doc comment saying what it means now.

Naming. The name amdkfd_loaded is kept for schema compatibility. The old module check still covers kernels from before kfd was merged into amdgpu. A separate field such as kfd_available was considered and rejected.

Tests

• module_or_host_state: module listed; built in with an empty or unrelated module list; neither present; module list unreadable with and without the driver on the host.
• kfd_present_on_host and amdgpu_present_on_host against a planted host root (e2e-test-hooks): sysfs, /dev/kfd, neither; for amdgpu, /sys/module/amdgpu alone does not count.
• amdgpu_implied_by_host with only /sys/class/kfd/kfd, then only /dev/kfd: true. Dropping the kfd disjunct fails it.
• driver_states against a planted host root, with an empty module list and with none, planting one signal at a time: kfd answers both fields, the amdgpu PCI driver answers only amdgpu_loaded, and with nothing planted the fields are false (list read) or unknown (no list). Swapping the host checks behind the two fields fails it.
• probe_modules_from, handed a module list that names neither driver, against a planted host root with /dev/kfd (both fields true) and with /sys/bus/pci/drivers/amdgpu (amdgpu_loaded true, amdkfd_loaded false): dropping the host check or swapping the two fields at the call site fails them.
• e2e scenarios examine-reports-kfd-built-into-the-kernel (examine-31) and examine-reports-kfd-absent-without-kfd-interfaces (examine-32): a planted host root and a lsmod that lists amdgpu and no amdkfd; examine --json must report amdkfd_loaded true with /dev/kfd and false without.
• e2e scenario examine-reports-amdgpu-built-into-the-kernel (examine-33): a planted host root with /sys/bus/pci/drivers/amdgpu, a lsmod that lists no amdgpu, a ROCm install and Secure Boot enabled. examine --json must report amdgpu_loaded true, and diagnose --json must cite none of the three evidence lines above.
• e2e scenario examine-reports-drivers-from-kfd-without-a-module-list (examine-34): a planted host root with /dev/kfd and no /proc/modules, and a lsmod that fails. examine --json must report both amdkfd_loaded and amdgpu_loaded true.

All of them run on any Linux runner and need no GPU.

cargo test -p rocm-core --lib passes, the new tests pass with --features e2e-test-hooks, and clippy with that feature is clean. Mutating the kfd probe (hard-coding the host check off, dropping the unreadable-list fallback, dropping the host check from the decision) or swapping the host checks behind the two fields fails the unit tests. examine-31 to examine-34 pass through cargo xtask e2e. Not run on real hardware; the behaviour was checked against the reporter's examine output and the upstream kernel Kconfig/Makefile.

[jussielo-amd]

This review was produced using Claude Code's review-code method (independent fan-out over the core probe logic, the e2e feature/steps, and AGENTS.md/impact compliance), at the request of @jussielo-amd. No GitHub approval is filed by this account for automated reviews — the merge decision stays with a human reviewer.

Blocking

• CI: "E2E tests (Strix Halo, WSL2, current)" is failing on the current head (7f9c703). The suite's own reconciliation step reports: FAIL: 'comfyui-source-download-shows-live-progress' was expected to pass on this host but FAILED — a regression. That scenario lives in comfyui.feature and isn't touched by this PR's diff (examine.rs, examine.feature, examine_steps.rs). Issue #580 ("comfyui-04 times out when managed Python bootstraps inside its 30s spinner budget") describes a similar timing flake in the same area on this job class, so this looks like a pre-existing flake rather than something this PR broke. Requesting changes mainly to get a rerun/confirmation before merge rather than because the diff itself looks wrong here — happy to withdraw once the WSL2 job is green (or confirmed as an unrelated, already-tracked flake).

Non-blocking

• crates/rocm-core/src/hardware_root.rs's module doc comment says it enumerates "every one of these reads, and only these" that go through the mockable host_path, but doesn't list the new /sys/class/kfd/kfd read added by kfd_present_on_host() in examine.rs. The read itself is correctly routed through host_path; this is just a stale enumeration. Suggest adding it alongside the existing topology-path mention.
• amdgpu_state/amdkfd_state in examine.rs are structurally identical modulo the literal module name and the host-presence bool — could collapse into one generic helper to remove the duplication.
• The new e2e step definitions in tests/e2e-cucumber/tests/e2e/examine_steps.rs duplicate the existing fake-executable/PATH-prepend idiom that plant_gpu_fixtures already implements (now three near-identical copies in this file). A small shared helper would remove the duplication.
• Pre-existing, pattern-relevant: the new /sys/class/kfd/kfd, /dev/kfd, and /sys/module/amdgpu checks use Path::exists(), which doesn't distinguish "absent" from "I/O error / permission denied" — same as every other host_path(...).exists() check already in this file. Not introduced by this change, and unlikely to matter in practice since /sys//dev entries are normally world-traversable, but noting it since the new code extends the same pattern.
• Minor: the pre-existing e.kfd field computation and the new kfd_present_on_host() each independently stat /dev/kfd through host_path — harmless but redundant.

Positive signals

• The full tri-state branch set for amdgpu_state/amdkfd_state is covered by unit tests, and a mutation check (flipping || to &&) correctly broke 2 of 3 tests — a real regression guard, not a tautological one.
• The e2e scenario asserting diagnose --json omits the DKMS/"module not loaded"/Secure-Boot evidence lines traces cleanly to diagnose.rs's three gates, all keyed on amdgpu_loaded == Some(false) — reverting the fix would reintroduce exactly those lines.
• Doc comments on the new fields/helpers accurately reflect real Linux kernel Kconfig/module_param behavior for built-in vs. loadable drivers.

Full diff reviewed: 03cdbcc (base) → 7f9c703 (head), all three changed files read completely.

[juhovainio]

Reviewed the fix for #448. The probe logic for amdgpu_loaded/amdkfd_loaded correctly folds in a host-presence check (sysfs class device / /dev/kfd for kfd, /sys/module/amdgpu for amdgpu) alongside the existing lsmod check, and preserves the three-state semantics: a readable module list ORs module-presence with host-presence, while an unreadable list still reports true if the host shows the driver active, and otherwise stays unknown rather than collapsing to a false negative. Cross-checked against diagnose.rs's fault-evidence checks, which gate strictly on == Some(false), so this directly fixes the false DKMS/Secure-Boot/module-missing evidence the linked issue reported, with no risk of masking a genuine negative.

Extending the same fix to amdgpu_loaded (not explicitly named in #448) is the right call — it's the identical root cause, and examine-31 adds dedicated e2e coverage that diagnose no longer cites those evidence lines for a built-in amdgpu. Keeping the field name amdkfd_loaded rather than splitting it is also reasonable: downstream consumers only care whether the driver is active, not whether it was loaded as a distinct module.

One non-blocking nit inline. CI is green aside from two unrelated, pre-existing flaky lanes (a comfyui download-timing scenario and the known Strix Halo Windows PATH-apostrophe/download-infra flake) — neither touches this diff.

[nowycondro]

Automated code review (medium effort, recall-biased). 7 findings posted inline, ranked roughly by severity: two correctness issues in the new built-in-driver detection logic (a /sys/module/amdgpu-exists check that doesn't confirm the driver actually bound, and amdgpu_state/amdkfd_state not cross-checking each other despite kfd being linked into amdgpu), one cross-file consistency gap (the diagnose/fix remedy flows still assume lsmod-module semantics), and four cleanup items (reuse of an existing host_gpu.rs check, a doc-comment invariant in hardware_root.rs that's now stale, and two minor simplification/efficiency notes).

[nowycondro]

🤖 Automated review (Claude Code) · 7f9c703

This supersedes/clarifies the review posted moments ago (pullrequestreview-5655465252) to match the required report format; the 7 inline findings on that review stand as the substantive output of this pass.

Scope read: full diff base 03cdbcc → head 7f9c7032 in crates/rocm-core/src/examine.rs (the amdgpu_state/amdkfd_state/kfd_present_on_host/amdgpu_present_on_host logic), plus the new e2e scenario/steps. No Rust toolchain available in this sandbox — I did not build or run the test suite; findings are from static reading of the diff only, cross-referenced against diagnose.rs, fix.rs, host_gpu.rs, and hardware_root.rs in the same tree.

Blocking (confidence: medium)

• examine.rs (amdgpu_present_on_host, ~line 1788): /sys/module/amdgpu existing only proves the kernel registered the module's parameters at an early initcall, not that the driver actually bound/probed successfully (e.g. under nomodeset or a probe failure). A loadable module in that broken state is correctly reported false; the new built-in-kernel path would now report true for the same broken state. Concrete failure: built-in amdgpu fails to bind a GPU, amdgpu_loaded reports true, and diagnose.rs's == Some(false) gates (DKMS/module-missing/Secure-Boot evidence) are suppressed even though the driver genuinely isn't working.
• examine.rs (amdgpu_state/amdkfd_state, ~line 1797): the two states are computed independently even though kfd is linked into amdgpu; a container that passes through /dev/kfd without exposing /sys/module can yield the contradictory amdgpu_loaded=false / amdkfd_loaded=true.

Non-blocking

• examine.rs (~line 1787) duplicates presence checks already implemented in host_gpu.rs::detect_driver_summary() instead of sharing one helper.
• diagnose.rs/fix.rs remedy flows (e.g. sudo modprobe amdgpu, lsmod | grep amdgpu verify step) still assume loadable-module semantics and can still fire on exactly the built-in-kernel machines this PR targets, producing a remedy that can never verify as succeeded.
• hardware_root.rs's module doc comment claims an exhaustive list of paths routed through host_path; the new /sys/class/kfd/kfd read isn't listed (stale invariant comment, not a behavior bug).
• amdgpu_state/amdkfd_state are structurally near-identical and could collapse into one parameterized helper.
• Minor/efficiency: host-presence checks run eagerly even when the module list already resolved the state to true.

Not verified: build, unit tests, e2e suite (no toolchain in this sandbox); CI status on the current head (see the other human/automated reviews on this PR for that signal).

🤖 Generated with Claude Code

[tomastola]

Pushed 6891205 for the 16:30 review.

Fixed

• amdgpu_loaded and amdkfd_loaded are no longer decided independently. kfd is linked into amdgpu, so kfd on the host now also answers amdgpu_loaded. A container with /dev/kfd but no /sys/module can't report amdkfd up and amdgpu down.
• amdgpu_state and amdkfd_state are now one module_or_host_state. The unit tests are collapsed to match.
• The host probe is a closure, so it is skipped when the module list already answers. A test pins this.
• hardware_root.rs now lists /sys/class/kfd/kfd.

Not changed

• /sys/module/amdgpu for a built-in driver that never bound (nomodeset, failed probe): this is a real gap, but I left it. /sys/module/amdgpu is already this codebase's signal for "amdgpu is present" (host_gpu.rs, hardware_root.rs, the version read). Switching examine alone to /sys/bus/pci/drivers/amdgpu would make the two disagree about the same machine. Better as a follow-up that moves both together.
• Sharing the check with host_gpu::detect_driver_summary: it reads /sys/module/amdgpu for a different purpose (module metadata, amdgpu_available), so I'd do that together with the follow-up above.
• diagnose/fix remedies (lsmod | grep amdgpu, modprobe amdgpu) still assume a loadable module. That is a separate change to the remedy recipes and out of scope for the detection fix here.

The WSL2 failure is comfyui-source-download-shows-live-progress, a scenario this diff doesn't touch (a timing flake, #580). I haven't re-run it, and the new run will show whether it recurs.

[nowycondro]

🔴 Automated review · pr-review-watcher · 47d8a2a

Review

Full review of the whole change (crates/rocm-core/src/examine.rs, crates/rocm-core/src/hardware_root.rs, tests/e2e-cucumber/features/examine.feature, tests/e2e-cucumber/tests/e2e/examine_steps.rs), base a9e9aa2…head 47d8a2a. The PR explicitly fixes issue #448 (amdkfd_loaded false negative) and extends the same fix to amdgpu_loaded for the same root cause; the PR body states that extension plainly, so it is not scope creep.

Blocking

• The one genuinely new behavior this PR adds — "kfd on the host implies amdgpu" — has no test that exercises it. crates/rocm-core/src/examine.rs:1863-1865 computes e.amdgpu_loaded as module_or_host_state(.., "amdgpu", || amdgpu_present_on_host() || kfd_present_on_host()). The kfd_present_on_host() disjunct is new and is called out in both the inline comment above it and in commit 0ae8927a's message ("kfd is linked into amdgpu, so kfd on the host now also answers amdgpu_loaded"). Every test that asserts amdgpu_loaded == Some(true) instead plants /sys/module/amdgpu directly: the unit test probe_modules_reports_amdgpu_when_it_is_on_the_host (examine.rs:6278-6300) and e2e scenario examine-32 via machine_with_built_in_amdgpu (tests/e2e-cucumber/tests/e2e/examine_steps.rs:1520-1549, asserted by assert_amdgpu_loaded at line 1574). No unit test or e2e scenario plants only a kfd signal (/dev/kfd or /sys/class/kfd/kfd) with no amdgpu module entry and no /sys/module/amdgpu, and checks that amdgpu_loaded becomes true through that path alone. Deleting || kfd_present_on_host() from that closure would not fail a single test in the suite.
  Confidence 85 · logic · Fix: add a unit test (and ideally an e2e scenario, e.g. a container-shaped host that passes through /dev/kfd but has no /sys/module/amdgpu) that plants kfd-only presence and asserts amdgpu_loaded == Some(true).

Non-blocking

• amdgpu_loaded's doc comment understates its own logic. examine.rs:228-231 says it is true when "listed as a module, or /sys/module/amdgpu exists" — it omits the third signal the implementation now ORs in (kfd_present_on_host(), line 1864). The sibling amdkfd_loaded doc comment is fully accurate about its own logic; this one is not.
  Confidence 85 · mechanical · Fix: mention the kfd-on-host fallback in the doc comment.
• amdgpu_present_on_host() (examine.rs:1821-1823, checking host_path("/sys/module/amdgpu").exists()) duplicates logic already in host_gpu.rs::detect_driver_summary() (crates/rocm-core/src/host_gpu.rs:2449-2456), which independently checks the same path (and already separately folds in /dev/kfd for its detail text). Two independent "is amdgpu present on this host" checks now exist in the crate.
  Confidence 70 · logic (reuse) · Fix: consider sharing one helper between the two call sites as a follow-up; not required for this PR's scope.
• The new ~140-line e2e fixture block duplicates the file's existing "write a fake executable, prepend it onto PATH" idiom (plant_machine_without_amdkfd_module at examine_steps.rs:1463, and the equivalent block inside machine_with_built_in_amdgpu at line 1520) rather than reusing it — a pre-existing instance of the idiom plus these two new ones means it's now written out three times in this file.
  Confidence 70 · mechanical (reuse) · Fix: extract a shared "plant fake lsmod/mokutil on PATH" helper now that the pattern recurs a third time.
• The new fixture block has no section-divider comment, unlike the rest of the file's established convention, and currently reads as though it belongs to the preceding "release file" section (gpu_machine_with_unreadable_release_file, examine_steps.rs:1445) rather than being its own section.
  Confidence 70 · mechanical · Fix: add the divider comment used elsewhere in the file.
• Pre-existing, pattern-relevant: diagnose.rs (:853, :978) and fix.rs (:333, :369) still print user-facing remediation strings of the form "lsmod | grep amdgpu && rocminfo | head -n ..." — the exact false-negative class this PR fixes in detection (a built-in amdgpu never appears in lsmod) still exists in the remediation text a user is told to run to confirm the driver is present. Not touched by this PR's four changed files, so out of scope here, but the same bug class and worth a follow-up.
  Confidence 70 · logic · Fix: a future change to those verify strings, e.g. checking /sys/module/amdgpu instead of/in addition to lsmod.
• Commit 0ae8927a is labeled refactor(examine): … but is not a pure refactor — it adds the kfd_present_on_host() disjunct to amdgpu_loaded's computation, which is new semantics, not a reshuffle, as its own commit message's second sentence states. The behavior is otherwise fully described in the message, documented in the doc comments, and intentional; this is a labeling nit only.
  Confidence 70 · mechanical · Fix: none needed; note for future commit-type labeling.

Decisions for the author

• Naming tradeoff, already made and documented — the PR keeps the field name amdkfd_loaded for JSON-schema compatibility rather than splitting it into e.g. amdkfd_module_loaded / kfd_functional as issue #448 suggested as an alternative. The PR body states this explicitly and gives the reason (schema stability). Reasonable; flagging only so the choice is visible as deliberate rather than overlooked.
• Historical continuity worth a one-line mention — PR #434 (merged earlier) explicitly observed and recorded amdkfd_loaded: false alongside has_amd_gpu: true on an MI300X host as a known, unresolved quirk at the time ("the MI300X host confirms it, reporting amdkfd_loaded: false"). This PR now fixes exactly that quirk but doesn't reference #434. Not a code defect — purely a continuity nicety for anyone searching history later.

Positive signals

• module_or_host_state's unit tests (examine.rs ~6186-6260) cover the full tri-state matrix cleanly: module listed, built-in-and-absent-from-the-list, neither-present, unreadable-list-follows-host, and host-probe-skipped-when-the-list-already-answers (that last one uses || unreachable!() as its own proof).
• hardware_root.rs's module doc comment, which claims to enumerate "every one of these reads, and only these" through the mockable host_path, was correctly updated to add the new /sys/class/kfd/kfd read — this invariant is accurate as of this PR.
• E2e scenario examine-32 does real impact-analysis work: it drives both examine --json and diagnose --json against the same planted host and asserts the diagnose evidence lines are actually absent, not just that the flag reads true — closing the loop to the real downstream consumer rather than stopping at the probe.
• Clean 7-commit history: each commit is independently meaningful, DCO-signed, and a numbering collision in the e2e scenario IDs was proactively caught and fixed in its own dedicated commit (47d8a2aa) rather than left for review to find.

Deployment notes

• Behavioral change, not just a bugfix in the abstract: on hosts where amdgpu/kfd are compiled into the kernel (increasingly common on stock distro kernels), rocm examine --json's amdgpu_loaded/amdkfd_loaded will now report true where they previously reported false, and rocm diagnose will stop emitting its DKMS-failure / module-not-loaded / Secure-Boot evidence lines for such hosts. JSON field names and types are unchanged (confirmed by reading Examination's struct definition), so this is a value-level behavior change, not a schema change.
• As of this review (live-checked against head 47d8a2aa), no check had failed, but several E2E lanes (MI300X, Strix Halo Ubuntu release/nightly, W7900) were still QUEUED/not finished — full CI had not completed.

What this covered

Read all four changed files in full at the PR head, plus the consumers of the changed fields (diagnose.rs, fix.rs, host_gpu.rs) and the full AGENTS.md, via three independent blind passes (core probe logic + consumers, e2e feature/steps, and a cross-cutting AGENTS.md/impact-analysis/history/commit-quality pass), then consolidated and independently re-verified the load-bearing claims directly against the source (the untested-branch claim, the doc-comment gap, the host_gpu.rs duplication, the stale lsmod remedy strings, the refactor-commit behavior change, and the PR #434 history) before scoring. Base a9e9aa2, head 47d8a2a, re-confirmed live immediately before publishing this review.

Did not run: cargo test/cargo clippy locally in this sandbox — reasoned from reading the code and test matrix directly instead, and cross-checked against the PR's own CI, where the rust/clippy/build-and-test/coverage checks are green on this head (several GPU-hardware E2E lanes were still queued, not failed, at review time). The "prior changes" pass (whether review comments on earlier, unrelated PRs touching these same files still apply here) was not separately run. Issue #448 was fetched directly (gh issue view) and checked against the diff and PR description — the PR accurately describes its own scope, including the deliberate extension beyond what #448 asked for.

Not reconciled against any prior review thread on this PR — this review was produced blind to that discussion by design.

[nowycondro]

🔴 Automated review · pr-review-watcher · 3266af4

Review — needs work

Full review of the whole change. The PR fixes issue #448 (amdkfd_loaded false negative when kfd is built into the kernel) and deliberately extends the same fix to amdgpu_loaded, stating that extension explicitly in its own body — not scope creep.

Blocking

• The "kfd present on the host implies amdgpu is loaded too" branch has no test that exercises it — crates/rocm-core/src/examine.rs:1866-1868 computes e.amdgpu_loaded as module_or_host_state(modules, "amdgpu", || amdgpu_present_on_host() || kfd_present_on_host()). The kfd_present_on_host() disjunct is new, deliberate behaviour (the inline comment at examine.rs:1864-1865 and the commit introducing it both say so explicitly). I traced every test touching amdgpu_loaded: the unit tests for module_or_host_state pass synthetic || true/|| false closures, never the real composed one; amdgpu_present_on_host_reads_the_pci_driver_dir and probe_modules_reports_amdgpu_when_it_is_on_the_host (examine.rs:6249, ~6300) plant only /sys/bus/pci/drivers/amdgpu, never kfd alone; e2e scenario examine-32 (machine_with_built_in_amdgpu, tests/e2e-cucumber/tests/e2e/examine_steps.rs:1520) plants the PCI driver dir directly and never plants /dev/kfd/kfd sysfs; examine-30/31 plant a fake lsmod that already lists amdgpu as a module, so modules.contains("amdgpu") answers before the host closure is ever reached, and neither scenario asserts amdgpu_loaded at all. Deleting || kfd_present_on_host() from examine.rs:1867 would not fail a single existing test.
  Confidence 90 · logic · Fix: add a unit test and/or e2e scenario that plants kfd-only presence (/dev/kfd or /sys/class/kfd/kfd) with no amdgpu module entry and no /sys/bus/pci/drivers/amdgpu, and asserts amdgpu_loaded == Some(true) through that path alone.

• The PR description no longer matches the shipped code. The body still says amdgpu_loaded is true when "amdgpu is in the module list or /sys/module/amdgpu exists" — the final commit on this PR (fix(examine): take a built-in amdgpu from the PCI driver, not /sys/module) deliberately replaced that check with /sys/bus/pci/drivers/amdgpu (examine.rs:1829-1833) specifically because /sys/module/amdgpu is created even when the driver's init aborted under nomodeset/amdgpu.modeset=0 — the description now describes the behaviour its own author judged unreliable. The description also never mentions the kfd_present_on_host() fallback inside the amdgpu computation (examine.rs:1867) at all. The Tests section names amdkfd_state/amdgpu_state (examine.rs, Tests paragraph) — functions that no longer exist, having been unified into module_or_host_state by an earlier commit in this same PR — and cites e2e scenario IDs examine-29/examine-30/examine-31, while the actual scenarios in tests/e2e-cucumber/features/examine.feature are numbered examine-30/examine-31/examine-32 after a later renumbering commit. A reader relying on the description to understand what shipped would get the wrong sysfs path, miss an entire code path, and be unable to find the cited tests by name or ID.
  Confidence 85 · logic · Fix: update the PR body to describe the final /sys/bus/pci/drivers/amdgpu check, mention the kfd-implies-amdgpu fallback, and correct the test/scenario names and IDs before merge.

Non-blocking

• Pre-existing, pattern-relevant: host_gpu.rs's detect_driver_summary() (crates/rocm-core/src/host_gpu.rs:2456) still decides status: "amdgpu_available" from host_path("/sys/module/amdgpu").exists() — the exact check this PR's own investigation found unreliable for a built-in driver (see the rationale at examine.rs:1829-1833). Not touched by this diff and not called from the changed code path, so out of scope here, but it's the identical bug class sitting right next to the fix.
  Confidence 70 · logic (reuse) · Fix: consider sharing amdgpu_present_on_host() with this call site as a follow-up.
• The fake-executable/PATH-prepend fixture idiom is now duplicated 3-4 times in tests/e2e-cucumber/tests/e2e/examine_steps.rs with no shared helper: the pre-existing plant_gpu_fixtures (:1409-1431), and the two new fixtures this PR adds, plant_machine_without_amdkfd_module (:1463-1490) and machine_with_built_in_amdgpu (:1520-1549), each write a fake executable and manually prepend it onto PATH via the same std::env::join_paths/split_paths sequence.
  Confidence 70 · mechanical (reuse) · Fix: extract a small fn prepend_fake_bin_to_path(world, bin) helper now that the pattern recurs a third time.

Decisions for the author

• Naming tradeoff, already made and documented — the PR keeps the field name amdkfd_loaded for JSON-schema compatibility rather than splitting it into e.g. kfd_functional, which issue #448 suggested as an alternative. The body states this explicitly with its reasoning; flagging only so the choice is visible as deliberate.
• Historical continuity worth a one-line mention — an earlier merged PR (#434) recorded amdkfd_loaded: false on an MI300X host as a known, unresolved quirk at the time. This PR fixes exactly that quirk but doesn't reference #434. Not a defect, purely a continuity nicety for anyone searching history later.

Positive signals

• module_or_host_state's unit tests cover the full tri-state matrix cleanly (module listed, built-in-and-absent, neither present, unreadable-list-follows-host, host-probe-skipped-once-the-list-answers — the last uses || unreachable!() as its own proof).
• diagnose.rs's fault-evidence checks all gate strictly on == Some(false), so the new tri-state semantics (module list OR host probes, falling back to None/"unknown" rather than a false negative) compose correctly with the existing consumer without any additional change needed there.
• hardware_root.rs's module doc comment, which claims to enumerate every host-path read in the crate, was correctly updated to add both the new /sys/class/kfd/kfd class-device read and /sys/bus/pci/drivers/amdgpu — this invariant holds as of this PR.
• Clean 8-commit history: each commit is independently meaningful and signed off, and the switch from /sys/module/amdgpu to the PCI-driver-dir check (a real correctness improvement, catching a case the earlier commits in this same PR had gotten wrong) was caught and fixed in its own dedicated commit rather than folded in silently.

Deployment notes

• Behavioural change, not just a bugfix in the abstract: on hosts where amdgpu/kfd are compiled into the kernel, rocm examine --json's amdgpu_loaded/amdkfd_loaded will now report true where they previously reported false, and rocm diagnose will stop emitting its DKMS-failure/module-not-loaded/Secure-Boot evidence lines for such hosts. JSON field names and types are unchanged, so this is a value-level behaviour change, not a schema change.
• As of this review (live-checked against head 3266af46), every CI check has completed and passed except the aggregate "E2E tests" check, which was still in progress; nothing had failed.

What this covered

Read both changed core-logic files (crates/rocm-core/src/examine.rs, crates/rocm-core/src/hardware_root.rs) and both changed e2e files (tests/e2e-cucumber/features/examine.feature, tests/e2e-cucumber/tests/e2e/examine_steps.rs) completely, via two independent blind fan-out passes grouped by coupling (core probe logic + its consumers; e2e feature/steps), plus my own direct reading of the full diff, the 8-commit history, the consumers in diagnose.rs/fix.rs/host_gpu.rs, and AGENTS.md. Consolidated and independently re-verified the load-bearing claims against the source (the untested-branch claim, the stale-description claim, the host_gpu.rs duplication, the e2e fixture duplication) before scoring. Base a9e9aa2103ae974b10c5f6e141c09dc6720d8c26, head 3266af461f84bff2e35a69d587f87461b6302a17, re-confirmed live immediately before publishing this review. Issue #448 was fetched directly and checked against the diff and description.

Did not run: cargo test/cargo clippy locally (no Rust toolchain in this sandbox) — reasoned from reading the code and test matrix directly instead, cross-checked against the PR's own CI, where every completed check is green on this head. The "prior changes" pass (whether review comments on earlier, unrelated PRs touching these same files still apply here) was not separately run beyond the one historical PR (#434) found by searching commit history for this exact field name.

Not reconciled against any prior review thread on this PR — this review was produced blind to that discussion by design.

[tomastola]

Pushed 108d462 for the 3266af4 review (it had no inline threads, so replying here).

Fixed

• Untested "kfd implies amdgpu" branch: the composed host probe is now amdgpu_implied_by_host, and kfd_alone_on_the_host_implies_amdgpu plants only /sys/class/kfd/kfd, then only /dev/kfd, and asserts true. I checked it fails with the kfd disjunct removed. I did not add an e2e scenario, because the unit test is independent of the lsmod of the machine running it.
• The amdgpu_loaded doc comment now names the kfd fallback.
• PR description: updated to the PCI driver directory check, the kfd fallback, module_or_host_state, and examine-30/31/32. It also now says the e2e scenarios were last run locally before the renumbering and the PCI-directory change.

Not changed

• host_gpu.rs still reads /sys/module/amdgpu, and the lsmod remedy strings in diagnose.rs/fix.rs: answered on the earlier threads, and followed up on examine: amdkfd_loaded=false is a false negative when kfd is built into the kernel (no module in lsmod) #448. This review brings no new evidence.
• Repeated fake-executable/PATH fixture in examine_steps.rs: this is the existing idiom in that file, and I'd rather not refactor unrelated steps in this PR.
• fix(examine): enumerate Instinct GPUs from lspci's accelerator class #434 reference: skipped; it is history only.

[nowycondro]

🔴 Automated review · pr-review-watcher · 108d462

Review — no blocking findings

Full review of the whole change: crates/rocm-core/src/examine.rs, crates/rocm-core/src/hardware_root.rs, tests/e2e-cucumber/features/examine.feature, and tests/e2e-cucumber/tests/e2e/examine_steps.rs, base a9e9aa21…head 108d462c. The PR fixes issue #448 (amdkfd_loaded reporting false on a host where kfd is built into the kernel) and deliberately extends the same reasoning to amdgpu_loaded, stating that extension explicitly in its own description. The most recent commit on this PR adds a dedicated unit test (kfd_alone_on_the_host_implies_amdgpu) and a named helper (amdgpu_implied_by_host) that together pin down the "kfd on the host alone implies amdgpu" path, which is exactly the kind of behavior that benefits from its own direct test rather than only being exercised incidentally through other fixtures. With that in place, and with CI fully green, this round found no blocking issues — only two small, pre-existing-style documentation/maintenance notes below.

Blocking

None

Non-blocking

• Two doc comments on this PR's own new code disagree about whether a pre-4.20 kernel's amdkfd ever shows up in lsmod. The amdkfd_loaded field doc (crates/rocm-core/src/examine.rs:234-237) says kfd is available "as a separate amdkfd module on kernels before 4.20 (where kfd was merged into amdgpu.ko)" — implying older kernels do list it as a module. The kfd_present_on_host() doc directly above it (crates/rocm-core/src/examine.rs:1813-1818) says the opposite: "amdkfd is linked into amdgpu, so no amdkfd line ever appears in the module list, built in or not." The field doc looks like the one that matches reality (and matches why probe_modules still checks the module list for "amdkfd" before falling back to the host probe) — the function doc should be narrowed to "on current kernels" rather than stated unconditionally.
• The fake-executable/PATH-prepend fixture idiom is now written out a third time in tests/e2e-cucumber/tests/e2e/examine_steps.rs: the pre-existing plant_gpu_fixtures (:1409), and this PR's two new fixtures plant_machine_without_amdkfd_module (:1463) and machine_with_built_in_amdgpu (:1520), each independently create a fake binary and splice it onto PATH. Not a functional problem, but a good candidate for a small shared helper now that the pattern recurs.

Decisions for the author

• Naming tradeoff, already made and documented — the PR keeps the field name amdkfd_loaded for JSON-schema compatibility rather than splitting it into separate module-vs-functional fields, which issue #448 raised as an alternative. The description states this explicitly with its reasoning; noting it here only so the choice reads as deliberate.

Positive signals

• The tri-state logic (module_or_host_state: module-list hit, host-probe hit, list unreadable but host answers, neither) is covered by unit tests for every branch, including a test that proves the host probe is skipped once the module list has already answered.
• The e2e scenario for a built-in amdgpu drives both examine --json and diagnose --json against the same planted host and asserts that diagnose's DKMS/module-not-loaded/Secure-Boot evidence lines are actually absent, closing the loop to the real downstream consumer rather than stopping at the probe's own output.
• The PR description's "Tests" section accurately names the current helper functions, test names, and e2e scenario IDs as they exist in the code at this head — it reads as kept in sync through the PR's later commits rather than written once and left stale.

Deployment notes

• Behavioral change, not just a bugfix in the abstract: on hosts where amdgpu/kfd are compiled into the kernel, rocm examine --json's amdgpu_loaded/amdkfd_loaded will now report true where they previously reported false, and rocm diagnose will stop emitting its DKMS-failure/module-not-loaded/Secure-Boot evidence lines for such hosts. JSON field names and types are unchanged, so this is a value-level behavior change, not a schema change.
• CI on this head: all 24 checks report either success (22) or an advisory skip (2, not required for merge); nothing failing or pending at review time.

What this covered

Read both core-logic files and both e2e files completely at the PR head, via two independent passes grouped by coupling (core probe logic + its mockable host-path primitive; e2e feature + step definitions), plus a direct read of the full project contributor-guidance file and a comparison of the PR description and linked issue (#448) against the shipped code. Did not run cargo test/cargo clippy/the e2e suite locally — no Rust toolchain or package-registry network access in this environment — so the above is reasoned from reading the code and tests directly, cross-checked against this PR's own CI (fully green). Not reconciled against any existing review thread on this PR; this review was produced independently against the code at the current head.

[tomastola]

Pushed a doc-only commit for the 108d462 review. The kfd_present_on_host doc comment now says kfd is linked into amdgpu on current kernels, and that before 4.20 it was a separate amdkfd module, so it agrees with the amdkfd_loaded field doc.

The fake-executable/PATH fixture written out three times in examine_steps.rs is a fair follow-up but out of scope here. Extracting it means touching unrelated steps in the same file.

[tomastola]

@jussielo-amd your blocking item was the "E2E tests (Strix Halo, WSL2, current)" lane. On 6891205 it passed (job).

That self-hosted run went red on two other lanes. Neither failure involves examine:

• Strix Halo, Windows: the lane stopped in setup, before any scenario ran. rocm engines install got HTTP 403 downloading the TheRock tarball for the Lemonade backend.
• MI300X: the failing steps are serve and engine-install scenarios. The Lemonade llama.cpp and TheRock tarball downloads failed after 6 attempts, and there is a vllm reinstall. Every examine scenario passed, including the new examine-31.

The self-hosted run on the current head (4d58eac) is still queued. Every other check on 4d58eac is green. If the WSL2 result is enough for you, could you dismiss the changes-requested review?

[nowycondro]

🔴 Automated review · pr-review-watcher · 4d58eac

Review — no blocking findings

Full review of the whole change (4 files, 457 lines, two independent fan-out workers over coupled file groups plus driver-level impact analysis).
Fixes #448 (GitHub issue, retrieved): "amdkfd_loaded=false is a false negative when kfd is built into the kernel." The change implements the issue's suggested functional-check fallback for amdkfd_loaded, and additionally fixes a related amdgpu_loaded blind spot not raised in the issue itself — this extra scope is disclosed and justified in the PR description, so it is not an undisclosed-work mismatch.

Blocking

None

Non-blocking

• assert_no_amdgpu_fault_evidence's first guarded substring doesn't verbatim-match one of the two evidence strings it's meant to guard — tests/e2e-cucumber/tests/e2e/examine_steps.rs:1593 (checked against crates/rocm-core/src/diagnose.rs:829 and :934)
  The assertion guards against "amdgpu module is not loaded". That matches check_5_amdgpu_blacklisted's evidence (diagnose.rs:934) exactly, but check_3_rocm_kernel_unsupported's evidence (diagnose.rs:829) reads "...the amdgpu kernel module is not loaded..." — the extra word "kernel" breaks the substring match. The test is not currently vacuous: a regression in check_3 would still be caught by the separate "DKMS failed" substring in the same assertion, since check_3's full evidence text also contains that phrase. But the two substrings now guard the two checks by accident of overlapping wording rather than by each substring matching its intended check, so a future independent wording tweak to check_5 (e.g. inserting "kernel" to match check_3's phrasing) would silently stop that substring from testing anything, with the assertion still passing.
  Confidence 75 · mechanical · Fix: match check_3's and check_5's evidence text precisely (two substrings, one per check), or assert on diagnosis IDs/titles rather than free-text evidence substrings.

Decisions for the author

• kfd presence on the host is treated as sufficient to imply amdgpu_loaded = true, without independently confirming /sys/bus/pci/drivers/amdgpu — tradeoff
  This is deliberate and documented (amdgpu_implied_by_host's doc comment: "a container that passes /dev/kfd through without /sys/bus/pci must not report amdkfd up and amdgpu down"), and is the right call for the stated container-passthrough case. The flip side: a mount namespace that still exposes a stale /dev/kfd node after the host's amdgpu driver has actually unbound (rare, but not guaranteed impossible across every container runtime) would make amdgpu_loaded report true when the PCI driver is genuinely gone. Worth confirming this residual risk is acceptable versus the false-negative it replaces, which was the entire point of the fix.

Positive signals

• The doc comments on amdgpu_present_on_host/amdgpu_implied_by_host give a precise, falsifiable reason for choosing /sys/bus/pci/drivers/amdgpu over /sys/module/amdgpu (the latter appears at an early initcall even when amdgpu_init then bails out on nomodeset), and the unit test amdgpu_present_on_host_reads_the_pci_driver_dir directly plants both directories separately to prove the distinction the comment claims.
• the_host_probe_is_skipped_when_the_module_list_already_answers uses an || unreachable!() closure to prove the laziness claimed in module_or_host_state's doc comment, rather than merely asserting the result.
• The PR description directly engages with the linked issue's own suggested alternative (splitting into amdkfd_module_loaded/kfd_functional) and states a specific reason for rejecting it (JSON schema compatibility) rather than silently picking one approach.

Deployment notes

None — both fields keep their existing names, types, and Option<bool> tri-state semantics (unknown stays unknown when neither the module list nor the host answers); this only makes Some(true) reachable in more true-positive cases. probe_modules remains gated to os_family == "linux", so Windows/WSL paths are unaffected.

What this covered

Every file in the diff read completely by at least one of two independent fan-out workers, grouped by coupling: (1) crates/rocm-core/src/examine.rs + crates/rocm-core/src/hardware_root.rs (core probe logic, its unit tests, and every amdgpu_loaded/amdkfd_loaded read site in crates/rocm-core/src/diagnose.rs for impact analysis), and (2) tests/e2e-cucumber/features/examine.feature + tests/e2e-cucumber/tests/e2e/examine_steps.rs (e2e scenarios and step definitions, cross-checked against the real probe_modules/probe_secure_boot parsers and the DiagnoseReport JSON schema). I additionally read the commit history on the branch (10 commits, confirming substantial iteration since the previously-reviewed commit 108d462 — notably a full switch of the amdgpu signal from /sys/module/amdgpu to /sys/bus/pci/drivers/amdgpu in commit 3266af4), checked for other consumers of these two fields and for required doc/README updates (none needed; no doc references these fields' detection semantics), retrieved the linked issue (#448) directly by its identifier via gh issue view, and read CI (gh pr view --json statusCheckRollup): every required check is SUCCESS; the two SKIPPED checks (Skill checks (skillscope), Sphinx docs build (-W)) are path-filtered and correctly skipped since this change touches neither skills/ nor docs.
Did not run: cargo/clippy locally (no cargo available in this sandbox) — relied on CI's passing clippy, build-and-test, and Coverage (rocm-dash crates, ratcheted) checks instead, per the review method's guidance to trust a passing check rather than re-derive what it covered. History pass (git log/git blame beyond the commit-message read above) and prior-changes pass were folded into the fan-out workers' context rather than run as fully separate passes. Not reconciled against any discussion on this PR — this report was produced blind to any existing review thread, per instructions, and the prior review at commit 108d462 was deliberately not read.

[tomastola]

Pushed bc2937d for the 4d58eac review. The review had no inline threads, so I'm replying here.

Fixed: the evidence substrings in assert_no_amdgpu_fault_evidence
The assertion now has one phrase per check, each copied from that check's evidence text:

• "the amdgpu kernel module is not loaded" for check_3
• "amdgpu module is not loaded" and the Secure Boot line for check_5

check_3 is no longer caught only through the "DKMS failed" overlap.

To check that each phrase works, I made amdgpu_loaded report false and ran examine-32 locally. The step failed on check_5's line. With the check_5 phrases removed from the list, it still failed, this time on check_3's line. The unmodified scenarios examine-31 and examine-32 pass.

Decision: kfd on the host implies amdgpu_loaded = true
I'm keeping it. A stale /dev/kfd would also make amdkfd_loaded true, because both fields read the same host evidence. Deriving amdgpu_loaded from it keeps the two fields consistent. Reporting kfd up and amdgpu down describes a state that cannot exist, since kfd is linked into amdgpu. Before this change, the container passthrough case (with /dev/kfd but no /sys/bus/pci) always read as "amdgpu not loaded". I judge that false negative to be the bigger risk.

[nowycondro]

🔴 Automated review · pr-review-watcher · bc2937d

Review — needs work

Full review of the whole change, read against a9e9aa21…bc2937d8 (the merge-base with origin/main).
Fixes #448, and goes beyond it: the issue reports only the amdkfd_loaded false negative, the change also fixes the analogous amdgpu_loaded false negative for a built-in driver. Both are described accurately in the PR body.

Blocking

• The new host-fallback wiring in probe_modules is under-tested: two ways it could regress would still pass every test. crates/rocm-core/src/examine.rs:1873-1874 (the call site), with the gap visible at :6294-6318 (probe_modules_reports_amdgpu_when_it_is_on_the_host), :6394-6418 (probe_modules_reports_amdkfd_when_kfd_is_on_the_host), :6237-6239 (an_unreadable_module_list_follows_the_host), and tests/e2e-cucumber/features/examine.feature:447-461 (examine-30/31).

  • (a) Swapping which closure answers which field at the call site is not caught anywhere. If line 1873 used amdgpu_present_on_host instead of amdgpu_implied_by_host (dropping "kfd on the host implies amdgpu"), or line 1874 used amdgpu_implied_by_host instead of kfd_present_on_host (making amdkfd_loaded true whenever amdgpu alone is present), no test fails:
    • probe_modules_reports_amdgpu_when_it_is_on_the_host only plants /sys/bus/pci/drivers/amdgpu and only asserts amdgpu_loaded.
    • probe_modules_reports_amdkfd_when_kfd_is_on_the_host only plants /dev/kfd and only asserts amdkfd_loaded.
    • kfd_alone_on_the_host_implies_amdgpu (examine.rs:6358-6387, per the diff) calls amdgpu_implied_by_host() directly, bypassing probe_modules's call-site wiring entirely.
    • e2e examine-30's fake lsmod already lists amdgpu, so the module list answers amdgpu_loaded before any host check runs; examine-32 plants the PCI driver dir directly, so it never isolates the kfd-only path either.
      I traced this by reading the tests against the production code, not by running a mutation (no cargo/rustc available in this sandbox).
  • (b) The "module list unreadable, host says true" path (module_or_host_state's None arm, examine.rs:1849-1858) has no scenario, only the plain helper unit test an_unreadable_module_list_follows_the_host, which calls module_or_host_state directly and asserts nothing about probe_modules's wiring or examine --json output. This is new user-observable behaviour this PR introduces (before: an unreadable module list left both fields None unconditionally; now it can resolve to Some(true)), and AGENTS.md §3 requires a scenario for CLI-observable behaviour, not only a unit test.
    Confidence 85 · logic · Fix: strengthen the two existing wiring tests rather than adding new ones — in probe_modules_reports_amdkfd_when_kfd_is_on_the_host (only /dev/kfd planted) also assert amdgpu_loaded == Some(true); in probe_modules_reports_amdgpu_when_it_is_on_the_host (only the PCI driver dir planted) also assert amdkfd_loaded == Some(false) (host-independent, since no modern kernel lists an amdkfd module). For (b), add a scenario/unit test with a failing lsmod and empty /proc/modules plus a planted /dev/kfd, asserting both fields come back true.

• docs/release-trust.md's "Simulated Host Root" path inventory was not updated with the new routed read. docs/release-trust.md:312-326, caused by crates/rocm-core/src/examine.rs:1821-1833 (new amdgpu_present_on_host/kfd_present_on_host reads) together with crates/rocm-core/src/hardware_root.rs:11-17.

  • The change correctly updated hardware_root.rs's own "every one of these reads, and only these" doc comment to add /sys/class/kfd/kfd and /sys/bus/pci/drivers/amdgpu.
  • docs/release-trust.md's "Simulated Host Root" section is the same enumeration, restated for a different audience (what the ROCM_CLI_TEST_HOST_ROOT override can steer): it still only mentions "/sys/module/amdgpu and its version file" and "the KFD topology under /sys/class/kfd", with no mention of /sys/bus/pci/drivers/amdgpu.
  • AGENTS.md §5 names this exact pattern: "the same behavior claim… often repeats across… docs/*.md; each drifts independently, so grep for the claim's wording across all of them… and check each against the actual code path." The code is what governs here — the new read genuinely goes through host_path.
  • Confidence 85 · mechanical · Fix: add /sys/bus/pci/drivers/amdgpu (and ideally call out the bare /sys/class/kfd/kfd class device as distinct from the topology-nodes subpath) to that paragraph.

Non-blocking

• A CI workflow comment enumerating which routed reads have planted-root unit assertions is now stale. .github/workflows/ci.yml:552-562. It lists which probes have planted-root assertions and says the rest (including /sys/module/amdgpu) have none; this change adds planted-root assertions for /sys/bus/pci/drivers/amdgpu, /sys/class/kfd/kfd, and /dev/kfd via probe_modules, which the comment doesn't mention. Confidence 70 · mechanical · Fix: add the new probe_modules/kfd_present_on_host/amdgpu_present_on_host tests to the list.

Decisions for the author

None.

Positive signals

• module_or_host_state keeps "module list unreadable and host silent" as None rather than Some(false), so rocm diagnose cannot score a driver as missing when nothing was actually observed.
• Choosing /sys/bus/pci/drivers/amdgpu over /sys/module/amdgpu is the right call for a built-in driver whose init bailed out (nomodeset), and it's backed by a unit test (amdgpu_present_on_host_reads_the_pci_driver_dir) pinning that /sys/module/amdgpu alone does not count.
• The 11-commit history is clean and well-scoped (one concern per commit: helper extraction, wiring, docs, each e2e scenario), with descriptive messages — no "fix"/"wip" noise.
• The PR description is unusually candid about what wasn't verified ("not re-run locally since [renumbering]... CI is the check for those"; "not run on real hardware"), which is exactly the kind of verified-vs-assumed distinction AGENTS.md asks for.

Deployment notes

• amdkfd_loaded and amdgpu_loaded in rocm examine --json change meaning for existing hosts: a modern-kernel host with kfd now reports amdkfd_loaded: true where it previously always reported false, and a host with amdgpu built into the kernel flips amdgpu_loaded from false to true. Field names and types are unchanged (schema-compatible). No consumer inside this repo (only diagnose.rs, checked) needs updating; an external consumer of the JSON contract that hard-coded the old (buggy) semantics would see a behavior change.

What this covered

Read completely: crates/rocm-core/src/examine.rs (6723 lines at head) and crates/rocm-core/src/hardware_root.rs (408 lines), tests/e2e-cucumber/features/examine.feature (474 lines) and tests/e2e-cucumber/tests/e2e/examine_steps.rs (2173 lines), via two independent fan-out workers grouped by coupling (production code; e2e test code), each reading its files in full plus the production code the scenarios exercise. I separately verified the key blocking finding myself by reading the exact test bodies and fake-lsmod fixtures against the call site, and checked diagnose.rs's three amdgpu_loaded-reading evidence lines, docs/release-trust.md, .github/workflows/ci.yml, and this repo's AGENTS.md directly.

Agent-instruction adherence (AGENTS.md) and code-comments passes were run directly rather than as separate workers, given the diff's size. History pass was limited to this branch's own 11-commit sequence (read and found clean); I did not search git blame/pre-existing history of examine.rs/hardware_root.rs beyond this branch. Prior-changes pass (review comments on earlier PRs touching these files) did not run — no search performed. Step 7 design questions were investigated directly (host-path routing consistency, diagnose.rs interaction with amdgpu_blacklisted_in, the examine.py-mirroring doc comment's relevance — found non-actionable, that file lives outside this repo) rather than via separate workers, again given the diff's size.

CI: read via gh pr view --json statusCheckRollup at the reviewed head; all required checks (Analyze ×3, clippy, CodeQL, build-and-test, Test (affected crates), windows-build-and-test, E2E tests, E2E consolidated report, license/notices/lint checks, etc.) report SUCCESS. Skill checks (skillscope) and Sphinx docs build (-W) report SKIPPED, unrelated to this diff.

Issue #448 was retrieved and read; the PR's description was read after all findings above were settled. Not reconciled against any PR review-thread discussion — this is a blind pass per the review method, and a prior nowycondro review exists on this PR at an earlier head (4d58eacc) that was not read.

[tomastola]

Pushed 672fcf4, 5474d87 and e0385fe for the bc2937d review. That review had no inline threads either, so I'm replying here.

Fixed: the probe_modules wiring could swap the host checks without any test failing (blocking, part a)
The two fields now come from driver_states(modules), which takes the module list as an argument. driver_states_answer_each_field_from_its_own_host_signal plants one signal at a time and runs it with an empty module list and with none. Swapping either closure at the call site now fails it. I applied both swaps you described and confirmed each one fails. Following your suggestion, the two probe_modules_* wiring tests now also assert the other field:

• with only /dev/kfd planted, amdgpu_loaded == Some(true);
• with only the PCI driver dir planted, amdkfd_loaded != Some(true).

That second assertion is != rather than == Some(false). If the runner's lsmod fails, the routed /proc/modules is empty, so None is also correct there.

Fixed: no scenario covered the unreadable module list (blocking, part b)
examine-33 (examine-reports-drivers-from-kfd-without-a-module-list) covers it. It plants /dev/kfd, no /proc/modules and a lsmod that exits 1, then requires examine --json to report both amdkfd_loaded and amdgpu_loaded true. examine-30 to examine-33 pass locally through cargo xtask e2e.

Fixed: docs/release-trust.md's path list (blocking)
The Simulated Host Root paragraph now lists the /sys/class/kfd/kfd class device separately from the topology under it, and adds /sys/bus/pci/drivers/amdgpu.

Fixed: the stale CI comment (non-blocking)
The planted-root coverage comment in ci.yml now names the /sys/class/kfd/kfd, /dev/kfd and /sys/bus/pci/drivers/amdgpu reads and the tests that assert them.

The PR description's Tests section now covers all of this.

[nowycondro]

🔴 Automated review · pr-review-watcher · e0385fe

Review

This round covers the 3 commits added since the last review at bc2937d8 (672fcf4, 5474d87, e0385fe), which were written specifically to close the two blocking findings and one non-blocking finding from that review: an untested "closures swapped at the call site" risk, an untested "module list unreadable, host still answers" path, and a stale docs/release-trust.md path inventory. All three are verified fixed below. No blocking findings; CI is fully green (24/24 checks).

Blocking

None

Non-blocking

• The fake-executable/PATH-prepend fixture idiom is now written out a fourth time. tests/e2e-cucumber/tests/e2e/examine_steps.rs's new machine_with_kfd_and_no_module_list (added in 5474d87) repeats the same "write a fake binary, splice it onto PATH via std::env::join_paths/split_paths" sequence as plant_gpu_fixtures, plant_machine_without_amdkfd_module, and machine_with_built_in_amdgpu. Raised in every prior round on this PR as a candidate for a shared helper; still unaddressed, still low-cost to leave as is.
  Confidence 70 · mechanical (reuse) · Fix: extract a fn prepend_fake_bin_to_path(world, name, script) helper now that the pattern recurs a fourth time.

Decisions for the author

• kfd presence alone is sufficient to imply amdgpu_loaded = true, without independently confirming /sys/bus/pci/drivers/amdgpu — tradeoff (unchanged from an earlier round, still present and still the deliberate, documented choice in amdgpu_implied_by_host's doc comment). A mount namespace that still exposes a stale /dev/kfd node after the host's amdgpu driver has actually unbound would make amdgpu_loaded report true when the PCI driver is genuinely gone. Worth re-confirming this residual risk is acceptable versus the false-negative it replaces.
• Naming tradeoff, already made and documented — amdkfd_loaded is kept rather than split into e.g. kfd_functional, which issue #448 suggested as an alternative. Stated explicitly in the PR body with its reasoning; noting only so the choice reads as deliberate.

Positive signals

• driver_states_answer_each_field_from_its_own_host_signal (new in 672fcf4, crates/rocm-core/src/examine.rs) directly closes the swap-detection gap from the last round: its pci_only/unread_pci_only cases assert (Some(true), Some(false))/(Some(true), None) — an asymmetric pair that a swapped driver_states tuple, or a swapped destructuring at the probe_modules call site, would actually fail. I traced the logic by hand (amdgpu_present_on_host/kfd_present_on_host/module_or_host_state against each planted-root case) and confirmed every asserted value is correct.
• The new e2e scenario examine-33 (5474d87) plants /dev/kfd with no readable module list at all (no /proc/modules, a failing fake lsmod) and asserts both amdkfd_loaded and amdgpu_loaded come back true through examine --json — this is real CLI-observable coverage for the "list unreadable, host answers" path the prior round flagged as unit-test-only.
• docs/release-trust.md and the .github/workflows/ci.yml planted-root coverage comment (e0385fe) were both updated to list /sys/class/kfd/kfd and /sys/bus/pci/drivers/amdgpu, matching what hardware_root.rs's own doc comment already enumerated — no drift between the three restatements of this invariant anymore.
• The PR description's Tests section is now byte-accurate against the shipped code (function names, scenario IDs examine-30..examine-33) — a genuine improvement over an earlier round in this PR's history where that section had drifted from a mid-PR renumbering.

Deployment notes

Unchanged from prior rounds: on hosts where amdgpu/kfd are compiled into the kernel, rocm examine --json's amdgpu_loaded/amdkfd_loaded now report true where they previously reported false, and rocm diagnose stops emitting its DKMS/module-not-loaded/Secure-Boot evidence lines for such hosts. Field names and types are unchanged (schema-compatible).

What this covered

Fetched the PR head read-only (git fetch origin pull/694/head, never checked out) and diffed a9e9aa21 (merge-base with main) through the current head e0385fe8. The prior nowycondro review at bc2937d8 is attributable, covered the rest of this file set in full at a head that differs from the current one only by these 3 commits, so I narrowed to the delta: git diff --stat bc2937d8..e0385fe8 shows only crates/rocm-core/src/examine.rs, .github/workflows/ci.yml, docs/release-trust.md, tests/e2e-cucumber/features/examine.feature, and tests/e2e-cucumber/tests/e2e/examine_steps.rs touched; hardware_root.rs is untouched by this delta and was not re-read. Read the full diff for each of those 5 files, the surrounding functions it touches in examine.rs (driver_states, module_or_host_state, probe_modules, all four host-presence helpers and their doc comments), and the full current field doc comments on amdgpu_loaded/amdkfd_loaded. The delta is small (147 lines) and each of the three commits maps one-to-one onto a specific prior finding, so this was done as a single direct pass rather than a fan-out (per the method's size threshold), not because the change was large.

Read issue #448 directly (gh issue view) — the PR's scope (fixing amdkfd_loaded, and disclosing the deliberate extension to amdgpu_loaded) still matches it. Read the PR description after settling findings; it is accurate against the current head, including the previously-stale Tests section.

Did not run: cargo test/cargo clippy locally (no Rust toolchain in this environment) — relied on this PR's own CI instead, where all 24 checks report pass on the current head (gh pr checks 694), re-confirmed live immediately before publishing. History pass beyond this PR's own 14-commit sequence, and a separate "prior changes on these files" search, were not run.

Not reconciled against any discussion on this PR, including the existing review thread with inline comments from tomastola and the prior nowycondro reviews — this review was produced blind to that discussion by design.

[tomastola]

Thanks for the review of e0385fe.

• Fake-executable/PATH fixture repeated four times: fixed in a3911ce. put_fake_executables_on_path now writes the fake binaries and puts them on PATH for all four fixtures. examine-23, -28 and -30 to -33 pass locally.
• kfd alone implies amdgpu_loaded: kept as is. /dev/kfd exists only while amdgpu is bound, so a stale node left after an unbind is the narrower risk. The false false it replaces affected every host with the driver built in.
• amdkfd_loaded naming: kept as is, for the reasons given in the description.

[nowycondro]

🔴 Automated review · pr-review-watcher · 30b69f5

Review — no blocking findings

Full review of the whole change. Implements and closes #448 (the reported amdkfd_loaded false-negative), and extends the same fix to amdgpu_loaded's analogous built-in-driver blind spot, which the description explains but which was not literally asked for in the issue.

Blocking

None

Non-blocking

None

Decisions for the author

None

Positive signals

• module_or_host_state/driver_states cleanly separate "which signal answers which field" from "how the wiring reads it", so probe_modules_from tests can verify the wiring (host root consulted, right signal to right field) independently of the pure-function policy tests — this is what let a subtle bug class (e.g. swapping which host check backs which field) get its own dedicated test (driver_states_answer_each_field_from_its_own_host_signal) rather than being an untested implicit property.
• The e2e scenario examine-33 plants a ROCm install and Secure Boot-enabled mokutil specifically so the three rocm diagnose evidence lines this bug used to produce are reachable, then asserts none of them fire — a real regression guard against the exact downstream symptom in #448, not just a probe-level unit test.
• Documentation of the change is unusually thorough and consistent across layers: field doc comments in examine.rs, the hardware_root.rs module doc listing newly-routed paths, docs/release-trust.md, and even the CI workflow comment naming which tests cover which planted-root reads — all four were updated together and agree with each other.

Deployment notes

None. The two JSON fields keep their names and types (Option<bool>); examination_top_level_keys_match_examine_py_contract (the frozen wire-contract test) is unaffected since no field was added/removed/renamed.

What this covered

Read the full diff (d59f596…30b69f54, the 6 changed files: crates/rocm-core/src/examine.rs, crates/rocm-core/src/hardware_root.rs, docs/release-trust.md, .github/workflows/ci.yml, tests/e2e-cucumber/features/examine.feature, tests/e2e-cucumber/tests/e2e/examine_steps.rs) completely, plus the surrounding code it touches: all amdgpu_loaded/amdkfd_loaded call sites in diagnose.rs (check_3_rocm_kernel_unsupported, check_5_amdgpu_blacklisted), host_path/read_host_text/read_module_list, the probe_modules call site and its Linux-only gating, the pre-existing wire-contract tests, and prior git history on probe_modules (no earlier review comments on this region beyond the one re-rooting commit). Traced the module/host decision logic by hand against each new unit test and each new e2e scenario's planted fixtures (including the no-module-list/host-still-answers path, and that amdgpu alone does not imply kfd) and found the implementation matches what every test asserts. Checked both required test verdicts (would the test still pass with the change reverted; does each branch have a mutation that fails it) by reading the assertions against the production logic rather than by executing the suite — this PR's own description reports that it ran the mutations and the e2e scenarios, and statusCheckRollup shows build-and-test, Test (affected crates), clippy, E2E tests and E2E consolidated report all green at this head.

Did not run the test/clippy/e2e suite myself (reasoned from the code and from CI's own green result instead of re-deriving it, per the instruction not to check out the PR branch). Did not fan out to parallel sub-reviewers — the diff is small and concentrated enough (~620 lines across one core function plus its tests) for a single complete pass, consistent with scaling review effort to the size of the change. Agent-instruction-file pass: checked AGENTS.md's regression-test, e2e-scenario-for-user-observable-behavior, and docs-sync rules — all satisfied (grepped README.md/docs/testing.md/docs/manual-testing.md for the old field semantics; no stale mentions found). Not reconciled against any other reviewer's discussion on this PR.

[tomastola]

@jussielo-amd your blocking item was the "E2E tests (Strix Halo, WSL2, current)" lane. That lane now passes (on 6891205 it was this job, and it is green on the later heads too).

I also rebased the branch onto current main (a2ac4d1), so it now picks up #779, which fixes the dash-doctor-report-is-reviewed-before-sending failure that kept the two Strix Halo Ubuntu lanes red. The diff against main is still the same six files, and the commits are unchanged apart from the rebase. The E2E lanes are re-running on the new head. Re-requesting your review.

[nowycondro]

🔴 Automated review · pr-review-watcher · a2ac4d1

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 — needs work

Full review of the whole change (85f0468a…a2ac4d1f). Implements the fix requested in #448 (amdkfd_loaded false-negative on a built-in kfd) and extends the same fix to the analogous amdgpu_loaded blind spot, which the issue did not report but is caused by the same root issue (a built-in driver never appears in lsmod).

Blocking

• CI is not green at the reviewed commit: the self-hosted hardware E2E workflow has not completed — workflow run E2E self-hosted (40868991686) for a2ac4d1f. Two jobs show conclusion: cancelled (E2E tests (Strix Halo, Ubuntu, release, current), E2E tests (Strix Halo, Ubuntu, nightly, current), both cancelled after ~45 minutes), one is in_progress (E2E tests (MI300X, current), running since 07:57 the next day) and one is still queued (E2E tests (W7900, current), queued since creation). The overall run's own status is queued. Everything in the main CI workflow (build-and-test, clippy, Test (affected crates), the mocked E2E tests, E2E consolidated report) is green, and the two cancellations look like self-hosted runner capacity/queueing rather than a regression from this change, but as of this review the hardware-backed E2E lanes have not actually validated the change end-to-end — exactly the validation this PR's own description says was not done locally ("Not run on real hardware").
  Confidence 85 · architectural · Fix: no code change; rerun the cancelled/stuck jobs (or get a maintainer to) and confirm green before merge.

Non-blocking

None

Decisions for the author

• Treating kfd-on-host as proof of amdgpu_loaded — tradeoff
  amdgpu_implied_by_host returns true from kfd_present_on_host() alone, without also requiring /sys/bus/pci/drivers/amdgpu. The PR body justifies this explicitly (kfd is linked into amdgpu, so a container passing through /dev/kfd without /sys/bus/pci visible must not report amdkfd up and amdgpu down) and ships a test (kfd_alone_on_the_host_implies_amdgpu) pinning it. It does mean a hypothetical host where /dev/kfd exists but the amdgpu driver has some other problem would report amdgpu_loaded: true and suppress the three diagnose evidence lines that depend on it being false. Worth the author confirming this is the intended trade (kfd cannot exist without amdgpu having initialized far enough to create it, so the scenario is believed impossible) rather than an oversight.

Positive signals

• The module-list check and the host fallback are unified behind one module_or_host_state helper, and driver_states pairs the two fields' checks — this makes "which host signal answers which field" independently testable (and tested) apart from a real lsmod, rather than duplicating the fallback logic per field.
• Test coverage is unusually rigorous for this kind of host-probing code: helper-level tests, a wiring-level test asserting probe_modules_from itself consults the host root (not just the helpers in isolation), and e2e scenarios reproducing the exact reported bug plus the no-module-list edge case. I verified the new tests, the driver_states/module_or_host_state logic, the e2e fixtures, and the diagnose.rs consumers of these two fields by reading the code; I could not execute cargo test/clippy myself (no Rust toolchain in this environment) and relied on the green CI workflow for that confirmation.

Deployment notes

None — the JSON schema (field names/types) is unchanged, as the PR states and examination_top_level_keys_match_examine_py_contract continues to assert.

What this covered

Every file in the diff read completely: crates/rocm-core/src/examine.rs (the new probe functions and their unit tests), crates/rocm-core/src/hardware_root.rs, docs/release-trust.md, .github/workflows/ci.yml (comment-only changes), tests/e2e-cucumber/features/examine.feature, and tests/e2e-cucumber/tests/e2e/examine_steps.rs, plus the surrounding code: diagnose.rs's three checks that read amdgpu_loaded, fix.rs's consumer, the Examination struct and its serialization tests, and the pre-existing e2e fixtures (plant_gpu_fixtures) to confirm this change doesn't flip their expectations. Read AGENTS.md for repo conventions. Single-context review (no sub-agent fan-out was used, given the change's size and tight coupling across the touched files) — not degraded, but noting no independent second pass was run.

Did not run: cargo test/clippy locally (no Rust toolchain available in this environment) — relied on the green CI workflow status for that; history/prior-changes passes were not run as separate passes (folded into the single-context read, which included git log on the PR branch). Not reconciled against any prior review discussion or comments on this PR — this review was reached blind to that discussion, per method.

[siloteemu]

🔴 Automated review · pr-review-watcher · a2ac4d1

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 — needs work

Full review of the whole change, prw-base...HEAD (16 commits, 6 files), read in full by three read-only workers plus the driver.
The description and the issue it fixes (#448, "amdkfd_loaded=false is a false negative when kfd is built into the kernel") are retrievable. The change implements that issue and also fixes amdgpu_loaded. The description says so, and it matches the diff.

Blocking

• [new] A broken driver-name literal leaves every added test green, so the module-list path of driver_states is unguarded — crates/rocm-core/src/examine.rs:1884-1890 (driver_states), tests/e2e-cucumber/features/examine.feature:471-484 (examine-31/32)

  • Unit tests: every added driver_states and probe_modules_from test passes a list that names neither driver (Some(&[]), None or NO_AMD_MODULES). The only tests that put amdgpu or amdkfd into a list call module_or_host_state directly, with the name passed in as an argument.
  • Mutation run: I changed the "amdgpu" and "amdkfd" literals inside driver_states to "amdgpuX" and "amdkfdX". All 115 examine::tests still passed (--features e2e-test-hooks, --test-threads=1).
  • E2E scenarios, by reading: I did not run them.
    • examine-31 and examine-32 plant an lsmod that lists amdgpu, but assert only amdkfd_loaded. In both, amdkfd is absent from the list and amdgpu is not asserted, so the amdgpu literal is never checked.
    • The "amdkfd" literal is not checked either: a typo there still reports kfd correctly in 31 (via /dev/kfd) and in 32 (no /dev/kfd).
    • The split_whitespace().next() parse in probe_modules_from is likewise reached only with lists naming neither driver (by reading; the parse-breaking mutation run was inconclusive).
  • Impact: this branch is the one that serves every machine where amdgpu or amdkfd really is a module. A regression there reports amdgpu_loaded=false, which rocm diagnose scores as a DKMS failure and a Secure Boot problem. The PR rewrote this code, so adding coverage is in scope.
  • Confidence 100 · logic · Fix: add a probe_modules_from or driver_states test that hands in a list naming amdgpu and amdkfd (with an empty planted root) and asserts both fields. Alternatively, assert amdgpu_loaded true in examine-31, whose planted lsmod already lists amdgpu.

• [new] A doc comment moved onto the wrong item by the last commit — crates/rocm-core/src/examine.rs:6585-6595

  • Commit a2ac4d1 inserted const NO_AMD_MODULES below the paragraph that begins "The wiring, not just the helpers: probe_modules_from must consult the host root…". Rustdoc now merges that paragraph and the const's own two-line doc onto the const. The const therefore documents amdgpu_loaded and the PCI directory, and probe_modules_reports_amdgpu_when_it_is_on_the_host has no doc at all. The sibling probe_modules_reports_amdkfd_when_kfd_is_on_the_host (about line 6768) is intact.
  • This PR caused the stale comment, so it is in scope.
  • Confidence 85 · mechanical · Fix: move the wiring paragraph onto the test fn and keep the const's own two lines on the const.

Non-blocking

• [new] Pre-existing unit-test race, now with more writers — crates/rocm-core/src/examine.rs:3467 (live_rocm_repo_paths_in_dir_skips_a_real_bak_file_on_disk), :3501 (merge_live_repo_paths_scans_every_family_dir_including_zypp)
  • Both read through host_path without taking HOST_ROOT_TEST_LOCK. Under plain cargo test -p rocm-core --lib --features e2e-test-hooks (threads in one process), a test that has set ROCM_CLI_TEST_HOST_ROOT re-roots their scan and they fail intermittently.
  • I observed 1 or 2 failures in most runs, on this PR's head and on prw-base alike. CI is green, probably because it uses nextest with one process per test. The PR raises the number of tests that set the env var from 4 to 10, so the window is larger than before, but the failure rate on this host was not measurably different.
  • Confidence 85 · mechanical · Fix: take HOST_ROOT_TEST_LOCK in those two tests, or in a shared guard.

Decisions for the author

• examine-32 passes with the fix reverted — tradeoff
  • It is a negative guard, not a regression test. It stops kfd_present_on_host and the module-or-host logic over-reporting true, which is worth having. The description and comments do not claim it proves the fix.
• examine-33 uses two commands in one When and two Thens — tradeoff
  • examine and diagnose are both run, the diagnose return code is asserted inside the When step, and the "inspection reports amdgpu as loaded" Then overlaps the diagnosis Then.
  • Combined, the scenario proves the user-facing consequence the issue is about. Split into two scenarios, each would carry a single reason to fail. Either is defensible.
• Detection mechanisms in scenario text — non-blocking-improvement
  • The Givens and titles "a machine whose kernel exposes kfd but lists no amdkfd module" and "…from the kernel's kfd node…" describe how the CLI decides rather than what the user sees. Per the bdd-scenarios rules (behavioural language), a phrasing such as "a machine whose compute driver is built into the kernel" with the planting detail left to comments would read better. The domain terms are acceptable.
• /dev/kfd alone implying amdgpu_loaded — tradeoff
  • This is what makes a container that passes /dev/kfd through without /sys/bus/pci report coherently. The cost is that a stale static /dev/kfd node on a host without the driver would suppress the diagnose "amdgpu missing" evidence. The description states this choice, so it is probably deliberate.

Positive signals

• Using /sys/bus/pci/drivers/amdgpu rather than /sys/module/amdgpu is well reasoned. The nomodeset and amdgpu.modeset=0 case is documented in the code, and a unit test plants /sys/module/amdgpu alone and asserts it does not count.
• module_or_host_state is tested with unreachable!() as the host probe, which proves the probe is skipped once the module list has answered.
• The examine-33 evidence phrases are verbatim copies of the three diagnose.rs strings (lines 835, 953, 961), and the scenario plants a ROCm install and Secure Boot so all three lines are reachable if the flag regresses.
• The new host reads are added consistently to hardware_root.rs, docs/release-trust.md and the CI comment, and the functions that comment names exist.

Deployment notes

None. The JSON schema is unchanged (field names and types are the same), and amdkfd_loaded and amdgpu_loaded can now be true where they were false.

What this covered

• Read: every file in the change completely, plus the consumers of amdgpu_loaded/amdkfd_loaded (diagnose.rs, fix.rs), host_gpu.rs, the e2e harness wiring for ROCM_CLI_TEST_HOST_ROOT, the root AGENTS.md and the BDD scenario-quality references. Range prw-base...a2ac4d1f.
• Run:
  • cargo test -p rocm-core --features e2e-test-hooks --lib examine::tests: 113 to 115 passed per run, with 0 to 2 failures that were the two racing tests above (also seen on prw-base).
  • The two failing tests pass when run alone.
  • Two mutation experiments on examine.rs were restored afterwards. The literal mutation stayed green. The parse mutation produced one failure in the_oldest_runtime_version_is_recorded, which I did not trace.
  • The e2e Cucumber scenarios (examine-31 to examine-34) were not run here; they were judged by reading.
  • Workspace-wide tests and clippy were not run here.
• Sign-off: check_signoff.py returned {"checked": 16, "missing": []} with exit 0, so all 16 commits carry a trailer. A separate CI check, "Commit signatures + sign-off", passed.
• CI (one gh pr view for this head):
  • Passing: unit, clippy, prek, hawkeye, lychee, Sphinx, skillscope, CodeQL, "E2E tests", "E2E tests (MI300X, current)", "E2E tests (Strix Halo, Windows, current)" and "E2E tests (Strix Halo, WSL2, current)".
  • Cancelled: "E2E tests (Strix Halo, Ubuntu, release, current)" and "E2E tests (Strix Halo, Ubuntu, nightly, current)". These did not run and are not a pass.
  • Queued: "E2E tests (W7900, current)".
• Did not run: the history pass found only a fixup-style commit trail (7a366fa is reversed by 7071210, bf58a98 is a renumber); every message is informative, so I raise nothing. I did not check the description against real hardware, which it says was not run either.
• Fan-out: three read-only workers (examine.rs code, e2e scenarios, source and instruction passes). All returned; none failed.
• Not reconciled against any PR discussion here.

[tomastola]

Thanks for the two reviews of a2ac4d1. Neither had inline threads, so I'm answering both here. I pushed 7e406af and 6c8661c.

Fixed: the module-list path of driver_states had no test (blocking, 09:40 review). The new test is probe_modules_reports_each_driver_its_module_list_names. It gives probe_modules_from three /proc/modules-style lists against an empty host root: one names both drivers, one names only amdgpu, one names only amdkfd. It asserts both fields for each list. I re-ran your mutations and every one now fails this test:

• "amdgpu" → "amdgpuX" in driver_states
• "amdkfd" → "amdkfdX" in driver_states
• split_whitespace().next() → .nth(1) in probe_modules_from. This one also fails the_oldest_runtime_version_is_recorded, the failure you didn't trace.

Fixed: the doc comment on the wrong item (blocking, 09:40 review). The wiring paragraph is back on probe_modules_reports_amdgpu_when_it_is_on_the_host. NO_AMD_MODULES keeps only its own two lines.

Fixed: the repo-scan test race (non-blocking, 09:40 review). live_rocm_repo_paths_in_dir_skips_a_real_bak_file_on_disk and merge_live_repo_paths_scans_every_family_dir_including_zypp now take HOST_ROOT_TEST_LOCK. Under plain cargo test -p rocm-core --features e2e-test-hooks --lib examine::tests, five runs in a row passed 116/116.

CI not complete on a2ac4d1 (blocking, 08:11 review). No code change. The push starts a fresh run on 6c8661c, including the self-hosted hardware lanes. I'll check it when the run finishes.

Decision: /dev/kfd alone implies amdgpu_loaded (both reviews). This is intended. kfd is created by amdgpu during its own init, so a live kfd interface means amdgpu got that far. The other choice would let a container that passes /dev/kfd through without /sys/bus/pci report amdkfd up and amdgpu down. A stale, statically created /dev/kfd on a host with no driver would hide the "amdgpu missing" evidence. I think that case is rare enough to accept.

Left as they are (09:40 review):

• examine-32 is a negative guard, as you note. It doesn't claim to prove the fix.
• examine-33 stays as one scenario on purpose. It checks the user-facing result the issue is about: a built-in driver must not produce diagnose evidence.
• Scenario wording: I kept "kfd" in the Givens because it is the domain term, and the issue is about kfd specifically. I can reword if a maintainer prefers.

[nowycondro]

🔴 Automated review · pr-review-watcher · 6c8661c

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 — needs work

Full review of the whole change (base 85f0468a0a6c35200c4c04e3d65b8c3b7807f316 → pinned head 6c8661c83638918aa5a3c0c79a4e408a5d1f6892), not narrowed. This is a fresh, independent pass at this exact head — I did not read the earlier nowycondro/other-reviewer rounds on this PR (several exist, at earlier commits up to a2ac4d1f; none exist yet at this pinned head).
Fixes #448 (the amdkfd_loaded false-negative when kfd is built into the kernel), and additionally fixes the analogous amdgpu_loaded false-negative for a built-in amdgpu — a scope extension the issue itself doesn't mention but which the PR description explains clearly and deliberately.

Blocking

• A built-in amdgpu/kfd driver is reported as "loaded" even with zero AMD GPUs present — crates/rocm-core/src/examine.rs:1842 (kfd_present_on_host), :1852 (amdgpu_present_on_host), :1860 (amdgpu_implied_by_host).
  amdgpu_present_on_host() treats the mere existence of /sys/bus/pci/drivers/amdgpu as proof the driver is loaded. Per the kernel's own driver-model documentation, that directory is created as soon as the driver registers with the bus (pci_register_driver()/driver_register()) — before any device match/bind happens; the bound-device symlink appears inside it only afterward (docs.kernel.org/PCI/sysfs-pci.html, kernel.org driver-model/binding.html). kfd_present_on_host() has the same shape of problem: the /dev/kfd character device is registered unconditionally during the amdkfd subsystem's init (kfd_chardev_init(), called from kfd_init()), independent of whether any compatible GPU has attached — only open() on it is gated on a live GPU node, not the node's existence (kfd_chardev.c/kfd_topology.c in ROCm/amdgpu).
  For a loadable module this was already true in principle but essentially never observed, because nothing modprobes amdgpu on hardware that doesn't match it. For a kernel that compiles the driver in statically (CONFIG_DRM_AMDGPU=y) — precisely the scenario this PR exists to detect — registration happens unconditionally at boot regardless of hardware, so every machine running that kernel, AMD GPU or not, would read amdgpu_loaded: true and amdkfd_loaded: true in rocm examine --json, with no way to tell "built in, GPU present" from "built in, no GPU at all." Nothing in probe_modules/examine() cross-checks this against has_amd_gpu/has_discrete_amd (computed earlier, from PCI/KFD-topology enumeration in summarise_gpu_categories) to catch it, and the new tests only plant the directory/node on an otherwise-empty simulated root — they don't exercise "driver present, no GPU."
  Confidence 85 · architectural · Fix: narrow the host signal to actual device binding rather than driver/subsystem presence — e.g. check for at least one PCI-address-shaped entry (symlink) inside /sys/bus/pci/drivers/amdgpu/, or cross-reference against a node in /sys/class/kfd/kfd/topology/nodes with an AMD vendor ID, rather than trusting directory/node existence alone.

Non-blocking

• CI comment miscounts the tests it names — .github/workflows/ci.yml:567.
  The new comment says the driver-presence reads are covered by "kfd_present_on_host, amdgpu_present_on_host, driver_states and the two probe_modules_reports_* tests," but three such tests exist at this head: probe_modules_reports_each_driver_its_module_list_names, probe_modules_reports_amdgpu_when_it_is_on_the_host, and probe_modules_reports_amdkfd_when_kfd_is_on_the_host (confirmed by grep -n "fn probe_modules_reports" crates/rocm-core/src/examine.rs at the pinned head). Purely a stale/miscounted comment — no functional effect.
  Confidence 90 · mechanical · Fix: say "three" (or name them), matching the file as it stands.

Decisions for the author

None

Positive signals

• The two HOST_ROOT_TEST_LOCK guards added to the pre-existing live_rocm_repo_paths_in_dir/merge_live_repo_paths tests correctly diagnose and close a real (if narrow) race: those tests read through crate::host_path, so a concurrently-running test that sets ROCM_CLI_TEST_HOST_ROOT (which this PR's new tests do) could otherwise re-root their temp-dir scan under a threaded cargo test — exactly the hazard the pre-existing CI comment at ci.yml already documents.
• module_or_host_state's short-circuit (modules.contains(&name) || on_host()) is paired with a test that passes || unreachable!() specifically to prove the host probe is skipped once the module list already answers — a precise, well-targeted test rather than a vague one.
• The doc comment rejecting /sys/module/amdgpu as a signal gives a specific, checkable technical reason (the kernel creates it at an early initcall, before amdgpu_init can bail out on nomodeset/amdgpu.modeset=0) rather than a hand-wave.

Deployment notes

• The JSON schema is unchanged (field names/types preserved, per the PR description) — this is a behavior fix, not a breaking API change.
• amdgpu_loaded/amdkfd_loaded will now read true more often on kernels that build these drivers in statically. That's the intended fix for real AMD hardware, but per the Blocking finding above it also applies with no AMD hardware at all on such kernels; downstream consumers of rocm examine --json/rocm diagnose --json (e.g. the rocm-doctor skill, rocm diagnose's own DKMS/Secure-Boot evidence checks) should be aware the field no longer reliably implies AMD hardware presence on a statically-linked kernel.

What this covered

Read the full diff (base…head, 6 files, +671/-26) completely: crates/rocm-core/src/examine.rs, crates/rocm-core/src/hardware_root.rs, docs/release-trust.md, .github/workflows/ci.yml, tests/e2e-cucumber/features/examine.feature, tests/e2e-cucumber/tests/e2e/examine_steps.rs — plus surrounding context: crates/rocm-core/src/diagnose.rs (the three e.amdgpu_loaded call sites), tests/e2e-cucumber/tests/e2e.rs (E2eWorld, run_rocm_with_scenario_env), and hardware_root.rs/test_env.rs in full.

Fan-out: spawned 2 of 3 intended independent workers in parallel (group 1: examine.rs+hardware_root.rs; group 2: examine.feature+examine_steps.rs); both returned, both reported no findings ≥70. A third worker (doc/CI-comment accuracy + git-history pass + the PCI-driver-directory design question) failed to spawn twice in a row on a harness-level "concurrent subagent limit reached" error (other, unrelated sessions were saturating the shared limit) — I ran that slice myself directly instead, so it was not independent; both findings above come from that slice. The CI-comment miscount was confirmed by direct grep, not reasoning. The PCI-driver-directory/kfd-chardev claim was checked via two web searches citing kernel.org/docs.kernel.org and the ROCm/amdgpu kernel driver source, not by compiling or running an actual kernel.

CI (read at face value via gh pr view 694 --json statusCheckRollup): all 24 checks report SUCCESS at this exact head, including "Test (affected crates)" and "E2E tests" — I did not re-derive what each job covers.

Did not run: no Rust/cargo toolchain was available in my sandbox (one sub-worker independently confirmed cargo is absent there too), so no test was compiled or executed directly by me; verification is from reading code/tests/doc-comments, CI's own reported results, and git history (git log on the PR's 17 commits and diagnose.rs's relevant lines showed nothing unresolved or contradictory). Agent-instruction adherence (this repo's AGENTS.md) was checked by me reading it directly rather than as a separate dedicated pass.

Not reconciled against any discussion on this PR — reached blind to review threads/comments, per method. The PR description and issue #448 were retrieved and read only after every finding above was settled.

[tomastola]

Thanks for the review of 6c8661c. It had no inline threads, so I'm replying here. I pushed 50224ab for it.

Fixed: the CI comment's test count (non-blocking). The comment in ci.yml now says "the three probe_modules_reports_* tests".

Not changed: "a built-in driver reads as loaded with no AMD GPU" (blocking). The kernel facts are right: a built-in amdgpu registers its PCI driver at boot, and /dev/kfd exists whether or not a GPU binds. But both fields are meant to report that the driver is present, not that a device is bound. Narrowing them to binding would give a wrong answer in the case these fields exist to diagnose.

• The fields have always meant presence. On the module-list path, which this PR doesn't change, modprobe amdgpu on a host with no matching GPU lists amdgpu in lsmod, and amdgpu_loaded reads true. With this PR, a built-in driver reads the same as a loaded module, which is the point of examine: amdkfd_loaded=false is a false negative when kfd is built into the kernel (no module in lsmod) #448. The field doc on amdgpu_loaded says so: the driver is "listed as a module, or registered as a PCI driver". "Is there an AMD GPU" is a separate question, and has_amd_gpu / has_discrete_amd answer it.
• Binding would make the diagnosis wrong. Every consumer in diagnose.rs (lines 832, 951 and 959) reads only amdgpu_loaded == Some(false), and each treats it as evidence that the module didn't load (DKMS failed against the kernel, a blacklist, or Secure Boot blocking an unsigned module). Take a host where amdgpu loaded but failed to bind, for example because firmware is missing. If the field tracked binding, it would read false, and rocm diagnose would blame DKMS or Secure Boot when neither is the cause.
• On a host with no AMD GPU, true has no effect. It only withholds evidence for module-load faults, and those can't apply on a host with nothing for the module to drive.

Telling "built in, with a GPU" apart from "built in, no GPU" means reading has_amd_gpu next to amdgpu_loaded. A field that combined the two would be a different field, and I'd rather not fold that into this fix.

[nowycondro]

🟢 Automated review · review-code · 50224ab

Review — no blocking findings

Full review of the whole change: crates/rocm-core/src/examine.rs, crates/rocm-core/src/hardware_root.rs, docs/release-trust.md, .github/workflows/ci.yml, tests/e2e-cucumber/features/examine.feature, tests/e2e-cucumber/tests/e2e/examine_steps.rs.
Implements and closes issue #448 (amdkfd_loaded false negative when kfd is built into the kernel), and extends the identical root cause to amdgpu_loaded's analogous blind spot — the PR body states that extension explicitly, so it reads as a disclosed scope choice rather than scope creep.

Blocking

None

Non-blocking

None

Decisions for the author

• amdgpu_loaded can now go true purely from a kfd signal, with no PCI-driver-directory evidence — tradeoff. In a container that bind-mounts /dev/kfd without exposing /sys/bus/pci, amdgpu_implied_by_host() reports amdgpu_loaded = true on the kfd disjunct alone, with nothing in that container's own view of the filesystem confirming a bound PCI driver. The code comment states this is deliberate (kfd is linked into amdgpu, so kfd running already implies amdgpu ran on the real host). That avoids the "kfd up, amdgpu down" inconsistency this PR set out to fix, at the cost of amdgpu_loaded no longer being strict evidence of a bound PCI driver when reached only through that path. Worth confirming this is the intended tradeoff for container/passthrough consumers of the field.
• amdkfd_loaded keeps its historical name though it no longer means "the amdkfd module is loaded" — tradeoff, already made and justified in the PR body (JSON-schema stability over splitting into e.g. kfd_available/amdkfd_module_loaded, the alternative issue #448 itself suggested). Reasonable; flagging only so the choice reads as deliberate rather than overlooked.

Positive signals

• Option<bool> tri-state modeling (unknown / confirmed-absent / confirmed-present) is preserved correctly everywhere the module list can fail to read, with dedicated tests distinguishing "list read and empty" (Some(false) possible) from "list unreadable" (stays None unless the host answers).
• Deliberate avoidance of /sys/module/amdgpu as a signal, since a built-in driver creates it at an early initcall even when amdgpu_init later bails under nomodeset/amdgpu.modeset=0 — documented in the doc comment and enforced by a test that plants exactly that directory alone and asserts it does not count.
• driver_states_answer_each_field_from_its_own_host_signal specifically guards against the two fields' host checks being swapped, rather than only checking each field in isolation.
• Clean 19-commit history, each commit independently meaningful (fix/test/docs/refactor/ci correctly labeled), converging through several review rounds to the current shared host_path-routed, lock-protected test design with no noise commits.

Deployment notes

Behavioral change, not just a bugfix: on hosts where amdgpu/kfd are compiled into the kernel, rocm examine --json's amdgpu_loaded/amdkfd_loaded will now report true where they previously reported false, and rocm diagnose will stop emitting its DKMS-failure / module-not-loaded / Secure-Boot evidence lines for such hosts. JSON field names and types are unchanged (confirmed against the Examination struct), so this is a value-level behavior change, not a schema change.

What this covered

Reviewed base 85f0468a0a6c35200c4c04e3d65b8c3b7807f316 → head 50224ab4f359280c0cf9a025615d8faac8d56522 (matches the PR's 19 commits exactly). All six changed files read completely, via two independent blind passes (core probe logic + its unit tests in examine.rs/hardware_root.rs; the e2e feature/step layer + the hardware_root.rs/docs/release-trust.md/ci.yml doc-consistency trio) plus a direct read of the full diff and history, cross-checked against every consumer of the changed fields (diagnose.rs, fix.rs, host_gpu.rs, apps/rocm, crates/rocm-dash-tui — none needed updating, confirmed by search) and against AGENTS.md's project conventions.

Did not run cargo test/cargo clippy locally in this sandbox; reasoned from the code and tests directly, and cross-checked against this PR's own CI, where every check (build-and-test, Test (affected crates), E2E tests, clippy, coverage) is green at the reviewed head. The "prior changes" pass (review comments on earlier, unrelated PRs touching these same files) was not separately run. Issue #448 was fetched directly by its identifier and checked against the diff and PR description; the description accurately scopes the change, including the deliberate extension beyond what #448 asked for.

Not reconciled against any discussion or prior review thread on this PR — produced blind to that discussion by design.