diff --git a/src/agent/routine_engine.rs b/src/agent/routine_engine.rs index 64c3b94c..d390094e 100644 --- a/src/agent/routine_engine.rs +++ b/src/agent/routine_engine.rs @@ -715,6 +715,7 @@ impl RoutineEngine { status, Some(summary), thread_id.as_deref(), + run.job_id, ) .await; @@ -1218,6 +1219,7 @@ async fn execute_routine(ctx: EngineContext, routine: Routine, run: RoutineRun) status, summary.as_deref(), thread_id.as_deref(), + run.job_id, ) .await; } @@ -1827,7 +1829,18 @@ async fn execute_routine_tool( Ok(result_str) } +/// Human-readable label for a run status, suitable for user-facing notifications. +fn status_display_label(status: RunStatus) -> &'static str { + match status { + RunStatus::Ok => "Completed", + RunStatus::Attention => "Needs attention", + RunStatus::Failed => "Failed", + RunStatus::Running => "Running", + } +} + /// Send a notification based on the routine's notify config and run status. +#[allow(clippy::too_many_arguments)] async fn send_notification( tx: &mpsc::Sender, notify: &NotifyConfig, @@ -1836,6 +1849,7 @@ async fn send_notification( status: RunStatus, summary: Option<&str>, thread_id: Option<&str>, + job_id: Option, ) { let should_notify = match status { RunStatus::Ok => notify.on_success, @@ -1855,23 +1869,34 @@ async fn send_notification( RunStatus::Running => "⏳", }; + let label = status_display_label(status); + let message = match summary { - Some(s) => format!("{} *Routine '{}'*: {}\n\n{}", icon, routine_name, status, s), - None => format!("{} *Routine '{}'*: {}", icon, routine_name, status), + Some(s) => { + let sanitized = sanitize_summary(s); + format!("{} *Routine '{}'*: {}\n\n{}", icon, routine_name, label, sanitized) + } + None => format!("{} *Routine '{}'*: {}", icon, routine_name, label), }; + let mut metadata = serde_json::json!({ + "source": "routine", + "routine_name": routine_name, + "status": status.to_string(), + "owner_id": owner_id, + "notify_user": notify.user, + "notify_channel": notify.channel, + }); + + if let Some(jid) = job_id { + metadata["job_id"] = serde_json::json!(jid.to_string()); + } + let response = OutgoingResponse { content: message, thread_id: thread_id.map(String::from), attachments: Vec::new(), - metadata: serde_json::json!({ - "source": "routine", - "routine_name": routine_name, - "status": status.to_string(), - "owner_id": owner_id, - "notify_user": notify.user, - "notify_channel": notify.channel, - }), + metadata, }; if let Err(e) = tx.send(response).await { @@ -1934,7 +1959,6 @@ fn truncate(s: &str, max: usize) -> String { /// 2. Strip HTML tags to prevent injection in web-rendered notifications /// 3. Collapse multiple whitespace/newlines to single spaces for cleaner output /// 4. Truncate to 500 chars to prevent oversized notifications -#[cfg(test)] fn sanitize_summary(s: &str) -> String { // Strip control characters (keep newline for now, collapse later) let no_control: String = s @@ -1962,7 +1986,6 @@ fn sanitize_summary(s: &str) -> String { } /// Remove HTML/XML tags from a string. -#[cfg(test)] fn strip_html_tags(s: &str) -> String { let mut result = String::with_capacity(s.len()); let mut in_tag = false; @@ -2558,4 +2581,162 @@ mod tests { assert!(result.len() <= 503); assert!(result.ends_with("...")); } + + #[test] + fn test_sanitize_summary_truncates_long_text() { + use super::sanitize_summary; + + let short = "This is a short summary."; + assert_eq!(sanitize_summary(short), short); + + let long = "x".repeat(600); + let result = sanitize_summary(&long); + assert!( + result.len() <= 503, + "Truncated summary should be at most 503 bytes (500 + '...')" + ); + assert!(result.ends_with("..."), "Truncated summary should end with ellipsis"); + } + + #[test] + fn test_status_display_label_readable() { + use super::status_display_label; + + assert_eq!(status_display_label(RunStatus::Ok), "Completed"); + assert_eq!(status_display_label(RunStatus::Failed), "Failed"); + assert_eq!(status_display_label(RunStatus::Attention), "Needs attention"); + assert_eq!(status_display_label(RunStatus::Running), "Running"); + } + + #[tokio::test] + async fn test_notification_message_uses_readable_status() { + use tokio::sync::mpsc; + + let (tx, mut rx) = mpsc::channel(1); + let notify = NotifyConfig { + on_success: true, + on_failure: true, + on_attention: true, + ..Default::default() + }; + + super::send_notification( + &tx, + ¬ify, + "user-1", + "my-routine", + RunStatus::Ok, + Some("All good"), + None, + None, + ) + .await; + + let msg = rx.recv().await.expect("should receive notification"); + assert!( + msg.content.contains("Completed"), + "Notification should use readable label 'Completed', got: {}", + msg.content + ); + assert!( + !msg.content.contains(": ok"), + "Notification should not contain raw lowercase status" + ); + } + + #[tokio::test] + async fn test_notification_includes_job_id_in_metadata() { + use tokio::sync::mpsc; + + let (tx, mut rx) = mpsc::channel(1); + let notify = NotifyConfig { + on_failure: true, + ..Default::default() + }; + let job_id = uuid::Uuid::new_v4(); + + super::send_notification( + &tx, + ¬ify, + "user-1", + "my-routine", + RunStatus::Failed, + Some("something broke"), + None, + Some(job_id), + ) + .await; + + let msg = rx.recv().await.expect("should receive notification"); + let meta_job_id = msg.metadata["job_id"] + .as_str() + .expect("metadata should contain job_id"); + assert_eq!(meta_job_id, job_id.to_string()); + } + + #[tokio::test] + async fn test_notification_omits_job_id_when_none() { + use tokio::sync::mpsc; + + let (tx, mut rx) = mpsc::channel(1); + let notify = NotifyConfig { + on_success: true, + ..Default::default() + }; + + super::send_notification( + &tx, + ¬ify, + "user-1", + "my-routine", + RunStatus::Ok, + Some("done"), + None, + None, + ) + .await; + + let msg = rx.recv().await.expect("should receive notification"); + assert!( + msg.metadata.get("job_id").is_none(), + "metadata should not contain job_id when None" + ); + } + + #[tokio::test] + async fn test_notification_truncates_long_summary() { + use tokio::sync::mpsc; + + let (tx, mut rx) = mpsc::channel(1); + let notify = NotifyConfig { + on_failure: true, + ..Default::default() + }; + + let long_summary = "z".repeat(1000); + super::send_notification( + &tx, + ¬ify, + "user-1", + "my-routine", + RunStatus::Failed, + Some(&long_summary), + None, + None, + ) + .await; + + let msg = rx.recv().await.expect("should receive notification"); + // The sanitized summary should be truncated to ~500 chars + "..." + // The full message includes icon + routine name + label, so just check + // it doesn't contain the full 1000-char string. + assert!( + !msg.content.contains(&long_summary), + "Notification should truncate long summaries" + ); + assert!( + msg.content.contains("..."), + "Truncated notification should contain ellipsis" + ); + } }