diff --git a/crates/goose/src/tool_inspection.rs b/crates/goose/src/tool_inspection.rs index 1a7ef80125ad..d72fe649bc12 100644 --- a/crates/goose/src/tool_inspection.rs +++ b/crates/goose/src/tool_inspection.rs @@ -238,9 +238,13 @@ pub fn apply_inspection_results_to_permissions( if let Some(request) = all_requests.get(request_id) { if !permission_result - .needs_approval + .denied .iter() .any(|req| req.id == *request_id) + && !permission_result + .needs_approval + .iter() + .any(|req| req.id == *request_id) { permission_result.needs_approval.push(request.clone()); } diff --git a/crates/goose/tests/tool_inspection_permission_precedence.rs b/crates/goose/tests/tool_inspection_permission_precedence.rs new file mode 100644 index 000000000000..82b1c2d4f331 --- /dev/null +++ b/crates/goose/tests/tool_inspection_permission_precedence.rs @@ -0,0 +1,100 @@ +use goose::conversation::message::ToolRequest; +use goose::permission::permission_judge::PermissionCheckResult; +use goose::tool_inspection::{ + apply_inspection_results_to_permissions, InspectionAction, InspectionResult, +}; +use rmcp::model::CallToolRequestParams; +use rmcp::object; + +fn request(id: &str) -> ToolRequest { + ToolRequest { + id: id.to_string(), + tool_call: Ok(CallToolRequestParams::new("test_tool").with_arguments(object!({}))), + metadata: None, + tool_meta: None, + } +} + +fn inspection(id: &str, action: InspectionAction) -> InspectionResult { + InspectionResult { + tool_request_id: id.to_string(), + action, + reason: "test decision".to_string(), + confidence: 1.0, + inspector_name: "test_inspector".to_string(), + finding_id: None, + } +} + +fn assert_only_denied(result: PermissionCheckResult, id: &str) { + assert!(result.approved.is_empty()); + assert!(result.needs_approval.is_empty()); + assert_eq!(result.denied, vec![request(id)]); +} + +#[test] +fn require_approval_does_not_resurrect_a_denied_request() { + let result = apply_inspection_results_to_permissions( + PermissionCheckResult { + approved: vec![], + needs_approval: vec![], + denied: vec![request("request-1")], + }, + &[inspection( + "request-1", + InspectionAction::RequireApproval(Some("security warning".to_string())), + )], + ); + + assert_only_denied(result, "request-1"); +} + +#[test] +fn denial_dominates_regardless_of_inspection_result_order() { + for inspection_results in [ + vec![ + inspection("request-1", InspectionAction::Deny), + inspection( + "request-1", + InspectionAction::RequireApproval(Some("security warning".to_string())), + ), + ], + vec![ + inspection( + "request-1", + InspectionAction::RequireApproval(Some("security warning".to_string())), + ), + inspection("request-1", InspectionAction::Deny), + ], + ] { + let result = apply_inspection_results_to_permissions( + PermissionCheckResult { + approved: vec![request("request-1")], + needs_approval: vec![], + denied: vec![], + }, + &inspection_results, + ); + + assert_only_denied(result, "request-1"); + } +} + +#[test] +fn require_approval_still_moves_an_approved_request_to_needs_approval() { + let result = apply_inspection_results_to_permissions( + PermissionCheckResult { + approved: vec![request("request-1")], + needs_approval: vec![], + denied: vec![], + }, + &[inspection( + "request-1", + InspectionAction::RequireApproval(Some("security warning".to_string())), + )], + ); + + assert!(result.approved.is_empty()); + assert_eq!(result.needs_approval, vec![request("request-1")]); + assert!(result.denied.is_empty()); +}