[jussielo-amd]

Summary

apps/rocm/src/serve_cmd.rs had grown to about 1,718 production lines (test module excluded, brace-matched), well past the 1,500-line size gate this series of module splits is working towards. Roughly 400 of those lines were one self-contained cluster: the pure transforms serve() applies, in order, to the selected engine's launch recipe hint (map the catalog record onto the protocol hint, then layer --tool-call-parser, --gpu-memory-utilization, --temperature/--top-p/--max-tokens and --engine-env on top). This PR moves that cluster out of serve_cmd.rs into a new serve_recipe.rs. serve() is the only production caller, and nothing in the moved span calls back into serve_cmd.rs, so the seam is clean. serve_cmd.rs drops to 1,318 production lines.

It is a pure move. No function body, message, flag or default changes, and the 28 tests that exercise the moved functions move with them, byte-identical and with the same leaf names.

What moved

All of these moved verbatim from serve_cmd.rs to serve_recipe.rs. The Visibility column is the only change: pub(crate) for every item serve() calls (two of them are also used by the AIM tests that stay in serve_cmd.rs), otherwise unchanged.

Symbol	Visibility in serve_recipe.rs
render_serve_engine_recipe_lines	pub(crate)
protocol_engine_recipe_hint	pub(crate) (also used by the AIM tests that stay)
engine_recipe_with_tool_call_override	pub(crate)
set_vllm_tool_call_parser	private
ServeGenerationDefaults (and its three fields)	pub(crate), because serve() builds it
engine_recipe_with_generation_defaults	pub(crate)
set_lemonade_generation_defaults, set_vllm_override_generation_config	private
engine_recipe_with_gpu_memory_utilization_override	pub(crate)
set_vllm_gpu_memory_utilization	private
parse_gpu_memory_utilization	pub(crate)
RESERVED_ENGINE_ENV_KEYS	private
engine_recipe_with_extra_env	pub(crate) (also used by the AIM tests that stay)
engine_env_ignored_note, engine_recipe_enables_tool_choice	pub(crate)

Other than the moved items, the diff is: mod serve_recipe; in main.rs; one use crate::serve_recipe::{...} in serve_cmd.rs (imported by name, so the serve() call sites are untouched); the now-unused imports removed from serve_cmd.rs (std::fmt::Write, ENGINE_RECIPE_CONTRACT_VERSION, EngineRecipeEndpointHint, EngineRecipeUnsupportedCombinationHint, and BTreeMap in its test module). I re-derived the claim "verbatim" by diffing the moved line ranges on origin/main against the new file with pub(crate) stripped: all 1,019 moved lines match in order, and the only other lines in serve_recipe.rs are the license header, module doc, imports and the mod tests scaffold.

Not moved, and why: the AIM group that sits between the two moved spans in the old file (engine_recipe_with_aim_gfx_profile, aim_gpu_preflight, aim_engine_record, ensure_aim_hf_token, aim_min_gpu_count, ensure_aim_multi_gpu_supported, aim_board_note) is a separate seam, the AIM pre-flight, so it stays in serve_cmd.rs with its tests. It is also the reason the cluster is not contiguous in the old file.

Test assignment

Tests were assigned by primary subject. The 28 that moved each test one of the moved functions on its own: protocol_engine_recipe_hint_maps_selected_engine_metadata, the four tool_call_override_*, the five gpu_memory_utilization_*, the seven generation_defaults_*, the extra_env_* and engine_env_* set (ten), and engine_recipe_enables_tool_choice_reflects_flags.

These tests call both a moved function and something that stays, and they stay in serve_cmd.rs because their subject is the AIM pre-flight, not the override they use to build their input: catalog_profile_pin_reaches_the_aim_hint_and_engine_env_overrides_it (subject: engine_recipe_with_aim_gfx_profile pinning the catalog's profile; it then uses engine_recipe_with_extra_env to show an explicit override wins), aim_serve_refuses_a_gpu_the_image_has_no_profile_for, gated_aim_model_needs_an_hf_token_before_launch, aim_board_note_only_fires_when_the_catalog_profile_is_what_runs, aim_gpu_preflight_probes_only_for_a_restricted_aim_record_off_cpu_only and aim_gpu_preflight_pins_the_profile_and_then_refuses_multi_gpu_images. They reach protocol_engine_recipe_hint and engine_recipe_with_extra_env through the use crate::serve_recipe::{...} import via use super::*; no shared fixture needed widening. The two serve_notes_* tests stay too (they test collect_serve_notes).

Size

Production lines are counted with #[cfg(test)] mod blocks excluded by brace matching.

File	Total	Test module	Production
serve_cmd.rs on origin/main	3,106	1,388	1,718
serve_cmd.rs after this PR	2,089	771	1,318
serve_recipe.rs (new)	1,051	622	429
main.rs on origin/main / after	5,074 / 5,075	2,375	2,699 / 2,700

The 1,318 is under the 1,500 gate. (The planning figure of about 1,640 for serve_cmd.rs came from an older audit; the same count on current origin/main is 1,718, because that count includes comment and blank lines.)

Crate edges

None added. serve_recipe.rs depends only on rocm_core, rocm_engine_protocol and anyhow, all already used by serve_cmd.rs; cargo xtask check-crate-edges is unchanged.

Verification

Ran, on each commit separately unless noted (commit 1 and commit 2 each compile and pass on their own):

• cargo fmt --all --check: clean.
• cargo clippy -p rocm -p xtask --all-targets -- -D warnings: clean.
• Test-name diff: cargo test -p rocm --bin rocm -- --list and -- --list --ignored, before (from origin/main) and after, compared by leaf name with the module prefix stripped: empty diff in both (1,475 tests, 8 ignored, before and after). Only the 28 moved tests change their full path (tests::x in serve_cmd to tests::x in serve_recipe); nothing is keyed to the old path (no --exact filter, include_str!, file!() or module_path!() use, and apps/rocm/proptest-regressions has no serve_cmd file).
• cargo test -p rocm --no-fail-fast: all pass (1,467 passed, 8 ignored in the bin target, plus the other targets). cargo test -p rocm --bin rocm serve --no-fail-fast: 132 passed. cargo test -p rocm advised_commands: 20 passed, 1 ignored.
• cargo test -p rocm-core --lib the_utilization_hint_states_the_bound_the_parser_accepts (its comment cites the moved parser): passes.
• cargo xtask manifest --check, cargo xtask check-crate-edges, cargo test -p xtask ascii: all pass.
• Path pins: grepped every moved symbol and serve_cmd across docs/, AGENTS.md, CONTRIBUTING.md, xtask/src, tests/, .github/ and .pre-commit-config.yaml. The xtask/src/ascii_output_contract.rs entries on serve_cmd.rs ("Starting {model} on {selected_engine}…" and "Running smoke test…") are correct as they are because both literals live in serve(). advised_commands.rs scans source by content, with no serve_cmd.rs entry. The only stale path was a comment in crates/rocm-core/src/fix.rs, fixed in commit 2.
• Commit hooks (license header, lychee link check, fmt) pass on both commits.

Not run:

• Windows cross-clippy (--target x86_64-pc-windows-msvc): cannot run on this host. The build of aws-lc-sys fails compiling C for the MSVC target (unknown type name 'pthread_rwlock_t') before reaching any workspace code. Nothing moved is cfg(windows)-gated (the only cfg in the new file is #[cfg(test)], and the cfg!(windows) use in select_serve_engine stays in serve_cmd.rs), so the Windows CI lane is the check.
• cargo xtask coverage: not run. Tests moved within the same crate and none were deleted or weakened.
• Mutation checks: not applicable, no test was added or edited.
• cargo xtask e2e and scripts/smoke_local.py: not run; CI carries them.

User-visible behaviour

None. This is a refactor with no change to command output, exit codes, files or engine selection, so no Gherkin scenario is added or needed. The behaviour the moved code produces is already covered by the existing serve scenarios and by the moved unit tests, which still pass unchanged.

Notes for reviewers

Read commit by commit, then the merge. Commit 1 is the move (new serve_recipe.rs, serve_cmd.rs shrink, one mod line in main.rs); git diff --color-moved shows the production block as moved text. Commit 2 is docs and comments only: the serve_cmd.rs entry in docs/architecture.md is rewritten, a serve_recipe.rs entry is added, and the one clause there that called ServeGenerationDefaults private is updated; serve_cmd.rs's module doc no longer lists the overrides as its own; the parse_gpu_memory_utilization doc comment that said "elsewhere in this file" now names serve_cmd.rs; and a test comment in crates/rocm-core/src/fix.rs that cites the parser by file now names serve_recipe.rs.

A third commit merges origin/main into the branch (a merge rather than a rebase, so the existing commits keep their signatures). The only textual conflict was docs/architecture.md, where the already-merged engine-helper move (#872) and this PR both edited the runtimes_cmd.rs and serve_cmd.rs neighbourhood: the runtimes_cmd.rs entry is main's version, and the serve_cmd.rs and serve_recipe.rs entries are this PR's. #872 also edited the imports of serve_cmd.rs (engine_manages_own_runtime now comes from engine_install_env.rs); git merged that without a conflict, and both of its lines are still present. The leaf-name test list of the rocm crate is the same on the merge result as on origin/main.

serve_cmd.rs is a hot file. I checked each open PR that touches it by merging its head with this branch (git merge-tree, read-only) and comparing against the same merge with origin/main; that survey predates the merge of origin/main into this branch and has not been repeated. No open PR edited a moved line: none of their serve_cmd.rs hunks mention a moved symbol or test. Conflicts that already existed against origin/main (in serve() and the managed-service code) are unchanged by this PR: #351, #437, #854, #883, #723, #724 and #725. New conflicts introduced by this PR are one-line and mechanical: mod serve_recipe; next to mod serve_cmd; in main.rs for #817 and #243 (keep both lines). #857 and #678 merged cleanly in serve_cmd.rs. #724 and #725 also rewrap the rocm_engine_protocol import in serve_cmd.rs; whichever PR lands second needs to drop the three names this PR makes unused (ENGINE_RECIPE_CONTRACT_VERSION, EngineRecipeEndpointHint, EngineRecipeUnsupportedCombinationHint) when resolving that already-conflicting hunk, or clippy flags unused imports.

[Copilot]

🟢 Approval recommended

No active review issues are recorded for the recipe-helper module extraction.

No open issues 🎉

What changed in this PR

Extracts the engine-recipe helpers from serve_cmd.rs into serve_recipe.rs, continuing the CLI’s module-splitting work without changing serving behavior.

Changes:

• Moves recipe mapping, flag overrides, generation defaults, and 28 existing tests into the new module.
• Preserves serve() call sites through named imports, while keeping engine selection and AIM pre-flight logic in serve_cmd.rs.
• Updates architecture documentation and source comments to reflect the new ownership.

File	Description
docs/​architecture.md	Documents the new module and revised responsibilities and visibility.
crates/​rocm-core/​src/​fix.rs	Updates a test comment to cite the parser’s new location.
apps/​rocm/​src/​serve_recipe.rs	Houses the relocated recipe helpers, defaults type, and tests.
apps/​rocm/​src/​serve_cmd.rs	Imports the extracted helpers and removes their former definitions and tests.
apps/​rocm/​src/​main.rs	Declares the private serve_recipe module.

_{Review effort: Balanced}

[juhovainio]

I read through the diff and this looks good. It is a pure move: the code removed from serve_cmd.rs matches what is added in serve_recipe.rs, apart from the license header, module docs, imports, the test scaffold and two doc-comment rewordings. No function body, message, flag or default changes. The architecture.md module map and its links are updated correctly, and CI is green.

Two small non-blocking notes:

• The "Relocated... no behavior change" history note in the serve_recipe.rs module doc will age; consider dropping it later.
• architecture.md drops "(Phase 6a)" from the serve_cmd.rs entry, which loses a bit of history.

This is based on a read of the diff plus the CI results.

[jussielo-amd]

Both notes are addressed in e8740e7, with main merged in afterwards (4c4e749, f8bb465: plain merge commits, no rebase) because it moved under this PR. The "Relocated from serve_cmd.rs with no behavior change." sentence is gone from the serve_recipe.rs module doc, so the doc only says what the module is. The "(Phase 6a)" label is back on the serve_cmd.rs entry in docs/architecture.md, exactly as on main; no other Phase label was added. The only conflict was fix.rs, which main split into a fix/ directory: I took main's version and re-applied this PR's one-line comment edit in the moved test in fix/catalog.rs. cargo test -p rocm --bin rocm passes and the leaf test-name list matches main.