fix: preserve finalized output on null response completion - #3345
Conversation
parse_response iterates response.output directly. Response.output is typed as a required list, but the SDK constructs models leniently, so when a backend emits a terminal response.completed event with output null (seen with the Codex backend for incomplete responses) the field becomes None and the loop raises TypeError: 'NoneType' object is not iterable. This is the streaming responses.stream() / responses.parse() crash path. Guard the loop with `response.output or []` so a response with no output items parses to an empty output list.
![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: 1f53041a27
ℹ️ 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.
In the streaming path, ResponseStreamState.accumulate_event builds up a snapshot from response.output_item.added and delta events, but on response.completed it calls parse_response(response=event.response). When the backend sends the terminal event with output: null after earlier streamed output items—the case described by this test—this coercion turns the final parsed response into output == [], so get_final_response() and the emitted response.completed event silently lose the text/tool calls already accumulated in the stream instead of parsing from the snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 859e182. The stream now retains finalized response.output_item.done payloads by output index. When completion output is null or missing, it supplies those items to a shallow copy of the terminal response and runs the existing parser. This preserves final statuses, annotations, refusals, structured output, tool arguments, and completion metadata without reconstructing final items from partial deltas. Explicit completion output, including [], remains authoritative.
Regression coverage exercises the public sync and async streaming APIs with null, missing, empty, and supplied completion output, both with and without streamed items. The Responses suite passes with Pydantic v1 and v2 (48 tests each); Ruff, Pyright, and Mypy also pass. Recovery requires finalized item events; it does not promote unfinished deltas into completed output.
When a backend sends the terminal response.completed event with a null or empty output after streaming output items, the streaming path passed that response straight to parse_response, so the final response dropped the text and tool calls already accumulated in the snapshot. Fall back to the accumulated snapshot output in that case. Addresses the streaming review note on this PR.
|
Good catch on the streaming path. The first commit only guarded parse_response against a null output, which would have collapsed the final response to an empty list when items were streamed before a null-output completed event. I pushed a follow-up: in ResponseStreamState.accumulate_event, when the terminal response.completed event has no output but the snapshot already accumulated output items, it now falls back to the snapshot output before parsing, so streamed text and tool calls are preserved. Added a streaming test (test_streaming_completed_with_null_output_keeps_snapshot) that drives the state through created/output_item.added/content_part.added/output_text.delta and then a completed event with output null, asserting the accumulated text survives. Full tests/lib/responses suite passes. |
marcuswood-oai
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
marcuswood-oai
left a comment
marcuswood-oai
left a comment
There was a problem hiding this comment.
Reviewed the full diff at 859e182. The fix preserves finalized streamed output when the completion payload has null or missing output, while keeping explicit empty and populated output authoritative. The earlier automated review feedback is addressed, and the diff review found no security concerns.
Validation:
- 48 Responses tests passed under each of Pydantic v1 and v2, including sync/async null, missing, empty, and populated terminal output cases.
- Ruff, Pyright, Mypy, and import checks passed locally.
- All 40 live smoke checks passed: 20 against this PR and 20 against current main, covering sync/async requests, structured parsing, streaming, structured streaming, and function calls across Pydantic v1/v2.
No public signatures, types, or dependencies change, and no regression was observed in the tested paths. CI is still running; this approval does not authorize merging. Please leave the PR unmerged pending explicit maintainer instruction.
## Summary Fork PRs openai#3760 and openai#3345 have green Python CI but no required Castiron budget statuses: both the workflow-run association and commit-to-PR endpoint return no PRs. The trusted evaluator fails with `source run must identify exactly one current PR targeting main`, and the publisher returns without posting statuses. Fall back to listing open PRs by the source run's fork owner and branch, then retain the existing exact-head, target, freshness, and ambiguity checks. Apply the fallback to report computation, budget evaluation, and publication; update the reporter integrity digest. ## Validation - Regression tests reproduce the evaluator failure and missing statuses on unpatched main. - Patched read-only lookup resolves the exact current heads of both affected PRs. - Castiron reporter/budget suite: 53 tests, OK (1 optional compiler-contract test skipped), including the JavaScript publishers, pagination, stale heads, wrong targets, and ambiguous matches. - Ruff, Pyright, and mypy for `scripts/castiron`. The trusted handler runs from main. After this fix merges, rerun the Castiron custom-code workflows for openai#3760 and openai#3345 to publish their required statuses.
Castiron custom code✅ No new custom-code files detected. 36 mixed files remain; 0 existing customizations changed. Compared 36 existing customizations unchanged
A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 34419242671 --repo openai/openai-python \
--name castiron-custom-code-34419242671-1 --dir /tmp/castiron-custom-code-34419242671-1
git apply --stat /tmp/castiron-custom-code-34419242671-1/custom-code.patch
cat /tmp/castiron-custom-code-34419242671-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 397ea08d8cf151039c069c1173f5de57cff5c081 364745a3a0e3edf9392f46085018776a9400d893
python3 scripts/castiron/custom_code_report.py report \
--base 397ea08d8cf151039c069c1173f5de57cff5c081 \
--head 364745a3a0e3edf9392f46085018776a9400d893 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-364745a3a0e3
cat /tmp/castiron-custom-code-364745a3a0e3/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 364745a3a0
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| elif event.type == "response.output_item.done": | ||
| self._completed_output[event.output_index] = event.item |
There was a problem hiding this comment.
For every normal stream containing response.output_item.done events, this dictionary retains the complete finalized item models even after response.completed has been parsed, including when the terminal response already supplies authoritative output. Keeping the stream object alive therefore retains an extra full copy of potentially large message, tool, or image data; clear this recovery-only cache once completion parsing has consumed it.
AGENTS.md reference: AGENTS.md:L114-L119
Useful? React with 👍 / 👎.
openai#3345 fixed the null-output crash in parse_response() and the streaming accumulator, but response.output_text still iterates self.output directly and still appends content.text without checking it's not None, so output_text('output': null) still raises TypeError, and a partial/aborted output_text content block (text: null) crashes the str.join instead of just being skipped. Same fix shape as openai#3345: guard with `self.output or []`, and skip a content.text that's None rather than appending it.
2 participants
Summary
Responses parsing raised
TypeErrorwhen a response containedoutput: null. Treat null output as empty for standalone parsing, and recover finalizedresponse.output_item.doneitems when a streaming completion has null or missing output.Recovery preserves item order, final statuses, annotations, refusals, structured output, tool arguments, and completion metadata. Explicit completion output, including
[], remains authoritative. This addresses the review feedback about losing streamed output without adding per-item finalization branches.Fixes #3325.
Validation
maininto the contributor branch.