[jussielo-amd]

Summary

The dashboard assistant advertised two agent tools, list_skills and skill_plan, that gave the model install recipes nothing in this repository can run and that do not match how the product installs anything. skill_plan install-lemonade returned steps to curl the GitHub releases API and tar an archive into a per-user directory, but Lemonade is installed with rocm engines install lemonade, which also keeps its llama.cpp backend pins aligned and goes through the approval flow. The auto-config-endpoint plan described config writes that nothing ever applies. The helper that was meant to execute the plans (apply_install_lemonade) and the rocm skill run command that the comments cite do not exist in this repository, so crates/rocm-dash-tui/src/skills.rs has been plan-only since the dashboard was merged in. Handing the model invented shell commands is also exactly what the assistant policy in docs/llm-tool-use.md rules out.

This removes the two tools and then the whole subsystem behind them, rather than building an executor: an executor would duplicate rocm engines install lemonade and bypass its approval flow.

Completes ROCMAI-834.

What moved

Nothing moved except the three endpoint constants; everything else is deleted.

Symbol	Result
ListSkillsTool, SkillPlanTool, SkillPlanArgs (agent/tools.rs), their re-exports in agent/mod.rs, their registration in register_telemetry_tools, their two entries in SKILL_NAMES (now four names)	removed
skills.rs (manifest types and parser, builtin_skills, build_plan, the two TOML manifests, the embeddable-artifact chain, auto_config_change, AutoConfigPlan, auto_config_plan, ConfigChange) and tests/fixtures/lemonade_release_latest.json	removed
LEMONADE_PORT, VLLM_PORT, ROCM_SERVE_PORT	removed, nothing but the deleted code read them
LEMONADE_ENDPOINT, VLLM_ENDPOINT, ROCM_SERVE_ENDPOINT	moved into llm.rs, same values, now private (their only user is the local endpoint probe there)
rocm-deps dependency of rocm-dash-tui	removed from Cargo.toml and Cargo.lock; skills.rs was its only user. The ("rocm-dash-tui", "rocm-deps") entry in the crate_edges.rs allowlist goes too, because that check fails on an allowlist entry with no matching edge
stale comments naming skills.rs / rocm skill run	xtask/src/ascii_output_contract.rs now cites apps/rocm/src/remote/doctor.rs (which embeds the ASCII-only tests/remote-ssh/examination.json from its test module) as the example; the ignored live scrape test in crates/rocm-dash-collectors/src/lemonade.rs names rocm engines install lemonade

toml stays as a dependency of rocm-dash-tui because app/chat.rs still uses it to persist the chat endpoint; only its Cargo.toml comment changed.

Verification

Ran: cargo fmt --all --check; cargo clippy -p rocm-dash-tui -p rocm-dash-collectors -p xtask --all-targets --locked -- -D warnings; cargo test -p rocm-dash-tui -p rocm-dash-collectors --no-fail-fast --locked (all green); cargo test -p xtask ascii and cargo test -p xtask crate_edges (green; every_embedded_data_file_is_ascii included); cargo xtask manifest --check and cargo xtask check-crate-edges (exit 0).

Test inventory against origin/main (leaf names of dash-tui and dash-collectors tests): 1180 before, 1166 after. Removed are the 13 skills::tests::* tests and the two agent::tools::tests tests that pinned the retired tools (skill_tools_expose_both_demo_skills, skill_tool_names_registered_in_skill_names); added is agent::tools::tests::retired_skill_registry_tools_are_not_advertised. In agent::clients::tests::run_agent_request_registers_tool_sets_and_enforces_turn_limit the registry-size pin drops from six to four and the set of tool names in the request actually sent to the model is asserted equal to the union of the three name tables (which rules out list_skills and skill_plan and any tool registered outside the tables). Both new assertions were shown to fail: renaming TokensPerWattTool::NAME to list_skills turned the tools test red, and temporarily registering a stand-in list_skills tool turned the wire-level test red; both edits were reverted.

Coverage: cargo xtask coverage measures the whole workspace, which I did not run; instead I measured cargo llvm-cov -p rocm-dash-tui on origin/main and on this branch with the same flags the xtask uses. dash-tui line coverage was 91.89% before and 91.82% after (26728/29086 vs 26404/28755 lines), against a committed floor of 89.33% with a 0.2pp tolerance, so no re-bless. The workspace-wide check is left to CI.

Not run: cargo clippy -p rocm-dash-tui --target x86_64-pc-windows-msvc. The cross-check fails in the aws-lc-sys build script (it needs a Windows C toolchain that is not installed here), before reaching this crate; nothing in the diff is cfg(windows)-gated, and skills.rs was not gated either. The Windows CI job covers it. Also not run: cargo xtask e2e and the full workspace gate, which CI carries.

User-visible behaviour

The set of tools the assistant advertises to the model changes: list_skills and skill_plan are gone, leaving the four telemetry tools plus the existing ROCm read and mutating tools. No Gherkin scenario applies. I searched tests/e2e-cucumber/features and the step definitions: nothing pins the dashboard assistant's tool set. Scenario chat-06 (@id:chat-tool-definitions-accepted) attaches a client-supplied gpu_status definition to a request for a served model and checks that such a request is accepted; it does not see the dashboard's registry. The dash.feature chat scenarios (for example dash-02) run against the CLI's --chat-mock offline assistant, which does not build the Rig tool list. The advertised set is only observable in the request sent to a model, and that is asserted in the unit test above, which uses a recording mock completion model and checks the tool definitions on the wire. docs/llm-tool-use.md, skills/rocm-cli-assistant/SKILL.md and docs/architecture.md do not mention either tool or skills.rs, so no text in them needed to change.

Notes for reviewers

Read the commits in order. The first removes the two tools from the agent (registry, registration, tests) and compiles and passes on its own with skills.rs still present. The second deletes skills.rs and the fixture, moves the endpoint constants into llm.rs, and drops the now-unused rocm-deps dependency with its allowlist entry and lockfile line. The third fixes the two stale comments. The fourth tightens the wire-level registry test in agent/clients.rs to compare the exact set of tool names sent to the model with SKILL_NAMES plus the read and mutating ROCm name tables (re-registering a list_skills tool turns it red), and corrects one call-site comment.

The ticket also asked for the docs/architecture.md entries that mention the module to be updated; there are none (the module was never listed there), so that file is untouched. The "Skill" wrappers wording for agent/tools.rs there is still accurate for the four telemetry tools that remain.

Other open pull requests touching the same files: #801 (approved) rewrites run_rocm_read_tool in agent/tools.rs and adds tests near the end of its test module, away from the code removed here, so I expect it to merge cleanly. #881 edits a comment in agent/tools.rs directly after SKILL_NAMES and the module doc of agent/mod.rs; the first hunk sits right next to the array this change shrinks, so a trivial conflict is possible. #855 (draft) only changes example version strings in doc comments of skills.rs (11.5.1 to 2026.44.1 in three places); if this lands first, that hunk is moot and the file simply stays deleted, and the pin itself lives in rocm-deps, which this change does not touch. #631 edits the header and imports of crates/rocm-dash-collectors/src/lemonade.rs and keeps the same comment this change rewrites in its context, so check that file if it conflicts. #447 and #563 edit neighbouring lines of crate_edges.rs and the dev-dependencies of rocm-dash-tui/Cargo.toml, #817 and #521 other entries of the ASCII-contract tables, none overlapping.

[Copilot]

🟢 Approval recommended

No active review issues block approval of the unused skills subsystem removal.

No open issues 🎉

What changed in this PR

Removes the dashboard assistant’s unused, plan-only skills subsystem so it no longer advertises installation recipes that diverge from the supported engine-install flow. Existing telemetry and ROCm tools remain available.

Changes:

• Removes list_skills, skill_plan, their implementations, and the unused manifest/planning subsystem.
• Preserves local endpoint detection by moving its constants into llm.rs, and removes the unused dependency.
• Strengthens tool-registry regression checks and corrects stale comments.

File	Description
xtask/​src/​crate_edges.rs	Removes the obsolete dash-TUI → rocm-deps allowlist entry.
xtask/​src/​ascii_output_contract.rs	Updates a comment to cite a surviving fixture example.
crates/​rocm-dash-tui/​tests/​fixtures/​lemonade_release_latest.json	Deletes the retired subsystem’s release fixture.
crates/​rocm-dash-tui/​src/​skills.rs	Deletes unused manifests, planning helpers, and associated tests.
crates/​rocm-dash-tui/​src/​llm.rs	Keeps endpoint constants locally with unchanged values and probe order.
crates/​rocm-dash-tui/​src/​lib.rs	Removes the skills module export.
crates/​rocm-dash-tui/​src/​agent/​tools.rs	Removes retired tools and adds assertions against their advertisement.
crates/​rocm-dash-tui/​src/​agent/​mod.rs	Removes retired tool re-exports.
crates/​rocm-dash-tui/​src/​agent/​clients.rs	Checks that advertised tool names exactly match the remaining registry.
crates/​rocm-dash-tui/​Cargo.toml	Drops rocm-deps and clarifies why toml remains.
crates/​rocm-dash-collectors/​src/​lemonade.rs	Corrects installation guidance in a live-test comment.
Cargo.lock	Removes dash-TUI’s rocm-deps dependency entry.

_{Review effort: Balanced}

[juhovainio]

Looks good. The removal is clean: I found no leftover references to list_skills, skill_plan, rocm skill run or the deleted module in the code, README, docs/ or skills/, and the rocm-deps dependency removal is consistent across Cargo.toml, Cargo.lock and the crate-edges allowlist. The wire-level test now asserts the exact set of tools sent to the model, which is a nice tightening. The scope matches ROCMAI-834.

One small non-blocking naming nit is inline. Also, retired_skill_registry_tools_are_not_advertised is now largely redundant with the exact-set assertion, so it could be dropped if you like.

This is based on reading the diff only; build/test CI jobs were still pending when I looked.

[juhovainio]

Non-blocking: SKILL_NAMES (and its doc comment above, "All Skill names") now holds only the four telemetry tools, so the name is misleading with the skills subsystem gone. Consider TELEMETRY_TOOL_NAMES in a follow-up; docs/architecture.md also still refers to "Skill" wrappers.

[jussielo-amd]

Agreed, the name no longer fits now that the skills subsystem is gone. I kept this PR to the removal and filed the rename plus the remaining "Skill" wording in the docs and comments as ROCMAI-864, so it can go in as one focused change.

On dropping retired_skill_registry_tools_are_not_advertised: I would keep it for now. The wire-level assertion compares what the model receives against the name tables, so if someone re-registered a retired tool and added its name to SKILL_NAMES in the same change, the exact-set check would still pass. The unit test pins the retired names out of the tables themselves. I am happy to fold it into the exact-set check as part of the rename if you would rather have a single assertion.

[nowycondro]

🤖 Automated review (Claude Code) · b61f9b7

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — needs work

Full review of the whole change (merge-base 872515423a9f … head b61f9b76406744fab1a2b33c212ad8775a6f754a).
Implements ROCMAI-834 in full: every scope item the ticket lists (remove list_skills/skill_plan and the skills.rs module behind them, keep the three endpoint constants, fix the two stale comments, drop the now-unused rocm-deps dependency and its crate_edges.rs allowlist entry) and every acceptance criterion, including the two items absorbed from ROCMAI-760, are satisfied. The PR's own "Completes ROCMAI-834" claim holds up against the ticket text.

Blocking

• A self-hosted E2E hardware lane fails at this exact head commit — E2E tests (Strix Halo, Ubuntu, release, current) and E2E tests (Strix Halo, Ubuntu, nightly, current) (workflow run 41836355803, jobs 126596324987 and 126596325108) each report Reconciliation: 7 xfail (failed as expected), 0 XPASS, 1 unexpected failure(s). FAIL: 'serve-stop-leaves-no-background-helper' was expected to pass on this host but FAILED — a regression. Two sibling lanes in the same workflow (Strix Halo Windows, Strix Halo WSL2) and the non-GPU build/test/clippy jobs are green; two more hardware lanes (MI300X, W7900) were still queued as of this review. Confidence 100 (read directly from both jobs' own logs). This scenario exercises rocm serve --stop / background-helper process cleanup, which has no connection to anything in this diff — the diff only touches the dash-TUI assistant's agent-tool registry and the now-deleted skills.rs — so it is very likely a pre-existing or infra-level issue this change doesn't cause. It is reported as blocking per CI-at-face-value policy (and this repo's own AGENTS.md §12: "A red check means 'not ready' until resolved... handle infrastructure flakes by rerun or maintainer escalation with evidence"). Fix: confirm whether this scenario also fails on origin/main's own latest CI (or rerun the job) before merging; if it's a pre-existing/infra flake unrelated to this change, resolve per §12 rather than let it block silently.

Non-blocking

None

Decisions for the author

None

Positive signals

• The four-commit structure is unusually reviewer-friendly: each commit (tool removal, module deletion, stale-comment fixes, test tightening) compiles and passes standalone, in an order that lets each be read as a self-contained step rather than one undifferentiated diff.
• Every quantitative claim in the PR description checked out independently: the "13 skills::tests::* + 2 agent::tools::tests removed, 1 added" nets to exactly the claimed 1180→1166 test count, and repo-wide greps confirmed zero remaining references to rocm skill run, list_skills, skill_plan, crate::skills::, and the three retired port constants — not just within the touched files.
• The wire-level test in agent/clients.rs was tightened from a one-directional "contains" check to exact BTreeSet equality against the three canonical tool-name tables, closing a real gap (a stray tool registered outside the tables would previously have gone undetected), and was shown to actually fail against a reintroduced list_skills tool.

Deployment notes

• User-visible change: the dashboard assistant no longer offers list_skills or skill_plan to the underlying model. No Gherkin scenario pinned the old tool set, so none needed updating (verified against tests/e2e-cucumber/features).
• No migration or config change; the three local-endpoint-detection constants moved into llm.rs with byte-identical values, so detect_local_endpoint() behaviour is unchanged.

What this covered

Every file in the diff read completely (Cargo.lock, crates/rocm-dash-collectors/src/lemonade.rs, crates/rocm-dash-tui/Cargo.toml, crates/rocm-dash-tui/src/agent/{clients,mod,tools}.rs, crates/rocm-dash-tui/src/lib.rs, crates/rocm-dash-tui/src/llm.rs, crates/rocm-dash-tui/src/skills.rs (deleted — read in full at its last live commit), tests/fixtures/lemonade_release_latest.json (deleted), xtask/src/ascii_output_contract.rs, xtask/src/crate_edges.rs), plus surrounding code needed to judge them in place (app/chat.rs, apps/rocm/src/remote/doctor.rs and its fixture, the engines install command, docs/architecture.md, docs/llm-tool-use.md, skills/rocm-cli-assistant/SKILL.md, this repo's AGENTS.md), plus repo-wide greps for every removed symbol/string. Fetched read-only via refs/pull/882/head, never checked out.

Fan-out: one independent worker reviewed agent/tools.rs + agent/mod.rs + agent/clients.rs and returned no findings. Two further groups (the skills.rs/llm.rs/lib.rs/Cargo.toml/Cargo.lock cluster, and the xtask/lemonade.rs comment-fix cluster) could not be spawned as independent workers — the shared environment's concurrent-subagent limit was hit on both the first attempt and one retry each — so those two were reviewed in this same driver context instead, which is a weaker guarantee than the first group's result; stating that explicitly rather than presenting all three as equally independent.

Did not run: a deep "prior changes" pass (no earlier independent review round on this diff exists to compare against); cargo check/clippy/test locally (no Rust toolchain available in this environment) — relied on the PR's own CI results instead, read directly from the Actions API. Ticket ROCMAI-834 was retrieved directly from Jira and its scope/acceptance criteria compared point-by-point against the diff. The PR's two existing GitHub reviews were read only to confirm no duplicate automated review already existed at this head; their content was not used to derive or steer any finding above, and this report is not reconciled against their comments.

🤖 Generated with Claude Code