Conversation
tonydwb
left a comment
There was a problem hiding this comment.
LGTM — Bounds Codex error response reads to 1MB max with a 500-char preview. Clean implementation with good test coverage (bounded preview test confirms truncation behavior).
98d53fd to
dd7cce6
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the Codex streamed-error path; the current-head premise is valid: plugins/image_gen/openai-codex/__init__.py:395-396 still fully reads the error body before truncating its displayed preview.
Problems
- The new
iter_bytes()loop in the PR has a byte cap but no hard deadline. A response that yields a prefix and then stalls remains blocked until the Codex client's 300-second read timeout (plugins/image_gen/openai-codex/__init__.py:387). - Current main already has
agent.bounded_response.read_streaming_error_body()(agent/bounded_response.py:56-125), which caps bytes and closes stalled responses after a hard deadline. The new local reader duplicates that responsibility with weaker behavior.
Suggested changes
- Reuse
read_streaming_error_body()from_collect_image_b64(), preserving the 500-character user-facing preview. Extend that shared helper if an explicit truncation marker is required. - Add a provider-level non-2xx streaming regression test; the shared helper already has real-socket oversize/stall coverage in
tests/agent/test_bounded_response.py:107-129.
Automated hermes-sweeper review.
| def _codex_error_response_preview(response: Any) -> str: | ||
| """Read a bounded preview from a streamed Codex error response.""" | ||
| chunks: List[bytes] = [] | ||
| total = 0 |
There was a problem hiding this comment.
iter_bytes() can block inside the socket read after a partial error body arrives, so this loop does not bound latency. Please reuse agent.bounded_response.read_streaming_error_body(), which applies both a byte cap and a hard deadline by closing stalled responses.
|
Heads-up: the error-preview code this PR targets changed on That PR removed the account-capability classifier from That does not resolve what you reported here. Your point stands and is untouched: we still call Only change needed on your side: the truncation now happens in Sorry for the churn, and thanks for the precise write-up on both this and #55248 — the scoping made the overlap easy to check. |
dd7cce6 to
95582a0
Compare
|
Landed in #111000 (3275ca8). The |
Summary
response.read()on non-2xx Codex Responses API errors before truncating the displayed messageFixes #54841
Validation
uv run --extra dev python -m pytest tests\plugins\image_gen\test_openai_codex_provider.py -q --basetemp .pytest-tmp-codex-image-error-preview(19 passed)git diff --checkautoreviewhelper unavailable locally (autoreview,agent-autoreview, andcodex-autoreviewnot found)