Fix MCP tool name parsing: use newline delimiter instead of colon (#48636)

Richard Feldman created 2 months ago

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

Change summary

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(-)

Detailed changes

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"
     );
 }

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"));
             }

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"
         );
     }