[jussielo-amd]

Closes ROCMAI-765, ROCMAI-833, ROCMAI-858, ROCMAI-871, ROCMAI-881 and ROCMAI-885.

Where the other parts of the closed tickets landed. ROCMAI-765 covers the whole assistant cluster, not only the chat/ seams in this diff. The rest is already on main: the three leaf modules that broke the chat and freeform-planner cycle (rocm_argv.rs, prompt_text.rs, tool_policy.rs) came in #888, and the three providers/ modules (tool_schema.rs, local_http.rs, wire.rs) came in #887, which is why this diff touches providers.rs only in a comment. What this PR adds is the remaining four chat/ seams (summaries.rs, mcp_bridge.rs, intent.rs, turn.rs), so with it every seam of the ticket's scope table is in its destination. ROCMAI-858's main work, moving the clap command tree into cli.rs, is already on main (#889, commit 88c344d, merged without naming the key so that the ticket stayed open); #889's own description states the move changes no clap attribute, help text, subcommand order, default or alias. What this diff delivers for ROCMAI-858 is the rest of its scope: the comfyui dispatch fold-in (the same change as ROCMAI-833, in the "From #938" section below, so main.rs ends under the limit) and the two convention texts the ticket names, docs/architecture.md and the new-subsystems bullet in AGENTS.md section 6, which now say that a dispatch function lives in its domain module. ROCMAI-833 is delivered entirely by this diff, by the same comfyui dispatch move.

Summary

apps/rocm had several structural problems that were being fixed one pull request at a time, in files that overlap: chat.rs still bundled the whole chat assistant in one file of roughly 4,500 lines, main.rs still held the one command dispatch function (comfyui) that every sibling command keeps in its own module, the managed-service modules imported each other in production code so that none of the three could be read or moved without the other two, one test-only visibility widening leaked a helper out of its module, and the freeform planner kept its own copy of the four rules the chat tool policy applies to a proposed rocm serve. Reviewing and merging these separately meant the same conflict-prone files (chat.rs, main.rs, docs/architecture.md) were edited in a queue of near-identical rebases, so this PR carries all of them as one reviewable unit: the original split of the deterministic summaries out of chat.rs, four earlier pull requests absorbed as ordinary merges (each keeps its own commits), and the three remaining seams of the chat.rs split.

This PR supersedes #938, #933, #930 and #921, which can be closed once this one merges. Only one change in it alters behaviour (the planner's serve-safety check, from #921); everything else is a pure move or a visibility or import-direction change with no observable effect, and the sections below say which is which.

chat.rs stays a file with a chat/ directory beside it (the layout providers.rs and comfyui.rs already use) instead of becoming chat/mod.rs. That keeps the include_str! depth change to one extra ../, avoids retargeting every doc and path pin that names chat.rs in a rename, and does not make the older open PRs that edit chat.rs conflict on a rename. The child modules are declared from chat.rs (mod summaries;, mod mcp_bridge;, mod intent;, mod turn;).

From #938: the comfyui dispatch function leaves main.rs

rocm comfyui was the one command whose dispatch function (the match over ComfyuiCommand that renders status, logs and the models path and runs install, start and stop, recording an audit event for each) was still defined in main.rs, while its siblings keep theirs in their own module. It moves to comfyui.rs as pub(crate) fn run(command: Option<ComfyuiCommand>), next to the code it calls, as a pure move: the body is unchanged apart from import paths (the comfyui:: prefix on calls into the module is dropped because they are now local, and comfyui.rs imports crate::cli::ComfyuiCommand and crate::cli_audit::record_cli_audit_event directly, so there is no new edge back into main.rs). The one arm of dispatch() becomes Some(Command::Comfyui { command }) => comfyui::run(command). The module-organization text in docs/architecture.md and the new-subsystems bullet in AGENTS.md section 6 said the dispatch function usually stays in main.rs and named comfyui() as the example; both now say what the code does. With the clap tree already in cli.rs, this takes main.rs to 1,202 production lines, under the 1,500-line limit.

Verification claims carried over: fmt, clippy, cargo test -p rocm, an identical leaf-name test list before and after (1,516 tests and 8 ignored), cargo xtask manifest --check, cargo xtask check-crate-edges, lychee, and the leak scan. Its Windows cross-check was not run on its own branch; on this branch it was (see Verification).

From #933: the managed-service modules stop importing each other

services_text.rs and service_launch.rs were a pair (the stop-result text needed the background-helper note from the launcher, and the launcher needed the stop-result renderers), and service_launch.rs and service_control.rs were a second pair (the launcher took run_internal_sandbox_tool from the control module for the attached-serve Ctrl-C, and the control module took the endpoint-key guard and the spawn helpers from the launcher); together they closed a three-module loop. Two commits make the graph one-way: background_helper_already_running, HelperNote, helper_note_for and background_helper_note (with their five tests) move verbatim into a new leaf background_helper.rs, and run_attached_service takes the Ctrl-C stop as a final impl FnOnce(&AppPaths, &str) -> Result<serde_json::Value> argument, which serve_cmd.rs (its only caller, already above both modules) passes as the same run_internal_sandbox_tool call that was made before. The two fixtures that the helper's tests share with the autostart and stop-output tests in service_launch.rs (runtime_state and set_helper_facts) live in the crate-level test_support.rs, so background_helper.rs's mod tests stays private. The first commit is a move; the second is not a move and cannot be one, because run_internal_sandbox_tool calls the stop and restart paths in service_control.rs. The dependency order is now services_text.rs below service_launch.rs below service_control.rs, with services_cmd.rs, runtime_services.rs and serve_cmd.rs above them.

Verification claims carried over: fmt, clippy, tests with an identical leaf-name list (including the five tests that moved to background_helper.rs), manifest and crate-edge checks, and the end-to-end scenarios that traverse the changed paths (service-stop-01 to service-stop-05, automations-11 and the seven service-cleanup scenarios), all green on its own branch. The changed Ctrl-C call site has no unit or scenario coverage before or after because it needs an interactive terminal, and was checked by reading it against the removed call; that still holds.

From #930: a test-only visibility widening is undone

When the rocm model catalog renderer moved into model_catalog_text.rs, model_registry_adapter_availability_note was widened from private to pub(crate) for one reason: a test left behind in examine_report_text.rs (linux_only_engine_runtime_status_is_explicit_on_native_windows) also called it. That test was two checks in one body, the runtime: label (which belongs to examine_report_text.rs) and the adapter-availability note (which belongs to the catalog renderer). The note assertions move, byte for byte and inside the same runtime if cfg!(windows) split, to model_catalog_text.rs as vllm_adapter_availability_note_is_explicit_on_native_windows, and the helper is private again. No assertion was deleted: one test became two, so the test count rises by one.

Verification claims carried over: the moved test was shown to fail when its target is broken (on Linux by making the Allowed arm return a note; the Windows branch by temporarily forcing the condition), and the model_readiness import claim in docs/architecture.md was checked and is unchanged.

From #921: the planner uses the shared serve-safety check (the one behaviour change)

When the provider-assisted planner proposed a rocm serve, it refused it with its own copy of the four rules the chat assistant's tool policy applies to the same command: no --allow-public-bind, no CPU device, a loopback --host if one is given, and managed serving (--managed, never --foreground). A rule added or reworded on one side would silently not reach the other. The four rules now live once in tool_policy::validate_serve_command_safety(args, actor); validate_chat_rocm_command_safety calls it with local assistant and the planner's serve arm calls it with provider planner, so each refusal still names who was refused. The planner still does not import chat. The first commit adds only tests (one table of unsafe and safe serve argvs shared by both suites, pinning the current behaviour on unmodified code); the second makes the planner call the shared check and changes the table to the unified wording.

The one message that changes is the planner's CPU refusal, which now ends ROCm GPU execution is required as the chat one does (it used to end rocm-cli requires ROCm GPU execution); it reaches the user in the planner's failure note. No argv that either surface refused is accepted now, and none that it accepted is refused. Known differences deliberately left alone (the case of the serve word, --allow-public-bind=true and --foreground with --managed, which clap refuses before the planner's check) are pinned by tests rather than changed, and the chat launch_server tool's own partial check is untouched.

Verification claims carried over: eleven new unit tests (six in freeform_planner, five in tool_policy), mutation checks on each of the four rules and on the planner's call into the shared check, and the mock-lane scenarios below.

The three remaining seams of the chat.rs split

The three commits at the end move the last parts of chat.rs into child modules, in this order: the MCP bridge, the prompt-intent heuristics and the turn loop. Each is a pure move: the functions are cut and pasted, tests move with the code they test and keep their leaf names, visibility is widened only where a caller stayed behind, and the docs are updated in the same commit. The symbol lists were re-located by name on the merged tree. The tool-policy, argv and prompt-text seams had already moved in earlier merged pull requests, and the summaries seam is the first commit of this PR.

Seam	Symbols	Destination
MCP bridge and in-process execution	run_internal_mcp_call, internal_mcp_arguments, internal_mcp_tool_success, internal_mcp_tool_result_from_command, run_rocm_capture_for_paths, run_rocm_command_for_paths, run_rocm_read_only_in_process, render_install_sdk_dry_run_for_args, internal_mcp_install_sdk_args, run_chat_read_only_tool, run_chat_path_exists_tool, run_chat_port_status_tool, render_services_tool_result_text, loopback_tcp_port_is_reachable, loopback_host_key, parse_optional_query, parse_optional_lines, mcp_tool_result_text; 20 tests	chat/mcp_bridge.rs: run_internal_mcp_call is pub(crate) and re-exported from chat.rs for main.rs and dash_seam.rs; run_chat_read_only_tool, loopback_host_key and mcp_tool_result_text are pub(super) for the turn loop, and run_chat_path_exists_tool is pub(super) for the command-preview property tests in chat.rs, which also read mcp_tool_result_text; the rest is private
Prompt-intent heuristics	latest_user_chat_message, fallback_rocm_tool_call_for_prompt, fallback_config_tool_call_for_prompt, fallback_engine_list_tool_call, fallback_services_list_tool_call, prompt_can_use_read_only_without_local_assistant, prompt_mentions_serving_engine_or_service, prompt_asks_engine_install_state, prompt_asks_running_or_status, install_sdk_without_prefix_chat_approval, install_sdk_chat_approval_for_prompt, prompt_requests_rocm_install_or_setup, prompt_requests_install_action; 16 tests	chat/intent.rs: ten functions are pub(super) for the turn loop, three private
Turn loop and transcript	ChatPromptResult, ChatToolRunResult, render_chat_prompt_result (test-only), render_chat_prompt_result_with_progress, report_chat_tool_progress, ROCM_CHAT_TOOL_SYSTEM_PROMPT, ROCM_CHAT_TOOL_SKILL, rocm_chat_tool_system_prompt, rocm_chat_tool_system_prompt_for_host, append_chat_tool_results and the rest of the turn-loop, display-label and local_* helpers, mcp_tool_result_is_error; 26 tests and their one helper	chat/turn.rs: render_chat_prompt_result_with_progress and ChatPromptResult are pub(super); rocm_chat_tool_system_prompt_for_host stays pub(crate) and is re-exported from chat.rs for dash.rs; the rest is private

chat.rs keeps read_piped_prompt, piped_prompt_from_input, render_chat_text, plain_provider_auth_status, render_chat_prompt_text, the three clap value parsers, the re-exports, the two piped-prompt tests and the command-preview property tests (which drive the tool-call guards, the planner's guards and the rendered command lines across the policy, the bridge and the turn loop, so they belong to no single seam). Production line counts, counted as every line outside a top-level #[cfg(test)] mod ... { } block (blank lines and comments included): chat.rs 241, chat/turn.rs 873, chat/mcp_bridge.rs 862, chat/intent.rs 565, chat/summaries.rs 631, main.rs 1,202.

Import edges of each child, production code only: summaries reads managed_service_status, providers and rocm_core::placeholder; intent reads prompt_text, rocm_argv, tool_policy, freeform_planner (format_structured_tool_call) and providers; mcp_bridge reads summaries (managed_service_running_state), tool_policy, rocm_argv, freeform_planner, managed_service_status, the read-only renderers it dispatches to and the crate-root bridge builders; turn reads intent, mcp_bridge, summaries, tool_policy, rocm_argv, freeform_planner, engine_runtime_select, managed_service_status, chat_host_facts and providers. Nothing below turn calls back into it, and freeform_planner.rs, tool_policy.rs, rocm_argv.rs and prompt_text.rs contain no chat:: path. mcp_tool_result_text formats an MCP tool result and the turn loop is one of its callers, so it lives in mcp_bridge and turn reads it from there; with that, no module below turn reaches back into it, in tests either.

Other changes the moves required, all in the same commits as the code they follow: the advised_commands.rs path pins follow their strings (rocm command requires at least one argument to chat/mcp_bridge.rs; rocm install folder, rocm installed at, rocm installed and rocm please to chat/intent.rs; rocm tools: {} and rocm command to chat/turn.rs); the assistant skill is included from one directory deeper, so the include_str! path gains a ../ and still resolves to skills/rocm-cli-assistant/SKILL.md; the re-exports of ChatRocmCommandAction and rocm_chat_tool_requested_args become #[cfg(test)] because chat.rs no longer calls them in production code; AGENTS.md and the skill-evals comment in ci.yml said main.rs embeds the assistant skill, and now name chat/turn.rs; and a number of module doc comments and docs/architecture.md entries that named chat.rs as the caller of an item now in a child name the child. The docs/architecture.md entry for chat.rs is rewritten for the file-plus-children layout and each child has its own entry.

Verification

Ran, on this branch (Linux unless stated): after each of the four merges, cargo fmt --all --check, cargo clippy -p rocm --all-targets -- -D warnings on Linux and on x86_64-pc-windows-msvc, cargo test -p rocm --no-fail-fast, cargo xtask manifest --check and cargo xtask check-crate-edges, all green; the unit-test total grows 1,516 (after #938 and #933), 1,517 (after #930, which adds one test), 1,528 (after #921, which adds eleven), with 8 ignored throughout, and cargo test -p rocm passed at each step (1,520 passed and 8 ignored in the rocm binary at the last merge). The MCP bridge commit got the same full set again (1,520 passed and 8 ignored, Windows clippy over all rocm targets, test list identical to its parent). After each of the three seam commits, and for the intent and turn commits in place of the full set: cargo fmt --all --check, cargo clippy -p rocm --bins -- -D warnings, cargo check -p rocm --tests, the targeted tests for the moved module and its neighbours (111 for the intent step, 334 for the turn step, which includes the advised_commands pin tests), cargo xtask manifest --check, cargo xtask check-crate-edges, and the leaf-name test list of cargo test -p rocm --bin rocm -- --list and -- --list --ignored diffed against the commit's parent, which is empty for each of the three seam commits (1,528 tests and 8 ignored). On the final tree: cargo fmt --all --check, cargo clippy -p rocm --bins -- -D warnings and cargo clippy -p rocm --tests -- -D warnings for x86_64-pc-windows-msvc (the C toolchain the Windows dependencies need is stubbed, since clippy never links), lychee offline over docs/architecture.md and AGENTS.md, git merge-tree --write-tree origin/main HEAD (exit 0), a conflict-marker grep and the leak scan against origin/main (the only hits are existing identifiers such as run_internal_mcp_call and internal_mcp_tool_success, and the word private in architecture entries).

After the two review-fix commits at the end (moving mcp_tool_result_text into chat/mcp_bridge.rs, and the background-helper fixtures into test_support.rs): cargo fmt --all --check, cargo clippy --locked -p rocm --all-targets -- -D warnings on Linux, cargo xtask manifest --check, cargo xtask check-crate-edges, lychee offline over docs/architecture.md, the leaf-name list of cargo test -p rocm --bin rocm -- --list diffed against the previous head (empty, 1,528 tests) and the 128 tests matching chat::, background_helper and service_launch, all passing. Neither commit touches platform-gated code, so the Windows cross-check was not repeated for them. After merging the current origin/main into this branch (conflicts in service_launch.rs imports and one docs/architecture.md entry, resolved by keeping both sides), the same fmt, clippy, manifest, crate-edge and lychee checks pass, no test present before the merge is missing from cargo test -p rocm --bin rocm -- --list (1,566 tests now, main having added 38), and the 143 tests matching chat::, background_helper and service_launch pass.

After the latest merge of origin/main (conflicts in service_launch.rs, tool_policy.rs and docs/architecture.md, resolved by keeping both sides): cargo fmt --all --check, cargo clippy --locked -p rocm --all-targets -- -D warnings, cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings, the targeted tests for chat::, background_helper, service_launch and freeform, cargo xtask manifest --check, cargo xtask check-crate-edges and lychee on docs/architecture.md, and no test that existed before the merge is missing from the --list leaf names. The merge brought one new rule into the chat serve check: main now also refuses a --port that rocm serve would refuse. That check stays in the chat policy only and runs after the shared four rules, so the planner is unchanged by it; the one visible difference is that a serve argv that is both missing --managed and carries an invalid port now reports the missing --managed first.

Test plan for the planner scenarios, which a review asked to see fail when the planner's serve arm is reverted: I built a debug rocm and rocmd with --features rocm/e2e-test-hooks and ran cargo test -p e2e-cucumber --test e2e -- -n "chat-1[3-6]" with ROCM_CLI_BINARY and ROCM_CLI_ROCMD_BINARY pointing at them (Linux under WSL, mock-backed, no GPU). All four pass on this branch; with the planner's serve arm refusal removed from validate_provider_planner_tool_call, all four fail, each on the assistant's rocm serve ... plan being shown (the revert was undone by a targeted edit and the four pass again). The review also noted that the "nothing ran" check looked only for an absent execution heading, so the step now also asserts the output's only unindented line is the request plan heading, which fails if anything a run printed appears; with an extra unindented line appended to the plan, that assertion fails all four scenarios, and the heading check alone would not.

Not run: cargo test -p rocm as one full run after the three seam commits (only the targeted filters above, plus the full run after each merge), cargo xtask e2e and the cucumber scenarios (chat-13 to chat-16 ran green on this combined tree as described above; chat-10 to chat-12 and the service scenarios named under #933 ran green on those pull requests' own branches, not on this combined tree, and the comfyui scenarios were not run locally even there), cargo test --workspace, scripts/smoke_local.py and cargo xtask coverage. CI carries those. No test was added or edited by the three seam commits, so there is no new test to show failing; the added tests are those of #930 and #921, whose mutation checks are described above.

User-visible behaviour

Only #921 changes what a user can observe: the planner's refusal of a CPU-device serve now ends ROCm GPU execution is required. Scenarios: chat-13 (chat-planning-assistant-cpu-serve-refused) is the one that depends on the change, and chat-14 (public bind), chat-15 (non-local host) and chat-16 (unmanaged serve) pass before and after and hold the shared rules at the binary level; all four are in tests/e2e-cucumber/features/chat.feature on the mock lane, and each asserts the plan the CLI printed, that no execution section appears, that the plan is the only output, and the exact refusal note. The existing planner scenarios chat-10 to chat-12 never reach the serve arm. Everything else is internal: the comfyui dispatch move, the service-module import changes, the visibility and test move, and the three chat seams change no command output, exit code, file or engine selection, so no new scenario applies, and the existing chat, comfyui and service scenarios exercise the same paths unmodified.

Notes for reviewers

Reading order, oldest first. The two commits of the first group came with this branch: refactor(rocm): move the chat assistant's deterministic tool summaries into chat/summaries.rs (pure move) and fix(rocm): gate the chat summary re-export for comfyui's tests to unix (the test-only re-export is an unused import on Windows). Then #938: refactor(rocm): move the comfyui dispatch function out of main.rs and its merge. Then #933: move the background-helper note out of service_launch.rs into background_helper.rs (move), hand run_attached_service the Ctrl-C stop instead of importing service_control (the one signature change), a merge of origin/main into that branch and the merge into this one. Then #930: make the model registry adapter note helper private again, its own merge of origin/main, and the merge here, whose only conflict was two adjacent entries in docs/architecture.md (kept both sides). Then #921: pin the serve safety rules of the chat tool policy and the freeform planner (tests only), make the freeform planner call the shared serve safety check (the behaviour change, with the scenarios and docs), its merge of origin/main and the merge here. Last, the three seam commits: move the chat assistant's MCP bridge into chat/mcp_bridge.rs, move the chat assistant's prompt-intent heuristics into chat/intent.rs and move the chat turn loop and transcript into chat/turn.rs. Review-fix commits follow. The first two are pure moves: move mcp_tool_result_text into chat/mcp_bridge.rs so the bridge has no edge back to the turn loop and move the background-helper test fixtures to test_support and make background_helper's tests module private again. The third, test(e2e): assert the refused planner plan is the only output, not just that no execution heading appears, tightens one e2e step and changes no product code; a merge of origin/main sits before it. The last commit, drop a needless borrow in the attached Ctrl-C stop call, is the one-character clippy fix that the latest merge made necessary. For the seam commits, git diff --color-moved shows the bodies as moved; what is not a move is the new module's header and imports, the pub(super) keywords, the import blocks left in chat.rs and the docs.

The merges of #938, #933 and #921 had no conflicts; the one in #930 is described above. Docs touched by the open pull requests that this one does not supersede (docs/architecture.md, docs/testing.md, docs/llm-tool-use.md and chat.feature) can still conflict mechanically, as can any open pull request that edits chat.rs: after this change chat.rs is a small file, so such a rebase has to re-apply the edit in the child module that now holds the code.

[Copilot]

🟢 Approval recommended

There are no active review issues blocking approval of the tool-output summary module extraction.

No open issues 🎉

What changed in this PR

Extracts the chat assistant’s deterministic tool-output summaries into a leaf module, reducing chat.rs responsibilities without changing user-visible behavior.

Changes:

• Moves summary builders, parsers, row types, and nine existing tests into chat/summaries.rs.
• Preserves turn-loop integration and the test-only ComfyUI summary access path.
• Updates architecture documentation and module comments to describe the split.

File	Description
docs/​architecture.md	Documents the new summaries module and revised ownership.
apps/​rocm/​src/​managed_service_status.rs	Updates a comment naming the engine-label helper’s consumer.
apps/​rocm/​src/​chat/​summaries.rs	Houses the extracted summaries, parsers, and tests.
apps/​rocm/​src/​chat.rs	Replaces moved code with imports and a test-only re-export.

_{Review effort: Balanced}

[Copilot]

🟢 Approval recommended

The review reports no active issues with the chat and service refactor or shared serve-safety validation changes.

No open issues 🎉

_{Review effort: Balanced}

[Copilot]

🟢 Approval recommended

No active review issues block the chat-module reorganization and shared serve-safety validation.

No open issues 🎉

_{Review effort: Balanced}

[juhovainio]

Requesting changes on one item: the ticket-closing claim. The code itself looks clean.

Blocking

• The PR body says Closes ROCMAI-765, but that ticket's scope also includes the providers.rs split (providers/tool_schema.rs, providers/local_http.rs, providers/wire.rs). This diff only touches a comment in providers.rs, and the body doesn't mention those seams. Merging would auto-close 765 with that work undelivered. Either narrow it to "Part of ROCMAI-765" (chat seams only) and keep the ticket open, or say where the providers seams landed.

Non-blocking

• The mcp_bridge tests import mcp_tool_result_text from turn, so the "leaf below turn" claim holds only for non-test code (inline comment).
• pub(crate) mod tests in background_helper.rs widens visibility for test fixtures (inline comment).
• ROCMAI-858 is listed as closed, but the clap-tree move and the byte-identical --help/completions proof aren't in this diff. Please confirm the earlier PR covered them.
• Bundling six tickets departs from the "one PR per ticket" note on 765/881/885. The body explains why, so this is a process call for the owner.

Checked

• The moves look behavior-identical: no function dropped, cfg gates unchanged, no tests removed, and validate_serve_command_safety keeps the old rule order. Docs and AGENTS.md updates are consistent.
• CI is green except Sphinx docs build (-W), which hit a GitHub API 403 rate limit and the job timeout. It needs a rerun.
• Not run: I did not build or re-run the mutation checks.

[juhovainio]

Test-only edge back to turn (mcp_tool_result_text). Production stays one-way, but the "leaf below turn" statement in the docs is only true for non-test code. Consider placing this helper in summaries.rs or mcp_bridge.rs so the edge and the pub(super) go away.

[jussielo-amd]

Fixed in 4ee1d10. mcp_tool_result_text only reads the text out of an MCP tool result, which is the shape this module builds, and the turn loop calls it too, so it now lives in chat/mcp_bridge.rs and chat/turn.rs imports it from there (an edge that already existed). The body is unchanged and its two tests moved with it under the same names; the test-only import from turn is gone, so the "nothing below the turn loop calls back" statement now holds for tests as well. The function stays pub(super) because the turn loop calls it in production code. docs/architecture.md and the module doc comments are updated in the same commit, and the leaf-name list of cargo test -p rocm --bin rocm -- --list is identical before and after.

[juhovainio]

pub(crate) mod tests widens visibility so service_launch.rs's tests can reuse runtime_state and set_helper_facts. It is test-gated, but consider test_support.rs for shared fixtures, consistent with ROCMAI-885's aim of not widening visibility for tests.

[jussielo-amd]

Fixed in fd18665. runtime_state and set_helper_facts moved, bodies unchanged, to the crate-level test_support.rs, both test modules import them from there, and background_helper's mod tests is private again. No test was renamed (the leaf-name list is identical before and after) and docs/architecture.md now says where the fixtures live.

[jussielo-amd]

On the blocking point (the ticket-closing claim): ROCMAI-765's scope also includes the providers.rs split, and it is already delivered. The three providers/ modules (tool_schema.rs, local_http.rs, wire.rs) landed in #887, and the three leaf modules that broke the chat and freeform-planner cycle (rocm_argv.rs, prompt_text.rs, tool_policy.rs) landed in #888, both on main. That is why this diff only touches a comment in providers.rs. This PR adds the remaining four chat/ seams, so with it every seam in the ticket's scope table is in its destination and closing the ticket is accurate. The PR description now says where each part landed.

On ROCMAI-858: the clap-tree move landed in #889 (commit 88c344d). It was merged without naming the key so the ticket stayed open; nothing for it is in this diff and this PR only closes it. #889's description states that the move changes no clap attribute, help text, subcommand order, default or alias.

On bundling six tickets: that is the author's decision to cut review overhead, recorded in the description. If you would rather split, say so and I will not merge it as is.

On the Sphinx docs job: it hit the GitHub API rate limit and the job timeout, and will be re-run. The two inline suggestions are being handled separately.

Fixed: the blocking ticket-closing point is answered on the PR (where each part of 765 and 858 landed), and the two non-blocking inline notes are fixed in 4ee1d10 and fd18665.

[Copilot]

🔵 Needs a closer look

The refactor spans approval/MCP execution and service lifecycle boundaries, requiring maintainer or CI validation of combined-tree Linux/Windows behavior and the untested interactive Ctrl-C shutdown path.

No open issues 🎉

_{Review effort: Balanced}

[juhovainio]

I found nothing blocking in this PR.

I compared the moved code function by function against the old chat.rs: everything lands in exactly one of summaries, mcp_bridge, intent or turn (or stays in chat.rs), with only visibility, the include_str! path depth, test wrappers and re-export gating differing. The comfyui::run body and the background_helper.rs bodies are identical to what they replaced, and all new files are under the 1,500-line limit. CI is green apart from the consolidated report that was still pending when I checked.

Small non-blocking notes:

1. The first line closes six ROCMAI keys, but ROCMAI-765's automation warning says its closing PR names no other ROCMAI key, so please confirm the multi-key close is fine. The ROCMAI-858 wording says nothing for it is in this diff, yet the comfyui dispatch move is.
2. This bundles five logical changes, one of them behavioral (the planner's CPU-serve refusal wording). It is disclosed and covered by ROCMAI-881, but it makes the "pure move" framing of ROCMAI-765 not strictly true.
3. One inline comment on the new e2e step's negative assertion.

The PR currently shows merge conflicts with main, so it will need a rebase before it can land.

[juhovainio]

This negative check passes if the plan's execution heading ever changes shape. Please show chat-13..16 fail with the planner's serve arm reverted (and say so in the PR body), or assert a positive marker of "nothing ran".

[jussielo-amd]

Fixed in 0925fb0, and I ran the scenarios to show it. The step now also asserts a positive marker of nothing having run: the plan renderer indents everything under its one request plan heading, so the output's only unindented line must be that heading, and anything a run printed of its own would add another. On a debug build with the e2e hooks (cargo test -p e2e-cucumber --test e2e -- -n "chat-1[3-6]", Linux under WSL, mock-backed) chat-13 to chat-16 pass; with the planner's serve arm refusal in validate_provider_planner_tool_call removed all four fail, each on the assistant's rocm serve ... plan being shown; and with an extra unindented line appended to the rendered plan, the new assertion fails all four, which the old absent-heading check would not have caught. Both mutations were undone by targeted edits and the four pass again. The PR body says so.

[jussielo-amd]

Thank you for the review and the function-by-function comparison. On the notes: (a) the first line does close six keys. The epic's rule was changed on 2026-11-06 to one PR per ticket with a Closes ROCMAI-N. line naming every ticket the PR fully delivers, and this is the combined PR I chose to cut review overhead on files that overlap; the ROCMAI-765 ticket text still carries the older wording that its closing PR names no other key. (b) You are right about ROCMAI-858 and the body was wrong: the comfyui dispatch move in this diff is exactly the fold-in that ROCMAI-858's scope table names (and the whole of ROCMAI-833), and the updated convention texts in docs/architecture.md and AGENTS.md section 6 are the ones it asks for. Only the clap tree move itself was already on main in #889. The body now says what in this diff delivers each of the two keys. (c) Agreed, and it is disclosed: the planner's CPU-serve refusal wording (ROCMAI-881) is the one behavioural change, so the "pure move" framing applies to the move commits only, which the body says by naming which commit is which.

[Copilot]

🟢 Approval recommended

No active review issues remain for the assistant-module refactor and shared serve-safety validation.

No open issues 🎉

_{Review effort: Balanced}