-
Notifications
You must be signed in to change notification settings - Fork 595
fix(deepseek): apply bounded JSON policy on HTTP SSE for Flash #1006
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -112,18 +112,56 @@ describe("the inbound scope survives the handleResponses replay", () => { | |||||
| return requests[0] ?? { url: "", body: {} }; | ||||||
| } | ||||||
|
|
||||||
| async function respondWithUpstreamJson( | ||||||
| payload: unknown, | ||||||
| options: { | ||||||
| clientStream?: boolean; | ||||||
| inboundTransport?: "websocket"; | ||||||
| upstreamHeaders?: HeadersInit; | ||||||
| } = {}, | ||||||
| ): Promise<Response> { | ||||||
| const upstreamHeaders = new Headers(options.upstreamHeaders); | ||||||
| upstreamHeaders.set("content-type", "application/json"); | ||||||
| globalThis.fetch = (async () => new Response(JSON.stringify(payload), { | ||||||
| status: 200, | ||||||
| headers: upstreamHeaders, | ||||||
| })) as typeof fetch; | ||||||
| const config = { providers: { deepseek: deepseekProvider() } } as unknown as OcxConfig; | ||||||
| return handleResponses( | ||||||
| new Request("http://localhost/v1/responses", { | ||||||
| method: "POST", | ||||||
| headers: { "content-type": "application/json" }, | ||||||
| body: JSON.stringify({ | ||||||
| model: MODEL, | ||||||
| input: "ping", | ||||||
| stream: options.clientStream ?? true, | ||||||
| }), | ||||||
| }), | ||||||
| config, | ||||||
| { model: "", provider: "" }, | ||||||
| { | ||||||
| inboundWire: "responses", | ||||||
| ...(options.inboundTransport === undefined ? {} : { inboundTransport: options.inboundTransport }), | ||||||
| }, | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| test("a native Responses request reaches the documented /responses route", async () => { | ||||||
| expect((await drive("responses")).url).toBe("https://api.deepseek.com/responses"); | ||||||
| }); | ||||||
|
|
||||||
| test("an Anthropic replay reaches /chat/completions, not /responses", async () => { | ||||||
| // Regression guard for the audit's critical finding: editing only the pre-flight | ||||||
| // resolution in claude-messages.ts left this URL on /responses. | ||||||
| expect((await drive("anthropic")).url).toBe("https://api.deepseek.com/chat/completions"); | ||||||
| const request = await drive("anthropic"); | ||||||
| expect(request.url).toBe("https://api.deepseek.com/chat/completions"); | ||||||
| expect(request.body.stream).toBe(true); | ||||||
| }); | ||||||
|
|
||||||
| test("a Chat replay reaches /chat/completions, not /responses", async () => { | ||||||
| expect((await drive("chat")).url).toBe("https://api.deepseek.com/chat/completions"); | ||||||
| const request = await drive("chat"); | ||||||
| expect(request.url).toBe("https://api.deepseek.com/chat/completions"); | ||||||
| expect(request.body.stream).toBe(true); | ||||||
| }); | ||||||
|
|
||||||
| test("a Codex WebSocket turn asks DeepSeek for bounded JSON upstream", async () => { | ||||||
|
|
@@ -132,8 +170,101 @@ describe("the inbound scope survives the handleResponses replay", () => { | |||||
| expect(request.body.stream).toBe(false); | ||||||
| }); | ||||||
|
|
||||||
| test("ordinary HTTP Responses requests keep streaming upstream", async () => { | ||||||
| expect((await drive("responses")).body.stream).toBe(true); | ||||||
| test("ordinary HTTP Responses turns also force bounded JSON for DeepSeek Flash", async () => { | ||||||
| // Codex Desktop defaults to HTTP/SSE while websockets stay opt-in. Flash still | ||||||
| // needs the terminal-safe upstream path on that transport. | ||||||
| const request = await drive("responses"); | ||||||
| expect(request.url).toBe("https://api.deepseek.com/responses"); | ||||||
| expect(request.body.stream).toBe(false); | ||||||
| }); | ||||||
|
|
||||||
| test("HTTP stream clients receive a terminal SSE sequence from bounded JSON", async () => { | ||||||
| const response = await respondWithUpstreamJson({ | ||||||
| id: "resp_deepseek", | ||||||
| object: "response", | ||||||
| status: "completed", | ||||||
| output: [{ | ||||||
| type: "function_call", | ||||||
| id: "fc_1", | ||||||
| call_id: "call_1", | ||||||
| name: "shell", | ||||||
| arguments: "{\"command\":\"pwd\"}", | ||||||
| status: "completed", | ||||||
| }], | ||||||
| }, { | ||||||
| upstreamHeaders: { | ||||||
| "content-length": "999", | ||||||
| "content-encoding": "gzip", | ||||||
| }, | ||||||
| }); | ||||||
| expect(response.status).toBe(200); | ||||||
| expect(response.headers.get("content-type") ?? "").toContain("text/event-stream"); | ||||||
| expect(response.headers.get("cache-control")).toBe("no-store"); | ||||||
| expect(response.headers.get("content-length")).toBeNull(); | ||||||
| expect(response.headers.get("content-encoding")).toBeNull(); | ||||||
| const body = await response.text(); | ||||||
| expect(body).toContain("event: response.created"); | ||||||
| expect(body).toContain("event: response.output_item.done"); | ||||||
| expect(body).toContain("event: response.completed"); | ||||||
| expect(body).toContain("function_call"); | ||||||
| }); | ||||||
|
|
||||||
| test("HTTP stream clients preserve failed terminal status from bounded JSON", async () => { | ||||||
| const response = await respondWithUpstreamJson({ | ||||||
| id: "resp_failed", | ||||||
| object: "response", | ||||||
| status: "failed", | ||||||
| output: [], | ||||||
| error: { code: "server_error", message: "upstream failed" }, | ||||||
| }); | ||||||
| const body = await response.text(); | ||||||
| expect(body).toContain("event: response.failed"); | ||||||
| expect(body).not.toContain("event: response.completed"); | ||||||
| expect(body).toContain("upstream failed"); | ||||||
| }); | ||||||
|
|
||||||
| test("HTTP stream clients preserve incomplete terminal status from bounded JSON", async () => { | ||||||
| const response = await respondWithUpstreamJson({ | ||||||
| id: "resp_incomplete", | ||||||
| object: "response", | ||||||
| status: "incomplete", | ||||||
| output: [], | ||||||
| incomplete_details: { reason: "upstream_stall_timeout" }, | ||||||
| }); | ||||||
| const body = await response.text(); | ||||||
| expect(body).toContain("event: response.incomplete"); | ||||||
| expect(body).not.toContain("event: response.completed"); | ||||||
| expect(body).toContain("upstream_stall_timeout"); | ||||||
| }); | ||||||
|
|
||||||
| 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 } }; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win Remove the duplicate Line 243 declares 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 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Learnings |
||||||
| expect(payload.error?.code).toBe("upstream_server_error"); | ||||||
| expect(payload.error?.message).toContain("malformed JSON"); | ||||||
| }); | ||||||
|
|
||||||
| test("non-streaming HTTP clients keep the bounded JSON response", async () => { | ||||||
| const response = await respondWithUpstreamJson({ | ||||||
| id: "resp_json", | ||||||
| object: "response", | ||||||
| status: "completed", | ||||||
| output: [], | ||||||
| }, { clientStream: false }); | ||||||
| expect(response.headers.get("content-type") ?? "").toContain("application/json"); | ||||||
| expect(await response.json()).toMatchObject({ id: "resp_json", status: "completed" }); | ||||||
| }); | ||||||
|
|
||||||
| test("WebSocket turns keep bounded JSON for the existing WS re-framer", async () => { | ||||||
| const response = await respondWithUpstreamJson({ | ||||||
| id: "resp_ws", | ||||||
| object: "response", | ||||||
| status: "completed", | ||||||
| output: [{ type: "message", role: "assistant", status: "completed", content: [] }], | ||||||
| }, { inboundTransport: "websocket" }); | ||||||
| expect(response.headers.get("content-type") ?? "").toContain("application/json"); | ||||||
| expect(await response.json()).toMatchObject({ id: "resp_ws", status: "completed" }); | ||||||
| }); | ||||||
|
|
||||||
| test("an oversized upstream JSON body fails closed instead of buffering without limit", async () => { | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a streaming HTTP upstream returns syntactically valid but non-object JSON, this cast accepts it without validation. In particular, a successful
application/jsonbody ofnullreachesresponsesJsonToClientSse(), which dereferencesresponse.outputand throws out of the request handler instead of returning the intended typed 502; arrays and primitives are similarly converted into a fabricatedresponse.completedevent. Validate that the parsed value is a non-array object with a Responses-compatible shape before reframing it, and returnformatErrorResponse()otherwise.AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.