gitops/optimclaw
1
0
Fork 0
mirror of https://github.com/outbackdingo/optimclaw.git synced 2026-08-29 08:59:31 +00:00
Code Issues Packages Projects Releases Wiki Activity

feat: add Slack approval buttons for tool execution in DMs (#796)

Browse Source 
* 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<RwLock> 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 <[email protected]>

* fix: apply rustfmt formatting to dispatcher test code

Co-Authored-By: Claude Opus 4.6 <[email protected]>

---------

Co-authored-by: Claude Opus 4.6 <[email protected]>
This commit is contained in:
Pierre LE GUEN
2026-03-12 11:38:53 -07:00
committed by GitHub
co-authored by Claude Opus 4.6
parent 8df51c04ae
commit c94ecf19db
2 changed files with 297 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/agent/dispatcher.rs
+72
View File
@@ -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"));
}
}
src/channels/relay/channel.rs
+225 -3
View File
@@ -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(&parameters).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}"
);
}
}