fix(responses): reject with OpenAIError when the stream emits an error event - #2021
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c34bfea5d9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
jbeckwith-oai
left a comment
There was a problem hiding this comment.
The conversion is still incomplete in two valid consumption paths:
-
#addEventcallsaccumulateResponse()before checkingevent.type, so anerrorframe sent beforeresponse.creatednever reaches the new branch. It still rejects with the accumulator's genericOpenAIErrorand loses the server message/code/param. Please convert error frames before snapshot accumulation and add coverage for an initial error frame. -
The raw error frame is emitted through the generic
eventchannel before theAPIErroris thrown, whileResponseStream[Symbol.asyncIterator]only rejects reads that are pending at the instant the error fires and does not retain the failure. A normalfor awaitconsumer therefore observes[response.created, error]and then completes cleanly; only a separatefinalResponse()call sees theAPIError. Please make async iteration reject consistently (the failure-retention pattern inEventStream.events()is a useful model) and add a regression test.
Validation at c34bfea5d94941e37038bb6ac8834240f84590c4: the focused ResponseStream suite passed (6/6), Prettier and ESLint passed on changed files, TypeScript checking passed, the package build passed, and git diff --check passed. I also reproduced both cases against the built package.
|
Reproduced both stream-error paths against the built package and fixed them in the same order.
The focused ResponseStream file passes 8/8; |
…sponse-stream-errors # Conflicts: # src/lib/responses/ResponseStream.ts # tests/lib/ResponseStream.test.ts
Superseded by verified commit 25e9f9c: initial nested/flat errors are converted before accumulation, generic error frames are not emitted, and async iteration retains failures through EventStream.events. Focused live-SSE/replay regressions and all 1,701 unit tests pass.
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
Validated against the actual first-party/Azure nested SSE error contract and the documented flat compatibility shape. Errors are normalized before accumulation, API metadata and request IDs are preserved, and async iteration reuses EventStream.events to retain delayed failures. Verified 1,701 unit tests, focused live-SSE/replay/cancellation cases, lint, TypeScript 4.9/6 published checks, and full CJS/ESM build.
Summary
Fixes #2020.
APIErrorbefore response accumulation or generic event emission, including errors received beforeresponse.created.EventStream.events('event')so asynchronous iteration retains failures that occur while an application is processing an earlier event, drains buffered events in order, and cleans up correctly on cancellation.errorlisteners typed asOpenAIErrorand preserve normalresponse.failedsnapshot handling.Verification