Allow users to easily add tools in the Asset folder #324
Conversation
…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
}
…lderIdentifier was duplicated>)
… GetToolsFolderIdentifier, fix tools copy dir, and limit scan scope
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ict to managed names.
…into fix-issue-308
This reverts commit 571ca8c.
WalkthroughAdds editor support to discover, track, and synchronize Python tool files: new ScriptableObject registry and state, a .py importer, registry and sync services with service-locator wiring, an editor processor for auto/manual sync, Python-side module discovery, tests, and updated docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Editor as Unity Editor
participant Proc as PythonToolSyncProcessor
participant AssetDB as AssetDatabase
participant RegistrySvc as PythonToolRegistryService
participant SyncSvc as ToolSyncService
participant Dest as MCP Server tools dir
Editor->>Proc: Asset change (.py added/modified/moved)
Proc->>AssetDB: query/import events
Proc->>SyncSvc: SyncAllTools()
SyncSvc->>RegistrySvc: GetAllRegistries()
RegistrySvc->>AssetDB: Find PythonToolsAsset assets
AssetDB-->>RegistrySvc: registries list
RegistrySvc-->>SyncSvc: registries
loop per registry -> per file
SyncSvc->>RegistrySvc: NeedsSync(registry, file)?
alt hashing enabled
RegistrySvc->>RegistrySvc: ComputeHash(file)
RegistrySvc-->>SyncSvc: yes/no
else
RegistrySvc-->>SyncSvc: yes
end
alt needs sync
SyncSvc->>Dest: Write .py file
SyncSvc->>RegistrySvc: RecordSync(registry, file)
RegistrySvc->>AssetDB: SetDirty(registry)
SyncSvc-->>SyncSvc: increment CopiedCount
else skipped
SyncSvc-->>SyncSvc: increment SkippedCount
end
end
SyncSvc->>RegistrySvc: CleanupStaleStates()
SyncSvc->>Dest: CleanupStaleFiles()
SyncSvc->>AssetDB: StartAssetEditing / StopAssetEditing
SyncSvc-->>Proc: ToolSyncResult
Proc-->>Editor: Log summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Good to give users a manual option
Don't react to PythonToolAsset changes - it only needs to react to Python file changes. And we also batch asset edits to minimise the DB refreshes
This fixes the issue with deletions in the asset, now file removals are synced
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/CUSTOM_TOOLS.md (2)
185-186: Append .png to the saved filename to match PNG encoding.The sample encodes PNGs but doesn’t add the extension; the earlier parameter hints “without extension”.
- string absolutePath = Path.Combine(Application.dataPath, "Screenshots", filename); + string absolutePath = Path.Combine(Application.dataPath, "Screenshots", filename + ".png");Also applies to: 219-221
200-217: Ensure RenderTexture/Texture2D are always released (finally).On exceptions before cleanup, RT/screenshot can leak. Wrap cleanup in finally.
- // Create a RenderTexture - RenderTexture rt = new RenderTexture(Screen.width, Screen.height, 24); - camera.targetTexture = rt; - // Render... - ... - Object.DestroyImmediate(rt); - // Save to file - byte[] bytes = screenshot.EncodeToPNG(); - File.WriteAllBytes(absolutePath, bytes); - Object.DestroyImmediate(screenshot); + RenderTexture rt = null; + Texture2D screenshot = null; + try + { + rt = new RenderTexture(Screen.width, Screen.height, 24); + camera.targetTexture = rt; + camera.Render(); + RenderTexture.active = rt; + screenshot = new Texture2D(Screen.width, Screen.height, TextureFormat.RGB24, false); + screenshot.ReadPixels(new Rect(0, 0, Screen.width, Screen.height), 0, 0); + screenshot.Apply(); + byte[] bytes = screenshot.EncodeToPNG(); + File.WriteAllBytes(absolutePath, bytes); + } + finally + { + camera.targetTexture = null; + RenderTexture.active = null; + if (rt) Object.DestroyImmediate(rt); + if (screenshot) Object.DestroyImmediate(screenshot); + }
🧹 Nitpick comments (24)
MCPForUnity/UnityMcpServer~/src/module_discovery.py (1)
31-35: Consider adding more context to the warning message.When logging import failures, including the full module path (not just the module name) and potentially the exception type would help with debugging.
try: full_module_name = f'.{module_name}' importlib.import_module(full_module_name, package_name) yield full_module_name except Exception as e: - logger.warning(f"Failed to import module {module_name}: {e}") + logger.warning(f"Failed to import module {package_name}.{module_name}: {type(e).__name__}: {e}")Apply a similar change at lines 48-55 for subdirectory imports.
MCPForUnity/Editor/Importers/PythonFileImporter.cs (1)
14-19: Consider specifying encoding explicitly.
File.ReadAllTextdefaults to UTF-8 on most systems, but explicitly specifyingEncoding.UTF8improves clarity and ensures consistent behavior across platforms.Apply this diff if you'd like to make the encoding explicit:
+using System.Text; + namespace MCPForUnity.Editor.Importers { /// <summary> /// Custom importer that allows Unity to recognize .py files as TextAssets. /// This enables Python files to be selected in the Inspector and used like any other text asset. /// </summary> [ScriptedImporter(1, "py")] public class PythonFileImporter : ScriptedImporter { public override void OnImportAsset(AssetImportContext ctx) { - var textAsset = new TextAsset(File.ReadAllText(ctx.assetPath)); + var textAsset = new TextAsset(File.ReadAllText(ctx.assetPath, Encoding.UTF8)); ctx.AddObjectToAsset("main obj", textAsset); ctx.SetMainObject(textAsset); } } }TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/ToolSyncServiceTests.cs (5)
20-25: Harden temp-dir cleanup in SetUp to avoid sporadic IO failures.Mirror TearDown's try/catch to prevent flaky failures on Windows AV/file locks.
Apply this diff:
- // Clean up any existing test directory - if (Directory.Exists(_testToolsDir)) - { - Directory.Delete(_testToolsDir, true); - } + // Clean up any existing test directory + if (Directory.Exists(_testToolsDir)) + { + try + { + Directory.Delete(_testToolsDir, true); + } + catch + { + // Ignore cleanup errors + } + }
52-60: Assert Success and SkippedCount for the “no assets” path.This verifies contract more fully.
Apply this diff:
var result = _service.SyncProjectTools(_testToolsDir); Assert.IsNotNull(result, "Should return a result"); - Assert.AreEqual(0, result.CopiedCount, "Should not copy any files"); - Assert.AreEqual(0, result.ErrorCount, "Should not have errors"); + Assert.IsTrue(result.Success, "Should succeed when no assets are present"); + Assert.AreEqual(0, result.CopiedCount, "Should not copy any files"); + Assert.AreEqual(0, result.SkippedCount, "Should not skip any files"); + Assert.AreEqual(0, result.ErrorCount, "Should not have errors");
92-100: Rename or ignore this placeholder test; it doesn’t exercise multiple assets.Either populate multiple PythonToolsAssets and assert counts, or mark ignored until an integration path exists.
Example diff (ignore for now):
- [Test] - public void SyncProjectTools_HandlesMultipleAssets() + [Test, Ignore("Populate multiple PythonToolsAssets and verify aggregation; convert to integration test.")] + public void SyncProjectTools_HandlesMultipleAssets() { // This would require creating multiple PythonToolsAssets // Better suited for integration testing var result = _service.SyncProjectTools(_testToolsDir); Assert.IsNotNull(result, "Should handle multiple assets gracefully"); }
102-110: Also assert Success reflects ErrorCount.Small contract check.
Apply this diff:
var result = _service.SyncProjectTools(_testToolsDir); Assert.IsTrue(result.CopiedCount >= 0, "Copied count should be non-negative"); Assert.IsTrue(result.SkippedCount >= 0, "Skipped count should be non-negative"); Assert.IsTrue(result.ErrorCount >= 0, "Error count should be non-negative"); + Assert.AreEqual(result.ErrorCount == 0, result.Success, "Success should be true when ErrorCount is zero.");
1-112: Add tests for single .py per MCPForUnityTools folder and version.txt update semantics.These are key behaviors from issues #308/#309 and the established design.
- Verify that a folder with multiple .py files is rejected or only one is used.
- Verify that changing version.txt triggers an update and copy; same version does not.
Based on learningsI can add Editor integration tests that scaffold temp MCPForUnityTools folders, import assets, and assert ToolSyncResult behavior.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Data/PythonToolsAssetTests.cs (2)
125-128: Prefer asserting non-empty identifier, not just non-null.Guard against empty GUIDs when assets aren’t in AssetDatabase.
Apply this diff:
- Assert.IsNotNull(_asset.fileStates[0].assetGuid, "Should store the GUID"); + Assert.IsFalse(string.IsNullOrEmpty(_asset.fileStates[0].assetGuid), + "Should store a non-empty GUID or fallback identifier");
139-147: Reduce flakiness: increase sleep or avoid timing dependence.10ms can be flaky on CI; bump to 50–100ms or assert with retry.
Apply this diff:
- System.Threading.Thread.Sleep(10); + System.Threading.Thread.Sleep(50);Even better: loop-wait until lastSyncTime changes with a short timeout. I can draft that if preferred.
MCPForUnity/Editor/Services/IToolSyncService.cs (2)
5-12: DTO looks good; consider a small convenience for callers.Optionally add a HadChanges property (CopiedCount > 0) and a Summary string to simplify UI/logging. Low effort, backward compatible.
public class ToolSyncResult { public int CopiedCount { get; set; } public int SkippedCount { get; set; } public int ErrorCount { get; set; } public List<string> Messages { get; set; } = new List<string>(); public bool Success => ErrorCount == 0; + public bool HadChanges => CopiedCount > 0; + public override string ToString() => + $"copied={CopiedCount}, skipped={SkippedCount}, errors={ErrorCount}"; }
16-16: Clarify destToolsDir contract.Add XML docs to state destToolsDir should point to the server’s custom tools folder (e.g., UnityMcpServer~/src/tools/custom). Prevents misuse.
docs/CUSTOM_TOOLS.md (4)
253-260: “description” is recommended, not strictly required — align wording.The code comment says “Required” but the note says “not strictly required.” Make both say “Recommended (strongly)” for consistency and accuracy.
- description="Tool description", # Required: describe what the tool does + description="Tool description", # Recommended: include for client compatibility ... -**Note:** All tools should have the `description` field. It's not strictly required, however, that parameter is the best place... +**Note:** All tools should include the `description` field. While not strictly required, it maximizes client compatibility and parsing quality...Also applies to: 267-267
163-168: Use consistent Response helper import to avoid fully qualified calls.Add Helpers using and drop long qualifiers in the example for clarity.
using System.IO; using Newtonsoft.Json.Linq; using UnityEngine; -using MCPForUnity.Editor.Tools; +using MCPForUnity.Editor.Helpers; +using MCPForUnity.Editor.Tools; // if needed elsewhere @@ - return MCPForUnity.Editor.Helpers.Response.Error("filename is required"); + return Response.Error("filename is required"); @@ - return MCPForUnity.Editor.Helpers.Response.Success($"Screenshot saved to {absolutePath}", new + return Response.Success($"Screenshot saved to {absolutePath}", new @@ - return MCPForUnity.Editor.Helpers.Response.Error($"Failed to capture screenshot: {ex.Message}"); + return Response.Error($"Failed to capture screenshot: {ex.Message}");Also applies to: 223-231
291-295: Call out cleanup behavior to prevent surprises.Note that sync deletes any .py in the destination not managed by PythonToolsAsset. Warn users not to place manual files there.
- Stale files are automatically cleaned up + - Stale files are automatically cleaned up + - Warning: any .py in the destination folder that isn’t managed by a PythonToolsAsset will be deleted during sync
356-361: Align troubleshooting log text with actual messages.ToolSyncService logs “Synced Python tool: .py” per file. Adjust the guidance to look for that line (or add an aggregate summary log if desired).
- - Check Unity Console for sync messages: "Python tools synced: X copied" + - Check Unity Console for: "Synced Python tool: <name>.py"MCPForUnity/Editor/Services/PythonToolRegistryService.cs (2)
1-9: Minor hygiene: remove unused using.System.IO isn’t used in this class.
-using System.IO;
42-53: Normalize line endings before hashing to reduce spurious resyncs across OS.Converts CRLF/LF inconsistencies to a canonical LF prior to hashing.
- using (var sha256 = SHA256.Create()) + using (var sha256 = SHA256.Create()) { - byte[] bytes = System.Text.Encoding.UTF8.GetBytes(file.text); + // Normalize line endings to LF to avoid OS-specific hash churn + string normalized = file.text.Replace("\r\n", "\n"); + byte[] bytes = System.Text.Encoding.UTF8.GetBytes(normalized); byte[] hash = sha256.ComputeHash(bytes); return BitConverter.ToString(hash).Replace("-", "").ToLower(); }MCPForUnity/Editor/Data/PythonToolsAsset.cs (3)
29-33: Filter to real .py assets and de‑dup by GUIDCurrently any TextAsset is accepted; duplicates are possible. Filter by path under Assets/ and .py extension, and de‑dup by GUID to avoid redundant work.
Apply:
- public IEnumerable<TextAsset> GetValidFiles() - { - return pythonFiles.Where(f => f != null); - } + public IEnumerable<TextAsset> GetValidFiles() + { + return pythonFiles + .Where(f => f != null) + .Where(f => + { + var path = UnityEditor.AssetDatabase.GetAssetPath(f); + return !string.IsNullOrEmpty(path) + && path.StartsWith("Assets/", StringComparison.Ordinal) + && path.EndsWith(".py", StringComparison.OrdinalIgnoreCase); + }) + .GroupBy(GetAssetGuid) + .Select(g => g.First()); + }
82-96: OnValidate may trigger redundant second syncRecordSync marks the asset dirty; OnValidate then schedules another SyncAllTools after the first completes, producing an unnecessary pass. Throttle scheduling (e.g., coalesce with a static pending flag) or only schedule when the pythonFiles list actually changes.
22-25: Hide internal state in InspectorfileStates is user-editable despite the “Read-only” hint. Hide to prevent accidental edits while keeping serialization.
- [Header("Sync State (Read-only)")] - [Tooltip("Internal tracking - do not modify")] - public List<PythonFileState> fileStates = new List<PythonFileState>(); + [Header("Sync State (Read-only)")] + [Tooltip("Internal tracking - do not modify")] + [HideInInspector] + public List<PythonFileState> fileStates = new List<PythonFileState>();MCPForUnity/Editor/Helpers/PythonToolSyncProcessor.cs (2)
51-59: Trigger only for project .py changes and handle case‑insensitive extensionsAvoid firing on package or external paths and handle “.PY”.
- foreach (string path in importedAssets.Concat(movedAssets)) + foreach (string path in importedAssets.Concat(movedAssets)) { // Check if any .py files were modified - if (path.EndsWith(".py")) + if (path.StartsWith("Assets/", System.StringComparison.Ordinal) + && path.EndsWith(".py", System.StringComparison.OrdinalIgnoreCase)) { needsSync = true; break; } } @@ - if (!needsSync && deletedAssets.Any(path => path.EndsWith(".py"))) + if (!needsSync && deletedAssets.Any(path => + path.StartsWith("Assets/", System.StringComparison.Ordinal) && + path.EndsWith(".py", System.StringComparison.OrdinalIgnoreCase))) { needsSync = true; }Also applies to: 61-71
94-101: Consider isolating sync into a managed subfolderTo prevent accidental deletion of user files, target tools/custom/unity-managed rather than tools/custom.
- string toolsDir = Path.Combine(srcPath, "tools", "custom"); + string toolsDir = Path.Combine(srcPath, "tools", "custom", "unity-managed");Coordinate with ToolSyncService cleanup accordingly. Based on learnings.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PythonToolRegistryServiceTests.cs (2)
79-91: Test masks empty GUID case; strengthen assertionTextAsset created via
new TextAsset(...)isn’t an asset; GUID is empty. The test asserts “not null” which passes on empty strings. Assert non‑empty and create a real asset to exercise GUID logic.- Assert.IsNotNull(asset.fileStates[0].assetGuid, "GUID should be stored"); + Assert.IsFalse(string.IsNullOrEmpty(asset.fileStates[0].assetGuid), "GUID should be stored");And create a temporary .py file under Assets for the test:
- Write Assets/TempTests/test_tool.py
AssetDatabase.ImportAsset(...)- Load
TextAssetviaAssetDatabase.LoadAssetAtPath<TextAsset>(...)- Clean up in TearDown.
20-27: Add a duplicate‑name collision testAdd a test ensuring two registries referencing different .py assets with the same name are detected and surfaced as an error by ToolSyncService. This guards against silent overwrites.
I can sketch the test if you want it added now.
Also applies to: 93-109
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/screenshots/v6_2_create_python_tools_asset.pngis excluded by!**/*.pngdocs/screenshots/v6_2_python_tools_asset.pngis excluded by!**/*.png
📒 Files selected for processing (30)
MCPForUnity/Editor/Data/McpClients.cs(0 hunks)MCPForUnity/Editor/Data/PythonToolsAsset.cs(1 hunks)MCPForUnity/Editor/Data/PythonToolsAsset.cs.meta(1 hunks)MCPForUnity/Editor/Helpers/PythonToolSyncProcessor.cs(1 hunks)MCPForUnity/Editor/Helpers/PythonToolSyncProcessor.cs.meta(1 hunks)MCPForUnity/Editor/Helpers/ServerInstaller.cs(2 hunks)MCPForUnity/Editor/Importers.meta(1 hunks)MCPForUnity/Editor/Importers/PythonFileImporter.cs(1 hunks)MCPForUnity/Editor/Importers/PythonFileImporter.cs.meta(1 hunks)MCPForUnity/Editor/Services/IPythonToolRegistryService.cs(1 hunks)MCPForUnity/Editor/Services/IPythonToolRegistryService.cs.meta(1 hunks)MCPForUnity/Editor/Services/IToolSyncService.cs(1 hunks)MCPForUnity/Editor/Services/IToolSyncService.cs.meta(1 hunks)MCPForUnity/Editor/Services/MCPServiceLocator.cs(3 hunks)MCPForUnity/Editor/Services/PythonToolRegistryService.cs(1 hunks)MCPForUnity/Editor/Services/PythonToolRegistryService.cs.meta(1 hunks)MCPForUnity/Editor/Services/ToolSyncService.cs(1 hunks)MCPForUnity/Editor/Services/ToolSyncService.cs.meta(1 hunks)MCPForUnity/UnityMcpServer~/src/module_discovery.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/__init__.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/__init__.py(2 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Data.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Data/PythonToolsAssetTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Data/PythonToolsAssetTests.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PythonToolRegistryServiceTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PythonToolRegistryServiceTests.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/ToolSyncServiceTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/ToolSyncServiceTests.cs.meta(1 hunks)docs/CUSTOM_TOOLS.md(6 hunks)
💤 Files with no reviewable changes (1)
- MCPForUnity/Editor/Data/McpClients.cs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: JohanHoltby
PR: CoplayDev/unity-mcp#309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
PR: CoplayDev/unity-mcp#309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
docs/CUSTOM_TOOLS.mdMCPForUnity/Editor/Data/PythonToolsAsset.cs
📚 Learning: 2025-10-03T22:11:46.002Z
Learnt from: msanatan
PR: CoplayDev/unity-mcp#301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
Applied to files:
docs/CUSTOM_TOOLS.md
🧬 Code graph analysis (12)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/ToolSyncServiceTests.cs (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Data/PythonToolsAssetTests.cs (14)
SetUp(13-17)TearDown(19-26)Test(28-34)Test(36-46)Test(48-62)Test(64-73)Test(75-84)Test(86-100)Test(102-115)Test(117-128)Test(130-148)Test(150-173)Test(175-186)Test(188-203)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PythonToolRegistryServiceTests.cs (5)
SetUp(14-18)Test(20-27)Test(29-42)Test(44-57)Test(59-76)
MCPForUnity/UnityMcpServer~/src/resources/__init__.py (1)
MCPForUnity/UnityMcpServer~/src/module_discovery.py (1)
discover_modules(13-55)
MCPForUnity/Editor/Services/PythonToolRegistryService.cs (2)
MCPForUnity/Editor/Data/PythonToolsAsset.cs (3)
IEnumerable(29-32)NeedsSync(37-43)RecordSync(48-62)MCPForUnity/Editor/Services/IPythonToolRegistryService.cs (4)
IEnumerable(9-9)NeedsSync(10-10)ComputeHash(12-12)RecordSync(11-11)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (1)
MCPForUnity/UnityMcpServer~/src/module_discovery.py (1)
discover_modules(13-55)
MCPForUnity/Editor/Services/IPythonToolRegistryService.cs (2)
MCPForUnity/Editor/Data/PythonToolsAsset.cs (2)
NeedsSync(37-43)RecordSync(48-62)MCPForUnity/Editor/Services/PythonToolRegistryService.cs (3)
NeedsSync(27-33)RecordSync(35-40)ComputeHash(42-53)
MCPForUnity/Editor/Services/IToolSyncService.cs (1)
MCPForUnity/Editor/Services/ToolSyncService.cs (1)
ToolSyncResult(19-101)
MCPForUnity/Editor/Helpers/PythonToolSyncProcessor.cs (2)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(8-64)MCPForUnity/Editor/Resources/MenuItems/GetMenuItems.cs (1)
Refresh(49-69)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (2)
MCPForUnity/Editor/Services/PythonToolRegistryService.cs (1)
PythonToolRegistryService(12-54)MCPForUnity/Editor/Services/ToolSyncService.cs (2)
ToolSyncService(10-133)ToolSyncService(14-17)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Data/PythonToolsAssetTests.cs (1)
MCPForUnity/Editor/Data/PythonToolsAsset.cs (1)
CleanupStaleStates(67-71)
MCPForUnity/Editor/Data/PythonToolsAsset.cs (3)
MCPForUnity/Editor/Services/IPythonToolRegistryService.cs (3)
IEnumerable(9-9)NeedsSync(10-10)RecordSync(11-11)MCPForUnity/Editor/Services/PythonToolRegistryService.cs (3)
IEnumerable(14-25)NeedsSync(27-33)RecordSync(35-40)MCPForUnity/Editor/Helpers/PythonToolSyncProcessor.cs (2)
PythonToolSyncProcessor(22-32)SyncAllTools(76-122)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PythonToolRegistryServiceTests.cs (2)
MCPForUnity/Editor/Services/PythonToolRegistryService.cs (1)
PythonToolRegistryService(12-54)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Data/PythonToolsAssetTests.cs (12)
Test(28-34)Test(36-46)Test(48-62)Test(64-73)Test(75-84)Test(86-100)Test(102-115)Test(117-128)Test(130-148)Test(150-173)Test(175-186)Test(188-203)
MCPForUnity/Editor/Services/ToolSyncService.cs (5)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(8-64)MCPForUnity/Editor/Services/IToolSyncService.cs (2)
ToolSyncResult(5-12)ToolSyncResult(16-16)MCPForUnity/Editor/Data/PythonToolsAsset.cs (3)
NeedsSync(37-43)RecordSync(48-62)CleanupStaleStates(67-71)MCPForUnity/Editor/Services/IPythonToolRegistryService.cs (2)
NeedsSync(10-10)RecordSync(11-11)MCPForUnity/Editor/Services/PythonToolRegistryService.cs (2)
NeedsSync(27-33)RecordSync(35-40)
🪛 Ruff (0.14.0)
MCPForUnity/UnityMcpServer~/src/module_discovery.py
34-34: Do not catch blind exception: Exception
(BLE001)
53-53: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (19)
MCPForUnity/Editor/Importers.meta (1)
1-8: No actionable review for Unity metadata file.This is an auto-generated Unity metadata file that defines folder-level asset configuration. The structure is correct and follows Unity standards. No functional code or logic to review.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Data.meta (1)
1-8: Standard Unity metadata file—looks good.This is a standard auto-generated folder metadata file for the test data directory. No concerns.
MCPForUnity/UnityMcpServer~/src/module_discovery.py (1)
13-55: Good refactoring to centralize module discovery.The implementation correctly discovers modules at both the top level and one level deep in subdirectories, with appropriate filtering of private modules and error handling. The broad
Exceptioncatches flagged by Ruff are actually appropriate here—import failures can stem from various causes (syntax errors, missing dependencies, etc.), and the discovery system should continue loading other modules rather than halt entirely.One minor suggestion: consider validating that
base_direxists and is a directory before attempting discovery, to provide clearer error messages if misconfigured.MCPForUnity/UnityMcpServer~/src/resources/__init__.py (1)
11-11: Clean refactoring to use centralized module discovery.The changes correctly replace the previous inline discovery logic with the new
discover_modulesutility, and the docstring accurately reflects that subdirectories are now included in discovery. Usinglist()to consume the generator ensures all modules are imported before proceeding with registration.Also applies to: 23-23, 30-31
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (1)
11-11: Clean refactoring mirroring the resources discovery pattern.The changes are consistent with the resources module refactoring, replacing inline discovery logic with the centralized
discover_modulesutility. The docstring update correctly documents the subdirectory discovery capability.Also applies to: 23-23, 30-31
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)
68-68: LGTM!The added blank lines improve code readability without affecting functionality.
Also applies to: 414-414
MCPForUnity/Editor/Services/PythonToolRegistryService.cs.meta (1)
1-11: Meta file looks standard.GUID committed; no concerns.
MCPForUnity/Editor/Data/PythonToolsAsset.cs.meta (1)
1-11: Meta file OK.No action needed.
MCPForUnity/Editor/Services/IPythonToolRegistryService.cs.meta (1)
1-11: Interface meta looks good.Consistent MonoImporter settings.
MCPForUnity/Editor/Services/ToolSyncService.cs.meta (1)
1-11: Service meta verified.Nothing to change.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PythonToolRegistryServiceTests.cs.meta (1)
1-11: Test meta fine.Committed GUID; all good.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Data/PythonToolsAssetTests.cs (1)
1-204: Nice coverage of asset behaviors.Tests exercise filtering, hashing toggles, state add/update, and cleanup paths. Looks solid.
MCPForUnity/Editor/Services/IPythonToolRegistryService.cs (1)
7-13: Interface contract is tight and clear.Methods map 1:1 to asset operations and hashing. LGTM.
MCPForUnity/Editor/Services/MCPServiceLocator.cs (2)
13-15: Service wiring looks correct.Lazy init + Register/Reset updates are consistent. No issues.
Also applies to: 20-23, 53-56, 60-63
37-42: Review comment is incorrect; no typo exists in the code.The verification shows that
ToolSyncService.csline 28 already uses the correct camelCase naming_registryService, not snake_case_registry_service. The field is declared on line 12 as_registryServiceand used consistently in camelCase throughout the file (lines 16, 28, 49, 57). There is no CS0103 compile error and no fix is needed.Likely an incorrect or invalid review comment.
MCPForUnity/Editor/Services/PythonToolRegistryService.cs (1)
14-25: Registry discovery and state updates look solid.AssetDatabase queries and SetDirty usage are appropriate for Editor context. No issues.
Also applies to: 27-40
MCPForUnity/Editor/Services/ToolSyncService.cs (1)
39-41: AssetDatabase batching is fine hereStart/StopAssetEditing bracketing is correct to minimize reimports; SaveAssets afterwards is appropriate for registry changes.
Ensure no code inside the try writes back into Assets/ to avoid long import stalls during large syncs.
Also applies to: 87-93
MCPForUnity/Editor/Helpers/PythonToolSyncProcessor.cs (1)
144-161: Editor UX helpers look goodReimportPythonFiles/ManualSync/ToggleAutoSync are straightforward and useful. Nice touch to default auto‑sync to on.
Also applies to: 166-171, 176-181
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PythonToolRegistryServiceTests.cs (1)
59-76: Happy path hash tests are goodHash compute + NeedsSync/RecordSync flow is covered. Nice.
| public bool NeedsSync(TextAsset file, string currentHash) | ||
| { | ||
| if (!useContentHashing) return true; // Always sync if hashing disabled | ||
|
|
||
| var state = fileStates.FirstOrDefault(s => s.assetGuid == GetAssetGuid(file)); | ||
| return state == null || state.contentHash != currentHash; | ||
| } |
There was a problem hiding this comment.
Non‑asset TextAssets get empty GUIDs → state collisions
AssetDatabase.GetAssetPath on non‑asset TextAssets returns empty, so AssetPathToGUID is empty. Multiple unsaved TextAssets then share the same key, corrupting fileStates. Guard and skip non‑asset entries.
- public bool NeedsSync(TextAsset file, string currentHash)
+ public bool NeedsSync(TextAsset file, string currentHash)
{
- if (!useContentHashing) return true; // Always sync if hashing disabled
-
- var state = fileStates.FirstOrDefault(s => s.assetGuid == GetAssetGuid(file));
+ if (!useContentHashing) return true; // Always sync if hashing disabled
+ if (!TryGetAssetGuid(file, out var guid)) return true; // treat as needing sync but don't record
+ var state = fileStates.FirstOrDefault(s => s.assetGuid == guid);
return state == null || state.contentHash != currentHash;
}
@@
- public void RecordSync(TextAsset file, string hash)
+ public void RecordSync(TextAsset file, string hash)
{
- string guid = GetAssetGuid(file);
- var state = fileStates.FirstOrDefault(s => s.assetGuid == guid);
+ if (!TryGetAssetGuid(file, out var guid))
+ {
+ Debug.LogWarning($"[{nameof(PythonToolsAsset)}] Skipping sync state for non-asset TextAsset '{file?.name}'. Save it under Assets/ to track.");
+ return;
+ }
+ var state = fileStates.FirstOrDefault(s => s.assetGuid == guid);
@@
- private string GetAssetGuid(TextAsset asset)
- {
- return UnityEditor.AssetDatabase.AssetPathToGUID(UnityEditor.AssetDatabase.GetAssetPath(asset));
- }
+ private string GetAssetGuid(TextAsset asset)
+ {
+ return UnityEditor.AssetDatabase.AssetPathToGUID(UnityEditor.AssetDatabase.GetAssetPath(asset));
+ }
+
+ private bool TryGetAssetGuid(TextAsset asset, out string guid)
+ {
+ guid = UnityEditor.AssetDatabase.AssetPathToGUID(UnityEditor.AssetDatabase.GetAssetPath(asset));
+ return !string.IsNullOrEmpty(guid);
+ }Also applies to: 48-62, 73-76
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Data/PythonToolsAsset.cs around lines 37-43 (also apply
same check to ranges 48-62 and 73-76): AssetDatabase.GetAssetPath returns an
empty path for non-asset/unsaved TextAsset which makes AssetPathToGUID return an
empty GUID, causing collisions in fileStates; modify methods that call
GetAssetGuid/GetAssetPath to detect empty asset paths (or empty GUIDs) and skip
processing those TextAssets (return false from NeedsSync, or do not add/update
entries in fileStates) so non-asset TextAssets are ignored. Ensure the guard is
applied consistently in all three affected ranges so you never use an empty GUID
as a key.
There was a problem hiding this comment.
This is super unlikely, devs are just going to drag Python files the scriptable object, or add them to the list directly.
There was a problem hiding this comment.
@msanatan, you're right—in the typical workflow where developers drag Python asset files from the Project window into the inspector, those will always have valid GUIDs. I understand the defensive guard isn't needed for the expected use case. Thanks for clarifying! 🐰
🧠 Learnings used
Learnt from: JohanHoltby
PR: CoplayDev/unity-mcp#309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
| public static IPythonToolRegistryService PythonToolRegistry => _pythonToolRegistryService ??= new PythonToolRegistryService(); | ||
| public static ITestRunnerService Tests => _testRunnerService ??= new TestRunnerService(); | ||
| public static IToolSyncService ToolSync => _toolSyncService ??= new ToolSyncService(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Name collision risk in sync output (cross-file).
ToolSyncService writes to dest as file.name + ".py". Two assets with same file name will overwrite each other, and cleanup may delete the other’s file. Consider namespacing by asset or GUID.
- string destPath = Path.Combine(destToolsDir, file.name + ".py");
+ var fileGuid = AssetDatabase.AssetPathToGUID(AssetDatabase.GetAssetPath(file));
+ string safeSuffix = fileGuid.Substring(0, 8);
+ string destPath = Path.Combine(destToolsDir, $"{file.name}.{safeSuffix}.py");Also update the else-branch and syncedFiles set similarly, and keep cleanup keyed to these paths.
🏁 Script executed:
#!/bin/bash
# Grep current destPath construction for both branches to update consistently
rg -n -C1 $'Path\\.Combine\\(destToolsDir,\\s*file\\.name\\s*\\+\\s*\"\\.py\"\\)' MCPForUnity/Editor/Services/ToolSyncService.csLength of output: 354
🏁 Script executed:
#!/bin/bash
# Get broader context around both occurrences to understand function structure
rg -n -B10 -A5 'string destPath = Path\.Combine\(destToolsDir, file\.name \+ "\.py"\)' MCPForUnity/Editor/Services/ToolSyncService.csLength of output: 1571
🏁 Script executed:
#!/bin/bash
# Search for cleanup or removal logic that might reference these synced paths
rg -n -A3 -B3 'syncedFiles' MCPForUnity/Editor/Services/ToolSyncService.csLength of output: 1226
🏁 Script executed:
#!/bin/bash
# Check for any GUID or asset path references already in use
rg -n 'AssetDatabase\|GetAssetPath\|AssetPathToGUID\|GUID' MCPForUnity/Editor/Services/ToolSyncService.csLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find CleanupStaleFiles method implementation
rg -n -A20 'void CleanupStaleFiles' MCPForUnity/Editor/Services/ToolSyncService.csLength of output: 959
🏁 Script executed:
#!/bin/bash
# Find the file object type - check the registry interface/implementation
rg -n -B5 'foreach.*file.*registry\.GetValidFiles' MCPForUnity/Editor/Services/ToolSyncService.cs | head -20Length of output: 256
🏁 Script executed:
#!/bin/bash
# Search for IPythonToolRegistry or related interface to understand file object
rg -n 'interface.*Registry\|class.*Registry' MCPForUnity/Editor/Services/ --type csLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find registry interface and file type - broader search
rg -n 'GetValidFiles\|interface IPython' MCPForUnity/ -A3 --type csLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check imports in ToolSyncService.cs
head -30 MCPForUnity/Editor/Services/ToolSyncService.csLength of output: 899
🏁 Script executed:
#!/bin/bash
# Search for file object definition or property
rg -n 'class.*PythonTool\|struct.*PythonTool' MCPForUnity/ --type csLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for service implementations and interfaces more broadly
fd -t f -e cs . MCPForUnity/Editor/Services/ | head -20Length of output: 682
🏁 Script executed:
#!/bin/bash
# Search for GetValidFiles definition
rg -n 'GetValidFiles' MCPForUnity/ --type cs -A5Length of output: 1576
🏁 Script executed:
#!/bin/bash
# Search for properties like .name and .text on potential file types
rg -n '\.name.*\.text\|file\.name\|file\.text' MCPForUnity/ --type cs -B3 -A1 | head -40Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get GetAssetGuid implementation and nearby code
rg -n -B3 -A8 'private string GetAssetGuid' MCPForUnity/Editor/Data/PythonToolsAsset.csLength of output: 471
🏁 Script executed:
#!/bin/bash
# Check IPythonToolRegistryService interface for available methods
cat MCPForUnity/Editor/Services/IPythonToolRegistryService.csLength of output: 423
Fix name collision in ToolSyncService file naming to prevent overwrites and stale cleanup issues.
The collision risk is valid: multiple TextAssets with identical names overwrite each other's destination path (lines 51 and 65 both construct Path.Combine(destToolsDir, file.name + ".py")). Since only the final written file path enters syncedFiles, CleanupStaleFiles will incorrectly delete the overwritten files as "stale."
The suggested GUID-based namespacing is sound and implementable. Note that PythonToolsAsset already has a private GetAssetGuid method using the same pattern, but since it's not exposed through IPythonToolRegistryService, the inline approach in the suggestion is the viable path. Ensure both branches (lines 51 and 65) and the cleanup keying use the namespaced paths consistently.
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Services/MCPServiceLocator.cs around lines 20-23 (and
update usages at lines ~51 and ~65 in the ToolSyncService implementation), there
is a filename collision risk because TextAssets are written to destToolsDir
using only file.name + ".py", which causes overwrites and makes
CleanupStaleFiles delete overwritten assets as stale; fix it by namespacing
generated filenames with a stable GUID per asset (e.g., compute or derive the
asset GUID similarly to PythonToolsAsset.GetAssetGuid and prepend or append it
to file.name), apply the same GUID-based naming on both branches where files are
written, and ensure the same namespaced path/key is what gets stored in
syncedFiles so CleanupStaleFiles uses the correct keys to identify stale files
consistently.
There was a problem hiding this comment.
It's OK to expect tools to have different names
There was a problem hiding this comment.
@msanatan, understood! That's a reasonable constraint for the tool system. I'll note that unique tool names are expected.
| foreach (var registry in registries) | ||
| { | ||
| foreach (var file in registry.GetValidFiles()) | ||
| { | ||
| try | ||
| { | ||
| // Check if needs syncing (hash-based or always) | ||
| if (_registryService.NeedsSync(registry, file)) | ||
| { | ||
| string destPath = Path.Combine(destToolsDir, file.name + ".py"); | ||
|
|
||
| // Write the Python file content | ||
| File.WriteAllText(destPath, file.text); | ||
|
|
||
| // Record sync | ||
| _registryService.RecordSync(registry, file); | ||
|
|
||
| result.CopiedCount++; | ||
| syncedFiles.Add(destPath); | ||
| McpLog.Info($"Synced Python tool: {file.name}.py"); | ||
| } | ||
| else | ||
| { | ||
| string destPath = Path.Combine(destToolsDir, file.name + ".py"); | ||
| syncedFiles.Add(destPath); | ||
| result.SkippedCount++; |
There was a problem hiding this comment.
Detect duplicates and sanitize file names before writing
Two tools with the same asset name will overwrite each other silently; invalid chars may also break paths. Sanitize and enforce uniqueness.
- foreach (var file in registry.GetValidFiles())
+ var seen = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
+ foreach (var file in registry.GetValidFiles())
{
@@
- if (_registryService.NeedsSync(registry, file))
+ var safeName = SanitizeFileName(file.name);
+ string destPath = Path.Combine(destToolsDir, safeName + ".py");
+ if (!seen.Add(destPath))
+ {
+ result.ErrorCount++;
+ result.Messages.Add($"Duplicate tool name '{safeName}.py' across registries; skipping subsequent copies.");
+ continue;
+ }
+ if (_registryService.NeedsSync(registry, file))
{
- string destPath = Path.Combine(destToolsDir, file.name + ".py");
-
// Write the Python file content
- File.WriteAllText(destPath, file.text);
+ File.WriteAllText(destPath, file.text, new System.Text.UTF8Encoding(encoderShouldEmitUTF8Identifier: false));
@@
- string destPath = Path.Combine(destToolsDir, file.name + ".py");
syncedFiles.Add(destPath);Add helper:
}
-}
+ private static string SanitizeFileName(string name)
+ {
+ foreach (var c in System.IO.Path.GetInvalidFileNameChars())
+ name = name.Replace(c, '_');
+ return name.Trim();
+ }
+}🤖 Prompt for AI Agents
In MCPForUnity/Editor/Services/ToolSyncService.cs around lines 42 to 67, the
current loop writes files using raw asset names which can contain invalid path
chars and can collide when two tools share a name; update the code to sanitize
file names (remove/replace invalid path characters and trim whitespace) and
enforce uniqueness before writing to destToolsDir by generating a safe base name
and, if that name already exists in syncedFiles (or on disk), append a
deterministic disambiguator (incrementing counter or short hash of asset id) to
the file name; implement a small helper method (e.g.,
GetUniqueSanitizedFilename(registry, file, destDir, HashSet<string> existing))
that returns the safe, unique file name and use it for both the copy and the
syncedFiles tracking so duplicates are not overwritten and paths remain valid.
There was a problem hiding this comment.
As mentioned before, it's OK to expect tools to have different names
There was a problem hiding this comment.
@msanatan Understood! If tools are expected to have unique names as part of the usage pattern, then the additional duplicate detection isn't necessary. Thank you for clarifying the design decision.
| private void CleanupStaleFiles(string destToolsDir, HashSet<string> currentFiles) | ||
| { | ||
| try | ||
| { | ||
| if (!Directory.Exists(destToolsDir)) return; | ||
|
|
||
| // Find all .py files in destination that aren't in our current set | ||
| var existingFiles = Directory.GetFiles(destToolsDir, "*.py"); | ||
|
|
||
| foreach (var file in existingFiles) | ||
| { | ||
| if (!currentFiles.Contains(file)) | ||
| { | ||
| try | ||
| { | ||
| File.Delete(file); | ||
| McpLog.Info($"Cleaned up stale tool: {Path.GetFileName(file)}"); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| McpLog.Warn($"Failed to cleanup {file}: {ex.Message}"); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| McpLog.Warn($"Failed to cleanup stale files: {ex.Message}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Cleanup deletes any .py in destToolsDir, including user‑managed files
Removing all non‑current .py files in tools/custom risks deleting hand‑placed tools or files managed outside this sync. Constrain cleanup to a dedicated subfolder (e.g., tools/custom/unity-managed) or manage a manifest of files created by this service and delete only those.
Follow‑up: If you adopt a subfolder, update PythonToolSyncProcessor to write into that subfolder and keep current broad cleanup.
There was a problem hiding this comment.
Fair enough, but it's very unlikely that the user will put something else there
There was a problem hiding this comment.
@msanatan, that's a fair point—tools/custom is specifically intended for this sync workflow, and users have the PythonToolsAsset system for adding tools properly. If someone manually places files there, they're working outside the intended pattern.
I'll accept your judgment on this. The risk is low given the specific directory and clear workflow.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/ToolSyncServiceTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
MCPForUnity/Editor/Helpers/PythonToolSyncProcessor.cs (1)
147-157: Consider usingAssetDatabase.FindAssetsfor better performance.While the current approach works correctly, iterating all assets can be slow on large projects. Consider using
AssetDatabase.FindAssetswith a path filter for better performance:-string[] allAssets = AssetDatabase.GetAllAssetPaths(); -int count = 0; - -foreach (string path in allAssets) -{ - if (path.EndsWith(".py") && path.StartsWith("Assets/")) - { - AssetDatabase.ImportAsset(path, ImportAssetOptions.ForceUpdate); - count++; - } -} +var pythonGuids = AssetDatabase.FindAssets("t:DefaultAsset", new[] { "Assets" }) + .Select(AssetDatabase.GUIDToAssetPath) + .Where(path => path.EndsWith(".py")) + .ToArray(); + +foreach (string path in pythonGuids) +{ + AssetDatabase.ImportAsset(path, ImportAssetOptions.ForceUpdate); +} + +int count = pythonGuids.Length;Since this is a manual action, the current implementation is acceptable, but the optimization would improve responsiveness on larger projects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Helpers/PythonToolSyncProcessor.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
MCPForUnity/Editor/Helpers/PythonToolSyncProcessor.cs (1)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(8-64)
🔇 Additional comments (8)
MCPForUnity/Editor/Helpers/PythonToolSyncProcessor.cs (8)
1-20: LGTM!The class structure and attributes are well-designed for Unity's asset pipeline. The
InitializeOnLoadandAssetPostprocessorcombination appropriately enables both startup synchronization and automatic monitoring of Python file changes.
22-32: LGTM!Using
EditorApplication.delayCallto defer the startup sync is the correct Unity pattern, ensuring the sync runs after Unity finishes initialization.
37-71: LGTM!The asset monitoring logic correctly guards against re-entrancy and efficiently detects Python file changes across import, move, and delete operations.
76-122: LGTM!The synchronization logic is well-structured with appropriate re-entrancy protection, error handling, and logging. The
try-finallyblock ensures the_isSyncingflag is always reset, preventing deadlock scenarios.
127-139: LGTM!The auto-sync enable/disable methods correctly use
EditorPrefsfor persistence, with an appropriate default oftrueto match the PR's intent of automatic synchronization by default.
166-171: LGTM!The manual sync menu item provides a clear way for users to trigger synchronization on demand.
176-180: LGTM!The toggle implementation correctly inverts the current state.
185-190: Past issue resolved - paths now match correctly!The menu path in
Menu.SetChecked(line 188) now correctly matches theMenuItemattribute path (line 176), so the checkbox will toggle properly. The previous review concern has been addressed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
MCPForUnity/Editor/Services/IPythonToolRegistryService.cs (1)
7-13: Consider adding XML documentation for the public API.To improve maintainability and developer experience, add XML documentation comments to clarify the contract:
GetAllRegistries: What registries are discovered and from whereNeedsSync: Return value meaning (true = needs sync, false = up-to-date) and behavior when hashing is disabledRecordSync: Side effects (updates registry state, marks asset dirty)ComputeHash: Return format (SHA-256 as lowercase hex) and null handling (returns empty string)Example:
+ /// <summary> + /// Service for managing Python tool registry assets and synchronization state. + /// </summary> public interface IPythonToolRegistryService { + /// <summary> + /// Discovers all PythonToolsAsset instances in the project. + /// </summary> + /// <returns>Enumerable of all registry assets found via AssetDatabase.</returns> IEnumerable<PythonToolsAsset> GetAllRegistries(); + + /// <summary> + /// Determines whether a Python file needs synchronization to the server. + /// </summary> + /// <param name="registry">The registry tracking the file state.</param> + /// <param name="file">The Python file to check.</param> + /// <returns>True if the file needs sync (hash mismatch or hashing disabled); otherwise false.</returns> bool NeedsSync(PythonToolsAsset registry, TextAsset file); + + /// <summary> + /// Records that a file has been synchronized, updating its hash and timestamp. + /// Marks the registry asset dirty to persist changes. + /// </summary> + /// <param name="registry">The registry to update.</param> + /// <param name="file">The synchronized Python file.</param> void RecordSync(PythonToolsAsset registry, TextAsset file); + + /// <summary> + /// Computes a SHA-256 hash of the file content. + /// </summary> + /// <param name="file">The file to hash.</param> + /// <returns>Lowercase hexadecimal SHA-256 hash, or empty string if file is null or empty.</returns> string ComputeHash(TextAsset file); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Helpers/PythonToolSyncProcessor.cs(1 hunks)MCPForUnity/Editor/Services/IPythonToolRegistryService.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/ToolSyncServiceTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- MCPForUnity/Editor/Helpers/PythonToolSyncProcessor.cs
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/ToolSyncServiceTests.cs
🧰 Additional context used
🧬 Code graph analysis (1)
MCPForUnity/Editor/Services/IPythonToolRegistryService.cs (2)
MCPForUnity/Editor/Data/PythonToolsAsset.cs (2)
NeedsSync(37-43)RecordSync(48-62)MCPForUnity/Editor/Services/PythonToolRegistryService.cs (3)
NeedsSync(27-33)RecordSync(35-40)ComputeHash(42-53)
🔇 Additional comments (1)
MCPForUnity/Editor/Services/IPythonToolRegistryService.cs (1)
7-13: LGTM! Clean interface design.The interface provides a well-structured contract for managing Python tool registries with clear responsibilities: discovery (
GetAllRegistries), synchronization checks (NeedsSync), state tracking (RecordSync), and content hashing (ComputeHash). The design follows SOLID principles and enables dependency injection and testing.
2 participants
One of the biggest problems facing users is adding tools. The Unity plugin dynamically gets tools with the
McpForUnityToolattribute. However, Python changes require that users add tools in the server's location. This works well if users install the package locally, however, only developers do that.We need a way for users to install tools, including the necessary Python code, until we can add a tool in pure C# (see #293)
Closes #308 and closes #309
Summary by CodeRabbit
New Features
Documentation
Tests