ROCMAI-82: extract driver-install, engines, serve from main.rs

[jussielo-amd]

Summary

• Phase 6a of the ongoing main.rs modularization effort (tracked in docs/architecture.md): mechanically relocates install_driver/reconcile_driver_install, engines(), and serve() out of main.rs into their own files (driver_install.rs, engines_cmd.rs, serve_cmd.rs), mirroring the existing automations.rs/uninstall.rs convention.
• Unlike those two, these three clusters own private types used nowhere else (driver-plan/state types, ServeArgs, engine-recipe structs). Those types moved with their functions rather than staying in main.rs, since leaving ~15 cluster-private types behind would defeat the purpose of shrinking the file. docs/architecture.md is updated to document this as a variant of the mechanical-relocation pattern.
• No behavior change. dispatch()'s call sites are byte-identical (diffed directly against the pre-change function — zero differences).
• Tests for these clusters moved with their code into each new file's own #[cfg(test)] mod tests, rather than staying behind in main.rs's test module.

Test plan

• cargo build — clean

• cargo clippy --workspace --all-targets -- -D warnings — clean

• cargo test --bin rocm — 971 tests before and after this change (exact match); 970 passed, 1 pre-existing ignored, 0 failures

• cargo xtask manifest --check / cargo xtask check-architecture-doc — clean

• prek run --all-files --no-group local-tools — all hooks pass

• tests/e2e-cucumber (cargo xtask e2e) — 151/153 scenarios, 0 unexpected failures (2 pre-existing xfail)

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. — N/A, not a bug fix.

• If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md (the clap declaration and dispatch wiring staying in main.rs/lib.rs is expected, not a violation). — no new subcommand; existing install driver/engines/serve command handlers relocated per this exact pattern.

• Every new or changed user-facing message was read against the code path that runs after it, and its test asserts the resulting state — not only the wording — per AGENTS.md §3. — no user-facing messages changed; pure code motion.

[jussielo-amd]

CodeQL check failure — not a new issue

The failing CodeQL check flags 6 "new" rust/cleartext-logging alerts (high severity), all on print!/println! calls that show a generated endpoint api_key to the user once at launch (see the endpoint_client_config_shows_key_once_with_bearer_guidance test) — intentional CLI UX, not a logging/persistence sink.

These aren't new: the identical pattern is already open and un-dismissed on main (alerts #780–#795, #768) in main.rs and serve_summary.rs. CodeQL's PR-diff analysis treats code landing in a new file as "new," so moving these print! calls into serve_cmd.rs as part of this extraction re-triggers alerts that are already accepted/tracked on main.

No code change made here — this refactor doesn't introduce a new sensitive-data exposure. Flagging for whoever owns the code-scanning policy to decide whether to dismiss these 6 as duplicates of the existing open alerts, or leave them open like the rest.

[rominf]

🔴 Automated review · pr-review-watcher · 2d5a4ee

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

This PR moves the install driver, engines and serve command clusters out of apps/rocm/src/main.rs into driver_install.rs, engines_cmd.rs and serve_cmd.rs, and updates docs/architecture.md. Needs work. The extraction itself is faithful, but the branch is behind main, and merging it with current main gives a main.rs that does not compile, even though git reports no conflicts.

Reviewed: the whole change, which is git diff main...HEAD over all 5 files and both commits. I also checked the blast radius: callers, re-imports, the xtask checkers, references in the docs, and how the branch merges with the 4 commits main has gained since the branch point be6650f5.

Verified:

• Moved code: comparing the moved lines against the originals shows them unchanged, apart from the added pub(crate) markers and imports.
• Dispatch call sites: byte-identical to base.
• Test count: 972 #[test]/#[tokio::test] functions both before and after.
• Lint: cargo clippy --locked -p rocm --all-targets -- -D warnings passes.
• Architecture doc check: cargo xtask check-architecture-doc passes.
• Tests: cargo test -p rocm --all-targets ran 969 tests. One failed: cli_progress::tests::animated_spinner_keeps_ticking_without_progress_calls, a timing test in a file this PR doesn't touch. It passed 3 of 3 times when re-run on its own. That run stopped at that binary, so some test targets did not run.
• Merge with main: the merged-with-main tree was checked statically with git merge-tree, not compiled.
• Not run: the Windows cross-check.

CI: CodeQL is red with 6 rust/cleartext-logging alerts. This PR did not cause them. They are the open alerts already on main (#790–#795, at the same serve_summary::render_summary print and render_endpoint_client_config test lines), relocated by the move. CodeQL is not a required check. Sphinx docs build (-W) and Skill checks (skillscope) were skipped, because this PR touches no paths they cover.

Blocking: 1 · Non-blocking: 9.

🚫 Blocking (must fix before merge)

• apps/rocm/src/serve_cmd.rs:44-49 and the main.rs import block: the branch does not build once merged with current main.

  What breaks. feat(diagnose): answer whether a model will run before downloading it #407 (5f72ac6, already on main) added code to main.rs that this PR moves out of reach:

  • assess_model_for_host and its tests call select_serve_engine(...) and read selection.engine. This PR makes select_serve_engine and ServeEngineSelection private in serve_cmd.
  • The same code calls detect_host_gpu_summary(...) unqualified. This PR drops detect_host_gpu_summary from main.rs's rocm_core import.

  git merge-tree gives a clean merge. In the merged main.rs, nothing defines, imports or glob-imports these names:

  • select_serve_engine, called at 3049 (production) and at 32491, 32508 and 32594 (tests);
  • detect_host_gpu_summary, called at 2899 (production) and at 32593 (test).

  Consequence. These are E0425 errors. The PR looks mergeable, but the merged crate does not compile, so the merge queue will eject it. A merge that bypasses the queue would break main. The second commit's premise, "ServeEngineSelection had zero remaining callers in main.rs", is also no longer true on main.

  Fix.

  1. Rebase onto current main.
  2. Make select_serve_engine and ServeEngineSelection (including its engine field) pub(crate) and re-import them in main.rs. Alternatively, move the feat(diagnose): answer whether a model will run before downloading it #407 readiness code next to them.
  3. Restore detect_host_gpu_summary to the rocm_core import in main.rs.
  4. Re-run clippy and the tests on the rebased tree.

Non-blocking

• apps/rocm/src/serve_cmd.rs:1765-1771 — the section header "Phase 9: reroute dispatch … We read this source file at test time" ended up at the end of serve_cmd's test module. The tests it introduces (main_rs_source(), launch_default_body) stayed in main.rs (~31800). Move the header back above main_rs_source().

• apps/rocm/src/serve_cmd.rs:11-13 — the module doc says start_managed_service/run_attached_service stay in main.rs "since it's shared with the background-service runner". That isn't true:

  • both functions are called only from serve_cmd.rs (846, 928);
  • spawn_managed_engine_child is called only by those two;
  • rocmd cannot depend on rocm.

  Fix the stated reason, or move the functions.

• apps/rocm/src/main.rs — some helpers whose only callers are now in the new modules stayed in main.rs. This is the same category the second commit already fixed for ServeEngineSelection:

  • collect_serve_notes (:3146) and serve_gpu_low_memory_warning (:18427) are used only by the serve path;
  • path_is_same_or_inside (:6159) and render_engine_inventory_text (:13061) are used only by engines_cmd.rs.

• apps/rocm/src/driver_install.rs:9-10 — the module doc quotes the re-import as {install_driver, reconcile_driver_install}, but main.rs:30-33 re-imports six items.

• apps/rocm/src/driver_install.rs — main.rs now imports general-purpose helpers back from driver_install for unrelated commands, which inverts the dependency direction:

  • empty_as_unknown, used by the services table at main.rs:11738;
  • read_os_release/parse_os_release_field, used at main.rs:6341;
  • run_argv_with_stdin, used at main.rs:6659.

  Consider leaving these at the crate root.

• apps/rocm/src/serve_cmd.rs:1627-1689 — render_engine_inventory_text_honors_configured_default_engine and examine_treats_a_blank_configured_engine_as_unset are filed under serve_cmd, but they exercise inventory and examine rendering that stayed in main.rs.

• apps/rocm/src/main.rs:16309 — the intra-doc link [`select_serve_engine`] no longer resolves now that the function is private in serve_cmd. CI does not run rustdoc. The blocking fix's re-import would make it resolve again; otherwise use plain backticks.

• apps/rocm/src/main.rs:27-28 — the existing comment "only the fn definitions moved out of main.rs" is now false. This PR also moves types, such as ServeArgs, ServeEngineSelection and the driver-plan types.

• docs/architecture.md:17 — says apps/rocm modules are "accessed via qualified paths (e.g. comfyui::render_status(...))". The three new modules are reached through use crate::x::{…} re-imports and unqualified calls, as automations/uninstall already are. That wording predates this PR, but it now has three more exceptions.

[rominf]

🔴 Automated review · pr-review-watcher · 2d5a4ee

The branch does not build once it is merged with current main. The full report is in this round's PR comment.

Problem. #407 (5f72ac6, already on main) added calls in main.rs that this PR leaves unresolved. git merge-tree reports a clean merge, but in the merged main.rs:

• select_serve_engine is called at 3049 (production) and at 32491, 32508 and 32594 (tests). This PR makes it private in serve_cmd, along with ServeEngineSelection.
• detect_host_gpu_summary is called unqualified at 2899 and 32593. This PR removes it from the rocm_core import.

The result is E0425 errors: the merge queue will eject the PR, and a merge that bypasses the queue would break main.

To resolve.

1. Rebase onto current main.
2. Expose select_serve_engine and ServeEngineSelection (including .engine) as pub(crate) and re-import them in main.rs, or move the #407 readiness code beside them.
3. Restore the detect_host_gpu_summary import.
4. Confirm that clippy and the tests pass on the rebased tree.

This change request will be withdrawn once the rebased head builds.

[jussielo-amd]

Rebased onto current main and pushed 4956655. Addressed:

Blocking — compile break fixed. select_serve_engine/ServeEngineSelection (incl. engine and source fields) are now pub(crate) and re-imported into main.rs; detect_host_gpu_summary is restored to main.rs's rocm_core import. cargo check -p rocm --all-targets, cargo clippy --locked -p rocm --all-targets -- -D warnings, cargo xtask check-architecture-doc, and cargo test -p rocm --all-targets (998 tests, single-threaded to rule out the pre-existing parallel-run flakes) all pass on the rebased tree. The merge conflict itself (new tests main gained at the same spot the mod tests visibility change touched) is resolved — a textual collision, not a logic conflict.

Non-blocking:

• serve_cmd.rs:1765-1771 section header — moved back above main_rs_source() in main.rs, where the tests it describes actually live.
• serve_cmd.rs:11-13 module doc — corrected: start_managed_service/run_attached_service/spawn_managed_engine_child aren't shared with anything outside this cluster; they stay in main.rs because they're entangled with other still-crate-root launch helpers (stream_attached_logs, record_cli_audit_event) that haven't moved yet, not because rocmd depends on them. Didn't do the larger relocation since it would only shift use statements, not reduce the coupling — happy to do it as a follow-up if you'd rather.
• Single-consumer helpers: moved render_engine_inventory_text and path_is_same_or_inside into engines_cmd.rs (both were already re-imported from the crate root with one external caller each). Left collect_serve_notes/serve_gpu_low_memory_warning in main.rs — tracing their dependencies (gpu_low_memory_warning, vram_capacity_is_meaningful) showed vram_capacity_is_meaningful has a second, independent production caller in main.rs (line ~3033), so moving the pair would also require re-exporting that helper back — same "just relocates the imports" problem as the spawning tail above.
• driver_install.rs:9-10 — fixed by the next point; the doc's claimed re-import list is accurate now.
• driver_install.rs general-purpose helpers — moved empty_as_unknown, parse_os_release_field, read_os_release, run_argv_with_stdin back to main.rs (crate root); driver_install.rs now imports them via use crate::{...}, restoring the intended dependency direction.
• serve_cmd.rs:1627-1689 — moved render_engine_inventory_text_honors_configured_default_engine and examine_treats_a_blank_configured_engine_as_unset to main.rs's test module, next to the other render_engine_inventory_text_with_paths tests they belong with.
• main.rs:16309 intra-doc link — resolves now as a side effect of re-importing select_serve_engine.
• main.rs:27-28 stale comment — reworded to say types and command-local helpers moved too, not just fn definitions.
• docs/architecture.md:17 — added the mechanical-relocation exception (automations.rs, uninstall.rs, and now driver_install.rs/engines_cmd.rs/serve_cmd.rs) to the qualified-paths claim.

Not done: actually relocating the managed-service-spawning tail and collect_serve_notes/serve_gpu_low_memory_warning — both would need their shared dependencies re-sorted first (see above) and felt like a separate, larger cleanup rather than part of this review pass. Let me know if you'd rather I do that here instead.

[rominf]

On whether to relocate the managed-service spawning tail and collect_serve_notes/serve_gpu_low_memory_warning here: a follow-up is fine. Those two items were non-blocking, and the reasoning you give holds up on its face. Their shared dependencies (stream_attached_logs, record_cli_audit_event, vram_capacity_is_meaningful) would need sorting out first, or the move would just relocate imports. Keeping this PR a faithful mechanical extraction also makes it easier to review.

If you defer them, it would help to make sure the module docs say why these stay in main.rs, so the reason isn't lost before the follow-up.

The rest of what you list as addressed, including the blocking compile break after the rebase, will be checked by the full review of the new head. Nothing here confirms it yet.

[rominf]

🔴 Automated review · pr-review-watcher · 3b4d8b7

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.

Informational, not binding. This report describes 3b4d8b74, not the current head. The head moved to 95f6d0af while this round ran, and 95f6d0af adds codeql[rust/cleartext-logging] suppression comments that this round did not read. Line references are relative to 3b4d8b74 and are not attached to the diff. Nothing here gates the merge. The change request from the round at 2d5a4ee remains in place for now; the next round, against the current head, will settle it. This round found that change request's objection resolved (see below).

Review — needs work (at 3b4d8b7)

Full review of the whole change: prw-base...3b4d8b74, 3 commits.
Implements ROCMAI-82 (Phase 6a: move driver-install, engines and serve out of main.rs) in full. It departs from the ticket by moving the clusters' private types too, which both the description and docs/architecture.md say openly.

Blocking

• (new) The CodeQL check fails on the reviewed head — apps/rocm/src/serve_cmd.rs:928, apps/rocm/src/main.rs:4259, main.rs:4540, main.rs:28200-28202
  The check-runs for 3b4d8b7 show CodeQL completed failure. Every other check that finished is green, and E2E tests (MI300X) was still queued.
  The flagged lines are code that existed before this change, now at new locations: the API key printed once for a public endpoint (print_managed_launch_plain, the attached-launch banner, render_endpoint_client_config and its test). The base has the same calls at old main.rs:6805/7482/7762/31575. So the alerts look like old alerts with new fingerprints after the move, not new behaviour.
  Confidence 100 · mechanical · Fix: triage the 6 alerts the same way the matching open alerts on main are handled, so the check goes green. Do not change the deliberate one-time key display.
  CodeQL is not among the branch's required status checks, so this would not have been filed as a change request in any case. 95f6d0af appears to target exactly this; the next round will look at it.

Settled standing blocker (from the round at 2d5a4ee): the branch no longer fails to build once merged with main. This round's blind review did not re-find it. It was then put to the reviewer together with the author's account of the fix; that check is anchored evidence, not a blind finding. The reviewer found that select_serve_engine/ServeEngineSelection are pub(crate) and re-imported in main.rs, and that detect_host_gpu_summary is back in the rocm_core import. On 3b4d8b74 merged with main (830f379), cargo clippy -p rocm --all-targets -- -D warnings exits 0, and cargo test -p rocm --bin rocm passes 1013 tests with 1 ignored.

Non-blocking

• (new) docs/architecture.md states the new "variant" rule on facts the code contradicts — docs/architecture.md:17-18, 28
  The code governs, because the doc's job is to describe it.
  • "Own private types used nowhere else" is false for serve_cmd.rs. ServeArgs and ServeEngineSelection are pub(crate) and used from main.rs (main.rs:2448 builds ServeArgs, and main.rs:3087 calls select_serve_engine). Only ServeGenerationDefaults is private.
  • "The dispatch fn itself relocates (install_driver()/…)" is wrong for install_driver. The install() dispatcher stays in place (main.rs:3419 calls it at 3566), and so does dispatch() (main.rs:2112). What moved are per-command handlers. That is the only distinction the note draws from full domain extraction.
  • "Mechanically relocated modules are the exception — reached through use crate::x re-imports" is wrong for endpoint_keys.rs and logging.rs, which the same doc classes as mechanically relocated. They are reached by qualified path (endpoint_keys::… throughout main.rs and serve_cmd.rs, and logging::init).
  • "Helpers … also called from serve_cmd.rs and from other root-level commands": only engine_manages_own_runtime is used from both places. The rest are used from one or the other.
    Confidence 85 · mechanical · Fix: say "handler fn" rather than "dispatch fn", drop the "used nowhere else" claim for serve_cmd.rs, and limit the unqualified-access sentence to the five modules it actually covers.
• (new) The new module headers and the main.rs re-import comment describe imports that do not exist — apps/rocm/src/driver_install.rs:11-12, engines_cmd.rs:9-10, serve_cmd.rs:9-10, main.rs:26-30
  • The three headers say "InstallTarget/Cli remain in the crate root and are reached through crate::". Neither name appears in any of the three files outside that comment (checked with grep).
  • The main.rs comment calls select_serve_engine (a fn) a "selection type".
  • It also says "these re-imports" keep the dispatch call sites byte-identical. That is true for the handler imports. The select_serve_engine import, however, serves assess_model_for_host (main.rs:3087), not dispatch.
    Confidence 85 · mechanical · Fix: say the types stay at the root and are not referenced from these modules, and describe the select_serve_engine import as serving the assess_model_for_host caller.

Decisions for the author

• Serve-only helpers stay in main.rs, but the serve_cmd.rs header explains only the spawn tail — non-blocking-improvement
  These have no non-test caller in main.rs and are used only by serve(): collect_serve_notes, validate_bind_host, resolve_endpoint_auth, ensure_public_bind_engine_supported, validate_pinned_gpu_index and print_managed_launch_plain. They are pure policy and formatting, not the entangled spawning tail the header gives as the reason for leaving things behind. Either move them in a later phase or widen the header's explanation. Leaving them is not wrong, since it keeps this diff smaller.
• Moving the cluster-private types contradicts the ticket's "move only the pub(crate) fn bodies" — tradeoff
  Moving them shrinks main.rs and keeps each cluster together. Following the ticket keeps the mechanical-relocation pattern pure. The PR picks the first option and says so in its description and in docs/architecture.md. Confirm that the ROCMAI-27 sequencing for 6b–6f accepts this variant.

Positive signals

• Commit 3 brings back the select_serve_engine/detect_host_gpu_summary reachability that feat(diagnose): answer whether a model will run before downloading it #407's assess_model_for_host needed after the rebase. It does this by re-importing them, not by duplicating the logic, so engine selection keeps its single implementation.

Deployment notes

None

What this covered

• Faithfulness of the move. Read the diff of all 5 files at prw-base…3b4d8b74 (merge-base 70d6aa1).
  • Removed and added lines across apps/rocm/src are identical apart from visibility (pub(crate)), use lists, module docs and mod lines. Every removed hunk reappears verbatim and in the same order at HEAD, broken only at function boundaries, so function bodies are unchanged.
  • #[test] count: 1012 at the merge-base and 1012 at HEAD. 1016 on both the base tip and the merged tree.
• Merged tree (3b4d8b74 + main, no conflicts, no markers): cargo clippy -p rocm --all-targets -D warnings exit 0; cargo test -p rocm --bin rocm 1013 passed, 1 ignored; cargo test -p xtask env_mutation 45 passed; cargo xtask check-architecture-doc exit 0.
• Tests. The change adds no tests; existing ones moved with their code. Production code is unchanged, so the per-test mutation verdicts do not apply, and no new e2e scenario is needed.
• Fan-out. One read-only worker checked the prose claims. The code passes were replaced by the mechanical line and hunk equivalence check above. The design questions were done in a single context, not by independent workers.
• Did not run: the prior-changes pass; a workspace-wide clippy; the GPU lanes. 95f6d0af was not read.
• Not reconciled against any PR discussion here.

[rominf]

🔴 Automated review · pr-review-watcher · ffc8177

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.

Round at ffc81772. The change request from the round at 2d5a4ee7 ("the branch does not build once merged with main") is being withdrawn after this review. The details are under Settled standing blocker below. The blocking item below is published as a comment and not as a change request. CodeQL is not a required check for main: branch protection lists 17 required contexts, CodeQL is not one of them, and there is no ruleset. The 5 alerts it reports are the same rust/cleartext-logging alerts that are already open on main (#790–#795, at old main.rs:6805/7481/7762/31576-31578), now at new locations because the code moved. So the only thing merging would leave behind is suppression comments that may not take effect. Where this report calls CodeQL "required", that is incorrect.

Review — needs work

Full review of the whole change.
Implements ROCMAI-82 (Phase 6a: move the driver-install, engines and serve clusters out of main.rs). It departs on purpose from the ticket's "move only the fn bodies, keep shared types at the root": cluster-owned types moved too. The commit messages and docs/architecture.md both say so. The PR description was not read; see coverage.

Blocking

• (standing since 3b4d8b7, revised) The required CodeQL check fails at the head commit, and the suppression comments added for it are probably in the wrong place — apps/rocm/src/main.rs:4259-4264, apps/rocm/src/main.rs:4544-4547, apps/rocm/src/serve_cmd.rs:936-939, apps/rocm/src/main.rs:28207-28210
  • statusCheckRollup for ffc8177 shows CodeQL | COMPLETED | FAILURE. Every other check passed.
  • Commit 95f6d0a added // codeql[rust/cleartext-logging] comments to silence these alerts. In four of the six places the marker is the first line of a three-line comment, so it is not on the line directly above the flagged statement. At main.rs:4259 it is four lines above render_endpoint_client_config(&report.endpoint_url, key).
  • Only the single-line markers at main.rs:28211 and main.rs:28213 sit directly above their statement.
  • The failing check is confirmed. That the placement causes it is inferred: I could not run CodeQL.
  • Confidence 85 · mechanical · Fix: put the codeql[...] marker on the line directly above the flagged statement, with the explanation above it, then confirm CodeQL goes green. If an alert stays open, dismiss it in code scanning with a reason.

Settled standing blocker (from the round at 2d5a4ee): the branch now builds once merged with main. This round's blind review checked the merged tree on its own initiative and found it builds. The objection was then put to the reviewer together with the author's account of the fix. That second check is anchored evidence, not a blind finding.

• On ffc81772 merged with current main (4fd7c810, a clean merge), select_serve_engine and ServeEngineSelection (both fields included) are pub(crate) and re-imported at main.rs:38.
• detect_host_gpu_summary is back in the rocm_core import at main.rs:55.
• cargo check and cargo clippy -p rocm --all-targets -- -D warnings exit 0.
• cargo test -p rocm --bin rocm passes 1013 tests, with 1 ignored.
• The fix landed in 3b4d8b7, and no later commit in the range undoes it.

Non-blocking

• (new; overlaps a 3b4d8b7 finding) New doc text says things the code contradicts — docs/architecture.md:18, apps/rocm/src/engines_cmd.rs:11-14, apps/rocm/src/serve_cmd.rs:8-9
  • In each case the code is what governs, because it is what compiles. Correct the text.
  • architecture.md says "driver_install.rs's and engines_cmd.rs's types are used nowhere else". But DriverInstallResult and DriverInstallError (driver_install.rs:304-311) are pub(crate) with pub(crate) fields, and install() in main.rs:3567-3593 reads result.output, result.executed, error.executed and error.source. Only the plan/state types are private to the module.
  • The engines_cmd.rs header says the helpers other than engine_manages_own_runtime "are used only from other root-level commands in main.rs". ensure_self_managed_engine_ready has no caller in main.rs; its only external caller is serve_cmd.rs:737. The header also leaves out that engine_manages_own_runtime is called from main.rs:8332.
  • The serve_cmd.rs header says it is re-imported via use crate::serve_cmd::{serve, ServeArgs};. The actual import at main.rs:38 is {ServeArgs, select_serve_engine, serve}.
  • Confidence 85 · mechanical · Fix: rewrite the three sentences to match the call graph.

Decisions for the author

• (new) main.rs's tests module now serves as a shared fixture module — tradeoff
  • mod tests became pub(crate) mod tests, and ScopedTestEnv, test_paths, write_test_pip_runtime and test_runtime_manifest_for_update were widened so the three new modules can import them from crate::tests::.
  • No earlier extraction in apps/rocm reaches into another module's tests like this. It keeps the diff to a pure move.
  • The alternative is a dedicated #[cfg(test)] test-support module (as rocm-core has test_support.rs). That is a cleaner base for phases 6b–6f, but the diff here would no longer be a pure move.
  • The pattern will spread with each later phase, so choose it on purpose.
• (new) Two tests moved into serve_cmd.rs only exercise code that stayed in main.rs — non-blocking-improvement
  • hybrid_planner_bakes_the_host_engine_into_the_generated_serve_command (serve_cmd.rs:1000) tests build_freeform_plan_with_recipes (main.rs:16850). Its sibling hybrid_planner_* tests stayed in main.rs.
  • launch_lock_makes_gpu_select_and_claim_atomic (serve_cmd.rs:1644) tests select_gpu_indices_under_launch_lock (main.rs:18614).
  • Both are about serve, so this is defensible. The cost is that the planner tests are now split across two files.

Positive signals

• The move is faithful.
  • I compared the multiset of removed lines with the multiset of added lines across apps/rocm/src. Everything matches except visibility widenings, use imports, module headers, mod declarations and the CodeQL comments.
  • The dispatch call sites are unchanged.
  • The set of test names in the merged result is identical to prw-base's set (536).
• Documenting the "mechanical relocation with owned types" variant in docs/architecture.md in the same change keeps the module map current. AGENTS.md §5 and the ticket both ask for that.

Deployment notes

None

What this covered

• Read: the whole change at 70d6aa1 (merge-base)…ffc81772.
  • The three new modules were read completely by two fan-out workers. A third worker read docs/architecture.md and checked AGENTS.md compliance and history.
  • I checked the main.rs changes with a line-multiset equivalence check plus reading the hunks that were not moves.
• Merge with current base:
  • The branch is 8 commits behind prw-base. git merge-tree against prw-base (4fd7c81) merges cleanly.
  • On the merged tree, cargo check -p rocm --all-targets and cargo clippy -p rocm --all-targets -- -D warnings both exited 0 on Linux.
  • Tests were not run locally; CI's build-and-test and windows-build-and-test passed at the head.
• Did not run:
  • The prior-changes pass: earlier review comments are off-limits to this run.
  • The design questions (Step 7) were worked through by the driver, not by independent workers.
  • The Windows cfg arms were checked by reading only; none of the three new files gate non-test code on cfg.
  • cargo xtask check-architecture-doc was not run locally; its CI check passed.
• Intent:
  • The PR description was not read, because this run was limited to the statusCheckRollup gh call. Intent was judged from the five commit messages and ROCMAI-82, which was retrieved.
  • The ticket's acceptance criteria are met: unchanged dispatch call sites, tests moved with their clusters, unchanged test count, architecture doc updated in the same PR.
• Not reconciled against the PR's discussion here.

Resolved: on ffc8177 merged with current main (4fd7c81), select_serve_engine/ServeEngineSelection are pub(crate) and re-imported, detect_host_gpu_summary is back in the rocm_core import, and cargo check, clippy -D warnings and cargo test -p rocm --bin rocm all pass. See review 5441388086.