From fb58c910c78fa3bd0628854c243f114d0bfcd577 Mon Sep 17 00:00:00 2001 From: Rami Chowdhury Date: Mon, 21 Jul 2025 13:15:13 -0400 Subject: [PATCH] feat: Work around Gemini API tool call quirks (#3328) Signed-off-by: Rami Chowdhury --- crates/goose/src/providers/formats/google.rs | 289 +++++++++++++++---- 1 file changed, 234 insertions(+), 55 deletions(-) diff --git a/crates/goose/src/providers/formats/google.rs b/crates/goose/src/providers/formats/google.rs index 2d0c4871..5a4f0b84 100644 --- a/crates/goose/src/providers/formats/google.rs +++ b/crates/goose/src/providers/formats/google.rs @@ -132,72 +132,89 @@ pub fn format_tools(tools: &[Tool]) -> Vec { let mut parameters = Map::new(); parameters.insert("name".to_string(), json!(tool.name)); parameters.insert("description".to_string(), json!(tool.description)); - let tool_input_schema = tool.input_schema.as_object().unwrap(); - let tool_input_schema_properties = tool_input_schema - .get("properties") - .unwrap_or(&json!({})) - .as_object() - .unwrap() - .clone(); - if !tool_input_schema_properties.is_empty() { - let accepted_tool_schema_attributes = vec![ - "type".to_string(), - "format".to_string(), - "description".to_string(), - "nullable".to_string(), - "enum".to_string(), - "maxItems".to_string(), - "minItems".to_string(), - "properties".to_string(), - "required".to_string(), - "items".to_string(), - ]; - parameters.insert( - "parameters".to_string(), - json!(process_map( - tool_input_schema, - &accepted_tool_schema_attributes, - None - )), - ); + if let Some(tool_input_schema) = tool.input_schema.as_object() { + // Only add the parameters key if the tool schema has non-empty properties. + if tool_input_schema + .get("properties") + .and_then(|v| v.as_object()) + .is_some_and(|p| !p.is_empty()) + { + parameters.insert( + "parameters".to_string(), + process_map(tool_input_schema, None), + ); + } } json!(parameters) }) .collect() } -/// Process a JSON map to filter out unsupported attributes -fn process_map( - map: &Map, - accepted_keys: &[String], - parent_key: Option<&str>, -) -> Value { - let mut filtered_map: Map = map +/// Get the accepted keys for a given parent key in the JSON schema. +fn get_accepted_keys(parent_key: Option<&str>) -> Vec<&str> { + match parent_key { + Some("properties") => vec![ + "anyOf", + "allOf", + "type", + // "format", // Google's APIs don't support this well + "description", + "nullable", + "enum", + "properties", + "required", + "items", + ], + Some("items") => vec!["type", "properties", "items", "required"], + // This is the top-level schema. + _ => vec!["type", "properties", "required", "anyOf", "allOf"], + } +} + +/// Process a JSON map to filter out unsupported attributes, mirroring the logic +/// from the official Google Gemini CLI. +/// See: https://github.com/google-gemini/gemini-cli/blob/8a6509ffeba271a8e7ccb83066a9a31a5d72a647/packages/core/src/tools/tool-registry.ts#L356 +fn process_map(map: &Map, parent_key: Option<&str>) -> Value { + let accepted_keys = get_accepted_keys(parent_key); + let filtered_map: Map = map .iter() .filter_map(|(key, value)| { - let should_remove = !accepted_keys.contains(key) && parent_key != Some("properties"); - if should_remove { - return None; + if !accepted_keys.contains(&key.as_str()) { + return None; // Skip if key is not accepted } - // Process nested maps recursively - let filtered_value = match value { - Value::Object(nested_map) => process_map( - &nested_map - .iter() - .map(|(k, v)| (k.clone(), v.clone())) - .collect(), - accepted_keys, - Some(key), - ), - _ => value.clone(), - }; - Some((key.clone(), filtered_value)) + match key.as_str() { + "properties" => { + // Process each property within the properties object + if let Some(nested_map) = value.as_object() { + let processed_properties: Map = nested_map + .iter() + .map(|(prop_key, prop_value)| { + if let Some(prop_obj) = prop_value.as_object() { + (prop_key.clone(), process_map(prop_obj, Some("properties"))) + } else { + (prop_key.clone(), prop_value.clone()) + } + }) + .collect(); + Some((key.clone(), Value::Object(processed_properties))) + } else { + None + } + } + "items" => { + // If it's a nested structure, recurse if it's an object. + value.as_object().map(|nested_map| { + (key.clone(), process_map(nested_map, Some(key.as_str()))) + }) + } + _ => { + // For other accepted keys, just clone the value. + Some((key.clone(), value.clone())) + } + } }) .collect(); - if parent_key != Some("properties") && !filtered_map.contains_key("type") { - filtered_map.insert("type".to_string(), Value::String("string".to_string())); - } Value::Object(filtered_map) } @@ -474,12 +491,90 @@ mod tests { "description": "B parameter", } }); + let params3 = json!({ + "body": { + "description": "Review comment text", + "type": "string" + }, + "comments": { + "description": "Line-specific comments array of objects to place comments on pull request changes. Requires path and body. For line comments use line or position. For multi-line comments use start_line and line with optional side parameters.", + "type": "array", + "items": { + "additionalProperties": false, + "properties": { + "body": { + "description": "comment body", + "type": "string" + }, + "line": { + "anyOf": [ + { "type": "number" }, + { "type": "null" } + ], + "description": "line number in the file to comment on. For multi-line comments, the end of the line range" + }, + "path": { + "description": "path to the file", + "type": "string" + }, + "position": { + "anyOf": [ + { "type": "number" }, + { "type": "null" } + ], + "description": "position of the comment in the diff" + }, + "side": { + "anyOf": [ + { "type": "string" }, + { "type": "null" } + ], + "description": "The side of the diff on which the line resides. For multi-line comments, this is the side for the end of the line range. (LEFT or RIGHT)" + }, + "start_line": { + "anyOf": [ + { "type": "number" }, + { "type": "null" } + ], + "description": "The first line of the range to which the comment refers. Required for multi-line comments." + }, + "start_side": { + "anyOf": [ + { "type": "string" }, + { "type": "null" } + ], + "description": "The side of the diff on which the start line resides for multi-line comments. (LEFT or RIGHT)" + } + }, + "required": ["path", "body", "position", "line", "side", "start_line", "start_side"], + "type": "object" + } + }, + "commitId": { + "description": "SHA of commit to review", + "type": "string" + }, + "event": { + "description": "Review action to perform", + "enum": ["APPROVE", "REQUEST_CHANGES", "COMMENT"], + "type": "string" + }, + "owner": { + "description": "Repository owner", + "type": "string" + }, + "pullNumber": { + "description": "Pull request number", + "type": "number" + } + }); let tools = vec![ set_up_tool("tool1", "description1", params1), set_up_tool("tool2", "description2", params2), + set_up_tool("tool3", "description3", params3), ]; let result = format_tools(&tools); - assert_eq!(result.len(), 2); + assert_eq!(result.len(), 3); assert_eq!(result[0]["name"], "tool1"); assert_eq!(result[0]["description"], "description1"); assert_eq!( @@ -498,6 +593,90 @@ mod tests { "description": "B parameter" })}) ); + + assert_eq!(result[2]["name"], "tool3"); + assert_eq!( + result[2]["parameters"]["properties"], + json!( + + { + "body": { + "description": "Review comment text", + "type": "string" + }, + "comments": { + "description": "Line-specific comments array of objects to place comments on pull request changes. Requires path and body. For line comments use line or position. For multi-line comments use start_line and line with optional side parameters.", + "type": "array", + "items": { + "properties": { + "body": { + "description": "comment body", + "type": "string" + }, + "line": { + "anyOf": [ + { "type": "number" }, + { "type": "null" } + ], + "description": "line number in the file to comment on. For multi-line comments, the end of the line range" + }, + "path": { + "description": "path to the file", + "type": "string" + }, + "position": { + "anyOf": [ + { "type": "number" }, + { "type": "null" } + ], + "description": "position of the comment in the diff" + }, + "side": { + "anyOf": [ + { "type": "string" }, + { "type": "null" } + ], + "description": "The side of the diff on which the line resides. For multi-line comments, this is the side for the end of the line range. (LEFT or RIGHT)" + }, + "start_line": { + "anyOf": [ + { "type": "number" }, + { "type": "null" } + ], + "description": "The first line of the range to which the comment refers. Required for multi-line comments." + }, + "start_side": { + "anyOf": [ + { "type": "string" }, + { "type": "null" } + ], + "description": "The side of the diff on which the start line resides for multi-line comments. (LEFT or RIGHT)" + } + }, + "required": ["path", "body", "position", "line", "side", "start_line", "start_side"], + "type": "object" + } + }, + "commitId": { + "description": "SHA of commit to review", + "type": "string" + }, + "event": { + "description": "Review action to perform", + "enum": ["APPROVE", "REQUEST_CHANGES", "COMMENT"], + "type": "string" + }, + "owner": { + "description": "Repository owner", + "type": "string" + }, + "pullNumber": { + "description": "Pull request number", + "type": "number" + } + } + ) + ); } #[test]