fix(diagnose): align fix-5-amdgpu-load's needs_reboot with the FixRecipe catalog

[jussielo-amd]

Summary

• fix-5-amdgpu-load had two independently-maintained sources for needs_reboot: the static FixRecipe catalog (fix.rs, always true) and diagnose.rs's check_5_amdgpu_blacklisted (conditional on whether a modprobe blacklist entry was found). rocm diagnose and rocm fix fix-5-amdgpu-load could report different needs_reboot values for the same underlying condition.
• Root cause: check_5_amdgpu_blacklisted was the only checker in diagnose.rs that computed needs_reboot from live state instead of a hardcoded literal matching its catalog counterpart -- every other fix-id's Fix.needs_reboot is a fixed literal.
• Fix: make diagnose.rs's value unconditional true, matching the catalog and the pattern of every other checker. This follows the same direction PR fix(diagnose): unify remediation flag wording with rocm fix #413 used to resolve the identical drift class for fix-9-igpu-dgpu's auto_applicable.
• Added a structural regression guard (assert_needs_reboot_matches_the_catalog in fix.rs, mirroring the existing assert_plan_matches_the_catalog_copy) so any future drift between the two sources fails a test rather than only being visible by eyeballing rocm diagnose vs rocm fix output.
• Updated two comments that named this exact drift as a known, deliberately-deferred gap (fix.rs's format_flags doc comment, and an e2e test comment in diagnose_steps.rs), since it's now closed.

Fixes #418

Why unconditional true is correct for the no-blacklist path too

#418 asked for a decision on which value is semantically correct here, not just alignment between the two sources. This PR resolves it in favor of the catalog's static true, for three reasons:

1. It follows the same direction PR fix(diagnose): unify remediation flag wording with rocm fix #413 used for the identical drift class on fix-9-igpu-dgpu's auto_applicable -- diagnose.rs conforms to the fix.rs catalog, not the reverse.
2. Every other checker's Fix.needs_reboot in diagnose.rs is already a hardcoded literal matching its FixRecipe counterpart; fix-5 was the only one computing it from live state on just one side, which is what caused the drift.
3. fix.rs's catalog is a static, compile-time table with no access to live Examination state. Making it conditional to match the diagnose-side precision would mean adding live blacklist probing to the rocm fix <id> command path itself -- a materially bigger change than this focused drift fix.

The tradeoff this accepts: in the no-blacklist path, the remediation is just sudo modprobe amdgpu, which doesn't actually require a reboot, so the unconditional true over-warns in that one case. The dangerous direction -- rocm diagnose telling a user no reboot is needed while rocm fix says one is -- is exactly what this closes; the residual cost is limited to over-warning, never under-warning. This reasoning is now also captured as a code comment at the needs_reboot: true assignment in diagnose.rs.

Test coverage for the user-observable output change (AGENTS.md section 3)

This changes rocm diagnose's rendered flags: line for fix-5-amdgpu-load in the no-blacklist case, which is user-observable CLI output. It isn't covered by a Gherkin scenario, for the same reason tests/e2e-cucumber/features/diagnose.feature already documents for this whole file: rocm diagnose's top match depends on live host state (here, amdgpu_loaded / amdgpu_blacklisted_in), which the e2e lane doesn't control deterministically, so no scenario can pin a specific fix-id's exact flags: line through that surface. The catalog-side flags (via rocm fix <id> --dry-run) have no such dependency and are already covered deterministically by diagnose-20. Coverage for this fix-id's exact needs_reboot behavior in both the blacklisted and non-blacklisted cases therefore lives in the two new rocm-core unit tests instead, per the "state the gap, cover at another CI level" allowance.

Test plan

• Two new regression tests in diagnose.rs (fix_5_amdgpu_load_needs_reboot_matches_catalog_when_blacklisted / ..._when_not_blacklisted) pin Fix.needs_reboot, the catalog-parity check, and the exact rendered flags: line for both scenarios -- the second one is the one that would have failed under the old conditional logic.
• cargo test --workspace --all-targets --exclude e2e-cucumber -- all passing.
• cargo clippy -p rocm-core --all-targets -- -D warnings and cargo fmt -p rocm-core -- --check -- clean.

[Copilot]

Copilot review overview

🟡 Changes recommended

The no-blacklist path now incorrectly requires a reboot, and the claimed structural guard covers only one fix ID.

Review effort: Balanced
Findings: 1 · 2

Open (3)

• Unconditionally reports reboot required after runtime-only module load · New
• Parity test covers only amdgpu fixes, not all fix IDs · New
• Intra-doc links to test-only helpers break normal rustdoc builds · New

What changed in this PR

Aligns diagnose metadata for fix-5-amdgpu-load with the static fix catalog and adds regression checks.

Changes:

• Makes needs_reboot unconditional.
• Adds catalog-parity and rendering tests.
• Updates related comments.

File	Description
crates/​rocm-core/​src/​diagnose.rs	Changes reboot behavior and adds tests.
crates/​rocm-core/​src/​fix.rs	Adds a catalog-parity assertion helper.
tests/​e2e-cucumber/​tests/​e2e/​diagnose_steps.rs	Updates test commentary.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[siloteemu]

🔴 Automated review · pr-review-watcher · 0d96764

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.

Summary

Makes check_5_amdgpu_blacklisted's emitted needs_reboot unconditionally true so rocm diagnose stops disagreeing with the fix-5-amdgpu-load recipe in the fix.rs catalog, adds a #[cfg(test)] catalog-comparison helper and two regression tests, and refreshes two comments. No blocking findings. The change moves the answer in the safe direction — the dangerous case (diagnose telling a user no reboot is needed while rocm fix says one is) is exactly what is removed; the residual cost is over-warning in the merely-not-loaded case. Verified: ran the two new unit tests in rocm-core plus three mutations on a separate copy — reverting crates/rocm-core/src/diagnose.rs:730 to !blacklisted.is_empty() fails ..._when_not_blacklisted (and only that one, so ..._when_blacklisted guards the other directions, not this line); flipping the catalog recipe's needs_reboot to false fails both tests through the new helper; rewording the rendered reboot flag fails the exact-flags assertion — so all three assertions are load-bearing and the pair discriminates both directions of the flag. Also confirmed by reading: needs_reboot feeds only format_flags for display (diagnose.rs:2199, fix.rs:837) and never apply()'s gating, which keys off applies_on/auto_applicable only; all five checkers that set it now use a literal, so the PR's "matching every other checker's pattern" claim is accurate; and comparing every fix-id across both sources on needs_sudo/needs_reboot/needs_relogin/auto_applicable leaves no remaining disagreement, so "align X with Y" holds in every case, not just the tested ones. The full suite was not run here. No leaks and no prompt-injection content found in the diff or commit messages. Checks at review time: 19 success, 1 pending, 1 skipped, 0 failures — but two GPU end-to-end lanes (Strix Halo WSL2 and Strix Halo Ubuntu) have gone red on this same commit since, so treat the clean-CI half of that sentence as out of date. Those two lanes are intermittently red across this repository and nothing in this diff was attributed to them, but they are worth a look before merging. Blocking: 0 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• crates/rocm-core/src/diagnose.rs:730 — the new unconditional true sits beside a conditionally built command list that, in the non-blacklisted case, is just sudo modprobe amdgpu, for which no reboot is needed; nothing at the site says the value is deliberately catalog-aligned rather than state-derived. That is precisely the reasoning that produced the original conditional, so a future reader is invited to "optimise" it back. The rationale currently lives only in a test comment ~2270 lines away. One line at the assignment (// Catalog-aligned, not state-derived: the fix-5 recipe says true unconditionally.) would prevent the recurrence.
• Contributor rules require user-observable CLI output changes to be covered by a Gherkin scenario, or the PR text to say why not; this changes rocm diagnose's rendered flags line, ships unit-test coverage only, and never states the gap. The justification looks sound (the diagnose side needs a host with amdgpu unloaded, so it is not deterministically reproducible in a hardware-independent lane — unlike the catalog side, which diagnose-20 already pins via rocm fix preview), but it should be stated rather than left implicit.
• tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:443 — names assert_needs_reboot_matches_the_catalog in a sentence about "unit tests in diagnose.rs"; the helper is defined in crates/rocm-core/src/fix.rs:716 and only called from diagnose.rs. Reads as if it lives there.
• crates/rocm-core/src/diagnose.rs:3003 — the asserted flags literal is byte-identical to the one already asserted for fix-11-iommu at diagnose.rs:2974, so a wording change to format_flags now breaks two hand-maintained copies of the same string; a shared assertion helper would keep the per-fix-id pin without the duplication.

[juhovainio]

Copilot already reviewed this one and I re-checked its two still-open points against the surrounding code -- both hold up, so I won't repeat them: the no-blacklist path may now be factually claiming "requires reboot" when it isn't true, and the new assert_needs_reboot_matches_the_catalog helper only guards fix-5, not every fix-id (you've replied to both already, and the second point mirrors the existing assert_plan_matches_the_catalog_copy helper's same characteristic, so that one reads more like a known, accepted limitation than a gap).

Two more things worth resolving before merge:

Issue #418 asked for a decision, and I don't think this PR makes one. It says explicitly: "This needs a decision, not just picking one side to match the other" and "Decide which value is semantically correct for each case." What's shipped here aligns diagnose.rs to the catalog by precedent (matches #413, matches every other checker's literal) -- it doesn't argue that unconditional-true is actually correct for the no-blacklist path. The real reasoning only shows up reactively, in your reply to the Copilot comment, and even there it's framed as "a legitimate accuracy-vs-consistency call a maintainer may want to weigh in on" -- which is the issue's own question, still open. If consistency really is the right call here (plausible -- a static catalog can't do live blacklist probing without a much bigger change), it'd be worth saying that reasoning in the PR description or as a comment at the needs_reboot: true line itself, so the next person doesn't have to go digging through review comments to find out this was a deliberate tradeoff and not an oversight.

The output change here doesn't have e2e coverage or a stated reason why not. The rendered flags: line for fix-5 now says "requires reboot" in a case where it didn't before -- that's user-observable CLI output, which AGENTS.md asks to be covered by a Gherkin scenario, or to say explicitly in the PR text why it's unit-test-only. The two new diagnose.rs tests are solid, but they're not that, and the PR body doesn't explain the gap. One sentence added to the test plan (e.g. "not blacklisted" isn't a scenario the e2e lane can deterministically reproduce, so it's unit-tested only) would close this cleanly.

Smaller thing, not blocking: since diagnose.rs is now unconditionally true, the two new tests both pin the same value -- the "not blacklisted" one only proves the old conditional is gone, it doesn't independently exercise a different expected answer for that case the way #418's phrasing ("pin the expected value for both... cases") seems to have had in mind. Worth a beat if you're already touching this file for the decision above.

[jussielo-amd]

Addressed the two outstanding review threads in 84a4e3a:

• Why unconditional needs_reboot: true is correct for the no-blacklist path too (discussion): added a code comment at the assignment and a dedicated section in the PR description spelling out the tradeoff (catalog-conformance precedent from fix(diagnose): unify remediation flag wording with rocm fix #413, every other checker already using a hardcoded literal, and the catalog's static/compile-time nature) rather than leaving it only in an earlier review reply.
• Missing Gherkin scenario / explicit gap justification (discussion): added a PR description section citing AGENTS.md section 3, explaining the diagnose-side flags: line depends on live host state the e2e lane can't deterministically control (per diagnose.feature's own documented environment-dependence), unlike the catalog side which diagnose-20 already covers deterministically.
• Also fixed a wording inaccuracy in diagnose_steps.rs that attributed assert_needs_reboot_matches_the_catalog to diagnose.rs -- it's defined in fix.rs, only called from diagnose.rs's tests (flagged by the automated review).

Left both threads open for @juhovainio to confirm the reasoning holds.

[siloteemu]

🔴 Automated review · pr-review-watcher · cdfd1a4

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.

Summary

Makes check_5_amdgpu_blacklisted's needs_reboot an unconditional true so diagnose stops disagreeing with the static fix catalog, adds a shared catalog-parity assertion plus two unit tests, and refreshes three comments that named the old drift. No blocking findings. Verified: on a scratch copy I reverted the production line to !blacklisted.is_empty() — ..._when_not_blacklisted fails, ..._when_blacklisted still passes — and separately flipped the catalog's own needs_reboot to false, which makes both tests fail through the new parity helper and which no other test in the crate catches, so the guard is real in both directions; a clean rocm-core lib run is 413 passed, 0 failed; I also confirmed independently that all 24 fix-ids now agree on needs_reboot across both sources (the five trues line up exactly, everything else is catalog-false against a Fix::default() false), that fix-5-amdgpu-load was genuinely the only diagnose-side value derived from live state, that needs_reboot is never mutated after construction, and that no doc or e2e text was left stale by the change. The full workspace and end-to-end suites were not run here. Checks at review time: 1 skipped, 20 success, no failures. Blocking: 0 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• crates/rocm-core/src/fix.rs:716 — the new parity helper is invoked for exactly one fix-id (as is the older plan-parity helper), so the same drift class stays unguarded on the other 23; the reworked format_flags comment says so honestly, but a small fix.rs-side test asserting the five needs_reboot: true ids against the catalog would close it cheaply.
• crates/rocm-core/src/diagnose.rs:733 — "see assert_needs_reboot_matches_the_catalog below" points at a call site 2200 lines down; the function itself lives in fix.rs, so say fix.rs in the comment.
• crates/rocm-core/src/diagnose.rs:2993 — the shared helper hardcodes assert!(fix.needs_reboot) next to the catalog-parity call, so a deliberate future catalog flip to false would fail a test named ..._matches_catalog even though the two sources would then agree.
• crates/rocm-core/src/diagnose.rs:731 — "the plan above is just modprobe amdgpu" understates the no-blacklist plan, which also carries the Secure Boot note whose remedy does need a reboot; the residual over-warning is narrower than the comment concedes.

[siloteemu]

🔴 Automated review · pr-review-watcher · d046a96

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.

Summary

Makes check_5_amdgpu_blacklisted's needs_reboot an unconditional true so rocm diagnose stops disagreeing with the static fix catalog, adds a shared catalog-parity assertion plus two regression tests, adds a catalog-side guard pinning the known needs_reboot: true fix-id set, and refreshes the comments that named the old drift. No blocking findings. Verified: on a scratch copy I ran four separate mutations — reverting the production line to !blacklisted.is_empty() fails only ..._when_not_blacklisted, and fails inside the parity helper rather than the rendered-text assertion, so the named mechanism is the first to fire; flipping the catalog's own value for this fix-id fails all three tests; flipping both sides so they agree still fails, via the rendered flags string (see the first non-blocking item); and flipping a different catalog entry in each direction is caught by the new set guard alone, so that guard is non-vacuous and not a value compared against itself. A clean rocm-core library run is 414 passed, 0 failed. I independently confirmed the PR's load-bearing claims: every field on every checker that also exists on a catalog recipe is a hardcoded literal except this one, so the "only state-derived value" claim holds across all fields and not just needs_reboot; all 24 catalog ids have a diagnose-side counterpart and agree on needs_reboot today; needs_reboot is never mutated after construction; the named five-id set is exactly the catalog's true set and exactly diagnose's five true sites; the rewritten comment's account of the no-blacklist plan (modprobe alone, or modprobe plus a Secure Boot note) matches the code exactly; and no doc, feature file or other comment was left describing the old conditional behaviour. All four non-blocking items from the previous round were acted on — three fully, one partially (first item below). The full test suite and the end-to-end suite were not run here. Checks at review time: success=28, skipped=1, no failures, none pending. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• crates/rocm-core/src/diagnose.rs:3005 — the shared helper still pins the whole rendered flags string, including "requires reboot" and the unrelated manual-only wording, so a deliberate future catalog flip to false (with both sources agreeing) or a reword of the auto/manual phrasing fails a test named for catalog matching; mutation confirms this, and one line in the helper noting that it also pins the rendered text would stop the next reader being surprised.
• crates/rocm-core/src/diagnose.rs:613 vs crates/rocm-core/src/fix.rs:147 — fix-3-rocm-kernel's verify has already drifted between the two hand-maintained copies ("head -n 20" in diagnose, "head -n 5" in the catalog); pre-existing and outside this PR's scope, but it is a live instance of exactly the drift class being guarded here and worth a follow-up.
• PR text — the change alters rocm diagnose output on the no-blacklist path, no scenario in the feature files covers this fix-id's flags, and the harness has no way to inject examination state; the contributor rules ask the PR to name the covering scenario or say why none is needed rather than leave it unexplained.
• crates/rocm-core/src/diagnose.rs:730 — the comment argues the alignment but never names the actual source of the old mismatch: the catalog's plan for this id unconditionally includes the initramfs/dracut regeneration steps, which do warrant a reboot, while diagnose prunes them on the no-blacklist path; saying that would make the catalog's unconditional true self-evidently right instead of merely asserted.
• crates/rocm-core/src/fix.rs:1737 — the new set guard pins the catalog side only; diagnose's five matching true literals stay unguarded and rely on the test message prompting a manual look, which is honestly stated but still leaves the other 23 ids' parity resting on prose.

Addressed

[juhovainio]

Both points from my earlier review are resolved:

• The needs_reboot semantic question (#418 asked which value is correct, not just alignment) now has a reasoned answer, both as a code comment at the needs_reboot: true assignment and as a dedicated PR-body section: follows PR #413's precedent of diagnose.rs conforming to the fix.rs catalog, matches every other checker's hardcoded-literal pattern, and avoids adding live-state probing to the rocm fix path. The accepted cost (over-warn in the no-blacklist case, never under-warn) is stated plainly rather than left implicit.
• The missing-Gherkin-scenario point now has an explicit justification in the PR body: the flags: line depends on live host state that diagnose.feature already documents the e2e lane can't pin deterministically, with the two new rocm-core unit tests as the stated alternative coverage (one of which would have failed under the old conditional logic).

CI is green across the board. Approving.