From da7945aeaeea1295f037c1ab04f511e0daf4d32e Mon Sep 17 00:00:00 2001 From: MartinYe1234 <52641447+MartinYe1234@users.noreply.github.com> Date: Thu, 4 Jun 2026 08:39:02 -0700 Subject: [PATCH] Hide Always Allow option for agent skills edits (#58463) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Edits targeting the agent skills directory (project `.agents/skills/` and global `~/.agents/skills/`) always prompt regardless of permission settings, because they route through `authorize_always_prompt` which never consults the persisted `always_allow` rules. The permission prompt still rendered an "Always allow" button, though. Clicking it wrote an `always_allow` rule into settings that was then silently ignored on the next skills edit, so users were re-prompted every time despite having chosen "always allow" — exactly the confusion reported in AI-346. This keeps the always-prompt behavior intact and just hides the misleading "Always allow" / "Always deny" options for skills, leaving once-only Allow/Deny. Applies to all mutating skills tools: `edit_file`, `write_file`, `copy_path`, `move_path`, `delete_path`, and `create_directory`. Implementation: - New `ToolPermissionScope::AgentSkills`, set via `ToolPermissionContext::for_agent_skills()`. - `build_permission_options` returns flat once-only Allow/Deny for that scope (mirrors the existing `SymlinkTarget` handling). - Wired into both authorization entry points (`authorize_file_edit` and `authorize_with_sensitive_settings`). - Tests assert no `AllowAlways` option is offered for each affected tool. Settings-file prompts (`.zed/settings.json`, global config) are intentionally left unchanged. Closes AI-346 Release Notes: - Removed the misleading "Always allow" button from agent permission prompts for edits to the agent skills directory, since those edits always require confirmation. --- crates/agent/src/thread.rs | 22 +++++++++++++++++++ crates/agent/src/tools/copy_path_tool.rs | 14 ++++++++++++ .../agent/src/tools/create_directory_tool.rs | 7 ++++++ crates/agent/src/tools/delete_path_tool.rs | 7 ++++++ crates/agent/src/tools/edit_file_tool.rs | 14 ++++++++++++ crates/agent/src/tools/move_path_tool.rs | 14 ++++++++++++ crates/agent/src/tools/tool_permissions.rs | 11 ++++++---- crates/agent/src/tools/write_file_tool.rs | 7 ++++++ 8 files changed, 92 insertions(+), 4 deletions(-) diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index 68b6bf03ab..716975bd17 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -852,6 +852,7 @@ pub struct ToolPermissionContext { pub enum ToolPermissionScope { ToolInput, SymlinkTarget, + AgentSkills, } impl ToolPermissionContext { @@ -871,6 +872,11 @@ impl ToolPermissionContext { } } + pub fn for_agent_skills(mut self) -> Self { + self.scope = ToolPermissionScope::AgentSkills; + self + } + /// Builds the permission options for this tool context. /// /// This is the canonical source for permission option generation. @@ -916,6 +922,22 @@ impl ToolPermissionContext { ]); } + // Skills always prompt, so offer only once-only allow/deny. + if self.scope == ToolPermissionScope::AgentSkills { + return acp_thread::PermissionOptions::Flat(vec![ + acp::PermissionOption::new( + acp::PermissionOptionId::new("allow"), + "Allow", + acp::PermissionOptionKind::AllowOnce, + ), + acp::PermissionOption::new( + acp::PermissionOptionId::new("deny"), + "Deny", + acp::PermissionOptionKind::RejectOnce, + ), + ]); + } + // Check if the user's shell supports POSIX-like command chaining. // See the doc comment above for the full explanation of why this is needed. let shell_supports_always_allow = if tool_name == TerminalTool::NAME { diff --git a/crates/agent/src/tools/copy_path_tool.rs b/crates/agent/src/tools/copy_path_tool.rs index 6d300551a5..42a7311c57 100644 --- a/crates/agent/src/tools/copy_path_tool.rs +++ b/crates/agent/src/tools/copy_path_tool.rs @@ -328,6 +328,13 @@ mod tests { title.contains("agent skills"), "Authorization title should mention agent skills, got: {title}", ); + assert!( + auth.options + .first_option_of_kind(acp::PermissionOptionKind::AllowAlways) + .is_none(), + "agent skills prompt must not offer an \"Always allow\" option: {:?}", + auth.options, + ); auth.response .send(acp_thread::SelectedPermissionOutcome::new( acp::PermissionOptionId::new("allow"), @@ -387,6 +394,13 @@ mod tests { title.contains("agent skills"), "Authorization title should mention agent skills, got: {title}", ); + assert!( + auth.options + .first_option_of_kind(acp::PermissionOptionKind::AllowAlways) + .is_none(), + "agent skills prompt must not offer an \"Always allow\" option: {:?}", + auth.options, + ); auth.response .send(acp_thread::SelectedPermissionOutcome::new( acp::PermissionOptionId::new("allow"), diff --git a/crates/agent/src/tools/create_directory_tool.rs b/crates/agent/src/tools/create_directory_tool.rs index dcd051c2a7..fdec1da2b7 100644 --- a/crates/agent/src/tools/create_directory_tool.rs +++ b/crates/agent/src/tools/create_directory_tool.rs @@ -243,6 +243,13 @@ mod tests { title.contains("agent skills"), "Authorization title should mention agent skills, got: {title}", ); + assert!( + auth.options + .first_option_of_kind(acp::PermissionOptionKind::AllowAlways) + .is_none(), + "agent skills prompt must not offer an \"Always allow\" option: {:?}", + auth.options, + ); auth.response .send(acp_thread::SelectedPermissionOutcome::new( acp::PermissionOptionId::new("allow"), diff --git a/crates/agent/src/tools/delete_path_tool.rs b/crates/agent/src/tools/delete_path_tool.rs index e791e6feb5..bae19d7692 100644 --- a/crates/agent/src/tools/delete_path_tool.rs +++ b/crates/agent/src/tools/delete_path_tool.rs @@ -329,6 +329,13 @@ mod tests { title.contains("agent skills"), "Authorization title should mention agent skills, got: {title}", ); + assert!( + auth.options + .first_option_of_kind(acp::PermissionOptionKind::AllowAlways) + .is_none(), + "agent skills prompt must not offer an \"Always allow\" option: {:?}", + auth.options, + ); auth.response .send(acp_thread::SelectedPermissionOutcome::new( acp::PermissionOptionId::new("allow"), diff --git a/crates/agent/src/tools/edit_file_tool.rs b/crates/agent/src/tools/edit_file_tool.rs index 2801c8878d..e80e2110a1 100644 --- a/crates/agent/src/tools/edit_file_tool.rs +++ b/crates/agent/src/tools/edit_file_tool.rs @@ -1320,6 +1320,20 @@ mod tests { event.tool_call.fields.title, Some("Edit `root/.agents/skills/my-skill/SKILL.md` (agent skills)".into()) ); + // Skills always prompt, so no "Always allow" option is offered. + assert!( + event + .options + .first_option_of_kind(acp::PermissionOptionKind::AllowAlways) + .is_none(), + "agent skills prompt must not offer an \"Always allow\" option: {:?}", + event.options, + ); + assert!( + matches!(event.options, acp_thread::PermissionOptions::Flat(_)), + "agent skills prompt should use flat allow/deny options: {:?}", + event.options, + ); // 5.6: The global .agents/skills directory is sensitive — still prompts let global_skill_path = agent_skills::global_skills_dir() diff --git a/crates/agent/src/tools/move_path_tool.rs b/crates/agent/src/tools/move_path_tool.rs index 000f17a38c..8156d0714a 100644 --- a/crates/agent/src/tools/move_path_tool.rs +++ b/crates/agent/src/tools/move_path_tool.rs @@ -354,6 +354,13 @@ mod tests { title.contains("agent skills"), "Authorization title should mention agent skills, got: {title}", ); + assert!( + auth.options + .first_option_of_kind(acp::PermissionOptionKind::AllowAlways) + .is_none(), + "agent skills prompt must not offer an \"Always allow\" option: {:?}", + auth.options, + ); auth.response .send(acp_thread::SelectedPermissionOutcome::new( acp::PermissionOptionId::new("allow"), @@ -413,6 +420,13 @@ mod tests { title.contains("agent skills"), "Authorization title should mention agent skills, got: {title}", ); + assert!( + auth.options + .first_option_of_kind(acp::PermissionOptionKind::AllowAlways) + .is_none(), + "agent skills prompt must not offer an \"Always allow\" option: {:?}", + auth.options, + ); auth.response .send(acp_thread::SelectedPermissionOutcome::new( acp::PermissionOptionId::new("allow"), diff --git a/crates/agent/src/tools/tool_permissions.rs b/crates/agent/src/tools/tool_permissions.rs index 7dd0972f0a..5fb1ade6ab 100644 --- a/crates/agent/src/tools/tool_permissions.rs +++ b/crates/agent/src/tools/tool_permissions.rs @@ -509,9 +509,11 @@ pub fn authorize_with_sensitive_settings( Some(SensitiveSettingsKind::Global) => { event_stream.authorize_always_prompt(format!("{title} (settings)"), context, cx) } - Some(SensitiveSettingsKind::AgentSkills) => { - event_stream.authorize_always_prompt(format!("{title} (agent skills)"), context, cx) - } + Some(SensitiveSettingsKind::AgentSkills) => event_stream.authorize_always_prompt( + format!("{title} (agent skills)"), + context.for_agent_skills(), + cx, + ), None => event_stream.authorize(title, context, cx), } } @@ -761,7 +763,8 @@ pub fn authorize_file_edit( let context = ToolPermissionContext::new( &tool_name, vec![path_owned.to_string_lossy().to_string()], - ); + ) + .for_agent_skills(); event_stream.authorize_always_prompt( format!("{title} (agent skills)"), context, diff --git a/crates/agent/src/tools/write_file_tool.rs b/crates/agent/src/tools/write_file_tool.rs index 735a9d23a9..af9c857f63 100644 --- a/crates/agent/src/tools/write_file_tool.rs +++ b/crates/agent/src/tools/write_file_tool.rs @@ -373,6 +373,13 @@ mod tests { title.contains("agent skills"), "Authorization title should mention agent skills, got: {title}", ); + assert!( + auth.options + .first_option_of_kind(acp::PermissionOptionKind::AllowAlways) + .is_none(), + "agent skills prompt must not offer an \"Always allow\" option: {:?}", + auth.options, + ); auth.response .send(acp_thread::SelectedPermissionOutcome::new( acp::PermissionOptionId::new("allow"),