feat(diagnose): refuse to describe hardware that is not publicly available

[volen-silo]

What this adds

rocm diagnose --report shows what this machine would contribute to a problem
report, and sends nothing. There is no transport yet, and there will be no
automatic one — a report leaves a machine only by its owner's own action. This
exists so that content can be read before any of it is shared.

Why the allowlist first

A report is destined for a public, indexed issue tracker. Everything else in the
reporting path is field-list discipline; the approved-architecture list is the
one real control — it is what stands between an unannounced product name and
something permanent and searchable. It is also the piece with no dependency on
transport, on the remaining open questions, or on any other work in flight, and
it is fully testable offline.

The question this list answers is ROCm support, not retail availability. Those
are different sets — some hardware sold today is simply not on the matrix —
and the predicate, the refusal text, and this description are all worded
against "the ROCm compatibility matrix" for that reason, not "publicly
available" or "on the market".

Ownership moves; enforcement does not

AMD's published ROCm compatibility matrix owns the list. The CLI does not keep a
rival copy — it ships a compiled snapshot, stamped with the release it was taken
from so staleness is a fact in the data rather than something someone has to
remember. That stamp (architecture_matrix) now rides in the report and the
refusal envelope alike, so a reader of either can tell which matrix revision
decided the verdict it is looking at.

The snapshot is compiled in rather than fetched, deliberately. A runtime lookup
would put the control somewhere an attacker can influence and would make the
catalog load from the network. Both are ruled out.

The matrix publishes as HTML, reStructuredText and PDF with no JSON, CSV or API,
so the snapshot is transcribed per ROCm release. That recurring work belongs with
the existing per-release catalog review rather than becoming a second process.

The gate is default-deny in four directions

Situation	Result
Architecture absent from the snapshot	No report
Architecture could not be read	No report — "we could not tell" is not permission
One unreleased GPU beside released ones	No report at all
WSL host	No report — this CLI does not probe the GPU on WSL yet, so there is nothing to confirm

The third is the non-obvious one: publishing the released half would leak the
other's existence by the shape of what was withheld. The fourth is not a
finding about the machine: it is a gap in this tool, worded that way so a
healthy WSL machine isn't told its GPU was unreadable when nothing looked.

A refusal exits 0. The command decided correctly and said why; a nonzero code
would send a caller looking for a fault that is not there. Anything scripting
this reads the outcome from --json, as rocm diagnose already asks callers to
do.

Two things found while building

The entry id is a leak vector. It is the only value in a report that comes
from a caller rather than from the machine, which makes it the one free-text hole
in a structure otherwise assembled field by field. It is now checked against the
catalog, so a forged or mistaken id cannot carry caller-supplied text onto a
public tracker.

The entry named must be an established cause. Several checkers open with a
nonzero score for a merely potentially relevant situation, so the match list is
rarely empty even on a healthy machine. Taking its head regardless would publish
a sub-threshold signal as though it were a cause, and every count built on those
reports would be wrong in a way nothing downstream could detect.

Tests

Eight assertions, each written and watched fail before the code existed, then
each checked by deliberately breaking the production code.

Two of them first passed vacuously against a stub that built an empty report —
an empty report carries no forbidden values either. Both now carry a paired
precondition that fails loudly if the report stops describing the machine.

Seven mutations; six were caught by exactly the intended test. One survived:
emitting the full OS build string instead of the major version passed everything,
because 22.04.3 is not a planted marker but a plausible real value. That gap is
now covered by its own assertion, and the mutation is caught.

• cargo test --workspace --all-targets — 0 failures
• cargo clippy --workspace --all-targets -- -D warnings — clean
• cargo fmt --all -- --check — clean
• cargo xtask e2e -- -n "diagnose-2" — 3 scenarios, 9 steps, 0 unexpected failures

Known limitation

Developed on a machine with no AMD GPU, so only the refusal path has run for
real. The prepared-report path is covered by unit tests but has never executed on
hardware. Both scenarios are written host-agnostically, so a GPU lane exercises
the other half.

Changed after review

• is_publicly_available is renamed is_rocm_supported, and every user-facing
  and doc-comment string that said "publicly available" / "already on the
  market" now says "on the ROCm compatibility matrix" instead, matching this
  description and the README. The gate itself is unchanged: same list, same
  logic, same refusal markers on the wire.
• APPROVED_ARCHITECTURES_SOURCE (the matrix-revision stamp) now reaches the
  data it stamps: it is a field on Report and on the refusal envelope, both
  covered by a test.
• The reader-side test that checks a refusal round-trips now builds its fixture
  by calling the same refusal_envelope function the CLI calls to print one,
  instead of a second, independent json! literal that could drift from it
  unnoticed.
• This description's table above was missing the WSL refusal, added in a later
  commit on this branch; it is listed now. The field list and the group key
  are both in the tree (see Tests above and the dedicated grouping test) — an
  earlier version of this section called them "deliberately not included",
  which no longer matched what ships.

[jussielo-amd]

Went through this against main and traced the two paths it exercises (also ran uname -v locally to sanity-check one of them against real output). Like the overall shape — compiled-in allowlist with the source stamped in, default-deny in all three directions, entry id checked against the catalog instead of trusted as free text. Found three things worth fixing before this merges, one of which affects the actual output:

1. os_major doesn't do what it's meant to (crates/rocm-core/src/report.rs:162)

os_major comes from examination.os_version, but that field holds raw uname -v output on Linux (e.g. #1 SMP PREEMPT_DYNAMIC Fri Jun 5 01:12:21 UTC 2026 — that's what uname -v gives on my machine right now) or cmd /C ver on Windows. Neither is a dotted release string. major_only() splits on ./- and takes the first token, so on a real machine this would emit a kernel-build fragment instead of a coarse OS version — which is the opposite of what the doc comment above it says it does.

The tests don't catch this because they set os_version = "22.04.3" by hand rather than going through probe_os, so this path has never run against real data — consistent with the "Known limitation" note about the prepared-report path only being unit-tested so far.

distro_version is already parsed from /etc/os-release's VERSION_ID and already gets the same kind of coarsening elsewhere (distro_clears_wsl_floor does split_once('.') on it). Swapping os_version → distro_version on that line should fix it. Might also be worth a test that goes through examine() for the Linux case so this doesn't slip through again.

2. "Doctor" shows up in what the user actually sees (apps/rocm/src/main.rs:2798)

The unreleased-hardware refusal message reads: "Doctor describes only hardware already on the market." Everywhere else this command is diagnose — help text, docs, the command name itself. "Doctor" only appears elsewhere in code comments as the old name. It also lands in the --json explanation field, so scripted consumers see it too. Suggest just dropping the word, e.g. "This describes only hardware already on the market."

3. Family comments in the allowlist are mismatched (crates/rocm-core/src/report.rs:46)

"gfx908", "gfx90a", "gfx942", "gfx950",  // RDNA 2
"gfx1030", // RDNA 3 / 3.5

gfx908/90a/942/950 are CDNA (Instinct MI100/200/300/350), not RDNA2. gfx1030 is RDNA2 (Navi 21), not RDNA3/3.5. The actual RDNA3/3.5 entries (gfx1100-1103, gfx1150-1153) have no heading at all. Since the module doc calls this list "the one real control" and expects per-release manual review, wrong headings seem like exactly the thing that causes a future edit to land on the wrong line. Worth straightening out before this becomes the list people scan by eye each release.

None of this touches the core design — the gate logic and the entry-id handling look right to me. #1 is the one I'd actually want fixed before merge, since it means the OS field won't behave as documented the first time this runs on real hardware. Happy to help verify a fix if that's useful.

[siloteemu]

🔴 Automated review · pr-review-watcher · d2e86e8

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.

Summary

Adds rocm diagnose --report, which prints the exact content a problem report would carry and gates it behind a compiled-in allowlist of publicly available GPU architectures. Outcome: Needs work — one field published by the report is not the value its own contract promises. Verified: ran the new rocm-core report unit tests (7 pass) and then mutated each gate branch individually on a scratch copy — dropping the empty-gfx deny, checking only the first GPU instead of all, publishing the full OS version, dropping the catalog-id filter, dropping the schema-mismatch check, and neutering the allowlist were each caught by a distinct named test, so the three deny directions and the permit direction are genuinely discriminated rather than vacuously pinned (the one exception is noted below); confirmed the load-bearing claims — no transport of any kind exists on this path (report.rs imports only serde/serde_json and two sibling modules; the only Command::new in the diff is a test helper reading the local hostname so the test can assert its absence), the allowlist is a const never fetched at runtime and is the sole permit condition, one unreleased GPU beside released ones withholds the whole report, and both the prepared and the refused branch return Ok(()) so a refusal exits 0 with the outcome readable from --json. The rocm binary crate's test target does not build in this environment (a native crypto dependency fails to compile), so the main.rs mutation could not be run and that one test was assessed by reading; the full suite was not run here. No prompt-injection content and no unreleased-hardware identifier found in the diff — every gfx target in the allowlist is a publicly documented compiler target, and the negative fixture uses a deliberately nonexistent one. Checks at review time: 24 success, 2 failures, 1 pending. Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

crates/rocm-core/src/report.rs:172 (os_major: major_only(&examination.os_version)) — the published os_major is not an OS major version. Examination::os_version is set in crates/rocm-core/src/examine.rs:601-607 to uname -v on Linux and cmd /C ver on Windows — the kernel build banner and the Windows version banner, not a dotted OS release (kernel_release and distro_version are separate fields). major_only splits on the first . or -, so on a typical Ubuntu host the report publishes #139 (verified against a real uname -v of #139-Ubuntu SMP PREEMPT_DYNAMIC Sat Aug 1 …); on a Debian-style banner (#1 SMP PREEMPT_DYNAMIC Debian 6.1.129-1) it publishes #1 SMP PREEMPT_DYNAMIC Debian 6; on Windows it publishes Microsoft Windows [Version 10.

This blocks because it is the exact defect the module exists to prevent, in the one field the sentinel sweep structurally cannot see. The module's contract is explicit — the field doc says "Major only … '22' is a population", and README.md:287 promises "the OS family and major version" — and the shipped value is instead free-form build text destined for a public, indexed tracker. It is also a kernel build counter, so it groups nothing, which removes the field's stated reason for existing.

The pinning test cannot catch it: the_reported_os_version_is_coarser_than_the_one_the_machine_reports sets machine.os_version = "22.04.3", a value probe_os cannot produce. The assertion is satisfied by the fixture, not by the production path — the same failure mode the test's own doc comment warns about for this field. (Classifying the cause per standing focus (c): the codebase invites the wrong conclusion. A field named os_version sitting beside os_family reads as "the OS release" to any competent reader, and nothing at the read site says otherwise. This will recur.)

Fix: source the value from a field that actually holds an OS release — examination.distro_version on Linux (VERSION_ID from os-release, e.g. 22.04 → 22), and extract the numeric build from the Windows banner — and, independently of the source, constrain major_only's output so a non-numeric result is never published (return an empty string or UNRECOGNISED rather than free text); the constraint alone closes the leak direction even if the source change is deferred. Add a unit test that drives major_only with the real raw shapes (#139-Ubuntu SMP …, #1 SMP PREEMPT_DYNAMIC Debian 6.1.129-1, Microsoft Windows [Version 10.0.22631.4460]) rather than an invented dotted string, and add a one-line comment at the read site naming what os_version actually holds — that is the cheap line that stops the next reader making the same inference.

Non-blocking

• crates/rocm-core/src/report.rs:48-56 — the family labels in the allowlist are attached to the wrong entries ("gfx950", // RDNA 2 on a CDNA part; "gfx1030", // RDNA 3 / 3.5 on an RDNA 2 part); every label is shifted by one because the formatter joined the lines. In the one hand-maintained list the PR calls the sole control, a mislabelled grouping actively misleads the per-release reviewer who adds or removes entries — #[rustfmt::skip] with each label on its own line fixes it.
• crates/rocm-core/src/report.rs:118 — is_publicly_available compares with ==, but gfx_target from the rocminfo path is stored verbatim (examine.rs:1199, validated only by starts_with("gfx")) and this codebase's own helper comment at examine.rs:1063 documents that the value can carry a :sramecc+:xnack- suffix; a released part arriving suffixed is refused and told it is "not publicly available". Fails closed, so not a leak, but it misreports released hardware — normalize on : before the lookup and pin it with a test.
• crates/rocm-core/src/report.rs:158 — no test covers the "no AMD GPU at all" deny direction: deleting amd.is_empty() || leaves all seven unit tests green (verified by mutation), and that is the branch the mock e2e lane actually reaches. A fixture with gpus: vec![] asserting Err(ArchitectureUnreadable) is a one-liner.
• crates/rocm-core/src/report.rs:35 — APPROVED_ARCHITECTURES_SOURCE's doc says the snapshot is stamped so a stale one "cannot be told apart from a current one", but nothing surfaces the stamp: it is not in Report, not printed by --report, and has no caller anywhere in the workspace. Same for read_report, ReadOutcome, APPROVED_ARCHITECTURES, is_publicly_available and UNRECOGNISED — exported from the crate root with no consumer outside this module and its tests. Carrying the stamp in the report would make the claim true and give the reader API a first user.
• apps/rocm/src/main.rs:2818 — the refusal branch prints only the explanation and never says nothing was sent, while the prepared branch does; the e2e step nothing_has_been_sent is conditional on the prepared branch, so on a lane without an approved GPU nothing asserts it at all. One sentence in the refusal text closes both.

[juhovainio]

This is a genuinely careful piece of work -- the default-deny gate, the field-by-field assembly, the sentinel-sweep test for leaks, the mutation testing on major_only -- and I'm not going to repeat what the automated review already found (the os_major/uname bug, the shifted gfx-family comments, the suffix-normalization gap, the missing no-GPU-at-all test, the unused staleness stamp, and the refusal-branch wording). All of that stands as written.

Two things I don't think that review caught:

docs/testing.md never got a --report entry. The PR's own "Known limitation" section says the prepared-report path has never run on real hardware -- that's exactly the case a manual-verification doc entry exists for, and it's missing alongside the new flag.

fix_offered is neither validated nor documented. prepare_report checks entry against the catalog and forces it to "unrecognised" when the id doesn't match, but fix_offered is a separate caller-supplied bool that never gets reconciled with that outcome -- so the function can return entry: "unrecognised" alongside fix_offered: true, which is a report that contradicts itself ("no established cause" but "a fix was offered"). Today's one call site keeps them in sync by construction, but prepare_report is pub and re-exported from the crate root, and its own doc comment says every field is here because it was agreed -- this one isn't enforced. Separately, the README's own list of what a report carries ("the matched entry, the GPU architecture, the OS family and major version, the CLI version") is five fields and doesn't mention fix_offered at all, so it's under-disclosed as well as unvalidated.

Three smaller things, not blocking on their own:

• --report silently conflicts with --distro (conflicts_with = "distro") -- a real restriction with no mention anywhere in the PR body.
• read_report/ReadOutcome is a full schema-compatibility reader, publicly exported, with no caller anywhere in this diff -- it's only exercised by its own tests. Might be worth cutting until something downstream actually reads a report.
• show_prepared_report hand-rolls its own println! sequence rather than going through the repo's shared ActionReport output convention -- may be a deliberate call given it's dumping a JSON blob rather than headline+details, but worth a line saying so.

[juhovainio]

New flag, but docs/testing.md (and possibly docs/manual-testing.md) don't get an entry for it. The PR body's own "Known limitation" section says the prepared-report path has never actually run on real hardware -- that's the case a manual-verification doc entry exists to cover, and AGENTS.md names docs/testing.md/docs/manual-testing.md explicitly alongside README/--help whenever a command's flags or observable behavior change.

[volen-silo]

Added a "Doctor Report Preflight" section to docs/testing.md covering both --report and --report --json, including the two refusal paths and the known limitation that the genuine-report path hasn't run on allowlisted hardware in CI. Left docs/manual-testing.md untouched, per your own round-4 note withdrawing that part of the request.

[juhovainio]

entry is validated against the catalog just above (forced to UNRECOGNISED when the id doesn't match), but fix_offered isn't reconciled with that outcome at all -- so this can construct a Report with entry: "unrecognised" and fix_offered: true at the same time, which is a self-contradictory published fact. The one call site today (established_entry in main.rs) happens to keep them in sync, but prepare_report is pub and re-exported, so nothing stops a future caller from breaking that. Worth deriving it post-validation, e.g. let fix_offered = fix_offered && entry != UNRECOGNISED; (adjusted for ordering). Also: README.md's own enumeration of report fields ("the matched entry, the GPU architecture, the OS family and major version, the CLI version") doesn't mention fix_offered -- it's under-disclosed as well as unvalidated.

[volen-silo]

Fixed, close to the wording you suggested: fix_offered is now derived post-validation as fix_offered && entry_recognised, so it can never disagree with whether entry matched the catalog, regardless of what a future caller of the public prepare_report passes in. Also added fix_offered (and schema) to README's field enumeration -- thanks for catching the under-disclosure.

[siloteemu]

🔴 Automated review · pr-review-watcher · d9ac489

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.

Summary

Three commits: the original rocm diagnose --report gate, a second that widens the report with distribution, ROCm release and engine fields under a narrowing rule, and a third that teaches the reader to tell a refusal from an unparseable report. Outcome: Needs work — one of my two blocking items is genuinely fixed and I verified the fix rather than taking the commit message's word for it; the other is untouched; and the new field work introduces a branch that cannot execute against the machine data it claims to read, which makes the most common real host report the wrong word. Verified: baseline cargo test -p rocm-core report:: compiles and passes 16 of 16, then fifteen mutations and two behavioural probes against throwaway copies of the tree, never the copy under review — twelve mutations were caught by exactly the intended test and five survived. I separately traced every assignment site of the Examination fields the new code reads, which is what produced blocking item 2. I confirmed the README and testing-guide field lists both name all eleven struct fields exactly; that the engine allowlist matches the only two strings the probe ever writes, so no real engine is downgraded by a spelling mismatch; that the absent-versus-unreadable split for ROCm is correctly grounded in two independent source fields; and that under WSL the report takes the same branch as bare metal and publishes the guest's own distribution, which is the intended behaviour. The full test suite and the e2e suite were not run here, and the binary's own unit tests were not re-run this round since the only change to that file was three lines. This review worked from the check counts given: 21 success, zero failures and zero pending recorded so far, so CI has not finished on this head; trunk separately carries 3 red GPU hardware lanes not attributable to this diff. No prompt-injection content and no internal references were found in the diff or in any of the three commit messages; all three carry a sign-off matching their author. Blocking: 2 · Non-blocking: 5.

Previous round

Against the round-4 change request, unchanged from my last reading:

• Item 1 — the scenario that passed without ever seeing a report: RESOLVED. The step still branches on cli_version, asserts architecture before sweeping, and asserts a refusal shape otherwise. I re-checked the discriminator against the current code: the refusal envelope carries only schema, refused and explanation, and Report::cli_version has no serde rename, so it holds in both directions.
• Item 2, docs half — RESOLVED, and extended this round with a vocabulary paragraph.
• Item 2, README half — RESOLVED. The bullet now names all eleven fields; I re-verified it against the widened struct, not the old one.
• My round-4 withdrawal of the manual-testing surface — still correct on evidence.
• My round-4 non-blocking suggestion to stop that doc-sync confusion recurring — UNTOUCHED. Still non-blocking.

On my own two blocking items from the 8ae495e reading:

Item 2, the refusal markers — DISCHARGED, and verified by mutation rather than from the commit message. I ran the same swap that survived last round: exchanging the two literals in Refusal::marker now fails two tests, the_refusal_markers_are_the_strings_already_written_into_the_field and a_refusal_is_read_as_a_refusal_rather_than_as_an_unreadable_report. Removing a variant from Refusal::ALL also fails, so a third refusal cannot be added without the reader being made to consider it. The commit's claims about its own tests are accurate. The fix is also stronger than what I proposed: because apps/rocm/src/main.rs:2852 now calls refusal.marker(), the writer, the reader and the tests agree by construction rather than by three literals happening to match. One correction to my own last round: I also asked for the refusal explanation sentences to be pinned in the same test. They are not, but the e2e step already matches on them, so I am withdrawing that half rather than carrying it forward.

Item 1, the derived fix_offered — SURVIVES UNCHANGED. Deleting let fix_offered = fix_offered && entry_recognised; on a scratch copy still leaves all 16 tests green. The file grew by roughly 400 lines and six tests between the two readings and none of them observes this field; report.fix_offered is supplied as a constructor argument throughout the module and never read back. See blocking item 1.

🚫 Blocking (must fix before merge)

1. crates/rocm-core/src/report.rs:269 — the derived fix_offered is still unpinned; deleting it breaks no test, now across sixteen.

Unchanged from my last round and re-measured at this head, so this is not a restatement from memory. The line's own doc comment states the reason it exists — "Derived here rather than trusted from the caller, because prepare_report is pub and re-exported, and nothing else enforces the two fields agree" — and the oldest commit message makes the same promise in prose, that "a caller could previously have shipped a report claiming a fix exists for a cause the catalog never established". Nothing enforces it. an_entry_id_the_catalog_does_not_know_is_never_published_verbatim at :919 still passes false for fix_offered on both of its calls, so the self-contradictory combination the guard exists to prevent is never constructed.

Fix, unchanged and still correct after re-checking it against the six new tests: add a third call to that test, prepare_report(&machine_of_sentinels("gfx1100"), Some("SENTINEL-FORGED-ENTRY"), true), asserting both entry == UNRECOGNISED and fix_offered == false. Adversarial pass over that fix: asserting fix_offered == false alone is satisfied by an implementation that forces the flag false unconditionally, and I confirmed no test in the file would refute that — not one asserts the flag is ever true. So it needs its paired premise in the same test, a call with a real catalog id and fix_offered: true asserting the flag survives. Both halves, or the hole moves rather than closes.

2. crates/rocm-core/src/report.rs:387-405 — engine_and_version decides on a value the machine never produces, so a host with no engine installed publishes the word the docs reserve for a different fact.

The function's doc comment states: "\"skipped\" is what examine.rs records when the probe did not run, and an empty value is what it leaves when the probe ran and found nothing." The second half is wrong. I traced every assignment to Examination::framework: there are exactly three, writing "skipped", "pytorch" and "llama-cpp", over a struct default of "unknown". No site ever writes an empty string. So the name.is_empty() branch returning (NONE, NONE) at :392-394 cannot execute against any examination this repository produces, and mutating it to return the other sentinel pair leaves all 16 tests green — it is both unreachable and unpinned.

The consequence is not cosmetic. On the --report path the probe always runs with FrameworkProbe::Auto, and when neither engine is found nothing writes to the field, so it stays "unknown", falls through the allowlist check and publishes engine: "unknown". docs/testing.md:1105-1107 defines that vocabulary for the reader: "none means the thing is absent, unknown means this build looked and could not tell". A machine with no PyTorch and no llama.cpp is the absent case, and it is the ordinary case for most hosts. It reports "looked and could not tell". This is the same defect the commit set out to fix one field over — its message says "an absent ROCm is kept distinct from one whose version could not be read, since examine collapses both into an empty string and a counter would otherwise report an install fault as an absence" — and rocm_release does get it right by reading two independent fields. The engine pair does not, and the field cannot be widened afterwards for reports already filed, which is the reason the commit gives for adding these fields now.

Fix: decide the engine vocabulary against the values the probe actually emits rather than an imagined empty string. Match the closed set explicitly — "pytorch" and "llama-cpp" to the name plus a narrowed version, "unknown" to NONE because on this path it means the probe ran and found nothing, "skipped" to UNKNOWN, anything else to UNKNOWN as the guard against a future probe — and delete the unreachable empty branch. Adversarial pass over that fix: mapping "unknown" to NONE is only right because --report always probes; a future caller handing in an unprobed examination would then report absence where nothing looked. So the mapping must not be silent about that — either the comment states that this function assumes a probed examination, or the distinction moves into examine.rs where the two cases are actually separable. Whichever is chosen, the doc comment here and the vocabulary paragraph in the testing guide have to be corrected together, because deleting the dead branch alone leaves both still describing behaviour the code does not have. And the fix needs tests: one pinning framework = "unknown" to the chosen engine value and one pinning "skipped", since no test currently exercises either.

Non-blocking

• crates/rocm-core/src/report.rs:340-356,414-429 — three more branches are unpinned: the empty-distro_id path returning UNKNOWN, major_minor's bare-major fallback for a non-numeric minor, and its wider delimiter set; mutating each left all 16 tests green, because every fixture supplies a non-empty distribution id and a version whose second component is all digits and dot-separated.
• crates/rocm-core/src/report.rs:203-208,481-485 — ReadOutcome::Refused hands back whatever string the refused field held; I fed the reader a fabricated marker and got it back verbatim, while the variant's doc comment says "The schema version gates this vocabulary: a reader that accepted the schema has accepted the set of markers that go with it". Refusal::ALL exists to enumerate that set and the reader does not consult it.
• README.md:290-291 and the middle commit message both say every version is cut back to a release, but cli_version at crates/rocm-core/src/report.rs:288 is the package version passed through with no narrowing at all; it is harmless today because that value comes from the repository rather than the machine, but the blanket claim is not true of all four version-bearing fields, and docs/testing.md:1104 says "Three of those fields" where distro, rocm, engine and engine_version are four.
• crates/rocm-core/src/report.rs:36,463 — APPROVED_ARCHITECTURES_SOURCE is still referenced by nothing at all, not even a test, and read_report/ReadOutcome still have no consumer anywhere in the workspace, so the provenance stamp the oldest commit message describes as making staleness "a fact in the data" is visible only to someone reading the source.
• apps/rocm/src/main.rs:161 — --report still carries conflicts_with = "distro" with no user-facing surface saying the two are exclusive; carried over from the previous round and unchanged.

Withdrawing this change request: the objection is discharged.

os_major is now derived by a dedicated helper that takes distro_version (VERSION_ID) on Linux and only the NT major on Windows. The kernel build banner no longer reaches the field. The added numeric guard closes the class rather than just this instance.

Verified by mutating both platform branches and the numeric guard individually on a scratch copy -- each mutation was killed by the test that names it.

Separate blocking findings, unrelated to this objection, are filed at the current head.

[siloteemu]

🔴 Automated review · pr-review-watcher · 3445503

Change request filed by automation. The findings below are the blocking half of the round published in the report comment on this pull request; the non-blocking notes stay there. This will be withdrawn once they are addressed — no human needs to clear it.

🚫 Blocking (must fix before merge)

• docs/testing.md, docs/manual-testing.md (untouched) and README.md:284-290 — the new flag is documented in one place out of the three the contributor rules name, and the one place it does reach under-describes what is published. AGENTS.md §5 is explicit: "when a command's flags, defaults, arguments, or observable behavior change, update README.md, its --help/doc comment, docs/testing.md, and docs/manual-testing.md in the same change — do not leave user-facing docs for a follow-up." README and the --help doc comment are updated; neither docs/testing.md (which does carry rocm diagnose --json and rocm diagnose --distro command lines) nor docs/manual-testing.md is in the diff. The same rule's second half — "the same behavior claim often repeats across README.md, --help doc comments, printed CLI output, and docs/*.md" — is what the README bullet trips over: it presents an exhaustive disclosure list, "The content is deliberately narrow (the matched entry, the GPU architecture, the OS family and major version, the CLI version)", but Report (crates/rocm-core/src/report.rs:77-91) also publishes fix_offered and schema. In a change whose entire thesis is exactly this much is disclosed and no more, an incomplete enumeration of the disclosed fields is the substantive half of this, not a formality. Fix: add --report to docs/testing.md and docs/manual-testing.md alongside the existing diagnose entries, and extend the README bullet to name fix_offered (and the schema version) so the list matches the struct. This independently confirms the other reviewer's docs thread.

• crates/rocm-core/src/report.rs:46-53 — the architecture-family comments in APPROVED_ARCHITECTURES are attached to the wrong lines, so the list reads as mislabelled. // CDNA and // RDNA 4 are leading comments, but the two in between are trailing: the CDNA line "gfx908", "gfx90a", "gfx942", "gfx950", // RDNA 2 labels four data-centre parts as RDNA 2; the next line "gfx1030", // RDNA 3 / 3.5 labels the one genuine RDNA 2 part as RDNA 3/3.5; and the line that actually holds RDNA 3 and RDNA 3.5 (gfx1100-gfx1103, gfx1150-gfx1153) is left with no label at all. Every family assignment in the repo contradicts the comments as written (crates/rocm-core/src/examine.rs:1030-1053, docs/ci-hardware-testing.md:36-40). This blocks because the file itself designates this list as "the only control preventing an unannounced product from being named in a public issue" and hands its upkeep to a recurring human review ("Reviewing it belongs to the per-release catalog review") — that review is the sole safeguard against the list going stale, and it is being handed a legend that points one line early. A reviewer adding a part under the label they read would put it in the wrong group or conclude a family is already covered when it is not. Fix: put each family comment on its own line immediately above the entries it introduces, as // CDNA and // RDNA 4 already are, and add #[rustfmt::skip] to the const so the layout survives formatting. (Checked against reformatting: without the skip attribute, re-wrapping can pull a leading comment back onto the preceding line, which is how the current state arose.)

• tests/e2e-cucumber/features/diagnose.feature:312-317 with tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:1200-1222 — scenario diagnose-22 passes without ever seeing a report. The scenario is titled "What a report would carry never identifies the machine", and the step sweeps the output for the user name, the host name and four path markers. On any lane whose hardware is not on the allowlist, --report --json emits only {"schema", "refused", "explanation"} — a fixed sentence that trivially contains none of those markers — and the step asserts nothing about a report. The feature's own comment says the refusal branch "is the case the mock lane actually has", and the PR body's Known limitation says the prepared-report path has never run on real hardware, so as it stands no lane has demonstrated the property this scenario names. There is no non-vacuity guard, which is exactly the discipline the unit tests in the same change apply everywhere else ("premise failed: ..."). This is the hard-blocker pattern: an assertion satisfied by something other than the thing under test. Fix: make the step branch explicitly on the outcome — when the answer carries "refused", assert the refusal shape and nothing more; otherwise assert "architecture" is present before sweeping for markers. That turns a silent vacuous pass into a stated one. Closing the gap properly needs the report branch exercised on a lane with allowlisted AMD hardware; if that lane is gated, AGENTS.md §3 requires the PR text to name it. (Checked against false failures: both branches assert only on content the command provably emits in that branch, so neither introduces flakiness — and note the fix makes the gap visible rather than closing it, which should be said plainly rather than left implied.)

Addressed

[siloteemu]

🔴 Automated review · pr-review-watcher · 8804ccc

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.

Two items from the previous round are still open. The family-label blocker is discharged and its fix verified load-bearing; these two are not.

1. A scenario that passes without ever seeing a report

tests/e2e-cucumber/features/diagnose.feature:313-317 with tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:1195-1217. The scenario is titled "What a report would carry never identifies the machine" and the step sweeps the output for the user name, the host name and four path markers. Running diagnose --report --json on a host with no AMD GPU emits {"explanation": "No AMD GPU architecture could be read here...", "refused": "architecture-unreadable", "schema": 1} and exits 0 — an envelope containing none of the swept markers, so every assertion is satisfied by output in which no report exists.

The sibling step nothing_has_been_sent handles the same asymmetry correctly: it branches on whether the output carries the report text and says in a comment why the other branch asserts nothing. The discipline is three lines away. Both new scenarios are also untagged, while this suite expresses hardware gating with @requires-gpu, so whether the report branch is ever reached is a property of whichever host runs the suite — and a run that never reaches it is indistinguishable from one that did. That makes the PR text's "a GPU lane exercises the other half" unverifiable from the change itself, which AGENTS.md §3 speaks to directly.

Fix: branch the step on the outcome — when the answer carries "refused", assert the refusal shape and nothing more; otherwise assert "architecture" is present before sweeping for markers. That converts a silent vacuous pass into a stated one. It makes the gap visible rather than closing it; closing it needs the report branch exercised on allowlisted hardware, named in the PR text.

2. The docs half, and an incomplete disclosure list

docs/testing.md is untouched. AGENTS.md §5 requires README, the --help doc comment, docs/testing.md and docs/manual-testing.md to move in the same change; the first two did. docs/testing.md already carries rocm diagnose --json and both rocm diagnose --distro forms, and the commit that introduced --distro updated it — so this departs from an established precedent, not only a written rule.

The substantive half is README.md:285-288. It presents an exhaustive list — "The content is deliberately narrow (the matched entry, the GPU architecture, the OS family and major version, the CLI version)" — but Report (crates/rocm-core/src/report.rs:87-101) also publishes fix_offered and schema. In a change whose entire thesis is exactly this much is disclosed and no more, an incomplete enumeration of the disclosed fields is the defect rather than a formality.

Fix: add --report to docs/testing.md alongside the existing diagnose entries, and extend the README bullet to name fix_offered and the schema version so the list matches the struct.

Correcting my own earlier round: I also named docs/manual-testing.md. That file has never carried diagnose coverage for any flag, --distro included, so omitting it here is consistent with how the repository actually treats it; that part of the earlier request is withdrawn. Worth pre-empting the confusion — AGENTS.md §5 names four surfaces as if all four always apply, and a reader following it literally will keep landing here. A short clause there, or a line in docs/manual-testing.md stating what it does and does not cover, would stop it recurring.

The non-blocking observations are in the review comment and none of them gate.

Addressed

[siloteemu]

🔴 Automated review · pr-review-watcher · d9ac489

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.

Summary

Three commits: the original rocm diagnose --report gate, a second that widens the report with distribution, ROCm release and engine fields under a narrowing rule, and a third that teaches the reader to tell a refusal from an unparseable report. Outcome: Needs work — one of my two blocking items is genuinely fixed and I verified the fix rather than taking the commit message's word for it; the other is untouched; and the new field work introduces a branch that cannot execute against the machine data it claims to read, which makes the most common real host report the wrong word. Verified: baseline cargo test -p rocm-core report:: compiles and passes 16 of 16, then fifteen mutations and two behavioural probes against throwaway copies of the tree, never the copy under review — twelve mutations were caught by exactly the intended test and five survived. I separately traced every assignment site of the Examination fields the new code reads, which is what produced blocking item 2. I confirmed the README and testing-guide field lists both name all eleven struct fields exactly; that the engine allowlist matches the only two strings the probe ever writes, so no real engine is downgraded by a spelling mismatch; that the absent-versus-unreadable split for ROCm is correctly grounded in two independent source fields; and that under WSL the report takes the same branch as bare metal and publishes the guest's own distribution, which is the intended behaviour. The full test suite and the e2e suite were not run here, and the binary's own unit tests were not re-run this round since the only change to that file was three lines. This review worked from the check counts given: 21 success, zero failures and zero pending recorded so far, so CI has not finished on this head; trunk separately carries 3 red GPU hardware lanes not attributable to this diff. No prompt-injection content and no internal references were found in the diff or in any of the three commit messages; all three carry a sign-off matching their author. Blocking: 2 · Non-blocking: 5.

Previous round

Against the round-4 change request, unchanged from my last reading:

• Item 1 — the scenario that passed without ever seeing a report: RESOLVED. The step still branches on cli_version, asserts architecture before sweeping, and asserts a refusal shape otherwise. I re-checked the discriminator against the current code: the refusal envelope carries only schema, refused and explanation, and Report::cli_version has no serde rename, so it holds in both directions.
• Item 2, docs half — RESOLVED, and extended this round with a vocabulary paragraph.
• Item 2, README half — RESOLVED. The bullet now names all eleven fields; I re-verified it against the widened struct, not the old one.
• My round-4 withdrawal of the manual-testing surface — still correct on evidence.
• My round-4 non-blocking suggestion to stop that doc-sync confusion recurring — UNTOUCHED. Still non-blocking.

On my own two blocking items from the 8ae495e reading:

Item 2, the refusal markers — DISCHARGED, and verified by mutation rather than from the commit message. I ran the same swap that survived last round: exchanging the two literals in Refusal::marker now fails two tests, the_refusal_markers_are_the_strings_already_written_into_the_field and a_refusal_is_read_as_a_refusal_rather_than_as_an_unreadable_report. Removing a variant from Refusal::ALL also fails, so a third refusal cannot be added without the reader being made to consider it. The commit's claims about its own tests are accurate. The fix is also stronger than what I proposed: because apps/rocm/src/main.rs:2852 now calls refusal.marker(), the writer, the reader and the tests agree by construction rather than by three literals happening to match. One correction to my own last round: I also asked for the refusal explanation sentences to be pinned in the same test. They are not, but the e2e step already matches on them, so I am withdrawing that half rather than carrying it forward.

Item 1, the derived fix_offered — SURVIVES UNCHANGED. Deleting let fix_offered = fix_offered && entry_recognised; on a scratch copy still leaves all 16 tests green. The file grew by roughly 400 lines and six tests between the two readings and none of them observes this field; report.fix_offered is supplied as a constructor argument throughout the module and never read back. See blocking item 1.

🚫 Blocking (must fix before merge)

1. crates/rocm-core/src/report.rs:269 — the derived fix_offered is still unpinned; deleting it breaks no test, now across sixteen.

Unchanged from my last round and re-measured at this head, so this is not a restatement from memory. The line's own doc comment states the reason it exists — "Derived here rather than trusted from the caller, because prepare_report is pub and re-exported, and nothing else enforces the two fields agree" — and the oldest commit message makes the same promise in prose, that "a caller could previously have shipped a report claiming a fix exists for a cause the catalog never established". Nothing enforces it. an_entry_id_the_catalog_does_not_know_is_never_published_verbatim at :919 still passes false for fix_offered on both of its calls, so the self-contradictory combination the guard exists to prevent is never constructed.

Fix, unchanged and still correct after re-checking it against the six new tests: add a third call to that test, prepare_report(&machine_of_sentinels("gfx1100"), Some("SENTINEL-FORGED-ENTRY"), true), asserting both entry == UNRECOGNISED and fix_offered == false. Adversarial pass over that fix: asserting fix_offered == false alone is satisfied by an implementation that forces the flag false unconditionally, and I confirmed no test in the file would refute that — not one asserts the flag is ever true. So it needs its paired premise in the same test, a call with a real catalog id and fix_offered: true asserting the flag survives. Both halves, or the hole moves rather than closes.

2. crates/rocm-core/src/report.rs:387-405 — engine_and_version decides on a value the machine never produces, so a host with no engine installed publishes the word the docs reserve for a different fact.

The function's doc comment states: "\"skipped\" is what examine.rs records when the probe did not run, and an empty value is what it leaves when the probe ran and found nothing." The second half is wrong. I traced every assignment to Examination::framework: there are exactly three, writing "skipped", "pytorch" and "llama-cpp", over a struct default of "unknown". No site ever writes an empty string. So the name.is_empty() branch returning (NONE, NONE) at :392-394 cannot execute against any examination this repository produces, and mutating it to return the other sentinel pair leaves all 16 tests green — it is both unreachable and unpinned.

The consequence is not cosmetic. On the --report path the probe always runs with FrameworkProbe::Auto, and when neither engine is found nothing writes to the field, so it stays "unknown", falls through the allowlist check and publishes engine: "unknown". docs/testing.md:1105-1107 defines that vocabulary for the reader: "none means the thing is absent, unknown means this build looked and could not tell". A machine with no PyTorch and no llama.cpp is the absent case, and it is the ordinary case for most hosts. It reports "looked and could not tell". This is the same defect the commit set out to fix one field over — its message says "an absent ROCm is kept distinct from one whose version could not be read, since examine collapses both into an empty string and a counter would otherwise report an install fault as an absence" — and rocm_release does get it right by reading two independent fields. The engine pair does not, and the field cannot be widened afterwards for reports already filed, which is the reason the commit gives for adding these fields now.

Fix: decide the engine vocabulary against the values the probe actually emits rather than an imagined empty string. Match the closed set explicitly — "pytorch" and "llama-cpp" to the name plus a narrowed version, "unknown" to NONE because on this path it means the probe ran and found nothing, "skipped" to UNKNOWN, anything else to UNKNOWN as the guard against a future probe — and delete the unreachable empty branch. Adversarial pass over that fix: mapping "unknown" to NONE is only right because --report always probes; a future caller handing in an unprobed examination would then report absence where nothing looked. So the mapping must not be silent about that — either the comment states that this function assumes a probed examination, or the distinction moves into examine.rs where the two cases are actually separable. Whichever is chosen, the doc comment here and the vocabulary paragraph in the testing guide have to be corrected together, because deleting the dead branch alone leaves both still describing behaviour the code does not have. And the fix needs tests: one pinning framework = "unknown" to the chosen engine value and one pinning "skipped", since no test currently exercises either.

Non-blocking

• crates/rocm-core/src/report.rs:340-356,414-429 — three more branches are unpinned: the empty-distro_id path returning UNKNOWN, major_minor's bare-major fallback for a non-numeric minor, and its wider delimiter set; mutating each left all 16 tests green, because every fixture supplies a non-empty distribution id and a version whose second component is all digits and dot-separated.
• crates/rocm-core/src/report.rs:203-208,481-485 — ReadOutcome::Refused hands back whatever string the refused field held; I fed the reader a fabricated marker and got it back verbatim, while the variant's doc comment says "The schema version gates this vocabulary: a reader that accepted the schema has accepted the set of markers that go with it". Refusal::ALL exists to enumerate that set and the reader does not consult it.
• README.md:290-291 and the middle commit message both say every version is cut back to a release, but cli_version at crates/rocm-core/src/report.rs:288 is the package version passed through with no narrowing at all; it is harmless today because that value comes from the repository rather than the machine, but the blanket claim is not true of all four version-bearing fields, and docs/testing.md:1104 says "Three of those fields" where distro, rocm, engine and engine_version are four.
• crates/rocm-core/src/report.rs:36,463 — APPROVED_ARCHITECTURES_SOURCE is still referenced by nothing at all, not even a test, and read_report/ReadOutcome still have no consumer anywhere in the workspace, so the provenance stamp the oldest commit message describes as making staleness "a fact in the data" is visible only to someone reading the source.
• apps/rocm/src/main.rs:161 — --report still carries conflicts_with = "distro" with no user-facing surface saying the two are exclusive; carried over from the previous round and unchanged.

[siloteemu]

🔴 Automated review · pr-review-watcher · d9ac489

Requesting changes on two items at this head. The full round, with the mutation evidence, is in the review comment posted alongside this.

First, the part that is settled: the refusal-marker defect raised at the previous head is discharged, and that was verified by re-running the same swap mutation rather than taken from the commit message. Exchanging the two marker literals now fails two named tests, and removing a variant from the enumerated set also fails, so a third refusal cannot be added without the reader being made to consider it. The fix is stronger than the one suggested: the writer, the reader and the tests now agree by construction rather than by three literals happening to match. One correction to our own previous round — we also asked for the refusal explanation sentences to be pinned in the same test; they are not, but the end-to-end step already matches on them, so that half is withdrawn rather than carried forward.

1. The derived guard on the fix-offered flag is still unpinned; deleting it breaks no test, now across sixteen. Re-measured at this head rather than restated. The line's own doc comment gives the reason it exists — that the entry point is public and re-exported and nothing else enforces the two fields agree — and the oldest commit message makes the same promise in prose, that a caller could previously have shipped a report claiming a fix exists for a cause the catalog never established. Nothing enforces it. The test that looks like coverage passes false for the flag on both of its calls, so the self-contradictory combination the guard exists to prevent is never constructed. The file grew by roughly four hundred lines and six tests between the two readings and none of them observes this field. Fix: add a third call to that test with a forged entry id and the flag set true, asserting the entry reads as unrecognised and the flag comes back false. Running the adversarial pass over that fix: asserting the flag is false on its own is satisfied by an implementation that forces it false unconditionally, and no test in the file would refute that, because not one asserts the flag is ever true — so it needs its paired premise in the same test, a call with a real catalog id and the flag set true, asserting it survives. Both halves, or the hole moves rather than closes.

2. The engine-and-version decision branches on a value the machine never produces, so the most common real host publishes the wrong word. The function's doc comment says an empty value is what the examination leaves when the probe ran and found nothing. That is not so: there are exactly three assignment sites for that field, writing "skipped" and the two engine names, over a struct default of "unknown", and no site ever writes an empty string. The empty-value branch is therefore unreachable, and mutating it to return the other sentinel pair leaves all sixteen tests green. The consequence is not cosmetic. On this path the probe always runs, and when neither engine is found nothing writes the field, so it keeps its default, falls through the allowlist and publishes "unknown". The testing guide defines that vocabulary for the reader: absent means the thing is not there, unknown means this build looked and could not tell. A machine with neither engine installed is the absent case, and it is the ordinary case for most hosts — it will report "looked and could not tell". This is the same defect the change set out to fix one field over, and the release field does get it right by reading two independent sources. Reports cannot be widened after filing, which is the reason the change gives for adding these fields now, so the wrong word is permanent in the corpus.

Fix: decide the vocabulary against the values the probe actually emits — the two engine names to the name plus a narrowed version, the default to absent because on this path it means the probe ran and found nothing, the skipped sentinel to unknown, anything else to unknown as the guard against a future probe — and delete the unreachable branch. Adversarial pass over that fix: mapping the default to absent is only right because this path always probes, so a future caller handing in an unprobed examination would then report absence where nothing looked; either the comment states that assumption or the distinction moves to where the two cases are actually separable. Either way the doc comment and the testing guide's vocabulary paragraph have to be corrected together, because deleting the dead branch alone leaves both describing behaviour the code does not have. Two tests are needed with it, pinning each of the two sentinel inputs, since no test exercises either today.

[volen-silo]

Addressed both blocking items and four of the five non-blocking items in db0d18e.

Blocking 1 (fix_offered unpinned): added the paired assertion exactly as proposed — a real catalog id with fix_offered: true asserting the flag survives, and the forged-id call asserting it's overruled to false. Both premises now live in an_entry_id_the_catalog_does_not_know_is_never_published_verbatim.

Blocking 2 (engine_and_version dead branch): confirmed by re-tracing Examination::framework's assignment sites — struct default "unknown", plus "skipped", "pytorch", "llama-cpp"; no site ever writes "". Replaced the is_empty() check with a check against the actual default (NONE for "probe ran, found nothing"; falls through the allowlist to UNKNOWN for "skipped" and anything unapproved). Added two tests pinning both sentinels. Doc comment and docs/testing.md's vocabulary paragraph updated together, and the doc now states this mapping assumes the caller always ran a probe.

Non-blocking, fixed:

• Empty distro_id and major_minor's bare-major/dash-delimited fallbacks are now pinned by two new tests.
• read_report now checks the refused marker against Refusal::ALL instead of accepting it verbatim.
• README/testing.md corrected: cli_version isn't cut back to a release (it's the CLI's own version, not read off the machine), and the vocabulary paragraph now covers four fields, not three.
• --report's conflicts_with = "distro" now has a doc comment explaining why.

Non-blocking, left alone: APPROVED_ARCHITECTURES_SOURCE and read_report/ReadOutcome still have no workspace consumer. This is expected incompleteness, not a defect — the reading side isn't wired into any CLI command yet.

Full bar re-verified at this head: cargo fmt --all -- --check, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace --all-targets all clean, and every new/changed test was mutation-tested (broken, confirmed red, reverted, confirmed green) before commit.

Addressed — see PR comment for details.

[jussielo-amd]

Automated review

Reviewed the diff against origin/main. One confirmed defect blocks approval; the rest are lower-confidence hardening / test-quality notes left inline for consideration, not blockers.

Blocking

• rocm_release() reports "none" on every Windows machine, regardless of actual install — see inline comment on report.rs.

Non-blocking (inline)

• Report's fields are all pub, so prepare_report's allowlist/scrubbing gate is bypassable by direct construction.
• is_publicly_available() matching is case-sensitive, unlike sibling functions in the same file.
• fix_offered only checks that the catalog id exists, not that it carries a fix for the current diagnosis.
• major_minor() (report.rs) and major_version() (diagnose.rs) are two version-truncation helpers with different semantics for the same input class.
• Three e2e test-quality gaps in diagnose_steps.rs: a silent skip on empty/short user or host name, a hardcoded path-prefix allowlist, and an unanchored substring match used as the report/refusal discriminator.
• No unit test distinguishes "zero AMD GPUs" from "AMD GPU with unreadable target" — both hit the same refusal today.

Happy to re-review once the Windows rocm_release() gap is addressed.

[jussielo-amd]

Confirmed bug — blocking. rocm_release() only reads examination.rocm_path / rocm_version, which examine.rs's probe_with_interpreter() populates exclusively on the Linux/WSL branches. Native Windows populates hip_sdk_path / hip_sdk_version instead (via probe_hip_sdk_windows()), and this function never reads those.

On a Windows machine with a fully installed HIP SDK, rocm diagnose --report --json will therefore always emit "rocm": "none" — falsely telling a public tracker that no ROCm/HIP install exists. os_major() and distro() elsewhere in this file correctly branch on os_family for their platform-specific source; this is the one field-builder that's missing that branch.

[volen-silo]

Fixed in 0d36c9e. You were right, and the diagnosis was exact.

rocm_release() now sources per platform, the same way os_major already does and for the same reason: no single field on the examination holds "the installed ROCm" on both. Windows reads hip_sdk_path / hip_sdk_version, everything else keeps the existing pair. Within each platform the path still decides absence and the version decides readability, so an install whose version could not be read stays distinguishable from one that is not there.

The regression test builds its fixture the way probe's Windows branch actually leaves an examination, with the HIP pair filled and the Linux pair empty, rather than a shape I invented. Three mutations confirm it can fail: reverting to the Linux-only pair, reading the HIP pair on every platform, and letting an unreadable version collapse into an absence. The first of those is the bug as shipped, so the test would have caught it.

Worth recording that this is the third field in this file sourced from a platform that does not fill it, after the OS release and the engine name. The commit message carries the question each future field needs asked of it: which branch of probe writes this, and what does it hold on the branches that do not. That is the real defect; the per-field fixes are symptoms of it.

Your other notes are read and none is dismissed. I have left them for a separate pass rather than folding them into this round, since mixing a correctness fix with hardening is how a PR here ends up churning. The two I consider most substantive are the unanchored cli_version discriminator and the leak sweep that silently no-ops on a short or empty username, both of which are tests that can pass having checked nothing.

[jussielo-amd]

Hardening note. Every field on Report is pub, and the type derives Serialize/Deserialize and is re-exported from the crate. Nothing currently stops a future caller from constructing a Report literal directly, bypassing prepare_report's allowlist/scrubbing gate. Worth considering a private field (or #[non_exhaustive] + constructor) so prepare_report stays the only path to a Report.

[jussielo-amd]

Consistency nit. This match is case-sensitive, while distro() and engine_and_version() in this same file lowercase before comparing. Today's upstream sources happen to keep this safe (rocminfo's case-sensitive gfx prefix, classify_amd_marketing_name's lowercase literals), but a future GPU-detection path producing a mixed-case gfx_target would silently refuse genuinely-supported hardware. Consider normalizing case here too.

[jussielo-amd]

Latent gap, flagged in your own doc comment above. fix_offered is derived from whether entry is any recognised catalog id (is_catalog_id only checks existence via find_recipe), not whether that id actually carries a fix relevant to the current diagnosis. Today's only caller keeps the two consistent, but since prepare_report is pub and re-exported specifically for other callers, a future caller passing fix_offered: true alongside an unrelated-but-valid catalog id would publish a report that falsely claims a fix was offered.

[jussielo-amd]

Duplication note. diagnose.rs already has major_version() (line 962) for version truncation, with different semantics (search-anywhere) than this major_minor() (strict leading-token split-and-check). Two helpers doing overlapping jobs invites divergence on the same input class, and a third truncation site added later is likely to duplicate again rather than reuse either. Consider consolidating.

[jussielo-amd]

Test-coverage gap. This test only covers a single AMD GPU with an empty gfx_target; there's no test for a machine with zero AMD GPUs at all. Both cases currently hit the same Refusal::ArchitectureUnreadable branch and the same user-facing message ("No AMD GPU architecture could be read here"), which is technically imprecise for the zero-GPU case and reads as if a GPU exists but was unreadable. Not urgent, but a future attempt to split these into distinct refusals could regress the all-non-AMD path silently since nothing exercises it as a first-class case today.

[jussielo-amd]

Test-quality gap. The report-vs-refusal discriminator is an unanchored substring match on "cli_version" against combined stdout+stderr, with no JSON parsing. If any unrelated stderr text ever contains the literal substring cli_version (e.g. a diagnostic like "failed to read cli_version cache"), this step would treat a refusal envelope as a genuine report and assert on "architecture" — masking a real refusal-vs-report bug behind an unrelated string collision. Consider parsing the JSON and branching on its shape instead.

[jussielo-amd]

Test-quality gap. This leak sweep silently no-ops whenever $USER/$USERNAME is empty or ≤2 characters — common on minimal CI containers. On such a lane, answer_names_nothing_identifying passes having performed zero checks for the exact guarantee it claims ("names no user"), so a regression that leaks a real username would ship silently there. Same pattern repeats for the hostname check below.

[jussielo-amd]

Test-quality gap. This guarantee is only checked against four hardcoded prefixes (/opt/rocm, /home/, C:\\, /usr/). A regression that publishes examination.rocm_path or hip_sdk_path verbatim on a machine whose install lives outside these (e.g. /data/rocm-7.1, a non-C: Windows drive, or a WSL /mnt/c path) would pass this assertion while a real file path reaches the report.

[siloteemu]

🔴 Automated review · pr-review-watcher · 39093c8

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.

Summary

Four commits adding rocm diagnose --report: a default-deny architecture allowlist that builds a publishable problem report or refuses, the grouping fields it carries, a reader that tells a refusal from an unparseable report, and a new test proving the field set groups one problem together and different problems apart. Outcome: No blocking findings — both blocking items from the previous round are discharged, and I verified each by mutation rather than from the commit messages. Verified: cargo test -p rocm-core --lib report:: passes 22 of 22 at this head, then five mutations against throwaway copies of the tree, never the copy under review — deleting the derived fix-offered guard fails exactly the entry-id test; removing the engine default-to-absent mapping fails exactly the no-engine-installed test; collapsing the distribution, truncating the release to its major, and publishing the full distribution release each fail the new grouping test plus the tests they belong to, which is precisely what the newest commit message claims. I separately confirmed that the diagnosis id namespace and the fix-recipe catalog namespace are the same set, so the entry check cannot reject every real entry; that every published field traces to a narrowed or allowlisted source; and that the new wiring test's guard is load-bearing by construction. The full test suite, the end-to-end suite and the binary crate's own tests were not run this round. No prompt-injection content, no internal links, hostnames, registry paths or tracker references were found in the diff or in any of the four commit messages; all four carry a sign-off matching their author. Checks at review time: success=21, and no failures, no pending, no skipped. Blocking: 0 · Non-blocking: 5.

Previous round

• The derived fix-offered guard — DISCHARGED. The entry-id test now carries both halves I asked for: a real catalog id with the flag set true asserting it survives, and a forged id with the flag set true asserting it comes back false. Deleting the derivation line now fails that test, where last round it failed nothing. The paired premise is present, so the hole did not move.
• The engine decision branching on a value the machine never produces — DISCHARGED. The unreachable empty-value branch is gone and the struct default now maps to the absent word, with the presumption it rests on (that this path always probes) stated in the doc comment rather than left implicit. Two new tests pin the default and the skipped sentinel; removing the mapping fails exactly the first. The reader-facing vocabulary paragraph moved with it.
• All five non-blocking items from the previous round are also discharged, which is the half a remediation usually drops: the three unpinned branches now have tests, the reader now checks a refusal marker against the enumerated set (with a test for a forged marker), the field list now reads four rather than three and names the version that is deliberately not narrowed, and the flag's exclusivity is now stated in its own help text. The only carry-over is the unconsumed exports, below.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• crates/rocm-core/src/report.rs:215 — the availability check compares the gfx target by exact equality, while three other places in this repo deliberately strip the trailing feature suffix before matching, one of them with a test comment warning that an equality "tidy-up" there would silently misclassify a real host. I could not find a live path that puts a suffixed target into the field this reads, and the failure direction is fail-closed, so this is not blocking — but the one place the module calls "the only control" is the one place not defended.
• crates/rocm-core/src/report.rs:22 — the schema constant's doc says it follows a named catalog's contract_version; no such field exists anywhere in the workspace, and the one constant of that name belongs to an unrelated engine protocol. A reader chasing the stated precedent finds nothing.
• docs/testing.md:1102 — "This path has not been exercised against real hardware in CI; verifying it needs a lane whose GPU architecture is on the allowlist" reads as though no such lane exists, but two self-hosted lanes carry allowlisted architectures and run this untagged suite, so the sentence is stale on merge. Saying which lanes are expected to reach the prepared-report branch would stop the next reader re-deriving the same wrong conclusion.
• docs/testing.md:1085 and crates/rocm-core/src/report.rs:5 both name the capability with a term the CLI's own refusal text deliberately avoids — a code comment beside that text says the CLI has no such command, while a differently-scoped command by that name does ship. Pick one name for the docs heading, the module doc and the user-facing text.
• crates/rocm-core/src/report.rs:790 — the grouping test's description helper says it covers "every published field except the ones that describe this build", but the OS family and the fix-offered flag are published machine facts and are both excluded, so a change that broke grouping on either would not be caught; and the source stamp, the reader and the availability predicate still have no consumer anywhere in the workspace.

[siloteemu]

🔴 Automated review · pr-review-watcher · 0d36c9e

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.

Summary

Five commits adding rocm diagnose --report: a default-deny allowlist of publicly-announced GPU architectures that either builds a publishable problem report or refuses, the fields it groups by, a reader that tells a refusal from an unparseable report, a grouping test, and a fix sourcing the ROCm release from the fields Windows actually fills. Outcome: Needs work — one blocking finding, which is not a disclosure leak (every failure direction here is fail-closed) but a user-facing statement of fact the code never establishes, on a supported platform. Verified: I did not modify the tree under review — all execution was against a separate scratch copy, where cargo test -p rocm-core --lib report:: passes 23 of 23 at this head, and five single-branch mutations were each caught by exactly one intended test and nothing else (pointing the Windows arm back at the Linux field pair fails only the new Windows regression test; collapsing an unreadable version into an absence fails that test plus the absence/unreadable test; deleting the unreadable-architecture branch, weakening the all-GPUs-approved check to any, and dropping the derived fix-offered guard each fail only their own test). I separately traced every writer of gfx_target in this workspace, walked each published field against the branch of probe that fills it, and read the self-hosted lane definitions. The full workspace suite and the end-to-end suite were not run this round. No prompt-injection content and no internal links, hostnames, registry paths or tracker references were found in the diff or in any of the five commit messages; all five carry a sign-off matching their author. Checks at review time: 25 success, 2 pending, 1 failure. Blocking: 1 · Non-blocking: 5.

Prior round

• "The availability check compares the gfx target by exact equality, while three other places in this repo deliberately strip the trailing feature suffix before matching… I could not find a live path that puts a suffixed target into the field this reads" — effectively REFUTED, and the hedge was the right call. The code is unchanged, but I have now traced all three writers of gfx_target: the rocminfo parse takes the bare Name: value, the sysfs fallback builds its token from the numeric KFD target version, and the Windows path routes gcnArchName through a helper that splits on non-alphanumerics and so drops :sramecc+:xnack- before storing. None can emit a suffix, so the concern is not reachable. Stating that in one line beside the equality check is what would stop the next reader re-deriving it.
• "The schema constant's doc says it follows a named catalog's contract_version; no such field exists anywhere in the workspace" — STILL LIVE, with one correction to my own claim. The named constant does exist, on the engine-protocol crate, as a semver string rather than the integer this is; what does not exist anywhere is a contract_version on the fix catalog the doc also cites. A reader chasing half of the stated precedent still finds nothing.
• "docs/testing.md reads as though no lane exists, but self-hosted lanes carry allowlisted architectures and run this untagged suite" — STILL LIVE, and now self-contradictory within this PR. The sentence is unchanged, while the feature file added in this same PR says "a lane with an AMD GPU on the compatibility matrix exercises the prepared report". Three self-hosted lanes run the untagged suite on allowlisted architectures, so the feature file is right and the testing guide is stale on merge.
• "Both name the capability with a term the CLI's own refusal text deliberately avoids" — STILL LIVE. The module doc, the schema doc, the refusal enum doc and the testing-guide heading all still use it, and the code comment beside the refusal text still says the CLI has no such command while a differently-scoped command by that name does ship.
• "The grouping test's description helper says it covers every published field except the ones that describe this build… and the source stamp, the reader and the availability predicate still have no consumer" — STILL LIVE. The helper still omits the OS family and the fix-offered flag, both of which are machine facts; the source stamp, the reader, its outcome enum and the availability predicate are still re-exported with no caller anywhere.

🚫 Blocking (must fix before merge)

• crates/rocm-core/src/report.rs:239-250 with apps/rocm/src/main.rs:2852 — on WSL the report path refuses for a reason the code never checked, and says so as a fact about the machine. prepare_report decides entirely from examination.gpus; the WSL arm of Examination::probe runs the WSL, ROCm-install, env, container, framework and shared-memory probes and no GPU probe at all, so gpus is always empty there. Every WSL host therefore gets ArchitectureUnreadable, and the CLI prints "No AMD GPU architecture could be read here, so nothing confirms this hardware is publicly available." Nothing read anything. The repo already names this exact trap in the doc comment on the WSL facts' rocm_sees_gpu field: "the probes that populate it are skipped here, so it is always false on WSL and reads as 'no GPU' on a perfectly healthy machine." Three things follow. First, this is the same deficiency the --report help text already blocks for the remote WSL case — "a WSL distribution reached remotely is not fully examined… so it cannot back the disclosure guard's architecture check" — applied to one of two identical sites: the local WSL run has the identical gap and is neither blocked nor explained. Second, the WSL arm added to rocm_release at crates/rocm-core/src/report.rs:382, and the doc above it naming WSL as a supported source, are unreachable through the CLI. Third, the feature file's claim that "a lane without [an allowlisted AMD GPU] exercises the refusal" is wrong for the WSL lane, which has one — so that lane's green scenario records the disclosure guard as firing correctly when it fired for an unrelated structural reason. This is not a leak; the direction is fail-closed throughout. What merging leaves behind is a false user-facing statement on a documented supported platform, in a command whose whole purpose is to be exact about what it can and cannot say. Minimum fix: give the refusal a third variant for "this platform was never asked", return it when the examination is a WSL one, and say so in the README and testing guide alongside the two refusals already listed — doing it now is cheaper than later, because the marker vocabulary is schema-gated and nothing has been filed yet. I checked that fix for the same failure mode: adding a variant forces the reader's enumerated set and the marker tests to be updated, which is exactly what those tests were built to force, so it cannot be added silently. The deeper fix — populating gpus on WSL, where the architecture is in fact reachable, since the WSL probe already asks whether ROCm enumerates a gfx device — is larger and belongs in its own change.

Non-blocking

• crates/rocm-core/src/report.rs:40 — "no marketing-name mapping sits between the machine and this decision" is false on Windows: the Windows GPU probe derives gfx_target from the marketing name by substring, and the HIP SDK probe only fills GPUs whose target is still empty, so on a Windows lane the published architecture is the guess and never the SDK's own gcnArchName.
• tests/e2e-cucumber/tests/e2e/diagnose_steps.rs — the "nothing has been sent" step has an empty else-branch, so it asserts nothing on exactly the lanes that reach a refusal. Its sibling step in the neighbouring scenario was given a real assertion in both arms; this one was not, and the commit message cites it as the model the sibling was fixed to match.
• docs/testing.md — the two refusals it enumerates omit the largest real-world class (any WSL host), and its CI-coverage sentence now contradicts the feature file added in the same PR.
• crates/rocm-core/src/report.rs:5, :22, :114 and the testing-guide heading — the capability name carried over from planning is still used in four places the user-facing text deliberately avoids, beside a comment asserting no such command exists when one does.
• crates/rocm-core/src/report.rs:793 — the grouping helper's description still claims to cover every machine-describing field while omitting two of them; and the reader, its outcome enum, the availability predicate and the snapshot's source stamp are exported with no consumer, so nothing outside this crate yet depends on the half of the contract they define.

One red check is reported. I cannot attribute it to a specific defect in this diff. I note, and am inferring rather than confirming, that this PR adds two untagged scenarios which will run on the GPU lanes for the first time, so a failing end-to-end lane here plausibly is this PR's own rather than the trunk's usual intermittent red.

[siloteemu]

🔴 Automated review · pr-review-watcher · 0d36c9e

Requesting changes on one item. The full round, including a walk of our earlier advisory round and five non-blocking notes, is in the comment posted alongside this.

On WSL the report path refuses for a reason the code never checked, and states it as a fact about the machine (crates/rocm-core/src/report.rs:239-250, reached from apps/rocm/src/main.rs:2852).

prepare_report decides entirely from the examination's GPU list. The WSL arm of the examination runs the WSL, install, environment, container, framework and shared-memory probes and no GPU probe at all, so that list is always empty there. Every WSL host therefore takes the unreadable-architecture path, and the CLI prints that no GPU architecture could be read here, so nothing confirms this hardware is publicly available. Nothing read anything.

To be clear about what this is and is not: it is not a disclosure leak. Every failure direction here is fail-closed, and the guard errs toward refusing. What merging leaves behind is a false user-facing statement of fact, on a documented supported platform, in a command whose entire purpose is to be exact about what it can and cannot say.

Three things make it worth fixing before merge rather than after:

• The repository already names this exact trap. The doc comment on the WSL facts' GPU-visibility field says the probes that populate it are skipped there, so it is always false on WSL and reads as "no GPU" on a perfectly healthy machine. The same footgun is stepped on again one layer up.
• It is the fix applied to one of two identical sites. The --report help text already blocks and explains the remote WSL case, saying such a distribution is not fully examined and so cannot back the architecture check. The local WSL run has the identical gap and is neither blocked nor explained.
• The feature file added in this PR claims that a lane without an allowlisted GPU exercises the refusal. That is wrong for the WSL lane, which has one — so that lane's green scenario records the guard as firing correctly when it fired for an unrelated structural reason.

A consequence worth noting: the WSL arm added to the release-version lookup, and the doc above it naming WSL as a supported source, are unreachable through the CLI as written.

Suggested fix. Give the refusal a third variant meaning "this platform was never asked", return it when the examination is a WSL one, and list it in the README and testing guide beside the two refusals already documented. We put the same adversarial pass over that remedy as over the finding: adding a variant forces the reader's enumerated set and the marker tests to be updated, which is exactly what those tests exist to force, so it cannot land silently. The deeper fix — populating the GPU list on WSL, where the architecture is in fact reachable, since the WSL probe already asks whether ROCm enumerates a device — is larger and belongs in its own change.

Doing this now is also cheaper than later: the marker vocabulary is schema-gated, and nothing has been filed against it yet.

[volen-silo]

Fixed in 71304ef. The finding is correct and I verified it in the source rather than taking it on trust: examine's WSL arm runs the WSL, install, environment, container, framework and shared-memory probes and then returns, so no GPU probe runs and the list is empty there whatever the hardware is.

Your framing of it was the useful part. It is fail-closed and not a disclosure risk, and it is still a false statement of fact on a supported platform, in the one command whose purpose is being exact about what it can say. Those are different failures and only the second one was happening.

Implemented as suggested: a third refusal, platform-not-probed, returned before the architecture check. The message says the CLI does not inspect the GPU on WSL yet and that this is a gap in the tool rather than a problem with the machine. The deeper fix, probing on WSL where the architecture is in fact reachable, belongs in its own change, as you said.

Your adversarial pass was right about the remedy too. Adding the variant broke the reader's exhaustive match and the marker-count assertion on the first build, before I had touched either. It could not have landed silently.

The regression test gives the WSL fixture a perfectly good approved GPU on purpose. A machine with no GPU would reach the right answer for the wrong reason and pass against code that never reads is_wsl. Mutation-checked in both directions: removing the check fails it, and refusing every machine as not-probed fails it too.

Also corrected the scenario comment you flagged. It claimed any lane without an allowlisted GPU exercises the refusal, which is wrong for the WSL lane and recorded the guard firing correctly when it fired for an unrelated structural reason. The README and the testing guide now name all three refusals and say why they are not interchangeable.

Worth recording that this is the fourth field in this file sourced from a platform that does not fill it, after the OS release, the engine name and the ROCm release. The commit message carries the generalisation: the question a new field needs is not what it is called, but which branch of probe writes it and what it holds on the branches that do not.

Addressed in 71304ef: third refusal variant added, verified in source, mutation-checked.

[juhovainio]

I reviewed this PR and found a few things worth fixing before merge, left inline below.

The main one is a naming-versus-meaning mismatch that makes the CLI state something untrue. APPROVED_ARCHITECTURES is transcribed from the ROCm compatibility matrix, but it is consumed by a predicate called is_publicly_available and surfaced to users as "hardware that is not publicly available" and "already on the market". Plenty of AMD silicon is sold at retail and absent from that matrix, so those users get a refusal that is factually wrong about their machine. The README already describes the gate correctly as the compatibility matrix, which is the wording the user-facing strings should match.

Smaller ones: the staleness stamp never reaches the data it stamps, one test's verification claim does not hold, and there is a fourth refusal the description's table does not list.

A few things I checked and did not flag: the rustfmt::skip usage is justified by the comments it preserves, the schema version bump is correct, and the refusal markers are consistent between the enum and the JSON surface.

[volen-silo]

Addressed all four items from the review at 71304ef (head is now 0097a67):

• Naming/wording (main item): is_publicly_available renamed is_rocm_supported; every user-facing refusal string and doc comment now says "on the ROCm compatibility matrix" instead of "publicly available" / "already on the market", matching the README. No logic change — same list, same check, same wire markers.
• Staleness stamp: APPROVED_ARCHITECTURES_SOURCE now reaches the data it stamps via a new architecture_matrix field on Report and on the refusal envelope (both tested).
• Test verification claim: the reader-side round-trip test now builds its fixture by calling the same refusal_envelope() function the CLI calls to print one, instead of an independent json! literal that could drift from it.
• Fourth refusal / stale description: the PR description's table now lists all four refusal directions including platform-not-probed (WSL), and the stale "deliberately not included" section is replaced — the full field list and the grouping fields are both in the tree, added in a later commit on this branch.

Verified: cargo fmt --all -- --check, cargo clippy --workspace --all-targets -- -D warnings, cargo clippy -p e2e-cucumber --test e2e -- -D warnings, and cargo test --workspace --all-targets all clean at the new head. Replied inline on each of the five review comments and resolved those threads. Dismissing the review since all four items it raised are handled.

Addressed: naming/wording fixed to match the README's compatibility-matrix framing, the staleness stamp now reaches architecture_matrix on Report and the refusal envelope, the reader-test now calls the same refusal_envelope() the writer calls, and the description's table/field-list text is reconciled with what ships. See PR comment for details.