diff --git a/src/commands/skills.rs b/src/commands/skills.rs index 1ed527c..c7b2b3b 100644 --- a/src/commands/skills.rs +++ b/src/commands/skills.rs @@ -378,61 +378,44 @@ mod tests { } #[test] - fn install_bails_when_type_filter_matches_nothing_on_platform() { + fn install_pi_named_skill_succeeds() { + // Regression test for https://github.com/DataDog/pup/issues/562 — pi + // supports skills; installing dd-apm on pi must succeed. + let tmp = TempDir::new("install_pi_skill"); let cfg = base_cfg(); - // Skills don't apply to pi — should get the richer extension-only hint. - let err = install( - &cfg, - Some("pi".to_string()), - None, - None, - Some("skill".to_string()), - false, - ) - .unwrap_err() - .to_string(); - assert!(err.contains("no install target"), "got: {err}"); - assert!(err.contains("type=skill"), "got: {err}"); - assert!(err.contains("only support extensions"), "got: {err}"); - assert!(err.contains("dd-pup-pi"), "got: {err}"); - } - - #[test] - fn install_pi_named_skill_gives_extension_only_hint() { - let cfg = base_cfg(); - // dd-apm is a skill; pi only supports extensions — this is the exact - // scenario from the bug report (pup skills install --name dd-apm pi). - let err = install( + install( &cfg, Some("pi".to_string()), Some("dd-apm".to_string()), - None, + Some(tmp.path().to_str().unwrap().to_string()), None, false, ) - .unwrap_err() - .to_string(); - assert!(err.contains("no install target"), "got: {err}"); - assert!(err.contains("name=dd-apm"), "got: {err}"); - assert!(err.contains("only support extensions"), "got: {err}"); - assert!(err.contains("dd-pup-pi"), "got: {err}"); + .unwrap(); + assert!( + tmp.path().join("dd-apm/SKILL.md").exists(), + "dd-apm skill should be installed for pi" + ); } #[test] - fn install_pi_type_agent_gives_extension_only_hint() { + fn install_pi_type_skill_succeeds() { + let tmp = TempDir::new("install_pi_type_skill"); let cfg = base_cfg(); - let err = install( + install( &cfg, Some("pi".to_string()), None, - None, - Some("agent".to_string()), + Some(tmp.path().to_str().unwrap().to_string()), + Some("skill".to_string()), false, ) - .unwrap_err() - .to_string(); - assert!(err.contains("only support extensions"), "got: {err}"); - assert!(err.contains("dd-pup-pi"), "got: {err}"); + .unwrap(); + // At least one skill should have been written. + let any_skill = std::fs::read_dir(tmp.path()) + .unwrap() + .any(|e| e.unwrap().path().join("SKILL.md").exists()); + assert!(any_skill, "expected at least one SKILL.md installed for pi"); } #[test] diff --git a/src/skills.rs b/src/skills.rs index 0513be4..a3049e7 100644 --- a/src/skills.rs +++ b/src/skills.rs @@ -524,8 +524,8 @@ pub static SKILLS: &[SkillEntry] = &[ /// /// Each platform tells us where skills, agents, and extension bundles live for /// both project-local and user-global scopes. Empty path strings mean "not -/// supported" — e.g. `pi` has no skills/agents dirs, and most platforms have -/// no extensions dir. +/// supported" — e.g. a hypothetical future platform with no skills/agents dirs, +/// and most platforms have no extensions dir. pub struct PlatformSpec { /// Canonical platform name as users type it on the CLI. pub name: &'static str, @@ -554,7 +554,7 @@ impl PlatformSpec { /// /// An empty agents path means agents share the skills dir; if skills is /// also empty the platform has no text-content support and can only receive - /// extension bundles (e.g. pi). + /// extension bundles. pub fn is_extension_only(&self) -> bool { self.user_skills.is_empty() && self.project_skills.is_empty() @@ -634,8 +634,8 @@ pub static PLATFORMS: &[PlatformSpec] = &[ PlatformSpec { name: "pi", aliases: &["pi-dev"], - project_skills: "", - user_skills: "", + project_skills: ".pi/skills", + user_skills: ".pi/agent/skills", project_agents: "", user_agents: "", project_extensions: ".pi/extensions", @@ -841,7 +841,7 @@ fn resolve_relative( /// `/.md` for platforms with [`PlatformSpec::uses_agent_md`] /// (Claude Code subagent format), and `//SKILL.md` elsewhere. /// -/// Returns `None` when the platform has no skills/agents dir (e.g. `pi`). +/// Returns `None` when the platform has no skills/agents dir. /// Panics if called for an `extension` entry; use [`install_paths`] for those. pub fn install_path( entry: &SkillEntry, @@ -855,7 +855,7 @@ pub fn install_path( "install_path() does not handle extensions; use install_paths()" ); - // Extension-only platforms (e.g. pi) have no skills or agents directory; + // Extension-only platforms have no skills or agents directory; // skills and agents cannot install there even when --dir overrides the path. let spec = lookup_platform(platform)?; if spec.is_extension_only() { @@ -891,8 +891,8 @@ pub fn install_path( /// per bundled file. /// /// Returns `Ok(vec![])` (no-op) when the entry isn't applicable to the -/// platform (e.g. asking for a `pi` extension on `claude-code`, or asking for -/// a skill on `pi`). The caller can treat an empty result as "skip". +/// platform (e.g. asking for a `pi` extension on `claude-code`). +/// The caller can treat an empty result as "skip". pub fn install_paths( entry: &SkillEntry, platform: &str, @@ -1162,9 +1162,21 @@ mod tests { } #[test] - fn test_skills_dir_pi_returns_none() { + fn test_skills_dir_pi_project_scope() { let root = PathBuf::from("/tmp/test-project"); - assert_eq!(skills_dir_with_home("pi", None, &root, false), None); + assert_eq!( + skills_dir_with_home("pi", None, &root, false), + Some(root.join(".pi/skills")) + ); + } + + #[test] + fn test_skills_dir_pi_user_scope() { + let home = PathBuf::from("/tmp/fake-home"); + assert_eq!( + skills_dir_with_home("pi", Some(&home), &PathBuf::from("/unused"), true), + Some(home.join(".pi/agent/skills")) + ); } #[test] @@ -1257,19 +1269,21 @@ mod tests { } #[test] - fn test_install_path_skill_on_pi_returns_none() { + fn test_install_path_skill_on_pi() { let root = PathBuf::from("/tmp/test-project"); let e = entry("dd-pup", "skill", ""); - assert!(install_path(&e, "pi", &root, None, false).is_none()); + let (path, fmt) = install_path(&e, "pi", &root, None, false).unwrap(); + assert_eq!(path, root.join(".pi/skills/dd-pup/SKILL.md")); + assert_eq!(fmt, InstallFormat::SkillMd); } #[test] - fn test_install_path_skill_on_pi_with_dir_override_returns_none() { - // --dir does not grant an extension-only platform the ability to install - // skills; the platform capability check runs before the dir override. + fn test_install_path_skill_on_pi_with_dir_override() { let root = PathBuf::from("/tmp/test-project"); let e = entry("dd-pup", "skill", ""); - assert!(install_path(&e, "pi", &root, Some("/tmp/out"), false).is_none()); + let (path, fmt) = install_path(&e, "pi", &root, Some("/tmp/out"), false).unwrap(); + assert_eq!(path, PathBuf::from("/tmp/out/dd-pup/SKILL.md")); + assert_eq!(fmt, InstallFormat::SkillMd); } #[test] @@ -1497,11 +1511,12 @@ mod tests { } #[test] - fn test_install_paths_skill_on_pi_is_empty() { + fn test_install_paths_skill_on_pi() { let root = PathBuf::from("/tmp/proj"); let e = entry("dd-pup", "skill", "body"); let paths = install_paths(&e, "pi", &root, None, false).unwrap(); - assert!(paths.is_empty()); + assert_eq!(paths.len(), 1); + assert_eq!(paths[0].0, root.join(".pi/skills/dd-pup/SKILL.md")); } #[test] @@ -1587,8 +1602,8 @@ mod tests { } #[test] - fn test_is_extension_only_pi() { - assert!(lookup_platform("pi").unwrap().is_extension_only()); + fn test_pi_is_not_extension_only() { + assert!(!lookup_platform("pi").unwrap().is_extension_only()); } #[test]