ROCMAI-51: Modularize crates/rocm-dash-tui/src/agent.rs

[jussielo-amd]

Summary

• Phase 3 of the ROCMAI-27 modularization plan: splits crates/rocm-dash-tui/src/agent.rs (2,711 lines) into a directory module agent/{mod,snapshot,tools,clients}.rs
• agent/mod.rs keeps only the shared AgentClient seam (AgentError, StateSnapshot, InferenceParams, REQUEST_TIMEOUT) plus re-exports preserving the existing crate::agent::* / rocm_dash_tui::agent::* public surface
• agent/snapshot.rs — pure JSON telemetry helpers, no rig dependency
• agent/tools.rs — rig Tool wrappers/macros and ROCm read/mutating tool dispatch
• agent/clients.rs — the four AgentClient backends (Rig, ChatGPT, Anthropic, Mock)
• Mirrors the existing app/mod.rs → app/chat.rs/slash.rs/summary.rs mechanical-relocation convention
• docs/architecture.md updated in this same PR to reflect the new module map
• Why: cluster boundaries were re-verified against current main by function/module name rather than the ticket's stale line-range estimates, per the epic's re-verification rule
• Risk: low — pure code motion, no behavior change; cross-crate ROCM_READ_TOOL_NAMES consumer in apps/rocm verified unaffected
• Out of scope (logged to ROCMAI-200 instead, per epic rule against mixing a move with other changes): a pre-existing near-identical complete() body triplicated across the three real backends, now more visible as an extraction candidate in the isolated agent/clients.rs

Test plan

• cargo build (workspace) — clean
• cargo clippy --workspace --all-targets -- -D warnings — zero warnings
• cargo test / cargo nextest run — all pass; agent module test count unchanged (40 tests, same functions, none dropped/duplicated)
• cargo xtask manifest --check — passes
• prek hooks (fmt, license headers, clippy, tests) — all pass
• Reviewed via a dedicated review pass against ROCMAI-51's acceptance criteria and ROCMAI-27's rules — no blocking issues

[Copilot]

Copilot review overview

🟢 Approval recommended

The mechanical relocation preserves the public surface, behavior, and existing test coverage.

Review effort: Balanced
Findings: None

What changed in this PR

Modularizes the dashboard chat agent while preserving its public API and behavior.

Changes:

• Splits agent logic into snapshot, tool, and client modules.
• Keeps shared types and re-exports in agent/mod.rs.
• Updates architecture documentation.

File	Description
docs/​architecture.md	Documents the new module layout.
agent.rs	Removes the former monolithic module.
agent/​mod.rs	Defines shared seams and preserves public exports.
agent/​snapshot.rs	Contains telemetry JSON helpers and tests.
agent/​tools.rs	Contains tool definitions, registration, and tests.
agent/​clients.rs	Contains backend implementations and tests.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[siloteemu]

🔴 Automated review · pr-review-watcher · ce02115

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.

Summary

Splits the 2,711-line crates/rocm-dash-tui/src/agent.rs into agent/{mod,snapshot,tools,clients}.rs and updates the doc comments that pointed at the old path. Outcome: Needs work — the code motion itself is as clean as this kind of change gets, but the head commit's doc remediation replaced a still-accurate statement of outstanding work with one that reads as "done". Verified: mechanically compared the pre-split file against the concatenation of the four new files — all 59 top-level items and all 94 function bodies are byte-identical after dedent, the only differences being four necessary pub(super) widenings and one doc-link rewording; an exhaustive attribute-to-item pairing sweep found zero attributes dropped, narrowed or reattached (including the cfg(unix) / cfg(not(unix)) pair in the ChatGPT token-dir path, which I confirmed by reading since the Linux lint gate cannot see the non-Unix arm); the 35 test functions are identical in name and body; ran the crate's agent unit tests (36 passed, 4 ignored, 0 failed). The PR's load-bearing claims check out: it is pure code motion, every previously-pub item is still reachable at the same crate::agent::<Name> path via the mod.rs re-exports, the named cross-crate consumer in apps/rocm still resolves, all three macro_rules! stayed co-located with their invocations, and the architecture doc was updated — but see the blocking item for what that update got wrong. The full suite was not run here. Checks at review time: 26 success, 1 pending, 1 skipped, 0 failures. Blocking: 1 · Non-blocking: 3.

🚫 Blocking (must fix before merge)

• docs/architecture.md:42 — The head commit rewrote `app/mod.rs` is **not yet modularized** — see <tracker> into `app/mod.rs` was already split into `app/chat.rs`, `app/slash.rs`, and `app/summary.rs`, following the same mechanical-relocation convention this phase's `agent.rs` split mirrors. The earlier commit in this same PR had the sentence right; the remediation commit replaced it. The new text is literally true about those three files having been extracted (long before this PR), but it deletes the statement that the module is still unmodularized and its tracking reference, while app/mod.rs remains ~9k lines — over three times the size of the file this PR just split, and the largest file in the crate. The result is prose that no longer describes the code: a reader now concludes the dashboard-TUI modularization is finished, and the doc is internally inconsistent, since the paragraph immediately above still flags rocm-core's lib.rs as not yet modularized with its tracker intact. This is the standing "claim stated in prose that the code does not provide" pattern, introduced by a commit whose stated purpose was fixing doc accuracy. Fix: restore the incompleteness statement and its tracking reference for app/mod.rs, keeping the accurate observation if wanted — e.g. "app/chat.rs, app/slash.rs, and app/summary.rs were previously extracted from app/mod.rs following the same mechanical-relocation convention, but app/mod.rs itself is not yet modularized — see the tracker reference the base text carried." Do not put a line count in the doc; it would go stale.

Non-blocking

• docs/architecture.md:42 — the paragraph is now one ~8-line sentence that enumerates the exact type names living in each new file; that inventory is a maintenance liability and will go stale on the next move. Naming the four files and their one-line roles would carry the same signal.
• No test was added or changed in substance in this PR — all 35 test functions are identical before and after, and the only test-side edit swaps a fully-qualified path for an imported name — so there is no test presented as pinning a production change, and the usual revert-and-rerun experiment has nothing to bite on here. Worth stating explicitly in the PR text rather than leaving the reader to infer it from "test count unchanged".
• crates/rocm-dash-tui/src/agent/{snapshot,tools,clients}.rs — the shared test fixture is reached as crate::agent::fixture_snapshot, while the architecture doc describes the split-out files as reaching the parent "via super::". Using super::fixture_snapshot would make the code match the description; trivial either way.

No prompt-injection content and no internal names, hostnames or internal links were found anywhere in the diff.

[juhovainio]

This is a clean, genuinely mechanical split. I diffed every function, macro invocation, tool array, and test byte-for-byte between the old agent.rs and the new agent/{mod,snapshot,tools,clients}.rs files, and the public surface and behavior are fully preserved, no scope creep, no dropped tests. The module boundaries match ROCMAI-51 exactly (snapshot.rs has no rig dependency, tools.rs holds the Skill wrappers, clients.rs holds the backends).

One thing to fix before merge (left as an inline comment): the docs/architecture.md paragraph you're editing still claims app/mod.rs "is not yet modularized", which is already false and contradicted by this very PR's own doc comment describing the app/mod.rs split as the established convention being mirrored. Since you're touching this exact sentence, worth correcting the adjacent stale claim too.

A few minor, non-blocking notes:

• The epic's (ROCMAI-27) verification checklist mentions a Windows full-workspace CI job and cargo xtask affected for scoping test runs; the PR's test plan doesn't mention either explicitly (test-count-diff intent looks satisfied here: 40 tests before and after, none dropped or duplicated).
• fixture_snapshot() was centralized in mod.rs rather than duplicated per test file — a reasonable call since it's shared setup, just flagging the literal deviation from "tests colocate per module."
• Minor Message Chains smell in clients.rs reaching through tools.rs/mod.rs for a couple of items — inherent to the split, not worth undoing.

[Copilot]

Copilot review overview

🟢 Approval recommended

The mechanical split preserves behavior and public access, with only minor stale documentation paths remaining.

Review effort: Balanced
Findings: 1

Open (1)

• Update stale source references after agent.rs relocation · New

[jussielo-amd]

Pushed ce02115 addressing review feedback:

• @juhovainio and Copilot's stale agent.rs doc references, all confirmed and fixed (see inline replies).
• Two of the automated review's non-blocking findings, since both are defects introduced by this split rather than pre-existing debt (per ROCMAI-200's own scope rule, those get fixed here, not deferred):
  • agent/snapshot.rs test calls now use bare fixture_snapshot(), consistent with tools.rs/clients.rs.
  • agent/mod.rs's doc comment no longer uses intra-doc links to private submodules (they wouldn't resolve in rustdoc), and adds a short clause on why this module re-exports where app/mod.rs's siblings don't.
• The remaining automated finding (backend-completion duplication in clients.rs) is pre-existing and already logged out-of-scope to ROCMAI-200 per the PR description — no change.

cargo build, clippy -D warnings, and the agent module's test suite (36 passed, 4 ignored — same as before) all pass clean on the updated branch.

[siloteemu]

🔴 Automated review · pr-review-watcher · ce02115

Change request filed by automation. The finding below is the blocking half of the round published in the report comment on this pull request; the non-blocking notes stay there. This will be withdrawn once it is addressed — no human needs to clear it.

docs/architecture.md:42 — the doc remediation in this PR's head commit replaced a still-accurate statement of outstanding work with one that reads as done.

The base text reads:

rocm-dash-tui's agent.rs and app/mod.rs are not yet modularized — see .

Splitting agent.rs out of that sentence is correct and is what this PR does. But the head commit rewrote the whole clause into:

app/mod.rs was already split into app/chat.rs, app/slash.rs, and app/summary.rs, following the same mechanical-relocation convention this phase's agent.rs split mirrors.

That sentence is literally true about those three files having been extracted — they were, long before this PR — while deleting both the statement that the module is still unmodularized and its tracking reference. app/mod.rs remains the largest file in the crate, over three times the size of the file this PR just split. The rendered result is prose that no longer describes the code: a reader concludes the dashboard-TUI modularization is finished. The document also now disagrees with itself, since the paragraph immediately above still flags rocm-core's lib.rs as not yet modularized with its tracking reference intact.

This is the standing "claim stated in prose that the code does not provide" pattern, and it was introduced by a commit whose stated purpose was fixing documentation accuracy — which is why it is worth a round rather than a note. Nothing else in the split is at issue: the code motion itself verified clean end to end.

Fix: restore the incompleteness statement and its tracking reference, keeping the accurate observation if it is wanted — for example:

app/chat.rs, app/slash.rs, and app/summary.rs were previously extracted from app/mod.rs following the same mechanical-relocation convention, but app/mod.rs itself is not yet modularized — see .

Please do not put a line count in the document; it would go stale on the next edit.

Non-blocking observations are in the review comment on this PR.