Skip to content

fix(responses): correct error message in finalResponse and safely accumulate string deltas - #2022

Open
hsusul wants to merge 2 commits into
openai:mainfrom
hsusul:fix/responses-final-response-error-message
Open

fix(responses): correct error message in finalResponse and safely accumulate string deltas#2022
hsusul wants to merge 2 commits into
openai:mainfrom
hsusul:fix/responses-final-response-error-message

Conversation

@hsusul

@hsusul hsusul commented Jul 27, 2026

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

Changes being requested

  • Fix error message thrown by ResponseStream.finalResponse() when a stream ends prematurely without producing a response (stream ended without producing a Response instead of stream ended without producing a ChatCompletion).
  • Fix typo in ResponseStream.finalResponse() docstring (REsponse -> Response).
  • Safely handle nullish initial values when accumulating string deltas in ResponseAccumulator.ts across output_text, refusal, function_call_arguments, reasoning_text, reasoning_summary_text, custom_tool_call_input, and mcp_call_arguments events using (target ?? '') + event.delta.
  • Add unit tests for finalResponse() error message and ResponseAccumulator string delta accumulation.

Additional context & links

Problem

  1. ResponseStream belongs to the Responses API. When finalResponse() is called on a stream that ended prematurely without producing a Response, it previously threw OpenAIError('stream ended without producing a ChatCompletion').
  2. When stream delta events (response.function_call_arguments.delta, response.refusal.delta, etc.) arrive on uninitialized string properties, += event.delta resulted in "undefined" prefixing the delta content.

Root cause

  1. Copy-paste typo from ChatCompletionStream.ts in ResponseStream.ts.
  2. Missing nullish fallback (str ?? '') on string property delta accumulation in ResponseAccumulator.ts.

Verification

  • pnpm test tests/lib/ResponseStream.test.ts (Pass)
  • pnpm build (Pass)
  • pnpm lint (Pass)

@hsusul
hsusul requested a review from a team as a code owner July 27, 2026 13:31

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5daeffe0fe

ℹ️ 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".

Comment thread tests/lib/ResponseStream.test.ts Outdated
type: 'function_call',
call_id: 'call_123',
name: 'get_weather',
arguments: '',

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Leave arguments uninitialized in the regression test

This initializes arguments to an empty string, so the test never exercises the nullish fallback it claims to cover: the previous output.arguments += event.delta implementation also produces '{"loc' and passes this assertion. Simulate the reported runtime payload with the property omitted or set to undefined (using an appropriate cast since the generated type requires a string) so the test fails if the fix is reverted.

Useful? React with 👍 / 👎.

@jbeckwith-oai jbeckwith-oai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Requesting one test correction before merge:

tests/lib/ResponseStream.test.ts:276 — the regression test never exercises the nullish case. The fixture initializes arguments to '', so the pre-change implementation (output.arguments += event.delta) already produces '{"loc' and passes this assertion. The test therefore cannot catch a regression in the new ?? '' behavior. Please model the reported wire payload by actually omitting/setting the field to undefined at a narrow cast boundary. Since this PR changes eight accumulator targets, table-driven coverage in the existing focused ResponseAccumulator.test.ts for the affected event families would make the contract explicit without duplicating full stream setup.

The runtime changes themselves are small, direct, and otherwise look safe.

Validation at exact head 5daeffe0febbdcb0d59672e8ee3eae3eab2fc79a: focused ResponseStream and ResponseAccumulator tests passed (14 total); changed-file ESLint and Prettier checks passed; git diff --check passed. Full repository type-check was blocked by missing optional example-only dependencies (@azure/identity, express, and next) in the available dependency tree. No hosted checks are attached to the PR.

The ResponseStream regression test initialized `arguments` to '', so it
passed against the old `+=` implementation and could not catch a
regression. Omit the field on the added item instead, and add
table-driven coverage in ResponseAccumulator.test.ts for all eight
string accumulator targets, each asserted both with an initialized and
with a missing target.
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.

3 participants