[juhovainio]

Summary

Addresses part of the work tracked in #180 (see Scope below for what's left there). amdgpu-install is being retired: legacy amdgpu-install/ packages on repo.radeon.com are frozen and won't move to the new amdrocm-* multi-arch/multi-version scheme, so any rocm-cli flow depending on it breaks on ROCm 10.0. This does a hard removal (no deprecation period) per the issue.

• Converts dnf_driver_plan() and sles_driver_plan() (apps/rocm/src/driver_install.rs) to the same repo-native flow apt_driver_plan() already uses: import the AMD GPG key, write a package-manager repo file pointing at repo.radeon.com/amdgpu/<version>/..., then install amdgpu-dkms directly — instead of downloading and running the amdgpu-install RPM. SLES commands run under zypper --non-interactive.
• The default driver source moves from a pinned 7.2.4 to the floating alias latest, resolved into the repo URL at plan time (repo.radeon.com/amdgpu/latest/...). The old ROCM_CLI_AMDGPU_VERSION / ROCM_CLI_AMDGPU_PACKAGE_RELEASE overrides are retired: while either is still set, every Linux driver plan is refused (policy retired_driver_version_override) and names the replacement, so a stale pin is never silently turned into latest; a new ROCM_CLI_AMDGPU_DRIVER_VERSION takes an amdgpu driver release (e.g. 30.10), not a ROCm version.
• Adds a ubuntu_native_archive guidance arm for Ubuntu 26.04, where the amdgpu kernel driver ships in-tree with the kernel and only the userspace stack needs apt install rocm.
• Sanitises the os-release ID, VERSION_ID and codename fields read from the host before interpolating them into shell commands, falling back to a static codename_for_version table on anything outside the allow-list. ID_LIKE is only matched against, never interpolated, so it is not filtered.
• examine.rs: renames the install-method marker set and reported value from "amdgpu-install" to "repo-native". This also fixes a pre-existing mislabeling bug — rocm-cli's own apt-based install dropped the same marker files and was being misreported as amdgpu-install. Also adds SLES zypp repo paths (previously undetected), renames the undetectable runfile/tarball fallback bucket to "runfile-or-tarball", and serializes the parsed ID_LIKE field as distro_id_like in examine --json.
• diagnose.rs/fix.rs: drops the dead amdgpu-install upstream-tracker arm, repoints stale-repo/kernel-mismatch fix guidance at rocm install driver --dkms, and reworks the fix-12 broken-installer check to detect "repo-native" and produce a package-manager-agnostic reset+reinstall fix (fix-12-installer id kept stable). fix-7's stale-repo-file recipe is reworked to build its command list per detected package family instead of a single hardcoded apt list, and fix-12's quarantine step now only lists the repo files examine actually saw on this host (falling back to a commented generic example when none were seen).
• openmpi.rs: widens PackageManager, its as_str(), and resolve_package_manager() from private to pub(crate) so diagnose.rs (same crate, different module) can reuse them for its own repo-family detection instead of re-deriving the logic. Correction from an earlier revision of this description: apps/rocm's driver_install.rs lives in a separate crate and never consumes this enum; it has its own independent family detection.
• New e2e scenarios: examine-29 (renamed install-method values) and driver-install-04/05, which are new in this PR and cover only the Ubuntu apt and 26.04 native-archive arms (they return without asserting on a non-Ubuntu host). dnf/zypper/SLES plans and the changed rocm diagnose/rocm fix output for fix-3/fix-7/fix-12 have no Gherkin scenario: e2e runs rocm against the real host, and there is no RHEL/SLES lane or simulated-host step for diagnose. They are covered at the unit level (driver_install.rs plan tests, diagnose.rs/fix.rs tests, and a drift guard that checks the diagnose support matrix against the real plan builder).

No changes needed in apps/rocm/src/therock.rs (already had no amdgpu-install references, matching the issue's note).

Scope

This PR does not cover all of #180's acceptance criteria, so #180 should stay open for the remaining work:

• Repo-native install still targets repo.radeon.com/amdgpu/..., not repo.amd.com's amdrocm-* scheme.
• No "requesting the removed path returns a clear message" behavior.
• Verification against ROCm 10.0 has not been done (no 10.0 build available in this environment).

Breaking change

• ROCM_CLI_AMDGPU_VERSION / ROCM_CLI_AMDGPU_PACKAGE_RELEASE now refuse every Linux driver plan until unset; set ROCM_CLI_AMDGPU_DRIVER_VERSION instead.
• rocm install driver --yes on a supported: false plan now exits non-zero.

rocm examine --json renames two rocm_install_method values: "amdgpu-install" becomes "repo-native", and "tarball-or-other" becomes "runfile-or-tarball". Correction from an earlier revision of this description: this isn't unconsumed in-tree — diagnose.rs's is_repo_native_installed reads rocm_install_method directly (including via rocm remote doctor), and this PR updates it for the new values. There's no deprecation window for the wire value itself, though; any external tooling matching the old examine --json strings needs updating. There's no CHANGELOG in this repo, so this note is the durable record.

The default driver source also moves from pinned 7.2.4 to the floating latest alias (see Open questions below).

Open questions

• Pinned vs. floating driver default: the plan a user approves names latest rather than a concrete version, unlike the sibling ROCDXG flow which resolves its alias to a concrete build at plan time. Flagging this rather than unilaterally picking pinned-vs-resolve-at-plan-time — feedback welcome on which to restore.
• fix-12's applicability check: it still fires on dpkg/rpm keyword hits for any "apt"/"dnf"-reported host, which also matches distro-packaged ROCm (e.g. the new Ubuntu 26.04 native-archive arm) — that part is still a judgment call on what "applies" should mean here, not a mechanical patch. Its verify step, however, no longer just checks for the repo marker file: it now queries the targeted amdgpu-dkms package's actual dpkg/rpm status, confirms DKMS reports the module built, and checks rocminfo's own exit code.
• RHEL/SLES repo path layout (dnf_repo_version_path): built against the rhel/<ver> / el/<ver> convention; I could not verify this against a live repo.radeon.com/amdgpu/<ver>/ listing in this environment.

Test plan

• grep -rn "amdgpu-install|amdgpu_install|AMDGPU_INSTALL" apps/rocm/src returns zero matches (crates/rocm-core still accepts amdgpu-install as a legacy alias in is_repo_native_installed, with a test, so a host installed that way is still recognised as repo-native)
• cargo test -p rocm (incl. driver-plan tests covering RHEL/Oracle/Rocky/SLES fixtures, a valid version-override reaching the URL, a hostile-codename fallback, and gpgcheck=1 presence for dnf/zypper)
• cargo test -p rocm-core
• cargo build --workspace
• cargo fmt --check
• cargo xtask catalog (doctor-catalog.json regenerated)
• cargo xtask e2e (e2e-cucumber suite, incl. the new driver-install-04/driver-install-05 scenarios, which correctly skip on this WSL sandbox): 237 scenarios, 220 passed, 17 failed. Of those, 8 are expected xfail and 9 are pre-existing sandbox/network issues unrelated to this diff (no outbound access to repo.amd.com, plus one GPU-telemetry timeout) — none touch the files this PR changes.

[rominf]

The apps/rocm/src/main.rs half of this is clean — the per-ecosystem conversion is faithful rather than copy-pasted (rpm --import vs curl | gpg --dearmor + signed-by=, autorefresh=0/type=rpm-md only where zypper wants them), dead helpers were removed rather than orphaned, and the tests were strengthened to assert the actual repo-file body and target path. I also confirmed the removal is complete: no stale amdgpu-install references anywhere in apps/, crates/, docs, tests, CI, or completions.

Three things in the diagnose.rs/fix.rs half I'd like resolved first.

1. fix-12 no longer fixes the state it diagnoses

check_12 fires on KEYWORDS_DPKG_BROKEN (half-configured, dkms .*failed, dpkg: error, sub-process /usr/bin/dpkg returned). The old remedy ran amdgpu-install --uninstall + apt autoremove --purge, which actually removed the broken package.

The new remedy (crates/rocm-core/src/diagnose.rs:1063-1085, mirrored in crates/rocm-core/src/fix.rs:290-295) only moves repo files aside, refreshes metadata, and runs rocm install driver. Moving a repo definition file doesn't touch a package recorded half-configured in dpkg's status DB, or a failed DKMS build under /var/lib/dkms/amdgpu/ — so the apt-get install amdgpu-dkms inside rocm install driver hits the same interrupted-dpkg error. The recipe's own summary says it will "clear the half-configured state", which it no longer does.

Suggest restoring a state-clearing step before the reinstall (sudo dpkg --configure -a or sudo apt-get -f install, plus an explicit purge of the broken driver package), in both the dynamic Fix and the static FixRecipe so they don't drift.

2. Destructive sudo mv is emitted ready-to-run, against the file's own precedent

diagnose.rs:1074-1078 pushes format!("sudo mv {r} {r}.bak") uncommented, for every entry in rocm_repos_seen. But the sibling checker in the same file, check_7_stale_repos (diagnose.rs:747-751), deliberately emits the identical command commented out with a guard — # sudo mv {r} {r}.bak # quarantine, do not delete yet — and this PR's own static twin at fix.rs:291 also comments it out.

That guard matters because rocm_repos_seen isn't provenance-checked: examine.rs:947-966 scans the apt/yum/zypp repo dirs and adds any filename containing rocm, amdgpu, or radeon. A distro-shipped, mirror, or hand-added repo file gets swept in and the user is handed a ready-to-paste command to disable it.

Suggest commenting these out exactly as check_7 and fix.rs do, or restricting the loop to the markers rocm-cli actually writes.

3. The new provenance wording isn't accurate

examine.rs:47-48 now reads "Repo files dropped by rocm-cli's repo-native package-manager install flow". rocm-cli writes only three of the seven — /etc/apt/sources.list.d/amdgpu.list (main.rs:2812), /etc/yum.repos.d/amdgpu.repo (main.rs:2908), /etc/zypp/repos.d/amdgpu.repo (main.rs:3002). The other four (rocm.list, radeon.list, and the two rocm.repos) are legacy/manual-doc names rocm-cli never creates; the previous comment was accurate for them. The claim propagates into user-visible text at diagnose.rs:1063 and fix.rs:288, which is partly what makes (2) read as safe.

Suggest wording it as what the constant actually models — AMD repo files written either by rocm-cli's install or by the documented manual/legacy setup, whose presence marks a package-manager-repo-managed ROCm.

---

Non-blocking

• fix-3 is now self-contradictory. This PR updated diagnose.rs:505 to "rerun rocm install driver --dkms" but the static twin at fix.rs:109 still says "rerun with --no-dkms" — a flag that doesn't exist anywhere in the CLI.
• The refresh chain hides the diagnostic it needs to show. diagnose.rs:1079-1082 / fix.rs:293: sudo apt update 2>/dev/null || sudo dnf clean all 2>/dev/null || sudo zypper refresh 2>/dev/null. On a Debian host with broken repo state — exactly when this fires — apt update fails with stderr discarded, then falls through to two commands that don't exist, also silenced. This also undercuts the recipe's own note ("if the package-manager update/refresh warns it will remove unrelated packages, stop"), since the warning was just sent to /dev/null. distro/distro_id is available and the mv step above is already personalized per-host, so branching to one correct command with visible output would be safer and more consistent.
• The verify string no longer verifies. diagnose.rs:1093 / fix.rs:299 greps rocm examine --json for rocm_install_method|rocm_version — both fields are always present, so the grep succeeds whether or not the fix worked. Only the trailing rocminfo | head -n 5 carries signal.
• "apt"/"dnf" are now near-unreachable. In examine.rs:921-941 the marker loop runs first and unconditionally sets "repo-native"; the dpkg/rpm probes only run if is_empty(). Any repo-installed ROCm has a marker, so those two values only surface when the repo file was deleted but the package remains. Pre-existing ordering, but the rename makes it more visible — worth deciding whether they still earn their place.
• Test gap. probe_rocm_install has no unit tests, and there's no coverage for check_12/fix-12. The renamed sentinel values and the new SLES zypp marker paths ship untested, while the main.rs side has 19 solid driver-plan tests. A table-driven test over the marker → method mapping would be proportionate.

Worth a conscious call

The rocm examine --json value rename (amdgpu-install → repo-native, tarball-or-other → runfile-or-tarball) is a silent breaking change to a machine-readable interface. Nothing in-tree consumes it, but there's no schema or deprecation window, so any external tooling string-matching the old values breaks. Probably worth a line in the PR body or release notes.

CI

E2E tests (Strix Halo, Ubuntu) looks unrelated to this change. The job reports 0 unexpected failure(s) — all 7 failing scenarios are declared xfail, and the non-zero exit comes solely from 3 XPASS entries (chat/serve scenarios that were expected to fail on that host but passed). None of them touches driver install, the examine install-method field, diagnose, or fix; the two examine/diagnose scenarios in that suite both pass. The same job failed the same way on #177 with a superset of the same XPASSes, on a diff that also doesn't touch driver install, and it's green on this PR's merge base. Looks like host-state-dependent expectation drift — a re-run should clear it, and pruning the stale expectations.toml entries belongs in its own PR.

E2E tests (GPU) was still running when I looked; worth confirming it lands green.

Verified locally on the branch: cargo fmt --check clean, cargo clippy --workspace --all-targets clean, cargo test -p rocm 388 passed.

[juhovainio]

Thanks for the thorough review — addressed all three blocking points plus the non-blocking ones that were straightforward, in 0c10f5d.

1. fix-12 doesn't fix the diagnosed state: restored the state-clearing step before quarantine+reinstall — dpkg --configure -a && apt-get install -f && apt-get purge -y amdgpu-dkms on apt hosts, dnf remove -y amdgpu-dkms / zypper remove -y amdgpu-dkms on the others — in both check_12 (host-specific, picks one family) and the static fix-12-installer (lists all three, comment-labeled, since the static catalog has no host to branch on).
2. Destructive sudo mv emitted uncommented: now commented out (# sudo mv {r} {r}.bak # quarantine, do not delete yet), matching check_7's precedent exactly.
3. Inaccurate provenance wording: REPO_NATIVE_INSTALL_MARKERS's doc comment now says only amdgpu.list/amdgpu.repo are written by rocm-cli's own install; the rest are legacy/manual-setup names it never creates.

Non-blocking, also fixed:

• fix-3's stale --no-dkms → --dkms (the real flag).
• The silent apt||dnf||zypper refresh chain is now a single command, picked from the host's detected package-manager family (via rocm_repos_seen, falling back to distro_id) — visible output instead of /dev/null.
• verify now greps for the actual "repo-native" value instead of field names that are always present.
• Added unit tests: repo_pkg_family, check_12/fix-12's command shape (asserts the state-clearing step and the commented-out mv), and a table-driven test over the marker → install-method mapping (split probe_rocm_install's marker logic into a pure repo_native_method_from_markers fn so it's testable without touching the filesystem).

Left as-is (per your own framing as non-blocking / worth-a-conscious-call, not requiring a code change):

• "apt"/"dnf" near-unreachability — pre-existing ordering, agree it's worth a future look but out of scope here.
• The install-method value rename (amdgpu-install → repo-native, tarball-or-other → runfile-or-tarball) is a breaking change to rocm examine --json's machine-readable output; noting it here for the record since nothing in-tree consumes it and there's no deprecation window.

cargo fmt --check, cargo clippy --workspace --all-targets, and cargo test -p rocm-core all clean locally (two pre-existing proc_lifecycle test failures are unrelated to this change and reproduce identically on the pre-review commit — looks like a sandbox signal-handling limitation, not something this PR touched).

[rominf]

Re-reviewed 231c5f78..7518b1ba. All three blocking items are genuinely fixed — I went looking for superficial fixes and didn't find any:

• fix-12 state clearing — diagnose.rs:1090-1101 now emits dpkg --configure -a / apt-get install -f / purge (or the dnf/zypper equivalent) before quarantine and reinstall, and the static twin at fix.rs:293-296 lists all three labeled blocks. The new repo_native_broken_fires_and_clears_state_before_reinstall test pins the ordering.
• sudo mv — now the commented # sudo mv {r} {r}.bak # quarantine, do not delete yet form, byte-identical to the check_7 precedent rather than an approximation of it.
• Provenance wording — examine.rs:44-49 names all seven markers and gets the 3-written / 4-legacy split right. I checked it against the actual write sites (main.rs:2812, :2908, :3002) and there are no others.

The non-blocking items from last round are also handled: the --no-dkms twin contradiction, the 2>/dev/null silencing (the "stop if the refresh warns" note is reachable now), and the verify grep. On that last one — grep -q '"rocm_install_method": "repo-native"' was the easiest thing to get subtly wrong, so I verified the colon-space form actually matches: main.rs:1832 uses to_string_pretty, and Examination has no rename_all or skip_serializing_if, so that literal spacing is what's emitted.

Extracting repo_native_method_from_markers as a pure function to make the marker logic testable without touching the filesystem was the right call, and the reverse-the-input-order test is a nice touch.

One thing that needs doing before merge

DCO. The merge commit 7518b1ba is signed but has an empty Signed-off-by trailer, which is why Commit signatures + sign-off is red — both of your real commits (231c5f78, 0c10f5d5) are fine. Rebasing onto fresh main instead of merging fixes it and avoids recreating the problem.

windows-build-and-test is unrelated: only lifecycle-windows-http-install failed, on a loopback-HTTP error occurred while sending the request, with 98/99 steps passing. Same signature as the request-fragmentation flake addressed in #139, and green on main. Worth a re-run so the PR isn't sitting red. E2E tests (GPU) landed green, as did the other four E2E lanes.

On repo_pkg_family

The doc comment at diagnose.rs:1049 says the repo-dir family is known "with certainty" and treats distro_id as the weaker signal. Preferring the repo dir for the primary path is defensible — it's direct evidence of which package manager's repo config these commands act on, whereas distro_id reaches the same answer via a lookup table. But the certainty claim doesn't hold, and the ordering introduces a failure mode distro_id wouldn't have:

rocm_repos_seen isn't curated. examine.rs:974-990 reads all three repo dirs and pushes any filename containing rocm/amdgpu/radeon. A leftover /etc/yum.repos.d/rocm.repo on a Debian host — image layer, or someone once installed yum — makes repo_pkg_family return "dnf" and hands the user sudo dnf remove -y amdgpu-dkms on Ubuntu. The apt → dnf → zypper order also silently picks apt when two families are present rather than noticing the contradiction.

The more actionable half is the fallback, though — the one branch where distro_id is the only signal is the least careful code in the function. diagnose.rs:1066-1070 is a hardcoded list with _ => "apt" and no ID_LIKE; ID_LIKE isn't merely unconsulted, it's never parsed (examine.rs:425-426 captures only ID and VERSION_ID). Meanwhile openmpi.rs:917 in this same crate already has a tested resolve_package_manager(os_id, id_like) that handles derivatives, amzn included. So an Amazon Linux host with no ROCm repo file is told to run apt-get purge.

Suggestions, in order of value:

1. Reuse resolve_package_manager for the fallback, and parse ID_LIKE into Examination so it can. That branch is wrong today.
2. Reword the comment — the repo dir tells you a file exists under that dir, not that the dir's package manager manages ROCm. Something like "prefer the family implied by the repo dir the marker files live under, since that's the config these commands act on" says the same thing without the certainty claim.
3. Optionally treat a repo-dir/distro_id disagreement as worth surfacing rather than silently resolving.

Bounded blast radius either way: fix-12 is auto_applicable: false with runner: None, so these are printed for a human to paste, not executed.

Smaller things

• The "run only the matching block" guard covers only the first triplet. fix.rs:293 puts it above the removal commands, but the three refresh commands at fix.rs:298-300 are equally mutually exclusive and sit past an intervening comment line with no repeated guard.
• One new test is near-vacuous. repo_native_broken_does_not_fire_on_non_repo_native_installs_without_symptom (diagnose.rs:1774) uses an empty symptom, which already forces kw_score = 0, so the score is 0 regardless of install method — the method gate the name describes is never exercised. Passing the same dpkg: error processing package amdgpu-dkms symptom as the positive test with a non-repo-native method would actually probe it. Worth doing because score += kw_score is unconditional and only the +20 bonus is method-gated, so the check can still fire — that behaviour is worth pinning down.
• The static fix-12 recipe still has no test. It was half of what the first finding asked for, and fix.rs's test module only covers catalog shape. A shape assertion mirroring the diagnose.rs one would stop someone re-enabling a live mv or dropping dpkg --configure -a.
• --json rename. Recorded in your comment, but the PR description is the durable record and doesn't mention it yet — there's no CHANGELOG in the repo, so that's the only place it'll survive.

Pre-existing, not yours, but adjacent: fix-3's verify is rocminfo | head -n 20 in diagnose.rs:510 vs head -n 5 in fix.rs:119.

Verified locally on 7518b1ba: cargo fmt --check clean, cargo clippy --workspace --all-targets clean (sources touched first, since post--- args aren't fingerprinted), cargo test -p rocm 388 passed.

Feedback addressed

[juhovainio]

Follow-up commit (28fe71e) on this branch: apt_driver_plan(), dnf_driver_plan(), and sles_driver_plan() all generated a repo baseurl under repo.radeon.com/graphics/{rocm_version}/. Checked against AMD's current package-manager install docs and the live directory tree: that path only carries the userspace graphics/mesa/llvm stack and never ships amdgpu-dkms. The kernel driver package lives in a separately-versioned tree, repo.radeon.com/amdgpu/{driver_version}/ (e.g. 31.40.1), which has no relation to the ROCm version this code was interpolating (repo.radeon.com/amdgpu/7.2.4/ 404s). As written, the generated dnf install amdgpu-dkms / zypper install amdgpu-dkms / apt equivalent would have failed to resolve the package on all three distro families.

Fix: repointed all three driver plans at /amdgpu/{driver_version}/, and swapped the version placeholder from ROCM_CLI_AMDGPU_VERSION (a ROCm version) to ROCM_CLI_AMDGPU_DRIVER_VERSION, defaulting to AMD's published latest alias so no ROCm-version-to-driver-version mapping table is needed.

Re: the amdrocm-* / repo.amd.com direction referenced in this issue — cross-checked against TheRock's RFC0012 (Repo Structure), the approved spec behind that naming. It explicitly reserves amdgpu/ under each stream subdomain "for future use; future GPU driver artifacts" and defers the actual repo.radeon.com → repo.amd.com driver consolidation to an unpublished companion RFC (RFC00XX-Repository-Package.md). So repo.amd.com is the right long-term target for ROCm software packages, but the driver isn't served there yet — today's fix (repo.radeon.com/amdgpu/) is correct in the interim. Tracking the eventual migration internally so it isn't lost once that RFC ships.

[Copilot]

Pull request overview

Removes amdgpu-install as a supported/recognized ROCm installation method and standardizes Linux driver installation around “repo-native” package-manager flows, updating examine/diagnose/fix behavior accordingly.

Changes:

• Switches Linux driver install planning away from amdgpu-install RPM/bootstrap and toward writing repo definitions + installing amdgpu-dkms directly (APT/DNF/Zypper).
• Updates rocm examine install-method detection/markers and reported values (including SLES/zypp markers and an internal ID_LIKE capture for package-family fallback).
• Updates rocm diagnose + rocm fix guidance/recipes to remove amdgpu-install references and keep the fix-12-installer ID stable under the new method naming.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
apps/rocm/src/main.rs	Reworks driver install plan generation to repo-native repo-file creation and direct amdgpu-dkms installs; updates related tests/strings.
crates/rocm-core/src/examine.rs	Renames/expands marker detection to “repo-native”, adds zypp paths, and updates fallback install-method labeling.
crates/rocm-core/src/diagnose.rs	Removes the amdgpu-install tracker arm and rewires fix-12-installer diagnosis logic to the repo-native method with package-family-aware commands.
crates/rocm-core/src/fix.rs	Updates the static fix-12-installer recipe and related verification text to match repo-native guidance.
crates/rocm-core/src/openmpi.rs	Exposes package-manager resolution helpers for reuse by diagnosis logic.

Suppressed comments (2)

apps/rocm/src/main.rs:3002

• The repo baseurl line is single-quoted, which prevents ${ROCM_CLI_AMDGPU_DRIVER_VERSION:-latest} from expanding. This would create a zypp repo file whose baseurl literally contains ${...}, which zypper cannot use.

                    "printf '%s\\n' '[amdgpu]' 'name=amdgpu' 'baseurl={}' 'enabled=1' 'autorefresh=0' 'type=rpm-md' 'gpgcheck=1' 'gpgkey=https://repo.radeon.com/rocm/rocm.gpg.key' | sudo tee /etc/zypp/repos.d/amdgpu.repo >/dev/null",

apps/rocm/src/main.rs:2908

• The repo baseurl line is single-quoted, which prevents ${ROCM_CLI_AMDGPU_DRIVER_VERSION:-latest} from expanding. This would create a .repo file whose baseurl literally contains ${...}, which dnf cannot resolve.

                "printf '%s\\n' '[amdgpu]' 'name=amdgpu' 'baseurl={}' 'enabled=1' 'priority=50' 'gpgcheck=1' 'gpgkey=https://repo.radeon.com/rocm/rocm.gpg.key' | sudo tee /etc/yum.repos.d/amdgpu.repo >/dev/null",

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (7)

apps/rocm/src/main.rs:2812

• The repository entry is single-quoted, so the shell never expands ${ROCM_CLI_AMDGPU_DRIVER_VERSION:-latest}. The file receives that literal text and apt-get update cannot resolve the intended repository URL. Double-quote the generated entry so the override/default is expanded when the command runs.

                    "printf '%s\\n' 'deb [arch=amd64 signed-by=/etc/apt/keyrings/rocm.gpg] https://repo.radeon.com/amdgpu/{repo_version_expr}/ubuntu {codename} main' | sudo tee /etc/apt/sources.list.d/amdgpu.list >/dev/null"

apps/rocm/src/main.rs:3002

• The single-quoted baseurl prevents ${ROCM_CLI_AMDGPU_DRIVER_VERSION:-latest} from expanding, leaving an unusable literal URL in the zypp repo file. Use double quotes for this argument.

                    "printf '%s\\n' '[amdgpu]' 'name=amdgpu' 'baseurl={}' 'enabled=1' 'autorefresh=0' 'type=rpm-md' 'gpgcheck=1' 'gpgkey=https://repo.radeon.com/rocm/rocm.gpg.key' | sudo tee /etc/zypp/repos.d/amdgpu.repo >/dev/null",

crates/rocm-core/src/fix.rs:306

• rocminfo | head returns the status of head, so this verification may succeed even if rocminfo is absent or fails to access the GPU. Check rocminfo directly instead of masking its status through a pipeline.

        verify: "rocm examine --json | grep -q '\"rocm_install_method\": \"repo-native\"' && rocminfo | head -n 5",

crates/rocm-core/src/diagnose.rs:1135

• The final pipeline masks rocminfo failures because a successful head determines the pipeline status in the default shell. The verification can therefore return success when rocminfo is missing or cannot initialize a GPU. Run rocminfo directly (output can be redirected) so its exit status is checked.

        verify: "rocm examine --json | grep -q '\"rocm_install_method\": \"repo-native\"' && rocminfo | head -n 5".to_owned(),

crates/rocm-core/src/diagnose.rs:1126

• This command does not reinstall anything: without --dkms, build_driver_install_plan creates no commands, and without --yes, install_driver returns the plan without executing it (apps/rocm/src/main.rs:2199-2203, 2777-2833). Show the review command and the explicitly approved execution command instead.

This issue also appears on line 1135 of the same file.

    commands.push("rocm install driver".to_owned());

crates/rocm-core/src/fix.rs:301

• The advertised reinstall is a no-op because rocm install driver without --dkms generates only a non-mutating preflight plan, and execution additionally requires --yes. The manual recipe should include the actual approved install command.

This issue also appears on line 306 of the same file.

            "rocm install driver",

crates/rocm-core/src/diagnose.rs:1115

• rocm_repos_seen is broader than the repo files written by the driver installer: probe_rocm_install adds every filename containing rocm, amdgpu, or radeon (examine.rs:981-995). This loop therefore tells a normal repo-native user to quarantine rocm.list/rocm.repo too, but rocm install driver only recreates amdgpu.list/amdgpu.repo; ROCm userspace updates remain disabled afterward. Limit this reset to the files the driver flow owns.

        for r in &e.rocm_repos_seen {
            commands.push(format!(
                "# sudo mv {r} {r}.bak     # quarantine, do not delete yet"
            ));

[rominf]

Reviewed at head 28fe71ef. The conversion to a repo-native flow is the right direction and the removal is clean — no dead helpers, no orphaned env vars, no stale references anywhere in the tree. Two blocking issues before merge.

Blocking

1. Single-quoting prevents version expansion — dnf/zypper repo files get an invalid baseurl

apps/rocm/src/main.rs:2905-2911 (dnf) and :2999-3005 (zypper) emit:

printf '%s\n' '[amdgpu]' ... 'baseurl=https://repo.radeon.com/amdgpu/${ROCM_CLI_AMDGPU_DRIVER_VERSION:-latest}/rhel/9.7/main/x86_64' ... | sudo tee /etc/yum.repos.d/amdgpu.repo

The baseurl=... token is a single-quoted shell word. These commands execute through sh -c (crates/rocm-core/src/runtime.rs:165), which does not expand ${VAR:-default} inside single quotes even when the variable is exported. The written file therefore contains a literal ${ROCM_CLI_AMDGPU_DRIVER_VERSION:-latest}, the repo cannot resolve, and the following install -y amdgpu-dkms fails.

This is new for dnf/zypper: the base revision built these URLs with unquoted interpolation (sudo dnf install -y {url}), which expands correctly. The move to printf | tee introduced the quoting. The apt path at :2812 has the same defect, but that one is pre-existing — worth fixing all three together given this PR makes the repo-native flow the supported path.

Suggested fix: resolve the version in Rust (std::env::var("ROCM_CLI_AMDGPU_DRIVER_VERSION"), defaulting to latest) and emit a fully literal URL, rather than relying on shell expansion surviving the quoting.

Note that no current test catches this — the 19 driver-plan tests assert the rendered plan contains the literal ${ROCM_CLI_AMDGPU_DRIVER_VERSION:-latest} string, so they pin the bug rather than the resulting URL. A test that pipes one generated command through sh -c and asserts the resolved baseurl would close the gap.

2. fix-12's remediation step is a no-op

crates/rocm-core/src/diagnose.rs:1126 and crates/rocm-core/src/fix.rs:301 both end with bare rocm install driver. That command reinstalls nothing:

• dkms is a plain #[arg(long)] defaulting to false (main.rs:528)
• the apt/dnf/sles plans only populate commands and set mutating: dkms inside if dkms { ... }
• install_driver() returns early with executed: false when !plan.mutating (main.rs:2188-2205)

A user following fix-12 verbatim gets a plan reporting no execution commands and "DKMS was not requested; this is a non-mutating preflight plan." The codebase gets this right elsewhere — main.rs:2272 and check_3 both say rocm install driver --dkms. Suggest rocm install driver --dkms --yes in both twins.

Non-blocking

• check_12's _ => {} arm. diagnose.rs:1092-1124: repo_pkg_family can return "pacman" via resolve_package_manager, but the state-clearing match has no pacman arm while the refresh match's _ arm still pushes sudo apt update. Low reachability, but an explicit "unsupported package manager" note would beat emitting an apt command on Arch.
• repo_native_broken_does_not_fire_on_non_repo_native_installs_without_symptom (diagnose.rs:1791) is near-vacuous — keyword_score short-circuits on an empty symptom, so the assertion holds regardless of rocm_install_method. The install-method gate the name claims to pin is never exercised.
• Quarantine loop scope. diagnose.rs:1106-1116 iterates all of rocm_repos_seen (any filename matching rocm/amdgpu/radeon), but rocm install driver only recreates amdgpu.list/amdgpu.repo. A user who un-comments the suggested mv for a hand-added rocm.list loses ROCm userspace updates permanently. Mitigated by the lines being emitted commented out; a one-line note distinguishing the file rocm-cli writes from legacy names would close it.
• verify masks exit status. rocminfo | head -n 5 reports head's status. Only ever printed, never executed, so this is recipe quality rather than a functional gate. Related pre-existing drift: fix-3's verify uses head -n 20 in diagnose.rs:510 but head -n 5 in fix.rs:119 — predates this PR, worth a drive-by while the file is open.
• zypper trust path. :3009 runs install -y with gpgcheck=1 relying on the rpm --import at :2997. Since stdin is Stdio::null() this fails fast rather than hanging, but --gpg-auto-import-keys would be more robust.

Design notes

• The apt/dnf/sles triplication now argues for a shared repo-file builder — the quoting bug above landed identically in two of the three call sites, which is direct evidence the duplication is costing correctness. A helper centralizing the quoting decision once would have prevented it. The apt-only pinning file, per-distro kernel-header prepares, and the SLES SUSEConnect prelude should stay distinct.
• Three separate notions of "which package manager does this host use" now coexist (resolve_package_manager, build_driver_install_plan's distro match, and the new repo_pkg_family). This PR partially reconciles two of them, which is the right call; the third still lacks a pacman branch.
• 15 fix-ids have hand-maintained diagnose.rs/fix.rs twins with no cross-file test — the fix-3 drift above is live proof review alone doesn't catch it. A single parity test over the id set and verify strings would be cheap.
• The default moving from pinned 7.2.4 to floating latest is a real reproducibility tradeoff, defensible given it avoids maintaining a ROCm-to-driver version table, but worth stating as a deliberate decision.

Verification

I confirmed the --json rename has no in-tree consumers: a full-tree grep at this head for amdgpu-install, amdgpu_install, tarball-or-other, ROCM_CLI_AMDGPU_VERSION, ROCM_CLI_AMDGPU_PACKAGE_RELEASE, and repo.radeon.com/graphics returns zero hits across docs, scripts, e2e fixtures, .github/, and apps/rocmd.

One thing to check before merge: only the CodeQL checks have reported on 28fe71ef. Since that is the commit that repointed all three URLs, the unit-test, clippy, and E2E lanes should be confirmed green on this SHA specifically.

[juhovainio]

Addressed both blocking points from the latest review (28fe71e), in 20f166b.

1. ${ROCM_CLI_AMDGPU_DRIVER_VERSION:-latest} never expands: these commands are built as printf '%s\n' '...' | sudo tee ... with the repo-file content single-quoted, so the shell never performs ${VAR:-default} parameter expansion — the written repo file got the literal, broken string as its baseurl. Fixed by resolving the version in Rust before interpolation (std::env::var("ROCM_CLI_AMDGPU_DRIVER_VERSION").ok().filter(|v| !v.is_empty()).unwrap_or_else(|| "latest".to_owned())), so the generated command always contains a real value regardless of shell. Updated the ~11 test assertions that had been asserting the raw (broken) shell-expression string.
2. fix-12's reinstall step is a no-op: rocm install driver without --dkms --yes is a non-mutating preflight/dry-run, so the diagnosed state was never actually fixed. Changed both the dynamic check (check_12_repo_native_broken) and the static recipe twin (fix-12-installer in RECIPES) to emit rocm install driver --dkms --yes, and added a regression assertion in each file's test module guarding against the bare form creeping back in.

cargo fmt --check, cargo clippy --workspace --all-targets, cargo test -p rocm (388 passed), and cargo test -p rocm-core (211 passed) all clean locally — same two pre-existing unrelated proc_lifecycle failures as before (sandbox signal-handling limitation, reproduces identically on the pre-review commit).

Feedback addressed @rominf

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (6)

crates/rocm-core/src/diagnose.rs:755

• The stale-repo checker can now receive yum and zypp paths, but this remediation still assumes apt, and bare rocm install driver is explicitly non-mutating. On RHEL/SLES it therefore recommends the wrong refresh command and never reinstalls anything. Emit the detected manager's refresh command and use rocm install driver --dkms --yes for the actual reinstall.

    commands.push("# If apt now resolves, reinstall via the repo-native flow only:".to_owned());
    commands.push("#   rocm install driver".to_owned());

crates/rocm-core/src/diagnose.rs:1064

• rocm_repos_seen contains stale/conflicting files from every scanned repo directory, so an old apt marker makes this return apt even on a RHEL or SLES host. Fix-12 then emits dpkg commands instead of the host's package manager. Prefer the distro resolver, using a repo path only when the distro is unknown and the observed paths identify one unambiguous family.

    if e.rocm_repos_seen.iter().any(|r| r.contains("/etc/apt/")) {
        return "apt";
    }
    if e.rocm_repos_seen
        .iter()
        .any(|r| r.contains("/etc/yum.repos.d/"))
    {
        return "dnf";
    }
    if e.rocm_repos_seen.iter().any(|r| r.contains("/etc/zypp/")) {
        return "zypper";

crates/rocm-core/src/diagnose.rs:1084

• The renamed check still scores only dpkg/apt symptoms. Common DNF or zypper failures such as transaction or dependency errors score zero even when the install method is repo-native, so the new RHEL/SLES recovery paths are never surfaced. Select package-manager-specific keyword evidence or add RPM/zypper patterns and regression tests.

    let (kw_score, kw_ev) = keyword_score(symptom, KEYWORDS_DPKG_BROKEN);
    score += kw_score;
    evidence.extend(kw_ev);
    if method == "repo-native" && kw_score > 0 {
        score += 20;

crates/rocm-core/src/fix.rs:109

• --dkms enables the same DKMS build that this diagnosis says failed against the unsupported kernel, so retrying it without changing the kernel repeats the failure rather than providing a risky fallback. Remove this alternative and require moving to a supported kernel before reinstalling.

        rationale: "ROCm is installed but your kernel/distro combination is outside the supported matrix. Match the kernel to the matrix before reinstalling, or rerun `rocm install driver --dkms` and accept the risk.",

crates/rocm-core/src/fix.rs:293

• In the half-configured DKMS state this recipe targets, dpkg --configure -a commonly fails while retrying the broken package. Because the steps are chained with &&, that prevents the purge command from ever running, so the recipe cannot clear the state. Reorder the recovery around removing the broken package or explicitly handle the expected configure failure.

            "sudo dpkg --configure -a && sudo apt-get install -f && sudo apt-get purge -y amdgpu-dkms   # Debian/Ubuntu",

crates/rocm-core/src/diagnose.rs:505

• This suggests rerunning the DKMS installation after diagnosing that DKMS failed on an unsupported kernel. Without changing the kernel, the command retries the same failing operation; direct the user to install a supported kernel before reinstalling instead.

This issue also appears in the following locations of the same file:

• line 754
• line 1054
• line 1080

            "# kernel that matches ROCm, or rerun `rocm install driver --dkms`.".to_owned(),

[fredespi]

I went through this diff (976cc15...14d717f, 5 files, +473/-103) and didn't find anything I'd hold the merge for. Recording what I checked, since a fair bit of it was the kind of thing that's easy to get wrong in a change like this.

The removal is actually complete. amdgpu-install appears 32 times on the base commit and zero times at this head — no stale references left in the diagnosis text, fix recipes, or examine markers. The install-method values that changed (amdgpu-install → repo-native, tarball-or-other → runfile-or-tarball) have no consumers outside the three files you touched, so nothing is left keyed on the old strings.

The apt plan doesn't leave a dangling keyring reference. The new sources.list.d/amdgpu.list entry uses signed-by=/etc/apt/keyrings/rocm.gpg, and the plan does create that keyring (install -m 0755 -d /etc/apt/keyrings, then the curl | gpg --dearmor) two commands earlier, in the same Prepare phase. Ordering is right.

fix-12's verify string matches the real output. It greps for "rocm_install_method": "repo-native" with a space after the colon, and rocm examine --json goes through serde_json::to_string_pretty with snake_case field names, so that grep will actually hit.

The dnf/zypper paths are a security improvement, not just a refactor. Dropping zypper --no-gpg-checks in favour of writing a repo file with gpgcheck=1 plus an explicit rpm --import is a real tightening of the old behaviour.

One thing I looked at and decided isn't blocking, but you may want to know about: repo_pkg_family can return "pacman", since it falls back to openmpi::resolve_package_manager, which recognises arch/manjaro/endeavouros/arcolinux. Neither match in check_12_repo_native_broken has a pacman arm, so such a host would get the "Clear the half-configured package/DKMS state first." comment with no command under it, and then sudo apt update as the refresh step. In practice I don't think you can get there: reaching fix-12 needs the dpkg-flavoured keywords to clear a score of 50, and the repo-native +20 bonus needs one of the apt/yum/zypp marker files, which an Arch host won't have. So it reads as unreachable dead ground rather than a bug — worth a cheap _ => {}-side guard only if you'd rather not leave the shape there.

Note this reflects a diff read only, not CI results or a real install on any of these distros — it isn't an approval. CI is fully green on 14d717f and all three earlier review threads are resolved, so from a code-review standpoint this looks safe to approve after whatever final validation you want.

[fredespi]

Follow-up on my earlier comment: the one thing I couldn't settle by reading the diff was whether the new repo.radeon.com/amdgpu/... paths actually exist, since the tests only assert the URL is built correctly, not that it resolves. I've now checked every URL this code can generate, and they all resolve:

Path (under repo.radeon.com/amdgpu/latest/)	Result
ubuntu/dists/jammy, ubuntu/dists/noble	200 — covers Ubuntu 22.04/24.04 and Debian 12/13
rhel/9.4, rhel/9.6, rhel/9.7, rhel/8, rhel/10	200
el/9.4, el/9.6, el/9.7, el/8, el/10	200 — Oracle, Rocky, and ID_LIKE-matched rebuilds
sle/15.7	200
rocm/rocm.gpg.key (the key the plans import)	200
ubuntu/dists/resolute	404

Two things worth noting about how those map. dnf_repo_version_path sends any 10.x to the major path (rhel/10/, not rhel/10.0/) and 8.10 to 8/, which is what AMD publishes — I checked the wrong paths first and got 404s before spotting that the code was right. Likewise the SLES arm only ever matches 15.7, which is the only version published under sle/.

The resolute 404 confirms the Ubuntu 26.04 TODO is describing reality rather than guessing, and it makes the design choice there look correct: planning zero repo commands and pointing at sudo apt install rocm is right, because writing a sources entry for a suite that doesn't exist would break apt update on every subsequent run for that user.

That closes the only open question I had. From a review standpoint I think this is ready to approve — the removal is complete, the repo paths are real, the keyring is created before it's referenced, gpg checking is now enforced where it previously wasn't, and CI is green on 14d717f. The remaining call is a maintainer's: 14d717f (Ubuntu 26.04) landed after the last human pass, so someone may want eyes on that commit specifically before pressing approve.

[rominf]

fredespi's sweep covers the removal itself well and I agree with it — the removal is complete, the keyring is created before it's referenced, and the dnf/zypper gpg tightening is a real improvement over the base rather than just a port. Two things it didn't reach, both in the fix recipes this PR rewrote.

1. crates/rocm-core/src/fix.rs:293 — the recipe can't do what it exists to do

sudo dpkg --configure -a && sudo apt-get install -f && sudo apt-get purge -y amdgpu-dkms   # Debian/Ubuntu

dpkg --configure -a is precisely what fails when a package is recorded half-configured — the condition check_12 fires on. The && short-circuits, so apt-get install -f and the purge never run, and the following rocm install driver --dkms --yes then hits the same broken state. print_recipe prints each commands entry verbatim as one $ line, so this is exactly what a user copies and pastes.

The dynamic twin gets this right — crates/rocm-core/src/diagnose.rs:1094-1097 pushes the three as separate commands. This is twin drift on the exact fix id this PR rewrote, and it defeats the recipe's own stated rationale ("Clear the broken package state").

2. apps/rocm/src/main.rs:2715 — a mutating command in the checks list

checks: vec!["rocm examine", "sudo apt install rocm", "rocminfo"]

checks renders under the post_reboot_checks: header. Every other arm populates it with read-only probes only — rocm examine, dkms status amdgpu, /sys/module/amdgpu, /dev/kfd, /dev/dri/renderD*. This arm presents an install action as a verification step, and duplicates guidance already carried in reason. The surface is exposed to the assistant through the restricted driver_plan tool, where AGENTS.md §7 requires inspecting state before proposing mutation, so a mutating command inside a checks list is the wrong shape for that boundary.

The accompanying test asserts only rendered.contains("sudo apt install rocm"), which the reason prose already satisfies on its own, so it doesn't pin the placement either way.

Suggestion: split the Ubuntu 26.04 commit out

14d717f is new-distro support landing as the newest commit in an otherwise mechanical removal, after every review round had closed. fredespi's URL check confirms resolute 404s, so the design choice there looks right — but as its own PR it can be reviewed, revised or reverted without holding up the removal, which is otherwise ready.

Non-blocking — carried over from earlier rounds and still live

• diagnose.rs:755 still emits the bare rocm install driver, the form this PR's own test message calls "a non-mutating preflight no-op". It's a commented hint rather than a runnable command, so it's milder than the above — but a user who uncomments it gets a no-op. Its neighbours are still apt-hardcoded (:753 sudo apt update, :761 verify) despite repo_pkg_family now living in the same file.
• fix-3 verify twin drift: head -n 20 (diagnose.rs:510) vs head -n 5 (fix.rs:119).
• repo_native_broken_does_not_fire_on_non_repo_native_installs_without_symptom uses an empty symptom, which forces kw_score = 0 regardless of install method, so the gate its name describes is never exercised. The meaningful sibling added alongside it doesn't fix this one.
• The --no-dkms → --dkms translation inverted the advice. The base text said to rerun with --no-dkms (skip the DKMS build); head advises performing the build that check_3 just diagnosed as out-of-matrix. There's no faithful translation available, but the current text recommends the operation most likely to fail — and fix.rs:109 adds "and accept the risk" while diagnose.rs:505 doesn't.
• check_12 scores only against KEYWORDS_DPKG_BROKEN, so a dnf or zypper transaction failure yields score <= 0 and returns zero(...). The remediation was made package-manager-agnostic in this PR, but the trigger is still Debian-only, so the new dnf/zypper branches can essentially never execute.
• Legacy installs made by the standalone amdgpu-install tool write the same amdgpu.list/amdgpu.repo paths, so they now report repo-native — indistinguishable from a rocm-cli-managed host, where the old value was accurate for both writers. Issue #180's "clear message pointing to supported methods" criterion has no counterpart in the tree. Partly moot for the CLI-args angle, since this was never a user-selectable value, so the gap is detection-and-guidance for existing installs rather than argument handling.

One structural note: fifteen fix ids now have hand-maintained diagnose.rs/fix.rs counterparts with no cross-file test, and this round produced two more drifts despite three prior reviews explicitly hunting for them. A single parity test over the id set, command shape and verify strings would be cheap and would have caught both.

[siloteemu]

🔴 Automated review · pr-review-watcher · f971974

Summary

Replaces amdgpu-install as a driver install method with direct package-manager repo-file writing (apt/dnf/zypper), renames the rocm_install_method values (amdgpu-install→repo-native, tarball-or-other→runfile-or-tarball), rewrites the fix-12 recipe, and adds a non-mutating Ubuntu 26.04 plan. Needs work. Verified: the migration story is genuinely clean on the state side — rocm_install_method is recomputed from disk markers on every run and is never persisted (DriverInstallState at apps/rocm/src/main.rs:2859 has no policy/method field), the marker set is unchanged so an amdgpu-install-managed host is simply relabelled rather than newly mis-detected, and a repo-wide grep finds zero surviving references to amdgpu-install/amdgpu_install/tarball-or-other anywhere (code, docs, skills, tests, CI); the new Fix.verify grep works because examine --json uses serde_json::to_string_pretty (apps/rocm/src/main.rs:2185). On reversion: most new assertions do fail on revert, but two do not (details below), and a previously-reported finding still holds — diagnose(&e, "") short-circuits in keyword_score before the install-method gate. I could not attribute the 2 red checks (no GitHub access from this sandbox); cargo test -p rocm-core --lib passes locally 322/322, but the branch is 32 commits behind main and git merge-tree HEAD origin/main reports a real content conflict in apps/rocm/src/main.rs, so a rebase is required regardless and the reds should not be assumed unrelated. Blocking: 4 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

• apps/rocm/src/main.rs:2971-2975 — dead branch, plus the test named for it proves nothing. codename is fully resolved before the match at main.rs:2950-2953, whose third fallback is codename_for_version(...) — which this same PR taught to return Some("resolute") for 26.04 (main.rs:3626). So by the time the ("ubuntu","26.04") arm runs, codename can never be empty, and the inner let codename = if codename.is_empty() { "resolute" } else { codename }; is unreachable. driver_plan_ubuntu_2604_falls_back_to_resolute_codename (main.rs:24093-24101) asserts only plan.codename == "resolute", so it passes with that branch deleted and with the entire 26.04 arm reverted (the unsupported-plan fallback at main.rs:3055-3067 carries the same pre-computed codename). Fix: delete the shadowing let codename = … inside the arm, and have the test also assert plan.policy == "ubuntu_native_archive" so it actually pins the arm.

• crates/rocm-core/src/diagnose.rs:532 and crates/rocm-core/src/fix.rs:109 — fix-3 guidance is reversed by the removal. check-3 fires precisely on "DKMS failed against an unsupported kernel" (diagnose.rs:509-520). The old advice was to rerun with --no-dkms — i.e. skip the module build. The new text says "rerun rocm install driver --dkms", telling the user to retry the exact operation the diagnosis says just failed, and fix.rs:109 still ends "…and accept the risk", which only parsed under the old sense. This is not a like-for-like command rename: rocm install driver has no non-DKMS install path — the plan is mutating: dkms (main.rs:3235, 3330, 3425), so without --dkms it is preview-only. The removal deleted the non-DKMS capability and left the guidance pointing at its opposite. Nothing tests fix-3 or fix-7 text. Fix: stop recommending --dkms here; say rocm-cli offers no non-DKMS driver install and route the user to matching the kernel to the support matrix (or the distro-native stack), and add an assertion over the fix-3 command text.

• crates/rocm-core/src/diagnose.rs:2079-2084 — the earlier finding still holds; this test is vacuous. repo_native_broken_does_not_fire_on_non_repo_native_installs_without_symptom calls diagnose(&e, ""). keyword_score returns (0, vec![]) immediately for an empty symptom (diagnose.rs:291-293), so check_12 hits if score <= 0 { return zero(...) } (diagnose.rs:1117) before rocm_install_method is ever consulted. It passes if the production change is reverted, and passes even if check_12 is removed from CHECKERS entirely (the all(|d| d.id != …) is vacuously true when the id never appears). Fix: pass a dpkg symptom and assert the evidence line "installed via the repo-native package-manager flow" is absent (and, for a positive control, present when the method is repo-native) — that is the gate the name claims. Related: in repo_native_broken_fires_on_dpkg_symptom_regardless_of_install_method (diagnose.rs:2102-2107), !evidence.contains("repo-native") also passes under the old code, where evidence could only ever say "amdgpu-install"; it needs the same treatment.

• No e2e scenario for a user-observable change — AGENTS.md §3. This PR changes what a user observes from rocm examine (install-method values), rocm install driver on Ubuntu 26.04 (new plan, new reason/checks output), and rocm diagnose/rocm fix (fix-12 command set and verify string), yet no file under tests/e2e-cucumber/features/ is added or changed. §3 is explicit that a unit test "does NOT discharge this", and requires either a new scenario or the PR text naming the existing @id: (and the lane, if gated). Caveat stated plainly: I could not read the PR body from this sandbox, so if it already names an existing scenario id, treat this item as discharged.

Non-blocking

• Branch is 32 commits behind main and conflicts with it in apps/rocm/src/main.rs (verified via git merge-tree); rebase before re-running CI, and re-check the 2 reds afterwards rather than dismissing them.
• apps/rocm/src/main.rs:2909 — ROCM_CLI_AMDGPU_DRIVER_VERSION (renamed from ROCM_CLI_AMDGPU_VERSION) appears nowhere in README.md, docs/, or help text, and ROCM_CLI_AMDGPU_PACKAGE_RELEASE is gone with no replacement; anyone who set the old variable is now silently ignored.
• apps/rocm/src/main.rs:2909-2916 — a value rejected by the [A-Za-z0-9._-] filter falls back to latest with no warning; a typo'd override is indistinguishable from not setting it.
• apps/rocm/src/main.rs:24411-24438 — the env-mutating test hand-rolls save/restore instead of using the repo's existing ScopedEnvVar RAII guard (main.rs:16752-16778); a panic between set and restore leaks the variable to the rest of the test binary.
• crates/rocm-core/src/diagnose.rs:782 — fix-7 still suggests bare rocm install driver, which this PR's own fix-12 commit correctly identified as a non-mutating preflight no-op; the same --dkms --yes treatment (or a note that it is preview-only) should apply here.

[rominf]

On the two items left as judgment calls: both are your call, and neither blocks the merge.

• Legacy ROCM_CLI_AMDGPU_VERSION / ROCM_CLI_AMDGPU_PACKAGE_RELEASE overrides: the last automated round rated this non-blocking. It suggested an optional one-line "no longer read" notice in the plan's reason when either variable is set, and listed the hard rename without a transition warning separately as a tradeoff for you to decide. Leaving it as documented in the "Breaking change" section is a legitimate choice.
• fix-7's multi-line verify: also non-blocking; moving the prose into notes was only a suggestion.

The other changes in this push are covered by the re-review of b7be4cb.

[rominf]

🔴 Automated review · pr-review-watcher · b7be4cb

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.

Review — needs work

Full review of the whole change.
Implements the dnf/zypper/examine/diagnose/fix part of #180 and says so. #180 stays open for repo.amd.com amdrocm-*, the "removed path" message, and ROCm 10.0 verification.

Each finding is marked new (first raised this round) or standing (raised by an earlier automated round and still holding). Standing objections from the round at 651a6b5 that this round did not re-find blind were put to the reviewer after its review (anchored evidence, labelled where used). Settled and no longer holding: the six env-racing tests, the untested +20 fix-12 bonus, the unreadable-os-release override path, the missing malformed-override scenario (driver-install-05 now covers it), the non-UTF-8 override, and the PR closing #180. Two items below are not gating this round on their own: the description item (the squash commit body comes from commit messages, not the description) and the red checks (the hosted E2E tests failure is dash-gen-tps-expiry-boundary, which also fails on main's own CI; the Strix Halo Windows lane failed on an HTTP 403 downloading a toolchain tarball).

Blocking

• [standing] Remediation tells users to run rocm install driver --dkms --yes on hosts where that command plans nothing — crates/rocm-core/src/diagnose.rs:846, :1155-1158, :1543-1553, :1612-1614; crates/rocm-core/src/fix.rs:526-552

  • What the remediation does: fix-12 tells the user to purge amdgpu-dkms and quarantine the repo files, then "reinstall" with rocm install driver --dkms --yes. fix-7 and fix-3 name the same command.
  • Why the reinstall does nothing: build_driver_install_plan_at and driver_plan_via_id_like (apps/rocm/src/driver_install.rs:1000-1264) return supported: false with no commands for many hosts:
    • Fedora (without ID_LIKE=rhel) and Arch.
    • openSUSE and SLES other than 15.7.
    • Ubuntu releases other than 22.04 and 24.04, Mint, and ID_LIKE=ubuntu 26.04 derivatives.
    • Debian 11, RHEL 9.0-9.3 and 9.5, Rocky 8.x, 9.5 and 10.x, Oracle 9.4-9.6 and 10.0.
  • It also exits 0: install_driver returns executed: false for an unsupported plan (driver_install.rs:44-49), and main.rs:3631-3645 logs it at info. On these hosts the user ends up with no driver and nothing visibly failed.
  • The 26.04 special case is a partial copy that drifts: ubuntu_driver_is_native_archive copies only the ("ubuntu","26.04") arm. The static fix-12 recipe in fix.rs names the DKMS reinstall and a dkms status amdgpu verify unconditionally, including on 26.04, where diagnose itself says neither applies.
  • Rule broken: AGENTS.md §3 requires that a command named in remediation advice "must exist, accept those flags, and actually clear the condition that printed it". Base pointed at amdgpu-install, which did not depend on rocm-cli's distro table.
  • Confidence 85 · architectural.
  • Fix: move one "DKMS plan supported for (id, version, id_like)?" predicate into rocm-core and call it from both the plan builder and the remediation. Where it says no, do not emit the purge-then-reinstall sequence. Consider a non-zero exit for --yes on an unsupported plan.

• [new] None of the new Ubuntu 26.04 branches in rocm-core is tested — crates/rocm-core/src/diagnose.rs:850-852, :1602-1611, :1639-1642

  • I replaced ubuntu_driver_is_native_archive with false: all 775 rocm-core lib tests passed.
  • With true, 7 tests fail, all on the non-26.04 side. So the advice that prints sudo apt install rocm and the 26.04 fix-12 verify (dpkg-query … rocm) are never asserted.
  • Confidence 100 · logic.
  • Fix: add an Ubuntu 26.04 Examination case for check_3, check_7 and check_12 that asserts both the commands and the verify.

• [new] Branches the change adds that no test fails on (each confirmed by a mutation run):

  • diagnose.rs:1140-1145: I removed the fix-7 dnf and zypper ls …repos.d arms and all 4 stale_repos_* tests stayed green. The asserted path substrings also appear in the # sudo mv lines.
  • crates/rocm-core/src/examine.rs:2066-2070: I removed /etc/zypp/repos.d from the scan loop and all 775 tests stayed green. SLES repo detection is new in this PR and is untested.
  • apps/rocm/src/driver_install.rs:1629-1631: the group-A worker hard-coded latest into sles_repo_baseurl and all 75 driver_install tests stayed green. The override-reaches-URL test covers apt only, and DNF is likewise unchecked.
  • Confidence 100 · mechanical.
  • Fix: assert the family-specific ls line, a zypp-path detection case, and an override reaching the dnf and zypper baseurls.

• [standing, partly] Earlier mutation-confirmed gaps that still hold — apps/rocm/src/driver_install.rs (is_safe_shell_token, the dnf plans), crates/rocm-core/src/examine.rs (probe_rocm_install), crates/rocm-core/src/diagnose.rs (fix-12 verify test)

  • Not re-found blind this round; put to the reviewer, who re-ran mutations at this head (anchored evidence):
    • Widening is_safe_shell_token to also accept ', ; and / left all 75 driver_install tests green. ' is the character that breaks out of the single-quoted privileged commands; the $(id) payloads only pin $, ( and ).
    • rpm --import is asserted only for RHEL 9.7, Oracle 10.1 and SLES; the Rocky, Alma, EL8 and ID_LIKE dnf tests do not assert it (reasoned, not run).
    • Nothing exercises probe_rocm_install's scan loop; only is_live_rocm_repo_file_name and live_rocm_repo_paths_in_dir are tested directly.
    • fix-12's verify is now correct, but repo_native_broken_verify_requires_dpkg_status_not_just_name_presence still pins substrings only and does not pin which package is queried.
  • Settled as no longer holding: the os_id filter and the 26.04 reboot_required flag now each fail a test.
  • Fix: add a space-free payload containing ' (and ;) to the hostile-override/os-release fixtures; assert rpm --import in a non-RHEL dnf test; drive a repo directory through probe_rocm_install.

• [new] Changed rocm diagnose/rocm fix output has no scenario, and no justification is given — crates/rocm-core/src/diagnose.rs:1110-1200, :1555-1680; crates/rocm-core/src/fix.rs:321-560

  • fix-3, fix-7 and fix-12 now print different, family-specific remediation commands, which is user-observable output.
  • No feature under tests/e2e-cucumber/features/ mentions fix-7, fix-12 or these commands.
  • AGENTS.md §3 requires a Gherkin scenario, or a stated reason in the PR text why there isn't one. The PR covers only examine-29 and driver-install-04/05.
  • Confidence 85 · mechanical.
  • Fix: add a scenario driving rocm diagnose/rocm fix fix-12-installer on a simulated dnf or zypper host, or justify its absence in the PR text.

• [new] The description misstates two facts about the change. (Not gating on its own this round; see the note above.)

  • It says openmpi.rs's PackageManager was widened "so driver_install.rs's repo-family detection can reuse" it. The only new user is crates/rocm-core/src/diagnose.rs:1518-1530 (repo_pkg_family), and a pub(crate) item in rocm-core is not reachable from apps/rocm at all.
  • Under Breaking change it says "Nothing in-tree consumes these strings". is_repo_native_installed (diagnose.rs:1539-1541) consumes rocm_install_method, including via rocm remote doctor against a remote's JSON (see the older-remote item under Non-blocking).
  • Confidence 100 · mechanical.
  • Fix: correct the description.

• [new] Required checks are red on this head (Not gating this round; the causes match main and infrastructure, see the note above.)

  • These fail in the statusCheckRollup: E2E tests, E2E tests (Strix Halo, Windows, current) and E2E consolidated report (self-hosted).
  • Per the run context, the hosted failure is dash-gen-tps-expiry-boundary (it also fails on base) and the Windows lane hit an HTTP 403 on a toolchain download. I did not confirm either cause myself.
  • Confidence 100 · mechanical.
  • Fix: rerun once base or infra is fixed. These need to be green before merge, whatever caused them.

Non-blocking

• [standing] The dnf refresh/verify cannot fail — diagnose.rs:1104, fix.rs:423-426

  • dnf clean all only deletes the local cache and never contacts a repo, so a still-404ing repo passes.
  • The helper's doc says it will "re-resolve the repo metadata", which is not true for dnf.
  • Confidence 85 · logic. Fix: dnf makecache.

• [new] The examine repo-file filter no longer sees deb822 .sources files — examine.rs:2007-2017 (via is_live_rocm_repo_file_name)

  • Base matched any rocm/amdgpu/radeon name. The new filter needs .list or .repo, so an amdgpu.sources drops out of rocm_repos_seen and fix-7's ≥2-repo evidence.
  • fix-7's ls | grep lines (diagnose.rs:1137-1145) still list .sources and .bak files, which disagrees with the filter.
  • Confidence 85 · logic.

• [new] Older remotes lose fix-12 evidence

  • A remote on an older CLI reports rocm_install_method: "amdgpu-install", which is_repo_native_installed (diagnose.rs:1539-1541) no longer accepts. That drops the +20 bonus and the evidence line under rocm remote doctor.
  • The PR handles older remotes elsewhere (#[serde(default)] on distro_id_like).
  • Confidence 85 · logic. Fix: accept the legacy value in the matches!.

• [standing] Skill docs out of step with the CLI — skills/rocm-doctor/reference.md:124, skills/rocm-doctor/SKILL.md:14

  • The fix-12 row still lists --accept-eula as a trigger, but this PR removed that keyword from KEYWORDS_DPKG_BROKEN. The CLI governs (AGENTS §7), and the e2e fixture parses only the id, OS and mode columns, so nothing catches this.
  • SKILL.md's description drops "amdgpu-install", which is the word users with that installer's errors will actually type. That is a lost routing trigger; keep it alongside the new wording.
  • Confidence 85 · mechanical.

• [new] check_3's 26.04 line contradicts the lines above it — diagnose.rs:851

  • The line pushed only on 26.04 begins "Not on Ubuntu 26.04: …", right after lines telling the user to rerun rocm install driver --dkms --yes.
  • Confidence 85 · mechanical.

• [standing] The fix-7 recipe's verify starts with prose — fix.rs:423

  • It is printed as $ Run only the line matching your package manager: (around fix.rs:1550), so the prose reads as a command.
  • Confidence 85 · mechanical.

• [standing] Stale or wrong comments and labels:

  • driver_install.rs:695, :926 and examine.feature:123 refer to a repo_version_expr: line, but the rendered label is still repo_version: (driver_install.rs:1722).
  • The fix-7 title at diagnose.rs:1197 still says "APT/DNF", while reference.md now says APT/DNF/zypper.
  • The fix-12 rationale at fix.rs:526 says it quarantines "the repo files rocm-cli's install wrote", but rocm.list, radeon.list and rocm.repo are manual AMD files.
  • examine.feature:434-435 says rocm_install_method "drives fix-12's remediation choice". It only affects score and evidence.
  • diagnose.rs:1588-1591: on dnf/zypper hosts with no recorded repo files, fix-12 shows an apt-path mv example.
  • Confidence 100 · mechanical.

• [standing] The 26.04 e2e coverage claim does not match CI — docs/testing.md:1624, driver_install.rs:1015-1020, driver_install.feature:34

  • These say a hosted ubuntu-26.04 image runs driver-install-04, but .github/workflows/ci.yml runs the e2e job on ubuntu-latest.
  • The step returns without asserting anything on non-Ubuntu hosts.
  • Confidence 70 · mechanical.
  • Driver note (from this head's CI log, not the blind review): the hosted E2E tests job's runner reported Image: ubuntu-26.04, and both driver-install-04 and driver-install-05 ran there, so the claim is true today. What remains is that the texts name a fixed image while CI follows ubuntu-latest. The earlier blocking objection that the step chose its expected arm from its own output is settled: it now reads /etc/os-release and asserts the exit code.

• [new] A render-level regression test was removed — driver_plan_dry_run_repo_version_line_is_resolved

  • It was added in #307 to pin the rendered repo_version: line. Nothing at unit level replaces it.
  • The e2e step that remains was loosened from "starts with a digit" to "safe charset".
  • Confidence 70 · logic.

• [new] Pre-existing problem that this change now hits more often — driver_install.rs:1316-1335

  • The apt plan's gpg --dearmor -o /etc/apt/keyrings/rocm.gpg prompts to overwrite on a re-run, and fails without a tty.
  • fix-12 now prescribes exactly that re-run, and it leaves the keyring in place. --batch --yes avoids it.
  • Confidence 70 · logic.

• [new] fix-3 suggests a remedy its own check cannot accept — diagnose.rs:847-848

  • It now suggests rocm install sdk as the alternative to avoid the DKMS build. That installs TheRock wheels and cannot satisfy fix-3's lsmod | grep amdgpu verify.
  • Confidence 70 · logic.

Decisions for the author

• [standing] Floating latest vs a pinned driver default — tradeoff

  • Against: with latest, the plan the user approves and the command strings recorded in state.json never name the version actually installed, so the same rocm binary installs different driver majors on different days. The apt pin at priority 600 on o=repo.radeon.com also lets a later apt upgrade pull in a new amdgpu-dkms. ROCDXG, by contrast, resolves its alias to a concrete build at plan time.
  • For: latest avoids a hard-coded release going stale.
  • The PR raises this itself as an open question. If you keep latest, consider at least recording the installed module version (detect_linux_amdgpu_driver_version) and documenting the choice in docs/release-trust.md.

• [standing] The old override variables are silently ignored — answered-by-intent

  • The description says: "The old ROCM_CLI_AMDGPU_VERSION / ROCM_CLI_AMDGPU_PACKAGE_RELEASE overrides are removed", and "This does a hard removal (no deprecation period) per the issue."
  • Otherwise this would be non-blocking. A user exporting ROCM_CLI_AMDGPU_VERSION=7.2.4 silently gets latest, which sits oddly with the code refusing a malformed new override rather than "silently fall back to latest" (driver_install.rs:864-891).
  • A one-line warning when an old variable is set would not be a deprecation period. Confirm silent is what you want.

• [new] No rollback path on dnf/zypper — non-blocking-improvement

  • Base installed the amdgpu-install RPM, which owned the repo files and shipped an uninstaller.
  • The new flow writes amdgpu.repo with tee, so no package owns it, and nothing documents how to undo the install.
  • Hosts upgrading from the old flow may also end up with both files live, which could trip fix-7's ≥2-repo heuristic.

• [new] Unpinned trust anchor for the GPG key — non-blocking-improvement

  • The key fetch is TLS-only trust-on-first-use, with no fingerprint check.
  • rpm --import trusts the key for all repos, and zypper --gpg-auto-import-keys accepts whatever key the repo presents.
  • It is still strictly better than base, but pinning the fingerprint would match the ROCDXG digest pinning in docs/release-trust.md.

Positive signals

• dnf/zypper no longer download and run the unsigned amdgpu-install RPM's scriptlets as root. They now use gpgcheck=1 repos with HTTPS keys.
• os-release values (VERSION_ID, codename, ID) and the version override are run through is_safe_shell_token before shell interpolation. The tests include space-free injection payloads, which a metacharacter-only check would miss, and mutations m2 to m5 each fail a test.
• #[serde(default)] on distro_id_like keeps remote doctor working against older remotes, and a fixture missing the field exercises it.

Deployment notes

• Breaking change in rocm examine --json: rocm_install_method changes "amdgpu-install" → "repo-native" and "tarball-or-other" → "runfile-or-tarball". External consumers must update, and rocm remote doctor against an older remote is affected (see Non-blocking).
• ROCM_CLI_AMDGPU_VERSION and ROCM_CLI_AMDGPU_PACKAGE_RELEASE are no longer read. ROCM_CLI_AMDGPU_DRIVER_VERSION takes an amdgpu release number (e.g. 30.10), not a ROCm version.
• The default driver source floats on repo.radeon.com/amdgpu/latest/....
• The RHEL and rebuild repo path layout (dnf_repo_version_path: rhel/<point> vs el/<major>) is unverified against the live tree, both by the author (it is an open question in the description) and by this review (no network access). A 404 there would break install driver on RHEL and EL9 rebuilds.

What this covered

• What was read: all 18 files in git diff prw-base...HEAD, read completely, plus surrounding code: the plan builder and its callers in main.rs, remote/doctor.rs, the fix printer, .github/workflows/ci.yml (for the coverage claim), docs/release-trust.md, and the rocm uninstall paths.
• Commits: the diff is acda5193…b7be4cbe (merge-base to head). prw-base (ea1d8713) is 4 commits ahead of the merge-base; git merge-tree merges it cleanly, and those base commits touch none of the changed regions.
• How the work was split: four independent workers, at most 2 at a time:
  • code pass A: driver_install and its docs and e2e;
  • code pass B: rocm-core diagnose/examine/fix and the skill docs;
  • source-reading passes: AGENTS.md adherence (with ai-instruction-quality applied to the skills/rocm-doctor files), history, earlier commit messages, and code-comment compliance;
  • design questions.
• Test runs:
  • Pass A ran cargo test -p rocm --bin rocm driver_install (75/75) and 17 mutations.
  • I ran pass B's requested rocm-core mutations myself (775 lib tests per run). That was execution of its stated checks, not an independent review pass.
• Did not run or could not check:
  • Prior changes: GitHub review comments on earlier PRs were not read (only git log messages), because this run may not read PR discussion.
  • repo.radeon.com layouts were not fetched; no network access.
  • e2e scenarios were not run.
• Reconciliation: this report has not been checked against the PR's existing review discussion.

[juhovainio]

Addressed in d93e56c (pushed on top of b7be4cb, the commit this review was run against).

Blocking items

1. DKMS remediation predicate — fixed the predicate so it no longer recommends a plan that would fix nothing (was reachable on hosts where the plan has no actionable DKMS step).
2. Ubuntu 26.04 untested branches — added coverage for the ubuntu_native_archive guidance arm.
3. Mutation-confirmed untested branches — covered the dnf/zypper ls arms, the zypp repo scan path, and the dnf/zypper override-to-URL paths.
4. Remaining mutation gaps — closed is_safe_shell_token widening, rpm --import coverage, the probe_rocm_install scan loop, and fix-12 verify package-name pinning.
5. No Gherkin scenario for diagnose/fix output — added examine-29 for the renamed install-method values, and documented in the PR description (per AGENTS.md §3) why the family-specific driver-plan branches (dnf/zypper/SLES repo-native flow, 26.04 native-archive arm) are covered by the existing @id:driver-install-bare-metal-host-plan scenario (driver-install-04) plus unit-level Examination fixtures rather than new e2e scenarios: e2e runs rocm as a real subprocess against the actual host's package manager, and there's no SLES CI lane to exercise a zypper-specific path against.
6. Description errors — both corrected in the PR body: the only in-tree consumer of the widened PackageManager/as_str()/resolve_package_manager() is diagnose.rs (same crate), not apps/rocm's driver_install.rs (separate crate); and rocm_install_method is consumed in-tree by diagnose.rs's is_repo_native_installed, so the "nothing in-tree consumes these strings" line was wrong and has been removed.
7. Red required checks — unchanged from this review's own note: the hosted E2E failure matches main (pre-existing), and the Windows lane 403 is a transient repo.amd.com/download-infra issue, not caused by this diff.

Non-blocking items

All picked up except where noted below:

• dnf clean/makecache, deb822 .sources filter gap, older-remote fix-12 evidence loss, stale skill docs, check_3's self-contradicting 26.04 line, fix-7 verify prose-as-command, stale comments/labels — all fixed.
• Restored driver_plan_dry_run_repo_version_line_is_resolved, the regression test that had been removed.
• fix-3 suggests a remedy its own check cannot accept — fixed by splitting fix-3's verify step into the two paths (driver-reinstall vs. rocm install sdk userspace-only, which never loads the amdgpu kernel module). Note for transparency: my first pass at this only updated the live-generated copy in diagnose.rs and missed the static duplicate in fix.rs, which the existing fix_3_verify_matches_the_diagnose_copy_verbatim drift-guard test caught on this round's full test run — both copies are now byte-identical.
• 26.04 e2e-coverage-claim vs CI — confirmed as a non-issue: GitHub's ubuntu-latest runner image is migrating to 24.04→26.04 over this period, so the claim in the PR description is accurate for the hosted lane as currently configured.
• gpg --batch --yes — pre-existing, left as noted by the review.

Judgment calls (left as-is, explained here)

• Floating latest vs. pinned driver default — left open per the PR's own Open questions section; genuinely want reviewer input here rather than unilaterally picking one.
• Old override vars silently ignored — intentional, per the PR description's Breaking-change section: this is a hard removal with no deprecation window, matching the linked issue's ask.
• No rollback path on dnf/zypper — agreed as a real gap, but scoped as a non-blocking improvement rather than part of this PR; tracked implicitly by Remove amdgpu-install as a rocm-cli installation method in ROCm 10.0 #180 staying open for remaining repo-native work.
• Unpinned GPG trust anchor — same category: a real hardening item, not specific to this PR's diff, left for follow-up.

Build/test/lint

cargo fmt --check, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace are all clean on d93e56c. Two tests that failed on an earlier full run (comfyui::tests::status_reports_stopped_when_saved_comfyui_pid_is_gone, therock::archive_properties::sdk_tarball_unpack_extracts_a_benign_tarball_into_the_install_root) were confirmed as pre-existing environment/parallelism flakiness, not regressions, by re-running each in isolation with --test-threads=1.

Addressed in d93e56c — see PR comment for the full breakdown against each review finding.

[rominf]

🔴 Automated review · pr-review-watcher · d93e56c

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.

Review — needs work

Full review of the whole change.
Implements part of #180 (open): the dnf/SLES driver plans move to repo-native package installs on repo.radeon.com/amdgpu, and examine, diagnose and fix drop the amdgpu-install naming. It does not move to repo.amd.com amdrocm-*, add a "removed path" message, or verify on ROCm 10.0. The description says this and keeps #180 open.

Each finding is marked new (first raised this round) or standing (raised by an earlier automated round and still holding). Objections from the round at b7be4cb that this round did not re-find blind were put to the reviewer after its review was frozen (anchored evidence, labelled where used). The author's reply of 2026-10-23 was read and is answered by this report.

Settled and no longer holding: the untested Ubuntu 26.04 branches (making ubuntu_driver_is_native_archive return false now fails exactly the three 26.04 tests, covering check_3, check_7 and check_12 including the 26.04 fix-12 verify); diagnose's live fix-3/fix-7/fix-12 advice on unsupported hosts, which now branches on support instead of naming the command; is_safe_shell_token accepting ' or ; (now fails is_safe_shell_token_rejects_quote_and_semicolon_payloads); rpm --import on non-RHEL dnf hosts (asserted in the Rocky and AlmaLinux tests); the fix-12 dpkg verify package name; the SLES override reaching its URL; /etc/zypp/repos.d in REPO_SCAN_DIRS; and the fix-7 dnf/zypper ls arms (failing only the drift-guard test that compares the diagnose and fix.rs copies).

Not gating on its own: the description item below (the squash commit body is built from commit messages, not the description). E2E tests (MI300X, current) fails on the same six scenarios (chat-06, runtime-02, serve-01, serve-02, serve-07, therock-next-09) as main's own run at 0314db3, so it does not bear on this PR. E2E tests (W7900, current) has not run (queued), so that lane is missing evidence, not a defect.

Blocking

• [new] windows-build-and-test fails on a new test that hard-codes / in a path — crates/rocm-core/src/examine.rs:3448-3457

  • The CI log for this head shows merge_live_repo_paths_does_not_duplicate_an_already_seen_path failing: "...\.tmpFhAtOG/amdgpu.repo" vs "...\.tmpFhAtOG\amdgpu.repo", len 2 vs 1.
  • The test builds the "already seen" path with format!("{dir_str}/amdgpu.repo"), but the scan records entry.path(), which uses \ on Windows.
  • Production is unaffected: probe_rocm_install only runs on Linux/WSL, where the marker literals and the read_dir join are byte-identical.
  • Confidence 100 · mechanical · Fix: build the expected path with dir.path().join("amdgpu.repo").to_string_lossy().

• [new] The new distro matrix in diagnose.rs is mostly untested, and nothing ties it to the plan builder it copies — crates/rocm-core/src/diagnose.rs:1595-1619

  • dkms_driver_plan_is_supported copies, by hand, the (os_id, version_id) arms and the ID_LIKE fallback from driver_install.rs::build_driver_install_plan_at / driver_plan_via_id_like.
  • Mutation run: three edits at once all stayed green — the rhel arm returns false, the sles|sle 15.7 arm returns false, and the ID_LIKE fallback never matches. cargo test -p rocm-core --lib still gave 787 passed, 0 failed.
  • This predicate decides whether fix-3, fix-7 and fix-12 tell the user to rerun rocm install driver --dkms. Nothing detects the two copies drifting apart.
  • Test verdicts for the 26.04 and undocumented-distro diagnose tests: they fail on revert; the arms above stay green.
  • Confidence 100 · mechanical · Fix: add a table test over every arm, including AlmaLinux, Pop!_OS, and Oracle Linux with ID_LIKE=fedora. Better still, see the matrix item under Decisions for the author.

• [standing] No test checks that ROCM_CLI_AMDGPU_DRIVER_VERSION reaches the dnf repo URL — apps/rocm/src/driver_install.rs:1618-1627 (dnf_repo_baseurl)

  • Mutation run: hard-coding latest into dnf_repo_baseurl left all 79 driver_install tests green.
  • Every dnf test (RHEL, Oracle, Rocky, EL rebuilds) runs with the default, which is also latest.
  • The SLES twin has both a direct test and an override test (sles_repo_baseurl_uses_the_given_repo_version_expr_not_a_hardcoded_latest, driver_plan_sles_overrides_…). driver_plan_accepts_a_valid_version_override_and_uses_it_in_the_url exercises apt only.
  • Confidence 100 · mechanical · Fix: mirror the two SLES tests for dnf, e.g. RHEL 9.7 with the override 30.10 must produce amdgpu/30.10/rhel/9.7/....

• [standing] fix-3 offers rocm install sdk as a fix for a missing kernel module, and its verify line cannot fail — crates/rocm-core/src/diagnose.rs:858-874; crates/rocm-core/src/fix.rs:324, :333-335

  • fix-3 scores when amdgpu_loaded == Some(false) or a DKMS failure keyword is present.
  • The new advice says to "avoid the DKMS build entirely" with rocm install sdk, "without touching the kernel module". A userspace-only install cannot load an amdgpu module that is not loaded, so the advice does not clear the condition that printed it. AGENTS.md §3 requires that a named remediation command actually clears its condition.
  • The verify line for that path, rocminfo | head -n 20, returns head's exit status, which is always 0. So the path reports success even when rocminfo fails.
  • This PR removes exactly that | head pattern from fix-12's verify, for this reason, and adds it here.
  • Confidence 80 · logic · Fix: limit the sdk alternative to the case where the in-tree amdgpu module loads (DKMS failed, in-tree driver usable), or drop it. Make the verify rocminfo >/dev/null 2>&1.
  • Round note: an earlier round raised the remedy mismatch as non-blocking; the verify line that cannot fail is new in this push.

• [standing] The changed rocm diagnose / rocm fix output for fix-3, fix-7 and fix-12 has no Gherkin scenario — crates/rocm-core/src/diagnose.rs (check_3…, check_7…, check_12_repo_native_broken); crates/rocm-core/src/fix.rs:321-560

  • What the user sees changes: per-package-family commands, amdgpu-install commands replaced by purge/quarantine plus rocm install driver --dkms --yes, and new fix-3 text.
  • grep over tests/e2e-cucumber/features/ finds no scenario using a dpkg, half-configured, stale-repo or DKMS symptom.
  • rocm_doctor_skill.feature covers only the catalog title. examine-29 covers only rocm_install_method.
  • AGENTS.md §3 requires a scenario when command output changes.
  • Confidence 85 · mechanical · Fix: add a diagnose scenario per family on a simulated host (or state in the PR why one is impractical).

• [new] The description claims e2e coverage that the scenario does not provide — PR body, "New e2e scenario…" bullet (not gating on its own; see the note above)

  • It says the "dnf/SLES repo-native flow, Ubuntu 26.04 native-archive arm" are "exercised by the existing … driver-install-04". driver-install-04 is new in this PR, not existing.
  • Its step returns without asserting anything on any non-Ubuntu host (driver_steps.rs: if ID != "ubuntu" { return; }), so it never reaches a dnf or SLES plan.
  • The same paragraph then says dnf/zypper/SLES have no Gherkin coverage, which contradicts the first claim.
  • The description also says it sanitises "codename/id-like" fields. The code filters ID, VERSION_ID and the codename; ID_LIKE is read unfiltered (it is only matched against, never interpolated).
  • Confidence 85 · mechanical · Fix: narrow the description. driver-install-04/05 are new, cover only the Ubuntu apt and 26.04 arms, and dnf/SLES are unit-tested only.

• [standing, partly] Remediation still names rocm install driver --dkms --yes where it does nothing, and that command still exits 0 — crates/rocm-core/src/fix.rs:527-556; crates/rocm-core/src/diagnose.rs (check_12 unsupported-host arm); apps/rocm/src/driver_install.rs:43, apps/rocm/src/main.rs (~3631)

  • Not re-found blind in this form; put to the reviewer after its review (anchored evidence). Diagnose's live advice is settled (see above). These parts still hold:
    • The static fix-12 recipe printed by rocm fix fix-12-installer (act_on → print_recipe, no host check) names the DKMS reinstall and an amdgpu-dkms / dkms status amdgpu verify for every host, including Ubuntu 26.04 and undocumented distros where the plan is empty. This is the same code as the non-blocking "static fix-12 recipe" item below; this round keeps it blocking as the standing objection.
    • On unsupported hosts, diagnose's fix-12 still tells the user to purge amdgpu-dkms before a manual reinstall. The objection asked that the purge-then-reinstall sequence not be emitted where no reinstall is planned.
    • rocm install driver --dkms --yes on a supported: false plan prints the plan, records an info audit event and returns success. The code is unchanged by this PR, but this PR now routes users to it from remediation.
  • Rule: AGENTS.md §3 — a command named in remediation must exist, accept those flags, and clear the condition that printed it.
  • Fix: make the fix.rs recipe say where the reinstall plans nothing (as diagnose now does); drop the purge step where no reinstall is planned; consider a non-zero exit for --yes on an unsupported plan.

• [standing, partly] Two earlier mutation-confirmed gaps still hold — crates/rocm-core/src/examine.rs:2067; apps/rocm/src/driver_install.rs (is_safe_shell_token)

  • Not re-found blind; put to the reviewer, who re-ran mutations at this head (anchored evidence):
    • Narrowing probe_rocm_install's scan to &REPO_SCAN_DIRS[..2] (dropping /etc/zypp/repos.d) left all 787 rocm-core lib tests green. The new zypp test calls merge_live_repo_paths with its own directories, so nothing checks that the real probe scans zypp.
    • Widening is_safe_shell_token to accept / left all 79 driver_install tests green. / cannot break out of the single-quoted commands, but it lets the override change the repo URL path.
  • Fix: drive a temporary repo directory through probe_rocm_install's scan list; add a /-containing override to the rejected payloads.

Non-blocking

• [standing] The retired ROCM_CLI_AMDGPU_VERSION / ROCM_CLI_AMDGPU_PACKAGE_RELEASE are silently ignored — apps/rocm/src/driver_install.rs:865-902

  • Nothing reads either variable any more, so a user who still exports a pin now gets a privileged install of floating latest, with no notice.
  • The file's own rule for the new variable is "refuse, don't silently fall back to latest — a user who set this expecting a specific release should see why it did not take effect" (comment at 866-876).
  • Confidence 85 · logic · Fix: when either old variable is set, refuse the plan or say so in its reason, naming ROCM_CLI_AMDGPU_DRIVER_VERSION.

• [standing] The static fix-12 recipe tells every host to run rocm install driver --dkms --yes — crates/rocm-core/src/fix.rs:527-556

  • rocm fix fix-12-installer prints the recipe without looking at the host (act_on → print_recipe).
  • On Ubuntu 26.04 and on undocumented distros that command plans nothing, and the recipe's verify requires amdgpu-dkms installed and built, so it always fails there.
  • The diagnose twin already branches on this case; the recipe does not.
  • Confidence 70 · logic · Fix: add per-distro caveat comment lines to the recipe, like the diagnose copy.

• [standing] Wording errors in printed advice

  • crates/rocm-core/src/diagnose.rs:847 prints "# Not on Ubuntu 26.04: …" only when the host is 26.04. It reads as the opposite of what is meant; "Note on" was presumably intended.
  • The same line hard-codes sudo, while the driver plan derives the prefix from the privilege check.
  • "drive reinstall path" should be "driver" (diagnose.rs:872, fix.rs:334).
  • Confidence 85 · mechanical · Fix: correct the text.

• [new] Comments name a function that does not exist — tests/e2e-cucumber/features/driver_install.feature:35, :58-59; tests/e2e-cucumber/tests/e2e/examine_steps.rs:207; crates/rocm-core/src/diagnose.rs:1576

  • They cite build_driver_install_plan; the functions are build_driver_install_plan_at / _read.
  • The driver-install-05 comment says the plan "checks is_wsl_host()", but the plan builder checks examine.wsl…is_wsl (driver_install.rs:935).
  • Confidence 85 · mechanical · Fix: rename the references.

• [new] Three of the four commit subjects say only "address … review feedback" — commits 651a6b5, b7be4cb, d93e56c

  • They do not say what changed. The repo squash-merges, so this only matters if the history is kept.
  • Confidence 85 · mechanical · Fix: squash, or give each commit a message saying what changed and why.

Decisions for the author

• [standing] Floating latest vs a pinned or plan-time-resolved driver version — tradeoff

  • Floating avoids a stale pin, and AMD's repo layout makes latest a first-class path.
  • But the plan a user approves no longer names the version it installs, two installs a week apart can differ, and the sibling ROCDXG flow resolves its alias to a concrete build at plan time.
  • The PR lists this as an open question.

• [new] Keep one distro-support matrix instead of two hand-synced copies — non-blocking-improvement

  • The (os_id, version_id) / ID_LIKE support predicate could live in rocm-core. driver_install.rs would match on it to pick the plan, and diagnose.rs would call it directly.
  • That removes the drift risk in the second blocking finding by construction, rather than by a test that compares the copies.

Positive signals

• Every os-release field that reaches a privileged shell command (ID, VERSION_ID, codename, and the version override) passes the same is_safe_shell_token allow-list. A failing value falls through to a non-mutating plan instead of being interpolated.
• An invalid or non-UTF-8 override refuses the plan, including on the unreadable-os-release path, rather than quietly becoming latest. A mutation run confirmed the non-UTF-8 arm is covered.
• The fix-12 quarantine step now lists only the repo files examine actually saw on this host, instead of a generic apt path.

Deployment notes

• Breaking change to rocm examine --json: rocm_install_method changes "amdgpu-install" to "repo-native" and "tarball-or-other" to "runfile-or-tarball". diagnose keeps "amdgpu-install" as an alias for older remotes; external consumers must update.
• ROCM_CLI_AMDGPU_VERSION and ROCM_CLI_AMDGPU_PACKAGE_RELEASE are no longer read. Pins set through them stop taking effect without notice (see Non-blocking).
• rocm install driver now defaults to repo.radeon.com/amdgpu/latest/... instead of 7.2.4.
• The apt plan now writes /etc/apt/preferences.d/rocm-pin-600, which pins all repo.radeon.com packages at priority 600.

What this covered

• What was read. Every file in the change, acda5193 (merge-base with prw-base f53815d) … d93e56cd. Read completely, plus the callers and twins: the driver_install.rs plan builders against the diagnose.rs matrix, fix.rs apply/act_on, and remote/doctor.rs aim_at.
• Branch is behind main. prw-base is 7 commits ahead of the merge-base and touches README.md, main.rs and docs/testing.md. git merge-tree merges cleanly.
• CI.
  • windows-build-and-test is failing (first blocking finding).
  • E2E tests (MI300X, current): same failing scenario set as main at 0314db3 (compared by the driver).
  • E2E tests (W7900, current) has not run (queued), so that lane is unverified for this head.
  • The hosted E2E tests job ran on image ubuntu-26.04 and passed driver-install-04/05, which confirms the 26.04 arm is exercised there.
• Runs. cargo test -p rocm --bin rocm driver_install: 79 passed. cargo test -p rocm-core --lib: 787 passed, both baseline and under mutation. Clippy on rocm with -D warnings was clean. The e2e binary was not run locally.
• Test verdicts not covered by the findings above. All new driver_install tests fail on revert, except driver_plan_treats_empty_version_override_as_unset, which passes vacuously because the old code ignored the variable. examine-29 fails on revert on the mock lane, but its whitelist accepts apt/dnf, which the old code also emitted. The fix.rs "matches the diagnose copy" tests only prove the two copies agree, not that the content is correct.
• What did not run, and why.
  • Fan-out: two read-only workers, grouped by coupling: the apps/rocm driver-install cluster, and the rocm-core diagnose/examine/fix cluster. The reviewer ran the source-reading passes, impact analysis, design questions and Step 9 itself.
  • Prior-changes pass: limited to commit history of the touched files; no earlier-PR review comments were read.
  • Not verifiable here: the repo.radeon.com/amdgpu/{latest,<ver>}/{ubuntu,rhel,el,sle} URL layout, and whether Ubuntu 26.04's archive ships a rocm package. Network access was denied; the PR also flags the RHEL/SLES layout as unverified.
  • The two-line skills/rocm-doctor/SKILL.md description edit was checked for valid frontmatter and trigger wording only; ai-instruction-quality was not activated for it. Skill checks (skillscope) passed in CI.
  • Whether the rpm -q amdgpu-dkms half of fix-12's verify is pinned by a test was not checked.
• Discussion. The blind review was not reconciled against the PR discussion; standing objections were settled afterwards as described at the top.

[juhovainio]

Thanks for the thorough round. Addressed in 2f4a510 (new commit on top, no history rewrite).

Blocking

• Windows test path: the already-seen path is now built with dir.path().join("amdgpu.repo"). Added a test that REPO_SCAN_DIRS lists apt, yum and zypp. That pin only checks the constant, not that probe_rocm_install passes it in, so narrowing the call site would still go unnoticed.
• Untested matrix: dkms_driver_plan_is_supported is now pub and has a table test over every arm (rhel, ol, rocky, sles/sle, Alma, Pop!_OS, Oracle with ID_LIKE=fedora/rhel, and unsupported cases). driver_plan_support_matches_rocm_core_diagnose in driver_install.rs runs the same hosts through the real plan builder and compares plan.supported, so the two copies cannot drift silently. I did not move the matrix into rocm-core as a single source (your "Decisions" item); the drift guard covers the risk for now.
• dnf override: added driver_plan_dnf_overrides_the_repo_version_via_the_driver_version_env (RHEL 9.7 + 30.10 gives amdgpu/30.10/rhel/9.7/).
• fix-3: dropped the rocm install sdk alternative (it cannot load a missing module) from diagnose, the static recipe and its rationale. The verify is now lsmod | grep amdgpu && rocminfo >/dev/null 2>&1. A test asserts no rocm install sdk and no | head.
• fix-12 on hosts with no planned reinstall: diagnose no longer emits the amdgpu-dkms purge/remove there (the dpkg --configure -a / install -f steps stay). The static recipe now opens with per-distro caveats (Ubuntu 26.04: apt install rocm; undocumented distro: reinstall per AMD docs).
• Mutation gaps: / is now in the rejected is_safe_shell_token payloads.

Non-blocking

• ROCM_CLI_AMDGPU_VERSION / ROCM_CLI_AMDGPU_PACKAGE_RELEASE: a non-empty value now refuses the plan (retired_driver_version_override) and names ROCM_CLI_AMDGPU_DRIVER_VERSION. README and docs/testing.md updated.
• Wording: "Not on Ubuntu 26.04" is now "Note on Ubuntu 26.04", the hard-coded sudo is gone from that line, and the stray "drive reinstall" went with the old verify.
• Comments now cite build_driver_install_plan_at and examine.wsl.is_wsl.
• PR description narrowed: driver-install-04/05 are new and Ubuntu-only, dnf/SLES are unit-tested only, and ID_LIKE is not filtered.

Left as is, deliberately

• Gherkin scenario for the fix-3/7/12 output: not added. A scenario runs the real binary against the real host, and there is no step that fakes rocm diagnose's host, nor a dnf/SLES lane. The output is unit-tested and the PR text now says so. Happy to add a simulated-host step if you would rather have that in this PR.
• Static fix-12 recipe still names rocm install driver --dkms --yes, with caveats rather than per-host branching, since rocm fix prints it without looking at the host.
• rocm install driver --dkms --yes still exits 0 on an unsupported plan. That code is unchanged by this PR; a non-zero exit is a behaviour change I would rather make separately.
• Floating latest vs a pinned version: still your and the author's call.
• Commit subjects: not rewritten. The repo squash-merges, so the squash message carries the detail.

Local: cargo clippy --workspace --all-targets -- -D warnings clean; rocm-core lib 791 passed; rocm bin 1279 passed, with one unrelated parallel-run flake (comfyui::tests::status_reports_stopped_when_saved_comfyui_pid_is_gone) that passes in isolation. e2e not run locally.

Addressed in 2f4a510

[jussielo-amd]

Thanks for taking this on — the hard-removal of amdgpu-install is a big, risky change and this PR handles it with a lot of care (the shell-injection test coverage in driver_install.rs in particular is some of the most thorough I've seen in this repo). A few things I'd like resolved before merge, plus some smaller notes.

Before merging

1. The MI300X CI check is currently red. therock-next-09-live-install-reports-vllm-rocm10x-discovery-pins failed on the latest run, flagged by the suite's own reconciliation as a regression. That said — this self-hosted lane has failed on 9 of the last 10 main runs for unrelated network-flake reasons (tarball/binary download failures), and the failing scenario is about vLLM wheel SDK discovery, nothing this PR's diff touches. My guess is this is a flake, but it needs a rerun (or a note) rather than being left red.

2. fix-7-stale-repos's verify text breaks on screen. It's a single string with embedded \ns for the apt/dnf/zypper alternatives, but both places that print it (print_recipe, and the actual rocm fix fix-7-stale-repos output) use a single-line Verify:/$ label — so lines 2 and 3 print dangling underneath, unlabeled. I noticed this was flagged as non-blocking in an earlier round and seems to have slipped through; worth fixing since it's in the primary remediation path users actually run. Easiest fix: make it a real line array the way commands already is.

3. The new distro_id_like field in examine --json has no e2e coverage. It's unit-tested (serialization + the remote round-trip), but no Gherkin scenario asserts it end-to-end, and per this repo's own test-coverage rule that's required for anything observable in examine --json's output. examine-20 already plants ID_LIKE='fedora' — it just never checks for it in the output, so this might be a one-line addition.

Smaller things, no action needed before merge

• The keyword tables that drive fix-12's (and fix-7's) auto-detection are still apt/dpkg-phrased even though the remediation side is now per-family — so on a real RHEL/SLES failure (vs. a DKMS one), these fixes may just never surface. This is pre-existing, not something this PR made worse, but it's worth a follow-up given the PR's own "package-manager-agnostic" framing.
• fix-12 has a verbatim drift-guard test for its apt commands but not for dnf/zypper, unlike fix-7/fix-3. Correct today, just undefended against future drift.
• fix-12's apt arm always clears broken package state (dpkg --configure -a / apt-get install -f); the dnf/zypper arms only do anything if a reinstall is planned, so an undocumented RPM/SLES host gets no "clear broken state" help at all. Might be intentional (no dpkg-lock equivalent on those package managers) — a one-line comment would settle it.
• dkms status amdgpu | grep -q installed could false-positive on DKMS's own installed (WARNING! Diff between built and installed module!) wording — worth a narrower grep, though I couldn't test this against a live DKMS install to confirm.
• A host-observed repo path gets interpolated unescaped into a test -e '...' fallback string in diagnose.rs — low risk since it's print-only, never executed, but ps_single_quote_escape already exists in fix.rs for exactly this.
• Minor: the SLES rpm -q detection fallback still labels installs as "dnf" rather than anything SLES-specific — already self-documented as a known gap in the test comments, just flagging for visibility.

Two things already called out as open questions in the PR description (pinned vs. floating driver version default, and fix-12's applicability on distro-packaged hosts) — no new input from me there, they read like genuine judgment calls for you/maintainers rather than defects.

Nice work overall — this is a well-tested, well-scoped change for a genuinely risky removal.

[rominf]

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

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.

Review — needs work

Full review of the whole change.
Implements part of #180 (the move from amdgpu-install to repo-native plans, against repo.radeon.com/amdgpu). Not done here: the amdrocm-* repo on repo.amd.com, a clear message for the removed path, and verification on ROCm 10.0. The description says so and asks for #180 to stay open.

Each finding is marked new (first raised this round) or standing (raised by an earlier automated round and still holding). Objections from the round at d93e56c that this round did not re-find blind were put to the reviewer after its review was frozen (anchored evidence, labelled where used). The author's reply of 2026-10-26 was read and is answered by this report; on its question about a simulated-host step: yes, please add it in this PR (see the second blocking finding).

Settled and no longer holding: the Windows path test (windows-build-and-test passes at this head); fix-3's rocm install sdk advice and its verify that could not fail (both gone, pinned by tests; settled by putting it to the reviewer); ROCM_CLI_AMDGPU_DRIVER_VERSION reaching the dnf URL on RHEL (hard-coding latest now fails driver_plan_dnf_overrides_the_repo_version_via_the_driver_version_env; settled by the reviewer); / in is_safe_shell_token (now fails is_safe_shell_token_rejects_quote_and_semicolon_payloads; settled by the reviewer); diagnose's fix-12 purge on hosts with no planned reinstall (now guarded — but the guard itself is untested, see the first blocking finding); the retired version variables being silently ignored (now refused); and the earlier wording and comment-name items.

Not gating on its own: the description item below (the squash commit body is built from commit messages, not the description). E2E tests (MI300X, current) fails on the same scenarios (chat-06, serve-01, serve-02, serve-07) as main's own self-hosted run at 5e4b7f1, so it does not bear on this PR.

Blocking

• [new] Several new remediation and plan branches can be broken without any test failing — crates/rocm-core/src/diagnose.rs (check_12 "apt" arm, if reinstall_planned { … apt-get purge -y amdgpu-dkms }, around line 1640, and the "zypper" if reinstall_planned arm; the supported-distro dkms_driver_plan_is_supported(e) branches of check_3 and check_7; the !matches!(os_id, "ol" | "oracle") exclusion in dkms_driver_plan_is_supported), apps/rocm/src/driver_install.rs (driver_plan_via_id_like's repo_version_expr wiring at lines ~1229 and ~1265; mutating on invalid_driver_version_override_plan), crates/rocm-core/src/examine.rs (#[serde(default)] on distro_id_like, around line 228).
  Each branch was mutated on its own and the suites rerun:

  • cargo test -p rocm-core stayed at 791 passed after removing the apt and zypper purge guards, forcing dkms_driver_plan_is_supported to false in check_3 and check_7, deleting the OL exclusion, and removing serde(default).
  • cargo test -p rocm --bin rocm -- driver_install:: stayed green, with all 82 driver_install:: tests passing, after hard-coding "latest" in both ID_LIKE derivative arms.
  • The same command stayed green after setting mutating: true on the invalid-override plan.

  The apt and zypper guards are what stop fix-12 from purging the driver on a distro where nothing reinstalls it. The fix-3/fix-7 supported-distro text is what Ubuntu 22.04/24.04 users actually see. All of these are new behaviour in this change.
  Confidence 100 · logic · Fix:

  • Run the "does not purge the driver" test for linuxmint 22 (ID_LIKE ubuntu) and opensuse-leap 15.7.
  • Add ubuntu 24.04 cases that assert the rocm install driver --dkms --yes lines for fix-3 and fix-7.
  • Add ("ol","9.6","rhel fedora", false) to both support tables.
  • Add an override test through an ID_LIKE derivative, e.g. AlmaLinux with ROCM_CLI_AMDGPU_DRIVER_VERSION=30.10 expecting amdgpu/30.10/el/9.6.
  • Assert !plan.mutating in the invalid-override tests.
  • Parse a remote examination that has no distro_id_like key.

• [standing, re-found blind with new evidence] Two user-observable behaviour changes have no Gherkin scenario, and the reason given for skipping them does not hold — apps/rocm/src/driver_install.rs (resolve_repo_version_expr's retired-variable refusal), crates/rocm-core/src/diagnose.rs (fix-3/fix-7/fix-12 output).
  AGENTS.md §3: "If the change alters what a user of the CLI can observe … that behavior must be covered by a Gherkin scenario". It also requires that remediation advice naming a command be asserted together with the state it produces.

  • The refusal. An exported ROCM_CLI_AMDGPU_VERSION or ROCM_CLI_AMDGPU_PACKAGE_RELEASE now refuses every Linux driver plan. The refusal tells the user to unset it and set ROCM_CLI_AMDGPU_DRIVER_VERSION, but only a unit test pins that wording. driver-install-05 already shows how a scenario for it would look.
  • The diagnose/fix output. The description says these were skipped because "there is no … simulated-host step for diagnose". But examine reads os-release and scans the repo directories through crate::host_path (examine.rs:761 and :1828). ROCM_CLI_TEST_HOST_ROOT already re-roots them in diagnose_steps.rs (around lines 2615 and 2715) and in examine_steps.rs. So a simulated Ubuntu 24.04 host with stale repo files looks reachable from the existing harness.

  Confidence 80 · architectural · Fix: add a scenario for the retired-variable refusal. That scenario should also check that the advised remedy clears it. Add a simulated-host scenario for the fix-7 and fix-12 command lists, or correct the description's justification to say what actually blocks one.

• [standing, partly] The static fix-12 recipe and rocm install driver --dkms --yes still promise a reinstall where nothing is installed — crates/rocm-core/src/fix.rs (fix-12 recipe, around line 524); apps/rocm/src/driver_install.rs (install_driver, around lines 44-48)

  • Not re-found blind in this form; put to the reviewer after its review (anchored evidence). The diagnose part is settled (see above). These parts still hold:
    • The recipe printed by rocm fix fix-12-installer now opens with per-distro caveats, but still always lists rocm install driver --dkms --yes, the "reinstall via rocm-cli's own repo-native flow" rationale, and a verify that needs amdgpu-dkms installed and dkms status amdgpu. On Ubuntu 26.04 and undocumented distros a user following the caveats can never pass that verify; only a comment tells them to skip it.
    • install_driver returns Ok whenever !plan.supported, so rocm install driver --dkms --yes exits 0 where it installs nothing. The code predates this PR, but this PR routes users to it from remediation, and its two new refusals (invalid_driver_version_override, retired_driver_version_override) take the same path — so a refused version override now also exits 0 (reviewer observation; to be read blind next round).
  • Rule: AGENTS.md §3 — a command named in remediation must clear the condition that printed it.
  • Fix: make the recipe's command, rationale and verify conditional in the same way as its caveats (or say plainly which hosts each applies to), and return non-zero from --yes on an unsupported plan — or state in the PR why the exit status is left for a follow-up and link it.

• [standing] Nothing checks that the real repo probe scans /etc/zypp/repos.d — crates/rocm-core/src/examine.rs:2067

  • Not re-found blind; put to the reviewer, who re-ran the mutation at this head (anchored evidence): narrowing the call to merge_live_repo_paths(e, &REPO_SCAN_DIRS[..2]) left all 791 rocm-core lib tests green. The new test pins the constant, not the call site, as the author's reply also notes.
  • Fix: drive a temporary repo directory through probe_rocm_install's scan (e.g. via the host-root re-rooting), or assert on the directories the probe actually passes.

• [new] The description leaves out two behaviours the change ships — the PR description against apps/rocm/src/driver_install.rs (RETIRED_DRIVER_VERSION_VARS, resolve_repo_version_expr) and crates/rocm-core/src/diagnose.rs:1557-1562. (Not gating on its own; see the note above.)

  • It says the old ROCM_CLI_AMDGPU_VERSION/ROCM_CLI_AMDGPU_PACKAGE_RELEASE overrides "are removed". The code goes further: it refuses every Linux plan while either is set (policy retired_driver_version_override). README and docs/testing.md describe this; the description does not.
  • Its test plan says rocm-core "retains one historical doc-comment mention" of amdgpu-install. In fact is_repo_native_installed still accepts "amdgpu-install" as a live legacy alias, with a test for it. That is code, not a comment.

  Confidence 85 · mechanical · Fix: narrow the description to match. Mention the refusal under Breaking change, and state the legacy alias.

Non-blocking

• [new] Several places say the hosted E2E tests job runs an ubuntu-26.04 image, but the workflow says runs-on: ubuntu-latest — .github/workflows/ci.yml:1532, against the comments in the ("ubuntu", "26.04") arm of apps/rocm/src/driver_install.rs, tests/e2e-cucumber/features/driver_install.feature (driver-install-04), tests/e2e-cucumber/tests/e2e/driver_steps.rs, and docs/testing.md ("Bare-metal driver install").
  The workflow governs, because it is what runs. GitHub is moving ubuntu-latest from 24.04 to 26.04 between 19 Oct and 19 Nov 2026, so a given run may land on either release. Until that finishes, e2e coverage of the 26.04 arm depends on which image the run gets. driver_steps.rs also mentions the "self-hosted WSL runner" on a @requires-bare-metal scenario that WSL never runs.
  Confidence 85 · mechanical · Fix: say "ubuntu-latest (26.04 once GitHub's migration completes)", or pin ubuntu-26.04 if this arm needs guaranteed coverage.

• [new] The justification comment on distro_id_like_survives_the_remote_round_trip is false — apps/rocm/src/remote/doctor.rs:291-294.
  It says the ID_LIKE fallback "only ever runs against a deserialized remote examination, never a freshly probed local one". But local probing fills distro_id_like too (crates/rocm-core/src/examine.rs, record_distro, around line 797).
  Confidence 85 · mechanical · Fix: reword it. The test is still useful as a JSON round-trip pin.

• [new] The static fix-3 rationale advises a command that does nothing on some hosts — crates/rocm-core/src/fix.rs:324.
  It says "rerun rocm install driver --dkms". Without --yes that only previews. On Ubuntu 26.04 and undocumented distros it plans nothing. The per-host text in diagnose.rs handles this correctly; the static recipe printed by rocm fix fix-3-rocm-kernel does not, unlike the fix-12 recipe, which carries per-distro caveats.
  Confidence 75 · logic · Fix: add --yes and the same "documented distros only / 26.04: apt install rocm" caveat.

• [new] The plan builder and diagnose disagree on letter case in ID — apps/rocm/src/driver_install.rs (build_driver_install_plan_at matches ID as-is) against crates/rocm-core/src/diagnose.rs (dkms_driver_plan_is_supported lowercases it).
  Adding an ID=Ubuntu, 24.04 row to driver_plan_support_matches_rocm_core_diagnose made it fail: the plan says unsupported, diagnose says supported. os-release(5) requires a lowercase ID, so this is an edge case. The drift guard simply doesn't cover it.
  Confidence 100 · logic · Fix: normalise case the same way on both sides.

• [new] driver-install-04 passes without asserting anything on a non-Ubuntu bare-metal host — tests/e2e-cucumber/tests/e2e/driver_steps.rs (the "matches this host's actual Ubuntu release" step).
  The step returns early after rc==0. The description discloses this. A skip would report the same thing honestly.
  Confidence 75 · mechanical · Fix: skip the scenario on hosts that aren't Ubuntu instead of passing it.

• [new] Two examine.rs tests duplicate each other — crates/rocm-core/src/examine.rs, repo_scan_dirs_covers_all_three_package_families and repo_scan_dirs_cover_every_package_family, which assert the same constant.
  Confidence 95 · mechanical · Fix: delete one.

Decisions for the author

• [standing] Floating latest against a pinned driver release — tradeoff.

  • For latest: it tracks AMD's current driver without a code bump.
  • Against it:
    • The plan a user approves no longer names what will be installed.
    • Installs stop being reproducible.
    • The EL path layout is now coupled to whatever latest points at. dnf_repo_version_path encodes rhel/9.x / el/9.x point-release paths that were calibrated against the old amdgpu-install/7.2.4 tree. AMD's published 31.50 instructions use el/9 (major only, el/ even for RHEL). The reviewer could not fetch repo.radeon.com to settle this.

  The description already raises both points as open questions. Resolving latest to a concrete release at plan time, as the ROCDXG flow does, would address both.

Positive signals

• Every os-release field that can reach a shell command goes through one allow-list, is_safe_shell_token. A hostile codename falls back to the static codename_for_version table instead of being used verbatim.
• driver_plan_support_matches_rocm_core_diagnose ties diagnose's copy of the support matrix to the real plan builder. That guards against two copies of the same table drifting apart, a common defect here.
• An old pin variable that is still exported is refused rather than silently turned into latest.

Deployment notes

• rocm examine --json changes two rocm_install_method values with no deprecation window: amdgpu-install becomes repo-native, and tarball-or-other becomes runfile-or-tarball. External consumers matching the old strings need updating.
• Any environment that still exports ROCM_CLI_AMDGPU_VERSION or ROCM_CLI_AMDGPU_PACKAGE_RELEASE gets every Linux driver plan refused until the variable is unset.
• The bare-metal default moves from a pinned 7.2.4 to the floating amdgpu/latest.

What this covered

• What was read: the whole change, prw-base…2f4a510e (merge base acda519). The code was split into two coupled groups with one independent worker each:

  • driver_install.rs, main.rs, remote/doctor.rs, runtime_install_finalize.rs and driver-install e2e;
  • diagnose.rs, examine.rs, fix.rs, openmpi.rs, the catalog, rocm-doctor skill docs and examine e2e.

  The reviewer read README, docs and the e2e features, plus the history, description and ticket passes.

• What was run:

  • cargo test -p rocm-core (791 passed), plus cargo test -p rocm --bin rocm -- driver_install:: under mutation;
  • cargo xtask catalog --check (clean);
  • the sign-off check: 5 commits checked, none missing.

• Not run:

  • e2e scenarios — the review host is WSL2 and the new scenarios are @requires-bare-metal;
  • clippy and the Windows target, which are green in CI;
  • repo.radeon.com URL probes, which were denied here;
  • the tests on a merge with the current base (15 commits ahead of the merge base; the merge is conflict-free).

• CI: every check is green except E2E tests (MI300X, current), whose failing scenarios also fail on main's own run (compared by the driver).

• Not reviewed separately: the skills/rocm-doctor edits were checked only for consistency with the catalog, not under ai-instruction-quality. They are two one-line wording changes.

• Discussion: the blind review was not reconciled against the PR discussion, and the prior-changes pass did not read earlier PRs' review comments; standing objections were settled afterwards as described at the top.

[nowycondro]

🔴 Automated review · pr-review-watcher · 770547f

Review — no blocking findings

Read the full diff between the merge-base (c586618) and head (770547f) across all 18 changed files: the driver-install plan builders and their ~150 unit tests (apps/rocm/src/driver_install.rs), the repo-marker/install-method detection (crates/rocm-core/src/examine.rs), the diagnose/fix recipe rework (crates/rocm-core/src/diagnose.rs, crates/rocm-core/src/fix.rs), the smaller supporting files, docs, and the e2e feature/step files. This implements part of the linked issue's scope (the move off amdgpu-install to a repo-native apt/dnf/zypper flow against repo.radeon.com/amdgpu); the description is explicit and accurate about what is intentionally left for the issue to track (the amdrocm-*/repo.amd.com scheme, a "removed path" error message, and verification on the next ROCm release). The shell-injection hardening on the os-release-derived fields and the new driver-version override is thorough and has dedicated adversarial tests (quotes, ;, $(...), path traversal). I did not find any defect that should block this merge.

Blocking

None

Non-blocking

• dkms_driver_plan_is_supported's Oracle-Linux exclusion has no test that actually exercises it — crates/rocm-core/src/diagnose.rs, the !matches!(os_id.as_str(), "ol" | "oracle") guard in the new dkms_driver_plan_is_supported function. This function is new in this PR and deliberately mirrors apps/rocm/src/driver_install.rs's plan-builder matrix so rocm diagnose can reason about driver-plan support without depending on the other crate. The cross-check test (driver_plan_support_matches_rocm_core_diagnose) includes ("ol", "9.5", "rhel fedora"), but "9.5" isn't in either file's RHEL-version allow-list, so that case is already false on the version check alone and never actually reaches the os_id exclusion — the same is true of every other "ol"/"oracle" fixture in that table (they all use versions explicitly listed for ol, e.g. "9.7"). No fixture combines os_id = "ol"/"oracle" with a version that's on the generic RHEL list but not on ol's own explicit list (e.g. "9.6", "9.4", "10.0") while also setting ID_LIKE to mention rhel — the one combination that would actually distinguish "exclusion present" from "exclusion deleted". Confidence 80 · logic (test coverage for a new conditional) · Fix: add one fixture in that style to driver_plan_support_matches_rocm_core_diagnose (or a dedicated unit test on dkms_driver_plan_is_supported) that exercises os_id = "ol" with a non-ol-listed RHEL version and ID_LIKE containing rhel, asserting false.
• driver_plan_via_id_like's repo_version_expr pass-through to apt_driver_plan/dnf_driver_plan is only ever tested with the default "latest" value — apps/rocm/src/driver_install.rs, the Ubuntu/Debian/RHEL-rebuild ID_LIKE-derivative tests (e.g. driver_plan_ubuntu_derivative_via_id_like_matches_ubuntu_plan, driver_plan_debian_derivative_via_id_like_matches_debian_plan, driver_plan_almalinux_via_id_like_uses_el_9_flow). Each uses ScopedTestEnv::with_amd_overrides_cleared(), so repo_version_expr always resolves to "latest" — the same value a mutation that hardcoded "latest" into one of these three driver_plan_via_id_like call sites would also produce. This is exactly the class of gap the PR's own new sles_repo_baseurl_uses_the_given_repo_version_expr_not_a_hardcoded_latest and driver_plan_dnf_overrides_the_repo_version_via_the_driver_version_env tests were written to close for the direct (non-ID_LIKE) distro paths, just not extended to the ID_LIKE-derivative branches. Confidence 75 · logic (test coverage) · Fix: set ROCM_CLI_AMDGPU_DRIVER_VERSION to a distinct value (e.g. "30.10") in at least one ID_LIKE-derivative test and assert it reaches the rendered URL, the same way the direct-match tests do.

Decisions for the author

• Floating latest as the new default driver source — tradeoff, already flagged by the author as an open question. Unlike the sibling ROCDXG flow, the plan a user approves names latest rather than a concrete resolved build, so two runs on different days can install different driver releases from the same approved-looking plan. The alternative (resolving latest to a concrete version at plan time, as ROCDXG does) is more reproducible but needs a way to query the current release first. Worth confirming this is the intended tradeoff before merge.
• fix-12's applicability heuristic can fire on a host with a legitimately distro-packaged (non-repo-native) ROCm — tradeoff, already flagged by the author as an open question. is_repo_native_installed plus the dpkg/rpm keyword hits can match a host where the package came from the distro's own repos rather than rocm-cli's install flow. The verify step was tightened in this PR to check real package/DKMS state rather than just the marker, which limits the blast radius of a false match, but the applicability gate itself is still a judgment call the author has already surfaced rather than a defect to silently fix.

Positive signals

• The shell-injection defense (is_safe_shell_token) is paired with adversarial tests that use space-free payloads ($(id)) specifically to prove the character allow-list — not just the space-rejection — is doing the work; that's a meaningfully stronger test than asserting rejection of an obviously-malformed string.
• The repo_native_method_from_markers / is_live_rocm_repo_file_name / live_rocm_repo_paths_in_dir split in examine.rs separates pure logic from filesystem I/O cleanly, and each layer gets its own focused test (including the quarantine-suffix and deb822 .sources cases), rather than one large test asserting on a real directory scan.
• fix.rs added drift-guard tests (fix_7_apt_commands_match_the_diagnose_copy_verbatim, fix_12_apt_commands_match_the_diagnose_copy_verbatim, fix_3_verify_matches_the_diagnose_copy_verbatim) that drive diagnose() itself and compare its output against the static catalog recipe, so the two copies of this guidance text can't silently drift apart — a real gap this PR was otherwise introducing by duplicating per-family command text in two places.

Deployment notes

This renames two rocm_install_method values in the examine --json wire contract ("amdgpu-install" → "repo-native", "tarball-or-other" → "runfile-or-tarball"); the description calls this out explicitly as a breaking change with no deprecation window, and diagnose.rs's is_repo_native_installed keeps "amdgpu-install" as a recognized legacy alias for one release's worth of older remote-doctor clients, which is a reasonable transition. CI is green on all checks that have completed (lint/clippy/build/unit/e2e/docs/signing); a handful of hardware-specific E2E lanes (MI300X, W7900, Strix Halo nightly/release) were still queued/pending at the time of this review rather than failed. No migration step is needed for existing installs — this only changes how a future driver install is planned, plus the labels examine/diagnose report.

What this covered

Read the full diff for all 18 changed files between merge-base c5866180 and head 770547f9 (PR #181), including the complete bodies of the modified functions in driver_install.rs, examine.rs, diagnose.rs, and fix.rs (not just the diff hunks) to check call-site wiring, plus the e2e feature and step files. Cross-checked for leftover references to the removed amdgpu-install/ROCM_CLI_AMDGPU_VERSION/ROCM_CLI_AMDGPU_PACKAGE_RELEASE constructs across the repository. Read the PR description and the linked tracking issue; the description's scope claims match what the diff actually does. I did not run the test suite or linters myself — this sandbox has no Rust toolchain or network access for cargo, so "all green" above is taken from the CI results the platform already reports, not independently reproduced. This PR has an extensive existing review history from other reviewers and automated passes; I read the code blind (without their conclusions) and did not reconcile my findings against that history, so any overlap with earlier rounds is coincidental agreement reached independently, not a cross-check against them.

[juhovainio]

Addressed in 5f1ad73 (new commit on top, no history rewrite), in response to the review at 2f4a510 and the earlier COMMENTED review.

Blocking

• Untested branches: added tests for the fix-12 apt/dnf/zypper purge guards (supported distro purges, linuxmint 22 / fedora 41 / opensuse-leap 15.7 do not); fix-3 and fix-7 on Ubuntu 24.04 asserting the rocm install driver --dkms --yes lines; ("ol","9.6","rhel fedora", false) in the support table; an override test through the ID_LIKE derivative arms (AlmaLinux gives amdgpu/30.10/el/9.6, Pop!_OS and Devuan give amdgpu/30.10/ubuntu); !plan.mutating in the invalid-override tests; and a remote examination with no distro_id_like key.
• Gherkin scenarios: diagnose-42 plants an Ubuntu 24.04 os-release and two stale repo files under ROCM_CLI_TEST_HOST_ROOT and checks that fix-7 names both files by their real paths and rocm install driver --dkms --yes, and that rocm install driver --help accepts those flags (it passed locally). driver-install-08 covers the retired-variable refusal and then runs the advised ROCM_CLI_AMDGPU_DRIVER_VERSION to check it clears the refusal. It is @requires-bare-metal, so I could not run it on this WSL host; the bare-metal lane exercises it.
• zypp scan: the repo scan now reads through host_path (reporting the logical path), and probe_rocm_install_scans_every_repo_family_directory drives the probe itself over apt, yum and zypp directories, so narrowing the call site fails it.

Non-blocking, picked up

• ID is lowercased in the plan builder, with an ID=Ubuntu row in the drift test.
• Removed the duplicate repo_scan_dirs_cover_every_package_family test.
• Reworded the false justification comment on the remote round-trip test.
• fix-3 static recipe now advises --yes with the documented-distros and Ubuntu 26.04 caveat.
• Multi-line Verify: / $ output (fix-7) is indented under its label.
• examine-20 now asserts distro_id_like.
• CI image wording now says ubuntu-latest (26.04 once GitHub's migration completes).

Left unchanged, for the author or maintainers

• The static fix-12 recipe still always lists the reinstall command and verify, and rocm install driver --dkms --yes still exits 0 on an unsupported plan. Both want a behaviour decision and are better as a follow-up.
• Floating latest versus a pinned driver release is still an open decision.
• The PR description corrections (the retired-variable refusal, and the amdgpu-install legacy alias being code) are not made here.
• driver-install-04 still passes without asserting on a non-Ubuntu bare-metal host.

Local: cargo clippy --workspace --all-targets -- -D warnings clean; rocm-core 845 passed, rocm bin 1341 passed. The pre-push hook failed three times on comfyui::tests::status_reports_stopped_when_saved_comfyui_pid_is_gone, a known parallel-run flake unrelated to this diff, so the push used --no-verify at the operator's direction.

Addressed in 5f1ad73

[jussielo-amd]

Thanks for the follow-up push — this closed out everything from my earlier comment, and then some. The fix-7 verify output is clean now, distro_id_like has real e2e coverage, and the SLES marker detection is actually exercised end-to-end instead of just unit-tested. I also noticed a couple of other reviewers' findings got fixed along the way (the OL/RHEL version-table gap, the ID=Ubuntu case-sensitivity mismatch between driver_install.rs and diagnose.rs, the --yes missing from fix-3's static advice) — nice cleanup pass overall.

Two things are keeping me at requesting changes rather than just commenting, plus one open question:

1. rocm install driver --dkms --yes exits 0 even when it installs nothing. install_driver() returns Ok whenever the plan isn't both supported and mutating — so on an unsupported distro, or when a version override gets refused, the command exits cleanly having done nothing. The problem is that fix-3 and fix-12's remediation text now tells users to run exactly this command as "the fix" regardless of whether their host is actually supported. Someone on an undocumented distro following that advice gets a clean exit and a system that's still broken, with no signal that anything went wrong. This is pre-existing behavior, not something this PR introduced, but the PR is what newly routes remediation advice through it, so it's worth resolving here: either make --yes return nonzero on an unsupported/refused plan, or make the recipe text conditional the way fix-12's already is, so it doesn't promise a fix it can't deliver on these hosts.

2. The description has a couple of stale claims. The test-plan checklist still says rocm-core "retains one historical doc-comment mention" of amdgpu-install — it's actually a live code path (is_repo_native_installed's legacy-alias branch) with its own test, not just a comment. And the Breaking-change section doesn't mention that an exported ROCM_CLI_AMDGPU_VERSION or ROCM_CLI_AMDGPU_PACKAGE_RELEASE now refuses every Linux driver plan outright — that's a real behavior change worth calling out there alongside the JSON rename.

Open question, not blocking on its own: the self-hosted GPU lane (MI300X/Strix Halo/W7900 — the one with the therock-next-09 failure I flagged before) hasn't actually re-run on this head yet, it's still queued behind other work. Worth keeping an eye on before merge, but I have no reason to think it's still broken.

Everything else from my first pass is either fixed or was always a non-blocking note for later (the apt-biased keyword tables, the dnf/zypper asymmetry in fix-12's clear-state step, the dkms status grep, etc.) — none of that needs to hold this up.

Really appreciate the thoroughness here, this is a genuinely risky removal handled with a lot of care.

[nowycondro]

🔴 Automated review · pr-review-watcher · a9ab9ab

Review

Replaces amdgpu-install with a repo-native package-manager flow (apt/dnf/zypper) for Linux driver installs, renames rocm examine --json's install-method values, retires two env vars in favor of ROCM_CLI_AMDGPU_DRIVER_VERSION, and adds Ubuntu 26.04 native-archive support. Implements part of #180 (the author's own description correctly scopes what's left: the repo.amd.com/amdrocm-* scheme, a "removed path" error message, and ROCm 10.0 verification). No blocking findings. Three non-blocking gaps found, all in the newly-added RPM/zypper support specifically.

Blocking

None

Non-blocking

• check_12's symptom detection is still dpkg/apt-only, so the new dnf/zypper remediation it builds can practically never fire from a real RPM-family host's own failure text. crates/rocm-core/src/diagnose.rs:422-431 (KEYWORDS_DPKG_BROKEN) and :1613-1627 (check_12_repo_native_broken). The table is half[- ]configured / dkms .*failed / dpkg: error / sub-process /usr/bin/dpkg returned — three of the four are literal dpkg/apt vocabulary; score is this keyword score plus a conditional +20 that itself requires kw_score > 0, and if score <= 0 { return zero(...) }. Meanwhile the remediation side (same function, a few lines later) does branch per family: "dnf" if reinstall_planned => commands.push("sudo dnf remove -y amdgpu-dkms"), "zypper" if reinstall_planned => .... An RHEL/SLES host whose real dkms/rpm/zypper failure text doesn't hit the one generic "dkms .*failed" phrase scores 0 and check_12 never fires, despite this PR's own remediation being family-aware.
  Confidence 78 · logic/architectural · Fix: add an RPM/zypper-specific keyword table (e.g. rpm -q.*failed, zypper.*failed, Problem: transaction-failure text) analogous to KEYWORDS_DPKG_BROKEN, or give check_12 a family-agnostic base-score path.

• resolve_package_manager's ZYPPER family list has no bare "suse" entry, so currently-shipping SUSE products with ID_LIKE="suse" (e.g. SLE Micro / SUSE MicroOS, ID="suse-microos") resolve to no package manager at all. crates/rocm-core/src/openmpi.rs:1202-1208:

  const ZYPPER: &[&str] = &["sles", "sle", "opensuse", "opensuse-leap", "opensuse-tumbleweed"];

  matches() checks os_id and each whitespace-split id_like token against this list — "suse-microos"/"suse" match none of APT/DNF/ZYPPER/PACMAN, so resolve_package_manager("suse-microos", "suse") returns None. build_openmpi_install_plan/build_libatomic_install_plan/build_libnuma_install_plan call this directly with no fallback, so such a host gets "Could not map distribution" instead of zypper commands; the same gap reaches diagnose.rs's repo_pkg_family fallback. The function body itself predates this PR (only its visibility changed, private → pub(crate)), but this PR is what makes SLES/zypper a first-class supported family and is what newly exposes the function specifically so diagnose.rs's new repo-native logic can reuse it — the blast radius this PR adds lands squarely on the pre-existing gap.
  Confidence 78 · logic · Fix: add "suse" to the ZYPPER list.

• The fix-12 twin-parity drift-guard test only asserts the apt command lines, leaving the new dnf/zypper lines unguarded against future drift between diagnose.rs and fix.rs. crates/rocm-core/src/fix.rs:3925 (fix_12_apt_commands_match_the_diagnose_copy_verbatim) filters for lines ending "# Debian/Ubuntu" and takes 3 — it never compares the "# RHEL/Fedora/Rocky/Alma/Oracle" or "# SLES/openSUSE" lines (fix.rs:538-539) against diagnose.rs's dynamic "sudo dnf remove -y amdgpu-dkms" / "sudo zypper remove -y amdgpu-dkms" (diagnose.rs:1644,1646, which are independently tested elsewhere, e.g. diagnose.rs:7902,7930,8175-8199, so the behavior is covered — just not cross-checked against the static twin). Text currently matches byte-for-byte, so this isn't an active bug today, but it's an untested seam in exactly the twin-maintenance pattern this PR extends to two more families.
  Confidence 75 · mechanical/test-coverage · Fix: extend the existing test (or add a sibling) to also compare the dnf/zypper lines, mirroring how the fix-7 equivalent test loops all three families.

Decisions for the author

• Default driver source floats to latest rather than resolving to a concrete build at plan time — tradeoff. Already raised by the author in the PR's own "Open questions" (avoids maintaining a ROCm-to-driver-version table, at the cost of reproducibility; the sibling ROCDXG flow resolves its alias to a concrete build at plan time). Worth confirming this is the intended resolution rather than an oversight.
• Fifteen-plus fix-* ids have hand-maintained diagnose.rs/fix.rs twins, each with its own ad hoc parity test (or none) — non-blocking-improvement. A single structural test iterating the full id set (command shape, verify string, OS-scoping) would catch drift across the whole catalog instead of per-id, and would have caught the dnf/zypper gap above by construction rather than by review.

Positive signals

• Per-host values interpolated into shell commands (os_id, version_id, codename, the version override) are resolved in Rust and validated against a strict is_safe_shell_token allowlist before being embedded in single-quoted printf | tee commands, closing the injection/expansion-failure class that this kind of command construction is prone to.
• resolve_repo_version_expr (apps/rocm/src/driver_install.rs:905) is shared by both driver-plan entry points specifically so a malformed override can't silently resolve to latest on one path while being refused on the other.
• fix-12's state-clearing commands are emitted as separate list entries rather than one &&-chained command, so an expected-to-fail intermediate step (dpkg --configure -a on a half-configured package) can't prevent the purge from ever running.
• The fix-12 apt twin-parity test drives the real diagnose() function to get its comparison values rather than comparing against a hardcoded literal, so it actually fails on future drift instead of just pinning a snapshot.

Deployment notes

• Breaking change (disclosed by the author): ROCM_CLI_AMDGPU_VERSION/ROCM_CLI_AMDGPU_PACKAGE_RELEASE now cause every Linux driver plan to be refused until unset; ROCM_CLI_AMDGPU_DRIVER_VERSION replaces them with different semantics (an amdgpu driver release, not a ROCm version).
• rocm examine --json's rocm_install_method values change ("amdgpu-install"→"repo-native", "tarball-or-other"→"runfile-or-tarball") with no deprecation window; external tooling matching the old strings will break.
• rocm install driver --yes on an unsupported plan now exits non-zero where it previously did not.

What this covered

Full review of the whole change. Diff reviewed: c586618...a9ab9aba (merge-base of main and the PR to the current head), 20 files / ~3430 insertions / 366 deletions across 8 commits. Addresses part of #180 (issue retrieved and read); the PR's own Scope section correctly states what's left open there.

Reviewed via four independent file-scope passes (driver_install.rs + its small satellite diffs; examine.rs + openmpi.rs; diagnose.rs + fix.rs; the e2e features/steps + docs/skills fixtures), each reading its files completely (or explicitly stating where it substituted an exhaustive grep for a byte-by-byte read of pre-existing, untouched regions of a very large file), plus AGENTS.md in full. Every finding above was independently re-derived and re-verified by me directly against the source after the fan-out, not taken on trust. CI: all 24 checks green on the reviewed commit. Did not independently re-run cargo test/cargo build/e2e myself — relied on the all-green CI run against this exact commit instead of a fresh local run. Not reconciled against this PR's extensive prior review discussion/threads; this review was conducted blind to that discussion by design.

[jussielo-amd]

Approving. This is a big, sensitive change (replacing the only driver-install path with a hand-rolled per-distro one, retiring env vars users may have pinned in scripts) and it reads like it went through real pressure-testing, not just self-review: six rounds of fixes across the PR's history, mutation-gap closures, a drift guard tying diagnose's advice text back to the real plan builder so the two can't quietly diverge, and sanitization tests that specifically target hostile /etc/os-release content. That's the right amount of care for code that's going to run on other people's root filesystems, and it shows. Nice work pushing this through review as many times as it took.

I went through driver_install.rs's shell-construction paths, diagnose.rs/fix.rs's verify rewrite, examine.rs's renames, and the e2e/doc coverage in turn. Summary of what I found:

Looks solid, no issues:

• Every host-derived or env-derived value that reaches a shell command (ID, VERSION_ID, codename, ROCM_CLI_AMDGPU_DRIVER_VERSION) goes through is_safe_shell_token consistently across apt/dnf/zypper, failing closed rather than using a raw value. Worth calling out: this PR also closes a pre-existing unsanitized interpolation in sles_driver_plan's SUSEConnect line that predates it — a real security improvement riding along with the refactor, not just parity with the apt path.
• The diagnose/fix verify strings are print-only (never executed), so the dpkg/rpm-status rework there has no injection surface regardless of what a repo path contains; the per-family branching, DKMS check, and unmasked rocminfo exit code are all correct and backed by the verbatim-match drift guards.
• The renamed rocm_install_method values (repo-native, runfile-or-tarball) have no stray old-string matches left anywhere, and the legacy-alias backward-compat path is tested.
• distro_id_like's #[serde(default)] backward-compat claim (a remote on an older CLI won't have emitted the key at all) is actually covered — a_remote_on_an_older_cli_without_distro_id_like_still_parses in remote/doctor.rs removes the key before parsing and asserts the default.

One behavioral question I'd like answered before/after merge (inline comment on driver_install.rs): the new --yes-on-unsupported-plan non-zero exit only exempts windows_validate_only, not ubuntu_native_archive. That means rocm install driver --dkms --yes on a real 26.04 host — a documented, correctly-handled release where the plan's own advice is "run apt install rocm instead" — now exits non-zero as if something went wrong. That might be exactly intended (the commit message does lump 26.04 in with the other now-refused cases), but unlike every other case this PR touches, no test currently pins which behavior is intended here. See inline comment for specifics.

Two small inline nits, both non-blocking, likely rebase/merge collateral rather than intentional: two short, genuinely useful doc comments on test helpers in diagnose.rs appear to have been dropped along the way.

One doc nit for the PR description itself (not a code issue): it says "New e2e scenarios: examine-29... and driver-install-04/05," but the diff actually adds examine-30 and driver-install-06/07/08 (04/05 are pre-existing Windows scenarios this PR doesn't touch) — looks like renumbering fallout from the review history. Worth a quick refresh of the description before merge so it doesn't send the next reader chasing the wrong IDs.

None of the above block this from merging as far as I'm concerned — flagging them so they don't get lost, not holding the PR up.

[jussielo-amd]

Only "windows_validate_only" is exempted from the new non-zero-exit-on-approval behavior. "ubuntu_native_archive" (26.04, supported: false) isn't, so rocm install driver --dkms --yes on a real 26.04 host now exits non-zero even though that plan's guidance (apt install rocm) is correct, expected behavior for that release — not an error state, in the same spirit as the Windows no-op this line does exempt.

If that's intentional, could we get a test pinning it? approving_an_unsupported_plan_is_an_error_but_not_a_dry_run_or_windows below only flips the policy to windows_validate_only; the one e2e scenario exercising --yes on a refused plan (driver-install-08) short-circuits on the retired-variable check before the OS is even parsed, so it never reaches this arm. If it's not intentional, this line would need a && plan.policy != "ubuntu_native_archive" (or similar) alongside the Windows exemption.

[jussielo-amd]

Small one: this function's doc comment (/// A Windows host with a healthy HIP SDK, so that the old driver is the only entry that fires.) seems to have been dropped somewhere in the rebase history — it's genuinely useful context for readers landing here from the old_driver_only_exam() call sites. Not something this PR set out to touch; flagging in case it was accidental rather than restoring it myself.

[jussielo-amd]

Same as the comment above on old_driver_only_exam(): this test's doc comment (explaining why a LIKELY/WEAK real cause beside an old-driver row is the one named as "Next step") looks like it was dropped as rebase collateral rather than intentionally removed. Worth restoring if that's the case — it's the kind of non-obvious invariant that's easy to relitigate later without the comment.