diff --git a/charts/openab/templates/configmap.yaml b/charts/openab/templates/configmap.yaml index ebbdb758..d9676775 100644 --- a/charts/openab/templates/configmap.yaml +++ b/charts/openab/templates/configmap.yaml @@ -162,6 +162,7 @@ data: [reactions] enabled = {{ if hasKey ($cfg.reactions) "enabled" }}{{ ($cfg.reactions).enabled }}{{ else }}true{{ end }} remove_after_reply = {{ if hasKey ($cfg.reactions) "removeAfterReply" }}{{ ($cfg.reactions).removeAfterReply }}{{ else }}false{{ end }} + empty_reply_placeholder = {{ if hasKey ($cfg.reactions) "emptyReplyPlaceholder" }}{{ ($cfg.reactions).emptyReplyPlaceholder }}{{ else }}true{{ end }} {{- if ($cfg.reactions).toolDisplay }} {{- if not (has ($cfg.reactions).toolDisplay (list "full" "compact" "none")) }} {{- fail (printf "agents.%s.reactions.toolDisplay must be one of: full, compact, none โ€” got: %s" $name ($cfg.reactions).toolDisplay) }} diff --git a/charts/openab/values.yaml b/charts/openab/values.yaml index 8e4f2fa9..156bb006 100644 --- a/charts/openab/values.yaml +++ b/charts/openab/values.yaml @@ -282,6 +282,8 @@ agents: # compact: show count summary (e.g. โœ… 3 ยท ๐Ÿ”ง 1 tool(s)) # full: show complete tool titles (for debugging) # none: hide tool lines entirely + # emptyReplyPlaceholder: true # set to false to silently suppress empty replies + # instead of posting "_(no response)_". Pairs with sentinel steering. stt: enabled: false apiKey: "" diff --git a/docs/steering/silence.md b/docs/steering/silence.md new file mode 100644 index 00000000..7bb826b0 --- /dev/null +++ b/docs/steering/silence.md @@ -0,0 +1,31 @@ +# Silence Steering + +## When to Stay Silent + +You may receive messages addressed to other agents, or batched messages where some +entries are not intended for you. In those cases, staying silent is the correct +behaviour. + +## How to Signal Silence + +**Output exactly `` and nothing else.** + +``` + +``` + +The gateway detects this sentinel and suppresses the message before posting to the +channel. No user-visible output is produced and no placeholder is left behind. + +**Do not** explain or justify staying silent. Do not write things like: +- "I'm staying silent because..." +- "This message wasn't addressed to me." +- "No response needed." + +Any text other than the exact sentinel string will be posted to the channel. + +## Sub-case: Multiple Bot Mentions + +When a message mentions several bots, read the full mention list before deciding +whether your UID is present. Do not infer "I should stay silent" solely because +other bot UIDs appear โ€” check whether your own UID is also in the list. diff --git a/src/adapter.rs b/src/adapter.rs index c8a2be45..cf7a6835 100644 --- a/src/adapter.rs +++ b/src/adapter.rs @@ -2,7 +2,7 @@ use anyhow::Result; use async_trait::async_trait; use serde::Serialize; use std::sync::Arc; -use tracing::{error, warn}; +use tracing::{debug, error, info, warn}; use crate::acp::{classify_notification, AcpEvent, ContentBlock, SessionPool}; use crate::config::{ReactionsConfig, ToolDisplay}; @@ -462,6 +462,7 @@ impl AdapterRouter { let streaming = adapter.use_streaming(other_bot_present); let table_mode = self.table_mode; let tool_display = self.reactions_config.tool_display; + let empty_reply_placeholder = self.reactions_config.empty_reply_placeholder; let prompt_hard_timeout = self.prompt_hard_timeout; let liveness_check_interval = self.liveness_check_interval; @@ -477,10 +478,14 @@ impl AdapterRouter { let mut text_buf = String::new(); let mut tool_lines: Vec = Vec::new(); - - if reset { - text_buf.push_str("โš ๏ธ _Session expired, starting fresh..._\n\n"); - } + // Kept separate from text_buf so the sentinel check ("is the + // agent's actual output ?") is not confused by the + // synthetic prelude. Prepended to final_content before send. + let reset_prelude = if reset { + "โš ๏ธ _Session expired, starting fresh..._\n\n" + } else { + "" + }; // Streaming edit: send placeholder, spawn edit loop let (buf_tx, placeholder_msg) = if streaming { @@ -540,6 +545,7 @@ impl AdapterRouter { _ = tokio::time::sleep(liveness_check_interval) => { if !conn.alive() { response_error = Some("Agent process died".into()); + warn!(platform = %adapter.platform(), "agent process died mid-prompt"); conn.abandon_request(request_id).await; break; } @@ -548,6 +554,11 @@ impl AdapterRouter { "Agent exceeded hard timeout ({}s)", prompt_hard_timeout.as_secs(), )); + warn!( + platform = %adapter.platform(), + elapsed_s = prompt_start.elapsed().as_secs(), + "agent hard timeout exceeded" + ); conn.abandon_request(request_id).await; break; } @@ -565,6 +576,12 @@ impl AdapterRouter { } if let Some(ref err) = notification.error { response_error = Some(format_coded_error(err.code, &err.message)); + warn!( + platform = %adapter.platform(), + code = err.code, + message = %err.message, + "agent JSON-RPC error" + ); } break; } @@ -573,13 +590,22 @@ impl AdapterRouter { match event { AcpEvent::Text(t) => { text_buf.push_str(&t); + // Don't stream potential-sentinel content to the edit + // loop โ€” if the agent is outputting the + // placeholder should stay as "โ€ฆ" until delete fires, + // not flash the literal sentinel text to users. if let Some(tx) = &buf_tx { - let _ = tx.send(compose_display( - &tool_lines, - &text_buf, - true, - tool_display, - )); + if text_buf.trim() != "" { + let _ = tx.send(format!( + "{reset_prelude}{}", + compose_display( + &tool_lines, + &text_buf, + true, + tool_display, + ) + )); + } } } AcpEvent::Thinking => { @@ -599,11 +625,14 @@ impl AdapterRouter { }); } if let Some(tx) = &buf_tx { - let _ = tx.send(compose_display( - &tool_lines, - &text_buf, - true, - tool_display, + let _ = tx.send(format!( + "{reset_prelude}{}", + compose_display( + &tool_lines, + &text_buf, + true, + tool_display, + ) )); } } @@ -627,11 +656,14 @@ impl AdapterRouter { }); } if let Some(tx) = &buf_tx { - let _ = tx.send(compose_display( - &tool_lines, - &text_buf, - true, - tool_display, + let _ = tx.send(format!( + "{reset_prelude}{}", + compose_display( + &tool_lines, + &text_buf, + true, + tool_display, + ) )); } } @@ -653,12 +685,39 @@ impl AdapterRouter { let (directives, stripped_text) = parse_output_directives(&text_buf); let text_buf = stripped_text; + // Sentinel: checked post-loop โ€” chunks may transiently match mid-stream. + if text_buf.trim() == "" { + info!(platform = %adapter.platform(), "agent emitted sentinel -- suppressing reply"); + reactions.suppress().await; + if let Some(msg) = placeholder_msg { + let a = adapter.clone(); + tokio::spawn(async move { + if let Err(e) = a.delete_message(&msg).await { + warn!(error = ?e, "delete placeholder failed after silent sentinel"); + } + }); + } + return Ok(()); + } + // Build final content let final_content = compose_display(&tool_lines, &text_buf, false, tool_display); let final_content = if final_content.is_empty() { if let Some(err) = response_error { format!("โš ๏ธ {err}") + } else if !empty_reply_placeholder { + debug!(platform = %adapter.platform(), "empty reply suppressed; empty_reply_placeholder disabled"); + reactions.suppress().await; + if let Some(msg) = placeholder_msg { + let a = adapter.clone(); + tokio::spawn(async move { + if let Err(e) = a.delete_message(&msg).await { + warn!(error = ?e, "delete placeholder failed after empty reply suppression"); + } + }); + } + return Ok(()); } else { "_(no response)_".to_string() } @@ -668,6 +727,7 @@ impl AdapterRouter { final_content }; + let final_content = format!("{reset_prelude}{final_content}"); let final_content = markdown::convert_tables(&final_content, table_mode); let chunks = format::split_message(&final_content, message_limit); if let Some(msg) = placeholder_msg { @@ -1195,4 +1255,26 @@ mod directive_tests { assert_eq!(directives.reply_to, Some("456".to_string())); assert_eq!(content, "็œ‹็œ‹ [[้€™ๅ€‹]] ๆ€Ž้บผๆจฃ"); } + + #[test] + fn silent_sentinel_passes_through_directive_parser() { + // is not a [[key:value]] directive โ€” parse_output_directives must + // return it unchanged so the post-loop sentinel check can match it. + let input = ""; + let (directives, content) = parse_output_directives(input); + assert_eq!(directives.reply_to, None); + assert_eq!(content, ""); + } + + #[test] + fn silent_sentinel_with_whitespace_passes_through() { + // Leading/trailing whitespace and newline variants must survive directive parsing. + for input in &[" ", "\n", "\n"] { + let (_, content) = parse_output_directives(input); + assert!( + content.trim() == "", + "expected trimmed content to equal '' for input {input:?}, got {content:?}" + ); + } + } } diff --git a/src/config.rs b/src/config.rs index 2187f0d7..d762c62b 100644 --- a/src/config.rs +++ b/src/config.rs @@ -408,6 +408,10 @@ pub struct ReactionsConfig { pub emojis: ReactionEmojis, #[serde(default)] pub timing: ReactionTiming, + /// When false, empty replies (no error) are silently suppressed instead of + /// posting "_(no response)_". Default true preserves existing behaviour. + #[serde(default = "default_true")] + pub empty_reply_placeholder: bool, } #[derive(Debug, Clone, Deserialize)] @@ -520,6 +524,7 @@ impl Default for ReactionsConfig { tool_display: ToolDisplay::default(), emojis: ReactionEmojis::default(), timing: ReactionTiming::default(), + empty_reply_placeholder: true, } } } @@ -922,4 +927,27 @@ echo_transcript = false assert!(cfg.stt.enabled); assert!(!cfg.stt.echo_transcript); } + + #[test] + fn empty_reply_placeholder_defaults_to_true() { + let toml = r#" +[agent] +command = "echo" +"#; + let cfg = parse_config(toml, "test").unwrap(); + assert!(cfg.reactions.empty_reply_placeholder, "default must be true for backward compat"); + } + + #[test] + fn empty_reply_placeholder_can_be_set_to_false() { + let toml = r#" +[agent] +command = "echo" + +[reactions] +empty_reply_placeholder = false +"#; + let cfg = parse_config(toml, "test").unwrap(); + assert!(!cfg.reactions.empty_reply_placeholder); + } } diff --git a/src/reactions.rs b/src/reactions.rs index 6e68f90b..ecde46b8 100644 --- a/src/reactions.rs +++ b/src/reactions.rs @@ -98,11 +98,15 @@ impl StatusReactionController { } let emoji = { self.inner.lock().await.emojis.done.clone() }; self.finish(&emoji).await; - // Add a random mood face + // Add a random mood face โ€” only if finish() actually ran (inner.current == emoji). + // If suppress() was called first, finished=true and current was cleared, so + // current != emoji and we skip the face to avoid reacting on a suppressed reply. let faces = ["๐Ÿ˜Š", "๐Ÿ˜Ž", "๐Ÿซก", "๐Ÿค“", "๐Ÿ˜", "โœŒ๏ธ", "๐Ÿ’ช", "๐Ÿฆพ"]; let face = faces[rand::random::() % faces.len()]; let inner = self.inner.lock().await; - let _ = inner.adapter.add_reaction(&inner.message, face).await; + if inner.current == emoji { + let _ = inner.adapter.add_reaction(&inner.message, face).await; + } } pub async fn set_error(&self) { @@ -118,6 +122,9 @@ impl StatusReactionController { return; } let mut inner = self.inner.lock().await; + if inner.finished { + return; + } cancel_timers(&mut inner); let current = inner.current.clone(); if !current.is_empty() { @@ -129,6 +136,26 @@ impl StatusReactionController { } } + /// Remove the current reaction, cancel all timers, and mark as finished so + /// subsequent set_done/set_error calls are no-ops. Used when a reply is + /// suppressed (sentinel or empty_reply_placeholder=false) โ€” prevents the + /// done emoji from appearing on a message the user never saw a reply for. + pub async fn suppress(&self) { + if !self.enabled { + return; + } + let mut inner = self.inner.lock().await; + inner.finished = true; + cancel_timers(&mut inner); + let current = inner.current.clone(); + if !current.is_empty() { + if let Err(e) = inner.adapter.remove_reaction(&inner.message, ¤t).await { + tracing::warn!(error = ?e, "suppress: failed to remove reaction"); + } + inner.current.clear(); + } + } + async fn apply_immediate(&self, emoji: &str) { let mut inner = self.inner.lock().await; if inner.finished || emoji == inner.current { @@ -274,3 +301,107 @@ fn cancel_timers(inner: &mut Inner) { h.abort(); } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::adapter::{ChannelRef, MessageRef}; + use async_trait::async_trait; + use std::sync::Arc; + use tokio::sync::Mutex; + + /// Minimal mock adapter that records add_reaction / remove_reaction calls. + struct MockAdapter { + calls: Arc>>, + } + + impl MockAdapter { + fn new() -> (Arc, Arc>>) { + let calls = Arc::new(Mutex::new(Vec::new())); + (Arc::new(Self { calls: calls.clone() }), calls) + } + } + + #[async_trait] + impl ChatAdapter for MockAdapter { + fn platform(&self) -> &'static str { "mock" } + fn message_limit(&self) -> usize { 2000 } + async fn send_message(&self, _ch: &ChannelRef, _content: &str) -> anyhow::Result { + unimplemented!() + } + async fn create_thread(&self, _ch: &ChannelRef, _trigger: &MessageRef, _title: &str) -> anyhow::Result { + unimplemented!() + } + async fn add_reaction(&self, _msg: &MessageRef, emoji: &str) -> anyhow::Result<()> { + self.calls.lock().await.push(format!("add:{emoji}")); + Ok(()) + } + async fn remove_reaction(&self, _msg: &MessageRef, emoji: &str) -> anyhow::Result<()> { + self.calls.lock().await.push(format!("remove:{emoji}")); + Ok(()) + } + fn use_streaming(&self, _other_bot_present: bool) -> bool { false } + } + + fn make_message_ref() -> MessageRef { + MessageRef { + channel: ChannelRef { + platform: "mock".into(), + channel_id: "C1".into(), + thread_id: None, + parent_id: None, + origin_event_id: None, + }, + message_id: "M1".into(), + } + } + + fn make_controller(adapter: Arc) -> StatusReactionController { + StatusReactionController::new( + true, + adapter, + make_message_ref(), + ReactionEmojis::default(), + ReactionTiming::default(), + ) + } + + #[tokio::test] + async fn suppress_removes_current_reaction_and_blocks_set_done() { + let (adapter, calls) = MockAdapter::new(); + let ctrl = make_controller(adapter); + + // set_queued uses apply_immediate (no debounce) so the reaction is + // present synchronously before suppress() is called. + ctrl.set_queued().await; + + ctrl.suppress().await; + + // suppress() must remove the current reaction + let snapshot = calls.lock().await.clone(); + let removed = snapshot.iter().any(|c| c.starts_with("remove:")); + assert!(removed, "suppress() should remove the current reaction; calls: {snapshot:?}"); + + // set_done() must be a no-op after suppress() + ctrl.set_done().await; + let after_done = calls.lock().await.clone(); + let new_adds: Vec<_> = after_done[snapshot.len()..].iter().filter(|c| c.starts_with("add:")).collect(); + assert!(new_adds.is_empty(), "set_done() must be no-op after suppress(); new add calls: {new_adds:?}"); + } + + #[tokio::test] + async fn clear_is_no_op_after_suppress() { + let (adapter, calls) = MockAdapter::new(); + let ctrl = make_controller(adapter); + + ctrl.set_queued().await; + + ctrl.suppress().await; + let after_suppress = calls.lock().await.len(); + + // clear() must not fire additional API calls after suppress() + ctrl.clear().await; + let after_clear = calls.lock().await.len(); + assert_eq!(after_suppress, after_clear, "clear() must be no-op after suppress()"); + } +} diff --git a/src/slack.rs b/src/slack.rs index 47d4c42d..32ba8e1b 100644 --- a/src/slack.rs +++ b/src/slack.rs @@ -447,6 +447,18 @@ impl ChatAdapter for SlackAdapter { } } + async fn delete_message(&self, msg: &MessageRef) -> Result<()> { + self.api_post( + "chat.delete", + serde_json::json!({ + "channel": msg.channel.channel_id, + "ts": msg.message_id, + }), + ) + .await?; + Ok(()) + } + async fn edit_message(&self, msg: &MessageRef, content: &str) -> Result<()> { let mrkdwn = markdown_to_mrkdwn(content); self.api_post(