diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index d34611613..0b8dc2ee2 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -279,7 +279,11 @@ const overrides = new Map([ // +13: fetch_login_shell_path_inner Windows guard (POSIX PATH → None). // resolve_git_bash made pub(crate) for Windows test access. // +1: login_shell_candidates doc comment expanded for resolve_bash_path. - ["src-tauri/src/managed_agents/discovery.rs", 1366], + // bundle-acps: bundled ACP bridge check at the top of the resolution sweep + // (+4 lines over the Windows baseline). Temporary — the codex version-gate + // retirement later in the same series deletes far more from this file and + // ratchets this back down. + ["src-tauri/src/managed_agents/discovery.rs", 1371], // rebase over codex-acp-package-swap: its version-probe tests union with the // doctor-install-reliability nvm/login-shell/semver tests — each side alone // stayed under the 1000 default; the union exceeds it. diff --git a/desktop/src-tauri/src/lib.rs b/desktop/src-tauri/src/lib.rs index a9d37c5dc..adb7d691a 100644 --- a/desktop/src-tauri/src/lib.rs +++ b/desktop/src-tauri/src/lib.rs @@ -446,6 +446,12 @@ pub fn run() { return Ok(()); } + // Register the bundled ACP bridge tools dir before anything can + // resolve agent commands — resolutions are cached for the app + // lifetime, so a resolve that runs before registration would pin + // the user-installed copy instead of the bundled one. + managed_agents::acp_tools::register_bundled_acp_tools_dir(&app_handle); + // Run all pre-identity data migrations before state loads from disk. if reset_outcome.completed { migration::run_boot_migrations_after_reset(&app_handle); diff --git a/desktop/src-tauri/src/managed_agents/acp_tools.rs b/desktop/src-tauri/src/managed_agents/acp_tools.rs new file mode 100644 index 000000000..e7b4e444b --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/acp_tools.rs @@ -0,0 +1,162 @@ +//! Bundled ACP bridge tool resolution. +//! +//! Buzz ships pinned ACP bridge CLIs (`claude-agent-acp`, `codex-acp`) as +//! Tauri application resources (see `desktop/acp-tools.lock.json` and +//! `desktop/scripts/prepare-acp-tools-resource.sh`). This module resolves the +//! staged bin directory once at app setup so the command resolution sweep and +//! the spawn-time PATH augmentation both prefer the bundled bridges over +//! user-installed copies, while everything else on the user's PATH (including +//! installed harness CLIs and their auth state) stays discoverable. + +use std::ffi::OsStr; +use std::path::{Path, PathBuf}; +use std::sync::OnceLock; + +use tauri::path::BaseDirectory; +use tauri::Manager; + +use super::discovery::{command_looks_like_path, executable_basename, is_executable_file}; + +/// Dev-mode override exported by `just dev` / `just staging`, pointing at the +/// freshly staged `src-tauri/resources/acp/bin` in the working tree. +pub const ACP_TOOLS_DIR_ENV: &str = "BUZZ_ACP_TOOLS_DIR"; +/// Bundled resource path, relative to the Tauri resource dir (mirrors the +/// `resources/acp` entry in `tauri.conf.json`). +const ACP_TOOLS_RESOURCE_DIR: &str = "resources/acp/bin"; + +static BUNDLED_ACP_TOOLS_DIR: OnceLock> = OnceLock::new(); + +/// Resolve and register the bundled ACP tools bin dir for the app lifetime: +/// the dev env override wins, then the Tauri resource dir for packaged apps. +/// Called once during app setup, before anything can resolve agent commands. +pub fn register_bundled_acp_tools_dir(app_handle: &tauri::AppHandle) { + let resolved = bundled_acp_tools_dir_from_parts( + std::env::var_os(ACP_TOOLS_DIR_ENV).as_deref(), + app_handle + .path() + .resolve(ACP_TOOLS_RESOURCE_DIR, BaseDirectory::Resource) + .ok() + .as_deref(), + ); + let _ = BUNDLED_ACP_TOOLS_DIR.set(resolved); +} + +/// The registered bundled ACP tools bin dir, if the app ships one. `None` +/// until [`register_bundled_acp_tools_dir`] runs (e.g. in unit tests), so +/// every consumer degrades to the pre-bundling resolution order. +pub(crate) fn bundled_acp_tools_dir() -> Option { + BUNDLED_ACP_TOOLS_DIR.get().cloned().flatten() +} + +/// Resolve `command` inside the bundled tools dir. Bare command names only — +/// a path-like command (absolute or multi-component) names a specific binary +/// the user picked and must never be redirected into the bundle. +pub(in crate::managed_agents) fn command_in_bundled_dir(command: &str) -> Option { + command_in_dir(&bundled_acp_tools_dir()?, command) +} + +fn command_in_dir(dir: &Path, command: &str) -> Option { + if command_looks_like_path(command) { + return None; + } + let candidate = dir.join(executable_basename(command)); + is_executable_file(&candidate).then_some(candidate) +} + +fn bundled_acp_tools_dir_from_parts( + env_override: Option<&OsStr>, + resource_dir: Option<&Path>, +) -> Option { + if let Some(value) = env_override { + if !value.is_empty() { + return Some(PathBuf::from(value)); + } + } + resource_dir.map(Path::to_path_buf) +} + +#[cfg(test)] +mod tests { + use super::{bundled_acp_tools_dir_from_parts, command_in_dir}; + use std::ffi::OsStr; + use std::path::Path; + + #[test] + fn env_override_wins_over_resource_dir() { + assert_eq!( + bundled_acp_tools_dir_from_parts( + Some(OsStr::new("/dev/acp/bin")), + Some(Path::new("/bundle/resources/acp/bin")), + ) + .as_deref(), + Some(Path::new("/dev/acp/bin")), + ); + } + + #[test] + fn empty_env_override_falls_back_to_resource_dir() { + assert_eq!( + bundled_acp_tools_dir_from_parts( + Some(OsStr::new("")), + Some(Path::new("/bundle/resources/acp/bin")), + ) + .as_deref(), + Some(Path::new("/bundle/resources/acp/bin")), + ); + } + + #[test] + fn missing_inputs_resolve_to_none() { + assert!(bundled_acp_tools_dir_from_parts(None, None).is_none()); + } + + #[cfg(unix)] + #[test] + fn command_in_dir_finds_executable_by_bare_name() { + use std::fs; + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().expect("temp dir"); + let tool = temp.path().join("claude-agent-acp"); + fs::write(&tool, "#!/bin/sh\n").expect("write tool"); + fs::set_permissions(&tool, fs::Permissions::from_mode(0o755)).expect("chmod tool"); + + assert_eq!( + command_in_dir(temp.path(), "claude-agent-acp").as_deref(), + Some(tool.as_path()), + ); + assert!(command_in_dir(temp.path(), "codex-acp").is_none()); + } + + #[cfg(unix)] + #[test] + fn command_in_dir_rejects_path_like_commands() { + use std::fs; + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().expect("temp dir"); + let tool = temp.path().join("codex-acp"); + fs::write(&tool, "#!/bin/sh\n").expect("write tool"); + fs::set_permissions(&tool, fs::Permissions::from_mode(0o755)).expect("chmod tool"); + + // An absolute path joined onto the bundled dir would *replace* it + // (Path::join semantics) — path-like commands must pass through to + // the regular resolution order untouched. + assert!(command_in_dir(temp.path(), tool.to_str().expect("utf8")).is_none()); + assert!(command_in_dir(temp.path(), "custom/codex-acp").is_none()); + } + + #[cfg(unix)] + #[test] + fn command_in_dir_skips_non_executable_files() { + use std::fs; + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().expect("temp dir"); + let tool = temp.path().join("claude-agent-acp"); + fs::write(&tool, "not executable").expect("write tool"); + fs::set_permissions(&tool, fs::Permissions::from_mode(0o644)).expect("chmod tool"); + + assert!(command_in_dir(temp.path(), "claude-agent-acp").is_none()); + } +} diff --git a/desktop/src-tauri/src/managed_agents/discovery.rs b/desktop/src-tauri/src/managed_agents/discovery.rs index 5ba0500aa..2f64a6a3b 100644 --- a/desktop/src-tauri/src/managed_agents/discovery.rs +++ b/desktop/src-tauri/src/managed_agents/discovery.rs @@ -268,12 +268,12 @@ fn workspace_root_dir() -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../..") } -fn command_looks_like_path(command: &str) -> bool { +pub(in crate::managed_agents) fn command_looks_like_path(command: &str) -> bool { let path = Path::new(command); path.is_absolute() || path.components().count() > 1 } -fn executable_basename(command: &str) -> String { +pub(in crate::managed_agents) fn executable_basename(command: &str) -> String { let suffix = std::env::consts::EXE_SUFFIX; if suffix.is_empty() || command.ends_with(suffix) { command.to_string() @@ -469,7 +469,7 @@ fn command_search_dirs() -> Vec { unique } -fn is_executable_file(path: &Path) -> bool { +pub(in crate::managed_agents) fn is_executable_file(path: &Path) -> bool { let Ok(metadata) = path.metadata() else { return false; }; @@ -629,6 +629,13 @@ fn command_basenames(command: &str) -> Vec { } fn resolve_command_uncached(command: &str) -> Option { + // Bundled ACP bridge tools (see `managed_agents::acp_tools`) win over + // every other source, so the pinned bridges shipped with the app are + // preferred to user-installed copies. + if let Some(path) = super::acp_tools::command_in_bundled_dir(command) { + return Some(path); + } + if let Some(path) = resolve_workspace_command(command) { return Some(path); } diff --git a/desktop/src-tauri/src/managed_agents/mod.rs b/desktop/src-tauri/src/managed_agents/mod.rs index d99d38f0f..e5ed4a4b1 100644 --- a/desktop/src-tauri/src/managed_agents/mod.rs +++ b/desktop/src-tauri/src/managed_agents/mod.rs @@ -1,3 +1,4 @@ +pub(crate) mod acp_tools; mod agent_env; pub(crate) mod agent_events; pub(crate) mod agent_snapshot; diff --git a/desktop/src-tauri/src/managed_agents/readiness/cli_probe.rs b/desktop/src-tauri/src/managed_agents/readiness/cli_probe.rs index 8e2f866b7..703f0d00f 100644 --- a/desktop/src-tauri/src/managed_agents/readiness/cli_probe.rs +++ b/desktop/src-tauri/src/managed_agents/readiness/cli_probe.rs @@ -2,7 +2,8 @@ use std::path::Path; use crate::managed_agents::runtime::build_augmented_path; -/// Build the augmented PATH for CLI probes, including nvm's default Node.js +/// Build the augmented PATH for CLI probes, including the bundled ACP bridge +/// tools dir (pinned bridges shipped with the app) and nvm's default Node.js /// bin directory so `#!/usr/bin/env node` shims (e.g. codex-acp) resolve. pub(crate) fn augmented_path() -> Option { let home = dirs::home_dir(); @@ -10,6 +11,7 @@ pub(crate) fn augmented_path() -> Option { .as_deref() .and_then(crate::managed_agents::find_nvm_default_bin); build_augmented_path( + crate::managed_agents::acp_tools::bundled_acp_tools_dir(), home, std::env::current_exe() .ok() diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 3745fe571..a61987392 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -1542,6 +1542,7 @@ pub fn spawn_agent_child( }; // Augment PATH for DMG launches so child processes can find: + // - bundled ACP bridge tools (pinned versions shipped with the app) // - bundled CLI via ~/.local/bin symlink // - nvm-managed node/npm (nvm initializes only in interactive shells) // - bundled sidecars (buzz, buzz-acp, etc.) via exe parent (Contents/MacOS/) @@ -1550,6 +1551,7 @@ pub fn spawn_agent_child( .as_deref() .and_then(super::find_nvm_default_bin); let augmented_path = build_augmented_path( + super::acp_tools::bundled_acp_tools_dir(), dirs::home_dir(), std::env::current_exe() .ok() diff --git a/desktop/src-tauri/src/managed_agents/runtime/path.rs b/desktop/src-tauri/src/managed_agents/runtime/path.rs index af2d2e96d..4db6e62db 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/path.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/path.rs @@ -5,10 +5,12 @@ use std::path::PathBuf; /// Assemble the augmented `PATH` for a launched managed-agent child process. /// /// Concatenates, in priority order: -/// 1. `/.local/bin` — bundled CLI symlink -/// 2. `nvm_bin` — nvm's default Node.js bin dir (if the user uses nvm) -/// 3. exe parent dir — DMG sidecars under `Contents/MacOS/` -/// 4. user's login-shell `PATH` — runtimes like node/python from other managers +/// 1. `bundled_acp_bin` — bundled ACP bridge tools dir, so pinned bridges +/// shipped with the app win over user-installed copies +/// 2. `/.local/bin` — bundled CLI symlink +/// 3. `nvm_bin` — nvm's default Node.js bin dir (if the user uses nvm) +/// 4. exe parent dir — DMG sidecars under `Contents/MacOS/` +/// 5. user's login-shell `PATH` — runtimes like node/python from other managers /// /// `shell_path` is the raw colon-delimited string from a login shell, so it is /// split into individual entries before joining. Pushing it as a single segment @@ -17,12 +19,16 @@ use std::path::PathBuf; /// guards against, which left managed agents unable to find `buzz`. Returns /// `None` only when no entries exist. pub(in crate::managed_agents) fn build_augmented_path( + bundled_acp_bin: Option, home: Option, exe_parent: Option, shell_path: Option, nvm_bin: Option, ) -> Option { let mut parts: Vec = Vec::new(); + if let Some(bundled) = bundled_acp_bin { + parts.push(bundled); + } if let Some(home) = home { parts.push(home.join(".local").join("bin")); } @@ -35,11 +41,37 @@ pub(in crate::managed_agents) fn build_augmented_path( if let Some(shell_path) = shell_path { parts.extend(std::env::split_paths(&shell_path)); } - if parts.is_empty() { + join_paths_best_effort(parts) +} + +/// Join PATH entries, degrading to a best-effort join when a single entry +/// embeds the platform separator (legal in macOS paths): `join_paths` rejects +/// the whole list for one such entry, which would collapse the entire +/// augmented `PATH` to `None` and hand child processes a bare GUI PATH. Drop +/// the un-joinable entries (logging each) instead of erasing every search +/// path. Returns `None` only when no joinable entries exist. +fn join_paths_best_effort(mut paths: Vec) -> Option { + if paths.is_empty() { return None; } // join_paths uses the platform separator (':' on Unix, ';' on Windows). - std::env::join_paths(parts) + if let Ok(joined) = std::env::join_paths(&paths) { + return Some(joined.to_string_lossy().into_owned()); + } + paths.retain(|path| { + let joinable = std::env::join_paths(std::iter::once(path)).is_ok(); + if !joinable { + eprintln!( + "buzz-desktop: dropping un-joinable PATH entry: {}", + path.display() + ); + } + joinable + }); + if paths.is_empty() { + return None; + } + std::env::join_paths(paths) .ok() .map(|s| s.to_string_lossy().into_owned()) } @@ -57,6 +89,7 @@ mod tests { // it and the whole augmented PATH collapses to None (managed agents then // lose `buzz`). let result = build_augmented_path( + None, Some(PathBuf::from("/home/agent")), Some(PathBuf::from("/Applications/Buzz.app/Contents/MacOS")), Some("/usr/local/bin:/opt/homebrew/bin:/usr/bin:/bin".to_string()), @@ -73,20 +106,46 @@ mod tests { #[test] fn none_when_no_inputs() { - assert_eq!(build_augmented_path(None, None, None, None), None); + assert_eq!(build_augmented_path(None, None, None, None, None), None); } #[cfg(unix)] #[test] fn shell_path_only() { - let result = build_augmented_path(None, None, Some("/usr/bin:/bin".to_string()), None); + let result = + build_augmented_path(None, None, None, Some("/usr/bin:/bin".to_string()), None); assert_eq!(result.as_deref(), Some("/usr/bin:/bin")); } + #[cfg(unix)] + #[test] + fn bundled_acp_bin_is_highest_priority_segment() { + let result = build_augmented_path( + Some(PathBuf::from( + "/Applications/Buzz.app/Contents/Resources/resources/acp/bin", + )), + Some(PathBuf::from("/home/user")), + Some(PathBuf::from("/Applications/Buzz.app/Contents/MacOS")), + Some("/usr/bin:/bin".to_string()), + Some(PathBuf::from("/home/user/.nvm/versions/node/v20.0.0/bin")), + ); + assert_eq!( + result.as_deref(), + Some( + "/Applications/Buzz.app/Contents/Resources/resources/acp/bin:\ +/home/user/.local/bin:\ +/home/user/.nvm/versions/node/v20.0.0/bin:\ +/Applications/Buzz.app/Contents/MacOS:\ +/usr/bin:/bin" + ), + ); + } + #[cfg(unix)] #[test] fn nvm_bin_inserted_after_local_bin_before_exe_parent() { let result = build_augmented_path( + None, Some(PathBuf::from("/home/user")), Some(PathBuf::from("/Applications/Buzz.app/Contents/MacOS")), Some("/usr/bin:/bin".to_string()), @@ -107,6 +166,7 @@ mod tests { #[test] fn nvm_bin_none_does_not_add_segment() { let result = build_augmented_path( + None, Some(PathBuf::from("/home/user")), Some(PathBuf::from("/usr/local/bin")), None, @@ -117,4 +177,38 @@ mod tests { Some("/home/user/.local/bin:/usr/local/bin"), ); } + + #[cfg(unix)] + #[test] + fn unjoinable_entry_is_dropped_instead_of_emptying_path() { + // A dir embedding the separator (legal in macOS paths) can't be joined + // into PATH; it must be dropped, not collapse the whole augmented PATH + // to None. + let result = build_augmented_path( + Some(PathBuf::from("/weird:dir/bin")), + None, + None, + Some("/shell/bin:/user/bin".to_string()), + None, + ); + let path = result.expect("PATH should survive an un-joinable entry"); + let paths: Vec<_> = std::env::split_paths(&path).collect(); + assert_eq!( + paths, + vec![PathBuf::from("/shell/bin"), PathBuf::from("/user/bin")] + ); + } + + #[cfg(unix)] + #[test] + fn none_when_all_entries_unjoinable() { + let result = build_augmented_path( + Some(PathBuf::from("/weird:dir/bin")), + None, + None, + None, + None, + ); + assert_eq!(result, None); + } }