-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix : preserve provider engine type when editing custom providers #6106
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
Signed-off-by: Abhijay007 <[email protected]>
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
This PR attempts to fix an issue where the Provider Type dropdown in the custom provider edit form always defaults to "OpenAI Compatible" regardless of the provider's actual engine type. However, the PR introduces critical bugs that will break custom provider creation and updates.
Key Issue
The PR misidentifies the root cause. The backend has an architectural inconsistency:
- Backend returns (via
GET /config/custom-providers/{id}):engine: "openai"(lowercase, no suffix) - Backend expects (via
POST/PUTto custom-providers endpoints):engine: "openai_compatible"(with_compatiblesuffix)
The old code correctly handled this with an engineMap that converted between formats. The PR removes this conversion, causing the frontend to send "openai" when the backend expects "openai_compatible", resulting in "Invalid provider type" errors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.tsx |
Removes engine value conversion mapping and changes dropdown options/values to use lowercase format without _compatible suffixes. This breaks API compatibility with the backend's create/update endpoints. |
ui/desktop/src/components/settings/providers/ProviderGrid.tsx |
Removes redundant .toLowerCase() call when passing engine value (harmless change since values are already lowercase). |
ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.tsx
Outdated
Show resolved
Hide resolved
ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.tsx
Outdated
Show resolved
Hide resolved
ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.tsx
Outdated
Show resolved
Hide resolved
ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Abhijay007 <[email protected]>
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.
the description of the PR says that this is due to a mismatch of the backend and the frontend and the _compatible suffix, but what the PR does is remove the toLowercase (which seems reasonable) - is this what you wanted to do?
Oh yes, that was the issue I initially suspected. However, I received a review from Copilot mentioning that this change would break the backend, as the backend would reject it with an “Invalid provider type” error. I looked into it further and found that the problem was due to a lowercase mismatch, the values were being sent in lowercase, which caused the backend validation to fail. Because of this, I reverted those changes and pushed a fix. For reference, you can check the Copilot review comments. |
|
hmm, not sure I follow still; does the backend use different values from what it sends and what it receives? if that is the case, can't we just fix that in the backend? |
The issue is that the backend expects engine values with the If we change these values to I revisited the issue and found that the reason the provider was defaulting back incorrectly was not due to the suffix, but because the value was being converted to lowercase, so instead of making changes in the backend (which may have affected other things) I changed the lowercase value conversion. This caused a mismatch, making the backend think it was receiving an invalid engine value. As a result, the UI fell back to showing the engine type from which the custom provider was originally created. In short, we don’t need to update or remove any suffix values. The fix was simply to remove the lowercase conversion that was causing the mismatch and backend validation error in provider grid and provider form. |
|
but can't we change the backend so it accepts this too? |
I don't think that is needed, as that might affect the backend api response, which might be getting used at other places too, and if we update that for this change, we might need to look into that as well, and the current change will not affect it |
alexhancock
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.
description seems a bit out of sync with the changes, but the changes look fine to make to me
* 'main' of github.com:block/goose: Claude 3.7 is out. we had some harcoded stuff (#6197) Release 1.19.0 chore: upgrade to node v24 as engine (#6361) chore(deps): bump rsa from 0.9.9 to 0.9.10 (#6358) Bump rust toolchain to 1.92 (current stable) (#6356) Hide advanced recipe options under expandable content (#6021) fix: use .config/agents (plural) for skills directory (#6357) fix: prevent KaTeX from treating underscores as subscripts in plain text (#6242) fix: make goose review PRs more like goose contributors do (#6240) fix : preserve provider engine type when editing custom providers (#6106) feat(providers): add retry for model fetching (#6347) allow goose issue solver to react to activation comments (#6239)
Closes: #6046
PR Description
When editing a custom provider in Goose Desktop, the Provider Type dropdown always defaulted to OpenAI Compatible, regardless of the provider’s actual engine (Anthropic or Ollama). Saving the form without reselecting the correct type overwrote the engine to
openai, resulting in an incorrect configuration.This issue was caused by a mismatch between frontend engine values (using
_compatiblesuffixes) and the backend’s expected engine values.Changes Made
file modified:
ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.tsxopenai,anthropic,ollama)initialData.engineuseEffectfile modified:
ui/desktop/src/components/settings/providers/ProviderGrid.tsx.toLowerCase()call when passing the engine valueType of Change
Testing
Screenshots / Demos
Before:
After: