Skip to content

fix(responses): safely handle stream deltas on uninitialized properties - #2016

Open
hsusul wants to merge 2 commits into
openai:mainfrom
hsusul:fix/response-accumulator-delta-coalescing
Open

fix(responses): safely handle stream deltas on uninitialized properties#2016
hsusul wants to merge 2 commits into
openai:mainfrom
hsusul:fix/response-accumulator-delta-coalescing

Conversation

@hsusul

@hsusul hsusul commented Jul 25, 2026

Copy link
Copy Markdown

Problem

When processing streaming events in accumulateResponse, accumulating string deltas (response.function_call_arguments.delta, response.refusal.delta, response.reasoning_text.delta, response.reasoning_summary_text.delta, response.custom_tool_call_input.delta, response.mcp_call_arguments.delta, and response.output_text.delta) on items where the target string property was not explicitly pre-initialized (e.g. part: { type: 'refusal' } or a function_call item with uninitialized arguments) results in string coercion of undefined, yielding "undefined<delta>" (such as "undefinedI cannot help").

Root Cause

ResponseAccumulator.ts previously used direct string addition (+=) for these event types without nullish coalescing default guards ((property ?? '') + delta), unlike response.code_interpreter_call_code.delta which already used (output.code ?? '') + event.delta.

Solution

Updated all string property delta handlers in ResponseAccumulator.ts to use nullish default fallback strings before appending deltas:

  • refusal.delta: content.refusal = (content.refusal ?? '') + event.delta;
  • function_call_arguments.delta: output.arguments = (output.arguments ?? '') + event.delta;
  • reasoning_text.delta: content.text = (content.text ?? '') + event.delta;
  • reasoning_summary_text.delta: part.text = (part.text ?? '') + event.delta;
  • custom_tool_call_input.delta: output.input = (output.input ?? '') + event.delta;
  • mcp_call_arguments.delta: output.arguments = (output.arguments ?? '') + event.delta;
  • output_text.delta: content.text = (content.text ?? '') + event.delta; and snapshot.output_text = (snapshot.output_text ?? '') + event.delta;

Regression Coverage

Added a unit test in tests/lib/ResponseAccumulator.test.ts ('safely accumulates deltas when initial string fields are uninitialized') covering function_call_arguments.delta and refusal.delta on uninitialized items.

Validation

  • pnpm test tests/lib/ResponseAccumulator.test.ts (Passed 8/8)
  • pnpm build (Passed)
  • pnpm lint (Passed)
  • git diff --check (Clean)

Generated Code Impact

None. Changes are isolated to src/lib/responses/ResponseAccumulator.ts and its tests under tests/lib/, which are manually maintained.

@hsusul
hsusul requested a review from a team as a code owner July 25, 2026 03:40

@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 a boundary-level correction before merge:

src/lib/responses/ResponseAccumulator.ts:35-64 / :93-235 — normalize incomplete wire shapes when they enter the snapshot, not independently in seven delta consumers. The regression fixtures demonstrate that response.output_item.added and response.content_part.added can omit string fields that the public types mark required. This patch prevents an "undefined" prefix once a delta arrives, but immediately after the added event—and permanently when the value stays empty or no delta arrives—accumulateResponse() still returns a value typed as Response whose required arguments, refusal, text, or input is undefined. The defaulting policy is also duplicated across seven branches.

Please model the incremental wire shape explicitly and normalize cloned items/parts once at the output_item.added, content_part.added, and reasoning-summary ingestion boundaries (plus replacement/lifecycle clones where applicable). Then every returned snapshot satisfies its Response contract and delta handlers can remain plain appends. Add no-delta assertions that inspect the snapshot immediately after the added events.

Relatedly, the as any casts at tests/lib/ResponseAccumulator.test.ts:239 and :266 show that ResponseStreamEvent currently claims these observed payloads are impossible. If omitted fields are valid server events, update the generated/upstream event contract or introduce a narrow typed wire-event boundary rather than leaving public event listeners unsound. A small table-driven matrix can cover all affected event families without repeating full stream setup.

Validation at exact head a21aa370e789db6f634cf8e2512fd154b0acd6c5: focused ResponseAccumulator suite passed (8 tests); changed-file ESLint and Prettier checks passed; git diff --check passed. No hosted checks are attached to the PR.

… boundary

`response.output_item.added`, `response.content_part.added` and
`response.reasoning_summary_part.added` can carry an item or part before the
string field its `*.delta` events append to exists, which left
`accumulateResponse()` returning a value typed `Response` whose `arguments`,
`input`, `refusal`, `text` or `code` was `undefined`.

Model that incremental wire shape explicitly and normalize cloned items and
parts once, as they enter the snapshot, including the `*.done` replacements and
the lifecycle response clones. Every returned snapshot now satisfies its
`Response` contract before any delta arrives, and the delta handlers stay plain
appends.

`code_interpreter_call.code` normalizes to `null` rather than `''` because its
contract is `string | null`, where `null` documents "not available"; that is why
its delta handler keeps a nullish default.

Typing the payloads means the accumulator accepts
`IncrementalResponseStreamEvent` alongside `ResponseStreamEvent`, so the tests
build the observed payloads without `as any`.
@hsusul

hsusul commented Aug 5, 2026

Copy link
Copy Markdown
Author

Thanks — you're right that the previous patch only masked the prefix and duplicated the policy. Reworked it as a boundary normalization.

Modelled the incremental wire shape. ResponseAccumulator.ts now declares the shape the server actually sends: IncrementalOutputItem, IncrementalContentPart, IncrementalSummaryPart, IncrementalResponse and IncrementalResponseStreamEvent, derived from the generated types so they stay in sync (Extract<ResponseOutputItem, { type: … }> / Extract<ResponseStreamEvent, { type: … }>) with only the delta-driven string fields made optional. accumulateResponse() accepts ResponseStreamEvent | IncrementalResponseStreamEvent | ResponseKeepAliveEvent, so the observed payloads are representable without casts. I left the generated event contract alone since it's Stainless-owned; if you'd rather push the optionality upstream instead, happy to swap this for that.

Normalized once, at ingestion. normalizeOutputItem() / normalizeContentPart() / normalizeSummaryPart() run on the clone as it enters the snapshot, at response.output_item.added, response.content_part.added and response.reasoning_summary_part.added, plus the *.done replacements and the lifecycle clones in cloneResponse(). Every snapshot accumulateResponse() returns therefore satisfies its Response contract immediately after the added event, when the value stays empty, and when no delta ever arrives.

Delta handlers are plain appends again — all seven are now byte-identical to main; git diff main -- src/lib/responses/ResponseAccumulator.ts shows no change to any of them.

One deliberate asymmetry: code_interpreter_call.code is string | null by contract, where null documents "not available", so the boundary normalizes an omitted code to null rather than '' and its delta handler keeps its (pre-existing) nullish default. Normalizing it to '' would satisfy the contract too, but would erase that distinction for a call that never streams code.

Tests are a table-driven matrix (incrementalWireCases()) over every affected event family — function_call arguments, mcp_call arguments, custom_tool_call input, output_text text, refusal, reasoning_text, reasoning summary_text, code_interpreter_call code — plus the output_item.done / content_part.done / reasoning_summary_part.done replacements and a response.completed lifecycle clone. Each case asserts snapshot.output[0] immediately after the ingesting event with no delta applied, then again after the matching delta; a separate case pins snapshot-level output_text. All fixtures are typed — no as any remains in the file. Against main's accumulator, 12 of the matrix cases fail; with the patch all 20 tests in the suite pass.

Validation at ab3753faf5e2e4a6689653be2e8d89c6fbf0cdeb:

  • ./node_modules/.bin/jest tests/lib/ResponseAccumulator.test.ts — 20/20 passed
  • ./node_modules/.bin/jest --testPathIgnorePatterns tests/api-resources tests/live — 987 passed, 42 suites
  • ./scripts/lint (prettier, eslint, build, tsc, attw, publint, jsr dry-run) — passed
  • git diff --check — clean

tests/api-resources and tests/live weren't run: they need the Steady mock server / live credentials, which aren't available in my environment.

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