From 54a49a8562547bf4c5e3bb269eb74784117a9eb0 Mon Sep 17 00:00:00 2001 From: Cameron Date: Wed, 18 Mar 2026 23:58:01 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20agentic=20loop=20robustness=20=E2=80=94?= =?UTF-8?q?=20tool=20arg=20sanitisation,=20geocoding,=20better=20errors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Sanitise tool call arguments before re-sending in conversation history: non-object values (bool, string, null) that some models produce are normalised to {} to prevent Ollama 500s - Map 'error parsing tool call' Ollama 500 to HTTP 400 with a descriptive message listing compatible models (llama3.1, llama3.2, qwen2.5, mistral-nemo) - Add reverse_geocode tool backed by existing Nominatim helper; description hints model can chain it after get_location_history results - Make get_sms_messages contact parameter optional (was required, forcing the model to guess); executor now passes None to fall back to all-contacts search - Log tool result outcomes at warn level for errors/empty results, info for successes; log SMS API errors with full detail; log full request body on Ollama 500 - Strengthen system prompt to require 3-4 tool calls before final answer - Try fallback server when checking model capabilities (primary-only check caused 500 for models only on fallback) Co-Authored-By: Claude Sonnet 4.6 --- src/ai/handlers.rs | 4 + src/ai/insight_generator.rs | 147 +++++++++++++++++++++++++++--------- src/ai/ollama.rs | 9 +++ 3 files changed, 125 insertions(+), 35 deletions(-) diff --git a/src/ai/handlers.rs b/src/ai/handlers.rs index 5bf3178..f91268b 100644 --- a/src/ai/handlers.rs +++ b/src/ai/handlers.rs @@ -304,6 +304,10 @@ pub async fn generate_agentic_insight_handler( HttpResponse::BadRequest().json(serde_json::json!({ "error": format!("Failed to generate agentic insight: {}", error_msg) })) + } else if error_msg.contains("error parsing tool call") { + HttpResponse::BadRequest().json(serde_json::json!({ + "error": "Model is not compatible with Ollama's tool calling protocol. Try a model known to support native tool calling (e.g. llama3.1, llama3.2, qwen2.5, mistral-nemo)." + })) } else { HttpResponse::InternalServerError().json(serde_json::json!({ "error": format!("Failed to generate agentic insight: {}", error_msg) diff --git a/src/ai/insight_generator.rs b/src/ai/insight_generator.rs index 67a7358..cac03a4 100644 --- a/src/ai/insight_generator.rs +++ b/src/ai/insight_generator.rs @@ -1347,15 +1347,26 @@ Return ONLY the summary, nothing else."#, image_base64: &Option, cx: &opentelemetry::Context, ) -> String { - match tool_name { + let result = match tool_name { "search_rag" => self.tool_search_rag(arguments, cx).await, "get_sms_messages" => self.tool_get_sms_messages(arguments, cx).await, "get_calendar_events" => self.tool_get_calendar_events(arguments, cx).await, "get_location_history" => self.tool_get_location_history(arguments, cx).await, "get_file_tags" => self.tool_get_file_tags(arguments, cx).await, "describe_photo" => self.tool_describe_photo(ollama, image_base64).await, + "reverse_geocode" => self.tool_reverse_geocode(arguments).await, unknown => format!("Unknown tool: {}", unknown), + }; + if result.starts_with("Error") || result.starts_with("No ") { + log::warn!("Tool '{}' result: {}", tool_name, result); + } else { + log::info!( + "Tool '{}' result: {} chars", + tool_name, + result.len() + ); } + result } /// Tool: search_rag — semantic search over daily summaries @@ -1408,10 +1419,10 @@ Return ONLY the summary, nothing else."#, Some(d) => d, None => return "Error: missing required parameter 'date'".to_string(), }; - let contact = match args.get("contact").and_then(|v| v.as_str()) { - Some(c) => c.to_string(), - None => return "Error: missing required parameter 'contact'".to_string(), - }; + let contact = args + .get("contact") + .and_then(|v| v.as_str()) + .map(|s| s.to_string()); let days_radius = args .get("days_radius") .and_then(|v| v.as_i64()) @@ -1424,28 +1435,15 @@ Return ONLY the summary, nothing else."#, let timestamp = date.and_hms_opt(12, 0, 0).unwrap().and_utc().timestamp(); log::info!( - "tool_get_sms_messages: date={}, contact='{}', days_radius={}", + "tool_get_sms_messages: date={}, contact={:?}, days_radius={}", date, contact, days_radius ); - // Use the SMS client's existing fetch mechanism - // Build start/end from days_radius - let start_ts = timestamp - (days_radius * 86400); - let end_ts = timestamp + (days_radius * 86400); - - let center_dt = DateTime::from_timestamp(timestamp, 0); - let start_dt = DateTime::from_timestamp(start_ts, 0); - let end_dt = DateTime::from_timestamp(end_ts, 0); - - if center_dt.is_none() || start_dt.is_none() || end_dt.is_none() { - return "Error: invalid timestamp range".to_string(); - } - match self .sms_client - .fetch_messages_for_contact(Some(&contact), timestamp) + .fetch_messages_for_contact(contact.as_deref(), timestamp) .await { Ok(messages) if !messages.is_empty() => { @@ -1467,7 +1465,10 @@ Return ONLY the summary, nothing else."#, ) } Ok(_) => "No messages found.".to_string(), - Err(e) => format!("Error fetching SMS messages: {}", e), + Err(e) => { + log::warn!("tool_get_sms_messages failed: {}", e); + format!("Error fetching SMS messages: {}", e) + } } } @@ -1659,6 +1660,25 @@ Return ONLY the summary, nothing else."#, } } + /// Tool: reverse_geocode — convert GPS coordinates to a human-readable place name + async fn tool_reverse_geocode(&self, args: &serde_json::Value) -> String { + let lat = match args.get("latitude").and_then(|v| v.as_f64()) { + Some(v) => v, + None => return "Error: missing required parameter 'latitude'".to_string(), + }; + let lon = match args.get("longitude").and_then(|v| v.as_f64()) { + Some(v) => v, + None => return "Error: missing required parameter 'longitude'".to_string(), + }; + + log::info!("tool_reverse_geocode: lat={}, lon={}", lat, lon); + + match self.reverse_geocode(lat, lon).await { + Some(place) => place, + None => "Could not resolve coordinates to a place name.".to_string(), + } + } + // ── Agentic insight generation ────────────────────────────────────── /// Build the list of tool definitions for the agentic loop @@ -1688,10 +1708,10 @@ Return ONLY the summary, nothing else."#, ), Tool::function( "get_sms_messages", - "Fetch SMS/text messages near a specific date for a contact. Returns the actual message conversation.", + "Fetch SMS/text messages near a specific date. Returns the actual message conversation. Omit contact to search across all conversations.", serde_json::json!({ "type": "object", - "required": ["date", "contact"], + "required": ["date"], "properties": { "date": { "type": "string", @@ -1699,7 +1719,7 @@ Return ONLY the summary, nothing else."#, }, "contact": { "type": "string", - "description": "The contact name to fetch messages for" + "description": "Optional contact name to filter messages. If omitted, searches all conversations." }, "days_radius": { "type": "integer", @@ -1760,6 +1780,25 @@ Return ONLY the summary, nothing else."#, ), ]; + tools.push(Tool::function( + "reverse_geocode", + "Convert GPS latitude/longitude coordinates to a human-readable place name (city, state). Use this when GPS coordinates are available in the photo metadata, or to resolve coordinates returned by get_location_history.", + serde_json::json!({ + "type": "object", + "required": ["latitude", "longitude"], + "properties": { + "latitude": { + "type": "number", + "description": "GPS latitude in decimal degrees" + }, + "longitude": { + "type": "number", + "description": "GPS longitude in decimal degrees" + } + } + }), + )); + if has_vision { tools.push(Tool::function( "describe_photo", @@ -1840,19 +1879,34 @@ Return ONLY the summary, nothing else."#, } } - // 2b. Check tool calling capability - let capabilities = OllamaClient::check_model_capabilities( + // 2b. Check tool calling capability — try primary, fall back to fallback URL + let model_name_for_caps = &ollama_client.primary_model; + let capabilities = match OllamaClient::check_model_capabilities( &ollama_client.primary_url, - &ollama_client.primary_model, + model_name_for_caps, ) .await - .map_err(|e| { - anyhow::anyhow!( - "Failed to check model capabilities for '{}': {}", - ollama_client.primary_model, - e - ) - })?; + { + Ok(caps) => caps, + Err(_) => { + // Model may only be on the fallback server + let fallback_url = ollama_client.fallback_url.as_deref().ok_or_else(|| { + anyhow::anyhow!( + "Failed to check model capabilities for '{}': model not found on primary server and no fallback configured", + model_name_for_caps + ) + })?; + OllamaClient::check_model_capabilities(fallback_url, model_name_for_caps) + .await + .map_err(|e| { + anyhow::anyhow!( + "Failed to check model capabilities for '{}': {}", + model_name_for_caps, + e + ) + })? + } + }; if !capabilities.has_tool_calling { return Err(anyhow::anyhow!( @@ -1939,7 +1993,13 @@ Return ONLY the summary, nothing else."#, }; // 7. Build system message - let base_system = "You are a personal photo memory assistant. You have access to tools to gather context about when and where this photo was taken. Use them to build a rich, personal insight. Call tools as needed, then write a final summary and title."; + let base_system = "You are a personal photo memory assistant helping to reconstruct a memory from a photo.\n\n\ + IMPORTANT INSTRUCTIONS:\n\ + 1. You MUST call multiple tools to gather context BEFORE writing any final insight. Do not produce a final answer after only one or two tool calls.\n\ + 2. Always call ALL of the following tools that are relevant: search_rag (search conversation summaries), get_sms_messages (fetch nearby messages), get_calendar_events (check what was happening that day), get_location_history (find where this was taken), get_file_tags (retrieve tags).\n\ + 3. Only produce your final insight AFTER you have gathered context from at least 3-4 tools.\n\ + 4. If a tool returns no results, that is useful information — continue calling the remaining tools anyway.\n\ + 5. Your final insight must be written in first person as Cameron, in a journal/memoir style."; let system_content = if let Some(ref custom) = custom_system_prompt { format!("{}\n\n{}", custom, base_system) } else { @@ -2012,6 +2072,23 @@ Return ONLY the summary, nothing else."#, .chat_with_tools(messages.clone(), tools.clone()) .await?; + // Sanitize tool call arguments before pushing back into history. + // Some models occasionally return non-object arguments (bool, string, null) + // which Ollama rejects when they are re-sent in a subsequent request. + let mut response = response; + if let Some(ref mut tool_calls) = response.tool_calls { + for tc in tool_calls.iter_mut() { + if !tc.function.arguments.is_object() { + log::warn!( + "Tool '{}' returned non-object arguments ({:?}), normalising to {{}}", + tc.function.name, + tc.function.arguments + ); + tc.function.arguments = serde_json::Value::Object(Default::default()); + } + } + } + messages.push(response.clone()); if let Some(ref tool_calls) = response.tool_calls diff --git a/src/ai/ollama.rs b/src/ai/ollama.rs index 38a0c6c..857427f 100644 --- a/src/ai/ollama.rs +++ b/src/ai/ollama.rs @@ -591,6 +591,10 @@ Analyze the image and use specific details from both the visual content and the options, }; + let request_json = serde_json::to_string(&request_body) + .unwrap_or_else(|e| format!("", e)); + log::debug!("chat_with_tools request body: {}", request_json); + let response = self .client .post(&url) @@ -602,6 +606,11 @@ Analyze the image and use specific details from both the visual content and the if !response.status().is_success() { let status = response.status(); let body = response.text().await.unwrap_or_default(); + log::error!( + "chat_with_tools request body that caused {}: {}", + status, + request_json + ); anyhow::bail!( "Ollama chat request failed with status {}: {}", status,