diff --git a/crates/acp-client/README.md b/crates/acp-client/README.md index 900b41fb2..8768c6c60 100644 --- a/crates/acp-client/README.md +++ b/crates/acp-client/README.md @@ -99,8 +99,7 @@ if let Some(agent) = find_acp_agent_by_id("goose") { ## How It Works 1. **Discovery**: Searches for agent executables using: - - Login shell `which` command (to get user's PATH) - - Direct command execution + - Login shell path lookup (to get user's PATH) - Common installation paths (`/opt/homebrew/bin`, `/usr/local/bin`, etc.) 2. **Execution**: diff --git a/crates/acp-client/src/types.rs b/crates/acp-client/src/types.rs index 263b36320..fdcf5f2b4 100644 --- a/crates/acp-client/src/types.rs +++ b/crates/acp-client/src/types.rs @@ -171,20 +171,16 @@ const COMMON_PATHS: &[&str] = &[ /// Find a CLI binary by command name. /// /// Searches in order: -/// 1. Login shell `which` (picks up user's PATH from `.zshrc` / `.bashrc`) +/// 1. Login shell path lookup (picks up user's PATH from `.zshrc` / `.bashrc`) /// 2. Common install locations pub fn find_command(cmd: &str) -> Option { - // Strategy 1: Login shell `which` if let Some(path) = find_via_login_shell(cmd) { - if path.exists() { - return Some(path); - } + return Some(path); } - // Strategy 2: Common paths for dir in COMMON_PATHS { let path = PathBuf::from(dir).join(cmd); - if path.exists() { + if is_executable_file(&path) { return Some(path); } } @@ -193,27 +189,71 @@ pub fn find_command(cmd: &str) -> Option { } fn find_via_login_shell(cmd: &str) -> Option { - let which_cmd = format!("which {cmd}"); + let quoted = shell_quote(cmd); + let lookups = [ + ("/bin/zsh", format!("whence -p -- {quoted}")), + ("/bin/bash", format!("type -P -- {quoted}")), + ]; - for shell in &["/bin/zsh", "/bin/bash"] { - if let Ok(output) = std::process::Command::new(shell) - .args(["-l", "-c", &which_cmd]) + for (shell, lookup_cmd) in lookups { + let Ok(output) = std::process::Command::new(shell) + .args(["-l", "-c", &lookup_cmd]) .output() - { - if output.status.success() { - let stdout = String::from_utf8_lossy(&output.stdout); - if let Some(path_str) = stdout.lines().rfind(|l| !l.is_empty()) { - let path_str = path_str.trim(); - if !path_str.is_empty() && path_str.starts_with('/') { - return Some(PathBuf::from(path_str)); - } - } + else { + continue; + }; + if !output.status.success() { + continue; + } + + let stdout = String::from_utf8_lossy(&output.stdout); + if let Some(path) = candidate_from_shell_output(stdout.as_ref()) { + if is_executable_file(&path) { + return Some(path); } } } None } +fn shell_quote(value: &str) -> String { + format!("'{}'", value.replace('\'', "'\\''")) +} + +fn candidate_from_shell_output(output: &str) -> Option { + let mut lines = output + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()); + let candidate = lines.next()?; + if lines.next().is_some() { + return None; + } + + let path = PathBuf::from(candidate); + path.is_absolute().then_some(path) +} + +fn is_executable_file(path: &Path) -> bool { + let Ok(metadata) = path.metadata() else { + return false; + }; + if !metadata.is_file() { + return false; + } + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + metadata.permissions().mode() & 0o111 != 0 + } + + #[cfg(not(unix))] + { + true + } +} + /// Map an agent ID to the `--command` value for `blox acp`. /// /// Returns `None` if the agent uses the workspace default (no flag needed). diff --git a/crates/blox-cli/src/lib.rs b/crates/blox-cli/src/lib.rs index 1ba1d7e2d..28c781ff8 100644 --- a/crates/blox-cli/src/lib.rs +++ b/crates/blox-cli/src/lib.rs @@ -4,7 +4,7 @@ use serde::{Deserialize, Deserializer, Serialize}; use std::io::Read; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; use std::time::{Duration, Instant}; use thiserror::Error; @@ -118,18 +118,16 @@ where /// Find a CLI binary by command name. /// /// Searches in order: -/// 1. Login shell `which` (picks up user's PATH from shell rc files) +/// 1. Login shell path lookup (picks up user's PATH from shell rc files) /// 2. Common install locations pub fn find_command(cmd: &str) -> Option { if let Some(path) = find_via_login_shell(cmd) { - if path.exists() { - return Some(path); - } + return Some(path); } for dir in COMMON_PATHS { let path = PathBuf::from(dir).join(cmd); - if path.exists() { + if is_executable_file(&path) { return Some(path); } } @@ -138,18 +136,24 @@ pub fn find_command(cmd: &str) -> Option { } fn find_via_login_shell(cmd: &str) -> Option { - let which_cmd = format!("which {cmd}"); - - for shell in ["/bin/zsh", "/bin/bash"] { - if let Ok(output) = Command::new(shell).args(["-l", "-c", &which_cmd]).output() { - if output.status.success() { - let stdout = String::from_utf8_lossy(&output.stdout); - if let Some(path_str) = stdout.lines().rfind(|line| !line.is_empty()) { - let path_str = path_str.trim(); - if !path_str.is_empty() && path_str.starts_with('/') { - return Some(PathBuf::from(path_str)); - } - } + let quoted = shell_quote(cmd); + let lookups = [ + ("/bin/zsh", format!("whence -p -- {quoted}")), + ("/bin/bash", format!("type -P -- {quoted}")), + ]; + + for (shell, lookup_cmd) in lookups { + let Ok(output) = Command::new(shell).args(["-l", "-c", &lookup_cmd]).output() else { + continue; + }; + if !output.status.success() { + continue; + } + + let stdout = String::from_utf8_lossy(&output.stdout); + if let Some(path) = candidate_from_shell_output(stdout.as_ref()) { + if is_executable_file(&path) { + return Some(path); } } } @@ -157,6 +161,44 @@ fn find_via_login_shell(cmd: &str) -> Option { None } +fn shell_quote(value: &str) -> String { + format!("'{}'", value.replace('\'', "'\\''")) +} + +fn candidate_from_shell_output(output: &str) -> Option { + let mut lines = output + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()); + let candidate = lines.next()?; + if lines.next().is_some() { + return None; + } + + let path = PathBuf::from(candidate); + path.is_absolute().then_some(path) +} + +fn is_executable_file(path: &Path) -> bool { + let Ok(metadata) = path.metadata() else { + return false; + }; + if !metadata.is_file() { + return false; + } + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + metadata.permissions().mode() & 0o111 != 0 + } + + #[cfg(not(unix))] + { + true + } +} + /// Locate the `sq` binary. pub fn find_sq_binary() -> Option { find_command("sq") diff --git a/crates/doctor/src/resolve.rs b/crates/doctor/src/resolve.rs index cc495e0a6..9fc866d30 100644 --- a/crates/doctor/src/resolve.rs +++ b/crates/doctor/src/resolve.rs @@ -1,34 +1,55 @@ //! Binary resolution and command output formatting helpers. -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::Command; use super::types::ResolvedBinary; -/// Resolve a binary by trying login shell `which` then common install paths. +/// Resolve a binary by trying login shell path lookup then common install paths. pub fn resolve_binary(cmd: &str) -> ResolvedBinary { let mut lines = vec![format!("resolve '{cmd}':")]; - // Strategy 1: Login shell `which` (primary) - lines.push(" strategy 1 — login shell `which`:".to_string()); - for shell in &["/bin/zsh", "/bin/bash"] { - let which_cmd = format!("which {cmd}"); - match Command::new(shell).args(["-l", "-c", &which_cmd]).output() { + // Strategy 1: Login shell path lookup (primary) + lines.push(" strategy 1 — login shell path lookup:".to_string()); + for (shell, lookup_cmd) in shell_lookup_commands(cmd) { + match Command::new(shell).args(["-l", "-c", &lookup_cmd]).output() { Ok(output) => { - let result = String::from_utf8_lossy(&output.stdout).trim().to_string(); - if output.status.success() && !result.is_empty() { - lines.push(format!( - " {shell} -l -c 'which {cmd}' => {result} (resolved)" - )); - return ResolvedBinary { - path: Some(PathBuf::from(&result)), - search_output: lines.join("\n"), - }; + let stdout = String::from_utf8_lossy(&output.stdout); + if !output.status.success() { + lines.push(format!(" {shell} -l -c '{lookup_cmd}' => not found")); + continue; + } + + match candidate_from_shell_output(stdout.as_ref()) { + Some(path) if is_executable_file(&path) => { + lines.push(format!( + " {shell} -l -c '{lookup_cmd}' => {} (resolved)", + path.display() + )); + return ResolvedBinary { + path: Some(path), + search_output: lines.join("\n"), + }; + } + Some(path) => { + lines.push(format!( + " {shell} -l -c '{lookup_cmd}' => {} (ignored: not an executable file)", + path.display() + )); + } + None if stdout.trim().is_empty() => { + lines.push(format!(" {shell} -l -c '{lookup_cmd}' => not found")); + } + None => { + lines.push(format!( + " {shell} -l -c '{lookup_cmd}' => {} (ignored: not an absolute path)", + summarize_output(stdout.as_ref()) + )); + } } - lines.push(format!(" {shell} -l -c 'which {cmd}' => not found")); } Err(e) => { - lines.push(format!(" {shell} -l -c 'which {cmd}' => error: {e}")); + lines.push(format!(" {shell} -l -c '{lookup_cmd}' => error: {e}")); } } } @@ -42,7 +63,7 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { "/home/linuxbrew/.linuxbrew/bin", ] { let path = PathBuf::from(dir).join(cmd); - if path.exists() { + if is_executable_file(&path) { lines.push(format!(" {} => found (resolved)", path.display())); return ResolvedBinary { path: Some(path), @@ -59,6 +80,62 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { } } +fn shell_lookup_commands(cmd: &str) -> [(&'static str, String); 2] { + let quoted = shell_quote(cmd); + [ + ("/bin/zsh", format!("whence -p -- {quoted}")), + ("/bin/bash", format!("type -P -- {quoted}")), + ] +} + +fn shell_quote(value: &str) -> String { + format!("'{}'", value.replace('\'', "'\\''")) +} + +fn candidate_from_shell_output(output: &str) -> Option { + let mut lines = output + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()); + let candidate = lines.next()?; + if lines.next().is_some() { + return None; + } + + let path = PathBuf::from(candidate); + path.is_absolute().then_some(path) +} + +fn is_executable_file(path: &Path) -> bool { + let Ok(metadata) = path.metadata() else { + return false; + }; + if !metadata.is_file() { + return false; + } + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + metadata.permissions().mode() & 0o111 != 0 + } + + #[cfg(not(unix))] + { + true + } +} + +fn summarize_output(output: &str) -> String { + let trimmed = output.trim(); + const MAX_LEN: usize = 120; + if trimmed.len() <= MAX_LEN { + return trimmed.replace('\n', "\\n"); + } + let summary: String = trimmed.chars().take(MAX_LEN).collect(); + format!("{}...", summary.replace('\n', "\\n")) +} + /// Format the raw output of a command invocation for debug diagnostics. pub fn format_command_output(cmd_desc: &str, output: &std::process::Output) -> String { let stdout = String::from_utf8_lossy(&output.stdout); @@ -72,3 +149,63 @@ pub fn format_command_output(cmd_desc: &str, output: &std::process::Output) -> S } raw } + +#[cfg(test)] +mod tests { + use super::{candidate_from_shell_output, is_executable_file, shell_quote}; + use std::fs::{self, File}; + use std::path::PathBuf; + + #[test] + fn candidate_accepts_single_absolute_path() { + assert_eq!( + candidate_from_shell_output("/opt/homebrew/bin/git\n"), + Some(PathBuf::from("/opt/homebrew/bin/git")) + ); + } + + #[test] + fn candidate_rejects_function_body_output() { + let output = "git () {\n\tcommand git \"$@\"\n}\n"; + assert_eq!(candidate_from_shell_output(output), None); + } + + #[test] + fn candidate_rejects_relative_or_command_name_output() { + assert_eq!(candidate_from_shell_output("git\n"), None); + } + + #[test] + fn shell_quote_handles_single_quotes() { + assert_eq!(shell_quote("git'bad"), "'git'\\''bad'"); + } + + #[test] + fn executable_file_validation_checks_file_and_mode() { + let dir = std::env::temp_dir().join(format!("doctor-resolve-test-{}", std::process::id())); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).unwrap(); + + let executable = dir.join("tool"); + File::create(&executable).unwrap(); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + + fs::set_permissions(&executable, fs::Permissions::from_mode(0o644)).unwrap(); + assert!(!is_executable_file(&executable)); + + fs::set_permissions(&executable, fs::Permissions::from_mode(0o755)).unwrap(); + assert!(is_executable_file(&executable)); + } + + #[cfg(not(unix))] + { + assert!(is_executable_file(&executable)); + } + + assert!(!is_executable_file(&dir)); + let _ = fs::remove_dir_all(&dir); + } +}