fix(parsing): guard against None response.output in parse_response#3517
fix(parsing): guard against None response.output in parse_response#3517rkfshakti wants to merge 6 commits into
Conversation
The chatgpt.com Codex backend sometimes sends response.output: null in the consolidated response.completed event, even when valid output_item.done events were streamed earlier. The SDK then raises TypeError: 'NoneType' object is not iterable inside the stream accumulator, killing the entire stream before the consumer can read the deltas. Fix: iterate over response.output or [] instead of response.output directly. Closes openai#3325
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb666413b4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| output_list: List[ParsedResponseOutputItem[TextFormatT]] = [] | ||
|
|
||
| for output in response.output: | ||
| for output in response.output or []: |
There was a problem hiding this comment.
When this is called from ResponseStreamState.accumulate_event for a response.completed event whose event.response.output is None, response.output or [] turns the final ParsedResponse into an empty output list. The stream has already accumulated output items/text in snapshot.output, but get_final_response() and ResponseCompletedEvent use _completed_response from this parse, so these streams finish with response.output/output_text empty instead of the text or tool calls just emitted.
Useful? React with 👍 / 👎.
|
Thanks for reviewing! 👋 This is my first contribution to the OpenAI Python SDK. I'm actively contributing to GenAI open-source projects — you can see my other PRs at https://github.com/rkfshakti (chroma-core/chroma, langchain-ai/langchain, langgenius/dify, NousResearch/hermes-agent, firecrawl/firecrawl). The fix is minimal — Happy to add a test case if that would help. Thanks! |
…as null output The chatgpt.com Codex backend sometimes sends response.output: null in the consolidated response.completed event even when valid output_item.done events were streamed earlier (issue openai#3325). The previous fix (response.output or []) prevented the TypeError but discarded the already-streamed snapshot.output, causing the final ParsedResponse to have empty output/output_text. Move the guard into ResponseStreamState.accumulate_event: when event.response.output is None and the snapshot has accumulated output items, build the completed response from the snapshot instead of calling parse_response with an empty output list. This preserves streamed text and tool calls in get_final_response() and ResponseCompletedEvent. Addresses Codex review feedback on openai#3517.
|
Thanks for the Codex review — the P2 point about preserving streamed output is correct and I've pushed a fix ( Problem with the previous approach: New fix: Moved the guard into The Happy to add a streaming test that exercises the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c050f6988
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| self._completed_response = construct_type_unchecked( | ||
| type_=ParsedResponse[TextFormatT], | ||
| value={ | ||
| **event.response.to_dict(), | ||
| "output": [item.to_dict() for item in snapshot.output], | ||
| }, |
There was a problem hiding this comment.
When a streaming response.completed has response.output == None and the caller supplied a text_format or parseable/strict function tool, this fallback constructs the final ParsedResponse directly from snapshot.output instead of passing that output through parse_response. The streamed snapshot only accumulates raw text and raw function-call arguments, so get_final_response() and the ResponseCompletedEvent.response will preserve output_text but leave output_parsed/parsed_arguments as None in exactly the null-output case this branch handles.
Useful? React with 👍 / 👎.
| # Calling `parse_response` with a null `output` would discard the | ||
| # already-accumulated `snapshot.output` and emit an empty final | ||
| # response, so we fall back to the streamed snapshot in that case. | ||
| if event.response.output is None and snapshot.output: |
There was a problem hiding this comment.
If the backend sends response.output: null on response.completed before any output item has been accumulated, this condition is false and the else path still calls parse_response with event.response.output is None, reintroducing the same TypeError this fix is meant to avoid. This affects legitimately empty completed responses (or any null-output stream with no prior response.output_item.added) instead of returning a parsed response with an empty output list.
Useful? React with 👍 / 👎.
| type_=ParsedResponse[TextFormatT], | ||
| value={ | ||
| **event.response.to_dict(), | ||
| "output": [item.to_dict() for item in snapshot.output], |
There was a problem hiding this comment.
When response.completed has a null output after normal streamed *.done events, this fallback serializes snapshot.output, but the accumulator above never applies response.output_item.done, response.content_part.done, or response.function_call_arguments.done to that snapshot. As a result, get_final_response() can expose stale in_progress item statuses or miss final content metadata carried by the done events even though those finalized items were already streamed.
Useful? React with 👍 / 👎.
| type_=ParsedResponse[TextFormatT], | ||
| value={ | ||
| **event.response.to_dict(), | ||
| "output": [item.to_dict() for item in snapshot.output], |
There was a problem hiding this comment.
For streams that emit response.output_text.annotation.added events, such as citation-bearing responses, the accumulator never adds those annotations to snapshot.output; with a null completed output this fallback serializes that incomplete snapshot instead of a final API output. In that scenario get_final_response().output loses annotations that were already streamed, leaving citation/file annotation arrays empty even though consumers saw the annotation events.
Useful? React with 👍 / 👎.
Address Codex P2 review feedback on the previous commit (1c050f6): 1. Run response parsing on the streamed fallback: instead of building ParsedResponse directly from snapshot.output via construct_type_unchecked (which left output_parsed/parsed_arguments as None), inject the streamed items into a shallow copy of the response and pass it through parse_response so text_format and parsed_arguments logic still runs. 2. Handle null completed output without streamed items: re-add the 'response.output or []' guard in parse_response() so a null-output response.completed with no prior output_item.added events returns a parsed response with an empty output list instead of raising TypeError. Together these cover both branches: null output WITH accumulated snapshot items (parse_response runs on the injected items) and null output WITHOUT items (parse_response returns empty output gracefully).
|
Thanks for the re-review on 1. Run response parsing on the streamed fallback — The previous 2. Handle null completed output without streamed items — Re-added the 3 & 4. Apply done events / preserve annotations before serializing — Both of these were symptoms of the same root cause: the fallback was serializing the raw snapshot instead of going through The two branches now cover all cases:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 479179ed84
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| type_=type(event.response), | ||
| value={ | ||
| **event.response.to_dict(), | ||
| "output": [item.to_dict() for item in snapshot.output], |
There was a problem hiding this comment.
When response.completed has output: null, this fallback serializes snapshot.output, but that snapshot is only built from response.output_item.added, response.content_part.added, and deltas; accumulate_event never replaces it with the finalized response.output_item.done item or applies annotation/done events. In the exact backend scenario described here, any fields that are only finalized on those earlier events (for example message status, annotations/citations, or logprobs) are dropped from ResponseCompletedEvent.response / get_final_response(), even though the stream did provide them.
Useful? React with 👍 / 👎.
…t init httpx's get_environment_proxies() only splits NO_PROXY by comma, not by newline. When NO_PROXY contains newline characters (common in Docker environments, .env files, or shell scripts), the newline becomes part of the hostname and httpx raises InvalidURL. Add _sanitize_no_proxy() which replaces newlines with commas and strips whitespace, called from _DefaultHttpxClient.__init__() before the httpx client is constructed. Closes openai#3303.
…px client init" This reverts commit 217dc74.
When response.completed has output: null, the fallback serializes snapshot.output, but accumulate_event never applied done events (response.output_text.done, response.output_item.done, response.content_part.done, response.function_call_arguments.done) to the snapshot. This meant get_final_response() could expose stale in_progress statuses and miss finalized text in the null-output case. Add handlers in accumulate_event for all four done event types so the snapshot reflects the finalized state before the fallback serializes it. Addresses Codex P2 review feedback on commit 479179e.
|
Thanks for the continued review — the P2 point about done events not being applied to the snapshot is correct. Pushed Fix: Added handlers in
Previously these events only produced consumer-facing @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b097d55f6a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if event.output_index < len(snapshot.output): | ||
| item = snapshot.output[event.output_index] | ||
| if hasattr(item, "status"): | ||
| item.status = "completed" |
There was a problem hiding this comment.
When response.completed has output=None, the fallback now serializes snapshot.output as the final response. For response.output_item.done, the server sends the finalized event.item; tool items such as file_search_call or code_interpreter_call can gain final fields like results/outputs, and items can also finish as failed or incomplete. This handler discards that payload and only forces status to completed, so the null-output fallback can return stale or misleading final items even though the done event contained the correct item.
Useful? React with 👍 / 👎.
| output = snapshot.output[event.output_index] | ||
| if output.type == "message" and event.content_index < len(output.content): | ||
| part = output.content[event.content_index] | ||
| if hasattr(part, "status"): | ||
| part.status = "completed" |
There was a problem hiding this comment.
When the completed event has output=None, the final response is built from snapshot.output, but response.content_part.done carries the finalized event.part and this branch only checks for a status attribute on the existing part. Output text/refusal parts do not have that status field, and metadata such as annotations/logprobs is not accumulated elsewhere, so responses with citations or final part metadata can lose it in the null-output fallback path.
Useful? React with 👍 / 👎.
| output = snapshot.output[event.output_index] | ||
| if output.type == "function_call": | ||
| if hasattr(output, "status"): | ||
| output.status = "completed" |
There was a problem hiding this comment.
In the null-output fallback path, parse_response() parses tool arguments from snapshot.output, but this done handler ignores event.arguments, the finalized argument string sent by response.function_call_arguments.done. If the accumulated deltas are incomplete or differ from the final payload, the final response can expose stale arguments and fail or mis-parse parsed_arguments even though the stream included the authoritative arguments.
Useful? React with 👍 / 👎.
1 participant
Problem
The chatgpt.com Codex backend sometimes sends
response.output: nullin the consolidatedresponse.completedevent, even when validoutput_item.doneevents were streamed earlier. The SDK then raisesTypeError: 'NoneType' object is not iterableinside the stream accumulator, killing the entire stream before the consumer can read the deltas.Closes #3325.
Fix
Change
for output in response.outputtofor output in response.output or []so thatNoneis handled gracefully (empty iteration instead of TypeError).Testing
python -c "import ast; ast.parse(open('src/openai/lib/_parsing/_responses.py').read())"passesparse_response(response=Response(output=None, ...))no longer raises TypeErrorChecklist