diff --git a/crates/chat-cli/src/cli/agent/mod.rs b/crates/chat-cli/src/cli/agent/mod.rs index bf69dcba03..04e634f0c6 100644 --- a/crates/chat-cli/src/cli/agent/mod.rs +++ b/crates/chat-cli/src/cli/agent/mod.rs @@ -810,7 +810,7 @@ impl Agents { // This "static" way avoids needing to construct a tool instance. fn default_permission_label(&self, tool_name: &str) -> String { let label = match tool_name { - "fs_read" => "trusted".dark_green().bold(), + "fs_read" => "trust working directory".dark_grey(), "fs_write" => "not trusted".dark_grey(), #[cfg(not(windows))] "execute_bash" => "trust read-only commands".dark_grey(), @@ -1137,9 +1137,9 @@ mod tests { let label = agents.display_label("fs_read", &ToolOrigin::Native); // With no active agent, it should fall back to default permissions - // fs_read has a default of "trusted" + // fs_read has a default of "trust working directory" assert!( - label.contains("trusted"), + label.contains("trust working directory"), "fs_read should show default trusted permission, instead found: {}", label ); @@ -1168,7 +1168,7 @@ mod tests { // Test default permissions for known tools let fs_read_label = agents.display_label("fs_read", &ToolOrigin::Native); assert!( - fs_read_label.contains("trusted"), + fs_read_label.contains("trust working directory"), "fs_read should be trusted by default, instead found: {}", fs_read_label ); diff --git a/crates/chat-cli/src/cli/chat/tools/fs_read.rs b/crates/chat-cli/src/cli/chat/tools/fs_read.rs index a11924e9a2..c4e60ae6cc 100644 --- a/crates/chat-cli/src/cli/chat/tools/fs_read.rs +++ b/crates/chat-cli/src/cli/chat/tools/fs_read.rs @@ -109,28 +109,32 @@ impl FsRead { allowed_paths: Vec, #[serde(default)] denied_paths: Vec, - #[serde(default = "default_allow_read_only")] + #[serde(default)] allow_read_only: bool, } - fn default_allow_read_only() -> bool { - true - } - let is_in_allowlist = matches_any_pattern(&agent.allowed_tools, "fs_read"); - match agent.tools_settings.get("fs_read") { - Some(settings) => { + let settings = agent.tools_settings.get("fs_read").cloned() + .unwrap_or_else(|| serde_json::json!({})); + + { let Settings { - allowed_paths, + mut allowed_paths, denied_paths, allow_read_only, - } = match serde_json::from_value::(settings.clone()) { + } = match serde_json::from_value::(settings) { Ok(settings) => settings, Err(e) => { error!("Failed to deserialize tool settings for fs_read: {:?}", e); return PermissionEvalResult::Ask; }, }; + + // Always add current working directory to allowed paths + if let Ok(cwd) = os.env.current_dir() { + allowed_paths.push(cwd.to_string_lossy().to_string()); + } + let allow_set = { let mut builder = GlobSetBuilder::new(); for path in &allowed_paths { @@ -259,10 +263,7 @@ impl FsRead { PermissionEvalResult::Ask }, } - }, - None if is_in_allowlist => PermissionEvalResult::Allow, - _ => PermissionEvalResult::Ask, - } + } } pub async fn invoke(&self, os: &Os, updates: &mut impl Write) -> Result { @@ -862,6 +863,7 @@ fn format_mode(mode: u32) -> [char; 9] { #[cfg(test)] mod tests { use std::collections::HashMap; + use std::path::PathBuf; use super::*; use crate::cli::agent::ToolSettingTarget; @@ -1397,7 +1399,7 @@ mod tests { } #[tokio::test] - async fn test_eval_perm() { + async fn test_eval_perm_denied_path() { const DENIED_PATH_OR_FILE: &str = "/some/denied/path"; const DENIED_PATH_OR_FILE_GLOB: &str = "/denied/glob/**/path"; @@ -1447,4 +1449,88 @@ mod tests { && deny_list.iter().filter(|p| *p == DENIED_PATH_OR_FILE).collect::>().len() == 2 )); } + + #[tokio::test] + async fn test_eval_perm_allowed_path_and_cwd() { + + // by default the fake env uses "/" as the CWD. + // change it to a sub folder so we can test fs_read reading files outside CWD + let os = Os::new().await.unwrap(); + os.env.set_current_dir_for_test(PathBuf::from("/home/user")); + + let agent = Agent { + name: "test_agent".to_string(), + tools_settings: { + let mut map = HashMap::new(); + map.insert( + ToolSettingTarget("fs_read".to_string()), + serde_json::json!({ + "allowedPaths": ["/explicitly/allowed/path"] + }), + ); + map + }, + ..Default::default() // Not in allowed_tools, allow_read_only = false + }; + + // Test 1: Explicitly allowed path should work + let allowed_tool = serde_json::from_value::(serde_json::json!({ + "operations": [ + { "path": "/explicitly/allowed/path", "mode": "Directory" }, + { "path": "/explicitly/allowed/path/file.txt", "mode": "Line" }, + ] + })).unwrap(); + let res = allowed_tool.eval_perm(&os, &agent); + assert!(matches!(res, PermissionEvalResult::Allow)); + + // Test 2: CWD should always be allowed + let cwd_tool = serde_json::from_value::(serde_json::json!({ + "operations": [ + { "path": "/home/user/", "mode": "Directory" }, + { "path": "/home/user/file.txt", "mode": "Line" }, + ] + })).unwrap(); + let res = cwd_tool.eval_perm(&os, &agent); + assert!(matches!(res, PermissionEvalResult::Allow)); + + // Test 3: Outside CWD and not explicitly allowed should ask + let outside_tool = serde_json::from_value::(serde_json::json!({ + "operations": [ + { "path": "/tmp/not/allowed/file.txt", "mode": "Line" } + ] + })).unwrap(); + let res = outside_tool.eval_perm(&os, &agent); + assert!(matches!(res, PermissionEvalResult::Ask)); + } + + #[tokio::test] + async fn test_eval_perm_no_settings_cwd_behavior() { + let os = Os::new().await.unwrap(); + os.env.set_current_dir_for_test(PathBuf::from("/home/user")); + + let agent = Agent { + name: "test_agent".to_string(), + tools_settings: HashMap::new(), // No fs_read settings + ..Default::default() + }; + + // Test 1: CWD should be allowed even with no settings + let cwd_tool = serde_json::from_value::(serde_json::json!({ + "operations": [ + { "path": "/home/user/", "mode": "Directory" }, + { "path": "/home/user/file.txt", "mode": "Line" }, + ] + })).unwrap(); + let res = cwd_tool.eval_perm(&os, &agent); + assert!(matches!(res, PermissionEvalResult::Allow)); + + // Test 2: Outside CWD should ask for permission + let outside_tool = serde_json::from_value::(serde_json::json!({ + "operations": [ + { "path": "/tmp/not/allowed/file.txt", "mode": "Line" } + ] + })).unwrap(); + let res = outside_tool.eval_perm(&os, &agent); + assert!(matches!(res, PermissionEvalResult::Ask)); + } } diff --git a/crates/chat-cli/src/cli/chat/tools/mod.rs b/crates/chat-cli/src/cli/chat/tools/mod.rs index 6b8baec18f..43ccb006cf 100644 --- a/crates/chat-cli/src/cli/chat/tools/mod.rs +++ b/crates/chat-cli/src/cli/chat/tools/mod.rs @@ -57,7 +57,7 @@ use crate::cli::agent::{ use crate::cli::chat::line_tracker::FileLineTracker; use crate::os::Os; -pub const DEFAULT_APPROVE: [&str; 1] = ["fs_read"]; +pub const DEFAULT_APPROVE: [&str; 0] = []; pub const NATIVE_TOOLS: [&str; 8] = [ "fs_read", "fs_write", diff --git a/crates/chat-cli/src/os/env.rs b/crates/chat-cli/src/os/env.rs index 40aac3b5fe..63e5449419 100644 --- a/crates/chat-cli/src/os/env.rs +++ b/crates/chat-cli/src/os/env.rs @@ -132,6 +132,13 @@ impl Env { } } + pub fn set_current_dir_for_test(&self, path: PathBuf) { + use inner::Inner; + if let Inner::Fake(fake) = &self.0 { + fake.lock().unwrap().cwd = path; + } + } + pub fn current_exe(&self) -> Result { use inner::Inner; match &self.0 {