Skip to content

Commit c371e72

Browse files
dingfelidingfeli
committed
simplifies overridden warning message printing logic
1 parent b39539c commit c371e72

File tree

10 files changed

+217
-188
lines changed

10 files changed

+217
-188
lines changed

‎crates/chat-cli/src/cli/agent/mod.rs‎

Lines changed: 12 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -54,10 +54,6 @@ pub use wrapper_types::{
5454
tool_settings_schema,
5555
};
5656

57-
use super::chat::tools::execute::ExecuteCommand;
58-
use super::chat::tools::fs_read::FsRead;
59-
use super::chat::tools::fs_write::FsWrite;
60-
use super::chat::tools::use_aws::UseAws;
6157
use super::chat::tools::{
6258
DEFAULT_APPROVE,
6359
NATIVE_TOOLS,
@@ -247,49 +243,22 @@ impl Agent {
247243
Ok(())
248244
}
249245

250-
pub fn validate_tool_settings(&self, output: &mut impl Write) -> Result<(), AgentConfigError> {
246+
pub fn print_overridden_permissions(&self, output: &mut impl Write) -> Result<(), AgentConfigError> {
251247
let execute_name = if cfg!(windows) { "execute_cmd" } else { "execute_bash" };
252248
for allowed_tool in &self.allowed_tools {
253249
if let Some(settings) = self.tools_settings.get(allowed_tool.as_str()) {
254250
// currently we only have four native tools that offers tool settings
255-
match allowed_tool.as_str() {
256-
"fs_read" => {
257-
if let Some(overridden_settings) = FsRead::allowable_field_to_be_overridden(settings) {
258-
queue_permission_override_warning(
259-
allowed_tool.as_str(),
260-
overridden_settings.as_str(),
261-
output,
262-
)?;
263-
}
264-
},
265-
"fs_write" => {
266-
if let Some(overridden_settings) = FsWrite::allowable_field_to_be_overridden(settings) {
267-
queue_permission_override_warning(
268-
allowed_tool.as_str(),
269-
overridden_settings.as_str(),
270-
output,
271-
)?;
272-
}
273-
},
274-
"use_aws" => {
275-
if let Some(overridden_settings) = UseAws::allowable_field_to_be_overridden(settings) {
276-
queue_permission_override_warning(
277-
allowed_tool.as_str(),
278-
overridden_settings.as_str(),
279-
output,
280-
)?;
281-
}
282-
},
283-
name if name == execute_name => {
284-
if let Some(overridden_settings) = ExecuteCommand::allowable_field_to_be_overridden(settings) {
285-
queue_permission_override_warning(
286-
allowed_tool.as_str(),
287-
overridden_settings.as_str(),
288-
output,
289-
)?;
290-
}
291-
},
292-
_ => {},
251+
let overridden_settings_key = match allowed_tool.as_str() {
252+
"fs_read" | "fs_write" => Some("allowedPaths"),
253+
"use_aws" => Some("allowedServices"),
254+
name if name == execute_name => Some("allowedCommands"),
255+
_ => None,
256+
};
257+
258+
if let Some(key) = overridden_settings_key {
259+
if let Some(ref override_settings) = settings.get(key).map(|value| format!("{key}: {value}")) {
260+
queue_permission_override_warning(allowed_tool.as_str(), override_settings, output)?;
261+
}
293262
}
294263
}
295264
}

‎crates/chat-cli/src/cli/chat/mod.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1177,7 +1177,7 @@ impl ChatSession {
11771177
}
11781178

11791179
if let Some(agent) = self.conversation.agents.get_active() {
1180-
agent.validate_tool_settings(&mut self.stderr)?;
1180+
agent.print_overridden_permissions(&mut self.stderr)?;
11811181
}
11821182

11831183
self.stderr.flush()?;
@@ -1750,7 +1750,7 @@ impl ChatSession {
17501750

17511751
if let Some(agent) = self.conversation.agents.get_active() {
17521752
agent
1753-
.validate_tool_settings(&mut self.stderr)
1753+
.print_overridden_permissions(&mut self.stderr)
17541754
.map_err(|_e| ChatError::Custom("Failed to validate agent tool settings".into()))?;
17551755
}
17561756
}
@@ -1820,7 +1820,7 @@ impl ChatSession {
18201820
self.conversation
18211821
.agents
18221822
.get_active()
1823-
.is_some_and(|a| match tool.tool.requires_acceptance(a) {
1823+
.is_some_and(|a| match tool.tool.requires_acceptance(os, a) {
18241824
PermissionEvalResult::Allow => true,
18251825
PermissionEvalResult::Ask => false,
18261826
PermissionEvalResult::Deny(matches) => {

‎crates/chat-cli/src/cli/chat/tools/custom_tool.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -287,7 +287,7 @@ impl CustomTool {
287287
+ TokenCounter::count_tokens(self.params.as_ref().map_or("", |p| p.as_str().unwrap_or_default()))
288288
}
289289

290-
pub fn eval_perm(&self, agent: &Agent) -> PermissionEvalResult {
290+
pub fn eval_perm(&self, _os: &Os, agent: &Agent) -> PermissionEvalResult {
291291
use crate::util::MCP_SERVER_TOOL_DELIMITER;
292292
let Self {
293293
name: tool_name,

‎crates/chat-cli/src/cli/chat/tools/execute/mod.rs‎

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -186,13 +186,7 @@ impl ExecuteCommand {
186186
Ok(())
187187
}
188188

189-
pub fn allowable_field_to_be_overridden(settings: &serde_json::Value) -> Option<String> {
190-
settings
191-
.get("allowedCommands")
192-
.map(|value| format!("allowedCommands: {}", value))
193-
}
194-
195-
pub fn eval_perm(&self, agent: &Agent) -> PermissionEvalResult {
189+
pub fn eval_perm(&self, _os: &Os, agent: &Agent) -> PermissionEvalResult {
196190
#[derive(Debug, Deserialize)]
197191
#[serde(rename_all = "camelCase")]
198192
struct Settings {
@@ -426,8 +420,8 @@ mod tests {
426420
}
427421
}
428422

429-
#[test]
430-
fn test_eval_perm() {
423+
#[tokio::test]
424+
async fn test_eval_perm() {
431425
let tool_name = if cfg!(windows) { "execute_cmd" } else { "execute_bash" };
432426
let mut agent = Agent {
433427
name: "test_agent".to_string(),
@@ -444,51 +438,52 @@ mod tests {
444438
},
445439
..Default::default()
446440
};
441+
let os = Os::new().await.unwrap();
447442

448443
let tool_one = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
449444
"command": "git status",
450445
}))
451446
.unwrap();
452447

453-
let res = tool_one.eval_perm(&agent);
448+
let res = tool_one.eval_perm(&os, &agent);
454449
assert!(matches!(res, PermissionEvalResult::Deny(ref rules) if rules.contains(&"\\Agit .*\\z".to_string())));
455450

456451
let tool_two = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
457452
"command": "this_is_not_a_read_only_command",
458453
}))
459454
.unwrap();
460455

461-
let res = tool_two.eval_perm(&agent);
456+
let res = tool_two.eval_perm(&os, &agent);
462457
assert!(matches!(res, PermissionEvalResult::Ask));
463458

464459
let tool_allow_wild_card = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
465460
"command": "allow_wild_card some_arg",
466461
}))
467462
.unwrap();
468-
let res = tool_allow_wild_card.eval_perm(&agent);
463+
let res = tool_allow_wild_card.eval_perm(&os, &agent);
469464
assert!(matches!(res, PermissionEvalResult::Allow));
470465

471466
let tool_allow_exact_should_ask = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
472467
"command": "allow_exact some_arg",
473468
}))
474469
.unwrap();
475-
let res = tool_allow_exact_should_ask.eval_perm(&agent);
470+
let res = tool_allow_exact_should_ask.eval_perm(&os, &agent);
476471
assert!(matches!(res, PermissionEvalResult::Ask));
477472

478473
let tool_allow_exact_should_allow = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
479474
"command": "allow_exact",
480475
}))
481476
.unwrap();
482-
let res = tool_allow_exact_should_allow.eval_perm(&agent);
477+
let res = tool_allow_exact_should_allow.eval_perm(&os, &agent);
483478
assert!(matches!(res, PermissionEvalResult::Allow));
484479

485480
agent.allowed_tools.insert(tool_name.to_string());
486481

487-
let res = tool_two.eval_perm(&agent);
482+
let res = tool_two.eval_perm(&os, &agent);
488483
assert!(matches!(res, PermissionEvalResult::Allow));
489484

490485
// Denied list should remain denied
491-
let res = tool_one.eval_perm(&agent);
486+
let res = tool_one.eval_perm(&os, &agent);
492487
assert!(matches!(res, PermissionEvalResult::Deny(ref rules) if rules.contains(&"\\Agit .*\\z".to_string())));
493488
}
494489

‎crates/chat-cli/src/cli/chat/tools/fs_read.rs‎

Lines changed: 34 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,7 @@ use eyre::{
1111
Result,
1212
bail,
1313
};
14-
use globset::{
15-
Glob,
16-
GlobSetBuilder,
17-
};
14+
use globset::GlobSetBuilder;
1815
use serde::{
1916
Deserialize,
2017
Serialize,
@@ -48,6 +45,7 @@ use crate::cli::chat::{
4845
sanitize_unicode_tags,
4946
};
5047
use crate::os::Os;
48+
use crate::util::directories;
5149

5250
#[derive(Debug, Clone, Deserialize)]
5351
pub struct FsRead {
@@ -102,13 +100,7 @@ impl FsRead {
102100
}
103101
}
104102

105-
pub fn allowable_field_to_be_overridden(settings: &serde_json::Value) -> Option<String> {
106-
settings
107-
.get("allowedPaths")
108-
.map(|value| format!("allowedPaths: {}", value))
109-
}
110-
111-
pub fn eval_perm(&self, agent: &Agent) -> PermissionEvalResult {
103+
pub fn eval_perm(&self, os: &Os, agent: &Agent) -> PermissionEvalResult {
112104
#[derive(Debug, Deserialize)]
113105
#[serde(rename_all = "camelCase")]
114106
struct Settings {
@@ -141,14 +133,11 @@ impl FsRead {
141133
let allow_set = {
142134
let mut builder = GlobSetBuilder::new();
143135
for path in &allowed_paths {
144-
let path = path.strip_suffix('/').unwrap_or(path.as_str());
145-
let Ok(path) = shellexpand::full(path) else {
136+
let Ok(path) = directories::canonicalizes_path(os, path) else {
146137
continue;
147138
};
148-
if let Ok(glob) = Glob::new(path.as_ref() as &str) {
149-
builder.add(glob);
150-
} else {
151-
warn!("Failed to create glob from path given: {path}. Ignoring.");
139+
if let Err(e) = directories::add_gitignore_globs(&mut builder, path.as_str()) {
140+
warn!("Failed to create glob from path given: {path}: {e}. Ignoring.");
152141
}
153142
}
154143
builder.build()
@@ -158,15 +147,17 @@ impl FsRead {
158147
let deny_set = {
159148
let mut builder = GlobSetBuilder::new();
160149
for path in &denied_paths {
161-
let processed_path = path.strip_suffix('/').unwrap_or(path.as_str());
162-
let Ok(processed_path) = shellexpand::full(processed_path) else {
150+
let Ok(processed_path) = directories::canonicalizes_path(os, path) else {
163151
continue;
164152
};
165-
if let Ok(glob) = Glob::new(processed_path.as_ref() as &str) {
166-
sanitized_deny_list.push(path);
167-
builder.add(glob);
168-
} else {
169-
warn!("Failed to create glob from path given: {path}. Ignoring.");
153+
match directories::add_gitignore_globs(&mut builder, processed_path.as_str()) {
154+
Ok(_) => {
155+
// Note that we need to push twice here because for each rule we
156+
// are creating two globs (one for file and one for directory)
157+
sanitized_deny_list.push(path);
158+
sanitized_deny_list.push(path);
159+
},
160+
Err(e) => warn!("Failed to create glob from path given: {path}: {e}. Ignoring."),
170161
}
171162
}
172163
builder.build()
@@ -182,8 +173,7 @@ impl FsRead {
182173
FsReadOperation::Line(FsLine { path, .. })
183174
| FsReadOperation::Directory(FsDirectory { path, .. })
184175
| FsReadOperation::Search(FsSearch { path, .. }) => {
185-
let path = path.strip_suffix('/').unwrap_or(path.as_str());
186-
let Ok(path) = shellexpand::full(path) else {
176+
let Ok(path) = directories::canonicalizes_path(os, path) else {
187177
ask = true;
188178
continue;
189179
};
@@ -213,8 +203,7 @@ impl FsRead {
213203
let denied_match_set = paths
214204
.iter()
215205
.flat_map(|path| {
216-
let path = path.strip_suffix('/').unwrap_or(path.as_str());
217-
let Ok(path) = shellexpand::full(path) else {
206+
let Ok(path) = directories::canonicalizes_path(os, path) else {
218207
return vec![];
219208
};
220209
deny_set.matches(path.as_ref() as &str)
@@ -1406,12 +1395,10 @@ mod tests {
14061395
);
14071396
}
14081397

1409-
#[test]
1410-
fn test_eval_perm() {
1411-
const DENIED_PATH_ONE: &str = "/some/denied/path";
1412-
const DENIED_PATH_GLOB: &str = "/denied/glob/**/path";
1413-
const ALLOW_PATH_ONE: &str = "/some/allow/path/**";
1414-
const ALLOW_PATH_GLOB: &str = "/allowed/glob/**/path/**";
1398+
#[tokio::test]
1399+
async fn test_eval_perm() {
1400+
const DENIED_PATH_OR_FILE: &str = "/some/denied/path";
1401+
const DENIED_PATH_OR_FILE_GLOB: &str = "/denied/glob/**/path";
14151402

14161403
let mut agent = Agent {
14171404
name: "test_agent".to_string(),
@@ -1420,44 +1407,43 @@ mod tests {
14201407
map.insert(
14211408
ToolSettingTarget("fs_read".to_string()),
14221409
serde_json::json!({
1423-
"allowedPaths": [ALLOW_PATH_ONE, ALLOW_PATH_GLOB],
1424-
"deniedPaths": [DENIED_PATH_ONE, DENIED_PATH_GLOB]
1410+
"deniedPaths": [DENIED_PATH_OR_FILE, DENIED_PATH_OR_FILE_GLOB]
14251411
}),
14261412
);
14271413
map
14281414
},
14291415
..Default::default()
14301416
};
14311417

1418+
let os = Os::new().await.unwrap();
1419+
14321420
let tool_one = serde_json::from_value::<FsRead>(serde_json::json!({
14331421
"operations": [
1434-
{ "path": DENIED_PATH_ONE, "mode": "Line", "start_line": 1, "end_line": 2 },
1435-
{ "path": format!("{DENIED_PATH_ONE}/"), "mode": "Line", "start_line": 1, "end_line": 2 },
1436-
{ "path": "/denied/glob", "mode": "Directory" },
1437-
{ "path": "/denied/glob/child_one/path", "mode": "Directory" },
1438-
{ "path": "/denied/glob/child_one/grand_child_one/path", "mode": "Directory" },
1439-
{ "path": TEST_FILE_PATH, "mode": "Search", "pattern": "hello" }
1422+
{ "path": DENIED_PATH_OR_FILE, "mode": "Line", "start_line": 1, "end_line": 2 },
1423+
{ "path": format!("{DENIED_PATH_OR_FILE}/child"), "mode": "Line", "start_line": 1, "end_line": 2 },
1424+
{ "path": "/denied/glob/middle_one/middle_two/path", "mode": "Line", "start_line": 1, "end_line": 2 },
1425+
{ "path": "/denied/glob/middle_one/middle_two/path/child", "mode": "Line", "start_line": 1, "end_line": 2 },
14401426
],
14411427
}))
14421428
.unwrap();
14431429

1444-
let res = tool_one.eval_perm(&agent);
1430+
let res = tool_one.eval_perm(&os, &agent);
14451431
assert!(matches!(
14461432
res,
14471433
PermissionEvalResult::Deny(ref deny_list)
1448-
if deny_list.iter().filter(|p| *p == DENIED_PATH_GLOB).collect::<Vec<_>>().len() == 2
1449-
&& deny_list.iter().filter(|p| *p == DENIED_PATH_ONE).collect::<Vec<_>>().len() == 2
1434+
if deny_list.iter().filter(|p| *p == DENIED_PATH_OR_FILE_GLOB).collect::<Vec<_>>().len() == 2
1435+
&& deny_list.iter().filter(|p| *p == DENIED_PATH_OR_FILE).collect::<Vec<_>>().len() == 2
14501436
));
14511437

14521438
agent.allowed_tools.insert("fs_read".to_string());
14531439

14541440
// Denied set should remain denied
1455-
let res = tool_one.eval_perm(&agent);
1441+
let res = tool_one.eval_perm(&os, &agent);
14561442
assert!(matches!(
14571443
res,
14581444
PermissionEvalResult::Deny(ref deny_list)
1459-
if deny_list.iter().filter(|p| *p == DENIED_PATH_GLOB).collect::<Vec<_>>().len() == 2
1460-
&& deny_list.iter().filter(|p| *p == DENIED_PATH_ONE).collect::<Vec<_>>().len() == 2
1445+
if deny_list.iter().filter(|p| *p == DENIED_PATH_OR_FILE_GLOB).collect::<Vec<_>>().len() == 2
1446+
&& deny_list.iter().filter(|p| *p == DENIED_PATH_OR_FILE).collect::<Vec<_>>().len() == 2
14611447
));
14621448
}
14631449
}

0 commit comments

Comments
 (0)