From 029acf7d042a38b5074f90500912adcbb54ee203 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Tue, 27 Oct 2026 08:04:02 +0000 Subject: [PATCH] ROCMAI-213: extract engine plugin binary resolution and IPC envelope into engine_ipc.rs Signed-off-by: Jussi Elo --- apps/rocm/src/engine_ipc.rs | 479 ++++++++++++++++++++++ apps/rocm/src/engines_cmd.rs | 7 +- apps/rocm/src/main.rs | 422 +------------------ apps/rocm/src/runtime_install_finalize.rs | 6 +- apps/rocm/src/serve_cmd.rs | 5 +- apps/rocm/src/service_launch.rs | 7 +- docs/architecture.md | 3 +- xtask/src/env_mutation_contract.rs | 5 +- 8 files changed, 513 insertions(+), 421 deletions(-) create mode 100644 apps/rocm/src/engine_ipc.rs diff --git a/apps/rocm/src/engine_ipc.rs b/apps/rocm/src/engine_ipc.rs new file mode 100644 index 000000000..b26dbd873 --- /dev/null +++ b/apps/rocm/src/engine_ipc.rs @@ -0,0 +1,479 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! Engine plugin binary resolution and the engine IPC request/response +//! envelope: locating an engine's executable (packaged plugin vs. the +//! platform-packaged fallback), encoding/decoding the JSON-over-stdio +//! envelope used to talk to it, and the built-in (in-process) fast path for +//! `lemonade`/`vllm` that skips spawning a child process entirely. +//! +//! Mechanically relocated from `main.rs` with no behavior change — Phase 6f +//! (ROCMAI-213) of the `main.rs` modularization (see `docs/architecture.md`). +//! `dispatch()` calls directly into this module at two points — +//! `Command::EngineStdio { engine } => run_builtin_engine_stdio(&engine)` and +//! the `__engine-serve-http` arm's `run_builtin_engine_serve_http(...)` call — +//! both re-imported via `use crate::engine_ipc::{run_builtin_engine_serve_http, +//! run_builtin_engine_stdio};`; `dispatch()`'s own body is otherwise +//! unaffected. +//! +//! This module owns: resolving an engine's binary path +//! (`resolve_engine_binary_path`, `resolve_engine_binary_path_with_paths`, +//! `find_engine_plugin_binary`), the stdio request/response envelope used to +//! talk to an external engine binary or the in-process built-in handler +//! (`engine_request`, `engine_request_with_env_root`, +//! `decode_engine_response`, the `ScopedEnvVar`/ +//! `with_scoped_builtin_engine_env` env-scoping helpers and the +//! `BUILTIN_ENGINE_ENV_LOCK` mutex they serialize on, `builtin_engine_request`, +//! `run_builtin_engine_stdio`), and the built-in HTTP-serving path +//! (`builtin_engine_available`, `run_builtin_engine_serve_http`, +//! `builtin_engine_serve_http_args`). Two items sit physically inside this +//! span in `main.rs` but stay there rather than moving: `const fn +//! missing_packaged_engine_reason` has callers outside this cluster (and its +//! own test) in addition to the one inside it, and `impl From +//! for WatcherMode` is unrelated clap-arg-to-domain-type conversion that only +//! happens to sit between `find_engine_plugin_binary` and `engine_request` in +//! the original file. +//! +//! `main.rs` calls into it for `resolve_engine_binary_path`, +//! `resolve_engine_binary_path_with_paths`, `engine_request`, +//! `builtin_engine_available`, and `builtin_engine_serve_http_args`, plus the +//! two `dispatch()` call sites above; `engines_cmd.rs` and `serve_cmd.rs` call +//! into it for `engine_request`; `engines_cmd.rs` and +//! `runtime_install_finalize.rs` call into it for +//! `engine_request_with_env_root`; `service_launch.rs` calls into it for +//! `builtin_engine_serve_http_args`. `find_engine_plugin_binary`, +//! `decode_engine_response`, `with_scoped_builtin_engine_env`, +//! `builtin_engine_request`, and `ScopedEnvVar` have no callers outside this +//! module and stay private. Shared helpers this module calls back into at the +//! crate root — `apply_app_path_env`, `app_path_env_vars`, +//! `with_sigpipe_ignored`, `optional_arg`, `engine_recipe_json_arg`, +//! `device_policy_name`, `missing_packaged_engine_reason`, +//! `parse_device_policy` — stay in `main.rs` and are reached unqualified via +//! `use crate::{...}`, same as for other mechanically relocated modules. + +use std::ffi::OsString; +use std::io::{self, BufRead, Read, Write}; +use std::path::{Path, PathBuf}; +use std::process::{Command as ProcessCommand, Stdio}; +use std::sync::{Mutex, OnceLock}; +use std::thread; + +use anyhow::{Context, Result, bail}; +use rocm_core::{AppPaths, engine_binary_path, engine_plugin_dirs}; +use rocm_engine_protocol::{ + DevicePolicy, EngineMethod, EnginePluginDescriptor, EngineRecipeHint, EngineRequestEnvelope, + EngineResponseEnvelope, +}; +use serde::Serialize; +use serde::de::DeserializeOwned; + +use crate::{ + app_path_env_vars, apply_app_path_env, device_policy_name, engine_recipe_json_arg, + missing_packaged_engine_reason, optional_arg, parse_device_policy, with_sigpipe_ignored, +}; + +static BUILTIN_ENGINE_ENV_LOCK: OnceLock> = OnceLock::new(); + +pub(crate) fn resolve_engine_binary_path(engine: &str) -> Result { + let paths = AppPaths::discover()?; + resolve_engine_binary_path_with_paths(engine, &paths) +} + +pub(crate) fn resolve_engine_binary_path_with_paths( + engine: &str, + paths: &AppPaths, +) -> Result { + if let Some(path) = find_engine_plugin_binary(engine, engine_plugin_dirs(paths))? { + return Ok(path); + } + if let Some(reason) = missing_packaged_engine_reason(engine) { + bail!("{reason}"); + } + engine_binary_path(engine) +} + +fn find_engine_plugin_binary(engine: &str, plugin_dirs: I) -> Result> +where + I: IntoIterator, + P: AsRef, +{ + Ok(rocm_engine_protocol::discover_engine_plugins(plugin_dirs) + .context("failed to discover engine plugin binaries")? + .into_iter() + .find(|plugin: &EnginePluginDescriptor| plugin.id == engine) + .map(|plugin| plugin.executable_path)) +} + +pub(crate) fn engine_request( + paths: Option<&AppPaths>, + engine: &str, + method: EngineMethod, + request: &T, +) -> Result +where + T: Serialize, + R: DeserializeOwned, +{ + engine_request_with_env_root(paths, engine, method, request, None) +} + +pub(crate) fn engine_request_with_env_root( + paths: Option<&AppPaths>, + engine: &str, + method: EngineMethod, + request: &T, + env_root: Option<&Path>, +) -> Result +where + T: Serialize, + R: DeserializeOwned, +{ + let stream_progress = matches!(&method, EngineMethod::Install); + let envelope = EngineRequestEnvelope { + method, + payload: serde_json::to_value(request) + .context("failed to encode engine request payload")?, + }; + if let Some(envelope) = with_scoped_builtin_engine_env(paths, env_root, || { + builtin_engine_request(engine, &envelope) + }) { + return decode_engine_response(envelope); + } + + let engine_binary = resolve_engine_binary_path(engine).with_context(|| { + format!( + "unable to locate engine binary for {engine}; build the workspace or install the engine package" + ) + })?; + let mut command = ProcessCommand::new(engine_binary); + command.arg("stdio"); + if let Some(paths) = paths { + apply_app_path_env(&mut command, paths); + } + if let Some(env_root) = env_root { + command.env("ROCM_CLI_ENGINE_ENVS_ROOT", env_root); + } + if stream_progress { + command.env("ROCM_ENGINE_PROGRESS_STDERR", "1"); + } + let mut child = command + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .context("failed to spawn engine stdio process")?; + + { + let mut stdin = child + .stdin + .take() + .context("engine stdio child did not expose stdin")?; + // Ignore SIGPIPE for the duration of the write: with SIG_DFL in effect + // process-wide, an engine that exits before reading its stdin would + // otherwise kill `rocm` with SIGPIPE before the exit-status diagnostics + // below can run. Under SIG_IGN the write returns EPIPE instead, which we + // surface as the normal "failed to write engine request" error and then + // let `wait()` report why the engine exited. + with_sigpipe_ignored(|| { + serde_json::to_writer(&mut stdin, &envelope) + .context("failed to write engine request")?; + stdin.write_all(b"\n")?; + Ok::<(), anyhow::Error>(()) + })?; + } + + let stderr_handle = child.stderr.take().map(|stderr| { + thread::spawn(move || { + let mut reader = io::BufReader::new(stderr); + let mut collected = Vec::new(); + let mut line = Vec::new(); + loop { + line.clear(); + match reader.read_until(b'\n', &mut line) { + Ok(0) => break, + Ok(_) => { + if let Ok(text) = std::str::from_utf8(&line) { + eprint!("{text}"); + } else { + let _ = io::stderr().write_all(&line); + } + collected.extend_from_slice(&line); + } + Err(error) => { + let message = format!("failed to read engine progress: {error}\n"); + eprint!("{message}"); + collected.extend_from_slice(message.as_bytes()); + break; + } + } + } + collected + }) + }); + let mut stdout = Vec::new(); + child + .stdout + .take() + .context("engine stdio child did not expose stdout")? + .read_to_end(&mut stdout) + .context("failed to read engine stdio response")?; + let status = child + .wait() + .context("failed waiting for engine stdio response")?; + let stderr = stderr_handle + .map(|handle| handle.join().unwrap_or_else(|_| Vec::new())) + .unwrap_or_default(); + + if !status.success() && stdout.is_empty() { + let stderr = String::from_utf8_lossy(&stderr).trim().to_owned(); + if stderr.is_empty() { + bail!("engine stdio process exited with status {status}"); + } + bail!("engine stdio process exited with status {status}: {stderr}"); + } + let envelope: EngineResponseEnvelope = serde_json::from_slice(&stdout).with_context(|| { + let stderr = String::from_utf8_lossy(&stderr).trim().to_owned(); + if stderr.is_empty() { + "failed to parse engine response envelope".to_owned() + } else { + format!("failed to parse engine response envelope; stderr: {stderr}") + } + })?; + decode_engine_response(envelope) +} + +fn decode_engine_response(envelope: EngineResponseEnvelope) -> Result +where + R: DeserializeOwned, +{ + if !envelope.ok { + let error = envelope.error.map_or_else( + || "unknown engine error".to_owned(), + |value| format!("{}: {}", value.code, value.message), + ); + bail!("{error}"); + } + let data = envelope + .data + .context("engine response envelope did not contain data")?; + serde_json::from_value(data).context("failed to decode engine response payload") +} + +struct ScopedEnvVar { + key: &'static str, + previous: Option, +} + +impl ScopedEnvVar { + #[allow(unsafe_code)] // std::env::set_var is unsafe in edition 2024 + fn set_path(key: &'static str, value: &Path) -> Self { + let previous = std::env::var_os(key); + unsafe { + std::env::set_var(key, value); + } + Self { key, previous } + } +} + +impl Drop for ScopedEnvVar { + #[allow(unsafe_code)] // std::env::set_var/remove_var are unsafe in edition 2024 + fn drop(&mut self) { + unsafe { + match self.previous.as_ref() { + Some(value) => std::env::set_var(self.key, value), + None => std::env::remove_var(self.key), + } + } + } +} + +fn with_scoped_builtin_engine_env( + paths: Option<&AppPaths>, + env_root: Option<&Path>, + f: impl FnOnce() -> R, +) -> R { + if paths.is_none() && env_root.is_none() { + return f(); + } + let lock = BUILTIN_ENGINE_ENV_LOCK.get_or_init(|| Mutex::new(())); + let _guard = lock.lock().expect("builtin engine env lock poisoned"); + let mut vars = Vec::new(); + if let Some(paths) = paths { + for (key, value) in app_path_env_vars(paths) { + vars.push(ScopedEnvVar::set_path(key, value)); + } + } + if let Some(env_root) = env_root { + vars.push(ScopedEnvVar::set_path( + "ROCM_CLI_ENGINE_ENVS_ROOT", + env_root, + )); + } + let result = f(); + drop(vars); + result +} + +fn builtin_engine_request( + engine: &str, + envelope: &EngineRequestEnvelope, +) -> Option { + match engine { + "lemonade" => Some(rocm_engine_lemonade::builtin_handle_envelope( + envelope.clone(), + )), + "vllm" => Some(rocm_engine_vllm::builtin_handle_envelope(envelope.clone())), + _ => None, + } +} + +pub(crate) fn run_builtin_engine_stdio(engine: &str) -> Result<()> { + let mut input = String::new(); + io::stdin() + .read_to_string(&mut input) + .context("failed to read engine stdio request")?; + let envelope: EngineRequestEnvelope = + serde_json::from_str(&input).context("failed to parse engine stdio request")?; + let response = builtin_engine_request(engine, &envelope) + .with_context(|| format!("engine `{engine}` is not built into this rocm binary"))?; + print!("{}", serde_json::to_string(&response)?); + Ok(()) +} + +pub(crate) fn builtin_engine_available(engine: &str) -> bool { + matches!(engine, "lemonade" | "vllm") +} + +#[allow(clippy::too_many_arguments)] +pub(crate) fn run_builtin_engine_serve_http( + engine: &str, + service_id: String, + model_ref: String, + host: String, + port: u16, + device_policy: &str, + gpu_indices: Vec, + runtime_id: Option, + env_id: Option, + state_path: PathBuf, + log_path: Option, + engine_recipe: Option, +) -> Result<()> { + let parsed_policy = parse_device_policy(Some(device_policy))?; + match engine { + "lemonade" => rocm_engine_lemonade::builtin_serve_http( + service_id, + model_ref, + host, + port, + parsed_policy, + gpu_indices, + runtime_id, + env_id, + state_path, + log_path, + engine_recipe, + ), + "vllm" => rocm_engine_vllm::builtin_serve_http( + service_id, + model_ref, + host, + port, + parsed_policy, + gpu_indices, + runtime_id, + env_id, + state_path, + log_path, + engine_recipe, + ), + other => bail!("engine `{other}` is not built into this rocm binary"), + } +} + +#[allow(clippy::too_many_arguments)] +pub(crate) fn builtin_engine_serve_http_args( + engine: &str, + service_id: &str, + canonical_model_id: &str, + host: &str, + port: u16, + device_policy: &DevicePolicy, + gpu_indices: &[u32], + runtime_id: Option<&str>, + env_id: Option<&str>, + engine_recipe: Option<&EngineRecipeHint>, + state_path: &Path, + log_path: Option<&Path>, +) -> Result> { + let mut args = vec![ + "__engine-serve-http".to_owned(), + engine.to_owned(), + service_id.to_owned(), + canonical_model_id.to_owned(), + "--host".to_owned(), + host.to_owned(), + "--port".to_owned(), + port.to_string(), + "--device-policy".to_owned(), + device_policy_name(device_policy).to_owned(), + ]; + if let Some(csv) = rocm_engine_protocol::gpu_indices_to_csv(gpu_indices) { + args.extend(["--gpu".to_owned(), csv]); + } + if env_id.is_none() { + args.extend(optional_arg("--runtime-id", runtime_id)); + } + args.extend(optional_arg("--env-id", env_id)); + args.extend(engine_recipe_json_arg(engine_recipe)?); + args.extend(["--state-path".to_owned(), state_path.display().to_string()]); + if let Some(log_path) = log_path { + args.extend(["--log-path".to_owned(), log_path.display().to_string()]); + } + Ok(args) +} + +#[cfg(test)] +mod tests { + use std::fs; + + use super::*; + use crate::tests::test_paths; + + #[test] + fn engine_plugin_discovery_finds_runtime_binary() -> Result<()> { + let (root, paths) = test_paths("engine-plugin"); + let plugin_dir = paths.primary_engine_plugin_dir(); + fs::create_dir_all(&plugin_dir)?; + let plugin_path = plugin_dir.join( + rocm_engine_protocol::platform_engine_plugin_binary_name("vllm"), + ); + fs::write(&plugin_path, "plugin")?; + + let discovered = find_engine_plugin_binary("vllm", engine_plugin_dirs(&paths))?; + let _ = fs::remove_dir_all(root); + + assert_eq!(discovered, Some(plugin_path)); + Ok(()) + } + + #[test] + fn engine_plugin_discovery_prefers_primary_plugin_dir() -> Result<()> { + let (root, paths) = test_paths("engine-plugin-precedence"); + let primary_dir = paths.primary_engine_plugin_dir(); + let compatibility_dir = paths.data_dir.join("engines"); + fs::create_dir_all(&primary_dir)?; + fs::create_dir_all(&compatibility_dir)?; + let name = rocm_engine_protocol::platform_engine_plugin_binary_name("vllm"); + let primary_path = primary_dir.join(&name); + fs::write(&primary_path, "primary")?; + fs::write(compatibility_dir.join(&name), "compatibility")?; + + let discovered = find_engine_plugin_binary("vllm", engine_plugin_dirs(&paths))?; + let _ = fs::remove_dir_all(root); + + assert_eq!(discovered, Some(primary_path)); + Ok(()) + } +} diff --git a/apps/rocm/src/engines_cmd.rs b/apps/rocm/src/engines_cmd.rs index 44ae5c9d6..b17443892 100644 --- a/apps/rocm/src/engines_cmd.rs +++ b/apps/rocm/src/engines_cmd.rs @@ -38,6 +38,7 @@ use rocm_engine_protocol::{ use serde::Deserialize; use crate::EnginesCommand; +use crate::engine_ipc::{engine_request, engine_request_with_env_root}; use crate::runtime_install_finalize::{ ensure_libatomic_for_torch, ensure_libnuma_for_torch, ensure_openmpi_for_vllm, settle_engine_install, @@ -48,9 +49,9 @@ use crate::runtimes_cmd::{ }; use crate::therock; use crate::{ - apply_app_path_env, engine_request, engine_request_with_env_root, record_cli_audit_event, - render_engine_inventory_text_with_paths, resolve_engine_selection, - resolve_runtime_selector_to_exact_key, validate_engine_selection_runtime, + apply_app_path_env, record_cli_audit_event, render_engine_inventory_text_with_paths, + resolve_engine_selection, resolve_runtime_selector_to_exact_key, + validate_engine_selection_runtime, }; pub(crate) fn render_engine_inventory_text() -> String { diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 384a4659e..ed0ad9df7 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -23,6 +23,7 @@ mod dash_seam; mod deletion_properties; mod driver_install; mod endpoint_keys; +mod engine_ipc; mod engines_cmd; mod freeform_execution; mod freeform_planner; @@ -68,6 +69,11 @@ mod uninstall_plan; // the same reason `engines_cmd`'s runtime-resolution helpers are re-imported. use crate::automations::automations; use crate::driver_install::{install_driver, reconcile_driver_install}; +use crate::engine_ipc::{ + builtin_engine_available, builtin_engine_serve_http_args, engine_request, + resolve_engine_binary_path, resolve_engine_binary_path_with_paths, + run_builtin_engine_serve_http, run_builtin_engine_stdio, +}; use crate::engines_cmd::{ engine_manages_own_runtime, engines, env_root_for_service, runtime_manifest_for_selector, }; @@ -112,21 +118,18 @@ use rocm_core::{ PERMISSIONS_MODE_FULL_ACCESS, RocmCliConfig, TELEMETRY_MODE_LOCAL, TELEMETRY_MODE_OFF, WatcherMode, append_audit_event, builtin_model_recipes, builtin_watchers, connect_tcp_stream, daemon_binary_path, default_engine_for_platform, detect_host_gfx_target, - detect_host_gpu_summary, engine_binary_path, engine_plugin_dirs, format_host_port, - format_http_base_url, interactive_terminal, load_model_recipe_registry, - load_recent_audit_events, load_recent_automation_events, load_recent_automation_proposals, - managed_pip_cache_dir, model_artifact_cache_status, model_catalog_platforms, - model_recipe_featured, model_recipe_target_platform_label, normalize_therock_family, - platform_matches_gfx_family, process_is_running, read_http_response_bounded, uv_cache_source, - write_all_tcp_stream, + detect_host_gpu_summary, engine_plugin_dirs, format_host_port, format_http_base_url, + interactive_terminal, load_model_recipe_registry, load_recent_audit_events, + load_recent_automation_events, load_recent_automation_proposals, managed_pip_cache_dir, + model_artifact_cache_status, model_catalog_platforms, model_recipe_featured, + model_recipe_target_platform_label, normalize_therock_family, platform_matches_gfx_family, + process_is_running, read_http_response_bounded, uv_cache_source, write_all_tcp_stream, }; use rocm_engine_protocol::{ DEFAULT_LOG_TAIL_LINES, DetectRequest, DetectResponse, DevicePolicy, - ENGINE_RECIPE_CONTRACT_VERSION, EngineMethod, EnginePluginDescriptor, EngineRecipeHint, - EngineRequestEnvelope, EngineResponseEnvelope, GpuSelection, StopRequest, StopResponse, + ENGINE_RECIPE_CONTRACT_VERSION, EngineMethod, EngineRecipeHint, GpuSelection, StopRequest, + StopResponse, }; -use serde::Serialize; -use serde::de::DeserializeOwned; use std::collections::{BTreeMap, HashMap, VecDeque}; use std::ffi::OsString; use std::fmt::Write as _; @@ -137,12 +140,11 @@ use std::process::ExitCode; #[cfg(all(test, not(windows)))] use std::process::ExitStatus; use std::process::{Command as ProcessCommand, Stdio}; -use std::sync::{Mutex, OnceLock}; +#[cfg(test)] +use std::sync::Mutex; use std::thread; use std::time::{Duration, SystemTime}; -static BUILTIN_ENGINE_ENV_LOCK: OnceLock> = OnceLock::new(); - /// User-selectable serving engines, in the order shown in `--help` and shell /// completions. Used as the clap `value_parser` for every user-facing `--engine` /// argument so the possible values stay in sync across the CLI surface. @@ -8041,37 +8043,10 @@ const fn provider_name(provider: Provider) -> &'static str { } } -fn resolve_engine_binary_path(engine: &str) -> Result { - let paths = AppPaths::discover()?; - resolve_engine_binary_path_with_paths(engine, &paths) -} - -fn resolve_engine_binary_path_with_paths(engine: &str, paths: &AppPaths) -> Result { - if let Some(path) = find_engine_plugin_binary(engine, engine_plugin_dirs(paths))? { - return Ok(path); - } - if let Some(reason) = missing_packaged_engine_reason(engine) { - bail!("{reason}"); - } - engine_binary_path(engine) -} - const fn missing_packaged_engine_reason(_engine: &str) -> Option { None } -fn find_engine_plugin_binary(engine: &str, plugin_dirs: I) -> Result> -where - I: IntoIterator, - P: AsRef, -{ - Ok(rocm_engine_protocol::discover_engine_plugins(plugin_dirs) - .context("failed to discover engine plugin binaries")? - .into_iter() - .find(|plugin: &EnginePluginDescriptor| plugin.id == engine) - .map(|plugin| plugin.executable_path)) -} - impl From for WatcherMode { fn from(value: WatcherModeArg) -> Self { match value { @@ -8082,335 +8057,6 @@ impl From for WatcherMode { } } -fn engine_request( - paths: Option<&AppPaths>, - engine: &str, - method: EngineMethod, - request: &T, -) -> Result -where - T: Serialize, - R: DeserializeOwned, -{ - engine_request_with_env_root(paths, engine, method, request, None) -} - -fn engine_request_with_env_root( - paths: Option<&AppPaths>, - engine: &str, - method: EngineMethod, - request: &T, - env_root: Option<&Path>, -) -> Result -where - T: Serialize, - R: DeserializeOwned, -{ - let stream_progress = matches!(&method, EngineMethod::Install); - let envelope = EngineRequestEnvelope { - method, - payload: serde_json::to_value(request) - .context("failed to encode engine request payload")?, - }; - if let Some(envelope) = with_scoped_builtin_engine_env(paths, env_root, || { - builtin_engine_request(engine, &envelope) - }) { - return decode_engine_response(envelope); - } - - let engine_binary = resolve_engine_binary_path(engine).with_context(|| { - format!( - "unable to locate engine binary for {engine}; build the workspace or install the engine package" - ) - })?; - let mut command = ProcessCommand::new(engine_binary); - command.arg("stdio"); - if let Some(paths) = paths { - apply_app_path_env(&mut command, paths); - } - if let Some(env_root) = env_root { - command.env("ROCM_CLI_ENGINE_ENVS_ROOT", env_root); - } - if stream_progress { - command.env("ROCM_ENGINE_PROGRESS_STDERR", "1"); - } - let mut child = command - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn() - .context("failed to spawn engine stdio process")?; - - { - let mut stdin = child - .stdin - .take() - .context("engine stdio child did not expose stdin")?; - // Ignore SIGPIPE for the duration of the write: with SIG_DFL in effect - // process-wide, an engine that exits before reading its stdin would - // otherwise kill `rocm` with SIGPIPE before the exit-status diagnostics - // below can run. Under SIG_IGN the write returns EPIPE instead, which we - // surface as the normal "failed to write engine request" error and then - // let `wait()` report why the engine exited. - with_sigpipe_ignored(|| { - serde_json::to_writer(&mut stdin, &envelope) - .context("failed to write engine request")?; - stdin.write_all(b"\n")?; - Ok::<(), anyhow::Error>(()) - })?; - } - - let stderr_handle = child.stderr.take().map(|stderr| { - thread::spawn(move || { - let mut reader = io::BufReader::new(stderr); - let mut collected = Vec::new(); - let mut line = Vec::new(); - loop { - line.clear(); - match reader.read_until(b'\n', &mut line) { - Ok(0) => break, - Ok(_) => { - if let Ok(text) = std::str::from_utf8(&line) { - eprint!("{text}"); - } else { - let _ = io::stderr().write_all(&line); - } - collected.extend_from_slice(&line); - } - Err(error) => { - let message = format!("failed to read engine progress: {error}\n"); - eprint!("{message}"); - collected.extend_from_slice(message.as_bytes()); - break; - } - } - } - collected - }) - }); - let mut stdout = Vec::new(); - child - .stdout - .take() - .context("engine stdio child did not expose stdout")? - .read_to_end(&mut stdout) - .context("failed to read engine stdio response")?; - let status = child - .wait() - .context("failed waiting for engine stdio response")?; - let stderr = stderr_handle - .map(|handle| handle.join().unwrap_or_else(|_| Vec::new())) - .unwrap_or_default(); - - if !status.success() && stdout.is_empty() { - let stderr = String::from_utf8_lossy(&stderr).trim().to_owned(); - if stderr.is_empty() { - bail!("engine stdio process exited with status {status}"); - } - bail!("engine stdio process exited with status {status}: {stderr}"); - } - let envelope: EngineResponseEnvelope = serde_json::from_slice(&stdout).with_context(|| { - let stderr = String::from_utf8_lossy(&stderr).trim().to_owned(); - if stderr.is_empty() { - "failed to parse engine response envelope".to_owned() - } else { - format!("failed to parse engine response envelope; stderr: {stderr}") - } - })?; - decode_engine_response(envelope) -} - -fn decode_engine_response(envelope: EngineResponseEnvelope) -> Result -where - R: DeserializeOwned, -{ - if !envelope.ok { - let error = envelope.error.map_or_else( - || "unknown engine error".to_owned(), - |value| format!("{}: {}", value.code, value.message), - ); - bail!("{error}"); - } - let data = envelope - .data - .context("engine response envelope did not contain data")?; - serde_json::from_value(data).context("failed to decode engine response payload") -} - -struct ScopedEnvVar { - key: &'static str, - previous: Option, -} - -impl ScopedEnvVar { - #[allow(unsafe_code)] // std::env::set_var is unsafe in edition 2024 - fn set_path(key: &'static str, value: &Path) -> Self { - let previous = std::env::var_os(key); - unsafe { - std::env::set_var(key, value); - } - Self { key, previous } - } -} - -impl Drop for ScopedEnvVar { - #[allow(unsafe_code)] // std::env::set_var/remove_var are unsafe in edition 2024 - fn drop(&mut self) { - unsafe { - match self.previous.as_ref() { - Some(value) => std::env::set_var(self.key, value), - None => std::env::remove_var(self.key), - } - } - } -} - -fn with_scoped_builtin_engine_env( - paths: Option<&AppPaths>, - env_root: Option<&Path>, - f: impl FnOnce() -> R, -) -> R { - if paths.is_none() && env_root.is_none() { - return f(); - } - let lock = BUILTIN_ENGINE_ENV_LOCK.get_or_init(|| Mutex::new(())); - let _guard = lock.lock().expect("builtin engine env lock poisoned"); - let mut vars = Vec::new(); - if let Some(paths) = paths { - for (key, value) in app_path_env_vars(paths) { - vars.push(ScopedEnvVar::set_path(key, value)); - } - } - if let Some(env_root) = env_root { - vars.push(ScopedEnvVar::set_path( - "ROCM_CLI_ENGINE_ENVS_ROOT", - env_root, - )); - } - let result = f(); - drop(vars); - result -} - -fn builtin_engine_request( - engine: &str, - envelope: &EngineRequestEnvelope, -) -> Option { - match engine { - "lemonade" => Some(rocm_engine_lemonade::builtin_handle_envelope( - envelope.clone(), - )), - "vllm" => Some(rocm_engine_vllm::builtin_handle_envelope(envelope.clone())), - _ => None, - } -} - -fn run_builtin_engine_stdio(engine: &str) -> Result<()> { - let mut input = String::new(); - io::stdin() - .read_to_string(&mut input) - .context("failed to read engine stdio request")?; - let envelope: EngineRequestEnvelope = - serde_json::from_str(&input).context("failed to parse engine stdio request")?; - let response = builtin_engine_request(engine, &envelope) - .with_context(|| format!("engine `{engine}` is not built into this rocm binary"))?; - print!("{}", serde_json::to_string(&response)?); - Ok(()) -} - -fn builtin_engine_available(engine: &str) -> bool { - matches!(engine, "lemonade" | "vllm") -} - -#[allow(clippy::too_many_arguments)] -fn run_builtin_engine_serve_http( - engine: &str, - service_id: String, - model_ref: String, - host: String, - port: u16, - device_policy: &str, - gpu_indices: Vec, - runtime_id: Option, - env_id: Option, - state_path: PathBuf, - log_path: Option, - engine_recipe: Option, -) -> Result<()> { - let parsed_policy = parse_device_policy(Some(device_policy))?; - match engine { - "lemonade" => rocm_engine_lemonade::builtin_serve_http( - service_id, - model_ref, - host, - port, - parsed_policy, - gpu_indices, - runtime_id, - env_id, - state_path, - log_path, - engine_recipe, - ), - "vllm" => rocm_engine_vllm::builtin_serve_http( - service_id, - model_ref, - host, - port, - parsed_policy, - gpu_indices, - runtime_id, - env_id, - state_path, - log_path, - engine_recipe, - ), - other => bail!("engine `{other}` is not built into this rocm binary"), - } -} - -#[allow(clippy::too_many_arguments)] -fn builtin_engine_serve_http_args( - engine: &str, - service_id: &str, - canonical_model_id: &str, - host: &str, - port: u16, - device_policy: &DevicePolicy, - gpu_indices: &[u32], - runtime_id: Option<&str>, - env_id: Option<&str>, - engine_recipe: Option<&EngineRecipeHint>, - state_path: &Path, - log_path: Option<&Path>, -) -> Result> { - let mut args = vec![ - "__engine-serve-http".to_owned(), - engine.to_owned(), - service_id.to_owned(), - canonical_model_id.to_owned(), - "--host".to_owned(), - host.to_owned(), - "--port".to_owned(), - port.to_string(), - "--device-policy".to_owned(), - device_policy_name(device_policy).to_owned(), - ]; - if let Some(csv) = rocm_engine_protocol::gpu_indices_to_csv(gpu_indices) { - args.extend(["--gpu".to_owned(), csv]); - } - if env_id.is_none() { - args.extend(optional_arg("--runtime-id", runtime_id)); - } - args.extend(optional_arg("--env-id", env_id)); - args.extend(engine_recipe_json_arg(engine_recipe)?); - args.extend(["--state-path".to_owned(), state_path.display().to_string()]); - if let Some(log_path) = log_path { - args.extend(["--log-path".to_owned(), log_path.display().to_string()]); - } - Ok(args) -} - fn parse_device_policy(value: Option<&str>) -> Result { match value.unwrap_or("gpu_required") { "auto" | "gpu" | "gpu_required" | "gpu_preferred" => Ok(DevicePolicy::GpuRequired), @@ -15810,42 +15456,6 @@ pub(crate) mod tests { Ok(()) } - #[test] - fn engine_plugin_discovery_finds_runtime_binary() -> Result<()> { - let (root, paths) = test_paths("engine-plugin"); - let plugin_dir = paths.primary_engine_plugin_dir(); - fs::create_dir_all(&plugin_dir)?; - let plugin_path = plugin_dir.join( - rocm_engine_protocol::platform_engine_plugin_binary_name("vllm"), - ); - fs::write(&plugin_path, "plugin")?; - - let discovered = find_engine_plugin_binary("vllm", engine_plugin_dirs(&paths))?; - let _ = fs::remove_dir_all(root); - - assert_eq!(discovered, Some(plugin_path)); - Ok(()) - } - - #[test] - fn engine_plugin_discovery_prefers_primary_plugin_dir() -> Result<()> { - let (root, paths) = test_paths("engine-plugin-precedence"); - let primary_dir = paths.primary_engine_plugin_dir(); - let compatibility_dir = paths.data_dir.join("engines"); - fs::create_dir_all(&primary_dir)?; - fs::create_dir_all(&compatibility_dir)?; - let name = rocm_engine_protocol::platform_engine_plugin_binary_name("vllm"); - let primary_path = primary_dir.join(&name); - fs::write(&primary_path, "primary")?; - fs::write(compatibility_dir.join(&name), "compatibility")?; - - let discovered = find_engine_plugin_binary("vllm", engine_plugin_dirs(&paths))?; - let _ = fs::remove_dir_all(root); - - assert_eq!(discovered, Some(primary_path)); - Ok(()) - } - #[test] fn install_sdk_survives_an_engine_install_failure() -> Result<()> { let (_root, paths) = test_paths("install-sdk-engine-failure"); diff --git a/apps/rocm/src/runtime_install_finalize.rs b/apps/rocm/src/runtime_install_finalize.rs index 01e74cf57..05c71b48c 100644 --- a/apps/rocm/src/runtime_install_finalize.rs +++ b/apps/rocm/src/runtime_install_finalize.rs @@ -23,6 +23,7 @@ use rocm_core::{ }; use rocm_engine_protocol::{EngineMethod, InstallRequest, InstallResponse}; +use crate::engine_ipc::engine_request_with_env_root; use crate::engines_cmd::{ engine_manages_own_runtime, env_root_for_engine_install, runtime_key_for_python, runtime_manifest_for_selector, @@ -31,10 +32,7 @@ use crate::runtimes_cmd::{ activate_runtime, newest_installed_runtime_manifest, recover_setup_runtime_registration, }; use crate::therock; -use crate::{ - ROLLBACK_RECOVERY_HINT, engine_request_with_env_root, record_cli_audit_event, runtime_services, - sanitize_log_value, -}; +use crate::{ROLLBACK_RECOVERY_HINT, record_cli_audit_event, runtime_services, sanitize_log_value}; #[derive(Debug, Clone)] pub(crate) struct SdkInstallFinalization { diff --git a/apps/rocm/src/serve_cmd.rs b/apps/rocm/src/serve_cmd.rs index 275115f2a..5c2fbf49c 100644 --- a/apps/rocm/src/serve_cmd.rs +++ b/apps/rocm/src/serve_cmd.rs @@ -37,6 +37,7 @@ use rocm_engine_protocol::{ use crate::DevicePolicyArg; use crate::endpoint_keys; +use crate::engine_ipc::engine_request; use crate::engines_cmd::{engine_manages_own_runtime, ensure_self_managed_engine_ready}; use crate::serve_summary; use crate::service_launch::{ @@ -45,8 +46,8 @@ use crate::service_launch::{ run_attached_service, start_managed_service, validate_bind_host, }; use crate::{ - cli_progress, collect_serve_notes, detect_gpu_count, device_policy_name, engine_request, - gpu_vram_usage, parse_device_policy, parse_gpu_selection, resolve_engine_selection, + cli_progress, collect_serve_notes, detect_gpu_count, device_policy_name, gpu_vram_usage, + parse_device_policy, parse_gpu_selection, resolve_engine_selection, select_gpu_indices_under_launch_lock, serve_gpu_low_memory_warning, validate_engine_selection_runtime, validate_pinned_gpu_index, }; diff --git a/apps/rocm/src/service_launch.rs b/apps/rocm/src/service_launch.rs index 5159d673e..6586b9a73 100644 --- a/apps/rocm/src/service_launch.rs +++ b/apps/rocm/src/service_launch.rs @@ -46,13 +46,14 @@ use rocm_core::{format_http_base_url, process_is_running}; use rocm_engine_protocol::{DevicePolicy, EngineRecipeHint, ResolveModelResponse}; use crate::endpoint_keys; +use crate::engine_ipc::builtin_engine_serve_http_args; use crate::engines_cmd::env_root_for_service; use crate::managed_service_status::existing_live_managed_service; use crate::service_hints; use crate::{ - SandboxToolArg, builtin_engine_serve_http_args, device_policy_name, - managed_service_launcher_path, record_cli_audit_event, render_service_action_result, - run_internal_sandbox_tool, status_for_readiness, wait_for_service_http_ready_with_progress, + SandboxToolArg, device_policy_name, managed_service_launcher_path, record_cli_audit_event, + render_service_action_result, run_internal_sandbox_tool, status_for_readiness, + wait_for_service_http_ready_with_progress, }; #[cfg(windows)] use crate::{app_path_env_var_refs, app_path_env_var_values}; diff --git a/docs/architecture.md b/docs/architecture.md index 76679e03b..4f2244be8 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -14,7 +14,7 @@ This is the living module map for rocm-cli. It's a contributor-facing reference New subcommands and subsystems default to their own file from day one — they should not grow inside `main.rs`/`lib.rs` waiting for a future extraction pass. Two extraction patterns already exist in the codebase; use whichever fits: -- **Full domain extraction** — a subsystem's domain implementation moves into its own file that owns its own types (structs/enums), not just relocated functions; that ownership is usually what distinguishes this pattern from mechanical relocation below, though the Phase 6a variant described next shows ownership alone doesn't fully tell them apart — every `apps/rocm` module is a private `mod x;` regardless of pattern, so module privacy alone doesn't tell the two apart either. Full-domain-extraction modules are typically reached via qualified paths (e.g. `comfyui::render_status(...)`); mechanically relocated modules with no owned types are reached the same way (e.g. [`endpoint_keys.rs`](../apps/rocm/src/endpoint_keys.rs), [`logging.rs`](../apps/rocm/src/logging.rs)). The exception — reached unqualified through `use crate::x::{...}` re-imports, matching their dispatch call sites' pre-extraction form — is limited to the clusters that moved as a dispatch-adjacent group: [`automations.rs`](../apps/rocm/src/automations.rs), [`uninstall.rs`](../apps/rocm/src/uninstall.rs), (as of Phase 6a, ROCMAI-82) [`driver_install.rs`](../apps/rocm/src/driver_install.rs), [`engines_cmd.rs`](../apps/rocm/src/engines_cmd.rs), [`serve_cmd.rs`](../apps/rocm/src/serve_cmd.rs), and (as of Phase 6e's follow-up, ROCMAI-726) [`freeform_execution.rs`](../apps/rocm/src/freeform_execution.rs). [`service_launch.rs`](../apps/rocm/src/service_launch.rs) (as of Phase 6b, ROCMAI-91) uses the same unqualified re-import form for its cross-module callers, but it is not a dispatch-adjacent cluster in the same sense — one of its callers, `automations.rs`, is not a dispatch call site, and its pre-extraction form was a qualified `crate::` call rather than an unqualified import, so the re-import is new rather than kept. This call-site form, not type ownership or dispatch-fn placement, is what actually separates the Phase 6a variant below from full domain extraction. Where a subsystem has a dedicated clap subcommand, its command *enum* usually stays in `main.rs` as part of the `Command`/`Cli` definition (e.g. `ComfyuiCommand`, `RuntimesCommand`) — but the dispatch function itself does not always stay with it: `comfyui()` does, while `engines()` (Phase 6a) and `runtimes()` (Phase 6c, ROCMAI-89) moved out to `engines_cmd.rs`/`runtimes_cmd.rs` respectively, following the mechanical-relocation re-import form below rather than full domain extraction's qualified-path form. Not every domain-extracted module has a dedicated command enum at all (`providers.rs` has none; it's invoked from the existing chat/config command flows). In library crates (`crates/rocm-core`) the module is `pub mod x;` plus a `pub use x::{...};` re-export, since it's part of the crate's public API. This is the default for new subsystems. Examples: [`apps/rocm/src/therock.rs`](../apps/rocm/src/therock.rs) (`RuntimesCommand`'s domain types), [`comfyui.rs`](../apps/rocm/src/comfyui.rs) (`ComfyuiCommand`), [`providers.rs`](../apps/rocm/src/providers.rs) (no dedicated command enum); `crates/rocm-core`'s [`diagnose.rs`](../crates/rocm-core/src/diagnose.rs)/[`examine.rs`](../crates/rocm-core/src/examine.rs). +- **Full domain extraction** — a subsystem's domain implementation moves into its own file that owns its own types (structs/enums), not just relocated functions; that ownership is usually what distinguishes this pattern from mechanical relocation below, though the Phase 6a variant described next shows ownership alone doesn't fully tell them apart — every `apps/rocm` module is a private `mod x;` regardless of pattern, so module privacy alone doesn't tell the two apart either. Full-domain-extraction modules are typically reached via qualified paths (e.g. `comfyui::render_status(...)`); mechanically relocated modules with no owned types are reached the same way (e.g. [`endpoint_keys.rs`](../apps/rocm/src/endpoint_keys.rs), [`logging.rs`](../apps/rocm/src/logging.rs)). The exception — reached unqualified through `use crate::x::{...}` re-imports, matching their dispatch call sites' pre-extraction form — is limited to the clusters that moved as a dispatch-adjacent group: [`automations.rs`](../apps/rocm/src/automations.rs), [`uninstall.rs`](../apps/rocm/src/uninstall.rs), (as of Phase 6a, ROCMAI-82) [`driver_install.rs`](../apps/rocm/src/driver_install.rs), [`engines_cmd.rs`](../apps/rocm/src/engines_cmd.rs), [`serve_cmd.rs`](../apps/rocm/src/serve_cmd.rs), (as of Phase 6e's follow-up, ROCMAI-726) [`freeform_execution.rs`](../apps/rocm/src/freeform_execution.rs), and (as of Phase 6f, ROCMAI-213) [`engine_ipc.rs`](../apps/rocm/src/engine_ipc.rs). [`service_launch.rs`](../apps/rocm/src/service_launch.rs) (as of Phase 6b, ROCMAI-91) uses the same unqualified re-import form for its cross-module callers, but it is not a dispatch-adjacent cluster in the same sense — one of its callers, `automations.rs`, is not a dispatch call site, and its pre-extraction form was a qualified `crate::` call rather than an unqualified import, so the re-import is new rather than kept. This call-site form, not type ownership or dispatch-fn placement, is what actually separates the Phase 6a variant below from full domain extraction. Where a subsystem has a dedicated clap subcommand, its command *enum* usually stays in `main.rs` as part of the `Command`/`Cli` definition (e.g. `ComfyuiCommand`, `RuntimesCommand`) — but the dispatch function itself does not always stay with it: `comfyui()` does, while `engines()` (Phase 6a) and `runtimes()` (Phase 6c, ROCMAI-89) moved out to `engines_cmd.rs`/`runtimes_cmd.rs` respectively, following the mechanical-relocation re-import form below rather than full domain extraction's qualified-path form. Not every domain-extracted module has a dedicated command enum at all (`providers.rs` has none; it's invoked from the existing chat/config command flows). In library crates (`crates/rocm-core`) the module is `pub mod x;` plus a `pub use x::{...};` re-export, since it's part of the crate's public API. This is the default for new subsystems. Examples: [`apps/rocm/src/therock.rs`](../apps/rocm/src/therock.rs) (`RuntimesCommand`'s domain types), [`comfyui.rs`](../apps/rocm/src/comfyui.rs) (`ComfyuiCommand`), [`providers.rs`](../apps/rocm/src/providers.rs) (no dedicated command enum); `crates/rocm-core`'s [`diagnose.rs`](../crates/rocm-core/src/diagnose.rs)/[`examine.rs`](../crates/rocm-core/src/examine.rs). - **Mechanical relocation** — a `pub(crate) fn` moves out verbatim, with shared types/config staying at the crate root and reached via `crate::`. Used for dispatch-adjacent clusters where a minimal, easy-to-review diff matters more than full extraction. Example: [`apps/rocm/src/automations.rs`](../apps/rocm/src/automations.rs). Variant: when the cluster is large enough to own private types (part of `main.rs`'s Phase 6a split, ROCMAI-82, `main.rs`'s Phase 6b split, ROCMAI-91, `main.rs`'s Phase 6c split, ROCMAI-89, and Phase 6e's follow-up, ROCMAI-726), those types move with it instead of staying in `main.rs` — [`engines_cmd.rs`](../apps/rocm/src/engines_cmd.rs)'s types are used nowhere else, but [`driver_install.rs`](../apps/rocm/src/driver_install.rs)'s `DriverInstallResult`/`DriverInstallError` are `pub(crate)` and read from `main.rs`'s `install()` (`result.output`/`result.executed`, `error.source`/`error.executed`); [`freeform_execution.rs`](../apps/rocm/src/freeform_execution.rs)'s `FreeformExecution` is `pub(crate)` and read from `main.rs`'s `execute_freeform_next_action()` (`execution.action`) — it was already `pub(crate)` before the split, unlike the two functions that return/consume it, which widened from module-private; [`serve_cmd.rs`](../apps/rocm/src/serve_cmd.rs)'s `ServeEngineSelection`/`ServeArgs` both stay `pub(crate)` and are both used from `main.rs` — `assess_model_for_host` calls `select_serve_engine` and reads the resulting `ServeEngineSelection`'s `.engine`/`.source` fields — and `ServeGenerationDefaults` is private; [`service_launch.rs`](../apps/rocm/src/service_launch.rs)'s `AttachOutcome`/`RawModeGuard`/`ManagedSpawn`/`AutostartClaim` are private, and its `ManagedLaunchReport` is `pub(crate)` and read from [`serve_cmd.rs`](../apps/rocm/src/serve_cmd.rs) for the interactive summary; [`runtimes_cmd.rs`](../apps/rocm/src/runtimes_cmd.rs)'s `RuntimeActivationResult` and `ActiveRuntimeMarker` are `pub(crate)` and read from `main.rs`'s `apply_runtime_update`/`append_update_activate_summary` and from [`storage.rs`](../apps/rocm/src/storage.rs) respectively; its sibling [`runtime_install_finalize.rs`](../apps/rocm/src/runtime_install_finalize.rs)'s `SdkInstallConsents` is `pub(crate)` and read from `main.rs`'s `install()`. The pattern is still "mechanical relocation" rather than full domain extraction because call sites keep their pre-extraction unqualified-re-import form — the exception named above — rather than switching to the qualified paths full-domain-extraction modules use. The top-level dispatch fns (`install()`/`dispatch()`) staying in `main.rs` is not what makes it so: full domain extraction usually keeps its dispatch fn in `main.rs` too (see `comfyui()` above). There is no file-line-count CI gate enforcing this — `too_many_lines = "allow"` in the workspace `Cargo.toml` is a deliberate, function-level choice, not an oversight. This convention is the guardrail instead. @@ -40,6 +40,7 @@ Each per-crate module inventory lists one file per line. Flat inventories (every - [`dash_seam.rs`](../apps/rocm/src/dash_seam.rs) — full domain extraction. - [`driver_install.rs`](../apps/rocm/src/driver_install.rs) — mechanical relocation, owns private types (part of [`main.rs`](../apps/rocm/src/main.rs)'s Phase 6a split, ROCMAI-82; see the Phase 6a variant note above): `install_driver()`/`reconcile_driver_install()`, plus the driver-plan/state types. - [`endpoint_keys.rs`](../apps/rocm/src/endpoint_keys.rs) — mechanical relocation, dispatch-adjacent, no owned types. +- [`engine_ipc.rs`](../apps/rocm/src/engine_ipc.rs) — mechanical relocation, owns private types (Phase 6f, ROCMAI-213): resolving an engine's binary path (`resolve_engine_binary_path`, `resolve_engine_binary_path_with_paths`, `find_engine_plugin_binary`), the stdio request/response envelope used to talk to an external engine binary or the in-process built-in handler (`engine_request`, `engine_request_with_env_root`, `decode_engine_response`, the `ScopedEnvVar`/`with_scoped_builtin_engine_env` env-scoping helpers and the `BUILTIN_ENGINE_ENV_LOCK` mutex they serialize on, `builtin_engine_request`, `run_builtin_engine_stdio`), and the built-in HTTP-serving path (`builtin_engine_available`, `run_builtin_engine_serve_http`, `builtin_engine_serve_http_args`). Two items that sit physically inside this span in `main.rs` stayed there instead of moving: `missing_packaged_engine_reason` has callers outside the cluster (plus its own test), and `impl From for WatcherMode` is unrelated clap-arg conversion that merely happened to be interleaved between `find_engine_plugin_binary` and `engine_request`. `resolve_engine_binary_path`, `resolve_engine_binary_path_with_paths`, `engine_request`, `builtin_engine_available`, and `builtin_engine_serve_http_args` are `pub(crate)` and read from `main.rs`; `engine_request` is also read from [`engines_cmd.rs`](../apps/rocm/src/engines_cmd.rs) and [`serve_cmd.rs`](../apps/rocm/src/serve_cmd.rs); `engine_request_with_env_root` is `pub(crate)` and read from [`engines_cmd.rs`](../apps/rocm/src/engines_cmd.rs) and [`runtime_install_finalize.rs`](../apps/rocm/src/runtime_install_finalize.rs) (not `main.rs`); `builtin_engine_serve_http_args` is also read from [`service_launch.rs`](../apps/rocm/src/service_launch.rs). `run_builtin_engine_stdio` and `run_builtin_engine_serve_http` are `pub(crate)` and reached directly from `main.rs`'s `dispatch()` (the `Command::EngineStdio` and `__engine-serve-http` arms); `find_engine_plugin_binary`, `decode_engine_response`, `with_scoped_builtin_engine_env`, `builtin_engine_request`, and `ScopedEnvVar` have no callers outside this module and stay private. - [`engines_cmd.rs`](../apps/rocm/src/engines_cmd.rs) — mechanical relocation, owns private types (Phase 6a): `engines()`, plus env-root/runtime resolution — several helpers here are also called from [`serve_cmd.rs`](../apps/rocm/src/serve_cmd.rs) and from other root-level commands, so they stay `pub(crate)`. - [`freeform_execution.rs`](../apps/rocm/src/freeform_execution.rs) — mechanical relocation, dispatch-adjacent, owns a `pub(crate)` type (as of Phase 6e's follow-up, ROCMAI-726; see the Phase 6a variant note above for the owned-types pattern). Holds the freeform natural-language planner's plan-*execution*/consent half that [`freeform_planner.rs`](../apps/rocm/src/freeform_planner.rs) below deliberately left out: `validate_freeform_execution_action()`, `prepare_freeform_execution()`, `apply_freeform_execution_consent()`, `render_freeform_execution_header()`, and the `pub(crate)` `FreeformExecution` struct that `prepare_freeform_execution` returns and `render_freeform_execution_header` consumes. `main.rs`'s `execute_freeform_next_action()` and `run_freeform()` stay in `main.rs` — both are dispatch-level, not part of deciding whether a plan's next action is safe to execute — and `execute_freeform_next_action()` calls in via the unqualified re-import `use crate::freeform_execution::{prepare_freeform_execution, render_freeform_execution_header};` (`run_freeform()` only calls `execute_freeform_next_action()`, never this module directly), matching [`driver_install.rs`](../apps/rocm/src/driver_install.rs)'s dispatch-adjacent form rather than `freeform_planner.rs`'s own qualified-path form. `prepare_freeform_execution` takes the already-built `plan` by value rather than rebuilding it from `request`/`paths`/`config` (ROCMAI-725's single-build guarantee, inherited unchanged from `main.rs`); `prepare_freeform_execution`/`render_freeform_execution_header` are `pub(crate)` for that cross-module call; `FreeformExecution` was already `pub(crate)` (read directly by `main.rs` and this module's own tests); `validate_freeform_execution_action`/`apply_freeform_execution_consent` have no caller outside this module and stay module-private, unchanged from before the move. The cluster's six regression tests moved out of `main.rs`'s shared `mod tests` into this module's own `mod tests`, inheriting the qualified `freeform_planner::` path they already used to reach `freeform_planner.rs`'s items as `main.rs`'s former tests — this module reaches in from outside using the same qualified path `main.rs` and `providers.rs`'s tests use (unlike `chat.rs`/`comfyui.rs`, which import it unqualified). - [`freeform_planner.rs`](../apps/rocm/src/freeform_planner.rs) — full domain extraction (Phase 6e, ROCMAI-87). Owns the freeform natural-language planner cluster's types (`StructuredRequestPlan`, `PlannerIntent`, `PlannedToolCall`, `FreeformPlanAction`, `ProviderPlannerResponse`, `ProviderPlannerToolCall`, all module-private except `FreeformPlanAction` and the fields of `StructuredRequestPlan`/`PlannedToolCall` that `main.rs` reads directly), the deterministic plan builder (`build_freeform_plan*`), the provider-assisted path (`resolve_freeform_plan_with_provider`, `build_provider_planner_prompt`, `provider_planner_response_to_plan`, `validate_provider_planner_tool_call`), and plan rendering (`render_structured_request_plan`). [`main.rs`](../apps/rocm/src/main.rs) calls `build_freeform_plan_with_context`/`render_structured_request_plan` directly for the dispatch/review flow (`main.rs`'s only other call into this module, `render_launch_summary` → `host_default_engine`, is unrelated: bare `rocm`'s non-interactive launch summary), while [`freeform_execution.rs`](../apps/rocm/src/freeform_execution.rs) above calls `plan_next_action`/`format_structured_tool_call` for the execution flow and reads `FreeformPlanAction`/`StructuredRequestPlan` by type for its execution-validation functions — those live there, not here, because they decide whether a plan's next action is safe to *execute*, a dispatch-adjacent concern distinct from *building* the plan; `main.rs`'s `run_freeform` builds a plan exactly once via `build_freeform_plan_with_context` and threads that same value through `execute_freeform_next_action` to `prepare_freeform_execution` rather than rebuilding it (ROCMAI-725: building a plan can call a configured cloud `planner_provider`, which is not guaranteed deterministic, so a second independent build could silently disagree with the first). [`chat.rs`](../apps/rocm/src/chat.rs)'s `natural_language_plan` MCP tool gets the same single-build guarantee from `plan_freeform_request_with_context`, which builds once and returns both the rendered text and the next action; `chat.rs` also calls `format_structured_tool_call` directly, like [`comfyui.rs`](../apps/rocm/src/comfyui.rs) does, and its `validate_chat_rocm_command_safety` reads `serve_args_request_cpu_device` directly as a no-CPU-fallback safety check on provider-proposed `serve` args. `host_default_engine`, `build_freeform_plan_with_context`, `render_structured_request_plan`, `plan_next_action`, `plan_freeform_request_with_context`, `provider_planner_response_to_plan`, `format_structured_tool_call`, and `serve_args_request_cpu_device` are `pub(crate)` for these reasons; `freeform_plan_next_action` and `render_freeform_plan` are test-only (`#[cfg(test)]`) and `FreeformPlanAction` was already `pub(crate)` before the split. `parse_freeform_invocation`/`should_treat_as_freeform` (deciding whether an invocation is freeform at all) stay in `main.rs`, a dispatch-level concern outside both this module and `freeform_execution.rs`. The cluster's regression tests live in `freeform_planner.rs`'s own `mod tests`; the handful of `main.rs` tests that exercise the invocation classifier/clap-suggestion-avoidance stay in `main.rs`'s shared `mod tests`, since neither is specific to this module or to `freeform_execution.rs` (and neither touches `freeform_planner::` at all), and the ROCMAI-725 regression tests proving the single-build guarantee live in `providers.rs`'s `mod tests` (reusing its mock local-provider server), reaching this module's items through the qualified `freeform_planner::` path like any other external caller. diff --git a/xtask/src/env_mutation_contract.rs b/xtask/src/env_mutation_contract.rs index d035f8042..0a2dd92fb 100644 --- a/xtask/src/env_mutation_contract.rs +++ b/xtask/src/env_mutation_contract.rs @@ -116,10 +116,11 @@ mod tests { /// Named helpers that serialize env mutation for their whole scope. /// /// Membership is "holds a lock for its whole scope", not "wraps an env - /// mutation". `ScopedEnvVar` (`apps/rocm/src/main.rs`) was listed here and + /// mutation". `ScopedEnvVar` (`apps/rocm/src/engine_ipc.rs` as of + /// ROCMAI-213; originally `apps/rocm/src/main.rs`) was listed here and /// only ever saved and restored — its one caller, /// `with_scoped_builtin_engine_env`, takes `BUILTIN_ENGINE_ENV_LOCK` - /// itself. The type is visible to the largest test module in the tree, in + /// itself. The type was visible to the largest test module in the tree, in /// the same file, so any test that merely named it was granted the full /// exemption while holding nothing: the exact failure this guard exists to /// prevent, written into the guard.