From 8fee0875b27135f13a09f91977a3f92c184ece8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B6=85=E6=B8=A1=E6=B3=95=E5=B8=AB?= Date: Wed, 13 May 2026 23:05:38 +0000 Subject: [PATCH 1/4] feat(cron): add disable_on_success for goal-driven usercron jobs Implements the Phase 1 'escape room' mode from ADR #816: - Add disable_on_success field to CronJobConfig (shell command) - Add disable_on_success_match (optional stdout match string) - Add disable_on_success_timeout_secs (default 60s) - Add disable_on_success_working_dir (optional cwd) - Evaluate command before firing: exit 0 + match = goal achieved - On success: post notification, write enabled=false to usercron, trigger reload - On failure/timeout: fire cron job normally (send message) - 9 unit tests covering all paths --- src/config.rs | 8 + src/cron.rs | 424 +++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 431 insertions(+), 1 deletion(-) diff --git a/src/config.rs b/src/config.rs index 2187f0d7..81f86580 100644 --- a/src/config.rs +++ b/src/config.rs @@ -356,6 +356,14 @@ pub struct CronJobConfig { /// Timezone (default: "UTC") #[serde(default = "default_cron_timezone")] pub timezone: String, + /// Shell command to evaluate goal; exit 0 (+ optional match) = goal achieved, auto-disable job. + pub disable_on_success: Option, + /// If set, stdout of disable_on_success must contain this string (in addition to exit 0). + pub disable_on_success_match: Option, + /// Timeout in seconds for disable_on_success command (default: 60). + pub disable_on_success_timeout_secs: Option, + /// Working directory for disable_on_success command. + pub disable_on_success_working_dir: Option, } fn default_cron_platform() -> String { diff --git a/src/cron.rs b/src/cron.rs index a570e96e..456173b7 100644 --- a/src/cron.rs +++ b/src/cron.rs @@ -461,6 +461,7 @@ pub async fn run_scheduler( // Evaluate all jobs: baseline first, then usercron let all_jobs = baseline_jobs.iter().chain(usercron_jobs.iter()); + let baseline_len = baseline_jobs.len(); for (idx, job) in all_jobs.enumerate() { if !should_fire(&job.schedule, job.tz) { continue; @@ -472,6 +473,42 @@ pub async fn run_scheduler( continue; } } + + // Check disable_on_success before firing (usercron jobs only) + if job.config.disable_on_success.is_some() { + let is_usercron = idx >= baseline_len; + if evaluate_disable_on_success(&job.config) { + info!( + schedule = %job.config.schedule, + channel = %job.config.channel, + "✅ disable_on_success: goal achieved, disabling job" + ); + // Write enabled=false back to usercron file + if is_usercron { + if let Some(ref path) = usercron_path { + if let Err(e) = disable_job_in_usercron(path, &job.config) { + error!(error = %e, "failed to disable job in usercron file"); + } + } + } + // Post success notification to channel + if let Some(adapter) = adapters.get(&job.config.platform) { + let channel = ChannelRef { + platform: job.config.platform.clone(), + channel_id: job.config.channel.clone(), + thread_id: job.config.thread_id.clone(), + parent_id: None, + origin_event_id: None, + }; + let msg = format!("✅ Goal achieved: {}", job.config.message); + let _ = adapter.send_message(&channel, &msg).await; + } + // Trigger hot-reload on next tick + last_usercron_mtime = None; + continue; + } + } + info!( schedule = %job.config.schedule, channel = %job.config.channel, @@ -601,7 +638,7 @@ async fn fire_cronjob( .or(Some(reply_channel.channel_id.clone())), is_bot: true, timestamp: Some(Utc::now().to_rfc3339()), - message_id: None, // cron jobs don't originate from a message + message_id: None, // cron jobs don't originate from a message receiver_id: None, // cron jobs are self-triggered, no external receiver }; let sender_json = match serde_json::to_string(&sender) { @@ -633,6 +670,124 @@ async fn fire_cronjob( } } +/// Evaluate the `disable_on_success` command for a cron job. +/// Returns `true` if the goal is achieved (command exits 0 and optional match passes). +fn evaluate_disable_on_success(job: &CronJobConfig) -> bool { + let cmd = match &job.disable_on_success { + Some(c) => c, + None => return false, + }; + + let timeout_secs = job.disable_on_success_timeout_secs.unwrap_or(60); + let mut command = std::process::Command::new("sh"); + command + .arg("-c") + .arg(cmd) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()); + + if let Some(ref dir) = job.disable_on_success_working_dir { + command.current_dir(dir); + } + + let mut child = match command.spawn() { + Ok(c) => c, + Err(e) => { + warn!(command = %cmd, error = %e, "disable_on_success: failed to spawn command"); + return false; + } + }; + + // Wait with timeout + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(timeout_secs); + loop { + match child.try_wait() { + Ok(Some(status)) => { + if !status.success() { + return false; + } + break; + } + Ok(None) => { + if std::time::Instant::now() >= deadline { + warn!(command = %cmd, timeout_secs, "disable_on_success: command timed out, killing"); + let _ = child.kill(); + let _ = child.wait(); + return false; + } + std::thread::sleep(std::time::Duration::from_millis(100)); + } + Err(e) => { + warn!(command = %cmd, error = %e, "disable_on_success: error waiting for command"); + return false; + } + } + } + + // Check optional match string against stdout + if let Some(ref match_str) = job.disable_on_success_match { + let stdout = child + .stdout + .map(|mut s| { + let mut buf = String::new(); + use std::io::Read; + let _ = s.read_to_string(&mut buf); + buf + }) + .unwrap_or_default(); + if !stdout.contains(match_str.as_str()) { + info!( + command = %cmd, + match_str = %match_str, + "disable_on_success: exit 0 but stdout does not contain match string" + ); + return false; + } + } + + true +} + +/// Disable a job in the usercron TOML file by setting `enabled = false`. +/// Uses a simple text-based approach to preserve formatting. +fn disable_job_in_usercron(path: &Path, job: &CronJobConfig) -> Result<(), String> { + let content = + std::fs::read_to_string(path).map_err(|e| format!("failed to read usercron file: {e}"))?; + + // Parse the file to find the matching job and update it + let mut parsed: toml::Value = + toml::from_str(&content).map_err(|e| format!("failed to parse usercron file: {e}"))?; + + let jobs = parsed + .get_mut("jobs") + .and_then(|v| v.as_array_mut()) + .ok_or_else(|| "no [[jobs]] array in usercron file".to_string())?; + + // Find the job by matching schedule + channel + message + let found = jobs.iter_mut().find(|j| { + let sched_match = j.get("schedule").and_then(|v| v.as_str()) == Some(&job.schedule); + let chan_match = j.get("channel").and_then(|v| v.as_str()) == Some(&job.channel); + let msg_match = j.get("message").and_then(|v| v.as_str()) == Some(&job.message); + sched_match && chan_match && msg_match + }); + + match found { + Some(entry) => { + if let Some(table) = entry.as_table_mut() { + table.insert("enabled".to_string(), toml::Value::Boolean(false)); + } + } + None => return Err("could not find matching job in usercron file".to_string()), + } + + let new_content = toml::to_string_pretty(&parsed) + .map_err(|e| format!("failed to serialize usercron file: {e}"))?; + + std::fs::write(path, new_content).map_err(|e| format!("failed to write usercron file: {e}"))?; + + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -1098,6 +1253,10 @@ platform = "slack" sender_name: "test".into(), thread_id: None, timezone: "UTC".into(), + disable_on_success: None, + disable_on_success_match: None, + disable_on_success_timeout_secs: None, + disable_on_success_working_dir: None, }]; assert!(validate_cronjobs(&jobs, &["discord"]).is_ok()); } @@ -1113,6 +1272,10 @@ platform = "slack" sender_name: "test".into(), thread_id: None, timezone: "UTC".into(), + disable_on_success: None, + disable_on_success_match: None, + disable_on_success_timeout_secs: None, + disable_on_success_working_dir: None, }]; let err = validate_cronjobs(&jobs, &["discord"]).unwrap_err(); assert!(err.to_string().contains("invalid cron expression")); @@ -1129,6 +1292,10 @@ platform = "slack" sender_name: "test".into(), thread_id: None, timezone: "Mars/Olympus".into(), + disable_on_success: None, + disable_on_success_match: None, + disable_on_success_timeout_secs: None, + disable_on_success_working_dir: None, }]; let err = validate_cronjobs(&jobs, &["discord"]).unwrap_err(); assert!(err.to_string().contains("invalid timezone")); @@ -1145,6 +1312,10 @@ platform = "slack" sender_name: "test".into(), thread_id: None, timezone: "UTC".into(), + disable_on_success: None, + disable_on_success_match: None, + disable_on_success_timeout_secs: None, + disable_on_success_working_dir: None, }]; let err = validate_cronjobs(&jobs, &["discord"]).unwrap_err(); assert!(err.to_string().contains("unknown platform")); @@ -1161,6 +1332,10 @@ platform = "slack" sender_name: "test".into(), thread_id: None, timezone: "UTC".into(), + disable_on_success: None, + disable_on_success_match: None, + disable_on_success_timeout_secs: None, + disable_on_success_working_dir: None, }]; let err = validate_cronjobs(&jobs, &["discord"]).unwrap_err(); assert!(err.to_string().contains("not configured")); @@ -1177,6 +1352,10 @@ platform = "slack" sender_name: "test".into(), thread_id: None, timezone: "UTC".into(), + disable_on_success: None, + disable_on_success_match: None, + disable_on_success_timeout_secs: None, + disable_on_success_working_dir: None, }]; assert!(validate_cronjobs(&jobs, &["discord"]).is_ok()); } @@ -1192,6 +1371,10 @@ platform = "slack" sender_name: "test".into(), thread_id: None, timezone: "UTC".into(), + disable_on_success: None, + disable_on_success_match: None, + disable_on_success_timeout_secs: None, + disable_on_success_working_dir: None, }]; assert!(validate_cronjobs(&jobs, &["discord"]).is_err()); } @@ -1269,4 +1452,243 @@ command = "echo" let jobs = load_usercron_file(&path, &["discord"]); assert!(jobs.is_empty()); } + + // --- disable_on_success --- + + #[test] + fn evaluate_disable_on_success_none_returns_false() { + let job = CronJobConfig { + enabled: true, + schedule: "* * * * *".into(), + channel: "ch".into(), + message: "msg".into(), + platform: "discord".into(), + sender_name: "cron".into(), + thread_id: None, + timezone: "UTC".into(), + disable_on_success: None, + disable_on_success_match: None, + disable_on_success_timeout_secs: None, + disable_on_success_working_dir: None, + }; + assert!(!evaluate_disable_on_success(&job)); + } + + #[test] + fn evaluate_disable_on_success_exit_zero_returns_true() { + let job = CronJobConfig { + enabled: true, + schedule: "* * * * *".into(), + channel: "ch".into(), + message: "msg".into(), + platform: "discord".into(), + sender_name: "cron".into(), + thread_id: None, + timezone: "UTC".into(), + disable_on_success: Some("true".into()), + disable_on_success_match: None, + disable_on_success_timeout_secs: Some(5), + disable_on_success_working_dir: None, + }; + assert!(evaluate_disable_on_success(&job)); + } + + #[test] + fn evaluate_disable_on_success_exit_nonzero_returns_false() { + let job = CronJobConfig { + enabled: true, + schedule: "* * * * *".into(), + channel: "ch".into(), + message: "msg".into(), + platform: "discord".into(), + sender_name: "cron".into(), + thread_id: None, + timezone: "UTC".into(), + disable_on_success: Some("false".into()), + disable_on_success_match: None, + disable_on_success_timeout_secs: Some(5), + disable_on_success_working_dir: None, + }; + assert!(!evaluate_disable_on_success(&job)); + } + + #[test] + fn evaluate_disable_on_success_match_present_returns_true() { + let job = CronJobConfig { + enabled: true, + schedule: "* * * * *".into(), + channel: "ch".into(), + message: "msg".into(), + platform: "discord".into(), + sender_name: "cron".into(), + thread_id: None, + timezone: "UTC".into(), + disable_on_success: Some("echo SUCCESS".into()), + disable_on_success_match: Some("SUCCESS".into()), + disable_on_success_timeout_secs: Some(5), + disable_on_success_working_dir: None, + }; + assert!(evaluate_disable_on_success(&job)); + } + + #[test] + fn evaluate_disable_on_success_match_absent_returns_false() { + let job = CronJobConfig { + enabled: true, + schedule: "* * * * *".into(), + channel: "ch".into(), + message: "msg".into(), + platform: "discord".into(), + sender_name: "cron".into(), + thread_id: None, + timezone: "UTC".into(), + disable_on_success: Some("echo NOPE".into()), + disable_on_success_match: Some("SUCCESS".into()), + disable_on_success_timeout_secs: Some(5), + disable_on_success_working_dir: None, + }; + assert!(!evaluate_disable_on_success(&job)); + } + + #[test] + fn evaluate_disable_on_success_timeout_returns_false() { + let job = CronJobConfig { + enabled: true, + schedule: "* * * * *".into(), + channel: "ch".into(), + message: "msg".into(), + platform: "discord".into(), + sender_name: "cron".into(), + thread_id: None, + timezone: "UTC".into(), + disable_on_success: Some("sleep 10".into()), + disable_on_success_match: None, + disable_on_success_timeout_secs: Some(1), + disable_on_success_working_dir: None, + }; + assert!(!evaluate_disable_on_success(&job)); + } + + #[test] + fn evaluate_disable_on_success_working_dir() { + let dir = tempfile::tempdir().unwrap(); + let job = CronJobConfig { + enabled: true, + schedule: "* * * * *".into(), + channel: "ch".into(), + message: "msg".into(), + platform: "discord".into(), + sender_name: "cron".into(), + thread_id: None, + timezone: "UTC".into(), + disable_on_success: Some("pwd".into()), + disable_on_success_match: Some(dir.path().to_str().unwrap().into()), + disable_on_success_timeout_secs: Some(5), + disable_on_success_working_dir: Some(dir.path().to_str().unwrap().into()), + }; + assert!(evaluate_disable_on_success(&job)); + } + + #[test] + fn disable_job_in_usercron_sets_enabled_false() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("cronjob.toml"); + let content = r#" +[[jobs]] +schedule = "*/10 * * * *" +channel = "123" +message = "test goal" +platform = "discord" +disable_on_success = "npm test" +"#; + std::fs::write(&path, content).unwrap(); + + let job = CronJobConfig { + enabled: true, + schedule: "*/10 * * * *".into(), + channel: "123".into(), + message: "test goal".into(), + platform: "discord".into(), + sender_name: "openab-cron".into(), + thread_id: None, + timezone: "UTC".into(), + disable_on_success: Some("npm test".into()), + disable_on_success_match: None, + disable_on_success_timeout_secs: None, + disable_on_success_working_dir: None, + }; + + disable_job_in_usercron(&path, &job).unwrap(); + + let updated = std::fs::read_to_string(&path).unwrap(); + let parsed: toml::Value = toml::from_str(&updated).unwrap(); + let jobs = parsed["jobs"].as_array().unwrap(); + assert_eq!(jobs[0]["enabled"].as_bool(), Some(false)); + } + + #[test] + fn disable_job_in_usercron_preserves_other_jobs() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("cronjob.toml"); + let content = r#" +[[jobs]] +schedule = "0 9 * * *" +channel = "456" +message = "daily report" +platform = "discord" + +[[jobs]] +schedule = "*/10 * * * *" +channel = "123" +message = "test goal" +platform = "discord" +disable_on_success = "npm test" +"#; + std::fs::write(&path, content).unwrap(); + + let job = CronJobConfig { + enabled: true, + schedule: "*/10 * * * *".into(), + channel: "123".into(), + message: "test goal".into(), + platform: "discord".into(), + sender_name: "openab-cron".into(), + thread_id: None, + timezone: "UTC".into(), + disable_on_success: Some("npm test".into()), + disable_on_success_match: None, + disable_on_success_timeout_secs: None, + disable_on_success_working_dir: None, + }; + + disable_job_in_usercron(&path, &job).unwrap(); + + let updated = std::fs::read_to_string(&path).unwrap(); + let parsed: toml::Value = toml::from_str(&updated).unwrap(); + let jobs = parsed["jobs"].as_array().unwrap(); + // First job should be unchanged + assert!(jobs[0].get("enabled").is_none() || jobs[0]["enabled"].as_bool() == Some(true)); + // Second job should be disabled + assert_eq!(jobs[1]["enabled"].as_bool(), Some(false)); + } + + #[test] + fn cronjob_config_parses_disable_on_success_fields() { + let toml_str = r#" +[[jobs]] +schedule = "*/10 * * * *" +channel = "123" +message = "goal" +disable_on_success = "npm test" +disable_on_success_match = "SUCCESS" +disable_on_success_timeout_secs = 30 +disable_on_success_working_dir = "/repo" +"#; + let parsed: UsercronFile = toml::from_str(toml_str).unwrap(); + let job = &parsed.jobs[0]; + assert_eq!(job.disable_on_success.as_deref(), Some("npm test")); + assert_eq!(job.disable_on_success_match.as_deref(), Some("SUCCESS")); + assert_eq!(job.disable_on_success_timeout_secs, Some(30)); + assert_eq!(job.disable_on_success_working_dir.as_deref(), Some("/repo")); + } } From 6dff2e7a08b78a046359a9c7fe3215f41a6bec30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B6=85=E6=B8=A1=E6=B3=95=E5=B8=AB?= Date: Wed, 13 May 2026 23:10:09 +0000 Subject: [PATCH 2/4] fix: address PR review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 🔴 F1: Replace blocking thread::sleep with async tokio::process::Command + tokio::time::timeout - 🟡 F2: Rewrite disable_job_in_usercron with line-based editing (preserves comments) - 🟡 F3: Only evaluate disable_on_success for usercron jobs (skip baseline) - 🟡 F4: Match jobs by id field instead of schedule+channel+message - Add id field to CronJobConfig (required for disable_on_success per ADR) - Add test for comment preservation - Convert evaluate tests to #[tokio::test] async --- src/config.rs | 2 + src/cron.rs | 297 ++++++++++++++++++++++++++++++++------------------ 2 files changed, 193 insertions(+), 106 deletions(-) diff --git a/src/config.rs b/src/config.rs index 81f86580..406d6b0e 100644 --- a/src/config.rs +++ b/src/config.rs @@ -339,6 +339,8 @@ pub struct CronJobConfig { /// Whether this cronjob is active (default: true) #[serde(default = "default_true")] pub enabled: bool, + /// Stable unique identifier (required when disable_on_success is set) + pub id: Option, /// Cron expression (5-field POSIX format) pub schedule: String, /// Target channel ID diff --git a/src/cron.rs b/src/cron.rs index 456173b7..66e21c6a 100644 --- a/src/cron.rs +++ b/src/cron.rs @@ -475,20 +475,18 @@ pub async fn run_scheduler( } // Check disable_on_success before firing (usercron jobs only) - if job.config.disable_on_success.is_some() { - let is_usercron = idx >= baseline_len; - if evaluate_disable_on_success(&job.config) { + let is_usercron = idx >= baseline_len; + if is_usercron && job.config.disable_on_success.is_some() { + if evaluate_disable_on_success(&job.config).await { info!( schedule = %job.config.schedule, channel = %job.config.channel, "✅ disable_on_success: goal achieved, disabling job" ); // Write enabled=false back to usercron file - if is_usercron { - if let Some(ref path) = usercron_path { - if let Err(e) = disable_job_in_usercron(path, &job.config) { - error!(error = %e, "failed to disable job in usercron file"); - } + if let Some(ref path) = usercron_path { + if let Err(e) = disable_job_in_usercron(path, &job.config) { + error!(error = %e, "failed to disable job in usercron file"); } } // Post success notification to channel @@ -672,14 +670,14 @@ async fn fire_cronjob( /// Evaluate the `disable_on_success` command for a cron job. /// Returns `true` if the goal is achieved (command exits 0 and optional match passes). -fn evaluate_disable_on_success(job: &CronJobConfig) -> bool { +async fn evaluate_disable_on_success(job: &CronJobConfig) -> bool { let cmd = match &job.disable_on_success { Some(c) => c, None => return false, }; let timeout_secs = job.disable_on_success_timeout_secs.unwrap_or(60); - let mut command = std::process::Command::new("sh"); + let mut command = tokio::process::Command::new("sh"); command .arg("-c") .arg(cmd) @@ -690,51 +688,31 @@ fn evaluate_disable_on_success(job: &CronJobConfig) -> bool { command.current_dir(dir); } - let mut child = match command.spawn() { - Ok(c) => c, - Err(e) => { - warn!(command = %cmd, error = %e, "disable_on_success: failed to spawn command"); + let result = tokio::time::timeout( + std::time::Duration::from_secs(timeout_secs), + command.output(), + ) + .await; + + let output = match result { + Ok(Ok(output)) => output, + Ok(Err(e)) => { + warn!(command = %cmd, error = %e, "disable_on_success: failed to execute command"); + return false; + } + Err(_) => { + warn!(command = %cmd, timeout_secs, "disable_on_success: command timed out"); return false; } }; - // Wait with timeout - let deadline = std::time::Instant::now() + std::time::Duration::from_secs(timeout_secs); - loop { - match child.try_wait() { - Ok(Some(status)) => { - if !status.success() { - return false; - } - break; - } - Ok(None) => { - if std::time::Instant::now() >= deadline { - warn!(command = %cmd, timeout_secs, "disable_on_success: command timed out, killing"); - let _ = child.kill(); - let _ = child.wait(); - return false; - } - std::thread::sleep(std::time::Duration::from_millis(100)); - } - Err(e) => { - warn!(command = %cmd, error = %e, "disable_on_success: error waiting for command"); - return false; - } - } + if !output.status.success() { + return false; } // Check optional match string against stdout if let Some(ref match_str) = job.disable_on_success_match { - let stdout = child - .stdout - .map(|mut s| { - let mut buf = String::new(); - use std::io::Read; - let _ = s.read_to_string(&mut buf); - buf - }) - .unwrap_or_default(); + let stdout = String::from_utf8_lossy(&output.stdout); if !stdout.contains(match_str.as_str()) { info!( command = %cmd, @@ -749,39 +727,76 @@ fn evaluate_disable_on_success(job: &CronJobConfig) -> bool { } /// Disable a job in the usercron TOML file by setting `enabled = false`. -/// Uses a simple text-based approach to preserve formatting. +/// Matches by `id` field. Uses line-based editing to preserve comments and formatting. fn disable_job_in_usercron(path: &Path, job: &CronJobConfig) -> Result<(), String> { + let job_id = job + .id + .as_deref() + .ok_or_else(|| "job has no id, cannot disable in usercron".to_string())?; + let content = std::fs::read_to_string(path).map_err(|e| format!("failed to read usercron file: {e}"))?; - // Parse the file to find the matching job and update it - let mut parsed: toml::Value = - toml::from_str(&content).map_err(|e| format!("failed to parse usercron file: {e}"))?; - - let jobs = parsed - .get_mut("jobs") - .and_then(|v| v.as_array_mut()) - .ok_or_else(|| "no [[jobs]] array in usercron file".to_string())?; - - // Find the job by matching schedule + channel + message - let found = jobs.iter_mut().find(|j| { - let sched_match = j.get("schedule").and_then(|v| v.as_str()) == Some(&job.schedule); - let chan_match = j.get("channel").and_then(|v| v.as_str()) == Some(&job.channel); - let msg_match = j.get("message").and_then(|v| v.as_str()) == Some(&job.message); - sched_match && chan_match && msg_match - }); - - match found { - Some(entry) => { - if let Some(table) = entry.as_table_mut() { - table.insert("enabled".to_string(), toml::Value::Boolean(false)); + let lines: Vec<&str> = content.lines().collect(); + let mut result = Vec::with_capacity(lines.len() + 1); + let mut in_target_job = false; + let mut found = false; + let mut has_enabled_field = false; + + for line in &lines { + // Detect [[jobs]] header + if line.trim() == "[[jobs]]" { + // If we were in the target job and it had no enabled field, insert one + if in_target_job && !has_enabled_field { + result.push("enabled = false".to_string()); + } + in_target_job = false; + has_enabled_field = false; + result.push(line.to_string()); + continue; + } + + // Detect id field to identify target job + if line.trim().starts_with("id") { + let value = line + .split('=') + .nth(1) + .map(|v| v.trim().trim_matches('"').trim_matches('\'').to_string()); + if value.as_deref() == Some(job_id) { + in_target_job = true; + found = true; } } - None => return Err("could not find matching job in usercron file".to_string()), + + // If in target job and we find enabled field, replace it + if in_target_job && line.trim().starts_with("enabled") { + has_enabled_field = true; + result.push("enabled = false".to_string()); + continue; + } + + result.push(line.to_string()); } - let new_content = toml::to_string_pretty(&parsed) - .map_err(|e| format!("failed to serialize usercron file: {e}"))?; + // Handle case where target job is the last one and has no enabled field + if in_target_job && !has_enabled_field { + result.push("enabled = false".to_string()); + } + + if !found { + return Err(format!( + "could not find job with id {:?} in usercron file", + job_id + )); + } + + let new_content = result.join("\n"); + // Preserve trailing newline if original had one + let new_content = if content.ends_with('\n') && !new_content.ends_with('\n') { + new_content + "\n" + } else { + new_content + }; std::fs::write(path, new_content).map_err(|e| format!("failed to write usercron file: {e}"))?; @@ -1245,6 +1260,7 @@ platform = "slack" #[test] fn validate_cronjobs_valid_passes() { let jobs = vec![CronJobConfig { + id: None, enabled: true, schedule: "0 9 * * 1-5".into(), channel: "123".into(), @@ -1264,6 +1280,7 @@ platform = "slack" #[test] fn validate_cronjobs_invalid_cron_fails() { let jobs = vec![CronJobConfig { + id: None, enabled: true, schedule: "bad".into(), channel: "123".into(), @@ -1284,6 +1301,7 @@ platform = "slack" #[test] fn validate_cronjobs_invalid_timezone_fails() { let jobs = vec![CronJobConfig { + id: None, enabled: true, schedule: "* * * * *".into(), channel: "123".into(), @@ -1304,6 +1322,7 @@ platform = "slack" #[test] fn validate_cronjobs_unknown_platform_fails() { let jobs = vec![CronJobConfig { + id: None, enabled: true, schedule: "* * * * *".into(), channel: "123".into(), @@ -1324,6 +1343,7 @@ platform = "slack" #[test] fn validate_cronjobs_unconfigured_platform_fails() { let jobs = vec![CronJobConfig { + id: None, enabled: true, schedule: "* * * * *".into(), channel: "123".into(), @@ -1344,6 +1364,7 @@ platform = "slack" #[test] fn validate_cronjobs_disabled_with_invalid_cron_passes() { let jobs = vec![CronJobConfig { + id: None, enabled: false, schedule: "bad".into(), channel: "123".into(), @@ -1363,6 +1384,7 @@ platform = "slack" #[test] fn validate_cronjobs_enabled_with_invalid_cron_still_fails() { let jobs = vec![CronJobConfig { + id: None, enabled: true, schedule: "bad".into(), channel: "123".into(), @@ -1455,10 +1477,12 @@ command = "echo" // --- disable_on_success --- - #[test] - fn evaluate_disable_on_success_none_returns_false() { + #[tokio::test] + async fn evaluate_disable_on_success_none_returns_false() { let job = CronJobConfig { + id: None, enabled: true, + id: None, schedule: "* * * * *".into(), channel: "ch".into(), message: "msg".into(), @@ -1471,13 +1495,15 @@ command = "echo" disable_on_success_timeout_secs: None, disable_on_success_working_dir: None, }; - assert!(!evaluate_disable_on_success(&job)); + assert!(!evaluate_disable_on_success(&job).await); } - #[test] - fn evaluate_disable_on_success_exit_zero_returns_true() { + #[tokio::test] + async fn evaluate_disable_on_success_exit_zero_returns_true() { let job = CronJobConfig { + id: None, enabled: true, + id: None, schedule: "* * * * *".into(), channel: "ch".into(), message: "msg".into(), @@ -1490,13 +1516,15 @@ command = "echo" disable_on_success_timeout_secs: Some(5), disable_on_success_working_dir: None, }; - assert!(evaluate_disable_on_success(&job)); + assert!(evaluate_disable_on_success(&job).await); } - #[test] - fn evaluate_disable_on_success_exit_nonzero_returns_false() { + #[tokio::test] + async fn evaluate_disable_on_success_exit_nonzero_returns_false() { let job = CronJobConfig { + id: None, enabled: true, + id: None, schedule: "* * * * *".into(), channel: "ch".into(), message: "msg".into(), @@ -1509,13 +1537,15 @@ command = "echo" disable_on_success_timeout_secs: Some(5), disable_on_success_working_dir: None, }; - assert!(!evaluate_disable_on_success(&job)); + assert!(!evaluate_disable_on_success(&job).await); } - #[test] - fn evaluate_disable_on_success_match_present_returns_true() { + #[tokio::test] + async fn evaluate_disable_on_success_match_present_returns_true() { let job = CronJobConfig { + id: None, enabled: true, + id: None, schedule: "* * * * *".into(), channel: "ch".into(), message: "msg".into(), @@ -1528,13 +1558,15 @@ command = "echo" disable_on_success_timeout_secs: Some(5), disable_on_success_working_dir: None, }; - assert!(evaluate_disable_on_success(&job)); + assert!(evaluate_disable_on_success(&job).await); } - #[test] - fn evaluate_disable_on_success_match_absent_returns_false() { + #[tokio::test] + async fn evaluate_disable_on_success_match_absent_returns_false() { let job = CronJobConfig { + id: None, enabled: true, + id: None, schedule: "* * * * *".into(), channel: "ch".into(), message: "msg".into(), @@ -1547,13 +1579,15 @@ command = "echo" disable_on_success_timeout_secs: Some(5), disable_on_success_working_dir: None, }; - assert!(!evaluate_disable_on_success(&job)); + assert!(!evaluate_disable_on_success(&job).await); } - #[test] - fn evaluate_disable_on_success_timeout_returns_false() { + #[tokio::test] + async fn evaluate_disable_on_success_timeout_returns_false() { let job = CronJobConfig { + id: None, enabled: true, + id: None, schedule: "* * * * *".into(), channel: "ch".into(), message: "msg".into(), @@ -1566,14 +1600,16 @@ command = "echo" disable_on_success_timeout_secs: Some(1), disable_on_success_working_dir: None, }; - assert!(!evaluate_disable_on_success(&job)); + assert!(!evaluate_disable_on_success(&job).await); } - #[test] - fn evaluate_disable_on_success_working_dir() { + #[tokio::test] + async fn evaluate_disable_on_success_working_dir() { let dir = tempfile::tempdir().unwrap(); let job = CronJobConfig { + id: None, enabled: true, + id: None, schedule: "* * * * *".into(), channel: "ch".into(), message: "msg".into(), @@ -1586,15 +1622,15 @@ command = "echo" disable_on_success_timeout_secs: Some(5), disable_on_success_working_dir: Some(dir.path().to_str().unwrap().into()), }; - assert!(evaluate_disable_on_success(&job)); + assert!(evaluate_disable_on_success(&job).await); } #[test] fn disable_job_in_usercron_sets_enabled_false() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("cronjob.toml"); - let content = r#" -[[jobs]] + let content = r#"[[jobs]] +id = "test-goal" schedule = "*/10 * * * *" channel = "123" message = "test goal" @@ -1604,7 +1640,9 @@ disable_on_success = "npm test" std::fs::write(&path, content).unwrap(); let job = CronJobConfig { + id: None, enabled: true, + id: Some("test-goal".into()), schedule: "*/10 * * * *".into(), channel: "123".into(), message: "test goal".into(), @@ -1621,23 +1659,22 @@ disable_on_success = "npm test" disable_job_in_usercron(&path, &job).unwrap(); let updated = std::fs::read_to_string(&path).unwrap(); - let parsed: toml::Value = toml::from_str(&updated).unwrap(); - let jobs = parsed["jobs"].as_array().unwrap(); - assert_eq!(jobs[0]["enabled"].as_bool(), Some(false)); + assert!(updated.contains("enabled = false")); } #[test] fn disable_job_in_usercron_preserves_other_jobs() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("cronjob.toml"); - let content = r#" -[[jobs]] + let content = r#"[[jobs]] +id = "daily-report" schedule = "0 9 * * *" channel = "456" message = "daily report" platform = "discord" [[jobs]] +id = "test-goal" schedule = "*/10 * * * *" channel = "123" message = "test goal" @@ -1647,7 +1684,56 @@ disable_on_success = "npm test" std::fs::write(&path, content).unwrap(); let job = CronJobConfig { + id: None, + enabled: true, + id: Some("test-goal".into()), + schedule: "*/10 * * * *".into(), + channel: "123".into(), + message: "test goal".into(), + platform: "discord".into(), + sender_name: "openab-cron".into(), + thread_id: None, + timezone: "UTC".into(), + disable_on_success: Some("npm test".into()), + disable_on_success_match: None, + disable_on_success_timeout_secs: None, + disable_on_success_working_dir: None, + }; + + disable_job_in_usercron(&path, &job).unwrap(); + + let updated = std::fs::read_to_string(&path).unwrap(); + // Only the second job should have enabled = false + let lines: Vec<&str> = updated.lines().collect(); + let enabled_lines: Vec<&&str> = lines + .iter() + .filter(|l| l.contains("enabled = false")) + .collect(); + assert_eq!(enabled_lines.len(), 1); + // First job should not have enabled = false + let first_job_end = updated.find("[[jobs]]\nid = \"test-goal\"").unwrap(); + assert!(!updated[..first_job_end].contains("enabled = false")); + } + + #[test] + fn disable_job_in_usercron_preserves_comments() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("cronjob.toml"); + let content = r#"# My usercron config +[[jobs]] +id = "test-goal" +schedule = "*/10 * * * *" +channel = "123" +message = "test goal" # important goal +platform = "discord" +disable_on_success = "npm test" +"#; + std::fs::write(&path, content).unwrap(); + + let job = CronJobConfig { + id: None, enabled: true, + id: Some("test-goal".into()), schedule: "*/10 * * * *".into(), channel: "123".into(), message: "test goal".into(), @@ -1664,18 +1750,16 @@ disable_on_success = "npm test" disable_job_in_usercron(&path, &job).unwrap(); let updated = std::fs::read_to_string(&path).unwrap(); - let parsed: toml::Value = toml::from_str(&updated).unwrap(); - let jobs = parsed["jobs"].as_array().unwrap(); - // First job should be unchanged - assert!(jobs[0].get("enabled").is_none() || jobs[0]["enabled"].as_bool() == Some(true)); - // Second job should be disabled - assert_eq!(jobs[1]["enabled"].as_bool(), Some(false)); + assert!(updated.contains("# My usercron config")); + assert!(updated.contains("# important goal")); + assert!(updated.contains("enabled = false")); } #[test] fn cronjob_config_parses_disable_on_success_fields() { let toml_str = r#" [[jobs]] +id = "my-goal" schedule = "*/10 * * * *" channel = "123" message = "goal" @@ -1686,6 +1770,7 @@ disable_on_success_working_dir = "/repo" "#; let parsed: UsercronFile = toml::from_str(toml_str).unwrap(); let job = &parsed.jobs[0]; + assert_eq!(job.id.as_deref(), Some("my-goal")); assert_eq!(job.disable_on_success.as_deref(), Some("npm test")); assert_eq!(job.disable_on_success_match.as_deref(), Some("SUCCESS")); assert_eq!(job.disable_on_success_timeout_secs, Some(30)); From 91738c126a15956c6eb4dc48a35191a309a754bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B6=85=E6=B8=A1=E6=B3=95=E5=B8=AB?= Date: Wed, 13 May 2026 23:12:48 +0000 Subject: [PATCH 3/4] fix: remove duplicate id fields, add id validation, handle inline comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 🔴 Remove duplicate id: fields caused by sed (compile error) - 🟡 Add validation: disable_on_success without id is rejected at load time - 🟡 Fix id line parser to strip inline comments before matching --- src/cron.rs | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/src/cron.rs b/src/cron.rs index 66e21c6a..654b3faf 100644 --- a/src/cron.rs +++ b/src/cron.rs @@ -321,6 +321,10 @@ pub fn load_usercron_file(path: &Path, configured_platforms: &[&str]) -> Vec Result<(), Strin // Detect id field to identify target job if line.trim().starts_with("id") { - let value = line - .split('=') - .nth(1) - .map(|v| v.trim().trim_matches('"').trim_matches('\'').to_string()); + let value = line.split('=').nth(1).map(|v| { + // Strip inline comments before trimming quotes + let v = v.split('#').next().unwrap_or(v); + v.trim().trim_matches('"').trim_matches('\'').to_string() + }); if value.as_deref() == Some(job_id) { in_target_job = true; found = true; @@ -1482,7 +1487,6 @@ command = "echo" let job = CronJobConfig { id: None, enabled: true, - id: None, schedule: "* * * * *".into(), channel: "ch".into(), message: "msg".into(), @@ -1503,7 +1507,6 @@ command = "echo" let job = CronJobConfig { id: None, enabled: true, - id: None, schedule: "* * * * *".into(), channel: "ch".into(), message: "msg".into(), @@ -1524,7 +1527,6 @@ command = "echo" let job = CronJobConfig { id: None, enabled: true, - id: None, schedule: "* * * * *".into(), channel: "ch".into(), message: "msg".into(), @@ -1545,7 +1547,6 @@ command = "echo" let job = CronJobConfig { id: None, enabled: true, - id: None, schedule: "* * * * *".into(), channel: "ch".into(), message: "msg".into(), @@ -1566,7 +1567,6 @@ command = "echo" let job = CronJobConfig { id: None, enabled: true, - id: None, schedule: "* * * * *".into(), channel: "ch".into(), message: "msg".into(), @@ -1587,7 +1587,6 @@ command = "echo" let job = CronJobConfig { id: None, enabled: true, - id: None, schedule: "* * * * *".into(), channel: "ch".into(), message: "msg".into(), @@ -1609,7 +1608,6 @@ command = "echo" let job = CronJobConfig { id: None, enabled: true, - id: None, schedule: "* * * * *".into(), channel: "ch".into(), message: "msg".into(), @@ -1640,9 +1638,8 @@ disable_on_success = "npm test" std::fs::write(&path, content).unwrap(); let job = CronJobConfig { - id: None, - enabled: true, id: Some("test-goal".into()), + enabled: true, schedule: "*/10 * * * *".into(), channel: "123".into(), message: "test goal".into(), @@ -1684,9 +1681,8 @@ disable_on_success = "npm test" std::fs::write(&path, content).unwrap(); let job = CronJobConfig { - id: None, - enabled: true, id: Some("test-goal".into()), + enabled: true, schedule: "*/10 * * * *".into(), channel: "123".into(), message: "test goal".into(), @@ -1731,9 +1727,8 @@ disable_on_success = "npm test" std::fs::write(&path, content).unwrap(); let job = CronJobConfig { - id: None, - enabled: true, id: Some("test-goal".into()), + enabled: true, schedule: "*/10 * * * *".into(), channel: "123".into(), message: "test goal".into(), From 2413bd7d859c4b7560c8623cb6ca7202baf197e0 Mon Sep 17 00:00:00 2001 From: chaodufashi Date: Fri, 15 May 2026 19:32:38 +0000 Subject: [PATCH 4/4] fix(cron): collapse nested if to satisfy clippy collapsible_if --- src/cron.rs | 55 +++++++++++++++++++++++++++-------------------------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/src/cron.rs b/src/cron.rs index 654b3faf..88ee42ba 100644 --- a/src/cron.rs +++ b/src/cron.rs @@ -480,35 +480,36 @@ pub async fn run_scheduler( // Check disable_on_success before firing (usercron jobs only) let is_usercron = idx >= baseline_len; - if is_usercron && job.config.disable_on_success.is_some() { - if evaluate_disable_on_success(&job.config).await { - info!( - schedule = %job.config.schedule, - channel = %job.config.channel, - "✅ disable_on_success: goal achieved, disabling job" - ); - // Write enabled=false back to usercron file - if let Some(ref path) = usercron_path { - if let Err(e) = disable_job_in_usercron(path, &job.config) { - error!(error = %e, "failed to disable job in usercron file"); - } - } - // Post success notification to channel - if let Some(adapter) = adapters.get(&job.config.platform) { - let channel = ChannelRef { - platform: job.config.platform.clone(), - channel_id: job.config.channel.clone(), - thread_id: job.config.thread_id.clone(), - parent_id: None, - origin_event_id: None, - }; - let msg = format!("✅ Goal achieved: {}", job.config.message); - let _ = adapter.send_message(&channel, &msg).await; + if is_usercron + && job.config.disable_on_success.is_some() + && evaluate_disable_on_success(&job.config).await + { + info!( + schedule = %job.config.schedule, + channel = %job.config.channel, + "✅ disable_on_success: goal achieved, disabling job" + ); + // Write enabled=false back to usercron file + if let Some(ref path) = usercron_path { + if let Err(e) = disable_job_in_usercron(path, &job.config) { + error!(error = %e, "failed to disable job in usercron file"); } - // Trigger hot-reload on next tick - last_usercron_mtime = None; - continue; } + // Post success notification to channel + if let Some(adapter) = adapters.get(&job.config.platform) { + let channel = ChannelRef { + platform: job.config.platform.clone(), + channel_id: job.config.channel.clone(), + thread_id: job.config.thread_id.clone(), + parent_id: None, + origin_event_id: None, + }; + let msg = format!("✅ Goal achieved: {}", job.config.message); + let _ = adapter.send_message(&channel, &msg).await; + } + // Trigger hot-reload on next tick + last_usercron_mtime = None; + continue; } info!(