§1: read-only ROCm tools + nav/session slash commands + parity map by michaelroy-amd · Pull Request #31 · ROCm/rocm-cli

michaelroy-amd · 2026-06-21T15:44:02Z

Stack 3/10 · base: supergoal/phase-2-execution-seam

12 read-only ROCm tools (doctor/engines/services/gpu/…) via a rocm_read_tool! macro, delegating through the Phase-2 seam; registered in both Rig + ChatGPT backends.
Nav/session + read-only slash commands (/home /help /clear /quit /doctor /runtimes /config /logs /gpu /model /daemon).
Seeds docs/dash-parity-map.md (30 legacy commands).

All cargo gates green; mock round-trip + slash-dispatch tests. Reviewed clean.

rominf

LGTM. Strong test coverage for a TUI PR — read-only round-trip, None-executor no-panic, full slash-command matrix, tool-name disjointness.

One note: rocm_command's "mutating subcommands are rejected" guarantee is enforced across the seam in the bin (correct layer) but isn't verifiable/tested in this PR — worth ensuring the executor impl lands a test rejecting e.g. ["serve"].

rominf

Requesting changes on signing grounds — this is separate from the code review above, which still stands.

Every commit on this PR is unsigned: GitHub reports verification.verified = false (reason unsigned) for all of them. Signed commits are required before this can merge.

To fix, configure commit signing and re-sign the branch history, e.g. with SSH signing:

git config gpg.format ssh
git config user.signingkey <your-signing-key.pub>
git config commit.gpgsign true
git rebase --exec 'git commit --amend --no-edit -S' <base-branch>
git push --force-with-lease

Since this is a stacked PR, re-signing is easiest from the bottom of the stack upward — re-signing a base branch rewrites its SHAs and the dependent branches will need rebasing/re-pushing on top.

Once the commits show as Verified I'll clear this.

rominf

Signing gate cleared — all commits on this PR now show Verified (verification.verified = true, reason valid). Re-reviewed the current diff against this PR's own base after the re-sign; it's substantively unchanged from my prior review.

Approving on both grounds: code is sound and commits are signed.

volen-silo

Request changes — one blocking bug: the doctor read-only tool is dead-on-arrival.

agent.rs registers a tool named doctor (first in ROCM_READ_TOOL_NAMES), but the bin engine has no doctor — the equivalent is examine. There is zero doctor handling in validate_chat_tool_call, chat_tool_call_is_read_only, or the run_internal_mcp_call dispatch in apps/rocm/src/main.rs.

So when the model actually calls it:

DoctorRocmTool::call → run_rocm_read_tool → BinToolExecutor::execute("doctor", …)
  → validate_chat_tool_call → bail!("local assistant requested unsupported ROCm tool `doctor`")
  → RocmToolOutcome::Error

Every invocation returns {"error":"…unsupported ROCm tool doctor"}. The other 11 names all match the bin's accept-list — doctor is the only break.

Why no test caught it: read_only_tool_round_trips_to_json uses FakeExec, which echoes a fixed value and ignores the name, so it never exercises the real name-validation path. The name-completeness test only checks the dash-side array against itself.

Fix — pick one:

Register the tool as examine to match the bin (simplest), or
Add a doctor → examine alias in the bin's three sites (validate, is_read_only, dispatch) so the doctor name works end-to-end and matches the /doctor overlay naming.

Either way, please add a guard test asserting ROCM_READ_TOOL_NAMES ⊆ the bin's accepted tool set (or an integration test driving the real BinToolExecutor), so a name-only tool can't silently ship again.

Non-blocking (carryover from the seam PR): the LLM read-only tools call e.execute() synchronously inside the async Tool::call, but execute() does blocking I/O. The slash-tool path correctly uses spawn_blocking; the LLM tool-call path does not, so a model-issued engines/services/bridge_snapshot blocks a tokio worker. Consider wrapping execute() in run_rocm_read_tool for parity.

The rest is solid — slash routing, the decoupled SlashToolReply, and the concise summarizer are all clean and test-locked.

Add an integration test (daemon_runs_real_foreground_loop) that spawns the built binary with a temp state dir and asserts the rocmd run banner appears, plus a unit test for daemon_run_argv.

…point + real argv test

Reuse the existing bin engine functions across the rocm-core-free seam: read-only intents return structured JSON, mutating intents return an approval descriptor, and approved CLI args dispatch in-process. Widen run_internal_mcp_call and dispatch to pub(crate) for the sibling module. Inject the executor only for a live dash (None for demo/replay/mock).

…eanups

…tor seam Add a declarative macro (rocm_read_tool!) generating 12 read-only Rig tools that forward the model's tool-call intent across the rocm-core-free tool_exec seam to the bin-supplied executor: doctor, engines, services, service_logs, bridge_snapshot, gpu_snapshot, automations, path_exists, port_status, update_check, install_sdk_dry_run, rocm_command. None-executor is graceful (error object, no panic); ApprovalRequired is handled defensively. Names live in ROCM_READ_TOOL_NAMES, mirroring SKILL_NAMES.

RigAgentClient::new and ChatGptAgentClient::new now take the optional SharedRocmToolExecutor and register the 12 read-only ROCm tools on both AgentBuilders via a shared register_rocm_read_tools helper. Live dash passes state.tool_executor; demo/replay/mock stay executor-less (None).

submit_chat now routes /-prefixed input through handle_slash_command (never to the LLM). Group A nav/session: /home, /gpu, /help, /?, /clear, /quit, /exit. Group B read-only overlays: /doctor, /runtimes, /config, /logs. Group B executor-backed (off-thread): /model, /daemon -> rocm_command, rendered as a concise summary (no raw JSON dump). Unknown /foo -> error turn. Adds SlashOutcome, should_quit, slash_tool fields and the event-loop drain.

One row per legacy rocm chat slash command across groups A-G. Groups A (nav/session) and B (read-only) marked covered (Phase 3) with concrete dash mechanism + Evidence test pointers; D/C/E/F/G marked pending for their phases.

agent.rs: FakeExec executor proving chat->tool->executor->JSON (read_only_tool_round_trips_to_json), None-executor graceful error (read_only_tool_none_executor_is_graceful), and name-set completeness/disjoint checks. app.rs: 15 slash-dispatch tests (nav/session, overlays, executor requests, unknown-command error turn, submit routing). Backfill the two new AppState fields in the instances.rs test literal.

…ity + parity-map accuracy

…-tool names The dash side registers the read-only machine-inspection tool as `doctor` (matching the `/doctor` overlay), but the bin only accepted `examine`, so a model-issued `doctor` call returned "unsupported ROCm tool `doctor`" — dead on arrival. Add `doctor` as an alias of `examine` in the bin's three sites (validate_chat_tool_call, chat_tool_call_is_read_only, run_internal_mcp_call dispatch) so the name resolves end-to-end. Add a hermetic guard test asserting every name in ROCM_READ_TOOL_NAMES is in the bin's accept-list (not 'unsupported ROCm tool'), so a name-only tool can't silently ship again. Addresses review on #31.

michaelroy-amd · 2026-06-24T16:57:11Z

Thanks @volen-silo — addressed in the latest push (force-pushed after a rebase onto current main + re-sign; all commits show Verified).

Blocking doctor bug — fixed (your option B, the alias).
The dash side intentionally registers the tool as doctor to match the /doctor overlay, so rather than rename, I added doctor as an alias of the bin's examine in all three sites you identified:

validate_chat_tool_call — "examine" | "doctor" accepted
chat_tool_call_is_read_only — "examine" | "doctor" treated read-only
run_internal_mcp_call dispatch — "examine" | "doctor" => runs the same ExamineSummary::gather() path

So a model-issued doctor now resolves end-to-end instead of returning unsupported ROCm tool \doctor``.

Guard test — added.
read_tool_names_are_subset_of_bin_accept_list iterates ROCM_READ_TOOL_NAMES and asserts each name is in the bin's accept-list (i.e. validate_chat_tool_call never rejects it as unsupported ROCm tool). It's deliberately hermetic — it checks name acceptance only, not arg validation, so it doesn't drive the real BinToolExecutor::execute / live I/O but still fails loudly if a name-only tool is ever added again. (Confirmed it goes RED on the pre-fix tree and GREEN after.)

Non-blocking spawn_blocking carryover — not in this round.
Tracking the blocking-execute()-inside-async-Tool::call parity separately rather than folding it into this PR; flagging so it's not silently dropped.

Re-review when you have a moment — happy to adjust the alias-vs-rename call if you'd prefer the simpler rename.

Copilot

Pull request overview

Introduces Phase-3 dash TUI parity work by adding read-only ROCm inspection tools (via an executor seam), plus local slash-command routing for navigation/session actions and a parity map documenting legacy rocm chat command coverage.

Changes:

Add reducer-level slash-command handling (/home, /help, /clear, /quit, /doctor, etc.) including executor-backed read-only slash commands (/model, /daemon).
Register 12 read-only ROCm tools in both Rig and ChatGPT agent backends, forwarding tool calls through an injected executor seam.
Add/update documentation and tests to lock in slash-command behavior and ensure dash tool names are accepted by the apps/rocm tool allowlist.

Reviewed changes

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

Show a summary per file

File	Description
docs/dash-parity-map.md	Adds a parity tracking table for migrating legacy `rocm chat` slash commands into dash TUI surfaces.
crates/rocm-dash-tui/src/ui/tabs/instances.rs	Updates test AppState construction to include newly added reducer fields.
crates/rocm-dash-tui/src/client.rs	Adds a `SlashToolReply` message type for executor-backed slash command results.
crates/rocm-dash-tui/src/app.rs	Implements slash-command routing, new reducer edges (`should_quit`, `slash_tool`), and concise slash-tool summarization.
crates/rocm-dash-tui/src/agent.rs	Adds a macro-defined set of read-only ROCm tools that forward execution across the tool-executor seam; wires executor into both agent clients.
apps/rocm/src/main.rs	Accepts `doctor` as an alias for `examine` and adds a regression test ensuring dash tool names are accepted by the bin allowlist.

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

+        // Slash commands are handled locally (nav/overlays/read-only tools) and
+        // never reach the LLM. A `/`-prefixed line is ALWAYS consumed here, even
+        // when unknown (it gets an error turn) — only non-slash text is sent on.
+        if self.handle_slash_command(&text) == SlashOutcome::Handled {
+            self.chat_input.clear();


+fn scalar_or_shape(v: &serde_json::Value) -> String {
+    use serde_json::Value;
+    match v {
+        Value::String(s) => s.clone(),
+        Value::Null => "null".to_string(),
+        Value::Bool(b) => b.to_string(),
+        Value::Number(n) => n.to_string(),
+        Value::Array(a) => format!("[{} items]", a.len()),
+        Value::Object(o) => format!("{{{} fields}}", o.len()),
+    }
+}


+| clear | A | covered (Phase 3) | `/clear` slash → empties `chat` transcript | `slash_clear_empties_transcript` |
+| quit | A | covered (Phase 3) | `/quit` slash → `should_quit` (event loop breaks) | `slash_quit_sets_should_quit` |
+| exit | A | covered (Phase 3) | `/exit` slash → `should_quit` (event loop breaks) | `slash_exit_sets_should_quit` |
+| doctor | B | covered (Phase 3) | `/doctor` slash → opens the doctor overlay (`doctor_manager`); the read-only `doctor` tool is covered separately via the LLM tool-call seam | `slash_doctor_opens_overlay` / `read_only_tool_round_trips_to_json` |


Dismissing: the blocking doctor bug and the requested guard test are both addressed in the current (rebased + re-signed) head. doctor is now accepted as an alias of the bin's examine in all three sites (validate_chat_tool_call, chat_tool_call_is_read_only, run_internal_mcp_call), and read_tool_names_are_subset_of_bin_accept_list guards ROCM_READ_TOOL_NAMES ⊆ accept-list. See my reply comment for details; re-review requested. The non-blocking spawn_blocking nit is tracked separately.

rominf

Re-reviewed the updated diff after the doctor-fix push.

The blocking doctor bug @volen-silo flagged is now resolved in apps/rocm/src/main.rs: examine | doctor is aliased across all three sites — validate_chat_tool_call, chat_tool_call_is_read_only, and the run_internal_mcp_call dispatch — so a model-issued doctor call now resolves end-to-end instead of bailing with unsupported ROCm tool. A regression test asserting the dash tool names are accepted by the bin allowlist was added, closing the name-only-tool gap that let it ship silently.

All 7 CI checks pass (build-and-test, windows-build-and-test, clippy, fmt, coverage, hawkeye, lint). Commits are signed/Verified.

Approving. @volen-silo — when you have a moment, please re-review so the prior CHANGES_REQUESTED can be cleared; this is the bottom of the stack and #32–#38 are queued behind it.

Copilot

Pull request overview

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

+| clear | A | covered (Phase 3) | `/clear` slash → empties `chat` transcript | `slash_clear_empties_transcript` |
+| quit | A | covered (Phase 3) | `/quit` slash → `should_quit` (event loop breaks) | `slash_quit_sets_should_quit` |
+| exit | A | covered (Phase 3) | `/exit` slash → `should_quit` (event loop breaks) | `slash_exit_sets_should_quit` |
+| doctor | B | covered (Phase 3) | `/doctor` slash → opens the doctor overlay (`doctor_manager`); the read-only `doctor` tool is covered separately via the LLM tool-call seam | `slash_doctor_opens_overlay` / `read_only_tool_round_trips_to_json` |


+fn scalar_or_shape(v: &serde_json::Value) -> String {
+    use serde_json::Value;
+    match v {
+        Value::String(s) => s.clone(),
+        Value::Null => "null".to_string(),
+        Value::Bool(b) => b.to_string(),
+        Value::Number(n) => n.to_string(),
+        Value::Array(a) => format!("[{} items]", a.len()),
+        Value::Object(o) => format!("{{{} fields}}", o.len()),
+    }
+}


+            RocmToolOutcome::ApprovalRequired(_) => {
+                json!({ "error": "this action requires approval (interactive chat only)" })
+            }


…eaders, lints) The dash redesign predates main's doctor_manager→examine_manager rename (PR #31) and main's hawkeye license-header enforcement. Rebasing --onto main surfaced three classes of gap in code that did not textually conflict: - references to retired symbols (OpenDoctor, doctor_manager, DoctorManagerState) auto-merged into app/mod.rs, tabs/rocm.rs and the characterization test — renamed to the examine_* API, - new files added by the redesign (dock, launcher, panel, the home/observe/ rocm/serving/pane tab modules, gen_mockups, dash_characterization) lacked the MIT SPDX header — added via hawkeye format, - a few fmt/clippy nits (redundant field name, field-reassign-with-default, semicolon-if-nothing-returned) surfaced by the merge. Gate is green: build --all-targets, cargo fmt --check, clippy -D warnings, hawkeye check, and the rocm-dash-tui suite (545 tests). Signed-off-by: Michael Roy <michael.roy@amd.com>

rominf approved these changes Jun 22, 2026

View reviewed changes

michaelroy-amd force-pushed the supergoal/phase-2-execution-seam branch from a556f7a to 86cd795 Compare June 23, 2026 03:40

michaelroy-amd force-pushed the supergoal/phase-3-readonly-and-nav branch from 4d900ab to 09c172d Compare June 23, 2026 03:41

rominf requested changes Jun 23, 2026

View reviewed changes

michaelroy-amd force-pushed the supergoal/phase-2-execution-seam branch from 86cd795 to 20612c5 Compare June 23, 2026 17:39

michaelroy-amd force-pushed the supergoal/phase-3-readonly-and-nav branch from 09c172d to ecb4c53 Compare June 23, 2026 17:39

rominf approved these changes Jun 24, 2026

View reviewed changes

rominf force-pushed the supergoal/phase-2-execution-seam branch from 20612c5 to f976cb8 Compare June 24, 2026 07:08

Base automatically changed from supergoal/phase-2-execution-seam to main June 24, 2026 07:27

rominf force-pushed the supergoal/phase-3-readonly-and-nav branch from ecb4c53 to 0a23116 Compare June 24, 2026 07:38

rominf requested review from r0x0r and volen-silo June 24, 2026 07:38

volen-silo previously requested changes Jun 24, 2026

View reviewed changes

michaelroy-amd enabled auto-merge June 24, 2026 09:19

michaelroy-amd added 14 commits June 24, 2026 08:59

test: prove rocm daemon runs the real foreground loop

5a7e475

Add an integration test (daemon_runs_real_foreground_loop) that spawns the built binary with a temp state dir and asserts the rocmd run banner appears, plus a unit test for daemon_run_argv.

chore: rustfmt daemon test and record rocmd lockfile edge

a68b7c7

refactor: address review — explicit pub(crate) daemon-lifecycle entry…

9799c18

…point + real argv test

refactor: defer execute_approved to phase 4 per review; must_use + cl…

da2ec77

…eanups

docs: seed dash parity map (30 legacy slash commands)

3e7dd92

One row per legacy rocm chat slash command across groups A-G. Groups A (nav/session) and B (read-only) marked covered (Phase 3) with concrete dash mechanism + Evidence test pointers; D/C/E/F/G marked pending for their phases.

style: cargo fmt for phase-3 read-only tools and slash commands

6dc5046

fix(dash): satisfy clippy equatable_if_let in submit_chat slash routing

5de620f

refactor: decouple slash-tool reply from agent state; tighten visibil…

0ab965a

…ity + parity-map accuracy

michaelroy-amd force-pushed the supergoal/phase-3-readonly-and-nav branch from 0a23116 to 29eefc5 Compare June 24, 2026 16:53

michaelroy-amd requested a review from volen-silo June 24, 2026 16:57

michaelroy-amd disabled auto-merge June 24, 2026 17:03

rominf requested review from Copilot and rominf June 24, 2026 17:34

Copilot started reviewing on behalf of rominf June 24, 2026 17:36 View session

Copilot AI reviewed Jun 24, 2026

View reviewed changes

rominf approved these changes Jun 24, 2026

View reviewed changes

rominf added this pull request to the merge queue Jun 24, 2026

Merged via the queue into main with commit b8fd635 Jun 24, 2026
8 checks passed

rominf deleted the supergoal/phase-3-readonly-and-nav branch June 24, 2026 17:47

rominf requested a review from Copilot June 24, 2026 17:50

Copilot AI reviewed Jun 24, 2026

View reviewed changes

michaelroy-amd mentioned this pull request Jun 27, 2026

feat(dash): IA redesign — 5-tab shell, inline Details, live TTFT/TPOT, Observe #62

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

§1: read-only ROCm tools + nav/session slash commands + parity map#31

§1: read-only ROCm tools + nav/session slash commands + parity map#31
rominf merged 14 commits into
mainfrom
supergoal/phase-3-readonly-and-nav

michaelroy-amd commented Jun 21, 2026

Uh oh!

rominf left a comment

Uh oh!

rominf left a comment

Uh oh!

rominf left a comment

Uh oh!

volen-silo left a comment

Uh oh!

michaelroy-amd commented Jun 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

rominf left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

michaelroy-amd commented Jun 21, 2026

Uh oh!

rominf left a comment

Choose a reason for hiding this comment

Uh oh!

rominf left a comment

Choose a reason for hiding this comment

Uh oh!

rominf left a comment

Choose a reason for hiding this comment

Uh oh!

volen-silo left a comment

Choose a reason for hiding this comment

Uh oh!

michaelroy-amd commented Jun 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

rominf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants