-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Fix: Recipe Extensions Not Loading in Desktop #6777
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
…ensions * 'main' of github.com:block/goose: chore: re-sync package-lock.json (#6783) upgrade electron to 39.3.0 (#6779) allow skipping providers in test_providers.sh (#6778) fix: enable custom model entry for OpenRouter provider (#6761) Remove codex skills flag support (#6775) Improve mcp test (#6671) Feat/anthropic custom headers (#6774) Fix/GitHub copilot error handling 5845 (#6771) fix(ui): respect width parameter in MCP app size-changed notifications (#6376) fix: address compilation issue in main (#6776)
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.
Pull request overview
Fixes goose Desktop sessions started from recipes to correctly include the UI-enabled extensions by merging recipe-specified extensions with the per-session extension overrides instead of letting recipe extensions fully replace UI overrides.
Changes:
- Initialize new sessions’ enabled extensions from
extension_overrides(UI settings) and then merge in any recipe-required extensions not already present by name. - Ensures recipe-required extensions are added while preserving the UI-selected extension config when names overlap.
|
I still don't understand the issue. this seems to be like the correct behavior. if the recipe specifies extensions, we should use those. if the recipe specifies an empty list, we should run with no extensions. if a recipe does not specify extensions, then we should use the one that the system has. /cc @lifeizhou-ap |
but it's not using the ones the system has. it loads as 0 extensions enabled and then is unable to run |
|
I discussed it with @DOsinga gonna take a different approach and handle it in the desktop code. The main issue is desktop is writing an empty extensions array rather than no extensions. |
Yes! I noticed this late yesterday when testing the sub recipe feature. When user creates recipe via UI it includes "extensions: []" in the file. Previously it does not matter in Desktop because Desktop ignores the extensions in the recipe. Since we unified the recipe extension behaviour in Desktop, CLI and Scheduler recently, Desktop user has different experiences now. I was about fixing this today. Thanks @zanesq for fixing this |
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.
can do with some cleaning of LLM comments
ui/desktop/src/recipe/index.ts
Outdated
| * This ensures recipes without explicit extensions will use the default enabled extensions | ||
| * from the user's config in the desktop rather than loading with no extensions. | ||
| */ | ||
| export function stripEmptyExtensions<T extends { extensions?: unknown[] | null }>(recipe: T): T { |
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 this complicated type? shouldn't this just be Recipe in, Recipe out? that would save us some casting above too
| } | ||
|
|
||
| function recipeToYaml(recipe: Recipe): string { | ||
| // Strip empty extensions to ensure scheduled recipes use default enabled extensions |
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, we discussed this, that's not how the scheduler works
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.
Yes, scheduler always respect the extension config in recipe, not like Desktop. Maybe we should not apply this stripEmptyExtensions here.
|
|
||
| if (recipe.extensions && recipe.extensions.length > 0) { | ||
| cleanRecipe.extensions = recipe.extensions.map((ext) => { | ||
| if (processedRecipe.extensions && processedRecipe.extensions.length > 0) { |
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.
and we're stripping it anyway here, so this feels like a no op
| setIsCreating(true); | ||
| try { | ||
| // Create the recipe object from form data | ||
| // Don't set extensions - let the recipe use default enabled extensions |
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.
typical LLM comment
| ) as ExtensionConfig[]; | ||
|
|
||
| // Use stripEmptyExtensions to avoid saving empty extensions array | ||
| // Empty extensions array would prevent default extensions from loading |
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.
do we? we do want to make the distinction between empty lists and no recipes. if you now load a recipe that pre-existed and had an empty list of extensions, and you save it, it now has no extensions.
you want to make sure that if we create a recipe the default extensions are undefined and then respect whatever it said already
| return recipe.extensions; | ||
| } | ||
| return []; | ||
| const [recipeExtensions] = useState<ExtensionConfig[] | undefined>(() => { |
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 great! I was about suggesting this too! You are quick :)
* 'main' of github.com:block/goose: (62 commits) Swap canonical model from openrouter to models.dev (#6625) Hook thinking status (#6815) Fetch new skills hourly (#6814) copilot instructions: Update "No prerelease docs" instruction (#6795) refactor: centralize audience filtering before providers receive messages (#6728) update doc to remind contributors to activate hermit and document minimal npm and node version (#6727) nit: don't spit out compaction when in term mode as it fills up the screen (#6799) fix: correct tool support detection in Tetrate provider model fetching (#6808) Session manager fixes (#6809) fix(desktop): handle quoted paths with spaces in extension commands (#6430) fix: we can default gooseignore without writing it out (#6802) fix broken link (#6810) docs: add Beads MCP extension tutorial (#6792) feat(goose): add support for AWS_BEARER_TOKEN_BEDROCK environment variable (#6739) [docs] Add OSS Skills Marketplace (#6752) feat: make skills available in codemode (#6763) Fix: Recipe Extensions Not Loading in Desktop (#6777) Different approach to determining final confidence level of prompt injection evaluation outcomes (#6729) fix: read_resource_tool deadlock causing test_compaction to hang (#6737) Upgrade error handling (#6747) ...
…sion-session * 'main' of github.com:block/goose: (78 commits) copilot instructions: Update "No prerelease docs" instruction (#6795) refactor: centralize audience filtering before providers receive messages (#6728) update doc to remind contributors to activate hermit and document minimal npm and node version (#6727) nit: don't spit out compaction when in term mode as it fills up the screen (#6799) fix: correct tool support detection in Tetrate provider model fetching (#6808) Session manager fixes (#6809) fix(desktop): handle quoted paths with spaces in extension commands (#6430) fix: we can default gooseignore without writing it out (#6802) fix broken link (#6810) docs: add Beads MCP extension tutorial (#6792) feat(goose): add support for AWS_BEARER_TOKEN_BEDROCK environment variable (#6739) [docs] Add OSS Skills Marketplace (#6752) feat: make skills available in codemode (#6763) Fix: Recipe Extensions Not Loading in Desktop (#6777) Different approach to determining final confidence level of prompt injection evaluation outcomes (#6729) fix: read_resource_tool deadlock causing test_compaction to hang (#6737) Upgrade error handling (#6747) Fix/filter audience 6703 local (#6773) chore: re-sync package-lock.json (#6783) upgrade electron to 39.3.0 (#6779) ...
Summary: Fix desktop recipes not using default extensions
Problem
Recipes with an empty
extensions: []array would causeresolve_extensions_for_new_sessionto return no extensions, instead of falling back to the user's default enabled extensions.
Solution
Strip empty extension arrays in the frontend before saving/running recipes.
Files Changed
ui/desktop/src/recipe/index.ts
stripEmptyExtensions()utility function that removes theextensionspropertyif it's an empty array
ui/desktop/src/components/recipes/RecipesView.tsx
handleStartRecipeChat(): Strip empty extensions in memory before running a reciperecipeIdparameterui/desktop/src/components/recipes/CreateEditRecipeModal.tsx
getCurrentRecipe(): UsestripEmptyExtensions()when building recipe to saveui/desktop/src/components/recipes/CreateRecipeFromSessionModal.tsx
handleCreateRecipe(): Simply don't setextensionsat all (instead of setting[])ui/desktop/src/components/schedule/ScheduleModal.tsx
recipeToYaml(): Strip empty extensions before converting recipe to YAML for schedulingHow it works
extensions: [], it gets stripped toextensions: undefinedresolve_extensions_for_new_session(None, override_extensions)then fallsthrough to use the UI's enabled extensions (desktop) or
get_enabled_extensions()(scheduler)