Fix responses adapter tool schema normalization#125
Merged
SantiagoDePolonia merged 1 commit intomainfrom Mar 8, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThese changes add normalization logic for converting Responses tool and tool choice formats to Chat-compatible structures, along with tests validating multimodal image input preservation and native tool format normalization. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/providers/responses_adapter_test.go`:
- Around line 101-155: The test "normalizes native responses tool format"
currently only asserts that tool "name" was promoted into the nested "function"
object; extend the test in internal/providers/responses_adapter_test.go to also
assert that "description" and "parameters" were promoted by
ConvertResponsesRequestToChat: check
req.Tools[0]["function"].(map[string]any)["description"] equals "Get weather by
city." and that ["parameters"] is present and has the expected properties (e.g.,
contains "city" with type "string"), and similarly assert
toolChoice.(map[string]any)["function"] contains the same "description" and
"parameters" fields so a regression dropping those promoted fields will fail the
test.
In `@internal/providers/responses_adapter.go`:
- Around line 85-87: The early return when tool["function"] is already a map
preserves an empty nested object; instead merge any missing top-level function
fields into that nested map before returning. In responses_adapter.go, locate
the branch that checks if _, ok := tool["function"].(map[string]any) and modify
it to: grab the nested map (the existing tool["function"]), for each top-level
keys you expect (e.g., "name", "description", "parameters") if they exist on
tool but are missing in nested, copy them into the nested map, assign the merged
map back to tool["function"], then return cloneStringAnyMap(tool). Apply the
same merge logic to the analogous branch at lines 115-117 (the
tool_choice.function case).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0c69254b-a063-424f-856d-a39a3e99940d
📒 Files selected for processing (3)
internal/providers/anthropic/anthropic_test.gointernal/providers/responses_adapter.gointernal/providers/responses_adapter_test.go
Comment on lines
+101
to
+155
| { | ||
| name: "normalizes native responses tool format", | ||
| input: &core.ResponsesRequest{ | ||
| Model: "test-model", | ||
| Input: "Hello", | ||
| Tools: []map[string]any{ | ||
| { | ||
| "type": "function", | ||
| "name": "lookup_weather", | ||
| "description": "Get weather by city.", | ||
| "parameters": map[string]any{ | ||
| "type": "object", | ||
| "properties": map[string]any{ | ||
| "city": map[string]any{"type": "string"}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| ToolChoice: map[string]any{ | ||
| "type": "function", | ||
| "name": "lookup_weather", | ||
| }, | ||
| }, | ||
| checkFn: func(t *testing.T, req *core.ChatRequest) { | ||
| if len(req.Tools) != 1 { | ||
| t.Fatalf("len(Tools) = %d, want 1", len(req.Tools)) | ||
| } | ||
|
|
||
| function, ok := req.Tools[0]["function"].(map[string]any) | ||
| if !ok { | ||
| t.Fatalf("Tools[0].function = %#v, want object", req.Tools[0]["function"]) | ||
| } | ||
| if function["name"] != "lookup_weather" { | ||
| t.Fatalf("Tools[0].function.name = %#v, want lookup_weather", function["name"]) | ||
| } | ||
| if _, ok := req.Tools[0]["name"]; ok { | ||
| t.Fatalf("Tools[0].name should be wrapped into function, got %+v", req.Tools[0]) | ||
| } | ||
|
|
||
| toolChoice, ok := req.ToolChoice.(map[string]any) | ||
| if !ok { | ||
| t.Fatalf("ToolChoice = %#v, want object", req.ToolChoice) | ||
| } | ||
| selected, ok := toolChoice["function"].(map[string]any) | ||
| if !ok { | ||
| t.Fatalf("ToolChoice.function = %#v, want object", toolChoice["function"]) | ||
| } | ||
| if selected["name"] != "lookup_weather" { | ||
| t.Fatalf("ToolChoice.function.name = %#v, want lookup_weather", selected["name"]) | ||
| } | ||
| if _, ok := toolChoice["name"]; ok { | ||
| t.Fatalf("ToolChoice.name should be wrapped into function, got %+v", toolChoice) | ||
| } | ||
| }, | ||
| }, |
Contributor
There was a problem hiding this comment.
Assert the promoted schema fields too.
This only proves name is wrapped. ConvertResponsesRequestToChat also promotes description and parameters, and Anthropic consumes those nested fields, so a regression dropping them would still pass here.
🧪 Suggested assertions
if function["name"] != "lookup_weather" {
t.Fatalf("Tools[0].function.name = %#v, want lookup_weather", function["name"])
}
+ if function["description"] != "Get weather by city." {
+ t.Fatalf("Tools[0].function.description = %#v, want preserved description", function["description"])
+ }
+ if _, ok := function["parameters"].(map[string]any); !ok {
+ t.Fatalf("Tools[0].function.parameters = %#v, want object", function["parameters"])
+ }
if _, ok := req.Tools[0]["name"]; ok {
t.Fatalf("Tools[0].name should be wrapped into function, got %+v", req.Tools[0])
}
+ if _, ok := req.Tools[0]["description"]; ok {
+ t.Fatalf("Tools[0].description should be wrapped into function, got %+v", req.Tools[0])
+ }
+ if _, ok := req.Tools[0]["parameters"]; ok {
+ t.Fatalf("Tools[0].parameters should be wrapped into function, got %+v", req.Tools[0])
+ }As per coding guidelines: **/*_test.go: Add or update tests for any behavior change.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| { | |
| name: "normalizes native responses tool format", | |
| input: &core.ResponsesRequest{ | |
| Model: "test-model", | |
| Input: "Hello", | |
| Tools: []map[string]any{ | |
| { | |
| "type": "function", | |
| "name": "lookup_weather", | |
| "description": "Get weather by city.", | |
| "parameters": map[string]any{ | |
| "type": "object", | |
| "properties": map[string]any{ | |
| "city": map[string]any{"type": "string"}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| ToolChoice: map[string]any{ | |
| "type": "function", | |
| "name": "lookup_weather", | |
| }, | |
| }, | |
| checkFn: func(t *testing.T, req *core.ChatRequest) { | |
| if len(req.Tools) != 1 { | |
| t.Fatalf("len(Tools) = %d, want 1", len(req.Tools)) | |
| } | |
| function, ok := req.Tools[0]["function"].(map[string]any) | |
| if !ok { | |
| t.Fatalf("Tools[0].function = %#v, want object", req.Tools[0]["function"]) | |
| } | |
| if function["name"] != "lookup_weather" { | |
| t.Fatalf("Tools[0].function.name = %#v, want lookup_weather", function["name"]) | |
| } | |
| if _, ok := req.Tools[0]["name"]; ok { | |
| t.Fatalf("Tools[0].name should be wrapped into function, got %+v", req.Tools[0]) | |
| } | |
| toolChoice, ok := req.ToolChoice.(map[string]any) | |
| if !ok { | |
| t.Fatalf("ToolChoice = %#v, want object", req.ToolChoice) | |
| } | |
| selected, ok := toolChoice["function"].(map[string]any) | |
| if !ok { | |
| t.Fatalf("ToolChoice.function = %#v, want object", toolChoice["function"]) | |
| } | |
| if selected["name"] != "lookup_weather" { | |
| t.Fatalf("ToolChoice.function.name = %#v, want lookup_weather", selected["name"]) | |
| } | |
| if _, ok := toolChoice["name"]; ok { | |
| t.Fatalf("ToolChoice.name should be wrapped into function, got %+v", toolChoice) | |
| } | |
| }, | |
| }, | |
| { | |
| name: "normalizes native responses tool format", | |
| input: &core.ResponsesRequest{ | |
| Model: "test-model", | |
| Input: "Hello", | |
| Tools: []map[string]any{ | |
| { | |
| "type": "function", | |
| "name": "lookup_weather", | |
| "description": "Get weather by city.", | |
| "parameters": map[string]any{ | |
| "type": "object", | |
| "properties": map[string]any{ | |
| "city": map[string]any{"type": "string"}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| ToolChoice: map[string]any{ | |
| "type": "function", | |
| "name": "lookup_weather", | |
| }, | |
| }, | |
| checkFn: func(t *testing.T, req *core.ChatRequest) { | |
| if len(req.Tools) != 1 { | |
| t.Fatalf("len(Tools) = %d, want 1", len(req.Tools)) | |
| } | |
| function, ok := req.Tools[0]["function"].(map[string]any) | |
| if !ok { | |
| t.Fatalf("Tools[0].function = %#v, want object", req.Tools[0]["function"]) | |
| } | |
| if function["name"] != "lookup_weather" { | |
| t.Fatalf("Tools[0].function.name = %#v, want lookup_weather", function["name"]) | |
| } | |
| if function["description"] != "Get weather by city." { | |
| t.Fatalf("Tools[0].function.description = %#v, want preserved description", function["description"]) | |
| } | |
| if _, ok := function["parameters"].(map[string]any); !ok { | |
| t.Fatalf("Tools[0].function.parameters = %#v, want object", function["parameters"]) | |
| } | |
| if _, ok := req.Tools[0]["name"]; ok { | |
| t.Fatalf("Tools[0].name should be wrapped into function, got %+v", req.Tools[0]) | |
| } | |
| if _, ok := req.Tools[0]["description"]; ok { | |
| t.Fatalf("Tools[0].description should be wrapped into function, got %+v", req.Tools[0]) | |
| } | |
| if _, ok := req.Tools[0]["parameters"]; ok { | |
| t.Fatalf("Tools[0].parameters should be wrapped into function, got %+v", req.Tools[0]) | |
| } | |
| toolChoice, ok := req.ToolChoice.(map[string]any) | |
| if !ok { | |
| t.Fatalf("ToolChoice = %#v, want object", req.ToolChoice) | |
| } | |
| selected, ok := toolChoice["function"].(map[string]any) | |
| if !ok { | |
| t.Fatalf("ToolChoice.function = %#v, want object", toolChoice["function"]) | |
| } | |
| if selected["name"] != "lookup_weather" { | |
| t.Fatalf("ToolChoice.function.name = %#v, want lookup_weather", selected["name"]) | |
| } | |
| if _, ok := toolChoice["name"]; ok { | |
| t.Fatalf("ToolChoice.name should be wrapped into function, got %+v", toolChoice) | |
| } | |
| }, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/providers/responses_adapter_test.go` around lines 101 - 155, The
test "normalizes native responses tool format" currently only asserts that tool
"name" was promoted into the nested "function" object; extend the test in
internal/providers/responses_adapter_test.go to also assert that "description"
and "parameters" were promoted by ConvertResponsesRequestToChat: check
req.Tools[0]["function"].(map[string]any)["description"] equals "Get weather by
city." and that ["parameters"] is present and has the expected properties (e.g.,
contains "city" with type "string"), and similarly assert
toolChoice.(map[string]any)["function"] contains the same "description" and
"parameters" fields so a regression dropping those promoted fields will fail the
test.
Comment on lines
+85
to
+87
| if _, ok := tool["function"].(map[string]any); ok { | ||
| return cloneStringAnyMap(tool) | ||
| } |
Contributor
There was a problem hiding this comment.
Backfill hybrid function shapes instead of bailing out early.
A payload like {"type":"function","function":{},"name":"lookup_weather"} still has enough data to normalize, but these early returns preserve the empty nested object and later fail downstream when internal/providers/anthropic/anthropic.go checks tool.function.name / tool_choice.function.name. Merge missing top-level fields into the existing function object before returning.
💡 Suggested fix
func normalizeResponsesToolForChat(tool map[string]any) map[string]any {
if len(tool) == 0 {
- return tool
+ return cloneStringAnyMap(tool)
}
toolType, _ := tool["type"].(string)
if strings.TrimSpace(toolType) != "function" {
return cloneStringAnyMap(tool)
}
- if _, ok := tool["function"].(map[string]any); ok {
- return cloneStringAnyMap(tool)
- }
normalized := cloneStringAnyMap(tool)
- function := map[string]any{}
+ function, _ := normalized["function"].(map[string]any)
+ function = cloneStringAnyMap(function)
+ if function == nil {
+ function = map[string]any{}
+ }
for _, key := range []string{"name", "description", "parameters", "strict"} {
if value, ok := normalized[key]; ok {
- function[key] = value
+ if _, exists := function[key]; !exists {
+ function[key] = value
+ }
delete(normalized, key)
}
}
if len(function) == 0 {
return normalized
@@
func normalizeResponsesToolChoiceForChat(choice any) any {
choiceMap, ok := choice.(map[string]any)
if !ok {
return choice
}
@@
choiceType, _ := choiceMap["type"].(string)
if strings.TrimSpace(choiceType) != "function" {
return choice
}
- if _, ok := choiceMap["function"].(map[string]any); ok {
- return cloneStringAnyMap(choiceMap)
- }
-
- name, hasName := choiceMap["name"]
- if !hasName {
- return cloneStringAnyMap(choiceMap)
- }
-
normalized := cloneStringAnyMap(choiceMap)
- delete(normalized, "name")
- normalized["function"] = map[string]any{"name": name}
+ function, _ := normalized["function"].(map[string]any)
+ function = cloneStringAnyMap(function)
+ if function == nil {
+ function = map[string]any{}
+ }
+ if _, exists := function["name"]; !exists {
+ if name, hasName := normalized["name"]; hasName {
+ function["name"] = name
+ }
+ }
+ delete(normalized, "name")
+ if len(function) > 0 {
+ normalized["function"] = function
+ }
return normalized
}Also applies to: 115-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/providers/responses_adapter.go` around lines 85 - 87, The early
return when tool["function"] is already a map preserves an empty nested object;
instead merge any missing top-level function fields into that nested map before
returning. In responses_adapter.go, locate the branch that checks if _, ok :=
tool["function"].(map[string]any) and modify it to: grab the nested map (the
existing tool["function"]), for each top-level keys you expect (e.g., "name",
"description", "parameters") if they exist on tool but are missing in nested,
copy them into the nested map, assign the merged map back to tool["function"],
then return cloneStringAnyMap(tool). Apply the same merge logic to the analogous
branch at lines 115-117 (the tool_choice.function case).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
Tests
Chores