[jussielo-amd]

Summary

chat.rs and freeform_planner.rs in apps/rocm imported each other: the planner called back into chat:: for a handful of small helpers while chat.rs calls the planner to build its plans. Mutual imports like that make it hard to split either file, because every helper either side shares has to stay reachable from both. This PR removes the cycle without changing behaviour, by moving the three groups of leaf helpers the planner reached into chat into modules of their own. It is part of the ongoing split of the assistant code; the larger splits of chat.rs and providers.rs follow separately.

After the last commit freeform_planner.rs has no chat import in production code and the only edge left between the two modules is chat -> freeform_planner (plan_freeform_request_with_context).

What moved

Every item moved verbatim. Visibility widened only where the item is now called across a module boundary.

Symbols	From	To
chat_cli_arg_value, chat_cli_arg_value_checked, chat_cli_has_flag, ensure_flag	chat.rs	rocm_argv.rs (new, no crate:: dependencies)
format_structured_tool_call, quote_tool_arg, serve_args_request_cpu_device, device_policy_is_cpu_mode	freeform_planner.rs	rocm_argv.rs
any_substring, requested_install_prefix_from_prompt, requested_bare_install_prefix_from_prompt, clean_requested_install_prefix, looks_like_user_path, requested_therock_build_date_from_prompt, requested_therock_version_from_prompt, looks_like_therock_runtime_version	chat.rs	prompt_text.rs (new, depends only on therock::RuntimeVersionSelector)
infer_engine_from_request and its test infer_engine_from_request_does_not_match_aim_as_a_substring	freeform_planner.rs	prompt_text.rs
ChatRocmCommandAction, ChatToolApprovalRequest, chat_tool_approval_request, validate_chat_tool_call, the validate_chat_*_tool_call family, validate_required_chat_string, validate_optional_chat_integer, chat_rocm_command_action, normalized_chat_rocm_command_args, chat_install_driver_action, chat_rocm_command_action_from_args, canonicalize_chat_rocm_command, validate_chat_rocm_command_safety, chat_install_prefix_is_system, chat_tool_call_is_read_only, rocm_chat_tool_requested_args, rocm_chat_tool_requested_command, push_optional_json_cli_arg, json_string, plus the 37 chat tests that exercise only these items	chat.rs	tool_policy.rs (new)

Widened to pub(crate): chat_cli_arg_value_checked and device_policy_is_cpu_mode (rocm_argv), requested_therock_version_from_prompt (prompt_text), and normalized_chat_rocm_command_args, json_string, chat_install_prefix_is_system, chat_rocm_command_action, rocm_chat_tool_requested_command (tool_policy).

Every other module keeps its path through re-exports: chat::ensure_flag (freeform_execution.rs), chat::any_substring (freeform_answer.rs), chat::validate_chat_tool_call, chat::chat_tool_call_is_read_only and chat::chat_tool_approval_request (dash_seam.rs), freeform_planner::format_structured_tool_call (comfyui.rs, freeform_execution.rs, providers.rs tests), and the chat:: paths that driver_plan.rs and system_packages.rs read from their test modules (a re-export gated on cfg(test) where chat.rs itself does not call the item). The one deliberate exception is that freeform_planner.rs now calls the new modules directly, because importing through chat would keep the cycle. freeform_planner.rs no longer re-exports serve_args_request_cpu_device: its only outside reader was validate_chat_rocm_command_safety, which moved.

Production line counts: chat.rs 4111 -> 3009, freeform_planner.rs 1293 -> 1242, new rocm_argv.rs 123, prompt_text.rs 182, tool_policy.rs 963.

Import edges between the modules involved (production code only, use crate:: and crate::<module> paths, before this PR from origin/main and after it):

module	before	after
chat	automations, chat_host_facts, comfyui, driver_install, engine_runtime_select, freeform_planner, managed_service_status, providers, runtimes_cmd, service_launch, services_text, therock, uninstall_plan, update_cmd	the same, minus service_launch, plus prompt_text, rocm_argv, tool_policy
freeform_planner	chat, driver_install, providers, service_launch	driver_install, prompt_text, providers, rocm_argv, service_launch, tool_policy
rocm_argv	(new)	none
prompt_text	(new)	therock
tool_policy	(new)	driver_install, providers, rocm_argv, service_launch, services_text, therock

Before: chat <-> freeform_planner, the only two-way edge touching any of these modules. After: no two-way edge between any of them and any other module (chat -> freeform_planner is one-way). A grep for chat:: or use crate::chat in freeform_planner.rs finds nothing.

Verification

Ran:

• cargo fmt --all --check and cargo clippy -p rocm -p xtask --all-targets -- -D warnings: clean (clippy on rocm re-run after the final rebase)
• cargo test -p rocm --no-fail-fast before the final rebase: all pass (the rocm binary: 1458 passed, 7 ignored; the other rocm targets pass too)
• the leaf-name test list diff, cargo test -p rocm --bin rocm -- --list and -- --list --ignored, against origin/main, after each commit: empty (1465 tests, 7 ignored, none lost or added; module paths change, leaf names do not). After the final rebase onto a newer origin/main the diff shows only additions, the 10 property tests that main gained in the meantime (gpu_selection_proptests.rs, providers/sse_emitter_proptests.rs, test_support.rs); none is missing
• cargo test -p rocm advised_commands: pass (also after the final rebase) (the pins on apps/rocm/src/chat.rs still match their strings, none of which moved)
• cargo xtask manifest --check, cargo xtask check-crate-edges, cargo test -p xtask ascii: pass
• cargo test -p e2e-cucumber --lib: 196 passed
• leak scan of the diff against origin/main: only identifiers such as run_internal_mcp_call and the existing words "private" in docs matched; nothing internal

Not run: cargo xtask e2e and scripts/smoke_local.py (left to CI); the Windows cross-check (cargo clippy --target x86_64-pc-windows-msvc) cannot run on the Linux build host because aws-lc-sys needs a Windows C toolchain, so the Windows lane is the check. No item gated on cfg(windows) moved; the one cfg(unix) test that moved, the_assistant_cannot_name_a_system_install_folder_by_respelling_it, keeps its gate. No new tests were added and no test body changed, so there was nothing new to break-test.

User-visible behaviour

None: a pure move of code and its tests, with no change to any command, output, exit code, file or engine selection. No Gherkin scenario applies. The existing chat and freeform scenarios in tests/e2e-cucumber cover these paths and are unmodified; CI runs them.

Notes for reviewers

Read the commits in order; each of the first three compiles and passes clippy and the tests on its own, and the fourth changes only comments and docs.

1. rocm_argv.rs: the argv helpers (leaf, no crate:: dependencies). The planner's chat::chat_cli_* calls switch to it.
2. prompt_text.rs: the phrase, install-folder, TheRock date/version and engine readers. The planner's chat::any_substring and chat::requested_* calls switch to it.
3. tool_policy.rs: the tool-call validation and approval policy and its tests. The planner's one remaining chat:: call switches to it. This is the large diff, but nearly all of it is lines deleted from chat.rs and added verbatim to the new file.
4. A docs-only commit that corrects who calls the moved helpers in the module docs and docs/architecture.md (prompt_text is used by chat.rs's prompt-intent code and the planner, not by tool_policy) and completes the dependency list of tool_policy.

git diff --color-moved=dimmed-zebra makes the moves easy to confirm.

Deviations worth a look: freeform_planner.rs stops re-exporting serve_args_request_cpu_device (no reader left); the small rocm_command_call test helper is duplicated into the new test module because a chat test that stays also uses it; chat_tool_call_requests_therock_install_without_prefix and rocm_command_args_install_sdk_without_prefix stay in chat.rs (they are intent heuristics, not policy). Module docs that named chat.rs as the reader of moved code (services_text.rs, driver_install.rs, quoting.rs) and the test-name comment in main.rs are updated; docs/architecture.md describes the three new modules and the chat.rs and freeform_planner.rs entries.

Open PRs that touch the same code, for merge order: #563 rewrites chat_tool_approval_request, validate_chat_tool_call and normalized_chat_rocm_command_args and edits quote_tool_arg (and its mention in quoting.rs), so it conflicts with commits 1 and 3. #243 edits validate_chat_launch_server_tool_call and validate_chat_rocm_command_safety and adds tests that call validate_chat_tool_call, so it conflicts with commit 3. #299, #678 and #724 touch chat.rs only outside the moved code (chat_tool_call_display_label, run_internal_mcp_call, render_services_tool_result_text, the imports) and #818 only a test, so at most the use block of chat.rs needs a manual merge. #243, #299, #563, #678, #723, #724, #725, #817 and others also touch docs/architecture.md.

[Copilot]

🟢 Approval recommended

No active review issues are identified for the shared-helper extraction.

No open issues 🎉

What changed in this PR

This refactor removes the mutual dependency between chat and freeform planning by extracting shared helpers into three modules, preparing the assistant code for further splits without intended behavior changes.

Changes:

• Move argument handling, prompt-text extraction, and tool-call policy into dedicated modules.
• Update the planner to use those modules directly, preserving existing caller paths through re-exports.
• Relocate existing tests and update architecture documentation and source comments.

File	Description
docs/​architecture.md	Documents the extracted modules and revised dependencies.
crates/​rocm-core/​src/​quoting.rs	Updates the comment identifying the tool-argument quoter’s location.
apps/​rocm/​src/​tool_policy.rs	Houses relocated tool validation, approval policy, and tests.
apps/​rocm/​src/​services_text.rs	Updates comments describing validation callers.
apps/​rocm/​src/​rocm_argv.rs	Collects shared argument parsing, flag handling, and command formatting.
apps/​rocm/​src/​prompt_text.rs	Collects prompt extraction and engine inference helpers with the existing inference test.
apps/​rocm/​src/​main.rs	Registers the three modules and updates a test-reference comment.
apps/​rocm/​src/​freeform_planner.rs	Replaces chat imports with direct helper-module calls and preserves the formatting re-export.
apps/​rocm/​src/​driver_install.rs	Updates the comment naming the policy helper’s caller.

_{Review effort: Balanced}

[nowycondro]

🤖 Automated review (Claude Code) · 2daed56

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 (fetched the PR head read-only, never checked out; diffed against the merge-base with main). No ticket named for this change — it's framed as one incremental step in an ongoing, undirected split of the assistant code (the larger chat.rs/providers.rs splits "follow separately"); no specific tracking ticket is claimed to be closed by this PR.

Blocking

None

Non-blocking

None

Decisions for the author

None

Positive signals

• Every moved function and test body was verified byte-for-byte identical between its origin in chat.rs/freeform_planner.rs at the base commit and its new location in rocm_argv.rs, prompt_text.rs, and tool_policy.rs — including the safety-critical validate_chat_rocm_command_safety and chat_install_prefix_is_system (no-CPU-fallback and system-install-folder refusal logic), confirmed unchanged apart from the documented pub(crate) visibility widenings.
• The stated goal is independently verifiable and true: freeform_planner.rs has zero chat::/use crate::chat references at HEAD, and none of the three new modules' own use blocks import chat or freeform_planner, so the cycle is genuinely broken rather than just hidden behind a re-export.
• Every old external call path (chat::ensure_flag, chat::any_substring, chat::validate_chat_tool_call, chat::chat_tool_call_is_read_only, chat::chat_tool_approval_request, freeform_planner::format_structured_tool_call, plus the #[cfg(test)]-gated paths driver_plan.rs and system_packages.rs use) resolves correctly through the new re-export shims; a crate-wide grep found no stale reference to a moved symbol.
• Test count is exactly preserved: 137 #[test] occurrences in chat.rs+freeform_planner.rs at base, 137 across the five touched files at head (73+26+0+1+37) — matching the PR's claim that tests were moved, not dropped or duplicated.
• Module doc comments and docs/architecture.md updates were checked against the actual use statements and call sites in the new/changed files and are accurate.

Deployment notes

• Pure internal refactor; no user-visible behavior, no migration, no new dependency.
• The author's own description lists nine open PRs (#563, #243, #299, #678, #723, #724, #725, #817, #818) that touch the same files and will need manual merge-order handling; GitHub currently reports mergeable_state: blocked (most likely pending required review/status, not a real conflict against main as of the reviewed commit).
• At review time, four hardware-runner E2E check runs (W7900, MI300X, Strix Halo Ubuntu nightly/release) were still queued rather than completed; every check that had completed was green (build-and-test, clippy, CodeQL, Windows build/test, Commit signatures+sign-off, Coverage ratcheted, E2E consolidated report and the completed E2E hardware lanes).

What this covered

Full review of all 10 changed files between base 3944380 and head 2daed56 (apps/rocm/src/{chat.rs,freeform_planner.rs,freeform_answer.rs,rocm_argv.rs,prompt_text.rs,tool_policy.rs,main.rs,driver_install.rs,services_text.rs}, crates/rocm-core/src/quoting.rs, docs/architecture.md), each read completely alongside the moved symbols' full bodies at both commits, plus the call sites in dash_seam.rs, comfyui.rs, freeform_execution.rs, system_packages.rs, driver_plan.rs, and providers.rs that reach the moved items. Agent-instruction adherence was checked against the repo's AGENTS.md (architecture guardrails, coverage-floor mechanism, doc-link convention); nothing in this diff violates it. History was checked via git log on chat.rs/freeform_planner.rs: recent activity is the same ongoing module-splitting series this PR belongs to, with no sign of an unrelated bugfix being silently reverted by the move.

Did not run: cargo check/cargo test (no Rust toolchain in the review environment) — compile and test-pass correctness was instead verified by manual, exhaustive cross-referencing of every import, visibility, and call site, and by reading the PR's own green CI (clippy, build-and-test on Linux/Windows, the full affected-crate test suite, and the e2e suite all passed on the reviewed commit). The "prior-changes" pass (do past review comments on these files apply here) found nothing to check — no human review threads existed on this PR beyond an automated bot's "no issues" comment. Not reconciled against any human discussion thread, since none existed at review time.

🤖 Generated with Claude Code

[juhovainio]

I read this as a pure move, and it holds up. Comparing removed and added lines (ignoring whitespace), the code in the three new modules matches what left chat.rs and freeform_planner.rs. The only differences are the nine visibility widenings, the new use blocks, re-wrapped signatures, call-site prefixes, and the duplicated test helper you disclosed. Test counts balance (37 moved out of chat.rs, 1 out of freeform_planner.rs), and I found no leaks of internal names.

Nothing blocking. Three small things, left inline: a wrong caller attribution in docs/architecture.md (serve, not install sdk), a comment in chat.rs that disagrees with the re-export gating, and a ragged comment re-wrap in prompt_text.rs.

Two things for you to weigh: the chat_cli_* names now live in rocm_argv, and nothing enforces the "planner never imports chat" rule at module level (the crate-edges check is crate-level only). Also, merge order against #563 and #243 needs sequencing.

One thing I couldn't check from the diff: that none of tool_policy's dependencies (providers, therock) import chat.

CI: the two Strix Halo Ubuntu E2E lanes (nightly and release) failed in the "Run E2E tests on Strix Halo" step and I couldn't get the logs. The diff changes no behaviour and the other E2E lanes passed, so it looks like a lane flake, but a re-run would confirm. W7900 and MI300X were still pending. This is a diff-level read, not a runtime check.

[juhovainio]

The sentence saying validate_provider_planner_tool_call calls that same function "for an install sdk call" is inaccurate: the install sdk branch calls tool_policy::validate_chat_rocm_command_safety, and serve_args_request_cpu_device is called in the serve branch. Please say serve, or name the function the install-sdk path really calls.

[jussielo-amd]

Correct, that was wrong: validate_provider_planner_tool_call calls serve_args_request_cpu_device in its own serve branch and hands an install sdk call to tool_policy::validate_chat_rocm_command_safety. The sentence now says exactly that (5339c70).

[juhovainio]

Nit: the header comment re-wrap left a ragged line ("...tool_policy.rs does not use it. They were relocated verbatim from chat.rs and"); please reflow.

[jussielo-amd]

Reflowed the header comment to normal wrapping, no wording change (5339c70).

[juhovainio]

This comment says ChatRocmCommandAction and rocm_chat_tool_requested_args are read through chat:: only by other modules' tests, but they are re-exported ungated while the two truly test-only items are #[cfg(test)]. Please make the comment and the gating agree.

[jussielo-amd]

The comment was wrong and the gating was right: chat.rs calls ChatRocmCommandAction and rocm_chat_tool_requested_args in its own non-test code, so that import cannot be #[cfg(test)]. It is pub(crate) only because the tests in driver_plan.rs and system_packages.rs read both through chat::, while the two truly test-only items stay #[cfg(test)]. I corrected the comment and left the code alone (5339c70).

[jussielo-amd]

Pushed b070434: one commit for the three inline findings (5339c70) and two merges of main (a7290e8, b070434; plain merge commits, no rebase or force-push), because main moved under this PR (#563 command-preview quoting, the providers split, the rocm-core split). Your reviewed commits are unchanged.

On your other notes. Nothing in tool_policy's dependencies imports chat: I followed the crate:: module imports transitively from tool_policy with test modules excluded, over the merged tree, and the only chat:: references in that set are in the test modules of driver_plan.rs and system_packages.rs. The scan follows module paths, not items reached through main.rs re-imports. Nothing enforces "planner never imports chat" at module level today (xtask/src/crate_edges.rs is crate-level), and I did not add a new guard in this PR; the xtask source-scan contracts would be the pattern to follow if you want one as a follow-up.

Merge resolution worth a look: main's control-character screening of tool arguments now lives in tool_policy.rs (reject_control_characters_in_tool_arguments and the wider check in normalized_chat_rocm_command_args, with the three quoted --prefix expectations in the moved tests), since the functions it changed moved there. rocm_argv.rs now quotes through shell_quote as quote_tool_arg was removed on main, and the architecture entries say so. cargo test -p rocm --bin rocm passes (1499 passed) and the leaf test-name list matches origin/main.

[Copilot]

🟢 Approval recommended

No active review issues are identified for the shared-helper refactor.

No open issues 🎉

_{Review effort: Balanced}

[juhovainio]

Re-reviewed at b070434 after the merges from main. I compared the removed and added lines across the .rs files: the moves are verbatim apart from imports, call-site path rewrites, rustfmt of the widened signatures and docs. No logic line differs.

• Test count is conserved (37 moved to tool_policy.rs, 1 to prompt_text.rs).
• The new leaf modules import nothing from chat, so the cycle is broken as described.
• Visibility widening is limited to items with a cross-module caller, and the test-only re-exports are gated on cfg(test).
• architecture.md, the module docs and the main.rs comment match the code.

Non-blocking:

• The history has two "Merge origin/main" commits; a rebase before merge would match the repo's preference.
• chat.rs remains a re-export hub. That is documented and fine here, but follow-up splits could repoint callers at the leaf modules and drop the re-exports.
• The rocm_command_call test helper is duplicated, as the PR body notes.

This is based on a read of the diff, not a local build. clippy and the build/test jobs were still pending when I looked, so please confirm they come back green before merging.