mirror of
https://github.com/zed-industries/zed.git
synced 2026-06-01 03:14:56 +07:00
Add granular tool permissions settings (#46112)
Adds granular per-tool permission settings for the Zed agent with regex-based rules. Release Notes: - N/A --------- Co-authored-by: Amp <amp@ampcode.com>
This commit is contained in:
Richard Feldman
GitHub
parent
0f75c079a5
commit
7a0b19b6b5
9 changed files with 699 additions and 861 deletions
2
Cargo.lock
generated
2
Cargo.lock
generated
|
|
@ -312,8 +312,10 @@ dependencies = [
|
|||
"fs",
|
||||
"gpui",
|
||||
"language_model",
|
||||
"log",
|
||||
"paths",
|
||||
"project",
|
||||
"regex",
|
||||
"schemars",
|
||||
"serde",
|
||||
"serde_json",
|
||||
|
|
|
|||
|
|
@ -961,6 +961,102 @@
|
|||
// Note: This setting has no effect on external agents that support permission modes, such as Claude Code.
|
||||
// You can set `agent_servers.claude.default_mode` to `bypassPermissions` to skip all permission requests.
|
||||
"always_allow_tool_actions": false,
|
||||
// Per-tool permission rules for granular control over tool actions.
|
||||
// This setting only applies to the native Zed agent.
|
||||
"tool_permissions": {
|
||||
"tools": {
|
||||
"terminal": {
|
||||
"default_mode": "confirm",
|
||||
"always_deny": [
|
||||
// Dangerous rm commands
|
||||
{ "pattern": "rm\\s+-rf\\s+(/|\\.\\.|\"|~|\\*)" },
|
||||
{ "pattern": "rm\\s+-rf\\s*$" },
|
||||
// Disk destruction
|
||||
{ "pattern": "> /dev/sd" },
|
||||
{ "pattern": "mkfs\\." },
|
||||
{ "pattern": "dd\\s+if=/dev/(zero|random)" },
|
||||
// Fork bomb
|
||||
{ "pattern": ":\\(\\)\\{\\s*:\\|:&\\s*\\};:" },
|
||||
// System files
|
||||
{ "pattern": "/etc/passwd" },
|
||||
{ "pattern": "/etc/shadow" },
|
||||
// Windows destructive commands
|
||||
{ "pattern": "del /f /s /q c:\\\\" },
|
||||
{ "pattern": "format c:" },
|
||||
{ "pattern": "rd /s /q" },
|
||||
],
|
||||
"always_confirm": [
|
||||
// File deletion
|
||||
{ "pattern": "rm\\s" },
|
||||
// Destructive git operations
|
||||
{ "pattern": "git\\s+(reset|clean)\\s+--hard" },
|
||||
{ "pattern": "git\\s+push\\s+(-f|--force)" },
|
||||
// Database operations
|
||||
{ "pattern": "DROP\\s+TABLE", "case_sensitive": true },
|
||||
{ "pattern": "DELETE\\s+FROM", "case_sensitive": true },
|
||||
// Privileged commands
|
||||
{ "pattern": "sudo\\s" },
|
||||
],
|
||||
"always_allow": [
|
||||
// Build and test commands
|
||||
{ "pattern": "^cargo\\s+(build|test|check|clippy|run)" },
|
||||
{ "pattern": "^npm\\s+(test|run|install)" },
|
||||
{ "pattern": "^pnpm\\s+(test|run|install)" },
|
||||
{ "pattern": "^yarn\\s+(test|run|install)" },
|
||||
// Safe read-only commands
|
||||
{ "pattern": "^ls(\\s|$)" },
|
||||
{ "pattern": "^cat\\s" },
|
||||
{ "pattern": "^head\\s" },
|
||||
{ "pattern": "^tail\\s" },
|
||||
{ "pattern": "^grep\\s" },
|
||||
{ "pattern": "^find\\s" },
|
||||
// Safe git commands
|
||||
{ "pattern": "^git\\s+(status|log|diff|branch|show)" },
|
||||
],
|
||||
},
|
||||
"edit_file": {
|
||||
"default_mode": "confirm",
|
||||
"always_deny": [
|
||||
// Secrets and credentials
|
||||
{ "pattern": "\\.env($|\\.)" },
|
||||
{ "pattern": "secrets?/" },
|
||||
{ "pattern": "\\.pem$" },
|
||||
{ "pattern": "\\.key$" },
|
||||
],
|
||||
},
|
||||
"delete_path": {
|
||||
"default_mode": "confirm",
|
||||
"always_deny": [
|
||||
// System directories
|
||||
{ "pattern": "^/etc" },
|
||||
{ "pattern": "^/usr" },
|
||||
{ "pattern": "^/bin" },
|
||||
{ "pattern": "^/sbin" },
|
||||
{ "pattern": "^/var" },
|
||||
{ "pattern": "^/boot" },
|
||||
{ "pattern": "^/root" },
|
||||
// Home directory root
|
||||
{ "pattern": "^~$" },
|
||||
{ "pattern": "^/Users/[^/]+$" },
|
||||
{ "pattern": "^/home/[^/]+$" },
|
||||
// Git directory
|
||||
{ "pattern": "\\.git/?$" },
|
||||
// Windows system directories
|
||||
{ "pattern": "^C:\\\\Windows" },
|
||||
{ "pattern": "^C:\\\\Program Files" },
|
||||
],
|
||||
},
|
||||
"fetch": {
|
||||
"default_mode": "confirm",
|
||||
"always_allow": [
|
||||
// Common documentation sites
|
||||
{ "pattern": "^https://(docs\\.|api\\.)?github\\.com" },
|
||||
{ "pattern": "^https://docs\\.rs" },
|
||||
{ "pattern": "^https://crates\\.io" },
|
||||
],
|
||||
},
|
||||
},
|
||||
},
|
||||
// When enabled, agent edits will be displayed in single-file editors for review
|
||||
"single_file_review": true,
|
||||
// When enabled, show voting thumbs for feedback on agent edits.
|
||||
|
|
|
|||
|
|
@ -20,7 +20,9 @@ convert_case.workspace = true
|
|||
fs.workspace = true
|
||||
gpui.workspace = true
|
||||
language_model.workspace = true
|
||||
log.workspace = true
|
||||
project.workspace = true
|
||||
regex.workspace = true
|
||||
schemars.workspace = true
|
||||
serde.workspace = true
|
||||
settings.workspace = true
|
||||
|
|
|
|||
|
|
@ -11,7 +11,7 @@ use schemars::JsonSchema;
|
|||
use serde::{Deserialize, Serialize};
|
||||
use settings::{
|
||||
DefaultAgentView, DockPosition, DockSide, LanguageModelParameters, LanguageModelSelection,
|
||||
NotifyWhenAgentWaiting, RegisterSetting, Settings,
|
||||
NotifyWhenAgentWaiting, RegisterSetting, Settings, ToolPermissionMode,
|
||||
};
|
||||
|
||||
pub use crate::agent_profile::*;
|
||||
|
|
@ -49,6 +49,7 @@ pub struct AgentSettings {
|
|||
pub expand_terminal_card: bool,
|
||||
pub use_modifier_to_send: bool,
|
||||
pub message_editor_min_lines: usize,
|
||||
pub tool_permissions: ToolPermissions,
|
||||
}
|
||||
|
||||
impl AgentSettings {
|
||||
|
|
@ -155,6 +156,68 @@ impl Default for AgentProfileId {
|
|||
}
|
||||
}
|
||||
|
||||
#[derive(Clone, Debug, Default)]
|
||||
pub struct ToolPermissions {
|
||||
pub tools: collections::HashMap<Arc<str>, ToolRules>,
|
||||
}
|
||||
|
||||
#[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>,
|
||||
}
|
||||
|
||||
impl Default for ToolRules {
|
||||
fn default() -> Self {
|
||||
Self {
|
||||
default_mode: ToolPermissionMode::Confirm,
|
||||
always_allow: Vec::new(),
|
||||
always_deny: Vec::new(),
|
||||
always_confirm: Vec::new(),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
pub struct CompiledRegex {
|
||||
pub pattern: String,
|
||||
pub case_sensitive: bool,
|
||||
pub regex: regex::Regex,
|
||||
}
|
||||
|
||||
impl std::fmt::Debug for CompiledRegex {
|
||||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||
f.debug_struct("CompiledRegex")
|
||||
.field("pattern", &self.pattern)
|
||||
.field("case_sensitive", &self.case_sensitive)
|
||||
.finish()
|
||||
}
|
||||
}
|
||||
|
||||
impl CompiledRegex {
|
||||
pub fn new(pattern: &str, case_sensitive: bool) -> Option<Self> {
|
||||
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 {
|
||||
pattern: pattern.to_string(),
|
||||
case_sensitive,
|
||||
regex,
|
||||
})
|
||||
}
|
||||
|
||||
pub fn is_match(&self, input: &str) -> bool {
|
||||
self.regex.is_match(input)
|
||||
}
|
||||
}
|
||||
|
||||
impl Settings for AgentSettings {
|
||||
fn from_settings(content: &settings::SettingsContent) -> Self {
|
||||
let agent = content.agent.clone().unwrap();
|
||||
|
|
@ -193,6 +256,467 @@ impl Settings for AgentSettings {
|
|||
expand_terminal_card: agent.expand_terminal_card.unwrap(),
|
||||
use_modifier_to_send: agent.use_modifier_to_send.unwrap(),
|
||||
message_editor_min_lines: agent.message_editor_min_lines.unwrap(),
|
||||
tool_permissions: compile_tool_permissions(agent.tool_permissions),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn compile_tool_permissions(content: Option<settings::ToolPermissionsContent>) -> ToolPermissions {
|
||||
let Some(content) = content else {
|
||||
return ToolPermissions::default();
|
||||
};
|
||||
|
||||
let tools = content
|
||||
.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
|
||||
.always_confirm
|
||||
.map(|v| compile_regex_rules(v.0))
|
||||
.unwrap_or_default(),
|
||||
};
|
||||
(tool_name, rules)
|
||||
})
|
||||
.collect();
|
||||
|
||||
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()
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use serde_json::json;
|
||||
use settings::ToolPermissionsContent;
|
||||
|
||||
#[test]
|
||||
fn test_compiled_regex_case_insensitive() {
|
||||
let regex = CompiledRegex::new("rm\\s+-rf", false).unwrap();
|
||||
assert!(regex.is_match("rm -rf /"));
|
||||
assert!(regex.is_match("RM -RF /"));
|
||||
assert!(regex.is_match("Rm -Rf /"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_compiled_regex_case_sensitive() {
|
||||
let regex = CompiledRegex::new("DROP\\s+TABLE", true).unwrap();
|
||||
assert!(regex.is_match("DROP TABLE users"));
|
||||
assert!(!regex.is_match("drop table users"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_invalid_regex_returns_none() {
|
||||
let result = CompiledRegex::new("[invalid(regex", false);
|
||||
assert!(result.is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_tool_permissions_parsing() {
|
||||
let json = json!({
|
||||
"tools": {
|
||||
"terminal": {
|
||||
"default_mode": "allow",
|
||||
"always_deny": [
|
||||
{ "pattern": "rm\\s+-rf" }
|
||||
],
|
||||
"always_allow": [
|
||||
{ "pattern": "^git\\s" }
|
||||
]
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
let content: ToolPermissionsContent = serde_json::from_value(json).unwrap();
|
||||
let permissions = compile_tool_permissions(Some(content));
|
||||
|
||||
let terminal_rules = permissions.tools.get("terminal").unwrap();
|
||||
assert_eq!(terminal_rules.default_mode, ToolPermissionMode::Allow);
|
||||
assert_eq!(terminal_rules.always_deny.len(), 1);
|
||||
assert_eq!(terminal_rules.always_allow.len(), 1);
|
||||
assert!(terminal_rules.always_deny[0].is_match("rm -rf /"));
|
||||
assert!(terminal_rules.always_allow[0].is_match("git status"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_tool_rules_default_mode() {
|
||||
let json = json!({
|
||||
"tools": {
|
||||
"edit_file": {
|
||||
"default_mode": "deny"
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
let content: ToolPermissionsContent = serde_json::from_value(json).unwrap();
|
||||
let permissions = compile_tool_permissions(Some(content));
|
||||
|
||||
let rules = permissions.tools.get("edit_file").unwrap();
|
||||
assert_eq!(rules.default_mode, ToolPermissionMode::Deny);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_tool_permissions_empty() {
|
||||
let permissions = compile_tool_permissions(None);
|
||||
assert!(permissions.tools.is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_tool_rules_default_returns_confirm() {
|
||||
let default_rules = ToolRules::default();
|
||||
assert_eq!(default_rules.default_mode, ToolPermissionMode::Confirm);
|
||||
assert!(default_rules.always_allow.is_empty());
|
||||
assert!(default_rules.always_deny.is_empty());
|
||||
assert!(default_rules.always_confirm.is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_tool_permissions_with_multiple_tools() {
|
||||
let json = json!({
|
||||
"tools": {
|
||||
"terminal": {
|
||||
"default_mode": "allow",
|
||||
"always_deny": [{ "pattern": "rm\\s+-rf" }]
|
||||
},
|
||||
"edit_file": {
|
||||
"default_mode": "confirm",
|
||||
"always_deny": [{ "pattern": "\\.env$" }]
|
||||
},
|
||||
"delete_path": {
|
||||
"default_mode": "deny"
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
let content: ToolPermissionsContent = serde_json::from_value(json).unwrap();
|
||||
let permissions = compile_tool_permissions(Some(content));
|
||||
|
||||
assert_eq!(permissions.tools.len(), 3);
|
||||
|
||||
let terminal = permissions.tools.get("terminal").unwrap();
|
||||
assert_eq!(terminal.default_mode, ToolPermissionMode::Allow);
|
||||
assert_eq!(terminal.always_deny.len(), 1);
|
||||
|
||||
let edit_file = permissions.tools.get("edit_file").unwrap();
|
||||
assert_eq!(edit_file.default_mode, ToolPermissionMode::Confirm);
|
||||
assert!(edit_file.always_deny[0].is_match("secrets.env"));
|
||||
|
||||
let delete_path = permissions.tools.get("delete_path").unwrap();
|
||||
assert_eq!(delete_path.default_mode, ToolPermissionMode::Deny);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_tool_permissions_with_all_rule_types() {
|
||||
let json = json!({
|
||||
"tools": {
|
||||
"terminal": {
|
||||
"always_deny": [{ "pattern": "rm\\s+-rf" }],
|
||||
"always_confirm": [{ "pattern": "sudo\\s" }],
|
||||
"always_allow": [{ "pattern": "^git\\s+status" }]
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
let content: ToolPermissionsContent = serde_json::from_value(json).unwrap();
|
||||
let permissions = compile_tool_permissions(Some(content));
|
||||
|
||||
let terminal = permissions.tools.get("terminal").unwrap();
|
||||
assert_eq!(terminal.always_deny.len(), 1);
|
||||
assert_eq!(terminal.always_confirm.len(), 1);
|
||||
assert_eq!(terminal.always_allow.len(), 1);
|
||||
|
||||
assert!(terminal.always_deny[0].is_match("rm -rf /"));
|
||||
assert!(terminal.always_confirm[0].is_match("sudo apt install"));
|
||||
assert!(terminal.always_allow[0].is_match("git status"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_invalid_regex_is_skipped_not_fail() {
|
||||
let json = json!({
|
||||
"tools": {
|
||||
"terminal": {
|
||||
"always_deny": [
|
||||
{ "pattern": "[invalid(regex" },
|
||||
{ "pattern": "valid_pattern" }
|
||||
]
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
let content: ToolPermissionsContent = serde_json::from_value(json).unwrap();
|
||||
let permissions = compile_tool_permissions(Some(content));
|
||||
|
||||
let terminal = permissions.tools.get("terminal").unwrap();
|
||||
assert_eq!(terminal.always_deny.len(), 1);
|
||||
assert!(terminal.always_deny[0].is_match("valid_pattern"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_default_json_tool_permissions_parse() {
|
||||
let default_json = include_str!("../../../assets/settings/default.json");
|
||||
|
||||
let value: serde_json::Value = serde_json_lenient::from_str(default_json)
|
||||
.expect("default.json should be valid JSON with comments");
|
||||
|
||||
let agent = value
|
||||
.get("agent")
|
||||
.expect("default.json should have 'agent' key");
|
||||
let tool_permissions = agent
|
||||
.get("tool_permissions")
|
||||
.expect("agent should have 'tool_permissions' key");
|
||||
|
||||
let content: ToolPermissionsContent = serde_json::from_value(tool_permissions.clone())
|
||||
.expect("tool_permissions should parse into ToolPermissionsContent");
|
||||
|
||||
let permissions = compile_tool_permissions(Some(content));
|
||||
|
||||
let terminal = permissions
|
||||
.tools
|
||||
.get("terminal")
|
||||
.expect("terminal tool should be configured");
|
||||
assert!(
|
||||
!terminal.always_deny.is_empty(),
|
||||
"terminal should have deny rules"
|
||||
);
|
||||
assert!(
|
||||
!terminal.always_confirm.is_empty(),
|
||||
"terminal should have confirm rules"
|
||||
);
|
||||
assert!(
|
||||
!terminal.always_allow.is_empty(),
|
||||
"terminal should have allow rules"
|
||||
);
|
||||
|
||||
let edit_file = permissions
|
||||
.tools
|
||||
.get("edit_file")
|
||||
.expect("edit_file tool should be configured");
|
||||
assert!(
|
||||
!edit_file.always_deny.is_empty(),
|
||||
"edit_file should have deny rules"
|
||||
);
|
||||
|
||||
let delete_path = permissions
|
||||
.tools
|
||||
.get("delete_path")
|
||||
.expect("delete_path tool should be configured");
|
||||
assert!(
|
||||
!delete_path.always_deny.is_empty(),
|
||||
"delete_path should have deny rules"
|
||||
);
|
||||
|
||||
let fetch = permissions
|
||||
.tools
|
||||
.get("fetch")
|
||||
.expect("fetch tool should be configured");
|
||||
assert!(
|
||||
!fetch.always_allow.is_empty(),
|
||||
"fetch should have allow rules"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_default_deny_rules_match_dangerous_commands() {
|
||||
let default_json = include_str!("../../../assets/settings/default.json");
|
||||
let value: serde_json::Value = serde_json_lenient::from_str(default_json).unwrap();
|
||||
let tool_permissions = value["agent"]["tool_permissions"].clone();
|
||||
let content: ToolPermissionsContent = serde_json::from_value(tool_permissions).unwrap();
|
||||
let permissions = compile_tool_permissions(Some(content));
|
||||
|
||||
let terminal = permissions.tools.get("terminal").unwrap();
|
||||
|
||||
let dangerous_commands = [
|
||||
"rm -rf /",
|
||||
"rm -rf ~",
|
||||
"rm -rf ..",
|
||||
"mkfs.ext4 /dev/sda",
|
||||
"dd if=/dev/zero of=/dev/sda",
|
||||
"cat /etc/passwd",
|
||||
"cat /etc/shadow",
|
||||
"del /f /s /q c:\\",
|
||||
"format c:",
|
||||
"rd /s /q c:\\windows",
|
||||
];
|
||||
|
||||
for cmd in &dangerous_commands {
|
||||
assert!(
|
||||
terminal.always_deny.iter().any(|r| r.is_match(cmd)),
|
||||
"Command '{}' should be blocked by deny rules",
|
||||
cmd
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_default_allow_rules_match_safe_commands() {
|
||||
let default_json = include_str!("../../../assets/settings/default.json");
|
||||
let value: serde_json::Value = serde_json_lenient::from_str(default_json).unwrap();
|
||||
let tool_permissions = value["agent"]["tool_permissions"].clone();
|
||||
let content: ToolPermissionsContent = serde_json::from_value(tool_permissions).unwrap();
|
||||
let permissions = compile_tool_permissions(Some(content));
|
||||
|
||||
let terminal = permissions.tools.get("terminal").unwrap();
|
||||
|
||||
let safe_commands = [
|
||||
"cargo build",
|
||||
"cargo test",
|
||||
"cargo check",
|
||||
"npm test",
|
||||
"pnpm install",
|
||||
"yarn run build",
|
||||
"ls",
|
||||
"ls -la",
|
||||
"cat file.txt",
|
||||
"git status",
|
||||
"git log",
|
||||
"git diff",
|
||||
];
|
||||
|
||||
for cmd in &safe_commands {
|
||||
assert!(
|
||||
terminal.always_allow.iter().any(|r| r.is_match(cmd)),
|
||||
"Command '{}' should be allowed by allow rules",
|
||||
cmd
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_deny_takes_precedence_over_allow_and_confirm() {
|
||||
let json = json!({
|
||||
"tools": {
|
||||
"terminal": {
|
||||
"default_mode": "allow",
|
||||
"always_deny": [{ "pattern": "dangerous" }],
|
||||
"always_confirm": [{ "pattern": "dangerous" }],
|
||||
"always_allow": [{ "pattern": "dangerous" }]
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
let content: ToolPermissionsContent = serde_json::from_value(json).unwrap();
|
||||
let permissions = compile_tool_permissions(Some(content));
|
||||
let terminal = permissions.tools.get("terminal").unwrap();
|
||||
|
||||
assert!(
|
||||
terminal.always_deny[0].is_match("run dangerous command"),
|
||||
"Deny rule should match"
|
||||
);
|
||||
assert!(
|
||||
terminal.always_allow[0].is_match("run dangerous command"),
|
||||
"Allow rule should also match (but deny takes precedence at evaluation time)"
|
||||
);
|
||||
assert!(
|
||||
terminal.always_confirm[0].is_match("run dangerous command"),
|
||||
"Confirm rule should also match (but deny takes precedence at evaluation time)"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_confirm_takes_precedence_over_allow() {
|
||||
let json = json!({
|
||||
"tools": {
|
||||
"terminal": {
|
||||
"default_mode": "allow",
|
||||
"always_confirm": [{ "pattern": "risky" }],
|
||||
"always_allow": [{ "pattern": "risky" }]
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
let content: ToolPermissionsContent = serde_json::from_value(json).unwrap();
|
||||
let permissions = compile_tool_permissions(Some(content));
|
||||
let terminal = permissions.tools.get("terminal").unwrap();
|
||||
|
||||
assert!(
|
||||
terminal.always_confirm[0].is_match("do risky thing"),
|
||||
"Confirm rule should match"
|
||||
);
|
||||
assert!(
|
||||
terminal.always_allow[0].is_match("do risky thing"),
|
||||
"Allow rule should also match (but confirm takes precedence at evaluation time)"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_regex_matches_anywhere_in_string_not_just_anchored() {
|
||||
let json = json!({
|
||||
"tools": {
|
||||
"terminal": {
|
||||
"always_deny": [
|
||||
{ "pattern": "rm\\s+-rf" },
|
||||
{ "pattern": "/etc/passwd" }
|
||||
]
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
let content: ToolPermissionsContent = serde_json::from_value(json).unwrap();
|
||||
let permissions = compile_tool_permissions(Some(content));
|
||||
let terminal = permissions.tools.get("terminal").unwrap();
|
||||
|
||||
assert!(
|
||||
terminal.always_deny[0].is_match("echo hello && rm -rf /"),
|
||||
"Should match rm -rf in the middle of a command chain"
|
||||
);
|
||||
assert!(
|
||||
terminal.always_deny[0].is_match("cd /tmp; rm -rf *"),
|
||||
"Should match rm -rf after semicolon"
|
||||
);
|
||||
assert!(
|
||||
terminal.always_deny[1].is_match("cat /etc/passwd | grep root"),
|
||||
"Should match /etc/passwd in a pipeline"
|
||||
);
|
||||
assert!(
|
||||
terminal.always_deny[1].is_match("vim /etc/passwd"),
|
||||
"Should match /etc/passwd as argument"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_fork_bomb_pattern_matches() {
|
||||
let fork_bomb_regex = CompiledRegex::new(r":\(\)\{\s*:\|:&\s*\};:", false).unwrap();
|
||||
assert!(
|
||||
fork_bomb_regex.is_match(":(){ :|:& };:"),
|
||||
"Should match the classic fork bomb"
|
||||
);
|
||||
assert!(
|
||||
fork_bomb_regex.is_match(":(){ :|:&};:"),
|
||||
"Should match fork bomb without spaces"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_default_json_fork_bomb_pattern_matches() {
|
||||
let default_json = include_str!("../../../assets/settings/default.json");
|
||||
let value: serde_json::Value = serde_json_lenient::from_str(default_json).unwrap();
|
||||
let tool_permissions = value["agent"]["tool_permissions"].clone();
|
||||
let content: ToolPermissionsContent = serde_json::from_value(tool_permissions).unwrap();
|
||||
let permissions = compile_tool_permissions(Some(content));
|
||||
|
||||
let terminal = permissions.tools.get("terminal").unwrap();
|
||||
|
||||
assert!(
|
||||
terminal
|
||||
.always_deny
|
||||
.iter()
|
||||
.any(|r| r.is_match(":(){ :|:& };:")),
|
||||
"Default deny rules should block the classic fork bomb"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -482,6 +482,7 @@ mod tests {
|
|||
expand_terminal_card: true,
|
||||
use_modifier_to_send: true,
|
||||
message_editor_min_lines: 1,
|
||||
tool_permissions: Default::default(),
|
||||
};
|
||||
|
||||
cx.update(|cx| {
|
||||
|
|
|
|||
|
|
@ -24,6 +24,12 @@ impl FeatureFlag for AcpBetaFeatureFlag {
|
|||
const NAME: &'static str = "acp-beta";
|
||||
}
|
||||
|
||||
pub struct ToolPermissionsFeatureFlag;
|
||||
|
||||
impl FeatureFlag for ToolPermissionsFeatureFlag {
|
||||
const NAME: &'static str = "tool-permissions";
|
||||
}
|
||||
|
||||
pub struct AgentSharingFeatureFlag;
|
||||
|
||||
impl FeatureFlag for AgentSharingFeatureFlag {
|
||||
|
|
|
|||
|
|
@ -3,7 +3,10 @@ use gpui::SharedString;
|
|||
use schemars::{JsonSchema, json_schema};
|
||||
use serde::{Deserialize, Serialize};
|
||||
use settings_macros::{MergeFrom, with_fallible_options};
|
||||
use std::{borrow::Cow, path::PathBuf, sync::Arc};
|
||||
use std::sync::Arc;
|
||||
use std::{borrow::Cow, path::PathBuf};
|
||||
|
||||
use crate::ExtendingVec;
|
||||
|
||||
use crate::{DockPosition, DockSide};
|
||||
|
||||
|
|
@ -119,6 +122,11 @@ pub struct AgentSettingsContent {
|
|||
///
|
||||
/// Default: 4
|
||||
pub message_editor_min_lines: Option<usize>,
|
||||
/// Per-tool permission rules for granular control over which tool actions require confirmation.
|
||||
///
|
||||
/// This setting only applies to the native Zed agent. External agent servers (Claude Code, Gemini CLI, etc.)
|
||||
/// have their own permission systems and are not affected by these settings.
|
||||
pub tool_permissions: Option<ToolPermissionsContent>,
|
||||
}
|
||||
|
||||
impl AgentSettingsContent {
|
||||
|
|
@ -466,3 +474,61 @@ pub enum CustomAgentServerSettings {
|
|||
favorite_config_option_values: HashMap<String, Vec<String>>,
|
||||
},
|
||||
}
|
||||
|
||||
#[with_fallible_options]
|
||||
#[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize, JsonSchema, MergeFrom)]
|
||||
pub struct ToolPermissionsContent {
|
||||
/// Per-tool permission rules.
|
||||
/// Keys: terminal, edit_file, delete_path, move_path, create_directory,
|
||||
/// save_file, fetch, web_search
|
||||
#[serde(default)]
|
||||
pub tools: HashMap<Arc<str>, ToolRulesContent>,
|
||||
}
|
||||
|
||||
#[with_fallible_options]
|
||||
#[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize, JsonSchema, MergeFrom)]
|
||||
pub struct ToolRulesContent {
|
||||
/// Default mode when no regex rules match.
|
||||
/// Default: confirm
|
||||
pub default_mode: Option<ToolPermissionMode>,
|
||||
|
||||
/// Regexes for inputs to auto-approve.
|
||||
/// For terminal: matches command. For file tools: matches path. For fetch: matches URL.
|
||||
/// Default: []
|
||||
pub always_allow: Option<ExtendingVec<ToolRegexRule>>,
|
||||
|
||||
/// Regexes for inputs to auto-reject.
|
||||
/// **SECURITY**: These take precedence over ALL other rules, across ALL settings layers.
|
||||
/// Default: []
|
||||
pub always_deny: Option<ExtendingVec<ToolRegexRule>>,
|
||||
|
||||
/// Regexes for inputs that must always prompt.
|
||||
/// Takes precedence over always_allow but not always_deny.
|
||||
/// Default: []
|
||||
pub always_confirm: Option<ExtendingVec<ToolRegexRule>>,
|
||||
}
|
||||
|
||||
#[with_fallible_options]
|
||||
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize, JsonSchema, MergeFrom)]
|
||||
pub struct ToolRegexRule {
|
||||
/// The regex pattern to match.
|
||||
pub pattern: String,
|
||||
|
||||
/// Whether the regex is case-sensitive.
|
||||
/// Default: false (case-insensitive)
|
||||
pub case_sensitive: Option<bool>,
|
||||
}
|
||||
|
||||
#[derive(
|
||||
Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize, JsonSchema, MergeFrom,
|
||||
)]
|
||||
#[serde(rename_all = "snake_case")]
|
||||
pub enum ToolPermissionMode {
|
||||
/// Auto-approve without prompting.
|
||||
Allow,
|
||||
/// Auto-reject with an error.
|
||||
Deny,
|
||||
/// Always prompt for confirmation (default behavior).
|
||||
#[default]
|
||||
Confirm,
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,198 +0,0 @@
|
|||
# Visual Test Plan: Agent Panel Image Rendering
|
||||
|
||||
## 🎯 The Goal
|
||||
|
||||
We want a visual regression test that **catches bugs in how `read_file` displays images**.
|
||||
|
||||
If someone changes the code in `ReadFileTool` or the UI rendering in `thread_view.rs`, this test should fail and show us visually what changed.
|
||||
|
||||
## ⚠️ Current Problem: The Test is Useless
|
||||
|
||||
**The current test in `crates/zed/src/visual_test_runner.rs` does NOT test the real code!**
|
||||
|
||||
Here's what it does now (WRONG):
|
||||
1. Creates a `StubAgentConnection`
|
||||
2. Hard-codes a fake tool call response with pre-baked image data
|
||||
3. Injects that directly into `AcpThread`
|
||||
4. Takes a screenshot
|
||||
|
||||
**Why this is useless:** If you change how `ReadFileTool` produces its output (in `crates/agent/src/tools/read_file_tool.rs`), the test will still pass because it never runs that code! The test bypasses the entire tool execution pipeline.
|
||||
|
||||
## ✅ What We Actually Need
|
||||
|
||||
The test should:
|
||||
1. Create a real project with a real image file
|
||||
2. Actually run the real `ReadFileTool::run()` method
|
||||
3. Let the tool produce its real output via `event_stream.update_fields()`
|
||||
4. Have that real output flow through to `AcpThread` and render in the UI
|
||||
5. Take a screenshot of the real rendered result
|
||||
|
||||
This way, if someone changes `ReadFileTool` or the UI rendering, the test will catch it.
|
||||
|
||||
## 📚 Architecture Background (For Newcomers)
|
||||
|
||||
Here's how the agent system works:
|
||||
|
||||
### The Two "Thread" Types
|
||||
- **`Thread`** (in `crates/agent/src/thread.rs`) - Runs tools, talks to LLMs, produces events
|
||||
- **`AcpThread`** (in `crates/acp_thread/src/acp_thread.rs`) - Receives events and stores data for UI rendering
|
||||
|
||||
### How Tools Work
|
||||
1. `Thread` has registered tools (like `ReadFileTool`)
|
||||
2. When a tool runs, it gets a `ToolCallEventStream`
|
||||
3. The tool calls `event_stream.update_fields(...)` to send updates
|
||||
4. Those updates become `ThreadEvent::ToolCallUpdate` events
|
||||
5. Events flow to `AcpThread` via `handle_thread_events()` in `NativeAgentConnection`
|
||||
6. `AcpThread` stores the data and the UI renders it
|
||||
|
||||
### The Key File Locations
|
||||
- **Tool implementation:** `crates/agent/src/tools/read_file_tool.rs`
|
||||
- Lines 163-188: Image file handling (calls `event_stream.update_fields()`)
|
||||
- **Event stream:** `crates/agent/src/thread.rs`
|
||||
- `ToolCallEventStream::update_fields()` - sends updates
|
||||
- `ToolCallEventStream::test()` - creates a test event stream
|
||||
- **UI rendering:** `crates/agent_ui/src/acp/thread_view.rs`
|
||||
- `render_image_output()` - renders images in tool call output
|
||||
- **Current (broken) test:** `crates/zed/src/visual_test_runner.rs`
|
||||
- `run_agent_thread_view_test()` - the function that needs fixing
|
||||
|
||||
## 🔧 Implementation Plan
|
||||
|
||||
### Option A: Direct Tool Invocation (Recommended)
|
||||
|
||||
Run the real tool and capture its output:
|
||||
|
||||
```rust
|
||||
// 1. Create a project with a real image file
|
||||
let fs = FakeFs::new(cx.executor());
|
||||
fs.insert_file("/project/test-image.png", EMBEDDED_TEST_IMAGE.to_vec()).await;
|
||||
let project = Project::test(fs.clone(), ["/project"], cx).await;
|
||||
|
||||
// 2. Create the ReadFileTool (needs Thread, ActionLog)
|
||||
let action_log = cx.new(|_| ActionLog::new(project.clone()));
|
||||
// ... create Thread with project ...
|
||||
let tool = Arc::new(ReadFileTool::new(thread.downgrade(), project.clone(), action_log));
|
||||
|
||||
// 3. Run the tool and capture events
|
||||
let (event_stream, mut event_receiver) = ToolCallEventStream::test();
|
||||
let input = ReadFileToolInput {
|
||||
path: "project/test-image.png".to_string(),
|
||||
start_line: None,
|
||||
end_line: None,
|
||||
};
|
||||
tool.run(input, event_stream, cx).await?;
|
||||
|
||||
// 4. Collect the ToolCallUpdateFields that the tool produced
|
||||
let updates = event_receiver.collect_updates();
|
||||
|
||||
// 5. Create an AcpThread and inject the real tool output
|
||||
// ... create AcpThread ...
|
||||
acp_thread.update(cx, |thread, cx| {
|
||||
// First create the tool call entry
|
||||
thread.upsert_tool_call(initial_tool_call, cx)?;
|
||||
// Then update it with the real output from the tool
|
||||
for update in updates {
|
||||
thread.update_tool_call(update, cx)?;
|
||||
}
|
||||
})?;
|
||||
|
||||
// 6. Render and screenshot
|
||||
```
|
||||
|
||||
### Required Exports
|
||||
|
||||
The `agent` crate needs to export these for the visual test:
|
||||
- `ReadFileTool` and `ReadFileToolInput`
|
||||
- `ToolCallEventStream::test()` (already has `#[cfg(feature = "test-support")]`)
|
||||
- `Thread` (to create the tool)
|
||||
|
||||
Check `crates/agent/src/lib.rs` and add exports if needed.
|
||||
|
||||
### Required Dependencies in `crates/zed/Cargo.toml`
|
||||
|
||||
The `visual-tests` feature needs:
|
||||
```toml
|
||||
"agent/test-support" # For ToolCallEventStream::test() and tool exports
|
||||
```
|
||||
|
||||
### Option B: Use NativeAgentConnection with Fake Model
|
||||
|
||||
Alternatively, use the full agent flow with a fake LLM:
|
||||
|
||||
1. Create `NativeAgentServer` with a `FakeLanguageModel`
|
||||
2. Program the fake model to return a tool call for `read_file`
|
||||
3. Let the real agent flow execute the tool
|
||||
4. The tool runs, produces output, flows through to UI
|
||||
|
||||
This is more complex but tests more of the real code path.
|
||||
|
||||
## 📋 Step-by-Step Implementation Checklist
|
||||
|
||||
### Phase 1: Enable Tool Access
|
||||
- [x] Add `agent/test-support` to `visual-tests` feature in `crates/zed/Cargo.toml`
|
||||
- [x] Verify `ReadFileTool`, `ReadFileToolInput`, `ToolCallEventStream::test()` are exported
|
||||
- [x] Added additional required features: `language_model/test-support`, `fs/test-support`, `action_log`
|
||||
|
||||
### Phase 2: Rewrite the Test
|
||||
- [x] In `run_agent_thread_view_test()`, remove the fake stub response
|
||||
- [x] Create a real temp directory with a real image file (FakeFs doesn't work in visual test runner)
|
||||
- [x] Create the real `ReadFileTool` with Thread, ActionLog, etc.
|
||||
- [x] Run the tool with `ToolCallEventStream::test()`
|
||||
- [x] Capture the `ToolCallUpdateFields` it produces
|
||||
- [x] Use the real tool output to populate the stub connection's response
|
||||
|
||||
### Phase 3: Verify It Works
|
||||
- [x] Run `UPDATE_BASELINE=1 cargo run -p zed --bin visual_test_runner --features visual-tests`
|
||||
- [x] Check the screenshot shows the real tool output
|
||||
- [x] Intentionally break `read_file_tool.rs` (comment out `event_stream.update_fields`)
|
||||
- [x] Verified the test fails with: "ReadFileTool did not produce any content - the tool is broken!"
|
||||
- [x] Restored the code and verified test passes again
|
||||
|
||||
## 🧪 How to Verify the Test is Actually Testing Real Code
|
||||
|
||||
After implementing, do this sanity check:
|
||||
|
||||
1. In `crates/agent/src/tools/read_file_tool.rs`, comment out lines 181-185:
|
||||
```rust
|
||||
// event_stream.update_fields(ToolCallUpdateFields::new().content(vec![
|
||||
// acp::ToolCallContent::Content(acp::Content::new(acp::ContentBlock::Image(
|
||||
// acp::ImageContent::new(language_model_image.source.clone(), "image/png"),
|
||||
// ))),
|
||||
// ]));
|
||||
```
|
||||
|
||||
2. Run the visual test - it should FAIL or produce a visibly different screenshot
|
||||
|
||||
3. Restore the code - test should pass again
|
||||
|
||||
If commenting out the real tool code doesn't affect the test, the test is still broken!
|
||||
|
||||
## 📁 Files Modified
|
||||
|
||||
| File | Change |
|
||||
|------|--------|
|
||||
| `crates/zed/Cargo.toml` | Added `agent/test-support`, `language_model/test-support`, `fs/test-support`, `action_log` to `visual-tests` feature |
|
||||
| `crates/zed/src/visual_test_runner.rs` | Rewrote `run_agent_thread_view_test()` to run the real `ReadFileTool` and capture its output |
|
||||
|
||||
Note: No changes needed to `crates/agent/src/lib.rs` - all necessary exports were already public.
|
||||
|
||||
## ✅ Already Completed (Don't Redo These)
|
||||
|
||||
These changes have already been made and are working:
|
||||
|
||||
1. **`read_file` tool sends image content** - `crates/agent/src/tools/read_file_tool.rs` now calls `event_stream.update_fields()` with image content blocks (lines 181-185)
|
||||
|
||||
2. **UI renders images** - `crates/agent_ui/src/acp/thread_view.rs` has `render_image_output()` that shows dimensions ("512×512 PNG") and a "Go to File" button
|
||||
|
||||
3. **Image tool calls auto-expand** - The UI automatically expands tool calls that return images
|
||||
|
||||
4. **Visual test infrastructure exists** - The test runner, baseline comparison, etc. all work
|
||||
|
||||
The only thing broken is that the test doesn't actually run the real tool code!
|
||||
|
||||
## 🔗 Related Code References
|
||||
|
||||
- Tool implementation: [read_file_tool.rs](file:///Users/rtfeldman/code/zed5/crates/agent/src/tools/read_file_tool.rs)
|
||||
- Event stream: [thread.rs lines 2501-2596](file:///Users/rtfeldman/code/zed5/crates/agent/src/thread.rs#L2501-L2596)
|
||||
- UI rendering: [thread_view.rs render_image_output](file:///Users/rtfeldman/code/zed5/crates/agent_ui/src/acp/thread_view.rs#L3146-L3217)
|
||||
- Current test: [visual_test_runner.rs run_agent_thread_view_test](file:///Users/rtfeldman/code/zed5/crates/zed/src/visual_test_runner.rs#L778-L943)
|
||||
|
|
@ -1,661 +0,0 @@
|
|||
# Visual Test Runner Improvements
|
||||
|
||||
This document describes improvements to make the visual test runner in `crates/zed/src/visual_test_runner.rs` more deterministic, faster, and less hacky.
|
||||
|
||||
## Background
|
||||
|
||||
The visual test runner captures screenshots of Zed's UI and compares them against baseline images. It currently works but has several issues:
|
||||
|
||||
1. **Non-deterministic timing**: Uses `timer().await` calls scattered throughout
|
||||
2. **Real filesystem I/O**: Uses `tempfile` and `RealFs` instead of `FakeFs`
|
||||
3. **Dead code**: Unused variables and counters from removed print statements
|
||||
4. **Code duplication**: Similar setup code repeated across tests
|
||||
5. **Limited production code coverage**: Some areas use stubs where real code could run
|
||||
|
||||
## How to Run the Visual Tests
|
||||
|
||||
```bash
|
||||
# Run the visual tests (compare against baselines)
|
||||
cargo run -p zed --bin zed_visual_test_runner --features visual-tests
|
||||
|
||||
# Update baseline images (when UI intentionally changes)
|
||||
UPDATE_BASELINE=1 cargo run -p zed --bin zed_visual_test_runner --features visual-tests
|
||||
```
|
||||
|
||||
The test runner is a separate binary, not a `#[test]` function. It uses `Application::new().run()` to create a real GPUI application context.
|
||||
|
||||
---
|
||||
|
||||
## Improvement 1: Replace Timer-Based Waits with `run_until_parked()`
|
||||
|
||||
### Problem
|
||||
|
||||
The code is littered with timing-based waits like:
|
||||
|
||||
```rust
|
||||
cx.background_executor()
|
||||
.timer(std::time::Duration::from_millis(500))
|
||||
.await;
|
||||
```
|
||||
|
||||
These appear ~15 times in the file. They are:
|
||||
- **Slow**: Adds up to several seconds of unnecessary waiting
|
||||
- **Non-deterministic**: Could flake on slow CI machines
|
||||
- **Arbitrary**: The durations (100ms, 200ms, 300ms, 500ms, 2s) were chosen by trial and error
|
||||
|
||||
### Solution
|
||||
|
||||
Use `run_until_parked()` which runs all pending async tasks until there's nothing left to do. This is:
|
||||
- **Instant**: Returns immediately when work is complete
|
||||
- **Deterministic**: Waits exactly as long as needed
|
||||
- **Standard**: Used throughout Zed's test suite
|
||||
|
||||
### How to Implement
|
||||
|
||||
The challenge is that the visual test runner uses `AsyncApp` (from `cx.spawn()`), not `TestAppContext`. The `run_until_parked()` method is on `BackgroundExecutor` which is accessible via `cx.background_executor()`.
|
||||
|
||||
However, `run_until_parked()` is a **blocking** call that runs the executor synchronously, while the visual tests are currently written as async code. You'll need to restructure the code.
|
||||
|
||||
**Option A: Keep async structure, call run_until_parked between awaits**
|
||||
|
||||
```rust
|
||||
// Before:
|
||||
cx.background_executor()
|
||||
.timer(std::time::Duration::from_millis(500))
|
||||
.await;
|
||||
|
||||
// After - run all pending work synchronously:
|
||||
cx.background_executor().run_until_parked();
|
||||
```
|
||||
|
||||
But this won't work directly in async context because `run_until_parked()` blocks.
|
||||
|
||||
**Option B: Restructure to use synchronous test pattern**
|
||||
|
||||
The standard Zed test pattern uses `#[gpui::test]` with `TestAppContext`:
|
||||
|
||||
```rust
|
||||
#[gpui::test]
|
||||
async fn test_something(cx: &mut TestAppContext) {
|
||||
cx.run_until_parked(); // This works!
|
||||
}
|
||||
```
|
||||
|
||||
For the visual test runner, you'd need to convert from `Application::new().run()` to the test harness. This is a bigger change but would be more idiomatic.
|
||||
|
||||
**Option C: Use cx.refresh() + small delay for rendering**
|
||||
|
||||
For purely rendering-related waits, `cx.refresh()` forces a repaint. A single small delay after refresh may be acceptable for GPU readback timing:
|
||||
|
||||
```rust
|
||||
cx.refresh().ok();
|
||||
// Minimal delay just for GPU work, not async task completion
|
||||
cx.background_executor()
|
||||
.timer(std::time::Duration::from_millis(16)) // ~1 frame
|
||||
.await;
|
||||
```
|
||||
|
||||
### Locations to Change
|
||||
|
||||
Search for `timer(std::time::Duration` in the file. Each occurrence should be evaluated:
|
||||
|
||||
| Line | Current Duration | Purpose | Replacement |
|
||||
|------|-----------------|---------|-------------|
|
||||
| 160-162 | 500ms | Wait for worktree | `run_until_parked()` or await the task |
|
||||
| 190-192 | 500ms | Wait for panel add | `run_until_parked()` |
|
||||
| 205-207 | 500ms | Wait for panel render | `cx.refresh()` |
|
||||
| 248-250 | 500ms | Wait for item activation | `run_until_parked()` |
|
||||
| 258-260 | 2000ms | Wait for UI to stabilize | `cx.refresh()` + minimal delay |
|
||||
| 294-296 | 500ms | Wait for panel close | `run_until_parked()` |
|
||||
| 752-754 | 100ms | Wait for worktree scan | `run_until_parked()` |
|
||||
| 860-862 | 100ms | Wait for workspace init | `run_until_parked()` |
|
||||
| 881-883 | 200ms | Wait for panel ready | `run_until_parked()` |
|
||||
| 893-895 | 200ms | Wait for thread view | `run_until_parked()` |
|
||||
| 912-914 | 500ms | Wait for response | `run_until_parked()` |
|
||||
| 937-939 | 300ms | Wait for refresh | `cx.refresh()` |
|
||||
| 956-958 | 300ms | Wait for UI update | `run_until_parked()` |
|
||||
| 968-970 | 300ms | Wait for refresh | `cx.refresh()` |
|
||||
|
||||
### How to Verify
|
||||
|
||||
After making changes:
|
||||
1. Run the tests: `cargo run -p zed --bin zed_visual_test_runner --features visual-tests`
|
||||
2. They should pass with the same baseline images
|
||||
3. They should run faster (measure before/after)
|
||||
|
||||
---
|
||||
|
||||
## Improvement 2: Use `FakeFs` Instead of Real Filesystem
|
||||
|
||||
### Problem
|
||||
|
||||
The code currently uses:
|
||||
```rust
|
||||
let fs = Arc::new(RealFs::new(None, cx.background_executor().clone()));
|
||||
let temp_dir = tempfile::tempdir().expect("Failed to create temp directory");
|
||||
```
|
||||
|
||||
This is:
|
||||
- **Slow**: Real I/O is slower than in-memory operations
|
||||
- **Non-deterministic**: Filesystem timing varies
|
||||
- **Messy**: Leaves temp directories on failure
|
||||
|
||||
### Solution
|
||||
|
||||
Use `FakeFs` which is an in-memory filesystem used throughout Zed's tests:
|
||||
|
||||
```rust
|
||||
use fs::FakeFs;
|
||||
|
||||
let fs = FakeFs::new(cx.background_executor().clone());
|
||||
fs.insert_tree(
|
||||
"/project",
|
||||
json!({
|
||||
"src": {
|
||||
"main.rs": "fn main() { println!(\"Hello\"); }"
|
||||
},
|
||||
"Cargo.toml": "[package]\nname = \"test\""
|
||||
}),
|
||||
).await;
|
||||
```
|
||||
|
||||
### How to Implement
|
||||
|
||||
1. **Add the dependency** in `crates/zed/Cargo.toml` if not present:
|
||||
```toml
|
||||
fs = { workspace = true, features = ["test-support"] }
|
||||
```
|
||||
|
||||
2. **Replace RealFs creation** in `init_app_state()` (around line 655):
|
||||
|
||||
```rust
|
||||
// Before:
|
||||
let fs = Arc::new(RealFs::new(None, cx.background_executor().clone()));
|
||||
|
||||
// After:
|
||||
let fs = FakeFs::new(cx.background_executor().clone());
|
||||
```
|
||||
|
||||
3. **Replace tempdir + file creation** (around lines 66-71 and 518-643):
|
||||
|
||||
```rust
|
||||
// Before:
|
||||
let temp_dir = tempfile::tempdir().expect("...");
|
||||
let project_path = temp_dir.path().join("project");
|
||||
std::fs::create_dir_all(&project_path).expect("...");
|
||||
create_test_files(&project_path);
|
||||
|
||||
// After:
|
||||
let fs = FakeFs::new(cx.background_executor().clone());
|
||||
fs.insert_tree("/project", json!({
|
||||
"src": {
|
||||
"main.rs": include_str!("test_content/main.rs"),
|
||||
"lib.rs": include_str!("test_content/lib.rs"),
|
||||
},
|
||||
"Cargo.toml": include_str!("test_content/Cargo.toml"),
|
||||
"README.md": include_str!("test_content/README.md"),
|
||||
})).await;
|
||||
let project_path = Path::new("/project");
|
||||
```
|
||||
|
||||
4. **For the test image** (around line 726):
|
||||
|
||||
```rust
|
||||
// Before:
|
||||
let temp_dir = tempfile::tempdir()?;
|
||||
let project_path = temp_dir.path().join("project");
|
||||
std::fs::create_dir_all(&project_path)?;
|
||||
let image_path = project_path.join("test-image.png");
|
||||
std::fs::write(&image_path, EMBEDDED_TEST_IMAGE)?;
|
||||
|
||||
// After:
|
||||
fs.insert_file("/project/test-image.png", EMBEDDED_TEST_IMAGE.to_vec()).await;
|
||||
let project_path = Path::new("/project");
|
||||
```
|
||||
|
||||
5. **Update `init_app_state`** to accept `fs` as a parameter instead of creating it internally.
|
||||
|
||||
### Reference Example
|
||||
|
||||
See `crates/project_panel/src/project_panel_tests.rs` lines 17-62 for a complete example of using `FakeFs` with `insert_tree()`.
|
||||
|
||||
### How to Verify
|
||||
|
||||
1. Run the tests - they should produce identical screenshots
|
||||
2. Verify no temp directories are created in `/tmp` or equivalent
|
||||
3. Check that tests run faster
|
||||
|
||||
---
|
||||
|
||||
## Improvement 3: Remove Dead Code
|
||||
|
||||
### Problem
|
||||
|
||||
After removing print statements, there's leftover dead code:
|
||||
|
||||
```rust
|
||||
let _ = update_baseline; // Line 63 - was used in removed print
|
||||
|
||||
let mut passed = 0; // Lines 263-265
|
||||
let mut failed = 0;
|
||||
let mut updated = 0;
|
||||
// ... counters incremented but never read
|
||||
|
||||
let _ = (passed, updated); // Line 327 - silences warning
|
||||
```
|
||||
|
||||
### Solution
|
||||
|
||||
Remove the unused code and restructure to not need counters.
|
||||
|
||||
### How to Implement
|
||||
|
||||
1. **Remove the `let _ = update_baseline;`** on line 63 - `update_baseline` is already used later
|
||||
|
||||
2. **Simplify test result handling**:
|
||||
|
||||
```rust
|
||||
// Before:
|
||||
let mut passed = 0;
|
||||
let mut failed = 0;
|
||||
let mut updated = 0;
|
||||
|
||||
match test_result {
|
||||
Ok(TestResult::Passed) => passed += 1,
|
||||
Ok(TestResult::BaselineUpdated(_)) => updated += 1,
|
||||
Err(_) => failed += 1,
|
||||
}
|
||||
// ... repeat for each test ...
|
||||
|
||||
let _ = (passed, updated);
|
||||
|
||||
if failed > 0 {
|
||||
std::process::exit(1);
|
||||
}
|
||||
|
||||
// After:
|
||||
let mut any_failed = false;
|
||||
|
||||
if run_visual_test("project_panel", ...).await.is_err() {
|
||||
any_failed = true;
|
||||
}
|
||||
|
||||
if run_visual_test("workspace_with_editor", ...).await.is_err() {
|
||||
any_failed = true;
|
||||
}
|
||||
|
||||
if run_agent_thread_view_test(...).await.is_err() {
|
||||
any_failed = true;
|
||||
}
|
||||
|
||||
if any_failed {
|
||||
std::process::exit(1);
|
||||
}
|
||||
```
|
||||
|
||||
3. **Or collect results into a Vec**:
|
||||
|
||||
```rust
|
||||
let results = vec![
|
||||
run_visual_test("project_panel", ...).await,
|
||||
run_visual_test("workspace_with_editor", ...).await,
|
||||
run_agent_thread_view_test(...).await,
|
||||
];
|
||||
|
||||
if results.iter().any(|r| r.is_err()) {
|
||||
std::process::exit(1);
|
||||
}
|
||||
```
|
||||
|
||||
### How to Verify
|
||||
|
||||
1. Run `cargo clippy -p zed --features visual-tests` - should have no warnings about unused variables
|
||||
2. Run the tests - behavior should be unchanged
|
||||
|
||||
---
|
||||
|
||||
## Improvement 4: Consolidate Test Setup Code
|
||||
|
||||
### Problem
|
||||
|
||||
There's significant duplication between:
|
||||
- Main workspace test setup (lines 106-260)
|
||||
- Agent panel test setup (lines 713-900)
|
||||
|
||||
Both create:
|
||||
- Projects with `Project::local()`
|
||||
- Worktrees with `find_or_create_worktree()`
|
||||
- Windows with `cx.open_window()`
|
||||
- Wait for things to settle
|
||||
|
||||
### Solution
|
||||
|
||||
Extract common setup into helper functions.
|
||||
|
||||
### How to Implement
|
||||
|
||||
1. **Create a `TestWorkspace` struct**:
|
||||
|
||||
```rust
|
||||
struct TestWorkspace {
|
||||
window: WindowHandle<Workspace>,
|
||||
project: Entity<Project>,
|
||||
}
|
||||
|
||||
impl TestWorkspace {
|
||||
async fn new(
|
||||
app_state: Arc<AppState>,
|
||||
size: Size<Pixels>,
|
||||
project_path: &Path,
|
||||
cx: &mut AsyncApp,
|
||||
) -> Result<Self> {
|
||||
let project = cx.update(|cx| {
|
||||
Project::local(
|
||||
app_state.client.clone(),
|
||||
app_state.node_runtime.clone(),
|
||||
app_state.user_store.clone(),
|
||||
app_state.languages.clone(),
|
||||
app_state.fs.clone(),
|
||||
None,
|
||||
false,
|
||||
cx,
|
||||
)
|
||||
})?;
|
||||
|
||||
// Add worktree
|
||||
let add_task = project.update(cx, |project, cx| {
|
||||
project.find_or_create_worktree(project_path, true, cx)
|
||||
})?;
|
||||
add_task.await?;
|
||||
|
||||
// Create window
|
||||
let bounds = Bounds {
|
||||
origin: point(px(0.0), px(0.0)),
|
||||
size,
|
||||
};
|
||||
|
||||
let window = cx.update(|cx| {
|
||||
cx.open_window(
|
||||
WindowOptions {
|
||||
window_bounds: Some(WindowBounds::Windowed(bounds)),
|
||||
focus: false,
|
||||
show: false,
|
||||
..Default::default()
|
||||
},
|
||||
|window, cx| {
|
||||
cx.new(|cx| {
|
||||
Workspace::new(None, project.clone(), app_state.clone(), window, cx)
|
||||
})
|
||||
},
|
||||
)
|
||||
})??;
|
||||
|
||||
cx.background_executor().run_until_parked();
|
||||
|
||||
Ok(Self { window, project })
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
2. **Create a `setup_project_panel` helper**:
|
||||
|
||||
```rust
|
||||
async fn setup_project_panel(
|
||||
workspace: &TestWorkspace,
|
||||
cx: &mut AsyncApp,
|
||||
) -> Result<Entity<ProjectPanel>> {
|
||||
let panel_task = workspace.window.update(cx, |_ws, window, cx| {
|
||||
let weak = cx.weak_entity();
|
||||
window.spawn(cx, async move |cx| ProjectPanel::load(weak, cx).await)
|
||||
})?;
|
||||
|
||||
let panel = panel_task.await?;
|
||||
|
||||
workspace.window.update(cx, |ws, window, cx| {
|
||||
ws.add_panel(panel.clone(), window, cx);
|
||||
ws.open_panel::<ProjectPanel>(window, cx);
|
||||
})?;
|
||||
|
||||
cx.background_executor().run_until_parked();
|
||||
|
||||
Ok(panel)
|
||||
}
|
||||
```
|
||||
|
||||
3. **Use helpers in tests**:
|
||||
|
||||
```rust
|
||||
// Test 1
|
||||
let workspace = TestWorkspace::new(
|
||||
app_state.clone(),
|
||||
size(px(1280.0), px(800.0)),
|
||||
&project_path,
|
||||
&mut cx,
|
||||
).await?;
|
||||
|
||||
setup_project_panel(&workspace, &mut cx).await?;
|
||||
open_file(&workspace, "src/main.rs", &mut cx).await?;
|
||||
|
||||
run_visual_test("project_panel", workspace.window.into(), &mut cx, update_baseline).await?;
|
||||
```
|
||||
|
||||
### How to Verify
|
||||
|
||||
1. Tests should produce identical screenshots
|
||||
2. Code should be shorter and more readable
|
||||
3. Adding new tests should be easier
|
||||
|
||||
---
|
||||
|
||||
## Improvement 5: Exercise More Production Code
|
||||
|
||||
### Problem
|
||||
|
||||
Some tests use minimal stubs where real production code could run deterministically.
|
||||
|
||||
### Current State (Good)
|
||||
|
||||
The agent thread test already runs the **real** `ReadFileTool`:
|
||||
```rust
|
||||
let tool = Arc::new(agent::ReadFileTool::new(...));
|
||||
tool.clone().run(input, event_stream, cx).await?;
|
||||
```
|
||||
|
||||
This is great! It exercises real tool execution.
|
||||
|
||||
### Opportunities for More Coverage
|
||||
|
||||
1. **Syntax highlighting**: Register real language grammars so the editor shows colored code
|
||||
|
||||
```rust
|
||||
// Currently just:
|
||||
let languages = Arc::new(LanguageRegistry::new(cx.background_executor().clone()));
|
||||
|
||||
// Could add:
|
||||
languages.register_native_grammars([
|
||||
tree_sitter_rust::LANGUAGE.into(),
|
||||
tree_sitter_markdown::LANGUAGE.into(),
|
||||
]);
|
||||
```
|
||||
|
||||
2. **File icons**: The project panel could show real file icons by registering file types
|
||||
|
||||
3. **Theme loading**: Currently uses `LoadThemes::JustBase`. Could load a full theme:
|
||||
```rust
|
||||
theme::init(theme::LoadThemes::All, cx);
|
||||
```
|
||||
(But this might make tests slower - evaluate trade-off)
|
||||
|
||||
4. **More tool types**: Test other tools like `ListFilesTool`, `GrepTool` that don't need network
|
||||
|
||||
### How to Implement Syntax Highlighting
|
||||
|
||||
1. Add language dependencies to `Cargo.toml`:
|
||||
```toml
|
||||
tree-sitter-rust = { workspace = true }
|
||||
tree-sitter-markdown = { workspace = true }
|
||||
```
|
||||
|
||||
2. In `init_app_state` or test setup:
|
||||
```rust
|
||||
let languages = Arc::new(LanguageRegistry::new(cx.background_executor().clone()));
|
||||
|
||||
// Register Rust grammar
|
||||
languages.register_native_grammars([
|
||||
("rust", tree_sitter_rust::LANGUAGE.into()),
|
||||
]);
|
||||
|
||||
// Register the Rust language config
|
||||
languages.register_available_language(
|
||||
LanguageConfig {
|
||||
name: "Rust".into(),
|
||||
grammar: Some("rust".into()),
|
||||
matcher: LanguageMatcher {
|
||||
path_suffixes: vec!["rs".into()],
|
||||
..Default::default()
|
||||
},
|
||||
..Default::default()
|
||||
},
|
||||
tree_sitter_rust::LANGUAGE.into(),
|
||||
vec![], // No LSP adapters needed for visual tests
|
||||
);
|
||||
```
|
||||
|
||||
### How to Verify
|
||||
|
||||
1. Update baselines after adding syntax highlighting
|
||||
2. Screenshots should show colored code instead of plain text
|
||||
3. Tests should still be deterministic (same colors every time)
|
||||
|
||||
---
|
||||
|
||||
## Improvement 6: Better Test Organization
|
||||
|
||||
### Problem
|
||||
|
||||
Tests are numbered in comments but the structure is ad-hoc:
|
||||
```rust
|
||||
// Run Test 1: Project Panel
|
||||
// Run Test 2: Workspace with Editor
|
||||
// Run Test 3: Agent Thread View
|
||||
```
|
||||
|
||||
### Solution
|
||||
|
||||
Create a test registry or use a more structured approach.
|
||||
|
||||
### How to Implement
|
||||
|
||||
1. **Define tests as structs**:
|
||||
|
||||
```rust
|
||||
trait VisualTest {
|
||||
fn name(&self) -> &'static str;
|
||||
async fn setup(&self, cx: &mut AsyncApp) -> Result<AnyWindowHandle>;
|
||||
async fn run(&self, window: AnyWindowHandle, cx: &mut AsyncApp, update_baseline: bool) -> Result<TestResult>;
|
||||
}
|
||||
|
||||
struct ProjectPanelTest;
|
||||
struct WorkspaceEditorTest;
|
||||
struct AgentThreadTest;
|
||||
|
||||
impl VisualTest for ProjectPanelTest {
|
||||
fn name(&self) -> &'static str { "project_panel" }
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
2. **Run all tests from a registry**:
|
||||
|
||||
```rust
|
||||
let tests: Vec<Box<dyn VisualTest>> = vec![
|
||||
Box::new(ProjectPanelTest),
|
||||
Box::new(WorkspaceEditorTest),
|
||||
Box::new(AgentThreadTest),
|
||||
];
|
||||
|
||||
let mut failed = false;
|
||||
for test in tests {
|
||||
match test.run(...).await {
|
||||
Ok(_) => {},
|
||||
Err(_) => failed = true,
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
This makes it easy to:
|
||||
- Add new tests (just add to the vec)
|
||||
- Run specific tests (filter by name)
|
||||
- See all tests in one place
|
||||
|
||||
---
|
||||
|
||||
## Improvement 7: Constants and Configuration
|
||||
|
||||
### Problem
|
||||
|
||||
Magic numbers scattered throughout:
|
||||
```rust
|
||||
size(px(1280.0), px(800.0)) // Window size for workspace tests
|
||||
size(px(500.0), px(900.0)) // Window size for agent panel
|
||||
Duration::from_millis(500) // Various delays
|
||||
const MATCH_THRESHOLD: f64 = 0.99; // Already a constant, good!
|
||||
```
|
||||
|
||||
### Solution
|
||||
|
||||
Move all configuration to constants at the top of the file.
|
||||
|
||||
### How to Implement
|
||||
|
||||
```rust
|
||||
// Window sizes
|
||||
const WORKSPACE_WINDOW_SIZE: Size<Pixels> = size(px(1280.0), px(800.0));
|
||||
const AGENT_PANEL_WINDOW_SIZE: Size<Pixels> = size(px(500.0), px(900.0));
|
||||
|
||||
// Timing (if any delays are still needed after Improvement 1)
|
||||
const FRAME_DELAY: Duration = Duration::from_millis(16);
|
||||
|
||||
// Image comparison
|
||||
const MATCH_THRESHOLD: f64 = 0.99;
|
||||
const PIXEL_TOLERANCE: i16 = 2; // Currently hardcoded in pixels_are_similar()
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Summary: Recommended Order of Implementation
|
||||
|
||||
1. **Remove dead code** (Improvement 3) - Quick win, low risk
|
||||
2. **Add constants** (Improvement 7) - Quick win, improves readability
|
||||
3. **Use FakeFs** (Improvement 2) - Medium effort, big determinism win
|
||||
4. **Replace timers** (Improvement 1) - Medium effort, biggest determinism win
|
||||
5. **Consolidate setup** (Improvement 4) - Medium effort, maintainability win
|
||||
6. **Exercise more production code** (Improvement 5) - Lower priority, nice to have
|
||||
7. **Better test organization** (Improvement 6) - Lower priority, nice to have for many tests
|
||||
|
||||
## Testing Your Changes
|
||||
|
||||
After each change:
|
||||
|
||||
```bash
|
||||
# Build to check for compile errors
|
||||
cargo build -p zed --features visual-tests
|
||||
|
||||
# Run clippy for warnings
|
||||
cargo clippy -p zed --features visual-tests
|
||||
|
||||
# Run the tests
|
||||
cargo run -p zed --bin zed_visual_test_runner --features visual-tests
|
||||
|
||||
# If screenshots changed intentionally, update baselines
|
||||
UPDATE_BASELINE=1 cargo run -p zed --bin zed_visual_test_runner --features visual-tests
|
||||
```
|
||||
|
||||
Baseline images are stored in `crates/zed/test_fixtures/visual_tests/`.
|
||||
|
||||
## Questions?
|
||||
|
||||
If you get stuck:
|
||||
1. Look at existing tests in `crates/project_panel/src/project_panel_tests.rs` for examples of `FakeFs` and `run_until_parked()`
|
||||
2. Look at `crates/gpui/src/app/test_context.rs` for `VisualTestContext` documentation
|
||||
3. The CLAUDE.md file at the repo root has Rust coding guidelines
|
||||
Loading…
Reference in a new issue