diff --git a/README.md b/README.md index e95c490df..1b41268a6 100644 --- a/README.md +++ b/README.md @@ -203,7 +203,11 @@ rocm install sdk This downloads TheRock ROCm wheels and a matching PyTorch stack into a managed environment. On machines with an existing ROCm install, `rocm examine` will show it as `legacy_rocm_status: detected_unmanaged` — running `rocm install sdk` -creates a separate managed runtime alongside it. +creates a separate managed runtime alongside it. Running the command when a +managed runtime is already the active default asks first, because the new +install takes over as the active default; see +[ROCm installation](https://github.com/ROCm/rocm-cli/blob/main/README.md#rocm-installation) +for that gate and the flags that approve it without a prompt. Then serve a model: @@ -297,6 +301,7 @@ sometimes because it also needs sudo or a reboot). rocm install sdk [--channel release|nightly] [--format wheel|tarball] [--version x.y.z | --build-date YYYY-MM-DD] [--family gfx110X-all] [--prefix PATH] [--dry-run] + [--approve-replacing-active-default] [--yes] rocm install driver [--dkms] [--yes] [--dry-run] [--reconcile] @@ -305,17 +310,39 @@ rocm update [--apply] [--runtime KEY] [--activate] [--dry-run] ``` `install sdk` downloads TheRock ROCm wheels into a Python environment managed -by rocm-cli. `install driver` installs the AMD kernel driver on Linux (DKMS or -native package). `update` checks for a newer ROCm package; pass `--apply` to -install it, or `--dry-run` to preview what `--apply` would do without changing -anything (`--dry-run` does not require `--apply`). `--runtime` and `--activate` -require `--apply` or `--dry-run` — pass one of those instead of naming a -runtime or requesting activation on its own. `--json` prints the check -result as a single line of JSON instead of text; `--timeout-secs` bounds its -network calls (`--timeout-secs` requires `--json`; both `--json` and -`--timeout-secs` conflict with `--apply`, and `--json` also conflicts with -`--dry-run`). `update --apply` never prompts; `--yes` is accepted for -consistency with other mutating commands but has no effect on it. +by rocm-cli. An install with no active default runtime never prompts, but once a +managed runtime is the active default every `install sdk` asks first, because +the new install takes over as the active default. That gate is not scoped to the +family or channel you are installing: a `--family` or `--channel` you have never +installed before takes over the active default just as a same-family upgrade +does, so it asks too. To approve that non-interactively — in scripts or CI, where +the prompt would otherwise refuse — pass `--approve-replacing-active-default`, +which is also what the refusal itself recommends and what ROCm CLI's own +non-interactive surfaces (chat, MCP, the dashboard) pass. `--yes` grants the same +approval *and* approves installing required system packages (such as OpenMPI for +vLLM), which means `sudo`; reach for it only where something can answer a sudo +password prompt — which an unattended job cannot, unless it has passwordless sudo +configured. In the default managed install root, the root and its manifest are +keyed by version, so an upgrade or downgrade keeps the previous install on disk +and only a same-version reinstall reuses the same root. `--prefix` opts out of +that: the folder you name is used verbatim for every version, so successive +installs into one prefix replace each other in place — and if the venv already +there no longer runs its own Python, it is removed outright and rebuilt. The +consent gate does not cover that: it asks about changing the active default +runtime, not about what a named prefix loses. `install driver` installs the AMD +kernel driver on Linux (DKMS or native package). `update` checks for a newer +ROCm package; pass `--apply` to install it, or `--dry-run` to preview what +`--apply` would do without changing anything (`--dry-run` does not require +`--apply`). `--runtime` and `--activate` require `--apply` or `--dry-run` — pass +one of those instead of naming a runtime or requesting activation on its own. +`--json` prints the check result as a single line of JSON instead of text; +`--timeout-secs` bounds its network calls (`--timeout-secs` requires `--json`; +both `--json` and `--timeout-secs` conflict with `--apply`, and `--json` also +conflicts with `--dry-run`). `update --apply` never prompts and needs no +approval flag: selecting a runtime to update is itself the approval, and it +leaves the active default alone unless you add `--activate`. `update` does +accept `--yes`, for consistency with other mutating commands, but it grants +nothing there — the approval line the update path prints never credits it. ROCm 10 and newer ship from a different source layout. It is opt-in, and asking for it takes two things together: pin the version with `--version`, and name the diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 18d6d1865..be8780bdd 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -623,9 +623,23 @@ rocm install sdk --family gfx110X-all --dry-run")] /// Resolve the install plan without changing files. #[arg(long)] dry_run: bool, - /// Approve required system-package installs (such as OpenMPI for vLLM) without asking. + /// Approve replacing the current active default ROCm runtime (and + /// required system-package installs such as OpenMPI for vLLM) without + /// prompting; required outside an interactive terminal whenever a + /// managed runtime is already the active default — including when this + /// install targets a different GPU family or channel, which takes over + /// the active default just the same. An install with no active default + /// runtime never prompts. #[arg(long)] yes: bool, + /// Approve replacing the current active default ROCm runtime, and only + /// that: unlike --yes it does not approve system-package installs, so it + /// never runs sudo. ROCm CLI's own non-interactive surfaces (chat, MCP, + /// the dashboard) pass this, because they spawn `rocm` with no terminal + /// and so have no way to answer a sudo password prompt; a missing system + /// package stays a warning there, as it was before. --yes implies this. + #[arg(long)] + approve_replacing_active_default: bool, }, /// Preview or install Linux AMD driver support. Driver { @@ -1568,17 +1582,110 @@ fn execute_freeform_next_action( paths: &AppPaths, config: &RocmCliConfig, ) -> Result<()> { - let action = freeform_plan_next_action_with_context(request, paths, config) - .context("natural-language plan did not produce a structured tool call")?; - validate_freeform_execution_action(&action)?; - print!("{}", render_freeform_execution_header(&action)); + let execution = prepare_freeform_execution(request, paths, config)?; + print!("{}", render_freeform_execution_header(&execution)); let mut argv = vec!["rocm".to_owned()]; - argv.extend(action.args); + argv.extend(execution.action.args); let cli = Cli::try_parse_from(argv)?; dispatch(cli) } +/// The argv `execute_freeform_next_action` is about to dispatch, plus whether +/// [`apply_freeform_execution_consent`] added a flag to it. +/// +/// The flag is tracked rather than re-detected from `action.args` because the +/// execution header uses it to tell the operator *why* its `tool_call:` differs +/// from the one in the request plan above. Looking for the flag in the final +/// argv would report "added here" for a plan that already carried it. +pub(crate) struct FreeformExecution { + pub action: FreeformPlanAction, + pub consent_added: bool, +} + +/// Everything `execute_freeform_next_action` decides before it hands the argv to +/// clap: plan, refuse what must not run unattended, and grant the consent the +/// outer `--yes` already carries. +/// +/// Split out so the consent injection is reachable from a test without +/// dispatching a real install — the header render and the `dispatch` call are +/// all that is left above it. +fn prepare_freeform_execution( + request: &str, + paths: &AppPaths, + config: &RocmCliConfig, +) -> Result { + let mut action = freeform_plan_next_action_with_context(request, paths, config) + .context("natural-language plan did not produce a structured tool call")?; + validate_freeform_execution_action(&action)?; + let consent_added = apply_freeform_execution_consent(&mut action.args); + Ok(FreeformExecution { + action, + consent_added, + }) +} + +/// Grant the generated tool call the consent the operator already gave on the +/// outer command line. +/// +/// Only ever reached from `run_freeform` with `approve` set, i.e. from +/// `rocm --yes `. The planner builds a bare +/// `install sdk ...` argv and `execute_freeform_next_action` re-parses and +/// dispatches it **in process**, so `install()` would otherwise run with +/// `yes = false, approve_replacing_active_default = false` no matter what the +/// outer invocation said. That made this surface print `approval: granted by +/// --yes` and then, with an active default runtime, refuse with "re-run with +/// `--approve-replacing-active-default`" — a flag this surface offers no way to +/// pass — or prompt on a terminal it had just said it did not need to ask. +/// +/// Not `--yes`, for the same reason every other internal caller picks the narrow +/// flag: `--yes` on `install sdk` carries a second, unrelated consent for +/// system-package installs that run `sudo`, and the outer `--yes` here means +/// "execute the plan you were just shown", not "install system packages". +/// +/// Injected before the execution header renders, so the printed `tool_call:` is +/// the argv that actually runs, and skipped under `--dry-run`, which returns +/// before the gate and needs no consent — matching the dry-run-aware arms in +/// `chat_rocm_command_action_from_args`, `rocmd` and dash-tui. The plan +/// rendering path is deliberately untouched: `rocm ` without `--yes` +/// prints a command for a human to review, and it must not hand them a +/// pre-approved one. +/// +/// That leaves `rocm --yes ` printing two `tool_call:` lines that +/// differ — `run_freeform` renders the plan section before calling +/// `execute_freeform_next_action`, and the flag is injected between them — and +/// that is also deliberate. The two sections report different things: "request +/// plan" is what the planner derived from the request, and "execution" is the +/// argv handed to clap, so the added consent showing up only under the +/// `execution` header is how this surface discloses that it granted it. Nothing +/// is being solicited in between; under `--yes` the operator already approved +/// on the outer command line, and under no `--yes` the execution section is +/// never reached, so neither line is an approval prompt whose subject could +/// drift from what runs. Injecting into the plan render instead would have to +/// reach `render_structured_request_plan`, which the no-`--yes` review path +/// shares, and would print a pre-approved command to a human being asked to +/// review it — the case the paragraph above rules out. +/// +/// So the difference stays deliberate but stops being unexplained: this returns +/// whether it actually added the flag, and the execution section says so in +/// words. A doc comment reaches the next reader of this file; the operator +/// looking at two `tool_call:` lines that disagree is the one who needs it. +/// +/// Returns `true` only when the flag was not already present, so the disclosure +/// is about a flag this function added and not one the argv arrived with. +fn apply_freeform_execution_consent(args: &mut Vec) -> bool { + let is_install_sdk = args.first().is_some_and(|arg| arg == "install") + && args.get(1).is_some_and(|arg| arg == "sdk"); + if !is_install_sdk || args.iter().any(|arg| arg == "--dry-run") { + return false; + } + let already_present = args + .iter() + .any(|arg| arg == "--approve-replacing-active-default"); + ensure_flag(args, "--approve-replacing-active-default"); + !already_present +} + fn validate_freeform_execution_action(action: &FreeformPlanAction) -> Result<()> { if action.has_placeholders { bail!( @@ -1594,7 +1701,8 @@ fn validate_freeform_execution_action(action: &FreeformPlanAction) -> Result<()> Ok(()) } -fn render_freeform_execution_header(action: &FreeformPlanAction) -> String { +fn render_freeform_execution_header(execution: &FreeformExecution) -> String { + let action = &execution.action; let mut output = String::new(); let _ = writeln!(output); let _ = writeln!(output, "execution"); @@ -1612,6 +1720,20 @@ fn render_freeform_execution_header(action: &FreeformPlanAction) -> String { " tool_call: {}", format_structured_tool_call("rocm", &action.args) ); + // Printed only when the two `tool_call:` lines actually disagree, and + // immediately under the one that runs. Without it the operator sees a + // consent flag on the executed command that the request plan above never + // showed, with nothing on screen saying where it came from — the natural + // reading being that something was approved behind their back rather than + // that their own `--yes` was carried through. + if execution.consent_added { + let _ = writeln!( + output, + " note: --approve-replacing-active-default was added here from your --yes, so this \ + tool_call differs from the one under `request plan` above; nothing was approved \ + between them." + ); + } output } @@ -2648,7 +2770,9 @@ fn install(target: InstallTarget) -> Result<()> { family, dry_run, yes, + approve_replacing_active_default, } => { + let consents = SdkInstallConsents::resolve(yes, approve_replacing_active_default); let format_name = match format { InstallFormat::Wheel => "wheel", InstallFormat::Tarball => "tarball", @@ -2669,12 +2793,14 @@ fn install(target: InstallTarget) -> Result<()> { version_selector, family.as_deref(), dry_run, + consents.replace_active_default, ) { - Ok(output) => { - let finalized = if dry_run { - None - } else { + Ok(result) => { + let therock::SdkInstallResult { output, mutated } = result; + let finalized = if mutated { finalize_successful_sdk_install(&paths)? + } else { + None }; print!("{output}"); if let Some(finalized) = &finalized { @@ -2684,8 +2810,8 @@ fn install(target: InstallTarget) -> Result<()> { // runtime (libnuma.so.1 / libnuma_1.2). Ensure both are // present for every SDK install, independent of which // engine (if any) is auto-installed below. - ensure_libatomic_for_torch(yes); - ensure_libnuma_for_torch(yes); + ensure_libatomic_for_torch(consents.system_packages); + ensure_libnuma_for_torch(consents.system_packages); } finish_sdk_install( &paths, @@ -2695,11 +2821,26 @@ fn install(target: InstallTarget) -> Result<()> { } else { "install_sdk" }, - format!( - "sdk install completed channel={channel} format={format_name} prefix={prefix_display} version_selector={version_selector_display} dry_run={dry_run}" - ), + { + // A real install that did not mutate the system was + // declined at the approval prompt; recording it as + // "completed" would lie in the audit trail. Dry-run + // never mutates but legitimately completes a preview. + let status = if dry_run || mutated { + "completed" + } else { + "cancelled" + }; + format!( + "sdk install {status} channel={channel} format={format_name} prefix={prefix_display} version_selector={version_selector_display} dry_run={dry_run}" + ) + }, |paths, finalized| { - maybe_auto_install_sdk_preferred_engine(paths, finalized, yes) + maybe_auto_install_sdk_preferred_engine( + paths, + finalized, + consents.system_packages, + ) }, )?; } @@ -7837,6 +7978,93 @@ fn preferred_engine_for_sdk_family(family: &str) -> Option<&'static str> { preferred_serve_engine_for_host_gpu_summary(&summary) } +/// The two unrelated consents `rocm install sdk` can be given. +/// +/// They are separate because they authorize different things and are answerable +/// in different places. Replacing the active default runtime is a decision, and +/// an argv can express it fully. Approving a system-package install means +/// approving `sudo`, which — unless the host is root or has passwordless sudo — +/// needs a human at a terminal to type a password. +/// +/// `--yes` grants both, which is what a user typing it at a terminal means. +/// ROCm CLI's own non-interactive surfaces need only the first: they spawn +/// `rocm` with null stdin, so a sudo password prompt there can never be +/// answered, and treating their approval as covering it would run sudo they +/// cannot complete — and, for the vLLM/OpenMPI plan, abort the engine +/// auto-install that used to warn and continue. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +struct SdkInstallConsents { + /// Approve replacing whatever runtime is currently the active default, and + /// which flag granted it. The source is carried rather than flattened to a + /// bool because the install log names it: crediting `--yes` on a surface + /// that only ever passed the narrow flag would tell the reader that consent + /// to run `sudo` had been given when it had not. + replace_active_default: therock::SdkInstallConsent, + /// Approve installing required system packages (OpenMPI, libatomic, + /// libnuma) through the system package manager, which means `sudo`. + system_packages: bool, +} + +impl SdkInstallConsents { + const fn resolve(yes: bool, approve_replacing_active_default: bool) -> Self { + let replace_active_default = if yes { + therock::SdkInstallConsent::Preapproved(therock::SdkInstallApprovalSource::AssumeYes) + } else if approve_replacing_active_default { + therock::SdkInstallConsent::Preapproved( + therock::SdkInstallApprovalSource::ApproveReplacingActiveDefault, + ) + } else { + therock::SdkInstallConsent::Ask + }; + Self { + replace_active_default, + system_packages: yes, + } + } +} + +/// What to do with a distro-aware system-package install plan, given the caller's +/// approval and what this host lets us do without a password. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum SystemPackageInstallAction { + /// Print the commands (and the preflight checks) and continue without them. + /// Nothing privileged is run, so nothing can block on a password prompt. + PrintManualCommands, + /// Run the plan. `run_system_package_install_plan` inherits stdin so an + /// interactive `sudo` password prompt can be answered. + RunPlan { + /// The `approval:` line explaining why this was allowed to run. + approval: &'static str, + /// Whether a failed command is an error rather than a warning. Only an + /// explicit approval escalates: the automatic (root/passwordless) path + /// must never let a missing system package fail an unattended install. + escalate_failure: bool, + }, +} + +/// Decide between the two, given whether the *system-package* consent was granted +/// and whether the host can install without prompting. +/// +/// Split out from the two callers below so the decision is testable without a +/// package manager, and so the "approved but no way to answer a password prompt" +/// case has one place to be reasoned about. +const fn system_package_install_action( + approved: bool, + can_autoinstall: bool, +) -> SystemPackageInstallAction { + if !approved && !can_autoinstall { + return SystemPackageInstallAction::PrintManualCommands; + } + SystemPackageInstallAction::RunPlan { + approval: if approved { + "granted by --yes" + } else { + "auto (root or passwordless sudo available)" + }, + escalate_failure: approved, + } +} + /// Ensure the OpenMPI runtime that vLLM requires is present before the vLLM wheel /// is installed. On Linux/WSL, when OpenMPI is missing, this installs it through /// the system package manager. @@ -7887,7 +8115,11 @@ fn ensure_openmpi_for_vllm(approved: bool) -> Result<()> { } let can_autoinstall = rocm_core::openmpi::can_autoinstall(); - if !approved && !can_autoinstall { + let SystemPackageInstallAction::RunPlan { + approval, + escalate_failure, + } = system_package_install_action(approved, can_autoinstall) + else { for check in &plan.preflight_checks { println!(" preflight: {check}"); } @@ -7896,16 +8128,9 @@ fn ensure_openmpi_for_vllm(approved: bool) -> Result<()> { "warning: passwordless sudo is unavailable; run the commands above manually, or rerun with --yes to approve an interactive sudo prompt" ); return Ok(()); - } + }; - println!( - " approval: {}", - if approved { - "granted by --yes" - } else { - "auto (root or passwordless sudo available)" - } - ); + println!(" approval: {approval}"); match run_system_package_install_plan(&plan) { Ok(()) => { if rocm_core::openmpi::detect_openmpi().present { @@ -7923,7 +8148,15 @@ fn ensure_openmpi_for_vllm(approved: bool) -> Result<()> { // past something the user asked for. The auto (unapproved) path keeps // the warn-and-continue behavior so a missing OpenMPI never blocks an // otherwise-unattended install. - if approved { + // + // "Surface" is the exact claim, and it is not the same as failing the + // command: this error propagates out of the engine auto-install, and + // `finish_sdk_install` routes it through + // `engine_auto_install_failure_is_fatal`, which matches only + // `UnusableRuntimeAfterInstall`. So `rocm install sdk` still prints + // the failure and exits 0. Said here because the downgrade happens + // far away and reads as a non-zero exit from this site alone. + if escalate_failure { return Err(error.context( "OpenMPI install approved with --yes failed; rerun the commands above manually or retry without --yes to continue without OpenMPI", )); @@ -8034,7 +8267,11 @@ fn ensure_torch_runtime_dep(approved: bool, dep: &TorchRuntimeDep) { } let can_autoinstall = rocm_core::openmpi::can_autoinstall(); - if !approved && !can_autoinstall { + // `escalate_failure` is deliberately ignored here: a missing libatomic/libnuma + // only warns, whatever approved the attempt. + let SystemPackageInstallAction::RunPlan { approval, .. } = + system_package_install_action(approved, can_autoinstall) + else { for check in &plan.preflight_checks { println!(" preflight: {check}"); } @@ -8046,16 +8283,9 @@ fn ensure_torch_runtime_dep(approved: bool, dep: &TorchRuntimeDep) { "warning: passwordless sudo is unavailable; run the commands above manually, or rerun with --yes to approve an interactive sudo prompt" ); return; - } + }; - println!( - " approval: {}", - if approved { - "granted by --yes" - } else { - "auto (root or passwordless sudo available)" - } - ); + println!(" approval: {approval}"); match run_system_package_install_plan(&plan) { Ok(()) => { if (dep.present)() { @@ -9964,7 +10194,7 @@ fn recover_setup_runtime_registration( Ok(Some(manifest.runtime_key)) } -fn current_runtime_manifest<'a>( +pub(crate) fn current_runtime_manifest<'a>( config: &RocmCliConfig, manifests: &'a [therock::InstalledRuntimeManifest], ) -> Option<&'a therock::InstalledRuntimeManifest> { @@ -12375,6 +12605,42 @@ fn chat_rocm_command_action_from_args(mut args: Vec) -> Result { + // The chat/MCP surfaces spawn `rocm` with null stdin, so + // `interactive_terminal()` is false and the consent prompt would + // refuse with a "re-run with `--approve-replacing-active-default`" + // error, which the schema-constrained `install_sdk` tool gives the + // user no way to answer. + // + // Not `--yes`: that flag also approves system-package installs, and + // this spawn has no terminal on which to answer the `sudo` password + // prompt such an install can raise. Granting it here would make the + // vLLM/OpenMPI step run a sudo it cannot complete and abort the + // engine auto-install that previously warned and continued. The + // narrow flag grants exactly the consent the prompt is asking for. + // + // `--yes` is stripped rather than merely not added, because the + // generic `rocm_command` tool takes a model-supplied argv: a + // model-emitted `--yes` would otherwise reach this null-stdin spawn + // and re-grant the system-package consent `76c6aa3c` removed. The + // strip runs before the argv is rendered for human approval, so what + // is shown is still what runs. + // + // The `--yes=...` form is stripped too. Clap rejects an attached + // value on this flag today, so an exact-match strip happens to be + // airtight — but only by borrowing a property of clap's error + // taxonomy that nothing here owns. Giving `--yes` `num_args`, or a + // clap release that starts accepting `--yes=true` on a bare `bool`, + // would silently restore the sudo consent this strip exists to + // remove. Matching the prefix keeps the guarantee local to this + // function. + args.retain(|arg| arg != "--yes" && !arg.starts_with("--yes=")); + // Withheld on `--dry-run`, which returns before the consent gate and + // so needs no consent: `rocmd` and dash-tui omit it there for the + // same reason, and a preview should not be recorded as carrying an + // approval it never used. + if !args.iter().any(|arg| arg == "--dry-run") { + ensure_flag(&mut args, "--approve-replacing-active-default"); + } Ok(ChatRocmCommandAction::Approval { args, pending_title: "Install ROCm".to_owned(), @@ -13848,7 +14114,23 @@ fn render_install_sdk_dry_run_for_args(paths: &AppPaths, args: &[String]) -> Res let version = chat_cli_arg_value(args, "--version").map(str::to_owned); let build_date = chat_cli_arg_value(args, "--build-date").map(str::to_owned); let selector = therock_install_version_selector(version, build_date)?; - therock::install_sdk(paths, channel, format, prefix, selector, None, true) + // Dry run, so nothing is displaced and the consent gate is never reached; + // the narrow consent is what this chat surface would pass for a real + // install, and passing `--yes`'s source here would be a lie waiting to be + // printed if the preview ever grew a gate. + Ok(therock::install_sdk( + paths, + channel, + format, + prefix, + selector, + None, + true, + therock::SdkInstallConsent::Preapproved( + therock::SdkInstallApprovalSource::ApproveReplacingActiveDefault, + ), + )? + .output) } fn run_command_with_timeout( @@ -14216,6 +14498,12 @@ fn rocm_chat_tool_requested_args(call: &providers::ChatToolCall) -> Option` dispatches the generated argv **in process**, so + // nothing carries the outer `--yes` into `install()` unless this does. + // Without it the surface prints `approval: granted by --yes` and then + // either refuses non-interactively, asking for a flag it offers no way to + // pass, or prompts on a terminal it just said it would not need to ask. + let request = + "install the latest TheRock nightly for this GPU into D:\\ROCm\\therock_venvs"; + let config = RocmCliConfig::default(); + + let planned = freeform_plan_next_action(request, &config) + .expect("install request should have next action"); + assert!( + !planned + .args + .iter() + .any(|arg| arg.starts_with("--yes") || arg.starts_with("--approve-")), + "the plan itself must stay unapproved so `rocm ` shows a \ + reviewable command: {:?}", + planned.args + ); + + // Through the real pre-dispatch path, not the injector in isolation: + // `execute_freeform_next_action` is this plus the header render and + // `dispatch`, so dropping the injection from the pipeline fails here. + let execution = prepare_freeform_execution(request, &test_app_paths(), &config) + .expect("install request should prepare for execution"); + let action = &execution.action; + + assert_eq!( + format_structured_tool_call("rocm", &action.args), + "rocm install sdk --channel nightly --format wheel --prefix \ + D:\\ROCm\\therock_venvs --approve-replacing-active-default" + ); + // The narrow flag, never `--yes`: this surface has no terminal promise to + // make about a sudo password prompt for system packages. + assert!(!action.args.iter().any(|arg| arg == "--yes")); + // The header renders after injection, so the printed tool call is the + // argv that actually runs. + let rendered = render_freeform_execution_header(&execution); + assert!(rendered.contains("--approve-replacing-active-default")); + // And the operator is told why this `tool_call:` carries a consent flag + // the `request plan` section above it did not show. The plan assertion at + // the top of this test is what makes the two lines differ here, so the + // disclosure and the difference are pinned by the same test. + assert!( + execution.consent_added, + "the plan arrived unapproved, so the injector must report adding the flag" + ); + assert!( + rendered.contains("was added here from your --yes"), + "the execution section must explain the differing tool_call: {rendered}" + ); + // Re-parsing must reach `install()` with the consent actually set. + let mut argv = vec!["rocm".to_owned()]; + argv.extend(execution.action.args); + let cli = Cli::try_parse_from(argv).expect("generated argv should parse"); + match cli.command { + Some(Command::Install { + target: + InstallTarget::Sdk { + yes, + approve_replacing_active_default, + .. + }, + }) => { + assert!(approve_replacing_active_default); + assert!(!yes); + } + other => panic!("expected `install sdk`, got {other:?}"), + } + } + + #[test] + fn freeform_execution_consent_is_scoped_to_mutating_sdk_installs() { + // Dry runs return before the consent gate, and the sibling dry-run-aware + // arms in chat, rocmd and dash-tui all withhold the flag there. + let mut dry_run = vec![ + "install".to_owned(), + "sdk".to_owned(), + "--dry-run".to_owned(), + ]; + assert!(!apply_freeform_execution_consent(&mut dry_run)); + assert_eq!( + dry_run, + vec![ + "install".to_owned(), + "sdk".to_owned(), + "--dry-run".to_owned() + ] + ); + + // Nothing else the planner can emit takes this flag; injecting it would + // not even parse. + for mut args in [ + vec!["install".to_owned(), "driver".to_owned()], + vec!["serve".to_owned(), "qwen".to_owned()], + vec!["comfyui".to_owned(), "install".to_owned()], + ] { + let before = args.clone(); + assert!(!apply_freeform_execution_consent(&mut args)); + assert_eq!(args, before); + } + + // Idempotent: a plan that already carries the flag is not given it twice, + // and reports that it added nothing — the execution section must not + // claim to have added a flag the argv arrived with. + let mut already = vec![ + "install".to_owned(), + "sdk".to_owned(), + "--approve-replacing-active-default".to_owned(), + ]; + assert!(!apply_freeform_execution_consent(&mut already)); + assert_eq!( + already + .iter() + .filter(|arg| *arg == "--approve-replacing-active-default") + .count(), + 1 + ); + } + #[test] fn freeform_execution_header_surfaces_explicit_approval_and_tool_call() { let action = freeform_plan_next_action("serve qwen3.5 with vllm", &RocmCliConfig::default()) .expect("serve request should have next action"); - let rendered = render_freeform_execution_header(&action); + let rendered = render_freeform_execution_header(&FreeformExecution { + action, + consent_added: false, + }); assert!(rendered.contains("execution")); assert!(rendered.contains("approval: granted by --yes")); assert!(rendered.contains( "tool_call: rocm serve Qwen/Qwen3.5-4B --engine vllm --device gpu_required --managed" )); + // Nothing was injected on this path, so the two `tool_call:` lines agree + // and the disclosure would be noise that contradicts the plan above. + assert!( + !rendered.contains("was added here"), + "the note must be scoped to an argv this surface actually changed: {rendered}" + ); } #[test] @@ -22734,7 +23268,7 @@ mod tests { assert_eq!( rocm_chat_tool_requested_command(&call).as_deref(), Some( - "rocm install sdk --channel release --format wheel --prefix D:\\ROCm\\therock_venvs" + "rocm install sdk --channel release --format wheel --approve-replacing-active-default --prefix D:\\ROCm\\therock_venvs" ) ); let approval = chat_tool_approval_request( @@ -22757,12 +23291,167 @@ mod tests { "release".to_owned(), "--format".to_owned(), "wheel".to_owned(), + "--approve-replacing-active-default".to_owned(), "--prefix".to_owned(), "D:\\ROCm\\therock_venvs".to_owned(), ] ); } + #[test] + fn chat_install_sdk_strips_model_supplied_yes_and_skips_consent_on_dry_run() { + // `rocm_command` carries a model-supplied argv that nothing else filters, + // so `--yes` can arrive here. This arm exists to grant the narrow consent + // only; letting `--yes` through would re-grant the system-package/sudo + // consent on a spawn with no terminal to answer a password prompt. + let classify = |args: &[&str]| -> Vec { + let action = chat_rocm_command_action_from_args( + args.iter().copied().map(str::to_owned).collect(), + ) + .expect("install sdk should classify"); + let ChatRocmCommandAction::Approval { args, .. } = action else { + panic!("install sdk is a mutating command"); + }; + args + }; + + assert_eq!( + classify(&["install", "sdk", "--prefix", "/tmp/therock", "--yes"]), + vec![ + "install".to_owned(), + "sdk".to_owned(), + "--prefix".to_owned(), + "/tmp/therock".to_owned(), + "--approve-replacing-active-default".to_owned(), + ] + ); + + // A dry run returns before the consent gate, so it is not given a consent + // it never uses — matching the dry-run-aware sibling arms. + assert_eq!( + classify(&["install", "sdk", "--prefix", "/tmp/therock", "--dry-run"]), + vec![ + "install".to_owned(), + "sdk".to_owned(), + "--prefix".to_owned(), + "/tmp/therock".to_owned(), + "--dry-run".to_owned(), + ] + ); + + // Both together: the strip is unconditional, so `--yes` still does not + // survive into a preview spawn, and the dry run still gains no consent. + // A model that emits both must not end up with either flag. + assert_eq!( + classify(&[ + "install", + "sdk", + "--prefix", + "/tmp/therock", + "--yes", + "--dry-run", + ]), + vec![ + "install".to_owned(), + "sdk".to_owned(), + "--prefix".to_owned(), + "/tmp/therock".to_owned(), + "--dry-run".to_owned(), + ] + ); + } + + #[test] + fn chat_install_sdk_strips_a_model_supplied_yes_in_both_its_bare_and_attached_forms() { + // `--yes` and `--yes=true` are both model-supplied argv that reach the + // chat arm intact: neither `canonicalize_chat_rocm_command` nor + // `validate_chat_rocm_command_safety` splits or rejects either. Whichever + // form survives re-grants into a null-stdin spawn the system-package/sudo + // consent `76c6aa3c` removed, with no terminal to answer the password + // prompt, so the strip has to catch both. + let classify = |args: &[&str]| -> Vec { + let action = chat_rocm_command_action_from_args( + args.iter().copied().map(str::to_owned).collect(), + ) + .expect("install sdk should classify"); + let ChatRocmCommandAction::Approval { args, .. } = action else { + panic!("install sdk is a mutating command"); + }; + args + }; + + // Both terms of `arg != "--yes" && !arg.starts_with("--yes=")` are driven + // here, and each alone: the bare form is caught only by the first, the + // `=` forms only by the second, so dropping either term reddens this test + // on its own rather than leaving one half to a sibling. + for supplied in ["--yes", "--yes=true", "--yes=1", "--yes=false"] { + let args = classify(&["install", "sdk", "--prefix", "/tmp/therock", supplied]); + assert!( + !args.iter().any(|arg| arg.starts_with("--yes")), + "`{supplied}` must not survive the chat strip, got {args:?}" + ); + assert_eq!( + args, + vec![ + "install".to_owned(), + "sdk".to_owned(), + "--prefix".to_owned(), + "/tmp/therock".to_owned(), + "--approve-replacing-active-default".to_owned(), + ], + "stripping `{supplied}` must leave the rest of the argv and the narrow consent alone" + ); + } + + // Future-proofing, not a guard on the `--yes=` term this test's other + // assertions pin: `--yes-not-a-flag` survives both the exact-match strip + // that preceded that term and the two-term strip that replaced it, so it + // would pass on either. What it does catch is the next edit — widening + // the second term to `starts_with("--yes")` to "simplify" it would start + // eating every argv token that merely begins the same way, silently + // dropping arguments the model legitimately sent. + let args = classify(&[ + "install", + "sdk", + "--prefix", + "/tmp/therock", + "--yes-not-a-flag", + ]); + assert!( + args.iter().any(|arg| arg == "--yes-not-a-flag"), + "the strip must match `--yes` and `--yes=…`, not every token starting with \ + `--yes`, got {args:?}" + ); + + // Second layer, and only the second: clap also refuses an attached value + // on this flag, so even an unstripped `--yes=true` would not parse today. + // That is what the strip above deliberately stops depending on — pinned + // here so a later `num_args` on `--yes` shows up as a failure of the + // backstop rather than passing unnoticed. + for attached in ["--yes=true", "--yes=1", "--yes=false"] { + let error = match Cli::try_parse_from(["rocm", "install", "sdk", attached]) { + Ok(cli) => panic!("`{attached}` must not parse, got {cli:?}"), + Err(error) => error, + }; + assert_eq!( + error.kind(), + clap::error::ErrorKind::TooManyValues, + "expected clap to reject the attached value on {attached}: {error}" + ); + } + + // Control: the bare form does parse, so the assertions above are about + // the `=`-form and not about `--yes` being rejected outright. + let cli = Cli::try_parse_from(["rocm", "install", "sdk", "--yes"]) + .expect("the bare flag is the form the chat arm strips"); + match cli.command { + Some(Command::Install { + target: InstallTarget::Sdk { yes, .. }, + }) => assert!(yes), + other => panic!("expected `install sdk`, got {other:?}"), + } + } + #[test] fn chat_tool_call_mutating_install_accepts_requested_build_date() { let call = providers::ChatToolCall { @@ -22779,7 +23468,7 @@ mod tests { assert_eq!( rocm_chat_tool_requested_command(&call).as_deref(), Some( - "rocm install sdk --channel release --format wheel --prefix D:\\ROCm\\therock_venvs --build-date 06052026" + "rocm install sdk --channel release --format wheel --prefix D:\\ROCm\\therock_venvs --build-date 06052026 --approve-replacing-active-default" ) ); let approval = @@ -22799,6 +23488,7 @@ mod tests { "D:\\ROCm\\therock_venvs".to_owned(), "--build-date".to_owned(), "06052026".to_owned(), + "--approve-replacing-active-default".to_owned(), ] ); } @@ -23256,7 +23946,7 @@ model recipes }), }, Some( - "rocm install sdk --channel release --format wheel --prefix D:\\ROCm\\therock_venvs", + "rocm install sdk --channel release --format wheel --approve-replacing-active-default --prefix D:\\ROCm\\therock_venvs", ), false, ), @@ -23818,6 +24508,143 @@ model recipes } } + /// Resolve the argv a non-interactive surface produces the way `install()` + /// does, so a test can assert what that argv actually consents to rather + /// than which flag string it happens to contain. + fn consents_for_install_sdk_argv(args: &[String]) -> SdkInstallConsents { + let cli = + Cli::try_parse_from(std::iter::once("rocm".to_owned()).chain(args.iter().cloned())) + .unwrap_or_else(|error| { + panic!("{args:?} must parse as a `rocm` invocation: {error}") + }); + let Some(Command::Install { + target: + InstallTarget::Sdk { + yes, + approve_replacing_active_default, + .. + }, + }) = cli.command + else { + panic!("{args:?} is not an `install sdk` invocation"); + }; + SdkInstallConsents::resolve(yes, approve_replacing_active_default) + } + + #[test] + fn install_sdk_chat_and_mcp_args_approve_the_replacement_for_a_non_interactive_spawn() { + // The chat/MCP surfaces spawn `rocm` with null stdin, so the consent + // prompt would refuse with "re-run with + // `--approve-replacing-active-default`" — a flag the user has no way to + // supply from chat or the dashboard. Both the chat classifier + // arm and the MCP tool-args builder must inject the consent so an + // install over the active default runtime is not silently refused. + let action = chat_rocm_command_action_from_args(vec![ + "install".to_owned(), + "sdk".to_owned(), + "--channel".to_owned(), + "release".to_owned(), + // The classifier requires a user-chosen (non-system) install folder. + "--prefix".to_owned(), + "/home/tester/rocm-managed".to_owned(), + ]) + .expect("install sdk classifies"); + let chat_args = match action { + ChatRocmCommandAction::Approval { args, .. } => args, + other @ ChatRocmCommandAction::ReadOnly(_) => { + panic!("install sdk must require approval, got {other:?}") + } + }; + + // The MCP `install_sdk` tool builds its own argv (it does not route + // through the classifier above), so it must add the flag independently. + let call = providers::ChatToolCall { + id: None, + name: "install_sdk".to_owned(), + arguments: serde_json::json!({ "channel": "release", "format": "wheel" }), + }; + let mcp_args = rocm_chat_tool_requested_args(&call).expect("install_sdk tool builds args"); + + for args in [&chat_args, &mcp_args] { + assert_eq!( + consents_for_install_sdk_argv(args).replace_active_default, + therock::SdkInstallConsent::Preapproved( + therock::SdkInstallApprovalSource::ApproveReplacingActiveDefault + ), + "the spawn must approve replacing the active default, and be credited \ + to the flag it actually passed rather than to --yes, got {args:?}" + ); + } + } + + #[test] + fn an_injected_consent_does_not_approve_privileged_package_installs() { + // The worked regression: Linux, a vLLM-preferred GPU family, OpenMPI + // absent, and neither root nor passwordless sudo. The chat, MCP and + // daemon surfaces spawn `rocm` with null stdin, so there is no terminal + // on which a `sudo` password prompt could ever be answered. Injecting + // `--yes` to clear the runtime-replacement prompt used to grant the + // second, unrelated consent that flag carries, which made + // `ensure_openmpi_for_vllm` run a sudo it cannot complete and then + // escalate the failure — aborting `maybe_auto_install_sdk_preferred_engine` + // before the vLLM engine install that previously warned and continued. + let call = providers::ChatToolCall { + id: None, + name: "install_sdk".to_owned(), + arguments: serde_json::json!({ "channel": "release", "format": "wheel" }), + }; + let injected = consents_for_install_sdk_argv( + &rocm_chat_tool_requested_args(&call).expect("install_sdk tool builds args"), + ); + assert_eq!( + injected.replace_active_default, + therock::SdkInstallConsent::Preapproved( + therock::SdkInstallApprovalSource::ApproveReplacingActiveDefault + ) + ); + assert!( + !injected.system_packages, + "an injected consent must not approve a privileged package install" + ); + assert_eq!( + system_package_install_action(injected.system_packages, false), + SystemPackageInstallAction::PrintManualCommands, + "on a host without passwordless sudo the spawn must print the commands, not run sudo" + ); + + // And the other half of the property: a `--yes` the user actually typed + // still approves both, and a failure of the install it asked for is + // still an error rather than a warning. + let typed = consents_for_install_sdk_argv(&[ + "install".to_owned(), + "sdk".to_owned(), + "--yes".to_owned(), + ]); + assert_eq!( + typed.replace_active_default, + therock::SdkInstallConsent::Preapproved(therock::SdkInstallApprovalSource::AssumeYes), + "a --yes the user typed must still be credited to --yes" + ); + assert!(typed.system_packages); + assert_eq!( + system_package_install_action(typed.system_packages, false), + SystemPackageInstallAction::RunPlan { + approval: "granted by --yes", + escalate_failure: true, + }, + ); + + // Root or passwordless sudo installs without any approval, as before — + // the consent split must not have made the automatic path conditional. + assert_eq!( + system_package_install_action(injected.system_packages, true), + SystemPackageInstallAction::RunPlan { + approval: "auto (root or passwordless sudo available)", + escalate_failure: false, + }, + ); + } + #[test] fn chat_rocm_command_runs_read_only_and_rejects_risky_shapes() { let status = providers::ChatToolCall { diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index a696cd942..e8aa7b9a2 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -5,13 +5,14 @@ use anyhow::{Context, Result, bail}; use rocm_core::{ AppPaths, ManagedToolConfig, RUNTIME_LIBRARY_PATH_ENV, RocmCliConfig, detect_host_gfx_target, - detect_host_gpu_diagnostics, detect_managed_therock_family, disk_space, ensure_uv_binary, - extract_first_gfx_token, known_therock_families, managed_tools_dir, - normalize_runtime_path_for_host, normalize_runtime_path_for_storage, - normalize_runtime_path_text_for_host, normalize_runtime_path_text_for_storage, - normalize_therock_family, runtime_is_windows, runtime_os_name, runtime_path_for_windows_child, - runtime_path_list_split, runtime_python_executable_in_env, unix_time_millis, uv_command_env, - uv_pip_install_base, uv_venv_args, verify_rsa_pkcs1_sha256_signature, + detect_host_gpu_diagnostics, detect_legacy_rocm_summary, detect_managed_therock_family, + disk_space, ensure_uv_binary, extract_first_gfx_token, interactive_terminal, + known_therock_families, managed_tools_dir, normalize_runtime_path_for_host, + normalize_runtime_path_for_storage, normalize_runtime_path_text_for_host, + normalize_runtime_path_text_for_storage, normalize_therock_family, runtime_is_windows, + runtime_os_name, runtime_path_for_windows_child, runtime_path_list_split, + runtime_python_executable_in_env, unix_time_millis, uv_command_env, uv_pip_install_base, + uv_venv_args, verify_rsa_pkcs1_sha256_signature, }; #[cfg(test)] use rocm_core::{ @@ -504,6 +505,12 @@ struct PipRuntimeResolution { /// this resolution produces records the stream that produced it. layout: SourceLayout, latest_version: String, + /// Newest `rocm` version offered by the repository for this channel, + /// regardless of whether it has a matching PyTorch wheel stack. When this is + /// newer than `latest_version`, the repo's newest release could not be + /// installed (no wheels) and we warn about it. `None` when a specific version + /// was requested (the "latest" concept does not apply). + newest_repo_version: Option, package_versions: TheRockPipPackageVersions, /// The device payload the resolved source must supply for this host, /// decided against the targets that source actually publishes. @@ -918,12 +925,49 @@ enum VersionStage { Stable, } +/// Outcome of an `install sdk` request. +/// +/// `mutated` is `false` for dry-run plans and for installs the user declined at +/// the confirmation prompt, so the caller can skip the post-install activation +/// and success reporting that only make sense after a real install. +#[derive(Debug)] +pub(crate) struct SdkInstallResult { + pub output: String, + pub mutated: bool, +} + +impl SdkInstallResult { + const fn plan(output: String) -> Self { + Self { + output, + mutated: false, + } + } + + const fn installed(output: String) -> Self { + Self { + output, + mutated: true, + } + } +} + #[derive(Clone, Copy, Debug, Default)] struct InstallSourceOverride<'a> { family: Option<&'a str>, device_target: Option<&'a str>, layout: Option, } + +/// Install a TheRock SDK runtime. +/// +/// `consent` carries the *source* of any up-front approval rather than a bare +/// bool, because the progress line names it: `--yes` and +/// `--approve-replacing-active-default` both clear this gate, but only the +/// former also approves a `sudo` system-package install, so collapsing them +/// would print "Approved by --yes" on every install ROCm CLI's own +/// terminal-less surfaces make. +#[allow(clippy::too_many_arguments)] pub(crate) fn install_sdk( paths: &AppPaths, channel: &str, @@ -932,7 +976,8 @@ pub(crate) fn install_sdk( version_selector: Option, family_override: Option<&str>, dry_run: bool, -) -> Result { + consent: SdkInstallConsent, +) -> Result { let channel = TheRockChannel::parse(channel)?; ensure_install_format_supported(format)?; match format { @@ -946,6 +991,7 @@ pub(crate) fn install_sdk( }, version_selector.as_ref(), dry_run, + consent, ), "tarball" => { // A tarball catalog lists whole archives, not a resolvable package @@ -967,6 +1013,7 @@ pub(crate) fn install_sdk( version_selector.as_ref(), None, dry_run, + consent, ) } other => bail!("unsupported install format: {other}"), @@ -975,6 +1022,19 @@ pub(crate) fn install_sdk( /// Apply an update using the exact family, device payload, and source layout /// resolved by its plan. +/// +/// Consent is preapproved rather than asked for: the update targets the runtime +/// the user selected (or the active default), so installing over it is the +/// operation requested, and `rocm update` has no terminal contract — reaching a +/// prompt here would only fail the command. `rocm update` does accept a `--yes` +/// flag for consistency with other mutating commands, but it is inert and never +/// reaches this function, so it grants nothing here. +/// +/// `activate_after_install` mirrors `rocm update --apply --activate` so the +/// approval line can state what will actually happen. Without it, +/// `apply_runtime_update` installs beside the active default and leaves the +/// default untouched, so the line must not claim an activation. +#[allow(clippy::too_many_arguments)] pub(crate) fn install_sdk_for_update( paths: &AppPaths, channel: &str, @@ -983,9 +1043,13 @@ pub(crate) fn install_sdk_for_update( device_target: Option<&str>, source_layout_generation: Option<&str>, dry_run: bool, -) -> Result { + activate_after_install: bool, +) -> Result { let channel = TheRockChannel::parse(channel)?; ensure_install_format_supported(format)?; + let consent = SdkInstallConsent::Preapproved(SdkInstallApprovalSource::UpdateApply { + activates: activate_after_install, + }); let layout = Some(SourceLayout::from_generation(source_layout_generation)?); match format { "wheel" => install_wheel_runtime( @@ -999,10 +1063,18 @@ pub(crate) fn install_sdk_for_update( }, None, dry_run, + consent, + ), + "tarball" => install_tarball_runtime( + paths, + channel, + None, + Some(family), + None, + layout, + dry_run, + consent, ), - "tarball" => { - install_tarball_runtime(paths, channel, None, Some(family), None, layout, dry_run) - } other => bail!("unsupported install format: {other}"), } } @@ -1560,6 +1632,7 @@ fn save_startup_update_check(paths: &AppPaths, record: &StartupUpdateCheckRecord ) } +#[allow(clippy::too_many_arguments)] fn install_wheel_runtime( paths: &AppPaths, channel: TheRockChannel, @@ -1567,7 +1640,8 @@ fn install_wheel_runtime( source_override: InstallSourceOverride<'_>, version_selector: Option<&RuntimeVersionSelector>, dry_run: bool, -) -> Result { + consent: SdkInstallConsent, +) -> Result { let InstallSourceOverride { family: family_override, device_target: device_target_override, @@ -1672,6 +1746,20 @@ fn install_wheel_runtime( " latest_compatible_version: {}", runtime_version_display(&resolution.latest_version) ); + // Probed once and reused by the progress line below: each call is a full + // filesystem scan for a legacy ROCm, and the summary block and the visible + // note report the same answer about the same resolved version. + let host_version_newer = host_rocm_version_newer_than(&resolution.latest_version); + if let Some(host_version) = host_version_newer.as_deref() { + let _ = writeln!( + output, + " version_note: {}", + wheel_host_version_note( + host_version, + &runtime_version_display(&resolution.latest_version) + ) + ); + } let _ = writeln!( output, " compatibility_key: {}", @@ -1704,6 +1792,19 @@ fn install_wheel_runtime( output, " package_policy: resolve the pinned target-complete rocm, torch, torchvision, and torchaudio plan from published package metadata, then install it in one uv transaction" ); + let no_wheel_warning = repo_version_without_wheels( + resolution.newest_repo_version.as_deref(), + &resolution.latest_version, + ) + .map(|newest| { + no_wheel_warning_message( + &newest, + &runtime_version_display(&resolution.latest_version), + ) + }); + if let Some(warning) = no_wheel_warning.as_deref() { + let _ = writeln!(output, " warning: {warning}"); + } if dry_run { let env_python = venv_python_path(&install_root); let mut install_args = uv_pip_install_base(&env_python); @@ -1733,13 +1834,36 @@ fn install_wheel_runtime( " activation: use the managed venv Python; TheRock libraries are resolved from that venv by rocm_sdk.initialize_process" ); let _ = writeln!(output, " manifest: {}", manifest_path.display()); - return Ok(output); + return Ok(SdkInstallResult::plan(output)); + } + + // Requirement: surface the "newest version has no wheels" case as a warning + // on the real install path too, not only in the dry-run plan. + if let Some(warning) = no_wheel_warning.as_deref() { + progress_line(format!("Warning: {warning}")); + } + + // Explain, in the visible install log, why an older TheRock ROCm is chosen + // when this host reports a newer legacy ROCm (the ticket's 7.14 case). The + // same note is recorded in the summary block above; surfacing it here keeps + // it from being buried at the end of a long key/value dump. + if let Some(host_version) = host_version_newer.as_deref() { + progress_line(format!( + "Note: this host reports ROCm {host_version}, but ROCm {resolved} is the newest TheRock ROCm with a matching PyTorch stack for this repository; installing {resolved} (pass `--version ` to override).", + resolved = runtime_version_display(&resolution.latest_version) + )); } // Past the preview, the plan has to be installable. A runtime composed // without its exact device payload loads and then faults on the first // kernel, so an undetermined target is refused here rather than papered // over with every published payload. + // + // Checked *before* the consent gate: an install that cannot work should say + // so, not first demand a consent flag for it. With the order reversed, a + // non-interactive host with an unresolvable target reports only "re-run with + // --approve-replacing-active-default", and supplying it just surfaces this + // error instead. if let Some(reason) = device_target.reason() { bail!( "cannot compose a canonical TheRock {} runtime: {reason}.\n\ @@ -1750,6 +1874,54 @@ fn install_wheel_runtime( ); } + // Installs with no active default runtime proceed with just an informational + // line. Only an install that would displace the current active default asks + // for confirmation, and it asks regardless of family or channel because + // activation is global. With no terminal to answer the prompt it refuses + // instead, naming `--approve-replacing-active-default` as the + // non-interactive approval — see `refuse_non_interactive_message` for why + // that flag and not `--yes`. + let existing = active_default_runtime_relation( + paths, + channel, + &resolution.family, + &resolution.latest_version, + )?; + match sdk_install_approval(existing.is_some(), consent, interactive_terminal()) { + SdkInstallApproval::ProceedFresh => { + progress_line(fresh_install_line( + &runtime_version_display(&resolution.latest_version), + &resolution.family, + )); + } + SdkInstallApproval::ProceedApproved(source) => { + progress_line(preapproved_install_line( + source, + existing.as_deref().unwrap_or_default(), + &runtime_version_display(&resolution.latest_version), + )); + } + SdkInstallApproval::PromptOverwrite => { + if !confirm_overwrite_existing_sdk( + channel, + &resolution.family, + &resolution.latest_version, + existing.as_deref().unwrap_or_default(), + )? { + let _ = writeln!( + output, + " status: cancelled by user; the existing ROCm SDK was left unchanged" + ); + return Ok(SdkInstallResult::plan(output)); + } + } + SdkInstallApproval::RefuseNonInteractive => { + bail!(refuse_non_interactive_message( + existing.as_deref().unwrap_or_default() + )); + } + } + let uv = ensure_uv_binary(paths)?; fs::create_dir_all( install_root @@ -1845,7 +2017,7 @@ fn install_wheel_runtime( let _ = writeln!(output, " rocm_sdk_target_family: {target_family}"); } let _ = writeln!(output, " manifest: {}", manifest_path.display()); - Ok(output) + Ok(SdkInstallResult::installed(output)) } fn therock_pip_package_specs( @@ -1916,6 +2088,487 @@ fn quote_display_arg(value: &str) -> String { } } +/// Describe the managed runtime this install would displace as the active +/// default: the one the runtime config's `active_runtime_key` (or an +/// unambiguous `default_runtime_id`) currently resolves to. Returns `None` only +/// when neither config pointer points at an active default — a genuinely fresh +/// install, where the new runtime takes a slot nothing occupies and there is +/// nothing to consent to. +/// +/// Deliberately NOT scoped to the target family and channel. Activation is +/// global: `finalize_successful_sdk_install` activates whatever was just +/// installed regardless of family or channel, so `rocm install sdk --family +/// gfx120X-all` displaces an active `gfx110X-all` runtime just as surely as a +/// same-family upgrade does. A family/channel-scoped gate would wave exactly +/// that case through unconfirmed while every message promised otherwise. +/// +/// Errors reading the manifest directory or the config are propagated rather +/// than treated as "no active default": silently falling back to a +/// fresh-install verdict on a read error would skip the confirmation gate +/// precisely when we are least sure what is currently active. +/// +/// The same reasoning covers the ways an active default can fail to resolve +/// without any I/O error at all, and all of them used to reach the +/// fresh-install verdict. `current_runtime_manifest` resolves through two +/// pointers — `active_runtime_key` first, then `default_runtime_id` — and each +/// has its own failure shapes: +/// +/// * a registry manifest that reads fine but does not deserialize — +/// `load_runtime_manifests` drops those silently, so a manifest written by an +/// older binary (`family_source`, `selected_artifact_url` and +/// `installed_at_unix_ms` carry no `#[serde(default)]`) vanishes from the +/// list and `current_runtime_manifest` misses; +/// * `active_runtime_key` naming a runtime whose manifest is not in the +/// registry at all; +/// * `default_runtime_id` matching no installed manifest — `runtime_id` is not +/// version-scoped, and `rocm config set-default-runtime` stores whatever it +/// is handed without validating it against the registry; +/// * `default_runtime_id` matching more than one installed manifest, which is +/// the ordinary state once two versions of the same family are installed, +/// because they share the one `therock-:` id. +/// `current_runtime_manifest` resolves only an exactly-one match, so both the +/// zero-match and the multi-match shapes arrive here. +/// +/// Every one of these is something `rocm runtimes list` already calls out, as +/// `active_status: missing manifest for active_runtime_key=...`, +/// `active_status: missing manifest for active_runtime_id=...` or +/// `active_status: ambiguous runtime_id=...`, so proceeding as a fresh install +/// would have one CLI assert both that a runtime is active and that none is. +/// These fail closed into the consent gate rather than into a hard error, so +/// `--approve-replacing-active-default` (or `--yes`) still gets an operator +/// through a stale or ambiguous config. +fn active_default_runtime_relation( + paths: &AppPaths, + channel: TheRockChannel, + family: &str, + resolved_version: &str, +) -> Result> { + let (manifests, unparsed) = load_runtime_manifests_reporting_unparsed(paths)?; + let config = RocmCliConfig::load(paths)?; + let Some(active) = crate::current_runtime_manifest(&config, &manifests) else { + return Ok(unresolved_active_default_relation_text( + config.active_runtime_key.as_deref(), + config.default_runtime_id.as_deref(), + crate::default_runtime_id_matches(&config, &manifests).len(), + &unparsed, + )); + }; + Ok(Some(active_default_relation_text( + active, + channel, + family, + resolved_version, + ))) +} + +/// Wording for the fail-closed half of [`active_default_runtime_relation`]: +/// nothing resolved, but the on-disk state says something should have. +/// +/// `None` here is the only genuinely fresh verdict, and it needs *neither* +/// config pointer to claim an active default: with no `active_runtime_key` and +/// no `default_runtime_id`, neither pointer asserts that a runtime is active, +/// so an unparsable manifest is a registry wart rather than a displacement +/// risk and demanding a consent flag would be a false positive on a genuinely +/// fresh install. Once a pointer does claim something, anything unresolved +/// names what could not be resolved, because the relation string is what the +/// prompt, the preapproved progress line and the non-interactive refusal all +/// print, and "unknown" is the honest answer the operator needs to see. +/// +/// `default_runtime_id_match_count` is the number of installed manifests whose +/// `runtime_id` equals `default_runtime_id`. The caller only reaches this +/// function when resolution failed, so that count is 0 (dangling) or greater +/// than 1 (ambiguous) — an exactly-one match is what +/// `current_runtime_manifest` resolves successfully. +/// +/// `unparsed` is reported two different ways on purpose. An entry is only the +/// *cause* of an unresolved `active_runtime_key` when it is that key's own +/// manifest; every other unreadable entry is a separate registry wart that +/// happens to be visible at the same time, so it is appended as a suffix rather +/// than named as the reason. Blaming an unrelated file — the ordinary +/// older-binary manifest is exactly that — would point the operator at the +/// wrong path while a deleted runtime went unmentioned. +/// +/// Both config pointers are named when both are set. `current_runtime_manifest` +/// tries `active_runtime_key` first and falls through to `default_runtime_id`, +/// so arriving here means *both* failed, and the message is what the prompt, +/// the preapproved progress line and the non-interactive refusal print: it has +/// to name everything that could not be resolved, not just the first pointer. +fn unresolved_active_default_relation_text( + active_runtime_key: Option<&str>, + default_runtime_id: Option<&str>, + default_runtime_id_match_count: usize, + unparsed: &[PathBuf], +) -> Option { + let unparsed_text = || { + unparsed + .iter() + .map(|path| path.display().to_string()) + .collect::>() + .join(", ") + }; + let unparsed_suffix = || { + if unparsed.is_empty() { + String::new() + } else { + format!("; unreadable runtime manifests: {}", unparsed_text()) + } + }; + let non_empty = |value: Option<&str>| { + value + .map(str::trim) + .filter(|value| !value.is_empty()) + .map(str::to_owned) + }; + // The registry stores each manifest at `.json` + // (`runtime_manifest_path`), so the file stem is the key. Compared + // case-insensitively because that is how `current_runtime_manifest` matches + // `active_runtime_key` against `runtime_key`. + let key_manifest_is_unparsable = |key: &str| { + unparsed.iter().any(|path| { + path.file_stem() + .and_then(|stem| stem.to_str()) + .is_some_and(|stem| stem.eq_ignore_ascii_case(key)) + }) + }; + // Reached only when `active_runtime_key` is also set and also unresolved, so + // this is always a continuation of the key clause, never a sentence by + // itself. The count is 0 or >1 for the reason given on the parameter. + // + // Pinned rather than left in prose because the else branch below states + // "no installed runtime manifest matches it" as fact: at exactly 1 that + // sentence is false, and the operator would be told nothing matched the + // recorded default while a manifest did — sending them to re-register a + // runtime that is already there. + debug_assert_ne!( + default_runtime_id_match_count, 1, + "an exactly-one match is what `current_runtime_manifest` resolves, so this function is unreachable with it" + ); + let also_unresolved_id = |id: &str| { + if default_runtime_id_match_count > 1 { + format!( + "; the recorded default runtime_id `{id}` does not settle it either, because {default_runtime_id_match_count} installed runtime manifests match it" + ) + } else { + format!( + "; the recorded default runtime_id `{id}` does not settle it either, because no installed runtime manifest matches it" + ) + } + }; + + match (non_empty(active_runtime_key), non_empty(default_runtime_id)) { + (Some(key), id) => { + let cause = if key_manifest_is_unparsable(&key) { + format!("recorded as `{key}`, but its manifest could not be read") + } else { + format!("recorded as `{key}`, but no installed runtime manifest matches it") + }; + Some(format!( + "{cause}, so what is currently active cannot be determined{}{}", + id.as_deref().map(also_unresolved_id).unwrap_or_default(), + unparsed_suffix() + )) + } + (None, Some(id)) if default_runtime_id_match_count > 1 => Some(format!( + "recorded as runtime_id `{id}`, which {default_runtime_id_match_count} installed runtime manifests match, so which one is currently active cannot be determined{}", + unparsed_suffix() + )), + (None, Some(id)) => Some(format!( + "recorded as runtime_id `{id}`, but no installed runtime manifest matches it, so what is currently active cannot be determined{}", + unparsed_suffix() + )), + (None, None) => None, + } +} + +/// Pure wording for [`active_default_runtime_relation`]. +/// +/// Two shapes, because the two cases are not the same event. When the active +/// default is the same family and channel the install targets, the version +/// comparison is meaningful and the user wants to read "upgrade"/"downgrade"/ +/// "reinstall". When it is a different family or channel, comparing versions +/// would invent a relation between two unrelated runtimes, so the text names +/// what is actually being displaced instead. +fn active_default_relation_text( + active: &InstalledRuntimeManifest, + channel: TheRockChannel, + family: &str, + resolved_version: &str, +) -> String { + if active.family == family && active.channel == channel.as_str() { + let relation = match compare_version_strings(resolved_version, &active.version) { + Ordering::Greater => "upgrade", + Ordering::Less => "downgrade", + Ordering::Equal => "reinstall", + }; + format!( + "{relation} from installed {installed} ({key})", + installed = runtime_version_display(&active.version), + key = active.runtime_key + ) + } else { + format!( + "replaces active default {installed} for family {active_family} on the {active_channel} channel ({key})", + installed = runtime_version_display(&active.version), + active_family = active.family, + active_channel = active.channel, + key = active.runtime_key + ) + } +} + +/// The host's legacy/system ROCm version when it is strictly newer than the +/// version rocm-cli is about to install, otherwise `None`. Used to explain why a +/// seemingly older wheel version is selected over the host's ROCm. +fn host_rocm_version_newer_than(resolved_version: &str) -> Option { + host_version_newer_than(detect_legacy_rocm_summary().version, resolved_version) +} + +/// Pure core of [`host_rocm_version_newer_than`]: given the host's detected ROCm +/// version (if any) and the version about to be installed, return the host +/// version only when it is strictly newer. Split out from the filesystem probe so +/// the newer-than decision is unit-testable without a real legacy ROCm on disk. +fn host_version_newer_than(host_version: Option, resolved_version: &str) -> Option { + let host_version = host_version?; + // Only claim the host is newer when BOTH versions parse and the host is + // strictly greater. `parse_host_version` normalises the shapes hosts + // actually report — a build suffix (`7.2.4-98` -> 7.2.4) and a + // two-component report (`7.4` -> 7.4.0) — so those are compared, not + // discarded. Only a string that still fails to parse is "can't tell", and + // "can't tell" is never "newer". Falling back to a lexicographic compare + // here wrongly ranks e.g. `7.2.4-98` above `7.13.0` (because '2' > '1' at + // the third char), inventing a host-newer note that misleads the user. + let host_parsed = parse_host_version(&host_version)?; + let resolved_parsed = parse_host_version(resolved_version)?; + (host_parsed > resolved_parsed).then_some(host_version) +} + +/// The wheel-path `version_note` body explaining why a host-newer legacy ROCm is +/// passed over for the newest TheRock version that still has a matching PyTorch +/// stack. Pure so the exact user-facing wording is unit-testable. +fn wheel_host_version_note(host_version: &str, resolved_display: &str) -> String { + format!( + "this host reports ROCm {host_version}, but {resolved_display} is the newest TheRock ROCm with a matching PyTorch stack, so it is selected; pass `--version ` to override" + ) +} + +/// The tarball-path `version_note` body explaining why a host-newer legacy ROCm is +/// passed over for the newest TheRock tarball for this GPU family. +fn tarball_host_version_note(host_version: &str, resolved_display: &str) -> String { + format!( + "this host reports ROCm {host_version}, but {resolved_display} is the newest TheRock ROCm tarball for this GPU family, so it is selected" + ) +} + +/// The `warning` body surfaced when the repository's newest version has no +/// installable PyTorch wheels for this Python/platform, so an older one is used. +fn no_wheel_warning_message(newest: &str, resolved_display: &str) -> String { + format!( + "ROCm {newest} is the newest version in this repository but has no installable PyTorch wheels for this Python and platform; installing ROCm {resolved_display} instead" + ) +} + +/// The repo's newest version when it is strictly newer than the version we are +/// about to install — i.e. the newest release exists in the index but has no +/// installable PyTorch wheels for this Python/platform, so an older one is used. +/// Returns `None` when the newest version is the one being installed (or is +/// unknown), so the caller only warns when there is a real gap. +fn repo_version_without_wheels( + newest_repo: Option<&str>, + resolved_version: &str, +) -> Option { + let newest = newest_repo?; + match compare_version_strings(newest, resolved_version) { + Ordering::Greater => Some(newest.to_owned()), + _ => None, + } +} + +/// Where an already-granted approval for displacing the active default came +/// from. Carried rather than collapsed to a bare bool so the line the CLI +/// prints can name the real source: `rocm update --apply` takes its approval +/// from the runtime the user selected, not from a flag, so a message crediting +/// `--yes` would name an approval that was never given. (`rocm update` does +/// accept a `--yes` flag, but it is inert by its own documentation — applying +/// never prompts — so it grants nothing to credit.) +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum SdkInstallApprovalSource { + /// The user passed `--yes` to `rocm install sdk`, which also approves + /// installing required system packages with `sudo`. + AssumeYes, + /// `--approve-replacing-active-default` was passed: the narrow consent, and + /// the one ROCm CLI's own terminal-less surfaces (chat, MCP, `rocmd`, the + /// dashboard, onboarding) inject. Kept distinct from `AssumeYes` so the + /// install log names the flag that was actually given — those surfaces never + /// pass `--yes`, and a line crediting it would tell a reader that consent to + /// run `sudo` had been granted when it was not. + ApproveReplacingActiveDefault, + /// `rocm update --apply`, which owns its own consent: the user named the + /// runtime to update, so replacing it is the operation asked for. Non- + /// interactive by construction, so it must never reach a prompt. + /// + /// `activates` records whether `--activate` was given. Only then does the + /// new install become the active default; without it `apply_runtime_update` + /// leaves the current default alone and prints a `runtimes activate` hint. + UpdateApply { activates: bool }, +} + +/// How consent for displacing the active default runtime is obtained. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum SdkInstallConsent { + /// Ask the user: prompt when a terminal is attached, refuse when not. + Ask, + /// Already granted before the install started, by the named source. + Preapproved(SdkInstallApprovalSource), +} + +/// Whether a real SDK install needs the user's approval before it runs. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum SdkInstallApproval { + /// No active default runtime — nothing is displaced, so install without asking. + ProceedFresh, + /// An active default runtime exists and consent was already granted — + /// displace it without asking, crediting the source. + ProceedApproved(SdkInstallApprovalSource), + /// An active default runtime exists and there is a terminal — prompt before + /// displacing it. + PromptOverwrite, + /// An active default runtime exists but there is no terminal and no + /// preapproval — refuse. + RefuseNonInteractive, +} + +/// Decide whether an SDK install proceeds, prompts, or is refused. Installs with +/// no active default runtime to displace (`existing == false`) always proceed; +/// displacing the active default needs consent, and outside an interactive +/// terminal that consent must have been granted up front. +const fn sdk_install_approval( + existing: bool, + consent: SdkInstallConsent, + interactive: bool, +) -> SdkInstallApproval { + match (existing, consent) { + (false, _) => SdkInstallApproval::ProceedFresh, + (true, SdkInstallConsent::Preapproved(source)) => { + SdkInstallApproval::ProceedApproved(source) + } + (true, SdkInstallConsent::Ask) if interactive => SdkInstallApproval::PromptOverwrite, + (true, SdkInstallConsent::Ask) => SdkInstallApproval::RefuseNonInteractive, + } +} + +/// The progress line printed when an install displaces the active default +/// without asking. Pure so the exact wording is pinned by unit tests. +/// +/// Each arm states only what that caller will actually do. `update --apply` +/// without `--activate` installs beside the active default and leaves it alone, +/// so claiming the new install "becomes the active default runtime" there would +/// be false — and crediting `--yes` would claim an approval the update path +/// never received, since `rocm update`'s `--yes` is inert and its approval +/// comes from the runtime selection instead. +/// +/// `pub(crate)` so the `Update` subcommand's own test can pin that invariant at +/// the site of the flag it must not credit. +pub(crate) fn preapproved_install_line( + source: SdkInstallApprovalSource, + relation: &str, + resolved_display: &str, +) -> String { + match source { + SdkInstallApprovalSource::AssumeYes => format!( + "Approved by --yes: an existing ROCm SDK is the active default runtime ({relation}); installing ROCm {resolved_display}, which becomes the active default runtime." + ), + SdkInstallApprovalSource::ApproveReplacingActiveDefault => format!( + "Approved by --approve-replacing-active-default: an existing ROCm SDK is the active default runtime ({relation}); installing ROCm {resolved_display}, which becomes the active default runtime." + ), + SdkInstallApprovalSource::UpdateApply { activates: true } => format!( + "Requested by `rocm update --apply --activate`: an existing ROCm SDK is the active default runtime ({relation}); installing ROCm {resolved_display}, which becomes the active default runtime." + ), + SdkInstallApprovalSource::UpdateApply { activates: false } => format!( + "Requested by `rocm update --apply`: an existing ROCm SDK is the active default runtime ({relation}); installing ROCm {resolved_display} alongside it. The active default runtime is unchanged; re-run with --activate, or use `rocm runtimes activate`, to switch to it." + ), + } +} + +/// The progress line printed when nothing is displaced. States only that no +/// active default exists, because whether this install *becomes* the active +/// default depends on the caller: `rocm install sdk` activates what it +/// installed, `rocm update --apply` without `--activate` does not. +fn fresh_install_line(resolved_display: &str, family: &str) -> String { + format!( + "No active ROCm SDK runtime is configured; installing ROCm SDK {resolved_display} for family {family}." + ) +} + +/// The error raised when the active default would be displaced but there is no +/// terminal to confirm it and no preapproval. +/// +/// This is the only message a script or CI job sees when it hits this gate, so +/// it names the narrow flag first: that is the whole consent the caller needs +/// here, and recommending `--yes` instead would hand an unattended caller the +/// second consent it carries — approval to install system packages with `sudo`, +/// whose password prompt a job with no terminal cannot answer. `--yes` is still +/// named, because a user at a terminal who wants both should not have to +/// discover it elsewhere. +fn refuse_non_interactive_message(relation: &str) -> String { + format!( + "an existing ROCm SDK is the active default runtime ({relation}); continuing would make the newly installed ROCm the active default runtime instead. Re-run with --approve-replacing-active-default to approve this non-interactively, for example `rocm install sdk --approve-replacing-active-default`. Use --yes instead only if you also want to approve installing required system packages with sudo, which needs a terminal to answer a password prompt" + ) +} + +/// Interactive confirmation gate for displacing the active default managed +/// runtime. Prints what would be replaced, then reads a yes/no answer from +/// stdin. Only reached when an active default runtime exists, consent was not +/// preapproved, and a terminal is attached (see `sdk_install_approval`). +/// +/// "Displacing" rather than "overwriting" is deliberate: in the default managed +/// install root `runtime_key` embeds the resolved version, so an upgrade or +/// downgrade lands in its own install root with its own manifest and the previous +/// install stays on disk — what changes is which runtime is the active default. +/// Only a same-version reinstall reuses the same root. The prompt says so instead +/// of claiming a deletion that does not happen. +/// +/// `--prefix` is the exception and the prompt does not claim otherwise: +/// `resolved_install_root` uses the given folder verbatim for every version, so +/// a second install into one prefix does replace the first in place (and +/// `ensure_uv_venv` will `remove_dir_all` it outright if the existing venv's +/// python no longer answers `--version`). The gate is unchanged either way — +/// what is being consented to is the change of active default, not a deletion. +fn confirm_overwrite_existing_sdk( + channel: TheRockChannel, + family: &str, + resolved_version: &str, + relation: &str, +) -> Result { + println!("sdk install: an existing ROCm SDK is the active default runtime"); + println!(" active default runtime: {relation}"); + println!( + " replacing with: ROCm {} for family {family} ({} channel)", + runtime_version_display(resolved_version), + channel.as_str() + ); + println!( + " effect: this install becomes the active default runtime, overriding the current default" + ); + // The host-newer ROCm explanation is already emitted as a visible progress + // line before this prompt on the real install path, so it is not repeated + // here. + prompt_yes_no("Replace the existing ROCm SDK as the active default? [y/N]: ") +} + +fn prompt_yes_no(prompt: &str) -> Result { + print!("{prompt}"); + std::io::stdout() + .flush() + .context("failed to flush confirmation prompt")?; + let mut response = String::new(); + std::io::stdin() + .read_line(&mut response) + .context("failed to read confirmation response")?; + let normalized = response.trim().to_ascii_lowercase(); + Ok(matches!(normalized.as_str(), "y" | "yes")) +} + +#[allow(clippy::too_many_arguments)] fn install_tarball_runtime( paths: &AppPaths, channel: TheRockChannel, @@ -1924,7 +2577,8 @@ fn install_tarball_runtime( version_selector: Option<&RuntimeVersionSelector>, layout_override: Option, dry_run: bool, -) -> Result { + consent: SdkInstallConsent, +) -> Result { let artifact = resolve_tarball_artifact( paths, channel, @@ -1961,13 +2615,72 @@ fn install_tarball_runtime( " latest_version: {}", runtime_version_display(&artifact.version) ); + // Probed once and reused by the progress line below; see the wheel path. + let host_version_newer = host_rocm_version_newer_than(&artifact.version); + if let Some(host_version) = host_version_newer.as_deref() { + let _ = writeln!( + output, + " version_note: {}", + tarball_host_version_note(host_version, &runtime_version_display(&artifact.version)) + ); + } let _ = writeln!(output, " target: {}", install_root.display()); let _ = writeln!(output, " cache_path: {}", cache_path.display()); let _ = writeln!(output, " runtime_key: {runtime_key}"); if dry_run { let _ = writeln!(output, " mode: dry-run"); let _ = writeln!(output, " manifest: {}", manifest_path.display()); - return Ok(output); + return Ok(SdkInstallResult::plan(output)); + } + + // Mirror the wheel path: surface the host-newer ROCm explanation as a visible + // line so "why this version and not the host's newer ROCm" is in the install + // log rather than only in the trailing summary block. + if let Some(host_version) = host_version_newer.as_deref() { + progress_line(format!( + "Note: this host reports ROCm {host_version}, but ROCm {resolved} is the newest TheRock ROCm tarball for this GPU family; installing {resolved}.", + resolved = runtime_version_display(&artifact.version) + )); + } + + // Same gate as the wheel path: only an install that would displace the + // current active default runtime asks for confirmation, and it asks + // regardless of family or channel because activation is global. + let existing = + active_default_runtime_relation(paths, channel, &artifact.family, &artifact.version)?; + match sdk_install_approval(existing.is_some(), consent, interactive_terminal()) { + SdkInstallApproval::ProceedFresh => { + progress_line(fresh_install_line( + &runtime_version_display(&artifact.version), + &artifact.family, + )); + } + SdkInstallApproval::ProceedApproved(source) => { + progress_line(preapproved_install_line( + source, + existing.as_deref().unwrap_or_default(), + &runtime_version_display(&artifact.version), + )); + } + SdkInstallApproval::PromptOverwrite => { + if !confirm_overwrite_existing_sdk( + channel, + &artifact.family, + &artifact.version, + existing.as_deref().unwrap_or_default(), + )? { + let _ = writeln!( + output, + " status: cancelled by user; the existing ROCm SDK was left unchanged" + ); + return Ok(SdkInstallResult::plan(output)); + } + } + SdkInstallApproval::RefuseNonInteractive => { + bail!(refuse_non_interactive_message( + existing.as_deref().unwrap_or_default() + )); + } } fs::create_dir_all(paths.cache_dir.join("therock"))?; @@ -2026,7 +2739,7 @@ fn install_tarball_runtime( let _ = writeln!(output, " extracted: {}", install_root.display()); let _ = writeln!(output, " manifest: {}", manifest_path.display()); - Ok(output) + Ok(SdkInstallResult::installed(output)) } fn resolve_pip_runtime( @@ -2216,12 +2929,23 @@ fn resolve_pip_runtime_from_index( })? }; let latest_version = package_versions.rocm.clone(); + // The repo's newest version for this channel, ignoring wheel availability. + // Only meaningful when we auto-selected "latest" (no explicit request), so a + // caller can warn when that newest version has no installable wheels. + let newest_repo_version = if version_selector.is_none() { + channel_rocm_candidates(&rocm_versions, channel) + .into_iter() + .last() + } else { + None + }; Ok(PipRuntimeResolution { family: family_resolution.family.clone(), family_source: family_resolution.source.clone(), index_url: index_url.to_owned(), layout: source.layout, latest_version, + newest_repo_version, package_versions, device_target: source.device_target.clone(), published_device_targets: source.published_device_targets.clone(), @@ -5152,6 +5876,54 @@ fn parse_version(value: &str) -> Option { }) } +/// Lenient parse of a host-reported ROCm version for the "is the host newer?" +/// decision. Unlike [`parse_version`], this tolerates the shapes a legacy/system +/// ROCm actually reports: a build suffix (`7.2.4-98`) and a missing patch +/// component (`7.4`). Major and minor are required; patch defaults to 0 when +/// absent and may still carry an `rc`/`a` stage suffix. Returns `None` when +/// major/minor cannot be read so an unparseable host string is treated as +/// "can't tell" instead of being compared lexicographically. +fn parse_host_version(value: &str) -> Option { + // Drop build/local metadata: `7.2.4-98`, `7.2.4+local` -> `7.2.4`. + let value = value.split(['+', '-']).next().unwrap_or(value).trim(); + let mut parts = value.splitn(3, '.'); + let major = parts.next()?.parse().ok()?; + let minor = parts.next()?.parse().ok()?; + let (patch, stage, stage_number) = match parts.next() { + // Two-component report (`7.4`) -> treat as `7.4.0`. + None => (0, VersionStage::Stable, 0), + Some(patch_and_rest) => { + let patch_len = patch_and_rest + .chars() + .take_while(char::is_ascii_digit) + .count(); + if patch_len == 0 { + return None; + } + let patch = patch_and_rest[..patch_len].parse().ok()?; + let suffix = &patch_and_rest[patch_len..]; + let (stage, stage_number) = if suffix.is_empty() { + (VersionStage::Stable, 0) + } else if let Some(rest) = suffix.strip_prefix("rc") { + (VersionStage::Rc, rest.parse().ok()?) + } else if let Some(rest) = suffix.strip_prefix('a') { + (VersionStage::Alpha, rest.parse().ok()?) + } else { + return None; + }; + (patch, stage, stage_number) + } + }; + + Some(ParsedVersion { + major, + minor, + patch, + stage, + stage_number, + }) +} + /// Recovery guidance appended to family/index resolution failures so a clean /// first run can recover without the user having to guess a `--family`. /// @@ -5361,12 +6133,29 @@ fn save_runtime_manifest(paths: &AppPaths, manifest: &InstalledRuntimeManifest) } pub(crate) fn load_runtime_manifests(paths: &AppPaths) -> Result> { + Ok(load_runtime_manifests_reporting_unparsed(paths)?.0) +} + +/// [`load_runtime_manifests`] plus the registry entries that read fine but did +/// not deserialize. +/// +/// A manifest written by an older binary is the ordinary way to land here: +/// `family_source`, `selected_artifact_url` and `installed_at_unix_ms` carry no +/// `#[serde(default)]`, so an older file fails `from_slice` against a newer +/// binary. Dropping those silently is right for the listing and lookup callers +/// — one stale file must not brick `rocm runtimes list` — but it is wrong for +/// the install consent gate, which has to know that its view of "what is +/// active" is incomplete. Hence two entry points rather than one hard error. +fn load_runtime_manifests_reporting_unparsed( + paths: &AppPaths, +) -> Result<(Vec, Vec)> { let registry_dir = runtime_registry_dir(paths); if !registry_dir.is_dir() { - return Ok(Vec::new()); + return Ok((Vec::new(), Vec::new())); } let mut manifests = Vec::new(); + let mut unparsed = Vec::new(); for entry in fs::read_dir(®istry_dir) .with_context(|| format!("failed to read {}", registry_dir.display()))? { @@ -5377,12 +6166,14 @@ pub(crate) fn load_runtime_manifests(paths: &AppPaths) -> Result(&bytes) { - manifests.push(manifest.normalize_host_paths()); + match serde_json::from_slice::(&bytes) { + Ok(manifest) => manifests.push(manifest.normalize_host_paths()), + Err(_) => unparsed.push(path), } } manifests.sort_by_key(|manifest| std::cmp::Reverse(manifest.installed_at_unix_ms)); - Ok(manifests) + unparsed.sort(); + Ok((manifests, unparsed)) } fn has_nontrivial_directory_contents(path: &Path) -> Result { @@ -8521,25 +9312,806 @@ echo Python 3.12.10 cache_dir: root.join("cache"), }; - let error = install_sdk(&paths, "release", "tarball", None, None, None, true) - .unwrap_err() - .to_string(); + let error = install_sdk( + &paths, + "release", + "tarball", + None, + None, + None, + true, + SdkInstallConsent::Preapproved(SdkInstallApprovalSource::AssumeYes), + ) + .unwrap_err() + .to_string(); assert!(error.contains("tarball installs are not supported on Windows")); assert!(error.contains("rocm install sdk --format wheel")); } - fn test_paths(name: &str) -> (PathBuf, AppPaths) { - let root = workspace_test_artifact_dir().join(format!( - "rocm-cli-therock-test-{name}-{}-{}", - std::process::id(), - unix_time_millis() - )); - ( - root.clone(), - AppPaths { - config_dir: root.join("config"), - data_dir: root.join("data"), + /// Register `manifest` and make it the active default runtime, the way a + /// completed `install sdk` does. + fn write_active_test_runtime( + paths: &AppPaths, + manifest: &InstalledRuntimeManifest, + ) -> Result<()> { + write_test_runtime_manifest(paths, manifest)?; + let mut config = RocmCliConfig::load(paths)?; + config.default_runtime_id = Some(manifest.runtime_id.clone()); + config.active_runtime_key = Some(manifest.runtime_key.clone()); + config.save(paths)?; + Ok(()) + } + + #[test] + fn active_default_relation_classifies_upgrade_downgrade_reinstall() -> Result<()> { + let (root, paths) = test_paths("active-default-relation"); + let mut manifest = test_runtime_manifest( + "release-wheel-gfx120X-all", + "therock-release:gfx120X-all", + 10, + ); + manifest.version = "7.13.0".to_owned(); + write_active_test_runtime(&paths, &manifest)?; + + let upgrade = active_default_runtime_relation( + &paths, + TheRockChannel::Release, + "gfx120X-all", + "7.14.0", + )? + .expect("relation should be reported while a runtime is the active default"); + assert!(upgrade.starts_with("upgrade from"), "got: {upgrade}"); + assert!(upgrade.contains("7.13.0")); + + let downgrade = active_default_runtime_relation( + &paths, + TheRockChannel::Release, + "gfx120X-all", + "7.12.0", + )? + .expect("relation should be reported while a runtime is the active default"); + assert!(downgrade.starts_with("downgrade from"), "got: {downgrade}"); + + let reinstall = active_default_runtime_relation( + &paths, + TheRockChannel::Release, + "gfx120X-all", + "7.13.0", + )? + .expect("relation should be reported while a runtime is the active default"); + assert!(reinstall.starts_with("reinstall from"), "got: {reinstall}"); + + let _ = fs::remove_dir_all(root); + Ok(()) + } + + #[test] + fn active_default_relation_gates_a_different_family_or_channel() -> Result<()> { + // The regression this gate exists for. `finalize_successful_sdk_install` + // activates whatever was installed last regardless of family or channel, + // so installing gfx120X-all while a gfx110X-all runtime is the active + // default displaces it. A family/channel-scoped gate reported "no + // existing SDK" here and let the displacement through unconfirmed. + let (root, paths) = test_paths("active-default-relation-cross-family"); + let mut manifest = test_runtime_manifest( + "release-wheel-gfx110X-all", + "therock-release:gfx110X-all", + 10, + ); + manifest.version = "7.13.0".to_owned(); + write_active_test_runtime(&paths, &manifest)?; + + let other_family = active_default_runtime_relation( + &paths, + TheRockChannel::Release, + "gfx120X-all", + "7.14.0", + )? + .expect("installing another family must still report the active default it displaces"); + assert!( + other_family.contains("replaces active default") + && other_family.contains("gfx110X-all"), + "got: {other_family}" + ); + + let other_channel = active_default_runtime_relation( + &paths, + TheRockChannel::Nightly, + "gfx110X-all", + "7.14.0", + )? + .expect("installing another channel must still report the active default it displaces"); + assert!( + other_channel.contains("replaces active default") + && other_channel.contains("release channel"), + "got: {other_channel}" + ); + + let _ = fs::remove_dir_all(root); + Ok(()) + } + + #[test] + fn active_default_relation_none_without_an_active_default() -> Result<()> { + // Nothing installed at all, and — the second case — a registered runtime + // that no config points at. Neither displaces anything, so neither may + // prompt: an install with no active default runtime is the fresh path. + let (root, paths) = test_paths("active-default-relation-empty"); + assert!( + active_default_runtime_relation( + &paths, + TheRockChannel::Release, + "gfx120X-all", + "7.14.0" + )? + .is_none() + ); + + let manifest = test_runtime_manifest( + "release-wheel-gfx120X-all", + "therock-release:gfx120X-all", + 10, + ); + write_test_runtime_manifest(&paths, &manifest)?; + assert!( + active_default_runtime_relation( + &paths, + TheRockChannel::Release, + "gfx120X-all", + "7.14.0" + )? + .is_none() + ); + + let _ = fs::remove_dir_all(root); + Ok(()) + } + + #[test] + fn active_default_relation_propagates_manifest_read_errors() -> Result<()> { + // A manifest entry that cannot be read (here, a directory sitting where a + // `*.json` manifest file is expected) must surface as an error, not be + // silently treated as "no active default" — that would skip the + // confirmation gate exactly when we're least sure what is active. + let (root, paths) = test_paths("active-default-relation-error"); + let registry_dir = paths.data_dir.join("runtimes").join("registry"); + fs::create_dir_all(registry_dir.join("broken.json"))?; + + let error = active_default_runtime_relation( + &paths, + TheRockChannel::Release, + "gfx120X-all", + "7.14.0", + ) + .expect_err("a manifest read failure should be propagated, not swallowed"); + assert!(!error.to_string().is_empty()); + + let _ = fs::remove_dir_all(root); + Ok(()) + } + + #[test] + fn active_default_relation_fails_closed_on_an_unparsable_active_manifest() -> Result<()> { + // The other half of the same policy, and the half that used to fail open: + // the manifest file *reads* fine, so nothing errors, but it does not + // deserialize. `load_runtime_manifests` dropped it silently, + // `current_runtime_manifest` then missed, and the gate was skipped with a + // fresh-install verdict while `rocm runtimes list` still reported the + // runtime as active. This is the older-manifest/newer-binary shape: + // `family_source` carries no `#[serde(default)]`. + let (root, paths) = test_paths("active-default-relation-unparsable"); + let manifest = test_runtime_manifest( + "release-wheel-gfx120X-all", + "therock-release:gfx120X-all", + 10, + ); + write_active_test_runtime(&paths, &manifest)?; + + // The helper carries the preconditions that make this the parse path and + // not the I/O path: the file still reads, and it no longer deserializes. + let _ = make_test_runtime_manifest_unparsable(&paths, &manifest.runtime_key)?; + + let relation = active_default_runtime_relation( + &paths, + TheRockChannel::Release, + "gfx120X-all", + "7.14.0", + )? + .expect("an unparsable active manifest must not yield a fresh-install verdict"); + assert!( + relation.contains(&manifest.runtime_key), + "the relation must name the runtime the config still calls active: {relation}" + ); + assert!( + relation.contains("could not be read"), + "the relation must say why the active default is unknown: {relation}" + ); + + // Fail closed means the consent gate engages, not that the install is + // blocked outright: a consent flag still gets an operator through. + assert_eq!( + sdk_install_approval(true, SdkInstallConsent::Ask, false), + SdkInstallApproval::RefuseNonInteractive + ); + + let _ = fs::remove_dir_all(root); + Ok(()) + } + + #[test] + fn active_default_relation_fails_closed_on_a_dangling_active_runtime_key() -> Result<()> { + // Same policy, third shape: nothing is unreadable or unparsable, the + // config simply names an active runtime the registry has no manifest for. + // `rocm runtimes list` reports this as + // `active_status: missing manifest for active_runtime_key=...`, so a + // fresh-install verdict here would have one CLI assert both that a + // runtime is active and that none is. + let (root, paths) = test_paths("active-default-relation-dangling"); + let mut config = RocmCliConfig::load(&paths)?; + config.active_runtime_key = Some("release-wheel-gfx120X-all".to_owned()); + config.save(&paths)?; + + let relation = active_default_runtime_relation( + &paths, + TheRockChannel::Release, + "gfx120X-all", + "7.14.0", + )? + .expect("a dangling active_runtime_key must not yield a fresh-install verdict"); + assert!( + relation.contains("release-wheel-gfx120X-all"), + "the relation must name the unresolved key: {relation}" + ); + + let _ = fs::remove_dir_all(root); + Ok(()) + } + + #[test] + fn active_default_relation_blames_an_unparsable_manifest_only_when_it_is_the_active_one() + -> Result<()> { + // A dangling `active_runtime_key` and an unparsable manifest belonging to + // some *other* runtime are two independent faults that show up together + // routinely: an older binary's manifest fails `from_slice` against this + // one while the runtime the config calls active was removed outright. + // Naming the stranger's file as "its manifest" would send the operator to + // repair a path that has nothing to do with the problem and never mention + // the runtime that actually went missing. + let (root, paths) = test_paths("active-default-relation-unrelated-unparsable"); + + let stranger = test_runtime_manifest( + "release-wheel-gfx110X-all", + "therock-release:gfx110X-all", + 10, + ); + write_test_runtime_manifest(&paths, &stranger)?; + let stranger_path = make_test_runtime_manifest_unparsable(&paths, &stranger.runtime_key)?; + + let mut config = RocmCliConfig::load(&paths)?; + // Nothing on disk is stored under this key, so the only unreadable entry + // in the registry is the stranger's. + config.active_runtime_key = Some("release-wheel-gfx120X-all".to_owned()); + config.save(&paths)?; + + let relation = active_default_runtime_relation( + &paths, + TheRockChannel::Release, + "gfx120X-all", + "7.14.0", + )? + .expect("a dangling active_runtime_key must not yield a fresh-install verdict"); + assert!( + relation.contains( + "recorded as `release-wheel-gfx120X-all`, but no installed runtime manifest matches it" + ), + "the missing runtime is the cause, not the stranger's manifest: {relation}" + ); + assert!( + !relation.contains("its manifest could not be read"), + "an unrelated unparsable manifest must not be blamed as the active one's: {relation}" + ); + assert!( + relation.contains(&format!( + "; unreadable runtime manifests: {}", + stranger_path.display() + )), + "the unrelated unparsable manifest is still reported, as a suffix: {relation}" + ); + + // The other direction, in the same registry: once the active key's *own* + // manifest is unparsable, "could not be read" is the right cause even + // though the stranger's file is unreadable too. + let active = test_runtime_manifest( + "release-wheel-gfx120X-all", + "therock-release:gfx120X-all", + 20, + ); + write_test_runtime_manifest(&paths, &active)?; + let active_path = make_test_runtime_manifest_unparsable(&paths, &active.runtime_key)?; + + let relation = active_default_runtime_relation( + &paths, + TheRockChannel::Release, + "gfx120X-all", + "7.14.0", + )? + .expect("an unparsable active manifest must not yield a fresh-install verdict"); + assert!( + relation.contains( + "recorded as `release-wheel-gfx120X-all`, but its manifest could not be read" + ), + "the active key's own unparsable manifest is the cause here: {relation}" + ); + assert!( + relation.contains(&active_path.display().to_string()) + && relation.contains(&stranger_path.display().to_string()), + "both unreadable entries are still listed: {relation}" + ); + + let _ = fs::remove_dir_all(root); + Ok(()) + } + + #[test] + fn active_default_relation_names_both_unresolved_config_pointers() -> Result<()> { + // `current_runtime_manifest` tries `active_runtime_key` and falls through + // to `default_runtime_id`, so arriving at the fail-closed path with both + // set means both failed. `rocm runtimes activate` writes the pair + // together, so removing that runtime while another version of the family + // remains strands them together too. Reporting only the key would have + // the operator repair half the config and hit the gate again. + let (root, paths) = test_paths("active-default-relation-both-pointers"); + let runtime_id = "therock-release:gfx120X-all"; + let mut older = test_runtime_manifest("release-wheel-gfx120X-all-7130", runtime_id, 10); + older.version = "7.13.0".to_owned(); + let mut newer = test_runtime_manifest("release-wheel-gfx120X-all-7140", runtime_id, 20); + newer.version = "7.14.0".to_owned(); + write_test_runtime_manifest(&paths, &older)?; + write_test_runtime_manifest(&paths, &newer)?; + + let mut config = RocmCliConfig::load(&paths)?; + config.active_runtime_key = Some("release-wheel-gfx120X-all-7120".to_owned()); + config.default_runtime_id = Some(runtime_id.to_owned()); + config.save(&paths)?; + + let relation = active_default_runtime_relation( + &paths, + TheRockChannel::Release, + "gfx120X-all", + "7.15.0", + )? + .expect("two unresolved pointers must not yield a fresh-install verdict"); + assert!( + relation.contains( + "recorded as `release-wheel-gfx120X-all-7120`, but no installed runtime manifest matches it" + ), + "the relation must name the unresolved key: {relation}" + ); + assert!( + relation.contains(&format!( + "; the recorded default runtime_id `{runtime_id}` does not settle it either, because 2 installed runtime manifests match it" + )), + "the relation must also name the ambiguous fallback id: {relation}" + ); + + // The zero-match half of the same pairing: the fallback is dangling + // rather than ambiguous, and must still be named. + let mut config = RocmCliConfig::load(&paths)?; + config.default_runtime_id = Some("therock-release:gfx110X-all".to_owned()); + config.save(&paths)?; + + let relation = active_default_runtime_relation( + &paths, + TheRockChannel::Release, + "gfx120X-all", + "7.15.0", + )? + .expect("two unresolved pointers must not yield a fresh-install verdict"); + assert!( + relation.contains( + "; the recorded default runtime_id `therock-release:gfx110X-all` does not settle it either, because no installed runtime manifest matches it" + ), + "the relation must also name the dangling fallback id: {relation}" + ); + + // Fail closed means the consent gate engages, not that the install is + // blocked outright: a consent flag still gets an operator through. + assert_eq!( + sdk_install_approval(true, SdkInstallConsent::Ask, false), + SdkInstallApproval::RefuseNonInteractive + ); + + let _ = fs::remove_dir_all(root); + Ok(()) + } + + #[test] + fn active_default_relation_fails_closed_on_an_ambiguous_default_runtime_id() -> Result<()> { + // Same policy, but the *other* resolution path. `current_runtime_manifest` + // falls back to `default_runtime_id` when `active_runtime_key` is unset, + // and resolves only an exactly-one match. `runtime_id` is + // `therock-:` with no version in it, so two installed + // versions of one family share it and the fallback returns `None` — while + // `rocm runtimes list` reports `active_status: ambiguous runtime_id=...`. + // `rocm config set-default-runtime` reaches this state directly: it stores + // the id unvalidated and clears `active_runtime_key`. + let (root, paths) = test_paths("active-default-relation-ambiguous-id"); + let runtime_id = "therock-release:gfx120X-all"; + let mut older = test_runtime_manifest("release-wheel-gfx120X-all-7130", runtime_id, 10); + older.version = "7.13.0".to_owned(); + let mut newer = test_runtime_manifest("release-wheel-gfx120X-all-7140", runtime_id, 20); + newer.version = "7.14.0".to_owned(); + write_test_runtime_manifest(&paths, &older)?; + write_test_runtime_manifest(&paths, &newer)?; + + let mut config = RocmCliConfig::load(&paths)?; + config.default_runtime_id = Some(runtime_id.to_owned()); + // Precondition: this is the `default_runtime_id` shape, not the + // `active_runtime_key` shape the sibling tests already cover. + config.active_runtime_key = None; + config.save(&paths)?; + + let relation = active_default_runtime_relation( + &paths, + TheRockChannel::Release, + "gfx120X-all", + "7.15.0", + )? + .expect("an ambiguous default_runtime_id must not yield a fresh-install verdict"); + assert!( + relation.contains(runtime_id), + "the relation must name the id that could not be resolved: {relation}" + ); + assert!( + relation.contains("2 installed runtime manifests match"), + "the relation must say the id is ambiguous and how badly: {relation}" + ); + + // Fail closed means the consent gate engages, not that the install is + // blocked outright: a consent flag still gets an operator through. + assert_eq!( + sdk_install_approval(true, SdkInstallConsent::Ask, false), + SdkInstallApproval::RefuseNonInteractive + ); + + let _ = fs::remove_dir_all(root); + Ok(()) + } + + #[test] + fn active_default_relation_fails_closed_on_a_dangling_default_runtime_id() -> Result<()> { + // The zero-match half of the same fallback path: `_ => None` in + // `current_runtime_manifest` swallows "no match" exactly as it swallows + // "many matches". `rocm config set-default-runtime` does not check the id + // against the registry, so a typo — or uninstalling the last runtime of a + // family — leaves the config asserting an active default that is not + // there, which `rocm runtimes list` reports as + // `active_status: missing manifest for active_runtime_id=...`. + let (root, paths) = test_paths("active-default-relation-dangling-id"); + let other = test_runtime_manifest( + "release-wheel-gfx110X-all", + "therock-release:gfx110X-all", + 10, + ); + write_test_runtime_manifest(&paths, &other)?; + + let mut config = RocmCliConfig::load(&paths)?; + config.default_runtime_id = Some("therock-release:gfx120X-all".to_owned()); + config.active_runtime_key = None; + config.save(&paths)?; + + let relation = active_default_runtime_relation( + &paths, + TheRockChannel::Release, + "gfx120X-all", + "7.14.0", + )? + .expect("a dangling default_runtime_id must not yield a fresh-install verdict"); + assert!( + relation.contains("therock-release:gfx120X-all"), + "the relation must name the id that could not be resolved: {relation}" + ); + assert!( + relation.contains("no installed runtime manifest matches it"), + "the relation must say the id resolved to nothing: {relation}" + ); + + let _ = fs::remove_dir_all(root); + Ok(()) + } + + #[test] + fn active_default_relation_is_fresh_when_no_config_pointer_claims_an_active_default() + -> Result<()> { + // The limit of the fail-closed policy. An unparsable registry manifest is + // only evidence of a *displacement* risk if something claims an active + // default; with neither `active_runtime_key` nor `default_runtime_id` set, + // nothing does, and demanding a consent flag would block a genuinely fresh + // install over an unrelated registry wart. + let (root, paths) = test_paths("active-default-relation-fresh-unparsable"); + let manifest = test_runtime_manifest( + "release-wheel-gfx120X-all", + "therock-release:gfx120X-all", + 10, + ); + write_test_runtime_manifest(&paths, &manifest)?; + // The helper carries the preconditions that make this the parse path and + // not the I/O path: the file still reads, and it no longer deserializes. + let _ = make_test_runtime_manifest_unparsable(&paths, &manifest.runtime_key)?; + + let config = RocmCliConfig::load(&paths)?; + assert!(config.active_runtime_key.is_none()); + assert!(config.default_runtime_id.is_none()); + + assert_eq!( + active_default_runtime_relation( + &paths, + TheRockChannel::Release, + "gfx120X-all", + "7.14.0", + )?, + None, + "no config pointer claims an active default, so this is a fresh install" + ); + + let _ = fs::remove_dir_all(root); + Ok(()) + } + + #[test] + fn sdk_install_approval_only_prompts_when_an_active_default_is_displaced() { + let assume_yes = SdkInstallConsent::Preapproved(SdkInstallApprovalSource::AssumeYes); + let update_apply = SdkInstallConsent::Preapproved(SdkInstallApprovalSource::UpdateApply { + activates: false, + }); + + // No active default runtime -> never prompt, regardless of terminal/consent. + assert_eq!( + sdk_install_approval(false, SdkInstallConsent::Ask, false), + SdkInstallApproval::ProceedFresh + ); + assert_eq!( + sdk_install_approval(false, SdkInstallConsent::Ask, true), + SdkInstallApproval::ProceedFresh + ); + assert_eq!( + sdk_install_approval(false, assume_yes, false), + SdkInstallApproval::ProceedFresh + ); + + // Active default present: preapproved consent proceeds and is credited to + // its real source; a terminal prompts; neither refuses. + assert_eq!( + sdk_install_approval(true, assume_yes, false), + SdkInstallApproval::ProceedApproved(SdkInstallApprovalSource::AssumeYes) + ); + assert_eq!( + sdk_install_approval( + true, + SdkInstallConsent::Preapproved( + SdkInstallApprovalSource::ApproveReplacingActiveDefault + ), + false + ), + SdkInstallApproval::ProceedApproved( + SdkInstallApprovalSource::ApproveReplacingActiveDefault + ) + ); + assert_eq!( + sdk_install_approval(true, update_apply, false), + SdkInstallApproval::ProceedApproved(SdkInstallApprovalSource::UpdateApply { + activates: false + }) + ); + assert_eq!( + sdk_install_approval(true, SdkInstallConsent::Ask, true), + SdkInstallApproval::PromptOverwrite + ); + assert_eq!( + sdk_install_approval(true, SdkInstallConsent::Ask, false), + SdkInstallApproval::RefuseNonInteractive + ); + } + + #[test] + fn preapproved_install_line_credits_the_real_consent_source() { + // `rocm update --apply` draws its approval from the runtime the user + // selected, not from a flag — its `--yes` is inert — so a line + // crediting `--yes` names an approval that was never given. And without + // `--activate`, `apply_runtime_update` leaves the active default alone, + // so claiming the install "becomes the active default runtime" is false. + let by_yes = preapproved_install_line( + SdkInstallApprovalSource::AssumeYes, + "upgrade from installed 7.13.0 (release-wheel-gfx120X-all)", + "7.14.0", + ); + assert!(by_yes.starts_with("Approved by --yes:"), "got: {by_yes}"); + assert!(by_yes.contains("becomes the active default runtime")); + + // The narrow flag is not `--yes`: ROCm CLI's own terminal-less surfaces + // pass only this one, and a line crediting `--yes` would tell whoever + // reads the chat or dashboard transcript that consent to run `sudo` was + // given when it never was. + let by_narrow = preapproved_install_line( + SdkInstallApprovalSource::ApproveReplacingActiveDefault, + "upgrade from installed 7.13.0 (release-wheel-gfx120X-all)", + "7.14.0", + ); + assert!( + by_narrow.starts_with("Approved by --approve-replacing-active-default:"), + "got: {by_narrow}" + ); + assert!( + !by_narrow.contains("--yes"), + "the narrow flag must not be credited to --yes: {by_narrow}" + ); + assert!(by_narrow.contains("becomes the active default runtime")); + + let update_activates = preapproved_install_line( + SdkInstallApprovalSource::UpdateApply { activates: true }, + "upgrade from installed 7.13.0 (release-wheel-gfx120X-all)", + "7.14.0", + ); + assert!( + !update_activates.contains("--yes"), + "the update path must not credit an approval `--yes` did not grant: {update_activates}" + ); + assert!(update_activates.contains("becomes the active default runtime")); + + let update_only = preapproved_install_line( + SdkInstallApprovalSource::UpdateApply { activates: false }, + "upgrade from installed 7.13.0 (release-wheel-gfx120X-all)", + "7.14.0", + ); + assert!( + !update_only.contains("--yes"), + "the update path must not credit an approval `--yes` did not grant: {update_only}" + ); + assert!( + !update_only.contains("becomes the active default runtime"), + "`update --apply` without --activate does not change the active default: {update_only}" + ); + assert!(update_only.contains("The active default runtime is unchanged")); + } + + #[test] + fn refusal_recommends_the_narrow_flag_before_yes() { + // This is the only message a script or CI job reads when it hits the + // gate, and it is the audience the narrow flag exists for. Recommending + // `--yes` first would hand an unattended caller the second consent that + // flag carries — approval to install system packages with `sudo` — and + // park it on a password prompt it has no terminal to answer. + let message = refuse_non_interactive_message( + "upgrade from installed 7.13.0 (release-wheel-gfx120X-all)", + ); + + let narrow = message + .find("--approve-replacing-active-default") + .unwrap_or_else(|| panic!("the refusal must name the narrow flag: {message}")); + // `--yes` still has to appear: a user at a terminal who wants both + // consents should not have to go looking for it. + let yes = message + .find("--yes") + .unwrap_or_else(|| panic!("the refusal must still explain --yes: {message}")); + assert!( + narrow < yes, + "the narrow flag must be recommended before --yes: {message}" + ); + assert!( + message.contains("system packages"), + "the refusal must say what --yes additionally approves: {message}" + ); + assert!( + message.contains("is the active default runtime"), + "the refusal must name what would be replaced: {message}" + ); + assert!( + !message.to_lowercase().contains("overwrit"), + "an upgrade leaves the previous install on disk; it replaces the \ + active default rather than overwriting it: {message}" + ); + } + + #[test] + fn fresh_install_line_claims_no_absent_sdk() { + // The fresh path is reached whenever no runtime is the active default, + // which does not mean no SDK is installed anywhere. Saying "no existing + // ROCm SDK found" there would be false on a host holding a registered + // but unactivated runtime. + let line = fresh_install_line("7.14.0", "gfx120X-all"); + assert!( + !line.to_lowercase().contains("no existing rocm sdk"), + "got: {line}" + ); + assert!(line.contains("No active ROCm SDK runtime is configured")); + assert!(line.contains("gfx120X-all")); + } + + #[test] + fn repo_version_without_wheels_warns_only_when_newest_is_newer() { + // Newest repo version has no wheels (newer than the installable one) -> warn. + assert_eq!( + repo_version_without_wheels(Some("7.14.0"), "7.13.0").as_deref(), + Some("7.14.0") + ); + // Newest repo version is the one being installed -> no warning. + assert!(repo_version_without_wheels(Some("7.13.0"), "7.13.0").is_none()); + // A specific version was requested (no "newest" known) -> no warning. + assert!(repo_version_without_wheels(None, "7.13.0").is_none()); + // Defensive: an older "newest" (should not happen) never warns. + assert!(repo_version_without_wheels(Some("7.12.0"), "7.13.0").is_none()); + } + + #[test] + fn host_version_newer_than_reports_only_a_strictly_newer_host() { + // Host ROCm is newer than the version being installed -> surface it. + assert_eq!( + host_version_newer_than(Some("7.14.0".to_owned()), "7.13.0").as_deref(), + Some("7.14.0") + ); + // Host ROCm matches the installed version -> nothing to explain. + assert!(host_version_newer_than(Some("7.13.0".to_owned()), "7.13.0").is_none()); + // Host ROCm is older than the installed version -> nothing to explain. + assert!(host_version_newer_than(Some("7.12.0".to_owned()), "7.13.0").is_none()); + // No legacy ROCm detected on the host -> nothing to explain. + assert!(host_version_newer_than(None, "7.13.0").is_none()); + + // A build-suffixed host version must not be lexicographically ranked + // above the resolved version: `7.2.4-98` is numerically OLDER than + // `7.13.0`, so no host-newer note. (This is the reported regression: + // char-compare put `7.2…` above `7.13…`.) + assert!(host_version_newer_than(Some("7.2.4-98".to_owned()), "7.13.0").is_none()); + // Two-component host reports are parsed as `.0`; still older here. + assert!(host_version_newer_than(Some("7.4".to_owned()), "7.13.0").is_none()); + assert!(host_version_newer_than(Some("7.9".to_owned()), "7.13.0").is_none()); + // A build suffix on an equal version is not "newer". + assert!(host_version_newer_than(Some("7.13.0-56".to_owned()), "7.13.0").is_none()); + // A genuinely newer build-suffixed host is surfaced, keeping the + // original reported string (suffix included) for the user-facing note. + assert_eq!( + host_version_newer_than(Some("7.20.1-33".to_owned()), "7.13.0").as_deref(), + Some("7.20.1-33") + ); + // A host string we cannot parse is "can't tell", never "newer". + assert!(host_version_newer_than(Some("unknown".to_owned()), "7.13.0").is_none()); + } + + #[test] + fn host_version_notes_and_warning_render_the_expected_text() { + // Wheel path: the note names both versions and offers the --version override. + let wheel = wheel_host_version_note("7.14.0", "7.13.0"); + assert_eq!( + wheel, + "this host reports ROCm 7.14.0, but 7.13.0 is the newest TheRock ROCm with a matching PyTorch stack, so it is selected; pass `--version ` to override" + ); + + // Tarball path: same explanation, phrased for the GPU-family tarball. + let tarball = tarball_host_version_note("7.14.0", "7.13.0"); + assert_eq!( + tarball, + "this host reports ROCm 7.14.0, but 7.13.0 is the newest TheRock ROCm tarball for this GPU family, so it is selected" + ); + + // The no-wheels warning names the repo-newest and the fallback it installs. + let warning = no_wheel_warning_message("7.14.0", "7.13.0"); + assert_eq!( + warning, + "ROCm 7.14.0 is the newest version in this repository but has no installable PyTorch wheels for this Python and platform; installing ROCm 7.13.0 instead" + ); + } + + fn test_paths(name: &str) -> (PathBuf, AppPaths) { + let root = workspace_test_artifact_dir().join(format!( + "rocm-cli-therock-test-{name}-{}-{}", + std::process::id(), + unix_time_millis() + )); + ( + root.clone(), + AppPaths { + config_dir: root.join("config"), + data_dir: root.join("data"), cache_dir: root.join("cache"), }, ) @@ -8611,6 +10183,33 @@ echo Python 3.12.10 Ok(()) } + /// Rewrite an already-written registry manifest so it still *reads* but no + /// longer deserializes, and return its path. Drops `family_source`, which + /// carries no `#[serde(default)]` — the real older-binary/newer-binary shape, + /// not an invented corruption. + fn make_test_runtime_manifest_unparsable( + paths: &AppPaths, + runtime_key: &str, + ) -> Result { + let path = runtime_manifest_path(paths, runtime_key); + let mut value: serde_json::Value = serde_json::from_slice(&fs::read(&path)?)?; + value + .as_object_mut() + .expect("manifest is a JSON object") + .remove("family_source") + .expect("manifest carries family_source"); + fs::write(&path, serde_json::to_vec_pretty(&value)?)?; + assert!( + fs::read(&path).is_ok(), + "the fixture must still read, or this is the I/O path, not the parse path" + ); + assert!( + serde_json::from_slice::(&fs::read(&path)?).is_err(), + "the test fixture must be unparsable, or this asserts nothing" + ); + Ok(path) + } + #[test] fn runtime_version_display_mentions_embedded_build_date() { assert_eq!( diff --git a/apps/rocmd/src/lib.rs b/apps/rocmd/src/lib.rs index f2c900713..dfc287f45 100644 --- a/apps/rocmd/src/lib.rs +++ b/apps/rocmd/src/lib.rs @@ -2594,6 +2594,28 @@ fn build_install_sdk_args( } if dry_run { argv.push("--dry-run".to_owned()); + } else { + // `run_rocm_capture_for_paths` spawns `rocm` with null stdin, so + // `interactive_terminal()` is false in the child and an active default + // managed runtime would make the approval gate refuse with "re-run with + // `--approve-replacing-active-default`" — a flag no MCP caller of this + // tool can supply. + // + // Not `--yes` itself: that flag carries a second, unrelated consent — + // approving required system-package installs, which run `sudo`. This + // spawn has no terminal, so it could never answer a sudo password + // prompt; granting that consent would make the vLLM/OpenMPI step attempt + // an install it cannot complete and abort the engine auto-install that + // previously warned and continued. `--approve-replacing-active-default` + // grants only the runtime-displacement consent the gate asks for. + // + // Consent is not bypassed: `install_sdk` is in + // `mcp_tool_requires_direct_approval`, so a direct `rocmd mcp-call` + // needs `--allow-mutation` after an explicit user approval, and over the + // MCP protocol the tool is annotated `destructiveHint` for the client's + // approval UI. Mirrors the chat/MCP arm in `apps/rocm`. The dry-run + // branch never reaches the gate (it returns earlier), so it stays bare. + argv.push("--approve-replacing-active-default".to_owned()); } Ok(argv) } @@ -5916,6 +5938,48 @@ mod tests { ); } + /// The `install_sdk` MCP tool spawns `rocm` with null stdin, so a real + /// install over an active default managed runtime would hit the approval + /// gate's non-interactive refusal and bail asking for a flag no MCP caller + /// can pass. The real-install argv must therefore carry the consent flag; + /// the dry-run argv must not, because a dry run never reaches the gate and + /// the flag there would claim an approval the caller did not give. + /// + /// It must be `--approve-replacing-active-default` and never `--yes`: + /// `--yes` additionally approves running `sudo` for required system + /// packages, and a null-stdin spawn has no terminal on which that password + /// prompt could be answered. + #[test] + fn install_sdk_real_install_args_approve_only_the_runtime_replacement() -> Result<()> { + let arguments = serde_json::Map::new(); + + let real = build_install_sdk_args(&arguments, false)?; + assert!( + real.contains(&"--approve-replacing-active-default".to_owned()), + "real install argv must approve the replacement for the null-stdin spawn: {real:?}" + ); + assert!( + !real.contains(&"--yes".to_owned()), + "real install argv must not grant the system-package consent it cannot answer: {real:?}" + ); + assert!( + !real.contains(&"--dry-run".to_owned()), + "real install argv must not be a dry run: {real:?}" + ); + + let dry = build_install_sdk_args(&arguments, true)?; + assert!( + !dry.contains(&"--approve-replacing-active-default".to_owned()) + && !dry.contains(&"--yes".to_owned()), + "dry-run argv must not carry a consent flag: {dry:?}" + ); + assert!( + dry.contains(&"--dry-run".to_owned()), + "dry-run argv must carry --dry-run: {dry:?}" + ); + Ok(()) + } + #[test] fn install_sdk_forwards_requested_build_date_and_rejects_conflict() -> Result<()> { let arguments = serde_json::Map::from_iter([( diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index b87db363d..776d65831 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -2957,7 +2957,7 @@ fn rocm_version_sort_key(version: &str) -> Vec { .collect() } -fn detect_legacy_rocm_summary() -> LegacyRocmSummary { +pub fn detect_legacy_rocm_summary() -> LegacyRocmSummary { // One resolver on both platforms, so the human report, the JSON probe and // the fix-6 runner cannot disagree about which installs exist or which one // is active. `discover_rocm_installs` picks the search roots and layout for diff --git a/crates/rocm-dash-tui/src/ui/install_manager.rs b/crates/rocm-dash-tui/src/ui/install_manager.rs index 4c3dca740..2d664de4d 100644 --- a/crates/rocm-dash-tui/src/ui/install_manager.rs +++ b/crates/rocm-dash-tui/src/ui/install_manager.rs @@ -153,6 +153,18 @@ impl InstallManagerState { } if self.dry_run { args.push("--dry-run".to_string()); + } else { + // The dashboard spawns `rocm` with null stdin, so a real install + // that would displace the active default runtime cannot answer the + // confirmation prompt and would refuse. Approve the replacement so + // the dashboard install proceeds; the dry-run preview never mutates, + // so it needs no flag. + // + // Not `--yes`: that also approves running `sudo` for required system + // packages. This child has no stdin to answer a password prompt + // with, and the dashboard owns the terminal in raw mode, so a sudo + // prompt reaching `/dev/tty` would stall behind the TUI. + args.push("--approve-replacing-active-default".to_string()); } Ok(args) } @@ -478,6 +490,13 @@ mod tests { assert!(args.windows(2).any(|p| p == ["--format", "tarball"])); assert!(args.windows(2).any(|p| p == ["--prefix", "/opt/rocm-sdk"])); assert!(!args.contains(&"--dry-run".to_string())); + // A real (non-dry-run) install must carry + // --approve-replacing-active-default so the null-stdin dashboard spawn + // is not refused at the consent prompt. Deliberately not --yes, which + // would also approve a `sudo` system-package install this spawn has no + // terminal to answer a password prompt on. + assert!(args.contains(&"--approve-replacing-active-default".to_string())); + assert!(!args.contains(&"--yes".to_string())); } #[test] diff --git a/crates/rocm-dash-tui/src/ui/onboarding.rs b/crates/rocm-dash-tui/src/ui/onboarding.rs index 2af6c21de..b2a54699c 100644 --- a/crates/rocm-dash-tui/src/ui/onboarding.rs +++ b/crates/rocm-dash-tui/src/ui/onboarding.rs @@ -192,6 +192,12 @@ fn build_install_args(cfg: &InstallConfig) -> Vec { cfg.channel.as_arg().to_string(), "--format".to_string(), "wheel".to_string(), + // Onboarding installs are spawned with null stdin, so a would-be + // consent prompt cannot be answered and the install would refuse. This + // keeps the first-run install non-interactive. Deliberately not `--yes`, + // which would also approve a `sudo` system-package install this spawn + // has no terminal to answer. + "--approve-replacing-active-default".to_string(), ]; let pin = cfg.pin_value.trim(); if let (Some(flag), false) = (cfg.pin_mode.arg(), pin.is_empty()) { @@ -673,7 +679,8 @@ mod tests { "--channel", "release", "--format", - "wheel" + "wheel", + "--approve-replacing-active-default" ], "default Release path must stay byte-identical to the pre-toggle args" ); @@ -920,7 +927,8 @@ mod tests { "--channel", "nightly", "--format", - "wheel" + "wheel", + "--approve-replacing-active-default" ] ); } @@ -950,6 +958,7 @@ mod tests { "nightly", "--format", "wheel", + "--approve-replacing-active-default", "--build-date", "2026-06-05" ] @@ -985,7 +994,8 @@ mod tests { "--channel", "release", "--format", - "wheel" + "wheel", + "--approve-replacing-active-default" ], "an empty pin must not add a flag" ); diff --git a/docs/manual-testing.md b/docs/manual-testing.md index 8f62a8e5f..e998f9e84 100644 --- a/docs/manual-testing.md +++ b/docs/manual-testing.md @@ -123,8 +123,37 @@ rocm examine Replace `` with the exact key printed by `rocm runtimes list`. Omit `--prefix` if you want rocm-cli to choose its standard managed folder. +Section 1 has already made a managed runtime the active default, so the +`install sdk` above will **ask for confirmation before it installs**: the new +install takes over as the active default. That is the expected behaviour, not a +regression. Answer the prompt to continue. The gate is not scoped to the family +or channel, so it asks even when this install targets a family this machine has +never held. To take the same step without a prompt — and this is required in a +non-interactive shell, where the command refuses instead of asking — re-run it +with `--approve-replacing-active-default`: + +```powershell +rocm install sdk --channel release --format wheel --prefix .\.rocm-work\data\envs\default --approve-replacing-active-default +``` + +On Linux and WSL, use `--yes` only if you also want to approve installing +required system packages with `sudo`, which needs a terminal to answer a +password prompt. On native Windows that second consent buys nothing — the +system-package step returns early there — so `--approve-replacing-active-default` +is the whole approval this gate needs either way. + Expected result: +- On a machine with an active default runtime (the state section 1 leaves + behind), the install prompts first and names what would be displaced; + declining leaves the existing runtime untouched. +- In a non-interactive shell with neither approval flag, the install refuses + rather than silently displacing the active default, and the error names + `--approve-replacing-active-default` as the flag to add. +- With `--approve-replacing-active-default`, the install proceeds without + asking and prints a line crediting that flag by name — not `--yes`. +- `rocm install sdk ... --dry-run` never prompts or refuses, whatever the + active default is: the preview stops before the gate. - rocm-cli creates or reuses a rocm-cli managed Python venv. - pip installs pinned `rocm`, `torch`, and `torchvision` requirements with exactly one `device-` extra (`rocm` also requests diff --git a/docs/testing.md b/docs/testing.md index d3f1ebebe..f84436f91 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -153,9 +153,36 @@ rocm install sdk --channel release --format wheel --dry-run The live SDK acceptance test creates an isolated test root under `target/`, creates a local bootstrap Python venv, runs: ```bash -rocm install sdk --channel release --format wheel +rocm install sdk --channel release --format wheel --yes ``` +`--yes` approves replacing whatever managed runtime is currently the active +default without prompting, which keeps the command non-interactive when the test +root is reused across runs (a root with no active default runtime never +prompts). The gate is not scoped to the family or channel being installed, so +`--yes` is needed on a reused root even when the install targets a family that +root has never held. It matches the invocation in +`scripts/therock_sdk_install_test.py`. + +`--yes` is used here because this test also wants the second approval it +carries: installing required system packages with `sudo`. When all you need is +to clear the active-default gate — the usual case for a script or a CI job — +pass the narrower `--approve-replacing-active-default` instead. That is the flag +the refusal message itself recommends, and the only one ROCm CLI's own +terminal-less surfaces pass. Without either flag, the same command on a reused +root prompts when a terminal is attached and fails outright when one is not; the +failure names the flag to add, so read the message before treating it as a +regression. Check both routes by hand after changing the gate: + +```bash +rocm install sdk --channel release --format wheel --approve-replacing-active-default +rocm install sdk --channel release --format wheel < /dev/null # expect the refusal +``` + +The preview path is unaffected: `--dry-run` returns before the gate is +consulted, so `rocm install sdk --channel release --format wheel --dry-run` +never prompts and never refuses, whatever the active default is. + Then it verifies: - runtime manifest metadata diff --git a/scripts/therock_sdk_install_test.py b/scripts/therock_sdk_install_test.py index 50e4b8af0..abe911fa7 100644 --- a/scripts/therock_sdk_install_test.py +++ b/scripts/therock_sdk_install_test.py @@ -481,6 +481,16 @@ def main() -> int: install_argv.extend(["--prefix", str(args.prefix)]) if args.dry_run: install_argv.append("--dry-run") + else: + # Approve replacing whatever runtime is the active default, so a reused + # test root stays non-interactive. Only on the real-install path: a dry + # run returns before the consent gate, and passing the flag there would + # claim an approval this harness was not asked for. `--yes` rather than + # `--approve-replacing-active-default` because this harness is meant to + # install the system packages the SDK needs too, and it is run by hand + # from a developer's terminal (no workflow invokes it), which can answer + # a sudo password prompt. + install_argv.append("--yes") install_output = run( "rocm install sdk pip", install_argv, diff --git a/tests/e2e-cucumber/features/runtime_setup.feature b/tests/e2e-cucumber/features/runtime_setup.feature index cdb3e1591..a9f03ef6b 100644 --- a/tests/e2e-cucumber/features/runtime_setup.feature +++ b/tests/e2e-cucumber/features/runtime_setup.feature @@ -184,3 +184,116 @@ Feature: Runtime configuration Scenario: runtime-10 - Stating rollback's single-level limit in --help When the user asks for rollback help Then the help states that rollback has no history + + # Installing over the active default managed runtime must not silently + # displace it. Outside an interactive terminal (as every e2e invocation + # is here), `install sdk` with neither consent flag must refuse rather than + # proceed, and the refusal has to name the flag the caller should actually + # reach for: `--approve-replacing-active-default`, not `--yes`, which would + # additionally approve a `sudo` system-package install no script can answer. + # GPU-gated because the precondition needs a GPU to have a runtime active. + # The refusal is not free: the gate reports the version relation, so + # it runs after the Python launcher is resolved and the channel index is read. + # Both are already warm here — the `Given` installed a runtime, so the launcher + # resolves to the saved managed Python rather than bootstrapping uv, and the + # index read is cached — but on a cold host the launcher step can still fetch. + # What the refusal does bail before is the SDK and torch download and any + # change on disk. + @id:runtime-install-sdk-overwrite-requires-yes @requires-gpu + Scenario: runtime-11 - Reinstalling the SDK over an existing runtime without consent is refused + Given a managed runtime is active + When the user reinstalls the SDK without confirming + Then the reinstall is refused + And the error explains how to approve the replacement non-interactively + + # Companion to Scenario runtime-11: with --yes the same reinstall proceeds and the + # runtime stays registered and active afterward. Nightly-gated in addition to + # GPU because, unlike Scenario runtime-11, this exercises a real second SDK install. + # The registered/active Thens hold from the Given alone, so the approval Then + # is what actually distinguishes this from a no-op: it fails if --yes ever + # regresses to a refusal or silently takes the fresh-install path. + @id:runtime-install-sdk-overwrite-with-yes @requires-gpu @nightly + Scenario: runtime-12 - Reinstalling the SDK over an existing runtime with --yes proceeds + Given a managed runtime is active + When the user reinstalls the SDK with --yes + Then the install reports that --yes approved replacing the existing runtime + And a runtime is registered + And the runtime is set as active + + # The case a family-and-channel-scoped gate waved through. Activation is + # global — whatever finishes installing last becomes the active default, no + # matter which family it was built for — so installing a family this host has + # never held displaces the active runtime exactly as a same-family reinstall + # does, and has to ask exactly as loudly. Scenario runtime-11 cannot catch + # this: it reinstalls the same family, so it passes under both the old + # family-scoped gate and this one. + # + # No `@nightly` despite the second family: like Scenario runtime-11 this is a + # refusal, so it bails before the multi-GiB download and costs a resolve, not + # an install. The third Then is what separates a correct refusal from an + # unrelated failure (a bad family name would also exit non-zero and could also + # name the consent flags in a usage line): only the real gate names the + # runtime it would replace. + # + # `@requires-os:linux` because the second family has to arrive by the tarball + # format to reach the gate at all, and tarball installs are refused outright on + # Windows. A wheel install picks its device payload from the GPU this host + # reports and refuses a family that target does not belong to *before* the + # consent gate — correctly, since that install could never have worked — so on + # a GPU host the wheel path answers with a target error and the displacement + # never comes up. The tarball path takes the family it is given, consults no + # host target, and reaches the same gate. What is lost on Windows is this + # cross-family case only: Scenario runtime-11 still covers the refusal there. + @id:runtime-install-sdk-other-family-requires-yes @requires-gpu @requires-os:linux + Scenario: runtime-13 - Installing a different GPU family while a runtime is active is refused without consent + Given a managed runtime is active + When the user installs a different GPU family without confirming + Then the reinstall is refused + And the error explains how to approve the replacement non-interactively + And the error names the active default runtime it would replace + + # `--yes` approves two unrelated things: replacing the active default runtime, + # and running `sudo` to install required system packages such as OpenMPI for + # vLLM. ROCm CLI's own non-interactive surfaces (chat, MCP, the dashboard) + # spawn `rocm` with null stdin, so they need the first and can never answer a + # password prompt for the second; they pass the narrow flag instead. A reader + # who believes the two flags are synonyms will reach for `--yes` from a script + # and get a sudo prompt nothing can answer, so `--help` has to state the + # difference (Scenario runtime-10 sets the precedent for pinning help text + # that a unit test on `render_long_help()` cannot prove reaches a real user). + # No runtime state needed, so this runs on the mock lane. + @id:runtime-install-sdk-help-separates-consents + Scenario: runtime-14 - Stating that the non-interactive consent flag does not approve sudo in --help + When the user asks for SDK install help + Then the help offers a consent flag that does not approve system-package installs + + # `rocm --yes ` prints the planned command twice — once under `request + # plan`, once under `execution` — and the two deliberately disagree: the plan + # render is shared with the no-`--yes` review path, which must never hand a + # human a pre-approved command, so the replacement consent is injected only + # after it. What the operator sees, though, is a consent flag appearing on the + # command that runs and nowhere on the command they were shown, which reads as + # something approved behind their back. The `note:` under the execution + # `tool_call:` is the only place that difference is explained, and it is + # command output, so a unit test on the renderer does not discharge it. + # + # The three Thens are one claim only if the note can be trusted on its own. It + # cannot: a note saying "this differs from the plan above" is a lie if the two + # lines actually agree, and a plan line that already carried the flag would + # make the note false without changing its text. So the first two Thens pin the + # difference the third one describes. + # + # The install itself must not run — on the GPU lanes this request resolves to a + # real multi-GiB SDK pull — and these assertions are about output the CLI + # prints *before* it dispatches. The Given makes the first step of `install + # sdk` (finding a Python) fail, which is deterministic, offline, writes + # nothing, and happens after the header is on stdout. That is also why the When + # tolerates a non-zero exit. No runtime state needed, so this runs on the mock + # lane and every other lane identically. + @id:runtime-freeform-yes-discloses-injected-consent + Scenario: runtime-15 - Disclosing the consent added to a natural-language install approved with --yes + Given the CLI cannot reach a usable Python + When the user approves a natural-language SDK install with --yes + Then the request plan shows an install command carrying no replacement consent + And the executed command carries the replacement consent + And the execution section says the consent came from the user's --yes diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index ab029f3f8..600abfedc 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -127,7 +127,7 @@ async fn setup_active_runtime(world: &mut E2eWorld) { world.use_shared_runtimes(); let (stdout, _, _) = crate::run_rocm(world, &["runtimes", "list"]); if stdout.contains("installed: none") { - crate::run_rocm_ok(world, &["install", "sdk"]); + crate::run_rocm_ok(world, &["install", "sdk", "--yes"]); } else { activate_shared_runtime_if_unset(world, &stdout); } @@ -153,7 +153,14 @@ async fn setup_runtime_with_engine(world: &mut E2eWorld) { world.use_shared_runtimes(); let (stdout, _, _) = crate::run_rocm(world, &["runtimes", "list"]); if stdout.contains("installed: none") { - crate::run_rocm_ok(world, &["install", "sdk"]); + // `--yes` for the same reason the sibling `a managed runtime is active` + // passes it: the harness spawns `rocm` with null stdin, so anything the + // consent gate does not read as an install with no active default + // refuses rather than prompts. `installed: none` no longer implies that + // on its own — the gate now keys on the config's active default, and a + // shared tree can carry one from a scenario that ran earlier — so the + // flag is load-bearing here, not just defensive. + crate::run_rocm_ok(world, &["install", "sdk", "--yes"]); } else { activate_shared_runtime_if_unset(world, &stdout); } @@ -205,8 +212,10 @@ async fn user_installs_sdk(world: &mut E2eWorld) { // opt-out is one — without the Gherkin naming an environment variable. The // exit code is still asserted here, with the same diagnostic bundle // `run_rocm_ok` prints: an install that failed leaves every Then behind it - // reading output that was never produced. - let args = ["install", "sdk"]; + // reading output that was never produced. `--yes` keeps the install + // non-interactive-safe: the e2e harness runs with null stdin, so the consent + // prompt would otherwise refuse rather than proceed. + let args = ["install", "sdk", "--yes"]; let (stdout, stderr, rc) = crate::run_rocm_with_scenario_env(world, &args); assert!( rc == 0, @@ -216,6 +225,65 @@ async fn user_installs_sdk(world: &mut E2eWorld) { world.cli_output = Some(stdout); } +#[when("the user reinstalls the SDK without confirming")] +async fn user_reinstalls_sdk_without_yes(world: &mut E2eWorld) { + let (stdout, stderr, rc) = crate::run_rocm(world, &["install", "sdk"]); + world.cli_output = Some(stdout); + world.cli_stderr = Some(stderr); + world.cli_rc = Some(rc); +} + +#[when("the user reinstalls the SDK with --yes")] +async fn user_reinstalls_sdk_with_yes(world: &mut E2eWorld) { + let stdout = crate::run_rocm_ok(world, &["install", "sdk", "--yes"]); + world.cli_output = Some(stdout); +} + +/// TheRock package families this step may ask for, in preference order. Real +/// published names, not placeholders: an unknown family is rejected during +/// resolution, which would exit non-zero for a reason that has nothing to do +/// with the consent gate and would still satisfy "the reinstall is refused". +/// Each is published as a `therock-dist-linux--.tar.gz` in the +/// canonical release tarball catalog, for the same reason: an artifact the +/// catalog does not carry fails resolution short of the gate. +const OTHER_FAMILY_CANDIDATES: &[&str] = &["gfx110X-all", "gfx120X-all", "gfx94X-dcgpu"]; + +#[when("the user installs a different GPU family without confirming")] +async fn user_installs_other_family_without_yes(world: &mut E2eWorld) { + // Pick a family the active runtime is not, rather than hard-coding one: + // this lane's GPU decides what the `Given` installed, and naming that same + // family would silently collapse this scenario into Scenario runtime-11. + // Matching is against the whole `runtimes list` text, which prints a + // case-preserving `family=` column — the runtime key alone would not do, + // since it is lowercase-slugified and would never match `gfx110X-all`. + let (runtimes, _, _) = crate::run_rocm(world, &["runtimes", "list"]); + let family = OTHER_FAMILY_CANDIDATES + .iter() + .find(|candidate| !runtimes.contains(*candidate)) + .copied() + .unwrap_or_else(|| { + panic!("no candidate family differs from the installed runtimes:\n{runtimes}") + }); + // `--format tarball`, because the wheel path cannot reach the consent gate + // with another family's name on a host that has a GPU. The wheel install + // composes its device payload from the target this host reports, and it + // validates that target against the resolved family *before* the gate + // (deliberately: an install that cannot work has to say so rather than first + // demand a consent flag for it). So `--family gfx110X-all` on a gfx942 host + // stops at "detected GPU target `gfx942` belongs to family `gfx94X-dcgpu`" + // and never reaches the displacement this scenario is about. The tarball + // path resolves the archive for the family it was given and consults no + // host target at all, so it reaches the same gate — the one call to + // `active_default_runtime_relation` shared by both formats — with a family + // the host has genuinely never held. The refusal still costs only the + // catalog listing: it bails before the multi-GiB archive is fetched. + let args = ["install", "sdk", "--format", "tarball", "--family", family]; + let (stdout, stderr, rc) = crate::run_rocm(world, &args); + world.cli_output = Some(stdout); + world.cli_stderr = Some(stderr); + world.cli_rc = Some(rc); +} + #[when("the user installs the SDK again")] async fn user_reinstalls_sdk(world: &mut E2eWorld) { user_installs_sdk(world).await; @@ -680,6 +748,23 @@ async fn assert_runtime_active(world: &mut E2eWorld) { ); } +#[then("the install reports that --yes approved replacing the existing runtime")] +async fn assert_install_reported_yes_approval(world: &mut E2eWorld) { + // The registered-and-active Thens are true from the `Given` alone, so they + // cannot tell an approved reinstall from a no-op. This asserts the approved + // branch was actually taken: with `--yes` the gate resolves to + // `ProceedApproved(AssumeYes)`, whose only externally visible signal is this + // line. The `Approved by --yes:` prefix is what discriminates — the + // fresh-install line ("No active ROCm SDK runtime is configured") does not + // carry it, so if `--yes` regressed to a refusal, or the install silently + // took the fresh path, this fails. + let output = world.cli_output.as_deref().expect("no install output"); + assert!( + output.contains("Approved by --yes: an existing ROCm SDK is the active default runtime"), + "reinstall with --yes did not report the approved replacement:\n{output}" + ); +} + #[then("the runtime includes an inference engine")] async fn assert_runtime_has_stack(world: &mut E2eWorld) { let (stdout, _, _) = crate::run_rocm(world, &["examine"]); @@ -827,6 +912,59 @@ async fn assert_adopt_error_explains(world: &mut E2eWorld) { ); } +#[then("the reinstall is refused")] +async fn assert_reinstall_refused(world: &mut E2eWorld) { + let rc = world.cli_rc.expect("no command was run"); + assert!( + rc != 0, + "install sdk unexpectedly succeeded without consent" + ); +} + +#[then("the error explains how to approve the replacement non-interactively")] +async fn assert_reinstall_error_explains_consent(world: &mut E2eWorld) { + let stdout = world.cli_output.as_deref().unwrap_or(""); + let stderr = world.cli_stderr.as_deref().unwrap_or(""); + let combined = format!("{stdout}{stderr}"); + // The narrow flag first, because this message is what a script or CI job + // reads: it is the whole consent needed here, while `--yes` would also + // approve a `sudo` system-package install whose password prompt an + // unattended caller cannot answer. + let narrow = combined + .find("--approve-replacing-active-default") + .unwrap_or_else(|| { + panic!("error does not name the narrow consent flag:\n{stdout}\n{stderr}") + }); + let yes = combined + .find("--yes") + .unwrap_or_else(|| panic!("error does not still explain --yes:\n{stdout}\n{stderr}")); + assert!( + narrow < yes, + "error recommends --yes ahead of the narrow flag:\n{stdout}\n{stderr}" + ); +} + +#[then("the error names the active default runtime it would replace")] +async fn assert_error_names_active_default(world: &mut E2eWorld) { + // What distinguishes the consent gate from any other non-zero exit that + // happens to print `--yes` in a usage line: only the gate reports the + // runtime it is about to displace, and for a family the host has never + // installed it must report the *active default* rather than claiming no SDK + // exists. Without this Then, a family the resolver rejected outright would + // satisfy the scenario. + let stdout = world.cli_output.as_deref().unwrap_or(""); + let stderr = world.cli_stderr.as_deref().unwrap_or(""); + let combined = format!("{stdout}{stderr}"); + assert!( + combined.contains("is the active default runtime"), + "error does not name the active default runtime it would replace:\n{stdout}\n{stderr}" + ); + assert!( + combined.contains("replaces active default"), + "error does not describe the cross-family displacement:\n{stdout}\n{stderr}" + ); +} + #[when("the user asks for rollback help")] async fn ask_rollback_help(world: &mut E2eWorld) { let stdout = crate::run_rocm_ok(world, &["runtimes", "rollback", "--help"]); @@ -841,3 +979,130 @@ async fn rollback_help_states_limit(world: &mut E2eWorld) { "expected `rocm runtimes rollback --help` to state the single-level limit, got:\n{out}" ); } + +#[when("the user asks for SDK install help")] +async fn ask_install_sdk_help(world: &mut E2eWorld) { + let stdout = crate::run_rocm_ok(world, &["install", "sdk", "--help"]); + world.cli_output = Some(stdout); +} + +#[then("the help offers a consent flag that does not approve system-package installs")] +async fn install_sdk_help_separates_consents(world: &mut E2eWorld) { + let out = world.cli_output.clone().unwrap_or_default(); + assert!( + out.contains("--approve-replacing-active-default"), + "expected `rocm install sdk --help` to offer the narrow consent flag, got:\n{out}" + ); + // The distinction is the point: without it a script author reads the flag as + // a synonym for `--yes` and reaches for `--yes`, which on a host without + // passwordless sudo raises a password prompt the script cannot answer. + assert!( + out.contains("does not approve system-package installs"), + "expected `rocm install sdk --help` to say the narrow flag excludes system-package installs, got:\n{out}" + ); +} + +/// Point the CLI at an interpreter that does not exist, so `install sdk` fails +/// on its very first step. +/// +/// A behavioural precondition, not a mechanism the feature file names — the same +/// idiom as the torch-alignment opt-out above. Scenario runtime-15 asserts on the +/// two sections `rocm --yes ` prints *before* it dispatches, and on the +/// GPU lanes the request it sends resolves to a real multi-GiB SDK install. This +/// makes `resolve_python_launcher` bail: offline, instantly, writing nothing, and +/// after the header is already on stdout. +#[given("the CLI cannot reach a usable Python")] +async fn setup_unusable_python(world: &mut E2eWorld) { + let missing = world + .isolated_root + .as_ref() + .expect("scenario has no isolated root") + .path() + .join("no-such-python"); + world + .command_env + .push(("ROCM_CLI_PYTHON", missing.into_os_string())); +} + +#[when("the user approves a natural-language SDK install with --yes")] +async fn user_approves_freeform_sdk_install(world: &mut E2eWorld) { + // A prefix inside the scenario's own temp root, so the words that make this a + // high-confidence `install sdk` plan cannot name a folder outside it even if + // the Given ever stops stopping the install. + let prefix = world + .isolated_root + .as_ref() + .expect("scenario has no isolated root") + .path() + .join("freeform-therock") + .to_string_lossy() + .into_owned(); + let request = format!("install the latest TheRock nightly for this GPU into {prefix}"); + // `run_rocm`, not `run_rocm_ok`: the Given guarantees the dispatched install + // fails, and the exit code is not what this scenario is about. + let (stdout, stderr, rc) = crate::run_rocm_with_scenario_env(world, &["--yes", &request]); + world.cli_output = Some(stdout); + world.cli_stderr = Some(stderr); + world.cli_rc = Some(rc); +} + +/// The `request plan` and `execution` halves of `rocm --yes ` output. +/// +/// Split rather than searched whole because both sections print a `note:` line +/// and a `tool_call:` line; asserting against the full text would let a match in +/// the wrong section satisfy the wrong claim. +fn freeform_plan_and_execution(world: &E2eWorld) -> (String, String) { + let out = world.cli_output.clone().unwrap_or_default(); + let (plan, execution) = out + .split_once("\nexecution\n") + .unwrap_or_else(|| panic!("no `execution` section in the freeform output:\n{out}")); + (plan.to_owned(), execution.to_owned()) +} + +#[then("the request plan shows an install command carrying no replacement consent")] +async fn assert_freeform_plan_is_unapproved(world: &mut E2eWorld) { + let (plan, _) = freeform_plan_and_execution(world); + assert!( + plan.contains("tool_call: rocm install sdk"), + "expected the request plan to propose an SDK install, got:\n{plan}" + ); + // The reviewable command a plain `rocm ` prints is this same render, + // so a consent flag reaching it would hand a human a pre-approved command. + assert!( + !plan.contains("--approve-replacing-active-default"), + "the request plan must stay unapproved, got:\n{plan}" + ); +} + +#[then("the executed command carries the replacement consent")] +async fn assert_freeform_execution_is_approved(world: &mut E2eWorld) { + let (_, execution) = freeform_plan_and_execution(world); + // The `tool_call:` line alone, not the whole section: the disclosure note + // below it quotes `--yes`, so a section-wide search could not tell a consent + // flag on the command from a mention of one in prose. + let executed = execution + .lines() + .find_map(|line| line.trim().strip_prefix("tool_call: ")) + .unwrap_or_else(|| panic!("no executed tool_call in:\n{execution}")); + assert!( + executed.starts_with("rocm install sdk") + && executed.contains("--approve-replacing-active-default"), + "expected the executed command to carry the narrow consent, got `{executed}`" + ); + // Never `--yes`: this surface spawns with no terminal on which to answer the + // sudo password prompt a system-package install can raise. + assert!( + !executed.split_whitespace().any(|arg| arg == "--yes"), + "the executed command must not carry `--yes`, got `{executed}`" + ); +} + +#[then("the execution section says the consent came from the user's --yes")] +async fn assert_freeform_execution_discloses_consent(world: &mut E2eWorld) { + let (_, execution) = freeform_plan_and_execution(world); + assert!( + execution.contains("was added here from your --yes"), + "the operator is shown a consent flag the plan above did not carry, with \ + nothing saying where it came from:\n{execution}" + ); +} diff --git a/xtask/src/e2e_prewarm.rs b/xtask/src/e2e_prewarm.rs index c28d8d62e..e990af28f 100644 --- a/xtask/src/e2e_prewarm.rs +++ b/xtask/src/e2e_prewarm.rs @@ -480,8 +480,20 @@ pub fn run(channel: &str, keep: usize, prewarm_dir: &Path) -> Result<()> { "pre-warm: installing the {channel} SDK into {}", prewarm_dir.display() ); + // `--yes` rather than `--approve-replacing-active-default`, and the + // second consent is why: pre-warm is provisioning, so it wants the + // required system packages installed too, and the runners it runs on + // have passwordless sudo for exactly that — no prompt is raised, and + // a package that cannot be installed warns and continues. + // + // The narrow flag would cover the first consent on its own: the gate + // keys on the tree's active default runtime, not on the channel + // `decide()` inspected, so a shared tree pre-warmed for `release` and + // then for `nightly` arrives here with a release runtime already + // active and no terminal to confirm on. It would just leave the + // packages behind. rocm_command(&rocm, prewarm_dir) - .args(["install", "sdk", "--channel", channel]) + .args(["install", "sdk", "--channel", channel, "--yes"]) .status_ok("rocm install sdk")?; } Decision::Update { runtime_key } => {