diff --git a/crates/goose-cli/src/session/mod.rs b/crates/goose-cli/src/session/mod.rs index 08122797..1d96ccee 100644 --- a/crates/goose-cli/src/session/mod.rs +++ b/crates/goose-cli/src/session/mod.rs @@ -628,17 +628,15 @@ impl Session { output::hide_thinking(); // Format the confirmation prompt - let prompt = "Goose would like to call the above tool, do you approve?".to_string(); + let prompt = "Goose would like to call the above tool, do you allow?".to_string(); // Get confirmation from user - let confirmed = cliclack::confirm(prompt).initial_value(true).interact()?; - let permission = if confirmed { - Permission::AllowOnce - } else { - Permission::DenyOnce - }; + let permission = cliclack::select(prompt) + .item(Permission::AllowOnce, "Allow", "Allow the tool call once") + .item(Permission::AlwaysAllow, "Always Allow", "Always allow the tool call") + .item(Permission::DenyOnce, "Deny", "Deny the tool call") + .interact()?; self.agent.handle_confirmation(confirmation.id.clone(), PermissionConfirmation { - principal_name: "tool_name_placeholder".to_string(), principal_type: PrincipalType::Tool, permission, },).await; @@ -656,7 +654,6 @@ impl Session { Permission::DenyOnce }; self.agent.handle_confirmation(enable_extension_request.id.clone(), PermissionConfirmation { - principal_name: "extension_name_placeholder".to_string(), principal_type: PrincipalType::Extension, permission, },).await; diff --git a/crates/goose-server/src/routes/reply.rs b/crates/goose-server/src/routes/reply.rs index 06447299..da3e0e05 100644 --- a/crates/goose-server/src/routes/reply.rs +++ b/crates/goose-server/src/routes/reply.rs @@ -398,7 +398,6 @@ async fn confirm_handler( .handle_confirmation( request.id.clone(), PermissionConfirmation { - principal_name: "tool_name_placeholder".to_string(), principal_type: PrincipalType::Tool, permission, }, diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index d3b05776..5852ab8b 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -11,11 +11,11 @@ use tracing::{debug, error, instrument, warn}; use crate::agents::extension::{ExtensionConfig, ExtensionResult, ToolInfo}; use crate::agents::extension_manager::{get_parameter_names, ExtensionManager}; use crate::agents::types::ToolResultReceiver; -use crate::config::{Config, ExtensionConfigManager}; +use crate::config::permission::PermissionLevel; +use crate::config::{Config, ExtensionConfigManager, PermissionManager}; use crate::message::{Message, MessageContent, ToolRequest}; -use crate::permission::{ - detect_read_only_tools, Permission, PermissionConfirmation, ToolPermissionStore, -}; +use crate::permission::permission_judge::check_tool_permissions; +use crate::permission::{Permission, PermissionConfirmation}; use crate::providers::base::Provider; use crate::providers::errors::ProviderError; use crate::providers::toolshim::{ @@ -331,16 +331,18 @@ impl Agent { tools.push(platform_tools::search_available_extensions_tool()); tools.push(platform_tools::enable_extension_tool()); - let (tools_with_readonly_annotation, tools_without_annotation): (Vec, Vec) = - tools.iter().fold((vec![], vec![]), |mut acc, tool| { + let (tools_with_readonly_annotation, tools_without_annotation): ( + HashSet, + HashSet, + ) = tools + .iter() + .fold((HashSet::new(), HashSet::new()), |mut acc, tool| { match &tool.annotations { - Some(annotations) => { - if annotations.read_only_hint { - acc.0.push(tool.name.clone()); - } + Some(annotations) if annotations.read_only_hint => { + acc.0.insert(tool.name.clone()); } - None => { - acc.1.push(tool.name.clone()); + _ => { + acc.1.insert(tool.name.clone()); } } acc @@ -481,53 +483,22 @@ impl Agent { // If there are install extension requests, always require confirmation // or if goose_mode is approve or smart_approve, check permissions for all tools if !enable_extension_requests.is_empty() || mode.as_str() == "approve" || mode.as_str() == "smart_approve" { - let mut needs_confirmation = Vec::<&ToolRequest>::new(); - let mut approved_tools = Vec::new(); - let mut llm_detect_candidates = Vec::<&ToolRequest>::new(); - let mut detected_read_only_tools = Vec::new(); + let mut permission_manager = PermissionManager::default(); + // Skip the platform tools + remaining_requests.retain(|req| { + if let Ok(tool_call) = &req.tool_call { + !tool_call.name.starts_with("platform__") + } else { + true // If there's an error (Err), don't skip the request + } + }); + let permission_check_result = check_tool_permissions(remaining_requests, + &mode, + tools_with_readonly_annotation.clone(), + tools_without_annotation.clone(), + &mut permission_manager, + self.provider()).await; - // If approve mode or smart approve mode, check permissions for all tools - if mode.as_str() == "approve" || mode.as_str() == "smart_approve" { - let store = ToolPermissionStore::load()?; - for request in &non_enable_extension_requests { - if let Ok(tool_call) = request.tool_call.clone() { - // Regular permission checking for other tools - if tools_with_readonly_annotation.contains(&tool_call.name) { - approved_tools.push((request.id.clone(), tool_call)); - } else if let Some(allowed) = store.check_permission(request) { - if allowed { - // Instead of executing immediately, collect approved tools - approved_tools.push((request.id.clone(), tool_call)); - } else { - // If the tool doesn't have any annotation, we can use llm-as-a-judge to check permission. - if tools_without_annotation.contains(&tool_call.name) { - llm_detect_candidates.push(request); - } - needs_confirmation.push(request); - } - } else { - if tools_without_annotation.contains(&tool_call.name) { - llm_detect_candidates.push(request); - } - needs_confirmation.push(request); - } - } - } - } - // Only check read-only status for tools needing confirmation - if !llm_detect_candidates.is_empty() && mode == "smart_approve" { - detected_read_only_tools = detect_read_only_tools(self.provider(), llm_detect_candidates.clone()).await; - // Remove install extensions from read-only tools - if !enable_extension_requests.is_empty() { - detected_read_only_tools.retain(|tool_name| { - !enable_extension_requests.iter().any(|req| { - req.tool_call.as_ref() - .map(|call| call.name == *tool_name) - .unwrap_or(false) - }) - }); - } - } // Handle pre-approved and read-only tools in parallel let mut tool_futures = Vec::new(); @@ -562,44 +533,58 @@ impl Agent { } } - // Process read-only tools - for request in &needs_confirmation { + // Skip the confirmation for approved tools + for request in &permission_check_result.approved { if let Ok(tool_call) = request.tool_call.clone() { let is_frontend_tool = self.is_frontend_tool(&tool_call.name); - // Skip confirmation if the tool_call.name is in the read_only_tools list - if detected_read_only_tools.contains(&tool_call.name) { - let tool_future = Self::create_tool_future(&extension_manager, tool_call, is_frontend_tool, request.id.clone()); - tool_futures.push(tool_future); - } else { - let confirmation = Message::user().with_tool_confirmation_request( - request.id.clone(), - tool_call.name.clone(), - tool_call.arguments.clone(), - Some("Goose would like to call the above tool. Allow? (y/n):".to_string()), - ); - yield confirmation; + let tool_future = Self::create_tool_future(&extension_manager, tool_call, is_frontend_tool, request.id.clone()); + tool_futures.push(tool_future); + } + } - // Wait for confirmation response through the channel - let mut rx = self.confirmation_rx.lock().await; - while let Some((req_id, tool_confirmation)) = rx.recv().await { - if req_id == request.id { - let confirmed = tool_confirmation.permission == Permission::AllowOnce || tool_confirmation.permission == Permission::AlwaysAllow; - if confirmed { - // Add this tool call to the futures collection - let tool_future = Self::create_tool_future(&extension_manager, tool_call, is_frontend_tool, request.id.clone()); - tool_futures.push(tool_future); - } else { - // User declined - add declined response - message_tool_response = message_tool_response.with_tool_response( - request.id.clone(), - Ok(vec![Content::text( - "The user has declined to run this tool. \ - DO NOT attempt to call this tool again. \ - If there are no alternative methods to proceed, clearly explain the situation and STOP.")]), - ); + let denied_content_text = Content::text( + "The user has declined to run this tool. \ + DO NOT attempt to call this tool again. \ + If there are no alternative methods to proceed, clearly explain the situation and STOP."); + for request in &permission_check_result.denied { + message_tool_response = message_tool_response.with_tool_response( + request.id.clone(), + Ok(vec![denied_content_text.clone()]), + ); + } + + // Process read-only tools + for request in &permission_check_result.needs_approval { + if let Ok(tool_call) = request.tool_call.clone() { + let is_frontend_tool = self.is_frontend_tool(&tool_call.name); + let confirmation = Message::user().with_tool_confirmation_request( + request.id.clone(), + tool_call.name.clone(), + tool_call.arguments.clone(), + Some("Goose would like to call the above tool. Allow? (y/n):".to_string()), + ); + yield confirmation; + + // Wait for confirmation response through the channel + let mut rx = self.confirmation_rx.lock().await; + while let Some((req_id, tool_confirmation)) = rx.recv().await { + if req_id == request.id { + let confirmed = tool_confirmation.permission == Permission::AllowOnce || tool_confirmation.permission == Permission::AlwaysAllow; + if confirmed { + // Add this tool call to the futures collection + let tool_future = Self::create_tool_future(&extension_manager, tool_call.clone(), is_frontend_tool, request.id.clone()); + tool_futures.push(tool_future); + if tool_confirmation.permission == Permission::AlwaysAllow { + permission_manager.update_user_permission(&tool_call.name, PermissionLevel::AlwaysAllow); } - break; // Exit the loop once the matching `req_id` is found + } else { + // User declined - add declined response + message_tool_response = message_tool_response.with_tool_response( + request.id.clone(), + Ok(vec![denied_content_text.clone()]), + ); } + break; // Exit the loop once the matching `req_id` is found } } } diff --git a/crates/goose/src/config/permission.rs b/crates/goose/src/config/permission.rs index ba259326..39757d09 100644 --- a/crates/goose/src/config/permission.rs +++ b/crates/goose/src/config/permission.rs @@ -1,6 +1,5 @@ use super::APP_STRATEGY; use etcetera::{choose_app_strategy, AppStrategy}; -use once_cell::sync::OnceCell; use serde::{Deserialize, Serialize}; use std::collections::HashMap; use std::fs; @@ -31,9 +30,6 @@ pub struct PermissionManager { permission_map: HashMap, // Mapping of permission names to configurations } -// Global singleton for the PermissionManager -static GLOBAL_PERMISSION_MANAGER: OnceCell = OnceCell::new(); - // Constants representing specific permission categories const USER_PERMISSION: &str = "user"; const SMART_APPROVE_PERMISSION: &str = "smart_approve"; @@ -68,11 +64,6 @@ impl Default for PermissionManager { } impl PermissionManager { - /// Returns the global instance of the PermissionManager, initializing it if necessary. - pub fn global() -> &'static PermissionManager { - GLOBAL_PERMISSION_MANAGER.get_or_init(PermissionManager::default) - } - /// Creates a new `PermissionManager` with a specified config path. pub fn new>(config_path: P) -> Self { let config_path = config_path.as_ref().to_path_buf(); diff --git a/crates/goose/src/permission/permission_confirmation.rs b/crates/goose/src/permission/permission_confirmation.rs index f110d79a..b33dd591 100644 --- a/crates/goose/src/permission/permission_confirmation.rs +++ b/crates/goose/src/permission/permission_confirmation.rs @@ -15,7 +15,6 @@ pub enum PrincipalType { #[derive(Debug, Serialize, Deserialize, Clone)] pub struct PermissionConfirmation { - pub principal_name: String, pub principal_type: PrincipalType, pub permission: Permission, } diff --git a/crates/goose/src/permission/permission_judge.rs b/crates/goose/src/permission/permission_judge.rs index ac492ad9..6118140a 100644 --- a/crates/goose/src/permission/permission_judge.rs +++ b/crates/goose/src/permission/permission_judge.rs @@ -33,11 +33,12 @@ fn create_read_only_tool() -> Tool { - `INSERT`, `UPDATE`, or `DELETE` in SQL. - Writing or appending to a file. - Modifying system configurations. + - Sending messages to Slack channel. How to analyze tool requests: - Inspect each tool request to identify its purpose based on its name and arguments. - Categorize the operation as read-only if it does not involve any state or data modification. - - Return a list of tool names that are strictly read-only. + - Return a list of tool names that are strictly read-only. If you cannot make the decision, then it is not read-only. Use this analysis to generate the list of tools performing read-only operations from the provided tool requests. "#} @@ -67,6 +68,16 @@ fn create_read_only_tool() -> Tool { /// Builds the message to be sent to the LLM for detecting read-only operations. fn create_check_messages(tool_requests: Vec<&ToolRequest>) -> Vec { + let tool_names: Vec = tool_requests + .iter() + .filter_map(|req| { + if let Ok(tool_call) = &req.tool_call { + Some(tool_call.name.clone()) + } else { + None // Skip requests with errors in tool_call + } + }) + .collect(); let mut check_messages = vec![]; check_messages.push(Message { role: mcp_core::Role::User, @@ -79,7 +90,7 @@ fn create_check_messages(tool_requests: Vec<&ToolRequest>) -> Vec { \n- Examples include file reading, SELECT queries in SQL, and directory listing. \ \n- Write operations include INSERT, UPDATE, DELETE, and file writing. \ \n\nPlease provide a list of tool names that qualify as read-only:", - tool_requests, + tool_names.join(", "), ), annotations: None, })], @@ -147,7 +158,7 @@ pub struct PermissionCheckResult { } pub async fn check_tool_permissions( - remaining_requests: Vec, + remaining_requests: Vec<&&ToolRequest>, mode: &str, tools_with_readonly_annotation: HashSet, tools_without_annotation: HashSet, @@ -159,14 +170,14 @@ pub async fn check_tool_permissions( let mut denied = vec![]; let mut llm_detect_candidates = vec![]; - for request in remaining_requests { + for &&request in &remaining_requests { if let Ok(tool_call) = request.tool_call.clone() { // 1. Check user-defined permission if let Some(level) = permission_manager.get_user_permission(&tool_call.name) { match level { - PermissionLevel::AlwaysAllow => approved.push(request), - PermissionLevel::AskBefore => needs_approval.push(request), - PermissionLevel::NeverAllow => denied.push(request), + PermissionLevel::AlwaysAllow => approved.push(request.clone()), + PermissionLevel::AskBefore => needs_approval.push(request.clone()), + PermissionLevel::NeverAllow => denied.push(request.clone()), } continue; } @@ -174,30 +185,30 @@ pub async fn check_tool_permissions( // 2. Fallback based on mode match mode { "manual_approve" => { - needs_approval.push(request); + needs_approval.push(request.clone()); } "smart_approve" => { if let Some(level) = permission_manager.get_smart_approve_permission(&tool_call.name) { match level { - PermissionLevel::AlwaysAllow => approved.push(request), - PermissionLevel::AskBefore => needs_approval.push(request), - PermissionLevel::NeverAllow => denied.push(request), + PermissionLevel::AlwaysAllow => approved.push(request.clone()), + PermissionLevel::AskBefore => needs_approval.push(request.clone()), + PermissionLevel::NeverAllow => denied.push(request.clone()), } continue; } if tools_with_readonly_annotation.contains(&tool_call.name) { - approved.push(request); + approved.push(request.clone()); } else if tools_without_annotation.contains(&tool_call.name) { - llm_detect_candidates.push(request); + llm_detect_candidates.push(request.clone()); } else { - needs_approval.push(request); + needs_approval.push(request.clone()); } } _ => { - needs_approval.push(request); + needs_approval.push(request.clone()); } } } @@ -210,13 +221,13 @@ pub async fn check_tool_permissions( for request in llm_detect_candidates { if let Ok(tool_call) = request.tool_call.clone() { if detected_readonly_tools.contains(&tool_call.name) { - approved.push(request); + approved.push(request.clone()); permission_manager.update_smart_approve_permission( &tool_call.name, PermissionLevel::AlwaysAllow, ); } else { - needs_approval.push(request); + needs_approval.push(request.clone()); permission_manager.update_smart_approve_permission( &tool_call.name, PermissionLevel::AskBefore, @@ -401,7 +412,11 @@ mod tests { }), }; - let remaining_requests = vec![tool_request_1, tool_request_2]; + // Store ToolRequests in a Vec + let tool_requests = vec![&tool_request_1, &tool_request_2]; + + // Create a Vec of references to ToolRequests + let remaining_requests: Vec<&&ToolRequest> = tool_requests.iter().collect(); // Call the function under test let result = check_tool_permissions(