[codex] Fix DeepSeek Claude trailing system messages - #5413
Conversation
WalkthroughThis PR implements Claude system-message normalization for DeepSeek Anthropic-compatible relay. It prevents empty assistant responses caused by trailing system messages by merging system content into adjacent user messages before forwarding to non-native-Claude upstreams. ChangesClaude system message normalization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
relay/channel/deepseek/claude_system_normalize.go (2)
38-40: 💤 Low valueEdge case: Trailing system messages lost if no user message exists.
If
normalizedMessagescontains only assistant messages (no user messages),mergePendingSystemTextIntoLastUserwill silently discard any buffered system text. While Claude API typically requires at least one user message, defensive handling could log a warning or raise an error when trailing system messages cannot be merged.🤖 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/deepseek/claude_system_normalize.go` around lines 38 - 40, When pending system text remains but there is no existing user message to merge into (i.e., len(pendingSystems)>0 and mergePendingSystemTextIntoLastUser would drop the text), detect this by checking normalizedMessages for any user-role message; if none, do not call mergePendingSystemTextIntoLastUser — instead log a warning and append a new user message containing strings.Join(pendingSystems, "\n\n") to normalizedMessages (or alternatively return an explicit error), ensuring pendingSystems is not silently discarded; reference mergePendingSystemTextIntoLastUser, pendingSystems, and normalizedMessages when making the change.
62-66: 💤 Low valueNon-text content blocks in system messages are silently dropped.
Only
ContentTypeTextblocks are extracted from structured system messages. If a system message contains images or other media, they will be silently discarded during normalization. This is likely intentional for text-only merging, but consider documenting this behavior if it's not obvious to future maintainers.🤖 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/deepseek/claude_system_normalize.go` around lines 62 - 66, The loop over contents only appends text blocks (contents, dto.ContentTypeText, builder.WriteString) which silently drops non-text system content; update claude_system_normalize.go by explicitly documenting this behavior (add a clear comment above the for loop mentioning that non-text content like images/attachments in structured system messages are intentionally ignored during normalization) or, if you prefer to preserve non-text, implement handling for other ContentType values (e.g., serialize or log them) and reference the same symbols (contents, ContentTypeText, GetText, builder.WriteString) so future maintainers understand the choice.
🤖 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/deepseek/claude_system_normalize.go`:
- Around line 38-40: When pending system text remains but there is no existing
user message to merge into (i.e., len(pendingSystems)>0 and
mergePendingSystemTextIntoLastUser would drop the text), detect this by checking
normalizedMessages for any user-role message; if none, do not call
mergePendingSystemTextIntoLastUser — instead log a warning and append a new user
message containing strings.Join(pendingSystems, "\n\n") to normalizedMessages
(or alternatively return an explicit error), ensuring pendingSystems is not
silently discarded; reference mergePendingSystemTextIntoLastUser,
pendingSystems, and normalizedMessages when making the change.
- Around line 62-66: The loop over contents only appends text blocks (contents,
dto.ContentTypeText, builder.WriteString) which silently drops non-text system
content; update claude_system_normalize.go by explicitly documenting this
behavior (add a clear comment above the for loop mentioning that non-text
content like images/attachments in structured system messages are intentionally
ignored during normalization) or, if you prefer to preserve non-text, implement
handling for other ContentType values (e.g., serialize or log them) and
reference the same symbols (contents, ContentTypeText, GetText,
builder.WriteString) so future maintainers understand the choice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58ce4b3c-76db-48c8-97fb-8f504cf718e2
📒 Files selected for processing (3)
relay/channel/deepseek/adaptor.gorelay/channel/deepseek/adaptor_test.gorelay/channel/deepseek/claude_system_normalize.go
|
原生 claude -> deepseek官方 路径的话,这种事情应由Provider去做而不是NewAPI |
Summary
messages[].role == "system"entries in the DeepSeek Claude request conversion path before forwarding to DeepSeek's Anthropic-compatible upstream.Root Cause
Claude Code
/effort ultracodecan append a trailingsystemmessage after the pending user turn. DeepSeek's Anthropic-compatible upstream may treat that shape as having no pending user turn and return an empty assistantend_turn.Impact
DeepSeek Anthropic-compatible relay requests avoid empty assistant turns for this request shape while preserving the existing native Claude relay behavior.
Validation
go test ./relay/channel/deepseek -count=1Notes
mainat20d3e73734527cded251aff23202dfbf5a2584caSummary by CodeRabbit
New Features
Tests