diff --git a/.gitignore b/.gitignore index 801357373f6..b8e61cc362e 100644 --- a/.gitignore +++ b/.gitignore @@ -4,6 +4,9 @@ .env.* !.env.example +# macOS +.DS_Store + # Claude Code worktrees and lock files .claude/worktrees/ .claude/scheduled_tasks.lock @@ -12,6 +15,9 @@ .sidecar/ .todos/ +# Cursor IDE +.cursor/ + target/ # Benchmark results (local runs, not committed) diff --git a/src/agent/dispatcher.rs b/src/agent/dispatcher.rs index 6118ec7d364..6006a02e8a6 100644 --- a/src/agent/dispatcher.rs +++ b/src/agent/dispatcher.rs @@ -863,6 +863,39 @@ impl Agent { } } +/// Normalize tool parameters from LLM output. +/// +/// Some OpenAI-compatible providers (e.g. Qwen via DashScope) return empty +/// strings `""` for optional parameters instead of `null` when the tool schema +/// uses strict-mode nullable types (`["string","null"]`). An empty string is +/// semantically equivalent to absent for optional tool parameters, but +/// `validate_tool_params` rejects empty strings and tools like `time` pass +/// them directly to parsers (e.g. `parse_timezone("")` → error). +/// +/// This function converts all empty string values to `null` in the parameter +/// object before validation and execution. Non-string values are left unchanged. +fn normalize_tool_params(params: &serde_json::Value) -> serde_json::Value { + match params { + serde_json::Value::Object(obj) => { + let cleaned = obj + .iter() + .map(|(k, v)| { + let normalized = match v { + serde_json::Value::String(s) if s.is_empty() => serde_json::Value::Null, + other => normalize_tool_params(other), + }; + (k.clone(), normalized) + }) + .collect(); + serde_json::Value::Object(cleaned) + } + serde_json::Value::Array(arr) => { + serde_json::Value::Array(arr.iter().map(normalize_tool_params).collect()) + } + other => other.clone(), + } +} + /// Execute a chat tool without requiring `&Agent`. /// /// This standalone function enables parallel invocation from spawned JoinSet @@ -875,6 +908,11 @@ pub(super) async fn execute_chat_tool_standalone( params: &serde_json::Value, job_ctx: &crate::context::JobContext, ) -> Result { + // Normalize params: convert empty strings to null so optional parameters + // left blank by strict-mode LLMs (e.g. Qwen) are treated as absent. + let params = normalize_tool_params(params); + let params = ¶ms; + let tool = tools .get(tool_name) .await @@ -926,7 +964,7 @@ pub(super) async fn execute_chat_tool_standalone( ); } Ok(Err(e)) => { - tracing::debug!( + tracing::warn!( tool = %tool_name, elapsed_ms = elapsed.as_millis() as u64, error = %e, @@ -2261,6 +2299,71 @@ mod tests { ); } + #[test] + fn test_normalize_tool_params_empty_strings_become_null() { + // Regression: strict-mode LLMs (e.g. Qwen via DashScope) return empty + // strings for optional nullable parameters instead of null. + // normalize_tool_params must convert these to null so optional_param + // helpers like `params.get("timezone").and_then(|v| v.as_str())` return + // None and tools treat the parameter as absent. + let params = serde_json::json!({ + "operation": "now", + "timezone": "", + "format": "", + "input": "2026-01-01" + }); + let normalized = super::normalize_tool_params(¶ms); + assert_eq!( + normalized["operation"].as_str(), + Some("now"), + "non-empty string must be preserved" + ); + assert!( + normalized["timezone"].is_null(), + "empty string must become null" + ); + assert!( + normalized["format"].is_null(), + "empty string must become null" + ); + assert_eq!( + normalized["input"].as_str(), + Some("2026-01-01"), + "non-empty string must be preserved" + ); + } + + #[test] + fn test_normalize_tool_params_nested_objects() { + // Nested objects (e.g. tool params with nested config) should also have + // their empty string values converted to null. + let params = serde_json::json!({ + "config": { "key": "", "other": "value" }, + "items": ["hello", "world"] + }); + let normalized = super::normalize_tool_params(¶ms); + assert!(normalized["config"]["key"].is_null(), "nested empty string must become null"); + assert_eq!(normalized["config"]["other"].as_str(), Some("value")); + // Array string elements are preserved as-is (arrays are passed through) + let items = normalized["items"].as_array().unwrap(); + assert_eq!(items[0].as_str(), Some("hello")); + assert_eq!(items[1].as_str(), Some("world")); + } + + #[test] + fn test_normalize_tool_params_non_string_values_unchanged() { + let params = serde_json::json!({ + "count": 42, + "flag": false, + "ratio": 3.14, + "nothing": null + }); + let normalized = super::normalize_tool_params(¶ms); + assert_eq!(normalized["count"].as_i64(), Some(42)); + assert_eq!(normalized["flag"].as_bool(), Some(false)); + assert!(normalized["nothing"].is_null()); + } + #[test] fn test_image_sentinel_empty_data_url_should_be_skipped() { // Regression: unwrap_or_default() on missing "data" field produces an empty diff --git a/src/channels/repl.rs b/src/channels/repl.rs index 33adc23f62d..4381bc38dc5 100644 --- a/src/channels/repl.rs +++ b/src/channels/repl.rs @@ -481,11 +481,17 @@ impl Channel for ReplChannel { StatusUpdate::ToolStarted { name } => { eprintln!(" \x1b[33m\u{25CB} {name}\x1b[0m"); } - StatusUpdate::ToolCompleted { name, success, .. } => { + StatusUpdate::ToolCompleted { + name, + success, + error, + .. + } => { if success { eprintln!(" \x1b[32m\u{25CF} {name}\x1b[0m"); } else { - eprintln!(" \x1b[31m\u{2717} {name} (failed)\x1b[0m"); + let detail = error.as_deref().unwrap_or("unknown error"); + eprintln!(" \x1b[31m\u{2717} {name}: {detail}\x1b[0m"); } } StatusUpdate::ToolResult { name: _, preview } => { diff --git a/src/safety/validator.rs b/src/safety/validator.rs index c56789ea9eb..de511ba3a7c 100644 --- a/src/safety/validator.rs +++ b/src/safety/validator.rs @@ -201,6 +201,13 @@ impl Validator { ) { match value { serde_json::Value::String(s) => { + // Empty strings are treated as absent optional parameters — + // some strict-mode LLMs (e.g. Qwen) emit "" instead of null + // for nullable fields. Skipping them here avoids spurious + // Empty errors on valid tool calls. + if s.is_empty() { + return; + } let string_result = validator.validate(s); *result = std::mem::take(result).merge(string_result); } @@ -312,4 +319,34 @@ mod tests { assert!(result.is_valid); // Still valid, just a warning assert!(!result.warnings.is_empty()); } + + #[test] + fn test_validate_tool_params_empty_string_is_valid() { + // Regression: strict-mode LLMs (e.g. Qwen) emit "" for optional nullable + // parameters. validate_tool_params must not reject these as Empty errors. + let validator = Validator::new(); + let params = serde_json::json!({ + "operation": "now", + "timezone": "", + "format": "" + }); + let result = validator.validate_tool_params(¶ms); + assert!( + result.is_valid, + "Empty optional params should not fail validation: {:?}", + result.errors + ); + } + + #[test] + fn test_validate_tool_params_non_empty_strings_still_validated() { + // Ensure actual string content continues to be validated. + let validator = Validator::new().forbid_pattern("injection_test"); + let params = serde_json::json!({ + "operation": "now", + "timezone": "injection_test" + }); + let result = validator.validate_tool_params(¶ms); + assert!(!result.is_valid, "Forbidden patterns should still be caught"); + } } diff --git a/src/tools/builtin/time.rs b/src/tools/builtin/time.rs index bafbd4d74af..c78d42b4bf3 100644 --- a/src/tools/builtin/time.rs +++ b/src/tools/builtin/time.rs @@ -534,4 +534,44 @@ mod tests { assert_eq!(dt.to_rfc3339(), "2026-03-08T07:30:00+00:00"); } + + #[tokio::test] + async fn test_now_with_empty_string_optional_params() { + // Regression: strict-mode LLMs (e.g. Qwen) emit "" for optional nullable + // parameters. The time tool must handle these gracefully — they should be + // treated as absent, not cause parse errors. + // normalize_tool_params (in dispatcher) converts "" to null before reaching + // here, but we also test that null params work correctly end-to-end. + let tool = TimeTool; + let ctx = JobContext::with_user("test", "chat", "test"); + + // null optional params (what normalize_tool_params produces from "") + let output = tool + .execute( + serde_json::json!({ + "operation": "now", + "timezone": null, + "format": null + }), + &ctx, + ) + .await + .expect("null optional params must not fail"); + + assert!(output.result.get("iso").is_some(), "should have iso field"); + } + + #[tokio::test] + async fn test_now_missing_optional_params_succeeds() { + // Baseline: when optional params are simply absent, tool succeeds. + let tool = TimeTool; + let ctx = JobContext::with_user("test", "chat", "test"); + + let output = tool + .execute(serde_json::json!({"operation": "now"}), &ctx) + .await + .expect("missing optional params must not fail"); + + assert!(output.result.get("iso").is_some()); + } }