From af7f7c5d11370e91e0263a6ccd3fe47d47915235 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Mon, 6 Jul 2026 23:01:16 +0100 Subject: [PATCH] Fix Gemini Patrol tool-call readiness Refs #1530 --- .../v6/internal/subsystems/ai-runtime.md | 18 ++ internal/ai/patrol_preflight.go | 3 + internal/ai/providers/anthropic.go | 35 +-- internal/ai/providers/anthropic_test.go | 67 ++++++ internal/ai/providers/gemini.go | 199 ++++++++++-------- internal/ai/providers/gemini_stream_test.go | 39 ++++ internal/ai/providers/gemini_test.go | 107 +++++++++- internal/ai/providers/openai.go | 17 +- internal/ai/providers/openai_test.go | 60 ++++++ internal/ai/providers/provider.go | 5 +- 10 files changed, 447 insertions(+), 103 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/ai-runtime.md b/docs/release-control/v6/internal/subsystems/ai-runtime.md index 3f71e1edc..6522a7aba 100644 --- a/docs/release-control/v6/internal/subsystems/ai-runtime.md +++ b/docs/release-control/v6/internal/subsystems/ai-runtime.md @@ -153,6 +153,24 @@ Provider definitions that declare a BaseURLField (today OpenAI, Ollama, and Z.ai) expose a user-overridable endpoint via the AI settings payload; the registry default base URL applies when no override is stored, so one provider can serve both standard and alternate (e.g. Z.ai coding) endpoint tiers. +Native provider tool-calling is a transport projection over the shared +provider-neutral `Tool`, `ToolChoice`, `ToolCall`, and `ToolResult` shapes. +Nil `ToolChoice` means provider-owned automatic selection, `none` disables +tools by omitting them from the provider request, and `required` is reserved for +callers that explicitly need provider-native forced tool use. That required mode +maps to OpenAI `required`, Anthropic `any`, and Gemini +`FunctionCallingConfig` `ANY`; normal chat and Patrol runs must not force tool +use unless the caller sets that override. Patrol's static model-readiness +self-test may use required mode for Gemini because it is a fixed capability +probe with no infrastructure identifiers and needs deterministic tool-call +proof. Provider-specific schema restrictions belong at the native transport +edge: Gemini strips unsupported `additionalProperties` only from Gemini function +declarations, while the registry-owned provider-neutral schemas and providers +that support stricter JSON Schema keep that field. Gemini function-call +continuations must preserve the provider `functionCall.id` on parsed +`ToolCall.ID` values and return that same value as `functionResponse.id`; the +function name remains a separate field resolved from the preceding assistant +call when building tool-result turns. ## Canonical Files diff --git a/internal/ai/patrol_preflight.go b/internal/ai/patrol_preflight.go index c1c73a592..51b67caa2 100644 --- a/internal/ai/patrol_preflight.go +++ b/internal/ai/patrol_preflight.go @@ -199,6 +199,9 @@ func (s *Service) RunPatrolToolPreflight(ctx context.Context, providerName, mode }, MaxTokens: 256, } + if parsedProvider == config.AIProviderGemini { + req.ToolChoice = &providers.ToolChoice{Type: providers.ToolChoiceRequired} + } resp, err := provider.Chat(ctx, req) result.DurationMs = time.Since(started).Milliseconds() diff --git a/internal/ai/providers/anthropic.go b/internal/ai/providers/anthropic.go index 28325691a..7dbc8f93e 100644 --- a/internal/ai/providers/anthropic.go +++ b/internal/ai/providers/anthropic.go @@ -73,10 +73,24 @@ type anthropicRequest struct { ToolChoice *anthropicToolChoice `json:"tool_choice,omitempty"` } -// anthropicToolChoice controls whether tools are available or disabled. -// Pulse uses automatic selection by default and none only as a safety brake. +// anthropicToolChoice controls whether tools are automatic, required, or disabled. +// Pulse omits automatic selection by default and only serializes explicit overrides. type anthropicToolChoice struct { - Type string `json:"type"` // "auto" or "none" + Type string `json:"type"` // "auto", "any", or "none" +} + +func convertToolChoiceToAnthropic(tc *ToolChoice) *anthropicToolChoice { + if tc == nil { + return nil + } + switch tc.Type { + case ToolChoiceNone: + return &anthropicToolChoice{Type: "none"} + case ToolChoiceRequired: + return &anthropicToolChoice{Type: "any"} + default: + return nil + } } type anthropicMessage struct { @@ -269,12 +283,10 @@ func (c *AnthropicClient) Chat(ctx context.Context, req ChatRequest) (*ChatRespo anthropicReq.Tools[len(anthropicReq.Tools)-1].CacheControl = &anthropicCacheControl{Type: "ephemeral"} } - // Add tool_choice if specified. Pulse only sends automatic selection or - // text-only safety brakes. + // Add tool_choice only for explicit overrides. Nil keeps Anthropic's default + // automatic tool selection. if shouldAddTools && req.ToolChoice != nil { - anthropicReq.ToolChoice = &anthropicToolChoice{ - Type: string(req.ToolChoice.Type), - } + anthropicReq.ToolChoice = convertToolChoiceToAnthropic(req.ToolChoice) } body, err := json.Marshal(anthropicReq) @@ -606,12 +618,9 @@ func (c *AnthropicClient) ChatStream(ctx context.Context, req ChatRequest, callb anthropicReq.Tools[len(anthropicReq.Tools)-1].CacheControl = &anthropicCacheControl{Type: "ephemeral"} } - // Add tool_choice if specified (same as non-streaming). Pulse only sends - // automatic selection or text-only safety brakes. + // Add tool_choice only for explicit overrides, same as non-streaming. if shouldAddTools && req.ToolChoice != nil { - anthropicReq.ToolChoice = &anthropicToolChoice{ - Type: string(req.ToolChoice.Type), - } + anthropicReq.ToolChoice = convertToolChoiceToAnthropic(req.ToolChoice) } body, err := json.Marshal(anthropicReq) diff --git a/internal/ai/providers/anthropic_test.go b/internal/ai/providers/anthropic_test.go index c611768bb..12ff434f0 100644 --- a/internal/ai/providers/anthropic_test.go +++ b/internal/ai/providers/anthropic_test.go @@ -210,6 +210,73 @@ func TestAnthropicClient_Chat_ToolChoiceNone_DropsTools(t *testing.T) { } } +func TestAnthropicClient_Chat_ToolChoiceRequired_KeepsToolsAndUsesAny(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var got map[string]any + if err := json.NewDecoder(r.Body).Decode(&got); err != nil { + t.Fatalf("decode request: %v", err) + } + + toolChoice, ok := got["tool_choice"].(map[string]any) + if !ok { + t.Fatalf("tool_choice field should be present when tools are required: %+v", got["tool_choice"]) + } + if toolChoice["type"] != "any" { + t.Fatalf("tool_choice.type = %v, want any", toolChoice["type"]) + } + tools, ok := got["tools"].([]any) + if !ok || len(tools) != 1 { + t.Fatalf("tools field should contain one tool when tool_choice is required: %+v", got["tools"]) + } + tool, ok := tools[0].(map[string]any) + if !ok || tool["name"] != "get_time" { + t.Fatalf("unexpected tool definition: %+v", tools[0]) + } + + _ = json.NewEncoder(w).Encode(anthropicResponse{ + ID: "msg_123", + Type: "message", + Role: "assistant", + Model: "claude-3-5-sonnet", + StopReason: "tool_use", + Content: []anthropicContent{ + { + Type: "tool_use", + ID: "tool_1", + Name: "get_time", + Input: map[string]interface{}{"tz": "UTC"}, + }, + }, + }) + })) + defer server.Close() + + client := NewAnthropicClientWithBaseURL("test-key", "claude-3-5-sonnet", server.URL+"/v1/messages", 0) + out, err := client.Chat(context.Background(), ChatRequest{ + Messages: []Message{{Role: "user", Content: "Hello"}}, + Tools: []Tool{ + { + Name: "get_time", + Description: "get time", + InputSchema: map[string]any{"type": "object"}, + }, + }, + ToolChoice: &ToolChoice{Type: ToolChoiceRequired}, + }) + if err != nil { + t.Fatalf("Chat: %v", err) + } + if out.StopReason != "tool_use" { + t.Fatalf("StopReason = %q, want tool_use", out.StopReason) + } + if len(out.ToolCalls) != 1 || out.ToolCalls[0].ID != "tool_1" || out.ToolCalls[0].Name != "get_time" { + t.Fatalf("unexpected ToolCalls: %+v", out.ToolCalls) + } + if out.ToolCalls[0].Input["tz"] != "UTC" { + t.Fatalf("unexpected ToolCall input: %+v", out.ToolCalls[0].Input) + } +} + func TestAnthropicClient_Chat_ToolResultInRequest(t *testing.T) { var got map[string]any server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/ai/providers/gemini.go b/internal/ai/providers/gemini.go index 3dc992260..ae21929f3 100644 --- a/internal/ai/providers/gemini.go +++ b/internal/ai/providers/gemini.go @@ -34,7 +34,6 @@ func NewGeminiClient(apiKey, model, baseURL string, timeout time.Duration) *Gemi baseURL = geminiAPIURL } // Strip provider prefix if present - the model should be just the model name - // Strip provider prefix if present - the model should be just the model name model = strings.TrimPrefix(model, "gemini:") if timeout <= 0 { timeout = 300 * time.Second // Default 5 minutes @@ -87,11 +86,13 @@ type geminiPart struct { } type geminiFunctionCall struct { + ID string `json:"id,omitempty"` Name string `json:"name"` Args map[string]interface{} `json:"args"` } type geminiFunctionResponse struct { + ID string `json:"id,omitempty"` Name string `json:"name"` Response struct { Content string `json:"content"` @@ -258,13 +259,92 @@ func sanitizeGeminiContents(contents []geminiContent) []geminiContent { } // convertToolChoiceToGemini converts our ToolChoice to Gemini's mode string. -// Pulse omits automatic tool config so tool use stays model-owned. -// See: https://ai.google.dev/api/caching#FunctionCallingConfig +// Pulse omits automatic tool config so normal tool use stays model-owned. +// See: https://ai.google.dev/gemini-api/docs/generate-content#function_calling func convertToolChoiceToGemini(tc *ToolChoice) string { - if tc != nil && tc.Type == ToolChoiceNone { - return "NONE" + if tc == nil { + return "" + } + switch tc.Type { + case ToolChoiceNone: + return "NONE" + case ToolChoiceRequired: + return "ANY" + default: + return "" + } +} + +func buildGeminiFunctionDeclarations(tools []Tool) []geminiFunctionDeclaration { + funcDecls := make([]geminiFunctionDeclaration, 0, len(tools)) + for _, t := range tools { + if t.Type != "" && t.Type != "function" { + continue + } + funcDecls = append(funcDecls, geminiFunctionDeclaration{ + Name: t.Name, + Description: t.Description, + Parameters: sanitizeGeminiToolSchema(t.InputSchema), + }) + } + return funcDecls +} + +func applyGeminiTools(geminiReq *geminiRequest, tools []Tool, toolChoice *ToolChoice) []geminiFunctionDeclaration { + shouldAddTools := len(tools) > 0 + if toolChoice != nil && toolChoice.Type == ToolChoiceNone { + shouldAddTools = false + } + if !shouldAddTools { + return nil + } + + funcDecls := buildGeminiFunctionDeclarations(tools) + if len(funcDecls) == 0 { + return nil + } + geminiReq.Tools = []geminiToolDef{{FunctionDeclarations: funcDecls}} + if mode := convertToolChoiceToGemini(toolChoice); mode != "" { + geminiReq.ToolConfig = &geminiToolConfig{ + FunctionCallingConfig: &geminiFunctionCallingConfig{Mode: mode}, + } + } + return funcDecls +} + +// sanitizeGeminiToolSchema projects Pulse's provider-neutral JSON schema onto +// Gemini's function declaration subset. Gemini rejects additionalProperties in +// function parameters, while other providers rely on it for stricter schemas. +func sanitizeGeminiToolSchema(schema map[string]interface{}) map[string]interface{} { + if schema == nil { + return nil + } + sanitized, _ := sanitizeGeminiSchemaValue(schema).(map[string]interface{}) + return sanitized +} + +func sanitizeGeminiSchemaValue(value interface{}) interface{} { + switch v := value.(type) { + case map[string]interface{}: + out := make(map[string]interface{}, len(v)) + for key, item := range v { + if key == "additionalProperties" { + continue + } + out[key] = sanitizeGeminiSchemaValue(item) + } + return out + case []interface{}: + out := make([]interface{}, len(v)) + for i, item := range v { + out[i] = sanitizeGeminiSchemaValue(item) + } + return out + case []string: + return append([]string(nil), v...) + default: + return v } - return "" } // convertMessagesToGemini converts provider-neutral messages into Gemini's @@ -302,13 +382,13 @@ func convertMessagesToGemini(reqMessages []Message) []geminiContent { return id } - parts := []geminiPart{geminiFunctionResponsePart(resolveName(m.ToolResult.ToolUseID), m.ToolResult.Content)} + parts := []geminiPart{geminiFunctionResponsePart(m.ToolResult.ToolUseID, resolveName(m.ToolResult.ToolUseID), m.ToolResult.Content)} for i+1 < len(reqMessages) { next := reqMessages[i+1] if next.ToolResult == nil { break } - parts = append(parts, geminiFunctionResponsePart(resolveName(next.ToolResult.ToolUseID), next.ToolResult.Content)) + parts = append(parts, geminiFunctionResponsePart(next.ToolResult.ToolUseID, resolveName(next.ToolResult.ToolUseID), next.ToolResult.Content)) i++ } @@ -327,6 +407,7 @@ func convertMessagesToGemini(reqMessages []Message) []geminiContent { for _, tc := range m.ToolCalls { parts = append(parts, geminiPart{ FunctionCall: &geminiFunctionCall{ + ID: tc.ID, Name: tc.Name, Args: tc.Input, }, @@ -355,8 +436,8 @@ func convertMessagesToGemini(reqMessages []Message) []geminiContent { return sanitizeGeminiContents(contents) } -func geminiFunctionResponsePart(name, content string) geminiPart { - response := geminiFunctionResponse{Name: name} +func geminiFunctionResponsePart(id, name, content string) geminiPart { + response := geminiFunctionResponse{ID: id, Name: name} response.Response.Content = content return geminiPart{FunctionResponse: &response} } @@ -398,41 +479,15 @@ func (c *GeminiClient) Chat(ctx context.Context, req ChatRequest) (*ChatResponse geminiReq.GenerationConfig.Temperature = req.Temperature } - // Add tools if provided (unless ToolChoice is None) - shouldAddTools := len(req.Tools) > 0 - if req.ToolChoice != nil && req.ToolChoice.Type == ToolChoiceNone { - shouldAddTools = false - } - - if shouldAddTools { - funcDecls := make([]geminiFunctionDeclaration, 0, len(req.Tools)) - for _, t := range req.Tools { - // Skip non-function tools - if t.Type != "" && t.Type != "function" { - continue + funcDecls := applyGeminiTools(&geminiReq, req.Tools, req.ToolChoice) + if len(funcDecls) > 0 { + log.Debug().Int("tool_count", len(funcDecls)).Strs("tool_names", func() []string { + names := make([]string, len(funcDecls)) + for i, f := range funcDecls { + names[i] = f.Name } - funcDecls = append(funcDecls, geminiFunctionDeclaration{ - Name: t.Name, - Description: t.Description, - Parameters: t.InputSchema, - }) - } - if len(funcDecls) > 0 { - geminiReq.Tools = []geminiToolDef{{FunctionDeclarations: funcDecls}} - if mode := convertToolChoiceToGemini(req.ToolChoice); mode != "" { - geminiReq.ToolConfig = &geminiToolConfig{ - FunctionCallingConfig: &geminiFunctionCallingConfig{Mode: mode}, - } - } - - log.Debug().Int("tool_count", len(funcDecls)).Strs("tool_names", func() []string { - names := make([]string, len(funcDecls)) - for i, f := range funcDecls { - names[i] = f.Name - } - return names - }()).Msg("gemini request includes tools") - } + return names + }()).Msg("gemini request includes tools") } body, err := json.Marshal(geminiReq) @@ -576,9 +631,10 @@ func (c *GeminiClient) Chat(ctx context.Context, req ChatRequest) (*ChatResponse textContent += part.Text } if part.FunctionCall != nil { - // Generate a unique ID for this tool call since Gemini doesn't provide one - // Use name + index to ensure uniqueness when same function is called multiple times - toolID := fmt.Sprintf("%s_%d", part.FunctionCall.Name, len(toolCalls)) + toolID := strings.TrimSpace(part.FunctionCall.ID) + if toolID == "" { + toolID = fmt.Sprintf("%s_%d", part.FunctionCall.Name, len(toolCalls)) + } signature := part.ThoughtSignature if len(signature) == 0 { signature = part.ThoughtSignatureSnake @@ -677,42 +733,16 @@ func (c *GeminiClient) ChatStream(ctx context.Context, req ChatRequest, callback geminiReq.GenerationConfig.Temperature = req.Temperature } - // Add tools if provided (unless ToolChoice is None) - same as non-streaming - shouldAddTools := len(req.Tools) > 0 - if req.ToolChoice != nil && req.ToolChoice.Type == ToolChoiceNone { - shouldAddTools = false - } - - if shouldAddTools { - funcDecls := make([]geminiFunctionDeclaration, 0, len(req.Tools)) - for _, t := range req.Tools { - if t.Type != "" && t.Type != "function" { - continue - } - funcDecls = append(funcDecls, geminiFunctionDeclaration{ - Name: t.Name, - Description: t.Description, - Parameters: t.InputSchema, - }) - } - if len(funcDecls) > 0 { - geminiReq.Tools = []geminiToolDef{{FunctionDeclarations: funcDecls}} - if mode := convertToolChoiceToGemini(req.ToolChoice); mode != "" { - geminiReq.ToolConfig = &geminiToolConfig{ - FunctionCallingConfig: &geminiFunctionCallingConfig{Mode: mode}, - } - } - - // Log tool names for debugging tool selection issues - toolNames := make([]string, len(funcDecls)) - for i, f := range funcDecls { - toolNames[i] = f.Name - } - log.Debug(). - Int("tool_count", len(funcDecls)). - Strs("tool_names", toolNames). - Msg("Gemini stream request includes tools") + funcDecls := applyGeminiTools(&geminiReq, req.Tools, req.ToolChoice) + if len(funcDecls) > 0 { + toolNames := make([]string, len(funcDecls)) + for i, f := range funcDecls { + toolNames[i] = f.Name } + log.Debug(). + Int("tool_count", len(funcDecls)). + Strs("tool_names", toolNames). + Msg("Gemini stream request includes tools") } body, err := json.Marshal(geminiReq) @@ -878,7 +908,10 @@ func (c *GeminiClient) ChatStream(ctx context.Context, req ChatRequest, callback } if part.FunctionCall != nil { - toolID := fmt.Sprintf("%s_%d", part.FunctionCall.Name, len(toolCalls)) + toolID := strings.TrimSpace(part.FunctionCall.ID) + if toolID == "" { + toolID = fmt.Sprintf("%s_%d", part.FunctionCall.Name, len(toolCalls)) + } signature := part.ThoughtSignature if len(signature) == 0 { signature = part.ThoughtSignatureSnake diff --git a/internal/ai/providers/gemini_stream_test.go b/internal/ai/providers/gemini_stream_test.go index ffefe9bf3..94e15e865 100644 --- a/internal/ai/providers/gemini_stream_test.go +++ b/internal/ai/providers/gemini_stream_test.go @@ -150,3 +150,42 @@ func TestChatStream_ToolResults_MultipleMerged(t *testing.T) { assert.Equal(t, "func3", mergedUserMsg.Parts[2].FunctionResponse.Name) assert.Equal(t, "res3", mergedUserMsg.Parts[2].FunctionResponse.Response.Content) } + +func TestGeminiClient_ChatStream_ToolChoiceRequiredUsesAnyAndSanitizesSchema(t *testing.T) { + var capturedBody geminiRequest + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = json.NewDecoder(r.Body).Decode(&capturedBody) + w.Header().Set("Content-Type", "text/event-stream") + w.Write([]byte("data: {}\n\n")) + })) + defer ts.Close() + + client := NewGeminiClient("fake-key", "gemini-pro", ts.URL, 10*time.Second) + + err := client.ChatStream(context.Background(), ChatRequest{ + Messages: []Message{{Role: "user", Content: "Run the self-test"}}, + Tools: []Tool{{ + Name: "verify_pulse_patrol", + InputSchema: map[string]interface{}{ + "type": "object", + "additionalProperties": false, + "properties": map[string]interface{}{ + "ok": map[string]interface{}{ + "type": "boolean", + "additionalProperties": false, + }, + }, + "required": []string{"ok"}, + }, + }}, + ToolChoice: &ToolChoice{Type: ToolChoiceRequired}, + }, func(event StreamEvent) {}) + + assert.NoError(t, err) + if assert.NotNil(t, capturedBody.ToolConfig) && assert.NotNil(t, capturedBody.ToolConfig.FunctionCallingConfig) { + assert.Equal(t, "ANY", capturedBody.ToolConfig.FunctionCallingConfig.Mode) + } + if assert.Len(t, capturedBody.Tools, 1) && assert.Len(t, capturedBody.Tools[0].FunctionDeclarations, 1) { + assert.False(t, hasMapKeyDeep(capturedBody.Tools[0].FunctionDeclarations[0].Parameters, "additionalProperties")) + } +} diff --git a/internal/ai/providers/gemini_test.go b/internal/ai/providers/gemini_test.go index e84fae885..47367395f 100644 --- a/internal/ai/providers/gemini_test.go +++ b/internal/ai/providers/gemini_test.go @@ -277,6 +277,7 @@ func TestGeminiClient_Chat_ToolCalls(t *testing.T) { Parts: []geminiPart{ { FunctionCall: &geminiFunctionCall{ + ID: "gemini-call-weather", Name: "get_weather", Args: map[string]interface{}{"location": "NYC"}, }, @@ -320,6 +321,77 @@ func TestGeminiClient_Chat_ToolCalls(t *testing.T) { if resp.ToolCalls[0].Name != "get_weather" { t.Errorf("expected tool name 'get_weather', got %q", resp.ToolCalls[0].Name) } + if resp.ToolCalls[0].ID != "gemini-call-weather" { + t.Errorf("expected provider tool id to be preserved, got %q", resp.ToolCalls[0].ID) + } +} + +func TestGeminiClient_Chat_ToolChoiceRequiredUsesAnyAndSanitizesSchema(t *testing.T) { + var receivedReq geminiRequest + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = json.NewDecoder(r.Body).Decode(&receivedReq) + resp := geminiResponse{ + Candidates: []geminiCandidate{ + { + Content: geminiContent{ + Parts: []geminiPart{{ + FunctionCall: &geminiFunctionCall{ + ID: "call-verify-1", + Name: "verify_pulse_patrol", + Args: map[string]interface{}{"ok": true}, + }, + }}, + }, + FinishReason: "STOP", + }, + }, + } + _ = json.NewEncoder(w).Encode(resp) + })) + defer server.Close() + + client := NewGeminiClient("test-key", "gemini-pro", server.URL, 0) + + resp, err := client.Chat(context.Background(), ChatRequest{ + Messages: []Message{{Role: "user", Content: "Run the self-test"}}, + Tools: []Tool{{ + Name: "verify_pulse_patrol", + Description: "Verify Patrol tool calling", + InputSchema: map[string]interface{}{ + "type": "object", + "additionalProperties": false, + "properties": map[string]interface{}{ + "ok": map[string]interface{}{ + "type": "boolean", + "description": "Always true.", + "additionalProperties": false, + }, + }, + "required": []string{"ok"}, + }, + }}, + ToolChoice: &ToolChoice{Type: ToolChoiceRequired}, + }) + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if receivedReq.ToolConfig == nil || receivedReq.ToolConfig.FunctionCallingConfig == nil { + t.Fatalf("expected Gemini toolConfig in required mode, got %+v", receivedReq.ToolConfig) + } + if receivedReq.ToolConfig.FunctionCallingConfig.Mode != "ANY" { + t.Fatalf("Gemini function calling mode = %q, want ANY", receivedReq.ToolConfig.FunctionCallingConfig.Mode) + } + if len(receivedReq.Tools) != 1 || len(receivedReq.Tools[0].FunctionDeclarations) != 1 { + t.Fatalf("expected one Gemini function declaration, got %+v", receivedReq.Tools) + } + params := receivedReq.Tools[0].FunctionDeclarations[0].Parameters + if hasMapKeyDeep(params, "additionalProperties") { + t.Fatalf("Gemini function parameters leaked unsupported additionalProperties: %#v", params) + } + if len(resp.ToolCalls) != 1 || resp.ToolCalls[0].ID != "call-verify-1" { + t.Fatalf("expected provider function-call id to survive response parsing, got %+v", resp.ToolCalls) + } } func TestGeminiClient_Chat_APIError(t *testing.T) { @@ -612,7 +684,7 @@ func TestGeminiClient_Chat_ToolResultsAndAssistantToolCalls(t *testing.T) { _, err := client.Chat(context.Background(), ChatRequest{ Messages: []Message{ {Role: "assistant", Content: "Calling tool", ToolCalls: []ToolCall{{ID: "tc1", Name: "get_time", Input: map[string]any{"tz": "UTC"}}}}, - {Role: "user", ToolResult: &ToolResult{ToolUseID: "get_time", Content: "{\"time\":\"00:00\"}"}}, + {Role: "user", ToolResult: &ToolResult{ToolUseID: "tc1", Content: "{\"time\":\"00:00\"}"}}, }, }) if err != nil { @@ -626,10 +698,19 @@ func TestGeminiClient_Chat_ToolResultsAndAssistantToolCalls(t *testing.T) { if got.Contents[0].Role != "model" || got.Contents[0].Parts[1].FunctionCall == nil { t.Errorf("Expected model role with function call, got %+v", got.Contents[0]) } + if got.Contents[0].Parts[1].FunctionCall.ID != "tc1" { + t.Errorf("Expected model function call id tc1, got %+v", got.Contents[0].Parts[1].FunctionCall) + } // Check tool result if got.Contents[1].Role != "user" || got.Contents[1].Parts[0].FunctionResponse == nil { t.Errorf("Expected user role with function response, got %+v", got.Contents[1]) } + if got.Contents[1].Parts[0].FunctionResponse.ID != "tc1" { + t.Errorf("Expected function response id tc1, got %+v", got.Contents[1].Parts[0].FunctionResponse) + } + if got.Contents[1].Parts[0].FunctionResponse.Name != "get_time" { + t.Errorf("Expected function response name get_time, got %+v", got.Contents[1].Parts[0].FunctionResponse) + } } func TestGeminiClient_Chat_ResolvesAndGroupsToolResults(t *testing.T) { @@ -673,9 +754,33 @@ func TestGeminiClient_Chat_ResolvesAndGroupsToolResults(t *testing.T) { if resultParts[0].FunctionResponse == nil || resultParts[0].FunctionResponse.Name != "get_time" { t.Fatalf("first function response = %+v, want get_time", resultParts[0].FunctionResponse) } + if resultParts[0].FunctionResponse.ID != "call-time" { + t.Fatalf("first function response id = %q, want call-time", resultParts[0].FunctionResponse.ID) + } if resultParts[1].FunctionResponse == nil || resultParts[1].FunctionResponse.Name != "get_weather" { t.Fatalf("second function response = %+v, want get_weather", resultParts[1].FunctionResponse) } + if resultParts[1].FunctionResponse.ID != "call-weather" { + t.Fatalf("second function response id = %q, want call-weather", resultParts[1].FunctionResponse.ID) + } +} + +func hasMapKeyDeep(value interface{}, key string) bool { + switch v := value.(type) { + case map[string]interface{}: + for k, item := range v { + if k == key || hasMapKeyDeep(item, key) { + return true + } + } + case []interface{}: + for _, item := range v { + if hasMapKeyDeep(item, key) { + return true + } + } + } + return false } func TestGeminiClient_Chat_Retry(t *testing.T) { diff --git a/internal/ai/providers/openai.go b/internal/ai/providers/openai.go index 0c74ad6e7..0ee7755b8 100644 --- a/internal/ai/providers/openai.go +++ b/internal/ai/providers/openai.go @@ -224,7 +224,7 @@ type openaiRequest struct { MaxCompletionTokens int `json:"max_completion_tokens,omitempty"` // For o1/o3 models Temperature float64 `json:"temperature,omitempty"` Tools []openaiTool `json:"tools,omitempty"` - ToolChoice interface{} `json:"tool_choice,omitempty"` // "auto" or "none" + ToolChoice interface{} `json:"tool_choice,omitempty"` // "none", "required", or provider-specific choices } // openaiTool represents a function tool in OpenAI format @@ -353,13 +353,20 @@ func (c *OpenAIClient) requiresMaxCompletionTokens(model string) bool { // convertToolChoiceToOpenAI converts our ToolChoice to OpenAI's format. // Pulse omits automatic tool_choice so tool use stays model-owned, and only -// uses text-only safety brakes when tools are removed from the request. +// serializes native override modes when the caller explicitly requests them. // See: https://platform.openai.com/docs/api-reference/chat/create#chat-create-tool_choice func convertToolChoiceToOpenAI(tc *ToolChoice) interface{} { - if tc != nil && tc.Type == ToolChoiceNone { - return "none" + if tc == nil { + return nil + } + switch tc.Type { + case ToolChoiceNone: + return "none" + case ToolChoiceRequired: + return "required" + default: + return nil } - return nil } // convertMessagesToOpenAI converts provider-neutral messages into the diff --git a/internal/ai/providers/openai_test.go b/internal/ai/providers/openai_test.go index 1e6425c33..766b76bcb 100644 --- a/internal/ai/providers/openai_test.go +++ b/internal/ai/providers/openai_test.go @@ -732,6 +732,66 @@ func TestOpenAIClient_Chat_ToolChoiceNone_DropsTools(t *testing.T) { assert.Equal(t, "No tools", resp.Content) } +func TestOpenAIClient_Chat_ToolChoiceRequired_KeepsToolsAndRequiresCall(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var got map[string]interface{} + require.NoError(t, json.NewDecoder(r.Body).Decode(&got)) + + assert.Equal(t, "required", got["tool_choice"]) + tools, ok := got["tools"].([]interface{}) + require.True(t, ok, "tools field should be present when tool_choice is required") + require.Len(t, tools, 1) + tool, ok := tools[0].(map[string]interface{}) + require.True(t, ok, "tool should be an object") + assert.Equal(t, "function", tool["type"]) + fn, ok := tool["function"].(map[string]interface{}) + require.True(t, ok, "function definition should be an object") + assert.Equal(t, "get_time", fn["name"]) + + _ = json.NewEncoder(w).Encode(openaiResponse{ + ID: "chatcmpl-required-tools", + Model: "gpt-4", + Choices: []openaiChoice{ + { + Message: openaiRespMsg{ + Role: "assistant", + ToolCalls: []openaiToolCall{ + { + ID: "call_1", + Type: "function", + Function: openaiToolFunction{ + Name: "get_time", + Arguments: `{"tz":"UTC"}`, + }, + }, + }, + }, + FinishReason: "tool_calls", + }, + }, + }) + })) + defer server.Close() + + client := NewOpenAIClient("sk-test", "gpt-4", server.URL, 0) + resp, err := client.Chat(context.Background(), ChatRequest{ + Messages: []Message{{Role: "user", Content: "Hi"}}, + Tools: []Tool{ + { + Name: "get_time", + Description: "get time", + InputSchema: map[string]interface{}{"type": "object"}, + }, + }, + ToolChoice: &ToolChoice{Type: ToolChoiceRequired}, + }) + require.NoError(t, err) + require.Len(t, resp.ToolCalls, 1) + assert.Equal(t, "call_1", resp.ToolCalls[0].ID) + assert.Equal(t, "get_time", resp.ToolCalls[0].Name) + assert.Equal(t, "UTC", resp.ToolCalls[0].Input["tz"]) +} + func TestOpenAIClient_ChatStream_ToolChoiceNone_DropsTools(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { var got map[string]interface{} diff --git a/internal/ai/providers/provider.go b/internal/ai/providers/provider.go index 00ad6426f..32b84bf88 100644 --- a/internal/ai/providers/provider.go +++ b/internal/ai/providers/provider.go @@ -57,6 +57,9 @@ type ToolChoiceType string const ( // ToolChoiceNone prevents the model from using any tools ToolChoiceNone ToolChoiceType = "none" + // ToolChoiceRequired asks providers with a native forcing mechanism to + // require at least one tool call. + ToolChoiceRequired ToolChoiceType = "required" ) // ToolChoice controls how the model selects tools @@ -73,7 +76,7 @@ type ChatRequest struct { Temperature float64 `json:"temperature,omitempty"` System string `json:"system,omitempty"` // System prompt (Anthropic style) Tools []Tool `json:"tools,omitempty"` // Available tools - ToolChoice *ToolChoice `json:"tool_choice,omitempty"` // nil = model-owned automatic selection; none = text-only safety brake + ToolChoice *ToolChoice `json:"tool_choice,omitempty"` // nil = model-owned automatic selection; none = text-only safety brake; required = provider-native forced tool use where supported } func (r ChatRequest) NormalizeCollections() ChatRequest {