fix(anthropic): omitted thinking block, close #1105#1113
fix(anthropic): omitted thinking block, close #1105#1113looplj merged 1 commit intorelease/v0.9.xfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Anthropic transformer's handling of 'thinking blocks,' particularly in streaming contexts and with the introduction of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the thinking_display feature to the Anthropic transformer, allowing users to specify how thinking content is displayed (summarized or omitted). It also includes defensive measures for handling signatures that arrive without preceding reasoning content in streaming responses, and adds integration tests to verify the new functionality.
|
|
||
| if anthropicReq.Thinking.Display != "" { | ||
| chatReq.TransformerMetadata[TransformerMetadataKeyThinkingDisplay] = anthropicReq.Thinking.Display | ||
| } |
There was a problem hiding this comment.
This if block and the one on lines 315-317 are essentially identical. Consider extracting this logic into a separate function to reduce code duplication and improve maintainability. This will also make the code easier to read.
func setThinkingDisplay(metadata map[string]any, display string) {
if display != "" {
metadata[TransformerMetadataKeyThinkingDisplay] = display
}
}
// ...
case "enabled":
chatReq.ReasoningEffort = thinkingBudgetToReasoningEffort(anthropicReq.Thinking.BudgetTokens)
chatReq.ReasoningBudget = lo.ToPtr(anthropicReq.Thinking.BudgetTokens)
setThinkingDisplay(chatReq.TransformerMetadata, anthropicReq.Thinking.Display)
case "adaptive":
// Adaptive thinking doesn't require a budget; preserve the type marker via TransformerMetadata.
chatReq.TransformerMetadata[TransformerMetadataKeyThinkingType] = "adaptive"
setThinkingDisplay(chatReq.TransformerMetadata, anthropicReq.Thinking.Display)|
|
||
| if anthropicReq.Thinking.Display != "" { | ||
| chatReq.TransformerMetadata[TransformerMetadataKeyThinkingDisplay] = anthropicReq.Thinking.Display | ||
| } |
There was a problem hiding this comment.
Similar to the previous comment, this if block duplicates the logic on lines 308-310. Extracting this logic into a function would improve code readability and maintainability.
func setThinkingDisplay(metadata map[string]any, display string) {
if display != "" {
metadata[TransformerMetadataKeyThinkingDisplay] = display
}
}
// ...
case "enabled":
chatReq.ReasoningEffort = thinkingBudgetToReasoningEffort(anthropicReq.Thinking.BudgetTokens)
chatReq.ReasoningBudget = lo.ToPtr(anthropicReq.Thinking.BudgetTokens)
setThinkingDisplay(chatReq.TransformerMetadata, anthropicReq.Thinking.Display)
case "adaptive":
// Adaptive thinking doesn't require a budget; preserve the type marker via TransformerMetadata.
chatReq.TransformerMetadata[TransformerMetadataKeyThinkingType] = "adaptive"
setThinkingDisplay(chatReq.TransformerMetadata, anthropicReq.Thinking.Display)| // closeThinkingBlock ensures any open or implied thinking block is properly | ||
| // closed. It handles three scenarios: | ||
| // 1. pendingSignature exists but no thinking block was started — creates a | ||
| // synthetic empty thinking block (start + signature_delta + stop). | ||
| // 2. A thinking block is open — flushes any pending signature as | ||
| // signature_delta, then emits content_block_stop. | ||
| // 3. Neither — no-op. |
There was a problem hiding this comment.
The documentation mentions three scenarios, but the third scenario is not explicitly stated in the documentation, which could lead to confusion. Clarify the third scenario (no pending signature and no open thinking block) in the documentation for better understanding.
| func (s *anthropicInboundStream) closeThinkingBlock() error { | ||
| if s.pendingSignature != nil && !s.hasThinkingContentStarted { | ||
| sig := s.pendingSignature | ||
| s.pendingSignature = nil | ||
|
|
||
| if err := s.enqueEvent(&StreamEvent{ | ||
| Type: "content_block_start", | ||
| Index: &s.contentIndex, | ||
| ContentBlock: &MessageContentBlock{ | ||
| Type: "thinking", | ||
| Thinking: lo.ToPtr(""), | ||
| }, | ||
| }); err != nil { | ||
| return fmt.Errorf("failed to enqueue thinking content_block_start for pending signature: %w", err) | ||
| } | ||
|
|
||
| if err := s.enqueEvent(&StreamEvent{ | ||
| Type: "content_block_delta", | ||
| Index: &s.contentIndex, | ||
| Delta: &StreamDelta{ | ||
| Type: lo.ToPtr("signature_delta"), | ||
| Signature: sig, | ||
| }, | ||
| }); err != nil { | ||
| return fmt.Errorf("failed to enqueue signature_delta for pending signature: %w", err) | ||
| } | ||
|
|
||
| if err := s.enqueEvent(&StreamEvent{ | ||
| Type: "content_block_stop", | ||
| Index: &s.contentIndex, | ||
| }); err != nil { | ||
| return fmt.Errorf("failed to enqueue content_block_stop for pending signature: %w", err) | ||
| } | ||
|
|
||
| s.contentIndex += 1 | ||
|
|
||
| return nil | ||
| } | ||
| sig := s.pendingSignature | ||
| s.pendingSignature = nil | ||
|
|
||
| return s.enqueEvent(&StreamEvent{ | ||
| Type: "content_block_delta", | ||
| Index: &s.contentIndex, | ||
| Delta: &StreamDelta{ | ||
| Type: lo.ToPtr("signature_delta"), | ||
| Signature: sig, | ||
| }, | ||
| }) | ||
|
|
||
| if s.hasThinkingContentStarted { | ||
| s.hasThinkingContentStarted = false | ||
|
|
||
| if s.pendingSignature != nil { | ||
| sig := s.pendingSignature | ||
| s.pendingSignature = nil | ||
|
|
||
| if err := s.enqueEvent(&StreamEvent{ | ||
| Type: "content_block_delta", | ||
| Index: &s.contentIndex, | ||
| Delta: &StreamDelta{ | ||
| Type: lo.ToPtr("signature_delta"), | ||
| Signature: sig, | ||
| }, | ||
| }); err != nil { | ||
| return fmt.Errorf("failed to enqueue signature_delta event: %w", err) | ||
| } | ||
| } | ||
|
|
||
| if err := s.enqueEvent(&StreamEvent{ | ||
| Type: "content_block_stop", | ||
| Index: &s.contentIndex, | ||
| }); err != nil { | ||
| return fmt.Errorf("failed to enqueue content_block_stop event: %w", err) | ||
| } | ||
|
|
||
| s.contentIndex += 1 | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
This function is quite long and handles multiple scenarios. Consider breaking it down into smaller, more focused functions to improve readability and maintainability. For example, each of the if blocks could be extracted into its own function.
| // Handle redacted reasoning content (redacted_thinking) | ||
| if choice.Delta != nil && choice.Delta.RedactedReasoningContent != nil && *choice.Delta.RedactedReasoningContent != "" { | ||
| // If the thinking content has started before the redacted thinking content, we need to stop it | ||
| if s.hasThinkingContentStarted { | ||
| s.hasThinkingContentStarted = false | ||
|
|
||
| if err := s.flushPendingSignature(); err != nil { | ||
| s.err = fmt.Errorf("failed to flush pending signature: %w", err) | ||
| return false | ||
| } | ||
|
|
||
| stopEvent := StreamEvent{ | ||
| Type: "content_block_stop", | ||
| Index: &s.contentIndex, | ||
| } | ||
|
|
||
| err := s.enqueEvent(&stopEvent) | ||
| if err != nil { | ||
| s.err = fmt.Errorf("failed to enqueue content_block_stop event: %w", err) | ||
| return false | ||
| } | ||
|
|
||
| s.contentIndex += 1 | ||
| if err := s.closeThinkingBlock(); err != nil { | ||
| s.err = fmt.Errorf("failed to close thinking block: %w", err) | ||
| return false | ||
| } |
There was a problem hiding this comment.
The closeThinkingBlock function is called here, but the error message in the fmt.Errorf call doesn't provide enough context about where the error occurred. Consider including the event type or other relevant information in the error message to aid in debugging.
if err := s.closeThinkingBlock(); err != nil {
s.err = fmt.Errorf("failed to close thinking block during redacted reasoning content handling: %w", err)
return false
}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Thinking Display field lost when req.Thinking is replaced by output_config effort fallback
When a request arrives with thinking.type = "adaptive", a non-empty thinking.display (e.g., "summarized"), and an output_config.effort value, and the outbound config targets a non-adaptive-supporting platform (e.g., DeepSeek, Doubao, Moonshot, etc.), the Display field is silently dropped. This happens because the display is set at line 89 on the current req.Thinking, but then at lines 99-102, req.Thinking is entirely replaced with a new Thinking{Type: "enabled", BudgetTokens: ...} struct that does not carry over the Display field.
Triggering flow in buildBaseRequest
- Line 78:
req.Thinking = &Thinking{Type: "adaptive"}(from metadata) - Line 89:
req.Thinking.Display = "summarized"(from metadata) - Line 98-102: Since
!supportsAdaptiveThinking(config)andreq.Thinking.Type == "adaptive", the struct is replaced:req.Thinking = &Thinking{Type: "enabled", BudgetTokens: ...}— Display is lost
(Refers to lines 98-103)
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Uh oh!
There was an error while loading. Please reload this page.