fix(diagnose): state in the JSON whether a cause was established - #256
Conversation
`rocm diagnose --json` gave a caller no way to ask the question it most needs answered. The obvious substitute -- is `matched` empty? -- is a different question, and on ordinary hardware it gives the wrong answer. `matched` carries every checker that fired at all, 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 scores 25 before anything has been inspected, an APU beside a discrete GPU scores 40. So a machine with nothing wrong with it hands back a non-empty list. A caller reading that list as a diagnosis proposes a fix for a healthy machine -- re-launch the container that already has its devices -- and never routes the user upstream, which is the one thing it was supposed to do when it recognised nothing. The verdict already existed as `has_match`, and the CLI already documented it as part of what callers read from `--json`. It was simply never serialized. This emits it, computed through the same predicate the accessor uses so the two cannot answer differently, and defaulted on the way in so existing documents still load. The human report was never affected: it has always labelled these entries WEAK. Nothing else changes. `matched` still carries sub-threshold entries -- the WEAK tier is deliberate and the rendered report depends on it -- and no score moves. A base 25 for "in a container" is the tiering working as designed; the missing verdict was the defect. The e2e scenario meant to pin this asserted that `matched` was non-empty, so it encoded the bug rather than the contract. It now reads the verdict. Four scenarios close the gaps that let this through: a verdict is stated when nothing matched; a platform the catalog does not cover is given no findings, cross-checked against the host report so the covered branch can fail too; a fix meant for another OS is declined with its own exit code rather than attempted; and the catalog is complete, with exactly the four entries the CLI carries out itself marked as such. Two comments in examine.feature still said "Expected to FAIL" for defects fixed earlier, whose expectation rows were removed at the time without the feature file following. Both scenarios pass; the comments now say what each is guarding instead of misreporting it as broken. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
a983706 to
3e6950f
Compare
r0x0r
left a comment
There was a problem hiding this comment.
Review: Approve
Well-scoped, correct, and unusually well-tested. Every factual claim in the description checks out against the source.
Correctness (verified against source)
- Additive and backward-compatible:
has_match: boolwith#[serde(default)];matchedunchanged; no scoring changes. - Single source of truth: the new
any_cleared_threshold()helper backs both the serialized field and thehas_match()accessor, so field and method cannot diverge. CATALOG_FIX_IDS(15 ids) matches theRECIPESorder exactly;AUTO_APPLICABLE_FIX_IDSmatches the fourauto_applicable: truerecipes exactly.- Exit code
3and the"This fix only applies on:"string matchfix.rs; the"[ AUTO]"marker matches the[{kind:>10}]formatting. fix_id_for_the_other_os()selects a genuinely non-applicable, print-only recipe on each platform, so the OS gate returns 3 before touching the machine.- The
examinestatus == "wsl"cross-check is valid: WSL2 is currently the onlyout_of_scopetrigger.
Test quality
Strong. Assertions are host-agnostic and self-consistent (assert_verdict_follows_scores holds the report to its own published threshold rather than pinning a verdict a faulty runner would break), the platform-scope test cross-checks diagnose against examine via a different code path rather than tautologically, and the catalog test is intentionally brittle to force a conscious update. The unit test engineers a fixture that pushes has_match to true so the true branch reaches the wire off the bare-metal lanes.
CI
The two red lanes (E2E tests (GPU), E2E tests (Strix Halo, Windows)) both fail only on the pre-existing serve-hf-checkpoint-inference flake, which is unrelated to the diagnose/examine path and is reproduced identically on other unrelated branches. This PR's own new scenarios (10, 11, 12) pass in both GPU logs. All other checks are green.
Minor (non-blocking)
assert_json_states_platform_scopeequatesout_of_scope.is_some()withexamine.status == "wsl". Correct today, but a future non-WSLout_of_scopereason without a matchingexaminestatus would silently break the cross-check — a one-line comment noting "WSL2 is currently the only out-of-scope trigger" would future-proof it.- The
@requires-bare-metalno-match half and the WSL2 out-of-scope half are exercised by no CI lane; the feature file states this honestly rather than implying coverage.
Summary
rocm diagnose --jsongave a caller no way to ask the question it most needs answered: did anything actually match? The obvious substitute — ismatchedempty? — is a different question, and on ordinary hardware it gives the wrong answer.Root cause.
matchedcarries every checker that fired at all, including ones scoring below the match threshold, and several checkers open with a nonzero score for a situation that is merely potentially relevant:check_10_container_devicescheck_9_igpu_dgpu_collisionSo a machine with nothing wrong with it returns a non-empty list. A caller reading that list as a diagnosis proposes a fix for a healthy machine — re-launch the container that already has its devices — and never routes the user upstream, which is the one thing it was supposed to do when it recognised nothing.
The verdict already existed as
has_match, andapps/rocm/src/main.rsalready documented it as part of what callers read from--json. It was simply never serialized.has_match, computed through the same predicate the accessor uses so the two cannot answer differently, and#[serde(default)]so existing documents still load.WEAK. Only the machine-readable form was missing the verdict.Non-obvious decisions
matchedis unchanged. It still carries sub-threshold entries: theWEAKtier is deliberate and the rendered report depends on it. Filtering the list would have been a breaking change to the human output to fix a machine-readable one.No scoring changes. A base 25 for "in a container" is the tiering system working as designed. The missing verdict was the defect, not the number.
No per-entry
tierfield. Derivable fromscoreplus the two thresholds the document already publishes. Adding both widens the contract for nothing gained.Test coverage
The e2e scenario meant to pin this asserted that
matchedwas non-empty — it encoded the bug rather than the contract. It now reads the verdict. Four scenarios close the gaps that let this through:examine --json, so the covered branch can fail too rather than returning earlyAlso: two comments in
examine.featurestill said "Expected to FAIL" for defects fixed earlier, whose expectation rows were removed at the time without the feature file following. Both scenarios pass; the comments now say what each is guarding instead of misreporting it as broken.Test plan
cargo test -p rocm-core diagnose— 22 passed, including a fixture reproducing the healthy-container case and one asserting the serialized field, the accessor, and the emitted document all agree.cargo clippy --workspace --all-targets— 0 warnings.cargo xtask e2e -- -i 'features/diagnose.feature'— 10 passed, 0 unexpected. Scenarios 1 and 3 skip on this host (@requires-bare-metal).cargo xtask e2e -- -i 'features/examine.feature'— 8 passed, 0 unexpected. Confirms the two "Expected to FAIL" comments were stale.out_of_scopetoNonefails the platform cross-check; forcinghas_matchtotruefails both the no-match scenario and the out-of-scope one. Both reverted.Not verifiable here: a real
has_match: trueover the wire needs a bare-metal AMD GPU, so the@requires-bare-metalhalf is deferred to the GPU lanes. There is also no WSL2 lane in CI, so the out-of-scope branch is covered by a local run only — the feature file says so rather than implying CI covers it.cargo test --workspace --all-targetshas one failure,therock::extracting_the_sdk_archive_removes_it, confirmed pre-existing by stashing this diff and reproducing it on unmodifiedmain. It passes in isolation — a known race between concurrent tests mutating the process-globalPATH.Risk: low. Additive JSON field, defaulted on read.
run_diagnosehas one caller. No existing key changes shape or value.tests/e2e-cucumber/expectations.tomlfor the fixed ticket ID and removed/narrowed any now-stale xfail rows. — No rows touched; reconciliation reports0 stale.