From c94ecf19db59f8c70c488f0a761c5f4cbf674a1a Mon Sep 17 00:00:00 2001 From: Pierre LE GUEN <26087574+PierreLeGuen@users.noreply.github.com> Date: Thu, 12 Mar 2026 11:38:53 -0700 Subject: [PATCH] feat: add Slack approval buttons for tool execution in DMs (#796) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: add channel-relay integration for Slack via external relay service - Add RelayChannel and RelayClient for connecting to channel-relay SSE streams - Add RelayConfig with env-based configuration (CHANNEL_RELAY_URL, CHANNEL_RELAY_API_KEY) - Add channel-relay extension lifecycle: install, OAuth auth, activate with hot-add - Add proxy message sending through channel-relay for Slack chat.postMessage - Add extension registry entry for Slack relay with OAuth auth hint - Add relay integration test with mock SSE server - Wire relay channel into app startup with reconnect on stored credentials - Add AuthRequired extension error variant for cleaner auth flow detection [skip-regression-check] * chore: apply cargo fmt * fix: remove remaining Telegram test references in relay channel * fix: address PR #790 review feedback — parser handle leak, CSRF, circuit breaker - Fix parser handle leak on reconnect by sharing Arc instead of creating a local copy in start() (shutdown now aborts the correct task) - Add CSRF state nonce to OAuth flow: generate in auth_channel_relay, validate in slack_relay_oauth_callback_handler, one-time use - Remove dead proxy_slack method, update integration test to use proxy_provider - Add reconnect circuit breaker (max_consecutive_failures, default 50) - Fix stale docs (Telegram refs), extract event_types constants * fix: double backoff in reconnect loop and UTF-8 chunk-boundary corruption - Remove second sleep+backoff in list_connections error branch to prevent O(4^n) backoff growth (was sleeping and doubling twice per iteration) - Buffer raw bytes in SSE parser instead of per-chunk String::from_utf8_lossy to prevent U+FFFD corruption when multi-byte chars span chunk boundaries * feat: add Slack approval buttons for tool execution in DMs Send Block Kit Approve/Deny buttons via relay when a tool requires approval in a DM context. Auto-deny approval-requiring tools in shared channels to prevent prompt injection and stuck threads. * fix: address PR #796 review — use PreflightOutcome::Rejected, add tests - Auto-deny in non-DM relay channels now uses PreflightOutcome::Rejected instead of manually pushing to reason_ctx.messages, so the post-flight handler properly records the error in the turn - Add regression tests for relay auto-deny decision logic - Remove test_clean.db artifact * feat: restore Block Kit approval buttons in send_status The send_status implementation was accidentally dropped during the staging merge. Restores Approve/Deny Block Kit buttons for DM tool approval, with required sender_id validation, payload size docs, and 4 regression tests. Also removes test_clean.db. Co-Authored-By: Claude Opus 4.6 * fix: apply rustfmt formatting to dispatcher test code Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Opus 4.6 --- src/agent/dispatcher.rs | 72 +++++++++++ src/channels/relay/channel.rs | 228 +++++++++++++++++++++++++++++++++- 2 files changed, 297 insertions(+), 3 deletions(-) diff --git a/src/agent/dispatcher.rs b/src/agent/dispatcher.rs index b791f6d7..d0ae98ad 100644 --- a/src/agent/dispatcher.rs +++ b/src/agent/dispatcher.rs @@ -554,6 +554,31 @@ impl<'a> LoopDelegate for ChatDelegate<'a> { }; if needs_approval { + // In non-DM relay channels, auto-deny approval- + // requiring tools to prevent stuck AwaitingApproval + // state and prompt injection from other users. + let is_relay = self.message.channel.ends_with("-relay"); + let is_dm = self + .message + .metadata + .get("event_type") + .and_then(|v| v.as_str()) + == Some("direct_message"); + if is_relay && !is_dm { + tracing::info!( + tool = %tc.name, + channel = %self.message.channel, + "Auto-denying approval-requiring tool in non-DM relay channel" + ); + let reject_msg = format!( + "Tool '{}' requires approval and cannot run in shared channels. \ + Ask the user to message me directly (DM) to use this tool.", + tc.name + ); + preflight.push((tc, PreflightOutcome::Rejected(reject_msg))); + continue; + } + approval_needed = Some((idx, tc, tool)); break; } @@ -2235,4 +2260,51 @@ mod tests { "Present 'data' field should produce non-empty string" ); } + + /// Test the relay channel auto-deny decision logic: + /// approval-requiring tools in non-DM relay channels must be rejected. + #[test] + fn test_relay_non_dm_auto_deny_decision() { + use crate::channels::IncomingMessage; + + // Case 1: relay channel + non-DM → should auto-deny + let msg = IncomingMessage::new("slack-relay", "u1", "hello") + .with_metadata(serde_json::json!({ "event_type": "message" })); + let is_relay = msg.channel.ends_with("-relay"); + let is_dm = + msg.metadata.get("event_type").and_then(|v| v.as_str()) == Some("direct_message"); + assert!(is_relay && !is_dm, "Should auto-deny in relay non-DM"); + + // Case 2: relay channel + DM → should NOT auto-deny + let msg_dm = IncomingMessage::new("slack-relay", "u1", "hello") + .with_metadata(serde_json::json!({ "event_type": "direct_message" })); + let is_dm_2 = + msg_dm.metadata.get("event_type").and_then(|v| v.as_str()) == Some("direct_message"); + assert!( + !msg_dm.channel.ends_with("-relay") || is_dm_2, + "Should NOT auto-deny in relay DM" + ); + + // Case 3: non-relay channel → should NOT auto-deny + let msg_web = IncomingMessage::new("web", "u1", "hello") + .with_metadata(serde_json::json!({ "event_type": "message" })); + assert!( + !msg_web.channel.ends_with("-relay"), + "Non-relay channel should not trigger auto-deny" + ); + } + + /// Test that the auto-deny produces a PreflightOutcome::Rejected-style message. + #[test] + fn test_relay_auto_deny_message_format() { + let tool_name = "shell"; + let result_msg = format!( + "Tool '{}' requires approval and cannot run in shared channels. \ + Ask the user to message me directly (DM) to use this tool.", + tool_name + ); + assert!(result_msg.contains("shell")); + assert!(result_msg.contains("approval")); + assert!(result_msg.contains("DM")); + } } diff --git a/src/channels/relay/channel.rs b/src/channels/relay/channel.rs index cb64e882..d6aa90cc 100644 --- a/src/channels/relay/channel.rs +++ b/src/channels/relay/channel.rs @@ -408,12 +408,120 @@ impl Channel for RelayChannel { Ok(()) } - /// Status updates are not forwarded to messaging providers to avoid noise. async fn send_status( &self, - _status: StatusUpdate, - _metadata: &serde_json::Value, + status: StatusUpdate, + metadata: &serde_json::Value, ) -> Result<(), ChannelError> { + // Only handle ApprovalNeeded — all other variants are no-ops + let StatusUpdate::ApprovalNeeded { + request_id, + tool_name, + description, + parameters, + } = status + else { + return Ok(()); + }; + + // Only send buttons in DMs (dispatcher gates upstream, but guard here too) + let event_type = metadata + .get("event_type") + .and_then(|v| v.as_str()) + .unwrap_or(""); + if event_type != "direct_message" { + tracing::warn!( + tool = %tool_name, + event_type, + "Approval requested in non-DM, skipping buttons" + ); + return Ok(()); + } + + // Extract required metadata — error if missing + let channel_id = metadata + .get("channel_id") + .and_then(|v| v.as_str()) + .ok_or_else(|| ChannelError::SendFailed { + name: self.name().to_string(), + reason: "Missing channel_id for approval buttons".into(), + })?; + let sender_id = metadata + .get("sender_id") + .and_then(|v| v.as_str()) + .ok_or_else(|| ChannelError::SendFailed { + name: self.name().to_string(), + reason: "Missing sender_id for approval buttons".into(), + })?; + let thread_id = metadata.get("thread_id").and_then(|v| v.as_str()); + let team_id = metadata + .get("team_id") + .and_then(|v| v.as_str()) + .unwrap_or(&self.team_id); + + // Button value payload (Slack limits button values to 2000 chars; + // safe with typical UUIDs but documented here as a constraint) + let value_payload = serde_json::json!({ + "instance_id": self.instance_id, + "team_id": team_id, + "channel_id": channel_id, + "thread_ts": thread_id, + "request_id": request_id, + "sender_id": sender_id, + }); + let value_str = value_payload.to_string(); + + // Parameters are already redacted via redact_params() in dispatcher.rs + let params_display = + serde_json::to_string_pretty(¶meters).unwrap_or_else(|_| parameters.to_string()); + + let blocks = serde_json::json!([ + { + "type": "section", + "text": { + "type": "mrkdwn", + "text": format!( + "*Tool approval required*\n`{tool_name}`: {description}\n```{params_display}```" + ) + } + }, + { + "type": "actions", + "elements": [ + { + "type": "button", + "text": { "type": "plain_text", "text": "Approve" }, + "style": "primary", + "action_id": "approve_tool", + "value": value_str, + }, + { + "type": "button", + "text": { "type": "plain_text", "text": "Deny" }, + "style": "danger", + "action_id": "deny_tool", + "value": value_str, + } + ] + } + ]); + + let mut body = serde_json::json!({ + "channel": channel_id, + "text": format!("Tool approval required: {tool_name} - {description}"), + "blocks": blocks, + }); + if let Some(tid) = thread_id { + body["thread_ts"] = serde_json::Value::String(tid.to_string()); + } + + self.proxy_send(team_id, "chat.postMessage", body) + .await + .map_err(|e| ChannelError::SendFailed { + name: self.name().to_string(), + reason: e.to_string(), + })?; + Ok(()) } @@ -639,4 +747,118 @@ mod tests { // The reconnect loop now skips team validation when team_id is empty, // so the channel remains alive. } + + #[tokio::test] + async fn test_send_status_non_approval_is_noop() { + let channel = RelayChannel::new( + test_client(), + "token".into(), + "T123".into(), + "inst1".into(), + "user1".into(), + ); + let metadata = serde_json::json!({}); + let result = channel + .send_status( + StatusUpdate::ToolStarted { + name: "echo".into(), + }, + &metadata, + ) + .await; + assert!(result.is_ok()); + } + + #[tokio::test] + async fn test_send_status_approval_non_dm_skips() { + let channel = RelayChannel::new( + test_client(), + "token".into(), + "T123".into(), + "inst1".into(), + "user1".into(), + ); + let metadata = serde_json::json!({ + "event_type": "message", + "channel_id": "C456", + "sender_id": "U789", + }); + let result = channel + .send_status( + StatusUpdate::ApprovalNeeded { + request_id: "req1".into(), + tool_name: "shell".into(), + description: "run command".into(), + parameters: serde_json::json!({}), + }, + &metadata, + ) + .await; + // Non-DM approval requests are silently skipped (no HTTP call) + assert!(result.is_ok()); + } + + #[tokio::test] + async fn test_send_status_approval_dm_missing_channel_id_errors() { + let channel = RelayChannel::new( + test_client(), + "token".into(), + "T123".into(), + "inst1".into(), + "user1".into(), + ); + let metadata = serde_json::json!({ + "event_type": "direct_message", + "sender_id": "U789", + }); + let result = channel + .send_status( + StatusUpdate::ApprovalNeeded { + request_id: "req1".into(), + tool_name: "shell".into(), + description: "run command".into(), + parameters: serde_json::json!({}), + }, + &metadata, + ) + .await; + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + assert!( + err.contains("channel_id"), + "expected channel_id error, got: {err}" + ); + } + + #[tokio::test] + async fn test_send_status_approval_dm_missing_sender_id_errors() { + let channel = RelayChannel::new( + test_client(), + "token".into(), + "T123".into(), + "inst1".into(), + "user1".into(), + ); + let metadata = serde_json::json!({ + "event_type": "direct_message", + "channel_id": "C456", + }); + let result = channel + .send_status( + StatusUpdate::ApprovalNeeded { + request_id: "req1".into(), + tool_name: "shell".into(), + description: "run command".into(), + parameters: serde_json::json!({}), + }, + &metadata, + ) + .await; + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + assert!( + err.contains("sender_id"), + "expected sender_id error, got: {err}" + ); + } }