From 50edb1e8a019f0a4c02dca27e1841419cd78f170 Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Tue, 18 Aug 2026 10:07:08 +0000 Subject: [PATCH 01/20] feat(install): add --yes flag for non-interactive SDK installation and update tests Signed-off-by: Roman Sirokov --- apps/rocm/src/main.rs | 23 +- apps/rocm/src/therock.rs | 475 +++++++++++++++++- crates/rocm-core/src/lib.rs | 2 +- scripts/therock_sdk_install_test.py | 1 + .../features/runtime_setup.feature | 22 + tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 39 +- 6 files changed, 534 insertions(+), 28 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 75a75ffc3..2a606949a 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -602,7 +602,10 @@ 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 overwriting an existing ROCm SDK (and required system-package + /// installs such as OpenMPI for vLLM) without prompting; required to + /// overwrite an existing SDK outside an interactive terminal. A fresh + /// install (no existing SDK) never prompts. #[arg(long)] yes: bool, }, @@ -2436,12 +2439,14 @@ fn install(target: InstallTarget) -> Result<()> { version_selector, family.as_deref(), dry_run, + yes, ) { - 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 { @@ -12967,7 +12972,7 @@ 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) + Ok(therock::install_sdk(paths, channel, format, prefix, selector, None, true, true)?.output) } fn run_command_with_timeout( @@ -15701,9 +15706,10 @@ fn apply_runtime_update( None, None, true, + true, )?; let _ = writeln!(output, " install_plan:"); - for line in install_plan.lines() { + for line in install_plan.output.lines() { let _ = writeln!(output, " {line}"); } return Ok(output); @@ -15717,6 +15723,7 @@ fn apply_runtime_update( None, None, false, + true, )?; let manifests_after = therock::load_runtime_manifests(paths)?; let installed = select_installed_update_runtime(&manifests_after, source, &plan.latest_version) @@ -15755,7 +15762,7 @@ fn apply_runtime_update( ); } let _ = writeln!(output, " install_output:"); - for line in install_output.lines() { + for line in install_output.output.lines() { let _ = writeln!(output, " {line}"); } Ok(output) diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index c520acf2f..898f05e4f 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -5,13 +5,13 @@ use anyhow::{Context, Result, bail}; use rocm_core::{ AppPaths, ManagedToolConfig, RocmCliConfig, detect_host_gpu_diagnostics, - detect_host_therock_family, detect_managed_therock_family, disk_space, ensure_uv_binary, - 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_therock_family, detect_legacy_rocm_summary, detect_managed_therock_family, + disk_space, ensure_uv_binary, 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::{ @@ -132,6 +132,12 @@ struct PipRuntimeResolution { family_source: String, index_url: String, 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, } @@ -467,6 +473,34 @@ 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, + } + } +} + +#[allow(clippy::too_many_arguments)] pub(crate) fn install_sdk( paths: &AppPaths, channel: &str, @@ -475,7 +509,8 @@ pub(crate) fn install_sdk( version_selector: Option, family_override: Option<&str>, dry_run: bool, -) -> Result { + assume_yes: bool, +) -> Result { let channel = TheRockChannel::parse(channel)?; ensure_install_format_supported(format)?; match format { @@ -486,12 +521,13 @@ pub(crate) fn install_sdk( family_override, version_selector.as_ref(), dry_run, + assume_yes, ), "tarball" => { if version_selector.is_some() { bail!("specific TheRock version selection is only supported for wheel installs") } - install_tarball_runtime(paths, channel, prefix, family_override, dry_run) + install_tarball_runtime(paths, channel, prefix, family_override, dry_run, assume_yes) } other => bail!("unsupported install format: {other}"), } @@ -794,7 +830,8 @@ fn install_wheel_runtime( family_override: Option<&str>, version_selector: Option<&RuntimeVersionSelector>, dry_run: bool, -) -> Result { + assume_yes: bool, +) -> Result { progress_line(format!( "Checking Python for the ROCm install; if needed, ROCm CLI will prepare Python {}.", managed_python_version() @@ -862,6 +899,13 @@ fn install_wheel_runtime( " latest_compatible_version: {}", runtime_version_display(&resolution.latest_version) ); + if let Some(host_version) = host_rocm_version_newer_than(&resolution.latest_version) { + let _ = writeln!( + output, + " version_note: this host reports ROCm {host_version}, but {resolved} is the newest TheRock ROCm with a matching PyTorch stack, so it is selected; pass `--version ` to override", + resolved = runtime_version_display(&resolution.latest_version) + ); + } let _ = writeln!( output, " compatibility_key: {}", @@ -894,6 +938,19 @@ fn install_wheel_runtime( output, " package_policy: find the newest TheRock ROCm SDK version that has a matching PyTorch stack in the same index, then install pinned rocm[libraries,devel], torch, torchvision, and torchaudio versions in one uv transaction" ); + let no_wheel_warning = repo_version_without_wheels( + resolution.newest_repo_version.as_deref(), + &resolution.latest_version, + ) + .map(|newest| { + format!( + "ROCm {newest} is the newest version in this repository but has no installable PyTorch wheels for this Python and platform; installing ROCm {resolved} instead", + resolved = 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); @@ -923,7 +980,70 @@ 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_rocm_version_newer_than(&resolution.latest_version) { + 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) + )); + } + + // Fresh installs proceed with just an informational line; only overwriting an + // existing managed SDK asks for confirmation (and needs `--yes` when there is + // no terminal to answer the prompt). + let existing = existing_runtime_relation( + paths, + channel, + &resolution.family, + &resolution.latest_version, + )?; + match sdk_install_approval(existing.is_some(), assume_yes, interactive_terminal()) { + SdkInstallApproval::ProceedFresh => { + progress_line(format!( + "No existing ROCm SDK found; installing ROCm SDK {} for family {}.", + runtime_version_display(&resolution.latest_version), + resolution.family + )); + } + SdkInstallApproval::ProceedApproved => { + progress_line(format!( + "Overwriting existing ROCm SDK ({}) with ROCm {}.", + 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!( + "an existing ROCm SDK ({}) would be overwritten; re-run with --yes to overwrite it non-interactively, for example `rocm install sdk --yes`", + existing.as_deref().unwrap_or_default() + ); + } } let uv = ensure_uv_binary(paths)?; @@ -1018,7 +1138,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(package_versions: &TheRockPipPackageVersions) -> Vec { @@ -1042,13 +1162,149 @@ fn quote_display_arg(value: &str) -> String { } } +/// Classify how the version rocm-cli is about to install relates to the managed +/// runtime it would displace as the active default — the newest existing install +/// for the same GPU family and channel. Returns `None` when no such runtime +/// exists (a fresh install for this family/channel). +/// +/// Errors reading the manifest directory are propagated rather than treated as +/// "no existing runtime": silently falling back to a fresh-install verdict on a +/// read error would skip the overwrite confirmation gate precisely when we are +/// least sure whether an existing SDK is present. +fn existing_runtime_relation( + paths: &AppPaths, + channel: TheRockChannel, + family: &str, + resolved_version: &str, +) -> Result> { + let manifests = load_runtime_manifests(paths)?; + let mut matching = manifests + .into_iter() + .filter(|manifest| manifest.family == family && manifest.channel == channel.as_str()) + .collect::>(); + matching.sort_by_key(|manifest| std::cmp::Reverse(manifest.installed_at_unix_ms)); + let Some(existing) = matching.into_iter().next() else { + return Ok(None); + }; + let relation = match compare_version_strings(resolved_version, &existing.version) { + Ordering::Greater => "upgrade", + Ordering::Less => "downgrade", + Ordering::Equal => "reinstall", + }; + Ok(Some(format!( + "{relation} from installed {installed} ({key})", + installed = runtime_version_display(&existing.version), + key = existing.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 { + let host_version = detect_legacy_rocm_summary().version?; + match compare_version_strings(&host_version, resolved_version) { + Ordering::Greater => Some(host_version), + _ => None, + } +} + +/// 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, + } +} + +/// Whether a real SDK install needs the user's approval before it runs. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum SdkInstallApproval { + /// No existing managed SDK for this family+channel — install without asking. + ProceedFresh, + /// An existing SDK is present and `--yes` was given — overwrite it silently. + ProceedApproved, + /// An existing SDK is present and there is a terminal — prompt to overwrite. + PromptOverwrite, + /// An existing SDK is present but there is no terminal and no `--yes` — refuse. + RefuseNonInteractive, +} + +/// Decide whether an SDK install proceeds, prompts, or is refused. Fresh installs +/// (no `existing` runtime) always proceed; only overwriting an existing SDK needs +/// confirmation, and outside an interactive terminal that confirmation must come +/// from `--yes`. +const fn sdk_install_approval( + existing: bool, + assume_yes: bool, + interactive: bool, +) -> SdkInstallApproval { + if !existing { + SdkInstallApproval::ProceedFresh + } else if assume_yes { + SdkInstallApproval::ProceedApproved + } else if interactive { + SdkInstallApproval::PromptOverwrite + } else { + SdkInstallApproval::RefuseNonInteractive + } +} + +/// Interactive confirmation gate for overwriting an existing managed SDK. Prints +/// what would be replaced, then reads a yes/no answer from stdin. Only reached +/// when an existing SDK is present, `--yes` was not passed, and a terminal is +/// attached (see `sdk_install_approval`). +fn confirm_overwrite_existing_sdk( + channel: TheRockChannel, + family: &str, + resolved_version: &str, + relation: &str, +) -> Result { + println!("sdk install: an existing ROCm SDK was found"); + println!(" existing 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("Overwrite the existing ROCm SDK? [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")) +} + fn install_tarball_runtime( paths: &AppPaths, channel: TheRockChannel, prefix: Option, family_override: Option<&str>, dry_run: bool, -) -> Result { + assume_yes: bool, +) -> Result { let artifact = resolve_tarball_artifact(paths, channel, family_override)?; let runtime_key = runtime_key( channel, @@ -1073,13 +1329,71 @@ fn install_tarball_runtime( " latest_version: {}", runtime_version_display(&artifact.version) ); + if let Some(host_version) = host_rocm_version_newer_than(&artifact.version) { + let _ = writeln!( + output, + " version_note: this host reports ROCm {host_version}, but {resolved} is the newest TheRock ROCm tarball for this GPU family, so it is selected", + resolved = 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_rocm_version_newer_than(&artifact.version) { + 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) + )); + } + + // Fresh installs proceed with just an informational line; only overwriting an + // existing managed SDK asks for confirmation (and needs `--yes` when there is + // no terminal to answer the prompt). + let existing = existing_runtime_relation(paths, channel, &artifact.family, &artifact.version)?; + match sdk_install_approval(existing.is_some(), assume_yes, interactive_terminal()) { + SdkInstallApproval::ProceedFresh => { + progress_line(format!( + "No existing ROCm SDK found; installing ROCm SDK {} for family {}.", + runtime_version_display(&artifact.version), + artifact.family + )); + } + SdkInstallApproval::ProceedApproved => { + progress_line(format!( + "Overwriting existing ROCm SDK ({}) with ROCm {}.", + 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!( + "an existing ROCm SDK ({}) would be overwritten; re-run with --yes to overwrite it non-interactively, for example `rocm install sdk --yes`", + existing.as_deref().unwrap_or_default() + ); + } } fs::create_dir_all(paths.cache_dir.join("therock"))?; @@ -1123,7 +1437,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( @@ -1238,11 +1552,22 @@ 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(), latest_version, + newest_repo_version, package_versions, }) } @@ -5566,7 +5891,7 @@ echo Python 3.12.10 cache_dir: root.join("cache"), }; - let error = install_sdk(&paths, "release", "tarball", None, None, None, true) + let error = install_sdk(&paths, "release", "tarball", None, None, None, true, true) .unwrap_err() .to_string(); @@ -5574,6 +5899,124 @@ echo Python 3.12.10 assert!(error.contains("rocm install sdk --format wheel")); } + #[test] + fn existing_runtime_relation_classifies_upgrade_downgrade_reinstall() -> Result<()> { + let (root, paths) = test_paths("existing-runtime-relation"); + let mut manifest = test_runtime_manifest( + "release-wheel-gfx120X-all", + "therock-release:gfx120X-all", + 10, + ); + manifest.version = "7.13.0".to_owned(); + write_test_runtime_manifest(&paths, &manifest)?; + + let upgrade = + existing_runtime_relation(&paths, TheRockChannel::Release, "gfx120X-all", "7.14.0")? + .expect("relation should be reported for a matching runtime"); + assert!(upgrade.starts_with("upgrade from"), "got: {upgrade}"); + assert!(upgrade.contains("7.13.0")); + + let downgrade = + existing_runtime_relation(&paths, TheRockChannel::Release, "gfx120X-all", "7.12.0")? + .expect("relation should be reported for a matching runtime"); + assert!(downgrade.starts_with("downgrade from"), "got: {downgrade}"); + + let reinstall = + existing_runtime_relation(&paths, TheRockChannel::Release, "gfx120X-all", "7.13.0")? + .expect("relation should be reported for a matching runtime"); + assert!(reinstall.starts_with("reinstall from"), "got: {reinstall}"); + + // A different family or channel is a fresh install, so no relation applies. + assert!( + existing_runtime_relation(&paths, TheRockChannel::Release, "gfx110X-all", "7.14.0")? + .is_none() + ); + assert!( + existing_runtime_relation(&paths, TheRockChannel::Nightly, "gfx120X-all", "7.14.0")? + .is_none() + ); + + let _ = fs::remove_dir_all(root); + Ok(()) + } + + #[test] + fn existing_runtime_relation_none_without_managed_runtimes() -> Result<()> { + let (root, paths) = test_paths("existing-runtime-relation-empty"); + assert!( + existing_runtime_relation(&paths, TheRockChannel::Release, "gfx120X-all", "7.14.0")? + .is_none() + ); + let _ = fs::remove_dir_all(root); + Ok(()) + } + + #[test] + fn existing_runtime_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 existing runtime" — that would skip the + // overwrite confirmation gate exactly when we're least sure whether an + // existing SDK is present. + let (root, paths) = test_paths("existing-runtime-relation-error"); + let registry_dir = paths.data_dir.join("runtimes").join("registry"); + fs::create_dir_all(registry_dir.join("broken.json"))?; + + let error = + existing_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 sdk_install_approval_only_prompts_when_overwriting_existing() { + // Fresh install: no existing SDK -> never prompt, regardless of terminal/--yes. + assert_eq!( + sdk_install_approval(false, false, false), + SdkInstallApproval::ProceedFresh + ); + assert_eq!( + sdk_install_approval(false, false, true), + SdkInstallApproval::ProceedFresh + ); + assert_eq!( + sdk_install_approval(false, true, false), + SdkInstallApproval::ProceedFresh + ); + + // Existing SDK: --yes overwrites silently; a terminal prompts; neither refuses. + assert_eq!( + sdk_install_approval(true, true, false), + SdkInstallApproval::ProceedApproved + ); + assert_eq!( + sdk_install_approval(true, false, true), + SdkInstallApproval::PromptOverwrite + ); + assert_eq!( + sdk_install_approval(true, false, false), + SdkInstallApproval::RefuseNonInteractive + ); + } + + #[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()); + } + fn test_paths(name: &str) -> (PathBuf, AppPaths) { let root = workspace_test_artifact_dir().join(format!( "rocm-cli-therock-test-{name}-{}-{}", diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index ec50c0724..699cd9564 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -2725,7 +2725,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/scripts/therock_sdk_install_test.py b/scripts/therock_sdk_install_test.py index 5e0db569b..e7798647e 100644 --- a/scripts/therock_sdk_install_test.py +++ b/scripts/therock_sdk_install_test.py @@ -473,6 +473,7 @@ def main() -> int: args.channel, "--format", "wheel", + "--yes", ] if args.prefix is not None: install_argv.extend(["--prefix", str(args.prefix)]) diff --git a/tests/e2e-cucumber/features/runtime_setup.feature b/tests/e2e-cucumber/features/runtime_setup.feature index c926591e5..ad0fab4fb 100644 --- a/tests/e2e-cucumber/features/runtime_setup.feature +++ b/tests/e2e-cucumber/features/runtime_setup.feature @@ -131,3 +131,25 @@ Feature: Runtime configuration When the user tries to adopt the existing install Then the adoption is refused And the error explains which install types can be adopted + + # Reinstalling over an existing managed SDK must not silently clobber the + # active runtime. Outside an interactive terminal (as every e2e invocation + # is here), `install sdk` without `--yes` must refuse rather than overwrite. + # Cheap even though GPU-gated: the precondition needs a GPU to have a runtime + # active, but the refusal itself bails before any download. + @id:runtime-install-sdk-overwrite-requires-yes @requires-gpu + Scenario: 6 - Reinstalling the SDK over an existing runtime without --yes 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 that --yes is required + + # Companion to Scenario 6: with --yes the same reinstall proceeds and the + # runtime stays registered and active afterward. Nightly-gated in addition to + # GPU because, unlike Scenario 6, this exercises a real second SDK install. + @id:runtime-install-sdk-overwrite-with-yes @requires-gpu @nightly + Scenario: 7 - Reinstalling the SDK over an existing runtime with --yes overwrites it + Given a managed runtime is active + When the user reinstalls the SDK with --yes + Then a runtime is registered + And the runtime is set as active diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index ea88af310..22a81247e 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"]); } // Name the runtime rather than leaving the CLI to infer it: the shared tree // grows a second runtime whenever the channel index publishes one, and the @@ -181,8 +181,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 an overwrite + // 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, @@ -192,6 +194,20 @@ 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); +} + #[when("the user installs the SDK again")] async fn user_reinstalls_sdk(world: &mut E2eWorld) { user_installs_sdk(world).await; @@ -591,3 +607,20 @@ async fn assert_adopt_error_explains(world: &mut E2eWorld) { "error does not explain TheRock requirement:\n{stdout}\n{stderr}" ); } + +#[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 --yes"); +} + +#[then("the error explains that --yes is required")] +async fn assert_reinstall_error_explains_yes(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}").to_lowercase(); + assert!( + combined.contains("--yes"), + "error does not mention --yes:\n{stdout}\n{stderr}" + ); +} From 0295ebf8c26997d1e3137f36517f227e68421c05 Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Fri, 28 Aug 2026 11:36:53 +0000 Subject: [PATCH 02/20] test(install): cover the host-vs-TheRock version-note explanation (EAI-7326) The legacy-ROCm-vs-TheRock version explanation (host_rocm_version_newer_than and the version_note/warning lines it drives in both the wheel and tarball install paths) shipped without direct coverage. Split the filesystem probe out of host_rocm_version_newer_than into a pure host_version_newer_than core, and route the version_note/warning strings through small pure builders. Add unit tests for the newer-than decision and the exact note/warning wording. No behavior change. Signed-off-by: Roman Sirokov --- apps/rocm/src/therock.rs | 91 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 83 insertions(+), 8 deletions(-) diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 898f05e4f..43e3f3014 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -902,8 +902,11 @@ fn install_wheel_runtime( if let Some(host_version) = host_rocm_version_newer_than(&resolution.latest_version) { let _ = writeln!( output, - " version_note: this host reports ROCm {host_version}, but {resolved} is the newest TheRock ROCm with a matching PyTorch stack, so it is selected; pass `--version ` to override", - resolved = runtime_version_display(&resolution.latest_version) + " version_note: {}", + wheel_host_version_note( + &host_version, + &runtime_version_display(&resolution.latest_version) + ) ); } let _ = writeln!( @@ -943,9 +946,9 @@ fn install_wheel_runtime( &resolution.latest_version, ) .map(|newest| { - format!( - "ROCm {newest} is the newest version in this repository but has no installable PyTorch wheels for this Python and platform; installing ROCm {resolved} instead", - resolved = runtime_version_display(&resolution.latest_version) + no_wheel_warning_message( + &newest, + &runtime_version_display(&resolution.latest_version), ) }); if let Some(warning) = no_wheel_warning.as_deref() { @@ -1202,13 +1205,46 @@ fn existing_runtime_relation( /// 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 { - let host_version = detect_legacy_rocm_summary().version?; + 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?; match compare_version_strings(&host_version, resolved_version) { Ordering::Greater => Some(host_version), _ => None, } } +/// 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. @@ -1332,8 +1368,8 @@ fn install_tarball_runtime( if let Some(host_version) = host_rocm_version_newer_than(&artifact.version) { let _ = writeln!( output, - " version_note: this host reports ROCm {host_version}, but {resolved} is the newest TheRock ROCm tarball for this GPU family, so it is selected", - resolved = runtime_version_display(&artifact.version) + " version_note: {}", + tarball_host_version_note(&host_version, &runtime_version_display(&artifact.version)) ); } let _ = writeln!(output, " target: {}", install_root.display()); @@ -6017,6 +6053,45 @@ echo Python 3.12.10 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()); + } + + #[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}-{}-{}", From 46d14a8b8ed3c0e57abbb14012fb8545da304a69 Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Thu, 3 Sep 2026 10:08:52 +0000 Subject: [PATCH 03/20] =?UTF-8?q?install:=20address=20--yes=20review=20?= =?UTF-8?q?=E2=80=94=20propagate=20flag,=20fix=20host-newer=20parse,=20hon?= =?UTF-8?q?est=20audit=20+=20scenario=20(EAI-7326)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Non-interactive callers (dashboard install manager, onboarding wizard, chat/MCP install_sdk) now pass --yes so a null-stdin spawn is not refused at the overwrite prompt. host_version_newer_than parses both versions leniently (build suffix, two-component) and returns None when either cannot be parsed, instead of a lexicographic compare that wrongly ranked e.g. 7.2.4-98 above 7.13.0. A declined real install is recorded as 'cancelled' rather than 'completed' in the audit trail. Reworded the runtime_setup overwrite-refusal comment to match where the refusal actually bails, and gave scenario 7 a distinguishing 'reports overwriting' assertion so it can no longer pass as a no-op. Signed-off-by: Roman Sirokov --- apps/rocm/src/main.rs | 35 ++++++-- apps/rocm/src/therock.rs | 80 ++++++++++++++++++- .../rocm-dash-tui/src/ui/install_manager.rs | 9 +++ crates/rocm-dash-tui/src/ui/onboarding.rs | 14 +++- .../features/runtime_setup.feature | 20 +++-- tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 15 ++++ 6 files changed, 153 insertions(+), 20 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 2a606949a..0d9da3967 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -2467,9 +2467,20 @@ 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) }, @@ -11524,6 +11535,12 @@ 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 an overwrite prompt would + // refuse with "re-run with `--yes`" — a flag the user cannot supply + // through chat. Add it here, matching the `install driver` and + // `services stop/restart` arms below. + ensure_flag(&mut args, "--yes"); Ok(ChatRocmCommandAction::Approval { args, pending_title: "Install ROCm".to_owned(), @@ -13340,6 +13357,10 @@ fn rocm_chat_tool_requested_args(call: &providers::ChatToolCall) -> Option Option { /// 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?; - match compare_version_strings(&host_version, resolved_version) { - Ordering::Greater => Some(host_version), - _ => None, - } + // Only claim the host is newer when BOTH versions parse and the host is + // strictly greater. An unparseable host string — an odd build suffix + // (`7.2.4-98`) or a truncated two-component report (`7.4`) — is "can't + // tell", not "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 @@ -4045,6 +4050,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, + }) +} + fn therock_index_urls(channel: TheRockChannel, family: &str) -> Vec { match channel { TheRockChannel::Release => vec![ @@ -6066,6 +6119,25 @@ echo Python 3.12.10 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] diff --git a/crates/rocm-dash-tui/src/ui/install_manager.rs b/crates/rocm-dash-tui/src/ui/install_manager.rs index 4c3dca740..32547d7a0 100644 --- a/crates/rocm-dash-tui/src/ui/install_manager.rs +++ b/crates/rocm-dash-tui/src/ui/install_manager.rs @@ -153,6 +153,12 @@ 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 overwrite an existing SDK cannot answer the confirmation + // prompt and would refuse. Pass `--yes` so the dashboard install + // proceeds; the dry-run preview never mutates, so it needs no flag. + args.push("--yes".to_string()); } Ok(args) } @@ -478,6 +484,9 @@ 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 --yes so the null-stdin + // dashboard spawn is not refused at the overwrite prompt. + 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 98b665964..b9d59fc8e 100644 --- a/crates/rocm-dash-tui/src/ui/onboarding.rs +++ b/crates/rocm-dash-tui/src/ui/onboarding.rs @@ -190,6 +190,10 @@ 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 + // overwrite prompt cannot be answered and the install would refuse. + // `--yes` keeps the first-run install non-interactive. + "--yes".to_string(), ]; let pin = cfg.pin_value.trim(); if let (Some(flag), false) = (cfg.pin_mode.arg(), pin.is_empty()) { @@ -671,7 +675,8 @@ mod tests { "--channel", "release", "--format", - "wheel" + "wheel", + "--yes" ], "default Release path must stay byte-identical to the pre-toggle args" ); @@ -918,7 +923,8 @@ mod tests { "--channel", "nightly", "--format", - "wheel" + "wheel", + "--yes" ] ); } @@ -948,6 +954,7 @@ mod tests { "nightly", "--format", "wheel", + "--yes", "--build-date", "2026-06-05" ] @@ -983,7 +990,8 @@ mod tests { "--channel", "release", "--format", - "wheel" + "wheel", + "--yes" ], "an empty pin must not add a flag" ); diff --git a/tests/e2e-cucumber/features/runtime_setup.feature b/tests/e2e-cucumber/features/runtime_setup.feature index ad0fab4fb..4897477e8 100644 --- a/tests/e2e-cucumber/features/runtime_setup.feature +++ b/tests/e2e-cucumber/features/runtime_setup.feature @@ -135,21 +135,27 @@ Feature: Runtime configuration # Reinstalling over an existing managed SDK must not silently clobber the # active runtime. Outside an interactive terminal (as every e2e invocation # is here), `install sdk` without `--yes` must refuse rather than overwrite. - # Cheap even though GPU-gated: the precondition needs a GPU to have a runtime - # active, but the refusal itself bails before any download. + # GPU-gated because the precondition needs a GPU to have a runtime active. + # The refusal resolves the Python launcher and reads the channel index first + # (both cheap) to learn which runtime would be overwritten, then bails before + # the SDK and torch packages are downloaded or anything on disk is changed. @id:runtime-install-sdk-overwrite-requires-yes @requires-gpu - Scenario: 6 - Reinstalling the SDK over an existing runtime without --yes is refused + Scenario: runtime-08 - Reinstalling the SDK over an existing runtime without --yes 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 that --yes is required - # Companion to Scenario 6: with --yes the same reinstall proceeds and the + # Companion to Scenario runtime-08: with --yes the same reinstall proceeds and the # runtime stays registered and active afterward. Nightly-gated in addition to - # GPU because, unlike Scenario 6, this exercises a real second SDK install. + # GPU because, unlike Scenario runtime-08, this exercises a real second SDK install. + # The registered/active Thens hold from the Given alone, so the overwrite 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: 7 - Reinstalling the SDK over an existing runtime with --yes overwrites it + Scenario: runtime-09 - Reinstalling the SDK over an existing runtime with --yes overwrites it Given a managed runtime is active When the user reinstalls the SDK with --yes - Then a runtime is registered + Then the install reports overwriting the existing runtime + And a runtime is registered And the runtime is set as active diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index 22a81247e..d3d1d4604 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -467,6 +467,21 @@ async fn assert_runtime_active(world: &mut E2eWorld) { ); } +#[then("the install reports overwriting the existing runtime")] +async fn assert_install_overwrote_existing(world: &mut E2eWorld) { + // The registered-and-active Thens are true from the `Given` alone, so they + // cannot tell an overwrite from a no-op. This asserts the overwrite branch + // was actually taken: with `--yes` the approval gate resolves to + // `ProceedApproved`, whose only externally visible signal is this line. If + // `--yes` regressed to a refusal, or the install silently took the fresh + // path, this line is absent and the scenario fails. + let output = world.cli_output.as_deref().expect("no install output"); + assert!( + output.contains("Overwriting existing ROCm SDK"), + "reinstall with --yes did not report overwriting the existing runtime:\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"]); From 82281a65d65c6710c7174e38c936853babc6f82f Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Tue, 8 Sep 2026 11:38:12 +0000 Subject: [PATCH 04/20] docs(install): document --yes for SDK reinstall and cover chat/MCP plumbing (EAI-7956) The --yes contract change for `rocm install sdk` was not reflected in the user-facing docs, and the chat/MCP arms that inject --yes had no regression test. - README.md: add [--yes] to the install sdk synopsis and explain that re-running over an existing managed SDK prompts (or refuses when spawned non-interactively) unless --yes is passed; note the same in the quickstart. - docs/testing.md: the live SDK acceptance test command now shows --yes, matching scripts/therock_sdk_install_test.py, so a reused test root is not refused at the overwrite prompt. - apps/rocm: add a regression test asserting both chat_rocm_command_action_from_args and rocm_chat_tool_requested_args inject --yes for install sdk, so the dashboard/assistant reinstall path cannot silently regress to a refusal. Signed-off-by: Roman Sirokov --- README.md | 15 ++++++++++----- apps/rocm/src/main.rs | 41 +++++++++++++++++++++++++++++++++++++++++ docs/testing.md | 7 ++++++- 3 files changed, 57 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 26edffe22..e67295703 100644 --- a/README.md +++ b/README.md @@ -199,7 +199,9 @@ 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. Re-running the command over a +managed runtime it already created asks before overwriting; add `--yes` to +approve that overwrite non-interactively, such as from a script. Then serve a model: @@ -250,7 +252,7 @@ the JSON report, not the human-readable one. ``` 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] + [--family gfx110X-all] [--prefix PATH] [--yes] [--dry-run] rocm install driver [--dkms] [--yes] [--dry-run] [--reconcile] @@ -258,9 +260,12 @@ 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. +by rocm-cli. A fresh install never prompts, but re-running it over an existing +managed SDK asks before overwriting; pass `--yes` to approve the overwrite +non-interactively (for example in scripts or CI, where the prompt would +otherwise refuse). `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. ### Runtime management diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 0d9da3967..9bbbe74ad 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -22364,6 +22364,47 @@ model recipes } } + #[test] + fn install_sdk_chat_and_mcp_args_carry_yes_for_non_interactive_spawn() { + // The chat/MCP surfaces spawn `rocm` with null stdin, so an overwrite + // prompt would refuse with "re-run with `--yes`" — 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 `--yes` so a reinstall + // over an existing SDK 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"); + match action { + ChatRocmCommandAction::Approval { args, .. } => assert!( + args.iter().any(|arg| arg == "--yes"), + "chat `install sdk` approval must carry --yes, got {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 `--yes` independently. + let call = providers::ChatToolCall { + id: None, + name: "install_sdk".to_owned(), + arguments: serde_json::json!({ "channel": "release", "format": "wheel" }), + }; + let args = rocm_chat_tool_requested_args(&call).expect("install_sdk tool builds args"); + assert!( + args.iter().any(|arg| arg == "--yes"), + "MCP install_sdk args must carry --yes, got {args:?}" + ); + } + #[test] fn chat_rocm_command_runs_read_only_and_rejects_risky_shapes() { let status = providers::ChatToolCall { diff --git a/docs/testing.md b/docs/testing.md index 3dc8256db..d6aa9fc9e 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -153,9 +153,14 @@ 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 overwriting an existing managed SDK without prompting, which +keeps the command non-interactive when the test root is reused across runs (a +fresh root never prompts). It matches the invocation in +`scripts/therock_sdk_install_test.py`. + Then it verifies: - runtime manifest metadata From 62105bb5216bd08cf7ebeea96d9e92f2ac5600d2 Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Wed, 9 Sep 2026 16:24:08 +0300 Subject: [PATCH 05/20] test(e2e): pass --yes on the remaining shared-tree SDK install (EAI-7956) `a managed runtime with an inference engine already installed` was the one shared-tree install step this PR left on a bare `install sdk`, while its sibling `a managed runtime is active` gained `--yes`. The `installed: none` guard means it is a fresh install today, so neither shape can reach the overwrite gate, but the asymmetry is a trap: the harness spawns `rocm` with null stdin, so if that guard ever loosens the step fails at a prompt nothing can answer instead of proceeding. Also correct the Scenario runtime-08 comment. It claimed the launcher and index resolution ahead of the approval gate are "both cheap"; on a cold host the launcher step can bootstrap a managed Python and download uv. The gate keys on the resolved family and version, so it cannot move ahead of that resolution. Say what is actually true: warm here because the Given installed a runtime, and what the refusal genuinely bails before is the SDK/torch download and any on-disk change. Signed-off-by: Roman Sirokov --- tests/e2e-cucumber/features/runtime_setup.feature | 10 +++++++--- tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 9 ++++++++- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/tests/e2e-cucumber/features/runtime_setup.feature b/tests/e2e-cucumber/features/runtime_setup.feature index 4897477e8..35ec8206c 100644 --- a/tests/e2e-cucumber/features/runtime_setup.feature +++ b/tests/e2e-cucumber/features/runtime_setup.feature @@ -136,9 +136,13 @@ Feature: Runtime configuration # active runtime. Outside an interactive terminal (as every e2e invocation # is here), `install sdk` without `--yes` must refuse rather than overwrite. # GPU-gated because the precondition needs a GPU to have a runtime active. - # The refusal resolves the Python launcher and reads the channel index first - # (both cheap) to learn which runtime would be overwritten, then bails before - # the SDK and torch packages are downloaded or anything on disk is changed. + # The refusal is not free: the gate keys on the resolved family and version, 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-08 - Reinstalling the SDK over an existing runtime without --yes is refused Given a managed runtime is active diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index d3d1d4604..918050b94 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -151,7 +151,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 + // overwrite gate does not read as a fresh install refuses rather than + // prompts. The `installed: none` guard makes this a fresh install today, + // so the flag changes nothing — it keeps the step correct if that guard + // ever loosens, instead of failing the lane at a prompt nothing can + // answer. + crate::run_rocm_ok(world, &["install", "sdk", "--yes"]); } // Same reason as `a managed runtime is active`: pin the runtime explicitly, // or the serve that follows refuses to pick one. Not for `assert_engine_ready` From 44e849a2a6831ed985651e4b1523099284158f33 Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Thu, 10 Sep 2026 10:32:14 +0300 Subject: [PATCH 06/20] fix(install): add --yes to the rocmd MCP install_sdk spawn and correct gate wording (EAI-7956) The `install_sdk` MCP tool in `apps/rocmd` builds its own `install sdk` argv, independently of the `apps/rocm` chat/MCP builder this PR already patched, and `run_rocm_capture_for_paths` spawns the child with null stdin. With an existing managed SDK for the family/channel the approval gate therefore resolved to `RefuseNonInteractive` and bailed asking for `--yes`, a flag no MCP caller of that tool could supply. Push `--yes` on the non-dry-run path only; the dry-run path returns before the gate. Consent is unchanged: `install_sdk` is in `mcp_tool_requires_direct_approval`, so a direct `rocmd mcp-call` still needs `--allow-mutation` after an explicit user approval. The gate's user-facing text also claimed an overwrite that does not happen. `runtime_key` embeds the resolved version, so an upgrade or downgrade gets its own install root and manifest and the previous install survives; only the active-default pointer moves. Reword the approved-install line, both refusal bails and the interactive prompt to state that effect, matching the wording the `existing_runtime_relation` docstring and the prompt's `effect:` line already used, and update the README, testing doc and the runtime-09 scenario to match. Signed-off-by: Roman Sirokov --- README.md | 16 ++++--- apps/rocm/src/therock.rs | 45 ++++++++++++------- apps/rocmd/src/lib.rs | 44 ++++++++++++++++++ docs/testing.md | 6 +-- .../features/runtime_setup.feature | 10 ++--- tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 19 ++++---- 6 files changed, 100 insertions(+), 40 deletions(-) diff --git a/README.md b/README.md index e67295703..6d59e7027 100644 --- a/README.md +++ b/README.md @@ -199,9 +199,10 @@ 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. Re-running the command over a -managed runtime it already created asks before overwriting; add `--yes` to -approve that overwrite non-interactively, such as from a script. +creates a separate managed runtime alongside it. Re-running the command when it +already created a managed runtime for the same GPU family and channel asks +first, because the new install takes over as the active default; add `--yes` to +approve that non-interactively, such as from a script. Then serve a model: @@ -260,10 +261,13 @@ rocm update [--apply] [--runtime KEY] [--activate] [--dry-run] ``` `install sdk` downloads TheRock ROCm wheels into a Python environment managed -by rocm-cli. A fresh install never prompts, but re-running it over an existing -managed SDK asks before overwriting; pass `--yes` to approve the overwrite +by rocm-cli. A fresh install never prompts, but when a managed SDK already +exists for the same GPU family and channel it asks first, because the new +install becomes the active default in its place; pass `--yes` to approve that non-interactively (for example in scripts or CI, where the prompt would -otherwise refuse). `install driver` installs the AMD kernel driver on Linux +otherwise refuse). Because the install root and manifest are keyed by version, +an upgrade or downgrade keeps the previous install on disk — only a same-version +reinstall replaces it in place. `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. diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 0b4e773b7..d640532a7 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -1003,9 +1003,10 @@ fn install_wheel_runtime( )); } - // Fresh installs proceed with just an informational line; only overwriting an - // existing managed SDK asks for confirmation (and needs `--yes` when there is - // no terminal to answer the prompt). + // Fresh installs proceed with just an informational line. Only an install + // that displaces an existing managed SDK for this family/channel as the + // active default asks for confirmation (and needs `--yes` when there is no + // terminal to answer the prompt). let existing = existing_runtime_relation( paths, channel, @@ -1022,7 +1023,7 @@ fn install_wheel_runtime( } SdkInstallApproval::ProceedApproved => { progress_line(format!( - "Overwriting existing ROCm SDK ({}) with ROCm {}.", + "Approved by --yes: an existing ROCm SDK was found ({}); installing ROCm {}, which becomes the active default runtime.", existing.as_deref().unwrap_or_default(), runtime_version_display(&resolution.latest_version) )); @@ -1043,7 +1044,7 @@ fn install_wheel_runtime( } SdkInstallApproval::RefuseNonInteractive => { bail!( - "an existing ROCm SDK ({}) would be overwritten; re-run with --yes to overwrite it non-interactively, for example `rocm install sdk --yes`", + "an existing ROCm SDK was found ({}); continuing would make the newly installed ROCm the active default runtime. Re-run with --yes to approve this non-interactively, for example `rocm install sdk --yes`", existing.as_deref().unwrap_or_default() ); } @@ -1271,18 +1272,20 @@ fn repo_version_without_wheels( enum SdkInstallApproval { /// No existing managed SDK for this family+channel — install without asking. ProceedFresh, - /// An existing SDK is present and `--yes` was given — overwrite it silently. + /// An existing SDK is present and `--yes` was given — displace it as the + /// active default without asking. ProceedApproved, - /// An existing SDK is present and there is a terminal — prompt to overwrite. + /// An existing SDK is present and there is a terminal — prompt before + /// displacing it as the active default. PromptOverwrite, /// An existing SDK is present but there is no terminal and no `--yes` — refuse. RefuseNonInteractive, } /// Decide whether an SDK install proceeds, prompts, or is refused. Fresh installs -/// (no `existing` runtime) always proceed; only overwriting an existing SDK needs -/// confirmation, and outside an interactive terminal that confirmation must come -/// from `--yes`. +/// (no `existing` runtime) always proceed; displacing an existing SDK as the +/// active default needs confirmation, and outside an interactive terminal that +/// confirmation must come from `--yes`. const fn sdk_install_approval( existing: bool, assume_yes: bool, @@ -1299,10 +1302,17 @@ const fn sdk_install_approval( } } -/// Interactive confirmation gate for overwriting an existing managed SDK. Prints +/// Interactive confirmation gate for displacing an existing managed SDK. Prints /// what would be replaced, then reads a yes/no answer from stdin. Only reached /// when an existing SDK is present, `--yes` was not passed, and a terminal is /// attached (see `sdk_install_approval`). +/// +/// "Displacing" rather than "overwriting" is deliberate: `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. fn confirm_overwrite_existing_sdk( channel: TheRockChannel, family: &str, @@ -1322,7 +1332,7 @@ fn confirm_overwrite_existing_sdk( // 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("Overwrite the existing ROCm SDK? [y/N]: ") + prompt_yes_no("Replace the existing ROCm SDK as the active default? [y/N]: ") } fn prompt_yes_no(prompt: &str) -> Result { @@ -1396,9 +1406,10 @@ fn install_tarball_runtime( )); } - // Fresh installs proceed with just an informational line; only overwriting an - // existing managed SDK asks for confirmation (and needs `--yes` when there is - // no terminal to answer the prompt). + // Fresh installs proceed with just an informational line. Only an install + // that displaces an existing managed SDK for this family/channel as the + // active default asks for confirmation (and needs `--yes` when there is no + // terminal to answer the prompt). let existing = existing_runtime_relation(paths, channel, &artifact.family, &artifact.version)?; match sdk_install_approval(existing.is_some(), assume_yes, interactive_terminal()) { SdkInstallApproval::ProceedFresh => { @@ -1410,7 +1421,7 @@ fn install_tarball_runtime( } SdkInstallApproval::ProceedApproved => { progress_line(format!( - "Overwriting existing ROCm SDK ({}) with ROCm {}.", + "Approved by --yes: an existing ROCm SDK was found ({}); installing ROCm {}, which becomes the active default runtime.", existing.as_deref().unwrap_or_default(), runtime_version_display(&artifact.version) )); @@ -1431,7 +1442,7 @@ fn install_tarball_runtime( } SdkInstallApproval::RefuseNonInteractive => { bail!( - "an existing ROCm SDK ({}) would be overwritten; re-run with --yes to overwrite it non-interactively, for example `rocm install sdk --yes`", + "an existing ROCm SDK was found ({}); continuing would make the newly installed ROCm the active default runtime. Re-run with --yes to approve this non-interactively, for example `rocm install sdk --yes`", existing.as_deref().unwrap_or_default() ); } diff --git a/apps/rocmd/src/lib.rs b/apps/rocmd/src/lib.rs index 1e6385285..624eb1e7f 100644 --- a/apps/rocmd/src/lib.rs +++ b/apps/rocmd/src/lib.rs @@ -2573,6 +2573,18 @@ 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 existing managed + // SDK for this family/channel would make the approval gate refuse with + // "re-run with `--yes`" — a flag no MCP caller of this tool can supply. + // 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("--yes".to_owned()); } Ok(argv) } @@ -5863,6 +5875,38 @@ mod tests { ); } + /// The `install_sdk` MCP tool spawns `rocm` with null stdin, so a real + /// install over an existing managed SDK 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 `--yes`; 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. + #[test] + fn install_sdk_real_install_args_carry_yes_but_dry_run_does_not() -> Result<()> { + let arguments = serde_json::Map::new(); + + let real = build_install_sdk_args(&arguments, false)?; + assert!( + real.contains(&"--yes".to_owned()), + "real install argv must carry --yes for the null-stdin spawn: {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(&"--yes".to_owned()), + "dry-run argv must not carry --yes: {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/docs/testing.md b/docs/testing.md index d6aa9fc9e..021af4100 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -156,9 +156,9 @@ The live SDK acceptance test creates an isolated test root under `target/`, crea rocm install sdk --channel release --format wheel --yes ``` -`--yes` approves overwriting an existing managed SDK without prompting, which -keeps the command non-interactive when the test root is reused across runs (a -fresh root never prompts). It matches the invocation in +`--yes` approves replacing an existing managed SDK as the active default without +prompting, which keeps the command non-interactive when the test root is reused +across runs (a fresh root never prompts). It matches the invocation in `scripts/therock_sdk_install_test.py`. Then it verifies: diff --git a/tests/e2e-cucumber/features/runtime_setup.feature b/tests/e2e-cucumber/features/runtime_setup.feature index 35ec8206c..9289e7386 100644 --- a/tests/e2e-cucumber/features/runtime_setup.feature +++ b/tests/e2e-cucumber/features/runtime_setup.feature @@ -132,9 +132,9 @@ Feature: Runtime configuration Then the adoption is refused And the error explains which install types can be adopted - # Reinstalling over an existing managed SDK must not silently clobber the + # Reinstalling over an existing managed SDK must not silently displace the # active runtime. Outside an interactive terminal (as every e2e invocation - # is here), `install sdk` without `--yes` must refuse rather than overwrite. + # is here), `install sdk` without `--yes` must refuse rather than proceed. # GPU-gated because the precondition needs a GPU to have a runtime active. # The refusal is not free: the gate keys on the resolved family and version, so # it runs after the Python launcher is resolved and the channel index is read. @@ -153,13 +153,13 @@ Feature: Runtime configuration # Companion to Scenario runtime-08: with --yes the same reinstall proceeds and the # runtime stays registered and active afterward. Nightly-gated in addition to # GPU because, unlike Scenario runtime-08, this exercises a real second SDK install. - # The registered/active Thens hold from the Given alone, so the overwrite Then + # 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-09 - Reinstalling the SDK over an existing runtime with --yes overwrites it + Scenario: runtime-09 - 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 overwriting the existing runtime + Then the install reports that --yes approved replacing the existing runtime And a runtime is registered And the runtime is set as active diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index 918050b94..6fb7c8158 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -474,18 +474,19 @@ async fn assert_runtime_active(world: &mut E2eWorld) { ); } -#[then("the install reports overwriting the existing runtime")] -async fn assert_install_overwrote_existing(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 overwrite from a no-op. This asserts the overwrite branch - // was actually taken: with `--yes` the approval gate resolves to - // `ProceedApproved`, whose only externally visible signal is this line. If - // `--yes` regressed to a refusal, or the install silently took the fresh - // path, this line is absent and the scenario fails. + // cannot tell an approved reinstall from a no-op. This asserts the approved + // branch was actually taken: with `--yes` the gate resolves to + // `ProceedApproved`, whose only externally visible signal is this line. The + // `Approved by --yes:` prefix is what discriminates — the fresh-install line + // ("No existing ROCm SDK found") 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("Overwriting existing ROCm SDK"), - "reinstall with --yes did not report overwriting the existing runtime:\n{output}" + output.contains("Approved by --yes: an existing ROCm SDK was found"), + "reinstall with --yes did not report the approved replacement:\n{output}" ); } From 3244d96287d32adf200390111e1b384de10c88f6 Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Thu, 10 Sep 2026 12:59:36 +0300 Subject: [PATCH 07/20] fix(install): say "replace as the active default" in the --yes help (EAI-7956) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `rocm install sdk --help` still described `--yes` as approving an overwrite, contradicting the prompt, the README and this crate's own gate docstring. `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 moves is the active default. Only a same-version reinstall reuses the same root. Reword the flag's help to match, and pin it with a test on the rendered `install sdk` long help so the next wording pass cannot miss this surface again. Behaviour is unchanged. Signed-off-by: Roman Sirokov --- apps/rocm/src/main.rs | 36 ++++++++++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 9bbbe74ad..54487a01b 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -602,10 +602,10 @@ rocm install sdk --family gfx110X-all --dry-run")] /// Resolve the install plan without changing files. #[arg(long)] dry_run: bool, - /// Approve overwriting an existing ROCm SDK (and required system-package - /// installs such as OpenMPI for vLLM) without prompting; required to - /// overwrite an existing SDK outside an interactive terminal. A fresh - /// install (no existing SDK) never prompts. + /// Approve replacing an existing ROCm SDK as the active default (and + /// required system-package installs such as OpenMPI for vLLM) without + /// prompting; required to replace an existing SDK outside an interactive + /// terminal. A fresh install (no existing SDK) never prompts. #[arg(long)] yes: bool, }, @@ -19343,6 +19343,34 @@ mod tests { } } + #[test] + fn install_sdk_help_describes_the_gate_as_replacing_the_active_default() { + // `rocm install sdk --help` is the most-read description of the `--yes` + // gate, and it is the one surface a "reword every site" pass can miss. + // The effect is a displacement, not a deletion: `runtime_key` embeds the + // resolved version, so an upgrade or downgrade lands in its own install + // root and the previous install stays on disk — only the active default + // moves. Claiming an overwrite here would promise a deletion that does + // not happen and contradict the prompt and the README. + let help = Cli::command() + .find_subcommand_mut("install") + .expect("install subcommand") + .find_subcommand_mut("sdk") + .expect("install sdk subcommand") + .render_long_help() + .to_string(); + assert!( + help.contains("active default"), + "`rocm install sdk --help` must describe --yes as approving a \ + replacement of the active default:\n{help}" + ); + assert!( + !help.to_lowercase().contains("overwrit"), + "`rocm install sdk --help` must not claim an overwrite; an upgrade \ + or downgrade leaves the previous install on disk:\n{help}" + ); + } + #[test] fn out_of_scope_commands_are_marked_preview_in_help() { let help = Cli::command().render_long_help().to_string(); From 04849a2202ca76810e262723936ab1f9e8807d7e Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Thu, 10 Sep 2026 15:23:45 +0300 Subject: [PATCH 08/20] fix(install): gate the SDK consent prompt on the active default runtime (EAI-7956) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The confirmation gate keyed on "a managed SDK exists for the same GPU family and channel", but activation is global: finalize_successful_sdk_install activates whatever finished installing last, regardless of family or channel. So `rocm install sdk --family ` (or `--channel nightly`) took over the active default without asking, printed "No existing ROCm SDK found" while an SDK plainly existed, and needed no --yes even in CI — exactly the event the prompt, the --yes help and the README all promise is confirmed first. Gate on what the runtime config's active_runtime_key / default_runtime_id actually resolves to. Any install that would displace the current active default now asks, whatever it is installing; an install with no active default runtime still never prompts, and re-installing or upgrading the runtime that is already the active default keeps the confirmation it has today. The user-facing strings follow: the fresh-install line no longer claims no SDK exists, and the prompt, the refusal, the --yes help, the README and docs/testing.md all say "active default" rather than "same family and channel". Separately, `rocm update --apply` passed assume_yes: true into that gate, so it printed "Approved by --yes: … which becomes the active default runtime" on essentially every apply over an existing runtime. Both halves were false: Update has no --yes flag for the user to have passed, and apply_runtime_update activates only under --activate. Replace the bool with a consent enum that records where the approval came from, and thread --activate through so the line states only what that path will actually do. The update path stays preapproved and never reaches a prompt. xtask's pre-warm `install sdk` gains --yes: it is now reachable with another channel's runtime active, and xtask has no terminal to answer a prompt with. Adds Gherkin coverage for the newly gated case (runtime-12: installing a different GPU family while a runtime is active is refused without --yes) and unit coverage for the relation scoping, the approval matrix, and the wording of every approved/fresh line. Signed-off-by: Roman Sirokov --- README.md | 30 +- apps/rocm/src/main.rs | 44 +- apps/rocm/src/therock.rs | 543 +++++++++++++----- .../rocm-dash-tui/src/ui/install_manager.rs | 11 +- crates/rocm-dash-tui/src/ui/onboarding.rs | 2 +- docs/testing.md | 9 +- .../features/runtime_setup.feature | 28 +- tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 71 ++- xtask/src/e2e_prewarm.rs | 7 +- 9 files changed, 564 insertions(+), 181 deletions(-) diff --git a/README.md b/README.md index 6d59e7027..d596cd6e1 100644 --- a/README.md +++ b/README.md @@ -199,10 +199,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. Re-running the command when it -already created a managed runtime for the same GPU family and channel asks -first, because the new install takes over as the active default; add `--yes` to -approve that non-interactively, such as from a script. +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 — that includes installing a different +GPU family or channel, which takes it over just the same. Add `--yes` to approve +that non-interactively, such as from a script. Then serve a model: @@ -261,15 +262,20 @@ rocm update [--apply] [--runtime KEY] [--activate] [--dry-run] ``` `install sdk` downloads TheRock ROCm wheels into a Python environment managed -by rocm-cli. A fresh install never prompts, but when a managed SDK already -exists for the same GPU family and channel it asks first, because the new -install becomes the active default in its place; pass `--yes` to approve that -non-interactively (for example in scripts or CI, where the prompt would -otherwise refuse). Because the install root and manifest are keyed by version, -an upgrade or downgrade keeps the previous install on disk — only a same-version -reinstall replaces it in place. `install driver` installs the AMD kernel driver on Linux +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. Pass `--yes` to approve that non-interactively (for example +in scripts or CI, where the prompt would otherwise refuse). Because the install +root and manifest are keyed by version, an upgrade or downgrade keeps the +previous install on disk — only a same-version reinstall reuses the same install +root. `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. +`--apply` to install it. `rocm update --apply` has no `--yes` flag and needs +none: selecting a runtime to update is itself the approval, and it leaves the +active default alone unless you add `--activate`. ### Runtime management diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index ed84a3f08..e8eba3405 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -604,10 +604,13 @@ rocm install sdk --family gfx110X-all --dry-run")] /// Resolve the install plan without changing files. #[arg(long)] dry_run: bool, - /// Approve replacing an existing ROCm SDK as the active default (and + /// Approve replacing the current active default ROCm runtime (and /// required system-package installs such as OpenMPI for vLLM) without - /// prompting; required to replace an existing SDK outside an interactive - /// terminal. A fresh install (no existing SDK) never prompts. + /// 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, }, @@ -9340,7 +9343,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> { @@ -11693,7 +11696,7 @@ 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 an overwrite prompt would + // `interactive_terminal()` is false and the consent prompt would // refuse with "re-run with `--yes`" — a flag the user cannot supply // through chat. Add it here, matching the `install driver` and // `services stop/restart` arms below. @@ -13514,7 +13517,7 @@ fn rocm_chat_tool_requested_args(call: &providers::ChatToolCall) -> Option Result { let channel = TheRockChannel::parse(channel)?; ensure_install_format_supported(format)?; + let consent = if assume_yes { + SdkInstallConsent::Preapproved(SdkInstallApprovalSource::AssumeYes) + } else { + SdkInstallConsent::Ask + }; match format { "wheel" => install_wheel_runtime( paths, @@ -767,13 +772,13 @@ pub(crate) fn install_sdk( None, version_selector.as_ref(), dry_run, - assume_yes, + consent, ), "tarball" => { if version_selector.is_some() { bail!("specific TheRock version selection is only supported for wheel installs") } - install_tarball_runtime(paths, channel, prefix, family_override, dry_run, assume_yes) + install_tarball_runtime(paths, channel, prefix, family_override, dry_run, consent) } other => bail!("unsupported install format: {other}"), } @@ -781,10 +786,15 @@ pub(crate) fn install_sdk( /// Apply an update using the exact family and device payload resolved by its plan. /// -/// `assume_yes` is passed through to the same approval gate `install_sdk` uses. -/// Callers pass `true` unconditionally: an update targets the family and channel -/// of the runtime it was resolved from, so displacing that runtime as the active -/// default is the operation the user asked for, not a side effect of it. +/// 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 `--yes` flag and no terminal +/// contract — reaching a prompt here would only fail the command. +/// +/// `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, @@ -793,10 +803,13 @@ pub(crate) fn install_sdk_for_update( family: &str, device_target: Option<&str>, dry_run: bool, - assume_yes: bool, + 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, + }); match format { "wheel" => install_wheel_runtime( paths, @@ -806,11 +819,9 @@ pub(crate) fn install_sdk_for_update( device_target, None, dry_run, - assume_yes, + consent, ), - "tarball" => { - install_tarball_runtime(paths, channel, None, Some(family), dry_run, assume_yes) - } + "tarball" => install_tarball_runtime(paths, channel, None, Some(family), dry_run, consent), other => bail!("unsupported install format: {other}"), } } @@ -1258,7 +1269,7 @@ fn install_wheel_runtime( device_target_override: Option<&str>, version_selector: Option<&RuntimeVersionSelector>, dry_run: bool, - assume_yes: bool, + consent: SdkInstallConsent, ) -> Result { progress_line(format!( "Checking Python for the ROCm install; if needed, ROCm CLI will prepare Python {}.", @@ -1457,29 +1468,28 @@ fn install_wheel_runtime( )); } - // Fresh installs proceed with just an informational line. Only an install - // that displaces an existing managed SDK for this family/channel as the - // active default asks for confirmation (and needs `--yes` when there is no - // terminal to answer the prompt). - let existing = existing_runtime_relation( + // Installs with no active default runtime proceed with just an informational + // line. Any install that would displace the current active default — whatever + // its family or channel, because activation is global — asks for confirmation + // (and needs `--yes` when there is no terminal to answer the prompt). + let existing = active_default_runtime_relation( paths, channel, &resolution.family, &resolution.latest_version, )?; - match sdk_install_approval(existing.is_some(), assume_yes, interactive_terminal()) { + match sdk_install_approval(existing.is_some(), consent, interactive_terminal()) { SdkInstallApproval::ProceedFresh => { - progress_line(format!( - "No existing ROCm SDK found; installing ROCm SDK {} for family {}.", - runtime_version_display(&resolution.latest_version), - resolution.family + progress_line(fresh_install_line( + &runtime_version_display(&resolution.latest_version), + &resolution.family, )); } - SdkInstallApproval::ProceedApproved => { - progress_line(format!( - "Approved by --yes: an existing ROCm SDK was found ({}); installing ROCm {}, which becomes the active default runtime.", + SdkInstallApproval::ProceedApproved(source) => { + progress_line(preapproved_install_line( + source, existing.as_deref().unwrap_or_default(), - runtime_version_display(&resolution.latest_version) + &runtime_version_display(&resolution.latest_version), )); } SdkInstallApproval::PromptOverwrite => { @@ -1497,10 +1507,9 @@ fn install_wheel_runtime( } } SdkInstallApproval::RefuseNonInteractive => { - bail!( - "an existing ROCm SDK was found ({}); continuing would make the newly installed ROCm the active default runtime. Re-run with --yes to approve this non-interactively, for example `rocm install sdk --yes`", + bail!(refuse_non_interactive_message( existing.as_deref().unwrap_or_default() - ); + )); } } @@ -1683,42 +1692,78 @@ fn quote_display_arg(value: &str) -> String { } } -/// Classify how the version rocm-cli is about to install relates to the managed -/// runtime it would displace as the active default — the newest existing install -/// for the same GPU family and channel. Returns `None` when no such runtime -/// exists (a fresh install for this family/channel). +/// 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 no active default resolves — a genuinely fresh install, where the new +/// runtime takes a slot nothing occupies and there is nothing to consent to. /// -/// Errors reading the manifest directory are propagated rather than treated as -/// "no existing runtime": silently falling back to a fresh-install verdict on a -/// read error would skip the overwrite confirmation gate precisely when we are -/// least sure whether an existing SDK is present. -fn existing_runtime_relation( +/// 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. +fn active_default_runtime_relation( paths: &AppPaths, channel: TheRockChannel, family: &str, resolved_version: &str, ) -> Result> { let manifests = load_runtime_manifests(paths)?; - let mut matching = manifests - .into_iter() - .filter(|manifest| manifest.family == family && manifest.channel == channel.as_str()) - .collect::>(); - matching.sort_by_key(|manifest| std::cmp::Reverse(manifest.installed_at_unix_ms)); - let Some(existing) = matching.into_iter().next() else { + let config = RocmCliConfig::load(paths)?; + let Some(active) = crate::current_runtime_manifest(&config, &manifests) else { return Ok(None); }; - let relation = match compare_version_strings(resolved_version, &existing.version) { - Ordering::Greater => "upgrade", - Ordering::Less => "downgrade", - Ordering::Equal => "reinstall", - }; - Ok(Some(format!( - "{relation} from installed {installed} ({key})", - installed = runtime_version_display(&existing.version), - key = existing.runtime_key + Ok(Some(active_default_relation_text( + active, + channel, + family, + resolved_version, ))) } +/// 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. @@ -1784,45 +1829,116 @@ fn repo_version_without_wheels( } } +/// 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` has no `--yes` flag, +/// so a message crediting one would name a flag the user could not have passed. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum SdkInstallApprovalSource { + /// The user passed `--yes` to `rocm install sdk`. + AssumeYes, + /// `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 existing managed SDK for this family+channel — install without asking. + /// No active default runtime — nothing is displaced, so install without asking. ProceedFresh, - /// An existing SDK is present and `--yes` was given — displace it as the - /// active default without asking. - ProceedApproved, - /// An existing SDK is present and there is a terminal — prompt before - /// displacing it as the active default. + /// 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 existing SDK is present but there is no terminal and no `--yes` — refuse. + /// 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. Fresh installs -/// (no `existing` runtime) always proceed; displacing an existing SDK as the -/// active default needs confirmation, and outside an interactive terminal that -/// confirmation must come from `--yes`. +/// 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, - assume_yes: bool, + consent: SdkInstallConsent, interactive: bool, ) -> SdkInstallApproval { - if !existing { - SdkInstallApproval::ProceedFresh - } else if assume_yes { - SdkInstallApproval::ProceedApproved - } else if interactive { - SdkInstallApproval::PromptOverwrite - } else { - SdkInstallApproval::RefuseNonInteractive + 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 name a flag `rocm update` does not +/// have. +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::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." + ), } } -/// Interactive confirmation gate for displacing an existing managed SDK. Prints -/// what would be replaced, then reads a yes/no answer from stdin. Only reached -/// when an existing SDK is present, `--yes` was not passed, and a terminal is -/// attached (see `sdk_install_approval`). +/// 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. +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 --yes to approve this non-interactively, for example `rocm install sdk --yes`" + ) +} + +/// 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: `runtime_key` embeds the /// resolved version, so an upgrade or downgrade lands in its own install root @@ -1836,8 +1952,8 @@ fn confirm_overwrite_existing_sdk( resolved_version: &str, relation: &str, ) -> Result { - println!("sdk install: an existing ROCm SDK was found"); - println!(" existing runtime: {relation}"); + 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), @@ -1871,7 +1987,7 @@ fn install_tarball_runtime( prefix: Option, family_override: Option<&str>, dry_run: bool, - assume_yes: bool, + consent: SdkInstallConsent, ) -> Result { let artifact = resolve_tarball_artifact(paths, channel, family_override)?; let runtime_key = runtime_key( @@ -1930,24 +2046,23 @@ fn install_tarball_runtime( )); } - // Fresh installs proceed with just an informational line. Only an install - // that displaces an existing managed SDK for this family/channel as the - // active default asks for confirmation (and needs `--yes` when there is no - // terminal to answer the prompt). - let existing = existing_runtime_relation(paths, channel, &artifact.family, &artifact.version)?; - match sdk_install_approval(existing.is_some(), assume_yes, interactive_terminal()) { + // 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(format!( - "No existing ROCm SDK found; installing ROCm SDK {} for family {}.", - runtime_version_display(&artifact.version), - artifact.family + progress_line(fresh_install_line( + &runtime_version_display(&artifact.version), + &artifact.family, )); } - SdkInstallApproval::ProceedApproved => { - progress_line(format!( - "Approved by --yes: an existing ROCm SDK was found ({}); installing ROCm {}, which becomes the active default runtime.", + SdkInstallApproval::ProceedApproved(source) => { + progress_line(preapproved_install_line( + source, existing.as_deref().unwrap_or_default(), - runtime_version_display(&artifact.version) + &runtime_version_display(&artifact.version), )); } SdkInstallApproval::PromptOverwrite => { @@ -1965,10 +2080,9 @@ fn install_tarball_runtime( } } SdkInstallApproval::RefuseNonInteractive => { - bail!( - "an existing ROCm SDK was found ({}); continuing would make the newly installed ROCm the active default runtime. Re-run with --yes to approve this non-interactively, for example `rocm install sdk --yes`", + bail!(refuse_non_interactive_message( existing.as_deref().unwrap_or_default() - ); + )); } } @@ -7224,41 +7338,102 @@ echo Python 3.12.10 assert!(error.contains("rocm install sdk --format wheel")); } + /// 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 existing_runtime_relation_classifies_upgrade_downgrade_reinstall() -> Result<()> { - let (root, paths) = test_paths("existing-runtime-relation"); + 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_test_runtime_manifest(&paths, &manifest)?; + write_active_test_runtime(&paths, &manifest)?; - let upgrade = - existing_runtime_relation(&paths, TheRockChannel::Release, "gfx120X-all", "7.14.0")? - .expect("relation should be reported for a matching runtime"); + 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 = - existing_runtime_relation(&paths, TheRockChannel::Release, "gfx120X-all", "7.12.0")? - .expect("relation should be reported for a matching runtime"); + 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 = - existing_runtime_relation(&paths, TheRockChannel::Release, "gfx120X-all", "7.13.0")? - .expect("relation should be reported for a matching runtime"); + 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}"); - // A different family or channel is a fresh install, so no relation applies. + 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!( - existing_runtime_relation(&paths, TheRockChannel::Release, "gfx110X-all", "7.14.0")? - .is_none() + 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!( - existing_runtime_relation(&paths, TheRockChannel::Nightly, "gfx120X-all", "7.14.0")? - .is_none() + other_channel.contains("replaces active default") + && other_channel.contains("release channel"), + "got: {other_channel}" ); let _ = fs::remove_dir_all(root); @@ -7266,30 +7441,58 @@ echo Python 3.12.10 } #[test] - fn existing_runtime_relation_none_without_managed_runtimes() -> Result<()> { - let (root, paths) = test_paths("existing-runtime-relation-empty"); + 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!( - existing_runtime_relation(&paths, TheRockChannel::Release, "gfx120X-all", "7.14.0")? - .is_none() + active_default_runtime_relation( + &paths, + TheRockChannel::Release, + "gfx120X-all", + "7.14.0" + )? + .is_none() ); + let _ = fs::remove_dir_all(root); Ok(()) } #[test] - fn existing_runtime_relation_propagates_manifest_read_errors() -> Result<()> { + 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 existing runtime" — that would skip the - // overwrite confirmation gate exactly when we're least sure whether an - // existing SDK is present. - let (root, paths) = test_paths("existing-runtime-relation-error"); + // 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 = - existing_runtime_relation(&paths, TheRockChannel::Release, "gfx120X-all", "7.14.0") - .expect_err("a manifest read failure should be propagated, not swallowed"); + 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); @@ -7297,36 +7500,104 @@ echo Python 3.12.10 } #[test] - fn sdk_install_approval_only_prompts_when_overwriting_existing() { - // Fresh install: no existing SDK -> never prompt, regardless of terminal/--yes. + 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, false, false), + sdk_install_approval(false, SdkInstallConsent::Ask, false), SdkInstallApproval::ProceedFresh ); assert_eq!( - sdk_install_approval(false, false, true), + sdk_install_approval(false, SdkInstallConsent::Ask, true), SdkInstallApproval::ProceedFresh ); assert_eq!( - sdk_install_approval(false, true, false), + sdk_install_approval(false, assume_yes, false), SdkInstallApproval::ProceedFresh ); - // Existing SDK: --yes overwrites silently; a terminal prompts; neither refuses. + // Active default present: preapproved consent proceeds and is credited to + // its real source; a terminal prompts; neither refuses. assert_eq!( - sdk_install_approval(true, true, false), - SdkInstallApproval::ProceedApproved + sdk_install_approval(true, assume_yes, false), + SdkInstallApproval::ProceedApproved(SdkInstallApprovalSource::AssumeYes) + ); + assert_eq!( + sdk_install_approval(true, update_apply, false), + SdkInstallApproval::ProceedApproved(SdkInstallApprovalSource::UpdateApply { + activates: false + }) ); assert_eq!( - sdk_install_approval(true, false, true), + sdk_install_approval(true, SdkInstallConsent::Ask, true), SdkInstallApproval::PromptOverwrite ); assert_eq!( - sdk_install_approval(true, false, false), + sdk_install_approval(true, SdkInstallConsent::Ask, false), SdkInstallApproval::RefuseNonInteractive ); } + #[test] + fn preapproved_install_line_credits_the_real_consent_source() { + // `rocm update --apply` has no `--yes` flag, so a line crediting one + // names something the user could not have passed. 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")); + + 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 a flag `rocm update` does not have: {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 a flag `rocm update` does not have: {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 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. diff --git a/crates/rocm-dash-tui/src/ui/install_manager.rs b/crates/rocm-dash-tui/src/ui/install_manager.rs index 32547d7a0..fe2383760 100644 --- a/crates/rocm-dash-tui/src/ui/install_manager.rs +++ b/crates/rocm-dash-tui/src/ui/install_manager.rs @@ -154,10 +154,11 @@ 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 overwrite an existing SDK cannot answer the confirmation - // prompt and would refuse. Pass `--yes` so the dashboard install - // proceeds; the dry-run preview never mutates, so it needs no flag. + // 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. Pass `--yes` so the + // dashboard install proceeds; the dry-run preview never mutates, so + // it needs no flag. args.push("--yes".to_string()); } Ok(args) @@ -485,7 +486,7 @@ mod tests { 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 --yes so the null-stdin - // dashboard spawn is not refused at the overwrite prompt. + // dashboard spawn is not refused at the consent prompt. assert!(args.contains(&"--yes".to_string())); } diff --git a/crates/rocm-dash-tui/src/ui/onboarding.rs b/crates/rocm-dash-tui/src/ui/onboarding.rs index b9d59fc8e..0f7ca7b43 100644 --- a/crates/rocm-dash-tui/src/ui/onboarding.rs +++ b/crates/rocm-dash-tui/src/ui/onboarding.rs @@ -191,7 +191,7 @@ fn build_install_args(cfg: &InstallConfig) -> Vec { "--format".to_string(), "wheel".to_string(), // Onboarding installs are spawned with null stdin, so a would-be - // overwrite prompt cannot be answered and the install would refuse. + // consent prompt cannot be answered and the install would refuse. // `--yes` keeps the first-run install non-interactive. "--yes".to_string(), ]; diff --git a/docs/testing.md b/docs/testing.md index bed8f39dc..1baacb3f7 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -156,9 +156,12 @@ The live SDK acceptance test creates an isolated test root under `target/`, crea rocm install sdk --channel release --format wheel --yes ``` -`--yes` approves replacing an existing managed SDK as the active default without -prompting, which keeps the command non-interactive when the test root is reused -across runs (a fresh root never prompts). It matches the invocation in +`--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`. Then it verifies: diff --git a/tests/e2e-cucumber/features/runtime_setup.feature b/tests/e2e-cucumber/features/runtime_setup.feature index 82890899f..7015ea07a 100644 --- a/tests/e2e-cucumber/features/runtime_setup.feature +++ b/tests/e2e-cucumber/features/runtime_setup.feature @@ -176,11 +176,11 @@ Feature: Runtime configuration Then the SDK preview reports canonical release provenance And the SDK preview requests the device payload for this host's GPU - # Reinstalling over an existing managed SDK must not silently displace the - # active runtime. Outside an interactive terminal (as every e2e invocation + # Installing over the active default managed runtime must not silently + # displace it. Outside an interactive terminal (as every e2e invocation # is here), `install sdk` without `--yes` must refuse rather than proceed. # GPU-gated because the precondition needs a GPU to have a runtime active. - # The refusal is not free: the gate keys on the resolved family and version, so + # 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 @@ -207,3 +207,25 @@ Feature: Runtime configuration 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-10 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-10 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 also + # mention `--yes` in the usage text): only the real gate names the runtime it + # would replace. + @id:runtime-install-sdk-other-family-requires-yes @requires-gpu + Scenario: runtime-12 - Installing a different GPU family while a runtime is active is refused without --yes + 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 that --yes is required + And the error names the active default runtime it would replace diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index 90a3b0a4d..3ca17a012 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -155,11 +155,11 @@ async fn setup_runtime_with_engine(world: &mut E2eWorld) { if stdout.contains("installed: none") { // `--yes` for the same reason the sibling `a managed runtime is active` // passes it: the harness spawns `rocm` with null stdin, so anything the - // overwrite gate does not read as a fresh install refuses rather than - // prompts. The `installed: none` guard makes this a fresh install today, - // so the flag changes nothing — it keeps the step correct if that guard - // ever loosens, instead of failing the lane at a prompt nothing can - // answer. + // 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); @@ -213,7 +213,7 @@ async fn user_installs_sdk(world: &mut E2eWorld) { // 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. `--yes` keeps the install - // non-interactive-safe: the e2e harness runs with null stdin, so an overwrite + // 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); @@ -239,6 +239,33 @@ async fn user_reinstalls_sdk_with_yes(world: &mut E2eWorld) { 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". +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-10. + // The runtime key carries the family, so the registry listing is enough. + 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}") + }); + let (stdout, stderr, rc) = + crate::run_rocm(world, &["install", "sdk", "--family", family]); + 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; @@ -708,13 +735,14 @@ 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`, whose only externally visible signal is this line. The - // `Approved by --yes:` prefix is what discriminates — the fresh-install line - // ("No existing ROCm SDK found") does not carry it, so if `--yes` regressed - // to a refusal, or the install silently took the fresh path, this fails. + // `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 was found"), + 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}" ); } @@ -882,3 +910,24 @@ async fn assert_reinstall_error_explains_yes(world: &mut E2eWorld) { "error does not mention --yes:\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}" + ); +} diff --git a/xtask/src/e2e_prewarm.rs b/xtask/src/e2e_prewarm.rs index c527476b2..652286965 100644 --- a/xtask/src/e2e_prewarm.rs +++ b/xtask/src/e2e_prewarm.rs @@ -480,8 +480,13 @@ pub fn run(channel: &str, keep: usize, prewarm_dir: &Path) -> Result<()> { "pre-warm: installing the {channel} SDK into {}", prewarm_dir.display() ); + // `--yes` because the consent gate keys on the tree's active default + // runtime, not on the channel `decide()` inspected: a shared tree + // pre-warmed for `release` and then pre-warmed for `nightly` reaches + // here with a release runtime already active, and xtask has no + // terminal to answer a prompt with, so the install would refuse. 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 } => { From ee8fac3f3ecfe51f51d5e485f3c1b369a7fba1ec Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Thu, 10 Sep 2026 15:57:29 +0300 Subject: [PATCH 09/20] style: rustfmt the active-default gate test and step Signed-off-by: Roman Sirokov --- apps/rocm/src/therock.rs | 3 ++- tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 3 +-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index d8b09fa2c..ebe12bb3d 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -7419,7 +7419,8 @@ echo Python 3.12.10 )? .expect("installing another family must still report the active default it displaces"); assert!( - other_family.contains("replaces active default") && other_family.contains("gfx110X-all"), + other_family.contains("replaces active default") + && other_family.contains("gfx110X-all"), "got: {other_family}" ); diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index 3ca17a012..e24a13afd 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -259,8 +259,7 @@ async fn user_installs_other_family_without_yes(world: &mut E2eWorld) { .unwrap_or_else(|| { panic!("no candidate family differs from the installed runtimes:\n{runtimes}") }); - let (stdout, stderr, rc) = - crate::run_rocm(world, &["install", "sdk", "--family", family]); + let (stdout, stderr, rc) = crate::run_rocm(world, &["install", "sdk", "--family", family]); world.cli_output = Some(stdout); world.cli_stderr = Some(stderr); world.cli_rc = Some(rc); From 76c6aa3c5a19f18271e0c28d7983d675c18a673b Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Fri, 11 Sep 2026 19:33:53 +0300 Subject: [PATCH 10/20] fix(install): stop injected --yes from approving sudo package installs (EAI-7956) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--yes` on `rocm install sdk` carries two unrelated consents: approve replacing the active default ROCm runtime, and approve installing required system packages, which means running `sudo`. The non-interactive surfaces added in this branch needed only the first and injected `--yes`, so they silently granted the second on exactly the spawns that have no terminal. On Linux, for a vLLM-preferred GPU family, with OpenMPI absent and neither root nor passwordless sudo, `ensure_openmpi_for_vllm` used to take its early return, print the manual commands with a warning, and let the vLLM engine auto-install continue. With the injected approval it instead falls through to `run_system_package_install_plan`, which runs `sudo` against the null stdin of the chat, MCP, dashboard and daemon spawns; the failure is then escalated to an error because the caller "asked for" the install, and the `?` in `maybe_auto_install_sdk_preferred_engine` skips the engine install entirely. (`rocm install sdk` still exits 0 — `engine_auto_install_failure_is_fatal` only catches `UnusableRuntimeAfterInstall` — but the engine the family wanted is no longer installed, and on a surface that owns a terminal the sudo prompt reaches `/dev/tty` with a TUI in raw mode in front of it.) Separate the two consents at the argv boundary rather than overloading one bool. `--approve-replacing-active-default` grants only the runtime displacement; `--yes` still grants both, unchanged, including escalating a failed package install it explicitly approved. The chat arm, the `apps/rocm` MCP tool-args builder, the `apps/rocmd` `install_sdk` builder, the dashboard install manager and the onboarding wizard now pass the narrow flag. `scripts/therock_sdk_install_test.py` and `xtask` keep `--yes`: both are provisioning harnesses meant to install the system packages too, and both run as root in CI or from a developer's terminal. `SdkInstallConsents` names the two consents so the resolution is one testable place, and `system_package_install_action` extracts the shared decision from `ensure_openmpi_for_vllm` and `ensure_torch_runtime_dep` so it can be exercised without a package manager. Also from review, all local and independent of the above: - reword the `host_version_newer_than` comment, which called `7.2.4-98` and `7.4` unparseable when `parse_host_version` normalises both; - probe `detect_legacy_rocm_summary` once per install path instead of twice; - pass `--yes` in the SDK install harness only on the real-install path, since a dry run returns before the consent gate. Tests, each falsified: - `an_injected_consent_does_not_approve_privileged_package_installs` parses the argv a non-interactive surface really produces and asserts what it consents to. Red when `resolve` folds the narrow flag into `system_packages`. - `install_sdk_real_install_args_approve_only_the_runtime_replacement` (rocmd). Red when the builder goes back to `--yes`. - `install_sdk_help_separates_the_two_consents_yes_carries`. Red when the help drops the distinction. - Scenario `runtime-14` covers the same help text through the built binary, on the mock lane. Signed-off-by: Roman Sirokov --- README.md | 10 +- apps/rocm/src/main.rs | 295 +++++++++++++++--- apps/rocm/src/therock.rs | 30 +- apps/rocmd/src/lib.rs | 47 ++- .../rocm-dash-tui/src/ui/install_manager.rs | 15 +- crates/rocm-dash-tui/src/ui/onboarding.rs | 16 +- scripts/therock_sdk_install_test.py | 10 +- .../features/runtime_setup.feature | 15 + tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 22 ++ 9 files changed, 371 insertions(+), 89 deletions(-) diff --git a/README.md b/README.md index 049d47c83..5fce344fb 100644 --- a/README.md +++ b/README.md @@ -254,7 +254,8 @@ the JSON report, not the human-readable one. ``` rocm install sdk [--channel release|nightly] [--format wheel|tarball] [--version x.y.z | --build-date YYYY-MM-DD] - [--family gfx110X-all] [--prefix PATH] [--yes] [--dry-run] + [--family gfx110X-all] [--prefix PATH] [--dry-run] + [--yes | --approve-replacing-active-default] rocm install driver [--dkms] [--yes] [--dry-run] [--reconcile] @@ -268,7 +269,12 @@ 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. Pass `--yes` to approve that non-interactively (for example -in scripts or CI, where the prompt would otherwise refuse). Because the install +in scripts or CI, where the prompt would otherwise refuse). `--yes` also approves +installing required system packages (such as OpenMPI for vLLM), which means +`sudo`; if you want only the first approval — because nothing can answer a sudo +password prompt where your command runs — pass +`--approve-replacing-active-default` instead. That is what ROCm CLI's own +non-interactive surfaces (chat, MCP, the dashboard) pass. Because the install root and manifest are keyed by version, an upgrade or downgrade keeps the previous install on disk — only a same-version reinstall reuses the same install root. `install driver` installs the AMD kernel driver on Linux diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index f17604c0a..675a0665b 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -619,6 +619,14 @@ rocm install sdk --family gfx110X-all --dry-run")] /// 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 { @@ -2553,7 +2561,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", @@ -2574,7 +2584,7 @@ fn install(target: InstallTarget) -> Result<()> { version_selector, family.as_deref(), dry_run, - yes, + consents.replace_active_default, ) { Ok(result) => { let therock::SdkInstallResult { output, mutated } = result; @@ -2591,8 +2601,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, @@ -2617,7 +2627,11 @@ fn install(target: InstallTarget) -> Result<()> { ) }, |paths, finalized| { - maybe_auto_install_sdk_preferred_engine(paths, finalized, yes) + maybe_auto_install_sdk_preferred_engine( + paths, + finalized, + consents.system_packages, + ) }, )?; } @@ -7359,6 +7373,80 @@ 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. + replace_active_default: bool, + /// 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 { + Self { + replace_active_default: yes || approve_replacing_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. @@ -7409,7 +7497,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}"); } @@ -7418,16 +7510,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 { @@ -7445,7 +7530,7 @@ 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 { + 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", )); @@ -7556,7 +7641,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}"); } @@ -7568,16 +7657,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)() { @@ -11841,9 +11923,15 @@ fn chat_rocm_command_action_from_args(mut args: Vec) -> Result Option 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_carry_yes_for_non_interactive_spawn() { + 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 `--yes`" — 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 `--yes` so a reinstall - // over an existing SDK is not silently refused. + // 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(), @@ -23128,27 +23267,85 @@ model recipes "/home/tester/rocm-managed".to_owned(), ]) .expect("install sdk classifies"); - match action { - ChatRocmCommandAction::Approval { args, .. } => assert!( - args.iter().any(|arg| arg == "--yes"), - "chat `install sdk` approval must carry --yes, got {args:?}" - ), + 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 `--yes` independently. + // 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!( + consents_for_install_sdk_argv(args).replace_active_default, + "the spawn must approve replacing the active default, 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 args = rocm_chat_tool_requested_args(&call).expect("install_sdk tool builds args"); + let injected = consents_for_install_sdk_argv( + &rocm_chat_tool_requested_args(&call).expect("install_sdk tool builds args"), + ); + assert!(injected.replace_active_default); assert!( - args.iter().any(|arg| arg == "--yes"), - "MCP install_sdk args must carry --yes, got {args:?}" + !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!(typed.replace_active_default && 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, + }, ); } diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 05b219fb3..872c20773 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -1672,12 +1672,16 @@ fn install_wheel_runtime( " latest_compatible_version: {}", runtime_version_display(&resolution.latest_version) ); - if let Some(host_version) = host_rocm_version_newer_than(&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, + host_version, &runtime_version_display(&resolution.latest_version) ) ); @@ -1769,7 +1773,7 @@ fn install_wheel_runtime( // 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_rocm_version_newer_than(&resolution.latest_version) { + 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) @@ -2087,11 +2091,13 @@ fn host_rocm_version_newer_than(resolved_version: &str) -> Option { 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. An unparseable host string — an odd build suffix - // (`7.2.4-98`) or a truncated two-component report (`7.4`) — is "can't - // tell", not "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. + // 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) @@ -2337,11 +2343,13 @@ fn install_tarball_runtime( " latest_version: {}", runtime_version_display(&artifact.version) ); - if let Some(host_version) = host_rocm_version_newer_than(&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)) + tarball_host_version_note(host_version, &runtime_version_display(&artifact.version)) ); } let _ = writeln!(output, " target: {}", install_root.display()); @@ -2356,7 +2364,7 @@ fn install_tarball_runtime( // 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_rocm_version_newer_than(&artifact.version) { + 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) diff --git a/apps/rocmd/src/lib.rs b/apps/rocmd/src/lib.rs index 9e15518e1..0ce25a856 100644 --- a/apps/rocmd/src/lib.rs +++ b/apps/rocmd/src/lib.rs @@ -2589,16 +2589,25 @@ fn build_install_sdk_args( 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 existing managed - // SDK for this family/channel would make the approval gate refuse with - // "re-run with `--yes`" — a flag no MCP caller of this tool can supply. + // `interactive_terminal()` is false in the child and an active default + // managed runtime would make the approval gate refuse with "re-run with + // `--yes`" — 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("--yes".to_owned()); + argv.push("--approve-replacing-active-default".to_owned()); } Ok(argv) } @@ -5894,19 +5903,28 @@ mod tests { } /// The `install_sdk` MCP tool spawns `rocm` with null stdin, so a real - /// install over an existing managed SDK 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 `--yes`; 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. + /// 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_carry_yes_but_dry_run_does_not() -> Result<()> { + 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(&"--yes".to_owned()), - "real install argv must carry --yes for the null-stdin spawn: {real:?}" + 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()), @@ -5915,8 +5933,9 @@ mod tests { let dry = build_install_sdk_args(&arguments, true)?; assert!( - !dry.contains(&"--yes".to_owned()), - "dry-run argv must not carry --yes: {dry:?}" + !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()), diff --git a/crates/rocm-dash-tui/src/ui/install_manager.rs b/crates/rocm-dash-tui/src/ui/install_manager.rs index fe2383760..d0c4d80ce 100644 --- a/crates/rocm-dash-tui/src/ui/install_manager.rs +++ b/crates/rocm-dash-tui/src/ui/install_manager.rs @@ -156,10 +156,15 @@ impl InstallManagerState { } 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. Pass `--yes` so the - // dashboard install proceeds; the dry-run preview never mutates, so - // it needs no flag. - args.push("--yes".to_string()); + // 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) } @@ -487,7 +492,7 @@ mod tests { assert!(!args.contains(&"--dry-run".to_string())); // A real (non-dry-run) install must carry --yes so the null-stdin // dashboard spawn is not refused at the consent prompt. - assert!(args.contains(&"--yes".to_string())); + assert!(args.contains(&"--approve-replacing-active-default".to_string())); } #[test] diff --git a/crates/rocm-dash-tui/src/ui/onboarding.rs b/crates/rocm-dash-tui/src/ui/onboarding.rs index 0f7ca7b43..a1b366059 100644 --- a/crates/rocm-dash-tui/src/ui/onboarding.rs +++ b/crates/rocm-dash-tui/src/ui/onboarding.rs @@ -191,9 +191,11 @@ fn build_install_args(cfg: &InstallConfig) -> Vec { "--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. - // `--yes` keeps the first-run install non-interactive. - "--yes".to_string(), + // 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()) { @@ -676,7 +678,7 @@ mod tests { "release", "--format", "wheel", - "--yes" + "--approve-replacing-active-default" ], "default Release path must stay byte-identical to the pre-toggle args" ); @@ -924,7 +926,7 @@ mod tests { "nightly", "--format", "wheel", - "--yes" + "--approve-replacing-active-default" ] ); } @@ -954,7 +956,7 @@ mod tests { "nightly", "--format", "wheel", - "--yes", + "--approve-replacing-active-default", "--build-date", "2026-06-05" ] @@ -991,7 +993,7 @@ mod tests { "release", "--format", "wheel", - "--yes" + "--approve-replacing-active-default" ], "an empty pin must not add a flag" ); diff --git a/scripts/therock_sdk_install_test.py b/scripts/therock_sdk_install_test.py index 06c95ab45..534fc0c1e 100644 --- a/scripts/therock_sdk_install_test.py +++ b/scripts/therock_sdk_install_test.py @@ -476,12 +476,20 @@ def main() -> int: args.channel, "--format", "wheel", - "--yes", ] if args.prefix is not None: 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 runs either as + # root in CI or from a developer's terminal. + 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 cdaec2eae..3ff422aef 100644 --- a/tests/e2e-cucumber/features/runtime_setup.feature +++ b/tests/e2e-cucumber/features/runtime_setup.feature @@ -238,3 +238,18 @@ Feature: Runtime configuration Then the reinstall is refused And the error explains that --yes is required 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 diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index f470f69d7..e9df5b11e 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -945,3 +945,25 @@ 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}" + ); +} From d6435b401f3b8612a02f6464c054bfe4d8231e0b Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Fri, 11 Sep 2026 20:41:05 +0300 Subject: [PATCH 11/20] fix(install): credit the consent flag that was actually passed (EAI-7956) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Splitting `--yes` into a narrow `--approve-replacing-active-default` left three user-facing strings still crediting or recommending `--yes` on paths that never pass it. `install_sdk` took a bare `assume_yes: bool`, so every install from a terminal-less surface — chat, MCP, `rocmd`, the dashboard, onboarding — printed "Approved by --yes", telling whoever read the transcript that consent to run `sudo` had been given when it had not. Carry the approval source through instead: `SdkInstallApprovalSource` gains an `ApproveReplacingActiveDefault` variant, `SdkInstallConsents` records which flag granted the replacement rather than flattening it to a bool, and `install_sdk` takes the `SdkInstallConsent` itself. The non-interactive refusal — the one message a script or CI job sees when it hits this gate — told the caller to re-run with `--yes`, which is the opposite of the advice the split exists to give: it would hand an unattended caller approval to install system packages with `sudo` and park it on a password prompt it cannot answer. It now names `--approve-replacing-active-default` first and still explains when `--yes` is the right choice. The e2e step and the two scenarios that assert on it follow, and the README leads with the same recommendation. Also: the dashboard `build_args` test comment still described the `--yes` it no longer asserts, and now also pins that `--yes` is absent; the pre-warm harness comment justified its `--yes` with the rationale for the narrow flag rather than the real one (it wants the system packages, and its runners have passwordless sudo); and the OpenMPI escalation site now says that `finish_sdk_install` downgrades that error via `engine_auto_install_failure_is_fatal`, so `install sdk` still exits 0. `--yes` continues to grant both consents, and the narrow flag still grants neither system-package approval nor anything beyond the replacement. Signed-off-by: Roman Sirokov --- README.md | 16 +-- apps/rocm/src/main.rs | 75 +++++++++-- apps/rocm/src/therock.rs | 122 ++++++++++++++++-- .../rocm-dash-tui/src/ui/install_manager.rs | 8 +- .../features/runtime_setup.feature | 19 +-- tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 27 +++- xtask/src/e2e_prewarm.rs | 17 ++- 7 files changed, 233 insertions(+), 51 deletions(-) diff --git a/README.md b/README.md index 5fce344fb..8406fe6c0 100644 --- a/README.md +++ b/README.md @@ -255,7 +255,7 @@ the JSON report, not the human-readable one. 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] - [--yes | --approve-replacing-active-default] + [--approve-replacing-active-default] [--yes] rocm install driver [--dkms] [--yes] [--dry-run] [--reconcile] @@ -268,13 +268,13 @@ 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. Pass `--yes` to approve that non-interactively (for example -in scripts or CI, where the prompt would otherwise refuse). `--yes` also approves -installing required system packages (such as OpenMPI for vLLM), which means -`sudo`; if you want only the first approval — because nothing can answer a sudo -password prompt where your command runs — pass -`--approve-replacing-active-default` instead. That is what ROCm CLI's own -non-interactive surfaces (chat, MCP, the dashboard) pass. Because the install +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. Because the install root and manifest are keyed by version, an upgrade or downgrade keeps the previous install on disk — only a same-version reinstall reuses the same install root. `install driver` installs the AMD kernel driver on Linux diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 675a0665b..3ecc39e86 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -7389,8 +7389,12 @@ fn preferred_engine_for_sdk_family(family: &str) -> Option<&'static str> { /// 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. - replace_active_default: bool, + /// 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, @@ -7398,8 +7402,17 @@ struct SdkInstallConsents { 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: yes || approve_replacing_active_default, + replace_active_default, system_packages: yes, } } @@ -7530,6 +7543,14 @@ 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. + // + // "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", @@ -11922,7 +11943,8 @@ 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 "re-run with `--yes`" — a flag the user cannot supply + // refuse with a "re-run with `--approve-replacing-active-default`" + // error — and neither consent flag is something the user can supply // through chat. // // Not `--yes`: that flag also approves system-package installs, and @@ -13380,7 +13402,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)?; - Ok(therock::install_sdk(paths, channel, format, prefix, selector, None, true, true)?.output) + // 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( @@ -23253,8 +23291,9 @@ model recipes #[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 `--yes`" — a flag the user has - // no way to supply from chat or the dashboard. Both the chat classifier + // 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![ @@ -23284,9 +23323,13 @@ model recipes let mcp_args = rocm_chat_tool_requested_args(&call).expect("install_sdk tool builds args"); for args in [&chat_args, &mcp_args] { - assert!( + assert_eq!( consents_for_install_sdk_argv(args).replace_active_default, - "the spawn must approve replacing the active default, got {args:?}" + 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:?}" ); } } @@ -23310,7 +23353,12 @@ model recipes let injected = consents_for_install_sdk_argv( &rocm_chat_tool_requested_args(&call).expect("install_sdk tool builds args"), ); - assert!(injected.replace_active_default); + 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" @@ -23329,7 +23377,12 @@ model recipes "sdk".to_owned(), "--yes".to_owned(), ]); - assert!(typed.replace_active_default && typed.system_packages); + 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 { diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 872c20773..21f8cfaba 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -958,6 +958,14 @@ struct InstallSourceOverride<'a> { 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, @@ -967,15 +975,10 @@ pub(crate) fn install_sdk( version_selector: Option, family_override: Option<&str>, dry_run: bool, - assume_yes: bool, + consent: SdkInstallConsent, ) -> Result { let channel = TheRockChannel::parse(channel)?; ensure_install_format_supported(format)?; - let consent = if assume_yes { - SdkInstallConsent::Preapproved(SdkInstallApprovalSource::AssumeYes) - } else { - SdkInstallConsent::Ask - }; match format { "wheel" => install_wheel_runtime( paths, @@ -2150,8 +2153,16 @@ fn repo_version_without_wheels( /// so a message crediting one would name a flag the user could not have passed. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum SdkInstallApprovalSource { - /// The user passed `--yes` to `rocm install sdk`. + /// 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. @@ -2223,6 +2234,9 @@ fn preapproved_install_line( 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." ), @@ -2244,9 +2258,17 @@ fn fresh_install_line(resolved_display: &str, family: &str) -> String { /// 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 --yes to approve this non-interactively, for example `rocm install sdk --yes`" + "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" ) } @@ -8601,9 +8623,18 @@ echo Python 3.12.10 cache_dir: root.join("cache"), }; - let error = install_sdk(&paths, "release", "tarball", None, None, None, true, 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")); @@ -8798,6 +8829,18 @@ echo Python 3.12.10 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 { @@ -8828,6 +8871,25 @@ echo Python 3.12.10 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)", @@ -8855,6 +8917,44 @@ echo Python 3.12.10 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, diff --git a/crates/rocm-dash-tui/src/ui/install_manager.rs b/crates/rocm-dash-tui/src/ui/install_manager.rs index d0c4d80ce..2d664de4d 100644 --- a/crates/rocm-dash-tui/src/ui/install_manager.rs +++ b/crates/rocm-dash-tui/src/ui/install_manager.rs @@ -490,9 +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 --yes so the null-stdin - // dashboard spawn is not refused at the consent prompt. + // 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/tests/e2e-cucumber/features/runtime_setup.feature b/tests/e2e-cucumber/features/runtime_setup.feature index 3ff422aef..b9404aa80 100644 --- a/tests/e2e-cucumber/features/runtime_setup.feature +++ b/tests/e2e-cucumber/features/runtime_setup.feature @@ -187,7 +187,10 @@ Feature: Runtime configuration # Installing over the active default managed runtime must not silently # displace it. Outside an interactive terminal (as every e2e invocation - # is here), `install sdk` without `--yes` must refuse rather than proceed. + # 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. @@ -197,11 +200,11 @@ Feature: Runtime configuration # 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 --yes is refused + 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 that --yes is required + 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 @@ -228,15 +231,15 @@ Feature: Runtime configuration # 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 also - # mention `--yes` in the usage text): only the real gate names the runtime it - # would replace. + # 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. @id:runtime-install-sdk-other-family-requires-yes @requires-gpu - Scenario: runtime-13 - Installing a different GPU family while a runtime is active is refused without --yes + 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 that --yes is required + 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, diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index e9df5b11e..60fc9d4fa 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -896,17 +896,32 @@ 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 --yes"); + assert!( + rc != 0, + "install sdk unexpectedly succeeded without consent" + ); } -#[then("the error explains that --yes is required")] -async fn assert_reinstall_error_explains_yes(world: &mut E2eWorld) { +#[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}").to_lowercase(); + 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!( - combined.contains("--yes"), - "error does not mention --yes:\n{stdout}\n{stderr}" + narrow < yes, + "error recommends --yes ahead of the narrow flag:\n{stdout}\n{stderr}" ); } diff --git a/xtask/src/e2e_prewarm.rs b/xtask/src/e2e_prewarm.rs index 652286965..fd1e11855 100644 --- a/xtask/src/e2e_prewarm.rs +++ b/xtask/src/e2e_prewarm.rs @@ -480,11 +480,18 @@ pub fn run(channel: &str, keep: usize, prewarm_dir: &Path) -> Result<()> { "pre-warm: installing the {channel} SDK into {}", prewarm_dir.display() ); - // `--yes` because the consent gate keys on the tree's active default - // runtime, not on the channel `decide()` inspected: a shared tree - // pre-warmed for `release` and then pre-warmed for `nightly` reaches - // here with a release runtime already active, and xtask has no - // terminal to answer a prompt with, so the install would refuse. + // `--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, "--yes"]) .status_ok("rocm install sdk")?; From 9ee039a598e67f72e93439ebae249c9de4ce7b6e Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Tue, 15 Sep 2026 13:48:11 +0300 Subject: [PATCH 12/20] fix(install): fail the consent gate closed and carry --yes through freeform MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways `install sdk` could displace the active default runtime without ever asking, both of them reaching the opposite verdict from the one the policy docstring states. `active_default_runtime_relation` trusted `load_runtime_manifests`, which drops a registry manifest that reads fine but does not deserialize. A manifest written by an older binary (`family_source`, `selected_artifact_url` and `installed_at_unix_ms` carry no `#[serde(default)]`) vanished from the list, `current_runtime_manifest` missed, and the gate was skipped with a fresh-install verdict — while `rocm runtimes list` still reported the runtime as active, so one CLI asserted both that a runtime was active and that none was. Parse failures are now reported alongside the manifests that loaded, and an `active_runtime_key` that resolves to nothing fails closed into the consent gate rather than into a hard error, so a consent flag still gets an operator past a stale manifest. `rocm --yes ` re-parses the planner's argv and dispatches it in process, so `install()` ran with both consent flags false no matter what the outer command line said. The surface printed `approval: granted by --yes` and then refused with "re-run with `--approve-replacing-active-default`" — a flag it offers no way to pass — or prompted on a terminal it had just said it need not ask. The narrow flag is now injected before the execution header renders, so what is printed is what runs. Not `--yes`: this surface's `--yes` means "execute the shown plan", not "install system packages with sudo". Also, from the same review round: - Strip a model-supplied `--yes` in the chat `install sdk` arm and withhold the narrow flag on `--dry-run`; correct the comment that claimed neither consent flag can arrive through chat, which is false for the generic `rocm_command` tool. - Validate the device target before the consent gate, so an unresolvable target reports its own error instead of first demanding a consent flag for an install that was never going to work. - Scope the "keyed by version, so the previous install stays on disk" claim to the default managed root in all three places it appeared; `--prefix` is used verbatim for every version and does replace in place. - Pin the `install sdk --help` test to the `--yes` argument's own help text; the page-wide assertion was satisfied by the sibling flag's doc. - Collapse the duplicated README consent paragraph to a pointer. - Fix comment rot in `rocmd` (the gate no longer names `--yes` first), the e2e family-selection comment (selection reads the case-preserving `family=` column, not the slugified key), and the stale "in CI" clause in `therock_sdk_install_test.py`, which no workflow references. EAI-7956 Signed-off-by: Roman Sirokov --- README.md | 18 +- apps/rocm/src/main.rs | 276 ++++++++++++++++-- apps/rocm/src/therock.rs | 237 +++++++++++++-- apps/rocmd/src/lib.rs | 3 +- scripts/therock_sdk_install_test.py | 5 +- tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 4 +- 6 files changed, 482 insertions(+), 61 deletions(-) diff --git a/README.md b/README.md index 1eb8dbc52..e21e17c7a 100644 --- a/README.md +++ b/README.md @@ -201,11 +201,9 @@ 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. Running the command when a managed runtime is already the active default asks first, because the new -install takes over as the active default — that includes installing a different -GPU family or channel, which takes it over just the same. Add -`--approve-replacing-active-default` to approve that non-interactively, such as -from a script; `--yes` also approves it but additionally approves installing -required system packages, which needs `sudo`. +install takes over as the active default; see +[ROCm installation](#rocm-installation) for that gate and the flags that approve +it without a prompt. Then serve a model: @@ -277,10 +275,12 @@ 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. Because the install -root and manifest are keyed by version, an upgrade or downgrade keeps the -previous install on disk — only a same-version reinstall reuses the same install -root. `install driver` installs the AMD kernel driver on Linux +password prompt, which an unattended job cannot. 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. `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. `--json` prints the check result as a single line of JSON instead of text; `--timeout-secs` bounds its network calls diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index fb6ae1e06..15da02596 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -1553,9 +1553,7 @@ 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)?; + let action = prepare_freeform_execution(request, paths, config)?; print!("{}", render_freeform_execution_header(&action)); let mut argv = vec!["rocm".to_owned()]; @@ -1564,6 +1562,59 @@ fn execute_freeform_next_action( dispatch(cli) } +/// 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)?; + apply_freeform_execution_consent(&mut action.args); + Ok(action) +} + +/// 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. +fn apply_freeform_execution_consent(args: &mut Vec) { + 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; + } + ensure_flag(args, "--approve-replacing-active-default"); +} + fn validate_freeform_execution_action(action: &FreeformPlanAction) -> Result<()> { if action.has_placeholders { bail!( @@ -12051,8 +12102,8 @@ fn chat_rocm_command_action_from_args(mut args: Vec) -> Result) -> Result` 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 action = prepare_freeform_execution(request, &test_app_paths(), &config) + .expect("install request should prepare for execution"); + + 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. + assert!( + render_freeform_execution_header(&action) + .contains("--approve-replacing-active-default") + ); + // Re-parsing must reach `install()` with the consent actually set. + let mut argv = vec!["rocm".to_owned()]; + argv.extend(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(), + ]; + 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(); + apply_freeform_execution_consent(&mut args); + assert_eq!(args, before); + } + + // Idempotent: a plan that already carries the flag is not given it twice. + let mut already = vec![ + "install".to_owned(), + "sdk".to_owned(), + "--approve-replacing-active-default".to_owned(), + ]; + 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 = @@ -22356,6 +22548,48 @@ mod tests { ); } + #[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(), + ] + ); + } + #[test] fn chat_tool_call_mutating_install_accepts_requested_build_date() { let call = providers::ChatToolCall { diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index e81dd450d..9eddaf859 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -1852,6 +1852,26 @@ fn install_wheel_runtime( )); } + // 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\ + The aggregate `rocm` distribution ships no GPU backend unless an exact `device-` extra requests one, so this install would produce a runtime that cannot run a kernel.\n\ + Re-run `rocm install sdk` on the target host, or preview the plan with `--dry-run`.\n\n{}", + channel.as_str(), + detect_host_gpu_diagnostics() + ); + } + // Installs with no active default runtime proceed with just an informational // line. Any install that would displace the current active default — whatever // its family or channel, because activation is global — asks for confirmation @@ -1897,20 +1917,6 @@ fn install_wheel_runtime( } } - // 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. - if let Some(reason) = device_target.reason() { - bail!( - "cannot compose a canonical TheRock {} runtime: {reason}.\n\ - The aggregate `rocm` distribution ships no GPU backend unless an exact `device-` extra requests one, so this install would produce a runtime that cannot run a kernel.\n\ - Re-run `rocm install sdk` on the target host, or preview the plan with `--dry-run`.\n\n{}", - channel.as_str(), - detect_host_gpu_diagnostics() - ); - } - let uv = ensure_uv_binary(paths)?; fs::create_dir_all( install_root @@ -2080,7 +2086,8 @@ 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 no active default resolves — a genuinely fresh install, where the new +/// when nothing on disk points at an active default at all — 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 @@ -2094,16 +2101,38 @@ fn quote_display_arg(value: &str) -> String { /// 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 two ways an active default can fail to +/// resolve without any I/O error at all, and both of them used to reach the +/// fresh-install verdict: +/// +/// * 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. +/// +/// Either way `rocm runtimes list` already calls this out as +/// `active_status: missing manifest for active_runtime_key=...`, 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 manifest. fn active_default_runtime_relation( paths: &AppPaths, channel: TheRockChannel, family: &str, resolved_version: &str, ) -> Result> { - let manifests = load_runtime_manifests(paths)?; + 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(None); + return Ok(unresolved_active_default_relation_text( + config.active_runtime_key.as_deref(), + &unparsed, + )); }; Ok(Some(active_default_relation_text( active, @@ -2113,6 +2142,45 @@ fn active_default_runtime_relation( ))) } +/// 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 — no configured active key +/// and every registry manifest parsed. Anything else 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. +fn unresolved_active_default_relation_text( + active_runtime_key: Option<&str>, + unparsed: &[PathBuf], +) -> Option { + let unparsed_text = || { + unparsed + .iter() + .map(|path| path.display().to_string()) + .collect::>() + .join(", ") + }; + match active_runtime_key + .map(str::trim) + .filter(|key| !key.is_empty()) + { + Some(key) if unparsed.is_empty() => Some(format!( + "recorded as `{key}`, but no installed runtime manifest matches it, so what is currently active cannot be determined" + )), + Some(key) => Some(format!( + "recorded as `{key}`, but its manifest could not be read; unreadable runtime manifests: {}", + unparsed_text() + )), + None if unparsed.is_empty() => None, + None => Some(format!( + "unknown: {} of the installed runtime manifests could not be read, so an active default cannot be ruled out; unreadable runtime manifests: {}", + unparsed.len(), + unparsed_text() + )), + } +} + /// Pure wording for [`active_default_runtime_relation`]. /// /// Two shapes, because the two cases are not the same event. When the active @@ -2346,12 +2414,19 @@ fn refuse_non_interactive_message(relation: &str) -> String { /// 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: `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. +/// "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, @@ -5943,12 +6018,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()))? { @@ -5959,12 +6051,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 { @@ -9204,6 +9298,95 @@ echo Python 3.12.10 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)?; + + let manifest_path = runtime_manifest_path(&paths, &manifest.runtime_key); + let mut value: serde_json::Value = serde_json::from_slice(&fs::read(&manifest_path)?)?; + value + .as_object_mut() + .expect("manifest is a JSON object") + .remove("family_source") + .expect("manifest carries family_source"); + fs::write(&manifest_path, serde_json::to_vec_pretty(&value)?)?; + + // Precondition: the file still reads, so this is not the I/O path. + assert!(fs::read(&manifest_path).is_ok()); + assert!( + serde_json::from_slice::(&fs::read(&manifest_path)?).is_err(), + "the test fixture must be unparsable, or this asserts nothing" + ); + + 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 sdk_install_approval_only_prompts_when_an_active_default_is_displaced() { let assume_yes = SdkInstallConsent::Preapproved(SdkInstallApprovalSource::AssumeYes); diff --git a/apps/rocmd/src/lib.rs b/apps/rocmd/src/lib.rs index 0ce25a856..a8aef6c31 100644 --- a/apps/rocmd/src/lib.rs +++ b/apps/rocmd/src/lib.rs @@ -2591,7 +2591,8 @@ fn build_install_sdk_args( // `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 - // `--yes`" — a flag no MCP caller of this tool can supply. + // `--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 diff --git a/scripts/therock_sdk_install_test.py b/scripts/therock_sdk_install_test.py index 534fc0c1e..abe911fa7 100644 --- a/scripts/therock_sdk_install_test.py +++ b/scripts/therock_sdk_install_test.py @@ -487,8 +487,9 @@ def main() -> int: # 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 runs either as - # root in CI or from a developer's terminal. + # 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", diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index 60fc9d4fa..4c0f18f78 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -250,7 +250,9 @@ 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-10. - // The runtime key carries the family, so the registry listing is enough. + // 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() From b4e78e2891cd08e5ad03640678a4a6f02a4ec2fc Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Tue, 15 Sep 2026 13:59:47 +0300 Subject: [PATCH 13/20] docs(readme): link the install-consent section by URL, not a page anchor `Sphinx docs build (-W)` failed on the pointer added in 9ee039a5: README.md: WARNING: 'myst' cross-reference target not found: 'rocm-installation' [myst.xref_missing] The README is not built as one page. `docs/rocm-docs` slices it into several pages with `{include}` plus `:start-after:`/`:end-before:`, and the new link and its target land in different slices: the sentence is inside getting-started's "Configure ROCm and serve a model" chunk, while `### ROCm installation` is inside the chunk that becomes `commands.md`. A bare in-page anchor cannot reach across that split, so the xref has no target. The same hazard already shaped the file: the sibling `[Model serving]` (#model-serving) pointer sits in the three-line gap the surrounding includes deliberately skip, which is why it never warned. Use the absolute form the README already uses for `CONTRIBUTING.md`. It keeps the jump working for readers on GitHub and is an external URL to MyST, so it is never resolved as an xref. Verified with the same command CI runs: `sphinx-build -b html docs/rocm-docs /tmp/rocm-cli-docs -W` succeeds with this change and fails with exactly one warning without it. EAI-7956 Signed-off-by: Roman Sirokov --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index e21e17c7a..b1130d4d6 100644 --- a/README.md +++ b/README.md @@ -202,8 +202,8 @@ show it as `legacy_rocm_status: detected_unmanaged` — running `rocm install sd 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](#rocm-installation) for that gate and the flags that approve -it without a prompt. +[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: From c5934622a5886dd3442167198e399b9b258f0233 Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Tue, 15 Sep 2026 16:35:08 +0300 Subject: [PATCH 14/20] fix(install): fail the consent gate closed on both default_runtime_id shapes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `current_runtime_manifest` resolves the active default through two pointers, but the fail-closed fix only covered the first. When `active_runtime_key` is unset it falls back to `default_runtime_id` and returns `Some` only for an exactly-one match; `_ => None` swallowed both the zero-match and the multi-match cases. `runtime_id` is `therock-:` with no version in it, so two installed versions of one family share it — and `rocm config set-default-runtime` stores the id unvalidated while clearing `active_runtime_key`, reaching both states from a documented command. In either state `rocm install sdk` saw no active default, took the `ProceedFresh` arm and displaced the active default with no prompt and no flag, while `rocm runtimes list` reported `active_status: ambiguous runtime_id=...` or `missing manifest for active_runtime_id=...` for the same config — one CLI asserting both that a runtime is active and that none is. Pass `default_runtime_id` and its match count into `unresolved_active_default_relation_text` and fail closed on both shapes, naming the id that could not be resolved. As before this fails closed into the consent gate rather than into a hard error, so `--approve-replacing-active-default` (or `--yes`) still gets an operator through. Also scope the unparsable-manifest arm to "a config pointer claims something is active". With neither pointer set nothing asserts a displacement risk, so demanding a consent flag there was a false positive on a genuinely fresh install. Co-Authored-By: Claude Opus 4.7 Signed-off-by: Roman Sirokov --- apps/rocm/src/main.rs | 21 ++++ apps/rocm/src/therock.rs | 234 ++++++++++++++++++++++++++++++++++----- 2 files changed, 229 insertions(+), 26 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 15da02596..8ca45f473 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -22588,6 +22588,27 @@ mod tests { "--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] diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 9eddaf859..7cc7ed9c5 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -2102,9 +2102,11 @@ fn quote_display_arg(value: &str) -> String { /// 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 two ways an active default can fail to -/// resolve without any I/O error at all, and both of them used to reach the -/// fresh-install verdict: +/// 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 @@ -2112,14 +2114,24 @@ fn quote_display_arg(value: &str) -> String { /// `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. +/// 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. /// -/// Either way `rocm runtimes list` already calls this out as -/// `active_status: missing manifest for active_runtime_key=...`, 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 manifest. +/// 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, @@ -2131,6 +2143,8 @@ fn active_default_runtime_relation( 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, )); }; @@ -2145,13 +2159,25 @@ fn active_default_runtime_relation( /// 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 — no configured active key -/// and every registry manifest parsed. Anything else 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. +/// `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`, nothing on disk 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. 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 = || { @@ -2161,23 +2187,37 @@ fn unresolved_active_default_relation_text( .collect::>() .join(", ") }; - match active_runtime_key - .map(str::trim) - .filter(|key| !key.is_empty()) - { - Some(key) if unparsed.is_empty() => Some(format!( + 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) + }; + + match (non_empty(active_runtime_key), non_empty(default_runtime_id)) { + (Some(key), _) if unparsed.is_empty() => Some(format!( "recorded as `{key}`, but no installed runtime manifest matches it, so what is currently active cannot be determined" )), - Some(key) => Some(format!( + (Some(key), _) => Some(format!( "recorded as `{key}`, but its manifest could not be read; unreadable runtime manifests: {}", unparsed_text() )), - None if unparsed.is_empty() => None, - None => Some(format!( - "unknown: {} of the installed runtime manifests could not be read, so an active default cannot be ruled out; unreadable runtime manifests: {}", - unparsed.len(), - unparsed_text() + (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, } } @@ -9387,6 +9427,148 @@ echo Python 3.12.10 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)?; + let manifest_path = runtime_manifest_path(&paths, &manifest.runtime_key); + let mut value: serde_json::Value = serde_json::from_slice(&fs::read(&manifest_path)?)?; + value + .as_object_mut() + .expect("manifest is a JSON object") + .remove("family_source") + .expect("manifest carries family_source"); + fs::write(&manifest_path, serde_json::to_vec_pretty(&value)?)?; + assert!( + serde_json::from_slice::(&fs::read(&manifest_path)?).is_err(), + "the test fixture must be unparsable, or this asserts nothing" + ); + + 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); From aa554f3d00ade997edc046c37e0ba614a298616c Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Tue, 15 Sep 2026 18:10:35 +0300 Subject: [PATCH 15/20] fix(install): blame only the active key's own unreadable manifest, name both pointers The `active_runtime_key` arm of `unresolved_active_default_relation_text` claimed "its manifest could not be read" whenever any registry entry failed to deserialize, without checking that an unparsable path was that key's manifest. A deleted active runtime plus an unrelated older-binary manifest therefore pointed the operator at a file that had nothing to do with the fault and never mentioned the runtime that went missing. The arm also ignored `default_runtime_id`, so a stale key alongside an ambiguous or dangling fallback reported only half of what failed to resolve, leaving the operator to repair half the config and hit the gate again. The registry stores each manifest at `.json`, so the file stem decides which cause is the true one; unrelated unparsable entries are appended as a suffix, matching how the two `default_runtime_id` arms already treat them. Both pointers are named when both are set, which is exactly when `current_runtime_manifest` tried and failed on both. Verdicts are unchanged: this still fails closed into the consent gate. Also pin that `install sdk --yes` rejects an attached value. The chat arm strips a model-supplied `--yes` by exact string match, so adding `num_args` to the flag would let `--yes=true` past the strip and re-grant the system-package/sudo consent on a spawn with null stdin, and document why `rocm --yes ` prints a different `tool_call:` in its plan section than in its execution header. EAI-7956 Signed-off-by: Roman Sirokov --- apps/rocm/src/main.rs | 52 +++++++++ apps/rocm/src/therock.rs | 243 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 288 insertions(+), 7 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 8ca45f473..85dfb2d70 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -1606,6 +1606,21 @@ fn prepare_freeform_execution( /// 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. fn apply_freeform_execution_consent(args: &mut Vec) { let is_install_sdk = args.first().is_some_and(|arg| arg == "install") && args.get(1).is_some_and(|arg| arg == "sdk"); @@ -22611,6 +22626,43 @@ mod tests { ); } + #[test] + fn install_sdk_yes_rejects_an_attached_value_so_the_chat_strip_cannot_be_evaded() { + // The chat arm strips a model-supplied `--yes` by exact string match, so + // that strip is only airtight because clap refuses the `=`-form: if + // `--yes=true` parsed, a model could smuggle the system-package/sudo + // consent past `args.retain(|arg| arg != "--yes")` and into a spawn with + // null stdin and no terminal to answer a password prompt. `--yes` on + // `install sdk` is a bare `bool` today, which is what produces the + // rejection; giving it `num_args` later would silently re-grant that + // consent, so pin the rejection here rather than leaving it implicit. + 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; the chat `--yes` strip is an exact string match, 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 the strip is written against 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 { diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 7cc7ed9c5..2a44d6f54 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -2174,6 +2174,20 @@ fn active_default_runtime_relation( /// 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>, @@ -2200,15 +2214,45 @@ fn unresolved_active_default_relation_text( .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. + 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), _) if unparsed.is_empty() => Some(format!( - "recorded as `{key}`, but no installed runtime manifest matches it, so what is currently active cannot be determined" - )), - (Some(key), _) => Some(format!( - "recorded as `{key}`, but its manifest could not be read; unreadable runtime manifests: {}", - unparsed_text() - )), + (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() @@ -9427,6 +9471,164 @@ echo Python 3.12.10 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` @@ -9892,6 +10094,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!( From 4a5361582fcf4b8202ba3f738eb1b2a9bf0a5abd Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Tue, 15 Sep 2026 19:04:27 +0300 Subject: [PATCH 16/20] fix(install): strip an attached-value --yes, disclose the injected consent The chat arm stripped a model-supplied `--yes` by exact string match, so it only stayed airtight because clap rejects `--yes=true` on a bare `bool`. That borrowed a property of clap's error taxonomy which this code does not own: a later `num_args` on the flag would silently restore the system-package/sudo consent 76c6aa3c removed, on a spawn with null stdin and no terminal to answer a password prompt. Match the `--yes=` prefix too, so the guarantee is local. Retarget the test that named the strip but never called it. It pinned the clap rejection only, which is the backstop rather than our code; it now drives `chat_rocm_command_action_from_args` with a model-supplied `--yes=true` argv and asserts the flag does not survive, keeping the clap assertion as an explicitly secondary layer. Without the prefix term the new test fails with the flag sitting in the spawn argv next to the injected consent. Tell the operator why `rocm --yes ` prints two differing `tool_call:` lines. The difference stays deliberate for the reason already documented -- forcing agreement would reach `render_structured_request_plan`, shared with the no-`--yes` review path, and hand a pre-approved command to a human asked to review it -- but the explanation lived only in a doc comment, which reaches the next reader of the file and not the operator looking at the two lines. `apply_freeform_execution_consent` now reports whether it added the flag, and the execution section says so in words when it did. Pin the `default_runtime_id_match_count != 1` invariant with a `debug_assert` rather than prose: at exactly 1 the message would tell the operator no manifest matched the recorded default while one did. Apply `make_test_runtime_manifest_unparsable` to the sibling test that still inlined the identical fixture-corruption steps. Signed-off-by: Roman Sirokov --- apps/rocm/src/main.rs | 179 +++++++++++++++++++++++++++++++-------- apps/rocm/src/therock.rs | 28 +++--- 2 files changed, 158 insertions(+), 49 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 85dfb2d70..96561df7e 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -1553,15 +1553,27 @@ fn execute_freeform_next_action( paths: &AppPaths, config: &RocmCliConfig, ) -> Result<()> { - let action = prepare_freeform_execution(request, paths, config)?; - 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. @@ -1573,12 +1585,15 @@ fn prepare_freeform_execution( request: &str, paths: &AppPaths, config: &RocmCliConfig, -) -> Result { +) -> 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)?; - apply_freeform_execution_consent(&mut action.args); - Ok(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 @@ -1621,13 +1636,25 @@ fn prepare_freeform_execution( /// 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. -fn apply_freeform_execution_consent(args: &mut Vec) { +/// +/// 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; + 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<()> { @@ -1645,7 +1672,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"); @@ -1663,6 +1691,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 } @@ -12133,7 +12175,16 @@ fn chat_rocm_command_action_from_args(mut args: Vec) -> Result 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 + }; + + for attached in ["--yes=true", "--yes=1", "--yes=false"] { + let args = classify(&["install", "sdk", "--prefix", "/tmp/therock", attached]); + assert!( + !args.iter().any(|arg| arg.starts_with("--yes")), + "`{attached}` 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 `{attached}` must leave the rest of the argv and the narrow consent alone" + ); + } + + // A prefix match must not reach flags that merely start the same way, or + // the strip would silently drop arguments the model legitimately sent. + let args = classify(&["install", "sdk", "--prefix", "/tmp/--yes-not-a-flag"]); + assert!( + args.iter().any(|arg| arg == "/tmp/--yes-not-a-flag"), + "the strip must only match the flag itself, 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; the chat `--yes` strip is an exact string match, got {cli:?}" - ), + Ok(cli) => panic!("`{attached}` must not parse, got {cli:?}"), Err(error) => error, }; assert_eq!( @@ -22650,9 +22762,8 @@ mod tests { ); } - // Control: the bare form the strip is written against does parse, so the - // assertions above are about the `=`-form and not about `--yes` being - // rejected outright. + // 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 { diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 2a44d6f54..adaafe9d7 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -2228,6 +2228,16 @@ fn unresolved_active_default_relation_text( // 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!( @@ -9399,21 +9409,9 @@ echo Python 3.12.10 ); write_active_test_runtime(&paths, &manifest)?; - let manifest_path = runtime_manifest_path(&paths, &manifest.runtime_key); - let mut value: serde_json::Value = serde_json::from_slice(&fs::read(&manifest_path)?)?; - value - .as_object_mut() - .expect("manifest is a JSON object") - .remove("family_source") - .expect("manifest carries family_source"); - fs::write(&manifest_path, serde_json::to_vec_pretty(&value)?)?; - - // Precondition: the file still reads, so this is not the I/O path. - assert!(fs::read(&manifest_path).is_ok()); - assert!( - serde_json::from_slice::(&fs::read(&manifest_path)?).is_err(), - "the test fixture must be unparsable, or this asserts nothing" - ); + // 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, From 27eef666a992319a22f1fabfa7e1c1c53ef1686d Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Tue, 15 Sep 2026 20:42:11 +0300 Subject: [PATCH 17/20] test(install): cover the consent disclosure end to end, honest strip guards The `note:` line 4a536158 added under the `execution` header is command output, and AGENTS.md 3 requires user-observable behaviour to be covered by a Gherkin scenario, not only a unit test. Scenario runtime-15 drives the real binary through `rocm --yes ` and pins all three halves of the claim: the `request plan` command carries no replacement consent, the executed one does, and the note says where it came from. A note promising a difference is a lie if the two lines agree, so the first two Thens are what make the third one mean anything. The scenario must not actually install: on a GPU lane that request resolves to a real multi-GiB SDK pull, and the assertions are about output printed before dispatch. The Given points `ROCM_CLI_PYTHON` at a path that does not exist, so `resolve_python_launcher` -- the first step of `install sdk` -- fails offline, instantly, and after the header is on stdout. Without the note the scenario fails on its last step. Retarget the over-broad-match guard in the chat strip test. It fed `--prefix /tmp/--yes-not-a-flag`, a value that starts with neither `--yes` nor `--yes=`, so it was kept by the exact-match strip that preceded this PR, by the two-term strip that replaced it, and by the widened `starts_with("--yes")` it was meant to rule out -- it pinned nothing at all. A bare `--yes-not-a-flag` token is eaten by that widening and kept by the shipped match, and the comment now says plainly that this guards the next edit rather than this one. Drive both terms of the strip from the test named for it. It fed only `--yes=` forms, leaving `arg != "--yes"` to a sibling test named for dry runs; the loop now covers the bare form too, so dropping either term reddens this test alone. Apply `make_test_runtime_manifest_unparsable` to the last test still inlining its fixture-corruption steps, finishing the de-duplication. Signed-off-by: Roman Sirokov --- apps/rocm/src/main.rs | 48 +++++--- apps/rocm/src/therock.rs | 15 +-- .../features/runtime_setup.feature | 31 ++++++ tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 105 ++++++++++++++++++ 4 files changed, 171 insertions(+), 28 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 96561df7e..88f95f81f 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -22700,13 +22700,13 @@ mod tests { } #[test] - fn chat_install_sdk_strips_a_model_supplied_yes_with_an_attached_value() { - // `--yes=true` is a model-supplied argv that reaches the chat arm intact: - // neither `canonicalize_chat_rocm_command` nor - // `validate_chat_rocm_command_safety` splits or rejects it. If the strip - // only matched `--yes` exactly, the flag would survive into a null-stdin - // spawn and re-grant the system-package/sudo consent `76c6aa3c` removed, - // on a spawn with no terminal to answer the password prompt. + 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(), @@ -22718,11 +22718,15 @@ mod tests { args }; - for attached in ["--yes=true", "--yes=1", "--yes=false"] { - let args = classify(&["install", "sdk", "--prefix", "/tmp/therock", attached]); + // 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")), - "`{attached}` must not survive the chat strip, got {args:?}" + "`{supplied}` must not survive the chat strip, got {args:?}" ); assert_eq!( args, @@ -22733,16 +22737,28 @@ mod tests { "/tmp/therock".to_owned(), "--approve-replacing-active-default".to_owned(), ], - "stripping `{attached}` must leave the rest of the argv and the narrow consent alone" + "stripping `{supplied}` must leave the rest of the argv and the narrow consent alone" ); } - // A prefix match must not reach flags that merely start the same way, or - // the strip would silently drop arguments the model legitimately sent. - let args = classify(&["install", "sdk", "--prefix", "/tmp/--yes-not-a-flag"]); + // 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 == "/tmp/--yes-not-a-flag"), - "the strip must only match the flag itself, got {args:?}" + 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 diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index adaafe9d7..f849f5c17 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -9737,18 +9737,9 @@ echo Python 3.12.10 10, ); write_test_runtime_manifest(&paths, &manifest)?; - let manifest_path = runtime_manifest_path(&paths, &manifest.runtime_key); - let mut value: serde_json::Value = serde_json::from_slice(&fs::read(&manifest_path)?)?; - value - .as_object_mut() - .expect("manifest is a JSON object") - .remove("family_source") - .expect("manifest carries family_source"); - fs::write(&manifest_path, serde_json::to_vec_pretty(&value)?)?; - assert!( - serde_json::from_slice::(&fs::read(&manifest_path)?).is_err(), - "the test fixture must be unparsable, or this asserts nothing" - ); + // 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()); diff --git a/tests/e2e-cucumber/features/runtime_setup.feature b/tests/e2e-cucumber/features/runtime_setup.feature index b9404aa80..f450db2ee 100644 --- a/tests/e2e-cucumber/features/runtime_setup.feature +++ b/tests/e2e-cucumber/features/runtime_setup.feature @@ -256,3 +256,34 @@ Feature: Runtime configuration 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 4c0f18f78..7681d04fe 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -984,3 +984,108 @@ async fn install_sdk_help_separates_consents(world: &mut E2eWorld) { "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}" + ); +} From 49ad726b71f9d0079a111b2b7b464117ba3ceb7f Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Wed, 16 Sep 2026 13:21:43 +0300 Subject: [PATCH 18/20] test(e2e): reach the consent gate for a cross-family install (EAI-7956) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scenario runtime-13 (`runtime-install-sdk-other-family-requires-yes`) failed on every GPU lane since `9ee039a5` moved the device-target check ahead of the consent gate. That ordering is right — an install that can never work has to say so rather than first demand a consent flag — but it means a *wheel* install named with a family this host's GPU does not belong to now stops at "detected GPU target `gfx942` belongs to family `gfx94X-dcgpu`", and the displacement the scenario exists to prove is never reached. The step's assertion was correct and the product is correct; the scenario was asking for the one thing the wheel path cannot deliver on a real GPU host. Ask by the tarball format instead. `install_tarball_runtime` resolves the archive for the family it was given, consults no host target, and calls the same `active_default_runtime_relation` gate, so the cross-family displacement is reachable there with the family axis intact — no relaxed assertion, and none of the three Thens weakened. The refusal still bails before the multi-GiB archive is fetched; it costs an 8 KB catalog listing from the host the release wheel index already lives on. Tarball installs are refused outright on Windows, so the scenario gains `@requires-os:linux`. What that gives up is this cross-family case on the Strix Halo Windows lane only — Scenario runtime-11 still covers the refusal there. The `@id:` tag is unchanged. Verified on an MI300X host with a pre-warmed shared runtime tree: runtime-10 through runtime-15 all pass. Falsified twice — with the pre-`9ee039a5` step the scenario reproduces the lane failure verbatim ("error does not name the narrow consent flag"); with the pre-PR family-scoped gate restored the same command installs and activates without asking at all, which the scenario's first two Thens reject. Separately, correct a docstring overclaim on `active_default_runtime_relation`: it returns `None` when neither *config pointer* names an active default, not when "nothing on disk" does. `data/runtimes/active.json` is on disk too and can outlive both pointers through the crash window in `uninstall_runtime`, where the config is saved before the marker is removed. Wording only — `rocm runtimes list` shares the blind spot, and the behaviour is unchanged. Signed-off-by: Roman Sirokov --- apps/rocm/src/therock.rs | 8 +++---- .../features/runtime_setup.feature | 12 ++++++++++- tests/e2e-cucumber/tests/e2e/runtime_steps.rs | 21 +++++++++++++++++-- 3 files changed, 34 insertions(+), 7 deletions(-) diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index def5a3856..39b163961 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -2086,9 +2086,9 @@ 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 nothing on disk points at an active default at all — a genuinely fresh -/// install, where the new -/// runtime takes a slot nothing occupies and there is nothing to consent to. +/// 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 @@ -2161,7 +2161,7 @@ fn active_default_runtime_relation( /// /// `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`, nothing on disk asserts that a runtime is active, +/// 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 diff --git a/tests/e2e-cucumber/features/runtime_setup.feature b/tests/e2e-cucumber/features/runtime_setup.feature index f450db2ee..a9f03ef6b 100644 --- a/tests/e2e-cucumber/features/runtime_setup.feature +++ b/tests/e2e-cucumber/features/runtime_setup.feature @@ -234,7 +234,17 @@ Feature: Runtime configuration # 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. - @id:runtime-install-sdk-other-family-requires-yes @requires-gpu + # + # `@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 diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index 7681d04fe..600abfedc 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -243,13 +243,16 @@ async fn user_reinstalls_sdk_with_yes(world: &mut E2eWorld) { /// 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-10. + // 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`. @@ -261,7 +264,21 @@ async fn user_installs_other_family_without_yes(world: &mut E2eWorld) { .unwrap_or_else(|| { panic!("no candidate family differs from the installed runtimes:\n{runtimes}") }); - let (stdout, stderr, rc) = crate::run_rocm(world, &["install", "sdk", "--family", family]); + // `--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); From bef9ea46d72a3b5d1de5e3eb52d103640f507e8d Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Wed, 16 Sep 2026 14:09:15 +0300 Subject: [PATCH 19/20] docs: cover the SDK install consent gate in the testing guides (EAI-7956) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AGENTS.md requires README.md, the --help text, docs/testing.md and docs/manual-testing.md to move together when observable behaviour changes. The consent gate reached only the first two. docs/manual-testing.md is the sharper gap. Section 1 leaves a managed runtime as the active default, then section 2 tells a tester to run `rocm install sdk --channel release --format wheel --prefix …`. That command now stops at the prompt, or refuses outright in a non-interactive shell, and neither the steps nor the expected result said so — a tester could not tell the feature from a regression. Section 2 now states the prompt is expected, gives the `--approve-replacing-active-default` re-run as the non-interactive route, notes `--yes` differs by also approving a sudo system-package install, and lists the prompt, the refusal, the flag-credited line, and the unaffected `--dry-run` preview as expected results. docs/testing.md named `--yes` for the live SDK acceptance test without ever naming the narrower flag, which is the one the refusal message recommends and the only one ROCm CLI's terminal-less surfaces pass. It now says why this test wants the second consent, points scripts and CI at `--approve-replacing-active-default`, gives both routes as hand checks, and records that `--dry-run` returns before the gate is consulted. Signed-off-by: Roman Sirokov --- docs/manual-testing.md | 26 ++++++++++++++++++++++++++ docs/testing.md | 19 +++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/docs/manual-testing.md b/docs/manual-testing.md index 8f62a8e5f..544e126dc 100644 --- a/docs/manual-testing.md +++ b/docs/manual-testing.md @@ -123,8 +123,34 @@ 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 +``` + +Use `--yes` only if you also want to approve installing required system +packages with `sudo`, which needs a terminal to answer a password prompt. + 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 532ed1193..f84436f91 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -164,6 +164,25 @@ prompts). The gate is not scoped to the family or channel being installed, so 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 From 34f7eb472995f7488e4ec1be0bc321952ee73719 Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Thu, 17 Sep 2026 10:01:48 +0300 Subject: [PATCH 20/20] docs: correct two comments the consent gate work falsified (EAI-7956) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both are comments only; no behaviour changes. `apply_runtime_update` still said the update path is preapproved because "there is no `--yes` on `rocm update`". #402 added that flag in the base merge (`main.rs:322`), so the premise died there. The merge commit enumerated four sites carrying it, reworded them, and reported the sweep complete — this was a fifth, missed because it sits in unchanged branch-side code rather than in a conflict hunk, so the merge message's "Checked the rest of the merged surface" claim was wrong when written. Reworded to the reason that survives and that the other four now give: the update path's approval comes from the runtime the user selected, not from a flag. The merge commit cannot be corrected without a rewrite, so it is corrected in the PR thread instead. `install_wheel_runtime`'s gate comment told a maintainer that a terminal-less caller "needs `--yes`" — the opposite of what the refusal this branch adds says, of `refuse_non_interactive_message`'s own reason for saying it (`--yes` additionally approves installing system packages with `sudo`, whose password prompt an unattended job cannot answer), and of README.md, docs/testing.md and docs/manual-testing.md. Rewritten around the tarball path's already-correct wording, plus the refusal and the flag it names. Swept the tree for both premises: no further instances. The `--yes` requirements in `storage.rs` and `rocm-core/src/fix.rs` are a different gate on different commands, where `--yes` is the whole consent and carries no sudo approval, and are correct as they stand. Three review nits alongside them. README no longer says an unattended job can never answer a sudo prompt, since this repo's own pre-warm passes `--yes` from one and relies on passwordless sudo (`xtask/src/e2e_prewarm.rs:496`). README's new `--prefix` paragraph now admits what `confirm_overwrite_existing_sdk`'s comment already says: a venv whose python stops answering is removed outright, and the gate does not cover it because it keys on the active default, not on the folder. docs/manual-testing.md's `--yes` aside is now scoped to Linux and WSL — the section serves both platforms, but `ensure_openmpi_for_vllm` and `ensure_torch_runtime_dep` both return early on Windows, so there the second consent buys nothing. Signed-off-by: Roman Sirokov --- README.md | 41 +++++++++++++++++++++------------------- apps/rocm/src/main.rs | 7 ++++--- apps/rocm/src/therock.rs | 9 ++++++--- docs/manual-testing.md | 7 +++++-- 4 files changed, 37 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index 47a2f8fcc..1b41268a6 100644 --- a/README.md +++ b/README.md @@ -321,25 +321,28 @@ 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. 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. `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. +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 48c55a1ac..be8780bdd 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -16945,9 +16945,10 @@ fn apply_runtime_update( } // `activate` rather than a bare `true`: the update path is preapproved either - // way (there is no `--yes` on `rocm update`, and no terminal contract), but - // the approval line it prints must not promise an activation that only - // `--activate` performs below. + // way (its approval comes from the runtime the user selected, not from a + // flag, and `rocm update` has no terminal contract), but the approval line it + // prints must not promise an activation that only `--activate` performs + // below. let install_output = therock::install_sdk_for_update( paths, &source.channel, diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index f64432945..e8aa7b9a2 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -1875,9 +1875,12 @@ fn install_wheel_runtime( } // Installs with no active default runtime proceed with just an informational - // line. Any install that would displace the current active default — whatever - // its family or channel, because activation is global — asks for confirmation - // (and needs `--yes` when there is no terminal to answer the prompt). + // 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, diff --git a/docs/manual-testing.md b/docs/manual-testing.md index 544e126dc..e998f9e84 100644 --- a/docs/manual-testing.md +++ b/docs/manual-testing.md @@ -136,8 +136,11 @@ with `--approve-replacing-active-default`: rocm install sdk --channel release --format wheel --prefix .\.rocm-work\data\envs\default --approve-replacing-active-default ``` -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 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: