Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the add_builtin method to differentiate between platform extensions and builtin extensions. Platform extensions (like todo and extensionmanager) are now properly routed to use ExtensionConfig::Platform instead of ExtensionConfig::Builtin. The test script is also updated to include the new platform extensions in testing.
- Adds logic to check if an extension is a platform extension using
PLATFORM_EXTENSIONS - Routes platform extensions to
ExtensionConfig::Platformconfiguration - Updates test script to include
todoandextensionmanagerin the builtin list
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| scripts/test_providers.sh | Adds todo and extensionmanager to the list of builtin extensions tested |
| crates/goose-cli/src/session/mod.rs | Implements conditional logic to create Platform vs Builtin extension configs based on extension type |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/goose-cli/src/session/mod.rs
Outdated
| description: name.trim().to_string(), | ||
| available_tools: Vec::new(), | ||
|
|
||
| let config = if PLATFORM_EXTENSIONS.contains_key(name) { |
There was a problem hiding this comment.
The contains_key check uses the untrimmed name variable, but PLATFORM_EXTENSIONS keys are lowercase without spaces (e.g., "extensionmanager", "todo", "chatrecall"). The trimmed extension_name should be used here, or better yet, apply the same normalize() function used in extension_manager.rs which strips whitespace and converts to lowercase. Without this, platform extensions won't be recognized unless the user provides the exact key format.
crates/goose-cli/src/session/mod.rs
Outdated
| description: name.trim().to_string(), | ||
| available_tools: Vec::new(), | ||
| } | ||
| } else { | ||
| ExtensionConfig::Builtin { | ||
| name: extension_name, | ||
| display_name: None, | ||
| timeout: None, | ||
| bundled: None, | ||
| description: name.trim().to_string(), |
There was a problem hiding this comment.
The name parameter is being called with trim() twice: once to create extension_name (line 300) and again here (line 306). Use extension_name consistently throughout both branches instead of re-trimming name.
| description: name.trim().to_string(), | |
| available_tools: Vec::new(), | |
| } | |
| } else { | |
| ExtensionConfig::Builtin { | |
| name: extension_name, | |
| display_name: None, | |
| timeout: None, | |
| bundled: None, | |
| description: name.trim().to_string(), | |
| description: extension_name.clone(), | |
| available_tools: Vec::new(), | |
| } | |
| } else { | |
| ExtensionConfig::Builtin { | |
| name: extension_name.clone(), | |
| display_name: None, | |
| timeout: None, | |
| bundled: None, | |
| description: extension_name, |
crates/goose-cli/src/session/mod.rs
Outdated
| name: extension_name, | ||
| bundled: None, | ||
| description: name.trim().to_string(), | ||
| available_tools: Vec::new(), | ||
| } | ||
| } else { | ||
| ExtensionConfig::Builtin { | ||
| name: extension_name, | ||
| display_name: None, | ||
| timeout: None, | ||
| bundled: None, | ||
| description: name.trim().to_string(), |
There was a problem hiding this comment.
The name parameter is being called with trim() twice: once to create extension_name (line 300) and again here (line 315). Use extension_name consistently throughout both branches instead of re-trimming name.
| name: extension_name, | |
| bundled: None, | |
| description: name.trim().to_string(), | |
| available_tools: Vec::new(), | |
| } | |
| } else { | |
| ExtensionConfig::Builtin { | |
| name: extension_name, | |
| display_name: None, | |
| timeout: None, | |
| bundled: None, | |
| description: name.trim().to_string(), | |
| name: extension_name.clone(), | |
| bundled: None, | |
| description: extension_name.clone(), | |
| available_tools: Vec::new(), | |
| } | |
| } else { | |
| ExtensionConfig::Builtin { | |
| name: extension_name.clone(), | |
| display_name: None, | |
| timeout: None, | |
| bundled: None, | |
| description: extension_name.clone(), |
71d7225 to
f662b34
Compare
|
(rebased this because I want to see if the provider tests are more reliable after recent merges) |
| ExtensionConfig::Builtin { | ||
| name: extension_name.to_string(), | ||
| display_name: None, | ||
| timeout: None, |
There was a problem hiding this comment.
what is the effect of the switch to have no timeout vs the prior
Some(goose::config::DEFAULT_EXTENSION_TIMEOUT)
?
There was a problem hiding this comment.
Interested to know, but doesn't seem like a big deal to me as we wouldn't expect the builtins to time out conceptually
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ExtensionConfig::Platform { | ||
| name: extension_name.to_string(), | ||
| bundled: None, | ||
| description: name.to_string(), |
There was a problem hiding this comment.
Inconsistent description field usage. Line 308 uses name.to_string() while line 317 also uses name.to_string(), but extension_name was already trimmed on line 302. Line 308 should use extension_name.to_string() to avoid potential whitespace in the description.
| display_name: None, | ||
| timeout: None, | ||
| bundled: None, | ||
| description: name.to_string(), |
There was a problem hiding this comment.
The description field should use extension_name.to_string() instead of name.to_string() to ensure it uses the trimmed version of the name and maintains consistency with the name field on line 313.
* main: (60 commits) 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) Token state not showing on load, or after message is finished. (#5606) Change the other location too (#5608) feat(ui): bring back quick launcher (#5144) Support platform tools through CLI (#5570) Avoid web double write (#5601) fix: gemini flash -> pro for mcp smoke tests (#5574) Manual compaction test and fix (#5568) fix: tidy up claude cli handling (#5594) Remove jetbrains (#5602) feat(githubcopilot): add support for newer Copilot AI Models (#5603) fix: customised recipe to yaml string to avoid minininjia parsing error (#5494) Add pending extension indicator to extension panel (#5493) Add environment subsition for auth blocks (#5439) acp: ToolCallLocations and working cancellation (#5588) feat(providers): add Mistral AI provider (#5009) Listen for ctrl-c during provider request (#5585) Also accept null as description, not just missing (#5589) ...
* main: (31 commits) 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) Token state not showing on load, or after message is finished. (#5606) Change the other location too (#5608) feat(ui): bring back quick launcher (#5144) Support platform tools through CLI (#5570) Avoid web double write (#5601) fix: gemini flash -> pro for mcp smoke tests (#5574) Manual compaction test and fix (#5568) fix: tidy up claude cli handling (#5594) Remove jetbrains (#5602) feat(githubcopilot): add support for newer Copilot AI Models (#5603) fix: customised recipe to yaml string to avoid minininjia parsing error (#5494) Add pending extension indicator to extension panel (#5493) Add environment subsition for auth blocks (#5439) acp: ToolCallLocations and working cancellation (#5588) ...
* main: (21 commits) 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) Token state not showing on load, or after message is finished. (#5606) Change the other location too (#5608) feat(ui): bring back quick launcher (#5144) Support platform tools through CLI (#5570) Avoid web double write (#5601) fix: gemini flash -> pro for mcp smoke tests (#5574) Manual compaction test and fix (#5568) fix: tidy up claude cli handling (#5594) Remove jetbrains (#5602) feat(githubcopilot): add support for newer Copilot AI Models (#5603) fix: customised recipe to yaml string to avoid minininjia parsing error (#5494) Add pending extension indicator to extension panel (#5493) ...
Co-authored-by: Douwe Osinga <douwe@squareup.com> Signed-off-by: fbalicchia <fbalicchia@gmail.com>
* origin/main: (34 commits) 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) Token state not showing on load, or after message is finished. (#5606) Change the other location too (#5608) feat(ui): bring back quick launcher (#5144) Support platform tools through CLI (#5570) ...
Co-authored-by: Douwe Osinga <douwe@squareup.com>
Co-authored-by: Douwe Osinga <douwe@squareup.com>
Co-authored-by: Douwe Osinga <douwe@squareup.com> Signed-off-by: Blair Allan <Blairallan@icloud.com>
Since platform tools are built-in but have their own type you couldn't explicitly enable them through the CLI. This changes that