fix(agent): tool permission (#2619)

* adds warnings for when tool settings are overridden by allowed tools

* adjusts tool settings eval order

* modifies doc

* moves warning to be displayed after splash screen

* canonicalizes paths prior to making glob sets

* simplifies overridden warning message printing logic

* adds more doc on path globbing

Author: dingfeli

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

@@ -213,8 +213,8 @@ impl Agent {
 
         self.path = Some(path.to_path_buf());
 
+        let mut stderr = std::io::stderr();
         if let (true, Some(legacy_mcp_config)) = (self.use_legacy_mcp_json, legacy_mcp_config) {
-            let mut stderr = std::io::stderr();
             for (name, legacy_server) in &legacy_mcp_config.mcp_servers {
                 if mcp_servers.mcp_servers.contains_key(name) {
                     let _ = queue!(
@@ -238,6 +238,31 @@ impl Agent {
             }
         }
 
+        stderr.flush()?;
+
+        Ok(())
+    }
+
+    pub fn print_overridden_permissions(&self, output: &mut impl Write) -> Result<(), AgentConfigError> {
+        let execute_name = if cfg!(windows) { "execute_cmd" } else { "execute_bash" };
+        for allowed_tool in &self.allowed_tools {
+            if let Some(settings) = self.tools_settings.get(allowed_tool.as_str()) {
+                // currently we only have four native tools that offers tool settings
+                let overridden_settings_key = match allowed_tool.as_str() {
+                    "fs_read" | "fs_write" => Some("allowedPaths"),
+                    "use_aws" => Some("allowedServices"),
+                    name if name == execute_name => Some("allowedCommands"),
+                    _ => None,
+                };
+
+                if let Some(key) = overridden_settings_key {
+                    if let Some(ref override_settings) = settings.get(key).map(|value| format!("{key}: {value}")) {
+                        queue_permission_override_warning(allowed_tool.as_str(), override_settings, output)?;
+                    }
+                }
+            }
+        }
+
         Ok(())
     }
 
@@ -861,6 +886,28 @@ async fn load_legacy_mcp_config(os: &Os) -> eyre::Result<Option<McpServerConfig>
     })
 }
 
+pub fn queue_permission_override_warning(
+    tool_name: &str,
+    overridden_settings: &str,
+    output: &mut impl Write,
+) -> Result<(), std::io::Error> {
+    Ok(queue!(
+        output,
+        style::SetForegroundColor(Color::Yellow),
+        style::Print("WARNING: "),
+        style::ResetColor,
+        style::Print("You have trusted "),
+        style::SetForegroundColor(Color::Green),
+        style::Print(tool_name),
+        style::ResetColor,
+        style::Print(" tool, which overrides the toolsSettings: "),
+        style::SetForegroundColor(Color::Cyan),
+        style::Print(overridden_settings),
+        style::ResetColor,
+        style::Print("\n"),
+    )?)
+}
+
 fn default_schema() -> String {
     "https://raw.githubusercontent.com/aws/amazon-q-developer-cli/refs/heads/main/schemas/agent-v1.json".into()
 }
@@ -1088,8 +1135,10 @@ mod tests {
 
     #[test]
     fn test_display_label_trust_all_tools() {
-        let mut agents = Agents::default();
-        agents.trust_all_tools = true;
+        let agents = Agents {
+            trust_all_tools: true,
+            ..Default::default()
+        };
 
         // Should be trusted even if not in allowed_tools
         let label = agents.display_label("random_tool", &ToolOrigin::Native);
@@ -1119,7 +1168,8 @@ mod tests {
             fs_write_label
         );
 
-        let execute_bash_label = agents.display_label("execute_bash", &ToolOrigin::Native);
+        let execute_name = if cfg!(windows) { "execute_cmd" } else { "execute_bash" };
+        let execute_bash_label = agents.display_label(execute_name, &ToolOrigin::Native);
         assert!(
             execute_bash_label.contains("read-only"),
             "execute_bash should show read-only by default, instead found: {}",

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

@@ -125,7 +125,10 @@ use util::{
 use winnow::Partial;
 use winnow::stream::Offset;
 
-use super::agent::PermissionEvalResult;
+use super::agent::{
+    DEFAULT_AGENT_NAME,
+    PermissionEvalResult,
+};
 use crate::api_client::model::ToolResultStatus;
 use crate::api_client::{
     self,
@@ -634,7 +637,7 @@ impl ChatSession {
                                 ": cannot resume conversation with {profile} because it no longer exists. Using default.\n"
                             ))
                         )?;
-                        let _ = agents.switch("default");
+                        let _ = agents.switch(DEFAULT_AGENT_NAME);
                     }
                 }
                 cs.agents = agents;
@@ -1207,6 +1210,11 @@ impl ChatSession {
                 ))
             )?;
         }
+
+        if let Some(agent) = self.conversation.agents.get_active() {
+            agent.print_overridden_permissions(&mut self.stderr)?;
+        }
+
         self.stderr.flush()?;
 
         if let Some(ref model_info) = self.conversation.model_info {
@@ -1775,6 +1783,12 @@ impl ChatSession {
                             .clone()
                             .unwrap_or(tool_use.name.clone());
                         self.conversation.agents.trust_tools(vec![formatted_tool_name]);
+
+                        if let Some(agent) = self.conversation.agents.get_active() {
+                            agent
+                                .print_overridden_permissions(&mut self.stderr)
+                                .map_err(|_e| ChatError::Custom("Failed to validate agent tool settings".into()))?;
+                        }
                     }
                     tool_use.accepted = true;
 
@@ -1842,7 +1856,7 @@ impl ChatSession {
                 self.conversation
                     .agents
                     .get_active()
-                    .is_some_and(|a| match tool.tool.requires_acceptance(a) {
+                    .is_some_and(|a| match tool.tool.requires_acceptance(os, a) {
                         PermissionEvalResult::Allow => true,
                         PermissionEvalResult::Ask => false,
                         PermissionEvalResult::Deny(matches) => {

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

@@ -275,7 +275,7 @@ impl CustomTool {
             + TokenCounter::count_tokens(self.params.as_ref().map_or("", |p| p.as_str().unwrap_or_default()))
     }
 
-    pub fn eval_perm(&self, agent: &Agent) -> PermissionEvalResult {
+    pub fn eval_perm(&self, _os: &Os, agent: &Agent) -> PermissionEvalResult {
         let Self {
             name: tool_name,
             client,

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

@@ -187,7 +187,7 @@ impl ExecuteCommand {
         Ok(())
     }
 
-    pub fn eval_perm(&self, agent: &Agent) -> PermissionEvalResult {
+    pub fn eval_perm(&self, _os: &Os, agent: &Agent) -> PermissionEvalResult {
         #[derive(Debug, Deserialize)]
         #[serde(rename_all = "camelCase")]
         struct Settings {
@@ -207,7 +207,7 @@ impl ExecuteCommand {
         let tool_name = if cfg!(windows) { "execute_cmd" } else { "execute_bash" };
         let is_in_allowlist = matches_any_pattern(&agent.allowed_tools, tool_name);
         match agent.tools_settings.get(tool_name) {
-            Some(settings) if is_in_allowlist => {
+            Some(settings) => {
                 let Settings {
                     allowed_commands,
                     denied_commands,
@@ -231,7 +231,9 @@ impl ExecuteCommand {
                     return PermissionEvalResult::Deny(denied_match_set);
                 }
 
-                if self.requires_acceptance(Some(&allowed_commands), allow_read_only) {
+                if is_in_allowlist {
+                    PermissionEvalResult::Allow
+                } else if self.requires_acceptance(Some(&allowed_commands), allow_read_only) {
                     PermissionEvalResult::Ask
                 } else {
                     PermissionEvalResult::Allow
@@ -268,10 +270,7 @@ pub fn format_output(output: &str, max_size: usize) -> String {
 
 #[cfg(test)]
 mod tests {
-    use std::collections::{
-        HashMap,
-        HashSet,
-    };
+    use std::collections::HashMap;
 
     use super::*;
     use crate::cli::agent::ToolSettingTarget;
@@ -422,44 +421,71 @@ mod tests {
         }
     }
 
-    #[test]
-    fn test_eval_perm() {
+    #[tokio::test]
+    async fn test_eval_perm() {
         let tool_name = if cfg!(windows) { "execute_cmd" } else { "execute_bash" };
-        let agent = Agent {
+        let mut agent = Agent {
             name: "test_agent".to_string(),
-            allowed_tools: {
-                let mut allowed_tools = HashSet::<String>::new();
-                allowed_tools.insert(tool_name.to_string());
-                allowed_tools
-            },
             tools_settings: {
                 let mut map = HashMap::<ToolSettingTarget, serde_json::Value>::new();
                 map.insert(
                     ToolSettingTarget(tool_name.to_string()),
                     serde_json::json!({
+                        "allowedCommands": ["allow_wild_card .*", "allow_exact"],
                         "deniedCommands": ["git .*"]
                     }),
                 );
                 map
             },
             ..Default::default()
         };
+        let os = Os::new().await.unwrap();
 
-        let tool = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
+        let tool_one = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
             "command": "git status",
         }))
         .unwrap();
 
-        let res = tool.eval_perm(&agent);
+        let res = tool_one.eval_perm(&os, &agent);
         assert!(matches!(res, PermissionEvalResult::Deny(ref rules) if rules.contains(&"\\Agit .*\\z".to_string())));
 
-        let tool = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
-            "command": "echo hello",
+        let tool_two = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
+            "command": "this_is_not_a_read_only_command",
+        }))
+        .unwrap();
+
+        let res = tool_two.eval_perm(&os, &agent);
+        assert!(matches!(res, PermissionEvalResult::Ask));
+
+        let tool_allow_wild_card = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
+            "command": "allow_wild_card some_arg",
+        }))
+        .unwrap();
+        let res = tool_allow_wild_card.eval_perm(&os, &agent);
+        assert!(matches!(res, PermissionEvalResult::Allow));
+
+        let tool_allow_exact_should_ask = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
+            "command": "allow_exact some_arg",
+        }))
+        .unwrap();
+        let res = tool_allow_exact_should_ask.eval_perm(&os, &agent);
+        assert!(matches!(res, PermissionEvalResult::Ask));
+
+        let tool_allow_exact_should_allow = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
+            "command": "allow_exact",
         }))
         .unwrap();
+        let res = tool_allow_exact_should_allow.eval_perm(&os, &agent);
+        assert!(matches!(res, PermissionEvalResult::Allow));
+
+        agent.allowed_tools.insert(tool_name.to_string());
 
-        let res = tool.eval_perm(&agent);
+        let res = tool_two.eval_perm(&os, &agent);
         assert!(matches!(res, PermissionEvalResult::Allow));
+
+        // Denied list should remain denied
+        let res = tool_one.eval_perm(&os, &agent);
+        assert!(matches!(res, PermissionEvalResult::Deny(ref rules) if rules.contains(&"\\Agit .*\\z".to_string())));
     }
 
     #[tokio::test]