diff --git a/Cargo.lock b/Cargo.lock index bdadfd87671de830fb42109e9edd0dd161fa837d..7b49b1062118ee6be3f376956e3af8c9c462a28b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -312,8 +312,10 @@ dependencies = [ "fs", "gpui", "language_model", + "log", "paths", "project", + "regex", "schemars", "serde", "serde_json", diff --git a/assets/settings/default.json b/assets/settings/default.json index fc886fb63cdcb44e3b5d0319c77d2d88875d1692..fdf4770b3d11f42283adfedb0ec4818726fe7bcc 100644 --- a/assets/settings/default.json +++ b/assets/settings/default.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. diff --git a/crates/agent_settings/Cargo.toml b/crates/agent_settings/Cargo.toml index 0d7163549f0a4b172773c9ac95dcbc84b7212667..4e8a571fe8362b8c0276a2cb6cc2a12b16fac895 100644 --- a/crates/agent_settings/Cargo.toml +++ b/crates/agent_settings/Cargo.toml @@ -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 diff --git a/crates/agent_settings/src/agent_settings.rs b/crates/agent_settings/src/agent_settings.rs index b513ec1a70b6f7ab02382dfa312ea2d4d6a47234..77b428b47317e2cacce3fb0111b9d55cc64ec3ac 100644 --- a/crates/agent_settings/src/agent_settings.rs +++ b/crates/agent_settings/src/agent_settings.rs @@ -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, ToolRules>, +} + +#[derive(Clone, Debug)] +pub struct ToolRules { + pub default_mode: ToolPermissionMode, + pub always_allow: Vec, + pub always_deny: Vec, + pub always_confirm: Vec, +} + +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 { + 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) -> 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) -> Vec { + 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" + ); + } +} diff --git a/crates/agent_ui/src/agent_ui.rs b/crates/agent_ui/src/agent_ui.rs index 6e308e06373603d933297e9d9be9e4cbad380d6e..99fee5a877c8fd393ba5660580767488dc9ab0e5 100644 --- a/crates/agent_ui/src/agent_ui.rs +++ b/crates/agent_ui/src/agent_ui.rs @@ -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| { diff --git a/crates/feature_flags/src/flags.rs b/crates/feature_flags/src/flags.rs index f146795eead3cacef0ffb501b0803a685358d60d..6d170476b30cb520612232d02ca418a393a5311a 100644 --- a/crates/feature_flags/src/flags.rs +++ b/crates/feature_flags/src/flags.rs @@ -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 { diff --git a/crates/settings/src/settings_content/agent.rs b/crates/settings/src/settings_content/agent.rs index 8473305dcd320e3c1a8d5d87018a8565a3e40649..7c97ff88b0539908db10a0ec37fb44afcafb8c1b 100644 --- a/crates/settings/src/settings_content/agent.rs +++ b/crates/settings/src/settings_content/agent.rs @@ -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, + /// 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, } impl AgentSettingsContent { @@ -466,3 +474,61 @@ pub enum CustomAgentServerSettings { favorite_config_option_values: HashMap>, }, } + +#[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, 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, + + /// Regexes for inputs to auto-approve. + /// For terminal: matches command. For file tools: matches path. For fetch: matches URL. + /// Default: [] + pub always_allow: Option>, + + /// Regexes for inputs to auto-reject. + /// **SECURITY**: These take precedence over ALL other rules, across ALL settings layers. + /// Default: [] + pub always_deny: Option>, + + /// Regexes for inputs that must always prompt. + /// Takes precedence over always_allow but not always_deny. + /// Default: [] + pub always_confirm: Option>, +} + +#[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, +} + +#[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, +} diff --git a/plans/agent-panel-image-visual-test.md b/plans/agent-panel-image-visual-test.md deleted file mode 100644 index 78f390cf7b77b6305f2a12555730f06ca546a62b..0000000000000000000000000000000000000000 --- a/plans/agent-panel-image-visual-test.md +++ /dev/null @@ -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) diff --git a/plans/visual-test-improvements.md b/plans/visual-test-improvements.md deleted file mode 100644 index 40f88392d9109a9406117776906698799d57863c..0000000000000000000000000000000000000000 --- a/plans/visual-test-improvements.md +++ /dev/null @@ -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, - project: Entity, - } - - impl TestWorkspace { - async fn new( - app_state: Arc, - size: Size, - project_path: &Path, - cx: &mut AsyncApp, - ) -> Result { - 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> { - 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::(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; - async fn run(&self, window: AnyWindowHandle, cx: &mut AsyncApp, update_baseline: bool) -> Result; - } - - 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> = 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 = size(px(1280.0), px(800.0)); -const AGENT_PANEL_WINDOW_SIZE: Size = 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