diff --git a/crates/goose-cli/src/session/completion.rs b/crates/goose-cli/src/session/completion.rs index 9b766f36c916..f9c3d2bf6795 100644 --- a/crates/goose-cli/src/session/completion.rs +++ b/crates/goose-cli/src/session/completion.rs @@ -123,7 +123,7 @@ impl GooseCompleter { /// Complete skill names for the /skills command fn complete_skill_names(&self, line: &str) -> Result<(usize, Vec)> { - use goose::agents::platform_extensions::skills::list_installed_skills; + use goose::skills::list_installed_skills; let cwd = std::env::current_dir().unwrap_or_default(); let skills = list_installed_skills(Some(&cwd)); diff --git a/crates/goose-cli/src/session/mod.rs b/crates/goose-cli/src/session/mod.rs index 493c54481343..4deb36ae3d14 100644 --- a/crates/goose-cli/src/session/mod.rs +++ b/crates/goose-cli/src/session/mod.rs @@ -907,7 +907,7 @@ impl CliSession { async fn handle_list_skills(&mut self) -> Result<()> { use comfy_table::{presets, Cell, ContentArrangement, Table}; - use goose::agents::platform_extensions::skills::list_installed_skills; + use goose::skills::list_installed_skills; let cwd = std::env::current_dir().unwrap_or_default(); let skills = list_installed_skills(Some(&cwd)); diff --git a/crates/goose-sdk/src/custom_requests.rs b/crates/goose-sdk/src/custom_requests.rs index ec68de957b10..822cd3e3444a 100644 --- a/crates/goose-sdk/src/custom_requests.rs +++ b/crates/goose-sdk/src/custom_requests.rs @@ -286,15 +286,33 @@ pub struct ProviderConfigKey { } /// The type of source entity. -#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +#[derive( + Debug, Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize, JsonSchema, +)] #[serde(rename_all = "camelCase")] pub enum SourceType { #[default] Skill, -} - -/// A source — a user-editable entity backed by an on-disk directory. Sources -/// may be either `global` (shared across all projects) or project-specific. + BuiltinSkill, + Recipe, + Subrecipe, + Agent, +} + +impl std::fmt::Display for SourceType { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + SourceType::Skill => write!(f, "skill"), + SourceType::BuiltinSkill => write!(f, "builtin skill"), + SourceType::Recipe => write!(f, "recipe"), + SourceType::Subrecipe => write!(f, "subrecipe"), + SourceType::Agent => write!(f, "agent"), + } + } +} + +/// A source discovered by Goose and backed by an on-disk path. Sources may be +/// either `global` (shared across all projects) or project-specific. #[derive(Debug, Default, Clone, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "camelCase")] pub struct SourceEntry { @@ -303,14 +321,31 @@ pub struct SourceEntry { pub name: String, pub description: String, pub content: String, - /// Absolute path to the source's directory on disk. + /// Absolute path to the source on disk. A directory for skills, a file for + /// recipes and agents. pub directory: String, /// True when the source lives in the user's global sources directory; false /// when it lives inside a specific project. pub global: bool, + /// Paths (absolute) of additional files that live alongside the source. + /// Only skills currently populate this; empty for other source types. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub supporting_files: Vec, +} + +impl SourceEntry { + /// Render this source as a markdown block suitable for injecting into an + /// LLM context. Used by the skills and summon runtimes when loading a + /// source into the current conversation. + pub fn to_load_text(&self) -> String { + format!( + "## {} ({})\n\n{}\n\n### Content\n\n{}", + self.name, self.source_type, self.description, self.content + ) + } } -/// Create a new source (global or project-scoped). +/// Create a new source in an explicit target scope (global or project-scoped). #[derive(Debug, Default, Clone, Serialize, Deserialize, JsonSchema, JsonRpcRequest)] #[request(method = "_goose/sources/create", response = CreateSourceResponse)] #[serde(rename_all = "camelCase")] @@ -332,8 +367,11 @@ pub struct CreateSourceResponse { pub source: SourceEntry, } -/// List sources. If `type` is omitted, sources of all known types are returned. -/// Both global and project-scoped sources are included when `project_dir` is set. +/// List discovered sources. +/// +/// Today this endpoint only returns skills. If `type` is omitted, it defaults +/// to listing skill sources. Both global and project-scoped skills are included +/// when `project_dir` is set. #[derive(Debug, Default, Clone, Serialize, Deserialize, JsonSchema, JsonRpcRequest)] #[request(method = "_goose/sources/list", response = ListSourcesResponse)] #[serde(rename_all = "camelCase")] @@ -350,19 +388,17 @@ pub struct ListSourcesResponse { pub sources: Vec, } -/// Update an existing source's description and content. +/// Update an existing source's name, description, and content by absolute path. #[derive(Debug, Default, Clone, Serialize, Deserialize, JsonSchema, JsonRpcRequest)] #[request(method = "_goose/sources/update", response = UpdateSourceResponse)] #[serde(rename_all = "camelCase")] pub struct UpdateSourceRequest { #[serde(rename = "type")] pub source_type: SourceType, + pub path: String, pub name: String, pub description: String, pub content: String, - pub global: bool, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub project_dir: Option, } #[derive(Debug, Default, Clone, Serialize, Deserialize, JsonSchema, JsonRpcResponse)] @@ -371,30 +407,24 @@ pub struct UpdateSourceResponse { pub source: SourceEntry, } -/// Delete a source and its on-disk directory. +/// Delete a source and its on-disk directory by absolute path. #[derive(Debug, Default, Clone, Serialize, Deserialize, JsonSchema, JsonRpcRequest)] #[request(method = "_goose/sources/delete", response = EmptyResponse)] #[serde(rename_all = "camelCase")] pub struct DeleteSourceRequest { #[serde(rename = "type")] pub source_type: SourceType, - pub name: String, - pub global: bool, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub project_dir: Option, + pub path: String, } -/// Export a source as a portable JSON payload. +/// Export a source at an absolute path as a portable JSON payload. #[derive(Debug, Default, Clone, Serialize, Deserialize, JsonSchema, JsonRpcRequest)] #[request(method = "_goose/sources/export", response = ExportSourceResponse)] #[serde(rename_all = "camelCase")] pub struct ExportSourceRequest { #[serde(rename = "type")] pub source_type: SourceType, - pub name: String, - pub global: bool, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub project_dir: Option, + pub path: String, } #[derive(Debug, Default, Clone, Serialize, Deserialize, JsonSchema, JsonRpcResponse)] @@ -405,8 +435,8 @@ pub struct ExportSourceResponse { } /// Import a source from a JSON export payload produced by `_goose/sources/export`. -/// The imported source is written under the given scope; on name collisions a -/// `-imported` suffix is appended. +/// The imported source is written into the explicit target scope; on name +/// collisions a `-imported` suffix is appended. #[derive(Debug, Default, Clone, Serialize, Deserialize, JsonSchema, JsonRpcRequest)] #[request(method = "_goose/sources/import", response = ImportSourcesResponse)] #[serde(rename_all = "camelCase")] diff --git a/crates/goose-server/src/routes/config_management.rs b/crates/goose-server/src/routes/config_management.rs index c8ed2d25b613..600a74cbeeca 100644 --- a/crates/goose-server/src/routes/config_management.rs +++ b/crates/goose-server/src/routes/config_management.rs @@ -11,6 +11,7 @@ use goose::config::declarative_providers::LoadedProvider; use goose::config::paths::Paths; use goose::config::ExtensionEntry; use goose::config::{Config, ConfigError}; +use goose::custom_requests::SourceType; use goose::model::ModelConfig; use goose::providers::base::{ProviderMetadata, ProviderType}; use goose::providers::canonical::maybe_get_canonical_model; @@ -427,9 +428,7 @@ pub async fn get_slash_commands( } let working_dir = query.working_dir.map(std::path::PathBuf::from); - for source in - goose::agents::platform_extensions::skills::list_installed_skills(working_dir.as_deref()) - { + for source in goose::skills::list_installed_skills(working_dir.as_deref()) { commands.push(SlashCommand { command: source.name, help: source.description, @@ -443,10 +442,9 @@ pub async fn get_slash_commands( for source in goose::agents::platform_extensions::summon::discover_filesystem_sources(discover_dir) { - use goose::agents::platform_extensions::SourceKind; if matches!( - source.kind, - SourceKind::Agent | SourceKind::Recipe | SourceKind::Subrecipe + source.source_type, + SourceType::Agent | SourceType::Recipe | SourceType::Subrecipe ) && !source.content.is_empty() { commands.push(SlashCommand { diff --git a/crates/goose/acp-schema.json b/crates/goose/acp-schema.json index 15906dfee915..a7ed35a715f3 100644 --- a/crates/goose/acp-schema.json +++ b/crates/goose/acp-schema.json @@ -804,14 +804,18 @@ "content", "global" ], - "description": "Create a new source (global or project-scoped).", + "description": "Create a new source in an explicit target scope (global or project-scoped).", "x-side": "agent", "x-method": "_goose/sources/create" }, "SourceType": { "type": "string", "enum": [ - "skill" + "skill", + "builtinSkill", + "recipe", + "subrecipe", + "agent" ], "description": "The type of source entity." }, @@ -845,11 +849,18 @@ }, "directory": { "type": "string", - "description": "Absolute path to the source's directory on disk." + "description": "Absolute path to the source on disk. A directory for skills, a file for\nrecipes and agents." }, "global": { "type": "boolean", "description": "True when the source lives in the user's global sources directory; false\nwhen it lives inside a specific project." + }, + "supportingFiles": { + "type": "array", + "items": { + "type": "string" + }, + "description": "Paths (absolute) of additional files that live alongside the source.\nOnly skills currently populate this; empty for other source types." } }, "required": [ @@ -860,7 +871,7 @@ "directory", "global" ], - "description": "A source — a user-editable entity backed by an on-disk directory. Sources\nmay be either `global` (shared across all projects) or project-specific." + "description": "A source discovered by Goose and backed by an on-disk path. Sources may be\neither `global` (shared across all projects) or project-specific." }, "ListSourcesRequest": { "type": "object", @@ -882,7 +893,7 @@ ] } }, - "description": "List sources. If `type` is omitted, sources of all known types are returned.\nBoth global and project-scoped sources are included when `project_dir` is set.", + "description": "List discovered sources.\n\nToday this endpoint only returns skills. If `type` is omitted, it defaults\nto listing skill sources. Both global and project-scoped skills are included\nwhen `project_dir` is set.", "x-side": "agent", "x-method": "_goose/sources/list" }, @@ -908,6 +919,9 @@ "type": { "$ref": "#/$defs/SourceType" }, + "path": { + "type": "string" + }, "name": { "type": "string" }, @@ -916,25 +930,16 @@ }, "content": { "type": "string" - }, - "global": { - "type": "boolean" - }, - "projectDir": { - "type": [ - "string", - "null" - ] } }, "required": [ "type", + "path", "name", "description", - "content", - "global" + "content" ], - "description": "Update an existing source's description and content.", + "description": "Update an existing source's name, description, and content by absolute path.", "x-side": "agent", "x-method": "_goose/sources/update" }, @@ -957,25 +962,15 @@ "type": { "$ref": "#/$defs/SourceType" }, - "name": { + "path": { "type": "string" - }, - "global": { - "type": "boolean" - }, - "projectDir": { - "type": [ - "string", - "null" - ] } }, "required": [ "type", - "name", - "global" + "path" ], - "description": "Delete a source and its on-disk directory.", + "description": "Delete a source and its on-disk directory by absolute path.", "x-side": "agent", "x-method": "_goose/sources/delete" }, @@ -985,25 +980,15 @@ "type": { "$ref": "#/$defs/SourceType" }, - "name": { + "path": { "type": "string" - }, - "global": { - "type": "boolean" - }, - "projectDir": { - "type": [ - "string", - "null" - ] } }, "required": [ "type", - "name", - "global" + "path" ], - "description": "Export a source as a portable JSON payload.", + "description": "Export a source at an absolute path as a portable JSON payload.", "x-side": "agent", "x-method": "_goose/sources/export" }, @@ -1044,7 +1029,7 @@ "data", "global" ], - "description": "Import a source from a JSON export payload produced by `_goose/sources/export`.\nThe imported source is written under the given scope; on name collisions a\n`-imported` suffix is appended.", + "description": "Import a source from a JSON export payload produced by `_goose/sources/export`.\nThe imported source is written into the explicit target scope; on name\ncollisions a `-imported` suffix is appended.", "x-side": "agent", "x-method": "_goose/sources/import" }, diff --git a/crates/goose/src/acp/server.rs b/crates/goose/src/acp/server.rs index 57149cb4fedf..1011e21ad2ac 100644 --- a/crates/goose/src/acp/server.rs +++ b/crates/goose/src/acp/server.rs @@ -3332,11 +3332,10 @@ impl GooseAcpAgent { ) -> Result { let source = crate::sources::update_source( req.source_type, + &req.path, &req.name, &req.description, &req.content, - req.global, - req.project_dir.as_deref(), )?; Ok(UpdateSourceResponse { source }) } @@ -3346,12 +3345,7 @@ impl GooseAcpAgent { &self, req: DeleteSourceRequest, ) -> Result { - crate::sources::delete_source( - req.source_type, - &req.name, - req.global, - req.project_dir.as_deref(), - )?; + crate::sources::delete_source(req.source_type, &req.path)?; Ok(EmptyResponse {}) } @@ -3360,12 +3354,7 @@ impl GooseAcpAgent { &self, req: ExportSourceRequest, ) -> Result { - let (json, filename) = crate::sources::export_source( - req.source_type, - &req.name, - req.global, - req.project_dir.as_deref(), - )?; + let (json, filename) = crate::sources::export_source(req.source_type, &req.path)?; Ok(ExportSourceResponse { json, filename }) } diff --git a/crates/goose/src/agents/execute_commands.rs b/crates/goose/src/agents/execute_commands.rs index b49713854e2d..3a488c70a1fb 100644 --- a/crates/goose/src/agents/execute_commands.rs +++ b/crates/goose/src/agents/execute_commands.rs @@ -140,8 +140,8 @@ impl Agent { } async fn handle_skills_command(&self, session_id: &str) -> Result> { - use super::platform_extensions::skills::list_installed_skills; - use super::platform_extensions::SourceKind; + use crate::skills::list_installed_skills; + use goose_sdk::custom_requests::SourceType; let working_dir = self .config @@ -153,7 +153,7 @@ impl Agent { let sources = list_installed_skills(working_dir.as_deref()); let skills: Vec<_> = sources .iter() - .filter(|s| matches!(s.kind, SourceKind::Skill | SourceKind::BuiltinSkill)) + .filter(|s| matches!(s.source_type, SourceType::Skill | SourceType::BuiltinSkill)) .collect(); let mut output = String::new(); @@ -165,7 +165,7 @@ impl Agent { } else { output.push_str(&format!("**Installed skills ({}):**\n\n", skills.len())); for skill in &skills { - let kind_label = if skill.kind == SourceKind::BuiltinSkill { + let kind_label = if skill.source_type == SourceType::BuiltinSkill { " *(builtin)*" } else { "" diff --git a/crates/goose/src/agents/mod.rs b/crates/goose/src/agents/mod.rs index 1b41a743182b..a221907e84ba 100644 --- a/crates/goose/src/agents/mod.rs +++ b/crates/goose/src/agents/mod.rs @@ -1,5 +1,4 @@ mod agent; -pub(crate) mod builtin_skills; pub mod container; pub mod execute_commands; pub mod extension; diff --git a/crates/goose/src/agents/platform_extensions/mod.rs b/crates/goose/src/agents/platform_extensions/mod.rs index 2166d3f3f715..c68d19ee5f89 100644 --- a/crates/goose/src/agents/platform_extensions/mod.rs +++ b/crates/goose/src/agents/platform_extensions/mod.rs @@ -6,74 +6,16 @@ pub mod code_execution; pub mod developer; pub mod ext_manager; pub mod orchestrator; -pub mod skills; pub mod summarize; pub mod summon; pub mod todo; pub mod tom; use std::collections::HashMap; -use std::path::PathBuf; use crate::agents::mcp_client::McpClientTrait; use crate::session::Session; use once_cell::sync::Lazy; -use serde::Deserialize; - -#[derive(Debug, Clone)] -pub struct Source { - pub name: String, - pub kind: SourceKind, - pub description: String, - pub path: PathBuf, - pub content: String, - pub supporting_files: Vec, -} - -#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] -pub enum SourceKind { - Subrecipe, - Recipe, - Skill, - Agent, - BuiltinSkill, -} - -impl std::fmt::Display for SourceKind { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - SourceKind::Subrecipe => write!(f, "subrecipe"), - SourceKind::Recipe => write!(f, "recipe"), - SourceKind::Skill => write!(f, "skill"), - SourceKind::Agent => write!(f, "agent"), - SourceKind::BuiltinSkill => write!(f, "builtin skill"), - } - } -} - -impl Source { - pub fn to_load_text(&self) -> String { - format!( - "## {} ({})\n\n{}\n\n### Content\n\n{}", - self.name, self.kind, self.description, self.content - ) - } -} - -pub fn parse_frontmatter Deserialize<'de>>( - content: &str, -) -> Result, serde_yaml::Error> { - let parts: Vec<&str> = content.split("---").collect(); - if parts.len() < 3 { - return Ok(None); - } - - let yaml_content = parts[1].trim(); - let metadata: T = serde_yaml::from_str(yaml_content)?; - - let body = parts[2..].join("---").trim().to_string(); - Ok(Some((metadata, body))) -} pub use ext_manager::MANAGE_EXTENSIONS_TOOL_NAME_COMPLETE; @@ -248,15 +190,15 @@ pub static PLATFORM_EXTENSIONS: Lazy ); map.insert( - skills::EXTENSION_NAME, + crate::skills::EXTENSION_NAME, PlatformExtensionDef { - name: skills::EXTENSION_NAME, + name: crate::skills::EXTENSION_NAME, display_name: "Skills", description: "Discover and provide skill instructions from filesystem and builtins", default_enabled: true, unprefixed_tools: true, hidden: false, - client_factory: |ctx| Box::new(skills::SkillsClient::new(ctx).unwrap()), + client_factory: |ctx| Box::new(crate::skills::SkillsClient::new(ctx).unwrap()), }, ); diff --git a/crates/goose/src/agents/platform_extensions/summon.rs b/crates/goose/src/agents/platform_extensions/summon.rs index f84a343025c5..68eb229e90ee 100644 --- a/crates/goose/src/agents/platform_extensions/summon.rs +++ b/crates/goose/src/agents/platform_extensions/summon.rs @@ -1,4 +1,3 @@ -use super::{parse_frontmatter, Source, SourceKind}; use crate::agents::extension::PlatformExtensionContext; use crate::agents::mcp_client::{Error, McpClientTrait}; use crate::agents::subagent_handler::{run_subagent_task, OnMessageCallback, SubagentRunParams}; @@ -13,8 +12,10 @@ use crate::recipe::local_recipes::load_local_recipe_file; use crate::recipe::{Recipe, Settings, RECIPE_FILE_EXTENSIONS}; use crate::session::extension_data::EnabledExtensionsState; use crate::session::SessionType; +use crate::sources::parse_frontmatter; use anyhow::Result; use async_trait::async_trait; +use goose_sdk::custom_requests::{SourceEntry, SourceType}; use rmcp::model::{ CallToolResult, Content, Implementation, InitializeResult, JsonObject, ListToolsResult, Meta, ServerCapabilities, ServerNotification, Tool, @@ -33,11 +34,11 @@ use tracing::{info, warn}; pub static EXTENSION_NAME: &str = "summon"; -fn kind_plural(kind: SourceKind) -> &'static str { +fn kind_plural(kind: SourceType) -> &'static str { match kind { - SourceKind::Subrecipe => "Subrecipes", - SourceKind::Recipe => "Recipes", - SourceKind::Agent => "Agents", + SourceType::Subrecipe => "Subrecipes", + SourceType::Recipe => "Recipes", + SourceType::Agent => "Agents", _ => "Other", } } @@ -95,7 +96,7 @@ struct AgentMetadata { model: Option, } -fn parse_agent_content(content: &str, path: &Path) -> Option { +fn parse_agent_content(content: &str, path: &Path) -> Option { let (metadata, body): (AgentMetadata, String) = match parse_frontmatter(content) { Ok(Some(parsed)) => parsed, Ok(None) => return None, @@ -119,20 +120,21 @@ fn parse_agent_content(content: &str, path: &Path) -> Option { format!("Agent{}", model_info) }); - Some(Source { + Some(SourceEntry { + source_type: SourceType::Agent, name: metadata.name, - kind: SourceKind::Agent, description, - path: path.to_path_buf(), content: body, + directory: path.to_string_lossy().into_owned(), + global: false, supporting_files: Vec::new(), }) } fn scan_recipes_from_dir( dir: &Path, - kind: SourceKind, - sources: &mut Vec, + kind: SourceType, + sources: &mut Vec, seen: &mut std::collections::HashSet, ) { let entries = match std::fs::read_dir(dir) { @@ -164,12 +166,13 @@ fn scan_recipes_from_dir( match Recipe::from_file_path(&path) { Ok(recipe) => { seen.insert(name.clone()); - sources.push(Source { + sources.push(SourceEntry { + source_type: kind, name, - kind, description: recipe.description.clone(), - path: path.clone(), content: recipe.instructions.clone().unwrap_or_default(), + directory: path.to_string_lossy().into_owned(), + global: false, supporting_files: Vec::new(), }); } @@ -182,7 +185,7 @@ fn scan_recipes_from_dir( fn scan_agents_from_dir( dir: &Path, - sources: &mut Vec, + sources: &mut Vec, seen: &mut std::collections::HashSet, ) { let entries = match std::fs::read_dir(dir) { @@ -218,8 +221,8 @@ fn scan_agents_from_dir( } } -pub fn discover_filesystem_sources(working_dir: &Path) -> Vec { - let mut sources: Vec = Vec::new(); +pub fn discover_filesystem_sources(working_dir: &Path) -> Vec { + let mut sources: Vec = Vec::new(); let mut seen: std::collections::HashSet = std::collections::HashSet::new(); let home = dirs::home_dir(); @@ -266,7 +269,7 @@ pub fn discover_filesystem_sources(working_dir: &Path) -> Vec { .collect(); for dir in local_recipe_dirs { - scan_recipes_from_dir(&dir, SourceKind::Recipe, &mut sources, &mut seen); + scan_recipes_from_dir(&dir, SourceType::Recipe, &mut sources, &mut seen); } for dir in local_agent_dirs { @@ -274,7 +277,7 @@ pub fn discover_filesystem_sources(working_dir: &Path) -> Vec { } for dir in global_recipe_dirs { - scan_recipes_from_dir(&dir, SourceKind::Recipe, &mut sources, &mut seen); + scan_recipes_from_dir(&dir, SourceType::Recipe, &mut sources, &mut seen); } for dir in global_agent_dirs { @@ -315,7 +318,7 @@ fn is_session_id(s: &str) -> bool { pub struct SummonClient { info: InitializeResult, context: PlatformExtensionContext, - source_cache: Mutex)>>, + source_cache: Mutex)>>, background_tasks: Mutex>, completed_tasks: Mutex>, notification_subscribers: Arc>>>, @@ -477,11 +480,11 @@ impl SummonClient { .unwrap_or_else(|| std::env::current_dir().unwrap_or_default()) } - async fn get_sources(&self, session_id: &str, working_dir: &Path) -> Vec { + async fn get_sources(&self, session_id: &str, working_dir: &Path) -> Vec { let fs_sources = self.get_filesystem_sources(working_dir).await; let mut seen: std::collections::HashSet = std::collections::HashSet::new(); - let mut sources: Vec = Vec::new(); + let mut sources: Vec = Vec::new(); self.add_subrecipes(session_id, &mut sources, &mut seen) .await; @@ -493,11 +496,11 @@ impl SummonClient { } } - sources.sort_by(|a, b| (&a.kind, &a.name).cmp(&(&b.kind, &b.name))); + sources.sort_by(|a, b| (&a.source_type, &a.name).cmp(&(&b.source_type, &b.name))); sources } - async fn get_filesystem_sources(&self, working_dir: &Path) -> Vec { + async fn get_filesystem_sources(&self, working_dir: &Path) -> Vec { let mut cache = self.source_cache.lock().await; if let Some((cached_at, cached_dir, sources)) = cache.as_ref() { if cached_dir == working_dir && cached_at.elapsed() < Duration::from_secs(60) { @@ -514,11 +517,11 @@ impl SummonClient { session_id: &str, name: &str, working_dir: &Path, - ) -> Result, String> { + ) -> Result, String> { let sources = self.get_sources(session_id, working_dir).await; if let Some(mut source) = sources.iter().find(|s| s.name == name).cloned() { - if source.kind == SourceKind::Subrecipe && source.content.is_empty() { + if source.source_type == SourceType::Subrecipe && source.content.is_empty() { source.content = self.load_subrecipe_content(session_id, &source.name).await; } return Ok(Some(source)); @@ -557,14 +560,14 @@ impl SummonClient { } } - fn discover_filesystem_sources(&self, working_dir: &Path) -> Vec { + fn discover_filesystem_sources(&self, working_dir: &Path) -> Vec { discover_filesystem_sources(working_dir) } async fn add_subrecipes( &self, session_id: &str, - sources: &mut Vec, + sources: &mut Vec, seen: &mut std::collections::HashSet, ) { let session = match self @@ -590,12 +593,13 @@ impl SummonClient { let description = self.build_subrecipe_description(sr).await; - sources.push(Source { + sources.push(SourceEntry { + source_type: SourceType::Subrecipe, name: sr.name.clone(), - kind: SourceKind::Subrecipe, description, - path: PathBuf::from(&sr.path), content: String::new(), + directory: sr.path.clone(), + global: false, supporting_files: Vec::new(), }); } @@ -841,8 +845,8 @@ impl SummonClient { } } - for kind in [SourceKind::Subrecipe, SourceKind::Recipe, SourceKind::Agent] { - let kind_sources: Vec<_> = sources.iter().filter(|s| s.kind == kind).collect(); + for kind in [SourceType::Subrecipe, SourceType::Recipe, SourceType::Agent] { + let kind_sources: Vec<_> = sources.iter().filter(|s| s.source_type == kind).collect(); if !kind_sources.is_empty() { output.push_str(&format!("\n{}:\n", kind_plural(kind))); for source in kind_sources { @@ -875,7 +879,7 @@ impl SummonClient { let output = format!( "# Loaded: {} ({})\n\n{}\n\n---\nThis knowledge is now available in your context.", - source.name, source.kind, content + source.name, source.source_type, content ); Ok(vec![Content::text(output)]) @@ -1080,16 +1084,16 @@ impl SummonClient { .await? .ok_or_else(|| format!("Source '{}' not found", source_name))?; - let mut recipe = match source.kind { - SourceKind::Recipe | SourceKind::Subrecipe => { + let mut recipe = match source.source_type { + SourceType::Recipe | SourceType::Subrecipe => { self.build_recipe_from_source(&source, params, session_id) .await? } - SourceKind::Agent => self.build_recipe_from_agent(&source, params)?, + SourceType::Agent => self.build_recipe_from_agent(&source, params)?, _ => { return Err(format!( "Source '{}' has kind '{}' which cannot be delegated from summon", - source_name, source.kind + source_name, source.source_type )) } }; @@ -1108,7 +1112,7 @@ impl SummonClient { async fn build_recipe_from_source( &self, - source: &Source, + source: &SourceEntry, params: &DelegateParams, session_id: &str, ) -> Result { @@ -1119,7 +1123,7 @@ impl SummonClient { .await .map_err(|e| format!("Failed to get session: {}", e))?; - if source.kind == SourceKind::Subrecipe { + if source.source_type == SourceType::Subrecipe { let sub_recipes = session.recipe.as_ref().and_then(|r| r.sub_recipes.as_ref()); if let Some(sub_recipes) = sub_recipes { @@ -1156,7 +1160,7 @@ impl SummonClient { } } - let recipe_file = load_local_recipe_file(source.path.to_str().unwrap_or("")) + let recipe_file = load_local_recipe_file(&source.directory) .map_err(|e| format!("Failed to load recipe '{}': {}", source.name, e))?; let param_values: Vec<(String, String)> = params @@ -1186,13 +1190,13 @@ impl SummonClient { fn build_recipe_from_agent( &self, - source: &Source, + source: &SourceEntry, params: &DelegateParams, ) -> Result { - let agent_content = if source.path.as_os_str().is_empty() { + let agent_content = if source.directory.is_empty() { return Err("Agent source has no path".to_string()); } else { - std::fs::read_to_string(&source.path) + std::fs::read_to_string(&source.directory) .map_err(|e| format!("Failed to read agent file: {}", e))? }; @@ -1747,14 +1751,14 @@ You review code."#; let recipe = sources .iter() - .find(|s| s.name == "deploy" && s.kind == SourceKind::Recipe) + .find(|s| s.name == "deploy" && s.source_type == SourceType::Recipe) .unwrap(); assert_eq!(recipe.description, "Deploy to production"); assert_eq!(recipe.content, "Run deploy steps"); let agent = sources .iter() - .find(|s| s.name == "reviewer" && s.kind == SourceKind::Agent) + .find(|s| s.name == "reviewer" && s.source_type == SourceType::Agent) .unwrap(); assert_eq!(agent.description, "Code reviewer"); assert!(agent.content.contains("You review code")); diff --git a/crates/goose/src/lib.rs b/crates/goose/src/lib.rs index 154e048b6258..ab64d11ca496 100644 --- a/crates/goose/src/lib.rs +++ b/crates/goose/src/lib.rs @@ -38,6 +38,7 @@ pub mod scheduler_trait; pub mod security; pub mod session; pub mod session_context; +pub mod skills; pub mod slash_commands; pub mod sources; pub mod subprocess; diff --git a/crates/goose/src/agents/builtin_skills/mod.rs b/crates/goose/src/skills/builtin.rs similarity index 70% rename from crates/goose/src/agents/builtin_skills/mod.rs rename to crates/goose/src/skills/builtin.rs index 039e7680448d..daaec140e29f 100644 --- a/crates/goose/src/agents/builtin_skills/mod.rs +++ b/crates/goose/src/skills/builtin.rs @@ -1,7 +1,6 @@ use include_dir::{include_dir, Dir}; -static BUILTIN_SKILLS_DIR: Dir = - include_dir!("$CARGO_MANIFEST_DIR/src/agents/builtin_skills/skills"); +static BUILTIN_SKILLS_DIR: Dir = include_dir!("$CARGO_MANIFEST_DIR/src/skills/builtins"); pub fn get_all() -> Vec<&'static str> { BUILTIN_SKILLS_DIR diff --git a/crates/goose/src/agents/builtin_skills/skills/goose_doc_guide.md b/crates/goose/src/skills/builtins/goose_doc_guide.md similarity index 100% rename from crates/goose/src/agents/builtin_skills/skills/goose_doc_guide.md rename to crates/goose/src/skills/builtins/goose_doc_guide.md diff --git a/crates/goose/src/agents/platform_extensions/skills.rs b/crates/goose/src/skills/client.rs similarity index 63% rename from crates/goose/src/agents/platform_extensions/skills.rs rename to crates/goose/src/skills/client.rs index c209cfea53be..c7f82af310a6 100644 --- a/crates/goose/src/agents/platform_extensions/skills.rs +++ b/crates/goose/src/skills/client.rs @@ -1,201 +1,19 @@ -use super::{parse_frontmatter, Source, SourceKind}; -use crate::agents::builtin_skills; +use super::discover_skills; use crate::agents::extension::PlatformExtensionContext; use crate::agents::mcp_client::{Error, McpClientTrait}; -use crate::agents::tool_execution::ToolCallContext; -use crate::config::paths::Paths; +use crate::agents::ToolCallContext; use async_trait::async_trait; +use goose_sdk::custom_requests::{SourceEntry, SourceType}; use rmcp::model::{ CallToolResult, Content, Implementation, InitializeResult, JsonObject, ListToolsResult, ServerCapabilities, ServerNotification, Tool, }; -use serde::Deserialize; -use std::collections::HashSet; use std::path::{Path, PathBuf}; use tokio::sync::mpsc; use tokio_util::sync::CancellationToken; -use tracing::warn; pub static EXTENSION_NAME: &str = "skills"; -#[derive(Debug, Deserialize)] -struct SkillMetadata { - name: String, - description: String, -} - -fn parse_skill_content(content: &str, path: PathBuf) -> Option { - let (metadata, body): (SkillMetadata, String) = match parse_frontmatter(content) { - Ok(Some(parsed)) => parsed, - Ok(None) => return None, - Err(e) => { - warn!("Failed to parse skill frontmatter: {}", e); - return None; - } - }; - - if metadata.name.contains('/') { - warn!("Skill name '{}' contains '/', skipping", metadata.name); - return None; - } - - Some(Source { - name: metadata.name, - kind: SourceKind::Skill, - description: metadata.description, - path, - content: body, - supporting_files: Vec::new(), - }) -} - -fn should_skip_dir(path: &Path) -> bool { - matches!( - path.file_name().and_then(|name| name.to_str()), - Some(".git") | Some(".hg") | Some(".svn") - ) -} - -fn walk_files_recursively( - dir: &Path, - visited_dirs: &mut HashSet, - should_descend: &mut G, - visit_file: &mut F, -) where - F: FnMut(&Path), - G: FnMut(&Path) -> bool, -{ - let canonical_dir = match std::fs::canonicalize(dir) { - Ok(path) => path, - Err(_) => return, - }; - - if !visited_dirs.insert(canonical_dir) { - return; - } - - let entries = match std::fs::read_dir(dir) { - Ok(e) => e, - Err(_) => return, - }; - - for entry in entries.flatten() { - let path = entry.path(); - if path.is_dir() { - if should_descend(&path) { - walk_files_recursively(&path, visited_dirs, should_descend, visit_file); - } - } else if path.is_file() { - visit_file(&path); - } - } -} - -fn scan_skills_from_dir(dir: &Path, seen: &mut HashSet) -> Vec { - let mut skill_files = Vec::new(); - let mut visited_dirs = HashSet::new(); - - walk_files_recursively( - dir, - &mut visited_dirs, - &mut |path| !should_skip_dir(path), - &mut |path| { - if path.file_name().and_then(|name| name.to_str()) == Some("SKILL.md") { - skill_files.push(path.to_path_buf()); - } - }, - ); - - let mut sources = Vec::new(); - for skill_file in skill_files { - let Some(skill_dir) = skill_file.parent() else { - continue; - }; - let content = match std::fs::read_to_string(&skill_file) { - Ok(c) => c, - Err(e) => { - warn!("Failed to read skill file {}: {}", skill_file.display(), e); - continue; - } - }; - - if let Some(mut source) = parse_skill_content(&content, skill_dir.to_path_buf()) { - if !seen.contains(&source.name) { - // Find supporting files in the skill directory - let mut files = Vec::new(); - let mut visited_support_dirs = HashSet::new(); - walk_files_recursively( - skill_dir, - &mut visited_support_dirs, - &mut |path| !should_skip_dir(path) && !path.join("SKILL.md").is_file(), - &mut |path| { - if path.file_name().and_then(|n| n.to_str()) != Some("SKILL.md") { - files.push(path.to_path_buf()); - } - }, - ); - source.supporting_files = files; - - seen.insert(source.name.clone()); - sources.push(source); - } - } - } - sources -} - -fn discover_skills(working_dir: &Path) -> Vec { - let mut sources = Vec::new(); - let mut seen = HashSet::new(); - - let home = dirs::home_dir(); - let config = Paths::config_dir(); - - let local_dirs = vec![ - working_dir.join(".goose/skills"), - working_dir.join(".claude/skills"), - working_dir.join(".agents/skills"), - ]; - - let global_dirs: Vec = [ - home.as_ref().map(|h| h.join(".agents/skills")), - Some(config.join("skills")), - home.as_ref().map(|h| h.join(".claude/skills")), - home.as_ref().map(|h| h.join(".config/agents/skills")), - ] - .into_iter() - .flatten() - .collect(); - - for dir in local_dirs { - sources.extend(scan_skills_from_dir(&dir, &mut seen)); - } - for dir in global_dirs { - sources.extend(scan_skills_from_dir(&dir, &mut seen)); - } - - for content in builtin_skills::get_all() { - if let Some(source) = parse_skill_content(content, PathBuf::new()) { - if !seen.contains(&source.name) { - seen.insert(source.name.clone()); - sources.push(Source { - kind: SourceKind::BuiltinSkill, - ..source - }); - } - } - } - - sources -} - -pub fn list_installed_skills(working_dir: Option<&Path>) -> Vec { - let dir = working_dir - .map(|p| p.to_path_buf()) - .unwrap_or_else(|| std::env::current_dir().unwrap_or_default()); - discover_skills(&dir) -} - pub struct SkillsClient { info: InitializeResult, working_dir: PathBuf, @@ -211,12 +29,14 @@ impl SkillsClient { let mut instructions = String::new(); if context.session.is_some() { - let sources = discover_skills(&working_dir); - let mut skills: Vec<&Source> = sources + let sources = discover_skills(Some(&working_dir)); + let mut skills: Vec<&SourceEntry> = sources .iter() - .filter(|s| s.kind == SourceKind::Skill || s.kind == SourceKind::BuiltinSkill) + .filter(|s| { + s.source_type == SourceType::Skill || s.source_type == SourceType::BuiltinSkill + }) .collect(); - skills.sort_by(|a, b| (&a.name, &a.path).cmp(&(&b.name, &b.path))); + skills.sort_by(|a, b| (&a.name, &a.directory).cmp(&(&b.name, &b.directory))); if !skills.is_empty() { instructions.push_str( @@ -300,24 +120,24 @@ impl McpClientTrait for SkillsClient { )])); } - let skills = discover_skills(&self.working_dir); + let skills = discover_skills(Some(&self.working_dir)); - // Direct skill match if let Some(skill) = skills.iter().find(|s| s.name == skill_name) { let mut output = format!( "# Loaded Skill: {} ({})\n\n{}\n", skill.name, - skill.kind, + skill.source_type, skill.to_load_text() ); if !skill.supporting_files.is_empty() { + let skill_dir = Path::new(&skill.directory); output.push_str(&format!( "\n## Supporting Files\n\nSkill directory: {}\n\n", - skill.path.display() + skill.directory )); for file in &skill.supporting_files { - if let Ok(relative) = file.strip_prefix(&skill.path) { + if let Ok(relative) = Path::new(file).strip_prefix(skill_dir) { let rel_str = relative.to_string_lossy().replace('\\', "/"); output.push_str(&format!( "- {} → load_skill(name: \"{}/{}\")\n", @@ -331,27 +151,27 @@ impl McpClientTrait for SkillsClient { return Ok(CallToolResult::success(vec![Content::text(output)])); } - // Supporting file match (skill_name contains '/') if let Some((parent_skill_name, raw_relative_path)) = skill_name.split_once('/') { let relative_path = raw_relative_path.replace('\\', "/"); if let Some(skill) = skills.iter().find(|s| { s.name == parent_skill_name - && matches!(s.kind, SourceKind::Skill | SourceKind::BuiltinSkill) + && matches!(s.source_type, SourceType::Skill | SourceType::BuiltinSkill) }) { - let canonical_skill_dir = skill - .path + let skill_dir = PathBuf::from(&skill.directory); + let canonical_skill_dir = skill_dir .canonicalize() - .unwrap_or_else(|_| skill.path.clone()); + .unwrap_or_else(|_| skill_dir.clone()); for file_path in &skill.supporting_files { - let Ok(rel) = file_path.strip_prefix(&skill.path) else { + let file_path_buf = Path::new(file_path); + let Ok(rel) = file_path_buf.strip_prefix(&skill_dir) else { continue; }; if rel.to_string_lossy().replace('\\', "/") != relative_path { continue; } - return Ok(match file_path.canonicalize() { + return Ok(match file_path_buf.canonicalize() { Ok(canonical) if canonical.starts_with(&canonical_skill_dir) => { match std::fs::read_to_string(&canonical) { Ok(content) => { @@ -381,7 +201,8 @@ impl McpClientTrait for SkillsClient { .supporting_files .iter() .filter_map(|f| { - f.strip_prefix(&skill.path) + Path::new(f) + .strip_prefix(&skill_dir) .ok() .map(|r| r.to_string_lossy().replace('\\', "/")) }) @@ -403,7 +224,6 @@ impl McpClientTrait for SkillsClient { } } - // No match — suggest similar skills let suggestions: Vec<&str> = skills .iter() .filter(|s| { diff --git a/crates/goose/src/skills/mod.rs b/crates/goose/src/skills/mod.rs new file mode 100644 index 000000000000..60f09d20b602 --- /dev/null +++ b/crates/goose/src/skills/mod.rs @@ -0,0 +1,386 @@ +//! Everything specific to skills: filesystem discovery (`SKILL.md` walking + +//! built-ins) and the runtime MCP client (`client` submodule). User-facing +//! CRUD lives in `crate::sources`, which generalizes across source types. + +mod builtin; +pub mod client; + +pub use client::{SkillsClient, EXTENSION_NAME}; + +use crate::config::paths::Paths; +use crate::sources::parse_frontmatter; +use goose_sdk::custom_requests::{SourceEntry, SourceType}; +use sacp::Error; +use serde::Deserialize; +use std::collections::HashSet; +use std::path::{Path, PathBuf}; +use tracing::warn; + +#[derive(Debug, Deserialize)] +pub struct SkillFrontmatter { + #[serde(default)] + pub name: Option, + #[serde(default)] + pub description: String, +} + +/// Canonical writable location for global user skills: `~/.agents/skills`. +pub fn global_skills_dir() -> Option { + dirs::home_dir().map(|h| h.join(".agents").join("skills")) +} + +/// Canonical writable location for project-scoped skills: +/// `/.goose/skills`. +pub fn project_skills_dir(project_dir: &Path) -> PathBuf { + project_dir.join(".goose").join("skills") +} + +pub(crate) fn skills_dir_global_or_err() -> Result { + global_skills_dir() + .ok_or_else(|| Error::internal_error().data("Could not determine home directory")) +} + +pub(crate) fn skills_dir_project_or_err(project_dir: &str) -> Result { + if project_dir.trim().is_empty() { + return Err( + Error::invalid_params().data("projectDir must not be empty when global is false") + ); + } + Ok(project_skills_dir(Path::new(project_dir))) +} + +pub(crate) fn skill_base_dir(global: bool, project_dir: Option<&str>) -> Result { + if global { + skills_dir_global_or_err() + } else { + let pd = project_dir.ok_or_else(|| { + Error::invalid_params().data("projectDir is required when global is false") + })?; + skills_dir_project_or_err(pd) + } +} + +pub(crate) fn validate_skill_name(name: &str) -> Result<(), Error> { + if name.is_empty() { + return Err(Error::invalid_params().data("Skill name must not be empty")); + } + if name.len() > 64 { + return Err(Error::invalid_params().data(format!( + "Invalid skill name \"{}\". Names must be at most 64 characters.", + name + ))); + } + if !name + .chars() + .all(|ch| ch.is_ascii_lowercase() || ch.is_ascii_digit() || ch == '-') + { + return Err(Error::invalid_params().data(format!( + "Invalid skill name \"{}\". Names may only contain lowercase letters, digits, and hyphens.", + name + ))); + } + if name.starts_with('-') || name.ends_with('-') { + return Err(Error::invalid_params().data(format!( + "Invalid skill name \"{}\". Names must not start or end with a hyphen.", + name + ))); + } + Ok(()) +} + +fn canonicalize_or_original(path: &Path) -> PathBuf { + path.canonicalize().unwrap_or_else(|_| path.to_path_buf()) +} + +fn inferred_discoverable_skill_root(path: &Path) -> Option { + let canonical_path = canonicalize_or_original(path); + + let mut global_roots = Vec::new(); + if let Some(global_root) = global_skills_dir() { + global_roots.push(global_root); + } + global_roots.push(Paths::config_dir().join("skills")); + if let Some(home) = dirs::home_dir() { + global_roots.push(home.join(".claude").join("skills")); + global_roots.push(home.join(".config").join("agents").join("skills")); + } + + for root in global_roots { + let canonical_root = canonicalize_or_original(&root); + if canonical_path.starts_with(&canonical_root) { + return Some(canonical_root); + } + } + + canonical_path.ancestors().find_map(|ancestor| { + let parent = ancestor.parent()?; + let is_project_skills_root = ancestor.file_name().and_then(|name| name.to_str()) + == Some("skills") + && matches!( + parent.file_name().and_then(|name| name.to_str()), + Some(".goose") | Some(".claude") | Some(".agents") + ); + is_project_skills_root.then(|| ancestor.to_path_buf()) + }) +} + +pub(crate) fn resolve_discoverable_skill_dir(path: &str) -> Result { + if path.is_empty() { + return Err(Error::invalid_params().data("Source path must not be empty")); + } + + 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() + || !canonical_dir.is_dir() + || !canonical_dir.join("SKILL.md").is_file() + { + return Err(Error::invalid_params().data(format!("Source \"{}\" not found", path))); + } + + Ok(canonical_dir) +} + +pub(crate) fn resolve_skill_dir(path: &str) -> Result { + resolve_discoverable_skill_dir(path) +} + +pub(crate) fn is_global_skill_dir(path: &Path) -> bool { + global_skills_dir().as_deref().is_some_and(|root| { + canonicalize_or_original(path).starts_with(canonicalize_or_original(root)) + }) +} + +pub(crate) fn infer_skill_name(dir: &Path) -> String { + let md = dir.join("SKILL.md"); + if let Ok(raw) = std::fs::read_to_string(&md) { + if let Ok(Some((meta, _))) = parse_frontmatter::(&raw) { + if let Some(n) = meta.name.filter(|n| !n.is_empty()) { + return n; + } + } + } + dir.file_name() + .and_then(|n| n.to_str()) + .unwrap_or("unnamed") + .to_string() +} + +pub(crate) fn build_skill_md(name: &str, description: &str, content: &str) -> String { + let safe_desc = description.replace('\'', "''"); + let mut md = format!("---\nname: {}\ndescription: '{}'\n---\n", name, safe_desc); + if !content.is_empty() { + md.push('\n'); + md.push_str(content); + md.push('\n'); + } + md +} + +pub(crate) fn parse_skill_frontmatter(raw: &str) -> (String, String) { + if !raw.trim_start().starts_with("---") { + return (String::new(), raw.to_string()); + } + match parse_frontmatter::(raw) { + Ok(Some((meta, body))) => (meta.description, body), + _ => (String::new(), raw.to_string()), + } +} + +/// Every directory the agent reads skills from, paired with whether each is a +/// 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(); + + if let Some(wd) = working_dir { + dirs.push((wd.join(".goose").join("skills"), false)); + dirs.push((wd.join(".claude").join("skills"), false)); + dirs.push((wd.join(".agents").join("skills"), false)); + } + + let home = dirs::home_dir(); + if let Some(h) = home.as_ref() { + dirs.push((h.join(".agents").join("skills"), true)); + } + dirs.push((Paths::config_dir().join("skills"), true)); + 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)); + } + + dirs +} + +fn parse_skill_content(content: &str, path: &Path, global: bool) -> Option { + let (metadata, body): (SkillFrontmatter, String) = match parse_frontmatter(content) { + Ok(Some(parsed)) => parsed, + Ok(None) => return None, + Err(e) => { + warn!("Failed to parse skill frontmatter: {}", e); + return None; + } + }; + + let name = match metadata.name.filter(|n| !n.is_empty()) { + Some(n) => n, + None => { + warn!( + "Skill at '{}' is missing a required 'name' in frontmatter, skipping", + path.display() + ); + return None; + } + }; + + if name.contains('/') { + warn!("Skill name '{}' contains '/', skipping", name); + return None; + } + + Some(SourceEntry { + source_type: SourceType::Skill, + name, + description: metadata.description, + content: body, + directory: path.to_string_lossy().into_owned(), + global, + supporting_files: Vec::new(), + }) +} + +fn should_skip_dir(path: &Path) -> bool { + matches!( + path.file_name().and_then(|name| name.to_str()), + Some(".git") | Some(".hg") | Some(".svn") + ) +} + +fn walk_files_recursively( + dir: &Path, + visited_dirs: &mut HashSet, + should_descend: &mut G, + visit_file: &mut F, +) where + F: FnMut(&Path), + G: FnMut(&Path) -> bool, +{ + let canonical_dir = match std::fs::canonicalize(dir) { + Ok(path) => path, + Err(_) => return, + }; + + if !visited_dirs.insert(canonical_dir) { + return; + } + + let entries = match std::fs::read_dir(dir) { + Ok(e) => e, + Err(_) => return, + }; + + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + if should_descend(&path) { + walk_files_recursively(&path, visited_dirs, should_descend, visit_file); + } + } else if path.is_file() { + visit_file(&path); + } + } +} + +fn scan_skills_from_dir(dir: &Path, global: bool, seen: &mut HashSet) -> Vec { + let mut skill_files = Vec::new(); + let mut visited_dirs = HashSet::new(); + + walk_files_recursively( + dir, + &mut visited_dirs, + &mut |path| !should_skip_dir(path), + &mut |path| { + if path.file_name().and_then(|name| name.to_str()) == Some("SKILL.md") { + skill_files.push(path.to_path_buf()); + } + }, + ); + + let mut sources = Vec::new(); + for skill_file in skill_files { + let Some(skill_dir) = skill_file.parent() else { + continue; + }; + let content = match std::fs::read_to_string(&skill_file) { + Ok(c) => c, + Err(e) => { + warn!("Failed to read skill file {}: {}", skill_file.display(), e); + continue; + } + }; + + if let Some(mut source) = parse_skill_content(&content, skill_dir, global) { + if !seen.contains(&source.name) { + let mut files = Vec::new(); + let mut visited_support_dirs = HashSet::new(); + walk_files_recursively( + skill_dir, + &mut visited_support_dirs, + &mut |path| !should_skip_dir(path) && !path.join("SKILL.md").is_file(), + &mut |path| { + if path.file_name().and_then(|n| n.to_str()) != Some("SKILL.md") { + files.push(path.to_string_lossy().into_owned()); + } + }, + ); + source.supporting_files = files; + + seen.insert(source.name.clone()); + sources.push(source); + } + } + } + sources +} + +/// Discover skills from all configured filesystem locations and built-ins. +/// 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 { + 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) { + sources.push(source); + } + } + + for content in builtin::get_all() { + if let Some(source) = parse_skill_content(content, &PathBuf::new(), true) { + if !seen.contains(&source.name) { + seen.insert(source.name.clone()); + sources.push(SourceEntry { + source_type: SourceType::BuiltinSkill, + ..source + }); + } + } + } + + sources +} + +pub fn list_installed_skills(working_dir: Option<&Path>) -> Vec { + let fallback; + let wd = match working_dir { + Some(p) => Some(p), + None => { + fallback = std::env::current_dir().ok(); + fallback.as_deref() + } + }; + discover_skills(wd) +} diff --git a/crates/goose/src/sources.rs b/crates/goose/src/sources.rs index 35d0b67805a4..f6d9c27eeb27 100644 --- a/crates/goose/src/sources.rs +++ b/crates/goose/src/sources.rs @@ -1,126 +1,47 @@ //! Filesystem-backed CRUD for [`SourceEntry`] values exchanged over ACP custom -//! methods. A source is a user-editable entity stored under a per-scope root -//! directory — `~/.agents/skills` for global sources and `/.goose/skills` -//! for project-specific sources. -use crate::agents::platform_extensions::parse_frontmatter; +use crate::skills::{ + build_skill_md, discover_skills, infer_skill_name, is_global_skill_dir, + parse_skill_frontmatter, resolve_discoverable_skill_dir, resolve_skill_dir, skill_base_dir, + validate_skill_name, +}; use fs_err as fs; use goose_sdk::custom_requests::{SourceEntry, SourceType}; use sacp::Error; use serde::Deserialize; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; -#[derive(Deserialize)] -struct SkillFront { - #[serde(default)] - description: String, -} - -const GLOBAL_SKILLS_SUBPATH: &[&str] = &[".agents", "skills"]; -const PROJECT_SKILLS_SUBPATH: &[&str] = &[".goose", "skills"]; - -fn home_dir() -> Result { - dirs::home_dir() - .ok_or_else(|| Error::internal_error().data("Could not determine home directory")) -} - -fn skills_dir_global() -> Result { - let mut dir = home_dir()?; - for part in GLOBAL_SKILLS_SUBPATH { - dir = dir.join(part); +pub fn parse_frontmatter Deserialize<'de>>( + content: &str, +) -> Result, serde_yaml::Error> { + let parts: Vec<&str> = content.split("---").collect(); + if parts.len() < 3 { + return Ok(None); } - Ok(dir) -} -fn skills_dir_project(project_dir: &str) -> Result { - if project_dir.trim().is_empty() { - return Err( - Error::invalid_params().data("projectDir must not be empty when global is false") - ); - } - let mut dir = PathBuf::from(project_dir); - for part in PROJECT_SKILLS_SUBPATH { - dir = dir.join(part); - } - Ok(dir) -} + let yaml_content = parts[1].trim(); + let metadata: T = serde_yaml::from_str(yaml_content)?; -fn source_base_dir( - source_type: SourceType, - global: bool, - project_dir: Option<&str>, -) -> Result { - match source_type { - SourceType::Skill => { - if global { - skills_dir_global() - } else { - let pd = project_dir.ok_or_else(|| { - Error::invalid_params().data("projectDir is required when global is false") - })?; - skills_dir_project(pd) - } - } - } + let body = parts[2..].join("---").trim().to_string(); + Ok(Some((metadata, body))) } -/// Kebab-case validation: `^[a-z0-9]+(-[a-z0-9]+)*$`. Prevents path traversal -/// via names like `../../.ssh/authorized_keys`. -fn validate_source_name(name: &str) -> Result<(), Error> { - if name.is_empty() { - return Err(Error::invalid_params().data("Source name must not be empty")); - } - let mut expect_alnum = true; - for ch in name.chars() { - if ch.is_ascii_lowercase() || ch.is_ascii_digit() { - expect_alnum = false; - } else if ch == '-' && !expect_alnum { - expect_alnum = true; - } else { - return Err(Error::invalid_params().data(format!( - "Invalid source name \"{}\". Names must be kebab-case (lowercase letters, digits, and hyphens; \ - must not start or end with a hyphen or contain consecutive hyphens).", - name - ))); - } - } - if expect_alnum { +fn require_skill_type(source_type: SourceType) -> Result<(), Error> { + if source_type != SourceType::Skill { return Err(Error::invalid_params().data(format!( - "Invalid source name \"{}\". Names must not end with a hyphen.", - name + "Source type '{}' is not supported. Only 'skill' is currently supported.", + source_type ))); } Ok(()) } -fn build_skill_md(name: &str, description: &str, content: &str) -> String { - // YAML single-quoted strings escape a literal single quote by doubling it. - let safe_desc = description.replace('\'', "''"); - let mut md = format!("---\nname: {}\ndescription: '{}'\n---\n", name, safe_desc); - if !content.is_empty() { - md.push('\n'); - md.push_str(content); - md.push('\n'); - } - md -} - -fn parse_skill_frontmatter(raw: &str) -> (String, String) { - if !raw.trim_start().starts_with("---") { - return (String::new(), raw.to_string()); - } - match parse_frontmatter::(raw) { - Ok(Some((meta, body))) => (meta.description, body), - _ => (String::new(), raw.to_string()), - } -} - fn source_entry( source_type: SourceType, name: &str, description: &str, content: &str, - dir: &Path, + dir: &std::path::Path, global: bool, ) -> SourceEntry { SourceEntry { @@ -130,6 +51,7 @@ fn source_entry( content: content.to_string(), directory: dir.to_string_lossy().to_string(), global, + supporting_files: Vec::new(), } } @@ -141,8 +63,9 @@ pub fn create_source( global: bool, project_dir: Option<&str>, ) -> Result { - validate_source_name(name)?; - let dir = source_base_dir(source_type, global, project_dir)?.join(name); + require_skill_type(source_type)?; + validate_skill_name(name)?; + let dir = skill_base_dir(global, project_dir)?.join(name); if dir.exists() { return Err( @@ -170,20 +93,42 @@ pub fn create_source( pub fn update_source( source_type: SourceType, + path: &str, name: &str, description: &str, content: &str, - global: bool, - project_dir: Option<&str>, ) -> Result { - validate_source_name(name)?; - let dir = source_base_dir(source_type, global, project_dir)?.join(name); + require_skill_type(source_type)?; + validate_skill_name(name)?; + + let dir = resolve_discoverable_skill_dir(path)?; + let current_dir_name = dir + .file_name() + .and_then(|value| value.to_str()) + .ok_or_else(|| Error::internal_error().data("Failed to resolve source directory name"))?; + + let target_dir = if name == current_dir_name { + dir.clone() + } else { + let base_dir = dir.parent().ok_or_else(|| { + Error::internal_error().data("Failed to resolve source base directory") + })?; + let target_dir = base_dir.join(name); + + if target_dir.exists() { + return Err( + Error::invalid_params().data(format!("A source named \"{}\" already exists", name)) + ); + } - if !dir.exists() { - return Err(Error::invalid_params().data(format!("Source \"{}\" not found", name))); - } + fs::rename(&dir, &target_dir).map_err(|e| { + Error::internal_error().data(format!("Failed to rename source directory: {e}")) + })?; - let file_path = dir.join("SKILL.md"); + target_dir + }; + + let file_path = target_dir.join("SKILL.md"); let md = build_skill_md(name, description, content); fs::write(&file_path, md) .map_err(|e| Error::internal_error().data(format!("Failed to write SKILL.md: {e}")))?; @@ -193,23 +138,14 @@ pub fn update_source( name, description, content, - &dir, - global, + &target_dir, + is_global_skill_dir(&target_dir), )) } -pub fn delete_source( - source_type: SourceType, - name: &str, - global: bool, - project_dir: Option<&str>, -) -> Result<(), Error> { - validate_source_name(name)?; - let dir = source_base_dir(source_type, global, project_dir)?.join(name); - - if !dir.exists() { - return Err(Error::invalid_params().data(format!("Source \"{}\" not found", name))); - } +pub fn delete_source(source_type: SourceType, path: &str) -> Result<(), Error> { + require_skill_type(source_type)?; + let dir = resolve_skill_dir(path)?; fs::remove_dir_all(&dir) .map_err(|e| Error::internal_error().data(format!("Failed to delete source: {e}")))?; Ok(()) @@ -219,97 +155,45 @@ pub fn list_sources( source_type: Option, project_dir: Option<&str>, ) -> Result, Error> { - let kinds: Vec = match source_type { - Some(k) => vec![k], - None => vec![SourceType::Skill], - }; - - let mut sources = Vec::new(); - for kind in kinds { - match kind { - SourceType::Skill => { - if let Some(pd) = project_dir { - if !pd.trim().is_empty() { - let dir = skills_dir_project(pd)?; - sources.extend(read_skill_dir(&dir, false)?); - } - } - let dir = skills_dir_global()?; - sources.extend(read_skill_dir(&dir, true)?); - } - } + if let Some(t) = source_type { + require_skill_type(t)?; } + + let working_dir = project_dir + .map(str::trim) + .filter(|p| !p.is_empty()) + .map(PathBuf::from); + + let mut sources: Vec = discover_skills(working_dir.as_deref()) + .into_iter() + .filter(|s| s.source_type == SourceType::Skill) + .collect(); + sources.sort_by(|a, b| a.name.cmp(&b.name)); Ok(sources) } -fn read_skill_dir(dir: &Path, global: bool) -> Result, Error> { - if !dir.exists() { - return Ok(Vec::new()); - } - let entries = fs::read_dir(dir) - .map_err(|e| Error::internal_error().data(format!("Failed to read skills dir: {e}")))?; - - let mut out = Vec::new(); - for entry in entries.flatten() { - let path = entry.path(); - if !path.is_dir() { - continue; - } - let skill_md = path.join("SKILL.md"); - if !skill_md.exists() { - continue; - } - let name = path - .file_name() - .and_then(|n| n.to_str()) - .unwrap_or("") - .to_string(); - let raw = fs::read_to_string(&skill_md).unwrap_or_default(); - let (description, content) = parse_skill_frontmatter(&raw); - out.push(source_entry( - SourceType::Skill, - &name, - &description, - &content, - &path, - global, - )); - } - Ok(out) -} - -pub fn export_source( - source_type: SourceType, - name: &str, - global: bool, - project_dir: Option<&str>, -) -> Result<(String, String), Error> { - validate_source_name(name)?; - let dir = source_base_dir(source_type, global, project_dir)?.join(name); - - if !dir.exists() { - return Err(Error::invalid_params().data(format!("Source \"{}\" not found", name))); - } +pub fn export_source(source_type: SourceType, path: &str) -> Result<(String, String), Error> { + require_skill_type(source_type)?; + let dir = resolve_discoverable_skill_dir(path)?; let md = dir.join("SKILL.md"); let raw = fs::read_to_string(&md) .map_err(|e| Error::internal_error().data(format!("Failed to read SKILL.md: {e}")))?; let (description, content) = parse_skill_frontmatter(&raw); - let type_slug = match source_type { - SourceType::Skill => "skill", - }; + let name = infer_skill_name(&dir); + let export = serde_json::json!({ "version": 1, - "type": type_slug, + "type": "skill", "name": name, "description": description, "content": content, }); let json = serde_json::to_string_pretty(&export) .map_err(|e| Error::internal_error().data(format!("Failed to serialize source: {e}")))?; - let filename = format!("{}.{}.json", name, type_slug); + let filename = format!("{}.skill.json", name); Ok((json, filename)) } @@ -331,15 +215,17 @@ pub fn import_sources( ); } - // Default to `skill` to preserve compatibility with pre-sources skill exports. - let source_type = match value + match value .get("type") .and_then(|v| v.as_str()) .unwrap_or("skill") { - "skill" => SourceType::Skill, + "skill" => {} other => { - return Err(Error::invalid_params().data(format!("Unsupported source type: {}", other))); + return Err(Error::invalid_params().data(format!( + "Source type '{}' is not supported. Only 'skill' is currently supported.", + other + ))); } }; @@ -361,7 +247,6 @@ pub fn import_sources( return Err(Error::invalid_params().data("Source description must not be empty")); } - // Accept both the new `content` key and the legacy skills `instructions` key. let content = value .get("content") .or_else(|| value.get("instructions")) @@ -369,9 +254,9 @@ pub fn import_sources( .unwrap_or("") .to_string(); - validate_source_name(&name)?; + validate_skill_name(&name)?; - let base = source_base_dir(source_type, global, project_dir)?; + let base = skill_base_dir(global, project_dir)?; let mut final_name = name.clone(); if base.join(&final_name).exists() { final_name = format!("{}-imported", name); @@ -392,7 +277,7 @@ pub fn import_sources( .map_err(|e| Error::internal_error().data(format!("Failed to write SKILL.md: {e}")))?; Ok(vec![source_entry( - source_type, + SourceType::Skill, &final_name, &description, &content, @@ -407,15 +292,17 @@ mod tests { use tempfile::TempDir; #[test] - fn kebab_case_validation() { - assert!(validate_source_name("my-skill").is_ok()); - assert!(validate_source_name("abc123").is_ok()); - assert!(validate_source_name("").is_err()); - assert!(validate_source_name("-leading").is_err()); - assert!(validate_source_name("trailing-").is_err()); - assert!(validate_source_name("double--hyphen").is_err()); - assert!(validate_source_name("CAPS").is_err()); - assert!(validate_source_name("../escape").is_err()); + fn skill_name_validation() { + assert!(validate_skill_name("my-skill").is_ok()); + assert!(validate_skill_name("abc123").is_ok()); + assert!(validate_skill_name("double--hyphen").is_ok()); + assert!(validate_skill_name("").is_err()); + assert!(validate_skill_name("-leading").is_err()); + assert!(validate_skill_name("trailing-").is_err()); + assert!(validate_skill_name("CAPS").is_err()); + assert!(validate_skill_name("../escape").is_err()); + assert!(validate_skill_name(&"a".repeat(64)).is_ok()); + assert!(validate_skill_name(&"a".repeat(65)).is_err()); } #[test] @@ -434,24 +321,25 @@ mod tests { .unwrap(); assert_eq!(created.name, "my-skill"); assert!(!created.global); - assert!(PathBuf::from(&created.directory).join("SKILL.md").exists()); + let dir = PathBuf::from(&created.directory); + assert!(dir.join("SKILL.md").exists()); let listed = list_sources(Some(SourceType::Skill), Some(project)).unwrap(); assert!(listed.iter().any(|s| s.name == "my-skill" && !s.global)); let updated = update_source( SourceType::Skill, + created.directory.as_str(), "my-skill", "now does a different thing", "step three", - false, - Some(project), ) .unwrap(); assert_eq!(updated.description, "now does a different thing"); + assert_eq!(updated.name, "my-skill"); - delete_source(SourceType::Skill, "my-skill", false, Some(project)).unwrap(); - assert!(!PathBuf::from(&created.directory).exists()); + delete_source(SourceType::Skill, created.directory.as_str()).unwrap(); + assert!(!dir.exists()); } #[test] @@ -489,13 +377,9 @@ mod tests { ) .unwrap(); - let (json, filename) = export_source( - SourceType::Skill, - "portable", - false, - Some(project_a.to_str().unwrap()), - ) - .unwrap(); + let portable_dir = project_a.join(".goose").join("skills").join("portable"); + let (json, filename) = + export_source(SourceType::Skill, portable_dir.to_str().unwrap()).unwrap(); assert_eq!(filename, "portable.skill.json"); let imported = import_sources(&json, false, Some(project_b.to_str().unwrap())).unwrap(); @@ -505,6 +389,61 @@ mod tests { assert_eq!(imported[0].content, "body goes here"); } + #[test] + fn export_allows_discovered_read_only_skill() { + let tmp = TempDir::new().unwrap(); + let project = tmp.path(); + let claude_skill_dir = project.join(".claude").join("skills").join("portable"); + std::fs::create_dir_all(&claude_skill_dir).unwrap(); + std::fs::write( + claude_skill_dir.join("SKILL.md"), + build_skill_md("portable", "describes itself", "body goes here"), + ) + .unwrap(); + + let listed = + list_sources(Some(SourceType::Skill), Some(project.to_str().unwrap())).unwrap(); + let exported_skill = listed + .iter() + .find(|skill| skill.name == "portable") + .expect("expected listed skill"); + + let (json, filename) = + export_source(SourceType::Skill, exported_skill.directory.as_str()).unwrap(); + assert_eq!(filename, "portable.skill.json"); + assert!(json.contains("\"name\": \"portable\"")); + } + + #[test] + fn update_allows_discovered_read_only_skill() { + let tmp = TempDir::new().unwrap(); + let project = tmp.path(); + let claude_skill_dir = project.join(".claude").join("skills").join("portable"); + std::fs::create_dir_all(&claude_skill_dir).unwrap(); + std::fs::write( + claude_skill_dir.join("SKILL.md"), + build_skill_md("portable", "describes itself", "body goes here"), + ) + .unwrap(); + + let updated = update_source( + SourceType::Skill, + claude_skill_dir.to_str().unwrap(), + "portable", + "updated description", + "updated body", + ) + .unwrap(); + + assert_eq!(updated.name, "portable"); + assert_eq!(updated.description, "updated description"); + assert_eq!(updated.content, "updated body"); + + let raw = std::fs::read_to_string(claude_skill_dir.join("SKILL.md")).unwrap(); + assert!(raw.contains("description: 'updated description'")); + assert!(raw.contains("updated body")); + } + #[test] fn import_collision_appends_suffix() { let tmp = TempDir::new().unwrap(); @@ -523,4 +462,116 @@ mod tests { let imported = import_sources(&payload, false, Some(project)).unwrap(); assert_eq!(imported[0].name, "busy-imported"); } + + #[test] + fn update_rejects_nonexistent_source() { + let tmp = TempDir::new().unwrap(); + let missing_dir = tmp + .path() + .join(".goose") + .join("skills") + .join("no-such-skill"); + let err = update_source( + SourceType::Skill, + missing_dir.to_str().unwrap(), + "no-such-skill", + "d", + "c", + ) + .unwrap_err(); + assert!(format!("{:?}", err).contains("not found")); + } + + #[test] + fn delete_rejects_nonexistent_source() { + let tmp = TempDir::new().unwrap(); + let missing_dir = tmp + .path() + .join(".goose") + .join("skills") + .join("no-such-skill"); + let err = delete_source(SourceType::Skill, missing_dir.to_str().unwrap()).unwrap_err(); + assert!(format!("{:?}", err).contains("not found")); + } + + #[test] + fn rejects_non_skill_source_type() { + let tmp = TempDir::new().unwrap(); + let project = tmp.path().to_str().unwrap(); + + let err = create_source( + SourceType::BuiltinSkill, + "x", + "d", + "c", + false, + Some(project), + ) + .unwrap_err(); + assert!(format!("{:?}", err).contains("not supported")); + + let err = update_source(SourceType::Recipe, "x", "x", "d", "c").unwrap_err(); + assert!(format!("{:?}", err).contains("not supported")); + + let err = delete_source(SourceType::Subrecipe, "x").unwrap_err(); + assert!(format!("{:?}", err).contains("not supported")); + + let err = list_sources(Some(SourceType::BuiltinSkill), Some(project)).unwrap_err(); + assert!(format!("{:?}", err).contains("not supported")); + + let err = export_source(SourceType::Recipe, "x").unwrap_err(); + assert!(format!("{:?}", err).contains("not supported")); + } + + #[test] + fn update_derives_name_from_frontmatter() { + let tmp = TempDir::new().unwrap(); + let project = tmp.path().to_str().unwrap(); + + create_source( + SourceType::Skill, + "my-dir", + "orig", + "body", + false, + Some(project), + ) + .unwrap(); + + let skill_dir = tmp.path().join(".goose").join("skills").join("my-dir"); + let updated = update_source( + SourceType::Skill, + skill_dir.to_str().unwrap(), + "my-dir", + "new description", + "new body", + ) + .unwrap(); + // Name is derived from the frontmatter written by create_source + assert_eq!(updated.name, "my-dir"); + } + + #[test] + fn update_rejects_path_traversal() { + let tmp = TempDir::new().unwrap(); + let project = tmp.path(); + let escaped_dir = project.join(".goose").join("escaped"); + std::fs::create_dir_all(&escaped_dir).unwrap(); + std::fs::write( + escaped_dir.join("SKILL.md"), + "---\nname: escaped\ndescription: escaped\n---\ncontent", + ) + .unwrap(); + + let attempted_escape = project.join(".goose").join("escaped"); + let err = update_source( + SourceType::Skill, + attempted_escape.to_str().unwrap(), + "escaped", + "new description", + "new content", + ) + .unwrap_err(); + assert!(format!("{:?}", err).contains("not found")); + } } diff --git a/ui/goose2/AGENTS.md b/ui/goose2/AGENTS.md index 3423b367e2dc..da4382ada4c6 100644 --- a/ui/goose2/AGENTS.md +++ b/ui/goose2/AGENTS.md @@ -150,7 +150,7 @@ React UI ──► @aaif/goose-sdk (TS) ──► goose-acp (WebSocket, ACP The skills → sources migration in [#8675](https://github.com/block/goose/pull/8675) is the clearest illustration of the rule. **It deleted 319 lines of Tauri-command code in `src-tauri/src/commands/skills.rs` and replaced them with ACP custom methods.** If you find yourself wanting to add an `invoke()` command that proxies to `goose`, that PR is what "doing it the other way" looks like. Copy this shape when adding new endpoints: -1. **Define the request/response in `crates/goose-sdk/src/custom_requests.rs`.** Use the `JsonRpcRequest` / `JsonRpcResponse` derives and the `#[request(method = "_goose//", response = ...)]` attribute. Sources uses namespaced methods like `_goose/sources/create`, `_goose/sources/list`, `_goose/sources/update`, `_goose/sources/delete`, `_goose/sources/export`, `_goose/sources/import` with paired request/response structs (`CreateSourceRequest` / `CreateSourceResponse`, etc.). +1. **Define the request/response in `crates/goose-sdk/src/custom_requests.rs`.** Use the `JsonRpcRequest` / `JsonRpcResponse` derives and the `#[request(method = "_goose//", response = ...)]` attribute. Sources uses namespaced methods like `_goose/sources/create`, `_goose/sources/list`, `_goose/sources/update`, `_goose/sources/delete`, `_goose/sources/export`, `_goose/sources/import` with paired request/response structs (`CreateSourceRequest` / `CreateSourceResponse`, etc.). Keep the docs on those structs aligned with the implementation: today `_goose/sources/list` is still skill-only; create/import take an explicit target scope (`global`, plus `projectDir` for project sources), while update/delete/export operate on an existing skill by absolute `path`. 2. **Implement the handler in `crates/goose-acp/src/server.rs`** with `#[custom_method(YourRequest)]`. Keep it thin: unpack the request, call into the `goose` crate, wrap the result. The sources handlers are ~5 lines each — e.g. `on_list_sources` just calls `goose::sources::list_sources(...)` and returns the typed response. Errors map to `sacp::Error::invalid_params()` / `internal_error()`. 3. **Put the real logic in the `goose` crate.** Sources lives in `crates/goose/src/sources.rs` — filesystem CRUD, frontmatter parsing, scope resolution, all of it. `goose-acp` knows nothing about where skills are stored on disk; it just forwards typed arguments. This separation is the point. 4. **Regenerate the SDK.** The TS methods on `GooseClient` are generated into `ui/sdk/src/generated/`. Do not hand-edit generated files. diff --git a/ui/goose2/src/features/skills/api/skills.ts b/ui/goose2/src/features/skills/api/skills.ts index 5f32147c1ad3..8fe1af7c9992 100644 --- a/ui/goose2/src/features/skills/api/skills.ts +++ b/ui/goose2/src/features/skills/api/skills.ts @@ -1,28 +1,36 @@ +import type { SourceEntry } from "@aaif/goose-sdk"; import { getClient } from "@/shared/api/acpConnection"; +const SKILL_SOURCE_TYPE = "skill" as const; + export interface SkillInfo { name: string; description: string; instructions: string; path: string; + fileLocation: string; } -// Shape returned by _goose/sources/*. Narrowed to skill-type sources here. -interface SourceEntry { - type: "skill"; - name: string; - description: string; - content: string; - directory: string; - global: boolean; +type SkillSourceEntry = SourceEntry & { type: typeof SKILL_SOURCE_TYPE }; + +function isSkillSource(source: SourceEntry): source is SkillSourceEntry { + return source.type === SKILL_SOURCE_TYPE; } -function toSkillInfo(source: SourceEntry): SkillInfo { +function getSkillFileLocation(directory: string): string { + const separator = directory.includes("\\") ? "\\" : "/"; + return directory.endsWith(separator) + ? `${directory}SKILL.md` + : `${directory}${separator}SKILL.md`; +} + +function toSkillInfo(source: SkillSourceEntry): SkillInfo { return { name: source.name, description: source.description, instructions: source.content, path: source.directory, + fileLocation: getSkillFileLocation(source.directory), }; } @@ -32,8 +40,8 @@ export async function createSkill( instructions: string, ): Promise { const client = await getClient(); - await client.extMethod("_goose/sources/create", { - type: "skill", + await client.goose.GooseSourcesCreate({ + type: SKILL_SOURCE_TYPE, name, description, content: instructions, @@ -43,46 +51,51 @@ export async function createSkill( export async function listSkills(): Promise { const client = await getClient(); - const raw = await client.extMethod("_goose/sources/list", { type: "skill" }); - const sources = (raw.sources ?? []) as SourceEntry[]; - return sources.map(toSkillInfo); + const response = await client.goose.GooseSourcesList({ + type: SKILL_SOURCE_TYPE, + }); + return response.sources.filter(isSkillSource).map(toSkillInfo); } -export async function deleteSkill(name: string): Promise { +export async function deleteSkill(path: string): Promise { const client = await getClient(); - await client.extMethod("_goose/sources/delete", { - type: "skill", - name, - global: true, + await client.goose.GooseSourcesDelete({ + type: SKILL_SOURCE_TYPE, + path, }); } export async function updateSkill( + path: string, name: string, description: string, instructions: string, ): Promise { const client = await getClient(); - const raw = await client.extMethod("_goose/sources/update", { - type: "skill", + const response = await client.goose.GooseSourcesUpdate({ + type: SKILL_SOURCE_TYPE, + path, name, description, content: instructions, - global: true, }); - return toSkillInfo(raw.source as SourceEntry); + + if (!isSkillSource(response.source)) { + throw new Error(`Unexpected source type returned: ${response.source.type}`); + } + + return toSkillInfo(response.source); } export async function exportSkill( - name: string, + path: string, ): Promise<{ json: string; filename: string }> { const client = await getClient(); - const raw = await client.extMethod("_goose/sources/export", { - type: "skill", - name, - global: true, + const response = await client.goose.GooseSourcesExport({ + type: SKILL_SOURCE_TYPE, + path, }); - return { json: raw.json as string, filename: raw.filename as string }; + return { json: response.json, filename: response.filename }; } export async function importSkills( @@ -92,12 +105,13 @@ export async function importSkills( if (!fileName.endsWith(".skill.json") && !fileName.endsWith(".json")) { throw new Error("File must have a .skill.json or .json extension"); } + const data = new TextDecoder().decode(new Uint8Array(fileBytes)); const client = await getClient(); - const raw = await client.extMethod("_goose/sources/import", { + const response = await client.goose.GooseSourcesImport({ data, global: true, }); - const sources = (raw.sources ?? []) as SourceEntry[]; - return sources.map(toSkillInfo); + + return response.sources.filter(isSkillSource).map(toSkillInfo); } diff --git a/ui/goose2/src/features/skills/ui/CreateSkillDialog.tsx b/ui/goose2/src/features/skills/ui/CreateSkillDialog.tsx index fd81b91c0eac..0497ac61f15a 100644 --- a/ui/goose2/src/features/skills/ui/CreateSkillDialog.tsx +++ b/ui/goose2/src/features/skills/ui/CreateSkillDialog.tsx @@ -1,6 +1,5 @@ import { useState, useEffect } from "react"; import { useTranslation } from "react-i18next"; -import { cn } from "@/shared/lib/cn"; import { Button } from "@/shared/ui/button"; import { Input } from "@/shared/ui/input"; import { Label } from "@/shared/ui/label"; @@ -14,13 +13,56 @@ import { } from "@/shared/ui/dialog"; import { createSkill, updateSkill } from "../api/skills"; -const KEBAB_CASE_REGEX = /^[a-z0-9]+(-[a-z0-9]+)*$/; +const MAX_SKILL_NAME_LENGTH = 64; + +function isValidSkillName(name: string): boolean { + return ( + name.length > 0 && + name.length <= MAX_SKILL_NAME_LENGTH && + !name.startsWith("-") && + !name.endsWith("-") && + [...name].every( + (char) => + (char >= "a" && char <= "z") || + (char >= "0" && char <= "9") || + char === "-", + ) + ); +} + +function formatSkillName(raw: string): string { + return raw + .toLowerCase() + .replace(/[^a-z0-9-]/g, "-") + .replace(/^-/, "") + .slice(0, MAX_SKILL_NAME_LENGTH); +} + +function getRenamedSkillFileLocation( + fileLocation: string, + name: string, +): string { + const separator = fileLocation.includes("\\") ? "\\" : "/"; + const parts = fileLocation.split(separator); + + if (parts.length >= 2) { + parts[parts.length - 2] = name; + } + + return parts.join(separator); +} interface CreateSkillDialogProps { isOpen: boolean; onClose: () => void; onCreated?: () => void; - editingSkill?: { name: string; description: string; instructions: string }; + editingSkill?: { + name: string; + description: string; + instructions: string; + path: string; + fileLocation: string; + }; } export function CreateSkillDialog({ @@ -53,17 +95,11 @@ export function CreateSkillDialog({ } }, [isOpen, editingSkill]); - const nameValid = name.length > 0 && KEBAB_CASE_REGEX.test(name); + const nameValid = isValidSkillName(name); const canSave = nameValid && description.trim().length > 0 && !saving; const handleNameChange = (raw: string) => { - if (isEditing) return; // name is read-only in edit mode - const formatted = raw - .toLowerCase() - .replace(/[^a-z0-9-]/g, "-") - .replace(/-+/g, "-") - .replace(/^-/, ""); - setName(formatted); + setName(formatSkillName(raw)); setError(null); }; @@ -82,7 +118,12 @@ export function CreateSkillDialog({ setError(null); try { if (isEditing) { - await updateSkill(name, description.trim(), instructions); + await updateSkill( + editingSkill.path, + name, + description.trim(), + instructions, + ); } else { await createSkill(name, description.trim(), instructions); } @@ -121,8 +162,6 @@ export function CreateSkillDialog({ value={name} onChange={(e) => handleNameChange(e.target.value)} placeholder={t("dialog.namePlaceholder")} - readOnly={isEditing} - className={cn(isEditing && "opacity-60 cursor-not-allowed")} /> {name.length > 0 && !nameValid && (

@@ -147,6 +186,13 @@ export function CreateSkillDialog({ /> + {isEditing && editingSkill && ( +

+ {t("dialog.pathOnDisk")}:{" "} + {getRenamedSkillFileLocation(editingSkill.fileLocation, name)} +

+ )} + {/* Instructions */}