ROCMAI-83: extract mcp.rs from apps/rocmd/src/lib.rs

[jussielo-amd]

Summary

Sixth PR of Phase 5 (ROCMAI-83, part of the modularization epic ROCMAI-27). Pulls the MCP stdio server, tool schema table, tool dispatch, and the rocm-subprocess capture/argv-building helpers behind the MCP tools into their own mcp.rs module.

• No behavior change. Reaches back into still-crate-root items (build_bridge_snapshot, gather_gpu_snapshot_for_config, bridge_engine_inventory, load_managed_services, stop_managed_service) via crate::, since the modules that will eventually own those (common.rs/persistence.rs/service.rs) are separate, independent PRs not present on this branch (see ROCMAI-83: extract persistence.rs from apps/rocmd/src/lib.rs #477, ROCMAI-83: extract common.rs from apps/rocmd/src/lib.rs #479, ROCMAI-83: extract webhook.rs from apps/rocmd/src/lib.rs #480, ROCMAI-83: extract cli.rs from apps/rocmd/src/lib.rs #481, ROCMAI-83: extract sandbox.rs from apps/rocmd/src/lib.rs #483).
• CommandCapture now lives in mcp.rs (it was mcp.rs-authored originally); sandbox-territory code still inline in lib.rs that takes it as a parameter type is repointed to mcp::CommandCapture.
• 15 tests that exercise this module's own logic (MCP tool-schema/dispatch shape, read-only-verb classification, install_sdk/install_engine/launch_server/watcher_enable argv building, and read_tail_lines) moved into mcp.rs's own #[cfg(test)] mod in this same PR. Tests that share a name prefix or construct these types but actually exercise Cli parsing, run_daemon, or watcher-domain logic stayed in lib.rs for their own later PRs.
• docs/architecture.md updated in this PR.

Independent of #477, #479, #480, #481, #483 — branched separately from main, not stacked, per the one-off PR approach for this phase.

Test plan

• cargo build (rocmd, and rocm+rocmd together to confirm the external API surface is untouched)
• cargo clippy --workspace --all-targets -- -D warnings
• cargo test -p rocmd — 130 tests, same count as before this change (129 passed + 1 pre-existing ignored)
• cargo xtask manifest --check
• cargo fmt / prek hooks clean
• tests/e2e-cucumber (CI)

[r0x0r]

Review (draft): MCP surface motion is clean, but CommandCapture is in the wrong module

The MCP extraction itself is solid and I found nothing wrong with the tool-surface motion. There is one placement decision that collides with #479 and #483, and it is worth settling before this comes out of draft — it is cheaper to move one struct now than to unpick it after either sibling merges.

CommandCapture belongs in common.rs, not mcp.rs

This PR puts pub(crate) struct CommandCapture at mcp.rs:846, on the grounds that it "was mcp.rs-authored originally". #479 puts the same struct at common.rs:204. Both are open against main.

Origin is the wrong test, and the phase already has the right one: common.rs admits anything used by ≥2 of the sandbox/mcp/service/watchers clusters. CommandCapture meets it on main, with the two consumer groups cleanly separated:

Cluster	Consumers on main	Destination PR
sandbox	sandbox_check_updates_value (lib.rs:1194), update_check_status (:1212), sandbox_driver_plan_value (:1252)	#483 → sandbox.rs
MCP	tool_result_from_command (:2332), command_capture_text (:2351), run_rocm_capture (:2373)	#484 → mcp.rs

So it is used by exactly two clusters, which is the rule, and #479's placement is the one that follows it.

The consequence is visible on this branch already: lib.rs:1197, :1215 and :1255 — all three sandbox-cluster functions — now say mcp::CommandCapture. Those three lines are precisely what #483 moves into sandbox.rs. If this lands first, #483's sandbox.rs has to reach for crate::mcp::CommandCapture, giving the sandbox module a hard dependency on the MCP module. That is the cross-cluster coupling Phase 5 exists to remove, and #483's body already assumes otherwise — it lists CommandCapture among the crate-root items it reaches back for, not an mcp:: item.

Worth noting this PR's own doc hunk agrees with #479 and not with its own code: it describes common.rs as holding "CommandCapture/command-timeout plumbing" while the code puts it in mcp.rs.

The fix is small: drop CommandCapture, run_rocm_capture, run_rocm_capture_for_paths and run_command_with_timeout from this PR and let #479 own them; keep tool_result_from_command and command_capture_text here, since those are genuinely MCP result-shaping and have no sandbox consumer.

How this merges with #479 today

Not silently, which is the one piece of good news — but not cleanly either. Both PRs edit the same signatures (sandbox_check_updates_value, update_check_status, sandbox_driver_plan_value) with different module paths, which is a plain text conflict; and #479 edits tool_result_from_command in place while this PR deletes it from lib.rs, which is an edit/delete conflict. Both surface at merge time rather than slipping through. The catch is that resolving them correctly requires knowing which home is right, so whoever hits the conflict inherits this decision at the worst moment. Settling it now turns a judgement call into a mechanical rebase.

The architecture-doc hunk, same as the rest of the phase

The doc text lists six extracted modules; this branch has one (apps/rocmd/src/ here is lib.rs, main.rs, mcp.rs). All six PRs in this phase rewrite the same single line, each branched independently from main, with cumulative text that is only true at merge order 477 → 479 → 480 → 481 → 483 → 484. Any of them merging out of order applies cleanly and leaves main asserting files that do not exist. Scoping each hunk to its own module fixes it at any order and turns the conflicts into trivial appends. Same note as on #479, #480, #481 and #483.

Verified clean

• MCP tool schema is byte-for-byte identical — all 16 tool schemas in rocm_mcp_tools(), every tool name, description, parameter name and required/optional flag, and every readOnlyHint/destructiveHint annotation. A silently renamed tool or dropped required parameter here would be an API break for any MCP client with no compile error to catch it, so it is worth stating that there is none.
• The approval gate is unchanged — mcp_tool_requires_direct_approval has the same six mutating-tool names, and ensure_rocm_command_is_read_only has the same branch arms. I checked set membership specifically: a tool drifting from the mutating set into the read-only set would be a privilege escalation (AGENTS.md §7 requires mutating actions to keep their approval flow), and none did.
• argv builders unchanged — build_install_sdk_args, build_install_engine_args, build_launch_server_args, build_watcher_enable_args; no flag added, dropped or reordered. read_tail_lines bounds arithmetic is identical.
• ensure_direct_mcp_call_allowed widens from private to pub(crate), which the extraction requires since lib.rs:371 calls it. The gate's logic is byte-identical — the visibility change is mechanical, not a loosening.
• Tests — 130 attributes conserved across the branch (115 + 15), matching the claimed 15 moved exactly; no #[ignore] added or dropped.
• Imports — VecDeque and BufRead dropped from lib.rs; I confirmed zero remaining references to either, so nothing fails under -D warnings. All five crate:: reach-backs (build_bridge_snapshot, gather_gpu_snapshot_for_config, bridge_engine_inventory, load_managed_services, stop_managed_service) are private in lib.rs and legally reachable from a child module.
• No TODOs, stubs or half-moved clusters despite the draft status.

---

🤖 by agent-hub on AMD AgentHub

[rominf]

🔴 Automated review · pr-review-watcher · 506e279

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

The PR moves the MCP stdio server, tool schema, tool dispatch, CommandCapture/run_command_with_timeout and the argv-building helpers, plus their 15 tests, out of apps/rocmd/src/lib.rs into a new apps/rocmd/src/mcp.rs. The code move is clean, but the line it adds to docs/architecture.md describes modules that do not exist. Outcome: Needs work. Reviewed: the whole change (prw-base...HEAD, one commit; apps/rocmd/src/lib.rs, apps/rocmd/src/mcp.rs, docs/architecture.md). Verified:

• cargo clippy -p rocmd --all-targets -- -D warnings is clean, and cargo test -p rocmd --lib gives 129 passed, 1 ignored.
• A line-by-line comparison shows the removed lib.rs code matches mcp.rs except for visibility, the crate:: paths, imports, the license header and one test helper.
• The test set is identical by name before and after: 127 tests became 112 in lib.rs plus 15 in mcp.rs.
• Every pub(crate) item is used from lib.rs.
• The crate:: calls back into lib.rs are exactly the five the commit message lists.
• No #[cfg] attributes were lost in the move, and no other docs or xtask/CI files reference the moved code.

Blocking: 1 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

• docs/architecture.md:34: the new line says persistence.rs, common.rs, webhook.rs, cli.rs and sandbox.rs are "Extracted so far". Only mcp.rs exists. At HEAD, apps/rocmd/src/ contains just lib.rs, main.rs and mcp.rs, and none of the other five paths exist on the base branch either. Everything the line assigns to those modules is still in lib.rs:

  • record_event and load_managed_services
  • Cli/Command, SandboxToolArg/SandboxToolPolicy, and run_cli/run_bin_cli/run_from_args
  • the sandbox code

  The line also says CommandCapture/command-timeout plumbing lives in common.rs, but this PR's code puts it in mcp.rs (mcp.rs:846, pub(crate) struct CommandCapture). The commit message agrees with the code, not the doc: it says the modules that will eventually own those items are separate PRs that are not on this branch. The code is what ships, so the doc is the side that must change. As written, merging leaves the architecture map claiming five modules that don't exist, which breaks AGENTS.md §5 (keep docs in sync with actual structure).

  Fix: list only mcp.rs as extracted, including that it currently hosts CommandCapture/run_command_with_timeout until a shared module exists. Move persistence.rs, common.rs, webhook.rs, cli.rs, sandbox.rs, service.rs and watchers.rs to "Still pending". Then let each sibling PR add its own entry when it lands.

Non-blocking

• apps/rocmd/src/lib.rs (capture_amd_smi_json, sandbox_check_updates_value, update_check_status, sandbox_driver_plan_value): GPU-snapshot and sandbox code now depends on mcp::CommandCapture and mcp::run_command_with_timeout. That is generic subprocess plumbing living in an MCP-named module. The commit message says this is temporary, but neither the code nor the doc says so. A one-line comment on CommandCapture saying it is due to move to a shared module would record that intent.
• apps/rocmd/src/mcp.rs (CommandCapture, run_command_with_timeout): the planned independent common.rs PR also claims ownership of these items (per the doc line). Whichever of the two lands second will have to move them again and will conflict on the same docs/architecture.md:34 line, so the order should be agreed before either merges.
• apps/rocmd/src/mcp.rs:1233: workspace_test_artifact_dir is a verbatim copy of the test helper at apps/rocmd/src/lib.rs:8051. Consider one shared #[cfg(test)] pub(crate) helper so the two can't drift apart.
• apps/rocmd/src/mcp.rs:6-7: #[cfg(test)] use rocm_core::unix_time_millis; is imported at module scope but only used inside mod tests. It belongs in that module's own imports.

[rominf]

🔴 Automated review · pr-review-watcher · 506e279

Requesting changes for one defect. The full report is in the PR comment.

docs/architecture.md:34 lists persistence.rs, common.rs, webhook.rs, cli.rs and sandbox.rs as "Extracted so far", but none of them exist. At this head, apps/rocmd/src/ contains only lib.rs, main.rs and mcp.rs. The line also puts CommandCapture in common.rs, while this PR's code puts it in mcp.rs:846.

To resolve: list only mcp.rs as extracted, and note that it hosts CommandCapture/run_command_with_timeout for now. Move the other modules to "Still pending", so each sibling PR adds its own entry when it lands.

[jussielo-amd]

Thanks for the thorough review — both points are resolved now that this PR is rebased onto the stack (base is now rocmai-83-sandbox; merge order is 477 → 479 → 480 → 481 → 483 → 484 → 487 → 489).

CommandCapture placement: confirmed common.rs (#479) keeps ownership — CommandCapture and run_command_with_timeout are no longer defined in mcp.rs; this module now imports both via use crate::common::{self, CommandCapture}; and calls common::run_command_with_timeout(...). run_rocm_capture/run_rocm_capture_for_paths stay here since common.rs doesn't claim those, and tool_result_from_command/command_capture_text stay here as you suggested, since they're genuinely MCP result-shaping with no sandbox consumer. The three sandbox-cluster call sites you flagged (sandbox_check_updates_value, update_check_status, sandbox_driver_plan_value) now live in sandbox.rs (landed via #483) and already reference common::CommandCapture — no mcp:: coupling.

While rebasing I did the same cross-check for every other symbol this module reaches back for, since #483 had hit the same independent-duplication issue with update_check_message: no other duplicates turned up. build_bridge_snapshot/gather_gpu_snapshot_for_config/bridge_engine_inventory now resolve via crate::common::, load_managed_services via crate::persistence::, and stop_managed_service stays a bare crate:: reach-back since it hasn't moved out of lib.rs yet. I also found and fixed two dangling bare-crate:: references in already-landed sibling files that were pointing at symbols this PR moves: cli.rs's MCP dispatch arms (run_mcp_server, print_json, rocm_mcp_tools, ensure_direct_mcp_call_allowed, handle_mcp_tool_call) and sandbox.rs's run_rocm_capture_for_paths import, both now crate::mcp::.

architecture.md: resolved by the stacking itself — the doc hunk now incrementally appends mcp.rs to the already-landed list (persistence/common/webhook/cli/sandbox) instead of describing modules not yet present on this branch.

All 130 rocmd tests (including the 15 moved into mcp.rs's own test module) pass, along with cargo clippy --all-targets -- -D warnings and cargo fmt --check.