From a7ab88d81e90b4ce7d2f8da0048de37501b67fcb Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Thu, 17 Sep 2026 12:03:51 +0000 Subject: [PATCH 1/9] fix(diagnose): unify remediation flag wording with rocm fix rocm fix and rocm diagnose each built the Flags:/flags: line with their own inline logic and different wording for the same concepts (e.g. "requires sudo" vs "sudo"), so the same fix-id read differently depending on which command surfaced it. diagnose's line also stayed silent on manual fixes instead of saying so explicitly. Extract a shared format_flags() in fix.rs and have both call sites use it, standardizing on fix.rs's clearer wording. Signed-off-by: Jussi Elo --- crates/rocm-core/src/diagnose.rs | 23 +++++--------- crates/rocm-core/src/fix.rs | 52 ++++++++++++++++++++++---------- 2 files changed, 43 insertions(+), 32 deletions(-) diff --git a/crates/rocm-core/src/diagnose.rs b/crates/rocm-core/src/diagnose.rs index 6ff9dcd74..0433f7909 100644 --- a/crates/rocm-core/src/diagnose.rs +++ b/crates/rocm-core/src/diagnose.rs @@ -2184,22 +2184,13 @@ pub fn render_report_text(report: &DiagnoseReport, top: usize) -> String { for c in &fix.commands { let _ = writeln!(out, " $ {c}"); } - let mut flags = Vec::new(); - if fix.needs_sudo { - flags.push("sudo"); - } - if fix.needs_reboot { - flags.push("reboot required"); - } - if fix.needs_relogin { - flags.push("re-login required"); - } - if fix.auto_applicable { - flags.push("rocm fix can run it"); - } - if !flags.is_empty() { - let _ = writeln!(out, " flags: {}", flags.join(", ")); - } + let flags = crate::fix::format_flags( + fix.needs_sudo, + fix.needs_reboot, + fix.needs_relogin, + fix.auto_applicable, + ); + let _ = writeln!(out, " flags: {}", flags.join(", ")); for n in &fix.notes { let _ = writeln!(out, " note: {n}"); } diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index 600cc543a..f54bf206a 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -762,6 +762,35 @@ pub fn list_recipes() -> String { out } +/// Canonical wording for a fix's remediation flags, shared by `rocm fix ` and +/// `rocm diagnose` so the same fix-id reads identically from either command. +// These mirror the `FixRecipe`/`Fix` struct fields (`struct_excessive_bools` is +// already allowed workspace-wide for that reason); this fn just forwards them. +#[allow(clippy::fn_params_excessive_bools)] +pub(crate) fn format_flags( + needs_sudo: bool, + needs_reboot: bool, + needs_relogin: bool, + auto_applicable: bool, +) -> Vec<&'static str> { + let mut flags = Vec::new(); + if needs_sudo { + flags.push("requires sudo"); + } + if needs_reboot { + flags.push("requires reboot"); + } + if needs_relogin { + flags.push("requires re-login"); + } + flags.push(if auto_applicable { + "rocm fix can run it" + } else { + "manual only (this command will NOT run it)" + }); + flags +} + fn print_recipe(r: &FixRecipe) { println!("Fix: {} -- {}", r.fix_id, r.title); println!("OS scope: {}", r.applies_on.join(", ")); @@ -772,22 +801,13 @@ fn print_recipe(r: &FixRecipe) { println!(" $ {c}"); } } - let mut flags = Vec::new(); - if r.needs_sudo { - flags.push("requires sudo"); - } - if r.needs_reboot { - flags.push("requires reboot"); - } - if r.needs_relogin { - flags.push("requires re-login"); - } - if !r.auto_applicable { - flags.push("manual only (this command will NOT run it)"); - } - if !flags.is_empty() { - println!("Flags: {}", flags.join(", ")); - } + let flags = format_flags( + r.needs_sudo, + r.needs_reboot, + r.needs_relogin, + r.auto_applicable, + ); + println!("Flags: {}", flags.join(", ")); for n in r.notes { println!("Note: {n}"); } From 4e31bd8c336c77390449cd21b21e6d6e1132c092 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Thu, 17 Sep 2026 12:52:46 +0000 Subject: [PATCH 2/9] test(fix): cover format_flags wording and the sudo+re-login combination Adds an exhaustive unit test over format_flags's 3 optional flags x 2 auto-applicable states, plus an e2e scenario (diagnose-20) proving fix-4-render-group's sudo+re-login+auto combination renders correctly through a real `rocm fix --dry-run` invocation, following diagnose-14's precedent for staying OS-ungated since print_recipe runs before the fix's own platform gate. Signed-off-by: Jussi Elo --- crates/rocm-core/src/fix.rs | 108 ++++++++++++++++++ tests/e2e-cucumber/features/diagnose.feature | 18 +++ .../e2e-cucumber/tests/e2e/diagnose_steps.rs | 41 +++++++ 3 files changed, 167 insertions(+) diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index f54bf206a..f7b804249 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -1664,4 +1664,112 @@ mod tests { assert_eq!(apply("#1", &FixOptions::default()), 2); assert_eq!(apply("bogus", &FixOptions::default()), 2); } + + #[test] + fn format_flags_covers_every_required_flag_combination_and_both_auto_states() { + // Exhaustive over the 3 optional flags (sudo/reboot/relogin) x both + // auto_applicable states, so a wording regression on any one flag, or + // on the always-present auto/manual marker, fails here rather than + // only being visible by eyeballing `rocm fix`/`rocm diagnose` output. + let cases: &[(bool, bool, bool, bool, &[&str])] = &[ + ( + false, + false, + false, + false, + &["manual only (this command will NOT run it)"], + ), + (false, false, false, true, &["rocm fix can run it"]), + ( + true, + false, + false, + false, + &[ + "requires sudo", + "manual only (this command will NOT run it)", + ], + ), + ( + true, + false, + false, + true, + &["requires sudo", "rocm fix can run it"], + ), + ( + false, + true, + false, + false, + &[ + "requires reboot", + "manual only (this command will NOT run it)", + ], + ), + ( + false, + true, + false, + true, + &["requires reboot", "rocm fix can run it"], + ), + ( + false, + false, + true, + false, + &[ + "requires re-login", + "manual only (this command will NOT run it)", + ], + ), + ( + false, + false, + true, + true, + &["requires re-login", "rocm fix can run it"], + ), + ( + true, + true, + true, + false, + &[ + "requires sudo", + "requires reboot", + "requires re-login", + "manual only (this command will NOT run it)", + ], + ), + ( + true, + true, + true, + true, + &[ + "requires sudo", + "requires reboot", + "requires re-login", + "rocm fix can run it", + ], + ), + ( + true, + false, + true, + true, + &["requires sudo", "requires re-login", "rocm fix can run it"], + ), + ]; + + for (needs_sudo, needs_reboot, needs_relogin, auto_applicable, expected) in cases { + let flags = format_flags(*needs_sudo, *needs_reboot, *needs_relogin, *auto_applicable); + assert_eq!( + flags, *expected, + "sudo={needs_sudo} reboot={needs_reboot} relogin={needs_relogin} auto={auto_applicable}" + ); + } + } } diff --git a/tests/e2e-cucumber/features/diagnose.feature b/tests/e2e-cucumber/features/diagnose.feature index d860d0324..7d1878454 100644 --- a/tests/e2e-cucumber/features/diagnose.feature +++ b/tests/e2e-cucumber/features/diagnose.feature @@ -46,6 +46,7 @@ Feature: Diagnosing failures and listing fixes When the user previews that fix without applying it Then the CLI describes what the fix would change And nothing on the machine is changed + And the preview states plainly that this fix is manual only @id:diagnose-fix-unknown-id-rejected Scenario: diagnose-06 - Asking for a fix the CLI does not know is refused clearly @@ -266,3 +267,20 @@ Feature: Diagnosing failures and listing fixes When the user asks the CLI to diagnose that machine Then the CLI refuses and explains that it could not reach that machine And no diagnosis of this machine is reported + + # diagnose-05 only proves the manual/zero-optional-flags wording, because + # PREVIEW_FIX_ID (fix-1-arch) needs none of sudo/reboot/re-login. The + # sudo+re-login combination only exists on a fix gated to bare-metal Linux + # (fix-4-render-group), so it needs its own scenario -- but, like + # diagnose-14, it is deliberately not OS-gated: `print_recipe` runs before + # the fix's own platform gate (see `apply` in fix.rs), so the Flags: text + # under test renders identically regardless of which lane runs it. The step + # asserts only that printed text, never the exit code -- `fix-4-render-group` + # gates its own dry-run on host state ($USER, `usermod`/`sudo` on PATH), so + # unlike PREVIEW_FIX_ID its exit code is not guaranteed to be 0 everywhere. + @id:diagnose-fix-preview-states-required-flags + Scenario: diagnose-20 - Previewing a fix that needs sudo and a re-login says so + Given a user who has chosen a fix that needs sudo and a re-login + When the user previews that fix without applying it + Then the preview states that the fix requires sudo and a re-login + And the preview states that the CLI can run it automatically diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs index 153257e86..f209ce4cd 100644 --- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs @@ -201,6 +201,11 @@ async fn user_chose_known_fix(world: &mut E2eWorld) { world.model_name = Some(PREVIEW_FIX_ID.to_string()); } +#[given("a user who has chosen a fix that needs sudo and a re-login")] +async fn user_chose_fix_needing_sudo_and_relogin(world: &mut E2eWorld) { + world.model_name = Some(COMMAND_FAILURE_FIX_ID.to_string()); +} + #[given("a user who names a fix the CLI does not offer")] async fn user_named_unknown_fix(world: &mut E2eWorld) { world.model_name = Some("fix-does-not-exist".to_string()); @@ -911,6 +916,42 @@ async fn assert_describes_change(world: &mut E2eWorld) { ); } +#[then("the preview states plainly that this fix is manual only")] +async fn assert_preview_states_manual_only(world: &mut E2eWorld) { + let output = world.cli_output.as_ref().expect("no fix preview output"); + assert!( + output.contains("Flags: manual only (this command will NOT run it)"), + "expected a bare manual-only Flags: line for {PREVIEW_FIX_ID}, with no \ + sudo/reboot/re-login flags ahead of it:\n{output}" + ); +} + +// Exercises COMMAND_FAILURE_FIX_ID (fix-4-render-group: needs_sudo + +// needs_relogin + auto_applicable), the only catalog entry that combines sudo, +// re-login, and AUTO in one recipe -- so it is the one place that can prove +// `format_flags` renders more than one optional flag, and the auto-applicable +// line, from a real `rocm fix --dry-run` invocation. Deliberately checked +// as one line, not three separate `contains`, so a regression that reordered +// the flags (e.g. put re-login before sudo) would also be caught. +#[then("the preview states that the fix requires sudo and a re-login")] +async fn assert_preview_states_sudo_and_relogin(world: &mut E2eWorld) { + let output = world.cli_output.as_ref().expect("no fix preview output"); + assert!( + output.contains("Flags: requires sudo, requires re-login,"), + "expected sudo and re-login flags, in that order, for \ + {COMMAND_FAILURE_FIX_ID}:\n{output}" + ); +} + +#[then("the preview states that the CLI can run it automatically")] +async fn assert_preview_states_auto_applicable(world: &mut E2eWorld) { + let output = world.cli_output.as_ref().expect("no fix preview output"); + assert!( + output.contains("rocm fix can run it"), + "expected the auto-applicable flag text for {COMMAND_FAILURE_FIX_ID}:\n{output}" + ); +} + #[then("nothing on the machine is changed")] async fn assert_no_mutation(world: &mut E2eWorld) { // A dry-run must not write MANAGED STATE. It may still create incidental From 98ee7433f2e24eaa6ead8ef901c8674297c2ceef Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Fri, 18 Sep 2026 07:46:59 +0000 Subject: [PATCH 3/9] fix(diagnose): match fix-9-igpu-dgpu auto_applicable to the fix.rs catalog check_9_igpu_dgpu_collision's Linux/else branch reported auto_applicable: false for fix-9-igpu-dgpu, but the fix.rs FixRecipe for that same fix-id already has auto_applicable: true and a real runner (run_hip_visible_devices). The diagnose report told users the fix was manual-only when `rocm fix fix-9-igpu-dgpu` could already apply it. Signed-off-by: Jussi Elo --- crates/rocm-core/src/diagnose.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/rocm-core/src/diagnose.rs b/crates/rocm-core/src/diagnose.rs index 0433f7909..ea948053f 100644 --- a/crates/rocm-core/src/diagnose.rs +++ b/crates/rocm-core/src/diagnose.rs @@ -1038,7 +1038,10 @@ fn check_9_igpu_dgpu_collision(e: &Examination, symptom: &str) -> Diagnosis { "# Persist in your shell rc or your launch script.".to_owned(), ], fix_id: "fix-9-igpu-dgpu".to_owned(), - auto_applicable: false, + // Matches the `fix-9-igpu-dgpu` FixRecipe in fix.rs (auto_applicable: + // true, runner: run_hip_visible_devices) -- `rocm fix` can already + // carry this out on Linux, so the report must not claim otherwise. + auto_applicable: true, verify: "HIP_VISIBLE_DEVICES=1 python -c \"import torch; print(torch.cuda.device_count())\"".to_owned(), notes: vec![note], ..Fix::default() From 6c34b87c0dc6dcfd8087eb1a83498a0524e90190 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Fri, 18 Sep 2026 07:47:22 +0000 Subject: [PATCH 4/9] fix(fix): name rocm fix explicitly in the manual-only flag wording "manual only (this command will NOT run it)" read fine from `rocm fix `, where "this command" is unambiguous, but the same string is now shared with `rocm diagnose`'s flags: line, where "this command" could be misread as diagnose itself. Name `rocm fix` explicitly so the wording is unambiguous regardless of which command surfaces it. Also replace format_flags's 11-case hand-written test table, which claimed to be exhaustive over sudo/reboot/relogin x auto_applicable but only covered 11 of the 16 combinations, with a loop over all 16. Signed-off-by: Jussi Elo --- crates/rocm-core/src/fix.rs | 133 ++++-------------- .../e2e-cucumber/tests/e2e/diagnose_steps.rs | 2 +- 2 files changed, 32 insertions(+), 103 deletions(-) diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index f7b804249..1a85d371d 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -786,7 +786,7 @@ pub(crate) fn format_flags( flags.push(if auto_applicable { "rocm fix can run it" } else { - "manual only (this command will NOT run it)" + "manual only (`rocm fix` will NOT run it automatically)" }); flags } @@ -1666,108 +1666,37 @@ mod tests { } #[test] - fn format_flags_covers_every_required_flag_combination_and_both_auto_states() { - // Exhaustive over the 3 optional flags (sudo/reboot/relogin) x both - // auto_applicable states, so a wording regression on any one flag, or - // on the always-present auto/manual marker, fails here rather than - // only being visible by eyeballing `rocm fix`/`rocm diagnose` output. - let cases: &[(bool, bool, bool, bool, &[&str])] = &[ - ( - false, - false, - false, - false, - &["manual only (this command will NOT run it)"], - ), - (false, false, false, true, &["rocm fix can run it"]), - ( - true, - false, - false, - false, - &[ - "requires sudo", - "manual only (this command will NOT run it)", - ], - ), - ( - true, - false, - false, - true, - &["requires sudo", "rocm fix can run it"], - ), - ( - false, - true, - false, - false, - &[ - "requires reboot", - "manual only (this command will NOT run it)", - ], - ), - ( - false, - true, - false, - true, - &["requires reboot", "rocm fix can run it"], - ), - ( - false, - false, - true, - false, - &[ - "requires re-login", - "manual only (this command will NOT run it)", - ], - ), - ( - false, - false, - true, - true, - &["requires re-login", "rocm fix can run it"], - ), - ( - true, - true, - true, - false, - &[ - "requires sudo", - "requires reboot", - "requires re-login", - "manual only (this command will NOT run it)", - ], - ), - ( - true, - true, - true, - true, - &[ - "requires sudo", - "requires reboot", - "requires re-login", - "rocm fix can run it", - ], - ), - ( - true, - false, - true, - true, - &["requires sudo", "requires re-login", "rocm fix can run it"], - ), - ]; - - for (needs_sudo, needs_reboot, needs_relogin, auto_applicable, expected) in cases { - let flags = format_flags(*needs_sudo, *needs_reboot, *needs_relogin, *auto_applicable); + fn format_flags_covers_every_flag_combination_and_both_auto_states() { + // Exhaustive over all 2^4 = 16 combinations of the 3 optional flags + // (sudo/reboot/relogin) x both auto_applicable states, so a wording + // regression on any one flag, or on the always-present auto/manual + // marker, fails here rather than only being visible by eyeballing + // `rocm fix`/`rocm diagnose` output. + for bits in 0..16u8 { + let needs_sudo = bits & 1 != 0; + let needs_reboot = bits & 2 != 0; + let needs_relogin = bits & 4 != 0; + let auto_applicable = bits & 8 != 0; + + let mut expected = Vec::new(); + if needs_sudo { + expected.push("requires sudo"); + } + if needs_reboot { + expected.push("requires reboot"); + } + if needs_relogin { + expected.push("requires re-login"); + } + expected.push(if auto_applicable { + "rocm fix can run it" + } else { + "manual only (`rocm fix` will NOT run it automatically)" + }); + + let flags = format_flags(needs_sudo, needs_reboot, needs_relogin, auto_applicable); assert_eq!( - flags, *expected, + flags, expected, "sudo={needs_sudo} reboot={needs_reboot} relogin={needs_relogin} auto={auto_applicable}" ); } diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs index f209ce4cd..a9e86c11b 100644 --- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs @@ -920,7 +920,7 @@ async fn assert_describes_change(world: &mut E2eWorld) { async fn assert_preview_states_manual_only(world: &mut E2eWorld) { let output = world.cli_output.as_ref().expect("no fix preview output"); assert!( - output.contains("Flags: manual only (this command will NOT run it)"), + output.contains("Flags: manual only (`rocm fix` will NOT run it automatically)"), "expected a bare manual-only Flags: line for {PREVIEW_FIX_ID}, with no \ sudo/reboot/re-login flags ahead of it:\n{output}" ); From 061238041841b4dfd03f7ea68fb08c61cd7f2651 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Fri, 18 Sep 2026 07:49:37 +0000 Subject: [PATCH 5/9] test(diagnose): cover the flags: line on the rocm diagnose surface diagnose-20 (from the prior commit) exercises format_flags's wording and sudo+re-login combination only through `rocm fix --dry-run`. Extend diagnose-01 with a step that asserts every reported cause in `rocm diagnose` output carries its own flags: line ending in the auto/manual marker, so the shared render_report_text call site gets direct coverage too. Assert the shape rather than a specific fix-id's exact flags, since the top-scoring cause is environment-dependent. Also add a comment on diagnose-05 clarifying it covers the `rocm fix --dry-run` surface, not this new `rocm diagnose` one. Signed-off-by: Jussi Elo --- tests/e2e-cucumber/features/diagnose.feature | 5 ++++ .../e2e-cucumber/tests/e2e/diagnose_steps.rs | 30 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/tests/e2e-cucumber/features/diagnose.feature b/tests/e2e-cucumber/features/diagnose.feature index 7d1878454..82e1111fb 100644 --- a/tests/e2e-cucumber/features/diagnose.feature +++ b/tests/e2e-cucumber/features/diagnose.feature @@ -20,6 +20,7 @@ Feature: Diagnosing failures and listing fixes When the user asks the CLI to diagnose that symptom Then the CLI reports a likely cause with a suggested fix And every reported cause comes with a command that applies it + And every reported cause states its remediation flags @id:diagnose-always-offers-a-way-forward Scenario: diagnose-02 - Diagnosing any failure always gives the user a way to escalate @@ -40,6 +41,10 @@ Feature: Diagnosing failures and listing fixes And each fix indicates whether the CLI can apply it automatically And the listing explains what those indicators mean + # This scenario exercises `rocm fix --dry-run` (print_recipe's `Flags:` + # line), not the `rocm diagnose` report itself -- see diagnose-01's "states + # its remediation flags" step for the equivalent `flags:` line on that + # surface. @id:diagnose-fix-dry-run-changes-nothing Scenario: diagnose-05 - Previewing a fix explains the change without making it Given a user who has chosen a known fix diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs index a9e86c11b..cbec23ad3 100644 --- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs @@ -426,6 +426,36 @@ async fn assert_every_cause_has_a_command(world: &mut E2eWorld) { ); } +#[then("every reported cause states its remediation flags")] +async fn assert_every_cause_has_flags(world: &mut E2eWorld) { + let output = world.cli_output.as_ref().expect("no diagnose output"); + // `flags:` is the line `render_report_text` builds from + // `crate::fix::format_flags` -- the same helper `rocm fix `'s `Flags:` + // line uses, so a fix-id reads identically from either command. This is the + // only scenario that exercises that line through the real `rocm diagnose` + // rendering surface rather than through `rocm fix --dry-run`. Assert + // the shape (present once per cause, ending in the always-on auto/manual + // marker) rather than a specific fix-id's exact flags, since the top match + // is environment-dependent. + let causes = output.lines().filter(|l| l.contains("score=")).count(); + let flag_lines: Vec<&str> = output + .lines() + .filter(|l| l.trim_start().starts_with("flags:")) + .collect(); + assert_eq!( + flag_lines.len(), + causes, + "each of the {causes} causes needs its own flags: line:\n{output}" + ); + for line in &flag_lines { + assert!( + line.contains("rocm fix can run it") + || line.contains("manual only (`rocm fix` will NOT run it automatically)"), + "expected the auto/manual marker on the flags: line:\n{line}" + ); + } +} + #[then("the listing explains what those indicators mean")] async fn assert_markers_explained(world: &mut E2eWorld) { let output = world.cli_output.as_ref().expect("no fix list output"); From ae49a8804ab9dcf02598a09bb2d926b435184f52 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Fri, 18 Sep 2026 08:11:37 +0000 Subject: [PATCH 6/9] test(diagnose): pin fix-9-igpu-dgpu auto_applicable regression Copilot flagged that the fix-9-igpu-dgpu auto_applicable change (false -> true on Linux) is a behavior fix, not text rendering only, and that no existing test would catch it reverting. Add a unit test asserting both the Fix struct field (drives --json output) and the rendered flags: line for fix-9-igpu-dgpu on Linux. Signed-off-by: Jussi Elo --- crates/rocm-core/src/diagnose.rs | 59 ++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/crates/rocm-core/src/diagnose.rs b/crates/rocm-core/src/diagnose.rs index ea948053f..245957865 100644 --- a/crates/rocm-core/src/diagnose.rs +++ b/crates/rocm-core/src/diagnose.rs @@ -2865,6 +2865,65 @@ mod tests { ); } + #[test] + fn fix_9_igpu_dgpu_is_auto_applicable_on_linux() { + // `check_9_igpu_dgpu_collision`'s Linux/else branch sets + // `auto_applicable: true` to match the `fix-9-igpu-dgpu` FixRecipe in + // fix.rs (`run_hip_visible_devices` already handles it on Linux). This + // is a behavioural change, not text-only: it flips both the `Fix` + // struct field that `rocm diagnose --json` serialises and the + // `flags:` line `render_report_text` prints. Pin it directly so a + // regression back to `false` (the pre-fix value) fails here instead of + // only being visible by eyeballing output. + let mut e = linux_base(); + e.has_apu = true; + e.has_discrete_amd = true; + e.gpus = vec![ + Gpu { + gfx_target: "gfx1103".to_owned(), + is_amd: true, + is_apu: Some(true), + ..Gpu::default() + }, + Gpu { + gfx_target: "gfx1100".to_owned(), + is_amd: true, + is_apu: Some(false), + ..Gpu::default() + }, + ]; + let report = diagnose(&e, "torch crashes with a segfault"); + let hit = report + .matched + .iter() + .find(|d| d.id == "fix-9-igpu-dgpu") + .expect("iGPU+dGPU collision should be diagnosed"); + let fix = hit.fix.as_ref().unwrap(); + assert!( + fix.auto_applicable, + "fix-9-igpu-dgpu must be auto_applicable on Linux, matching the fix.rs catalog" + ); + + 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-9-igpu-dgpu") + .expect("fix-9-igpu-dgpu should appear in the rendered report"); + let flags_line = lines[id_line..] + .iter() + .find(|l| l.trim_start().starts_with("flags:")) + .expect("fix-9-igpu-dgpu should have a flags: line"); + assert!( + flags_line.contains("rocm fix can run it"), + "rendered flags: line must say fix-9-igpu-dgpu is auto-applicable, not manual only: {flags_line}" + ); + assert!( + !flags_line.contains("manual only"), + "rendered flags: line must not claim fix-9-igpu-dgpu is manual only: {flags_line}" + ); + } + #[test] fn arch_covered_is_negative_and_filtered() { let mut e = linux_base(); From 8f7e2e089887a8bb4662649b6f4bcdab9830f280 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Mon, 21 Sep 2026 12:43:34 +0000 Subject: [PATCH 7/9] test(diagnose): close residual non-blocking review items - render_report_text's flags: line now has a unit-level pin: the existing fix-9 assertion is exact-match instead of contains, and a new fix-11-iommu test pins a fix whose optional flags (sudo+reboot, manual only) actually differ between the old and new wording, so a revert of the diagnose-surface call site fails deterministically. - diagnose_steps.rs's "every reported cause states its remediation flags" step now asserts causes > 0 itself, so it can't vacuously pass a diagnose run with zero causes if reused standalone. - diagnose-20's title now mentions the auto-applicable assertion it makes, not just the sudo/re-login one. Signed-off-by: Jussi Elo --- crates/rocm-core/src/diagnose.rs | 53 ++++++++++++++++--- tests/e2e-cucumber/features/diagnose.feature | 2 +- .../e2e-cucumber/tests/e2e/diagnose_steps.rs | 1 + 3 files changed, 49 insertions(+), 7 deletions(-) diff --git a/crates/rocm-core/src/diagnose.rs b/crates/rocm-core/src/diagnose.rs index 245957865..8017d29f5 100644 --- a/crates/rocm-core/src/diagnose.rs +++ b/crates/rocm-core/src/diagnose.rs @@ -2914,13 +2914,54 @@ mod tests { .iter() .find(|l| l.trim_start().starts_with("flags:")) .expect("fix-9-igpu-dgpu should have a flags: line"); - assert!( - flags_line.contains("rocm fix can run it"), - "rendered flags: line must say fix-9-igpu-dgpu is auto-applicable, not manual only: {flags_line}" + // Exact match, not `contains`: fix-9 carries only the auto flag, so a + // revert of the `render_report_text` call site to its pre-PR inline + // logic would still print a line containing "rocm fix can run it" and + // not "manual only" -- `contains` can't tell the two implementations + // apart. See `fix_11_iommu_rendered_flags_line_is_exact` for a fix-id + // whose optional flags actually differ between old and new wording. + assert_eq!( + flags_line.trim_start(), + "flags: rocm fix can run it", + "rendered flags: line for fix-9-igpu-dgpu: {flags_line}" ); - assert!( - !flags_line.contains("manual only"), - "rendered flags: line must not claim fix-9-igpu-dgpu is manual only: {flags_line}" + } + + #[test] + fn fix_11_iommu_rendered_flags_line_is_exact() { + // Companion to `fix_9_igpu_dgpu_is_auto_applicable_on_linux`: that test + // only pins a fix-id with just the auto flag set, which an exact-match + // assertion can't distinguish from the pre-PR `render_report_text` + // inline logic (both print "rocm fix can run it" for it). fix-11-iommu + // carries sudo+reboot+manual, so this pins the full comma-joined, + // reworded `flags:` line through the real render call site. + let mut e = linux_base(); + e.iommu_kernel_param = "on".to_owned(); + e.gpus = vec![ + Gpu { + is_amd: true, + ..Gpu::default() + }, + Gpu { + is_amd: true, + ..Gpu::default() + }, + ]; + let report = diagnose(&e, ""); + 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-11-iommu") + .expect("fix-11-iommu should appear in the rendered report"); + let flags_line = lines[id_line..] + .iter() + .find(|l| l.trim_start().starts_with("flags:")) + .expect("fix-11-iommu 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-11-iommu: {flags_line}" ); } diff --git a/tests/e2e-cucumber/features/diagnose.feature b/tests/e2e-cucumber/features/diagnose.feature index 82e1111fb..d33c22e74 100644 --- a/tests/e2e-cucumber/features/diagnose.feature +++ b/tests/e2e-cucumber/features/diagnose.feature @@ -284,7 +284,7 @@ Feature: Diagnosing failures and listing fixes # gates its own dry-run on host state ($USER, `usermod`/`sudo` on PATH), so # unlike PREVIEW_FIX_ID its exit code is not guaranteed to be 0 everywhere. @id:diagnose-fix-preview-states-required-flags - Scenario: diagnose-20 - Previewing a fix that needs sudo and a re-login says so + Scenario: diagnose-20 - Previewing a fix that needs sudo and a re-login says so, and that it's auto-applicable Given a user who has chosen a fix that needs sudo and a re-login When the user previews that fix without applying it Then the preview states that the fix requires sudo and a re-login diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs index cbec23ad3..4e217b32b 100644 --- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs @@ -438,6 +438,7 @@ async fn assert_every_cause_has_flags(world: &mut E2eWorld) { // marker) rather than a specific fix-id's exact flags, since the top match // is environment-dependent. 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 .lines() .filter(|l| l.trim_start().starts_with("flags:")) From 6836c4da434824b184adf2e30262bc5686e00be9 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Tue, 22 Sep 2026 05:16:58 +0000 Subject: [PATCH 8/9] docs(fix): sharpen scope of shared flag wording, warn fix-9's no-op case - format_flags's doc comment now states its "reads identically" claim is scoped to `rocm fix ` and `rocm diagnose` only, not the bare `rocm fix` catalog listing, which keeps its own separate AUTO/ PRINT-ONLY vocabulary for the same property. - The #[allow(fn_params_excessive_bools)] comment now names both that lint and the workspace-wide struct_excessive_bools allow explicitly, so the two reads as complementary rather than one making the other redundant. - fix-9-igpu-dgpu's diagnose-side note now carries the same --device-index caveat README already states for `rocm fix`: without it, the fix only prints the query and changes nothing, despite being marked AUTO. Pinned with an assertion on the existing note test. Signed-off-by: Jussi Elo --- crates/rocm-core/src/diagnose.rs | 11 +++++++++++ crates/rocm-core/src/fix.rs | 14 ++++++++++---- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/crates/rocm-core/src/diagnose.rs b/crates/rocm-core/src/diagnose.rs index 8017d29f5..a1ef94f23 100644 --- a/crates/rocm-core/src/diagnose.rs +++ b/crates/rocm-core/src/diagnose.rs @@ -1011,6 +1011,13 @@ fn check_9_igpu_dgpu_collision(e: &Examination, symptom: &str) -> Diagnosis { "Detected gfx targets: {gfx_targets:?}. Discrete GPU(s): {discrete_targets:?}; integrated APU(s): {apu_targets:?}. Pin HIP_VISIBLE_DEVICES to the discrete GPU — do not assume the higher-numbered gfx target is the dGPU (on RDNA3 the APU can be higher)." ) }; + // Marked auto_applicable below, but `rocm fix fix-9-igpu-dgpu` still needs + // --device-index to actually make the change: without it, both the Linux + // and Windows runners only print the query that finds the index and + // change nothing (see README's --device-index caveat). + let note = format!( + "{note} Without --device-index, `rocm fix` only prints this query and makes no change, despite being marked AUTO." + ); let fix = if e.os_family == "windows" { Fix { summary: "Pin the HIP runtime to the discrete GPU with HIP_VISIBLE_DEVICES so the iGPU is hidden.".to_owned(), @@ -2863,6 +2870,10 @@ mod tests { !note.contains("usually the higher-numbered"), "note must not repeat the old wrong gfx-number heuristic: {note}" ); + assert!( + note.contains("Without --device-index") && note.contains("despite being marked AUTO"), + "note must warn that fix-9 is a no-op without --device-index: {note}" + ); } #[test] diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index 1a85d371d..b7602fd7a 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -762,10 +762,16 @@ pub fn list_recipes() -> String { out } -/// Canonical wording for a fix's remediation flags, shared by `rocm fix ` and -/// `rocm diagnose` so the same fix-id reads identically from either command. -// These mirror the `FixRecipe`/`Fix` struct fields (`struct_excessive_bools` is -// already allowed workspace-wide for that reason); this fn just forwards them. +/// Canonical wording for a fix's remediation flags, shared by `rocm fix ` +/// and `rocm diagnose` so a fix-id's flags read identically whichever of +/// those two commands renders them. Scoped to those two only: 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 +// `clippy::fn_params_excessive_bools` is separately allowed below. #[allow(clippy::fn_params_excessive_bools)] pub(crate) fn format_flags( needs_sudo: bool, From c99aa6444d31dbd6c06fe420ad798205c5d0151b Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Tue, 22 Sep 2026 05:48:47 +0000 Subject: [PATCH 9/9] docs(fix): narrow format_flags's identical-output claim to wording only format_flags 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). The doc comment and the matching e2e step comment both overstated this as "reads identically from either command"; narrowed both to the actual guarantee. Signed-off-by: Jussi Elo --- crates/rocm-core/src/fix.rs | 8 ++++++-- tests/e2e-cucumber/tests/e2e/diagnose_steps.rs | 15 +++++++++------ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index b7602fd7a..ed32e2d12 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -763,8 +763,12 @@ pub fn list_recipes() -> String { } /// Canonical wording for a fix's remediation flags, shared by `rocm fix ` -/// and `rocm diagnose` so a fix-id's flags read identically whichever of -/// those two commands renders them. Scoped to those two only: the bare +/// and `rocm diagnose` so the same `(sudo, reboot, relogin, auto_applicable)` +/// 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. diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs index 4e217b32b..fb6d2da6a 100644 --- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs @@ -431,12 +431,15 @@ async fn assert_every_cause_has_flags(world: &mut E2eWorld) { let output = world.cli_output.as_ref().expect("no diagnose output"); // `flags:` is the line `render_report_text` builds from // `crate::fix::format_flags` -- the same helper `rocm fix `'s `Flags:` - // line uses, so a fix-id reads identically from either command. This is the - // only scenario that exercises that line through the real `rocm diagnose` - // rendering surface rather than through `rocm fix --dry-run`. Assert - // the shape (present once per cause, ending in the always-on auto/manual - // marker) rather than a specific fix-id's exact flags, since the top match - // is environment-dependent. + // line uses, so the same flag values render as the same text from either + // command. This is the only scenario that exercises that line through the + // real `rocm diagnose` rendering surface rather than through `rocm fix + // --dry-run`. Assert the shape (present once per cause, ending in + // 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). 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