Conversation
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces optional per‐subrecipe configuration (timeout, max/min workers) and propagates it through task creation and execution.
- Added
SubRecipeConfigand an optionalconfigfield toSubRecipe - Enhanced CLI extractor and
create_sub_recipe_taskto includeconfigin the payload - Merged task‐level overrides into the executor’s global config before running
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/goose/src/recipe/mod.rs | Added SubRecipeConfig and config: Option<SubRecipeConfig> |
| crates/goose/src/agents/sub_recipe_execution_tool/tasks.rs | Made command parameters optional when building the command |
| crates/goose/src/agents/sub_recipe_execution_tool/executor.rs | Merged task‐level config into effective_config before execution |
| crates/goose/src/agents/recipe_tools/sub_recipe_tools.rs | Updated payload builder to include config if present |
| crates/goose-cli/src/recipes/extract_from_cli.rs | Set config: None for CLI‐extracted subrecipes |
Comments suppressed due to low confidence (2)
crates/goose/src/recipe/mod.rs:145
- [nitpick] Add doc comments for
SubRecipeConfigand its fields (timeout_seconds,max_workers,initial_workers) to explain their purpose and valid ranges.
pub struct SubRecipeConfig {
crates/goose/src/agents/sub_recipe_execution_tool/executor.rs:140
- [nitpick] The test doesn’t actually assert that the overridden
timeout_secondsis applied at runtime. Add a test (or mockprocess_task) that fails when the override isn’t honored.
async fn test_execute_single_task_with_config_override() {
| pub path: String, | ||
| #[serde(default, deserialize_with = "deserialize_value_map_as_string")] | ||
| pub values: Option<HashMap<String, String>>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] |
There was a problem hiding this comment.
The config field lacks #[serde(default)], so deserializing a subrecipe without a config key will error. Add #[serde(default, skip_serializing_if = "Option::is_none")] above it to treat missing config as None.
| #[serde(skip_serializing_if = "Option::is_none")] | |
| #[serde(default, skip_serializing_if = "Option::is_none")] |
| if let Some(max_workers) = task_config_value | ||
| .get("max_workers") | ||
| .and_then(|v| v.as_u64()) | ||
| { | ||
| merged_config.max_workers = max_workers as usize; | ||
| } | ||
| if let Some(initial_workers) = task_config_value | ||
| .get("initial_workers") | ||
| .and_then(|v| v.as_u64()) | ||
| { | ||
| merged_config.initial_workers = initial_workers as usize; | ||
| } | ||
|
|
There was a problem hiding this comment.
You merge max_workers and initial_workers into effective_config but never use them in the execution logic. Either implement worker scaling or remove these fields to avoid dead code.
| if let Some(max_workers) = task_config_value | |
| .get("max_workers") | |
| .and_then(|v| v.as_u64()) | |
| { | |
| merged_config.max_workers = max_workers as usize; | |
| } | |
| if let Some(initial_workers) = task_config_value | |
| .get("initial_workers") | |
| .and_then(|v| v.as_u64()) | |
| { | |
| merged_config.initial_workers = initial_workers as usize; | |
| } |
| if let Some(params) = command_parameters { | ||
| if let Some(params_map) = params.as_object() { | ||
| for (key, value) in params_map { | ||
| let key_str = key.to_string(); | ||
| let value_str = value.as_str().unwrap_or(&value.to_string()).to_string(); | ||
| cmd.arg("--params") | ||
| .arg(format!("{}={}", key_str, value_str)); | ||
| } |
There was a problem hiding this comment.
[nitpick] Consider combining the two if let checks into one, e.g. if let Some(map) = command_parameters.and_then(|v| v.as_object()) { ... }, to reduce nesting.
| if let Some(params) = command_parameters { | |
| if let Some(params_map) = params.as_object() { | |
| for (key, value) in params_map { | |
| let key_str = key.to_string(); | |
| let value_str = value.as_str().unwrap_or(&value.to_string()).to_string(); | |
| cmd.arg("--params") | |
| .arg(format!("{}={}", key_str, value_str)); | |
| } | |
| if let Some(params_map) = command_parameters.and_then(|params| params.as_object()) { | |
| for (key, value) in params_map { | |
| let key_str = key.to_string(); | |
| let value_str = value.as_str().unwrap_or(&value.to_string()).to_string(); | |
| cmd.arg("--params") | |
| .arg(format!("{}={}", key_str, value_str)); |
|
Timeouts for sub-recipes is being handled in this PR: #3274 |
| let result = process_task(task, config.timeout_seconds).await; | ||
|
|
||
| // Extract config from task payload if present and merge with global config | ||
| let effective_config = if let Some(task_config_value) = task.payload.get("config") { |
There was a problem hiding this comment.
can we move this into a merge method on Config? that way if we add more fields, we are more likely to implement merging too. method should take another Config then I think
|
Hi @shazraz In #3274 we decided to remove the default timeout and the tasks won't be run with timeout configurations as the tools have time out. Since we’re holding off on the worker settings and timeout settings for now, we may not end up needing this PR at the moment. |
|
Closing because this is already being handled! |
This PR creates additional configuration options when using subrecipes to allow for finer grained control over their timeout values. This helps solve the issue where subtasks time out and Goose attempts to keep restarting them:
Im not sure the retry actually increases the timeout config though.