Richard Feldman and Amp created
This builds on https://github.com/zed-industries/zed/pull/46112 (which
should be merged first) and adds the permission evaluation logic and
integrates it with the terminal tool as part of the tool permissions
feature. This is a separate PR for [stacked
diffs](https://newsletter.pragmaticengineer.com/p/stacked-diffs) to make
review easier.
## Changes
- **Add `tool_permissions.rs`** with `decide_permission()` function that
implements:
- `deny > confirm > allow` precedence hierarchy (security-critical)
- Case-sensitive and case-insensitive regex matching
- Integration with existing `always_allow_tool_actions` setting
- Comprehensive unit tests (15 tests)
- **Integrate with terminal tool**:
- Commands matching deny rules are blocked immediately with an error
- Commands matching allow rules proceed without confirmation dialog
- Commands requiring confirmation show the dialog as before
- Added integration tests for deny and allow rule scenarios
Co-Authored-By: Claude Opus 4.5
Release Notes:
- N/A
---------
Co-authored-by: Amp <amp@ampcode.com>crates/agent/src/agent.rs | 2
crates/agent/src/tests/mod.rs | 235 +++++++++
crates/agent/src/thread.rs | 8
crates/agent/src/tool_permissions.rs | 547 +++++++++++++++++++++++
crates/agent/src/tools/terminal_tool.rs | 25
crates/agent_settings/src/agent_settings.rs | 151 +++++-
6 files changed, 940 insertions(+), 28 deletions(-)
@@ -8,6 +8,7 @@ mod templates;
#[cfg(test)]
mod tests;
mod thread;
+mod tool_permissions;
mod tools;
use context_server::ContextServerId;
@@ -16,6 +17,7 @@ pub use history_store::*;
pub use native_agent_server::NativeAgentServer;
pub use templates::*;
pub use thread::*;
+pub use tool_permissions::*;
pub use tools::*;
use acp_thread::{AcpThread, AgentModelSelector, UserMessageId};
@@ -93,6 +93,25 @@ impl FakeTerminalHandle {
}
}
+ fn new_with_immediate_exit(cx: &mut App, exit_code: u32) -> Self {
+ let killed = Arc::new(AtomicBool::new(false));
+ let stopped_by_user = Arc::new(AtomicBool::new(false));
+ let (exit_sender, _exit_receiver) = futures::channel::oneshot::channel();
+
+ let wait_for_exit = cx
+ .spawn(async move |_cx| acp::TerminalExitStatus::new().exit_code(exit_code))
+ .shared();
+
+ Self {
+ killed,
+ stopped_by_user,
+ exit_sender: std::cell::RefCell::new(Some(exit_sender)),
+ wait_for_exit,
+ output: acp::TerminalOutputResponse::new("command output".to_string(), false),
+ id: acp::TerminalId::new("fake_terminal".to_string()),
+ }
+ }
+
fn was_killed(&self) -> bool {
self.killed.load(Ordering::SeqCst)
}
@@ -3596,3 +3615,219 @@ async fn test_tokens_before_message_after_truncate(cx: &mut TestAppContext) {
);
});
}
+
+#[gpui::test]
+async fn test_terminal_tool_permission_rules(cx: &mut TestAppContext) {
+ init_test(cx);
+
+ let fs = FakeFs::new(cx.executor());
+ fs.insert_tree("/root", json!({})).await;
+ let project = Project::test(fs, ["/root".as_ref()], cx).await;
+
+ // Test 1: Deny rule blocks command
+ {
+ let handle = Rc::new(cx.update(|cx| FakeTerminalHandle::new_never_exits(cx)));
+ let environment = Rc::new(FakeThreadEnvironment {
+ handle: handle.clone(),
+ });
+
+ cx.update(|cx| {
+ let mut settings = agent_settings::AgentSettings::get_global(cx).clone();
+ settings.tool_permissions.tools.insert(
+ "terminal".into(),
+ agent_settings::ToolRules {
+ default_mode: settings::ToolPermissionMode::Confirm,
+ always_allow: vec![],
+ always_deny: vec![
+ agent_settings::CompiledRegex::new(r"rm\s+-rf", false).unwrap(),
+ ],
+ always_confirm: vec![],
+ invalid_patterns: vec![],
+ },
+ );
+ agent_settings::AgentSettings::override_global(settings, cx);
+ });
+
+ #[allow(clippy::arc_with_non_send_sync)]
+ let tool = Arc::new(crate::TerminalTool::new(project.clone(), environment));
+ let (event_stream, _rx) = crate::ToolCallEventStream::test();
+
+ let task = cx.update(|cx| {
+ tool.run(
+ crate::TerminalToolInput {
+ command: "rm -rf /".to_string(),
+ cd: ".".to_string(),
+ timeout_ms: None,
+ },
+ event_stream,
+ cx,
+ )
+ });
+
+ let result = task.await;
+ assert!(
+ result.is_err(),
+ "expected command to be blocked by deny rule"
+ );
+ assert!(
+ result.unwrap_err().to_string().contains("blocked"),
+ "error should mention the command was blocked"
+ );
+ }
+
+ // Test 2: Allow rule skips confirmation (and overrides default_mode: Deny)
+ {
+ let handle = Rc::new(cx.update(|cx| FakeTerminalHandle::new_with_immediate_exit(cx, 0)));
+ let environment = Rc::new(FakeThreadEnvironment {
+ handle: handle.clone(),
+ });
+
+ cx.update(|cx| {
+ let mut settings = agent_settings::AgentSettings::get_global(cx).clone();
+ settings.always_allow_tool_actions = false;
+ settings.tool_permissions.tools.insert(
+ "terminal".into(),
+ agent_settings::ToolRules {
+ default_mode: settings::ToolPermissionMode::Deny,
+ always_allow: vec![
+ agent_settings::CompiledRegex::new(r"^echo\s", false).unwrap(),
+ ],
+ always_deny: vec![],
+ always_confirm: vec![],
+ invalid_patterns: vec![],
+ },
+ );
+ agent_settings::AgentSettings::override_global(settings, cx);
+ });
+
+ #[allow(clippy::arc_with_non_send_sync)]
+ let tool = Arc::new(crate::TerminalTool::new(project.clone(), environment));
+ let (event_stream, mut rx) = crate::ToolCallEventStream::test();
+
+ let task = cx.update(|cx| {
+ tool.run(
+ crate::TerminalToolInput {
+ command: "echo hello".to_string(),
+ cd: ".".to_string(),
+ timeout_ms: None,
+ },
+ event_stream,
+ cx,
+ )
+ });
+
+ let update = rx.expect_update_fields().await;
+ assert!(
+ update.content.iter().any(|blocks| {
+ blocks
+ .iter()
+ .any(|c| matches!(c, acp::ToolCallContent::Terminal(_)))
+ }),
+ "expected terminal content (allow rule should skip confirmation and override default deny)"
+ );
+
+ let result = task.await;
+ assert!(
+ result.is_ok(),
+ "expected command to succeed without confirmation"
+ );
+ }
+
+ // Test 3: Confirm rule forces confirmation even with always_allow_tool_actions=true
+ {
+ let handle = Rc::new(cx.update(|cx| FakeTerminalHandle::new_with_immediate_exit(cx, 0)));
+ let environment = Rc::new(FakeThreadEnvironment {
+ handle: handle.clone(),
+ });
+
+ cx.update(|cx| {
+ let mut settings = agent_settings::AgentSettings::get_global(cx).clone();
+ settings.always_allow_tool_actions = true;
+ settings.tool_permissions.tools.insert(
+ "terminal".into(),
+ agent_settings::ToolRules {
+ default_mode: settings::ToolPermissionMode::Allow,
+ always_allow: vec![],
+ always_deny: vec![],
+ always_confirm: vec![
+ agent_settings::CompiledRegex::new(r"sudo", false).unwrap(),
+ ],
+ invalid_patterns: vec![],
+ },
+ );
+ agent_settings::AgentSettings::override_global(settings, cx);
+ });
+
+ #[allow(clippy::arc_with_non_send_sync)]
+ let tool = Arc::new(crate::TerminalTool::new(project.clone(), environment));
+ let (event_stream, mut rx) = crate::ToolCallEventStream::test();
+
+ let _task = cx.update(|cx| {
+ tool.run(
+ crate::TerminalToolInput {
+ command: "sudo rm file".to_string(),
+ cd: ".".to_string(),
+ timeout_ms: None,
+ },
+ event_stream,
+ cx,
+ )
+ });
+
+ let auth = rx.expect_authorization().await;
+ assert!(
+ auth.tool_call.fields.title.is_some(),
+ "expected authorization request for sudo command despite always_allow_tool_actions=true"
+ );
+ }
+
+ // Test 4: default_mode: Deny blocks commands when no pattern matches
+ {
+ let handle = Rc::new(cx.update(|cx| FakeTerminalHandle::new_never_exits(cx)));
+ let environment = Rc::new(FakeThreadEnvironment {
+ handle: handle.clone(),
+ });
+
+ cx.update(|cx| {
+ let mut settings = agent_settings::AgentSettings::get_global(cx).clone();
+ settings.always_allow_tool_actions = true;
+ settings.tool_permissions.tools.insert(
+ "terminal".into(),
+ agent_settings::ToolRules {
+ default_mode: settings::ToolPermissionMode::Deny,
+ always_allow: vec![],
+ always_deny: vec![],
+ always_confirm: vec![],
+ invalid_patterns: vec![],
+ },
+ );
+ agent_settings::AgentSettings::override_global(settings, cx);
+ });
+
+ #[allow(clippy::arc_with_non_send_sync)]
+ let tool = Arc::new(crate::TerminalTool::new(project.clone(), environment));
+ let (event_stream, _rx) = crate::ToolCallEventStream::test();
+
+ let task = cx.update(|cx| {
+ tool.run(
+ crate::TerminalToolInput {
+ command: "echo hello".to_string(),
+ cd: ".".to_string(),
+ timeout_ms: None,
+ },
+ event_stream,
+ cx,
+ )
+ });
+
+ let result = task.await;
+ assert!(
+ result.is_err(),
+ "expected command to be blocked by default_mode: Deny"
+ );
+ assert!(
+ result.unwrap_err().to_string().contains("disabled"),
+ "error should mention the tool is disabled"
+ );
+ }
+}
@@ -2663,6 +2663,14 @@ impl ToolCallEventStream {
return Task::ready(Ok(()));
}
+ self.authorize_required(title, cx)
+ }
+
+ /// Like `authorize`, but always prompts for confirmation regardless of
+ /// the `always_allow_tool_actions` setting. Use this when tool-specific
+ /// permission rules (like `always_confirm` patterns) have already determined
+ /// that confirmation is required.
+ pub fn authorize_required(&self, title: impl Into<String>, cx: &mut App) -> Task<Result<()>> {
let (response_tx, response_rx) = oneshot::channel();
self.stream
.0
@@ -0,0 +1,547 @@
+use agent_settings::{AgentSettings, ToolPermissions, ToolRules};
+use settings::ToolPermissionMode;
+
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum ToolPermissionDecision {
+ Allow,
+ Deny(String),
+ Confirm,
+}
+
+/// Determines the permission decision for a tool invocation based on configured rules.
+///
+/// # Precedence Order (highest to lowest)
+///
+/// 1. **`always_deny`** - If any deny pattern matches, the tool call is blocked immediately.
+/// This takes precedence over all other rules for security.
+/// 2. **`always_confirm`** - If any confirm pattern matches (and no deny matched),
+/// the user is prompted for confirmation regardless of other settings.
+/// 3. **`always_allow`** - If any allow pattern matches (and no deny/confirm matched),
+/// the tool call proceeds without prompting.
+/// 4. **`default_mode`** - If no patterns match, falls back to the tool's default mode.
+/// 5. **`always_allow_tool_actions`** - Global setting used as fallback when no tool-specific
+/// rules are configured, or when `default_mode` is `Confirm`.
+///
+/// # Pattern Matching Tips
+///
+/// Patterns are matched as regular expressions against the tool input (e.g., the command
+/// string for the terminal tool). Some tips for writing effective patterns:
+///
+/// - Use word boundaries (`\b`) to avoid partial matches. For example, pattern `rm` will
+/// match "storm" and "arms", but `\brm\b` will only match the standalone word "rm".
+/// This is important for security rules where you want to block specific commands
+/// without accidentally blocking unrelated commands that happen to contain the same
+/// substring.
+/// - Patterns are case-insensitive by default. Set `case_sensitive: true` for exact matching.
+/// - Use `^` and `$` anchors to match the start/end of the input.
+pub fn decide_permission(
+ tool_name: &str,
+ input: &str,
+ permissions: &ToolPermissions,
+ always_allow_tool_actions: bool,
+) -> ToolPermissionDecision {
+ let rules = permissions.tools.get(tool_name);
+
+ let rules = match rules {
+ Some(rules) => rules,
+ None => {
+ return if always_allow_tool_actions {
+ ToolPermissionDecision::Allow
+ } else {
+ ToolPermissionDecision::Confirm
+ };
+ }
+ };
+
+ // Check for invalid regex patterns before evaluating rules.
+ // If any patterns failed to compile, block the tool call entirely.
+ if let Some(error) = check_invalid_patterns(tool_name, rules) {
+ return ToolPermissionDecision::Deny(error);
+ }
+
+ if rules.always_deny.iter().any(|r| r.is_match(input)) {
+ return ToolPermissionDecision::Deny(format!(
+ "Command blocked by security rule for {} tool",
+ tool_name
+ ));
+ }
+
+ if rules.always_confirm.iter().any(|r| r.is_match(input)) {
+ return ToolPermissionDecision::Confirm;
+ }
+
+ if rules.always_allow.iter().any(|r| r.is_match(input)) {
+ return ToolPermissionDecision::Allow;
+ }
+
+ match rules.default_mode {
+ ToolPermissionMode::Deny => {
+ ToolPermissionDecision::Deny(format!("{} tool is disabled", tool_name))
+ }
+ ToolPermissionMode::Allow => ToolPermissionDecision::Allow,
+ ToolPermissionMode::Confirm => {
+ if always_allow_tool_actions {
+ ToolPermissionDecision::Allow
+ } else {
+ ToolPermissionDecision::Confirm
+ }
+ }
+ }
+}
+
+/// Checks if the tool rules contain any invalid regex patterns.
+/// Returns an error message if invalid patterns are found.
+fn check_invalid_patterns(tool_name: &str, rules: &ToolRules) -> Option<String> {
+ if rules.invalid_patterns.is_empty() {
+ return None;
+ }
+
+ let count = rules.invalid_patterns.len();
+ let pattern_word = if count == 1 { "pattern" } else { "patterns" };
+
+ Some(format!(
+ "The {} tool cannot run because {} regex {} failed to compile. \
+ Please fix the invalid patterns in your tool_permissions settings.",
+ tool_name, count, pattern_word
+ ))
+}
+
+/// Convenience wrapper that extracts permission settings from `AgentSettings`.
+///
+/// This is the primary entry point for tools to check permissions. It extracts
+/// `tool_permissions` and `always_allow_tool_actions` from the settings and
+/// delegates to [`decide_permission`].
+pub fn decide_permission_from_settings(
+ tool_name: &str,
+ input: &str,
+ settings: &AgentSettings,
+) -> ToolPermissionDecision {
+ decide_permission(
+ tool_name,
+ input,
+ &settings.tool_permissions,
+ settings.always_allow_tool_actions,
+ )
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use agent_settings::{CompiledRegex, InvalidRegexPattern, ToolRules};
+ use std::sync::Arc;
+
+ fn empty_permissions() -> ToolPermissions {
+ ToolPermissions {
+ tools: collections::HashMap::default(),
+ }
+ }
+
+ fn terminal_rules_with_deny(patterns: &[&str]) -> ToolPermissions {
+ let mut tools = collections::HashMap::default();
+ tools.insert(
+ Arc::from("terminal"),
+ ToolRules {
+ default_mode: ToolPermissionMode::Confirm,
+ always_allow: vec![],
+ always_deny: patterns
+ .iter()
+ .filter_map(|p| CompiledRegex::new(p, false))
+ .collect(),
+ always_confirm: vec![],
+ invalid_patterns: vec![],
+ },
+ );
+ ToolPermissions { tools }
+ }
+
+ fn terminal_rules_with_allow(patterns: &[&str]) -> ToolPermissions {
+ let mut tools = collections::HashMap::default();
+ tools.insert(
+ Arc::from("terminal"),
+ ToolRules {
+ default_mode: ToolPermissionMode::Confirm,
+ always_allow: patterns
+ .iter()
+ .filter_map(|p| CompiledRegex::new(p, false))
+ .collect(),
+ always_deny: vec![],
+ always_confirm: vec![],
+ invalid_patterns: vec![],
+ },
+ );
+ ToolPermissions { tools }
+ }
+
+ #[test]
+ fn test_deny_takes_precedence_over_allow() {
+ let mut tools = collections::HashMap::default();
+ tools.insert(
+ Arc::from("terminal"),
+ ToolRules {
+ default_mode: ToolPermissionMode::Allow,
+ always_allow: vec![CompiledRegex::new("dangerous", false).unwrap()],
+ always_deny: vec![CompiledRegex::new("dangerous", false).unwrap()],
+ always_confirm: vec![],
+ invalid_patterns: vec![],
+ },
+ );
+ let permissions = ToolPermissions { tools };
+
+ let decision = decide_permission("terminal", "run dangerous command", &permissions, true);
+ assert!(matches!(decision, ToolPermissionDecision::Deny(_)));
+ }
+
+ #[test]
+ fn test_deny_takes_precedence_over_confirm() {
+ let mut tools = collections::HashMap::default();
+ tools.insert(
+ Arc::from("terminal"),
+ ToolRules {
+ default_mode: ToolPermissionMode::Allow,
+ always_allow: vec![],
+ always_deny: vec![CompiledRegex::new("dangerous", false).unwrap()],
+ always_confirm: vec![CompiledRegex::new("dangerous", false).unwrap()],
+ invalid_patterns: vec![],
+ },
+ );
+ let permissions = ToolPermissions { tools };
+
+ let decision = decide_permission("terminal", "run dangerous command", &permissions, true);
+ assert!(matches!(decision, ToolPermissionDecision::Deny(_)));
+ }
+
+ #[test]
+ fn test_confirm_takes_precedence_over_allow() {
+ let mut tools = collections::HashMap::default();
+ tools.insert(
+ Arc::from("terminal"),
+ ToolRules {
+ default_mode: ToolPermissionMode::Allow,
+ always_allow: vec![CompiledRegex::new("risky", false).unwrap()],
+ always_deny: vec![],
+ always_confirm: vec![CompiledRegex::new("risky", false).unwrap()],
+ invalid_patterns: vec![],
+ },
+ );
+ let permissions = ToolPermissions { tools };
+
+ let decision = decide_permission("terminal", "do risky thing", &permissions, true);
+ assert_eq!(decision, ToolPermissionDecision::Confirm);
+ }
+
+ #[test]
+ fn test_no_tool_rules_uses_global_setting() {
+ let permissions = empty_permissions();
+
+ let decision = decide_permission("terminal", "any command", &permissions, false);
+ assert_eq!(decision, ToolPermissionDecision::Confirm);
+
+ let decision = decide_permission("terminal", "any command", &permissions, true);
+ assert_eq!(decision, ToolPermissionDecision::Allow);
+ }
+
+ #[test]
+ fn test_default_mode_fallthrough() {
+ // default_mode: Allow - should allow regardless of global setting
+ let mut tools = collections::HashMap::default();
+ tools.insert(
+ Arc::from("terminal"),
+ ToolRules {
+ default_mode: ToolPermissionMode::Allow,
+ always_allow: vec![],
+ always_deny: vec![],
+ always_confirm: vec![],
+ invalid_patterns: vec![],
+ },
+ );
+ let permissions = ToolPermissions { tools };
+ let decision = decide_permission("terminal", "any command", &permissions, false);
+ assert_eq!(decision, ToolPermissionDecision::Allow);
+
+ // default_mode: Deny - should deny regardless of global setting
+ let mut tools = collections::HashMap::default();
+ tools.insert(
+ Arc::from("terminal"),
+ ToolRules {
+ default_mode: ToolPermissionMode::Deny,
+ always_allow: vec![],
+ always_deny: vec![],
+ always_confirm: vec![],
+ invalid_patterns: vec![],
+ },
+ );
+ let permissions = ToolPermissions { tools };
+ let decision = decide_permission("terminal", "any command", &permissions, true);
+ assert!(matches!(decision, ToolPermissionDecision::Deny(_)));
+
+ // default_mode: Confirm - respects global always_allow_tool_actions
+ let mut tools = collections::HashMap::default();
+ tools.insert(
+ Arc::from("terminal"),
+ ToolRules {
+ default_mode: ToolPermissionMode::Confirm,
+ always_allow: vec![],
+ always_deny: vec![],
+ always_confirm: vec![],
+ invalid_patterns: vec![],
+ },
+ );
+ let permissions = ToolPermissions { tools };
+ let decision = decide_permission("terminal", "any command", &permissions, false);
+ assert_eq!(decision, ToolPermissionDecision::Confirm);
+ let decision = decide_permission("terminal", "any command", &permissions, true);
+ assert_eq!(decision, ToolPermissionDecision::Allow);
+ }
+
+ #[test]
+ fn test_empty_input() {
+ let permissions = terminal_rules_with_deny(&["rm"]);
+
+ // Empty input doesn't match the deny pattern, so falls through to default_mode (Confirm)
+ let decision = decide_permission("terminal", "", &permissions, false);
+ assert_eq!(decision, ToolPermissionDecision::Confirm);
+
+ // With always_allow_tool_actions=true and default_mode=Confirm, it returns Allow
+ let decision = decide_permission("terminal", "", &permissions, true);
+ assert_eq!(decision, ToolPermissionDecision::Allow);
+ }
+
+ #[test]
+ fn test_multiple_patterns_any_match() {
+ // Multiple deny patterns - any match should deny
+ let permissions = terminal_rules_with_deny(&["rm", "dangerous", "delete"]);
+
+ let decision = decide_permission("terminal", "run dangerous command", &permissions, true);
+ assert!(matches!(decision, ToolPermissionDecision::Deny(_)));
+
+ let decision = decide_permission("terminal", "delete file", &permissions, true);
+ assert!(matches!(decision, ToolPermissionDecision::Deny(_)));
+
+ // Multiple allow patterns - any match should allow
+ let permissions = terminal_rules_with_allow(&["^cargo", "^npm", "^git"]);
+
+ let decision = decide_permission("terminal", "cargo build", &permissions, false);
+ assert_eq!(decision, ToolPermissionDecision::Allow);
+
+ let decision = decide_permission("terminal", "npm install", &permissions, false);
+ assert_eq!(decision, ToolPermissionDecision::Allow);
+
+ // No pattern matches - falls through to default
+ let decision = decide_permission("terminal", "rm file", &permissions, false);
+ assert_eq!(decision, ToolPermissionDecision::Confirm);
+ }
+
+ #[test]
+ fn test_case_insensitive_matching() {
+ // Case-insensitive by default (case_sensitive: false)
+ let mut tools = collections::HashMap::default();
+ tools.insert(
+ Arc::from("terminal"),
+ ToolRules {
+ default_mode: ToolPermissionMode::Confirm,
+ always_allow: vec![],
+ always_deny: vec![CompiledRegex::new(r"\brm\b", false).unwrap()],
+ always_confirm: vec![],
+ invalid_patterns: vec![],
+ },
+ );
+ let permissions = ToolPermissions { tools };
+
+ // Should match regardless of case
+ let decision = decide_permission("terminal", "RM file.txt", &permissions, true);
+ assert!(matches!(decision, ToolPermissionDecision::Deny(_)));
+
+ let decision = decide_permission("terminal", "Rm file.txt", &permissions, true);
+ assert!(matches!(decision, ToolPermissionDecision::Deny(_)));
+
+ let decision = decide_permission("terminal", "rm file.txt", &permissions, true);
+ assert!(matches!(decision, ToolPermissionDecision::Deny(_)));
+ }
+
+ #[test]
+ fn test_case_sensitive_matching() {
+ // Case-sensitive matching when explicitly enabled
+ let mut tools = collections::HashMap::default();
+ tools.insert(
+ Arc::from("terminal"),
+ ToolRules {
+ default_mode: ToolPermissionMode::Confirm,
+ always_allow: vec![],
+ always_deny: vec![CompiledRegex::new("DROP TABLE", true).unwrap()],
+ always_confirm: vec![],
+ invalid_patterns: vec![],
+ },
+ );
+ let permissions = ToolPermissions { tools };
+
+ // Should only match exact case
+ let decision = decide_permission("terminal", "DROP TABLE users", &permissions, true);
+ assert!(matches!(decision, ToolPermissionDecision::Deny(_)));
+
+ // Should NOT match different case
+ let decision = decide_permission("terminal", "drop table users", &permissions, true);
+ assert_eq!(decision, ToolPermissionDecision::Allow);
+ }
+
+ #[test]
+ fn test_multi_tool_isolation() {
+ // Rules for terminal should not affect edit_file
+ let mut tools = collections::HashMap::default();
+ tools.insert(
+ Arc::from("terminal"),
+ ToolRules {
+ default_mode: ToolPermissionMode::Deny,
+ always_allow: vec![],
+ always_deny: vec![CompiledRegex::new("dangerous", false).unwrap()],
+ always_confirm: vec![],
+ invalid_patterns: vec![],
+ },
+ );
+ tools.insert(
+ Arc::from("edit_file"),
+ ToolRules {
+ default_mode: ToolPermissionMode::Allow,
+ always_allow: vec![],
+ always_deny: vec![],
+ always_confirm: vec![],
+ invalid_patterns: vec![],
+ },
+ );
+ let permissions = ToolPermissions { tools };
+
+ // Terminal with "dangerous" should be denied
+ let decision = decide_permission("terminal", "run dangerous command", &permissions, true);
+ assert!(matches!(decision, ToolPermissionDecision::Deny(_)));
+
+ // edit_file with "dangerous" should be allowed (no deny rules for edit_file)
+ let decision = decide_permission("edit_file", "dangerous_file.txt", &permissions, true);
+ assert_eq!(decision, ToolPermissionDecision::Allow);
+
+ // Terminal without "dangerous" should still be denied due to default_mode: Deny
+ let decision = decide_permission("terminal", "safe command", &permissions, true);
+ assert!(matches!(decision, ToolPermissionDecision::Deny(_)));
+ }
+
+ #[test]
+ fn test_invalid_patterns_block_tool() {
+ let mut tools = collections::HashMap::default();
+ tools.insert(
+ Arc::from("terminal"),
+ ToolRules {
+ default_mode: ToolPermissionMode::Allow,
+ always_allow: vec![CompiledRegex::new("echo", false).unwrap()],
+ always_deny: vec![],
+ always_confirm: vec![],
+ invalid_patterns: vec![InvalidRegexPattern {
+ pattern: "[invalid(regex".to_string(),
+ rule_type: "always_deny".to_string(),
+ error: "unclosed character class".to_string(),
+ }],
+ },
+ );
+ let permissions = ToolPermissions { tools };
+
+ // Even though "echo" matches always_allow, the tool should be blocked
+ // because there are invalid patterns
+ let decision = decide_permission("terminal", "echo hello", &permissions, true);
+ assert!(matches!(decision, ToolPermissionDecision::Deny(_)));
+
+ if let ToolPermissionDecision::Deny(msg) = decision {
+ assert!(
+ msg.contains("regex"),
+ "error message should mention regex: {}",
+ msg
+ );
+ assert!(
+ msg.contains("settings"),
+ "error message should mention settings: {}",
+ msg
+ );
+ assert!(
+ msg.contains("terminal"),
+ "error message should mention the tool name: {}",
+ msg
+ );
+ }
+ }
+
+ #[test]
+ fn test_same_pattern_in_deny_and_allow_deny_wins() {
+ // When the same pattern appears in both deny and allow lists, deny should win
+ let mut tools = collections::HashMap::default();
+ tools.insert(
+ Arc::from("terminal"),
+ ToolRules {
+ default_mode: ToolPermissionMode::Allow,
+ always_allow: vec![CompiledRegex::new("deploy", false).unwrap()],
+ always_deny: vec![CompiledRegex::new("deploy", false).unwrap()],
+ always_confirm: vec![],
+ invalid_patterns: vec![],
+ },
+ );
+ let permissions = ToolPermissions { tools };
+
+ let decision = decide_permission("terminal", "deploy production", &permissions, true);
+ assert!(matches!(decision, ToolPermissionDecision::Deny(_)));
+ }
+
+ #[test]
+ fn test_same_pattern_in_confirm_and_allow_confirm_wins() {
+ // When the same pattern appears in both confirm and allow lists, confirm should win
+ let mut tools = collections::HashMap::default();
+ tools.insert(
+ Arc::from("terminal"),
+ ToolRules {
+ default_mode: ToolPermissionMode::Allow,
+ always_allow: vec![CompiledRegex::new("deploy", false).unwrap()],
+ always_deny: vec![],
+ always_confirm: vec![CompiledRegex::new("deploy", false).unwrap()],
+ invalid_patterns: vec![],
+ },
+ );
+ let permissions = ToolPermissions { tools };
+
+ let decision = decide_permission("terminal", "deploy production", &permissions, true);
+ assert_eq!(decision, ToolPermissionDecision::Confirm);
+ }
+
+ #[test]
+ fn test_partial_tool_name_does_not_match() {
+ // Rules for "term" should not affect "terminal"
+ let mut tools = collections::HashMap::default();
+ tools.insert(
+ Arc::from("term"),
+ ToolRules {
+ default_mode: ToolPermissionMode::Deny,
+ always_allow: vec![],
+ always_deny: vec![],
+ always_confirm: vec![],
+ invalid_patterns: vec![],
+ },
+ );
+ let permissions = ToolPermissions { tools };
+
+ // "terminal" should not be affected by "term" rules, falls back to global setting
+ let decision = decide_permission("terminal", "echo hello", &permissions, true);
+ assert_eq!(decision, ToolPermissionDecision::Allow);
+
+ let decision = decide_permission("terminal", "echo hello", &permissions, false);
+ assert_eq!(decision, ToolPermissionDecision::Confirm);
+ }
+
+ #[test]
+ fn test_very_long_input() {
+ // Test that very long inputs are handled correctly
+ let permissions = terminal_rules_with_deny(&[r"\brm\b"]);
+
+ // Long input without the pattern should not match
+ let long_safe_input = "echo ".to_string() + &"a".repeat(100_000);
+ let decision = decide_permission("terminal", &long_safe_input, &permissions, true);
+ assert_eq!(decision, ToolPermissionDecision::Allow);
+
+ // Long input with the pattern should match
+ let long_dangerous_input = "a".repeat(50_000) + " rm " + &"b".repeat(50_000);
+ let decision = decide_permission("terminal", &long_dangerous_input, &permissions, true);
+ assert!(matches!(decision, ToolPermissionDecision::Deny(_)));
+ }
+}
@@ -1,10 +1,12 @@
use agent_client_protocol as acp;
+use agent_settings::AgentSettings;
use anyhow::Result;
use futures::FutureExt as _;
use gpui::{App, Entity, SharedString, Task};
use project::Project;
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};
+use settings::Settings;
use std::{
path::{Path, PathBuf},
rc::Rc,
@@ -13,7 +15,10 @@ use std::{
};
use util::markdown::MarkdownInlineCode;
-use crate::{AgentTool, ThreadEnvironment, ToolCallEventStream};
+use crate::{
+ AgentTool, ThreadEnvironment, ToolCallEventStream, ToolPermissionDecision,
+ decide_permission_from_settings,
+};
const COMMAND_OUTPUT_LIMIT: u64 = 16 * 1024;
@@ -103,9 +108,23 @@ impl AgentTool for TerminalTool {
Err(err) => return Task::ready(Err(err)),
};
- let authorize = event_stream.authorize(self.initial_title(Ok(input.clone()), cx), cx);
+ let settings = AgentSettings::get_global(cx);
+ let decision = decide_permission_from_settings("terminal", &input.command, settings);
+
+ let authorize = match decision {
+ ToolPermissionDecision::Allow => None,
+ ToolPermissionDecision::Deny(reason) => {
+ return Task::ready(Err(anyhow::anyhow!("{}", reason)));
+ }
+ ToolPermissionDecision::Confirm => {
+ // Use authorize_required since permission rules already determined confirmation is needed
+ Some(event_stream.authorize_required(self.initial_title(Ok(input.clone()), cx), cx))
+ }
+ };
cx.spawn(async move |cx| {
- authorize.await?;
+ if let Some(authorize) = authorize {
+ authorize.await?;
+ }
let terminal = self
.environment
@@ -162,12 +162,42 @@ pub struct ToolPermissions {
pub tools: collections::HashMap<Arc<str>, ToolRules>,
}
+impl ToolPermissions {
+ /// Returns all invalid regex patterns across all tools.
+ pub fn invalid_patterns(&self) -> Vec<&InvalidRegexPattern> {
+ self.tools
+ .values()
+ .flat_map(|rules| rules.invalid_patterns.iter())
+ .collect()
+ }
+
+ /// Returns true if any tool has invalid regex patterns.
+ pub fn has_invalid_patterns(&self) -> bool {
+ self.tools
+ .values()
+ .any(|rules| !rules.invalid_patterns.is_empty())
+ }
+}
+
+/// Represents a regex pattern that failed to compile.
+#[derive(Clone, Debug)]
+pub struct InvalidRegexPattern {
+ /// The pattern string that failed to compile.
+ pub pattern: String,
+ /// Which rule list this pattern was in (e.g., "always_deny", "always_allow", "always_confirm").
+ pub rule_type: String,
+ /// The error message from the regex compiler.
+ pub error: String,
+}
+
#[derive(Clone, Debug)]
pub struct ToolRules {
pub default_mode: ToolPermissionMode,
pub always_allow: Vec<CompiledRegex>,
pub always_deny: Vec<CompiledRegex>,
pub always_confirm: Vec<CompiledRegex>,
+ /// Patterns that failed to compile. If non-empty, tool calls should be blocked.
+ pub invalid_patterns: Vec<InvalidRegexPattern>,
}
impl Default for ToolRules {
@@ -177,6 +207,7 @@ impl Default for ToolRules {
always_allow: Vec::new(),
always_deny: Vec::new(),
always_confirm: Vec::new(),
+ invalid_patterns: Vec::new(),
}
}
}
@@ -199,15 +230,14 @@ impl std::fmt::Debug for CompiledRegex {
impl CompiledRegex {
pub fn new(pattern: &str, case_sensitive: bool) -> Option<Self> {
+ Self::try_new(pattern, case_sensitive).ok()
+ }
+
+ pub fn try_new(pattern: &str, case_sensitive: bool) -> Result<Self, regex::Error> {
let regex = regex::RegexBuilder::new(pattern)
.case_insensitive(!case_sensitive)
- .build()
- .map_err(|e| {
- log::warn!("Invalid regex pattern '{}': {}", pattern, e);
- e
- })
- .ok()?;
- Some(Self {
+ .build()?;
+ Ok(Self {
pattern: pattern.to_string(),
case_sensitive,
regex,
@@ -272,20 +302,47 @@ fn compile_tool_permissions(content: Option<settings::ToolPermissionsContent>) -
.tools
.into_iter()
.map(|(tool_name, rules_content)| {
- let rules = ToolRules {
- default_mode: rules_content.default_mode.unwrap_or_default(),
- always_allow: rules_content
- .always_allow
- .map(|v| compile_regex_rules(v.0))
- .unwrap_or_default(),
- always_deny: rules_content
- .always_deny
- .map(|v| compile_regex_rules(v.0))
- .unwrap_or_default(),
- always_confirm: rules_content
+ let mut invalid_patterns = Vec::new();
+
+ let (always_allow, allow_errors) = compile_regex_rules(
+ rules_content.always_allow.map(|v| v.0).unwrap_or_default(),
+ "always_allow",
+ );
+ invalid_patterns.extend(allow_errors);
+
+ let (always_deny, deny_errors) = compile_regex_rules(
+ rules_content.always_deny.map(|v| v.0).unwrap_or_default(),
+ "always_deny",
+ );
+ invalid_patterns.extend(deny_errors);
+
+ let (always_confirm, confirm_errors) = compile_regex_rules(
+ rules_content
.always_confirm
- .map(|v| compile_regex_rules(v.0))
+ .map(|v| v.0)
.unwrap_or_default(),
+ "always_confirm",
+ );
+ invalid_patterns.extend(confirm_errors);
+
+ // Log invalid patterns for debugging. Users will see an error when they
+ // attempt to use a tool with invalid patterns in their settings.
+ for invalid in &invalid_patterns {
+ log::error!(
+ "Invalid regex pattern in tool_permissions for '{}' tool ({}): '{}' - {}",
+ tool_name,
+ invalid.rule_type,
+ invalid.pattern,
+ invalid.error,
+ );
+ }
+
+ let rules = ToolRules {
+ default_mode: rules_content.default_mode.unwrap_or_default(),
+ always_allow,
+ always_deny,
+ always_confirm,
+ invalid_patterns,
};
(tool_name, rules)
})
@@ -294,11 +351,28 @@ fn compile_tool_permissions(content: Option<settings::ToolPermissionsContent>) -
ToolPermissions { tools }
}
-fn compile_regex_rules(rules: Vec<settings::ToolRegexRule>) -> Vec<CompiledRegex> {
- rules
- .into_iter()
- .filter_map(|rule| CompiledRegex::new(&rule.pattern, rule.case_sensitive.unwrap_or(false)))
- .collect()
+fn compile_regex_rules(
+ rules: Vec<settings::ToolRegexRule>,
+ rule_type: &str,
+) -> (Vec<CompiledRegex>, Vec<InvalidRegexPattern>) {
+ let mut compiled = Vec::new();
+ let mut errors = Vec::new();
+
+ for rule in rules {
+ let case_sensitive = rule.case_sensitive.unwrap_or(false);
+ match CompiledRegex::try_new(&rule.pattern, case_sensitive) {
+ Ok(regex) => compiled.push(regex),
+ Err(error) => {
+ errors.push(InvalidRegexPattern {
+ pattern: rule.pattern,
+ rule_type: rule_type.to_string(),
+ error: error.to_string(),
+ });
+ }
+ }
+ }
+
+ (compiled, errors)
}
#[cfg(test)]
@@ -448,13 +522,16 @@ mod tests {
}
#[test]
- fn test_invalid_regex_is_skipped_not_fail() {
+ fn test_invalid_regex_is_tracked_and_valid_ones_still_compile() {
let json = json!({
"tools": {
"terminal": {
"always_deny": [
{ "pattern": "[invalid(regex" },
{ "pattern": "valid_pattern" }
+ ],
+ "always_allow": [
+ { "pattern": "[another_bad" }
]
}
}
@@ -464,8 +541,32 @@ mod tests {
let permissions = compile_tool_permissions(Some(content));
let terminal = permissions.tools.get("terminal").unwrap();
+
+ // Valid patterns should still be compiled
assert_eq!(terminal.always_deny.len(), 1);
assert!(terminal.always_deny[0].is_match("valid_pattern"));
+
+ // Invalid patterns should be tracked (order depends on processing order)
+ assert_eq!(terminal.invalid_patterns.len(), 2);
+
+ let deny_invalid = terminal
+ .invalid_patterns
+ .iter()
+ .find(|p| p.rule_type == "always_deny")
+ .expect("should have invalid pattern from always_deny");
+ assert_eq!(deny_invalid.pattern, "[invalid(regex");
+ assert!(!deny_invalid.error.is_empty());
+
+ let allow_invalid = terminal
+ .invalid_patterns
+ .iter()
+ .find(|p| p.rule_type == "always_allow")
+ .expect("should have invalid pattern from always_allow");
+ assert_eq!(allow_invalid.pattern, "[another_bad");
+
+ // ToolPermissions helper methods should work
+ assert!(permissions.has_invalid_patterns());
+ assert_eq!(permissions.invalid_patterns().len(), 2);
}
#[test]