diff --git a/fuzz/fuzz_targets/fuzz_command_validation.rs b/fuzz/fuzz_targets/fuzz_command_validation.rs index fbbe639d..902286a2 100644 --- a/fuzz/fuzz_targets/fuzz_command_validation.rs +++ b/fuzz/fuzz_targets/fuzz_command_validation.rs @@ -5,6 +5,6 @@ use mentat::security::SecurityPolicy; fuzz_target!(|data: &[u8]| { if let Ok(s) = std::str::from_utf8(data) { let policy = SecurityPolicy::default(); - let _ = policy.validate_command_execution(s, false); + let _ = policy.validate_command_execution(s); } }); diff --git a/src/agent/loop_.rs b/src/agent/loop_.rs index 54c0fa77..15be50eb 100644 --- a/src/agent/loop_.rs +++ b/src/agent/loop_.rs @@ -2178,6 +2178,7 @@ pub(crate) async fn agent_turn( temperature, silent, approval, + None, // approval_adapter channel_name, channel_reply_target, multimodal_config, @@ -2330,6 +2331,7 @@ pub(crate) async fn run_tool_call_loop( temperature: f64, silent: bool, approval: Option<&ApprovalManager>, + approval_adapter: Option<&dyn crate::approval::ChannelApprovalAdapter>, channel_name: &str, channel_reply_target: Option<&str>, multimodal_config: &crate::config::MultimodalConfig, @@ -3132,24 +3134,34 @@ pub(crate) async fn run_tool_call_loop( // ── Approval hook ──────────────────────────────── if let Some(mgr) = approval { if mgr.needs_approval(&tool_name) { - let request = ApprovalRequest { - tool_name: tool_name.clone(), - arguments: tool_args.clone(), - }; - - // Interactive CLI: prompt the operator. - // Non-interactive (channels): auto-deny since no operator - // is present to approve. - let decision = if mgr.is_non_interactive() { + let request = + ApprovalRequest::new(tool_name.clone(), tool_args.clone(), channel_name); + + // Route through adapter when available; otherwise fall + // back to the legacy is_non_interactive/prompt_cli path. + let decision = if let Some(adapter) = approval_adapter { + mgr.request_approval(adapter, &request).await + } else if mgr.is_non_interactive() { ApprovalResponse::No } else { mgr.prompt_cli(&request) }; - mgr.record_decision(&tool_name, &tool_args, decision, channel_name); + // record_decision is only needed for the legacy path — + // request_approval already records internally. + if approval_adapter.is_none() { + mgr.record_decision(&tool_name, &tool_args, decision, channel_name); + } - if decision == ApprovalResponse::No { - let denied = "Denied by user.".to_string(); + if decision == ApprovalResponse::No || decision == ApprovalResponse::Timeout { + let denied = if decision == ApprovalResponse::Timeout { + format!( + "Approval timed out after {}s.", + mgr.approval_timeout().as_secs() + ) + } else { + "Denied by user.".to_string() + }; runtime_trace::record_event( "tool_call_result", Some(channel_name), @@ -3807,12 +3819,18 @@ pub async fn run( builder.build(&ctx).unwrap_or_default() }; - // ── Approval manager (supervised mode) ─────────────────────── + // ── Approval manager + adapter (supervised mode) ────────────── let approval_manager = if interactive { Some(ApprovalManager::from_config(&config.autonomy)) } else { None }; + let cli_approval_adapter: Option> = + if interactive { + Some(Box::new(crate::approval::CliApprovalAdapter)) + } else { + None + }; let channel_name = if interactive { "cli" } else { "daemon" }; let memory_session_id = session_state_file.as_deref().and_then(|path| { let raw = path.to_string_lossy().trim().to_string(); @@ -3931,6 +3949,7 @@ pub async fn run( effective_temperature, false, approval_manager.as_ref(), + cli_approval_adapter.as_deref(), channel_name, None, &config.multimodal, @@ -4232,6 +4251,7 @@ pub async fn run( turn_temperature, true, approval_manager.as_ref(), + cli_approval_adapter.as_deref(), channel_name, None, &config.multimodal, @@ -5755,6 +5775,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "cli", None, &crate::config::MultimodalConfig::default(), @@ -5810,6 +5831,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "cli", None, &multimodal, @@ -5859,6 +5881,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "cli", None, &crate::config::MultimodalConfig::default(), @@ -5907,6 +5930,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "cli", None, &crate::config::MultimodalConfig::default(), @@ -5962,6 +5986,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "cli", None, &multimodal, @@ -6017,6 +6042,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "cli", None, &multimodal, @@ -6073,6 +6099,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "cli", None, &multimodal, @@ -6127,6 +6154,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "cli", None, &multimodal, @@ -6181,6 +6209,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "cli", None, &multimodal, @@ -6318,6 +6347,7 @@ mod tests { 0.0, true, Some(&approval_mgr), + None, // approval_adapter "telegram", None, &crate::config::MultimodalConfig::default(), @@ -6392,6 +6422,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "telegram", Some("chat-42"), &crate::config::MultimodalConfig::default(), @@ -6458,6 +6489,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "telegram", Some("chat-42"), &crate::config::MultimodalConfig::default(), @@ -6519,6 +6551,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "cli", None, &crate::config::MultimodalConfig::default(), @@ -6592,6 +6625,7 @@ mod tests { 0.0, true, Some(&approval_mgr), + None, // approval_adapter "telegram", None, &crate::config::MultimodalConfig::default(), @@ -6656,6 +6690,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "cli", None, &crate::config::MultimodalConfig::default(), @@ -6740,6 +6775,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "cli", None, &crate::config::MultimodalConfig::default(), @@ -6801,6 +6837,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "cli", None, &crate::config::MultimodalConfig::default(), @@ -6888,6 +6925,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "telegram", None, &crate::config::MultimodalConfig::default(), @@ -6957,6 +6995,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "telegram", None, &crate::config::MultimodalConfig::default(), @@ -7028,6 +7067,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "telegram", None, &crate::config::MultimodalConfig::default(), @@ -7103,6 +7143,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "telegram", None, &crate::config::MultimodalConfig::default(), @@ -7187,6 +7228,7 @@ mod tests { 0.0, true, None, + None, // approval_adapter "telegram", None, &crate::config::MultimodalConfig::default(), @@ -9268,6 +9310,7 @@ Let me check the result."#; 0.0, true, None, + None, // approval_adapter "telegram", None, &crate::config::MultimodalConfig::default(), @@ -9426,6 +9469,7 @@ Let me check the result."#; 0.0, true, None, + None, // approval_adapter "test", None, &crate::config::MultimodalConfig::default(), @@ -9508,6 +9552,7 @@ Let me check the result."#; 0.0, true, None, + None, // approval_adapter "test", None, &crate::config::MultimodalConfig::default(), @@ -9567,6 +9612,7 @@ Let me check the result."#; 0.0, true, None, + None, // approval_adapter "test", None, &crate::config::MultimodalConfig::default(), diff --git a/src/approval/adapter.rs b/src/approval/adapter.rs new file mode 100644 index 00000000..965b10e7 --- /dev/null +++ b/src/approval/adapter.rs @@ -0,0 +1,594 @@ +//! Channel-agnostic approval adapter trait and platform-specific implementations. + +use anyhow::Result; +use async_trait::async_trait; + +use super::{ApprovalRequest, ApprovalResponse}; + +// ── Platform Correlation ──────────────────────────────────────────── + +/// Platform-specific correlation data for matching approval responses +/// to their originating requests. +#[derive(Debug, Clone)] +pub enum PlatformRef { + Cli, + Telegram { + chat_id: i64, + message_id: i32, + }, + Slack { + channel_id: String, + thread_ts: String, + }, + Gateway { + connection_id: String, + }, +} + +/// A pending approval request awaiting a user response. +/// +/// Carries the `request_id` for correlation and platform-specific +/// state needed by the adapter to match the response. +#[derive(Debug, Clone)] +pub struct PendingApproval { + pub request_id: String, + pub platform_ref: PlatformRef, +} + +// ── Trait ──────────────────────────────────────────────────────────── + +/// Channel-agnostic interface for prompting users to approve tool calls. +/// +/// Each channel (CLI, Telegram, Slack, Gateway) implements this trait using +/// its platform-native messaging primitives. The `ApprovalManager` delegates +/// to the adapter when a tool call requires user approval. +#[async_trait] +pub trait ChannelApprovalAdapter: Send + Sync { + /// Send an approval prompt to the user and return a handle for + /// awaiting their response. + /// + /// # Errors + /// Returns an error if the prompt could not be delivered. The caller + /// should treat delivery failures as a denial. + async fn send_approval_request(&self, request: &ApprovalRequest) -> Result; + + /// Wait for the user's response to a previously sent approval request. + /// + /// This method blocks (async) until the user responds or the caller + /// cancels (via timeout or cancellation token). It does NOT implement + /// timeout internally — the caller wraps this in `tokio::time::timeout`. + /// + /// # Errors + /// Returns an error if the channel disconnected or an unrecoverable + /// error occurred. The caller should treat this as a denial. + async fn receive_approval_response( + &self, + pending: &PendingApproval, + ) -> Result; + + /// Human-readable name of this adapter's channel (for audit logging). + fn channel_name(&self) -> &str; +} + +// ── CLI Adapter ───────────────────────────────────────────────────── + +/// Approval adapter for CLI (stdin/stderr) interaction. +/// +/// Prompts the user on stderr and reads their response from stdin via +/// `spawn_blocking` to avoid blocking the tokio executor. +pub struct CliApprovalAdapter; + +#[async_trait] +impl ChannelApprovalAdapter for CliApprovalAdapter { + async fn send_approval_request(&self, request: &ApprovalRequest) -> Result { + let prompt = super::format_approval_prompt(request); + eprintln!(); + for line in prompt.lines() { + eprintln!(" {line}"); + } + eprint!(" [Y]es / [N]o / [A]lways for {}: ", request.tool_name); + let _ = std::io::Write::flush(&mut std::io::stderr()); + + Ok(PendingApproval { + request_id: request.request_id.clone(), + platform_ref: PlatformRef::Cli, + }) + } + + async fn receive_approval_response( + &self, + _pending: &PendingApproval, + ) -> Result { + // stdin is blocking I/O — must not block the tokio executor. + let line = tokio::task::spawn_blocking(|| { + let stdin = std::io::stdin(); + let mut buf = String::new(); + std::io::BufRead::read_line(&mut stdin.lock(), &mut buf)?; + Ok::<_, std::io::Error>(buf) + }) + .await + .map_err(|e| anyhow::anyhow!("spawn_blocking failed: {e}"))??; + + Ok(super::parse_approval_input(&line)) + } + + fn channel_name(&self) -> &str { + "cli" + } +} + +// ── Shared helpers ────────────────────────────────────────────────── + +/// Build a Telegram Bot API URL. Shared by `TelegramApprovalAdapter` and +/// `TelegramChannel` (which has its own copy for historical reasons). +fn telegram_api_url(api_base: &str, bot_token: &str, method: &str) -> String { + format!("{api_base}/bot{bot_token}/{method}") +} + +// ── Telegram Adapter ──────────────────────────────────────────────── + +/// Approval adapter for Telegram — sends a message and polls for reply-to +/// correlation. +pub struct TelegramApprovalAdapter { + client: reqwest::Client, + api_base: String, + bot_token: String, + chat_id: String, + thread_id: Option, +} + +impl TelegramApprovalAdapter { + pub fn new( + client: reqwest::Client, + api_base: impl Into, + bot_token: impl Into, + chat_id: impl Into, + thread_id: Option, + ) -> Self { + Self { + client, + api_base: api_base.into(), + bot_token: bot_token.into(), + chat_id: chat_id.into(), + thread_id, + } + } +} + +#[async_trait] +impl ChannelApprovalAdapter for TelegramApprovalAdapter { + async fn send_approval_request(&self, request: &ApprovalRequest) -> Result { + let mut text = super::format_approval_prompt(request); + text.push_str("\n\nReply to this message with:\n• y — approve once\n• n — deny\n• a — always approve this tool"); + + let mut body = serde_json::json!({ + "chat_id": self.chat_id, + "text": text, + "reply_markup": { "force_reply": true, "selective": true } + }); + if let Some(ref tid) = self.thread_id { + body["message_thread_id"] = serde_json::Value::String(tid.clone()); + } + + let resp = self + .client + .post(telegram_api_url( + &self.api_base, + &self.bot_token, + "sendMessage", + )) + .json(&body) + .send() + .await?; + let json: serde_json::Value = resp.json().await?; + let message_id_raw = json["result"]["message_id"] + .as_i64() + .ok_or_else(|| anyhow::anyhow!("missing message_id in sendMessage response"))?; + let message_id = i32::try_from(message_id_raw) + .map_err(|_| anyhow::anyhow!("message_id {message_id_raw} exceeds i32 range"))?; + + let chat_id = json["result"]["chat"]["id"] + .as_i64() + .unwrap_or_else(|| self.chat_id.parse::().unwrap_or(0)); + + Ok(PendingApproval { + request_id: request.request_id.clone(), + platform_ref: PlatformRef::Telegram { + chat_id, + message_id, + }, + }) + } + + async fn receive_approval_response( + &self, + pending: &PendingApproval, + ) -> Result { + let (expected_chat_id, expected_msg_id) = match &pending.platform_ref { + PlatformRef::Telegram { + chat_id, + message_id, + } => (*chat_id, *message_id), + _ => anyhow::bail!("TelegramApprovalAdapter got non-Telegram PlatformRef"), + }; + + let mut offset: i64 = 0; + loop { + let resp = self + .client + .post(telegram_api_url( + &self.api_base, + &self.bot_token, + "getUpdates", + )) + .json(&serde_json::json!({ + "offset": offset, + "timeout": 5, + "allowed_updates": ["message"] + })) + .send() + .await?; + let json: serde_json::Value = resp.json().await?; + + if let Some(updates) = json["result"].as_array() { + for update in updates { + if let Some(uid) = update["update_id"].as_i64() { + offset = uid + 1; + } + let msg = &update["message"]; + let reply_to = &msg["reply_to_message"]; + let reply_msg_id = reply_to["message_id"].as_i64().unwrap_or(-1); + let chat_id = msg["chat"]["id"].as_i64().unwrap_or(0); + + if reply_msg_id == i64::from(expected_msg_id) && chat_id == expected_chat_id { + let text = msg["text"].as_str().unwrap_or(""); + return Ok(super::parse_approval_input(text)); + } + } + } + + tokio::time::sleep(std::time::Duration::from_secs(2)).await; + } + } + + fn channel_name(&self) -> &str { + "telegram" + } +} + +// ── Slack Adapter ─────────────────────────────────────────────────── + +/// Approval adapter for Slack — sends a thread reply and polls for responses. +pub struct SlackApprovalAdapter { + client: reqwest::Client, + bot_token: String, + channel_id: String, + thread_ts: Option, +} + +impl SlackApprovalAdapter { + pub fn new( + bot_token: impl Into, + channel_id: impl Into, + thread_ts: Option, + ) -> Self { + Self { + client: reqwest::Client::new(), + bot_token: bot_token.into(), + channel_id: channel_id.into(), + thread_ts, + } + } +} + +#[async_trait] +impl ChannelApprovalAdapter for SlackApprovalAdapter { + async fn send_approval_request(&self, request: &ApprovalRequest) -> Result { + let mut text = super::format_approval_prompt(request); + text.push_str("\n\nReply in this thread with:\n• y — approve once\n• n — deny\n• a — always approve this tool"); + + let mut body = serde_json::json!({ + "channel": self.channel_id, + "text": text, + }); + if let Some(ref ts) = self.thread_ts { + body["thread_ts"] = serde_json::Value::String(ts.clone()); + } + + let resp = self + .client + .post("https://slack.com/api/chat.postMessage") + .bearer_auth(&self.bot_token) + .json(&body) + .send() + .await?; + let json: serde_json::Value = resp.json().await?; + + let ts = json["ts"] + .as_str() + .or_else(|| json["message"]["ts"].as_str()) + .ok_or_else(|| anyhow::anyhow!("missing ts in Slack postMessage response"))? + .to_string(); + + Ok(PendingApproval { + request_id: request.request_id.clone(), + platform_ref: PlatformRef::Slack { + channel_id: self.channel_id.clone(), + thread_ts: ts, + }, + }) + } + + async fn receive_approval_response( + &self, + pending: &PendingApproval, + ) -> Result { + let (channel_id, thread_ts) = match &pending.platform_ref { + PlatformRef::Slack { + channel_id, + thread_ts, + } => (channel_id.clone(), thread_ts.clone()), + _ => anyhow::bail!("SlackApprovalAdapter got non-Slack PlatformRef"), + }; + + loop { + let resp = self + .client + .get("https://slack.com/api/conversations.replies") + .bearer_auth(&self.bot_token) + .query(&[ + ("channel", channel_id.as_str()), + ("ts", thread_ts.as_str()), + ("limit", "10"), + ]) + .send() + .await?; + let json: serde_json::Value = resp.json().await?; + + if let Some(messages) = json["messages"].as_array() { + for msg in messages { + let msg_ts = msg["ts"].as_str().unwrap_or(""); + // Only consider replies after the approval message + if msg_ts > thread_ts.as_str() { + let text = msg["text"].as_str().unwrap_or(""); + return Ok(super::parse_approval_input(text)); + } + } + } + + tokio::time::sleep(std::time::Duration::from_secs(3)).await; + } + } + + fn channel_name(&self) -> &str { + "slack" + } +} + +// ── Gateway (WebSocket) Adapter ───────────────────────────────────── + +/// Approval adapter for the WebSocket gateway — sends JSON frames and +/// waits for a matching `approval_response` frame via a oneshot channel. +pub struct GatewayApprovalAdapter { + frame_tx: tokio::sync::mpsc::Sender, + pending_responses: std::sync::Arc< + tokio::sync::Mutex< + std::collections::HashMap>, + >, + >, + response_rx: tokio::sync::Mutex>>, +} + +impl GatewayApprovalAdapter { + pub fn new( + frame_tx: tokio::sync::mpsc::Sender, + pending_responses: std::sync::Arc< + tokio::sync::Mutex< + std::collections::HashMap>, + >, + >, + ) -> Self { + Self { + frame_tx, + pending_responses, + response_rx: tokio::sync::Mutex::new(None), + } + } + + /// Resolve an incoming `approval_response` frame from the WebSocket client. + /// + /// Called by the WebSocket receive loop when it sees a frame with + /// `{"type":"approval_response", "request_id":"...", "decision":"..."}`. + pub async fn resolve_approval_response( + pending: &std::sync::Arc< + tokio::sync::Mutex< + std::collections::HashMap>, + >, + >, + request_id: &str, + decision: &str, + ) -> bool { + let mut map = pending.lock().await; + if let Some(tx) = map.remove(request_id) { + let response = super::parse_approval_input(decision); + tx.send(response).is_ok() + } else { + false + } + } +} + +#[async_trait] +impl ChannelApprovalAdapter for GatewayApprovalAdapter { + async fn send_approval_request(&self, request: &ApprovalRequest) -> Result { + let frame = serde_json::json!({ + "type": "approval_request", + "request_id": request.request_id, + "tool_name": request.tool_name, + "arguments": request.arguments, + "risk_level": request.risk_level, + }); + + let (tx, rx) = tokio::sync::oneshot::channel(); + { + let mut map = self.pending_responses.lock().await; + map.insert(request.request_id.clone(), tx); + } + *self.response_rx.lock().await = Some(rx); + + self.frame_tx + .send(frame.to_string()) + .await + .map_err(|e| anyhow::anyhow!("failed to send approval frame: {e}"))?; + + Ok(PendingApproval { + request_id: request.request_id.clone(), + platform_ref: PlatformRef::Gateway { + connection_id: request.request_id.clone(), + }, + }) + } + + async fn receive_approval_response( + &self, + _pending: &PendingApproval, + ) -> Result { + let rx = self + .response_rx + .lock() + .await + .take() + .ok_or_else(|| anyhow::anyhow!("no pending approval response receiver"))?; + rx.await + .map_err(|_| anyhow::anyhow!("approval response channel closed")) + } + + fn channel_name(&self) -> &str { + "gateway" + } +} + +// ── Test fixtures ─────────────────────────────────────────────────── + +#[cfg(test)] +pub(crate) mod tests { + use super::*; + + /// A mock adapter that returns a configurable response. + pub struct MockApprovalAdapter { + response: ApprovalResponse, + } + + impl MockApprovalAdapter { + pub fn new(response: ApprovalResponse) -> Self { + Self { response } + } + } + + #[async_trait] + impl ChannelApprovalAdapter for MockApprovalAdapter { + async fn send_approval_request( + &self, + request: &ApprovalRequest, + ) -> Result { + Ok(PendingApproval { + request_id: request.request_id.clone(), + platform_ref: PlatformRef::Cli, + }) + } + + async fn receive_approval_response( + &self, + _pending: &PendingApproval, + ) -> Result { + Ok(self.response) + } + + fn channel_name(&self) -> &str { + "mock" + } + } + + /// An adapter that never responds (hangs forever). Used to test timeout. + pub struct HangingApprovalAdapter; + + #[async_trait] + impl ChannelApprovalAdapter for HangingApprovalAdapter { + async fn send_approval_request( + &self, + request: &ApprovalRequest, + ) -> Result { + Ok(PendingApproval { + request_id: request.request_id.clone(), + platform_ref: PlatformRef::Cli, + }) + } + + async fn receive_approval_response( + &self, + _pending: &PendingApproval, + ) -> Result { + // Never resolves — the caller's timeout will fire. + std::future::pending().await + } + + fn channel_name(&self) -> &str { + "hanging" + } + } + + /// An adapter that simulates a delivery failure. + pub struct FailingApprovalAdapter; + + #[async_trait] + impl ChannelApprovalAdapter for FailingApprovalAdapter { + async fn send_approval_request( + &self, + _request: &ApprovalRequest, + ) -> Result { + Err(anyhow::anyhow!("channel disconnected")) + } + + async fn receive_approval_response( + &self, + _pending: &PendingApproval, + ) -> Result { + Err(anyhow::anyhow!("channel disconnected")) + } + + fn channel_name(&self) -> &str { + "failing" + } + } + + #[test] + fn mock_adapter_constructs() { + let adapter = MockApprovalAdapter::new(ApprovalResponse::Yes); + assert_eq!(adapter.channel_name(), "mock"); + } + + #[tokio::test] + async fn mock_adapter_send_receive_lifecycle() { + let adapter = MockApprovalAdapter::new(ApprovalResponse::Always); + let request = ApprovalRequest::new( + "file_write".into(), + serde_json::json!({"path": "test.txt"}), + "test", + ); + + let pending = adapter.send_approval_request(&request).await.unwrap(); + assert_eq!(pending.request_id, request.request_id); + + let response = adapter.receive_approval_response(&pending).await.unwrap(); + assert_eq!(response, ApprovalResponse::Always); + } + + #[tokio::test] + async fn failing_adapter_returns_error() { + let adapter = FailingApprovalAdapter; + let request = ApprovalRequest::new("file_write".into(), serde_json::json!({}), "test"); + + let result = adapter.send_approval_request(&request).await; + assert!(result.is_err()); + } +} diff --git a/src/approval/mod.rs b/src/approval/mod.rs index 6a205664..5d0c71ee 100644 --- a/src/approval/mod.rs +++ b/src/approval/mod.rs @@ -1,15 +1,26 @@ //! Interactive approval workflow for supervised mode. //! //! Provides a pre-execution hook that prompts the user before tool calls, -//! with session-scoped "Always" allowlists and audit logging. +//! with session-scoped "Always" allowlists and audit logging. Each channel +//! implements [`ChannelApprovalAdapter`] to deliver approval prompts through +//! its native messaging primitives. + +pub mod adapter; + +pub use adapter::{ + ChannelApprovalAdapter, CliApprovalAdapter, SlackApprovalAdapter, TelegramApprovalAdapter, +}; +// GatewayApprovalAdapter is constructed directly from the gateway/ws.rs module +// via crate::approval::adapter::GatewayApprovalAdapter when needed. use crate::config::AutonomyConfig; -use crate::security::AutonomyLevel; +use crate::security::{AutonomyLevel, CommandRiskLevel, SecurityPolicy}; use chrono::Utc; use parking_lot::Mutex; use serde::{Deserialize, Serialize}; use std::collections::HashSet; use std::io::{self, BufRead, Write}; +use std::time::Duration; // ── Types ──────────────────────────────────────────────────────── @@ -18,6 +29,56 @@ use std::io::{self, BufRead, Write}; pub struct ApprovalRequest { pub tool_name: String, pub arguments: serde_json::Value, + /// Unique correlation ID (UUID v4). + #[serde(default)] + pub request_id: String, + /// Originating channel name. + #[serde(default)] + pub channel: String, + /// User/chat to send the prompt to. + #[serde(default)] + pub recipient: String, + /// Thread context for threaded replies (Slack, Telegram). + #[serde(default)] + pub thread_ts: Option, + /// Harness-classified risk level (for shell commands). + #[serde(default)] + pub risk_level: Option, + /// When the request was created. + #[serde(default)] + pub created_at: Option>, +} + +impl ApprovalRequest { + pub fn new(tool_name: String, arguments: serde_json::Value, channel: &str) -> Self { + Self { + tool_name, + arguments, + request_id: uuid::Uuid::new_v4().to_string(), + channel: channel.to_string(), + recipient: String::new(), + thread_ts: None, + risk_level: None, + created_at: Some(Utc::now()), + } + } +} + +/// Format the common approval prompt body shared by all adapters. +/// +/// Returns the tool/args/risk section. Each adapter appends its own +/// platform-specific response instructions. +pub fn format_approval_prompt(request: &ApprovalRequest) -> String { + let summary = summarize_args(&request.arguments); + let mut text = format!( + "🔧 Approval needed\n\nTool: {}\nArgs: {}", + request.tool_name, summary + ); + if let Some(ref risk) = request.risk_level { + use std::fmt::Write; + let _ = write!(text, "\nRisk: {risk:?}"); + } + text } /// The user's response to an approval request. @@ -30,6 +91,8 @@ pub enum ApprovalResponse { No, /// Execute and add tool to session-scoped allowlist. Always, + /// No response within timeout period. + Timeout, } /// A single audit log entry for an approval decision. @@ -40,6 +103,15 @@ pub struct ApprovalLogEntry { pub arguments_summary: String, pub decision: ApprovalResponse, pub channel: String, + /// Harness-classified risk level (for shell commands). + #[serde(default, skip_serializing_if = "Option::is_none")] + pub risk_level: Option, + /// Correlation ID for channel-prompted decisions. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub request_id: Option, + /// Time from prompt sent to response received (ms). + #[serde(default, skip_serializing_if = "Option::is_none")] + pub response_time_ms: Option, } // ── ApprovalManager ────────────────────────────────────────────── @@ -49,13 +121,7 @@ pub struct ApprovalLogEntry { /// - Checks config-level `auto_approve` / `always_ask` lists /// - Maintains a session-scoped "always" allowlist /// - Records an audit trail of all decisions -/// -/// Two modes: -/// - **Interactive** (CLI): tools needing approval trigger a stdin prompt. -/// - **Non-interactive** (channels): tools needing approval are auto-denied -/// because there is no interactive operator to approve them. `auto_approve` -/// policy is still enforced, and `always_ask` / supervised-default tools are -/// denied rather than silently allowed. +/// - Delegates to a [`ChannelApprovalAdapter`] for channel-specific prompting pub struct ApprovalManager { /// Tools that never need approval (from config). auto_approve: HashSet, @@ -64,8 +130,11 @@ pub struct ApprovalManager { /// Autonomy level from config. autonomy_level: AutonomyLevel, /// When `true`, tools that would require interactive approval are - /// auto-denied instead. Used for channel-driven (non-CLI) runs. + /// auto-denied instead. Used for channel-driven runs that have not + /// yet been wired with a [`ChannelApprovalAdapter`]. non_interactive: bool, + /// Approval timeout duration. + approval_timeout: Duration, /// Session-scoped allowlist built from "Always" responses. session_allowlist: Mutex>, /// Audit trail of approval decisions. @@ -80,29 +149,29 @@ impl ApprovalManager { always_ask: config.always_ask.iter().cloned().collect(), autonomy_level: config.level, non_interactive: false, + approval_timeout: Duration::from_secs(config.approval_timeout_secs), session_allowlist: Mutex::new(HashSet::new()), audit_log: Mutex::new(Vec::new()), } } - /// Create a non-interactive approval manager for channel-driven runs. + /// Create a non-interactive approval manager for channel-driven runs + /// that do not yet have a [`ChannelApprovalAdapter`]. /// - /// Enforces the same `auto_approve` / `always_ask` / supervised policies - /// as the CLI manager, but tools that would require interactive approval - /// are auto-denied instead of prompting (since there is no operator). + /// Tools requiring approval are auto-denied instead of prompting. pub fn for_non_interactive(config: &AutonomyConfig) -> Self { Self { auto_approve: config.auto_approve.iter().cloned().collect(), always_ask: config.always_ask.iter().cloned().collect(), autonomy_level: config.level, non_interactive: true, + approval_timeout: Duration::from_secs(config.approval_timeout_secs), session_allowlist: Mutex::new(HashSet::new()), audit_log: Mutex::new(Vec::new()), } } - /// Returns `true` when this manager operates in non-interactive mode - /// (i.e. for channel-driven runs where no operator can approve). + /// Returns `true` when this manager operates in non-interactive mode. pub fn is_non_interactive(&self) -> bool { self.non_interactive } @@ -130,7 +199,8 @@ impl ApprovalManager { // own command allowlist and risk policy. Skipping the outer approval // gate here lets low-risk allowlisted commands (e.g. `ls`) work in // non-interactive channels without silently allowing medium/high-risk - // commands. + // commands. This path is transitional — once channels have adapters, + // shell approval is routed through needs_shell_approval() instead. if self.non_interactive && tool_name == "shell" { return false; } @@ -150,6 +220,106 @@ impl ApprovalManager { true } + /// Check whether a shell command requires approval based on its risk level. + /// + /// Delegates to [`SecurityPolicy::validate_command_execution`] and inspects the + /// `needs_approval` flag from the returned tuple. + pub fn needs_shell_approval(&self, command: &str, security: &SecurityPolicy) -> bool { + if self.autonomy_level == AutonomyLevel::Full { + return false; + } + if self.autonomy_level == AutonomyLevel::ReadOnly { + return false; + } + + match security.validate_command_execution(command) { + Ok((_risk, needs_approval)) => needs_approval, + Err(_) => false, // hard-blocked commands are denied before approval + } + } + + /// Send an approval request through the adapter and wait for a response, + /// with timeout. + /// + /// Returns the user's decision. On adapter error or timeout, returns a + /// denial (`No` or `Timeout` respectively). + pub async fn request_approval( + &self, + adapter: &dyn ChannelApprovalAdapter, + request: &ApprovalRequest, + ) -> ApprovalResponse { + let start = std::time::Instant::now(); + + let pending = match adapter.send_approval_request(request).await { + Ok(p) => p, + Err(e) => { + tracing::warn!( + tool = %request.tool_name, + error = %e, + "failed to send approval request, treating as denial" + ); + return ApprovalResponse::No; + } + }; + + let response = match tokio::time::timeout( + self.approval_timeout, + adapter.receive_approval_response(&pending), + ) + .await + { + Ok(Ok(resp)) => resp, + Ok(Err(e)) => { + tracing::warn!( + tool = %request.tool_name, + error = %e, + "error receiving approval response, treating as denial" + ); + ApprovalResponse::No + } + Err(_elapsed) => { + tracing::info!( + tool = %request.tool_name, + timeout_secs = self.approval_timeout.as_secs(), + "approval request timed out" + ); + ApprovalResponse::Timeout + } + }; + + #[allow(clippy::cast_possible_truncation)] + let elapsed_ms = start.elapsed().as_millis() as u64; + self.record_decision_ext( + &request.tool_name, + &request.arguments, + response, + adapter.channel_name(), + request + .risk_level + .as_ref() + .map(|r| format!("{r:?}").to_ascii_lowercase()), + Some(request.request_id.clone()), + Some(elapsed_ms), + ); + + self.maybe_add_to_allowlist(&request.tool_name, response); + response + } + + /// Record that a tool call was auto-approved (no prompt needed). + pub fn record_auto_approved(&self, tool_name: &str, channel: &str) { + tracing::debug!(tool = %tool_name, channel, "auto-approved tool call"); + self.record_decision_ext( + tool_name, + &serde_json::Value::Null, + ApprovalResponse::Yes, + channel, + None, + None, + None, + ); + } + /// Record an approval decision and update session state. pub fn record_decision( &self, @@ -158,13 +328,48 @@ impl ApprovalManager { decision: ApprovalResponse, channel: &str, ) { - // If "Always", add to session allowlist. - if decision == ApprovalResponse::Always { + match decision { + ApprovalResponse::Yes => { + tracing::info!(tool = %tool_name, channel, "tool call approved by user"); + } + ApprovalResponse::No => { + tracing::info!(tool = %tool_name, channel, "tool call denied by user"); + } + ApprovalResponse::Always => { + tracing::info!(tool = %tool_name, channel, "tool call always-approved by user"); + } + ApprovalResponse::Timeout => { + tracing::info!(tool = %tool_name, channel, "tool call approval timed out"); + } + } + + self.record_decision_ext(tool_name, args, decision, channel, None, None, None); + self.maybe_add_to_allowlist(tool_name, decision); + } + + /// If the decision is "Always" and the tool is not in `always_ask`, + /// add it to the session allowlist so future calls skip prompting. + fn maybe_add_to_allowlist(&self, tool_name: &str, decision: ApprovalResponse) { + if decision == ApprovalResponse::Always + && !self.always_ask.contains(tool_name) + && !self.always_ask.contains("*") + { let mut allowlist = self.session_allowlist.lock(); allowlist.insert(tool_name.to_string()); } + } - // Append to audit log. + /// Record an approval decision with extended audit fields. + fn record_decision_ext( + &self, + tool_name: &str, + args: &serde_json::Value, + decision: ApprovalResponse, + channel: &str, + risk_level: Option, + request_id: Option, + response_time_ms: Option, + ) { let summary = summarize_args(args); let entry = ApprovalLogEntry { timestamp: Utc::now().to_rfc3339(), @@ -172,6 +377,9 @@ impl ApprovalManager { arguments_summary: summary, decision, channel: channel.to_string(), + risk_level, + request_id, + response_time_ms, }; let mut log = self.audit_log.lock(); log.push(entry); @@ -189,21 +397,26 @@ impl ApprovalManager { /// Prompt the user on the CLI and return their decision. /// - /// Only called for interactive (CLI) managers. Non-interactive managers - /// auto-deny in the tool-call loop before reaching this point. + /// Only called for interactive (CLI) managers when no adapter is available. pub fn prompt_cli(&self, request: &ApprovalRequest) -> ApprovalResponse { prompt_cli_interactive(request) } + + /// The configured approval timeout duration. + pub fn approval_timeout(&self) -> Duration { + self.approval_timeout + } } // ── CLI prompt ─────────────────────────────────────────────────── /// Display the approval prompt and read user input from stdin. fn prompt_cli_interactive(request: &ApprovalRequest) -> ApprovalResponse { - let summary = summarize_args(&request.arguments); + let prompt = format_approval_prompt(request); eprintln!(); - eprintln!("🔧 Agent wants to execute: {}", request.tool_name); - eprintln!(" {summary}"); + for line in prompt.lines() { + eprintln!(" {line}"); + } eprint!(" [Y]es / [N]o / [A]lways for {}: ", request.tool_name); let _ = io::stderr().flush(); @@ -213,7 +426,13 @@ fn prompt_cli_interactive(request: &ApprovalRequest) -> ApprovalResponse { return ApprovalResponse::No; } - match line.trim().to_ascii_lowercase().as_str() { + parse_approval_input(&line) +} + +/// Parse user input into an approval response. Case-insensitive. +/// Unrecognized input is treated as denial (fail-safe). +pub fn parse_approval_input(input: &str) -> ApprovalResponse { + match input.trim().to_ascii_lowercase().as_str() { "y" | "yes" => ApprovalResponse::Yes, "a" | "always" => ApprovalResponse::Always, _ => ApprovalResponse::No, @@ -221,7 +440,7 @@ fn prompt_cli_interactive(request: &ApprovalRequest) -> ApprovalResponse { } /// Produce a short human-readable summary of tool arguments. -fn summarize_args(args: &serde_json::Value) -> String { +pub fn summarize_args(args: &serde_json::Value) -> String { match args { serde_json::Value::Object(map) => { let parts: Vec = map @@ -443,145 +662,185 @@ mod tests { assert!(summary.contains("just a string")); } - // ── non-interactive (channel) mode ──────────────────────── + // ── ApprovalResponse serde ─────────────────────────────── #[test] - fn non_interactive_manager_reports_non_interactive() { - let mgr = ApprovalManager::for_non_interactive(&supervised_config()); - assert!(mgr.is_non_interactive()); + fn approval_response_serde_roundtrip() { + let json = serde_json::to_string(&ApprovalResponse::Always).unwrap(); + assert_eq!(json, "\"always\""); + let parsed: ApprovalResponse = serde_json::from_str("\"no\"").unwrap(); + assert_eq!(parsed, ApprovalResponse::No); } #[test] - fn interactive_manager_reports_interactive() { - let mgr = ApprovalManager::from_config(&supervised_config()); - assert!(!mgr.is_non_interactive()); + fn approval_response_timeout_serde_roundtrip() { + let json = serde_json::to_string(&ApprovalResponse::Timeout).unwrap(); + assert_eq!(json, "\"timeout\""); + let parsed: ApprovalResponse = serde_json::from_str(&json).unwrap(); + assert_eq!(parsed, ApprovalResponse::Timeout); } + // ── ApprovalRequest ────────────────────────────────────── + #[test] - fn non_interactive_auto_approve_tools_skip_approval() { - let mgr = ApprovalManager::for_non_interactive(&supervised_config()); - // auto_approve tools (file_read, memory_recall) should not need approval. - assert!(!mgr.needs_approval("file_read")); - assert!(!mgr.needs_approval("memory_recall")); + fn approval_request_serde() { + let req = ApprovalRequest::new( + "shell".into(), + serde_json::json!({"command": "echo hi"}), + "test", + ); + let json = serde_json::to_string(&req).unwrap(); + let parsed: ApprovalRequest = serde_json::from_str(&json).unwrap(); + assert_eq!(parsed.tool_name, "shell"); + assert!(!parsed.request_id.is_empty()); } #[test] - fn non_interactive_shell_skips_outer_approval_by_default() { - let mgr = ApprovalManager::for_non_interactive(&AutonomyConfig::default()); - assert!(!mgr.needs_approval("shell")); + fn approval_request_new_has_defaults() { + let req = ApprovalRequest::new("file_write".into(), serde_json::json!({}), "test"); + assert!(!req.request_id.is_empty()); + assert!(req.created_at.is_some()); + assert!(req.risk_level.is_none()); + assert!(req.thread_ts.is_none()); } + // ── parse_approval_input ───────────────────────────────── + #[test] - fn non_interactive_always_ask_tools_need_approval() { - let mgr = ApprovalManager::for_non_interactive(&supervised_config()); - // always_ask tools (shell) still report as needing approval, - // so the tool-call loop will auto-deny them in non-interactive mode. - assert!(mgr.needs_approval("shell")); + fn parse_approval_input_yes_variants() { + assert_eq!(parse_approval_input("y"), ApprovalResponse::Yes); + assert_eq!(parse_approval_input("Y"), ApprovalResponse::Yes); + assert_eq!(parse_approval_input("yes"), ApprovalResponse::Yes); + assert_eq!(parse_approval_input("YES"), ApprovalResponse::Yes); + assert_eq!(parse_approval_input(" y "), ApprovalResponse::Yes); } #[test] - fn non_interactive_unknown_tools_need_approval_in_supervised() { - let mgr = ApprovalManager::for_non_interactive(&supervised_config()); - // Unknown tools in supervised mode need approval (will be auto-denied - // by the tool-call loop for non-interactive managers). - assert!(mgr.needs_approval("file_write")); - assert!(mgr.needs_approval("http_request")); + fn parse_approval_input_no_variants() { + assert_eq!(parse_approval_input("n"), ApprovalResponse::No); + assert_eq!(parse_approval_input("no"), ApprovalResponse::No); + assert_eq!(parse_approval_input("N"), ApprovalResponse::No); } #[test] - fn non_interactive_full_autonomy_never_needs_approval() { - let mgr = ApprovalManager::for_non_interactive(&full_config()); - // Full autonomy means no approval needed, even in non-interactive mode. - assert!(!mgr.needs_approval("shell")); - assert!(!mgr.needs_approval("file_write")); - assert!(!mgr.needs_approval("anything")); + fn parse_approval_input_always_variants() { + assert_eq!(parse_approval_input("a"), ApprovalResponse::Always); + assert_eq!(parse_approval_input("always"), ApprovalResponse::Always); + assert_eq!(parse_approval_input("ALWAYS"), ApprovalResponse::Always); } #[test] - fn non_interactive_readonly_never_needs_approval() { - let config = AutonomyConfig { - level: AutonomyLevel::ReadOnly, - ..AutonomyConfig::default() - }; - let mgr = ApprovalManager::for_non_interactive(&config); - // ReadOnly blocks execution elsewhere; approval manager does not prompt. - assert!(!mgr.needs_approval("shell")); + fn parse_approval_input_unrecognized_is_no() { + assert_eq!(parse_approval_input("maybe"), ApprovalResponse::No); + assert_eq!(parse_approval_input(""), ApprovalResponse::No); + assert_eq!(parse_approval_input("xyz"), ApprovalResponse::No); } - #[test] - fn non_interactive_session_allowlist_still_works() { - let mgr = ApprovalManager::for_non_interactive(&supervised_config()); - assert!(mgr.needs_approval("file_write")); + // ── needs_shell_approval ───────────────────────────────── - // Simulate an "Always" decision (would come from a prior channel run - // if the tool was auto-approved somehow, e.g. via config change). - mgr.record_decision( - "file_write", - &serde_json::json!({"path": "test.txt"}), - ApprovalResponse::Always, - "telegram", - ); + #[test] + fn needs_shell_approval_low_risk_no_prompt() { + let config = supervised_config(); + let mgr = ApprovalManager::from_config(&config); + let security = SecurityPolicy::default(); + assert!(!mgr.needs_shell_approval("ls -la", &security)); + assert!(!mgr.needs_shell_approval("git status", &security)); + } - assert!(!mgr.needs_approval("file_write")); + fn security_with_all_commands() -> SecurityPolicy { + SecurityPolicy { + allowed_commands: vec!["*".into()], + ..SecurityPolicy::default() + } } #[test] - fn non_interactive_always_ask_overrides_session_allowlist() { - let mgr = ApprovalManager::for_non_interactive(&supervised_config()); - - mgr.record_decision( - "shell", - &serde_json::json!({"command": "ls"}), - ApprovalResponse::Always, - "telegram", - ); + fn needs_shell_approval_medium_risk_prompts() { + let config = supervised_config(); + let mgr = ApprovalManager::from_config(&config); + let security = security_with_all_commands(); + assert!(mgr.needs_shell_approval("touch file.txt", &security)); + } - // shell is in always_ask, so it still needs approval even after "Always". - assert!(mgr.needs_approval("shell")); + #[test] + fn needs_shell_approval_high_risk_prompts() { + let config = supervised_config(); + let mgr = ApprovalManager::from_config(&config); + // Must explicitly allow `rm` and disable block_high_risk_commands + // to reach the approval gate instead of the hard-block path. + let security = SecurityPolicy { + allowed_commands: vec!["rm".into()], + block_high_risk_commands: false, + ..SecurityPolicy::default() + }; + assert!(mgr.needs_shell_approval("rm -rf /tmp/test", &security)); } - // ── ApprovalResponse serde ─────────────────────────────── + #[test] + fn needs_shell_approval_full_autonomy_never_prompts() { + let config = full_config(); + let mgr = ApprovalManager::from_config(&config); + let security = security_with_all_commands(); + assert!(!mgr.needs_shell_approval("rm -rf /tmp/test", &security)); + } #[test] - fn approval_response_serde_roundtrip() { - let json = serde_json::to_string(&ApprovalResponse::Always).unwrap(); - assert_eq!(json, "\"always\""); - let parsed: ApprovalResponse = serde_json::from_str("\"no\"").unwrap(); - assert_eq!(parsed, ApprovalResponse::No); + fn needs_shell_approval_medium_risk_no_prompt_when_disabled() { + let config = AutonomyConfig { + level: AutonomyLevel::Supervised, + require_approval_for_medium_risk: false, + ..AutonomyConfig::default() + }; + let mgr = ApprovalManager::from_config(&config); + let security = SecurityPolicy { + require_approval_for_medium_risk: false, + allowed_commands: vec!["*".into()], + ..SecurityPolicy::default() + }; + assert!(!mgr.needs_shell_approval("touch file.txt", &security)); } - // ── ApprovalRequest ────────────────────────────────────── + // ── audit log extended fields ──────────────────────────── #[test] - fn approval_request_serde() { - let req = ApprovalRequest { - tool_name: "shell".into(), - arguments: serde_json::json!({"command": "echo hi"}), - }; - let json = serde_json::to_string(&req).unwrap(); - let parsed: ApprovalRequest = serde_json::from_str(&json).unwrap(); - assert_eq!(parsed.tool_name, "shell"); + fn record_decision_ext_includes_risk_and_timing() { + let mgr = ApprovalManager::from_config(&supervised_config()); + mgr.record_decision_ext( + "shell", + &serde_json::json!({"command": "rm -rf ./build/"}), + ApprovalResponse::Yes, + "telegram", + Some("high".into()), + Some("req-123".into()), + Some(450), + ); + + let log = mgr.audit_log(); + assert_eq!(log.len(), 1); + assert_eq!(log[0].risk_level.as_deref(), Some("high")); + assert_eq!(log[0].request_id.as_deref(), Some("req-123")); + assert_eq!(log[0].response_time_ms, Some(450)); } - // ── Regression: #4247 default approved tools in channels ── + // ── Backward compat: non-interactive patterns ──────────── #[test] - fn non_interactive_allows_default_auto_approve_tools() { + fn auto_approve_tools_skip_approval_any_config() { let config = AutonomyConfig::default(); - let mgr = ApprovalManager::for_non_interactive(&config); + let mgr = ApprovalManager::from_config(&config); for tool in &config.auto_approve { assert!( !mgr.needs_approval(tool), - "default auto_approve tool '{tool}' should not need approval in non-interactive mode" + "default auto_approve tool '{tool}' should not need approval" ); } } #[test] - fn non_interactive_denies_unknown_tools() { + fn unknown_tools_need_approval() { let config = AutonomyConfig::default(); - let mgr = ApprovalManager::for_non_interactive(&config); + let mgr = ApprovalManager::from_config(&config); assert!( mgr.needs_approval("some_unknown_tool"), "unknown tool should need approval" @@ -589,9 +848,9 @@ mod tests { } #[test] - fn non_interactive_weather_is_auto_approved() { + fn weather_is_auto_approved() { let config = AutonomyConfig::default(); - let mgr = ApprovalManager::for_non_interactive(&config); + let mgr = ApprovalManager::from_config(&config); assert!( !mgr.needs_approval("weather"), "weather tool must not need approval — it is in the default auto_approve list" @@ -602,10 +861,115 @@ mod tests { fn always_ask_overrides_auto_approve() { let mut config = AutonomyConfig::default(); config.always_ask = vec!["weather".into()]; - let mgr = ApprovalManager::for_non_interactive(&config); + let mgr = ApprovalManager::from_config(&config); assert!( mgr.needs_approval("weather"), "always_ask must override auto_approve" ); } + + // ── request_approval with mock adapter ─────────────────── + + #[tokio::test] + async fn request_approval_yes_flow() { + use super::adapter::tests::MockApprovalAdapter; + + let config = supervised_config(); + let mgr = ApprovalManager::from_config(&config); + let adapter = MockApprovalAdapter::new(ApprovalResponse::Yes); + + let request = ApprovalRequest::new( + "file_write".into(), + serde_json::json!({"path": "test.txt"}), + "test", + ); + + let result = mgr.request_approval(&adapter, &request).await; + assert_eq!(result, ApprovalResponse::Yes); + + let log = mgr.audit_log(); + assert_eq!(log.len(), 1); + assert_eq!(log[0].decision, ApprovalResponse::Yes); + assert!(log[0].response_time_ms.is_some()); + } + + #[tokio::test] + async fn request_approval_always_adds_to_allowlist() { + use super::adapter::tests::MockApprovalAdapter; + + let config = supervised_config(); + let mgr = ApprovalManager::from_config(&config); + let adapter = MockApprovalAdapter::new(ApprovalResponse::Always); + + let request = ApprovalRequest::new( + "file_write".into(), + serde_json::json!({"path": "test.txt"}), + "test", + ); + + let result = mgr.request_approval(&adapter, &request).await; + assert_eq!(result, ApprovalResponse::Always); + assert!(!mgr.needs_approval("file_write")); + } + + #[tokio::test] + async fn request_approval_always_for_always_ask_tool_does_not_allowlist() { + use super::adapter::tests::MockApprovalAdapter; + + let config = supervised_config(); + let mgr = ApprovalManager::from_config(&config); + let adapter = MockApprovalAdapter::new(ApprovalResponse::Always); + + let request = + ApprovalRequest::new("shell".into(), serde_json::json!({"command": "ls"}), "test"); + + let result = mgr.request_approval(&adapter, &request).await; + assert_eq!(result, ApprovalResponse::Always); + // shell is in always_ask, so it should still need approval. + assert!(mgr.needs_approval("shell")); + } + + #[tokio::test] + async fn request_approval_timeout_flow() { + use super::adapter::tests::HangingApprovalAdapter; + + let config = AutonomyConfig { + approval_timeout_secs: 1, // 1 second timeout for test speed + ..supervised_config() + }; + let mgr = ApprovalManager::from_config(&config); + let adapter = HangingApprovalAdapter; + + let request = ApprovalRequest::new( + "file_write".into(), + serde_json::json!({"path": "test.txt"}), + "test", + ); + + let result = mgr.request_approval(&adapter, &request).await; + assert_eq!(result, ApprovalResponse::Timeout); + + let log = mgr.audit_log(); + assert_eq!(log.len(), 1); + assert_eq!(log[0].decision, ApprovalResponse::Timeout); + } + + #[tokio::test] + async fn request_approval_deny_flow() { + use super::adapter::tests::MockApprovalAdapter; + + let config = supervised_config(); + let mgr = ApprovalManager::from_config(&config); + let adapter = MockApprovalAdapter::new(ApprovalResponse::No); + + let request = ApprovalRequest::new( + "file_write".into(), + serde_json::json!({"path": "test.txt"}), + "test", + ); + + let result = mgr.request_approval(&adapter, &request).await; + assert_eq!(result, ApprovalResponse::No); + assert!(mgr.needs_approval("file_write")); + } } diff --git a/src/channels/mod.rs b/src/channels/mod.rs index c03a90c7..4db032c4 100644 --- a/src/channels/mod.rs +++ b/src/channels/mod.rs @@ -2833,6 +2833,12 @@ async fn process_channel_message( let cost_tracking_context = ctx.cost_tracking.clone().map(|state| { crate::agent::loop_::ToolLoopCostTrackingContext::new(state.tracker, state.prices) }); + // Create a channel-specific approval adapter for interactive approval prompts. + let channel_approval_adapter: Option> = + target_channel + .as_ref() + .and_then(|ch| ch.create_approval_adapter(&msg)); + let llm_call_start = Instant::now(); #[allow(clippy::cast_possible_truncation)] let elapsed_before_llm_ms = started_at.elapsed().as_millis() as u64; @@ -2857,6 +2863,7 @@ async fn process_channel_message( route.temperature.unwrap_or(runtime_defaults.temperature), true, Some(&*ctx.approval_manager), + channel_approval_adapter.as_deref(), msg.channel.as_str(), Some(msg.reply_target.as_str()), &ctx.multimodal, @@ -4288,7 +4295,7 @@ pub async fn start_channels(config: Config) -> Result<()> { } else { None }, - approval_manager: Arc::new(ApprovalManager::for_non_interactive(&config.autonomy)), + approval_manager: Arc::new(ApprovalManager::from_config(&config.autonomy)), activated_tools: ch_activated_handle, cost_tracking: crate::cost::CostTracker::get_or_init_global( config.cost.clone(), diff --git a/src/channels/slack.rs b/src/channels/slack.rs index 40f73fff..0c660f10 100644 --- a/src/channels/slack.rs +++ b/src/channels/slack.rs @@ -4004,6 +4004,24 @@ impl Channel for SlackChannel { } Ok(()) } + + fn create_approval_adapter( + &self, + msg: &ChannelMessage, + ) -> Option> { + // Extract channel_id from the message's reply_target or sender + // The channel_id is embedded in the message id: "slack_{channel_id}_{ts}" + let channel_id = if let Some(stripped) = msg.id.strip_prefix("slack_") { + stripped.split('_').next().unwrap_or(&msg.reply_target) + } else { + &msg.reply_target + }; + Some(Box::new(crate::approval::SlackApprovalAdapter::new( + self.bot_token.clone(), + channel_id, + msg.thread_ts.clone(), + ))) + } } #[cfg(test)] diff --git a/src/channels/telegram.rs b/src/channels/telegram.rs index 2158b028..69e88961 100644 --- a/src/channels/telegram.rs +++ b/src/channels/telegram.rs @@ -3022,6 +3022,24 @@ Ensure only one `mentat` process is using this bot token." } Ok(()) } + + fn create_approval_adapter( + &self, + msg: &ChannelMessage, + ) -> Option> { + // Extract chat_id from reply_target (format: "chat_id" or "chat_id:thread_id") + let (chat_id, thread_id) = match msg.reply_target.split_once(':') { + Some((chat, thread)) => (chat.to_string(), Some(thread.to_string())), + None => (msg.reply_target.clone(), None), + }; + Some(Box::new(crate::approval::TelegramApprovalAdapter::new( + self.http_client(), + self.api_base.clone(), + self.bot_token.clone(), + chat_id, + thread_id, + ))) + } } #[cfg(test)] diff --git a/src/channels/traits.rs b/src/channels/traits.rs index 384982c9..99013abd 100644 --- a/src/channels/traits.rs +++ b/src/channels/traits.rs @@ -211,6 +211,17 @@ pub trait Channel: Send + Sync { ) -> anyhow::Result<()> { Ok(()) } + + /// Create a channel-specific approval adapter for interactive approval prompting. + /// + /// The `msg` provides context for the adapter (chat_id, thread_ts, etc.). + /// Returns `None` if this channel does not support interactive approval. + fn create_approval_adapter( + &self, + _msg: &ChannelMessage, + ) -> Option> { + None + } } #[cfg(test)] diff --git a/src/config/schema.rs b/src/config/schema.rs index dbb73308..0bdf683f 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -5121,6 +5121,10 @@ pub struct AutonomyConfig { #[serde(default)] pub allowed_roots: Vec, + /// Timeout in seconds for approval requests before auto-denying. Default: `120`. + #[serde(default = "default_approval_timeout_secs")] + pub approval_timeout_secs: u64, + /// Tools to exclude from non-CLI channels (e.g. Telegram, Discord). /// /// When a tool is listed here, non-CLI channels will not expose it to the @@ -5147,6 +5151,10 @@ fn default_always_ask() -> Vec { vec![] } +fn default_approval_timeout_secs() -> u64 { + 120 +} + impl AutonomyConfig { /// Merge the built-in default `auto_approve` entries into the current /// list, preserving any user-supplied additions. @@ -5220,6 +5228,7 @@ impl Default for AutonomyConfig { shell_env_passthrough: vec![], auto_approve: default_auto_approve(), always_ask: default_always_ask(), + approval_timeout_secs: default_approval_timeout_secs(), allowed_roots: Vec::new(), non_cli_excluded_tools: Vec::new(), } @@ -9670,6 +9679,7 @@ auto_save = true shell_env_passthrough: vec!["DATABASE_URL".into()], auto_approve: vec!["file_read".into()], always_ask: vec![], + approval_timeout_secs: 120, allowed_roots: vec![], non_cli_excluded_tools: vec![], }, diff --git a/src/cron/mod.rs b/src/cron/mod.rs index 06a3fac9..2366f6f6 100644 --- a/src/cron/mod.rs +++ b/src/cron/mod.rs @@ -27,9 +27,9 @@ pub use types::{ /// /// Returns `Ok(())` if the command passes all checks, or an error describing /// why it was blocked. -pub fn validate_shell_command(config: &Config, command: &str, approved: bool) -> Result<()> { +pub fn validate_shell_command(config: &Config, command: &str) -> Result<()> { let security = SecurityPolicy::from_config(&config.autonomy, &config.workspace_dir); - validate_shell_command_with_security(&security, command, approved) + validate_shell_command_with_security(&security, command) } /// Validate a shell command using an existing `SecurityPolicy` instance. @@ -38,12 +38,14 @@ pub fn validate_shell_command(config: &Config, command: &str, approved: bool) -> pub(crate) fn validate_shell_command_with_security( security: &SecurityPolicy, command: &str, - approved: bool, ) -> Result<()> { - security - .validate_command_execution(command, approved) - .map(|_| ()) - .map_err(|reason| anyhow!("blocked by security policy: {reason}")) + let (_, needs_approval) = security + .validate_command_execution(command) + .map_err(|reason| anyhow!("blocked by security policy: {reason}"))?; + if needs_approval { + bail!("blocked by security policy: command requires explicit approval"); + } + Ok(()) } pub(crate) fn validate_delivery_config(delivery: Option<&DeliveryConfig>) -> Result<()> { @@ -89,9 +91,8 @@ pub fn add_shell_job_with_approval( schedule: Schedule, command: &str, delivery: Option, - approved: bool, ) -> Result { - validate_shell_command(config, command, approved)?; + validate_shell_command(config, command)?; validate_delivery_config(delivery.as_ref())?; store::add_shell_job(config, name, schedule, command, delivery) } @@ -103,24 +104,18 @@ pub fn update_shell_job_with_approval( config: &Config, job_id: &str, patch: CronJobPatch, - approved: bool, ) -> Result { if let Some(command) = patch.command.as_deref() { - validate_shell_command(config, command, approved)?; + validate_shell_command(config, command)?; } update_job(config, job_id, patch) } /// Create a one-shot validated shell job from a delay string (e.g. "30m"). -pub fn add_once_validated( - config: &Config, - delay: &str, - command: &str, - approved: bool, -) -> Result { +pub fn add_once_validated(config: &Config, delay: &str, command: &str) -> Result { let duration = parse_delay(delay)?; let at = chrono::Utc::now() + duration; - add_once_at_validated(config, at, command, approved) + add_once_at_validated(config, at, command) } /// Create a one-shot validated shell job at an absolute timestamp. @@ -128,13 +123,12 @@ pub fn add_once_at_validated( config: &Config, at: chrono::DateTime, command: &str, - approved: bool, ) -> Result { let schedule = Schedule::At { at }; - add_shell_job_with_approval(config, None, schedule, command, None, approved) + add_shell_job_with_approval(config, None, schedule, command, None) } -// Convenience wrappers for CLI paths (default approved=false). +// Convenience wrappers for CLI paths. pub(crate) fn add_shell_job( config: &Config, @@ -142,7 +136,7 @@ pub(crate) fn add_shell_job( schedule: Schedule, command: &str, ) -> Result { - add_shell_job_with_approval(config, name, schedule, command, None, false) + add_shell_job_with_approval(config, name, schedule, command, None) } pub(crate) fn add_job(config: &Config, expression: &str, command: &str) -> Result { @@ -420,7 +414,7 @@ pub fn handle_command(command: crate::CronCommands, config: &Config) -> Result<( ..CronJobPatch::default() }; - let job = update_shell_job_with_approval(config, &id, patch, false)?; + let job = update_shell_job_with_approval(config, &id, patch)?; println!("\u{2705} Updated cron job {}", job.id); println!(" Expr: {}", job.expression); println!(" Next: {}", job.next_run.to_rfc3339()); @@ -446,7 +440,7 @@ pub fn handle_command(command: crate::CronCommands, config: &Config) -> Result<( } pub(crate) fn add_once(config: &Config, delay: &str, command: &str) -> Result { - add_once_validated(config, delay, command, false) + add_once_validated(config, delay, command) } pub(crate) fn add_once_at( @@ -454,7 +448,7 @@ pub(crate) fn add_once_at( at: chrono::DateTime, command: &str, ) -> Result { - add_once_at_validated(config, at, command, false) + add_once_at_validated(config, at, command) } pub fn pause_job(config: &Config, id: &str) -> Result { @@ -774,7 +768,9 @@ mod tests { .contains("explicit approval") ); - let approved = add_shell_job_with_approval( + // add_shell_job_with_approval no longer accepts an approved flag; + // medium-risk commands are always blocked at this layer. + let also_denied = add_shell_job_with_approval( &config, None, Schedule::Cron { @@ -783,9 +779,8 @@ mod tests { }, "touch cron-medium-risk", None, - true, ); - assert!(approved.is_ok(), "{approved:?}"); + assert!(also_denied.is_err()); } #[test] @@ -795,6 +790,8 @@ mod tests { config.autonomy.allowed_commands = vec!["echo".into(), "touch".into()]; let job = make_job(&config, "*/5 * * * *", None, "echo original"); + // update_shell_job_with_approval no longer accepts an approved flag; + // medium-risk commands are always blocked at this layer. let denied = update_shell_job_with_approval( &config, &job.id, @@ -802,7 +799,6 @@ mod tests { command: Some("touch cron-medium-risk-update".into()), ..CronJobPatch::default() }, - false, ); assert!(denied.is_err()); assert!( @@ -811,18 +807,6 @@ mod tests { .to_string() .contains("explicit approval") ); - - let approved = update_shell_job_with_approval( - &config, - &job.id, - CronJobPatch { - command: Some("touch cron-medium-risk-update".into()), - ..CronJobPatch::default() - }, - true, - ) - .unwrap(); - assert_eq!(approved.command, "touch cron-medium-risk-update"); } #[test] @@ -855,7 +839,7 @@ mod tests { let tmp = TempDir::new().unwrap(); let config = test_config(&tmp); - let job = add_once_validated(&config, "1h", "echo one-shot", false).unwrap(); + let job = add_once_validated(&config, "1h", "echo one-shot").unwrap(); assert_eq!(job.command, "echo one-shot"); assert!(matches!(job.schedule, Schedule::At { .. })); } @@ -867,7 +851,7 @@ mod tests { config.autonomy.allowed_commands = vec!["echo".into()]; config.autonomy.level = crate::security::AutonomyLevel::Supervised; - let result = add_once_validated(&config, "1h", "curl https://example.com", false); + let result = add_once_validated(&config, "1h", "curl https://example.com"); assert!(result.is_err()); assert!( result @@ -883,7 +867,7 @@ mod tests { let config = test_config(&tmp); let at = chrono::Utc::now() + chrono::Duration::hours(1); - let job = add_once_at_validated(&config, at, "echo at-shot", false).unwrap(); + let job = add_once_at_validated(&config, at, "echo at-shot").unwrap(); assert_eq!(job.command, "echo at-shot"); assert!(matches!(job.schedule, Schedule::At { .. })); } @@ -895,7 +879,9 @@ mod tests { config.autonomy.allowed_commands = vec!["echo".into(), "touch".into()]; let at = chrono::Utc::now() + chrono::Duration::hours(1); - let denied = add_once_at_validated(&config, at, "touch at-medium", false); + // add_once_at_validated no longer accepts an approved flag; + // medium-risk commands are always blocked at this layer. + let denied = add_once_at_validated(&config, at, "touch at-medium"); assert!(denied.is_err()); assert!( denied @@ -903,9 +889,6 @@ mod tests { .to_string() .contains("explicit approval") ); - - let approved = add_once_at_validated(&config, at, "touch at-medium", true); - assert!(approved.is_ok(), "{approved:?}"); } #[test] @@ -915,7 +898,7 @@ mod tests { config.autonomy.allowed_commands = vec!["echo".into()]; config.autonomy.level = crate::security::AutonomyLevel::Supervised; - // Simulate gateway API path: add_shell_job_with_approval(approved=false) + // Simulate gateway API path: add_shell_job_with_approval blocks disallowed commands let result = add_shell_job_with_approval( &config, None, @@ -925,7 +908,6 @@ mod tests { }, "curl https://example.com", None, - false, ); assert!(result.is_err()); assert!( @@ -945,8 +927,7 @@ mod tests { let security = SecurityPolicy::from_config(&config.autonomy, &config.workspace_dir); // Simulate scheduler validation path - let result = - validate_shell_command_with_security(&security, "curl https://example.com", false); + let result = validate_shell_command_with_security(&security, "curl https://example.com"); assert!(result.is_err()); assert!( result diff --git a/src/cron/scheduler.rs b/src/cron/scheduler.rs index a8ea63dd..fc0e7e2e 100644 --- a/src/cron/scheduler.rs +++ b/src/cron/scheduler.rs @@ -528,10 +528,7 @@ async fn run_job_command_with_timeout( // Jobs created via the validated helpers were already checked at creation // time, but we re-validate at execution time to catch policy changes and // manually-edited job stores. - let approved = false; // scheduler runs are never pre-approved - if let Err(error) = - crate::cron::validate_shell_command_with_security(security, &job.command, approved) - { + if let Err(error) = crate::cron::validate_shell_command_with_security(security, &job.command) { return (false, error.to_string()); } diff --git a/src/gateway/api.rs b/src/gateway/api.rs index 9109dce7..f025e8d1 100644 --- a/src/gateway/api.rs +++ b/src/gateway/api.rs @@ -332,7 +332,7 @@ pub async fn handle_api_cron_add( } }; - crate::cron::add_shell_job_with_approval(&config, name, schedule, command, delivery, false) + crate::cron::add_shell_job_with_approval(&config, name, schedule, command, delivery) }; match result { @@ -444,7 +444,7 @@ pub async fn handle_api_cron_patch( ..crate::cron::CronJobPatch::default() }; - match crate::cron::update_shell_job_with_approval(&config, &id, patch, false) { + match crate::cron::update_shell_job_with_approval(&config, &id, patch) { Ok(job) => Json(serde_json::json!({"status": "ok", "job": job})).into_response(), Err(e) => ( StatusCode::INTERNAL_SERVER_ERROR, diff --git a/src/gateway/ws.rs b/src/gateway/ws.rs index fae9365e..f37d78aa 100644 --- a/src/gateway/ws.rs +++ b/src/gateway/ws.rs @@ -525,6 +525,125 @@ async fn process_chat_message( } } +// ── GatewayApprovalAdapter ─────────────────────────────────────────────────── + +use crate::approval::{ + ApprovalRequest, ApprovalResponse, + adapter::{ChannelApprovalAdapter, PendingApproval, PlatformRef}, +}; +use async_trait::async_trait; +use std::collections::HashMap; +use std::sync::Arc; +use tokio::sync::{Mutex, oneshot}; + +/// Approval adapter for the WebSocket gateway channel. +/// +/// Sends an `approval_request` JSON frame to the connected client and waits +/// for an `approval_response` frame routed back via [`resolve_approval_response`]. +pub struct GatewayApprovalAdapter { + /// Channel for sending JSON frames to the WebSocket client. + frame_tx: tokio::sync::mpsc::Sender, + /// Registry of pending oneshot senders, keyed by request_id. + pending_responses: Arc>>>, + /// Receiver for the most recently sent approval request. + response_rx: Mutex>>, +} + +impl GatewayApprovalAdapter { + pub fn new( + frame_tx: tokio::sync::mpsc::Sender, + pending_responses: Arc>>>, + ) -> Self { + Self { + frame_tx, + pending_responses, + response_rx: Mutex::new(None), + } + } + + /// Route an `approval_response` frame from the WebSocket receive loop back + /// to the waiting [`receive_approval_response`] call. + /// + /// Returns `true` if the request_id was found and the response delivered. + pub async fn resolve_approval_response( + pending: &Arc>>>, + request_id: &str, + decision: &str, + ) -> bool { + let response = crate::approval::parse_approval_input(decision); + let mut map = pending.lock().await; + if let Some(tx) = map.remove(request_id) { + let _ = tx.send(response); + true + } else { + false + } + } +} + +#[async_trait] +impl ChannelApprovalAdapter for GatewayApprovalAdapter { + async fn send_approval_request( + &self, + request: &ApprovalRequest, + ) -> anyhow::Result { + let request_id = request.request_id.clone(); + + // Build the approval_request frame. + let frame = serde_json::json!({ + "type": "approval_request", + "request_id": request_id, + "tool_name": request.tool_name, + "arguments": request.arguments, + "risk_level": request.risk_level.as_ref().map(|r| format!("{r:?}").to_ascii_lowercase()), + "timeout_secs": 120, + }); + + // Register the oneshot sender before sending the frame to avoid a race. + let (tx, rx) = oneshot::channel::(); + { + let mut map = self.pending_responses.lock().await; + map.insert(request_id.clone(), tx); + } + + // Store the receiver so receive_approval_response can pick it up. + { + let mut slot = self.response_rx.lock().await; + *slot = Some(rx); + } + + self.frame_tx + .send(frame.to_string()) + .await + .map_err(|e| anyhow::anyhow!("failed to send approval frame: {e}"))?; + + Ok(PendingApproval { + request_id: request_id.clone(), + platform_ref: PlatformRef::Gateway { + connection_id: request_id, + }, + }) + } + + async fn receive_approval_response( + &self, + _pending: &PendingApproval, + ) -> anyhow::Result { + let rx = { + let mut slot = self.response_rx.lock().await; + slot.take() + .ok_or_else(|| anyhow::anyhow!("no pending approval receiver"))? + }; + + rx.await + .map_err(|_| anyhow::anyhow!("approval channel closed before response arrived")) + } + + fn channel_name(&self) -> &str { + "gateway" + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/security/mod.rs b/src/security/mod.rs index d803b26f..1df116e3 100644 --- a/src/security/mod.rs +++ b/src/security/mod.rs @@ -59,7 +59,7 @@ pub use estop::{EstopLevel, EstopManager, EstopState, ResumeSelector}; pub use otp::OtpValidator; #[allow(unused_imports)] pub use pairing::PairingGuard; -pub use policy::{AutonomyLevel, SecurityPolicy}; +pub use policy::{AutonomyLevel, CommandRiskLevel, SecurityPolicy}; #[allow(unused_imports)] pub use secrets::SecretStore; #[allow(unused_imports)] diff --git a/src/security/policy.rs b/src/security/policy.rs index fe2ee2c5..5e682ed7 100644 --- a/src/security/policy.rs +++ b/src/security/policy.rs @@ -18,7 +18,8 @@ pub enum AutonomyLevel { } /// Risk score for shell command execution. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize, serde::Deserialize)] +#[serde(rename_all = "lowercase")] pub enum CommandRiskLevel { Low, Medium, @@ -838,12 +839,16 @@ impl SecurityPolicy { // This ordering ensures deny-by-default: unknown commands are rejected // before any risk or autonomy logic runs. - /// Validate full command execution policy (allowlist + risk gate). + /// Validate command execution policy (allowlist + risk classification). + /// + /// Returns `(risk_level, needs_approval)` where `needs_approval` is `true` + /// when the command requires user approval before execution. Hard-blocked + /// commands (high-risk + `block_high_risk_commands` + not explicitly allowed) + /// return `Err` — they cannot be overridden even with approval. pub fn validate_command_execution( &self, command: &str, - approved: bool, - ) -> Result { + ) -> Result<(CommandRiskLevel, bool), String> { if !self.is_command_allowed(command) { return Err(format!("Command not allowed by security policy: {command}")); } @@ -856,32 +861,26 @@ impl SecurityPolicy { // autonomy gates entirely. See #4485. let has_wildcard = self.allowed_commands.iter().any(|c| c.trim() == "*"); if has_wildcard && !self.block_high_risk_commands { - return Ok(risk); + return Ok((risk, false)); } if risk == CommandRiskLevel::High { if self.block_high_risk_commands && !self.is_command_explicitly_allowed(command) { return Err("Command blocked: high-risk command is disallowed by policy".into()); } - if self.autonomy == AutonomyLevel::Supervised && !approved { - return Err( - "Command requires explicit approval (approved=true): high-risk operation" - .into(), - ); + if self.autonomy == AutonomyLevel::Supervised { + return Ok((risk, true)); } } if risk == CommandRiskLevel::Medium && self.autonomy == AutonomyLevel::Supervised && self.require_approval_for_medium_risk - && !approved { - return Err( - "Command requires explicit approval (approved=true): medium-risk operation".into(), - ); + return Ok((risk, true)); } - Ok(risk) + Ok((risk, false)) } /// Returns unquoted segments that are NOT comments (i.e. not starting with `#`). @@ -1645,7 +1644,7 @@ mod tests { assert!(p.is_command_allowed("/usr/bin/antigravity")); // Wildcard still respects risk gates in validate_command_execution. - let blocked = p.validate_command_execution("rm -rf /tmp/test", true); + let blocked = p.validate_command_execution("rm -rf /tmp/test"); assert!(blocked.is_err()); assert!(blocked.unwrap_err().contains("high-risk")); } @@ -1734,12 +1733,8 @@ mod tests { ..SecurityPolicy::default() }; - let denied = p.validate_command_execution("touch test.txt", false); - assert!(denied.is_err()); - assert!(denied.unwrap_err().contains("requires explicit approval"),); - - let allowed = p.validate_command_execution("touch test.txt", true); - assert_eq!(allowed.unwrap(), CommandRiskLevel::Medium); + let result = p.validate_command_execution("touch test.txt"); + assert_eq!(result.unwrap(), (CommandRiskLevel::Medium, true)); } #[test] @@ -1753,7 +1748,7 @@ mod tests { ..SecurityPolicy::default() }; - let result = p.validate_command_execution("rm -rf /tmp/test", true); + let result = p.validate_command_execution("rm -rf /tmp/test"); assert!(result.is_err()); assert!(result.unwrap_err().contains("high-risk")); } @@ -1770,8 +1765,8 @@ mod tests { ..SecurityPolicy::default() }; - let result = p.validate_command_execution("curl https://api.example.com/data", true); - assert_eq!(result.unwrap(), CommandRiskLevel::High); + let result = p.validate_command_execution("curl https://api.example.com/data"); + assert_eq!(result.unwrap(), (CommandRiskLevel::High, false)); } #[test] @@ -1783,9 +1778,8 @@ mod tests { ..SecurityPolicy::default() }; - let result = - p.validate_command_execution("wget https://releases.example.com/v1.tar.gz", true); - assert_eq!(result.unwrap(), CommandRiskLevel::High); + let result = p.validate_command_execution("wget https://releases.example.com/v1.tar.gz"); + assert_eq!(result.unwrap(), (CommandRiskLevel::High, false)); } #[test] @@ -1798,7 +1792,7 @@ mod tests { ..SecurityPolicy::default() }; - let result = p.validate_command_execution("wget https://evil.com", true); + let result = p.validate_command_execution("wget https://evil.com"); assert!(result.is_err()); assert!(result.unwrap_err().contains("not allowed")); } @@ -1813,15 +1807,14 @@ mod tests { ..SecurityPolicy::default() }; - let result = p.validate_command_execution("rm -rf /tmp/test", true); - assert_eq!(result.unwrap(), CommandRiskLevel::High); + let result = p.validate_command_execution("rm -rf /tmp/test"); + assert_eq!(result.unwrap(), (CommandRiskLevel::High, false)); } #[test] fn validate_command_high_risk_still_needs_approval_in_supervised() { - // Even when explicitly allowed, supervised mode still requires - // approval for high-risk commands (the approval gate is separate - // from the block gate). + // Even when explicitly allowed, supervised mode returns needs_approval=true + // for high-risk commands. let p = SecurityPolicy { autonomy: AutonomyLevel::Supervised, allowed_commands: vec!["curl".into()], @@ -1829,12 +1822,8 @@ mod tests { ..SecurityPolicy::default() }; - let denied = p.validate_command_execution("curl https://api.example.com", false); - assert!(denied.is_err()); - assert!(denied.unwrap_err().contains("requires explicit approval")); - - let allowed = p.validate_command_execution("curl https://api.example.com", true); - assert_eq!(allowed.unwrap(), CommandRiskLevel::High); + let result = p.validate_command_execution("curl https://api.example.com"); + assert_eq!(result.unwrap(), (CommandRiskLevel::High, true)); } #[test] @@ -1848,8 +1837,8 @@ mod tests { ..SecurityPolicy::default() }; - let result = p.validate_command_execution("curl https://api.example.com | grep data", true); - assert_eq!(result.unwrap(), CommandRiskLevel::High); + let result = p.validate_command_execution("curl https://api.example.com | grep data"); + assert_eq!(result.unwrap(), (CommandRiskLevel::High, false)); } #[test] @@ -1861,14 +1850,14 @@ mod tests { ..SecurityPolicy::default() }; - let result = p.validate_command_execution("touch test.txt", false); - assert_eq!(result.unwrap(), CommandRiskLevel::Medium); + let result = p.validate_command_execution("touch test.txt"); + assert_eq!(result.unwrap(), (CommandRiskLevel::Medium, false)); } #[test] fn validate_command_rejects_background_chain_bypass() { let p = default_policy(); - let result = p.validate_command_execution("ls & python3 -c 'print(1)'", false); + let result = p.validate_command_execution("ls & python3 -c 'print(1)'"); assert!(result.is_err()); assert!(result.unwrap_err().contains("not allowed")); } @@ -3184,15 +3173,9 @@ mod tests { workspace_only: false, ..SecurityPolicy::default() }; - assert!( - p.validate_command_execution("rm -rf /tmp/test", true) - .is_ok() - ); - assert!(p.validate_command_execution("nohup firefox", true).is_ok()); - assert!( - p.validate_command_execution("ls /usr/bin/firefox", true) - .is_ok() - ); + assert!(p.validate_command_execution("rm -rf /tmp/test").is_ok()); + assert!(p.validate_command_execution("nohup firefox").is_ok()); + assert!(p.validate_command_execution("ls /usr/bin/firefox").is_ok()); } #[test] @@ -3205,7 +3188,7 @@ mod tests { block_high_risk_commands: true, ..SecurityPolicy::default() }; - let result = p.validate_command_execution("rm -rf /tmp/test", true); + let result = p.validate_command_execution("rm -rf /tmp/test"); assert!(result.is_err()); assert!(result.unwrap_err().contains("high-risk")); } diff --git a/src/tools/cron_add.rs b/src/tools/cron_add.rs index 9f70a17f..625c4333 100644 --- a/src/tools/cron_add.rs +++ b/src/tools/cron_add.rs @@ -76,11 +76,6 @@ impl Tool for CronAddTool { "type": "boolean", "description": "If true, the job is automatically deleted after its first successful run. Defaults to true for 'at' schedules." }, - "approved": { - "type": "boolean", - "description": "Set true to explicitly approve medium/high-risk shell commands in supervised mode", - "default": false - } }, "required": ["schedule", "job_type"] }) @@ -144,10 +139,6 @@ impl Tool for CronAddTool { .get("delete_after_run") .and_then(serde_json::Value::as_bool) .unwrap_or(default_delete_after_run); - let approved = args - .get("approved") - .and_then(serde_json::Value::as_bool) - .unwrap_or(false); let delivery = match args.get("delivery") { Some(v) => match serde_json::from_value::(v.clone()) { Ok(cfg) => Some(cfg), @@ -175,26 +166,11 @@ impl Tool for CronAddTool { } }; - if let Err(reason) = self.security.validate_command_execution(command, approved) { - return Ok(ToolResult { - success: false, - output: String::new(), - error: Some(reason), - }); - } - if let Some(blocked) = enforce_security_policy(&self.security, "cron_add") { return Ok(blocked); } - cron::add_shell_job_with_approval( - &self.config, - name, - schedule, - command, - delivery, - approved, - ) + cron::add_shell_job_with_approval(&self.config, name, schedule, command, delivery) } JobType::Agent => { let prompt = match args.get("prompt").and_then(serde_json::Value::as_str) { @@ -480,16 +456,8 @@ mod tests { .contains("explicit approval") ); - let approved = tool - .execute(json!({ - "schedule": { "kind": "cron", "expr": "*/5 * * * *" }, - "job_type": "shell", - "command": "touch cron-approval-test", - "approved": true - })) - .await - .unwrap(); - assert!(approved.success, "{:?}", approved.error); + // The approved flag has been removed; medium-risk commands are always + // blocked at this layer regardless. } #[tokio::test] diff --git a/src/tools/cron_run.rs b/src/tools/cron_run.rs index 27fc9cda..1cffee76 100644 --- a/src/tools/cron_run.rs +++ b/src/tools/cron_run.rs @@ -33,12 +33,7 @@ impl Tool for CronRunTool { "type": "object", "additionalProperties": false, "properties": { - "job_id": { "type": "string" }, - "approved": { - "type": "boolean", - "description": "Set true to explicitly approve medium/high-risk shell commands in supervised mode", - "default": false - } + "job_id": { "type": "string" } }, "required": ["job_id"] }) @@ -63,11 +58,6 @@ impl Tool for CronRunTool { }); } }; - let approved = args - .get("approved") - .and_then(serde_json::Value::as_bool) - .unwrap_or(false); - if !self.security.can_act() { return Ok(ToolResult { success: false, @@ -96,15 +86,22 @@ impl Tool for CronRunTool { }; if matches!(job.job_type, JobType::Shell) { - if let Err(reason) = self - .security - .validate_command_execution(&job.command, approved) - { - return Ok(ToolResult { - success: false, - output: String::new(), - error: Some(reason), - }); + match self.security.validate_command_execution(&job.command) { + Ok((_, true)) => { + return Ok(ToolResult { + success: false, + output: String::new(), + error: Some("Command requires explicit approval".into()), + }); + } + Err(reason) => { + return Ok(ToolResult { + success: false, + output: String::new(), + error: Some(reason), + }); + } + Ok((_, false)) => {} } } @@ -236,9 +233,19 @@ mod tests { config.autonomy.allowed_commands = vec!["touch".into()]; std::fs::create_dir_all(&config.workspace_dir).unwrap(); let cfg = Arc::new(config); - // Create with explicit approval so the job persists for the run test. + // Create the job using Full autonomy config so medium-risk is not blocked at creation. + let full_cfg = Arc::new(Config { + workspace_dir: cfg.workspace_dir.clone(), + config_path: cfg.config_path.clone(), + autonomy: crate::config::AutonomyConfig { + level: AutonomyLevel::Full, + allowed_commands: vec!["touch".into()], + ..crate::config::AutonomyConfig::default() + }, + ..Config::default() + }); let job = cron::add_shell_job_with_approval( - &cfg, + &full_cfg, None, cron::Schedule::Cron { expr: "*/5 * * * *".into(), @@ -246,7 +253,6 @@ mod tests { }, "touch cron-run-approval", None, - true, ) .unwrap(); let tool = CronRunTool::new(cfg.clone(), test_security(&cfg)); diff --git a/src/tools/cron_update.rs b/src/tools/cron_update.rs index 708fa287..a9c11365 100644 --- a/src/tools/cron_update.rs +++ b/src/tools/cron_update.rs @@ -80,11 +80,6 @@ impl Tool for CronUpdateTool { "delivery": delivery_json_schema() } }, - "approved": { - "type": "boolean", - "description": "Set true to explicitly approve medium/high-risk shell commands in supervised mode", - "default": false - } }, "required": ["job_id", "patch"] }) @@ -131,16 +126,11 @@ impl Tool for CronUpdateTool { }); } }; - let approved = args - .get("approved") - .and_then(serde_json::Value::as_bool) - .unwrap_or(false); - if let Some(blocked) = enforce_security_policy(&self.security, "cron_update") { return Ok(blocked); } - match cron::update_shell_job_with_approval(&self.config, job_id, patch, approved) { + match cron::update_shell_job_with_approval(&self.config, job_id, patch) { Ok(job) => Ok(ToolResult { success: true, output: serde_json::to_string_pretty(&job)?, @@ -301,6 +291,8 @@ mod tests { let job = cron::add_job(&cfg, "*/5 * * * *", "echo ok").unwrap(); let tool = CronUpdateTool::new(cfg.clone(), test_security(&cfg)); + // The approved flag has been removed; medium-risk commands are always + // blocked at this layer regardless. let denied = tool .execute(json!({ "job_id": job.id, @@ -315,16 +307,6 @@ mod tests { .unwrap_or_default() .contains("explicit approval") ); - - let approved = tool - .execute(json!({ - "job_id": job.id, - "patch": { "command": "touch cron-update-approval-test" }, - "approved": true - })) - .await - .unwrap(); - assert!(approved.success, "{:?}", approved.error); } #[test] diff --git a/src/tools/delegate.rs b/src/tools/delegate.rs index 6ca17884..db07c4a5 100644 --- a/src/tools/delegate.rs +++ b/src/tools/delegate.rs @@ -1127,6 +1127,7 @@ impl DelegateTool { temperature, true, None, + None, // approval_adapter "delegate", None, &self.multimodal_config, diff --git a/src/tools/schedule.rs b/src/tools/schedule.rs index a90cf484..cd8e1d9d 100644 --- a/src/tools/schedule.rs +++ b/src/tools/schedule.rs @@ -60,11 +60,6 @@ impl Tool for ScheduleTool { "type": "string", "description": "Shell command to execute. Required for create/add/once." }, - "approved": { - "type": "boolean", - "description": "Set true to explicitly approve medium/high-risk shell commands in supervised mode", - "default": false - }, "id": { "type": "string", "description": "Task ID. Required for get/cancel/remove/pause/resume." @@ -83,13 +78,7 @@ impl Tool for ScheduleTool { let id = require_str!(args, "id"); self.handle_get(id) } - "create" | "add" | "once" => { - let approved = args - .get("approved") - .and_then(serde_json::Value::as_bool) - .unwrap_or(false); - self.handle_create_like(action, &args, approved) - } + "create" | "add" | "once" => self.handle_create_like(action, &args), "cancel" | "remove" => { if let Some(blocked) = self.enforce_mutation_allowed(action) { return Ok(blocked); @@ -207,12 +196,7 @@ impl ScheduleTool { } } - fn handle_create_like( - &self, - action: &str, - args: &serde_json::Value, - approved: bool, - ) -> Result { + fn handle_create_like(&self, action: &str, args: &serde_json::Value) -> Result { let command = require_str!(args, "command"); if command.trim().is_empty() { return ToolResult::err("Missing required parameter 'command'"); @@ -284,7 +268,6 @@ impl ScheduleTool { }, command, None, - approved, ) { Ok(job) => job, Err(error) => { @@ -309,7 +292,7 @@ impl ScheduleTool { } if let Some(value) = delay { - let job = match cron::add_once_validated(&self.config, value, command, approved) { + let job = match cron::add_once_validated(&self.config, value, command) { Ok(job) => job, Err(error) => { return Ok(ToolResult { @@ -336,8 +319,7 @@ impl ScheduleTool { .map_err(|error| anyhow::anyhow!("Invalid run_at timestamp: {error}"))? .with_timezone(&Utc); - let job = match cron::add_once_at_validated(&self.config, run_at_parsed, command, approved) - { + let job = match cron::add_once_at_validated(&self.config, run_at_parsed, command) { Ok(job) => job, Err(error) => { return Ok(ToolResult { @@ -767,15 +749,7 @@ mod tests { .contains("explicit approval") ); - let approved = tool - .execute(json!({ - "action": "create", - "expression": "*/5 * * * *", - "command": "touch schedule-policy-test", - "approved": true - })) - .await - .unwrap(); - assert!(approved.success, "{:?}", approved.error); + // The approved flag has been removed; medium-risk commands are always + // blocked at this layer regardless. } } diff --git a/src/tools/shell.rs b/src/tools/shell.rs index cd2fc156..60290d00 100644 --- a/src/tools/shell.rs +++ b/src/tools/shell.rs @@ -131,11 +131,6 @@ impl Tool for ShellTool { "command": { "type": "string", "description": "The shell command to execute. Runs via /bin/sh -c on Unix." - }, - "approved": { - "type": "boolean", - "description": "Set true to explicitly approve medium/high-risk commands in supervised mode. Required for commands that modify files, install packages, or access the network.", - "default": false } }, "required": ["command"] @@ -144,10 +139,6 @@ impl Tool for ShellTool { async fn execute(&self, args: serde_json::Value) -> anyhow::Result { let command = require_str!(args, "command"); - let approved = args - .get("approved") - .and_then(|v| v.as_bool()) - .unwrap_or(false); if self.security.is_rate_limited() { return Ok(ToolResult { @@ -157,7 +148,7 @@ impl Tool for ShellTool { }); } - match self.security.validate_command_execution(command, approved) { + match self.security.validate_command_execution(command) { Ok(_) => {} Err(reason) => { return Ok(ToolResult { @@ -309,7 +300,8 @@ mod tests { .expect("schema required field should be an array") .contains(&json!("command")) ); - assert!(schema["properties"]["approved"].is_object()); + // approved param was removed — risk is now harness-enforced + assert!(schema["properties"]["approved"].is_null()); } #[tokio::test] @@ -614,28 +606,17 @@ mod tests { ..SecurityPolicy::default() }); + // validate_command_execution now returns (Medium, needs_approval=true) + // but the shell tool itself proceeds — the approval gate is in the + // agent loop, not here. The tool only blocks on hard policy errors. let tool = ShellTool::new(security.clone(), test_runtime()); - let denied = tool + let result = tool .execute(json!({"command": "touch mentat_shell_approval_test"})) .await - .expect("unapproved command should return a result"); - assert!(!denied.success); - assert!( - denied - .error - .as_deref() - .unwrap_or("") - .contains("explicit approval") - ); - - let allowed = tool - .execute(json!({ - "command": "touch mentat_shell_approval_test", - "approved": true - })) - .await - .expect("approved command execution should succeed"); - assert!(allowed.success); + .expect("medium-risk command should return a result"); + // The tool succeeds because validate_command_execution returns Ok; + // the harness approval gate would have blocked it before reaching here. + assert!(result.success); let _ = tokio::fs::remove_file(std::env::temp_dir().join("mentat_shell_approval_test")).await; diff --git a/src/tools/skill_tool.rs b/src/tools/skill_tool.rs index 8bebd1c7..d67fc49a 100644 --- a/src/tools/skill_tool.rs +++ b/src/tools/skill_tool.rs @@ -116,10 +116,15 @@ impl Tool for SkillShellTool { }); } - // Security validation — always requires explicit approval (approved=true) - // since skill tools are user-defined and should be treated as medium-risk. - match self.security.validate_command_execution(&command, true) { - Ok(_) => {} + // Security validation for skill tools (user-defined, treated as medium-risk). + match self.security.validate_command_execution(&command) { + Ok((_, needs_approval)) if needs_approval => { + return Ok(ToolResult { + success: false, + output: String::new(), + error: Some("Command requires explicit approval".into()), + }); + } Err(reason) => { return Ok(ToolResult { success: false, @@ -127,6 +132,7 @@ impl Tool for SkillShellTool { error: Some(reason), }); } + Ok(_) => {} } if let Some(path) = self.security.forbidden_path_argument(&command) {