From cab418a64bb79165f263360ceb9aa4198525356c Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Fri, 6 Feb 2026 17:31:50 -0500 Subject: [PATCH] Fix MCP tool name parsing: use newline delimiter instead of colon (#48636) MCP tool names can contain colons (e.g. `mcp:server:tool`), which broke the `splitn(3, ':')` parsing of always-allow/always-deny pattern option IDs. This switches to newline (`\n`) as the delimiter between tool name and pattern, since newlines cannot appear in either component. ## Changes - **Option ID format**: Changed from `always_allow_pattern:{tool}:{pattern}` to `always_allow_pattern:{tool}\n{pattern}` - **Response parsing**: Replaced `splitn(3, ':')` with `strip_prefix` + `split_once('\n')` - **Error logging**: Added `log::error!` when pattern parsing fails (previously silent) - **Tests**: Updated test assertions in `agent` and `agent_ui` crates No release notes because granular tool permissions are still feature-flagged. Release Notes: - N/A --- crates/agent/src/tests/mod.rs | 4 ++-- crates/agent/src/thread.rs | 30 ++++++++++++++------------ crates/agent_ui/src/acp/thread_view.rs | 4 ++-- 3 files changed, 20 insertions(+), 18 deletions(-) diff --git a/crates/agent/src/tests/mod.rs b/crates/agent/src/tests/mod.rs index 3b6a047256efff90796c184b63cc2c0b89d94714..8a6f13cd28ac696b938c10189ac3b74d43828628 100644 --- a/crates/agent/src/tests/mod.rs +++ b/crates/agent/src/tests/mod.rs @@ -1065,7 +1065,7 @@ fn test_permission_option_ids_for_terminal() { assert!( allow_ids .iter() - .any(|id| id.starts_with("always_allow_pattern:terminal:")), + .any(|id| id.starts_with("always_allow_pattern:terminal\n")), "Missing allow pattern option" ); @@ -1074,7 +1074,7 @@ fn test_permission_option_ids_for_terminal() { assert!( deny_ids .iter() - .any(|id| id.starts_with("always_deny_pattern:terminal:")), + .any(|id| id.starts_with("always_deny_pattern:terminal\n")), "Missing deny pattern option" ); } diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index 57aca268ed9eb43949c0a761cfb83aac34e7c91f..119c9e936975aaae53136330c6c91b961313bb77 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -728,8 +728,8 @@ impl ToolPermissionContext { }; push_choice( button_text, - format!("always_allow_pattern:{}:{}", tool_name, pattern), - format!("always_deny_pattern:{}:{}", tool_name, pattern), + format!("always_allow_pattern:{}\n{}", tool_name, pattern), + format!("always_deny_pattern:{}\n{}", tool_name, pattern), acp::PermissionOptionKind::AllowAlways, acp::PermissionOptionKind::RejectAlways, ); @@ -3287,12 +3287,11 @@ impl ToolCallEventStream { return Err(anyhow!("Permission to run tool denied by user")); } - // Handle "always allow pattern" - e.g., "always_allow_pattern:terminal:^cargo\s" - if response_str.starts_with("always_allow_pattern:") { - let parts: Vec<&str> = response_str.splitn(3, ':').collect(); - if parts.len() == 3 { - let pattern_tool_name = parts[1].to_string(); - let pattern = parts[2].to_string(); + // Handle "always allow pattern" - e.g., "always_allow_pattern:mcp:server:tool\n^cargo\s" + if let Some(rest) = response_str.strip_prefix("always_allow_pattern:") { + if let Some((pattern_tool_name, pattern)) = rest.split_once('\n') { + let pattern_tool_name = pattern_tool_name.to_string(); + let pattern = pattern.to_string(); if let Some(fs) = fs.clone() { cx.update(|cx| { update_settings_file(fs, cx, move |settings, _| { @@ -3303,16 +3302,17 @@ impl ToolCallEventStream { }); }); } + } else { + log::error!("Failed to parse always allow pattern: missing newline separator in '{rest}'"); } return Ok(()); } - // Handle "always deny pattern" - e.g., "always_deny_pattern:terminal:^cargo\s" - if response_str.starts_with("always_deny_pattern:") { - let parts: Vec<&str> = response_str.splitn(3, ':').collect(); - if parts.len() == 3 { - let pattern_tool_name = parts[1].to_string(); - let pattern = parts[2].to_string(); + // Handle "always deny pattern" - e.g., "always_deny_pattern:mcp:server:tool\n^cargo\s" + if let Some(rest) = response_str.strip_prefix("always_deny_pattern:") { + if let Some((pattern_tool_name, pattern)) = rest.split_once('\n') { + let pattern_tool_name = pattern_tool_name.to_string(); + let pattern = pattern.to_string(); if let Some(fs) = fs.clone() { cx.update(|cx| { update_settings_file(fs, cx, move |settings, _| { @@ -3323,6 +3323,8 @@ impl ToolCallEventStream { }); }); } + } else { + log::error!("Failed to parse always deny pattern: missing newline separator in '{rest}'"); } return Err(anyhow!("Permission to run tool denied by user")); } diff --git a/crates/agent_ui/src/acp/thread_view.rs b/crates/agent_ui/src/acp/thread_view.rs index d9bd7b28ad1112aadc60ab3fbf8b84d9e18a3c0c..8bc1cd1647acfb133164f6a2f72e89f62319e868 100644 --- a/crates/agent_ui/src/acp/thread_view.rs +++ b/crates/agent_ui/src/acp/thread_view.rs @@ -4944,7 +4944,7 @@ pub(crate) mod tests { assert!( allow_ids .iter() - .any(|id| id.starts_with("always_allow_pattern:terminal:")), + .any(|id| id.starts_with("always_allow_pattern:terminal\n")), "Missing allow pattern option" ); } @@ -4969,7 +4969,7 @@ pub(crate) mod tests { assert!( deny_ids .iter() - .any(|id| id.starts_with("always_deny_pattern:terminal:")), + .any(|id| id.starts_with("always_deny_pattern:terminal\n")), "Missing deny pattern option" ); }