Repository navigation
fix: emit stream errors in the client protocol - #2355
diegosouzapw merged 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces protocol-specific error handling for SSE streams, specifically adding support for OpenAI Responses and Claude formats. It includes a new buildStreamErrorChunks utility to generate format-appropriate error events and updates the stream controller to track the requested client response format. Comprehensive unit tests have been added to verify these changes. Feedback highlights an opportunity to refactor redundant status code mapping logic and notes a potential risk of SSE format breakage if error messages contain raw newlines.
| function responsesErrorType(statusCode: number): string { | ||
| if (statusCode === 429) return "rate_limit_error"; | ||
| if (statusCode === 401 || statusCode === 403) return "authentication_error"; | ||
| if (statusCode >= 400 && statusCode < 500) return "invalid_request_error"; | ||
| return "server_error"; | ||
| } | ||
|
|
||
| function responsesErrorCode(statusCode: number): string { | ||
| if (statusCode === 429) return "rate_limit_exceeded"; | ||
| if (statusCode === 401) return "invalid_authentication"; | ||
| if (statusCode === 403) return "permission_denied"; | ||
| if (statusCode >= 400 && statusCode < 500) return "bad_request"; | ||
| return "server_error"; | ||
| } | ||
|
|
||
| function claudeErrorType(statusCode: number): string { | ||
| if (statusCode === 429) return "rate_limit_error"; | ||
| if (statusCode === 401) return "authentication_error"; | ||
| if (statusCode === 403) return "permission_error"; | ||
| if (statusCode >= 400 && statusCode < 500) return "invalid_request_error"; | ||
| return "api_error"; | ||
| } |
There was a problem hiding this comment.
The logic for mapping HTTP status codes to protocol-specific error types and codes is repeated across responsesErrorType, responsesErrorCode, and claudeErrorType. While these protocols have slight differences, a more unified mapping structure or a shared helper could improve maintainability and reduce the risk of inconsistencies as more protocols are added.
| if (isResponsesClientFormat(clientResponseFormat)) { | ||
| const errorEvent = { | ||
| type: "response.failed", | ||
| response: { | ||
| id: null, | ||
| status: "failed", | ||
| error: { | ||
| message: errorMsg, | ||
| type: responsesErrorType(statusCode), | ||
| code: responsesErrorCode(statusCode), | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| return [encoder.encode(`event: response.failed\ndata: ${JSON.stringify(errorEvent)}\n\n`)]; | ||
| } | ||
|
|
||
| if (clientResponseFormat === FORMATS.CLAUDE) { | ||
| const errorEvent = { | ||
| type: "error", | ||
| error: { | ||
| type: claudeErrorType(statusCode), | ||
| message: errorMsg, | ||
| }, | ||
| }; | ||
|
|
||
| return [encoder.encode(`event: error\ndata: ${JSON.stringify(errorEvent)}\n\n`)]; | ||
| } | ||
|
|
||
| const errorEvent = { | ||
| object: "chat.completion.chunk", | ||
| choices: [ | ||
| { | ||
| index: 0, | ||
| delta: {}, | ||
| finish_reason: "error", | ||
| }, | ||
| ], | ||
| error: { | ||
| message: errorMsg, | ||
| type: "upstream_error", | ||
| code: statusCode, | ||
| }, | ||
| }; | ||
|
|
||
| return [ | ||
| encoder.encode(`data: ${JSON.stringify(errorEvent)}\n\n`), | ||
| encoder.encode(`data: [DONE]\n\n`), | ||
| ]; |
There was a problem hiding this comment.
In buildStreamErrorChunks, the SSE event construction uses manual string concatenation with \n\n and JSON.stringify. While functional, ensure that errorMsg does not contain characters that could break the SSE format (e.g., raw newlines). Although JSON.stringify handles this for the data: field, the event: field or any future fields might be vulnerable if not carefully handled.
f636442
into
cheaper-inference:release/v3.8.0
|
Thanks @dhaern! Excellent fix for the stream error protocol compliance. The tests are solid. This has been merged into the upcoming v3.8.0 release. |
Integrated into release/v3.8.0
Integrated into release/v3.8.0
Integrated into release/v3.8.0
Summary
response.failedSSE events for Responses clients, including the legacyopenai-responsealias.event: errorSSE events for Claude clients.Motivation
When an upstream stream fails after the response pipeline has already been selected,
createDisconnectAwareStream()currently serializes every mid-stream failure as a Chat Completions chunk:{ "object": "chat.completion.chunk", "choices": [{ "delta": {}, "finish_reason": "error" }], "error": { "type": "upstream_error" } }That is valid only for Chat Completions clients. For
/v1/responsesclients, this leaks a Chat Completions chunk into a Responses stream and strict clients fail schema validation instead of receiving a normal protocol error event.Scope
This is intentionally limited to the shared stream wrapper:
chatCorenow passes the selectedclientResponseFormatinto the stream controller.streamHandlerchooses the error SSE envelope based on that client format.[DONE]termination.response.failedevents with OpenAI-like error type/code mapping.event: errorpayloads without Chat Completions fields.Validation
node --import tsx --test tests/unit/stream-handler.test.tsgit diff --checkNote:
npm run typecheck:corecurrently fails on unrelated existing errors inopen-sse/services/contextManager.ts(Argument of type ... is not assignable to parameter of type 'never'). No touched file was reported in that failure.