-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Rewrite extension management tools #5057
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
da55282 to
a25dc3a
Compare
|
closes #5068 |
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.
Three things spring out;
- We need to update the system prompt so that we no longer refer to this
- the router stuff seems intimately related to this, so shouldn't that be part of the platform-tool-ification too?
- how do we test this?
clippy-baselines/too_many_lines.txt
Outdated
| crates/goose/src/agents/agent.rs::create_recipe | ||
| crates/goose/src/agents/agent.rs::reply_internal | ||
| crates/goose/src/agents/extension_manager.rs::add_extension | ||
| crates/goose/src/agents/extension_manager_extension.rs::manage_extensions_impl |
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.
hmm, this seems a bit suspicious; this is a new file, no? these exceptions are only supposed to be for pre-existing things
| .filter(|tool| { | ||
| tool.name != PLATFORM_LIST_RESOURCES_TOOL_NAME | ||
| && tool.name != PLATFORM_READ_RESOURCE_TOOL_NAME | ||
| tool.name != LIST_RESOURCES_TOOL_NAME && tool.name != READ_RESOURCE_TOOL_NAME |
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.
I think we no longer want this. was always odd
crates/goose/src/agents/extension.rs
Outdated
| /// The name used to identify this extension | ||
| name: String, | ||
| description: String, | ||
| display_name: Option<String>, // needed for the UI |
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.
why do we need this for the UI?
| use tracing::error; | ||
|
|
||
| pub static EXTENSION_NAME: &str = "extensionmanager"; | ||
| pub static DISPLAY_NAME: &str = "Extension Manager"; |
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.
Why do we need both?
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.
somewhere in the code keeps modify the name "Extension Manager" to "extensionmanager", and then failing with a Unknown platform extension error. I spent sometime figuring out why that is, and decided it is faster to just add a display name like the built-ins.
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, I don't think that's a good reason. having names and display names is in general a recipe for confusion and mismatches, if we can't track them down ourselves that's even worse.
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.
i will track it down
| OperationFailed { message: String }, | ||
| } | ||
|
|
||
| // Tool name constants |
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.
that's not very informative
| Use this tool when you're unable to find a specific feature or functionality you need to complete your task, or when standard approaches aren't working. | ||
| These extensions might provide the exact tools needed to solve your problem. | ||
| If you find a relevant one, consider using your tools to enable it.".to_string(), | ||
| object!({ |
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.
this feels really brittle - we have had issues with wrong json schema here. @jamadeo & @alexhancock is there a better way?
| SEARCH_AVAILABLE_EXTENSIONS_TOOL_NAME => self | ||
| .handle_search_available_extensions() | ||
| .await | ||
| .map_err(|e| ExtensionManagerToolError::OperationFailed { |
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.
does returns a different type of error. any reason for that? if not, I think you could just collapse this thing and have result be future that you then await and map error
| } | ||
| // 4. Special case for extension management | ||
| else if tool_name == PLATFORM_MANAGE_EXTENSIONS_TOOL_NAME { | ||
| else if tool_name == MANAGE_EXTENSIONS_TOOL_NAME { |
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.
ugh
ui/desktop/src/components/settings/extensions/subcomponents/ExtensionList.tsx
Outdated
Show resolved
Hide resolved
| use super::platform_tools::{ | ||
| PLATFORM_LIST_RESOURCES_TOOL_NAME, PLATFORM_MANAGE_EXTENSIONS_TOOL_NAME, | ||
| PLATFORM_READ_RESOURCE_TOOL_NAME, PLATFORM_SEARCH_AVAILABLE_EXTENSIONS_TOOL_NAME, | ||
| use crate::agents::extension_manager_extension::{ |
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.
shouldn't this tool also be part of the extension_manager_extension? if you disable the extension_manager_extension_manager_manager, this will just be confusing to the agent
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.
I will add it.
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 second thought, the router tools should be separate from the extension_manager_extension. These tools are used for picking tools from an extension for a user query. And they should continue to work even if user disables the dynamic loading of extensions. So we should make that a separate extension.
|
closes #2331 |
3980020 to
7078a98
Compare
|
a73c170 to
92b1dd5
Compare
Signed-off-by: Angela Ning <[email protected]>
92b1dd5 to
e00e84e
Compare
* 'main' of github.com:block/goose: (22 commits) Rewrite extension management tools (#5057) fix: re-sync package-lock.json (#5235) docs: Hacktoberfest MCP youtube short entry to community-content.json (#5150) feat: add schedule button to recipe entries (#5217) Autocompact threshold UI cleanup (#5232) fix: correct schema for openai tools (#5229) Break compaction back into check_ and do_ compaction (#5212) fix: revert built app name to uppercase Goose (#5206) feat: add Code Documentation Generator recipe (#5121) (#5125) Revert "feat: enhance goose to search sessions for easy recall (#5177)" (#5209) Blog: Best Practices for Prompt Engineering with goose (#5204) force WAL sync after session create (#5202) Feat: goose Apify MCP integration docs (#5047) feat: enhance goose to search sessions for easy recall (#5177) Skip hidden & real format (#5194) docs: Hacktoberfest blog submission - Best Practices for Using Goose in Enterprise Environments by Anudhyan Datta. (#5184) docs: prompt injection detection (#5193) Fix mcp large response race condition (#5065) Compaction overhaul (#5186) fix: #3960 better approach to input schema for dynamic task params (#5189) ...
* main: delete flaky pricing integration tests (block#5207) chore: upgrade most npm packages to latest (block#5185) Release/1.11.0 (block#5224) Rewrite extension management tools (block#5057) fix: re-sync package-lock.json (block#5235) docs: Hacktoberfest MCP youtube short entry to community-content.json (block#5150) feat: add schedule button to recipe entries (block#5217) Autocompact threshold UI cleanup (block#5232) fix: correct schema for openai tools (block#5229) Break compaction back into check_ and do_ compaction (block#5212) fix: revert built app name to uppercase Goose (block#5206) feat: add Code Documentation Generator recipe (block#5121) (block#5125)
This reverts commit bccfa01.
* main: Standardize Session Name Attribute (#5085) Docs: Include step-by-step model configuration instructions for first… (#5239) Delete message visibility filter (#5216) delete flaky pricing integration tests (#5207) chore: upgrade most npm packages to latest (#5185) Release/1.11.0 (#5224) Rewrite extension management tools (#5057) fix: re-sync package-lock.json (#5235) docs: Hacktoberfest MCP youtube short entry to community-content.json (#5150) feat: add schedule button to recipe entries (#5217) Autocompact threshold UI cleanup (#5232) fix: correct schema for openai tools (#5229)
* origin/main: (66 commits) Revert "Rewrite extension management tools" (#5243) Standardize Session Name Attribute (#5085) Docs: Include step-by-step model configuration instructions for first… (#5239) Delete message visibility filter (#5216) delete flaky pricing integration tests (#5207) chore: upgrade most npm packages to latest (#5185) Release/1.11.0 (#5224) Rewrite extension management tools (#5057) fix: re-sync package-lock.json (#5235) docs: Hacktoberfest MCP youtube short entry to community-content.json (#5150) feat: add schedule button to recipe entries (#5217) Autocompact threshold UI cleanup (#5232) fix: correct schema for openai tools (#5229) Break compaction back into check_ and do_ compaction (#5212) fix: revert built app name to uppercase Goose (#5206) feat: add Code Documentation Generator recipe (#5121) (#5125) Revert "feat: enhance goose to search sessions for easy recall (#5177)" (#5209) Blog: Best Practices for Prompt Engineering with goose (#5204) force WAL sync after session create (#5202) Feat: goose Apify MCP integration docs (#5047) ...
* main: (32 commits) turn off WAL (#5203) Skip subagents for gemini (#5257) Revert "Standardize Session Name Attribute" (#5250) improve provider request logging a bit (#5236) Fix OpenAI empty choices panic (#5248) Revert "Rewrite extension management tools" (#5243) Standardize Session Name Attribute (#5085) Docs: Include step-by-step model configuration instructions for first… (#5239) Delete message visibility filter (#5216) delete flaky pricing integration tests (#5207) chore: upgrade most npm packages to latest (#5185) Release/1.11.0 (#5224) Rewrite extension management tools (#5057) fix: re-sync package-lock.json (#5235) docs: Hacktoberfest MCP youtube short entry to community-content.json (#5150) feat: add schedule button to recipe entries (#5217) Autocompact threshold UI cleanup (#5232) fix: correct schema for openai tools (#5229) Break compaction back into check_ and do_ compaction (#5212) fix: revert built app name to uppercase Goose (#5206) ...
* 'main' of github.com:block/goose: Rewrite extension management tools (#5057) fix: re-sync package-lock.json (#5235) docs: Hacktoberfest MCP youtube short entry to community-content.json (#5150) feat: add schedule button to recipe entries (#5217) Autocompact threshold UI cleanup (#5232) fix: correct schema for openai tools (#5229)
Signed-off-by: Angela Ning <[email protected]> Signed-off-by: vietbui99 <[email protected]>
Summary
This PR refactors extension management tools from being hardcoded in the Agent into a proper platform extension (ExtensionManagerClient), allowing it to be enabled/disabled like any other extension.
Type of Change
Testing
Manually tested on UI and CLI
Related Issues
Addesses #2331
Screenshots/Demos (for UX changes)
Before:
After:

Agent should not have access to the extension management tools when the extension_manager extension is disabled
Re-enabled the extension, agent can now autonomously discover and enable/disable tools.

