Skip to content

Conversation

JohanHoltby
Copy link

@JohanHoltby JohanHoltby commented Oct 9, 2025

Fixes Issue #308

This fixes the problem that adding a tool to the MCPForUnity Requires users to move around the py files and specific instructions on how to install so the MCPforUnity is editable.

Feature improvement
It out finds all folders called: MCPForUnityTools and check for a py file and an version.txt. On Domain reload if version.txt is updated the tool gets updated to the server folder e.g. C:\Users\UserName\AppData\Local\UnityMCP\UnityMcpServer\src\tools

This allows any one to make there asset MCP enabled by just installing MCPForUnity and add this folder to the tool. All would be automatic for end users.

Custom Tool folder structure
A good fodler structure on a custom tool would look like this:
CustomTool/
├── CustomTool.MCPEnabler.asmdef
├── CustomTool.MCPEnabler.asmdef.meta
├── ExternalAssetToolFunction.cs
├── ExternalAssetToolFunction.cs.meta
├── external_asset_tool_function.py
├── external_asset_tool_function.py.meta
├── version.txt
└── version.txt.meta

The asmdef is recommended to allow for dependency on MCPForUnity when MCP For Unity is installed:

asmdef example
{
"name": "CustomTool.MCPEnabler",
"rootNamespace": "MCPForUnity.Editor.Tools",
"references": [
"CustomTool",
"MCPForUnity.Editor"
],
"includePlatforms": [
"Editor"
],
"excludePlatforms": [],
"allowUnsafeCode": false,
"overrideReferences": false,
"precompiledReferences": [],
"autoReferenced": true,
"defineConstraints": [],
"versionDefines": [],
"noEngineReferences": false
}

CS files are left in the tools folder.

version.txt and the py file get copied.

Summary by CodeRabbit

  • New Features

    • Syncs project-specific tools into the embedded server during install/rebuild with per-folder version tracking to copy only changed tools.
    • Tool discovery now includes one-level subdirectories for improved plugin/module loading.
    • Automatically removes stale upstream tool folders during synchronization.
  • Bug Fixes

    • Update/install flow now triggers when tool versions change or on related errors.
    • More robust error handling and clearer logs for copied, skipped, and failed tool files.

…n.txt

This allows for auto finding new tools. A good dir on a custom tool would look like this:
CustomTool/
├── CustomTool.MCPEnabler.asmdef
├── CustomTool.MCPEnabler.asmdef.meta
├── ExternalAssetToolFunction.cs
├── ExternalAssetToolFunction.cs.meta
├── external_asset_tool_function.py
├── external_asset_tool_function.py.meta
├── version.txt
└── version.txt.meta

CS files are left in the tools folder. The asmdef is recommended to allow for dependency on MCPForUnity when MCP For Unity is installed:

asmdef example
{
    "name": "CustomTool.MCPEnabler",
    "rootNamespace": "MCPForUnity.Editor.Tools",
    "references": [
        "CustomTool",
        "MCPForUnity.Editor"
    ],
    "includePlatforms": [
        "Editor"
    ],
    "excludePlatforms": [],
    "allowUnsafeCode": false,
    "overrideReferences": false,
    "precompiledReferences": [],
    "autoReferenced": true,
    "defineConstraints": [],
    "versionDefines": [],
    "noEngineReferences": false
}
Copy link
Contributor

coderabbitai bot commented Oct 9, 2025

Walkthrough

Adds per-folder MCPForUnityTools version detection to PackageDetector to force updates when project tool versions differ; extends ServerInstaller to copy per-folder Python tools into the embedded server with per-folder identifiers, version tracking, and stale-folder cleanup; Python tool discovery now includes one-level subdirectories.

Changes

Cohort / File(s) Summary of Changes
Update decision and tool version detection
MCPForUnity/Editor/Helpers/PackageDetector.cs
Added ToolsVersionsChanged to scan project MCPForUnityTools folders and compare each folder's version.txt to the server _version.txt; added GetToolsFolderIdentifier; integrated toolsNeedUpdate into update gating and wrapped checks in try/catch to force update on errors. No public API changes.
Server install and project tools synchronization
MCPForUnity/Editor/Helpers/ServerInstaller.cs
Added CopyUnityProjectTools(destToolsDir) to locate MCPForUnityTools in project roots and copy non-__init__.py .py files into per-folder subdirectories under server tools; added per-folder _version.txt tracking, GetToolsFolderIdentifier, and CleanupStaleToolFolders to purge missing upstream folders; invoked from EnsureServerInstalled and RebuildMcpServer with logging and error handling.
Server tools discovery (Python)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py
Extended auto-discovery to include modules in one-level subdirectories under tools/, importing submodule modules via importlib.import_module with package context; preserves skipping of private/hidden names and existing import error warnings.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor UnityEditor as Unity Editor
  participant PackageDetector
  participant ServerInstaller
  participant Project as Unity Project (MCPForUnityTools)
  participant Server as Embedded Server (destSrc/tools)

  UnityEditor->>PackageDetector: Initialize / Check package state
  PackageDetector->>Project: Scan MCPForUnityTools folders & read version.txt
  PackageDetector->>Server: Read per-folder _version.txt in server tools
  alt mismatch / missing / error
    PackageDetector-->>UnityEditor: toolsNeedUpdate = true
  else all match
    PackageDetector-->>UnityEditor: toolsNeedUpdate = false
  end

  alt toolsNeedUpdate or other update flags
    UnityEditor->>ServerInstaller: EnsureServerInstalled / RebuildMcpServer
    ServerInstaller->>Project: Discover MCPForUnityTools folders (Assets/Packages/Library/PackageCache)
    ServerInstaller->>Server: Copy .py files (except __init__.py) to destSrc/tools/<folder_id>
    ServerInstaller->>Server: Write/update per-folder _version.txt
    ServerInstaller->>Server: Cleanup stale tool folders not in active set
    ServerInstaller-->>UnityEditor: Install/Update complete
  else no update
    PackageDetector-->>UnityEditor: No action
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I’m a rabbit in the code-filled glade,
I sniff version crumbs the devs have laid.
I hop and copy, tidy every nest—
Per-folder tools aligned at my behest.
Now I nap, content; the server’s dressed. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly references the targeted issue and clearly describes the primary change of discovering .py files and version.txt in MCPForUnityTools, which matches the main enhancements in the PR.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (5)
MCPForUnity/Editor/Helpers/PackageDetector.cs (2)

135-137: Limit scan scope to Assets/Packages to avoid heavy project-wide traversal.

Scanning the entire project root can walk Library, Temp, etc. Prefer Assets and Packages only.

Apply this diff:

-                // Find all MCPForUnityTools folders in project
-                var toolsFolders = System.IO.Directory.GetDirectories(projectRoot, "MCPForUnityTools", System.IO.SearchOption.AllDirectories);
+                // Find all MCPForUnityTools folders under Assets and Packages only
+                var toolsFolders = new System.Collections.Generic.List<string>();
+                string assetsDir = System.IO.Path.Combine(projectRoot, "Assets");
+                if (System.IO.Directory.Exists(assetsDir))
+                    toolsFolders.AddRange(System.IO.Directory.GetDirectories(assetsDir, "MCPForUnityTools", System.IO.SearchOption.AllDirectories));
+                string packagesDir = System.IO.Path.Combine(projectRoot, "Packages");
+                if (System.IO.Directory.Exists(packagesDir))
+                    toolsFolders.AddRange(System.IO.Directory.GetDirectories(packagesDir, "MCPForUnityTools", System.IO.SearchOption.AllDirectories));

181-213: Deduplicate GetToolsFolderIdentifier to avoid drift.

This logic is duplicated here and in ServerInstaller. Prefer a single source of truth.

  • Change ServerInstaller.GetToolsFolderIdentifier(...) to internal static.
  • Use that here and remove this duplicate method.

Proposed changes:

In ServerInstaller.cs (make method internal):

-        private static string GetToolsFolderIdentifier(string toolsFolderPath)
+        internal static string GetToolsFolderIdentifier(string toolsFolderPath)

In PackageDetector.cs (remove this method and call ServerInstaller.GetToolsFolderIdentifier(folder) where needed).

MCPForUnity/Editor/Helpers/ServerInstaller.cs (3)

421-423: Reduce scan scope to Assets/Packages for performance.

Avoid traversing Library/Temp etc. on each domain reload.

Apply this diff:

-                // Find all MCPForUnityTools folders
-                var toolsFolders = Directory.GetDirectories(projectRoot, "MCPForUnityTools", SearchOption.AllDirectories);
+                // Find all MCPForUnityTools folders under Assets and Packages only
+                var toolsFolders = new List<string>();
+                var assetsDir = Path.Combine(projectRoot, "Assets");
+                if (Directory.Exists(assetsDir))
+                    toolsFolders.AddRange(Directory.GetDirectories(assetsDir, "MCPForUnityTools", SearchOption.AllDirectories));
+                var packagesDir = Path.Combine(projectRoot, "Packages");
+                if (Directory.Exists(packagesDir))
+                    toolsFolders.AddRange(Directory.GetDirectories(packagesDir, "MCPForUnityTools", SearchOption.AllDirectories));

494-531: Expose GetToolsFolderIdentifier for reuse.

Make this helper internal so PackageDetector can reuse it and remove duplicate logic.

Apply this diff:

-        private static string GetToolsFolderIdentifier(string toolsFolderPath)
+        internal static string GetToolsFolderIdentifier(string toolsFolderPath)

483-487: Optional: log skipped tools for visibility.

You track skippedCount but don't surface it.

Apply this diff:

                 if (copiedCount > 0)
                 {
                     McpLog.Info($"Copied {copiedCount} Unity project tool(s) to server");
                 }
+                else if (skippedCount > 0)
+                {
+                    McpLog.Info($"All Unity project tools up-to-date ({skippedCount} skipped)");
+                }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 50844c2 and f713810.

📒 Files selected for processing (2)
  • MCPForUnity/Editor/Helpers/PackageDetector.cs (2 hunks)
  • MCPForUnity/Editor/Helpers/ServerInstaller.cs (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
MCPForUnity/Editor/Helpers/PackageDetector.cs (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (3)
  • ServerInstaller (12-836)
  • GetServerPath (117-120)
  • GetToolsFolderIdentifier (498-531)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)
MCPForUnity/Editor/Helpers/PackageDetector.cs (1)
  • GetToolsFolderIdentifier (184-213)
🔇 Additional comments (2)
MCPForUnity/Editor/Helpers/PackageDetector.cs (1)

26-29: Good: update path now considers tool version drift.

This ensures domain-reload updates when project tool versions change.

MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)

450-465: Namespace copied tools to avoid collisions
Flat-copying all .py files into tools allows same-named files from different folderIdentifier to overwrite each other. Choose one:

  • Copy into per-source subfolders:
    var targetDir = Path.Combine(destToolsDir, folderIdentifier);
    Directory.CreateDirectory(targetDir);
    var destFile = Path.Combine(targetDir, fileName);
    (Ensure your server loads tools recursively.)
  • Or detect duplicates before File.Copy (e.g. if (!seen.Add(fileName) || File.Exists(destFile)) log and skip.)

Confirm whether the server’s tool discovery supports nested directories.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)

410-492: Prevent copy failures, avoid filename collisions, and limit scan scope.

  • Ensure dest tools dir exists before File.Copy.
  • Flat-copying .py into a shared tools root risks overwriting between multiple MCPForUnityTools folders (same filenames). Copy into a per-folder subdirectory keyed by folderIdentifier.
  • Limit scan to Assets/ and Packages/ to avoid deep recursion into Library/.git and speed up. Also clean stale .py removed upstream.

Apply this diff:

 private static void CopyUnityProjectTools(string destToolsDir)
 {
     try
     {
         // Get Unity project root
         string projectRoot = Directory.GetParent(Application.dataPath)?.FullName;
         if (string.IsNullOrEmpty(projectRoot))
         {
             return;
         }
 
+        // Ensure destination tools directory exists
+        Directory.CreateDirectory(destToolsDir);
+
-        // Find all MCPForUnityTools folders
-        var toolsFolders = Directory.GetDirectories(projectRoot, "MCPForUnityTools", SearchOption.AllDirectories);
+        // Find all MCPForUnityTools folders (limit to Assets/ and Packages/)
+        var searchRoots = new[] { Path.Combine(projectRoot, "Assets"), Path.Combine(projectRoot, "Packages") }
+            .Where(Directory.Exists);
+        var toolsFolders = searchRoots
+            .SelectMany(root => Directory.GetDirectories(root, "MCPForUnityTools", SearchOption.AllDirectories))
+            .ToArray();
 
         int copiedCount = 0;
         int skippedCount = 0;
 
         foreach (var folder in toolsFolders)
         {
             // Generate unique identifier for this tools folder based on its parent directory structure
             // e.g., "MooseRunner_MCPForUnityTools" or "MyPackage_MCPForUnityTools"
             string folderIdentifier = GetToolsFolderIdentifier(folder);
-            string versionTrackingFile = Path.Combine(destToolsDir, $"{folderIdentifier}_version.txt");
+            string destToolFolder = Path.Combine(destToolsDir, folderIdentifier);
+            string versionTrackingFile = Path.Combine(destToolFolder, "_version.txt");
 
             // Read source version
             string sourceVersionFile = Path.Combine(folder, "version.txt");
             string sourceVersion = ReadVersionFile(sourceVersionFile) ?? "0.0.0";
 
             // Read installed version (tracked separately per tools folder)
             string installedVersion = ReadVersionFile(versionTrackingFile);
 
             // Check if update is needed (version different or no tracking file)
             bool needsUpdate = string.IsNullOrEmpty(installedVersion) || sourceVersion != installedVersion;
 
             if (needsUpdate)
             {
                 // Get all .py files (excluding __init__.py)
                 var pyFiles = Directory.GetFiles(folder, "*.py")
                     .Where(f => !Path.GetFileName(f).Equals("__init__.py", StringComparison.OrdinalIgnoreCase));
 
+                // Ensure per-tool destination exists
+                Directory.CreateDirectory(destToolFolder);
+
+                // Cleanup stale .py files that were removed upstream
+                try
+                {
+                    var sourceNames = new HashSet<string>(pyFiles.Select(f => Path.GetFileName(f)), StringComparer.OrdinalIgnoreCase);
+                    foreach (var destPy in Directory.GetFiles(destToolFolder, "*.py"))
+                    {
+                        if (!sourceNames.Contains(Path.GetFileName(destPy)))
+                        {
+                            File.Delete(destPy);
+                        }
+                    }
+                }
+                catch (Exception cleanupEx)
+                {
+                    McpLog.Warn($"Failed to cleanup stale files for {folderIdentifier}: {cleanupEx.Message}");
+                }
+
                 foreach (var pyFile in pyFiles)
                 {
                     string fileName = Path.GetFileName(pyFile);
-                    string destFile = Path.Combine(destToolsDir, fileName);
+                    string destFile = Path.Combine(destToolFolder, fileName);
 
                     try
                     {
                         File.Copy(pyFile, destFile, overwrite: true);
                         copiedCount++;
                         McpLog.Info($"Copied Unity project tool: {fileName} from {folderIdentifier} (v{sourceVersion})");
                     }
                     catch (Exception ex)
                     {
                         McpLog.Warn($"Failed to copy {fileName}: {ex.Message}");
                     }
                 }
 
                 // Update version tracking file
                 try
                 {
                     File.WriteAllText(versionTrackingFile, sourceVersion);
                 }
                 catch (Exception ex)
                 {
                     McpLog.Warn($"Failed to write version tracking file for {folderIdentifier}: {ex.Message}");
                 }
             }
             else
             {
                 skippedCount++;
             }
         }
 
         if (copiedCount > 0)
         {
             McpLog.Info($"Copied {copiedCount} Unity project tool(s) to server");
         }
     }
     catch (Exception ex)
     {
         McpLog.Warn($"Failed to scan Unity project for tools: {ex.Message}");
     }
 }

Note: If tools rely on package-style relative imports, consider copying their __init__.py into the per-folder subdir to preserve package semantics. Please confirm expected import pattern.

🧹 Nitpick comments (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)

498-531: Consolidate GetToolsFolderIdentifier into a shared helper
Extract the duplicate logic from ServerInstaller.cs and PackageDetector.cs into a single utility method to keep identifier rules consistent.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f713810 and 48d912e.

📒 Files selected for processing (2)
  • MCPForUnity/Editor/Helpers/PackageDetector.cs (2 hunks)
  • MCPForUnity/Editor/Helpers/ServerInstaller.cs (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • MCPForUnity/Editor/Helpers/PackageDetector.cs
🔇 Additional comments (2)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (2)

61-64: Good integration point for project tools.

Running tools copy independently of server version makes sense. No issues at the call site.


597-600: Rebuild path also wiring tools copy — good.

Hooking tools copy into rebuild keeps server and tools in sync.

… GetToolsFolderIdentifier, fix tools copy dir, and limit scan scope
@JohanHoltby
Copy link
Author

So all ready to go if any one who approves likes it. :)

@JohanHoltby
Copy link
Author

So now pycache fix is done. Used the _skipDirs as that sounds about right ;)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 68f09e1 and 069f1be.

📒 Files selected for processing (1)
  • MCPForUnity/Editor/Helpers/ServerInstaller.cs (3 hunks)
🔇 Additional comments (4)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (4)

55-63: LGTM!

The integration of CopyUnityProjectTools into EnsureServerInstalled is correct. The method is called after server installation/update, and the comment clearly indicates that tool copying runs independently of server version updates, which aligns with the PR objectives.


534-574: LGTM!

The cleanup logic correctly removes stale tool subdirectories that no longer have corresponding source folders in the Unity project. The use of goto on lines 551 and 567 is an acceptable C# idiom for breaking out of nested loops in this context.


580-613: LGTM!

The identifier generation logic correctly walks up the directory tree to find a distinctive parent folder. The method appropriately stops at Assets or Packages boundaries and provides reasonable fallbacks for edge cases.


679-681: LGTM!

The integration of CopyUnityProjectTools into RebuildMcpServer ensures that Unity project tools remain synchronized after a server rebuild, maintaining consistency with the EnsureServerInstalled flow.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 069f1be and b1f696d.

📒 Files selected for processing (1)
  • MCPForUnity/Editor/Helpers/ServerInstaller.cs (3 hunks)
🔇 Additional comments (5)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (5)

404-404: LGTM!

The skip list appropriately covers Python virtual environments, cache directories, and version control artifacts that should be excluded during server installation and cleanup operations.


62-63: LGTM!

The integration of CopyUnityProjectTools after server installation ensures that Unity project tools are synchronized independently of server version updates, which aligns with the PR objectives for automatic tool deployment.


686-688: LGTM!

Consistent integration pattern with EnsureServerInstalled, ensuring tools are copied after server rebuild operations.


411-536: LGTM!

The CopyUnityProjectTools implementation is comprehensive and well-structured:

  • ✅ Properly searches limited roots (Assets, Packages, PackageCache) to avoid performance issues
  • ✅ Creates destination directory before operations (addresses previous review comment)
  • ✅ Per-folder versioning with version.txt tracking enables conditional updates
  • ✅ Validates and skips folders without .py files (addresses previous review comment)
  • ✅ Robust error handling with informative logging
  • ✅ Cleanup of stale tool folders ensures consistency

The version comparison using strict string equality (line 476) is appropriate for this use case where exact version matching determines when to copy tools.


541-581: LGTM!

The cleanup logic correctly removes stale tool folders while preserving Python cache and virtual environment directories. The error handling ensures that failures to delete individual directories don't halt the entire cleanup process.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)

455-489: Don’t create per-folder dirs when no .py files; add to active set only if a tool exists.

Currently the subdir is created and left empty when no .py files are found. Move enumeration before creating the subdir and before adding to activeFolderIdentifiers.

Apply this diff:

-                    // Generate unique identifier for this tools folder based on its parent directory structure
-                    // e.g., "MooseRunner_MCPForUnityTools" or "MyPackage_MCPForUnityTools"
-                    string folderIdentifier = GetToolsFolderIdentifier(folder);
-                    activeFolderIdentifiers.Add(folderIdentifier);
-
-                    // Create per-folder subdirectory in destToolsDir
-                    string destFolderSubdir = Path.Combine(destToolsDir, folderIdentifier);
-                    Directory.CreateDirectory(destFolderSubdir);
-
-                    string versionTrackingFile = Path.Combine(destFolderSubdir, "version.txt");
-
-                    // Read source version
-                    string sourceVersionFile = Path.Combine(folder, "version.txt");
-                    string sourceVersion = ReadVersionFile(sourceVersionFile) ?? "0.0.0";
-
-                    // Read installed version (tracked separately per tools folder)
-                    string installedVersion = ReadVersionFile(versionTrackingFile);
-
-                    // Check if update is needed (version different or no tracking file)
-                    bool needsUpdate = string.IsNullOrEmpty(installedVersion) || sourceVersion != installedVersion;
-
-                    if (needsUpdate)
-                    {
-                        // Get all .py files (excluding __init__.py)
-                        var pyFiles = Directory.GetFiles(folder, "*.py")
-                            .Where(f => !Path.GetFileName(f).Equals("__init__.py", StringComparison.OrdinalIgnoreCase));
-                            
-                        // Skip folders with no .py files
-                        if (!pyFiles.Any())
-                        { 
-                            skippedCount++; 
-                            continue; 
-                        }
+                    // Get all .py files (excluding __init__.py)
+                    var pyFiles = Directory.GetFiles(folder, "*.py")
+                        .Where(f => !Path.GetFileName(f).Equals("__init__.py", StringComparison.OrdinalIgnoreCase));
+
+                    // Skip folders with no .py files (don't add to active set and don't create subdir)
+                    if (!pyFiles.Any())
+                    {
+                        skippedCount++;
+                        continue;
+                    }
+
+                    // Generate identifier only for valid tool folders and mark active
+                    string folderIdentifier = GetToolsFolderIdentifier(folder);
+                    activeFolderIdentifiers.Add(folderIdentifier);
+
+                    string destFolderSubdir = Path.Combine(destToolsDir, folderIdentifier);
+                    string versionTrackingFile = Path.Combine(destFolderSubdir, "version.txt");
+
+                    // Read versions
+                    string sourceVersionFile = Path.Combine(folder, "version.txt");
+                    string sourceVersion = ReadVersionFile(sourceVersionFile) ?? "0.0.0";
+                    string installedVersion = ReadVersionFile(versionTrackingFile);
+
+                    bool needsUpdate = string.IsNullOrEmpty(installedVersion) || sourceVersion != installedVersion;
+
+                    if (needsUpdate)
+                    {
+                        Directory.CreateDirectory(destFolderSubdir);
🧹 Nitpick comments (2)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (2)

435-447: Deduplicate “Packages” vs “Library/PackageCache” results.

Scanning both can find the same tool twice. Prefer the Packages path when both exist; skip PackageCache duplicates.

Would you like me to propose a small de-dupe map keyed by package folder name (preferring “Packages/” over “Library/PackageCache/”)?


480-489: Verify intent: excluding init.py and not copying subdirectories.

If tools use packages or one‑level submodules, skipping init.py and not copying subdirs may break imports.

  • Option A: keep single‑file tool policy; document it clearly.
  • Option B: copy one‑level subdirectories (skip pycache, venvs) and include init.py when present.
    Do you want a patch for Option B?
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b1f696d and 88a6491.

📒 Files selected for processing (1)
  • MCPForUnity/Editor/Helpers/ServerInstaller.cs (3 hunks)
🔇 Additional comments (2)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (2)

422-424: LGTM: ensure destination tools dir exists.

Creating destToolsDir prevents File.Copy failures on fresh installs.


61-64: LGTM: project tools copied during install and rebuild.

Hooks are placed correctly after server copy.

Also applies to: 686-689

JohanHoltby and others added 3 commits October 10, 2025 22:36
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)

596-629: Critical: Address unresolved identifier collision and PackageCache issues.

The previous review identified two problems that remain unfixed:

  1. PackageCache not handled: For paths like Library/PackageCache/com.example@1.0.0/Editor/MCPForUnityTools, the code doesn't recognize PackageCache as a valid root, falling back to the generic Editor_MCPForUnityTools identifier.

  2. Collision risk: Multiple packages under the same parent produce identical identifiers. For example:

    • Assets/Plugins/Package1/Editor/MCPForUnityToolsAssets_Plugins_MCPForUnityTools
    • Assets/Plugins/Package2/Editor/MCPForUnityToolsAssets_Plugins_MCPForUnityTools

    These collisions cause tools to overwrite each other.

Apply the diff from the previous review to fix both issues:

 internal static string GetToolsFolderIdentifier(string toolsFolderPath)
 {
     try
     {
-        // Get parent directory name (e.g., "Editor" or package name)
-        DirectoryInfo parent = Directory.GetParent(toolsFolderPath);
-        if (parent == null) return "MCPForUnityTools";
-
-        // Walk up to find a distinctive parent (Assets/PackageName or Packages/PackageName)
-        DirectoryInfo current = parent;
-        while (current != null)
-        {
-            string name = current.Name;
-            DirectoryInfo grandparent = current.Parent;
-
-            // Stop at Assets, Packages, or if we find a package-like structure
-            if (grandparent != null &&
-                (grandparent.Name.Equals("Assets", StringComparison.OrdinalIgnoreCase) ||
-                 grandparent.Name.Equals("Packages", StringComparison.OrdinalIgnoreCase)))
-            {
-                return $"{grandparent.Name}_{name}_MCPForUnityTools";
-            }
-
-            current = grandparent;
-        }
-
-        // Fallback: use immediate parent
-        return $"{parent.Name}_MCPForUnityTools";
+        // toolsFolderPath → .../<PackageFolder>/Editor/MCPForUnityTools
+        var editorDir = Directory.GetParent(toolsFolderPath);      // Editor
+        var packageDir = editorDir?.Parent;                         // <PackageFolder>
+        if (packageDir == null) return "MCPForUnityTools";
+
+        // Find root (Assets / Packages / PackageCache)
+        DirectoryInfo current = packageDir;
+        while (current?.Parent != null)
+        {
+            var p = current.Parent;
+            if (p.Name.Equals("Assets", StringComparison.OrdinalIgnoreCase) ||
+                p.Name.Equals("Packages", StringComparison.OrdinalIgnoreCase) ||
+                p.Name.Equals("PackageCache", StringComparison.OrdinalIgnoreCase))
+            {
+                return $"{p.Name}_{current.Name}_MCPForUnityTools";
+            }
+            current = p;
+        }
+
+        // Fallback: use package dir name
+        return $"{packageDir.Name}_MCPForUnityTools";
     }
     catch
     {
         return "MCPForUnityTools";
     }
 }

This ensures unique identifiers by including the package folder name directly above Editor, preventing collisions between different packages under the same parent directory, and properly handling PackageCache locations.

🧹 Nitpick comments (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)

462-465: Consider deferring subdirectory creation until after version check.

The subdirectory is created on line 464 before checking if an update is needed on line 478. For up-to-date folders, this performs unnecessary directory creation operations.

Apply this diff to defer directory creation:

         string folderIdentifier = GetToolsFolderIdentifier(folder);
         activeFolderIdentifiers.Add(folderIdentifier);
 
-        // Create per-folder subdirectory in destToolsDir
-        string destFolderSubdir = Path.Combine(destToolsDir, folderIdentifier);
-        Directory.CreateDirectory(destFolderSubdir);
-
+        string destFolderSubdir = Path.Combine(destToolsDir, folderIdentifier);
         string versionTrackingFile = Path.Combine(destFolderSubdir, "version.txt");
 
         // Read source version
         string sourceVersionFile = Path.Combine(folder, "version.txt");
         string sourceVersion = ReadVersionFile(sourceVersionFile) ?? "0.0.0";
 
         // Read installed version (tracked separately per tools folder)
         string installedVersion = ReadVersionFile(versionTrackingFile);
 
         // Check if update is needed (version different or no tracking file)
         bool needsUpdate = string.IsNullOrEmpty(installedVersion) || sourceVersion != installedVersion;
 
         if (needsUpdate)
         {
+            // Create per-folder subdirectory in destToolsDir
+            Directory.CreateDirectory(destFolderSubdir);
+
             // Get all .py files (excluding __init__.py)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6a4d55b and 3fef23e.

📒 Files selected for processing (1)
  • MCPForUnity/Editor/Helpers/ServerInstaller.cs (3 hunks)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Py files not found and tracked by external tools added.

1 participant