Skip to content

Commit 9316c6b

Browse files
dingfelidingfeli
authored
Revert "fix(agent): tool permission override (#2606)" (#2618)
This reverts commit dfaa580.
1 parent 1ef0e7e commit 9316c6b

File tree

7 files changed

+68
-198
lines changed

7 files changed

+68
-198
lines changed

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

Lines changed: 1 addition & 79 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,
@@ -217,8 +213,8 @@ impl Agent {
217213

218214
self.path = Some(path.to_path_buf());
219215

220-
let mut stderr = std::io::stderr();
221216
if let (true, Some(legacy_mcp_config)) = (self.use_legacy_mcp_json, legacy_mcp_config) {
217+
let mut stderr = std::io::stderr();
222218
for (name, legacy_server) in &legacy_mcp_config.mcp_servers {
223219
if mcp_servers.mcp_servers.contains_key(name) {
224220
let _ = queue!(
@@ -242,58 +238,6 @@ impl Agent {
242238
}
243239
}
244240

245-
stderr.flush()?;
246-
247-
Ok(())
248-
}
249-
250-
pub fn validate_tool_settings(&self, output: &mut impl Write) -> Result<(), AgentConfigError> {
251-
let execute_name = if cfg!(windows) { "execute_cmd" } else { "execute_bash" };
252-
for allowed_tool in &self.allowed_tools {
253-
if let Some(settings) = self.tools_settings.get(allowed_tool.as_str()) {
254-
// 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-
_ => {},
293-
}
294-
}
295-
}
296-
297241
Ok(())
298242
}
299243

@@ -859,28 +803,6 @@ async fn load_legacy_mcp_config(os: &Os) -> eyre::Result<Option<McpServerConfig>
859803
})
860804
}
861805

862-
pub fn queue_permission_override_warning(
863-
tool_name: &str,
864-
overridden_settings: &str,
865-
output: &mut impl Write,
866-
) -> Result<(), std::io::Error> {
867-
Ok(queue!(
868-
output,
869-
style::SetForegroundColor(Color::Yellow),
870-
style::Print("WARN: "),
871-
style::ResetColor,
872-
style::Print("You have trusted "),
873-
style::SetForegroundColor(Color::Green),
874-
style::Print(tool_name),
875-
style::ResetColor,
876-
style::Print(" tool, which overrides the toolsSettings: "),
877-
style::SetForegroundColor(Color::Cyan),
878-
style::Print(overridden_settings),
879-
style::ResetColor,
880-
style::Print("\n"),
881-
)?)
882-
}
883-
884806
fn default_schema() -> String {
885807
"https://raw.githubusercontent.com/aws/amazon-q-developer-cli/refs/heads/main/schemas/agent-v1.json".into()
886808
}

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

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -123,10 +123,7 @@ use util::{
123123
use winnow::Partial;
124124
use winnow::stream::Offset;
125125

126-
use super::agent::{
127-
DEFAULT_AGENT_NAME,
128-
PermissionEvalResult,
129-
};
126+
use super::agent::PermissionEvalResult;
130127
use crate::api_client::model::ToolResultStatus;
131128
use crate::api_client::{
132129
self,
@@ -614,7 +611,7 @@ impl ChatSession {
614611
": cannot resume conversation with {profile} because it no longer exists. Using default.\n"
615612
))
616613
)?;
617-
let _ = agents.switch(DEFAULT_AGENT_NAME);
614+
let _ = agents.switch("default");
618615
}
619616
}
620617
cs.agents = agents;
@@ -625,10 +622,6 @@ impl ChatSession {
625622
false => ConversationState::new(conversation_id, agents, tool_config, tool_manager, model_id, os).await,
626623
};
627624

628-
if let Some(agent) = conversation.agents.get_active() {
629-
agent.validate_tool_settings(&mut stderr)?;
630-
}
631-
632625
// Spawn a task for listening and broadcasting sigints.
633626
let (ctrlc_tx, ctrlc_rx) = tokio::sync::broadcast::channel(4);
634627
tokio::spawn(async move {
@@ -1746,12 +1739,6 @@ impl ChatSession {
17461739
.clone()
17471740
.unwrap_or(tool_use.name.clone());
17481741
self.conversation.agents.trust_tools(vec![formatted_tool_name]);
1749-
1750-
if let Some(agent) = self.conversation.agents.get_active() {
1751-
agent
1752-
.validate_tool_settings(&mut self.stderr)
1753-
.map_err(|_e| ChatError::Custom("Failed to validate agent tool settings".into()))?;
1754-
}
17551742
}
17561743
tool_use.accepted = true;
17571744

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

Lines changed: 16 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -186,12 +186,6 @@ 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-
195189
pub fn eval_perm(&self, agent: &Agent) -> PermissionEvalResult {
196190
#[derive(Debug, Deserialize)]
197191
#[serde(rename_all = "camelCase")]
@@ -212,7 +206,7 @@ impl ExecuteCommand {
212206
let tool_name = if cfg!(windows) { "execute_cmd" } else { "execute_bash" };
213207
let is_in_allowlist = agent.allowed_tools.contains(tool_name);
214208
match agent.tools_settings.get(tool_name) {
215-
Some(settings) => {
209+
Some(settings) if is_in_allowlist => {
216210
let Settings {
217211
allowed_commands,
218212
denied_commands,
@@ -236,7 +230,7 @@ impl ExecuteCommand {
236230
return PermissionEvalResult::Deny(denied_match_set);
237231
}
238232

239-
if !is_in_allowlist || self.requires_acceptance(Some(&allowed_commands), allow_read_only) {
233+
if self.requires_acceptance(Some(&allowed_commands), allow_read_only) {
240234
PermissionEvalResult::Ask
241235
} else {
242236
PermissionEvalResult::Allow
@@ -273,7 +267,10 @@ pub fn format_output(output: &str, max_size: usize) -> String {
273267

274268
#[cfg(test)]
275269
mod tests {
276-
use std::collections::HashMap;
270+
use std::collections::{
271+
HashMap,
272+
HashSet,
273+
};
277274

278275
use super::*;
279276
use crate::cli::agent::ToolSettingTarget;
@@ -427,8 +424,13 @@ mod tests {
427424
#[test]
428425
fn test_eval_perm() {
429426
let tool_name = if cfg!(windows) { "execute_cmd" } else { "execute_bash" };
430-
let mut agent = Agent {
427+
let agent = Agent {
431428
name: "test_agent".to_string(),
429+
allowed_tools: {
430+
let mut allowed_tools = HashSet::<String>::new();
431+
allowed_tools.insert(tool_name.to_string());
432+
allowed_tools
433+
},
432434
tools_settings: {
433435
let mut map = HashMap::<ToolSettingTarget, serde_json::Value>::new();
434436
map.insert(
@@ -442,32 +444,20 @@ mod tests {
442444
..Default::default()
443445
};
444446

445-
let tool_one = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
447+
let tool = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
446448
"command": "git status",
447449
}))
448450
.unwrap();
449451

450-
let res = tool_one.eval_perm(&agent);
452+
let res = tool.eval_perm(&agent);
451453
assert!(matches!(res, PermissionEvalResult::Deny(ref rules) if rules.contains(&"\\Agit .*\\z".to_string())));
452454

453-
let tool_two = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
455+
let tool = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
454456
"command": "echo hello",
455457
}))
456458
.unwrap();
457459

458-
let res = tool_two.eval_perm(&agent);
459-
assert!(matches!(res, PermissionEvalResult::Ask));
460-
461-
agent.allowed_tools.insert(tool_name.to_string());
462-
463-
let res = tool_two.eval_perm(&agent);
464-
assert!(matches!(res, PermissionEvalResult::Allow));
465-
466-
// Denied list should remain denied
467-
let res = tool_one.eval_perm(&agent);
468-
assert!(matches!(res, PermissionEvalResult::Deny(ref rules) if rules.contains(&"\\Agit .*\\z".to_string())));
469-
470-
let res = tool_two.eval_perm(&agent);
460+
let res = tool.eval_perm(&agent);
471461
assert!(matches!(res, PermissionEvalResult::Allow));
472462
}
473463

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

Lines changed: 15 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -102,12 +102,6 @@ impl FsRead {
102102
}
103103
}
104104

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-
111105
pub fn eval_perm(&self, agent: &Agent) -> PermissionEvalResult {
112106
#[derive(Debug, Deserialize)]
113107
#[serde(rename_all = "camelCase")]
@@ -126,7 +120,7 @@ impl FsRead {
126120

127121
let is_in_allowlist = agent.allowed_tools.contains("fs_read");
128122
match agent.tools_settings.get("fs_read") {
129-
Some(settings) => {
123+
Some(settings) if is_in_allowlist => {
130124
let Settings {
131125
allowed_paths,
132126
denied_paths,
@@ -188,7 +182,7 @@ impl FsRead {
188182

189183
// We only want to ask if we are not allowing read only
190184
// operation
191-
if !is_in_allowlist && !allow_read_only && !allow_set.is_match(path) {
185+
if !allow_read_only && !allow_set.is_match(path) {
192186
ask = true;
193187
}
194188
},
@@ -209,10 +203,7 @@ impl FsRead {
209203

210204
// We only want to ask if we are not allowing read only
211205
// operation
212-
if !is_in_allowlist
213-
&& !allow_read_only
214-
&& !paths.iter().any(|path| allow_set.is_match(path))
215-
{
206+
if !allow_read_only && !paths.iter().any(|path| allow_set.is_match(path)) {
216207
ask = true;
217208
}
218209
},
@@ -847,7 +838,10 @@ fn format_mode(mode: u32) -> [char; 9] {
847838

848839
#[cfg(test)]
849840
mod tests {
850-
use std::collections::HashMap;
841+
use std::collections::{
842+
HashMap,
843+
HashSet,
844+
};
851845

852846
use super::*;
853847
use crate::cli::agent::ToolSettingTarget;
@@ -1387,8 +1381,13 @@ mod tests {
13871381
const DENIED_PATH_ONE: &str = "/some/denied/path";
13881382
const DENIED_PATH_GLOB: &str = "/denied/glob/**/path";
13891383

1390-
let mut agent = Agent {
1384+
let agent = Agent {
13911385
name: "test_agent".to_string(),
1386+
allowed_tools: {
1387+
let mut allowed_tools = HashSet::<String>::new();
1388+
allowed_tools.insert("fs_read".to_string());
1389+
allowed_tools
1390+
},
13921391
tools_settings: {
13931392
let mut map = HashMap::<ToolSettingTarget, serde_json::Value>::new();
13941393
map.insert(
@@ -1402,7 +1401,7 @@ mod tests {
14021401
..Default::default()
14031402
};
14041403

1405-
let tool_one = serde_json::from_value::<FsRead>(serde_json::json!({
1404+
let tool = serde_json::from_value::<FsRead>(serde_json::json!({
14061405
"operations": [
14071406
{ "path": DENIED_PATH_ONE, "mode": "Line", "start_line": 1, "end_line": 2 },
14081407
{ "path": "/denied/glob", "mode": "Directory" },
@@ -1413,17 +1412,7 @@ mod tests {
14131412
}))
14141413
.unwrap();
14151414

1416-
let res = tool_one.eval_perm(&agent);
1417-
assert!(matches!(
1418-
res,
1419-
PermissionEvalResult::Deny(ref deny_list)
1420-
if deny_list.iter().filter(|p| *p == DENIED_PATH_GLOB).collect::<Vec<_>>().len() == 2
1421-
&& deny_list.iter().filter(|p| *p == DENIED_PATH_ONE).collect::<Vec<_>>().len() == 1
1422-
));
1423-
1424-
agent.allowed_tools.insert("fs_read".to_string());
1425-
1426-
let res = tool_one.eval_perm(&agent);
1415+
let res = tool.eval_perm(&agent);
14271416
assert!(matches!(
14281417
res,
14291418
PermissionEvalResult::Deny(ref deny_list)

0 commit comments

Comments
 (0)