From 80726511de0baaf75d9a922bfa010b0699d7659e Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Mon, 16 Feb 2026 21:02:55 +0000 Subject: [PATCH 1/2] Use directory copy instead of symlinks for skills on Windows On Windows, creating symlinks requires Administrator privileges. Update the skills installer to copy skill directories on Windows instead of using symlinks, while keeping symlinks on Unix. The uninstall logic is also updated to handle both symlinks (for existing Unix installs) and copied directories (for Windows). Closes #533 Co-Authored-By: Sasha Varlamov --- src/mdm/skills_installer.rs | 81 +++++++++++++++++++++---------------- 1 file changed, 47 insertions(+), 34 deletions(-) diff --git a/src/mdm/skills_installer.rs b/src/mdm/skills_installer.rs index c58622f93..4184d9226 100644 --- a/src/mdm/skills_installer.rs +++ b/src/mdm/skills_installer.rs @@ -41,14 +41,16 @@ fn claude_skills_dir() -> Option { dirs::home_dir().map(|h| h.join(".claude").join("skills")) } -/// Create a symlink from link_path to target, removing any existing file/symlink first -fn create_skills_symlink(target: &PathBuf, link_path: &PathBuf) -> Result<(), GitAiError> { +/// Link a skill directory to the target location. +/// On Unix, creates a symlink. On Windows, copies the directory to avoid requiring +/// Administrator privileges (which symlink creation requires on Windows). +fn link_skill_dir(target: &PathBuf, link_path: &PathBuf) -> Result<(), GitAiError> { // Create parent directory if needed if let Some(parent) = link_path.parent() { fs::create_dir_all(parent)?; } - // Remove existing file/symlink if present + // Remove existing file/symlink/directory if present if link_path.exists() || link_path.symlink_metadata().is_ok() { if link_path.is_dir() && !link_path @@ -56,33 +58,50 @@ fn create_skills_symlink(target: &PathBuf, link_path: &PathBuf) -> Result<(), Gi .map(|m| m.file_type().is_symlink()) .unwrap_or(false) { - // It's a real directory, not a symlink - remove it fs::remove_dir_all(link_path)?; } else { - // It's a file or symlink fs::remove_file(link_path)?; } } - // Create the symlink #[cfg(unix)] std::os::unix::fs::symlink(target, link_path)?; #[cfg(windows)] - std::os::windows::fs::symlink_dir(target, link_path)?; + copy_dir_recursive(target, link_path)?; Ok(()) } -/// Remove a symlink if it exists -fn remove_skills_symlink(link_path: &PathBuf) -> Result<(), GitAiError> { - if link_path.symlink_metadata().is_ok() - && link_path +/// Recursively copy a directory and its contents from src to dst. +#[cfg(windows)] +fn copy_dir_recursive(src: &PathBuf, dst: &PathBuf) -> Result<(), GitAiError> { + fs::create_dir_all(dst)?; + for entry in fs::read_dir(src)? { + let entry = entry?; + let entry_path = entry.path(); + let dest_path = dst.join(entry.file_name()); + if entry_path.is_dir() { + copy_dir_recursive(&entry_path, &dest_path)?; + } else { + fs::copy(&entry_path, &dest_path)?; + } + } + Ok(()) +} + +/// Remove a skill link (symlink on Unix, copied directory on Windows) if it exists. +fn remove_skill_link(link_path: &PathBuf) -> Result<(), GitAiError> { + if link_path.symlink_metadata().is_ok() { + let is_symlink = link_path .symlink_metadata() .map(|m| m.file_type().is_symlink()) - .unwrap_or(false) - { - fs::remove_file(link_path)?; + .unwrap_or(false); + if is_symlink { + fs::remove_file(link_path)?; + } else if link_path.is_dir() { + fs::remove_dir_all(link_path)?; + } } Ok(()) } @@ -95,9 +114,9 @@ fn remove_skills_symlink(link_path: &PathBuf) -> Result<(), GitAiError> { /// └── prompt-analysis/ /// └── SKILL.md /// -/// Then symlinks each skill to: -/// - ~/.agents/skills/{skill-name} -/// - ~/.claude/skills/{skill-name} +/// Then links each skill to: +/// - ~/.agents/skills/{skill-name} (symlink on Unix, copy on Windows) +/// - ~/.claude/skills/{skill-name} (symlink on Unix, copy on Windows) pub fn install_skills(dry_run: bool, _verbose: bool) -> Result { let skills_base = skills_dir_path().ok_or_else(|| { GitAiError::Generic("Could not determine skills directory path".to_string()) @@ -128,26 +147,20 @@ pub fn install_skills(dry_run: bool, _verbose: bool) -> Result ~/.git-ai/skills/{skill-name} if let Some(agents_dir) = agents_skills_dir() { let agents_link = agents_dir.join(skill.name); - if let Err(e) = create_skills_symlink(&skill_dir, &agents_link) { - eprintln!( - "Warning: Failed to create symlink at {:?}: {}", - agents_link, e - ); + if let Err(e) = link_skill_dir(&skill_dir, &agents_link) { + eprintln!("Warning: Failed to link skill at {:?}: {}", agents_link, e); } } // ~/.claude/skills/{skill-name} -> ~/.git-ai/skills/{skill-name} if let Some(claude_dir) = claude_skills_dir() { let claude_link = claude_dir.join(skill.name); - if let Err(e) = create_skills_symlink(&skill_dir, &claude_link) { - eprintln!( - "Warning: Failed to create symlink at {:?}: {}", - claude_link, e - ); + if let Err(e) = link_skill_dir(&skill_dir, &claude_link) { + eprintln!("Warning: Failed to link skill at {:?}: {}", claude_link, e); } } } @@ -158,7 +171,7 @@ pub fn install_skills(dry_run: bool, _verbose: bool) -> Result Result { let skills_base = skills_dir_path().ok_or_else(|| { GitAiError::Generic("Could not determine skills directory path".to_string()) @@ -178,14 +191,14 @@ pub fn uninstall_skills(dry_run: bool, _verbose: bool) -> Result Result Date: Mon, 16 Feb 2026 21:48:00 +0000 Subject: [PATCH 2/2] Add unit tests for skills installation and linking logic Co-Authored-By: Sasha Varlamov --- src/mdm/skills_installer.rs | 160 +++++++++++++++++++++++++++++++++++- 1 file changed, 159 insertions(+), 1 deletion(-) diff --git a/src/mdm/skills_installer.rs b/src/mdm/skills_installer.rs index 4184d9226..d3bee9be7 100644 --- a/src/mdm/skills_installer.rs +++ b/src/mdm/skills_installer.rs @@ -231,7 +231,6 @@ mod tests { #[test] fn test_embedded_skills_are_loaded() { - // Verify that the embedded skills are not empty for skill in EMBEDDED_SKILLS { assert!(!skill.name.is_empty(), "Skill name should not be empty"); assert!( @@ -255,4 +254,163 @@ mod tests { assert!(parent.ends_with(".git-ai")); } } + + #[test] + fn test_link_skill_dir_creates_link_and_content_is_accessible() { + let tmp = tempfile::tempdir().unwrap(); + let source = tmp.path().join("source-skill"); + fs::create_dir_all(&source).unwrap(); + fs::write(source.join("SKILL.md"), "test content").unwrap(); + + let link = tmp.path().join("linked-skill"); + link_skill_dir(&source, &link).unwrap(); + + assert!(link.exists()); + assert!(link.join("SKILL.md").exists()); + assert_eq!( + fs::read_to_string(link.join("SKILL.md")).unwrap(), + "test content" + ); + } + + #[test] + fn test_link_skill_dir_replaces_existing_directory() { + let tmp = tempfile::tempdir().unwrap(); + let source = tmp.path().join("source-skill"); + fs::create_dir_all(&source).unwrap(); + fs::write(source.join("SKILL.md"), "new content").unwrap(); + + let link = tmp.path().join("linked-skill"); + fs::create_dir_all(&link).unwrap(); + fs::write(link.join("SKILL.md"), "old content").unwrap(); + + link_skill_dir(&source, &link).unwrap(); + + assert_eq!( + fs::read_to_string(link.join("SKILL.md")).unwrap(), + "new content" + ); + } + + #[test] + fn test_link_skill_dir_replaces_existing_file() { + let tmp = tempfile::tempdir().unwrap(); + let source = tmp.path().join("source-skill"); + fs::create_dir_all(&source).unwrap(); + fs::write(source.join("SKILL.md"), "content").unwrap(); + + let link = tmp.path().join("linked-skill"); + fs::write(&link, "i am a file").unwrap(); + + link_skill_dir(&source, &link).unwrap(); + + assert!(link.is_dir() || link.is_symlink()); + assert!(link.join("SKILL.md").exists()); + } + + #[test] + fn test_link_skill_dir_creates_parent_directories() { + let tmp = tempfile::tempdir().unwrap(); + let source = tmp.path().join("source-skill"); + fs::create_dir_all(&source).unwrap(); + fs::write(source.join("SKILL.md"), "content").unwrap(); + + let link = tmp.path().join("deep").join("nested").join("linked-skill"); + link_skill_dir(&source, &link).unwrap(); + + assert!(link.exists()); + assert!(link.join("SKILL.md").exists()); + } + + #[cfg(unix)] + #[test] + fn test_link_skill_dir_creates_symlink_on_unix() { + let tmp = tempfile::tempdir().unwrap(); + let source = tmp.path().join("source-skill"); + fs::create_dir_all(&source).unwrap(); + fs::write(source.join("SKILL.md"), "content").unwrap(); + + let link = tmp.path().join("linked-skill"); + link_skill_dir(&source, &link).unwrap(); + + assert!(link.symlink_metadata().unwrap().file_type().is_symlink()); + assert_eq!(fs::read_link(&link).unwrap(), source); + } + + #[test] + fn test_remove_skill_link_removes_directory() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().join("skill-dir"); + fs::create_dir_all(&dir).unwrap(); + fs::write(dir.join("SKILL.md"), "content").unwrap(); + + remove_skill_link(&dir).unwrap(); + assert!(!dir.exists()); + } + + #[cfg(unix)] + #[test] + fn test_remove_skill_link_removes_symlink() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("target"); + fs::create_dir_all(&target).unwrap(); + + let link = tmp.path().join("link"); + std::os::unix::fs::symlink(&target, &link).unwrap(); + + remove_skill_link(&link).unwrap(); + assert!(!link.symlink_metadata().is_ok()); + assert!(target.exists(), "original target should not be removed"); + } + + #[test] + fn test_remove_skill_link_noop_on_nonexistent_path() { + let tmp = tempfile::tempdir().unwrap(); + let nonexistent = tmp.path().join("does-not-exist"); + remove_skill_link(&nonexistent).unwrap(); + } + + #[test] + fn test_install_and_uninstall_skills_lifecycle() { + let skills_base = skills_dir_path().unwrap(); + + // Dry run should not create anything + let dry_result = install_skills(true, false).unwrap(); + assert!(dry_result.changed); + assert_eq!(dry_result.installed_count, EMBEDDED_SKILLS.len()); + + // Install creates skill files with correct content + let result = install_skills(false, false).unwrap(); + assert!(result.changed); + assert_eq!(result.installed_count, EMBEDDED_SKILLS.len()); + assert!(skills_base.exists()); + for skill in EMBEDDED_SKILLS { + let skill_md = skills_base.join(skill.name).join("SKILL.md"); + assert!(skill_md.exists(), "SKILL.md missing for {}", skill.name); + let content = fs::read_to_string(&skill_md).unwrap(); + assert_eq!(content, skill.skill_md); + } + + // Install again is idempotent + let result2 = install_skills(false, false).unwrap(); + assert!(result2.changed); + for skill in EMBEDDED_SKILLS { + let skill_md = skills_base.join(skill.name).join("SKILL.md"); + assert!( + skill_md.exists(), + "SKILL.md missing after re-install for {}", + skill.name + ); + } + + // Uninstall removes skills directory + let uninstall_result = uninstall_skills(false, false).unwrap(); + assert!(uninstall_result.changed); + assert!(!skills_base.exists()); + + // Uninstall again is a no-op + let noop_result = uninstall_skills(false, false).unwrap(); + assert!(!noop_result.changed); + assert_eq!(noop_result.installed_count, 0); + } }