fix: recipe extensions ignored when run from UI and scheduler#6438
fix: recipe extensions ignored when run from UI and scheduler#6438Abhijay007 wants to merge 1 commit intoblock:mainfrom
Conversation
Signed-off-by: Abhijay007 <Abhijay007j@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where recipe extensions were being ignored when recipes were run from the UI and scheduler. The fix ensures that recipe extensions are properly saved to the session's extension_data so they can be loaded correctly.
Changes:
- Added logic to merge recipe extensions with global/override extensions in the agent start flow
- Added code to save recipe extensions to session extension_data in the scheduler execution flow
- Imported necessary types for extension handling
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/goose/src/scheduler.rs | Added import for extension types and logic to save recipe extensions to session extension_data during scheduled job execution |
| crates/goose-server/src/routes/agent.rs | Modified agent start logic to merge recipe extensions with global/override extensions before saving to session |
| // Save recipe to session | ||
| SessionManager::update_session(&session.id) | ||
| .recipe(Some(recipe.clone())) | ||
| .apply() | ||
| .await?; | ||
|
|
There was a problem hiding this comment.
The recipe is being saved to the session twice - once here and again at lines 837-841. Remove this duplicate update since line 839 already saves the recipe to the session.
| // Save recipe to session | |
| SessionManager::update_session(&session.id) | |
| .recipe(Some(recipe.clone())) | |
| .apply() | |
| .await?; |
| .extension_data(extension_data.clone()) | ||
| .apply() | ||
| .await?; | ||
| session.extension_data = extension_data; |
There was a problem hiding this comment.
After updating extension_data in the database, you're also updating the local session object with the same data. However, this local mutation doesn't persist since the session object isn't used further in this function - it's only referenced once more to get the id on line 772. Consider removing this line as it serves no purpose.
| session.extension_data = extension_data; |
|
Hi @Abhijay007, Thanks for the contribution! This looks good as a short-term fix to unblock the issue. For recipe extensions, we still have some inconsistencies across CLI, Desktop, and Scheduler. It would be great if we could align on a single resolution across all three. Current inconsistent behaviour when starting a new session:
I am thinking we could unify the logic and use the CLI behaviour as the source of truth, implemented in a centralized place. I’ve started a draft PR (still WIP) to explore: #6464. WDYT? |
|
thanks @lifeizhou-ap - that makes a lot of sense to me! |
|
Hi @Abhijay007, I have merged PR #6464 to main branch, and it fixes issue #6023. Just wondering whether you would like to close this PR? |
I will resolve this one asap, I think the one you merged is for cli and this one is for UI ? |
The one I merged is also for Desktop, now CLI, Desktop and Scheduler shared the same logic to load the recipe extensions |
Oh so we can close this one ig |
3 participants
Closes: #6023
PR Description
This PR fixes a bug where recipe extensions were being ignored when recipes were run from the UI and scheduler. The fix ensures that recipe extensions are properly saved to the session's extension_data so they can be loaded correctly.
Type of Change
AI Assistance
Testing
Screenshots/Demos (for UX changes)
Before:
After: