fix(deepseek): apply bounded JSON policy on HTTP SSE for Flash - #1006
fix(deepseek): apply bounded JSON policy on HTTP SSE for Flash#1006Michael-Han0608 wants to merge 2 commits into
Conversation
Extend the DeepSeek Flash terminal-safe Responses path beyond WebSocket so default Codex Desktop HTTP/SSE turns also force bounded upstream JSON and reframe a complete client event sequence.
📝 WalkthroughWalkthroughThe proxy applies bounded upstream JSON compatibility to HTTP SSE and WebSocket Responses requests. It preserves the client’s stream preference and converts valid bounded JSON into client-facing SSE events when required. ChangesResponses streaming compatibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant handleResponses
participant DeepSeek
Client->>handleResponses: Send Responses request
handleResponses->>DeepSeek: Request bounded JSON
DeepSeek-->>handleResponses: Return Responses JSON
handleResponses-->>Client: Return SSE events or JSON
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Extends the existing DeepSeek Flash “bounded upstream JSON” compatibility policy from WebSocket-only Responses turns to the default HTTP/SSE Responses path, synthesizing a terminal-safe SSE event sequence when the client requested streaming.
Changes:
- Apply the registry’s
modelWebsocketUpstreamStreaming: falsepolicy to HTTP Responses turns (forcestream: falseupstream when streaming is unreliable). - When the client requested
stream: trueover HTTP and upstream is bounded JSON, synthesize a minimal SSE sequence ending in a terminal event (response.completed/response.failed/response.incomplete). - Update docs and expand tests to cover HTTP/SSE bounded-JSON reframing and terminal-status preservation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/deepseek-inbound-wire.test.ts | Adds coverage for bounded-JSON forcing on HTTP and SSE synthesis from completed JSON. |
| structure/04_transports-and-sidecars.md | Updates transport documentation to reflect bounded-JSON reframing for both WS and HTTP/SSE clients. |
| src/types.ts | Adds _clientRequestedStream to preserve client streaming intent after compatibility rewrites. |
| src/server/responses/core.ts | Implements JSON→SSE reframing and applies bounded-JSON upstream policy beyond WebSocket turns. |
| src/providers/registry.ts | Clarifies registry hint semantics and documents broader (HTTP+WS) applicability while preserving the field name. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const sseHeaders = new Headers(headers); | ||
| sseHeaders.set("content-type", "text/event-stream; charset=utf-8"); | ||
| sseHeaders.set("cache-control", "no-cache"); | ||
| return new Response(responsesJsonToClientSse(responseJson), { |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f4204eb60
ℹ️ 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".
| const responsesUpstreamStreaming = providerModelWebsocketUpstreamStreaming( | ||
| route.providerName, | ||
| route.provider, | ||
| route.modelId, | ||
| ); |
There was a problem hiding this comment.
For DeepSeek Flash requests originating from /v1/chat/completions or Anthropic Messages, inboundWire causes resolveWireProtocolOverride() to retain the openai-chat adapter, but this compatibility hint is resolved before that selection and then unconditionally sets parsed.stream = false. Consequently, the OpenAI Chat adapter sends stream: false upstream and streaming Chat/Claude clients receive no incremental output even though the terminal-event defect is specific to DeepSeek's Responses stream. Resolve the wire first and apply this policy only when the final adapter is openai-responses (or otherwise gate it on the Responses inbound).
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| let responseJson: Record<string, unknown>; | ||
| try { | ||
| responseJson = JSON.parse(restoredText) as Record<string, unknown>; | ||
| } catch { |
There was a problem hiding this comment.
When a streaming HTTP upstream returns syntactically valid but non-object JSON, this cast accepts it without validation. In particular, a successful application/json body of null reaches responsesJsonToClientSse(), which dereferences response.output and throws out of the request handler instead of returning the intended typed 502; arrays and primitives are similarly converted into a fabricated response.completed event. Validate that the parsed value is a non-array object with a Responses-compatible shape before reframing it, and return formatErrorResponse() otherwise.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/responses/core.ts`:
- Around line 2202-2212: Validate the result assigned in the
parsed._clientRequestedStream SSE branch before passing it to
responsesJsonToClientSse: require a non-null, non-array object and return the
existing 502 upstream_error response for invalid JSON shapes, including null.
Add a regression test covering a null upstream JSON body.
- Around line 2209-2216: Update the synthesized SSE response in the
responsesJsonToClientSse flow to set Cache-Control to no-store instead of
no-cache, preventing authenticated completions from being stored. Extend the
existing HTTP SSE regression test to assert the response includes the no-store
directive.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 167de94c-6d18-447b-a2e9-0dda973df8c6
📒 Files selected for processing (5)
src/providers/registry.tssrc/server/responses/core.tssrc/types.tsstructure/04_transports-and-sidecars.mdtests/deepseek-inbound-wire.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/deepseek-inbound-wire.test.ts`:
- Line 243: The test callback contains three `const payload` declarations which
violates TypeScript's lexical scoping rules for const declarations. Identify all
three payload const declarations within the same test callback and remove or
rename the duplicate declarations so only one const payload exists in that
scope. Preserve the payload declaration shown at line 243 and update any other
payload declarations with distinct variable names to avoid the duplicate const
error.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d798c9a7-4e3e-44c1-bdb8-132ad1bbc04f
📒 Files selected for processing (2)
src/server/responses/core.tstests/deepseek-inbound-wire.test.ts
| test("HTTP stream clients reject null bounded JSON with a typed upstream error", async () => { | ||
| const response = await respondWithUpstreamJson(null); | ||
| expect(response.status).toBe(502); | ||
| const payload = (await response.json()) as { error?: { code?: string; message?: string } }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate payload declarations.
Line 243 declares const payload three times in the same test() callback. TypeScript rejects duplicate const declarations in one lexical scope. Bun cannot load this test module, so this regression coverage does not run.
Proposed fix
const payload = (await response.json()) as { error?: { code?: string; message?: string } };
- const payload = (await response.json()) as { error?: { code?: string; message?: string } };
- const payload = (await response.json()) as { error?: { code?: string; message?: string } };Based on learnings: repeated const declarations are valid only in separate test() callback scopes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const payload = (await response.json()) as { error?: { code?: string; message?: string } }; | |
| const payload = (await response.json()) as { error?: { code?: string; message?: string } }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/deepseek-inbound-wire.test.ts` at line 243, The test callback contains
three `const payload` declarations which violates TypeScript's lexical scoping
rules for const declarations. Identify all three payload const declarations
within the same test callback and remove or rename the duplicate declarations so
only one const payload exists in that scope. Preserve the payload declaration
shown at line 243 and update any other payload declarations with distinct
variable names to avoid the duplicate const error.
Source: Learnings
Summary
5dd965a13introduced a DeepSeek Flash compatibility policy that forces a bounded upstream Responses body and then reframes it for the client. That policy is currently applied only when the inbound transport is WebSocket.Codex Desktop still uses HTTP Responses/SSE by default (
websocketsis opt-in). On that default path, DeepSeek Flash can return useful output without a usable terminal event, leaving the client pending until idle timeout.This PR applies the same bounded-upstream policy to HTTP Responses turns as well:
stream: falseupstream for both HTTP and WebSocket Responses turns.stream: trueon HTTP, reframe the completed JSON body into a minimal SSE sequence that always ends on a terminal event:response.createdresponse.output_item.donefor each output itemresponse.completed/response.failed/response.incompletesendResponsesJsonAsEvents().This keeps the historical registry field name (
modelWebsocketUpstreamStreaming) to minimize churn; the caller now applies it beyond WebSocket-only turns.Why
Default path today:
After this change for DeepSeek Flash:
Test plan
bun test tests/deepseek-inbound-wire.test.tsbun test tests/deepseek-reasoning-replay.test.tswebsockets: falsewebsockets: trueNotes
5dd965a13alone is sufficient for pure WebSocket environmentsSummary by CodeRabbit
New Features
Bug Fixes