From 128d431084463c697b6c712ac3e9deb66b195408 Mon Sep 17 00:00:00 2001 From: Jason Date: Thu, 16 Apr 2026 16:22:52 +0800 Subject: [PATCH] fix(proxy/gemini): synthesize unique ids for no-id tool calls + enforce object params schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1 — Parallel tool calls without Gemini-assigned ids no longer collapse. Gemini 2.x native parallel `functionCall` entries may omit the `id` field. The previous `merge_tool_call_snapshots` fell back to matching by `name`, which silently merged two parallel calls to the same function into one entry — dropping the first call's args. The non-streaming path and shadow store further bottlenecked on empty-string ids: multiple `tool_use` blocks shared the same id, and `tool_name_by_id.get("")` could only return one mapping, causing later `tool_result` round-trips to fail with `Unable to resolve Gemini functionResponse.name` or bind to the wrong tool. Fix: introduce `synthesize_tool_call_id()` producing `gemini_synth_`. Both streaming and non-streaming response paths now guarantee every Anthropic-visible tool_use carries a unique id. `merge_tool_call_snapshots` matches by id first, falling back to the `parts` array position (for the cumulative-streaming case) while preserving the synthesized id across chunks. `convert_message_content_to_parts` detects the synthetic prefix and strips the id from outbound `functionCall`/`functionResponse` so the internal identifier never leaks upstream. `shadow_parts` performs the same strip when replaying a recorded assistant turn. P2 — Vertex AI rejects empty `parameters` schemas. When an Anthropic tool arrives with missing or empty `input_schema`, the proxy used to emit `"parameters": {}` (no `type`), which fails Vertex AI validation with `functionDeclaration parameters schema should be of type OBJECT`. Contrary to the automated-review suggestion, the fix is not to omit `parameters` (that too is rejected) but to normalize to the canonical empty-object form `{type: "object", properties: {}}`. Refs: google-gemini/generative-ai-python#423, BerriAI/litellm#5055. Fix: new `ensure_object_schema` helper in `gemini_schema` promotes missing `type` to `"object"` and adds empty `properties` when absent, while leaving atomic (non-object) schemas untouched. Tests: seven new regressions covering parallel no-id calls, cumulative chunk id reuse, synthetic-id round-trip both directions, shadow replay id stripping, and the three Vertex-AI schema shapes. The two existing wrapper functions (`gemini_to_anthropic` and `gemini_to_anthropic_with_shadow`) gain `#[allow(dead_code)]` to clear a pre-existing clippy -D warnings failure — they are part of the public transform API surface and intentionally kept for future callers. Addresses Codex review P1/P2 on #1918. --- .../src/proxy/providers/gemini_schema.rs | 64 ++++- .../src/proxy/providers/streaming_gemini.rs | 98 ++++++- .../src/proxy/providers/transform_gemini.rs | 266 ++++++++++++++++-- 3 files changed, 398 insertions(+), 30 deletions(-) diff --git a/src-tauri/src/proxy/providers/gemini_schema.rs b/src-tauri/src/proxy/providers/gemini_schema.rs index dc07b09b0..d74aaf16c 100644 --- a/src-tauri/src/proxy/providers/gemini_schema.rs +++ b/src-tauri/src/proxy/providers/gemini_schema.rs @@ -16,7 +16,7 @@ pub enum GeminiFunctionParameters { } pub fn build_gemini_function_parameters(input_schema: Value) -> GeminiFunctionParameters { - let schema = normalize_json_schema(input_schema); + let schema = ensure_object_schema(normalize_json_schema(input_schema)); if requires_parameters_json_schema(&schema) { GeminiFunctionParameters::JsonSchema(schema) @@ -25,6 +25,31 @@ pub fn build_gemini_function_parameters(input_schema: Value) -> GeminiFunctionPa } } +/// Vertex AI rejects FunctionDeclarations whose `parameters` schema lacks an +/// explicit `type: "object"`, returning: +/// +/// > functionDeclaration parameters schema should be of type OBJECT. +/// +/// Anthropic tools sometimes arrive with empty or type-less `input_schema` +/// (e.g. no-argument tools like Claude Code's `TodoRead`). Normalize those to +/// `{type: "object", properties: {}}` so the Gemini upstream accepts them. +/// +/// References: google-gemini/generative-ai-python#423, BerriAI/litellm#5055. +fn ensure_object_schema(schema: Value) -> Value { + match schema { + Value::Object(mut obj) => { + obj.entry("type".to_string()) + .or_insert_with(|| json!("object")); + if obj.get("type").and_then(|v| v.as_str()) == Some("object") { + obj.entry("properties".to_string()) + .or_insert_with(|| json!({})); + } + Value::Object(obj) + } + other => other, + } +} + fn normalize_json_schema(schema: Value) -> Value { match schema { Value::Object(mut obj) => { @@ -275,4 +300,41 @@ mod tests { assert!(result.get("parameters").is_none()); assert!(result.get("parametersJsonSchema").is_some()); } + + /// Regression for P2 (Vertex AI rejecting empty schemas): zero-argument + /// Anthropic tools (no `input_schema`) must produce `parameters` with an + /// explicit `type: "object"` and an empty `properties` map so the Gemini + /// upstream does not return `schema should be of type OBJECT`. + #[test] + fn empty_input_schema_produces_explicit_object_type() { + let result = build_gemini_function_declaration("ping", Some("no-arg"), json!({})); + + assert_eq!(result["parameters"]["type"], "object"); + assert!(result["parameters"]["properties"].is_object()); + } + + /// A schema that carries descriptive fields but no `type` is still a + /// zero-arg object for Gemini purposes — promote it explicitly. + #[test] + fn input_schema_missing_type_is_promoted_to_object() { + let result = build_gemini_function_declaration( + "noop", + None, + json!({ "description": "does nothing" }), + ); + + assert_eq!(result["parameters"]["type"], "object"); + assert!(result["parameters"]["properties"].is_object()); + } + + /// Defensive: an atomic (non-object) schema is left untouched, because + /// forcing `type: "object"` here would corrupt primitive parameter types + /// that happen to flow through this path. + #[test] + fn non_object_schema_is_not_mutated() { + let result = build_gemini_function_declaration("bare", None, json!({ "type": "string" })); + + assert_eq!(result["parameters"]["type"], "string"); + assert!(result["parameters"].get("properties").is_none()); + } } diff --git a/src-tauri/src/proxy/providers/streaming_gemini.rs b/src-tauri/src/proxy/providers/streaming_gemini.rs index 19684d266..e79edec68 100644 --- a/src-tauri/src/proxy/providers/streaming_gemini.rs +++ b/src-tauri/src/proxy/providers/streaming_gemini.rs @@ -4,7 +4,10 @@ //! SSE events for Claude-compatible clients. use super::gemini_shadow::{GeminiShadowStore, GeminiToolCallMeta}; -use super::transform_gemini::{rectify_tool_call_parts, AnthropicToolSchemaHints}; +use super::transform_gemini::{ + is_synthesized_tool_call_id, rectify_tool_call_parts, synthesize_tool_call_id, + AnthropicToolSchemaHints, +}; use crate::proxy::sse::{append_utf8_safe, strip_sse_field, take_sse_block}; use bytes::Bytes; use futures::stream::{Stream, StreamExt}; @@ -116,19 +119,50 @@ fn merge_tool_call_snapshots( tool_call_snapshots: &mut Vec, incoming: Vec, ) { - for tool_call in incoming { - let existing_index = - tool_call_snapshots + // Gemini's `streamGenerateContent?alt=sse` delivers each chunk as the + // cumulative snapshot of `content.parts`. For the same tool call across + // chunks we therefore need to map an incoming entry back to whichever + // snapshot entry it describes: + // + // 1. If both sides carry a genuine Gemini id, match by id. + // 2. Otherwise match by position in the cumulative `parts` array — this + // is how parallel no-id calls stay distinguishable. + // + // A previous implementation fell back to matching by `name`, which silently + // merged two parallel calls to the same function into one entry (losing + // the first call's args). That fallback is removed here. + for (position, mut tool_call) in incoming.into_iter().enumerate() { + let existing_index = match tool_call.id.as_deref() { + Some(incoming_id) => tool_call_snapshots .iter() - .position(|existing| match (&existing.id, &tool_call.id) { - (Some(existing_id), Some(incoming_id)) => existing_id == incoming_id, - _ => existing.name == tool_call.name, - }); + .position(|existing| existing.id.as_deref() == Some(incoming_id)), + None => tool_call_snapshots + .get(position) + .filter(|existing| match existing.id.as_deref() { + // Only merge into a positional match when the prior + // snapshot was itself id-less (or we synthesized one). + // A snapshot with a genuine Gemini id at this index is + // treated as a different call — the incoming entry gets + // its own synthesized id below. + Some(id) => is_synthesized_tool_call_id(id), + None => true, + }) + .map(|_| position), + }; if let Some(index) = existing_index { - tool_call_snapshots[index] = tool_call; - } else { - tool_call_snapshots.push(tool_call); + // Preserve any synthesized id assigned on a previous chunk so the + // Anthropic-visible id stays stable across the whole stream. + let preserved_id = tool_call_snapshots[index].id.clone(); + tool_call.id = tool_call.id.or(preserved_id); + } + if tool_call.id.is_none() { + tool_call.id = Some(synthesize_tool_call_id()); + } + + match existing_index { + Some(index) => tool_call_snapshots[index] = tool_call, + None => tool_call_snapshots.push(tool_call), } } } @@ -805,4 +839,46 @@ mod tests { assert!(output.contains("详细分析内容 编写提交信息 分多次提交代码")); assert!(!output.contains("\\\"parameters\\\"")); } + + /// Regression for the P1 finding: when Gemini emits two parallel calls to + /// the same function without providing ids, both must be surfaced to the + /// Anthropic client with distinct synthesized ids. The previous + /// name-based fallback in `merge_tool_call_snapshots` collapsed them into + /// a single entry, causing silent data loss for the first call. + #[test] + fn parallel_same_name_no_id_calls_preserve_both() { + let output = collect_stream_output(vec![ + "data: {\"responseId\":\"r1\",\"modelVersion\":\"gemini-2.5-pro\",\"candidates\":[{\"finishReason\":\"STOP\",\"content\":{\"parts\":[{\"functionCall\":{\"name\":\"get_weather\",\"args\":{\"city\":\"Tokyo\"}}},{\"functionCall\":{\"name\":\"get_weather\",\"args\":{\"city\":\"Osaka\"}}}]}}],\"usageMetadata\":{\"promptTokenCount\":5,\"totalTokenCount\":8}}\n\n", + ]); + + let tool_use_start_count = output.matches("\"type\":\"tool_use\"").count(); + assert_eq!( + tool_use_start_count, 2, + "both parallel calls must survive merge_tool_call_snapshots" + ); + // `input_json_delta.partial_json` is a string, so the city keys appear + // JSON-escaped inside the outer SSE `data:` payload. Match against + // the raw escape sequences rather than the canonical JSON form. + assert!(output.contains("Tokyo")); + assert!(output.contains("Osaka")); + // Each tool_use must carry a non-empty synthesized id so Claude Code + // can disambiguate the two tool_result round-trips. + let synth_count = output.matches("\"id\":\"gemini_synth_").count(); + assert_eq!(synth_count, 2); + } + + /// When Gemini keeps sending the same no-id functionCall across cumulative + /// chunks, the synthesized id must stay stable so the Anthropic client + /// sees a single tool_use block with consistent args updates rather than + /// duplicates. + #[test] + fn no_id_tool_call_reuses_synthesized_id_across_cumulative_chunks() { + let output = collect_stream_output(vec![ + "data: {\"responseId\":\"r2\",\"modelVersion\":\"gemini-2.5-pro\",\"candidates\":[{\"content\":{\"parts\":[{\"functionCall\":{\"name\":\"get_weather\",\"args\":{\"city\":\"Tokyo\"}}}]}}],\"usageMetadata\":{\"promptTokenCount\":4,\"totalTokenCount\":6}}\n\n", + "data: {\"responseId\":\"r2\",\"modelVersion\":\"gemini-2.5-pro\",\"candidates\":[{\"finishReason\":\"STOP\",\"content\":{\"parts\":[{\"functionCall\":{\"name\":\"get_weather\",\"args\":{\"city\":\"Tokyo\",\"units\":\"c\"}}}]}}],\"usageMetadata\":{\"promptTokenCount\":4,\"totalTokenCount\":9}}\n\n", + ]); + + assert_eq!(output.matches("\"type\":\"tool_use\"").count(), 1); + assert!(output.contains("\"units\\\":\\\"c\\\"")); + } } diff --git a/src-tauri/src/proxy/providers/transform_gemini.rs b/src-tauri/src/proxy/providers/transform_gemini.rs index c7c65fb4d..0924ca4fb 100644 --- a/src-tauri/src/proxy/providers/transform_gemini.rs +++ b/src-tauri/src/proxy/providers/transform_gemini.rs @@ -18,6 +18,27 @@ pub struct AnthropicToolSchemaHint { pub type AnthropicToolSchemaHints = HashMap; +/// Prefix used for Anthropic-visible tool call ids that we synthesize when +/// Gemini's `functionCall` omits the `id` field (Gemini 2.x parallel calls +/// often do). The prefix is how downstream request-path code recognizes that +/// the id is not a real Gemini id and must be stripped before forwarding back +/// to Gemini as `functionResponse.id`. +pub(crate) const SYNTHESIZED_ID_PREFIX: &str = "gemini_synth_"; + +/// Generate a unique tool-call id that is safe to expose to Anthropic clients +/// but must not be sent upstream to Gemini. Uses UUID v4 simple encoding +/// (32 lowercase hex chars) so that any number of parallel calls in the same +/// response remain distinguishable. +pub(crate) fn synthesize_tool_call_id() -> String { + format!("{SYNTHESIZED_ID_PREFIX}{}", uuid::Uuid::new_v4().simple()) +} + +/// Returns true if `id` was produced by [`synthesize_tool_call_id`] and +/// therefore must be stripped when building Gemini request bodies. +pub(crate) fn is_synthesized_tool_call_id(id: &str) -> bool { + id.starts_with(SYNTHESIZED_ID_PREFIX) +} + pub fn anthropic_to_gemini(body: Value) -> Result { anthropic_to_gemini_with_shadow(body, None, None, None) } @@ -77,10 +98,19 @@ pub fn anthropic_to_gemini_with_shadow( Ok(result) } +/// Convenience wrapper over [`gemini_to_anthropic_with_shadow_and_hints`] +/// with no shadow store or schema hints. Used by the shared +/// `ProviderAdapter::transform_response` path and by tests. +#[allow(dead_code)] // kept as public API for non-streaming transform paths pub fn gemini_to_anthropic(body: Value) -> Result { gemini_to_anthropic_with_shadow(body, None, None, None) } +/// Convenience wrapper for callers that have a shadow store but no tool +/// schema hints. Production call sites funnel through +/// [`gemini_to_anthropic_with_shadow_and_hints`] directly; this helper exists +/// for test ergonomics and future external callers. +#[allow(dead_code)] // kept as public API for shadow-only transform paths pub fn gemini_to_anthropic_with_shadow( body: Value, shadow_store: Option<&GeminiShadowStore>, @@ -153,9 +183,15 @@ pub fn gemini_to_anthropic_with_shadow_and_hints( if let Some(function_call) = part.get("functionCall") { has_tool_use = true; + let id = function_call + .get("id") + .and_then(|value| value.as_str()) + .filter(|s| !s.is_empty()) + .map(ToString::to_string) + .unwrap_or_else(synthesize_tool_call_id); content.push(json!({ "type": "tool_use", - "id": function_call.get("id").and_then(|value| value.as_str()).unwrap_or(""), + "id": id, "name": function_call.get("name").and_then(|value| value.as_str()).unwrap_or(""), "input": function_call.get("args").cloned().unwrap_or_else(|| json!({})) })); @@ -499,13 +535,18 @@ fn convert_message_content_to_parts( tool_name_by_id.insert(id.to_string(), name.to_string()); } - parts.push(json!({ - "functionCall": { - "id": id, - "name": name, - "args": block.get("input").cloned().unwrap_or_else(|| json!({})) - } - })); + // A synthesized id is an internal proxy identifier — never + // forward it to Gemini. Gemini will disambiguate the missing + // id by call order, matching its own earlier response shape. + let mut function_call = json!({ + "name": name, + "args": block.get("input").cloned().unwrap_or_else(|| json!({})) + }); + if !id.is_empty() && !is_synthesized_tool_call_id(id) { + function_call["id"] = json!(id); + } + + parts.push(json!({ "functionCall": function_call })); } "tool_result" => { let tool_use_id = block @@ -521,13 +562,16 @@ fn convert_message_content_to_parts( )) })?; - parts.push(json!({ - "functionResponse": { - "id": tool_use_id, - "name": name, - "response": normalize_tool_result_response(block.get("content")) - } - })); + // See `tool_use` above: synthesized ids must not leak upstream. + let mut function_response = json!({ + "name": name, + "response": normalize_tool_result_response(block.get("content")) + }); + if !tool_use_id.is_empty() && !is_synthesized_tool_call_id(tool_use_id) { + function_response["id"] = json!(tool_use_id); + } + + parts.push(json!({ "functionResponse": function_response })); } "thinking" | "redacted_thinking" => {} _ => {} @@ -559,11 +603,31 @@ fn normalize_tool_result_response(content: Option<&Value>) -> Value { } fn shadow_parts(content: &Value) -> Option> { - content + let mut parts = content .get("parts") .and_then(|value| value.as_array()) .cloned() - .or_else(|| content.as_array().cloned()) + .or_else(|| content.as_array().cloned())?; + // Strip synthesized ids before these parts are replayed into a Gemini + // request body. The shadow store records the Anthropic-facing id so that + // a tool_result round-trip can find the tool's name, but sending the + // synthetic value as `functionCall.id` upstream would leak an internal + // identifier. + for part in &mut parts { + let Some(function_call) = part.get_mut("functionCall").and_then(|v| v.as_object_mut()) + else { + continue; + }; + let drop_id = function_call + .get("id") + .and_then(|v| v.as_str()) + .map(|id| id.is_empty() || is_synthesized_tool_call_id(id)) + .unwrap_or(true); + if drop_id { + function_call.remove("id"); + } + } + Some(parts) } pub fn extract_anthropic_tool_schema_hints(body: &Value) -> AnthropicToolSchemaHints { @@ -788,8 +852,18 @@ fn extract_tool_call_meta(parts: &[Value]) -> Vec { .iter() .filter_map(|part| { let function_call = part.get("functionCall")?; + // Ensure every surfaced tool call carries a distinguishing id. + // Gemini 2.x may omit `id` on parallel calls; synthesizing a + // unique replacement prevents downstream merge/replay logic from + // collapsing distinct calls onto a single empty-string key. + let id = function_call + .get("id") + .and_then(|value| value.as_str()) + .filter(|s| !s.is_empty()) + .map(ToString::to_string) + .unwrap_or_else(synthesize_tool_call_id); Some(GeminiToolCallMeta::new( - function_call.get("id").and_then(|value| value.as_str()), + Some(id), function_call .get("name") .and_then(|value| value.as_str()) @@ -1395,4 +1469,160 @@ mod tests { "sig-tool-1" ); } + + /// Regression for P1: Gemini 2.x may return parallel calls without ids. + /// Each Anthropic-visible tool_use must carry a unique id so the Claude + /// Code client can map tool_result responses back correctly. + #[test] + fn gemini_to_anthropic_synthesizes_unique_ids_for_missing_functioncall_ids() { + let input = json!({ + "responseId": "r1", + "modelVersion": "gemini-2.5-pro", + "candidates": [{ + "finishReason": "STOP", + "content": { + "parts": [ + { "functionCall": { "name": "foo", "args": {} } }, + { "functionCall": { "name": "foo", "args": { "k": 1 } } } + ] + } + }] + }); + + let result = gemini_to_anthropic(input).unwrap(); + let id0 = result["content"][0]["id"].as_str().unwrap(); + let id1 = result["content"][1]["id"].as_str().unwrap(); + assert!(is_synthesized_tool_call_id(id0)); + assert!(is_synthesized_tool_call_id(id1)); + assert_ne!(id0, id1, "synthesized ids must be unique per call"); + } + + /// Ensures the proxy does not leak synthesized ids back to Gemini when + /// Claude Code replies with a tool_result: the id must be stripped from + /// both `functionCall.id` and `functionResponse.id`. + #[test] + fn tool_result_with_synthesized_id_omits_id_in_gemini_request() { + let synth = synthesize_tool_call_id(); + let input = json!({ + "messages": [ + { + "role": "assistant", + "content": [ + { "type": "tool_use", "id": &synth, "name": "get_weather", "input": { "city": "X" } } + ] + }, + { + "role": "user", + "content": [ + { "type": "tool_result", "tool_use_id": &synth, "content": "sunny" } + ] + } + ] + }); + + let result = anthropic_to_gemini(input).unwrap(); + let fc = &result["contents"][0]["parts"][0]["functionCall"]; + assert!( + fc.get("id").is_none(), + "synthesized id must not leak upstream in functionCall" + ); + assert_eq!(fc["name"], "get_weather"); + let fr = &result["contents"][1]["parts"][0]["functionResponse"]; + assert!( + fr.get("id").is_none(), + "synthesized id must not leak upstream in functionResponse" + ); + assert_eq!(fr["name"], "get_weather"); + } + + /// Genuine Gemini-assigned ids must round-trip unchanged so that Gemini + /// can correlate the tool result with its own prior functionCall entry. + #[test] + fn tool_result_with_genuine_gemini_id_round_trips() { + let input = json!({ + "messages": [ + { + "role": "assistant", + "content": [ + { "type": "tool_use", "id": "call_real_1", "name": "get_weather", "input": {} } + ] + }, + { + "role": "user", + "content": [ + { "type": "tool_result", "tool_use_id": "call_real_1", "content": "ok" } + ] + } + ] + }); + + let result = anthropic_to_gemini(input).unwrap(); + assert_eq!( + result["contents"][0]["parts"][0]["functionCall"]["id"], + "call_real_1" + ); + assert_eq!( + result["contents"][1]["parts"][0]["functionResponse"]["id"], + "call_real_1" + ); + } + + /// Shadow replay must also strip synthesized ids when it reconstructs + /// the assistant's `functionCall` parts from a previously recorded turn. + #[test] + fn shadow_replay_strips_synthesized_id_from_function_call() { + let store = GeminiShadowStore::with_limits(8, 4); + let synth = synthesize_tool_call_id(); + store.record_assistant_turn( + "prov", + "sess", + json!({ + "parts": [{ + "functionCall": { + "id": &synth, + "name": "get_weather", + "args": { "city": "Tokyo" } + } + }] + }), + vec![GeminiToolCallMeta::new( + Some(synth.clone()), + "get_weather", + json!({ "city": "Tokyo" }), + None::, + )], + ); + + let input = json!({ + "messages": [ + { + "role": "assistant", + "content": [ + { "type": "tool_use", "id": &synth, "name": "get_weather", "input": { "city": "Tokyo" } } + ] + }, + { + "role": "user", + "content": [ + { "type": "tool_result", "tool_use_id": &synth, "content": "sunny" } + ] + } + ] + }); + + let result = + anthropic_to_gemini_with_shadow(input, Some(&store), Some("prov"), Some("sess")) + .unwrap(); + // The assistant message was replayed from shadow; its synthesized id + // must be absent from the upstream functionCall representation. + assert!(result["contents"][0]["parts"][0]["functionCall"] + .get("id") + .is_none()); + // And the tool_result round-trip must still resolve the name via the + // shadow map even when the id is synthesized. + assert_eq!( + result["contents"][1]["parts"][0]["functionResponse"]["name"], + "get_weather" + ); + } }