Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
.env.*
!.env.example

# macOS
.DS_Store

# Claude Code worktrees and lock files
.claude/worktrees/
.claude/scheduled_tasks.lock
Expand All @@ -12,6 +15,9 @@
.sidecar/
.todos/

# Cursor IDE
.cursor/

target/

# Benchmark results (local runs, not committed)
Expand Down
105 changes: 104 additions & 1 deletion src/agent/dispatcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -875,6 +908,11 @@ pub(super) async fn execute_chat_tool_standalone(
params: &serde_json::Value,
job_ctx: &crate::context::JobContext,
) -> Result<String, Error> {
// 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 = &params;

let tool = tools
.get(tool_name)
.await
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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(&params);
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(&params);
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(&params);
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
Expand Down
10 changes: 8 additions & 2 deletions src/channels/repl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 } => {
Expand Down
37 changes: 37 additions & 0 deletions src/safety/validator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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(&params);
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(&params);
assert!(!result.is_valid, "Forbidden patterns should still be caught");
}
}
40 changes: 40 additions & 0 deletions src/tools/builtin/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}