fix(passthrough): fail over on single-tool-call-limit provider errors - #986
fix(passthrough): fail over on single-tool-call-limit provider errors#986seonghobae wants to merge 18 commits into
Conversation
discover_provider_models made exactly one HTTP attempt per provider and
raised ProviderDiscoveryError immediately on any transient failure,
zeroing out that whole provider's contribution to the discovery pass.
Observed live: a single Bytez HTTP 500 during a hosted noema-review run
propagated all the way to the org's shared orchestrator/free review
pool failing closed for every consumer repo, even though Bytez models
were never free-eligible in the first place (a separate, pre-existing
fact unrelated to this specific incident).
Reuse the existing is_transient_error classifier (already trusted for
completion-call retries) to add one bounded retry -- short fixed delay,
shortened timeout -- for exactly the 5xx/timeout/connection-reset class
that a retry can actually fix. Non-transient failures (bad credential,
malformed response) are never retried, matching existing behavior.
Deliberately out of scope: ModelClient.proxy_send_once's single-shot
completion-call guarantee ("cross-provider failover cannot amplify
load") is left untouched. Retrying there too was considered and
rejected after review found it risks stacking latency past CI callers'
own time budgets and reintroducing exactly the request amplification
that guarantee exists to prevent.
Devin review on #923: the retry attempt hardcoded _DISCOVERY_RETRY_TIMEOUT_SECONDS (5.0s) regardless of what timeout the caller requested, so a caller budgeting e.g. 2s per attempt could see the retry alone exceed that budget. Use min(timeout, _DISCOVERY_RETRY_TIMEOUT_SECONDS) instead.
Root-caused a live Strix required-check failure blocking contextual-orchestrator#923: openai.BadRequestError 400 invalid_stream_options, "stream_options.include_usage=true is not supported with tools or response_format" -- raised by this gateway's own /v1/chat/completions validation, not an upstream provider limitation. Strix's openai-agents SDK always sends tools + stream_options.include_usage=true together, which is normal agentic-client behavior. proxy_completion's single-agent tool passthrough always fetches a complete, non-streamed upstream response (upstream["stream"] = False) and frames it locally via _chat_response_sse_chunks, which already emits a real, honestly-labeled usage chunk (usage_source: reported/estimated) alongside tool-call deltas -- the combination was already fully supported downstream, so the upfront rejection was a stale, avoidable restriction. Narrowed the check to still reject response_format's separate multi-agent "conduct" path, which has no equivalent aggregate-usage story yet. Updated the two tests whose docstrings encoded the old (incorrect) assumption that "structured passthrough cannot emit usage SSE" -- both now verify the tools case returns a real 200 SSE with a usage chunk, while response_format-only still fails closed with invalid_stream_options. Verified: targeted files 15 passed; full suite run in progress.
…-commercialization-afow1j' into claude/noema-contextualwisdomlab-commercialization-afow1j
…ansient_error Devin review on #923 found: urlopen wraps a TLS handshake's ssl.SSLCertVerificationError as URLError(reason=...), not as a bare ssl.SSLError. is_transient_error's blanket "any URLError is transient" branch matched first and returned True before the existing bare-SSLError unwrap could ever see it, so a permanently invalid certificate was being retried as if it were a network blip. Fixed the shared classifier itself (not just the discovery retry call site Devin's suggested diff targeted), so every current and future caller benefits. Added a regression test covering both the previously-broken case and that an ordinary URLError(ConnectionResetError(...)) is still correctly transient. Verified: targeted test_provider_reliability.py 25 passed; full suite run in progress.
…alwisdomlab-commercialization-afow1j # Conflicts: # contextual_orchestrator/server.py # tests/test_chat_tools_passthrough_controls_http_honesty.py # tests/test_stream_options_null_flags_noop_http_honesty.py
…alwisdomlab-commercialization-afow1j # Conflicts: # contextual_orchestrator/model_discovery.py
…alwisdomlab-commercialization-afow1j # Conflicts: # CHANGELOG.md # contextual_orchestrator/model_discovery.py
Two real gaps Devin's review found in #923's own retry loop, both capable of crashing the entire discover_all_models sweep instead of isolating a single provider's failure: - provider_error_body's exc.read() had no guard, so a stalled/dropped connection raising http.client.IncompleteRead (not an OSError subclass) during is_transient_error's HTTP-error-body inspection would escape classification entirely. Now degrades to an empty body on any read failure, matching safe_provider_message's existing "bodies are untrusted input" handling. - discover_provider_models's retry loop didn't catch RuntimeError, but ModelClient._resolve_addresses (used by the configured_gateway transport) wraps a DNS resolution failure as plain RuntimeError. Now caught and classified as transport_error, isolated to that provider.
…r wrapper Devin's follow-up on the prior DNS-isolation fix: ModelClient._resolve_addresses wraps socket.gaierror as plain RuntimeError, so is_transient_error rejected it outright -- a genuinely temporary DNS hiccup (EAI_AGAIN) was isolated instead of retried, unlike every other transient failure this PR retries. is_transient_error now unwraps a RuntimeError's __cause__ and defers to the existing EAI_AGAIN check when it's a socket.gaierror; any other RuntimeError (malformed URL, no resolvable address) still falls through to non-transient.
Full-suite run surfaced two failures #941 (which removed model_discovery._provider_family) missed: - test_discovery_bootstrap_selection.py's nim-primary/sub test asserted the old family-collapsing outcome for select_bootstrap_discovered_agents (a sibling of provider_bootstrap.py's select_provider_diverse_models, which #941 did update) -- nvidia_nim and nvidia_nim_sub are independent providers now, so both occupy first-pass diversity slots directly. - test_model_discovery_boundaries.py's "unclassified failure" contract test used a plain RuntimeError as its example of an unclassified type, but RuntimeError is now deliberately classified (transport_error, for the configured-gateway DNS/validation wrapper). Switched to KeyError, a type genuinely outside every classified branch.
…alwisdomlab-commercialization-afow1j # Conflicts: # tests/test_discovery_bootstrap_selection.py
…alwisdomlab-commercialization-afow1j
Some NVIDIA NIM-hosted models (observed live: a vision-capable Llama variant, ContextualWisdomLab/naruon#1486 Strix scan) reject any turn with more than one tool call. The rejection surfaces as a generic invalid_request_error (not a distinct error code), with the model's own capability-limit sentence embedded in a longer agent-prefixed message -- so the message text is the only reliable signal. Without recognizing this, the orchestrator raised instead of moving on to the next capability-matched agent, ending the whole scan on a single model's tool-call-count limitation.
|
Warning Review limit reachedNext included review available in 56 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ling The org's standing directive requires central Strix/OpenCode/Noema scans to get at least a 3-hour floor (observed real runs go well past that). Strix's own budget was 150-minute process / 155-minute total (obfuscated via budget_suffix, per contract, so the literal env var names never appear in workflow logs), bounded by a 170-minute step and 200-minute job -- 155 minutes falls short of the 3-hour floor. Raise process/total/step/job proportionally to the actual maximum job execution time GitHub-hosted runners allow (6 hours): job 200->360 (the platform ceiling itself), step 170->330, total budget 9300->18900s (315 min), process budget 9000->18600s (310 min) -- preserving the original buffer ratios between each layer. Update the matching scripts/ci/test_strix_quick_gate.sh contract assertions in lockstep. Also records this investigation, plus the contextual-orchestrator single-tool-call-limit failover fix (ContextualWisdomLab/contextual-orchestrator#986) and confirmation that Strix already scans the full codebase and the @opencode-agent mention convention is already correct, in docs/product-technical-gap-baseline.md.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codex Review (#986): the new _is_single_tool_call_limit_error was wired into the failover decision (_is_passthrough_failover_error) but not into the exemption that keeps a failure from tripping the circuit breaker or counting as a failed stability observation. A model rejecting a request shape it can never support (too many tool descriptions, more than one tool call per turn) says nothing about that model's health for a differently-shaped future request -- unlike a reliability failure, it must not penalize the model. Add _is_capability_mismatch_failover_error(exc), covering both _is_provider_tool_description_limit_error and _is_single_tool_call_limit_error, and check it alongside the existing _is_request_too_large_error exemption at the health-penalty call site. every_failure_was_request_too_large (which decides whether to raise ProviderRequestTooLargeError when every candidate failed) is untouched -- it has a distinct, size-specific meaning. New parametrized test (test_capability_mismatch_failover_does_not_penalize_the_model) proves the circuit breaker stays clean for both capability-mismatch shapes. Verified genuine RED on the single-tool-call-limit case before the fix (the circuit recorded a real failure) and GREEN after.
|
The Root cause: This exact defect is already fixed in I've triggered one re-run of the failed job as due diligence — the specific JSON malformation depends on which free-tier model got selected this time, so a re-run has a real chance of landing on a model that returns well-formed JSON and passing incidentally. That would not fix the underlying issue (still present on Generated by Claude Code |
Codex Review (#986): the new _is_single_tool_call_limit_error was wired into the failover decision (_is_passthrough_failover_error) but not into the exemption that keeps a failure from tripping the circuit breaker or counting as a failed stability observation. A model rejecting a request shape it can never support (too many tool descriptions, more than one tool call per turn) says nothing about that model's health for a differently-shaped future request -- unlike a reliability failure, it must not penalize the model. Add _is_capability_mismatch_failover_error(exc), covering both _is_provider_tool_description_limit_error and _is_single_tool_call_limit_error, and check it alongside the existing _is_request_too_large_error exemption at the health-penalty call site. every_failure_was_request_too_large (which decides whether to raise ProviderRequestTooLargeError when every candidate failed) is untouched -- it has a distinct, size-specific meaning. New parametrized test (test_capability_mismatch_failover_does_not_penalize_the_model) proves the circuit breaker stays clean for both capability-mismatch shapes. Verified genuine RED on the single-tool-call-limit case before the fix (the circuit recorded a real failure) and GREEN after.
…alwisdomlab-commercialization-afow1j
|
Merged current Re-ran the full suite post-merge and saw 5 failures: Verified the other 4 are pre-existing on Full suite after merge: 5 failed, 3301 passed, 2 skipped. This PR's own test file: 59/59 passed. Generated by Claude Code |
|
자동 정리: base 대비 실제 변경(diff)이 0건이라 이 PR을 닫습니다. 변경을 추가한 뒤 reopen하세요. |
2 participants
Summary
ContextualWisdomLab/naruon#1486) reject any turn that makes more than one tool call.invalid_request_error(not a distinct error code), with the model's own capability-limit sentence embedded in a longer, agent-prefixed message — so the message text is the only reliable signal, similar in shape to the existing_is_provider_tool_description_limit_errorcarve-out._is_passthrough_failover_errordid not treat it as a safe-to-fail-over condition, so the orchestrator raised instead of moving on to the next capability-matched agent — ending an entire scan on one model's tool-call-count limitation._SINGLE_TOOL_CALL_LIMIT_MESSAGEand_is_single_tool_call_limit_error(error)(mirroring the existing tool-description-limit classifier), and wires it into_is_passthrough_failover_erroralongside the other narrow, deliberate 400-class failover carve-outs.Test plan
test_virtual_passthrough_fails_over_on_single_tool_call_limitintests/test_passthrough_provider_failover.py, reproducing the exact observed NVIDIA NIM litellm error shape (invalid_request_errorcode, agent-prefixed message containing "This model only supports single tool-calls at once!").ProviderUpstreamErrorraised instead of failing over to the next agent) and GREEN after (53/53 passed in this file).python -m pytest tests -q— 2824 passed, 1 skipped, 1 failed. The one failure (tests/test_psychometric_routing.py::test_fast_mlsirm_fit_uses_judge_acceptance_item_for_context_score,ModuleNotFoundError: No module named 'fast_mlsirm') is a pre-existing optional-dependency gap in this sandbox, unrelated to this change — this PR touches onlycontextual_orchestrator/orchestrator.pyandtests/test_passthrough_provider_failover.py.Generated by Claude Code