feat: Enhance MCP tools and add test framework #15
Conversation
Key improvements across multiple tools: RunTestsTool: - Fix async implementation using ICallbacks - Add proper validation for test modes - Improve test result reporting - Add checks for when no tests match criteria SelectGameObjectTool: - Fix namespace for Resources.FindObjectsOfTypeAll - Add validation for GameObject existence - Improve error messages for non-existent objects - Support both path and name-based GameObject lookup UpdateComponentTool: - Add comprehensive field validation - Track and report invalid fields with reasons - Validate field existence and accessibility - Add type compatibility checking - Improve error messages with detailed feedback These changes make the tools more robust and user-friendly by: - Providing clear error messages - Preventing silent failures - Maintaining partial updates where possible - Following Unity best practices for object lookup
Added support for finding resources by URI in addition to resource name. This fixes compatibility with Claude Code MCP client, which sends requests using the URI ('unity://hierarchy') instead of the resource name ('get_hierarchy').
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
📝 WalkthroughWalkthroughThis pull request updates various configuration files, tool implementations, and testing infrastructure. Modifications include enhanced error handling and logging in asset, message, and component update tools; refactoring of test execution logic with counters and structured responses; improvements in resource lookup and message handling in Unity bridge components; updates to documentation, package configuration, and project naming; and the introduction of new test files, assembly definitions, and metadata for both Editor and Runtime tests. Changes
Sequence Diagram(s)Resource Request Handling in McpUnitySocketHandlersequenceDiagram
participant U as Unity
participant H as McpUnitySocketHandler
participant R as Resource Lookup
U->>H: Send raw message
H->>H: Log raw message and parse JSON
alt Unknown method
H->>R: FindResourceByUri(uri)
R-->>H: Resource Found / Not Found
alt Resource Found
H->>H: Initiate resource fetch coroutine
else Resource Not Found
H->>H: Return error response ("unknown method")
end
else Known method
H->>H: Process message normally
end
H->>H: Try sending response (with try-catch logging)
Test Execution Flow in RunTestsToolsequenceDiagram
participant U as User
participant RT as RunTestsTool
participant L as Logger
U->>RT: Call ExecuteAsync(parameters, tcs)
RT->>RT: Initialize counters & clear results
RT->>RT: Validate test mode and set completion source
RT->>RT: Run tests (TestStarted, TestFinished events)
note right of RT: Counters (_testCount, _passCount, _failCount) updated
RT->>RT: Call RunFinished(testResults)
RT->>L: Log summary and create response object
RT-->>U: Return test execution summary
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (15)
Tests/README.md (1)
1-27: Documentation for test structure and setup looks good.The README provides clear instructions for working with the test framework, especially addressing the common issue of test discovery with local packages in Unity. The workaround instructions are practical and will help developers properly run the tests.
One minor formatting issue to fix:
-Tests are organized following Unity's package layout guidelines: - -- `Editor`: Contains tests that run in Edit mode -- `Runtime`: Contains tests that run in Play mode (currently not implemented) +Tests are organized following Unity's package layout guidelines: + +- `Editor`: Contains tests that run in Edit mode +- `Runtime`: Contains tests that run in Play mode (currently not implemented)🧰 Tools
🪛 LanguageTool
[uncategorized] ~24-~24: Loose punctuation mark.
Context: ...s package layout guidelines: -Editor: Contains tests that run in Edit mode - ...(UNLIKELY_OPENING_PUNCTUATION)
README.md (1)
1-4: Improve Grammatical Consistency in the Package Description
In line 3, consider updating the text for better grammar. For example, change “a MCP Unity Server” to “an MCP Unity Server” and “request Editor information” to “requesting Editor information.”-The purpose of this package is to provide a MCP Unity Server for executing Unity operations and request Editor information from AI MCP enabled hosts. +The purpose of this package is to provide an MCP Unity Server for executing Unity operations and requesting Editor information from AI MCP enabled hosts.🧰 Tools
🪛 LanguageTool
[misspelling] ~3-~3: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...e purpose of this package is to provide a MCP Unity Server for executing Unity op...(EN_A_VS_AN)
Tests/Editor/SimpleTest.cs (2)
4-4: Consider standardizing namespace across test files.I notice this file uses
McpUnity.Testsnamespace, while according to the summary, there's an assembly definition forMcpUnity.Editor.Tests. For consistency, consider standardizing the namespace pattern across all test files.-namespace McpUnity.Tests +namespace McpUnity.Editor.Tests
8-13: Test is similar to other baseline tests.This test follows the same pattern as other tests like SimplePlayModeTest, BasicTests, TestRunner, and AddAssetToSceneToolTests. This consistency is good, but consider adding more meaningful tests that actually exercise the functionality of MCP tools as mentioned in the PR objectives.
Tests/Editor/TestRunner.cs (1)
10-15: Basic test framework validationThis test simply verifies that the test framework itself is working. While useful as an initial verification, consider expanding this to test actual functionality from the enhanced tools mentioned in the PR objectives.
Tests/McpTests.md (2)
5-7: Grammatical improvement needed in step 1There's a minor preposition issue in step 1.
-1. In your main project, create a new folder at `Assets/McpTests` +1. In your main project, create a new folder in `Assets/McpTests`🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: The preposition ‘to’ seems more likely in this position.
Context: ... your main project, create a new folder atAssets/McpTests2. Copy the test file...(AI_HYDRA_LEO_REPLACE_AT_TO)
3-41: Missing validation process for the workaroundWhile the document provides good instructions, it doesn't include a way for users to verify that the workaround was successful beyond checking if tests appear in the Test Runner. Consider adding a troubleshooting section.
This workaround is necessary because Unity sometimes has difficulty discovering tests in local packages. When the package is published to a registry, this workaround should not be needed. +## Troubleshooting + +If tests still do not appear after following these steps: + +1. Check the Unity console for any compilation errors +2. Verify that the Assembly Definition file was saved correctly +3. Try restarting Unity +4. Ensure that your test files contain properly formatted test classes with the `[Test]` attribute🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: The preposition ‘to’ seems more likely in this position.
Context: ... your main project, create a new folder atAssets/McpTests2. Copy the test file...(AI_HYDRA_LEO_REPLACE_AT_TO)
Editor/UnityBridge/McpUnitySocketHandler.cs (2)
224-229: Potential performance enhancement for resource lookupsFor efficiency in larger systems, consider replacing the linear search with a dictionary-based lookup for resources by URI.
/// <summary> /// Find a resource by its URI /// </summary> /// <param name="uri">The URI to search for</param> /// <returns>The resource if found, null otherwise</returns> private McpResourceBase FindResourceByUri(string uri) { - // Get resources from the server - var resources = _server.Resources; - - // Look for a resource with a matching URI - foreach (var resource in resources.Values) - { - if (resource.Uri == uri) - { - Debug.Log($"[MCP Unity] Found resource {resource.Name} by URI {uri}"); - return resource; - } - } + // Short-circuit if URI is null or empty + if (string.IsNullOrEmpty(uri)) + { + return null; + } + + // Cache URI-to-resource mapping if used frequently + // For now, do a linear search + foreach (var resource in _server.Resources.Values) + { + if (uri.Equals(resource.Uri, StringComparison.OrdinalIgnoreCase)) + { + Debug.Log($"[MCP Unity] Found resource {resource.Name} by URI {uri}"); + return resource; + } + } return null; }
102-109: Consider more robust error handling for send failuresWhile the current error handling is good, you might want to consider implementing a retry mechanism or connection health check after a failed send.
Editor/Tools/AddAssetToSceneTool.cs (1)
116-121: Enhanced error handling for missing parent GameObject.This change improves the tool behavior by returning a clear error response instead of silently adding the asset to the scene root when the parent isn't found. The error message now includes whether the parent was identified by ID or path, making debugging easier.
Consider logging an error message before returning the error response to ensure the console has a record of this issue:
// Parent not found, return an error instead of adding to root string identifier = parentId.HasValue ? $"ID {parentId.Value}" : $"path '{parentPath}'"; +Debug.LogError($"[MCP Unity] Failed to add asset '{asset.name}' to scene: Parent GameObject not found with {identifier}"); return McpUnitySocketHandler.CreateErrorResponse( $"Parent GameObject not found with {identifier}", "parent_not_found" );Editor/Tools/UpdateComponentTool.cs (1)
64-68: Reflection-based component type look-up.
Though it works for an Editor tool, consider caching these lookups if performance becomes a concern.Server/build/unity/mcpUnity.js (1)
93-119: Consider potential PII exposure in logs.
These new debug statements greatly aid troubleshooting, but they may inadvertently log sensitive data in raw messages. Consider introducing a mechanism to sanitize or redact PII when logging at debug level.Server/src/unity/mcpUnity.ts (1)
135-163: Take care when logging raw messages.
Similar to the .js file, logging raw messages and stringifying results is useful for debugging but may expose sensitive data. Ensure that no PII or credentials are logged inadvertently.Editor/Tools/RunTestsTool.cs (1)
43-71: Initialization and parameter parsing in ExecuteAsync.
Resetting counters and validating thetestModelogic is well-structured. Consider adding a concurrency check if a second test run is triggered while the first is still in progress.Tests.meta (1)
1-9: Compliant Folder Metadata FileThe file declares a
fileFormatVersionand a uniqueguidand correctly flags the folder asset withfolderAsset: yes. TheDefaultImportersection is present with empty values, which is acceptable for now.Suggestion: Ensure that the empty fields (especially
userData) do not unintentionally include trailing spaces or unwanted defaults.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
.gitignore(1 hunks)Editor/Services/TestRunnerService.cs(1 hunks)Editor/Tests.meta(1 hunks)Editor/Tools/AddAssetToSceneTool.cs(1 hunks)Editor/Tools/NotifyMessageTool.cs(2 hunks)Editor/Tools/RunTestsTool.cs(2 hunks)Editor/Tools/SelectGameObjectTool.cs(1 hunks)Editor/Tools/UpdateComponentTool.cs(2 hunks)Editor/UnityBridge/McpUnityServer.cs(1 hunks)Editor/UnityBridge/McpUnitySocketHandler.cs(3 hunks)README.md(1 hunks)Server/build/tools/selectGameObjectTool.js(1 hunks)Server/build/unity/mcpUnity.js(1 hunks)Server/src/tools/selectGameObjectTool.ts(3 hunks)Server/src/unity/mcpUnity.ts(1 hunks)Tests.meta(1 hunks)Tests/.gitignore(1 hunks)Tests/Editor/AddAssetToSceneToolTests.cs(1 hunks)Tests/Editor/AddAssetToSceneToolTests.cs.meta(1 hunks)Tests/Editor/BasicTests.cs(1 hunks)Tests/Editor/BasicTests.cs.meta(1 hunks)Tests/Editor/McpUnity.Tests.Editor.asmdef(1 hunks)Tests/Editor/SimpleTest.cs(1 hunks)Tests/Editor/SimpleTest.cs.meta(1 hunks)Tests/Editor/TestRunner.cs(1 hunks)Tests/Editor/TestRunner.cs.meta(1 hunks)Tests/McpTests.md(1 hunks)Tests/README.md(1 hunks)Tests/Runtime/McpUnity.Tests.Runtime.asmdef(1 hunks)Tests/Runtime/SimplePlayModeTest.cs(1 hunks)Tests/Runtime/SimplePlayModeTest.cs.meta(1 hunks)package.json(1 hunks)
🧰 Additional context used
🧬 Code Definitions (12)
Tests/Editor/BasicTests.cs (3)
Tests/Editor/AddAssetToSceneToolTests.cs (2)
Test(13-18)Test(20-25)Tests/Editor/SimpleTest.cs (1)
Test(8-13)Tests/Editor/TestRunner.cs (2)
Test(10-15)Test(17-22)
Tests/Editor/SimpleTest.cs (4)
Tests/Editor/AddAssetToSceneToolTests.cs (2)
Test(13-18)Test(20-25)Tests/Editor/BasicTests.cs (1)
Test(8-13)Tests/Editor/TestRunner.cs (2)
Test(10-15)Test(17-22)Tests/Runtime/SimplePlayModeTest.cs (1)
Test(11-16)
Editor/Tools/AddAssetToSceneTool.cs (1)
Editor/UnityBridge/McpUnitySocketHandler.cs (2)
McpUnitySocketHandler(20-260)McpUnitySocketHandler(27-30)
Editor/Tools/SelectGameObjectTool.cs (3)
Editor/Tools/UpdateComponentTool.cs (2)
GameObject(182-222)JObject(28-175)Editor/UnityBridge/McpUnitySocketHandler.cs (4)
McpUnitySocketHandler(20-260)McpUnitySocketHandler(27-30)JObject(38-48)JObject(240-259)Editor/Resources/GetGameObjectResource.cs (3)
JObject(27-74)JObject(82-108)JObject(177-224)
Editor/UnityBridge/McpUnityServer.cs (1)
Editor/UnityBridge/McpUnitySocketHandler.cs (1)
McpResourceBase(216-232)
Tests/Editor/TestRunner.cs (2)
Tests/Editor/SimpleTest.cs (1)
Test(8-13)Tests/Editor/BasicTests.cs (1)
Test(8-13)
Tests/Runtime/SimplePlayModeTest.cs (4)
Tests/Editor/AddAssetToSceneToolTests.cs (2)
Test(13-18)Test(20-25)Tests/Editor/SimpleTest.cs (1)
Test(8-13)Tests/Editor/BasicTests.cs (1)
Test(8-13)Tests/Editor/TestRunner.cs (2)
Test(10-15)Test(17-22)
Server/build/tools/selectGameObjectTool.js (6)
Server/build/tools/updateComponentTool.js (3)
toolName(4-4)toolDescription(5-5)paramsSchema(6-11)Server/build/tools/notifyMessageTool.js (4)
toolName(4-4)toolDescription(5-5)paramsSchema(6-9)params(43-43)Server/build/tools/addPackageTool.js (4)
toolName(4-4)toolDescription(5-5)paramsSchema(6-13)params(47-47)Server/build/tools/menuItemTool.js (4)
toolName(4-4)toolDescription(5-5)paramsSchema(6-8)params(42-42)Server/build/tools/runTestsTool.js (4)
toolName(4-4)toolDescription(5-5)paramsSchema(6-9)params(43-43)Server/build/index.js (1)
mcpUnity(36-36)
Editor/Tools/NotifyMessageTool.cs (2)
Editor/Tools/McpToolBase.cs (2)
McpToolBase(12-61)JObject(52-60)Editor/UnityBridge/McpUnitySocketHandler.cs (4)
McpUnitySocketHandler(20-260)McpUnitySocketHandler(27-30)JObject(38-48)JObject(240-259)
Server/build/unity/mcpUnity.js (12)
Server/build/tools/runTestsTool.js (1)
response(45-51)Server/build/tools/menuItemTool.js (1)
response(43-46)Server/build/resources/getTestsResource.js (1)
response(70-75)Server/build/resources/getConsoleLogResource.js (1)
response(38-41)Server/build/resources/getPackagesResource.js (1)
response(38-41)Server/build/tools/selectGameObjectTool.js (1)
response(70-73)Server/build/resources/getHierarchyResource.js (1)
response(38-41)Server/build/resources/getGameObjectResource.js (1)
response(50-55)Server/build/tools/notifyMessageTool.js (1)
response(45-51)Server/build/resources/getAssetsResource.js (1)
response(39-42)Server/build/resources/getMenuItemResource.js (1)
response(38-41)Server/build/utils/errors.js (1)
McpUnityError(10-26)
Server/src/unity/mcpUnity.ts (11)
Server/build/tools/runTestsTool.js (1)
response(45-51)Server/build/tools/menuItemTool.js (1)
response(43-46)Server/build/resources/getTestsResource.js (1)
response(70-75)Server/build/tools/selectGameObjectTool.js (1)
response(70-73)Server/build/resources/getConsoleLogResource.js (1)
response(38-41)Server/build/resources/getHierarchyResource.js (1)
response(38-41)Server/build/tools/notifyMessageTool.js (1)
response(45-51)Server/build/resources/getAssetsResource.js (1)
response(39-42)Server/build/resources/getMenuItemResource.js (1)
response(38-41)Server/build/resources/getPackagesResource.js (1)
response(38-41)Server/src/utils/errors.ts (1)
McpUnityError(10-28)
Editor/Tools/RunTestsTool.cs (3)
Editor/UnityBridge/McpUnitySocketHandler.cs (4)
JObject(38-48)JObject(240-259)McpUnitySocketHandler(20-260)McpUnitySocketHandler(27-30)Editor/Tools/McpToolBase.cs (2)
JObject(52-60)ExecuteAsync(38-43)Editor/Services/TestRunnerService.cs (1)
ExecuteTests(54-87)
🪛 LanguageTool
Tests/README.md
[uncategorized] ~24-~24: Loose punctuation mark.
Context: ...s package layout guidelines: - Editor: Contains tests that run in Edit mode - ...
(UNLIKELY_OPENING_PUNCTUATION)
Tests/McpTests.md
[uncategorized] ~5-~5: The preposition ‘to’ seems more likely in this position.
Context: ... your main project, create a new folder at Assets/McpTests 2. Copy the test file...
(AI_HYDRA_LEO_REPLACE_AT_TO)
README.md
[misspelling] ~3-~3: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...e purpose of this package is to provide a MCP Unity Server for executing Unity op...
(EN_A_VS_AN)
[grammar] ~11-~11: Please check the verb form.
Context: ...y 2022.3 or higher ### Installation 1. Open the Package Manager in Unity 2. Click t...
(SHE_LIVE)
🔇 Additional comments (71)
Tests/Editor/McpUnity.Tests.Editor.asmdef (1)
1-25: Assembly definition for Editor tests looks properly configured.The assembly definition correctly sets up the Editor tests with:
- Appropriate namespace (
McpUnity.Tests)- Required references to the editor assembly and test assemblies
- Platform restrictions to Editor only
- Necessary precompiled references to NUnit and Newtonsoft.Json
- Proper test constraints
This is aligned with Unity's recommended practices for organizing test code.
Editor/Services/TestRunnerService.cs (1)
58-58: Increased test timeout from 1 to 10 minutes.The default timeout for test execution has been increased from 1 to 10 minutes. This is a reasonable change as it allows for more complex test scenarios to complete without timing out, especially for PlayMode tests which often require more time to execute.
Tests/Runtime/McpUnity.Tests.Runtime.asmdef (1)
1-21: Runtime test assembly definition is ready for future implementation.The assembly definition is properly configured with:
- Appropriate namespace matching the Editor tests
- Required references for testing
- No platform restrictions (suitable for runtime tests)
- Necessary precompiled references to NUnit and Newtonsoft.Json
However, I notice that the "references" array is empty. This might be intentional since the PR objectives mention that runtime tests are not yet implemented, but make sure to add references to the runtime assemblies when you implement these tests.
When implementing runtime tests in the future, make sure to update the references array to include the necessary runtime assemblies (e.g., "McpUnity.Runtime").
README.md (6)
7-9: Requirements Section Clarity
The new “### Requirements” section clearly specifies the Unity version prerequisite, which improves project setup clarity.
10-15: Clear Installation Instructions
The “### Installation” steps are concise and easy to follow. The numbered steps effectively guide users on how to add the package via Git URL.🧰 Tools
🪛 LanguageTool
[grammar] ~11-~11: Please check the verb form.
Context: ...y 2022.3 or higher ### Installation 1. Open the Package Manager in Unity 2. Click t...(SHE_LIVE)
16-20: Accurate Dependency Addition via Manifest
The JSON snippet provided for adding the dependency tomanifest.jsonvia the Git URL is correct and clearly presented.
21-25: Local Development Instructions Are Clear
The instructions along with the JSON snippet for adding the package via the filesystem are straightforward and useful for local development.
26-34: Detailed Running Tests Guidance
The “## Running Tests” section gives clear, step-by-step directions for setting up the test environment in Unity. This will be very helpful for users to see and run tests properly.
35-36: Further Documentation Reference
The “## Further Documentation” section includes a clear reference to the project Wiki. This concise pointer to additional resources is well executed.Tests/.gitignore (1)
1-2: .gitignore for Test Assets
The.gitignorefile in the Tests directory correctly specifies that all files with the.metaextension should be ignored. This helps keep the repository clean from Unity-generated metadata files.Tests/Editor/AddAssetToSceneToolTests.cs.meta (1)
1-2: Metadata Setup for AddAssetToSceneToolTests.cs
The metadata file properly sets thefileFormatVersionto 2 and assigns a unique GUID, ensuring correct versioning and identification for the test file.Tests/Editor/BasicTests.cs.meta (1)
1-2: Metadata Setup for BasicTests.cs
The file correctly includes thefileFormatVersionand a unique GUID, which aligns with the standard used for other test metadata files.Tests/Editor/TestRunner.cs.meta (1)
1-2: Metadata Setup for TestRunner.cs
This metadata file correctly specifies both thefileFormatVersionand a unique GUID, supporting consistent file management and versioning across the test suite.Tests/Editor/SimpleTest.cs (1)
4-15: LGTM: Well-structured basic test.This is a clean implementation of a basic NUnit test that establishes the foundation for the test framework mentioned in the PR objectives.
Tests/Editor/BasicTests.cs (1)
1-15: Test implements basic verification logic correctlyThe
BasicTestmethod follows standard NUnit testing patterns with proper assertion for a simple mathematical operation. This provides a good foundation for the test framework being introduced in this PR.I see it follows a similar pattern to other test files in the project, like
TestRunnerandSimpleTest.Editor/UnityBridge/McpUnityServer.cs (1)
69-72: Public exposure of resources dictionary enhances functionalityThis change supports the new
FindResourceByUrimethod inMcpUnitySocketHandler.csby exposing the previously private resources collection. The property is correctly implemented with proper XML documentation.Editor/Tests.meta (1)
1-9: Meta file is correctly configuredThis standard Unity meta file is properly configured with the necessary format version, GUID, and folder asset settings required for Unity's asset management system.
Tests/Runtime/SimplePlayModeTest.cs (1)
1-26: PlayMode tests correctly implement both test typesThe file properly implements two types of Unity tests:
- Standard NUnit test with
[Test]attribute- Unity-specific test with
[UnityTest]attribute and coroutine implementationThis follows Unity's best practices for PlayMode testing. However, note that according to the PR description, there may be issues with PlayMode tests disconnecting MCP socket connections during domain reloads.
Have you verified that these PlayMode tests work correctly with the MCP socket connection, given the concern mentioned in the PR description about socket disconnection during domain reloads?
Tests/Editor/AddAssetToSceneToolTests.cs (2)
1-8: Appropriate imports for Unity testingThe imports include the necessary libraries for Unity testing (NUnit, UnityEngine.TestTools) and seem appropriate for this test class. Good inclusion of McpUnity.Tools and Newtonsoft.Json.JLink which may be needed for testing the AddAssetToSceneTool functionality.
9-12: Well-structured test class setupProper namespace organization with the test class placed in McpUnity.Tests.
Tests/Editor/TestRunner.cs (2)
1-5: Standard test importsThe imports include the necessary libraries for Unity testing, though UnityEngine.TestTools is imported but not used in this file.
6-9: Well-structured test namespaceProper namespace organization consistent with other test files.
Tests/McpTests.md (3)
1-4: Clear introduction to the test instructionsThe document provides a good explanation of why the workaround is needed, setting proper expectations for users.
9-35: Well-structured Assembly Definition configurationThe JSON configuration is comprehensive and includes all necessary references for the test assembly, including proper platform targets and precompiled references.
37-40: Clear instructions for accessing testsSteps 4-6 provide clear guidance on how to access and run the tests after setting up the workaround.
Editor/UnityBridge/McpUnitySocketHandler.cs (3)
80-90: Good enhancement for resource lookup by URIThis new code adds the ability to look up resources by URI when a method is not directly found, which is a useful extension of functionality that aligns with the PR objectives of enhancing the tools.
102-110: Improved error handling for response sendingWrapping the
Sendmethod in a try-catch block with detailed error logging is a good improvement for reliability and debugging. This aligns with the PR objectives of improving error reporting.
211-232: Well-implemented URI lookup methodThe
FindResourceByUrimethod is cleanly implemented with good documentation and logging. It properly searches through server resources for URI matches..gitignore (1)
12-16: Added exclusions for development-specific files.The additions to ignore development logs and scripts keep the repository clean by preventing developer-specific files from being committed. This is particularly useful for logging scripts that may contain environment-specific paths.
Editor/Tools/SelectGameObjectTool.cs (5)
40-42: Improved variable organization for GameObject lookup.Adding these variables helps track both the found object and the method used to find it, which will be used for better error reporting and logging.
46-55: Refactored GameObject lookup for better error reporting.The code now stores the found GameObject reference and tracking information in variables instead of directly assigning to Selection, which allows for proper null checking before proceeding.
57-64: Added proper null checking for GameObject lookup.This change enhances the error handling by explicitly checking if the GameObject was found and returning a detailed error response when it isn't, including the specific identifier that was used in the search.
67-71: Refactored selection and logging to use the found object.Using the cached
foundObjectreference improves code consistency and the enhanced logging now includes how the GameObject was found, which is helpful for debugging.
77-79: Improved response message with GameObject name.The response now includes the name of the selected GameObject, which provides better feedback to the client about what was actually selected.
package.json (5)
4-4: Updated display name to MCP Unity Server.The new name better represents the package's purpose as a server component.
9-10: Added test framework dependency.The addition of the Unity Test Framework dependency is essential for supporting the new test infrastructure introduced in this PR.
12-15: Added keywords for improved discoverability.Keywords help with package discoverability in the Unity Package Manager and other registries.
16-18: Updated author field to object format.Changed the author format to use an object structure, which is the preferred format in modern package.json files.
19-21: Added testables configuration for test discovery.This configuration ensures that Unity's test runner can discover and execute tests within the package, supporting the test framework mentioned in the PR objectives.
Editor/Tools/NotifyMessageTool.cs (8)
1-5: No issues with the added namespaces.
UsingSystemandUnityEditorin an Editor script context is valid, and there are no conflicts or concerns.
11-11: Documentation update aligns with functionality.
Renaming the summary to emphasize "displaying messages in the Unity console" accurately describes the tool's purpose.
15-24: Enum-based approach for message types is a solid improvement.
This addition provides type safety and clear delineation of message categories.
40-41: Minor comment block addition.
This line simply adds clarity without altering functionality.
45-48: Well-formed validation for missing messages.
This block properly catches and reports an empty or undefined message parameter.
50-54: Robust validation of the message type.
UsingEnum.TryParseensures unrecognized message types produce a coherent error.
60-72: Appropriate handling of different message severities.
Switching on the newMessageTypeenum clarifies the log output.
77-78: Success response is concise and informative.
Returning the displayed message under"message"helps upstream code confirm the tool’s outcome.Server/build/tools/selectGameObjectTool.js (1)
33-59: Consolidated return object and handler improve clarity.
The new structure returning{ name, description, paramsSchema, handler }is consistent and provides robust validation and error handling forobjectPathorinstanceId.Server/src/tools/selectGameObjectTool.ts (3)
7-7: Importing ToolDefinition for strong typing is appropriate.
Ensures the returned object follows the expected structure for tool registration.
25-25: Updated signature to return ToolDefinition.
This makes the function’s contract explicit and aligns with a typed approach.
46-81: Handler refactor with custom validation and response is clear.
The approach parallels the JavaScript version, ensuring consistency and robust error reporting.Editor/Tools/UpdateComponentTool.cs (10)
9-9: Added LINQ usage.
Bringing inSystem.Linqis valid, especially for the reflection-based type search below.
14-14: Updated class docstring.
The new summary explicitly states that this tool updates component fields, clarifying its purpose.
30-44: Improved logic for locating the target GameObject.
UsingEditorUtility.InstanceIDToObjectfirst and thenGameObject.Findis a sensible fallback strategy.
45-52: Clear error for missing GameObject.
Returning an error response with the attempted path/ID helps diagnose issues quickly.
54-63: Validation of componentName is correct.
Capturing an explicit error for a missing component name prevents unintended behavior downstream.
69-75: Specific error for unfound component type.
Returning"component_not_found"gives clear feedback for debugging.
77-79: Conditional component creation is appropriate.
UsingGetComponentor adding a new one helps ensure the component is ready for updates.
80-82: Strict check for null componentData.
Properly enforced requirement prevents ambiguous partial updates.
90-155: Detailed field-by-field reflection update logic.
Provides granular error reporting for invalid or read-only fields, which is handy for debugging.
157-175: Well-structured JSON response summarizing invalid fields.
This clarifies which parts succeeded versus which failed, improving user feedback.Editor/Tools/RunTestsTool.cs (8)
2-2: Imports appear correct.
These newly introduced imports align with the added references to collections, services, and Unity Editor APIs.Also applies to: 5-7
19-24: New private fields for test tracking.
Introducing_testCompletionSource,_testResults, and counters improves clarity and state management for test runs.
28-28: Updated constructor details.
Providing a clear, user-friendly description and initializing theList<JObject>for test results clarifies the tool’s behavior.Also applies to: 33-33
36-38: Preventing synchronous execution.
Throwing aNotSupportedExceptionfor non-async execution helps ensure the tool is always used in the intended asynchronous manner.
79-84: Logging test run start.
Capturing the test case count provides useful context for subsequent reporting.
87-120: Handling run completion and assembling summary.
The structured JSON response is thorough, including test counts and a success flag. Exception handling also looks solid.
122-126: TestStarted logging.
Displaying the test name as it starts is helpful for real-time monitoring.
128-161: Tracking per-test outcomes.
Incrementing pass/fail counters and storing details in_testResultsis a clear approach. Handling errors or missing data is appropriately managed.Tests/Runtime/SimplePlayModeTest.cs.meta (1)
1-2: Valid Metadata File for Runtime Play Mode TestsThe metadata file correctly specifies the
fileFormatVersionand a uniqueguid. The content is concise and adheres to expected Unity meta file conventions.Tests/Editor/SimpleTest.cs.meta (1)
1-2: Valid Metadata File for Editor TestsThis metadata file is correct with a proper declaration of
fileFormatVersionand a uniqueguid. It clearly supports the associated test scripts.
| public class AddAssetToSceneToolTests | ||
| { | ||
| [Test] | ||
| public void SimpleTest() | ||
| { | ||
| Debug.Log("[MCP Unity Test] Running simple verification test"); | ||
| Assert.Pass("Simple test passed"); | ||
| } | ||
|
|
||
| [Test] | ||
| public void AnotherSimpleTest() | ||
| { | ||
| Debug.Log("[MCP Unity Test] Running another verification test"); | ||
| Assert.That(true, Is.True, "Truth value should be true"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Need for comprehensive test cases
The current tests are basic verification tests that don't validate the enhanced functionality mentioned in the PR objectives. Consider adding specific tests for the improvements made to the AddAssetToSceneTool, especially around stricter input validation and error reporting.
There was a problem hiding this comment.
Good point. Maybe a separate PR to follow up with.
There was a problem hiding this comment.
@dsarno That's a good approach. Breaking complex work into separate PRs helps keep changes manageable. When you do create those follow-up tests, focusing on validating the error handling for invalid inputs (missing parent objects) and the enhanced error reporting would be particularly valuable. Let me know if you need any assistance with that future PR.
🧬 Code Definitions Used
Relevant Code Snippets
From Tests/Runtime/SimplePlayModeTest.cs
[Test]
public void SimplePlayModeTestPasses()
{
Debug.Log("Running Simple PlayMode Test");
Assert.Pass("This play mode test should pass");
}- Description: A simple test that logs a message and asserts that the test passes.
- Line Range: 11-16
From Tests/Editor/BasicTests.cs
[Test]
public void BasicTest()
{
Debug.Log("[MCP Unity Test] Running basic test");
Assert.That(1 + 1, Is.EqualTo(2), "Basic math should work");
}- Description: A basic test that checks if the sum of 1 + 1 equals 2 and logs a message.
- Line Range: 8-13
From Tests/Editor/TestRunner.cs
[Test]
public void SimplePassTest()
{
Debug.Log("Running a very simple pass test");
Assert.Pass("This test should always pass");
}- Description: A simple test that logs a message and asserts that the test always passes.
- Line Range: 10-15
[Test]
public void SimpleAssertionTest()
{
Debug.Log("Running simple assertion test");
Assert.That(1 + 1, Is.EqualTo(2), "Basic math should work");
}- Description: A simple assertion test that checks if 1 + 1 equals 2 and logs a message.
- Line Range: 17-22
From Tests/Editor/SimpleTest.cs
[Test]
public void SimplePassingTest()
{
Debug.Log("Running SimplePassingTest");
Assert.Pass("This test should pass");
}- Description: A simple test that logs a message and asserts that the test passes.
- Line Range: 8-13
public class SimpleTest
{
[Test]
public void SimplePassingTest()
{
Debug.Log("Running SimplePassingTest");
Assert.Pass("This test should pass");
}
}- Description: A class containing a simple test that logs a message and asserts that the test passes.
- Line Range: 6-14
Summary
The relevant snippets include various simple tests that log messages and assert conditions, which may provide context or examples for the tests in AddAssetToSceneToolTests.cs.
Code Analysis Tools
Tool Analysis Results
CodeRabbit ran static analysis tools on your files and found the following:
🪛 LanguageTool
README.md
[misspelling] ~3-~3: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...e purpose of this package is to provide a MCP Unity Server for executing Unity op...
(EN_A_VS_AN)
[style] ~8-~8: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 1670 characters long)
Context: ...# Requirements - Unity 2022.3 or higher
(EN_EXCESSIVE_EXCLAMATION)
[grammar] ~49-~49: Please check the verb form.
Context: ... ╚═╝ ``` ### Installation 1. Open the Package Manager in Unity 2. Click t...
(SHE_LIVE)
[uncategorized] ~68-~68: The preposition ‘to’ seems more likely in this position.
Context: ...ct to run them: 1. Create a new folder at Assets/McpTests 2. Copy the test file...
(AI_HYDRA_LEO_REPLACE_AT_TO)
Tests/McpTests.md
[uncategorized] ~5-~5: The preposition ‘to’ seems more likely in this position.
Context: ... your main project, create a new folder at Assets/McpTests 2. Copy the test file...
(AI_HYDRA_LEO_REPLACE_AT_TO)
Tests/README.md
[uncategorized] ~24-~24: Loose punctuation mark.
Context: ...s package layout guidelines: - Editor: Contains tests that run in Edit mode - ...
(UNLIKELY_OPENING_PUNCTUATION)
Files with Issues
README.md
🪛 LanguageTool
[misspelling] ~3-~3: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...e purpose of this package is to provide a MCP Unity Server for executing Unity op...
(EN_A_VS_AN)
[style] ~8-~8: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 1670 characters long)
Context: ...# Requirements - Unity 2022.3 or higher
(EN_EXCESSIVE_EXCLAMATION)
[grammar] ~49-~49: Please check the verb form.
Context: ... ╚═╝ ``` ### Installation 1. Open the Package Manager in Unity 2. Click t...
(SHE_LIVE)
[uncategorized] ~68-~68: The preposition ‘to’ seems more likely in this position.
Context: ...ct to run them: 1. Create a new folder at Assets/McpTests 2. Copy the test file...
(AI_HYDRA_LEO_REPLACE_AT_TO)
Tests/McpTests.md
🪛 LanguageTool
[uncategorized] ~5-~5: The preposition ‘to’ seems more likely in this position.
Context: ... your main project, create a new folder at Assets/McpTests 2. Copy the test file...
(AI_HYDRA_LEO_REPLACE_AT_TO)
Tests/README.md
🪛 LanguageTool
[uncategorized] ~24-~24: Loose punctuation mark.
Context: ...s package layout guidelines: - Editor: Contains tests that run in Edit mode - ...
(UNLIKELY_OPENING_PUNCTUATION)
Next Steps
Based on these results, you can:
- Fix the issues identified by the tools
- Ask me to suggest specific fixes for these issues
- Proceed with planning or implementing changes while accounting for these issues
There was a problem hiding this comment.
I'm ok to have a future PR with the specific test cases.
I would ask then to remove this unecessary tests and just keep the code for future guidance
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| [Test] | ||
| public void SimpleTest() | ||
| { | ||
| Debug.Log("[MCP Unity Test] Running simple verification test"); | ||
| Assert.Pass("Simple test passed"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider replacing this placeholder test with actual functionality tests
While this test confirms the testing framework works, it doesn't validate any actual functionality of the AddAssetToSceneTool. Consider implementing tests that exercise the tool's core capabilities mentioned in the PR objectives.
[Test]
public void SimpleTest()
{
Debug.Log("[MCP Unity Test] Running simple verification test");
- Assert.Pass("Simple test passed");
+ // Test actual functionality, for example:
+ // Setup a test scene
+ // var tool = new AddAssetToSceneTool();
+ // var parameters = new JObject { ["assetPath"] = "path/to/test/asset" };
+ // var result = tool.Execute(parameters);
+ // Assert.That(result["success"].Value<bool>(), Is.True, "Asset should be added successfully");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [Test] | |
| public void SimpleTest() | |
| { | |
| Debug.Log("[MCP Unity Test] Running simple verification test"); | |
| Assert.Pass("Simple test passed"); | |
| } | |
| [Test] | |
| public void SimpleTest() | |
| { | |
| Debug.Log("[MCP Unity Test] Running simple verification test"); | |
| // Test actual functionality, for example: | |
| // Setup a test scene | |
| // var tool = new AddAssetToSceneTool(); | |
| // var parameters = new JObject { ["assetPath"] = "path/to/test/asset" }; | |
| // var result = tool.Execute(parameters); | |
| // Assert.That(result["success"].Value<bool>(), Is.True, "Asset should be added successfully"); | |
| } |
There was a problem hiding this comment.
See above. Follow up PR if this is accepted.
There was a problem hiding this comment.
@dsarno I understand. Your approach to address the test implementation in a follow-up PR makes sense. This helps keep the current PR focused while still acknowledging the need for more thorough testing. Let me know if you'd like assistance when you're ready to implement those functional tests.
🧬 Code Definitions Used
Relevant Code Snippets
From Tests/Runtime/SimplePlayModeTest.cs
[Test]
public void SimplePlayModeTestPasses()
{
Debug.Log("Running Simple PlayMode Test");
Assert.Pass("This play mode test should pass");
}- Line Range: 11-16
- Description: This is a simple test that logs a message and asserts that the test passes.
From Tests/Editor/BasicTests.cs
[Test]
public void BasicTest()
{
Debug.Log("[MCP Unity Test] Running basic test");
Assert.That(1 + 1, Is.EqualTo(2), "Basic math should work");
}- Line Range: 8-13
- Description: This test verifies a basic mathematical operation, ensuring that 1 + 1 equals 2.
From Tests/Editor/TestRunner.cs
[Test]
public void SimplePassTest()
{
Debug.Log("Running a very simple pass test");
Assert.Pass("This test should always pass");
}- Line Range: 10-15
- Description: This is another simple test that logs a message and asserts that the test passes.
[Test]
public void SimpleAssertionTest()
{
Debug.Log("Running simple assertion test");
Assert.That(1 + 1, Is.EqualTo(2), "Basic math should work");
}- Line Range: 17-22
- Description: Similar to the previous basic test, this one checks that 1 + 1 equals 2.
From Tests/Editor/SimpleTest.cs
[Test]
public void SimplePassingTest()
{
Debug.Log("Running SimplePassingTest");
Assert.Pass("This test should pass");
}- Line Range: 8-13
- Description: This test logs a message and asserts that it passes.
public class SimpleTest
{
[Test]
public void SimplePassingTest()
{
Debug.Log("Running SimplePassingTest");
Assert.Pass("This test should pass");
}
}- Line Range: 6-14
- Description: This is a class containing a simple test that logs a message and asserts that it passes.
Summary
The relevant snippets include various simple tests that log messages and assert conditions, which may provide context or examples for the tests in AddAssetToSceneToolTests.cs.
Code Analysis Tools
Tool Analysis Results
CodeRabbit ran static analysis tools on your files and found the following:
🪛 LanguageTool
README.md
[misspelling] ~3-~3: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...e purpose of this package is to provide a MCP Unity Server for executing Unity op...
(EN_A_VS_AN)
[style] ~8-~8: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 1670 characters long)
Context: ...# Requirements - Unity 2022.3 or higher
(EN_EXCESSIVE_EXCLAMATION)
[grammar] ~49-~49: Please check the verb form.
Context: ... ╚═╝ ``` ### Installation 1. Open the Package Manager in Unity 2. Click t...
(SHE_LIVE)
[uncategorized] ~68-~68: The preposition ‘to’ seems more likely in this position.
Context: ...ct to run them: 1. Create a new folder at Assets/McpTests 2. Copy the test file...
(AI_HYDRA_LEO_REPLACE_AT_TO)
Tests/README.md
[uncategorized] ~24-~24: Loose punctuation mark.
Context: ...s package layout guidelines: - Editor: Contains tests that run in Edit mode - ...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
README.md
75-75: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
75-75: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
Files with Issues
README.md
🪛 LanguageTool
[misspelling] ~3-~3: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...e purpose of this package is to provide a MCP Unity Server for executing Unity op...
(EN_A_VS_AN)
[style] ~8-~8: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 1670 characters long)
Context: ...# Requirements - Unity 2022.3 or higher
(EN_EXCESSIVE_EXCLAMATION)
[grammar] ~49-~49: Please check the verb form.
Context: ... ╚═╝ ``` ### Installation 1. Open the Package Manager in Unity 2. Click t...
(SHE_LIVE)
[uncategorized] ~68-~68: The preposition ‘to’ seems more likely in this position.
Context: ...ct to run them: 1. Create a new folder at Assets/McpTests 2. Copy the test file...
(AI_HYDRA_LEO_REPLACE_AT_TO)
🪛 markdownlint-cli2 (0.17.2)
75-75: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
75-75: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
Tests/README.md
🪛 LanguageTool
[uncategorized] ~24-~24: Loose punctuation mark.
Context: ...s package layout guidelines: - Editor: Contains tests that run in Edit mode - ...
(UNLIKELY_OPENING_PUNCTUATION)
Next Steps
Based on these results, you can:
- Fix the issues identified by the tools
- Ask me to suggest specific fixes for these issues
- Proceed with planning or implementing changes while accounting for these issues
| public class TestRunner | ||
| { | ||
| [Test] | ||
| public void SimplePassTest() | ||
| { | ||
| Debug.Log("Running a very simple pass test"); | ||
| Assert.Pass("This test should always pass"); | ||
| } | ||
|
|
||
| [Test] | ||
| public void SimpleAssertionTest() | ||
| { | ||
| Debug.Log("Running simple assertion test"); | ||
| Assert.That(1 + 1, Is.EqualTo(2), "Basic math should work"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
TestRunner class needs to test actual tools
The TestRunner class currently contains only framework verification tests. Given the PR's focus on enhancing tools like NotifyMessageTool, RunTestsTool, SelectGameObjectTool, and UpdateComponentTool, this class should include tests for these specific tools to validate the mentioned improvements.
| [Test] | ||
| public void SimpleAssertionTest() | ||
| { | ||
| Debug.Log("Running simple assertion test"); | ||
| Assert.That(1 + 1, Is.EqualTo(2), "Basic math should work"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Trivial assertion test
This test is nearly identical to tests in other files (like BasicTests.cs) and doesn't validate any of the enhanced functionality mentioned in the PR objectives. Consider adding more meaningful tests.
[Test]
public void SimpleAssertionTest()
{
Debug.Log("Running simple assertion test");
- Assert.That(1 + 1, Is.EqualTo(2), "Basic math should work");
+ // Test actual functionality of one of the enhanced tools
+ // For example, testing RunTestsTool with its improved reporting:
+ // var tool = new RunTestsTool();
+ // var parameters = new JObject { ["mode"] = "EditMode" };
+ // var result = tool.Execute(parameters);
+ // Assert.That(result["testResults"], Is.Not.Null, "Should return test results");
}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
README.md (1)
76-85: 💡 Verification agent❓ Verification inconclusive
Unresolved merge conflict detected.
Between lines 76 and 85, there are conflict markers (e.g.,=======) along with alternative blocks containing licensing and acknowledgements information. Please resolve these conflicts by choosing the intended content and removing the conflict markers so that the final document is coherent.
Resolve Unresolved Merge Conflict in README.md
Between lines 76 and 85 of the README.md file, conflict markers (e.g.,
=======) are present along with differing content blocks for licensing and acknowledgements. Please choose the correct content, remove all merge conflict markers, and ensure that the final document is coherent.
- File affected: README.md (lines 76–85)
- Required action: Remove conflict markers (e.g.,
=======) and merge the intended licensing and acknowledgements content appropriately.
🧹 Nitpick comments (2)
README.md (2)
1-3: Fix grammatical errors and article usage.
On line 3, replace
The purpose of this package is to provide a MCP Unity Server for executing Unity operations and request Editor information from AI MCP enabled hosts.
with
The purpose of this package is to provide an MCP Unity Server for executing Unity operations and requesting Editor information from AI MCP-enabled hosts.
This correction fixes the article choice (“an MCP”) and ensures parallel structure as well as consistent hyphenation in “MCP-enabled hosts.”🧰 Tools
🪛 LanguageTool
[misspelling] ~3-~3: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...e purpose of this package is to provide a MCP Unity Server for executing Unity op...(EN_A_VS_AN)
65-73: Refine directory creation instruction.
On line 69, consider replacing “Create a new folder atAssets/McpTests” with “Create a new folder inAssets/McpTests” for improved clarity about the location where the folder should be created.🧰 Tools
🪛 LanguageTool
[uncategorized] ~69-~69: The preposition ‘to’ seems more likely in this position.
Context: ...ct to run them: 1. Create a new folder atAssets/McpTests2. Copy the test file...(AI_HYDRA_LEO_REPLACE_AT_TO)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[misspelling] ~3-~3: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...e purpose of this package is to provide a MCP Unity Server for executing Unity op...
(EN_A_VS_AN)
[style] ~9-~9: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 1676 characters long)
Context: ...ements - Unity 2022.3 or higher =======
(EN_EXCESSIVE_EXCLAMATION)
[grammar] ~50-~50: Please check the verb form.
Context: ... ╚═╝ ``` ### Installation 1. Open the Package Manager in Unity 2. Click t...
(SHE_LIVE)
[uncategorized] ~69-~69: The preposition ‘to’ seems more likely in this position.
Context: ...ct to run them: 1. Create a new folder at Assets/McpTests 2. Copy the test file...
(AI_HYDRA_LEO_REPLACE_AT_TO)
🪛 markdownlint-cli2 (0.17.2)
README.md
75-75: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
75-75: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
🔇 Additional comments (2)
README.md (2)
49-64: Installation section looks good.
The instructions for adding the package via the Unity Package Manager and the alternative manifest.json method are clear and concise.🧰 Tools
🪛 LanguageTool
[grammar] ~50-~50: Please check the verb form.
Context: ... ╚═╝ ``` ### Installation 1. Open the Package Manager in Unity 2. Click t...(SHE_LIVE)
74-75: Further Documentation section is clear.
The section directing users to the project’s Wiki for full documentation is well defined and appropriate.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
75-75: Heading style
Expected: atx; Actual: setext(MD003, heading-style)
75-75: Trailing punctuation in heading
Punctuation: '.'(MD026, no-trailing-punctuation)
Improves validation, error handling, and robustness of several MCP Unity tools. Simultaneously introduces a new testing framework structure under (migrated from ) to facilitate automated testing. **Key Tool Enhancements:** * **NotifyMessageTool**: Validates message content and type (info, warning, error enum). * **RunTestsTool**: Refactored execution, added mode validation, provides detailed pass/fail counts and individual results. Handles 'no tests found' case. * **SelectGameObjectTool**: Requires instanceId/objectPath, improves search logic (ID > path > name), specific 'not found' errors. * **UpdateComponentTool**: Requires componentName/Data, improves search (ID > path), finds type across assemblies, adds component if missing, uses Reflection for safer updates, reports invalid fields. * **McpUnitySocketHandler**: Added capability to look up Resources by URI. * **General**: More specific error codes/messages for various failures. **Test Framework Setup:** * Adds new testing structure under the top-level directory. * Includes Assembly Definitions () for EditMode and PlayMode tests. * Adds initial test files () and necessary files. * Moves/reorganizes existing test files from to the new structure. * Updates , , and accordingly.
dsarno
force-pushed
the
fix/tool-validation
branch
from
75ad1c2 to
c95a660
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
Server/src/tools/selectGameObjectTool.ts (1)
92-111: 🛠️ Refactor suggestionRemove redundant toolHandler function
The toolHandler function is now redundant as its functionality has been completely incorporated into the new handler function defined in the returned ToolDefinition. The new implementation also adds more fields to the response.
-/** - * Handles selecting a GameObject in Unity - * - * @param mcpUnity The McpUnity instance to communicate with Unity - * @param params The parameters for the tool - * @returns A promise that resolves to the tool execution result - * @throws McpUnityError if the request to Unity fails - */ -async function toolHandler(mcpUnity: McpUnity, params: any): Promise<CallToolResult> { - const response = await mcpUnity.sendRequest({ - method: toolName, - params - }); - - if (!response.success) { - throw new McpUnityError( - ErrorType.TOOL_EXECUTION, - response.message || `Failed to select GameObject` - ); - } - - return { - content: [{ - type: response.type, - text: response.message || `Successfully selected GameObject` - }] - }; -}Then update the server.tool registration to use the handler directly:
server.tool( toolName, toolDescription, paramsSchema.shape, async (params: any) => { try { logger.info(`Executing tool: ${toolName}`, params); - const result = await toolHandler(mcpUnity, params); + const result = await handler(params); logger.info(`Tool execution successful: ${toolName}`); return result; } catch (error) { logger.error(`Tool execution failed: ${toolName}`, error); throw error; } } );Server/build/tools/selectGameObjectTool.js (1)
69-83: 🛠️ Refactor suggestionRemove redundant toolHandler function
Similar to the TypeScript file, the toolHandler function is now redundant as its functionality has been incorporated into the handler function defined in the returned object. The new implementation also provides a more comprehensive response structure.
-/** - * Handles selecting a GameObject in Unity - * - * @param mcpUnity The McpUnity instance to communicate with Unity - * @param params The parameters for the tool - * @returns A promise that resolves to the tool execution result - * @throws McpUnityError if the request to Unity fails - */ -async function toolHandler(mcpUnity, params) { - const response = await mcpUnity.sendRequest({ - method: toolName, - params - }); - if (!response.success) { - throw new McpUnityError(ErrorType.TOOL_EXECUTION, response.message || `Failed to select GameObject`); - } - return { - content: [{ - type: response.type, - text: response.message || `Successfully selected GameObject` - }] - }; -}Then update the server.tool registration to use the handler directly.
♻️ Duplicate comments (1)
README.md (1)
74-76:⚠️ Potential issueMerge Conflict Artifact Detected
A merge conflict marker ("=======") is present at line 76. Please remove this marker and ensure that only the intended content remains.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
75-75: Heading style
Expected: atx; Actual: setext(MD003, heading-style)
75-75: Trailing punctuation in heading
Punctuation: '.'(MD026, no-trailing-punctuation)
🧹 Nitpick comments (9)
Tests/README.md (2)
9-17: Step-by-Step Local Development Workaround
The workaround steps for running tests (creating a folder, copying files, and opening the Test Runner) are clear and sequential. For extra clarity, consider highlighting key folder names (e.g., Assets/McpTests) in bold.
20-27: Well-Organized Test Structure Section
The test structure is described clearly with separate bullet points for theEditorandRuntimedirectories. Double-check the punctuation around these entries based on the static analysis hints.🧰 Tools
🪛 LanguageTool
[uncategorized] ~24-~24: Loose punctuation mark.
Context: ...s package layout guidelines: -Editor: Contains tests that run in Edit mode - ...(UNLIKELY_OPENING_PUNCTUATION)
Tests/McpTests.md (1)
5-7: Step-by-Step Instructions with Minor Preposition Suggestion
The instructions are clear overall. However, the static analysis hint suggests that in the phrase "create a new folder atAssets/McpTests" it may be clearer to use "to" instead of "at". Consider updating this for improved readability.🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: The preposition ‘to’ seems more likely in this position.
Context: ... your main project, create a new folder atAssets/McpTests2. Copy the test file...(AI_HYDRA_LEO_REPLACE_AT_TO)
README.md (3)
1-3: Title and Introduction Update
The updated title "MCP Unity Server" and introduction succinctly describe the purpose of the package. Consider revising the article in line 3 to "an MCP Unity Server" (if the acronym is pronounced with a vowel sound) for grammatical consistency.🧰 Tools
🪛 LanguageTool
[misspelling] ~3-~3: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...e purpose of this package is to provide a MCP Unity Server for executing Unity op...(EN_A_VS_AN)
17-46: ASCII Art Branding
The ASCII art is visually striking and contributes to the branding of the project. It would be good to test its display in various markdown renderers to ensure consistency.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
17-17: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
64-72: Concise Instructions for Running Tests
The section detailing how to run tests for local packages is clear. As in the other test documentation, consider revising preposition usage (e.g., "create a new folder at" may be clearer as "create a new folder in" or "to") to improve readability.🧰 Tools
🪛 LanguageTool
[uncategorized] ~68-~68: The preposition ‘to’ seems more likely in this position.
Context: ...ct to run them: 1. Create a new folder atAssets/McpTests2. Copy the test file...(AI_HYDRA_LEO_REPLACE_AT_TO)
Editor/Tools/UpdateComponentTool.cs (3)
45-50: Refine the error message if only one parameter is provided.
Currently, the message references both path and instance ID even if the user only supplied one. While minor, consider customizing the error text for clarity.Here's a potential snippet:
- $"GameObject with path '{parameters["objectPath"]}' or instance ID {parameters["instanceId"]} not found" + string detailedMessage; + if (parameters["objectPath"] != null && parameters["instanceId"] != null) + { + detailedMessage = $"GameObject not found with path '{parameters["objectPath"]}' or instance ID {parameters["instanceId"]}"; + } + else if (parameters["objectPath"] != null) + { + detailedMessage = $"GameObject not found with path '{parameters["objectPath"]}'"; + } + else + { + detailedMessage = $"GameObject not found with instance ID {parameters["instanceId"]}"; + } + return McpUnitySocketHandler.CreateErrorResponse(detailedMessage, "object_not_found");
64-74: Consider caching the result of dynamic assembly scanning.
UsingSelectMany(a => a.GetTypes())each time could become expensive in larger projects. Caching the discovered component types by name would improve performance without changing behavior.
263-310: Inconsistent case handling for fields vs. properties.
While properties are found withBindingFlags.IgnoreCase, fields aren't. If you want uniform case-insensitive matching, consider adding it to the field resolution as well. Otherwise, user-supplied JSON might have to match the exact field name.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
.gitignore(1 hunks)Editor/Services/TestRunnerService.cs(1 hunks)Editor/Tests.meta(1 hunks)Editor/Tools/AddAssetToSceneTool.cs(1 hunks)Editor/Tools/RunTestsTool.cs(2 hunks)Editor/Tools/SelectGameObjectTool.cs(1 hunks)Editor/Tools/UpdateComponentTool.cs(4 hunks)Editor/UnityBridge/McpUnitySocketHandler.cs(3 hunks)README.md(3 hunks)Server/build/tools/selectGameObjectTool.js(1 hunks)Server/build/unity/mcpUnity.js(1 hunks)Server/src/tools/selectGameObjectTool.ts(3 hunks)Server/src/unity/mcpUnity.ts(1 hunks)Tests.meta(1 hunks)Tests/.gitignore(1 hunks)Tests/Editor/AddAssetToSceneToolTests.cs(1 hunks)Tests/Editor/AddAssetToSceneToolTests.cs.meta(1 hunks)Tests/Editor/BasicTests.cs(1 hunks)Tests/Editor/BasicTests.cs.meta(1 hunks)Tests/Editor/McpUnity.Tests.Editor.asmdef(1 hunks)Tests/Editor/SimpleTest.cs(1 hunks)Tests/Editor/SimpleTest.cs.meta(1 hunks)Tests/Editor/TestRunner.cs(1 hunks)Tests/Editor/TestRunner.cs.meta(1 hunks)Tests/McpTests.md(1 hunks)Tests/README.md(1 hunks)Tests/Runtime/McpUnity.Tests.Runtime.asmdef(1 hunks)Tests/Runtime/SimplePlayModeTest.cs(1 hunks)Tests/Runtime/SimplePlayModeTest.cs.meta(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (21)
- Tests/.gitignore
- Tests/Editor/BasicTests.cs.meta
- Tests/Editor/AddAssetToSceneToolTests.cs.meta
- Tests/Editor/SimpleTest.cs.meta
- Tests/Editor/SimpleTest.cs
- Tests/Editor/TestRunner.cs.meta
- Tests/Runtime/SimplePlayModeTest.cs.meta
- Tests/Editor/BasicTests.cs
- Editor/Services/TestRunnerService.cs
- Tests.meta
- Tests/Editor/TestRunner.cs
- Tests/Editor/AddAssetToSceneToolTests.cs
- Tests/Runtime/McpUnity.Tests.Runtime.asmdef
- Tests/Editor/McpUnity.Tests.Editor.asmdef
- Tests/Runtime/SimplePlayModeTest.cs
- Editor/Tests.meta
- Editor/Tools/SelectGameObjectTool.cs
- .gitignore
- Editor/UnityBridge/McpUnitySocketHandler.cs
- Server/build/unity/mcpUnity.js
- Server/src/unity/mcpUnity.ts
🧰 Additional context used
🧬 Code Definitions (4)
Server/build/tools/selectGameObjectTool.js (5)
Server/build/tools/updateComponentTool.js (4)
toolName(4-4)toolDescription(5-5)paramsSchema(6-11)response(54-62)Server/build/tools/addPackageTool.js (5)
toolName(4-4)toolDescription(5-5)paramsSchema(6-13)params(47-47)response(59-62)Server/build/tools/menuItemTool.js (5)
toolName(4-4)toolDescription(5-5)paramsSchema(6-8)params(42-42)response(43-46)Server/build/tools/notifyMessageTool.js (5)
toolName(4-4)toolDescription(5-5)paramsSchema(6-9)params(43-43)response(45-51)Server/build/tools/runTestsTool.js (5)
toolName(4-4)toolDescription(5-5)paramsSchema(6-9)params(43-43)response(45-51)
Editor/Tools/AddAssetToSceneTool.cs (1)
Editor/UnityBridge/McpUnitySocketHandler.cs (2)
McpUnitySocketHandler(20-260)McpUnitySocketHandler(27-30)
Editor/Tools/UpdateComponentTool.cs (4)
Editor/UnityBridge/McpUnitySocketHandler.cs (4)
McpUnitySocketHandler(20-260)McpUnitySocketHandler(27-30)JObject(38-48)JObject(240-259)Editor/Tools/SelectGameObjectTool.cs (1)
JObject(25-81)Editor/Tools/McpToolBase.cs (1)
JObject(52-60)Server/build/tools/updateComponentTool.js (1)
response(54-62)
Editor/Tools/RunTestsTool.cs (3)
Editor/UnityBridge/McpUnitySocketHandler.cs (4)
JObject(38-48)JObject(240-259)McpUnitySocketHandler(20-260)McpUnitySocketHandler(27-30)Editor/Tools/McpToolBase.cs (2)
JObject(52-60)ExecuteAsync(38-43)Editor/Services/TestRunnerService.cs (1)
ExecuteTests(54-87)
🪛 LanguageTool
README.md
[misspelling] ~3-~3: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...e purpose of this package is to provide a MCP Unity Server for executing Unity op...
(EN_A_VS_AN)
[style] ~8-~8: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 1670 characters long)
Context: ...# Requirements - Unity 2022.3 or higher
(EN_EXCESSIVE_EXCLAMATION)
[grammar] ~49-~49: Please check the verb form.
Context: ... ╚═╝ ``` ### Installation 1. Open the Package Manager in Unity 2. Click t...
(SHE_LIVE)
[uncategorized] ~68-~68: The preposition ‘to’ seems more likely in this position.
Context: ...ct to run them: 1. Create a new folder at Assets/McpTests 2. Copy the test file...
(AI_HYDRA_LEO_REPLACE_AT_TO)
Tests/McpTests.md
[uncategorized] ~5-~5: The preposition ‘to’ seems more likely in this position.
Context: ... your main project, create a new folder at Assets/McpTests 2. Copy the test file...
(AI_HYDRA_LEO_REPLACE_AT_TO)
Tests/README.md
[uncategorized] ~24-~24: Loose punctuation mark.
Context: ...s package layout guidelines: - Editor: Contains tests that run in Edit mode - ...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
README.md
75-75: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
75-75: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
🔇 Additional comments (43)
package.json (6)
4-4: Display Name Field Addition
The addition of"displayName": "MCP Unity Server"enhances the package metadata by providing a user-friendly name in the Unity Package Manager.
6-6: Explicit Unity Version Specification
Specifying"unity": "2022.3"ensures that consumers of this package are aware of the required Unity version for compatibility.
9-10: Dependency Update – Adding Unity Test Framework
The dependencies now include"com.unity.test-framework": "1.1.33", which aligns with the PR’s objective of enhancing the test framework support. Additionally, keeping"com.unity.editorcoroutines": "1.0.0"unchanged maintains backward compatibility while providing the required enhancements.
12-15: Introduction of Keywords Field
Adding the"keywords": ["mcp", "unity"]array improves discoverability and categorization of the package in package registries and tooling.
16-17: Structured Author Field Improvement
Converting the"author"field from a string to an object with a"name"property aligns with common package metadata practices and allows for potential future expansion (e.g., adding an email or URL).
19-21: Addition of Testables Field
Introducing"testables": ["com.gamelovers.mcp-unity"]clearly marks the package as testable. This assists automated tooling and manual testing processes by identifying which packages should be included in test runs.Server/src/tools/selectGameObjectTool.ts (3)
7-7: Good addition of ToolDefinition importAdding the ToolDefinition import aligns with the PR objective of enhancing tools with stricter validation and better error reporting. This type definition will help ensure type safety for the tool implementation.
25-25: Updated function signature with proper return typeChanging the return type from void to ToolDefinition improves type safety and makes the function's contract clearer. This is aligned with the PR objectives of enhancing the tool framework.
46-81: Well-implemented ToolDefinition structure with enhanced validationThe new return structure properly implements the ToolDefinition interface with name, description, params schema, and a handler function. The handler includes proper validation to ensure either
objectPathorinstanceIdis provided, addressing the PR objective of stricter input validation.The response structure also includes more comprehensive information, including the instanceId in the content, which will help clients better handle the tool's response.
Server/build/tools/selectGameObjectTool.js (2)
33-59: Good implementation of ToolDefinition return structureThe JavaScript implementation correctly returns the ToolDefinition structure with name, description, paramsSchema, and handler properties. The handler function includes proper validation to ensure either
objectPathorinstanceIdis provided, matching the TypeScript implementation.The success response structure is also enhanced to include the instanceId in the content, which aligns with the PR objectives of providing more specific information for clients.
21-32: Update tool registration to use the new handlerWith the handler function now part of the returned ToolDefinition, you should update the tool registration to use this handler directly or consider if both registrations are intentional.
Please clarify whether maintaining both the server.tool registration and returning a ToolDefinition is intentional. If both are needed, consider extracting the common handler logic to avoid duplication. If only one is needed, consider removing the duplicate registration.
Tests/README.md (2)
1-3: File Introduction is Clear
The introductory section clearly explains that this directory contains the unit tests for the MCP Unity Server package.
5-8: Explanation of Test Discovery Challenges
The "Important: Test Discovery in Unity" section is well written and details the limitations of test discovery in local packages effectively.Tests/McpTests.md (3)
1-3: Clear File Introduction
The title and opening lines set the context well for the instructions on running MCP Unity tests.
9-35: Assembly Definition Configuration is Solid
The JSON configuration for the Assembly Definition file is well structured and covers essential settings such as references, platforms, and precompiled references. Please verify that the provided GUIDs are valid in your Unity project context.
37-41: Final Steps Are Well Defined
The concluding steps to save the assembly definition file, compile it, and open the Test Runner are clearly stated.README.md (5)
5-7: Clear Setup and Requirements Section
The "Setup" and "Requirements" sections clearly outline the prerequisites (Unity 2022.3 or higher) needed to work with the package.
8-16: Badges and Localization Links are Well-Formatted
The badges and the language selection table are neatly organized and add useful context.🧰 Tools
🪛 LanguageTool
[style] ~8-~8: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 1670 characters long)
Context: ...# Requirements - Unity 2022.3 or higher
[
🪛 markdownlint-cli2 (0.17.2)
10-10: Images should have alternate text (alt text)
null(MD045, no-alt-text)
11-11: Images should have alternate text (alt text)
null(MD045, no-alt-text)
12-12: Images should have alternate text (alt text)
null(MD045, no-alt-text)
48-53: Installation Instructions are Clear
The installation steps for adding the package via the Unity Package Manager, as well as via themanifest.json, are straightforward and easy to follow.🧰 Tools
🪛 LanguageTool
[grammar] ~49-~49: Please check the verb form.
Context: ... ╚═╝ ``` ### Installation 1. Open the Package Manager in Unity 2. Click t...(SHE_LIVE)
54-62: Local Development Instructions are Accurate
The alternative method for adding the package from the filesystem is clearly explained. Ensure that the file paths match your project structure in Unity.
77-86:⚠️ Potential issueInconsistent Merge Resolution in License and Acknowledgements
There appears to be unresolved content related to the project's license and acknowledgements. Decide whether to include the "MIT License" information or the separate acknowledgements section, and remove any extraneous merge conflict artifacts.Editor/Tools/AddAssetToSceneTool.cs (1)
116-121: Confirm removal of fallback behavior when parent is missing.
This updated logic now returns an error rather than placing the asset at the scene root when the parent cannot be found. While this improves clarity and avoids silent failures, consider verifying that no existing workflows rely on the previous fallback.Do you want to confirm there isn't code expecting the asset to be instantiated at the root? If needed, I can provide a script to search for references relying on that behavior.
Editor/Tools/UpdateComponentTool.cs (11)
9-9: New using statement for LINQ.
This is necessary for the reflection-based queries.
14-14: Documentation looks good.
Clear summary describing the tool’s purpose.
30-43: Robust approach to find the target GameObject.
You correctly prioritizeinstanceIdoverobjectPath. This matches similar logic in other tools (e.g.,SelectGameObjectTool).
54-60: Validation of 'componentName' is appropriate.
Denying execution early ifcomponentNameis missing ensures cleaner error handling.
77-79: Graceful "Get or Add" approach.
This line is straightforward and handles scenarios where the component may not already exist.
81-94: Validation of 'componentData' parameter.
RequiringcomponentDataensures there's meaningful data to apply. This check is well-placed.
98-103: Properly marking objects as dirty and preserving prefab modifications.
Ensuring that changes are recorded prevents lost updates, especially in prefab scenarios.
107-126: Verify correct detection of newly added component.
The ternary in line 112 checkstargetObject.GetComponent(componentType) == nullto decide if the component was added vs. updated. Since you already add a new component if it doesn’t exist, that check will likely always return non-null. You may want to store a boolean flag if the component was newly added, then set the message accordingly.
128-135: Error aggregation logic looks clean.
Capturing the list of errors and assigning them to the response ensures the caller can see partial failures.
245-253: Return a list of error details instead of a bool.
Switching from a simple boolean to a list of JObjects for errors provides significantly more debugging context.
439-440: Error rethrow on cast failure is appropriate.
Logging the error and throwing an exception ensures the caller is informed of conversion failures without silently ignoring them.Editor/Tools/RunTestsTool.cs (10)
2-7: New imports for test services, collection usage, and editor-specific APIs.
These are all required for the new testing approach.
19-24: Added fields for capturing test progress.
Storing counts and results in_testResultsimproves the clarity and detail of final reporting.
28-34: Asynchronous test framework setup.
Custom description,IsAsync = true, and registration of callbacks sets the stage for robust test runs.
36-39: Throwing exception in synchronous Execute.
Clearly indicates that the tool is only designed for async usage, preventing accidental misuse.
43-51: Initialize test execution state.
Clearing and reinitializing counters each time avoids data contamination between runs.
66-77: Robust error handling for test execution.
Surrounding the call to_testRunnerService.ExecuteTestswith try/catch ensures the fallback is a structured JSON error response, rather than an unhandled exception.
79-85: RunStarted properly logs the total tests.
Storing_testCountis essential for final summary.
87-120: Consolidated final test summary.
You generate a well-structured response with pass/fail counts and an array of detailed test results. This effectively captures the overall test run outcome.
122-127: TestStarted callback logs the start of each test.
Clear logging ensures test-level visibility for debugging.
128-161: Detailed tracking of individual test results.
Incrementing pass/fail counters and appending structured info to_testResultshelps with thorough reporting.
|
Thank you for creating this PR @dsarno |
|
Sure thing @CoderGamester. A good way to test is to tell the client to It should see and run the basic unit tests in the package's test folder. |
There was a problem hiding this comment.
Revert the README changes or create a wiki to keep the old documentation present
There was a problem hiding this comment.
Is there a need for all this logs?
The information is already being returned to the MCP client to be debugged. Aren't the logs in each tool suficient?
There was a problem hiding this comment.
Is there a need for all this logs?
The information is already being returned to the MCP client to be debugged. Aren't the logs in each tool suficient?
There was a problem hiding this comment.
why this change?
ToolDefinition and toolregistry are no longer used in the project
There was a problem hiding this comment.
This changes makes the code not compile. Why the return?
There was a problem hiding this comment.
this is very good documentation but I suggest to move to wiki page, ~/docs folder
This way all documentation is properly setup
There was a problem hiding this comment.
This tests are not necessary. Remove them
There was a problem hiding this comment.
Why the changes on this tool?
Seems to be creating uncessary code and make ti more verbose to read.
Is there a value add for this changes?
There was a problem hiding this comment.
Why the changes on this toold?
Seems to be creating uncessary code and make ti more verbose to read.
Is there a value add for this changes?
There was a problem hiding this comment.
Why the changes on this toold?
Seems to be creating uncessary code and make ti more verbose to read.
Is there a value add for this changes?
@CoderGamester thanks for taking the time to review the PR. You're right, I think this is a little too ambitious and messy (I did use AI to help with it, but tested it and iterated it a lot myself). I might withdraw the PR and work on a new one that focuses on a test framework. For what it's worth, I'd originally started on this one to fix response formatting errors like the one below, but i believe you've taken care of most of those issues since I started working on them. 
|
@dsarno Makes sense. In the meantime, feel free to create any PR for improvements you have. Love the ideia for this MCP to become community driven |
PR Summary
This PR enhances several MCP Unity tools by adding stricter input validation, more specific error reporting, and more robust object/component handling. It also includes the setup and reorganization of a test framework for these tools.
Key Tool Enhancements:
NotifyMessageTool: Validates message content and type (info, warning, error enum).⚠️ RunTestsTool⚠️: Refactored execution, added mode validation, provides detailed pass/fail counts and individual results in the response. Handles 'no tests found' case.SelectGameObjectTool: RequiresinstanceIdorobjectPath, improves search logic (ID > path > name), provides specific 'not found' errors.UpdateComponentTool: RequirescomponentName/Data, improves search (ID > path), finds component type across assemblies, adds component if missing. Uses Reflection to safely update public properties (likeTransform.position,BoxCollider.center) and fields, performs type conversion, reports invalid/unwritable members or conversion errors individually, and correctly reports overall success/failure status.McpUnitySocketHandler: Added capability to look up Resources by URI.Testing Validation:
Manual testing using the available MCP tool calls confirmed that the core validation, error handling, and logic improvements for
NotifyMessageTool,RunTestsTool(EditMode),SelectGameObjectTool, andUpdateComponentTool(including property updates likeTransform.positionandBoxCollider.center) are functioning correctly.Testing Shortcomings & Areas for Further Review:
⚠️ RunTestsTool(PlayMode):AddAssetToSceneTool/AddPackageTool: Tests for these tools mostly failed due to the specific test environment lacking the required assets or packages, or potential connection issues (for packages). The tool logic itself wasn't fully validated by these tests, though basic parameter validation seems correct. Further testing in an environment with known valid/invalid assets and packages is needed.Editor/Tests/,Tests/) were part of this commit but haven't been fully run or validated yet.Overall: The enhancements to tool validation, error handling, and functionality (including component updates) appear solid. Further testing is advised for the asset/package management tools. The included test framework should also be reviewed and utilized. The PlayMode test result reporting limitation needs to be addressed separately.
Summary by CodeRabbit