From 2f0132a2bec40caa5f52c5c82c4d67b38b1d5737 Mon Sep 17 00:00:00 2001 From: Eugene Volen Date: Mon, 28 Sep 2026 08:56:29 +0000 Subject: [PATCH 1/5] feat(diagnose): refuse to describe hardware that is not publicly available `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, so this exists to let a machine's owner read the exact content before any of it is shared. A report is destined for a public, indexed issue tracker, which makes the approved-architecture list the one real control in the whole path: it is what stands between an unannounced product name and something permanent and searchable. AMD's published compatibility matrix owns that 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. Ownership moves to the documentation, enforcement stays in the signed binary -- a runtime lookup would put the control somewhere an attacker can reach and would make the catalog load from the network, and both are ruled out. The gate is default-deny in three directions. Hardware absent from the snapshot is refused. Hardware whose architecture could not be read is refused, because "we could not tell" is not permission. And one unreleased GPU withholds the whole report rather than its own entry: publishing the released half would leak the other's existence by the shape of what was withheld. A report is assembled field by field. The one value that comes from a caller rather than from the machine -- the entry id -- is checked against the catalog, so a forged or mistaken id cannot carry caller-supplied text onto a public tracker. The entry named is one the diagnosis established, not the loudest signal it saw: several checkers open with a nonzero score for a merely potentially relevant situation, and naming one of those would look like an established cause to every counter downstream. A fix offered for that entry is now derived from the same catalog check rather than trusted from the caller: `prepare_report` is `pub` and re-exported, and nothing else enforced that `fix_offered` and a recognised `entry` agreed, so a caller could previously have shipped a report claiming a fix exists for a cause the catalog never established. The OS major version is reduced at its actual source, not read from whichever field happens to sit next to the OS family. On Linux that source is the distro release recorded in /etc/os-release, not the kernel build banner `uname -v` returns -- the two were being conflated, so a kernel build timestamp was reaching the report where a release like "22" belonged. Windows has no equivalent structured field, so its NT major is parsed from the `ver` banner instead. Either source is then constrained the same way: only a numeric result reaches the report, anything else collapses to empty, so a future edit that points the source at the wrong field again still cannot carry free text onto a public tracker. Refusing exits 0. The command decided correctly and said why, and a nonzero code would send a caller looking for a fault that is not there. The schema version ships now rather than later. A reader that meets a newer agreement reports the report as unread, never as carrying nothing: a counter that read it as zero would undercount every report from a newer CLI while its totals still looked healthy. Not included, and waiting on the remaining gate answers: the full field list and the group key. The group key matches on ROCm major and minor version, and fixing that before the granularity question is settled is the decision the gate exists to prevent. The allowlist's family labels said the wrong thing. Written as headings above their groups, they were reflowed by rustfmt onto the end of the preceding line, so CDNA parts read as RDNA 2 and gfx1030 read as RDNA 3. The grouping is meaning rather than formatting, and now says so with rustfmt::skip -- writing one target per line was not enough, because the next format pass packed them back exactly as before. The refusal text no longer says "Doctor". That is what the epic calls this capability; the CLI has no such command, so a user reading it has nothing to run and nothing to look up. The e2e step behind "what a report would carry never identifies the machine" swept the answer for the absence of a user name, a host name, and a few path markers, but never checked that a report -- or a stated refusal -- existed to sweep. On any lane with no AMD GPU, the CLI answers with `Refusal::ArchitectureUnreadable`, which contains none of the swept markers either, so the assertions passed while asserting nothing. The step now branches on the outcome the same way its sibling step already does: a genuine report is checked for the fields it should carry before the absence sweep runs, and a refusal is checked for its own shape instead. The discriminator is `cli_version`, a field only a real report carries -- not `architecture`, which also appears inside the refusal's own explanation text and would have reintroduced the same silent pass under a different name. README and the testing guide undersold what a report discloses: both described a report as carrying the entry, the architecture, the OS family and major version, and the CLI version, omitting the two fields the struct already carried, `schema` and `fix_offered`. Corrected to name what the struct actually sends. Signed-off-by: Eugene Volen --- README.md | 11 +- apps/rocm/src/main.rs | 159 ++++- crates/rocm-core/src/fix.rs | 11 + crates/rocm-core/src/lib.rs | 5 + crates/rocm-core/src/report.rs | 575 ++++++++++++++++++ docs/testing.md | 21 + tests/e2e-cucumber/features/diagnose.feature | 25 + .../e2e-cucumber/tests/e2e/diagnose_steps.rs | 116 ++++ 8 files changed, 920 insertions(+), 3 deletions(-) create mode 100644 crates/rocm-core/src/report.rs diff --git a/README.md b/README.md index 499a5b8be..48b70bf2b 100644 --- a/README.md +++ b/README.md @@ -227,6 +227,7 @@ form works depends on the engine your GPU selects. | `rocm` | Open the launcher menu (setup, serve, diagnose, chat, dashboard) | | `rocm examine` | Check GPU, ROCm install, engines, and managed folders | | `rocm diagnose` | Match this machine against known ROCm/PyTorch/llama.cpp failure modes | +| `rocm diagnose --report` | Show what this machine would contribute to a problem report, and send nothing | | `rocm fix []` | Apply a fix reported by `rocm diagnose` | | `rocm install sdk` | Install TheRock ROCm wheels into a managed Python environment | | `rocm install driver` | Install the AMD kernel driver on Linux | @@ -260,7 +261,7 @@ the JSON report, not the human-readable one. ### Diagnose and fix ``` -rocm diagnose [--symptom TEXT] [--top N] [--json] [--distro [NAME]] +rocm diagnose [--symptom TEXT] [--top N] [--json] [--distro [NAME]] [--report] rocm fix [] [--yes] [--dry-run] [--device-index N] ``` @@ -281,6 +282,14 @@ fix` takes the id, not the position. skips checks that need to read the distribution's own environment (`HSA_OVERRIDE_GFX_VERSION`, `PATH`, the framework/ROCm pairing) — run `rocm diagnose` inside the distribution for those. +- `--report` shows exactly 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. The + content is deliberately narrow (a schema version, the matched entry, whether + a fix was offered for it, the GPU architecture, the OS family and major + version, the CLI version), and it carries no host name, user name, file + path, or error text. Hardware that is not on AMD's published compatibility + matrix produces no report at all, and the CLI says why. `fix` applies a known fix by the `id:` that `diagnose` reported — not the ranking position noted above, which isn't a stable name. Run it with no id diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index e600faf20..7c508bc93 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -153,6 +153,13 @@ enum Command { /// name only when more than one is installed. #[arg(long, value_name = "NAME", num_args = 0..=1, default_missing_value = "")] distro: Option, + /// Show the report this machine would contribute, and send nothing. + /// + /// Nothing leaves the machine: this prints the exact content so it can + /// be read before any of it is shared. Hardware that is not on AMD's + /// published compatibility matrix produces no report at all. + #[arg(long, conflicts_with = "distro")] + report: bool, }, /// Apply a known fix by id (see `rocm diagnose`); run with no id to list fixes. /// @@ -2066,7 +2073,8 @@ fn dispatch(cli: Cli) -> Result<()> { top, json, distro, - }) => diagnose(symptom, top, json, distro), + report, + }) => diagnose(symptom, top, json, distro, report), // Keep this error chained rather than discarding it into a fresh // `anyhow!(...)` (e.g. via a `.map_err` that restringifies it) -- see // `FixExitCode`'s doc comment for why that would silently break its @@ -2737,7 +2745,13 @@ fn examine(json: bool, framework: rocm_core::FrameworkProbe) -> Result<()> { Ok(()) } -fn diagnose(symptom: Option, top: usize, json: bool, distro: Option) -> Result<()> { +fn diagnose( + symptom: Option, + top: usize, + json: bool, + distro: Option, + report_requested: bool, +) -> Result<()> { // `rocm diagnose` is a query: it exits 0 whether it matched, found nothing, // or is out of scope. Callers read `has_match` / `out_of_scope` / // `route_when_no_match` from `--json` rather than branching on the exit code. @@ -2770,6 +2784,9 @@ fn diagnose(symptom: Option, top: usize, json: bool, distro: Option, top: usize, json: bool, distro: Option (Option<&str>, bool) { + if !report.has_match { + return (None, false); + } + report.matched.first().map_or((None, false), |top| { + (Some(top.id.as_str()), top.fix.is_some()) + }) +} + +/// Print the report this machine would contribute, and send nothing. +fn show_prepared_report( + examination: &rocm_core::Examination, + report: &rocm_core::DiagnoseReport, + json: bool, +) -> Result<()> { + let (entry, fix_offered) = established_entry(report); + // Exit 0 either way. A refusal is this command working, not failing: it + // decided correctly and said why, and a nonzero code would send a caller + // looking for a fault. Anything scripting this reads the outcome from + // `--json` rather than from the exit code, exactly as `rocm diagnose` itself + // already asks callers to do. + match rocm_core::prepare_report(examination, entry, fix_offered) { + Ok(prepared) => { + if json { + println!("{}", serde_json::to_string_pretty(&prepared)?); + } else { + println!("This is the whole of what a report would carry:"); + println!(); + println!("{}", serde_json::to_string_pretty(&prepared)?); + println!(); + println!("Nothing has been sent. Sending is not implemented yet."); + } + Ok(()) + } + Err(refusal) => { + let explanation = match refusal { + rocm_core::ReportRefusal::UnreleasedHardware => { + // "Doctor" is what the epic calls this capability; the CLI + // has no such command, so a user reading this has nothing + // to run and nothing to look up. + "This machine holds hardware that is not on AMD's published ROCm \ + compatibility matrix, so no report was prepared. A report describes only \ + hardware the compatibility matrix lists as supported." + } + rocm_core::ReportRefusal::ArchitectureUnreadable => { + "No AMD GPU architecture could be read here, so nothing confirms this \ + hardware is on the ROCm compatibility matrix. No report was prepared." + } + }; + if json { + let marker = match refusal { + rocm_core::ReportRefusal::UnreleasedHardware => "unreleased-hardware", + rocm_core::ReportRefusal::ArchitectureUnreadable => "architecture-unreadable", + }; + println!( + "{}", + serde_json::to_string_pretty(&serde_json::json!({ + "schema": rocm_core::REPORT_SCHEMA_VERSION, + "refused": marker, + "explanation": explanation, + "architecture_matrix": rocm_core::APPROVED_ARCHITECTURES_SOURCE, + }))? + ); + } else { + println!("{explanation}"); + } + Ok(()) + } + } +} + fn fix(fix_id: Option, yes: bool, dry_run: bool, device_index: Option) -> Result<()> { let Some(fix_id) = fix_id else { print!("{}", rocm_core::list_fix_recipes()); @@ -22063,6 +22159,65 @@ fn treat_as_natural_language(args: &[String]) -> bool { #[cfg(test)] mod tests { + + /// A diagnosis report holding exactly one finding. + /// + /// `has_match` is passed independently of the score on purpose: the point + /// under test is that the two are read together, so a fixture that derived + /// one from the other could not express the case being guarded against. + fn report_of( + has_match: bool, + id: &str, + score: i32, + fix: Option, + ) -> rocm_core::DiagnoseReport { + rocm_core::DiagnoseReport { + has_match, + matched: vec![rocm_core::Diagnosis { + id: id.to_owned(), + title: "under test".to_owned(), + score, + evidence: Vec::new(), + fix, + }], + min_score_for_match: 50, + high_confidence_threshold: 80, + route_when_no_match: rocm_core::diagnose::Route { + target: String::new(), + url: String::new(), + }, + out_of_scope: None, + } + } + + /// The entry a report names is one the diagnosis established, not merely + /// the strongest signal it saw. + /// + /// This is a wiring test, not a logic one. `established_entry` is correct in + /// itself; what it could get wrong is being handed `matched.first()` + /// unconditionally. Several checkers open with a nonzero score for a + /// situation that is only potentially relevant, so a healthy machine + /// produces a `matched` list full of sub-threshold entries — and a report + /// naming one of those would look like an established cause to every + /// counter downstream, with nothing able to tell the difference afterwards. + #[test] + fn a_report_names_an_established_cause_and_not_the_loudest_weak_signal() { + let weak_only = report_of(false, "fix-10-container", 25, None); + assert_eq!( + established_entry(&weak_only), + (None, false), + "nothing cleared the bar, so the report has no entry to name" + ); + + // Non-vacuity: an established cause must come through, or the assertion + // above is satisfied by never naming anything. + let established = report_of(true, "fix-6-path", 90, Some(rocm_core::Fix::default())); + assert_eq!( + established_entry(&established), + (Some("fix-6-path"), true), + "an established cause with a fix is exactly what a report is for" + ); + } use std::process::ExitCode; /// `Ok(())` must map to a clean exit so `rocm`'s successful commands don't diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index dc056566f..ceed7c155 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -765,6 +765,17 @@ pub(crate) fn torch_rocm_indexes_named_in<'a>( .collect() } +/// Whether `fix_id` names an entry in the catalog. +/// +/// A predicate rather than the lookup itself, because the caller that needs it +/// is deciding whether a caller-supplied string may be published, not reading a +/// recipe. Handing back the recipe would also make a private type reachable +/// from outside this module. +#[must_use] +pub(crate) fn is_catalog_id(fix_id: &str) -> bool { + find_recipe(fix_id).is_some() +} + /// The platform family a recipe's `applies_on` is matched against. /// /// WSL2 is its own family rather than `linux`, mirroring `diagnose`. That is what diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index c9d096f50..fa534aa97 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -33,6 +33,7 @@ pub mod examine; pub mod fix; pub mod openmpi; pub mod proc_lifecycle; +pub mod report; pub mod runtime; #[cfg(test)] mod test_env; @@ -55,6 +56,10 @@ pub use proc_lifecycle::{ IdentityState, KillScope, ProcessIdentity, TerminationOutcome, identity_state, process_start_ticks, terminate_verified, }; +pub use report::{ + APPROVED_ARCHITECTURES, APPROVED_ARCHITECTURES_SOURCE, REPORT_SCHEMA_VERSION, ReadOutcome, + Refusal as ReportRefusal, Report, is_rocm_supported, prepare_report, read_report, +}; use runtime::env_path_override; pub use runtime::{ RUNTIME_LIBRARY_PATH_ENV, RuntimeHost, RuntimePlatform, current_executable_path, diff --git a/crates/rocm-core/src/report.rs b/crates/rocm-core/src/report.rs new file mode 100644 index 000000000..ca412d9a3 --- /dev/null +++ b/crates/rocm-core/src/report.rs @@ -0,0 +1,575 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! The content of a Doctor report, and the rule that decides whether one may +//! exist at all. +//! +//! A report is destined for a public, indexed issue tracker. Nothing here sends +//! one: this module builds content and refuses to build it, and that is all. No +//! network, no filesystem, no paths. +//! +//! The report is assembled field by field. A larger structure is never copied +//! wholesale, because that is how host names, file paths and error text leak +//! into something published. + +use serde::{Deserialize, Serialize}; + +use crate::examine::Examination; + +/// The agreement a reader and a report share. +/// +/// Follows `ENGINE_RECIPE_CONTRACT_VERSION` and the Doctor catalog's +/// `contract_version`. An added field keeps this number; a removed field or a +/// changed type raises it. +pub const REPORT_SCHEMA_VERSION: u32 = 1; + +/// Where [`APPROVED_ARCHITECTURES`] was transcribed from. +/// +/// The list is not maintained here. AMD's ROCm compatibility matrix is the +/// authoritative statement of which hardware ROCm supports, and this is a +/// snapshot of it, compiled in so that it ships signed and is never fetched. +/// +/// Stamped rather than remembered: a snapshot with no provenance cannot be told +/// apart from a current one, and this list going stale is the failure mode that +/// matters. Reviewing it belongs to the per-release catalog review. +pub const APPROVED_ARCHITECTURES_SOURCE: &str = "ROCm compatibility matrix, ROCm 7.1"; + +/// Hardware the ROCm compatibility matrix lists as supported, by LLVM gfx +/// target. +/// +/// The same vocabulary [`crate::examine::Gpu::gfx_target`] reports, so no +/// marketing-name mapping sits between the machine and this decision. +/// +/// This is an allowlist and it is the only control preventing an unannounced +/// product from being named in a public issue. Anything absent is refused, +/// including anything unreadable. Absence from this list is not a claim about +/// retail availability -- plenty of hardware sold today is simply not on the +/// matrix yet, or never will be -- it is a claim about ROCm support, which is +/// the only question this gate is positioned to answer. +// `rustfmt::skip` because the line breaks here are meaning, not formatting: +// each comment labels the group beneath it, and reflowing packs the targets +// onto shared lines so every label ends up trailing the group *above* it. That +// is how this list came to say CDNA parts were RDNA 2 -- a comment that changed +// meaning because of what it ended up next to, in the one file whose job is to +// be exact about which hardware may be named in public. +#[rustfmt::skip] +pub const APPROVED_ARCHITECTURES: &[&str] = &[ + // CDNA 1 through 4. + "gfx908", "gfx90a", "gfx942", "gfx950", + // RDNA 2. + "gfx1030", + // RDNA 3 and 3.5. + "gfx1100", "gfx1101", "gfx1102", "gfx1103", + "gfx1150", "gfx1151", "gfx1152", "gfx1153", + // RDNA 4. + "gfx1200", "gfx1201", +]; + +/// Why no report was produced. +/// +/// Named rather than a bare `None`: Doctor has to explain the refusal, and +/// "hardware we cannot identify" and "hardware that is not released" call for +/// different sentences. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Refusal { + /// A GPU on this machine is not on the ROCm compatibility matrix. + /// + /// Carries no identifier on purpose. Naming the target here would put it in + /// whatever the caller prints, which is the leak this refusal exists to + /// prevent. + UnreleasedHardware, + /// No GPU architecture could be read, so nothing confirms the hardware is + /// on the compatibility matrix. Refused rather than assumed. + ArchitectureUnreadable, +} + +/// A report, as it would be published. +/// +/// Every field is here because it was agreed, not because it was available. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct Report { + pub schema: u32, + /// The gfx target, which is also the only thing said about the hardware. + pub architecture: String, + /// Which snapshot of the ROCm compatibility matrix this build checked + /// `architecture` against, verbatim from [`APPROVED_ARCHITECTURES_SOURCE`]. + /// + /// The matrix changes release to release, and a `Report` carries no other + /// trace of which revision decided its verdict. Without this, two reports + /// naming the same architecture could disagree about whether it was + /// supported, and nothing would say why. + pub architecture_matrix: String, + /// The catalog entry that matched, or [`UNRECOGNISED`] when none did. + pub entry: String, + pub os_family: String, + /// Major only, e.g. `"22"` on Linux or `"10"` on Windows. The exact build + /// identifies a machine far more narrowly than it helps group a problem, + /// and this crate never reads that source: see `os_major` in + /// `report.rs` for the field each platform's value actually comes from. + pub os_major: String, + pub cli_version: String, + pub fix_offered: bool, +} + +/// What a report says when the catalog recognised nothing. +pub const UNRECOGNISED: &str = "unrecognised"; + +/// What a reader made of a report. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ReadOutcome { + Understood(Box), + /// The report is written to an agreement this reader does not know. + /// + /// Distinct from an empty report on purpose. A counter that read this as + /// "nothing here" would silently undercount every report from a newer CLI, + /// and the counts would look healthy while being wrong. + Unread { + schema_seen: u32, + }, +} + +/// Whether a gfx target is on the ROCm compatibility matrix. +#[must_use] +pub fn is_rocm_supported(gfx_target: &str) -> bool { + APPROVED_ARCHITECTURES.contains(&gfx_target) +} + +/// Build the report for this machine, or refuse and say which rule refused. +/// +/// # Errors +/// When the machine holds hardware that is not on the ROCm compatibility +/// matrix, or hardware whose architecture could not be read. +pub fn prepare_report( + examination: &Examination, + entry: Option<&str>, + fix_offered: bool, +) -> Result { + // Every AMD GPU is checked, not just the one a finding concerns. A released + // GPU sitting beside an unreleased one does not make the machine + // reportable: publishing the released half would leak the other's existence + // by the shape of what was withheld. + let amd: Vec<&str> = examination + .gpus + .iter() + .filter(|g| g.is_amd) + .map(|g| g.gfx_target.trim()) + .collect(); + + // Default-deny, and this is the branch that enforces it. A machine with no + // readable AMD architecture has nothing confirming its hardware is on the + // compatibility matrix, and "we could not tell" is not permission. + if amd.is_empty() || amd.iter().any(|gfx| gfx.is_empty()) { + return Err(Refusal::ArchitectureUnreadable); + } + if !amd.iter().all(|gfx| is_rocm_supported(gfx)) { + return Err(Refusal::UnreleasedHardware); + } + + // `entry` is the only value here that comes from a caller rather than from + // the machine. Checked against the catalog so that a forged or mistaken id + // cannot carry caller-supplied text onto a public tracker. + let entry_recognised = entry.is_some_and(crate::fix::is_catalog_id); + let entry = if entry_recognised { + entry.expect("checked Some above").to_owned() + } else { + UNRECOGNISED.to_owned() + }; + // A fix cannot be offered for a cause the catalog did not establish: that + // is a self-contradictory fact once it reaches a public tracker. Derived + // here rather than trusted from the caller, because `prepare_report` is + // `pub` and re-exported, and nothing else enforces the two fields agree. + let fix_offered = fix_offered && entry_recognised; + + Ok(Report { + schema: REPORT_SCHEMA_VERSION, + // The first AMD architecture. All of them are on the compatibility + // matrix by the check above, so this narrows what is said rather than + // choosing what to hide; a machine holding two approved architectures + // is rare enough that a second field would buy grouping accuracy + // nobody needs. + architecture: (*amd.first().expect("checked non-empty above")).to_owned(), + architecture_matrix: APPROVED_ARCHITECTURES_SOURCE.to_owned(), + entry, + os_family: examination.os_family.clone(), + os_major: os_major(examination), + cli_version: env!("CARGO_PKG_VERSION").to_owned(), + fix_offered, + }) +} + +/// The population-level OS major version to publish. +/// +/// Sourced per platform, because no single `Examination` field holds "the OS +/// release" on both: see the two branches below for what each one actually +/// reads. Whatever the source, the result is passed through [`leading_digits`], +/// which discards anything that is not purely numeric -- so a future change to +/// either probe cannot reopen the leak this closes by feeding free text back +/// in through here. +fn os_major(examination: &Examination) -> String { + match examination.os_family.as_str() { + "linux" => { + // `distro_version` is `VERSION_ID` from `/etc/os-release` + // (`examine.rs::probe_os`), e.g. "22.04" -- the actual distro + // release. `os_version` on Linux is `uname -v`'s kernel *build* + // banner (e.g. "#1 SMP PREEMPT_DYNAMIC Thu Jun 18 21:54:43 UTC + // 2026"): a timestamp, not a release, and reading it here is the + // leak this function exists to close. A field named `os_version` + // sitting beside `os_family` reads as "the OS release" to any + // competent reader; it is not, on this platform. + leading_digits(&examination.distro_version) + } + "windows" => { + // Windows has no `/etc/os-release` analogue: `distro_id` / + // `distro_version` are populated only under `runtime_is_linux()` + // in `examine.rs::probe_os`, and stay empty here. The only + // version-bearing field on this platform is `os_version` itself, + // `cmd /C ver`'s banner, e.g. "Microsoft Windows [Version + // 10.0.22631.4460]" -- extract the NT major component that + // follows "Version " rather than reading the banner whole. + windows_os_major(&examination.os_version) + } + // No other platform is supported by `examine.rs`; nothing here is + // known to hold a release, so nothing is published. + _ => String::new(), + } +} + +/// The leading dot/dash-delimited component of `version`, kept only when it +/// is entirely ASCII digits. +/// +/// A full build string narrows a machine much further than it helps group a +/// problem: "22.04.3 with kernel 6.5.0-41" is close to an identifier, while +/// "22" is a population. Anything that survives the split but is not a bare +/// number is discarded rather than passed through -- that non-numeric +/// remainder is exactly the free text a report bound for a public tracker +/// must not carry, whatever field it came from. +fn leading_digits(version: &str) -> String { + let candidate = version.split(['.', '-']).next().unwrap_or_default(); + if !candidate.is_empty() && candidate.bytes().all(|b| b.is_ascii_digit()) { + candidate.to_owned() + } else { + String::new() + } +} + +/// The NT major version out of a `cmd /C ver` banner such as +/// "Microsoft Windows [Version 10.0.22631.4460]", or empty when the banner +/// does not have the expected "Version " marker (a localized banner, for +/// instance) -- an unrecognised shape is refused rather than guessed at. +fn windows_os_major(ver_banner: &str) -> String { + ver_banner + .split_once("Version ") + .map_or(String::new(), |(_, rest)| { + leading_digits(rest.trim_end_matches(']')) + }) +} + +/// Read a report written by some version of this CLI. +#[must_use] +pub fn read_report(json: &str) -> ReadOutcome { + // The schema is read before the body. Deserializing first and checking + // after would make a newer report look malformed rather than merely + // unfamiliar, and those need different answers from a counter. + let Ok(envelope) = serde_json::from_str::(json) else { + return ReadOutcome::Unread { schema_seen: 0 }; + }; + let schema_seen = envelope + .get("schema") + .and_then(serde_json::Value::as_u64) + .and_then(|v| u32::try_from(v).ok()) + .unwrap_or(0); + if schema_seen != REPORT_SCHEMA_VERSION { + return ReadOutcome::Unread { schema_seen }; + } + serde_json::from_value::(envelope) + .map_or(ReadOutcome::Unread { schema_seen }, |report| { + ReadOutcome::Understood(Box::new(report)) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::examine::Gpu; + + /// A machine whose every free-text field is a distinctive marker. + /// + /// The markers are what [`no_report_carries_a_value_the_machine_did_not_agree_to_publish`] + /// sweeps for. Real values are avoided on purpose: a plausible-looking path + /// could be missed by eye in a rendered report, whereas one of these could + /// not. + fn machine_of_sentinels(gfx: &str) -> Examination { + Examination { + os_family: "linux".to_owned(), + // A real `uname -v` shape (this is what it prints on the host + // this fix was written on), not a release: see + // [`the_reported_os_major_comes_from_the_distro_release_not_the_kernel_banner`] + // for why that distinction is the whole point. + os_version: "#1 SMP PREEMPT_DYNAMIC Thu Jun 18 21:54:43 UTC 2026".to_owned(), + distro_version: "22.04".to_owned(), + user_name: "SENTINEL-USER".to_owned(), + rocm_path: "/SENTINEL-PATH/rocm".to_owned(), + kernel_cmdline: "SENTINEL-CMDLINE".to_owned(), + hip_sdk_path: "C:/SENTINEL-PATH".to_owned(), + cpu_model: "SENTINEL-CPU".to_owned(), + distro_id: "SENTINEL-DISTRO".to_owned(), + rocminfo_status: "SENTINEL-ERROR-TEXT".to_owned(), + has_amd_gpu: true, + gpus: vec![Gpu { + name: "SENTINEL-MARKETING-NAME".to_owned(), + gfx_target: gfx.to_owned(), + pci_id: "SENTINEL-PCI".to_owned(), + is_apu: Some(false), + is_amd: true, + }], + ..Examination::default() + } + } + + /// Every marker planted above, so the sweep cannot silently check fewer + /// than it was given. + const SENTINELS: &[&str] = &[ + "SENTINEL-USER", + "SENTINEL-PATH", + "SENTINEL-CMDLINE", + "SENTINEL-CPU", + "SENTINEL-DISTRO", + "SENTINEL-ERROR-TEXT", + "SENTINEL-MARKETING-NAME", + "SENTINEL-PCI", + ]; + + /// An architecture no product will ever have. + const NOT_RELEASED: &str = "gfx9999"; + + /// I1 — a machine holding hardware that is not on the ROCm compatibility + /// matrix never produces a report. + /// + /// The paired assertion is the one that matters. "Unapproved machine is + /// refused" alone is satisfied by an implementation that refuses + /// everything, so the same machine with the hardware swapped for something + /// released has to come back with a report. + #[test] + fn hardware_off_the_rocm_compatibility_matrix_never_produces_a_report() { + let released = prepare_report(&machine_of_sentinels("gfx1100"), None, false); + assert!( + released.is_ok(), + "premise failed: a machine holding only released hardware must produce a report, \ + otherwise the refusal below is satisfied by refusing everything. Got {released:?}" + ); + + assert_eq!( + prepare_report(&machine_of_sentinels(NOT_RELEASED), None, false), + Err(Refusal::UnreleasedHardware), + "{NOT_RELEASED} is on no compatibility matrix, so it must not be describable" + ); + } + + /// I1, continued — one unreleased GPU withholds the whole report. + /// + /// Reporting the released half would leak the other's existence by the + /// shape of what was withheld. + #[test] + fn one_unreleased_gpu_withholds_the_whole_report_not_just_its_own_entry() { + let mut mixed = machine_of_sentinels("gfx1100"); + mixed.gpus.push(Gpu { + name: "SENTINEL-MARKETING-NAME".to_owned(), + gfx_target: NOT_RELEASED.to_owned(), + pci_id: "SENTINEL-PCI".to_owned(), + is_apu: Some(false), + is_amd: true, + }); + assert_eq!( + prepare_report(&mixed, None, false), + Err(Refusal::UnreleasedHardware), + "a released GPU beside an unreleased one does not make the machine reportable" + ); + } + + /// I3 — hardware that could not be identified is refused, not assumed. + #[test] + fn hardware_that_could_not_be_identified_is_refused_rather_than_assumed() { + assert_eq!( + prepare_report(&machine_of_sentinels(""), None, false), + Err(Refusal::ArchitectureUnreadable), + "nothing confirmed this hardware is on the compatibility matrix, and default-deny \ + is the whole point" + ); + } + + /// I2 — no report carries a value the machine did not agree to publish. + /// + /// Sweeps the serialized bytes rather than enumerating field names, because + /// the mistake being guarded against is a larger structure copied wholesale: + /// a field list check passes while a nested examination rides along inside + /// an approved field. + #[test] + fn no_report_carries_a_value_the_machine_did_not_agree_to_publish() { + let report = prepare_report(&machine_of_sentinels("gfx1100"), Some("fix-6-path"), true) + .expect("a released machine must produce a report"); + let serialized = serde_json::to_string(&report).expect("a report must serialize"); + + // Non-vacuity. An empty report carries no markers either, so without + // this the sweep below would pass against a report that says nothing. + assert!( + serialized.contains("gfx1100"), + "the report has to actually describe the machine before 'it leaks nothing' means \ + anything: {serialized}" + ); + + for sentinel in SENTINELS { + assert!( + !serialized.contains(sentinel), + "{sentinel} reached a report bound for a public issue tracker: {serialized}" + ); + } + } + + /// I6 — the report's OS major comes from the distro release, never from + /// the kernel build banner that actually lives in `os_version` on Linux. + /// + /// Found by mutation, not by design: publishing `examination.os_version` + /// whole passed every other test here. The sentinel sweep could not catch + /// it because a kernel banner is not a planted marker, it is a real and + /// plausible-looking value — and that is exactly what makes it easy to + /// ship. A prior version of this test set `os_version = "22.04.3"`, a + /// shape `probe_os` cannot produce on Linux (`uname -v` prints a build + /// banner, not a release), so the assertion was satisfied by an invented + /// fixture rather than by the production path. These three banners are + /// real: the first is what `uname -v` prints on the host this fix was + /// written on; the other two are the Ubuntu and Debian shapes. + #[test] + fn the_reported_os_major_comes_from_the_distro_release_not_the_kernel_banner() { + let kernel_banners = [ + "#1 SMP PREEMPT_DYNAMIC Thu Jun 18 21:54:43 UTC 2026", + "#139-Ubuntu SMP Fri Sep 27 14:22:11 UTC 2024", + "#1 SMP PREEMPT_DYNAMIC Debian 6.1.129-1", + ]; + for banner in kernel_banners { + let mut machine = machine_of_sentinels("gfx1100"); + machine.os_version = banner.to_owned(); + machine.distro_version = "22.04".to_owned(); + + let report = prepare_report(&machine, None, false) + .expect("a released machine must produce a report"); + assert_eq!( + report.os_major, "22", + "os_major must come from distro_version, not the kernel banner in os_version \ + ({banner:?})" + ); + + let serialized = serde_json::to_string(&report).expect("a report must serialize"); + assert!( + !serialized.contains(banner), + "the kernel build banner reached a report bound for a public tracker: {serialized}" + ); + } + } + + /// I6, continued — the Windows equivalent. `distro_version` is never + /// populated there (`examine.rs::probe_os` only sets it under + /// `runtime_is_linux()`), so the NT major version has to come out of + /// `os_version` itself, `cmd /C ver`'s banner — but only the major + /// component, never the banner whole. + #[test] + fn the_reported_os_major_on_windows_is_the_nt_major_version_not_the_ver_banner() { + let mut machine = machine_of_sentinels("gfx1100"); + machine.os_family = "windows".to_owned(); + machine.os_version = "Microsoft Windows [Version 10.0.22631.4460]".to_owned(); + + let report = prepare_report(&machine, None, false) + .expect("a released machine must produce a report"); + assert_eq!( + report.os_major, "10", + "the report groups by NT major version, so that is what it carries" + ); + + let serialized = serde_json::to_string(&report).expect("a report must serialize"); + assert!( + !serialized.contains("22631"), + "the exact Windows build reached a report bound for a public tracker: {serialized}" + ); + assert!( + !serialized.contains("Microsoft Windows"), + "the ver banner reached a report bound for a public tracker: {serialized}" + ); + } + + /// I6, continued — a non-numeric source is refused rather than + /// published, regardless of platform. This is the guard that keeps the + /// leak closed even if a future edit changes the source again: whatever + /// feeds `os_major` next, free text still cannot pass through it. + #[test] + fn a_non_numeric_os_release_source_never_reaches_the_report_as_free_text() { + let mut machine = machine_of_sentinels("gfx1100"); + machine.distro_version = "SENTINEL-UNPARSEABLE-RELEASE".to_owned(); + + let report = prepare_report(&machine, None, false) + .expect("a released machine must produce a report"); + assert_eq!( + report.os_major, "", + "a release string that does not reduce to a bare number must not pass through as \ + free text" + ); + } + + /// I5 — an entry id the catalog does not know is never published verbatim. + /// + /// `entry` is the one field whose value comes from a caller rather than + /// from the machine, which makes it the one free-text hole in a structure + /// that is otherwise assembled field by field. A forged or mistaken id must + /// not ride through to a public tracker. + #[test] + fn an_entry_id_the_catalog_does_not_know_is_never_published_verbatim() { + // Non-vacuity: a real id has to reach the report, or "the forged one + // does not" is satisfied by discarding every id. + let known = prepare_report(&machine_of_sentinels("gfx1100"), Some("fix-6-path"), false) + .expect("a released machine must produce a report"); + assert_eq!( + known.entry, "fix-6-path", + "premise failed: a real catalog id must reach the report, otherwise the assertion \ + below passes against an implementation that publishes no id at all" + ); + + let forged = prepare_report( + &machine_of_sentinels("gfx1100"), + Some("SENTINEL-FORGED-ENTRY"), + false, + ) + .expect("a released machine must produce a report"); + assert_eq!( + forged.entry, UNRECOGNISED, + "an id the catalog does not know is not a finding, and publishing it verbatim would \ + put caller-supplied text on a public tracker" + ); + } + + /// I4 — a reader that does not know the agreement says so, rather than + /// reading the report as carrying nothing. + #[test] + fn a_reader_that_does_not_understand_a_report_says_so_rather_than_counting_it_as_empty() { + let newer = format!( + r#"{{"schema":{},"architecture":"gfx1100","entry":"fix-6-path","os_family":"linux","os_major":"22","cli_version":"9.9.9","fix_offered":true,"field_added_later":"x"}}"#, + REPORT_SCHEMA_VERSION + 1 + ); + assert_eq!( + read_report(&newer), + ReadOutcome::Unread { + schema_seen: REPORT_SCHEMA_VERSION + 1 + }, + "a newer agreement is unread, never counted as zero" + ); + + let current = serde_json::to_string( + &prepare_report(&machine_of_sentinels("gfx1100"), Some("fix-6-path"), true) + .expect("a released machine must produce a report"), + ) + .expect("a report must serialize"); + assert!( + matches!(read_report(¤t), ReadOutcome::Understood(_)), + "premise failed: a report this reader does write must be one it can read, or the \ + assertion above is satisfied by a reader that understands nothing" + ); + } +} diff --git a/docs/testing.md b/docs/testing.md index c4f249acc..d68c2294e 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -1172,3 +1172,24 @@ The install is covered by unit tests over the generated plan (`cargo test -p rocm --bin rocm wsl_rocdxg`). Running it end to end needs a WSL2 host with `/dev/dxg` and dxcore present, since the plan refuses before installing otherwise. + +## Doctor Report Preflight + +Preview the content a problem report would carry, without sending anything: + +```bash +rocm diagnose --report +rocm diagnose --report --json +``` + +The command refuses rather than prepares a report on two hosts: one with an +architecture the ROCm compatibility matrix does not list as supported, and +one whose AMD GPU architecture could not be read at all. Both refusals exit 0 +and are distinguishable from `--json`'s `refused` field. + +On a host with an approved architecture (see `APPROVED_ARCHITECTURES` in +`crates/rocm-core/src/report.rs`), the command prints the full `Report`: +`schema`, `architecture`, `architecture_matrix`, `entry`, `os_family`, +`os_major`, `cli_version`, and `fix_offered`. This path has not been exercised +against real hardware in CI; verifying it needs a lane whose GPU architecture +is on the allowlist. diff --git a/tests/e2e-cucumber/features/diagnose.feature b/tests/e2e-cucumber/features/diagnose.feature index d33c22e74..9d1488944 100644 --- a/tests/e2e-cucumber/features/diagnose.feature +++ b/tests/e2e-cucumber/features/diagnose.feature @@ -289,3 +289,28 @@ Feature: Diagnosing failures and listing fixes When the user previews that fix without applying it Then the preview states that the fix requires sudo and a re-login And the preview states that the CLI can run it automatically + + # Nothing here sends a report -- transport does not exist yet -- so what these + # two prove is the part that has to be right before it does: that the machine + # can see exactly what would be published, and that asking produces either a + # report or a stated refusal and never a silent send. + # + # Host-independent on purpose, and the two halves land on different lanes. A + # lane with an AMD GPU on the compatibility matrix exercises the prepared + # report; a lane without one exercises the refusal, which is the case the mock + # lane actually has. Written so that whichever branch a lane reaches is a real + # assertion rather than a skip. + @id:diagnose-report-is-shown-and-not-sent + Scenario: diagnose-21 - Asking what a report would say shows it and sends nothing + When the user asks the CLI what a report would carry + Then the CLI either shows the whole report or says why it will not prepare one + And the CLI states that nothing has been sent + + # The rule this guards is that a report is assembled field by field, never by + # copying a larger structure. The unit tests sweep for planted markers; this + # asserts the same property against whatever this real machine happens to be, + # which is the case a fixture cannot reproduce. + @id:diagnose-report-carries-no-identifying-detail + Scenario: diagnose-22 - What a report would carry never identifies the machine + When the user asks the CLI what a report would carry in machine-readable form + Then the answer names no user, no host, and no file path diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs index 2c59e6de1..b0b9277fb 100644 --- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs @@ -1133,3 +1133,119 @@ async fn assert_command_failure_reported_on_stderr(world: &mut E2eWorld) { "the command-failure explanation must not also be on stdout:\n{stdout}" ); } + +#[when("the user asks the CLI what a report would carry")] +async fn user_asks_what_a_report_would_carry(world: &mut E2eWorld) { + let (stdout, stderr, rc) = crate::run_rocm(world, &["diagnose", "--report"]); + world.cli_output = Some(format!("{stdout}\n{stderr}")); + world.cli_rc = Some(rc); +} + +#[when("the user asks the CLI what a report would carry in machine-readable form")] +async fn user_asks_what_a_report_would_carry_json(world: &mut E2eWorld) { + let (stdout, stderr, rc) = crate::run_rocm(world, &["diagnose", "--report", "--json"]); + world.cli_output = Some(format!("{stdout}\n{stderr}")); + world.cli_rc = Some(rc); +} + +#[then("the CLI either shows the whole report or says why it will not prepare one")] +async fn report_is_shown_or_refused(world: &mut E2eWorld) { + let out = world.cli_output.clone().expect("no CLI output"); + let shown = out.contains("a report would carry"); + let refused = out.contains("no report was prepared") || out.contains("No report was prepared"); + assert!( + shown || refused, + "asking for a report produced neither a report nor a stated refusal, which leaves a \ + user unable to tell what would be published:\n{out}" + ); + assert_eq!( + world.cli_rc, + Some(0), + "a refusal is this command working, not failing, so both branches exit 0:\n{out}" + ); +} + +#[then("the CLI states that nothing has been sent")] +async fn nothing_has_been_sent(world: &mut E2eWorld) { + let out = world.cli_output.clone().expect("no CLI output"); + // Only the prepared-report branch makes the promise; a refusal prepared + // nothing to send, so requiring the sentence there would assert about a + // report that does not exist. + if out.contains("a report would carry") { + assert!( + out.contains("Nothing has been sent"), + "the report was shown without saying it stayed here, which is the one thing a user \ + needs to know before reading it:\n{out}" + ); + } +} + +#[then("the answer names no user, no host, and no file path")] +async fn answer_names_nothing_identifying(world: &mut E2eWorld) { + let out = world.cli_output.clone().expect("no CLI output"); + // A refusal envelope (`{"schema","refused","explanation"}`) trivially + // contains none of the markers swept below, so on a lane whose hardware is + // not on the allowlist -- the common case, since most lanes have no AMD + // GPU at all -- every sweep would pass without a report ever having + // existed to sweep. Branch on the outcome, the same way the sibling step + // `nothing_has_been_sent` already does. + // + // `cli_version` is the discriminator, not `architecture`: the + // `ArchitectureUnreadable` refusal's own explanation text ("No AMD GPU + // *architecture* could be read here...") contains the word "architecture", + // so keying off that field name would make this same vacuous pass survive + // under a different guise on exactly the refusal this sandbox reaches. + // `cli_version` is a field `Report` carries and no refusal explanation + // does. + if out.contains("cli_version") { + assert!( + out.contains("architecture"), + "a genuine report is missing the architecture field it is supposed to carry:\n{out}" + ); + } else { + assert!( + out.contains("no report was prepared") + || out.contains("No report was prepared") + || out.contains("\"refused\""), + "the output is neither a genuine report nor a stated refusal, so this assertion \ + would otherwise pass without a report ever existing to check:\n{out}" + ); + return; + } + let user = std::env::var("USER") + .or_else(|_| std::env::var("USERNAME")) + .unwrap_or_default(); + if !user.is_empty() && user.len() > 2 { + assert!( + !out.contains(&user), + "the user name reached what a report would publish:\n{out}" + ); + } + let host = hostname_of_this_machine(); + if !host.is_empty() && host.len() > 2 { + assert!( + !out.contains(&host), + "the host name reached what a report would publish:\n{out}" + ); + } + for path_marker in ["/opt/rocm", "/home/", "C:\\", "/usr/"] { + assert!( + !out.contains(path_marker), + "a file path ({path_marker}) reached what a report would publish:\n{out}" + ); + } +} + +/// This machine's host name, or empty when it cannot be read. +/// +/// Read here rather than from the CLI: the assertion is that the name never +/// appears in a report, so taking it from the thing under test would compare +/// the report against itself. +fn hostname_of_this_machine() -> String { + std::process::Command::new("hostname") + .output() + .ok() + .and_then(|o| String::from_utf8(o.stdout).ok()) + .map(|s| s.trim().to_owned()) + .unwrap_or_default() +} From a0a0c9dfdd90d050c209e68ee86e364da385c6a6 Mon Sep 17 00:00:00 2001 From: Eugene Volen Date: Wed, 30 Sep 2026 08:12:10 +0000 Subject: [PATCH 2/5] feat(diagnose): carry the fields a report has to group by A report said the OS family and its major version, which cannot separate Ubuntu 22 from any other distribution numbered 22. Reports are grouped by exact match on their fields, so that granularity collapses distinct populations into one group and offers no way to tell which of them a problem belongs to. A filed report cannot be widened later, so a field missing now is missing from the whole corpus. Adds the distribution, the ROCm release, and the engine with its release, beside the existing fields rather than replacing them: an addition keeps the schema version where a removal would raise it. Each new field is narrowed on the way out. Versions are cut to major and minor, because 7.0 and 7.1 are different problems while a build number narrows toward one machine. The distribution is checked against a list of known names: `ID=` in /etc/os-release is free text written by whoever built the image, and an unrecognised value is as likely to name a company as a distribution, so it becomes `other` and still groups. The engine is checked too, guarding a future probe that sets the field from parsed output rather than from a literal. The engine and its version are decided together so the pair cannot disagree, and 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. Signed-off-by: Eugene Volen --- README.md | 10 +- crates/rocm-core/src/report.rs | 293 +++++++++++++++++++++++++++++++++ docs/testing.md | 15 +- 3 files changed, 312 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 48b70bf2b..ccffe8288 100644 --- a/README.md +++ b/README.md @@ -286,9 +286,13 @@ fix` takes the id, not the position. 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. The content is deliberately narrow (a schema version, the matched entry, whether - a fix was offered for it, the GPU architecture, the OS family and major - version, the CLI version), and it carries no host name, user name, file - path, or error text. Hardware that is not on AMD's published compatibility + a fix was offered for it, the GPU architecture, the OS family, distribution + and major version, the ROCm release, the inference engine and its release, + the CLI version), and it carries no host name, user name, file path, or + error text. Every version is cut back to a release, so a build number that + would narrow toward one machine never appears, and the distribution is + checked against a list of known names rather than repeated from the + machine. Hardware that is not on AMD's published compatibility matrix produces no report at all, and the CLI says why. `fix` applies a known fix by the `id:` that `diagnose` reported — not the diff --git a/crates/rocm-core/src/report.rs b/crates/rocm-core/src/report.rs index ca412d9a3..54af7b937 100644 --- a/crates/rocm-core/src/report.rs +++ b/crates/rocm-core/src/report.rs @@ -66,6 +66,53 @@ pub const APPROVED_ARCHITECTURES: &[&str] = &[ "gfx1200", "gfx1201", ]; +/// Distribution identifiers that may be published, as `/etc/os-release` spells +/// them in `ID=`. +/// +/// This list exists for a different reason than [`APPROVED_ARCHITECTURES`]. +/// No distribution is a secret, and none is withheld here. The hazard is that +/// `ID=` is free text read from a file on the user's machine: a vendor image, +/// a derivative, or a private build writes whatever it likes there, and a +/// report bound for an issue tracker must not carry it. Anything absent +/// becomes [`DISTRO_OTHER`], which still groups and says nothing. +/// +/// Generous on purpose. A name that is missing costs grouping accuracy for +/// real users, while a name that is present costs nothing, so the bar for +/// adding one is only that it is a distribution rather than a description of +/// somebody's fleet. +// `rustfmt::skip` for the same reason as `APPROVED_ARCHITECTURES`: the comments +// label the group beneath them, and reflowing moves each label onto the group +// above it. +#[rustfmt::skip] +pub const APPROVED_DISTROS: &[&str] = &[ + // Named by the ROCm compatibility matrix. + "ubuntu", "rhel", "sles", "ol", "debian", "rocky", "azurelinux", + // Common elsewhere, and grouped rather than flattened into `other`. + "almalinux", "centos", "fedora", "opensuse-leap", "opensuse-tumbleweed", + "arch", "linuxmint", "pop", +]; + +/// What the distribution field says when the identifier is not one this build +/// recognises. A real value, so that such machines still group together. +pub const DISTRO_OTHER: &str = "other"; + +/// What a field says when this build looked and could not tell. +/// +/// Kept apart from [`NONE`]: "no ROCm is installed" and "ROCm is installed and +/// its version could not be read" are different facts, and a counter that +/// merged them would report an install problem as an absence. +pub const UNKNOWN: &str = "unknown"; + +/// What a field says when the thing is absent rather than unreadable. +pub const NONE: &str = "none"; + +/// Engine names that may be published. +/// +/// Every value is written by this crate rather than parsed from a machine, so +/// this guards against a future probe rather than against today's. The +/// accompanying version is parsed, and is truncated instead. +pub const APPROVED_ENGINES: &[&str] = &["pytorch", "llama-cpp"]; + /// Why no report was produced. /// /// Named rather than a bare `None`: Doctor has to explain the refusal, and @@ -108,6 +155,23 @@ pub struct Report { /// and this crate never reads that source: see `os_major` in /// `report.rs` for the field each platform's value actually comes from. pub os_major: String, + /// The distribution, as an `/etc/os-release` `ID=` value on the approved + /// list, or [`DISTRO_OTHER`]. `"windows"` on Windows, which has no such + /// file. + /// + /// Carried beside `os_family` rather than replacing it. Grouping needs to + /// tell Ubuntu 22 from any other distribution numbered 22, which the + /// family and the major version cannot do between them. + pub distro: String, + /// The installed ROCm release as major and minor, e.g. `"7.1"`, or + /// [`NONE`] / [`UNKNOWN`]. + pub rocm: String, + /// The inference engine found on this machine, or [`NONE`] / [`UNKNOWN`]. + pub engine: String, + /// That engine's release as major and minor, truncated the same way as + /// every other version here because it is parsed from an installed + /// package rather than written by this crate. + pub engine_version: String, pub cli_version: String, pub fix_offered: bool, } @@ -180,6 +244,9 @@ pub fn prepare_report( // here rather than trusted from the caller, because `prepare_report` is // `pub` and re-exported, and nothing else enforces the two fields agree. let fix_offered = fix_offered && entry_recognised; + // Together, so the pair cannot disagree. A version beside `none` would + // describe an engine the report also says is not installed. + let (engine, engine_version) = engine_and_version(examination); Ok(Report { schema: REPORT_SCHEMA_VERSION, @@ -193,6 +260,10 @@ pub fn prepare_report( entry, os_family: examination.os_family.clone(), os_major: os_major(examination), + distro: distro(examination), + rocm: rocm_release(examination), + engine, + engine_version, cli_version: env!("CARGO_PKG_VERSION").to_owned(), fix_offered, }) @@ -235,6 +306,107 @@ fn os_major(examination: &Examination) -> String { } } +/// The distribution to publish. +/// +/// Linux reads `ID=` from `/etc/os-release`, which is free text written by +/// whoever built the image. It is checked against [`APPROVED_DISTROS`] rather +/// than published, because an unrecognised value is as likely to name a +/// company as a distribution. +/// +/// Windows has no such file: `examine.rs::probe_os` populates `distro_id` only +/// under `runtime_is_linux()`, so the value there is a constant rather than +/// anything read from the machine. +fn distro(examination: &Examination) -> String { + match examination.os_family.as_str() { + "linux" => { + let id = examination.distro_id.trim().to_ascii_lowercase(); + if id.is_empty() { + // `/etc/os-release` was missing or unreadable. Distinct from an + // unrecognised name: this build did not get to decide. + UNKNOWN.to_owned() + } else if APPROVED_DISTROS.contains(&id.as_str()) { + id + } else { + DISTRO_OTHER.to_owned() + } + } + "windows" => "windows".to_owned(), + _ => String::new(), + } +} + +/// The installed ROCm release, as major and minor. +/// +/// `rocm_path` decides absence and `rocm_version` decides readability, because +/// `examine.rs` collapses both into an empty string: `rocm_version` is +/// `install.version.unwrap_or_default()`, so "no install was found" and "an +/// install was found whose version could not be read" arrive identical. Those +/// are different facts to anybody counting, in the same way [`ReadOutcome`] +/// keeps an unreadable report apart from an absent one. +/// +/// The path itself is only ever read here. It is never published. +fn rocm_release(examination: &Examination) -> String { + if examination.rocm_path.trim().is_empty() { + return NONE.to_owned(); + } + let release = major_minor(&examination.rocm_version); + if release.is_empty() { + UNKNOWN.to_owned() + } else { + release + } +} + +/// The engine and its release, decided together. +/// +/// `framework` is written by this crate from a closed set, so it is checked +/// against [`APPROVED_ENGINES`] to catch a future probe rather than today's. +/// `framework_version` is parsed out of an installed package, so it is +/// truncated like every other version here. +fn engine_and_version(examination: &Examination) -> (String, String) { + let name = examination.framework.trim().to_ascii_lowercase(); + // `"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. + // Neither is an engine, and neither may carry a version. + if name.is_empty() { + return (NONE.to_owned(), NONE.to_owned()); + } + if !APPROVED_ENGINES.contains(&name.as_str()) { + return (UNKNOWN.to_owned(), UNKNOWN.to_owned()); + } + let version = major_minor(&examination.framework_version); + let version = if version.is_empty() { + UNKNOWN.to_owned() + } else { + version + }; + (name, version) +} + +/// The leading `major.minor` of `version`, keeping only components that are +/// entirely ASCII digits. +/// +/// Wider than [`leading_digits`], which keeps the major alone. A ROCm or +/// engine release without its minor does not group: 7.0 and 7.1 are different +/// problems, while 22.04 and 22.10 are the same population. Anything past the +/// minor is a build, which narrows toward one machine, so it is dropped. +fn major_minor(version: &str) -> String { + let mut parts = version.trim().split(['.', '-', '+', '_']); + let major = parts.next().unwrap_or_default(); + if major.is_empty() || !major.bytes().all(|b| b.is_ascii_digit()) { + return String::new(); + } + match parts.next() { + Some(minor) if !minor.is_empty() && minor.bytes().all(|b| b.is_ascii_digit()) => { + format!("{major}.{minor}") + } + // A bare major is still a population. A non-numeric minor is the free + // text this function exists to drop, and dropping it must not take the + // major with it. + _ => major.to_owned(), + } +} + /// The leading dot/dash-delimited component of `version`, kept only when it /// is entirely ASCII digits. /// @@ -315,6 +487,16 @@ mod tests { cpu_model: "SENTINEL-CPU".to_owned(), distro_id: "SENTINEL-DISTRO".to_owned(), rocminfo_status: "SENTINEL-ERROR-TEXT".to_owned(), + // Real shapes with a marker in the tail, not pure markers. A pure + // marker is rejected outright and proves only that garbage is + // dropped; these prove the published head survives while the build + // tail -- the part that narrows toward one machine -- does not. + // `framework` is a real approved value for the same reason: an + // unapproved one short-circuits before the version is ever read, + // so the version sweep would pass without that path running. + rocm_version: "7.1.0-SENTINEL-ROCM-BUILD".to_owned(), + framework: "pytorch".to_owned(), + framework_version: "2.5.1+SENTINEL-ENGINE-BUILD".to_owned(), has_amd_gpu: true, gpus: vec![Gpu { name: "SENTINEL-MARKETING-NAME".to_owned(), @@ -338,6 +520,8 @@ mod tests { "SENTINEL-ERROR-TEXT", "SENTINEL-MARKETING-NAME", "SENTINEL-PCI", + "SENTINEL-ROCM-BUILD", + "SENTINEL-ENGINE-BUILD", ]; /// An architecture no product will ever have. @@ -496,6 +680,115 @@ mod tests { ); } + /// The grouping fields carry a population, not a machine. + /// + /// Asserts the published values rather than the absence of the markers. + /// The sweep alone would pass if every one of these fields were empty, and + /// an empty field is exactly what a grouper cannot use: this states that + /// the numeric head survived while the build tail did not. + #[test] + fn the_grouping_fields_keep_the_release_and_drop_the_build() { + let report = prepare_report(&machine_of_sentinels("gfx1100"), None, false) + .expect("a released machine must produce a report"); + + assert_eq!( + report.rocm, "7.1", + "the ROCm release has to survive truncation: 7.0 and 7.1 are different problems" + ); + assert_eq!(report.engine, "pytorch"); + assert_eq!( + report.engine_version, "2.5", + "the engine release has to survive truncation the same way" + ); + assert_eq!( + report.distro, DISTRO_OTHER, + "an ID this build does not recognise has to group, not be republished" + ); + } + + /// A distribution name is checked, not trusted. + /// + /// Paired, because "always answers `other`" satisfies the unrecognised + /// half on its own and would throw away every real distribution. + #[test] + fn a_recognised_distribution_is_named_and_an_unrecognised_one_is_not() { + let mut known = machine_of_sentinels("gfx1100"); + known.distro_id = "Ubuntu".to_owned(); + let report = prepare_report(&known, None, false).expect("a released machine reports"); + assert_eq!( + report.distro, "ubuntu", + "premise failed: a distribution on the list must be named, otherwise the case below \ + is satisfied by discarding every name" + ); + + // The shape that matters: a private image whose `ID=` names its owner + // rather than a distribution. + let mut vendor = machine_of_sentinels("gfx1100"); + vendor.distro_id = "SENTINEL-CORP-INTERNAL-IMAGE".to_owned(); + let report = prepare_report(&vendor, None, false).expect("a released machine reports"); + assert_eq!(report.distro, DISTRO_OTHER); + let serialized = serde_json::to_string(&report).expect("a report must serialize"); + assert!( + !serialized.contains("SENTINEL-CORP"), + "an ID written by whoever built the image reached a public tracker: {serialized}" + ); + } + + /// An engine name this build does not recognise is not published. + /// + /// Today every value of `framework` is a literal written by `examine.rs`, + /// so this guards a future probe rather than the current one: the moment + /// one sets that field from parsed output, an allowlist is the difference + /// between a name and whatever the parse produced. Found by mutation -- + /// removing the check changed nothing, because the shared fixture uses an + /// approved engine and so never reached the branch. + #[test] + fn an_engine_this_build_does_not_recognise_is_not_named_in_the_report() { + let mut machine = machine_of_sentinels("gfx1100"); + machine.framework = "SENTINEL-UNAPPROVED-ENGINE".to_owned(); + let report = prepare_report(&machine, None, false).expect("a released machine reports"); + + assert_eq!( + report.engine, UNKNOWN, + "an engine name off the list has to be withheld, not republished" + ); + assert_eq!( + report.engine_version, UNKNOWN, + "a version cannot describe an engine the report declines to name" + ); + let serialized = serde_json::to_string(&report).expect("a report must serialize"); + assert!( + !serialized.contains("SENTINEL-UNAPPROVED"), + "an unrecognised engine name reached a public tracker: {serialized}" + ); + } + + /// An absent ROCm and an unreadable one are different facts. + /// + /// `examine.rs` collapses both into an empty `rocm_version`, so without + /// this the report would say "not installed" about a machine whose install + /// merely could not be read -- turning an install problem into an absence + /// for anybody counting. + #[test] + fn no_rocm_installed_reads_differently_from_a_rocm_that_could_not_be_read() { + let mut absent = machine_of_sentinels("gfx1100"); + absent.rocm_path = String::new(); + absent.rocm_version = String::new(); + let absent = prepare_report(&absent, None, false).expect("a released machine reports"); + + let mut unreadable = machine_of_sentinels("gfx1100"); + unreadable.rocm_version = String::new(); + let unreadable = + prepare_report(&unreadable, None, false).expect("a released machine reports"); + + assert_eq!(absent.rocm, NONE); + assert_eq!(unreadable.rocm, UNKNOWN); + assert_ne!( + absent.rocm, unreadable.rocm, + "a counter cannot tell an absent ROCm from an unreadable one" + ); + } + /// I6, continued — a non-numeric source is refused rather than /// published, regardless of platform. This is the guard that keeps the /// leak closed even if a future edit changes the source again: whatever diff --git a/docs/testing.md b/docs/testing.md index d68c2294e..a1f282f0f 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -1190,6 +1190,15 @@ and are distinguishable from `--json`'s `refused` field. On a host with an approved architecture (see `APPROVED_ARCHITECTURES` in `crates/rocm-core/src/report.rs`), the command prints the full `Report`: `schema`, `architecture`, `architecture_matrix`, `entry`, `os_family`, -`os_major`, `cli_version`, and `fix_offered`. This path has not been exercised -against real hardware in CI; verifying it needs a lane whose GPU architecture -is on the allowlist. +`os_major`, `distro`, `rocm`, `engine`, `engine_version`, `cli_version`, and +`fix_offered`. This path has not been exercised against real hardware in CI; +verifying it needs a lane whose GPU architecture is on the allowlist. + +Three of those fields answer with a word rather than a value, and the words +are not interchangeable. `none` means the thing is absent, `unknown` means this +build looked and could not tell, and `other` means a distribution was named but +is not one this build recognises. A host with no ROCm installed reports +`"rocm": "none"`, while a host whose install exists but whose version could not +be read reports `"rocm": "unknown"` — worth checking by hand on a machine with +a partial install, since the two are easy to merge by accident and a counter +cannot tell them apart afterwards. From 410bd95c16df5e5259ca333aaa975cbbf067bc8a Mon Sep 17 00:00:00 2001 From: Eugene Volen Date: Wed, 30 Sep 2026 08:19:30 +0000 Subject: [PATCH 3/5] fix(diagnose): read a refusal as a refusal, not as an unreadable report A refusal envelope carries the schema version and none of the report's fields, so the reader accepted the schema, failed to deserialize the body, and answered that the report could not be read. A machine declining to describe itself and a reader that cannot parse what it was given are opposite facts: the first is the disclosure guard working, the second is a fault. Counted as the same thing, a guard firing on every machine it was meant to fire on is indistinguishable from a reader that is simply broken, and the refusals are the only trace the guard leaves. The reader now checks for the refusal marker before the body and answers with the rule that refused. The markers were literals at the point of printing, so a reader or a test agreed with what the CLI writes only by coincidence. They move next to the refusals themselves, spelled out arm by arm because the vocabulary is part of the schema and a renamed variant must not silently rename a marker that reports already written were carrying. Tests iterate the full set, so a third refusal cannot be added without the reader being made to consider it, and pin the two strings as literals: a test that derives the marker from the function under test stays green when the two arms are swapped, which is exactly the mistake that would mislabel every refusal in the record. Review follow-up: `engine_and_version` special-cased `framework.is_empty()` as "no engine found", a branch `examine.rs` never actually produces -- the struct default is the literal string "unknown", so the ordinary "nothing installed" case fell through the allowlist check and was mislabelled `unknown` (unreadable) instead of `none` (absent). Fixed by checking against that default directly, and added tests pinning both the ordinary absence and a skipped probe (which must stay `unknown`, since a probe that never ran cannot say an engine is absent). The `entry`/`fix_offered` test only checked `entry`, leaving the paired invariant -- a fix cannot be offered for a cause the catalog did not establish -- unpinned. Expanded it to assert `fix_offered` survives for a recognised entry and is overruled for a forged one. `read_report` accepted a `"refused"` marker outside `Refusal::ALL` verbatim, the same free-text leak this module exists to refuse elsewhere for a forged or corrupted envelope. Now checked against the schema-gated vocabulary, with a test proving an unrecognised marker reads as unread rather than as a genuine refusal. Added tests pinning two previously-unpinned branches: a missing `/etc/os-release` reads as `unknown` rather than `other`, and `major_minor`'s fallback paths (a bare major, and a dash-delimited build string) both still group. Updated `docs/testing.md` and `README.md` to state the `none`/`unknown` distinction for `engine`/ `engine_version` the same way it already did for `rocm`, and documented `--report`'s mutual exclusivity with `--distro` in `apps/rocm/src/main.rs`. Signed-off-by: Eugene Volen --- README.md | 19 +- apps/rocm/src/main.rs | 19 +- crates/rocm-core/src/lib.rs | 1 + crates/rocm-core/src/report.rs | 307 ++++++++++++++++++++++++++++++++- docs/testing.md | 20 ++- 5 files changed, 333 insertions(+), 33 deletions(-) diff --git a/README.md b/README.md index ccffe8288..2844caa84 100644 --- a/README.md +++ b/README.md @@ -286,14 +286,17 @@ fix` takes the id, not the position. 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. The content is deliberately narrow (a schema version, the matched entry, whether - a fix was offered for it, the GPU architecture, the OS family, distribution - and major version, the ROCm release, the inference engine and its release, - the CLI version), and it carries no host name, user name, file path, or - error text. Every version is cut back to a release, so a build number that - would narrow toward one machine never appears, and the distribution is - checked against a list of known names rather than repeated from the - machine. Hardware that is not on AMD's published compatibility - matrix produces no report at all, and the CLI says why. + a fix was offered for it, the GPU architecture and which compatibility + matrix snapshot it was checked against, the OS family, distribution and + major version, the ROCm release, the inference engine and its release, the + CLI version), and it carries no host name, user name, file path, or error + text. The ROCm release and the inference engine's release are each cut + back to a release, so a build number that would narrow toward one machine + never appears there; the CLI's own version is the exception, since it names + the tool that wrote the report rather than something read off the machine. + The distribution is checked against a list of known names rather than + repeated from the machine. Hardware that is not on AMD's published + compatibility matrix produces no report at all, and the CLI says why. `fix` applies a known fix by the `id:` that `diagnose` reported — not the ranking position noted above, which isn't a stable name. Run it with no id diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 7c508bc93..ef9be4b06 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -158,6 +158,11 @@ enum Command { /// Nothing leaves the machine: this prints the exact content so it can /// be read before any of it is shared. Hardware that is not on AMD's /// published compatibility matrix produces no report at all. + /// + /// Not combinable with `--distro`: a report describes this machine, and + /// a WSL distribution reached remotely is not fully examined (see + /// `--distro`'s own help), so it cannot back the disclosure guard's + /// architecture check. #[arg(long, conflicts_with = "distro")] report: bool, }, @@ -2865,18 +2870,12 @@ fn show_prepared_report( } }; if json { - let marker = match refusal { - rocm_core::ReportRefusal::UnreleasedHardware => "unreleased-hardware", - rocm_core::ReportRefusal::ArchitectureUnreadable => "architecture-unreadable", - }; println!( "{}", - serde_json::to_string_pretty(&serde_json::json!({ - "schema": rocm_core::REPORT_SCHEMA_VERSION, - "refused": marker, - "explanation": explanation, - "architecture_matrix": rocm_core::APPROVED_ARCHITECTURES_SOURCE, - }))? + serde_json::to_string_pretty(&rocm_core::refusal_envelope( + refusal, + explanation + ))? ); } else { println!("{explanation}"); diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index fa534aa97..80ccf01ec 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -59,6 +59,7 @@ pub use proc_lifecycle::{ pub use report::{ APPROVED_ARCHITECTURES, APPROVED_ARCHITECTURES_SOURCE, REPORT_SCHEMA_VERSION, ReadOutcome, Refusal as ReportRefusal, Report, is_rocm_supported, prepare_report, read_report, + refusal_envelope, }; use runtime::env_path_override; pub use runtime::{ diff --git a/crates/rocm-core/src/report.rs b/crates/rocm-core/src/report.rs index 54af7b937..d400e8580 100644 --- a/crates/rocm-core/src/report.rs +++ b/crates/rocm-core/src/report.rs @@ -118,7 +118,7 @@ pub const APPROVED_ENGINES: &[&str] = &["pytorch", "llama-cpp"]; /// Named rather than a bare `None`: Doctor has to explain the refusal, and /// "hardware we cannot identify" and "hardware that is not released" call for /// different sentences. -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Refusal { /// A GPU on this machine is not on the ROCm compatibility matrix. /// @@ -131,6 +131,27 @@ pub enum Refusal { ArchitectureUnreadable, } +impl Refusal { + /// The marker a written refusal carries, and the value + /// [`ReadOutcome::Refused`] hands back. + /// + /// Here rather than at the point of printing, so that the writer, the + /// reader and the tests all name the same constant. Spelled out arm by arm + /// rather than derived, because this vocabulary is part of the schema: a + /// renamed variant must not silently rename a marker that reports already + /// in the field were written with. + #[must_use] + pub const fn marker(self) -> &'static str { + match self { + Self::UnreleasedHardware => "unreleased-hardware", + Self::ArchitectureUnreadable => "architecture-unreadable", + } + } + + /// Every refusal, so a reader or a test cannot cover fewer than exist. + pub const ALL: &'static [Self] = &[Self::UnreleasedHardware, Self::ArchitectureUnreadable]; +} + /// A report, as it would be published. /// /// Every field is here because it was agreed, not because it was available. @@ -183,6 +204,20 @@ pub const UNRECOGNISED: &str = "unrecognised"; #[derive(Debug, Clone, PartialEq, Eq)] pub enum ReadOutcome { Understood(Box), + /// The machine declined to describe itself, and named the rule that + /// declined. + /// + /// A refusal is evidence, not a gap. It is the only trace the disclosure + /// guard leaves, so counting refusals is how anybody learns whether the + /// guard fires on the machines it was meant to fire on. Merged into + /// [`ReadOutcome::Unread`] it would instead read as a reader fault, and + /// the guard working would be indistinguishable from the reader broken. + Refused { + /// The marker the writer used, e.g. `"unreleased-hardware"`. The + /// schema version gates this vocabulary: a reader that accepted the + /// schema has accepted the set of markers that go with it. + reason: String, + }, /// The report is written to an agreement this reader does not know. /// /// Distinct from an empty report on purpose. A counter that read this as @@ -363,12 +398,26 @@ fn rocm_release(examination: &Examination) -> String { /// against [`APPROVED_ENGINES`] to catch a future probe rather than today's. /// `framework_version` is parsed out of an installed package, so it is /// truncated like every other version here. +/// +/// `"unknown"` is `Examination`'s struct default for this field +/// (`examine.rs`), and a completed probe leaves it in place when neither +/// engine was found, so it maps to [`NONE`] rather than [`UNKNOWN`]: the +/// ordinary "no engine installed" case is an absence, not a case where this +/// build looked and could not tell (the vocabulary [`UNKNOWN`] and [`NONE`] +/// document, which `docs/testing.md`'s report-fields section also states). +/// This mapping presumes the caller always ran a probe before building a +/// report -- an `Examination` built some other way, whose `framework` was +/// simply never touched, would be indistinguishable from that ordinary case +/// and would also read as absent here. +/// +/// `"skipped"` is what `examine.rs` records when +/// [`crate::examine::FrameworkProbe::Skip`] was requested and the probe never +/// ran at all. Unlike the default, that is not +/// "looked and found nothing", so it is left to fall through the allowlist +/// check below rather than special-cased, and comes back [`UNKNOWN`]. fn engine_and_version(examination: &Examination) -> (String, String) { let name = examination.framework.trim().to_ascii_lowercase(); - // `"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. - // Neither is an engine, and neither may carry a version. - if name.is_empty() { + if name == UNKNOWN { return (NONE.to_owned(), NONE.to_owned()); } if !APPROVED_ENGINES.contains(&name.as_str()) { @@ -437,6 +486,22 @@ fn windows_os_major(ver_banner: &str) -> String { }) } +/// The JSON envelope `rocm diagnose --report --json` prints for a refusal. +/// +/// Built here rather than at the call site, so the writer and this module's +/// own reader tests construct the identical shape from the identical +/// function. A hand-rolled duplicate literal at either end can drift from +/// what the other actually produces and stay green; calling this cannot. +#[must_use] +pub fn refusal_envelope(refusal: Refusal, explanation: &str) -> serde_json::Value { + serde_json::json!({ + "schema": REPORT_SCHEMA_VERSION, + "refused": refusal.marker(), + "explanation": explanation, + "architecture_matrix": APPROVED_ARCHITECTURES_SOURCE, + }) +} + /// Read a report written by some version of this CLI. #[must_use] pub fn read_report(json: &str) -> ReadOutcome { @@ -454,6 +519,28 @@ pub fn read_report(json: &str) -> ReadOutcome { if schema_seen != REPORT_SCHEMA_VERSION { return ReadOutcome::Unread { schema_seen }; } + // Before the body, because a refusal envelope carries the schema and none + // of the report's fields, so deserializing first would fail and report a + // deliberate refusal as a reader fault. + // + // Checked against `Refusal::ALL` rather than accepted verbatim: the schema + // version gates this vocabulary, so a marker outside it cannot have been + // written by any version of this CLI that speaks this schema. Accepting it + // anyway would carry whatever a forged or corrupted envelope put there + // through to a reason field a counter treats as trusted vocabulary, which + // is the same free-text leak this whole module exists to refuse elsewhere. + if let Some(reason) = envelope.get("refused").and_then(serde_json::Value::as_str) { + return if Refusal::ALL + .iter() + .any(|refusal| refusal.marker() == reason) + { + ReadOutcome::Refused { + reason: reason.to_owned(), + } + } else { + ReadOutcome::Unread { schema_seen } + }; + } serde_json::from_value::(envelope) .map_or(ReadOutcome::Unread { schema_seen }, |report| { ReadOutcome::Understood(Box::new(report)) @@ -734,6 +821,145 @@ mod tests { ); } + /// A missing `/etc/os-release` reads as unreadable, not as an unrecognised + /// distribution. + /// + /// Distinct from [`DISTRO_OTHER`] on purpose: "the file was missing" and + /// "the file named something this build does not recognise" are different + /// facts, and only the first is this build never getting to decide. + #[test] + fn a_missing_os_release_reads_as_unknown_not_as_an_unrecognised_distribution() { + let mut machine = machine_of_sentinels("gfx1100"); + machine.distro_id = String::new(); + let report = prepare_report(&machine, None, false).expect("a released machine reports"); + assert_eq!( + report.distro, UNKNOWN, + "an empty ID means the file could not be read, which is different from a file that \ + named something off the list" + ); + } + + /// A release with no minor still groups, and a release with an unusual + /// build separator still yields its release. + /// + /// Both are `major_minor`'s fallback paths: a bare major survives rather + /// than being discarded with a non-numeric minor, and every delimiter the + /// function accepts, not only `.`, is exercised at least once. + #[test] + fn a_release_with_no_minor_or_an_unusual_build_separator_still_groups() { + let mut bare_major = machine_of_sentinels("gfx1100"); + bare_major.rocm_version = "7".to_owned(); + let report = prepare_report(&bare_major, None, false).expect("a released machine reports"); + assert_eq!( + report.rocm, "7", + "a release with no minor is still a population and must not be discarded" + ); + + let mut dash_delimited = machine_of_sentinels("gfx1100"); + dash_delimited.rocm_version = "7-1-0".to_owned(); + let report = + prepare_report(&dash_delimited, None, false).expect("a released machine reports"); + assert_eq!( + report.rocm, "7.1", + "a dash-delimited build string must yield the same release as a dot-delimited one" + ); + } + + /// A refusal reads as a refusal, not as a report nobody could parse. + /// + /// Both refusals are checked, because a reader that recognised only one + /// would leave the other counted as a reader fault, and the two rules are + /// the two halves of the disclosure guard. + /// + /// The fixture is built by calling [`refusal_envelope`], the same function + /// `diagnose --report --json` calls to print one, rather than a second, + /// independent `json!` literal. The two cannot drift apart: either both + /// change together, through the one function, or neither does. + #[test] + fn a_refusal_is_read_as_a_refusal_rather_than_as_an_unreadable_report() { + for refusal in Refusal::ALL { + let marker = refusal.marker(); + let envelope = refusal_envelope(*refusal, "why no report was prepared").to_string(); + + match read_report(&envelope) { + ReadOutcome::Refused { reason } => assert_eq!( + reason, marker, + "the refusal was recognised but its rule was lost, so nothing can tell the \ + two halves of the guard apart" + ), + other => panic!( + "a refusal read as {other:?}. Counted that way, the guard firing is \ + indistinguishable from the reader failing, and the trial that exists to \ + watch the guard cannot see it." + ), + } + } + } + + /// A `"refused"` value outside the known markers is not trusted vocabulary. + /// + /// The schema version gates the marker set, so a marker this reader does + /// not recognise cannot have been written by any CLI that speaks this + /// schema -- it is forged or corrupted, and reading it as a genuine refusal + /// would carry that text through to a reason field a counter treats as + /// trusted. Found by mutation: accepting `reason` verbatim, with no check + /// against [`Refusal::ALL`], passed every other test in this file. + #[test] + fn an_unrecognised_refusal_marker_is_not_read_as_a_refusal() { + let envelope = serde_json::json!({ + "schema": REPORT_SCHEMA_VERSION, + "refused": "SENTINEL-FORGED-REFUSAL", + "explanation": "why no report was prepared", + }) + .to_string(); + + assert_eq!( + read_report(&envelope), + ReadOutcome::Unread { + schema_seen: REPORT_SCHEMA_VERSION + }, + "a marker outside the schema-gated vocabulary must not be trusted as a genuine refusal" + ); + } + + /// The refusal markers are wire vocabulary, and are pinned as literals. + /// + /// The round-trip test above cannot catch a change here: it derives the + /// value it expects from the same function it is checking, so swapping the + /// two arms keeps it green. Found by mutation. A swap would attribute + /// every "hardware not released" refusal to "architecture unreadable" and + /// the reverse, which is the precise question the trial exists to answer, + /// so these strings are written out rather than computed. + #[test] + fn the_refusal_markers_are_the_strings_already_written_into_the_field() { + assert_eq!(Refusal::UnreleasedHardware.marker(), "unreleased-hardware"); + assert_eq!( + Refusal::ArchitectureUnreadable.marker(), + "architecture-unreadable" + ); + assert_eq!( + Refusal::ALL.len(), + 2, + "a refusal was added without deciding what it is called on the wire" + ); + } + + /// A report still reads as a report. + /// + /// The paired half: a reader that answered `Refused` to everything would + /// satisfy the test above on its own. + #[test] + fn recognising_refusals_did_not_stop_reports_being_read() { + let report = prepare_report(&machine_of_sentinels("gfx1100"), None, false) + .expect("a released machine must produce a report"); + let serialized = serde_json::to_string(&report).expect("a report must serialize"); + + match read_report(&serialized) { + ReadOutcome::Understood(read_back) => assert_eq!(*read_back, report), + other => panic!("a genuine report read as {other:?}"), + } + } + /// An engine name this build does not recognise is not published. /// /// Today every value of `framework` is a literal written by `examine.rs`, @@ -763,6 +989,55 @@ mod tests { ); } + /// A machine with no engine installed reads as absent, not unreadable. + /// + /// `"unknown"` is `Examination::framework`'s struct default, and a + /// completed probe leaves it there when neither engine was found -- the + /// ordinary case for most machines. Found by mutation: the prior code + /// special-cased an empty string here, a value `examine.rs` never + /// actually writes, so every one of these tests passed against a branch + /// that could never run, while the case that does run every day fell + /// through to [`UNKNOWN`] and mislabelled an absence as unreadable. + #[test] + fn no_engine_found_by_a_completed_probe_reads_as_none_not_unknown() { + let mut machine = machine_of_sentinels("gfx1100"); + machine.framework = "unknown".to_owned(); + let report = prepare_report(&machine, None, false).expect("a released machine reports"); + + assert_eq!( + report.engine, NONE, + "the default a completed probe leaves in place means no engine was found, which is \ + an absence, not a case where this build looked and could not tell" + ); + assert_eq!( + report.engine_version, NONE, + "a version cannot describe an engine the report says is absent" + ); + } + + /// A skipped probe reads as unreadable, not absent. + /// + /// `"skipped"` means [`crate::examine::FrameworkProbe::Skip`] was + /// requested and the probe never ran at all, which is a different fact + /// from the probe running and finding nothing: this build did not look, + /// so it cannot say the engine is absent. + #[test] + fn a_skipped_probe_reads_as_unreadable_not_as_no_engine_installed() { + let mut machine = machine_of_sentinels("gfx1100"); + machine.framework = "skipped".to_owned(); + let report = prepare_report(&machine, None, false).expect("a released machine reports"); + + assert_eq!( + report.engine, UNKNOWN, + "a probe that never ran cannot report an absence; that would say a machine has no \ + engine when this build simply never looked" + ); + assert_eq!( + report.engine_version, UNKNOWN, + "a version cannot describe an engine this build never checked for" + ); + } + /// An absent ROCm and an unreadable one are different facts. /// /// `examine.rs` collapses both into an empty `rocm_version`, so without @@ -813,22 +1088,35 @@ mod tests { /// from the machine, which makes it the one free-text hole in a structure /// that is otherwise assembled field by field. A forged or mistaken id must /// not ride through to a public tracker. + /// + /// Also pins `fix_offered`, the field derived from `entry_recognised`: a + /// fix cannot be offered for a cause the catalog did not establish, so a + /// caller asking for `fix_offered: true` alongside a forged id must be + /// overruled. Both calls below pass `true`, so the paired assertion cannot + /// be satisfied by an implementation that forces the flag `false` + /// unconditionally -- the real-id case has to show the flag surviving. #[test] fn an_entry_id_the_catalog_does_not_know_is_never_published_verbatim() { // Non-vacuity: a real id has to reach the report, or "the forged one // does not" is satisfied by discarding every id. - let known = prepare_report(&machine_of_sentinels("gfx1100"), Some("fix-6-path"), false) + let known = prepare_report(&machine_of_sentinels("gfx1100"), Some("fix-6-path"), true) .expect("a released machine must produce a report"); assert_eq!( known.entry, "fix-6-path", "premise failed: a real catalog id must reach the report, otherwise the assertion \ below passes against an implementation that publishes no id at all" ); + assert!( + known.fix_offered, + "premise failed: fix_offered must survive for a recognised entry, otherwise the \ + assertion below passes against an implementation that forces the flag false \ + unconditionally" + ); let forged = prepare_report( &machine_of_sentinels("gfx1100"), Some("SENTINEL-FORGED-ENTRY"), - false, + true, ) .expect("a released machine must produce a report"); assert_eq!( @@ -836,6 +1124,11 @@ mod tests { "an id the catalog does not know is not a finding, and publishing it verbatim would \ put caller-supplied text on a public tracker" ); + assert!( + !forged.fix_offered, + "a fix cannot be offered for a cause the catalog did not establish; this would \ + publish a fix pointer next to an entry the report itself calls unrecognised" + ); } /// I4 — a reader that does not know the agreement says so, rather than diff --git a/docs/testing.md b/docs/testing.md index a1f282f0f..160eda783 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -1194,11 +1194,15 @@ On a host with an approved architecture (see `APPROVED_ARCHITECTURES` in `fix_offered`. This path has not been exercised against real hardware in CI; verifying it needs a lane whose GPU architecture is on the allowlist. -Three of those fields answer with a word rather than a value, and the words -are not interchangeable. `none` means the thing is absent, `unknown` means this -build looked and could not tell, and `other` means a distribution was named but -is not one this build recognises. A host with no ROCm installed reports -`"rocm": "none"`, while a host whose install exists but whose version could not -be read reports `"rocm": "unknown"` — worth checking by hand on a machine with -a partial install, since the two are easy to merge by accident and a counter -cannot tell them apart afterwards. +Four of those fields — `distro`, `rocm`, `engine`, and `engine_version` — can +answer with a word rather than a value, and the words are not interchangeable. +`none` means the thing is absent, `unknown` means this build looked and could +not tell, and `other` means a distribution was named but is not one this build +recognises. A host with no ROCm installed reports `"rocm": "none"`, while a +host whose install exists but whose version could not be read reports `"rocm": +"unknown"` — worth checking by hand on a machine with a partial install, since +the two are easy to merge by accident and a counter cannot tell them apart +afterwards. `engine`/`engine_version` carry the same distinction: a host a +probe found no engine on reports `"engine": "none"`, while a host whose engine +probe never ran (skipped rather than completed) reports `"engine": "unknown"`, +since a probe that never ran cannot say an engine is absent. From ac989bd04c91e9afb89671164fa3054e6d13d473 Mon Sep 17 00:00:00 2001 From: Eugene Volen Date: Wed, 30 Sep 2026 10:29:10 +0000 Subject: [PATCH 4/5] test(diagnose): prove the published fields group problems, not machines Reports are grouped by exact match on their fields, so the field set has to put one problem in one group and two problems in two. Nothing checked either half, and neither is visible by reading the field list. The failure this catches is not a wrong value. It is a field set that is too fine, giving every machine its own group so no group ever describes a problem, or too coarse, collapsing distinct problems into one. Both look correct field by field, and both make the whole reporting path worthless while every other test stays green. Eight machines with one problem, differing in kernel build, patch release, CPU, GPU marketing name, PCI address and user, must produce one group. Six machines differing in one thing the report is meant to separate on must produce six. Checked by mutation in both directions. Removing the distribution collapses Ubuntu 22 and RHEL 22 into one group, which is what the field was added for. Truncating the ROCm release to its major makes 7.0 and 7.1 one problem. Publishing the full distro release instead of its major splits the eight-machine group into eight. Each of those is caught. Worth having now rather than later: a filed report cannot be widened, so a field set that does not group is only cheap to fix before any report is collected. Signed-off-by: Eugene Volen --- crates/rocm-core/src/report.rs | 134 +++++++++++++++++++++++++++++++++ 1 file changed, 134 insertions(+) diff --git a/crates/rocm-core/src/report.rs b/crates/rocm-core/src/report.rs index d400e8580..ad9858d86 100644 --- a/crates/rocm-core/src/report.rs +++ b/crates/rocm-core/src/report.rs @@ -767,6 +767,140 @@ mod tests { ); } + /// A machine described by the things a report may carry, so a population + /// can be built out of them. + /// + /// Every argument is something the report is supposed to distinguish. The + /// fields varied *inside* this helper are the ones it is supposed to + /// ignore, and they differ on every call, so a report that leaked any of + /// them would split a group that must stay whole. + fn machine(gfx: &str, distro: (&str, &str), rocm: &str, engine: (&str, &str)) -> Examination { + use std::sync::atomic::{AtomicU32, Ordering}; + static NTH: AtomicU32 = AtomicU32::new(0); + let nth = NTH.fetch_add(1, Ordering::Relaxed); + + Examination { + os_family: "linux".to_owned(), + distro_id: distro.0.to_owned(), + // A patch component that differs per machine. Two hosts on 22.04 + // and 22.04.3 are one population, and a report that said otherwise + // would make every host its own group. + distro_version: format!("{}.{nth}", distro.1), + os_version: format!("#{nth} SMP PREEMPT_DYNAMIC Thu Jun 18 21:54:43 UTC 2026"), + rocm_path: "/opt/rocm".to_owned(), + rocm_version: format!("{rocm}.{nth}"), + framework: engine.0.to_owned(), + framework_version: format!("{}.{nth}", engine.1), + cpu_model: format!("cpu-model-{nth}"), + user_name: format!("user-{nth}"), + has_amd_gpu: true, + gpus: vec![Gpu { + name: format!("marketing-name-{nth}"), + gfx_target: gfx.to_owned(), + pci_id: format!("pci-{nth}"), + is_apu: Some(false), + is_amd: true, + }], + ..Examination::default() + } + } + + /// The description a grouper matches on: every published field except the + /// ones that describe this build rather than this machine. + fn description(report: &Report) -> String { + format!( + "{}|{}|{}-{}|{}|{}-{}", + report.entry, + report.architecture, + report.distro, + report.os_major, + report.rocm, + report.engine, + report.engine_version, + ) + } + + fn describe(machine: &Examination, entry: Option<&str>) -> String { + description(&prepare_report(machine, entry, false).expect("a released machine reports")) + } + + /// Reports are grouped by exact match on their fields, so the fields have + /// to put the same problem in one group and different problems in + /// different ones. Nothing else checks this, and neither half is visible + /// by reading the field list. + /// + /// The failure this exists to catch is not a wrong value. It is a field + /// set that is too fine, making every machine its own group, or too + /// coarse, collapsing distinct problems into one. Both look fine field by + /// field and make the whole reporting path worthless. + #[test] + fn the_published_fields_group_one_problem_together_and_two_problems_apart() { + // Same problem, different machines. Every difference here is something + // a report must not carry: a kernel build, a patch release, a CPU, a + // GPU's marketing name, a PCI address, a user. + let same: std::collections::HashSet = (0..8) + .map(|_| { + describe( + &machine("gfx942", ("ubuntu", "22"), "7.1", ("pytorch", "2.5")), + Some("fix-6-path"), + ) + }) + .collect(); + assert_eq!( + same.len(), + 1, + "eight machines with one problem produced {} groups. A field set this fine gives \ + every host its own group, and no group ever describes a problem: {same:?}", + same.len() + ); + + // Different problems. Each differs from the first in exactly one thing + // the report is supposed to separate on. + let distinct = [ + // The case that motivated the distribution field. Same family, + // same major: without the distribution these two are one group. + describe( + &machine("gfx942", ("rhel", "22"), "7.1", ("pytorch", "2.5")), + Some("fix-6-path"), + ), + describe( + &machine("gfx1100", ("ubuntu", "22"), "7.1", ("pytorch", "2.5")), + Some("fix-6-path"), + ), + describe( + &machine("gfx942", ("ubuntu", "24"), "7.1", ("pytorch", "2.5")), + Some("fix-6-path"), + ), + // A minor release apart. 7.0 and 7.1 are different problems. + describe( + &machine("gfx942", ("ubuntu", "22"), "7.0", ("pytorch", "2.5")), + Some("fix-6-path"), + ), + describe( + &machine("gfx942", ("ubuntu", "22"), "7.1", ("llama-cpp", "0.9")), + Some("fix-6-path"), + ), + describe( + &machine("gfx942", ("ubuntu", "22"), "7.1", ("pytorch", "2.5")), + None, + ), + ]; + let base = same.into_iter().next().expect("one group above"); + for (nth, other) in distinct.iter().enumerate() { + assert_ne!( + *other, base, + "difference {nth} did not change the description, so two different problems \ + land in one group and a team reading it cannot tell them apart" + ); + } + let unique: std::collections::HashSet<&String> = distinct.iter().collect(); + assert_eq!( + unique.len(), + distinct.len(), + "two different problems share a description: {distinct:?}" + ); + } + /// The grouping fields carry a population, not a machine. /// /// Asserts the published values rather than the absence of the markers. From 0097a67b4365f56e2672103add03c8e9e359a33d Mon Sep 17 00:00:00 2001 From: Eugene Volen Date: Wed, 30 Sep 2026 12:50:49 +0000 Subject: [PATCH 5/5] fix(diagnose): source each published field from the platform that fills it Two platforms were reported on using fields they never populate, and both told the user something false about their own machine. On Windows the ROCm release read `rocm_path` and `rocm_version`. `probe` calls `probe_rocm_install` on Linux and WSL only; the Windows branch calls `probe_hip_sdk_windows` instead, which fills a different pair. A Windows machine with a fully installed HIP SDK published `"rocm": "none"`, which reads as "no ROCm here" and is the opposite of true. The release is now sourced per platform, the same way the OS major version already is. On WSL the architecture read the GPU list. `examine`'s WSL arm returns before any GPU probe runs, so that list is empty there whatever the hardware is, and every WSL host took the unreadable-architecture path. The CLI told a healthy machine that no GPU architecture could be read, when nothing had looked. That is fail-closed and not a disclosure risk, but it is a false statement of fact on a supported platform, in the one command whose whole purpose is being exact about what it can and cannot say. A refusal cannot carry that, so there is a third one. "We looked and could not read it" is a finding about the machine. "We never looked" is a gap in this tool, and the message says so. The deeper fix is to probe on WSL, where the architecture is in fact reachable; until then this says what is true. Adding the variant broke the reader's exhaustive match and the marker count assertion on the first build, which is what those exist to force. The regression tests give each platform a fixture shaped the way `probe` actually leaves it, and the WSL one carries a perfectly good approved GPU on purpose: a GPU-less machine would reach the right answer for the wrong reason and pass against code that never reads `is_wsl`. Checked by mutation in both directions. A scenario comment claimed that any lane without an allowlisted GPU exercises the refusal. That was wrong for the WSL lane, which has one, so that lane recorded the guard firing correctly when it fired for an unrelated structural reason. Corrected, along with the README and the testing guide, which now name all three refusals and say why they are not interchangeable. This is the fourth field in this file sourced from a platform that does not fill it, after the OS release and the engine name. The question each new field needs asked of it is not what it is called, but which branch of `probe` writes it, and what it holds on the branches that do not. Signed-off-by: Eugene Volen --- README.md | 6 +- apps/rocm/src/main.rs | 8 + crates/rocm-core/src/report.rs | 146 +++++++++++++++++-- docs/testing.md | 15 +- tests/e2e-cucumber/features/diagnose.feature | 13 +- 5 files changed, 168 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 2844caa84..e4265e908 100644 --- a/README.md +++ b/README.md @@ -296,7 +296,11 @@ fix` takes the id, not the position. the tool that wrote the report rather than something read off the machine. The distribution is checked against a list of known names rather than repeated from the machine. Hardware that is not on AMD's published - compatibility matrix produces no report at all, and the CLI says why. + compatibility matrix produces no report at all, and the CLI says why. So + does a WSL machine, for a different reason: this CLI does not inspect the + GPU on WSL yet, so it cannot confirm the hardware is on the compatibility + matrix and says that rather than claiming the architecture could not be + read. `fix` applies a known fix by the `id:` that `diagnose` reported — not the ranking position noted above, which isn't a stable name. Run it with no id diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index ef9be4b06..7e4cf2cc1 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -2868,6 +2868,14 @@ fn show_prepared_report( "No AMD GPU architecture could be read here, so nothing confirms this \ hardware is on the ROCm compatibility matrix. No report was prepared." } + rocm_core::ReportRefusal::PlatformNotProbed => { + // Says what happened rather than dressing it as a finding + // about the machine. The earlier wording told a healthy WSL + // user their GPU could not be read, when nothing had looked. + "This CLI does not inspect the GPU on WSL yet, so it cannot confirm whether \ + this hardware is on the ROCm compatibility matrix. No report was prepared. \ + This is a gap in the tool, not a problem with the machine." + } }; if json { println!( diff --git a/crates/rocm-core/src/report.rs b/crates/rocm-core/src/report.rs index ad9858d86..bd7bbea74 100644 --- a/crates/rocm-core/src/report.rs +++ b/crates/rocm-core/src/report.rs @@ -129,6 +129,20 @@ pub enum Refusal { /// No GPU architecture could be read, so nothing confirms the hardware is /// on the compatibility matrix. Refused rather than assumed. ArchitectureUnreadable, + /// Nothing on this machine was asked about the hardware, so there is no + /// answer to refuse on. + /// + /// Separate from [`Refusal::ArchitectureUnreadable`] because the two say + /// different things and only one of them is about the machine. "We looked + /// and could not read it" is a finding. "We never looked" is a gap in this + /// tool. Reporting the second as the first tells a healthy machine it has + /// no readable GPU, which is false. + /// + /// Reached on WSL today: `examine`'s WSL arm returns before any GPU probe + /// runs, so the GPU list is empty there whatever the hardware is. The + /// deeper fix is to probe on WSL, where the architecture is in fact + /// reachable. Until then this says what is true. + PlatformNotProbed, } impl Refusal { @@ -145,11 +159,16 @@ impl Refusal { match self { Self::UnreleasedHardware => "unreleased-hardware", Self::ArchitectureUnreadable => "architecture-unreadable", + Self::PlatformNotProbed => "platform-not-probed", } } /// Every refusal, so a reader or a test cannot cover fewer than exist. - pub const ALL: &'static [Self] = &[Self::UnreleasedHardware, Self::ArchitectureUnreadable]; + pub const ALL: &'static [Self] = &[ + Self::UnreleasedHardware, + Self::ArchitectureUnreadable, + Self::PlatformNotProbed, + ]; } /// A report, as it would be published. @@ -255,6 +274,15 @@ pub fn prepare_report( .map(|g| g.gfx_target.trim()) .collect(); + // Checked before the architecture, because an empty GPU list means two + // different things and only this branch can tell them apart. `examine`'s + // WSL arm returns before any GPU probe runs, so the list there is empty + // whatever the hardware is. Reading that as "could not be read" tells a + // healthy machine something false about itself. + if examination.is_wsl { + return Err(Refusal::PlatformNotProbed); + } + // Default-deny, and this is the branch that enforces it. A machine with no // readable AMD architecture has nothing confirming its hardware is on the // compatibility matrix, and "we could not tell" is not permission. @@ -372,19 +400,36 @@ fn distro(examination: &Examination) -> String { /// The installed ROCm release, as major and minor. /// -/// `rocm_path` decides absence and `rocm_version` decides readability, because -/// `examine.rs` collapses both into an empty string: `rocm_version` is -/// `install.version.unwrap_or_default()`, so "no install was found" and "an -/// install was found whose version could not be read" arrive identical. Those -/// are different facts to anybody counting, in the same way [`ReadOutcome`] -/// keeps an unreadable report apart from an absent one. +/// Sourced per platform, for the same reason [`os_major`] is: no single +/// `Examination` field holds "the installed ROCm" on both. `probe` calls +/// `probe_rocm_install` only on Linux and WSL, which is what fills +/// `rocm_path` / `rocm_version`; the Windows branch calls +/// `probe_hip_sdk_windows` instead and fills `hip_sdk_path` / +/// `hip_sdk_version`, leaving the other pair empty. Reading only the Linux +/// pair therefore reported `none` -- "no ROCm installed" -- on a Windows +/// machine with a fully installed HIP SDK. +/// +/// Within each platform the path decides absence and the version decides +/// readability, because `examine.rs` collapses both into an empty string: +/// "no install was found" and "an install was found whose version could not +/// be read" arrive identical. Those are different facts to anybody counting, +/// in the same way [`ReadOutcome`] keeps an unreadable report apart from an +/// absent one. /// -/// The path itself is only ever read here. It is never published. +/// Neither path is ever published. They are read here to tell absence from +/// unreadability, and nothing else. fn rocm_release(examination: &Examination) -> String { - if examination.rocm_path.trim().is_empty() { + let (path, version) = match examination.os_family.as_str() { + "windows" => (&examination.hip_sdk_path, &examination.hip_sdk_version), + // Linux and WSL, the platforms `probe_rocm_install` runs on. Anything + // else reaches neither probe, so both pairs are empty and this + // correctly reports an absence. + _ => (&examination.rocm_path, &examination.rocm_version), + }; + if path.trim().is_empty() { return NONE.to_owned(); } - let release = major_minor(&examination.rocm_version); + let release = major_minor(version); if release.is_empty() { UNKNOWN.to_owned() } else { @@ -669,6 +714,38 @@ mod tests { ); } + /// A machine nobody asked about is told so, not told its GPU is unreadable. + /// + /// `examine`'s WSL arm returns before any GPU probe runs, so the GPU list + /// is empty there whatever the hardware is. Reading that as "could not be + /// read" told a healthy WSL machine something false about itself, in the + /// one command whose purpose is to be exact about what it can say. + /// + /// The fixture carries a perfectly good approved GPU on purpose. A machine + /// with no GPU would reach the right answer for the wrong reason, and the + /// test would pass against code that still never looked at `is_wsl`. + #[test] + fn a_wsl_machine_is_told_its_platform_was_not_inspected_not_that_its_gpu_is_unreadable() { + let mut wsl = machine_of_sentinels("gfx1100"); + wsl.is_wsl = true; + + assert_eq!( + prepare_report(&wsl, None, false), + Err(Refusal::PlatformNotProbed), + "a platform this CLI never inspects must say so, rather than report a finding \ + about hardware nothing looked at" + ); + + // The premise. Without it the assertion above is satisfied by refusing + // the same machine for the old reason, or by refusing everything. + let mut bare_metal = wsl; + bare_metal.is_wsl = false; + assert!( + prepare_report(&bare_metal, None, false).is_ok(), + "premise failed: the same machine off WSL has an approved GPU and must report" + ); + } + /// I2 — no report carries a value the machine did not agree to publish. /// /// Sweeps the serialized bytes rather than enumerating field names, because @@ -1071,9 +1148,10 @@ mod tests { Refusal::ArchitectureUnreadable.marker(), "architecture-unreadable" ); + assert_eq!(Refusal::PlatformNotProbed.marker(), "platform-not-probed"); assert_eq!( Refusal::ALL.len(), - 2, + 3, "a refusal was added without deciding what it is called on the wire" ); } @@ -1198,6 +1276,52 @@ mod tests { ); } + /// A Windows machine's ROCm comes from the fields Windows actually fills. + /// + /// `probe` calls `probe_rocm_install` on Linux and WSL only; the Windows + /// branch calls `probe_hip_sdk_windows`, which fills a different pair and + /// leaves `rocm_path` / `rocm_version` empty. Reading only the Linux pair + /// reported `none` on a Windows host with a fully installed SDK, which + /// reads as "no ROCm here" and is the opposite of true. + /// + /// The fixture is built the way `probe`'s Windows branch leaves an + /// examination -- the Linux pair empty, the HIP pair filled -- so it + /// cannot pass against a shape that platform never produces. + #[test] + fn a_windows_machine_reports_the_hip_sdk_release_rather_than_no_rocm() { + let mut windows = machine_of_sentinels("gfx1100"); + windows.os_family = "windows".to_owned(); + windows.os_version = "Microsoft Windows [Version 10.0.22631.4460]".to_owned(); + // What `probe_hip_sdk_windows` fills. + windows.hip_sdk_path = "C:/SENTINEL-PATH/hip".to_owned(); + windows.hip_sdk_version = "6.2.4".to_owned(); + // What it does not: the Linux probe never runs on this platform. + windows.rocm_path = String::new(); + windows.rocm_version = String::new(); + + let report = prepare_report(&windows, None, false).expect("a released machine reports"); + assert_eq!( + report.rocm, "6.2", + "a Windows host with an installed SDK must report its release, not an absence" + ); + + // Absence still reads as absence on this platform, so the fix did not + // buy the version by making `none` unreachable. + let mut bare = windows.clone(); + bare.hip_sdk_path = String::new(); + bare.hip_sdk_version = String::new(); + let bare = prepare_report(&bare, None, false).expect("a released machine reports"); + assert_eq!(bare.rocm, NONE); + + // And an install whose version cannot be read is still distinguishable + // from one that is not there. + let mut unreadable = windows; + unreadable.hip_sdk_version = String::new(); + let unreadable = + prepare_report(&unreadable, None, false).expect("a released machine reports"); + assert_eq!(unreadable.rocm, UNKNOWN); + } + /// I6, continued — a non-numeric source is refused rather than /// published, regardless of platform. This is the guard that keeps the /// leak closed even if a future edit changes the source again: whatever diff --git a/docs/testing.md b/docs/testing.md index 160eda783..e27d573c6 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -1182,10 +1182,17 @@ rocm diagnose --report rocm diagnose --report --json ``` -The command refuses rather than prepares a report on two hosts: one with an -architecture the ROCm compatibility matrix does not list as supported, and -one whose AMD GPU architecture could not be read at all. Both refusals exit 0 -and are distinguishable from `--json`'s `refused` field. +The command refuses rather than prepares a report on three hosts, and the +three reasons are not interchangeable. One holds an architecture the ROCm +compatibility matrix does not list as supported (`unreleased-hardware`). One +has an AMD GPU architecture that could not be read (`architecture-unreadable`). +The third is any WSL host (`platform-not-probed`): `examine` returns before +any GPU probe runs there, so nothing has looked, and saying the architecture +could not be read would state a finding about hardware nothing inspected. All +three exit 0 and are told apart by `--json`'s `refused` field, and the refusal +envelope also carries `architecture_matrix`, the same compatibility-matrix +snapshot stamp a genuine report carries, so a refusal is just as traceable to +a matrix revision as a report is. On a host with an approved architecture (see `APPROVED_ARCHITECTURES` in `crates/rocm-core/src/report.rs`), the command prints the full `Report`: diff --git a/tests/e2e-cucumber/features/diagnose.feature b/tests/e2e-cucumber/features/diagnose.feature index 9d1488944..fa1ffac68 100644 --- a/tests/e2e-cucumber/features/diagnose.feature +++ b/tests/e2e-cucumber/features/diagnose.feature @@ -295,11 +295,16 @@ Feature: Diagnosing failures and listing fixes # can see exactly what would be published, and that asking produces either a # report or a stated refusal and never a silent send. # - # Host-independent on purpose, and the two halves land on different lanes. A + # Host-independent on purpose, and the branches land on different lanes. A # lane with an AMD GPU on the compatibility matrix exercises the prepared - # report; a lane without one exercises the refusal, which is the case the mock - # lane actually has. Written so that whichever branch a lane reaches is a real - # assertion rather than a skip. + # report; a lane without one exercises the unreadable-architecture refusal, + # which is the case the mock lane actually has. The WSL lane reaches neither: + # `examine` returns before any GPU probe there, so it refuses because the + # platform was never inspected, whatever hardware it holds. Saying "a lane + # without an allowlisted GPU exercises the refusal" would be wrong for that + # lane, and would record the guard as firing correctly when it fired for an + # unrelated structural reason. Written so that whichever branch a lane + # reaches is a real assertion rather than a skip. @id:diagnose-report-is-shown-and-not-sent Scenario: diagnose-21 - Asking what a report would say shows it and sends nothing When the user asks the CLI what a report would carry