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

crates/rocm-core/src/diagnose.rs

@@ -727,7 +727,14 @@ 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(),
+        // Catalog-aligned, not state-derived: fix.rs's FixRecipe for this fix-id sets
+        // 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,
         verify: "lsmod | grep amdgpu && rocminfo | head -n 5".to_owned(),
@@ -2976,6 +2983,56 @@ 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();
+        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();

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 <id>` 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; `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
@@ -1712,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"
+        );
+    }
 }

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 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