From 45b322c1df30295caaceb23a8a043fc9fa032527 Mon Sep 17 00:00:00 2001 From: Jasper Date: Fri, 21 Aug 2026 15:36:31 +0000 Subject: [PATCH] fix(security): honor plugin enablement for skills (#11439) Signed-off-by: Jasper Hugo --- crates/goose/src/plugins/discovery.rs | 162 ++++++- crates/goose/src/plugins/mod.rs | 72 ++- crates/goose/src/skills/client.rs | 212 ++++++++- crates/goose/src/skills/mod.rs | 642 +++++++++++++++++++++++++- 4 files changed, 1019 insertions(+), 69 deletions(-) diff --git a/crates/goose/src/plugins/discovery.rs b/crates/goose/src/plugins/discovery.rs index ee4402f26..cc8450d6c 100644 --- a/crates/goose/src/plugins/discovery.rs +++ b/crates/goose/src/plugins/discovery.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; use serde::{Deserialize, Serialize}; @@ -6,13 +6,13 @@ use serde::{Deserialize, Serialize}; use crate::config::{paths::Paths, Config}; use crate::plugins::plugin_install_dir; -const PLUGINS_CONFIG_KEY: &str = "plugins"; +pub(in crate::plugins) const PLUGINS_CONFIG_KEY: &str = "plugins"; /// Per-plugin entry stored under the `plugins` map in `config.yaml`, keyed by /// the plugin's filesystem path. #[derive(Debug, Clone, Serialize, Deserialize)] -struct PluginConfigEntry { - enabled: bool, +pub(in crate::plugins) struct PluginConfigEntry { + pub enabled: bool, } /// A plugin found on disk and not disabled by any settings file. @@ -53,36 +53,68 @@ pub fn discover_enabled_plugins(project_root: Option<&Path>) -> Vec, config: &Config, ) -> Vec { let scoped_settings = load_all_settings(project_root); - let mut found: HashMap = HashMap::new(); + let user_plugins_dir = plugin_install_dir(); + let mut found = Vec::new(); if let Some(root) = project_root { - for (name, root) in list_dir_children(&project_plugin_dir(root)) { - found.entry(name.clone()).or_insert(DiscoveredPlugin { - name, - root, - scope: PluginScope::Project, - }); + let project_plugins_dir = project_plugin_dir(root); + if !equivalent_paths(&project_plugins_dir, &user_plugins_dir) { + found.extend(list_dir_children(&project_plugins_dir).into_iter().map( + |(name, root)| DiscoveredPlugin { + name, + root, + scope: PluginScope::Project, + }, + )); } } - for (name, root) in list_dir_children(&plugin_install_dir()) { - found.entry(name.clone()).or_insert(DiscoveredPlugin { - name, - root, - scope: PluginScope::User, - }); - } + found.extend( + list_dir_children(&user_plugins_dir) + .into_iter() + .map(|(name, root)| DiscoveredPlugin { + name, + root, + scope: PluginScope::User, + }), + ); - let enabled_by_settings: Vec = found - .into_values() + let mut enabled_plugins: Vec = filter_by_config(found, config) + .into_iter() .filter(|plugin| is_enabled(&plugin.name, &scoped_settings)) .collect(); + enabled_plugins.sort_by(|left, right| { + plugin_scope_rank(left.scope) + .cmp(&plugin_scope_rank(right.scope)) + .then_with(|| left.name.cmp(&right.name)) + .then_with(|| left.root.cmp(&right.root)) + }); - filter_by_config(enabled_by_settings, config) + let mut seen_names = HashSet::new(); + enabled_plugins + .into_iter() + .filter(|plugin| seen_names.insert(plugin.name.clone())) + .collect() +} + +fn equivalent_paths(left: &Path, right: &Path) -> bool { + left == right + || left + .canonicalize() + .ok() + .zip(right.canonicalize().ok()) + .is_some_and(|(left, right)| left == right) +} + +fn plugin_scope_rank(scope: PluginScope) -> u8 { + match scope { + PluginScope::Project => 0, + PluginScope::User => 1, + } } /// Apply the `plugins` map in `config.yaml`. Newly discovered plugins are added @@ -281,6 +313,7 @@ mod tests { fn disabled_in_project_settings_drops_plugin() { let tmp = tempfile::tempdir().unwrap(); let project = tmp.path(); + let plugin_root = project.join(".agents/plugins/demo"); write_plugin_dir(&project.join(".agents").join("plugins"), "demo"); write_settings( @@ -288,8 +321,16 @@ mod tests { r#"{"disabledPlugins":["demo"]}"#, ); - let found = discover(project); + let cfg_dir = tempfile::tempdir().unwrap(); + let config = test_config(cfg_dir.path()); + let found = discover_with_config(project, &config); assert!(found.iter().all(|p| p.name != "demo")); + + let entries: HashMap = + config.get_param(PLUGINS_CONFIG_KEY).unwrap(); + assert!(entries + .get(&plugin_root.to_string_lossy().into_owned()) + .is_some_and(|entry| entry.enabled)); } #[test] @@ -364,6 +405,48 @@ mod tests { ); } + #[test] + fn disabled_project_plugin_falls_back_to_enabled_user_plugin_with_same_name() { + let project = tempfile::tempdir().unwrap(); + let project_plugins = project.path().join(".agents/plugins"); + write_plugin_dir(&project_plugins, "demo"); + + let path_root = tempfile::tempdir().unwrap(); + let user_plugins = path_root.path().join(".agents/plugins"); + write_plugin_dir(&user_plugins, "demo"); + let config_dir = tempfile::tempdir().unwrap(); + let config = test_config(config_dir.path()); + config + .set_param( + PLUGINS_CONFIG_KEY, + HashMap::from([ + ( + project_plugins.join("demo").to_string_lossy().into_owned(), + PluginConfigEntry { enabled: false }, + ), + ( + user_plugins.join("demo").to_string_lossy().into_owned(), + PluginConfigEntry { enabled: true }, + ), + ]), + ) + .unwrap(); + let _guard = env_lock::lock_env([ + ("GOOSE_PATH_ROOT", path_root.path().to_str()), + ("PLUGINS", None), + ]); + + let found = discover_enabled_plugins_with_config(Some(project.path()), &config); + let demo: Vec<_> = found + .into_iter() + .filter(|plugin| plugin.name == "demo") + .collect(); + + assert_eq!(demo.len(), 1); + assert_eq!(demo[0].scope, PluginScope::User); + assert_eq!(demo[0].root, user_plugins.join("demo")); + } + #[test] fn newly_discovered_plugin_is_added_to_config_as_enabled() { let tmp = tempfile::tempdir().unwrap(); @@ -439,4 +522,37 @@ mod tests { config.get_param(PLUGINS_CONFIG_KEY).unwrap(); assert!(entries.get(&key).is_some_and(|e| e.enabled)); } + + #[test] + fn orders_plugins_by_scope_then_name() { + let project = tempfile::tempdir().unwrap(); + write_plugin_dir(&project.path().join(".agents/plugins"), "z-project-plugin"); + write_plugin_dir(&project.path().join(".agents/plugins"), "a-project-plugin"); + + let path_root = tempfile::tempdir().unwrap(); + write_plugin_dir(&path_root.path().join(".agents/plugins"), "z-user-plugin"); + write_plugin_dir(&path_root.path().join(".agents/plugins"), "a-user-plugin"); + let config_dir = tempfile::tempdir().unwrap(); + let config = test_config(config_dir.path()); + let _guard = env_lock::lock_env([ + ("GOOSE_PATH_ROOT", path_root.path().to_str()), + ("PLUGINS", None), + ]); + + let found = discover_enabled_plugins_with_config(Some(project.path()), &config); + let ordered: Vec<_> = found + .into_iter() + .map(|plugin| (plugin.name, plugin.scope)) + .collect(); + + assert_eq!( + ordered, + vec![ + ("a-project-plugin".to_string(), PluginScope::Project), + ("z-project-plugin".to_string(), PluginScope::Project), + ("a-user-plugin".to_string(), PluginScope::User), + ("z-user-plugin".to_string(), PluginScope::User), + ] + ); + } } diff --git a/crates/goose/src/plugins/mod.rs b/crates/goose/src/plugins/mod.rs index 512f348dc..dd074df97 100644 --- a/crates/goose/src/plugins/mod.rs +++ b/crates/goose/src/plugins/mod.rs @@ -3,12 +3,14 @@ pub mod formats; pub mod mcp_servers; use crate::config::paths::Paths; +use crate::config::Config; +use crate::plugins::discovery::PluginScope; use crate::subprocess::{git_command, SubprocessExt}; use anyhow::{anyhow, bail, Result}; use chrono::{DateTime, Duration, Utc}; use fs_err as fs; use serde::{Deserialize, Serialize}; -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; use tracing::warn; @@ -82,6 +84,49 @@ struct InstallMetadata { } pub fn installed_plugin_skill_dirs() -> Vec { + enabled_plugin_skill_dirs(None) + .into_iter() + .map(|(path, _)| path) + .collect() +} + +pub(crate) fn enabled_plugin_skill_dirs( + project_root: Option<&Path>, +) -> Vec<(PathBuf, PluginScope)> { + enabled_plugin_skill_dirs_with_config(project_root, Config::global()) +} + +fn is_project_plugin_install_dir(path: &Path) -> bool { + path.parent().is_some_and(|parent| { + parent.file_name().and_then(|name| name.to_str()) == Some("plugins") + && parent + .parent() + .and_then(|grandparent| grandparent.file_name()) + .and_then(|name| name.to_str()) + == Some(".agents") + }) +} + +pub(crate) fn configured_project_plugin_skill_dirs(config: &Config) -> Vec { + let entries: HashMap = config + .get_param(discovery::PLUGINS_CONFIG_KEY) + .unwrap_or_default(); + let user_plugins_dir = plugin_install_dir(); + let mut seen = HashSet::new(); + + entries + .into_keys() + .map(PathBuf::from) + .filter(|path| is_project_plugin_install_dir(path) && !path.starts_with(&user_plugins_dir)) + .flat_map(|path| formats::open_plugins::installed_skill_dirs(&path)) + .filter(|path| seen.insert(path.clone())) + .collect() +} + +pub(crate) fn enabled_plugin_skill_dirs_with_config( + project_root: Option<&Path>, + config: &Config, +) -> Vec<(PathBuf, PluginScope)> { let plugins_dir = plugin_install_dir(); for update in auto_update_plugins_at_root(Utc::now(), &plugins_dir) { if let Err(err) = update.result { @@ -92,25 +137,16 @@ pub fn installed_plugin_skill_dirs() -> Vec { } } - let entries = match fs::read_dir(plugins_dir) { - Ok(entries) => entries, - Err(_) => return Vec::new(), - }; - let mut seen = HashSet::new(); - entries - .flatten() - .flat_map(|entry| { - let plugin_dir = entry.path(); - let default_skills_dir = plugin_dir.join("skills"); - let mut skill_dirs = Vec::new(); - if default_skills_dir.is_dir() { - skill_dirs.push(default_skills_dir); - } - skill_dirs.extend(formats::open_plugins::installed_skill_dirs(&plugin_dir)); - skill_dirs + discovery::discover_enabled_plugins_with_config(project_root, config) + .into_iter() + .flat_map(|plugin| { + let plugin_dir = plugin.root; + formats::open_plugins::installed_skill_dirs(&plugin_dir) + .into_iter() + .map(move |dir| (dir, plugin.scope)) }) - .filter(|path| seen.insert(path.clone())) + .filter(|(path, _)| seen.insert(path.clone())) .collect() } diff --git a/crates/goose/src/skills/client.rs b/crates/goose/src/skills/client.rs index d0816cb8e..76546bf82 100644 --- a/crates/goose/src/skills/client.rs +++ b/crates/goose/src/skills/client.rs @@ -1,8 +1,9 @@ -use super::discover_skills; +use super::discover_skills_with_config; use super::loaded_skill_context_with_args; use crate::agents::extension::PlatformExtensionContext; use crate::agents::mcp_client::{Error, McpClientTrait}; use crate::agents::ToolCallContext; +use crate::config::Config; use async_trait::async_trait; use goose_sdk_types::custom_requests::{SourceEntry, SourceType}; use rmcp::model::{ @@ -19,6 +20,7 @@ pub struct SkillsClient { info: InitializeResult, working_dir: PathBuf, exclude_builtin_skills: bool, + config: &'static Config, } impl SkillsClient { @@ -36,6 +38,7 @@ impl SkillsClient { info, working_dir, exclude_builtin_skills: false, + config: Config::global(), }) } @@ -46,8 +49,14 @@ impl SkillsClient { self } + #[cfg(test)] + fn with_config(mut self, config: &'static Config) -> Self { + self.config = config; + self + } + fn discover_skills(&self) -> Vec { - discover_skills(Some(&self.working_dir)) + discover_skills_with_config(Some(&self.working_dir), self.config) .into_iter() .filter(|skill| { !self.exclude_builtin_skills || skill.source_type != SourceType::BuiltinSkill @@ -148,18 +157,28 @@ impl McpClientTrait for SkillsClient { s.name == parent_skill_name && matches!(s.source_type, SourceType::Skill | SourceType::BuiltinSkill) }) { - let skill_dir = PathBuf::from(&skill.path); + let listed_skill_dir = PathBuf::from(&skill.path); + let load_skill_dir = match listed_skill_dir.canonicalize() { + Ok(path) => path, + Err(e) => { + return Ok(CallToolResult::error(vec![ContentBlock::text(format!( + "Failed to resolve '{}': {}", + parent_skill_name, e + ))])); + } + }; for file_path in &skill.supporting_files { let file_path_buf = Path::new(file_path); - let Ok(rel) = file_path_buf.strip_prefix(&skill_dir) else { + let Ok(rel) = file_path_buf.strip_prefix(&listed_skill_dir) else { continue; }; if rel.to_string_lossy().replace('\\', "/") != relative_path { continue; } - let result = match super::load_supporting_file(&skill_dir, rel, skill_name) { + let result = match super::load_supporting_file(&load_skill_dir, rel, skill_name) + { Ok(content) => CallToolResult::success(vec![ContentBlock::text(content)]), Err(e) => CallToolResult::error(vec![ContentBlock::text(format!( "Failed to read '{}': {}", @@ -174,7 +193,7 @@ impl McpClientTrait for SkillsClient { .iter() .filter_map(|f| { Path::new(f) - .strip_prefix(&skill_dir) + .strip_prefix(&listed_skill_dir) .ok() .map(|r| r.to_string_lossy().replace('\\', "/")) }) @@ -256,10 +275,191 @@ impl McpClientTrait for SkillsClient { #[cfg(test)] mod tests { use super::*; + use crate::config::Config; + use std::collections::HashMap; use std::fs; use std::sync::Arc; use tempfile::TempDir; + fn write_plugin_skill( + project: &Path, + plugin_name: &str, + skill_name: &str, + description: &str, + body: &str, + ) { + let skill_dir = project + .join(".agents/plugins") + .join(plugin_name) + .join("skills") + .join(skill_name); + fs::create_dir_all(&skill_dir).unwrap(); + fs::write( + skill_dir.join("SKILL.md"), + format!("---\nname: {skill_name}\ndescription: {description}\n---\n{body}"), + ) + .unwrap(); + } + + fn write_open_plugin_manifest(project: &Path, plugin_name: &str) { + let plugin_dir = project.join(".agents/plugins").join(plugin_name); + fs::write( + plugin_dir.join("plugin.json"), + format!( + r#"{{"name":"{plugin_name}","skills":{{"paths":["./skills","./custom-skills"]}}}}"# + ), + ) + .unwrap(); + } + + fn test_client(project: &Path, plugin_name: &str, enabled: bool) -> SkillsClient { + let config = Box::leak(Box::new( + Config::new(project.join("test-config.yaml"), "goose-skills-test").unwrap(), + )); + let plugin_root = project.join(".agents/plugins").join(plugin_name); + config + .set_param( + "plugins", + HashMap::from([( + plugin_root.to_string_lossy().into_owned(), + HashMap::from([("enabled", enabled)]), + )]), + ) + .unwrap(); + let session = Arc::new(crate::session::Session { + working_dir: project.to_path_buf(), + ..crate::session::Session::default() + }); + SkillsClient::new(PlatformExtensionContext { + extension_manager: None, + session_manager: Arc::new(crate::session::SessionManager::instance()), + scheduler: None, + session: Some(session), + use_login_shell_path: false, + }) + .unwrap() + .with_builtin_skills(false) + .with_config(config) + } + + fn result_text(result: &CallToolResult) -> &str { + match &result.content[0] { + ContentBlock::Text(text) => &text.text, + _ => panic!("expected text"), + } + } + + #[tokio::test] + async fn disabled_plugin_skill_is_not_listed_or_loadable() { + let _guard = env_lock::lock_env([("PLUGINS", None::<&str>)]); + let project = TempDir::new().unwrap(); + write_plugin_skill( + project.path(), + "disabled-plugin", + "disabled-plugin-skill", + "Disabled plugin metadata", + "disabled plugin full body", + ); + let client = test_client(project.path(), "disabled-plugin", false); + + assert!(client + .get_instructions() + .is_none_or(|instructions| !instructions.contains("disabled-plugin-skill"))); + + let ctx = ToolCallContext::new("test".to_string(), None, None); + let args = serde_json::from_value(serde_json::json!({ + "name": "disabled-plugin-skill" + })) + .unwrap(); + let result = client + .call_tool(&ctx, "load_skill", Some(args), CancellationToken::new()) + .await + .unwrap(); + + assert!(result.is_error.unwrap_or(false)); + assert!(!result_text(&result).contains("disabled plugin full body")); + } + + #[tokio::test] + async fn enabled_plugin_skill_is_listed_and_loadable() { + let _guard = env_lock::lock_env([("PLUGINS", None::<&str>)]); + let project = TempDir::new().unwrap(); + write_plugin_skill( + project.path(), + "enabled-plugin", + "enabled-plugin-skill", + "Enabled plugin metadata", + "enabled plugin full body", + ); + let custom_skill_dir = project + .path() + .join(".agents/plugins/enabled-plugin/custom-skills/custom-plugin-skill"); + fs::create_dir_all(&custom_skill_dir).unwrap(); + fs::write( + custom_skill_dir.join("SKILL.md"), + "---\nname: custom-plugin-skill\ndescription: Custom plugin metadata\n---\ncustom plugin full body", + ) + .unwrap(); + write_open_plugin_manifest(project.path(), "enabled-plugin"); + let client = test_client(project.path(), "enabled-plugin", true); + + let instructions = client.get_instructions().unwrap(); + assert!(instructions.contains("enabled-plugin-skill")); + assert!(instructions.contains("Enabled plugin metadata")); + assert!(instructions.contains("custom-plugin-skill")); + assert!(instructions.contains("Custom plugin metadata")); + + let ctx = ToolCallContext::new("test".to_string(), None, None); + let args = serde_json::from_value(serde_json::json!({ + "name": "custom-plugin-skill" + })) + .unwrap(); + let result = client + .call_tool(&ctx, "load_skill", Some(args), CancellationToken::new()) + .await + .unwrap(); + + assert!(!result.is_error.unwrap_or(false)); + assert!(result_text(&result).contains("custom plugin full body")); + } + + #[cfg(unix)] + #[tokio::test] + async fn symlinked_project_plugin_supporting_file_is_loadable() { + let _guard = env_lock::lock_env([("PLUGINS", None::<&str>)]); + let project = TempDir::new().unwrap(); + let external = TempDir::new().unwrap(); + write_plugin_skill( + external.path(), + "symlinked-plugin", + "symlinked-skill", + "Symlinked skill metadata", + "symlinked skill body", + ); + write_open_plugin_manifest(external.path(), "symlinked-plugin"); + let external_plugin = external.path().join(".agents/plugins/symlinked-plugin"); + let supporting_file = external_plugin.join("skills/symlinked-skill/guide.md"); + fs::write(&supporting_file, "Symlinked supporting guidance.").unwrap(); + + let plugin_link = project.path().join(".agents/plugins/symlinked-plugin"); + fs::create_dir_all(plugin_link.parent().unwrap()).unwrap(); + std::os::unix::fs::symlink(&external_plugin, &plugin_link).unwrap(); + let client = test_client(project.path(), "symlinked-plugin", true); + + let ctx = ToolCallContext::new("test".to_string(), None, None); + let args = serde_json::from_value(serde_json::json!({ + "name": "symlinked-skill/guide.md" + })) + .unwrap(); + let result = client + .call_tool(&ctx, "load_skill", Some(args), CancellationToken::new()) + .await + .unwrap(); + + assert!(!result.is_error.unwrap_or(false)); + assert!(result_text(&result).contains("Symlinked supporting guidance.")); + } + #[tokio::test] async fn test_load_filesystem_skill_without_builtin_skills() { let temp_dir = TempDir::new().unwrap(); diff --git a/crates/goose/src/skills/mod.rs b/crates/goose/src/skills/mod.rs index 2531305c4..6c641e43f 100644 --- a/crates/goose/src/skills/mod.rs +++ b/crates/goose/src/skills/mod.rs @@ -11,7 +11,11 @@ pub use client::{SkillsClient, EXTENSION_NAME}; pub(crate) use supporting_files::{load_supporting_file, read_source_file}; use crate::config::{paths::Paths, Config}; -use crate::plugins::installed_plugin_skill_dirs; +use crate::plugins::discovery::PluginScope; +use crate::plugins::{ + configured_project_plugin_skill_dirs, enabled_plugin_skill_dirs_with_config, + installed_plugin_skill_dirs, +}; use crate::sources::parse_frontmatter; use agent_client_protocol::Error; use anyhow::Result; @@ -192,6 +196,21 @@ fn canonicalize_or_original(path: &Path) -> PathBuf { path.canonicalize().unwrap_or_else(|_| path.to_path_buf()) } +fn is_lexical_project_plugin_path(path: &Path) -> bool { + if path.starts_with(Paths::plugins_dir()) { + return false; + } + + path.ancestors().any(|ancestor| { + ancestor.file_name().and_then(|name| name.to_str()) == Some("plugins") + && ancestor + .parent() + .and_then(|parent| parent.file_name()) + .and_then(|name| name.to_str()) + == Some(".agents") + }) +} + fn inferred_discoverable_skill_root(path: &Path) -> Option { let canonical_path = canonicalize_or_original(path); @@ -225,16 +244,30 @@ fn inferred_discoverable_skill_root(path: &Path) -> Option { }) } -pub(crate) fn resolve_discoverable_skill_dir(path: &str) -> Result { +fn resolve_discoverable_skill_dir_with_config( + path: &str, + config: &Config, +) -> Result { if path.is_empty() { return Err(Error::invalid_params().data("Source path must not be empty")); } + if is_lexical_project_plugin_path(Path::new(path)) { + return Err(Error::invalid_params().data(format!("Source \"{}\" not found", path))); + } + let canonical_dir = Path::new(path) .canonicalize() .map_err(|_| Error::invalid_params().data(format!("Source \"{}\" not found", path)))?; - if inferred_discoverable_skill_root(&canonical_dir).is_none() + let configured_project_plugin_path = !Path::new(path).starts_with(Paths::plugins_dir()) + && configured_project_plugin_skill_dirs(config) + .into_iter() + .map(|root| canonicalize_or_original(&root)) + .any(|root| canonical_dir.starts_with(root)); + + if configured_project_plugin_path + || inferred_discoverable_skill_root(&canonical_dir).is_none() || !canonical_dir.is_dir() || !canonical_dir.join("SKILL.md").is_file() { @@ -244,6 +277,10 @@ pub(crate) fn resolve_discoverable_skill_dir(path: &str) -> Result Result { + resolve_discoverable_skill_dir_with_config(path, Config::global()) +} + pub(crate) fn resolve_skill_dir(path: &str) -> Result { resolve_discoverable_skill_dir(path) } @@ -316,34 +353,99 @@ pub(crate) fn parse_skill_frontmatter(raw: &str) -> (String, String) { /// global (home-rooted) location. Order matches discovery precedence: project /// dirs first, then global dirs. pub fn all_skill_dirs(working_dir: Option<&Path>) -> Vec<(PathBuf, bool)> { - let mut dirs: Vec<(PathBuf, bool)> = Vec::new(); + all_skill_dirs_with_config(working_dir, Config::global()) + .into_iter() + .map(|dir| (dir.path, dir.is_global)) + .collect() +} + +struct SkillDirectory { + path: PathBuf, + is_global: bool, + writable: bool, + preserve_path: bool, +} + +fn all_skill_dirs_with_config(working_dir: Option<&Path>, config: &Config) -> Vec { + let mut dirs = Vec::new(); + let plugin_dirs = enabled_plugin_skill_dirs_with_config(working_dir, config); if let Some(wd) = working_dir { - dirs.push((wd.join(".agents").join("skills"), false)); - dirs.push((wd.join(".goose").join("skills"), false)); - dirs.push((wd.join(".claude").join("skills"), false)); + for path in [ + wd.join(".agents").join("skills"), + wd.join(".goose").join("skills"), + wd.join(".claude").join("skills"), + ] { + dirs.push(SkillDirectory { + path, + is_global: false, + writable: true, + preserve_path: false, + }); + } } + dirs.extend( + plugin_dirs + .iter() + .filter(|(_, scope)| *scope == PluginScope::Project) + .map(|(path, _)| SkillDirectory { + path: path.clone(), + is_global: false, + writable: false, + preserve_path: true, + }), + ); let home = dirs::home_dir(); if let Some(h) = home.as_ref() { - dirs.push((h.join(".agents").join("skills"), true)); + dirs.push(SkillDirectory { + path: h.join(".agents").join("skills"), + is_global: true, + writable: true, + preserve_path: false, + }); } - dirs.push((Paths::config_dir().join("skills"), true)); + dirs.push(SkillDirectory { + path: Paths::config_dir().join("skills"), + is_global: true, + writable: true, + preserve_path: false, + }); if let Some(h) = home.as_ref() { - dirs.push((h.join(".claude").join("skills"), true)); - dirs.push((h.join(".config").join("agents").join("skills"), true)); + for path in [ + h.join(".claude").join("skills"), + h.join(".config").join("agents").join("skills"), + ] { + dirs.push(SkillDirectory { + path, + is_global: true, + writable: true, + preserve_path: false, + }); + } } dirs.extend( - installed_plugin_skill_dirs() + plugin_dirs .into_iter() - .map(|dir| (dir, true)), + .filter(|(_, scope)| *scope == PluginScope::User) + .map(|(path, _)| SkillDirectory { + path, + is_global: true, + writable: true, + preserve_path: true, + }), ); dirs } -fn parse_skill_content(content: &str, path: &Path, global: bool) -> Option { +fn parse_skill_content( + content: &str, + path: &Path, + global: bool, + writable: bool, +) -> Option { let (metadata, body): (SkillFrontmatter, String) = match parse_frontmatter(content) { Ok(Some(parsed)) => parsed, Ok(None) => return None, @@ -376,7 +478,7 @@ fn parse_skill_content(content: &str, path: &Path, global: bool) -> Option( } } -fn scan_skills_from_dir(dir: &Path, global: bool, seen: &mut HashSet) -> Vec { +fn scan_skills_from_dir( + dir: &Path, + global: bool, + writable: bool, + preserve_path: bool, + seen: &mut HashSet, +) -> Vec { let mut skill_files = Vec::new(); let mut visited_dirs = HashSet::new(); @@ -444,8 +552,13 @@ fn scan_skills_from_dir(dir: &Path, global: bool, seen: &mut HashSet) -> let Some(skill_dir) = skill_file.parent() else { continue; }; - let Ok(registered_skill_dir) = skill_dir.canonicalize() else { - continue; + let registered_skill_dir = if preserve_path { + skill_dir.to_path_buf() + } else { + let Ok(canonical_dir) = skill_dir.canonicalize() else { + continue; + }; + canonical_dir }; let content = match std::fs::read_to_string(&skill_file) { Ok(c) => c, @@ -455,7 +568,9 @@ fn scan_skills_from_dir(dir: &Path, global: bool, seen: &mut HashSet) -> } }; - if let Some(mut source) = parse_skill_content(&content, ®istered_skill_dir, global) { + if let Some(mut source) = + parse_skill_content(&content, ®istered_skill_dir, global, writable) + { if !seen.contains(&source.name) { let mut files = Vec::new(); let mut visited_support_dirs = HashSet::new(); @@ -490,17 +605,27 @@ fn scan_skills_from_dir(dir: &Path, global: bool, seen: &mut HashSet) -> /// Each returned entry has `global` set according to the directory it was /// found in (or `true` for built-ins). pub fn discover_skills(working_dir: Option<&Path>) -> Vec { + discover_skills_with_config(working_dir, Config::global()) +} + +fn discover_skills_with_config(working_dir: Option<&Path>, config: &Config) -> Vec { let mut sources: Vec = Vec::new(); let mut seen = HashSet::new(); - for (dir, is_global) in all_skill_dirs(working_dir) { - for source in scan_skills_from_dir(&dir, is_global, &mut seen) { + for dir in all_skill_dirs_with_config(working_dir, config) { + for source in scan_skills_from_dir( + &dir.path, + dir.is_global, + dir.writable, + dir.preserve_path, + &mut seen, + ) { sources.push(source); } } for content in builtin::get_all() { - if let Some(source) = parse_skill_content(content, &PathBuf::new(), true) { + if let Some(source) = parse_skill_content(content, &PathBuf::new(), true, true) { if !seen.contains(&source.name) { seen.insert(source.name.clone()); let path = format!("builtin://skills/{}", source.name); @@ -533,6 +658,44 @@ mod tests { use super::*; use serde_json::json; + fn write_test_skill(dir: &Path, name: &str, body: &str) { + std::fs::create_dir_all(dir).unwrap(); + std::fs::write( + dir.join("SKILL.md"), + format!("---\nname: {name}\ndescription: Test skill\n---\n{body}"), + ) + .unwrap(); + } + + fn assert_path_rejected_by_source_crud(path: &str, name: &str, skill_dir: &Path) { + let update_err = crate::sources::update_source_with_roots( + SourceType::Skill, + path, + name, + "updated", + "updated body", + crate::sources::UpdateSourceOptions { + properties: Some(HashMap::new()), + additional_roots: &[], + }, + ) + .unwrap_err(); + assert!(format!("{update_err:?}").contains("not found")); + + let delete_err = crate::sources::delete_source(SourceType::Skill, path).unwrap_err(); + assert!(format!("{delete_err:?}").contains("not found")); + + let export_err = crate::sources::export_source(SourceType::Skill, path).unwrap_err(); + assert!(format!("{export_err:?}").contains("not found")); + assert!(skill_dir.join("SKILL.md").is_file()); + } + + fn assert_read_only_and_rejected_by_source_crud(skill: &SourceEntry, skill_dir: &Path) { + assert!(!skill.global); + assert!(!skill.writable); + assert_path_rejected_by_source_crud(&skill.path, &skill.name, skill_dir); + } + fn skill_with_content(content: &str) -> SourceEntry { SourceEntry { source_type: SourceType::Skill, @@ -619,4 +782,439 @@ mod tests { assert_eq!(rendered, skill.content); } + + #[test] + fn project_plugin_skill_precedes_global_skill_with_same_name() { + let project = tempfile::tempdir().unwrap(); + let path_root = tempfile::tempdir().unwrap(); + let plugin_root = project.path().join(".agents/plugins/project-plugin"); + write_test_skill( + &plugin_root.join("skills/collision"), + "collision", + "project plugin body", + ); + write_test_skill( + &path_root.path().join("config/skills/collision"), + "collision", + "global body", + ); + + let config = Config::new(path_root.path().join("test-config.yaml"), "skills-test").unwrap(); + config + .set_param( + "plugins", + HashMap::from([( + plugin_root.to_string_lossy().into_owned(), + HashMap::from([("enabled", true)]), + )]), + ) + .unwrap(); + let _guard = env_lock::lock_env([ + ("GOOSE_PATH_ROOT", path_root.path().to_str()), + ("PLUGINS", None), + ]); + + let skill = discover_skills_with_config(Some(project.path()), &config) + .into_iter() + .find(|skill| skill.name == "collision") + .unwrap(); + + assert_eq!(skill.content.trim(), "project plugin body"); + assert!(!skill.global); + assert!(!skill.writable); + assert!(Path::new(&skill.path).starts_with(&plugin_root)); + } + + #[test] + fn user_plugin_skill_remains_writable_when_project_root_is_path_root() { + let path_root = tempfile::tempdir().unwrap(); + let plugin_root = path_root.path().join(".agents/plugins/user-plugin"); + write_test_skill( + &plugin_root.join("skills/user-owned"), + "user-owned", + "user body", + ); + + let config = Config::new(path_root.path().join("test-config.yaml"), "skills-test").unwrap(); + config + .set_param( + "plugins", + HashMap::from([( + plugin_root.to_string_lossy().into_owned(), + HashMap::from([("enabled", true)]), + )]), + ) + .unwrap(); + let _guard = env_lock::lock_env([ + ("GOOSE_PATH_ROOT", path_root.path().to_str()), + ("PLUGINS", None), + ]); + + let skill = discover_skills_with_config(Some(path_root.path()), &config) + .into_iter() + .find(|skill| skill.name == "user-owned") + .unwrap(); + + assert!(skill.global); + assert!(skill.writable); + } + + #[test] + fn exclusive_project_plugin_manifest_omits_default_skill_root() { + let project = tempfile::tempdir().unwrap(); + let path_root = tempfile::tempdir().unwrap(); + let plugin_root = project.path().join(".agents/plugins/project-plugin"); + write_test_skill( + &plugin_root.join("skills/excluded"), + "excluded", + "excluded body", + ); + write_test_skill( + &plugin_root.join("custom-skills/included"), + "included", + "included body", + ); + std::fs::write( + plugin_root.join("plugin.json"), + r#"{"name":"project-plugin","skills":{"exclusive":true,"paths":["./custom-skills"]}}"#, + ) + .unwrap(); + + let config = Config::new(path_root.path().join("test-config.yaml"), "skills-test").unwrap(); + config + .set_param( + "plugins", + HashMap::from([( + plugin_root.to_string_lossy().into_owned(), + HashMap::from([("enabled", true)]), + )]), + ) + .unwrap(); + let _guard = env_lock::lock_env([ + ("GOOSE_PATH_ROOT", path_root.path().to_str()), + ("PLUGINS", None), + ]); + + let skills = discover_skills_with_config(Some(project.path()), &config); + + assert!(skills.iter().any(|skill| skill.name == "included")); + assert!(!skills.iter().any(|skill| skill.name == "excluded")); + } + + #[test] + fn project_plugin_skill_is_rejected_by_source_crud_before_discovery() { + let project = tempfile::tempdir().unwrap(); + let path_root = tempfile::tempdir().unwrap(); + let plugin_root = project.path().join(".agents/plugins/project-plugin"); + let skill_dir = plugin_root.join(".agents/skills/plugin-owned"); + write_test_skill(&skill_dir, "plugin-owned", "plugin body"); + std::fs::write( + plugin_root.join("plugin.json"), + r#"{"name":"project-plugin","skills":{"paths":["./.agents/skills"]}}"#, + ) + .unwrap(); + let _guard = env_lock::lock_env([ + ("GOOSE_PATH_ROOT", path_root.path().to_str()), + ("PLUGINS", None), + ]); + + assert_path_rejected_by_source_crud( + skill_dir.to_str().unwrap(), + "plugin-owned", + &skill_dir, + ); + } + + #[cfg(unix)] + #[test] + fn configured_disabled_project_plugin_canonical_path_is_rejected() { + let project = tempfile::tempdir().unwrap(); + let path_root = tempfile::tempdir().unwrap(); + let external_root = tempfile::tempdir().unwrap(); + let plugin_link = project.path().join(".agents/plugins/project-plugin"); + let skill_dir = external_root.path().join(".agents/skills/plugin-owned"); + write_test_skill(&skill_dir, "plugin-owned", "plugin body"); + std::fs::write( + external_root.path().join("plugin.json"), + r#"{"name":"project-plugin","skills":{"paths":["./.agents/skills"]}}"#, + ) + .unwrap(); + std::fs::create_dir_all(plugin_link.parent().unwrap()).unwrap(); + std::os::unix::fs::symlink(external_root.path(), &plugin_link).unwrap(); + let config = Config::new(path_root.path().join("test-config.yaml"), "skills-test").unwrap(); + config + .set_param( + "plugins", + HashMap::from([( + plugin_link.to_string_lossy().into_owned(), + HashMap::from([("enabled", false)]), + )]), + ) + .unwrap(); + let _guard = env_lock::lock_env([ + ("GOOSE_PATH_ROOT", path_root.path().to_str()), + ("PLUGINS", None), + ]); + + let err = resolve_discoverable_skill_dir_with_config(skill_dir.to_str().unwrap(), &config) + .unwrap_err(); + + assert!(format!("{err:?}").contains("not found")); + assert!(skill_dir.join("SKILL.md").is_file()); + } + + #[cfg(unix)] + #[test] + fn settings_disabled_project_plugin_canonical_path_is_rejected() { + let project = tempfile::tempdir().unwrap(); + let path_root = tempfile::tempdir().unwrap(); + let external_root = tempfile::tempdir().unwrap(); + let plugin_link = project.path().join(".agents/plugins/project-plugin"); + let skill_dir = external_root.path().join(".agents/skills/plugin-owned"); + write_test_skill(&skill_dir, "plugin-owned", "plugin body"); + std::fs::write( + external_root.path().join("plugin.json"), + r#"{"name":"project-plugin","skills":{"paths":["./.agents/skills"]}}"#, + ) + .unwrap(); + std::fs::create_dir_all(plugin_link.parent().unwrap()).unwrap(); + std::os::unix::fs::symlink(external_root.path(), &plugin_link).unwrap(); + let settings_dir = project.path().join(".config/goose"); + std::fs::create_dir_all(&settings_dir).unwrap(); + std::fs::write( + settings_dir.join("settings.json"), + r#"{"disabledPlugins":["project-plugin"]}"#, + ) + .unwrap(); + let config = Config::new(path_root.path().join("test-config.yaml"), "skills-test").unwrap(); + let _guard = env_lock::lock_env([ + ("GOOSE_PATH_ROOT", path_root.path().to_str()), + ("PLUGINS", None), + ]); + + let plugins = crate::plugins::discovery::discover_enabled_plugins_with_config( + Some(project.path()), + &config, + ); + assert!(plugins.iter().all(|plugin| plugin.name != "project-plugin")); + + let err = resolve_discoverable_skill_dir_with_config(skill_dir.to_str().unwrap(), &config) + .unwrap_err(); + assert!(format!("{err:?}").contains("not found")); + assert!(skill_dir.join("SKILL.md").is_file()); + } + + #[test] + fn nested_project_plugin_skill_is_listed_read_only_and_rejected_by_source_crud() { + let project = tempfile::tempdir().unwrap(); + let path_root = tempfile::tempdir().unwrap(); + let plugin_root = project.path().join(".agents/plugins/project-plugin"); + let skill_dir = plugin_root.join(".agents/skills/plugin-owned"); + write_test_skill(&skill_dir, "plugin-owned", "plugin body"); + std::fs::write( + plugin_root.join("plugin.json"), + r#"{"name":"project-plugin","skills":{"paths":["./.agents/skills"]}}"#, + ) + .unwrap(); + + let config = Config::new(path_root.path().join("test-config.yaml"), "skills-test").unwrap(); + config + .set_param( + "plugins", + HashMap::from([( + plugin_root.to_string_lossy().into_owned(), + HashMap::from([("enabled", true)]), + )]), + ) + .unwrap(); + let _guard = env_lock::lock_env([ + ("GOOSE_PATH_ROOT", path_root.path().to_str()), + ("PLUGINS", None), + ]); + + let skill = discover_skills_with_config(Some(project.path()), &config) + .into_iter() + .find(|skill| skill.name == "plugin-owned") + .unwrap(); + assert_read_only_and_rejected_by_source_crud(&skill, &skill_dir); + } + + #[cfg(unix)] + #[test] + fn symlinked_project_plugin_skill_is_rejected_by_source_crud() { + let project = tempfile::tempdir().unwrap(); + let path_root = tempfile::tempdir().unwrap(); + let external_root = tempfile::tempdir().unwrap(); + let plugin_link = project.path().join(".agents/plugins/project-plugin"); + let skill_dir = external_root.path().join(".agents/skills/plugin-owned"); + write_test_skill(&skill_dir, "plugin-owned", "plugin body"); + std::fs::write( + external_root.path().join("plugin.json"), + r#"{"name":"project-plugin","skills":{"paths":["./.agents/skills"]}}"#, + ) + .unwrap(); + std::fs::create_dir_all(plugin_link.parent().unwrap()).unwrap(); + std::os::unix::fs::symlink(external_root.path(), &plugin_link).unwrap(); + + let config = Config::new(path_root.path().join("test-config.yaml"), "skills-test").unwrap(); + config + .set_param( + "plugins", + HashMap::from([( + plugin_link.to_string_lossy().into_owned(), + HashMap::from([("enabled", true)]), + )]), + ) + .unwrap(); + let _guard = env_lock::lock_env([ + ("GOOSE_PATH_ROOT", path_root.path().to_str()), + ("PLUGINS", None), + ]); + + let skill = discover_skills_with_config(Some(project.path()), &config) + .into_iter() + .find(|skill| skill.name == "plugin-owned") + .unwrap(); + + assert!(Path::new(&skill.path).starts_with(&plugin_link)); + assert_read_only_and_rejected_by_source_crud(&skill, &skill_dir); + } + + #[cfg(unix)] + #[test] + fn symlinked_user_plugin_skill_remains_writable_for_source_crud() { + let path_root = tempfile::tempdir().unwrap(); + let external_parent = tempfile::tempdir().unwrap(); + let external_root = external_parent.path().join(".agents/plugins/user-plugin"); + let skill_dir = external_root.join(".agents/skills/user-owned"); + write_test_skill(&skill_dir, "user-owned", "user body"); + std::fs::write( + external_root.join("plugin.json"), + r#"{"name":"user-plugin","skills":{"paths":["./.agents/skills"]}}"#, + ) + .unwrap(); + + let project = tempfile::tempdir().unwrap(); + let project_path_root = tempfile::tempdir().unwrap(); + let project_plugin_link = project.path().join(".agents/plugins/user-plugin"); + std::fs::create_dir_all(project_plugin_link.parent().unwrap()).unwrap(); + std::os::unix::fs::symlink(&external_root, &project_plugin_link).unwrap(); + let project_config = Config::new( + project_path_root.path().join("test-config.yaml"), + "skills-test", + ) + .unwrap(); + project_config + .set_param( + "plugins", + HashMap::from([( + project_plugin_link.to_string_lossy().into_owned(), + HashMap::from([("enabled", true)]), + )]), + ) + .unwrap(); + { + let _guard = env_lock::lock_env([ + ("GOOSE_PATH_ROOT", project_path_root.path().to_str()), + ("PLUGINS", None), + ]); + let project_skill = discover_skills_with_config(Some(project.path()), &project_config) + .into_iter() + .find(|skill| skill.name == "user-owned") + .unwrap(); + assert!(!project_skill.writable); + } + + let plugin_link = path_root.path().join(".agents/plugins/user-plugin"); + std::fs::create_dir_all(plugin_link.parent().unwrap()).unwrap(); + std::os::unix::fs::symlink(&external_root, &plugin_link).unwrap(); + + let config = Config::new(path_root.path().join("test-config.yaml"), "skills-test").unwrap(); + config + .set_param( + "plugins", + HashMap::from([( + plugin_link.to_string_lossy().into_owned(), + HashMap::from([("enabled", true)]), + )]), + ) + .unwrap(); + let _guard = env_lock::lock_env([ + ("GOOSE_PATH_ROOT", path_root.path().to_str()), + ("PLUGINS", None), + ]); + + let skill = discover_skills_with_config(None, &config) + .into_iter() + .find(|skill| skill.name == "user-owned") + .unwrap(); + assert!(skill.global); + assert!(skill.writable); + assert!(Path::new(&skill.path).starts_with(&plugin_link)); + assert!(resolve_discoverable_skill_dir_with_config(&skill.path, &project_config).is_ok()); + + let updated = crate::sources::update_source_with_roots( + SourceType::Skill, + &skill.path, + "user-owned", + "updated", + "updated body", + crate::sources::UpdateSourceOptions { + properties: Some(HashMap::new()), + additional_roots: &[], + }, + ) + .unwrap(); + assert_eq!(updated.content, "updated body"); + + let (exported, _) = crate::sources::export_source(SourceType::Skill, &skill.path).unwrap(); + assert!(exported.contains("updated body")); + + crate::sources::delete_source(SourceType::Skill, &skill.path).unwrap(); + assert!(!skill_dir.exists()); + } + + #[test] + fn ordinary_project_skill_under_plugin_manifest_remains_writable() { + let project = tempfile::tempdir().unwrap(); + let path_root = tempfile::tempdir().unwrap(); + let skill_dir = project.path().join(".agents/skills/project-owned"); + write_test_skill(&skill_dir, "project-owned", "project body"); + std::fs::write( + project.path().join("plugin.json"), + r#"{"name":"ordinary-project","skills":{"paths":["./.agents/skills"]}}"#, + ) + .unwrap(); + let config = Config::new(path_root.path().join("test-config.yaml"), "skills-test").unwrap(); + let _guard = env_lock::lock_env([ + ("GOOSE_PATH_ROOT", path_root.path().to_str()), + ("PLUGINS", None), + ]); + + let skill = discover_skills_with_config(Some(project.path()), &config) + .into_iter() + .find(|skill| skill.name == "project-owned") + .unwrap(); + assert!(skill.writable); + + let updated = crate::sources::update_source_with_roots( + SourceType::Skill, + &skill.path, + "project-owned", + "updated", + "updated body", + crate::sources::UpdateSourceOptions { + properties: Some(HashMap::new()), + additional_roots: &[], + }, + ) + .unwrap(); + assert_eq!(updated.content, "updated body"); + + let (exported, _) = crate::sources::export_source(SourceType::Skill, &skill.path).unwrap(); + assert!(exported.contains("updated body")); + + crate::sources::delete_source(SourceType::Skill, &skill.path).unwrap(); + assert!(!skill_dir.exists()); + } }