fix(routing): classify primary provider transport failures explicitly - #922
Conversation
TaskOrchestrator._invoke's route/Conduct primary chat call is a bounded, side-effect-free model completion request, not a tool invocation, so it can never produce the ambiguous-outcome risk classify_tool_failure's fail-closed rows exist to guard against. It was still classified through that tool-execution-oriented, message-text heuristic: a generic ProviderUpstreamError (5xx/429/network) usually fell through to the `unknown` catch-all and correctly kept failing over, but only incidentally -- an upstream error body that happened to also say something like "invalid arguments" (naming an unrelated field, not a tool) could be misclassified into invalid_arguments/permission/policy and stop orchestrator/free and orchestrator/auto request-time failover on a request that never touched a tool. tool_fallback.classify_provider_transport_failure(retryable: bool) now classifies this specific call directly from the provider taxonomy's own already-computed retryable flag, never from message text, and never returns fail_closed: retryable failures get one bounded same-agent retry then sequential failover; non-retryable failures fail over immediately. classify_tool_failure itself, the existing 413 request-too-large failover, and the free-tier/priced-tier candidate-set boundary in _failover_candidates (orchestrator/free never advances into a priced agent; exhausting every candidate still fails closed with the last classified provider error) are all unchanged. Also hardens the shared Models.dev fetch discover_all_models depends on for nvidia_nim/nvidia_nim_sub free-tier classification (ADR 0041): it had exactly one attempt against an unauthenticated third-party endpoint already observed to reject urllib's default user agent, so one transient blip could erase every dependent provider's orchestrator/free coverage for a whole discovery run. _fetch_models_dev_metadata now retries with a small bounded budget before degrading to the existing "no evidence, not free" fail-closed behavior. Motivated by the orchestrator/free review-sidecar reliability gap in ContextualWisdomLab/.github PR #1433 (majority of noema-review runs failing with an opaque 502 from the gateway preflight). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
…-strix-orchestration-sexqzc # Conflicts: # CHANGELOG.md # contextual_orchestrator/model_discovery.py # contextual_orchestrator/orchestrator.py # tests/test_model_discovery.py
|
Warning Review limit reachedNext included review available in 47 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: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughProvider 전송 오류 분류를 ChangesProvider 장애 조치 분류
Models.dev 메타데이터 재시도
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🔵 Low · up to This change improves provider failover and metadata discovery resilience without promoting free routes to paid providers or expanding credential exposure. It is mergeable with explicit owner awareness for bounded duplicate provider calls during compound failures, less predictable mixed-error reporting, and two minor documentation corrections. Sequence Diagram(s)sequenceDiagram
participant TaskOrchestrator
participant ModelClient
participant classify_provider_transport_failure
participant AgentPool
TaskOrchestrator->>ModelClient: route/Conduct chat 호출
ModelClient-->>TaskOrchestrator: ProviderUpstreamError
TaskOrchestrator->>classify_provider_transport_failure: retryable 전달
classify_provider_transport_failure-->>TaskOrchestrator: 재시도 또는 failover 결정
TaskOrchestrator->>AgentPool: 다음 eligible agent 선택
sequenceDiagram
participant discover_all_models
participant _fetch_models_dev_metadata
participant ModelsDev
discover_all_models->>_fetch_models_dev_metadata: 카탈로그 조회 요청
_fetch_models_dev_metadata->>ModelsDev: HTTP fetch
ModelsDev-->>_fetch_models_dev_metadata: 데이터 또는 일시적 오류
_fetch_models_dev_metadata->>ModelsDev: 실패 시 최대 재시도
_fetch_models_dev_metadata-->>discover_all_models: 메타데이터 또는 None
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 5 files. (5 skipped: 4 unsupported, 1 too large.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
Follow-up to a dedicated adversarial root-cause review of PR #922: it correctly flagged that ToolFallbackStoppedError bypasses candidate failover entirely via an immediate re-raise in _invoke, the same shape as the ProviderUpstreamError misclassification this PR already fixed. Verified this is a distinct case, not a leftover instance of the fixed bug, and deliberately did not change its behavior: every path that raises it resolves to ambiguous_outcome, permission_denied, policy_blocked, or invalid_arguments (via classify_tool_failure's FAIL_CLOSED branches, or the provider's own explicit terminal tool-execution-state signal). ADR 0001 states as an explicit invariant that permission/policy failures must never fall through to another agent, and that non-idempotent timeout/transport uncertainty must never replay automatically -- for an ambiguous server-side outcome specifically, a different agent asking again does not make the uncertainty go away, so converting this to failover would need an explicit product decision about which failure kinds that is actually safe for, not a mechanical port of the ProviderUpstreamError fix. Left unimplemented pending that decision; documented inline so it reads as a deliberate boundary rather than an oversight next time it's audited. (Also verified in the same follow-up: ProviderResponseError's identical carve-out was already fixed independently on main's PR #868, which this branch is already rebased onto -- allowed_agent_ids is not None for every orchestrator/free and orchestrator/auto call, so it already advances to the next candidate and only fails closed after every candidate in the pool has structurally failed. No code change needed there.) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
Second, .github-local half of the 2026-08-30 request-time-failover investigation (contextual-orchestrator's own routing-level fix is tracked separately as ContextualWisdomLab/contextual-orchestrator#922, draft, not yet merged: classify_provider_transport_failure corrects a provider transport failure being misclassified through a tool-execution-failure classifier). That fix cannot help here: contextual_orchestrator_review_launcher.py's _preflight_review_agents calls client.proxy_send_once directly per candidate and never reaches TaskOrchestrator's own routing/failover at all. Its client is configured with max_retries=0 and proxy_send_once is a single-shot transport by design, so before this change a single transient blip (a 503, a timeout) permanently rejected an otherwise-healthy route with zero retry budget of this loop's own -- in the worst case where every discovered candidate hit the same transient blip in one run, the whole sidecar would exit before healthz regardless of how good the gateway's own failover is. _is_retryable_preflight_error classifies a caught exception by type and, for an HTTP failure, status code alone (408/429/500/502/503/504, or a connection-level failure with no status) -- never from response text, and with no dependency on the vendored contextual_orchestrator package (which this file's own coverage-omit note documents as unavailable to this repository's test suite; importing it at call time here would silently pass only in environments where it happens to be installed and fail in real CI). _preflight_review_agents now gives each candidate up to REVIEW_PREFLIGHT_ATTEMPTS_PER_ROUTE (2) attempts: one bounded retry on a retryable failure, none on a non-retryable one, preserving the exact same report/row shape. Bears directly on the exact-head review's acceptance criteria 3 and 4 for Strix's orchestrator/free access: a single flaky preflight attempt can no longer take the whole review pipeline down before Strix ever runs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if isinstance(exc, ProviderUpstreamError): | ||
| last_upstream_error = exc | ||
| if isinstance(exc, ProviderResponseError): | ||
| # The primary chat call is a bounded, side-effect-free | ||
| # model request, not a tool invocation: classify from | ||
| # the provider's own already-computed retryability | ||
| # instead of classify_tool_failure's message-text | ||
| # heuristics, so free/auto virtual-model failover can | ||
| # never be accidentally downgraded to fail-closed by | ||
| # incidental wording in an upstream error body (e.g. a | ||
| # 400 that happens to mention "invalid arguments"). | ||
| decision = classify_provider_transport_failure(exc.retryable) |
There was a problem hiding this comment.
📝 Info: Layered retries remain bounded
ModelClient.chat exhausts transport retries before _invoke applies its orchestration budget. The multiplication is finite and preserves the existing two-layer contract.
Was this helpful? React with 👍 or 👎 to provide feedback.
| return _decision( | ||
| ToolFailureKind.TRANSPORT_ERROR, | ||
| ToolFallbackAction.FAILOVER_AGENT, | ||
| circuit_failure=True, | ||
| ) |
There was a problem hiding this comment.
📝 Info: Permanent failures stay pool-bound
Non-retryable provider errors advance candidates, but _failover_candidates retains free-tier and model-group boundaries. Exhaustion returns the final typed provider error.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if isinstance(exc, ProviderUpstreamError): | ||
| last_upstream_error = exc | ||
| if isinstance(exc, ProviderResponseError): | ||
| # The primary chat call is a bounded, side-effect-free | ||
| # model request, not a tool invocation: classify from | ||
| # the provider's own already-computed retryability | ||
| # instead of classify_tool_failure's message-text | ||
| # heuristics, so free/auto virtual-model failover can | ||
| # never be accidentally downgraded to fail-closed by | ||
| # incidental wording in an upstream error body (e.g. a | ||
| # 400 that happens to mention "invalid arguments"). | ||
| decision = classify_provider_transport_failure(exc.retryable) |
There was a problem hiding this comment.
📝 Info: Size failures bypass classification
The earlier _is_request_too_large_error guard intercepts 413 failures. They retain penalty-free failover and the aggregate exhaustion error.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for attempt in range(_MODELS_DEV_FETCH_ATTEMPTS): | ||
| try: | ||
| return _fetch_json(_MODELS_DEV_URL, timeout=timeout) | ||
| except (urllib.error.URLError, TimeoutError, ValueError, OSError): | ||
| if attempt < _MODELS_DEV_FETCH_ATTEMPTS - 1: | ||
| time.sleep(_MODELS_DEV_FETCH_RETRY_DELAY_SECONDS) | ||
| return None |
There was a problem hiding this comment.
📝 Info: Catalog retries preserve cost safety
Only parsed metadata reaches the exact model join. Exhaustion returns None, leaving cost unknown rather than classifying any model as free.
Was this helpful? React with 👍 or 👎 to provide feedback.
Adds the regression test ADR-0020 (ContextualWisdomLab/.github) calls out as still missing: a live incident reported against .github#1437 showed a required Strix run's three bounded outer attempts against orchestrator/free all landing on the same agent and failing with a provider HTTP 400 invalid_request_error (retryable: false), with the gateway ultimately declaring the whole free pool exhausted. This PR's own classify_provider_transport_failure change already routes a non-retryable ProviderUpstreamError to FAILOVER_AGENT instead of same-agent retry, but no existing test proved that end-to-end for a 4xx specifically (test_free_model_advances_through_the_free_pool_on_retryable_5xx only covers retryable 502/503). The new test reproduces the exact incident shape: a 400 on free_route_a advances immediately (no same-agent retry, unlike the 5xx case) to free_route_b. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
|
Added the regression test A live incident reported against The new test reproduces the exact incident shape and passes: a 400 on Full local suite: Generated by Claude Code |
|
Standing down on the required This is the same pre-existing, org-wide Generated by Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Two CodeRabbit nits on PR #922: - docs/architecture.md's _invoke description listed 5xx/429/network/413 as the failures that advance to the next candidate but omitted non-retryable 4xx (401/403/404), which classify_provider_transport_failure(False) also routes to FAILOVER_AGENT -- an operator reading only this list could wrongly conclude those errors aren't failed over. Added them and noted the list is illustrative, not exhaustive. - The ADR-0041 amendment said _fetch_models_dev_metadata "retries... 3 times", which reads as 3 retries (4 total attempts). The code makes at most 3 total attempts (the initial attempt plus 2 retries). Fixed the wording; no behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
|
The Generated by Claude Code |
2 participants
Summary
Investigates and hardens the
orchestrator/free/orchestrator/autorequest-time reliability gap behindContextualWisdomLab/.githubPR #1433 (thenoema-reviewsidecar's gateway preflight returning an opaque502instead of the required review passing).Root cause, confirmed and refined from the original hypothesis. The hypothesis in the task ("the gateway never retries the next-ranked candidate on a request-time failure") turned out to be mostly already false:
TaskOrchestrator._invokealready builds a full ranked candidate list per request (_failover_candidates) and already advances past a 413 and past most 5xx/network failures. But that advancing behavior depended on an implicit, fragile coincidence: the primary chat call's classifiedProviderUpstreamErrorwas routed throughclassify_tool_failure— a classifier designed for tool-execution failures, whereinvalid_arguments/permission_denied/policy_blockedcorrectlyfail_closed. A plain provider transport failure only avoided that fate because its message text usually didn't contain a tool-fallback keyword. A 500/502/429 whose body happened to also say something like "invalid arguments" (naming an unrelated field, not a tool) would have been misclassified intofail_closedand stopped free/auto failover on a request that never touched a tool — exactly the kind of thing that would surface as an intermittent opaque 502 in CI, matching the evidence.What changed (
contextual-orchestrator):contextual_orchestrator/tool_fallback.py: newclassify_provider_transport_failure(retryable: bool). It classifies a plain, side-effect-free provider/model transport failure directly from the provider taxonomy's own already-computedretryableflag — never from message text — and never returnsfail_closed: retryable (429/500/502/503/504/408/network) gets one bounded same-agent retry then sequential failover; non-retryable (401/403/404/422/...) fails over immediately. 413 is unaffected (already handled earlier, unconditionally, by_is_request_too_large_error).contextual_orchestrator/orchestrator.py:_invokenow uses that classifier forProviderUpstreamErrorinstead ofclassify_tool_failure.classify_tool_failureitself, the 413 failover, and the free-tier/priced-tier candidate boundary (orchestrator/freenever advances into a priced agent; exhausting every candidate still fails closed with the last classified provider error) are all unchanged.contextual_orchestrator/model_discovery.py: while reviewing discovery, found thatnvidia_nim/nvidia_nim_sub's entireorchestrator/freecoverage depends on one shared, unauthenticated, third-party Models.dev fetch (ADR 0041, merged ontomaintoday) that had exactly one attempt against an endpoint already documented in this same file as intermittently rejecting urllib's default user agent._fetch_models_dev_metadatanow retries that one fetch with a small bounded budget before degrading to the existing "no evidence, not free" behavior — a lone transient blip in a service this gateway doesn't control no longer has to erase that coverage for a whole discovery run.docs/adr/0001-tool-execution-fallback-policy.mdanddocs/planning/adrs/0041-generalize-models-dev-cost-classification.md(both explicitly stated intents this now fulfills more robustly), plus a pointer indocs/architecture.md, and aCHANGELOG.mdentry.Explicitly out of scope / flagged, not fixed here: a second, later symptom the coordinator observed mid-task — "sidecar exited before healthz" from a
ReviewPreflightError— was traced toContextualWisdomLab/.github's owncontextual_orchestrator_review_launcher.py(_preflight_review_agents, a sequential single-shot probe loop over the composed catalog withmax_retries=0), which lives entirely in the.githubrepo. Verified in-scope here:discover_all_modelsalready isolates one provider's discovery failure from the others (untouched, already correct), and the launcher's own zero-eligible-route path already fails closed with a distinct, typed message. No.githubfiles were touched, per the task's instructions.Not needed / considered and rejected: proactively excluding a free model with a small context/request-size limit from ranking (task point 4's first option) — the existing unconditional 413→failover (its second option) already covers this, so adding size-aware ranking would be new, untested scope for no behavioral gain right now.
Follow-up: adversarial re-review findings (addressed)
A separate, dedicated adversarial root-cause pass identified two more
_invokeexception carve-outs shaped like the bug this PR fixes, plus one discovery-quality issue. All three were investigated on this branch; here's the disposition of each:ProviderResponseError(HTTP 200 with empty/reasoning-only content, surfaced asinvalid_structured_output/502) — same immediate-raise-bypasses-failover shape as theProviderUpstreamErrorbug this PR fixes. Verified already resolved, independently, bymain's PR fix(api): accept the advertised gateway-default model on chat surfaces #868 (which this branch is rebased onto, commit913bb986):_invokenow advances to the next candidate wheneverallowed_agent_ids is not None— true for everyorchestrator/free/orchestrator/autocall — and only fails closed once every candidate in the pool has structurally failed (bounded_provider_response_failures == len(candidates)). No code change needed.ToolFallbackStoppedError(raised by the provider's own explicit terminaltool_execution_stoppedsignal, or byclassify_tool_failureresolvingFAIL_CLOSED) — confirmed still an unconditionalraisein_invokewith zero failover, on both the original pinned commit and currentmain. Investigated whether to port the same "advance while candidates remain, fail closed on exhaustion" pattern here, and deliberately did not: every path that raises this exception resolves toambiguous_outcome,permission_denied,policy_blocked, orinvalid_arguments. ADR 0001 states as an explicit invariant that permission/policy failures must never fall through to another agent, and that non-idempotent timeout/transport uncertainty must never replay automatically; for an ambiguous server-side outcome specifically, asking a different agent doesn't resolve the ambiguity about whether the first one's action already took effect — the risk is about the task's state, not the specific agent. Converting this to failover needs an explicit product decision about which failure kinds that's actually safe for (if any); it is not a mechanical port of the fix in this PR. Added an inline comment at theraise(new commit) explaining this is a deliberate boundary, not an oversight, since it's now the second time this exact line has been flagged as suspicious. Left unimplemented pending product sign-off.nvidia_nim/nvidia_nim_subadmitting retired/404 model IDs into the free catalog — plausible contributor to a thin catalog (reproduction reportedly saw 4/4 free candidates rejected, including 404s on retired models). A live/v1/models-style cross-check before admitting a discovered row (mirroring a pattern used elsewhere in.github's tooling) would help, but it's a new discovery-time network dependency with its own failure-mode design questions (fail-open vs. fail-closed on the cross-check's own failure, added latency/rate-limit exposure per discovery run, whether it generalizes past NIM) — not a small addition, and risks shrinking the catalog further if the cross-check itself is unreliable. Flagging for a separate, dedicated PR rather than folding into this one.One correction carried over from the adversarial pass for the record: the
request_failed status=413 code=request_too_largelog line in the original CI evidence is confirmed unrelated — it's the sidecar shell script's own deliberate self-test artifact, not a real failure. No code here was built around that line as evidence.Test plan
contextual_orchestrator/tool_fallback.py: new unit tests forclassify_provider_transport_failure(never fail-closed, boolean type-check).tests/test_provider_reliability.py:orchestrator/freeadvances through a 3-member free pool on 502/503 without ever calling a higher-priority priced agent in the pool; exhausting every free candidate fails closed with the last classified error and never calls the priced agent; a 500 whose body incidentally says "invalid arguments"/mentionstool_choicestill fails over (regression guard for the exact fragility this PR removes);orchestrator/auto's existing failover behavior is unchanged.tests/test_model_discovery.py: the shared Models.dev fetch retries a transient failure and recovers full free-tier evidence; exhausting the retry budget still degrades tois_free=False(not a crash), matching the ADR 0041 cost-safety argument.python -m pytest tests -q→ 2776 passed, 1 skipped, 1 deselected (the 1 deselected is the pre-existing, already-documentedfast-mlsirmsandbox-unreachable-dependency gap noted in this repo's own gap baseline; unrelated to this change), re-verified clean after the fix(api): accept the advertised gateway-default model on chat surfaces #868 rebase merge.python -m pytest tests/fuzz -q→ 14 passed.python tests/test_self_check.py/test_paper_contracts.py/test_conventions.py→ all pass. Targeted subset re-run after the follow-up (comment-only) commit → 235 passed.interrogateon the touched modules → 100%.pip-audit --requirement requirements.lock --strict→ no known vulnerabilities (no dependency changes in this PR).Rebased onto the
fix/gateway-default-chat-model(#868) merge that landed onmainmid-task; that PR independently touched the same_invokeexception-handling block (a new boundedProviderResponseErrorfailover for virtual pools — see follow-up section above) and the samemodel_discovery.pyregion (SSL/certifi retry, configured-gateway discovery, OpenRouter ZDR/privacy metadata) — merged cleanly, both behaviors preserved, full suite green afterward.Once this lands on
main(merged, not just opened), the vendoredORCHESTRATOR_PIN_SHAinContextualWisdomLab/.github'sscripts/ci/contextual_orchestrator_review_sidecar.shneeds a follow-up bump in a separate PR there — intentionally not done in this PR.🤖 Generated with Claude Code
https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
Summary by CodeRabbit
새로운 기능
버그 수정
문서