[jussielo-amd]

Phase 6d of the main.rs modularization (epic ROCMAI-27). Third and final PR for this cluster (follows #665, #667).

Finishes extracting the chat/MCP-bridge cluster from main.rs into chat.rs: moves the cluster's last two tests, relocates render_services_tool_result_text, and completes the pub(crate) visibility audit across both functions and types. No behavior change.

Tests

Moves the cluster's last two tests out of main.rs's shared mod tests block into chat.rs's own mod tests (the rest of the cluster's tests moved in #665/#667):

• services_tool_result_text_includes_running_interpretation
• the chat-approval-title half of runtimes_help_uses_the_runtime_noun_throughout, renamed runtimes_activate_approval_modal_uses_the_runtime_noun since it no longer shares a test function with the clap --help check. That check asserts clap structure only, so it stays in main.rs, unmodified, beside its sibling help tests (out_of_scope_commands_are_marked_preview_in_help, the_preview_marker_is_explained_and_not_contradicted).

Test count is unchanged: 1266 (1260 passed, 6 ignored) before and after this PR, same names except for the one rename/split above.

render_services_tool_result_text

Moved from main.rs into chat.rs, next to managed_service_running_state (its only caller) and the rest of the cluster's deterministic tool-output summarization. This was the one piece of that domain still in main.rs, and the reason chat::managed_service_running_state had to stay pub(crate).

Visibility audit

Re-derived chat.rs's zero-external-caller set by grepping every pub(crate) item against the five files that call into chat:: (main.rs, dash.rs, dash_seam.rs, comfyui.rs, runtime_install_finalize.rs), then compile-checking the result. Covers both functions and types:

• 42 functions and 7 types tightened to module-private.
• 20 functions and 1 type (ChatToolApprovalRequest, reached by dash_seam.rs as chat_tool_approval_request's return type) remain pub(crate), each with a confirmed external caller, production or test-only.

docs/architecture.md's chat.rs entry and the module's own doc comment now describe this completed state, including a corrected function count (123 functions, not the ~61 an earlier revision said).

Verification: cargo clippy -p rocm --all-targets -- -D warnings and cargo test -p rocm --bin rocm pass locally (1260 passed, 6 ignored, 0 failed — one PID-race flake in comfyui::tests::status_reports_stopped_when_saved_comfyui_pid_is_gone, unrelated to this diff: confirmed by running it in isolation and by a clean full-suite rerun). cargo xtask manifest --check, tests/e2e-cucumber, and the Windows job are CI-only and weren't run here.

Compliance with epic ROCMAI-27's rules

• One PR per cluster, largest clusters split 2-3 ways: this cluster (main.rs chat/MCP-bridge) is one of the two the epic names explicitly (alongside rocm-core's host_gpu.rs) as warranting a 2-3 PR split; this is PR 3 of 3 (ROCMAI-88: extract chat/MCP-bridge types and leaf helpers (1/3) #665, ROCMAI-88: extract chat/MCP-bridge function bodies (2/3) #667, this PR), and the final one.
• Never mix a mechanical move with a behavior change: no behavior change — test count is identical before and after, and every moved or relocated item is unmodified apart from its containing module (for the split test, apart from which half of its assertions it keeps).
• Tests colocate and move in the same PR as the code they test: closes this out for the cluster — no cluster test is left in main.rs's shared block, and the one test that spanned two modules (clap help text in main.rs, chat approval-title logic in chat.rs) is split so each half sits with the code it actually tests.
• Docs update in the same PR as the code: docs/architecture.md's chat.rs entry is rewritten to its final, settled wording now that the whole cluster (types, bodies, call-site qualification, and tests) has moved and the visibility audit is complete.
• Re-locate cluster boundaries by function/module name before implementing, not stored line numbers: the cluster's current boundaries were re-verified by name against the current main.rs/chat.rs, not against any earlier PR's line ranges.
• Verification per PR: see Verification above.

🤖 Generated with Claude Code

[jussielo-amd]

Conflict resolution for this PR, as a patch to apply on top of current main. The agent sandbox cannot push to jussielo-amd/rocm-cli (egress proxy returns 403 on git push), so the rebased branch could not be updated directly.

What happened. #665 and #667 have both merged. #667's final version already moved 78 of this PR's 79 cluster tests into chat.rs, so the old 3/3 commit conflicted in main.rs, chat.rs and docs/architecture.md. The 3/3 commit was rebased onto main (1f19198) taking main's side of all three files, then the remaining delta re-applied:

• moved runtimes_help_uses_the_runtime_noun_throughout, the one cluster test still in main.rs, into chat.rs's mod tests (dropped the now-redundant chat:: qualifiers, added use clap::CommandFactory)
• updated docs/architecture.md and the chat.rs module doc to say the tests moved across parts 2 and 3

Not yet done: cargo build / clippy / cargo test -p rocm (no Rust toolchain in the sandbox), and the two extra scope items in the PR description (render_services_tool_result_text move, pub(crate) audit), which the existing commit never covered.

To pick up (patch verified: applies cleanly to main 1f19198 and yields the same tree as the sandbox's resolved commit). Paste the patch below into 668.patch, then:

git fetch upstream
git checkout rocmai-88-chat-mcp-bridge-3
git reset --hard upstream/main      # drops the stale commits; #665/#667 are already in main
git am < 668.patch
git push --force-with-lease origin rocmai-88-chat-mcp-bridge-3

668.patch (git format-patch, 3 files, +44/-44)

From a3364a4f2f957c3795265e500a77fc1d026ffaa6 Mon Sep 17 00:00:00 2001
From: Jussi Elo <jussi.elo@amd.com>
Date: Wed, 14 Oct 2026 06:42:14 +0000
Subject: [PATCH] ROCMAI-88: partition cluster tests out of main.rs, update
 docs (3/3)

Phase 6d of the main.rs modularization (epic ROCMAI-27), third and
final PR for this cluster (follows #665, #667).

Rebased onto main after #665 and #667 merged. #667's final version
already carried most of the cluster's regression tests into chat.rs's
mod tests; what remains here is moving the last cluster test still in
main.rs's shared block, runtimes_help_uses_the_runtime_noun_throughout,
into chat.rs, and settling the docs/architecture.md chat.rs entry and
chat.rs's module doc comment to say the tests moved across parts 2
and 3.

Tests that only incidentally reference chat:: stay in main.rs
(command_chat_honors_chat_mock_and_keeps_prompt_passthrough,
command_chat_reads_prompt_from_piped_stdin,
serve_negative_sampling_space_form_reaches_range_validator).

No behavior change; the moved test is unmodified apart from dropping
the now-redundant chat:: qualifiers.

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
---
 apps/rocm/src/chat.rs | 44 +++++++++++++++++++++++++++++++++++++++++--
 apps/rocm/src/main.rs | 40 ---------------------------------------
 docs/architecture.md  |  4 ++--
 3 files changed, 44 insertions(+), 44 deletions(-)

diff --git a/apps/rocm/src/chat.rs b/apps/rocm/src/chat.rs
index fb496fe..d73ecc4 100644
--- a/apps/rocm/src/chat.rs
+++ b/apps/rocm/src/chat.rs
@@ -18,7 +18,7 @@
 //! three PRs given the cluster's size: this module's contents are now parts
 //! 1 and 2's combined slice (types, leaf helpers, and the orchestration
 //! bodies above), plus the cluster's regression tests, moved out of
-//! `main.rs`'s shared `mod tests` block in part 2. Auditing which of this
+//! `main.rs`'s shared `mod tests` block in parts 2 and 3. Auditing which of this
 //! module's `pub(crate)` items have no caller outside it and could be
 //! module-private is the third PR's job.
 
@@ -4119,7 +4119,7 @@ mod tests {
     use crate::runtime_install_finalize::tests::consents_for_install_sdk_argv;
     use crate::tests::test_paths;
     use crate::{Cli, Command, InstallTarget};
-    use clap::Parser;
+    use clap::{CommandFactory, Parser};
     use rocm_core::ManagedServiceRecord;
     use std::fs;
     use std::net::TcpListener;
@@ -4132,6 +4132,46 @@ mod tests {
         }
     }
 
+    #[test]
+    fn runtimes_help_uses_the_runtime_noun_throughout() {
+        // `comfyui install`'s selection errors steer the user to `rocm runtimes`
+        // and say "ROCm runtime". The help for the command they land on must use
+        // the same noun — including its own about line, which `rocm runtimes
+        // --help` prints above the subcommand list and which the rename missed
+        // while every subcommand below it already said "runtime".
+        let help = Cli::command()
+            .find_subcommand_mut("runtimes")
+            .expect("runtimes subcommand")
+            .render_long_help()
+            .to_string();
+        assert!(
+            help.contains("ROCm runtimes"),
+            "`rocm runtimes --help` should describe itself with the `runtime` noun:\n{help}"
+        );
+        assert!(
+            !help.contains("ROCm install"),
+            "`rocm runtimes --help` must not reintroduce the `ROCm install` noun:\n{help}"
+        );
+
+        // The help is not the only `runtimes` string a user reads: running a
+        // mutating `rocm runtimes …` from chat raises an approval modal whose
+        // title is written here, not by clap, so the help assertions above
+        // cannot reach it. It said "Change ROCm install" until this rename.
+        let action = chat_rocm_command_action_from_args(vec![
+            "runtimes".to_owned(),
+            "activate".to_owned(),
+            "some-runtime-key".to_owned(),
+        ])
+        .expect("a mutating runtimes command classifies");
+        let ChatRocmCommandAction::Approval { pending_title, .. } = action else {
+            panic!("`rocm runtimes activate` must require approval, got {action:?}");
+        };
+        assert!(
+            pending_title.contains("runtime") && !pending_title.contains("install"),
+            "the `runtimes` approval modal must use the `runtime` noun, got {pending_title:?}"
+        );
+    }
+
     #[test]
     fn command_capture_text_prefers_stdout_only_when_stderr_is_blank() {
         let output = CommandCapture {
diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs
index 37d4c6e..a5edfc6 100644
--- a/apps/rocm/src/main.rs
+++ b/apps/rocm/src/main.rs
@@ -12269,46 +12269,6 @@ pub(crate) mod tests {
         }
     }
 
-    #[test]
-    fn runtimes_help_uses_the_runtime_noun_throughout() {
-        // `comfyui install`'s selection errors steer the user to `rocm runtimes`
-        // and say "ROCm runtime". The help for the command they land on must use
-        // the same noun — including its own about line, which `rocm runtimes
-        // --help` prints above the subcommand list and which the rename missed
-        // while every subcommand below it already said "runtime".
-        let help = Cli::command()
-            .find_subcommand_mut("runtimes")
-            .expect("runtimes subcommand")
-            .render_long_help()
-            .to_string();
-        assert!(
-            help.contains("ROCm runtimes"),
-            "`rocm runtimes --help` should describe itself with the `runtime` noun:\n{help}"
-        );
-        assert!(
-            !help.contains("ROCm install"),
-            "`rocm runtimes --help` must not reintroduce the `ROCm install` noun:\n{help}"
-        );
-
-        // The help is not the only `runtimes` string a user reads: running a
-        // mutating `rocm runtimes …` from chat raises an approval modal whose
-        // title is written here, not by clap, so the help assertions above
-        // cannot reach it. It said "Change ROCm install" until this rename.
-        let action = chat::chat_rocm_command_action_from_args(vec![
-            "runtimes".to_owned(),
-            "activate".to_owned(),
-            "some-runtime-key".to_owned(),
-        ])
-        .expect("a mutating runtimes command classifies");
-        let chat::ChatRocmCommandAction::Approval { pending_title, .. } = action else {
-            panic!("`rocm runtimes activate` must require approval, got {action:?}");
-        };
-        assert!(
-            pending_title.contains("runtime") && !pending_title.contains("install"),
-            "the `runtimes` approval modal must use the `runtime` noun, got {pending_title:?}"
-        );
-    }
-
     #[test]
     fn out_of_scope_commands_are_marked_preview_in_help() {
         let help = Cli::command().render_long_help().to_string();
diff --git a/docs/architecture.md b/docs/architecture.md
index e2976b5..b7eb6f5 100644
--- a/docs/architecture.md
+++ b/docs/architecture.md
@@ -27,11 +27,11 @@ Each per-crate module inventory lists one file per line. Flat inventories (every
 
 ### `apps/rocm` — main CLI binary
 
-[`main.rs`](../apps/rocm/src/main.rs) itself is **partially modularized** — Phase 6a (ROCMAI-82) extracted the driver-install/engines/serve clusters, Phase 6b (ROCMAI-91) extracted the managed-process-launch cluster, Phase 6c (ROCMAI-89) extracted the `runtimes` command/uninstall/rollback/SDK-install-finalization clusters, Phase 6d (ROCMAI-88, in progress) is extracting the chat/assistant + MCP tool-bridge cluster below across three PRs — the first two (types/leaf helpers, then the orchestration bodies and crate-wide `chat::` call-site qualification) have landed; the rest of the split is planned across several more PRs, one cluster at a time (see EAI-7768).
+[`main.rs`](../apps/rocm/src/main.rs) itself is **partially modularized** — Phase 6a (ROCMAI-82) extracted the driver-install/engines/serve clusters, Phase 6b (ROCMAI-91) extracted the managed-process-launch cluster, Phase 6c (ROCMAI-89) extracted the `runtimes` command/uninstall/rollback/SDK-install-finalization clusters, Phase 6d (ROCMAI-88) extracted the chat/assistant + MCP tool-bridge cluster below across three PRs (types/leaf helpers, then the orchestration bodies and crate-wide `chat::` call-site qualification, then the remaining test partitioning); the rest of the split is planned across several more PRs, one cluster at a time (see EAI-7768).
 
 - [`automations.rs`](../apps/rocm/src/automations.rs) — mechanical relocation, dispatch-adjacent, no owned types.
 - [`bootstrap.rs`](../apps/rocm/src/bootstrap.rs) — a further-extracted variant of full domain extraction: it owns its clap command enum (`BootstrapCommand`) and dispatch function too, rather than leaving them in [`main.rs`](../apps/rocm/src/main.rs).
-- [`chat.rs`](../apps/rocm/src/chat.rs) — full domain extraction (Phase 6d, ROCMAI-88), mirrors [`providers.rs`](../apps/rocm/src/providers.rs). Owns the chat/MCP-bridge cluster's `pub(crate)` types (`ChatRocmCommandAction`, `CommandCapture`, `DeterministicModelRecipe`, `DeterministicServiceRow`, `DeterministicEngineRow`), its validation/approval/execution logic (`validate_chat_tool_call` and the `validate_chat_*_tool_call` family), deterministic tool-output summarization, and the MCP tool-bridge entry points (`run_internal_mcp_call` and its helpers); a handful of internals stay private. Every call site in [`main.rs`](../apps/rocm/src/main.rs), [`dash.rs`](../apps/rocm/src/dash.rs), [`dash_seam.rs`](../apps/rocm/src/dash_seam.rs), [`comfyui.rs`](../apps/rocm/src/comfyui.rs), and [`runtime_install_finalize.rs`](../apps/rocm/src/runtime_install_finalize.rs) goes through the `chat::` module boundary. As a child module of the crate root (`mod chat;` in `main.rs`), Rust's module-privacy rule already makes most of `main.rs`'s private helpers visible to `chat.rs` without any visibility change, when `chat.rs` calls into `main.rs`. The other direction needs real widening: `chat.rs`'s own functions — including `managed_service_running_state`, called from `main.rs` — are reached from `main.rs`, `dash.rs`, `dash_seam.rs`, `comfyui.rs`, and `runtime_install_finalize.rs`, so most of `chat.rs`'s declared items are `pub(crate)`; a visibility audit (tightening any that turn out to have no caller outside `chat.rs` to module-private) is left to the third PR. The cluster's regression tests now live in `chat.rs`'s own `mod tests`, moved out of `main.rs`'s shared `mod tests` block in this second PR; a few `main.rs` tests that reference `chat::` only incidentally (structural checks on `main.rs`'s own dispatch arms, and a `runtimes --help` wording check) stay in `main.rs`.
+- [`chat.rs`](../apps/rocm/src/chat.rs) — full domain extraction (Phase 6d, ROCMAI-88), mirrors [`providers.rs`](../apps/rocm/src/providers.rs). Owns the chat/MCP-bridge cluster's `pub(crate)` types (`ChatRocmCommandAction`, `CommandCapture`, `DeterministicModelRecipe`, `DeterministicServiceRow`, `DeterministicEngineRow`), its validation/approval/execution logic (`validate_chat_tool_call` and the `validate_chat_*_tool_call` family), deterministic tool-output summarization, and the MCP tool-bridge entry points (`run_internal_mcp_call` and its helpers); a handful of internals stay private. Every call site in [`main.rs`](../apps/rocm/src/main.rs), [`dash.rs`](../apps/rocm/src/dash.rs), [`dash_seam.rs`](../apps/rocm/src/dash_seam.rs), [`comfyui.rs`](../apps/rocm/src/comfyui.rs), and [`runtime_install_finalize.rs`](../apps/rocm/src/runtime_install_finalize.rs) goes through the `chat::` module boundary. As a child module of the crate root (`mod chat;` in `main.rs`), Rust's module-privacy rule already makes most of `main.rs`'s private helpers visible to `chat.rs` without any visibility change, when `chat.rs` calls into `main.rs`. The other direction needs real widening: `chat.rs`'s own functions — including `managed_service_running_state`, called from `main.rs` — are reached from `main.rs`, `dash.rs`, `dash_seam.rs`, `comfyui.rs`, and `runtime_install_finalize.rs`, so most of `chat.rs`'s declared items are `pub(crate)`; a visibility audit (tightening any that turn out to have no caller outside `chat.rs` to module-private) is left to the third PR. The cluster's regression tests now live in `chat.rs`'s own `mod tests`, moved out of `main.rs`'s shared `mod tests` block across the second and third PRs; a few `main.rs` tests that reference `chat::` only incidentally (structural checks on `main.rs`'s own dispatch arms, and a clap value-parser wiring test) stay in `main.rs`.
 - [`chat_host_facts.rs`](../apps/rocm/src/chat_host_facts.rs) — full domain extraction.
 - [`cli_progress.rs`](../apps/rocm/src/cli_progress.rs) — shared CLI-output component (`Spinner`, `AnimatedSpinner`).
 - [`cli_report.rs`](../apps/rocm/src/cli_report.rs) — shared CLI-output component (`ActionReport`).
-- 
2.39.5

---

🤖 by agent-hub on AMD AgentHub

[Copilot]

🔵 Needs a closer look

The broad visibility changes need confirmed compilation and CI results, which were not independently verified here.

2 open issues

• Complete visibility audit and move service result renderer to chat.rs
• Distinguish test-only chat references from production dependencies

1 resolved since last review

• Restore deleted negative sampling space-separated regression test

_{Review effort: Balanced}

[juhovainio]

Reviewed the full diff (chat.rs visibility audit, main.rs test partitioning, docs/architecture.md update). This is the third PR in the ROCMAI-88 series and matches the ticket's acceptance criteria: test count is unchanged (tests moved, not rewritten), docs are updated in the same PR, and there's no behavior change — just tightening pub(crate) down to module-private where nothing outside the module calls a given function.

Two small non-blocking notes inline, plus one that isn't tied to a diff line:

• The PR description's narrative about test counts is based on an earlier snapshot of the series and reads a little stale next to the final numbers in this PR — worth a quick pass before merging so reviewers skimming the description aren't misled, but not a reason to hold this up.

CI: all green except E2E tests (Strix Halo, Windows, current), which is failing on an infra issue (download/runner flake on that lane) unrelated to this diff — consistent with flakiness seen on other unrelated PRs on this runner recently.

No blocking issues. Approving.

[juhovainio]

Minor: this line says every remaining pub(crate) function in chat.rs has "a confirmed external caller," but chat_read_only_tool_status_label is still pub(crate) after this pass and isn't actually touched by this diff's caller-audit hunks — worth double-checking it still has one, or softening the claim slightly so it doesn't overstate the audit's completeness for every single function.

[juhovainio]

Non-blocking: these assertions dropped their chat:: qualifier (body.contains("render_chat_prompt_text(") instead of body.contains("chat::render_chat_prompt_text(")), here and at lines 20796, 20814, 20822, and 20824 below. Functionally fine post-refactor, but it slightly loosens what the test is actually pinning down — it'd now pass even if the handler called a same-named function from an unrelated module.

[rominf]

🔴 Automated review · pr-review-watcher · f810b51

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.
This PR finishes ROCMAI-88 (Phase 6d), the last of the three PRs for this cluster. It adds the chat.rs visibility audit, moves the last cluster function and its two tests out of main.rs, and settles the docs. No issue is closed by it.

First automated round on this PR: every finding below is new.

Blocking

• The new module doc says "every item still pub(crate) has a confirmed caller outside this module". That is false for 7 of the 8 pub(crate) types — apps/rocm/src/chat.rs:21-22 (claim), apps/rocm/src/chat.rs:56, 66, 252, 327, 396, 613, 618 (types)
  • The 7 types are ChatRocmCommandAction, CommandCapture, DeterministicModelRecipe, DeterministicServiceRow, DeterministicEngineRow, ChatPromptResult and ChatToolRunResult. A whole-word grep finds none of them in any file under apps/rocm/src other than chat.rs.
  • ChatRocmCommandAction lost its last outside user in this PR, when runtimes_help_uses_the_runtime_noun_throughout moved into chat.rs.
  • The only type that must stay pub(crate) is ChatToolApprovalRequest: dash_seam.rs reaches it as the return type of chat_tool_approval_request.
  • Lines 5-7 of the same doc still list the five types as part of the cluster's pub(crate) surface.
  • The code governs, because the audit can be finished cleanly. I removed pub(crate) from all seven and cargo clippy -p rocm --all-targets -- -D warnings stayed clean.
  • Confidence 100 · mechanical · Fix: make the 7 types module-private, which finishes the audit the doc describes. Otherwise narrow the doc to "every function".
• docs/architecture.md says "21 of chat.rs's ~61 functions". chat.rs has 123 non-test functions — docs/architecture.md:34
  • Counting top-level fn before mod tests at HEAD gives 123. Of those, 21 are pub(crate), which is right.
  • The pub(crate) count before the audit was 63, which is probably where "~61" came from.
  • The code governs: the count is a grep anyone can re-run.
  • Confidence 100 · mechanical · Fix: say "of 123", or drop the denominator.
• The PR description describes a different diff from the one under review — PR #668 description, and commit c296252's message, which is the same text
  • What it claims:
    • "Of 354 tests in the shared block, 82 … moved".
    • "Updates docs/architecture.md with a chat.rs module-map entry".
    • "Test count unchanged: 1147 passed".
    • "20 remain pub(crate)".
    • Kept-draft and rebase-pending notes.
  • What the diff actually does:
    • Moves 2 tests: runtimes_help_uses_the_runtime_noun_throughout and services_tool_result_text_includes_running_interpretation. The other cluster tests were already in chat.rs at prw-base, with 109 chat::tests entries there.
    • Rewrites an existing doc entry rather than adding one.
    • Has 1265 tests both before and after.
    • Leaves 21 functions pub(crate).
  • What it leaves out: the move of runtimes_help_uses_the_runtime_noun_throughout into chat.rs. The base docs said that test stays in main.rs, and moving it is what allowed chat_rocm_command_action_from_args to go private.
  • rocm-cli squash-merges, so this text becomes the commit message on main.
  • Confidence 100 · logic · Fix: rewrite the description, and with it the squash message, to describe this diff: the 42-function audit, the relocation of render_services_tool_result_text, the two test moves and the doc rewrite.

Non-blocking

• The comment on run_internal_mcp_call names only dash_seam.rs as its reason for being pub(crate) — apps/rocm/src/chat.rs:3286
  • main.rs:2283 also calls it in production, and the updated architecture doc cites exactly that call.
  • This code is older than the PR. It is in scope because it is the only per-item visibility rationale in the file this audit settles.
  • Confidence 85 · mechanical · Fix: name both callers, or drop the comment.

Decisions for the author

• Where runtimes_help_uses_the_runtime_noun_throughout lives — tradeoff
  • Moving it into chat.rs: lets chat_rocm_command_action_from_args go private. The cost is that a clap --help wording check now sits away from its sibling help tests in main.rs, such as out_of_scope_commands_are_marked_preview_in_help.
  • Keeping it in main.rs: the function stays pub(crate) for a test-only caller. The audit already accepts that for rocm_chat_tool_requested_args and deterministic_comfyui_tool_summary.
  • Splitting it: help assertions in main.rs, the approval-title check in chat.rs. This gets both benefits.
  • Either way, the audit should apply its test-only-caller rule the same way to all three functions.

Positive signals

• The function-level audit is exact. 63 pub(crate) functions became 21, and none of the 42 that went private is referenced outside chat.rs under any cfg, including Windows-only and test modules.
• Moving render_services_tool_result_text next to managed_service_running_state, with its test, removes the only reason that helper was crate-visible. The function body is byte-identical apart from the dropped chat:: qualifier.

Deployment notes

None

What this covered

What was read: the whole diff c7a0d65f…f810b517 (chat.rs, main.rs, docs/architecture.md), read completely, plus:

• every reference to chat:: across apps/rocm/src;
• the other crates, checked for name collisions;
• the four commit messages;
• after the findings were settled, the PR description and ROCMAI-88.

What was run in this checkout:

• cargo clippy -p rocm --all-targets -- -D warnings and cargo clippy -p rocm -- -D warnings: both clean.
• cargo test -p rocm --bin rocm chat::: 111 passed, including both moved tests.
• Test inventory: 1265 tests both before and after. The names are identical; only the module paths of the 2 moved tests changed.
• One mutation, making the 7 types private: compiled clean.

CI at the head commit:

• Everything finished is green except E2E tests (Strix Halo, Windows, current). It failed on an HTTP 403 downloading the TheRock tarball, and the job at main's tip c7a0d65f fails identically, so this PR did not cause it.
• Ubuntu release/nightly and W7900 E2E were still queued or running.

Moved tests, both verdicts:

• Would they pass with the change reverted? Not applicable as a test of new behaviour. Reverting deletes the moved function from chat.rs, so the relocated test cannot compile; this PR adds no new behaviour for a test to cover.
• Would they still be green with one branch broken? The privatisation has no runtime branches; the compiler is the check.

What did not run, and why:

• Fan-out used 2 workers, the operator's cap: one for the code side, one for docs, history, agent-instruction adherence and code comments.
• The prior-changes pass (earlier PR review comments) did not run, because this review may not read PR discussion.
• Step 7 design questions were answered in the driver context, not by independent workers, because the cap was used up.
• No Windows-target build was run locally. CI's windows-build-and-test passed.

Discussion: this report was not reconciled against any PR discussion.

fixed

[jussielo-amd]

Addressed in 1771065 (on top of a rebase onto current main):

Blocking — module doc overclaimed "every item"

Confirmed independently: 7 of chat.rs's 8 pub(crate) types (ChatRocmCommandAction, CommandCapture, DeterministicModelRecipe, DeterministicServiceRow, DeterministicEngineRow, ChatPromptResult, ChatToolRunResult) had no caller outside the module — a whole-word grep across apps/rocm/src found none. Made all 7 module-private; kept ChatToolApprovalRequest pub(crate), since dash_seam.rs reaches it as chat_tool_approval_request's return type. cargo clippy -p rocm --all-targets -- -D warnings is clean. The module doc and docs/architecture.md now describe this (and the doc's "every item" claim is narrowed to "every function", since the audit never covered types before now).

Blocking — function count

Recounted directly: 123 non-test functions in chat.rs, 20 pub(crate). Fixed docs/architecture.md to say 123 instead of "~61".

Blocking — PR description vs. diff

Rewritten to describe what this diff actually does against origin/main: moves 2 tests (not 82), relocates render_services_tool_result_text, and finishes the visibility audit across functions and types (not just the ~12-function estimate). Test count is unchanged at 1266 (1260 passed, 6 ignored) before and after.

Non-blocking — run_internal_mcp_call comment

Confirmed main.rs:2283 also calls it in production, alongside dash_seam.rs. Comment now names both.

Decision — where runtimes_help_uses_the_runtime_noun_throughout lives

Split it: the clap --help assertions stay in main.rs under the same name, next to their real siblings (out_of_scope_commands_are_marked_preview_in_help, the_preview_marker_is_explained_and_not_contradicted); the chat approval-modal-title assertion moved to chat.rs as runtimes_activate_approval_modal_uses_the_runtime_noun. This also resolves the "apply the test-only-caller rule consistently" note — chat_rocm_command_action_from_args no longer has any external (test or otherwise) caller to reconcile against rocm_chat_tool_requested_args/deterministic_comfyui_tool_summary.

Re-verified after rebasing onto main (one new, unrelated commit, no conflicts): cargo clippy -p rocm --all-targets -- -D warnings clean, cargo test -p rocm --bin rocm green (one PID-race flake in comfyui::tests::status_reports_stopped_when_saved_comfyui_pid_is_gone, confirmed unrelated — passes in isolation and on a clean full-suite rerun).