[rominf]

Fixes #543.

/etc/os-release was parsed in four places, each with its own trim_matches. Compared with sh sourcing the same file, they disagreed with each other:

file	sh	driver plan	rocm examine	OpenMPI hint
ID='ubuntu'	ubuntu	'ubuntu'	'ubuntu'	ubuntu
ID=debian then ID=ubuntu	ubuntu	debian	ubuntu	debian

So a spec-valid single-quoted file read as an unsupported distro. On a file with a duplicated ID, rocm examine reported Ubuntu while the driver plan was built for Debian 12.

The fix

Every reader now goes through rocm_core::os_release. That covers the driver plan; the OpenMPI, libatomic and libnuma install plans and the hints that name them; rocm examine's distro_id/distro_version; and the distro name it prints. The module keeps one rule: the value sh would assign, or nothing.

• For a file made only of blank lines, comments and spec-valid assignments, it returns sh's value. That means:
  • one matching quote pair removed;
  • \" \\ \` \$ decoded inside double quotes;
  • nothing decoded inside single quotes;
  • \x meaning x in a bare value;
  • leading spaces and tabs, and indented comments, allowed;
  • a # comment after a value;
  • the last assignment winning.
• Any other line makes the whole file unreadable. To sh such a line is a command or the start of one: export ID=…, X=1; ID=…, unset ID, or an unterminated quote that swallows the lines after it. A command can change any variable, so reading past it would report values sh does not assign. A CRLF file is unreadable too: sh assigns ubuntu\r.

What an unreadable file costs, and where it is reported

This is fail-closed. On a CRLF file, or one with a single stray line (ANSI_COLOR=0;31, ID =x), these now plan nothing where the old readers worked:

• the driver plan;
• the OpenMPI, libatomic and libnuma auto-installs (rocm engines install vllm, and the engine step of rocm install sdk).

Against public files this is rare (see Real files), but hand-edited and custom images are exposed. There is no override; the fix is to edit the line.

The line is reported, as line N is not a plain assignment: "<line>" (Debug-quoted so control characters cannot reach a terminal), in every place that uses the distro:

• rocm install driver: policy: unreadable_os_release, supported: false, no commands, and the line in reason. Anything matching on policy should expect the new value.
• OpenMPI/libatomic/libnuma install plans: the line ahead of the manual-install advice in the plan's detail:.
• vLLM's missing-runtime warnings (install and runtime detection) and its launch errors: the line after the manual-install advice. They no longer offer rocm engines install vllm --yes, because that install would plan nothing. The offer now comes from the same plan the install runs, so it is also dropped for an unknown distribution, where it was already misleading.
• rocm examine: distro: Linux (/etc/os-release line N …). With --json, also an empty distro_id/distro_version, the same line in probe_failures, and status: degraded on an otherwise healthy GPU host.

Wherever the user has to act (driver reason, plan reasons, hints), the message also says what to change, judged from the whole line: save the file with LF line endings; put that line's value in single quotes; or remove that line or rewrite it as NAME=value.

Real files

Checked against dash: all 50 container images, and 396 of the 397 files in which-distro/os-release at 10dfd5bc, read exactly as dash reads them. The one exception is a CRLF file, which reads as unreadable. No file read a value dash does not assign.

Tests

• The parser against /bin/sh itself. The shell runs with an empty environment, a sentinel HOME and a PATH that does not exist. Inputs: every spec-valid encoding of every value up to three characters, and every right-hand side up to three characters over a hostile alphabet. Each must read as sh reads it or not at all, and a list of well-formed cases must read as a value. Linux-only.
• Parser line rules: single tests for indented comments and indented unreadable lines (the reported text keeps its indentation), ID/ID_LIKE order with a multi-word ID_LIKE, and the fix advice for each kind of line.
• Plans and hints: a multi-word ID_LIKE derivative plans, and its hint equals the plan's commands exactly. Each of the three hints uses its own builder and manual text. The --yes offer is checked together with whether the plan installs.
• Driver plan (driver_install.rs):
  • every supported distro rewritten in single quotes plans exactly as in double quotes (a_single_quoted_os_release_builds_the_same_plan);
  • a duplicated ID plans from its last assignment (a_duplicated_os_release_key_is_planned_from_its_last_assignment);
  • an unreadable file is refused with the line and the fix, and no commands (an_unreadable_os_release_names_its_line_and_plans_nothing).
• Host-root test: os_release_reads_follow_the_simulated_host_root (with e2e-test-hooks) checks that both rocm examine reads answer from a planted root through the shared parser. The planted file is one the old readers misread.
• Every new test was mutation-checked: each fails under the mutation it targets.

Scenarios

In tests/e2e-cucumber/features/examine.feature, all @requires-os:linux and all planting /etc/os-release under the simulated host root:

• @id:examine-reads-single-quoted-os-release (examine-20): a single-quoted file is read as sh reads it.
• @id:examine-reads-last-os-release-assignment (examine-21): the last of a repeated key wins.
• @id:examine-reports-unreadable-os-release (examine-22): an unreadable file names line 3 in probe_failures and in the distro name.
• @id:examine-degrades-on-unreadable-os-release (examine-23): status: degraded, with the release-file line as the only failure. The planted root has a GPU in its kernel topology, and a stand-in lspci on PATH lists an empty bus, so this runs on every Linux lane with or without a GPU. Without the planted GPU the verdict would be no-amd-gpu whatever the file says.

There is no scenario for the driver plan, the package plans, the hints, or vLLM's messages. They deliberately read the real /etc/os-release (read_os_release in runtime_install_finalize.rs, and real_os_release in openmpi.rs), because their commands run against the real machine and a simulated host must not choose their distro. A scenario could only exercise them by rewriting the runner's own /etc/os-release. The unit tests above cover them, each asserting the message together with the plan it describes.

Docs

• docs/testing.md has a new /etc/os-release section: what each reader reports for an unreadable file, the --yes rule, and the test commands.
• docs/release-trust.md's Simulated Host Root section is updated to match.
• The CI comment on "Test rocm-core with e2e-test-hooks" now lists /etc/os-release among the sampled reads.

Verification

On the current head, rebased onto ee3131be:

• cargo fmt --check
• rocm-core --lib (651 passed), plus e2e-test-hooks (6 passed)
• rocm --bin rocm (1150 passed), vllm (98), xtask (252)
• e2e-cucumber --lib (142), feature_naming (4) and check --test e2e
• cargo xtask e2e -- -n examine-2 (4 scenarios, 15 steps passed)
• both CI clippy invocations and a Windows-target clippy, all with -D warnings

Not run locally: examine-23 on a GPU lane (no GPU here), and the Windows lane.

[jussielo-amd]

This is excellent work — one of the more careful parser fixes I've seen come through. Routing every reader through one function validated against a live /bin/sh oracle (not just hand-picked fixtures) is exactly the right way to kill "four parsers, four opinions" bugs like #543, and the fail-closed design (one non-assignment line makes the whole file unreadable, loudly, rather than guessing) is well-justified: a shell command can retroactively mutate any variable already read, so there's no safe way to salvage a partial parse.

Ran the full suite locally: rocm-core (474 tests) and rocm (975 tests) pass, clippy and fmt are clean, and the old parse_os_release_field helpers are fully gone with zero dangling references anywhere in the repo. The deferred e2e scenarios are reasonably justified by the #510 dependency and backed by solid integration-level tests in the meantime.

Two small things worth a look, neither blocking:

• The new reason string for an unreadable file embeds the raw offending line verbatim, which is a different register from the other static reason strings nearby (e.g. the Windows/DKMS ones). Worth a glance at whether that's the tone you want here, though the Debug-quoting already handles the control-character safety angle.
• docs/testing.md / docs/manual-testing.md don't mention the new probe_failures entry or unreadable_os_release policy. Not required since no flag/default changed, but a one-liner would save the next person some head-scratching.

Approving.

[rominf]

Rebased onto 7f72556d (now 014c92c6).

Re-applied by hand. Since the approval, main moved code this PR edits:

• ROCMAI-82: extract driver-install, engines, serve from main.rs #540 moved the driver plan from apps/rocm/src/main.rs to driver_install.rs.
• ROCMAI-85: extract host/GPU detection, ROCm install discovery, and managed-runtime block #528 moved the host distro name from rocm-core/src/lib.rs to host_gpu.rs.

In both cases the moved copies still called the old parsers, so resolving the conflicts toward main would have compiled with the fix gone. I re-applied the edits at the new locations:

• build_driver_install_plan: the shared parser, plus the unreadable_os_release refusal.
• Its three tests, now in driver_install.rs.
• parse_os_release_pretty_name.

I also deleted the old main.rs parser, which main had made pub(crate) for the moved code. No old os-release parser is left in the workspace, and the rest of the change is line-for-line what was approved.

One new commit, 014c92c6. Reverting parse_os_release_pretty_name to its old scan left every test green, because the existing fixture reads the same through both parsers. pretty_name_reads_as_sh_reads_it now covers it (a duplicated key with a trailing comment, and an unreadable file) and fails on the old code. The same commit adds the docs from the review.

Verified on the new head:

• fmt, both clippy invocations, and a Windows-target cargo check -D warnings
• rocm-core --lib (604), rocm --bin rocm (1027), xtask (225) and e2e-cucumber --lib (142)
• feature_naming, check --test e2e and check-crate-edges

Each re-applied hunk, mutated back to the old behaviour, turns a test red. I re-ran one of them myself: swallowing the unreadable-file error fails an_unreadable_os_release_names_its_line_and_plans_nothing.

On the two notes in the approval:

• The raw line in the reason: I'd like to keep it. The other reasons describe a fixed policy, but here the line is the thing the user has to fix, so naming it turns "the distro cannot be read" into a one-line edit. It's Debug-quoted, so control characters and CRLF come out escaped. There is precedent in the same file: the WSL refusal reasons already include a version string the user supplied.
• Docs: done in 014c92c6, as a short /etc/os-release section in docs/testing.md. It covers the single parser, the unreadable_os_release plan (which plans no commands), the probe_failures entry in rocm examine, and how to run the tests. I left docs/manual-testing.md out, since checking this by hand means editing /etc/os-release on the test host.

Heads-up for merge order: #510 will conflict textually with this PR in examine.rs and next to detect_distro_name in host_gpu.rs. Whichever lands second resolves to record_distro(e, &read_host_text("/etc/os-release")). That keeps #510's re-rooting and this PR's parser, and leaves read_os_release on the real host.

Stale: approved ce6c0e7; head is now 014c92c, after rebasing onto main. #540 and #528 moved the driver plan and the host distro name, so those edits were re-applied at the new locations, and 014c92c adds the requested docs plus a test for the distro name. A fresh look is needed on the current head. Details are in the PR conversation.

[siloteemu]

🔴 Automated review · pr-review-watcher · 014c92c

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 (prw-base...HEAD, merge-base 7f72556d, head 014c92c6).
Description names issue #543 (not retrieved) and claims every reader of /etc/os-release now goes through the new parser, with unreadable files "reported, not hidden", each hunk mutation-checked, and the missing scenario deferred to #510.

Blocking

• [new] Several readers still hide an unreadable file, and now fail on files they used to accept — apps/rocm/src/main.rs:6849-6852, apps/rocm/src/main.rs:7024-7027, crates/rocm-core/src/openmpi.rs:831-833, :856-858, :881-883, crates/rocm-core/src/host_gpu.rs:1884-1893 and :1897
field() turns an unreadable file into None (os_release.rs:96-98), and these callers then use unwrap_or_default(). On a CRLF file or one with a stray non-assignment line, the OpenMPI/torch auto-install paths and the three install hints now print "Could not map distribution (ID=<unknown>)" (openmpi.rs:570-583) and give generic advice. The offending line is never named. The old parsers trim()ed the \r and skipped non-assignment lines, so these paths used to work on such files.
  The human-readable rocm examine shows distro: Linux with no explanation. detect_distro_name falls back to "Linux" (host_gpu.rs:1884-1893), and probe_failures is only reachable through --json.
  The description's "An unreadable file is reported, not hidden" and the os_release.rs:55-57 doc hold only for the driver plan and rocm examine --json. The misattribution to the distro is the same one this change fixes for the driver plan.
  Confidence 85 · logic · Fix: use parse() on these paths and put the UnreadableLine in the user-facing reason, or narrow the claims. Say in the description that CRLF and stray-line files lose auto-install.

• [new] No Gherkin scenario for changed user-observable behaviour, and the stated reason does not hold — tests/e2e-cucumber/features/driver_install.feature, tests/e2e-cucumber/features/examine.feature, AGENTS.md §3
  The change alters the rocm install driver --dry-run reason and policy, the rocm examine distro fields, probe_failures, and the status value (degraded, via compute_status at examine.rs:546). Only unit tests cover them. driver_install.feature has only WSL scenarios. examine-12 runs against the real host file and checks only that repo_version has no ${.
  AGENTS.md §3 exempts only purely internal changes and gated lanes. "No seam to plant a file" is neither, and commit 4da0340 ends in a broken sentence ("the host-root re-rooting proposed in / against that seam") with the reference missing.
  A scenario needs no planted file. On a @requires-os:linux lane, assert that rocm examine --json distro_id and distro_version equal what sh sources from the host's /etc/os-release.
  Confidence 85 · architectural · Fix: add that scenario. If the single-quoted and unreadable cases genuinely wait on #510, say so explicitly and fix the commit message.

• [new] Added tests stay green when the code they name is broken — crates/rocm-core/src/os_release.rs:138-139, :154, :194, :203; apps/rocm/src/driver_install.rs:845-860; apps/rocm/src/main.rs:6850, :7025; crates/rocm-core/src/openmpi.rs:858, :883
  I ran each mutation alone and found the suite green every time:

  • Parser, cargo test -p rocm-core --lib os_release (17 passed):
    • tilde_expands = false → true (194): a\:~ and \a~ would read None, which the oracle allows, and only \~ is in MUST_READ.
    • is_control() → is_ascii_control() (154): no test uses DEL or a C1 character.
    • Removing '<' (203) or ')' (203) from the refusal arm: neither is in the unit lists, and the oracle alphabet leaves them out.
    • is_ascii_alphabetic → is_ascii_alphanumeric for the first name character (138): no test names 1A=x, which sh runs as a command.
    • Accepting - in names (139): no VERSION-ID= test.
    • Accepting an empty name (135-140): no =x test.
  • Unreadable driver plan, driver_install (57 passed): os_id set to "ubuntu" (848), mutating: true (846), reboot_required: true (859), and checks emptied (858) all stay green. The test pins reason, supported, commands and policy only. An unreadable plan that still claims a distro is the exact contradiction the change exists to prevent.
  • Callers, no test reaches them: replacing "ID" with a bad key at main.rs:6850 and :7025 leaves all 1027 rocm tests green. Mangling the libatomic and libnuma ID_LIKE lookups at openmpi.rs:858 and :883 leaves all 604 rocm-core tests green. This contradicts the description's "each hunk … reverting it … turns a test red".
  • examine.rs:546 (degraded): the new examine test asserts probe_failures but never compute_status(). The degraded consequence is untested and undocumented.
    Confidence 100 · mechanical · Fix: add one case per surviving branch. Assert os_id, version_id, codename, mutating and reboot_required in the unreadable-plan test. Add an ID/ID_LIKE resolution test through the ensure_* and hint paths. Assert compute_status after record_distro.

• [new] Docs and comments describe behaviour the code does not have — crates/rocm-core/src/os_release.rs:18-19, :22-29, :43-45; docs/testing.md:1292-1306

  • os_release.rs:43-45 lists "a space or tab between = and the value" as a refusal. The code accepts K= #a as "", and MUST_READ contains " #a", which passed in my run.
  • os_release.rs:18-29 claims sh's value for every file made only of spec-allowed assignments. K=a\ (escaped trailing space) is valid and assigns "a ", but trailing blanks are trimmed before decoding, leaving a dangling \, so the whole file is refused. The test comment at :444-447 admits it. The refusal is safe; the doc is wrong.
  • docs/testing.md says every reader goes through the module and that rocm examine "leaves the distro empty and records the same line in probe_failures". The human report does not, as in the first finding. The WSL-from-Windows probe still sources the file through sh and splits lines itself (examine.rs:830, :909-923).
  • The section's cargo test … os_release filters do not match pretty_name_reads_as_sh_reads_it, although the section lists the host distro name as covered. The reader list omits ensure_openmpi_for_vllm and ensure_torch_runtime_dep.
    Confidence 85 · mechanical · Fix: the code governs in the first two cases, because it is what the oracle tests pin. Correct the module doc and the testing section to match.

Non-blocking

• [new] Oracle tests are gated on cfg(unix), which includes macOS — crates/rocm-core/src/os_release.rs:484
  AGENTS.md §6 says tests that apply only to supported hosts should be gated on target_os = "linux" or ignored with a reason. They also spawn /bin/sh, whose behaviour was only verified against dash and bash.
  Confidence 70 · mechanical · Fix: cfg(target_os = "linux").
• [new] ScratchDir reinvents a unique-name helper — os_release.rs:499-508
crate::test_support::unique_suffix() exists (test_support.rs:35) for this.
  Confidence 70 · mechanical.
• [new] UnreadableLine has Display but no std::error::Error — os_release.rs:64-89
  It is the Err of a public fallible parse, so callers cannot use ? into anyhow.
  Confidence 70 · mechanical.
• [new] Module doc enumerates callers — os_release.rs:7-10
  That list goes stale as callers change.
  Confidence 70 · mechanical.
• [new] Test doc comments narrate removed behaviour — apps/rocm/src/driver_install.rs:1767-1790
  "The parser stripped only "" belongs in the commit message.
  Confidence 70 · mechanical.
• [new] a_file_that_runs_a_command_never_reads_as_something_sh_does_not_assign pins only "parser returns None" — os_release.rs:716-731
  It is a regression guard for the fail-closed rule. It should not be counted as coverage of the command-line rules.
  Confidence 70 · mechanical.

Decisions for the author

• Whole-file fail-closed versus line-tolerant parsing — tradeoff
  For: sh is a precise reference, the driver plan runs privileged commands, and os-release(5) forbids the refused forms. Against: systemd's env-file reader treats \r as a newline and Python's platform.freedesktop_os_release skips lines it cannot parse. So a CRLF or hand-edited file that those tools read now blocks the driver plan and hides the distro on every path. A middle option is to strip a trailing \r only when every line has one, and keep fail-closed for anything sh would run. Please confirm fail-closed everywhere was deliberate.
• Several lines the parser refuses are accepted by sh (lone $, C1 controls, escaped trailing blank) — tradeoff
  These fail closed, within the stated rule. Confirm that over-refusal is the intended side.

Positive signals

• The /bin/sh oracle tests check the parser against the reference itself, so the encoder and parser cannot share a misunderstanding. MUST_READ plus the read > len/10 bar stops a parser that refuses everything from passing vacuously.
• Reading through one shared parser fixes the real driver-plan versus rocm examine disagreement on duplicated keys, and the single-quote misread.
• The unreadable-file message names the line, 1-based and Debug-quoted, so control characters are shown rather than acted on.

Deployment notes

• Behaviour change for hosts whose /etc/os-release has CRLF endings or any non-assignment line: rocm install driver plans nothing, rocm examine --json reports status: degraded with an empty distro, and the OpenMPI/torch auto-install paths and hints fall back to generic "unsupported" text.
• rocm examine JSON status can now be degraded where it was ok.
• The OpenMPI hints no longer accept ID =x.

What this covered

Every file in the diff, read completely: os_release.rs (all 733 lines), the changed regions and surrounding functions in driver_install.rs, main.rs, examine.rs, host_gpu.rs and openmpi.rs, and docs/testing.md. Also read: callers of the new API across apps/, crates/ and engines/vllm, and docs/testing.md against AGENTS.md. Range: merge-base 7f72556d … 014c92c6.

Run: four fan-out workers (parser, callers, source passes, design questions). The description was read after the findings were settled (Step 9). My own runs:

• cargo test -p rocm-core --lib os_release and the new examine and host_gpu tests passed (17 and 2 tests).
• The mutation runs listed in the third blocking finding.
• A worker's differential fuzz of a Python port of parse/decode against dash found 0 mismatches across 3,866 accepted cases. It was not re-run by me and is not a finding.

Per the CI summary supplied with this review, every check that reported on this head passed. "Skill checks" and "Sphinx docs build" did not report, so nothing here rests on them.

Not run: a full-workspace build or clippy, scripts/smoke_local.py, and the Windows lane. History, prior-changes and comment passes ran. Issue #543 was not retrieved. The Gherkin scenarios were not executed.

Not reconciled against any discussion here.

[jussielo-amd]

Re-reviewed at HEAD 014c92c, which supersedes the dismissed review I left on ce6c0e7 after the 10/06 round. Both follow-up points from that review are resolved: docs/testing.md now documents the /etc/os-release parser, the unreadable_os_release policy, and how to run the tests (docs/testing.md:1292-1304), and the duplicated-key/unreadable-file scenarios I asked about are covered by dedicated tests in driver_install.rs, examine.rs and host_gpu.rs. The reason-string register point I raised was left as-is, which is fine since I'd already called it non-blocking.

Checks performed on this diff (origin/main...HEAD, three-dot): read through crates/rocm-core/src/os_release.rs's decode/parse/ends_cleanly logic by hand against the module docs' stated sh-compatibility rules; ran cargo test -p rocm-core --lib os_release locally (17/17 pass, including the /bin/sh oracle and brute-force encoding tests); confirmed the old parse_os_release_field helper and all call sites are fully removed with no dangling references; ran a leak scan and a conflict-marker scan against the diff (both clean); and confirmed all CI checks on 014c92c are green, including Commit signatures + sign-off and the coverage-floor gate.

Two small follow-up opportunities, neither blocking: (1) the new unreadable-file diagnostic that names the offending line is wired into the driver plan (apps/rocm/src/driver_install.rs) and rocm examine (crates/rocm-core/src/examine.rs), but the three other callers this PR touched — ensure_openmpi_for_vllm and ensure_torch_runtime_dep (apps/rocm/src/main.rs:6850-6851, 7025-7026) and the OpenMPI/libatomic/libnuma hints (crates/rocm-core/src/openmpi.rs:832, 857, 882) — still collapse an unreadable file into the same empty-ID fallback as a merely-absent or unrecognized one, so a user hitting that path doesn't get pointed at the specific malformed line the way driver-install and examine users now do; (2) those same five call sites each parse the full file twice (once per field) via repeated os_release::field(...) calls instead of parsing once and reading both keys off the map, unlike build_driver_install_plan, which already does this correctly. Both are consistency/efficiency nits on an otherwise very solid and thoroughly-tested fix, not correctness issues.

Approving.

[rominf]

Rebased onto main 1e5aac8f, which includes #510, and addressed both reviews. The head is now f32fd941.

The automated review (5488764142):

• B1, readers hiding an unreadable file: fixed in 38958624.
  • The OpenMPI, libatomic and libnuma installs (ensure_openmpi_for_vllm, ensure_torch_runtime_dep) now go through openmpi::plan_for_os_release, whose reason names the line ahead of the manual advice.
  • The three hints append the line.
  • The human rocm examine prints distro: Linux (/etc/os-release line N is not a plain assignment: "…").
  • ID/ID_LIKE are looked up once, in os_release::id_and_id_like.
• B2, a scenario: examine-20, 21 and 22 plant the file under test(core): let e2e builds re-root the hardware probes #510's host root and run rocm examine --json: single quotes, a repeated key, and an unreadable line, the last asserting the message together with the state. All three fail against the old parsers. The driver plan deliberately reads the real /etc/os-release (test(core): let e2e builds re-root the hardware probes #510 does that because its commands run on the real machine, and a test pins it), so the driver plan stays unit-tested and the driver-plan scenarios the description promised aren't possible. The broken sentence in the first commit message is fixed.
• B3, surviving mutants: each one you listed now fails a test.
  • The unreadable driver plan also asserts os_id/version_id/codename, mutating, reboot_required, the preflight and checks.
  • Examine asserts degraded/ok via compute_status.
  • The main.rs key lookups moved into id_and_id_like, which is tested.
• B4, docs:
  • The module doc now says that anything after the value other than blanks and a comment is refused (K= #c assigns ""), and states the K=a\ exception.
  • docs/testing.md lists every reader, the diagnose --distro probe that runs the distro's own sh, and what each reader shows.
• Non-blocking:
  • The oracle tests are gated on target_os = "linux".
  • The scratch directory uses unique_suffix.
  • UnreadableLine implements Error.
  • The module-doc caller list is gone, and test docs no longer narrate the old behaviour.
• Decisions: failing closed everywhere is deliberate. A CRLF file assigns ubuntu\r to sh, and every path now names the line, so the fix is one dos2unix away. CRLF tolerance can follow as its own change if wanted. Over-refusing a lone $, C1 controls and a\ is the intended side of "sh's value or nothing".

@jussielo-amd's follow-ups are both in 38958624: the OpenMPI and torch installs and the three hints now name the unreadable line, and id_and_id_like parses once for both keys. I kept the line verbatim in the reason string, since it is Debug-quoted and is the actionable part.

Every /etc/os-release read now:

• rocm examine and the host distro name read through the simulated host root and the shared parser.
• The package-plan readers read the real file, by test(core): let e2e builds re-root the hardware probes #510's design, but parse through rocm_core::os_release.

All 22 mutations are caught, including routing read_os_release through host_path and bypassing the host root on either examine read.

Verified locally:

• fmt
• tests for rocm-core (621, and 623 with e2e-test-hooks), rocm --bin (1038), xtask (247) and e2e-cucumber lib (142), plus feature_naming
• check --test e2e and check-crate-edges
• clippy on Linux and for x86_64-pc-windows-gnu
• the three new scenarios

Note: open PR #526 also uses the numbers examine-20 to 22, so whichever lands second renumbers.

This approved 014c92c. The head is now f32fd94: a rebase onto main 1e5aac8 (#510), then 3895862, 2c32fb8, d8673df and f32fd94, which address both reviews. The unreadable line is named in the OpenMPI and torch installs and the hints, examine-20/21/22 are added on the host-root seam, and there are tests for the surviving mutants. A fresh approval is needed on the current head.

[jussielo-amd]

Re-reviewed at head f32fd941 (rebased onto main 1e5aac8f, includes #510). Verified rather than taken on trust — checked out the branch locally and confirmed each claim in the latest commits against the code, not just the description:

• B1 (automated review): ensure_openmpi_for_vllm/ensure_torch_runtime_dep (apps/rocm/src/main.rs) and the OpenMPI/libatomic/libnuma hints (crates/rocm-core/src/openmpi.rs) now route through openmpi::plan_for_os_release/install_hint_for, which embed the unreadable line in the reason instead of silently falling back to an empty ID. id_and_id_like now parses the file once for both keys, closing my earlier nit about the double parse.
• B2: examine-20/-21/-22 in tests/e2e-cucumber/features/examine.feature cover the single-quote, duplicate-key and unreadable-line cases against a planted host root.
• B3: the unreadable driver-plan test in apps/rocm/src/driver_install.rs now asserts os_id/version_id/codename/mutating/reboot_required/checks explicitly, closing the mutation gaps the automated review listed.
• B4: docs/testing.md now documents every reader, the rocm diagnose --distro exception, and how to run the new scenarios.

Ran cargo test -p rocm-core --lib os_release (19/19, including the /bin/sh oracle), openmpi:: (28/28) and driver_install:: (57/57) locally against this head — all green.

Approving.

[rominf]

Rebased onto main 2844d797, after #599's lib.rs split and #625; the head is now b761c4dc. The only conflict was the pub mod list in crates/rocm-core/src/lib.rs: os_release now sits next to the new net/paths/process modules. The PR's diff is otherwise unchanged.

Verified locally:

• fmt
• tests for rocm-core (638), rocm --bin (1097), xtask and e2e-cucumber lib, plus feature_naming
• check --test e2e and check-crate-edges
• both clippy invocations, on Linux and for x86_64-pc-windows-gnu
• examine-20, 21 and 22 (3 of 3 passed)

This approved f32fd94. The head is now b761c4d, a rebase onto main 2844d79 (#599, #625); the only conflict was the pub mod list in rocm-core lib.rs, and the PR's diff is otherwise unchanged. A fresh approval is needed on the current head.

[jussielo-amd]

Re-reviewed at head b761c4dc (rebased onto main 2844d797 after #599's lib.rs split and #625). My previous approval was dismissed automatically by the force-push, so I wanted to be sure that was just bookkeeping and not something worth a fresh look.

It was. I pulled both this head and the one I last approved (f32fd941), diffed each against its own base, and compared the two patches with line numbers stripped out — they're identical. The only change anywhere is in crates/rocm-core/src/lib.rs, where pub mod os_release; moved to sit next to the new net/paths/process modules; everything else in the fix is untouched.

This has had a thorough couple of rounds — the automated review's findings, the two rebases to keep up with a moving main, the new examine scenarios. Nice work keeping it all straight through that. Approving again, happily.

[siloteemu]

🔴 Automated review · pr-review-watcher · b761c4d

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.
Fixes #543 ("/etc/os-release is parsed four different ways, and they disagree on spec-valid files"). The change does what that issue asks: every reader now goes through one parser, and an unreadable file is reported by the line that makes it so.

Blocking

• [new] The PR description no longer matches the diff — PR description, "Scenarios" and "The fix" sections
  The description says: "There is no e2e scenario in this PR … Scenarios for a single-quoted file and a duplicated ID, through rocm install driver --dkms --dry-run, … follow once it lands."

  • The diff adds three scenarios, examine-20 to examine-22, and they go through rocm examine --json, not the driver plan.
  • The new comment in examine.feature says the driver plan "cannot be exercised this way". That contradicts the promised driver scenarios.
  • The description also leaves out:
    • the libatomic and libnuma plans and hints, which now use the shared parser too;
    • the new unreadable_os_release driver-plan policy value;
    • the distro: Linux (/etc/os-release line N …) human-readable output;
    • the new docs/testing.md section.

  AGENTS.md §3 asks the PR text to name the @id:s that cover user-observable behaviour and to justify any missing scenario.
  Confidence 85 · mechanical · Fix: update the description to match the diff. Name @id:examine-reads-single-quoted-os-release, @id:examine-reads-last-os-release-assignment and @id:examine-reports-unreadable-os-release, and give the real reason there is no driver-plan scenario.

• [new] No test catches an ID_LIKE regression in the new plan and hint helpers — crates/rocm-core/src/os_release.rs:109-115, crates/rocm-core/src/openmpi.rs:857-860, tests at crates/rocm-core/src/openmpi.rs:1140-1170
  I ran two mutations and the whole openmpi:: and os_release test set stayed green for both:

  • swapping ID and ID_LIKE in id_and_id_like;
  • changing install_hint_for's Ok arm to build(&os_id, ""), which drops ID_LIKE from the hints.

  Why the tests miss it:

  • The only derivative case uses a single-token ID_LIKE='fedora', and resolve_package_manager accepts either argument for a single token.
  • The hint assertion is only starts_with("install it with ")onID=ubuntu`.

  Real impact: a derivative with a multi-token ID_LIKE (for example ID=foo, ID_LIKE="rhel fedora") would plan nothing under the first mutation. Under the second, every derivative would get the manual hint and no command.
  Confidence 100 · logic · Fix:

  • Assert id_and_id_like("ID=rocky\nID_LIKE=\"rhel fedora\"\n") returns the two values in order.
  • Add a multi-token derivative case to the plan and hint tests.
  • Assert the hint equals the joined plan commands rather than a prefix.

• [new] An indented comment line is not tested, so a parser that refuses it stays green — crates/rocm-core/src/os_release.rs:145-146
  The module doc promises sh's value for any file made of blank lines, # comments and spec-valid assignments, and leading spaces and tabs are allowed.
  I changed the comment check to raw_line.starts_with('#'), so an indented comment makes the whole file unreadable. All 133 os_release, pretty_name, openmpi:: and examine tests still passed.
  The sh oracle cannot catch this: it accepts None everywhere, and MUST_READ covers single right-hand sides only.
  Confidence 100 · logic · Fix: add " \t# comment\nID=ubuntu\n" expecting Some("ubuntu"). Also consider an indented invalid line, asserting that UnreadableLine.text keeps the indentation.

• [new] A test comment claims to catch something its assertions cannot fail on — crates/rocm-core/src/examine.rs, doc on os_release_reads_follow_the_simulated_host_root
  The comment says "one that bypassed the parser would keep the quotes." For the distro-name read that is false.

  • The old PRETTY_NAME reader also stripped single quotes.
  • I put the old lenient PRETTY_NAME parsing back in front of distro_name_from_os_release, and this test still passed under --features e2e-test-hooks.
  • The shared parser for the name is in fact covered by pretty_name_reads_as_sh_reads_it. The problem is that this comment points readers at the wrong test.

  Confidence 100 · logic · Fix: limit the claim to routing through the host root plus the quote handling of distro_id/distro_version. Or plant a PRETTY_NAME the old reader got wrong, such as a repeated key.

• [new] os_release::field is dead code — crates/rocm-core/src/os_release.rs:117-124
  It is pub, but a workspace grep finds callers only in this module's tests. Production code calls parse (driver plan, examine, host_gpu) and id_and_id_like (openmpi).
  The module doc justifies it as being "for a caller that only needs the value", and no such caller exists.
  Confidence 85 · mechanical · Fix: make it #[cfg(test)] (or a test helper), or drop it and update the doc sentence.

• [new] docs/testing.md puts the unreadable line in the wrong place for the install hints — docs/testing.md ("The OpenMPI, libatomic and libnuma installs and hints" bullet), crates/rocm-core/src/openmpi.rs:857-862
  The doc says "no commands, and the line ahead of the manual-install advice."

  • That is true of plan_for_os_release's reason.
  • install_hint_for returns "{manual}; /etc/os-release {line}, so the distribution cannot be read", so in the hints the line comes after the advice.
  • The code governs: the function's own doc comment ("followed … by the line") and its test both pin that order.

  Confidence 85 · mechanical · Fix: say "ahead of" for the plans and "after" for the hints.

• [new] A CI comment is now wrong because of this PR — .github/workflows/ci.yml:539-556
  The comment on "Test rocm-core with e2e-test-hooks" says the step covers "the planted-root probe test in examine.rs" (one test). It also lists /etc/os-release among reads that "have no planted-root unit assertion".
  This PR adds os_release_reads_follow_the_simulated_host_root to that step's run, and it asserts both /etc/os-release reads.
  Confidence 85 · mechanical · Fix: move /etc/os-release into the list of sampled reads and mention the new test.

• [standing] The description still does not say what an unreadable file costs — PR description, "An unreadable file is reported, not hidden" and "CRLF files are unreadable too"
  Standing since the review at 014c92c (first blocking finding there), not re-found blind this round. Settled by putting the objection to this round's reviewer after its report was frozen; that verdict is anchored, not blind. The code part no longer holds: every reader now names the line. The description part still holds. It says CRLF files are unreadable but not that, on a CRLF file or one with a stray non-assignment line, the driver plan and the OpenMPI, libatomic and libnuma auto-installs now plan nothing where the old readers worked. Its "reported" sentence still names only the driver plan and probe_failures.
  Fix: state that consequence in the description, and list every place the line is now reported.

• [standing] The PR text does not justify the missing scenarios as AGENTS.md §3 requires — PR description "Scenarios" section, crates/rocm-core/src/examine.rs:546-557, AGENTS.md §3
  Standing since the review at 014c92c (second blocking finding there), not re-found blind this round. Settled the same way; anchored, not blind. The rocm examine distro fields and probe_failures are now covered by examine-20 to examine-22, and the commit-message sentence is fixed. Two parts still hold:

  • The reason the driver plan's --dry-run reason and policy: unreadable_os_release have no scenario is stated in the code and examine.feature, but not in the PR text, which instead promises driver-plan scenarios.
  • The degraded status has no scenario and no named gated lane. The planted host root has no GPU, so examine-22 lands on no-amd-gpu, and only a unit test calls compute_status. §3 asks for a scenario, or for the PR text to name the gated lane that will exercise it.

  Fix: put the driver-plan justification in the PR text, and either add a GPU-lane scenario for degraded or name the lane and why it is not covered here.

Non-blocking

• [new] The vLLM warning still offers --yes when the release file is unreadable — engines/vllm/src/runtime.rs:162-178, engines/vllm/src/process.rs:206-236
  These callers wrap each hint as "{hint}, or rerun rocm engines install vllm --yes.".

  • For an unreadable file the plan is unsupported, so ensure_openmpi_for_vllm and ensure_torch_runtime_dep only warn, and --yes installs nothing.
  • The same wording was already misleading for unknown distros. This PR adds a case where the CLI knows it cannot install and still suggests it.

  Confidence 70 · logic · Fix: for the unreadable case, tell the user to fix the named line rather than rerun with --yes.

• [new] The rewritten hint functions are not tested for which builder and manual text each one uses — crates/rocm-core/src/openmpi.rs:901-955
libatomic_install_hint and libnuma_install_hint are now one-line calls into install_hint_for. Swapping their builder or *_MANUAL_HINT argument would keep every test green, because the BUILDERS table in the tests is separate from production code. This wiring was untested before the PR too.
  Confidence 70 · logic · Fix: test the three public wrappers against fixed text, or have production code and tests share one table.

Decisions for the author

• One unusual line makes the whole file unreadable — tradeoff
  • For: after a line sh would run as a command, no earlier value can be trusted. Planning sudo package commands from a value sh would not assign is worse than planning nothing.
  • Against: a file the old readers handled now blocks rocm install driver and the OpenMPI/libatomic/libnuma auto-installs. Examples are a CRLF file, or one stray line such as ANSI_COLOR=0;31 or ID =x. The message names the line but gives no fix (for example "quote the value") and there is no override.
  • The description's check against 396 of 397 public os-release files suggests mainstream distros are unaffected; hand-edited and custom images are the exposure.
  • Confirm the fail-closed scope is what you want.

Positive signals

• The /bin/sh oracle runs every spec-valid encoding, and every right-hand side up to three characters over a hostile alphabet, against a real shell. It runs with an empty environment, a sentinel HOME and a PATH that does not exist. That compares the parser to its stated reference, not to the author's own idea of the format.
• An unreadable file is reported as itself everywhere: the driver-plan policy, probe_failures, and the human distro: line. The reported line is Debug-quoted, so control characters in it cannot reach a terminal. The old behaviour blamed an unsupported distro.
• The install plans keep reading the real /etc/os-release and the descriptive reads follow the simulated host root. That split is stated consistently in code comments, the feature file and docs/testing.md.

Deployment notes

• rocm install driver JSON output can now carry a new policy value, unreadable_os_release, with supported: false and no commands. Anything that matches on policy should expect it.
• Hosts with a CRLF or otherwise non-plain /etc/os-release will no longer get automatic OpenMPI, libatomic, libnuma or driver plans. rocm examine on those hosts reports degraded, unless the host is WSL or has no AMD GPU.

What this covered

What was read: every file in prw-base…b761c4dc (ee3131be…b761c4dc), all 6 commits. This includes os_release.rs in full, the consumers in openmpi.rs, driver_install.rs, main.rs, examine.rs and host_gpu.rs, the e2e feature and steps, and docs/testing.md. Outside the diff I also read the hint callers in engines/vllm, report.rs's use of distro_id, and hardware_root.rs.

What was run:

• cargo test -p rocm-core --lib filtered to os_release, pretty_name, openmpi:: and examine: 133 passed.
• The same crate with --features e2e-test-hooks, the host-root test: passed.
• cargo test -p rocm --bin rocm filtered to os_release and driver_plan: 30 passed.
• Four single-branch mutations, three reported above as surviving plus the ID_LIKE swap.

What did not run, and why:

• The e2e scenarios were not run locally. They were judged by reading the code: each of examine-20 to examine-22 fails against the old code. For examine-21, only the summary/distro assertion makes it fail, because the old examine loop already kept the last ID.
• The Step 7 design questions were worked through by the driver, not by independent workers.
• The prior-review-comments pass did not run, by design.
• CI on this head:
  • "E2E tests (MI300X)" was still queued, so it has not reported.
  • "Skill checks" and "Sphinx docs build" were skipped. The Sphinx tree is docs/rocm-docs, which the PR does not touch, and no skill files changed.
  • Every other check passed.

Discussion: not reconciled against any PR discussion here.

Standing objections from the review at 014c92c

That round filed a change request with four blocking findings; it has since been dismissed. Each was put to this round's reviewer after its report was frozen (anchored evidence, not blind). Two still hold in part and are listed under Blocking as standing. The other two no longer hold and are withdrawn: every mutant listed in the third finding is now killed by a named test, and all four documentation points in the fourth finding are fixed.

[rominf]

Rebased onto main ee3131be; the head is now ac3c4d03. #653 moved the os_release readers and the ensure_* callers into runtime_install_finalize.rs, so this PR's changes are re-applied there. The PR's added and removed lines are identical before and after the rebase. Neither #634 nor #546 touches what this PR uses. The description is rewritten.

This push answers the automated review of b761c4dc. Each new test fails under the matching mutation.

Blocking

• ID_LIKE tests. id_and_id_like_come_back_in_that_order checks ID=rocky, ID_LIKE="rhel fedora". The plan test now has a multi-word derivative, and the hint must equal the joined plan commands exactly. Both of your mutations now fail.
• Indented comments. Added an_indented_comment_is_a_comment and an_indented_unreadable_line_keeps_its_indentation, which checks the reported text keeps the indentation.
• The examine.rs doc. The test now plants two PRETTY_NAMEs, and the old reader took the first. Putting it back now fails the test, so the doc's claim holds.
• os_release::field. Now a test-only helper, and the module doc no longer mentions it.
• docs/testing.md. The line goes ahead of the advice in the plans and after it in the hints.
• ci.yml. /etc/os-release is now listed among the sampled reads, and the comment names os_release_reads_follow_the_simulated_host_root.
• What an unreadable file costs (standing). The description now says which plans an unreadable file empties, and lists every place the line is reported.
• Scenarios (standing). The description now gives the reason for no driver-plan scenario: those plans read the real /etc/os-release on purpose. degraded now has a scenario. examine-23 (@id:examine-degrades-on-unreadable-os-release) plants a GPU in the kernel topology plus a stand-in lspci that lists an empty bus. It asserts degraded, with the release-file line as the only failure. It runs on every Linux lane. Making compute_status ignore probe failures fails it.

Non-blocking

• vLLM's --yes offer. Each hint now reports whether its plan installs anything, read from the same plan_for_os_release that --yes runs. The offer appears only when it does, which covers the unreadable case and the unknown-distro case you mentioned. the_automatic_install_is_offered_only_when_its_plan_installs asserts the message and the plan together.
• Hint wiring. each_hint_names_its_own_runtime checks each of the three wrappers against fixed text. Swapping a builder or a manual string fails it.

Fail-closed scope: kept. After a line sh would run as a command, no earlier value can be vouched for. Planning sudo package commands from such a value is worse than planning nothing. What was missing was the fix: every message that asks the user to act now also says what to change. That is LF line endings, single quotes around the value, or a NAME=value line. The advice is judged from the whole line, and the_fix_offered_matches_what_is_wrong_with_the_line tests it. There is still no override; editing the line is the fix.

Verified locally on ac3c4d03, all passing:

• fmt
• tests: rocm-core --lib (651) plus e2e-test-hooks (6), rocm --bin (1150), vLLM (98), xtask (252), e2e-cucumber lib (142) plus feature_naming
• cargo check --test e2e
• e2e examine-2 (4 of 4)
• clippy, both invocations, plus Windows clippy

examine-23 has not run on a GPU lane yet.

Addressed in f3cd8a5, 73cca5f, 53ce833, b5eda2c and ac3c4d0 (head ac3c4d0, rebased onto ee3131b), plus the rewritten description. The PR comment of 2026-10-13 answers each finding.

[jussielo-amd]

Re-reviewed at head ac3c4d03 (rebased after #653 moved the os_release readers and ensure_* callers into runtime_install_finalize.rs). I checked this round's claim against the diff directly rather than taking it on trust: every blocking and non-blocking finding from the automated review at b761c4dc is genuinely fixed in the code —

• id_and_id_like_come_back_in_that_order covers the multi-word ID_LIKE case
• an_indented_comment_is_a_comment / an_indented_unreadable_line_keeps_its_indentation cover the indentation handling
• os_release::field is now test-only, moved inside mod tests
• docs/testing.md and the ci.yml comment both match the actual line-order/coverage behavior
• examine-23 (@id:examine-degrades-on-unreadable-os-release) covers degraded on a GPU host with an unreadable release file
• the vLLM --yes offer (the_automatic_install_is_offered_only_when_its_plan_installs) and the hint wiring (each_hint_names_its_own_runtime) are both now pinned by tests

The PR description is also rewritten to cover what an unreadable file costs and why there's no driver-plan scenario, closing out the two standing findings from the earlier round.

The one red check, "E2E tests" (engine-shell-marks-the-prompt), is a pre-existing test-harness flake unrelated to this PR's changes — #663 explicitly names #544 as one of the PRs hit by it and has a fix in flight. Every other check is green; MI300X was still queued at review time.

Approving again.