From d13cc0804ae5d81328e19352a3628becd5d732a9 Mon Sep 17 00:00:00 2001 From: Justin Truong Date: Sun, 19 Jul 2026 21:45:13 +0700 Subject: [PATCH] fix(providers): carry tool name on tool results for Gemini Gemini drops tool_call_id and pairs functionCall/functionResponse by function name, so its OpenAI-compat shim requires a non-empty FunctionResponse.name. The name was only recoverable through a reverse id->name lookup over assistant tool_calls still present in the request window, which fails after pruning, truncation or tool_call collapse and produces HTTP 400 "Name cannot be empty" on any follow-up iteration. Add Message.ToolName, set it at every tool-result creation site and preserve it through context pruning, then prefer it when serializing. Fall back to the existing index for history persisted before the field, so old sessions keep working without a reset. When neither source resolves a name, drop the unlabelled tool result instead of emitting an empty one: an empty name is a guaranteed 400 and a synthetic name would match no prior functionCall. Gated behind the existing Gemini detection so other OpenAI-compat hosts, which pair by tool_call_id, are unaffected. Field is JSON-optional, so session history needs no migration and older binaries ignore it on rollback. --- internal/agent/loop_history_sanitize.go | 1 + .../agent/loop_pipeline_tool_callbacks.go | 1 + internal/agent/loop_tools.go | 1 + internal/agent/memoryflush.go | 1 + internal/agent/pruning.go | 5 + internal/pipeline/tool_stage.go | 2 + internal/providers/openai_request.go | 26 ++++- .../openai_tool_message_name_test.go | 106 ++++++++++++++++-- internal/providers/types.go | 10 ++ internal/tools/subagent_exec.go | 1 + 10 files changed, 136 insertions(+), 18 deletions(-) diff --git a/internal/agent/loop_history_sanitize.go b/internal/agent/loop_history_sanitize.go index a55b2bf9..7a009dcf 100644 --- a/internal/agent/loop_history_sanitize.go +++ b/internal/agent/loop_history_sanitize.go @@ -152,6 +152,7 @@ func sanitizeHistory(msgs []providers.Message) ([]providers.Message, int) { Role: "tool", Content: "[Tool result missing — session was compacted]", ToolCallID: tc.ID, + ToolName: tc.Name, }) dropped++ } diff --git a/internal/agent/loop_pipeline_tool_callbacks.go b/internal/agent/loop_pipeline_tool_callbacks.go index a79bd26e..155f08a3 100644 --- a/internal/agent/loop_pipeline_tool_callbacks.go +++ b/internal/agent/loop_pipeline_tool_callbacks.go @@ -168,6 +168,7 @@ func (l *Loop) makeExecuteToolRaw(req *RunRequest) func(ctx context.Context, tc Role: "tool", Content: result.ForLLM, ToolCallID: tc.ID, + ToolName: tc.Name, IsError: result.IsError, } return msg, &toolRawResult{result: result, duration: dur, start: start, spanID: spanID, toolName: registryName, rawName: tc.Name}, nil diff --git a/internal/agent/loop_tools.go b/internal/agent/loop_tools.go index 7639051d..37c3c4ef 100644 --- a/internal/agent/loop_tools.go +++ b/internal/agent/loop_tools.go @@ -158,6 +158,7 @@ func (l *Loop) processToolResult( Role: "tool", Content: result.ForLLM, ToolCallID: tc.ID, + ToolName: tc.Name, IsError: result.IsError, } diff --git a/internal/agent/memoryflush.go b/internal/agent/memoryflush.go index 7db493a0..2d95b5d8 100644 --- a/internal/agent/memoryflush.go +++ b/internal/agent/memoryflush.go @@ -232,6 +232,7 @@ func (l *Loop) runMemoryFlush(ctx context.Context, sessionKey string, settings * Role: "tool", Content: result.ForLLM, ToolCallID: tc.ID, + ToolName: tc.Name, }) } } diff --git a/internal/agent/pruning.go b/internal/agent/pruning.go index 6cec5e05..07e04976 100644 --- a/internal/agent/pruning.go +++ b/internal/agent/pruning.go @@ -299,6 +299,10 @@ func pruneContextMessages(msgs []providers.Message, contextWindowTokens int, cfg Role: msg.Role, Content: trimmed, ToolCallID: msg.ToolCallID, + // Preserve ToolName: Gemini needs it to build FunctionResponse.name, + // and pruned results are exactly the ones whose assistant tool_call + // may already be out of the request window. + ToolName: msg.ToolName, } totalTokens += est.estimateTokens(trimmed) - msgTokens if stats != nil { @@ -352,6 +356,7 @@ func pruneContextMessages(msgs []providers.Message, contextWindowTokens int, cfg Role: msg.Role, Content: settings.hardClearPlaceholder, ToolCallID: msg.ToolCallID, + ToolName: msg.ToolName, } afterTokens := est.estimateTokens(settings.hardClearPlaceholder) totalTokens += afterTokens - beforeTokens diff --git a/internal/pipeline/tool_stage.go b/internal/pipeline/tool_stage.go index f6f1f0b1..b9b0c5ef 100644 --- a/internal/pipeline/tool_stage.go +++ b/internal/pipeline/tool_stage.go @@ -167,6 +167,7 @@ func (s *ToolStage) preflightToolCall(ctx context.Context, state *RunState, tc p Role: "tool", Content: reason, ToolCallID: tc.ID, + ToolName: tc.Name, IsError: true, } } @@ -191,6 +192,7 @@ func (s *ToolStage) preflightToolCall(ctx context.Context, state *RunState, tc p Role: "tool", Content: "Hook blocked: pre_tool_use", ToolCallID: tc.ID, + ToolName: tc.Name, } } if r.UpdatedToolInput != nil { diff --git a/internal/providers/openai_request.go b/internal/providers/openai_request.go index 61de8d87..8cfa6172 100644 --- a/internal/providers/openai_request.go +++ b/internal/providers/openai_request.go @@ -146,13 +146,27 @@ func (p *OpenAIProvider) buildRequestBody(model string, req ChatRequest, stream // (FunctionResponse.name). Most other OpenAI-compat hosts (Together, Groq, // vLLM) either ignore or reject unknown fields — gate to Gemini only to // avoid silent 400s on stricter proxies. - if supportsThoughtSignature { - if name := toolNameByID[m.ToolCallID]; name != "" { - msg["name"] = name - } else if m.Role == "tool" { - slog.Warn("openai: tool msg without matching tool_call", - "provider", p.name, "tool_call_id", m.ToolCallID) + if supportsThoughtSignature && m.Role == "tool" { + // Prefer the name carried on the message: it survives pruning, + // truncation and tool_call collapse. Fall back to the reverse index + // for history persisted before Message.ToolName existed. + name := m.ToolName + if name == "" { + name = toolNameByID[m.ToolCallID] } + if name == "" { + // Gemini pairs functionCall↔functionResponse by name and has no + // tool_call_id to fall back on, so an empty name is a hard 400 + // ("Name cannot be empty"). A synthetic name is not an option + // either — it would match no prior functionCall. Dropping the + // unlabelled result is the only way to keep the request valid. + // Reachable only for legacy history whose assistant tool_call is + // already out of the window, so nothing dangles by removing it. + slog.Warn("openai: dropping tool result with unresolvable tool name", + "provider", p.name, "tool_call_id", m.ToolCallID) + continue + } + msg["name"] = name } } diff --git a/internal/providers/openai_tool_message_name_test.go b/internal/providers/openai_tool_message_name_test.go index 7a0c7c22..5e88f0f7 100644 --- a/internal/providers/openai_tool_message_name_test.go +++ b/internal/providers/openai_tool_message_name_test.go @@ -82,11 +82,65 @@ func TestBuildRequestBody_ToolMessageNameLookupUsesRawID(t *testing.T) { } } -// TestBuildRequestBody_ToolMessageWithoutMatchingCallOmitsName verifies that a -// stray tool message (no preceding tool_call with matching ID) does NOT emit an -// empty name field — better to drop the field than send "" which Gemini rejects -// just the same. Logged via slog.Warn for observability. -func TestBuildRequestBody_ToolMessageWithoutMatchingCallOmitsName(t *testing.T) { +// TestBuildRequestBody_ToolNameSurvivesMissingAssistant is the core regression +// test for the Gemini 400. Message.ToolName must be used directly, so a tool +// result still serializes a valid FunctionResponse.name even when the assistant +// tool_call it originated from is no longer in the request window (pruned, +// truncated, or collapsed). The reverse id→name index cannot help here. +func TestBuildRequestBody_ToolNameSurvivesMissingAssistant(t *testing.T) { + p := NewOpenAIProvider("test-gemini", "key", + "https://generativelanguage.googleapis.com/v1beta/openai", "gemini-3-flash-preview") + + req := ChatRequest{ + Messages: []Message{ + {Role: "user", Content: "hi"}, + // No assistant tool_call in the window — only the carried name can save this. + {Role: "tool", ToolCallID: "call_gone", ToolName: "mcp__srv__lookup", Content: "ok"}, + }, + } + + body := p.buildRequestBody("gemini-3-flash-preview", req, false) + msgs := body["messages"].([]map[string]any) + if len(msgs) != 2 { + t.Fatalf("tool msg must be kept when ToolName is present, got %d msgs", len(msgs)) + } + if got := msgs[1]["name"]; got != "mcp__srv__lookup" { + t.Fatalf("msg[1] name = %v, want mcp__srv__lookup", got) + } +} + +// TestBuildRequestBody_ToolNameBeatsStaleIndex verifies precedence: the name +// carried on the message wins over the reverse index, so a rewritten/aliased +// tool_call ID cannot resurface a mismatched name. +func TestBuildRequestBody_ToolNameBeatsStaleIndex(t *testing.T) { + p := NewOpenAIProvider("test-gemini", "key", + "https://generativelanguage.googleapis.com/v1beta/openai", "gemini-3-flash-preview") + + req := ChatRequest{ + Messages: []Message{ + {Role: "user", Content: "hi"}, + {Role: "assistant", ToolCalls: []ToolCall{ + { + ID: "call_1", Name: "indexed_name", + Metadata: map[string]string{"thought_signature": "sig"}, + }, + }}, + {Role: "tool", ToolCallID: "call_1", ToolName: "carried_name", Content: "ok"}, + }, + } + + body := p.buildRequestBody("gemini-3-flash-preview", req, false) + msgs := body["messages"].([]map[string]any) + if got := msgs[2]["name"]; got != "carried_name" { + t.Fatalf("carried ToolName must win; got %v", got) + } +} + +// TestBuildRequestBody_UnresolvableToolNameDropped verifies that a tool result +// with neither a carried name nor an index match is DROPPED for Gemini. Emitting +// it with an empty (or absent) name is what produced HTTP 400 "Name cannot be +// empty" — Gemini has no tool_call_id fallback to pair on. +func TestBuildRequestBody_UnresolvableToolNameDropped(t *testing.T) { p := NewOpenAIProvider("test-gemini", "key", "https://generativelanguage.googleapis.com/v1beta/openai", "gemini-3-flash-preview") @@ -99,13 +153,41 @@ func TestBuildRequestBody_ToolMessageWithoutMatchingCallOmitsName(t *testing.T) body := p.buildRequestBody("gemini-3-flash-preview", req, false) msgs := body["messages"].([]map[string]any) - if len(msgs) < 2 { - t.Fatalf("expected at least 2 msgs after collapse, got %d", len(msgs)) - } - last := msgs[len(msgs)-1] - if last["role"] == "tool" { - if _, present := last["name"]; present { - t.Fatalf("orphan tool msg should omit name field, got %v", last["name"]) + + for i, m := range msgs { + if m["role"] == "tool" { + t.Fatalf("msg[%d]: unresolvable tool result must be dropped, got %v", i, m) + } + if name, present := m["name"]; present && name == "" { + t.Fatalf("msg[%d]: empty name must never reach Gemini", i) } } } + +// TestBuildRequestBody_UnresolvableToolNameKeptForNonGemini is the regression +// guard for every other OpenAI-compat host sharing this code path (OpenAI, Qwen, +// DeepSeek, Together, ...). They pair by tool_call_id and never need `name`, so +// the drop must not apply — silently losing tool results there would be a bug. +func TestBuildRequestBody_UnresolvableToolNameKeptForNonGemini(t *testing.T) { + p := NewOpenAIProvider("together", "key", + "https://api.together.xyz/v1", "meta-llama/Llama-3-70b") + + req := ChatRequest{ + Messages: []Message{ + {Role: "user", Content: "hi"}, + {Role: "tool", ToolCallID: "orphan_id", Content: "stale"}, + }, + } + + body := p.buildRequestBody("meta-llama/Llama-3-70b", req, false) + msgs := body["messages"].([]map[string]any) + if len(msgs) != 2 { + t.Fatalf("non-Gemini must keep tool msg, got %d msgs", len(msgs)) + } + if msgs[1]["role"] != "tool" { + t.Fatalf("msg[1] role = %v, want tool", msgs[1]["role"]) + } + if _, present := msgs[1]["name"]; present { + t.Fatalf("non-Gemini must not emit name, got %v", msgs[1]["name"]) + } +} diff --git a/internal/providers/types.go b/internal/providers/types.go index 672d4bc3..12e161dc 100644 --- a/internal/providers/types.go +++ b/internal/providers/types.go @@ -152,6 +152,16 @@ type Message struct { ToolCallID string `json:"tool_call_id,omitempty"` // for role="tool" responses IsError bool `json:"is_error,omitempty"` // for role="tool" responses + // ToolName is the originating tool's name for role="tool" messages. + // Google Gemini drops tool_call_id entirely and pairs functionCall↔functionResponse + // by function name, so its OpenAI-compat shim requires a non-empty + // FunctionResponse.name. Carrying the name on the message itself keeps it + // recoverable after pruning, truncation or tool_call collapse — all of which + // can remove the assistant tool_call that a reverse id→name lookup depends on. + // Must match the assistant ToolCall.Name that was sent on the wire (raw name, + // not the resolved registry name). Empty on history persisted before this field. + ToolName string `json:"tool_name,omitempty"` + // Phase is a Codex-specific field (gpt-5.3-codex) indicating message purpose. // Values: "commentary" (intermediate), "final_answer" (closeout), or "" (unset). // Must be persisted and passed back in subsequent requests for Codex performance. diff --git a/internal/tools/subagent_exec.go b/internal/tools/subagent_exec.go index a15efe5b..f5c6554f 100644 --- a/internal/tools/subagent_exec.go +++ b/internal/tools/subagent_exec.go @@ -369,6 +369,7 @@ func (sm *SubagentManager) executeTask(ctx context.Context, task *SubagentTask) Role: "tool", Content: result.ForLLM, ToolCallID: tc.ID, + ToolName: tc.Name, }) } }