Skip to content

fix(responses): initialize null streaming output before accumulating items - #3881

Open
Chirag-Bhardwaj wants to merge 1 commit into
openai:mainfrom
Chirag-Bhardwaj:fix/responses-empty-initial-output
Open

Chirag-Bhardwaj wants to merge 1 commit into
openai:mainfrom
Chirag-Bhardwaj:fix/responses-empty-initial-output

Conversation

@Chirag-Bhardwaj

Copy link
Copy Markdown
  • I understand that this repository is auto-generated and my pull request may not be merged

Changes being requested

Normalize null or omitted response.created.response.output to an empty list in the Responses stream's internal snapshot. The index map already tolerates these values, but the snapshot retains None, so the next response.output_item.added raises AttributeError: '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:

  • Expanded regression matrix on unchanged main: 16 failed, 32 passed; with the fix: 48 passed.
  • Responses suite: 270 passed with each Pydantic version.
  • ./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, and git diff --check passed.
  • Main's unchanged custom-code budget checker passed; no policy or generation metadata changes.

@feiiiiii5 feiiiiii5 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 passed
  • pytest tests/lib/responses tests/test_streaming.py -> 311 passed, 12 skipped; all 12 skips are tests/test_streaming.py:30: requires the legacy HTTPX compatibility lane, unrelated to this change
  • ruff check and ruff format --check against 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants