From 1f5c82a3e9f006eb22261a8f3c3cb33c260dfea9 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 28 Sep 2026 07:52:19 +0000 Subject: [PATCH 1/4] fix(diagnose): align fix-5-amdgpu-load's needs_reboot with the FixRecipe catalog diagnose.rs computed needs_reboot conditionally on whether a modprobe blacklist entry was found, while the fix.rs catalog always said true for the same fix-id -- rocm diagnose and rocm fix could disagree for the same machine. Make diagnose's value unconditional, matching the catalog and every other checker's pattern, and add a structural guard plus regression tests pinning both the blacklisted and non-blacklisted cases (#418). Signed-off-by: Jussi Elo --- crates/rocm-core/src/diagnose.rs | 56 ++++++++++++++++++- crates/rocm-core/src/fix.rs | 30 ++++++++-- .../e2e-cucumber/tests/e2e/diagnose_steps.rs | 5 +- 3 files changed, 83 insertions(+), 8 deletions(-) diff --git a/crates/rocm-core/src/diagnose.rs b/crates/rocm-core/src/diagnose.rs index 6c92c9a49..cfe065371 100644 --- a/crates/rocm-core/src/diagnose.rs +++ b/crates/rocm-core/src/diagnose.rs @@ -727,7 +727,7 @@ fn check_5_amdgpu_blacklisted(e: &Examination, symptom: &str) -> Diagnosis { summary: "Remove amdgpu from any modprobe blacklist and load it.".to_owned(), commands, needs_sudo: true, - needs_reboot: !blacklisted.is_empty(), + needs_reboot: true, fix_id: "fix-5-amdgpu-load".to_owned(), auto_applicable: false, verify: "lsmod | grep amdgpu && rocminfo | head -n 5".to_owned(), @@ -2976,6 +2976,60 @@ mod tests { ); } + fn assert_fix_5_needs_reboot(e: &Examination) { + let report = diagnose(e, ""); + let hit = report + .matched + .iter() + .find(|d| d.id == "fix-5-amdgpu-load") + .expect("amdgpu-not-loaded should be diagnosed"); + let fix = hit.fix.as_ref().unwrap(); + assert!( + fix.needs_reboot, + "fix-5-amdgpu-load must report needs_reboot, matching the fix.rs catalog" + ); + crate::fix::assert_needs_reboot_matches_the_catalog("fix-5-amdgpu-load", fix.needs_reboot); + + let text = render_report_text(&report, report.matched.len()); + let lines: Vec<&str> = text.lines().collect(); + let id_line = lines + .iter() + .position(|l| l.trim_start() == "id: fix-5-amdgpu-load") + .expect("fix-5-amdgpu-load should appear in the rendered report"); + let flags_line = lines[id_line..] + .iter() + .find(|l| l.trim_start().starts_with("flags:")) + .expect("fix-5-amdgpu-load should have a flags: line"); + assert_eq!( + flags_line.trim_start(), + "flags: requires sudo, requires reboot, manual only (`rocm fix` will NOT run it automatically)", + "rendered flags: line for fix-5-amdgpu-load: {flags_line}" + ); + } + + #[test] + fn fix_5_amdgpu_load_needs_reboot_matches_catalog_when_blacklisted() { + let mut e = linux_base(); + e.amdgpu_loaded = Some(false); + e.amdgpu_blacklisted_in = vec!["/etc/modprobe.d/blacklist.conf".to_owned()]; + assert_fix_5_needs_reboot(&e); + } + + #[test] + fn fix_5_amdgpu_load_needs_reboot_matches_catalog_when_not_blacklisted() { + // `check_5_amdgpu_blacklisted` used to compute `needs_reboot` as + // `!blacklisted.is_empty()`, so this case (module simply not loaded, + // no blacklist entry involved) used to report `false` here while the + // `fix-5-amdgpu-load` FixRecipe catalog said `true` unconditionally -- + // the drift issue #418 fixed. Pin the now-unconditional `true` so a + // regression back to the conditional fails here rather than only + // being visible by eyeballing `rocm diagnose` vs `rocm fix` output. + let mut e = linux_base(); + e.amdgpu_loaded = Some(false); + assert!(e.amdgpu_blacklisted_in.is_empty()); + assert_fix_5_needs_reboot(&e); + } + #[test] fn arch_covered_is_negative_and_filtered() { let mut e = linux_base(); diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index ce46bd2b8..0daa1b1f1 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -704,6 +704,25 @@ pub(crate) fn assert_plan_matches_the_catalog_copy(fix_id: &str, commands: &[&st ); } +/// Assert that the `needs_reboot` a [`crate::diagnose::Fix`] carries matches +/// the catalog recipe's. +/// +/// Same hand-maintained-copies problem as [`assert_plan_matches_the_catalog_copy`], +/// for a single flag instead of the command block: `FixRecipe` and `Fix` set +/// `needs_reboot` independently, so a silent divergence means `rocm diagnose` +/// and `rocm fix ` tell a user different things about the same fix-id +/// (this is what happened with `fix-5-amdgpu-load` before it was closed here). +#[cfg(test)] +pub(crate) fn assert_needs_reboot_matches_the_catalog(fix_id: &str, needs_reboot: bool) { + let recipe = find_recipe(fix_id) + .unwrap_or_else(|| panic!("{fix_id}: no catalog recipe to compare against")); + assert_eq!( + recipe.needs_reboot, needs_reboot, + "{fix_id}: diagnose's needs_reboot has drifted from the catalog recipe \ + `rocm fix` reports; a user may see either surface for the same fix-id" + ); +} + fn find_recipe(fix_id: &str) -> Option<&'static FixRecipe> { RECIPES.iter().find(|r| r.fix_id == fix_id) } @@ -768,11 +787,12 @@ pub fn list_recipes() -> String { /// values render as the same text from either command. This only /// standardizes wording, not the underlying values: `FixRecipe` (fix.rs) and /// diagnose's `Fix` still supply those independently, so a fix-id's rendered -/// flags can still differ if the two disagree on a value (known example: -/// `fix-5-amdgpu-load`'s `needs_reboot`). Also out of scope: the bare -/// `rocm fix` catalog listing (`list_recipes`) describes the same -/// `auto_applicable` property with a separate, untouched AUTO/PRINT-ONLY -/// vocabulary. +/// flags can still differ if the two disagree on a value; see +/// [`assert_needs_reboot_matches_the_catalog`] and +/// [`assert_plan_matches_the_catalog_copy`] for the guards that catch that. +/// Also out of scope: the bare `rocm fix` catalog listing (`list_recipes`) +/// describes the same `auto_applicable` property with a separate, untouched +/// AUTO/PRINT-ONLY vocabulary. // These mirror the `FixRecipe`/`Fix` struct fields, where // `clippy::struct_excessive_bools` is already allowed workspace-wide; that // allow doesn't reach this free function's parameters, so diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs index fb6d2da6a..09a0ccbbc 100644 --- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs @@ -438,8 +438,9 @@ async fn assert_every_cause_has_flags(world: &mut E2eWorld) { // the always-on auto/manual marker) rather than a specific fix-id's exact // flags: the top match is environment-dependent, and a shared vocabulary // doesn't guarantee diagnose and the fix.rs catalog agree on the - // underlying values for a given fix-id (known drift: fix-5-amdgpu-load's - // needs_reboot). + // underlying values for a given fix-id -- unit tests in diagnose.rs pin + // per-fix-id values against the catalog (e.g. + // `assert_needs_reboot_matches_the_catalog`) for that. let causes = output.lines().filter(|l| l.contains("score=")).count(); assert!(causes > 0, "no scored causes to check:\n{output}"); let flag_lines: Vec<&str> = output From 0d967642ec10addc54c2c4c67e692dd4bf3a2437 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 28 Sep 2026 08:33:43 +0000 Subject: [PATCH 2/4] docs(fix): use code spans instead of broken intra-doc links format_flags's doc comment linked to two cfg(test)-only helper functions from a normally-compiled function, producing unresolved rustdoc links. Render them as code text, and reword to not imply blanket drift coverage for every fix-id -- these are targeted regression tests for the fix-ids that have one, not a general guard. Addresses review feedback on #450. Signed-off-by: Jussi Elo --- crates/rocm-core/src/fix.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index 0daa1b1f1..251e97fcc 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -787,12 +787,12 @@ pub fn list_recipes() -> String { /// values render as the same text from either command. This only /// standardizes wording, not the underlying values: `FixRecipe` (fix.rs) and /// diagnose's `Fix` still supply those independently, so a fix-id's rendered -/// flags can still differ if the two disagree on a value; see -/// [`assert_needs_reboot_matches_the_catalog`] and -/// [`assert_plan_matches_the_catalog_copy`] for the guards that catch that. -/// Also out of scope: the bare `rocm fix` catalog listing (`list_recipes`) -/// describes the same `auto_applicable` property with a separate, untouched -/// AUTO/PRINT-ONLY vocabulary. +/// flags can still differ if the two disagree on a value; `assert_needs_reboot_matches_the_catalog` +/// and `assert_plan_matches_the_catalog_copy` are targeted regression tests +/// that pin specific fix-ids against that drift, not a blanket guarantee for +/// every fix-id. Also out of scope: the bare `rocm fix` catalog listing +/// (`list_recipes`) describes the same `auto_applicable` property with a +/// separate, untouched AUTO/PRINT-ONLY vocabulary. // These mirror the `FixRecipe`/`Fix` struct fields, where // `clippy::struct_excessive_bools` is already allowed workspace-wide; that // allow doesn't reach this free function's parameters, so From cdfd1a43d5fe148c735a7daa7ca306b17b323424 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Tue, 29 Sep 2026 12:23:43 +0000 Subject: [PATCH 3/4] docs(diagnose): explain fix-5's catalog-aligned needs_reboot in code Reviewer asked for the accuracy tradeoff to be documented at the call site, not just in a review reply -- add a comment at the needs_reboot assignment, and correct diagnose_steps.rs's comment attributing assert_needs_reboot_matches_the_catalog to diagnose.rs (it's defined in fix.rs, only called from diagnose.rs's tests). Signed-off-by: Jussi Elo --- crates/rocm-core/src/diagnose.rs | 6 ++++++ tests/e2e-cucumber/tests/e2e/diagnose_steps.rs | 6 +++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/crates/rocm-core/src/diagnose.rs b/crates/rocm-core/src/diagnose.rs index cfe065371..64a26194e 100644 --- a/crates/rocm-core/src/diagnose.rs +++ b/crates/rocm-core/src/diagnose.rs @@ -727,6 +727,12 @@ fn check_5_amdgpu_blacklisted(e: &Examination, symptom: &str) -> Diagnosis { summary: "Remove amdgpu from any modprobe blacklist and load it.".to_owned(), commands, needs_sudo: true, + // Catalog-aligned, not state-derived: fix.rs's FixRecipe for this fix-id sets + // needs_reboot unconditionally, even on this no-blacklist path where the plan + // above is just `modprobe amdgpu`. Diagnose conforms to the catalog rather + // than the reverse (see assert_needs_reboot_matches_the_catalog below) so + // `rocm diagnose` and `rocm fix` never disagree; occasionally over-warning + // here is judged cheaper than the drift it replaces. needs_reboot: true, fix_id: "fix-5-amdgpu-load".to_owned(), auto_applicable: false, diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs index 09a0ccbbc..2c59e6de1 100644 --- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs @@ -438,9 +438,9 @@ async fn assert_every_cause_has_flags(world: &mut E2eWorld) { // the always-on auto/manual marker) rather than a specific fix-id's exact // flags: the top match is environment-dependent, and a shared vocabulary // doesn't guarantee diagnose and the fix.rs catalog agree on the - // underlying values for a given fix-id -- unit tests in diagnose.rs pin - // per-fix-id values against the catalog (e.g. - // `assert_needs_reboot_matches_the_catalog`) for that. + // underlying values for a given fix-id -- unit tests in diagnose.rs call + // fix.rs's `assert_needs_reboot_matches_the_catalog` to pin per-fix-id + // values against the catalog for that. let causes = output.lines().filter(|l| l.contains("score=")).count(); assert!(causes > 0, "no scored causes to check:\n{output}"); let flag_lines: Vec<&str> = output From d046a9641fb170c1866e6c9839da2755ac0ae7dd Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 30 Sep 2026 05:31:59 +0000 Subject: [PATCH 4/4] fix(diagnose): address non-blocking review nits on fix-5 needs_reboot Extend the catalog-parity guard beyond fix-5-amdgpu-load with a fix.rs-side test pinning the known needs_reboot:true set, drop a redundant hardcoded assert in the shared test helper, and correct two comments (one pointing at the wrong module, one overstating the no-blacklist over-warning by ignoring the Secure Boot sub-case). Signed-off-by: Jussi Elo --- crates/rocm-core/src/diagnose.rs | 15 ++++++--------- crates/rocm-core/src/fix.rs | 30 ++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/crates/rocm-core/src/diagnose.rs b/crates/rocm-core/src/diagnose.rs index 64a26194e..656aabb58 100644 --- a/crates/rocm-core/src/diagnose.rs +++ b/crates/rocm-core/src/diagnose.rs @@ -728,11 +728,12 @@ fn check_5_amdgpu_blacklisted(e: &Examination, symptom: &str) -> Diagnosis { commands, needs_sudo: true, // Catalog-aligned, not state-derived: fix.rs's FixRecipe for this fix-id sets - // needs_reboot unconditionally, even on this no-blacklist path where the plan - // above is just `modprobe amdgpu`. Diagnose conforms to the catalog rather - // than the reverse (see assert_needs_reboot_matches_the_catalog below) so - // `rocm diagnose` and `rocm fix` never disagree; occasionally over-warning - // here is judged cheaper than the drift it replaces. + // needs_reboot unconditionally. On this no-blacklist path the plan is just + // `modprobe amdgpu` (or that plus a Secure Boot signing note, whose own + // remedy can require a reboot) -- diagnose conforms to the catalog rather + // than the reverse (see fix.rs's assert_needs_reboot_matches_the_catalog) so + // `rocm diagnose` and `rocm fix` never disagree; over-warning is limited to + // the plain no-Secure-Boot sub-case, judged cheaper than the drift it replaces. needs_reboot: true, fix_id: "fix-5-amdgpu-load".to_owned(), auto_applicable: false, @@ -2990,10 +2991,6 @@ mod tests { .find(|d| d.id == "fix-5-amdgpu-load") .expect("amdgpu-not-loaded should be diagnosed"); let fix = hit.fix.as_ref().unwrap(); - assert!( - fix.needs_reboot, - "fix-5-amdgpu-load must report needs_reboot, matching the fix.rs catalog" - ); crate::fix::assert_needs_reboot_matches_the_catalog("fix-5-amdgpu-load", fix.needs_reboot); let text = render_report_text(&report, report.matched.len()); diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index 251e97fcc..acf22e88b 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -1732,4 +1732,34 @@ mod tests { ); } } + + #[test] + fn needs_reboot_true_fix_ids_match_the_known_set() { + // assert_needs_reboot_matches_the_catalog only ever runs for + // fix-5-amdgpu-load, so the other 23 fix-ids' hardcoded needs_reboot + // literals in diagnose.rs have no guard against drifting from the + // catalog. This doesn't reach into diagnose.rs, but it does catch an + // accidental catalog edit and pins the expected set by name so a + // deliberate catalog change forces a look at diagnose.rs's matching + // literals too. + let expected: std::collections::BTreeSet<&str> = [ + "fix-3-rocm-kernel", + "fix-5-amdgpu-load", + "fix-11-iommu", + "fix-12-installer", + "fix-14-adrenalin-too-old", + ] + .into_iter() + .collect(); + let actual: std::collections::BTreeSet<&str> = RECIPES + .iter() + .filter(|r| r.needs_reboot) + .map(|r| r.fix_id) + .collect(); + assert_eq!( + actual, expected, + "the catalog's needs_reboot:true set has changed -- update diagnose.rs's \ + matching literals and this test's expected set together" + ); + } }