fix(core): preserve Responses usage and failure semantics - #135
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| except StreamedAPIError as e: | ||
| last_exception = e | ||
| original = getattr(e, "data", e) | ||
| original = getattr(e, "data", None) or e |
There was a problem hiding this comment.
Using or discards valid falsey error payloads; fall back to the exception only when data is None.
| original = getattr(e, "data", None) or e | |
| original = getattr(e, "data", None) | |
| if original is None: | |
| original = e |
#ai-review-inline
| image["detail"] = image_url["detail"] | ||
| parts.append(image) | ||
| elif ptype == "input_image": | ||
| parts.append({"type": "input_image", "image_url": part.get("image_url", "")}) |
There was a problem hiding this comment.
The input_image path still drops a top-level detail value, so preserve it here for consistent image handling.
| parts.append({"type": "input_image", "image_url": part.get("image_url", "")}) | |
| image = {"type": "input_image", "image_url": part.get("image_url", "")} | |
| if part.get("detail") is not None: | |
| image["detail"] = part["detail"] | |
| parts.append(image) |
#ai-review-inline
| usage.prompt_tokens_details = { | ||
| "cached_tokens": cached, | ||
| } | ||
| cached = input_details.get("cached_tokens") |
There was a problem hiding this comment.
Extract the duplicated usage-detail mapping into a helper so the streaming and non-streaming paths cannot drift.
#ai-review-inline
| f"applying {cooldown_duration:.0f}s cooldown (backoff={backoff})" | ||
| ) | ||
|
|
||
| elif error.error_type == "model_not_supported": |
There was a problem hiding this comment.
Remove the temporary # added annotations to keep production code clean and consistent.
| elif error.error_type == "model_not_supported": | |
| elif error.error_type == "model_not_supported": | |
| # Do not block the shared Codex quota group or unrelated models. | |
| group_key = normalized_model | |
| cooldown_duration = cooldown_duration or 900 |
#ai-review-inline
|
|
||
| error_text = str(e) | ||
| if isinstance(e, httpx.HTTPStatusError): | ||
| error_text += " " + e.response.text |
There was a problem hiding this comment.
Accessing response.text can raise httpx.ResponseNotRead for streaming responses, causing the error classifier itself to fail.
| error_text += " " + e.response.text | |
| if isinstance(e, httpx.HTTPStatusError): | |
| try: | |
| error_text += " " + e.response.text | |
| except httpx.ResponseNotRead: | |
| pass |
#ai-review-inline
| Returns: | ||
| ClassifiedError with error_type, status_code, retry_after, etc. | ||
| """ | ||
| from .core.errors import StreamedAPIError |
There was a problem hiding this comment.
Remove the temporary # added annotations throughout this block because they add noise and duplicate version-control history.
#ai-review-inline
| from .core.errors import StreamedAPIError | ||
|
|
||
| if isinstance(e, StreamedAPIError) and getattr(e, "data", None) is not None: | ||
| return classify_error(e.data, provider) |
There was a problem hiding this comment.
Do not return an unknown payload classification unconditionally, because this bypasses the StreamedAPIError-specific handling below and loses the original wrapper exception.
#ai-review-inline
| # Check if cooldown qualifies as exhaustion | ||
| cooldown_duration = cooldown_until - now | ||
| if cooldown_duration >= self._config.exhaustion_cooldown_threshold: | ||
| if reason != "model_not_supported" and cooldown_duration >= self._config.exhaustion_cooldown_threshold: |
There was a problem hiding this comment.
Avoid a magic string here; use the shared reason enum or constant so renames or spelling changes do not silently re-enable exhaustion.
#ai-review-inline
| # Check if cooldown qualifies as exhaustion | ||
| cooldown_duration = cooldown_until - now | ||
| if cooldown_duration >= self._config.exhaustion_cooldown_threshold: | ||
| if reason != "model_not_supported" and cooldown_duration >= self._config.exhaustion_cooldown_threshold: |
There was a problem hiding this comment.
Remove the temporary '# added' annotation because version control already documents this change.
| if reason != "model_not_supported" and cooldown_duration >= self._config.exhaustion_cooldown_threshold: | |
| if reason != "model_not_supported" and cooldown_duration >= self._config.exhaustion_cooldown_threshold: |
#ai-review-inline
|
|
||
|
|
||
| def events(raw): | ||
| return [json.loads(line[5:].strip()) for line in raw.splitlines() if line.startswith("data:")] |
There was a problem hiding this comment.
Skip the standard SSE [DONE] sentinel so this helper does not attempt to parse it as JSON.
| return [json.loads(line[5:].strip()) for line in raw.splitlines() if line.startswith("data:")] | |
| parsed = [] | |
| for line in raw.splitlines(): | |
| if not line.startswith("data:"): | |
| continue | |
| payload = line[5:].strip() | |
| if payload and payload != "[DONE]": | |
| parsed.append(json.loads(payload)) | |
| return parsed |
#ai-review-inline
| "content": output, | ||
| }) | ||
| if images: | ||
| messages.append({"role": "user", "content": images}) |
There was a problem hiding this comment.
Appending a user message here can place it between responses to parallel tool calls and violate Chat Completions ordering, so buffer lifted images until all contiguous tool messages are emitted.
#ai-review-inline
| result = { | ||
| "input_tokens": usage.get("prompt_tokens", 0), | ||
| "output_tokens": usage.get("completion_tokens", 0), | ||
| "total_tokens": usage.get("total_tokens", 0), |
There was a problem hiding this comment.
When an OpenAI-compatible provider omits total_tokens, derive it from prompt and completion tokens instead of reporting an inconsistent zero.
| "total_tokens": usage.get("total_tokens", 0), | |
| "total_tokens": usage.get("total_tokens") if usage.get("total_tokens") is not None else (usage.get("prompt_tokens") or 0) + (usage.get("completion_tokens") or 0), |
#ai-review-inline
| # user message, preserving the tool result ID and never tokenizing | ||
| # image bytes as JSON text. Codex converts these back to input_image. | ||
| images = [] | ||
| if isinstance(output, list) and any( |
There was a problem hiding this comment.
Selecting this branch when any image exists silently drops unsupported siblings such as input_file content, so preserve or explicitly serialize every non-image part.
#ai-review-inline
|
|
||
| # Test finalization | ||
| # Test finalization after the upstream finish marker. | ||
| converter.convert_chunk('data: {"choices":[{"delta":{},"finish_reason":"tool_calls"}]}') |
There was a problem hiding this comment.
Assert that this finish-marker call emits no final events, otherwise its discarded return value can hide premature finalization.
#ai-review-inline
| converter.convert_chunk(f"data: {json.dumps(chunk4)}") | ||
|
|
||
| # Finalize | ||
| converter.convert_chunk('data: {"choices":[{"delta":{},"finish_reason":"tool_calls"}]}') |
There was a problem hiding this comment.
Assert that this finish-marker call emits no final events so the test clearly verifies that finalization occurs on [DONE].
#ai-review-inline
| assert "response.reasoning_summary_part.added" not in event_types3 | ||
|
|
||
| # Finalize — should emit summary_text.done, summary_part.done, output_item.done | ||
| converter.convert_chunk('data: {"choices":[{"delta":{},"finish_reason":"stop"}]}') |
There was a problem hiding this comment.
Assert that this finish-marker call emits no final events, otherwise the test may discard the events it intends to validate after [DONE].
#ai-review-inline
|
The changes substantially improve Responses API integrity with idempotent terminal stream handling, richer usage preservation, multimodal tool outputs, and more precise Codex error/cooldown behavior, backed by broad regression tests. One robustness improvement remains: classify_error accesses HTTPStatusError.response.text directly, which can raise ResponseNotRead for unconsumed streaming responses; safely attempting to read the body or guarding that access would prevent error classification itself from failing. #ai-review-summary |
claw-io
left a comment
claw-io
left a comment
There was a problem hiding this comment.
Code Review: PR #135 (fix(core): preserve Responses usage and failure semantics)
While this PR makes significant progress toward Responses API terminal idempotency and usage fidelity, several critical protocol invariants and error handling gaps require changes before merging.
1. Critical: Truncated Codex Streams Treated as Clean Success
- Location:
src/rotator_library/providers/codex_provider.py:939-1141 - Issue: In
_parse_response_events(), if the upstream connection or WebSocket abruptly disconnects without an explicit terminal event (response.completed,response.incomplete,response.failed), the generator simply finishes (StopAsyncIteration). Downstream,wrap_stream()interprets this as a normal clean completion, callsmark_success(), and emits[DONE]. - Impact: Premature termination is presented to clients as success, usage tracking records false success, and rotation/failover is bypassed.
- Required Fix: Track whether an explicit terminal event was observed. If the event stream terminates prematurely, raise a
StreamedAPIErrorwithcode="incomplete_stream".
2. Critical: Structured Error Data Stripped at Provider Boundary
- Location:
src/rotator_library/providers/codex_provider.py:1128-1141,2005-2011 - Issue: When Codex returns
response.failedor a WebSocket error containing machine-readable attributes (code,type,status), the provider extracts onlymessageand raisesStreamedAPIError(message)without setting thedataattribute. - Impact: Downstream error classification cannot inspect structured fields and falls back to brittle string heuristics.
- Required Fix: Attach the full error dictionary when raising:
error = evt.get("response", {}).get("error") or {} raise StreamedAPIError(error.get("message", "Response failed"), data={"error": error})
3. High: /v1/responses Catches and Converts HTTPExceptions to 500
- Location:
src/proxy_app/main.py:1182-1186, 1259-1265, 1295-1301 - Issue: The endpoint raises
HTTPException(400)for invalid JSON (line 1186) andHTTPException(429)for provider exhaustion (line 1265). However, these are not re-raised before the broadexcept Exception:block at line 1295, causing them to be returned to clients as genericHTTP 500 Internal Server Error. - Required Fix: Explicitly re-raise
HTTPExceptionbefore catching genericException:except HTTPException: raise except ProxyExhaustionError as e: ... except Exception as e: ...
4. High: Parallel Tool Call Multimodal Image Lifting Violates Chat Completions Sequencing
- Location:
src/proxy_app/responses_compat.py:109-136 - Issue: When an assistant executes multiple tool calls (
call_1,call_2) andcall_1returns an image, inserting ausermultimodal message immediately aftertool(call_1)splits the tool response block (tool -> user -> tool). - Impact: Upstream OpenAI Chat Completions endpoints reject this with an HTTP 400 validation error (tool messages must be contiguous).
- Required Fix: Buffer lifted multimodal images and emit the synthetic user message only after all contiguous
function_call_outputmessages have been appended.
5. Medium: Potential Unread Stream Crash in Error Classifier
- Location:
src/rotator_library/error_handler.py:926-930 - Issue:
e.response.textcan raisehttpx.ResponseNotReadfor unbuffered streaming responses. - Required Fix: Guard response text access:
if isinstance(e, httpx.HTTPStatusError): try: error_text += " " + e.response.text except httpx.ResponseNotRead: pass
6. Medium: Falsy Data Fallback in Executor
- Location:
src/rotator_library/client/executor.py:1158 - Issue:
original = getattr(e, "data", None) or ediscards valid empty dictionaries/payloads{}. - Required Fix: Fall back only if
original is None.
7. Medium: Responses API Schema Incompleteness
- Location:
src/proxy_app/responses_compat.py:346-347, 676-692 - Issue: When
status == "incomplete", the OpenAI Responses API schema expectsincomplete_details(e.g.{"reason": "max_output_tokens"}). - Required Fix: Populate
incomplete_detailswhen mapping truncation reasons.
3 participants
Related: #116, #131
Problem
The Responses compatibility route drops cached-token and reasoning-token usage details. It also serializes image-containing tool outputs as text, inflating input size.
Upstream errors and truncated streams can appear as empty successful responses. Context-overflow errors can unnecessarily rotate credentials, while account-specific model rejection can prevent trying another eligible credential.
Changes
response.failedevent for upstream errors, exceptions, or truncated streams.No model aliases, deployment configuration, dependency changes, or speculative session-affinity changes are included.
Verification
F401,F811,F821,E9), andgit diff --checkpassed.Equivalent production code was also checked with bounded direct HTTP requests:
These were direct HTTP canaries—not Pi E2E tests or proof that historical intermittent cache misses are resolved.
Limitations
Independent review and exact-head GitHub CI remain outstanding. Live overflow stress tests, invalid-credential probes, and exhaustive cross-provider multimodal tests were not run.