fix: ignore deprecated skills extension#7139
Conversation
Signed-off-by: anonwurcod <anonwurcod@proton.me> Co-Authored-By: Warp <agent@warp.dev>
| } else { | ||
| resolve_extensions_for_new_session(recipe.and_then(|r| r.extensions.as_deref()), None) | ||
| }; | ||
| configured_extensions.retain(|config| !is_deprecated_platform_extension(config)); |
There was a problem hiding this comment.
shouldn't we be doing this at a higher level? this fixes it for the cli, but how does this work for the desktop?
I think in general we should just ignore trying to load platform extensions that don't exist. we offer a let goose help you fix this for you, but for platform extensions and built-in extensions there is just nothing you can do
There was a problem hiding this comment.
question 1: what is currently implemented makes it CLI-specific for new or resumed sessions. If the desktop app has a parallel session builder it wouldn't directly benefit from this filter—though the migration should still mitigate the issue by cleaning the config upfront
question 2: Regarding a higher-level approach: agreed, This could be handled in the shared extension resolver/loader (e.g., in crates/goose/src/agents/extension.rs or wherever PLATFORM_EXTENSIONS is checked during startup). That would uniformly cover CLI, desktop, and any future interfaces
i can put in a quick fix for #2 if you wish
There was a problem hiding this comment.
Yeah, I think all we should do is just #2 - don't load platform extensions that are not present in the system. this doesn't seem like a good candidate for migrations. maybe I am just temporarily on a new version. if you wanted to do a migration it should also be done in or using the extensions.rs - you are now duplicating get_extensions_map_with_config. you could apply your filtere there. but like I said, my preference would be to skip any platform extension that cannot be loaded in extension manager.
There was a problem hiding this comment.
@DOsinga my commits touch .github/workflows/canary.yml which triggers an auth error when i try to push the changes required to fix the merge conflicts, how to resolve?
Co-Authored-By: Warp <agent@warp.dev> Signed-off-by: anonwurcod <anonwurcod@proton.me>
Co-Authored-By: Warp <agent@warp.dev> Signed-off-by: anonwurcod <anonwurcod@proton.me>
not sure I follow, how does it touch the canary.yml? if you somehow get into trouble, I can always branch of this and get it landed for you |
Co-Authored-By: Warp <agent@warp.dev> Signed-off-by: anonwurcod <anonwurcod@proton.me>
|
@DOsinga should be good now if you wish to review |
DOsinga
left a comment
There was a problem hiding this comment.
I think builder.rs & migrations.rs have no empty diffs and git is not happy about it?
Co-Authored-By: Warp <agent@warp.dev> Signed-off-by: anonwurcod <anonwurcod@proton.me>
|
thanks for the quick turnaround! |
|
np! |
* origin/main: (42 commits) fix: use dynamic port for Tetrate auth callback server (#7228) docs: removing LLM Usage admonitions (#7227) feat(otel): respect standard OTel env vars for exporter selection (#7144) fix: fork session (#7219) Bump version numbers for 1.24.0 release (#7214) Move platform extensions into their own folder (#7210) fix: ignore deprecated skills extension (#7139) Add a goosed over HTTP integration test, and test the developer tool PATH (#7178) feat: add onFallbackRequest handler to McpAppRenderer (#7208) feat: add streaming support for Claude Code CLI provider (#6833) fix: The detected filetype is PLAIN_TEXT, but the provided filetype was HTML (#6885) Add prompts (#7212) Add testing instructions for speech to text (#7185) Diagnostic files copying (#7209) fix: allow concurrent tool execution within the same MCP extension (#7202) fix: handle missing arguments in MCP tool calls to prevent GUI crash (#7143) Filter Apps page to only show standalone Goose Apps (#6811) opt: use static for Regex (#7205) nit: show dir in title, and less... jank (#7138) feat(gemini-cli): use stream-json output and re-use session (#7118) ...
Summary
Reproduction
Testing
Fixes #7134