Repository navigation
ROCMAI-864: rename SKILL_NAMES to TELEMETRY_TOOL_NAMES and drop the stale Skill wording - #901
Conversation
After the dash-tui skills subsystem was removed, SKILL_NAMES held only the four telemetry tools (gpu_status, list_instances, bench_summary, tokens_per_watt), so the name described something that no longer exists. Rename the constant, its re-export in agent/mod.rs and every user in agent/tools.rs and agent/clients.rs (including the wire-level registry test), and fix its doc comment. The tool name strings themselves are untouched, so the names sent to the model are unchanged. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
The four telemetry tools (gpu_status, list_instances, bench_summary, tokens_per_watt) were called skills in comments, test names and messages, and in docs/architecture.md. Reword those to plain "tool" wording, name the four tools and the TELEMETRY_TOOL_NAMES table in the architecture entry for agent/tools.rs, and point the lemonade port comment at the chat probe in rocm_dash_tui::llm instead of the removed rocm_dash_tui::skills module. Renamed test leaf names, all in rocm-dash-tui: - skill_names_are_unique_and_non_empty -> telemetry_tool_names_are_unique_and_non_empty - rocm_read_tool_names_are_complete_and_disjoint_from_skills -> rocm_read_tool_names_are_complete_and_disjoint_from_telemetry_tools - annotate_reply_appends_deduped_skills -> annotate_reply_appends_deduped_tools - mock_tool_calling_answer_cites_skill -> mock_tool_calling_answer_cites_tool - transcript_renders_skill_annotation_on_its_own_line -> transcript_renders_tool_annotation_on_its_own_line The tracing field skills_fired on the "chat request complete" event is now tools_fired. The tool name strings sent to the model, the "via:" annotation text and every other behaviour are unchanged. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues block the telemetry terminology cleanup.
No open issues 🎉
What changed in this PR
Updates dash-TUI terminology after the skills subsystem removal in #882, naming the remaining telemetry tools accurately without changing tool registration or reply annotations.
Changes:
- Renames
SKILL_NAMEStoTELEMETRY_TOOL_NAMESacross its definition, re-export, and callers. - Removes stale “Skill” wording from documentation, comments, assertions, and five test names.
- Renames the structured tracing field
skills_firedtotools_fired.
| File | Description |
|---|---|
docs/architecture.md |
Documents the four telemetry tools and their renamed name table. |
crates/rocm-dash-tui/src/ui/tabs/chat.rs |
Updates annotation terminology and the corresponding test name. |
crates/rocm-dash-tui/src/agent/tools.rs |
Renames the telemetry name table and updates comments and tests. |
crates/rocm-dash-tui/src/agent/mod.rs |
Re-exports TELEMETRY_TOOL_NAMES. |
crates/rocm-dash-tui/src/agent/clients.rs |
Updates identifier usage, terminology, tests, and the tracing field. |
crates/rocm-dash-collectors/src/lemonade.rs |
Corrects a comment to reference the chat probe rather than the removed skills module. |
Review effort: Balanced
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
Looks good. This is a clean rename of SKILL_NAMES to TELEMETRY_TOOL_NAMES: the constant, its re-export, every user, and the registry test are updated together, and the tool wire names are unchanged. The leftover "Skill" wording in comments and docs is cleaned up in the same change, and it matches the scope of ROCMAI-864. CI is green on everything that ran.
The only observable change is the tracing field rename noted inline, which the PR description already calls out. This is a read of the diff plus CI status, not a runtime check.
| elapsed_ms = started.elapsed().as_millis() as u64, | ||
| reply_len = reply.len(), | ||
| skills_fired = skills.len(), | ||
| tools_fired = fired_tools.len(), |
There was a problem hiding this comment.
Nit: this renames the structured tracing field skills_fired to tools_fired, the only non-cosmetic change in the PR. Fine since nothing in-repo reads it; flagging only so it stays a conscious choice if any external log filter keys on the old name.
There was a problem hiding this comment.
Agreed it should not be a silent change: 5f11053 restores the skills_fired tracing field name, so the PR now has no observable log change, and the body says so. Renaming that key can be its own deliberate change if anyone wants it.
The "chat request complete" tracing field is a log key that external filters may match on, so the rename is dropped to keep this PR free of observable changes. Nothing else changes. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
Summary
ROCMAI-864. The dash-TUI skills subsystem and its
list_skillsandskill_planagent tools were removed in #882. After that, the constantSKILL_NAMESincrates/rocm-dash-tui/src/agent/tools.rsholds only the four telemetry tools (gpu_status,list_instances,bench_summary,tokens_per_watt), so its name, its doc comment ("All Skill names") and a number of comments, test names and assertion messages that call these tools "Skills" describe something that no longer exists.docs/architecture.mdstill describedagent/tools.rsas holding therig::tool::Tool"Skill" wrappers, and a comment in the Lemonade collector pointed at arocm_dash_tui::skillsmodule that is gone. This change renames the constant toTELEMETRY_TOOL_NAMESand removes the stale wording. It is a rename and wording change only.What changed
The first commit renames
SKILL_NAMEStoTELEMETRY_TOOL_NAMESand fixes its doc comment: the definition and the "mirrors" doc link inagent/tools.rs, the re-export inagent/mod.rs, and every user inagent/tools.rsandagent/clients.rs, including the wire-level registry test. The second commit rewords the rest: module and section comments, theFiredLogdoc, theannotate_replydoc and parameter name, test messages and comments inagent/tools.rs,agent/clients.rsandui/tabs/chat.rs, thedocs/architecture.mdentry foragent/tools.rs(now naming the four telemetry tools andTELEMETRY_TOOL_NAMES), and the Lemonade port comment incrates/rocm-dash-collectors/src/lemonade.rs(now points at the chat probe inrocm_dash_tui::llm).Five test leaf names that were plainly false after the removal were renamed, all in
rocm-dash-tui:skill_names_are_unique_and_non_emptybecametelemetry_tool_names_are_unique_and_non_empty;rocm_read_tool_names_are_complete_and_disjoint_from_skillsbecamerocm_read_tool_names_are_complete_and_disjoint_from_telemetry_tools;annotate_reply_appends_deduped_skillsbecameannotate_reply_appends_deduped_tools;mock_tool_calling_answer_cites_skillbecamemock_tool_calling_answer_cites_tool; andtranscript_renders_skill_annotation_on_its_own_linebecametranscript_renders_tool_annotation_on_its_own_line. No other test leaf names changed.Left alone on purpose:
retired_skill_registry_tools_are_not_advertisedand the comments around it, which refer accurately to the retiredlist_skillsandskill_plantools; and every reference to the real repository skills (skills/rocm-cli-assistant,skills/rocm-doctor,SKILL.md, skillscope,docs/skill-quality.md).docs/llm-tool-use.mdandskills/rocm-cli-assistant/SKILL.mdcontain nothing that depends on the old constant name.Verification
Run locally on Linux:
cargo fmt --all --check;cargo clippy -p rocm-dash-tui -p rocm-dash-collectors --all-targets -- -D warnings;cargo test -p rocm-dash-tui --no-fail-fast; the test-list diff between origin/main and this branch, which shows exactly the five intentional leaf renames above and nothing else;cargo xtask manifest --check;cargo xtask check-crate-edges;cargo xtask lychee(markdown changed); and the leak scan overgit diff origin/main...HEAD. The results are in the hand-off report. The full workspace gate,scripts/smoke_local.pyand the Windows cross-check were not run locally; CI carries them. A repository-wide grep confirms noSKILL_NAMESor "Skill names" remains.User-visible behaviour
None, so no Gherkin scenario applies: this is a purely internal rename and wording change. The tool name strings sent to the model are byte-identical, and the wire-level registry test
run_agent_request_registers_tool_sets_and_enforces_turn_limitincrates/rocm-dash-tui/src/agent/clients.rsstill asserts the exact tool set sent to the model; it differs only in the renamed identifier. The "via:" annotation text is unchanged.Notes for reviewers
Read the commits in order: the first is the mechanical identifier rename (plus the rustfmt re-wrap and import re-sort it forces), the second is wording, test leaf names and docs, and the third restores the
skills_firedtracing field name on the "chat request complete" event infinish_agent_request. The second commit had renamed that field totools_fired; because it is an externally observable log key, the third commit keeps the original name so this PR has no observable change. The field name is therefore the one place that still says "skills" for what is now a count of fired tools, left deliberately. No other open PR touchesagent/,ui/tabs/chat.rsor the Lemonade collector; thirteen open PRs touchdocs/architecture.mdelsewhere, but none edits theagent/tools.rsentry, so any conflict there should be a trivial context one.