From d85f709bfe49bf320971ca5ce19ced40c34f8ef5 Mon Sep 17 00:00:00 2001 From: David Date: Thu, 16 Jul 2026 20:52:21 -0700 Subject: [PATCH] =?UTF-8?q?feat(goal):=20/config=20skeptic-local=20?= =?UTF-8?q?=E2=80=94=20auto-managed=20local=20model=20for=20the=20skeptic?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turn the /goal skeptic review over to a small local model with one command, building on the opt-in skeptic_endpoint from #110. `/config skeptic-local on` now does the whole setup automatically: - detect the machine's local-inference backend (Apple-Silicon MLX or NVIDIA CUDA); no backend → clear message, skeptic stays on the main model - resolve a small default review model (~3B, 4-bit), overridable via HI_SKEPTIC_LOCAL_REPO / _MODEL_ID / _GGUF - download it if it isn't cached, reusing the same .hi/models layout as `/hf run --mlx` so a model fetched either way is shared - launch `hi-local serve` and wait for /health, then point skeptic_endpoint/skeptic_model at it and rebuild the skeptic provider - `/config skeptic-local off` stops the server and restores prior settings Every step degrades gracefully: a missing backend, missing binary, failed download, or unhealthy server returns an error and leaves the skeptic on the main provider — config is only mutated after the server is healthy. The server is torn down on session exit (CLI via kill_background_processes, TUI via a drop guard). The blocking, progress-to-terminal download runs only on the plain CLI; the TUI reports NeedsDownload rather than corrupting its alternate screen. New: hi-tools `local_server` (spawn + health, private registry) and hi-agent `local_skeptic` (backend/model policy + Agent enable/disable). Deterministic core is unit-tested; the live download+spawn is hardware-validated. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/hi-agent/src/agent/lifecycle.rs | 17 +- crates/hi-agent/src/command.rs | 22 +- crates/hi-agent/src/lib.rs | 7 + crates/hi-agent/src/local_skeptic.rs | 354 +++++++++++++++++++++ crates/hi-agent/src/tests.rs | 1 + crates/hi-agent/src/tests/local_skeptic.rs | 141 ++++++++ crates/hi-cli/src/commands.rs | 46 ++- crates/hi-cli/src/repl.rs | 9 + crates/hi-tools/src/hf.rs | 2 +- crates/hi-tools/src/lib.rs | 5 + crates/hi-tools/src/local_server.rs | 102 ++++++ crates/hi-tui/src/app/commands.rs | 40 +++ crates/hi-tui/src/app/run.rs | 13 + scripts/file_size_baseline.txt | 10 +- 14 files changed, 751 insertions(+), 18 deletions(-) create mode 100644 crates/hi-agent/src/local_skeptic.rs create mode 100644 crates/hi-agent/src/tests/local_skeptic.rs create mode 100644 crates/hi-tools/src/local_server.rs diff --git a/crates/hi-agent/src/agent/lifecycle.rs b/crates/hi-agent/src/agent/lifecycle.rs index c18315c8..a2be83c6 100644 --- a/crates/hi-agent/src/agent/lifecycle.rs +++ b/crates/hi-agent/src/agent/lifecycle.rs @@ -108,18 +108,13 @@ impl crate::Agent { model: config.model.clone(), }; // Opt-in: route the skeptic review to a separate OpenAI-compatible - // endpoint (e.g. a local hi-local server) when configured. - let skeptic_provider: Option> = - config.skeptic_endpoint.as_deref().map(|url| { - let key = config - .skeptic_endpoint_key - .clone() - .unwrap_or_else(|| "local".to_string()); - Arc::new(hi_ai::OpenAiProvider::new(url.to_string(), key)) as Arc - }); + // endpoint (e.g. a local hi-local server) when configured. Shared with + // the runtime `/config skeptic-local` toggle so their wiring can't drift. + let skeptic_provider = crate::local_skeptic::build_skeptic_provider(&config); Ok(Self { provider, skeptic_provider, + local_skeptic: None, config, runtime, task_context: None, @@ -733,9 +728,11 @@ impl crate::Agent { self.runtime.background().kill_started_after(before) } - /// Stop every background process owned by this agent runtime. + /// Stop every background process owned by this agent runtime, plus any + /// auto-managed local skeptic server, on session shutdown. pub fn kill_background_processes(&self) { self.runtime.background().kill_all(); + self.stop_local_skeptic_server(); } /// Finalize a turn whose future was cancelled by its frontend. Reconcile diff --git a/crates/hi-agent/src/command.rs b/crates/hi-agent/src/command.rs index 8d9dbd4c..767eb6dc 100644 --- a/crates/hi-agent/src/command.rs +++ b/crates/hi-agent/src/command.rs @@ -605,6 +605,11 @@ pub enum ConfigArg { /// `On` forces streaming, `Off` forces resident, `Auto` (the default) lets /// the loader auto-enable when the model exceeds the memory budget. MoeStreaming(MoeStreamingMode), + /// `/config skeptic-local ` — turn the auto-managed local model for + /// the `/goal` skeptic review on or off. `on` detects the machine's backend, + /// downloads a small default review model if needed, and spawns a local + /// server; `off` stops it and restores the prior skeptic settings. + SkepticLocal(bool), /// Unrecognized option or bad value; carries a usage/error hint. Invalid(String), } @@ -720,12 +725,27 @@ pub fn parse_config_arg(arg: &str) -> ConfigArg { } } } + "skeptic-local" | "local-skeptic" => match val.to_ascii_lowercase().as_str() { + "" => ConfigArg::Invalid("usage: /config skeptic-local ".into()), + "on" | "enable" | "enabled" | "1" | "true" | "yes" => ConfigArg::SkepticLocal(true), + "off" | "disable" | "disabled" | "0" | "false" | "no" => ConfigArg::SkepticLocal(false), + _ => ConfigArg::Invalid(format!( + "unknown skeptic-local mode '{val}' — use on or off" + )), + }, other => ConfigArg::Invalid(format!( - "unknown /config option '{other}' — try: show, reasoning , temp , steps , moe-streaming " + "unknown /config option '{other}' — try: show, reasoning , temp , steps , moe-streaming , skeptic-local " )), } } +/// Whether a `/config` argument is the async `skeptic-local` toggle. The CLI +/// routes this through its async handler (it may download a model and spawn a +/// server) rather than the synchronous `/config` path. +pub fn config_is_skeptic_local(arg: &str) -> bool { + matches!(parse_config_arg(arg), ConfigArg::SkepticLocal(_)) +} + fn read_only_macro_prompt(kind: &str, topic: &str) -> String { let topic = topic.trim(); let topic = if topic.is_empty() { diff --git a/crates/hi-agent/src/lib.rs b/crates/hi-agent/src/lib.rs index e10c2dd8..42b536a0 100644 --- a/crates/hi-agent/src/lib.rs +++ b/crates/hi-agent/src/lib.rs @@ -10,6 +10,7 @@ mod context_index; mod decision; mod goal; mod heuristics; +pub mod local_skeptic; mod memory; mod outcome; mod prompt; @@ -37,6 +38,7 @@ pub use config::{ }; pub use heuristics::humanize_count; pub use hi_tools::{PlanStatus, PlanStep}; +pub use local_skeptic::LocalSkepticOutcome; pub use memory::{ AnnotatedBullet, global_memory_file, memory_file, read_global_memory, read_memory, read_project_annotated, should_distill_memory, @@ -557,6 +559,11 @@ pub struct Agent { /// as it always has. Lets the frequent, fail-open review loop run on a local /// model while the driver stays on the session model. pub(crate) skeptic_provider: Option>, + /// Session state for an auto-managed local skeptic server started by + /// `/config skeptic-local on` (`None` when off). Held so the server can be + /// stopped, the prior skeptic settings restored, and the process killed on + /// session shutdown. + pub(crate) local_skeptic: Option, pub(crate) config: AgentConfig, pub(crate) runtime: WorkspaceRuntime, /// Per-turn ranked repository data and scoped instructions. diff --git a/crates/hi-agent/src/local_skeptic.rs b/crates/hi-agent/src/local_skeptic.rs new file mode 100644 index 00000000..965c114a --- /dev/null +++ b/crates/hi-agent/src/local_skeptic.rs @@ -0,0 +1,354 @@ +//! Auto-managed local model for the `/goal` skeptic review (`/config +//! skeptic-local on`). +//! +//! The skeptic gate runs a bounded, fail-open critique call before a turn may +//! advance a sub-goal. It fires often, so routing it to a small local model +//! keeps the coding driver and planner on the main model while making the review +//! free and private. Turning the feature on detects the machine's +//! local-inference backend (Apple-Silicon MLX or NVIDIA CUDA), fetches a small +//! default review model if it isn't already cached, launches a `hi-local` +//! server, waits for it to become healthy, and points +//! `skeptic_endpoint`/`skeptic_model` at it. Every step degrades gracefully: a +//! missing backend, missing binary, failed download, or unhealthy server leaves +//! the skeptic on the main provider and reports why. + +use crate::Agent; +use anyhow::{Context, Result, bail}; +use hi_ai::Provider; +use std::path::{Path, PathBuf}; +use std::sync::Arc; + +/// A local-inference backend that `hi-local serve` can drive. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum LocalBackend { + /// Apple-Silicon MLX. Serves a model *directory*. + Mlx, + /// NVIDIA CUDA. Serves a single GGUF *file*. + Cuda, +} + +impl LocalBackend { + /// The `--backend` value for `hi-local serve`. + pub fn serve_flag(self) -> &'static str { + match self { + LocalBackend::Mlx => "mlx", + LocalBackend::Cuda => "cuda", + } + } +} + +/// Choose a backend from probed hardware facts. Pure so it can be unit tested; +/// the environment probe is [`detect_backend`]. MLX wins on Apple Silicon; +/// otherwise CUDA when an NVIDIA runtime is present; otherwise none. +pub fn pick_backend(is_apple_silicon: bool, has_nvidia: bool) -> Option { + if is_apple_silicon { + Some(LocalBackend::Mlx) + } else if has_nvidia { + Some(LocalBackend::Cuda) + } else { + None + } +} + +/// Probe the host for a usable local backend. +pub fn detect_backend() -> Option { + let is_apple_silicon = cfg!(all(target_os = "macos", target_arch = "aarch64")); + let has_nvidia = !is_apple_silicon && nvidia_present(); + pick_backend(is_apple_silicon, has_nvidia) +} + +fn nvidia_present() -> bool { + // `nvidia-smi` on PATH is the cheapest reliable signal a CUDA runtime exists. + std::process::Command::new("nvidia-smi") + .arg("-L") + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .map(|status| status.success()) + .unwrap_or(false) +} + +/// A default local review model for a backend. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct LocalModelSpec { + /// HuggingFace repo id to fetch when the weights are absent. + pub repo: String, + /// Model id the server advertises over the OpenAI API (the skeptic model). + pub model_id: String, + /// For CUDA/GGUF, the single weight filename to serve inside the repo. + /// `None` for MLX, where the whole downloaded directory is the model. + pub gguf_file: Option, + /// The backend this spec targets. + pub backend: LocalBackend, +} + +/// The bundled default review model for a backend: a ~3B instruct model, 4-bit +/// quantized — strong enough to catch premature "done", small enough to run +/// beside the coding model. +pub fn default_model(backend: LocalBackend) -> LocalModelSpec { + match backend { + LocalBackend::Mlx => LocalModelSpec { + repo: "mlx-community/Qwen2.5-3B-Instruct-4bit".to_string(), + model_id: "Qwen2.5-3B-Instruct-4bit".to_string(), + gguf_file: None, + backend, + }, + LocalBackend::Cuda => LocalModelSpec { + repo: "Qwen/Qwen2.5-3B-Instruct-GGUF".to_string(), + model_id: "qwen2.5-3b-instruct".to_string(), + gguf_file: Some("qwen2.5-3b-instruct-q4_k_m.gguf".to_string()), + backend, + }, + } +} + +/// [`default_model`] overlaid with any `HI_SKEPTIC_LOCAL_*` env overrides, so a +/// user can point the skeptic at their own local model. +pub fn resolve_model(backend: LocalBackend) -> LocalModelSpec { + let env = |key: &str| { + std::env::var(key) + .ok() + .map(|v| v.trim().to_string()) + .filter(|v| !v.is_empty()) + }; + let mut spec = default_model(backend); + let id_override = env("HI_SKEPTIC_LOCAL_MODEL_ID"); + if let Some(repo) = env("HI_SKEPTIC_LOCAL_REPO") { + // A fresh repo without an explicit id defaults to the repo's last path + // segment, which is what MLX servers advertise for a model directory. + if id_override.is_none() { + spec.model_id = repo.rsplit('/').next().unwrap_or(&repo).to_string(); + } + spec.repo = repo; + } + if let Some(id) = id_override { + spec.model_id = id; + } + if let Some(file) = env("HI_SKEPTIC_LOCAL_GGUF") { + spec.gguf_file = Some(file); + } + spec +} + +/// Whether the model's weights are already cached in `dir`. +pub fn model_present(dir: &Path, spec: &LocalModelSpec) -> bool { + match &spec.gguf_file { + // MLX: a loadable model directory carries a config.json (matches `/hf`). + None => dir.join("config.json").exists(), + // CUDA: the specific GGUF file must be on disk. + Some(file) => dir.join(file).exists(), + } +} + +/// The path passed to `hi-local serve`: the model *directory* for MLX, the GGUF +/// *file* for CUDA. +pub fn serve_model_path(dir: &Path, spec: &LocalModelSpec) -> PathBuf { + match &spec.gguf_file { + None => dir.to_path_buf(), + Some(file) => dir.join(file), + } +} + +/// The OpenAI-compatible base URL for a served model. +pub fn endpoint_url(host: &str, port: u16) -> String { + format!("http://{host}:{port}/v1") +} + +/// Build the `hi-local serve …` argument vector. +pub fn serve_args(model_path: &Path, spec: &LocalModelSpec, host: &str, port: u16) -> Vec { + vec![ + "serve".to_string(), + model_path.to_string_lossy().into_owned(), + "--backend".to_string(), + spec.backend.serve_flag().to_string(), + "--host".to_string(), + host.to_string(), + "--port".to_string(), + port.to_string(), + "--model-id".to_string(), + spec.model_id.clone(), + ] +} + +/// Locate the `hi-local` binary: `$HI_LOCAL_BIN`, else a sibling of the current +/// executable, else the bare name resolved on `PATH` at spawn. +pub fn find_hi_local() -> PathBuf { + if let Some(path) = std::env::var_os("HI_LOCAL_BIN") { + return PathBuf::from(path); + } + if let Ok(current) = std::env::current_exe() + && let Some(dir) = current.parent() + { + let sibling = dir.join(format!("hi-local{}", std::env::consts::EXE_SUFFIX)); + if sibling.exists() { + return sibling; + } + } + PathBuf::from("hi-local") +} + +/// Pick a free localhost port for the server, honoring `HI_SKEPTIC_LOCAL_PORT` +/// as the starting point (default 8080). +fn pick_free_port() -> u16 { + let start = std::env::var("HI_SKEPTIC_LOCAL_PORT") + .ok() + .and_then(|v| v.trim().parse::().ok()) + .unwrap_or(8080); + let end = start.saturating_add(64); + for port in start..=end { + if std::net::TcpListener::bind(("127.0.0.1", port)).is_ok() { + return port; + } + } + start +} + +/// Outcome of turning the local skeptic on. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum LocalSkepticOutcome { + /// The server is up and the skeptic now routes to `endpoint`. + Ready { endpoint: String, model_id: String }, + /// Weights aren't cached and an inline download wasn't allowed (the TUI). + /// The caller should fetch `repo` into `dir` once, then re-run. + NeedsDownload { repo: String, dir: PathBuf }, + /// No local-inference backend was detected on this machine. + NoBackend, +} + +/// Session state for an active local skeptic, kept so it can be torn down and +/// the prior skeptic settings restored on `/config skeptic-local off`. +pub(crate) struct LocalSkepticState { + pub(crate) process_id: String, + pub(crate) endpoint: String, + pub(crate) model_id: String, + prev_skeptic_model: Option, + prev_endpoint: Option, + prev_endpoint_key: Option, +} + +/// Build the optional skeptic provider from an endpoint config. Shared by the +/// constructor and the runtime toggle so their wiring can't drift. +pub(crate) fn build_skeptic_provider(config: &crate::AgentConfig) -> Option> { + config.skeptic_endpoint.as_deref().map(|url| { + let key = config + .skeptic_endpoint_key + .clone() + .unwrap_or_else(|| "local".to_string()); + Arc::new(hi_ai::OpenAiProvider::new(url.to_string(), key)) as Arc + }) +} + +impl Agent { + /// Whether an auto-managed local skeptic is currently running, and at what + /// endpoint. + pub fn local_skeptic_endpoint(&self) -> Option<&str> { + self.local_skeptic.as_ref().map(|s| s.endpoint.as_str()) + } + + /// Rebuild [`Agent::skeptic_provider`] from the current config after the + /// skeptic endpoint changes at runtime. + pub(crate) fn rebuild_skeptic_provider(&mut self) { + self.skeptic_provider = build_skeptic_provider(&self.config); + } + + /// Turn the auto-managed local skeptic on: detect a backend, fetch the + /// default review model if needed, launch `hi-local`, wait for health, and + /// route the skeptic review to it. `allow_download` gates the blocking, + /// progress-to-terminal model fetch — the plain CLI passes `true`; the TUI + /// passes `false` (a multi-GB download would corrupt its alternate screen) + /// and gets [`LocalSkepticOutcome::NeedsDownload`] when the model is absent. + /// + /// Idempotent: a second call while already on just reports `Ready`. On any + /// failure it returns `Err` and leaves the skeptic on the main provider. + pub async fn enable_local_skeptic( + &mut self, + allow_download: bool, + ) -> Result { + if let Some(state) = &self.local_skeptic { + return Ok(LocalSkepticOutcome::Ready { + endpoint: state.endpoint.clone(), + model_id: state.model_id.clone(), + }); + } + let Some(backend) = detect_backend() else { + return Ok(LocalSkepticOutcome::NoBackend); + }; + let spec = resolve_model(backend); + let dir = hi_tools::skeptic_model_dir(&spec.repo); + if !model_present(&dir, &spec) { + if !allow_download { + return Ok(LocalSkepticOutcome::NeedsDownload { + repo: spec.repo.clone(), + dir, + }); + } + hi_tools::download_repo_keep_foreground(&spec.repo, &dir) + .await + .with_context(|| format!("downloading local skeptic model {}", spec.repo))?; + if !model_present(&dir, &spec) { + bail!( + "downloaded {} but its weights are still missing under {}", + spec.repo, + dir.display() + ); + } + } + let abs_dir = std::fs::canonicalize(&dir).unwrap_or(dir); + let model_path = serve_model_path(&abs_dir, &spec); + let bin = find_hi_local(); + let host = "127.0.0.1"; + let port = pick_free_port(); + let args = serve_args(&model_path, &spec, host, port); + let handle = hi_tools::start_local_server(&bin, &args, host, port) + .await + .with_context(|| { + format!( + "starting hi-local ({}) — is the `hi-local` binary built with the {} backend?", + bin.display(), + spec.backend.serve_flag() + ) + })?; + + let prev_skeptic_model = self.config.skeptic_model.clone(); + let prev_endpoint = self.config.skeptic_endpoint.clone(); + let prev_endpoint_key = self.config.skeptic_endpoint_key.clone(); + self.config.skeptic_endpoint = Some(handle.endpoint.clone()); + self.config.skeptic_endpoint_key = Some("local".to_string()); + self.config.skeptic_model = Some(spec.model_id.clone()); + self.rebuild_skeptic_provider(); + self.local_skeptic = Some(LocalSkepticState { + process_id: handle.process_id, + endpoint: handle.endpoint.clone(), + model_id: spec.model_id.clone(), + prev_skeptic_model, + prev_endpoint, + prev_endpoint_key, + }); + Ok(LocalSkepticOutcome::Ready { + endpoint: handle.endpoint, + model_id: spec.model_id, + }) + } + + /// Turn the local skeptic off: stop the server and restore the prior skeptic + /// settings. Returns whether one was running. + pub fn disable_local_skeptic(&mut self) -> bool { + let Some(state) = self.local_skeptic.take() else { + return false; + }; + hi_tools::stop_local_server(&state.process_id); + self.config.skeptic_model = state.prev_skeptic_model; + self.config.skeptic_endpoint = state.prev_endpoint; + self.config.skeptic_endpoint_key = state.prev_endpoint_key; + self.rebuild_skeptic_provider(); + true + } + + /// Stop any auto-managed local skeptic server without touching config, for + /// session shutdown. Called from [`Agent::kill_background_processes`]. + pub(crate) fn stop_local_skeptic_server(&self) { + if let Some(state) = &self.local_skeptic { + hi_tools::stop_local_server(&state.process_id); + } + } +} diff --git a/crates/hi-agent/src/tests.rs b/crates/hi-agent/src/tests.rs index e10b66a5..f640c90e 100644 --- a/crates/hi-agent/src/tests.rs +++ b/crates/hi-agent/src/tests.rs @@ -12,6 +12,7 @@ mod explore; mod finalize; mod goal; mod goal_contract; +mod local_skeptic; mod memory; mod mutation_recovery; mod outcome; diff --git a/crates/hi-agent/src/tests/local_skeptic.rs b/crates/hi-agent/src/tests/local_skeptic.rs new file mode 100644 index 00000000..f72a3a48 --- /dev/null +++ b/crates/hi-agent/src/tests/local_skeptic.rs @@ -0,0 +1,141 @@ +//! Unit tests for the deterministic core of the auto-managed local skeptic: +//! backend selection, the default-model registry, weight-presence checks, the +//! `hi-local serve` argument builder, and `/config skeptic-local` parsing. The +//! live orchestration (download + spawn + health) needs real hardware and is +//! exercised manually, not here. + +use crate::command::{ConfigArg, config_is_skeptic_local, parse_config_arg}; +use crate::local_skeptic::{ + LocalBackend, default_model, endpoint_url, model_present, pick_backend, serve_args, + serve_model_path, +}; +use std::path::{Path, PathBuf}; + +fn scratch_dir(tag: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!("hi-local-skeptic-test-{tag}")); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + dir +} + +#[test] +fn pick_backend_prefers_mlx_then_cuda_then_none() { + assert_eq!(pick_backend(true, false), Some(LocalBackend::Mlx)); + // Apple Silicon wins even if an NVIDIA runtime is somehow also present. + assert_eq!(pick_backend(true, true), Some(LocalBackend::Mlx)); + assert_eq!(pick_backend(false, true), Some(LocalBackend::Cuda)); + assert_eq!(pick_backend(false, false), None); +} + +#[test] +fn default_model_matches_backend() { + let mlx = default_model(LocalBackend::Mlx); + assert_eq!(mlx.backend, LocalBackend::Mlx); + assert_eq!(mlx.backend.serve_flag(), "mlx"); + // MLX serves a whole directory — no single GGUF file. + assert!(mlx.gguf_file.is_none()); + assert!(!mlx.repo.is_empty()); + assert!(!mlx.model_id.is_empty()); + + let cuda = default_model(LocalBackend::Cuda); + assert_eq!(cuda.backend, LocalBackend::Cuda); + assert_eq!(cuda.backend.serve_flag(), "cuda"); + // CUDA serves one GGUF file inside the repo. + assert!( + cuda.gguf_file + .as_deref() + .is_some_and(|f| f.ends_with(".gguf")) + ); +} + +#[test] +fn model_present_checks_config_json_for_mlx() { + let dir = scratch_dir("mlx"); + let spec = default_model(LocalBackend::Mlx); + assert!(!model_present(&dir, &spec), "empty dir is not present"); + std::fs::write(dir.join("config.json"), "{}").unwrap(); + assert!(model_present(&dir, &spec), "config.json marks MLX present"); + let _ = std::fs::remove_dir_all(&dir); +} + +#[test] +fn model_present_checks_the_gguf_file_for_cuda() { + let dir = scratch_dir("cuda"); + let spec = default_model(LocalBackend::Cuda); + let file = spec.gguf_file.clone().unwrap(); + // A config.json is not enough for CUDA — the specific GGUF must exist. + std::fs::write(dir.join("config.json"), "{}").unwrap(); + assert!(!model_present(&dir, &spec)); + std::fs::write(dir.join(&file), b"gguf").unwrap(); + assert!(model_present(&dir, &spec)); + let _ = std::fs::remove_dir_all(&dir); +} + +#[test] +fn serve_model_path_is_dir_for_mlx_and_file_for_cuda() { + let dir = Path::new("/models/repo"); + let mlx = default_model(LocalBackend::Mlx); + assert_eq!(serve_model_path(dir, &mlx), dir.to_path_buf()); + + let cuda = default_model(LocalBackend::Cuda); + let expected = dir.join(cuda.gguf_file.clone().unwrap()); + assert_eq!(serve_model_path(dir, &cuda), expected); +} + +#[test] +fn serve_args_builds_the_expected_invocation() { + let spec = default_model(LocalBackend::Mlx); + let path = Path::new("/models/repo"); + let args = serve_args(path, &spec, "127.0.0.1", 8123); + assert_eq!( + args, + vec![ + "serve".to_string(), + "/models/repo".to_string(), + "--backend".to_string(), + "mlx".to_string(), + "--host".to_string(), + "127.0.0.1".to_string(), + "--port".to_string(), + "8123".to_string(), + "--model-id".to_string(), + spec.model_id.clone(), + ] + ); +} + +#[test] +fn endpoint_url_is_openai_compatible() { + assert_eq!(endpoint_url("127.0.0.1", 8080), "http://127.0.0.1:8080/v1"); +} + +#[test] +fn config_parses_skeptic_local_on_off_and_invalid() { + assert_eq!( + parse_config_arg("skeptic-local on"), + ConfigArg::SkepticLocal(true) + ); + assert_eq!( + parse_config_arg("skeptic-local off"), + ConfigArg::SkepticLocal(false) + ); + // Alias + case-insensitive value. + assert_eq!( + parse_config_arg("local-skeptic ON"), + ConfigArg::SkepticLocal(true) + ); + assert!(matches!( + parse_config_arg("skeptic-local"), + ConfigArg::Invalid(_) + )); + assert!(matches!( + parse_config_arg("skeptic-local maybe"), + ConfigArg::Invalid(_) + )); + + assert!(config_is_skeptic_local("skeptic-local on")); + assert!(config_is_skeptic_local("skeptic-local off")); + // A different /config option must not be misrouted to the async handler. + assert!(!config_is_skeptic_local("reasoning high")); + assert!(!config_is_skeptic_local("skeptic-local nonsense")); +} diff --git a/crates/hi-cli/src/commands.rs b/crates/hi-cli/src/commands.rs index 713fcf3f..44328836 100644 --- a/crates/hi-cli/src/commands.rs +++ b/crates/hi-cli/src/commands.rs @@ -159,7 +159,7 @@ pub(crate) fn handle_command(agent: &mut Agent, command: hi_agent::Command) -> b "\x1b[2m╰────────────────────────────────────────────────────╯\x1b[0m" ); println!( - "\x1b[2mset: /config reasoning · /config temp <0.0-2.0|off> · /config steps <1+|auto|off> · /config moe-streaming \x1b[0m" + "\x1b[2mset: /config reasoning · /config temp <0.0-2.0|off> · /config steps <1+|auto|off> · /config moe-streaming · /config skeptic-local \x1b[0m" ); } ConfigArg::Reasoning(effort) => { @@ -223,6 +223,14 @@ pub(crate) fn handle_command(agent: &mut Agent, command: hi_agent::Command) -> b } } } + ConfigArg::SkepticLocal(_) => { + // Routed through the async `handle_skeptic_local` from the + // REPL loop; only reachable if `/config skeptic-local` is + // dispatched outside it. + eprintln!( + "\x1b[33m/config skeptic-local must be run from the interactive prompt\x1b[0m" + ); + } ConfigArg::Invalid(m) => eprintln!("\x1b[33m{m}\x1b[0m"), } } @@ -563,6 +571,42 @@ pub(crate) async fn handle_goal_planned(agent: &mut hi_agent::Agent, objective: } } +/// Async handler for `/config skeptic-local `. Turning it on detects the +/// machine's local backend, downloads a small review model if needed (progress +/// prints to the terminal), spawns a `hi-local` server, and routes the `/goal` +/// skeptic review to it. Every failure degrades gracefully to the main model. +pub(crate) async fn handle_skeptic_local(agent: &mut Agent, arg: &str) { + use hi_agent::command::{ConfigArg, parse_config_arg}; + let on = match parse_config_arg(arg) { + ConfigArg::SkepticLocal(on) => on, + _ => return, + }; + if on { + println!( + "\x1b[2mlocal skeptic: detecting backend… (first run downloads a small review model)\x1b[0m" + ); + match agent.enable_local_skeptic(true).await { + Ok(hi_agent::LocalSkepticOutcome::Ready { endpoint, model_id }) => println!( + "\x1b[32m✓ local skeptic on\x1b[0m \x1b[2m→ {model_id} at {endpoint} (used for /goal team reviews)\x1b[0m" + ), + Ok(hi_agent::LocalSkepticOutcome::NoBackend) => eprintln!( + "\x1b[33mno local backend found — needs Apple-Silicon MLX or an NVIDIA GPU. Skeptic stays on the main model.\x1b[0m" + ), + Ok(hi_agent::LocalSkepticOutcome::NeedsDownload { repo, dir }) => println!( + "\x1b[2mmodel {repo} isn't cached — fetch it into {} first, then retry\x1b[0m", + dir.display() + ), + Err(err) => eprintln!( + "\x1b[33mcouldn't start local skeptic: {err:#}\nSkeptic stays on the main model.\x1b[0m" + ), + } + } else if agent.disable_local_skeptic() { + println!("\x1b[2mlocal skeptic off — skeptic review back on the main model\x1b[0m"); + } else { + println!("\x1b[2mlocal skeptic was not on\x1b[0m"); + } +} + fn handle_goal_limit(agent: &mut hi_agent::Agent, limit: hi_agent::command::GoalLimitArg) { use hi_agent::command::GoalLimitArg; match limit { diff --git a/crates/hi-cli/src/repl.rs b/crates/hi-cli/src/repl.rs index b1ad1821..8f2d518d 100644 --- a/crates/hi-cli/src/repl.rs +++ b/crates/hi-cli/src/repl.rs @@ -528,6 +528,15 @@ pub(crate) async fn repl( crate::commands::handle_lsp(agent, &arg); continue; } + // `/config skeptic-local ` may download a model + // and spawn a local server, so it runs on the async path; + // every other `/config …` falls through to the sync handler. + Command::Config(arg) + if hi_agent::command::config_is_skeptic_local(&arg) => + { + crate::commands::handle_skeptic_local(agent, &arg).await; + continue; + } // `/goal ` with a planner: decompose (one bounded // call), then install the structured goal. Control subcommands // (clear/pause/resume/limit) and the no-planner case fall diff --git a/crates/hi-tools/src/hf.rs b/crates/hi-tools/src/hf.rs index 7f53b5fb..dca6844e 100644 --- a/crates/hi-tools/src/hf.rs +++ b/crates/hi-tools/src/hf.rs @@ -768,7 +768,7 @@ fn whole_repo_mode(input: Option<&str>) -> Option { } } -fn safe_path(input: &str) -> String { +pub(crate) fn safe_path(input: &str) -> String { let mut out = String::with_capacity(input.len()); for c in input.chars() { if c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '-') { diff --git a/crates/hi-tools/src/lib.rs b/crates/hi-tools/src/lib.rs index d6edb723..242b719f 100644 --- a/crates/hi-tools/src/lib.rs +++ b/crates/hi-tools/src/lib.rs @@ -19,6 +19,7 @@ mod edit; mod effects; mod hf; mod internal_snapshot; +mod local_server; mod paths; mod process; mod read; @@ -32,6 +33,10 @@ pub use hf::{ HfCommandResult, HfCommandState, HfMlxRun, download_repo_keep_foreground, handle_hf_command, handle_hf_command_result, }; +pub use local_server::{ + LocalServerHandle, skeptic_model_dir, start_local_server, stop_all_local_servers, + stop_local_server, +}; pub use lsp::lsp_status_report_for; pub use paths::ReadCache; pub use process::{AdoptableOutcome, ProcessExecution, ProcessRunner, RunningChild}; diff --git a/crates/hi-tools/src/local_server.rs b/crates/hi-tools/src/local_server.rs new file mode 100644 index 00000000..a411a9f0 --- /dev/null +++ b/crates/hi-tools/src/local_server.rs @@ -0,0 +1,102 @@ +//! Auto-managed local model server for the `/goal` skeptic review. +//! +//! Spawns `hi-local serve …` into a private background registry, waits for its +//! `/health` endpoint to report ready, and hands back the OpenAI-compatible base +//! URL. The policy around it — which backend, which default model, when to turn +//! it on — lives in `hi-agent`'s `local_skeptic` module; this file owns only the +//! process and HTTP mechanics, mirroring the proven `/hf run --mlx` path. + +use anyhow::{Result, anyhow, bail}; +use std::path::{Path, PathBuf}; +use std::sync::LazyLock; +use std::time::Duration; + +// Private registry so these servers stay isolated from any workspace runtime and +// can be stopped by id when the user turns the feature off. Like the `/hf` +// sidecar registry, entries are never adopted by an agent's tool registry. +static LOCAL_SERVERS: LazyLock = + LazyLock::new(crate::BackgroundRegistry::default); + +/// A running local model server routed to the skeptic review. +pub struct LocalServerHandle { + /// Background-registry handle, used to stop the server later. + pub process_id: String, + /// OpenAI-compatible base URL (e.g. `http://127.0.0.1:8080/v1`). + pub endpoint: String, +} + +/// Cache directory for a downloaded local model, matching `/hf run --mlx`'s +/// layout (`$HI_MLX_MODELS_DIR` or `./.hi/models`, repo id sanitized) so a model +/// fetched by either path is reused rather than downloaded twice. Uses the +/// `main` revision (no `@rev` suffix). +pub fn skeptic_model_dir(repo_id: &str) -> PathBuf { + let root = std::env::var_os("HI_MLX_MODELS_DIR") + .map(PathBuf::from) + .unwrap_or_else(|| PathBuf::from(".hi").join("models")); + root.join(crate::hf::safe_path(repo_id)) +} + +/// Spawn `bin serve ` in the background and wait for `/health` to report +/// ready. On failure the process is killed and its captured output is folded +/// into the error so the caller can surface a real diagnosis. `host`/`port` must +/// match the `--host`/`--port` already present in `serve_args`. +pub async fn start_local_server( + bin: &Path, + serve_args: &[String], + host: &str, + port: u16, +) -> Result { + let mut command = crate::web::shell_quote(&bin.to_string_lossy()); + for arg in serve_args { + command.push(' '); + command.push_str(&crate::web::shell_quote(arg)); + } + let runner = crate::ProcessRunner::new(std::env::current_dir()?)?; + let process_id = LOCAL_SERVERS.spawn(&runner, &command)?; + match wait_for_health(host, port).await { + Ok(()) => Ok(LocalServerHandle { + process_id, + endpoint: format!("http://{host}:{port}/v1"), + }), + Err(err) => { + let output = LOCAL_SERVERS.poll(&process_id).unwrap_or_default(); + let _ = LOCAL_SERVERS.kill(&process_id); + bail!("hi-local did not become healthy at http://{host}:{port}: {err}\n{output}"); + } + } +} + +/// Stop a server started by [`start_local_server`]. No-op if already gone. +pub fn stop_local_server(process_id: &str) { + let _ = LOCAL_SERVERS.kill(process_id); +} + +/// Stop every local model server started by this process. For session shutdown: +/// the registry only ever holds `/goal` skeptic servers, so a frontend can call +/// this from a drop guard to cover all exit paths without tracking ids. +pub fn stop_all_local_servers() { + LOCAL_SERVERS.kill_all(); +} + +async fn wait_for_health(host: &str, port: u16) -> Result<()> { + let url = format!("http://{host}:{port}/health"); + let client = reqwest::Client::new(); + let mut last_error = None; + for _ in 0..40 { + match client.get(&url).send().await { + Ok(response) if response.status().is_success() => match response.json().await { + Ok(body) if health_ready(&body) => return Ok(()), + Ok(body) => last_error = Some(anyhow!("health returned not-ready body: {body}")), + Err(err) => last_error = Some(anyhow!("health response was not valid JSON: {err}")), + }, + Ok(response) => last_error = Some(anyhow!("health returned {}", response.status())), + Err(err) => last_error = Some(anyhow!(err)), + } + tokio::time::sleep(Duration::from_millis(250)).await; + } + Err(last_error.unwrap_or_else(|| anyhow!("health check timed out"))) +} + +fn health_ready(body: &serde_json::Value) -> bool { + body.get("ready").and_then(serde_json::Value::as_bool) == Some(true) +} diff --git a/crates/hi-tui/src/app/commands.rs b/crates/hi-tui/src/app/commands.rs index b416a860..dec502bc 100644 --- a/crates/hi-tui/src/app/commands.rs +++ b/crates/hi-tui/src/app/commands.rs @@ -851,6 +851,46 @@ impl crate::App { }; self.push(Line::styled(msg, dim())); } + ConfigArg::SkepticLocal(on) => { + if on { + self.push(Line::styled( + "local skeptic: detecting backend…".to_string(), + dim(), + )); + // The TUI owns an alternate screen, so it can't run + // the progress-to-terminal model download inline — + // it reports NeedsDownload instead of corrupting it. + let msg = match agent.enable_local_skeptic(false).await { + Ok(hi_agent::LocalSkepticOutcome::Ready { endpoint, model_id }) => { + format!( + "local skeptic on → {model_id} at {endpoint} (used for /goal team reviews)" + ) + } + Ok(hi_agent::LocalSkepticOutcome::NoBackend) => { + "no local backend (needs Apple-Silicon MLX or an NVIDIA GPU) — skeptic stays on the main model".to_string() + } + Ok(hi_agent::LocalSkepticOutcome::NeedsDownload { repo, dir }) => { + format!( + "model {repo} isn't cached — run `hi` in a plain terminal with `/config skeptic-local on` once to fetch it into {}, then retry here", + dir.display() + ) + } + Err(err) => { + format!( + "couldn't start local skeptic: {err:#} — skeptic stays on the main model" + ) + } + }; + self.push(Line::styled(msg, dim())); + } else { + let msg = if agent.disable_local_skeptic() { + "local skeptic off — review back on the main model" + } else { + "local skeptic was not on" + }; + self.push(Line::styled(msg.to_string(), dim())); + } + } ConfigArg::Invalid(m) => { self.push(Line::styled(m, Style::default().fg(Color::Yellow))); } diff --git a/crates/hi-tui/src/app/run.rs b/crates/hi-tui/src/app/run.rs index 937f3e9d..cf6d98ff 100644 --- a/crates/hi-tui/src/app/run.rs +++ b/crates/hi-tui/src/app/run.rs @@ -35,6 +35,17 @@ use crate::{ /// the file used to persist input history across sessions (shared with the /// plain REPL). `profiles` is the list of configured profiles (for `/provider` /// with no arg); `resolver` resolves a name to a built provider at runtime. +/// Drop guard that stops any auto-managed local skeptic server when the TUI +/// session ends, covering every `return`/`break` exit path in [`run`]. The +/// server registry only holds skeptic servers, so a blanket kill is correct. +struct LocalServerGuard; + +impl Drop for LocalServerGuard { + fn drop(&mut self) { + hi_tools::stop_all_local_servers(); + } +} + #[allow(clippy::too_many_arguments)] pub async fn run( agent: &mut Agent, @@ -71,6 +82,8 @@ pub async fn run( // Install immediately after raw mode so any later startup error restores // the terminal before main falls back to plain mode. let _restore = Restore; + // Tear down any auto-managed `/goal` skeptic server on every exit path. + let _local_servers = LocalServerGuard; execute!(io::stdout(), EnterAlternateScreen).context("entering alternate screen")?; // Bracketed paste: the terminal wraps a paste so it arrives as one // Event::Paste instead of per-line Enter keys (which would submit each line). diff --git a/scripts/file_size_baseline.txt b/scripts/file_size_baseline.txt index b778cf43..ad7c16eb 100644 --- a/scripts/file_size_baseline.txt +++ b/scripts/file_size_baseline.txt @@ -2,7 +2,7 @@ # Only pre-existing files above the 800-line new-file limit need entries. 1466 crates/hi-agent/src/agent/lifecycle.rs 5032 crates/hi-agent/src/agent/turn.rs -1865 crates/hi-agent/src/command.rs +1885 crates/hi-agent/src/command.rs 1433 crates/hi-agent/src/heuristics.rs 924 crates/hi-agent/src/memory.rs 1118 crates/hi-agent/src/tests/retry.rs @@ -14,7 +14,7 @@ 1819 crates/hi-ai/src/openai/stream.rs 2606 crates/hi-cli/src/config.rs 2583 crates/hi-cli/src/main.rs -961 crates/hi-cli/src/repl.rs +969 crates/hi-cli/src/repl.rs 1582 crates/hi-cli/src/session.rs 2384 crates/hi-cli/src/sync.rs 1377 crates/hi-eval/src/artifacts.rs @@ -24,13 +24,13 @@ 1151 crates/hi-tools/src/edit.rs 1057 crates/hi-tools/src/hf.rs 1048 crates/hi-tools/src/internal_snapshot.rs -954 crates/hi-tools/src/lib.rs +958 crates/hi-tools/src/lib.rs 3111 crates/hi-tools/src/tools.rs 1489 crates/hi-tools/src/transaction.rs 1301 crates/hi-tools/src/web.rs -1047 crates/hi-tui/src/app/commands.rs +1087 crates/hi-tui/src/app/commands.rs 1444 crates/hi-tui/src/app/render.rs -2569 crates/hi-tui/src/app/run.rs +2582 crates/hi-tui/src/app/run.rs 1758 crates/hi-tui/src/dashboard.rs 2527 crates/hi-tui/src/loops.rs 2888 crates/hi-tui/src/tests.rs