diff --git a/README.md b/README.md index 6c4135b78..2e9c2ede3 100644 --- a/README.md +++ b/README.md @@ -228,6 +228,7 @@ form works depends on the engine your GPU selects. | `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 --model ` | Say whether a model will run here, before downloading it | +| `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 | @@ -261,7 +262,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 diagnose --model [--json] rocm fix [] [--yes] [--dry-run] [--device-index N] ``` @@ -283,6 +284,37 @@ 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 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. 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. +- `--send`, which requires `--report`, additionally offers a prefilled mail + carrying that report. It still sends nothing: the mail opens already filled + in with the content `--report` just printed, addressed to `ROCmCLI@amd.com`, + and it leaves the machine only when you send it yourself. Requiring + `--report` is what guarantees the content is shown before the mail is + offered. A mail client opens only when you asked and the machine looks like + a desktop you are at; over SSH, with no display, or with `ROCM_NO_BROWSER` + set, the address and the link are printed instead, which is also what + happens on a machine with no mail client. It is not combinable with + `--json`, which exists for scripts, and a script is not a person who can + read a mail before sending it. Note that a mail carries your address, which + the report itself does not. `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/comfyui.rs b/apps/rocm/src/comfyui.rs index 24ab029ee..14ac0f625 100644 --- a/apps/rocm/src/comfyui.rs +++ b/apps/rocm/src/comfyui.rs @@ -6,6 +6,7 @@ use crate::cli_progress::AnimatedSpinner; use crate::{format_structured_tool_call, runtime_usability_status, therock}; use anyhow::{Context, Result, bail}; use flate2::read::GzDecoder; +use rocm_core::browser::{Opener, SystemOpener}; use rocm_core::{ AppPaths, RocmCliConfig, download_file_to_path_with_progress, ensure_uv_binary, format_http_base_url, runtime_is_linux, runtime_is_windows, runtime_path_for_windows_child, @@ -474,7 +475,7 @@ pub(crate) fn start(paths: &AppPaths, options: ComfyUiStartOptions) -> Result "opened".to_owned(), Err(error) => format!("not opened ({error})"), } @@ -1938,37 +1939,6 @@ fn child_path_string(path: &Path) -> String { } } -fn open_browser(url: &str) -> Result<()> { - let status = if runtime_is_windows() { - Command::new("cmd") - .args(["/C", "start", "", url]) - .stdin(Stdio::null()) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status() - } else if cfg!(target_os = "macos") && !runtime_is_linux() { - Command::new("open") - .arg(url) - .stdin(Stdio::null()) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status() - } else { - Command::new("xdg-open") - .arg(url) - .stdin(Stdio::null()) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status() - } - .context("failed to open browser")?; - if status.success() { - Ok(()) - } else { - bail!("browser opener exited with status {status}") - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 02edda207..8f2a34055 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -28,6 +28,7 @@ use crate::uninstall::uninstall; use anyhow::{Context, Result, bail}; use clap::{CommandFactory, FromArgMatches, Parser, Subcommand, ValueEnum}; +use rocm_core::browser::Opener; use rocm_core::model_readiness::{ AcceleratorMemory, HostEngineChoice, HostFacts, ModelCatalogSource, ModelReadiness, }; @@ -169,6 +170,30 @@ enum Command { /// today, and one added now would be ambiguous against `--symptom`. #[arg(long, value_name = "MODEL")] model: 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. + /// + /// 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, + /// Also offer the prefilled issue form, so the report can be filed. + /// + /// Still sends nothing. This opens the form with the same content + /// `--report` printed, already filled in; it reaches the tracker only + /// when you submit it yourself. On a machine with no desktop, or one + /// reached over SSH, the link is printed instead of opened. + /// + /// Requires `--report`, so the content is always shown before the + /// form is offered. Not combinable with `--json`, which is for + /// scripts, and a script is not a person who can read a form. + #[arg(long, requires = "report", conflicts_with = "json")] + send: bool, }, /// Apply a known fix by id (see `rocm diagnose`); run with no id to list fixes. /// @@ -2098,7 +2123,9 @@ fn dispatch(cli: Cli) -> Result<()> { json, distro, model, - }) => diagnose(symptom, top, json, distro, model), + report, + send, + }) => diagnose(symptom, top, json, distro, model, report, send), // 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 @@ -2775,6 +2802,8 @@ fn diagnose( json: bool, distro: Option, model: Option, + report_requested: bool, + send: bool, ) -> Result<()> { // The model verdict is about THIS machine, always. `--distro` retargets the // environment examination at another one, but the GPU memory and engine @@ -2832,6 +2861,9 @@ fn diagnose( if let Some(model_ref) = &model { report.model = Some(assess_model_on_this_host(model_ref, &examination)); } + if report_requested { + return show_prepared_report(&examination, &report, json, send); + } if json { println!("{}", serde_json::to_string_pretty(&report)?); } else { @@ -3081,6 +3113,136 @@ fn unsupported_here_for(engine: &str, ruled_out: bool) -> Option { .then(|| format!("{engine} has no adapter on native Windows; serve it from WSL or Linux")) } +/// The catalog entry a report should name, and whether a fix was offered for it. +/// +/// Reads `has_match` rather than taking the head of `matched`. Several checkers +/// open with a nonzero score for a situation that is merely *potentially* +/// relevant, so `matched` is rarely empty even on a healthy machine — taking its +/// head regardless would publish a sub-threshold signal as though it were an +/// established cause, and the counts built on those reports would be wrong in a +/// way nothing downstream could detect. +fn established_entry(report: &rocm_core::DiagnoseReport) -> (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()) + }) +} + +/// Act on a delivery decision, and say what happened. +/// +/// Takes the decision rather than making it, and takes the opener rather than +/// being one. Both for the same reason: the decision is tested in `rocm-core` +/// against every environment, and this half has to be tested against an opener +/// that does not exist, on a machine with no browser. A function that decided +/// and opened could be verified on neither. +fn perform_delivery( + delivery: &rocm_core::report_delivery::Delivery, + opener: &dyn Opener, +) -> String { + use rocm_core::report_delivery::{DESTINATION, Delivery}; + match delivery { + // No mail client is started here on purpose, and the reason is worth + // the line: this is the branch for a machine held over SSH, or a + // server with no mail client at all, where starting one would open on + // somebody else's desktop or fail silently. The address is named as + // well as the link, because a machine in this state often cannot act + // on a `mailto:` at all and the user has to send the mail by hand. + Delivery::Show(url) => format!( + "Nothing has been sent. To send this yourself, mail the report above to \ + {DESTINATION}, or open:\n {url}" + ), + Delivery::Open(url) => match opener.open(url) { + Ok(()) => format!( + "Nothing has been sent yet. A prefilled mail to {DESTINATION} was opened, and \ + it is sent only when you send it:\n {url}" + ), + // A failed open is not a failed command. The user still has the + // address and the link, which is the whole of what this offers. + Err(error) => format!( + "Nothing has been sent. A mail client could not be started ({error}). To send \ + this yourself, mail the report above to {DESTINATION}, or open:\n {url}" + ), + }, + } +} + +/// Print the report this machine would contribute, and send nothing. +fn show_prepared_report( + examination: &rocm_core::Examination, + report: &rocm_core::DiagnoseReport, + json: bool, + send: 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!(); + // The content is printed above before this decides anything, + // so a report is always read before its form is offered. That + // ordering is the promise `--send` makes, and `--send` + // requires `--report` so it cannot be skipped. + let delivery = rocm_core::report_delivery::deliver(&prepared, send, &|key| { + std::env::var(key).ok() + }); + println!( + "{}", + perform_delivery(&delivery, &rocm_core::browser::SystemOpener) + ); + } + 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." + } + 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!( + "{}", + serde_json::to_string_pretty(&rocm_core::refusal_envelope( + refusal, + explanation + ))? + ); + } 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()); @@ -22551,6 +22713,173 @@ fn treat_as_natural_language(args: &[String]) -> bool { #[cfg(test)] mod tests { + use std::cell::RefCell; + + use rocm_core::browser::Opener; + use rocm_core::report_delivery::Delivery; + + use super::perform_delivery; + + /// An opener that records rather than opens, and can be told to fail. + /// + /// The whole reason the opener is a trait: the real one spawns a browser + /// against whatever desktop exists, so neither "it was opened" nor "it was + /// deliberately not opened" can be observed in CI without this. + struct RecordingOpener { + opened: RefCell>, + fails: bool, + } + + impl RecordingOpener { + fn working() -> Self { + Self { + opened: RefCell::new(Vec::new()), + fails: false, + } + } + fn broken() -> Self { + Self { + opened: RefCell::new(Vec::new()), + fails: true, + } + } + fn opened(&self) -> Vec { + self.opened.borrow().clone() + } + } + + impl Opener for RecordingOpener { + fn open(&self, url: &str) -> anyhow::Result<()> { + self.opened.borrow_mut().push(url.to_owned()); + if self.fails { + anyhow::bail!("no browser here"); + } + Ok(()) + } + } + + /// Nothing is opened unless the decision was to open. + /// + /// The assertion that matters is on the opener, not on the wording. A + /// message saying no browser was started is satisfied by any string; an + /// opener that recorded nothing is the actual claim. + #[test] + fn a_delivery_that_is_not_an_open_never_reaches_the_browser() { + let delivery = Delivery::Show("mailto:nobody@example.invalid".to_owned()); + let opener = RecordingOpener::working(); + let said = perform_delivery(&delivery, &opener); + + assert!( + opener.opened().is_empty(), + "a mail client was started for {delivery:?}, which is the one thing this path must \ + not do on a machine the user is holding over SSH" + ); + assert!( + said.contains("Nothing has been sent"), + "the user has to be told nothing left the machine: {said}" + ); + // A machine in this state often cannot act on a `mailto:` at all, so + // the address has to be readable on its own, not only inside the link. + assert!( + said.contains(rocm_core::report_delivery::DESTINATION), + "a user who has to send the mail by hand needs the address: {said}" + ); + } + + /// Opening is what an open decision does, and the user is told it is not + /// filed yet. + #[test] + fn an_open_decision_reaches_the_browser_and_is_still_not_a_send() { + let url = "https://example.invalid/new?body=x"; + let opener = RecordingOpener::working(); + let said = perform_delivery(&Delivery::Open(url.to_owned()), &opener); + + assert_eq!( + opener.opened(), + vec![url.to_owned()], + "premise failed: an open decision must reach the opener, otherwise the cases above \ + are satisfied by never opening anything" + ); + assert!( + said.contains("only when you send it"), + "opening a prefilled mail is not sending it, and the user has to know which one \ + happened: {said}" + ); + } + + /// A browser that will not start still leaves the user the link. + #[test] + fn a_browser_that_fails_to_start_still_hands_the_user_the_link() { + let url = "https://example.invalid/new?body=x"; + let said = perform_delivery(&Delivery::Open(url.to_owned()), &RecordingOpener::broken()); + + assert!( + said.contains(url), + "the link is the whole of what this offers, so a failed browser must not lose it: \ + {said}" + ); + assert!(said.contains("Nothing has been sent")); + } + + /// 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, + model: 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/browser.rs b/crates/rocm-core/src/browser.rs new file mode 100644 index 000000000..6be0a5549 --- /dev/null +++ b/crates/rocm-core/src/browser.rs @@ -0,0 +1,67 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! Handing a URL to the user's browser. +//! +//! Behind a trait because starting a browser is the one thing in this area +//! that cannot run in CI: the real implementation spawns a process against +//! whatever desktop the machine has, so any code path that opens a URL is +//! untestable unless the opening itself can be replaced. Callers take +//! `&dyn Opener`, tests pass a fake, and the decision to open stays separate +//! from the opening. +//! +//! One implementation, deliberately. A second opener somewhere else is how the +//! seam stops being a seam. + +use std::process::{Command, Stdio}; + +use anyhow::{Context, Result, bail}; + +use crate::{runtime_is_linux, runtime_is_windows}; + +/// Something that can show the user a URL. +pub trait Opener { + /// Hand `url` to the user's browser. + /// + /// # Errors + /// When no browser could be started, or the opener reported failure. + fn open(&self, url: &str) -> Result<()>; +} + +/// The real one: whatever this platform uses to open a link. +#[derive(Debug, Clone, Copy, Default)] +pub struct SystemOpener; + +impl Opener for SystemOpener { + fn open(&self, url: &str) -> Result<()> { + let status = if runtime_is_windows() { + Command::new("cmd") + .args(["/C", "start", "", url]) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status() + } else if cfg!(target_os = "macos") && !runtime_is_linux() { + Command::new("open") + .arg(url) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status() + } else { + Command::new("xdg-open") + .arg(url) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status() + } + .context("failed to open browser")?; + if status.success() { + Ok(()) + } else { + bail!("browser opener exited with status {status}") + } + } +} diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index 7577ababc..ea443f3cf 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -874,6 +874,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() +} + /// What the catalog says `rocm fix ` does on `os`. /// /// `None` when the id is not in the catalog, or when it is but does not apply diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index 86b1a7cd7..823dffea2 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -27,6 +27,7 @@ use windows_sys::Win32::System::Threading::{ WaitForSingleObject, }; +pub mod browser; pub mod diagnose; pub mod disk_space; pub mod examine; @@ -34,6 +35,8 @@ pub mod fix; pub mod model_readiness; pub mod openmpi; pub mod proc_lifecycle; +pub mod report; +pub mod report_delivery; pub mod runtime; #[cfg(test)] mod test_env; @@ -56,6 +59,11 @@ 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, + refusal_envelope, +}; 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..bd7bbea74 --- /dev/null +++ b/crates/rocm-core/src/report.rs @@ -0,0 +1,1419 @@ +// 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", +]; + +/// 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 +/// "hardware we cannot identify" and "hardware that is not released" call for +/// different sentences. +#[derive(Debug, Clone, Copy, 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, + /// 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 { + /// 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", + 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, + Self::PlatformNotProbed, + ]; +} + +/// 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, + /// 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, +} + +/// 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 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 + /// "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(); + + // 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. + 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; + // 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, + // 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), + distro: distro(examination), + rocm: rocm_release(examination), + engine, + engine_version, + 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 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. +/// +/// 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. +/// +/// Neither path is ever published. They are read here to tell absence from +/// unreadability, and nothing else. +fn rocm_release(examination: &Examination) -> String { + 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(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. +/// +/// `"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(); + if name == UNKNOWN { + 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. +/// +/// 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(']')) + }) +} + +/// 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 { + // 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 }; + } + // 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)) + }) +} + +#[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(), + // 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(), + 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", + "SENTINEL-ROCM-BUILD", + "SENTINEL-ENGINE-BUILD", + ]; + + /// 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" + ); + } + + /// 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 + /// 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}" + ); + } + + /// 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. + /// 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}" + ); + } + + /// 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::PlatformNotProbed.marker(), "platform-not-probed"); + assert_eq!( + Refusal::ALL.len(), + 3, + "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`, + /// 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}" + ); + } + + /// 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 + /// 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" + ); + } + + /// 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 + /// 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. + /// + /// 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"), 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"), + true, + ) + .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" + ); + 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 + /// 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/crates/rocm-core/src/report_delivery.rs b/crates/rocm-core/src/report_delivery.rs new file mode 100644 index 000000000..6f620e93f --- /dev/null +++ b/crates/rocm-core/src/report_delivery.rs @@ -0,0 +1,491 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! How a report leaves the machine, which is only ever by the user's own act. +//! +//! This module builds a link and decides whether a browser may be started. It +//! never sends anything, and there is deliberately no code here that could: +//! no HTTP client, no token, nothing to authenticate with. A report reaches a +//! tracker because a person read it and pressed a button. +//! +//! Kept apart from [`crate::report`], which decides what a report *contains*. +//! That module is pure by design and says so; this one is where the outside +//! world starts, so the boundary is worth keeping visible. + +use crate::report::{Report, UNRECOGNISED}; + +/// Where a report is sent. +/// +/// Fixed in code rather than configurable on purpose: an address a caller can +/// choose is an address an attacker can choose, and the user would be reading a +/// report they believe is going to AMD while it goes somewhere else. +/// +/// A mailbox rather than an issue tracker. That choice costs the report its +/// anonymity, because a mail envelope carries the sender's address whatever the +/// body says, and it costs the ability to count reports, because a mailbox has +/// no query. Both are recorded where the decision was made rather than here. +pub const DESTINATION: &str = "ROCmCLI@amd.com"; + +/// The first word of every subject line, so a mail rule can route the whole set. +pub const SUBJECT_TAG: &str = "[rocm-doctor]"; + +/// What a subject says when the catalog recognised nothing. +pub const UNRECOGNISED_SUBJECT: &str = "unrecognised"; + +/// What should happen next, decided here and performed by the caller. +/// +/// A value rather than an action, so the decision can be tested without a +/// browser, a network, or a display. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Delivery { + /// Hand this to the user's mail client, then tell them what was opened. + Open(String), + /// Show this and start nothing. + /// + /// The headless case, and the honest default whenever there is doubt. A + /// link on screen costs a user one paste, while a mail client started on a + /// machine they are holding over SSH is a process they did not ask for on + /// a display that is not theirs. + /// + /// This case matters more for mail than it did for a web link. Servers, + /// lab machines and containers usually have no mail client at all, so the + /// link would fail silently rather than open anything. + Show(String), +} + +/// The subject line: a routing tag, the cause, and the coarse description. +/// +/// A mailbox has no labels, so the classification an issue would carry in +/// metadata has to live somewhere a mail rule and a human scanning an inbox can +/// both read. The subject is the only such place. +/// +/// Ordered most stable first, so a sorted inbox groups by cause and then by +/// machine. Nothing is here that the body does not already carry: a subject is +/// as public as the body, and a field that is not approved for one is not +/// approved for the other. +#[must_use] +pub fn subject_for(report: &Report) -> String { + let cause = if report.entry == UNRECOGNISED { + UNRECOGNISED_SUBJECT + } else { + report.entry.as_str() + }; + format!( + "{SUBJECT_TAG} {cause} on {} / {}-{}", + report.architecture, report.distro, report.os_major + ) +} + +/// The prefilled mail link for a report. +/// +/// The body is the report as the user was shown it. Nothing is added here: a +/// field that is not in [`Report`] has not been through the approved-field +/// check, and this is exactly the seam where "just one more useful detail" +/// would bypass it. +#[must_use] +pub fn report_url(report: &Report) -> String { + mail_to(DESTINATION, report) +} + +/// The link builder, separated from the destination so it can be exercised +/// against an address that is not the real mailbox. A test that sends to the +/// real one would be indistinguishable from a bug that does. +fn mail_to(destination: &str, report: &Report) -> String { + // `expect` rather than a fallible return: every field of `Report` is a + // `String`, a `u32` or a `bool`, so this has no failing case to handle, + // and inventing one would add a branch no test could ever reach. + let body = serde_json::to_string_pretty(report).expect("a report has no unserializable field"); + // The address is not escaped. RFC 6068 allows `@` and `.` unescaped in the + // address part, and a percent-escaped `@` there is handled poorly by some + // mail clients. This is safe because the address is a compile-time + // constant this crate owns, never a value read from a machine. The query + // values that follow are escaped, because they carry machine-derived text. + format!( + "mailto:{destination}?subject={}&body={}", + percent_encode(&subject_for(report)), + percent_encode(&body), + ) +} + +/// Whether a browser may be started for this user. +/// +/// Reads the environment rather than probing anything, and every unknown +/// answers no. Starting a browser is the one irreversible thing this module +/// can do, so it happens only where there is positive evidence of a desktop +/// the user is sitting at. +#[must_use] +pub fn may_open_browser(env: &dyn Fn(&str) -> Option) -> bool { + let set = |key: &str| env(key).is_some_and(|value| !value.trim().is_empty()); + + // A session reached over SSH belongs to a display somewhere else. Opening + // a browser here either fails or opens it on a machine the user is not + // looking at. + if set("SSH_CONNECTION") || set("SSH_CLIENT") || set("SSH_TTY") { + return false; + } + // An explicit opt-out is honoured before any positive evidence: a user who + // said no has said no. + if set("ROCM_NO_BROWSER") { + return false; + } + if cfg!(target_os = "windows") || cfg!(target_os = "macos") { + return true; + } + // On Linux a desktop is not implied by anything except a display. + set("DISPLAY") || set("WAYLAND_DISPLAY") +} + +/// Decide what to do with a report. +/// +/// `sending` is whether the user asked to be taken to a prefilled mail, rather +/// than only to read what it would say. Both conditions have to hold before a +/// mail client starts: the user asked, and this looks like a desktop they are +/// sitting at. Either one alone is not enough, and the user's is checked +/// first, because a machine that could open a mail client is not a reason to. +#[must_use] +pub fn deliver(report: &Report, sending: bool, env: &dyn Fn(&str) -> Option) -> Delivery { + choose(report_url(report), sending, env) +} + +/// The choice itself, taking the link rather than building it, so the two +/// branches can be tested without reaching the real mailbox. +fn choose(url: String, sending: bool, env: &dyn Fn(&str) -> Option) -> Delivery { + if sending && may_open_browser(env) { + Delivery::Open(url) + } else { + Delivery::Show(url) + } +} + +/// Percent-encode for a query-string value. +/// +/// Written out rather than taken from a crate: this is the only encoding this +/// binary needs, and a signed artifact is not worth a dependency for fifteen +/// lines. Everything outside the unreserved set of RFC 3986 is escaped, which +/// is stricter than necessary and wrong in no case. +fn percent_encode(value: &str) -> String { + const HEX: &[u8; 16] = b"0123456789ABCDEF"; + let mut out = String::with_capacity(value.len()); + for byte in value.bytes() { + if byte.is_ascii_alphanumeric() || matches!(byte, b'-' | b'.' | b'_' | b'~') { + out.push(byte as char); + } else { + out.push('%'); + out.push(HEX[(byte >> 4) as usize] as char); + out.push(HEX[(byte & 0x0f) as usize] as char); + } + } + out +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::examine::{Examination, Gpu}; + use crate::report::prepare_report; + + /// A machine that produces a report, so the link under test is built from + /// what the product actually emits rather than a hand-written `Report`. + fn reportable_machine() -> Examination { + Examination { + os_family: "linux".to_owned(), + distro_id: "ubuntu".to_owned(), + distro_version: "22.04".to_owned(), + os_version: "#1 SMP PREEMPT_DYNAMIC Thu Jun 18 21:54:43 UTC 2026".to_owned(), + has_amd_gpu: true, + // Markers rather than plausible values, so a leak into the subject + // or the body is visible by eye in a failure message instead of + // reading like a real machine. + user_name: "SENTINEL-USER".to_owned(), + rocm_path: "/SENTINEL-PATH/rocm".to_owned(), + cpu_model: "SENTINEL-CPU".to_owned(), + gpus: vec![Gpu { + name: "SENTINEL-MARKETING-NAME".to_owned(), + gfx_target: "gfx1100".to_owned(), + pci_id: "SENTINEL-PCI".to_owned(), + is_apu: Some(false), + is_amd: true, + }], + ..Examination::default() + } + } + + fn report_of(entry: Option<&str>) -> Report { + prepare_report(&reportable_machine(), entry, false) + .expect("a released machine must produce a report") + } + + /// No environment at all, which is the headless shape. + /// + /// Linux-only, like its two call sites: on Windows and macOS + /// [`may_open_browser`] answers yes without consulting the environment, so + /// there is no "no display" case to construct there, and an unguarded + /// helper would be dead code on those targets -- which is exactly what + /// failed the Windows lane here before this was guarded. + #[cfg(target_os = "linux")] + fn no_env() -> impl Fn(&str) -> Option { + |_| None + } + + fn env_of(pairs: &'static [(&'static str, &'static str)]) -> impl Fn(&str) -> Option { + move |key| { + pairs + .iter() + .find(|(k, _)| *k == key) + .map(|(_, v)| (*v).to_owned()) + } + } + + /// The link carries the report and nothing else. + /// + /// Compared as JSON rather than as a `Report`, which is the whole point. + /// Deserializing into `Report` discards fields the struct does not know, + /// so a body carrying an extra `"hostname"` beside the approved fields + /// round-trips to an identical `Report` and passes. Found by mutation: a + /// planted leak survived the first version of this test. Comparing the + /// parsed values keeps every key, including ones nothing agreed to. + #[test] + fn the_link_body_is_the_report_the_user_was_shown_and_nothing_more() { + let report = report_of(None); + let url = mail_to("nobody@example.invalid", &report); + + let body = query_value(&url, "body").expect("the link must carry a body"); + let carried: serde_json::Value = + serde_json::from_str(body.trim()).expect("the body must be the report as JSON"); + let approved = serde_json::to_value(&report).expect("a report must serialize"); + + assert_eq!( + carried, approved, + "the link body is not exactly the report the user approved. An added field has not \ + been through the approved-field check, and this seam is where one would be added" + ); + } + + /// The link addresses the mailbox and carries the subject. + #[test] + fn the_link_addresses_the_destination_and_carries_the_subject() { + let report = report_of(Some("fix-6-path")); + let url = report_url(&report); + + assert!( + url.starts_with(&format!("mailto:{DESTINATION}?")), + "a report has to address the mailbox it is meant for: {url}" + ); + assert_eq!( + query_value(&url, "subject").as_deref(), + Some(subject_for(&report).as_str()), + "the subject is the only classification a mailbox can route on, so it has to \ + survive the link: {url}" + ); + } + + /// The subject names the cause, because a mailbox has no labels. + /// + /// Paired, so "always says unrecognised" cannot satisfy the first half. + #[test] + fn the_subject_names_the_cause_so_a_mailbox_can_be_sorted_by_it() { + let unrecognised = subject_for(&report_of(None)); + assert!(unrecognised.starts_with(SUBJECT_TAG)); + assert!( + unrecognised.contains(UNRECOGNISED_SUBJECT), + "a report with no matched cause has to say so in the subject: {unrecognised}" + ); + + let recognised = subject_for(&report_of(Some("fix-6-path"))); + assert!(recognised.starts_with(SUBJECT_TAG)); + assert!( + recognised.contains("fix-6-path"), + "premise failed: a matched entry has to reach the subject, or the case above is \ + satisfied by never naming a cause: {recognised}" + ); + assert!( + !recognised.contains(UNRECOGNISED_SUBJECT), + "a matched entry must not also be called unrecognised: {recognised}" + ); + } + + /// The subject carries nothing the body does not. + /// + /// A subject line is as public as the body and travels further, since it + /// shows in an inbox list. Anything here that is not an approved field has + /// bypassed the field check by a side door. + #[test] + fn the_subject_carries_no_field_the_report_does_not() { + let machine = reportable_machine(); + let report = report_of(None); + let subject = subject_for(&report); + + for planted in [ + machine.user_name.as_str(), + machine.rocm_path.as_str(), + machine.cpu_model.as_str(), + machine.gpus[0].pci_id.as_str(), + machine.gpus[0].name.as_str(), + ] { + if planted.is_empty() { + continue; + } + assert!( + !subject.contains(planted), + "'{planted}' reached the subject line: {subject}" + ); + } + } + + /// A session reached over SSH is never given a browser, on any platform. + /// + /// Both refusals here are checked before the platform is consulted, so + /// they hold everywhere and are asserted unconditionally. The rules that + /// depend on a display live in the Linux-only test below, because Windows + /// and macOS have no `DISPLAY` to reason about and + /// [`may_open_browser`] treats them as a desktop outright. + #[test] + fn a_session_over_ssh_is_shown_the_link_rather_than_having_a_browser_started() { + assert!( + !may_open_browser(&env_of(&[ + ("DISPLAY", ":0"), + ("SSH_CONNECTION", "10.0.0.1 22") + ])), + "a display variable does not make an SSH session local" + ); + assert!( + !may_open_browser(&env_of(&[("DISPLAY", ":0"), ("ROCM_NO_BROWSER", "1")])), + "an explicit opt-out is not overridden by a display" + ); + + // The premise. Without it both assertions above are satisfied by a + // function that refuses everything, which would take the feature with + // it. Linux-only because it is the platform that needs evidence: see + // the test below. + #[cfg(target_os = "linux")] + assert!( + may_open_browser(&env_of(&[("DISPLAY", ":0")])), + "premise failed: a plain local display must be allowed" + ); + } + + /// On Linux, a desktop has to be evidenced, and an empty variable is not + /// evidence. + /// + /// Linux-only, and that is the point rather than a convenience. Windows + /// and macOS have no `DISPLAY`, so [`may_open_browser`] answers yes there + /// without looking at the environment at all, and asserting the Linux rule + /// on them tests nothing about either platform. The first version of this + /// was not guarded and failed the Windows lane, having passed locally. + #[cfg(target_os = "linux")] + #[test] + fn an_empty_display_variable_does_not_count_as_a_desktop_on_linux() { + assert!(!may_open_browser(&env_of(&[("DISPLAY", "")]))); + assert!(!may_open_browser(&env_of(&[("DISPLAY", " ")]))); + assert!( + !may_open_browser(&no_env()), + "no display at all is not a desktop either" + ); + + // Paired, so the three refusals above cannot be satisfied by refusing + // everything. + assert!( + may_open_browser(&env_of(&[("WAYLAND_DISPLAY", "wayland-0")])), + "premise failed: a Wayland display is evidence of a desktop" + ); + } + + /// The destination is the agreed mailbox and nothing else. + /// + /// Pinned as a literal because it is the one value in this module that + /// decides where a user's machine description goes. A typo here sends + /// every report somewhere nobody is watching, or somewhere nobody should + /// be watching, and no other test would notice. + #[test] + fn reports_address_the_agreed_mailbox() { + assert_eq!(DESTINATION, "ROCmCLI@amd.com"); + assert!( + report_url(&report_of(None)).contains(DESTINATION), + "the destination has to survive into the link the user is handed" + ); + } + + /// Reading a report is not asking to file one. + /// + /// Two conditions gate a browser, and this covers the one the machine + /// cannot tell you: the user has to have asked. A desktop is permission + /// from the environment, never from the person. Without this, adding a + /// display to a machine would change what `--report` does. + #[test] + fn a_desktop_is_not_permission_to_open_anything_the_user_did_not_ask_for() { + let url = "https://example.invalid/new".to_owned(); + let desktop = env_of(&[("DISPLAY", ":0")]); + + assert_eq!( + choose(url.clone(), false, &desktop), + Delivery::Show(url.clone()), + "a report the user only asked to read must not open a browser, whatever the \ + machine looks like" + ); + + // The premise. Without this the assertion above is satisfied by never + // opening anything, which would take the feature with it. Every + // platform reaches this: a `DISPLAY` is evidence on Linux, and + // Windows and macOS are a desktop regardless. + assert_eq!( + choose(url.clone(), true, &desktop), + Delivery::Open(url.clone()), + "premise failed: asking, on a desktop, has to open" + ); + + // The machine's half of the gate, which only Linux can express. On + // Windows and macOS there is no environment that means "not a + // desktop", so asserting this there would test nothing. + #[cfg(target_os = "linux")] + assert_eq!( + choose(url.clone(), true, &no_env()), + Delivery::Show(url.clone()), + "asking does not override a machine with no desktop" + ); + + // The user's half, which every platform can express, because an + // explicit opt-out is honoured before the platform is consulted. + assert_eq!( + choose( + url.clone(), + true, + &env_of(&[("DISPLAY", ":0"), ("ROCM_NO_BROWSER", "1")]) + ), + Delivery::Show(url), + "an opt-out has to hold on every platform, not only where a display is read" + ); + } + + /// Everything outside the unreserved set is escaped. + #[test] + fn a_value_is_escaped_so_it_cannot_end_the_query_or_start_a_new_field() { + assert_eq!(percent_encode("a b"), "a%20b"); + assert_eq!(percent_encode("a&labels=x"), "a%26labels%3Dx"); + assert_eq!(percent_encode("a#b"), "a%23b"); + assert_eq!(percent_encode("-._~"), "-._~"); + assert_eq!(percent_encode("é"), "%C3%A9"); + } + + /// Read one query-string value back out of a link, decoded. + fn query_value(url: &str, key: &str) -> Option { + let query = url.split_once('?')?.1; + let raw = query + .split('&') + .find_map(|pair| pair.strip_prefix(&format!("{key}=")))?; + let bytes = raw.as_bytes(); + let mut out = Vec::with_capacity(bytes.len()); + let mut i = 0; + while i < bytes.len() { + if bytes[i] == b'%' && i + 2 < bytes.len() { + let hex = std::str::from_utf8(&bytes[i + 1..i + 3]).ok()?; + out.push(u8::from_str_radix(hex, 16).ok()?); + i += 3; + } else { + out.push(bytes[i]); + i += 1; + } + } + String::from_utf8(out).ok() + } +} diff --git a/docs/testing.md b/docs/testing.md index b4f9de30d..8d4767506 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -1268,3 +1268,74 @@ The e2e suite (`cargo xtask e2e -- -n diagnose-2`) exercises all four verdicts, including the two that need a synthetic signed catalog to trigger deterministically (`ModelNotCurated`, `Degraded`) since no built-in recipe can produce them on an arbitrary real host. + +## 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 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. + +Offer a prefilled mail carrying that report, which still sends nothing: + +```bash +rocm diagnose --report --send +``` + +Two argument rules are worth checking by hand, because both are the kind that +only break when somebody reorders a declaration. `--send` without `--report` +must be refused, since showing the content first is the guarantee `--send` +makes. `--send` with `--json` must also be refused: that combination is for +scripts, and starting a browser from a scripted invocation is not wanted. + +Whether `--send` opens a mail client or prints the address and link depends on +the machine, and the printed line says which happened. It prints rather than +opens over SSH, with no `DISPLAY` or `WAYLAND_DISPLAY` on Linux, or with +`ROCM_NO_BROWSER` set to a non-empty value. A machine with no mail client +configured reaches the same printed form, which is why the address appears on +its own and not only inside the `mailto:` link. That is the common case on +servers and in containers. The opt-out is the easiest to check on a desktop: + +```bash +ROCM_NO_BROWSER=1 rocm diagnose --report --send +``` + +The destination is `ROCmCLI@amd.com`, fixed in code. A report also carries its +classification in the mail subject, because a mailbox has no labels: the +subject names the matched catalog entry, or `unrecognised`, then the +architecture and the distribution. Check that the subject carries no field the +report body does not. + +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`, `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. + +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. diff --git a/tests/e2e-cucumber/features/diagnose.feature b/tests/e2e-cucumber/features/diagnose.feature index 259a7a67c..b2b7960c3 100644 --- a/tests/e2e-cucumber/features/diagnose.feature +++ b/tests/e2e-cucumber/features/diagnose.feature @@ -417,3 +417,62 @@ Feature: Diagnosing failures and listing fixes Given a user who has chosen a fix that cannot run until it is told what to act on When the user asks the CLI to apply it without saying what to act on Then the CLI names what it still needs and reports no change + + # 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 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 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-29 - 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-30 - 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 + + # `--send` promises the report is always read before its form is offered. + # That promise only holds if asking for the form without asking to see the + # report first is refused outright, before anything about this machine is + # examined — so this is the same exit code any other argument mistake gets, + # not a diagnosis outcome, and it is true on every host and every lane. + @id:diagnose-send-without-report-is-refused + Scenario: diagnose-31 - Asking the CLI for a way to send a report, without asking to see it first, is refused + When the user asks the CLI for a way to send a report, without asking to see the report first + Then the CLI refuses and explains that the report must be requested too + + # Forces the same headless shape a server or container presents: no display, + # no forwarded display, no override asking for a browser anyway. Linux-only + # because the CLI only reads the environment for this decision on Linux; + # Windows and macOS always treat a user as present, so there is no + # environment that forces this branch on those hosts. + # + # Host-independent beyond that, and for the same structural reason + # diagnose-29 and diagnose-30 are: the WSL lane refuses before any GPU + # probe, and most other lanes have no GPU on the compatibility matrix + # either, so a report is prepared on some lanes and refused on others. + # Written so whichever branch a lane reaches is a real assertion rather + # than a skip. + @id:diagnose-send-on-a-headless-machine-prints-instead-of-opening @requires-os:linux + Scenario: diagnose-32 - Asking to send on a machine with no desktop prints the address and a link instead of starting a mail client + When the user asks the CLI for a way to send a report, with no desktop available to open it on + 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 + And the CLI prints the address to mail and a link, and starts nothing diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs index 2bad76b30..16b87ad9e 100644 --- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs @@ -1909,6 +1909,13 @@ async fn user_diagnoses_with_model_and_distro(world: &mut E2eWorld) { world.cli_rc = Some(rc); } +#[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); +} + #[then("the CLI refuses and says --model answers for this machine, not the one --distro names")] async fn assert_model_with_distro_refused(world: &mut E2eWorld) { let output = world.cli_output.clone().unwrap_or_default(); @@ -1949,3 +1956,195 @@ async fn assert_no_model_verdict_on_refusal(world: &mut E2eWorld) { "a refused request must not also report a model verdict for the wrong machine:\n{output}" ); } + +#[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}" + ); + } +} + +/// The mailbox `--send` offers to prefill, mirrored from +/// `rocm_core::report_delivery::DESTINATION`. Kept as a literal rather than a +/// dependency on `rocm-core`: this crate only runs the built binary, it does +/// not link the library behind it. +const REPORT_DESTINATION: &str = "ROCmCLI@amd.com"; + +#[when("the user asks the CLI for a way to send a report, without asking to see the report first")] +async fn user_asks_to_send_without_report(world: &mut E2eWorld) { + let (stdout, stderr, rc) = crate::run_rocm(world, &["diagnose", "--send"]); + world.cli_output = Some(format!("{stdout}\n{stderr}")); + world.cli_rc = Some(rc); +} + +#[then("the CLI refuses and explains that the report must be requested too")] +async fn assert_send_without_report_refused(world: &mut E2eWorld) { + let out = world.cli_output.clone().expect("no CLI output"); + assert_eq!( + world.cli_rc, + Some(2), + "asking for a way to send a report without asking to see it first is an argument \ + mistake, caught before anything is examined, so it exits the way any other bad \ + argument combination does:\n{out}" + ); + assert!( + out.contains("--report"), + "the refusal does not name the flag the user needed to add first:\n{out}" + ); +} + +/// Forces the headless branch deterministically: no display of any kind, no +/// SSH-forwarded display, and no override asking for a browser regardless. +/// Linux-only in effect, because the CLI under test only reads these on +/// Linux — but the scenario that uses this is the one tagged +/// `@requires-os:linux`, not this helper, so nothing here needs to branch on +/// host. +#[when("the user asks the CLI for a way to send a report, with no desktop available to open it on")] +async fn user_asks_to_send_on_a_headless_machine(world: &mut E2eWorld) { + let (stdout, stderr, rc) = crate::run_rocm_with_env( + world, + &["diagnose", "--report", "--send"], + &[ + ("DISPLAY", ""), + ("WAYLAND_DISPLAY", ""), + ("SSH_CONNECTION", ""), + ("SSH_CLIENT", ""), + ("SSH_TTY", ""), + ("ROCM_NO_BROWSER", ""), + ], + ); + world.cli_output = Some(format!("{stdout}\n{stderr}")); + world.cli_rc = Some(rc); +} + +#[then("the CLI prints the address to mail and a link, and starts nothing")] +async fn assert_send_headless_prints_address_and_link(world: &mut E2eWorld) { + let out = world.cli_output.clone().expect("no CLI output"); + // Same discriminator as `answer_names_nothing_identifying`: a refusal + // envelope has no `cli_version` field, so branch on its presence rather + // than asserting a shape that only a genuine report has. + if out.contains("cli_version") { + assert!( + out.contains(REPORT_DESTINATION), + "a headless machine was not given the address to mail by hand:\n{out}" + ); + assert!( + out.contains("mailto:"), + "a headless machine was not given a link, only the sentence around it:\n{out}" + ); + assert!( + !out.contains("was opened"), + "a mail client was reported opened on a machine with no desktop to open it on:\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}" + ); + } +} + +/// 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() +}