diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 5c14ea484..a3b659b96 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -63,6 +63,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}; @@ -1121,7 +1122,84 @@ fn with_sigpipe_ignored(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()`. +/// +/// `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); + +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 {} + +/// 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 {} + +/// 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()) +} + +/// Maps `run()`'s result to a process exit code, unwrapping a `FixExitCode` +/// 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::() { + ExitCode::from(*code as u8) + } else if let Some(ClapExitCode(code)) = e.downcast_ref::() { + ExitCode::from(*code as u8) + } else { + // 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 + } + } + } +} + +fn run() -> Result<()> { reset_sigpipe(); // Held for the whole process lifetime: dropping it flushes and stops the @@ -1148,7 +1226,7 @@ fn main() -> 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(); + return Err(clap_exit_code(err)); } return run_freeform( freeform_invocation.request_args.join(" "), @@ -1161,7 +1239,7 @@ fn main() -> Result<()> { ); } - dispatch(parse_cli()) + dispatch(parse_cli()?) } /// Build the root `rocm` command with its top-level subcommands ordered @@ -1185,9 +1263,16 @@ 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 { - let matches = cli_command().get_matches(); - Cli::from_arg_matches(&matches).unwrap_or_else(|err| err.exit()) +/// +/// 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. 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 { + 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 @@ -1664,6 +1749,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), + // 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. `.context(...)` is fine. Some(Command::Fix { fix_id, yes, @@ -2274,7 +2363,7 @@ fn fix(fix_id: Option, yes: bool, dry_run: bool, device_index: Option 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)); + } + + /// `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)); + } + + /// `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 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 + /// `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); + } + + /// 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. discarding the + /// error into a fresh `anyhow!(...)`) silently breaking the downcast and + /// falling through to the generic exit 1. + #[test] + 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. 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()), + yes: true, + dry_run: false, + device_index: None, + }), + }; + let result = super::dispatch(cli); + drop(env); + 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 3fafec9eb..3218e920b 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -22,6 +22,23 @@ 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)*); + }}; +} + +/// 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 { @@ -590,20 +607,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-N` names 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; }; @@ -612,7 +627,7 @@ pub fn apply(fix_id: &str, opts: &FixOptions) -> i32 { let os = current_os(); if !recipe.applies_on.contains(&os) { - println!( + fail!( "This fix only applies on: {}. Running OS is: {os}.", recipe.applies_on.join(", ") ); @@ -631,7 +646,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 } } @@ -645,15 +660,21 @@ 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."); + 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") } @@ -700,16 +721,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."); + fail!("Could not determine current user from $USER/$LOGNAME."); return 3; } if !which("usermod") { - println!("`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 { - println!("`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) = if root { @@ -744,10 +765,9 @@ 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 { - println!("usermod exited {rc}; group membership NOT changed."); + fail!("usermod exited {rc}; group membership NOT changed."); return 4; } println!("Added {user} to render,video."); @@ -842,10 +862,9 @@ 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 { - println!("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."); @@ -878,12 +897,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."); + fail!("No ROCm install found; nothing to add to PATH."); return 3; }; let bin_path = install.path.join("bin"); if !bin_path.is_dir() { - println!( + fail!( "{} does not exist; nothing to add to PATH.", bin_path.display() ); @@ -892,7 +911,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."); + fail!("Could not determine your home directory."); return 3; }; let export_line = format!("export PATH=\"{bin_dir}:$PATH\""); @@ -921,7 +940,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()); + fail!("Failed to write {}: {exc}", rc_file.display()); return 4; } println!( @@ -938,12 +957,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."); + 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() { - println!( + fail!( "{} does not exist on disk; HIP SDK install looks incomplete.", bin_dir.display() ); @@ -970,10 +989,9 @@ 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 { - println!("setx exited {rc}; User PATH NOT changed."); + fail!("setx exited {rc}; User PATH NOT changed."); return 4; } println!( @@ -985,7 +1003,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})."); + fail!("--device-index must be >= 0 (got {idx})."); return 3; } if runtime_is_windows() { @@ -1005,7 +1023,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."); + fail!("Could not determine your home directory."); return 3; }; let export_line = format!("export HIP_VISIBLE_DEVICES={idx}"); @@ -1032,7 +1050,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()); + fail!("Failed to write {}: {exc}", rc_file.display()); return 4; } println!( @@ -1076,10 +1094,9 @@ 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 { - println!("setx exited {rc}; HIP_VISIBLE_DEVICES NOT changed."); + fail!("setx exited {rc}; HIP_VISIBLE_DEVICES NOT changed."); return 4; } println!( @@ -1132,6 +1149,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. diff --git a/tests/e2e-cucumber/features/diagnose.feature b/tests/e2e-cucumber/features/diagnose.feature index 777c0c984..28dfd5102 100644 --- a/tests/e2e-cucumber/features/diagnose.feature +++ b/tests/e2e-cucumber/features/diagnose.feature @@ -182,3 +182,31 @@ Feature: Diagnosing failures and listing fixes Given a user who has chosen the fix for the engine-startup import failure When the user previews that fix without applying it Then the printed plan says which shell each step runs in + + # 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-15 - 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 + + # 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-16 - 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 + 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 28f0f9b53..0f4351bc4 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 @@ -40,6 +41,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 @@ -119,6 +129,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")] @@ -164,6 +186,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 @@ -231,6 +281,52 @@ 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); +} + +#[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`. + // + // `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 + .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")] @@ -587,10 +683,15 @@ 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 stderr = world.cli_stderr.as_deref().unwrap_or(""); + assert!( + 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!( - output.contains("This fix only applies on:"), - "the refusal must say which platforms the fix is for:\n{output}" + !stdout.contains("This fix only applies on:"), + "the platform refusal must not also be on stdout:\n{stdout}" ); } @@ -696,14 +797,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}" ); } @@ -716,41 +813,59 @@ 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}" ); } #[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 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!( - output.contains("--yes"), - "the refusal must name what to pass to proceed:\n{output}" + stderr.contains("--yes"), + "the refusal must name what to pass to proceed, on stderr:\n{stderr}" ); assert!( - output.contains("refusing to apply"), - "the refusal must say it did not apply the fix:\n{output}" + 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!( 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{stderr}" + ); +} + +#[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}" ); } @@ -774,3 +889,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}" + ); +} diff --git a/tests/e2e-cucumber/tests/e2e/tui_driver.rs b/tests/e2e-cucumber/tests/e2e/tui_driver.rs index 3ec8d0d85..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::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, args: &[&str], + ) -> Result { + Self::spawn_binary_with_env(world, binary, args, &[]) + } + + fn spawn_binary_with_env( + world: &E2eWorld, + binary: impl AsRef, + args: &[&str], + extra_env: &[(&str, &str)], ) -> Result { 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 @@ -458,20 +485,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 { 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}")),