From 00dd9bc9a117d2db957329d12c31427185ac881a Mon Sep 17 00:00:00 2001
From: Jussi Elo <jussi.elo@amd.com>
Date: Mon, 7 Sep 2026 10:37:03 +0000
Subject: [PATCH 01/11] fix(cli): route rocm fix failures to stderr, preserve
 log guard on exit

Failure explanations in `rocm fix` (declined confirmation, wrong-OS
refusal, subprocess failures) were printed to stdout despite the
command exiting non-zero, so a caller capturing only stdout on
success sees a clean-looking failure with no visible reason.

Separately, `rocm fix` was the only place in the codebase calling
std::process::exit directly, which skips main()'s _log_guard
destructor and can silently truncate the log on exactly the failure
paths where it matters most. Its exit code now flows back through
main()'s ordinary return via a FixExitCode marker error instead.

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
---
 apps/rocm/src/main.rs                         | 67 ++++++++++++++++++-
 crates/rocm-core/src/fix.rs                   | 28 ++++----
 .../e2e-cucumber/tests/e2e/diagnose_steps.rs  | 26 ++++---
 3 files changed, 96 insertions(+), 25 deletions(-)

diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs
index 08cbf8cf7..b9890ebdc 100644
--- a/apps/rocm/src/main.rs
+++ b/apps/rocm/src/main.rs
@@ -62,6 +62,7 @@ use std::fs;
 use std::io::{self, BufRead, Read, Write};
 use std::net::{TcpStream, ToSocketAddrs};
 use std::path::{Path, PathBuf};
+use std::process::ExitCode;
 #[cfg(not(windows))]
 use std::process::ExitStatus;
 use std::process::{Command as ProcessCommand, Stdio};
@@ -1120,7 +1121,43 @@ fn with_sigpipe_ignored<T>(f: impl FnOnce() -> T) -> T {
     f()
 }
 
-fn main() -> Result<()> {
+/// Marker error carrying `rocm fix`'s exit code back through `main()`'s
+/// ordinary return path, instead of calling `std::process::exit` mid-stack
+/// and skipping the `_log_guard` destructor held in `run()`.
+#[derive(Debug)]
+struct FixExitCode(i32);
+
+impl std::fmt::Display for FixExitCode {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "fix exited with code {}", self.0)
+    }
+}
+
+impl std::error::Error for FixExitCode {}
+
+fn main() -> ExitCode {
+    exit_code_for(run())
+}
+
+/// Maps `run()`'s result to a process exit code, unwrapping a `FixExitCode`
+/// to its carried code and otherwise reproducing the standard
+/// `Result<(), anyhow::Error>` `Termination` behavior (print the error to
+/// stderr, exit 1).
+fn exit_code_for(result: Result<()>) -> ExitCode {
+    match result {
+        Ok(()) => ExitCode::SUCCESS,
+        Err(e) => {
+            if let Some(FixExitCode(code)) = e.downcast_ref::<FixExitCode>() {
+                ExitCode::from(*code as u8)
+            } else {
+                eprintln!("Error: {e:?}");
+                ExitCode::FAILURE
+            }
+        }
+    }
+}
+
+fn run() -> Result<()> {
     reset_sigpipe();
 
     // Held for the whole process lifetime: dropping it flushes and stops the
@@ -2273,7 +2310,7 @@ fn fix(fix_id: Option<String>, yes: bool, dry_run: bool, device_index: Option<i6
     };
     let code = rocm_core::apply_fix(&fix_id, &opts);
     if code != 0 {
-        std::process::exit(code);
+        return Err(FixExitCode(code).into());
     }
     Ok(())
 }
@@ -19235,6 +19272,32 @@ fn treat_as_natural_language(args: &[String]) -> bool {
 
 #[cfg(test)]
 mod tests {
+    use std::process::ExitCode;
+
+    /// `Ok(())` must map to a clean exit so `rocm`'s successful commands don't
+    /// regress to a nonzero code.
+    #[test]
+    fn exit_code_for_ok_is_success() {
+        assert_eq!(super::exit_code_for(Ok(())), ExitCode::SUCCESS);
+    }
+
+    /// `fix()`'s marker error must carry its exact code through, since that
+    /// code (2/3/4/5) is part of `rocm fix`'s documented contract.
+    #[test]
+    fn exit_code_for_fix_exit_code_carries_the_code() {
+        let err = anyhow::Error::new(super::FixExitCode(3));
+        assert_eq!(super::exit_code_for(Err(err)), ExitCode::from(3));
+    }
+
+    /// Any other error must still fail with exit 1, matching what
+    /// `Result<(), anyhow::Error>`'s `Termination` impl already does today for
+    /// every subcommand other than `fix`.
+    #[test]
+    fn exit_code_for_generic_error_is_failure() {
+        let err = anyhow::anyhow!("boom");
+        assert_eq!(super::exit_code_for(Err(err)), ExitCode::FAILURE);
+    }
+
     /// A cache that has moved inside a directory uninstall already removes must
     /// not be reported as "not removed" — the note would be false.
     #[test]
diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs
index 2a11ba085..167fe4221 100644
--- a/crates/rocm-core/src/fix.rs
+++ b/crates/rocm-core/src/fix.rs
@@ -477,7 +477,7 @@ pub fn apply(fix_id: &str, opts: &FixOptions) -> i32 {
 
     let os = current_os();
     if !recipe.applies_on.contains(&os) {
-        println!(
+        eprintln!(
             "This fix only applies on: {}. Running OS is: {os}.",
             recipe.applies_on.join(", ")
         );
@@ -510,7 +510,7 @@ fn confirm(prompt: &str, assume_yes: bool) -> bool {
         return true;
     }
     if !std::io::stdin().is_terminal() {
-        println!("Non-interactive shell and --yes not passed; refusing to apply.");
+        eprintln!("Non-interactive shell and --yes not passed; refusing to apply.");
         return false;
     }
     print!("{prompt} [y/N]: ");
@@ -565,16 +565,16 @@ fn run_render_group(opts: &FixOptions) -> i32 {
         .or_else(|_| std::env::var("LOGNAME"))
         .unwrap_or_default();
     if user.is_empty() {
-        println!("Could not determine current user from $USER/$LOGNAME.");
+        eprintln!("Could not determine current user from $USER/$LOGNAME.");
         return 3;
     }
     if !which("usermod") {
-        println!("`usermod` not on PATH; cannot add groups.");
+        eprintln!("`usermod` not on PATH; cannot add groups.");
         return 3;
     }
     let root = is_root();
     if !which("sudo") && !root {
-        println!("`sudo` is not on PATH and we are not root; cannot add groups.");
+        eprintln!("`sudo` is not on PATH and we are not root; cannot add groups.");
         return 3;
     }
     let (program, args): (&str, Vec<String>) = if root {
@@ -612,7 +612,7 @@ fn run_render_group(opts: &FixOptions) -> i32 {
     print!("{out}");
     eprint!("{err}");
     if rc != 0 {
-        println!("usermod exited {rc}; group membership NOT changed.");
+        eprintln!("usermod exited {rc}; group membership NOT changed.");
         return 4;
     }
     println!("Added {user} to render,video.");
@@ -710,7 +710,7 @@ fn run_unset_override_windows(opts: &FixOptions) -> i32 {
             print!("{out}");
             eprint!("{err}");
             if rc != 0 {
-                println!("setx exited {rc}; User scope NOT changed.");
+                eprintln!("setx exited {rc}; User scope NOT changed.");
                 return 4;
             }
             println!("Cleared from User scope. Reopen your terminal for it to take effect.");
@@ -757,7 +757,7 @@ fn run_path_export_linux(opts: &FixOptions) -> i32 {
     let bin_dir_owned = bin_path.to_string_lossy().into_owned();
     let bin_dir = bin_dir_owned.as_str();
     let Some(rc_file) = shell_rc_file() else {
-        println!("Could not determine your home directory.");
+        eprintln!("Could not determine your home directory.");
         return 3;
     };
     let export_line = format!("export PATH=\"{bin_dir}:$PATH\"");
@@ -786,7 +786,7 @@ fn run_path_export_linux(opts: &FixOptions) -> i32 {
         "# Added by rocm examine (fix-6-path)",
         &export_line,
     ) {
-        println!("Failed to write {}: {exc}", rc_file.display());
+        eprintln!("Failed to write {}: {exc}", rc_file.display());
         return 4;
     }
     println!(
@@ -838,7 +838,7 @@ fn run_path_export_windows(opts: &FixOptions) -> i32 {
     print!("{out}");
     eprint!("{err}");
     if rc != 0 {
-        println!("setx exited {rc}; User PATH NOT changed.");
+        eprintln!("setx exited {rc}; User PATH NOT changed.");
         return 4;
     }
     println!(
@@ -850,7 +850,7 @@ fn run_path_export_windows(opts: &FixOptions) -> i32 {
 /// fix-9: persist HIP_VISIBLE_DEVICES so the iGPU is hidden.
 fn run_hip_visible_devices(opts: &FixOptions) -> i32 {
     if let Some(idx) = opts.device_index.filter(|&i| i < 0) {
-        println!("--device-index must be >= 0 (got {idx}).");
+        eprintln!("--device-index must be >= 0 (got {idx}).");
         return 3;
     }
     if runtime_is_windows() {
@@ -870,7 +870,7 @@ fn run_hip_visible_devices_linux(opts: &FixOptions) -> i32 {
         return 0;
     };
     let Some(rc_file) = shell_rc_file() else {
-        println!("Could not determine your home directory.");
+        eprintln!("Could not determine your home directory.");
         return 3;
     };
     let export_line = format!("export HIP_VISIBLE_DEVICES={idx}");
@@ -897,7 +897,7 @@ fn run_hip_visible_devices_linux(opts: &FixOptions) -> i32 {
         "# Added by rocm examine (fix-9-igpu-dgpu)",
         &export_line,
     ) {
-        println!("Failed to write {}: {exc}", rc_file.display());
+        eprintln!("Failed to write {}: {exc}", rc_file.display());
         return 4;
     }
     println!(
@@ -944,7 +944,7 @@ fn run_hip_visible_devices_windows(opts: &FixOptions) -> i32 {
     print!("{out}");
     eprint!("{err}");
     if rc != 0 {
-        println!("setx exited {rc}; HIP_VISIBLE_DEVICES NOT changed.");
+        eprintln!("setx exited {rc}; HIP_VISIBLE_DEVICES NOT changed.");
         return 4;
     }
     println!(
diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs
index 74272ed19..96347eb09 100644
--- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs
+++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs
@@ -480,10 +480,14 @@ async fn assert_inapplicable_fix_declined(world: &mut E2eWorld) {
         Some(3),
         "a fix that does not apply here should exit 3, distinct from 2/4/5"
     );
-    let output = world.cli_output.as_ref().expect("no fix output");
+    let combined = format!(
+        "{}{}",
+        world.cli_output.as_deref().unwrap_or(""),
+        world.cli_stderr.as_deref().unwrap_or("")
+    );
     assert!(
-        output.contains("This fix only applies on:"),
-        "the refusal must say which platforms the fix is for:\n{output}"
+        combined.contains("This fix only applies on:"),
+        "the refusal must say which platforms the fix is for:\n{combined}"
     );
 }
 
@@ -627,23 +631,27 @@ async fn assert_position_argument_corrected(world: &mut E2eWorld) {
 
 #[then("the CLI refuses and explains that it needs agreement")]
 async fn assert_refuses_without_agreement(world: &mut E2eWorld) {
-    let output = world.cli_output.as_ref().expect("no fix output");
+    let combined = format!(
+        "{}{}",
+        world.cli_output.as_deref().unwrap_or(""),
+        world.cli_stderr.as_deref().unwrap_or("")
+    );
     // The refusal has to say *why* and how to proceed. A bare non-zero exit
     // reads as a broken fix rather than a deliberate stop.
     assert!(
-        output.contains("--yes"),
-        "the refusal must name what to pass to proceed:\n{output}"
+        combined.contains("--yes"),
+        "the refusal must name what to pass to proceed:\n{combined}"
     );
     assert!(
-        output.contains("refusing to apply"),
-        "the refusal must say it did not apply the fix:\n{output}"
+        combined.contains("refusing to apply"),
+        "the refusal must say it did not apply the fix:\n{combined}"
     );
     // Distinct from the unknown-id refusal (2), so a script can tell "you did
     // not agree" apart from "no such fix".
     assert_eq!(
         world.cli_rc,
         Some(5),
-        "declining to apply is its own outcome, not an error:\n{output}"
+        "declining to apply is its own outcome, not an error:\n{combined}"
     );
 }
 

From 2c0d0ae230c1560f6dd2cf34dc611708cfa43194 Mon Sep 17 00:00:00 2001
From: Jussi Elo <jussi.elo@amd.com>
Date: Mon, 7 Sep 2026 11:48:53 +0000
Subject: [PATCH 02/11] fix(core): route two more rocm-fix failure paths to
 stderr

run_path_export_linux/windows had four failure-explanation println!
calls left over when this branch's other rocm-fix failure messages
moved to stderr; they return the same exit code (3) as sibling lines
in the same functions that were converted, so leaving them on stdout
broke the stream/exit-code consistency this change established.

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
---
 crates/rocm-core/src/fix.rs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs
index 167fe4221..9175558a5 100644
--- a/crates/rocm-core/src/fix.rs
+++ b/crates/rocm-core/src/fix.rs
@@ -743,12 +743,12 @@ fn run_path_export_linux(opts: &FixOptions) -> i32 {
     // Same resolver `examine` uses, so the line we append names the install the
     // report pointed at -- including a versioned root like /opt/rocm-6.4.1.
     let Some(install) = crate::discover_rocm_installs().into_iter().next() else {
-        println!("No ROCm install found; nothing to add to PATH.");
+        eprintln!("No ROCm install found; nothing to add to PATH.");
         return 3;
     };
     let bin_path = install.path.join("bin");
     if !bin_path.is_dir() {
-        println!(
+        eprintln!(
             "{} does not exist; nothing to add to PATH.",
             bin_path.display()
         );
@@ -803,12 +803,12 @@ fn run_path_export_windows(opts: &FixOptions) -> i32 {
         sdk_path = newest_rocm_install_dir();
     }
     if sdk_path.is_empty() {
-        println!("No HIP SDK install found. Run fix-13-hip-sdk-missing first.");
+        eprintln!("No HIP SDK install found. Run fix-13-hip-sdk-missing first.");
         return 3;
     }
     let bin_dir = Path::new(&sdk_path).join("bin");
     if !bin_dir.is_dir() {
-        println!(
+        eprintln!(
             "{} does not exist on disk; HIP SDK install looks incomplete.",
             bin_dir.display()
         );

From d99472a4c4529db9eaaef890af8030a88a9a0dc7 Mon Sep 17 00:00:00 2001
From: Jussi Elo <jussi.elo@amd.com>
Date: Mon, 7 Sep 2026 11:49:08 +0000
Subject: [PATCH 03/11] test(e2e): assert rocm-fix refusals land on stderr
 specifically

assert_inapplicable_fix_declined and assert_refuses_without_agreement
checked stdout+stderr concatenated, so they would still pass if the
refusal text regressed back onto stdout -- the one thing this PR's
stderr move needs to guarantee. Assert against cli_stderr alone.

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
---
 .../e2e-cucumber/tests/e2e/diagnose_steps.rs  | 26 +++++++------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs
index 96347eb09..6b3c9d63b 100644
--- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs
+++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs
@@ -480,14 +480,10 @@ async fn assert_inapplicable_fix_declined(world: &mut E2eWorld) {
         Some(3),
         "a fix that does not apply here should exit 3, distinct from 2/4/5"
     );
-    let combined = format!(
-        "{}{}",
-        world.cli_output.as_deref().unwrap_or(""),
-        world.cli_stderr.as_deref().unwrap_or("")
-    );
+    let stderr = world.cli_stderr.as_deref().unwrap_or("");
     assert!(
-        combined.contains("This fix only applies on:"),
-        "the refusal must say which platforms the fix is for:\n{combined}"
+        stderr.contains("This fix only applies on:"),
+        "the refusal must say which platforms the fix is for, on stderr:\n{stderr}"
     );
 }
 
@@ -631,27 +627,23 @@ async fn assert_position_argument_corrected(world: &mut E2eWorld) {
 
 #[then("the CLI refuses and explains that it needs agreement")]
 async fn assert_refuses_without_agreement(world: &mut E2eWorld) {
-    let combined = format!(
-        "{}{}",
-        world.cli_output.as_deref().unwrap_or(""),
-        world.cli_stderr.as_deref().unwrap_or("")
-    );
+    let stderr = world.cli_stderr.as_deref().unwrap_or("");
     // The refusal has to say *why* and how to proceed. A bare non-zero exit
     // reads as a broken fix rather than a deliberate stop.
     assert!(
-        combined.contains("--yes"),
-        "the refusal must name what to pass to proceed:\n{combined}"
+        stderr.contains("--yes"),
+        "the refusal must name what to pass to proceed, on stderr:\n{stderr}"
     );
     assert!(
-        combined.contains("refusing to apply"),
-        "the refusal must say it did not apply the fix:\n{combined}"
+        stderr.contains("refusing to apply"),
+        "the refusal must say it did not apply the fix, on stderr:\n{stderr}"
     );
     // Distinct from the unknown-id refusal (2), so a script can tell "you did
     // not agree" apart from "no such fix".
     assert_eq!(
         world.cli_rc,
         Some(5),
-        "declining to apply is its own outcome, not an error:\n{combined}"
+        "declining to apply is its own outcome, not an error:\n{stderr}"
     );
 }
 

From f437fdd6e2a7732a410e66943afdaf3c2dec3ae8 Mon Sep 17 00:00:00 2001
From: Jussi Elo <jussi.elo@amd.com>
Date: Mon, 7 Sep 2026 12:25:20 +0000
Subject: [PATCH 04/11] fix(cli): don't panic if stderr write fails in
 exit_code_for

eprintln! panics on a write failure (e.g. closed stderr pipe), which
would replace the intended exit code with a panic. Match std's
Termination behavior for Result<(), E> and ignore the write failure
instead.

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
---
 apps/rocm/src/main.rs | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs
index b9890ebdc..3027a98cd 100644
--- a/apps/rocm/src/main.rs
+++ b/apps/rocm/src/main.rs
@@ -1150,7 +1150,11 @@ fn exit_code_for(result: Result<()>) -> ExitCode {
             if let Some(FixExitCode(code)) = e.downcast_ref::<FixExitCode>() {
                 ExitCode::from(*code as u8)
             } else {
-                eprintln!("Error: {e:?}");
+                // Match the standard `Result<(), E>` `Termination` behavior exactly:
+                // ignore a failed write here rather than `eprintln!`, which panics.
+                // A caller with a closed stderr pipe must still see exit code 1, not
+                // a panic that replaces it.
+                let _ = writeln!(io::stderr(), "Error: {e:?}");
                 ExitCode::FAILURE
             }
         }

From 333a663adca7c7e9d0963255dfaf37d8bf91fa12 Mon Sep 17 00:00:00 2001
From: Jussi Elo <jussi.elo@amd.com>
Date: Mon, 7 Sep 2026 12:25:33 +0000
Subject: [PATCH 05/11] test(e2e): cover fix-4-render-group's command-failure
 branch

diagnose-08/-11 cover the refusal branches (no agreement, wrong OS),
but the third failure shape -- an approved, applicable fix whose
helper command itself fails -- had no e2e coverage. Add diagnose-13,
which forces usermod/sudo to fail via a scenario-scoped PATH override
and asserts the explanation lands on stderr with exit code 4.

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
---
 tests/e2e-cucumber/features/diagnose.feature  | 13 +++
 .../e2e-cucumber/tests/e2e/diagnose_steps.rs  | 79 +++++++++++++++++++
 2 files changed, 92 insertions(+)

diff --git a/tests/e2e-cucumber/features/diagnose.feature b/tests/e2e-cucumber/features/diagnose.feature
index f49bcd8df..eb50485ed 100644
--- a/tests/e2e-cucumber/features/diagnose.feature
+++ b/tests/e2e-cucumber/features/diagnose.feature
@@ -140,3 +140,16 @@ Feature: Diagnosing failures and listing fixes
     When the user asks the CLI which fixes it offers
     Then every fix the catalog documents is listed
     And only the fixes the CLI can carry out itself are marked as such
+
+  # diagnose-08/-11 cover the refusal branches (no agreement, wrong OS); this
+  # covers the third failure shape a fix can hit -- an approved, applicable fix
+  # whose underlying command itself fails (e.g. `usermod` exiting non-zero).
+  # Until now that branch of `fix-4-render-group` had no e2e coverage: a
+  # regression could move the explanation back to stdout, or off exit code 4,
+  # while every other listed scenario kept passing. Linux-only because the
+  # recipe itself is `applies_on: LINUX_ONLY`.
+  @id:diagnose-fix-command-failure-reported-on-stderr @requires-os:linux
+  Scenario: diagnose-13 - A fix whose helper command fails explains why, on stderr, with exit code 4
+    Given a user who has approved a fix whose helper command will fail
+    When the user asks the CLI to apply the approved fix
+    Then the CLI reports the command failure on stderr with exit code 4
diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs
index 6b3c9d63b..18e0a8e9c 100644
--- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs
+++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs
@@ -30,6 +30,15 @@ const PREVIEW_FIX_ID: &str = "fix-1-arch";
 /// found". This one needs only `--device-index`, which the scenario supplies.
 const MUTATING_FIX_ID: &str = "fix-9-igpu-dgpu";
 
+/// The recipe used to prove a failed helper command is explained on stderr with
+/// exit code 4. `fix-4-render-group` is the only AUTO recipe whose command-failure
+/// branch this suite can force deterministically: its helper (`usermod`, run
+/// directly as root or via `sudo` otherwise) is resolved off `$PATH`, so a
+/// scenario-controlled `$PATH` (see `command_fails_bin_dir`) can stand a fake
+/// `usermod`/`sudo` in for it, unconditionally failing, without needing real
+/// root or touching real group membership.
+const COMMAND_FAILURE_FIX_ID: &str = "fix-4-render-group";
+
 /// Every fix-id in the closed catalog, in the order `rocm fix` lists them.
 ///
 /// A duplicate of the catalog, on purpose: a test that derived this list from
@@ -105,6 +114,18 @@ fn fix_rc_file(world: &E2eWorld) -> std::path::PathBuf {
     fix_home(world).join(".bashrc")
 }
 
+/// The directory this scenario stands in for `$PATH`, holding the fake
+/// `usermod`/`sudo` scripts. Scoped under the scenario's isolated root so it is
+/// cleaned up with everything else.
+fn command_fails_bin_dir(world: &E2eWorld) -> std::path::PathBuf {
+    world
+        .isolated_root
+        .as_ref()
+        .expect("no isolated root")
+        .path()
+        .join("fake-bin")
+}
+
 // ── Given ──────────────────────────────────────────────────────────
 
 #[given("a user who hit a known ROCm failure")]
@@ -140,6 +161,34 @@ async fn user_chose_fix_for_another_os(world: &mut E2eWorld) {
     world.model_name = Some(fix_id_for_the_other_os().to_string());
 }
 
+#[given("a user who has approved a fix whose helper command will fail")]
+async fn user_approved_fix_that_will_fail(world: &mut E2eWorld) {
+    let bin_dir = command_fails_bin_dir(world);
+    std::fs::create_dir_all(&bin_dir).expect("failed to create the scenario's fake PATH dir");
+    // `usermod` only has to exist for the `which` probe; the command actually run
+    // -- `usermod` directly if already root, `sudo usermod ...` otherwise -- goes
+    // through one of these two scripts either way, and both fail unconditionally.
+    for name in ["usermod", "sudo"] {
+        let script = bin_dir.join(name);
+        std::fs::write(&script, "#!/bin/sh\nexit 1\n")
+            .unwrap_or_else(|e| panic!("failed to write fake {name}: {e}"));
+        #[cfg(unix)]
+        {
+            use std::os::unix::fs::PermissionsExt;
+            std::fs::set_permissions(&script, std::fs::Permissions::from_mode(0o755))
+                .unwrap_or_else(|e| panic!("failed to chmod fake {name}: {e}"));
+        }
+    }
+    // Restricting `$PATH` to only the fakes above also makes the recipe's
+    // root-detection deterministic: it shells out to `id -u`, which is not on
+    // this PATH, so the spawn itself fails and reads as "not root" regardless of
+    // who runs the suite -- the same `sudo usermod` branch fails on every host,
+    // CI or developer machine, root or not.
+    world.command_env.push(("PATH", bin_dir.into_os_string()));
+    world.command_env.push(("USER", "e2e-test-user".into()));
+    world.model_name = Some(COMMAND_FAILURE_FIX_ID.to_string());
+}
+
 #[given("a user who refers to a cause by its position in the diagnosis")]
 async fn user_named_diagnosis_position(world: &mut E2eWorld) {
     // Quoted deliberately: unquoted, the shell treats `#1` as a comment and the
@@ -207,6 +256,15 @@ async fn user_applies_fix_without_agreeing(world: &mut E2eWorld) {
     world.cli_rc = Some(rc);
 }
 
+#[when("the user asks the CLI to apply the approved fix")]
+async fn user_applies_approved_fix(world: &mut E2eWorld) {
+    let fix_id = world.model_name.clone().expect("no fix id set");
+    let (stdout, stderr, rc) = crate::run_rocm_with_scenario_env(world, &["fix", &fix_id, "--yes"]);
+    world.cli_output = Some(stdout);
+    world.cli_stderr = Some(stderr);
+    world.cli_rc = Some(rc);
+}
+
 // ── Then ───────────────────────────────────────────────────────────
 
 #[then("the CLI reports a likely cause with a suggested fix")]
@@ -667,3 +725,24 @@ async fn assert_rc_file_untouched(world: &mut E2eWorld) {
         rc_file.display()
     );
 }
+
+#[then("the CLI reports the command failure on stderr with exit code 4")]
+async fn assert_command_failure_reported_on_stderr(world: &mut E2eWorld) {
+    // 4 is its own outcome: a command that ran and failed, distinct from 3
+    // (does not apply here) and 5 (user declined).
+    assert_eq!(
+        world.cli_rc,
+        Some(4),
+        "a fix whose helper command fails should exit 4"
+    );
+    let stderr = world.cli_stderr.as_deref().unwrap_or("");
+    assert!(
+        stderr.contains("usermod exited") && stderr.contains("group membership NOT changed"),
+        "the command-failure explanation must be on stderr:\n{stderr}"
+    );
+    let stdout = world.cli_output.as_deref().unwrap_or("");
+    assert!(
+        !stdout.contains("group membership NOT changed"),
+        "the command-failure explanation must not also be on stdout:\n{stdout}"
+    );
+}

From 4c1c2561d0c65f6ff213d07717fdd7dbbdf2deeb Mon Sep 17 00:00:00 2001
From: Jussi Elo <jussi.elo@amd.com>
Date: Tue, 8 Sep 2026 06:17:25 +0000
Subject: [PATCH 06/11] test(e2e): assert two more fix refusals on stderr
 specifically

assert_unknown_fix_refused and assert_position_argument_corrected
checked stdout+stderr concatenated, the same weakness already fixed
for their sibling assertions in this branch's stderr migration. Both
messages are eprintln!-only already, so tighten these two the same
way for consistency.

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
---
 .../e2e-cucumber/tests/e2e/diagnose_steps.rs  | 24 +++++++------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs
index 18e0a8e9c..074fcae81 100644
--- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs
+++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs
@@ -647,14 +647,10 @@ async fn assert_unknown_fix_refused(world: &mut E2eWorld) {
         Some(2),
         "unknown fix-id should exit 2 (unknown id)"
     );
-    let combined = format!(
-        "{}{}",
-        world.cli_output.as_deref().unwrap_or(""),
-        world.cli_stderr.as_deref().unwrap_or("")
-    );
+    let stderr = world.cli_stderr.as_deref().unwrap_or("");
     assert!(
-        combined.contains("Unknown fix-id"),
-        "expected an 'Unknown fix-id' message:\n{combined}"
+        stderr.contains("Unknown fix-id"),
+        "expected an 'Unknown fix-id' message on stderr:\n{stderr}"
     );
 }
 
@@ -667,19 +663,15 @@ async fn assert_position_argument_corrected(world: &mut E2eWorld) {
         Some(2),
         "a position argument should exit 2 like any unknown id"
     );
-    let combined = format!(
-        "{}{}",
-        world.cli_output.as_deref().unwrap_or(""),
-        world.cli_stderr.as_deref().unwrap_or("")
-    );
+    let stderr = world.cli_stderr.as_deref().unwrap_or("");
     assert!(
-        combined.contains("position"),
-        "the refusal must say the argument was read as a position:\n{combined}"
+        stderr.contains("position"),
+        "the refusal must say the argument was read as a position, on stderr:\n{stderr}"
     );
     // And it must point at what to use instead, or the correction is useless.
     assert!(
-        combined.contains("id:"),
-        "the refusal must name the identifier to use instead:\n{combined}"
+        stderr.contains("id:"),
+        "the refusal must name the identifier to use instead, on stderr:\n{stderr}"
     );
 }
 

From 22b10c919aa08bfda32d86f75ca15007dd47ea52 Mon Sep 17 00:00:00 2001
From: Jussi Elo <jussi.elo@amd.com>
Date: Tue, 8 Sep 2026 06:17:35 +0000
Subject: [PATCH 07/11] docs(cli): warn against wrapping the FixExitCode
 carrier

exit_code_for recovers the exit code via downcast_ref, which only
works if FixExitCode reaches it unwrapped. Note the invariant so a
future .context() on the fix() call doesn't silently break it.

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
---
 apps/rocm/src/main.rs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs
index 3027a98cd..5aa2fedfd 100644
--- a/apps/rocm/src/main.rs
+++ b/apps/rocm/src/main.rs
@@ -1124,6 +1124,12 @@ fn with_sigpipe_ignored<T>(f: impl FnOnce() -> T) -> T {
 /// Marker error carrying `rocm fix`'s exit code back through `main()`'s
 /// ordinary return path, instead of calling `std::process::exit` mid-stack
 /// and skipping the `_log_guard` destructor held in `run()`.
+///
+/// `exit_code_for` recovers the code via `downcast_ref`, which only works if
+/// this error reaches it unwrapped. Do not wrap the `fix()` call (e.g. with
+/// `.context(...)`) between where it's constructed and `exit_code_for` — that
+/// would break the downcast and silently fall through to the generic
+/// "Error: ..." branch instead of the carried exit code.
 #[derive(Debug)]
 struct FixExitCode(i32);
 

From e088e984ea32c0583e2a4eb8082d53ef93ea310e Mon Sep 17 00:00:00 2001
From: Jussi Elo <jussi.elo@amd.com>
Date: Wed, 9 Sep 2026 05:40:29 +0000
Subject: [PATCH 08/11] fix(fix): explain every decline and harden failure
 writes against I/O errors

- confirm() now prints "Not confirmed; refusing to apply." on every
  decline path (interactive no/EOF, not just the non-interactive
  refusal), and its yes/no parsing is extracted into
  is_affirmative_answer() with direct unit tests.
- Replace fix.rs's failure-explanation eprintln! calls with a fail!
  macro that ignores write errors, matching the panic-safe pattern
  main.rs already uses for its generic error fallback.
- Add a regression test exercising the real
  dispatch -> fix -> FixExitCode -> exit_code_for chain, plus a
  pointer comment at the Fix dispatch arm warning against wrapping
  the call (e.g. with .context(...)), which would silently break the
  exit-code downcast.

The interactive decline path was manually verified via a real PTY
(prints the message, exits 5); no new PTY e2e scenario was added since
it would require extending the TUI-specific PTY driver for a case
already covered by the unit test and the shared confirm() fix.

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
---
 apps/rocm/src/main.rs       | 31 +++++++++++++
 crates/rocm-core/src/fix.rs | 87 +++++++++++++++++++++++++------------
 2 files changed, 91 insertions(+), 27 deletions(-)

diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs
index 5aa2fedfd..25642d15d 100644
--- a/apps/rocm/src/main.rs
+++ b/apps/rocm/src/main.rs
@@ -1710,6 +1710,9 @@ fn dispatch(cli: Cli) -> Result<()> {
     match cli.command {
         Some(Command::Examine { json, framework }) => examine(json, framework.into()),
         Some(Command::Diagnose { symptom, top, json }) => diagnose(symptom, top, json),
+        // Do not wrap this call (e.g. with `.context(...)`) -- see
+        // `FixExitCode`'s doc comment for why that would silently break its
+        // exit-code-carrying downcast.
         Some(Command::Fix {
             fix_id,
             yes,
@@ -19308,6 +19311,34 @@ mod tests {
         assert_eq!(super::exit_code_for(Err(err)), ExitCode::FAILURE);
     }
 
+    /// Exercises the real `dispatch -> fix -> FixExitCode -> exit_code_for`
+    /// chain end to end, not just `exit_code_for` in isolation. Guards against
+    /// a future change at the `Command::Fix` dispatch arm (e.g. wrapping the
+    /// call with `.context(...)`) silently breaking the downcast and falling
+    /// through to the generic exit 1.
+    #[test]
+    #[allow(unsafe_code)] // std::env::set_var is unsafe in edition 2024
+    fn dispatch_carries_fixs_exit_code_through_to_exit_code_for() {
+        // Skip the startup update check: it's a side effect unrelated to what
+        // this test verifies, and could otherwise touch the network.
+        unsafe {
+            std::env::set_var("ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK", "1");
+        }
+        let cli = super::Cli {
+            command: Some(super::Command::Fix {
+                fix_id: Some("fix-does-not-exist".to_owned()),
+                yes: true,
+                dry_run: false,
+                device_index: None,
+            }),
+        };
+        let result = super::dispatch(cli);
+        unsafe {
+            std::env::remove_var("ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK");
+        }
+        assert_eq!(super::exit_code_for(result), ExitCode::from(2));
+    }
+
     /// A cache that has moved inside a directory uninstall already removes must
     /// not be reported as "not removed" — the note would be false.
     #[test]
diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs
index 9175558a5..ca91183b3 100644
--- a/crates/rocm-core/src/fix.rs
+++ b/crates/rocm-core/src/fix.rs
@@ -22,6 +22,15 @@ use std::time::Duration;
 const RUN_TIMEOUT: Duration = Duration::from_mins(1);
 const QUERY_TIMEOUT: Duration = Duration::from_secs(8);
 
+/// Print a failure explanation to stderr, ignoring write failures (closed
+/// stderr, full disk) so an I/O error while explaining a failure can't itself
+/// panic the process.
+macro_rules! fail {
+    ($($arg:tt)*) => {{
+        let _ = writeln!(std::io::stderr(), $($arg)*);
+    }};
+}
+
 /// Options controlling how a fix is applied.
 #[derive(Debug, Clone, Default)]
 pub struct FixOptions {
@@ -455,20 +464,18 @@ fn print_recipe(r: &FixRecipe) {
 #[must_use]
 pub fn apply(fix_id: &str, opts: &FixOptions) -> i32 {
     let Some(recipe) = find_recipe(fix_id) else {
-        eprintln!("Unknown fix-id: {fix_id}");
+        fail!("Unknown fix-id: {fix_id}");
         if looks_like_a_diagnosis_position(fix_id) {
             // `rocm diagnose` ranks findings `#1`, `#2`, and users reach for that
             // number here. It is a position in one report, not a name -- and it
             // does not line up with the catalog's `fix-1 … fix-15` either, so a
             // bare "unknown id" left them with nothing to correct.
-            eprintln!(
-                "`{fix_id}` looks like a position in a `rocm diagnose` report, not a fix-id."
-            );
-            eprintln!(
+            fail!("`{fix_id}` looks like a position in a `rocm diagnose` report, not a fix-id.");
+            fail!(
                 "Use the `id:` shown against that cause — `rocm diagnose` prints an `apply with:` line you can copy."
             );
         } else {
-            eprintln!("Run `rocm diagnose` to see which fix-id applies.");
+            fail!("Run `rocm diagnose` to see which fix-id applies.");
         }
         return 2;
     };
@@ -477,7 +484,7 @@ pub fn apply(fix_id: &str, opts: &FixOptions) -> i32 {
 
     let os = current_os();
     if !recipe.applies_on.contains(&os) {
-        eprintln!(
+        fail!(
             "This fix only applies on: {}. Running OS is: {os}.",
             recipe.applies_on.join(", ")
         );
@@ -496,7 +503,7 @@ pub fn apply(fix_id: &str, opts: &FixOptions) -> i32 {
     } else {
         // Internal error (auto-applicable recipe with no runner) -> 1, not 4
         // (4 is reserved for "attempted but the command failed").
-        eprintln!("Internal error: auto-applicable recipe has no runner.");
+        fail!("Internal error: auto-applicable recipe has no runner.");
         1
     }
 }
@@ -510,15 +517,21 @@ fn confirm(prompt: &str, assume_yes: bool) -> bool {
         return true;
     }
     if !std::io::stdin().is_terminal() {
-        eprintln!("Non-interactive shell and --yes not passed; refusing to apply.");
+        fail!("Non-interactive shell and --yes not passed; refusing to apply.");
         return false;
     }
     print!("{prompt} [y/N]: ");
     let _ = std::io::stdout().flush();
     let mut line = String::new();
-    if std::io::stdin().read_line(&mut line).is_err() {
-        return false;
+    let confirmed = std::io::stdin().read_line(&mut line).is_ok() && is_affirmative_answer(&line);
+    if !confirmed {
+        fail!("Not confirmed; refusing to apply.");
     }
+    confirmed
+}
+
+/// Parse a user's typed response to a `[y/N]` prompt.
+fn is_affirmative_answer(line: &str) -> bool {
     matches!(line.trim().to_lowercase().as_str(), "y" | "yes")
 }
 
@@ -565,16 +578,16 @@ fn run_render_group(opts: &FixOptions) -> i32 {
         .or_else(|_| std::env::var("LOGNAME"))
         .unwrap_or_default();
     if user.is_empty() {
-        eprintln!("Could not determine current user from $USER/$LOGNAME.");
+        fail!("Could not determine current user from $USER/$LOGNAME.");
         return 3;
     }
     if !which("usermod") {
-        eprintln!("`usermod` not on PATH; cannot add groups.");
+        fail!("`usermod` not on PATH; cannot add groups.");
         return 3;
     }
     let root = is_root();
     if !which("sudo") && !root {
-        eprintln!("`sudo` is not on PATH and we are not root; cannot add groups.");
+        fail!("`sudo` is not on PATH and we are not root; cannot add groups.");
         return 3;
     }
     let (program, args): (&str, Vec<String>) = if root {
@@ -612,7 +625,7 @@ fn run_render_group(opts: &FixOptions) -> i32 {
     print!("{out}");
     eprint!("{err}");
     if rc != 0 {
-        eprintln!("usermod exited {rc}; group membership NOT changed.");
+        fail!("usermod exited {rc}; group membership NOT changed.");
         return 4;
     }
     println!("Added {user} to render,video.");
@@ -710,7 +723,7 @@ fn run_unset_override_windows(opts: &FixOptions) -> i32 {
             print!("{out}");
             eprint!("{err}");
             if rc != 0 {
-                eprintln!("setx exited {rc}; User scope NOT changed.");
+                fail!("setx exited {rc}; User scope NOT changed.");
                 return 4;
             }
             println!("Cleared from User scope. Reopen your terminal for it to take effect.");
@@ -743,12 +756,12 @@ fn run_path_export_linux(opts: &FixOptions) -> i32 {
     // Same resolver `examine` uses, so the line we append names the install the
     // report pointed at -- including a versioned root like /opt/rocm-6.4.1.
     let Some(install) = crate::discover_rocm_installs().into_iter().next() else {
-        eprintln!("No ROCm install found; nothing to add to PATH.");
+        fail!("No ROCm install found; nothing to add to PATH.");
         return 3;
     };
     let bin_path = install.path.join("bin");
     if !bin_path.is_dir() {
-        eprintln!(
+        fail!(
             "{} does not exist; nothing to add to PATH.",
             bin_path.display()
         );
@@ -757,7 +770,7 @@ fn run_path_export_linux(opts: &FixOptions) -> i32 {
     let bin_dir_owned = bin_path.to_string_lossy().into_owned();
     let bin_dir = bin_dir_owned.as_str();
     let Some(rc_file) = shell_rc_file() else {
-        eprintln!("Could not determine your home directory.");
+        fail!("Could not determine your home directory.");
         return 3;
     };
     let export_line = format!("export PATH=\"{bin_dir}:$PATH\"");
@@ -786,7 +799,7 @@ fn run_path_export_linux(opts: &FixOptions) -> i32 {
         "# Added by rocm examine (fix-6-path)",
         &export_line,
     ) {
-        eprintln!("Failed to write {}: {exc}", rc_file.display());
+        fail!("Failed to write {}: {exc}", rc_file.display());
         return 4;
     }
     println!(
@@ -803,12 +816,12 @@ fn run_path_export_windows(opts: &FixOptions) -> i32 {
         sdk_path = newest_rocm_install_dir();
     }
     if sdk_path.is_empty() {
-        eprintln!("No HIP SDK install found. Run fix-13-hip-sdk-missing first.");
+        fail!("No HIP SDK install found. Run fix-13-hip-sdk-missing first.");
         return 3;
     }
     let bin_dir = Path::new(&sdk_path).join("bin");
     if !bin_dir.is_dir() {
-        eprintln!(
+        fail!(
             "{} does not exist on disk; HIP SDK install looks incomplete.",
             bin_dir.display()
         );
@@ -838,7 +851,7 @@ fn run_path_export_windows(opts: &FixOptions) -> i32 {
     print!("{out}");
     eprint!("{err}");
     if rc != 0 {
-        eprintln!("setx exited {rc}; User PATH NOT changed.");
+        fail!("setx exited {rc}; User PATH NOT changed.");
         return 4;
     }
     println!(
@@ -850,7 +863,7 @@ fn run_path_export_windows(opts: &FixOptions) -> i32 {
 /// fix-9: persist HIP_VISIBLE_DEVICES so the iGPU is hidden.
 fn run_hip_visible_devices(opts: &FixOptions) -> i32 {
     if let Some(idx) = opts.device_index.filter(|&i| i < 0) {
-        eprintln!("--device-index must be >= 0 (got {idx}).");
+        fail!("--device-index must be >= 0 (got {idx}).");
         return 3;
     }
     if runtime_is_windows() {
@@ -870,7 +883,7 @@ fn run_hip_visible_devices_linux(opts: &FixOptions) -> i32 {
         return 0;
     };
     let Some(rc_file) = shell_rc_file() else {
-        eprintln!("Could not determine your home directory.");
+        fail!("Could not determine your home directory.");
         return 3;
     };
     let export_line = format!("export HIP_VISIBLE_DEVICES={idx}");
@@ -897,7 +910,7 @@ fn run_hip_visible_devices_linux(opts: &FixOptions) -> i32 {
         "# Added by rocm examine (fix-9-igpu-dgpu)",
         &export_line,
     ) {
-        eprintln!("Failed to write {}: {exc}", rc_file.display());
+        fail!("Failed to write {}: {exc}", rc_file.display());
         return 4;
     }
     println!(
@@ -944,7 +957,7 @@ fn run_hip_visible_devices_windows(opts: &FixOptions) -> i32 {
     print!("{out}");
     eprint!("{err}");
     if rc != 0 {
-        eprintln!("setx exited {rc}; HIP_VISIBLE_DEVICES NOT changed.");
+        fail!("setx exited {rc}; HIP_VISIBLE_DEVICES NOT changed.");
         return 4;
     }
     println!(
@@ -997,6 +1010,26 @@ mod tests {
     // running concurrently can otherwise see each other's value mid-test.
     static PROCESS_ENV_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
 
+    #[test]
+    fn is_affirmative_answer_accepts_only_y_and_yes() {
+        for accepted in ["y", "Y", "yes", "YES", "Yes", "  y  ", "  yes\n"] {
+            assert!(
+                is_affirmative_answer(accepted),
+                "expected {accepted:?} to be treated as a yes"
+            );
+        }
+    }
+
+    #[test]
+    fn is_affirmative_answer_rejects_everything_else() {
+        for declined in ["n", "no", "", "\n", "yep", "ye"] {
+            assert!(
+                !is_affirmative_answer(declined),
+                "expected {declined:?} to be treated as a decline"
+            );
+        }
+    }
+
     /// Plant a directory the shared resolver will accept as a ROCm install.
     /// `bin/rocminfo` is one of the markers it gates on; a bare directory is
     /// deliberately not enough.

From 0cef54d56ee8465332b96dd5ac5a3c856a71ab04 Mon Sep 17 00:00:00 2001
From: Jussi Elo <jussi.elo@amd.com>
Date: Wed, 9 Sep 2026 06:13:48 +0000
Subject: [PATCH 09/11] fix(cli): correct FixExitCode downcast claim, use
 ScopedTestEnv in new test

Two review comments from earlier in the PR were already resolved by
prior commits (exit_code_for's panic-safe write, e2e coverage for the
stderr command-failure branch). Two were real:

- The doc comment claiming .context(...) breaks the FixExitCode
  downcast was wrong: anyhow::Error::downcast_ref searches the whole
  error chain, so context-wrapping is safe. What actually breaks the
  downcast is discarding the error into a fresh anyhow!(...) instead
  of chaining it. Corrected the claim at all three sites (the
  FixExitCode doc comment, the dispatch-arm pointer comment, and the
  regression test's doc comment).
- The new dispatch-level regression test mutated
  ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK via raw env::set_var/remove_var
  instead of the existing ScopedTestEnv guard, so it wasn't serialized
  against other env-touching tests and would leak the mutation on a
  panic. Switched to ScopedTestEnv.

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
---
 apps/rocm/src/main.rs | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs
index 25642d15d..0f590bd00 100644
--- a/apps/rocm/src/main.rs
+++ b/apps/rocm/src/main.rs
@@ -1125,11 +1125,12 @@ fn with_sigpipe_ignored<T>(f: impl FnOnce() -> T) -> T {
 /// ordinary return path, instead of calling `std::process::exit` mid-stack
 /// and skipping the `_log_guard` destructor held in `run()`.
 ///
-/// `exit_code_for` recovers the code via `downcast_ref`, which only works if
-/// this error reaches it unwrapped. Do not wrap the `fix()` call (e.g. with
-/// `.context(...)`) between where it's constructed and `exit_code_for` — that
-/// would break the downcast and silently fall through to the generic
-/// "Error: ..." branch instead of the carried exit code.
+/// `exit_code_for` recovers the code via `downcast_ref`, which searches the
+/// whole error chain — wrapping the `fix()` call with `.context(...)` is
+/// fine. What does break it is discarding this error instead of chaining it,
+/// e.g. `.map_err(|e| anyhow!("fix failed: {e}"))`, which loses the
+/// underlying type and silently falls through to the generic "Error: ..."
+/// branch instead of the carried exit code.
 #[derive(Debug)]
 struct FixExitCode(i32);
 
@@ -1710,9 +1711,10 @@ fn dispatch(cli: Cli) -> Result<()> {
     match cli.command {
         Some(Command::Examine { json, framework }) => examine(json, framework.into()),
         Some(Command::Diagnose { symptom, top, json }) => diagnose(symptom, top, json),
-        // Do not wrap this call (e.g. with `.context(...)`) -- see
+        // Keep this error chained rather than discarding it into a fresh
+        // `anyhow!(...)` (e.g. via a `.map_err` that restringifies it) -- see
         // `FixExitCode`'s doc comment for why that would silently break its
-        // exit-code-carrying downcast.
+        // exit-code-carrying downcast. `.context(...)` is fine.
         Some(Command::Fix {
             fix_id,
             yes,
@@ -19313,17 +19315,17 @@ mod tests {
 
     /// Exercises the real `dispatch -> fix -> FixExitCode -> exit_code_for`
     /// chain end to end, not just `exit_code_for` in isolation. Guards against
-    /// a future change at the `Command::Fix` dispatch arm (e.g. wrapping the
-    /// call with `.context(...)`) silently breaking the downcast and falling
-    /// through to the generic exit 1.
+    /// a future change at the `Command::Fix` dispatch arm (e.g. discarding the
+    /// error into a fresh `anyhow!(...)`) silently breaking the downcast and
+    /// falling through to the generic exit 1.
     #[test]
-    #[allow(unsafe_code)] // std::env::set_var is unsafe in edition 2024
     fn dispatch_carries_fixs_exit_code_through_to_exit_code_for() {
         // Skip the startup update check: it's a side effect unrelated to what
-        // this test verifies, and could otherwise touch the network.
-        unsafe {
-            std::env::set_var("ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK", "1");
-        }
+        // this test verifies, and could otherwise touch the network. Goes
+        // through `ScopedTestEnv` so it's serialized against every other test
+        // that touches process env and restored on drop even on panic.
+        let mut env = ScopedTestEnv::new();
+        env.set("ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK", "1");
         let cli = super::Cli {
             command: Some(super::Command::Fix {
                 fix_id: Some("fix-does-not-exist".to_owned()),
@@ -19333,9 +19335,7 @@ mod tests {
             }),
         };
         let result = super::dispatch(cli);
-        unsafe {
-            std::env::remove_var("ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK");
-        }
+        drop(env);
         assert_eq!(super::exit_code_for(result), ExitCode::from(2));
     }
 

From 294a183233c5fe083252b53b71c6ab53b2e3bc17 Mon Sep 17 00:00:00 2001
From: Jussi Elo <jussi.elo@amd.com>
Date: Thu, 10 Sep 2026 07:21:44 +0000
Subject: [PATCH 10/11] fix(cli): flush log guard on clap exit, harden output
 relay, cover interactive decline

Closes the remaining non-blocking findings from PR #350's review
(https://github.com/ROCm/rocm-cli/pull/350#issuecomment-5606253254), all in
the same class of bug that PR fixed for `fix()`:

- `parse_cli()` and the mistyped-subcommand branch in `run()` still called
  `clap::Error::exit()` directly, bypassing `_log_guard`'s destructor the
  same way the pre-PR `fix()` did. Both now return a `ClapExitCode` marker
  error through the ordinary `Result` path instead.
- Four call sites relaying a captured subprocess's stdout/stderr with
  `print!`/`eprint!` could still panic on a closed pipe, unlike `fail!`.
  Consolidated them behind a `relay_output` helper that ignores write
  failures for the same reason `fail!` does.
- `confirm()`'s interactive-decline branch had no e2e coverage: the piped-
  stdin harness can never reach it, since `is_terminal()` is always false
  there. Added a PTY-driven scenario (diagnose-14) that types "n" at a real
  terminal prompt and asserts the same refusal outcome as the non-interactive
  path.
- Strengthened the diagnose-08/-11 refusal assertions to also check the
  message never leaks onto stdout, matching the bar diagnose-13 already set.

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
---
 apps/rocm/src/main.rs                         | 69 +++++++++++++++++--
 crates/rocm-core/src/fix.rs                   | 20 +++---
 tests/e2e-cucumber/features/diagnose.feature  | 14 ++++
 .../e2e-cucumber/tests/e2e/diagnose_steps.rs  | 53 ++++++++++++++
 tests/e2e-cucumber/tests/e2e/tui_driver.rs    | 27 +++++---
 5 files changed, 159 insertions(+), 24 deletions(-)

diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs
index 0f590bd00..71a998359 100644
--- a/apps/rocm/src/main.rs
+++ b/apps/rocm/src/main.rs
@@ -1142,20 +1142,40 @@ impl std::fmt::Display for FixExitCode {
 
 impl std::error::Error for FixExitCode {}
 
+/// Marker error carrying a clap usage/parse error's exit code back through
+/// `main()`'s ordinary return path, for the same reason [`FixExitCode`]
+/// exists: `clap::Error::exit()` calls `std::process::exit` mid-stack, which
+/// would skip the `_log_guard` destructor held in `run()`. The error is
+/// printed at the point it's constructed (clap knows which stream and
+/// formatting a given error kind wants); this type only carries the exit
+/// code onward.
+#[derive(Debug)]
+struct ClapExitCode(i32);
+
+impl std::fmt::Display for ClapExitCode {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "clap exited with code {}", self.0)
+    }
+}
+
+impl std::error::Error for ClapExitCode {}
+
 fn main() -> ExitCode {
     exit_code_for(run())
 }
 
 /// Maps `run()`'s result to a process exit code, unwrapping a `FixExitCode`
-/// to its carried code and otherwise reproducing the standard
-/// `Result<(), anyhow::Error>` `Termination` behavior (print the error to
-/// stderr, exit 1).
+/// or `ClapExitCode` to its carried code and otherwise reproducing the
+/// standard `Result<(), anyhow::Error>` `Termination` behavior (print the
+/// error to stderr, exit 1).
 fn exit_code_for(result: Result<()>) -> ExitCode {
     match result {
         Ok(()) => ExitCode::SUCCESS,
         Err(e) => {
             if let Some(FixExitCode(code)) = e.downcast_ref::<FixExitCode>() {
                 ExitCode::from(*code as u8)
+            } else if let Some(ClapExitCode(code)) = e.downcast_ref::<ClapExitCode>() {
+                ExitCode::from(*code as u8)
             } else {
                 // Match the standard `Result<(), E>` `Termination` behavior exactly:
                 // ignore a failed write here rather than `eprintln!`, which panics.
@@ -1195,7 +1215,9 @@ fn run() -> Result<()> {
         // exit, instead of dumping a request plan from the natural-language
         // planner.
         if let Some(err) = command_invocation_error(&freeform_invocation.request_args) {
-            err.exit();
+            let code = err.exit_code();
+            let _ = err.print();
+            return Err(ClapExitCode(code).into());
         }
         return run_freeform(
             freeform_invocation.request_args.join(" "),
@@ -1208,7 +1230,7 @@ fn run() -> Result<()> {
         );
     }
 
-    dispatch(parse_cli())
+    dispatch(parse_cli()?)
 }
 
 /// Build the root `rocm` command with its top-level subcommands ordered
@@ -1232,9 +1254,17 @@ fn cli_command() -> clap::Command {
 /// `rocm help` list subcommands alphabetically. Mirrors the derived
 /// `Cli::parse()`, which builds from `Cli::command()` directly and therefore
 /// cannot pick up the reordering.
-fn parse_cli() -> Cli {
+///
+/// Returns a [`ClapExitCode`]-carrying error instead of calling
+/// `clap::Error::exit()` directly, so `run()`'s caller can unwrap the code
+/// after `_log_guard` has dropped rather than mid-stack.
+fn parse_cli() -> Result<Cli> {
     let matches = cli_command().get_matches();
-    Cli::from_arg_matches(&matches).unwrap_or_else(|err| err.exit())
+    Cli::from_arg_matches(&matches).map_err(|err| {
+        let code = err.exit_code();
+        let _ = err.print();
+        ClapExitCode(code).into()
+    })
 }
 
 /// Legacy `uv` cache location, used before the cache was colocated with the managed
@@ -19304,6 +19334,31 @@ mod tests {
         assert_eq!(super::exit_code_for(Err(err)), ExitCode::from(3));
     }
 
+    /// `ClapExitCode` exists so a usage/parse error can reach `main()` through
+    /// the ordinary return path (letting `_log_guard` drop) instead of
+    /// `clap::Error::exit()` calling `std::process::exit` mid-stack. Guard the
+    /// downcast the same way `exit_code_for_fix_exit_code_carries_the_code`
+    /// guards `FixExitCode`'s.
+    #[test]
+    fn exit_code_for_clap_exit_code_carries_the_code() {
+        let err = anyhow::Error::new(super::ClapExitCode(2));
+        assert_eq!(super::exit_code_for(Err(err)), ExitCode::from(2));
+    }
+
+    /// `parse_cli`'s internal clap error now returns a `ClapExitCode` instead
+    /// of calling `err.exit()` directly. Exercise the real
+    /// `clap parse failure -> ClapExitCode -> exit_code_for` chain end to end
+    /// via `command_invocation_error`, which shares the same
+    /// `clap::Error::exit_code()`/`print()` handoff.
+    #[test]
+    fn clap_error_exit_code_survives_the_clap_exit_code_round_trip() {
+        let err = command_invocation_error(&["instal".to_owned()])
+            .expect("`instal` should read as a mistyped subcommand");
+        let code = err.exit_code();
+        let result: Result<()> = Err(super::ClapExitCode(code).into());
+        assert_eq!(super::exit_code_for(result), ExitCode::from(code as u8));
+    }
+
     /// Any other error must still fail with exit 1, matching what
     /// `Result<(), anyhow::Error>`'s `Termination` impl already does today for
     /// every subcommand other than `fix`.
diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs
index ca91183b3..39945a890 100644
--- a/crates/rocm-core/src/fix.rs
+++ b/crates/rocm-core/src/fix.rs
@@ -31,6 +31,14 @@ macro_rules! fail {
     }};
 }
 
+/// Relay a captured command's stdout/stderr, ignoring write failures for the
+/// same reason `fail!` does — relaying subprocess output can't itself panic
+/// the process if the pipe on the other end is closed.
+fn relay_output(out: &str, err: &str) {
+    let _ = write!(std::io::stdout(), "{out}");
+    let _ = write!(std::io::stderr(), "{err}");
+}
+
 /// Options controlling how a fix is applied.
 #[derive(Debug, Clone, Default)]
 pub struct FixOptions {
@@ -622,8 +630,7 @@ fn run_render_group(opts: &FixOptions) -> i32 {
     }
     let arg_refs: Vec<&str> = args.iter().map(String::as_str).collect();
     let (rc, out, err) = run(program, &arg_refs, RUN_TIMEOUT);
-    print!("{out}");
-    eprint!("{err}");
+    relay_output(&out, &err);
     if rc != 0 {
         fail!("usermod exited {rc}; group membership NOT changed.");
         return 4;
@@ -720,8 +727,7 @@ fn run_unset_override_windows(opts: &FixOptions) -> i32 {
             println!("  (dry-run; not executed)");
         } else if confirm("Clear HSA_OVERRIDE_GFX_VERSION from User scope?", opts.yes) {
             let (rc, out, err) = run("setx", &["HSA_OVERRIDE_GFX_VERSION", ""], RUN_TIMEOUT);
-            print!("{out}");
-            eprint!("{err}");
+            relay_output(&out, &err);
             if rc != 0 {
                 fail!("setx exited {rc}; User scope NOT changed.");
                 return 4;
@@ -848,8 +854,7 @@ fn run_path_export_windows(opts: &FixOptions) -> i32 {
         return 5;
     }
     let (rc, out, err) = run("setx", &["PATH", &new_path], RUN_TIMEOUT);
-    print!("{out}");
-    eprint!("{err}");
+    relay_output(&out, &err);
     if rc != 0 {
         fail!("setx exited {rc}; User PATH NOT changed.");
         return 4;
@@ -954,8 +959,7 @@ fn run_hip_visible_devices_windows(opts: &FixOptions) -> i32 {
         &["HIP_VISIBLE_DEVICES", &idx.to_string()],
         RUN_TIMEOUT,
     );
-    print!("{out}");
-    eprint!("{err}");
+    relay_output(&out, &err);
     if rc != 0 {
         fail!("setx exited {rc}; HIP_VISIBLE_DEVICES NOT changed.");
         return 4;
diff --git a/tests/e2e-cucumber/features/diagnose.feature b/tests/e2e-cucumber/features/diagnose.feature
index eb50485ed..f50741d73 100644
--- a/tests/e2e-cucumber/features/diagnose.feature
+++ b/tests/e2e-cucumber/features/diagnose.feature
@@ -153,3 +153,17 @@ Feature: Diagnosing failures and listing fixes
     Given a user who has approved a fix whose helper command will fail
     When the user asks the CLI to apply the approved fix
     Then the CLI reports the command failure on stderr with exit code 4
+
+  # diagnose-08 proves the non-interactive refusal (piped stdin, `is_terminal()`
+  # false); this proves the sibling branch on a real terminal — the CLI must
+  # print the confirmation prompt, read the typed answer, and, on anything but
+  # y/yes, decline the same way. That branch has no piped-stdin equivalent: a
+  # real TTY is required to reach it at all, so this is the one scenario in the
+  # suite driven through the pseudo-terminal harness instead of piped stdin.
+  # Linux-only for the same reason diagnose-08 is: the recipe under test
+  # (`fix-9-igpu-dgpu`) only appends a shell rc file on Linux.
+  @id:diagnose-fix-interactive-decline-reported @requires-os:linux
+  Scenario: diagnose-14 - Declining the confirmation prompt on a real terminal is reported the same way
+    Given a user who has chosen a fix that would change the machine
+    When the user is asked interactively to apply it and types no
+    Then the CLI declines on the terminal and explains that it needs agreement
diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs
index 074fcae81..6d69c1988 100644
--- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs
+++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs
@@ -5,6 +5,7 @@
 use cucumber::{given, then, when};
 
 use crate::E2eWorld;
+use crate::e2e::tui_driver::{TuiSession, default_timeout};
 
 /// A symptom string that scores a catalog match on both Linux and Windows. It
 /// keys off `check_1_arch_not_in_wheel` (a `LINUX_AND_WINDOWS` checker), which
@@ -265,6 +266,31 @@ async fn user_applies_approved_fix(world: &mut E2eWorld) {
     world.cli_rc = Some(rc);
 }
 
+#[when("the user is asked interactively to apply it and types no")]
+async fn user_declines_fix_interactively(world: &mut E2eWorld) {
+    let fix_id = world.model_name.clone().expect("no fix id set");
+    // `run_rocm`'s piped stdin can never reach `confirm()`'s interactive
+    // branch: `is_terminal()` is always false there. A real pseudo-terminal is
+    // the only way to reach it, so this step (unlike every other one in this
+    // file) drives the CLI through `TuiSession` instead of `run_rocm`.
+    let mut session = TuiSession::spawn(world, &["fix", &fix_id, "--device-index", "1"])
+        .unwrap_or_else(|e| panic!("failed to open the fix prompt: {e}"));
+    session
+        .wait_for_screen("[y/N]:", default_timeout())
+        .await
+        .unwrap_or_else(|e| panic!("the confirmation prompt never appeared: {e}"));
+    session
+        .send("n\r")
+        .unwrap_or_else(|e| panic!("failed to type the decline: {e}"));
+    let rc = session
+        .wait_for_exit_code(default_timeout())
+        .await
+        .unwrap_or_else(|e| panic!("the CLI never exited after declining: {e}"));
+    world.cli_output = Some(session.screen_text());
+    world.cli_rc = Some(rc);
+    world.tui = Some(session);
+}
+
 // ── Then ───────────────────────────────────────────────────────────
 
 #[then("the CLI reports a likely cause with a suggested fix")]
@@ -543,6 +569,11 @@ async fn assert_inapplicable_fix_declined(world: &mut E2eWorld) {
         stderr.contains("This fix only applies on:"),
         "the refusal must say which platforms the fix is for, on stderr:\n{stderr}"
     );
+    let stdout = world.cli_output.as_deref().unwrap_or("");
+    assert!(
+        !stdout.contains("This fix only applies on:"),
+        "the platform refusal must not also be on stdout:\n{stdout}"
+    );
 }
 
 #[then("every fix the catalog documents is listed")]
@@ -688,6 +719,11 @@ async fn assert_refuses_without_agreement(world: &mut E2eWorld) {
         stderr.contains("refusing to apply"),
         "the refusal must say it did not apply the fix, on stderr:\n{stderr}"
     );
+    let stdout = world.cli_output.as_deref().unwrap_or("");
+    assert!(
+        !stdout.contains("refusing to apply"),
+        "the agreement refusal must not also be on stdout:\n{stdout}"
+    );
     // Distinct from the unknown-id refusal (2), so a script can tell "you did
     // not agree" apart from "no such fix".
     assert_eq!(
@@ -697,6 +733,23 @@ async fn assert_refuses_without_agreement(world: &mut E2eWorld) {
     );
 }
 
+#[then("the CLI declines on the terminal and explains that it needs agreement")]
+async fn assert_interactive_decline_reported(world: &mut E2eWorld) {
+    // Same outcome as the non-interactive refusal (diagnose-08): declining is
+    // its own outcome, not an error.
+    assert_eq!(
+        world.cli_rc,
+        Some(5),
+        "declining an interactive prompt is its own outcome, not an error"
+    );
+    let screen = world.cli_output.as_deref().unwrap_or("");
+    assert!(
+        screen.contains("Not confirmed; refusing to apply."),
+        "the terminal must show the same decline message the non-interactive \
+         path reports, on screen:\n{screen}"
+    );
+}
+
 #[then("the file the fix would have changed is untouched")]
 async fn assert_rc_file_untouched(world: &mut E2eWorld) {
     let rc_file = fix_rc_file(world);
diff --git a/tests/e2e-cucumber/tests/e2e/tui_driver.rs b/tests/e2e-cucumber/tests/e2e/tui_driver.rs
index 3ec8d0d85..bf3c538aa 100644
--- a/tests/e2e-cucumber/tests/e2e/tui_driver.rs
+++ b/tests/e2e-cucumber/tests/e2e/tui_driver.rs
@@ -458,20 +458,29 @@ impl TuiSession {
 
     /// Poll until the child exits, asserting a successful (zero) exit code.
     pub async fn wait_for_exit(&mut self, timeout: Duration) -> Result<(), String> {
+        match self.wait_for_exit_code(timeout).await? {
+            0 => Ok(()),
+            code => Err(format!(
+                "TUI exited unsuccessfully (code {code}).\n{}",
+                self.framed_screen()
+            )),
+        }
+    }
+
+    /// Poll until the child exits, returning its raw exit code regardless of
+    /// whether it is zero. Used by journeys (e.g. a declined confirmation
+    /// prompt) whose success case is a specific *nonzero* code, where
+    /// [`wait_for_exit`](Self::wait_for_exit)'s built-in zero-only assertion
+    /// would reject the very outcome under test.
+    pub async fn wait_for_exit_code(&mut self, timeout: Duration) -> Result<i32, String> {
         let deadline = Instant::now() + timeout;
         loop {
             match self.child.try_wait() {
                 Ok(Some(status)) => {
+                    let code = i32::try_from(status.exit_code()).unwrap_or(-1);
                     self.finished = true;
-                    self.record_once(i32::try_from(status.exit_code()).unwrap_or(-1));
-                    return if status.success() {
-                        Ok(())
-                    } else {
-                        Err(format!(
-                            "TUI exited unsuccessfully ({status:?}).\n{}",
-                            self.framed_screen()
-                        ))
-                    };
+                    self.record_once(code);
+                    return Ok(code);
                 }
                 Ok(None) => {}
                 Err(e) => return Err(format!("failed to poll TUI child: {e}")),

From 8649dff95301543e74fd5855cc39f07067483e37 Mon Sep 17 00:00:00 2001
From: Jussi Elo <jussi.elo@amd.com>
Date: Thu, 10 Sep 2026 12:14:17 +0000
Subject: [PATCH 11/11] fix(cli): route parse_cli through try_get_matches,
 cover PTY decline path

parse_cli() called clap's get_matches(), which self-exits on a parse
error and skips the log guard's flush -- the same bug this PR already
fixed for fix(). Switch to try_get_matches() and share the print+wrap
logic with the mistyped-subcommand branch via a new clap_exit_code
helper, and rewrite the round-trip test to exercise that helper on a
real clap::Error instead of a hand-built marker.

Also give TuiSession callers a way to override HOME/SHELL the way the
piped run_rocm_with_env path already can, and use it in diagnose-16 so
the interactive-decline scenario points at the same rc file the Given
step planted -- plus assert that file stays untouched, matching its
piped sibling.

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
---
 apps/rocm/src/main.rs                         | 67 ++++++++++++++-----
 tests/e2e-cucumber/features/diagnose.feature  |  1 +
 .../e2e-cucumber/tests/e2e/diagnose_steps.rs  | 16 ++++-
 tests/e2e-cucumber/tests/e2e/tui_driver.rs    | 27 ++++++++
 4 files changed, 91 insertions(+), 20 deletions(-)

diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs
index 1d7c5b48c..a3b659b96 100644
--- a/apps/rocm/src/main.rs
+++ b/apps/rocm/src/main.rs
@@ -1161,6 +1161,16 @@ impl std::fmt::Display for ClapExitCode {
 
 impl std::error::Error for ClapExitCode {}
 
+/// Print a clap usage/parse error on the stream and in the format clap itself
+/// chooses, then carry its exit code onward as a [`ClapExitCode`] instead of
+/// calling `err.exit()`, which would `std::process::exit` mid-stack and skip
+/// the `_log_guard` destructor.
+fn clap_exit_code(err: clap::Error) -> anyhow::Error {
+    let code = err.exit_code();
+    let _ = err.print();
+    ClapExitCode(code).into()
+}
+
 fn main() -> ExitCode {
     exit_code_for(run())
 }
@@ -1216,9 +1226,7 @@ fn run() -> Result<()> {
         // exit, instead of dumping a request plan from the natural-language
         // planner.
         if let Some(err) = command_invocation_error(&freeform_invocation.request_args) {
-            let code = err.exit_code();
-            let _ = err.print();
-            return Err(ClapExitCode(code).into());
+            return Err(clap_exit_code(err));
         }
         return run_freeform(
             freeform_invocation.request_args.join(" "),
@@ -1258,14 +1266,13 @@ fn cli_command() -> clap::Command {
 ///
 /// Returns a [`ClapExitCode`]-carrying error instead of calling
 /// `clap::Error::exit()` directly, so `run()`'s caller can unwrap the code
-/// after `_log_guard` has dropped rather than mid-stack.
+/// after `_log_guard` has dropped rather than mid-stack. This covers both
+/// places clap can fail here: `try_get_matches()` for an ordinary argv parse
+/// error (a bad flag, `--help`, a missing required argument — the common
+/// case), and `from_arg_matches()` for the derive step below it.
 fn parse_cli() -> Result<Cli> {
-    let matches = cli_command().get_matches();
-    Cli::from_arg_matches(&matches).map_err(|err| {
-        let code = err.exit_code();
-        let _ = err.print();
-        ClapExitCode(code).into()
-    })
+    let matches = cli_command().try_get_matches().map_err(clap_exit_code)?;
+    Cli::from_arg_matches(&matches).map_err(clap_exit_code)
 }
 
 /// Legacy `uv` cache location, used before the cache was colocated with the managed
@@ -19405,18 +19412,42 @@ mod tests {
         assert_eq!(super::exit_code_for(Err(err)), ExitCode::from(2));
     }
 
-    /// `parse_cli`'s internal clap error now returns a `ClapExitCode` instead
-    /// of calling `err.exit()` directly. Exercise the real
-    /// `clap parse failure -> ClapExitCode -> exit_code_for` chain end to end
-    /// via `command_invocation_error`, which shares the same
-    /// `clap::Error::exit_code()`/`print()` handoff.
+    /// `run()`'s mistyped-subcommand branch and `parse_cli()` both route a
+    /// real `clap::Error` through the production `clap_exit_code` helper
+    /// (not a hand-built `ClapExitCode`), so this calls that same helper on a
+    /// real parse failure to exercise the actual
+    /// `clap parse failure -> clap_exit_code -> ClapExitCode -> exit_code_for`
+    /// chain end to end. Reverting `clap_exit_code` to call `err.exit()`
+    /// directly, or dropping its use from either call site, breaks this.
     #[test]
     fn clap_error_exit_code_survives_the_clap_exit_code_round_trip() {
         let err = command_invocation_error(&["instal".to_owned()])
             .expect("`instal` should read as a mistyped subcommand");
-        let code = err.exit_code();
-        let result: Result<()> = Err(super::ClapExitCode(code).into());
-        assert_eq!(super::exit_code_for(result), ExitCode::from(code as u8));
+        let expected_code = err.exit_code();
+        let result: Result<()> = Err(super::clap_exit_code(err));
+        assert_eq!(
+            super::exit_code_for(result),
+            ExitCode::from(expected_code as u8)
+        );
+    }
+
+    /// `parse_cli()` itself reads `std::env::args_os()` (via
+    /// `Command::try_get_matches()`), which a unit test cannot redirect, so
+    /// this exercises the same `cli_command()` builder with an explicit argv
+    /// instead: an unrecognised flag is the common case `parse_cli()` was
+    /// still routing through `err.exit()` before it switched from
+    /// `get_matches()` to `try_get_matches()`.
+    #[test]
+    fn cli_command_rejects_unknown_flag_through_clap_exit_code() {
+        let err = super::cli_command()
+            .try_get_matches_from(["rocm", "--this-flag-does-not-exist"])
+            .expect_err("an unknown flag must be a parse error");
+        let expected_code = err.exit_code();
+        let result: Result<()> = Err(super::clap_exit_code(err));
+        assert_eq!(
+            super::exit_code_for(result),
+            ExitCode::from(expected_code as u8)
+        );
     }
 
     /// Any other error must still fail with exit 1, matching what
diff --git a/tests/e2e-cucumber/features/diagnose.feature b/tests/e2e-cucumber/features/diagnose.feature
index 76de789c6..28dfd5102 100644
--- a/tests/e2e-cucumber/features/diagnose.feature
+++ b/tests/e2e-cucumber/features/diagnose.feature
@@ -209,3 +209,4 @@ Feature: Diagnosing failures and listing fixes
     Given a user who has chosen a fix that would change the machine
     When the user is asked interactively to apply it and types no
     Then the CLI declines on the terminal and explains that it needs agreement
+    And the file the fix would have changed is untouched
diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs
index ce1486585..0f4351bc4 100644
--- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs
+++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs
@@ -293,12 +293,24 @@ async fn user_applies_approved_fix(world: &mut E2eWorld) {
 #[when("the user is asked interactively to apply it and types no")]
 async fn user_declines_fix_interactively(world: &mut E2eWorld) {
     let fix_id = world.model_name.clone().expect("no fix id set");
+    let home = fix_home(world).display().to_string();
     // `run_rocm`'s piped stdin can never reach `confirm()`'s interactive
     // branch: `is_terminal()` is always false there. A real pseudo-terminal is
     // the only way to reach it, so this step (unlike every other one in this
     // file) drives the CLI through `TuiSession` instead of `run_rocm`.
-    let mut session = TuiSession::spawn(world, &["fix", &fix_id, "--device-index", "1"])
-        .unwrap_or_else(|e| panic!("failed to open the fix prompt: {e}"));
+    //
+    // `TuiSession`'s own isolation (`pty_env`) sets HOME to a PTY-only sandbox
+    // it owns and never sets SHELL, so without overriding both here the fix
+    // would resolve to a different — and possibly nonexistent — rc file than
+    // the one the `Given` step planted, the same way the piped sibling
+    // (`user_applies_fix_without_agreeing`) overrides them via
+    // `run_rocm_with_env`.
+    let mut session = TuiSession::spawn_with_env(
+        world,
+        &["fix", &fix_id, "--device-index", "1"],
+        &[("HOME", home.as_str()), ("SHELL", "/bin/bash")],
+    )
+    .unwrap_or_else(|e| panic!("failed to open the fix prompt: {e}"));
     session
         .wait_for_screen("[y/N]:", default_timeout())
         .await
diff --git a/tests/e2e-cucumber/tests/e2e/tui_driver.rs b/tests/e2e-cucumber/tests/e2e/tui_driver.rs
index bf3c538aa..b65eb9b9c 100644
--- a/tests/e2e-cucumber/tests/e2e/tui_driver.rs
+++ b/tests/e2e-cucumber/tests/e2e/tui_driver.rs
@@ -126,6 +126,19 @@ impl TuiSession {
         Self::spawn_binary(world, crate::rocm_binary(), args)
     }
 
+    /// Like [`spawn`](Self::spawn), but overlaying `extra_env` on top of the
+    /// scenario's isolation environment — for a step whose `Given` planted
+    /// scenario-owned state (e.g. a shell rc file) that only the piped
+    /// (`run_rocm_with_env`) path would otherwise pick up, since [`pty_env`]'s
+    /// `HOME`/lack of `SHELL` are the PTY's own isolation, not that state.
+    pub fn spawn_with_env(
+        world: &E2eWorld,
+        args: &[&str],
+        extra_env: &[(&str, &str)],
+    ) -> Result<Self, String> {
+        Self::spawn_binary_with_env(world, crate::rocm_binary(), args, extra_env)
+    }
+
     /// Spawn a specific `rocm` binary under a fresh PTY.
     ///
     /// Most scenarios use [`spawn`](Self::spawn) and exercise the harness-built
@@ -135,6 +148,15 @@ impl TuiSession {
         world: &E2eWorld,
         binary: impl AsRef<std::ffi::OsStr>,
         args: &[&str],
+    ) -> Result<Self, String> {
+        Self::spawn_binary_with_env(world, binary, args, &[])
+    }
+
+    fn spawn_binary_with_env(
+        world: &E2eWorld,
+        binary: impl AsRef<std::ffi::OsStr>,
+        args: &[&str],
+        extra_env: &[(&str, &str)],
     ) -> Result<Self, String> {
         let pair = native_pty_system()
             .openpty(PtySize {
@@ -159,6 +181,11 @@ impl TuiSession {
         for (key, value) in world.isolate_env().into_iter().chain(world.pty_env()) {
             cmd.env(key, value);
         }
+        // Caller-supplied overrides win over the scenario's own isolation
+        // (e.g. a `Given` step's HOME/SHELL for state it planted itself).
+        for (key, value) in extra_env {
+            cmd.env(key, value);
+        }
         // Provider configuration changes product startup semantics: a host API
         // key or endpoint suppresses local managed-service detection. These PTY
         // journeys exercise deterministic local/mock chat, so do not let the