Repository navigation
fix(sse): skip disabled providers in combo fallback (404 instead of 400) - #4500
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![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 ports a change from upstream to return a 404 NOT_FOUND status instead of 400 BAD_REQUEST when no active credentials are found for a provider, allowing combo routing to fall through to subsequent targets. The log level has also been downgraded from error to warning, and all relevant integration and unit tests have been updated. The reviewer feedback is highly accurate and valuable, pointing out that the newly added comments in chatHelpers.ts and chat-helpers.test.ts incorrectly describe how the 404 status code is processed by the fallback engine. Updating these comments to correctly reference the status_404 rule in errorConfig.ts will prevent future maintainer confusion.
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.
| // Ported from upstream decolua/9router#336 (Ibrahim Ryan): surface as 404 | ||
| // NOT_FOUND instead of 400 BAD_REQUEST so combo routing can fall through to | ||
| // the next target. The combo target loop (open-sse/services/combo.ts) treats | ||
| // 400 as a hard stop to break body-specific infinite fallback loops | ||
| // (PR #4316 / issue #4279). 404 flows through checkFallbackError as | ||
| // `shouldFallback: true` (generic-error catch-all path in | ||
| // open-sse/services/accountFallback.ts), letting a combo like | ||
| // `antigravity/opus → github/opus` skip a provider whose credentials are | ||
| // all disabled. log level is `warn` rather than `error` because zero active | ||
| // credentials is an expected operator-driven state, not a server fault. |
There was a problem hiding this comment.
The comment states that 404 flows through the generic-error catch-all path in open-sse/services/accountFallback.ts (lines 1593-1599). However, checkFallbackError actually matches the status_404 rule defined in ERROR_RULES (in open-sse/config/errorConfig.ts line 156) via findMatchingErrorRule, which returns shouldFallback: true with a 2-minute cooldown (COOLDOWN_MS.notFound).
While this doesn't negatively affect the behavior (since there are no active credentials to apply a cooldown to anyway), we should correct the comment to avoid misleading future maintainers about how the 404 status code is routed through the fallback engine.
| // Ported from upstream decolua/9router#336 (Ibrahim Ryan): surface as 404 | |
| // NOT_FOUND instead of 400 BAD_REQUEST so combo routing can fall through to | |
| // the next target. The combo target loop (open-sse/services/combo.ts) treats | |
| // 400 as a hard stop to break body-specific infinite fallback loops | |
| // (PR #4316 / issue #4279). 404 flows through checkFallbackError as | |
| // `shouldFallback: true` (generic-error catch-all path in | |
| // open-sse/services/accountFallback.ts), letting a combo like | |
| // `antigravity/opus → github/opus` skip a provider whose credentials are | |
| // all disabled. log level is `warn` rather than `error` because zero active | |
| // credentials is an expected operator-driven state, not a server fault. | |
| // Ported from upstream decolua/9router#336 (Ibrahim Ryan): surface as 404 | |
| // NOT_FOUND instead of 400 BAD_REQUEST so combo routing can fall through to | |
| // the next target. The combo target loop (open-sse/services/combo.ts) treats | |
| // 400 as a hard stop to break body-specific infinite fallback loops | |
| // (PR #4316 / issue #4279). 404 flows through checkFallbackError as | |
| // `shouldFallback: true` (matching the status_404 rule in | |
| // open-sse/config/errorConfig.ts), letting a combo like | |
| // `antigravity/opus → github/opus` skip a provider whose credentials are | |
| // all disabled. log level is `warn` rather than `error` because zero active | |
| // credentials is an expected operator-driven state, not a server fault. |
| // Ported from upstream decolua/9router#336 (Ibrahim Ryan): when a provider has | ||
| // zero usable connections (all disabled, or none configured at all), the | ||
| // historical 400 BAD_REQUEST classified the failure as non-fallbackable, so a | ||
| // combo like `antigravity/opus → github/opus` died on the first leg with a | ||
| // hard 400 even though the next combo target was perfectly healthy. | ||
| // | ||
| // The combo target loop (open-sse/services/combo.ts) deliberately breaks on | ||
| // 400 to prevent infinite fallback loops with body-specific 4xx errors | ||
| // (#4279/PR#4316). 404 NOT_FOUND, by contrast, flows through checkFallbackError | ||
| // as `shouldFallback: true` (generic-error catch-all path, | ||
| // open-sse/services/accountFallback.ts:1593-1599) so the next combo target is | ||
| // tried. We surface "no active credentials" as 404 so combo can skip past a | ||
| // disabled-credentials provider instead of failing the whole request. |
There was a problem hiding this comment.
Similar to the comment in chatHelpers.ts, 404 actually matches the status_404 rule in ERROR_RULES (defined in open-sse/config/errorConfig.ts) rather than flowing through the generic-error catch-all path. Let's update this comment to be technically accurate.
// Ported from upstream decolua/9router#336 (Ibrahim Ryan): when a provider has
// zero usable connections (all disabled, or none configured at all), the
// historical 400 BAD_REQUEST classified the failure as non-fallbackable, so a
// combo like `antigravity/opus → github/opus` died on the first leg with a
// hard 400 even though the next combo target was perfectly healthy.
//
// The combo target loop (open-sse/services/combo.ts) deliberately breaks on
// 400 to prevent infinite fallback loops with body-specific 4xx errors
// (#4279/PR#4316). 404 NOT_FOUND, by contrast, flows through checkFallbackError
// as `shouldFallback: true` (matching the status_404 rule in
// open-sse/config/errorConfig.ts) so the next combo target is
// tried. We surface "no active credentials" as 404 so combo can skip past a
// disabled-credentials provider instead of failing the whole request.
When a combo target (e.g. `antigravity/opus → github/opus`) hit a leg whose only configured connections were disabled, `handleNoCredentials` in `src/sse/handlers/chatHelpers.ts` returned 400 BAD_REQUEST. The combo target loop treats 400 as a hard stop — added by PR #4316 / issue #4279 to prevent infinite fallback loops on body-specific 4xx errors — so the combo died on the first leg even when the next target was perfectly healthy. The "no usable credentials, no connections tried yet" branch now returns 404 NOT_FOUND with the message "No active credentials for provider: <p>" instead. 404 flows through `checkFallbackError` as `shouldFallback: true` via the generic-error catch-all path in `open-sse/services/accountFallback.ts` (no explicit 404 special-case + transient cooldown), so the combo loop tries the next target. The log level for this branch also drops from `error` to `warn` because zero active credentials is an expected operator-driven state, not a server fault. The sibling branches in `handleNoCredentials` were left untouched: - `allRateLimited` already returns the upstream status (combo-fallbackable). - `allExpired` already returns 401 with a reconnect hint (combo-fallbackable via `checkFallbackError`). - The post-exhaustion branch (after at least one connection tried) already preserves `lastStatus`. Tests updated to encode the new contract (TDD: failing-then-passing per Hard Rule #18): `chat-helpers`, `chat-route-coverage`, `vscode-token-routes`, `chat-pipeline`, `llama-cpp-provider`, `combo-routing-e2e`. The `vscode-token-routes` assertions also flip `error.code` from `bad_request` to `model_not_found` per the OpenAI-compatible status mapping in `open-sse/config/errorConfig.ts:29`. Routes that emit `400 No credentials for provider` directly (rerank, audio, moderations, image edits, embeddings) are untouched — they don't flow through combo, so the 400 hard-stop semantics don't apply. Inspired-by upstream decolua/9router#336 (Ibrahim Ryan). Co-authored-by: Ibrahim Ryan <ryan@nuevanext.com>
dbfc2ff to
595719f
Compare
Rebuilt onto release/v3.8.33 (squash-base-stale). Integrated into release/v3.8.33.
1 participant
Summary
Ports an upstream fix by Ibrahim Ryan (@East-rayyy) into OmniRoute.
When a combo target (e.g.
antigravity/opus → github/opus) hit a leg whose only configured connections were disabled,handleNoCredentialsinsrc/sse/handlers/chatHelpers.tsreturned 400 BAD_REQUEST. The combo target loop (open-sse/services/combo.ts) treats400as a hard stop — added in PR #4316 / issue #4279 to prevent infinite fallback loops on body-specific 4xx errors — so the combo died on the first leg even when the next target was perfectly healthy.The "no usable credentials, no connections tried yet" branch now returns 404 NOT_FOUND with
"No active credentials for provider: <p>"instead.404flows throughcheckFallbackErrorasshouldFallback: truevia the generic-error catch-all path inopen-sse/services/accountFallback.ts:1593-1599(no explicit 404 special-case + transient cooldown), so the combo loop tries the next target. The log level for this branch also drops fromerror→warnbecause zero active credentials is an expected operator-driven state, not a server fault.The sibling branches in
handleNoCredentialswere intentionally left untouched:allRateLimitedalready returns the upstream status (combo-fallbackable).allExpiredalready returns401with a reconnect hint (combo-fallbackable viacheckFallbackError).lastStatus.Routes that emit
400 No credentials for providerdirectly (/v1/rerank,/v1/audio/{speech,transcriptions},/v1/moderations,/v1/images/edits,/v1/providers/[provider]/embeddings) are untouched — they don't flow through combo, so the 400-hard-stop semantics don't apply.Co-authored-by: Ibrahim Ryan ryan@nuevanext.com
Test plan
tests/unit/chat-helpers.test.tsreproduces the bug (400 ≠ 404), the fix turns it green, becomes the regression guard.tests/unit/chat-helpers.test.ts— 22/22tests/unit/vscode-token-routes.test.ts— 37/37 (also flipserror.codefrombad_request→model_not_foundper the OpenAI-compatible status mapping inopen-sse/config/errorConfig.ts:29)tests/unit/chat-route-coverage.test.ts— 14/14tests/integration/chat-pipeline.test.ts— 28/29 (the 1 unrelated failure is a pre-existingCodex CLI fingerprintJSON-body-order check that is also red on the rebased base branch without this PR's changes)tests/integration/llama-cpp-provider.test.ts— 3/3tests/integration/combo-routing-e2e.test.ts— 9/9tests/unit/auth-clear-provider-routes.test.tsfor/v1/moderations(which still emits 400) confirmed green — 4/4 — proving the change is scoped to the chat-handler path.npm run typecheck:coreclean.no-explicit-anywarnings).