[jussielo-amd]

#544 merged into main. This PR was rebased directly onto main: the commits it had inherited from fix/os-release-one-parser were superseded by #544's squash merge and dropped, keeping only this PR's own contribution.

Fixes #545.

The problem

os-release(5) says to read /etc/os-release and, if that file does not exist, fall back to /usr/lib/os-release. Every reader in rocm-cli read only /etc/os-release, so on a host that ships the file only at /usr/lib/os-release (minimal or container images, some distro variants), the distro read as unknown: rocm examine reported nothing, and rocm install driver planned nothing.

The fix

Adds one shared helper, hardware_root::read_os_release, that tries /etc/os-release then /usr/lib/os-release, parameterized by a path-resolution closure, and falls back only when the primary is absent — never for another read error (permission denied, a directory in its place), so a merely-unreadable primary is never silently replaced by a possibly stale fallback. It returns an OsRelease enum (Found/Absent/Unreadable) rather than a bare string, so a file that exists but could not be read is reported as itself — a new probe_failures entry and status: degraded from rocm examine, and a dedicated unreadable_os_release policy from the install plans — distinctly from both a malformed line and an unknown distribution. Before this, that case was silently folded into an empty, unreported distro.

#544 had already consolidated the original read sites down to two functions that do all the in-process reading (examine.rs/host_gpu.rs's host_path-routed distro probes, and openmpi.rs's/runtime_install_finalize.rs's real-host package-manager readers); wiring the fallback in required changing those two, plus a seventh site this PR's own review caught: the WSL remote-distro shell probe, which runs in the WSL guest rather than this process and so carries its own embedded shell copy of the same existence-based fallback. Every message that names an unreadable line or file now names the candidate actually read (/etc/os-release or /usr/lib/os-release), not always the former.

Tests

• Resolver-injected unit tests for the shared helper itself (primary found, primary absent with fallback present, neither present, and each candidate existing but erroring for a reason other than absence) — deterministic, no real-host dependency.
• Per-call-site unit tests (the driver install plan, the OpenMPI/libatomic/libnuma plans and hints, rocm examine's probe failures, and the distro name) pinning that a fallback-sourced read plans/reports identically to a primary-sourced read with the same text, that a malformed line read through the fallback path names the fallback path rather than a hardcoded /etc/os-release, and that a present-but-unreadable file is reported as itself rather than as an unknown distribution.
• A WSL-host unit test confirming a locally-unreadable read never reaches the Unreadable branch there: WSL has its own guest-side probe, so the local Rust-side read is never authoritative for it.
• A shell-level test for the WSL probe's own fallback logic, run for real under /bin/sh, confirming existence is not readability — see its own doc comment for the one case it cannot cover under CI without root (a genuinely permission-denied primary), which predates this PR and is out of scope here.
• Two new cucumber scenarios reusing the simulated-host-root infrastructure: examine-27 plants only /usr/lib/os-release to exercise the fallback end to end, and examine-28 plants a directory in place of /etc/os-release (a non-NotFound error needing no root privilege) to exercise the degraded verdict and failure message. (Numbered 27/28, not 24/25: fix(rocmd): drain child pipes concurrently so output size stops faking timeouts #526's amd-smi telemetry scenarios claimed 24-26 in the same feature file first.)
• No new test for the real-host package-manager readers' fallback specifically: they intentionally read the actual machine, and there's no safe way to make a dev/CI host's real /etc/os-release disappear to exercise it. Covered by the shared helper's own unit tests instead — the same gap fix: read /etc/os-release with one parser that matches sh #544 already states for this exact split.

Docs updated in the same change: hardware_root.rs's module doc, docs/release-trust.md's Simulated Host Root section, and docs/testing.md's /etc/os-release section.

Scope

openmpi::plan_for_os_release — a non-test pub fn with no production caller, made redundant by the OsRelease-aware seam this PR adds — is deleted. install_hint_for and build_driver_install_plan stay as bare-text test helpers: they're load-bearing for dozens of unrelated test scenarios (privilege escalation, WSL plans, AMDGPU version overrides, quote-style parsing), not redundant plumbing, so collapsing them is left to a follow-up (#690) rather than bundled in here.

The two verify: hint strings in fix.rs/diagnose.rs (shown to a WSL user as manual-verification advice) are now fallback-aware too, since they were the last os-release readers in the tree without it.

Verification

• cargo build --workspace — clean.
• cargo test --workspace --all-targets — every file this PR touches passed cleanly.
• cargo clippy --workspace --all-targets -- -D warnings — clean.
• cargo fmt --all -- --check — clean.
• cargo xtask check-crate-edges — clean, no new cross-crate dependency edges.
• tests/feature_naming.rs — clean (new scenario is sequential, unique, feature-qualified).
• cargo xtask e2e -- -n 'examine-2[0-3] |examine-2[7-8] ' — 6 scenarios, 22 steps, all passed against a live run.

[Copilot]

🟡 Changes recommended

Non-absence read failures incorrectly trigger fallback, and the WSL distro probe and an existing host-dependent test remain inconsistent with the new behavior.

3 open findings

• Update host-read test for /usr/lib/os-release fallback · New
• Apply os-release fallback to WSL remote distro probe · New
• Fallback on missing os-release only, not other read errors · New

What changed in this PR

Adds /usr/lib/os-release fallback support to the shared distro-detection paths introduced by dependent PR #544.

Changes:

• Adds a shared fallback reader and integrates it with host probes and package planning.
• Adds unit and end-to-end fallback coverage.
• Updates testing, trust, and CI documentation.

File	Description
.github/​workflows/​ci.yml	Documents fallback test coverage.
apps/​rocm/​src/​runtime_install_finalize.rs	Uses the shared reader for package plans.
crates/​rocm-core/​src/​examine.rs	Adds fallback-based examination and tests.
crates/​rocm-core/​src/​hardware_root.rs	Implements the shared fallback helper.
crates/​rocm-core/​src/​host_gpu.rs	Uses fallback for distro naming.
crates/​rocm-core/​src/​openmpi.rs	Uses fallback for install hints.
docs/​release-trust.md	Documents simulated-root behavior.
docs/​testing.md	Documents fallback behavior and tests.
tests/​e2e-cucumber/​features/​examine.feature	Adds the fallback scenario.
tests/​e2e-cucumber/​tests/​e2e/​examine_steps.rs	Plants fallback-only test data.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[tomastola]

These findings were reviewed at commit 6046b8437e9e0c6bd78de823e4462b86887a30ac, which is no longer on this PR: the head has since moved to 838a521c (the branch was rebased or force-pushed, and the old commit is not fetchable). They are filed as a change request at the engineer's request, but they are unanchored and were not re-checked against the new head. Several may already be fixed or may no longer apply; in particular the new head sits on top of main's "one parser that matches sh (#544)" change, which I did not review. Verify each against the current code before acting.

Review — needs work (at 6046b84)

Full review of the whole change (base ee3131be … head 6046b84, PR #675, 15 files).
The PR implements the /usr/lib/os-release fallback. It also swaps in a new strict os-release parser, which is a larger change than the title says. No ticket is named. The PR description was not read (see What this covered).

Blocking

• Parse errors name /etc/os-release even when /usr/lib/os-release was read. crates/rocm-core/src/examine.rs:763, crates/rocm-core/src/host_gpu.rs:~1915, crates/rocm-core/src/openmpi.rs:844 and ~899, apps/rocm/src/driver_install.rs:853.
  hardware_root::read_os_release returns only Option<String> (hardware_root.rs:130), so the path it read is lost. On a machine that has only the fallback file, which is the case this PR exists for, a bad line tells the user to fix a file that does not exist. The new tests pin that wording as correct: an_unreadable_os_release_names_its_line_and_plans_nothing (driver_install.rs) and an_unreadable_file_plans_nothing_and_names_its_line (openmpi.rs). No test covers a malformed fallback file.
  Confidence 85 · logic · Fix: return the path read alongside the text, and have every message use it.

• An unreadable /etc/os-release is still reported as an unsupported distro. apps/rocm/src/driver_install.rs:38, apps/rocm/src/runtime_install_finalize.rs:207 and :382, openmpi.rs real_os_release(), examine.rs record_distro (~756), host_gpu.rs detect_distro_name (~1888).
  read_os_release returns None when /etc/os-release exists but cannot be read (permission denied, a directory, invalid UTF-8) and when neither file exists. Every caller turns that into empty text with unwrap_or_default(), and empty text parses as Ok(empty). The driver plan then says "no commands were guessed for this distro". The openmpi, libatomic and libnuma plans say "Could not map distribution (ID=)". examine reports an empty distro with no entry in probe_failures. detect_distro_name returns None and distro prints <unknown> with no reason.
  The PR adds line-level diagnostics for a bad line but not for a file that cannot be read, which is the same class of fault it set out to close. The Result returned by runtime_install_finalize::read_os_release is dead, because all three callers discard the error.
  Confidence 80 · logic · Fix: carry the read error to the reporting sites.

• Nothing pins the fallback at the install-plan call sites. crates/rocm-core/src/openmpi.rs (real_os_release() and the three *_install_hint() functions), apps/rocm/src/runtime_install_finalize.rs:2094-2130.

  • Revert check, verdict 1: install_hint_is_non_empty_and_actionable passes with the old /etc-only read, because the manual hint also contains "install".
  • read_os_release_ignores_a_simulated_host_root: it compares read_os_release() with hardware_root::read_os_release(...), which is the same function, so for the fallback it asserts nothing. It also only compiles under e2e-test-hooks.
  • Reverting only runtime_install_finalize::read_os_release to read /etc/os-release: the apps tests still pass on any host that has /etc/os-release.
  • Where the fallback is tested: only the hardware_root unit tests and the examine path (examine-24 and os_release_reads_fall_back_to_usr_lib_when_etc_is_absent) exercise it.
  • Bypass of host_path: no test checks that openmpi.rs's own real_os_release() bypasses it.
    Confidence 70 · logic.

• Dangling doc reference to rocm_core::detect_distro_name. apps/rocm/src/runtime_install_finalize.rs:158.
  The function is pub(crate) in host_gpu.rs:1888 and is not re-exported in lib.rs, so the path in the comment does not resolve.
  Confidence 85 · mechanical · Fix: reference the real call path, or drop the path.

• The new test try_os_release_sources_the_fallback_only_when_the_primary_is_absent claims a branch it cannot fail on. crates/rocm-core/src/examine.rs:~5750 and the TRY_OS_RELEASE_FN doc.
  The doc says a permission-denied primary is not replaced by the fallback, but the test covers only an absent primary and a present one. Changing -e to -r in the shell function would still pass.
  Confidence 65 · mechanical · Fix: add an unreadable-primary case, or drop the claim.

• The E2E tests check was red. CI run 38545719607, job 115793891572.
  The failing scenario is chat-06 (chat tool definitions). Its step timed out on GET /v1/models, 18.8 s and 10.5 s against a 10 s budget. serve-01 and serve-02 failed as expected xfails. All five new examine-20 to examine-24 scenarios passed. The chat path is untouched and does not read os-release, so this is probably a flake, though I did not see a passing run on the base commit. A failing check blocks regardless until it is re-run green. (main has since landed a chat-06 stall fix, #662, which the new head includes.)
  Confidence 80 that it is unrelated · Fix: re-run the job.

Non-blocking

• A single bad line now voids every field. crates/rocm-core/src/os_release.rs parse, with consumers in examine.rs:~731, host_gpu.rs:~1895, driver_install.rs:~849 and openmpi.rs.
  The old readers took ID, VERSION_ID and the name line by line and skipped what they did not understand. The new parse is whole-file and fail-closed. Files that used to work and now fail: CRLF files, files with a BOM, an unrelated unquoted value such as ANSI_COLOR=0;31, and a \r-only blank line. The module documents this as deliberate, and I found no parser defect against os-release(5). The cost lands on hand-edited or Windows-edited files in WSL. The PR title does not say it widens the behaviour change.
  Confidence 55 · architectural.

• Dead code. crates/rocm-core/src/openmpi.rs:882, impl Display for InstallHint: no caller formats an InstallHint with {}.
  The Err arm of plan_for_os_release is unreachable from the hint path, because install_hint_for checks and returns first. The file is parsed twice for each hint.
  Confidence 60 · mechanical.

• docs/testing.md is garbled and overclaims. The paragraph "The two reads this covers are os_release.rs's own…" is wrong, because os_release.rs does no file reads. The two reads are the examine.rs and host_gpu.rs call sites. The same section says every reader goes through hardware_root::read_os_release, which is not true of the WSL remote probe, which has its own shell copy.
  Confidence 65 · mechanical.

Decisions for the author

• Whole-file strict parse (fail-closed) versus the old tolerant per-line read — tradeoff. Fail-closed means a malformed file is refused outright, never planned from a half-read line, and the one rocm_core::os_release parser keeps examine and the driver plan in agreement. The cost is that examine goes degraded and the install plans refuse on files that used to work (see the first non-blocking item). Confirm this is intended, and consider saying so in the title and description.
• Fall back only on NotFound — tradeoff. This matches the -e shell check, and a permission-denied primary is not silently replaced by a possibly stale fallback. The price is the unreadable-file silence in blocking finding 2. Confirm that is deliberate.

Positive signals

• Both fallback readers route through hardware_root, so a simulated host root cannot leak to the real host. Both candidate paths resolve under the root.
• The examine-24 scenario and os_release_reads_fall_back_to_usr_lib_when_etc_is_absent both fail if the fallback is reverted. I traced this by reading the code; I did not run it.
• Checking -e rather than readability in the WSL shell probe is a sound choice and is documented.

Deployment notes

None. There is no migration or new dependency. The user-visible changes are the new unreadable_os_release plan policy and the probe_failures text.

What this covered

I read the full diff and the full text of os_release.rs, hardware_root.rs, openmpi.rs, the e2e files, the docs and the CI hunks. I read the changed hunks and surrounding code of examine.rs, host_gpu.rs, driver_install.rs and runtime_install_finalize.rs; I did not read those four files end to end. The review was split across three workers by coupling. Nothing was built or run, so no finding rests on a run. Review base: ee3131be…6046b84.
Did not run: the history and prior-changes passes. Step 9 (description and ticket) was not done: I did not read the PR description, and no ticket is named.
Not reconciled against any discussion here.

[juhovainio]

Reviewed this (closes #545) with a fan-out correctness pass plus a separate Standards/Spec pass. The core fix is solid: one shared helper (hardware_root::read_os_release) that falls back from /etc/os-release to /usr/lib/os-release only on NotFound (never masking a real permission error with a stale fallback), wired into every one of the six original call sites plus a 7th (a WSL shell probe) the author's own second commit caught and fixed. CI is fully green, tests exist at all three levels, and the two-commit shape (fix, then review-feedback response) is honest about what changed when.

Requesting changes on one real test-quality gap and a couple of now-stale error strings, left as inline comments below - none of these affect runtime behavior, but a test that can't fail and an error message that now says less than it knows both deserve a quick pass before merge:

1. The assert_eq! in runtime_install_finalize.rs's simulated-root test now compares the function under test's output against the exact same helper call it's built from (same hardware_root::read_os_release, same identity resolver) - it'll pass even if the wrapper's .context(...) or path logic breaks, since it's no longer checking against an independent source of truth.
2. examine.rs::record_distro's and host_gpu.rs::distro_name_from_os_release's unreadable/malformed-line error messages still hardcode "/etc/os-release" - now potentially wrong, since the text being reported on could have come from the fallback file.
3. Two small docs/testing.md inaccuracies: it attributes the probe-routing logic to os_release.rs (that module is just the shared parser; the actual host-root-aware reads live in examine.rs/host_gpu.rs), and it describes hardware_root::tests::read_os_release* as covering 3 cases (primary found, primary absent, neither found) when a 4th test (read_os_release_does_not_fall_back_when_the_primary_errors_for_another_reason) exists and isn't mentioned.

A few more things worth a look, not blocking: a naming collision between two different functions both effectively called read_os_release (the raw helper vs. the package-manager-plan wrapper) - no actual bug, just a readability nit; whether the four near-identical WSL/host-path resolver closures (|p| host_path(p) etc.) are worth factoring into one helper; and a couple of untouched advisory strings elsewhere that are now very slightly stale but out of this PR's stated scope.

None of this undermines the fix itself - closing the three error-text/test-quality items would make this good to merge.

[juhovainio]

This now compares read (the output of read_os_release(), the function under test) against rocm_core::hardware_root::read_os_release(|p| PathBuf::from(p)).unwrap_or_default() - which is exactly the same helper, called with the same identity resolver, that read_os_release() itself wraps one line away (apps/rocm/src/runtime_install_finalize.rs:160-161). The comment above explains why the old comparison (fs::read_to_string("/etc/os-release")) was wrong on a fallback-only host, which is correct - but the replacement isn't an independent check either, it's the implementation comparing itself to itself. If the wrapper's .context(...) call or its resolver closure ever introduced a bug, this assertion would still pass. Worth comparing against something that doesn't share the same code path - e.g. reading both candidate paths directly under the simulated root and asserting the result matches whichever one actually exists.

[jussielo-amd]

Confirmed, and fixed further: the follow-up commit 2dcb8314 ("fix: make the simulated-host-root test compare against an independent read") stopped comparing against hardware_root::read_os_release a second time, but still accepted Absent/Unreadable interchangeably. 5c78fdbf ("fix: assert the exact OsRelease variant in the simulated-root test") tightens that further: the expected value is now built as the same three-way OsRelease the reader returns (via an ExpectedOsRelease enum mirroring hardware_root::read_os_release's own branches), so the test fails if the real read ever blurs Absent and Unreadable.

[juhovainio]

Not on this line directly, but record_distro's unreadable-text branch a little further down (examine.rs:763, unchanged by this diff) still reports "/etc/os-release {unreadable}; the distro is not reported." hardcoded. Now that this call can hand record_distro text that actually came from /usr/lib/os-release, that message can name the wrong file to an operator debugging a malformed-line case on a fallback-only host. Same issue applies to host_gpu.rs:1992's format!("Linux (/etc/os-release {unreadable})"). Worth having hardware_root::read_os_release (or its callers) track which candidate was actually read, so these messages can name it.

[jussielo-amd]

Fixed in b6b0abdf ("fix: non-blocking doc/comment nits and the last two /etc-only readers") and earlier commits in this chain: record_distro's message at examine.rs:761 now interpolates {path} from the OsRelease::Unreadable variant rather than a hardcoded string, so it names whichever candidate (/etc/os-release or /usr/lib/os-release) was actually read.

[juhovainio]

Same stale-error-text issue as the one left on examine.rs:733: distro_name_from_os_release's unreadable branch at host_gpu.rs:1992 (unchanged by this diff) still says "Linux (/etc/os-release {unreadable})" unconditionally, even though the text it's formatting an error about may have come from /usr/lib/os-release via this new call.

[jussielo-amd]

Fixed: distro_name_from_os_release at host_gpu.rs:1994 now interpolates {path} from the OsRelease::Unreadable variant instead of a hardcoded /etc/os-release, so Linux (<path> could not be read: ...) names the candidate actually read.

[juhovainio]

This says "The two reads this covers are os_release.rs's own" - but os_release.rs (per its own module doc) is "the one parser every reader of the file uses," not a reader itself. The two reads that route through a simulated test root live in examine.rs (probe_os/record_distro) and host_gpu.rs (detect_distro_name), both calling hardware_root::read_os_release with host_path as the resolver. Worth correcting the attribution so a reader looking for the probe logic doesn't go hunting in the wrong file.

[jussielo-amd]

Fixed in b6b0abdf: the paragraph now attributes routing to hardware_root::read_os_release (hardware_root.rs), not os_release.rs, and describes the two actual reads as the host_path-routed probe (examine/host_gpu) versus the real-machine package-manager plan (openmpi/driver_install/runtime_install_finalize) — see docs/testing.md's /etc/os-release section.

[juhovainio]

This lists three cases for hardware_root::tests::read_os_release* ("primary found, primary absent, neither found"), but there are four tests in hardware_root.rs: those three plus read_os_release_does_not_fall_back_when_the_primary_errors_for_another_reason, which is arguably the most important one to document since it's what keeps a permission-denied primary from silently reading a stale fallback. Worth adding it to the list.

[jussielo-amd]

Fixed in b6b0abdf: the sentence now lists all four hardware_root::tests::read_os_release* cases (primary found, primary absent, neither found, and each candidate existing but erroring for a reason other than absence), naming read_os_release_does_not_fall_back_when_the_primary_errors_for_another_reason explicitly.

[rominf]

🔴 Automated review · pr-review-watcher · 5f2c3ff

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.

First automated round on this PR: every finding below is new.

Review — needs work

Full review of the whole change.
Implements #545 (open): the change routes every in-process reader through one helper and gives the WSL remote probe its own shell copy of the fallback. The description says "Fixes #545", and that holds.

Blocking

• The prek (lint / hygiene) CI check fails on cargo fmt, so the test and Windows jobs never ran for this head — apps/rocm/src/driver_install.rs:38, :817, crates/rocm-core/src/examine.rs:3180, crates/rocm-core/src/openmpi.rs:1030, :1337, :1350

  • cargo fmt --all -- --check reproduces it locally, and every hunk is in lines this PR adds.
  • Because prek failed, CI skipped "build-and-test", "Test (affected crates)" and "windows-build-and-test". This head has no CI unit-test run and no Windows build.
  • I could not cover Windows locally either: the cross-check for x86_64-pc-windows-gnu fails because ring needs x86_64-w64-mingw32-gcc, which is not installed here.

  Confidence 100 · mechanical · Fix: run cargo fmt --all, push, and let the skipped jobs run.

• The two behaviours the newest commit and the new seams add are not tested: naming the file actually read, and the WSL/Windows guard — crates/rocm-core/src/openmpi.rs:894, :1001, crates/rocm-core/src/examine.rs:772, crates/rocm-core/src/host_gpu.rs:2002, apps/rocm/src/driver_install.rs:1013-1014, :1036

  • Commit 5f2c3ff ("name the file a fallback read actually read") exists to change these messages. I replaced read.path() with "/etc/os-release" at all five sites. The full rocm-core lib suite with --features e2e-test-hooks still passed (676 tests), and so did cargo test -p rocm --bin rocm -- driver_install os_release (59 tests).
  • No test has a Found { path: "/usr/lib/os-release" } value whose text fails to parse. Both a_fallback_read_plans* tests use valid ID=ubuntu text, so the path never shows up in the output.
  • In the same run I also replaced the reaches_os_release_parse guard with true, and it stayed green. On WSL with an unreadable file, that mutation turns the ROCDXG plan into unreadable_os_release.
  • The test comments at openmpi.rs:1409-1413 and driver_install.rs:1943-1946 say a revert "is a type error here". It is not: OsRelease::Found { path: "/etc/os-release", text: read_to_string(..).unwrap_or_default() } compiles.

  Confidence 100 · logic · Fix: add tests with Found { path: "/usr/lib/os-release", text: "unset ID\n" } that assert the path in the plan reason, the hint, the driver reason, record_distro's probe failure and the distro name. Add a WSL test_examine + Unreadable case that asserts the WSL policy. Correct the "type error" comments.

• In examine-24, the step "the inspection reports no trouble reading the release file" can never fail — tests/e2e-cucumber/tests/e2e/examine_steps.rs:1401-1410, :1431-1435

  • os_release_failures keeps only entries that contain "/etc/os-release".
  • examine-24 plants only /usr/lib/os-release, and since this PR every failure message names that file. A read or parse failure there is filtered out before the assertion sees it.
  • The scenario still catches a revert, through its distribution assertions. This step is the only vacuous one.

  Confidence 85 · logic · Fix: match on "os-release", or on both paths.

• The new handling of a file that exists but cannot be read changes what users see and has no scenario (AGENTS.md §3) — crates/rocm-core/src/examine.rs:759-764, crates/rocm-core/src/host_gpu.rs:1971-1978, :1990-1992

  • Before this PR, an unreadable but present /etc/os-release went through read_host_text and came back empty. That produced no probe failure, no degraded verdict, and summary.distro was None.
  • Now rocm examine --json gets a new probe_failures entry, the status becomes degraded, and the printed distro reads Linux (<path> could not be read: …). The driver and OpenMPI/libatomic/libnuma plans also get a new reason.
  • Only unit tests with a hand-built Unreadable value cover this.
  • A scenario is easy to plant: put a directory at host-root/etc/os-release (a non-NotFound error that needs no root) and reuse examine-23's GPU planting.

  Confidence 85 · logic · Fix: add a Gherkin scenario that asserts the degraded verdict, the failure text and the distro line together.

• The test for "the primary exists but errors" in the WSL probe never hits an error, and a real unreadable primary aborts the whole probe — crates/rocm-core/src/examine.rs:860-866, the test try_os_release_does_not_source_the_fallback_when_the_primary_exists_but_errors (~:5844), docs/testing.md (the "same non-absence case" sentence)

  • The probe runs as wsl.exe --exec /bin/sh -c …, and /bin/sh is dash on Debian and Ubuntu.
  • Under dash, the test's directory fixture is sourced silently with rc 0 and prints id=. The test therefore only tells -e apart from -f, not "errors".
  • A genuinely unreadable primary (mode 000, as uid 1000) makes dash abort try_os_release with rc 2 and print nothing. The caller at :953 then reports "could not inspect ''; the distribution may be stopped or unreachable" and throws away every other probe line.
  • The abort on an unreadable primary existed before this PR. What is new is the test and the docs, which claim this matches the Rust readers. The Rust readers report Unreadable; this probe fails entirely.

  Confidence 100 (behaviour reproduced with dash -c) · logic · Fix: test with an unreadable file, not a directory (and note that CI running as root bypasses mode bits). Either make the shell guard survive it, for example [ -r "$1" ] inside the -e branch plus an os_release_unreadable= marker, or narrow the doc claims.

• The description understates what the change does — PR body

  • It says the change wires in the fallback while "preserving each one's existing resolution strategy and error handling exactly". The diff changes the error handling: it adds the Found/Absent/Unreadable states, a new degraded verdict and distro text in examine, and new plan and hint reasons.
  • It does not mention the rewrite of WSL_REMOTE_PROBE, the path-naming change, or the new unreadable-file tests.
  • It says "keeping only the two commits"; the branch has three.

  Confidence 85 · mechanical · Fix: update the description to cover the work. The fix here is the description, not narrowing the code.

Non-blocking

• docs/testing.md gets the delegation wrong (the new paragraph near "The three _read-suffixed functions"): it says the _read functions delegate to the bare-text functions. They delegate to the _at functions; the bare-text functions are siblings that also call _at. The code is right and the doc should change. Confidence 75 · mechanical.
• The doc comments on plan_for_os_release_read overstate what the message avoids — crates/rocm-core/src/openmpi.rs:874-879, :886-891.
  • The comment says the change avoids the "Could not map distribution (ID=)" message.
  • The reason still appends build("", "").reason, which is that exact message, after the real cause.
  • The test only checks starts_with, so per AGENTS.md §3 the message is not asserted together with the behaviour.
  • Either drop the appended reason or reword the comment. Confidence 70 · mechanical.
• Comments that are now stale — Confidence 70 · mechanical.
  • openmpi.rs:901: the InstallHint doc names plan_for_os_release, but production now calls plan_for_os_release_read.
  • openmpi.rs:1024, :1047, :1073: "On Linux it reads /etc/os-release" does not mention the fallback.
  • host_gpu.rs:4838: a test doc says "unchanged by this PR". That is wrong for Unreadable (None became Some("Linux (…)")) and will be meaningless once merged.
• Two verify: strings still read only /etc/os-release — crates/rocm-core/src/fix.rs:749, crates/rocm-core/src/diagnose.rs:2309 (grep VERSION_ID /etc/os-release). These are the only os-release readers in the tree without the fallback. They predate this PR and only print on WSL, where Ubuntu ships the file, so the risk is low. Confidence 70 · mechanical.

Decisions for the author

• Three entry points per builder (bare / _at / _read) — non-blocking-improvement
  • build_driver_install_plan and install_hint_for became #[cfg(test)], but openmpi::plan_for_os_release stays a non-test pub fn with no production caller.
  • Tests could build OsRelease::found(...) and call the _read seam directly. That would remove the bare variants and the always-/etc path they keep alive.
• A duplicated guard and plan literal in build_driver_install_plan_read (driver_install.rs:1013-1036) — tradeoff
  • Today the guard and the plan literal match build_driver_install_plan_at's early returns and its unreadable-line plan field for field, apart from the reason.
  • Keeping them separate keeps the file-level case out of _at's signature. The cost is that a new early branch added to _at would silently be pre-empted.

Positive signals

• hardware_root::read_os_release falls back only on NotFound. A file that exists but is unreadable is reported rather than quietly replaced by a possibly stale /usr/lib/os-release, and each branch has its own resolver-injected unit test.
• try_os_release_fn_is_embedded_in_the_wsl_remote_probe stops the tested shell constant from drifting away from the probe that embeds it.

Deployment notes

• When /etc/os-release (or the fallback) exists but cannot be read, rocm examine --json now reports status: degraded and a new probe_failures entry, where it used to report a silently empty distro. Anyone consuming the JSON verdict will see the change.

What this covered

I read every file in the change (11 files), plus the os-release readers across the whole tree, the WSL_REMOTE_PROBE caller, and the cucumber step helpers. Range: 21be003…5f2c3ffc.

• The code passes ran as two independent read-only workers, one on the examine/hardware_root/host_gpu/e2e group and one on the openmpi/driver_install/runtime_install_finalize/docs group. I did the consolidation, the verification runs and the mutations.
• Evidence runs:
  • The fmt check.
  • cargo test -p rocm-core --lib --features e2e-test-hooks filtered: 46 os-release tests passed, including every new one.
  • Mutations: the full rocm-core lib (676 passed) and -p rocm --bin rocm -- driver_install os_release (59 passed).
  • dash probes of try_os_release.

Did not run:

• The Step 7 design questions ran in the driver context, not as independent workers (subagent budget).
• The prior-changes pass: earlier review comments are out of bounds for this run.
• The Windows-target check: mingw is not installed.
• The full workspace tests and cucumber e2e: I relied on CI's passing E2E job for examine-24.

Not reconciled against any PR discussion here.

[jussielo-amd]

Status update at fe42a743 — CI is fully green (including windows-build-and-test's native Windows clippy pass and all self-hosted E2E lanes).

tomastola's review (filed against a stale head, explicitly flagged as unanchored)

Re-checked each item against the current code rather than trusting the flag:

• Parse errors naming /etc/os-release even when the fallback was read: fixed — every message now names the path actually read (5f2c3ffc).
• Unreadable /etc/os-release reported as an unsupported distro: fixed — hardware_root::OsRelease::Unreadable reports it as itself, not an unknown distro (838a521c).
• Nothing pinning the fallback at install-plan call sites: fixed — a dedicated test per call site asserting the fallback path appears in the message/plan (2b533f9f), plus the simulated-root test now reads independently rather than via the function under test (2dcb8314).
• Dangling doc reference to rocm_core::detect_distro_name: gone — that function is no longer referenced from runtime_install_finalize.rs.
• The WSL-probe test claiming parity it couldn't back up: addressed — a new test (try_os_release_does_not_source_the_fallback_when_the_primary_is_not_sourceable) adds the existence-vs-readability case with a directory fixture, and its doc comment is explicit about the one gap it still can't cover under CI-as-root (a genuinely permission-denied primary aborts the whole embedded probe; tracked as out of scope here, not silently ignored).
• The red chat-06 E2E check: resolved — CI is green on the current head.

rominf's automated review (pr-review-watcher, at 5f2c3ffc)

• cargo fmt failure blocking build-and-test/windows-build-and-test: fixed.
• Path-naming and WSL/Windows guard behaviors untested: fixed — one test per message-building call site (2b533f9f), plus both the WSL (2b533f9f) and Windows (fe42a743) halves of reaches_os_release_parse now have dedicated tests.
• examine-24's "no trouble reading the release file" step could never fail: fixed — the e2e failure filter now matches either candidate path, and examine-25 was added to cover the previously-uncovered degraded-verdict case (83c528c1).
• WSL probe test overclaiming parity with the Rust-side Unreadable case: fixed — renamed and narrowed to state exactly what it proves, per the doc comment above (192d61f4).
• PR description understating the change: fixed — current body covers the WSL remote-probe site, the degraded-verdict behavior, and all three test levels.
• All non-blocking items (stale doc comments, the docs/testing.md delegation description, the two /etc-only verify: strings) addressed in 856a2933/31881cec/fce44bf4, plus two further self-found issues from a follow-up /code-review pass: a shared hardware_root::unreadable_prefix() helper replacing five hand-duplicated message fragments, and the WSL verify-grep fallback made existence-only (it was previously falling back on any grep miss, not just absence).

juhovainio's review

All three items replied to inline with the specific fix commits (test comparing against an independent read, the two stale hardcoded-path error messages, and the two docs/testing.md inaccuracies).

Nothing outstanding that I'm aware of — re-requesting review from all three.

[juhovainio]

Re-reviewing after the head moved past my earlier CHANGES_REQUESTED (commit 838a521) - this covers the current state fresh, via gh pr diff (no checkout).

The commits pushed since my original review read as a genuine continuation of that feedback: a message-naming fix (naming the file a fallback read actually read, not always /etc/os-release), dead-code removal, stronger test assertions on the simulated-host-root test, a probe_failures filter fix so it matches either os-release path, and switching the WSL-adjacent verify: check from readability-based to existence-based - the same bug class the original fallback fix addressed. I independently re-verified all of these are currently correct in the code with no regression; no blocking findings remain.

The new OsRelease (Found/Absent/Unreadable) enum is a clean replacement for the old lossy Option<String>, and the "fall back only on NotFound, never on another error" discipline is correct on both the Rust and WSL-shell sides, with regression tests guarding against reintroducing a hardcoded path.

Two non-blocking notes for a future pass, not blocking this:

• There's a pre-existing (not introduced here) WSL-guest edge case where a permission-denied /etc/os-release aborts the shell probe entirely rather than degrading gracefully. Already disclosed as a known, scoped-out gap in a test doc-comment.
• The new verify: fallback one-liners in diagnose.rs/fix.rs aren't exercised under a real shell, unlike the structurally identical WSL probe fragment which does get one.

On spec: the fallback matches issue #545 exactly and all original call sites are covered. The PR does go a bit beyond the issue's literal ask (the Unreadable tri-state, not just absent-fallback), but that's disclosed in the PR's own Scope section and is well-tested, not scope creep slipped in quietly.

CI is effectively green; only windows-build-and-test was still pending at last check.

fixed

[rominf]

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

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.

Round 2. This PR was merged at exactly this head (4b3d460) while the round was running, and the earlier automated change request (at 5f2c3ff) had already been dismissed. Nothing here gates anything any more. The findings describe code that is now on main; treat the blocking ones as follow-up candidates. Each finding is marked new or standing (re-found blind in a narrower form).

Blockers from the 5f2c3ff round that this round did not re-find (the cargo fmt failure, the untested path-naming messages and WSL/Windows guard, the examine-24 step that could not fail, the missing scenario for an unreadable release file) were not put through a separate objection check, because the change request holding them was already dismissed and the PR merged. CI on this head is all green, and examine-28 and per-call-site tests were added for them. That is the extent of what this round says about them.

Review — needs work

Full review of the whole change.
Implements all of #545: one shared reader for /etc/os-release that falls back to /usr/lib/os-release, used at every read site. #545 is already CLOSED on the tracker.

Blocking

• [new] The examine-28 step checks less than its wording says, and the new distro-name line has no scenario — tests/e2e-cucumber/tests/e2e/examine_steps.rs:1551-1577, crates/rocm-core/src/host_gpu.rs:1976-1990

  • The step says "its only failure says the release file could not be read". It filters probe_failures down to entries containing "os-release" and then checks len() == 1. Any other probe failure could be what degrades the verdict, and the step would still pass.
  • The sibling examine-23 step asserts the whole probe_failures list, on the same GPU fixtures.
  • The step also never reads /summary/distro. That field now shows a new line users can see: Linux (/etc/os-release could not be read: …).
  • In mutation testing by a worker, removing the Unreadable arm in distro_name_from_os_release broke no scenario; only a unit test catches it.
  • This conflicts with AGENTS.md §3: user-observable output needs a scenario, and a message is asserted together with the behaviour.
  • Confidence 85 · logic · Fix: assert the full probe_failures list, as examine-23 does (prefix/suffix match for the OS error text). Also assert /summary/distro starts with Linux (/etc/os-release could not be read: .

• [standing, narrower form] The WSL probe's shell test cannot fail on the claim it is named for — crates/rocm-core/src/examine.rs (the try_os_release_does_not_source_the_fallback_when_the_primary_is_not_sourceable test and the TRY_OS_RELEASE_FN doc), docs/testing.md:596-604

  • The doc and docs/testing.md say the test proves "existence is not readability" ([ -e ] rather than a readability check).
  • A worker changed [ -e "$1" ] to [ -r "$1" ] in both TRY_OS_RELEASE_FN and WSL_REMOTE_PROBE, and all 96 tests stayed green. A directory is readable, so -r treats the fixture exactly as -e does.
  • The fixture only tells -e apart from -f. The -r regression the doc is guarding against, which would source a stale fallback in place of a permission-denied primary, goes undetected.
  • Confidence 85 · logic · Fix: either reword the doc to the claim the fixture actually supports, or add a mode-000 case that is skipped when running as uid 0.

• [new] The PR description repeats two claims the code does not support — PR body, "Tests" section

  • "A shell-level test … confirming existence is not readability" — the finding above shows it does not.
  • "WSL has its own guest-side probe, so the local Rust-side read is never authoritative for it." The same reasoning is in the a_wsl_host_ignores_an_unreadable_local_os_release_read doc (apps/rocm/src/driver_install.rs:2046-2054).
    • WSL_REMOTE_PROBE is run from a Windows host into a distro over wsl.exe (examine.rs:848).
    • On a WSL guest, rocm install driver runs inside the guest, so the local read is the guest's own file.
    • The guard holds for another reason: wsl_rocdxg_driver_plan never looks at the distro.
  • Confidence 70 · logic · Fix: correct the test doc (the description no longer matters now the PR is merged). The code is fine as written.

Non-blocking

• [new] The new wsl-5 verify command always exits 0 and prints nothing when there is no file or it cannot be read — crates/rocm-core/src/diagnose.rs:2424, crates/rocm-core/src/fix.rs:749

  • I ran it under sh. With neither file present: empty output, rc 0. With /etc/os-release as a directory: empty, rc 0. With no VERSION_ID line: empty, rc 0.
  • The old grep VERSION_ID /etc/os-release showed an error for the first two cases.
  • 2>/dev/null is left over from the two-file grep in 6731fe8. It now hides the permission-denied case that 31881ce's message is about.
  • | head -1 makes the pipeline's exit status head's, and picks the first match, whereas the parser lets the last assignment win. An unanchored VERSION_ID also matched a VERSION_ID_LIKE=1 line I planted ahead of the real one.
  • No test pins either copy of the string. The diagnose/fix divergence test does not cover this recipe.
  • Confidence 85 · logic · Fix: grep '^VERSION_ID=' "$f" with no redirect and no head, plus a divergence check covering this recipe.

• [new] The duplicated version default contradicts its own doc comment — apps/rocm/src/driver_install.rs:825-837, :863

  • unreadable_os_release_plan's doc says sharing repo_version's resolution means "a bumped default … is not silently absent from the other".
  • In fact "${ROCM_CLI_AMDGPU_VERSION:-7.2.4}" is now hard-coded both there and in build_driver_install_plan_at. Bumping one leaves the other stale.
  • No test asserts the unreadable plan's repo_version: the worker's mutation setting it to String::new() survived.
  • Confidence 85 · mechanical · Fix: one const, used by both.

• [new] host_path_for_str / identity_path_for_str exist for a reason that does not hold — crates/rocm-core/src/hardware_root.rs:81-105

  • The docs say callers pass these "instead of |p| host_path(p)" / "|p| PathBuf::from(p)", because bare host_path / PathBuf::from are rejected.
  • The bare forms are indeed rejected. But I compiled both closures against an impl Fn(&str) -> PathBuf parameter and they are accepted; the crate's own tests pass |name| candidate(dir.path(), name) the same way.
  • Two new pub functions therefore add API surface that a closure would do without.
  • Confidence 85 · mechanical · Fix: use closures and drop the helpers, or fix the docs to say they are a naming convenience.

• [new] Stale comment — apps/rocm/src/runtime_install_finalize.rs:2133-2135 refers to the helper's ".context(...) call". That call was removed along with the old Result-returning read_os_release. Confidence 85 · mechanical · Fix: drop the clause.

• [new] Comments narrate history instead of describing current state — hardware_root.rs (OsRelease doc: "Before this distinction existed…"), examine.feature (examine-28 comment: "Before this change… It now reports"), host_gpu.rs (test doc "new behavior; it used to be…", and the "pre-fallback behaviour this function has always had" arm comment), docs/testing.md ("now gets", "now read through"). These go stale on merge. Confidence 85 · mechanical · Fix: state current behaviour only.

• [new] examine-23 and examine-28 have the same title — tests/e2e-cucumber/features/examine.feature:324, :427. Both read "An unreadable release file degrades the verdict on a machine with a GPU". feature_naming (passes, 4 tests) does not check title uniqueness, so reports cannot tell the two apart. Confidence 70 · mechanical · Fix: retitle examine-28 to say the file itself cannot be read.

• [new] detect_distro_name's Absent => None arm is not pinned — crates/rocm-core/src/host_gpu.rs:1976. In a worker's mutation run, changing it to Some("Linux") broke no test. The arm keeps existing behaviour rather than adding new, so this is not blocking. Confidence 70 · logic · Fix: add a fallback-root case with neither file present to the feature-gated test.

• [new] The fallback in the real-host install readers is reachable only through the delegation — crates/rocm-core/src/openmpi.rs:1014, apps/rocm/src/runtime_install_finalize.rs:160

  • In a worker's mutation runs, replacing either real_os_release() body with an /etc-only read that still returns an OsRelease passed every test.
  • The a_fallback_read_plans_* tests compare two inputs whose path is never used for a well-formed file, so they killed none of the path mutants.
  • The PR body states this gap honestly. The test docs saying "reverting … is a type error" hold only for a revert to String.
  • Confidence 70 · logic · Fix: narrow those docs, or call read_os_release from a test with an injected resolver that mimics identity_path_for_str.

Decisions for the author

• reaches_os_release_parse duplicates build_driver_install_plan_at's Windows/WSL early returns (apps/rocm/src/driver_install.rs:1026) — tradeoff.
  • For: it keeps the Unreadable check outside _at, which stays text-based for its many existing tests.
  • Against: it is a twin predicate that will drift if a new early return is added. Each half is pinned by its own test today.

Positive signals

• read_os_release falls back only on NotFound. Each candidate's non-NotFound error has its own test, so a stale /usr/lib/os-release can never replace a present-but-unreadable primary.
• try_os_release_fn_is_embedded_in_the_wsl_remote_probe stops the tested shell constant from silently drifting away from the copy that actually runs.

Deployment notes

• rocm examine --json now reports status: degraded, with a new probe_failures entry, on hosts where /etc/os-release exists but cannot be read. The install plans get a new unreadable_os_release outcome for that case. Consumers keyed on status will see that change.

What this covered

Read every file in the change completely, plus the callers of the changed and removed functions across the workspace. Diff 0716645 (merge-base)…4b3d460; prw-base is at 80ca67f. The branch merges main, so history was read with --first-parent.

Two independent read-only workers, grouped by coupling:

• Install-plan consumers (openmpi.rs, driver_install.rs, runtime_install_finalize.rs, with hardware_root.rs). Focused unit tests: 42 rocm-core and 63 rocm passed. Clippy clean. 15 single-branch mutations.
• The reader, examine, host_gpu, diagnose/fix, e2e and docs, plus the agent-instruction, history and code-comment passes and a sibling-reader grep. 47 rocm-core tests passed with e2e-test-hooks. feature_naming 4/4. Mutation runs.

I compiled a closure-inference check and ran the verify shell string myself.

Every check on the head commit passed, including every required check, E2E tests and windows-build-and-test.

Did not run:

• The e2e suite; examine-27/28 verdicts are reasoned.
• A Windows-target cargo check: there is no mingw here.
• The prior-changes pass: earlier review comments were deliberately not read.

Not reconciled against any PR discussion.