[jussielo-amd]

Summary

apps/rocmd/src/mcp.rs held the MCP stdio server, the tool table and dispatch, and the argument handling behind the tools that spawn rocm, all in one file of about 1,650 lines. This moves the argument handling into its own module, apps/rocmd/src/mcp_argv.rs, together with the tests that exercise it. It is a pure, behaviour-preserving move and part of the Phase 8.2-D follow-up work to finish splitting the oversized rocmd modules. After the move mcp.rs has about 860 production lines (was about 1,140) and mcp_argv.rs about 305.

What moved

Symbol	From	To	Visibility
normalized_rocm_command_args	mcp.rs	mcp_argv.rs	private to pub(crate)
ensure_rocm_command_is_read_only (the read-only allowlist behind the rocm_command tool)	mcp.rs	mcp_argv.rs	private to pub(crate)
build_install_sdk_args	mcp.rs	mcp_argv.rs	private to pub(crate)
build_install_engine_args	mcp.rs	mcp_argv.rs	private to pub(crate)
build_launch_server_args	mcp.rs	mcp_argv.rs	private to pub(crate)
build_watcher_enable_args	mcp.rs	mcp_argv.rs	private to pub(crate)
is_loopback_host, system_prefix_requires_ack (helpers of the builders)	mcp.rs	mcp_argv.rs	stay private

Twelve tests moved with them into mcp_argv.rs's own mod tests, keeping their names and the cfg(unix) gate on install_sdk_rejects_system_prefix_reached_by_escaping_home: remote_read_only_verbs_are_allowed_and_mutating_ones_are_not, rocm_command_helper_allows_only_read_only_rocm_commands, rocm_command_helper_treats_setup_status_as_read_only_and_reset_as_mutating, rocm_command_helper_treats_runtimes_uninstall_dry_run_as_read_only, storage_report_is_read_only_but_removal_is_not, launch_server_rejects_public_bind_without_ack, launch_server_forwards_public_bind_ack, install_sdk_rejects_system_prefix_without_ack, install_sdk_rejects_system_prefix_reached_by_escaping_home, install_sdk_real_install_args_approve_only_the_runtime_replacement, install_sdk_forwards_requested_build_date_and_rejects_conflict and watcher_enable_builds_mode_args. The count of 12 matches the plan, and none of them uses test_support.

What stays in mcp.rs: the server loop, rocm_mcp_tools and rocm_mcp_tool, the direct-approval classification (mcp_tool_requires_direct_approval, ensure_direct_mcp_call_allowed) and its two tests (they read the tool table, so moving them would make mcp_argv.rs import mcp.rs back), the result shapers, read_tail_lines, and the six remaining tests.

The only non-move edits are: the six visibility widenings above; the use lines (mcp.rs gains use crate::mcp_argv::{...} and drops the now-unused DEFAULT_LOCAL_HOST import); mod mcp_argv; in lib.rs; the docs/architecture.md entry for mcp_argv.rs and the updated mcp.rs entry; and one comment in apps/rocm/src/remote/mod.rs that named apps/rocmd/src/lib.rs as the home of ensure_rocm_command_is_read_only (it never lived there) and now names mcp_argv.rs. rustfmt re-wrapped the signatures of four functions after the pub(crate) prefix lengthened them.

Verification

Ran locally:

• cargo fmt --all --check passes.
• cargo clippy -p rocmd --all-targets -- -D warnings passes.
• cargo test -p rocmd --no-fail-fast: 168 passed, 0 failed, 1 ignored.
• Test-list diff by leaf name, cargo test -p rocmd -- --list and the same with --list --ignored, origin/main against this branch (the baseline built in a separate clean worktree): both empty, 169 and 1 entries on each side. The new list has 12 tests under mcp_argv::tests and 6 under mcp::tests, where origin/main had 18 under mcp::tests.
• A line-level comparison of the removed lines of mcp.rs against mcp_argv.rs: every removed line is present in the new file apart from the six signature lines that gained pub(crate) or were re-wrapped, and the removed use rocm_core::{...} line.
• cargo xtask manifest --check and cargo xtask check-crate-edges pass; cargo xtask lychee passes (the commit hooks, including lychee and the license-header check, also passed).
• Leak scan on git diff origin/main...HEAD with the AGENTS.md pattern: no hits. No Phase wording in the added lines of docs/architecture.md.

Not run:

• cargo clippy -p rocmd --all-targets --target x86_64-pc-windows-msvc: the moved code has no cfg(windows) part (only the one cfg(unix) test, which moved with its gate), but the cross-check could not be completed here because a dependency's build script needs lib.exe (cc-rs fails with "failed to find tool lib.exe"). The Windows CI lane covers it.
• cargo clippy -p rocm: the only change in apps/rocm is a comment.
• The full workspace gate, cargo xtask e2e and scripts/smoke_local.py: left to CI.

User-visible behaviour

None. This is a pure internal move with no change to any command output, exit code, file the CLI creates or engine selection, so no Gherkin scenario is added; the allowlist and argv-builder behaviour stays covered by the same unit tests, now in mcp_argv.rs.

Notes for reviewers

One commit. A good reading order is docs/architecture.md, mcp_argv.rs, then the removal in mcp.rs. A git diff --color-moved of the commit shows the bodies as moved blocks.

Module edges: mcp_argv.rs has no use crate:: imports at all (it uses only anyhow, rocm_core::DEFAULT_LOCAL_HOST, serde_json::Value, and calls rocm_core and rocm_engine_protocol by path), so there is no cycle. mcp.rs gains the single edge crate::mcp_argv; its existing edges (common, host_snapshot, persistence, and service by path) are unchanged. rocmd still has no dependency on rocm.

Open pull requests that touch the same files, read-only check:

• fix(serve): make model serving ports collision-safe #243 edits build_launch_server_args and adds tests in apps/rocmd/src/mcp.rs, so it will conflict with this move and needs its hunks re-applied in mcp_argv.rs (the test hunk goes in mcp_argv.rs's mod tests if it targets the argv builder).
• feat(serve): expose an Anthropic-compatible /v1/messages endpoint (ROCMAI-597) #883, feat(serve): serve speech-to-text models through Lemonade (ROCMAI-672) #724 and feat: serve API docs for every running model server (ROCMAI-671) #723 each add a mod line near the top of apps/rocmd/src/lib.rs; the conflict is mechanical.
• docs/architecture.md is touched by a large number of open pull requests; the rocmd section conflicts only with the ones that also edit the mcp.rs or neighbouring entries (for example ROCMAI-859: split rocmd watchers.rs into a watchers/ directory #885, which splits watchers.rs).
• fix: make every command preview denote the argv it runs (POSIX quoting, follow-up to #500) (ROCMAI-530) #563 touches apps/rocm/src/remote/mod.rs but not the comment changed here.

[Copilot]

🟢 Approval recommended

There are no active review issues blocking the argument-handling refactor.

No open issues 🎉

What changed in this PR

This refactor separates MCP argument handling from rocmd’s server and dispatch code, reducing mcp.rs without changing user-visible behavior.

Changes:

• Moves argument normalization, the read-only allowlist, four argument builders, and their 12 tests into mcp_argv.rs.
• Registers the new module and imports its six crate-visible functions, leaving dispatch and approval checks in mcp.rs.
• Updates architecture documentation and corrects a source-path comment.

File	Description
docs/​architecture.md	Documents the new module and revised MCP responsibilities.
apps/​rocmd/​src/​mcp.rs	Imports extracted helpers and removes relocated code and tests.
apps/​rocmd/​src/​mcp_argv.rs	Contains argument handling, validation helpers, and 12 relocated tests.
apps/​rocmd/​src/​lib.rs	Registers the private mcp_argv module.
apps/​rocm/​src/​remote/​mod.rs	Corrects the allowlist’s location in a comment; no functional change.

_{Review effort: Balanced}

[juhovainio]

Reviewed the move of the MCP argv builders and read-only allowlist from mcp.rs into mcp_argv.rs. The moved bodies are identical to the originals; the only differences are the six fns widened from private to pub(crate) (stated in the PR body), the new module header/use lines, the tests wrapper, and a trimmed rocm_core import list in mcp.rs. All 12 tests moved with their cfg(unix) gate, mod mcp_argv; is declared in lib.rs, the architecture.md links resolve, and the stale comment in remote/mod.rs is corrected. No blocking or non-blocking findings.

CI note: the Strix Halo E2E lane failed with 'llama-server failed to start', which looks like infra rather than this change; the MI300X and W7900 lanes were still pending when I checked.