fix(cron): skip chat delivery for failed or empty scheduled jobs (#3788) (#3789)

Co-authored-by: Claude <noreply@anthropic.com>

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<Agent> {
     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.

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);
+}