Skip to content

feat(cli): consolidate examine and diagnose into rocm doctor - #250

Draft
volen-silo wants to merge 1 commit into
mainfrom
feat/consolidate-doctor-command
Draft

feat(cli): consolidate examine and diagnose into rocm doctor#250
volen-silo wants to merge 1 commit into
mainfrom
feat/consolidate-doctor-command

Conversation

@volen-silo

Copy link
Copy Markdown
Collaborator

Summary

rocm examine and rocm diagnose both read as "check my machine", and split one job in half. The overlap was structural, not cosmetic:

  • The host probe ran twice. diagnose re-ran the same probe examine --json had just run, framework interpreter start included, with no way to hand one command's output to the other.
  • The WSL2 route-out verdict was written twice, in two independently-authored wordings that had already drifted, with the same doc URL duplicated in both files.
  • A stale comment cited an exit_code() that no longer exists.

This adds rocm doctor: one read-only pass that reports what the machine has and what the catalog makes of it.

  • --from-examination <path|-> diagnoses a report captured earlier without inspecting the machine again. Since that report may describe a different host, it prints findings only rather than mixing in local inventory.
  • doctor --json is a strict superset of examine --json — same keys in the same places, plus findings.
  • examine and diagnose are hidden but still dispatch, output unchanged, so scripts, the packaged daemon, and the suite's own capability probe keep working.

The dash already advertised rocm doctor, routed /doctor, and registered a doctor LLM tool. The CLI was the only layer that did not — this closes that gap rather than opening a new name.

Non-obvious decisions

Read-only is enforced, not asserted. The host report carried one write: re-registering a setup runtime that is present on disk but missing from the registry. That is a repair, and a command whose help says it changes nothing must not do it. The write is now opt-in — examine still repairs, doctor declines. The visible cost: for that user, doctor reports the install as unregistered instead of silently fixing it.

Hidden, not deleted. This decouples the user-facing consolidation from migrating ~30 internal call sites, and keeps existing scripts working. Migrating rocmd and the e2e capability probe is mechanical follow-up.

examine and diagnose output is byte-identical to before, with exactly one deliberate exception: the WSL2 note, which is the de-duplication this change exists to do. Verified by diffing both commands against a build of main. Nothing in the repo asserts on that text.

Out of scope

WSL2 catalog dispatch, splitting the CLI's own inventory out as rocm status, and folding the coarse five-bucket status into the catalog. Each is a follow-up.

Test plan

  • cargo test --workspace --all-targets — green. Two tests flake on this host (therock::extracting_the_sdk_archive_removes_it, a pre-existing race where tests mutate the process-global PATH and a concurrent test spawning tar dies; and a provider test needing a live local service). Both confirmed flaking on unmodified main.
  • cargo clippy --workspace --all-targets — 0 warnings.
  • python3 scripts/smoke_local.py — passes.
  • E2E: 50 scenarios, 0 unexpected failures, 3 pre-existing xfail. New doctor.feature covers 8 scenarios including a planted repairable install that asserts doctor leaves the registry untouched and that examine writes it — the second half is a control, so the read-only claim cannot pass vacuously.
  • Manual on a WSL2 host without GPU paravirtualisation: doctor, doctor --json, both --from-examination forms, and byte-diffs of examine/diagnose against main.

Not verifiable here: @requires-gpu / @requires-bare-metal scenarios need the amd-gpu lane.

Risk: low. Additive command; superseded commands keep working with unchanged output; the one behaviour change (no repair write under doctor) is deliberate and covered.

  • If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. — Not a bug fix; no rows touched. The E2E reconciliation reports 0 stale, confirming none became obsolete.

@volen-silo
volen-silo requested a review from a team as a code owner August 13, 2026 10:29
`examine` and `diagnose` both read as "check my machine" and split one job
in half. The overlap was structural, not cosmetic: the host probe ran twice
with no way to pass one command's output to the other, and the WSL2
route-out verdict was written twice, in two wordings that had already
drifted apart.

`rocm doctor` makes one read-only pass and reports both what the machine
has and what the catalog makes of it. `--from-examination` diagnoses a
report captured earlier, without inspecting the machine again; because that
report may describe a different host, it prints findings only rather than
mixing in local inventory.

`examine` and `diagnose` are hidden but still dispatch, with output
unchanged, so scripts, the packaged daemon, and the suite's own capability
probe keep working. Only the WSL2 note differs, which is the de-duplication
this change exists to do.

Read-only is enforced rather than asserted. The host report carried one
write -- re-registering a setup runtime present on disk but missing from
the registry -- and that is now opt-in: `examine` still repairs, `doctor`
declines. An acceptance scenario plants a repairable install and checks
both halves, so the claim cannot pass vacuously.

The dash already advertised `rocm doctor`, routed /doctor, and registered a
`doctor` LLM tool. The CLI was the only layer that did not.

Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
@volen-silo
volen-silo force-pushed the feat/consolidate-doctor-command branch from 9b1dfae to 9f082b1 Compare August 13, 2026 10:35
@volen-silo
volen-silo marked this pull request as draft August 14, 2026 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant