diff --git a/crates/acp_thread/src/acp_thread.rs b/crates/acp_thread/src/acp_thread.rs index bf1d933acd..61bee3ff85 100644 --- a/crates/acp_thread/src/acp_thread.rs +++ b/crates/acp_thread/src/acp_thread.rs @@ -2126,8 +2126,6 @@ impl AcpThread { if let ToolCallStatus::WaitingForConfirmation { respond_tx, .. } = curr_status { respond_tx.send(outcome).ok(); - } else if cfg!(debug_assertions) { - panic!("tried to authorize an already authorized tool call"); } cx.emit(AcpThreadEvent::EntryUpdated(ix)); diff --git a/crates/agent/src/tests/mod.rs b/crates/agent/src/tests/mod.rs index a6419b52a0..f8d74e0df9 100644 --- a/crates/agent/src/tests/mod.rs +++ b/crates/agent/src/tests/mod.rs @@ -4127,6 +4127,7 @@ async fn setup(cx: &mut TestAppContext, model: TestModel) -> ThreadTest { DelayTool::NAME: true, WordListTool::NAME: true, ToolRequiringPermission::NAME: true, + ToolRequiringPermission2::NAME: true, InfiniteTool::NAME: true, CancellationAwareTool::NAME: true, StreamingEchoTool::NAME: true, @@ -6531,6 +6532,394 @@ async fn test_fetch_tool_allow_rule_skips_confirmation(cx: &mut TestAppContext) ); } +/// Approving one pending tool call with "Always for " auto-resolves +/// sibling pending authorizations for the same tool in the same turn. +#[gpui::test] +async fn test_always_allow_resolves_pending_authorizations(cx: &mut TestAppContext) { + let ThreadTest { model, thread, .. } = setup(cx, TestModel::Fake).await; + let fake_model = model.as_fake(); + + let mut events = thread + .update(cx, |thread, cx| { + thread.add_tool(ToolRequiringPermission); + thread.send(UserMessageId::new(), ["abc"], cx) + }) + .unwrap(); + cx.run_until_parked(); + + // Two parallel tool calls, both require permission. + for id in ["tool_id_1", "tool_id_2"] { + fake_model.send_last_completion_stream_event(LanguageModelCompletionEvent::ToolUse( + LanguageModelToolUse { + id: id.into(), + name: ToolRequiringPermission::NAME.into(), + raw_input: "{}".into(), + input: json!({}), + is_input_complete: true, + thought_signature: None, + }, + )); + } + fake_model.end_last_completion_stream(); + + let tool_call_auth_1 = next_tool_call_authorization(&mut events).await; + let tool_call_auth_2 = next_tool_call_authorization(&mut events).await; + + // Approve the first with "always allow" — this persists a setting that + // makes the tool unconditionally allowed. The second pending + // authorization should resolve without user interaction. + tool_call_auth_1 + .response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("always_allow:tool_requiring_permission"), + acp::PermissionOptionKind::AllowAlways, + )) + .unwrap(); + cx.run_until_parked(); + + // The second tool's receiver was dropped by the auto-resolve path, so + // sending a late response should fail. + let late_send = tool_call_auth_2 + .response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )); + assert!( + late_send.is_err(), + "expected tool 2's response receiver to be dropped after auto-resolve" + ); + + let completion = fake_model.pending_completions().pop().unwrap(); + let message = completion.messages.last().unwrap(); + let results: Vec<_> = message + .content + .iter() + .filter_map(|c| match c { + language_model::MessageContent::ToolResult(r) => Some(r), + _ => None, + }) + .collect(); + assert_eq!( + results.len(), + 2, + "both tool calls should have produced results" + ); + assert!( + results.iter().all(|r| !r.is_error), + "both results should be successful after auto-resolve, got: {:?}", + results + ); +} + +/// Externally editing settings (e.g. the user opening settings.json and +/// adding an `always_allow` rule) resolves pending authorization prompts +/// for tool calls that match the new rule. +#[gpui::test] +async fn test_external_settings_edit_resolves_pending_authorization(cx: &mut TestAppContext) { + let ThreadTest { model, thread, .. } = setup(cx, TestModel::Fake).await; + let fake_model = model.as_fake(); + + let mut events = thread + .update(cx, |thread, cx| { + thread.add_tool(ToolRequiringPermission); + thread.send(UserMessageId::new(), ["abc"], cx) + }) + .unwrap(); + cx.run_until_parked(); + + fake_model.send_last_completion_stream_event(LanguageModelCompletionEvent::ToolUse( + LanguageModelToolUse { + id: "tool_id_1".into(), + name: ToolRequiringPermission::NAME.into(), + raw_input: "{}".into(), + input: json!({}), + is_input_complete: true, + thought_signature: None, + }, + )); + fake_model.end_last_completion_stream(); + + let tool_call_auth = next_tool_call_authorization(&mut events).await; + + // Simulate the user editing settings.json to globally allow the tool. + cx.update(|cx| { + let mut settings = agent_settings::AgentSettings::get_global(cx).clone(); + settings.tool_permissions.tools.insert( + ToolRequiringPermission::NAME.into(), + agent_settings::ToolRules { + default: Some(settings::ToolPermissionMode::Allow), + always_allow: vec![], + always_deny: vec![], + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + agent_settings::AgentSettings::override_global(settings, cx); + }); + cx.run_until_parked(); + + // The pending prompt auto-resolves without the user clicking anything. + let late_send = tool_call_auth + .response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )); + assert!( + late_send.is_err(), + "response receiver should have been dropped after settings-driven auto-resolve" + ); + + let completion = fake_model.pending_completions().pop().unwrap(); + let message = completion.messages.last().unwrap(); + let result = message + .content + .iter() + .find_map(|c| match c { + language_model::MessageContent::ToolResult(r) => Some(r), + _ => None, + }) + .expect("expected a tool result"); + assert!(!result.is_error, "tool should have been auto-allowed"); +} + +/// Externally adding a deny rule to settings dismisses a pending +/// authorization prompt and returns the tool call as denied. +#[gpui::test] +async fn test_external_deny_rule_resolves_pending_authorization(cx: &mut TestAppContext) { + let ThreadTest { model, thread, .. } = setup(cx, TestModel::Fake).await; + let fake_model = model.as_fake(); + + let mut events = thread + .update(cx, |thread, cx| { + thread.add_tool(ToolRequiringPermission); + thread.send(UserMessageId::new(), ["abc"], cx) + }) + .unwrap(); + cx.run_until_parked(); + + fake_model.send_last_completion_stream_event(LanguageModelCompletionEvent::ToolUse( + LanguageModelToolUse { + id: "tool_id_1".into(), + name: ToolRequiringPermission::NAME.into(), + raw_input: "{}".into(), + input: json!({}), + is_input_complete: true, + thought_signature: None, + }, + )); + fake_model.end_last_completion_stream(); + + let tool_call_auth = next_tool_call_authorization(&mut events).await; + + // Simulate the user adding a deny default for the tool. + cx.update(|cx| { + let mut settings = agent_settings::AgentSettings::get_global(cx).clone(); + settings.tool_permissions.tools.insert( + ToolRequiringPermission::NAME.into(), + agent_settings::ToolRules { + default: Some(settings::ToolPermissionMode::Deny), + always_allow: vec![], + always_deny: vec![], + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + agent_settings::AgentSettings::override_global(settings, cx); + }); + cx.run_until_parked(); + + let late_send = tool_call_auth + .response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )); + assert!( + late_send.is_err(), + "response receiver should have been dropped after deny auto-resolve" + ); + + let completion = fake_model.pending_completions().pop().unwrap(); + let message = completion.messages.last().unwrap(); + let result = message + .content + .iter() + .find_map(|c| match c { + language_model::MessageContent::ToolResult(r) => Some(r), + _ => None, + }) + .expect("expected a tool result"); + assert!( + result.is_error, + "tool should have been auto-denied by the new rule" + ); +} + +/// Unrelated settings changes must not spuriously resolve pending +/// authorizations: if the re-check still returns `Confirm`, the prompt +/// stays visible and waits for the user. +#[gpui::test] +async fn test_unrelated_settings_change_does_not_resolve_pending_authorization( + cx: &mut TestAppContext, +) { + let ThreadTest { model, thread, .. } = setup(cx, TestModel::Fake).await; + let fake_model = model.as_fake(); + + let mut events = thread + .update(cx, |thread, cx| { + thread.add_tool(ToolRequiringPermission); + thread.send(UserMessageId::new(), ["abc"], cx) + }) + .unwrap(); + cx.run_until_parked(); + + fake_model.send_last_completion_stream_event(LanguageModelCompletionEvent::ToolUse( + LanguageModelToolUse { + id: "tool_id_1".into(), + name: ToolRequiringPermission::NAME.into(), + raw_input: "{}".into(), + input: json!({}), + is_input_complete: true, + thought_signature: None, + }, + )); + fake_model.end_last_completion_stream(); + + let tool_call_auth = next_tool_call_authorization(&mut events).await; + + // Touch SettingsStore with a change that doesn't affect tool + // permissions; the pending authorization should remain pending. + cx.update(|cx| { + let mut settings = agent_settings::AgentSettings::get_global(cx).clone(); + settings.single_file_review = !settings.single_file_review; + agent_settings::AgentSettings::override_global(settings, cx); + }); + cx.run_until_parked(); + + // The user still has to act — resolve with an Allow Once. + tool_call_auth + .response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) + .expect("response receiver should still be alive"); + cx.run_until_parked(); + + let completion = fake_model.pending_completions().pop().unwrap(); + let message = completion.messages.last().unwrap(); + let result = message + .content + .iter() + .find_map(|c| match c { + language_model::MessageContent::ToolResult(r) => Some(r), + _ => None, + }) + .expect("expected a tool result"); + assert!(!result.is_error); +} + +/// Approving one pending tool call with "Always for " must not +/// dismiss a sibling pending authorization for a *different* tool: the +/// persisted rule is scoped to tool A, so tool B's prompt stays visible +/// and waits for the user. +#[gpui::test] +async fn test_always_allow_does_not_resolve_unrelated_tool_authorization(cx: &mut TestAppContext) { + let ThreadTest { model, thread, .. } = setup(cx, TestModel::Fake).await; + let fake_model = model.as_fake(); + + let mut events = thread + .update(cx, |thread, cx| { + thread.add_tool(ToolRequiringPermission); + thread.add_tool(ToolRequiringPermission2); + thread.send(UserMessageId::new(), ["abc"], cx) + }) + .unwrap(); + cx.run_until_parked(); + + // Two parallel tool calls, each for a distinct tool with its own + // permission scope. + for (id, name) in [ + ("tool_id_1", ToolRequiringPermission::NAME), + ("tool_id_2", ToolRequiringPermission2::NAME), + ] { + fake_model.send_last_completion_stream_event(LanguageModelCompletionEvent::ToolUse( + LanguageModelToolUse { + id: id.into(), + name: name.into(), + raw_input: "{}".into(), + input: json!({}), + is_input_complete: true, + thought_signature: None, + }, + )); + } + fake_model.end_last_completion_stream(); + + let auth_a = next_tool_call_authorization(&mut events).await; + let auth_b = next_tool_call_authorization(&mut events).await; + + // Match prompts back to their originating tools via the authorization + // context so the test doesn't depend on scheduling order. + let (auth_for_tool_1, auth_for_tool_2) = { + let a_name = auth_a + .context + .as_ref() + .expect("settings-driven authorization must carry a context") + .tool_name + .clone(); + if a_name == ToolRequiringPermission::NAME { + (auth_a, auth_b) + } else { + (auth_b, auth_a) + } + }; + + // Approve tool 1 with "always allow". Only tool 1's rule is persisted. + auth_for_tool_1 + .response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("always_allow:tool_requiring_permission"), + acp::PermissionOptionKind::AllowAlways, + )) + .unwrap(); + cx.run_until_parked(); + + // Tool 2's receiver must still be alive: its permission is unrelated + // to the rule that was just added, so its prompt stays pending. + auth_for_tool_2 + .response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) + .expect("tool 2's response receiver should still be alive"); + cx.run_until_parked(); + + let completion = fake_model.pending_completions().pop().unwrap(); + let message = completion.messages.last().unwrap(); + let results: Vec<_> = message + .content + .iter() + .filter_map(|c| match c { + language_model::MessageContent::ToolResult(r) => Some(r), + _ => None, + }) + .collect(); + assert_eq!( + results.len(), + 2, + "both tool calls should have produced results" + ); + assert!( + results.iter().all(|r| !r.is_error), + "both results should be successful, got: {:?}", + results + ); +} + #[gpui::test] async fn test_queued_message_ends_turn_at_boundary(cx: &mut TestAppContext) { init_test(cx); diff --git a/crates/agent/src/tests/test_tools.rs b/crates/agent/src/tests/test_tools.rs index 4744204fae..750ea48dc8 100644 --- a/crates/agent/src/tests/test_tools.rs +++ b/crates/agent/src/tests/test_tools.rs @@ -1,5 +1,4 @@ use super::*; -use agent_settings::AgentSettings; use gpui::{App, SharedString, Task}; use std::future; use std::sync::Mutex; @@ -317,31 +316,59 @@ impl AgentTool for ToolRequiringPermission { .await .map_err(|e| format!("Failed to receive tool input: {e}"))?; - let decision = cx.update(|cx| { - decide_permission_from_settings( - Self::NAME, - &[String::new()], - AgentSettings::get_global(cx), - ) + let authorize = cx.update(|cx| { + let context = crate::ToolPermissionContext::new(Self::NAME, vec![String::new()]); + event_stream.authorize("Authorize?", context, cx) }); + authorize.await.map_err(|e| e.to_string())?; + Ok("Allowed".to_string()) + }) + } +} - let authorize = match decision { - ToolPermissionDecision::Allow => None, - ToolPermissionDecision::Deny(reason) => { - return Err(reason); - } - ToolPermissionDecision::Confirm => Some(cx.update(|cx| { - let context = crate::ToolPermissionContext::new( - "tool_requiring_permission", - vec![String::new()], - ); - event_stream.authorize("Authorize?", context, cx) - })), - }; +/// A second tool that also requires permission, used to verify that +/// permission decisions scoped to one tool don't leak into prompts for a +/// different tool. +#[derive(JsonSchema, Serialize, Deserialize)] +pub struct ToolRequiringPermission2Input {} - if let Some(authorize) = authorize { - authorize.await.map_err(|e| e.to_string())?; - } +pub struct ToolRequiringPermission2; + +impl AgentTool for ToolRequiringPermission2 { + type Input = ToolRequiringPermission2Input; + type Output = String; + + const NAME: &'static str = "tool_requiring_permission_2"; + + fn kind() -> acp::ToolKind { + acp::ToolKind::Other + } + + fn initial_title( + &self, + _input: Result, + _cx: &mut App, + ) -> SharedString { + "This tool also requires permission".into() + } + + fn run( + self: Arc, + input: ToolInput, + event_stream: ToolCallEventStream, + cx: &mut App, + ) -> Task> { + cx.spawn(async move |cx| { + let _input = input + .recv() + .await + .map_err(|e| format!("Failed to receive tool input: {e}"))?; + + let authorize = cx.update(|cx| { + let context = crate::ToolPermissionContext::new(Self::NAME, vec![String::new()]); + event_stream.authorize("Authorize?", context, cx) + }); + authorize.await.map_err(|e| e.to_string())?; Ok("Allowed".to_string()) }) } diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index da5602050d..89b3b0eb25 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -46,7 +46,9 @@ use prompt_store::ProjectContext; use schemars::{JsonSchema, Schema}; use serde::de::DeserializeOwned; use serde::{Deserialize, Serialize}; -use settings::{LanguageModelSelection, Settings, ToolPermissionMode, update_settings_file}; +use settings::{ + LanguageModelSelection, Settings, SettingsStore, ToolPermissionMode, update_settings_file, +}; use std::{ collections::BTreeMap, marker::PhantomData, @@ -3739,121 +3741,221 @@ impl ToolCallEventStream { display_name: String, cx: &mut App, ) -> Task> { - let settings = agent_settings::AgentSettings::get_global(cx); + let title = title.into(); + let options = acp_thread::PermissionOptions::Dropdown(vec![ + acp_thread::PermissionOptionChoice { + allow: acp::PermissionOption::new( + acp::PermissionOptionId::new(format!("always_allow_mcp:{tool_id}")), + format!("Always for {display_name} MCP tool"), + acp::PermissionOptionKind::AllowAlways, + ), + deny: acp::PermissionOption::new( + acp::PermissionOptionId::new(format!("always_deny_mcp:{tool_id}")), + format!("Always for {display_name} MCP tool"), + acp::PermissionOptionKind::RejectAlways, + ), + sub_patterns: vec![], + }, + acp_thread::PermissionOptionChoice { + allow: acp::PermissionOption::new( + acp::PermissionOptionId::new("allow"), + "Only this time", + acp::PermissionOptionKind::AllowOnce, + ), + deny: acp::PermissionOption::new( + acp::PermissionOptionId::new("deny"), + "Only this time", + acp::PermissionOptionKind::RejectOnce, + ), + sub_patterns: vec![], + }, + ]); - let decision = decide_permission_from_settings(&tool_id, &[String::new()], &settings); + // MCP tools are gated only by tool id (no per-input pattern + // matching), so we pass a single empty input value just to satisfy + // `decide_permission_from_settings`' signature. + let check_settings: Box ToolPermissionDecision> = + Box::new(move |cx: &App| { + let settings = agent_settings::AgentSettings::get_global(cx); + decide_permission_from_settings(&tool_id, &[String::new()], settings) + }); - match decision { - ToolPermissionDecision::Allow => return Task::ready(Ok(())), - ToolPermissionDecision::Deny(reason) => return Task::ready(Err(anyhow!(reason))), - ToolPermissionDecision::Confirm => {} - } - - let (response_tx, response_rx) = oneshot::channel(); - if let Err(error) = self - .stream - .0 - .unbounded_send(Ok(ThreadEvent::ToolCallAuthorization( - ToolCallAuthorization { - tool_call: acp::ToolCallUpdate::new( - self.tool_use_id.to_string(), - acp::ToolCallUpdateFields::new().title(title.into()), - ), - options: acp_thread::PermissionOptions::Dropdown(vec![ - acp_thread::PermissionOptionChoice { - allow: acp::PermissionOption::new( - acp::PermissionOptionId::new(format!( - "always_allow_mcp:{}", - tool_id - )), - format!("Always for {} MCP tool", display_name), - acp::PermissionOptionKind::AllowAlways, - ), - deny: acp::PermissionOption::new( - acp::PermissionOptionId::new(format!( - "always_deny_mcp:{}", - tool_id - )), - format!("Always for {} MCP tool", display_name), - acp::PermissionOptionKind::RejectAlways, - ), - sub_patterns: vec![], - }, - acp_thread::PermissionOptionChoice { - allow: acp::PermissionOption::new( - acp::PermissionOptionId::new("allow"), - "Only this time", - acp::PermissionOptionKind::AllowOnce, - ), - deny: acp::PermissionOption::new( - acp::PermissionOptionId::new("deny"), - "Only this time", - acp::PermissionOptionKind::RejectOnce, - ), - sub_patterns: vec![], - }, - ]), - response: response_tx, - context: None, - }, - ))) - { - log::error!("Failed to send tool call authorization: {error}"); - return Task::ready(Err(anyhow!( - "Failed to send tool call authorization: {error}" - ))); - } - - let fs = self.fs.clone(); - cx.spawn(async move |cx| { - let outcome = response_rx.await?; - let is_allow = Self::persist_permission_outcome(&outcome, fs, &cx); - if is_allow { - Ok(()) - } else { - Err(anyhow!("Permission to run tool denied by user")) - } - }) + self.run_authorization_loop(title, options, None, Some(check_settings), cx) } + /// Gate a tool call on user permission, driven by the agent's + /// tool-permission settings. + /// + /// Evaluates the current settings up-front: returns `Ok(())` immediately + /// if the tool is already allowed, an error if it is denied, and + /// otherwise prompts the user for a decision. While a prompt is pending, + /// a subscription to `SettingsStore` watches for changes (for example, + /// when the user clicks "Always for …" on a sibling tool call and the + /// new rule becomes globally visible). When settings change, the current + /// prompt is dismissed and the decision is re-evaluated. This closes the + /// gap where an "Always for …" decision on one pending tool call would + /// not propagate to other pending tool calls in the same turn or in + /// subagent turns. + /// + /// For authorizations that must always prompt regardless of settings + /// (e.g. symlink-escape confirmations, sensitive settings-file edits), + /// use [`Self::prompt`] instead. pub fn authorize( &self, title: impl Into, context: ToolPermissionContext, cx: &mut App, ) -> Task> { + let title = title.into(); let options = context.build_permission_options(); - let (response_tx, response_rx) = oneshot::channel(); - if let Err(error) = self - .stream - .0 - .unbounded_send(Ok(ThreadEvent::ToolCallAuthorization( - ToolCallAuthorization { - tool_call: acp::ToolCallUpdate::new( - self.tool_use_id.to_string(), - acp::ToolCallUpdateFields::new().title(title.into()), - ), - options, - response: response_tx, - context: Some(context), - }, - ))) - { - log::error!("Failed to send tool call authorization: {error}"); - return Task::ready(Err(anyhow!( - "Failed to send tool call authorization: {error}" - ))); + let tool_name = context.tool_name.clone(); + let input_values = context.input_values.clone(); + let check_settings: Box ToolPermissionDecision> = + Box::new(move |cx: &App| { + decide_permission_from_settings( + &tool_name, + &input_values, + agent_settings::AgentSettings::get_global(cx), + ) + }); + + self.run_authorization_loop(title, options, Some(context), Some(check_settings), cx) + } + + /// Like [`Self::authorize`], but always prompts the user without + /// consulting settings. Use this for authorizations that must be + /// confirmed even when the user has configured `always_allow` rules — + /// for example, symlink-escape confirmations or edits that target + /// sensitive settings files. + pub fn authorize_always_prompt( + &self, + title: impl Into, + context: ToolPermissionContext, + cx: &mut App, + ) -> Task> { + let title = title.into(); + let options = context.build_permission_options(); + self.run_authorization_loop(title, options, Some(context), None, cx) + } + + /// Prompts the user for authorization. + /// + /// When `check_settings` is `Some`, this gate is settings-driven: the + /// settings are evaluated up-front (an Allow or Deny result resolves the + /// task immediately without prompting), and while a prompt is pending a + /// `SettingsStore` subscription watches for changes. A subsequent Allow + /// or Deny dismisses the prompt UI and resolves the task without user + /// interaction. + /// + /// When `check_settings` is `None`, the user is always prompted and + /// settings changes are ignored. This suits prompts that aren't + /// settings-driven (e.g. symlink-escape confirmations). + fn run_authorization_loop( + &self, + title: String, + options: acp_thread::PermissionOptions, + context: Option, + check_settings: Option ToolPermissionDecision>>, + cx: &mut App, + ) -> Task> { + // Short-circuit when current settings yield a definitive answer. + if let Some(check) = check_settings.as_ref() { + match check(cx) { + ToolPermissionDecision::Allow => return Task::ready(Ok(())), + ToolPermissionDecision::Deny(reason) => { + return Task::ready(Err(anyhow!(reason))); + } + ToolPermissionDecision::Confirm => {} + } } let fs = self.fs.clone(); + let stream = self.stream.clone(); + let tool_use_id = self.tool_use_id.clone(); cx.spawn(async move |cx| { - let outcome = response_rx.await?; - let is_allow = Self::persist_permission_outcome(&outcome, fs, &cx); - if is_allow { - Ok(()) - } else { - Err(anyhow!("Permission to run tool denied by user")) + let (response_tx, mut response_rx) = oneshot::channel(); + if let Err(error) = stream + .0 + .unbounded_send(Ok(ThreadEvent::ToolCallAuthorization( + ToolCallAuthorization { + tool_call: acp::ToolCallUpdate::new( + tool_use_id.to_string(), + acp::ToolCallUpdateFields::new().title(title), + ), + options, + response: response_tx, + context, + }, + ))) + { + log::error!("Failed to send tool call authorization: {error}"); + return Err(anyhow!("Failed to send tool call authorization: {error}")); + } + + let Some(check_settings) = check_settings else { + let outcome = response_rx + .await + .map_err(|_| anyhow!("authorization channel closed"))?; + + return Self::persist_permission_outcome(&outcome, fs, cx); + }; + + let (mut settings_tx, mut settings_rx) = watch::channel(()); + let _settings_subscription = cx.update(|cx| { + cx.observe_global::(move |_cx| { + settings_tx.send(()).ok(); + }) + }); + + // Race the user's response against settings changes. On each + // settings change, re-evaluate `check_settings`: if it now + // yields a definitive Allow or Deny, resolve the prompt + // without user interaction. Otherwise keep waiting on the + // same prompt. + loop { + let settings_changed = async { + if settings_rx.changed().await.is_err() { + std::future::pending::<()>().await; + } + }; + futures::select_biased! { + outcome = (&mut response_rx).fuse() => { + let outcome = outcome + .map_err(|_| anyhow!("authorization channel closed"))?; + return Self::persist_permission_outcome(&outcome, fs.clone(), cx); + } + _ = settings_changed.fuse() => { + // On auto-resolve, we dismiss the prompt UI by + // replacing the tool call's `WaitingForConfirmation` + // status with `InProgress` (or `Failed`). Dropping + // `response_rx` closes the `oneshot` held by the + // UI, so any late click by the user is a no-op. + match cx.update(|cx| check_settings(cx)) { + ToolPermissionDecision::Allow => { + drop(response_rx); + stream.update_tool_call_fields( + &tool_use_id, + acp::ToolCallUpdateFields::new() + .status(acp::ToolCallStatus::InProgress), + None, + ); + return Ok(()); + } + ToolPermissionDecision::Deny(reason) => { + drop(response_rx); + stream.update_tool_call_fields( + &tool_use_id, + acp::ToolCallUpdateFields::new() + .status(acp::ToolCallStatus::Failed), + None, + ); + return Err(anyhow!(reason)); + } + ToolPermissionDecision::Confirm => continue, + } + } + } } }) } @@ -3864,8 +3966,9 @@ impl ToolCallEventStream { outcome: &acp_thread::SelectedPermissionOutcome, fs: Option>, cx: &AsyncApp, - ) -> bool { + ) -> Result<()> { let option_id = outcome.option_id.0.as_ref(); + let err = || Err(anyhow!("Permission to run tool denied by user")); let always_permission = option_id .strip_prefix("always_allow:") @@ -3889,7 +3992,11 @@ impl ToolCallEventStream { if let Some((tool, mode)) = always_permission { let params = outcome.params.as_ref(); Self::persist_always_permission(tool, mode, params, fs, cx); - return mode == ToolPermissionMode::Allow; + return if mode == ToolPermissionMode::Allow { + Ok(()) + } else { + err() + }; } // Handle simple "allow" / "deny" (once, no persistence) @@ -3898,11 +4005,12 @@ impl ToolCallEventStream { outcome.params.is_none(), "unexpected params for once-only permission" ); - return option_id == "allow"; + return if option_id == "allow" { Ok(()) } else { err() }; } debug_assert!(false, "unexpected permission option_id: {option_id}"); - false + + err() } /// Persists an "always allow" or "always deny" permission, using sub_patterns diff --git a/crates/agent/src/tools/copy_path_tool.rs b/crates/agent/src/tools/copy_path_tool.rs index 063742cef8..b40f26bee7 100644 --- a/crates/agent/src/tools/copy_path_tool.rs +++ b/crates/agent/src/tools/copy_path_tool.rs @@ -1,9 +1,10 @@ use super::tool_permissions::{ - SensitiveSettingsKind, authorize_symlink_escapes, canonicalize_worktree_roots, - collect_symlink_escapes, sensitive_settings_kind, + authorize_symlink_escapes, canonicalize_worktree_roots, collect_symlink_escapes, + sensitive_settings_kind, }; use crate::{ - AgentTool, ToolCallEventStream, ToolInput, ToolPermissionDecision, decide_permission_for_paths, + AgentTool, ToolCallEventStream, ToolInput, ToolPermissionDecision, + authorize_with_sensitive_settings, decide_permission_for_paths, }; use agent_client_protocol::schema as acp; use agent_settings::AgentSettings; @@ -141,12 +142,13 @@ impl AgentTool for CopyPathTool { vec![input.source_path.clone(), input.destination_path.clone()], ); let title = format!("Copy {src} to {dest}"); - let title = match sensitive_kind { - Some(SensitiveSettingsKind::Local) => format!("{title} (local settings)"), - Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), - None => title, - }; - event_stream.authorize(title, context, cx) + authorize_with_sensitive_settings( + sensitive_kind, + context, + &title, + &event_stream, + cx, + ) })) } else { None diff --git a/crates/agent/src/tools/create_directory_tool.rs b/crates/agent/src/tools/create_directory_tool.rs index 0e5261d071..602b880932 100644 --- a/crates/agent/src/tools/create_directory_tool.rs +++ b/crates/agent/src/tools/create_directory_tool.rs @@ -1,6 +1,6 @@ use super::tool_permissions::{ - SensitiveSettingsKind, authorize_symlink_access, canonicalize_worktree_roots, - detect_symlink_escape, sensitive_settings_kind, + authorize_symlink_access, canonicalize_worktree_roots, detect_symlink_escape, + sensitive_settings_kind, }; use agent_client_protocol::schema as acp; use agent_settings::AgentSettings; @@ -14,7 +14,8 @@ use std::sync::Arc; use util::markdown::MarkdownInlineCode; use crate::{ - AgentTool, ToolCallEventStream, ToolInput, ToolPermissionDecision, decide_permission_for_path, + AgentTool, ToolCallEventStream, ToolInput, ToolPermissionDecision, + authorize_with_sensitive_settings, decide_permission_for_path, }; use std::path::Path; @@ -126,16 +127,15 @@ impl AgentTool for CreateDirectoryTool { ToolPermissionDecision::Allow => None, ToolPermissionDecision::Confirm => Some(cx.update(|cx| { let title = format!("Create directory {}", MarkdownInlineCode(&input.path)); - let title = match &sensitive_kind { - Some(SensitiveSettingsKind::Local) => { - format!("{title} (local settings)") - } - Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), - None => title, - }; let context = crate::ToolPermissionContext::new(Self::NAME, vec![input.path.clone()]); - event_stream.authorize(title, context, cx) + authorize_with_sensitive_settings( + sensitive_kind, + context, + &title, + &event_stream, + cx, + ) })), ToolPermissionDecision::Deny(_) => None, } diff --git a/crates/agent/src/tools/delete_path_tool.rs b/crates/agent/src/tools/delete_path_tool.rs index d790896425..9e48d42641 100644 --- a/crates/agent/src/tools/delete_path_tool.rs +++ b/crates/agent/src/tools/delete_path_tool.rs @@ -1,9 +1,10 @@ use super::tool_permissions::{ - SensitiveSettingsKind, authorize_symlink_access, canonicalize_worktree_roots, - detect_symlink_escape, sensitive_settings_kind, + authorize_symlink_access, canonicalize_worktree_roots, detect_symlink_escape, + sensitive_settings_kind, }; use crate::{ - AgentTool, ToolCallEventStream, ToolInput, ToolPermissionDecision, decide_permission_for_path, + AgentTool, ToolCallEventStream, ToolInput, ToolPermissionDecision, + authorize_with_sensitive_settings, decide_permission_for_path, }; use action_log::ActionLog; use agent_client_protocol::schema as acp; @@ -132,14 +133,13 @@ impl AgentTool for DeletePathTool { let context = crate::ToolPermissionContext::new(Self::NAME, vec![path.clone()]); let title = format!("Delete {}", MarkdownInlineCode(&path)); - let title = match settings_kind { - Some(SensitiveSettingsKind::Local) => { - format!("{title} (local settings)") - } - Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), - None => title, - }; - event_stream.authorize(title, context, cx) + authorize_with_sensitive_settings( + settings_kind, + context, + &title, + &event_stream, + cx, + ) })), ToolPermissionDecision::Deny(_) => None, } diff --git a/crates/agent/src/tools/fetch_tool.rs b/crates/agent/src/tools/fetch_tool.rs index 8723a6d988..ca8e9a3697 100644 --- a/crates/agent/src/tools/fetch_tool.rs +++ b/crates/agent/src/tools/fetch_tool.rs @@ -3,7 +3,6 @@ use std::sync::Arc; use std::{borrow::Cow, cell::RefCell}; use agent_client_protocol::schema as acp; -use agent_settings::AgentSettings; use anyhow::{Context as _, Result, bail}; use futures::{AsyncReadExt as _, FutureExt as _}; use gpui::{App, AppContext as _, Task}; @@ -11,14 +10,10 @@ use html_to_markdown::{TagHandler, convert_html_to_markdown, markdown}; use http_client::{AsyncBody, HttpClientWithUrl}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use settings::Settings; use ui::SharedString; use util::markdown::{MarkdownEscaped, MarkdownInlineCode}; -use crate::{ - AgentTool, ToolCallEventStream, ToolInput, ToolPermissionDecision, - decide_permission_from_settings, -}; +use crate::{AgentTool, ToolCallEventStream, ToolInput}; #[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Copy)] enum ContentType { @@ -153,37 +148,22 @@ impl AgentTool for FetchTool { .await .map_err(|e| format!("Failed to receive tool input: {e}"))?; - let decision = cx.update(|cx| { - decide_permission_from_settings( - Self::NAME, - std::slice::from_ref(&input.url), - AgentSettings::get_global(cx), + let authorize = cx.update(|cx| { + let context = + crate::ToolPermissionContext::new(Self::NAME, vec![input.url.clone()]); + + event_stream.authorize( + format!("Fetch {}", MarkdownInlineCode(&input.url)), + context, + cx, ) }); - let authorize = match decision { - ToolPermissionDecision::Allow => None, - ToolPermissionDecision::Deny(reason) => { - return Err(reason); - } - ToolPermissionDecision::Confirm => Some(cx.update(|cx| { - let context = - crate::ToolPermissionContext::new(Self::NAME, vec![input.url.clone()]); - event_stream.authorize( - format!("Fetch {}", MarkdownInlineCode(&input.url)), - context, - cx, - ) - })), - }; - let fetch_task = cx.background_spawn({ let http_client = http_client.clone(); let url = input.url.clone(); async move { - if let Some(authorize) = authorize { - authorize.await?; - } + authorize.await?; Self::build_message(http_client, &url).await } }); diff --git a/crates/agent/src/tools/move_path_tool.rs b/crates/agent/src/tools/move_path_tool.rs index 4a8aad8455..abf45a7ec1 100644 --- a/crates/agent/src/tools/move_path_tool.rs +++ b/crates/agent/src/tools/move_path_tool.rs @@ -1,9 +1,10 @@ use super::tool_permissions::{ - SensitiveSettingsKind, authorize_symlink_escapes, canonicalize_worktree_roots, - collect_symlink_escapes, sensitive_settings_kind, + authorize_symlink_escapes, canonicalize_worktree_roots, collect_symlink_escapes, + sensitive_settings_kind, }; use crate::{ - AgentTool, ToolCallEventStream, ToolInput, ToolPermissionDecision, decide_permission_for_paths, + AgentTool, ToolCallEventStream, ToolInput, ToolPermissionDecision, + authorize_with_sensitive_settings, decide_permission_for_paths, }; use agent_client_protocol::schema as acp; use agent_settings::AgentSettings; @@ -154,12 +155,13 @@ impl AgentTool for MovePathTool { vec![input.source_path.clone(), input.destination_path.clone()], ); let title = format!("Move {src} to {dest}"); - let title = match sensitive_kind { - Some(SensitiveSettingsKind::Local) => format!("{title} (local settings)"), - Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), - None => title, - }; - event_stream.authorize(title, context, cx) + authorize_with_sensitive_settings( + sensitive_kind, + context, + &title, + &event_stream, + cx, + ) })) } else { None diff --git a/crates/agent/src/tools/restore_file_from_disk_tool.rs b/crates/agent/src/tools/restore_file_from_disk_tool.rs index 6953e234e9..aaab281046 100644 --- a/crates/agent/src/tools/restore_file_from_disk_tool.rs +++ b/crates/agent/src/tools/restore_file_from_disk_tool.rs @@ -1,7 +1,6 @@ use super::tool_permissions::{ - ResolvedProjectPath, SensitiveSettingsKind, authorize_symlink_access, - canonicalize_worktree_roots, path_has_symlink_escape, resolve_project_path, - sensitive_settings_kind, + ResolvedProjectPath, authorize_symlink_access, canonicalize_worktree_roots, + path_has_symlink_escape, resolve_project_path, sensitive_settings_kind, }; use agent_client_protocol::schema as acp; use agent_settings::AgentSettings; @@ -18,7 +17,8 @@ use std::sync::Arc; use util::markdown::MarkdownInlineCode; use crate::{ - AgentTool, ToolCallEventStream, ToolInput, ToolPermissionDecision, decide_permission_for_path, + AgentTool, ToolCallEventStream, ToolInput, ToolPermissionDecision, + authorize_with_sensitive_settings, decide_permission_for_path, }; /// Discards unsaved changes in open buffers by reloading file contents from disk. @@ -161,13 +161,16 @@ impl AgentTool for RestoreFileFromDiskTool { break; } } - let title = match settings_kind { - Some(SensitiveSettingsKind::Local) => format!("{title} (local settings)"), - Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), - None => title, - }; let context = crate::ToolPermissionContext::new(Self::NAME, confirmation_paths); - let authorize = cx.update(|cx| event_stream.authorize(title, context, cx)); + let authorize = cx.update(|cx| { + authorize_with_sensitive_settings( + settings_kind, + context, + &title, + &event_stream, + cx, + ) + }); authorize.await.map_err(|e| e.to_string())?; } let mut buffers_to_reload: FxHashSet> = FxHashSet::default(); diff --git a/crates/agent/src/tools/save_file_tool.rs b/crates/agent/src/tools/save_file_tool.rs index 904e9ba864..8fe27fc350 100644 --- a/crates/agent/src/tools/save_file_tool.rs +++ b/crates/agent/src/tools/save_file_tool.rs @@ -13,12 +13,12 @@ use std::sync::Arc; use util::markdown::MarkdownInlineCode; use super::tool_permissions::{ - ResolvedProjectPath, SensitiveSettingsKind, authorize_symlink_access, - canonicalize_worktree_roots, path_has_symlink_escape, resolve_project_path, - sensitive_settings_kind, + ResolvedProjectPath, authorize_symlink_access, canonicalize_worktree_roots, + path_has_symlink_escape, resolve_project_path, sensitive_settings_kind, }; use crate::{ - AgentTool, ToolCallEventStream, ToolInput, ToolPermissionDecision, decide_permission_for_path, + AgentTool, ToolCallEventStream, ToolInput, ToolPermissionDecision, + authorize_with_sensitive_settings, decide_permission_for_path, }; /// Saves files that have unsaved changes. @@ -155,14 +155,17 @@ impl AgentTool for SaveFileTool { break; } } - let title = match settings_kind { - Some(SensitiveSettingsKind::Local) => format!("{title} (local settings)"), - Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), - None => title, - }; let context = crate::ToolPermissionContext::new(Self::NAME, confirmation_paths.clone()); - let authorize = cx.update(|cx| event_stream.authorize(title, context, cx)); + let authorize = cx.update(|cx| { + authorize_with_sensitive_settings( + settings_kind, + context, + &title, + &event_stream, + cx, + ) + }); authorize.await.map_err(|e| e.to_string())?; } diff --git a/crates/agent/src/tools/terminal_tool.rs b/crates/agent/src/tools/terminal_tool.rs index 33560f2cf7..ffbd4393bc 100644 --- a/crates/agent/src/tools/terminal_tool.rs +++ b/crates/agent/src/tools/terminal_tool.rs @@ -1,11 +1,11 @@ use agent_client_protocol::schema as acp; -use agent_settings::AgentSettings; use anyhow::Result; use futures::FutureExt as _; use gpui::{App, Entity, SharedString, Task}; use project::Project; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +#[cfg(test)] use settings::Settings; use std::{ path::{Path, PathBuf}, @@ -14,10 +14,7 @@ use std::{ time::Duration, }; -use crate::{ - AgentTool, ThreadEnvironment, ToolCallEventStream, ToolInput, ToolPermissionDecision, - decide_permission_from_settings, -}; +use crate::{AgentTool, ThreadEnvironment, ToolCallEventStream, ToolInput}; const COMMAND_OUTPUT_LIMIT: u64 = 16 * 1024; @@ -100,35 +97,14 @@ impl AgentTool for TerminalTool { let (working_dir, authorize) = cx.update(|cx| { let working_dir = working_dir(&input, &self.project, cx).map_err(|err| err.to_string())?; - - let decision = decide_permission_from_settings( - Self::NAME, - std::slice::from_ref(&input.command), - AgentSettings::get_global(cx), - ); - - let authorize = match decision { - ToolPermissionDecision::Allow => None, - ToolPermissionDecision::Deny(reason) => { - return Err(reason); - } - ToolPermissionDecision::Confirm => { - let context = crate::ToolPermissionContext::new( - Self::NAME, - vec![input.command.clone()], - ); - Some(event_stream.authorize( - self.initial_title(Ok(input.clone()), cx), - context, - cx, - )) - } - }; - Ok((working_dir, authorize)) + let context = + crate::ToolPermissionContext::new(Self::NAME, vec![input.command.clone()]); + let authorize = + event_stream.authorize(self.initial_title(Ok(input.clone()), cx), context, cx); + Result::<_, String>::Ok((working_dir, authorize)) })?; - if let Some(authorize) = authorize { - authorize.await.map_err(|e| e.to_string())?; - } + + authorize.await.map_err(|e| e.to_string())?; let terminal = self .environment diff --git a/crates/agent/src/tools/tool_permissions.rs b/crates/agent/src/tools/tool_permissions.rs index 6fa6215a44..4304877cd0 100644 --- a/crates/agent/src/tools/tool_permissions.rs +++ b/crates/agent/src/tools/tool_permissions.rs @@ -251,7 +251,25 @@ pub fn authorize_symlink_access( vec![canonical_target.display().to_string()], ); - event_stream.authorize(title, context, cx) + event_stream.authorize_always_prompt(title, context, cx) +} + +pub fn authorize_with_sensitive_settings( + kind: Option, + context: ToolPermissionContext, + title: &str, + event_stream: &ToolCallEventStream, + cx: &mut App, +) -> Task> { + match kind { + Some(SensitiveSettingsKind::Local) => { + event_stream.authorize_always_prompt(format!("{title} (local settings)"), context, cx) + } + Some(SensitiveSettingsKind::Global) => { + event_stream.authorize_always_prompt(format!("{title} (settings)"), context, cx) + } + None => event_stream.authorize(title, context, cx), + } } /// Creates a single authorization prompt for multiple symlink escapes. @@ -287,7 +305,7 @@ pub fn authorize_symlink_escapes( .collect(), ); - event_stream.authorize(title, context, cx) + event_stream.authorize_always_prompt(title, context, cx) } /// Checks whether a path escapes the project via symlink, without creating @@ -467,7 +485,7 @@ pub fn authorize_file_edit( &tool_name, vec![path_owned.to_string_lossy().to_string()], ); - event_stream.authorize( + event_stream.authorize_always_prompt( format!("{} (local settings)", display_description), context, cx, @@ -481,7 +499,7 @@ pub fn authorize_file_edit( &tool_name, vec![path_owned.to_string_lossy().to_string()], ); - event_stream.authorize( + event_stream.authorize_always_prompt( format!("{} (settings)", display_description), context, cx, diff --git a/crates/agent/src/tools/web_search_tool.rs b/crates/agent/src/tools/web_search_tool.rs index 3b4b5a4563..271829c626 100644 --- a/crates/agent/src/tools/web_search_tool.rs +++ b/crates/agent/src/tools/web_search_tool.rs @@ -1,11 +1,7 @@ use std::sync::Arc; -use crate::{ - AgentTool, ToolCallEventStream, ToolInput, ToolPermissionDecision, - decide_permission_from_settings, -}; +use crate::{AgentTool, ToolCallEventStream, ToolInput}; use agent_client_protocol::schema as acp; -use agent_settings::AgentSettings; use anyhow::Result; use cloud_llm_client::WebSearchResponse; use futures::FutureExt as _; @@ -15,7 +11,6 @@ use language_model::{ }; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use settings::Settings; use ui::prelude::*; use util::markdown::MarkdownInlineCode; use web_search::WebSearchRegistry; @@ -86,43 +81,28 @@ impl AgentTool for WebSearchTool { error: format!("Failed to receive tool input: {e}"), })?; - let (authorize, search_task) = cx.update(|cx| { - let decision = decide_permission_from_settings( - Self::NAME, - std::slice::from_ref(&input.query), - AgentSettings::get_global(cx), - ); - - let authorize = match decision { - ToolPermissionDecision::Allow => None, - ToolPermissionDecision::Deny(reason) => { - return Err(WebSearchToolOutput::Error { error: reason }); - } - ToolPermissionDecision::Confirm => { - let context = - crate::ToolPermissionContext::new(Self::NAME, vec![input.query.clone()]); - Some(event_stream.authorize( - format!("Search the web for {}", MarkdownInlineCode(&input.query)), - context, - cx, - )) - } - }; + let authorize = cx.update(|cx| { + let context = + crate::ToolPermissionContext::new(Self::NAME, vec![input.query.clone()]); + event_stream.authorize( + format!("Search the web for {}", MarkdownInlineCode(&input.query)), + context, + cx, + ) + }); + authorize + .await + .map_err(|e| WebSearchToolOutput::Error { error: e.to_string() })?; + let search_task = cx.update(|cx| { let Some(provider) = WebSearchRegistry::read_global(cx).active_provider() else { return Err(WebSearchToolOutput::Error { error: "Web search is not available.".to_string(), }); }; - - let search_task = provider.search(input.query, cx); - Ok((authorize, search_task)) + Ok(provider.search(input.query, cx)) })?; - if let Some(authorize) = authorize { - authorize.await.map_err(|e| WebSearchToolOutput::Error { error: e.to_string() })?; - } - let response = futures::select! { result = search_task.fuse() => { match result {