resolved all the entensions to load in cli#6464
Conversation
…pass Recipe directly to SessionBuilderConfig
* main: fix: require auth when running goose on non loopback address (#6478) chore(deps): bump hono from 4.11.3 to 4.11.4 in /ui/desktop (#6485) feat(cli): graceful fallback for keyring failures (#5808) fix: support global .gooseignore and negation patterns (#6157) docs: manual config for jetbrains (#6490) fix: Recipe slash command doesn't work with single optional parameter (#6235) fix(openrouter): Handle Gemini thoughtSignature for tool calls (#6370) docs: fix extensions page (#6484) Allow customizing the new line keybinding in the CLI (#5956) Ask for permission in the CLI (#6475) docs: add Ralph Loop tutorial for multi-model iterative development (#6455) Remove gitignore fallback from gooseignore docs (#6480) fix: clean up result recording for code mode (#6343) fix(code_execution): handle model quirks with tool calls (#6352) feat(ui): support prefersBorder option for MCP Apps (#6465) fixed line breaks (#6459) Use Intl.NumberFormat for token formatting in SessionsInsights (#6466) feat(ui): format large and small token counts for readability (#6449) fix: apply subrecipes when using slash commands (#6460)
There was a problem hiding this comment.
Pull request overview
This PR refactors extension resolution in the CLI to centralize the logic for determining which extensions to load across different contexts (CLI, Desktop, Scheduler). The changes eliminate redundant intermediate structs and simplify the session builder configuration.
Changes:
- Created a new
resolve_extensions_for_new_session()function to centralize extension resolution logic with clear priority: recipe extensions > override extensions > global defaults - Refactored
SessionBuilderConfigto use a singlerecipe: Option<Recipe>field instead of multiple recipe-related fields - Converted CLI session extension methods to static parsing methods that return
ExtensionConfig, enabling parallel extension loading
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/goose/src/session/mod.rs | Exports new extension resolver module |
| crates/goose/src/session/extension_resolver.rs | New module with centralized extension resolution logic and tests |
| crates/goose/src/scheduler.rs | Updated to use new extension resolver |
| crates/goose-server/src/routes/agent.rs | Updated to use new extension resolver |
| crates/goose-cli/src/session/mod.rs | Refactored extension methods to be static parsers |
| crates/goose-cli/src/session/builder.rs | Simplified config struct and extraction of extension loading logic |
| crates/goose-cli/src/recipes/extract_from_cli.rs | Returns Recipe directly instead of RecipeInfo struct |
| crates/goose-cli/src/commands/bench.rs | Updated to use simplified SessionBuilderConfig |
| crates/goose-cli/src/cli.rs | Removed RecipeInfo struct, simplified input handling |
| } | ||
|
|
||
| spinner.clear(); | ||
| let cli_flag_extension_extensions_to_load = parse_cli_flag_extensions( |
There was a problem hiding this comment.
The variable name cli_flag_extension_extensions_to_load has "extension" duplicated in it. Consider renaming to cli_flag_extensions_to_load for better clarity.
| pub fn resolve_extensions_for_new_session( | ||
| recipe_extensions: Option<&[ExtensionConfig]>, | ||
| override_extensions: Option<Vec<ExtensionConfig>>, | ||
| ) -> Vec<ExtensionConfig> { | ||
| if let Some(exts) = recipe_extensions { | ||
| return exts.to_vec(); | ||
| } | ||
|
|
||
| if let Some(exts) = override_extensions { | ||
| return exts; | ||
| } | ||
|
|
||
| get_enabled_extensions() | ||
| } |
There was a problem hiding this comment.
The test coverage for resolve_extensions_for_new_session is incomplete. There's no test case covering the scenario where both recipe_extensions and override_extensions are None, which should fall back to calling get_enabled_extensions(). Consider adding a test case for this default behavior.
* main: fixed 0 token in openrouter steaming (#6493) feat(goose-acp): enable parallel sessions with isolated agent state (#6392) copilot instruction to flag prelease docs (#6504) docs: acp mcp support (#6491) feat: add flatpak support for linux (#6387) fix(code_execution): serialize record_result output as JSON (#6495) perf(google): avoid accumulating thoughtSignatures across conversation history (#6462) fix(openai): make tool_call arguments optional and fix silent stream termination (#6309) fix: Improve error messages for invalid tool calls (#6483)
| use goose::recipe::Recipe; | ||
| use goose::session::session_manager::SessionType; | ||
| use goose::session::{EnabledExtensionsState, ExtensionState}; | ||
| use goose::session::SessionManager; |
There was a problem hiding this comment.
The import SessionManager is unused. Remove it to keep the code clean.
| use goose::session::SessionManager; |
| use goose::session::session_manager::SessionType; | ||
| use goose::session::{EnabledExtensionsState, ExtensionState}; | ||
| use goose::session::SessionManager; | ||
| use goose::session::{resolve_extensions_for_new_session, EnabledExtensionsState, ExtensionState}; |
There was a problem hiding this comment.
The import ExtensionState is unused. Remove it to keep the code clean.
| use goose::session::{resolve_extensions_for_new_session, EnabledExtensionsState, ExtensionState}; | |
| use goose::session::{resolve_extensions_for_new_session, EnabledExtensionsState}; |
| use super::CliSession; | ||
| use console::style; | ||
| use goose::agents::types::{RetryConfig, SessionConfig}; | ||
| use goose::agents::extension::PlatformExtensionContext; |
There was a problem hiding this comment.
The import PlatformExtensionContext is unused. Remove it to keep the code clean.
| use goose::agents::extension::PlatformExtensionContext; |
| .iter() | ||
| .map(|cfg| (cfg.name(), cfg.clone())) | ||
| .collect(); | ||
| extensions_to_load.extend(cli_flag_extensions_to_load); |
There was a problem hiding this comment.
CLI flag extensions are always added to sessions, even on resume. This changes the behavior from the original code where CLI flags were only added to new sessions (not resumed ones). If this is intentional, the behavior should be documented or clearly commented.
| extensions_to_load.extend(cli_flag_extensions_to_load); | |
| if !session_config.resume { | |
| // Preserve original behavior: only apply CLI flag extensions to new sessions, not resumed ones. | |
| extensions_to_load.extend(cli_flag_extensions_to_load); | |
| } |
There was a problem hiding this comment.
The goose cli resume accept the cli flag extension, it should be added to the session
| // Create new session | ||
| let mut session = CliSession::new( | ||
| let session = CliSession::new( | ||
| Arc::try_unwrap(agent_ptr).unwrap_or_else(|_| panic!("There should be no more references")), |
There was a problem hiding this comment.
Using panic in production code violates the project's error handling guidelines. This should return an error with proper context instead.
| @@ -0,0 +1,62 @@ | |||
| use crate::config::{get_enabled_extensions, ExtensionConfig}; | |||
|
|
|||
| pub fn resolve_extensions_for_new_session( | |||
There was a problem hiding this comment.
not sure this warrants its own module. also copilots complaints about test coverage, since this is just basically an if statement in a function, I dont know that we need any testing. can we move this somewhere?
* main: increase worker threads for ci (#6614) docs: todo tutorial update (#6613) Added goose doc map md file for goose agent to find relevant doc easily. (#6598) add back goose branding to home (#6617) fix: actually set the working dir for extensions from session (#6612) Multi chat (#6428) Lifei/fixed accumulated token count (#6587) Dont show MCP UI/Apps until tool is approved (#6492) docs: max tokens config (#6596) User configurable templates (#6420) docs: http proxy environment variables (#6594) feat: exclude subagent tool from code_execution filtering (#6531) Fix path for global agent skills (#6591) recipes: add mcp server (#6552) feat(gcp-vertex): add model list with org policy filtering (#6393) chore: encourage extension searching (#6582) blog: mobile apps consolidation and roadmap (#6580) chore: remove unused dependencies in cargo.toml (#6561) resolved all the extensions to load in cli (#6464)
Signed-off-by: fbalicchia <fbalicchia@cuebiq.com>
Summary
This PR refactors the CLI session builder to simplify extension resolution and eliminate redundant intermediate structs.
Key Changes
Removed intermediate structs: Eliminated RecipeInfo, SessionSettings, and InputConfig.extensions_override - now passing the Recipe directly to SessionBuilderConfig
Centralized extension resolution: Created a new resolve_extensions_for_new_session() function as the single source of truth for determining which extensions to load for CLI, Desktop and Scheduler
Simplified SessionBuilderConfig: Reduced fields by consolidating recipe-related fields (sub_recipes, final_output_response, retry_config, settings, extensions_override) into a single recipe: Option field
Refactored extension loading in CLI: Extracted load_extensions() and parse_cli_flag_extensions() helper functions to clean up the session builder logic
Unified extension parsing: Changed CliSession methods (add_extension, add_streamable_http_extension, add_builtin) to use static parsing methods that return ExtensionConfig, enabling parallel extension loading
Type of Change
AI Assistance
Testing
Unit test and Manual testing
Related Issues
Relates to Issue #6023