[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

[tomastola]

Re-review of #728: ready to approve

Every point from my previous review is answered.

This review covers only the new commit, 9cf7266, which adds tests only. The production fix from 034970b is unchanged, and I already reviewed it as correct. The branch still merges cleanly into current main. The new scenario IDs runtime-20 and runtime-21 don't clash with anything on main, which stops at runtime-19.

What happened to each point from my last review

Point (034970b)	Status	Evidence
Blocking: the empty-tail guard had no test	Fixed	Adds assert_eq!(parse_host_version("7.2.4-"), None). I removed !tail.is_empty(), and the test now fails at therock.rs:12151 (left: Some(VersionOrderKey { release: [7, 2, 4], .. }), right: None). Before this commit, that mutation went unnoticed.
Blocking: no e2e scenario for the changed install-preview note	Fixed	Adds runtime-20 (a readable newer host such as 7.20.1-33 prints the version_note) and runtime-21 (the unreadable 7.20.1-33rc1 makes no newer-host claim). Both pass locally. I reverted the fix to the old starts_with rule, and runtime-21 fails, printing the false note: version_note: this host reports ROCm 7.20.1-33rc1, but 7.10.0 is … selected. So the scenario catches the original bug, not just the helper function.
Non-blocking: duplicated 7.20.1-33 assertion	Fixed	Removed. The same check still exists in the neighbouring test.
Decision: strict vs lenient handling of distro-style tails	Decided: strict, and now pinned by tests	Tests now assert that -98.el9, -120~22.04, -98-1 and -98+local are all unreadable. A comment says why: a host with such a version loses the note, but never gets a false one. That puts the choice on record.

The new test code

• runtime-20 makes runtime-21 meaningful. It shows the same preview printing the note when the host version is readable, so the note's absence in runtime-21 is real evidence. Both step comments say so.
• runtime-20 asserts the note text together with the selected artifact (tarball: and latest_version: 7.10.0). That follows the AGENTS.md rule that a message must be asserted together with the behaviour it describes.
• The scenarios need no network or GPU: they use a loopback catalog, a named --family and a planted .info/version. They're tagged @requires-os:linux, which is correct because --format tarball is refused on Windows. They should therefore run on the ordinary Linux E2E lane.
• plant_unmanaged_rocm now delegates to plant_unmanaged_rocm_reporting("6.0.0"), so existing callers behave the same.

Blocking

None.

Non-blocking

• The PR description is out of date (PR body, "Test" section)
  • It still describes only the unit test, and lists "7.20.1-33 still is [newer]" as one of that test's cases. This commit removed that assertion from the test.
  • It doesn't mention runtime-20 / runtime-21, the 7.2.4- case, or the distro-tail cases. AGENTS.md asks the PR text to name the scenario that covers a user-visible change.
  • Confidence 90 · documentation · Fix: list the two scenario @id:s and the new unit-test cases under "Test".

Decisions for the author

None left. The strict-vs-lenient question is settled and covered by tests.

Verification I ran (branch at 9cf7266, Linux)

• cargo test -p rocm --bin rocm -- host_version: 3 tests pass.
• With the emptiness guard removed: the new test fails. With it restored: it passes.
• cargo test -p e2e-cucumber --test e2e -- -n "runtime-2[01]": 2 scenarios, 9 steps, all pass.
• With the production fix reverted: runtime-21 fails with the false note shown above.
• cargo fmt --all -- --check is clean. cargo clippy -p rocm -p e2e-cucumber --all-targets -- -D warnings is clean.

Note

CI has not run on 9cf7266 yet. The only result on the commit is Read the Docs (pass). There are no Actions runs at all (CI, CodeQL, E2E self-hosted), whereas 034970b got all three. The workflows may need re-triggering before merge; the local runs above are the only evidence so far.

[nowycondro]

🤖 Automated review (Claude Code) · cb98413

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 (two commits: 0f3cc3a9 the production fix, cb984134 adding unit-test and e2e coverage), run as four independent fan-out passes (production-fix + unit-test; e2e scenarios + step definitions + helper; agent-instruction adherence against AGENTS.md; history and prior-review-applicability), then consolidated here.

Implements and fully closes Jira ROCMAI-674 ("a host ROCm version with a mixed build suffix such as 7.2.4-98rc1 is read as 7.2.4") — retrieved directly from Jira, and its description matches the PR's own account of the bug exactly, with no unaddressed remainder. The production fix matches both the ticket and the PR description; the PR description itself, however, does not mention everything the current diff contains (see Blocking).

Blocking

• The PR description omits the second commit's additions — PR body ("Test" section); undocumented work lands at tests/e2e-cucumber/features/runtime_setup.feature:431-453, tests/e2e-cucumber/tests/e2e/therock_steps.rs:1511-1596, apps/rocm/src/therock.rs:6151-6161, tests/e2e-cucumber/tests/e2e.rs:604-618
  The live PR body's "Test" section still describes only parse_host_version_drops_only_an_all_digit_build_suffix's original cases (all-digit suffix, 7.0.0-rc1, 7.2.4-98rc1, and "7.20.1-33rc1 is not reported as newer... while 7.20.1-33 still is"). It says nothing about the two new Gherkin scenarios (@id:runtime-install-preview-notes-newer-host-rocm / runtime-20, @id:runtime-install-preview-ignores-unreadable-host-rocm / runtime-21), the empty-tail case (7.2.4-), the four distro-packaging-tail cases, or the plant_unmanaged_rocm_reporting helper — all of which are present at the current head. It also still claims "7.20.1-33 still is [newer]" as one of the new test's own assertions, but that duplicate assertion was removed from the new test by the second commit (it now exists only in the neighboring pre-existing test). AGENTS.md §5 asks the PR body to be written from the final diff and re-read after every follow-up commit, listing every user-visible change and naming the lane that exercises new behavior — exactly what the two new scenarios are and aren't named here.
  Confidence 90 · documentation · Fix: update the PR body's "Test" section to name both scenario @id:s and the additional unit-test cases, and correct the stale duplicate-assertion claim.

• CI is not green on this commit — checks E2E tests (Strix Halo, Windows, current) (FAILURE), E2E tests (MI300X, current) (QUEUED), E2E tests (Strix Halo, Ubuntu, release, current) (QUEUED), E2E tests (Strix Halo, Ubuntu, nightly, current) (IN_PROGRESS), as observed at review time
  Traced the Windows failure directly from its job log (run 41661809200, job 126006886858): scenario runtime-09 - Previewing a release SDK install resolves the canonical aggregate for this GPU (@id:runtime-resolve-canonical-release) fails on assertion left == right, canonical_source: expected https://repo.amd.com/rocm/whl-multi-arch, got https://mirror-lax.techaccelerator.com/repository/pypi-therock — a live-hardware package-mirror mismatch, unrelated to parse_host_version/host_version_newer_than/the host-version note this PR touches. The checks that actually exercise this diff (build-and-test, windows-build-and-test, Test (affected crates), clippy) all pass. Three GPU-dependent E2E lanes had not finished as of this review (two queued, one in progress), so the diff's full E2E surface isn't yet confirmed all-green.
  Confidence 85 · mechanical · Fix: rerun/wait on the GPU lanes (or escalate per AGENTS.md §12); no code change indicated for the Windows failure.

Non-blocking

None.

Decisions for the author

• The all-digit rule also rejects distro-style packaging tails, not just the reported rc1 shape — tradeoff
  7.2.4-98.el9, 6.4.1-120~22.04, 7.2.4-98-1 and 7.2.4-98+local all become unreadable ("can't tell") under the new rule, same as 7.2.4-98rc1. This is already deliberate and tested (the four distro-tail cases are asserted None in the new unit test, with a comment explaining the cost is only ever a missing host-newer note, never a false one), so there's nothing left to fix — flagging only so the choice (strict/no-false-positives over lenient/fewer-missed-notes) stays visible as a design call rather than reading as an afterthought.

Positive signals

• The production fix was hand-traced against every existing and new assertion, every call site of host_rocm_version_newer_than (both the wheel and tarball version_note sites), and edge cases beyond the tests (multiple dashes, non-ASCII digits, +local ordering relative to the - split) — no correctness defect found anywhere.
• All three doc-comment sites describing the old "numeric suffix" rule were updated to "all digits" consistently; a full-file grep for the old wording turned up nothing stale.
• runtime-20 and runtime-21 are genuinely coupled, not just parallel scenarios: runtime-20 proves the install preview can print the host-newer note from this exact fixture shape, which is what makes runtime-21's absence assertion real evidence rather than a vacuous pass, and preview_explains_host_newer asserts the note's text together with the state it describes, per AGENTS.md's "message asserted together with the behavior" rule.
• This is a traceable, legitimate follow-up: the exact gap closed here was flagged, almost word-for-word, as a non-blocking note in the human review of the parent PR (#566) that introduced parse_host_version, and the PR's own "follow-up to #566" claim checks out against that review text.
• No existing test was weakened or deleted, and the one duplicated assertion a prior round of feedback identified has since been cleanly removed rather than left in place.

Deployment notes

None — this only narrows when an existing "host is newer" note is shown (removing false positives); it changes no CLI flag, message wording, or routing behavior.

What this covered

Read completely at PR head cb98413491dfe05327a77d770cacfb5a6ace6d84 (merge-base e6d2f7c0d7b9a8051a475dfaca84756e5b97aecf): apps/rocm/src/therock/version.rs in full; the full mod tests region of apps/rocm/src/therock.rs plus every non-test call site of parse_host_version/host_version_newer_than/host_rocm_version_newer_than in that file; the full tests/e2e-cucumber/features/runtime_setup.feature; the full tests/e2e-cucumber/tests/e2e/therock_steps.rs; the diffed region of tests/e2e-cucumber/tests/e2e.rs plus every caller of plant_unmanaged_rocm across the e2e test tree. Read AGENTS.md in full for the agent-instruction-adherence pass. Ran the history/prior-changes pass against full local git history, including PR #566's merged diff and its human review text. Checked CI via a live gh pr view --json statusCheckRollup query and traced the one failing job's log directly, not from workflow-definition guesswork.

Did not run: cargo test/cargo clippy locally (no Rust toolchain in this environment) — relied on hand-tracing every branch against the PR-head source, cross-checked against GitHub's own passing Test (affected crates)/clippy/build-and-test checks for this exact commit.

Not reconciled against any PR discussion or prior review thread content: the fan-out passes that produced every finding above read only the code, AGENTS.md, and git history, never any review or comment. As orchestrator I did see this PR's existing reviews while performing the required same-identity claim-check (to confirm no prior review of mine already covers this exact commit); none of their text, conclusions, or framing was passed to the workers, and every finding and positive signal above was independently re-derived from the code, logs, or a live query rather than carried over.

---

🤖 Generated with Claude Code