fix: preserve Claude reasoning content in message conversions - #4497
fix: preserve Claude reasoning content in message conversions#4497seefs001 wants to merge 2 commits into
Conversation
WalkthroughAdds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 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 (2)
relay/channel/claude/relay-claude.go (1)
337-343: Marshaling a Go string tojson.RawMessageis correct, but the assignment runs for every non-system role.
common.Marshal(string)always returns a JSON-quoted string and effectively cannot fail at runtime, so the error branch is defensive-only — that's fine and consistent with house style.One subtle behavior worth a sanity check: this assignment runs unconditionally inside the
elsebranch for all non-system roles, includingtool. In thetool→ merge-into-prior-user path (line 361continue) the value is harmlessly discarded along withclaudeMessage. But in the standalonetoolpath (lines 362-371)claudeMessage.Roleis rewritten to"user", which would emit auserClaude message carryingreasoning_content— semantically odd, even if real-world clients never putreasoning_contenton tool messages.If you want belt-and-suspenders behavior, gate the assignment to assistant role:
🛡️ Optional gate
- if message.ReasoningContent != "" { + if message.Role == "assistant" && message.ReasoningContent != "" { reasoningContent, err := common.Marshal(message.ReasoningContent) if err != nil { return nil, err } claudeMessage.ReasoningContent = reasoningContent }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/claude/relay-claude.go` around lines 337 - 343, The code currently marshals message.ReasoningContent into a json.RawMessage via common.Marshal and assigns it to claudeMessage.ReasoningContent for every non-system role; to avoid emitting reasoning_content on messages that are later rewritten from tool→user, only set claudeMessage.ReasoningContent when the role is "assistant" (i.e., guard the common.Marshal + assignment with a check like if message.Role == "assistant" { ... }), leaving the existing error handling intact and ensuring tool messages that are converted to user do not carry reasoning_content.relay/channel/claude/relay_claude_test.go (1)
384-414: Consider a more representative test value and a companion reverse-direction test.The test correctly exercises the forward path, but the reasoning string is a single space (
" "). A more realistic value (multi-line text, embedded quotes/backslashes) would catch JSON-escaping regressions that a whitespace-only payload misses, e.g.:♻️ Stronger payload
- message := dto.Message{ - Role: "assistant", - Content: "", - ReasoningContent: " ", - } + const reasoning = "thought:\n step 1: analyze \"input\"\n step 2: call tool" + message := dto.Message{ + Role: "assistant", + Content: "", + ReasoningContent: reasoning, + } @@ - require.JSONEq(t, `" "`, string(claudeRequest.Messages[1].ReasoningContent)) + expected, _ := json.Marshal(reasoning) + require.JSONEq(t, string(expected), string(claudeRequest.Messages[1].ReasoningContent))Additionally, the linked issue (
#4408) covers the reverse direction (Claude → OpenAI) viaservice.ClaudeToOpenAIRequest, but there's no test for the new mapping atservice/convert.go:137-139. A short companion test that constructs adto.ClaudeRequestwithReasoningContentset on a message and asserts the resultingdto.Message.ReasoningContentwould lock in the round-trip behavior (and would surface theJsonRawMessageToStringquoting question I raised onservice/convert.go).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/claude/relay_claude_test.go` around lines 384 - 414, Update the tests to use a realistic, hard-to-escape reasoning string and add a reverse-direction companion test: modify TestRequestOpenAI2ClaudeMessagePreservesReasoningContent to set ReasoningContent to a multi-line string containing quotes, backslashes and JSON-like fragments (e.g. "Line1\nQuote:\"text\"\nPath:\\\\server\\share\n{\"key\":\"val\"}") to exercise JSON-escaping, verify it round-trips through RequestOpenAI2ClaudeMessage by asserting the preserved value, and add a new test for service.ClaudeToOpenAIRequest that constructs a dto.ClaudeRequest with a message whose ReasoningContent uses the same complex payload and asserts the resulting dto.Message.ReasoningContent matches exactly (this will also validate JsonRawMessageToString behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@relay/channel/claude/relay_claude_test.go`:
- Around line 384-414: Update the tests to use a realistic, hard-to-escape
reasoning string and add a reverse-direction companion test: modify
TestRequestOpenAI2ClaudeMessagePreservesReasoningContent to set ReasoningContent
to a multi-line string containing quotes, backslashes and JSON-like fragments
(e.g. "Line1\nQuote:\"text\"\nPath:\\\\server\\share\n{\"key\":\"val\"}") to
exercise JSON-escaping, verify it round-trips through
RequestOpenAI2ClaudeMessage by asserting the preserved value, and add a new test
for service.ClaudeToOpenAIRequest that constructs a dto.ClaudeRequest with a
message whose ReasoningContent uses the same complex payload and asserts the
resulting dto.Message.ReasoningContent matches exactly (this will also validate
JsonRawMessageToString behavior).
In `@relay/channel/claude/relay-claude.go`:
- Around line 337-343: The code currently marshals message.ReasoningContent into
a json.RawMessage via common.Marshal and assigns it to
claudeMessage.ReasoningContent for every non-system role; to avoid emitting
reasoning_content on messages that are later rewritten from tool→user, only set
claudeMessage.ReasoningContent when the role is "assistant" (i.e., guard the
common.Marshal + assignment with a check like if message.Role == "assistant" {
... }), leaving the existing error handling intact and ensuring tool messages
that are converted to user do not carry reasoning_content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b8bed8a-2d2b-41c8-8b55-c7ea5f42ea65
📒 Files selected for processing (4)
dto/claude.gorelay/channel/claude/relay-claude.gorelay/channel/claude/relay_claude_test.goservice/convert.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
relay/channel/claude/relay_claude_test.go (1)
405-413: Make this assertion less order-coupled.Lines 407-413 assume the tool-use message is always at index 1. Consider selecting the message by
tool_usecontent type first, then asserting itsReasoningContent; this avoids brittle failures if conversion ordering changes without breaking behavior.Proposed test hardening
- require.Len(t, claudeRequest.Messages, 2) - require.JSONEq(t, `" "`, string(claudeRequest.Messages[1].ReasoningContent)) - - content, ok := claudeRequest.Messages[1].Content.([]dto.ClaudeMediaMessage) + require.Len(t, claudeRequest.Messages, 2) + var target *dto.ClaudeMessage + for i := range claudeRequest.Messages { + content, ok := claudeRequest.Messages[i].Content.([]dto.ClaudeMediaMessage) + if !ok || len(content) == 0 { + continue + } + if content[len(content)-1].Type == "tool_use" { + target = &claudeRequest.Messages[i] + break + } + } + require.NotNil(t, target) + require.JSONEq(t, `" "`, string(target.ReasoningContent)) + + content, ok := target.Content.([]dto.ClaudeMediaMessage) require.True(t, ok) require.NotEmpty(t, content) require.Equal(t, "tool_use", content[len(content)-1].Type)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/claude/relay_claude_test.go` around lines 405 - 413, The test is brittle because it assumes the tool-use message is at index 1; update the assertions in the RequestOpenAI2ClaudeMessage test to locate the message with Type == "tool_use" instead of using a fixed index: iterate claudeRequest.Messages, find the entry where Content casts to []dto.ClaudeMediaMessage and the last element's Type == "tool_use", then assert that that message's ReasoningContent equals the expected JSON and that the content slice is non-empty; keep references to claudeRequest, Messages, ReasoningContent, Content, dto.ClaudeMediaMessage and Type to find the correct message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@relay/channel/claude/relay_claude_test.go`:
- Around line 405-413: The test is brittle because it assumes the tool-use
message is at index 1; update the assertions in the RequestOpenAI2ClaudeMessage
test to locate the message with Type == "tool_use" instead of using a fixed
index: iterate claudeRequest.Messages, find the entry where Content casts to
[]dto.ClaudeMediaMessage and the last element's Type == "tool_use", then assert
that that message's ReasoningContent equals the expected JSON and that the
content slice is non-empty; keep references to claudeRequest, Messages,
ReasoningContent, Content, dto.ClaudeMediaMessage and Type to find the correct
message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d07cbc9-5cf5-42c1-a9ef-644665b7d730
📒 Files selected for processing (8)
dto/openai_request.godto/openai_request_zero_value_test.gorelay/channel/claude/relay-claude.gorelay/channel/claude/relay_claude_test.gorelay/channel/gemini/relay-gemini.gorelay/channel/ollama/stream.gorelay/channel/openai/relay-openai.goservice/convert.go
✅ Files skipped from review due to trivial changes (1)
- dto/openai_request_zero_value_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- service/convert.go
- relay/channel/claude/relay-claude.go
|
很急火速审核发布好吗 |
补充：PR #4497 合并后仍存在
|
| 层面 | PR #4497 解决的问题 | 本次修复解决的问题 |
|---|---|---|
| 位置 | DTO 序列化 | Claude→OpenAI 转换逻辑 |
| 根因 | omitempty 吞掉空字符串 |
thinking block 未被映射到 reasoning_content |
| 场景 | OpenAI 格式入口，字段已存在但被丢弃 | Claude 格式入口，字段从未被生成 |
两个修复互补：PR #4497 确保"有值不丢"，本次修复确保"值能被生成"。建议将本次修复也合入主线。
现在的问题是对接官方可以对接这个服务不行，我的修改是打算让直接原生格式对接可以生效，不走转换的场景，你这也没前置说明你是走怎么个逻辑怎么个转发，就让AI给你指一下可能毫不相干转换逻辑，然后生成这么一大段内容，我的修改未必是正确的，但是你这种回复方式真的令人感到不舒服，没有任何前置说明让AI替你回复内容，这很不礼貌。 |
抱歉，我的回复确实没有说清楚前置说明，给您造成困扰了；我再次梳理了场景与代码，我们针对的是不同场景，您此次pr修复的是接收claude格式转发为claude格式的场景，我在本地应用了这个pr后没有认真阅读你修改的场景，尝试用claude格式转发给openai格式的上游仍然报错，于是我打日志进行了debug并修改了代码；是我滥用AI进行回复了，冒犯到您，再次给您说声抱歉；最后想请教您一下，我们针对不同的场景，那么我此次的修复需要另提一个pr吗 |
|
这个pr咋还不合进去呢？ claude - newapi- kimi2.6claudeapi，1.0.0-rc4还是疯狂报错 |
4 participants
Important
📝 变更描述 / Description
(简述：做了什么？为什么这样改能生效？请基于你对代码逻辑的理解来写，避免粘贴未经整理的内容)
deepseek/kimi 的交错思考在message上新增了reasoning_content，和原生Anthropic端点不一致，需要补充字段进行兼容。
该PR处理
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
(请在此粘贴截图、关键日志或测试报告，以证明变更生效)
Summary by CodeRabbit