diff --git a/crates/agent/src/pattern_extraction.rs b/crates/agent/src/pattern_extraction.rs index 96accf5421bf175ad2ca24a141e43e3c5432be3f..56fb8cfaa1d840271b0b46899eadaabc36662fbc 100644 --- a/crates/agent/src/pattern_extraction.rs +++ b/crates/agent/src/pattern_extraction.rs @@ -1,6 +1,12 @@ use crate::shell_parser::extract_commands; +use std::path::{Path, PathBuf}; use url::Url; +/// Normalize path separators to forward slashes for consistent cross-platform patterns. +fn normalize_separators(path_str: &str) -> String { + path_str.replace('\\', "/") +} + /// Extracts the command name from a shell command using the shell parser. /// /// This parses the command properly to extract just the command name (first word), @@ -41,23 +47,64 @@ pub fn extract_terminal_pattern_display(command: &str) -> Option { } pub fn extract_path_pattern(path: &str) -> Option { - let parent = std::path::Path::new(path).parent()?; - let parent_str = parent.to_str()?; + let parent = Path::new(path).parent()?; + let parent_str = normalize_separators(parent.to_str()?); if parent_str.is_empty() || parent_str == "/" { return None; } - Some(format!("^{}/", regex::escape(parent_str))) + Some(format!("^{}/", regex::escape(&parent_str))) } pub fn extract_path_pattern_display(path: &str) -> Option { - let parent = std::path::Path::new(path).parent()?; - let parent_str = parent.to_str()?; + let parent = Path::new(path).parent()?; + let parent_str = normalize_separators(parent.to_str()?); if parent_str.is_empty() || parent_str == "/" { return None; } Some(format!("{}/", parent_str)) } +fn common_parent_dir(path_a: &str, path_b: &str) -> Option { + let parent_a = Path::new(path_a).parent()?; + let parent_b = Path::new(path_b).parent()?; + + let components_a: Vec<_> = parent_a.components().collect(); + let components_b: Vec<_> = parent_b.components().collect(); + + let common_count = components_a + .iter() + .zip(components_b.iter()) + .take_while(|(a, b)| a == b) + .count(); + + if common_count == 0 { + return None; + } + + let common: PathBuf = components_a[..common_count].iter().collect(); + Some(common) +} + +pub fn extract_copy_move_pattern(input: &str) -> Option { + let (source, dest) = input.split_once('\n')?; + let common = common_parent_dir(source, dest)?; + let common_str = normalize_separators(common.to_str()?); + if common_str.is_empty() || common_str == "/" { + return None; + } + Some(format!("^{}/", regex::escape(&common_str))) +} + +pub fn extract_copy_move_pattern_display(input: &str) -> Option { + let (source, dest) = input.split_once('\n')?; + let common = common_parent_dir(source, dest)?; + let common_str = normalize_separators(common.to_str()?); + if common_str.is_empty() || common_str == "/" { + return None; + } + Some(format!("{}/", common_str)) +} + pub fn extract_url_pattern(url: &str) -> Option { let parsed = Url::parse(url).ok()?; let domain = parsed.host_str()?; @@ -170,4 +217,56 @@ mod tests { Some("^https?://test\\.example\\.com".to_string()) ); } + + #[test] + fn test_extract_copy_move_pattern_same_directory() { + assert_eq!( + extract_copy_move_pattern( + "/Users/alice/project/src/old.rs\n/Users/alice/project/src/new.rs" + ), + Some("^/Users/alice/project/src/".to_string()) + ); + } + + #[test] + fn test_extract_copy_move_pattern_sibling_directories() { + assert_eq!( + extract_copy_move_pattern( + "/Users/alice/project/src/old.rs\n/Users/alice/project/dst/new.rs" + ), + Some("^/Users/alice/project/".to_string()) + ); + } + + #[test] + fn test_extract_copy_move_pattern_no_common_prefix() { + assert_eq!( + extract_copy_move_pattern("/home/file.txt\n/tmp/file.txt"), + None + ); + } + + #[test] + fn test_extract_copy_move_pattern_relative_paths() { + assert_eq!( + extract_copy_move_pattern("src/old.rs\nsrc/new.rs"), + Some("^src/".to_string()) + ); + } + + #[test] + fn test_extract_copy_move_pattern_display() { + assert_eq!( + extract_copy_move_pattern_display( + "/Users/alice/project/src/old.rs\n/Users/alice/project/dst/new.rs" + ), + Some("/Users/alice/project/".to_string()) + ); + } + + #[test] + fn test_extract_copy_move_pattern_no_arrow() { + assert_eq!(extract_copy_move_pattern("just/a/path.rs"), None); + assert_eq!(extract_copy_move_pattern_display("just/a/path.rs"), None); + } } diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index 119c9e936975aaae53136330c6c91b961313bb77..1947553c1670b98ec4a33b2333981257dc205b10 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -675,9 +675,16 @@ impl ToolPermissionContext { extract_terminal_pattern(input_value), extract_terminal_pattern_display(input_value), ) + } else if tool_name == CopyPathTool::NAME || tool_name == MovePathTool::NAME { + // input_value is "source\ndestination"; extract a pattern from the + // common parent directory of both paths so that "always allow" covers + // future checks against both the source and the destination. + ( + extract_copy_move_pattern(input_value), + extract_copy_move_pattern_display(input_value), + ) } else if tool_name == EditFileTool::NAME || tool_name == DeletePathTool::NAME - || tool_name == MovePathTool::NAME || tool_name == CreateDirectoryTool::NAME || tool_name == SaveFileTool::NAME { diff --git a/crates/agent/src/tools/copy_path_tool.rs b/crates/agent/src/tools/copy_path_tool.rs index fdeb426f64bf0a5a31fc917f0fa83bd5cf5c9eec..0d8e5b3cfe960dac806c0d9b3b570961ff747d55 100644 --- a/crates/agent/src/tools/copy_path_tool.rs +++ b/crates/agent/src/tools/copy_path_tool.rs @@ -1,15 +1,17 @@ -use crate::{ - AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_from_settings, +use super::edit_file_tool::{ + SensitiveSettingsKind, is_sensitive_settings_path, sensitive_settings_kind, }; +use crate::{AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_for_path}; use agent_client_protocol::ToolKind; use agent_settings::AgentSettings; use anyhow::{Context as _, Result, anyhow}; use futures::FutureExt as _; -use gpui::{App, AppContext, Entity, Task}; +use gpui::{App, Entity, Task}; use project::Project; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use settings::Settings; +use std::path::Path; use std::sync::Arc; use util::markdown::MarkdownInlineCode; @@ -83,57 +85,71 @@ impl AgentTool for CopyPathTool { ) -> Task> { let settings = AgentSettings::get_global(cx); - let source_decision = - decide_permission_from_settings(Self::NAME, &input.source_path, settings); + let source_decision = decide_permission_for_path(Self::NAME, &input.source_path, settings); if let ToolPermissionDecision::Deny(reason) = source_decision { return Task::ready(Err(anyhow!("{}", reason))); } let dest_decision = - decide_permission_from_settings(Self::NAME, &input.destination_path, settings); + decide_permission_for_path(Self::NAME, &input.destination_path, settings); if let ToolPermissionDecision::Deny(reason) = dest_decision { return Task::ready(Err(anyhow!("{}", reason))); } let needs_confirmation = matches!(source_decision, ToolPermissionDecision::Confirm) - || matches!(dest_decision, ToolPermissionDecision::Confirm); + || matches!(dest_decision, ToolPermissionDecision::Confirm) + || (!settings.always_allow_tool_actions + && matches!(source_decision, ToolPermissionDecision::Allow) + && is_sensitive_settings_path(Path::new(&input.source_path))) + || (!settings.always_allow_tool_actions + && matches!(dest_decision, ToolPermissionDecision::Allow) + && is_sensitive_settings_path(Path::new(&input.destination_path))); let authorize = if needs_confirmation { let src = MarkdownInlineCode(&input.source_path); let dest = MarkdownInlineCode(&input.destination_path); let context = crate::ToolPermissionContext { tool_name: Self::NAME.to_string(), - input_value: input.source_path.clone(), + input_value: format!("{}\n{}", input.source_path, input.destination_path), }; - Some(event_stream.authorize(format!("Copy {src} to {dest}"), context, cx)) + let title = format!("Copy {src} to {dest}"); + let sensitive_kind = sensitive_settings_kind(Path::new(&input.source_path)) + .or_else(|| sensitive_settings_kind(Path::new(&input.destination_path))); + let title = match sensitive_kind { + Some(SensitiveSettingsKind::Local) => format!("{title} (local settings)"), + Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), + None => title, + }; + Some(event_stream.authorize(title, context, cx)) } else { None }; - let copy_task = self.project.update(cx, |project, cx| { - match project - .find_project_path(&input.source_path, cx) - .and_then(|project_path| project.entry_for_path(&project_path, cx)) - { - Some(entity) => match project.find_project_path(&input.destination_path, cx) { - Some(project_path) => project.copy_entry(entity.id, project_path, cx), - None => Task::ready(Err(anyhow!( - "Destination path {} was outside the project.", - input.destination_path - ))), - }, - None => Task::ready(Err(anyhow!( - "Source path {} was not found in the project.", - input.source_path - ))), - } - }); - - cx.background_spawn(async move { + let project = self.project.clone(); + cx.spawn(async move |cx| { if let Some(authorize) = authorize { authorize.await?; } + let copy_task = project.update(cx, |project, cx| { + match project + .find_project_path(&input.source_path, cx) + .and_then(|project_path| project.entry_for_path(&project_path, cx)) + { + Some(entity) => match project.find_project_path(&input.destination_path, cx) { + Some(project_path) => Ok(project.copy_entry(entity.id, project_path, cx)), + None => Err(anyhow!( + "Destination path {} was outside the project.", + input.destination_path + )), + }, + None => Err(anyhow!( + "Source path {} was not found in the project.", + input.source_path + )), + } + })?; + let result = futures::select! { result = copy_task.fuse() => result, _ = event_stream.cancelled_by_user().fuse() => { diff --git a/crates/agent/src/tools/create_directory_tool.rs b/crates/agent/src/tools/create_directory_tool.rs index daaa5c946ea685cc3e2a113d47364a83a6de5c73..2683f6237c27c9dcc0bc599143cabfa3bdeb7dff 100644 --- a/crates/agent/src/tools/create_directory_tool.rs +++ b/crates/agent/src/tools/create_directory_tool.rs @@ -1,3 +1,4 @@ +use super::edit_file_tool::{SensitiveSettingsKind, sensitive_settings_kind}; use agent_client_protocol::ToolKind; use agent_settings::AgentSettings; use anyhow::{Context as _, Result, anyhow}; @@ -7,12 +8,11 @@ use project::Project; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use settings::Settings; +use std::path::Path; use std::sync::Arc; use util::markdown::MarkdownInlineCode; -use crate::{ - AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_from_settings, -}; +use crate::{AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_for_path}; /// Creates a new directory at the specified path within the project. Returns confirmation that the directory was created. /// @@ -71,7 +71,15 @@ impl AgentTool for CreateDirectoryTool { cx: &mut App, ) -> Task> { let settings = AgentSettings::get_global(cx); - let decision = decide_permission_from_settings(Self::NAME, &input.path, settings); + let mut decision = decide_permission_for_path(Self::NAME, &input.path, settings); + let sensitive_kind = sensitive_settings_kind(Path::new(&input.path)); + + if matches!(decision, ToolPermissionDecision::Allow) + && !settings.always_allow_tool_actions + && sensitive_kind.is_some() + { + decision = ToolPermissionDecision::Confirm; + } let authorize = match decision { ToolPermissionDecision::Allow => None, @@ -79,35 +87,35 @@ impl AgentTool for CreateDirectoryTool { return Task::ready(Err(anyhow!("{}", reason))); } ToolPermissionDecision::Confirm => { + 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 { tool_name: Self::NAME.to_string(), input_value: input.path.clone(), }; - Some(event_stream.authorize( - format!("Create directory {}", MarkdownInlineCode(&input.path)), - context, - cx, - )) + Some(event_stream.authorize(title, context, cx)) } }; - let project_path = match self.project.read(cx).find_project_path(&input.path, cx) { - Some(project_path) => project_path, - None => { - return Task::ready(Err(anyhow!("Path to create was outside the project"))); - } - }; let destination_path: Arc = input.path.as_str().into(); - let create_entry = self.project.update(cx, |project, cx| { - project.create_entry(project_path.clone(), true, cx) - }); - - cx.spawn(async move |_cx| { + let project = self.project.clone(); + cx.spawn(async move |cx| { if let Some(authorize) = authorize { authorize.await?; } + let create_entry = project.update(cx, |project, cx| { + match project.find_project_path(&input.path, cx) { + Some(project_path) => Ok(project.create_entry(project_path, true, cx)), + None => Err(anyhow!("Path to create was outside the project")), + } + })?; + futures::select! { result = create_entry.fuse() => { result.with_context(|| format!("Creating directory {destination_path}"))?; diff --git a/crates/agent/src/tools/delete_path_tool.rs b/crates/agent/src/tools/delete_path_tool.rs index 5787b867bcf22595321266eb05e734903c7dbd31..8f0c721b289e495ccb7e12e331a12dfcf841a14a 100644 --- a/crates/agent/src/tools/delete_path_tool.rs +++ b/crates/agent/src/tools/delete_path_tool.rs @@ -1,6 +1,7 @@ -use crate::{ - AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_from_settings, +use super::edit_file_tool::{ + SensitiveSettingsKind, is_sensitive_settings_path, sensitive_settings_kind, }; +use crate::{AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_for_path}; use action_log::ActionLog; use agent_client_protocol::ToolKind; use agent_settings::AgentSettings; @@ -11,6 +12,7 @@ use project::{Project, ProjectPath}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use settings::Settings; +use std::path::Path; use std::sync::Arc; use util::markdown::MarkdownInlineCode; @@ -76,7 +78,14 @@ impl AgentTool for DeletePathTool { let path = input.path; let settings = AgentSettings::get_global(cx); - let decision = decide_permission_from_settings(Self::NAME, &path, settings); + let mut decision = decide_permission_for_path(Self::NAME, &path, settings); + + if matches!(decision, ToolPermissionDecision::Allow) + && !settings.always_allow_tool_actions + && is_sensitive_settings_path(Path::new(&path)) + { + decision = ToolPermissionDecision::Confirm; + } let authorize = match decision { ToolPermissionDecision::Allow => None, @@ -88,53 +97,16 @@ impl AgentTool for DeletePathTool { tool_name: Self::NAME.to_string(), input_value: path.clone(), }; - Some(event_stream.authorize( - format!("Delete {}", MarkdownInlineCode(&path)), - context, - cx, - )) + let title = format!("Delete {}", MarkdownInlineCode(&path)); + let title = match sensitive_settings_kind(Path::new(&path)) { + Some(SensitiveSettingsKind::Local) => format!("{title} (local settings)"), + Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), + None => title, + }; + Some(event_stream.authorize(title, context, cx)) } }; - let Some(project_path) = self.project.read(cx).find_project_path(&path, cx) else { - return Task::ready(Err(anyhow!( - "Couldn't delete {path} because that path isn't in this project." - ))); - }; - - let Some(worktree) = self - .project - .read(cx) - .worktree_for_id(project_path.worktree_id, cx) - else { - return Task::ready(Err(anyhow!( - "Couldn't delete {path} because that path isn't in this project." - ))); - }; - - let worktree_snapshot = worktree.read(cx).snapshot(); - let (mut paths_tx, mut paths_rx) = mpsc::channel(256); - cx.background_spawn({ - let project_path = project_path.clone(); - async move { - for entry in - worktree_snapshot.traverse_from_path(true, false, false, &project_path.path) - { - if !entry.path.starts_with(&project_path.path) { - break; - } - paths_tx - .send(ProjectPath { - worktree_id: project_path.worktree_id, - path: entry.path.clone(), - }) - .await?; - } - anyhow::Ok(()) - } - }) - .detach(); - let project = self.project.clone(); let action_log = self.action_log.clone(); cx.spawn(async move |cx| { @@ -142,6 +114,41 @@ impl AgentTool for DeletePathTool { authorize.await?; } + let (project_path, worktree_snapshot) = project.read_with(cx, |project, cx| { + let project_path = project.find_project_path(&path, cx).ok_or_else(|| { + anyhow!("Couldn't delete {path} because that path isn't in this project.") + })?; + let worktree = project + .worktree_for_id(project_path.worktree_id, cx) + .ok_or_else(|| { + anyhow!("Couldn't delete {path} because that path isn't in this project.") + })?; + let worktree_snapshot = worktree.read(cx).snapshot(); + anyhow::Ok((project_path, worktree_snapshot)) + })?; + + let (mut paths_tx, mut paths_rx) = mpsc::channel(256); + cx.background_spawn({ + let project_path = project_path.clone(); + async move { + for entry in + worktree_snapshot.traverse_from_path(true, false, false, &project_path.path) + { + if !entry.path.starts_with(&project_path.path) { + break; + } + paths_tx + .send(ProjectPath { + worktree_id: project_path.worktree_id, + path: entry.path.clone(), + }) + .await?; + } + anyhow::Ok(()) + } + }) + .detach(); + loop { let path_result = futures::select! { path = paths_rx.next().fuse() => path, diff --git a/crates/agent/src/tools/edit_file_tool.rs b/crates/agent/src/tools/edit_file_tool.rs index 0b63587a5aae031c17c0890269de86c63a09f306..d5a6a51433cbfc66c1c735bb59afd4a6ca072d5e 100644 --- a/crates/agent/src/tools/edit_file_tool.rs +++ b/crates/agent/src/tools/edit_file_tool.rs @@ -2,7 +2,7 @@ use super::restore_file_from_disk_tool::RestoreFileFromDiskTool; use super::save_file_tool::SaveFileTool; use crate::{ AgentTool, Templates, Thread, ToolCallEventStream, ToolPermissionDecision, - decide_permission_from_settings, + decide_permission_for_path, edit_agent::{EditAgent, EditAgentOutput, EditAgentOutputEvent, EditFormat}, }; use acp_thread::Diff; @@ -161,72 +161,154 @@ impl EditFileTool { event_stream: &ToolCallEventStream, cx: &mut App, ) -> Task> { - let path_str = input.path.to_string_lossy(); - let settings = agent_settings::AgentSettings::get_global(cx); - let decision = decide_permission_from_settings(Self::NAME, &path_str, settings); - - match decision { - ToolPermissionDecision::Allow => return Task::ready(Ok(())), - ToolPermissionDecision::Deny(reason) => { - return Task::ready(Err(anyhow!("{}", reason))); + authorize_file_edit( + Self::NAME, + &input.path, + &input.display_description, + &self.thread, + event_stream, + cx, + ) + } +} + +pub enum SensitiveSettingsKind { + Local, + Global, +} + +/// Canonicalize a path, stripping the Windows extended-length path prefix (`\\?\`) +/// that `std::fs::canonicalize` adds on Windows. This ensures that canonicalized +/// paths can be compared with non-canonicalized paths via `starts_with`. +fn safe_canonicalize(path: &Path) -> std::io::Result { + let canonical = std::fs::canonicalize(path)?; + #[cfg(target_os = "windows")] + { + let s = canonical.to_string_lossy(); + if let Some(stripped) = s.strip_prefix("\\\\?\\") { + return Ok(PathBuf::from(stripped)); + } + } + Ok(canonical) +} + +/// Returns the kind of sensitive settings location this path targets, if any: +/// either inside a `.zed/` local-settings directory or inside the global config dir. +pub fn sensitive_settings_kind(path: &Path) -> Option { + let local_settings_folder = paths::local_settings_folder_name(); + if path.components().any(|component| { + component.as_os_str() == <_ as AsRef>::as_ref(&local_settings_folder) + }) { + return Some(SensitiveSettingsKind::Local); + } + + // Walk up the path hierarchy until we find an ancestor that exists and can + // be canonicalized, then reconstruct the path from there. This handles + // cases where multiple levels of subdirectories don't exist yet (e.g. + // ~/.config/zed/new_subdir/evil.json). + let canonical_path = { + let mut current: Option<&Path> = Some(path); + let mut suffix_components = Vec::new(); + loop { + match current { + Some(ancestor) => match safe_canonicalize(ancestor) { + Ok(canonical) => { + let mut result = canonical; + for component in suffix_components.into_iter().rev() { + result.push(component); + } + break Some(result); + } + Err(_) => { + if let Some(file_name) = ancestor.file_name() { + suffix_components.push(file_name.to_os_string()); + } + current = ancestor.parent(); + } + }, + None => break None, } - ToolPermissionDecision::Confirm => {} } + }; + if let Some(canonical_path) = canonical_path { + let config_dir = safe_canonicalize(paths::config_dir()) + .unwrap_or_else(|_| paths::config_dir().to_path_buf()); + if canonical_path.starts_with(&config_dir) { + return Some(SensitiveSettingsKind::Global); + } + } + + None +} + +pub fn is_sensitive_settings_path(path: &Path) -> bool { + sensitive_settings_kind(path).is_some() +} + +pub fn authorize_file_edit( + tool_name: &str, + path: &Path, + display_description: &str, + thread: &WeakEntity, + event_stream: &ToolCallEventStream, + cx: &mut App, +) -> Task> { + let path_str = path.to_string_lossy(); + let settings = agent_settings::AgentSettings::get_global(cx); + let decision = decide_permission_for_path(tool_name, &path_str, settings); + + if let ToolPermissionDecision::Deny(reason) = decision { + return Task::ready(Err(anyhow!("{}", reason))); + } - // If any path component matches the local settings folder, then this could affect - // the editor in ways beyond the project source, so prompt. - let local_settings_folder = paths::local_settings_folder_name(); - let path = Path::new(&input.path); - if path.components().any(|component| { - component.as_os_str() == <_ as AsRef>::as_ref(&local_settings_folder) - }) { + let explicitly_allowed = matches!(decision, ToolPermissionDecision::Allow); + + if explicitly_allowed + && (settings.always_allow_tool_actions || !is_sensitive_settings_path(path)) + { + return Task::ready(Ok(())); + } + + match sensitive_settings_kind(path) { + Some(SensitiveSettingsKind::Local) => { let context = crate::ToolPermissionContext { - tool_name: Self::NAME.to_string(), + tool_name: tool_name.to_string(), input_value: path_str.to_string(), }; return event_stream.authorize( - format!("{} (local settings)", input.display_description), + format!("{} (local settings)", display_description), context, cx, ); } - - // It's also possible that the global config dir is configured to be inside the project, - // so check for that edge case too. - // TODO this is broken when remoting - if let Ok(canonical_path) = std::fs::canonicalize(&input.path) - && canonical_path.starts_with(paths::config_dir()) - { + Some(SensitiveSettingsKind::Global) => { let context = crate::ToolPermissionContext { - tool_name: Self::NAME.to_string(), + tool_name: tool_name.to_string(), input_value: path_str.to_string(), }; return event_stream.authorize( - format!("{} (global settings)", input.display_description), + format!("{} (settings)", display_description), context, cx, ); } + None => {} + } - // Check if path is inside the global config directory - // First check if it's already inside project - if not, try to canonicalize - let Ok(project_path) = self.thread.read_with(cx, |thread, cx| { - thread.project().read(cx).find_project_path(&input.path, cx) - }) else { - return Task::ready(Err(anyhow!("thread was dropped"))); + let Ok(project_path) = thread.read_with(cx, |thread, cx| { + thread.project().read(cx).find_project_path(path, cx) + }) else { + return Task::ready(Err(anyhow!("thread was dropped"))); + }; + + if project_path.is_some() { + Task::ready(Ok(())) + } else { + let context = crate::ToolPermissionContext { + tool_name: tool_name.to_string(), + input_value: path_str.to_string(), }; - - // If the path is inside the project, and it's not one of the above edge cases, - // then no confirmation is necessary. Otherwise, confirmation is necessary. - if project_path.is_some() { - Task::ready(Ok(())) - } else { - let context = crate::ToolPermissionContext { - tool_name: Self::NAME.to_string(), - input_value: path_str.to_string(), - }; - event_stream.authorize(&input.display_description, context, cx) - } + event_stream.authorize(display_description, context, cx) } } @@ -2283,4 +2365,42 @@ mod tests { error_msg ); } + + #[test] + fn test_sensitive_settings_kind_detects_nonexistent_subdirectory() { + let config_dir = paths::config_dir(); + let path = config_dir.join("nonexistent_subdir_xyz").join("evil.json"); + assert!( + matches!( + sensitive_settings_kind(&path), + Some(SensitiveSettingsKind::Global) + ), + "Path in non-existent subdirectory of config dir should be detected as sensitive: {:?}", + path + ); + } + + #[test] + fn test_sensitive_settings_kind_detects_deeply_nested_nonexistent_subdirectory() { + let config_dir = paths::config_dir(); + let path = config_dir.join("a").join("b").join("c").join("evil.json"); + assert!( + matches!( + sensitive_settings_kind(&path), + Some(SensitiveSettingsKind::Global) + ), + "Path in deeply nested non-existent subdirectory of config dir should be detected as sensitive: {:?}", + path + ); + } + + #[test] + fn test_sensitive_settings_kind_returns_none_for_non_config_path() { + let path = PathBuf::from("/tmp/not_a_config_dir/some_file.json"); + assert!( + sensitive_settings_kind(&path).is_none(), + "Path outside config dir should not be detected as sensitive: {:?}", + path + ); + } } diff --git a/crates/agent/src/tools/move_path_tool.rs b/crates/agent/src/tools/move_path_tool.rs index 77091e22d81985608f32565c0133469516bdadee..572f01c04e76269d37ebb4d0ee271fe534725560 100644 --- a/crates/agent/src/tools/move_path_tool.rs +++ b/crates/agent/src/tools/move_path_tool.rs @@ -1,11 +1,12 @@ -use crate::{ - AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_from_settings, +use super::edit_file_tool::{ + SensitiveSettingsKind, is_sensitive_settings_path, sensitive_settings_kind, }; +use crate::{AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_for_path}; use agent_client_protocol::ToolKind; use agent_settings::AgentSettings; use anyhow::{Context as _, Result, anyhow}; use futures::FutureExt as _; -use gpui::{App, AppContext, Entity, SharedString, Task}; +use gpui::{App, Entity, SharedString, Task}; use project::Project; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -97,57 +98,71 @@ impl AgentTool for MovePathTool { ) -> Task> { let settings = AgentSettings::get_global(cx); - let source_decision = - decide_permission_from_settings(Self::NAME, &input.source_path, settings); + let source_decision = decide_permission_for_path(Self::NAME, &input.source_path, settings); if let ToolPermissionDecision::Deny(reason) = source_decision { return Task::ready(Err(anyhow!("{}", reason))); } let dest_decision = - decide_permission_from_settings(Self::NAME, &input.destination_path, settings); + decide_permission_for_path(Self::NAME, &input.destination_path, settings); if let ToolPermissionDecision::Deny(reason) = dest_decision { return Task::ready(Err(anyhow!("{}", reason))); } let needs_confirmation = matches!(source_decision, ToolPermissionDecision::Confirm) - || matches!(dest_decision, ToolPermissionDecision::Confirm); + || matches!(dest_decision, ToolPermissionDecision::Confirm) + || (!settings.always_allow_tool_actions + && matches!(source_decision, ToolPermissionDecision::Allow) + && is_sensitive_settings_path(Path::new(&input.source_path))) + || (!settings.always_allow_tool_actions + && matches!(dest_decision, ToolPermissionDecision::Allow) + && is_sensitive_settings_path(Path::new(&input.destination_path))); let authorize = if needs_confirmation { let src = MarkdownInlineCode(&input.source_path); let dest = MarkdownInlineCode(&input.destination_path); let context = crate::ToolPermissionContext { tool_name: Self::NAME.to_string(), - input_value: input.source_path.clone(), + input_value: format!("{}\n{}", input.source_path, input.destination_path), }; - Some(event_stream.authorize(format!("Move {src} to {dest}"), context, cx)) + let title = format!("Move {src} to {dest}"); + let settings_kind = sensitive_settings_kind(Path::new(&input.source_path)) + .or_else(|| sensitive_settings_kind(Path::new(&input.destination_path))); + let title = match settings_kind { + Some(SensitiveSettingsKind::Local) => format!("{title} (local settings)"), + Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), + None => title, + }; + Some(event_stream.authorize(title, context, cx)) } else { None }; - let rename_task = self.project.update(cx, |project, cx| { - match project - .find_project_path(&input.source_path, cx) - .and_then(|project_path| project.entry_for_path(&project_path, cx)) - { - Some(entity) => match project.find_project_path(&input.destination_path, cx) { - Some(project_path) => project.rename_entry(entity.id, project_path, cx), - None => Task::ready(Err(anyhow!( - "Destination path {} was outside the project.", - input.destination_path - ))), - }, - None => Task::ready(Err(anyhow!( - "Source path {} was not found in the project.", - input.source_path - ))), - } - }); - - cx.background_spawn(async move { + let project = self.project.clone(); + cx.spawn(async move |cx| { if let Some(authorize) = authorize { authorize.await?; } + let rename_task = project.update(cx, |project, cx| { + match project + .find_project_path(&input.source_path, cx) + .and_then(|project_path| project.entry_for_path(&project_path, cx)) + { + Some(entity) => match project.find_project_path(&input.destination_path, cx) { + Some(project_path) => Ok(project.rename_entry(entity.id, project_path, cx)), + None => Err(anyhow!( + "Destination path {} was outside the project.", + input.destination_path + )), + }, + None => Err(anyhow!( + "Source path {} was not found in the project.", + input.source_path + )), + } + })?; + let result = futures::select! { result = rename_task.fuse() => result, _ = event_stream.cancelled_by_user().fuse() => { diff --git a/crates/agent/src/tools/save_file_tool.rs b/crates/agent/src/tools/save_file_tool.rs index c199df414c54e22556a3d6dc562ce93c28fc5762..70994c55db115264dbff9b2b0909bf6ed07cdf18 100644 --- a/crates/agent/src/tools/save_file_tool.rs +++ b/crates/agent/src/tools/save_file_tool.rs @@ -9,13 +9,32 @@ use project::Project; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use settings::Settings; -use std::path::PathBuf; +use std::path::{Component, Path, PathBuf}; use std::sync::Arc; use util::markdown::MarkdownInlineCode; -use crate::{ - AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_from_settings, +use super::edit_file_tool::{ + SensitiveSettingsKind, is_sensitive_settings_path, sensitive_settings_kind, }; +use crate::{AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_for_path}; + +fn common_parent_for_paths(paths: &[String]) -> Option { + let first = paths.first()?; + let mut common: Vec> = Path::new(first).parent()?.components().collect(); + for path in &paths[1..] { + let parent: Vec> = Path::new(path).parent()?.components().collect(); + let prefix_len = common + .iter() + .zip(parent.iter()) + .take_while(|(a, b)| a == b) + .count(); + common.truncate(prefix_len); + } + if common.is_empty() { + return None; + } + Some(common.iter().collect()) +} /// Saves files that have unsaved changes. /// @@ -66,53 +85,65 @@ impl AgentTool for SaveFileTool { cx: &mut App, ) -> Task> { let settings = AgentSettings::get_global(cx); - let mut needs_confirmation = false; + let mut confirmation_paths: Vec = Vec::new(); for path in &input.paths { let path_str = path.to_string_lossy(); - let decision = decide_permission_from_settings(Self::NAME, &path_str, settings); + let decision = decide_permission_for_path(Self::NAME, &path_str, settings); match decision { - ToolPermissionDecision::Allow => {} + ToolPermissionDecision::Allow => { + if !settings.always_allow_tool_actions + && is_sensitive_settings_path(Path::new(&*path_str)) + { + confirmation_paths.push(path_str.to_string()); + } + } ToolPermissionDecision::Deny(reason) => { return Task::ready(Err(anyhow::anyhow!("{}", reason))); } ToolPermissionDecision::Confirm => { - needs_confirmation = true; + confirmation_paths.push(path_str.to_string()); } } } - let authorize = if needs_confirmation { - let title = if input.paths.len() == 1 { - format!( - "Save {}", - MarkdownInlineCode(&input.paths[0].to_string_lossy()) - ) + let authorize = if !confirmation_paths.is_empty() { + let title = if confirmation_paths.len() == 1 { + format!("Save {}", MarkdownInlineCode(&confirmation_paths[0])) } else { - let paths: Vec<_> = input - .paths + let paths: Vec<_> = confirmation_paths .iter() .take(3) - .map(|p| p.to_string_lossy().to_string()) + .map(|p| p.as_str()) .collect(); - if input.paths.len() > 3 { + if confirmation_paths.len() > 3 { format!( "Save {}, and {} more", paths.join(", "), - input.paths.len() - 3 + confirmation_paths.len() - 3 ) } else { format!("Save {}", paths.join(", ")) } }; - let first_path = input - .paths - .first() - .map(|p| p.to_string_lossy().to_string()) - .unwrap_or_default(); + let sensitive_kind = confirmation_paths + .iter() + .find_map(|p| sensitive_settings_kind(Path::new(p))); + let title = match sensitive_kind { + Some(SensitiveSettingsKind::Local) => format!("{title} (local settings)"), + Some(SensitiveSettingsKind::Global) => format!("{title} (settings)"), + None => title, + }; + let input_value = if confirmation_paths.len() == 1 { + confirmation_paths[0].clone() + } else { + common_parent_for_paths(&confirmation_paths) + .map(|parent| format!("{}/_", parent.display())) + .unwrap_or_else(|| confirmation_paths[0].clone()) + }; let context = crate::ToolPermissionContext { tool_name: Self::NAME.to_string(), - input_value: first_path, + input_value, }; Some(event_stream.authorize(title, context, cx)) } else { diff --git a/crates/agent/src/tools/streaming_edit_file_tool.rs b/crates/agent/src/tools/streaming_edit_file_tool.rs index dc921fe08e84d3d416eb012472fac176c0187637..bbdc05c05bca13c582f16da61296e2cb3001a48e 100644 --- a/crates/agent/src/tools/streaming_edit_file_tool.rs +++ b/crates/agent/src/tools/streaming_edit_file_tool.rs @@ -2,8 +2,8 @@ use super::edit_file_tool::EditFileTool; use super::restore_file_from_disk_tool::RestoreFileFromDiskTool; use super::save_file_tool::SaveFileTool; use crate::{ - AgentTool, Templates, Thread, ToolCallEventStream, ToolPermissionDecision, - decide_permission_from_settings, edit_agent::streaming_fuzzy_matcher::StreamingFuzzyMatcher, + AgentTool, Templates, Thread, ToolCallEventStream, + edit_agent::streaming_fuzzy_matcher::StreamingFuzzyMatcher, }; use acp_thread::Diff; use agent_client_protocol::{self as acp, ToolCallLocation, ToolCallUpdateFields}; @@ -11,14 +11,11 @@ use anyhow::{Context as _, Result, anyhow}; use gpui::{App, AppContext, AsyncApp, Entity, Task, WeakEntity}; use language::LanguageRegistry; use language_model::LanguageModelToolResultContent; -use paths; use project::{Project, ProjectPath}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use settings::Settings; -use std::ffi::OsStr; use std::ops::Range; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use std::sync::Arc; use text::BufferSnapshot; use ui::SharedString; @@ -169,63 +166,14 @@ impl StreamingEditFileTool { event_stream: &ToolCallEventStream, cx: &mut App, ) -> Task> { - let path_str = input.path.to_string_lossy(); - let settings = agent_settings::AgentSettings::get_global(cx); - let decision = decide_permission_from_settings(Self::NAME, &path_str, settings); - - match decision { - ToolPermissionDecision::Allow => return Task::ready(Ok(())), - ToolPermissionDecision::Deny(reason) => { - return Task::ready(Err(anyhow!("{}", reason))); - } - ToolPermissionDecision::Confirm => {} - } - - let local_settings_folder = paths::local_settings_folder_name(); - let path = Path::new(&input.path); - if path.components().any(|component| { - component.as_os_str() == <_ as AsRef>::as_ref(&local_settings_folder) - }) { - let context = crate::ToolPermissionContext { - tool_name: EditFileTool::NAME.to_string(), - input_value: path_str.to_string(), - }; - return event_stream.authorize( - format!("{} (local settings)", input.display_description), - context, - cx, - ); - } - - if let Ok(canonical_path) = std::fs::canonicalize(&input.path) - && canonical_path.starts_with(paths::config_dir()) - { - let context = crate::ToolPermissionContext { - tool_name: EditFileTool::NAME.to_string(), - input_value: path_str.to_string(), - }; - return event_stream.authorize( - format!("{} (global settings)", input.display_description), - context, - cx, - ); - } - - let Ok(project_path) = self.thread.read_with(cx, |thread, cx| { - thread.project().read(cx).find_project_path(&input.path, cx) - }) else { - return Task::ready(Err(anyhow!("thread was dropped"))); - }; - - if project_path.is_some() { - Task::ready(Ok(())) - } else { - let context = crate::ToolPermissionContext { - tool_name: EditFileTool::NAME.to_string(), - input_value: path_str.to_string(), - }; - event_stream.authorize(&input.display_description, context, cx) - } + super::edit_file_tool::authorize_file_edit( + EditFileTool::NAME, + &input.path, + &input.display_description, + &self.thread, + event_stream, + cx, + ) } }