From a3ae974676c349e96e834e56fe6c1565fbcb41a8 Mon Sep 17 00:00:00 2001 From: Paulo Aboim Pinto Date: Sun, 31 May 2026 11:24:25 +0200 Subject: [PATCH 01/15] feat(tui): add ExternalTool abstraction layer (#2294) Harvested with thanks to @aboimpinto. Includes the ExternalTool abstraction layer plus follow-up fixes for lossy REPL stdout handling and unquoted unicode git diff paths. Validation included full CI and focused local checks for non-UTF8 REPL stdout, git_diff, and external_tool behavior. --- crates/tui/src/commands/debug.rs | 32 +- crates/tui/src/commands/share.rs | 16 +- crates/tui/src/config_ui.rs | 33 +- crates/tui/src/core/engine/tests.rs | 25 ++ crates/tui/src/core/engine/tool_catalog.rs | 31 +- crates/tui/src/dependencies.rs | 457 ++++++++++++++++++++ crates/tui/src/main.rs | 33 +- crates/tui/src/repl/runtime.rs | 43 +- crates/tui/src/runtime_api.rs | 9 +- crates/tui/src/snapshot/repo.rs | 25 +- crates/tui/src/tools/diagnostics.rs | 14 +- crates/tui/src/tools/git.rs | 117 ++--- crates/tui/src/tools/git_history.rs | 24 +- crates/tui/src/tools/github.rs | 16 +- crates/tui/src/tools/js_execution.rs | 24 +- crates/tui/src/tools/review.rs | 10 +- crates/tui/src/tools/tasks.rs | 58 ++- crates/tui/src/tools/test_runner.rs | 12 +- crates/tui/src/tui/clipboard.rs | 149 ++----- crates/tui/src/tui/file_picker_relevance.rs | 7 +- crates/tui/src/tui/ui.rs | 37 +- crates/tui/src/tui/ui/tests.rs | 27 ++ crates/tui/src/tui/workspace_context.rs | 7 +- crates/tui/src/utils.rs | 88 ++++ 24 files changed, 895 insertions(+), 399 deletions(-) diff --git a/crates/tui/src/commands/debug.rs b/crates/tui/src/commands/debug.rs index 85b21faea..80bdb3343 100644 --- a/crates/tui/src/commands/debug.rs +++ b/crates/tui/src/commands/debug.rs @@ -7,6 +7,7 @@ use std::time::Instant; use super::CommandResult; use crate::client::{PromptInspection, inspect_prompt_for_request}; use crate::compaction::estimate_input_tokens_conservative; +use crate::dependencies::{ExternalTool, Git}; use crate::localization::{Locale, MessageId, tr}; use crate::models::{ContentBlock, MessageRequest, SystemPrompt, context_window_for_model}; use crate::tui::app::{App, AppAction, TurnCacheRecord}; @@ -1857,15 +1858,18 @@ pub fn patch_undo(app: &mut App) -> CommandResult { } // Show diff stat so the user knows what changed. - let diff_stat = std::process::Command::new("git") - .args(["diff", "--stat"]) - .current_dir(&workspace) - .output() - .ok() - .and_then(|o| { - let s = String::from_utf8_lossy(&o.stdout).trim().to_string(); - if s.is_empty() { None } else { Some(s) } - }); + let diff_stat = Git::command() + .map(|mut git| { + git.args(["diff", "--stat"]) + .current_dir(&workspace) + .output() + .ok() + .and_then(|o| { + let s = String::from_utf8_lossy(&o.stdout).trim().to_string(); + if s.is_empty() { None } else { Some(s) } + }) + }) + .unwrap_or(None); let short = &target.id.as_str()[..target.id.as_str().len().min(8)]; let summary = match diff_stat { @@ -1936,11 +1940,17 @@ pub fn edit(app: &mut App) -> CommandResult { pub fn diff(app: &mut App) -> CommandResult { let workspace = app.workspace.clone(); - let name_only_output = std::process::Command::new("git") + let Some(mut name_only_cmd) = Git::command() else { + return CommandResult::error("git not found on PATH"); + }; + let Some(mut stat_cmd) = Git::command() else { + return CommandResult::error("git not found on PATH"); + }; + let name_only_output = name_only_cmd .args(["diff", "--name-only"]) .current_dir(&workspace) .output(); - let stat_output = std::process::Command::new("git") + let stat_output = stat_cmd .args(["diff", "--stat"]) .current_dir(&workspace) .output(); diff --git a/crates/tui/src/commands/share.rs b/crates/tui/src/commands/share.rs index 9923af0b5..43e42ec9c 100644 --- a/crates/tui/src/commands/share.rs +++ b/crates/tui/src/commands/share.rs @@ -12,6 +12,7 @@ use std::io::Write; use std::path::Path; use super::CommandResult; +use crate::dependencies::ExternalTool; use crate::tui::app::{App, AppAction}; /// Share the current session as a web URL. @@ -155,20 +156,25 @@ fn write_temp_html(html: &str) -> Result { /// Upload a file as a GitHub Gist using the `gh` CLI. async fn upload_gist(path: &Path) -> Result { - let output = tokio::process::Command::new("gh") - .args([ + let path_owned = path.to_path_buf(); + let output = tokio::task::spawn_blocking(move || { + let mut cmd = crate::dependencies::Gh::command() + .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "gh not found"))?; + cmd.args([ "gist", "create", "--public", - &path.to_string_lossy(), + &path_owned.to_string_lossy().to_string(), "--filename", "session-export.html", "--desc", "codewhale Session Export", ]) .output() - .await - .map_err(|e| format!("Failed to run `gh gist create`: {e}"))?; + }) + .await + .map_err(|join_err| format!("gh gist create panicked: {join_err}"))? + .map_err(|e| format!("Failed to run `gh gist create`: {e}"))?; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); diff --git a/crates/tui/src/config_ui.rs b/crates/tui/src/config_ui.rs index 33cd9bb65..8526f10a2 100644 --- a/crates/tui/src/config_ui.rs +++ b/crates/tui/src/config_ui.rs @@ -1,8 +1,6 @@ #[cfg(feature = "web")] use std::net::SocketAddr; #[cfg(feature = "web")] -use std::process::Command; -#[cfg(feature = "web")] use std::time::Duration; use anyhow::{Context, Result, bail}; @@ -605,36 +603,7 @@ pub fn parse_document(value: Value) -> Result { #[cfg(feature = "web")] pub fn open_browser(url: &str) -> Result<()> { - #[cfg(target_os = "macos")] - let mut command = { - let mut command = Command::new("open"); - command.arg(url); - command - }; - #[cfg(target_os = "linux")] - let mut command = { - let mut command = Command::new("xdg-open"); - command.arg(url); - command - }; - #[cfg(target_os = "windows")] - let mut command = { - let mut command = Command::new("cmd"); - command.args(["/C", "start", "", url]); - command - }; - #[cfg(not(any(target_os = "macos", target_os = "linux", target_os = "windows")))] - return Err(anyhow::anyhow!( - "browser opening is unsupported on this platform" - )); - - let status = command - .status() - .context("failed to launch browser command")?; - if !status.success() { - bail!("browser command exited with status {status}"); - } - Ok(()) + crate::utils::open_url(url) } fn validate_document(doc: &ConfigUiDocument) -> Result<()> { diff --git a/crates/tui/src/core/engine/tests.rs b/crates/tui/src/core/engine/tests.rs index f26e05644..ade5dd690 100644 --- a/crates/tui/src/core/engine/tests.rs +++ b/crates/tui/src/core/engine/tests.rs @@ -444,6 +444,26 @@ fn non_yolo_mode_retains_default_defer_policy() { AppMode::Agent, &always_load )); + assert!(!should_default_defer_tool( + "apply_patch", + AppMode::Agent, + &always_load + )); + assert!(!should_default_defer_tool( + "fetch_url", + AppMode::Agent, + &always_load + )); + assert!(!should_default_defer_tool( + "git_diff", + AppMode::Agent, + &always_load + )); + assert!(!should_default_defer_tool( + "git_status", + AppMode::Agent, + &always_load + )); assert!(!should_default_defer_tool( "run_tests", AppMode::Agent, @@ -459,6 +479,11 @@ fn non_yolo_mode_retains_default_defer_policy() { AppMode::Agent, &always_load )); + assert!(!should_default_defer_tool( + "web_search", + AppMode::Agent, + &always_load + )); assert!(!should_default_defer_tool( "write_file", AppMode::Agent, diff --git a/crates/tui/src/core/engine/tool_catalog.rs b/crates/tui/src/core/engine/tool_catalog.rs index 60b6166ba..867c68951 100644 --- a/crates/tui/src/core/engine/tool_catalog.rs +++ b/crates/tui/src/core/engine/tool_catalog.rs @@ -15,6 +15,8 @@ use crate::models::Tool; use crate::tools::spec::{ToolError, ToolResult, optional_u64, required_str}; use crate::tui::app::AppMode; +use crate::dependencies::ExternalTool; + pub(super) const MULTI_TOOL_PARALLEL_NAME: &str = "multi_tool_use.parallel"; pub(super) const REQUEST_USER_INPUT_NAME: &str = "request_user_input"; pub(super) const CODE_EXECUTION_TOOL_NAME: &str = "code_execution"; @@ -720,20 +722,9 @@ pub(super) async fn execute_code_execution_tool( // Resolve the locally-installed Python interpreter we cached at // catalog-build time. If it's absent now (somehow registered but // disappeared between startup and this call — concurrent uninstall, - // PATH change, etc.) we fail fast with a clear message rather than - // dropping into `tokio::process::Command::new("python3")` and - // surfacing the cryptic "program not found" the contributor - // originally hit on Windows. - let interpreter = crate::dependencies::resolve_python_interpreter().ok_or_else(|| { - ToolError::execution_failed(format!( - "code_execution: no Python interpreter found on PATH (tried {:?}). \ - Install Python 3 and ensure one of these is on PATH, then restart \ - codewhale.", - crate::dependencies::PYTHON_CANDIDATES, - )) - })?; - let (program, args) = crate::dependencies::split_interpreter_spec(&interpreter); - + // PATH change, etc.) the ExternalTool::tokio_command() will return + // None and we fail fast with a clear message. + // // Write the code to a temp file and execute it as a script rather // than passing it via `-c ""`. Reasons: // * `-c` has length limits (argv) on Windows. @@ -750,12 +741,12 @@ pub(super) async fn execute_code_execution_tool( .await .map_err(|e| ToolError::execution_failed(format!("tempfile write failed: {e}")))?; - let mut cmd = tokio::process::Command::new(&program); - for arg in &args { - cmd.arg(arg); - } - cmd.arg(&script_path); - cmd.current_dir(workspace); + let mut cmd = crate::dependencies::Python::tokio_command().ok_or_else(|| { + ToolError::execution_failed( + "code_execution: Python interpreter became unavailable".to_string(), + ) + })?; + cmd.arg(&script_path).current_dir(workspace); let output = tokio::time::timeout(Duration::from_secs(120), cmd.output()) .await diff --git a/crates/tui/src/dependencies.rs b/crates/tui/src/dependencies.rs index 1918c291e..bb35c59ca 100644 --- a/crates/tui/src/dependencies.rs +++ b/crates/tui/src/dependencies.rs @@ -198,6 +198,234 @@ pub fn resolve_node() -> Option { .clone() } +// --------------------------------------------------------------------------- +// ExternalTool trait — unified subprocess interface +// --------------------------------------------------------------------------- + +/// A tool that DeepSeek-TUI shells out to. Instead of scattering +/// `Command::new("git")` / `Command::new("gh")` across the codebase, +/// each external dependency implements this trait once in this module. +/// Callers ask the tool for a pre-populated [`Command`] and chain their +/// own args, working directory, and spawn method. +/// +/// # Example +/// +/// ```ignore +/// let output = Git::command() +/// .expect("git not found") +/// .args(["diff", "--stat"]) +/// .current_dir(&workspace) +/// .output()?; +/// ``` +pub trait ExternalTool { + /// Candidate binary names, tried in order until one responds to + /// `--version`. For single-binary tools (git, gh, node) this is a + /// one-element slice. + fn candidates() -> &'static [&'static str]; + + /// Resolve the best candidate once per process (cached). Returns + /// the spec string (e.g. `"python3"` or `"py -3"`). + fn resolve() -> Option; + + /// Quick availability check — true when the tool was found on PATH. + #[allow(dead_code)] + fn available() -> bool { + Self::resolve().is_some() + } + + /// Build a `std::process::Command` pre-populated with the resolved + /// binary (and any fixed arguments from a multi-word candidate like + /// `"py -3"`). Returns `None` when the tool isn't installed. + /// + /// Callers should chain `.args(...)`, `.current_dir(...)`, and then + /// call `.output()`, `.status()`, or `.spawn()`. + fn command() -> Option { + let spec = Self::resolve()?; + let (program, fixed_args) = split_interpreter_spec(&spec); + let mut cmd = Command::new(&program); + for arg in &fixed_args { + cmd.arg(arg); + } + Some(cmd) + } + + /// Convenience: run the tool with arguments in a working directory + /// and return the captured output. + fn output(args: &[&str], cwd: &std::path::Path) -> std::io::Result { + let mut cmd = Self::command().ok_or_else(|| { + std::io::Error::new( + std::io::ErrorKind::NotFound, + format!("{} not found on PATH", std::any::type_name::()), + ) + })?; + cmd.args(args).current_dir(cwd).output() + } + + /// Convenience: run the tool with arguments and return only the + /// exit status (discards stdout/stderr). + #[allow(dead_code)] + fn status(args: &[&str], cwd: &std::path::Path) -> std::io::Result { + let mut cmd = Self::command().ok_or_else(|| { + std::io::Error::new( + std::io::ErrorKind::NotFound, + format!("{} not found on PATH", std::any::type_name::()), + ) + })?; + cmd.args(args).current_dir(cwd).status() + } + + /// Build a `tokio::process::Command` pre-populated with the resolved + /// binary (and any fixed arguments from a multi-word candidate like + /// `"py -3"`). Returns `None` when the tool isn't installed. + /// + /// Async callers (`code_execution`, `js_execution`) use this instead + /// of [`ExternalTool::command`] so they can `.await` the child. + fn tokio_command() -> Option { + let spec = Self::resolve()?; + let (program, fixed_args) = split_interpreter_spec(&spec); + let mut cmd = tokio::process::Command::new(&program); + for arg in &fixed_args { + cmd.arg(arg); + } + Some(cmd) + } +} + +// --------------------------------------------------------------------------- +// Concrete tool implementations +// --------------------------------------------------------------------------- + +/// Git version control. +pub struct Git; + +impl ExternalTool for Git { + fn candidates() -> &'static [&'static str] { + &["git"] + } + + fn resolve() -> Option { + static CACHE: OnceLock> = OnceLock::new(); + CACHE + .get_or_init(|| { + for candidate in Self::candidates() { + if probe_executable(candidate) { + tracing::info!(target: "tool_dependencies", "Resolved git binary"); + return Some((*candidate).to_string()); + } + } + None + }) + .clone() + } +} + +/// GitHub CLI. +pub struct Gh; + +impl ExternalTool for Gh { + fn candidates() -> &'static [&'static str] { + &["gh"] + } + + fn resolve() -> Option { + static CACHE: OnceLock> = OnceLock::new(); + CACHE + .get_or_init(|| { + for candidate in Self::candidates() { + if probe_executable(candidate) { + tracing::info!(target: "tool_dependencies", "Resolved gh binary"); + return Some((*candidate).to_string()); + } + } + None + }) + .clone() + } +} + +/// Rust compiler — used for version reporting in diagnostics. +pub struct RustC; + +impl ExternalTool for RustC { + fn candidates() -> &'static [&'static str] { + &["rustc"] + } + + fn resolve() -> Option { + static CACHE: OnceLock> = OnceLock::new(); + CACHE + .get_or_init(|| { + for candidate in Self::candidates() { + if probe_executable(candidate) { + tracing::info!(target: "tool_dependencies", "Resolved rustc binary"); + return Some((*candidate).to_string()); + } + } + None + }) + .clone() + } +} + +/// Rust build tool — used by the `run_tests` tool. +pub struct Cargo; + +impl ExternalTool for Cargo { + fn candidates() -> &'static [&'static str] { + &["cargo"] + } + + fn resolve() -> Option { + static CACHE: OnceLock> = OnceLock::new(); + CACHE + .get_or_init(|| { + for candidate in Self::candidates() { + if probe_executable(candidate) { + tracing::info!(target: "tool_dependencies", "Resolved cargo binary"); + return Some((*candidate).to_string()); + } + } + None + }) + .clone() + } +} + +/// Python interpreter — used by `code_execution` tool and RLM REPL. +/// Delegates to the existing [`resolve_python_interpreter`] so the +/// multi-candidate ladder (`python3` → `python` → `py -3`) is +/// shared with legacy callers until they migrate to the trait. +pub struct Python; + +impl ExternalTool for Python { + fn candidates() -> &'static [&'static str] { + PYTHON_CANDIDATES + } + + fn resolve() -> Option { + resolve_python_interpreter() + } +} + +/// Node.js runtime — used by the `js_execution` tool. +/// The binary name `node` is the same on every platform we support, +/// so this is a single probe rather than a candidate ladder. +pub struct Node; + +impl ExternalTool for Node { + fn candidates() -> &'static [&'static str] { + &["node"] + } + + fn resolve() -> Option { + resolve_node() + } +} + +// --------------------------------------------------------------------------- +// Legacy interpreter helpers (kept for existing callers until migrated) +// --------------------------------------------------------------------------- + /// Split an interpreter spec like `"py -3"` into the program name /// and any initial arguments. Returns `("py", vec!["-3"])` for the /// example; returns `("python3", vec![])` for a bare name. @@ -288,4 +516,233 @@ mod tests { ); } } + + // =================================================================== + // ExternalTool trait tests + // =================================================================== + + #[test] + fn python_candidates_matches_const() { + assert_eq!(Python::candidates(), PYTHON_CANDIDATES); + } + + #[test] + fn node_candidates_is_node_only() { + assert_eq!(Node::candidates(), &["node"]); + } + + #[test] + fn git_candidates_is_git_only() { + assert_eq!(Git::candidates(), &["git"]); + } + + #[test] + fn gh_candidates_is_gh_only() { + assert_eq!(Gh::candidates(), &["gh"]); + } + + #[test] + fn rustc_candidates_is_rustc_only() { + assert_eq!(RustC::candidates(), &["rustc"]); + } + + #[test] + fn cargo_candidates_is_cargo_only() { + assert_eq!(Cargo::candidates(), &["cargo"]); + } + + #[test] + fn concrete_resolvers_do_not_cross_contaminate_when_available() { + let values = [ + Git::resolve().map(|v| ("git", v)), + Gh::resolve().map(|v| ("gh", v)), + RustC::resolve().map(|v| ("rustc", v)), + Cargo::resolve().map(|v| ("cargo", v)), + Node::resolve().map(|v| ("node", v)), + ]; + let resolved: Vec<(&str, String)> = values.into_iter().flatten().collect(); + + for i in 0..resolved.len() { + for j in (i + 1)..resolved.len() { + assert_ne!( + resolved[i].1, resolved[j].1, + "{} and {} unexpectedly resolved to the same binary", + resolved[i].0, resolved[j].0 + ); + } + } + } + + #[test] + fn git_resolve_is_cached() { + let first = Git::resolve(); + let second = Git::resolve(); + assert_eq!(first, second); + } + + #[test] + fn gh_resolve_is_cached() { + let first = Gh::resolve(); + let second = Gh::resolve(); + assert_eq!(first, second); + } + + #[test] + fn python_trait_resolve_is_cached() { + let first = Python::resolve(); + let second = Python::resolve(); + assert_eq!(first, second); + } + + #[test] + fn node_resolve_is_cached() { + let first = Node::resolve(); + let second = Node::resolve(); + assert_eq!(first, second); + } + + #[test] + fn rustc_resolve_is_cached() { + let first = RustC::resolve(); + let second = RustC::resolve(); + assert_eq!(first, second); + } + + #[test] + fn cargo_resolve_is_cached() { + let first = Cargo::resolve(); + let second = Cargo::resolve(); + assert_eq!(first, second); + } + + #[test] + fn git_available_matches_resolve() { + assert_eq!(Git::available(), Git::resolve().is_some()); + } + + #[test] + fn python_available_matches_resolve() { + assert_eq!(Python::available(), Python::resolve().is_some()); + } + + #[test] + fn node_available_matches_resolve() { + assert_eq!(Node::available(), Node::resolve().is_some()); + } + + #[test] + fn rustc_available_matches_resolve() { + assert_eq!(RustC::available(), RustC::resolve().is_some()); + } + + #[test] + fn cargo_available_matches_resolve() { + assert_eq!(Cargo::available(), Cargo::resolve().is_some()); + } + + #[test] + fn git_command_returns_some_when_available() { + if Git::available() { + assert!(Git::command().is_some()); + } + } + + #[test] + fn python_command_returns_some_when_available() { + if Python::available() { + assert!(Python::command().is_some()); + } + } + + #[test] + fn python_tokio_command_returns_some_when_available() { + if Python::available() { + assert!(Python::tokio_command().is_some()); + } + } + + #[test] + fn node_tokio_command_returns_some_when_available() { + if Node::available() { + assert!(Node::tokio_command().is_some()); + } + } + + #[test] + fn git_output_version_succeeds() { + // Only run when git is actually installed. + if !Git::available() { + return; + } + let tmp = std::env::temp_dir(); + let out = Git::output(&["--version"], &tmp); + assert!( + out.is_ok(), + "git --version must succeed when git is available" + ); + let out = out.unwrap(); + assert!(out.status.success(), "git --version must exit 0"); + let stdout = String::from_utf8_lossy(&out.stdout); + assert!( + stdout.contains("git version"), + "git --version stdout must contain 'git version', got: {}", + stdout.trim() + ); + } + + #[test] + fn python_output_version_succeeds() { + if !Python::available() { + return; + } + let tmp = std::env::temp_dir(); + let out = Python::output(&["--version"], &tmp); + assert!(out.is_ok(), "python --version must spawn"); + let out = out.unwrap(); + // Python --version writes to stdout on 3.x, so just check + // that it succeeded (exit 0). + assert!(out.status.success(), "python --version must exit 0"); + } + + #[test] + fn node_output_version_succeeds() { + if !Node::available() { + return; + } + let tmp = std::env::temp_dir(); + let out = Node::output(&["--version"], &tmp); + assert!(out.is_ok(), "node --version must spawn"); + let out = out.unwrap(); + assert!(out.status.success(), "node --version must exit 0"); + } + + #[test] + fn cargo_output_version_succeeds() { + if !Cargo::available() { + return; + } + let tmp = std::env::temp_dir(); + let out = Cargo::output(&["--version"], &tmp); + assert!(out.is_ok(), "cargo --version must spawn"); + let out = out.unwrap(); + assert!(out.status.success(), "cargo --version must exit 0"); + } + + #[test] + fn external_tool_output_respects_cwd() { + // Verify that `output()` runs in the requested directory. + if !Git::available() { + return; + } + let tmp = std::env::temp_dir(); + let out = Git::output(&["rev-parse", "--show-toplevel"], &tmp); + assert!(out.is_ok(), "git rev-parse must spawn"); + let out = out.unwrap(); + // rev-parse --show-toplevel in a non-git dir should fail + // because temp_dir is not a git repo. That's expected. + // The key assertion: the command executed without IO errors. + // We don't assert success because temp_dir might or might not + // be inside a git worktree. + let _ = out; // just checking it didn't panic/IO-error + } } diff --git a/crates/tui/src/main.rs b/crates/tui/src/main.rs index 340b3b4e4..0954a1c92 100644 --- a/crates/tui/src/main.rs +++ b/crates/tui/src/main.rs @@ -12,6 +12,8 @@ use dotenvy::dotenv; use tempfile::NamedTempFile; use wait_timeout::ChildExt; +use crate::dependencies::ExternalTool; + mod acp_server; mod artifacts; mod audit; @@ -3541,13 +3543,15 @@ async fn test_api_connectivity(config: &Config) -> Result<()> { } fn rustc_version() -> String { - // Try to get rustc version, fall back to "unknown" - std::process::Command::new("rustc") - .arg("--version") - .output() - .ok() - .and_then(|o| String::from_utf8(o.stdout).ok()) - .map_or_else(|| "unknown".to_string(), |s| s.trim().to_string()) + let Some(mut cmd) = crate::dependencies::RustC::command() else { + return "unknown".to_string(); + }; + let Ok(output) = cmd.arg("--version").output() else { + return "unknown".to_string(); + }; + String::from_utf8(output.stdout) + .map(|s| s.trim().to_string()) + .unwrap_or_else(|_| "unknown".to_string()) } /// List saved sessions @@ -3965,7 +3969,8 @@ struct GhPullRequest { } fn run_gh_pr_view(number: u32, repo: Option<&str>) -> Result { - let mut cmd = Command::new("gh"); + let mut cmd = crate::dependencies::Gh::command() + .ok_or_else(|| anyhow::anyhow!("gh not found on PATH"))?; cmd.arg("pr").arg("view").arg(number.to_string()); if let Some(r) = repo { cmd.arg("--repo").arg(r); @@ -3999,7 +4004,8 @@ fn run_gh_pr_view(number: u32, repo: Option<&str>) -> Result { } fn run_gh_pr_diff(number: u32, repo: Option<&str>) -> Result { - let mut cmd = Command::new("gh"); + let mut cmd = crate::dependencies::Gh::command() + .ok_or_else(|| anyhow::anyhow!("gh not found on PATH"))?; cmd.arg("pr").arg("diff").arg(number.to_string()); if let Some(r) = repo { cmd.arg("--repo").arg(r); @@ -4015,7 +4021,8 @@ fn run_gh_pr_diff(number: u32, repo: Option<&str>) -> Result { } fn run_gh_pr_checkout(number: u32, repo: Option<&str>) -> Result<()> { - let mut cmd = Command::new("gh"); + let mut cmd = crate::dependencies::Gh::command() + .ok_or_else(|| anyhow::anyhow!("gh not found on PATH"))?; cmd.arg("pr").arg("checkout").arg(number.to_string()); if let Some(r) = repo { cmd.arg("--repo").arg(r); @@ -4089,7 +4096,8 @@ fn format_pr_prompt(number: u32, view: &GhPullRequest, diff: &str) -> String { } fn collect_diff(args: &ReviewArgs) -> Result { - let mut cmd = Command::new("git"); + let mut cmd = crate::dependencies::Git::command() + .ok_or_else(|| anyhow::anyhow!("git not found on PATH"))?; cmd.arg("diff"); if args.staged { cmd.arg("--cached"); @@ -4130,7 +4138,8 @@ fn run_apply(args: ApplyArgs) -> Result<()> { tmp.write_all(patch.as_bytes())?; let tmp_path = tmp.path().to_path_buf(); - let output = Command::new("git") + let output = crate::dependencies::Git::command() + .ok_or_else(|| anyhow::anyhow!("git not found on PATH"))? .arg("apply") .arg("--whitespace=nowarn") .arg(&tmp_path) diff --git a/crates/tui/src/repl/runtime.rs b/crates/tui/src/repl/runtime.rs index 8286ef6c9..29072cce5 100644 --- a/crates/tui/src/repl/runtime.rs +++ b/crates/tui/src/repl/runtime.rs @@ -25,11 +25,11 @@ use std::time::{Duration, Instant}; use serde::{Deserialize, Serialize}; use serde_json::Value; use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; -use tokio::process::{Child, ChildStdin, ChildStdout, Command}; +use tokio::process::{Child, ChildStdin, ChildStdout}; use uuid::Uuid; use crate::child_env; -use crate::dependencies::{PYTHON_CANDIDATES, resolve_python_interpreter, split_interpreter_spec}; +use crate::dependencies::ExternalTool; // --------------------------------------------------------------------------- // Public types @@ -203,22 +203,12 @@ impl PythonRuntime { let session_id = Uuid::new_v4().simple().to_string(); let bootstrap = render_bootstrap(&session_id); - let interpreter = resolve_python_interpreter().ok_or_else(|| { - format!( - "no Python interpreter found on PATH (tried {PYTHON_CANDIDATES:?}). \ - Install Python 3 and ensure one of these commands works, then restart codewhale.", - ) + let mut cmd = crate::dependencies::Python::tokio_command().ok_or_else(|| { + "no Python interpreter found on PATH (tried python3, python, py -3). \ + Install Python 3 and restart deepseek-tui." + .to_string() })?; - let (program, interpreter_args) = split_interpreter_spec(&interpreter); - if program.is_empty() { - return Err(format!( - "resolved Python interpreter is empty: {interpreter:?}" - )); - } - - let mut cmd = Command::new(&program); - cmd.args(&interpreter_args) - .arg("-u") + cmd.arg("-u") .arg("-c") .arg(&bootstrap) .stdin(Stdio::piped()) @@ -238,16 +228,16 @@ impl PythonRuntime { let mut child = cmd .spawn() - .map_err(|e| format!("failed to spawn Python interpreter `{interpreter}`: {e}"))?; + .map_err(|e| format!("failed to spawn Python interpreter: {e}"))?; let stdin = child .stdin .take() - .ok_or_else(|| format!("Python interpreter `{interpreter}` stdin pipe missing"))?; + .ok_or_else(|| "Python interpreter stdin pipe missing".to_string())?; let raw_stdout = child .stdout .take() - .ok_or_else(|| format!("Python interpreter `{interpreter}` stdout pipe missing"))?; + .ok_or_else(|| "Python interpreter stdout pipe missing".to_string())?; let stdout = BufReader::new(raw_stdout); let mut rt = Self { @@ -271,14 +261,12 @@ impl PythonRuntime { Ok(Ok(())) => Ok(rt), Ok(Err(e)) => { let _ = rt.child.kill().await; - Err(format!( - "Python interpreter `{interpreter}` bootstrap failed: {e}" - )) + Err(format!("Python interpreter bootstrap failed: {e}")) } Err(_) => { let _ = rt.child.kill().await; Err(format!( - "Python interpreter `{interpreter}` bootstrap did not signal ready within {}s", + "Python interpreter bootstrap did not signal ready within {}s", SPAWN_READY_TIMEOUT.as_secs() )) } @@ -943,11 +931,10 @@ if _ctx_file: except Exception as e: _sys.stderr.write(f"[bootstrap] failed to load context: {e}\n") content = _context -_ctx = _context _BOOTSTRAP_NAMES = { "_SID","_REQ","_RESP","_FINAL","_ERR","_RUN","_END","_DONE","_READY", - "_rpc","_ctx_file","_context","_ctx","_slice_chars","_slice_lines","_BOOTSTRAP_NAMES","_main_loop", + "_rpc","_ctx_file","_context","_slice_chars","_slice_lines","_BOOTSTRAP_NAMES","_main_loop", "_emit_final","_json_safe","_slice_text","_prompt_with_slice", "_normalize_dependency_mode","_batch_dependency_error", "llm_query","llm_query_batched","rlm_query","rlm_query_batched", @@ -1146,10 +1133,10 @@ mod tests { .await .expect("spawn"); let round = rt - .execute("print(content == _context, _ctx == _context, 'context' in globals(), 'ctx' in globals())") + .execute("print(content == _context, 'context' in globals(), 'ctx' in globals())") .await .expect("execute"); - assert!(round.stdout.contains("True True False False")); + assert!(round.stdout.contains("True False False")); rt.shutdown().await; } diff --git a/crates/tui/src/runtime_api.rs b/crates/tui/src/runtime_api.rs index 11d2e4347..6473ed2d8 100644 --- a/crates/tui/src/runtime_api.rs +++ b/crates/tui/src/runtime_api.rs @@ -5,7 +5,6 @@ use std::convert::Infallible; use std::fs; use std::net::{SocketAddr, UdpSocket}; use std::path::{Path as FsPath, PathBuf}; -use std::process::Command; use std::sync::Arc; use std::time::Duration; @@ -28,6 +27,8 @@ use tokio::sync::Mutex; use tokio_util::sync::CancellationToken; use tower_http::cors::{Any, CorsLayer}; +use crate::dependencies::ExternalTool; + use crate::automation_manager::{ AutomationManager, AutomationRecord, AutomationRunRecord, AutomationSchedulerConfig, CreateAutomationRequest, SharedAutomationManager, UpdateAutomationRequest, spawn_scheduler, @@ -1881,11 +1882,7 @@ fn collect_workspace_status(workspace: &std::path::Path) -> WorkspaceStatusRespo } fn run_git(workspace: &std::path::Path, args: &[&str]) -> Option { - let output = Command::new("git") - .args(args) - .current_dir(workspace) - .output() - .ok()?; + let output = crate::dependencies::Git::output(args, workspace).ok()?; if !output.status.success() { return None; } diff --git a/crates/tui/src/snapshot/repo.rs b/crates/tui/src/snapshot/repo.rs index c982db2da..fe016fad3 100644 --- a/crates/tui/src/snapshot/repo.rs +++ b/crates/tui/src/snapshot/repo.rs @@ -15,9 +15,11 @@ use std::collections::HashSet; use std::io; use std::path::{Component, Path, PathBuf}; -use std::process::{Command, Output}; +use std::process::Output; use std::time::{Duration, SystemTime, UNIX_EPOCH}; +use crate::dependencies::ExternalTool; + use super::paths::{ensure_snapshot_dir, snapshot_git_dir}; /// Identifier for a snapshot — currently the underlying git commit SHA. @@ -249,7 +251,8 @@ impl SnapshotRepo { // and stores metadata in `.git`. We then continue to use // explicit `--git-dir` / `--work-tree` flags for every other // command so behaviour is invariant of cwd. - let init = Command::new("git") + let init = crate::dependencies::Git::command() + .ok_or_else(|| io_other("git not found on PATH"))? .arg("init") .arg("--quiet") .arg(parent) @@ -815,7 +818,8 @@ fn cleanup_stale_pack_temps_in( } fn run_git(git_dir: &Path, work_tree: &Path, args: &[&str]) -> io::Result { - Command::new("git") + crate::dependencies::Git::command() + .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, "git not found on PATH"))? .arg("--git-dir") .arg(git_dir) .arg("--work-tree") @@ -1049,7 +1053,8 @@ mod tests { let tmp = tempdir().unwrap(); let workspace = tmp.path().join("workspace"); std::fs::create_dir_all(&workspace).unwrap(); - Command::new("git") + crate::dependencies::Git::command() + .expect("git not found") .arg("-C") .arg(&workspace) .arg("init") @@ -1057,14 +1062,16 @@ mod tests { .status() .unwrap(); std::fs::write(workspace.join("tracked.txt"), b"committed").unwrap(); - Command::new("git") + crate::dependencies::Git::command() + .expect("git not found") .arg("-C") .arg(&workspace) .arg("add") .arg("tracked.txt") .status() .unwrap(); - Command::new("git") + crate::dependencies::Git::command() + .expect("git not found") .arg("-C") .arg(&workspace) .arg("-c") @@ -1077,7 +1084,8 @@ mod tests { .arg("init") .status() .unwrap(); - let user_head_before = Command::new("git") + let user_head_before = crate::dependencies::Git::command() + .expect("git not found") .arg("-C") .arg(&workspace) .args(["rev-parse", "HEAD"]) @@ -1093,7 +1101,8 @@ mod tests { repo.snapshot("post-turn:1").unwrap(); repo.restore(&id).unwrap(); - let user_head_after = Command::new("git") + let user_head_after = crate::dependencies::Git::command() + .expect("git not found") .arg("-C") .arg(&workspace) .args(["rev-parse", "HEAD"]) diff --git a/crates/tui/src/tools/diagnostics.rs b/crates/tui/src/tools/diagnostics.rs index b03011da6..f09d00841 100644 --- a/crates/tui/src/tools/diagnostics.rs +++ b/crates/tui/src/tools/diagnostics.rs @@ -228,26 +228,18 @@ fn run_command(program: &str, args: &[&str], cwd: &Path) -> CommandProbe { #[cfg(test)] mod tests { use super::*; + use crate::dependencies::ExternalTool; use std::fs; use std::path::Path; - use std::process::Command; use tempfile::tempdir; fn git_available() -> bool { - Command::new("git") - .arg("--version") - .output() - .map(|o| o.status.success()) - .unwrap_or(false) + crate::dependencies::Git::available() } fn init_git_repo(root: &Path) { let run = |args: &[&str]| { - let status = Command::new("git") - .args(args) - .current_dir(root) - .status() - .expect("git should spawn"); + let status = crate::dependencies::Git::status(args, root).expect("git should spawn"); assert!(status.success(), "git {args:?} failed"); }; run(&["init", "-q"]); diff --git a/crates/tui/src/tools/git.rs b/crates/tui/src/tools/git.rs index 0efef659b..27540df4a 100644 --- a/crates/tui/src/tools/git.rs +++ b/crates/tui/src/tools/git.rs @@ -5,11 +5,12 @@ use std::fs; use std::path::{Path, PathBuf}; -use std::process::Command; use async_trait::async_trait; use serde_json::{Value, json}; +use crate::dependencies::ExternalTool; + use super::spec::{ ApprovalRequirement, ToolCapability, ToolContext, ToolError, ToolResult, ToolSpec, optional_bool, optional_str, optional_u64, @@ -63,6 +64,8 @@ impl ToolSpec for GitStatusTool { let git_ctx = resolve_git_context(context, optional_str(&input, "path"))?; let mut args = vec![ + "-c".to_string(), + "core.quotepath=false".to_string(), "status".to_string(), "--porcelain=v1".to_string(), "-b".to_string(), @@ -72,11 +75,8 @@ impl ToolSpec for GitStatusTool { args.push(pathspec.display().to_string()); } - let mut status_args = vec!["-c".to_string(), "core.quotepath=false".to_string()]; - status_args.extend(args); - - let command_str = format_command(&git_ctx.working_dir, &status_args); - let output = run_git_command(&git_ctx.working_dir, &status_args)?; + let command_str = format_command(&git_ctx.working_dir, &args); + let output = run_git_command(&git_ctx.working_dir, &args)?; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); @@ -158,6 +158,8 @@ impl ToolSpec for GitDiffTool { let unified = optional_u64(&input, "unified", DEFAULT_UNIFIED).min(MAX_UNIFIED); let mut args = vec![ + "-c".to_string(), + "core.quotepath=false".to_string(), "diff".to_string(), "--no-color".to_string(), "--no-ext-diff".to_string(), @@ -260,7 +262,11 @@ fn pathspec_from(working_dir: &Path, resolved: &Path) -> PathBuf { } fn run_git_command(working_dir: &Path, args: &[String]) -> Result { - let mut cmd = Command::new("git"); + let Some(mut cmd) = crate::dependencies::Git::command() else { + return Err(ToolError::not_available( + "git is not installed or not in PATH", + )); + }; cmd.args(args).current_dir(working_dir); cmd.output().map_err(|e| { if e.kind() == std::io::ErrorKind::NotFound { @@ -314,25 +320,16 @@ fn char_boundary_index(text: &str, max_chars: usize) -> usize { mod tests { use super::*; use std::fs; - use std::process::Command; use tempfile::tempdir; fn git_available() -> bool { - Command::new("git") - .arg("--version") - .output() - .map(|o| o.status.success()) - .unwrap_or(false) + crate::dependencies::Git::available() } fn init_git_repo(root: &Path) { let run = |args: &[&str]| { - let status = Command::new("git") - .args(args) - .current_dir(root) - .status() - .expect("git should spawn"); - assert!(status.success(), "git {args:?} failed"); + let status = crate::dependencies::Git::status(args, root).expect("git should spawn"); + assert!(status.success(), "git {:?} failed", args); }; run(&["init", "-q"]); @@ -342,12 +339,8 @@ mod tests { fn commit_all(root: &Path, message: &str) { let run = |args: &[&str]| { - let status = Command::new("git") - .args(args) - .current_dir(root) - .status() - .expect("git should spawn"); - assert!(status.success(), "git {args:?} failed"); + let status = crate::dependencies::Git::status(args, root).expect("git should spawn"); + assert!(status.success(), "git {:?} failed", args); }; run(&["add", "."]); run(&["commit", "-q", "-m", message]); @@ -375,6 +368,38 @@ mod tests { assert!(result.content.contains("file.txt")); } + #[tokio::test] + async fn git_status_reports_unquoted_unicode_paths() { + if !git_available() { + return; + } + + let tmp = tempdir().expect("tempdir"); + init_git_repo(tmp.path()); + + let file = tmp.path().join("中文-данные.txt"); + fs::write(&file, "hello\n").expect("write"); + commit_all(tmp.path(), "init"); + + fs::write(&file, "hello\nworld\n").expect("modify"); + + let ctx = ToolContext::new(tmp.path()); + let tool = GitStatusTool; + let result = tool.execute(json!({}), &ctx).await.expect("execute"); + assert!(result.success); + assert!( + result + .metadata + .as_ref() + .and_then(|m| m.get("command")) + .and_then(Value::as_str) + .is_some_and(|command| command.contains("-c core.quotepath=false")) + ); + assert!(result.content.contains("中文-данные.txt")); + assert!(!result.content.contains("\\344")); + assert!(!result.content.contains("\\320")); + } + #[tokio::test] async fn git_diff_supports_cached_and_path_scoping() { if !git_available() { @@ -402,11 +427,8 @@ mod tests { assert!(uncached.content.contains("diff --git")); assert!(uncached.content.contains("lib.rs")); - let _ = Command::new("git") - .args(["add", "src/lib.rs"]) - .current_dir(tmp.path()) - .status() - .expect("git add"); + let _ = + crate::dependencies::Git::status(&["add", "src/lib.rs"], tmp.path()).expect("git add"); let cached = tool .execute(json!({ "path": "src", "cached": true }), &ctx) @@ -425,42 +447,37 @@ mod tests { } #[tokio::test] - async fn git_status_reports_unquoted_unicode_paths() { + async fn git_diff_reports_unquoted_unicode_paths() { if !git_available() { return; } + let tmp = tempdir().expect("tempdir"); init_git_repo(tmp.path()); - let run_git = |args: &[&str]| { - let status = Command::new("git") - .args(args) - .current_dir(tmp.path()) - .status() - .expect("git should spawn"); - assert!(status.success(), "git {args:?} failed"); - }; - - run_git(&["config", "core.quotepath", "true"]); - - let workdir = tmp.path().join("中文目录"); - fs::create_dir_all(&workdir).expect("mkdir"); - let file = workdir.join("新文件.md"); + let unicode_name = "\u{4e2d}\u{6587}-\u{0434}\u{0430}\u{043d}\u{043d}\u{044b}\u{0435}.txt"; + let file = tmp.path().join(unicode_name); fs::write(&file, "hello\n").expect("write"); - commit_all(tmp.path(), "init unicode path"); + commit_all(tmp.path(), "init"); fs::write(&file, "hello\nworld\n").expect("modify"); let ctx = ToolContext::new(tmp.path()); - let tool = GitStatusTool; + let tool = GitDiffTool; let result = tool.execute(json!({}), &ctx).await.expect("execute"); assert!(result.success); assert!( - result.content.contains("中文目录/新文件.md"), - "expected decoded unicode filename in git_status output, got: {}", - result.content + result + .metadata + .as_ref() + .and_then(|m| m.get("command")) + .and_then(Value::as_str) + .is_some_and(|command| command.contains("-c core.quotepath=false")) ); + assert!(result.content.contains(unicode_name)); + assert!(!result.content.contains("\\344")); + assert!(!result.content.contains("\\320")); } #[test] diff --git a/crates/tui/src/tools/git_history.rs b/crates/tui/src/tools/git_history.rs index 308a24dc9..2062a9da7 100644 --- a/crates/tui/src/tools/git_history.rs +++ b/crates/tui/src/tools/git_history.rs @@ -5,7 +5,7 @@ use std::fs; use std::path::{Path, PathBuf}; -use std::process::{Command, Output}; +use std::process::Output; use async_trait::async_trait; use serde_json::{Value, json}; @@ -14,6 +14,7 @@ use super::spec::{ ApprovalRequirement, ToolCapability, ToolContext, ToolError, ToolResult, ToolSpec, optional_bool, optional_str, optional_u64, required_str, }; +use crate::dependencies::ExternalTool; const MAX_OUTPUT_CHARS: usize = 40_000; const DEFAULT_LOG_MAX_COUNT: u64 = 20; @@ -445,7 +446,11 @@ fn pathspec_from(working_dir: &Path, resolved: &Path) -> PathBuf { } fn run_git_command(working_dir: &Path, args: &[String]) -> Result { - let mut cmd = Command::new("git"); + let Some(mut cmd) = crate::dependencies::Git::command() else { + return Err(ToolError::not_available( + "git is not installed or not in PATH", + )); + }; cmd.args(args).current_dir(working_dir); cmd.output().map_err(|e| { if e.kind() == std::io::ErrorKind::NotFound { @@ -500,24 +505,15 @@ mod tests { use super::*; use std::fs; use std::path::Path; - use std::process::Command; use tempfile::tempdir; fn git_available() -> bool { - Command::new("git") - .arg("--version") - .output() - .map(|o| o.status.success()) - .unwrap_or(false) + crate::dependencies::Git::available() } fn run_git(root: &Path, args: &[&str]) { - let status = Command::new("git") - .args(args) - .current_dir(root) - .status() - .expect("git should spawn"); - assert!(status.success(), "git {args:?} failed"); + let status = crate::dependencies::Git::status(args, root).expect("git should spawn"); + assert!(status.success(), "git {:?} failed", args); } fn init_git_repo(root: &Path) { diff --git a/crates/tui/src/tools/github.rs b/crates/tui/src/tools/github.rs index 521724af3..b2a0ad507 100644 --- a/crates/tui/src/tools/github.rs +++ b/crates/tui/src/tools/github.rs @@ -3,6 +3,7 @@ use std::path::{Path, PathBuf}; use std::process::Command; +use crate::dependencies::ExternalTool; use async_trait::async_trait; use chrono::Utc; use serde_json::{Value, json}; @@ -430,11 +431,11 @@ fn run_gh_json(context: &ToolContext, args: &[&str]) -> Result } fn ensure_github_repo(context: &ToolContext) -> Result<(), ToolError> { - let out = Command::new("git") - .args(["rev-parse", "--is-inside-work-tree"]) - .current_dir(&context.workspace) - .output() - .map_err(|e| ToolError::execution_failed(format!("failed to run git: {e}")))?; + let out = crate::dependencies::Git::output( + &["rev-parse", "--is-inside-work-tree"], + &context.workspace, + ) + .map_err(|e| ToolError::execution_failed(format!("failed to run git: {e}")))?; if out.status.success() { Ok(()) } else { @@ -445,10 +446,7 @@ fn ensure_github_repo(context: &ToolContext) -> Result<(), ToolError> { } fn git_status_porcelain(context: &ToolContext) -> Result { - let out = Command::new("git") - .args(["status", "--porcelain"]) - .current_dir(&context.workspace) - .output() + let out = crate::dependencies::Git::output(&["status", "--porcelain"], &context.workspace) .map_err(|e| ToolError::execution_failed(format!("failed to run git status: {e}")))?; Ok(String::from_utf8_lossy(&out.stdout).to_string()) } diff --git a/crates/tui/src/tools/js_execution.rs b/crates/tui/src/tools/js_execution.rs index 3bedef6be..b2436c971 100644 --- a/crates/tui/src/tools/js_execution.rs +++ b/crates/tui/src/tools/js_execution.rs @@ -17,6 +17,7 @@ use std::path::Path; use std::time::Duration; +use crate::dependencies::ExternalTool; use serde_json::{Value, json}; use crate::models::Tool; @@ -73,20 +74,8 @@ pub async fn execute_js_execution_tool( ) -> Result { let code = required_str(input, "code")?; - // Resolve the Node runtime we cached at catalog-build time. If - // it's absent now (somehow registered but disappeared between - // startup and this call — concurrent uninstall, PATH change) - // fail fast with a clear message rather than dropping into - // `tokio::process::Command::new("node")` and surfacing the - // generic "program not found" error. - let node = crate::dependencies::resolve_node().ok_or_else(|| { - ToolError::execution_failed( - "js_execution: no Node.js runtime found on PATH (tried `node`). \ - Install Node 18+ and ensure `node` is on PATH, then restart \ - codewhale." - .to_string(), - ) - })?; + // Resolve the Node runtime via ExternalTool. If it's absent now + // tokio_command() returns None and we fail fast with a clear message. let temp_dir = tempfile::tempdir() .map_err(|e| ToolError::execution_failed(format!("tempdir failed: {e}")))?; @@ -95,9 +84,10 @@ pub async fn execute_js_execution_tool( .await .map_err(|e| ToolError::execution_failed(format!("tempfile write failed: {e}")))?; - let mut cmd = tokio::process::Command::new(&node); - cmd.arg(&script_path); - cmd.current_dir(workspace); + let mut cmd = crate::dependencies::Node::tokio_command().ok_or_else(|| { + ToolError::execution_failed("js_execution: Node.js runtime became unavailable".to_string()) + })?; + cmd.arg(&script_path).current_dir(workspace); let output = tokio::time::timeout(Duration::from_secs(120), cmd.output()) .await diff --git a/crates/tui/src/tools/review.rs b/crates/tui/src/tools/review.rs index 92f166455..de597f5b3 100644 --- a/crates/tui/src/tools/review.rs +++ b/crates/tui/src/tools/review.rs @@ -2,13 +2,13 @@ use std::fs; use std::path::Path; -use std::process::Command; use async_trait::async_trait; use serde::{Deserialize, Serialize}; use serde_json::{Value, json}; use crate::client::DeepSeekClient; +use crate::dependencies::ExternalTool; use crate::llm_client::LlmClient; use crate::models::{ContentBlock, Message, MessageRequest, SystemPrompt, Usage}; use crate::utils::truncate_with_ellipsis; @@ -361,7 +361,9 @@ fn resolve_diff_target( staged: bool, base: Option<&str>, ) -> Result { - let mut cmd = Command::new("git"); + let Some(mut cmd) = crate::dependencies::Git::command() else { + return Err(ToolError::execution_failed("git not found")); + }; cmd.arg("diff"); if staged { cmd.arg("--cached"); @@ -391,7 +393,9 @@ fn resolve_diff_target( } fn gh_pr_diff(pr: &PullRequestRef, workspace: &Path) -> Result { - let mut cmd = Command::new("gh"); + let Some(mut cmd) = crate::dependencies::Gh::command() else { + return Err(ToolError::execution_failed("gh not found")); + }; cmd.arg("pr") .arg("diff") .arg(&pr.number) diff --git a/crates/tui/src/tools/tasks.rs b/crates/tui/src/tools/tasks.rs index 2c3ce95b9..811d14f2f 100644 --- a/crates/tui/src/tools/tasks.rs +++ b/crates/tui/src/tools/tasks.rs @@ -11,6 +11,7 @@ use tokio::process::Command; use uuid::Uuid; use crate::command_safety::{SafetyLevel, analyze_command}; +use crate::dependencies::ExternalTool; use crate::task_manager::{ NewTaskRequest, TaskArtifactRef, TaskAttemptRecord, TaskGateRecord, TaskRecord, }; @@ -24,6 +25,23 @@ const MAX_SUMMARY_CHARS: usize = 900; const DEFAULT_GATE_TIMEOUT_MS: u64 = 120_000; const MAX_GATE_TIMEOUT_MS: u64 = 600_000; +fn build_gate_command_parts(command: &str) -> (String, Vec) { + ( + "/bin/sh".to_string(), + vec!["-lc".to_string(), command.to_string()], + ) +} + +fn build_gate_command(command: &str, cwd: &Path) -> Command { + let (program, args) = build_gate_command_parts(command); + let mut cmd = Command::new(program); + cmd.args(args) + .current_dir(cwd) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + cmd +} + pub struct TaskCreateTool; pub struct TaskListTool; pub struct TaskReadTool; @@ -287,12 +305,7 @@ impl ToolSpec for TaskGateRunTool { } let started = Instant::now(); - let mut cmd = Command::new("/bin/sh"); - cmd.arg("-lc") - .arg(&command) - .current_dir(&cwd) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()); + let mut cmd = build_gate_command(&command, &cwd); let output = tokio::time::timeout(std::time::Duration::from_millis(timeout_ms), cmd.output()).await; @@ -748,13 +761,20 @@ impl ToolSpec for PrAttemptPreflightTool { .as_ref() .ok_or_else(|| ToolError::invalid_input("Attempt has no patch artifact"))?; let patch_path = manager.artifact_absolute_path(patch_ref); - let out = Command::new("git") - .args(["apply", "--check"]) - .arg(&patch_path) - .current_dir(&context.workspace) - .output() - .await - .map_err(|e| ToolError::execution_failed(format!("git apply --check failed: {e}")))?; + let workspace = context.workspace.clone(); + let out = tokio::task::spawn_blocking(move || { + crate::dependencies::Git::command() + .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "git not found"))? + .args(["apply", "--check"]) + .arg(&patch_path) + .current_dir(&workspace) + .output() + }) + .await + .map_err(|join_err| { + ToolError::execution_failed(format!("git apply --check panicked: {join_err}")) + })? + .map_err(|e| ToolError::execution_failed(format!("git apply --check failed: {e}")))?; let stdout = String::from_utf8_lossy(&out.stdout).to_string(); let stderr = String::from_utf8_lossy(&out.stderr).to_string(); Ok(ToolResult::json(&json!({ @@ -900,10 +920,7 @@ fn task_id_schema() -> Value { } fn git_output(workspace: &Path, args: &[&str]) -> Result { - let out = std::process::Command::new("git") - .args(args) - .current_dir(workspace) - .output() + let out = crate::dependencies::Git::output(args, workspace) .map_err(|e| ToolError::execution_failed(format!("failed to run git: {e}")))?; if !out.status.success() { return Err(ToolError::execution_failed(format!( @@ -1009,4 +1026,11 @@ mod tests { assert_eq!(wait_schema["required"][0], "task_id"); assert!(wait_schema["properties"]["gate"].is_object()); } + + #[test] + fn gate_command_uses_login_shell_invocation() { + let (program, args) = build_gate_command_parts("echo hello"); + assert_eq!(program, "/bin/sh"); + assert_eq!(args, vec!["-lc".to_string(), "echo hello".to_string()]); + } } diff --git a/crates/tui/src/tools/test_runner.rs b/crates/tui/src/tools/test_runner.rs index 6bbe42c44..9cf3b49d8 100644 --- a/crates/tui/src/tools/test_runner.rs +++ b/crates/tui/src/tools/test_runner.rs @@ -4,7 +4,6 @@ //! approval policy as the other code-executing tools. use std::path::Path; -use std::process::Command; use async_trait::async_trait; use serde::{Deserialize, Serialize}; @@ -16,6 +15,8 @@ use super::spec::{ optional_bool, optional_str, }; +use crate::dependencies::ExternalTool; + const MAX_OUTPUT_CHARS: usize = 40_000; /// Tool for running `cargo test` in the workspace root. @@ -121,7 +122,11 @@ impl ToolSpec for RunTestsTool { // === Helpers === fn run_cargo(workspace: &Path, args: &[String]) -> Result { - let mut cmd = Command::new("cargo"); + let Some(mut cmd) = crate::dependencies::Cargo::command() else { + return Err(ToolError::not_available( + "cargo is not installed or not in PATH", + )); + }; cmd.args(args).current_dir(workspace); cmd.output().map_err(|e| { if e.kind() == std::io::ErrorKind::NotFound { @@ -189,7 +194,8 @@ mod tests { fn init_cargo_project(root: &Path) -> std::path::PathBuf { let project_dir = root.join("project"); fs::create_dir_all(&project_dir).expect("create project dir"); - let status = Command::new("cargo") + let status = crate::dependencies::Cargo::command() + .expect("cargo not found") .args([ "init", "--lib", diff --git a/crates/tui/src/tui/clipboard.rs b/crates/tui/src/tui/clipboard.rs index bbefcac85..f6d92b256 100644 --- a/crates/tui/src/tui/clipboard.rs +++ b/crates/tui/src/tui/clipboard.rs @@ -7,17 +7,12 @@ //! endpoint, so we materialize the bytes to disk instead of base64-embedding //! them in the request). -#[cfg(any(not(test), all(test, unix)))] -use std::io::Write; #[cfg(not(test))] -use std::io::{self, IsTerminal}; +use std::io::{self, IsTerminal, Write}; use std::path::{Path, PathBuf}; -#[cfg(any( - all(test, unix), - all( - any(target_os = "macos", target_os = "windows", target_os = "linux"), - not(test) - ) +#[cfg(all( + any(target_os = "macos", target_os = "windows", target_os = "linux"), + not(test) ))] use std::process::{Command, Stdio}; use std::time::{SystemTime, UNIX_EPOCH}; @@ -87,10 +82,10 @@ impl ClipboardHandler { /// /// On Linux, `arboard::Clipboard::new()` opens a blocking X11 connection. /// When no X server is running (headless, WSL2 without WSLg), the connect - /// call can hang indefinitely. We spawn the connection attempt on a + /// call can hang indefinitely. We spawn the connection attempt on a /// temporary thread and give it 500 ms; if it doesn't return in time the /// handler stays in fallback/no-op mode and `read`/`write_text` fall - /// through to their OSC 52 and pbcopy/powershell fallbacks. + /// through to their OSC 52 and pbcopy/powershell fallbacks. fn ensure_clipboard(&mut self) { if self.clipboard_init_attempted { return; @@ -101,8 +96,6 @@ impl ClipboardHandler { std::thread::spawn(move || { let _ = tx.send(Clipboard::new().ok()); }); - // 500 ms is generous for a local Unix socket connect — the - // kernel either answers or doesn't. self.clipboard = rx .recv_timeout(std::time::Duration::from_millis(500)) .ok() @@ -187,12 +180,39 @@ fn write_text_with_set_clipboard(text: &str) -> Result<()> { ) } +#[cfg(all(any(target_os = "macos", target_os = "windows"), not(test)))] +fn write_text_with_stdin_command( + program: &str, + args: &[&str], + text: &str, + label: &str, +) -> Result<()> { + let mut child = Command::new(program) + .args(args) + .stdin(Stdio::piped()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .map_err(|e| anyhow::anyhow!("Failed to run {label}: {e}"))?; + if let Some(mut stdin) = child.stdin.take() { + stdin + .write_all(text.as_bytes()) + .map_err(|e| anyhow::anyhow!("Failed to write to {label}: {e}"))?; + } + let _ = std::thread::Builder::new() + .name("clipboard-wait".to_string()) + .spawn(move || { + let _ = child.wait(); + }); + Ok(()) +} + #[cfg(all(target_os = "linux", not(test)))] fn write_text_with_wlcopy(text: &str) -> Result<()> { write_text_with_wlcopy_using_argv("wl-copy", text) } -#[cfg(target_os = "linux")] +#[cfg(all(target_os = "linux", not(test)))] fn write_text_with_wlcopy_using_argv(program: &str, text: &str) -> Result<()> { let mut child = Command::new(program) .stdin(Stdio::piped()) @@ -215,36 +235,6 @@ fn write_text_with_wlcopy_using_argv(program: &str, text: &str) -> Result<()> { Ok(()) } -#[cfg(any( - all(test, unix), - all(any(target_os = "macos", target_os = "windows"), not(test)) -))] -fn write_text_with_stdin_command( - program: &str, - args: &[&str], - text: &str, - label: &str, -) -> Result<()> { - let mut child = Command::new(program) - .args(args) - .stdin(Stdio::piped()) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .spawn() - .map_err(|e| anyhow::anyhow!("Failed to run {label}: {e}"))?; - if let Some(mut stdin) = child.stdin.take() { - stdin - .write_all(text.as_bytes()) - .map_err(|e| anyhow::anyhow!("Failed to write to {label}: {e}"))?; - } - let _ = std::thread::Builder::new() - .name("clipboard-wait".to_string()) - .spawn(move || { - let _ = child.wait(); - }); - Ok(()) -} - #[cfg(not(test))] fn write_text_with_osc52(text: &str) -> Result<()> { let mut stdout = io::stdout(); @@ -279,7 +269,7 @@ fn osc52_sequence(text: &str, in_tmux: bool) -> Result { /// `/clipboard-images/` if the home dir is unavailable. pub(crate) fn clipboard_images_dir(workspace: &Path) -> PathBuf { if let Some(home) = dirs::home_dir() { - return home.join(".codewhale").join("clipboard-images"); + return home.join(".deepseek").join("clipboard-images"); } workspace.join("clipboard-images") } @@ -370,48 +360,6 @@ mod tests { assert_eq!(&header[..8], b"\x89PNG\r\n\x1a\n"); } - #[cfg(unix)] - #[test] - fn stdin_clipboard_command_returns_before_helper_exits() { - use std::time::{Duration, Instant}; - - let dir = tempfile::tempdir().unwrap(); - let marker = dir.path().join("clipboard.txt"); - let script = dir.path().join("slow-clipboard.sh"); - std::fs::write(&script, "#!/bin/sh\ncat > \"$1\"\nsleep 1\n").unwrap(); - - use std::os::unix::fs::PermissionsExt; - let mut permissions = std::fs::metadata(&script).unwrap().permissions(); - permissions.set_mode(0o755); - std::fs::set_permissions(&script, permissions).unwrap(); - - let started = Instant::now(); - write_text_with_stdin_command( - script.to_str().unwrap(), - &[marker.to_str().unwrap()], - "copied", - "test-clipboard", - ) - .unwrap(); - assert!( - started.elapsed() < Duration::from_millis(250), - "clipboard helper wait leaked onto caller path" - ); - - let deadline = Instant::now() + Duration::from_secs(2); - let mut last_body = String::new(); - while Instant::now() < deadline { - if let Ok(body) = std::fs::read_to_string(&marker) { - if body == "copied" { - return; - } - last_body = body; - } - std::thread::sleep(Duration::from_millis(20)); - } - panic!("clipboard helper did not receive stdin; last body: {last_body:?}"); - } - #[test] fn pasted_image_labels_format_correctly() { let p = PastedImage { @@ -424,33 +372,6 @@ mod tests { assert_eq!(p.size_label(), "235KB"); } - #[cfg(target_os = "linux")] - #[test] - fn wlcopy_helper_errors_when_binary_missing() { - let result = - write_text_with_wlcopy_using_argv("/nonexistent/path/to/wlcopy_binary_xyz", "test"); - assert!(result.is_err()); - } - - #[cfg(target_os = "linux")] - #[test] - fn wlcopy_helper_errors_when_binary_exits_nonzero() { - let result = write_text_with_wlcopy_using_argv("false", "test"); - assert!(result.is_err()); - } - - #[cfg(target_os = "linux")] - #[test] - fn wlcopy_helper_succeeds_when_binary_returns_zero() { - // Use `cat` instead of `true` because `true` exits immediately - // without reading stdin, causing EPIPE before we can check the - // exit status. `cat` consumes stdin until EOF (when we drop the - // pipe) and then exits 0, faithfully modelling a successful - // wl-copy invocation. - let result = write_text_with_wlcopy_using_argv("cat", "test"); - assert!(result.is_ok()); - } - #[test] fn osc52_sequence_encodes_text_clipboard_write() { let sequence = osc52_sequence("hello", false).expect("sequence"); diff --git a/crates/tui/src/tui/file_picker_relevance.rs b/crates/tui/src/tui/file_picker_relevance.rs index a7fde2e00..dbb07e220 100644 --- a/crates/tui/src/tui/file_picker_relevance.rs +++ b/crates/tui/src/tui/file_picker_relevance.rs @@ -14,9 +14,9 @@ //! make path discovery resilient to quoting, leading `./`, and //! trailing `:line` markers. +use crate::dependencies::{ExternalTool, Git}; use std::collections::HashSet; use std::path::{Path, PathBuf}; -use std::process::Command; use crate::tui::app::App; use crate::tui::app::ToolDetailRecord; @@ -70,7 +70,10 @@ pub(super) fn build_relevance(app: &App) -> FilePickerRelevance { } fn modified_workspace_paths(workspace: &Path) -> Vec { - let Ok(output) = Command::new("git") + let Some(mut cmd) = Git::command() else { + return Vec::new(); + }; + let Ok(output) = cmd .arg("-C") .arg(workspace) .args(["status", "--short", "--untracked-files=normal"]) diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index 7b4f77e52..6d7bda2a0 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -3,8 +3,6 @@ use std::fmt::Write as _; use std::io::{self, Stdout, Write}; use std::path::PathBuf; -#[cfg(any(target_os = "macos", target_os = "linux", target_os = "windows"))] -use std::process::{Command, Stdio}; use std::sync::{Arc, LazyLock}; use std::time::{Duration, Instant}; @@ -5414,12 +5412,15 @@ async fn apply_command_result( Ok(false) } +#[cfg(test)] +use std::process::{Command, Stdio}; + #[cfg(any(target_os = "macos", target_os = "linux", target_os = "windows"))] fn open_external_url(url: &str) -> Result<()> { - spawn_external_url_command(external_url_command(url)) + crate::utils::open_url(url) } -#[cfg(any(target_os = "macos", target_os = "linux", target_os = "windows"))] +#[cfg(test)] fn spawn_external_url_command(mut command: Command) -> Result<()> { command .stdin(Stdio::null()) @@ -5430,34 +5431,6 @@ fn spawn_external_url_command(mut command: Command) -> Result<()> { .map_err(|err| anyhow::anyhow!("failed to launch browser command: {err}")) } -#[cfg(not(any(target_os = "macos", target_os = "linux", target_os = "windows")))] -fn open_external_url(_url: &str) -> Result<()> { - Err(anyhow::anyhow!( - "browser opening is unsupported on this platform" - )) -} - -#[cfg(target_os = "macos")] -fn external_url_command(url: &str) -> Command { - let mut command = Command::new("open"); - command.arg(url); - command -} - -#[cfg(target_os = "linux")] -fn external_url_command(url: &str) -> Command { - let mut command = Command::new("xdg-open"); - command.arg(url); - command -} - -#[cfg(target_os = "windows")] -fn external_url_command(url: &str) -> Command { - let mut command = Command::new("cmd"); - command.args(["/C", "start", "", url]); - command -} - fn apply_workspace_runtime_state(app: &mut App, config: &Config, workspace: PathBuf) { app.workspace = workspace.clone(); app.hooks = HookExecutor::new(config.hooks_config(), workspace.clone()); diff --git a/crates/tui/src/tui/ui/tests.rs b/crates/tui/src/tui/ui/tests.rs index 0f2677c4a..308afc196 100644 --- a/crates/tui/src/tui/ui/tests.rs +++ b/crates/tui/src/tui/ui/tests.rs @@ -6653,6 +6653,33 @@ fn composer_arrows_scroll_config_overrides_default() { ); } +#[test] +fn history_arrow_down_handles_empty_input() { + let mut app = create_test_app(); + app.composer_arrows_scroll = false; + app.input_history.push("older".to_string()); + app.input_history.push("newer".to_string()); + + // Empty composer + Up → newest entry (draft saved as empty string). + assert!(handle_composer_history_arrow( + &mut app, + KeyEvent::new(KeyCode::Up, KeyModifiers::NONE), + false, + false, + )); + assert_eq!(app.input, "newer"); + + // Down from newest → end of history → restores the saved empty draft. + assert!(handle_composer_history_arrow( + &mut app, + KeyEvent::new(KeyCode::Down, KeyModifiers::NONE), + false, + false, + )); + assert!(app.input.is_empty()); + assert!(app.history_index.is_none()); +} + #[test] fn home_jumps_to_line_start_multiline() { let mut app = create_test_app(); diff --git a/crates/tui/src/tui/workspace_context.rs b/crates/tui/src/tui/workspace_context.rs index cfe04618c..696ac83ef 100644 --- a/crates/tui/src/tui/workspace_context.rs +++ b/crates/tui/src/tui/workspace_context.rs @@ -7,8 +7,8 @@ //! current Tokio runtime; tests and non-async callers fall through to //! a synchronous call. +use crate::dependencies::{ExternalTool, Git}; use std::path::Path; -use std::process::Command; use std::time::{Duration, Instant}; use crate::tui::app::App; @@ -170,10 +170,7 @@ fn change_summary(workspace: &Path) -> Option { } fn run_git(workspace: &Path, args: &[&str]) -> std::io::Result { - let output = Command::new("git") - .args(args) - .current_dir(workspace) - .output()?; + let output = Git::output(args, workspace)?; if !output.status.success() { return Err(std::io::Error::other("git command failed")); } diff --git a/crates/tui/src/utils.rs b/crates/tui/src/utils.rs index 15c231990..e7ea299e6 100644 --- a/crates/tui/src/utils.rs +++ b/crates/tui/src/utils.rs @@ -3,6 +3,7 @@ use std::fs; use std::io::Write; use std::path::{Path, PathBuf}; +use std::process::Command; use crate::models::{ContentBlock, Message}; use anyhow::{Context, Result}; @@ -208,6 +209,54 @@ pub fn flush_and_sync(writer: &mut std::io::BufWriter) -> std::io writer.get_ref().sync_all() } +/// Open a URL in the system's default browser. +/// +/// Dispatches to the platform-appropriate opener: +/// - macOS: `open` +/// - Linux: `xdg-open` +/// - Windows: `cmd /C start ""` +/// - Other: returns an error. +/// +/// This is the single entry point for URL opening — every call site in +/// the codebase should use this instead of hardcoding `Command::new("open")`, +/// `Command::new("xdg-open")`, or `Command::new("cmd")`. +pub fn open_url(url: &str) -> Result<()> { + #[cfg(target_os = "macos")] + let mut command = Command::new("open"); + #[cfg(target_os = "linux")] + let mut command = Command::new("xdg-open"); + #[cfg(target_os = "windows")] + let _command = { + let mut cmd = Command::new("cmd"); + cmd.args(["/C", "start", "", url]); + return match cmd + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .spawn() + { + Ok(_) => Ok(()), + Err(e) => Err(anyhow::anyhow!("failed to launch browser command: {e}")), + }; + }; + + // macOS / Linux path + #[cfg(any(target_os = "macos", target_os = "linux"))] + { + command.arg(url); + command + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .spawn() + .map(|_| ()) + .map_err(|e| anyhow::anyhow!("failed to launch browser command: {e}")) + } + + #[cfg(not(any(target_os = "macos", target_os = "linux", target_os = "windows")))] + Err(anyhow::anyhow!( + "browser opening is unsupported on this platform" + )) +} + /// Spawn a tokio task with panic supervision. /// /// Wraps the future in `AssertUnwindSafe` + `catch_unwind`. On panic: @@ -773,4 +822,43 @@ mod project_mapping_tests { .expect("prefix"); assert_eq!(suffix, "Makefile, README.md"); } + + // =================================================================== + // open_url tests + // =================================================================== + + #[test] + fn open_url_does_not_panic_on_valid_url() { + // We can't open a browser in CI, but we can verify the function + // doesn't panic and returns a Result (either Ok or Err with a + // meaningful message). + let result = super::open_url("https://example.com"); + match result { + Ok(()) => {} // browser opened — fine + Err(e) => { + let msg = e.to_string(); + // The error must contain something about "browser" or + // "unsupported" — not a random panic message. + assert!( + msg.contains("browser") + || msg.contains("unsupported") + || msg.contains("failed"), + "unexpected error message: {msg}" + ); + } + } + } + + #[test] + fn open_url_rejects_empty_url_gracefully() { + // An empty URL should fail with a clear error, not panic. + let result = super::open_url(""); + match result { + Ok(()) => {} // some openers might accept empty string + Err(e) => { + let msg = e.to_string(); + assert!(!msg.is_empty(), "error message must not be empty"); + } + } + } } From 42b25a2cb59cf6d95df35e44977bfc9ce62925a1 Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Sun, 31 May 2026 02:27:32 -0700 Subject: [PATCH 02/15] docs: add macos python FAQ (#2409) Harvested from #2407 with thanks to @axobase001. Adds a first-time macOS Python FAQ and completes the virtualenv flow with the final run command. Fixes #2351. --- docs/GUIDE.md | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/docs/GUIDE.md b/docs/GUIDE.md index 603625708..fa58b6966 100644 --- a/docs/GUIDE.md +++ b/docs/GUIDE.md @@ -453,6 +453,32 @@ Approvals are part of the safety model. Shell commands, paid tools, writes, and actions outside the expected workspace can have side effects. Approval prompts let you keep control while still letting the model do useful work. +### How do I run a Python file on macOS? + +Open Terminal in the folder that contains the file and run: + +```bash +python3 your_file.py +``` + +If macOS says `python3` is missing, install Python from +[python.org](https://www.python.org/downloads/macos/) or with Homebrew: + +```bash +brew install python +``` + +Inside CodeWhale, ask the agent to inspect the file and run it with +`python3 your_file.py`. If the script needs packages, install them in a virtual +environment first: + +```bash +python3 -m venv .venv +source .venv/bin/activate +python3 -m pip install -r requirements.txt +python3 your_file.py +``` + ### Where is my config stored? New CodeWhale config uses `~/.codewhale/config.toml`. Legacy From a5cef5b2f6b0bb38a67cbf3034e6047f206512b1 Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Sun, 31 May 2026 02:29:21 -0700 Subject: [PATCH 03/15] feat(tui): expose auto route in turn metadata (#2410) Harvested from #2406 with thanks to @axobase001. Adds auto-selected model and reasoning effort to turn metadata while keeping user text isolated in its own content block. Includes the comment cleanup requested in review. Partially addresses #2380. --- crates/tui/src/core/engine.rs | 60 ++++++++++++++++++++++++----- crates/tui/src/core/engine/tests.rs | 26 +++++++++++++ 2 files changed, 76 insertions(+), 10 deletions(-) diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index 785952175..24d28230f 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -910,7 +910,13 @@ impl Engine { self.emit_session_updated().await; } - fn turn_metadata_block(&self) -> ContentBlock { + fn turn_metadata_block( + &self, + routed_model: &str, + auto_model: bool, + reasoning_effort: Option<&str>, + reasoning_effort_auto: bool, + ) -> ContentBlock { let today = chrono::Local::now().format("%Y-%m-%d").to_string(); let working_set_summary = self .session @@ -919,11 +925,17 @@ impl Engine { .map(|s| s.trim().to_string()) .filter(|s| !s.is_empty()); - let summary = if let Some(working_set_summary) = working_set_summary { - format!("Current local date: {today}\n{working_set_summary}") - } else { - format!("Current local date: {today}") - }; + let mut lines = vec![format!("Current local date: {today}")]; + if auto_model { + lines.push(format!("Auto model route: {routed_model}")); + } + if reasoning_effort_auto && let Some(reasoning_effort) = reasoning_effort { + lines.push(format!("Auto reasoning effort: {reasoning_effort}")); + } + if let Some(working_set_summary) = working_set_summary { + lines.push(working_set_summary); + } + let summary = lines.join("\n"); ContentBlock::Text { text: format!("\n{summary}\n"), @@ -932,10 +944,32 @@ impl Engine { } fn user_text_message_with_turn_metadata(&self, text: String) -> Message { + self.user_text_message_with_turn_metadata_for_route( + text, + &self.session.model, + self.session.auto_model, + self.session.reasoning_effort.as_deref(), + self.session.reasoning_effort_auto, + ) + } + + fn user_text_message_with_turn_metadata_for_route( + &self, + text: String, + routed_model: &str, + auto_model: bool, + reasoning_effort: Option<&str>, + reasoning_effort_auto: bool, + ) -> Message { Message { role: "user".to_string(), content: vec![ - self.turn_metadata_block(), + self.turn_metadata_block( + routed_model, + auto_model, + reasoning_effort, + reasoning_effort_auto, + ), ContentBlock::Text { text, cache_control: None, @@ -989,7 +1023,7 @@ impl Engine { // failure is non-fatal (the helper logs at WARN). if self.config.snapshots_enabled { // Clone the user prompt now — `content` is moved into - // `user_text_message_with_turn_metadata` below, so we need + // `user_text_message_with_turn_metadata_for_route` below, so we need // a copy for both pre- and post-turn snapshot labels. The // label carries a truncated first line so `/restore` // listings are human-readable. @@ -1010,7 +1044,7 @@ impl Engine { crate::retry_status::clear(); // Clone user prompt for post-turn snapshot label before `content` - // is moved into `user_text_message_with_turn_metadata` below. + // is moved into `user_text_message_with_turn_metadata_for_route` below. let snapshot_prompt_post = content.clone(); // Check if we have the appropriate client @@ -1041,7 +1075,13 @@ impl Engine { let force_update_plan_first = should_force_update_plan_first(mode, &content); // Add user message to session - let user_msg = self.user_text_message_with_turn_metadata(content); + let user_msg = self.user_text_message_with_turn_metadata_for_route( + content, + &model, + auto_model, + reasoning_effort.as_deref(), + reasoning_effort_auto, + ); self.session.add_message(user_msg); let previous_goal_objective = self.config.goal_objective.clone(); diff --git a/crates/tui/src/core/engine/tests.rs b/crates/tui/src/core/engine/tests.rs index ade5dd690..06ae40306 100644 --- a/crates/tui/src/core/engine/tests.rs +++ b/crates/tui/src/core/engine/tests.rs @@ -1467,6 +1467,32 @@ fn turn_metadata_includes_current_local_date_without_working_set() { assert!(text.contains(&format!("Current local date: {today}"))); } +#[test] +fn turn_metadata_includes_auto_model_route() { + let tmp = tempdir().expect("tempdir"); + let config = EngineConfig { + workspace: tmp.path().to_path_buf(), + ..Default::default() + }; + let (engine, _handle) = Engine::new(config, &Config::default()); + + let user_msg = engine.user_text_message_with_turn_metadata_for_route( + "debug this regression".to_string(), + "deepseek-v4-pro", + true, + Some("max"), + true, + ); + let first_block = user_msg.content.first().expect("turn metadata block"); + let ContentBlock::Text { text, .. } = first_block else { + panic!("expected text metadata block"); + }; + + assert!(text.contains("Auto model route: deepseek-v4-pro")); + assert!(text.contains("Auto reasoning effort: max")); + assert!(!text.contains("debug this regression")); +} + #[test] fn user_text_message_keeps_current_turn_input_after_turn_metadata() { let tmp = tempdir().expect("tempdir"); From 26e2de86f0c9d9694c3dbde505cfc983cfb7f0bb Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Sun, 31 May 2026 02:39:53 -0700 Subject: [PATCH 04/15] docs: align configuration paths with codewhale home (#2413) Harvested from #2400 with thanks to @axobase001.\n\nAligns the configuration docs around CODEWHALE_HOME while preserving the DEEPSEEK_HOME legacy alias and spelling out how legacy checkpoints can still be scanned during cleanup.\n\nCloses #2322. --- docs/CONFIGURATION.md | 73 ++++++++++++++++++++++++++++--------------- 1 file changed, 48 insertions(+), 25 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 8830e71ee..0fe1dd3fe 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -9,22 +9,26 @@ only the provider and safety knobs you need. Default config path: -- `~/.deepseek/config.toml` +- `~/.codewhale/config.toml` +- Legacy fallback: `~/.deepseek/config.toml` Overrides: - CLI: `codewhale --config /path/to/config.toml` -- Env: `DEEPSEEK_CONFIG_PATH=/path/to/config.toml` +- Env: `CODEWHALE_CONFIG_PATH=/path/to/config.toml` +- Legacy env alias: `DEEPSEEK_CONFIG_PATH=/path/to/config.toml` If both are set, `--config` wins. Environment variable overrides are applied after the file is loaded. ### Per-project overlay (#485) When the TUI starts in a workspace that contains a -`/.deepseek/config.toml` file, the values declared in that -file are merged on top of the global config. This lets a repo lock its -own provider, model, sandbox policy, or approval policy without -touching the user's `~/.deepseek/config.toml`. Pass +`/.codewhale/config.toml` file, the values declared in that +file are merged on top of the global config. Legacy +`/.deepseek/config.toml` files are still read when the +CodeWhale path is absent. This lets a repo lock its own provider, +model, sandbox policy, or approval policy without touching the user's +`~/.codewhale/config.toml`. Pass `--no-project-config` to skip the overlay for one launch. Supported keys in the project overlay (top-level fields only): @@ -52,8 +56,9 @@ specific use case. The `codewhale` facade and `codewhale-tui` binary share the same config file for DeepSeek auth and model defaults. `codewhale auth set --provider deepseek` (and the legacy `codewhale login --api-key ...` alias) saves the key to -`~/.deepseek/config.toml`, and `codewhale --model deepseek-v4-flash` is forwarded -to the TUI as `DEEPSEEK_MODEL`. +`~/.codewhale/config.toml` (migrating legacy `~/.deepseek/config.toml` on first +launch when needed), and `codewhale --model deepseek-v4-flash` is forwarded to +the TUI as `DEEPSEEK_MODEL`. Credential lookup uses `config -> keyring -> env` after any explicit CLI `--api-key`. Run `codewhale auth status` to inspect the active provider's config @@ -298,11 +303,17 @@ Remaining variables: - `DEEPSEEK_MANAGED_CONFIG_PATH` - `DEEPSEEK_REQUIREMENTS_PATH` - `DEEPSEEK_MAX_SUBAGENTS` (clamped to `1..=20`) -- `DEEPSEEK_TASKS_DIR` (runtime task queue/artifact storage, default `~/.deepseek/tasks`) +- `DEEPSEEK_TASKS_DIR` (runtime task queue/artifact storage, default + `~/.codewhale/tasks`, with legacy `~/.deepseek/tasks` fallback when only the + legacy directory exists) - `DEEPSEEK_ALLOW_INSECURE_HTTP` (`1`/`true` allows non-local `http://` base URLs; default is reject) - `DEEPSEEK_FORCE_HTTP1` (`1|true|yes|on` pins the HTTP client to HTTP/1.1, disabling HTTP/2; useful on Windows or behind proxies that mishandle long-lived H2 streams) -- `DEEPSEEK_HOME` (override the base data directory; defaults to `~/.deepseek`) -- `DEEPSEEK_AUTOMATIONS_DIR` (override the automations storage directory; defaults to `~/.deepseek/automations`) +- `CODEWHALE_HOME` (override the base data directory; defaults to `~/.codewhale`). + If you previously exported `DEEPSEEK_HOME`, rename it to `CODEWHALE_HOME`; + the old env var is not used for new CodeWhale state paths. +- `DEEPSEEK_AUTOMATIONS_DIR` (override the automations storage directory; uses + `~/.codewhale/automations` when that directory exists, otherwise the legacy + `~/.deepseek/automations` path) - `DEEPSEEK_CAPACITY_ENABLED` - `DEEPSEEK_CAPACITY_LOW_RISK_MAX` - `DEEPSEEK_CAPACITY_MEDIUM_RISK_MAX` @@ -336,7 +347,7 @@ concatenated, in declared order, alongside the auto-loaded ```toml instructions = [ "./AGENTS.md", - "~/.deepseek/global.md", + "~/.codewhale/global.md", "~/team/agents-shared.md", ] ``` @@ -348,7 +359,8 @@ Rules: truncated with a `[…elided]` marker rather than skipped. - Missing files are skipped with a tracing warning so a stale entry doesn't fail the launch. -- Project config (`/.deepseek/config.toml`) +- Project config (`/.codewhale/config.toml`, or legacy + `/.deepseek/config.toml`) **replaces** the user array wholesale rather than merging. If you want both, list `~/global.md` inside the project array. Set `instructions = []` in the project to clear the @@ -508,24 +520,28 @@ If you are upgrading from older releases: `[subagents.models]` accepts lower-case role or type keys such as `worker`, `explorer`, `general`, `explore`, `plan`, and `review`. Values must normalize to a supported DeepSeek model id before an agent is spawned. -- `skills_dir` (string, optional): defaults to `~/.deepseek/skills` (each skill is +- `skills_dir` (string, optional): defaults to `~/.codewhale/skills` (each skill is a directory containing `SKILL.md`). Workspace-local `.agents/skills` or `./skills` are preferred when present; the runtime also discovers global agentskills.io-compatible `~/.agents/skills` and the broader Claude-ecosystem `~/.claude/skills`. First launch installs versioned bundled skills for common workflows including skill creation, delegation, MCP/plugin scaffolding, documents, presentations, spreadsheets, PDFs, and Feishu/Lark. -- `mcp_config_path` (string, optional): defaults to `~/.deepseek/mcp.json`. +- `mcp_config_path` (string, optional): defaults to `~/.codewhale/mcp.json`, with + legacy `~/.deepseek/mcp.json` fallback when the CodeWhale path is absent. It is visible in `/config` and can be changed from the TUI. The new path is used immediately by `/mcp`, but rebuilding the model-visible MCP tool pool requires restarting the TUI. -- `notes_path` (string, optional): defaults to `~/.deepseek/notes.txt` and is used by the model-visible `note` tool. +- `notes_path` (string, optional): defaults to `~/.codewhale/notes.txt`, with + legacy `~/.deepseek/notes.txt` fallback when the CodeWhale path is absent, and + is used by the model-visible `note` tool. - `[memory].enabled` (bool, optional): defaults to `false`. When `true`, the TUI loads the user memory file into a `` prompt block, enables `# foo` quick-capture in the composer, surfaces the `/memory` slash command, and registers the `remember` tool. The same toggle is available via `DEEPSEEK_MEMORY=on`. -- `memory_path` (string, optional): defaults to `~/.deepseek/memory.md`. +- `memory_path` (string, optional): defaults to `~/.codewhale/memory.md`, with + legacy `~/.deepseek/memory.md` fallback when the CodeWhale path is absent. Used by the user-memory feature when enabled — see [`MEMORY.md`](MEMORY.md) for the full feature surface (`# foo` composer prefix, `/memory` slash command, `remember` tool, opt-in @@ -533,7 +549,10 @@ If you are upgrading from older releases: - `snapshots.*` (optional): side-git workspace snapshots for file rollback: - `[snapshots].enabled` (bool, default `true`) - `[snapshots].max_age_days` (int, default `7`) - - snapshots live under `~/.deepseek/snapshots///.git` and never use the workspace's own `.git` directory + - snapshots live under + `~/.codewhale/snapshots///.git`, with legacy + `~/.deepseek/snapshots/...` fallback when only the legacy state exists, and + never use the workspace's own `.git` directory - `context.*` (optional): append-only Fin seam manager, currently opt-in. Fin is the fast `deepseek-v4-flash` path with thinking off used for coordination work such as routing, summaries, and context maintenance. @@ -617,7 +636,7 @@ User memory is split across one top-level path setting and one opt-in toggle table: ```toml -memory_path = "~/.deepseek/memory.md" +memory_path = "~/.codewhale/memory.md" [memory] enabled = true @@ -755,7 +774,8 @@ See `docs/capacity_controller.md` for formulas, intervention behavior, and telem ## Notes On `codewhale-tui doctor` `codewhale-tui doctor` follows the same config resolution rules as the rest of the -TUI. That means `--config` / `DEEPSEEK_CONFIG_PATH` are respected, and MCP/skills +TUI. That means `--config`, `CODEWHALE_CONFIG_PATH`, and the legacy +`DEEPSEEK_CONFIG_PATH` are respected, and MCP/skills checks use the resolved `mcp_config_path` / `skills_dir` (including env overrides). To bootstrap missing MCP/skills paths, run `codewhale-tui setup --all`. You can @@ -790,19 +810,22 @@ configure reasoning effort. MCP/skills/tools/plugins counts, sandbox, `.env` presence). Read-only and network-free; safe to run in CI. If `.env` is missing and `.env.example` is present in the workspace, the status output points at `cp .env.example .env`. -- `--tools` — scaffold `~/.deepseek/tools/` with a `README.md` describing the +- `--tools` — scaffold `~/.codewhale/tools/` with a `README.md` describing the self-describing frontmatter convention (`# name:` / `# description:` / `# usage:`) and an `example.sh` that follows it. The directory is intentionally not auto-loaded; wire individual scripts into the agent via MCP, hooks, or skills. -- `--plugins` — scaffold `~/.deepseek/plugins/` with a `README.md` and an +- `--plugins` — scaffold `~/.codewhale/plugins/` with a `README.md` and an `example/PLUGIN.md` placeholder using the same frontmatter shape as `SKILL.md`. Plugins are not loaded automatically either; reference them from a skill, hook, or MCP wrapper when you want them active. - `--all` now scaffolds MCP + skills + tools + plugins together. -- `--clean` — list `~/.deepseek/sessions/checkpoints/latest.json` and - `offline_queue.json` if they exist. Pass `--force` to actually remove them. - This never touches real session history or the task queue. +- `--clean` — list `~/.codewhale/sessions/checkpoints/latest.json` and + `offline_queue.json` if they exist. Legacy + `~/.deepseek/sessions/checkpoints/` files are not scanned automatically; set + `CODEWHALE_HOME=~/.deepseek` for a one-off legacy cleanup. Pass `--force` to + actually remove matched files. This never touches real session history or the + task queue. `--status` and `--clean` are mutually exclusive with the scaffold flags. From f38864d801eaa91ec63d73fd5f2a1da614da7923 Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Sun, 31 May 2026 02:43:38 -0700 Subject: [PATCH 05/15] fix(tui): compact statusline token chip (#2411) Harvested from #2405 with thanks to @axobase001.\n\nCompacts the statusline token chip to use the short label while preserving the existing token detail and adding focused coverage for the rendered label.\n\nPartially addresses #2309. --- crates/tui/src/tui/footer_ui.rs | 16 +++++----------- crates/tui/src/tui/ui/tests.rs | 20 ++++++++++++++++++-- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/crates/tui/src/tui/footer_ui.rs b/crates/tui/src/tui/footer_ui.rs index 70b84e15f..141b6d6fe 100644 --- a/crates/tui/src/tui/footer_ui.rs +++ b/crates/tui/src/tui/footer_ui.rs @@ -631,22 +631,16 @@ pub(crate) fn should_show_footer_cost(displayed_cost: f64) -> bool { /// Session token-usage chip for the footer right cluster. /// -/// Renders the accumulated input / cache-hit / output token breakdown -/// since the current runtime session started (not persisted across -/// restarts). Returns empty when no tokens have been recorded yet. +/// Renders a compact accumulated token count for the current runtime session. +/// Detailed cache stats live in the separate `cache` chip. pub(crate) fn footer_session_tokens_spans(app: &App) -> Vec> { let session = &app.session; if session.total_input_tokens == 0 && session.total_output_tokens == 0 { return Vec::new(); } - let in_str = format_token_count_compact(u64::from(session.total_input_tokens)); - let out_str = format_token_count_compact(u64::from(session.total_output_tokens)); - let text = if session.total_cache_hit_tokens == 0 && session.total_cache_miss_tokens == 0 { - format!("{in_str} in · {out_str} out") - } else { - let cache_str = format_token_count_compact(u64::from(session.total_cache_hit_tokens)); - format!("{in_str} in · {cache_str} cch · {out_str} out") - }; + let total = u64::from(session.total_input_tokens) + .saturating_add(u64::from(session.total_output_tokens)); + let text = format!("tok {}", format_token_count_compact(total)); vec![Span::styled(text, Style::default().fg(palette::TEXT_MUTED))] } diff --git a/crates/tui/src/tui/ui/tests.rs b/crates/tui/src/tui/ui/tests.rs index 308afc196..50790f31f 100644 --- a/crates/tui/src/tui/ui/tests.rs +++ b/crates/tui/src/tui/ui/tests.rs @@ -10,8 +10,9 @@ use crate::tui::file_mention::{ }; use crate::tui::footer_ui::{ active_tool_status_label, footer_auxiliary_spans, footer_balance_spans, footer_cache_spans, - footer_coherence_spans, footer_state_label, footer_status_line_spans, format_context_budget, - format_token_count_compact, friendly_subagent_progress, render_footer_from, + footer_coherence_spans, footer_session_tokens_spans, footer_state_label, + footer_status_line_spans, format_context_budget, format_token_count_compact, + friendly_subagent_progress, render_footer_from, }; use crate::tui::history::{ ExecCell, ExecSource, GenericToolCell, HistoryCell, ToolCell, ToolStatus, @@ -2421,6 +2422,21 @@ fn format_token_count_compact_formats_units() { assert_eq!(format_token_count_compact(1_000_000), "1.0M"); } +#[test] +fn footer_session_tokens_chip_uses_single_compact_total() { + let mut app = create_test_app(); + app.session.total_input_tokens = 900_000; + app.session.total_cache_hit_tokens = 700_000; + app.session.total_cache_miss_tokens = 200_000; + app.session.total_output_tokens = 600_000; + + let text = spans_text(&footer_session_tokens_spans(&app)); + + assert_eq!(text, "tok 1.5M"); + assert!(!text.contains(" cch ")); + assert!(!text.contains(" out")); +} + #[test] fn format_context_budget_caps_overflow_display() { assert_eq!(format_context_budget(5_000, 128_000), "5.0k/128.0k"); From 6e8477334de4910a65f24a44e32b9bc9b9391302 Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Sun, 31 May 2026 02:47:48 -0700 Subject: [PATCH 06/15] test(shell): cover tty controlling terminal (#2414) Harvested from #2408 with thanks to @axobase001. Adds regression coverage proving tty:true shell commands receive a controlling terminal, with a longer wait margin so the test is stable on slower CI hosts. Partially addresses #2372. --- crates/tui/src/tools/shell/tests.rs | 34 +++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/crates/tui/src/tools/shell/tests.rs b/crates/tui/src/tools/shell/tests.rs index 7bcd643ca..c460f2306 100644 --- a/crates/tui/src/tools/shell/tests.rs +++ b/crates/tui/src/tools/shell/tests.rs @@ -241,6 +241,40 @@ fn test_write_stdin_streams_output() { assert!(delta2.result.stdout.is_empty()); } +#[test] +#[cfg(unix)] +fn background_tty_command_has_controlling_terminal() { + let tmp = tempdir().expect("tempdir"); + let mut manager = ShellManager::new(tmp.path().to_path_buf()); + + let result = manager + .execute_with_options( + "sh -c 'exec 3<>/dev/tty && printf tty-ok && exec 3>&-'", + None, + 5000, + true, + None, + true, + Some(ExecutionSandboxPolicy::DangerFullAccess), + ) + .expect("execute tty command"); + + let task_id = result + .task_id + .expect("background tty execution should return task_id"); + + let done = manager + .get_output(&task_id, true, 10_000) + .expect("get tty command output"); + + assert_eq!(done.status, ShellStatus::Completed); + assert_eq!(done.exit_code, Some(0)); + assert!( + done.stdout.contains("tty-ok"), + "tty output should confirm /dev/tty opened; got {done:?}" + ); +} + #[test] fn test_job_list_poll_cancel_and_stale_snapshot() { let tmp = tempdir().expect("tempdir"); From 0dd7f0b8022d83f58d74d68549a715fb36cfaf23 Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Sun, 31 May 2026 02:48:41 -0700 Subject: [PATCH 07/15] fix(runtime): harden mobile QR smoke output (#2417) Harvested from #2415 with thanks to @axobase001. Keeps the denser mobile QR renderer and replaces the fixed binding-warning sleep with health polling plus an explicit timeout failure path, so slow starts fail with the useful cause instead of drifting into misleading assertions. Follow-up to #2403. --- crates/tui/src/runtime_api.rs | 2 +- scripts/mobile-smoke.sh | 15 ++++++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/crates/tui/src/runtime_api.rs b/crates/tui/src/runtime_api.rs index 6473ed2d8..820e0823e 100644 --- a/crates/tui/src/runtime_api.rs +++ b/crates/tui/src/runtime_api.rs @@ -712,7 +712,7 @@ fn print_mobile_urls(addr: SocketAddr, token: Option<&str>, auth_enabled: bool, if show_qr { match qrcode::QrCode::new(qr_url.as_bytes()) { Ok(qr) => { - let qr_str = qr.render::().module_dimensions(2, 1).build(); + let qr_str = qr.render::().build(); println!("\n{qr_str}"); } Err(e) => { diff --git a/scripts/mobile-smoke.sh b/scripts/mobile-smoke.sh index 2b94a8985..d333ff5ee 100755 --- a/scripts/mobile-smoke.sh +++ b/scripts/mobile-smoke.sh @@ -155,7 +155,20 @@ log "=== Test Group 3: Binding warnings (0.0.0.0 default) ===" STDOUT_FILE=$(mktemp) "$BINARY" serve --port "$PORT" --mobile --insecure > "$STDOUT_FILE" 2>&1 & SERVER_PID=$! -sleep 2 +SERVER_READY=0 +for _ in $(seq 1 30); do + if curl -sf "http://127.0.0.1:${PORT}/health" > /dev/null 2>&1; then + SERVER_READY=1 + break + fi + sleep 0.3 +done +if [[ "$SERVER_READY" -ne 1 ]]; then + rm -f "$STDOUT_FILE" + fail "Server did not become ready on port $PORT" + cleanup + exit 1 +fi STDOUT=$(cat "$STDOUT_FILE") rm -f "$STDOUT_FILE" From ce72b0cbc481ecccdd1b05f31b8cba93bf6142f2 Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Sun, 31 May 2026 02:52:24 -0700 Subject: [PATCH 08/15] fix(tui): clarify shell tool availability errors (#2412) Harvested from #2402 with thanks to @axobase001. Keeps `allow_shell` guidance visible for gated shell tools even when missing-tool suggestions exist, removes the nonexistent task_shell_cancel matcher, and broadens regression coverage. Partially addresses #2328. --- crates/tui/src/core/engine/tests.rs | 38 +++++++++++++++++++++ crates/tui/src/core/engine/tool_catalog.rs | 39 ++++++++++++++++++++-- 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/crates/tui/src/core/engine/tests.rs b/crates/tui/src/core/engine/tests.rs index 06ae40306..f73973619 100644 --- a/crates/tui/src/core/engine/tests.rs +++ b/crates/tui/src/core/engine/tests.rs @@ -2359,6 +2359,44 @@ fn missing_tool_error_message_includes_discovery_guidance_when_no_match() { assert!(message.contains(TOOL_SEARCH_BM25_NAME)); } +#[test] +fn missing_shell_tool_error_message_names_allow_shell_gate() { + let catalog = vec![api_tool("read_file")]; + + for tool_name in [ + "exec_shell", + "exec_shell_wait", + "exec_shell_interact", + "task_shell_start", + "task_shell_wait", + ] { + let message = missing_tool_error_message(tool_name, &catalog); + assert!(message.contains("not available in the current tool catalog")); + assert!(message.contains("allow_shell"), "{tool_name}: {message}"); + assert!( + message.contains("trusted workspaces"), + "{tool_name}: {message}" + ); + assert!( + message.contains(TOOL_SEARCH_BM25_NAME), + "{tool_name}: {message}" + ); + } +} + +#[test] +fn missing_shell_tool_error_message_keeps_allow_shell_hint_with_suggestions() { + let catalog = vec![api_tool("exec")]; + + let message = missing_tool_error_message("exec_shell", &catalog); + + assert!(message.contains("Did you mean:")); + assert!(message.contains("exec")); + assert!(message.contains("allow_shell")); + assert!(message.contains("trusted workspaces")); + assert!(message.contains(TOOL_SEARCH_BM25_NAME)); +} + #[test] fn filter_tool_call_delta_strips_bracket_marker() { let mut in_block = false; diff --git a/crates/tui/src/core/engine/tool_catalog.rs b/crates/tui/src/core/engine/tool_catalog.rs index 867c68951..517896326 100644 --- a/crates/tui/src/core/engine/tool_catalog.rs +++ b/crates/tui/src/core/engine/tool_catalog.rs @@ -438,17 +438,52 @@ fn suggest_tool_names(catalog: &[Tool], requested: &str, limit: usize) -> Vec String { let suggestions = suggest_tool_names(catalog, tool_name, 3); + let shell_hint = if is_shell_tool_name(tool_name) { + Some(shell_tool_allow_shell_hint()) + } else { + None + }; if suggestions.is_empty() { + if let Some(shell_hint) = shell_hint { + return format!( + "Tool '{tool_name}' is not available in the current tool catalog. \ + {shell_hint}, or use {TOOL_SEARCH_BM25_NAME} with a short query." + ); + } return format!( "Tool '{tool_name}' is not available in the current tool catalog. \ Verify mode/feature flags, or use {TOOL_SEARCH_BM25_NAME} with a short query." ); } + let suggestion_text = format!("Did you mean: {}?", suggestions.join(", ")); + if let Some(shell_hint) = shell_hint { + return format!( + "Tool '{tool_name}' is not available in the current tool catalog. \ + {suggestion_text} {shell_hint}. \ + You can also use {TOOL_SEARCH_BM25_NAME} to discover tools." + ); + } + format!( "Tool '{tool_name}' is not available in the current tool catalog. \ - Did you mean: {}? You can also use {TOOL_SEARCH_BM25_NAME} to discover tools.", - suggestions.join(", ") + {suggestion_text} You can also use {TOOL_SEARCH_BM25_NAME} to discover tools." + ) +} + +fn shell_tool_allow_shell_hint() -> &'static str { + "Shell tools are gated by `allow_shell`; enable `allow_shell = true` for trusted workspaces, \ + or switch to an auto-approve mode that permits shell access" +} + +fn is_shell_tool_name(tool_name: &str) -> bool { + matches!( + tool_name, + "exec_shell" + | "exec_shell_wait" + | "exec_shell_interact" + | "task_shell_start" + | "task_shell_wait" ) } From 3899ca3f589bec6c22eb5ebf3c4bd7b65a69efd8 Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Sun, 31 May 2026 02:53:08 -0700 Subject: [PATCH 09/15] feat(project-context): stabilize pack ordering (#2418) Harvested from #2392 with thanks to @wplll. Makes project context pack path ordering deterministic across Unix and Windows-style separators while keeping README/config/source entries prioritized before general directory noise. --- crates/tui/src/project_context.rs | 101 ++++++++++++++++++++++++++++-- 1 file changed, 95 insertions(+), 6 deletions(-) diff --git a/crates/tui/src/project_context.rs b/crates/tui/src/project_context.rs index d6c3a4c51..70af398ca 100644 --- a/crates/tui/src/project_context.rs +++ b/crates/tui/src/project_context.rs @@ -170,7 +170,7 @@ struct ReadmePack { pub fn generate_project_context_pack(workspace: &Path) -> Option { let mut entries = Vec::new(); collect_pack_entries(workspace, workspace, 0, &mut entries); - entries.sort(); + sort_pack_paths(&mut entries); entries.truncate(PACK_MAX_ENTRIES); let mut config_files = entries @@ -179,7 +179,7 @@ pub fn generate_project_context_pack(workspace: &Path) -> Option { .take(PACK_MAX_CONFIG_FILES) .cloned() .collect::>(); - config_files.sort(); + sort_pack_paths(&mut config_files); let mut key_source_files = entries .iter() @@ -187,7 +187,7 @@ pub fn generate_project_context_pack(workspace: &Path) -> Option { .take(PACK_MAX_SOURCE_FILES) .cloned() .collect::>(); - key_source_files.sort(); + sort_pack_paths(&mut key_source_files); let readme = read_readme_excerpt(workspace, &entries); let mut counts = BTreeMap::new(); @@ -289,10 +289,50 @@ fn relative_slash_path(root: &Path, path: &Path) -> Option { for component in relative.components() { parts.push(component.as_os_str().to_string_lossy().to_string()); } - if parts.is_empty() { - None + normalize_pack_relative_path(&parts.join("/")) +} + +fn normalize_pack_relative_path(path: &str) -> Option { + let normalized = path.replace('\\', "/"); + let mut parts = Vec::new(); + for part in normalized.split('/') { + if part.is_empty() || part == "." { + continue; + } + if part == ".." { + return None; + } + parts.push(part); + } + (!parts.is_empty()).then(|| parts.join("/")) +} + +fn sort_pack_paths(paths: &mut [String]) { + paths.sort_by(|a, b| { + pack_path_priority(a) + .cmp(&pack_path_priority(b)) + .then_with(|| pack_path_sort_key(a).cmp(&pack_path_sort_key(b))) + .then_with(|| a.cmp(b)) + }); +} + +fn pack_path_sort_key(path: &str) -> String { + path.replace('\\', "/").to_ascii_lowercase() +} + +fn pack_path_priority(path: &str) -> u8 { + let lower = pack_path_sort_key(path); + let name = lower.trim_end_matches('/').rsplit('/').next().unwrap_or(""); + if matches!(name, "readme.md" | "readme.txt" | "readme") { + 0 + } else if is_config_file(&lower) { + 1 + } else if is_source_file(&lower) { + 2 + } else if lower.ends_with('/') { + 3 } else { - Some(parts.join("/")) + 4 } } @@ -1029,6 +1069,55 @@ mod tests { ); } + #[test] + fn project_context_pack_sort_is_cross_platform_and_priority_aware() { + let mut unix_paths = vec![ + "src/z.rs".to_string(), + "docs/".to_string(), + "README.md".to_string(), + "Cargo.toml".to_string(), + "src/a.rs".to_string(), + "notes.txt".to_string(), + ]; + let mut windows_paths = vec![ + "src\\z.rs".to_string(), + "docs\\".to_string(), + "README.md".to_string(), + "Cargo.toml".to_string(), + "src\\a.rs".to_string(), + "notes.txt".to_string(), + ]; + + sort_pack_paths(&mut unix_paths); + sort_pack_paths(&mut windows_paths); + + let normalized_windows = windows_paths + .iter() + .map(|path| path.replace('\\', "/")) + .collect::>(); + assert_eq!(unix_paths, normalized_windows); + assert_eq!( + unix_paths, + vec![ + "README.md", + "Cargo.toml", + "src/a.rs", + "src/z.rs", + "docs/", + "notes.txt", + ] + ); + } + + #[test] + fn normalize_pack_relative_path_rejects_parent_segments() { + assert_eq!( + normalize_pack_relative_path(".\\src\\main.rs"), + Some("src/main.rs".to_string()) + ); + assert_eq!(normalize_pack_relative_path("../secret.txt"), None); + } + #[test] fn test_load_global_agents_when_project_has_no_context() { let workspace = tempdir().expect("workspace tempdir"); From 3c0d56d42477b6a4b51a77981e3a348e3024dd9a Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Sun, 31 May 2026 02:53:29 -0700 Subject: [PATCH 10/15] test(cache): cover medium tool result dedup (#2419) Harvested from #2393 with thanks to @wplll. Strengthens the tool-result dedup regression coverage by exercising repeated medium-sized outputs that are above the dedup threshold but below the truncation budget. --- crates/tui/src/client/chat.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/tui/src/client/chat.rs b/crates/tui/src/client/chat.rs index 9e87bb2ec..39682cbe7 100644 --- a/crates/tui/src/client/chat.rs +++ b/crates/tui/src/client/chat.rs @@ -2853,8 +2853,12 @@ mod stream_decoder_tests { } #[test] - fn request_builder_deduplicates_large_identical_tool_results_with_retrieval_hint() { + fn request_builder_deduplicates_medium_identical_tool_results_with_retrieval_hint() { with_tool_result_sha_spillover_root(|| { + // 2,000 chars is intentionally above TOOL_RESULT_DEDUP_MIN_CHARS + // (1,024) but below TOOL_RESULT_SENT_CHAR_BUDGET (12,000). This + // verifies the cache-saving path for repeated medium outputs that + // do not otherwise need truncation. let output = "A".repeat(2_000); let messages = vec![ tool_use_message("tool-1", "read_file", json!({"path": "README.md"})), @@ -2868,6 +2872,7 @@ mod stream_decoder_tests { let second = tool_message_content(&built, 1); assert_eq!(first, output); + assert!(!first.contains("[TOOL_RESULT_TRUNCATED]"), "got: {first}"); assert!( second.starts_with(" Date: Sun, 31 May 2026 03:05:28 -0700 Subject: [PATCH 11/15] feat(agent): register AtlasCloud static models (#2421) Harvested from #2343 with thanks to @lucaszhu-hue. Registers AtlasCloud static model rows for Pro and Flash resolution, adds provider-hinted alias coverage, and updates neutral provider docs and env examples while leaving promotional assets/copy out. --- .env.example | 6 ++++++ crates/agent/src/lib.rs | 48 +++++++++++++++++++++++++++++++++++++++++ docs/PROVIDERS.md | 10 +++++---- 3 files changed, 60 insertions(+), 4 deletions(-) diff --git a/.env.example b/.env.example index eb28de82d..d059e02b6 100644 --- a/.env.example +++ b/.env.example @@ -25,6 +25,12 @@ # NVIDIA_BASE_URL=https://integrate.api.nvidia.com/v1 # NVIDIA_NIM_MODEL=deepseek-ai/deepseek-v4-pro +# AtlasCloud OpenAI-compatible endpoint +# DEEPSEEK_PROVIDER=atlascloud +# ATLASCLOUD_API_KEY= +# ATLASCLOUD_BASE_URL=https://api.atlascloud.ai/v1 +# ATLASCLOUD_MODEL=deepseek-ai/deepseek-v4-flash + # Logging # `DEEPSEEK_LOG_LEVEL` is forwarded by the facade; `RUST_LOG` enables the # TUI's lightweight verbose logs for info/debug/trace directives. diff --git a/crates/agent/src/lib.rs b/crates/agent/src/lib.rs index ece173053..8d93d65bf 100644 --- a/crates/agent/src/lib.rs +++ b/crates/agent/src/lib.rs @@ -87,6 +87,26 @@ impl Default for ModelRegistry { supports_tools: true, supports_reasoning: true, }, + ModelInfo { + id: "deepseek-ai/deepseek-v4-flash".to_string(), + provider: ProviderKind::Atlascloud, + aliases: vec![ + "deepseek-v4-flash".to_string(), + "atlascloud-deepseek-v4-flash".to_string(), + ], + supports_tools: true, + supports_reasoning: true, + }, + ModelInfo { + id: "deepseek-ai/deepseek-v4-pro".to_string(), + provider: ProviderKind::Atlascloud, + aliases: vec![ + "deepseek-v4-pro".to_string(), + "atlascloud-deepseek-v4-pro".to_string(), + ], + supports_tools: true, + supports_reasoning: true, + }, ModelInfo { id: "deepseek-reasoner".to_string(), provider: ProviderKind::WanjieArk, @@ -434,6 +454,34 @@ mod tests { assert_eq!(resolved.resolved.id, "deepseek-ai/deepseek-v4-flash"); } + #[test] + fn atlascloud_default_uses_namespaced_model_id() { + let registry = ModelRegistry::default(); + let resolved = registry.resolve(None, Some(ProviderKind::Atlascloud)); + + assert_eq!(resolved.resolved.provider, ProviderKind::Atlascloud); + assert_eq!(resolved.resolved.id, "deepseek-ai/deepseek-v4-flash"); + assert!(resolved.resolved.supports_reasoning); + } + + #[test] + fn deepseek_v4_flash_alias_resolves_to_atlascloud_when_provider_hinted() { + let registry = ModelRegistry::default(); + let resolved = registry.resolve(Some("deepseek-v4-flash"), Some(ProviderKind::Atlascloud)); + + assert_eq!(resolved.resolved.provider, ProviderKind::Atlascloud); + assert_eq!(resolved.resolved.id, "deepseek-ai/deepseek-v4-flash"); + } + + #[test] + fn deepseek_v4_pro_alias_resolves_to_atlascloud_when_provider_hinted() { + let registry = ModelRegistry::default(); + let resolved = registry.resolve(Some("deepseek-v4-pro"), Some(ProviderKind::Atlascloud)); + + assert_eq!(resolved.resolved.provider, ProviderKind::Atlascloud); + assert_eq!(resolved.resolved.id, "deepseek-ai/deepseek-v4-pro"); + } + #[test] fn openrouter_default_uses_namespaced_model_id() { let registry = ModelRegistry::default(); diff --git a/docs/PROVIDERS.md b/docs/PROVIDERS.md index 425155d44..abfa1bbec 100644 --- a/docs/PROVIDERS.md +++ b/docs/PROVIDERS.md @@ -114,7 +114,7 @@ endpoint. | `deepseek` | `[providers.deepseek]` | `DEEPSEEK_API_KEY` | `CODEWHALE_BASE_URL` / `DEEPSEEK_BASE_URL`; default `https://api.deepseek.com/beta` | `deepseek-v4-pro`, `deepseek-v4-flash`; compatibility aliases `deepseek-chat`, `deepseek-reasoner` | First-class default. Beta URL enables strict tool mode, chat prefix completion, and FIM completion. Set `https://api.deepseek.com` or `/v1` explicitly to opt out of beta-only features. | | `nvidia-nim` | `[providers.nvidia_nim]` | `NVIDIA_API_KEY`, `NVIDIA_NIM_API_KEY`, fallback `DEEPSEEK_API_KEY` | `NVIDIA_NIM_BASE_URL`, `NIM_BASE_URL`, `NVIDIA_BASE_URL`; default `https://integrate.api.nvidia.com/v1` | `deepseek-ai/deepseek-v4-pro`, `deepseek-ai/deepseek-v4-flash` | Hosted DeepSeek V4 through NVIDIA NIM. `NVIDIA_NIM_MODEL` is accepted by the TUI config path. | | `openai` | `[providers.openai]` | `OPENAI_API_KEY` | `OPENAI_BASE_URL`; default `https://api.openai.com/v1` | Registry entries: `deepseek-v4-pro`, `deepseek-v4-flash`; default config model `deepseek-v4-pro` | Generic OpenAI-compatible route for gateways and custom endpoints. Use this for explicit third-party OpenAI-compatible routes instead of inventing a new provider ID. `OPENAI_MODEL` is accepted. | -| `atlascloud` | `[providers.atlascloud]` | `ATLASCLOUD_API_KEY` | `ATLASCLOUD_BASE_URL`; default `https://api.atlascloud.ai/v1` | Default config model `deepseek-ai/deepseek-v4-flash` | OpenAI-compatible hosted route. `ATLASCLOUD_MODEL` is accepted by the TUI config path. The static `ModelRegistry` does not currently list AtlasCloud rows. | +| `atlascloud` | `[providers.atlascloud]` | `ATLASCLOUD_API_KEY` | `ATLASCLOUD_BASE_URL`; default `https://api.atlascloud.ai/v1` | `deepseek-ai/deepseek-v4-flash`, `deepseek-ai/deepseek-v4-pro` | OpenAI-compatible hosted route. `ATLASCLOUD_MODEL` is accepted by the TUI config path, and the static `ModelRegistry` includes AtlasCloud fallback rows for CLI model resolution. | | `wanjie-ark` | `[providers.wanjie_ark]` | `WANJIE_ARK_API_KEY`, `WANJIE_API_KEY`, `WANJIE_MAAS_API_KEY` | `WANJIE_ARK_BASE_URL`, `WANJIE_BASE_URL`, `WANJIE_MAAS_BASE_URL`; default `https://maas-openapi.wanjiedata.com/api/v1` | `deepseek-reasoner` | OpenAI-compatible hosted route. `WANJIE_ARK_MODEL`, `WANJIE_MODEL`, and `WANJIE_MAAS_MODEL` are accepted. | | `volcengine` | `[providers.volcengine]` | `VOLCENGINE_API_KEY`, `VOLCENGINE_ARK_API_KEY`, `ARK_API_KEY` | `VOLCENGINE_BASE_URL`, `VOLCENGINE_ARK_BASE_URL`, `ARK_BASE_URL`; default `https://ark.cn-beijing.volces.com/api/coding/v3` | `DeepSeek-V4-Pro`, `DeepSeek-V4-Flash` | Volcengine/Volcano Engine Ark OpenAI-compatible coding endpoint. `VOLCENGINE_MODEL` and `VOLCENGINE_ARK_MODEL` are accepted. | | `openrouter` | `[providers.openrouter]` | `OPENROUTER_API_KEY` | `OPENROUTER_BASE_URL`; default `https://openrouter.ai/api/v1` | `deepseek/deepseek-v4-pro`, `deepseek/deepseek-v4-flash` | Additive open-model routing layer. It does not replace DeepSeek; it lets users route supported model IDs through OpenRouter when they choose it. | @@ -148,6 +148,7 @@ endpoint when the endpoint supports model listing. | `deepseek` | `deepseek-v4-pro`, `deepseek-v4-flash` | yes | yes | | `nvidia-nim` | `deepseek-ai/deepseek-v4-pro`, `deepseek-ai/deepseek-v4-flash` | yes | yes | | `openai` | `deepseek-v4-pro`, `deepseek-v4-flash` | yes | yes | +| `atlascloud` | `deepseek-ai/deepseek-v4-flash`, `deepseek-ai/deepseek-v4-pro` | yes | yes | | `wanjie-ark` | `deepseek-reasoner` | yes | yes | | `volcengine` | `DeepSeek-V4-Pro`, `DeepSeek-V4-Flash` | yes | yes | | `openrouter` | `deepseek/deepseek-v4-pro`, `deepseek/deepseek-v4-flash` | yes | yes | @@ -160,9 +161,10 @@ endpoint when the endpoint supports model listing. | `vllm` | `deepseek-ai/DeepSeek-V4-Pro`, `deepseek-ai/DeepSeek-V4-Flash` | yes | yes | | `ollama` | `deepseek-coder:1.3b`; custom tags pass through when provider hint is `ollama` | yes | no | -The registry currently has no AtlasCloud entry even though AtlasCloud is a -supported provider in config and TUI selection. AtlasCloud should use the -configured model or live model listing. +AtlasCloud keeps the same default model as the config layer and adds +provider-scoped aliases for the Pro and Flash rows. Other AtlasCloud model IDs +should still be selected through `ATLASCLOUD_MODEL`, config, or live model +listing when available. ## Capability Metadata From 58e45d384e3b5052c3cbebc978a399dad64ea40e Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Sun, 31 May 2026 03:16:53 -0700 Subject: [PATCH 12/15] feat(subagents): inherit MCP tools in child runtimes (#2422) Harvested from #2377 with thanks to @buko. Threads the parent MCP tool pool into child SubAgentRuntime construction and registers MCP-backed tools for child agents when MCP is enabled, while leaving the broader mention-browser/provider/config work for focused follow-ups. Validation: - cargo fmt --all -- --check - CARGO_TARGET_DIR=/Volumes/VIXinSSD/codewhale-pr2377-target cargo test -p codewhale-tui tools::subagent - CARGO_TARGET_DIR=/Volumes/VIXinSSD/codewhale-target/harvest-2377-recheck cargo test -p codewhale-tui tools::subagent --all-features --- crates/tui/src/core/engine.rs | 14 +++++++++ crates/tui/src/tools/registry.rs | 1 - crates/tui/src/tools/subagent/mod.rs | 40 +++++++++++++++++++------- crates/tui/src/tools/subagent/tests.rs | 1 + 4 files changed, 44 insertions(+), 12 deletions(-) diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index 24d28230f..b45f0bea2 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -697,6 +697,12 @@ impl Engine { continue; }; + let mcp_pool = if self.config.features.enabled(Feature::Mcp) { + self.ensure_mcp_pool().await.ok() + } else { + None + }; + let mut runtime = SubAgentRuntime::new( client, self.session.model.clone(), @@ -714,6 +720,7 @@ impl Engine { ) .with_max_spawn_depth(self.config.max_spawn_depth) .with_step_api_timeout(self.config.subagent_api_timeout) + .with_mcp_pool(mcp_pool) .background_runtime(); let route = resolve_subagent_assignment_route( &runtime, @@ -1179,6 +1186,12 @@ impl Engine { None }; + let mcp_pool = if self.config.features.enabled(Feature::Mcp) { + self.ensure_mcp_pool().await.ok() + } else { + None + }; + let tool_registry = match mode { AppMode::Agent | AppMode::Yolo => { if self.config.features.enabled(Feature::Subagents) { @@ -1199,6 +1212,7 @@ impl Engine { ) .with_max_spawn_depth(self.config.max_spawn_depth) .with_step_api_timeout(self.config.subagent_api_timeout) + .with_mcp_pool(mcp_pool.clone()) .with_parent_completion_tx(self.tx_subagent_completion.clone()); if let Some(context) = fork_context_for_runtime.clone() { rt = rt.with_fork_context(context); diff --git a/crates/tui/src/tools/registry.rs b/crates/tui/src/tools/registry.rs index 2e3d09c9b..a27daca3d 100644 --- a/crates/tui/src/tools/registry.rs +++ b/crates/tui/src/tools/registry.rs @@ -777,7 +777,6 @@ impl ToolRegistryBuilder { /// MCP tools are marked `defer_loading` by default (except discovery /// helpers) to keep the model-visible catalog compact. #[must_use] - #[allow(dead_code)] pub fn with_mcp_tools( mut self, mcp_pool: std::sync::Arc>, diff --git a/crates/tui/src/tools/subagent/mod.rs b/crates/tui/src/tools/subagent/mod.rs index cf10a930a..67d3cd17f 100644 --- a/crates/tui/src/tools/subagent/mod.rs +++ b/crates/tui/src/tools/subagent/mod.rs @@ -786,6 +786,8 @@ pub struct SubAgentRuntime { pub parent_completion_tx: Option>, /// Snapshot of the request prefix visible to an opt-in forked child. pub fork_context: Option, + /// The parent's MCP pool if available. + pub mcp_pool: Option>>, /// Per-step DeepSeek API timeout for the child's `create_message` call. /// Resolved from `[subagents] api_timeout_secs` (clamped to 1..=1800) at /// engine construction so a slow but legitimate model turn does not @@ -825,10 +827,21 @@ impl SubAgentRuntime { mailbox: None, parent_completion_tx: None, fork_context: None, + mcp_pool: None, step_api_timeout: DEFAULT_STEP_API_TIMEOUT, } } + /// Attach an MCP pool so the subagent can execute MCP tools. + #[must_use] + pub fn with_mcp_pool( + mut self, + pool: Option>>, + ) -> Self { + self.mcp_pool = pool; + self + } + /// Override the per-step DeepSeek API timeout (default /// `DEFAULT_STEP_API_TIMEOUT`). Called by the engine after reading /// `[subagents] api_timeout_secs`. Tests may use this to fail fast @@ -959,6 +972,7 @@ impl SubAgentRuntime { mailbox: self.mailbox.clone(), parent_completion_tx: self.parent_completion_tx.clone(), fork_context: self.fork_context.clone(), + mcp_pool: self.mcp_pool.clone(), step_api_timeout: self.step_api_timeout, } } @@ -4762,17 +4776,21 @@ impl SubAgentToolRegistry { // review, RLM, sub-agent management (so grandchildren can spawn), // plus per-child fresh todo/plan state. let context = runtime.context.clone(); - let registry = ToolRegistryBuilder::new() - .with_full_agent_surface( - Some(runtime.client.clone()), - runtime.model.clone(), - runtime.manager.clone(), - runtime.clone(), - runtime.allow_shell, - todo_list, - plan_state, - ) - .build(context); + let mut registry = ToolRegistryBuilder::new().with_full_agent_surface( + Some(runtime.client.clone()), + runtime.model.clone(), + runtime.manager.clone(), + runtime.clone(), + runtime.allow_shell, + todo_list, + plan_state, + ); + + if let Some(pool) = runtime.mcp_pool.as_ref() { + registry = registry.with_mcp_tools(std::sync::Arc::clone(pool)); + } + + let registry = registry.build(context); Self { allowed_tools: explicit_allowed_tools, diff --git a/crates/tui/src/tools/subagent/tests.rs b/crates/tui/src/tools/subagent/tests.rs index 39fd5780b..9c53604ed 100644 --- a/crates/tui/src/tools/subagent/tests.rs +++ b/crates/tui/src/tools/subagent/tests.rs @@ -1736,6 +1736,7 @@ fn stub_runtime() -> SubAgentRuntime { mailbox: None, parent_completion_tx: None, fork_context: None, + mcp_pool: None, step_api_timeout: DEFAULT_STEP_API_TIMEOUT, } } From d5f774ec14b4660160b1e8b418a0dd0f37d3f22a Mon Sep 17 00:00:00 2001 From: Paulo Aboim Pinto Date: Sun, 31 May 2026 11:51:30 +0200 Subject: [PATCH 13/15] feat(tui): add pluggable tool registry --- config.example.toml | 53 ++ crates/tui/src/config.rs | 40 ++ crates/tui/src/core/engine.rs | 65 ++- crates/tui/src/core/engine/tests.rs | 11 + crates/tui/src/main.rs | 7 +- crates/tui/src/runtime_threads.rs | 1 + crates/tui/src/tools/mod.rs | 1 + crates/tui/src/tools/plugin.rs | 840 ++++++++++++++++++++++++++++ crates/tui/src/tools/registry.rs | 73 +++ crates/tui/src/tui/clipboard.rs | 36 +- crates/tui/src/tui/ui.rs | 1 + 11 files changed, 1119 insertions(+), 9 deletions(-) create mode 100644 crates/tui/src/tools/plugin.rs diff --git a/config.example.toml b/config.example.toml index 858aa7130..89247b23d 100644 --- a/config.example.toml +++ b/config.example.toml @@ -652,6 +652,59 @@ default_text_model = "deepseek-ai/deepseek-v4-pro" # [runtime_api] # cors_origins = ["http://localhost:5173", "http://127.0.0.1:5173"] +# ───────────────────────────────────────────────────────────────────────────────── +# Tool Overrides & Plugins ([tools]) +# ───────────────────────────────────────────────────────────────────────────────── +# The `[tools]` table lets you replace any built-in tool with a custom +# implementation (script or command) or disable it entirely — without +# forking or recompiling the binary. +# +# Plugin scripts dropped in the plugin directory are auto-discovered and +# registered as model-visible tools alongside the built-in ones. +# +# Scripts receive the tool's JSON input on **stdin** and must return a +# JSON `ToolResult` (`{"content": "...", "success": true}`) on **stdout**. +# +# [tools] +# # Custom plugin directory (defaults to `~/.codewhale/tools/`) +# plugin_dir = "~/.codewhale/tools" +# +# [tools.overrides] +# # Disable a tool entirely — removes it from the model-visible catalog. +# "code_execution" = { type = "disabled" } +# +# # Replace a tool with a script. Relative paths resolve against plugin_dir. +# "exec_shell" = { type = "script", path = "audit-exec-shell.sh" } +# +# # Replace a tool with a command (binary on PATH or absolute path). +# "read_file" = { type = "command", command = "bat", args = ["--paging=never"] } +# +# # Scripts can also accept static arguments before the JSON input: +# "fetch_url" = { type = "script", path = "cached-fetch.sh", args = ["--ttl", "300"] } + +# ──────────── Enterprise example: audit-logging exec_shell wrapper ────────────── +# Drop `audit-exec-shell.sh` in `~/.codewhale/tools/` and enable with: +# +# [tools.overrides] +# "exec_shell" = { type = "script", path = "audit-exec-shell.sh" } +# +# The wrapper logs every command to `~/.codewhale/audit/exec_shell.log` before +# executing it, then runs the real `exec_shell` tool logic via stdin/stdout +# passthrough. No code changes, no fork, no recompile. +# +# ```sh +# #!/usr/bin/env sh +# # name: exec_shell +# # description: Audit-logging wrapper for exec_shell +# # approval: required +# LOGDIR="${HOME}/.codewhale/audit" +# mkdir -p "$LOGDIR" +# LOGFILE="$LOGDIR/exec_shell.log" +# input=$(cat) +# echo "[$(date -Iseconds)] $input" >> "$LOGFILE" +# echo "$input" | exec /bin/sh -s +# ``` + # ───────────────────────────────────────────────────────────────────────────────── # Requirements (admin constraints) example file # ───────────────────────────────────────────────────────────────────────────────── diff --git a/crates/tui/src/config.rs b/crates/tui/src/config.rs index 0dae7de9f..1d1ae7d93 100644 --- a/crates/tui/src/config.rs +++ b/crates/tui/src/config.rs @@ -830,6 +830,19 @@ pub struct ToolsConfig { /// default core catalog. Unknown names are harmless and simply never match. #[serde(default)] pub always_load: Vec, + + /// Optional directory to scan for plugin tool scripts. Scripts with a + /// frontmatter header (`# name:`, `# description:`, `# schema:`) are + /// auto-discovered and registered as tools. + /// + /// Defaults to `~/.codewhale/tools/` when `None`. + #[serde(default)] + pub plugin_dir: Option, + + /// Per-tool overrides keyed by built-in tool name. + /// Each override replaces or disables the named tool. + #[serde(default)] + pub overrides: Option>, } /// One configurable footer item. @@ -1301,6 +1314,33 @@ pub struct Config { pub vision_model: Option, } +/// How a user wants to replace or disable a built-in tool. +#[derive(Debug, Clone, Deserialize)] +#[serde(tag = "type", rename_all = "snake_case")] +pub enum ToolOverride { + /// Run a local script file. The script receives the tool's JSON input + /// on stdin and must return a JSON `ToolResult` on stdout. + Script { + /// Path to the script (absolute, or relative to `~/.codewhale/tools/`). + path: String, + /// Optional static arguments prepended before the tool's JSON input. + #[serde(default)] + args: Option>, + }, + /// Run an external command. The command receives the tool's JSON input + /// on stdin and must return a JSON `ToolResult` on stdout. + Command { + /// The command to run (binary name or absolute path). + command: String, + /// Optional static arguments prepended before the tool's JSON input. + #[serde(default)] + args: Option>, + }, + /// Completely disable a built-in tool. The tool will not appear in the + /// model-visible catalog and cannot be called. + Disabled, +} + /// Vision model configuration for the `image_analyze` tool. /// Uses an OpenAI-compatible vision model API. #[derive(Debug, Clone, Deserialize)] diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index b45f0bea2..faab6f3ea 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -194,6 +194,10 @@ pub struct EngineConfig { /// through bubblewrap instead of relying solely on Landlock (#2184). #[allow(dead_code)] // Wired through ShellManager in follow-up PR pub prefer_bwrap: bool, + /// Tool override and plugin configuration (`[tools]` table in config.toml). + /// Applied to the per-turn tool registry after built-in tools are registered. + /// When `None`, no overrides or plugin loading occurs. + pub tools: Option, } impl Default for EngineConfig { @@ -242,6 +246,7 @@ impl Default for EngineConfig { ), tools_always_load: HashSet::new(), prefer_bwrap: false, + tools: None, } } } @@ -1192,7 +1197,7 @@ impl Engine { None }; - let tool_registry = match mode { + let mut tool_registry = match mode { AppMode::Agent | AppMode::Yolo => { if self.config.features.enabled(Feature::Subagents) { let runtime = if let Some(client) = self.deepseek_client.clone() { @@ -1241,18 +1246,55 @@ impl Engine { _ => Some(builder.build(tool_context)), }; + // Load plugin tools from the user's tools directory and apply any + // config.toml overrides. Plugin scripts are auto-discovered and + // registered without requiring a `[tools]` config section — the + // default `~/.codewhale/tools/` directory is always checked. + let mut plugin_tool_names: std::collections::HashSet = + std::collections::HashSet::new(); + if let Some(ref mut tool_registry) = tool_registry { + let names_before: std::collections::HashSet = tool_registry + .names() + .into_iter() + .map(|s| s.to_string()) + .collect(); + + let plugin_dir = plugin_tools_dir(self.config.tools.as_ref()); + + if let Some(ref tools_config) = self.config.tools + && let Some(ref overrides) = tools_config.overrides + { + tool_registry.apply_overrides(overrides, &plugin_dir); + } + + tool_registry.load_plugins(&plugin_dir); + + let names_after: std::collections::HashSet = tool_registry + .names() + .into_iter() + .map(|s| s.to_string()) + .collect(); + plugin_tool_names = &names_after - &names_before; + } + let mcp_tools = if self.config.features.enabled(Feature::Mcp) { self.mcp_tools().await } else { Vec::new() }; let tools = tool_registry.as_ref().map(|registry| { - build_model_tool_catalog( + let mut catalog = build_model_tool_catalog( registry.to_api_tools_with_cache(true), mcp_tools, mode, &self.config.tools_always_load, - ) + ); + for tool in &mut catalog { + if plugin_tool_names.contains(&tool.name) { + tool.defer_loading = Some(false); + } + } + catalog }); // Main turn loop @@ -2113,6 +2155,23 @@ impl Engine { } } +fn default_plugin_tools_dir() -> PathBuf { + codewhale_config::codewhale_home() + .unwrap_or_else(|_| { + dirs::home_dir().map_or_else(|| PathBuf::from(".codewhale"), |h| h.join(".codewhale")) + }) + .join("tools") +} + +fn plugin_tools_dir(tools_config: Option<&crate::config::ToolsConfig>) -> PathBuf { + if let Some(tools_config) = tools_config + && let Some(custom_dir) = tools_config.plugin_dir.as_deref() + { + return PathBuf::from(shellexpand::tilde(custom_dir).as_ref()); + } + default_plugin_tools_dir() +} + fn system_prompt_hash(prompt: Option<&SystemPrompt>) -> u64 { let mut hasher = DefaultHasher::new(); match prompt { diff --git a/crates/tui/src/core/engine/tests.rs b/crates/tui/src/core/engine/tests.rs index f73973619..776c9e644 100644 --- a/crates/tui/src/core/engine/tests.rs +++ b/crates/tui/src/core/engine/tests.rs @@ -115,6 +115,17 @@ fn config_auth_error_does_not_blame_env() { assert_eq!(message, "Authentication failed: invalid API key"); } +#[test] +fn plugin_tools_dir_honors_missing_custom_directory_without_fallback() { + let missing = PathBuf::from("definitely-missing-codewhale-plugin-dir"); + let tools_config = crate::config::ToolsConfig { + plugin_dir: Some(missing.to_string_lossy().to_string()), + ..Default::default() + }; + + assert_eq!(plugin_tools_dir(Some(&tools_config)), missing); +} + fn make_plan( read_only: bool, supports_parallel: bool, diff --git a/crates/tui/src/main.rs b/crates/tui/src/main.rs index 0954a1c92..457cd4b54 100644 --- a/crates/tui/src/main.rs +++ b/crates/tui/src/main.rs @@ -1530,8 +1530,12 @@ fn tools_readme_template() -> &'static str { "# Local tools\n\n\ Drop self-describing scripts here so they can be discovered by\n\ `codewhale-tui setup --status` and surfaced in `codewhale-tui doctor`.\n\n\ + When `[tools.plugin_dir]` is set in config.toml (or when the default\n\ + `~/.codewhale/tools/` directory exists), they are auto-discovered and\n\ + registered as model-visible tools.\n\n\ Each script should start with a frontmatter-style header so the\n\ - description is visible without executing the file:\n\n\ + description is visible without executing the file and the agent knows\n\ + the tool name, description, and input schema:\n\n\ ```\n\ # name: my-tool\n\ # description: One-line summary of what this tool does\n\ @@ -5375,6 +5379,7 @@ async fn run_exec_agent( search_provider: config.search_provider(), search_api_key: config.search.as_ref().and_then(|s| s.api_key.clone()), tools_always_load: config.tools_always_load(), + tools: config.tools.clone(), }; let engine_handle = spawn_engine(engine_config, config); diff --git a/crates/tui/src/runtime_threads.rs b/crates/tui/src/runtime_threads.rs index 22ae0218c..7f259535e 100644 --- a/crates/tui/src/runtime_threads.rs +++ b/crates/tui/src/runtime_threads.rs @@ -2027,6 +2027,7 @@ impl RuntimeThreadManager { search_provider: self.config.search_provider(), search_api_key: self.config.search.as_ref().and_then(|s| s.api_key.clone()), tools_always_load: self.config.tools_always_load(), + tools: self.config.tools.clone(), }; let engine = spawn_engine(engine_cfg, &self.config); diff --git a/crates/tui/src/tools/mod.rs b/crates/tui/src/tools/mod.rs index e54270654..db1e0f707 100644 --- a/crates/tui/src/tools/mod.rs +++ b/crates/tui/src/tools/mod.rs @@ -33,6 +33,7 @@ pub mod notify; pub mod pandoc; pub mod parallel; pub mod plan; +pub mod plugin; pub mod project; pub mod recall_archive; pub mod registry; diff --git a/crates/tui/src/tools/plugin.rs b/crates/tui/src/tools/plugin.rs new file mode 100644 index 000000000..017467a72 --- /dev/null +++ b/crates/tui/src/tools/plugin.rs @@ -0,0 +1,840 @@ +//! Plugin tool system — scripts and commands as first-class tools. +//! +//! Users can drop self-describing scripts in `~/.codewhale/tools/` and they +//! are auto-discovered, parsed for frontmatter, and registered as model-visible +//! tools alongside built-in implementations. +//! +//! # Script frontmatter format +//! +//! Every plugin script must have a frontmatter header in its first 20 lines: +//! +//! ```sh +//! # name: my-tool +//! # description: Does something useful +//! # schema: {"type":"object","properties":{"input":{"type":"string"}}} +//! # approval: auto +//! ``` +//! +//! The script receives the tool's JSON input on **stdin** and must return +//! a JSON `ToolResult` (`{"content": "...", "success": true}`) on **stdout**. +//! Non-JSON output is wrapped in a `ToolResult` with `success: false`. + +use std::path::{Path, PathBuf}; +use std::sync::Arc; +use std::time::Duration; + +use async_trait::async_trait; +use serde_json::Value; +use tokio::io::AsyncWriteExt; + +use super::spec::{ + ApprovalRequirement, ToolCapability, ToolContext, ToolError, ToolResult, ToolSpec, +}; + +use crate::config::ToolOverride; + +/// Timeout for plugin script execution (120 seconds). +const PLUGIN_EXECUTION_TIMEOUT: Duration = Duration::from_secs(120); + +/// Metadata extracted from a plugin script's frontmatter header. +#[derive(Debug, Clone)] +pub struct PluginMetadata { + /// Tool name (from `# name:`). + pub name: String, + /// Human-readable description (from `# description:`). + pub description: String, + /// JSON Schema for the tool's input (from `# schema:`). + /// Defaults to a permissive `{"type": "object"}` when absent. + pub input_schema: Value, + /// Approval requirement (from `# approval:`). + /// Defaults to `Suggest`. + pub approval: ApprovalRequirement, +} + +/// A tool backed by an external script or executable dropped into the +/// plugins directory. The script receives JSON input on stdin and writes +/// a JSON `ToolResult` to stdout. +struct ScriptPluginTool { + metadata: PluginMetadata, + /// Absolute path to the script. + script_path: PathBuf, + /// Optional static arguments passed before the JSON input. + args: Vec, +} + +impl std::fmt::Debug for ScriptPluginTool { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("ScriptPluginTool") + .field("name", &self.metadata.name) + .field("script_path", &self.script_path) + .finish() + } +} + +#[async_trait] +impl ToolSpec for ScriptPluginTool { + fn name(&self) -> &str { + &self.metadata.name + } + + fn description(&self) -> &str { + &self.metadata.description + } + + fn input_schema(&self) -> Value { + self.metadata.input_schema.clone() + } + + fn capabilities(&self) -> Vec { + // Unknown plugin — conservative: mark as requiring execution + approval. + vec![ + ToolCapability::ExecutesCode, + ToolCapability::RequiresApproval, + ] + } + + fn approval_requirement(&self) -> ApprovalRequirement { + self.metadata.approval + } + + async fn execute(&self, input: Value, _context: &ToolContext) -> Result { + let (interpreter, script_args) = script_command_parts(&self.script_path, &self.args); + let label = self.script_path.display().to_string(); + run_plugin_child(&interpreter, &script_args, &label, input).await + } +} + +/// A tool backed by an arbitrary shell command from config.toml overrides. +/// Behaves like `ScriptPluginTool` but uses the user-specified command string. +struct CommandPluginTool { + name: String, + description: String, + input_schema: Value, + command: String, + args: Vec, + approval: ApprovalRequirement, +} + +impl std::fmt::Debug for CommandPluginTool { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("CommandPluginTool") + .field("name", &self.name) + .field("command", &self.command) + .finish() + } +} + +#[async_trait] +impl ToolSpec for CommandPluginTool { + fn name(&self) -> &str { + &self.name + } + + fn description(&self) -> &str { + &self.description + } + + fn input_schema(&self) -> Value { + self.input_schema.clone() + } + + fn capabilities(&self) -> Vec { + vec![ + ToolCapability::ExecutesCode, + ToolCapability::RequiresApproval, + ] + } + + fn approval_requirement(&self) -> ApprovalRequirement { + self.approval + } + + async fn execute(&self, input: Value, _context: &ToolContext) -> Result { + // On Windows, if the command doesn't have an extension, try wrapping + // in `cmd /c` or use `powershell` for `.ps1` files. For portability + // we let tokio::process::Command resolve via PATH. + let mut cmd = if cfg!(windows) && !self.command.contains('.') { + let mut c = tokio::process::Command::new("cmd"); + c.arg("/c").arg(&self.command); + c + } else { + tokio::process::Command::new(&self.command) + }; + cmd.args(&self.args); + let label = format!("command '{}'", self.command); + run_plugin_child_raw(&mut cmd, &label, input).await + } +} + +// --------------------------------------------------------------------------- +// Script interpreter resolution +// --------------------------------------------------------------------------- + +/// Parse a shebang line (`#!/usr/bin/env node`) to extract the interpreter. +fn parse_shebang(path: &Path) -> Option<(String, Vec)> { + let mut file = std::fs::File::open(path).ok()?; + let content = read_prefix_to_string(&mut file, 256)?; + let first_line = content.lines().next()?; + let rest = first_line.strip_prefix("#!")?; + let parts: Vec<&str> = rest.split_whitespace().collect(); + if parts.is_empty() { + return None; + } + let interpreter = parts[0].to_string(); + let args: Vec = parts[1..].iter().map(|s| s.to_string()).collect(); + Some((interpreter, args)) +} + +/// Resolve the interpreter binary and pre-args for a script file. +/// +/// Priority: +/// 1. Shebang line from the script itself (`#!/usr/bin/env node`) +/// 2. Extension-based fallback for known script types +/// 3. Direct execution (assumes the OS knows how to run it) +fn resolve_interpreter(path: &Path) -> (String, Vec) { + // 1. Try shebang + if let Some((interp, shebang_args)) = parse_shebang(path) { + let bin_name = interp.rsplit('/').next().unwrap_or(&interp); + // `env` is a special case: `#!/usr/bin/env node` → `node` + // On Windows, `env` is not available, so extract the intended binary. + if bin_name == "env" && !shebang_args.is_empty() { + return (shebang_args[0].clone(), shebang_args[1..].to_vec()); + } + if cfg!(windows) { + return (bin_name.to_string(), shebang_args); + } + return (interp, shebang_args); + } + + // 2. Extension-based fallback for common script types + let ext = path + .extension() + .and_then(|e| e.to_str()) + .unwrap_or("") + .to_lowercase(); + match ext.as_str() { + "ps1" => ("powershell".into(), vec!["-File".into()]), + "py" => ("python".into(), vec![]), + "js" | "mjs" => ("node".into(), vec![]), + "ts" => ("npx".into(), vec!["tsx".into()]), + "rb" => ("ruby".into(), vec![]), + "sh" | "bash" | "zsh" => { + // On Windows, route shell scripts through sh if available + if cfg!(windows) { + ("sh".into(), vec![]) + } else { + (path.to_string_lossy().into(), vec![]) + } + } + _ => (path.to_string_lossy().into(), vec![]), + } +} + +fn script_command_parts(script_path: &Path, args: &[String]) -> (String, Vec) { + let (interpreter, mut script_args) = resolve_interpreter(script_path); + let script_path_arg = script_path.to_string_lossy().to_string(); + if interpreter != script_path_arg { + script_args.push(script_path_arg); + } + script_args.extend(args.iter().cloned()); + (interpreter, script_args) +} + +fn read_prefix_to_string(reader: impl std::io::Read, max_bytes: u64) -> Option { + use std::io::Read; + + let mut buf = Vec::new(); + reader.take(max_bytes).read_to_end(&mut buf).ok()?; + Some(String::from_utf8_lossy(&buf).into_owned()) +} + +// --------------------------------------------------------------------------- +// Shared child process helpers +// --------------------------------------------------------------------------- + +/// Spawn a command, pipe JSON input to stdin, collect ToolResult from stdout. +async fn run_plugin_child( + command: &str, + args: &[String], + label: &str, + input: Value, +) -> Result { + let mut cmd = tokio::process::Command::new(command); + cmd.args(args); + run_plugin_child_raw(&mut cmd, label, input).await +} + +/// Run a pre-configured tokio Command, pipe JSON input, collect ToolResult. +async fn run_plugin_child_raw( + cmd: &mut tokio::process::Command, + label: &str, + input: Value, +) -> Result { + let input_bytes = serde_json::to_vec(&input) + .map_err(|e| ToolError::invalid_input(format!("failed to serialize input: {e}")))?; + + cmd.stdin(std::process::Stdio::piped()); + cmd.stdout(std::process::Stdio::piped()); + cmd.stderr(std::process::Stdio::piped()); + + let mut child = cmd + .spawn() + .map_err(|e| ToolError::execution_failed(format!("failed to spawn {label}: {e}")))?; + + let stdin_writer = child.stdin.take().map(|mut stdin| { + tokio::spawn(async move { + if stdin.write_all(&input_bytes).await.is_ok() { + let _ = stdin.shutdown().await; + } + }) + }); + + let output = tokio::time::timeout(PLUGIN_EXECUTION_TIMEOUT, child.wait_with_output()) + .await + .map_err(|_| ToolError::Timeout { + seconds: PLUGIN_EXECUTION_TIMEOUT.as_secs(), + })? + .map_err(|e| ToolError::execution_failed(format!("process error: {e}")))?; + + if let Some(stdin_writer) = stdin_writer { + let _ = stdin_writer.await; + } + + if output.status.success() { + let stdout = String::from_utf8_lossy(&output.stdout).to_string(); + if let Ok(parsed) = serde_json::from_str::(&stdout) { + Ok(parsed) + } else { + Ok(ToolResult::success(stdout)) + } + } else { + let stderr = String::from_utf8_lossy(&output.stderr).to_string(); + let stdout = String::from_utf8_lossy(&output.stdout).to_string(); + let combined = if stderr.is_empty() { + stdout + } else if stdout.is_empty() { + stderr + } else { + format!("{stdout}\n{stderr}") + }; + Err(ToolError::execution_failed(combined)) + } +} + +// --------------------------------------------------------------------------- +// Frontmatter parsing +// --------------------------------------------------------------------------- + +/// Parse frontmatter header from the first `max_lines` lines of a text file. +/// +/// Expected format (one `# key: value` per line): +/// ```text +/// # name: my-tool +/// # description: Does something +/// # schema: {"type":"object"} +/// # approval: auto +/// ``` +/// +/// Also supports `// ` prefix for JavaScript/TypeScript scripts and `-- ` for Lua. +pub fn parse_frontmatter(content: &str) -> PluginMetadata { + let mut name = String::new(); + let mut description = String::new(); + let mut schema_str = String::new(); + let mut approval_str = String::new(); + + for line in content.lines().take(20) { + let line = line.trim(); + // Strip leading comment markers: `#`, `//`, `--`. + let rest = line + .strip_prefix('#') + .or_else(|| line.strip_prefix("//")) + .or_else(|| line.strip_prefix("--")); + let Some(rest) = rest else { continue }; + if let Some((key, value)) = rest.trim_start().split_once(':') { + let key = key.trim().to_lowercase(); + let value = value.trim(); + match key.as_str() { + "name" => name = value.to_string(), + "description" => description = value.to_string(), + "schema" => schema_str = value.to_string(), + "approval" => approval_str = value.to_string(), + _ => {} + } + } + } + + let input_schema = if schema_str.is_empty() { + // Default: accept any object payload + serde_json::json!({"type": "object"}) + } else { + serde_json::from_str(&schema_str).unwrap_or_else(|_| serde_json::json!({"type": "object"})) + }; + + let approval = match approval_str.to_lowercase().as_str() { + "auto" => ApprovalRequirement::Auto, + "required" => ApprovalRequirement::Required, + _ => ApprovalRequirement::Suggest, + }; + + PluginMetadata { + name: if name.is_empty() { + "unnamed-plugin".to_string() + } else { + name + }, + description: if description.is_empty() { + "User-provided plugin tool".to_string() + } else { + description + }, + input_schema, + approval, + } +} + +/// Read the first 4 KB of a file and parse its frontmatter. +fn read_script_metadata(path: &Path) -> Option { + let mut file = std::fs::File::open(path).ok()?; + let content = read_prefix_to_string(&mut file, 4096)?; + let meta = parse_frontmatter(&content); + // Require at least the `name` field to consider it a valid plugin. + if meta.name == "unnamed-plugin" { + return None; + } + Some(meta) +} + +// --------------------------------------------------------------------------- +// Directory scanning +// --------------------------------------------------------------------------- + +/// Scan a directory for plugin script files with frontmatter headers. +/// +/// Files are considered eligible when: +/// - They are regular files (not directories, not symlinks) +/// - They don't start with `.` (hidden files) +/// - They are not `README.md` +/// - Their first 20 lines contain `# name:` frontmatter +pub fn scan_plugin_dir(dir: &Path) -> Vec<(PathBuf, PluginMetadata)> { + let mut results = Vec::new(); + + let entries = match std::fs::read_dir(dir) { + Ok(entries) => entries, + Err(e) => { + tracing::warn!("Failed to read plugin directory {}: {e}", dir.display()); + return results; + } + }; + + for entry in entries.flatten() { + let path = entry.path(); + + // Skip directories and hidden files + if path.is_dir() { + continue; + } + if let Some(name) = path.file_name().and_then(|n| n.to_str()) { + if name.starts_with('.') || name == "README.md" { + continue; + } + } + + // Try to parse frontmatter + if let Some(meta) = read_script_metadata(&path) { + results.push((path, meta)); + } + } + + results +} + +/// Load all plugin tools from a directory. Each eligible script becomes +/// a registered `ScriptPluginTool`. +pub fn load_plugin_tools(plugin_dir: &Path) -> Vec> { + let discovered = scan_plugin_dir(plugin_dir); + let mut tools: Vec> = Vec::with_capacity(discovered.len()); + + for (path, meta) in discovered { + tracing::info!( + "Discovered plugin tool '{}' at {}", + meta.name, + path.display() + ); + tools.push(Arc::new(ScriptPluginTool { + metadata: meta, + script_path: path, + args: Vec::new(), + })); + } + + tools +} + +/// Create a single tool from a `ToolOverride` config entry. +/// +/// Returns `None` for `Disabled` (the caller handles removal separately). +pub fn tool_from_override( + tool_name: &str, + override_cfg: &ToolOverride, + plugin_dir: &Path, +) -> Option> { + match override_cfg { + ToolOverride::Disabled => None, + ToolOverride::Script { path, args } => { + let script_path = if Path::new(path).is_absolute() { + PathBuf::from(path) + } else { + // Relative paths resolve relative to the plugin directory. + plugin_dir.join(path) + }; + + if !script_path.exists() { + tracing::warn!( + "Override script for '{}' not found at {}", + tool_name, + script_path.display() + ); + return None; + } + + // Read the script's own frontmatter for metadata, or provide + // defaults if it has none. + let meta = read_script_metadata(&script_path).unwrap_or_else(|| PluginMetadata { + name: tool_name.to_string(), + description: format!("Override for built-in tool '{tool_name}'"), + input_schema: serde_json::json!({"type": "object"}), + approval: ApprovalRequirement::Suggest, + }); + + Some(Arc::new(ScriptPluginTool { + metadata: meta, + script_path, + args: args.clone().unwrap_or_default(), + }) as Arc) + } + ToolOverride::Command { command, args } => { + // Build a description that includes the command. + let description = format!("Override for '{tool_name}' — runs: {command}"); + let cmd_args = args.clone().unwrap_or_default(); + + Some(Arc::new(CommandPluginTool { + name: tool_name.to_string(), + description, + input_schema: serde_json::json!({"type": "object"}), + command: command.clone(), + args: cmd_args, + approval: ApprovalRequirement::Suggest, + }) as Arc) + } + } +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::TempDir; + + const DEADLOCK_CHILD_ENV: &str = "CODEWHALE_PLUGIN_DEADLOCK_CHILD"; + + #[test] + fn test_parse_frontmatter_full() { + let content = "\ +#!/usr/bin/env sh +# name: my-tool +# description: A useful custom tool +# schema: {\"type\":\"object\",\"properties\":{\"input\":{\"type\":\"string\"}}} +# approval: required +echo hello +"; + let meta = parse_frontmatter(content); + assert_eq!(meta.name, "my-tool"); + assert_eq!(meta.description, "A useful custom tool"); + assert_eq!(meta.approval, ApprovalRequirement::Required); + assert_eq!( + meta.input_schema, + serde_json::json!({"type":"object","properties":{"input":{"type":"string"}}}) + ); + } + + #[test] + fn test_parse_frontmatter_accepts_compact_and_spaced_markers() { + let content = "\ +#!/usr/bin/env node +#name:compact-name +// description: spaced description +-- schema : {\"type\":\"object\",\"properties\":{\"ok\":{\"type\":\"boolean\"}}} +# approval: auto +"; + + let meta = parse_frontmatter(content); + + assert_eq!(meta.name, "compact-name"); + assert_eq!(meta.description, "spaced description"); + assert_eq!(meta.approval, ApprovalRequirement::Auto); + assert_eq!( + meta.input_schema, + serde_json::json!({"type":"object","properties":{"ok":{"type":"boolean"}}}) + ); + } + + #[test] + fn test_parse_frontmatter_minimal() { + let content = "# name: mini"; + let meta = parse_frontmatter(content); + assert_eq!(meta.name, "mini"); + assert_eq!(meta.description, "User-provided plugin tool"); + assert_eq!(meta.approval, ApprovalRequirement::Suggest); + } + + #[test] + fn test_parse_frontmatter_missing_name() { + let content = "# description: no name here"; + let meta = parse_frontmatter(content); + assert_eq!(meta.name, "unnamed-plugin"); + // read_script_metadata would return None for this. + } + + #[test] + fn test_read_prefix_collects_multiple_short_reads() { + struct OneByteReader { + bytes: Vec, + pos: usize, + } + + impl std::io::Read for OneByteReader { + fn read(&mut self, buf: &mut [u8]) -> std::io::Result { + if self.pos >= self.bytes.len() { + return Ok(0); + } + buf[0] = self.bytes[self.pos]; + self.pos += 1; + Ok(1) + } + } + + let reader = OneByteReader { + bytes: b"# name: short-read\n# description: ok\n".to_vec(), + pos: 0, + }; + + assert_eq!( + read_prefix_to_string(reader, 4096).as_deref(), + Some("# name: short-read\n# description: ok\n") + ); + } + + #[test] + fn test_resolve_interpreter_handles_absolute_shebang_by_platform() { + let dir = TempDir::new().unwrap(); + let script = dir.path().join("tool"); + std::fs::write( + &script, + "#!/opt/custom/bin/tool-runner --safe\n# name: tool\n", + ) + .unwrap(); + + let (interpreter, args) = resolve_interpreter(&script); + + if cfg!(windows) { + assert_eq!(interpreter, "tool-runner"); + } else { + assert_eq!(interpreter, "/opt/custom/bin/tool-runner"); + } + assert_eq!(args, vec!["--safe"]); + } + + #[test] + fn test_script_command_parts_does_not_pass_direct_script_as_own_arg() { + let dir = TempDir::new().unwrap(); + let script = dir.path().join("direct-tool"); + std::fs::write(&script, "# name: direct\n").unwrap(); + + let (interpreter, args) = + script_command_parts(&script, &["--flag".to_string(), "value".to_string()]); + + assert_eq!(interpreter, script.to_string_lossy()); + assert_eq!(args, vec!["--flag", "value"]); + } + + #[test] + fn test_script_command_parts_passes_script_to_external_interpreter() { + let dir = TempDir::new().unwrap(); + let script = dir.path().join("script.py"); + std::fs::write(&script, "# name: py\n").unwrap(); + + let (interpreter, args) = script_command_parts(&script, &["--flag".to_string()]); + + assert_eq!(interpreter, "python"); + assert_eq!( + args, + vec![script.to_string_lossy().to_string(), "--flag".to_string()] + ); + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_run_plugin_child_drains_stdout_while_writing_large_stdin() { + let mut cmd = tokio::process::Command::new(std::env::current_exe().unwrap()); + cmd.arg("plugin_deadlock_child_process") + .arg("--nocapture") + .env(DEADLOCK_CHILD_ENV, "1"); + + let input = serde_json::json!({ "payload": "y".repeat(1024 * 1024) }); + let result = tokio::time::timeout( + Duration::from_secs(10), + run_plugin_child_raw(&mut cmd, "deadlock child", input), + ) + .await + .expect("plugin execution should not deadlock") + .expect("plugin child should succeed"); + + assert!(result.success); + assert!(result.content.len() > 64 * 1024); + } + + #[test] + fn plugin_deadlock_child_process() { + if std::env::var_os(DEADLOCK_CHILD_ENV).is_none() { + return; + } + + use std::io::{Read, Write}; + + let mut stdout = std::io::stdout(); + stdout.write_all(&vec![b'x'; 1024 * 1024]).unwrap(); + stdout.flush().unwrap(); + + let mut stdin = Vec::new(); + std::io::stdin().read_to_end(&mut stdin).unwrap(); + writeln!( + stdout, + "{{\"content\":\"read {} bytes\",\"success\":true}}", + stdin.len() + ) + .unwrap(); + std::process::exit(0); + } + + #[test] + fn test_scan_plugin_dir_finds_scripts() { + let dir = TempDir::new().unwrap(); + + // Valid plugin + std::fs::write( + dir.path().join("my-plugin.sh"), + "# name: my-plugin\n# description: test\n", + ) + .unwrap(); + + // Hidden file — should be skipped + std::fs::write( + dir.path().join(".hidden.sh"), + "# name: hidden\n# description: should skip\n", + ) + .unwrap(); + + // README — should be skipped + std::fs::write(dir.path().join("README.md"), "# Tools\n").unwrap(); + + // No frontmatter — should be skipped + std::fs::write(dir.path().join("random.sh"), "echo hi\n").unwrap(); + + let discovered = scan_plugin_dir(dir.path()); + assert_eq!(discovered.len(), 1); + assert_eq!(discovered[0].1.name, "my-plugin"); + } + + #[test] + fn test_load_plugin_tools_creates_tools() { + let dir = TempDir::new().unwrap(); + std::fs::write( + dir.path().join("greet.sh"), + "# name: greet\n# description: Say hello\n# schema: {\"type\":\"object\",\"properties\":{\"name\":{\"type\":\"string\"}},\"required\":[\"name\"]}\n", + ) + .unwrap(); + + let tools = load_plugin_tools(dir.path()); + assert_eq!(tools.len(), 1); + assert_eq!(tools[0].name(), "greet"); + assert_eq!(tools[0].description(), "Say hello"); + } + + #[test] + fn test_tool_from_override_script() { + let dir = TempDir::new().unwrap(); + std::fs::write( + dir.path().join("wrapper.sh"), + "# name: exec_shell\n# description: Audit wrapper for exec_shell\n", + ) + .unwrap(); + + let override_cfg = ToolOverride::Script { + path: "wrapper.sh".to_string(), + args: None, + }; + + let tool = tool_from_override("exec_shell", &override_cfg, dir.path()); + assert!(tool.is_some()); + assert_eq!(tool.unwrap().name(), "exec_shell"); + } + + #[test] + fn test_tool_from_override_disabled() { + let dir = TempDir::new().unwrap(); + let override_cfg = ToolOverride::Disabled; + let tool = tool_from_override("code_execution", &override_cfg, dir.path()); + assert!(tool.is_none()); + } + + #[test] + fn test_tool_from_override_command() { + let dir = TempDir::new().unwrap(); + let override_cfg = ToolOverride::Command { + command: "my-custom-reader".to_string(), + args: Some(vec!["--format".to_string(), "json".to_string()]), + }; + let tool = tool_from_override("read_file", &override_cfg, dir.path()); + assert!(tool.is_some()); + assert_eq!(tool.unwrap().name(), "read_file"); + } + + #[test] + fn test_tool_from_override_script_absolute_path() { + let dir = TempDir::new().unwrap(); + let script_path = dir.path().join("audit.sh"); + std::fs::write(&script_path, "# name: exec_shell\n# description: Audit\n").unwrap(); + + let override_cfg = ToolOverride::Script { + path: script_path.to_str().unwrap().to_string(), + args: None, + }; + + let tool = tool_from_override("exec_shell", &override_cfg, dir.path()); + assert!(tool.is_some()); + } + + #[test] + fn test_approval_variants() { + let check = |content: &str, expected: ApprovalRequirement| { + assert_eq!(parse_frontmatter(content).approval, expected); + }; + + check("# name: x\n# approval: auto", ApprovalRequirement::Auto); + check( + "# name: x\n# approval: required", + ApprovalRequirement::Required, + ); + check( + "# name: x\n# approval: suggest", + ApprovalRequirement::Suggest, + ); + check( + "# name: x\n# approval: unknown", + ApprovalRequirement::Suggest, + ); + check("# name: x", ApprovalRequirement::Suggest); + } +} diff --git a/crates/tui/src/tools/registry.rs b/crates/tui/src/tools/registry.rs index a27daca3d..6b33f0cd9 100644 --- a/crates/tui/src/tools/registry.rs +++ b/crates/tui/src/tools/registry.rs @@ -9,6 +9,8 @@ use std::collections::HashMap; use std::sync::{Arc, OnceLock}; +use std::path::Path; + use serde_json::Value; use crate::client::DeepSeekClient; @@ -388,6 +390,77 @@ impl ToolRegistry { self.tools.clear(); self.invalidate_api_cache(); } + + /// Remove a tool from the registry by name. Returns `true` if the tool + /// was present and removed, `false` if no tool with that name existed. + pub fn remove_tool(&mut self, name: &str) -> bool { + let existed = self.tools.remove(name).is_some(); + if existed { + self.invalidate_api_cache(); + } + existed + } + + /// Apply config.toml tool overrides to this registry. + /// + /// For each entry in `overrides`: + /// - `Disabled` removes the tool. + /// - `Script` / `Command` replaces the tool with the user's implementation. + /// + /// `plugin_dir` is used as the base for relative script paths. + pub fn apply_overrides( + &mut self, + overrides: &std::collections::HashMap, + plugin_dir: &Path, + ) { + for (tool_name, override_cfg) in overrides { + match override_cfg { + crate::config::ToolOverride::Disabled => { + if self.remove_tool(tool_name) { + tracing::info!("Tool '{}' disabled via config override", tool_name); + } else { + tracing::warn!("Cannot disable tool '{}': not registered", tool_name); + } + } + _ => { + // Script and Command overrides create replacement tools. + use crate::tools::plugin::tool_from_override; + if let Some(replacement) = + tool_from_override(tool_name, override_cfg, plugin_dir) + { + self.register(replacement); + tracing::info!("Tool '{}' replaced via config override", tool_name); + } + } + } + } + } + + /// Load and register plugin tools from a directory. + /// + /// Each script with valid frontmatter (`# name:`, `# description:`, etc.) + /// becomes a registered `ScriptPluginTool`. Tools whose name matches an + /// already-registered tool will overwrite it. + pub fn load_plugins(&mut self, plugin_dir: &Path) { + if !plugin_dir.exists() { + tracing::debug!( + "Plugin directory {} does not exist, skipping", + plugin_dir.display() + ); + return; + } + let plugins = crate::tools::plugin::load_plugin_tools(plugin_dir); + let count = plugins.len(); + for tool in plugins { + self.register(tool); + } + if count > 0 { + tracing::info!( + "Loaded {count} plugin tool(s) from {}", + plugin_dir.display() + ); + } + } } /// Builder for constructing a `ToolRegistry` with common tools. diff --git a/crates/tui/src/tui/clipboard.rs b/crates/tui/src/tui/clipboard.rs index f6d92b256..d0f839340 100644 --- a/crates/tui/src/tui/clipboard.rs +++ b/crates/tui/src/tui/clipboard.rs @@ -1,7 +1,7 @@ //! Clipboard handling for paste support in TUI //! //! Supports text and image paste operations. Images on the clipboard are -//! encoded as PNG and persisted under `~/.deepseek/clipboard-images/` so the +//! encoded as PNG and persisted under `~/.codewhale/clipboard-images/` so the //! model can reach them via the existing `@`-mention / file tools (DeepSeek //! V4 does not currently accept inline image input on its Chat Completions //! endpoint, so we materialize the bytes to disk instead of base64-embedding @@ -104,7 +104,7 @@ impl ClipboardHandler { /// Read the clipboard and return the parsed content. /// - /// `workspace` is used as a fallback location when `~/.deepseek/` cannot + /// `workspace` is used as a fallback location when `~/.codewhale/` cannot /// be resolved (e.g. running with a stripped HOME in CI sandboxes). pub fn read(&mut self, workspace: &Path) -> Option { self.ensure_clipboard(); @@ -264,12 +264,17 @@ fn osc52_sequence(text: &str, in_tmux: bool) -> Result { } /// Resolve the directory pasted images should land in. Prefers -/// `~/.deepseek/clipboard-images/` so the path is stable across worktrees and +/// `~/.codewhale/clipboard-images/` so the path is stable across worktrees and /// matches the location described in user-facing docs; falls back to /// `/clipboard-images/` if the home dir is unavailable. pub(crate) fn clipboard_images_dir(workspace: &Path) -> PathBuf { - if let Some(home) = dirs::home_dir() { - return home.join(".deepseek").join("clipboard-images"); + let home = dirs::home_dir(); + clipboard_images_dir_for_home(workspace, home.as_deref()) +} + +fn clipboard_images_dir_for_home(workspace: &Path, home: Option<&Path>) -> PathBuf { + if let Some(home) = home { + return home.join(".codewhale").join("clipboard-images"); } workspace.join("clipboard-images") } @@ -360,6 +365,27 @@ mod tests { assert_eq!(&header[..8], b"\x89PNG\r\n\x1a\n"); } + #[test] + fn clipboard_images_dir_uses_codewhale_home_directory() { + let home = tempfile::tempdir().unwrap(); + let workspace = tempfile::tempdir().unwrap(); + + assert_eq!( + clipboard_images_dir_for_home(workspace.path(), Some(home.path())), + home.path().join(".codewhale").join("clipboard-images") + ); + } + + #[test] + fn clipboard_images_dir_falls_back_to_workspace_without_home() { + let workspace = tempfile::tempdir().unwrap(); + + assert_eq!( + clipboard_images_dir_for_home(workspace.path(), None), + workspace.path().join("clipboard-images") + ); + } + #[test] fn pasted_image_labels_format_correctly() { let p = PastedImage { diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index 6d7bda2a0..51ce7ec75 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -778,6 +778,7 @@ fn build_engine_config(app: &App, config: &Config) -> EngineConfig { search_provider: config.search_provider(), search_api_key: config.search.as_ref().and_then(|s| s.api_key.clone()), tools_always_load: config.tools_always_load(), + tools: config.tools.clone(), } } From 50887cc944da90bcce91e66082d75b15908bddd1 Mon Sep 17 00:00:00 2001 From: Hunter B Date: Sun, 31 May 2026 03:10:38 -0700 Subject: [PATCH 14/15] fix(tools): preserve explicit plugin override precedence Follow-up on #2420 after the contributor branch update. Keeps explicit overrides above auto-discovered tools, makes directory scan ordering deterministic, and fixes the audit wrapper example. --- config.example.toml | 8 ++++---- crates/tui/src/core/engine.rs | 9 ++++----- crates/tui/src/tools/plugin.rs | 5 ++++- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/config.example.toml b/config.example.toml index 89247b23d..a7ff5e097 100644 --- a/config.example.toml +++ b/config.example.toml @@ -688,9 +688,9 @@ default_text_model = "deepseek-ai/deepseek-v4-pro" # [tools.overrides] # "exec_shell" = { type = "script", path = "audit-exec-shell.sh" } # -# The wrapper logs every command to `~/.codewhale/audit/exec_shell.log` before -# executing it, then runs the real `exec_shell` tool logic via stdin/stdout -# passthrough. No code changes, no fork, no recompile. +# The wrapper logs every request to `~/.codewhale/audit/exec_shell.log`, then +# delegates to your own approved shell executor. Do not pipe the raw JSON +# request into `sh -s`; parse the command field and enforce your policy first. # # ```sh # #!/usr/bin/env sh @@ -702,7 +702,7 @@ default_text_model = "deepseek-ai/deepseek-v4-pro" # LOGFILE="$LOGDIR/exec_shell.log" # input=$(cat) # echo "[$(date -Iseconds)] $input" >> "$LOGFILE" -# echo "$input" | exec /bin/sh -s +# printf '%s\n' '{"content":"audit wrapper placeholder: configure an executor","success":false}' # ``` # ───────────────────────────────────────────────────────────────────────────────── diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index faab6f3ea..8c3f0478f 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -1247,9 +1247,8 @@ impl Engine { }; // Load plugin tools from the user's tools directory and apply any - // config.toml overrides. Plugin scripts are auto-discovered and - // registered without requiring a `[tools]` config section — the - // default `~/.codewhale/tools/` directory is always checked. + // config.toml overrides. Explicit overrides win over auto-discovered + // scripts with the same tool name. let mut plugin_tool_names: std::collections::HashSet = std::collections::HashSet::new(); if let Some(ref mut tool_registry) = tool_registry { @@ -1261,14 +1260,14 @@ impl Engine { let plugin_dir = plugin_tools_dir(self.config.tools.as_ref()); + tool_registry.load_plugins(&plugin_dir); + if let Some(ref tools_config) = self.config.tools && let Some(ref overrides) = tools_config.overrides { tool_registry.apply_overrides(overrides, &plugin_dir); } - tool_registry.load_plugins(&plugin_dir); - let names_after: std::collections::HashSet = tool_registry .names() .into_iter() diff --git a/crates/tui/src/tools/plugin.rs b/crates/tui/src/tools/plugin.rs index 017467a72..5dda30fad 100644 --- a/crates/tui/src/tools/plugin.rs +++ b/crates/tui/src/tools/plugin.rs @@ -426,7 +426,10 @@ pub fn scan_plugin_dir(dir: &Path) -> Vec<(PathBuf, PluginMetadata)> { } }; - for entry in entries.flatten() { + let mut entries: Vec<_> = entries.flatten().collect(); + entries.sort_by_key(|entry| entry.file_name()); + + for entry in entries { let path = entry.path(); // Skip directories and hidden files From a637159139de4d50fff7207d5adcc08e4742a751 Mon Sep 17 00:00:00 2001 From: Paulo Aboim Pinto Date: Sun, 31 May 2026 12:14:49 +0200 Subject: [PATCH 15/15] test(tui): cover plugin registry review regressions --- crates/tui/src/core/engine.rs | 50 ++++++++++++++++------------- crates/tui/src/core/engine/tests.rs | 37 ++++++++++++++++++++- crates/tui/src/tools/plugin.rs | 23 +++++++++++++ 3 files changed, 87 insertions(+), 23 deletions(-) diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index 8c3f0478f..d72d5863e 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -1252,28 +1252,7 @@ impl Engine { let mut plugin_tool_names: std::collections::HashSet = std::collections::HashSet::new(); if let Some(ref mut tool_registry) = tool_registry { - let names_before: std::collections::HashSet = tool_registry - .names() - .into_iter() - .map(|s| s.to_string()) - .collect(); - - let plugin_dir = plugin_tools_dir(self.config.tools.as_ref()); - - tool_registry.load_plugins(&plugin_dir); - - if let Some(ref tools_config) = self.config.tools - && let Some(ref overrides) = tools_config.overrides - { - tool_registry.apply_overrides(overrides, &plugin_dir); - } - - let names_after: std::collections::HashSet = tool_registry - .names() - .into_iter() - .map(|s| s.to_string()) - .collect(); - plugin_tool_names = &names_after - &names_before; + plugin_tool_names = configure_plugin_tools(tool_registry, self.config.tools.as_ref()); } let mcp_tools = if self.config.features.enabled(Feature::Mcp) { @@ -2171,6 +2150,33 @@ fn plugin_tools_dir(tools_config: Option<&crate::config::ToolsConfig>) -> PathBu default_plugin_tools_dir() } +fn configure_plugin_tools( + tool_registry: &mut crate::tools::ToolRegistry, + tools_config: Option<&crate::config::ToolsConfig>, +) -> std::collections::HashSet { + let names_before: std::collections::HashSet = tool_registry + .names() + .into_iter() + .map(|s| s.to_string()) + .collect(); + + let plugin_dir = plugin_tools_dir(tools_config); + tool_registry.load_plugins(&plugin_dir); + + if let Some(tools_config) = tools_config + && let Some(ref overrides) = tools_config.overrides + { + tool_registry.apply_overrides(overrides, &plugin_dir); + } + + let names_after: std::collections::HashSet = tool_registry + .names() + .into_iter() + .map(|s| s.to_string()) + .collect(); + &names_after - &names_before +} + fn system_prompt_hash(prompt: Option<&SystemPrompt>) -> u64 { let mut hasher = DefaultHasher::new(); match prompt { diff --git a/crates/tui/src/core/engine/tests.rs b/crates/tui/src/core/engine/tests.rs index 776c9e644..783f31283 100644 --- a/crates/tui/src/core/engine/tests.rs +++ b/crates/tui/src/core/engine/tests.rs @@ -5,7 +5,7 @@ use crate::models::SystemBlock; use crate::test_support::lock_test_env; use crate::tools::spec::ToolCapability; use serde_json::json; -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::ffi::OsString; use std::fs; use std::path::{Path, PathBuf}; @@ -126,6 +126,41 @@ fn plugin_tools_dir_honors_missing_custom_directory_without_fallback() { assert_eq!(plugin_tools_dir(Some(&tools_config)), missing); } +#[test] +fn configure_plugin_tools_applies_overrides_after_discovered_plugins() { + let tmp = tempdir().expect("tempdir"); + let plugin_dir = tmp.path().join("tools"); + fs::create_dir(&plugin_dir).expect("plugin dir"); + fs::write( + plugin_dir.join("same-name.sh"), + "# name: same_tool\n# description: discovered plugin\n", + ) + .expect("plugin script"); + + let mut overrides = HashMap::new(); + overrides.insert( + "same_tool".to_string(), + crate::config::ToolOverride::Command { + command: "configured-command".to_string(), + args: None, + }, + ); + let tools_config = crate::config::ToolsConfig { + plugin_dir: Some(plugin_dir.to_string_lossy().to_string()), + overrides: Some(overrides), + ..Default::default() + }; + + let ctx = crate::tools::ToolContext::new(tmp.path().to_path_buf()); + let mut registry = crate::tools::ToolRegistry::new(ctx); + + let plugin_names = configure_plugin_tools(&mut registry, Some(&tools_config)); + + let tool = registry.get("same_tool").expect("same_tool registered"); + assert!(tool.description().contains("configured-command")); + assert!(plugin_names.contains("same_tool")); +} + fn make_plan( read_only: bool, supports_parallel: bool, diff --git a/crates/tui/src/tools/plugin.rs b/crates/tui/src/tools/plugin.rs index 5dda30fad..b373d85ee 100644 --- a/crates/tui/src/tools/plugin.rs +++ b/crates/tui/src/tools/plugin.rs @@ -750,6 +750,29 @@ echo hello assert_eq!(discovered[0].1.name, "my-plugin"); } + #[test] + fn test_scan_plugin_dir_returns_files_sorted_by_name() { + let dir = TempDir::new().unwrap(); + std::fs::write( + dir.path().join("z-plugin.sh"), + "# name: z-plugin\n# description: z\n", + ) + .unwrap(); + std::fs::write( + dir.path().join("a-plugin.sh"), + "# name: a-plugin\n# description: a\n", + ) + .unwrap(); + + let discovered = scan_plugin_dir(dir.path()); + + let names: Vec<_> = discovered + .iter() + .map(|(_, meta)| meta.name.as_str()) + .collect(); + assert_eq!(names, vec!["a-plugin", "z-plugin"]); + } + #[test] fn test_load_plugin_tools_creates_tools() { let dir = TempDir::new().unwrap();