diff --git a/crates/code_assistant_core/src/tools/impls/browser/batch.rs b/crates/code_assistant_core/src/tools/impls/browser/batch.rs index e5fd8fff..e31e81c9 100644 --- a/crates/code_assistant_core/src/tools/impls/browser/batch.rs +++ b/crates/code_assistant_core/src/tools/impls/browser/batch.rs @@ -126,27 +126,68 @@ fn step_label(step: &BatchStep) -> String { } } +/// Parse a step's input the way a direct call of `tool` would: coerced +/// against that tool's schema first, so empty placeholders and lone scalars +/// behave the same inside a batch. +fn parse_step(step: &BatchStep, tool: T) -> Result +where + T::Input: DeserializeOwned, +{ + let mut input = step.input.clone(); + tools_core::coerce::coerce_to_schema(&mut input, &tool.spec().parameters_schema); + serde_json::from_value(input) + .map_err(|e| anyhow::anyhow!("invalid input for {}: {e}", step.name)) +} + async fn run_step(manager: &BrowserSessionManager, step: &BatchStep) -> Result { - fn parse(step: &BatchStep) -> Result { - serde_json::from_value(step.input.clone()) - .map_err(|e| anyhow::anyhow!("invalid input for {}: {e}", step.name)) - } let name = step.name.trim_start_matches("browser_"); Ok(match name { - "navigate" => page::navigate(manager, &parse(step)?).await, - "read_page" => page::read_page(manager, &parse(step)?).await, - "find" => page::find(manager, &parse(step)?).await, - "get_page_text" => page::get_page_text(manager, &parse(step)?).await, - "form_input" => page::form_input(manager, &parse(step)?).await, - "javascript" => page::javascript(manager, &parse(step)?).await, - "computer" => computer::computer(manager, &parse(step)?).await, - "read_console_messages" => devtools::read_console(manager, &parse(step)?).await, - "read_network_requests" => devtools::read_network(manager, &parse(step)?).await, - "resize_window" => tabs::resize_window(manager, &parse(step)?).await, - "tabs_context" => tabs::tabs_context(manager, &parse(step)?).await, - "tabs_create" => tabs::tabs_create(manager, &parse(step)?).await, - "tabs_select" => tabs::tabs_select(manager, &parse(step)?).await, - "tabs_close" => tabs::tabs_close(manager, &parse(step)?).await, + "navigate" => page::navigate(manager, &parse_step(step, page::BrowserNavigateTool)?).await, + "read_page" => { + page::read_page(manager, &parse_step(step, page::BrowserReadPageTool)?).await + } + "find" => page::find(manager, &parse_step(step, page::BrowserFindTool)?).await, + "get_page_text" => { + page::get_page_text(manager, &parse_step(step, page::BrowserGetPageTextTool)?).await + } + "form_input" => { + page::form_input(manager, &parse_step(step, page::BrowserFormInputTool)?).await + } + "javascript" => { + page::javascript(manager, &parse_step(step, page::BrowserJavascriptTool)?).await + } + "computer" => { + computer::computer(manager, &parse_step(step, computer::BrowserComputerTool)?).await + } + "read_console_messages" => { + devtools::read_console( + manager, + &parse_step(step, devtools::BrowserReadConsoleTool)?, + ) + .await + } + "read_network_requests" => { + devtools::read_network( + manager, + &parse_step(step, devtools::BrowserReadNetworkTool)?, + ) + .await + } + "resize_window" => { + tabs::resize_window(manager, &parse_step(step, tabs::BrowserResizeWindowTool)?).await + } + "tabs_context" => { + tabs::tabs_context(manager, &parse_step(step, tabs::BrowserTabsContextTool)?).await + } + "tabs_create" => { + tabs::tabs_create(manager, &parse_step(step, tabs::BrowserTabsCreateTool)?).await + } + "tabs_select" => { + tabs::tabs_select(manager, &parse_step(step, tabs::BrowserTabsSelectTool)?).await + } + "tabs_close" => { + tabs::tabs_close(manager, &parse_step(step, tabs::BrowserTabsCloseTool)?).await + } other => anyhow::bail!("browser_{other} cannot run in a batch"), }) } @@ -165,6 +206,21 @@ mod tests { } } + #[test] + fn a_step_drops_empty_placeholders_like_a_direct_call() -> Result<()> { + let input = parse_step( + &step( + "browser_computer", + json!({"action": "left_click", "ref": "ref_7", "coordinate": [], "tab_id": ""}), + ), + computer::BrowserComputerTool, + )?; + assert_eq!(input.r#ref.as_deref(), Some("ref_7")); + assert!(input.coordinate.is_none()); + assert!(input.target.tab_id.is_none()); + Ok(()) + } + #[tokio::test] async fn steps_run_in_order_and_stop_at_the_first_error() -> Result<()> { let page = data_url( diff --git a/crates/llm/src/ollama.rs b/crates/llm/src/ollama.rs index bd6420b7..1c77f7ae 100644 --- a/crates/llm/src/ollama.rs +++ b/crates/llm/src/ollama.rs @@ -188,12 +188,19 @@ impl OllamaClient { current_images.clear(); } - // ToolResult as separate "tool" message (text only) + // ToolResult as separate "tool" message; images (screenshots) + // ride along like on a user message messages.push(OllamaMessage { role: "tool".to_string(), content: content.text_content().to_string(), thinking: "".to_string(), - images: None, + images: content.has_images().then(|| { + content + .images() + .iter() + .map(|image| image.base64_data.clone()) + .collect() + }), tool_calls: None, }); } @@ -530,3 +537,37 @@ impl LLMProvider for OllamaClient { Ok(response) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn tool_result_images_travel_on_the_tool_message() { + let messages = OllamaClient::convert_user_message(&[ + ContentBlock::new_tool_result("call_0", "Clicked"), + ContentBlock::new_tool_result_with_images( + "call_1", + "Screenshot taken", + vec![crate::types::ToolResultImage { + media_type: "image/png".to_string(), + base64_data: "QUJD".to_string(), + }], + ), + ]); + + assert_eq!( + serde_json::to_value(&messages).unwrap(), + serde_json::json!([ + {"role": "tool", "content": "Clicked", "thinking": "", "tool_calls": null}, + { + "role": "tool", + "content": "Screenshot taken", + "thinking": "", + "images": ["QUJD"], + "tool_calls": null, + }, + ]) + ); + } +} diff --git a/crates/llm/src/openai_responses.rs b/crates/llm/src/openai_responses.rs index d06feb84..1c2a0851 100644 --- a/crates/llm/src/openai_responses.rs +++ b/crates/llm/src/openai_responses.rs @@ -310,6 +310,22 @@ impl ModelCapabilities { } } +/// Render a tool definition as a Responses API function tool. +/// +/// Without an explicit `strict`, the Responses API normalizes the schema into +/// strict mode, which makes every property required: the model then fills +/// optional parameters with placeholders (`""`, `0`, `[0, 0]`) that the tools +/// cannot tell from real values. Opting out lets it omit them. +pub(crate) fn function_tool(tool: ToolDefinition) -> serde_json::Value { + serde_json::json!({ + "type": "function", + "name": tool.name, + "description": tool.description, + "parameters": tool.parameters, + "strict": false, + }) +} + /// Place an explicit cache breakpoint on the last breakpoint-eligible content /// block within `items[..end]`. /// @@ -351,7 +367,7 @@ enum ResponseInputItem { }, FunctionCallOutput { call_id: String, - output: String, + output: FunctionCallOutput, }, Reasoning { id: String, @@ -378,6 +394,34 @@ impl PromptCacheBreakpoint { } } +/// `output` of a `function_call_output` item: plain text, or a list of +/// input content when the tool result carries images (screenshots), which a +/// string cannot hold. +#[derive(Debug, Serialize)] +#[serde(untagged)] +enum FunctionCallOutput { + Text(String), + Content(Vec), +} + +impl From<&ToolResultContent> for FunctionCallOutput { + fn from(content: &ToolResultContent) -> Self { + if !content.has_images() { + return Self::Text(content.text_content().to_string()); + } + let mut items = vec![ResponseContentItem::input_text( + content.text_content().to_string(), + )]; + items.extend(content.images().iter().map(|image| { + ResponseContentItem::input_image(format!( + "data:{};base64,{}", + image.media_type, image.base64_data + )) + })); + Self::Content(items) + } +} + /// Content item within messages #[derive(Debug, Serialize)] #[serde(tag = "type", rename_all = "snake_case")] @@ -1539,19 +1583,9 @@ impl LLMProvider for OpenAIResponsesClient { ); } - let tools = request.tools.map(|tools| { - tools - .into_iter() - .map(|tool| { - serde_json::json!({ - "type": "function", - "name": tool.name, - "description": tool.description, - "parameters": tool.parameters - }) - }) - .collect() - }); + let tools = request + .tools + .map(|tools| tools.into_iter().map(function_tool).collect()); // Configure for stateless mode with encrypted reasoning let store = false; @@ -1730,7 +1764,7 @@ fn convert_structured_message( pending.flush(result); result.push(ResponseInputItem::FunctionCallOutput { call_id: tool_use_id.clone(), - output: content.text_content().to_string(), + output: content.into(), }); } ContentBlock::RedactedThinking { @@ -1845,15 +1879,72 @@ mod tests { ])]; let converted = client.convert_messages_with_cache(messages, false); - assert_eq!(converted.len(), 1); + assert_eq!( + serde_json::to_value(&converted).unwrap(), + serde_json::json!([{ + "type": "function_call_output", + "call_id": "test_id", + "output": "Tool output", + }]) + ); + } - match &converted[0] { - ResponseInputItem::FunctionCallOutput { call_id, output } => { - assert_eq!(call_id, "test_id"); - assert_eq!(output, "Tool output"); - } - _ => panic!("Expected FunctionCallOutput"), - } + #[test] + fn test_function_tools_opt_out_of_strict_mode() { + let tool = function_tool(ToolDefinition { + name: "click".to_string(), + description: "Click".to_string(), + parameters: serde_json::json!({ + "type": "object", + "properties": {"ref": {"type": "string"}}, + }), + }); + assert_eq!( + tool, + serde_json::json!({ + "type": "function", + "name": "click", + "description": "Click", + "parameters": { + "type": "object", + "properties": {"ref": {"type": "string"}}, + }, + "strict": false, + }) + ); + } + + #[test] + fn test_tool_result_images_travel_in_function_call_output() { + let client = OpenAIResponsesClient::new( + "test_key".to_string(), + "gpt-5".to_string(), + "https://api.openai.com/v1".to_string(), + ); + + let messages = vec![Message::new_user_content(vec![ + ContentBlock::new_tool_result_with_images( + "call_1", + "Screenshot taken", + vec![crate::types::ToolResultImage { + media_type: "image/jpeg".to_string(), + base64_data: "QUJD".to_string(), + }], + ), + ])]; + + let converted = client.convert_messages_with_cache(messages, false); + assert_eq!( + serde_json::to_value(&converted).unwrap(), + serde_json::json!([{ + "type": "function_call_output", + "call_id": "call_1", + "output": [ + {"type": "input_text", "text": "Screenshot taken"}, + {"type": "input_image", "image_url": "data:image/jpeg;base64,QUJD"}, + ], + }]) + ); } #[test] diff --git a/crates/llm/src/openai_responses_ws.rs b/crates/llm/src/openai_responses_ws.rs index 2feadacc..befa5728 100644 --- a/crates/llm/src/openai_responses_ws.rs +++ b/crates/llm/src/openai_responses_ws.rs @@ -66,7 +66,9 @@ use tokio_tungstenite::{ use tracing::{debug, info, warn}; // Re-export types shared with the HTTP provider -use crate::openai_responses::{PromptCacheBreakpoint, Verbosity, model_supports_explicit_cache}; +use crate::openai_responses::{ + PromptCacheBreakpoint, Verbosity, function_tool, model_supports_explicit_cache, +}; // ============================================================================ // Request / Response types (WebSocket-specific envelope) @@ -149,7 +151,7 @@ enum WsInputItem { }, FunctionCallOutput { call_id: String, - output: String, + output: FunctionCallOutput, }, Reasoning { id: String, @@ -158,6 +160,34 @@ enum WsInputItem { }, } +/// `output` of a `function_call_output` item: plain text, or a list of +/// input content when the tool result carries images (screenshots), which a +/// string cannot hold. +#[derive(Debug, Clone, Serialize)] +#[serde(untagged)] +enum FunctionCallOutput { + Text(String), + Content(Vec), +} + +impl From<&ToolResultContent> for FunctionCallOutput { + fn from(content: &ToolResultContent) -> Self { + if !content.has_images() { + return Self::Text(content.text_content().to_string()); + } + let mut items = vec![WsContentItem::input_text( + content.text_content().to_string(), + )]; + items.extend(content.images().iter().map(|image| { + WsContentItem::input_image(format!( + "data:{};base64,{}", + image.media_type, image.base64_data + )) + })); + Self::Content(items) + } +} + /// Content item within messages. #[derive(Debug, Clone, Serialize)] #[serde(tag = "type", rename_all = "snake_case")] @@ -945,19 +975,9 @@ impl OpenAIResponsesWsClient { None }; - let tools = request.tools.map(|tools| { - tools - .into_iter() - .map(|tool| { - serde_json::json!({ - "type": "function", - "name": tool.name, - "description": tool.description, - "parameters": tool.parameters, - }) - }) - .collect() - }); + let tools = request + .tools + .map(|tools| tools.into_iter().map(function_tool).collect()); let reasoning = if capabilities.supports_reasoning { Some(ReasoningConfig { @@ -1548,12 +1568,14 @@ fn convert_structured_message( pending.push(WsContentItem::input_text(text.clone()), None, items); } } - ContentBlock::Image { data, .. } => { + ContentBlock::Image { + media_type, data, .. + } => { // `input_image` is user content; assistant images (none of // the Responses providers produce any) are not round-tripped. if !is_assistant { pending.push( - WsContentItem::input_image(format!("data:image/png;base64,{}", data)), + WsContentItem::input_image(format!("data:{media_type};base64,{data}")), None, items, ); @@ -1605,7 +1627,7 @@ fn convert_structured_message( pending.flush(items); items.push(WsInputItem::FunctionCallOutput { call_id: tool_use_id.clone(), - output: content.text_content().to_string(), + output: content.into(), }); } } @@ -2209,6 +2231,56 @@ mod tests { assert!(json.get("phase").is_none()); } + #[test] + fn test_ws_tool_result_images_travel_in_function_call_output() { + let blocks = vec![ + ContentBlock::new_tool_result("call_0", "Clicked"), + ContentBlock::new_tool_result_with_images( + "call_1", + "Screenshot taken", + vec![crate::types::ToolResultImage { + media_type: "image/jpeg".to_string(), + base64_data: "QUJD".to_string(), + }], + ), + ]; + let mut items = Vec::new(); + convert_structured_message(&MessageRole::User, &blocks, &mut items); + + assert_eq!( + serde_json::to_value(&items).unwrap(), + serde_json::json!([ + {"type": "function_call_output", "call_id": "call_0", "output": "Clicked"}, + { + "type": "function_call_output", + "call_id": "call_1", + "output": [ + {"type": "input_text", "text": "Screenshot taken"}, + {"type": "input_image", "image_url": "data:image/jpeg;base64,QUJD"}, + ], + }, + ]) + ); + } + + #[test] + fn test_ws_user_image_keeps_its_media_type() { + let blocks = vec![ContentBlock::Image { + media_type: "image/jpeg".to_string(), + data: "QUJD".to_string(), + start_time: None, + end_time: None, + }]; + let mut items = Vec::new(); + convert_structured_message(&MessageRole::User, &blocks, &mut items); + + let json = serde_json::to_value(&items).unwrap(); + assert_eq!( + json[0]["content"][0]["image_url"], + "data:image/jpeg;base64,QUJD" + ); + } + fn ws_items_with_breakpoint(items: &[WsInputItem]) -> Vec { items .iter() diff --git a/crates/llm/src/vertex.rs b/crates/llm/src/vertex.rs index 5ddd04a2..6f9f488c 100644 --- a/crates/llm/src/vertex.rs +++ b/crates/llm/src/vertex.rs @@ -258,6 +258,16 @@ struct VertexPartialArg { struct VertexFunctionResponse { name: String, response: serde_json::Value, + /// Media of a multimodal function response (screenshots), which the JSON + /// `response` cannot carry. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + parts: Vec, +} + +#[derive(Debug, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +struct VertexFunctionResponsePart { + inline_data: VertexInlineData, } /// Rate limit information extracted from response headers @@ -955,8 +965,17 @@ impl VertexClient { .unwrap_or(tool_use_id) .to_string() }), - // Wrap content in a proper JSON object (text only) response: json!({ "result": content.text_content() }), + parts: content + .images() + .iter() + .map(|image| VertexFunctionResponsePart { + inline_data: VertexInlineData { + mime_type: image.media_type.clone(), + data: image.base64_data.clone(), + }, + }) + .collect(), }), }), _ => None, @@ -1817,6 +1836,37 @@ mod tests { ); } + #[test] + fn tool_result_images_travel_as_function_response_parts() { + use std::collections::HashMap; + + let mut tool_names = HashMap::new(); + tool_names.insert("tool-1-1".to_string(), "browser_computer".to_string()); + let message = Message::new_user_content(vec![ + ContentBlock::new_tool_result("tool-1-0", "Clicked"), + ContentBlock::new_tool_result_with_images( + "tool-1-1", + "Screenshot taken", + vec![crate::types::ToolResultImage { + media_type: "image/jpeg".to_string(), + base64_data: "QUJD".to_string(), + }], + ), + ]); + let converted = VertexClient::convert_message(&message, &tool_names); + let value = serde_json::to_value(&converted).unwrap(); + + assert!(value["parts"][0]["functionResponse"].get("parts").is_none()); + assert_eq!( + value["parts"][1]["functionResponse"], + json!({ + "name": "browser_computer", + "response": {"result": "Screenshot taken"}, + "parts": [{"inlineData": {"mimeType": "image/jpeg", "data": "QUJD"}}], + }) + ); + } + #[test] fn empty_text_parts_are_dropped() { // A model turn often carries an empty text block next to a function diff --git a/crates/tools_core/src/coerce.rs b/crates/tools_core/src/coerce.rs index f244d824..ac4eed4a 100644 --- a/crates/tools_core/src/coerce.rs +++ b/crates/tools_core/src/coerce.rs @@ -8,9 +8,15 @@ //! schema and wraps such scalars in a one-element array, so the common mistake //! parses on the first try. //! -//! The coercion is deliberately conservative: it only ever *adds* an array -//! wrapper around a value the schema says should be an array, and it never -//! discards or reinterprets data. Because [`crate::dyn_tool::DynTool::invoke`] +//! Models also fill optional parameters with empty placeholders (`""`, `[]`, +//! `null`) instead of omitting them — a tab id `""` then fails as an unknown +//! tab, a coordinate `[]` as a malformed one. Such values are dropped from +//! properties the schema does not list as `required`, so the tool sees the +//! parameter as absent. +//! +//! Beyond that the coercion is deliberately conservative: it only ever *adds* +//! an array wrapper around a value the schema says should be an array, and it +//! never reinterprets data. Because [`crate::dyn_tool::DynTool::invoke`] //! re-serializes the parsed input back into `params`, a coerced value also //! becomes the canonical form recorded in the conversation history. @@ -52,8 +58,12 @@ pub fn coerce_to_schema(value: &mut Value, schema: &Value) { schema.get("properties").and_then(Value::as_object), value.as_object_mut(), ) { + let required = schema.get("required").and_then(Value::as_array); + let is_required = |key: &str| required.is_some_and(|r| r.iter().any(|k| k == key)); for (key, prop_schema) in props { - if let Some(field) = map.get_mut(key) { + if !is_required(key) && map.get(key).is_some_and(is_empty_placeholder) { + map.remove(key); + } else if let Some(field) = map.get_mut(key) { coerce_to_schema(field, prop_schema); } } @@ -63,6 +73,20 @@ pub fn coerce_to_schema(value: &mut Value, schema: &Value) { } } +/// Values a model emits for an optional parameter it means to leave out: +/// some providers (strict function calling) force every property to be +/// present, and models fill the unused ones with `""`, `[]` or `null`. +/// Numbers, booleans and objects are left alone, since `0`, `false` and `{}` +/// are as likely to be meant. +fn is_empty_placeholder(value: &Value) -> bool { + match value { + Value::Null => true, + Value::String(s) => s.is_empty(), + Value::Array(a) => a.is_empty(), + _ => false, + } +} + enum SchemaType { Array, Object, @@ -201,7 +225,8 @@ mod tests { fn does_not_wrap_null() { let schema = json!({ "type": "object", - "properties": {"paths": {"type": "array", "items": {"type": "string"}}} + "properties": {"paths": {"type": "array", "items": {"type": "string"}}}, + "required": ["paths"] }); let out = coerced(json!({"paths": null}), &schema); assert_eq!(out, json!({"paths": null})); @@ -289,6 +314,79 @@ mod tests { assert_eq!(out, json!({"value": "x"})); } + #[test] + fn drops_empty_placeholders_in_optional_fields() { + let schema = json!({ + "type": "object", + "properties": { + "action": {"type": "string"}, + "tab_id": {"type": "string"}, + "coordinate": {"type": "array", "items": {"type": "integer"}}, + "ref": {"type": "string"}, + }, + "required": ["action"] + }); + let out = coerced( + json!({"action": "left_click", "tab_id": "", "coordinate": [], "ref": null}), + &schema, + ); + assert_eq!(out, json!({"action": "left_click"})); + } + + #[test] + fn keeps_empty_values_in_required_fields() { + let schema = json!({ + "type": "object", + "properties": { + "old_text": {"type": "string"}, + "new_text": {"type": "string"}, + }, + "required": ["old_text", "new_text"] + }); + let out = coerced(json!({"old_text": "x", "new_text": ""}), &schema); + assert_eq!(out, json!({"old_text": "x", "new_text": ""})); + } + + #[test] + fn keeps_meaningful_falsy_values_in_optional_fields() { + let schema = json!({ + "type": "object", + "properties": { + "duration": {"type": "number"}, + "shift": {"type": "boolean"}, + "options": {"type": "object"}, + } + }); + let value = json!({"duration": 0, "shift": false, "options": {}}); + assert_eq!(coerced(value.clone(), &schema), value); + } + + #[test] + fn drops_empty_placeholders_in_nested_objects() { + let schema = json!({ + "type": "object", + "properties": { + "actions": { + "type": "array", + "items": { + "type": "object", + "properties": { + "name": {"type": "string"}, + "tab_id": {"type": "string"}, + }, + "required": ["name"] + } + } + }, + "required": ["actions"] + }); + let out = coerced( + json!({"actions": [{"name": "screenshot", "tab_id": ""}]}), + &schema, + ); + assert_eq!(out, json!({"actions": [{"name": "screenshot"}]})); + } + #[test] fn no_schema_is_noop() { let out = coerced(json!({"paths": "x"}), &json!(true));