From f91970f092a1790b31a5bce817d92ca9269b3887 Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Fri, 8 May 2026 02:37:21 -0500 Subject: [PATCH] fix(skills): accept workflow pack archive layouts (#1164) Teach /skill install to recognize compatible skill directories such as .claude/skills//SKILL.md, nested packages/.../skills//SKILL.md, and single nested skill repos while still extracting only the selected subtree. Also make /init treat an existing AGENTS.md as an idempotent no-op so the TUI matches the dispatcher behavior instead of surfacing a scary error for an already-initialized project. --- crates/tui/src/commands/init.rs | 11 ++- crates/tui/src/skills/install.rs | 90 +++++++++++++++++------- crates/tui/tests/skill_install.rs | 112 ++++++++++++++++++++++++++++++ 3 files changed, 186 insertions(+), 27 deletions(-) diff --git a/crates/tui/src/commands/init.rs b/crates/tui/src/commands/init.rs index bc2409ea..06ea14d0 100644 --- a/crates/tui/src/commands/init.rs +++ b/crates/tui/src/commands/init.rs @@ -14,7 +14,10 @@ pub fn init(app: &mut App) -> CommandResult { // Check if AGENTS.md already exists let agents_path = workspace.join("AGENTS.md"); if agents_path.exists() { - return CommandResult::error("AGENTS.md already exists. Delete it first to reinitialize."); + return CommandResult::message(format!( + "AGENTS.md already exists at {}. Delete it first to reinitialize.", + agents_path.display() + )); } // Detect project type and generate appropriate content @@ -197,12 +200,16 @@ mod tests { } #[test] - fn test_init_fails_if_exists() { + fn test_init_is_noop_if_exists() { let tmpdir = TempDir::new().unwrap(); let mut app = create_test_app_with_tmpdir(&tmpdir); // Create file first std::fs::write(tmpdir.path().join("AGENTS.md"), "existing").unwrap(); let result = init(&mut app); + assert!( + !result.is_error, + "existing AGENTS.md is an idempotent no-op, not an error" + ); assert!(result.message.is_some()); assert!(result.message.unwrap().contains("already exists")); } diff --git a/crates/tui/src/skills/install.rs b/crates/tui/src/skills/install.rs index 3303f3cf..19d51652 100644 --- a/crates/tui/src/skills/install.rs +++ b/crates/tui/src/skills/install.rs @@ -1066,7 +1066,7 @@ fn scan_tarball(bytes: &[u8], max_size: u64) -> Result { let mut total_size: u64 = 0; let mut prefix: Option = None; - let mut skill_md_relative: Option<(String, Vec)> = None; + let mut skill_md_relative: Option<(SkillMdCandidate, Vec)> = None; let mut link_paths: Vec = Vec::new(); for entry in archive @@ -1122,46 +1122,38 @@ fn scan_tarball(bytes: &[u8], max_size: u64) -> Result { continue; } - // SKILL.md detection. Match either: + // SKILL.md detection. Match the same workflow layouts that runtime + // discovery understands: // * `/SKILL.md` - // * `/skills//SKILL.md` + // * `/*/skills//SKILL.md` + // * `//SKILL.md` if entry_type.is_file() { let stripped = strip_prefix(&path_str, prefix.as_deref().unwrap_or("")); - if stripped.eq_ignore_ascii_case("SKILL.md") - || stripped.starts_with("skills/") - && stripped.ends_with("/SKILL.md") - && stripped.matches('/').count() == 2 - { + if let Some(candidate) = skill_md_candidate(&stripped) { let mut buf = Vec::new(); entry .read_to_end(&mut buf) .context("failed to read SKILL.md from archive")?; - // Prefer the first match — we don't support multi-skill - // archives where a tarball ships several SKILL.mds at once. - if skill_md_relative.is_none() { - skill_md_relative = Some((stripped.to_string(), buf)); + // Prefer the most explicit match: repo-root SKILL.md first, + // then known skill-directory layouts, then a single nested + // `/SKILL.md` repository. + let replace = skill_md_relative + .as_ref() + .is_none_or(|(current, _)| candidate.rank < current.rank); + if replace { + skill_md_relative = Some((candidate, buf)); } } } } let prefix = prefix.unwrap_or_default(); - let (skill_md_path, skill_md_bytes) = skill_md_relative + let (skill_md, skill_md_bytes) = skill_md_relative .ok_or(InstallError::MissingSkillMd) .map_err(anyhow::Error::from)?; - let skill_root = if skill_md_path == "SKILL.md" { - String::new() - } else { - // strip trailing /SKILL.md - skill_md_path - .strip_suffix("/SKILL.md") - .unwrap_or("") - .to_string() - }; - for link_path in link_paths { - if is_within_selected_root(&link_path, &prefix, &skill_root) { + if is_within_selected_root(&link_path, &prefix, &skill_md.skill_root) { return Err(InstallError::SymlinkRejected.into()); } } @@ -1174,10 +1166,58 @@ fn scan_tarball(bytes: &[u8], max_size: u64) -> Result { Ok(TarballScan { skill_name: name, prefix, - skill_root, + skill_root: skill_md.skill_root, }) } +struct SkillMdCandidate { + rank: u8, + skill_root: String, +} + +fn skill_md_candidate(stripped_path: &str) -> Option { + if stripped_path.eq_ignore_ascii_case("SKILL.md") { + return Some(SkillMdCandidate { + rank: 0, + skill_root: String::new(), + }); + } + + let parts: Vec<&str> = stripped_path.split('/').collect(); + if parts + .last() + .is_none_or(|last| !last.eq_ignore_ascii_case("SKILL.md")) + { + return None; + } + + // Common workflow-pack layouts: + // `skills//SKILL.md`, `.agents/skills//SKILL.md`, + // `.claude/skills//SKILL.md`, and nested package layouts such as + // `packages/foo/skills//SKILL.md`. + if parts.len() >= 3 { + let container = parts[parts.len() - 3]; + let name = parts[parts.len() - 2]; + if container.eq_ignore_ascii_case("skills") && !name.is_empty() { + return Some(SkillMdCandidate { + rank: 1, + skill_root: parts[..parts.len() - 1].join("/"), + }); + } + } + + // Single-skill repos sometimes keep their root tidy with + // `/SKILL.md` plus sibling docs at repo root. + if parts.len() == 2 && !parts[0].is_empty() { + return Some(SkillMdCandidate { + rank: 2, + skill_root: parts[0].to_string(), + }); + } + + None +} + fn extract_into(scan: &TarballScan, bytes: &[u8], dest: &Path, max_size: u64) -> Result<()> { let cursor = std::io::Cursor::new(bytes); let gz = GzDecoder::new(cursor); diff --git a/crates/tui/tests/skill_install.rs b/crates/tui/tests/skill_install.rs index e4578cdf..f30cd559 100644 --- a/crates/tui/tests/skill_install.rs +++ b/crates/tui/tests/skill_install.rs @@ -287,6 +287,118 @@ async fn install_rejects_missing_skill_md() { shutdown(tx, handle); } +#[tokio::test] +async fn install_accepts_claude_compatible_skill_directory_archive() { + let tarball = make_tarball(&[ + ( + "repo-main/.claude/skills/workflow-pack/SKILL.md", + &skill_md("workflow-pack", "Workflow pack"), + ), + ( + "repo-main/.claude/skills/workflow-pack/scripts/check.sh", + b"echo ok", + ), + ("repo-main/README.md", b"outside the selected skill subtree"), + ]); + let (url, tx, handle) = spawn_tarball_server(tarball); + + let tmp = TempDir::new().unwrap(); + let policy = allow_all_policy(); + let outcome = install::install( + InstallSource::DirectUrl(url), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect("claude-compatible skill dir should install"); + let installed = match outcome { + InstallOutcome::Installed(installed) => installed, + other => panic!("expected Installed, got {other:?}"), + }; + + assert_eq!(installed.name, "workflow-pack"); + assert!(installed.path.join("SKILL.md").is_file()); + assert!(installed.path.join("scripts/check.sh").is_file()); + assert!(!installed.path.join("README.md").exists()); + + shutdown(tx, handle); +} + +#[tokio::test] +async fn install_accepts_nested_workflow_pack_skill_directory() { + let tarball = make_tarball(&[ + ( + "repo-main/packages/superpowers/5.1.0/skills/using-superpowers/SKILL.md", + &skill_md("using-superpowers", "Use Superpowers workflow"), + ), + ( + "repo-main/packages/superpowers/5.1.0/skills/using-superpowers/references/guide.md", + b"guide", + ), + ]); + let (url, tx, handle) = spawn_tarball_server(tarball); + + let tmp = TempDir::new().unwrap(); + let policy = allow_all_policy(); + let outcome = install::install( + InstallSource::DirectUrl(url), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect("nested workflow-pack skill dir should install"); + let installed = match outcome { + InstallOutcome::Installed(installed) => installed, + other => panic!("expected Installed, got {other:?}"), + }; + + assert_eq!(installed.name, "using-superpowers"); + assert!(installed.path.join("SKILL.md").is_file()); + assert!(installed.path.join("references/guide.md").is_file()); + + shutdown(tx, handle); +} + +#[tokio::test] +async fn install_accepts_single_skill_subdirectory_archive() { + let tarball = make_tarball(&[ + ( + "repo-main/my-workflow/SKILL.md", + &skill_md("my-workflow", "Nested workflow"), + ), + ("repo-main/my-workflow/examples/example.md", b"example"), + ("repo-main/README.md", b"outside the selected skill subtree"), + ]); + let (url, tx, handle) = spawn_tarball_server(tarball); + + let tmp = TempDir::new().unwrap(); + let policy = allow_all_policy(); + let outcome = install::install( + InstallSource::DirectUrl(url), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect("single nested skill dir should install"); + let installed = match outcome { + InstallOutcome::Installed(installed) => installed, + other => panic!("expected Installed, got {other:?}"), + }; + + assert_eq!(installed.name, "my-workflow"); + assert!(installed.path.join("SKILL.md").is_file()); + assert!(installed.path.join("examples/example.md").is_file()); + assert!(!installed.path.join("README.md").exists()); + + shutdown(tx, handle); +} + #[tokio::test] async fn install_rejects_missing_required_frontmatter() { let tarball = make_tarball(&[("repo-main/SKILL.md", b"---\nname: test\n---\nbody\n")]);