Conversation
There was a problem hiding this comment.
looplj has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
Code Review
This pull request introduces custom JSON marshaling for MessageContentBlock and StreamDelta within the Anthropic transformer to handle thinking blocks. The implementation ensures that the signature field is included in the output even when empty. Review feedback suggests that the thinking field should also be explicitly included for consistency with the API schema and that the test suite should be expanded to include nil pointer scenarios to properly verify the custom marshaling behavior.
| if b.Type == "thinking" { | ||
| type thinkingBlock struct { | ||
| blockAlias | ||
| Signature string `json:"signature"` | ||
| } | ||
|
|
||
| return json.Marshal(thinkingBlock{ | ||
| blockAlias: blockAlias(b), | ||
| Signature: lo.FromPtr(b.Signature), | ||
| }) | ||
| } |
There was a problem hiding this comment.
For thinking content blocks, both the thinking and signature fields are typically required by the Anthropic API schema. While you've correctly forced the signature field to be present (even if empty), the thinking field should also be handled similarly to ensure it's not omitted when it's an empty string or nil.
| if b.Type == "thinking" { | |
| type thinkingBlock struct { | |
| blockAlias | |
| Signature string `json:"signature"` | |
| } | |
| return json.Marshal(thinkingBlock{ | |
| blockAlias: blockAlias(b), | |
| Signature: lo.FromPtr(b.Signature), | |
| }) | |
| } | |
| if b.Type == "thinking" { | |
| type thinkingBlock struct { | |
| blockAlias | |
| Thinking string "json:\"thinking\"" | |
| Signature string "json:\"signature\"" | |
| } | |
| return json.Marshal(thinkingBlock{ | |
| blockAlias: blockAlias(b), | |
| Thinking: lo.FromPtr(b.Thinking), | |
| Signature: lo.FromPtr(b.Signature), | |
| }) | |
| } |
| func TestMessageContentBlockMarshalJSON_PreservesEmptyThinkingSignature(t *testing.T) { | ||
| data, err := json.Marshal(MessageContentBlock{ | ||
| Type: "thinking", | ||
| Thinking: lo.ToPtr(""), | ||
| Signature: lo.ToPtr(""), | ||
| }) | ||
| require.NoError(t, err) | ||
| require.JSONEq(t, `{"type":"thinking","thinking":"","signature":""}`, string(data)) | ||
| } |
There was a problem hiding this comment.
The current test uses lo.ToPtr(""), which doesn't fully verify the custom MarshalJSON logic because a non-nil pointer to an empty string is already preserved by the default JSON marshaler (it's not considered an "empty value" for the pointer itself). To properly test that the custom marshaler forces these fields to be present even when they are uninitialized, you should add a test case where Thinking and Signature are nil.
There was a problem hiding this comment.
looplj has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Uh oh!
There was an error while loading. Please reload this page.