diff --git a/docs/adding-executors.md b/docs/adding-executors.md index a893fbd..e9d9dc2 100644 --- a/docs/adding-executors.md +++ b/docs/adding-executors.md @@ -145,10 +145,10 @@ pub trait AiCliExecutor: Send + Sync { /// Returns the CLI command name used by this executor. fn command(&self) -> &'static str; - /// Checks if this executor's CLI tool is available in PATH. - /// Default implementation uses `which `. - async fn is_available(&self) -> bool { - check_cli_available(self.command()).await.unwrap_or(false) + /// Checks if this executor's CLI tool is available. + /// Default implementation uses the shell-aware resolution strategy. + fn is_available(&self) -> bool { + check_cli_available(self.command()) } } ``` @@ -244,3 +244,132 @@ pub struct ClaudeExecutor; - Use `&'static str` for `name()` and `command()` to avoid allocations - The `run_cli_with_output()` helper handles process spawning, output streaming, and cleanup - On Linux, child processes are automatically killed when the parent dies (via `PR_SET_PDEATHSIG`) + +## CLI Availability Resolution Strategy + +This section documents the cross-shell strategy for resolving CLI commands (codex, claude, gemini) that accounts for PATH, shell aliases/functions, and executability without introducing shell injection risk. + +### Problem Statement + +The basic `which`/`where` approach only searches PATH and misses commands that are available via: +- Shell aliases (e.g., `alias claude='/usr/local/bin/claude-cli'`) +- Shell functions (e.g., `claude() { /path/to/claude "$@"; }`) +- PATH modifications in shell profiles (e.g., `~/.zshrc`, `~/.bashrc`) + +This is particularly common on macOS where tools installed via Homebrew or npm may only be accessible after shell profile initialization. + +### Resolution Algorithm + +The CLI resolution follows this priority order: + +1. **Direct PATH lookup** (fast path): Check if the command exists directly in PATH using `which`/`where`. If found and executable, use it. + +2. **Shell-based resolution** (fallback): If not found in PATH, invoke the user's shell to resolve the command: + ``` + $SHELL -l -i -c "command -v " + ``` + - `-l`: Login shell (loads `~/.profile`, `~/.bash_profile`, `~/.zprofile`) + - `-i`: Interactive shell (loads `~/.bashrc`, `~/.zshrc`) + - `command -v`: POSIX-compliant way to resolve commands (preferred over `type` or `which`) + +3. **Output classification**: Parse the output of `command -v` to determine the command type: + - **Path** (e.g., `/usr/local/bin/claude`): Direct executable in PATH + - **Alias** (e.g., `alias claude='...'`): Shell alias definition + - **Function** (e.g., `claude is a function`): Shell function + - **Builtin** (e.g., `builtin`): Shell builtin command + - **Not found** (empty output or error): Command unavailable + +### Shell Invocation Behavior by Shell Type + +| Shell | Login Profile | Interactive Profile | `command -v` Support | +|-------|---------------|---------------------|----------------------| +| bash | `~/.bash_profile` or `~/.profile` | `~/.bashrc` | Yes (POSIX) | +| zsh | `~/.zprofile` | `~/.zshrc` | Yes (POSIX) | +| sh | `~/.profile` | - | Yes (POSIX) | +| fish | `~/.config/fish/config.fish` | Same | Yes (via `-v` alias) | + +**Note**: Fish shell supports `command -v` as an alias for `command --search`, which prints the path to the external command. The implementation uses `command -v` universally across all shells. + +### Executability Verification + +After resolving the command path, verify executability: + +1. **For PATH executables**: Check that the resolved path: + - Exists as a regular file (not directory or symlink to directory) + - Has execute permission for the current user + +2. **For aliases/functions**: The command is considered available but cannot be verified for executability until actual invocation. Mark as "available via shell". + +3. **For builtins**: Shell builtins are always executable within their shell context. + +### Classification Rules + +Commands are classified into these categories for UI display and execution strategy: + +| Classification | Resolution Output | Execution Method | UI Indicator | +|---------------|-------------------|------------------|--------------| +| `PathExecutable` | Absolute path | `Command::new(path)` | ✓ Available | +| `ShellAlias` | `alias name='...'` | Shell invocation required | ⚡ Available (alias) | +| `ShellFunction` | Function definition | Shell invocation required | ⚡ Available (function) | +| `ShellBuiltin` | `builtin` | Shell invocation required | ⚡ Available (builtin) | +| `NotFound` | Empty/error | N/A | ✗ Not found | + +### Security Considerations + +**Command Name Validation** (Critical): +- Command names MUST be validated before shell invocation +- Allow only: alphanumeric characters, hyphens (`-`), underscores (`_`) +- Reject any command containing: spaces, quotes, semicolons, pipes, redirects, backticks, `$`, etc. +- Maximum length: 64 characters + +**Safe validation regex**: `^[a-zA-Z0-9_-]{1,64}$` + +**Shell Injection Prevention**: +- Never interpolate untrusted input into shell commands +- Use the validated command name directly with `command -v` +- Do not use shell expansion or eval + +**Example safe invocation**: +```rust +// SAFE: command_name is pre-validated to match ^[a-zA-Z0-9_-]{1,64}$ +let shell = std::env::var("SHELL").unwrap_or_else(|_| "/bin/sh".to_string()); +Command::new(&shell) + .args(["-l", "-i", "-c", &format!("command -v {}", command_name)]) + .output() +``` + +### Execution Strategy + +Based on the classification, choose the execution method: + +1. **PathExecutable**: Execute directly with `Command::new(resolved_path)` + - Most reliable and fastest + - Inherits environment from McGravity process + +2. **ShellAlias/ShellFunction/ShellBuiltin**: Execute via shell wrapper + ```rust + Command::new(&shell) + .args(["-l", "-i", "-c", &format!("{} {}", command_name, escaped_args)]) + ``` + - Required for alias/function resolution + - Slower due to shell startup overhead + - Args must be properly escaped for shell + +### Alignment with Trait-Based Executor Abstraction + +Per `CLAUDE.md` "Trait-Based Executor Abstraction" and `docs/architecture.md`: + +- The `AiCliExecutor::is_available()` method should use the resolution algorithm above +- The `AiCliExecutor::command()` returns the command name (e.g., "claude") +- Execution via `execute()` should use the appropriate execution strategy based on classification +- The resolution result can be cached at startup to avoid repeated shell invocations + +### CI Pipeline Compliance + +Per `.github/workflows/ci.yaml`, the implementation must: +- Pass `cargo fmt --all -- --check` +- Pass `cargo clippy --all-targets --all-features -- -D warnings` +- Pass `cargo build --locked --all-features` +- Pass `cargo test --locked --all-features` + +The shell-based resolution should be implemented behind a feature flag or as a fallback to maintain fast CI execution where shell environment is minimal. diff --git a/src/app/render/initial_setup.rs b/src/app/render/initial_setup.rs index 2e65e2d..57ff327 100644 --- a/src/app/render/initial_setup.rs +++ b/src/app/render/initial_setup.rs @@ -135,13 +135,13 @@ impl App { /// Renders an error line for an unavailable CLI. /// /// This is used by both the initial setup modal and the settings panel - /// to display a consistent error message when a CLI tool is not found. + /// to display a consistent error message when a CLI tool is unavailable. pub(crate) fn render_unavailable_error(&self, model: Model) -> Line<'static> { let command = model.command(); Line::from(vec![ Span::raw(" "), // Indent to align with model field Span::styled( - format!("⚠ `{command}` command not found in PATH"), + format!("⚠ `{command}` is not available or not executable"), self.theme.error_style(), ), ]) diff --git a/src/app/tests/initial_setup.rs b/src/app/tests/initial_setup.rs index bf9432d..9aac780 100644 --- a/src/app/tests/initial_setup.rs +++ b/src/app/tests/initial_setup.rs @@ -1007,10 +1007,10 @@ mod render_tests { "│ │Select your default AI CLI tools. │ │", "│ │ │ │", "│ │› Planning Model [Codex] │ │", - "│ │ ⚠ `codex` command not found in PATH │ │", + "│ │ ⚠ `codex` is not available or not executable │ │", "│ │ │ │", "│ │ Execution Model [Codex] │ │", - "│ │ ⚠ `codex` command not found in PATH │ │", + "│ │ ⚠ `codex` is not available or not executable │ │", "│ │ │ │", "│ │ │ │", "│ │ │ │", diff --git a/src/core/cli_check.rs b/src/core/cli_check.rs index 5c01865..26aff7a 100644 --- a/src/core/cli_check.rs +++ b/src/core/cli_check.rs @@ -1,40 +1,346 @@ -//! Synchronous CLI availability checking utilities. +//! CLI availability checking utilities with shell-aware resolution. //! -//! This module provides synchronous functions for checking whether AI CLI tools -//! are available in the system's PATH. Unlike the async version in `executor.rs`, -//! these functions can be called during UI initialization before the async runtime -//! context is fully established. +//! This module provides functions for checking whether AI CLI tools are available +//! in the system. It implements a two-stage resolution strategy: +//! +//! 1. **Fast PATH lookup**: Direct `which`/`where` check for executables +//! 2. **Shell-based resolution**: Fallback using `$SHELL -l -i -c "command -v "` +//! to detect commands available via aliases, functions, or PATH modifications +//! in shell profiles +//! +//! # Resolution Order +//! +//! The resolution algorithm follows this priority: +//! +//! 1. Direct PATH scan via `which` (Unix) or `where` (Windows) +//! 2. If not found, shell-based resolution via the user's login shell +//! 3. Executability verification for PATH-resolved commands +//! +//! # Security +//! +//! Command names are validated against `^[a-zA-Z0-9_-]{1,64}$` before any shell +//! invocation to prevent command injection. See [`is_safe_command_name`] for details. +//! +//! # Classification +//! +//! Commands are classified into categories for appropriate execution strategy: +//! - [`CommandResolution::PathExecutable`]: Direct executable, can use `Command::new(path)` +//! - [`CommandResolution::ShellAlias`]: Requires shell wrapper for execution +//! - [`CommandResolution::ShellFunction`]: Requires shell wrapper for execution +//! - [`CommandResolution::ShellBuiltin`]: Requires shell wrapper for execution +//! - [`CommandResolution::NotFound`]: Command unavailable +//! +//! See `docs/adding-executors.md` for the complete resolution strategy documentation. +use std::path::{Path, PathBuf}; use std::process::Command; -/// Checks if a CLI command is available in the system's PATH. +/// Result of resolving a CLI command. /// -/// Uses `which` on Unix systems and `where` on Windows to check for command availability. -/// This is a synchronous function suitable for use during UI initialization. +/// Indicates how a command was found and the appropriate execution strategy. +/// See `docs/adding-executors.md` for detailed classification rules. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum CommandResolution { + /// Command found as executable in PATH. + /// + /// Can be executed directly via `Command::new(path)`. + PathExecutable(PathBuf), + /// Command available as shell alias. + /// + /// Requires shell wrapper: `$SHELL -l -i -c " "`. + ShellAlias(String), + /// Command available as shell function. + /// + /// Requires shell wrapper: `$SHELL -l -i -c " "`. + ShellFunction(String), + /// Command is a shell builtin. + /// + /// Requires shell wrapper: `$SHELL -l -i -c " "`. + ShellBuiltin, + /// Command not found by any resolution method. + NotFound, +} + +impl CommandResolution { + /// Returns `true` if the command was found by any method. + #[must_use] + pub const fn is_available(&self) -> bool { + !matches!(self, Self::NotFound) + } + + /// Returns the resolved path if this is a `PathExecutable`. + #[must_use] + pub fn path(&self) -> Option<&PathBuf> { + match self { + Self::PathExecutable(path) => Some(path), + _ => None, + } + } + + /// Returns `true` if execution requires a shell wrapper. + #[must_use] + pub const fn requires_shell(&self) -> bool { + matches!( + self, + Self::ShellAlias(_) | Self::ShellFunction(_) | Self::ShellBuiltin + ) + } +} + +/// Validates that a command name is safe for shell invocation. /// -/// # Arguments +/// This function prevents command injection by ensuring the command name +/// contains only safe characters. Per `rust.mdc` "Security Best Practices", +/// input validation is critical at system boundaries. /// -/// * `command` - The command name to check (e.g., "codex", "claude", "gemini") +/// # Rules +/// - Only alphanumeric characters, hyphens (`-`), and underscores (`_`) allowed +/// - Maximum 64 characters +/// - Must not be empty /// /// # Returns +/// `true` if the command name is safe, `false` otherwise. +/// +/// # Examples +/// ``` +/// use mcgravity::core::cli_check::is_safe_command_name; /// -/// `true` if the command is found in PATH, `false` otherwise. +/// assert!(is_safe_command_name("claude")); +/// assert!(is_safe_command_name("codex-cli")); +/// assert!(is_safe_command_name("my_tool")); +/// assert!(!is_safe_command_name("")); // empty +/// assert!(!is_safe_command_name("cmd; rm -rf")); // injection attempt +/// assert!(!is_safe_command_name("$(whoami)")); // command substitution +/// ``` #[must_use] -pub fn check_cli_in_path(command: &str) -> bool { +pub fn is_safe_command_name(command: &str) -> bool { + if command.is_empty() || command.len() > 64 { + return false; + } + + command + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') +} + +/// Resolves a CLI command using the shell-aware resolution strategy. +/// +/// This function implements the resolution algorithm documented in +/// `docs/adding-executors.md` "CLI Availability Resolution Strategy". +/// +/// # Resolution Order +/// 1. Fast PATH lookup via `which`/`where` +/// 2. Shell-based resolution via `$SHELL -l -i -c "command -v "` +/// +/// # Security +/// The command name is validated against `^[a-zA-Z0-9_-]{1,64}$` before +/// any shell invocation to prevent command injection. +/// +/// # Arguments +/// * `command` - The command name to resolve (e.g., "claude", "codex") +/// +/// # Returns +/// A [`CommandResolution`] indicating how the command can be executed. +/// +/// # Examples +/// ```no_run +/// use mcgravity::core::cli_check::resolve_cli_command; +/// +/// let resolution = resolve_cli_command("claude"); +/// if resolution.is_available() { +/// println!("Claude CLI is available"); +/// } +/// ``` +#[must_use] +pub fn resolve_cli_command(command: &str) -> CommandResolution { + // Security: validate command name first + if !is_safe_command_name(command) { + return CommandResolution::NotFound; + } + + // Step 1: Fast PATH lookup + if let Some(resolution) = try_path_lookup(command) { + return resolution; + } + + // Step 2: Shell-based resolution (Unix only) + #[cfg(unix)] + if let Some(resolution) = try_shell_resolution(command) { + return resolution; + } + + CommandResolution::NotFound +} + +/// Attempts to find a command via direct PATH lookup using `which`/`where`. +/// +/// Returns `Some(CommandResolution::PathExecutable)` if found and executable, +/// `None` otherwise. +fn try_path_lookup(command: &str) -> Option { #[cfg(windows)] let check_cmd = "where"; #[cfg(not(windows))] let check_cmd = "which"; - Command::new(check_cmd) + let output = Command::new(check_cmd) .arg(command) - .stdout(std::process::Stdio::null()) + .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::null()) - .status() - .map(|status| status.success()) + .output() + .ok()?; + + if !output.status.success() { + return None; + } + + // Parse the path from output + let path_str = String::from_utf8_lossy(&output.stdout); + let path_str = path_str.trim(); + + if path_str.is_empty() { + return None; + } + + // On Windows, `where` may return multiple paths; take the first + let first_path = path_str.lines().next()?; + let path = PathBuf::from(first_path); + + // Verify executability + if is_executable(&path) { + Some(CommandResolution::PathExecutable(path)) + } else { + None + } +} + +/// Checks if a path points to an executable file. +#[cfg(unix)] +fn is_executable(path: &Path) -> bool { + use std::os::unix::fs::PermissionsExt; + + match std::fs::metadata(path) { + Ok(metadata) => { + // Must be a regular file with execute permission + metadata.is_file() && (metadata.permissions().mode() & 0o111 != 0) + } + Err(_) => false, + } +} + +#[cfg(windows)] +fn is_executable(path: &Path) -> bool { + // On Windows, check if the file exists and has an executable extension + if !path.is_file() { + return false; + } + + // Windows executable extensions + path.extension() + .and_then(|ext| ext.to_str()) + .map(|ext| { + let ext_lower = ext.to_lowercase(); + matches!(ext_lower.as_str(), "exe" | "cmd" | "bat" | "com" | "ps1") + }) .unwrap_or(false) } +/// Attempts shell-based resolution using the user's login shell. +/// +/// Invokes `$SHELL -l -i -c "command -v "` and parses the output +/// to determine command type (path, alias, function, or builtin). +#[cfg(unix)] +fn try_shell_resolution(command: &str) -> Option { + let shell = std::env::var("SHELL").unwrap_or_else(|_| "/bin/sh".to_string()); + + // Security: command is already validated by resolve_cli_command + let check_command = format!("command -v {command}"); + + let output = Command::new(&shell) + .args(["-l", "-i", "-c", &check_command]) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::null()) + .output() + .ok()?; + + if !output.status.success() { + return None; + } + + let result = String::from_utf8_lossy(&output.stdout); + let result = result.trim(); + + if result.is_empty() { + return None; + } + + // Classify the output + Some(classify_command_v_output(result, command)) +} + +/// Classifies the output of `command -v` into a `CommandResolution`. +#[cfg(unix)] +fn classify_command_v_output(output: &str, command: &str) -> CommandResolution { + // Check for alias definition (e.g., "alias claude='...'") + if output.starts_with("alias ") { + return CommandResolution::ShellAlias(output.to_string()); + } + + // Check for function (varies by shell, but often contains "function" or "()") + if output.contains(" is a function") + || output.contains("() {") + || output.starts_with(&format!("{command} ()")) + { + return CommandResolution::ShellFunction(output.to_string()); + } + + // Check for builtin + if output == command || output == "builtin" { + // `command -v` returns just the name for builtins in some shells + // Need to distinguish from a relative path + if !output.contains('/') && !output.contains('\\') { + // Could be a builtin; check explicitly + return CommandResolution::ShellBuiltin; + } + } + + // Check if it's a path + if output.starts_with('/') || output.contains('/') { + let path = PathBuf::from(output); + if is_executable(&path) { + return CommandResolution::PathExecutable(path); + } + } + + // If we get here and have output, it's likely available but type unknown + // Treat as alias (requires shell) for safety + CommandResolution::ShellAlias(output.to_string()) +} + +/// Checks if a CLI command is available and executable. +/// +/// Uses the shell-aware resolution strategy documented in `docs/adding-executors.md`: +/// 1. Fast PATH lookup via `which`/`where` with executability verification +/// 2. Shell-based resolution via `$SHELL -l -i -c "command -v "` (Unix only) +/// +/// This is a synchronous function suitable for use during UI initialization. +/// +/// # Arguments +/// +/// * `command` - The command name to check (e.g., "codex", "claude", "gemini") +/// +/// # Returns +/// +/// `true` if the command is found and executable, `false` otherwise. +/// Returns `false` for non-executable files in PATH. +/// +/// # Security +/// +/// The command name is validated before any shell invocation. See [`is_safe_command_name`]. +#[must_use] +pub fn check_cli_in_path(command: &str) -> bool { + resolve_cli_command(command).is_available() +} + /// Stores CLI availability status for all supported AI models. /// /// This struct caches the results of CLI availability checks, allowing @@ -80,6 +386,380 @@ impl ModelAvailability { mod tests { use super::*; + // ========================================================================= + // is_safe_command_name Tests + // ========================================================================= + + mod is_safe_command_name_tests { + use super::*; + + /// Tests valid command names. + #[test] + fn accepts_valid_command_names() { + assert!(is_safe_command_name("claude")); + assert!(is_safe_command_name("codex")); + assert!(is_safe_command_name("gemini")); + assert!(is_safe_command_name("codex-cli")); + assert!(is_safe_command_name("my_tool")); + assert!(is_safe_command_name("tool123")); + assert!(is_safe_command_name("a")); // single char + } + + /// Tests that empty string is rejected. + #[test] + fn rejects_empty_string() { + assert!(!is_safe_command_name("")); + } + + /// Tests that strings over 64 chars are rejected. + #[test] + fn rejects_too_long_names() { + let long_name = "a".repeat(65); + assert!(!is_safe_command_name(&long_name)); + + // Exactly 64 chars should be accepted + let max_name = "a".repeat(64); + assert!(is_safe_command_name(&max_name)); + } + + /// Tests that shell injection attempts are rejected. + #[test] + fn rejects_injection_attempts() { + assert!(!is_safe_command_name("cmd; rm -rf")); + assert!(!is_safe_command_name("$(whoami)")); + assert!(!is_safe_command_name("`whoami`")); + assert!(!is_safe_command_name("cmd | cat")); + assert!(!is_safe_command_name("cmd > file")); + assert!(!is_safe_command_name("cmd < file")); + assert!(!is_safe_command_name("cmd && bad")); + assert!(!is_safe_command_name("cmd || bad")); + assert!(!is_safe_command_name("cmd\nwhoami")); + assert!(!is_safe_command_name("cmd'whoami")); + assert!(!is_safe_command_name("cmd\"whoami")); + } + + /// Tests that paths are rejected (slashes not allowed). + #[test] + fn rejects_paths() { + assert!(!is_safe_command_name("/usr/bin/cmd")); + assert!(!is_safe_command_name("../cmd")); + assert!(!is_safe_command_name("./cmd")); + assert!(!is_safe_command_name("dir/cmd")); + } + + /// Tests that spaces are rejected. + #[test] + fn rejects_spaces() { + assert!(!is_safe_command_name("cmd arg")); + assert!(!is_safe_command_name(" cmd")); + assert!(!is_safe_command_name("cmd ")); + } + } + + // ========================================================================= + // CommandResolution Tests + // ========================================================================= + + mod command_resolution_tests { + use super::*; + + /// Tests `is_available` method. + #[test] + fn is_available_returns_correct_value() { + assert!(CommandResolution::PathExecutable(PathBuf::from("/bin/sh")).is_available()); + assert!(CommandResolution::ShellAlias("alias x='y'".to_string()).is_available()); + assert!(CommandResolution::ShellFunction("x () {}".to_string()).is_available()); + assert!(CommandResolution::ShellBuiltin.is_available()); + assert!(!CommandResolution::NotFound.is_available()); + } + + /// Tests `path` method. + #[test] + fn path_returns_correct_value() { + let path = PathBuf::from("/bin/sh"); + assert_eq!( + CommandResolution::PathExecutable(path.clone()).path(), + Some(&path) + ); + assert_eq!(CommandResolution::ShellAlias("x".to_string()).path(), None); + assert_eq!(CommandResolution::NotFound.path(), None); + } + + /// Tests `requires_shell` method. + #[test] + fn requires_shell_returns_correct_value() { + assert!(!CommandResolution::PathExecutable(PathBuf::from("/bin/sh")).requires_shell()); + assert!(CommandResolution::ShellAlias("x".to_string()).requires_shell()); + assert!(CommandResolution::ShellFunction("x".to_string()).requires_shell()); + assert!(CommandResolution::ShellBuiltin.requires_shell()); + assert!(!CommandResolution::NotFound.requires_shell()); + } + + /// Tests Debug trait. + #[test] + fn debug_format() { + let resolution = CommandResolution::PathExecutable(PathBuf::from("/bin/sh")); + let debug_str = format!("{resolution:?}"); + assert!(debug_str.contains("PathExecutable")); + } + + /// Tests Clone trait. + #[test] + fn clone_works() { + let original = CommandResolution::PathExecutable(PathBuf::from("/bin/sh")); + let cloned = original.clone(); + assert_eq!(original, cloned); + } + + /// Tests Eq trait. + #[test] + fn equality() { + let a = CommandResolution::PathExecutable(PathBuf::from("/bin/sh")); + let b = CommandResolution::PathExecutable(PathBuf::from("/bin/sh")); + let c = CommandResolution::PathExecutable(PathBuf::from("/bin/bash")); + assert_eq!(a, b); + assert_ne!(a, c); + } + } + + // ========================================================================= + // resolve_cli_command Tests + // ========================================================================= + + mod resolve_cli_command_tests { + use super::*; + + /// Tests that an executable found via PATH scanning is reported as available. + /// + /// This test verifies that `resolve_cli_command` correctly identifies + /// executables in PATH and returns `PathExecutable` with the resolved path. + #[test] + fn executable_in_path_is_available() { + // 'sh' should exist on all Unix systems as an executable in PATH + #[cfg(not(windows))] + { + let resolution = resolve_cli_command("sh"); + assert!(resolution.is_available(), "sh should be available via PATH"); + + // Should be resolved as PathExecutable with a valid path + if let CommandResolution::PathExecutable(path) = &resolution { + assert!(path.exists(), "Resolved path should exist"); + assert!(is_executable(path), "Resolved path should be executable"); + } else { + // Could also be resolved via shell, which is acceptable + assert!( + resolution.is_available(), + "sh should be available by some method" + ); + } + } + + // 'cmd' should exist on Windows + #[cfg(windows)] + { + let resolution = resolve_cli_command("cmd"); + assert!( + resolution.is_available(), + "cmd should be available on Windows" + ); + } + } + + /// Tests that a nonexistent command returns `NotFound`. + #[test] + fn nonexistent_command_returns_not_found() { + let resolution = resolve_cli_command("this_command_definitely_does_not_exist_xyz789"); + assert_eq!(resolution, CommandResolution::NotFound); + assert!(!resolution.is_available()); + } + + /// Tests that invalid command names return `NotFound`. + #[test] + fn invalid_command_names_return_not_found() { + // Empty + assert_eq!(resolve_cli_command(""), CommandResolution::NotFound); + + // With path separators + assert_eq!( + resolve_cli_command("not/a/valid/command"), + CommandResolution::NotFound + ); + + // Injection attempt + assert_eq!( + resolve_cli_command("cmd; rm -rf"), + CommandResolution::NotFound + ); + } + + /// Tests that `resolve_cli_command` validates command names before shell invocation. + #[test] + fn validates_command_before_resolution() { + // These should all return NotFound due to validation failure, + // not due to actual command resolution + let dangerous_inputs = ["$(whoami)", "`id`", "x; y", "x | y", "x && y", "x || y"]; + + for input in dangerous_inputs { + let resolution = resolve_cli_command(input); + assert_eq!( + resolution, + CommandResolution::NotFound, + "Dangerous input '{input}' should return NotFound" + ); + } + } + } + + // ========================================================================= + // is_executable Tests (Unix-specific) + // ========================================================================= + + #[cfg(unix)] + #[allow(clippy::expect_used)] // Test code uses expect for clear panic messages + mod is_executable_tests { + use super::*; + use std::fs::{self, File}; + use std::os::unix::fs::PermissionsExt; + + /// Tests that a non-executable file in PATH returns false. + /// + /// This test creates a temporary file without execute permissions + /// and verifies that `is_executable` correctly returns false. + #[test] + fn non_executable_file_returns_false() { + // Create a temp file without execute permission + let temp_dir = std::env::temp_dir(); + let test_file = temp_dir.join("mcgravity_test_non_exec_file"); + + // Create the file + File::create(&test_file).expect("Failed to create test file"); + + // Ensure no execute permissions (read-only) + let metadata = fs::metadata(&test_file).expect("Failed to get metadata"); + let mut perms = metadata.permissions(); + perms.set_mode(0o644); // rw-r--r-- + fs::set_permissions(&test_file, perms).expect("Failed to set permissions"); + + // Verify it's not executable + assert!( + !is_executable(&test_file), + "Non-executable file should return false" + ); + + // Clean up + let _ = fs::remove_file(&test_file); + } + + /// Tests that a file with execute permission returns true. + #[test] + fn executable_file_returns_true() { + // Create a temp file with execute permission + let temp_dir = std::env::temp_dir(); + let test_file = temp_dir.join("mcgravity_test_exec_file"); + + // Create the file + File::create(&test_file).expect("Failed to create test file"); + + // Set execute permissions + let metadata = fs::metadata(&test_file).expect("Failed to get metadata"); + let mut perms = metadata.permissions(); + perms.set_mode(0o755); // rwxr-xr-x + fs::set_permissions(&test_file, perms).expect("Failed to set permissions"); + + // Verify it's executable + assert!( + is_executable(&test_file), + "Executable file should return true" + ); + + // Clean up + let _ = fs::remove_file(&test_file); + } + + /// Tests that a directory returns false (not executable as a command). + #[test] + fn directory_returns_false() { + let temp_dir = std::env::temp_dir(); + assert!(!is_executable(&temp_dir), "Directory should return false"); + } + + /// Tests that a nonexistent path returns false. + #[test] + fn nonexistent_path_returns_false() { + let fake_path = PathBuf::from("/this/path/does/not/exist/at/all"); + assert!( + !is_executable(&fake_path), + "Nonexistent path should return false" + ); + } + } + + // ========================================================================= + // Shell Resolution Tests (Unix-specific) + // ========================================================================= + + #[cfg(unix)] + mod shell_resolution_tests { + use super::*; + + /// Tests that shell-resolved commands succeed when the shell is available. + /// + /// This test verifies that the shell resolution fallback works correctly + /// by resolving a known command that should be available via the shell. + #[test] + fn shell_resolves_known_command() { + // First check if we have a working shell + let shell = std::env::var("SHELL").unwrap_or_else(|_| "/bin/sh".to_string()); + let shell_exists = PathBuf::from(&shell).exists(); + + if !shell_exists { + // Skip test if no shell available (unlikely in practice) + eprintln!("Skipping shell resolution test: no shell available"); + return; + } + + // `echo` is a builtin in most shells, should be resolvable + // Note: We test with 'ls' which is typically a PATH executable + // to ensure the shell can resolve commands + let resolution = resolve_cli_command("ls"); + assert!( + resolution.is_available(), + "ls should be resolvable via shell or PATH" + ); + } + + /// Tests classification of alias output. + #[test] + fn classifies_alias_output() { + let output = "alias claude='/usr/local/bin/claude'"; + let result = classify_command_v_output(output, "claude"); + assert!(matches!(result, CommandResolution::ShellAlias(_))); + } + + /// Tests classification of function output. + #[test] + fn classifies_function_output() { + let output = "claude is a function"; + let result = classify_command_v_output(output, "claude"); + assert!(matches!(result, CommandResolution::ShellFunction(_))); + + let output2 = "claude () {\n /usr/local/bin/claude \"$@\"\n}"; + let result2 = classify_command_v_output(output2, "claude"); + assert!(matches!(result2, CommandResolution::ShellFunction(_))); + } + + /// Tests classification of path output. + #[test] + fn classifies_path_output() { + // Use /bin/sh which should exist and be executable + let output = "/bin/sh"; + let result = classify_command_v_output(output, "sh"); + if PathBuf::from(output).exists() && is_executable(&PathBuf::from(output)) { + assert!(matches!(result, CommandResolution::PathExecutable(_))); + } + } + } + // ========================================================================= // check_cli_in_path Tests // ========================================================================= diff --git a/src/core/executor.rs b/src/core/executor.rs index 6a97e8d..aa93ccd 100644 --- a/src/core/executor.rs +++ b/src/core/executor.rs @@ -2,6 +2,17 @@ //! //! Provides a trait-based abstraction for executing different AI CLI tools //! (Codex, Claude, Gemini, etc.) with a unified interface. +//! +//! # Command Resolution +//! +//! This module uses the shell-aware resolution strategy from [`crate::core::cli_check`]: +//! 1. Fast PATH lookup via `which`/`where` with executability verification +//! 2. Shell-based resolution via `$SHELL -l -i -c "command -v "` (Unix only) +//! +//! This ensures that commands installed via macOS aliases, shell functions, or +//! PATH modifications in shell profiles are properly detected and executed. +//! +//! See `docs/adding-executors.md` for the complete resolution strategy documentation. use anyhow::{Context, Result}; use async_trait::async_trait; @@ -123,9 +134,12 @@ pub trait AiCliExecutor: Send + Sync { /// Returns the CLI command name used by this executor. fn command(&self) -> &'static str; - /// Checks if this executor's CLI tool is available in PATH. - async fn is_available(&self) -> bool { - check_cli_available(self.command()).await.unwrap_or(false) + /// Checks if this executor's CLI tool is available. + /// + /// Uses the shell-aware resolution strategy to detect commands available + /// via PATH, shell aliases, functions, or shell profile modifications. + fn is_available(&self) -> bool { + check_cli_available(self.command()) } } @@ -242,10 +256,69 @@ struct SpawnedProcess { /// Spawns a CLI process with stdout and stderr captured. /// +/// Uses the shell-aware resolution strategy to find the command: +/// 1. Resolves the command using [`crate::core::cli_check::resolve_cli_command`] +/// 2. For `PathExecutable`: spawns directly using the resolved path +/// 3. For shell-resolved commands (alias/function/builtin): spawns via shell wrapper +/// 4. For `NotFound`: returns a contextual error +/// /// On Linux, configures the child to be killed when the parent dies via `PR_SET_PDEATHSIG`. +/// +/// # Arguments +/// +/// * `command` - The command name to execute (e.g., "claude", "codex") +/// * `args` - Arguments to pass to the command +/// +/// # Errors +/// +/// Returns an error if the command cannot be resolved or if spawning fails. fn spawn_cli_process(command: &str, args: &[&str]) -> Result { - let mut cmd = Command::new(command); - cmd.args(args).stdout(Stdio::piped()).stderr(Stdio::piped()); + use crate::core::cli_check::{CommandResolution, resolve_cli_command}; + + let resolution = resolve_cli_command(command); + + let mut cmd = match &resolution { + CommandResolution::PathExecutable(path) => { + // Direct execution with resolved path + let mut c = Command::new(path); + c.args(args); + c + } + CommandResolution::ShellAlias(_) + | CommandResolution::ShellFunction(_) + | CommandResolution::ShellBuiltin => { + // Shell wrapper required + #[cfg(unix)] + { + let shell = std::env::var("SHELL").unwrap_or_else(|_| "/bin/sh".to_string()); + + // Build the command string with properly escaped args + let escaped_args: Vec = + args.iter().map(|arg| shell_escape_arg(arg)).collect(); + let full_command = format!("{} {}", command, escaped_args.join(" ")); + + let mut c = Command::new(&shell); + c.args(["-l", "-i", "-c", &full_command]); + c + } + #[cfg(windows)] + { + // On Windows, shell-resolved commands are less common + // Fall back to direct execution attempt + let mut c = Command::new(command); + c.args(args); + c + } + } + CommandResolution::NotFound => { + anyhow::bail!( + "CLI command '{command}' not found. Ensure it is installed and available in PATH, \ + or via shell alias/function. Run `which {command}` or check your shell profile." + ); + } + }; + + cmd.stdout(Stdio::piped()).stderr(Stdio::piped()); // On Linux, set up the child to be killed when the parent dies. // This ensures cleanup even if the parent is killed with SIGKILL. @@ -274,6 +347,23 @@ fn spawn_cli_process(command: &str, args: &[&str]) -> Result { }) } +/// Escapes a shell argument for safe inclusion in a shell command string. +/// +/// Uses single quotes for most cases, with proper handling of embedded single quotes. +#[cfg(unix)] +fn shell_escape_arg(arg: &str) -> String { + // If the arg contains no special characters, return as-is + if arg + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_' || c == '.' || c == '/') + { + return arg.to_string(); + } + + // Use single quotes, escaping embedded single quotes as '\'' + format!("'{}'", arg.replace('\'', "'\\''")) +} + /// Runs a CLI process with custom stdout processing. /// /// This is the core execution function that handles: @@ -396,23 +486,25 @@ async fn run_claude_cli_with_output( .await } -/// Checks if a CLI tool is available in PATH. +/// Checks if a CLI tool is available using the shell-aware resolution strategy. /// -/// # Errors +/// This function delegates to [`crate::core::cli_check::resolve_cli_command`] +/// which implements the resolution algorithm documented in `docs/adding-executors.md`: +/// 1. Fast PATH lookup via `which`/`where` with executability verification +/// 2. Shell-based resolution via `$SHELL -l -i -c "command -v "` (Unix only) /// -/// Returns an error if the check fails. -pub async fn check_cli_available(name: &str) -> Result { - let result = Command::new("which") - .arg(name) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .output() - .await; - - match result { - Ok(output) => Ok(output.status.success()), - Err(_) => Ok(false), - } +/// Returns `false` for non-executable files in PATH. +/// +/// # Arguments +/// +/// * `name` - The command name to check (e.g., "codex", "claude", "gemini") +/// +/// # Returns +/// +/// `true` if the command is available, `false` otherwise. +#[must_use] +pub fn check_cli_available(name: &str) -> bool { + crate::core::cli_check::resolve_cli_command(name).is_available() } /// Waits for a shutdown signal on the watch channel. @@ -828,21 +920,18 @@ mod tests { use super::*; /// Tests `check_cli_available` with a command that should exist (sh). - #[tokio::test] - async fn check_cli_available_finds_sh() -> anyhow::Result<()> { + #[test] + fn check_cli_available_finds_sh() { // 'sh' should exist on all Unix systems - let result = check_cli_available("sh").await?; + let result = check_cli_available("sh"); assert!(result); - Ok(()) } /// Tests `check_cli_available` with a non-existent command. - #[tokio::test] - async fn check_cli_available_nonexistent_returns_false() -> anyhow::Result<()> { - let result = - check_cli_available("this_command_definitely_does_not_exist_12345").await?; + #[test] + fn check_cli_available_nonexistent_returns_false() { + let result = check_cli_available("this_command_definitely_does_not_exist_12345"); assert!(!result); - Ok(()) } /// Tests `wait_for_shutdown` returns immediately when already signaled. diff --git a/src/core/mod.rs b/src/core/mod.rs index b71a54f..7059504 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -9,7 +9,10 @@ pub mod retry; pub mod runner; pub mod task_utils; -pub use cli_check::{ModelAvailability, check_cli_in_path}; +pub use cli_check::{ + CommandResolution, ModelAvailability, check_cli_in_path, is_safe_command_name, + resolve_cli_command, +}; pub use commands::{ ClearCommand, CommandContext, CommandRegistry, CommandResult, ExitCommand, SettingsCommand, SlashCommand, diff --git a/src/core/runner.rs b/src/core/runner.rs index 5d7a599..75ec39f 100644 --- a/src/core/runner.rs +++ b/src/core/runner.rs @@ -927,7 +927,7 @@ mod tests { "mock" } - async fn is_available(&self) -> bool { + fn is_available(&self) -> bool { true } } @@ -989,7 +989,7 @@ mod tests { "mock" } - async fn is_available(&self) -> bool { + fn is_available(&self) -> bool { true } }