Fix tool result images and optional parameters for GPT, Gemini and Ollama

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<T: Tool>(step: &BatchStep, tool: T) -> Result<T::Input>
+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<BrowserOutput> {
-    fn parse<T: DeserializeOwned>(step: &BatchStep) -> Result<T> {
-        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(

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,
+                },
+            ])
+        );
+    }
+}

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<ResponseContentItem>),
+}
+
+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]