[jussielo-amd]

Summary

Phase 6f-6 of the main.rs modularization (epic ROCMAI-27). Mechanical relocation, no behavior change: moves engine plugin binary resolution and the engine IPC envelope (request/response encode-decode, builtin-engine subprocess spawning) out of apps/rocm/src/main.rs into a new apps/rocm/src/engine_ipc.rs.

• resolve_engine_binary_path(_with_paths), find_engine_plugin_binary, engine_request(_with_env_root), decode_engine_response, with_scoped_builtin_engine_env, builtin_engine_request, run_builtin_engine_stdio, builtin_engine_available, run_builtin_engine_serve_http, builtin_engine_serve_http_args moved, plus ScopedEnvVar and BUILTIN_ENGINE_ENV_LOCK, which are exclusively used by this cluster but weren't named in the ticket's own cluster summary.
• The original ticket's line span was not fully contiguous: missing_packaged_engine_reason and impl From<WatcherModeArg> for WatcherMode sit physically inside it but are unrelated and have other callers — both stayed in main.rs.
• dispatch() calls two of these functions directly (run_builtin_engine_stdio, run_builtin_engine_serve_http) — its body is still byte-identical (independently re-verified by SHA-256) since the fix was adding an import, not touching dispatch()'s own lines.
• Real external callers beyond main.rs, found by whole-repo grep rather than assumed: engines_cmd.rs, serve_cmd.rs, service_launch.rs, runtime_install_finalize.rs.
• A stale doc-comment in xtask/src/env_mutation_contract.rs citing ScopedEnvVar's old location was corrected in the same commit.
• 2 tests partitioned into the new module's own test block; total workspace test count unchanged.
• docs/architecture.md updated with this module's entry.

Test plan

• cargo build --workspace
• cargo clippy --workspace --all-targets -- -D warnings
• cargo fmt --all -- --check
• cargo test --workspace — 4322 passed, 0 failed, 17 ignored (independently re-run, matches baseline exactly)
• cargo xtask manifest --check
• python scripts/smoke_local.py --profile debug --skip-build
• dispatch() call sites confirmed byte-identical (SHA-256 of the function body)

Depends on #747 (ROCMAI-212) — third PR in the ROCMAI-86 (Phase 6f) stack.
Part of Jira ROCMAI-213 / epic ROCMAI-27.

[Copilot]

🔵 Needs a closer look

Dependency #747 is still open and draft, so final approval should wait for its merge and post-rebase integration validation.

No open issues 🎉

What changed in this PR

Continues the main.rs modularization by moving engine binary resolution and inter-process communication helpers into engine_ipc.rs, with no intended behavior change.

Changes:

• Relocates engine discovery, request/response handling, environment scoping, and built-in engine entry points.
• Updates caller imports and moves two existing discovery tests into the new module.
• Documents the module and corrects a historical code-location comment.

File	Description
xtask/​src/​env_mutation_contract.rs	Updates the comment describing ScopedEnvVar’s location.
docs/​architecture.md	Documents the new module and its boundaries.
apps/​rocm/​src/​service_launch.rs	Imports HTTP argument construction from the new module.
apps/​rocm/​src/​serve_cmd.rs	Updates the engine request import.
apps/​rocm/​src/​runtime_install_finalize.rs	Updates the environment-aware engine request import.
apps/​rocm/​src/​main.rs	Removes relocated helpers and tests; registers and imports the module.
apps/​rocm/​src/​engines_cmd.rs	Updates engine request helper imports.
apps/​rocm/​src/​engine_ipc.rs	Houses the relocated engine helpers and two discovery tests.

_{Review effort: Balanced}

[nowycondro]

🔴 Automated review · pr-review-watcher · b79d767

Review — needs work

Full review of the whole change (single commit, origin/main...pr/749, base d59f596f, head b79d767c).
Implements ROCMAI-213 (a mechanical-relocation sub-phase of the main.rs modularization effort); the PR's stated acceptance criteria (byte-identical dispatch(), test partitioning into the new module, module-map doc update, no behavior change) are independently verified to be met.

Blocking

• PR description carries an AI-generated attribution footer, which this repo's own contributor guide explicitly says to avoid — PR description (final line).
  This repository's AGENTS.md §11 ("PR/Issue/Review Conduct") states: "avoid AI-generated boilerplate footers." The PR body's last line reads "🤖 Generated with Claude Code" verbatim — a direct, unambiguous instance of what that rule names.
  Confidence 100 · convention · Fix: remove the footer from the PR description.

Non-blocking

• docs/architecture.md:17's "dispatch-adjacent group" enumeration (the sentence naming which modules may use the unqualified use crate::x::{...} re-import form) omits the new engine_ipc.rs — contradicted by apps/rocm/src/main.rs:72, apps/rocm/src/engines_cmd.rs:41, apps/rocm/src/serve_cmd.rs:40, apps/rocm/src/runtime_install_finalize.rs:26, and apps/rocm/src/service_launch.rs:49, all of which import from engine_ipc in exactly that unqualified form.
  Verified pre-existing rather than introduced here: that same sentence (byte-identical between base and head) already omits runtime_install_finalize.rs, which used this same import form before this PR too. So this PR adds one more instance of an existing gap rather than causing a new one — stated as pre-existing and pattern-relevant, not blocking.
  Confidence 80 · mechanical · Fix (optional follow-up): append engine_ipc.rs (and runtime_install_finalize.rs) to that sentence's enumeration.

Decisions for the author

None

Positive signals

• engine_ipc.rs's module doc comment enumerates every external caller and every function each one reaches, and it matches the actual use crate::engine_ipc::{...} import lists in all five consuming files exactly (checked by grep across the whole tree) — an unusual level of self-verification for a routine relocation.
• The PR correctly keeps code that only sat physically nearby in main.rs out of the move — missing_packaged_engine_reason (has a caller outside the cluster plus its own test) and impl From<WatcherModeArg> for WatcherMode (unrelated clap conversion) both stay behind rather than being swept in just because they were interleaved with the cluster.

Deployment notes

None — internal, mechanical code relocation with no behavior, API, or configuration change.

What this covered

Every changed file read completely at the PR head against base origin/main: apps/rocm/src/engine_ipc.rs (new), apps/rocm/src/main.rs, apps/rocm/src/engines_cmd.rs, apps/rocm/src/serve_cmd.rs, apps/rocm/src/service_launch.rs, apps/rocm/src/runtime_install_finalize.rs, docs/architecture.md, xtask/src/env_mutation_contract.rs, plus the surrounding code at each call site and dispatch() in full. Independently confirmed dispatch()'s 456-line body is byte-identical between base and head, and the moved logic (including the Mutex/ScopedEnvVar env-scoping path) is otherwise unchanged. All CI checks in the status rollup passed (clippy, Linux/Windows build-and-test, CodeQL, coverage, E2E, commit-signature/sign-off, docs-link check); two checks (Skill checks (skillscope), Sphinx docs build (-W)) reported SKIPPED, consistent with this change not touching their path filters rather than a failure. The named Jira ticket was retrieved directly and its acceptance criteria checked against the diff.

Fan-out: one background worker independently ran the history, prior-changes, and agent-instruction-adherence passes blind (clean on history and prior-changes; its one flagged item is the non-blocking finding above, whose severity this driver corrected after independently verifying the doc line predates this PR). A second fan-out worker for an independent code-review pass could not be spawned (environment's concurrent-subagent limit) and was not retried per the harness's own guidance; that pass's checks (import/visibility correctness, call-site resolution, dead code, cross-file consumer search) were instead performed directly by this driver against the source rather than by an independent worker.

Not reconciled against any PR discussion or review thread; none was read.

[jussielo-amd]

Fixed — removed the AI-generated attribution footer from the PR description (AGENTS.md §11). The non-blocking architecture.md gap is pre-existing (predates this PR, already missing runtime_install_finalize.rs too) so leaving it as an optional follow-up per the review's own note.