diff --git a/crates/goose/src/agents/platform_tools.rs b/crates/goose/src/agents/platform_tools.rs index 037c25905ab6..841c18d43f9e 100644 --- a/crates/goose/src/agents/platform_tools.rs +++ b/crates/goose/src/agents/platform_tools.rs @@ -143,7 +143,7 @@ pub fn manage_schedule_tool() -> Tool { }, "job_id": {"type": "string", "description": "Job identifier for operations on existing jobs"}, "recipe_path": {"type": "string", "description": "Path to recipe file for create action"}, - "cron_expression": {"type": "string", "description": "A six field cron expression for create action"}, + "cron_expression": {"type": "string", "description": "A cron expression for create action. Supports both 5-field (minute hour day month weekday) and 6-field (second minute hour day month weekday) formats. 5-field expressions are automatically converted to 6-field by prepending '0' for seconds."}, "execution_mode": {"type": "string", "description": "Execution mode for create action: 'foreground' or 'background'", "enum": ["foreground", "background"], "default": "background"}, "limit": {"type": "integer", "description": "Limit for sessions list", "default": 50}, "session_id": {"type": "string", "description": "Session identifier for session_content action"} diff --git a/crates/goose/src/cron_test.rs b/crates/goose/src/cron_test.rs new file mode 100644 index 000000000000..bbc1e7ae65dc --- /dev/null +++ b/crates/goose/src/cron_test.rs @@ -0,0 +1,60 @@ +#[cfg(test)] +mod cron_parsing_tests { + use crate::scheduler::normalize_cron_expression; + use tokio_cron_scheduler::Job; + + #[test] + fn test_normalize_cron_expression() { + // Test 5-field to 6-field conversion + assert_eq!(normalize_cron_expression("0 12 * * *"), "0 0 12 * * *"); + assert_eq!(normalize_cron_expression("*/5 * * * *"), "0 */5 * * * *"); + assert_eq!(normalize_cron_expression("0 0 * * 1"), "0 0 0 * * 1"); + + // Test 6-field expressions (should remain unchanged) + assert_eq!(normalize_cron_expression("0 0 12 * * *"), "0 0 12 * * *"); + assert_eq!( + normalize_cron_expression("*/30 */5 * * * *"), + "*/30 */5 * * * *" + ); + + // Test invalid expressions (should remain unchanged but warn) + assert_eq!(normalize_cron_expression("* * *"), "* * *"); + assert_eq!(normalize_cron_expression("* * * * * * *"), "* * * * * * *"); + assert_eq!(normalize_cron_expression(""), ""); + } + + #[tokio::test] + async fn test_cron_expression_formats() { + // Test different cron formats to see which ones work + let test_expressions = vec![ + ("0 0 * * *", "5-field: every day at midnight"), + ("0 0 0 * * *", "6-field: every day at midnight"), + ("* * * * *", "5-field: every minute"), + ("* * * * * *", "6-field: every second"), + ("0 */5 * * *", "5-field: every 5 minutes"), + ("0 0 */5 * * *", "6-field: every 5 minutes"), + ("0 0 12 * * *", "6-field: every day at noon"), + ("0 12 * * *", "5-field: every day at noon"), + ]; + + for (expr, desc) in test_expressions { + println!("Testing cron expression: '{}' ({})", expr, desc); + let expr_owned = expr.to_string(); + + // Test with normalization + let normalized = normalize_cron_expression(expr); + println!(" Normalized to: '{}'", normalized); + + match Job::new_async(&normalized, move |_uuid, _l| { + let expr_clone = expr_owned.clone(); + Box::pin(async move { + println!("Job executed for: {}", expr_clone); + }) + }) { + Ok(_) => println!(" ✅ Successfully parsed normalized: '{}'", normalized), + Err(e) => println!(" ❌ Failed to parse normalized '{}': {}", normalized, e), + } + println!(); + } + } +} diff --git a/crates/goose/src/lib.rs b/crates/goose/src/lib.rs index 7e892a867f53..83c4934d76fa 100644 --- a/crates/goose/src/lib.rs +++ b/crates/goose/src/lib.rs @@ -15,3 +15,6 @@ pub mod temporal_scheduler; pub mod token_counter; pub mod tool_monitor; pub mod tracing; + +#[cfg(test)] +mod cron_test; diff --git a/crates/goose/src/scheduler.rs b/crates/goose/src/scheduler.rs index 4ed68dbc7d4e..b9779eb8c1ff 100644 --- a/crates/goose/src/scheduler.rs +++ b/crates/goose/src/scheduler.rs @@ -27,6 +27,34 @@ use crate::session::storage::SessionMetadata; type RunningTasksMap = HashMap; type JobsMap = HashMap; +/// Converts a 5-field cron expression to a 6-field expression by prepending "0" for seconds +/// If the expression already has 6 fields, returns it unchanged +/// If the expression is invalid, returns the original expression +pub fn normalize_cron_expression(cron_expr: &str) -> String { + let fields: Vec<&str> = cron_expr.split_whitespace().collect(); + + match fields.len() { + 5 => { + // 5-field cron: minute hour day month weekday + // Convert to 6-field: second minute hour day month weekday + format!("0 {}", cron_expr) + } + 6 => { + // Already 6-field, return as-is + cron_expr.to_string() + } + _ => { + // Invalid number of fields, return original (will likely fail parsing later) + tracing::warn!( + "Invalid cron expression '{}': expected 5 or 6 fields, got {}", + cron_expr, + fields.len() + ); + cron_expr.to_string() + } + } +} + pub fn get_default_scheduler_storage_path() -> Result { let strategy = choose_app_strategy(config::APP_STRATEGY.clone()) .map_err(|e| io::Error::new(io::ErrorKind::NotFound, e.to_string()))?; @@ -233,7 +261,16 @@ impl Scheduler { let storage_path_for_task = self.storage_path.clone(); let running_tasks_for_task = self.running_tasks.clone(); - let cron_task = Job::new_async(&stored_job.cron, move |_uuid, _l| { + tracing::info!("Attempting to parse cron expression: '{}'", stored_job.cron); + let normalized_cron = normalize_cron_expression(&stored_job.cron); + if normalized_cron != stored_job.cron { + tracing::info!( + "Normalized cron expression from '{}' to '{}'", + stored_job.cron, + normalized_cron + ); + } + let cron_task = Job::new_async(&normalized_cron, move |_uuid, _l| { let task_job_id = job_for_task.id.clone(); let current_jobs_arc = jobs_arc_for_task.clone(); let local_storage_path = storage_path_for_task.clone(); @@ -389,7 +426,20 @@ impl Scheduler { let storage_path_for_task = self.storage_path.clone(); let running_tasks_for_task = self.running_tasks.clone(); - let cron_task = Job::new_async(&job_to_load.cron, move |_uuid, _l| { + tracing::info!( + "Loading job '{}' with cron expression: '{}'", + job_to_load.id, + job_to_load.cron + ); + let normalized_cron = normalize_cron_expression(&job_to_load.cron); + if normalized_cron != job_to_load.cron { + tracing::info!( + "Normalized cron expression from '{}' to '{}'", + job_to_load.cron, + normalized_cron + ); + } + let cron_task = Job::new_async(&normalized_cron, move |_uuid, _l| { let task_job_id = job_for_task.id.clone(); let current_jobs_arc = jobs_arc_for_task.clone(); let local_storage_path = storage_path_for_task.clone(); @@ -747,7 +797,20 @@ impl Scheduler { let storage_path_for_task = self.storage_path.clone(); let running_tasks_for_task = self.running_tasks.clone(); - let cron_task = Job::new_async(&new_cron, move |_uuid, _l| { + tracing::info!( + "Updating job '{}' with new cron expression: '{}'", + sched_id, + new_cron + ); + let normalized_cron = normalize_cron_expression(&new_cron); + if normalized_cron != new_cron { + tracing::info!( + "Normalized cron expression from '{}' to '{}'", + new_cron, + normalized_cron + ); + } + let cron_task = Job::new_async(&normalized_cron, move |_uuid, _l| { let task_job_id = job_for_task.id.clone(); let current_jobs_arc = jobs_arc_for_task.clone(); let local_storage_path = storage_path_for_task.clone(); diff --git a/crates/goose/src/temporal_scheduler.rs b/crates/goose/src/temporal_scheduler.rs index 8fdf81cb3f8f..6e155ac21127 100644 --- a/crates/goose/src/temporal_scheduler.rs +++ b/crates/goose/src/temporal_scheduler.rs @@ -9,7 +9,7 @@ use serde::{Deserialize, Serialize}; use tokio::time::sleep; use tracing::{info, warn}; -use crate::scheduler::{ScheduledJob, SchedulerError}; +use crate::scheduler::{normalize_cron_expression, ScheduledJob, SchedulerError}; use crate::scheduler_trait::SchedulerTrait; use crate::session::storage::SessionMetadata; @@ -487,10 +487,21 @@ impl TemporalScheduler { "TemporalScheduler: add_scheduled_job() called for job '{}'", job.id ); + + // Normalize the cron expression to ensure it's 6-field format + let normalized_cron = normalize_cron_expression(&job.cron); + if normalized_cron != job.cron { + tracing::info!( + "TemporalScheduler: Normalized cron expression from '{}' to '{}'", + job.cron, + normalized_cron + ); + } + let request = JobRequest { action: "create".to_string(), job_id: Some(job.id.clone()), - cron: Some(job.cron.clone()), + cron: Some(normalized_cron), recipe_path: Some(job.source.clone()), execution_mode: job.execution_mode.clone(), }; @@ -690,10 +701,20 @@ impl TemporalScheduler { new_cron ); + // Normalize the cron expression to ensure it's 6-field format + let normalized_cron = normalize_cron_expression(&new_cron); + if normalized_cron != new_cron { + tracing::info!( + "TemporalScheduler: Normalized cron expression from '{}' to '{}'", + new_cron, + normalized_cron + ); + } + let request = JobRequest { action: "update".to_string(), job_id: Some(sched_id.to_string()), - cron: Some(new_cron), + cron: Some(normalized_cron), recipe_path: None, execution_mode: None, }; @@ -1284,4 +1305,24 @@ mod tests { } } } + + #[test] + fn test_cron_normalization_in_temporal_scheduler() { + // Test that the temporal scheduler uses cron normalization correctly + use crate::scheduler::normalize_cron_expression; + + // Test cases that should be normalized + assert_eq!(normalize_cron_expression("0 12 * * *"), "0 0 12 * * *"); + assert_eq!(normalize_cron_expression("*/5 * * * *"), "0 */5 * * * *"); + assert_eq!(normalize_cron_expression("0 0 * * 1"), "0 0 0 * * 1"); + + // Test cases that should remain unchanged + assert_eq!(normalize_cron_expression("0 0 12 * * *"), "0 0 12 * * *"); + assert_eq!( + normalize_cron_expression("*/30 */5 * * * *"), + "*/30 */5 * * * *" + ); + + println!("✅ Cron normalization works correctly in TemporalScheduler"); + } }