From 96efe0cb66ac772ae9a148a32d1ef4013658b920 Mon Sep 17 00:00:00 2001 From: hatayama Date: Thu, 9 Apr 2026 23:43:07 +0900 Subject: [PATCH 1/2] Tighten skill auto-install target detection Require an existing skills directory before treating an AI tool configuration as an install target so toggling tools no longer creates skill folders from parent directories alone. Add editor tests that cover both the parent-only case and the empty skills-directory case, and update the setup UI message to match the stricter detection rule. --- .../Editor/ToolSkillSynchronizerTests.cs | 57 ++++++++++++++++++- .../Editor/Config/ToolSkillSynchronizer.cs | 24 ++++---- .../src/Editor/UI/Setup/SetupWizardWindow.cs | 2 +- 3 files changed, 69 insertions(+), 14 deletions(-) diff --git a/Assets/Tests/Editor/ToolSkillSynchronizerTests.cs b/Assets/Tests/Editor/ToolSkillSynchronizerTests.cs index c8b6368b4..f19976aeb 100644 --- a/Assets/Tests/Editor/ToolSkillSynchronizerTests.cs +++ b/Assets/Tests/Editor/ToolSkillSynchronizerTests.cs @@ -8,6 +8,8 @@ namespace io.github.hatayama.uLoopMCP [TestFixture] public class ToolSkillSynchronizerTests { + private const string SkillsDirName = "skills"; + private string _projectRoot; private string[] _nonExistentDirsBefore; @@ -31,7 +33,7 @@ public void TearDown() if (Directory.Exists(fullPath)) { // Only delete if it was created by this test (didn't exist before) - string skillsPath = Path.Combine(fullPath, "skills"); + string skillsPath = Path.Combine(fullPath, SkillsDirName); if (Directory.Exists(skillsPath)) { Directory.Delete(skillsPath, true); @@ -64,6 +66,59 @@ public async Task InstallSkillFiles_DoesNotCreateNonExistentTargetDirectories() } } + [Test] + public void DetectTargets_DoesNotIncludeTargetsWithOnlyParentDirectory() + { + // Arrange + Assert.IsNotEmpty(_nonExistentDirsBefore, + "At least one target directory should not exist for this test to be meaningful"); + + foreach (string dir in _nonExistentDirsBefore) + { + Directory.CreateDirectory(Path.Combine(_projectRoot, dir)); + } + + // Act + string[] detectedTargetDirs = ToolSkillSynchronizer.DetectTargets() + .Select(target => target.DirName) + .ToArray(); + + // Assert + foreach (string dir in _nonExistentDirsBefore) + { + Assert.IsFalse(detectedTargetDirs.Contains(dir), + $"Target '{dir}' should not be detected when only the parent directory exists"); + } + } + + [Test] + public void DetectTargets_IncludesTargetsWhenSkillsDirectoryExists() + { + // Arrange + Assert.IsNotEmpty(_nonExistentDirsBefore, + "At least one target directory should not exist for this test to be meaningful"); + + foreach (string dir in _nonExistentDirsBefore) + { + Directory.CreateDirectory(Path.Combine(_projectRoot, dir, SkillsDirName)); + } + + // Act + ToolSkillSynchronizer.SkillTargetInfo[] detectedTargets = ToolSkillSynchronizer.DetectTargets() + .Where(target => _nonExistentDirsBefore.Contains(target.DirName)) + .ToArray(); + + // Assert + Assert.AreEqual(_nonExistentDirsBefore.Length, detectedTargets.Length, + "Targets with a skills directory should be detected"); + + foreach (ToolSkillSynchronizer.SkillTargetInfo target in detectedTargets) + { + Assert.IsFalse(target.HasExistingSkills, + $"Target '{target.DirName}' should not be treated as already installed when skills directory is empty"); + } + } + [Test] public void RemoveSkillFiles_DoesNotCreateNonExistentTargetDirectories() { diff --git a/Packages/src/Editor/Config/ToolSkillSynchronizer.cs b/Packages/src/Editor/Config/ToolSkillSynchronizer.cs index f39bfdd47..7d2421089 100644 --- a/Packages/src/Editor/Config/ToolSkillSynchronizer.cs +++ b/Packages/src/Editor/Config/ToolSkillSynchronizer.cs @@ -17,6 +17,9 @@ namespace io.github.hatayama.uLoopMCP /// public static class ToolSkillSynchronizer { + private const string SkillsDirName = "skills"; + private const string SkillFileName = "SKILL.md"; + public readonly struct SkillTargetDefinition { public readonly string DirName; @@ -78,7 +81,7 @@ public static void RemoveSkillFiles(string toolName) foreach (string targetDir in SkillTargetDirs) { - string skillsRoot = Path.Combine(projectRoot, targetDir, "skills"); + string skillsRoot = Path.Combine(projectRoot, targetDir, SkillsDirName); if (!Directory.Exists(skillsRoot)) { continue; @@ -103,7 +106,7 @@ public static bool IsSkillInstalled(string toolName) foreach (string targetDir in SkillTargetDirs) { - string skillsRoot = Path.Combine(projectRoot, targetDir, "skills"); + string skillsRoot = Path.Combine(projectRoot, targetDir, SkillsDirName); if (!Directory.Exists(skillsRoot)) { continue; @@ -123,7 +126,7 @@ public static bool IsSkillInstalled(string toolName) } /// - /// Checks parent directories (.claude/, .agents/, etc.), not skills/ subdirectories. + /// Checks skills directories (.claude/skills, .agents/skills, etc.) before auto-installing. /// public static List DetectTargets() { @@ -134,16 +137,14 @@ public static List DetectTargets() foreach (SkillTargetDefinition target in SkillTargets) { - string parentDir = Path.Combine(projectRoot, target.DirName); - if (!Directory.Exists(parentDir)) + string skillsRoot = Path.Combine(projectRoot, target.DirName, SkillsDirName); + if (!Directory.Exists(skillsRoot)) { continue; } - string skillsRoot = Path.Combine(parentDir, "skills"); - bool hasULoopSkills = Directory.Exists(skillsRoot) - && Directory.EnumerateDirectories(skillsRoot, CliConstants.SKILL_DIR_GLOB) - .Any(skillDir => File.Exists(Path.Combine(skillDir, "SKILL.md"))); + bool hasULoopSkills = Directory.EnumerateDirectories(skillsRoot, CliConstants.SKILL_DIR_GLOB) + .Any(skillDir => File.Exists(Path.Combine(skillDir, SkillFileName))); targets.Add(new SkillTargetInfo(target.DisplayName, target.DirName, hasULoopSkills)); } @@ -151,8 +152,7 @@ public static List DetectTargets() } /// - /// Re-install skills for all detected targets. - /// Checks parent directories (.claude/, .agents/, etc.) to determine targets. + /// Re-installs skills for targets that already have a skills directory. /// public static async Task InstallSkillFiles() { @@ -196,7 +196,7 @@ public static async Task InstallSkillFiles(List Date: Thu, 9 Apr 2026 23:54:32 +0900 Subject: [PATCH 2/2] Separate auto-install target detection from setup detection Limit stricter skills-directory checks to implicit auto-install flows so explicit setup can still offer first-time installs for existing tool roots. Move ToolSkillSynchronizer target detection behind reusable helpers, keep the setup wizard messaging aligned with root-directory detection, and rewrite the editor tests to use temporary project roots instead of environment-dependent assumptions. --- .../Editor/ToolSkillSynchronizerTests.cs | 46 +++++++++++++------ .../Editor/Config/ToolSkillSynchronizer.cs | 34 ++++++++++---- .../src/Editor/UI/Setup/SetupWizardWindow.cs | 2 +- 3 files changed, 57 insertions(+), 25 deletions(-) diff --git a/Assets/Tests/Editor/ToolSkillSynchronizerTests.cs b/Assets/Tests/Editor/ToolSkillSynchronizerTests.cs index f19976aeb..19258acb8 100644 --- a/Assets/Tests/Editor/ToolSkillSynchronizerTests.cs +++ b/Assets/Tests/Editor/ToolSkillSynchronizerTests.cs @@ -12,6 +12,7 @@ public class ToolSkillSynchronizerTests private string _projectRoot; private string[] _nonExistentDirsBefore; + private string[] _temporaryRoots; [SetUp] public void SetUp() @@ -21,6 +22,7 @@ public void SetUp() _nonExistentDirsBefore = ToolSkillSynchronizer.SkillTargetDirs .Where(dir => !Directory.Exists(Path.Combine(_projectRoot, dir))) .ToArray(); + _temporaryRoots = new string[0]; } [TearDown] @@ -45,6 +47,14 @@ public void TearDown() } } } + + foreach (string temporaryRoot in _temporaryRoots) + { + if (Directory.Exists(temporaryRoot)) + { + Directory.Delete(temporaryRoot, true); + } + } } [Test] @@ -70,21 +80,19 @@ public async Task InstallSkillFiles_DoesNotCreateNonExistentTargetDirectories() public void DetectTargets_DoesNotIncludeTargetsWithOnlyParentDirectory() { // Arrange - Assert.IsNotEmpty(_nonExistentDirsBefore, - "At least one target directory should not exist for this test to be meaningful"); - - foreach (string dir in _nonExistentDirsBefore) + string temporaryRoot = CreateTemporaryProjectRoot(); + foreach (string dir in ToolSkillSynchronizer.SkillTargetDirs) { - Directory.CreateDirectory(Path.Combine(_projectRoot, dir)); + Directory.CreateDirectory(Path.Combine(temporaryRoot, dir)); } // Act - string[] detectedTargetDirs = ToolSkillSynchronizer.DetectTargets() + string[] detectedTargetDirs = ToolSkillSynchronizer.DetectTargets(temporaryRoot, requireSkillsDirectory: true) .Select(target => target.DirName) .ToArray(); // Assert - foreach (string dir in _nonExistentDirsBefore) + foreach (string dir in ToolSkillSynchronizer.SkillTargetDirs) { Assert.IsFalse(detectedTargetDirs.Contains(dir), $"Target '{dir}' should not be detected when only the parent directory exists"); @@ -95,21 +103,18 @@ public void DetectTargets_DoesNotIncludeTargetsWithOnlyParentDirectory() public void DetectTargets_IncludesTargetsWhenSkillsDirectoryExists() { // Arrange - Assert.IsNotEmpty(_nonExistentDirsBefore, - "At least one target directory should not exist for this test to be meaningful"); - - foreach (string dir in _nonExistentDirsBefore) + string temporaryRoot = CreateTemporaryProjectRoot(); + foreach (string dir in ToolSkillSynchronizer.SkillTargetDirs) { - Directory.CreateDirectory(Path.Combine(_projectRoot, dir, SkillsDirName)); + Directory.CreateDirectory(Path.Combine(temporaryRoot, dir, SkillsDirName)); } // Act - ToolSkillSynchronizer.SkillTargetInfo[] detectedTargets = ToolSkillSynchronizer.DetectTargets() - .Where(target => _nonExistentDirsBefore.Contains(target.DirName)) + ToolSkillSynchronizer.SkillTargetInfo[] detectedTargets = ToolSkillSynchronizer.DetectTargets(temporaryRoot, requireSkillsDirectory: true) .ToArray(); // Assert - Assert.AreEqual(_nonExistentDirsBefore.Length, detectedTargets.Length, + Assert.AreEqual(ToolSkillSynchronizer.SkillTargetDirs.Length, detectedTargets.Length, "Targets with a skills directory should be detected"); foreach (ToolSkillSynchronizer.SkillTargetInfo target in detectedTargets) @@ -154,5 +159,16 @@ public void IsSkillInstalled_DoesNotCreateNonExistentTargetDirectories() $"Directory '{dir}' should not be created by IsSkillInstalled"); } } + + private string CreateTemporaryProjectRoot() + { + string temporaryRoot = Path.Combine( + Path.GetTempPath(), + "ToolSkillSynchronizerTests", + System.Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(temporaryRoot); + _temporaryRoots = _temporaryRoots.Append(temporaryRoot).ToArray(); + return temporaryRoot; + } } } diff --git a/Packages/src/Editor/Config/ToolSkillSynchronizer.cs b/Packages/src/Editor/Config/ToolSkillSynchronizer.cs index 7d2421089..424324b55 100644 --- a/Packages/src/Editor/Config/ToolSkillSynchronizer.cs +++ b/Packages/src/Editor/Config/ToolSkillSynchronizer.cs @@ -125,26 +125,42 @@ public static bool IsSkillInstalled(string toolName) return false; } - /// - /// Checks skills directories (.claude/skills, .agents/skills, etc.) before auto-installing. - /// public static List DetectTargets() + { + return DetectTargets(requireSkillsDirectory: false); + } + + internal static List DetectTargets(bool requireSkillsDirectory) { string projectRoot = UnityMcpPathResolver.GetProjectRoot(); Debug.Assert(!string.IsNullOrEmpty(projectRoot), "projectRoot must not be null or empty"); + return DetectTargets(projectRoot, requireSkillsDirectory); + } + + internal static List DetectTargets(string projectRoot, bool requireSkillsDirectory) + { + Debug.Assert(!string.IsNullOrEmpty(projectRoot), "projectRoot must not be null or empty"); + List targets = new(); foreach (SkillTargetDefinition target in SkillTargets) { - string skillsRoot = Path.Combine(projectRoot, target.DirName, SkillsDirName); - if (!Directory.Exists(skillsRoot)) + string targetRoot = Path.Combine(projectRoot, target.DirName); + if (!Directory.Exists(targetRoot)) + { + continue; + } + + string skillsRoot = Path.Combine(targetRoot, SkillsDirName); + if (requireSkillsDirectory && !Directory.Exists(skillsRoot)) { continue; } - bool hasULoopSkills = Directory.EnumerateDirectories(skillsRoot, CliConstants.SKILL_DIR_GLOB) - .Any(skillDir => File.Exists(Path.Combine(skillDir, SkillFileName))); + bool hasULoopSkills = Directory.Exists(skillsRoot) + && Directory.EnumerateDirectories(skillsRoot, CliConstants.SKILL_DIR_GLOB) + .Any(skillDir => File.Exists(Path.Combine(skillDir, SkillFileName))); targets.Add(new SkillTargetInfo(target.DisplayName, target.DirName, hasULoopSkills)); } @@ -152,11 +168,11 @@ public static List DetectTargets() } /// - /// Re-installs skills for targets that already have a skills directory. + /// Re-installs skills only for targets that already opted in via an existing skills directory. /// public static async Task InstallSkillFiles() { - List targets = DetectTargets(); + List targets = DetectTargets(requireSkillsDirectory: true); return await InstallSkillFiles(targets); } diff --git a/Packages/src/Editor/UI/Setup/SetupWizardWindow.cs b/Packages/src/Editor/UI/Setup/SetupWizardWindow.cs index d8c2e30f0..19d38d240 100644 --- a/Packages/src/Editor/UI/Setup/SetupWizardWindow.cs +++ b/Packages/src/Editor/UI/Setup/SetupWizardWindow.cs @@ -200,7 +200,7 @@ private void UpdateSkillsStep( if (targets.Count == 0) { - _skillsStatusLabel.text = "No AI skills directories detected (.claude/skills, .agents/skills, etc.)"; + _skillsStatusLabel.text = "No AI tool directories detected (.claude/, .agents/, etc.)"; _installSkillsButton.SetEnabled(false); return; }