fix(diagnose): state in the JSON whether a cause was established

crates/rocm-core/src/diagnose.rs

@@ -59,7 +59,26 @@ pub struct Route {
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct DiagnoseReport {
     /// All nonzero-score diagnoses, highest score first.
+    ///
+    /// Every checker that fired at all lands here, including ones scoring below
+    /// [`MIN_SCORE_FOR_MATCH`] — so a non-empty `matched` does NOT mean a cause
+    /// was established. Read [`DiagnoseReport::has_match`] for that.
     pub matched: Vec<Diagnosis>,
+    /// Whether any entry in `matched` cleared [`MIN_SCORE_FOR_MATCH`].
+    ///
+    /// Serialized because a JSON consumer cannot otherwise tell a real cause
+    /// from a weak signal, and the obvious substitute — "is `matched` empty?" —
+    /// is wrong. Several checkers open with a nonzero base score for a
+    /// situation that is merely *potentially* relevant (being in a container,
+    /// having an APU alongside a discrete GPU), so a perfectly healthy host
+    /// produces a non-empty `matched` full of sub-threshold entries. A caller
+    /// gating on emptiness proposes a fix for a machine with nothing wrong, and
+    /// never routes the user to `route_when_no_match`.
+    ///
+    /// Computed at construction; [`DiagnoseReport::has_match`] recomputes from
+    /// `matched` and stays the authority for Rust callers.
+    #[serde(default)]
+    pub has_match: bool,
     pub min_score_for_match: i32,
     pub high_confidence_threshold: i32,
     pub route_when_no_match: Route,
@@ -70,11 +89,19 @@ pub struct DiagnoseReport {
     pub out_of_scope: Option<String>,
 }
 
+/// Whether any diagnosis cleared [`MIN_SCORE_FOR_MATCH`].
+///
+/// One place the rule is written, so the serialized `has_match` field and the
+/// [`DiagnoseReport::has_match`] accessor cannot answer differently.
+fn any_cleared_threshold(matched: &[Diagnosis]) -> bool {
+    matched.iter().any(|d| d.score >= MIN_SCORE_FOR_MATCH)
+}
+
 impl DiagnoseReport {
     /// Whether at least one diagnosis cleared [`MIN_SCORE_FOR_MATCH`].
     #[must_use]
     pub fn has_match(&self) -> bool {
-        self.matched.iter().any(|d| d.score >= MIN_SCORE_FOR_MATCH)
+        any_cleared_threshold(&self.matched)
     }
 }
 
@@ -1310,6 +1337,7 @@ pub fn diagnose(e: &Examination, symptom: &str) -> DiagnoseReport {
         run_all_checks(e, symptom)
     };
     DiagnoseReport {
+        has_match: any_cleared_threshold(&matched),
         matched,
         min_score_for_match: MIN_SCORE_FOR_MATCH,
         high_confidence_threshold: HIGH_CONFIDENCE,
@@ -1740,6 +1768,91 @@ mod tests {
         assert!(report.route_when_no_match.url.contains("pytorch/pytorch"));
     }
 
+    #[test]
+    fn a_healthy_container_reports_no_match_despite_a_nonempty_list() {
+        // The case `has_match` exists for. `check_10_container_devices` opens at
+        // 25 for merely being in a container, before it has looked at anything,
+        // and this fixture adds the 20 for no visible render device (a probe
+        // that found nothing, not a device that is missing) -- 45, short of the
+        // threshold and still in `matched`. A caller reading "is `matched`
+        // empty?" as "did anything match?" would propose re-launching a
+        // container over what is only a thin probe, and would never route the
+        // user upstream.
+        let mut e = linux_base();
+        e.in_container = true;
+        e.container_kind = "docker".to_owned();
+        let report = diagnose(&e, "");
+
+        assert!(
+            !report.matched.is_empty(),
+            "fixture must produce an entry for this test to mean anything"
+        );
+        assert!(
+            report.matched.iter().all(|d| d.score < MIN_SCORE_FOR_MATCH),
+            "fixture must stay below the threshold; got {:?}",
+            report
+                .matched
+                .iter()
+                .map(|d| (&d.id, d.score))
+                .collect::<Vec<_>>()
+        );
+        assert!(
+            !report.has_match,
+            "a list of sub-threshold entries is not a match"
+        );
+    }
+
+    #[test]
+    fn the_serialized_verdict_agrees_with_the_accessor() {
+        // Rust callers read the method, tooling reads the field. They answer the
+        // same question, so a host where they disagree would hand the two
+        // audiences different verdicts.
+        let mut in_container = linux_base();
+        in_container.in_container = true;
+        let mut render_group_missing = linux_base();
+        render_group_missing.in_render_group = Some(false);
+        render_group_missing.in_video_group = Some(false);
+
+        // The fixtures above all land BELOW the threshold, so on their own they
+        // would only ever exercise the `false` branch -- and a field that is
+        // always false serializes correctly by accident. The last one carries a
+        // symptom that pushes the same finding past HIGH_CONFIDENCE, so `true`
+        // reaches the wire here too and not only on a bare-metal e2e lane.
+        let mut real_fault = linux_base();
+        real_fault.in_render_group = Some(false);
+
+        let cases = [
+            (linux_base(), ""),
+            (in_container, ""),
+            (render_group_missing, ""),
+            (real_fault, "RuntimeError: unable to open /dev/kfd"),
+        ];
+        assert!(
+            cases
+                .iter()
+                .any(|(e, symptom)| diagnose(e, symptom).has_match),
+            "at least one fixture must clear the threshold, or this only ever \
+             proves the false branch"
+        );
+
+        for (e, symptom) in cases {
+            let report = diagnose(&e, symptom);
+            assert_eq!(
+                report.has_match,
+                report.has_match(),
+                "field and accessor disagree for {:?}",
+                report.matched.iter().map(|d| &d.id).collect::<Vec<_>>()
+            );
+            let json: serde_json::Value =
+                serde_json::from_str(&serde_json::to_string(&report).unwrap()).unwrap();
+            assert_eq!(
+                json.get("has_match").and_then(serde_json::Value::as_bool),
+                Some(report.has_match),
+                "the verdict must survive into the emitted document"
+            );
+        }
+    }
+
     #[test]
     fn no_match_default_route_is_rocm_core() {
         let e = linux_base();
@@ -1818,6 +1931,7 @@ mod tests {
         let v = serde_json::to_value(&report).unwrap();
         for key in [
             "matched",
+            "has_match",
             "min_score_for_match",
             "high_confidence_threshold",
             "route_when_no_match",

tests/e2e-cucumber/features/diagnose.feature

@@ -78,3 +78,65 @@ Feature: Diagnosing failures and listing fixes
     When the user asks the CLI to apply it without agreeing to the change
     Then the CLI refuses and explains that it needs agreement
     And the file the fix would have changed is untouched
+
+  # The other half of scenario 3, and the half every host can prove. A caller
+  # cannot read "did anything match?" off the size of the list: every checker
+  # that fires at all is reported, including ones scoring too low to act on,
+  # and several open with a nonzero score for a situation that is merely
+  # POTENTIALLY relevant — being in a container, having an APU beside a
+  # discrete GPU. So a healthy machine hands back a non-empty list of things
+  # that are not wrong with it. A caller treating that as a diagnosis proposes
+  # a fix for a machine with nothing wrong, and never routes the user onward.
+  @id:diagnose-json-states-when-nothing-matched
+  Scenario: 9 - A tool is told plainly when no cause was established
+    Given a user who hit a failure the CLI does not recognise
+    When the user asks the CLI to diagnose that symptom in machine-readable form
+    Then the result states that no cause was established
+    And the CLI always points to somewhere the problem can be reported
+
+  # Host-agnostic on purpose: the scenario asks the CLI what it makes of this
+  # platform and then holds it to the matching half of the contract. A caller
+  # decides whether to diagnose at all from this verdict, and nothing pinned it
+  # before — the suite only ever SKIPPED the bare-metal scenarios on WSL2, which
+  # proves nothing about what gets reported there.
+  #
+  # Be precise about where each half runs, because the halves are not equal.
+  # There is NO WSL2 lane in CI (every job pins a native runner), so the
+  # route-out half is proven only by a developer running the suite on WSL2.
+  # What CI gets is the covered half plus the cross-check against the host
+  # report — both of which can fail, which is the bar an assertion has to clear
+  # to be worth writing. An earlier version of this scenario returned early on a
+  # covered platform and asserted nothing at all on any lane CI runs.
+  @id:diagnose-states-whether-the-platform-is-covered
+  Scenario: 10 - A platform the catalog does not cover says so and routes onward
+    Given a user who hit a known ROCm failure
+    When the user asks the CLI to diagnose that symptom in machine-readable form
+    Then the result says whether this platform is covered
+    And a platform that is not covered is given no diagnosis
+    And a platform that is covered gets a verdict that follows the evidence
+    And the CLI always points to somewhere the problem can be reported
+
+  # A fix that cannot run here is a different outcome from one that failed, and
+  # from one the user declined — a caller that cannot tell them apart reports a
+  # broken machine when the truth is "wrong operating system". The scenario
+  # picks whichever catalog entry belongs to the OTHER platform, so it carries
+  # the same weight on the Linux and Windows lanes.
+  @id:fix-inapplicable-here-is-declined-not-attempted
+  Scenario: 11 - A fix meant for another operating system is declined, not attempted
+    Given a user who has chosen a fix meant for a different operating system
+    When the user asks the CLI to apply that fix
+    Then the CLI declines because the fix does not apply to this machine
+    And nothing on the machine is changed
+
+  # Scenario 4 proves the listing works; this proves it is COMPLETE. Which
+  # failure modes exist, and which of them the CLI will carry out itself, are
+  # part of the published contract rather than private detail — so a mode added
+  # or removed is a change to what callers were promised, and it should not be
+  # possible to make it quietly. This is deliberately the brittle test that
+  # breaks when the catalog changes; that break is the notification. Do not
+  # loosen it.
+  @id:fix-catalog-is-complete
+  Scenario: 12 - The CLI offers every fix its catalog documents
+    When the user asks the CLI which fixes it offers
+    Then every fix the catalog documents is listed
+    And only the fixes the CLI can carry out itself are marked as such

tests/e2e-cucumber/features/examine.feature

@@ -50,26 +50,27 @@ Feature: GPU detection and system inspection
     And the inspection does not claim nothing is installed
     And the inspection suggests setting up a CLI-managed install
 
-  # Expected to FAIL. The machine-readable form is a separate code path, not a
-  # re-rendering of the human one: it answers before the CLI has loaded its paths
-  # or config, so every CLI-side fact is out of reach. Eleven things the human
-  # report states have no field in it at all — among them which engine this host
-  # will serve on and whether an existing ROCm install was found. Tooling reads
-  # this form; it should not be the weaker of the two.
+  # The machine-readable form is a separate code path, not a re-rendering of the
+  # human one: it used to answer before the CLI had loaded its paths or config,
+  # putting every CLI-side fact out of reach. Eleven things the human report
+  # states had no field in it at all — among them which engine this host will
+  # serve on and whether an existing ROCm install was found. Since fixed (those
+  # facts now travel under `summary`), and this is what holds the two forms
+  # level: tooling reads this one, and it must not drift back into being the
+  # weaker of the two.
   @id:examine-machine-readable-report
   Scenario: 7 - What the inspection tells a tool matches what it tells a person
     When the user inspects the system both for reading and for scripting
     Then the machine-readable form states everything the readable one does
 
-  # Expected to FAIL on Instinct. The harness parses the human text rather than
-  # this form precisely because of this defect, and says so in capability.rs —
-  # on a real MI300X the machine-readable form reported no AMD GPU on a machine
-  # that has one. That workaround makes the disagreement load-bearing: every
-  # host capability the suite resolves comes from scraped text because this form
-  # could not be trusted.
-  #
-  # It is narrower than "any host with a GPU": Strix Halo (gfx1151) agrees,
-  # MI300X (gfx943) does not. The expectations row is scoped to match.
+  # The harness parses the human text rather than this form because of a defect
+  # this scenario caught, and says so in capability.rs — on a real MI300X the
+  # machine-readable form reported no AMD GPU on a machine that has one, while
+  # Strix Halo (gfx1151) agreed. That workaround makes the disagreement
+  # load-bearing: every host capability the suite resolves comes from scraped
+  # text, so if the two forms ever diverge again, every capability-keyed
+  # expectation silently resolves against the wrong host. Since fixed; this is
+  # the guard that keeps it fixed.
   @id:examine-both-forms-agree-on-gpu
   Scenario: 8 - Both forms of the inspection agree about the GPU
     When the user inspects the system both for reading and for scripting