fix(responses): initialize null streaming output before accumulating items - #3881
Chirag-Bhardwaj wants to merge 1 commit into
Conversation
feiiiiii5
left a comment
There was a problem hiding this comment.
Measured against openai/openai-python at base b77076d23b6f and this head 6095bb5f9277, same venv for both; each run printed openai.__file__ so the base numbers really come from the base tree.
The failure set pins the trigger, and it matches the mechanism. Applying only this PR's test file to base:
16 failed, 32 passed in 2.53s
All 16 are test_stream_recovers_finalized_output[with-items-<terminal>-<null|missing>-<sync|async>]: has_items=True, terminal output in any of the four shapes, and response.output either null or absent in response.created. Every without-items case and every initial_output=empty case passes at base.
That is what the code predicts. At base _create_initial_response (:392) defaults only the local argument — dict(enumerate(snapshot.output or [])) at :398 — so the snapshot it stores still carries output = None. The crash surfaces later, in accumulate_event, whose response.output_item.added arm calls snapshot.output.append(...) at :340, :346 and :350:
AttributeError: 'NoneType' object has no attribute 'append'
So snapshot.output = [] is the load-bearing line: defaulting only the enumerate call would still leave the stored snapshot broken for every consumer. Normalizing once at construction also covers the :350 arm and any later read instead of patching each append, and dropping the now-unreachable or [] on the next line keeps that consistent.
Head, same environment:
pytest tests/lib/responses/test_null_output.py->48 passedpytest tests/lib/responses tests/test_streaming.py->311 passed, 12 skipped; all 12 skips aretests/test_streaming.py:30: requires the legacy HTTPX compatibility lane, unrelated to this changeruff checkandruff format --checkagainst the repo's[tool.ruff]config on both touched files -> clean (ruff 0.14.7, which is not necessarily the locked version)
I did not run scripts/run-pyright or mypy, and CI has not executed on this head: commits/6095bb5f9277.../check-runs returns 0 check runs and the four workflow runs for this sha are all action_required, pending maintainer approval. mergeable_state=blocked reflects that gate, not a conflict (mergeable=true).
One question rather than a request. ParsedResponse.output is declared List[ParsedResponseOutputItem[ContentType]] at src/openai/types/responses/parsed_response.py:112 and :114, never Optional, so per the annotations both the old or [] and the new is None branch are unreachable — the getattr(snapshot, "output", None) form reads as a deliberate way to keep pyright strict from flagging an unnecessary comparison. Since the stream evidently can carry "output": null, is the intent to make the codegen field Optional, or to keep normalizing at the streaming boundary? #3345 took the boundary route on the parsing side and this extends it to stream initialization, so either answer leaves this PR in the right shape.
Nothing here I'd ask the author to change.
Changes being requested
Normalize null or omitted
response.created.response.outputto an empty list in the Responses stream's internal snapshot. The index map already tolerates these values, but the snapshot retainsNone, so the nextresponse.output_item.addedraisesAttributeError: 'NoneType' object has no attribute 'append'in both sync and async streams.This extends the existing null-output handling from #3345 to stream initialization. The change is confined to the handwritten streaming helper. Existing output lists and terminal-response precedence are preserved.
The existing public-client regression test now covers null, omitted, and empty initial output across sync/async streams, with and without items, and all four terminal-output variants. It verifies final structured text, function arguments, citations, refusals, status, and usage using synthetic SSE through
httpx2.MockTransport; no live API calls or credentials are required. This is a locally reproduced SDK edge case, not a claim that the production API currently emits this payload.Additional context & links
Validated on Python 3.10.16 against main
b77076d23b6f3e34453b0fadd8cd2a001627e365:./scripts/test -n 4: 13,209 passed / 162 skipped on Pydantic v2; 13,195 passed / 176 skipped on v1../scripts/lint: Ruff, Pyright, Mypy, and import check passed../scripts/build, changed-file formatting, andgit diff --checkpassed.