Better search paths and handling of CLI providers (#5554)

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

crates/goose/src/config/base.rs

@@ -8,6 +8,7 @@ use serde_json::Value;
 use serde_yaml::Mapping;
 use std::collections::HashMap;
 use std::env;
+use std::ffi::OsString;
 use std::fs::OpenOptions;
 use std::io::Write;
 use std::path::{Path, PathBuf};
@@ -138,17 +139,77 @@ impl Default for Config {
     }
 }
 
-macro_rules! declare_param {
-    ($param_name:ident, $param_type:ty) => {
-        paste::paste! {
-            pub fn [<get_ $param_name:lower>](&self) -> Result<$param_type, ConfigError> {
-                self.get_param(stringify!($param_name))
+pub trait ConfigValue {
+    const KEY: &'static str;
+    const DEFAULT: &'static str;
+}
+
+macro_rules! config_value {
+    ($key:ident, $type:ty) => {
+        impl Config {
+            paste::paste! {
+                pub fn [<get_ $key:lower>](&self) -> Result<$type, ConfigError> {
+                    self.get_param(stringify!($key))
+                }
+            }
+            paste::paste! {
+                pub fn [<set_ $key:lower>](&self, v: impl Into<$type>) -> Result<(), ConfigError> {
+                    self.set_param(stringify!($key), &v.into())
+                }
             }
         }
+    };
+
+    ($key:ident, $inner:ty, $default:expr) => {
         paste::paste! {
-            pub fn [<set_ $param_name:lower>](&self, v: impl Into<$param_type>) -> Result<(), ConfigError> {
-                self.set_param(stringify!($param_name), &v.into())
+            #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
+            #[serde(transparent)]
+            pub struct [<$key:camel>]($inner);
+
+            impl ConfigValue for [<$key:camel>] {
+                const KEY: &'static str = stringify!($key);
+                const DEFAULT: &'static str = $default;
             }
+
+            impl Default for [<$key:camel>] {
+                fn default() -> Self {
+                    [<$key:camel>]($default.into())
+                }
+            }
+
+            impl std::ops::Deref for [<$key:camel>] {
+                type Target = $inner;
+
+                fn deref(&self) -> &Self::Target {
+                    &self.0
+                }
+            }
+
+            impl std::ops::DerefMut for [<$key:camel>] {
+                fn deref_mut(&mut self) -> &mut Self::Target {
+                    &mut self.0
+                }
+            }
+
+            impl std::fmt::Display for [<$key:camel>] {
+                fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+                    write!(f, "{:?}", self.0)
+                }
+            }
+
+            impl From<$inner> for [<$key:camel>] {
+                fn from(value: $inner) -> Self {
+                    [<$key:camel>](value)
+                }
+            }
+
+            impl From<[<$key:camel>]> for $inner {
+                fn from(value: [<$key:camel>]) -> $inner {
+                    value.0
+                }
+            }
+
+            config_value!($key, [<$key:camel>]);
         }
     };
 }
@@ -738,13 +799,17 @@ impl Config {
         };
         Ok(())
     }
-
-    declare_param!(GOOSE_SEARCH_PATHS, Vec<String>);
-    declare_param!(GOOSE_MODE, GooseMode);
-    declare_param!(GOOSE_PROVIDER, String);
-    declare_param!(GOOSE_MODEL, String);
 }
 
+config_value!(CLAUDE_CODE_COMMAND, OsString, "claude");
+config_value!(GEMINI_CLI_COMMAND, OsString, "gemini");
+config_value!(CURSOR_AGENT_COMMAND, OsString, "cursor-agent");
+
+config_value!(GOOSE_SEARCH_PATHS, Vec<String>);
+config_value!(GOOSE_MODE, GooseMode);
+config_value!(GOOSE_PROVIDER, String);
+config_value!(GOOSE_MODEL, String);
+
 /// Load init-config.yaml from workspace root if it exists.
 /// This function is shared between the config recovery and the init_config endpoint.
 pub fn load_init_config_from_workspace() -> Result<Mapping, ConfigError> {

crates/goose/src/config/search_path.rs

@@ -1,25 +1,123 @@
-use std::{env, ffi::OsString, path::PathBuf};
+use std::{
+    env::{self},
+    ffi::{OsStr, OsString},
+    path::PathBuf,
+};
 
-use crate::config::{Config, ConfigError};
+use anyhow::{Context, Result};
 
-pub fn search_path_var() -> Result<OsString, ConfigError> {
-    let paths = Config::global()
-        .get_goose_search_paths()
-        .or_else(|err| match err {
-            ConfigError::NotFound(_) => Ok(vec![]),
-            err => Err(err),
-        })?
-        .into_iter()
-        .map(|s| PathBuf::from(shellexpand::tilde(&s).as_ref()));
+use crate::config::Config;
 
-    env::join_paths(
-        paths.chain(
-            env::var_os("PATH")
-                .as_ref()
-                .map(env::split_paths)
-                .into_iter()
-                .flatten(),
-        ),
-    )
-    .map_err(|e| ConfigError::DeserializeError(format!("{}", e)))
+pub struct SearchPaths {
+    paths: Vec<PathBuf>,
+}
+
+impl SearchPaths {
+    pub fn builder() -> Self {
+        let mut paths = Config::global()
+            .get_goose_search_paths()
+            .unwrap_or_default();
+
+        paths.push("~/.local/bin".into());
+
+        #[cfg(unix)]
+        {
+            paths.push("/usr/local/bin".into());
+        }
+
+        if cfg!(target_os = "macos") {
+            paths.push("/opt/homebrew/bin".into());
+            paths.push("/opt/local/bin".into());
+        }
+
+        Self {
+            paths: paths
+                .into_iter()
+                .map(|s| PathBuf::from(shellexpand::tilde(&s).as_ref()))
+                .collect(),
+        }
+    }
+
+    pub fn with_npm(mut self) -> Self {
+        if cfg!(windows) {
+            if let Some(appdata) = dirs::data_dir() {
+                self.paths.push(appdata.join("npm"));
+            }
+        } else if let Some(home) = dirs::home_dir() {
+            self.paths.push(home.join(".npm-global/bin"));
+        }
+        self
+    }
+
+    pub fn path(self) -> Result<OsString> {
+        env::join_paths(
+            self.paths.into_iter().chain(
+                env::var_os("PATH")
+                    .as_ref()
+                    .map(env::split_paths)
+                    .into_iter()
+                    .flatten(),
+            ),
+        )
+        .map_err(Into::into)
+    }
+
+    pub fn resolve<N>(self, name: N) -> Result<PathBuf>
+    where
+        N: AsRef<OsStr>,
+    {
+        which::which_in_global(name.as_ref(), Some(self.path()?))?
+            .next()
+            .with_context(|| {
+                format!(
+                    "could not resolve command '{}': file does not exist",
+                    name.as_ref().to_string_lossy()
+                )
+            })
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_path_preserves_existing_path() {
+        let search_paths = SearchPaths::builder();
+        let combined_path = search_paths.path().unwrap();
+
+        if let Some(existing_path) = env::var_os("PATH") {
+            let combined_str = combined_path.to_string_lossy();
+            let existing_str = existing_path.to_string_lossy();
+
+            assert!(combined_str.contains(&existing_str.to_string()));
+        }
+    }
+
+    #[test]
+    fn test_resolve_nonexistent_executable() {
+        let search_paths = SearchPaths::builder();
+
+        let result = search_paths.resolve("nonexistent_executable_12345_abcdef");
+
+        assert!(
+            result.is_err(),
+            "Resolving nonexistent executable should return an error"
+        );
+    }
+
+    #[test]
+    fn test_resolve_common_executable() {
+        let search_paths = SearchPaths::builder();
+
+        #[cfg(unix)]
+        let test_executable = "sh";
+
+        #[cfg(windows)]
+        let test_executable = "cmd";
+
+        search_paths
+            .resolve(test_executable)
+            .expect("should resolve sh (or cmd on Windows)");
+    }
 }