mirror of
https://github.com/zed-industries/zed.git
synced 2026-06-01 03:14:56 +07:00
Cherry-pick of #57678 to preview ---- Summary - Allow write_file and edit_file to create or modify files under ~/.agents/skills. - Keep the global skills exception constrained to that directory and preserve existing project-path behavior. - Document global skill file editing support in the built-in create-skill instructions. Tests - cargo fmt -p agent - cargo test -p agent global_skill_file - cargo test -p agent test_create_directory_allows_global_skill_directory Release Notes: - Fixed agent file editing for global skills --------- Co-authored-by: Richard Feldman <oss@rtfeldman.com> Co-authored-by: MartinYe1234 <52641447+MartinYe1234@users.noreply.github.com> Co-authored-by: Richard Feldman <oss@rtfeldman.com>
This commit is contained in:
parent
d8a151ee29
commit
67e5465fc2
8 changed files with 626 additions and 37 deletions
|
|
@ -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"
|
||||
/// </example>
|
||||
///
|
||||
/// <example>
|
||||
/// To create a global agent skill directory, you may provide a path under `~/.agents/skills`, such as `~/.agents/skills/my-skill`.
|
||||
/// </example>
|
||||
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);
|
||||
|
|
|
|||
|
|
@ -25,11 +25,13 @@ 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.
|
||||
#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)]
|
||||
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
|
||||
|
|
@ -44,6 +46,10 @@ pub struct EditFileToolInput {
|
|||
/// <example>
|
||||
/// `frontend/db.js`
|
||||
/// </example>
|
||||
///
|
||||
/// <example>
|
||||
/// To edit a global agent skill file, you may provide a path under `~/.agents/skills`, such as `~/.agents/skills/my-skill/SKILL.md`.
|
||||
/// </example>
|
||||
pub path: PathBuf,
|
||||
|
||||
/// List of edit operations to apply sequentially.
|
||||
|
|
@ -457,6 +463,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) =
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
@ -352,6 +353,16 @@ pub(crate) struct EditSession {
|
|||
_finalize_diff_guard: Deferred<Box<dyn FnOnce()>>,
|
||||
}
|
||||
|
||||
/// 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<ProjectPath>,
|
||||
}
|
||||
|
||||
enum Pipeline {
|
||||
Write(WritePipeline),
|
||||
Edit(EditPipeline),
|
||||
|
|
@ -639,16 +650,34 @@ impl EditSession {
|
|||
event_stream: &ToolCallEventStream,
|
||||
cx: &mut AsyncApp,
|
||||
) -> Result<Self, String> {
|
||||
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())]),
|
||||
|
|
@ -658,11 +687,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?;
|
||||
|
|
@ -1055,6 +1093,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<Option<PathBuf>, 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,
|
||||
|
|
|
|||
|
|
@ -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`.
|
||||
///
|
||||
/// <example>
|
||||
/// 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`.
|
||||
/// </example>
|
||||
///
|
||||
/// <example>
|
||||
/// To list a global agent skill directory, you may provide a path under `~/.agents/skills`, such as `~/.agents/skills/my-skill`.
|
||||
/// </example>
|
||||
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
|
||||
{
|
||||
|
|
|
|||
|
|
@ -86,11 +86,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`.
|
||||
///
|
||||
/// <example>
|
||||
/// If the project has the following root directories:
|
||||
|
|
@ -101,6 +103,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`.
|
||||
/// </example>
|
||||
///
|
||||
/// <example>
|
||||
/// To read a global agent skill file, you may provide a path under `~/.agents/skills`, such as `~/.agents/skills/my-skill/SKILL.md`.
|
||||
/// </example>
|
||||
pub path: String,
|
||||
/// Optional line number to start reading on (1-based index)
|
||||
#[serde(default)]
|
||||
|
|
@ -182,8 +188,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) =
|
||||
|
|
|
|||
|
|
@ -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<PathBuf> {
|
||||
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<PathBuf> {
|
||||
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<PathBuf> {
|
||||
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<PathBuf> {
|
||||
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<PathBuf> {
|
||||
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);
|
||||
|
|
|
|||
|
|
@ -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 {
|
|||
/// <example>
|
||||
/// `frontend/db.js`
|
||||
/// </example>
|
||||
///
|
||||
/// <example>
|
||||
/// 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`.
|
||||
/// </example>
|
||||
pub path: PathBuf,
|
||||
|
||||
/// The entire content for the file.
|
||||
|
|
@ -272,7 +278,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};
|
||||
|
||||
|
|
@ -327,6 +333,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) =
|
||||
|
|
|
|||
|
|
@ -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).
|
||||
|
|
|
|||
Loading…
Reference in a new issue