diff --git a/src/openhuman/cron/scheduler.rs b/src/openhuman/cron/scheduler.rs index 3856d32a1..6dac36d14 100644 --- a/src/openhuman/cron/scheduler.rs +++ b/src/openhuman/cron/scheduler.rs @@ -260,9 +260,11 @@ pub(crate) async fn tick_once( /// Public entry point for delivering a job's output via the configured /// delivery mode (proactive / announce). Called by `cron_run` ("Run Now") -/// so manual runs also push notifications and alerts. +/// so manual runs also push notifications and alerts. Manual runs are treated +/// as `success = true` so the user always sees the result they explicitly +/// triggered (empty output is still skipped). pub async fn deliver_job(config: &Config, job: &CronJob, output: &str) { - if let Err(e) = deliver_if_configured(config, job, output).await { + if let Err(e) = deliver_if_configured(config, job, output, true).await { if job.delivery.best_effort { tracing::warn!("[cron] delivery failed (best_effort, Run Now): {e}"); } else { @@ -618,7 +620,7 @@ async fn run_agent_job(config: &Config, job: &CronJob) -> (bool, String, Option< Ok(response) => ( true, if response.trim().is_empty() { - "agent job executed".to_string() + EMPTY_AGENT_OUTPUT.to_string() } else { response }, @@ -638,6 +640,10 @@ async fn run_agent_job(config: &Config, job: &CronJob) -> (bool, String, Option< } } +/// Placeholder recorded in run history when an agent job succeeds but returns +/// no text. Never delivered to chat — used only for the run-history record. +const EMPTY_AGENT_OUTPUT: &str = "agent job executed"; + fn build_agent_for_cron_job(config: &Config, job: &CronJob) -> anyhow::Result { if let Some(agent_id) = job.agent_id.as_deref() { match Agent::from_config_for_agent(config, agent_id) { @@ -674,7 +680,7 @@ async fn persist_job_result( ) -> bool { let duration_ms = (finished_at - started_at).num_milliseconds(); - if let Err(e) = deliver_if_configured(config, job, output).await { + if let Err(e) = deliver_if_configured(config, job, output, success).await { if job.delivery.best_effort { tracing::warn!("Cron delivery failed (best_effort): {e}"); } else { @@ -752,57 +758,116 @@ fn warn_if_high_frequency_agent_job(job: &CronJob) { } } -async fn deliver_if_configured(config: &Config, job: &CronJob, output: &str) -> Result<()> { +/// True when an agent job produced no meaningful text — blank output or the +/// [`EMPTY_AGENT_OUTPUT`] placeholder. Such runs are never injected into chat. +fn cron_output_is_empty(output: &str) -> bool { + output.trim().is_empty() || output == EMPTY_AGENT_OUTPUT +} + +/// Whether a cron job's output should be injected into the user's chat thread. +/// Skips failed runs and empty/placeholder output; failures still surface in +/// the alerts tab and run history (handled separately by the caller). +fn should_deliver_cron_output_to_chat(success: bool, output: &str) -> bool { + success && !cron_output_is_empty(output) +} + +/// Whether a completed cron run should surface in the alerts tab +/// (`/notifications`). Failures stay visible even when they produce no output; +/// only successful-but-empty runs are dropped entirely. +fn cron_result_should_alert(success: bool, output: &str) -> bool { + !success || !cron_output_is_empty(output) +} + +async fn deliver_if_configured( + config: &Config, + job: &CronJob, + output: &str, + success: bool, +) -> Result<()> { let delivery: &DeliveryConfig = &job.delivery; + // Don't post failed or empty cron runs into the user's chat: a failed turn + // (e.g. a transient network error) would otherwise deliver a canned + // "Something went wrong" message into the conversation with no user + // message behind it. Failures still reach the alerts tab (`push_cron_alert`) + // and the run-history / health signals, which are recorded elsewhere. + let is_empty = cron_output_is_empty(output); + let deliver_to_chat = should_deliver_cron_output_to_chat(success, output); + if !deliver_to_chat { + tracing::debug!( + job_id = %job.id, + success, + is_empty, + "[cron] skipping chat delivery for failed/empty cron run" + ); + } + + // Failures must stay visible in /notifications even when they produce no + // output; only successful-but-empty runs are suppressed entirely. + let alert_to_notifications = cron_result_should_alert(success, output); + let alert_body = if is_empty { + "Scheduled job failed without output." + } else { + output + }; + let mode = delivery.mode.trim().to_ascii_lowercase(); match mode.as_str() { // Proactive delivery — the channels module decides where to send. // Used by morning briefings, welcome messages, and other // user-facing proactive agents. "proactive" => { - let source = format!("cron:{}", job.id); - tracing::debug!( - job_id = %job.id, - source = %source, - "[cron] publishing ProactiveMessageRequested event" - ); - publish_global(DomainEvent::ProactiveMessageRequested { - source, - message: output.to_string(), - job_name: job.name.clone(), - }); + if deliver_to_chat { + let source = format!("cron:{}", job.id); + tracing::debug!( + job_id = %job.id, + source = %source, + "[cron] publishing ProactiveMessageRequested event" + ); + publish_global(DomainEvent::ProactiveMessageRequested { + source, + message: output.to_string(), + job_name: job.name.clone(), + }); + } - // Also push to the alerts tab so the user sees it in /notifications. - push_cron_alert(config, job, output); + // Surface in the alerts tab (/notifications) for any result that + // isn't a successful-but-empty run, so failed scheduled jobs stay + // visible even though they aren't injected into chat. + if alert_to_notifications { + push_cron_alert(config, job, alert_body); + } } // Announce delivery — the cron job specifies the exact channel // and target. Used for explicit channel-targeted output. "announce" => { - let channel = delivery - .channel - .as_deref() - .ok_or_else(|| anyhow::anyhow!("delivery.channel is required for announce mode"))?; - let target = delivery - .to - .as_deref() - .ok_or_else(|| anyhow::anyhow!("delivery.to is required for announce mode"))?; + if deliver_to_chat { + let channel = delivery.channel.as_deref().ok_or_else(|| { + anyhow::anyhow!("delivery.channel is required for announce mode") + })?; + let target = delivery + .to + .as_deref() + .ok_or_else(|| anyhow::anyhow!("delivery.to is required for announce mode"))?; - tracing::debug!( - job_id = %job.id, - channel = %channel, - target = %target, - "[cron] publishing CronDeliveryRequested event" - ); - publish_global(DomainEvent::CronDeliveryRequested { - job_id: job.id.clone(), - channel: channel.to_string(), - target: target.to_string(), - output: output.to_string(), - }); + tracing::debug!( + job_id = %job.id, + channel = %channel, + target = %target, + "[cron] publishing CronDeliveryRequested event" + ); + publish_global(DomainEvent::CronDeliveryRequested { + job_id: job.id.clone(), + channel: channel.to_string(), + target: target.to_string(), + output: output.to_string(), + }); + } - push_cron_alert(config, job, output); + if alert_to_notifications { + push_cron_alert(config, job, alert_body); + } } // No delivery configured — output is stored in last_output only. diff --git a/src/openhuman/cron/scheduler_tests.rs b/src/openhuman/cron/scheduler_tests.rs index 3a1adfec1..ee2a779f3 100644 --- a/src/openhuman/cron/scheduler_tests.rs +++ b/src/openhuman/cron/scheduler_tests.rs @@ -568,7 +568,9 @@ async fn deliver_if_configured_skips_non_announce_mode() { let job = test_job("echo ok"); // Default delivery mode is not "announce", so nothing is published. - assert!(deliver_if_configured(&config, &job, "x").await.is_ok()); + assert!(deliver_if_configured(&config, &job, "x", true) + .await + .is_ok()); } #[tokio::test] @@ -624,7 +626,9 @@ async fn deliver_if_configured_publishes_event_for_announce_mode() { assert_eq!(received.load(Ordering::SeqCst), 1); // Also verify the function itself succeeds. - assert!(deliver_if_configured(&config, &job, "hello").await.is_ok()); + assert!(deliver_if_configured(&config, &job, "hello", true) + .await + .is_ok()); } #[test] @@ -720,7 +724,9 @@ async fn deliver_if_configured_skips_empty_mode() { let config = test_config(&tmp).await; let mut job = test_job("echo ok"); job.delivery.mode = "".into(); - assert!(deliver_if_configured(&config, &job, "output").await.is_ok()); + assert!(deliver_if_configured(&config, &job, "output", true) + .await + .is_ok()); } #[tokio::test] @@ -734,7 +740,7 @@ async fn deliver_if_configured_announce_missing_channel_errors() { to: Some("target".into()), best_effort: true, }; - let result = deliver_if_configured(&config, &job, "out").await; + let result = deliver_if_configured(&config, &job, "out", true).await; assert!(result.is_err()); } @@ -749,7 +755,7 @@ async fn deliver_if_configured_announce_missing_target_errors() { to: None, best_effort: true, }; - let result = deliver_if_configured(&config, &job, "out").await; + let result = deliver_if_configured(&config, &job, "out", true).await; assert!(result.is_err()); } @@ -764,7 +770,9 @@ async fn deliver_if_configured_proactive_mode_succeeds() { to: None, best_effort: true, }; - assert!(deliver_if_configured(&config, &job, "hello").await.is_ok()); + assert!(deliver_if_configured(&config, &job, "hello", true) + .await + .is_ok()); } // ────────────────────────────────────────────────────────────────────── @@ -1210,3 +1218,109 @@ async fn scheduler_tick_once_does_not_re_emit_recovery_signal_on_steady_state() ); } } + +// ── Chat-delivery gating (skip failed + empty cron runs) ──────────────────── + +#[test] +fn chat_delivery_skipped_for_failed_runs() { + // A failed cron turn (e.g. a transient network/DNS error) yields a + // non-empty canned message; it must NOT be injected into the chat thread. + assert!(!should_deliver_cron_output_to_chat( + false, + "Something went wrong. Please try again." + )); +} + +#[test] +fn chat_delivery_skipped_for_empty_runs() { + assert!(!should_deliver_cron_output_to_chat(true, "")); + assert!(!should_deliver_cron_output_to_chat(true, " \n ")); + // The empty-run placeholder counts as empty and is not delivered. + assert!(cron_output_is_empty(EMPTY_AGENT_OUTPUT)); + assert!(!should_deliver_cron_output_to_chat( + true, + EMPTY_AGENT_OUTPUT + )); +} + +#[test] +fn chat_delivery_allowed_for_successful_nonempty_runs() { + assert!(!cron_output_is_empty( + "Good morning! You have 3 meetings today." + )); + assert!(should_deliver_cron_output_to_chat( + true, + "Good morning! You have 3 meetings today." + )); +} + +#[test] +fn failed_runs_still_alert_even_when_empty() { + // Failures must remain visible in /notifications even with no output. + assert!(cron_result_should_alert(false, "")); + assert!(cron_result_should_alert(false, EMPTY_AGENT_OUTPUT)); + assert!(cron_result_should_alert( + false, + "Something went wrong. Please try again." + )); + // Successful non-empty runs alert; successful-but-empty runs do not. + assert!(cron_result_should_alert(true, "done")); + assert!(!cron_result_should_alert(true, "")); + assert!(!cron_result_should_alert(true, EMPTY_AGENT_OUTPUT)); +} + +fn proactive_job() -> CronJob { + let mut job = test_job(""); + job.delivery = DeliveryConfig { + mode: "proactive".into(), + channel: None, + to: None, + best_effort: true, + }; + job +} + +async fn cron_alerts(config: &Config) -> usize { + crate::openhuman::notifications::store::list(config, 10, 0, Some("cron"), None) + .unwrap() + .len() +} + +#[tokio::test] +async fn deliver_if_configured_failure_skips_chat_but_alerts() { + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp).await; + let job = proactive_job(); + // Failed run (non-empty canned error): no chat injection, but still alerts. + assert!( + deliver_if_configured(&config, &job, "Something went wrong.", false) + .await + .is_ok() + ); + assert_eq!(cron_alerts(&config).await, 1); +} + +#[tokio::test] +async fn deliver_if_configured_empty_failure_alerts_with_fallback_body() { + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp).await; + let job = proactive_job(); + // Empty failed run: still surfaces in /notifications with a fallback body. + assert!(deliver_if_configured(&config, &job, "", false) + .await + .is_ok()); + let items = + crate::openhuman::notifications::store::list(&config, 10, 0, Some("cron"), None).unwrap(); + assert_eq!(items.len(), 1); + assert!(items[0].body.contains("failed without output")); +} + +#[tokio::test] +async fn deliver_if_configured_empty_success_skips_chat_and_alert() { + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp).await; + let job = proactive_job(); + // Successful but empty: nothing delivered anywhere. + assert!(deliver_if_configured(&config, &job, "", true).await.is_ok()); + assert_eq!(cron_alerts(&config).await, 0); +}