From 0d3f6f4ba94083f49d042ea13b2c7c059e9b220c Mon Sep 17 00:00:00 2001 From: kkashilk Date: Wed, 17 Sep 2025 11:19:09 -0700 Subject: [PATCH 01/14] Bump up version for hotfix --- Cargo.lock | 4 ++-- Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cc2f536f7..f85bfb292 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1207,7 +1207,7 @@ checksum = "613afe47fcd5fac7ccf1db93babcb082c5994d996f20b8b159f2ad1658eb5724" [[package]] name = "chat_cli" -version = "1.16.0" +version = "1.16.1" dependencies = [ "amzn-codewhisperer-client", "amzn-codewhisperer-streaming-client", @@ -5647,7 +5647,7 @@ dependencies = [ [[package]] name = "semantic_search_client" -version = "1.16.0" +version = "1.16.1" dependencies = [ "anyhow", "bm25", diff --git a/Cargo.toml b/Cargo.toml index f35a1a154..bc7b16ff3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,7 +8,7 @@ authors = ["Amazon Q CLI Team (q-cli@amazon.com)", "Chay Nabors (nabochay@amazon edition = "2024" homepage = "https://aws.amazon.com/q/" publish = false -version = "1.16.0" +version = "1.16.1" license = "MIT OR Apache-2.0" [workspace.dependencies] From 012e81d1e8344e4e928e3deeef067083929be0df Mon Sep 17 00:00:00 2001 From: Brandon Kiser <51934408+brandonskiser@users.noreply.github.com> Date: Wed, 17 Sep 2025 12:18:20 -0700 Subject: [PATCH 02/14] chore: add 1.16.1 feed.json entry (#2919) --- crates/chat-cli/src/cli/feed.json | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/crates/chat-cli/src/cli/feed.json b/crates/chat-cli/src/cli/feed.json index d4c928005..a9d0f68eb 100644 --- a/crates/chat-cli/src/cli/feed.json +++ b/crates/chat-cli/src/cli/feed.json @@ -10,6 +10,18 @@ "hidden": true, "changes": [] }, + { + "type": "release", + "date": "2025-09-17", + "version": "1.16.1", + "title": "Version 1.16.1", + "changes": [ + { + "type": "fixed", + "description": "Dashboard not updating after logging in - [#688](https://github.com/aws/amazon-q-developer-cli-autocomplete/pull/688)" + } + ] + }, { "type": "release", "date": "2025-09-16", From a845c9154d730d74e53716c0c1e7a4448e1dda20 Mon Sep 17 00:00:00 2001 From: evanliu048 Date: Tue, 16 Sep 2025 14:50:16 -0700 Subject: [PATCH 03/14] fix format (#2895) --- crates/chat-cli/src/cli/chat/tools/fs_read.rs | 296 +++++++++--------- 1 file changed, 150 insertions(+), 146 deletions(-) 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 c4e60ae6c..5c5c0abf2 100644 --- a/crates/chat-cli/src/cli/chat/tools/fs_read.rs +++ b/crates/chat-cli/src/cli/chat/tools/fs_read.rs @@ -114,156 +114,156 @@ impl FsRead { } let is_in_allowlist = matches_any_pattern(&agent.allowed_tools, "fs_read"); - let settings = agent.tools_settings.get("fs_read").cloned() + let settings = agent + .tools_settings + .get("fs_read") + .cloned() .unwrap_or_else(|| serde_json::json!({})); - + { - let Settings { - mut allowed_paths, - denied_paths, - allow_read_only, - } = 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 Settings { + mut allowed_paths, + denied_paths, + allow_read_only, + } = 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 { + let Ok(path) = directories::canonicalizes_path(os, path) else { + continue; + }; + if let Err(e) = directories::add_gitignore_globs(&mut builder, path.as_str()) { + warn!("Failed to create glob from path given: {path}: {e}. Ignoring."); + } } - - let allow_set = { - let mut builder = GlobSetBuilder::new(); - for path in &allowed_paths { - let Ok(path) = directories::canonicalizes_path(os, path) else { - continue; - }; - if let Err(e) = directories::add_gitignore_globs(&mut builder, path.as_str()) { - warn!("Failed to create glob from path given: {path}: {e}. Ignoring."); - } + builder.build() + }; + + let mut sanitized_deny_list = Vec::<&String>::new(); + let deny_set = { + let mut builder = GlobSetBuilder::new(); + for path in &denied_paths { + let Ok(processed_path) = directories::canonicalizes_path(os, path) else { + continue; + }; + match directories::add_gitignore_globs(&mut builder, processed_path.as_str()) { + Ok(_) => { + // Note that we need to push twice here because for each rule we + // are creating two globs (one for file and one for directory) + sanitized_deny_list.push(path); + sanitized_deny_list.push(path); + }, + Err(e) => warn!("Failed to create glob from path given: {path}: {e}. Ignoring."), } - builder.build() - }; + } + builder.build() + }; - let mut sanitized_deny_list = Vec::<&String>::new(); - let deny_set = { - let mut builder = GlobSetBuilder::new(); - for path in &denied_paths { - let Ok(processed_path) = directories::canonicalizes_path(os, path) else { - continue; - }; - match directories::add_gitignore_globs(&mut builder, processed_path.as_str()) { - Ok(_) => { - // Note that we need to push twice here because for each rule we - // are creating two globs (one for file and one for directory) - sanitized_deny_list.push(path); - sanitized_deny_list.push(path); + match (allow_set, deny_set) { + (Ok(allow_set), Ok(deny_set)) => { + let mut deny_list = Vec::::new(); + let mut ask = false; + + for op in &self.operations { + match op { + FsReadOperation::Line(FsLine { path, .. }) + | FsReadOperation::Directory(FsDirectory { path, .. }) + | FsReadOperation::Search(FsSearch { path, .. }) => { + let Ok(path) = directories::canonicalizes_path(os, path) else { + ask = true; + continue; + }; + let denied_match_set = deny_set.matches(path.as_ref() as &str); + if !denied_match_set.is_empty() { + let deny_res = PermissionEvalResult::Deny({ + denied_match_set + .iter() + .filter_map(|i| sanitized_deny_list.get(*i).map(|s| (*s).clone())) + .collect::>() + }); + deny_list.push(deny_res); + continue; + } + + // We only want to ask if we are not allowing read only + // operation + if !is_in_allowlist && !allow_read_only && !allow_set.is_match(path.as_ref() as &str) { + ask = true; + } + }, + FsReadOperation::Image(fs_image) => { + let paths = &fs_image.image_paths; + let denied_match_set = paths + .iter() + .flat_map(|path| { + let Ok(path) = directories::canonicalizes_path(os, path) else { + return vec![]; + }; + deny_set.matches(path.as_ref() as &str) + }) + .collect::>(); + if !denied_match_set.is_empty() { + let deny_res = PermissionEvalResult::Deny({ + denied_match_set + .iter() + .filter_map(|i| sanitized_deny_list.get(*i).map(|s| (*s).clone())) + .collect::>() + }); + deny_list.push(deny_res); + continue; + } + + // We only want to ask if we are not allowing read only + // operation + if !is_in_allowlist + && !allow_read_only + && !paths.iter().any(|path| allow_set.is_match(path)) + { + ask = true; + } }, - Err(e) => warn!("Failed to create glob from path given: {path}: {e}. Ignoring."), } } - builder.build() - }; - - match (allow_set, deny_set) { - (Ok(allow_set), Ok(deny_set)) => { - let mut deny_list = Vec::::new(); - let mut ask = false; - - for op in &self.operations { - match op { - FsReadOperation::Line(FsLine { path, .. }) - | FsReadOperation::Directory(FsDirectory { path, .. }) - | FsReadOperation::Search(FsSearch { path, .. }) => { - let Ok(path) = directories::canonicalizes_path(os, path) else { - ask = true; - continue; - }; - let denied_match_set = deny_set.matches(path.as_ref() as &str); - if !denied_match_set.is_empty() { - let deny_res = PermissionEvalResult::Deny({ - denied_match_set - .iter() - .filter_map(|i| sanitized_deny_list.get(*i).map(|s| (*s).clone())) - .collect::>() - }); - deny_list.push(deny_res); - continue; - } - - // We only want to ask if we are not allowing read only - // operation - if !is_in_allowlist - && !allow_read_only - && !allow_set.is_match(path.as_ref() as &str) - { - ask = true; - } - }, - FsReadOperation::Image(fs_image) => { - let paths = &fs_image.image_paths; - let denied_match_set = paths - .iter() - .flat_map(|path| { - let Ok(path) = directories::canonicalizes_path(os, path) else { - return vec![]; - }; - deny_set.matches(path.as_ref() as &str) - }) - .collect::>(); - if !denied_match_set.is_empty() { - let deny_res = PermissionEvalResult::Deny({ - denied_match_set - .iter() - .filter_map(|i| sanitized_deny_list.get(*i).map(|s| (*s).clone())) - .collect::>() - }); - deny_list.push(deny_res); - continue; - } - - // We only want to ask if we are not allowing read only - // operation - if !is_in_allowlist - && !allow_read_only - && !paths.iter().any(|path| allow_set.is_match(path)) - { - ask = true; - } - }, - } - } - if !deny_list.is_empty() { - PermissionEvalResult::Deny({ - deny_list.into_iter().fold(Vec::::new(), |mut acc, res| { - if let PermissionEvalResult::Deny(mut rules) = res { - acc.append(&mut rules); - } - acc - }) + if !deny_list.is_empty() { + PermissionEvalResult::Deny({ + deny_list.into_iter().fold(Vec::::new(), |mut acc, res| { + if let PermissionEvalResult::Deny(mut rules) = res { + acc.append(&mut rules); + } + acc }) - } else if ask { - PermissionEvalResult::Ask - } else { - PermissionEvalResult::Allow - } - }, - (allow_res, deny_res) => { - if let Err(e) = allow_res { - warn!("fs_read failed to build allow set: {:?}", e); - } - if let Err(e) = deny_res { - warn!("fs_read failed to build deny set: {:?}", e); - } - warn!("One or more detailed args failed to parse, falling back to ask"); + }) + } else if ask { PermissionEvalResult::Ask - }, - } + } else { + PermissionEvalResult::Allow + } + }, + (allow_res, deny_res) => { + if let Err(e) = allow_res { + warn!("fs_read failed to build allow set: {:?}", e); + } + if let Err(e) = deny_res { + warn!("fs_read failed to build deny set: {:?}", e); + } + warn!("One or more detailed args failed to parse, falling back to ask"); + PermissionEvalResult::Ask + }, } + } } pub async fn invoke(&self, os: &Os, updates: &mut impl Write) -> Result { @@ -1452,12 +1452,11 @@ mod tests { #[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: { @@ -1479,7 +1478,8 @@ mod tests { { "path": "/explicitly/allowed/path", "mode": "Directory" }, { "path": "/explicitly/allowed/path/file.txt", "mode": "Line" }, ] - })).unwrap(); + })) + .unwrap(); let res = allowed_tool.eval_perm(&os, &agent); assert!(matches!(res, PermissionEvalResult::Allow)); @@ -1489,7 +1489,8 @@ mod tests { { "path": "/home/user/", "mode": "Directory" }, { "path": "/home/user/file.txt", "mode": "Line" }, ] - })).unwrap(); + })) + .unwrap(); let res = cwd_tool.eval_perm(&os, &agent); assert!(matches!(res, PermissionEvalResult::Allow)); @@ -1498,7 +1499,8 @@ mod tests { "operations": [ { "path": "/tmp/not/allowed/file.txt", "mode": "Line" } ] - })).unwrap(); + })) + .unwrap(); let res = outside_tool.eval_perm(&os, &agent); assert!(matches!(res, PermissionEvalResult::Ask)); } @@ -1507,7 +1509,7 @@ mod tests { 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 @@ -1520,7 +1522,8 @@ mod tests { { "path": "/home/user/", "mode": "Directory" }, { "path": "/home/user/file.txt", "mode": "Line" }, ] - })).unwrap(); + })) + .unwrap(); let res = cwd_tool.eval_perm(&os, &agent); assert!(matches!(res, PermissionEvalResult::Allow)); @@ -1529,7 +1532,8 @@ mod tests { "operations": [ { "path": "/tmp/not/allowed/file.txt", "mode": "Line" } ] - })).unwrap(); + })) + .unwrap(); let res = outside_tool.eval_perm(&os, &agent); assert!(matches!(res, PermissionEvalResult::Ask)); } From 57dea4fffa7658fc3a60ef3786f665a1f266b542 Mon Sep 17 00:00:00 2001 From: Felix Ding Date: Wed, 17 Sep 2025 10:52:23 -0700 Subject: [PATCH 04/14] fix(mcp): shell expansion not being done for stdio command (#2915) --- crates/chat-cli/src/mcp_client/client.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/chat-cli/src/mcp_client/client.rs b/crates/chat-cli/src/mcp_client/client.rs index fbf5d745d..4156cf34a 100644 --- a/crates/chat-cli/src/mcp_client/client.rs +++ b/crates/chat-cli/src/mcp_client/client.rs @@ -60,7 +60,10 @@ use crate::cli::chat::tools::custom_tool::{ TransportType, }; use crate::os::Os; -use crate::util::directories::DirectoryError; +use crate::util::directories::{ + DirectoryError, + canonicalizes_path, +}; /// Fetches all pages of specified resources from a server macro_rules! paginated_fetch { @@ -504,7 +507,8 @@ impl McpClientService { match transport_type { TransportType::Stdio => { - let command = Command::new(command_as_str).configure(|cmd| { + let expanded_cmd = canonicalizes_path(os, command_as_str)?; + let command = Command::new(expanded_cmd).configure(|cmd| { if let Some(envs) = config_envs { process_env_vars(envs, &os.env); cmd.envs(envs); From a7dabae45dc782655145aa9ffab20292dfef86d8 Mon Sep 17 00:00:00 2001 From: Erben Mo Date: Wed, 17 Sep 2025 15:24:37 -0700 Subject: [PATCH 05/14] Change autocomplete shortcut from ctrl-f to ctrl-g (#2825) * Change autocomplete shortcut from ctrl-f to ctrl-g The reason is ctrl-f is the standard shortcut in UNIX for moving cursor forward by 1 character. You can find it being supported everywhere... in your browser, your terminal, etc. * make the autocompletion key configurable --- crates/chat-cli/src/cli/chat/prompt.rs | 43 +++++++++++++++++++++--- crates/chat-cli/src/database/settings.rs | 4 +++ 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/crates/chat-cli/src/cli/chat/prompt.rs b/crates/chat-cli/src/cli/chat/prompt.rs index e7addd8f3..06d449fdc 100644 --- a/crates/chat-cli/src/cli/chat/prompt.rs +++ b/crates/chat-cli/src/cli/chat/prompt.rs @@ -474,19 +474,23 @@ pub fn rl( EventHandler::Simple(Cmd::Insert(1, "\n".to_string())), ); - // Add custom keybinding for Ctrl+J to insert a newline + // Add custom keybinding for Ctrl+j to insert a newline rl.bind_sequence( KeyEvent(KeyCode::Char('j'), Modifiers::CTRL), EventHandler::Simple(Cmd::Insert(1, "\n".to_string())), ); - // Add custom keybinding for Ctrl+F to accept hint (like fish shell) + // Add custom keybinding for autocompletion hint acceptance (configurable) + let autocompletion_key_char = match os.database.settings.get_string(Setting::AutocompletionKey) { + Some(key) if key.len() == 1 => key.chars().next().unwrap_or('g'), + _ => 'g', // Default to 'g' if setting is missing or invalid + }; rl.bind_sequence( - KeyEvent(KeyCode::Char('f'), Modifiers::CTRL), + KeyEvent(KeyCode::Char(autocompletion_key_char), Modifiers::CTRL), EventHandler::Simple(Cmd::CompleteHint), ); - // Add custom keybinding for Ctrl+T to toggle tangent mode (configurable) + // Add custom keybinding for Ctrl+t to toggle tangent mode (configurable) let tangent_key_char = match os.database.settings.get_string(Setting::TangentModeKey) { Some(key) if key.len() == 1 => key.chars().next().unwrap_or('t'), _ => 't', // Default to 't' if setting is missing or invalid @@ -722,4 +726,35 @@ mod tests { let hint = hinter.hint(line, pos, &ctx); assert_eq!(hint, None); } + + #[tokio::test] + // If you get a unit test failure for key override, please consider using a new key binding instead. + // The list of reserved keybindings here are the standard in UNIX world so please don't take them + async fn test_no_emacs_keybindings_overridden() { + let (sender, _) = tokio::sync::broadcast::channel::(1); + let (_, receiver) = tokio::sync::broadcast::channel::(1); + + // Create a mock Os for testing + let mock_os = crate::os::Os::new().await.unwrap(); + let mut test_editor = rl(&mock_os, sender, receiver).unwrap(); + + // Reserved Emacs keybindings that should not be overridden + let reserved_keys = ['a', 'e', 'f', 'b', 'k']; + + for &key in &reserved_keys { + let key_event = KeyEvent(KeyCode::Char(key), Modifiers::CTRL); + + // Try to bind and get the previous handler + let previous_handler = test_editor.bind_sequence( + key_event, + EventHandler::Simple(Cmd::Noop) + ); + + // If there was a previous handler, it means the key was already bound + // (which could be our custom binding overriding Emacs) + if previous_handler.is_some() { + panic!("Ctrl+{} appears to be overridden (found existing binding)", key); + } + } + } } diff --git a/crates/chat-cli/src/database/settings.rs b/crates/chat-cli/src/database/settings.rs index 21e8e9809..bc005438c 100644 --- a/crates/chat-cli/src/database/settings.rs +++ b/crates/chat-cli/src/database/settings.rs @@ -41,6 +41,8 @@ pub enum Setting { KnowledgeIndexType, #[strum(message = "Key binding for fuzzy search command (single character)")] SkimCommandKey, + #[strum(message = "Key binding for autocompletion hint acceptance (single character)")] + AutocompletionKey, #[strum(message = "Enable tangent mode feature (boolean)")] EnabledTangentMode, #[strum(message = "Key binding for tangent mode toggle (single character)")] @@ -94,6 +96,7 @@ impl AsRef for Setting { Self::KnowledgeChunkOverlap => "knowledge.chunkOverlap", Self::KnowledgeIndexType => "knowledge.indexType", Self::SkimCommandKey => "chat.skimCommandKey", + Self::AutocompletionKey => "chat.autocompletionKey", Self::EnabledTangentMode => "chat.enableTangentMode", Self::TangentModeKey => "chat.tangentModeKey", Self::IntrospectTangentMode => "introspect.tangentMode", @@ -139,6 +142,7 @@ impl TryFrom<&str> for Setting { "knowledge.chunkOverlap" => Ok(Self::KnowledgeChunkOverlap), "knowledge.indexType" => Ok(Self::KnowledgeIndexType), "chat.skimCommandKey" => Ok(Self::SkimCommandKey), + "chat.autocompletionKey" => Ok(Self::AutocompletionKey), "chat.enableTangentMode" => Ok(Self::EnabledTangentMode), "chat.tangentModeKey" => Ok(Self::TangentModeKey), "introspect.tangentMode" => Ok(Self::IntrospectTangentMode), From c603d2dbe135a7a3eb28e70bbd5db929604bba94 Mon Sep 17 00:00:00 2001 From: Erben Mo Date: Wed, 17 Sep 2025 18:02:12 -0700 Subject: [PATCH 06/14] feat: add support for preToolUse and postToolUse hook (#2875) --- crates/chat-cli/src/cli/agent/hook.rs | 16 +- crates/chat-cli/src/cli/agent/legacy/hooks.rs | 1 + crates/chat-cli/src/cli/agent/mod.rs | 77 +++- crates/chat-cli/src/cli/chat/cli/hooks.rs | 416 ++++++++++++++++-- crates/chat-cli/src/cli/chat/context.rs | 8 +- crates/chat-cli/src/cli/chat/conversation.rs | 12 +- crates/chat-cli/src/cli/chat/mod.rs | 348 +++++++++++++++ crates/chat-cli/src/cli/chat/tool_manager.rs | 5 +- .../src/cli/chat/tools/custom_tool.rs | 8 +- .../chat-cli/src/cli/chat/tools/introspect.rs | 5 + crates/chat-cli/src/cli/chat/tools/mod.rs | 1 + docs/agent-format.md | 31 +- docs/hooks.md | 161 +++++++ 13 files changed, 1037 insertions(+), 52 deletions(-) create mode 100644 docs/hooks.md diff --git a/crates/chat-cli/src/cli/agent/hook.rs b/crates/chat-cli/src/cli/agent/hook.rs index 1cb899f5a..89ca74146 100644 --- a/crates/chat-cli/src/cli/agent/hook.rs +++ b/crates/chat-cli/src/cli/agent/hook.rs @@ -1,4 +1,3 @@ -use std::collections::HashMap; use std::fmt::Display; use schemars::JsonSchema; @@ -11,9 +10,6 @@ const DEFAULT_TIMEOUT_MS: u64 = 30_000; const DEFAULT_MAX_OUTPUT_SIZE: usize = 1024 * 10; const DEFAULT_CACHE_TTL_SECONDS: u64 = 0; -#[derive(Debug, Clone, Serialize, Deserialize, Eq, PartialEq, JsonSchema)] -pub struct Hooks(HashMap); - #[derive(Debug, Clone, Copy, Serialize, Deserialize, Eq, PartialEq, JsonSchema, Hash)] #[serde(rename_all = "camelCase")] pub enum HookTrigger { @@ -21,6 +17,10 @@ pub enum HookTrigger { AgentSpawn, /// Triggered per user message submission UserPromptSubmit, + /// Triggered before tool execution + PreToolUse, + /// Triggered after tool execution + PostToolUse, } impl Display for HookTrigger { @@ -28,6 +28,8 @@ impl Display for HookTrigger { match self { HookTrigger::AgentSpawn => write!(f, "agentSpawn"), HookTrigger::UserPromptSubmit => write!(f, "userPromptSubmit"), + HookTrigger::PreToolUse => write!(f, "preToolUse"), + HookTrigger::PostToolUse => write!(f, "postToolUse"), } } } @@ -61,6 +63,11 @@ pub struct Hook { #[serde(default = "Hook::default_cache_ttl_seconds")] pub cache_ttl_seconds: u64, + /// Optional glob matcher for hook + /// Currently used for matching tool name of PreToolUse and PostToolUse hook + #[serde(skip_serializing_if = "Option::is_none")] + pub matcher: Option, + #[schemars(skip)] #[serde(default, skip_serializing)] pub source: Source, @@ -73,6 +80,7 @@ impl Hook { timeout_ms: Self::default_timeout_ms(), max_output_size: Self::default_max_output_size(), cache_ttl_seconds: Self::default_cache_ttl_seconds(), + matcher: None, source, } } diff --git a/crates/chat-cli/src/cli/agent/legacy/hooks.rs b/crates/chat-cli/src/cli/agent/legacy/hooks.rs index 2a7a639d7..6929049b3 100644 --- a/crates/chat-cli/src/cli/agent/legacy/hooks.rs +++ b/crates/chat-cli/src/cli/agent/legacy/hooks.rs @@ -80,6 +80,7 @@ impl From for Option { timeout_ms: value.timeout_ms, max_output_size: value.max_output_size, cache_ttl_seconds: value.cache_ttl_seconds, + matcher: None, source: Default::default(), }) } diff --git a/crates/chat-cli/src/cli/agent/mod.rs b/crates/chat-cli/src/cli/agent/mod.rs index 9ef82c15f..3dcc65920 100644 --- a/crates/chat-cli/src/cli/agent/mod.rs +++ b/crates/chat-cli/src/cli/agent/mod.rs @@ -959,6 +959,7 @@ mod tests { use serde_json::json; use super::*; + use crate::cli::agent::hook::Source; const INPUT: &str = r#" { "name": "some_agent", @@ -968,21 +969,21 @@ mod tests { "fetch": { "command": "fetch3.1", "args": [] }, "git": { "command": "git-mcp", "args": [] } }, - "tools": [ + "tools": [ "@git" ], "toolAliases": { "@gits/some_tool": "some_tool2" }, - "allowedTools": [ - "fs_read", + "allowedTools": [ + "fs_read", "@fetch", "@gits/git_status" ], - "resources": [ + "resources": [ "file://~/my-genai-prompts/unittest.md" ], - "toolsSettings": { + "toolsSettings": { "fs_write": { "allowedPaths": ["~/**"] }, "@git/git_status": { "git_user": "$GIT_USER" } } @@ -1353,4 +1354,70 @@ mod tests { assert_eq!(agents.get_active().and_then(|a| a.model.as_ref()), None); } + + #[test] + fn test_agent_with_hooks() { + let agent_json = json!({ + "name": "test-agent", + "hooks": { + "agentSpawn": [ + { + "command": "git status" + } + ], + "preToolUse": [ + { + "matcher": "fs_write", + "command": "validate-tool.sh" + }, + { + "matcher": "fs_read", + "command": "enforce-tdd.sh" + } + ], + "postToolUse": [ + { + "matcher": "fs_write", + "command": "format-python.sh" + } + ] + } + }); + + let agent: Agent = serde_json::from_value(agent_json).expect("Failed to deserialize agent"); + + // Verify agent name + assert_eq!(agent.name, "test-agent"); + + // Verify agentSpawn hook + assert!(agent.hooks.contains_key(&HookTrigger::AgentSpawn)); + let agent_spawn_hooks = &agent.hooks[&HookTrigger::AgentSpawn]; + assert_eq!(agent_spawn_hooks.len(), 1); + assert_eq!(agent_spawn_hooks[0].command, "git status"); + assert_eq!(agent_spawn_hooks[0].matcher, None); + + // Verify preToolUse hooks + assert!(agent.hooks.contains_key(&HookTrigger::PreToolUse)); + let pre_tool_hooks = &agent.hooks[&HookTrigger::PreToolUse]; + assert_eq!(pre_tool_hooks.len(), 2); + + assert_eq!(pre_tool_hooks[0].command, "validate-tool.sh"); + assert_eq!(pre_tool_hooks[0].matcher, Some("fs_write".to_string())); + + assert_eq!(pre_tool_hooks[1].command, "enforce-tdd.sh"); + assert_eq!(pre_tool_hooks[1].matcher, Some("fs_read".to_string())); + + // Verify postToolUse hooks + assert!(agent.hooks.contains_key(&HookTrigger::PostToolUse)); + + // Verify default values are set correctly + for hooks in agent.hooks.values() { + for hook in hooks { + assert_eq!(hook.timeout_ms, 30_000); + assert_eq!(hook.max_output_size, 10_240); + assert_eq!(hook.cache_ttl_seconds, 0); + assert_eq!(hook.source, Source::Agent); + } + } + } } diff --git a/crates/chat-cli/src/cli/chat/cli/hooks.rs b/crates/chat-cli/src/cli/chat/cli/hooks.rs index 38763285c..96e1e842d 100644 --- a/crates/chat-cli/src/cli/chat/cli/hooks.rs +++ b/crates/chat-cli/src/cli/chat/cli/hooks.rs @@ -37,6 +37,8 @@ use crate::cli::agent::hook::{ Hook, HookTrigger, }; +use crate::cli::agent::is_mcp_tool_ref; +use crate::util::MCP_SERVER_TOOL_DELIMITER; use crate::cli::chat::consts::AGENT_FORMAT_HOOKS_DOC_URL; use crate::cli::chat::util::truncate_safe; use crate::cli::chat::{ @@ -44,6 +46,47 @@ use crate::cli::chat::{ ChatSession, ChatState, }; +use crate::util::pattern_matching::matches_any_pattern; + +/// Hook execution result: (exit_code, output) +/// Output is stdout if exit_code is 0, stderr otherwise. +pub type HookOutput = (i32, String); + +/// Check if a hook matches a tool name based on its matcher pattern +fn hook_matches_tool(hook: &Hook, tool_name: &str) -> bool { + match &hook.matcher { + None => true, // No matcher means the hook runs for all tools + Some(pattern) => { + match pattern.as_str() { + "*" => true, // Wildcard matches all tools + "@builtin" => !is_mcp_tool_ref(tool_name), // Built-in tools are not MCP tools + _ => { + // If tool_name is MCP, check server pattern first + if is_mcp_tool_ref(tool_name) { + if let Some(server_name) = tool_name.strip_prefix('@').and_then(|s| s.split(MCP_SERVER_TOOL_DELIMITER).next()) { + let server_pattern = format!("@{}", server_name); + if pattern == &server_pattern { + return true; + } + } + } + + // Use matches_any_pattern for both MCP and built-in tools + let mut patterns = std::collections::HashSet::new(); + patterns.insert(pattern.clone()); + matches_any_pattern(&patterns, tool_name) + } + } + } + } +} + +#[derive(Debug, Clone)] +pub struct ToolContext { + pub tool_name: String, + pub tool_input: serde_json::Value, + pub tool_response: Option, +} #[derive(Debug, Clone)] pub struct CachedHook { @@ -74,22 +117,32 @@ impl HookExecutor { &mut self, hooks: HashMap>, output: &mut impl Write, + cwd: &str, prompt: Option<&str>, - ) -> Result, ChatError> { + tool_context: Option, + ) -> Result, ChatError> { let mut cached = vec![]; let mut futures = FuturesUnordered::new(); for hook in hooks .into_iter() .flat_map(|(trigger, hooks)| hooks.into_iter().map(move |hook| (trigger, hook))) { + // Filter hooks by tool matcher + if let Some(tool_ctx) = &tool_context { + if !hook_matches_tool(&hook.1, &tool_ctx.tool_name) { + continue; // Skip this hook - doesn't match tool + } + } + if let Some(cache) = self.get_cache(&hook) { - cached.push((hook.clone(), cache.clone())); + // Note: we only cache successful hook run. hence always using 0 as exit code for cached hook + cached.push((hook.clone(), (0, cache))); continue; } - futures.push(self.run_hook(hook, prompt)); + futures.push(self.run_hook(hook, cwd, prompt, tool_context.clone())); } - let mut complete = 0; + let mut complete = 0; // number of hooks that are run successfully with exit code 0 let total = futures.len(); let mut spinner = None; let spinner_text = |complete: usize, total: usize| { @@ -138,9 +191,25 @@ impl HookExecutor { } // Process results regardless of output enabled - if let Ok(output) = result { - complete += 1; - results.push((hook, output)); + if let Ok((exit_code, hook_output)) = &result { + // Print warning if exit code is not 0 + if *exit_code != 0 { + queue!( + output, + style::SetForegroundColor(style::Color::Red), + style::Print("✗ "), + style::ResetColor, + style::Print(format!("{} \"", hook.0)), + style::Print(&hook.1.command), + style::Print("\""), + style::SetForegroundColor(style::Color::Red), + style::Print(format!(" failed with exit code: {}, stderr: {})\n", exit_code, hook_output.trim_end())), + style::ResetColor, + )?; + } else { + complete += 1; + } + results.push((hook, result.unwrap())); } // Display ending summary or add a new spinner @@ -167,12 +236,17 @@ impl HookExecutor { drop(futures); // Fill cache with executed results, skipping what was already from cache - for ((trigger, hook), output) in &results { + for ((trigger, hook), (exit_code, output)) in &results { + if *exit_code != 0 { + continue; // Only cache successful hooks + } self.cache.insert((*trigger, hook.clone()), CachedHook { output: output.clone(), expiry: match trigger { HookTrigger::AgentSpawn => None, HookTrigger::UserPromptSubmit => Some(Instant::now() + Duration::from_secs(hook.cache_ttl_seconds)), + HookTrigger::PreToolUse => Some(Instant::now() + Duration::from_secs(hook.cache_ttl_seconds)), + HookTrigger::PostToolUse => Some(Instant::now() + Duration::from_secs(hook.cache_ttl_seconds)), }, }); } @@ -185,8 +259,10 @@ impl HookExecutor { async fn run_hook( &self, hook: (HookTrigger, Hook), + cwd: &str, prompt: Option<&str>, - ) -> ((HookTrigger, Hook), Result, Duration) { + tool_context: Option, + ) -> ((HookTrigger, Hook), Result, Duration) { let start_time = Instant::now(); let command = &hook.1.command; @@ -213,33 +289,61 @@ impl HookExecutor { let timeout = Duration::from_millis(hook.1.timeout_ms); - // Set USER_PROMPT environment variable if provided + // Generate hook command input in JSON format + let mut hook_input = serde_json::json!({ + "hook_event_name": hook.0.to_string(), + "cwd": cwd + }); + + // Set USER_PROMPT environment variable and add to JSON input if provided if let Some(prompt) = prompt { // Sanitize the prompt to avoid issues with special characters let sanitized_prompt = sanitize_user_prompt(prompt); cmd.env("USER_PROMPT", sanitized_prompt); + hook_input["prompt"] = serde_json::Value::String(prompt.to_string()); } - let command_future = cmd.output(); + // ToolUse specific input + if let Some(tool_ctx) = tool_context { + hook_input["tool_name"] = serde_json::Value::String(tool_ctx.tool_name); + hook_input["tool_input"] = tool_ctx.tool_input; + if let Some(response) = tool_ctx.tool_response { + hook_input["tool_response"] = response; + } + } + let json_input = serde_json::to_string(&hook_input).unwrap_or_default(); + + // Build a future for hook command w/ the JSON input passed in through STDIN + let command_future = async move { + let mut child = cmd.spawn()?; + if let Some(stdin) = child.stdin.take() { + use tokio::io::AsyncWriteExt; + let mut stdin = stdin; + let _ = stdin.write_all(json_input.as_bytes()).await; + let _ = stdin.shutdown().await; + } + child.wait_with_output().await + }; // Run with timeout let result = match tokio::time::timeout(timeout, command_future).await { - Ok(Ok(result)) => { - if result.status.success() { - let stdout = result.stdout.to_str_lossy(); - let stdout = format!( - "{}{}", - truncate_safe(&stdout, hook.1.max_output_size), - if stdout.len() > hook.1.max_output_size { - " ... truncated" - } else { - "" - } - ); - Ok(stdout) + Ok(Ok(output)) => { + let exit_code = output.status.code().unwrap_or(-1); + let raw_output = if exit_code == 0 { + output.stdout.to_str_lossy() } else { - Err(eyre!("command returned non-zero exit code: {}", result.status)) - } + output.stderr.to_str_lossy() + }; + let formatted_output = format!( + "{}{}", + truncate_safe(&raw_output, hook.1.max_output_size), + if raw_output.len() > hook.1.max_output_size { + " ... truncated" + } else { + "" + } + ); + Ok((exit_code, formatted_output)) }, Ok(Err(err)) => Err(eyre!("failed to execute command: {}", err)), Err(_) => Err(eyre!("command timed out after {} ms", timeout.as_millis())), @@ -330,3 +434,263 @@ impl HooksArgs { }) } } + +#[cfg(test)] +mod tests { + use super::*; + use std::collections::HashMap; + use crate::cli::agent::hook::{Hook, HookTrigger}; + use tempfile::TempDir; + + #[test] + fn test_hook_matches_tool() { + let hook_no_matcher = Hook { + command: "echo test".to_string(), + timeout_ms: 5000, + cache_ttl_seconds: 0, + max_output_size: 1000, + matcher: None, + source: crate::cli::agent::hook::Source::Session, + }; + + let fs_write_hook = Hook { + command: "echo test".to_string(), + timeout_ms: 5000, + cache_ttl_seconds: 0, + max_output_size: 1000, + matcher: Some("fs_write".to_string()), + source: crate::cli::agent::hook::Source::Session, + }; + + let fs_wildcard_hook = Hook { + command: "echo test".to_string(), + timeout_ms: 5000, + cache_ttl_seconds: 0, + max_output_size: 1000, + matcher: Some("fs_*".to_string()), + source: crate::cli::agent::hook::Source::Session, + }; + + let all_tools_hook = Hook { + command: "echo test".to_string(), + timeout_ms: 5000, + cache_ttl_seconds: 0, + max_output_size: 1000, + matcher: Some("*".to_string()), + source: crate::cli::agent::hook::Source::Session, + }; + + let builtin_hook = Hook { + command: "echo test".to_string(), + timeout_ms: 5000, + cache_ttl_seconds: 0, + max_output_size: 1000, + matcher: Some("@builtin".to_string()), + source: crate::cli::agent::hook::Source::Session, + }; + + let git_server_hook = Hook { + command: "echo test".to_string(), + timeout_ms: 5000, + cache_ttl_seconds: 0, + max_output_size: 1000, + matcher: Some("@git".to_string()), + source: crate::cli::agent::hook::Source::Session, + }; + + let git_status_hook = Hook { + command: "echo test".to_string(), + timeout_ms: 5000, + cache_ttl_seconds: 0, + max_output_size: 1000, + matcher: Some("@git/status".to_string()), + source: crate::cli::agent::hook::Source::Session, + }; + + // No matcher should match all tools + assert!(hook_matches_tool(&hook_no_matcher, "fs_write")); + assert!(hook_matches_tool(&hook_no_matcher, "execute_bash")); + assert!(hook_matches_tool(&hook_no_matcher, "@git/status")); + + // Exact matcher should only match exact tool + assert!(hook_matches_tool(&fs_write_hook, "fs_write")); + assert!(!hook_matches_tool(&fs_write_hook, "fs_read")); + + // Wildcard matcher should match pattern + assert!(hook_matches_tool(&fs_wildcard_hook, "fs_write")); + assert!(hook_matches_tool(&fs_wildcard_hook, "fs_read")); + assert!(!hook_matches_tool(&fs_wildcard_hook, "execute_bash")); + + // * should match all tools + assert!(hook_matches_tool(&all_tools_hook, "fs_write")); + assert!(hook_matches_tool(&all_tools_hook, "execute_bash")); + assert!(hook_matches_tool(&all_tools_hook, "@git/status")); + + // @builtin should match built-in tools only + assert!(hook_matches_tool(&builtin_hook, "fs_write")); + assert!(hook_matches_tool(&builtin_hook, "execute_bash")); + assert!(!hook_matches_tool(&builtin_hook, "@git/status")); + + // @git should match all git server tools + assert!(hook_matches_tool(&git_server_hook, "@git/status")); + assert!(!hook_matches_tool(&git_server_hook, "@other/tool")); + assert!(!hook_matches_tool(&git_server_hook, "fs_write")); + + // @git/status should match exact MCP tool + assert!(hook_matches_tool(&git_status_hook, "@git/status")); + assert!(!hook_matches_tool(&git_status_hook, "@git/commit")); + assert!(!hook_matches_tool(&git_status_hook, "fs_write")); + } + + #[tokio::test] + async fn test_hook_executor_with_tool_context() { + let mut executor = HookExecutor::new(); + let mut output = Vec::new(); + + // Create temp directory and file + let temp_dir = TempDir::new().unwrap(); + let test_file = temp_dir.path().join("hook_output.json"); + let test_file_str = test_file.to_string_lossy(); + + // Create a simple hook that writes JSON input to a file + #[cfg(unix)] + let command = format!("cat > {}", test_file_str); + #[cfg(windows)] + let command = format!("type > {}", test_file_str); + + let hook = Hook { + command, + timeout_ms: 5000, + cache_ttl_seconds: 0, + max_output_size: 1000, + matcher: Some("fs_write".to_string()), + source: crate::cli::agent::hook::Source::Session, + }; + + let mut hooks = HashMap::new(); + hooks.insert(HookTrigger::PreToolUse, vec![hook]); + + let tool_context = ToolContext { + tool_name: "fs_write".to_string(), + tool_input: serde_json::json!({ + "command": "create", + "path": "/test/file.py" + }), + tool_response: None, + }; + + // Run the hook + let result = executor.run_hooks( + hooks, + &mut output, + ".", + None, + Some(tool_context) + ).await; + + assert!(result.is_ok()); + + // Verify the hook wrote the JSON input to the file + if let Ok(content) = std::fs::read_to_string(&test_file) { + let json: serde_json::Value = serde_json::from_str(&content).unwrap(); + assert_eq!(json["hook_event_name"], "preToolUse"); + assert_eq!(json["tool_name"], "fs_write"); + assert_eq!(json["tool_input"]["command"], "create"); + assert_eq!(json["cwd"], "."); + } + // TempDir automatically cleans up when dropped + } + + #[tokio::test] + async fn test_hook_filtering_no_match() { + let mut executor = HookExecutor::new(); + let mut output = Vec::new(); + + // Hook that matches execute_bash (should NOT run for fs_write tool call) + let execute_bash_hook = Hook { + command: "echo 'should not run'".to_string(), + timeout_ms: 5000, + cache_ttl_seconds: 0, + max_output_size: 1000, + matcher: Some("execute_bash".to_string()), + source: crate::cli::agent::hook::Source::Session, + }; + + let mut hooks = HashMap::new(); + hooks.insert(HookTrigger::PostToolUse, vec![execute_bash_hook]); + + let tool_context = ToolContext { + tool_name: "fs_write".to_string(), + tool_input: serde_json::json!({"command": "create"}), + tool_response: Some(serde_json::json!({"success": true})), + }; + + // Run the hooks + let result = executor.run_hooks( + hooks, + &mut output, + ".", // cwd - using current directory for now + None, // prompt - no user prompt for this test + Some(tool_context) + ).await; + + assert!(result.is_ok()); + let hook_results = result.unwrap(); + + // Should run 0 hooks because matcher doesn't match tool_name + assert_eq!(hook_results.len(), 0); + + // Output should be empty since no hooks ran + assert!(output.is_empty()); + } + + #[tokio::test] + async fn test_hook_exit_code_2() { + let mut executor = HookExecutor::new(); + let mut output = Vec::new(); + + // Create a hook that exits with code 2 and outputs to stderr + #[cfg(unix)] + let command = "echo 'Tool execution blocked by security policy' >&2; exit 2"; + #[cfg(windows)] + let command = "echo Tool execution blocked by security policy 1>&2 & exit /b 2"; + + let hook = Hook { + command: command.to_string(), + timeout_ms: 5000, + cache_ttl_seconds: 0, + max_output_size: 1000, + matcher: Some("fs_write".to_string()), + source: crate::cli::agent::hook::Source::Session, + }; + + let hooks = HashMap::from([ + (HookTrigger::PreToolUse, vec![hook]) + ]); + + let tool_context = ToolContext { + tool_name: "fs_write".to_string(), + tool_input: serde_json::json!({ + "command": "create", + "path": "/sensitive/file.py" + }), + tool_response: None, + }; + + let results = executor.run_hooks( + hooks, + &mut output, + ".", // cwd + None, // prompt + Some(tool_context) + ).await.unwrap(); + + // Should have one result + assert_eq!(results.len(), 1); + + let ((trigger, _hook), (exit_code, hook_output)) = &results[0]; + assert_eq!(*trigger, HookTrigger::PreToolUse); + assert_eq!(*exit_code, 2); + assert!(hook_output.contains("Tool execution blocked by security policy")); + } +} diff --git a/crates/chat-cli/src/cli/chat/context.rs b/crates/chat-cli/src/cli/chat/context.rs index 1fdcc5e8e..fa39f8039 100644 --- a/crates/chat-cli/src/cli/chat/context.rs +++ b/crates/chat-cli/src/cli/chat/context.rs @@ -16,6 +16,7 @@ use serde::{ use super::cli::model::context_window_tokens; use super::util::drop_matched_context_files; +use super::cli::hooks::HookOutput; use crate::cli::agent::Agent; use crate::cli::agent::hook::{ Hook, @@ -247,11 +248,14 @@ impl ContextManager { &mut self, trigger: HookTrigger, output: &mut impl Write, + os: &crate::os::Os, prompt: Option<&str>, - ) -> Result, ChatError> { + tool_context: Option, + ) -> Result, ChatError> { let mut hooks = self.hooks.clone(); hooks.retain(|t, _| *t == trigger); - self.hook_executor.run_hooks(hooks, output, prompt).await + let cwd = os.env.current_dir()?.to_string_lossy().to_string(); + self.hook_executor.run_hooks(hooks, output, &cwd, prompt, tool_context).await } } diff --git a/crates/chat-cli/src/cli/chat/conversation.rs b/crates/chat-cli/src/cli/chat/conversation.rs index 50025f229..917969780 100644 --- a/crates/chat-cli/src/cli/chat/conversation.rs +++ b/crates/chat-cli/src/cli/chat/conversation.rs @@ -29,6 +29,7 @@ use tracing::{ }; use super::cli::compact::CompactStrategy; +use super::cli::hooks::HookOutput; use super::cli::model::context_window_tokens; use super::consts::{ DUMMY_TOOL_NAME, @@ -563,12 +564,12 @@ impl ConversationState { let mut agent_spawn_context = None; if let Some(cm) = self.context_manager.as_mut() { let user_prompt = self.next_message.as_ref().and_then(|m| m.prompt()); - let agent_spawn = cm.run_hooks(HookTrigger::AgentSpawn, output, user_prompt).await?; + let agent_spawn = cm.run_hooks(HookTrigger::AgentSpawn, output, os, user_prompt, None /* tool_context */).await?; agent_spawn_context = format_hook_context(&agent_spawn, HookTrigger::AgentSpawn); if let (true, Some(next_message)) = (run_perprompt_hooks, self.next_message.as_mut()) { let per_prompt = cm - .run_hooks(HookTrigger::UserPromptSubmit, output, next_message.prompt()) + .run_hooks(HookTrigger::UserPromptSubmit, output, os, next_message.prompt(), None /* tool_context */) .await?; if let Some(ctx) = format_hook_context(&per_prompt, HookTrigger::UserPromptSubmit) { next_message.additional_context = ctx; @@ -1030,8 +1031,9 @@ impl From for ToolInputSchema { /// # Returns /// [Option::Some] if `hook_results` is not empty and at least one hook has content. Otherwise, /// [Option::None] -fn format_hook_context(hook_results: &[((HookTrigger, Hook), String)], trigger: HookTrigger) -> Option { - if hook_results.iter().all(|(_, content)| content.is_empty()) { +fn format_hook_context(hook_results: &[((HookTrigger, Hook), HookOutput)], trigger: HookTrigger) -> Option { + // Note: only format context when hook command exit code is 0 + if hook_results.iter().all(|(_, (exit_code, content))| *exit_code != 0 || content.is_empty()) { return None; } @@ -1044,7 +1046,7 @@ fn format_hook_context(hook_results: &[((HookTrigger, Hook), String)], trigger: } context_content.push_str("\n\n"); - for (_, output) in hook_results.iter().filter(|((h_trigger, _), _)| *h_trigger == trigger) { + for (_, (_, output)) in hook_results.iter().filter(|((h_trigger, _), (exit_code, _))| *h_trigger == trigger && *exit_code == 0) { context_content.push_str(&format!("{output}\n\n")); } context_content.push_str(CONTEXT_ENTRY_END_HEADER); diff --git a/crates/chat-cli/src/cli/chat/mod.rs b/crates/chat-cli/src/cli/chat/mod.rs index 942653bf0..2da6b3d5c 100644 --- a/crates/chat-cli/src/cli/chat/mod.rs +++ b/crates/chat-cli/src/cli/chat/mod.rs @@ -42,6 +42,7 @@ use clap::{ ValueEnum, }; use cli::compact::CompactStrategy; +use cli::hooks::ToolContext; use cli::model::{ find_model, get_available_models, @@ -2261,6 +2262,7 @@ impl ChatSession { }); } + // All tools are allowed now // Execute the requested tools. let mut tool_results = vec![]; let mut image_blocks: Vec = Vec::new(); @@ -2426,6 +2428,45 @@ impl ChatSession { } } + // Run PostToolUse hooks for all executed tools after we have the tool_results + if let Some(cm) = self.conversation.context_manager.as_mut() { + for result in &tool_results { + if let Some(tool) = self.tool_uses.iter().find(|t| t.id == result.tool_use_id) { + let content: Vec = result.content.iter().map(|block| { + match block { + ToolUseResultBlock::Text(text) => serde_json::Value::String(text.clone()), + ToolUseResultBlock::Json(json) => json.clone(), + } + }).collect(); + + let tool_response = match result.status { + ToolResultStatus::Success => serde_json::json!({"success": true, "result": content}), + ToolResultStatus::Error => serde_json::json!({"success": false, "error": content}), + }; + + let tool_context = ToolContext { + tool_name: match &tool.tool { + Tool::Custom(custom_tool) => custom_tool.namespaced_tool_name(), // for MCP tool, pass MCP name to the hook + _ => tool.name.clone(), + }, + tool_input: tool.tool_input.clone(), + tool_response: Some(tool_response), + }; + + // Here is how we handle postToolUse output: + // Exit code is 0: nothing. stdout is not shown to user. We don't support processing the PostToolUse hook output yet. + // Exit code is non-zero: display an error to user (already taken care of by the ContextManager.run_hooks) + let _ = cm.run_hooks( + crate::cli::agent::hook::HookTrigger::PostToolUse, + &mut std::io::stderr(), + os, + None, + Some(tool_context) + ).await; + } + } + } + if !image_blocks.is_empty() { let images = image_blocks.into_iter().map(|(block, _)| block).collect(); self.conversation.add_tool_results_with_images(tool_results, images); @@ -2761,6 +2802,7 @@ impl ChatSession { } } + // Validate the tool use request from LLM, including basic checks like fs_read file should exist, as well as user-defined preToolUse hook check. async fn validate_tools(&mut self, os: &Os, tool_uses: Vec) -> Result { let conv_id = self.conversation.conversation_id().to_owned(); debug!(?tool_uses, "Validating tool uses"); @@ -2770,6 +2812,7 @@ impl ChatSession { for tool_use in tool_uses { let tool_use_id = tool_use.id.clone(); let tool_use_name = tool_use.name.clone(); + let tool_input = tool_use.args.clone(); let mut tool_telemetry = ToolUseEventBuilder::new( conv_id.clone(), tool_use.id.clone(), @@ -2791,6 +2834,7 @@ impl ChatSession { name: tool_use_name, tool, accepted: false, + tool_input, }); }, Err(err) => { @@ -2862,9 +2906,75 @@ impl ChatSession { )); } + // Execute PreToolUse hooks for all validated tools + // The mental model is preToolHook is like validate tools, but its behavior can be customized by user + // Note that after preTookUse hook, user can still reject the took run + if let Some(cm) = self.conversation.context_manager.as_mut() { + for tool in &queued_tools { + let tool_context = ToolContext { + tool_name: match &tool.tool { + Tool::Custom(custom_tool) => custom_tool.namespaced_tool_name(), // for MCP tool, pass MCP name to the hook + _ => tool.name.clone(), + }, + tool_input: tool.tool_input.clone(), + tool_response: None, + }; + + let hook_results = cm.run_hooks( + crate::cli::agent::hook::HookTrigger::PreToolUse, + &mut std::io::stderr(), + os, + None, /* prompt */ + Some(tool_context) + ).await?; + + // Here is how we handle the preToolUse hook output: + // Exit code is 0: nothing. stdout is not shown to user. + // Exit code is 2: block the tool use. return stderr to LLM. show warning to user + // Other error: show warning to user. + + // Check for exit code 2 and add to tool_results + for (_, (exit_code, output)) in &hook_results { + if *exit_code == 2 { + tool_results.push(ToolUseResult { + tool_use_id: tool.id.clone(), + content: vec![ToolUseResultBlock::Text(format!("PreToolHook blocked the tool execution: {}", output))], + status: ToolResultStatus::Error, + }); + } + } + } + } + + // If we have any hook validation errors, return them immediately to the model + if !tool_results.is_empty() { + debug!(?tool_results, "Error found in PreToolUse hooks"); + for tool_result in &tool_results { + for block in &tool_result.content { + if let ToolUseResultBlock::Text(content) = block { + queue!( + self.stderr, + style::Print("\n"), + style::SetForegroundColor(Color::Red), + style::Print(format!("{}\n", content)), + style::SetForegroundColor(Color::Reset), + )?; + } + } + } + + self.conversation.add_tool_results(tool_results); + return Ok(ChatState::HandleResponseStream( + self.conversation + .as_sendable_conversation_state(os, &mut self.stderr, false) + .await?, + )); + } + self.tool_uses = queued_tools; self.pending_tool_index = Some(0); self.tool_turn_start_time = Some(Instant::now()); + Ok(ChatState::ExecuteTools) } @@ -3786,6 +3896,244 @@ mod tests { .unwrap(); } + // Integration test for PreToolUse hook functionality. + // + // In this integration test we create a preToolUse hook that logs tool info into a file + // and we run fs_read and verify the log is generated with the correct ToolContext data. + #[tokio::test] + async fn test_tool_hook_integration() { + use crate::cli::agent::hook::{Hook, HookTrigger}; + use std::collections::HashMap; + + let mut os = Os::new().await.unwrap(); + os.client.set_mock_output(serde_json::json!([ + [ + "I'll read that file for you", + { + "tool_use_id": "1", + "name": "fs_read", + "args": { + "operations": [ + { + "mode": "Line", + "path": "/test.txt", + "start_line": 1, + "end_line": 3 + } + ] + } + } + ], + [ + "Here's the file content!", + ], + ])); + + // Create test file + os.fs.write("/test.txt", "line1\nline2\nline3\n").await.unwrap(); + + // Create agent with PreToolUse and PostToolUse hooks + let mut agents = Agents::default(); + let mut hooks = HashMap::new(); + + // Get the real path in the temp directory for the hooks to write to + let pre_hook_log_path = os.fs.chroot_path_str("/pre-hook-test.log"); + let post_hook_log_path = os.fs.chroot_path_str("/post-hook-test.log"); + let pre_hook_command = format!("cat > {}", pre_hook_log_path); + let post_hook_command = format!("cat > {}", post_hook_log_path); + + hooks.insert(HookTrigger::PreToolUse, vec![Hook { + command: pre_hook_command, + timeout_ms: 5000, + max_output_size: 1024, + cache_ttl_seconds: 0, + matcher: Some("fs_*".to_string()), // Match fs_read, fs_write, etc. + source: crate::cli::agent::hook::Source::Agent, + }]); + + hooks.insert(HookTrigger::PostToolUse, vec![Hook { + command: post_hook_command, + timeout_ms: 5000, + max_output_size: 1024, + cache_ttl_seconds: 0, + matcher: Some("fs_*".to_string()), // Match fs_read, fs_write, etc. + source: crate::cli::agent::hook::Source::Agent, + }]); + + let agent = Agent { + name: "TestAgent".to_string(), + hooks, + ..Default::default() + }; + agents.agents.insert("TestAgent".to_string(), agent); + agents.switch("TestAgent").expect("Failed to switch agent"); + + let tool_manager = ToolManager::default(); + let tool_config = serde_json::from_str::>(include_str!("tools/tool_index.json")) + .expect("Tools failed to load"); + + // Test that PreToolUse hook runs + ChatSession::new( + &mut os, + std::io::stdout(), + std::io::stderr(), + "fake_conv_id", + agents, + None, // No initial input + InputSource::new_mock(vec![ + "read /test.txt".to_string(), + "y".to_string(), // Accept tool execution + "exit".to_string(), + ]), + false, + || Some(80), + tool_manager, + None, + tool_config, + true, + false, + None, + ) + .await + .unwrap() + .spawn(&mut os) + .await + .unwrap(); + + // Verify the PreToolUse hook was called + if let Ok(pre_log_content) = os.fs.read_to_string("/pre-hook-test.log").await { + let pre_hook_data: serde_json::Value = serde_json::from_str(&pre_log_content) + .expect("PreToolUse hook output should be valid JSON"); + + assert_eq!(pre_hook_data["hook_event_name"], "preToolUse"); + assert_eq!(pre_hook_data["tool_name"], "fs_read"); + assert_eq!(pre_hook_data["tool_response"], serde_json::Value::Null); + + let tool_input = &pre_hook_data["tool_input"]; + assert!(tool_input["operations"].is_array()); + + println!("✓ PreToolUse hook validation passed: {}", pre_log_content); + } else { + panic!("PreToolUse hook log file not found - hook may not have been called"); + } + + // Verify the PostToolUse hook was called + if let Ok(post_log_content) = os.fs.read_to_string("/post-hook-test.log").await { + let post_hook_data: serde_json::Value = serde_json::from_str(&post_log_content) + .expect("PostToolUse hook output should be valid JSON"); + + assert_eq!(post_hook_data["hook_event_name"], "postToolUse"); + assert_eq!(post_hook_data["tool_name"], "fs_read"); + + // Validate tool_response structure for successful execution + let tool_response = &post_hook_data["tool_response"]; + assert_eq!(tool_response["success"], true); + assert!(tool_response["result"].is_array()); + + let result_blocks = tool_response["result"].as_array().unwrap(); + assert!(!result_blocks.is_empty()); + let content = result_blocks[0].as_str().unwrap(); + assert!(content.contains("line1\nline2\nline3")); + + println!("✓ PostToolUse hook validation passed: {}", post_log_content); + } else { + panic!("PostToolUse hook log file not found - hook may not have been called"); + } + } + + #[tokio::test] + async fn test_pretool_hook_blocking_integration() { + use crate::cli::agent::hook::{Hook, HookTrigger}; + use std::collections::HashMap; + + let mut os = Os::new().await.unwrap(); + + // Create a test file to read + os.fs.write("/sensitive.txt", "classified information").await.unwrap(); + + // Mock LLM responses: first tries fs_read, gets blocked, then responds to error + os.client.set_mock_output(serde_json::json!([ + [ + "I'll read that file for you", + { + "tool_use_id": "1", + "name": "fs_read", + "args": { + "operations": [ + { + "mode": "Line", + "path": "/sensitive.txt" + } + ] + } + } + ], + [ + "I understand the security policy blocked access to that file.", + ], + ])); + + // Create agent with blocking PreToolUse hook + let mut agents = Agents::default(); + let mut hooks = HashMap::new(); + + // Create a hook that blocks fs_read of sensitive files with exit code 2 + #[cfg(unix)] + let hook_command = "echo 'Security policy violation: cannot read sensitive files' >&2; exit 2"; + #[cfg(windows)] + let hook_command = "echo Security policy violation: cannot read sensitive files 1>&2 & exit /b 2"; + + hooks.insert(HookTrigger::PreToolUse, vec![Hook { + command: hook_command.to_string(), + timeout_ms: 5000, + max_output_size: 1024, + cache_ttl_seconds: 0, + matcher: Some("fs_read".to_string()), + source: crate::cli::agent::hook::Source::Agent, + }]); + + let agent = Agent { + name: "SecurityAgent".to_string(), + hooks, + ..Default::default() + }; + agents.agents.insert("SecurityAgent".to_string(), agent); + agents.switch("SecurityAgent").expect("Failed to switch agent"); + + let tool_manager = ToolManager::default(); + let tool_config = serde_json::from_str::>(include_str!("tools/tool_index.json")) + .expect("Tools failed to load"); + + // Run chat session - hook should block tool execution + let result = ChatSession::new( + &mut os, + std::io::stdout(), + std::io::stderr(), + "test_conv_id", + agents, + None, + InputSource::new_mock(vec![ + "read /sensitive.txt".to_string(), + "exit".to_string(), + ]), + false, + || Some(80), + tool_manager, + None, + tool_config, + true, + false, + None, + ) + .await + .unwrap() + .spawn(&mut os) + .await; + + // The session should complete successfully (hook blocks tool but doesn't crash) + assert!(result.is_ok(), "Chat session should complete successfully even when hook blocks tool"); + } + #[test] fn test_does_input_reference_file() { let tests = &[ diff --git a/crates/chat-cli/src/cli/chat/tool_manager.rs b/crates/chat-cli/src/cli/chat/tool_manager.rs index 7f72b874a..30c0473a7 100644 --- a/crates/chat-cli/src/cli/chat/tool_manager.rs +++ b/crates/chat-cli/src/cli/chat/tool_manager.rs @@ -99,8 +99,7 @@ use crate::util::MCP_SERVER_TOOL_DELIMITER; use crate::util::directories::home_dir; const NAMESPACE_DELIMITER: &str = "___"; -// This applies for both mcp server and tool name since in the end the tool name as seen by the -// model is just {server_name}{NAMESPACE_DELIMITER}{tool_name} +// This applies for both mcp server and tool name const VALID_TOOL_NAME: &str = "^[a-zA-Z][a-zA-Z0-9_]*$"; const SPINNER_CHARS: [char; 10] = ['⠋', '⠙', '⠹', '⠸', '⠼', '⠴', '⠦', '⠧', '⠇', '⠏']; @@ -873,7 +872,7 @@ impl ToolManager { "thinking" => Tool::Thinking(serde_json::from_value::(value.args).map_err(map_err)?), "knowledge" => Tool::Knowledge(serde_json::from_value::(value.args).map_err(map_err)?), "todo_list" => Tool::Todo(serde_json::from_value::(value.args).map_err(map_err)?), - // Note that this name is namespaced with server_name{DELIMITER}tool_name + // Note that this name is NO LONGER namespaced with server_name{DELIMITER}tool_name name => { // Note: tn_map also has tools that underwent no transformation. In otherwords, if // it is a valid tool name, we should get a hit. diff --git a/crates/chat-cli/src/cli/chat/tools/custom_tool.rs b/crates/chat-cli/src/cli/chat/tools/custom_tool.rs index 907f70d35..45b37b210 100644 --- a/crates/chat-cli/src/cli/chat/tools/custom_tool.rs +++ b/crates/chat-cli/src/cli/chat/tools/custom_tool.rs @@ -96,6 +96,11 @@ pub struct CustomTool { } impl CustomTool { + /// Returns the full tool name with server prefix in the format @server_name/tool_name + pub fn namespaced_tool_name(&self) -> String { + format!("@{}{}{}", self.server_name, MCP_SERVER_TOOL_DELIMITER, self.name) + } + pub async fn invoke(&self, _os: &Os, _updates: &mut impl Write) -> Result { let params = CallToolRequestParam { name: Cow::from(self.name.clone()), @@ -157,7 +162,6 @@ impl CustomTool { } pub fn eval_perm(&self, _os: &Os, agent: &Agent) -> PermissionEvalResult { - let Self { name: tool_name, .. } = self; let server_name = &self.server_name; let server_pattern = format!("@{server_name}"); @@ -165,7 +169,7 @@ impl CustomTool { return PermissionEvalResult::Allow; } - let tool_pattern = format!("@{server_name}{MCP_SERVER_TOOL_DELIMITER}{tool_name}"); + let tool_pattern = self.namespaced_tool_name(); if matches_any_pattern(&agent.allowed_tools, &tool_pattern) { return PermissionEvalResult::Allow; } diff --git a/crates/chat-cli/src/cli/chat/tools/introspect.rs b/crates/chat-cli/src/cli/chat/tools/introspect.rs index 9e42ec0f6..f14351ab5 100644 --- a/crates/chat-cli/src/cli/chat/tools/introspect.rs +++ b/crates/chat-cli/src/cli/chat/tools/introspect.rs @@ -71,6 +71,9 @@ impl Introspect { documentation.push_str("\n\n--- docs/todo-lists.md ---\n"); documentation.push_str(include_str!("../../../../../../docs/todo-lists.md")); + documentation.push_str("\n\n--- docs/hooks.md ---\n"); + documentation.push_str(include_str!("../../../../../../docs/hooks.md")); + documentation.push_str("\n\n--- changelog (from feed.json) ---\n"); // Include recent changelog entries from feed.json let feed = crate::cli::feed::Feed::load(); @@ -120,6 +123,8 @@ impl Introspect { ); documentation .push_str("• Todo Lists: https://github.com/aws/amazon-q-developer-cli/blob/main/docs/todo-lists.md\n"); + documentation + .push_str("• Hooks: https://github.com/aws/amazon-q-developer-cli/blob/main/docs/hooks.md\n"); documentation .push_str("• Contributing: https://github.com/aws/amazon-q-developer-cli/blob/main/CONTRIBUTING.md\n"); diff --git a/crates/chat-cli/src/cli/chat/tools/mod.rs b/crates/chat-cli/src/cli/chat/tools/mod.rs index 43ccb006c..96cc0d76f 100644 --- a/crates/chat-cli/src/cli/chat/tools/mod.rs +++ b/crates/chat-cli/src/cli/chat/tools/mod.rs @@ -271,6 +271,7 @@ pub struct QueuedTool { pub name: String, pub accepted: bool, pub tool: Tool, + pub tool_input: serde_json::Value, } /// The schema specification describing a tool's fields. diff --git a/docs/agent-format.md b/docs/agent-format.md index c707b476e..35adc8cfa 100644 --- a/docs/agent-format.md +++ b/docs/agent-format.md @@ -256,19 +256,37 @@ Resources can include: ## Hooks Field -The `hooks` field defines commands to run at specific trigger points. The output of these commands is added to the agent's context. +The `hooks` field defines commands to run at specific trigger points during agent lifecycle and tool execution. + +For detailed information about hook behavior, input/output formats, and examples, see the [Hooks documentation](hooks.md). ```json { "hooks": { "agentSpawn": [ { - "command": "git status", + "command": "git status" } ], "userPromptSubmit": [ { - "command": "ls -la", + "command": "ls -la" + } + ], + "preToolUse": [ + { + "matcher": "execute_bash", + "command": "{ echo \"$(date) - Bash command:\"; cat; echo; } >> /tmp/bash_audit_log" + }, + { + "matcher": "use_aws", + "command": "{ echo \"$(date) - AWS CLI call:\"; cat; echo; } >> /tmp/aws_audit_log" + } + ], + "postToolUse": [ + { + "matcher": "fs_write", + "command": "cargo fmt --all" } ] } @@ -277,10 +295,13 @@ The `hooks` field defines commands to run at specific trigger points. The output Each hook is defined with: - `command` (required): The command to execute +- `matcher` (optional): Pattern to match tool names for `preToolUse` and `postToolUse` hooks. See [built-in tools documentation](./built-in-tools.md) for available tool names. Available hook triggers: -- `agentSpawn`: Triggered when the agent is initialized -- `userPromptSubmit`: Triggered when the user submits a message +- `agentSpawn`: Triggered when the agent is initialized. +- `userPromptSubmit`: Triggered when the user submits a message. +- `preToolUse`: Triggered before a tool is executed. Can block the tool use. +- `postToolUse`: Triggered after a tool is executed. ## UseLegacyMcpJson Field diff --git a/docs/hooks.md b/docs/hooks.md new file mode 100644 index 000000000..4a0636a06 --- /dev/null +++ b/docs/hooks.md @@ -0,0 +1,161 @@ +# Hooks + +Hooks allow you to execute custom commands at specific points during agent lifecycle and tool execution. This enables security validation, logging, formatting, context gathering, and other custom behaviors. + +## Defining Hooks + +Hooks are defined in the agent configuration file. See the [agent format documentation](agent-format.md#hooks-field) for the complete syntax and examples. + +## Hook Event + +Hooks receive hook event in JSON format via STDIN: + +```json +{ + "hook_event_name": "agentSpawn", + "cwd": "/current/working/directory" +} +``` + +For tool-related hooks, additional fields are included: +- `tool_name`: Name of the tool being executed +- `tool_input`: Tool-specific parameters (see individual tool documentation) +- `tool_response`: Tool execution results (PostToolUse only) + +## Hook Output + +- **Exit code 0**: Hook succeeded. STDOUT is captured but not shown to user. +- **Exit code 2**: (PreToolUse only) Block tool execution. STDERR is returned to the LLM. +- **Other exit codes**: Hook failed. STDERR is shown as warning to user. + +## Tool Matching + +Use the `matcher` field to specify which tools the hook applies to: + +### Examples +- `"fs_write"` - Exact match for built-in tools +- `"fs_*"` - Wildcard pattern for built-in tools +- `"@git"` - All tools from git MCP server +- `"@git/status"` - Specific tool from git MCP server +- `"*"` - All tools (built-in and MCP) +- `"@builtin"` - All built-in tools only +- No matcher - Applies to all tools + +For complete tool reference format, see [agent format documentation](agent-format.md#tools-field). + +## Hook Types + +### AgentSpawn + +Runs when agent is activated. No tool context provided. + +**Hook Event** +```json +{ + "hook_event_name": "agentSpawn", + "cwd": "/current/working/directory" +} +``` + +**Exit Code Behavior:** +- **0**: Hook succeeded, STDOUT is added to agent's context +- **Other**: Show STDERR warning to user + +### UserPromptSubmit + +Runs when user submits a prompt. Output is added to conversation context. + +**Hook Event** +```json +{ + "hook_event_name": "userPromptSubmit", + "cwd": "/current/working/directory", + "prompt": "user's input prompt" +} +``` + +**Exit Code Behavior:** +- **0**: Hook succeeded, STDOUT is added to agent's context +- **Other**: Show STDERR warning to user + +### PreToolUse + +Runs before tool execution. Can validate and block tool usage. + +**Hook Event** +```json +{ + "hook_event_name": "preToolUse", + "cwd": "/current/working/directory", + "tool_name": "fs_read", + "tool_input": { + "operations": [ + { + "mode": "Line", + "path": "/current/working/directory/docs/hooks.md" + } + ] + } +} +``` + +**Exit Code Behavior:** +- **0**: Allow tool execution. +- **2**: Block tool execution, return STDERR to LLM. +- **Other**: Show STDERR warning to user, allow tool execution. + +### PostToolUse + +Runs after tool execution with access to tool results. + +**Hook Event** +```json +{ + "hook_event_name": "postToolUse", + "cwd": "/current/working/directory", + "tool_name": "fs_read", + "tool_input": { + "operations": [ + { + "mode": "Line", + "path": "/current/working/directory/docs/hooks.md" + } + ] + }, + "tool_response": { + "success": true, + "result": ["# Hooks\n\nHooks allow you to execute..."] + } +} +``` + +**Exit Code Behavior:** +- **0**: Hook succeeded. +- **Other**: Show STDERR warning to user. Tool already ran. + +### MCP Example + +For MCP tools, the tool name includes the full namespaced format including the MCP Server name: + +**Hook Event** +```json +{ + "hook_event_name": "preToolUse", + "cwd": "/current/working/directory", + "tool_name": "@postgres/query", + "tool_input": { + "sql": "SELECT * FROM orders LIMIT 10;" + } +} +``` + +## Timeout + +Default timeout is 30 seconds (30,000ms). Configure with `timeout_ms` field. + +## Caching + +Successfull hook results are cached based on `cache_ttl_seconds`: +- `0`: No caching (default) +- `> 0`: Cache successful results for specified seconds +- AgentSpawn hooks are never cached \ No newline at end of file From b37770e39410a7f2fea21312f067e016d5c40399 Mon Sep 17 00:00:00 2001 From: Felix Ding Date: Thu, 18 Sep 2025 09:48:17 -0700 Subject: [PATCH 07/14] fix(mcp): oauth issues (#2925) * fix incorrect scope for mcp oauth * reverts custom tool config enum change * fixes display task overriding sign in notice * updates schema --- crates/chat-cli/src/cli/chat/prompt.rs | 5 +- crates/chat-cli/src/cli/chat/tool_manager.rs | 1 + .../src/cli/chat/tools/custom_tool.rs | 21 ++++-- crates/chat-cli/src/cli/mcp.rs | 2 +- crates/chat-cli/src/mcp_client/client.rs | 40 +++++++---- crates/chat-cli/src/mcp_client/oauth_util.rs | 66 +++++++++++++++---- schemas/agent-v1.json | 21 ++++++ 7 files changed, 125 insertions(+), 31 deletions(-) diff --git a/crates/chat-cli/src/cli/chat/prompt.rs b/crates/chat-cli/src/cli/chat/prompt.rs index 06d449fdc..2ee6640ba 100644 --- a/crates/chat-cli/src/cli/chat/prompt.rs +++ b/crates/chat-cli/src/cli/chat/prompt.rs @@ -745,10 +745,7 @@ mod tests { let key_event = KeyEvent(KeyCode::Char(key), Modifiers::CTRL); // Try to bind and get the previous handler - let previous_handler = test_editor.bind_sequence( - key_event, - EventHandler::Simple(Cmd::Noop) - ); + let previous_handler = test_editor.bind_sequence(key_event, EventHandler::Simple(Cmd::Noop)); // If there was a previous handler, it means the key was already bound // (which could be our custom binding overriding Emacs) diff --git a/crates/chat-cli/src/cli/chat/tool_manager.rs b/crates/chat-cli/src/cli/chat/tool_manager.rs index 30c0473a7..6ef93bea2 100644 --- a/crates/chat-cli/src/cli/chat/tool_manager.rs +++ b/crates/chat-cli/src/cli/chat/tool_manager.rs @@ -1210,6 +1210,7 @@ fn spawn_display_task( terminal::Clear(terminal::ClearType::CurrentLine), )?; queue_oauth_message(&name, &mut output)?; + queue_init_message(spinner_logo_idx, complete, failed, total, &mut output)?; }, }, Err(_e) => { diff --git a/crates/chat-cli/src/cli/chat/tools/custom_tool.rs b/crates/chat-cli/src/cli/chat/tools/custom_tool.rs index 45b37b210..5457c9ee1 100644 --- a/crates/chat-cli/src/cli/chat/tools/custom_tool.rs +++ b/crates/chat-cli/src/cli/chat/tools/custom_tool.rs @@ -22,7 +22,10 @@ use crate::cli::agent::{ }; use crate::cli::chat::CONTINUATION_LINE; use crate::cli::chat::token_counter::TokenCounter; -use crate::mcp_client::RunningService; +use crate::mcp_client::{ + RunningService, + oauth_util, +}; use crate::os::Os; use crate::util::MCP_SERVER_TOOL_DELIMITER; use crate::util::pattern_matching::matches_any_pattern; @@ -43,17 +46,20 @@ impl Default for TransportType { } #[derive(Clone, Serialize, Deserialize, Debug, Eq, PartialEq, JsonSchema)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct CustomToolConfig { - /// The type of transport the mcp server is expecting. For http transport, only url (for now) - /// is taken into account. + /// The transport type to use for communication with the MCP server #[serde(default)] pub r#type: TransportType, - /// The URL endpoint for HTTP-based MCP servers + /// The URL for HTTP-based MCP server communication #[serde(default)] pub url: String, /// HTTP headers to include when communicating with HTTP-based MCP servers #[serde(default)] pub headers: HashMap, + /// Scopes with which oauth is done + #[serde(default = "get_default_scopes")] + pub oauth_scopes: Vec, /// The command string used to initialize the mcp server #[serde(default)] pub command: String, @@ -74,6 +80,13 @@ pub struct CustomToolConfig { pub is_from_legacy_mcp_json: bool, } +pub fn get_default_scopes() -> Vec { + oauth_util::get_default_scopes() + .iter() + .map(|s| (*s).to_string()) + .collect::>() +} + pub fn default_timeout() -> u64 { 120 * 1000 } diff --git a/crates/chat-cli/src/cli/mcp.rs b/crates/chat-cli/src/cli/mcp.rs index 0b709ef88..f0e8b9788 100644 --- a/crates/chat-cli/src/cli/mcp.rs +++ b/crates/chat-cli/src/cli/mcp.rs @@ -406,7 +406,7 @@ impl StatusArgs { style::Print(format!("Disabled: {}\n", cfg.disabled)), style::Print(format!( "Env Vars: {}\n", - cfg.env.as_ref().map_or_else( + cfg.env.map_or_else( || "(none)".into(), |e| e .iter() diff --git a/crates/chat-cli/src/mcp_client/client.rs b/crates/chat-cli/src/mcp_client/client.rs index 4156cf34a..b05ed3ad2 100644 --- a/crates/chat-cli/src/mcp_client/client.rs +++ b/crates/chat-cli/src/mcp_client/client.rs @@ -151,6 +151,8 @@ pub enum McpClientError { Parse(#[from] url::ParseError), #[error(transparent)] Auth(#[from] crate::auth::AuthError), + #[error("{0}")] + MalformedConfig(&'static str), } /// Decorates the method passed in with retry logic, but only if the [RunningService] has an @@ -336,7 +338,7 @@ impl McpClientService { HttpTransport::WithAuth((transport, mut auth_client)) => { // The crate does not automatically refresh tokens when they expire. We // would need to handle that here - let url = self.config.url.clone(); + let url = &backup_config.url; let service = match self.into_dyn().serve(transport).await.map_err(Box::new) { Ok(service) => service, Err(e) if matches!(*e, ClientInitializeError::ConnectionClosed(_)) => { @@ -344,12 +346,15 @@ impl McpClientService { let refresh_res = auth_client.refresh_token().await; let new_self = McpClientService::new( server_name.clone(), - backup_config, + backup_config.clone(), messenger_clone.clone(), ); + let scopes = &backup_config.oauth_scopes; + let timeout = backup_config.timeout; + let headers = &backup_config.headers; let new_transport = - get_http_transport(&os_clone, &url, Some(auth_client.auth_client.clone()), &*messenger_dup).await?; + get_http_transport(&os_clone, url, timeout, scopes, headers,Some(auth_client.auth_client.clone()), &*messenger_dup).await?; match new_transport { HttpTransport::WithAuth((new_transport, new_auth_client)) => { @@ -367,8 +372,8 @@ impl McpClientService { // again. We do this by deleting the cred // and discarding the client to trigger a full auth flow tokio::fs::remove_file(&auth_client.cred_full_path).await?; - let new_transport = - get_http_transport(&os_clone, &url, None, &*messenger_dup).await?; + let new_transport = + get_http_transport(&os_clone, url, timeout, scopes,headers,None, &*messenger_dup).await?; match new_transport { HttpTransport::WithAuth((new_transport, new_auth_client)) => { @@ -495,17 +500,32 @@ impl McpClientService { } async fn get_transport(&mut self, os: &Os, messenger: &dyn Messenger) -> Result { - // TODO: figure out what to do with headers let CustomToolConfig { - r#type: transport_type, + r#type, url, + headers, + oauth_scopes: scopes, command: command_as_str, args, env: config_envs, + timeout, .. } = &mut self.config; - match transport_type { + let is_malformed_http = matches!(r#type, TransportType::Http) && url.is_empty(); + let is_malformed_stdio = matches!(r#type, TransportType::Stdio) && command_as_str.is_empty(); + + if is_malformed_http { + return Err(McpClientError::MalformedConfig( + "MCP config is malformed: transport type is specified to be http but url is empty", + )); + } else if is_malformed_stdio { + return Err(McpClientError::MalformedConfig( + "MCP config is malformed: transport type is specified to be stdio but command is empty", + )); + } + + match r#type { TransportType::Stdio => { let expanded_cmd = canonicalizes_path(os, command_as_str)?; let command = Command::new(expanded_cmd).configure(|cmd| { @@ -525,7 +545,7 @@ impl McpClientService { Ok(Transport::Stdio((tokio_child_process, child_stderr))) }, TransportType::Http => { - let http_transport = get_http_transport(os, url, None, messenger).await?; + let http_transport = get_http_transport(os, url, *timeout, scopes, headers, None, messenger).await?; Ok(Transport::Http(http_transport)) }, @@ -562,7 +582,6 @@ impl McpClientService { async fn on_tool_list_changed(&self, context: NotificationContext) { let NotificationContext { peer, .. } = context; - let _timeout = self.config.timeout; paginated_fetch! { final_result_type: ListToolsResult, @@ -578,7 +597,6 @@ impl McpClientService { async fn on_prompt_list_changed(&self, context: NotificationContext) { let NotificationContext { peer, .. } = context; - let _timeout = self.config.timeout; paginated_fetch! { final_result_type: ListPromptsResult, diff --git a/crates/chat-cli/src/mcp_client/oauth_util.rs b/crates/chat-cli/src/mcp_client/oauth_util.rs index f284265cf..9c5319392 100644 --- a/crates/chat-cli/src/mcp_client/oauth_util.rs +++ b/crates/chat-cli/src/mcp_client/oauth_util.rs @@ -1,10 +1,14 @@ +use std::collections::HashMap; use std::net::SocketAddr; use std::path::PathBuf; use std::pin::Pin; use std::str::FromStr; use std::sync::Arc; -use http::StatusCode; +use http::{ + HeaderMap, + StatusCode, +}; use http_body_util::Full; use hyper::Response; use hyper::body::Bytes; @@ -69,6 +73,8 @@ pub enum OauthUtilError { Directory(#[from] DirectoryError), #[error(transparent)] Reqwest(#[from] reqwest::Error), + #[error("{0}")] + Http(String), #[error("Malformed directory")] MalformDirectory, #[error("Missing credential")] @@ -162,13 +168,16 @@ pub enum HttpTransport { WithoutAuth(WorkerTransport>), } -fn get_scopes() -> &'static [&'static str] { - &["openid", "mcp", "email", "profile"] +pub fn get_default_scopes() -> &'static [&'static str] { + &["openid", "email", "profile", "offline_access"] } pub async fn get_http_transport( os: &Os, url: &str, + timeout: u64, + scopes: &[String], + headers: &HashMap, auth_client: Option>, messenger: &dyn Messenger, ) -> Result { @@ -178,7 +187,13 @@ pub async fn get_http_transport( let cred_full_path = cred_dir.join(format!("{key}.token.json")); let reg_full_path = cred_dir.join(format!("{key}.registration.json")); - let reqwest_client = reqwest::Client::default(); + let mut client_builder = reqwest::ClientBuilder::new().timeout(std::time::Duration::from_millis(timeout)); + if !headers.is_empty() { + let headers = HeaderMap::try_from(headers).map_err(|e| OauthUtilError::Http(e.to_string()))?; + client_builder = client_builder.default_headers(headers); + }; + let reqwest_client = client_builder.build()?; + let probe_resp = reqwest_client.get(url.clone()).send().await?; match probe_resp.status() { StatusCode::UNAUTHORIZED | StatusCode::FORBIDDEN => { @@ -186,8 +201,14 @@ pub async fn get_http_transport( let auth_client = match auth_client { Some(auth_client) => auth_client, None => { - let am = - get_auth_manager(url.clone(), cred_full_path.clone(), reg_full_path.clone(), messenger).await?; + let am = get_auth_manager( + url.clone(), + cred_full_path.clone(), + reg_full_path.clone(), + scopes, + messenger, + ) + .await?; AuthClient::new(reqwest_client, am) }, }; @@ -204,7 +225,12 @@ pub async fn get_http_transport( Ok(HttpTransport::WithAuth((transport, auth_dg))) }, _ => { - let transport = StreamableHttpClientTransport::from_uri(url.as_str()); + let transport = + StreamableHttpClientTransport::with_client(reqwest_client, StreamableHttpClientTransportConfig { + uri: url.as_str().into(), + allow_stateless: false, + ..Default::default() + }); Ok(HttpTransport::WithoutAuth(transport)) }, @@ -215,6 +241,7 @@ async fn get_auth_manager( url: Url, cred_full_path: PathBuf, reg_full_path: PathBuf, + scopes: &[String], messenger: &dyn Messenger, ) -> Result { let cred_as_bytes = tokio::fs::read(&cred_full_path).await; @@ -237,7 +264,7 @@ async fn get_auth_manager( _ => { info!("Error reading cached credentials"); debug!("## mcp: cache read failed. constructing auth manager from scratch"); - let (am, redirect_uri) = get_auth_manager_impl(oauth_state, messenger).await?; + let (am, redirect_uri) = get_auth_manager_impl(oauth_state, scopes, messenger).await?; // Client registration is done in [start_authorization] // If we have gotten past that point that means we have the info to persist the @@ -246,7 +273,10 @@ async fn get_auth_manager( let reg = Registration { client_id, client_secret: None, - scopes: get_scopes().iter().map(|s| (*s).to_string()).collect::>(), + scopes: get_default_scopes() + .iter() + .map(|s| (*s).to_string()) + .collect::>(), redirect_uri, }; let reg_as_str = serde_json::to_string_pretty(®)?; @@ -268,6 +298,7 @@ async fn get_auth_manager( async fn get_auth_manager_impl( mut oauth_state: OAuthState, + scopes: &[String], messenger: &dyn Messenger, ) -> Result<(AuthorizationManager, String), OauthUtilError> { let socket_addr = SocketAddr::from(([127, 0, 0, 1], 0)); @@ -278,7 +309,9 @@ async fn get_auth_manager_impl( info!("Listening on local host port {:?} for oauth", actual_addr); let redirect_uri = format!("http://{}", actual_addr); - oauth_state.start_authorization(get_scopes(), &redirect_uri).await?; + let scopes_as_str = scopes.iter().map(String::as_str).collect::>(); + let scopes_as_slice = scopes_as_str.as_slice(); + oauth_state.start_authorization(scopes_as_slice, &redirect_uri).await?; let auth_url = oauth_state.get_authorization_url().await?; _ = messenger.send_oauth_link(auth_url).await; @@ -333,9 +366,19 @@ async fn make_svc( let query = uri.query().unwrap_or(""); let params: std::collections::HashMap = url::form_urlencoded::parse(query.as_bytes()).into_owned().collect(); + debug!("## mcp: uri: {}, query: {}, params: {:?}", uri, query, params); let self_clone = self.clone(); Box::pin(async move { + let error = params.get("error"); + let resp = if let Some(err) = error { + mk_response(format!( + "Oauth failed. Check url for precise reasons. Possible reasons: {err}.\nIf this is scope related. You can try configuring the server scopes to be an empty array via adding oauth_scopes: []" + )) + } else { + mk_response("You can close this page now".to_string()) + }; + let code = params.get("code").cloned().unwrap_or_default(); if let Some(sender) = self_clone .one_shot_sender @@ -345,7 +388,8 @@ async fn make_svc( { sender.send(code).map_err(LoopBackError::Send)?; } - mk_response("You can close this page now".to_string()) + + resp }) } } diff --git a/schemas/agent-v1.json b/schemas/agent-v1.json index c7b63492d..5e72b0847 100644 --- a/schemas/agent-v1.json +++ b/schemas/agent-v1.json @@ -73,6 +73,27 @@ "type": "string", "default": "" }, + "headers": { + "description": "HTTP headers to include when communicating with HTTP-based MCP servers", + "type": "object", + "additionalProperties": { + "type": "string" + }, + "default": {} + }, + "oauthScopes": { + "description": "Scopes with which oauth is done", + "type": "array", + "items": { + "type": "string" + }, + "default": [ + "openid", + "email", + "profile", + "offline_access" + ] + }, "command": { "description": "The command string used to initialize the mcp server", "type": "string", From 102bd0f96f3efe00d6da603722dcb68dff9e89e8 Mon Sep 17 00:00:00 2001 From: nirajchowdhary <226941436+nirajchowdhary@users.noreply.github.com> Date: Thu, 18 Sep 2025 11:01:52 -0700 Subject: [PATCH 08/14] fix(chat): reset pending tool state when clearing conversation (#2855) Reset tool_uses, pending_tool_index, and tool_turn_start_time to prevent orphaned tool approval prompts after conversation history is cleared. Co-authored-by: Niraj Chowdhary --- crates/chat-cli/src/cli/chat/cli/clear.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/crates/chat-cli/src/cli/chat/cli/clear.rs b/crates/chat-cli/src/cli/chat/cli/clear.rs index b31bccd28..de274e2c0 100644 --- a/crates/chat-cli/src/cli/chat/cli/clear.rs +++ b/crates/chat-cli/src/cli/chat/cli/clear.rs @@ -52,6 +52,12 @@ impl ClearArgs { if let Some(cm) = session.conversation.context_manager.as_mut() { cm.hook_executor.cache.clear(); } + + // Reset pending tool state to prevent orphaned tool approval prompts + session.tool_uses.clear(); + session.pending_tool_index = None; + session.tool_turn_start_time = None; + execute!( session.stderr, style::SetForegroundColor(Color::Green), From 4b437f6741f8995aff0459a8098a8013139b5589 Mon Sep 17 00:00:00 2001 From: kkashilk <93673379+kkashilk@users.noreply.github.com> Date: Thu, 18 Sep 2025 11:46:47 -0700 Subject: [PATCH 09/14] Trim region to avoid login failures (#2930) --- crates/chat-cli/src/cli/chat/cli/hooks.rs | 139 ++++++++++-------- crates/chat-cli/src/cli/chat/context.rs | 6 +- crates/chat-cli/src/cli/chat/conversation.rs | 28 +++- crates/chat-cli/src/cli/chat/mod.rs | 138 +++++++++-------- .../chat-cli/src/cli/chat/tools/introspect.rs | 3 +- crates/chat-cli/src/cli/user.rs | 2 +- docs/hooks.md | 2 +- 7 files changed, 185 insertions(+), 133 deletions(-) diff --git a/crates/chat-cli/src/cli/chat/cli/hooks.rs b/crates/chat-cli/src/cli/chat/cli/hooks.rs index 96e1e842d..05baee5bb 100644 --- a/crates/chat-cli/src/cli/chat/cli/hooks.rs +++ b/crates/chat-cli/src/cli/chat/cli/hooks.rs @@ -38,7 +38,6 @@ use crate::cli::agent::hook::{ HookTrigger, }; use crate::cli::agent::is_mcp_tool_ref; -use crate::util::MCP_SERVER_TOOL_DELIMITER; use crate::cli::chat::consts::AGENT_FORMAT_HOOKS_DOC_URL; use crate::cli::chat::util::truncate_safe; use crate::cli::chat::{ @@ -46,6 +45,7 @@ use crate::cli::chat::{ ChatSession, ChatState, }; +use crate::util::MCP_SERVER_TOOL_DELIMITER; use crate::util::pattern_matching::matches_any_pattern; /// Hook execution result: (exit_code, output) @@ -58,26 +58,29 @@ fn hook_matches_tool(hook: &Hook, tool_name: &str) -> bool { None => true, // No matcher means the hook runs for all tools Some(pattern) => { match pattern.as_str() { - "*" => true, // Wildcard matches all tools + "*" => true, // Wildcard matches all tools "@builtin" => !is_mcp_tool_ref(tool_name), // Built-in tools are not MCP tools _ => { // If tool_name is MCP, check server pattern first if is_mcp_tool_ref(tool_name) { - if let Some(server_name) = tool_name.strip_prefix('@').and_then(|s| s.split(MCP_SERVER_TOOL_DELIMITER).next()) { + if let Some(server_name) = tool_name + .strip_prefix('@') + .and_then(|s| s.split(MCP_SERVER_TOOL_DELIMITER).next()) + { let server_pattern = format!("@{}", server_name); if pattern == &server_pattern { return true; } } } - + // Use matches_any_pattern for both MCP and built-in tools let mut patterns = std::collections::HashSet::new(); patterns.insert(pattern.clone()); matches_any_pattern(&patterns, tool_name) - } + }, } - } + }, } } @@ -133,7 +136,7 @@ impl HookExecutor { continue; // Skip this hook - doesn't match tool } } - + if let Some(cache) = self.get_cache(&hook) { // Note: we only cache successful hook run. hence always using 0 as exit code for cached hook cached.push((hook.clone(), (0, cache))); @@ -203,7 +206,11 @@ impl HookExecutor { style::Print(&hook.1.command), style::Print("\""), style::SetForegroundColor(style::Color::Red), - style::Print(format!(" failed with exit code: {}, stderr: {})\n", exit_code, hook_output.trim_end())), + style::Print(format!( + " failed with exit code: {}, stderr: {})\n", + exit_code, + hook_output.trim_end() + )), style::ResetColor, )?; } else { @@ -437,11 +444,16 @@ impl HooksArgs { #[cfg(test)] mod tests { - use super::*; use std::collections::HashMap; - use crate::cli::agent::hook::{Hook, HookTrigger}; + use tempfile::TempDir; + use super::*; + use crate::cli::agent::hook::{ + Hook, + HookTrigger, + }; + #[test] fn test_hook_matches_tool() { let hook_no_matcher = Hook { @@ -452,7 +464,7 @@ mod tests { matcher: None, source: crate::cli::agent::hook::Source::Session, }; - + let fs_write_hook = Hook { command: "echo test".to_string(), timeout_ms: 5000, @@ -461,7 +473,7 @@ mod tests { matcher: Some("fs_write".to_string()), source: crate::cli::agent::hook::Source::Session, }; - + let fs_wildcard_hook = Hook { command: "echo test".to_string(), timeout_ms: 5000, @@ -470,7 +482,7 @@ mod tests { matcher: Some("fs_*".to_string()), source: crate::cli::agent::hook::Source::Session, }; - + let all_tools_hook = Hook { command: "echo test".to_string(), timeout_ms: 5000, @@ -479,7 +491,7 @@ mod tests { matcher: Some("*".to_string()), source: crate::cli::agent::hook::Source::Session, }; - + let builtin_hook = Hook { command: "echo test".to_string(), timeout_ms: 5000, @@ -488,7 +500,7 @@ mod tests { matcher: Some("@builtin".to_string()), source: crate::cli::agent::hook::Source::Session, }; - + let git_server_hook = Hook { command: "echo test".to_string(), timeout_ms: 5000, @@ -497,7 +509,7 @@ mod tests { matcher: Some("@git".to_string()), source: crate::cli::agent::hook::Source::Session, }; - + let git_status_hook = Hook { command: "echo test".to_string(), timeout_ms: 5000, @@ -506,36 +518,36 @@ mod tests { matcher: Some("@git/status".to_string()), source: crate::cli::agent::hook::Source::Session, }; - + // No matcher should match all tools assert!(hook_matches_tool(&hook_no_matcher, "fs_write")); assert!(hook_matches_tool(&hook_no_matcher, "execute_bash")); assert!(hook_matches_tool(&hook_no_matcher, "@git/status")); - + // Exact matcher should only match exact tool assert!(hook_matches_tool(&fs_write_hook, "fs_write")); assert!(!hook_matches_tool(&fs_write_hook, "fs_read")); - + // Wildcard matcher should match pattern assert!(hook_matches_tool(&fs_wildcard_hook, "fs_write")); assert!(hook_matches_tool(&fs_wildcard_hook, "fs_read")); assert!(!hook_matches_tool(&fs_wildcard_hook, "execute_bash")); - + // * should match all tools assert!(hook_matches_tool(&all_tools_hook, "fs_write")); assert!(hook_matches_tool(&all_tools_hook, "execute_bash")); assert!(hook_matches_tool(&all_tools_hook, "@git/status")); - + // @builtin should match built-in tools only assert!(hook_matches_tool(&builtin_hook, "fs_write")); assert!(hook_matches_tool(&builtin_hook, "execute_bash")); assert!(!hook_matches_tool(&builtin_hook, "@git/status")); - + // @git should match all git server tools assert!(hook_matches_tool(&git_server_hook, "@git/status")); assert!(!hook_matches_tool(&git_server_hook, "@other/tool")); assert!(!hook_matches_tool(&git_server_hook, "fs_write")); - + // @git/status should match exact MCP tool assert!(hook_matches_tool(&git_status_hook, "@git/status")); assert!(!hook_matches_tool(&git_status_hook, "@git/commit")); @@ -546,18 +558,18 @@ mod tests { async fn test_hook_executor_with_tool_context() { let mut executor = HookExecutor::new(); let mut output = Vec::new(); - + // Create temp directory and file let temp_dir = TempDir::new().unwrap(); let test_file = temp_dir.path().join("hook_output.json"); let test_file_str = test_file.to_string_lossy(); - + // Create a simple hook that writes JSON input to a file #[cfg(unix)] let command = format!("cat > {}", test_file_str); #[cfg(windows)] let command = format!("type > {}", test_file_str); - + let hook = Hook { command, timeout_ms: 5000, @@ -566,10 +578,10 @@ mod tests { matcher: Some("fs_write".to_string()), source: crate::cli::agent::hook::Source::Session, }; - + let mut hooks = HashMap::new(); hooks.insert(HookTrigger::PreToolUse, vec![hook]); - + let tool_context = ToolContext { tool_name: "fs_write".to_string(), tool_input: serde_json::json!({ @@ -578,18 +590,14 @@ mod tests { }), tool_response: None, }; - + // Run the hook - let result = executor.run_hooks( - hooks, - &mut output, - ".", - None, - Some(tool_context) - ).await; - + let result = executor + .run_hooks(hooks, &mut output, ".", None, Some(tool_context)) + .await; + assert!(result.is_ok()); - + // Verify the hook wrote the JSON input to the file if let Ok(content) = std::fs::read_to_string(&test_file) { let json: serde_json::Value = serde_json::from_str(&content).unwrap(); @@ -605,7 +613,7 @@ mod tests { async fn test_hook_filtering_no_match() { let mut executor = HookExecutor::new(); let mut output = Vec::new(); - + // Hook that matches execute_bash (should NOT run for fs_write tool call) let execute_bash_hook = Hook { command: "echo 'should not run'".to_string(), @@ -615,31 +623,33 @@ mod tests { matcher: Some("execute_bash".to_string()), source: crate::cli::agent::hook::Source::Session, }; - + let mut hooks = HashMap::new(); hooks.insert(HookTrigger::PostToolUse, vec![execute_bash_hook]); - + let tool_context = ToolContext { tool_name: "fs_write".to_string(), tool_input: serde_json::json!({"command": "create"}), tool_response: Some(serde_json::json!({"success": true})), }; - + // Run the hooks - let result = executor.run_hooks( - hooks, - &mut output, - ".", // cwd - using current directory for now - None, // prompt - no user prompt for this test - Some(tool_context) - ).await; - + let result = executor + .run_hooks( + hooks, + &mut output, + ".", // cwd - using current directory for now + None, // prompt - no user prompt for this test + Some(tool_context), + ) + .await; + assert!(result.is_ok()); let hook_results = result.unwrap(); - + // Should run 0 hooks because matcher doesn't match tool_name assert_eq!(hook_results.len(), 0); - + // Output should be empty since no hooks ran assert!(output.is_empty()); } @@ -654,7 +664,7 @@ mod tests { let command = "echo 'Tool execution blocked by security policy' >&2; exit 2"; #[cfg(windows)] let command = "echo Tool execution blocked by security policy 1>&2 & exit /b 2"; - + let hook = Hook { command: command.to_string(), timeout_ms: 5000, @@ -664,9 +674,7 @@ mod tests { source: crate::cli::agent::hook::Source::Session, }; - let hooks = HashMap::from([ - (HookTrigger::PreToolUse, vec![hook]) - ]); + let hooks = HashMap::from([(HookTrigger::PreToolUse, vec![hook])]); let tool_context = ToolContext { tool_name: "fs_write".to_string(), @@ -677,17 +685,20 @@ mod tests { tool_response: None, }; - let results = executor.run_hooks( - hooks, - &mut output, - ".", // cwd - None, // prompt - Some(tool_context) - ).await.unwrap(); + let results = executor + .run_hooks( + hooks, + &mut output, + ".", // cwd + None, // prompt + Some(tool_context), + ) + .await + .unwrap(); // Should have one result assert_eq!(results.len(), 1); - + let ((trigger, _hook), (exit_code, hook_output)) = &results[0]; assert_eq!(*trigger, HookTrigger::PreToolUse); assert_eq!(*exit_code, 2); diff --git a/crates/chat-cli/src/cli/chat/context.rs b/crates/chat-cli/src/cli/chat/context.rs index fa39f8039..89edfb47a 100644 --- a/crates/chat-cli/src/cli/chat/context.rs +++ b/crates/chat-cli/src/cli/chat/context.rs @@ -14,9 +14,9 @@ use serde::{ Serializer, }; +use super::cli::hooks::HookOutput; use super::cli::model::context_window_tokens; use super::util::drop_matched_context_files; -use super::cli::hooks::HookOutput; use crate::cli::agent::Agent; use crate::cli::agent::hook::{ Hook, @@ -255,7 +255,9 @@ impl ContextManager { let mut hooks = self.hooks.clone(); hooks.retain(|t, _| *t == trigger); let cwd = os.env.current_dir()?.to_string_lossy().to_string(); - self.hook_executor.run_hooks(hooks, output, &cwd, prompt, tool_context).await + self.hook_executor + .run_hooks(hooks, output, &cwd, prompt, tool_context) + .await } } diff --git a/crates/chat-cli/src/cli/chat/conversation.rs b/crates/chat-cli/src/cli/chat/conversation.rs index 917969780..3d064c018 100644 --- a/crates/chat-cli/src/cli/chat/conversation.rs +++ b/crates/chat-cli/src/cli/chat/conversation.rs @@ -564,12 +564,26 @@ impl ConversationState { let mut agent_spawn_context = None; if let Some(cm) = self.context_manager.as_mut() { let user_prompt = self.next_message.as_ref().and_then(|m| m.prompt()); - let agent_spawn = cm.run_hooks(HookTrigger::AgentSpawn, output, os, user_prompt, None /* tool_context */).await?; + let agent_spawn = cm + .run_hooks( + HookTrigger::AgentSpawn, + output, + os, + user_prompt, + None, // tool_context + ) + .await?; agent_spawn_context = format_hook_context(&agent_spawn, HookTrigger::AgentSpawn); if let (true, Some(next_message)) = (run_perprompt_hooks, self.next_message.as_mut()) { let per_prompt = cm - .run_hooks(HookTrigger::UserPromptSubmit, output, os, next_message.prompt(), None /* tool_context */) + .run_hooks( + HookTrigger::UserPromptSubmit, + output, + os, + next_message.prompt(), + None, // tool_context + ) .await?; if let Some(ctx) = format_hook_context(&per_prompt, HookTrigger::UserPromptSubmit) { next_message.additional_context = ctx; @@ -1033,7 +1047,10 @@ impl From for ToolInputSchema { /// [Option::None] fn format_hook_context(hook_results: &[((HookTrigger, Hook), HookOutput)], trigger: HookTrigger) -> Option { // Note: only format context when hook command exit code is 0 - if hook_results.iter().all(|(_, (exit_code, content))| *exit_code != 0 || content.is_empty()) { + if hook_results + .iter() + .all(|(_, (exit_code, content))| *exit_code != 0 || content.is_empty()) + { return None; } @@ -1046,7 +1063,10 @@ fn format_hook_context(hook_results: &[((HookTrigger, Hook), HookOutput)], trigg } context_content.push_str("\n\n"); - for (_, (_, output)) in hook_results.iter().filter(|((h_trigger, _), (exit_code, _))| *h_trigger == trigger && *exit_code == 0) { + for (_, (_, output)) in hook_results + .iter() + .filter(|((h_trigger, _), (exit_code, _))| *h_trigger == trigger && *exit_code == 0) + { context_content.push_str(&format!("{output}\n\n")); } context_content.push_str(CONTEXT_ENTRY_END_HEADER); diff --git a/crates/chat-cli/src/cli/chat/mod.rs b/crates/chat-cli/src/cli/chat/mod.rs index 2da6b3d5c..e15509945 100644 --- a/crates/chat-cli/src/cli/chat/mod.rs +++ b/crates/chat-cli/src/cli/chat/mod.rs @@ -2432,37 +2432,42 @@ impl ChatSession { if let Some(cm) = self.conversation.context_manager.as_mut() { for result in &tool_results { if let Some(tool) = self.tool_uses.iter().find(|t| t.id == result.tool_use_id) { - let content: Vec = result.content.iter().map(|block| { - match block { + let content: Vec = result + .content + .iter() + .map(|block| match block { ToolUseResultBlock::Text(text) => serde_json::Value::String(text.clone()), ToolUseResultBlock::Json(json) => json.clone(), - } - }).collect(); - + }) + .collect(); + let tool_response = match result.status { ToolResultStatus::Success => serde_json::json!({"success": true, "result": content}), ToolResultStatus::Error => serde_json::json!({"success": false, "error": content}), }; - + let tool_context = ToolContext { tool_name: match &tool.tool { - Tool::Custom(custom_tool) => custom_tool.namespaced_tool_name(), // for MCP tool, pass MCP name to the hook + Tool::Custom(custom_tool) => custom_tool.namespaced_tool_name(), /* for MCP tool, pass MCP name to the hook */ _ => tool.name.clone(), }, tool_input: tool.tool_input.clone(), tool_response: Some(tool_response), }; - + // Here is how we handle postToolUse output: - // Exit code is 0: nothing. stdout is not shown to user. We don't support processing the PostToolUse hook output yet. - // Exit code is non-zero: display an error to user (already taken care of by the ContextManager.run_hooks) - let _ = cm.run_hooks( - crate::cli::agent::hook::HookTrigger::PostToolUse, - &mut std::io::stderr(), - os, - None, - Some(tool_context) - ).await; + // Exit code is 0: nothing. stdout is not shown to user. We don't support processing the PostToolUse + // hook output yet. Exit code is non-zero: display an error to user (already + // taken care of by the ContextManager.run_hooks) + let _ = cm + .run_hooks( + crate::cli::agent::hook::HookTrigger::PostToolUse, + &mut std::io::stderr(), + os, + None, + Some(tool_context), + ) + .await; } } } @@ -2802,7 +2807,8 @@ impl ChatSession { } } - // Validate the tool use request from LLM, including basic checks like fs_read file should exist, as well as user-defined preToolUse hook check. + // Validate the tool use request from LLM, including basic checks like fs_read file should exist, as + // well as user-defined preToolUse hook check. async fn validate_tools(&mut self, os: &Os, tool_uses: Vec) -> Result { let conv_id = self.conversation.conversation_id().to_owned(); debug!(?tool_uses, "Validating tool uses"); @@ -2907,38 +2913,44 @@ impl ChatSession { } // Execute PreToolUse hooks for all validated tools - // The mental model is preToolHook is like validate tools, but its behavior can be customized by user - // Note that after preTookUse hook, user can still reject the took run + // The mental model is preToolHook is like validate tools, but its behavior can be customized by + // user Note that after preTookUse hook, user can still reject the took run if let Some(cm) = self.conversation.context_manager.as_mut() { for tool in &queued_tools { let tool_context = ToolContext { tool_name: match &tool.tool { - Tool::Custom(custom_tool) => custom_tool.namespaced_tool_name(), // for MCP tool, pass MCP name to the hook + Tool::Custom(custom_tool) => custom_tool.namespaced_tool_name(), // for MCP tool, pass MCP + // name to the hook _ => tool.name.clone(), }, tool_input: tool.tool_input.clone(), tool_response: None, }; - - let hook_results = cm.run_hooks( - crate::cli::agent::hook::HookTrigger::PreToolUse, - &mut std::io::stderr(), - os, - None, /* prompt */ - Some(tool_context) - ).await?; + + let hook_results = cm + .run_hooks( + crate::cli::agent::hook::HookTrigger::PreToolUse, + &mut std::io::stderr(), + os, + None, // prompt + Some(tool_context), + ) + .await?; // Here is how we handle the preToolUse hook output: // Exit code is 0: nothing. stdout is not shown to user. // Exit code is 2: block the tool use. return stderr to LLM. show warning to user // Other error: show warning to user. - + // Check for exit code 2 and add to tool_results for (_, (exit_code, output)) in &hook_results { if *exit_code == 2 { tool_results.push(ToolUseResult { tool_use_id: tool.id.clone(), - content: vec![ToolUseResultBlock::Text(format!("PreToolHook blocked the tool execution: {}", output))], + content: vec![ToolUseResultBlock::Text(format!( + "PreToolHook blocked the tool execution: {}", + output + ))], status: ToolResultStatus::Error, }); } @@ -2974,7 +2986,7 @@ impl ChatSession { self.tool_uses = queued_tools; self.pending_tool_index = Some(0); self.tool_turn_start_time = Some(Instant::now()); - + Ok(ChatState::ExecuteTools) } @@ -3897,14 +3909,18 @@ mod tests { } // Integration test for PreToolUse hook functionality. - // - // In this integration test we create a preToolUse hook that logs tool info into a file + // + // In this integration test we create a preToolUse hook that logs tool info into a file // and we run fs_read and verify the log is generated with the correct ToolContext data. #[tokio::test] async fn test_tool_hook_integration() { - use crate::cli::agent::hook::{Hook, HookTrigger}; use std::collections::HashMap; + use crate::cli::agent::hook::{ + Hook, + HookTrigger, + }; + let mut os = Os::new().await.unwrap(); os.client.set_mock_output(serde_json::json!([ [ @@ -3935,13 +3951,13 @@ mod tests { // Create agent with PreToolUse and PostToolUse hooks let mut agents = Agents::default(); let mut hooks = HashMap::new(); - + // Get the real path in the temp directory for the hooks to write to let pre_hook_log_path = os.fs.chroot_path_str("/pre-hook-test.log"); let post_hook_log_path = os.fs.chroot_path_str("/post-hook-test.log"); let pre_hook_command = format!("cat > {}", pre_hook_log_path); let post_hook_command = format!("cat > {}", post_hook_log_path); - + hooks.insert(HookTrigger::PreToolUse, vec![Hook { command: pre_hook_command, timeout_ms: 5000, @@ -4002,16 +4018,16 @@ mod tests { // Verify the PreToolUse hook was called if let Ok(pre_log_content) = os.fs.read_to_string("/pre-hook-test.log").await { - let pre_hook_data: serde_json::Value = serde_json::from_str(&pre_log_content) - .expect("PreToolUse hook output should be valid JSON"); - + let pre_hook_data: serde_json::Value = + serde_json::from_str(&pre_log_content).expect("PreToolUse hook output should be valid JSON"); + assert_eq!(pre_hook_data["hook_event_name"], "preToolUse"); assert_eq!(pre_hook_data["tool_name"], "fs_read"); assert_eq!(pre_hook_data["tool_response"], serde_json::Value::Null); - + let tool_input = &pre_hook_data["tool_input"]; assert!(tool_input["operations"].is_array()); - + println!("✓ PreToolUse hook validation passed: {}", pre_log_content); } else { panic!("PreToolUse hook log file not found - hook may not have been called"); @@ -4019,22 +4035,22 @@ mod tests { // Verify the PostToolUse hook was called if let Ok(post_log_content) = os.fs.read_to_string("/post-hook-test.log").await { - let post_hook_data: serde_json::Value = serde_json::from_str(&post_log_content) - .expect("PostToolUse hook output should be valid JSON"); - + let post_hook_data: serde_json::Value = + serde_json::from_str(&post_log_content).expect("PostToolUse hook output should be valid JSON"); + assert_eq!(post_hook_data["hook_event_name"], "postToolUse"); assert_eq!(post_hook_data["tool_name"], "fs_read"); - + // Validate tool_response structure for successful execution let tool_response = &post_hook_data["tool_response"]; assert_eq!(tool_response["success"], true); assert!(tool_response["result"].is_array()); - + let result_blocks = tool_response["result"].as_array().unwrap(); assert!(!result_blocks.is_empty()); let content = result_blocks[0].as_str().unwrap(); assert!(content.contains("line1\nline2\nline3")); - + println!("✓ PostToolUse hook validation passed: {}", post_log_content); } else { panic!("PostToolUse hook log file not found - hook may not have been called"); @@ -4043,20 +4059,24 @@ mod tests { #[tokio::test] async fn test_pretool_hook_blocking_integration() { - use crate::cli::agent::hook::{Hook, HookTrigger}; use std::collections::HashMap; + use crate::cli::agent::hook::{ + Hook, + HookTrigger, + }; + let mut os = Os::new().await.unwrap(); - + // Create a test file to read os.fs.write("/sensitive.txt", "classified information").await.unwrap(); - + // Mock LLM responses: first tries fs_read, gets blocked, then responds to error os.client.set_mock_output(serde_json::json!([ [ "I'll read that file for you", { - "tool_use_id": "1", + "tool_use_id": "1", "name": "fs_read", "args": { "operations": [ @@ -4076,13 +4096,13 @@ mod tests { // Create agent with blocking PreToolUse hook let mut agents = Agents::default(); let mut hooks = HashMap::new(); - + // Create a hook that blocks fs_read of sensitive files with exit code 2 #[cfg(unix)] let hook_command = "echo 'Security policy violation: cannot read sensitive files' >&2; exit 2"; #[cfg(windows)] let hook_command = "echo Security policy violation: cannot read sensitive files 1>&2 & exit /b 2"; - + hooks.insert(HookTrigger::PreToolUse, vec![Hook { command: hook_command.to_string(), timeout_ms: 5000, @@ -4112,10 +4132,7 @@ mod tests { "test_conv_id", agents, None, - InputSource::new_mock(vec![ - "read /sensitive.txt".to_string(), - "exit".to_string(), - ]), + InputSource::new_mock(vec!["read /sensitive.txt".to_string(), "exit".to_string()]), false, || Some(80), tool_manager, @@ -4131,7 +4148,10 @@ mod tests { .await; // The session should complete successfully (hook blocks tool but doesn't crash) - assert!(result.is_ok(), "Chat session should complete successfully even when hook blocks tool"); + assert!( + result.is_ok(), + "Chat session should complete successfully even when hook blocks tool" + ); } #[test] diff --git a/crates/chat-cli/src/cli/chat/tools/introspect.rs b/crates/chat-cli/src/cli/chat/tools/introspect.rs index f14351ab5..4968cd8e9 100644 --- a/crates/chat-cli/src/cli/chat/tools/introspect.rs +++ b/crates/chat-cli/src/cli/chat/tools/introspect.rs @@ -123,8 +123,7 @@ impl Introspect { ); documentation .push_str("• Todo Lists: https://github.com/aws/amazon-q-developer-cli/blob/main/docs/todo-lists.md\n"); - documentation - .push_str("• Hooks: https://github.com/aws/amazon-q-developer-cli/blob/main/docs/hooks.md\n"); + documentation.push_str("• Hooks: https://github.com/aws/amazon-q-developer-cli/blob/main/docs/hooks.md\n"); documentation .push_str("• Contributing: https://github.com/aws/amazon-q-developer-cli/blob/main/CONTRIBUTING.md\n"); diff --git a/crates/chat-cli/src/cli/user.rs b/crates/chat-cli/src/cli/user.rs index 50e761b9b..9394746c2 100644 --- a/crates/chat-cli/src/cli/user.rs +++ b/crates/chat-cli/src/cli/user.rs @@ -118,7 +118,7 @@ impl LoginArgs { }; let start_url = input("Enter Start URL", default_start_url.as_deref())?; - let region = input("Enter Region", default_region.as_deref())?; + let region = input("Enter Region", default_region.as_deref())?.trim().to_string(); let _ = os.database.set_start_url(start_url.clone()); let _ = os.database.set_idc_region(region.clone()); diff --git a/docs/hooks.md b/docs/hooks.md index 4a0636a06..d7cfa3d50 100644 --- a/docs/hooks.md +++ b/docs/hooks.md @@ -155,7 +155,7 @@ Default timeout is 30 seconds (30,000ms). Configure with `timeout_ms` field. ## Caching -Successfull hook results are cached based on `cache_ttl_seconds`: +Successful hook results are cached based on `cache_ttl_seconds`: - `0`: No caching (default) - `> 0`: Cache successful results for specified seconds - AgentSpawn hooks are never cached \ No newline at end of file From 09a05aa02cabf802abe1fbcc11284cba8aaaf207 Mon Sep 17 00:00:00 2001 From: Felix Ding Date: Thu, 18 Sep 2025 12:11:42 -0700 Subject: [PATCH 10/14] chore: copy change for warning message for oauth redirect page (#2931) --- crates/chat-cli/src/mcp_client/oauth_util.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/chat-cli/src/mcp_client/oauth_util.rs b/crates/chat-cli/src/mcp_client/oauth_util.rs index 9c5319392..c4d4869bf 100644 --- a/crates/chat-cli/src/mcp_client/oauth_util.rs +++ b/crates/chat-cli/src/mcp_client/oauth_util.rs @@ -373,7 +373,11 @@ async fn make_svc( let error = params.get("error"); let resp = if let Some(err) = error { mk_response(format!( - "Oauth failed. Check url for precise reasons. Possible reasons: {err}.\nIf this is scope related. You can try configuring the server scopes to be an empty array via adding oauth_scopes: []" + "OAuth failed. Check URL for precise reasons. Possible reasons: {}.\n\ + If this is scope related, you can try configuring the server scopes \n\ + to be an empty array by adding \"oauthScopes\": [] to your server config.\n\ + Example: {{\"type\": \"http\", \"uri\": \"https://example.com/mcp\", \"oauthScopes\": []}}\n", + err )) } else { mk_response("You can close this page now".to_string()) From ad10417c0c8c240eb4a5792eb4042e1406e7f079 Mon Sep 17 00:00:00 2001 From: Felix Ding Date: Thu, 18 Sep 2025 14:42:50 -0700 Subject: [PATCH 11/14] fix: removes deny unknown fields in mcp config (#2935) --- crates/chat-cli/src/cli/chat/tools/custom_tool.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/chat-cli/src/cli/chat/tools/custom_tool.rs b/crates/chat-cli/src/cli/chat/tools/custom_tool.rs index 5457c9ee1..976f2d382 100644 --- a/crates/chat-cli/src/cli/chat/tools/custom_tool.rs +++ b/crates/chat-cli/src/cli/chat/tools/custom_tool.rs @@ -46,7 +46,7 @@ impl Default for TransportType { } #[derive(Clone, Serialize, Deserialize, Debug, Eq, PartialEq, JsonSchema)] -#[serde(rename_all = "camelCase", deny_unknown_fields)] +#[serde(rename_all = "camelCase")] pub struct CustomToolConfig { /// The transport type to use for communication with the MCP server #[serde(default)] From b33fe2066e18b5a25ee7c13b02e4638e19edc6b4 Mon Sep 17 00:00:00 2001 From: kkashilk <93673379+kkashilk@users.noreply.github.com> Date: Thu, 18 Sep 2025 15:27:41 -0700 Subject: [PATCH 12/14] Normalize and expand relative-paths to absolute paths (#2933) --- crates/chat-cli/src/cli/chat/cli/clear.rs | 2 +- crates/chat-cli/src/util/directories.rs | 93 +++++++++++++++++++++-- 2 files changed, 88 insertions(+), 7 deletions(-) diff --git a/crates/chat-cli/src/cli/chat/cli/clear.rs b/crates/chat-cli/src/cli/chat/cli/clear.rs index de274e2c0..994ed35e0 100644 --- a/crates/chat-cli/src/cli/chat/cli/clear.rs +++ b/crates/chat-cli/src/cli/chat/cli/clear.rs @@ -57,7 +57,7 @@ impl ClearArgs { session.tool_uses.clear(); session.pending_tool_index = None; session.tool_turn_start_time = None; - + execute!( session.stderr, style::SetForegroundColor(Color::Green), diff --git a/crates/chat-cli/src/util/directories.rs b/crates/chat-cli/src/util/directories.rs index 89c6f3bc4..2bb53381d 100644 --- a/crates/chat-cli/src/util/directories.rs +++ b/crates/chat-cli/src/util/directories.rs @@ -185,7 +185,45 @@ pub fn canonicalizes_path(os: &Os, path_as_str: &str) -> Result { let context = |input: &str| Ok(os.env.get(input).ok()); let home_dir = || os.env.home().map(|p| p.to_string_lossy().to_string()); - Ok(shellexpand::full_with_context(path_as_str, home_dir, context)?.to_string()) + let expanded = shellexpand::full_with_context(path_as_str, home_dir, context)?; + let path_buf = if !expanded.starts_with("/") { + // Convert relative paths to absolute paths + let current_dir = os.env.current_dir()?; + current_dir.join(expanded.as_ref() as &str) + } else { + // Already absolute path + PathBuf::from(expanded.as_ref() as &str) + }; + + // Try canonicalize first, fallback to manual normalization if it fails + match path_buf.canonicalize() { + Ok(normalized) => Ok(normalized.as_path().to_string_lossy().to_string()), + Err(_) => { + // If canonicalize fails (e.g., path doesn't exist), do manual normalization + let normalized = normalize_path(&path_buf); + Ok(normalized.to_string_lossy().to_string()) + }, + } +} + +/// Manually normalize a path by resolving . and .. components +fn normalize_path(path: &std::path::Path) -> std::path::PathBuf { + let mut components = Vec::new(); + for component in path.components() { + match component { + std::path::Component::CurDir => { + // Skip current directory components + }, + std::path::Component::ParentDir => { + // Pop the last component for parent directory + components.pop(); + }, + _ => { + components.push(component); + }, + } + } + components.iter().collect() } /// Given a globset builder and a path, build globs for both the file and directory patterns @@ -445,28 +483,71 @@ mod tests { // Test home directory expansion let result = canonicalizes_path(&test_os, "~/test").unwrap(); + #[cfg(windows)] + assert_eq!(result, "\\home\\testuser\\test"); + #[cfg(unix)] assert_eq!(result, "/home/testuser/test"); // Test environment variable expansion let result = canonicalizes_path(&test_os, "$TEST_VAR/path").unwrap(); - assert_eq!(result, "test_value/path"); + #[cfg(windows)] + assert_eq!(result, "\\test_value\\path"); + #[cfg(unix)] + assert_eq!(result, "/test_value/path"); // Test combined expansion let result = canonicalizes_path(&test_os, "~/$TEST_VAR").unwrap(); + #[cfg(windows)] + assert_eq!(result, "\\home\\testuser\\test_value"); + #[cfg(unix)] assert_eq!(result, "/home/testuser/test_value"); + // Test ~, . and .. expansion + let result = canonicalizes_path(&test_os, "~/./.././testuser").unwrap(); + #[cfg(windows)] + assert_eq!(result, "\\home\\testuser"); + #[cfg(unix)] + assert_eq!(result, "/home/testuser"); + // Test absolute path (no expansion needed) let result = canonicalizes_path(&test_os, "/absolute/path").unwrap(); + #[cfg(windows)] + assert_eq!(result, "\\absolute\\path"); + #[cfg(unix)] assert_eq!(result, "/absolute/path"); - // Test relative path (no expansion needed) + // Test ~, . and .. expansion for a path that does not exist + let result = canonicalizes_path(&test_os, "~/./.././testuser/new/path/../../new").unwrap(); + #[cfg(windows)] + assert_eq!(result, "\\home\\testuser\\new"); + #[cfg(unix)] + assert_eq!(result, "/home/testuser/new"); + + // Test path with . and .. + let result = canonicalizes_path(&test_os, "/absolute/./../path").unwrap(); + #[cfg(windows)] + assert_eq!(result, "\\path"); + #[cfg(unix)] + assert_eq!(result, "/path"); + + // Test relative path (which should be expanded because now all inputs are converted to + // absolute) let result = canonicalizes_path(&test_os, "relative/path").unwrap(); - assert_eq!(result, "relative/path"); + #[cfg(windows)] + assert_eq!(result, "\\relative\\path"); + #[cfg(unix)] + assert_eq!(result, "/relative/path"); // Test glob prefixed paths let result = canonicalizes_path(&test_os, "**/path").unwrap(); - assert_eq!(result, "**/path"); + #[cfg(windows)] + assert_eq!(result, "\\**\\path"); + #[cfg(unix)] + assert_eq!(result, "/**/path"); let result = canonicalizes_path(&test_os, "**/middle/**/path").unwrap(); - assert_eq!(result, "**/middle/**/path"); + #[cfg(windows)] + assert_eq!(result, "\\**\\middle\\**\\path"); + #[cfg(unix)] + assert_eq!(result, "/**/middle/**/path"); } } From 69df9311a031b4fa434ea361e533f59e7ec42eb3 Mon Sep 17 00:00:00 2001 From: kkashilk <93673379+kkashilk@users.noreply.github.com> Date: Thu, 18 Sep 2025 16:54:17 -0700 Subject: [PATCH 13/14] Bump version to 1.16.2 and update feed.json (#2938) --- Cargo.lock | 4 ++-- Cargo.toml | 2 +- crates/chat-cli/src/cli/feed.json | 32 +++++++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f85bfb292..7230af9f7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1207,7 +1207,7 @@ checksum = "613afe47fcd5fac7ccf1db93babcb082c5994d996f20b8b159f2ad1658eb5724" [[package]] name = "chat_cli" -version = "1.16.1" +version = "1.16.2" dependencies = [ "amzn-codewhisperer-client", "amzn-codewhisperer-streaming-client", @@ -5647,7 +5647,7 @@ dependencies = [ [[package]] name = "semantic_search_client" -version = "1.16.1" +version = "1.16.2" dependencies = [ "anyhow", "bm25", diff --git a/Cargo.toml b/Cargo.toml index bc7b16ff3..7f0b79de2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,7 +8,7 @@ authors = ["Amazon Q CLI Team (q-cli@amazon.com)", "Chay Nabors (nabochay@amazon edition = "2024" homepage = "https://aws.amazon.com/q/" publish = false -version = "1.16.1" +version = "1.16.2" license = "MIT OR Apache-2.0" [workspace.dependencies] diff --git a/crates/chat-cli/src/cli/feed.json b/crates/chat-cli/src/cli/feed.json index a9d0f68eb..e81a34d24 100644 --- a/crates/chat-cli/src/cli/feed.json +++ b/crates/chat-cli/src/cli/feed.json @@ -10,6 +10,38 @@ "hidden": true, "changes": [] }, + { + "type": "release", + "date": "2025-09-19", + "version": "1.16.2", + "title": "Version 1.16.2", + "changes": [ + { + "type": "added", + "description": "Add support for preToolUse and postToolUse hook - [#2875](https://github.com/aws/amazon-q-developer-cli/pull/2875)" + }, + { + "type": "added", + "description": "Support for specifying oauth scopes via config - [#2925]( https://github.com/aws/amazon-q-developer-cli/pull/2925)" + }, + { + "type": "fixed", + "description": "Support for headers ingestion for remote mcp - [#2925]( https://github.com/aws/amazon-q-developer-cli/pull/2925)" + }, + { + "type": "added", + "description": "Change autocomplete shortcut from ctrl-f to ctrl-g - [#2634](https://github.com/aws/amazon-q-developer-cli/pull/2634)" + }, + { + "type": "fixed", + "description": "Fix file-path expansion in mcp-config - [#2915]( https://github.com/aws/amazon-q-developer-cli/pull/2915)" + }, + { + "type": "fixed", + "description": "Fix filepath expansion to use absolute paths - [#2933](https://github.com/aws/amazon-q-developer-cli/pull/2933)" + } + ] + }, { "type": "release", "date": "2025-09-17", From c4c80bb44cb22b6e19129ed7475f2793ff20135d Mon Sep 17 00:00:00 2001 From: dingfeli Date: Thu, 18 Sep 2025 17:20:19 -0700 Subject: [PATCH 14/14] changes how mcp command gets expanded --- crates/chat-cli/src/mcp_client/client.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/crates/chat-cli/src/mcp_client/client.rs b/crates/chat-cli/src/mcp_client/client.rs index b05ed3ad2..44473c3a7 100644 --- a/crates/chat-cli/src/mcp_client/client.rs +++ b/crates/chat-cli/src/mcp_client/client.rs @@ -60,10 +60,7 @@ use crate::cli::chat::tools::custom_tool::{ TransportType, }; use crate::os::Os; -use crate::util::directories::{ - DirectoryError, - canonicalizes_path, -}; +use crate::util::directories::DirectoryError; /// Fetches all pages of specified resources from a server macro_rules! paginated_fetch { @@ -153,6 +150,8 @@ pub enum McpClientError { Auth(#[from] crate::auth::AuthError), #[error("{0}")] MalformedConfig(&'static str), + #[error(transparent)] + LookUp(#[from] shellexpand::LookupError), } /// Decorates the method passed in with retry logic, but only if the [RunningService] has an @@ -527,8 +526,11 @@ impl McpClientService { match r#type { TransportType::Stdio => { - let expanded_cmd = canonicalizes_path(os, command_as_str)?; - let command = Command::new(expanded_cmd).configure(|cmd| { + let context = |input: &str| Ok(os.env.get(input).ok()); + let home_dir = || os.env.home().map(|p| p.to_string_lossy().to_string()); + let expanded_cmd = shellexpand::full_with_context(command_as_str, home_dir, context)?; + + let command = Command::new(expanded_cmd.as_ref() as &str).configure(|cmd| { if let Some(envs) = config_envs { process_env_vars(envs, &os.env); cmd.envs(envs);