fix: guard against null output in parse_response and output_text property - #3404
fix: guard against null output in parse_response and output_text property#3404adhavan18 wants to merge 6 commits into
Conversation
…erty When the Responses API returns `output: null` in a `response.completed` event (observed with the chatgpt.com Codex backend), iterating `response.output` raised `TypeError: 'NoneType' object is not iterable`. A second related case is `output_text` content items whose `text` field is `null`, which caused `parse_response` to pass `None` to structured text parsing and `output_text` to concatenate `None` into the result. Fixes: - `parse_response` in `lib/_parsing/_responses.py`: iterate `response.output or []` so a null list is treated as empty. - Same function: skip structured parsing for `output_text` items where `item.text is None`; set `parsed=None` for those items instead. - `Response.output_text` property in `types/responses/response.py`: iterate `self.output or []` and skip content items with `text is None`. Adds four regression tests covering all three cases. Closes openai#3325, openai#3063
![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: 49ab38b624
ℹ️ 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 streamed Responses path, ResponseStreamState.accumulate_event parses event.response on response.completed (src/openai/lib/streaming/responses/_responses.py:359-364) rather than the accumulated snapshot, so when that final completed event has output=None after earlier output item/text delta events, this new fallback turns the final parsed response into output=[]. In that backend case, stream.get_final_response().output_text and the emitted completed event become empty even though the stream already received valid text; the null completed payload should fall back to the accumulated snapshot instead of discarding it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This was accurate against the commit it was written on, and it is the reason the streaming file is in this PR at all. Addressed in bbd1830, pushed about an hour and a half after this comment.
accumulate_event now patches the completed event with the accumulated snapshot before parsing, rather than letting parse_response iterate a null output:
completed_response = event.response
if completed_response.output is None and snapshot is not None:
completed_response = build(
type(completed_response),
**{**completed_response.to_dict(), "output": [item.to_dict() for item in snapshot.output]},
)
self._completed_response = parse_response(
text_format=self._text_format,
response=completed_response,
input_tools=self._input_tools,
)So on a null completed payload the final ParsedResponse carries the streamed items rather than [], which is what you asked for. build is imported at the top of that module, and tests/lib/responses/test_null_output.py covers the case.
The response.output or [] guard you commented on stays as the last line of defence for the non-streaming path, where there is no snapshot to fall back to.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7928863ad7
ℹ️ 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".
| """Regression tests for null-output edge cases in the Responses API.""" | ||
| from __future__ import annotations | ||
|
|
||
| import pytest |
There was a problem hiding this comment.
With the repository lint target running ruff check . and selecting F401, this new test file fails before the regression tests can run because pytest is imported but never used; I confirmed ruff check tests/lib/responses/test_null_output.py reports F401 here. Removing the unused import and formatting the import blocks keeps ./scripts/lint green.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in de0ac7b. You were right, and it was worse than just the unused import: ruff check also flagged three I001 import-order errors in the same file.
tests/lib/responses/test_null_output.py:
2:1 I001 Import block is un-sorted or un-formatted
4:8 F401 `pytest` imported but unused
52:5 I001 Import block is un-sorted or un-formatted
83:5 I001 Import block is un-sorted or un-formatted
Found 4 errors.
ruff check . across the repo is clean now.
| """ | ||
| texts: List[str] = [] | ||
| for output in self.output: | ||
| for output in self.output or []: |
There was a problem hiding this comment.
When a client opts into _strict_response_validation=True, a /responses payload with output: null is rejected in _base_client.py via validate_type() before this output_text guard can ever run, because the model still declares output as a non-optional List[ResponseOutputItem]. That means the null-output backend case this change is trying to tolerate still crashes for strict clients; normalize the payload before validation or make the response field accept None as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and I do not think it can be fixed from this PR.
output is declared non-optional here:
output: List[ResponseOutputItem]and this file opens with:
# File generated from our OpenAPI spec by Stainless. See CONTRIBUTING.md for details.
So making the field accept None means changing the OpenAPI spec, which is not something a contributor can do in this repository. The same applies to the sibling comment about ResponseOutputText.text.
That leaves this PR fixing the non-strict path, which is the default, while _strict_response_validation=True still raises earlier in validate_type(). That is a genuine remaining gap and I would rather it be stated in the PR than discovered later, so I have noted it in the description.
Worth flagging to a maintainer: this PR does edit the generated response.py to guard the output_text property. If that file is regenerated without the change being carried in the Stainless config, the guard will be dropped. If that is a concern, the property guard could move to a non-generated module instead, and I am happy to restructure it that way.
There was a problem hiding this comment.
Correcting one thing I said here: I claimed the guard could be dropped on regeneration. That is wrong. CONTRIBUTING states that manual modifications are persisted between generations, so the only cost is a possible merge conflict with generator changes, not a silent revert.
The substantive point stands unchanged: Response.output is generated as non-optional, so a strict client still fails in validate_type() before reaching this guard, and closing that gap needs an OpenAPI spec change rather than an edit here.
| if output.type == "message": | ||
| for content in output.content: | ||
| if content.type == "output_text": | ||
| if content.type == "output_text" and content.text is not None: |
There was a problem hiding this comment.
For clients using _strict_response_validation=True, a response containing an output_text block with text: null is still rejected while validating ResponseOutputText.text: str, so execution never reaches this new content.text is not None guard. Since the change explicitly tries to tolerate null text in content blocks, strict clients still see the same crash unless the payload is normalized before validation or the generated text field accepts None.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same root cause as the output comment above: ResponseOutputText.text is typed str in a Stainless-generated file, so a strict client rejects text: null in validate_type() before reaching this guard. Changing that field needs an OpenAPI spec change rather than an edit here.
The guard in this PR still covers the default non-strict path, where text: null currently concatenates None into the aggregated string.
| if completed_response.output is None and snapshot is not None: | ||
| completed_response = build( | ||
| type(completed_response), | ||
| **{**completed_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, copying snapshot.output here can return stale items because the accumulator above only records output_item.added, content_part.added, and deltas, while the stream also carries final response.output_item.done / response.content_part.done payloads. In that null-completed streaming case, get_final_response() can expose in_progress item statuses or miss final part data such as annotations/logprobs even though the done events already provided them; update the snapshot from the done events before using it as the completed response output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and worth separating from this change: the accumulator has never consumed response.output_item.done or response.content_part.done. That is true on main today, independently of this PR:
$ git show main:src/openai/lib/streaming/responses/_responses.py | grep -c 'output_item\.done\|content_part\.done'
0
accumulate_event handles output_item.added, content_part.added, the two delta events, and completed. So a snapshot built from those can carry in_progress statuses and miss annotations or logprobs regardless of what this PR does.
What changes here is only what happens when completed arrives with output: null. Today that yields output=[] and every streamed item is lost. With the fallback it yields the accumulated items, which may have stale status. Partial data with a stale status seems clearly better than no data, so I would rather land this and treat "consume the done events in the accumulator" as its own change, since it affects every stream and not just the null-completed case.
Happy to open a separate issue for it, or to fold it in here if you would prefer one PR.
ruff was reporting F401 for the unused pytest import plus three I001 import-order errors in this file, which fails scripts/lint before the regression tests get a chance to run. ruff check . is clean now.
|
Status summary, since this has picked up a few review rounds. Everything raised is now answered or fixed.
The lint break was real and I had missed it. One decision I would like your call on before this goes further. The If that file is regenerated and the change is not carried in the Stainless config, the guard is silently dropped and the bug returns with no failing test to catch it. Two options, and I am happy to do either:
I lean towards option 2 because it survives regeneration without anyone needing to remember, but it is your codebase and your generation setup, so I would rather ask than guess. Related note for the same reason: the strict-validation gap the reviewer raised cannot be closed from this PR at all, since |
|
Correction to my previous comment: disregard the question about moving the
So the guard is not at risk of being silently dropped; manual patches persist. The worst case is a merge conflict when the generator touches the same lines, which is a normal maintenance cost rather than a correctness risk. No decision is needed from you on that point and no restructuring is proposed. I have corrected the PR description too. The rest of the previous comment stands: the lint break is fixed, |
The lint job was failing on pyright with 12 errors across the four files. Three of them are the defensive guards themselves: output and text are generated as non-optional, so pyright reads `is not None` as a comparison that can never be false. That is exactly the backend behaviour this PR exists to tolerate, so each guard now carries a targeted `pyright: ignore[reportUnnecessaryComparison]` with a comment saying why, rather than being removed. build() returns an untyped value, which made completed_response Unknown and leaked into the parse_response call. It is now annotated as Response and the build() result cast back to it. The rest were in the test: parse_response takes Omit rather than NotGiven since the merge with main, and the fixture helper had no parameter annotation. pyright is clean on all four files, ruff check and ruff format pass, and the null-output tests still pass.
The previous commit fixed pyright but broke mypy, which reported the branch as unreachable for the same reason: output is generated as non-optional, so `is None` looks impossible to both checkers. Rather than silencing them separately, the value is now read through getattr, which neither narrows. The `snapshot is not None` half of the condition was genuinely redundant, since accumulate_event returns early when snapshot is None, so it is gone. to_dict() returns dict[str, object], so the ** spread could not be checked field by field against Response; it is now built as an explicit dict[str, Any]. That also made build() resolve to Response, so the cast is no longer needed. Test functions gained return annotations, which mypy requires. mypy and pyright are both clean on the four changed files, ruff check and ruff format pass, and the null-output tests pass.
1 participant
Fixes #3325 and #3063.
Problem
Two crashes in the Responses API when the server returns unexpected null values:
output: nullin the finalresponse.completedevent even when output items were already streamed.parse_responseandResponse.output_textboth iterateresponse.outputdirectly →TypeError: 'NoneType' object is not iterable.text: null,parse_responsepassesNoneto the structured parser andoutput_textconcatenatesNoneinto the string.Fix
_parsing/_responses.py— iterateresponse.output or []; skip structured parsing whenitem.text is Nonetypes/responses/response.py— sameor []guard in theoutput_textproperty; skip content items wheretext is Nonelib/streaming/responses/_responses.py— whenresponse.completedarrives withoutput: null, fall back to the accumulated snapshot output instead of parsing an empty listTest
All existing tests in
tests/lib/responses/pass.Known limitation: strict response validation
This fixes the default, non-strict path. Clients using
_strict_response_validation=Trueare unaffected, becausevalidate_type()rejects the payload earlier:Response.outputis typedList[ResponseOutputItem]andResponseOutputText.textis typedstr, both in Stainless-generated files. AllowingNonethere needs an OpenAPI spec change rather than an edit in this repo.The
output_textguard does live in the generatedsrc/openai/types/responses/response.py. Per CONTRIBUTING that is supported: manual modifications are persisted between generations, though they can conflict with generator changes. So no restructuring is proposed here.