[rominf]

Summary

• rocm version always led with the workspace package version, so a tagged release and a build from main both read as 0.1.0. A vX.Y.Z tag now leads with that version (X.Y.Z (vX.Y.Z, <hash>)). Every other resolved ref, including main, leads with 0.0.0-dev. The same string is what the update report prints.
• @id:examine-version checks that shape on every CLI surface. Unit coverage is build_metadata, version_surfaces_share_one_build_script_string, and the update-report surface test.

Test plan

• build_metadata (7 tests), version_surfaces_share_one_build_script_string, and the update-report surface test passed locally. The report path calls rocm_core::prepare_report.
• CI runs those unit tests and @id:examine-version on the default e2e lane.
• Full workspace cargo test and clippy were not run. This worktree was not rebuilt with cargo build --workspace --all-targets. A later build of the origin/main checkout does not prove this branch compiles.

[nowycondro]

🤖 Automated review (Claude Code) · b003c58

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 (10 files, b003c58 vs. base cc56179).
No ticket named in the PR body. The description's "update report" claim checks out (rocm update's own report, apps/rocm/src/update_cmd.rs, is correctly fixed) — but the new README text separately and explicitly claims the rocm diagnose --report/--send surface shares the same string, and that one was not fixed (see Blocking #1).

Blocking

• rocm diagnose --report/--send still emits the frozen workspace version, contradicting the PR's own new README text — crates/rocm-core/src/report.rs:405, README.md (new paragraph added by this diff).
  prepare_report() still sets cli_version: env!("CARGO_PKG_VERSION").to_owned() — confirmed via git diff that crates/rocm-core/src/report.rs is untouched by this PR. rocm-core's Cargo.toml inherits the same frozen workspace version (0.1.0) the PR's own commit message names as the bug it fixes. The call path (apps/rocm/src/main.rs:3216's show_prepared_report → rocm_core::prepare_report) never overrides this field with the new cli_version_string()/cli_version_number(). Meanwhile this diff adds a new README paragraph describing exactly this --report content and states: "That value is the same string rocm version prints" — which is false for this surface as shipped. This is doubly significant because the module doc for report.rs says this report is "destined for a public, indexed issue tracker" — so public triage reports will keep showing a meaningless, frozen 0.1.0 for every build, release or not, the exact problem this PR exists to fix.
  Confidence 100 (traced the full call path; confirmed empty diff on the file; cross-checked against the new README wording). Category: architectural (missed consumer of the new shared version string).
  Fix: thread cli_version_string()/cli_version_number() from apps/rocm into prepare_report's cli_version (e.g. as a parameter, since rocm-core has no way to read apps/rocm's build-script env var itself), or narrow the new README claim to the surfaces this PR actually fixes.

• Required CI check "Commit signatures + sign-off" is failing on this commit — b003c58a8c9d1ef0d4a0d8f434c5c8063d498e53.
  Job log: verify-commits: 1 check(s) failed: b003c58a ... -> signature is not GitHub-"Verified". The commit does carry an SSH signature and a Signed-off-by trailer, but GitHub does not recognize the signature as "Verified" (likely an unregistered signing key or an unverified committer email). AGENTS.md documents this as an enforced, blocking CI gate (docs/commit-signatures.md), and the PR's mergeable_state is blocked.
  Confidence 100 (read directly from the job log). Category: mechanical/process.
  Fix: register the SSH/GPG signing key on the author's GitHub account (or correct the committer email verification) and re-push so the commit shows "Verified".

Non-blocking

• The release-tag half of the new behavior is never exercised by CI end-to-end, only by unit tests — apps/rocm/build.rs (format_cli_version_reports_a_release_tag_without_its_leading_v), .github/workflows/release.yml.
  ref_descriptor() resolves to a branch name or main in every workflow that runs scripts/smoke_local.py or the @id:examine-version e2e scenario (push/PR/merge_group triggers); release.yml, the only tag-triggered workflow, runs neither smoke_local.py nor the e2e-cucumber suite — only scripts/release_readiness.py, which has no version assertions. So the vX.Y.Z (vX.Y.Z, <hash>) shape is solidly unit-tested but never produced and checked from an actual tagged build.
  Confidence 85 (confirmed workflow trigger conditions and grepped release_readiness.py/release.yml for version-related calls). Category: test-coverage.
  Fix: have release.yml run assert_version_string/smoke_local.py (or an equivalent check) against the built release binary before publishing.

• The "unknown" sentinel is indistinguishable from a resolved ref literally named unknown — apps/rocm/build.rs's displayed_version/ref_descriptor.
  A local branch named unknown, or ROCM_CLI_RELEASE_REF=unknown/GITHUB_REF_NAME=unknown, would take the "ref did not resolve" fallback path (display the package version) rather than the correct "non-release dev build" path (0.0.0-dev), even though the ref actually resolved. Real-world likelihood is very low, and an existing test already encodes this as current behavior, but the module doc never calls out the ambiguity.
  Confidence 78. Category: logic (edge case).
  Fix: use Option<String> internally for "ref not resolved" instead of overloading the string "unknown", or document the limitation.

• A few edge cases of release_version are correct but untested — apps/rocm/build.rs tests module.
  v1.2.3.4 (too many components — explicitly called out by the function's own code comment), a combined -rc.1+build suffix, and the symmetric v1.2.3+ (empty build suffix) all parse correctly today (verified by hand-tracing/simulation) but have no regression test, unlike their sibling cases.
  Confidence 80. Category: test-coverage.
  Fix: add these three cases to the existing format_cli_version_reports_main_as_a_dev_build/...without_its_leading_v tests.

• A unit test's doc comment overclaims what it verifies — apps/rocm/src/main.rs:5546, version_surfaces_share_one_build_script_string.
  Its doc comment says -V/--version and rocm version "cannot disagree," but the test never captures rocm version's actual printed stdout — it only checks cli_command().get_version() and shape/derivation consistency against the build-script env vars. The real cross-surface guarantee comes from the e2e assert_version_returned step, which does run the compiled binary.
  Confidence 75. Category: mechanical (doc/test-claim mismatch).
  Fix: narrow the doc comment to what this specific unit test checks.

Decisions for the author

None.

Positive signals

• release_version's vX.Y.Z/pre-release/build-suffix parsing logic was independently traced and simulated against roughly 20 edge cases (too-few/too-many components, combined -/+ suffixes, empty suffixes, non-digit components, names merely starting with v) by two independent reviewers and holds up in every case.
• scripts/smoke_local.py's assert_version_string and the e2e assert_version_returned step compare the parsed version/ref against each other from the binary's own actual output, rather than re-implementing release_version's parsing independently — this avoids the Python/Rust parser drift that would otherwise be a real risk.
• The chat.rs MCP test change from a weak substring check to an exact-line match is a genuine strengthening, not just different brittleness — traced the real code path and confirmed no incidental formatting could cause a false pass.
• ROCM_CLI_VERSION is computed exactly once in build.rs and exported as a single cargo env var, and embedded_cli_version_uses_format_cli_version ties the real build output to the tested function so the two can't silently drift.

Deployment notes

• For every non-tag build (branches, main, local dev, standard CI), the CLI's reported leading version changes from the frozen 0.1.0 to 0.0.0-dev across rocm version, -V/--version, the MCP version tool, and rocm update's cli: installed= field. Anything downstream that parses or compares against a literal 0.1.0 CLI version string needs updating.
• The rocm diagnose --report/--send surface (crates/rocm-core) is unaffected by this PR (see Blocking #1) and will keep reporting 0.1.0 regardless of build, contrary to the PR's own new documentation.

What this covered

Read all 10 changed files completely (README.md, apps/rocm/build.rs, apps/rocm/src/chat.rs, apps/rocm/src/main.rs, apps/rocm/src/update_cmd.rs, apps/rocm/tests/build_metadata.rs, scripts/smoke_local.py, tests/e2e-cucumber/features/examine.feature, tests/e2e-cucumber/tests/e2e/examine_steps.rs, xtask/src/e2e_prewarm.rs), plus surrounding/caller context: ref_descriptor, every cli_version_string/cli_version_number call site, rocm_core::prepare_report and its one call site, the repo's AGENTS.md conventions, CI workflow trigger conditions (ci.yml, release.yml, e2e-selfhosted.yml, nightly.yml), and git history of apps/rocm/build.rs. Diffed against base cc56179ced9dd1be42a3000b2592ace9394dbff1. Work was split across three independent fan-out workers (core build.rs logic; main.rs/update_cmd.rs/chat.rs consumers; docs/e2e coverage) plus this driver's own consolidation and impact-analysis pass.

Read CI check-runs directly: all required checks pass (build-and-test, clippy, Windows build, Test (affected crates), Coverage, E2E tests, CodeQL, etc.) except "Commit signatures + sign-off" (failing, see Blocking) and a few hardware-gated E2E jobs still in progress/queued at time of review.

Did not run: a separate "prior changes" pass (no prior review threads existed on this PR to check against at review time, confirmed via get_reviews). Did not execute cargo test/clippy locally; relied on the PR's own CI check-run results instead. Not reconciled against any PR discussion (none existed at time of this review).

---

🤖 Generated with Claude Code

[juhovainio]

Two things need to change before this can merge.

Blocking

1. The README and PR body say the --report CLI version is the same string rocm version prints, but crates/rocm-core/src/report.rs still sets cli_version: env!("CARGO_PKG_VERSION") (0.1.0). rocm-core never sees ROCM_CLI_VERSION, and the report tests pin "9.9.9" rather than the real value. Either plumb the version through with a test on the real value, or drop the claim.
2. The Commit signatures + sign-off check fails: the commit is not GitHub-Verified (unknown_key). The sign-off is present, so the commit needs re-signing with a key GitHub knows.

Non-blocking

• The e2e step, smoke script and main.rs unit test would still pass if a tag build printed 0.0.0-dev (vX.Y.Z, h), since the else-branch asserts exactly that. Only the build.rs unit tests cover the tag shape; please say so in the PR text and show that the new tests go red when the target is broken.
• rocmd MCP serverInfo, DAEMON_VERSION and the dash client string still report 0.1.0, which conflicts with "every CLI surface". Fix them or say they are out of scope.
• cli_version_number() splits on the first space, an implicit coupling to the format update_cmd relies on.
• Please check docs/testing.md and docs/manual-testing.md for claims about the version shape.
• The two Strix Halo Ubuntu nightly/release e2e jobs fail on an HTTP 404 fetching llama.cpp b9752; that looks unrelated to this diff but needs a rerun.

[juhovainio]

This says the report's CLI version is the same string rocm version prints, but crates/rocm-core/src/report.rs still sets cli_version: env!("CARGO_PKG_VERSION"), so the report keeps reporting 0.1.0. Either plumb ROCM_CLI_VERSION into the report (and test the real value) or drop this sentence and the matching PR-body claim.

[rominf]

Fixed in 82d3a76: prepare_report now takes the CLI version from its caller, and rocm passes the leading version rocm version prints (ROCM_CLI_VERSION_NUMBER, exported by build.rs beside the full string). Only that token is published, because the ref after it can be a local branch name; anything that is not a short version token is published as unknown. Covered by a_problem_report_names_the_version_rocm_version_leads_with and by the new @id:diagnose-report-names-the-cli-version (diagnose-48), which plants a released GPU on a simulated host so it prepares a real report on every Linux lane. The README sentence now says it carries the leading version only.

[juhovainio]

Tag-shape coverage lives only in the build.rs unit tests. The e2e step and smoke script pass even if a tag build printed 0.0.0-dev (vX.Y.Z, h) (the else-branch asserts exactly that). Please say in the PR text that tag shape is covered at unit level only.

[rominf]

Agreed; the PR text now says the tag shape is covered only by the format_cli_version_* unit tests in build.rs (checked to fail when the parser is broken). The e2e step and smoke script check the shape and that the leading version is consistent, not the tag case, because no CI build runs from a tag.

[nowycondro]

🤖 Automated review (Claude Code) · 82d3a76

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 (15 files, 82d3a76 vs. base 3dec0eb). No ticket named in the PR body. The PR's own claims are scoped correctly this round (it no longer overclaims "every CLI surface" beyond the rocm binary, and the README/docs text matches the implemented behavior).

Both blocking issues from the prior review round on this PR are now resolved: crates/rocm-core/src/report.rs's prepare_report now takes cli_version as a parameter threaded from the real build-time version string (traced the full call path fresh, with non-vacuous tests asserting real version shapes, not a pinned placeholder), and the "Commit signatures + sign-off" CI check now passes. Two new issues surfaced in this round instead.

Blocking

• A new clippy lint violation in this PR's own new e2e code fails required CI gates — tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:3340-3344.

  let leading = first_line
      .strip_prefix("rocm-cli ")
      .and_then(|rest| rest.split_once(" ("))
      .map(|(version, _)| version)
      .unwrap_or_else(|| panic!("expected 'rocm-cli <version> (<ref>, <hash>)': {first_line}"));

  This is exactly the .map(f).unwrap_or_else(g) shape clippy::map_unwrap_or flags, and the workspace denies clippy warnings (-D warnings). Read directly from the live CI job logs: the clippy job and the windows-build-and-test → Windows build and test (debug) job both fail on this identical line. Per .github/workflows/ci.yml, build-and-test, test, pr-linux-binaries, and remote-ssh all declare needs: [changes, clippy, prek], so the clippy failure cascades into those being skipped rather than run — this single line is responsible for the large majority of this run's red/skipped checks. AGENTS.md §8/§12 make cargo clippy --workspace --all-targets -- -D warnings a required local gate and CI a required "drive to green" gate.
  Confidence 100 (read the exact failing line directly from two independent live job logs, confirmed the lint's syntactic match, confirmed the needs: graph in ci.yml directly). Category: mechanical.
  Fix: replace with .map_or_else(|| panic!(...), |(version, _)| version) as clippy itself suggests.

• The PR branch currently has real merge conflicts with the current tip of main, independent of GitHub's mergeable_state label — tests/e2e-cucumber/features/diagnose.feature, tests/e2e-cucumber/tests/e2e/diagnose_steps.rs.
  Verified directly rather than trusting the API's "mergeable_state": "dirty" field: ran git merge-tree between this PR's head and main's current tip (which has moved past this PR's merge-base) and got actual CONFLICT (content) markers in both files — almost certainly because another PR added different diagnose-NN scenarios to the same file regions in the meantime.
  Confidence 100 (ran the merge directly and read the conflict output). Category: mechanical.
  Fix: rebase onto current main and resolve the conflicting diagnose.feature/diagnose_steps.rs hunks.

Non-blocking

• The release-tag half of the version logic is still never exercised end-to-end against a real tagged build, only by unit tests — apps/rocm/build.rs, .github/workflows/release.yml (unchanged by this PR; re-confirmed fresh this round).
  release.yml is still the only tag-triggered workflow and still runs neither scripts/smoke_local.py nor the e2e-cucumber suite — only release_readiness.py, which has no version assertions. Every push/PR/merge_group CI run can only ever resolve a branch ref, never a real vX.Y.Z tag, so the tag-shape branch of format_cli_version/release_version is solidly unit-tested but still never produced and checked from an actually-built release artifact.
  Confidence 85 (confirmed release.yml has no diff in this PR and grepped it directly for version-related calls). Category: test-coverage.
  Fix: have release.yml run an equivalent of assert_version_string against the built release binary before publishing.

• Sibling binaries still derive their version from the frozen workspace package version, the same bug class this PR fixes for rocm — apps/rocmd/src/mcp.rs:96 (MCP serverInfo.version), crates/rocm-dash-daemon/src/lib.rs:21 (DAEMON_VERSION), crates/rocm-dash-tui/src/client.rs:160 (dash client string) — all still env!("CARGO_PKG_VERSION").
  Stated plainly as pre-existing and out of this PR's actual scope: unlike the prior review round, the current README/PR text no longer claims this fix covers "every CLI surface" unqualified — it's now scoped to rocm version's output and the diagnose/report cli_version field, both of which are genuinely fixed. No description/implementation mismatch here, just the same latent issue sitting one layer over in sibling binaries nobody has asked this PR to touch.
  Confidence 80 (grepped CARGO_PKG_VERSION across the repo and confirmed each remaining call site directly). Category: test-coverage/architectural (pattern-relevant, pre-existing).

• A handful of hardware-gated E2E jobs are failing for reasons unrelated to this diff — E2E tests (Strix Halo, Windows, current) (an existing runtime-09/runtime-resolve-canonical-release scenario asserts a canonical AMD package-index URL but got back a different configured mirror URL), E2E tests (Strix Halo, Ubuntu, release, current) ("No space left on device" on the runner), and the paired nightly job (cancelled as a consequence). None of the 15 changed files touch runtime/package-index resolution or disk usage, so these read as environmental/infra flakes rather than regressions from this change — consistent with a similar unrelated-failure note on the prior review round. E2E tests (MI300X, current) and (W7900, current) were still queued, not yet run, at review time.
  Confidence 75 (read the job logs directly; no code-path connection to this diff). Category: test-coverage (flake, needs rerun not a code fix).

Decisions for the author

None.

Positive signals

• report.rs's prepare_report now requires cli_version as a parameter (compiler-enforced), both production call site (main.rs) and the one test call site (report_delivery.rs) were updated, and the new validator/tests assert against real version shapes ("0.2.1", "0.2.1-rc.1", "0.0.0-dev", etc.), not a placeholder constant — a genuine fix, independently re-traced rather than taken on faith.
• build.rs's release_version parser now has explicit regression tests for every edge case the prior review round flagged as untested (v1.2.3.4, combined -rc.1+build.5, empty-suffix v1.2.3+), and the "unknown"-sentinel collision limitation the prior round asked to be documented is now spelled out directly in the module doc comment.
• The new diagnose-48 e2e scenario has real teeth: it independently re-derives the expected cli_version from a separately-run rocm version rather than re-implementing the parsing logic, so it would have caught the exact bug the prior review round found.
• cli_version_number()'s previously-fragile string-splitting (flagged last round) is gone, replaced by a dedicated ROCM_CLI_VERSION_NUMBER build-script export used directly by update_cmd.rs and the report path.

Deployment notes

• For every non-tag build (branches, main, local dev, standard CI), the CLI's reported leading version changes from the frozen 0.1.0 to 0.0.0-dev across rocm version, -V/--version, the MCP version tool, rocm update's cli: installed= field, and now also the rocm diagnose --report/--send payload. Anything downstream parsing/comparing a literal 0.1.0 CLI version string needs updating.
• rocmd's MCP server info, the dash daemon, and the dash-TUI client string are unaffected by this PR and keep reporting the frozen 0.1.0.
• The branch needs a rebase onto current main before it can merge (see Blocking).

What this covered

Read all 15 changed files completely at 82d3a7635001a98831ed9430e6d77b6139411ae2 (README.md, apps/rocm/build.rs, apps/rocm/src/chat.rs, apps/rocm/src/main.rs, apps/rocm/src/update_cmd.rs, apps/rocm/tests/build_metadata.rs, crates/rocm-core/src/report.rs, crates/rocm-core/src/report_delivery.rs, docs/testing.md, scripts/smoke_local.py, tests/e2e-cucumber/features/diagnose.feature, tests/e2e-cucumber/features/examine.feature, tests/e2e-cucumber/tests/e2e/diagnose_steps.rs, tests/e2e-cucumber/tests/e2e/examine_steps.rs, xtask/src/e2e_prewarm.rs), plus surrounding/caller context (every cli_version_string/cli_version_number/prepare_report call site, repo-wide CARGO_PKG_VERSION grep, AGENTS.md, .github/workflows/ci.yml's job dependency graph). Diffed against merge-base 3dec0eb2ad9b1b6169cdc9fdd437203582b13379. Work was split across three independent blind fan-out workers (crates/rocm-core report path; apps/rocm consumers main.rs/update_cmd.rs/chat.rs; docs/scripts/e2e), each given only file scope and no prior-review content, plus this driver's own pass over apps/rocm/build.rs/build_metadata.rs (run directly by the driver, not by an independent worker, after repeated subagent-concurrency-limit errors from unrelated concurrent sessions on this host — disclosed as not independent) and the CI/merge-conflict verification.

Read CI check-runs and job logs directly rather than assuming: confirmed the clippy failure's exact code and the needs: dependency graph explaining the cascaded skips; confirmed the merge-conflict finding by running git merge-tree directly rather than trusting the API's mergeable_state field alone.

Did not run: a separate "history"/git-blame pass or a "prior changes" pass beyond what's disclosed above. Did not execute cargo test/cargo clippy locally in a full workspace build; relied on the PR's own live CI results instead.

One disclosure for transparency: this review's driver necessarily read the prior review round's full text (including its finding wording) while performing the required claim-check against the current head commit before starting. The actual findings in this report were independently re-derived — the report.rs fix and the CI/clippy status were confirmed fresh by blind workers and direct CI/git inspection that were not shown the prior review's text — but this is disclosed rather than silently presented as a fully blind pass throughout. Not reconciled against any PR discussion beyond that.

---

🤖 Generated with Claude Code