feat(errors): classify provider failures and enrich telemetry evidence - #879
Conversation
Every upstream model failure used to collapse into one generic internal_error: _send_with_retry/_send_raw_with_retry raised bare RuntimeErrors and the server catch-all mapped everything to 500. Telemetry spans recorded only transport metadata, and orchestration traces omitted which model served a step. - add contextual_orchestrator.provider_errors: upstream HTTP status, network, TLS, and transport failures classify into OpenAI-compatible codes with client status, retryability, and bounded redacted messages (CWE-209); chat/passthrough/stream/batch transports surface the classification; agent failover preserves the final classified cause; server error payloads gain next-step guidance per failure family. - telemetry spans now record GenAI semantic-convention usage counts, served response model, finish reason, request latency, and classified error.type plus upstream status on failures. - route/stream/batch/conduct trace steps carry model, provider, and latency_ms alongside usage. Full suite: 2279 passed. Docstrings 100% (interrogate); statement coverage 100% on provider_errors.py and telemetry.py.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthrough제공자 오류를 Changes제공자 신뢰성 및 관측성
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟡 Moderate · up to This PR improves provider error responses and trace evidence, but several current request paths can still misclassify failures, lose actionable provider details, or crash instead of returning the intended contract, while trace labels and audit coverage remain inconsistent. The change is not merge-ready until these bounded correctness and observability issues are addressed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant ModelClient
participant Provider
participant Telemetry
Client->>Server: Chat 또는 Responses 요청
Server->>ModelClient: provider 호출
ModelClient->>Provider: 요청 전송
Provider-->>ModelClient: 응답 또는 provider 실패
ModelClient->>Telemetry: usage, 모델, 종료 사유, 지연 시간 기록
ModelClient-->>Server: 응답 또는 ProviderUpstreamError
Server-->>Client: HTTP 또는 SSE 응답
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 67.65% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 102 functions across 10 files. (3 skipped: 2 unsupported, 1 too large.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
![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.
![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.
# Conflicts: # CHANGELOG.md # contextual_orchestrator/orchestrator.py
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
# Conflicts: # docs/product-technical-gap-baseline.md
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Exact-head remediation at |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| def proxy_get_json(self, agent: ModelAgent, endpoint: str, *, max_response_bytes: int) -> dict[str, Any]: | ||
| """Retrieve provider JSON from the exact agent that owns an async job.""" |
There was a problem hiding this comment.
📝 Info: Provider-affine errors remain specialized
File and video transports retain route-specific public errors. Applying the general taxonomy there would erase established resource semantics.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if isinstance(current, ProviderUpstreamError): | ||
| if current.provider_status in ( | ||
| _PASSTHROUGH_UNAVAILABLE_STATUS | TRANSIENT_HTTP_STATUS | ||
| ): | ||
| return True |
There was a problem hiding this comment.
📝 Info: Ambiguous passthrough remains single-shot
Statusless timeouts and connection failures do not authorize cross-provider replay. Only explicit upstream statuses prove safe passthrough failover.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def provider_error_body(exc: urllib.error.HTTPError) -> bytes: | ||
| """Read and cache one bounded upstream error body for all classifiers.""" | ||
| cache_key = "_contextual_orchestrator_provider_error_body" | ||
| cached = getattr(exc, cache_key, None) | ||
| if isinstance(cached, bytes): | ||
| return cached | ||
| body = exc.read(MAX_PROVIDER_ERROR_BODY_BYTES + 1)[:MAX_PROVIDER_ERROR_BODY_BYTES] | ||
| try: | ||
| setattr(exc, cache_key, body) | ||
| except (AttributeError, TypeError): # pragma: no cover - HTTPError is mutable | ||
| pass | ||
| return body |
There was a problem hiding this comment.
📝 Info: Shared error bodies remain reusable
Retry, tool-limit, and public-message classifiers reuse one bounded cached body. Classifier order no longer consumes evidence needed downstream.
Was this helpful? React with 👍 or 👎 to provide feedback.
| """Provider failure taxonomy: typed, caller-actionable model errors. | ||
|
|
||
| Every upstream provider/model failure is classified into one OpenAI-compatible | ||
| error surface so callers learn *which* model failed, *why*, and *whether to | ||
| retry* — instead of receiving one opaque ``internal_error`` for every cause. | ||
|
|
||
| The classification derives from the upstream HTTP status (RFC 9110 semantics) | ||
| and the OpenAI error-code conventions used across compatible providers. | ||
| Provider response bodies are never surfaced raw: only a bounded, control-free | ||
| message field is kept, because provider diagnostics can embed secrets, | ||
| prompts, or internal topology (CWE-209). | ||
|
|
||
| References | ||
| ---------- | ||
| - Fielding, R., Nottingham, M., & Reschke, J. (2022). *HTTP semantics* | ||
| (RFC 9110). IETF. https://doi.org/10.17487/RFC9110 | ||
| - OpenAI. (2026). *Error codes*. https://platform.openai.com/docs/guides/error-codes | ||
| """ |
There was a problem hiding this comment.
🔍 Research artifact is missing
This substantive gateway feature adds no academic paper or linked summary. AGENTS.md requires research grounding for substantive feature PRs.
Was this helpful? React with 👍 or 👎 to provide feedback.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| except Exception as exc: # noqa: BLE001 - classify provider transport failures | ||
| raise classify_provider_failure( | ||
| exc, agent_id=agent.id, model=agent.model, transport="passthrough" | ||
| ) from None |
There was a problem hiding this comment.
🟡 Binary size rejections damage provider health
When a binary provider returns 413, classify_provider_failure runs before size detection. The request penalizes provider health and loses size-exhaustion semantics.
Prompt for agents
Preserve request-size semantics for binary capability calls. proxy_send_bytes now classifies raw HTTP 413 into ProviderUpstreamError, but _is_request_too_large_error does not recognize that typed representation. Make typed request_too_large/provider_status 413 failures detectable without weakening existing exception-chain limits, then test binary proxy_capability failover and all-candidate exhaustion. Verify 413 attempts do not update group-router health.
Was this helpful? React with 👍 or 👎 to provide feedback.
| 401: (401, "authentication_error", False), | ||
| 402: (402, "payment_required", False), | ||
| 403: (403, "permission_error", False), | ||
| 404: (404, "model_not_found", False), |
There was a problem hiding this comment.
🟡 Missing batch resources become missing models
Any 404 from batch polling or output retrieval becomes model_not_found. Missing batch resources tell callers to replace a valid model.
Prompt for agents
Make 404 classification transport or operation aware. The global PROVIDER_STATUS_SURFACES entry cannot distinguish a missing chat model from a missing batch, file, job, or passthrough resource. Preserve model_not_found for provider calls where 404 identifies the requested model, and use an accurate resource/API failure for batch lifecycle and non-model endpoints. Add tests for 404 during batch creation/polling/output-file retrieval as well as chat model lookup.
Was this helpful? React with 👍 or 👎 to provide feedback.
| guidance = _PROVIDER_FAILURE_GUIDANCE.get( | ||
| exc.error_code, "Review the request or contact the operator." | ||
| ) | ||
| return f"Model '{exc.model}' via agent '{exc.agent_id}': {exc}. {guidance}" |
There was a problem hiding this comment.
🔍 Provider wording precedes gateway guidance
Safe upstream text remains in caller messages before fixed guidance. Contradictory provider wording can make the human-readable recommendation ambiguous.
Was this helpful? React with 👍 or 👎 to provide feedback.
Two of this PR's own tests hardcoded assumptions that main invalidated during the merge above (32 commits, base was e7618a3 -> 5f2753a): - tests/test_nim_benchmark_workflow_contract.py read .github/workflows/tests.yml, which main renamed to ci.yml in 9b0a356 ("use conventional workflow filename"); the nim_benchmark_quality job content the tests check for is present and intact under the new name. Point both reads at ci.yml. - tests/test_nim_benchmark_release_acceptance.py:: test_budgeted_client_fallback_and_transport_errors matched the old error string "provider .* request failed". Main's contextual_orchestrator/provider_errors.py (PR #879, now on main) reclassifies provider HTTP failures through ProviderUpstreamError (still a RuntimeError subclass) with the new fixed message "provider rejected the request with HTTP {status}"; updated the match regex to the new message. One more failure, tests/test_nim_benchmark_release_acceptance.py:: test_smoke_manifest_cannot_authorize_production_routing, is NOT caused by this merge: it fails identically (same 1280-vs-1283 token-budget numbers on task trick_arithmetic_lily_pads/conduct_bounded) on this PR's own unmerged head b0167b0, contradicting the PR description's claimed "112 passed" NIM-focused run. Left untouched rather than loosening the equal-budget assertion or the 30/0.9 evidence thresholds without the author's input on why token usage grew by 3 tokens for that one locked task; flagged in the gap baseline for follow-up. Full suite after both fixes: 2797 passed, 2 failed (the pre-existing token-budget gap above, plus tests/test_psychometric_routing.py needing the private fast-mlsirm package that is unreachable in this sandbox, same documented blocker as PR #917). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2 participants
Summary
Closes three customer-visible gaps in one slice: (1) every upstream model/provider failure surfaced as a generic
internal_error; (2) OpenTelemetry spans carried no per-call evidence; (3) orchestration traces omitted which model/provider served each step.Problem
_send_with_retry/_send_raw_with_retrycollapsed provider HTTP errors into bareRuntimeErrors and the server catch-all mapped everything to500 internal_error, so callers could not distinguish a throttled model from an auth failure.latency_ms; routed/conducted steps omittedmodel/provider.Approach
contextual_orchestrator/provider_errors.py): upstream HTTP status (RFC 9110), network, TLS, and transport failures classify into OpenAI-compatible codes (rate_limit_exceeded,authentication_error,model_not_found,provider_timeout, …) with client status, retryability, and one bounded redacted message (CWE-209). Only JSONerror.messagefields pass through; URLs/secrets never do.ProviderUpstreamErrorinstead of an opaque collapse.ProviderUpstreamErrorto its client status/code with structured detail (agent_id, model, provider_status, retryable, transport) plus actionable next-step guidance per failure family.gen_ai.usage.input_tokens/output_tokens/total_tokensfrom provider-reported counts, servedgen_ai.response.model,gen_ai.response.finish_reasons, request latency, and classifiederror.type+ upstream status on failure (replacing exception-class labels).model,provider,latency_msalongside usage.Validation
a62e1ca3): 2279 passed.interrogate, org gate).provider_errors.pyandtelemetry.py.429 rate_limit_exceededwith guidance — neverinternal_error.Summary by CodeRabbit
--production인증 게이트와 공용 바인드 보호가 추가되었습니다.