-
Notifications
You must be signed in to change notification settings - Fork 5.1k
fix(parsing): guard against None response.output in parse_response #3517
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
486a8f5
8a3d39a
8e55bf9
650fc64
860247e
b042e5a
63f9035
1a208f4
b5c9a6c
05071e1
7636d0e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |
|
|
||
| import inspect | ||
| from types import TracebackType | ||
| from typing import Any, List, Generic, Iterable, Awaitable, cast | ||
| from typing import Any, List, Generic, Iterable, Optional, Awaitable, cast | ||
| from typing_extensions import Self, Callable, Iterator, AsyncIterator | ||
|
|
||
| from ._types import ParsedResponseSnapshot | ||
|
|
@@ -15,9 +15,15 @@ | |
| ) | ||
| from ...._types import Omit, omit | ||
| from ...._utils import is_given, consume_sync_iterator, consume_async_iterator | ||
| from ...._compat import PYDANTIC_V1 | ||
| from ...._models import build, construct_type_unchecked | ||
| from ...._streaming import Stream, AsyncStream | ||
| from ....types.responses import ParsedResponse, ResponseStreamEvent as RawResponseStreamEvent | ||
| from ....types.responses import ( | ||
| Response, | ||
| ParsedResponse, | ||
| ResponseOutputItem, | ||
| ResponseStreamEvent as RawResponseStreamEvent, | ||
| ) | ||
| from ..._parsing._responses import TextFormatT, parse_text, parse_response | ||
| from ....types.responses.tool_param import ToolParam | ||
| from ....types.responses.parsed_response import ( | ||
|
|
@@ -330,22 +336,18 @@ def accumulate_event(self, event: RawResponseStreamEvent) -> ParsedResponseSnaps | |
| if event.type == "response.output_item.added": | ||
| if event.item.type == "function_call": | ||
| snapshot.output.append( | ||
| construct_type_unchecked( | ||
| type_=cast(Any, ParsedResponseFunctionToolCall), value=event.item.to_dict() | ||
| ) | ||
| construct_type_unchecked(type_=cast(Any, ParsedResponseFunctionToolCall), value=event.item) | ||
| ) | ||
| elif event.item.type == "message": | ||
| snapshot.output.append( | ||
| construct_type_unchecked(type_=cast(Any, ParsedResponseOutputMessage), value=event.item.to_dict()) | ||
| construct_type_unchecked(type_=cast(Any, ParsedResponseOutputMessage), value=event.item) | ||
| ) | ||
| else: | ||
| snapshot.output.append(event.item) | ||
| elif event.type == "response.content_part.added": | ||
| output = snapshot.output[event.output_index] | ||
| if output.type == "message": | ||
| output.content.append( | ||
| construct_type_unchecked(type_=cast(Any, ParsedContent), value=event.part.to_dict()) | ||
| ) | ||
| output.content.append(construct_type_unchecked(type_=cast(Any, ParsedContent), value=event.part)) | ||
| elif event.type == "response.output_text.delta": | ||
| output = snapshot.output[event.output_index] | ||
| if output.type == "message": | ||
|
|
@@ -356,12 +358,120 @@ def accumulate_event(self, event: RawResponseStreamEvent) -> ParsedResponseSnaps | |
| output = snapshot.output[event.output_index] | ||
| if output.type == "function_call": | ||
| output.arguments += event.delta | ||
| elif event.type == "response.output_text.done": | ||
| output = snapshot.output[event.output_index] | ||
| if output.type == "message": | ||
| content = output.content[event.content_index] | ||
| assert content.type == "output_text" | ||
| content.text = event.text | ||
| elif event.type == "response.output_item.done": | ||
| # Replace the item in the snapshot with the finalized item from the | ||
| # done event. The server sends the authoritative item payload here, | ||
| # which may include final fields like `results`/`outputs` on tool | ||
| # items, or a final `status` of `failed`/`incomplete`. Simply | ||
| # setting `status = "completed"` would discard those fields and | ||
| # produce a stale final response in the null-output fallback path. | ||
| # | ||
| # Use event.item directly instead of event.item.to_dict() — | ||
| # construct_type_unchecked is shallow, so round-tripping through | ||
| # to_dict() would leave nested content parts as plain dicts, | ||
| # causing parse_response() to crash on output.content[].type. | ||
| if event.output_index < len(snapshot.output): | ||
| snapshot.output[event.output_index] = construct_type_unchecked( | ||
| type_=type(snapshot.output[event.output_index]), | ||
| value=event.item, | ||
| ) | ||
| elif event.type == "response.content_part.done": | ||
| # Replace the content part in the snapshot with the finalized part | ||
| # from the done event. The server sends the authoritative part | ||
| # payload here, which may include metadata like annotations, | ||
| # logprobs, or finalized text/refusal content that the delta | ||
| # accumulation may not fully capture. | ||
| # | ||
| # Use event.part directly instead of event.part.to_dict() for the | ||
| # same shallow-construction reason as output_item.done above. | ||
| output = snapshot.output[event.output_index] | ||
| if output.type == "message" and event.content_index < len(output.content): | ||
| output.content[event.content_index] = construct_type_unchecked( | ||
| type_=type(output.content[event.content_index]), | ||
| value=event.part, | ||
| ) | ||
| elif event.type == "response.function_call_arguments.done": | ||
| # Apply the finalized arguments string from the done event. | ||
| # The server sends the authoritative arguments payload here, which | ||
| # may differ from the accumulated deltas. Using the finalized | ||
| # arguments ensures `parse_response()` can correctly parse | ||
| # `parsed_arguments` in the null-output fallback path. | ||
| output = snapshot.output[event.output_index] | ||
| if output.type == "function_call": | ||
| output.arguments = event.arguments | ||
| if hasattr(output, "status"): | ||
| output.status = "completed" | ||
|
Comment on lines
+405
to
+409
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In the null-output fallback path, Useful? React with 👍 / 👎. |
||
| elif event.type == "response.completed": | ||
| self._completed_response = parse_response( | ||
| text_format=self._text_format, | ||
| response=event.response, | ||
| input_tools=self._input_tools, | ||
| ) | ||
| # 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 (see issue #3325). | ||
| # `parse_response()` guards against `None` with `response.output or []`, | ||
| # but that would discard the already-accumulated `snapshot.output` and | ||
| # emit an empty final response. When the completed event has no | ||
| # output but the snapshot has accumulated items, inject the streamed | ||
| # items into a shallow copy of the response so `parse_response()` can | ||
| # still run its text_format / parsed_arguments logic on them. | ||
| # | ||
| # `output` is typed as non-nullable but the wire value can violate | ||
| # that contract; cast to `Optional[List[...]]` at the boundary so | ||
| # both Pyright and Mypy accept the None check without weakening | ||
| # the model contract. | ||
| # | ||
| # In the default streaming path, SSE data is converted through | ||
| # `construct_type(...)`. For a `response.completed` payload whose | ||
| # nested `response.output` is `null`, validation of the nested | ||
| # `Response` can fail and the discriminator fallback | ||
| # shallow-constructs the event, leaving `event.response` as the | ||
| # raw dict. Normalize it to a `Response` model before | ||
| # dereferencing `.output` so the guard doesn't raise | ||
| # `AttributeError` before the fallback can run. | ||
| response = event.response | ||
| if not isinstance(response, Response): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In this Useful? React with 👍 / 👎. |
||
| response = construct_type_unchecked(type_=Response, value=response) | ||
| output = cast(Optional[List[ResponseOutputItem]], response.output) | ||
| if output is None and snapshot.output: | ||
| # Build a copy of the response with the accumulated output | ||
| # items injected. Use warnings=False on Pydantic v2 to suppress | ||
| # the serializer warning from dumping the invalid null output | ||
| # field (the repo's pytest config treats warnings as errors). | ||
| # On Pydantic v1, warnings=False is not supported, so we build | ||
| # the dict without dumping the invalid field — exclude_unset | ||
| # skips the null output entirely. | ||
| if PYDANTIC_V1: | ||
| base_dict = response.to_dict() | ||
| else: | ||
| base_dict = response.to_dict(warnings=False) # type: ignore[call-arg] | ||
| response_with_output = construct_type_unchecked( | ||
| type_=type(response), | ||
| value=cast( | ||
| Any, | ||
| { | ||
| **base_dict, | ||
| # Preserve the accumulated model objects directly | ||
| # instead of converting to dicts — construct_type_unchecked | ||
| # is shallow, so dicts would stay dicts and | ||
| # parse_response() would crash on `output.type`. | ||
| "output": list(snapshot.output), | ||
| }, | ||
| ), | ||
| ) | ||
| self._completed_response = parse_response( | ||
| text_format=self._text_format, | ||
| response=response_with_output, | ||
| input_tools=self._input_tools, | ||
| ) | ||
| else: | ||
| self._completed_response = parse_response( | ||
| text_format=self._text_format, | ||
| response=response, | ||
| input_tools=self._input_tools, | ||
| ) | ||
|
|
||
| return snapshot | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a caller retains the
response.output_item.addedevent, this now stores the exact sameevent.itemobject in the accumulator; subsequentresponse.content_part.added, text deltas, or argument deltas mutatesnapshot.output, so the already-yielded event payload changes from its original in-progress/empty state to later content or arguments. The previousto_dict()path kept the accumulator separate from streamed event objects, so please copy/reconstruct the item before mutating it.Useful? React with 👍 / 👎.