[jussielo-amd]

Summary

This is a proposal, meant to be reviewed as one. It implements one option of an open question for the maintainers: whether apps/rocm may keep its whole clap command tree in a cli.rs, as apps/rocmd already does, instead of in main.rs. If the answer is no, this PR can simply be closed; nothing else depends on it.

The symptom that prompts the question is size. apps/rocm/src/main.rs is about 5,075 lines, roughly 2,600 of them production code (inline test modules excluded by brace matching), and about 1,340 of those are one thing: the Cli and Command definitions, the ten subcommand enums nested under them and the clap value enums behind --provider, --device, --framework and similar arguments. Every open PR that adds a flag edits that region, so it is also where reviews collide. With the tree in its own file, main.rs is down to about 1,260 production lines and holds what is really glue: the process entry, dispatch, the completion builder, the exit-code types and the shared helpers. The other half of the size gap, the comfyui dispatch function, is a separate follow-up; this PR deliberately does not touch it.

The change is a pure move. No clap attribute, help text, subcommand order, default or alias changes. The convention texts that said the clap enum usually stays in main.rs (the module organization section of docs/architecture.md and the architecture guardrails in AGENTS.md) are updated to match, so the proposal and the documented convention agree.

What moved

Symbol	From	To
Cli, Command, BenchCommand, InstallTarget, EnginesCommand, RuntimesCommand, StorageCommand, ComfyuiCommand, ServicesCommand, AutomationsCommand, ConfigCommand, SetupCommand	main.rs	cli.rs
FrameworkArg, InstallFormat, Provider, WatcherModeArg, TelemetryModeArg, DevicePolicyArg, SandboxToolArg, PermissionsModeArg, with their as_str-style methods and From<FrameworkArg>	main.rs	cli.rs
SUPPORTED_ENGINES (the value_parser choices of the --engine arguments; its only other user is a test in advised_commands.rs, which reaches it through a test-only re-import in main.rs)	main.rs	cli.rs
impl From<WatcherModeArg> for WatcherMode (sits next to the enum it converts)	main.rs	cli.rs
35 clap parsing and help tests, plus the helpers parse_serve, parse_runtimes, runtimes_restart_flags and serve_possible_values	main.rs test module	cli.rs test module

Moved items become pub(crate) (including the Cli.command field and the small conversion methods), and main.rs re-imports the types so every other module keeps reaching them as crate::Command, crate::Provider and so on. Stays in main.rs: dispatch, the process entry, cli_command and the completion builders, FixExitCode and ClapExitCode, FreeformInvocation, ChatInferenceParams, GatedEngineAction, provider_name, device_policy_name and the other shared helpers; each was checked against its callers and none of them is part of the tree.

The test assignment goes by primary subject. Moved to cli.rs: cli_command_definition_is_valid, top_level_help_lists_commands_alphabetically, every_framework_choice_reaches_the_library, the_framework_choice_is_offered_on_the_command_line, install_sdk_help_describes_the_gate_as_replacing_the_active_default, install_sdk_help_separates_the_two_consents_yes_carries, yes_help_says_what_happens_without_it_outside_a_terminal, out_of_scope_commands_are_marked_preview_in_help, runtimes_help_uses_the_runtime_noun_throughout, the_preview_marker_is_explained_and_not_contradicted, every_help_text_in_the_command_tree_is_ascii, bench_load_rejects_zero_and_unbounded_numeric_arguments, t5_bench_load_clap_parse_smoke, serve_engine_help_lists_match_engine_inventory, serve_device_help_lists_match_device_policy_names, serve_rejects_unknown_device_policy_as_usage_error, serve_accepts_device_policy_values_and_aliases, serve_parses_verbose_and_no_smoke_test_flags, serve_verbose_conflicts_with_managed, serve_defaults_have_all_flags_off, serve_parses_generation_flags, serve_rejects_out_of_range_sampling, serve_negative_sampling_space_form_reaches_range_validator, runtimes_activate_and_rollback_refuse_yes_without_restart_services, runtimes_restart_flags_parse_together_and_alone, runtimes_activate_parses_the_runtime_it_was_given, chat_negative_sampling_space_form_reaches_range_validator, chat_parses_generation_flags, chat_rejects_zero_max_tokens, update_rejects_zero_timeout_secs, services_prune_rejects_any_age_with_an_explicit_age, update_dry_run_does_not_require_apply, update_dry_run_conflicts_with_json, install_sdk_accepts_family_override and install_sdk_devel_flag_defaults_off_and_wires_through_when_passed.

Stayed in main.rs even though some of them parse through the tree as a side step, because their subject is something else: the exit-code and dispatch tests (exit_code_for_*, clap_error_exit_code_survives_the_clap_exit_code_round_trip, cli_command_rejects_unknown_flag_through_clap_exit_code, dispatch_carries_fixs_exit_code_through_to_exit_code_for), the completion builder tests (completions_generate_for_every_shell, completion_command_excludes_hidden_subcommands), the natural-language routing tests (top_level_cli_commands_are_not_treated_as_freeform, services_is_structured_not_freeform, remote_is_structured_not_freeform, completions_command_is_structured_not_freeform, completions_rejects_unknown_shell, the freeform_invocation_* and mistyped-command tests), config_set_permissions_sets_mode (a config round trip) and update_apply_approval_never_credits_the_inert_yes_flag (about the approval line). The count is 35 rather than the 36 first estimated: the one that would make 36 is the freeform-routing group, where parsing is incidental.

Verification

Ran locally on this branch before it was brought up to date with main; the checks marked (re-run) were repeated on the merge commit that brings main in (the engine runtime helper move and the comfyui split):

• rocm --help and the --help and -h output of every visible subcommand (76 of them, found by walking the help text recursively), --help of the hidden subcommands, --version, rocm help, eight clap parse-error renderings (invalid values for the value enums, among others) and rocm completions for bash, zsh, fish, powershell and elvish were captured from a binary built on the base commit and from one built on this branch, and diffed byte for byte: the diff is empty. The one expected difference is --version, which embeds the git commit hash; it is identical once the hash is masked.
• cargo test -p rocm --bin rocm -- --list and --list --ignored, compared by leaf test name before and after: identical (1,475 and 8, re-run against the current main). All 1,467 non-ignored tests pass (re-run); the 8 ignored ones are the same 8 as before. Thirty-five of the passing tests now report as cli::tests::....
• cargo fmt --all --check (re-run); cargo clippy -p rocm --all-targets -- -D warnings (re-run; earlier also with -p xtask); cargo test -p rocm --bin rocm advised_commands (it walks the clap tree); cargo test -p xtask (including the ASCII output contract, whose doc comment named the old home of the help walk); cargo xtask check-crate-edges, cargo xtask manifest --check and cargo xtask lychee (all re-run).
• Size, measured by brace-matching out the inline test modules: main.rs 2,615 to 1,262 production lines on the current base, cli.rs 1,384. Both are under 1,500.
• Not run: the Windows cross-check (cargo clippy --target x86_64-pc-windows-msvc), which cannot build in this environment because the TLS dependency's C build needs a Windows toolchain. The moved code carries no cfg(windows) or other platform gates (the only cfg in cli.rs is cfg(test)), so the Windows CI lane is the check that matters for it. Also not run locally: the full workspace gate, the e2e suite and the smoke script; CI carries them.
• No proptest lives in main.rs's test module, so none moved and no regression-seed file needed to move with it.

User-visible behaviour

None, so there is no scenario to add or update. This is a pure internal move, and the byte-identical help and completion output above is the behaviour proof. The advised-command contract test runs unmodified and no e2e scenario or step definition is touched.

Notes for reviewers

Read it commit by commit (four commits, then a merge of main with two conflicted files: the engines_cmd.rs module doc and four entries in docs/architecture.md, each taking main's text with the clap-tree-location edit re-applied on top; main's own main.rs changes touch no moved item). Two items beyond the usual tree and value enums moved with them: SUPPORTED_ENGINES and the From<WatcherModeArg> for WatcherMode conversion; the first commit's message says three items but names these two, so read it as two. The first commit creates cli.rs and moves the tree and the value enums; its diff against main.rs is the deletion of one contiguous block plus the new use lines, and the module docs that said the clap types stay in main.rs are corrected there. The second moves the 35 tests and their helpers; the moved bodies are line-for-line identical (checked by comparing the removed and added lines), only the module path changes from tests:: to cli::tests::, and a few comments in other modules that named main.rs as the test home now name cli.rs. The third updates docs/architecture.md (the convention, the main.rs entry, a new cli.rs entry and every per-module entry that said its command enum stays in main.rs) and the matching sentence in AGENTS.md, which says the enum usually lives in cli.rs and names bootstrap.rs and the remote module as the subsystems that own their own enum. The fourth sweeps the remaining claims about where the tree lives: the services-prune entry in architecture.md (the constant is now imported only by cli.rs), the pull request template's note on the clap declaration, a module doc in driver_install.rs and the engine IPC entry.

One detail worth knowing when reading the new module doc: declaration order sets the order of every nested --help command list, but the top-level list is sorted alphabetically by cli_command(), which stays in main.rs. Nothing about either changes here.

The move conflicts with open PRs that edit the tree. A trial merge of each open PR against this branch (compared with a trial merge against the base, so existing conflicts do not count) shows new conflicts in main.rs for the PRs that edit flags or help text, namely 299, 816, 817, 818, 819, 820, 826 and 857, plus a draft, 568. In each of these the conflicting hunks are doc comments, attributes or fields inside the moved region, or tests that moved, so re-applying them to cli.rs is mechanical. Two others (880 and 888) conflict in main.rs outside the tree: they touch a moved test and the import block. 853 only touches the docs and module comments edited here. (872, which was in this group, has since merged, and this branch already contains it.) Several older PRs (243, 631, 678, 723, 724, 725 and others) already conflict with the base in main.rs, so they need a rebase regardless; the ones that also touch the tree (a new task field on Command::Serve, for example) would re-apply the same way. The change is deliberately landed in a few small commits so it can be reviewed as a proposal and dropped without affecting anything else.

[Copilot]

🔵 Needs a closer look

Maintainers still need to decide whether cli.rs should own the apps/rocm command tree before the proposed architectural convention can be accepted.

No open issues 🎉

What changed in this PR

This proposal separates rocm’s clap command definitions from process and dispatch logic, following the existing rocmd layout. It aims to reduce main.rs size and editing conflicts without changing CLI behavior.

Changes:

• Relocates the command tree, value enums, conversions, and 35 parsing/help tests to cli.rs, retaining crate-root imports for existing callers.
• Keeps entry points, dispatch, completion builders, and shared helpers in main.rs.
• Updates architecture guidance and source comments to reflect the new locations.

File	Description
xtask/​src/​ascii_output_contract.rs	Updates the ASCII-help test location in documentation.
docs/​architecture.md	Documents the command-tree module and revised ownership.
apps/​rocm/​src/​setup_cmd.rs	Updates the SetupCommand location comment.
apps/​rocm/​src/​services_cmd.rs	Updates the ServicesCommand location comment.
apps/​rocm/​src/​service_records.rs	Corrects comments about the prune constant’s importer.
apps/​rocm/​src/​service_launch.rs	Updates command-tree ownership comments.
apps/​rocm/​src/​runtimes_cmd.rs	Documents the enum location and retained crate-root import.
apps/​rocm/​src/​runtime_services.rs	Updates the RuntimesCommand location comment.
apps/​rocm/​src/​main.rs	Removes relocated definitions and tests; adds module imports.
apps/​rocm/​src/​install_cmd.rs	Updates comments naming clap types and the moved test.
apps/​rocm/​src/​examine_report_text.rs	Documents the relocated ASCII-help test.
apps/​rocm/​src/​engines_cmd.rs	Updates enum ownership and import comments.
apps/​rocm/​src/​engine_ipc.rs	Documents the watcher conversion’s new location.
apps/​rocm/​src/​driver_install.rs	Updates comments locating CLI types.
apps/​rocm/​src/​config_cmd.rs	Updates the ConfigCommand location comment.
apps/​rocm/​src/​chat.rs	Updates a comment referencing the moved help test.
apps/​rocm/​src/​chat_cmd.rs	Clarifies where Provider and inference parameters live.
apps/​rocm/​src/​bridge_snapshot.rs	Updates the engine-inventory test location comment.
AGENTS.md	Revises command-tree placement guidance.
.github/​pull_request_template.md	Aligns the architecture checklist with the proposed layout.

_{Review effort: Balanced}

[juhovainio]

This is a clean move. I compared the lines added to cli.rs against the lines removed from main.rs, and the only differences are pub(crate) visibility, re-split use lines, one doc link now pointing at crate::gpu_policy::..., and the new module header and test scaffolding. No attributes, doc comments, help text, defaults, aliases or variant order were dropped, and the moved tests are identical. The docs (AGENTS.md, architecture.md, the PR template and module docs) are updated consistently, and CI is green.

This reflects a diff read plus CI, not a runtime check. The PR currently conflicts with main, so it needs a rebase or merge first, and it is worth confirming that any edits main made to the clap tree region were re-applied in cli.rs before merging.

[jussielo-amd]

Merged main into this branch (e0d1f05, f161096, 4b7a2f3: plain merge commits, no rebase or force-push), and checked what you asked: that any edit main made to the clap tree was re-applied in cli.rs. Since this PR's previous merge base, one main commit touched main.rs at all, #563, and its only hunk there is the mod shell_quote; line, which merged into the new main.rs; no commit touched the clap tree itself, so there was nothing to re-apply in cli.rs. As an independent check I compared every line of main's main.rs against the merged main.rs plus cli.rs: 2226 lines left main.rs, and the only ones not present verbatim in cli.rs are visibility prefixes, imports, the parse_device_policy doc link (gpu_policy:: is now crate::gpu_policy::) and a re-import comment. The only conflict was docs/architecture.md (the service_hints, service_launch, service_records, services_cmd, setup_cmd and shell_quote entries): I kept this PR's wording that the command enums live in cli.rs, and took main's new shell_quote entry and its new sentence on service_hints.rs. I also built main and this branch and diffed --help for 25 invocations (top level, serve, install, install sdk, engines, runtimes, services, storage, comfyui, config, setup, bench, chat, examine, fix, diagnose, uninstall, update, version and others): identical text and exit codes. cargo test -p rocm --bin rocm passes with the same counts on both builds.

[jussielo-amd]

Merged main into the branch again (403bb36, 7771079, e998882: plain merge commits, no rebase) because it moved under this PR again (#888, #891, #903, #908, #911, #890 and others). Since the previous merge base only two main commits touched main.rs at all, #891 (the mod serve_recipe; line) and #888 (three mod lines and a comment edit in a help test), and neither edited the clap tree itself, so nothing needed re-applying in cli.rs. #888 had moved the three clap help tests and runtimes_activate_approval_modal_uses_the_runtime_noun into tool_policy.rs; I kept them only in cli.rs where this PR put them, with the comment links updated to match. The other conflicts were adjacent-line edits in docs/architecture.md, merged line by line. A line comparison of main's main.rs against the merged main.rs plus cli.rs again leaves only visibility prefixes, imports, the gpu_policy:: link path and a re-import comment. cargo test -p rocm --bin rocm gives identical leaf test names (1518) on main and this branch, and --help text and exit codes are identical for 25 invocations between main's binary and this branch's; clippy, fmt, manifest --check, check-crate-edges and lychee are clean.