fix(auth): resolve oauth aliases before suspension checks#2441
fix(auth): resolve oauth aliases before suspension checks#2441luispater merged 2 commits intorouter-for-me:devfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07b7c1a1e0
ℹ️ 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".
| m.mu.RUnlock() | ||
| return nil, nil, "", errAvailable | ||
| } | ||
| selected, errPick := m.selector.Pick(ctx, "mixed", "", opts, available) |
There was a problem hiding this comment.
Forward route model to selector in legacy pick paths
pickNextLegacy/pickNextMixedLegacy now call selector.Pick with an empty model string after availableAuthsForRouteModel prefilters by alias-resolved model. For built-in selectors, this triggers a second availability check via getAvailableAuths(..., model=""), which falls back to aggregate auth-level blocking (isAuthBlockedForModel with empty model) and can reject credentials that were just deemed usable for the resolved alias model. A concrete case is an auth with auth.Unavailable=true from a cooled-down route model state but a healthy alias target: prefilter admits it, then selector drops it and still returns auth_unavailable, so the alias-suspension fix is bypassed in real cooldown states.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request introduces route-aware model selection and aliasing logic to the Manager. Key changes include new methods for resolving model aliases, determining execution state models, and filtering authentication candidates by priority and blocking status. The scheduler's picking logic is updated to handle these aliases, with a fallback to legacy paths when route-aware selection is required. Review feedback suggests simplifying the alias resolution logic, adopting idiomatic Go loops, and deduplicating model support checks into a helper method to improve maintainability.
| resolvedModel := m.applyOAuthModelAlias(auth, requestedModel) | ||
| if strings.TrimSpace(resolvedModel) == "" { | ||
| resolvedModel = requestedModel | ||
| } | ||
| return resolvedModel |
There was a problem hiding this comment.
This block can be simplified for better readability and conciseness, while maintaining the same logic.
| resolvedModel := m.applyOAuthModelAlias(auth, requestedModel) | |
| if strings.TrimSpace(resolvedModel) == "" { | |
| resolvedModel = requestedModel | |
| } | |
| return resolvedModel | |
| if resolvedModel := m.applyOAuthModelAlias(auth, requestedModel); strings.TrimSpace(resolvedModel) != "" { | |
| return resolvedModel | |
| } | |
| return requestedModel |
| for i := 0; i < len(auths); i++ { | ||
| candidate := auths[i] | ||
| checkModel := m.selectionModelForAuth(candidate, routeModel) | ||
| blocked, reason, next := isAuthBlockedForModel(candidate, checkModel, now) | ||
| if !blocked { | ||
| priority := authPriority(candidate) | ||
| availableByPriority[priority] = append(availableByPriority[priority], candidate) | ||
| continue | ||
| } | ||
| if reason == blockReasonCooldown { | ||
| cooldownCount++ | ||
| if !next.IsZero() && (earliest.IsZero() || next.Before(earliest)) { | ||
| earliest = next | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This for loop can be converted to a for...range loop, which is more idiomatic in Go and improves readability.
for _, candidate := range auths {
checkModel := m.selectionModelForAuth(candidate, routeModel)
blocked, reason, next := isAuthBlockedForModel(candidate, checkModel, now)
if !blocked {
priority := authPriority(candidate)
availableByPriority[priority] = append(availableByPriority[priority], candidate)
continue
}
if reason == blockReasonCooldown {
cooldownCount++
if !next.IsZero() && (earliest.IsZero() || next.Before(earliest)) {
earliest = next
}
}
}| if modelKey != "" && registryRef != nil { | ||
| supportsModel := registryRef.ClientSupportsModel(candidate.ID, modelKey) | ||
| if !supportsModel { | ||
| selectionKey := canonicalModelKey(m.selectionModelForAuth(candidate, model)) | ||
| if selectionKey != "" && selectionKey != modelKey { | ||
| supportsModel = registryRef.ClientSupportsModel(candidate.ID, selectionKey) | ||
| } | ||
| } | ||
| if !supportsModel { | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of logic for checking model support with aliases is duplicated in pickNextMixedLegacy (lines 2526-2537). To improve maintainability and reduce code duplication, consider extracting this logic into a helper method. For example:
func (m *Manager) clientSupportsModelWithAlias(candidate *Auth, model, modelKey string, registryRef *registry.ModelRegistry) bool {
if modelKey == "" || registryRef == nil {
return true // No model to check, or no registry to check against
}
if registryRef.ClientSupportsModel(candidate.ID, modelKey) {
return true
}
selectionKey := canonicalModelKey(m.selectionModelForAuth(candidate, model))
if selectionKey != "" && selectionKey != modelKey {
return registryRef.ClientSupportsModel(candidate.ID, selectionKey)
}
return false
}You could then replace this block with a call to the new helper:
if !m.clientSupportsModelWithAlias(candidate, model, modelKey, registryRef) { continue }
|
Follow-up pushed in This keeps the same fix but tightens the implementation in two places:
Verification rerun after the refactor:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f611dd6e96
ℹ️ 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".
| selectionModel := m.selectionModelForAuth(auth, routeModel) | ||
| if canonicalModelKey(selectionModel) == canonicalModelKey(upstreamModel) && strings.TrimSpace(selectionModel) != "" { | ||
| return strings.TrimSpace(upstreamModel) |
There was a problem hiding this comment.
Keep prefixed route model when recording execution state
stateModelForExecution now derives the state key from selectionModelForAuth, which calls rewriteModelForAuth and strips auth.Prefix; for requests like team-a/model-x, this makes result.Model become model-x instead of the routed team-a/model-x. MarkResult uses result.Model to call registry suspension/quota APIs, so prefixed registrations (added via applyModelPrefixes in sdk/cliproxy/service.go) are no longer updated, and registry consumers can keep seeing the prefixed route model as available even while that route is actually in cooldown/error.
Useful? React with 👍 / 👎.
luispater
left a comment
There was a problem hiding this comment.
Summary
- Fixes OAuth alias routing so auth availability / suspension checks and execution-state handling use the alias-resolved upstream model (avoids
auth_unavailablewhen the route model is blocked but the alias target is healthy). - Adds a focused regression test for the reported scenario.
- Refactors/dedupes route-aware model support checks.
What I liked
availableAuthsForRouteModelchecksselectionModelForAuth(...)beforeisAuthBlockedForModel(...), which directly addresses the bug.stateModelForExecution(...)keeps model-state tracking aligned with the actual upstream model when alias routing changes the model.- Regression test
TestManagerExecute_OAuthAliasBypassesBlockedRouteModelis deterministic and validates both payload and executor model.
Non-blocking notes
- Please confirm the intent of passing
""as the selector model arg for built-in selectors in legacy fallback (cursor becomes provider-scoped). - The scheduler fast-path fallback adds an O(n) scan over
m.authsper request whenmodel != ""; might be worth caching if n is large.
Test plan
- go test ./sdk/cliproxy/auth -run TestManagerExecute_OAuthAliasBypassesBlockedRouteModel -count=1
- go test ./...
This is an automated Codex review result and still requires manual verification by a human reviewer.
Summary:
Why:
auth_unavailable: no auth availablewhen the client-visible route model was suspended, even though the alias target model was healthyVerification:
Closes #2421