From 984f7d43474e364b0b7bbe2b894e2778246a543f Mon Sep 17 00:00:00 2001 From: Argenis Date: Tue, 24 Mar 2026 00:44:39 -0400 Subject: [PATCH] fix(cron): treat empty allowed_tools as None and sanitize tool names (#4482) Fixes #4442. Empty allowed_tools arrays caused all tools to be filtered out, making cron jobs fail. Also adds tool name validation in the OpenRouter provider to skip tools with names that violate the OpenAI naming regex. --- src/cron/store.rs | 8 ++- src/providers/openrouter.rs | 105 +++++++++++++++++++++++++++++++----- src/tools/cron_add.rs | 34 +++++++++++- src/tools/cron_update.rs | 37 +++++++++++++ 4 files changed, 169 insertions(+), 15 deletions(-) diff --git a/src/cron/store.rs b/src/cron/store.rs index 2dffc6043..e1aa69c73 100644 --- a/src/cron/store.rs +++ b/src/cron/store.rs @@ -261,7 +261,13 @@ pub fn update_job(config: &Config, job_id: &str, patch: CronJobPatch) -> Result< job.delete_after_run = delete_after_run; } if let Some(allowed_tools) = patch.allowed_tools { - job.allowed_tools = Some(allowed_tools); + // Empty list means "clear the allowlist" (all tools available), + // not "allow zero tools". + if allowed_tools.is_empty() { + job.allowed_tools = None; + } else { + job.allowed_tools = Some(allowed_tools); + } } if schedule_changed { diff --git a/src/providers/openrouter.rs b/src/providers/openrouter.rs index b1e1ea4ca..68f0cda6c 100644 --- a/src/providers/openrouter.rs +++ b/src/providers/openrouter.rs @@ -170,19 +170,23 @@ impl OpenRouterProvider { if items.is_empty() { return None; } - Some( - items - .iter() - .map(|tool| NativeToolSpec { - kind: "function".to_string(), - function: NativeToolFunctionSpec { - name: tool.name.clone(), - description: tool.description.clone(), - parameters: tool.parameters.clone(), - }, - }) - .collect(), - ) + let valid: Vec = items + .iter() + .filter(|tool| is_valid_openai_tool_name(&tool.name)) + .map(|tool| NativeToolSpec { + kind: "function".to_string(), + function: NativeToolFunctionSpec { + name: tool.name.clone(), + description: tool.description.clone(), + parameters: tool.parameters.clone(), + }, + }) + .collect(); + if valid.is_empty() { + None + } else { + Some(valid) + } } fn convert_messages(messages: &[ChatMessage]) -> Vec { @@ -645,6 +649,16 @@ impl Provider for OpenRouterProvider { } } +/// Check if a tool name is valid for OpenAI-compatible APIs. +/// Must match `^[a-zA-Z0-9_-]{1,64}$`. +fn is_valid_openai_tool_name(name: &str) -> bool { + !name.is_empty() + && name.len() <= 64 + && name + .bytes() + .all(|b| b.is_ascii_alphanumeric() || b == b'_' || b == b'-') +} + #[cfg(test)] mod tests { use super::*; @@ -1154,4 +1168,69 @@ mod tests { let provider = OpenRouterProvider::new(Some("key")).with_timeout_secs(300); assert_eq!(provider.timeout_secs, 300); } + + // ═══════════════════════════════════════════════════════════════════════ + // tool name validation tests + // ═══════════════════════════════════════════════════════════════════════ + + #[test] + fn valid_openai_tool_names() { + assert!(is_valid_openai_tool_name("shell")); + assert!(is_valid_openai_tool_name("file_read")); + assert!(is_valid_openai_tool_name("web-search")); + assert!(is_valid_openai_tool_name("Tool123")); + assert!(is_valid_openai_tool_name("a")); + } + + #[test] + fn invalid_openai_tool_names() { + assert!(!is_valid_openai_tool_name("")); + assert!(!is_valid_openai_tool_name("mcp:server.tool")); + assert!(!is_valid_openai_tool_name("node.js")); + assert!(!is_valid_openai_tool_name("tool name")); + assert!(!is_valid_openai_tool_name( + "this_tool_name_is_way_too_long_and_exceeds_the_sixty_four_character_limit_xxxxx" + )); + } + + #[test] + fn convert_tools_skips_invalid_names() { + use crate::tools::ToolSpec; + + let tools = vec![ + ToolSpec { + name: "valid_tool".into(), + description: "A valid tool".into(), + parameters: serde_json::json!({"type": "object"}), + }, + ToolSpec { + name: "mcp:server.bad".into(), + description: "Invalid name".into(), + parameters: serde_json::json!({"type": "object"}), + }, + ToolSpec { + name: "another-valid".into(), + description: "Also valid".into(), + parameters: serde_json::json!({"type": "object"}), + }, + ]; + + let result = OpenRouterProvider::convert_tools(Some(&tools)).unwrap(); + assert_eq!(result.len(), 2); + assert_eq!(result[0].function.name, "valid_tool"); + assert_eq!(result[1].function.name, "another-valid"); + } + + #[test] + fn convert_tools_returns_none_when_all_invalid() { + use crate::tools::ToolSpec; + + let tools = vec![ToolSpec { + name: "mcp:bad.name".into(), + description: "Invalid".into(), + parameters: serde_json::json!({"type": "object"}), + }]; + + assert!(OpenRouterProvider::convert_tools(Some(&tools)).is_none()); + } } diff --git a/src/tools/cron_add.rs b/src/tools/cron_add.rs index 293ad5c08..1b56ab5ff 100644 --- a/src/tools/cron_add.rs +++ b/src/tools/cron_add.rs @@ -315,7 +315,13 @@ impl Tool for CronAddTool { .map(str::to_string); let allowed_tools = match args.get("allowed_tools") { Some(v) => match serde_json::from_value::>(v.clone()) { - Ok(v) => Some(v), + Ok(v) => { + if v.is_empty() { + None // Treat empty list same as unset + } else { + Some(v) + } + } Err(e) => { return Ok(ToolResult { success: false, @@ -694,6 +700,32 @@ mod tests { ); } + #[tokio::test] + async fn empty_allowed_tools_stored_as_none() { + let tmp = TempDir::new().unwrap(); + let cfg = test_config(&tmp).await; + let tool = CronAddTool::new(cfg.clone(), test_security(&cfg)); + + let result = tool + .execute(json!({ + "schedule": { "kind": "cron", "expr": "*/5 * * * *" }, + "job_type": "agent", + "prompt": "check status", + "allowed_tools": [] + })) + .await + .unwrap(); + + assert!(result.success, "{:?}", result.error); + + let jobs = cron::list_jobs(&cfg).unwrap(); + assert_eq!(jobs.len(), 1); + assert_eq!( + jobs[0].allowed_tools, None, + "empty allowed_tools should be stored as None" + ); + } + #[tokio::test] async fn delivery_schema_includes_matrix_channel() { let tmp = TempDir::new().unwrap(); diff --git a/src/tools/cron_update.rs b/src/tools/cron_update.rs index 1abe96b0e..603db29ca 100644 --- a/src/tools/cron_update.rs +++ b/src/tools/cron_update.rs @@ -508,6 +508,43 @@ mod tests { assert!(cron::get_job(&cfg, &job.id).unwrap().enabled); } + #[tokio::test] + async fn empty_allowed_tools_patch_stored_as_none() { + let tmp = TempDir::new().unwrap(); + let cfg = test_config(&tmp).await; + let job = cron::add_agent_job( + &cfg, + None, + crate::cron::Schedule::Cron { + expr: "*/5 * * * *".into(), + tz: None, + }, + "check status", + crate::cron::SessionTarget::Isolated, + None, + None, + false, + Some(vec!["file_read".into()]), + ) + .unwrap(); + let tool = CronUpdateTool::new(cfg.clone(), test_security(&cfg)); + + let result = tool + .execute(json!({ + "job_id": job.id, + "patch": { "allowed_tools": [] } + })) + .await + .unwrap(); + + assert!(result.success, "{:?}", result.error); + assert_eq!( + cron::get_job(&cfg, &job.id).unwrap().allowed_tools, + None, + "empty allowed_tools patch should clear to None" + ); + } + #[tokio::test] async fn updates_agent_allowed_tools() { let tmp = TempDir::new().unwrap();