fix: strip -customtools variant suffix for provider and auth lookup#1958
fix: strip -customtools variant suffix for provider and auth lookup#1958thebtf wants to merge 1 commit intorouter-for-me:mainfrom
Conversation
Google's Gemini CLI appends "-customtools" to model names for a behavioral fine-tune that prioritizes custom tools over bash. CPA does not recognize these suffixed names, causing 502 errors. Add stripModelVariantSuffix() at two points: 1. getRequestDetails (handlers.go) — provider lookup fallback 2. ClientSupportsModel (model_registry.go) — auth selection fallback The suffix is stripped only when the original name yields no matches, preserving existing behavior for standard model names.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where Google's Gemini CLI appends a Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly identifies and addresses an issue with model names having a -customtools suffix. The approach of stripping the suffix as a fallback is sound. However, the implementation introduces code duplication for the stripping logic across two different packages. Additionally, there is a critical bug in sdk/api/handlers/handlers.go where the suffix stripping fails for model names that also include a thinking suffix. I've provided comments with suggestions to address these issues.
| if len(providers) == 0 { | ||
| if stripped := stripModelVariantSuffix(baseModel); stripped != baseModel { | ||
| providers = util.GetProviderName(stripped) | ||
| resolvedModelName = stripModelVariantSuffix(resolvedModelName) |
There was a problem hiding this comment.
There's a potential bug here. The stripModelVariantSuffix function uses strings.HasSuffix to find and remove the variant suffix. If resolvedModelName contains a thinking suffix (e.g., (8192)), strings.HasSuffix will fail to find the variant suffix (e.g., -customtools), and resolvedModelName will not be stripped as intended. This would cause issues with providers that don't accept variant suffixes.
A more robust way to strip the variant suffix while preserving the thinking suffix would be to replace the baseModel part within resolvedModelName with the stripped version.
| resolvedModelName = stripModelVariantSuffix(resolvedModelName) | |
| resolvedModelName = strings.Replace(resolvedModelName, baseModel, stripped, 1) |
| // knownModelVariantSuffixes lists suffixes that Google appends to model names | ||
| // for specialised fine-tune variants (e.g. "-customtools" for tool-calling). | ||
| // These variants share the same provider/executor as the base model. | ||
| var knownModelVariantSuffixes = []string{"-customtools"} | ||
|
|
||
| func stripModelVariantSuffix(model string) string { | ||
| for _, suffix := range knownModelVariantSuffixes { | ||
| if strings.HasSuffix(model, suffix) { | ||
| return strings.TrimSuffix(model, suffix) | ||
| } | ||
| } | ||
| return model | ||
| } |
There was a problem hiding this comment.
This helper function and the knownModelVariantSuffixes variable are also defined in sdk/api/handlers/handlers.go. To avoid code duplication and improve maintainability, they should be moved to a shared package. The internal/thinking/suffix.go file seems like a suitable location, as it already handles model name suffix parsing. You would then need to add an import for internal/thinking in this file and use the shared function.
| // knownModelVariantSuffixes lists suffixes that Google appends to model names | ||
| // for specialised fine-tune variants (e.g. "-customtools" for tool-calling). | ||
| // These variants share the same provider/executor as the base model. | ||
| var knownModelVariantSuffixes = []string{"-customtools"} | ||
|
|
||
| func stripModelVariantSuffix(model string) string { | ||
| for _, suffix := range knownModelVariantSuffixes { | ||
| if strings.HasSuffix(model, suffix) { | ||
| return strings.TrimSuffix(model, suffix) | ||
| } | ||
| } | ||
| return model | ||
| } |
There was a problem hiding this comment.
This helper function and the knownModelVariantSuffixes variable are duplicated from internal/registry/model_registry.go. To avoid code duplication and improve maintainability, they should be moved to a shared package like internal/thinking/suffix.go. Since this file already imports the thinking package, you can remove this duplicated code and call the function from there.
xkonjin
left a comment
xkonjin
left a comment
There was a problem hiding this comment.
Code Review
Overall:
What's good
- Fallback approach (strip suffix → retry provider lookup) is the right pattern instead of adding duplicates to model lists.
- Stripping from both and is correct — OAuth routes reject suffixed names, so the resolved name must be stripped too.
- fallback in registry is clean and isolated.
Confirmed bug (flagged by gemini-code-assist, needs fix)
**Ordering of suffix stripping for thinking models in ** — if a model name has both a thinking suffix and -customtools (e.g. gemini-2.0-flash-thinking-exp-customtools), stripModelVariantSuffix only strips -customtools, leaving gemini-2.0-flash-thinking-exp. That's correct. But if the suffix ordering is reversed (e.g. gemini-2.0-flash-customtools-thinking), the function returns the model unchanged because HasSuffix for -customtools fails. This is a latent ordering fragility.
The bigger issue: resolvedModelName may already have been transformed (thinking suffix stripped, version pinned, etc.) by the time we call stripModelVariantSuffix on it. Stripping from resolvedModelName without knowing its current form could produce an incorrect base name. Safer: strip from baseModel only for provider lookup, and strip from modelID input in registry — don't mutate resolvedModelName in the fallback path.
Code duplication
The knownModelVariantSuffixes var and stripModelVariantSuffix function are copy-pasted verbatim between internal/registry/model_registry.go and sdk/api/handlers/handlers.go. Move to a shared util package and import in both.
Non-blocking
stripModelVariantSuffixiterates the suffix list on every call. With one entry this is negligible, but if the suffix list grows, pre-compile with a regex or strings.Replacer.- Consider a unit test with a thinking+customtools composite model name to lock in the expected behavior.
xkonjin
left a comment
xkonjin
left a comment
There was a problem hiding this comment.
Code Review
Overall:
What's good
- Fallback approach (strip suffix, retry provider lookup) is the right pattern instead of adding duplicates to model lists.
- Stripping from both baseModel and resolvedModelName is correct — OAuth routes reject suffixed names, so the resolved name must be stripped too.
- ClientSupportsModel fallback in registry is clean and isolated.
Confirmed bug (flagged by gemini-code-assist, needs fix)
Ordering of suffix stripping for thinking models in handlers.go — if a model name has both a thinking suffix and -customtools (e.g. gemini-2.0-flash-thinking-exp-customtools), stripModelVariantSuffix only strips -customtools, leaving gemini-2.0-flash-thinking-exp. That's correct for this ordering. But if the suffix ordering is reversed (e.g. gemini-2.0-flash-customtools-thinking), the function returns the model unchanged because HasSuffix for -customtools fails. Latent ordering fragility.
Bigger issue: resolvedModelName may already have been transformed (thinking suffix stripped, version pinned, etc.) before we call stripModelVariantSuffix on it. Stripping from resolvedModelName without knowing its current form could produce an incorrect base name. Safer: strip from baseModel only for provider lookup; strip from the raw modelID in registry — don't mutate resolvedModelName in the fallback path.
Code duplication
knownModelVariantSuffixes and stripModelVariantSuffix are copy-pasted verbatim between internal/registry/model_registry.go and sdk/api/handlers/handlers.go. Move to a shared util package and import in both.
Non-blocking
- stripModelVariantSuffix iterates the suffix list on every call. With one entry this is negligible, but if the list grows, consider pre-compiling.
- Add a unit test with a thinking+customtools composite model name to lock in the expected behavior and catch future regressions.
xkonjin
left a comment
xkonjin
left a comment
There was a problem hiding this comment.
This fixes the simple *-customtools case, but it still misses one important path: models with a thinking suffix.
In getRequestDetails(), provider lookup is done against baseModel, so gemini-foo-customtools(8192) can now resolve its provider. But the code then normalizes the outbound model with stripModelVariantSuffix(resolvedModelName). That call is against the full string, so when resolvedModelName still has a trailing thinking suffix, HasSuffix("-customtools") is false and the variant suffix is preserved. That means the downstream CPA/cloudcode-pa request can still receive the unsupported -customtools model name and 404.
I think the fix needs to strip the variant on the parsed model portion, then re-attach any thinking suffix, and it needs a regression test for something like ...-customtools(8192). Without that, the most failure-prone path still slips through.
luispater
left a comment
luispater
left a comment
There was a problem hiding this comment.
Summary:
The main fallback direction looks right, but there is still one correctness gap before this is safe to merge.
Key findings:
- Blocking:
stripModelVariantSuffix()only works when-customtoolsis the terminal suffix. We already support combined forms such asgemini-...-customtools(high)/(auto), so those requests can still flow downstream with the variant suffix intact and still miss provider/auth lookup. Please add regression coverage for the combined-suffix case and normalize that path as well. - Non-blocking:
/v1/responseswebsocket capability probing still uses the old provider/model lookup path, so suffixed models there still take inconsistent fallback behavior.
Test plan:
- go test ./...
Follow-up:
- Reuse the same normalization helper across the request path, auth selection path, and websocket capability probe so the suffix handling stays consistent.
3 participants
Summary
-customtoolsto model names for a behavioral fine-tune variant. CPA doesn't recognize these suffixed names, causing 502 errors for all Gemini CLI users when this suffix is active.stripModelVariantSuffix()fallback at two points:getRequestDetails()inhandlers.go— provider lookupClientSupportsModel()inmodel_registry.go— auth selectionFiles changed
sdk/api/handlers/handlers.go— variant suffix stripping in provider lookupinternal/registry/model_registry.go— variant suffix stripping in client model support checkTest plan
gemini-2.5-pro-preview-05-06-customtoolsthrough CPA