fix(server): classify provider response 502 detail - #976
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughProvider 응답의 콘텐츠 누락 원인을 기계 판독 가능한 실패 유형으로 분류합니다. 서버는 이 유형을 ChangesProvider 응답 실패 처리
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR adds a bounded provider failure classification to existing 502 responses while preserving the established status and error code, helping clients distinguish failure modes without changing authorization or response handling. No actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant ModelClient
participant ProviderResponseError
participant server.do_POST
participant HTTP Client
ModelClient->>ProviderResponseError: failure_kind 저장
ProviderResponseError-->>server.do_POST: detail 전달
server.do_POST-->>HTTP Client: 502 invalid_structured_output 응답
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation 변경 사항은 Issue Full details: Docstring CoverageExplanation Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 3 files. (1 skipped: 1 too large.) ✨ Finishing Touches 💡 1📝 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.
|
Cross-PR integration contract: routing identity is provider-neutral |
…-20260901-issue932 # Conflicts: # contextual_orchestrator/server.py # tests/test_chat_response_format_http_honesty.py
|
Pushed
Validation: full suite run (
The three targeted test files this PR's own body names ( Generated by Claude Code |
| failure_kind: str = "invalid_provider_response", | ||
| detail: Mapping[str, Any] | None = None, | ||
| ) -> None: | ||
| super().__init__(message) | ||
| self.failure_kind = failure_kind | ||
| self.detail = { | ||
| **(dict(detail) if detail else {}), | ||
| "provider_response_failure_kind": failure_kind, |
There was a problem hiding this comment.
📝 Info: Legacy failures stay generically classified
Other ProviderResponseError sites receive invalid_provider_response. This avoids mislabeling file, size, schema, and structured-output failures as content-specific failures.
Was this helpful? React with 👍 or 👎 to provide feedback.
| self.detail = { | ||
| **(dict(detail) if detail else {}), | ||
| "provider_response_failure_kind": failure_kind, | ||
| } |
There was a problem hiding this comment.
📝 Info: Canonical classification wins detail merges
provider_response_failure_kind overwrites any same-named detail entry. Supplementary fields survive without allowing contradictory classifications.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if isinstance(content, str) and content: | ||
| return content | ||
| if isinstance(message, dict) and message.get("reasoning"): | ||
| raise ProviderResponseError( | ||
| f"provider {agent.id} returned reasoning without content; " | ||
| "for mlx-lm set chat_template_args={\"enable_thinking\": false} or increase max_output_tokens" | ||
| "for mlx-lm set chat_template_args={\"enable_thinking\": false} or increase max_output_tokens", | ||
| failure_kind="reasoning_without_content", | ||
| ) | ||
| raise ProviderResponseError(f"provider {agent.id} response did not contain assistant content") | ||
| raise ProviderResponseError( | ||
| f"provider {agent.id} response did not contain assistant content", | ||
| failure_kind="assistant_content_missing", |
There was a problem hiding this comment.
📝 Info: Empty content follows missing-content handling
An empty answer no longer succeeds. Responses with reasoning retain the reasoning-only classification; other empty answers use the missing-content classification.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Two follow-ups on this PR's current head (
No action needed on either point; noting both so the reconciliation instruction isn't left silently unaddressed in the thread. Generated by Claude Code |
|
Third follow-up on this PR's current head ( Traced this to This isn't a code bug in Generated by Claude Code |
…-20260901-issue932
seonghobae
added
bug
ChatGPT Codex Connector
…-20260901-issue932 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
noema-review failure investigation + branch updateActual Since the PR was also
Generated by Claude Code |
…-20260901-issue932
"Full unit and contract suite" failure — root-caused and resolvedWhat failed: run 33658571997/100343181374 failed with a single Root cause confirmed as already-fixed, not a defect in this PR: this branch's previous head ( Fix applied: Verification (Python 3.11 sandbox venv, hash-locked
PR is now caught up with Generated by Claude Code |
ProviderResponseError.detail was a read-only property that rebuilt a fresh dict on every read. Sibling changes give the class a caller-owned detail: #1004 assigns and mutates it in __init__ (`self.detail["workflow_run_id"] = ...`) and #976 sets and reads `provider_response_failure_kind`. Merged with either, the property raised at construction (no setter) or silently dropped item writes (fresh dict per read). detail is now backed by a stored dict returned on every read, with a setter that adopts a caller-supplied dict; attempts/stop_reason are mirrored into the stored dict on read so failover evidence stays visible and never goes stale, matching the contract pinned by tests/test_provider_response_error_detail_compat.py (red on the previous head: "property 'detail' ... has no setter"). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2 participants
Summary
provider_response_failure_kinddetail forProviderResponseErrorinvalid_structured_outputcontract while surfacing the new bounded detail in HTTP responsesTesting
uv run pytest -q tests/test_provider_reliability.py tests/test_chat_response_format_http_honesty.pyuv run pytest -q tests/test_provider_error_taxonomy.pyFixes #932.
Summary by CodeRabbit
개선 사항
테스트