Skip to content

fix(claude): buffer SSE for non-stream clients - #6292

Open
linjzy wants to merge 2 commits into
QuantumNous:mainfrom
linjzy:fix/claude-nonstream-buffering
Open

fix(claude): buffer SSE for non-stream clients#6292
linjzy wants to merge 2 commits into
QuantumNous:mainfrom
linjzy:fix/claude-nonstream-buffering

Conversation

@linjzy

@linjzy linjzy commented Jul 18, 2026

Copy link
Copy Markdown

⚠️ 提交说明 / PR Notice

Important

  • 请提供人工撰写的简洁摘要,避免直接粘贴未经整理的 AI 输出。

Note

AI-assisted disclosure: 此实现和初始 PR 草稿由 OpenAI Codex 在仓库所有者请求下协助完成;仓库所有者已人工确认变更内容和影响范围。

📝 变更描述 / Description

Claude Messages 的请求可能声明 stream: false,但部分兼容上游仍返回 text/event-stream。现有处理会根据上游 Content-Type 将 info.IsStream 置为 true,随后把 SSE 直接返回给只接受普通 JSON 的客户端。

本变更在这种情况下聚合 Claude SSE 事件,按内容块索引还原 text、thinking/signature 和 tool_use 输入,并合并 message_start/message_delta 中的 usage、stop_reason 与 stop_sequence,最终复用现有非流式响应及计费处理输出标准 Claude Messages JSON。客户端原本请求流式时继续使用现有流式处理器。

🚀 变更类型 / Type of change

  • 🐛 Bug 修复 (Bug fix) - 请关联对应 Issue,避免将设计取舍、理解偏差或预期不一致直接归类为 bug
  • ✨ 新功能 (New feature) - 重大特性建议先通过 Issue 沟通
  • ⚡ 性能优化 / 重构 (Refactor)
  • 📝 文档更新 (Documentation)

🔗 关联任务 / Related Issue

✅ 提交前检查项 / Checklist

  • 人工确认: 我已亲自整理并撰写此描述,没有直接粘贴未经处理的 AI 输出。
  • 非重复提交: 我已搜索现有的 IssuesPRs,确认不是重复提交。
  • Bug fix 说明: 已关联 fix: buffer Claude SSE for non-stream clients #6294;该问题会让非流式 Claude Messages 客户端收到不兼容的 SSE 响应。
  • 变更理解: 我已理解这些更改的工作原理及可能影响。
  • 范围聚焦: 本 PR 未包含任何与当前任务无关的代码改动。
  • 本地验证: 已在本地运行并通过测试或手动验证,维护者可以据此复核结果。
  • 安全合规: 代码中无敏感凭据,且符合项目代码规范。

📸 运行证明 / Proof of Work

以下命令均已通过:

go test ./relay/channel/claude ./dto
go test ./relay/...

回归测试覆盖:非流式客户端聚合 SSE 后得到 JSON、usage/thinking/tool_use 字段保留,以及流式客户端仍收到 SSE。

@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Claude relay handling now buffers upstream SSE responses into standard JSON when the client requests non-streaming output. Streaming clients continue receiving SSE, while buffered responses preserve content, tool inputs, usage, stop metadata, and related Claude fields.

Changes

Claude SSE buffering

Layer / File(s) Summary
Buffer contract and accumulator state
dto/claude.go, relay/channel/claude/buffered_stream.go
Adds optional stop-sequence serialization and accumulator state for response metadata, usage, indexed content blocks, and streamed tool fragments.
Event processing and response finalization
relay/channel/claude/buffered_stream.go
Processes Claude lifecycle and delta events, merges usage, finalizes tool inputs, and materializes the completed response.
Adaptor routing and behavioral validation
relay/channel/claude/adaptor.go, relay/channel/claude/buffered_stream.go, relay/channel/claude/buffered_stream_test.go
Routes non-streaming clients through the buffered handler, preserves SSE for streaming clients, and tests both response paths.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ClaudeClient
  participant AdaptorDoResponse
  participant ClaudeUpstream
  participant ClaudeBufferedStreamHandler
  participant HandleClaudeResponseData
  ClaudeClient->>AdaptorDoResponse: Request stream preference
  AdaptorDoResponse->>ClaudeUpstream: Receive Claude response
  AdaptorDoResponse->>ClaudeBufferedStreamHandler: Buffer SSE for non-stream client
  ClaudeUpstream-->>ClaudeBufferedStreamHandler: Claude SSE events
  ClaudeBufferedStreamHandler->>HandleClaudeResponseData: Completed Claude JSON and usage
  HandleClaudeResponseData-->>ClaudeClient: JSON response
Loading

Possibly related PRs

Poem

I’m a rabbit buffering streams in the night,
Turning SSE into JSON just right.
Tool crumbs gathered, stop sequences shine,
Usage and thinking fall neatly in line.
Streamers still hop through the event-flow lane.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes implement the linked issue requirements for buffering Claude SSE, preserving content, usage, stop reason, and stop sequence.
Out of Scope Changes check ✅ Passed The added DTO field, buffered handler, adapter routing, and tests all align with the Claude SSE buffering objective.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: buffering Claude SSE responses for clients that did not request streaming.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
relay/channel/claude/buffered_stream_test.go (1)

75-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Exercise preservation of a non-null stop_sequence.

The test currently proves only that an absent value is emitted as null; it would not catch the buffered path dropping an actual stop sequence.

Proposed test adjustment
-		`data: {"type":"message_delta","delta":{"stop_reason":"tool_use"},"usage":{"output_tokens":9}}`,
+		`data: {"type":"message_delta","delta":{"stop_reason":"tool_use","stop_sequence":"tool-end"},"usage":{"output_tokens":9}}`,
...
 	stopSequence, exists := responseObject["stop_sequence"]
 	assert.True(t, exists)
-	assert.Nil(t, stopSequence)
+	assert.Equal(t, "tool-end", stopSequence)

As per coding guidelines, backend tests must protect real behavior and API contracts.

Also applies to: 103-105

🤖 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 `@relay/channel/claude/buffered_stream_test.go` around lines 75 - 76, Update
the buffered stream test’s message_delta fixture and corresponding assertions to
include a non-null stop_sequence value, verifying that the buffered path
preserves and emits the actual sequence instead of converting or dropping it;
apply the same adjustment to the related lines noted in the comment.

Source: Coding guidelines

🤖 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.

Nitpick comments:
In `@relay/channel/claude/buffered_stream_test.go`:
- Around line 75-76: Update the buffered stream test’s message_delta fixture and
corresponding assertions to include a non-null stop_sequence value, verifying
that the buffered path preserves and emits the actual sequence instead of
converting or dropping it; apply the same adjustment to the related lines noted
in the comment.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a86c09d4-cf37-47dc-b7a8-3b79f104e6dc

📥 Commits

Reviewing files that changed from the base of the PR and between 5a6c53d and 9adfb9f.

📒 Files selected for processing (4)
  • dto/claude.go
  • relay/channel/claude/adaptor.go
  • relay/channel/claude/buffered_stream.go
  • relay/channel/claude/buffered_stream_test.go

@linjzy

linjzy commented Jul 18, 2026

Copy link
Copy Markdown
Author

Addressed the review nit in 7e7b2b8: the buffered-stream regression test now includes a non-null stop_sequence and verifies the exact value is preserved. go test ./relay/channel/claude passes.

@linjzy
linjzy marked this pull request as ready for review July 18, 2026 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: buffer Claude SSE for non-stream clients

1 participant