fix(combo): reject known context overflow without exhausting providers - #7177
diegosouzapw merged 5 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces request-scoped upstream failure detection to prevent model-specific or request-specific errors (such as context length exceeded or empty responses) from triggering provider-wide resilience penalties like cooldowns or circuit breakers. It also adds early rejection for requests that exceed the context limits of all targets in a combo, and improves error logging for failed combo responses. The review feedback suggests adding a defensive guard check in getKnownContextLimit to prevent potential runtime TypeError crashes when model capabilities are undefined.
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.
| function getKnownContextLimit( | ||
| capabilities: { | ||
| maxInputTokens?: number | null; | ||
| contextWindow?: number | null; | ||
| }, | ||
| requestedOutputTokens = 0 | ||
| ): number | null { | ||
| const limits: number[] = []; | ||
| if (capabilities.maxInputTokens != null) { | ||
| limits.push(capabilities.maxInputTokens + requestedOutputTokens); | ||
| } | ||
| if (capabilities.contextWindow != null) { | ||
| limits.push(capabilities.contextWindow); | ||
| } | ||
| return limits.length > 0 ? Math.min(...limits) : null; | ||
| } |
There was a problem hiding this comment.
To prevent potential runtime TypeError crashes if getResolvedModelCapabilities returns null or undefined (e.g., for unknown or custom models), we should add a defensive guard check at the beginning of getKnownContextLimit.
| function getKnownContextLimit( | |
| capabilities: { | |
| maxInputTokens?: number | null; | |
| contextWindow?: number | null; | |
| }, | |
| requestedOutputTokens = 0 | |
| ): number | null { | |
| const limits: number[] = []; | |
| if (capabilities.maxInputTokens != null) { | |
| limits.push(capabilities.maxInputTokens + requestedOutputTokens); | |
| } | |
| if (capabilities.contextWindow != null) { | |
| limits.push(capabilities.contextWindow); | |
| } | |
| return limits.length > 0 ? Math.min(...limits) : null; | |
| } | |
| function getKnownContextLimit( | |
| capabilities: { | |
| maxInputTokens?: number | null; | |
| contextWindow?: number | null; | |
| } | null | undefined, | |
| requestedOutputTokens = 0 | |
| ): number | null { | |
| if (!capabilities) return null; | |
| const limits: number[] = []; | |
| if (capabilities.maxInputTokens != null) { | |
| limits.push(capabilities.maxInputTokens + requestedOutputTokens); | |
| } | |
| if (capabilities.contextWindow != null) { | |
| limits.push(capabilities.contextWindow); | |
| } | |
| return limits.length > 0 ? Math.min(...limits) : null; | |
| } |
There was a problem hiding this comment.
Not applying this one — verified false positive.
getResolvedModelCapabilities (src/lib/modelCapabilities.ts:385) is declared (input: CapabilityInput): ResolvedModelCapabilities — a non-nullable return type — and its body has exactly one return, an object literal. There is no code path that returns null/undefined, including for unknown/custom models: those resolve to an object whose maxInputTokens/contextWindow fields are null, which getKnownContextLimit already handles (both != null guards fail → limits stays empty → returns null → the caller fails open and keeps the legacy behavior). That unknown-metadata path is covered by tests/unit/combo-context-window-filter.test.ts ("unknown context metadata keeps overflow detection fail-open").
Widening the parameter type to | null | undefined would weaken the signature to accept a value the compiler already proves cannot occur, so the guard would be dead code.
Note getKnownContextLimit moved in 63966f5 — it is now exported from comboStructure.ts and consumed by the new open-sse/services/combo/knownContextOverflow.ts leaf (file-size ratchet extraction). Leaving this thread open for the maintainer to close.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Thanks for this — both pieces are real fixes. The pre-flight overflow rejection (avoiding wasted upstream dispatch + a clean One thing to flag before merge: there's another open PR (#7052) that fixes the exact same double-counting bug (#7039) in |
…t + fix context-overflow boundary bug - Extract getKnownContextOverflow (+ its KnownContextOverflow type) out of comboStructure.ts into a new open-sse/services/combo/knownContextOverflow.ts leaf, so the file-size ratchet (cap 800 for new files) passes. - Extract the skipConnectionDisable predicate out of handleSingleModelChat in chat.ts into open-sse/services/combo/comboPredicates.ts::shouldSkipConnDisable, and consolidate the new combo-failure-handling imports, to keep chat.ts under its frozen file-size baseline (1796) after the diegosouzapw#7177 request-scoped-failure wiring. - Fix a real boundary bug in getKnownContextOverflow surfaced by the merge: estimateRequestInputTokens counted a caller-omitted `messages: []` (which some combo entrypoints default in) as real content, charging a few phantom "structural" JSON.stringify tokens toward the estimate. That was enough to falsely trip the new known-context-overflow rejection for a request with no real input when max_tokens exactly equals the target's context window (a common config where limit_input === limit_output === limit_context), regressing tests/unit/combo-routing-engine.test.ts's pre-existing diegosouzapw#3587 "non-reasoning model does not get max_tokens buffer" case. Empty arrays/objects no longer count as estimable content. - Add a regression test for the exact-boundary empty-content case. Co-authored-by: Diego Rodrigues de Sa e Souza <diegosouza.pw@gmail.com>
Babysit summary — now green ✅Base was 25 commits behind Fixed —
Fixed —
Local validation (beyond CI): Review threads — 1 open (
No policy collision with the open GPT-5.x contract PRs (#7012/#7101/#7242) or the reasoning-summary pair (#7095/#7176): the only shared file is Authorship preserved (@JxnLexn). No tests weakened, no CI/baseline edits, no |
….ts (file-size cap) combo.ts's net delta for this PR was +108 lines, which would push the frozen file-size baseline (3387) past its ceiling once diegosouzapw#7177 merges first (+64). Move buildRecoveryHint() (pure terminalReason -> ComboRecoveryHint mapper) and buildNoUpstreamResponseDiagnostics() (the "no upstream response" fallback diagnostics literal) out of combo.ts into a new combo/pinRecovery.ts module. Both are pure, self-contained projections with no dependency on handleComboChat's local closure state, so this is a code move with no behavior change — combo.ts keeps only the call-site wiring (recordComboFailure/clearComboFailureTracking calls stay put, since those close over local state). Net result: combo.ts delta drops from +108 to +51 (58 insertions/7 deletions vs the PR's merge-base). Added tests/unit/combo/pin-recovery.test.ts for direct unit coverage of both extracted functions. buildNoUpstreamResponseDiagnostics was previously an inline object literal (no function-coverage surface of its own); extracting it without a direct test would have nudged coverage.functions down further after the prior commit's rebaseline. Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
… (file-size cap) comboStructure.ts is not frozen in the file-size baseline but is capped at 800 lines; this PR's net +29 on that file alone would push it over once merged. knownContextOverflow.ts already exists in this PR as the dedicated home for "known context limit" logic, so move the genuinely new pieces there instead of leaving them in comboStructure.ts: - hasEstimableContent (new): its own doc comment already frames it purely in terms of the known-context-overflow boundary check, so it belongs next to that check, not in the general request-compatibility file. - getKnownContextLimit (new, requestedOutputTokens-aware): this *is* the "how big is a target's known context window" primitive knownContextOverflow already consumes; hosting it there is a better fit than comboStructure.ts. - getLegacyKnownContextLimit: kept alongside its sibling rather than split across two files, since both are alternate implementations of the same concept (used only by comboStructure.ts's hasKnownCompatibleContextLimit). comboStructure.ts now imports all three back for its own internal callers (estimateRequestInputTokens, getTargetCompatibilityFailures, hasKnownCompatibleContextLimit). deriveRequestCompatibilityRequirements and the RequestCompatibilityRequirements type stay in comboStructure.ts exactly as this PR already has them (still consumed internally there), so knownContextOverflow.ts keeps importing those two, same as before. No behavior change — pure relocation, verified by the existing PR test suite (combo-context-window-filter, combo-breaker-429, combo-failure-log-message, combo-target-exhaustion, diagnostics) plus the pre-existing combo-vision-aware-routing/combo-context-requirements/combo-roundrobin-compat-fallback-6238 suites, all green. Net effect on open-sse/services/combo/comboStructure.ts vs. this PR's merge base: -2 lines (was +29). typecheck:core, lint, and the complexity ratchets (check:complexity, check:cognitive-complexity) are unchanged from this PR's current HEAD — the 4 pre-existing complexity/max-lines findings in valueContainsImagePart/filterTargetsByRequestCompatibility are untouched by this move (same violations, same total ratchet counts, just shifted line numbers). Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
… for compatibility fit, drop legacy variant Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
c1a3d83
into
diegosouzapw:release/v3.8.49
|
Thanks @JxnLexn! Merged into release/v3.8.49 after combined merge-train validation (static gates + tests + vitest green on the merged tree). Where quality gates flagged growth we decomposed/extracted inside your branch keeping you as author — including renumbering the reasoning-routing migration slot and the ProviderDetailPageClient rebaseline annotation. |
diegosouzapw#7177) * Fix context-window exhaustion classification * fix(combo): keep chat.ts/comboStructure.ts under the file-size ratchet + fix context-overflow boundary bug - Extract getKnownContextOverflow (+ its KnownContextOverflow type) out of comboStructure.ts into a new open-sse/services/combo/knownContextOverflow.ts leaf, so the file-size ratchet (cap 800 for new files) passes. - Extract the skipConnectionDisable predicate out of handleSingleModelChat in chat.ts into open-sse/services/combo/comboPredicates.ts::shouldSkipConnDisable, and consolidate the new combo-failure-handling imports, to keep chat.ts under its frozen file-size baseline (1796) after the diegosouzapw#7177 request-scoped-failure wiring. - Fix a real boundary bug in getKnownContextOverflow surfaced by the merge: estimateRequestInputTokens counted a caller-omitted `messages: []` (which some combo entrypoints default in) as real content, charging a few phantom "structural" JSON.stringify tokens toward the estimate. That was enough to falsely trip the new known-context-overflow rejection for a request with no real input when max_tokens exactly equals the target's context window (a common config where limit_input === limit_output === limit_context), regressing tests/unit/combo-routing-engine.test.ts's pre-existing diegosouzapw#3587 "non-reasoning model does not get max_tokens buffer" case. Empty arrays/objects no longer count as estimable content. - Add a regression test for the exact-boundary empty-content case. Co-authored-by: Diego Rodrigues de Sa e Souza <diegosouza.pw@gmail.com> * refactor(combo): move overflow logic into knownContextOverflow module (file-size cap) comboStructure.ts is not frozen in the file-size baseline but is capped at 800 lines; this PR's net +29 on that file alone would push it over once merged. knownContextOverflow.ts already exists in this PR as the dedicated home for "known context limit" logic, so move the genuinely new pieces there instead of leaving them in comboStructure.ts: - hasEstimableContent (new): its own doc comment already frames it purely in terms of the known-context-overflow boundary check, so it belongs next to that check, not in the general request-compatibility file. - getKnownContextLimit (new, requestedOutputTokens-aware): this *is* the "how big is a target's known context window" primitive knownContextOverflow already consumes; hosting it there is a better fit than comboStructure.ts. - getLegacyKnownContextLimit: kept alongside its sibling rather than split across two files, since both are alternate implementations of the same concept (used only by comboStructure.ts's hasKnownCompatibleContextLimit). comboStructure.ts now imports all three back for its own internal callers (estimateRequestInputTokens, getTargetCompatibilityFailures, hasKnownCompatibleContextLimit). deriveRequestCompatibilityRequirements and the RequestCompatibilityRequirements type stay in comboStructure.ts exactly as this PR already has them (still consumed internally there), so knownContextOverflow.ts keeps importing those two, same as before. No behavior change — pure relocation, verified by the existing PR test suite (combo-context-window-filter, combo-breaker-429, combo-failure-log-message, combo-target-exhaustion, diagnostics) plus the pre-existing combo-vision-aware-routing/combo-context-requirements/combo-roundrobin-compat-fallback-6238 suites, all green. Net effect on open-sse/services/combo/comboStructure.ts vs. this PR's merge base: -2 lines (was +29). typecheck:core, lint, and the complexity ratchets (check:complexity, check:cognitive-complexity) are unchanged from this PR's current HEAD — the 4 pre-existing complexity/max-lines findings in valueContainsImagePart/filterTargetsByRequestCompatibility are untouched by this move (same violations, same total ratchet counts, just shifted line numbers). Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> --------- Co-authored-by: Diego Rodrigues de Sa e Souza <diegosouza.pw@gmail.com> Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
diegosouzapw#7177) * Fix context-window exhaustion classification * fix(combo): keep chat.ts/comboStructure.ts under the file-size ratchet + fix context-overflow boundary bug - Extract getKnownContextOverflow (+ its KnownContextOverflow type) out of comboStructure.ts into a new open-sse/services/combo/knownContextOverflow.ts leaf, so the file-size ratchet (cap 800 for new files) passes. - Extract the skipConnectionDisable predicate out of handleSingleModelChat in chat.ts into open-sse/services/combo/comboPredicates.ts::shouldSkipConnDisable, and consolidate the new combo-failure-handling imports, to keep chat.ts under its frozen file-size baseline (1796) after the diegosouzapw#7177 request-scoped-failure wiring. - Fix a real boundary bug in getKnownContextOverflow surfaced by the merge: estimateRequestInputTokens counted a caller-omitted `messages: []` (which some combo entrypoints default in) as real content, charging a few phantom "structural" JSON.stringify tokens toward the estimate. That was enough to falsely trip the new known-context-overflow rejection for a request with no real input when max_tokens exactly equals the target's context window (a common config where limit_input === limit_output === limit_context), regressing tests/unit/combo-routing-engine.test.ts's pre-existing diegosouzapw#3587 "non-reasoning model does not get max_tokens buffer" case. Empty arrays/objects no longer count as estimable content. - Add a regression test for the exact-boundary empty-content case. Co-authored-by: Diego Rodrigues de Sa e Souza <diegosouza.pw@gmail.com> * refactor(combo): move overflow logic into knownContextOverflow module (file-size cap) comboStructure.ts is not frozen in the file-size baseline but is capped at 800 lines; this PR's net +29 on that file alone would push it over once merged. knownContextOverflow.ts already exists in this PR as the dedicated home for "known context limit" logic, so move the genuinely new pieces there instead of leaving them in comboStructure.ts: - hasEstimableContent (new): its own doc comment already frames it purely in terms of the known-context-overflow boundary check, so it belongs next to that check, not in the general request-compatibility file. - getKnownContextLimit (new, requestedOutputTokens-aware): this *is* the "how big is a target's known context window" primitive knownContextOverflow already consumes; hosting it there is a better fit than comboStructure.ts. - getLegacyKnownContextLimit: kept alongside its sibling rather than split across two files, since both are alternate implementations of the same concept (used only by comboStructure.ts's hasKnownCompatibleContextLimit). comboStructure.ts now imports all three back for its own internal callers (estimateRequestInputTokens, getTargetCompatibilityFailures, hasKnownCompatibleContextLimit). deriveRequestCompatibilityRequirements and the RequestCompatibilityRequirements type stay in comboStructure.ts exactly as this PR already has them (still consumed internally there), so knownContextOverflow.ts keeps importing those two, same as before. No behavior change — pure relocation, verified by the existing PR test suite (combo-context-window-filter, combo-breaker-429, combo-failure-log-message, combo-target-exhaustion, diagnostics) plus the pre-existing combo-vision-aware-routing/combo-context-requirements/combo-roundrobin-compat-fallback-6238 suites, all green. Net effect on open-sse/services/combo/comboStructure.ts vs. this PR's merge base: -2 lines (was +29). typecheck:core, lint, and the complexity ratchets (check:complexity, check:cognitive-complexity) are unchanged from this PR's current HEAD — the 4 pre-existing complexity/max-lines findings in valueContainsImagePart/filterTargetsByRequestCompatibility are untouched by this move (same violations, same total ratchet counts, just shifted line numbers). Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> --------- Co-authored-by: Diego Rodrigues de Sa e Souza <diegosouza.pw@gmail.com> Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
What changed
context_length_exceededHTTP 400 response with combo diagnostics.status: "failed"responses without usable output as request-scoped upstream failures.all targets exhausted.Why
Oversized Codex requests were still dispatched even when all combo targets had known insufficient context windows. Codex could answer HTTP 200 with
status: "failed"and empty output; OmniRoute converted this into a synthetic 502 and incorrectly treated it as provider-wide failure. Repetition then opened the Codex breaker and produced the misleadingall targets exhausteddashboard state.Scope
This PR is based directly on
release/v3.8.49. It contains only the local context-window fix diff fromJxnLexn/OmniRoute:dev/fix_error_context_window_sizeagainst its unchangedJxnLexn/OmniRoute:mainbase. No additional changes fromJxnLexn/OmniRoute:mainand no encrypted-reasoning changes are included.Validation
npm run typecheck:coregit diff --check