fix: preserve OpenAI-compatible alias matching before canonical fallback#3747
fix: preserve OpenAI-compatible alias matching before canonical fallback#3747ht1072 wants to merge 2 commits into
Conversation
|
This pull request targeted The base branch has been automatically changed to |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f374aefb1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return | ||
| } | ||
| provider := strings.TrimSpace(executor.Identifier()) | ||
| provider := executorProviderKey(executor.Identifier()) |
There was a problem hiding this comment.
Because executors are now stored only under the canonical lower-case key here, the auto-refresh path still misses them when it does manager.executors[auth.Provider] in sdk/cliproxy/auth/auto_refresh_loop.go:236. For auths whose provider is mixed/upper case (for example a custom/OpenAI-compatible provider registered as YT), due auto-refresh sees exec == nil and just reschedules, so expiring credentials are never refreshed. Use the same canonical key helper for that lookup as well.
Useful? React with 👍 / 👎.
![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 introduces a helper function executorProviderKey to canonicalize provider keys (by trimming whitespace and converting to lowercase) and refactors various methods in conductor.go to use this helper. It also adds a direct model support check in authSupportsRouteModel and includes a new unit test to verify that pickNextMixedLegacy correctly finds executors using canonical provider keys. The review feedback suggests adding defensive nil checks for the Manager instance in pickNextLegacy and pickNextMixedLegacy to prevent potential nil-pointer panics when accessing mutexes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| provider = executorProviderKey(provider) | ||
| if provider == "" { | ||
| return nil, nil, &Error{Code: "provider_not_found", Message: "no provider supplied"} | ||
| } |
There was a problem hiding this comment.
If the Manager instance m is nil, calling m.HomeEnabled() on line 3163 will return false safely, but the code will then proceed to line 3167 and eventually panic on m.mu.RLock() at line 3175. Adding a defensive nil check for m at the beginning of the legacy path prevents this potential panic.
| provider = executorProviderKey(provider) | |
| if provider == "" { | |
| return nil, nil, &Error{Code: "provider_not_found", Message: "no provider supplied"} | |
| } | |
| if m == nil { | |
| return nil, nil, &Error{Code: "manager_nil", Message: "manager is nil"} | |
| } | |
| provider = executorProviderKey(provider) | |
| if provider == "" { | |
| return nil, nil, &Error{Code: "provider_not_found", Message: "no provider supplied"} | |
| } |
| pinnedAuthID := pinnedAuthIDFromMetadata(opts.Metadata) | ||
| disallowFreeAuth := disallowFreeAuthFromMetadata(opts.Metadata) | ||
|
|
||
| providerSet := make(map[string]struct{}, len(providers)) |
There was a problem hiding this comment.
If the Manager instance m is nil, calling m.HomeEnabled() on line 3303 will return false safely, but the code will then proceed to line 3310 and eventually panic on m.mu.RLock() at line 3328. Adding a defensive nil check for m at the beginning of the legacy path prevents this potential panic.
if m == nil {
return nil, nil, "", &Error{Code: "manager_nil", Message: "manager is nil"}
}
providerSet := make(map[string]struct{}, len(providers))
|
Thanks for the automated review feedback. I pushed a follow-up commit that:
Validation:
GitHub checks are green on the latest commit. |
|
Closing this PR rather than leaving a stale/conflicted branch open. The fix still looks directionally useful, but the branch is now diverged from |
1 participant
Summary
Problem
OpenAI-compatible aliases that include a visible suffix such as
(high)can be listed successfully, but fail during auth selection when the runtime checks support only against canonicalized model keys. If the registry stores the exact alias while selection strips the suffix first, the candidate auth can be filtered out before any upstream request is attempted.Root cause
authSupportsRouteModel()checked canonicalized route keys before considering the exact route model string, while OpenAI-compatible registries can expose the exact alias string as the supported model id. In mixed provider-id casing scenarios, executor lookup also benefits from using the same normalization helper everywhere.Fix
executorProviderKey()and use it consistently for executor register/lookup/unregister and legacy picker pathsauthSupportsRouteModel()try the rawrouteModelfirst, then fall back to canonical/selection keysIdentifier() == "YT", auth provideryt, and route modelYT-GPT-5.5(high)Validation