Skip to content

Commit 504be11

Browse files
dingfelidingfeli
committed
merges main
2 parents 0ce09b4 + 95a5097 commit 504be11

File tree

12 files changed

+425
-143
lines changed

12 files changed

+425
-143
lines changed

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

Lines changed: 54 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -213,8 +213,8 @@ impl Agent {
213213

214214
self.path = Some(path.to_path_buf());
215215

216+
let mut stderr = std::io::stderr();
216217
if let (true, Some(legacy_mcp_config)) = (self.use_legacy_mcp_json, legacy_mcp_config) {
217-
let mut stderr = std::io::stderr();
218218
for (name, legacy_server) in &legacy_mcp_config.mcp_servers {
219219
if mcp_servers.mcp_servers.contains_key(name) {
220220
let _ = queue!(
@@ -238,6 +238,31 @@ impl Agent {
238238
}
239239
}
240240

241+
stderr.flush()?;
242+
243+
Ok(())
244+
}
245+
246+
pub fn print_overridden_permissions(&self, output: &mut impl Write) -> Result<(), AgentConfigError> {
247+
let execute_name = if cfg!(windows) { "execute_cmd" } else { "execute_bash" };
248+
for allowed_tool in &self.allowed_tools {
249+
if let Some(settings) = self.tools_settings.get(allowed_tool.as_str()) {
250+
// currently we only have four native tools that offers tool settings
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+
}
262+
}
263+
}
264+
}
265+
241266
Ok(())
242267
}
243268

@@ -816,6 +841,28 @@ async fn load_legacy_mcp_config(os: &Os) -> eyre::Result<Option<McpServerConfig>
816841
})
817842
}
818843

844+
pub fn queue_permission_override_warning(
845+
tool_name: &str,
846+
overridden_settings: &str,
847+
output: &mut impl Write,
848+
) -> Result<(), std::io::Error> {
849+
Ok(queue!(
850+
output,
851+
style::SetForegroundColor(Color::Yellow),
852+
style::Print("WARNING: "),
853+
style::ResetColor,
854+
style::Print("You have trusted "),
855+
style::SetForegroundColor(Color::Green),
856+
style::Print(tool_name),
857+
style::ResetColor,
858+
style::Print(" tool, which overrides the toolsSettings: "),
859+
style::SetForegroundColor(Color::Cyan),
860+
style::Print(overridden_settings),
861+
style::ResetColor,
862+
style::Print("\n"),
863+
)?)
864+
}
865+
819866
fn default_schema() -> String {
820867
"https://raw.githubusercontent.com/aws/amazon-q-developer-cli/refs/heads/main/schemas/agent-v1.json".into()
821868
}
@@ -972,8 +1019,10 @@ mod tests {
9721019

9731020
#[test]
9741021
fn test_display_label_trust_all_tools() {
975-
let mut agents = Agents::default();
976-
agents.trust_all_tools = true;
1022+
let agents = Agents {
1023+
trust_all_tools: true,
1024+
..Default::default()
1025+
};
9771026

9781027
// Should be trusted even if not in allowed_tools
9791028
let label = agents.display_label("random_tool", &ToolOrigin::Native);
@@ -1003,7 +1052,8 @@ mod tests {
10031052
fs_write_label
10041053
);
10051054

1006-
let execute_bash_label = agents.display_label("execute_bash", &ToolOrigin::Native);
1055+
let execute_name = if cfg!(windows) { "execute_cmd" } else { "execute_bash" };
1056+
let execute_bash_label = agents.display_label(execute_name, &ToolOrigin::Native);
10071057
assert!(
10081058
execute_bash_label.contains("read-only"),
10091059
"execute_bash should show read-only by default, instead found: {}",

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

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,10 @@ use util::{
125125
use winnow::Partial;
126126
use winnow::stream::Offset;
127127

128-
use super::agent::PermissionEvalResult;
128+
use super::agent::{
129+
DEFAULT_AGENT_NAME,
130+
PermissionEvalResult,
131+
};
129132
use crate::api_client::model::ToolResultStatus;
130133
use crate::api_client::{
131134
self,
@@ -620,7 +623,7 @@ impl ChatSession {
620623
": cannot resume conversation with {profile} because it no longer exists. Using default.\n"
621624
))
622625
)?;
623-
let _ = agents.switch("default");
626+
let _ = agents.switch(DEFAULT_AGENT_NAME);
624627
}
625628
}
626629
cs.agents = agents;
@@ -1181,6 +1184,11 @@ impl ChatSession {
11811184
))
11821185
)?;
11831186
}
1187+
1188+
if let Some(agent) = self.conversation.agents.get_active() {
1189+
agent.print_overridden_permissions(&mut self.stderr)?;
1190+
}
1191+
11841192
self.stderr.flush()?;
11851193

11861194
if let Some(ref model_info) = self.conversation.model_info {
@@ -1749,6 +1757,12 @@ impl ChatSession {
17491757
.clone()
17501758
.unwrap_or(tool_use.name.clone());
17511759
self.conversation.agents.trust_tools(vec![formatted_tool_name]);
1760+
1761+
if let Some(agent) = self.conversation.agents.get_active() {
1762+
agent
1763+
.print_overridden_permissions(&mut self.stderr)
1764+
.map_err(|_e| ChatError::Custom("Failed to validate agent tool settings".into()))?;
1765+
}
17521766
}
17531767
tool_use.accepted = true;
17541768

@@ -1816,7 +1830,7 @@ impl ChatSession {
18161830
self.conversation
18171831
.agents
18181832
.get_active()
1819-
.is_some_and(|a| match tool.tool.requires_acceptance(a) {
1833+
.is_some_and(|a| match tool.tool.requires_acceptance(os, a) {
18201834
PermissionEvalResult::Allow => true,
18211835
PermissionEvalResult::Ask => false,
18221836
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
@@ -275,7 +275,7 @@ impl CustomTool {
275275
+ TokenCounter::count_tokens(self.params.as_ref().map_or("", |p| p.as_str().unwrap_or_default()))
276276
}
277277

278-
pub fn eval_perm(&self, agent: &Agent) -> PermissionEvalResult {
278+
pub fn eval_perm(&self, _os: &Os, agent: &Agent) -> PermissionEvalResult {
279279
let Self {
280280
name: tool_name,
281281
client,

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

Lines changed: 46 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ impl ExecuteCommand {
187187
Ok(())
188188
}
189189

190-
pub fn eval_perm(&self, agent: &Agent) -> PermissionEvalResult {
190+
pub fn eval_perm(&self, _os: &Os, agent: &Agent) -> PermissionEvalResult {
191191
#[derive(Debug, Deserialize)]
192192
#[serde(rename_all = "camelCase")]
193193
struct Settings {
@@ -207,7 +207,7 @@ impl ExecuteCommand {
207207
let tool_name = if cfg!(windows) { "execute_cmd" } else { "execute_bash" };
208208
let is_in_allowlist = matches_any_pattern(&agent.allowed_tools, tool_name);
209209
match agent.tools_settings.get(tool_name) {
210-
Some(settings) if is_in_allowlist => {
210+
Some(settings) => {
211211
let Settings {
212212
allowed_commands,
213213
denied_commands,
@@ -231,7 +231,9 @@ impl ExecuteCommand {
231231
return PermissionEvalResult::Deny(denied_match_set);
232232
}
233233

234-
if self.requires_acceptance(Some(&allowed_commands), allow_read_only) {
234+
if is_in_allowlist {
235+
PermissionEvalResult::Allow
236+
} else if self.requires_acceptance(Some(&allowed_commands), allow_read_only) {
235237
PermissionEvalResult::Ask
236238
} else {
237239
PermissionEvalResult::Allow
@@ -268,10 +270,7 @@ pub fn format_output(output: &str, max_size: usize) -> String {
268270

269271
#[cfg(test)]
270272
mod tests {
271-
use std::collections::{
272-
HashMap,
273-
HashSet,
274-
};
273+
use std::collections::HashMap;
275274

276275
use super::*;
277276
use crate::cli::agent::ToolSettingTarget;
@@ -422,44 +421,71 @@ mod tests {
422421
}
423422
}
424423

425-
#[test]
426-
fn test_eval_perm() {
424+
#[tokio::test]
425+
async fn test_eval_perm() {
427426
let tool_name = if cfg!(windows) { "execute_cmd" } else { "execute_bash" };
428-
let agent = Agent {
427+
let mut agent = Agent {
429428
name: "test_agent".to_string(),
430-
allowed_tools: {
431-
let mut allowed_tools = HashSet::<String>::new();
432-
allowed_tools.insert(tool_name.to_string());
433-
allowed_tools
434-
},
435429
tools_settings: {
436430
let mut map = HashMap::<ToolSettingTarget, serde_json::Value>::new();
437431
map.insert(
438432
ToolSettingTarget(tool_name.to_string()),
439433
serde_json::json!({
434+
"allowedCommands": ["allow_wild_card .*", "allow_exact"],
440435
"deniedCommands": ["git .*"]
441436
}),
442437
);
443438
map
444439
},
445440
..Default::default()
446441
};
442+
let os = Os::new().await.unwrap();
447443

448-
let tool = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
444+
let tool_one = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
449445
"command": "git status",
450446
}))
451447
.unwrap();
452448

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

456-
let tool = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
457-
"command": "echo hello",
452+
let tool_two = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
453+
"command": "this_is_not_a_read_only_command",
454+
}))
455+
.unwrap();
456+
457+
let res = tool_two.eval_perm(&os, &agent);
458+
assert!(matches!(res, PermissionEvalResult::Ask));
459+
460+
let tool_allow_wild_card = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
461+
"command": "allow_wild_card some_arg",
462+
}))
463+
.unwrap();
464+
let res = tool_allow_wild_card.eval_perm(&os, &agent);
465+
assert!(matches!(res, PermissionEvalResult::Allow));
466+
467+
let tool_allow_exact_should_ask = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
468+
"command": "allow_exact some_arg",
469+
}))
470+
.unwrap();
471+
let res = tool_allow_exact_should_ask.eval_perm(&os, &agent);
472+
assert!(matches!(res, PermissionEvalResult::Ask));
473+
474+
let tool_allow_exact_should_allow = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
475+
"command": "allow_exact",
458476
}))
459477
.unwrap();
478+
let res = tool_allow_exact_should_allow.eval_perm(&os, &agent);
479+
assert!(matches!(res, PermissionEvalResult::Allow));
480+
481+
agent.allowed_tools.insert(tool_name.to_string());
460482

461-
let res = tool.eval_perm(&agent);
483+
let res = tool_two.eval_perm(&os, &agent);
462484
assert!(matches!(res, PermissionEvalResult::Allow));
485+
486+
// Denied list should remain denied
487+
let res = tool_one.eval_perm(&os, &agent);
488+
assert!(matches!(res, PermissionEvalResult::Deny(ref rules) if rules.contains(&"\\Agit .*\\z".to_string())));
463489
}
464490

465491
#[tokio::test]

0 commit comments

Comments
 (0)