[rominf]

Follow-up to #566, from the first non-blocking note in @juhovainio's approving review.

parse_host_version drops the packaging build number that a system ROCm install reports (7.2.4-98 is read as 7.2.4). Its doc says only a numeric - suffix is dropped. The code, however, only checked that the suffix starts with a digit, so 7.2.4-98rc1 would have had -98rc1 dropped silently and been read as 7.2.4.

Change

• The suffix is now dropped only when every character is a digit (tail.chars().all(|c| c.is_ascii_digit())).
• Any other suffix stays on the string, and the whole string goes to the PEP 440 ordering parser:
  • 7.0.0-rc1 is read as a release candidate, as before.
  • 7.2.4-98rc1 cannot be read, so the host version is "can't tell".
• The doc comment now states this behaviour.

What users see
parse_host_version has one production caller: the check behind the install-time version_note ("this host reports ROCm X, but Y … is selected"). For a host string that has a mixed suffix, that note now never appears. Before, it could appear based on the version with its suffix dropped. "Can't tell" never claims that the host is newer.

rocm update statuses do not go through this function: they use compare_version_strings. So none of the update- scenarios need to change. update-07 and update-08 compare catalog versions with installed runtimes, not host strings.

Test
parse_host_version_drops_only_an_all_digit_build_suffix covers:

• an all-digit suffix (7.2.4-98 is read as 7.2.4);
• 7.0.0-rc1;
• 7.2.4-98rc1 (unreadable);
• the user-facing effect: 7.20.1-33rc1 is not reported as newer than 7.13.0, while 7.20.1-33 still is.

Mutation check: with the check reverted to starts_with, the new test fails (left: Some(VersionOrderKey { release: [7, 2, 4], .. }), right: None).

The proptest oracle also calls parse_host_version. Its version alphabet has no suffix that starts with a digit and goes on with something else, so its results are unchanged.

Verified locally:

• fmt

• rocm --bin rocm tests: 1307 pass

• xtask tests: 317 pass

• both clippy invocations on Linux and for x86_64-pc-windows-gnu

• No ticket ID; no xfail rows apply.

• No new subcommand or subsystem.

• No message wording changed. The test asserts when the host-newer note is produced, not its text.

[tomastola]

Review of #728: needs work

The production fix is correct, and the doc comments now match the code. The PR is held back by two gaps around the new behaviour: one guard has no test, and there is no e2e scenario.

Full review of the whole change (one commit, apps/rocm/src/therock.rs).
No ticket named. It is a follow-up to #566, and the description matches what the diff does.

Blocking

• The new empty-tail guard is not tested — apps/rocm/src/therock.rs:6953, test at :12127

  • The new condition has two parts: !tail.is_empty() && tail.chars().all(|c| c.is_ascii_digit()).
  • I removed !tail.is_empty() and ran cargo test -p rocm --bin rocm -- host_version. All 3 tests still passed.
  • Without the guard, .all() is vacuously true on an empty tail. So 7.2.4- would quietly be read as 7.2.4, when today (and under the old rule) it is None.
  • The repo's AGENTS.md rule ("test fails before fix and passes after fix") covers the reverted fix. It does not cover this branch.
  • Confidence 100 · logic · Fix: add assert_eq!(parse_host_version("7.2.4-"), None);.

• No e2e scenario, and no stated reason for skipping one — apps/rocm/src/therock.rs:6948

  • AGENTS.md §3: "If the change alters what a user of the CLI can observe — command output … that behavior must be covered by a Gherkin scenario … a unit test asserting the internal helper does NOT discharge this."
  • The description itself says the install-time version_note "now never appears" for a mixed-suffix host. That is a change in command output.
  • No scenario in tests/e2e-cucumber/ covers the host-newer note. A grep for this host reports ROCm, version_note and host.*newer finds nothing.
  • The description explains why the update- scenarios are unaffected, but never addresses the scenario rule for the install path.
  • A scenario looks practical: plant_unmanaged_rocm (tests/e2e-cucumber/tests/e2e.rs:579) already writes .info/version.
  • Confidence 85 · convention · Fix: either add a scenario that plants .info/version with a mixed suffix and asserts the note is absent, or state in the PR text why one is not warranted.

Non-blocking

• The last assertion in the new test duplicates an existing one — apps/rocm/src/therock.rs:12149-12152
  • host_version_newer_than(Some("7.20.1-33"), "7.13.0") == Some("7.20.1-33") is already asserted, word for word, at :12117-12120.
  • Confidence 85 · mechanical · Fix: delete it, or replace it with the 7.2.4- case above.

Decisions for the author

• Other packaging tails now become "can't tell" too — tradeoff

  • The all-digit rule rejects more than 98rc1. Tracing by hand (not run):

    Host string	Old rule	New rule
    7.2.4-98.el9	7.2.4	None
    6.4.1-120~22.04	6.4.1	None
    7.2.4-98-1	7.2.4	None
    7.2.4-98+local	7.2.4	None

  • The +local case is a little inconsistent. split_once('-') runs before the ordering parser strips +…, so 7.2.4+local-98 parses but 7.2.4-98+local does not.

  • Real risk looks low. The host string comes from .info/version* or the rocm-X.Y.Z directory name (crates/rocm-core/src/rocm_install.rs:210). .info/version is normally X.Y.Z-N, and I could not confirm what version-utils or version-libs contain on real installs. The worst case is a missing note, never a false one.

  • Strict (current): no way to misread a version, at the cost of losing the note for distro-style tails.

  • Lenient alternative: drop the leading digit run when it is followed by end, ., ~, + or -, and reject only when letters follow directly (the 98rc1 case).

  • Either way, a couple of these shapes as test cases would put the choice on record.

Positive signals

• host_version_newer_than("7.20.1-33rc1", "7.13.0").is_none() tests what the user sees: under the old rule it would have produced a false "host is newer" note.
• The new doc paragraph explains why checking only that the suffix starts with a digit was wrong, which stops it coming back.
• The change is confined to the host-parsing path. The comparator, version_relation and the update flow are untouched.

Deployment notes

None.

[nowycondro]

🔴 Automated review · pr-review-watcher · 034970b

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 1c33248…pr-728 034970b). Implements and fully closes ROCMAI-674 ("a host ROCm version with a mixed build suffix such as 7.2.4-98rc1 is read as 7.2.4") — a narrow, self-contained fix with no unaddressed remainder.

Blocking

• CI is red on this commit, for reasons unrelated to the diff — E2E tests (MI300X, current) (job 121516351833), E2E tests (Strix Halo, Windows, current) (job 121516351719), and E2E consolidated report (self-hosted) (job 121564010295, a cascade of the Windows failure not uploading its report artifact).
  Traced both root causes directly from the job logs: the Windows lane fails downloading a TheRock tarball (HTTP error 403 from repo.amd.com) before any rocm code under test runs; the MI300X lane fails scenario therock-next-09-live-install-reports-vllm-rocm10x-discovery-pins on a uv pip install dependency conflict between a nightly vllm build and its pinned amd-aiter version — again before touching parse_host_version or host_version_newer_than. Neither failure exercises the changed code path. build-and-test, windows-build-and-test, Test (affected crates), and clippy all pass, which is the evidence that actually bears on this change.
  Confidence 85 · mechanical · Fix: rerun the two GPU lanes (or escalate per AGENTS.md §12's "infrastructure flakes" handling); no code change indicated.

Non-blocking

None

Decisions for the author

None

Positive signals

• The fix is traced correctly against every call site. parse_host_version has exactly one production caller (host_version_newer_than), and the new all-digit check was verified by hand against every existing assertion (7.2.4-98, 7.0.0-rc1, 7.4, 7.13.0-56, unknown) plus the new ones (7.2.4-98rc1, 7.20.1-33rc1) — all parse exactly as documented, with no regression on the pre-existing test host_version_newer_than_reports_only_a_strictly_newer_host.
• The three doc comments describing the old "numeric suffix" rule were all updated to "all digits," consistently (compare_version_strings's and parse_version_for_ordering's module docs, plus parse_host_version's own doc and inline comment) — a grep for the old wording turned up nothing stale.
• The new test sits next to, and mirrors the structure of, the existing host_version_newer_than_reports_only_a_strictly_newer_host, including a comment explaining the user-visible consequence of the fix, consistent with this file's documentation-heavy style.

Deployment notes

None — this narrows an existing edge case (an unreadable host version is treated as "can't tell" rather than silently misread); it does not change any CLI flag, message wording, or routing behavior, per the PR's own description, which this review's blind trace corroborates.

What this covered

Read completely: apps/rocm/src/therock.rs's changed region and its surrounding context — compare_version_strings, parse_version_for_ordering, parse_version_stage, split_leading_number, parse_version, parse_host_version, host_version_newer_than, host_rocm_version_newer_than, and wheel_host_version_note/tarball_host_version_note — plus the new and neighboring unit tests. Read AGENTS.md in full for agent-instruction adherence. Ran the history pass (git log, targeted git show) and traced parse_host_version to its introduction in #566 (same author), confirming this is a legitimate follow-up fix to that PR's own logic, matching the PR body's "follow-up to #566" claim. Checked impact: parse_host_version has exactly one caller, so the fix's blast radius is fully covered by the single changed call path.

Diff is small and tightly coupled to one function, its two doc sites, and one new test, so this ran as a single pass rather than a fan-out, per the task's own Step 3 guidance for a change this size.

Did not run: cargo test/cargo clippy locally — no Rust toolchain available in this sandbox; relied on manual trace of every branch (cross-checked against all assertions in both the new and pre-existing tests) plus GitHub's own Test (affected crates) and clippy checks, which both report success for this commit. "Prior-changes" pass found no separate review-thread history to check beyond the commit message of #566 (not retrieved as PR review discussion, consistent with this review's blind-findings rule). Not reconciled against any PR discussion or review comments — none were read, per this method's rule that discussion is never read even at Step 9.

[nowycondro]

🤖 Automated review (Claude Code) · 034970b

Reviewed the single changed file, apps/rocm/src/therock.rs, focusing on parse_host_version and its production caller host_rocm_version_newer_than. The change (requiring the hyphen-tail to be all-digits, rather than merely digit-starting, before stripping it as a build number) correctly implements the stated intent, and the new test (parse_host_version_drops_only_an_all_digit_build_suffix) exercises the documented cases well: 7.2.4-98 → 7.2.4, 7.0.0-rc1 preserved, and 7.2.4-98rc1 → unreadable. I traced edge cases (multiple hyphens, leading/trailing hyphens, empty tails) and confirmed compare_version_strings/version_relation (used by rocm update) are untouched by this fix, matching the PR description.

One low-confidence, non-blocking behavioral note: at apps/rocm/src/therock.rs:6952, the all-digits check on the hyphen-tail runs before any local-version metadata (+...) would be stripped. A host string like 7.2.4-98+local now falls through to the PEP 440 ordering parser with a dangling -98+local suffix and likely returns "can't tell" where the old code would have (accidentally) read it as 7.2.4. This is a narrowing of behavior consistent with the PR's stated goal of being stricter, and such local-metadata suffixes aren't expected in .info/version files or ROCm directory names, so it's very unlikely to matter in practice — flagging only for awareness, not as a blocker.

I did not build or run the test suite myself (no Rust toolchain set up in this sandbox); I relied on manual tracing of the diff and the PR's reported local verification (fmt, rocm/xtask test runs, clippy). No other correctness issues found.

🤖 Generated with Claude Code