Repository navigation
fix(combo): advance to next model on 400 'model not supported' - #5249
diegosouzapw merged 1 commit into
Conversation
When a combo target returns 400 with 'model not supported', the combo should advance to the next target (different providers serve different models). The 400 guard in handleComboChat included MODEL_CAPACITY in its block list, which contradicted the comment stating model access denied errors 'should fall through'. Remove RateLimitReason.MODEL_CAPACITY from the 400 guard condition and clean up the now-unused import. Test: combo advances from first model (400) to second model (200).
![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 removes the RateLimitReason.MODEL_CAPACITY check from the fallback condition in handleComboChat and adds a unit test to verify that the priority combo strategy correctly falls back to the next model when a 400 error is encountered. The feedback suggests updating the test case to use an error message containing 'invalid' to ensure the fallback logic is robust against providers returning 'invalid' in their unsupported model errors.
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.
| if (modelStr === "openai/gpt-4") { | ||
| return Response.json( | ||
| { error: { message: "requested model is not supported" } }, | ||
| { status: 400 } | ||
| ); | ||
| } |
There was a problem hiding this comment.
To ensure the fallback logic is robust against providers returning "invalid" in their unsupported model errors (e.g., "requested model is invalid"), we should update the test case to use an error message containing "invalid".
This will verify that the guard condition in handleComboChat correctly ignores "invalid" when it is associated with the model, allowing the fallback to proceed.
if (modelStr === "openai/gpt-4") {
return Response.json(
{ error: { message: "requested model is invalid" } },
{ status: 400 }
);
}
|
I reproduced the failing #4279 body-specific 400 regression on this PR and opened a small helper PR against the source branch: What it does:
Local validation on the helper branch:
Commit: KooshaPari:fix/5249-body-400-guard @ bc9e85b |
|
I reproduced the current #5249 unit failure and pushed a helper fix that preserves the intended model-catalog 400 fallthrough without regressing the body-specific 400 stop guard. Helper PR into the source fork: Branch/commit:
Validation on the #5249 head + helper commit:
The narrow change adds a cross-target model unsupported classifier and keeps account/body-specific messages such as |
|
I reproduced the red #5249 CI locally and pushed a focused helper fix for the guard regression: Helper PR: Chewji9875#2 Root cause: the PR correctly wants generic Validation on the PR head + helper commit:
|
|
Thanks for this, @Chewji9875 — the goal (a combo should advance to the next model when one model isn't supported) is valid. But as written this can't merge yet, because it regresses an existing anti-infinite-loop guard. The conflict: removing the So we have two legitimate-but-opposite cases that both surface as a 400 "model not supported":
Blanket-removing the Suggested direction: keep the stop-guard, but narrow it so it only fires for the account-scoped variant (e.g. detect the account/plan-scoped phrasing, or check whether the same rejection applies to all remaining targets) and let the genuinely per-model "not supported" fall through to the next target. A test covering both Leaving this open for you to refine — happy to review again once both scenarios pass. 🙏 |
|
I reproduced the current #5249 failure and opened a focused helper PR into the source branch: It keeps this PR's intended behavior for generic Validation on
|
12de25b
into
cheaper-inference:release/v3.8.40
#5300) #5249 deliberately changed a MODEL_CAPACITY 400 ('model X not supported with this account') from STOP to ADVANCE-to-next-combo-target, since a different model in the combo may be supported. It added a regression test in combo-strategies.test.ts but did not update the separate combo-body-specific-400-stop-4279.test.ts, which still asserted the old STOP behavior for that exact model-not-supported text — leaving the release branch red on the Unit 2/2 shard (every PR inherited the failure). Re-point the #4279 test at a genuinely body-specific malformed 400 ('invalid message format'), which still recurs identically on every target and must STOP. This preserves the #2101 anti-loop {ok,response} contract coverage while the advance-on-model-400 path stays covered by combo-strategies.test.ts. Test-only; no production behavior change.
|
@diegosouzapw @Chewji9875 I am still facing this issue, I think we have to dig deep into this again.. |
…st (#8252) #2101 still hard-stopped model-not-supported failures when upstream wrapped them as invalid_request_error / Bad Request. Keep models in the combo and try the next target (#8251, residual of #5249). - export MODEL_ACCESS_DENIED_PATTERNS + broaden does-not-support shapes - add isModelScoped400() and exempt it from the body-specific stop guard - regression tests for wrapper forms; body-specific stop still intact Co-authored-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>
…st (cheaper-inference#8252) cheaper-inference#2101 still hard-stopped model-not-supported failures when upstream wrapped them as invalid_request_error / Bad Request. Keep models in the combo and try the next target (cheaper-inference#8251, residual of cheaper-inference#5249). - export MODEL_ACCESS_DENIED_PATTERNS + broaden does-not-support shapes - add isModelScoped400() and exempt it from the body-specific stop guard - regression tests for wrapper forms; body-specific stop still intact Co-authored-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>
…on fork-main
Fork-only commit. Strategy note (Aug 2026): upstream cadence
slowed to 1 release every 2-3 weeks. We land changes here first
and cherry-pick upstream only when diffs are small enough.
Adds tests/unit/combo-decision-matrix.test.ts with 25+ matrix
rows encoding the (provider, status, error envelope) → {advance |
stop | retry-then-stop} contract that the combo loop enforces
through checkFallbackError.
Recent history (cheaper-inference#2101 / cheaper-inference#4279 / cheaper-inference#5249 / cheaper-inference#8251 / cheaper-inference#8252) has been
six rounds of 'a substring in the 400 envelope was broadened;
combo advances where it used to stop.' Having one canonical
matrix file lets a future maintainer (and a code reviewer)
verify the contract at a glance.
Differs from the upstream-targeted PR-β draft:
- That draft assumed MODEL_ACCESS_DENIED_PATTERNS and
isModelScoped400 were exported from comboPredicates.ts.
- Audit correction: MODEL_ACCESS_DENIED_PATTERNS is 'const'
(internal to accountFallback.ts); isModelScoped400 does not
exist as a function. The matrix here exercises
checkFallbackError directly — the public predicate surface
combo dispatchers actually consult in production.
Case matrix covers:
- 400 advance on model-not-supported across anthropic/openai/
gemini/azure-openai (structured + text patterns)
- 400 stop on malformed/overflow/param-validation
- 400 stop on auth-credential 400 (must never re-classify to
model-access advance)
- 401 stop on plain auth failures
- 429 retry-then-stop on plain rate limits
- 429 stop on quota-exhausted with full reset window
- 5xx retry-then-stop on transient upstream outages
- 503 retry-then-stop via ALL_ACCOUNTS_RATE_LIMITED_PATTERNS
- Negative-path tests guarding against substring leakage on
non-400 statuses
Files: +1 (test only), 0 production-code lines touched.
Behavior-preserving.
… requests (#11781) Follow-up to #11762/#11774, same bug class in combo's own model-lockout wiring: GitHub rejects several models (gpt-5.4, gpt-5.3-codex, etc.) with a 400 that's permanently unavailable for this account's Copilot integration, but nothing recorded a cross-request lockout — combo's #5249 in-request advance guard is correct but doesn't persist, so the same doomed model gets retried from scratch on every new request, indefinitely. Fix: on a model-scoped 400 (`isModelScoped400`), call `lockModelIfPerModelQuota(provider, connectionId, rawModel, "model_capacity", 1h)`. GitHub already has per-model-quota enabled, so only the rejected model locks — siblings keep working. `isModelLocked()` is already checked pre-dispatch, so no other wiring needed. Validated: 3/3 new tests + fixed a pre-existing test-isolation gap in combo-model-scoped-400-advance.test.ts (shared model name across sub-tests without clearing lockout state). Thanks!
…er-inference#5249) A 400 classified as MODEL_CAPACITY (e.g. 'requested model is not supported') hit the cheaper-inference#2101 anti-loop stop-branch and halted the combo instead of advancing. Drop the MODEL_CAPACITY trigger from that stop condition so model-specific 400s fall through to the next combo target; genuinely body-specific 400s (malformed/invalid/bad-request substrings) still stop. Validated: combo-strategies suite 16/16 (incl. the new regression test) green on current release tip, typecheck clean. Stale pre-merge CI was from an older base. Co-authored-by: Chewji9875 <Chewji9875@users.noreply.github.com>
…eaper-inference#5249 advance policy (cheaper-inference#5300) cheaper-inference#5249 deliberately changed a MODEL_CAPACITY 400 ('model X not supported with this account') from STOP to ADVANCE-to-next-combo-target, since a different model in the combo may be supported. It added a regression test in combo-strategies.test.ts but did not update the separate combo-body-specific-400-stop-4279.test.ts, which still asserted the old STOP behavior for that exact model-not-supported text — leaving the release branch red on the Unit 2/2 shard (every PR inherited the failure). Re-point the cheaper-inference#4279 test at a genuinely body-specific malformed 400 ('invalid message format'), which still recurs identically on every target and must STOP. This preserves the cheaper-inference#2101 anti-loop {ok,response} contract coverage while the advance-on-model-400 path stays covered by combo-strategies.test.ts. Test-only; no production behavior change.
…st (cheaper-inference#8252) cheaper-inference#2101 still hard-stopped model-not-supported failures when upstream wrapped them as invalid_request_error / Bad Request. Keep models in the combo and try the next target (cheaper-inference#8251, residual of cheaper-inference#5249). - export MODEL_ACCESS_DENIED_PATTERNS + broaden does-not-support shapes - add isModelScoped400() and exempt it from the body-specific stop guard - regression tests for wrapper forms; body-specific stop still intact Co-authored-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>
… requests (cheaper-inference#11781) Follow-up to cheaper-inference#11762/cheaper-inference#11774, same bug class in combo's own model-lockout wiring: GitHub rejects several models (gpt-5.4, gpt-5.3-codex, etc.) with a 400 that's permanently unavailable for this account's Copilot integration, but nothing recorded a cross-request lockout — combo's cheaper-inference#5249 in-request advance guard is correct but doesn't persist, so the same doomed model gets retried from scratch on every new request, indefinitely. Fix: on a model-scoped 400 (`isModelScoped400`), call `lockModelIfPerModelQuota(provider, connectionId, rawModel, "model_capacity", 1h)`. GitHub already has per-model-quota enabled, so only the rejected model locks — siblings keep working. `isModelLocked()` is already checked pre-dispatch, so no other wiring needed. Validated: 3/3 new tests + fixed a pre-existing test-isolation gap in combo-model-scoped-400-advance.test.ts (shared model name across sub-tests without clearing lockout state). Thanks!
Problem
When a combo target returns 400 with
"requested model is not supported", the combo routing engine stops instead of advancing to the next target. This means a request routed through a combo that hits a provider lacking a specific model gets a hard 400 failure — even though other targets in the combo (different providers) may support that model.Root Cause
In
open-sse/services/combo.ts, the 400 guard condition inhandleComboChat(priority strategy) includedfallbackResult.reason === RateLimitReason.MODEL_CAPACITYin its "block fallback" list:checkFallbackError()correctly classifies "model not supported" asMODEL_CAPACITYwithshouldFallback: true— meaning "yes, try the next target." But the guard condition blocked exactly this case, contradicting the comment at line 2106: "Model access denied: different providers serve different model sets — These should fall through."Fix
1 line removed from the guard condition + cleanup of the now-unused
RateLimitReasonimport:handleRoundRobinCombodoes not have this same guard — only one fix location.Test
Added test: "priority combo advances to next model when first returns 400 'model not supported'"
openai/gpt-4→openai/gpt-3.5-turbo){ error: { message: "requested model is not supported" } }16/16 tests pass on
release/v3.8.40.