[jussielo-amd]

Why

apps/rocm/src/main.rs was still about 9,500 lines because it held the command handlers themselves, not just the clap declarations and dispatch, so every change to any command touched the same file. This change moves the remaining handler clusters out into their own modules, as pure moves, until main.rs is mostly the CLI definition, dispatch and glue: it goes from 9,506 to 4,895 lines, with the production part ending at line 2,545 and the rest being its tests. Completes ROCMAI-759; moving the comfyui dispatch function, which that ticket also listed, is tracked separately as ROCMAI-833 because it needs a maintainer decision first.

What changes

Fifteen commits, one per step, so the branch can be reviewed commit by commit. Each moves one cluster out of main.rs into a module of its own, with its tests beside it and an entry in docs/architecture.md: cli_audit.rs (the CLI audit and lifecycle log), service_hints.rs (the service action label helpers), service_records.rs (service-record removal and pruning), services_cmd.rs, config_cmd.rs, model_fit.rs (the model-fit assessment), diagnose_cmd.rs (diagnose, report delivery and fix), install_cmd.rs (which also takes sdk_install_request and its two tests from chat.rs), freeform_answer.rs, setup_cmd.rs, bridge_snapshot.rs (the Codex-bridge snapshot builders), examine_cmd.rs, collect_serve_notes into serve_cmd.rs, and the update and chat command arms.

The dispatch() call sites are unchanged except for the Update and Chat arms, whose bodies are now calls to update() in update_cmd.rs and run_chat_command in a new chat_cmd.rs; both are new functions that take the values the arm used to bind, so those two arms are not byte-identical moves.

Deviations from a plain move, each also stated in the relevant commit message:

• The chat handler goes into a new chat_cmd.rs rather than into chat.rs, because chat.rs is already reached from dash.rs and the handler calls dash::run_chat, so placing it in chat.rs would create a chat and dash cycle. chat_cmd.rs depends on both and neither depends on it. The four command_chat_* source-text tests keep their names and assertions and now live in chat_cmd.rs's own test module, where they read the handler next to them (they share slice_braced_block and strip_line_comments with main.rs's tests); each of the four was checked to go red when the handler is broken.
• Items that the new modules or the remaining main.rs tests use are widened to pub(crate), and a few are re-imported under cfg(test).
• prompt_text::any_substring does not exist, so the freeform answers use chat::any_substring.
• update() and run_chat_command carry a #[allow] for the clippy lints their longer signatures trigger (fn_params_excessive_bools, too_many_arguments).
• Path pins and comments that named main.rs follow the moved code: the ascii_output_contract exempt literals in xtask, the advised_commands assertions, docs/testing.md, AGENTS.md, and a doc comment in rocm-dash-tui.
• docs/architecture.md statements that the moves made false are corrected in the commit that made them false.

Verification

• cargo fmt --check is clean, and cargo clippy -p rocm --all-targets -- -D warnings is clean on the current head, which is rebased onto the latest main. cargo test -p rocm --bin rocm passes: 1,379 passed and 7 ignored, none failed.
• The leaf-name test lists are identical before and after the moves: 1,376 tests and 7 ignored, none lost or duplicated, and each commit builds and passes its moved module's tests. The subprocess-style test whose filter path changed (model_fit::tests::docker_unreachable_subprocess_helper) was shown to run its helper by breaking the helper and seeing the parent test fail.
• cargo xtask manifest --check and cargo xtask check-crate-edges pass, and a scan found no new two-module cycle. The use crate:: edges each new module has in its production code (explicit use crate:: imports and crate::<module>:: paths, test code excluded) are listed below; items that stay in main.rs are named separately.
  • cli_audit: none.
  • service_hints: none, apart from SandboxToolArg from main.rs.
  • service_records: cli_progress, cli_report, endpoint_keys, managed_service_status, service_hints, services_text.
  • services_cmd: cli_audit, service_hints, service_records, services_text; ServicesCommand from main.rs.
  • config_cmd: cli_audit, provider_keys, providers, runtimes_cmd; ConfigCommand and provider_name from main.rs.
  • model_fit: gpu_policy, serve_cmd.
  • diagnose_cmd: model_fit; FixExitCode from main.rs.
  • install_cmd: cli_audit, driver_install, runtime_install_finalize, therock; InstallFormat and InstallTarget from main.rs.
  • freeform_answer: chat, comfyui, freeform_planner, runtimes_cmd, therock.
  • setup_cmd: runtimes_cmd, therock; SetupCommand from main.rs.
  • bridge_snapshot: managed_service_status.
  • examine_cmd: examine_report_text, runtimes_cmd, therock.
  • serve_cmd (which gained collect_serve_notes): cli_progress, endpoint_keys, engine_ipc, engine_runtime_select, engines_cmd, gpu_policy, managed_service_status, serve_summary, service_launch; DevicePolicyArg and device_policy_name from main.rs.
  • update_cmd: cli_audit, runtime_services, runtimes_cmd, therock; ROLLBACK_RECOVERY_HINT, engine_inventory and sanitize_log_value from main.rs.
  • chat_cmd: chat, dash; ChatInferenceParams, Provider and provider_name from main.rs. Neither chat nor dash depends on it.
• Not run locally: the full workspace tests, cargo test -p xtask (the edited ascii_output_contract pin was only compiled), the tests/e2e-cucumber suite (including the scenarios for rocm update and rocm chat that exercise the two moved arms) and the Windows lane. No Windows-only code moved.

Scope

This is an internal refactor with no user-observable change, so no end-to-end scenario is added; the unmodified tests/e2e-cucumber suite covers the unchanged behaviour. The move of the comfyui dispatch function is deliberately not included (it is ROCMAI-833: it contradicts the current comfyui.rs entry in docs/architecture.md, so the convention has to be decided first), and a few other open PRs (for example #817, #799, #709, #826 and #837) edit code that moved here and will need rebasing onto the new modules.

Template checklist: not a bug fix, so there are no stale xfail rows to narrow; no new subcommand or subsystem; no user-facing message is added or changed.

[Copilot]

🔵 Needs a closer look

Missing main.rs and service_records.rs diffs prevent verification of the broad CLI relocation, and maintainer confirmation is needed for move equivalence and full-workspace, end-to-end, and Windows validation on the rebased head.

No open issues 🎉

What changed in this PR

This PR continues the CLI modularization by moving command handlers and supporting helpers out of main.rs, leaving it focused on CLI definitions, dispatch, and glue. The changes are intended to preserve behavior; the ComfyUI dispatch extraction remains a separate follow-up.

Changes:

• Relocates command handlers, audit logging, model-fit assessment, and bridge helpers into focused modules alongside their existing tests.
• Extracts the update and chat dispatch bodies into functions, keeping chat orchestration separate to avoid a circular module dependency.
• Updates imports, visibility, source-path assertions, and architecture documentation to follow the relocated code.

File	Description
xtask/​src/​ascii_output_contract.rs	Retargets the spinner-literal exemption to its new module.
docs/​testing.md	Updates the service-pruning test location.
docs/​architecture.md	Documents extracted modules and revised dependencies.
crates/​rocm-dash-tui/​src/​ui/​examine_manager.rs	Updates a comment identifying the refusal-text source.
apps/​rocm/​src/​update_cmd.rs	Receives the update command handler.
apps/​rocm/​src/​setup_cmd.rs	Houses setup handlers, rendering, and existing tests.
apps/​rocm/​src/​services_text.rs	Updates helper imports and module documentation.
apps/​rocm/​src/​services_cmd.rs	Houses service dispatch and approval-gated actions.
apps/​rocm/​src/​service_launch.rs	Updates comments describing relocated service helpers.
apps/​rocm/​src/​service_hints.rs	Receives service-action label and tool-mapping helpers.
apps/​rocm/​src/​serve_cmd.rs	Receives serve-warning collection and its tests.
apps/​rocm/​src/​runtime_install_finalize.rs	Updates the documented install-handler location.
apps/​rocm/​src/​model_fit.rs	Houses model-fit assessment, platform gates, and tests.
apps/​rocm/​src/​managed_service_status.rs	Updates documentation of callers.
apps/​rocm/​src/​install_cmd.rs	Houses install handlers, SDK request mapping, and tests.
apps/​rocm/​src/​gpu_policy.rs	Updates documentation references to moved helpers.
apps/​rocm/​src/​freeform_answer.rs	Houses read-only freeform answer rendering.
apps/​rocm/​src/​examine_report_text.rs	Updates module documentation for relocated callers.
apps/​rocm/​src/​examine_cmd.rs	Houses examine dispatch and JSON document types.
apps/​rocm/​src/​driver_install.rs	Updates comments identifying the install caller.
apps/​rocm/​src/​diagnose_cmd.rs	Houses diagnose, fix, report delivery, and existing tests.
apps/​rocm/​src/​config_cmd.rs	Houses configuration commands, rendering, and tests.
apps/​rocm/​src/​cli_audit.rs	Houses audit and lifecycle logging with existing tests.
apps/​rocm/​src/​chat.rs	Removes SDK request mapping and tests relocated to install.
apps/​rocm/​src/​chat_cmd.rs	Houses chat orchestration separately from chat internals.
apps/​rocm/​src/​bridge_snapshot.rs	Houses bridge snapshot and engine-inventory builders.
apps/​rocm/​src/​advised_commands.rs	Updates source-path assertions for moved command text.
AGENTS.md	Updates the setup-test documentation link.

_{Review effort: Balanced}

[nowycondro]

🤖 Automated review (Claude Code) · b7776c4

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.

Review — no blocking findings

Full review of the whole change. Implements PRs 1–15 of ROCMAI-759 (Phase 8.1-G): mechanically moving 13 command-handler/helper clusters, plus the Update and Chat dispatch-arm bodies, out of apps/rocm/src/main.rs into their own modules. PR 16 of that ticket (moving the comfyui dispatch function) is explicitly deferred to ROCMAI-833, consistent with the ticket's own "Open decision" section requiring a maintainer call on where the clap definition lives before that move happens. The ticket's own automation note says only the terminating PR for its scope may name the ticket key in its body (earlier PRs must not, or automation closes the ticket early) — this PR does name it, which is consistent with it being the last PR against the (now comfyui-excluded) scope of ROCMAI-759, not a premature auto-close risk.

Blocking

None.

Non-blocking

• cli_audit.rs's own module doc comment is stale about its caller list — apps/rocm/src/cli_audit.rs:14-19
  The doc comment (added verbatim by this PR's first commit, f622129, and never touched by any later commit in this PR) enumerates the external callers of record_cli_audit_event as main.rs, chat.rs, engines_cmd.rs, runtime_install_finalize.rs, runtimes_cmd.rs, service_launch.rs, storage.rs. At the PR's actual head, git grep -l 'record_cli_audit_event' apps/rocm/src also shows config_cmd.rs, install_cmd.rs, service_records.rs, services_cmd.rs, and update_cmd.rs as callers — all five added by this same PR's own later commits, none reflected back into this list. By contrast, service_hints.rs's equivalent doc comment was correctly updated by a later commit (dc4625a) when its caller moved — so the project clearly treats this kind of caller-list accuracy as something each PR in this stack is expected to maintain, and this one instance slipped through.
  Confidence 85 · mechanical · Fix: extend the caller list (or genericize the wording, the way the docs/architecture.md entry for the same module already does) to include the five newer callers.

Decisions for the author

• Giving the Chat arm its own chat_cmd.rs instead of folding it into chat.rs — tradeoff
  The ticket's own plan (ROCMAI-759 PR 15) called for moving the Command::Chat arm body into a function inside the existing chat module. The PR deviates and creates a new chat_cmd.rs instead, because chat_cmd's handler calls both chat:: and dash::, and dash.rs already calls into chat.rs — folding it into chat.rs would create a chat ↔ dash cycle. Verified directly: chat.rs has no use/call into dash, dash.rs calls into chat.rs, and chat_cmd.rs imports both. This is a reasonable, disclosed deviation from the ticket's plan that avoids a real cycle, at the cost of one more small dispatch-only file in an already large (13-new-file) module count for this single ticket. Worth the author/maintainers confirming they're comfortable with the module count this stacked ticket is producing, versus, e.g., folding some of the smallest dispatch-only modules together.

Positive signals

• Every one of the 15 commits was independently verified byte-identical between its pre-move location (diffed against that specific commit's own parent, not a looser main-relative diff) and its new module, modulo only the documented pub(crate) visibility widenings — across all clusters, including the two that are a real code-shape change rather than a textual move (update()/run_chat_command, extracted from inline dispatch() match arms into standalone functions with the same body, same parameter order, and narrowly-scoped #[allow(clippy::...)] attributes matched to the exact lint each longer signature triggers).
• The stated rationale for chat_cmd.rs avoiding a chat ↔ dash module cycle checks out against the actual use statements, not just the commit prose.
• The correctness-critical FixExitCode error-chaining detail (an explicit code comment warns that re-stringifying the error would silently break its exit-code-carrying downcast) is preserved exactly in the moved fix(), and the regression test exercising that exact path was deliberately left in main.rs rather than moved, so it still proves the chain end-to-end.
• A subtle test-portability hazard — a subprocess test that re-invokes itself by a hardcoded test-path string (model_fit::tests::docker_unreachable_subprocess_helper) — was correctly updated for the module's new location, with no stale "tests::..." string left anywhere in the tree.
• docs/architecture.md is updated in the same commit as each move (never deferred), and its new/updated entries were independently verified against the code (callers, pub(crate) rationale, dependency direction, exact test counts) rather than taken on faith — including entries for files this PR didn't even touch directly, corrected because the move made their existing text false.
• All 15 commits carry DCO sign-off from a real company identity and no AI-attribution footers, consistent with this repo's OSS-contribution requirements.
• The one bot-review concern on this PR ("missing main.rs and service_records.rs diffs prevent verification") does not hold up: both files are fully present in the diff fetched directly via git (main.rs 1,464+/6,075-, service_records.rs a new 1,759-line file) and were fully reviewed; this looks like a diff-rendering limitation of whatever surfaced that bot's view, not a gap in the actual PR contents.

Deployment notes

• The PR description itself names several other open PRs (#817, #799, #709, #826, #837) that touch code relocated here and will need rebasing after this merges — a coordination point for whoever merges next, not a defect in this change.
• CI for the reviewed head (b7776c4a) is green on every lane that ran: clippy, the main build-and-test/affected-crates lanes, Windows build-and-test, the E2E suite and its consolidated report, Strix Halo (Ubuntu current, WSL2), commit-signatures+sign-off, markdown-links (lychee), license headers, coverage (ratcheted), and CodeQL. Four hardware-gated E2E lanes (Strix Halo Ubuntu nightly, Strix Halo Ubuntu release, W7900, MI300X) show queued rather than completed for this head — not a failure, but hardware-only verification on those specific lanes isn't confirmed either way from CI alone.

What this covered

Every one of the 15 commits in 6eeec62...b7776c4 (the PR's full range against origin/main, fetched read-only via pull/840/head), read in full via 8 independent fan-out workers grouped by cluster coupling, each diffing the moved code against that specific commit's own parent (not a single main-relative diff, to avoid misattributing earlier stacked commits' changes). This includes apps/rocm/src/main.rs's full diff and the new service_records.rs (1,765 lines) in their entirety, despite a prior bot review's claim that these were unavailable for review — both were fetched and read completely via git. Also covered directly by the reviewing driver: CI status for the head commit, commit metadata (DCO sign-off, no AI footers) for all 15 commits, the full docs/architecture.md/AGENTS.md/docs/testing.md diffs, the final main.rs structure (confirming only the ticket's "stays" list of functions remains before its test module), the chat/dash/chat_cmd import graph, and the named ticket ROCMAI-759 (retrieved directly by key, not via a link in the PR body).

Did not run: a local cargo build/cargo test/cargo clippy (no Rust toolchain available in this review environment) — relied on the PR's own green CI for that verification instead, per the above. History and prior-changes passes were folded into each worker's per-commit diffing rather than run as separate passes, since the "history" here is the commit stack itself. Not reconciled against this PR's own review discussion or existing comments here — this review was produced blind to that, by design.

---

🤖 Generated with Claude Code

[juhovainio]

I went through the diff and found nothing blocking. The moves look behaviour-preserving: the removed and added lines differ only in pub(crate) widening, rustfmt re-wraps, use blocks and doc text. The test count is unchanged (75 moved), the Update and Chat arm bodies are identical to before, and the architecture doc and path pins are updated.

Things worth a look before or after merge (none blocking):

• The PR is currently conflicting with main and needs a rebase. After that, check that nothing #817/#799/#709/#826/#837 added to main.rs was dropped or re-added.
• The PR says it completes ROCMAI-759, but the ticket still lists the comfyui dispatch move and "main.rs holds only the stays list". That move is deferred to ROCMAI-833, so the ticket text should be amended before the key auto-closes it.
• The four command_chat_* source-text tests stay in main.rs but read chat_cmd.rs. They would sit more naturally in chat_cmd.rs's own tests.
• The ticket asks for each new module's use crate:: edges in the PR body; the body only says a scan found no new two-module cycle.
• The two Strix Halo Ubuntu E2E jobs (release, nightly) hit the 45m limit with the step cancelled and no assertion failure visible. The same hardware passes on WSL2 and Windows, so this looks like infrastructure, but a rerun would confirm it.

This is a diff-only read; it doesn't replace CI or a local run.

[juhovainio]

Nit: this doc line was joined rather than re-wrapped after the edit and now runs well past the surrounding width; re-wrap it like the neighbouring lines.

[jussielo-amd]

Re-wrapped in 5f1694d: the sentence now breaks after "also re-imported there." so the lines match their neighbours.

[jussielo-amd]

Thanks for the reviews. The branch is rebased onto the latest main (two commits landed since the last push, #703 and #848; the only conflicts were in serve_cmd.rs doc comments) and the review fixes are in one new commit on top, 5f1694d. The earlier approval and the line-wrap thread are on the previous head; the diff since then is that one commit plus the rebase.

What changed, by finding:

• cli_audit.rs caller list is stale (nowycondro): the module doc no longer enumerates callers at all, since it had already drifted from the code. It now says git grep for the name finds them and keeps only which items are pub(crate), which matches the docs/architecture.md entry.
• The four command_chat_* tests belong in chat_cmd.rs (juhovainio): moved into that module's own mod tests with their names and assertions unchanged. slice_braced_block and strip_line_comments are now pub(crate) in main.rs's tests and shared through crate::tests::, the same way cli_audit.rs shares test_paths. The chat_cmd.rs header and the docs/architecture.md entries for chat.rs and chat_cmd.rs follow. I broke the handler (provider.is_some() flipped) and confirmed command_chat_interactive_notes_dropped_provider_flag fails from its new location, then reverted.
• Over-long doc line in examine_report_text.rs (juhovainio, inline): re-wrapped.
• use crate:: edges per new module (juhovainio): added to the Verification section of the PR body, from production code only.
• Rebase and what feat(serve): exempt reuse of a running service from the no-GPU refusal #703 added to main.rs (juhovainio): feat(serve): exempt reuse of a running service from the no-GPU refusal #703's additions to main.rs are doc comments on the serve command; all three paragraphs I spot-checked are present exactly once. fix(serve): keep startup progress until the server is ready or stalled #817, fix(services): don't report an unconfirmed stop as stopped #799, fix(rocm): verify start-ticks in managed service liveness #709, feat(diagnose): estimate context size and memory for a GGUF model #826 and ROCMAI-823: refuse vLLM serve and install up front on native Windows #837 are still open, so nothing from them was involved.

Checked on this head: cargo fmt --check and cargo clippy -p rocm --all-targets -- -D warnings are clean, and cargo test -p rocm --bin rocm passes with 1,379 passed and 7 ignored. The pre-push hooks (signatures and sign-off, license headers, clippy including the e2e harness, cargo test) also passed.

Not addressed here: the ticket text for the deferred comfyui move is a ticket-side change rather than part of this diff, and the two Strix Halo Ubuntu E2E timeouts are not something I could rerun from here, so the merge-queue run on the rebased head will be the check on those lanes.

[nowycondro]

🤖 Automated review (Claude Code) · 5f1694d

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.

Review — needs work

Full review of the whole change (merge-base c6ab82b54f685b01e6a3dacd87403c5fd16c9c4c to head 5f1694d5af91c20048e41ce559a12f8a5a4f931f, all 16 commits, fetched read-only via pull/840/head into a scratch clone, never checked out). Every file in the diff read completely by 7 independent fan-out workers grouped by cluster coupling, plus a dedicated full-main.rs/docs pass and a dedicated impact-analysis/ripple/history pass across the whole repository.
Implements all 15 "PR" steps of ROCMAI-759 (the ticket's own comments confirm "Steps 1 to 15 are covered by #840"); step 16 of that ticket (moving the comfyui dispatch function) is explicitly and separately tracked as ROCMAI-833, pending a maintainer decision on where the clap definition lives — consistent with this PR's own description.

Blocking

• The branch is not rebased onto the current tip of main, and the description's "rebased onto the latest main" claim does not hold for this head — integration risk, not a defect in the reviewed code itself
  git merge-base --is-ancestor 6b175eeeb1e881d4b293b9670894fb1811f61de1 pr/840 fails: main's tip (6b175ee, "ROCMAI-829: report an unknown service id without the OS error", merged the same morning, before this PR's own last push) is not an ancestor of this PR's head; GitHub reports the PR as blocked/not mergeable. That main commit and this PR both touch apps/rocm/src/services_text.rs and docs/testing.md, in adjacent-but-currently-disjoint hunks (confirmed by diffing both changesets against their shared base) — so a mechanical rebase looks likely to apply cleanly, but this is unverified by any actual rebase or build, since neither has happened yet.
  Confidence 100 (directly confirmed by git merge-base --is-ancestor and by diffing both changesets against the same base) · architectural · Fix: rebase onto current main, re-run cargo clippy/cargo test, specifically re-check the services_text.rs area (read_managed_service_manifest) after the rebase, and update the PR description's verification section once that's done.

Non-blocking

• Two functions stay pub(crate) with no external caller, inconsistent with this stack's own visibility-tightening pattern — apps/rocm/src/update_cmd.rs:253,257 (update_should_preview_or_apply, apply_runtime_update)
  Both are called only from update() and their own tests, all within update_cmd.rs itself — every other hit of either name across apps/rocm/src is a doc-comment mention, not a call. chat.rs's own docs/architecture.md entry states this stack's "visibility audit tightened every other function to module-private, so each function still pub(crate) has a confirmed external caller"; these two do not.
  Confidence 85 · mechanical · Fix: make both a private fn rather than pub(crate) fn.
• Doubled word in a doc comment — apps/rocm/src/service_launch.rs:24
  Reads "pruning it dispatches to to service_records.rs".
  Confidence 85 · mechanical · Fix: drop the repeated "to".

Decisions for the author

• 13 new single-purpose files (plus 2 dispatch-arm extractions) for one ticket, several dispatch-only with no owned types (chat_cmd.rs, bridge_snapshot.rs, service_hints.rs) — tradeoff
  Every module boundary checked out as correct and cycle-free — e.g. chat_cmd.rs depends on both chat.rs and dash.rs precisely because folding it into either would create the cycle dash.rs's existing call into chat.rs would otherwise close — so nothing here is wrong, but it is a lot of small files for one ticket. Worth the author/maintainers confirming they're comfortable with the resulting module count versus, e.g., folding the smallest dispatch-only modules together.
• Two e2e-cucumber feature-file comments went stale because of this PR's own moves — answered-by-intent
  tests/e2e-cucumber/features/model_catalog.feature:15 still says "see engine_ruled_out_by_platform in main.rs" (moved to model_fit.rs by this PR), and tests/e2e-cucumber/features/service_stop.feature:3 still says "the approval gate in main.rs" (moved to services_cmd.rs). ROCMAI-759's own "Out of scope" section says: "Pre-existing stale comments this ticket does not own: ... e2e-cucumber feature comments citing main.rs (the suite stays unmodified)." That is a blanket, written decision to leave this whole class of comment stale rather than touch the e2e suite as part of these move PRs — these two would otherwise have been non-blocking findings. (Several other e2e-cucumber comments citing main.rs were already stale before this PR, from unrelated earlier moves; the same policy covers those too.)

Positive signals

• Every one of the 15 move-commits was independently verified content-identical (whitespace/rustfmt-reflow aside) against its own immediate pre-move location by diffing, not by eye — including the two non-literal moves (update()/run_chat_command, extracted from inline match-arm bodies with identical parameter order and narrowly-scoped #[allow(clippy::...)] matched to the exact lint their longer signatures trigger).
• The correctness-critical FixExitCode downcast chain in diagnose_cmd.rs's fix() is preserved exactly, and its regression test was deliberately left in main.rs so it still exercises the real dispatch → fix → FixExitCode → exit_code_for path rather than a stub.
• A subprocess test that re-invokes itself by a hardcoded module-path string (model_fit::tests::docker_unreachable_subprocess_helper) was correctly updated for the new location.
• Module doc comments and docs/architecture.md entries are unusually rigorous and self-correcting across the stack: every caller-list, pub(crate)-rationale, and dependency-direction claim spot-checked (well over a dozen, across all seven review passes) verified true against the actual code via git grep, including entries this PR didn't directly touch but whose text the moves made stale.
• The newest commit (5f1694d5, a post-review fixup) is itself clean, correctly-executed work: it relocates four tests to sit with the handler they assert on (with matching pub(crate) widening of the shared slice_braced_block/strip_line_comments test helpers so the cross-module reference actually resolves), and replaces a stale hardcoded caller-list doc comment with a self-maintaining "git grep finds them" pointer.
• No dead code, no orphaned modules (all 13 new files are declared and reachable), and no internal/proprietary names or secrets anywhere in the diff.

Deployment notes

• This PR cannot currently merge because it is one commit behind main's tip (see Blocking, above). Whoever rebases should specifically re-diff apps/rocm/src/services_text.rs and docs/testing.md against the post-rebase result: main's 6b175ee changed read_managed_service_manifest's body in the former and inserted a paragraph in the latter, in hunks adjacent to but not overlapping this PR's own edits to the same two files — a clean textual merge is likely but has not actually been attempted or built.
• The PR description names five other open PRs (#817, #799, #709, #826, #837) that touch code relocated here and will need rebasing after this merges — a coordination point for whoever merges next.
• CI for the reviewed head (5f1694d5) was green on every lane that had completed as of this review (clippy, build-and-test/affected-crates, Windows build/test/release, CodeQL, commit-signatures+sign-off, license headers, markdown-links, coverage). The four hardware-gated E2E lanes (Strix Halo Ubuntu nightly/release, W7900, MI300X) were still queued and the umbrella "E2E tests" job still in progress at the time of this check — not a failure, but not yet confirmed either way from CI alone.

What this covered

Every file in the diff (c6ab82b54f685b01e6a3dacd87403c5fd16c9c4c…5f1694d5af91c20048e41ce559a12f8a5a4f931f, all 16 commits), read completely by 7 independent fan-out workers split by cluster coupling (chat/update/cli_audit; services; config/install/setup; diagnose/model_fit/serve; examine/freeform/bridge; a dedicated full-main.rs-plus-docs pass; and a dedicated impact-analysis/ripple/history pass across the whole repository, not just the 30 changed files), each diffing every relevant commit against its own first parent rather than relying on a single merge-base-relative diff. ROCMAI-759 was retrieved directly by key (not via a link in the PR body) and read in full, including its PR-by-PR plan, risk notes, and out-of-scope section.

Did not run: cargo build/cargo test/cargo clippy locally (no Rust toolchain in this review environment) — relied on the PR's own CI for that, per the note above. No design-review question rose to "fundamental approach problem" — the one real structural question (module count) is recorded above as a tradeoff.

Exposure disclosure: this review's mandated claim-check step (checking for an existing "nowycondro" review before starting) necessarily returned the full body of three prior reviews on this PR, and the mandated head-sha check also returned the full PR description, both before code-reading began. Neither was used to derive any finding above — every finding was independently re-established from git (diffs, git grep, ancestry checks) by the fan-out workers or by direct verification in this driver context, and the fan-out workers themselves were never shown any of that text. The PR description was read again, deliberately, only after the findings list was otherwise settled, to check the change's own claims about itself, per this method's Step 9. The prior reviews' and comments' discussion was not reconciled against this one — this review was produced blind to it, by design.

---

🤖 Generated with Claude Code

[jussielo-amd]

Main moved twice more after the last push, so the branch now merges origin/main in twice (803bcaf and 0e0a3aa) instead of being rebased again, which keeps the commits reviewers already looked at, and the 5f1694d cited above, unchanged. The conflicts were only in places where main edited code this PR moves:

• main's new native-Windows vLLM gate (GatedEngineAction, ensure_engine_allowed_on_platform, ensure_engine_allowed_on_os) and its tests are kept in main.rs where main put them, so serve_cmd.rs and engines_cmd.rs keep importing them from the crate root. Putting them in model_fit.rs would have made serve_cmd and model_fit depend on each other.
• the probe-injectable engine_ruled_out_by_platform_with from the AIM catalog change is applied to the moved engine_ruled_out_by_platform in model_fit.rs, and re-imported in main.rs like its sibling.
• the new AIM engine description is applied to the engine-inventory builder that now lives in bridge_snapshot.rs.
• the examine_report_text.rs module doc and the serve_cmd.rs entry in docs/architecture.md combine main's wording with this PR's.

On the merged head, cargo fmt --check and cargo clippy -p rocm --all-targets -- -D warnings are clean, and cargo test -p rocm passes (1,406 passed, 7 ignored). cargo xtask check-crate-edges, cargo xtask manifest --check and cargo test -p xtask also pass on the first merge. The local pre-push hooks were skipped for these two pushes, so the signature and license checks run only in CI.