Skip to content

rename: 'rocm doctor' subcommand to 'rocm examine' - #23

Merged
rominf merged 1 commit into
mainfrom
rename-doctor-to-examine
Jun 22, 2026
Merged

rename: 'rocm doctor' subcommand to 'rocm examine'#23
rominf merged 1 commit into
mainfrom
rename-doctor-to-examine

Conversation

@rominf

@rominf rominf commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

Renames the rocm doctor subcommand to rocm examine throughout the codebase.
examine better describes what the command does — it inspects and reports system
state — and avoids a naming collision with external tooling that uses the "doctor"
convention for different purposes.

Changes

  • CLI command: rocm doctorrocm examine
  • TUI command: /doctor/examine
  • MCP/OpenAI tool name: "doctor""examine"
  • Restricted sandbox tool: doctor_snapshotexamine_snapshot (enum
    DoctorSnapshotExamineSnapshot), and the CodexBridgeSnapshot payload
    key doctorexamine
  • All internal Rust identifiers (DoctorSummaryExamineSummary, etc.)
  • File rename: doctor_manager.rsexamine_manager.rs
  • All user-visible help text, error messages, docs, install scripts, and
    CI/smoke/acceptance scripts

Decisions

  • No backwards-compat alias. rocm doctor is a deliberate hard rename — no
    hidden doctor alias — so existing rocm doctor invocations now error. The
    command is pre-1.0 and the clean break keeps the surface coherent.
  • Wire format. examine_snapshot and CodexBridgeSnapshot belong to the
    rocmd-codex-bridge-v0 protocol, which has no stability guarantee yet, so the
    tool name and payload key were renamed in full rather than aliased.

Test plan

  • cargo build --workspace and full cargo test --workspace pass clean
  • Three slash-completion/editing tests updated to the new prefix (/exa, /ex)
  • cargo fmt --check and cargo clippy --workspace clean

@rominf
rominf force-pushed the rename-doctor-to-examine branch from a3e869b to e6fd381 Compare June 17, 2026 18:37
@rominf
rominf requested a review from Copilot June 17, 2026 18:38

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR renames the rocm doctor inspection surface to rocm examine across the CLI, TUI, MCP/OpenAI tool integrations, docs, and CI/scripts to better reflect the command’s purpose and avoid “doctor” naming collisions.

Changes:

  • Renamed user-facing CLI/TUI command surfaces: doctorexamine (including help text, prompts, and slash-command completions).
  • Updated MCP/OpenAI tool naming and related Rust types (DoctorSummaryExamineSummary, etc.), plus updated scripts/CI smoke paths.
  • Refreshed documentation and plans to reference examine instead of doctor.

Reviewed changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
scripts/smoke_local.py Updates local smoke to run/assert rocm examine output.
scripts/acceptance-install-upgrade-tui-uninstall.ps1 Updates acceptance script log naming and calls to rocm examine.
README.md Replaces quick-start “doctor” invocation with “examine”.
plans/rocm-cli-remaining-implementation-plan.md Updates plan references from doctor → examine.
plans/rocm-cli-pytorch-engine-spec.md Updates spec references from doctor → examine.
plans/rocm-cli-implementation-plan.md Updates implementation plan references from doctor → examine.
plans/cosmopolitan-runtime-boundary-plan.md Updates plan/test command references to examine.
install.sh Updates post-install guidance to run rocm examine.
install.ps1 Updates post-install guidance to run rocm examine.
docs/wsl.md Updates section heading and references to rocm examine.
docs/ux-guidelines.md Updates UX guidance text referencing “Doctor” to “Examine”.
docs/testing.md Updates testing instructions and tool-call examples to examine.
docs/manual-testing.md Updates manual test instructions to rocm examine.
docs/implementation-completion-audit.md Updates audit doc references from Doctor → Examine.
docs/current-tui-therock-correction-plan.md Updates doc references from doctor → examine.
docs/cosmopolitan-universal-binary-plan.md Updates universal-binary plan references from doctor → examine.
docs/codex-vendoring-plan.md Updates vendoring plan tool/surface references to examine.
docs/automations.md Updates restricted tool docs wording for doctor_snapshot vs examine.
crates/rocm-dash-tui/src/ui/tabs/instances.rs Renames app state field from doctor_manager → examine_manager in tests.
crates/rocm-dash-tui/src/ui/mod.rs Switches UI overlay wiring and footer label to examine.
crates/rocm-dash-tui/src/ui/logs_view.rs Updates test comment referencing examine re-attach semantics.
crates/rocm-dash-tui/src/ui/examine_manager.rs Introduces/updates examine overlay implementation and tests.
crates/rocm-dash-tui/src/ui/command_screen.rs Updates examples/tests to use examine.
crates/rocm-dash-tui/src/app.rs Renames overlay state/action plumbing to examine.
crates/rocm-core/src/lib.rs Renames summary/inventory types and changes bridge snapshot field to examine.
apps/rocmd/src/lib.rs Renames MCP tool to examine and updates snapshot/tool outputs and guards.
apps/rocm/src/tui.rs Renames TUI surface/logic from doctor → examine, including tool routing and tests.
apps/rocm/src/providers.rs Renames OpenAI tool definition references from doctor → examine.
apps/rocm/src/main.rs Renames clap subcommand + internal rendering/planning/tool logic to examine.
.github/workflows/ci.yml Updates CI host-detection step to run rocm.exe examine.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/rocm-core/src/lib.rs
Comment thread apps/rocmd/src/lib.rs
Comment thread docs/automations.md Outdated
@rominf
rominf force-pushed the rename-doctor-to-examine branch from e6fd381 to 5e6d633 Compare June 22, 2026 08:39
Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
@rominf
rominf force-pushed the rename-doctor-to-examine branch from 5e6d633 to 6907bf6 Compare June 22, 2026 10:52
@rominf
rominf requested a review from Copilot June 22, 2026 11:43
@rominf
rominf marked this pull request as ready for review June 22, 2026 11:43

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

apps/rocm/src/tui.rs:451

  • If you keep doctor as a compatibility alias, is_examine_command_input should treat both examine and doctor as the same command; otherwise /doctor won't get the special-case behavior around background examine refresh blocking/deferral.

Comment thread apps/rocmd/src/lib.rs
Comment thread apps/rocm/src/main.rs
Comment thread apps/rocm/src/tui.rs
Comment thread apps/rocm/src/tui.rs
@rominf
rominf requested review from r0x0r and volen-silo June 22, 2026 12:17
@volen-silo
volen-silo force-pushed the rename-doctor-to-examine branch from 93bdba6 to 6907bf6 Compare June 22, 2026 13:43
@volen-silo

Copy link
Copy Markdown
Collaborator

Review — clean, complete, mergeable

Reviewed the full diff. It's a disciplined pure rename; every change is a doctorexamine swap (locals, functions, enum variants, test names, status strings, the /doctor slash command + its completion tests, and the wire/tool renames). No logic or behavior change is mixed in.

Verified

  • No stragglersgit grep -i doctor across source/docs/scripts/install returns zero hits outside the external rocm-doctor skill name.
  • Internal wire consistency is compiler-guaranteed — the bridge snapshot key, SandboxToolArg::DoctorSnapshotExamineSnapshot, DoctorSummaryExamineSummary, and the doctor_manager.rsexamine_manager.rs module are defined once in shared types, so a half-rename wouldn't compile. Builds + (renamed) tests pass.
  • +796/−791 across 30 files — the signature of a rename with no net additions.

Advisory notes (non-blocking)

  1. Hard break, no alias — sequencing matters most. rocm doctor now errors. That's fine in-repo, but the rocm-doctor skill in amd/skills shells out to rocm doctor, so it breaks the moment this ships until it's re-pointed at rocm examine. Recommend landing this in coordination with the skill update (and a heads-up to anyone with rocm doctor in scripts/CI).
  2. Protocol/tool-name string renames (examine_snapshot, MCP tool "doctor""examine", bridge key) are safe in-repo but would break any external agent manifest hardcoding the old doctor tool name. Acceptable given rocmd-codex-bridge-v0 has no stability guarantee — just calling it out.
  3. Renamed internal id strings in providers.rs (fallback-doctorfallback-examine, etc.) — cosmetic unless any persisted state compares these id strings across versions. Almost certainly ephemeral; quick confirm would close it.
  4. Nice-to-have: a one-line user-facing note (README/CHANGELOG) that doctor was renamed to examine with no alias, since it's a breaking CLI change.

Bottom line

Mechanically correct and complete. The only real consideration is merge sequencing with the skill so the catalog isn't calling a command that no longer exists.

@rominf
rominf added this pull request to the merge queue Jun 22, 2026
Merged via the queue into main with commit da2ddc1 Jun 22, 2026
13 checks passed
@rominf
rominf deleted the rename-doctor-to-examine branch June 22, 2026 13:54
fredespi added a commit that referenced this pull request Jul 12, 2026
…I300X first)

MI300X ran `install sdk` 9× (once per isolated scenario), each a multi-GB TheRock
cold install, pushing the job past its time cap (~37min, cancelled → no report).
Raising the cap failed twice; the fix is to install the runtime ONCE and share it.

- harness: `use_shared_runtimes()` opts a scenario into a shared runtimes tree by
  symlinking its `data/runtimes` at `E2E_SHARED_RUNTIMES_DIR` (only `runtimes/`;
  config/services/engines stay isolated). No-op when the env var is unset (local
  + mock runs stay fully isolated — verified mock still 7 pass / 2 xfail). Called
  from the "a managed runtime is active" precondition only; the clean-slate
  scenarios ("no CLI-managed runtimes", "Installing the SDK") deliberately do NOT
  opt in. Active-runtime resolution falls back to most-recently-installed, so a
  shared registry is active without touching isolated config.
- ci (e2e-gpu / MI300X only for now): pre-warm ONE runtime SERIALLY in the run
  step before the suite (avoids two concurrent scenarios racing installs into the
  shared dir), then point E2E_SHARED_RUNTIMES_DIR at it. Validating on MI300X
  before propagating to the two Strix jobs.

Relates to task #22; unblocks #23 (same cold-install root cause).

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
fredespi added a commit that referenced this pull request Jul 14, 2026
…ail (#23)

`rocm serve <model>` with no --engine is recipe-driven, not platform-driven: it
resolves the request to the recipe's preferred model+engine, which may differ
from what was requested (e.g. a safetensors request resolves to a GGUF recipe on
lemonade). The default-engine scenarios hardcoded the requested model in the
readiness wait, so they timed out whenever the recipe resolved to a different
model — on a lemonade-default host the safetensors request isn't even servable.

Wait on the model the CLI actually resolved (parsed from the serve plan) instead
of the requested id, via new resolved_model() + ready_substr_for() helpers; also
dedup two existing `resolved model:` parses onto resolved_model().

Re-key the Instinct (effective_engine=vllm) xfail from EAI-7333 to EAI-7052:
verified on MI300X that the default serve resolves to a GGUF recipe on lemonade,
whose Vulkan backend hangs on Instinct (EAI-7052) — the default path doesn't use
vLLM at all here, so EAI-7333 was the wrong bug.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
fredespi added a commit that referenced this pull request Jul 14, 2026
TEMPORARY: scoped-probe input forwarded to the cucumber harness on the
strix-ubuntu job so a dispatch can run just the two serve-default-engine-*
scenarios and validate the #23 recipe-aware fix on the lemonade-native Strix
path. Empty = full suite. Remove after validation.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
fredespi added a commit that referenced this pull request Jul 14, 2026
…23 xfail

- expectation.rs: `#[serde(deny_unknown_fields)]` on `Condition`/`XfailEntry` so
  a typo'd key can't parse to an all-None (unconditionally matching) xfail.
- e2e-report: route `scenario_pass_map` through canonical `scenario_passed`; split
  reconcile's `Missing` — an expected pass/xfail with NO result is now `Absent`
  (a problem), so a lost-results run reds the platform instead of passing.
- model_serving.feature: gate `serve-vllm-default-on-instinct` with
  `@requires-engine:vllm` (false failure on lemonade-default hosts).
- expectations.toml: xfail both `serve-default-engine-*` on effective_engine=
  lemonade (EAI-7423: first serve runs a backend install that hides the plan
  line; endpoint dies pre-inference via lemonade Vulkan instability).
- ci.yml/nightly.yml: SHA-pin the newly-added actions/upload-artifact@v4 (v4.6.2)
  and download-artifact@v4 (v4.3.0) per AGENTS.md §6.
- chat_steps.rs / mock_server.rs: remove dead step + never-read received_models.
- dash_journeys.rs: assert real transitions (theme changes; chat error surfaced
  as an Error-role turn), not just a non-empty render.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
fredespi added a commit that referenced this pull request Jul 15, 2026
…ail (#23)

`rocm serve <model>` with no --engine is recipe-driven, not platform-driven: it
resolves the request to the recipe's preferred model+engine, which may differ
from what was requested (e.g. a safetensors request resolves to a GGUF recipe on
lemonade). The default-engine scenarios hardcoded the requested model in the
readiness wait, so they timed out whenever the recipe resolved to a different
model — on a lemonade-default host the safetensors request isn't even servable.

Wait on the model the CLI actually resolved (parsed from the serve plan) instead
of the requested id, via new resolved_model() + ready_substr_for() helpers; also
dedup two existing `resolved model:` parses onto resolved_model().

Re-key the Instinct (effective_engine=vllm) xfail from EAI-7333 to EAI-7052:
verified on MI300X that the default serve resolves to a GGUF recipe on lemonade,
whose Vulkan backend hangs on Instinct (EAI-7052) — the default path doesn't use
vLLM at all here, so EAI-7333 was the wrong bug.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
fredespi added a commit that referenced this pull request Jul 15, 2026
TEMPORARY: scoped-probe input forwarded to the cucumber harness on the
strix-ubuntu job so a dispatch can run just the two serve-default-engine-*
scenarios and validate the #23 recipe-aware fix on the lemonade-native Strix
path. Empty = full suite. Remove after validation.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
fredespi added a commit that referenced this pull request Jul 15, 2026
…23 xfail

- expectation.rs: `#[serde(deny_unknown_fields)]` on `Condition`/`XfailEntry` so
  a typo'd key can't parse to an all-None (unconditionally matching) xfail.
- e2e-report: route `scenario_pass_map` through canonical `scenario_passed`; split
  reconcile's `Missing` — an expected pass/xfail with NO result is now `Absent`
  (a problem), so a lost-results run reds the platform instead of passing.
- model_serving.feature: gate `serve-vllm-default-on-instinct` with
  `@requires-engine:vllm` (false failure on lemonade-default hosts).
- expectations.toml: xfail both `serve-default-engine-*` on effective_engine=
  lemonade (EAI-7423: first serve runs a backend install that hides the plan
  line; endpoint dies pre-inference via lemonade Vulkan instability).
- ci.yml/nightly.yml: SHA-pin the newly-added actions/upload-artifact@v4 (v4.6.2)
  and download-artifact@v4 (v4.3.0) per AGENTS.md §6.
- chat_steps.rs / mock_server.rs: remove dead step + never-read received_models.
- dash_journeys.rs: assert real transitions (theme changes; chat error surfaced
  as an Error-role turn), not just a non-empty render.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
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.

3 participants