From c1d8e27e739e6ed09b07974b6171d322501f3050 Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Mon, 9 Mar 2026 18:03:52 -0400 Subject: [PATCH] feat(summon): make skill supporting files individually loadable via load() (#7583) --- .../src/agents/platform_extensions/summon.rs | 337 +++++++++++++++++- ui/desktop/package-lock.json | 28 +- 2 files changed, 329 insertions(+), 36 deletions(-) diff --git a/crates/goose/src/agents/platform_extensions/summon.rs b/crates/goose/src/agents/platform_extensions/summon.rs index f63abca1..395290ae 100644 --- a/crates/goose/src/agents/platform_extensions/summon.rs +++ b/crates/goose/src/agents/platform_extensions/summon.rs @@ -169,6 +169,14 @@ fn parse_frontmatter Deserialize<'de>>(content: &str) -> Option<(T, fn parse_skill_content(content: &str, path: PathBuf) -> Option { let (metadata, body): (SkillMetadata, String) = parse_frontmatter(content)?; + if metadata.name.contains('/') { + warn!( + "Skill name '{}' contains '/' which is not allowed, skipping", + metadata.name + ); + return None; + } + Some(Source { name: metadata.name, kind: SourceKind::Skill, @@ -693,15 +701,58 @@ impl SummonClient { session_id: &str, name: &str, working_dir: &Path, - ) -> Option { + ) -> Result, String> { let sources = self.get_sources(session_id, working_dir).await; - let mut source = sources.into_iter().find(|s| s.name == name)?; - if source.kind == SourceKind::Subrecipe && source.content.is_empty() { - source.content = self.load_subrecipe_content(session_id, &source.name).await; + if let Some(mut source) = sources.iter().find(|s| s.name == name).cloned() { + if source.kind == SourceKind::Subrecipe && source.content.is_empty() { + source.content = self.load_subrecipe_content(session_id, &source.name).await; + } + return Ok(Some(source)); } - Some(source) + if let Some((skill_name, raw_relative_path)) = name.split_once('/') { + let relative_path = raw_relative_path.replace('\\', "/"); + if let Some(skill) = sources.iter().find(|s| { + s.name == skill_name + && matches!(s.kind, SourceKind::Skill | SourceKind::BuiltinSkill) + }) { + let canonical_skill_dir = skill + .path + .canonicalize() + .unwrap_or_else(|_| skill.path.clone()); + + for file_path in &skill.supporting_files { + if let Ok(rel) = file_path.strip_prefix(&skill.path) { + let rel_normalized = rel.to_string_lossy().replace('\\', "/"); + if rel_normalized == relative_path { + let canonical_file = file_path + .canonicalize() + .map_err(|e| format!("Failed to resolve '{}': {}", name, e))?; + if !canonical_file.starts_with(&canonical_skill_dir) { + return Err(format!( + "Refusing to load '{}': file resolves outside the skill directory", + name + )); + } + return match std::fs::read_to_string(&canonical_file) { + Ok(content) => Ok(Some(Source { + name: name.to_string(), + kind: SourceKind::Skill, + description: format!("Supporting file for {}", skill_name), + path: file_path.clone(), + content, + supporting_files: vec![], + })), + Err(e) => Err(format!("Failed to read '{}': {}", name, e)), + }; + } + } + } + } + } + + Ok(None) } async fn load_subrecipe_content(&self, session_id: &str, name: &str) -> String { @@ -1041,7 +1092,7 @@ impl SummonClient { name: &str, working_dir: &Path, ) -> Result, String> { - let source = self.resolve_source(session_id, name, working_dir).await; + let source = self.resolve_source(session_id, name, working_dir).await?; match source { Some(source) => { @@ -1059,11 +1110,15 @@ impl SummonClient { )); for file in &source.supporting_files { if let Ok(relative) = file.strip_prefix(&source.path) { - output.push_str(&format!("- {}\n", relative.display())); + let rel_str = relative.to_string_lossy().replace('\\', "/"); + output.push_str(&format!( + "- {} → load(source: \"{}/{}\")\n", + rel_str, source.name, rel_str + )); } } output.push_str( - "\nUse the file tools to read these files or run scripts as directed.\n", + "\nUse load(source: \"/\") to load individual files into context, or use file tools to read/run them directly.\n", ); } @@ -1073,6 +1128,42 @@ impl SummonClient { } None => { let sources = self.get_sources(session_id, working_dir).await; + + if let Some((skill_name, _)) = name.split_once('/') { + if let Some(skill) = sources.iter().find(|s| { + s.name == skill_name + && matches!(s.kind, SourceKind::Skill | SourceKind::BuiltinSkill) + }) { + let available: Vec = skill + .supporting_files + .iter() + .filter_map(|f| { + f.strip_prefix(&skill.path) + .ok() + .map(|r| r.to_string_lossy().replace('\\', "/")) + }) + .collect(); + if !available.is_empty() { + let total = available.len(); + let display: Vec<_> = available.into_iter().take(10).collect(); + let suffix = if total > 10 { + format!(" (and {} more)", total - 10) + } else { + String::new() + }; + return Err(format!( + "Source '{}' not found. Available files for {}: {}{}", + name, + skill_name, + display.join(", "), + suffix + )); + } else { + return Err(format!("Skill '{}' has no supporting files.", skill_name)); + } + } + } + let suggestions: Vec<&str> = sources .iter() .filter(|s| { @@ -1244,9 +1335,18 @@ impl SummonClient { ) -> Result { let source = self .resolve_source(session_id, source_name, working_dir) - .await + .await? .ok_or_else(|| format!("Source '{}' not found", source_name))?; + if source_name.contains('/') + && matches!(source.kind, SourceKind::Skill | SourceKind::BuiltinSkill) + { + return Err(format!( + "Cannot delegate to supporting file '{}'. Use load() to read it instead.", + source_name + )); + } + let mut recipe = match source.kind { SourceKind::Recipe | SourceKind::Subrecipe => { self.build_recipe_from_source(&source, params, session_id) @@ -1922,6 +2022,225 @@ You review code."#; assert!(file_names.contains(&"report.txt".to_string())); } + #[tokio::test] + async fn test_load_source_lists_supporting_files_with_load_names() { + let temp_dir = TempDir::new().unwrap(); + + let skill_dir = temp_dir.path().join(".goose/skills/my-skill"); + fs::create_dir_all(skill_dir.join("references")).unwrap(); + fs::write( + skill_dir.join("SKILL.md"), + "---\nname: my-skill\ndescription: A skill\n---\nSee references.", + ) + .unwrap(); + fs::write( + skill_dir.join("references/ops.md"), + "# Ops Guide\n\nDo the thing.", + ) + .unwrap(); + fs::write(skill_dir.join("run.sh"), "#!/bin/bash\necho ok").unwrap(); + + let client = SummonClient::new(create_test_context()).unwrap(); + let result = client + .handle_load_source("test", "my-skill", temp_dir.path()) + .await + .unwrap(); + + let text = &result[0].as_text().expect("expected text content").text; + + assert!( + !text.contains("Ops Guide"), + "md file content should not be inlined" + ); + assert!( + !text.contains("Do the thing."), + "md file content should not be inlined" + ); + assert!( + !text.contains("#!/bin/bash"), + "script content should not be inlined" + ); + assert!( + text.contains("load(source: \"my-skill/references/ops.md\")"), + "md file should be listed with load() name" + ); + assert!( + text.contains("load(source: \"my-skill/run.sh\")"), + "script should be listed with load() name" + ); + assert!( + text.contains("load(source: \"/\")"), + "should include usage hint" + ); + } + + #[tokio::test] + async fn test_load_supporting_file_by_path() { + let temp_dir = TempDir::new().unwrap(); + + let skill_dir = temp_dir.path().join(".goose/skills/my-skill"); + fs::create_dir_all(skill_dir.join("references")).unwrap(); + fs::write( + skill_dir.join("SKILL.md"), + "---\nname: my-skill\ndescription: A skill\n---\nSee references.", + ) + .unwrap(); + fs::write( + skill_dir.join("references/ops.md"), + "# Ops Guide\n\nDo the thing.", + ) + .unwrap(); + fs::write(skill_dir.join("run.sh"), "#!/bin/bash\necho ok").unwrap(); + + let client = SummonClient::new(create_test_context()).unwrap(); + + let md_result = client + .handle_load_source("test", "my-skill/references/ops.md", temp_dir.path()) + .await + .unwrap(); + let md_text = &md_result[0].as_text().expect("expected text content").text; + assert!( + md_text.contains("Ops Guide"), + "markdown content should be loaded" + ); + assert!(md_text.contains("Do the thing.")); + + let sh_result = client + .handle_load_source("test", "my-skill/run.sh", temp_dir.path()) + .await + .unwrap(); + let sh_text = &sh_result[0].as_text().expect("expected text content").text; + assert!( + sh_text.contains("#!/bin/bash"), + "script content should be loaded" + ); + } + + #[tokio::test] + async fn test_load_supporting_file_not_found_suggests_available() { + let temp_dir = TempDir::new().unwrap(); + + let skill_dir = temp_dir.path().join(".goose/skills/my-skill"); + fs::create_dir_all(skill_dir.join("references")).unwrap(); + fs::write( + skill_dir.join("SKILL.md"), + "---\nname: my-skill\ndescription: A skill\n---\nSee references.", + ) + .unwrap(); + fs::write( + skill_dir.join("references/ops.md"), + "# Ops Guide\n\nDo the thing.", + ) + .unwrap(); + + let client = SummonClient::new(create_test_context()).unwrap(); + let err = client + .handle_load_source( + "test", + "my-skill/references/nonexistent.md", + temp_dir.path(), + ) + .await + .unwrap_err(); + + assert!( + err.contains("references/ops.md"), + "error should list available files: {}", + err + ); + assert!( + err.contains("my-skill"), + "error should name the skill: {}", + err + ); + } + + #[cfg(unix)] + #[tokio::test] + async fn test_resolve_source_blocks_symlink_outside_skill_dir() { + let temp_dir = TempDir::new().unwrap(); + let outside_dir = TempDir::new().unwrap(); + + let skill_dir = temp_dir.path().join(".goose/skills/my-skill"); + fs::create_dir_all(&skill_dir).unwrap(); + fs::write( + skill_dir.join("SKILL.md"), + "---\nname: my-skill\ndescription: A skill\n---\nContent.", + ) + .unwrap(); + + let secret_file = outside_dir.path().join("secret.txt"); + fs::write(&secret_file, "top secret data").unwrap(); + std::os::unix::fs::symlink(&secret_file, skill_dir.join("evil.md")).unwrap(); + + let client = SummonClient::new(create_test_context()).unwrap(); + let result = client + .handle_load_source("test", "my-skill/evil.md", temp_dir.path()) + .await; + + assert!( + result.is_err(), + "symlink outside skill dir should be blocked" + ); + let err = result.unwrap_err(); + assert!( + err.contains("resolves outside the skill directory"), + "error should mention path traversal: {}", + err + ); + } + + #[tokio::test] + async fn test_resolve_source_blocks_path_traversal_input() { + let temp_dir = TempDir::new().unwrap(); + + let skill_dir = temp_dir.path().join(".goose/skills/my-skill"); + fs::create_dir_all(&skill_dir).unwrap(); + fs::write( + skill_dir.join("SKILL.md"), + "---\nname: my-skill\ndescription: A skill\n---\nContent.", + ) + .unwrap(); + fs::write(skill_dir.join("legit.md"), "legit content").unwrap(); + + let client = SummonClient::new(create_test_context()).unwrap(); + + // ../../../etc/passwd won't match any supporting_files entry, so it returns Ok (not found) + // which becomes the "not found" error path in handle_load_source + let result = client + .handle_load_source("test", "my-skill/../../../etc/passwd", temp_dir.path()) + .await; + + assert!(result.is_err(), "traversal path should not load content"); + let err = result.unwrap_err(); + assert!( + !err.contains("root:"), + "should not contain /etc/passwd content: {}", + err + ); + } + + #[tokio::test] + async fn test_skill_name_with_slash_is_rejected() { + let temp_dir = TempDir::new().unwrap(); + + let skill_dir = temp_dir.path().join(".goose/skills/bad-skill"); + fs::create_dir_all(&skill_dir).unwrap(); + fs::write( + skill_dir.join("SKILL.md"), + "---\nname: bad/skill\ndescription: A skill with slash\n---\nContent.", + ) + .unwrap(); + + let client = SummonClient::new(create_test_context()).unwrap(); + let sources = client.get_sources("test", temp_dir.path()).await; + + assert!( + !sources.iter().any(|s| s.name == "bad/skill"), + "skill with '/' in name should be rejected" + ); + } + #[tokio::test] async fn test_client_tools_and_unknown_tool() { let client = SummonClient::new(create_test_context()).unwrap(); diff --git a/ui/desktop/package-lock.json b/ui/desktop/package-lock.json index 7ea56dbf..294d05ec 100644 --- a/ui/desktop/package-lock.json +++ b/ui/desktop/package-lock.json @@ -123,7 +123,7 @@ } }, "../acp": { - "name": "goose-acp-types", + "name": "@block/goose-acp", "version": "0.1.0", "dependencies": { "zod": "^3.25.76" @@ -10330,32 +10330,6 @@ "node": ">= 0.8" } }, - "node_modules/encoding": { - "version": "0.1.13", - "resolved": "https://registry.npmjs.org/encoding/-/encoding-0.1.13.tgz", - "integrity": "sha512-ETBauow1T35Y/WZMkio9jiM0Z5xjHHmJ4XmjZOq1l/dXz3lr2sRn87nJy20RupqSh1F2m3HHPSp8ShIPQJrJ3A==", - "dev": true, - "license": "MIT", - "optional": true, - "peer": true, - "dependencies": { - "iconv-lite": "^0.6.2" - } - }, - "node_modules/encoding/node_modules/iconv-lite": { - "version": "0.6.3", - "resolved": "https://registry.npmjs.org/iconv-lite/-/iconv-lite-0.6.3.tgz", - "integrity": "sha512-4fCk79wshMdzMp2rH06qWrJE4iolqLhCUH+OiuIgU++RB0+94NlDL81atO7GX55uUKueo0txHNtvEyI6D7WdMw==", - "dev": true, - "license": "MIT", - "optional": true, - "dependencies": { - "safer-buffer": ">= 2.1.2 < 3.0.0" - }, - "engines": { - "node": ">=0.10.0" - } - }, "node_modules/end-of-stream": { "version": "1.4.5", "resolved": "https://registry.npmjs.org/end-of-stream/-/end-of-stream-1.4.5.tgz",