[jussielo-amd]

Summary

apps/rocm/src/providers.rs had grown to about 1,600 production lines that mix four unrelated jobs: choosing and calling a provider, the ROCm tool catalog advertised to models, a hand-rolled HTTP/1.1 and SSE client for local engines, and the OpenAI-compatible and Anthropic request and response formats. Reading or reviewing any one of them meant scrolling past the others. This change moves the ROCm tool schema, the local HTTP/SSE transport and the provider wire formats into three child modules under providers/, one commit each, plus a small fourth commit that keeps the saved property-test seeds replaying. It is part of the follow-up work on splitting the assistant modules.

It is a pure move. No function body changes, no behaviour changes, no CLI output changes. The only edits beyond relocation are use lines, pub(super) where a moved item has a caller in another module, rustfmt re-wrapping of the five signatures that became longer than a line, and doc comments.

What moved

Symbols	From	To
rocm_openai_tool_definitions, rocm_openai_tool, rocm_anthropic_tool_definitions	providers.rs	providers/tool_schema.rs
post_json_to_local_endpoint, local_bearer_header, post_json_to_local_endpoint_body, stream_json_from_local_endpoint, read_sse_body, header_value, read_fixed_sse_body, read_close_delimited_sse_body, LOCAL_STREAM_TRUNCATED, body_truncated_at_eof, read_chunked_sse_body, parse_http_endpoint	providers.rs	providers/local_http.rs
providers/proptests.rs (property tests of the SSE body readers)	providers/proptests.rs	providers/local_http/proptests.rs, declared from local_http.rs
proptest-regressions/providers/proptests.txt (three saved failure seeds, contents unchanged)	apps/rocm/proptest-regressions/providers/	apps/rocm/proptest-regressions/providers/local_http/proptests.txt (commit 4)
openai_chat_request_body, openai_stream_chat_request_body, openai_local_chat_request_body, openai_local_stream_chat_request_body, openai_chat_request_body_with_stream, anthropic_chat_request_body, anthropic_stream_chat_request_body, anthropic_chat_request_body_with_stream, apply_local_model_request_defaults, local_model_should_disable_thinking, local_openai_compatible_request	providers.rs	providers/wire.rs
parse_openai_chat_message, parse_openai_tool_calls, parse_openai_sse_line, parse_anthropic_chat_message, parse_anthropic_sse_line, and the test-only parse_openai_chat_content, parse_anthropic_chat_content, parse_openai_sse_chat_events	providers.rs	providers/wire.rs
OpenAiSseEmitter, AnthropicSseEmitter and their SseBytesEmitter impls	providers.rs	providers/wire.rs
providers/sse_emitter_proptests.rs (split-invariance and panic-guard properties of the emitters and line parsers, added by #557 while this branch was open)	providers/sse_emitter_proptests.rs	providers/wire/sse_emitter_proptests.rs, declared from wire.rs

providers.rs keeps the request/response types, ProviderAdapter, the provider_* dispatch, the LocalProvider and RemoteProvider impls, select_local_chat_service, ready_local_services, resolve_remote_model, remote_endpoint, the ureq remote transport (post_json_with_headers, stream_json_with_headers) and the SseBytesEmitter trait.

Visibility widened only where a moved item has a caller outside its new module: post_json_to_local_endpoint, stream_json_from_local_endpoint and LOCAL_STREAM_TRUNCATED (the last only read by providers.rs's test module, imported under #[cfg(test)]), the six request-body builders, parse_openai_chat_message, parse_anthropic_chat_message, the two emitter structs with their new and finish, and the two tool-definition functions. Each is pub(super); nothing becomes pub(crate), and no other module in the crate referenced a moved item.

Symbols that differ from the older list this work started from: none moved, disappeared or were renamed. New since that list: the test-only helpers parse_openai_chat_content, parse_anthropic_chat_content and parse_openai_sse_chat_events (moved with the parsers they wrap), parse_openai_tool_calls, parse_openai_sse_line and parse_anthropic_sse_line (the SSE and tool-call parsers behind the named parse_* families), and providers/sse_emitter_proptests.rs from #557.

Tests

providers.rs's test module had 35 #[test] functions on this base (the older count of 34 is stale), plus the two property-test files. They are partitioned by primary subject; no test is added, removed or renamed, and the leaf-name list of cargo test -p rocm --bin rocm -- --list and -- --list --ignored is identical to origin/main after each commit.

Commit	Tests that moved
1, tool_schema.rs	none. The two tests that assert on the tool catalog (openai_request_body_includes_rocm_tools_only_for_non_streaming_tool_mode, anthropic_request_body_includes_rocm_tools_and_system_only_for_non_streaming_tool_mode) go through the request-body builders, so they move with the builders in commit 3 rather than twice.
2, local_http.rs	local_provider_endpoint_parser_supports_ipv6_loopback (into local_http.rs's mod tests), and all of providers/proptests.rs (now local_http/proptests.rs)
3, wire.rs	parses_openai_chat_content, parses_openai_chat_tool_calls_without_text, parses_anthropic_chat_content, parses_anthropic_tool_use_without_text, openai_and_anthropic_request_bodies_include_model_and_messages, request_bodies_include_temperature_and_top_p_only_when_set, openai_local_qwen3_request_disables_thinking_for_visible_answers, the two ROCm-tools body tests above, parses_openai_sse_chat_events, and all of sse_emitter_proptests.rs (now wire/sse_emitter_proptests.rs)

The remaining 24 tests stay in providers.rs: they drive LocalProvider, RemoteProvider or the provider_* dispatch end to end against a mock server, so their primary subject is the provider rather than the transport or the format. That keeps spawn_local_provider_test_server and spawn_sequenced_local_provider_test_server in providers.rs's test module; neither helper moves.

Module edges

Each child's use edges, read from the final tree. Test-only imports are marked.

Module	use super:: / use crate:: edges
providers/tool_schema.rs	none
providers/local_http.rs	super::wire::OpenAiSseEmitter; super::{ProviderStreamEvent, SseBytesEmitter}. Test-only: super::*, and in local_http/proptests.rs crate::test_support::{proptest_cases, split_at_offsets}
providers/wire.rs	super::tool_schema::{rocm_anthropic_tool_definitions, rocm_openai_tool_definitions}; super::{ChatMessage, ChatRequest, ChatToolCall, ProviderStreamEvent, SseBytesEmitter}. Test-only: super::*, and in wire/sse_emitter_proptests.rs crate::test_support::{proptest_cases, split_at_offsets}

Among the children the order is local_http -> wire -> tool_schema, with no edge back: nothing in wire.rs or tool_schema.rs names local_http, and tool_schema.rs names nothing. Every child also reads the shared types that stay in providers.rs through super, and providers.rs imports the children it calls, which is the ordinary parent/child relationship and adds no crate-level edge: none of the three children uses crate:: outside its test-only files, so cargo xtask check-crate-edges is unaffected.

Verification

Ran locally, on each of the three commits individually (checked out in turn) and again on the final head:

• cargo fmt --all --check
• cargo clippy -p rocm -p xtask --all-targets -- -D warnings
• leaf-name test diff, cargo test -p rocm --bin rocm -- --list and -- --list --ignored, against origin/main: empty after each commit
• cargo test -p rocm --bin rocm providers (the 24 staying tests, the moved tests and both property-test files)
• cargo test -p rocm --bin rocm advised_commands and cargo test -p xtask ascii (no path pin names providers.rs or its children; the proptests.rs fragment in xtask/src/ascii_output_contract.rs matches by file name and still covers the moved files)
• cargo xtask check-crate-edges, cargo xtask manifest --check
• cargo test -p rocm --bin rocm --no-fail-fast once on the final head: 1467 passed, 0 failed, 8 ignored
• a line-multiset comparison of the old providers.rs against the four new files, ignoring pub(super), super:: and whitespace: the only differences are use lines, module headers and the five re-wrapped signatures

Not run here: the full workspace gate, cargo xtask e2e and scripts/smoke_local.py (CI carries them), and the Windows cross-clippy. No moved item is behind a cfg(windows) gate, and the moved code has no platform-specific imports, so the Windows lane is the check.

User-visible behaviour

None, so no new scenario. This is an internal module move; the existing chat and serve scenarios that reach the local provider and its streaming path exercise the moved code unchanged.

Notes for reviewers

proptest finds its saved-failure file by the test's source path, so moving proptests.rs alone would have silently stopped replaying the three saved seeds. Commit 4 moves the regression file to match; I checked the new path is the one read by appending an unparsable line and seeing proptest report it for the new path, then removing the line. wire/sse_emitter_proptests.rs has no regression file, here or on origin/main.

Read the commits in order. Commit 1 is the leaf (no dependencies); commit 2 moves the transport and the SSE-reader property tests, importing OpenAiSseEmitter from the parent for now; commit 3 moves the wire formats and the emitter property tests and re-points local_http at wire, which makes the edge one-way. Commits 1 to 3 each build, pass clippy and pass the tests on their own; commit 4 only renames a data file. docs/architecture.md is updated in the same commit as the code it describes.

The branch was rebased once onto #557 (merged while this work was open), which added providers/sse_emitter_proptests.rs; that file moved with the emitters in commit 3.

Other open PRs that touch providers.rs (read-only check of their current diffs): none of them edits anything this change moves, so none should conflict textually with any of the three commits. #678, #723 and #883 each add one field to a struct literal inside serve_smoke_test_reports_no_metrics_when_engine_dies_mid_chunked_stream, which stays in providers.rs. #724 and #725 (drafts, stacked on #723) edit LocalProvider::status, select_local_chat_service, ready_local_services and add or extend tests that drive those against a mock server, all of which stay in providers.rs. A follow-up that touches the request-body builders, the SSE emitters or the local transport would belong in wire.rs or local_http.rs after this lands.

[Copilot]

🟢 Approval recommended

No active review issues were identified, though tests and builds were not rerun in this review environment.

No open issues 🎉

What changed in this PR

This PR splits ROCm CLI’s provider implementation into focused child modules, continuing the assistant-module cleanup without changing provider behavior or CLI output.

Changes:

• Extracts the tool catalog, local HTTP/SSE transport, and OpenAI/Anthropic wire formats while keeping provider selection and dispatch in providers.rs.
• Moves existing tests alongside their implementations and relocates saved property-test seeds to preserve replay.
• Documents module responsibilities, visibility, and dependencies.

Tests and builds were not rerun in this review environment.

File	Description
docs/​architecture.md	Documents the provider module split and dependency boundaries.
apps/​rocm/​src/​providers/​wire/​sse_emitter_proptests.rs	Relocates emitter property tests and updates explanatory comments.
apps/​rocm/​src/​providers/​wire.rs	Houses request builders, response parsers, SSE emitters, and their unit tests.
apps/​rocm/​src/​providers/​tool_schema.rs	Isolates the shared ROCm tool catalog and provider-specific schema formatting.
apps/​rocm/​src/​providers/​local_http/​proptests.rs	Relocates transport property tests beside the body readers.
apps/​rocm/​src/​providers/​local_http.rs	Houses local HTTP transport, SSE body readers, and endpoint parsing.
apps/​rocm/​src/​providers.rs	Imports extracted functionality while retaining provider orchestration and integration tests.
apps/​rocm/​proptest-regressions/​providers/​local_http/​proptests.txt	Relocates the three saved failure seeds to match the test source path.

_{Review effort: Balanced}

[nowycondro]

🤖 Automated review (Claude Code) · e023bd1

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 (base 3944380...head e023bd1, 4 commits, 8 files).
No ticket named. The description is self-contained: it claims a pure mechanical move of the ROCm tool schema, local HTTP/SSE transport and provider wire formats out of providers.rs into three new child modules, with no function-body or behaviour changes.

Blocking

None

Non-blocking

None

Decisions for the author

• wire.rs still bundles both the OpenAI and Anthropic wire formats (855 lines) — tradeoff. Splitting by provider (openai.rs/anthropic.rs) instead of by layer (schema/transport/wire) was a reasonable alternative grouping; the chosen layer-based split keeps the OpenAI/Anthropic request builders next to the parsers and emitters that share their shapes, which has its own value for reviewing request/response round-trips together. Not a defect either way.
• providers.rs still keeps ProviderAdapter, the provider_* dispatch, LocalProvider/RemoteProvider, and the remote ureq transport (2308 lines after this PR) — tradeoff, explicitly scoped out by the author ("a follow-up... would belong in wire.rs or local_http.rs after this lands"). This PR's description accurately states what stays and why, so this is a deliberate boundary rather than an incomplete extraction.

Positive signals

• The "pure move" claim held up under independent verification: a normalized line-multiset diff (ignoring whitespace, pub(super), and super::/self:: prefixes) between the old providers.rs and the four new files showed only license headers, mod/use declarations, the visibility keywords the PR claims to add, new module-level doc comments, and the handful of rustfmt re-wrapped multi-line signatures — no unexplained content on either side.
• Commit structure is genuinely incremental and independently buildable (tool_schema → local_http → wire → proptest-regression rename), each with a commit message that says what moved and why, which made this an easy diff to review honestly.
• The proptest regression-seed relocation (proptest-regressions/providers/proptests.txt → .../providers/local_http/proptests.txt) is confirmed byte-identical content, just renamed, and the PR description explains why this file had to move (proptest keys saved failures by test source path) rather than leaving that as a silent side effect.
• docs/architecture.md is updated in the same commit as the code it describes, and its new bullets for tool_schema.rs/local_http.rs/wire.rs accurately match the pub(super) surface and one-way dependency edge (local_http → wire → tool_schema, no edge back) actually implemented.

Deployment notes

None — this is an internal module reorganization with no behavioural or interface change, confirmed by four independent passes plus an original-vs-moved line-multiset comparison.

What this covered

Read completely and independently verified in four parallel passes grouped by coupling: (1) providers/tool_schema.rs plus its old location and the docs/architecture.md bullet; (2) providers/local_http.rs + providers/local_http/proptests.rs plus the old providers/proptests.rs and the regression-seed rename; (3) providers/wire.rs + providers/wire/sse_emitter_proptests.rs plus the old providers/sse_emitter_proptests.rs; (4) the full remaining providers.rs diff (old 3572 lines → new 2308 lines) for completeness, orphaned code, and import/export matching against the three new modules' pub(super) surfaces. Each pass independently found the moved code byte-identical to its old location modulo visibility keywords, doc comments, and rustfmt re-wrapping. I separately re-ran this as a normalized line-multiset comparison myself (not delegated) as an additional independent check. I also checked: AGENTS.md's module-splitting guardrails (consistent — "full domain extraction" via child modules with docs/architecture.md as the module map is the documented pattern), MANIFEST.md (does not reference providers.rs, no update needed), the codebase for stale references to the old file paths (none found), and that xtask/src/ascii_output_contract.rs's proptests.rs path-fragment match still covers both new proptest files as the PR claims. CI at this commit: every completed check (build, clippy, fmt/lint, Windows build/test, CodeQL, commit sign-off, affected-crate tests, E2E consolidated report) is green; four hardware-runner E2E jobs (MI300X, W7900, Strix Halo Ubuntu nightly/release) were still queued at review time rather than failed.

Did not run: no local cargo toolchain was available in this environment, so the build/clippy/test commands the PR author reports were not independently re-executed here — this relies on the PR's own verification log and on CI's green checks for that. Prior-changes pass (whether earlier review comments on providers.rs apply here) was not run as a dedicated search of historical review threads. Not reconciled against any discussion on this PR.

🤖 Generated with Claude Code

[juhovainio]

I went through the saved diff line by line and this is a clean move. The only changes are use lines, mod declarations, file headers, pub(super) widenings, rustfmt re-wraps and doc-comment path updates; no function body changed. rocm_openai_tool stays private, and LOCAL_STREAM_TRUNCATED is imported under #[cfg(test)] only. Both proptest files and the regression seed file are renamed into the matching module directories, so the saved seeds keep replaying. The new docs/architecture.md entries link to files that exist.

This reflects a diff-only read, not a runtime check. The Strix Halo nightly and release E2E checks were red when I looked and their logs weren't available; they look like a runner issue, but worth a re-run or a comparison against main before merging.