From 0c23d209a097dfc710f567eddb88f06205cbfb60 Mon Sep 17 00:00:00 2001 From: dave Date: Fri, 15 May 2026 00:53:54 +0000 Subject: [PATCH] huskies: merge 1077 --- server/src/service/notifications/format.rs | 91 ++++++++++++++++--- .../src/service/notifications/io/listener.rs | 12 +-- .../notifications/io/tests_notifications.rs | 83 +++++++++++++++++ 3 files changed, 166 insertions(+), 20 deletions(-) diff --git a/server/src/service/notifications/format.rs b/server/src/service/notifications/format.rs index 6219ba1e..5c4eb33f 100644 --- a/server/src/service/notifications/format.rs +++ b/server/src/service/notifications/format.rs @@ -240,21 +240,28 @@ pub fn format_new_item_notification( (plain, html) } -/// Extract the first non-empty line from a merge failure reason, truncated to `max_len` chars. +/// Maximum number of trailing gate-output lines included in a merge-failure +/// chat notification. /// -/// Used to produce a compact snippet for chat notifications. -pub fn merge_failure_snippet(reason: &str, max_len: usize) -> String { - let line = reason - .lines() - .find(|l| !l.trim().is_empty()) - .unwrap_or(reason); - let mut chars = line.chars(); - let truncated: String = chars.by_ref().take(max_len).collect(); - if chars.next().is_some() { - format!("{truncated}\u{2026}") // append … - } else { - truncated +/// Gate output can be hundreds of lines; only the tail (where errors appear) +/// is useful at a glance. Full output remains available via `get_merge_status` +/// or the web UI — this limit is chat-display-only. +pub const MERGE_FAILURE_TAIL_LINES: usize = 30; + +/// Truncate `gate_output` to its last `max_lines` lines for chat notifications. +/// +/// If the output contains more than `max_lines` non-empty lines, a leading +/// marker line `[...output truncated, last N lines shown...]` is prepended to +/// the tail so readers know output was cut. If the output fits within the +/// limit it is returned unchanged (no marker added). +pub fn truncate_gate_output(gate_output: &str, max_lines: usize) -> String { + let lines: Vec<&str> = gate_output.lines().collect(); + if lines.len() <= max_lines { + return gate_output.to_string(); } + let tail = &lines[lines.len() - max_lines..]; + let marker = format!("[...output truncated, last {max_lines} lines shown...]"); + format!("{marker}\n{}", tail.join("\n")) } #[cfg(test)] @@ -588,6 +595,64 @@ mod tests { assert_eq!(plain, "\u{1F916} #42 \u{2014} coder-1 started"); } + // ── truncate_gate_output ────────────────────────────────────────────────── + + #[test] + fn truncate_gate_output_short_output_returned_unchanged() { + let output = "line1\nline2\nline3"; + assert_eq!(truncate_gate_output(output, 30), output); + } + + #[test] + fn truncate_gate_output_exact_limit_returned_unchanged() { + let lines: Vec = (1..=30).map(|i| format!("line{i}")).collect(); + let output = lines.join("\n"); + assert_eq!(truncate_gate_output(&output, 30), output); + } + + #[test] + fn truncate_gate_output_over_limit_prepends_marker() { + let lines: Vec = (1..=35).map(|i| format!("line{i}")).collect(); + let output = lines.join("\n"); + let result = truncate_gate_output(&output, 30); + assert!( + result.starts_with("[...output truncated, last 30 lines shown...]"), + "must start with truncation marker; got: {result}" + ); + } + + #[test] + fn truncate_gate_output_over_limit_contains_tail_lines() { + let lines: Vec = (1..=35).map(|i| format!("line{i}")).collect(); + let output = lines.join("\n"); + let result = truncate_gate_output(&output, 30); + // Last 30 lines are line6..line35. + assert!(result.contains("line35"), "must contain last line"); + assert!(result.contains("line6"), "must contain first tail line"); + assert!(!result.contains("line5"), "must not contain dropped line"); + } + + #[test] + fn truncate_gate_output_empty_input_returned_unchanged() { + assert_eq!(truncate_gate_output("", 30), ""); + } + + #[test] + fn truncate_gate_output_single_line_returned_unchanged() { + assert_eq!(truncate_gate_output("only one line", 30), "only one line"); + } + + #[test] + fn truncate_gate_output_marker_contains_configured_limit() { + let lines: Vec = (1..=10).map(|i| format!("x{i}")).collect(); + let output = lines.join("\n"); + let result = truncate_gate_output(&output, 5); + assert!( + result.contains("last 5 lines shown"), + "marker must state configured limit; got: {result}" + ); + } + // ── format_agent_completed_notification ─────────────────────────────────── #[test] diff --git a/server/src/service/notifications/io/listener.rs b/server/src/service/notifications/io/listener.rs index f8752bbe..a3480b32 100644 --- a/server/src/service/notifications/io/listener.rs +++ b/server/src/service/notifications/io/listener.rs @@ -14,10 +14,10 @@ use tokio::sync::broadcast; use super::super::events::classify; use super::super::filter::{AGENT_EVENT_DEBOUNCE, should_send_rate_limit}; use super::super::format::{ - format_agent_completed_notification, format_agent_started_notification, - format_blocked_notification, format_error_notification, format_new_item_notification, - format_oauth_account_swapped, format_oauth_accounts_exhausted, format_rate_limit_notification, - merge_failure_snippet, + MERGE_FAILURE_TAIL_LINES, format_agent_completed_notification, + format_agent_started_notification, format_blocked_notification, format_error_notification, + format_new_item_notification, format_oauth_account_swapped, format_oauth_accounts_exhausted, + format_rate_limit_notification, truncate_gate_output, }; use super::super::route::rooms_for_notification; use super::{find_story_name_any_stage, read_story_name}; @@ -120,9 +120,7 @@ pub fn spawn_notification_listener( continue; }; let story_name = read_story_name(&project_root, "4_merge", story_id); - // AC3: include only the first non-empty line of the failure, - // truncated to ~120 chars. - let snippet = merge_failure_snippet(reason, 120); + let snippet = truncate_gate_output(reason, MERGE_FAILURE_TAIL_LINES); let (plain, html) = format_error_notification(story_id, &story_name, &snippet); slog!("[bot] Sending error notification: {plain}"); for room_id in &rooms_for_notification(&get_room_ids) { diff --git a/server/src/service/notifications/io/tests_notifications.rs b/server/src/service/notifications/io/tests_notifications.rs index 9df5a620..296ebbbc 100644 --- a/server/src/service/notifications/io/tests_notifications.rs +++ b/server/src/service/notifications/io/tests_notifications.rs @@ -5,6 +5,89 @@ use super::spawn_notification_listener; use crate::io::watcher::WatcherEvent; use tokio::sync::broadcast; +// ── spawn_notification_listener: MergeFailure ──────────────────────────────── + +/// Long gate output is truncated to the tail and includes the marker line. +#[tokio::test] +async fn merge_failure_long_output_is_truncated_to_tail() { + let tmp = tempfile::tempdir().unwrap(); + let (watcher_tx, watcher_rx) = broadcast::channel::(16); + let (transport, calls) = MockTransport::new(); + + spawn_notification_listener( + transport, + || vec!["!room1:example.org".to_string()], + watcher_rx, + tmp.path().to_path_buf(), + ); + + // Build a reason with 50 lines (more than MERGE_FAILURE_TAIL_LINES = 30). + let long_reason: String = (1..=50).map(|i| format!("gate-line-{i}\n")).collect(); + + watcher_tx + .send(WatcherEvent::MergeFailure { + story_id: "1077_story_trunc".to_string(), + reason: long_reason, + }) + .unwrap(); + + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + + let calls = calls.lock().unwrap(); + assert_eq!(calls.len(), 1, "Expected exactly one notification"); + let (_, plain, _) = &calls[0]; + assert!( + plain.contains("truncated"), + "notification must contain the truncation marker; got: {plain}" + ); + assert!( + plain.contains("gate-line-50"), + "notification must contain the last line; got: {plain}" + ); + assert!( + !plain.contains("gate-line-1\n"), + "notification must not contain the first (dropped) line; got: {plain}" + ); +} + +/// Short gate output (within limit) passes through unchanged, no marker added. +#[tokio::test] +async fn merge_failure_short_output_passes_through_unchanged() { + let tmp = tempfile::tempdir().unwrap(); + let (watcher_tx, watcher_rx) = broadcast::channel::(16); + let (transport, calls) = MockTransport::new(); + + spawn_notification_listener( + transport, + || vec!["!room1:example.org".to_string()], + watcher_rx, + tmp.path().to_path_buf(), + ); + + let short_reason = "error: type mismatch on line 42\nexpected i32, found &str".to_string(); + + watcher_tx + .send(WatcherEvent::MergeFailure { + story_id: "1077_story_short".to_string(), + reason: short_reason.clone(), + }) + .unwrap(); + + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + + let calls = calls.lock().unwrap(); + assert_eq!(calls.len(), 1, "Expected exactly one notification"); + let (_, plain, _) = &calls[0]; + assert!( + !plain.contains("truncated"), + "short output must not have a truncation marker; got: {plain}" + ); + assert!( + plain.contains("type mismatch"), + "short output must be included verbatim; got: {plain}" + ); +} + // ── spawn_notification_listener: RateLimitWarning ──────────────────────────── /// AC2 + AC3: when a RateLimitWarning event arrives, send_message is called