From b0f4e2a0f1a5507aa0a0fbe73d0b5c77a3e86845 Mon Sep 17 00:00:00 2001 From: Jasper Date: Tue, 21 Jul 2026 06:54:03 -0500 Subject: [PATCH] fix(permissions): enforce manual approval for code mode (#10528) --- crates/goose/src/agents/agent.rs | 4 -- .../platform_extensions/code_execution.rs | 38 +++++++++- crates/goose/src/agents/reply_parts.rs | 72 ++++++++++++++++++- .../src/permission/permission_inspector.rs | 7 +- 4 files changed, 111 insertions(+), 10 deletions(-) diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index 9f556520e..93978574f 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -757,10 +757,6 @@ impl Agent { let goose_mode = *self.current_goose_mode.lock().await; - if goose_mode == GooseMode::SmartApprove { - self.tool_inspection_manager.apply_tool_annotations(&tools); - } - let tool_call_cut_off = match Config::global().get_param::("GOOSE_TOOL_CALL_CUTOFF") { Ok(v) => v, diff --git a/crates/goose/src/agents/platform_extensions/code_execution.rs b/crates/goose/src/agents/platform_extensions/code_execution.rs index 11bd3e06b..1eebe6c2a 100644 --- a/crates/goose/src/agents/platform_extensions/code_execution.rs +++ b/crates/goose/src/agents/platform_extensions/code_execution.rs @@ -497,11 +497,11 @@ impl McpClientTrait for CodeExecutionClient { schema::(), ) .annotate(ToolAnnotations::from_raw( - Some("Get function details".to_string()), - Some(true), + Some("Execute Bash".to_string()), Some(false), Some(true), Some(false), + Some(true), )), McpTool::new( "execute_typescript".to_string(), @@ -874,6 +874,40 @@ mod tests { assert!(!moim.contains("ask_heimdall")); } + #[tokio::test] + async fn execute_bash_annotations_require_approval() { + let temp = tempfile::tempdir().unwrap(); + let client = CodeExecutionClient::new( + PlatformExtensionContext { + extension_manager: None, + session_manager: Arc::new(crate::session::SessionManager::new( + temp.path().join("sessions"), + )), + session: None, + use_login_shell_path: false, + }, + ToolDisclosure::Filesystem, + ) + .unwrap(); + + let tools = client + .list_tools("test", None, CancellationToken::new()) + .await + .unwrap() + .tools; + let execute_bash = tools + .iter() + .find(|tool| tool.name == "execute_bash") + .unwrap(); + let annotations = execute_bash.annotations.as_ref().unwrap(); + + assert_eq!(annotations.title.as_deref(), Some("Execute Bash")); + assert_eq!(annotations.read_only_hint, Some(false)); + assert_eq!(annotations.destructive_hint, Some(true)); + assert_eq!(annotations.idempotent_hint, Some(false)); + assert_eq!(annotations.open_world_hint, Some(true)); + } + fn self_referential_any_schema() -> Value { json!({ "$ref": "#/$defs/Any", diff --git a/crates/goose/src/agents/reply_parts.rs b/crates/goose/src/agents/reply_parts.rs index 8238e1f1e..0a22d6e81 100644 --- a/crates/goose/src/agents/reply_parts.rs +++ b/crates/goose/src/agents/reply_parts.rs @@ -11,7 +11,7 @@ use tracing::debug; use super::super::agents::Agent; #[cfg(feature = "code-mode")] use crate::agents::platform_extensions::code_execution; -use crate::config::Config; +use crate::config::{Config, GooseMode}; use crate::conversation::message::{Message, MessageContent, MessageUsage, ToolRequest}; use crate::conversation::{fix_conversation, Conversation}; #[cfg(test)] @@ -251,6 +251,10 @@ impl Agent { let goose_mode = *self.current_goose_mode.lock().await; + if goose_mode == GooseMode::SmartApprove { + self.tool_inspection_manager.apply_tool_annotations(&tools); + } + let prompt_manager = self.prompt_manager.lock().await; let mut system_prompt = prompt_manager .builder() @@ -672,6 +676,7 @@ pub fn is_tool_visible_to_model(tool: &Tool) -> bool { mod tests { use super::*; use crate::agents::{AgentConfig, GoosePlatform}; + use crate::config::permission::PermissionLevel; use crate::config::{GooseMode, PermissionManager}; use crate::conversation::message::{Message, SystemNotificationType}; use crate::providers::base::Provider; @@ -679,7 +684,7 @@ mod tests { use async_trait::async_trait; use goose_providers::conversation::token_usage::{ProviderStats, ProviderUsage, Usage}; use goose_providers::model::ModelConfig; - use rmcp::model::{AnnotateAble, RawTextContent, Role}; + use rmcp::model::{AnnotateAble, RawTextContent, Role, ToolAnnotations}; use rmcp::object; use std::sync::Mutex; use std::time::{Duration, Instant}; @@ -935,6 +940,69 @@ mod tests { Ok(()) } + #[tokio::test] + async fn prepare_toolshim_tools_applies_writable_annotations() -> anyhow::Result<()> { + let data_dir = tempfile::tempdir()?; + let data_path = data_dir.path().to_path_buf(); + let session_manager = Arc::new(SessionManager::new(data_path.clone())); + let permission_manager = Arc::new(PermissionManager::new(data_path)); + permission_manager + .update_smart_approve_permission("frontend__write_tool", PermissionLevel::AlwaysAllow); + let agent = Agent::with_config(AgentConfig::new( + Arc::clone(&session_manager), + Arc::clone(&permission_manager), + None, + GooseMode::SmartApprove, + false, + GoosePlatform::GooseCli, + )); + let session = session_manager + .create_session( + std::env::current_dir()?, + "test-toolshim-annotations".to_string(), + SessionType::Hidden, + GooseMode::SmartApprove, + ) + .await?; + let model_config = ModelConfig::new("test-model").with_toolshim(true); + agent + .update_provider(Arc::new(MockProvider), model_config, &session.id) + .await?; + agent + .add_extension( + crate::agents::extension::ExtensionConfig::Frontend { + name: "frontend".to_string(), + description: "desc".to_string(), + tools: vec![Tool::new( + "frontend__write_tool", + "Write tool", + object!({ "type": "object", "properties": { } }), + ) + .annotate(ToolAnnotations::new().read_only(false))], + instructions: None, + bundled: None, + available_tools: vec![], + }, + &session.id, + ) + .await?; + + let (tools, toolshim_tools, _, _) = agent + .prepare_tools_and_prompt(&session.id, session.working_dir.as_path()) + .await?; + + assert!(tools.is_empty()); + assert!(toolshim_tools + .iter() + .any(|tool| tool.name == "frontend__write_tool")); + assert_eq!( + permission_manager.get_smart_approve_permission("frontend__write_tool"), + Some(PermissionLevel::AskBefore) + ); + + Ok(()) + } + #[tokio::test] async fn test_stream_error_propagation() { use futures::StreamExt; diff --git a/crates/goose/src/permission/permission_inspector.rs b/crates/goose/src/permission/permission_inspector.rs index 692c12926..94e11d3db 100644 --- a/crates/goose/src/permission/permission_inspector.rs +++ b/crates/goose/src/permission/permission_inspector.rs @@ -169,8 +169,10 @@ impl ToolInspector for PermissionInspector { InspectionAction::RequireApproval(None) } } - // 2. Check if the tool is explicitly annotated as read-only - } else if self.is_readonly_annotated_tool(tool_name) { + // 2. Check for a read-only annotation in SmartApprove mode + } else if goose_mode == GooseMode::SmartApprove + && self.is_readonly_annotated_tool(tool_name) + { InspectionAction::Allow // 3. Special case for extension management } else if tool_name == MANAGE_EXTENSIONS_TOOL_NAME_COMPLETE { @@ -321,6 +323,7 @@ mod tests { #[test_case(GooseMode::SmartApprove, false, None, InspectionAction::RequireApproval(None); "smart_approve_unknown_defers")] #[test_case(GooseMode::Approve, false, None, InspectionAction::RequireApproval(None); "approve_requires_approval")] #[test_case(GooseMode::Approve, false, Some(PermissionLevel::AlwaysAllow), InspectionAction::RequireApproval(None); "approve_ignores_cache")] + #[test_case(GooseMode::Approve, true, None, InspectionAction::RequireApproval(None); "approve_ignores_annotation")] #[tokio::test] async fn test_inspect_action( mode: GooseMode,