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

[jussielo-amd]

Summary

Fourth PR of Phase 5 (ROCMAI-83, part of the modularization epic ROCMAI-27). Pulls the Cli/Command clap definitions, SandboxToolArg/SandboxToolPolicy, and the top-level dispatch (run_cli/run_bin_cli/run_from_args) into their own cli.rs module.

This follows the mechanical-relocation pattern (per docs/architecture.md's convention) rather than full domain extraction: run_cli is thin dispatch, and its match arms call into clusters that haven't been extracted yet (sandbox, mcp, service, watchers) via crate::-qualified paths, to be updated as each of those lands in a later PR.

• lib.rs re-exports run_bin_cli/run_from_args via pub use cli::{...}; — the crate's only two externally-consumed symbols are untouched.
• No behavior change. SandboxToolArg/SandboxToolPolicy are consumed pervasively by the still-inline sandbox.rs cluster and its test suite — all ~89 call sites repointed to cli::SandboxToolArg/cli::SandboxToolPolicy.
• The 4 tests that exercise Cli/Command parsing directly (not mixed with sandbox/mcp/webhook/service domain logic that happens to share a name prefix, or merely use these types as a parameter) moved into cli.rs's own #[cfg(test)] mod in this same PR.
• docs/architecture.md updated in this PR.

Independent of #477, #479, #480 — 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)

[Copilot]

Copilot review overview

🟡 Changes recommended

The architecture map incorrectly claims three independently developed modules are already present.

Review effort: Balanced
Findings: 1

Open (1)

• Correct architecture map to match extracted files in this branch · New

What changed in this PR

Extracts rocmd CLI definitions and dispatch into a dedicated module without intended behavior changes.

Changes:

• Adds cli.rs with CLI types, dispatch, and parsing tests.
• Re-exports existing public entry points from lib.rs.
• Updates architecture documentation.

File	Description
apps/​rocmd/​src/​cli.rs	Houses CLI definitions, dispatch, policies, and tests.
apps/​rocmd/​src/​lib.rs	Registers the module and updates internal references.
docs/​architecture.md	Documents modularization progress.

---

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

[jussielo-amd]

Resolved by stacking rather than editing the doc hunk: this PR is now rebased onto rocmai-83-webhook and its base is set to that branch (merge order 477 → 479 → 480 → 481), so persistence.rs, common.rs, and webhook.rs are genuinely present in this branch's tree — the architecture.md wording now matches the checked-in files. See r0x0r's review for the fuller writeup of why stacking (rather than scoping the doc hunk to cli.rs only) was the chosen fix for the whole Phase-5 batch.

[r0x0r]

Review: code is clean; holding approval on one doc line

The extraction verified as pure code motion, including every clap attribute — which is where this particular diff could have gone wrong silently. One issue in docs/architecture.md is the only thing between this and an approval, plus one terminology point worth settling before the remaining cluster PRs cite it.

The doc hunk describes a state this branch is not in

The new text claims persistence.rs, common.rs and webhook.rs are extracted alongside cli.rs. None of those three exists in this branch — apps/rocmd/src/ here contains only cli.rs, lib.rs, main.rs.

The reason this is more than a doc nit is only visible across the phase, not from inside this PR:

• All five PRs rewrite the same single line of docs/architecture.md, each branched independently from main. Four are guaranteed to conflict on it.
• Each text is a cumulative prefix assuming the earlier sequence has merged — true only at merge order 477 → 479 → 480 → 481 → 483.
• The failure is quiet rather than loud: this PR merging first finds nothing to conflict with, applies cleanly, and leaves main asserting three files that do not exist. The conflict that would have caught it only exists once something else has touched that line.

Given docs/architecture.md's own framing — updated in the same PR as the code it documents, and "a stale-but-plausible-looking note is worse than an explicit prompt to check" — either fix works:

1. Scope the hunk to cli.rs only, letting each sibling append as it lands. Correct at any merge order; conflicts become trivial appends.
2. Keep the cumulative text and gate merge order with Depends on #477, #479, #480 in the body.

Non-blocking: this is the bootstrap.rs pattern, not mechanical relocation

The PR body describes the change as following the mechanical-relocation pattern. docs/architecture.md defines that pattern by the module having no owned types — shared types stay at the crate root and are reached via crate::. This PR moves four type definitions into cli.rs: Cli, Command, SandboxToolArg and SandboxToolPolicy.

The doc also says that for a subsystem with a dedicated clap subcommand the command enum and dispatch usually stay put, and names bootstrap.rs as the further-extracted variant that owns its clap enum and its dispatch function. That is precisely what cli.rs is here.

Nothing about the code is wrong — this is a labelling question, and I'd not hold the PR for it. But it is worth correcting rather than leaving, because sandbox.rs, mcp.rs, service.rs and watchers.rs are still to come and will reach for a precedent in exactly this doc. If "mechanical relocation" is recorded as covering a module that owns four types, the distinction the convention is drawing stops doing any work. Citing the bootstrap.rs precedent instead costs a sentence and keeps the taxonomy meaningful.

Verified clean

• Clap attributes preserved byte-for-byte — I went through Cli and Command attribute by attribute against lib.rs:64–215 on main: every #[arg(...)] / #[command(...)], every long/short/default_value/value_enum/requires/conflicts_with, every help string, the #[command(hide = true)] markers, and the #[cfg(target_os = "linux")] guards on writes_data/writes_cache. This is the one place in this diff where a silent regression was possible — a dropped attribute changes the CLI's user-facing surface without failing to compile — so it is worth saying explicitly that there is none. Constants in as_cli_value match, and SandboxToolPolicy::from_cli is unchanged.
• Public API surface is exactly preserved — main exported precisely run_bin_cli and run_from_args; this branch exports precisely those two via pub use cli::{...} from a private mod cli, so rocmd::cli::* stays unreachable from outside. Both external consumers (apps/rocmd/src/main.rs:6, apps/rocm/src/main.rs:2442) are unaffected.
• All ~89 SandboxToolArg/SandboxToolPolicy sites repointed; the lib.rs tests reach them through use super::* and can still build struct literals because the fields are pub(crate).
• Imports — clap::{Parser, Subcommand, ValueEnum} correctly dropped from lib.rs and clap::CommandFactory from its test module, with no clap symbol left referenced there; nothing fails under -D warnings. Every crate::-qualified call out of cli.rs resolves to a function still present in lib.rs.
• Tests — 130 attributes conserved (126 + 4), matching the claimed 4 exactly.
• Crate-layering (AGENTS.md §6) — cli.rs imports from rocm_core, not rocm; the rocmd-must-not-depend-on-rocm invariant holds.
• Blast radius — the comment at apps/rocm/src/main.rs:22227 referring to rocmd::Cli/rocmd::Command as "crate-private in rocmd" remains accurate after the move, since it describes visibility rather than location. No change needed there.
• Leak scan (AGENTS.md §2) — the one hit is the pre-existing phrase "restricted internal tool API" in a help string, moved verbatim. Clean.

---

🤖 by agent-hub on AMD AgentHub

[jussielo-amd]

@r0x0r thanks for the thorough verification pass — replying to both points raised in your review:

Blocking: architecture.md describing sibling-PR state. Resolved by stacking, not by editing the doc hunk. The whole Phase-5 batch is now stacked per AGENTS.md §11, merge order 477 (persistence.rs) → 479 (common.rs) → 480 (webhook.rs) → 481 (this PR, cli.rs) → 483 → 484 → 487 → 489. This PR's base is now rocmai-83-webhook, so persistence.rs, common.rs, and webhook.rs are genuinely present in the branch, and the architecture.md text matches the checked-in tree rather than describing a future merge state. That also closes the quiet-failure scenario you flagged — this PR can no longer land first out of order, since it now depends on the predecessor branches directly instead of only on a shared doc line.

Non-blocking: mechanical-relocation vs. bootstrap.rs-style taxonomy. Fair point, and I agree with the read — cli.rs owns four types (Cli, Command, SandboxToolArg, SandboxToolPolicy) plus its own dispatch, which is closer to the bootstrap.rs pattern than pure mechanical relocation. Leaving the PR description/scope as-is for now rather than reworking the framing here, but noting it so the sandbox.rs/mcp.rs/service.rs/watchers.rs PRs that follow don't cite this one as precedent for the wrong pattern.