diff --git a/crates/agent/src/tools/create_directory_tool.rs b/crates/agent/src/tools/create_directory_tool.rs index ca61a9e632e..dcd051c2a72 100644 --- a/crates/agent/src/tools/create_directory_tool.rs +++ b/crates/agent/src/tools/create_directory_tool.rs @@ -1,6 +1,6 @@ use super::tool_permissions::{ authorize_symlink_access, canonicalize_worktree_roots, detect_symlink_escape, - sensitive_settings_kind, + resolve_creatable_global_skill_path, sensitive_settings_kind, }; use agent_client_protocol::schema as acp; use agent_settings::AgentSettings; @@ -22,6 +22,7 @@ use std::path::Path; /// Creates a new directory at the specified path within the project. Returns confirmation that the directory was created. /// /// This tool creates a directory and all necessary parent directories. It should be used whenever you need to create new directories within the project. +/// The only supported path outside the project is `~/.agents/skills` or a descendant, for global agent skills. #[derive(Debug, Serialize, Deserialize, JsonSchema)] pub struct CreateDirectoryToolInput { /// The path of the new directory. @@ -34,6 +35,10 @@ pub struct CreateDirectoryToolInput { /// /// You can create a new directory by providing a path of "directory1/new_directory" /// + /// + /// + /// To create a global agent skill directory, you may provide a path under `~/.agents/skills`, such as `~/.agents/skills/my-skill`. + /// pub path: String, } @@ -144,6 +149,21 @@ impl AgentTool for CreateDirectoryTool { authorize.await.map_err(|e| e.to_string())?; } + if let Some(global_skill_directory) = + resolve_creatable_global_skill_path(Path::new(&input.path), fs.as_ref()).await + { + futures::select! { + result = fs.create_dir(&global_skill_directory).fuse() => { + result.map_err(|e| format!("Creating directory {destination_path}: {e}"))?; + } + _ = event_stream.cancelled_by_user().fuse() => { + return Err("Create directory cancelled by user".to_string()); + } + } + + return Ok(format!("Created directory {destination_path}")); + } + let create_entry = project.update(cx, |project, cx| { match project.find_project_path(&input.path, cx) { Some(project_path) => Ok(project.create_entry(project_path, true, cx)), @@ -190,6 +210,96 @@ mod tests { }); } + #[gpui::test] + async fn test_create_directory_allows_global_skill_directory(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree(path!("/root/project"), json!({})).await; + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(CreateDirectoryTool::new(project)); + let input_path = PathBuf::from("~") + .join(".agents") + .join("skills") + .join("my-skill") + .to_string_lossy() + .into_owned(); + let created_path = agent_skills::global_skills_dir().join("my-skill"); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + tool.run( + ToolInput::resolved(CreateDirectoryToolInput { path: input_path }), + event_stream, + cx, + ) + }); + + let auth = event_rx.expect_authorization().await; + let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); + assert!( + title.contains("agent skills"), + "Authorization title should mention agent skills, got: {title}", + ); + auth.response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) + .expect("authorization response should send"); + + let result = task.await; + assert!( + result.is_ok(), + "Tool should create global skill directory: {result:?}" + ); + assert!(fs.is_dir(&created_path).await); + } + + #[gpui::test] + async fn test_create_directory_rejects_other_global_paths(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree(path!("/root/project"), json!({})).await; + let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let tool = Arc::new(CreateDirectoryTool::new(project)); + let outside_path = agent_skills::global_skills_dir() + .parent() + .expect("global skills directory should have a parent") + .join("not-skills"); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let result = cx + .update(|cx| { + tool.run( + ToolInput::resolved(CreateDirectoryToolInput { + path: outside_path.to_string_lossy().into_owned(), + }), + event_stream, + cx, + ) + }) + .await; + + assert!( + result.is_err(), + "Tool should reject paths outside the project and global skills directory" + ); + assert!(!fs.is_dir(&outside_path).await); + assert!( + !matches!( + event_rx.try_recv(), + Ok(Ok(crate::ThreadEvent::ToolCallAuthorization(_))) + ), + "Non-skill global path should not emit an agent-skills authorization prompt", + ); + } + #[gpui::test] async fn test_create_directory_symlink_escape_requests_authorization(cx: &mut TestAppContext) { init_test(cx); diff --git a/crates/agent/src/tools/edit_file_tool.rs b/crates/agent/src/tools/edit_file_tool.rs index 60d93111316..82c6463dd11 100644 --- a/crates/agent/src/tools/edit_file_tool.rs +++ b/crates/agent/src/tools/edit_file_tool.rs @@ -26,6 +26,8 @@ const DEFAULT_UI_TEXT: &str = "Editing file"; /// Before using this tool, use the `read_file` tool to understand the file's contents and context. /// To create a new file or overwrite an existing one with completely new contents, use the `write_file` tool instead. /// +/// The only supported path outside the project is `~/.agents/skills` or a descendant, for global agent skills. +/// /// `read_file` prefixes each line of its output with a line number right-aligned in a /// 6-character field followed by a single tab, then the line's actual content. When you /// derive `old_text` or `new_text` from that output, strip this prefix and keep only what @@ -35,7 +37,7 @@ const DEFAULT_UI_TEXT: &str = "Editing file"; pub struct EditFileToolInput { /// The full path of the file to edit in the project. /// - /// WARNING: When specifying which file path need changing, you MUST start each path with one of the project's root directories. + /// WARNING: When specifying which file path need changing, you MUST start each path with one of the project's root directories, unless it's a global agent skill under `~/.agents/skills`. /// /// The following examples assume we have two root directories in the project: /// - /a/b/backend @@ -50,6 +52,10 @@ pub struct EditFileToolInput { /// /// `frontend/db.js` /// + /// + /// + /// To edit a global agent skill file, you may provide a path under `~/.agents/skills`, such as `~/.agents/skills/my-skill/SKILL.md`. + /// pub path: PathBuf, /// List of edit operations to apply sequentially. @@ -464,6 +470,63 @@ mod tests { assert_eq!(input_path, None); } + #[gpui::test] + async fn test_streaming_edit_global_skill_file(cx: &mut TestAppContext) { + init_test(cx); + + let fs = project::FakeFs::new(cx.executor()); + fs.insert_tree(path!("/root"), json!({})).await; + let skill_dir = agent_skills::global_skills_dir().join("my-skill"); + fs.insert_tree(&skill_dir, json!({ "SKILL.md": "old content\n" })) + .await; + let (edit_tool, _project, _action_log, fs, _thread) = + setup_test_with_fs(cx, fs, &[path!("/root").as_ref()]).await; + + let input_path = PathBuf::from("~") + .join(".agents") + .join("skills") + .join("my-skill") + .join("SKILL.md"); + let skill_file = agent_skills::global_skills_dir() + .join("my-skill") + .join("SKILL.md"); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + edit_tool.clone().run( + ToolInput::resolved(EditFileToolInput { + path: input_path, + edits: vec![Edit { + old_text: "old content".into(), + new_text: "new content".into(), + }], + }), + event_stream, + cx, + ) + }); + + event_rx.expect_update_fields().await; + let auth = event_rx.expect_authorization().await; + let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); + assert!( + title.contains("agent skills"), + "Authorization title should mention agent skills, got: {title}", + ); + auth.response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) + .expect("authorization response should send"); + + let EditFileToolOutput::Success { new_text, .. } = task.await.unwrap() else { + panic!("expected success"); + }; + assert_eq!(new_text, "new content\n"); + assert_eq!(fs.load(&skill_file).await.unwrap(), "new content\n"); + } + #[gpui::test] async fn test_streaming_edit_failed_match(cx: &mut TestAppContext) { let (edit_tool, _project, _action_log, _fs, _thread) = diff --git a/crates/agent/src/tools/edit_session.rs b/crates/agent/src/tools/edit_session.rs index 3eea6e57f3d..ed5112e908e 100644 --- a/crates/agent/src/tools/edit_session.rs +++ b/crates/agent/src/tools/edit_session.rs @@ -2,6 +2,7 @@ mod reindent; mod streaming_fuzzy_matcher; mod streaming_parser; +use super::tool_permissions::resolve_creatable_global_skill_path; use crate::{Thread, ToolCallEventStream}; use acp_thread::Diff; use action_log::ActionLog; @@ -363,6 +364,16 @@ pub(crate) struct EditSession { _finalize_diff_guard: Deferred>, } +/// The destination of an edit session, identified by its absolute path on +/// disk. `project_path` is `Some` for files that live inside one of the +/// project's worktrees (i.e. that the standard project-path machinery can +/// resolve), and `None` for global skill files reached through the +/// `~/.agents/skills` allowlist. +struct EditSessionTarget { + abs_path: PathBuf, + project_path: Option, +} + enum Pipeline { Write(WritePipeline), Edit(EditPipeline), @@ -650,16 +661,34 @@ impl EditSession { event_stream: &ToolCallEventStream, cx: &mut AsyncApp, ) -> Result { - let project_path = cx.update(|cx| resolve_path(mode, &path, &context.project, cx))?; + let target = if let Some(abs_path) = + resolve_global_skill_path_for_edit_session(mode, &path, &context, cx).await? + { + EditSessionTarget { + abs_path, + project_path: None, + } + } else { + let project_path = cx.update(|cx| resolve_path(mode, &path, &context.project, cx))?; - let Some(abs_path) = - cx.update(|cx| context.project.read(cx).absolute_path(&project_path, cx)) - else { - return Err(format!( - "Worktree at '{}' does not exist", - path.to_string_lossy() - )); + let Some(abs_path) = + cx.update(|cx| context.project.read(cx).absolute_path(&project_path, cx)) + else { + return Err(format!( + "Worktree at '{}' does not exist", + path.to_string_lossy() + )); + }; + + EditSessionTarget { + abs_path, + project_path: Some(project_path), + } }; + let EditSessionTarget { + abs_path, + project_path, + } = target; event_stream.update_fields( ToolCallUpdateFields::new().locations(vec![ToolCallLocation::new(abs_path.clone())]), @@ -669,11 +698,20 @@ impl EditSession { .await .map_err(|e| e.to_string())?; - let buffer = context - .project - .update(cx, |project, cx| project.open_buffer(project_path, cx)) - .await - .map_err(|e| e.to_string())?; + let buffer = match project_path { + Some(project_path) => context + .project + .update(cx, |project, cx| project.open_buffer(project_path, cx)) + .await + .map_err(|e| e.to_string())?, + None => context + .project + .update(cx, |project, cx| { + project.open_local_buffer(abs_path.clone(), cx) + }) + .await + .map_err(|e| e.to_string())?, + }; let file_changed_since_last_read = ensure_buffer_saved(&buffer, &abs_path, mode, &context, event_stream, cx).await?; @@ -1066,6 +1104,72 @@ async fn resolve_dirty_buffer( Ok(()) } +/// Mirrors [`resolve_path`]'s pre-auth validation for the global-skill +/// branch: returns `Ok(Some(abs_path))` if the path lives under +/// `~/.agents/skills` and is in a valid state for the requested mode, +/// `Ok(None)` if the path isn't a global skill at all (so the caller should +/// fall through to project-path resolution), or `Err(message)` if the path +/// is a global skill but can't be used (missing in Edit mode, parent +/// missing in Write mode, etc.). +/// +/// Errors returned from here surface to the model as tool-result errors +/// without prompting the user — same contract as [`resolve_path`]. The +/// idea is that "file doesn't exist" or "parent isn't a directory" are +/// model mistakes, not decisions the user should be asked to approve. +async fn resolve_global_skill_path_for_edit_session( + mode: EditSessionMode, + path: &PathBuf, + context: &EditSessionContext, + cx: &mut AsyncApp, +) -> Result, String> { + let fs = context + .project + .read_with(cx, |project, _cx| project.fs().clone()); + let Some(abs_path) = resolve_creatable_global_skill_path(path, fs.as_ref()).await else { + return Ok(None); + }; + + match mode { + EditSessionMode::Edit => { + let metadata = fs + .metadata(&abs_path) + .await + .map_err(|e| format!("Can't edit file: {e}"))? + .ok_or_else(|| "Can't edit file: path not found".to_string())?; + if metadata.is_dir { + return Err("Can't edit file: path is a directory".to_string()); + } + } + EditSessionMode::Write => { + if let Some(metadata) = fs + .metadata(&abs_path) + .await + .map_err(|e| format!("Can't write to file: {e}"))? + { + if metadata.is_dir { + return Err("Can't write to file: path is a directory".to_string()); + } + } else { + let parent_path = abs_path + .parent() + .ok_or_else(|| "Can't create file: incorrect path".to_string())?; + let parent_metadata = fs + .metadata(parent_path) + .await + .map_err(|e| format!("Can't create file: {e}"))? + .ok_or_else(|| { + "Can't create file: parent directory doesn't exist".to_string() + })?; + if !parent_metadata.is_dir { + return Err("Can't create file: parent is not a directory".to_string()); + } + } + } + } + + Ok(Some(abs_path)) +} + fn resolve_path( mode: EditSessionMode, path: &PathBuf, diff --git a/crates/agent/src/tools/list_directory_tool.rs b/crates/agent/src/tools/list_directory_tool.rs index b8c4dfd1c77..637c625bce0 100644 --- a/crates/agent/src/tools/list_directory_tool.rs +++ b/crates/agent/src/tools/list_directory_tool.rs @@ -18,11 +18,13 @@ use std::sync::Arc; use util::markdown::MarkdownInlineCode; /// Lists files and directories in a given path. Prefer the `grep` or `find_path` tools when searching the codebase. +/// +/// The only supported path outside the project is `~/.agents/skills` or a descendant, for global agent skills. #[derive(Debug, Serialize, Deserialize, JsonSchema)] pub struct ListDirectoryToolInput { /// The fully-qualified path of the directory to list in the project. /// - /// This path should never be absolute, and the first component of the path should always be a root directory in a project. + /// This path should never be absolute, and the first component of the path should always be a root directory in a project, unless it's a global agent skill directory under `~/.agents/skills`. /// /// /// If the project has the following root directories: @@ -41,6 +43,10 @@ pub struct ListDirectoryToolInput { /// /// If you wanna list contents in the directory `foo/baz`, you should use the path `foo/baz`. /// + /// + /// + /// To list a global agent skill directory, you may provide a path under `~/.agents/skills`, such as `~/.agents/skills/my-skill`. + /// pub path: String, } @@ -234,7 +240,7 @@ impl AgentTool for ListDirectoryTool { // Fast path: a global skill resource lives outside any worktree, so // standard project-path resolution would refuse it. If the path - // resolves under the global skills tree, list it directly. + // expands and resolves under the global skills tree, list it directly. if let Some(skill_path) = resolve_global_skill_path(Path::new(&input.path), fs.as_ref()).await { diff --git a/crates/agent/src/tools/read_file_tool.rs b/crates/agent/src/tools/read_file_tool.rs index 83ca602431d..cf075d74a29 100644 --- a/crates/agent/src/tools/read_file_tool.rs +++ b/crates/agent/src/tools/read_file_tool.rs @@ -155,11 +155,13 @@ use crate::{AgentTool, ToolCallEventStream, ToolInput, outline}; /// Do NOT retry reading the same file without line numbers if you receive an outline. /// - This tool supports reading image files. Supported formats: PNG, JPEG, WebP, GIF, BMP, TIFF. /// Image files are returned as visual content that you can analyze directly. +/// +/// The only supported path outside the project is `~/.agents/skills` or a descendant, for global agent skills. #[derive(Debug, Serialize, Deserialize, JsonSchema)] pub struct ReadFileToolInput { /// The relative path of the file to read. /// - /// This path should never be absolute, and the first component of the path should always be a root directory in a project. + /// This path should never be absolute, and the first component of the path should always be a root directory in a project, unless it's a global agent skill under `~/.agents/skills`. /// /// /// If the project has the following root directories: @@ -170,6 +172,10 @@ pub struct ReadFileToolInput { /// If you want to access `file.txt` in `directory1`, you should use the path `directory1/file.txt`. /// If you want to access `file.txt` in `directory2`, you should use the path `directory2/file.txt`. /// + /// + /// + /// To read a global agent skill file, you may provide a path under `~/.agents/skills`, such as `~/.agents/skills/my-skill/SKILL.md`. + /// pub path: String, /// Optional line number to start reading on (1-based index) #[serde(default)] @@ -251,8 +257,8 @@ impl AgentTool for ReadFileTool { .map_err(tool_content_err)?; let fs = project.read_with(cx, |project, _cx| project.fs().clone()); - // Fast path: if the model passes an absolute path that resolves - // under the global skills directory, read it directly via the + // Fast path: if the model passes a path that resolves under the + // global skills directory, read it directly via the // filesystem. Global skills live outside any worktree, so the // standard project-path machinery would refuse them. if let Some(skill_path) = diff --git a/crates/agent/src/tools/tool_permissions.rs b/crates/agent/src/tools/tool_permissions.rs index ec343071a9b..ebcc888db16 100644 --- a/crates/agent/src/tools/tool_permissions.rs +++ b/crates/agent/src/tools/tool_permissions.rs @@ -9,9 +9,9 @@ use fs::Fs; use gpui::{App, Entity, Task, WeakEntity}; use project::{Project, ProjectPath}; use settings::Settings; -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; use std::sync::Arc; -use util::paths::component_matches_ignore_ascii_case; +use util::{normalize_path, paths::component_matches_ignore_ascii_case}; pub enum SensitiveSettingsKind { Local, @@ -116,25 +116,80 @@ fn is_within_any_worktree(canonical_path: &Path, canonical_worktree_roots: &[Pat .any(|root| canonical_path.starts_with(root)) } -/// If `path` is an absolute path under the global skills directory -/// (`~/.agents/skills`), return the canonicalized absolute path. Returns -/// `None` for any path that resolves outside the global skills tree, for -/// relative paths, or if the skills directory itself can't be canonicalized -/// (fail closed — better to refuse access than to compare against a -/// non-canonical path). +/// If `path` names `~/.agents/skills` or one of its descendants, return the +/// canonicalized absolute path. Returns `None` for any path that resolves +/// outside the global skills tree, for relative paths that don't start with +/// `~`, or if the skills directory itself can't be canonicalized (fail closed +/// — better to refuse access than to compare against a non-canonical path). /// /// This is the gate that lets `read_file` / `list_directory` reach into the /// global skills directory — which lives outside any worktree — without /// also opening up arbitrary external paths. pub async fn resolve_global_skill_path(path: &Path, fs: &dyn Fs) -> Option { - if !path.is_absolute() { + let normalized_path = resolve_lexical_global_skill_path(path)?; + + // Canonicalize both sides so symlinks can't sneak the path out of the + // skills tree (and so different but equivalent path representations + // match). The lexical check above intentionally runs first, so a + // symlinked `~/.agents/skills` root can't broaden the allowlist to every + // path under the symlink target. + let canonical_path = fs.canonicalize(&normalized_path).await.ok()?; + let canonical_skills_dir = canonical_global_skills_dir(fs).await?; + + if canonical_path.starts_with(&canonical_skills_dir) { + Some(canonical_path) + } else { + None + } +} + +fn expand_home_prefix(path: &Path) -> Option { + if path.is_absolute() { + return Some(path.to_path_buf()); + } + + let mut components = path.components(); + let first_component = components.next()?; + if !matches!(first_component, Component::Normal(component) if component == "~") { return None; } - // Canonicalize both sides so symlinks and `..` segments can't sneak the - // path out of the skills tree (and so different but equivalent path - // representations match). - let canonical_path = fs.canonicalize(path).await.ok()?; + let mut expanded = paths::home_dir().clone(); + for component in components { + match component { + Component::Normal(component) => expanded.push(component), + Component::CurDir => {} + Component::ParentDir => expanded.push(".."), + Component::Prefix(_) | Component::RootDir => return None, + } + } + Some(expanded) +} + +fn expand_and_normalize_absolute_path(path: &Path) -> Option { + let expanded_path = expand_home_prefix(path)?; + let normalized_path = normalize_path(&expanded_path); + normalized_path.is_absolute().then_some(normalized_path) +} + +fn resolve_lexical_global_skill_path(path: &Path) -> Option { + let normalized_path = expand_and_normalize_absolute_path(path)?; + let normalized_skills_dir = normalize_path(&agent_skills::global_skills_dir()); + + normalized_path + .starts_with(&normalized_skills_dir) + .then_some(normalized_path) +} + +/// If `path` names `~/.agents/skills` or one of its descendants, return a +/// canonical absolute path for it. Unlike [`resolve_global_skill_path`], the +/// target path may or may not exist on disk yet — the caller decides whether +/// to read, write, or create it. Returns `None` for any other path, including +/// siblings of the global skills tree or paths that would escape it with `..` +/// or symlinks. +pub async fn resolve_creatable_global_skill_path(path: &Path, fs: &dyn Fs) -> Option { + let normalized_path = resolve_lexical_global_skill_path(path)?; + let canonical_path = canonicalize_with_ancestors(&normalized_path, fs).await?; let canonical_skills_dir = canonical_global_skills_dir(fs).await?; if canonical_path.starts_with(&canonical_skills_dir) { @@ -773,6 +828,189 @@ mod tests { roots } + #[gpui::test] + async fn test_resolve_creatable_global_skill_path_allows_tilde_path(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + let input_path = PathBuf::from("~") + .join(".agents") + .join("skills") + .join("my-skill"); + let expected_path = agent_skills::global_skills_dir().join("my-skill"); + + let resolved = resolve_creatable_global_skill_path(&input_path, fs.as_ref()) + .await + .expect("global skill path should resolve"); + + assert_eq!(resolved, expected_path); + } + + #[gpui::test] + async fn test_resolve_global_skill_path_allows_tilde_path(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + let skill_file = agent_skills::global_skills_dir() + .join("my-skill") + .join("SKILL.md"); + fs.insert_tree( + skill_file + .parent() + .expect("skill file should have a parent"), + json!({ "SKILL.md": "---\nname: my-skill\ndescription: test\n---" }), + ) + .await; + + let input_path = PathBuf::from("~") + .join(".agents") + .join("skills") + .join("my-skill") + .join("SKILL.md"); + let resolved = resolve_global_skill_path(&input_path, fs.as_ref()) + .await + .expect("global skill file should resolve"); + + assert_eq!(resolved, skill_file); + } + + #[gpui::test] + async fn test_resolve_creatable_global_skill_path_rejects_other_home_paths( + cx: &mut TestAppContext, + ) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + let sibling_path = PathBuf::from("~").join(".agents").join("not-skills"); + let escaped_path = PathBuf::from("~") + .join(".agents") + .join("skills") + .join("..") + .join("not-skills"); + + assert!( + resolve_creatable_global_skill_path(&sibling_path, fs.as_ref()) + .await + .is_none() + ); + assert!( + resolve_creatable_global_skill_path(&escaped_path, fs.as_ref()) + .await + .is_none() + ); + } + + #[gpui::test] + async fn test_resolve_creatable_global_skill_path_rejects_symlink_escape( + cx: &mut TestAppContext, + ) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + let skills_dir = agent_skills::global_skills_dir(); + fs.create_dir(&skills_dir) + .await + .expect("global skills directory should be created"); + fs.create_dir(path!("/external").as_ref()) + .await + .expect("external directory should be created"); + fs.create_symlink(&skills_dir.join("link"), PathBuf::from(path!("/external"))) + .await + .expect("symlink should be created"); + + let escaped_path = PathBuf::from("~") + .join(".agents") + .join("skills") + .join("link") + .join("new-dir"); + + assert!( + resolve_creatable_global_skill_path(&escaped_path, fs.as_ref()) + .await + .is_none() + ); + } + + #[gpui::test] + async fn test_global_skill_path_resolvers_reject_absolute_paths_when_skills_dir_is_symlink_to_root( + cx: &mut TestAppContext, + ) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree(paths::home_dir(), json!({ ".agents": {} })) + .await; + fs.insert_tree(path!("/tmp"), json!({ "outside.txt": "outside" })) + .await; + + let skills_dir = agent_skills::global_skills_dir(); + fs.create_symlink(&skills_dir, PathBuf::from(path!("/"))) + .await + .expect("global skills directory should be symlinked to root"); + + let outside_path = PathBuf::from(path!("/tmp/outside.txt")); + assert!( + resolve_global_skill_path(&outside_path, fs.as_ref()) + .await + .is_none(), + "existing absolute paths outside the lexical global skills tree should not resolve", + ); + assert!( + resolve_creatable_global_skill_path(&outside_path, fs.as_ref()) + .await + .is_none(), + "creatable absolute paths outside the lexical global skills tree should not resolve", + ); + + let traversed_path = PathBuf::from("~") + .join(".agents") + .join("skills") + .join("..") + .join("outside"); + assert!( + resolve_creatable_global_skill_path(&traversed_path, fs.as_ref()) + .await + .is_none(), + "paths that normalize outside the lexical global skills tree should not resolve", + ); + } + + #[gpui::test] + async fn test_global_skill_path_resolvers_reject_absolute_paths_when_skills_dir_is_symlink_to_home( + cx: &mut TestAppContext, + ) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + paths::home_dir(), + json!({ + ".agents": {}, + "outside.txt": "outside", + }), + ) + .await; + + let skills_dir = agent_skills::global_skills_dir(); + fs.create_symlink(&skills_dir, paths::home_dir().clone()) + .await + .expect("global skills directory should be symlinked to home"); + + let outside_path = paths::home_dir().join("outside.txt"); + assert!( + resolve_global_skill_path(&outside_path, fs.as_ref()) + .await + .is_none(), + "existing absolute paths outside the lexical global skills tree should not resolve", + ); + assert!( + resolve_creatable_global_skill_path(&outside_path, fs.as_ref()) + .await + .is_none(), + "creatable absolute paths outside the lexical global skills tree should not resolve", + ); + } + #[gpui::test] async fn test_resolve_project_path_safe_for_normal_files(cx: &mut TestAppContext) { init_test(cx); diff --git a/crates/agent/src/tools/write_file_tool.rs b/crates/agent/src/tools/write_file_tool.rs index 5e8ce0016ce..89942c316ce 100644 --- a/crates/agent/src/tools/write_file_tool.rs +++ b/crates/agent/src/tools/write_file_tool.rs @@ -22,11 +22,13 @@ const DEFAULT_UI_TEXT: &str = "Writing file"; /// To make granular edits to an existing file, prefer the `edit_file` tool instead. /// /// Before using this tool, verify the directory path is correct (only applicable when creating new files). Use the `list_directory` tool to verify the parent directory exists and is the correct location +/// +/// The only supported path outside the project is `~/.agents/skills` or a descendant, for global agent skills. #[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)] pub struct WriteFileToolInput { /// The full path of the file to create or overwrite in the project. /// - /// WARNING: When specifying which file path need changing, you MUST start each path with one of the project's root directories. + /// WARNING: When specifying which file path need changing, you MUST start each path with one of the project's root directories, unless it's a global agent skill under `~/.agents/skills`. /// /// The following examples assume we have two root directories in the project: /// - /a/b/backend @@ -41,6 +43,10 @@ pub struct WriteFileToolInput { /// /// `frontend/db.js` /// + /// + /// + /// To create or overwrite a global agent skill file, you may provide a path under `~/.agents/skills`, such as `~/.agents/skills/my-skill/SKILL.md`. + /// pub path: PathBuf, /// The entire content for the file. @@ -273,7 +279,7 @@ mod tests { use prompt_store::ProjectContext; use serde_json::json; use settings::{Settings, SettingsStore}; - use std::sync::Arc; + use std::{path::PathBuf, sync::Arc}; use util::path; use util::rel_path::{RelPath, rel_path}; @@ -328,6 +334,62 @@ mod tests { assert_eq!(*old_text, "old content"); } + #[gpui::test] + async fn test_streaming_write_global_skill_file(cx: &mut TestAppContext) { + init_test(cx); + + let fs = project::FakeFs::new(cx.executor()); + fs.insert_tree(path!("/root"), json!({})).await; + let skill_dir = agent_skills::global_skills_dir().join("my-skill"); + fs.insert_tree(&skill_dir, json!({})).await; + let (write_tool, _project, _action_log, fs, _thread) = + setup_test_with_fs(cx, fs, &[path!("/root").as_ref()]).await; + + let input_path = PathBuf::from("~") + .join(".agents") + .join("skills") + .join("my-skill") + .join("SKILL.md"); + let skill_file = agent_skills::global_skills_dir() + .join("my-skill") + .join("SKILL.md"); + + let (event_stream, mut event_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + write_tool.clone().run( + ToolInput::resolved(WriteFileToolInput { + path: input_path, + content: "# My Skill\n".into(), + }), + event_stream, + cx, + ) + }); + + event_rx.expect_update_fields().await; + let auth = event_rx.expect_authorization().await; + let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); + assert!( + title.contains("agent skills"), + "Authorization title should mention agent skills, got: {title}", + ); + auth.response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) + .expect("authorization response should send"); + + let EditSessionOutput::Success { new_text, .. } = task.await.unwrap() else { + panic!("expected success"); + }; + assert_eq!(new_text, "# My Skill\n"); + assert_eq!( + fs.load(&skill_file).await.unwrap().replace("\r\n", "\n"), + "# My Skill\n" + ); + } + #[gpui::test] async fn test_streaming_path_completeness_heuristic(cx: &mut TestAppContext) { let (write_tool, _project, _action_log, _fs, _thread) = diff --git a/crates/agent_skills/builtin/create-skill/SKILL.md b/crates/agent_skills/builtin/create-skill/SKILL.md index 4728ca46714..e388d84f708 100644 --- a/crates/agent_skills/builtin/create-skill/SKILL.md +++ b/crates/agent_skills/builtin/create-skill/SKILL.md @@ -88,8 +88,8 @@ Reference these in the skill body. The agent can read them using the file path s 1. Decide on scope (global vs project-local) based on the user's needs. 2. Choose a descriptive, hyphenated name. -3. Create the directory structure. -4. Write the `SKILL.md` with frontmatter and instructions. +3. Create the directory structure. The `create_directory` tool normally only creates directories inside the current project, but it has a special allow case for global skills under `~/.agents/skills`. +4. Write the `SKILL.md` with frontmatter and instructions. The `write_file` and `edit_file` tools also have a special allow case for creating or modifying files under `~/.agents/skills`. 5. Optionally add supporting files (templates, examples, references). After creating the skill, it will be automatically discovered by Zed's agent on the next conversation (no restart needed for global skills if the `~/.agents/skills/` directory already exists).