[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.

[nowycondro]

🔴 Automated review · pr-review-watcher · a427b54

Review — needs work

Full review of the whole change.
Implements the ROCMAI-213 ticket ("Phase 6f-6: Extract engine plugin binary resolution + engine IPC envelope") in full, per the ticket's title and the PR's own description; epic ROCMAI-27 is a container epic, not something this single PR claims to close.

Blocking

• docs/architecture.md misattributes which files call engine_request vs. engine_request_with_env_root — docs/architecture.md:43
  The new engine_ipc.rs bullet states: "engine_request and engine_request_with_env_root are also read from engines_cmd.rs and serve_cmd.rs/runtime_install_finalize.rs respectively" — i.e. it pairs engine_request with engines_cmd.rs only, and engine_request_with_env_root with serve_cmd.rs/runtime_install_finalize.rs. Verified directly against the PR head: serve_cmd.rs (apps/rocm/src/serve_cmd.rs:40,739) imports and calls only engine_request, never engine_request_with_env_root — there are zero occurrences of that name anywhere in the file. engine_request_with_env_root is actually called from engines_cmd.rs (lines 91, 362, 685) and runtime_install_finalize.rs (line 542) only. engine_request is called from engines_cmd.rs (line 339) and serve_cmd.rs (line 739), not just engines_cmd.rs. So under the natural "respectively" reading the sentence gets both pairings wrong, and under any reading it never credits engines_cmd.rs as a caller of engine_request_with_env_root even though it calls it three times. This is new content the PR itself is adding, in the one paragraph whose job is to state this module's exact caller set accurately — docs/architecture.md is maintained as this repo's "living module map" that later contributors rely on and that every other entry in this file is held to the same precision on.
  Confidence 85 · mechanical · Fix: correct the attribution to "engine_request is read from engines_cmd.rs and serve_cmd.rs; engine_request_with_env_root is read from engines_cmd.rs and runtime_install_finalize.rs" (or equivalent correct wording).

Non-blocking

None

Decisions for the author

None

Positive signals

• The relocation itself is a verified byte-for-byte move: every one of the 14 relocated items (functions, the ScopedEnvVar type/Drop impl, the BUILTIN_ENGINE_ENV_LOCK static, and the 2 tests) was diffed against the pre-move main.rs content and found identical modulo an immaterial rustfmt re-wrap of one signature — a real no-behavior-change mechanical move, and the BUILTIN_ENGINE_ENV_LOCK mutual-exclusion invariant is preserved (exactly one definition exists repo-wide after the move).
• The new module's top-of-file doc comment makes an unusually specific, checkable claim about exactly which files call which pub(crate) items and which two interleaved items deliberately did not move — every individual claim in it was checked against the code and repo-wide grep results and holds (the one inaccuracy found is in docs/architecture.md's separate, newer summary of the same facts, not in the module's own doc comment).

Deployment notes

None — pure intra-crate code relocation with no behavior, API, or dependency change; all CI checks (build, clippy, tests, coverage, CodeQL, E2E) passed on this head commit.

What this covered

Every file in the diff, read completely at the PR head and cross-checked against the base: apps/rocm/src/engine_ipc.rs (new file) and the corresponding removed code in apps/rocm/src/main.rs; the four caller files apps/rocm/src/engines_cmd.rs, serve_cmd.rs, service_launch.rs, runtime_install_finalize.rs; docs/architecture.md; xtask/src/env_mutation_contract.rs. Also checked: repo-wide grep for every relocated symbol name to rule out missed callers or a duplicated lock; AGENTS.md for project-specific conventions (the "mechanical relocation, owns private types" pattern and bare-Jira-ID-permitted rule both match this PR); git history on the touched caller files (nothing notable beyond the established Phase 6a–6f precedent series); CI status via gh pr view --json statusCheckRollup (all required checks green, two docs-only checks correctly skipped). Reviewed against commits d59f596 (base, origin/main) … a427b54 (head).

Did not run: prior-changes pass (PR review/discussion threads are out of scope for this review by rule and were not read at any point). Not reconciled against any discussion on this PR — this is a first, blind pass. Ticket ROCMAI-213 was retrieved directly (title/status only, no comments read): "Phase 6f-6: Extract engine plugin binary resolution + engine IPC envelope", status In Progress — title matches the PR's stated scope exactly.

[juhovainio]

Looks good. I compared the removed main.rs hunks against the new engine_ipc.rs side by side: the moved functions, the shared env lock, ScopedEnvVar and both tests are unchanged apart from the visibility bumps needed for the new module boundary, and dispatch() is untouched. docs/architecture.md is updated in both the exception list and the module inventory, and CI is green. This reflects a read of the diff; I did not re-run the suite locally.

Two non-blocking nits are left inline.

[juhovainio]

Nit: this module doc duplicates the architecture.md inventory entry (function lists, caller lists, visibility). Consider keeping a short summary here and leaving the detail to docs/architecture.md so the two cannot drift.

[jussielo-amd]

Fair nit, and it does have precedent elsewhere in this file — crates/rocm-core's newer Phase 7 modules (automation.rs, config.rs, host_gpu.rs, managed_runtime.rs, model_catalog.rs, quoting.rs, rocm_install.rs, service_record.rs) all use a lighter "see the module's own doc comment for what it owns" deferral in docs/architecture.md instead of duplicating the detail.

That said, apps/rocm's Phase 6 mechanically-relocated modules have consistently done the opposite across this whole PR series: managed_service_status.rs, uninstall_plan.rs, driver_install.rs, freeform_execution.rs, and now engine_ipc.rs all duplicate the full ownership/caller detail in both the module doc comment and the docs/architecture.md entry. Trimming just this one entry would make it inconsistent with every one of its own siblings rather than with the repo as a whole.

Given that, I'd rather not change it in this PR — it reads to me like a good candidate for a separate, repo-wide follow-up (standardize all the Phase 6 apps/rocm entries on the Phase 7 deferral pattern) than a one-off here.

[juhovainio]

Nit: the "At the time, ..." rewrite of this existing comment is historical prose that is not needed for the path update; consider keeping the original wording and changing only the path.

[jussielo-amd]

Good catch on this being unnecessary prose, but I don't think I can revert it to the literal original wording — that would now be factually wrong. ScopedEnvVar moved out of main.rs in this PR, and main.rs is still by a wide margin the largest test module in the tree (243 #[test] functions vs. engine_ipc.rs's 2), so the sentence can no longer claim the type is "in the same file" as that largest test module in the present tense.

Instead of the "At the time, ..." framing, I trimmed it to the more minimal fix: dropped that lead-in and kept it in plain past tense ("The type was visible to..." instead of "At the time, the type was visible to..."). That addresses the "unneeded historical prose" concern without reintroducing a false claim. This is in the commit just pushed.

[nowycondro]

🔴 Automated review · pr-review-watcher · 029acf7

Review — no blocking findings

Full review of the whole change. Part of Jira ROCMAI-213 (Phase 6f-6 of the main.rs modularization epic ROCMAI-27, stacked on ROCMAI-212/#747); ROCMAI-213/ROCMAI-27/ROCMAI-212/ROCMAI-86 are internal Jira identifiers not retrievable from here.

Blocking

None

Non-blocking

None

Decisions for the author

None

Positive signals

• The PR description's own verification claims hold up under independent re-check: dispatch()'s two call sites (run_builtin_engine_stdio, run_builtin_engine_serve_http) are byte-identical before/after (diffed directly), every moved function body is a verbatim copy of its main.rs original (only visibility widened to pub(crate)), and the two relocated tests (engine_plugin_discovery_finds_runtime_binary, engine_plugin_discovery_prefers_primary_plugin_dir) moved with unchanged bodies and no duplication left behind in main.rs.
• The two items the description says deliberately stayed in main.rs (missing_packaged_engine_reason, impl From<WatcherModeArg> for WatcherMode) do in fact stay, and the stated reasons (outside callers / unrelated conversion) check out against the code.
• A whole-repo grep found no stray references to the moved symbols outside the five files the description names as callers (main.rs, engines_cmd.rs, serve_cmd.rs, service_launch.rs, runtime_install_finalize.rs); the two remaining hits in runtime_services.rs/storage.rs are comments naming the function, not call sites.
• The xtask/src/env_mutation_contract.rs doc-comment update is correct, and more importantly cosmetic only: that guard walks the whole source tree at test time (collect_rust_files), so the ScopedEnvVar/BUILTIN_ENGINE_ENV_LOCK relocation could not have silently broken the contract check even without the comment fix.

Deployment notes

None — no behavior change, no migration, no new dependency or config.

What this covered

Read in full: apps/rocm/src/engine_ipc.rs (new, 479 lines), the touched regions of apps/rocm/src/main.rs (diffed old vs. new in full around every moved symbol and the dispatch() call sites, plus the module-list/import header), 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 (each in full), xtask/src/env_mutation_contract.rs (in full, to confirm the contract check is path-independent), and docs/architecture.md's changed section. Commits ea86403c...029acf7d (single commit 029acf7d).

CI: all applicable checks pass (build-and-test, windows-build-and-test, Test (affected crates), clippy, fmt/prek, CodeQL ×3, license headers, third-party notices, coverage, E2E, markdown links, commit sign-off, Linux PR binaries, remote control channel). Skill checks (skillscope) and Sphinx docs build (-W) report SKIPPED rather than a result — not treated as blocking since no check reports failure, but noted since docs/architecture.md changed and the skipped Sphinx job nominally covers docs.

Did not run: history/git blame pass and prior-PR-comment pass — no indication of relevant prior review history on these specific lines beyond what GraphQL already surfaced (addressed by the claim-check step, not this review). Design review (Step 7) was done inline given the change's small, single-purpose scope rather than as separate fanned-out workers, per instruction to prefer a careful single-context pass for a diff this size (~1,100 lines, mechanical move); no fundamental approach concerns surfaced. Not reconciled against any PR discussion/comments — this review was conducted blind to any such discussion.