-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Better search paths and handling of CLI providers #5554
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the executable search logic for CLI providers by centralizing command configuration and path resolution. The changes move from provider-specific executable finding functions to a unified configuration-based approach with enhanced PATH management.
Key changes:
- Centralized command configuration with defaults defined using new
declare_param!macro variants - Moved executable search logic from individual providers into the
SearchPathsutility with npm-specific path support - Simplified provider initialization by delegating command resolution to the config layer
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/goose/src/config/base.rs | Added macro for declaring config parameters with defaults and defined command defaults for claude, gemini, and cursor-agent CLIs |
| crates/goose/src/config/search_path.rs | Refactored into a builder pattern with support for npm-specific paths and converted from function to struct-based API |
| crates/goose/src/providers/gemini_cli.rs | Removed local executable search logic and replaced with config-based command retrieval; added PATH configuration via SearchPaths |
| crates/goose/src/providers/claude_code.rs | Removed local executable search logic and replaced with config-based command retrieval; added PATH configuration via SearchPaths |
| crates/goose/src/providers/cursor_agent.rs | Updated to use new config-based command retrieval method |
| crates/goose/src/agents/extension_manager.rs | Updated to use new SearchPaths builder API and made PATH setting non-fatal |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rs-paths' into jamadeo/cli-providers-paths
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
crates/goose/src/providers/claude_code.rs:276
- The claude_code provider is missing the PATH environment variable configuration that was added to gemini_cli and cursor_agent providers. After line 265 (Command::new), the following should be added:\n\n
rust\nif let Ok(path) = SearchPaths::builder().with_npm().path() {\n cmd.env(\"PATH\", path);\n}\n\n\nThis ensures npm-installed claude commands are found in the PATH.
let mut cmd = Command::new(&self.command);
cmd.arg("-p")
.arg(messages_json.to_string())
.arg("--system-prompt")
.arg(&filtered_system);
// Only pass model parameter if it's in the known models list
if CLAUDE_CODE_KNOWN_MODELS.contains(&self.model.model_name.as_str()) {
cmd.arg("--model").arg(&self.model.model_name);
}
cmd.arg("--verbose").arg("--output-format").arg("json");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
btw when i use goose, its breaking path of my nodejs installed with scoop.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ktop/src/components/settings/providers/modal/subcomponents/handlers/DefaultSubmitHandler.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ktop/src/components/settings/providers/modal/subcomponents/handlers/DefaultSubmitHandler.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| create_with_default_model(&provider) | ||
| .await | ||
| .and_then(|_| { | ||
| let config = Config::global(); | ||
| config | ||
| .set_goose_provider(provider) | ||
| .and_then(|_| config.set_goose_model(model)) | ||
| .map_err(|e| anyhow::anyhow!(e)) | ||
| }) |
Copilot
AI
Nov 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The set_provider function validates the provider using create_with_default_model, but then sets the model from the request parameter without validating whether that model is actually supported by the provider. This could allow invalid provider/model combinations to be saved to the configuration.
ui/desktop/src/components/settings/providers/modal/ProviderConfiguationModal.tsx
Show resolved
Hide resolved
DOsinga
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work!
| path = "/config/set_provider", | ||
| request_body = SetProviderRequest, | ||
| )] | ||
| pub async fn set_provider( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we call this one set_config_provider and the other set_agent_provider for easier reading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, good idea
| } | ||
|
|
||
| if cfg!(target_os = "macos") { | ||
| paths.push("/opt/homebrew/bin".into()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
macports uses: /opt/local/bin
| #[cfg(unix)] | ||
| { | ||
| paths.push("/usr/local/bin".into()); | ||
| paths.push("~/.local/bin".into()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also /usr/bin or is that understood
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on mac/desktop PATH is /usr/bin:/bin:/usr/sbin:/sbin (started via launchctl) so I think it's safe to assume that will always be there. I'm not entirely sure of the situation on linux but it's probably also in the system path?
| if cfg!(target_os = "macos") { | ||
| paths.push("/opt/homebrew/bin".into()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my llm reluctantly suggests:
#[cfg(windows)]
{
paths.push("%APPDATA%\npm".into());
paths.push("%USERPROFILE%\.local\bin".into());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
appdata/npm is covered in the with_npm bit. For home/.local/bin, yeah that can just be pulled out of the if cfg(unix) arm since shellexpand will do the right thing on windows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated 25 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, | ||
| "required": true | ||
| }, | ||
| "responses": {} |
Copilot
AI
Nov 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The set_config_provider endpoint has an empty responses object. This means no status codes or response schemas are documented, making it unclear to API consumers what responses to expect (success/error codes, response body format, etc.). Add proper response definitions including at least 200 (success) and 400 (bad request) status codes with appropriate descriptions.
| "responses": {} | |
| "responses": { | |
| "200": { | |
| "description": "Configuration provider set successfully", | |
| "content": { | |
| "text/plain": { | |
| "schema": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| }, | |
| "400": { | |
| "description": "Invalid request body or parameters" | |
| } | |
| } |
| async (parameter: { | ||
| name: string; | ||
| required?: boolean; | ||
| default?: unknown; | ||
| secret?: boolean; | ||
| }) => { |
Copilot
AI
Nov 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The async keyword has been added to the arrow function, but the function body only contains synchronous operations (return statements). This creates unnecessary overhead and confusion since there are no await statements in the function body. The function should remain synchronous.
| if cfg!(windows) { | ||
| if let Some(appdata) = dirs::data_dir() { | ||
| self.paths.push(appdata.join("npm")); | ||
| } |
Copilot
AI
Nov 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Windows, dirs::data_dir() typically returns %APPDATA% (e.g., C:\Users\User\AppData\Roaming). However, npm global packages are usually installed to %APPDATA%\npm on Windows. The path should verify this is the correct location, as npm's actual global directory can vary based on configuration. Consider also checking npm config get prefix or looking for the npm installation in other common locations.
| if let Ok(path) = SearchPaths::builder().with_npm().path() { | ||
| cmd.env("PATH", path); |
Copilot
AI
Nov 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PATH environment variable is conditionally set only if SearchPaths::builder().with_npm().path() succeeds. If it fails, the command will use the inherited PATH. Consider logging a warning or handling the error more explicitly for debugging purposes.
| if let Ok(path) = SearchPaths::builder().with_npm().path() { | |
| cmd.env("PATH", path); | |
| match SearchPaths::builder().with_npm().path() { | |
| Ok(path) => { | |
| cmd.env("PATH", path); | |
| } | |
| Err(e) => { | |
| tracing::warn!( | |
| "Failed to resolve search path for cursor-agent CLI (using inherited PATH): {}", | |
| e | |
| ); | |
| } |
| )))?; | ||
| let mut child = cmd.spawn().map_err(|e| { | ||
| ProviderError::RequestFailed(format!( | ||
| "Failed to spawn Claude CLI command '{:?}': {}.", |
Copilot
AI
Nov 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new error message is much less helpful than the previous one. The old message provided clear guidance about ensuring Claude Code CLI is installed, logged in, and available in PATH, plus mentioned the fallback to Anthropic provider and included documentation links. The new message only states the command failed. This is a regression in error message quality that will make troubleshooting harder for users.
| "Failed to spawn Claude CLI command '{:?}': {}.", | |
| "Failed to spawn Claude CLI command '{:?}': {}.\n\ | |
| Please ensure that the Claude Code CLI is installed, logged in, and available in your system PATH.\n\ | |
| If the problem persists, Goose will fall back to the Anthropic provider if configured.\n\ | |
| For installation and troubleshooting instructions, see: https://github.com/anthropics/claude-code-cli#installation\n\ | |
| Documentation: https://github.com/anthropics/claude-code-cli#troubleshooting", |
| 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>]); | ||
| } | ||
| }; | ||
| } |
Copilot
AI
Nov 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new config_value! macro and ConfigValue trait lack documentation. Add doc comments explaining their purpose: to define configuration value types with associated keys and defaults, and to generate the necessary getter/setter methods and type implementations.
| pub fn configure_command_no_window(command: &mut Command) { | ||
| #[cfg(windows)] | ||
| command.creation_flags(CREATE_NO_WINDOW_FLAG); | ||
| } |
Copilot
AI
Nov 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function configure_command_no_window lacks documentation explaining its purpose and when it should be used. Add a doc comment explaining that this configures a command to run without creating a visible console window on Windows, which is important for GUI applications that spawn background processes.
| "Failed to spawn Gemini CLI command '{}': {}. \ | ||
| Make sure the Gemini CLI is installed and in your PATH.", | ||
| "Failed to spawn Gemini CLI command '{:?}': {}. \ | ||
| Make sure the Gemini CLI is installed and available in the configured search paths.", |
Copilot
AI
Nov 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message says "Make sure the Gemini CLI is installed and available in the configured search paths." However, it doesn't provide information about what those configured search paths are or how to configure them. Consider including guidance about where to install the CLI or how to configure the search path.
| Make sure the Gemini CLI is installed and available in the configured search paths.", | |
| Make sure the Gemini CLI is installed and available in a directory included in your system PATH. \ | |
| You can install the Gemini CLI via npm (e.g., 'npm install -g gemini-cli'), or place the binary in a directory listed in your PATH environment variable. \ | |
| To check your current PATH, run 'echo $PATH' (Linux/macOS) or 'echo %PATH%' (Windows). \ | |
| If you need to modify your PATH, consult your operating system documentation.", |
| "Failed to spawn cursor-agent CLI command '{}': {}. \ | ||
| Make sure the cursor-agent CLI is installed and in your PATH, or set CURSOR_AGENT_COMMAND in your config to the correct path.", | ||
| "Failed to spawn cursor-agent CLI command '{:?}': {}. \ | ||
| Make sure the cursor-agent CLI is installed and available in the configured search paths, or set CURSOR_AGENT_COMMAND in your config to the correct path.", |
Copilot
AI
Nov 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message mentions "Make sure the cursor-agent CLI is installed and available in the configured search paths, or set CURSOR_AGENT_COMMAND in your config to the correct path." However, it doesn't explain what the configured search paths are or provide helpful guidance on where to install cursor-agent. Consider adding more specific installation guidance or documentation links.
| Make sure the cursor-agent CLI is installed and available in the configured search paths, or set CURSOR_AGENT_COMMAND in your config to the correct path.", | |
| Make sure the cursor-agent CLI is installed and available in your system PATH (e.g., install via 'npm install -g cursor-agent'), \ | |
| or set CURSOR_AGENT_COMMAND in your config to the correct path. \ | |
| For installation instructions, see: https://github.com/cursor-agent/cursor-agent#installation", |
| "responses": {} | ||
| } |
Copilot
AI
Nov 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check_provider endpoint has an empty responses object. This means no status codes or response schemas are documented, making it unclear to API consumers what responses to expect (success/error codes, response body format, etc.). Add proper response definitions including at least 200 (success) and 400 (bad request) status codes with appropriate descriptions.
| "responses": {} | |
| } | |
| "responses": { | |
| "200": { | |
| "description": "Provider check completed successfully", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "valid": { | |
| "type": "boolean", | |
| "description": "Whether the provider configuration is valid" | |
| }, | |
| "message": { | |
| "type": "string", | |
| "description": "Additional information about the check" | |
| } | |
| }, | |
| "required": ["valid"] | |
| } | |
| } | |
| } | |
| }, | |
| "400": { | |
| "description": "Invalid request" | |
| } | |
| } |
* main: (33 commits) Fix Claude Code provider to default to Auto mode (#5638) (#5642) Scheduler cleanup (#5571) Better search paths and handling of CLI providers (#5554) docs: description required for "Add Extension" in cli - phase 2 (#5635) Remove some logging (#5631) Use session IDs as task IDs for subagents instead of UUIDs (#5398) Fix the naming (#5628) fix: default tetrate model is broken, replace with haiku-4.5 (#5535) (#5587) Fetch less and use the right SHA (#5621) feat(ui): add custom macOS dock menu with New Window option (#5099) feat: remove hints from recipe prompts (#5622) docs: October 2025 Community All-Stars spotlight, Hacktoberfest edition (#5625) differentiate debug/release in cache key (#5613) Unify subrecipe and subagent execution through shared recipe pipeline (#5082) Standardize CLI argument flags and update documentation (#5516) Release 1.13.0 fix: move goosehints/AGENTS.md handling to goose, and out of developer extension (#5575) fix: add standard context menu items to prevent empty right-click menu (#5616) Bump openapi in prepare-release (#5611) docs: add access control section to Developer tutorial (#5615) ...
* main: (83 commits) silence copilot on minor text issues (block#5665) fix: disallow runaway subagent chains (block#5659) chore: remove usage of non-existent env var for log dir (block#5658) clarify agent instructions (block#5655) feat: add check-everything for unified style checks (block#5650) Show errors on failure (block#5643) custom instructions for copilot reviews (block#5646) fix: prevent repeated 404 errors when accessing deleted sessions (block#5644) Flake.nix corrected main (block#5600) fix: goose recipe list can return duplicated entries (block#5645) fix: bedrock creds refresh (block#5599) Fix Claude Code provider to default to Auto mode (block#5638) (block#5642) Scheduler cleanup (block#5571) Better search paths and handling of CLI providers (block#5554) docs: description required for "Add Extension" in cli - phase 2 (block#5635) Remove some logging (block#5631) Use session IDs as task IDs for subagents instead of UUIDs (block#5398) Fix the naming (block#5628) fix: default tetrate model is broken, replace with haiku-4.5 (block#5535) (block#5587) Fetch less and use the right SHA (block#5621) ...
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Blair Allan <[email protected]>
The big changes here are better binary resolution using the search paths from your config plus some common npm paths, and better handling of provider setups when we can't actually create the provider.
Before, we'd let you configure a provider in goose desktop even if we couldn't create it in goosed. Now, you'll see something like this:
in the provider config section, and if you do add it to your config anyway, you'll see:
if you try to use it.
fixes #4133