fix(streaming): handle anthropic ping chunk silently - #2089
Conversation
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
WalkthroughThe change updates Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/gateway/src/chat/tools/transform-streaming-to-openai.ts (1)
307-323: Consider suppressing ping chunks entirely (transformedData = null) to match the OpenAI keepalive precedent.Anthropic
pingevents are pure connection keepalives with no payload. Synthesizing a fullchat.completion.chunkwithrole: "assistant"for each one forwards a redundant SSE event to the client on every keepalive and is inconsistent with how the analogous OpenAIkeepalivetype is handled in this same file (lines 699-701 settransformedData = null).The PR's goal — silencing the "Unrecognized Anthropic chunk" warn — is achieved either way, but null-suppression aligns with the existing pattern (already confirmed safe by the caller's
if (!transformedData) continue;guard at line 6277-6280) and avoids emitting empty assistant-role deltas to downstream SSE consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/tools/transform-streaming-to-openai.ts` around lines 307 - 323, Replace the synthesized chat.completion.chunk for Anthropic keepalives by setting transformedData = null when data.type === "ping" (instead of building an assistant-role delta); update the ping branch in transform-streaming-to-openai so it mirrors the OpenAI keepalive handling and returns null (the caller already skips null transformedData), and remove the normalizeAnthropicUsage(...) usage for ping since there is no payload to forward.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/gateway/src/chat/tools/transform-streaming-to-openai.ts`:
- Around line 307-323: Replace the synthesized chat.completion.chunk for
Anthropic keepalives by setting transformedData = null when data.type === "ping"
(instead of building an assistant-role delta); update the ping branch in
transform-streaming-to-openai so it mirrors the OpenAI keepalive handling and
returns null (the caller already skips null transformedData), and remove the
normalizeAnthropicUsage(...) usage for ping since there is no payload to
forward.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cabdf8a9-20a2-419c-8900-7c520e543a2e
📒 Files selected for processing (1)
apps/gateway/src/chat/tools/transform-streaming-to-openai.ts
There was a problem hiding this comment.
Pull request overview
Updates Anthropic streaming-to-OpenAI transformation to treat type: "ping" keepalive events as a known chunk type, eliminating noisy “Unrecognized Anthropic chunk” warnings.
Changes:
- Adds explicit handling for Anthropic streaming
pingchunks to avoid warning logs. - Emits an OpenAI-compatible no-op chunk for
pingevents instead of falling through to the “unrecognized” warning path.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| transformedData = { | ||
| id: data.id ?? `chatcmpl-${Date.now()}`, | ||
| object: "chat.completion.chunk", | ||
| created: data.created ?? Math.floor(Date.now() / 1000), | ||
| model: data.model ?? usedModel, | ||
| choices: [ | ||
| { | ||
| index: 0, | ||
| delta: { | ||
| role: "assistant", | ||
| }, | ||
| finish_reason: null, | ||
| }, | ||
| ], | ||
| usage: normalizeAnthropicUsage(data.usage), | ||
| }; |
There was a problem hiding this comment.
Anthropic ping is a keepalive with no user-visible delta; transforming it into an OpenAI chat.completion.chunk adds extra downstream SSE traffic and can interfere with buffering logic that forwards chunks when delta.role is present (see apps/gateway/src/chat/chat.ts:6443-6453). Consider returning null here (like the OpenAI keepalive case) or emitting an actually-empty delta (no role) so ping events are ignored instead of forwarded.
| transformedData = { | |
| id: data.id ?? `chatcmpl-${Date.now()}`, | |
| object: "chat.completion.chunk", | |
| created: data.created ?? Math.floor(Date.now() / 1000), | |
| model: data.model ?? usedModel, | |
| choices: [ | |
| { | |
| index: 0, | |
| delta: { | |
| role: "assistant", | |
| }, | |
| finish_reason: null, | |
| }, | |
| ], | |
| usage: normalizeAnthropicUsage(data.usage), | |
| }; | |
| transformedData = null; |
| } else if (data.type === "ping") { | ||
| transformedData = { | ||
| id: data.id ?? `chatcmpl-${Date.now()}`, | ||
| object: "chat.completion.chunk", | ||
| created: data.created ?? Math.floor(Date.now() / 1000), | ||
| model: data.model ?? usedModel, | ||
| choices: [ | ||
| { | ||
| index: 0, | ||
| delta: { | ||
| role: "assistant", | ||
| }, | ||
| finish_reason: null, | ||
| }, | ||
| ], | ||
| usage: normalizeAnthropicUsage(data.usage), | ||
| }; |
There was a problem hiding this comment.
This new anthropic ping-handling branch isn’t covered by existing unit tests for transformStreamingToOpenai (there are streaming transform tests in transform-streaming-to-openai.spec.ts). Add a test that passes { type: "ping" } for provider anthropic and asserts no warning is logged and that the event is handled as intended (e.g., returns null if treated as a keepalive, or returns the expected no-op chunk).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/gateway/src/chat/tools/transform-streaming-to-openai.ts (1)
307-308: Targeted handling ofpingchunks looks correct.Returning
nullhere is safely handled by the caller inapps/gateway/src/chat/chat.ts(theif (!transformedData) { … continue; }guard), so this cleanly silences the spuriousUnrecognized Anthropic chunkwarnings forclaude-haiku-4-5keepalives without altering downstream stream semantics.One small stylistic note (optional): every other branch in this
switchassigns totransformedDataand falls through to the singlereturn transformedDataat the end of the function (e.g. AWS Bedrock usestransformedData = null;for analogous no-op events at lines 1124, 1126, 1203). Using an earlyreturn nullhere is functionally equivalent but inconsistent with the surrounding pattern. Feel free to align if you prefer uniformity:♻️ Optional consistency tweak
- } else if (data.type === "ping") { - return null; + } else if (data.type === "ping") { + transformedData = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/tools/transform-streaming-to-openai.ts` around lines 307 - 308, The ping branch currently does an early "return null;" which is functionally fine but inconsistent with the surrounding pattern; change the branch handling for data.type === "ping" to set transformedData = null (instead of returning) and fall through to the function's single "return transformedData" at the end so it matches other branches (e.g., AWS Bedrock branches) — update the branch in the same switch handling data.type, referencing transformedData and data.type to locate the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/gateway/src/chat/tools/transform-streaming-to-openai.ts`:
- Around line 307-308: The ping branch currently does an early "return null;"
which is functionally fine but inconsistent with the surrounding pattern; change
the branch handling for data.type === "ping" to set transformedData = null
(instead of returning) and fall through to the function's single "return
transformedData" at the end so it matches other branches (e.g., AWS Bedrock
branches) — update the branch in the same switch handling data.type, referencing
transformedData and data.type to locate the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4c94cf33-0b50-4789-a02f-73c6ec892af1
📒 Files selected for processing (1)
apps/gateway/src/chat/tools/transform-streaming-to-openai.ts
Summary
pingkeepalive events as a known chunk type and emit a no-op delta, eliminating the noisyUnrecognized Anthropic chunkwarn log seen onclaude-haiku-4-5.Test plan
Unrecognized Anthropic chunkwarnings fortype: "ping"are logged.🤖 Generated with Claude Code
Summary by CodeRabbit