feat(install): add --yes flag for non-interactive SDK installation (EAI-7956)

[r0x0r]

Summary

Adds a --yes flag to rocm install sdk so an SDK install can proceed non-interactively, and makes displacing the active default ROCm runtime an explicit, opt-in action.

• Once a managed runtime is the active default, rocm install sdk asks before proceeding, and outside an interactive terminal it refuses unless --approve-replacing-active-default (or --yes) is passed — instead of silently taking over as the default.
• An install with no active default runtime never prompts and is unaffected.
• --yes also continues to approve required system-package installs (e.g. OpenMPI for vLLM) without asking.
• --approve-replacing-active-default grants only the runtime-displacement consent. ROCm CLI's own non-interactive surfaces pass that instead of --yes, because they spawn rocm with no terminal and so cannot answer a sudo password prompt.

What the gate is keyed on, and why

The gate is keyed on what the runtime config's active_runtime_key / default_runtime_id actually resolves to — not on the family and channel being installed.

That scoping is load-bearing. Activation is global: finalize_successful_sdk_install activates whatever finished installing last, with no family or channel guard, so rocm install sdk --family gfx120X-all displaces an active gfx110X-all runtime exactly as a same-family upgrade does. An earlier revision of this PR scoped the gate to the same family and channel; that let the cross-family case through with no prompt and no --yes even in CI, while the CLI printed "No existing ROCm SDK found" on a host that plainly had one. active_default_runtime_relation now reports the active default regardless of family or channel.

This is a real behaviour change: users are now prompted in cases they were not, specifically --family/--channel installs onto a host that already has an active default. That is intended — those installs take over the default. The consent property is only ever strengthened: nothing displaces the active default without an interactive confirmation or --yes. An install with no active default still never prompts (nothing is displaced), and re-installing or upgrading the runtime that is already the active default keeps the confirmation it has today.

What the gate actually guards

runtime_key embeds the resolved version, so install_root and the runtime manifest are version-distinct. An upgrade or downgrade lands in its own directory 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 install root.

Every user-facing string states that and only that:

• fresh path: No active ROCm SDK runtime is configured; installing ROCm SDK <v> for family <f>. — it no longer claims no SDK exists anywhere, because the fresh path is now reachable on a host holding a registered-but-unactivated runtime;
• prompt: sdk install: an existing ROCm SDK is the active default runtime / active default runtime: <relation>;
• refusal, --yes clap help, README.md, docs/testing.md — all say "active default", and the help and README explicitly say a different family or channel takes over the default just the same.

The relation text has two shapes: upgrade|downgrade|reinstall from installed <v> (<key>) when the active default is the same family and channel (a version comparison is meaningful), and replaces active default <v> for family <f> on the <c> channel (<key>) when it is not (comparing versions across unrelated runtimes would invent a relation).

rocm update --apply prints the truth now

apply_runtime_update used to pass assume_yes: true into the gate, so essentially every rocm update --apply over an existing runtime printed Approved by --yes: … which becomes the active default runtime. Both halves were false: Command::Update has no --yes flag for the user to have passed, and apply_runtime_update activates only inside if activate.

The bool is replaced by a consent enum that records where the approval came from:

enum SdkInstallConsent { Ask, Preapproved(SdkInstallApprovalSource) }
enum SdkInstallApprovalSource { AssumeYes, UpdateApply { activates: bool } }

install_sdk_for_update now takes activate_after_install and builds UpdateApply { activates }, so the line states only what that path will do: with --activate it says the install becomes the active default; without it, it says The active default runtime is unchanged; re-run with --activate, or use \rocm runtimes activate`, to switch to it.Neither credits--yes. The update path stays **preapproved** — it is non-interactive by construction and must never reach a prompt — so nothing about --yes` propagation is weakened.

Non-interactive callers

Every surface that spawns rocm with null stdin passes a consent flag, so the refusal cannot silently break installs from chat, MCP, the dashboard, or CI.

Which flag, and why it matters. --yes has always carried a second consent: approve required system-package installs, which means running sudo. It is threaded straight through to maybe_auto_install_sdk_preferred_engine(paths, finalized, yes) → ensure_openmpi_for_vllm(approved). Injecting --yes on a terminal-less spawn therefore granted an approval that spawn can never honour: on Linux, for a vLLM-preferred family, with OpenMPI absent and neither root nor passwordless sudo, ensure_openmpi_for_vllm stopped taking its early return at main.rs:7412 (print the manual commands, warn, continue) and instead fell through to run_system_package_install_plan, which runs sudo … with Stdio::inherit() against null stdin. 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 vLLM engine install that previously proceeded. (rocm install sdk still exits 0: engine_auto_install_failure_is_fatal only catches UnusableRuntimeAfterInstall. What is lost is the engine, and on a surface that owns a terminal the sudo prompt reaches /dev/tty with a TUI in raw mode in front of it.)

The two consents are now separate at the argv boundary rather than overloaded onto one bool. SdkInstallConsents::resolve maps --yes to both and --approve-replacing-active-default to the first only; system_package_install_action extracts the shared decision out of ensure_openmpi_for_vllm and ensure_torch_runtime_dep so it is testable without a package manager. A user-typed --yes is unchanged in every respect, including still escalating a failed package install it explicitly approved.

These pass --approve-replacing-active-default:

• the apps/rocm chat install sdk arm and its MCP install_sdk tool-args builder;
• the apps/rocmd MCP install_sdk tool (build_install_sdk_args) — a second, independent argv builder whose child is spawned with .stdin(Stdio::null()). Real-install path only; the dry-run path returns before the gate, so it stays bare;
• the dashboard install manager build_args() — real-install branch only;
• the onboarding wizard build_install_args;
  xtask/src/e2e_prewarm.rs and scripts/therock_sdk_install_test.py keep --yes — deliberately, and this is the one place the two flags diverge. xtask was previously left bare because the old family-scoped gate could not fire there; under active-default scoping it can (a shared tree pre-warmed for release and then for nightly reaches that call with a release runtime active, and xtask has no terminal), so it needs a consent flag. But both are provisioning harnesses whose job includes installing the system packages the SDK needs, and both run as root in CI or from a developer's terminal, where a sudo prompt is answerable and a failed package install is worth failing on. The SDK install harness now appends --yes only on the real-install path, matching apps/rocmd: a dry run returns before the gate.

Consent is preserved rather than bypassed: the chat/MCP paths still go through ChatRocmCommandAction::Approval with --yes visible in the approved command, and rocmd's install_sdk remains in mcp_tool_requires_direct_approval and is annotated destructiveHint.

Also in this PR: host-ROCm-vs-TheRock version explanation

Alongside the --yes work, this PR surfaces a short explanation when the host's legacy/system ROCm is newer than the TheRock ROCm version being installed, so a user does not read a seemingly-older selected version as a regression:

• host_rocm_version_newer_than reports the host's ROCm version only when it is strictly newer than the version being installed.
• A version_note: line is added to both the wheel and tarball install summaries, and echoed as a visible progress line on the real-install path.
• repo_version_without_wheels drives a warning: line when the repository's newest version has no installable PyTorch wheels for this Python/platform.
• The comparison does not reuse the PyPI index comparator, which falls back to a lexicographic compare and so ranked host strings such as 7.2.4-98 above 7.13.0. A lenient parse_host_version strips a build suffix, tolerates a two-component report, and yields None — "can't tell", not "newer" — when either side is unparseable.

Implementation notes

• therock::install_sdk returns SdkInstallResult { output, mutated }; callers finalize the runtime only when the install actually mutated state, rather than keying off dry_run alone.
• active_default_runtime_relation propagates manifest-directory and config read errors instead of degrading to "no active default" — a read error is precisely when we are least sure what is active, and swallowing it would skip the gate.
• current_runtime_manifest is now pub(crate) so the gate resolves the active default the same way runtimes list, runtimes activate and update do, rather than reimplementing the active_runtime_key → unambiguous-default_runtime_id fallback.
• The audit record is derived from mutated, so an install declined at the prompt is logged as sdk install cancelled ... rather than completed.
• Every user-facing line is produced by a small pure helper (fresh_install_line, preapproved_install_line, refuse_non_interactive_message, active_default_relation_text) so the exact wording is unit-testable.

Testing

Gates run on Linux: cargo test --workspace --all-targets and cargo clippy --workspace --all-targets -- -D warnings, both clean on this head.

Unit tests (each falsified — the covered code was broken, the test confirmed red, then restored):

• active_default_relation_gates_a_different_family_or_channel — the regression this revision fixes. Falsified by restoring the family/channel filter.
• active_default_relation_none_without_an_active_default — no active default, with and without registered manifests, must not prompt. Falsified by adding a newest-manifest fallback.
• active_default_relation_classifies_upgrade_downgrade_reinstall — same-family relation wording preserved.
• active_default_relation_propagates_manifest_read_errors — a read error must not degrade to "fresh".
• sdk_install_approval_only_prompts_when_an_active_default_is_displaced — full matrix over Ask / Preapproved(AssumeYes) / Preapproved(UpdateApply) × terminal. Falsified by making the non-interactive Ask arm proceed.
• preapproved_install_line_credits_the_real_consent_source — the update --apply arms must not credit --yes, and the no---activate arm must not claim the install becomes the active default. Falsified by collapsing the arms onto the --yes text.
• fresh_install_line_claims_no_absent_sdk — the fresh line must not say "no existing ROCm SDK found". Falsified by restoring that string.
• update_has_no_yes_flag_to_credit — pins the premise of UpdateApply: rocm update --help offers no --yes. Falsified by adding one.
• an_injected_consent_does_not_approve_privileged_package_installs — the consent split. It parses the argv a non-interactive surface actually produces and asserts what that argv consents to, rather than which flag string it contains, then pins both halves: the injected consent maps to PrintManualCommands on a host without passwordless sudo, a user-typed --yes still maps to RunPlan { escalate_failure: true }, and the root/passwordless path still installs with no approval at all. Falsified by folding the narrow flag back into system_packages.
• install_sdk_real_install_args_approve_only_the_runtime_replacement (apps/rocmd) — the second, independent argv builder must carry the narrow flag and never --yes. Falsified by restoring --yes.
• install_sdk_help_separates_the_two_consents_yes_carries — --help must document the narrow flag and say it excludes system-package installs, because a reader who takes the two flags for synonyms reaches for --yes from a script and gets a sudo prompt nothing can answer. Falsified by dropping the distinction from the help text.
• install_sdk_help_describes_the_gate_as_replacing_the_active_default, plus the chat/MCP/dashboard argv tests (updated to assert the resolved consent, not the flag spelling).
• update_has_no_yes_flag_to_credit is a canary, not a regression test: it pins the premise of SdkInstallApprovalSource::UpdateApply (that rocm update offers no --yes to credit) and would still pass with the production change reverted. Stated here because that is worth knowing when reading the test list. Every other added test references symbols or strings absent at prw-base and fails on revert.

E2E scenarios in tests/e2e-cucumber/features/runtime_setup.feature:

• @id:runtime-install-sdk-overwrite-requires-yes (runtime-11, @requires-gpu) — a reinstall with neither consent flag exits non-zero, and the error names --approve-replacing-active-default ahead of --yes. Runs on the per-push self-hosted GPU lane.

• @id:runtime-install-sdk-other-family-requires-yes (runtime-13, new, @requires-gpu) — the newly gated case: installing a different GPU family while a runtime is the active default is refused without either consent flag. runtime-11 cannot catch this — it reinstalls the same family, so it passed under the old family-scoped gate too. The third Then asserts the error names the active default it would replace, which is what separates a real gate refusal from any other non-zero exit that happens to print a consent flag. No @nightly: it is a refusal, so it costs a resolve, not a multi-GiB install.

• @id:runtime-install-sdk-overwrite-with-yes (runtime-12, @requires-gpu @nightly) — with --yes the reinstall proceeds; assertion updated to the new Approved by --yes: an existing ROCm SDK is the active default runtime line. Exercised by the scheduled Nightly workflow's e2e-gpu-nightly job, or an include_nightly dispatch of e2e-selfhosted.yml.

• @id:runtime-install-sdk-help-separates-consents (runtime-14, new) — rocm install sdk --help must offer --approve-replacing-active-default and state that it does not approve system-package installs. Help-text coverage through the built binary, which a render_long_help() unit test cannot prove; runtime-11 sets that precedent. No runtime state, no GPU, so this one does run on the default GitHub-hosted PR lane.

Where each lane covers what: runtime-14 runs on the default GitHub-hosted PR lane (ci.yml); the other three are @requires-gpu, so none of those run there (ci.yml) — there, coverage of the gate is the unit tests above. runtime-11 and runtime-13 run on the per-push self-hosted GPU lanes (e2e-selfhosted.yml); runtime-12 is additionally @nightly.

Gap: cargo xtask e2e and python scripts/smoke_local.py were not run for this revision — the Linux build host became unreachable partway through the e2e run and did not return. The full workspace test and clippy gates above did complete on this exact tree. The CI lanes are authoritative for the e2e result.

Note on cost: the refusal is not download-free. The gate reports the version relation, so it runs after the Python launcher is resolved and the channel index is read; on a cold host the launcher step can still fetch. What it does bail before is the SDK and torch download and any change on disk.

Merged with main

origin/main was merged into this branch (no rebase, no force-push). Conflicts and their resolutions are recorded in the merge commit message. Notably, both sides appended scenarios numbered runtime-08/runtime-09; main's canonical-provenance scenarios keep 08/09 and this branch's --yes scenarios were renumbered (they are runtime-11/runtime-12, the cross-family one is runtime-13, and the help scenario is runtime-14) so indexes stay sequential per tests/e2e-cucumber/tests/feature_naming.rs. The @id: tags are unchanged.

Follow-up at d6435b40

Three user-facing strings still credited or recommended --yes on paths that
never pass it:

• install_sdk took a bare assume_yes: bool, so every install from a
  terminal-less surface printed Approved by --yes.
  SdkInstallApprovalSource gains an ApproveReplacingActiveDefault variant,
  SdkInstallConsents records which flag granted the replacement instead of
  flattening it to a bool, and install_sdk takes the SdkInstallConsent.
  preapproved_install_line_credits_the_real_consent_source now pins the new
  arm's wording.
• refuse_non_interactive_message told scripts and CI to re-run with --yes —
  the flag whose second consent they cannot honour. It now names
  --approve-replacing-active-default first and still explains when --yes is
  right. New refusal_recommends_the_narrow_flag_before_yes pins the order; the
  e2e step is now the error explains how to approve the replacement non-interactively and asserts the same ordering, and runtime-11/runtime-13
  are retitled "without consent".
• The dashboard build_args test comment still described a --yes it no longer
  asserts; it now also pins that --yes is absent.

Also, from the non-blocking list: the pre-warm comment now gives the real reason
it keeps --yes (it wants the system packages, and its runners have passwordless
sudo), the OpenMPI escalation site records that finish_sdk_install downgrades
that error via engine_auto_install_failure_is_fatal so install sdk still
exits 0, and the README synopsis no longer implies the two consent flags are
mutually exclusive.

[michaelroy-amd]

Current head 792ff7ffce02b4107c3206ac8a10377da0ce4cde is CONFLICTING/DIRTY, so its otherwise-green checks do not validate the merge result. Please rebase onto current main, resolve the install-path conflicts, rerun the required install and Cucumber gates on the rebased head, and then re-request review.

[r0x0r]

Rebased onto current main (now 0a2a239) — the branch is MERGEABLE again.

Conflict resolved (1 file):

• .github/workflows/e2e-selfhosted.yml — main reworked the shared-runtime pre-warm block to the cache-aware cargo xtask e2e-prewarm --channel release path (EAI-8057). That supersedes this branch's edit, which had added --yes to the old inline install sdk guard. I took main's pre-warm block wholesale: the cold install it performs is a fresh install (empty tree), and per the flag's own contract a fresh install never prompts, so no --yes is needed there. The --yes feature itself (in main.rs/therock.rs) merged cleanly.

Also fixed a rebase-induced numbering collision:

• tests/e2e-cucumber/features/runtime_setup.feature — main now uses Scenario numbers 1–5, so this branch's two new scenarios (runtime-install-sdk-overwrite-requires-yes, runtime-install-sdk-overwrite-with-yes) were renumbered from 4/5 to 6/7. The @id: tags were already unique; only the human-facing numbers collided.

Re-ran on the rebased head (local, macOS):

• cargo test -p rocm --bin rocm therock — 77 passed (1 ignored: the ENOSPC disk-fill test)
• cargo test -p rocm --bin rocm sdk_install_approval — the overwrite-only-prompts test passes
• cargo test -p e2e-cucumber --no-run — Cucumber suite compiles green

The GPU/nightly install scenarios (Scenario 6/7, @requires-gpu) run on the self-hosted lane. Leak scan clean; commit signed + DCO. Re-requesting review.

[rominf]

I read through this PR against the diff (didn't check anything out locally, just read the patch and the surrounding code).

The --yes flag mechanism itself looks solid: fresh installs never prompt, overwriting an existing managed SDK now goes through a clear approval gate (sdk_install_approval), the non-interactive refusal path bails before any download, and install_sdk returning SdkInstallResult { output, mutated } instead of keying finalization off dry_run is a genuinely better signal (the "user said no at the prompt" case correctly reports mutated: false and skips activation/engine auto-install). The new unit tests (existing_runtime_relation_*, sdk_install_approval_only_prompts_when_overwriting_existing) cover the interesting edge cases, including propagating a manifest-read error instead of silently treating it as "no existing runtime." The e2e scenarios match the stated intent and the call sites were all threaded through consistently.

One thing worth flagging before merge: this PR also quietly adds a second, unrelated feature. Alongside the --yes work, apps/rocm/src/therock.rs gained host_rocm_version_newer_than, repo_version_without_wheels, the newest_repo_version field on PipRuntimeResolution, and the version_note:/warning: lines they produce in both the wheel and tarball install paths — none of which have anything to do with non-interactive installation. This is a legacy-ROCm-vs-TheRock-version explanation feature, and it's not mentioned anywhere in the PR title, summary, or implementation notes, so a reviewer approving "the --yes flag PR" is also approving a second behavior change they weren't told about. It's also the one part of the diff with materially thinner testing: repo_version_without_wheels got a unit test, but host_rocm_version_newer_than (used at all four call sites) has none, and neither has any assertion (unit or e2e) on the actual version_note/warning output text. It also means every SDK install/dry-run now does an extra unconditional filesystem probe (detect_legacy_rocm_summary walking /opt, /usr/local, etc.) that has nothing to do with the stated change. I'd ask for this to be split into its own PR, or at minimum have the summary updated to describe it and get equivalent test coverage.

Everything else — the #[allow(clippy::too_many_arguments)] on the now 8-arg install_sdk, the prompt/confirmation code mirroring the existing confirm_uninstall pattern, the apply_runtime_update and dry-run call sites always passing assume_yes: true where that's appropriate (a fresh dry-run/update install never prompts) — looked fine on read-through.

[rominf]

This is where the bundled, undisclosed second feature starts (host_rocm_version_newer_than, and further down repo_version_without_wheels / newest_repo_version / the warning: line and the tarball-path duplicates at the equivalent spot in install_tarball_runtime). It explains a host's legacy ROCm version vs. the TheRock version being installed — unrelated to the --yes flag this PR is about, not mentioned in the PR description, and the only new logic in the diff without direct test coverage (repo_version_without_wheels has a unit test; host_rocm_version_newer_than itself doesn't, and no test asserts on the version_note:/warning: output text). Please split this into its own PR (with its own description and tests) or fold it into this one's summary so reviewers know it's in scope.

[r0x0r]

Thanks — took the "disclose + cover it" option rather than splitting, since the version explanation and the --yes work touch the same install paths.

• Disclosed: the PR summary now has an "Also in this PR: host-ROCm-vs-TheRock version explanation" section describing host_rocm_version_newer_than, the version_note:/warning: lines in both the wheel and tarball paths, newest_repo_version / repo_version_without_wheels, and the detect_legacy_rocm_summary probe.
• Coverage (b64d39f): split the filesystem probe out of host_rocm_version_newer_than into a pure host_version_newer_than core and routed the note/warning strings through small pure builders (wheel_host_version_note, tarball_host_version_note, no_wheel_warning_message). Added host_version_newer_than_reports_only_a_strictly_newer_host (the newer-than decision across newer/equal/older/none) and host_version_notes_and_warning_render_the_expected_text (asserts the exact version_note/warning output text for both paths). No behavior change.

(The branch was force-pushed after re-signing the original commit, which is content-identical to before; the test commit is the only new content.)

[r0x0r]

Re-verified at 62105bb. Nothing changed here since your re-read at b64d39f; confirming the disclosure and the coverage still hold at the current head, and that the correctness fix from the later round landed inside this same feature:

• Disclosure: the PR description's "Also in this PR: host-ROCm-vs-TheRock version explanation" section still names host_rocm_version_newer_than, the version_note:/warning: lines on both the wheel and tarball paths, newest_repo_version/repo_version_without_wheels, and the detect_legacy_rocm_summary probe.
• Coverage: host_version_newer_than_reports_only_a_strictly_newer_host (therock.rs:6110) and host_version_notes_and_warning_render_the_expected_text, alongside repo_version_without_wheels_warns_only_when_newest_is_newer.
• The gap the tests then exposed: the newer-than decision was still routed through compare_version_strings, which degraded to a lexicographic compare for build-suffixed and two-component host versions. That is fixed (lenient parse_host_version, None when either side is unparseable) and the test now covers the realistic host shapes — see the reply on the therock.rs:1215 thread.

So the "only new logic without direct test coverage" is no longer that, and the coverage found a real bug rather than just recording current behavior.

Green on Linux at 62105bb: cargo test --workspace --all-targets, cargo clippy --workspace --all-targets -- -D warnings; cargo xtask e2e on a GPU host passes runtime-08.

[rominf]

Re-reviewed at head b64d39f (delta since the reviewed 0a2a239b is apps/rocm/src/therock.rs only, +83/-8).

Prior scope finding: resolved. The bundled host-ROCm-vs-TheRock version-note feature (host_rocm_version_newer_than, repo_version_without_wheels, newest_repo_version, the version_note:/warning: lines) wasn't split out, but the other remedy I offered was taken instead:

• Disclosed: the PR description now has an explicit "Also in this PR: host-ROCm-vs-TheRock version explanation" section naming every symbol involved (host_rocm_version_newer_than, newest_repo_version/repo_version_without_wheels, the version_note/warning lines on both paths, and the detect_legacy_rocm_summary probe), so a reviewer approving this PR now knows what's in scope.
• Covered: commit b64d39f splits the filesystem probe out of host_rocm_version_newer_than into a pure host_version_newer_than(host_version: Option<String>, resolved_version: &str) core, and routes the wording through pure builders (wheel_host_version_note, tarball_host_version_note, no_wheel_warning_message). Two new unit tests now cover the newer/equal/older/none decision matrix and assert the exact version_note/warning output text for both the wheel and tarball paths. I confirmed apps/rocm/src/main.rs is byte-identical between 0a2a239b and b64d39f — no behavior changed, only the missing test coverage was added.

That resolves the concern as raised (thin/missing tests, no disclosure); the extra unconditional detect_legacy_rocm_summary filesystem probe on every install/dry-run is a design tradeoff of the (now-disclosed) feature itself rather than something left unaddressed.

Rest of the diff (--yes mechanism): re-checked SdkInstallApproval/sdk_install_approval, existing_runtime_relation, confirm_overwrite_existing_sdk, and all 4 install_sdk/SdkInstallResult{output, mutated} call sites (main.rs:2383, 11658, 14392, 14409). All consistent: fresh installs never prompt, overwrite goes through the approval gate, apply_runtime_update's two call sites intentionally pass assume_yes: true unconditionally (correct — updating an existing runtime for the same family/channel is the entire point of update --apply, so re-prompting to overwrite what the user just asked to update would be wrong). cargo fmt --check on the changed file passes clean. No AGENTS.md violations found (commits signed off, no AI boilerplate in commit messages, PR now correctly scoped/disclosed).

No new findings at reviewable confidence. Looks safe to approve after final CI validation — note the self-hosted GPU E2E lane is currently red at this head (bench-load-real-serve, chat-end-to-end-local-model, serve-vllm-inference, serve-vllm-default-on-instinct), but those scenarios are serve/inference-related, not install/--yes/version-note, and that lane is advisory rather than a required check — worth a glance before merge but not something I'd attribute to this diff.

[r0x0r]

Thanks for the thorough re-read at b64d39f. Nothing to change here — the disclosure + split-out pure host_version_newer_than core and the wheel/tarball version_note/warning builder tests were exactly the remedy offered, and main.rs is byte-identical to 0a2a239b so no behavior moved. Agreed the red self-hosted GPU E2E lane (bench-load-real-serve, chat-end-to-end-local-model, serve-vllm-inference, serve-vllm-default-on-instinct) is serve/inference-scoped and unrelated to --yes/version-note; that lane is advisory, not required. Ready for your approval whenever you're set.

[r0x0r]

@rominf the bundled version-explanation feature is now disclosed and covered, in b64d39f:

• Disclosed — the PR summary has an "Also in this PR" section describing host_rocm_version_newer_than, the version_note:/warning: lines in the wheel and tarball paths, newest_repo_version / repo_version_without_wheels, and the detect_legacy_rocm_summary probe.
• Covered — split the filesystem probe out into a pure host_version_newer_than core with note/warning builders, and added host_version_newer_than_reports_only_a_strictly_newer_host (newer/equal/older/none) and host_version_notes_and_warning_render_the_expected_text (asserts the exact note/warning text for both paths). No behavior change.

Ready for another look when you get a chance — thanks.

[rominf]

I took another pass over this at b64d39f, this time building and running the version comparator and tracing every install sdk argv call site rather than reading them. That turned up real problems, so I'm reversing my earlier comment on this same head ("no new findings at reviewable confidence; looks safe to approve after final CI validation") — that was wrong, and the details are in the inline comments below.

The core issue is that the new RefuseNonInteractive bail is a hard failure for callers that never got the --yes flag. interactive_terminal() is stdin().is_terminal() && stdout().is_terminal(), and its own doc comment names this exact case: "the dashboard spawning rocm as a child" makes it false. Every non-TTY caller of install sdk in this repo — the chat approval arm, the install_sdk MCP tool, both TUI install paths, the e2e step definition, and the prewarm xtask — still emits a bare install sdk. Once a runtime exists, all of them now bail instead of installing. The chat arm is the clearest tell: its four sibling arms all call ensure_flag(&mut args, "--yes") and the SDK arm doesn't.

Beyond that: the new host_version_newer_than feeds host version strings into a comparator written for PyPI index versions, and that comparator falls back to a plain string compare when either side doesn't parse. Host versions frequently don't parse (build suffixes, two-component forms), and the fallback reports "host is newer" whenever the host string sorts lexicographically above the resolved one. 7.2.4-98 vs 7.13.0 is a false positive — and 7.2.4 is exactly what the GPU runner reports today (legacy_rocm_version: 7.2.4 in the lane on this head).

Two smaller ones: the cancel path returns mutated: false but the audit log still records "sdk install completed", and the new scenario 7 asserts only things its own Given already guarantees.

Merge state: this PR is CONFLICTING/DIRTY against main. git merge-tree --write-tree origin/main <head> conflicts in tests/e2e-cucumber/tests/e2e/runtime_steps.rs — the base is 18 commits behind, and main rewrote user_installs_sdk to route through run_rocm_with_scenario_env with a cli_failure_report while this branch rewrote the same function to add --yes. This is a recurrence of the conflict flagged in the 2026-08-26 review on an earlier head. Whatever the checks say at b64d39f, they do not validate the merge result — worth re-running after the rebase.

CI at this head: the GPU lane is red — 85 scenarios, 9 failed, 4 unexpected (bench-load-real-serve, chat-end-to-end-local-model, serve-vllm-inference, serve-vllm-default-on-instinct). All four are serve/inference, none touch install or --yes, so they look unrelated to this change — but the lane still needs to end green. Note also that scenario 6 passed there while scenario 7 never ran: it's @nightly, so it only executes under E2E_INCLUDE_NIGHTLY=1 and gets no per-PR signal at all.

A couple of things I suspected and checked that did not hold up, for the record: the scenario numbering in runtime_setup.feature does not collide with main (head has 1,3,4,5,2,6,7; main has 1,3,4,9,5,8,2), and the 2-minute subprocess timeout on approved installs is pre-existing on main, not introduced here.

[rominf]

The --yes flag never reaches the non-interactive callers, so they now hard-fail.

(Anchored here on the new flag doc — the actual sites are listed below, all outside this diff.)

Every one of these spawns rocm with .stdin(Stdio::null()), so interactive_terminal() is false, so with an existing runtime the new SdkInstallApproval::RefuseNonInteractive arm bails with "an existing ROCm SDK would be overwritten; re-run with --yes" — a flag the user has no way to supply through these surfaces:

• apps/rocm/src/main.rs:10212 — the chat install sdk arm returns ChatRocmCommandAction::Approval { args, .. } without touching args. Its four siblings (:10220 driver, :10256, :10283, :10297) all call ensure_flag(&mut args, "--yes") first. Path: dash_seam.rs:64 execute_approved -> run_internal_mcp_call(.., true) -> main.rs:11261 -> run_rocm_capture_for_paths -> main.rs:11440-11463 (Stdio::null() stdin).
• apps/rocm/src/main.rs:12021-12043 — rocm_chat_tool_requested_args for the install_sdk MCP tool, same story via main.rs:11356.
• crates/rocm-dash-tui/src/ui/install_manager.rs:136-158 build_args() — never adds --yes; spawned by crates/rocm-dash-tui/src/jobs.rs:73-79 with null stdin.
• crates/rocm-dash-tui/src/ui/onboarding.rs:185-200 build_install_args — same.

Adding ensure_flag(&mut args, "--yes") to the chat/MCP arms matches the existing convention exactly. The TUI paths need the same treatment (or an explicit confirm step in the TUI that then passes --yes). Either way this should be covered before merge, because the failure mode is "reinstall silently stops working from the dashboard" rather than anything loud.

[r0x0r]

Verified at 62105bb — fixed on the prior head (82281a6), confirmed against current code. All four sites you listed now carry the flag:

• chat install sdk arm — main.rs:11537 ensure_flag(&mut args, "--yes"), matching the install driver arm immediately below it.
• MCP install_sdk tool — rocm_chat_tool_requested_args (main.rs:13352) pushes --yes into the argv it builds. It does not route through the classifier, so it needed its own fix.
• install_manager.rs:153 build_args() — --yes on the real-install branch only; the dry-run branch stays flag-free, since a preview never mutates and so never reaches the gate.
• onboarding.rs:193 build_install_args.

Consent is preserved rather than bypassed: the chat/MCP paths still return ChatRocmCommandAction::Approval, and the rendered command shown in that approval now includes --yes (asserted in rocm_chat_tool_requested_command), so the user approves the overwriting command explicitly.

Regression test: install_sdk_chat_and_mcp_args_carry_yes_for_non_interactive_spawn (main.rs:22367) covers both the classifier arm and the MCP args builder, which previously had none. The TUI paths are asserted in their own crates (install_manager.rs:489, onboarding.rs:679/927/957/994), including the negative case that the dry-run preview does not carry the flag.

One site I checked and left alone: internal_mcp_install_sdk_args (main.rs:13034) has no --yes, but both its callers pass dry_run: true, so it can never reach the approval gate.

Green on Linux at 62105bb: cargo test --workspace --all-targets, cargo clippy --workspace --all-targets -- -D warnings.

[rominf]

host_version_newer_than reports "host is newer" for a large class of realistic host versions.

compare_version_strings (:3686-3693) falls back to a raw string compare when either side fails parse_version:

match (parse_version(left), parse_version(right)) {
    (Some(l), Some(r)) => l.cmp(&r).then_with(|| left.cmp(right)),
    _ => left.cmp(right),   // lexicographic
}

parse_version (:3695-3728) requires MAJOR.MINOR.PATCH[rcN|aN], so build-suffixed and two-component host versions return None. That comparator was written for PyPI index versions, which always have that shape; host versions don't. I extracted these four functions verbatim into a scratch binary and ran them:

host=7.14.0     resolved=7.13.0            parses=true   -> HOST NEWER  (correct)
host=7.2.4-98   resolved=7.13.0            parses=false  -> HOST NEWER  (WRONG)
host=7.2.4-98   resolved=7.14.0            parses=false  -> HOST NEWER  (WRONG)
host=7.4        resolved=7.13.0            parses=false  -> HOST NEWER  (WRONG)
host=7.9        resolved=7.13.0            parses=false  -> HOST NEWER  (WRONG)
host=7.2        resolved=7.13.0            parses=false  -> HOST NEWER  (WRONG)
host=7.13.0-56  resolved=7.13.0            parses=false  -> HOST NEWER  (WRONG)
host=7.9.0-1    resolved=7.13.0            parses=false  -> HOST NEWER  (WRONG)
host=7.9.0-1    resolved=7.13.0a20260416   parses=false  -> HOST NEWER  (WRONG)
host=6.4.1-123  resolved=6.13.0            parses=false  -> HOST NEWER  (WRONG)
host=6.4.1-123  resolved=7.13.0            parses=false  -> ok
host=5.7.1-4    resolved=7.13.0            parses=false  -> ok
host=7.0.0-77   resolved=7.13.0            parses=false  -> ok

The rule is just "does the host string sort above the resolved string" — "7.2.4-98" > "7.13.0" because '2' > '1'. This isn't hypothetical: the GPU runner reports legacy_rocm_version: 7.2.4 today, so a build-suffixed variant of it is the expected shape, and any 7.2/7.4/7.9 host lands in the wrong bucket against a 7.13 resolution.

The new unit test at :5609-5622 only uses clean three-component versions, which is why it passes.

Suggested fix: don't route host versions through the index comparator. Parse the host version leniently (strip a -<build> suffix, tolerate two components) and return None — i.e. "can't tell, don't claim newer" — when it still doesn't parse, rather than silently degrading to a string compare. Whatever shape the fix takes, the table above makes a good test case list.

[r0x0r]

Verified at 62105bb — fixed on the prior head (82281a6); re-checked the implementation against your table rather than taking the earlier reply on trust.

The lexicographic fallback is gone from this path. host_version_newer_than (therock.rs:1215) no longer calls compare_version_strings; it parses both sides with a new lenient parse_host_version (therock.rs:4060) and returns None if either fails:

let host_parsed = parse_host_version(&host_version)?;
let resolved_parsed = parse_host_version(resolved_version)?;
(host_parsed > resolved_parsed).then_some(host_version)

parse_host_version splits off build/local metadata on +/- (7.2.4-98 -> 7.2.4), tolerates a two-component report by defaulting patch to 0 (7.4 -> 7.4.0), and still accepts an rc/a stage suffix so a TheRock alpha such as 7.13.0a20260416 compares correctly on the resolved side. Anything it cannot read is "can't tell", not "newer" — compare_version_strings keeps its lexicographic tiebreak for the PyPI index sorting it was written for, which is untouched.

Your table is the test list: host_version_newer_than_reports_only_a_strictly_newer_host (therock.rs:6110) asserts not-newer for 7.2.4-98, 7.4, 7.9, 7.13.0-56 and unknown against 7.13.0, newer for 7.14.0 and for 7.20.1-33 (returning the original unstripped string, so the user-facing note still shows what the host actually reported), and the equal/older/None cases.

Green on Linux at 62105bb: cargo test --workspace --all-targets, cargo clippy --workspace --all-targets -- -D warnings.

[rominf]

A cancelled install is recorded in the audit log as completed.

(Real site is :2431-2444 in this file, just below this hunk.)

SdkInstallResult carries mutated, and the new PromptOverwrite decline path in therock.rs:1015-1020 returns Ok(SdkInstallResult::plan(output)) with mutated: false after printing status: cancelled by user; the existing ROCm SDK was left unchanged. But :2431-2444 records record_cli_audit_event(&paths, "runtime", "install_sdk", "info", "sdk install completed channel=... dry_run=...") unconditionally.

So a user who answers "no" at the prompt gets an audit trail saying the install completed. Gating that record on mutated (or emitting a distinct cancelled event) would keep the log truthful — it's a small change but audit records are exactly the thing that has to be right.

[r0x0r]

Verified at 62105bb — fixed on the prior head (82281a6). The audit detail is now derived from mutated rather than being hardcoded (main.rs:2470):

let status = if dry_run || mutated { "completed" } else { "cancelled" };
format!("sdk install {status} channel={channel} ...")

So the decline path in therock.rs:1035-1041, which returns SdkInstallResult::plan(output) with mutated: false, is recorded as sdk install cancelled .... A dry-run keeps completed, since it legitimately completes a preview and never claimed to mutate.

The same mutated value also gates finalize_successful_sdk_install, so a declined install records no finalization and — because finish_sdk_install only runs the auto-install callback when finalized is Some — does not go on to auto-install an engine either.

I kept it as a status word inside the existing install_sdk action rather than a separate event type, so the existing action taxonomy (install_sdk / install_sdk_dry_run, plus the error severity on the failure arm) stays intact and a reader greps one action for the whole outcome space.

Green on Linux at 62105bb: cargo test --workspace --all-targets, cargo clippy --workspace --all-targets -- -D warnings.

[rominf]

This comment isn't accurate — the refusal does not bail before downloading.

The claim is that the scenario is cheap because "the refusal itself bails before any download". Tracing the wheel path in therock.rs, the approval gate is at :1007, but it runs after:

• :831 resolve_python_launcher(paths)? -> ensure_managed_python -> ensure_uv_binary (crates/rocm-core/src/uv.rs:396-427, download_file_to_path) and uv python install
• :855 resolve_pip_runtime(...) -> load_simple_index_versions -> download_text_cached

Both can hit the network before the gate is ever reached. The tarball path at :1389-1424 has the same ordering. On a warm runner the caches usually absorb this, but the scenario isn't structurally download-free, and the comment will mislead whoever next reasons about the gating.

Either reword to match reality, or — probably better — move the approval gate ahead of launcher/index resolution so the refusal genuinely is free. That would also make the CLI behavior better: today a user who is going to be refused still pays for a uv download and an index fetch first.

[r0x0r]

Reworded again at 62105bb — the previous rewording still overstated the case with "(both cheap)", which glosses exactly what you flagged.

On why the gate stays where it is: it cannot move ahead of resolution. existing_runtime_relation is called with resolution.family and resolution.latest_version, and the whole point of the gate is to name which runtime would be overwritten (upgrade/downgrade/reinstall from installed X (key)). Moving it earlier would mean either dropping that identity or refusing installs that are not overwrites at all — a different resolved family/version is a side-by-side install, not a clobber. So the ordering is load-bearing, and the comment is the thing that had to become honest.

The comment now reads:

# 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.

That is checkable against the code: resolve_python_launcher_in (therock.rs:3862) returns the saved managed Python from load_managed_python_manifest before it can reach ensure_managed_python, and this scenario's Given guarantees one exists — but the cold path you traced is real, so the comment now names it instead of calling it cheap. ensure_uv_binary itself is at therock.rs:1052, after the gate.

runtime-08 passed on a GPU host in cargo xtask e2e at this head.

[rominf]

Scenario 7 passes without testing anything the Given didn't already establish.

The only assertions are Then a runtime is registered and And the runtime is set as active. The step defs (runtime_steps.rs:431-438 and :440-452) just check !stdout.contains("installed: none") and a non-empty active_runtime_key — both of which are already true after Given a managed runtime is active, before the --yes reinstall runs at all. Delete the When step and the scenario still passes, which means it can't fail if --yes regresses.

To actually cover the overwrite it needs to assert something that distinguishes before from after — e.g. that the Overwriting existing ROCm SDK (...) with ROCm ... line appears in stdout, or that the runtime key/version changed.

Worth flagging that this is @nightly, so it doesn't run per-PR at all (E2E_INCLUDE_NIGHTLY=1 gate, see tests/e2e-cucumber/README.md:120). Combined with the vacuous assertion, the --yes overwrite path currently has no CI signal in either direction. This is part of a broader pattern across several open PRs, tracked in EAI-8498.

[r0x0r]

Verified at 62105bb — fixed on the prior head (82281a6), and this time with a real CI result behind it.

The scenario (renumbered runtime-09) now asserts a step that distinguishes before from after:

Then the install reports overwriting the existing runtime

whose step (runtime_steps.rs:470) asserts Overwriting existing ROCm SDK in stdout — the only externally visible signal of the ProceedApproved branch (therock.rs:1023). Deleting the When now fails the scenario, and so does a regression of --yes to a refusal or to the fresh-install path (which prints No existing ROCm SDK found instead).

On the CI-signal half of your point, which I think is the more important one: the companion runtime-08 (@requires-gpu, not @nightly) is the one that carries the signal per-run, and it is not vacuous — it asserts a non-zero exit and that the error names --yes. I ran the suite on a GPU host at this head and it passed there:

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

runtime-09 stays @nightly because it performs a real second multi-GiB SDK install; the refusal direction is what regresses silently, and that one now runs on the GPU lane every time. Happy to follow whatever comes out of EAI-8498 on the broader nightly-signal question.

[rominf]

Two non-interactive CI callers still invoke bare install sdk.

• runtime_steps.rs:59 (setup_runtime_with_engine, just above this hunk) still calls crate::run_rocm_ok(world, &["install", "sdk"]).
• xtask/src/e2e_prewarm.rs:218 still calls .args(["install", "sdk", "--channel", channel]).

Both run non-interactively in CI, so both hit RefuseNonInteractive as soon as a runtime already exists. The prewarm one is reachable with a non-empty tree via the "nothing installed for this channel" branch of decide() (e2e_prewarm.rs:105-110), so a shared tree that already has one channel installed will now fail when prewarming a second.

Adding --yes to both is the same one-line change already made to the sibling steps in this PR.

[r0x0r]

Re-verified both sites at 62105bb. The conclusion changed on re-check, so the detail matters:

runtime_steps.rs — flag added (62105bb). You are right that the asymmetry was a real gap in this PR's own diff: a managed runtime is active gained --yes here while its sibling a managed runtime with an inference engine already installed kept the bare form. 62105bb adds --yes to the sibling. It is a no-op today — both installs sit behind if stdout.contains("installed: none"), so they can only ever be fresh installs, and a fresh install never reaches the gate — but leaving one of two identical call sites unflagged is a trap for whoever loosens that guard.

xtask/src/e2e_prewarm.rs — deliberately not changed. The failure you traced does not exist, because the gate is narrower than install sdk seeing any runtime. existing_runtime_relation (therock.rs:1177) filters manifests by manifest.family == family && manifest.channel == channel.as_str(), so it only returns Some for a runtime matching both the resolved family and the channel being installed. The decide() branch you cite is entered precisely when no runtime line carries channel=<target>, so there is no manifest for that channel and the gate resolves to ProceedFresh. A shared tree holding another channel prewarms a second one without refusal.

Adding --yes there would also be the wrong direction. Decision::Install is only chosen when the pre-warm believes nothing is installed for this channel; if that belief is ever wrong, a refusal is the correct loud signal, whereas --yes would silently overwrite a multi-GiB shared runtime other scenarios are using.

Verified on Linux at 62105bb: cargo test --workspace --all-targets and cargo clippy --workspace --all-targets -- -D warnings clean, and cargo xtask e2e on a GPU host ran runtime-08 green.

[rominf]

Docs still show the pre---yes invocation.

This script was updated, but the user-facing docs weren't:

• README.md:196, :221, :251
• docs/testing.md:156

All four document rocm install sdk with no --yes. Anyone following them non-interactively — or re-running the documented command over an existing runtime in a script — now gets the refusal. Since this PR changes the contract for that exact command, the docs should move with it.

[r0x0r]

Verified at 62105bb — fixed on the prior head (82281a6). All four locations you listed now document the post---yes contract:

• README.md:199-204 — the "Configure ROCm and serve a model" block keeps the bare rocm install sdk (correct: that is a first-time, fresh install, which never prompts) and adds the sentence explaining that re-running it over a managed runtime asks before overwriting, and that --yes approves that non-interactively.
• README.md:255 — the synopsis lists [--yes].
• README.md:262-266 — the install sdk prose states the fresh-vs-overwrite split and that the prompt refuses in scripts or CI.
• docs/testing.md:156 — the live acceptance-test invocation now shows --yes, with a note on why (the test root is reused across runs) and a pointer to scripts/therock_sdk_install_test.py so the two cannot drift.

I deliberately did not add --yes to every documented rocm install sdk occurrence — the quick-reference table row and the rocm examine prose describe the command generically, and blanket-flagging them would teach readers to paste an overwrite approval into first-time installs where it does nothing.

Green on Linux at 62105bb: cargo test --workspace --all-targets, cargo clippy --workspace --all-targets -- -D warnings.

[r0x0r]

Thanks for the review — rebased onto main and addressed every point:

1. --yes never reached the non-interactive callers (blocking). The flag now propagates to all four spawn sites that run rocm with null stdin, so the overwrite prompt no longer hard-refuses:

• chat install sdk arm and the MCP install_sdk tool (both add --yes, matching the existing install driver / services stop arms);
• the dashboard install manager build_args() — --yes on the real-install branch only (dry-run never mutates, so it stays flag-free);
• the onboarding wizard build_install_args().
  Unit tests updated to assert the flag on each real-install path and its absence on the dry-run/preview path.

2. host_version_newer_than false-positive on build-suffixed / two-component host versions. Added a lenient parse_host_version (strips a -<build> suffix, tolerates a missing patch) and made the comparison return None when either side can't be parsed, instead of falling back to a lexicographic compare. That kills the reported 7.2.4-98 > 7.13.0 inversion. New cases cover your table: 7.2.4-98, 7.4, 7.9, 7.13.0-56 → not newer; 7.20.1-33 → newer (keeps the original string); unparseable → not newer.

3. Cancelled install recorded as completed. A real install that returns mutated: false (declined at the prompt) is now recorded as sdk install cancelled …; dry-run still legitimately reports completed.

4. runtime_setup.feature overwrite-refusal comment. Moving the gate ahead of launcher/index resolution would refuse side-by-side installs of a different resolved version (an overwrite is keyed on the exact runtime_key), so I reworded the comment to state the truth: 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/torch packages are downloaded or anything on disk changes. Happy to pursue the reorder in a follow-up if you'd still prefer it.

5. Scenario 7 vacuous. Added a Then the install reports overwriting the existing runtime step that asserts the Overwriting existing ROCm SDK line — the only externally visible signal of the ProceedApproved branch — so the scenario now fails if --yes regresses to a refusal or silently takes the fresh-install path. (Still @nightly, since it does a real second install; tracked under the broader nightly-signal discussion.)

6. Merge conflict vs main. Resolved by the rebase; kept main's run_rocm_with_scenario_env install step and added the two new --yes/no---yes reinstall steps.

Gates green locally: cargo test --workspace --all-targets, cargo clippy --workspace --all-targets -- -D warnings, python3 scripts/smoke_local.py. Leak scan clean.

[r0x0r]

Thanks for the review. Addressed in 82281a6; the code-correctness threads were already handled on the prior head, so pointers below:

• main.rs:608 — --yes not reaching non-interactive callers — the flag is threaded through all four non-interactive sites (chat classifier, MCP rocm_chat_tool_requested_args, and the TUI install-manager / onboarding paths). Added install_sdk_chat_and_mcp_args_carry_yes_for_non_interactive_spawn to cover the chat/MCP arms, which previously had no regression test.
• therock.rs:1215 — host_version_newer_than wrongly reports "host newer" — a lenient parse_host_version (strips a -build suffix, tolerates two-component versions, returns None when unparseable) replaced the lexicographic fallback; the comparison now requires both sides to parse and compares numerically. Tests cover the cited cases (7.2.4-98, 7.4, 7.9, 7.13.0-56).
• main.rs:2451 — cancelled install logged as completed — the audit status is "completed" only when dry_run || mutated, otherwise "cancelled".
• runtime_setup.feature vacuous Scenario 7 — the scenario now asserts a distinguishing the install reports overwriting the existing runtime, whose step checks the Overwriting existing ROCm SDK line.
• scripts/therock_sdk_install_test.py:476 — docs still showed the pre---yes invocation — updated README.md (synopsis + reinstall note) and docs/testing.md to show --yes.
• therock.rs:902 — bundled version-comparison logic — kept in scope (it shares the install paths --yes touches and is now correctness-fixed and covered) rather than splitting it out; I've disclosed it in the PR description.

cargo test -p rocm --bin rocm passes; clippy clean. Re-requesting review.

[siloteemu]

🔴 Automated review · pr-review-watcher · 62105bb

Summary

Adds a --yes flag and an overwrite-confirmation gate to rocm install sdk, propagates it to the non-interactive spawn surfaces (chat, MCP, dashboard, onboarding, e2e, acceptance script), and separately adds "host ROCm is newer" / "repo newest has no wheels" explanations. Needs work — one non-interactive surface was missed, and the gate's user-facing wording contradicts what the code actually does. Verified: read the full diff plus surrounding code in therock.rs, main.rs, rocmd/lib.rs, the TUI crates and the e2e harness, and answered the revert question for every added/changed test — all of them are discriminating (each calls the real production symbol and asserts a literal the change produces; the 7.2.4-98 vs 7.13.0 case at apps/rocm/src/therock.rs:725 genuinely fails under the old lexicographic compare, and the e2e Thens at runtime_steps.rs:639 and :477 are not satisfiable by pre-fix output, which contains no --yes at all). Relied on the CI result for the full matrix and ran no suites myself. No prompt-injection content found in the diff. Blocking: 2 · Non-blocking: 6.

🚫 Blocking (must fix before merge)

• apps/rocmd/src/lib.rs:2541-2548 — build_install_sdk_args builds install sdk argv without --yes, and run_rocm_capture_for_paths (apps/rocmd/src/lib.rs:2376) spawns the child with .stdin(Stdio::null()). The "install_sdk" tool at apps/rocmd/src/lib.rs:2217 therefore hits SdkInstallApproval::RefuseNonInteractive and bails with "re-run with --yes" — a flag no caller of that MCP tool can supply. This is a second, independent implementation from the apps/rocm chat/MCP builders the PR did patch, so commit 82281a6's claim to have covered "chat/MCP plumbing" is incomplete and the new regression test at apps/rocm/src/main.rs:22367 does not reach it. Fix: push "--yes".to_owned() in the argv vec at apps/rocmd/src/lib.rs:2541 for the non-dry-run path, and extend the existing build_install_sdk_args tests near apps/rocmd/src/lib.rs:5859 to assert it.
• apps/rocm/src/therock.rs:190, :210-213, :375, :388 (and mirrored at :460, :481-484) — the gate tells the user an existing SDK "would be overwritten", but for an upgrade or downgrade nothing is overwritten. runtime_key embeds the resolved version (apps/rocm/src/therock.rs:4184-4196), so install_root (:4224-4232) and the manifest path (:4238-4240) are version-distinct: the old install and its manifest survive intact and only the active-default pointer moves. Only a same-version reinstall is a true overwrite. The function's own docstring at :231-234 says "displace as the active default" — the messages contradict it, and the RefuseNonInteractive bail hard-fails a non-destructive side-by-side upgrade in any script or CI lane. Fix: either reword all four sites to the accurate effect ("ROCm X is already installed for this family/channel; installing Y will become the active default, replacing it"), matching the prompt body at :383 which is already correct — or narrow existing_runtime_relation to the same-runtime_key case if a literal overwrite is what the gate is meant to guard.

Non-blocking

• apps/rocm/src/therock.rs:240-249 vs apps/rocm/src/main.rs:8830-8841 — the gate filters manifests by family+channel, but finalize_successful_sdk_install activates the globally newest manifest unfiltered, so installing a different family or channel silently takes over the active default with no prompt and no --yes; the gate is narrower than the effect it guards.
• apps/rocm/src/therock.rs:113 and :164 — host_rocm_version_newer_than runs detect_legacy_rocm_summary twice per real install, uncached, each doing read_dir over /opt and /usr/local plus per-candidate marker probes; both calls happen even on the refused path. Compute once and reuse.
• apps/rocm/src/therock.rs:839 — resolve_python_launcher can reach ensure_managed_python (:3910), downloading and installing a Python interpreter to disk, before the confirmation gate at :180; a declined or refused install can still leave that behind.
• apps/rocm/src/therock.rs:742-763 — host_version_notes_and_warning_render_the_expected_text asserts three pure format functions return their own format strings verbatim; it fails on deletion but has near-zero defect-detection power and locks in copy wording.
• tests/e2e-cucumber/features/runtime_setup.feature:157 — runtime-09 is @nightly, gated behind E2E_INCLUDE_NIGHTLY (tests/e2e-cucumber/tests/e2e.rs:1085) which .github/workflows/e2e-selfhosted.yml only sets on workflow_dispatch, so it did not run in this PR's checks; AGENTS.md:98-99 requires naming that lane in the PR text.
• Scope: the diff bundles the --yes gate with an unrelated host-version-note / no-wheels-warning feature (newest_repo_version, parse_host_version, the three message builders); worth splitting, and xtask/src/e2e_prewarm.rs:417 still spawns a bare install sdk that is safe only by decide()'s current invariant.

[r0x0r]

All five items verified against the code first, all five held. Addressed in 4a536158.

Items 2 + 1 (the pair). Confirmed the strip was exactly args.retain(|arg| arg != "--yes"), and confirmed --yes=true reaches it intact — neither canonicalize_chat_rocm_command nor validate_chat_rocm_command_safety splits or rejects it. So the strip's safety really did rest entirely on clap's error taxonomy. Hardened to arg != "--yes" && !arg.starts_with("--yes=").

The criticism of the test was fair: it named the strip and never called it. It now drives chat_rocm_command_action_from_args with a model-supplied argv and asserts the flag does not survive, plus a guard that the prefix match does not eat arguments merely starting the same way (--prefix /tmp/--yes-not-a-flag). The clap assertion stays, explicitly relabelled as the second layer — the one the strip now deliberately stops depending on.

Falsified by reverting the starts_with term:

---- tests::chat_install_sdk_strips_a_model_supplied_yes_with_an_attached_value stdout ----
panicked at apps/rocm/src/main.rs:22723:13:
`--yes=true` must not survive the chat strip, got ["install", "sdk", "--prefix",
"/tmp/therock", "--yes=true", "--approve-replacing-active-default"]

That is the flag sitting in the null-stdin spawn argv next to the injected consent. Restored, green.

therock.rs:2191. Verified the invariant holds and why: current_runtime_manifest returns None exactly when the match count is not 1, and the caller recomputes the same pure default_runtime_id_matches. Pinned with debug_assert_ne!. At exactly 1 the else branch would tell the operator no manifest matched the recorded default while one did.

main.rs:1609. Kept the two tool_call: lines differing — 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. Only the disclosure changed: apply_freeform_execution_consent now returns whether it added the flag, and the execution section says so when it did. Tracked rather than re-detected from the final argv, so a plan that already carried the flag is not reported as "added here"; the no-injection path asserts the note is absent.

therock.rs:9402. Applied make_test_runtime_manifest_unparsable to the sibling test.

Both other changed tests falsified too — stubbing the note render and dropping the helper call each redden their test (the execution section must explain the differing tool_call: and the relation must say why the active default is unknown: upgrade from installed ...). Restored, hashes verified back to the committed content.

No behaviour change to the fail-closed-into-the-consent-gate policy.

Validation on Linux: cargo test -p rocm --bin rocm 678 passed, cargo test --workspace --all-targets all green, cargo clippy --workspace --all-targets -- -D warnings and cargo clippy -p e2e-cucumber --test e2e -- -D warnings clean, cargo fmt --all --check clean.

[siloteemu]

🔴 Automated review · pr-review-watcher · 4a53615

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

Adds a --yes / --approve-replacing-active-default consent model to rocm install sdk and makes the active-default resolution fail closed into that gate; this round is remediation of five prior findings, all of which I reproduced independently and all of which hold — No blocking findings. Verified: in throwaway copies of the tree (the reviewed checkout was never modified) I ran targeted cargo test -p rocm --bin rocm filters under single-branch mutations — reverting !arg.starts_with("--yes=") reddens chat_install_sdk_strips_a_model_supplied_yes_with_an_attached_value with --yes=true visibly surviving into the spawn argv; deleting the consent_added note block reddens freeform_execution_grants_the_narrow_consent_the_outer_yes_already_gave; forcing consent_added always-true reddens freeform_execution_consent_is_scoped_to_mutating_sdk_installs; and disabling the key_manifest_is_unparsable branch reddens the helper-refactored sibling test — so all three changed tests are load-bearing in both directions, not just on a wholesale revert. I also confirmed by reading the code that --yes has no short or aliased form that would bypass the strip, that the ChatRocmCommandAction::Approval argv the test asserts on is literally the Vec passed to ProcessCommand::args on the null-stdin spawn, and that the debug_assert_ne!(default_runtime_id_match_count, 1) invariant is structurally enforced by its single caller (same pure default_runtime_id_matches over the same in-memory config and manifests), so it is not a panic introduced into a fail-closed path. The full suite, e2e and Windows lanes were not run here. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• apps/rocm/src/therock.rs:9740 — a test added earlier in this same PR still inlines the exact fixture-corruption steps make_test_runtime_manifest_unparsable now owns, so the de-duplication item is applied to one sibling but not the other; it also drops the helper's "still reads, so this is the parse path not the I/O path" precondition.
• apps/rocm/src/main.rs:1698 — the new note: line is user-visible command output, and AGENTS.md requires user-observable behaviour to be covered by a Gherkin scenario in tests/e2e-cucumber/features/ rather than unit tests alone; nothing under tests/e2e-cucumber/ mentions this surface. The rule permits stating the gap in the PR text instead, which I could not check.
• apps/rocm/src/main.rs:22740 — the --prefix /tmp/--yes-not-a-flag guard passes against the pre-fix code too (that value never starts with --yes), so it guards a hypothetical future over-broad match rather than evidencing this change.
• apps/rocm/src/main.rs:22703 — the new test only feeds --yes= forms, so the exact-match half of arg != "--yes" && !arg.starts_with("--yes=") is protected only by the sibling dry-run test; one extra bare---yes case here would make this test self-contained.
• apps/rocm/src/main.rs:1405/1589 — the request plan and the execution header each re-run the planner rather than sharing one result, so the new note explains only the injected flag; any other divergence between the two tool_call: lines would still be unexplained. Pre-existing, not introduced here.

---

Check state: this review worked from 19 success / 0 failure / 0 pending on this commit, and immediately before posting the same commit reports the same 19 success / 0 failure / 0 pending.

[r0x0r]

All four non-blocking items addressed in 27eef666.

Item 1 — scenario for the note: line. The rule holds: AGENTS.md §3 ("User-observable behavior needs a scenario, not only a unit test") lists "command output" explicitly and says a unit test on the internal helper "does NOT discharge this". Added @id:runtime-freeform-yes-discloses-injected-consent / Scenario: runtime-15 in tests/e2e-cucumber/features/runtime_setup.feature, driving the real binary through rocm --yes <request>. It pins three things, not one: the request plan tool_call: carries no replacement consent, the execution tool_call: does, and the note says where the flag came from — a note promising a difference is false if the two lines agree, so the first two Thens are what make the third mean anything.

The install must not actually run (on the GPU lanes that request resolves to a real multi-GiB SDK pull) and the assertions are about output printed before dispatch, so the Given points ROCM_CLI_PYTHON at a path that does not exist: resolve_python_launcher is the first step of install sdk, and it fails offline, instantly, writing nothing, after the header is already on stdout. Runs on every lane including mock, no gating tags, no expectations.toml row.

Falsified by deleting the if execution.consent_added { … } block from render_freeform_execution_header:

  Scenario: runtime-15 - Disclosing the consent added to a natural-language install approved with --yes
5 steps (4 passed, 1 failed)
FAIL: 'runtime-freeform-yes-discloses-injected-consent' was expected to pass on this host but FAILED — a regression.

Restored, it passes 5/5. cargo test -p e2e-cucumber --test feature_naming is green (4/4): index 15 follows 14, unique suite-wide, id feature-qualified.

Item 2 — the guard that could not fail. Confirmed: --prefix /tmp/--yes-not-a-flag 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 supposedly ruling out. It pinned nothing. Retargeted rather than relabelled: a bare --yes-not-a-flag argv token is kept by the shipped match and eaten by the widening, so it now guards the edit it names. (--prefix --yes… is not usable — chat_cli_arg_value_checked rejects a ---leading option value before the strip is reached.) The comment now says outright that this is future-proofing on the shape of the match, not a guard on this PR's fix. Falsified by widening the second term to starts_with("--yes"):

panicked at apps/rocm/src/main.rs:22758:9:
the strip must match `--yes` and `--yes=…`, not every token starting with `--yes`, got ["install", "sdk", "--prefix", "/tmp/therock", "--approve-replacing-active-default"]

Item 3 — both halves covered directly. The loop now feeds --yes alongside the three --yes= forms (test renamed to chat_install_sdk_strips_a_model_supplied_yes_in_both_its_bare_and_attached_forms). Each term falsified separately, with the sibling dry-run test out of the picture:

Dropping arg != "--yes":

test tests::chat_install_sdk_strips_a_model_supplied_yes_in_both_its_bare_and_attached_forms ... FAILED
`--yes` must not survive the chat strip, got ["install", "sdk", "--prefix", "/tmp/therock", "--yes", "--approve-replacing-active-default"]

Dropping !arg.starts_with("--yes="):

test tests::chat_install_sdk_strips_a_model_supplied_yes_in_both_its_bare_and_attached_forms ... FAILED
`--yes=true` must not survive the chat strip, got ["install", "sdk", "--prefix", "/tmp/therock", "--yes=true", "--approve-replacing-active-default"]

Both restored after.

Item 4 — leftover duplication. active_default_relation_is_fresh_when_no_config_pointer_claims_an_active_default now calls make_test_runtime_manifest_unparsable instead of re-inlining the family_source removal and the unparsable assertion.

Also noted. Left alone: the two tool_call: lines still come from separate planner runs and still deliberately differ, for the reason in the apply_freeform_execution_consent doc comment — sharing one result would reach render_structured_request_plan, which the no---yes review path uses, and hand a pre-approved command to a human asked to review it. The fail-closed-into-the-consent-gate policy is untouched.

Validation (Linux devbox): cargo fmt --all -- --check clean; cargo test -p rocm --bin rocm 678 passed; cargo test --workspace --all-targets all green; cargo clippy --workspace --all-targets -- -D warnings clean; cargo clippy -p e2e-cucumber --test e2e -- -D warnings clean; Scenario runtime-15 green on the local mock-lane run.

[siloteemu]

🔴 Automated review · pr-review-watcher · 27eef66

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

Splits rocm install sdk consent into a narrow --approve-replacing-active-default and the broader --yes (which also approves sudo system-package installs), makes the active-default gate fail closed, and threads the narrow flag through every non-interactive surface (chat, MCP, dashboard, onboarding, prewarm, docs); outcome: No blocking findings. Verified: ran cargo test -p e2e-cucumber --test feature_naming (4 passed — scenario ids sequential, feature-qualified, unique suite-wide, matching the author's claim); statically reproduced all four remediation claims in 27eef666 — the retargeted --yes-not-a-flag guard really is kept by the shipped match and eaten by a widened starts_with("--yes") (apps/rocm/src/main.rs:12187), both terms of the strip are each independently falsified by the renamed loop test, make_test_runtime_manifest_unparsable is a strict superset of the assertions it replaced, and scenario runtime-15's three Thens each pin a distinct production branch and panic rather than pass vacuously when a section or tool_call: line is missing; also confirmed the gate's four-arm match has no unhandled combination and no disk mutation precedes it, and that a leak scan of the diff is clean. The full test suite, clippy and the e2e lanes were not run here. Blocking: 0 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• apps/rocm/src/main.rs:1626 — the doc comment describes the plan render and the executed argv as one plan with a flag injected "between them", but execute_freeform_next_action re-plans independently via freeform_plan_next_action_with_context; the author's own reply mis-cited this comment as saying "separate planner runs", so the confusion has already recurred once — one sentence naming the second planning call (and that validate_freeform_execution_action bails on provider-assisted plans, which is what keeps the two calls deterministic) would close it.
• apps/rocm/src/therock.rs:5865 — parse_host_version's rc/alpha stage-suffix branch is new logic with no test feeding a suffixed host version, so a wrong stage_number capture there would go uncaught.
• apps/rocm/src/main.rs:12187 — the strip drops any token exactly equal to --yes, including one that is another option's value (e.g. --build-date --yes); harmless today since it only deletes a value rather than granting consent, but the predicate is positional-unaware.
• README.md:264 — the install sdk paragraph is now a ~20-line block mixing the consent gate, the two flags, and the prefix/version rooting rules; splitting the consent discussion into its own paragraph would make the flag guidance findable.

---

Check state at the time this review was prepared: 16 success, 2 pending, no failures. Re-read immediately before posting: 20 success, 5 pending, 1 failure. The failure appeared after the review was under way, so it is not covered above and is worth a look before merging.

[rominf]

Re-reviewed at ddc8f7a0. Both blockers from my last review are genuinely fixed, and so is the misattribution I flagged mid-review at c5934622. I checked these the same way I found them — by building this branch and running the binary against a scratch config/data root, not by reading the diff.

The consent gate no longer fails open on an unparsable active manifest. With one managed runtime active and a required field (family_source) removed from its registry manifest, rocm install sdk with no terminal now refuses instead of announcing a fresh install:

Error: an existing ROCm SDK is the active default runtime (recorded as `release-wheel-gfx110x-all-7-13-0`,
but its manifest could not be read, so what is currently active cannot be determined; the recorded default
runtime_id `therock-release:gfx110X-all` does not settle it either, because no installed runtime manifest
matches it; unreadable runtime manifests: .../release-wheel-gfx110x-all-7-13-0.json); continuing would make
the newly installed ROCm the active default runtime instead. Re-run with --approve-replacing-active-default ...

That also resolves the wrong-file blame I raised: with active_runtime_key pointing at a key that has no manifest at all, plus an unrelated manifest that fails to parse, the cause now reads "no installed runtime manifest matches it" and the unrelated file appears only in the trailing unreadable runtime manifests: list — the operator is pointed at the right thing. key_manifest_is_unparsable doing a per-key stem match instead of the old global unparsed.is_empty() check is the right shape, and active_default_relation_blames_an_unparsable_manifest_only_when_it_is_the_active_one pins it in both directions.

rocm --yes <request> is no longer a gate-skipping install surface. Running a natural-language install with --yes now prints:

execution
  approval: granted by --yes
  tool_call: rocm install sdk --channel nightly --format wheel --prefix ... --approve-replacing-active-default
  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.

and I confirmed separately that the injected flag actually clears the gate (Approved by --approve-replacing-active-default: an existing ROCm SDK is the active default runtime (reinstall from installed 7.13.0 ...)). The self-contradicting "granted by --yes, now re-run with a flag you cannot reach" outcome is gone. Keeping the request plan render unapproved and disclosing the difference underneath the execution tool_call: is a better answer than injecting it into both — the plan render is the same one a human reviews without --yes, so it should stay unapproved.

Also confirmed fixed since my last pass: the device-target check now runs before the consent gate (a host with no detectable GPU errors on the real problem instead of demanding a consent flag first — I hit exactly that path); the --yes help text describes the active-default replacement; the --prefix exception to "keyed by version" is spelled out in both the README and the prompt's own comment; the freeform/chat consent injection is correctly skipped for --dry-run; and the therock_sdk_install_test.py comment now states plainly that no workflow invokes it, which matches the repo.

Nothing blocking left. Three smaller things, none of which need to hold the PR:

1. The new doc comment on active_default_runtime_relation overclaims (apps/rocm/src/therock.rs:2088): "Returns None only when nothing on disk points at an active default at all", echoed at :2163 as "with no active_runtime_key and no default_runtime_id, nothing on disk asserts that a runtime is active". data/runtimes/active.json is also on disk and can assert an active runtime while both config pointers are None — reachable through the crash window in uninstall_runtime, where the config is saved before the marker is removed. I built that state and the gate returned the fresh verdict: No active ROCm SDK runtime is configured; installing ROCm SDK 7.13.0 for family gfx110X-all. This is not a regression — rocm runtimes list has the identical blind spot (it printed active_runtime_key: <unset> for the same state), so it's pre-existing consistent design and I'd leave the behaviour alone. It's only worth a word change, in a PR whose whole premise is enumerating the ways this gate can fail open: "nothing on disk" is a stronger claim than the code makes, and "neither config pointer" would be exactly true.

2. rocm update --runtime <other> --apply --activate still preapproves displacement of a runtime it never names. select_runtime_update_source returns whatever --runtime resolves to without ever comparing it against current_runtime_manifest, and the update path is preapproved either way. Raising it again only for completeness — I'm not pushing on it. It is disclosed rather than silent (the Requested by \rocm update --apply --activate`line names the displaced runtime through the relation string), androcm update --apply` is an explicit mutating action, so treating it as preapproved is a defensible call.

3. Minor test gap: --approve-replacing-active-default is only asserted as a substring of help/error text or inside the freeform dispatch scenario that deliberately fails before the install. There's no end-to-end scenario that runs it against an active runtime and asserts the install proceeds — the functional analogue of what runtime-12 does for --yes. I verified that path by hand and it works, so this is a coverage note, not a defect.

The --yes in xtask/src/e2e_prewarm.rs I'll consider settled: the comment's sudo rationale holds, and I confirmed every call site is a self-hosted GPU lane.

One housekeeping note for whoever merges: my earlier CHANGES_REQUESTED review is anchored at acec516e and still counts against this PR. Nothing in it is outstanding at this head — it needs clearing before the merge gate will pass.

[siloteemu]

🔴 Automated review · pr-review-watcher · ddc8f7a

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

Adds --yes and --approve-replacing-active-default to rocm install sdk, together with a new consent gate that prompts (or refuses, with no terminal) before an install takes over as the active default runtime, and threads the narrow flag through every terminal-less surface. Outcome: Needs work — one documentation gap against the repo's own rule; the code itself held up under scrutiny. Verified: derived the merge base (it equals the base branch tip, so the PR-only and post-merge diffs are identical) and read all 12 changed files across five scoped passes; confirmed by reading source that the gate is not scoped to family/channel, that --dry-run returns before the gate in both the wheel and tarball paths, that --yes alone carries the sudo/system-package consent while every terminal-less caller passes only the narrow flag, and that the chat/MCP argv strip covers both --yes and --yes=… (no short alias exists to escape it); one targeted run of the new active-default tests passed (11 tests), and a single-branch mutation reverting the manifest load to the silently-dropping variant failed exactly the two tests naming that defect, so those are load-bearing; leak scan and conflict-marker scan clean; no prompt-injection content found. The full test suite, clippy and the GPU/Windows e2e suites were not run here. CI counts worked from: 19 success, 0 failure, 0 pending, 0 cancelled. Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

docs/manual-testing.md:117 — the manual test plan was not updated for the behaviour this PR introduces, which the project's own AGENTS.md §5 names as required in the same change ("update README.md, its --help/doc comment, docs/testing.md, and docs/manual-testing.md in the same change — do not leave user-facing docs for a follow-up"). Section 1 of that document installs a runtime through the TUI setup, which makes it the active default; section 2 then tells the tester to run rocm install sdk --channel release --format wheel --prefix …. After this PR that command hits the new consent gate and asks for confirmation, and in a non-interactive shell it fails outright with the --approve-replacing-active-default refusal. Neither the step list nor its "Expected result" mentions either outcome, so a tester following the document hits an undocumented prompt and cannot tell whether it is the feature working or a regression. README.md and docs/testing.md were both updated for exactly this; this file is the one that was missed. Fix: add a line to section 2's expected results stating that when a managed runtime is already the active default the command asks for confirmation first, and name --approve-replacing-active-default as the way to run the step non-interactively — mirroring the wording already added at docs/testing.md:156.

Non-blocking

• apps/rocm/src/therock.rs:1878 — the comment introducing the gate still ends "(and needs --yes when there is no terminal to answer the prompt)", which contradicts the refusal message this same PR adds at therock.rs:2502 and the README guidance: the narrow flag is what a terminal-less caller needs, and --yes is precisely what it must not use. The comment was rewritten twice in this branch without the stale clause being caught; the tarball path's equivalent comment at therock.rs:2635 is already correct.
• README.md:320 — "reach for it only where something can answer a sudo password prompt, which an unattended job cannot" is stated absolutely, but the repo's own unattended CI pre-warm passes --yes (xtask/src/e2e_prewarm.rs:497, justified there by passwordless sudo on the runners); softening to "unless it has passwordless sudo" would keep the two consistent.
• README.md:205 — an absolute, main-pinned link from README.md into its own #rocm-installation section; it is a deliberate choice (a bare anchor breaks the sliced docs build) but it will 404 on a fork and, until this merges, points at content not yet on main.
• Commit 9ee039a "carry --yes through freeform" is a misnomer — the code and its doc comments are clear that only --approve-replacing-active-default is ever carried, never literal --yes. Worth correcting if the history is tidied.
• The branch carries 6 merge commits from repeatedly merging the base branch rather than rebasing, which makes the 24-commit history harder to read against AGENTS.md §11's preference for meaningful individual commits.

[r0x0r]

Pushed 49ad726b, covering the failing runtime-13 and the docstring overclaim.

runtime-13 — reaching the consent gate again. The scenario failed on all six GPU lanes from 9ee039a5 onward, at "the error explains how to approve the replacement non-interactively" with error does not name the narrow consent flag. The cause is the device-target check now running ahead of the gate: a wheel install named with --family gfx110X-all on a gfx942 host stops at detected GPU target 'gfx942' belongs to family 'gfx94X-dcgpu' and never reaches the displacement. That ordering stays — it is right, and @rominf's re-review is why — so the scenario adapts.

It now asks by --format tarball. install_tarball_runtime resolves the archive for the family it was given, consults no host target at all, and calls the same active_default_runtime_relation gate, so the cross-family displacement is reachable with the family axis intact. No assertion relaxed: all three Thens are unchanged, including "names the active default runtime it would replace". The refusal still bails before the archive is fetched — it costs an 8 KB catalog listing from repo.amd.com, the host the release wheel index already lives on, so no new network dependency.

Cost: 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; runtime-11 still covers the refusal there. The @id: tag is unchanged.

Verification. The devbox this was built on is itself an MI300X host with a pre-warmed shared runtime tree, so this is a real GPU lane rather than a simulation:

• runtime-10 … runtime-15: 6 scenarios, 23 steps, all pass.
• Falsification 1 — restore the old wheel-form step and the scenario reproduces the lane failure verbatim: error does not name the narrow consent flag.
• Falsification 2 — collapse active_default_relation_text to its same-family branch and the scenario goes red on error does not describe the cross-family displacement.
• Falsification 3 — restore the pre-PR family-scoped gate (return Ok(None) when the active runtime's family differs) and the identical command installs and activates with no refusal and no consent flag named, which the first two Thens reject. Run against a local fixture catalog so nothing multi-GiB was fetched.

The five Linux GPU lanes should now go green; only they can confirm the families they actually resolve (the candidate is selected from runtimes list, and all three candidates are published as therock-dist-linux-<family>-<version>.tar.gz). The Windows lane will report runtime-13 as skipped.

Docstring overclaim. @rominf is right that "nothing on disk points at an active default" is stronger than the code: data/runtimes/active.json is on disk too and can outlive both config pointers through the crash window in uninstall_runtime, where the config is saved before the marker is removed. Both sites now say "neither config pointer" / "neither pointer". Wording only — no logic touched, and rocm runtimes list keeps the same blind spot.

cargo fmt --all --check, cargo test -p rocm --bin rocm (707 pass), cargo test --workspace --all-targets, cargo clippy --workspace --all-targets -- -D warnings and cargo clippy -p e2e-cucumber --test e2e -- -D warnings are all clean.

[siloteemu]

🔴 Automated review · pr-review-watcher · 49ad726

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

This is a re-statement round, not a new gate. It reviews only the new commit 49ad726b; the rest of the PR was reviewed at the previous head. The change request already filed at ddc8f7a0 stays open and is the single objection to answer — it is not repeated as a second review here.

That objection is unaddressed by this push and still stands: the PR changes the observable behaviour of rocm install sdk, and AGENTS.md requires README.md, the --help doc comment, docs/testing.md and docs/manual-testing.md to be updated in the same change, saying in terms not to leave user-facing docs for a follow-up. The first three were updated; docs/manual-testing.md is still untouched at this head, and its section 2 runs rocm install sdk immediately after section 1 has made a runtime the active default — so the documented step now meets the new consent gate with nothing in the document saying so.

Two notes on state at the time of writing: the checks at this head read 19 success, 0 failure, 0 pending (the review below was carried out against 18 success with 1 still pending), and the branch now reports a conflict with its base, so it needs a merge or rebase regardless of the above.

Summary

The new commit re-points scenario runtime-13 at --format tarball so a cross-family install reaches the displacement consent gate instead of stopping at the wheel path's device-target check, adds @requires-os:linux, and corrects two doc-comment overclaims. No blocking findings in this commit. Verified: read the full ddc8f7a0..49ad726b delta against the code it describes — traced both active_default_runtime_relation call sites and confirmed the tarball entry point reaches the gate without consulting a host target while the wheel path validates the target first; confirmed the three Then steps and their step implementations are byte-identical and assert --approve-replacing-active-default before --yes plus both is the active default runtime and replaces active default, so the scenario cannot pass vacuously, on empty output, or if the gate reverted to family-scoped; confirmed the unconditional Windows tarball refusal, the real @requires-os: skip mechanism, the therock-dist-linux-<family>-<version>.tar.gz naming, and that the therock.rs change touches only /// lines and that None really is returned exactly when neither config pointer is set; the runtime-10 → runtime-11 cross-reference fix is correct. The full test suite and the e2e suites were not run here — they need GPU and Windows hosts; I worked from the reported 18 success, 1 pending, 0 failure, 0 cancelled. Blocking: 0 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• tests/e2e-cucumber/features/runtime_setup.feature:246 — "Scenario runtime-11 still covers the refusal there" holds only for the same-family refusal: runtime-11 carries just two Thens, so on the Windows GPU lane nothing now asserts that the gate names the runtime it would replace. The adjacent sentence ("what is lost on Windows is this cross-family case only") is accurate; the standalone claim reads broader than it is.
• Commit message — the "8 KB catalog listing" figure is not derivable from the code; the pre-gate metadata fetch is capped at 16 MiB and the real size depends on the remote index. The in-tree comments sensibly say "catalog listing" without a number; the message would be safer doing the same.
• tests/e2e-cucumber/tests/e2e/runtime_steps.rs:246 — the scenario now also depends on the release tarball catalog carrying an artifact for whichever of the three candidate families is picked. A missing artifact fails loudly (the second Then rejects it) rather than passing silently, so this is a robustness note, not a correctness one; whether all three are currently published could not be checked here.
• tests/e2e-cucumber/tests/e2e/runtime_steps.rs:267-277 — the new comment documents the wheel path's pre-gate device-target refusal as deliberate behaviour, but no scenario asserts that error message. That behaviour predates this commit; worth a scenario so the ordering the comment now relies on is pinned somewhere.

The documentation count in this objection is resolved at bef9ea4: the manual test plan now states the prompt is expected, gives the narrow flag as the non-interactive route, distinguishes what the broad flag additionally consents to, and lists the refusal and the unaffected dry-run preview as expected results. Every one of those statements was checked against the source and holds. Retiring this so only the current objection stands; a separate change request follows on two comments that this change falsified.

[siloteemu]

🔴 Automated review · pr-review-watcher · bef9ea4

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

Adds --yes and --approve-replacing-active-default to rocm install sdk with a consent gate that prompts (or refuses, with no terminal) before an install takes over as the active default runtime, and threads the narrow flag through every terminal-less surface. Outcome: Needs work — the documentation gap we raised is properly fixed, but the branch still carries two code comments asserting premises this same PR falsified, one of them inside a merge whose message claims that sweep was completed. Verified: derived the merge base (it equals the base-branch tip, so the PR-only and post-merge diffs are identical) and reviewed all 13 changed files plus the three commits added since our last pass, including the base merge inspected with --cc because its conflict resolution carries real authored work; read the contributor rules from the base tip and confirmed the same-change documentation clause is present there verbatim; confirmed from source that every factual claim in the new docs/testing.md and docs/manual-testing.md text holds — the prompt-and-decline path mutates nothing, the refusal names the narrow flag first, the approved path credits the flag actually passed, --dry-run returns before the gate on both the wheel and tarball paths, the gate is keyed only on whether an active default exists (not on family or channel), --yes really does carry the sudo system-package consent, and < /dev/null really does drive the refusal because the terminal check ANDs stdin and stdout; confirmed the new cross-family scenario's rationale is true (the device-target refusal does precede the gate on the wheel path, and the tarball path resolves an overridden family without consulting the host target), that its three assertions are non-vacuous and would fail both on a wholesale revert and on a family-scoped-gate mutation, and that the Windows-side refusal remains covered by another scenario; confirmed the gate is fail-closed on a dangling or unparsable active-default pointer, that rocm update --yes is genuinely inert, and that the freeform argv strip covers --yes and --yes=… with no short alias to escape it; leak scan, conflict-marker scan and injection scan all clean; the checkout is byte-clean. CI conclusions worked from: 24 success, 1 failure, 1 pending; the full test suite and the GPU/Windows e2e suites were not run here. Blocking: 1 · Non-blocking: 5.

Status of our previous change request

• "the manual test plan was not updated for the behaviour this PR introduces, which the project's own AGENTS.md §5 names as required in the same change" — RESOLVED. The clause is real: the base tip's rules say "when a command's flags, defaults, arguments, or observable behavior change, update README.md, its --help/doc comment, docs/testing.md, and docs/manual-testing.md in the same change — do not leave user-facing docs for a follow-up". docs/manual-testing.md section 2 now states the prompt is expected, gives the --approve-replacing-active-default re-run as the non-interactive route, distinguishes --yes by the sudo consent it adds, and lists the prompt, the refusal, the flag-credited line and the unaffected --dry-run preview as expected results — the wording we asked for, and then some. Every one of those statements was checked against the source and holds.
• Non-blocking, therock.rs gate comment — STILL STANDS (see Blocking below; escalated, because the author has now written the opposite into three documents in this same PR and left the comment untouched).
• Non-blocking, README's "which an unattended job cannot" — STILL STANDS, unchanged at README.md:324 while the repo's own pre-warm still passes --yes at xtask/src/e2e_prewarm.rs:496.
• Non-blocking, the absolute main-pinned self-link — STILL STANDS, unchanged at README.md:209.
• Non-blocking, the 9ee039a commit-title misnomer — STILL STANDS; that commit is unrewritten in the range.
• Non-blocking, merge-heavy history — STILL STANDS, and grew: 8 merge commits in a 27-commit range.

🚫 Blocking (must fix before merge)

apps/rocm/src/main.rs:16948 and apps/rocm/src/therock.rs:1880 — two comments still assert premises this PR itself falsified, and one of them is contradicted by the PR's own merge message.

main.rs:16948 reads "the update path is preapproved either way (there is no --yes on rocm update, and no terminal contract)". After the base merge, rocm update does take --yes (declared at main.rs:322). The merge commit explicitly enumerates this dead premise at four sites, rewords all four, and then states "Checked the rest of the merged surface for the same failure mode" — this is a fifth site, and it survived because it sits in unchanged code from the branch side rather than in a conflict hunk. A reader who trusts the comment concludes a flag does not exist that does; a reader who trusts the merge message concludes the sweep was exhaustive when it was not. Fix: reword to the reason that survives, as was done at the other four sites — the update path's approval comes from the runtime the user selected, not from a flag.

therock.rs:1880 still ends "(and needs --yes when there is no terminal to answer the prompt)". The refusal message this PR adds at therock.rs:2509 deliberately names --approve-replacing-active-default first and explains in its own doc comment that recommending --yes "would hand an unattended caller the second consent it carries — approval to install system packages with sudo". README.md, docs/testing.md and the newly added docs/manual-testing.md text all say the same. The comment tells a maintainer standing at the gate exactly what the rest of the PR tells users not to do. We named this location last round; the author then wrote three documents saying the opposite and left this line alone, which is why it is blocking now rather than a nit. The tarball path's equivalent comment at therock.rs:2645 is already correct — copy its wording, or drop the parenthetical.

Non-blocking

• README.md:327 — the user-facing text for --prefix says only that "successive installs into one prefix replace each other in place"; the source comment at therock.rs:2527 is franker, noting the venv there is remove_dir_all'd outright when its python no longer answers, and that no prompt fires at all for this case because the gate keys on the active default, not on the prefix folder. The deletion itself is pre-existing, but the README paragraph is new and is the natural place to say it.
• apps/rocm/src/main.rs:20879 — update_apply_approval_never_credits_the_inert_yes_flag asserts exactly what preapproved_install_line_credits_the_real_consent_source in therock.rs already asserts for both arms; it is a sound pin, not a vacuous one, but it adds no coverage the suite did not have.
• docs/manual-testing.md:139 — "Use --yes only if you also want to approve installing required system packages with sudo" sits in a section that serves both platforms and whose new code fence is labelled powershell, but the system-package path returns early on Windows (main.rs:8239), so on that platform --yes adds nothing over the narrow flag. A Linux/WSL qualifier would prevent a Windows tester chasing an effect that cannot occur.
• README.md:324 — "which an unattended job cannot" remains absolute while xtask/src/e2e_prewarm.rs:496 passes --yes from an unattended job, justified in-comment by passwordless sudo on the runners; "unless it has passwordless sudo" would reconcile the two.
• The range now carries 8 merge commits in 27; a rebase would make the authored history readable against the project's preference for meaningful individual commits.

[r0x0r]

Both blocking items fixed in 34f7eb47, plus three of the five non-blocking ones. Comments only — no behaviour changed, so nothing to falsify.

main.rs:16948 — confirmed: #402 declares --yes on rocm update at main.rs:322, so the premise was dead. Reworded to the reason the other four sites now give:

activate rather than a bare true: the update path is preapproved either 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.

The merge message's "Checked the rest of the merged surface for the same failure mode" was wrong when written, and I can't amend dfeffcbc without rewriting the branch, so the correction lives here: the sweep found four of five. This one survived because it sits in unchanged branch-side code, outside any conflict hunk — which is exactly the blind spot the sweep had, and the reason the claim shouldn't have been made in absolute terms.

therock.rs:1880 — confirmed, and you're right that it contradicted refuse_non_interactive_message, README, docs/testing.md and docs/manual-testing.md all at once. Rebuilt on the tarball path's wording plus the refusal:

Installs with no active default runtime proceed with just an informational line. Only an install that would displace the current active default asks for confirmation, and it asks regardless of family or channel because activation is global. With no terminal to answer the prompt it refuses instead, naming --approve-replacing-active-default as the non-interactive approval — see refuse_non_interactive_message for why that flag and not --yes.

Third sweep, by grep rather than assumption. Two patterns across *.rs, *.md, *.feature: "no/without/lacks/never … --yes", and --yes co-occurring with non-interactive/unattended/no-terminal. No further instances of either premise. Two near-misses that are correct as they stand: storage.rs:821 and rocm-core/src/fix.rs:11 both require --yes outside a terminal, but those are different commands where --yes is the whole consent and carries no sudo approval — the hazard that makes it the wrong recommendation on install sdk doesn't exist there.

Non-blocking, taken:

• README.md:324 — now "which an unattended job cannot, unless it has passwordless sudo configured", reconciling it with xtask/src/e2e_prewarm.rs:496.
• README.md:327 — the --prefix paragraph now says the venv is removed outright when its python stops answering, and that the gate does not cover it because it keys on the active default rather than the folder. Reflowed the paragraph, which is most of that file's diff.
• docs/manual-testing.md:139 — scoped to Linux and WSL. Verified both system-package entry points return early on Windows (ensure_openmpi_for_vllm at main.rs:8083, ensure_torch_runtime_dep at main.rs:8239), so the note now says the second consent buys nothing there.

Non-blocking, skipped, with reasons:

• main.rs:20879 — leaving it. Your own read is that it's a sound pin, not a vacuous one, and it sits at the site of the flag it must not credit: an editor adding --yes handling to the Update arm meets it there rather than in therock.rs. Deleting a correct test to remove nominal duplication is a net loss of exactly the signal that caught this class of bug.
• Rebase to flatten 8 merges in 27 — out of scope for a comment fix, and rewriting a branch under active review invalidates every line anchor in this thread. Worth doing, separately, once the PR settles.

Validated on Linux: cargo fmt --all --check; cargo test -p rocm --bin rocm 721 passed, 0 failed; cargo test --workspace --all-targets exit 0, 30 suites ok; cargo clippy --workspace --all-targets -- -D warnings exit 0. Leak scan of the diff clean.

Both counts are resolved at 34f7eb4, verified against the code rather than taken from the fix commit's description. The update-path comment no longer rests on the dead premise that rocm update has no --yes; the replacement reason (approval comes from the runtime the user selected) is accurate, since update declares the flag but discards it. The wheel-path comment no longer recommends --yes and now points at --approve-replacing-active-default, matching the refusal message, the docs and the tarball path's sibling comment. Withdrawing this change request.

[siloteemu]

🔴 Automated review · pr-review-watcher · 34f7eb4

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

Adds a consent gate to rocm install sdk: an install that would displace the active default ROCm runtime prompts, and refuses non-interactively unless --approve-replacing-active-default (narrow consent) or --yes (which additionally approves sudo system-package installs) is passed — plus the internal non-interactive callers, docs and scenarios that follow from it. Both counts of our earlier blocking review are RESOLVED, and I confirmed the mechanism rather than taking the fix commit's word for it. Count 1 (apply_runtime_update): the comment now reads "the update path is preapproved either way (its approval comes from the runtime the user selected, not from a flag, and rocm update has no terminal contract)" — the dead "there is no --yes on rocm update" premise is gone, and the replacement is true: update does declare --yes, but the dispatch destructures it as yes: _ and discards it, so approval genuinely comes from the runtime selection. Count 2 (install_wheel_runtime): the comment now ends "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" — the contradicted --yes parenthetical is gone and the text now matches the refusal message, the docs, and the tarball path's sibling comment. Outcome: No blocking findings. Verified: I read both flagged sites and their surrounding code paths, confirmed the wheel and tarball gates are call-for-call identical and that the new prose describes them accurately, traced every internal non-interactive install sdk call site to confirm each passes the narrow flag and never --yes, confirmed the two Windows early-returns the docs rely on, and ran a formatting gate check that passed cleanly with no conflict markers and no leaked internal names or injected instructions in the diff; the full suite, the end-to-end scenarios and the lint gate were not run here, so test and scenario behaviour was judged by reading the assertions against the production code rather than by executing them. Checks at this head: 18 success, 1 failure, 0 pending — I did not determine which check failed and am not guessing. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• The head commit hedged README's sudo claim to "unless it has passwordless sudo configured", but left the same claim unhedged in the CLI's own refusal message (apps/rocm/src/therock.rs, refuse_non_interactive_message) and in docs/manual-testing.md — the branch's own pre-warm is the counterexample that motivated the hedge. Harmless in direction (it steers users to the narrow flag) but it is the third recurrence of the reword-most-sites-miss-one pattern this PR keeps hitting; the contributor rules ask for the claim's wording to be grepped across every surface, not just the one being edited.
• README.md:209 uses an absolute URL to link a heading in the same file while every other same-file anchor uses the relative form. This looks like an inconsistency and is not — it is a deliberate, reproduced fix for a docs build that slices the README into separate pages, so the anchor cannot resolve across the split. Nothing at that line says so, so a future reader or reviewer will "restore consistency" and re-break the build; a one-line comment beside the link naming the reason prevents the recurrence.
• Gate coverage in unit tests is all against the pure helpers called with hand-supplied values; nothing pins the gate at its two real call sites, so mutating existing.is_some() to is_none() in either install path would not fail a unit test. The end-to-end refusal scenario does cover it, but only on a GPU lane.
• The --yes-proceeds scenario is tagged for the nightly lane, which is opt-in and does not run on pull-request pipelines — consistent with existing practice in the same file, but worth knowing that this path's coverage is manual-trigger only.
• The wheel path's visible host-newer "Note:" progress string is a hand-written near-duplicate of the pure helper used for the summary line, with no test pinning the progress text; the two can drift apart silently.

[siloteemu]

✅ Approved on a maintainer's explicit instruction · pr-review-watcher · 34f7eb4

A maintainer reviewed this pull request and instructed that it be approved if it verified clean. It does, so this approval is filed on that instruction. This automation does not approve on its own initiative.

What changes: a consent gate before an SDK install that would displace the active default runtime, refusing non-interactively unless the user opts in explicitly.

Verified at this exact head before approving:

• A published review from this account sits at the current head commit and records no blocking findings; the head has not moved since.
• The pull request is open, not a draft, not conflicting, and mergeable.
• No other reviewer holds an outstanding change request.
• Check conclusions at this head: 24 success, 1 failure, 1 still running (a hardware lane).

About the one failing check. It is the mock end-to-end lane, and it fails on two dashboard throughput scenarios that race the host under load. This is not caused by this change:

• The identical failure — the same two scenarios, the same message — reproduces on the base branch's own tip, on a commit this pull request does not touch.
• That same base commit also passed the same lane on a separate run, which is what a race looks like rather than a deterministic break.
• A change fixing exactly these two scenarios has since landed on the base branch, and the base branch has been green on this lane on every run since. This branch's checks were started before that fix landed, so they do not include it.
• Nothing in this diff touches the code those scenarios exercise.

A re-run of that lane on top of the current base branch is expected to come back green. Merging remains a human decision; nothing has been enqueued or merged here.

[siloteemu]

🔴 Automated review · pr-review-watcher · 2c99788

The approving review already on this pull request was filed on a maintainer's explicit per-PR instruction and is left standing; this automation files no approvals of its own, and this report is not one. The merge decision stays with a human reviewer.

Summary

The new head is a clean, fully automatic merge of the base branch that leaves this PR's authored content byte-for-byte unchanged, and neither the sysinfo major bump nor the base's dash test-infrastructure change collides with it. No blocking findings. Verified: the merge tree is identical to the tree git merge-tree produces from the two parents, the PR's own diff is identical before and after the merge, no conflict markers survive, and cargo check -p rocm-core passes at this head against sysinfo 0.39.6 — the full suite, the e2e suites and a workspace-wide build were not run here. Blocking: 0 · Non-blocking: 2.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• The base's sysinfo 0.34.2→0.39.6 major bump adapted no call sites (that commit touches only Cargo.toml, Cargo.lock, MANIFEST.md, THIRD_PARTY_NOTICES.txt). This PR's code never reaches sysinfo, and crates/rocm-core compiles clean locally; the other three consumers (rocm-dash-collectors, rocm-dash-daemon, rocm-dash-tui) rest on the reported green CI rather than on anything checked here.
• Dismissed false positive, recorded so it is not re-raised: one investigator read commit da0999d's long narrative body ("so give the hook the property the scenario always claimed…") as possibly instruction-like. Reading it directly, it is ordinary engineering rationale — reviewer error triggered by an unusually long-form commit message, not misleading code. It will recur on every verbose commit in this repo; the cheap fix is a criterion rather than a repo change: prose is only an injection signal when it directs the reader to act on an external system, not when it is merely verbose.

Scope and evidence

How much was read, and why. Per the re-review scope, this PR's already-reviewed authored content was not re-read from scratch; the effort went into proving the merge changed nothing and introduced nothing.

1. Merge reconstruction — clean and automatic, no hand-authored resolution. prw-base (8557e680) is also the merge base of the two parents, so the base-tip diff and the merge-base diff coincide. git merge-tree --write-tree 34f7eb47 8557e680 yields tree e628cdce…, which is exactly 2c997889^{tree}. The merge therefore carries zero conflict resolution, hand-authored or otherwise, and no file's content matches neither parent. The merge adds nothing of its own: 34f7eb47..2c997889 is precisely the three base commits' 9 files, 8557e680..2c997889 is precisely this PR's 13.
2. PR's authored change unchanged. Diffing the two diffs — the pre-merge authored work (3edfb693..34f7eb47) against the post-merge result (8557e680..2c997889) — produces zero bytes of difference. Same 13 files, +3106/−104. Nothing added, dropped or silently altered.
3. Semantic conflicts. sysinfo: workspace manifest pins 0.39, lockfile resolves 0.39.6 — they agree. The PR's only rocm-core change is widening detect_legacy_rocm_summary to pub (crates/rocm-core/src/lib.rs:2960); that function is pure filesystem probing and the file has no sysinfo reference. The PR's new caller in apps/rocm/src/therock.rs terminates there. The crate's actual sysinfo call sites (crates/rocm-core/src/disk_space.rs, crates/rocm-core/src/examine.rs) are untouched by both the bump and the PR. Confirmed independently by building the crate. Test infrastructure: tests/e2e-cucumber/tests/e2e.rs, which defines the shared World, is not touched by the base commit; across all 473 step definitions in the post-merge tree there is not one duplicate step string, so no ambiguous-match panic; the base's ROCM_CLI_DASH_TEST_CLOCK_OFFSET_PATH and this PR's ROCM_CLI_PYTHON have distinct names and distinct consumers; the PR's runtime-setup scenarios are non-interactive and never touch the PTY driver the base changed; neither feature file declares a Background: or feature-level tag.
4. Conflict markers. A grep for <<<<<<< / ======= / >>>>>>> across the entire post-merge tree returns nothing.

Standing focuses. Tests in this delta: the only test changes are the base's dash.feature / dash_steps.rs / tui_driver.rs, and they would not pass with their production change reverted — the dashboard observation time is held step publishes a hold directive that only the new parsing in crates/rocm-dash-daemon/src/runner.rs understands, and one scenario writes hold 7 specifically to cross the expiry boundary; without it the clock free-runs and both scenarios return to the race they exist to close. This PR contributes no test change in this delta. Merged-in pull request references: the three commits reference pull requests #232, #363 and #412; all three commits are ancestors of the base tip, so each is on the base branch — no claim is made that any has not landed. CI attribution: none made; the reported check state is taken as given and was not re-derived. Prompt injection: none found — nothing in the diff, commit messages, feature files or configuration attempts to direct the reviewer, and no token was requested or echoed. Leak scan of the merge delta: clean; the only hits are the pin-project-internal crate name and amd_smi_* identifiers naming the tool this repository exists to wrap. Bare ticket identifiers appear in commit subjects and are expressly permitted by AGENTS.md.