Fix/scriptableobject tools #105
Conversation
CodexConfigHelperTests was calling MCPServiceLocator.Reset() in TearDown, which disposes the active bridge/transport during MCP-driven test runs. Replace with restoring only the mutated service (IPlatformService).
Capture the original IPlatformService before this fixture runs and restore it in TearDown. This preserves the MCP connection safety fix (no MCPServiceLocator.Reset()) while avoiding global state leakage to subsequent tests.
…sts; remove vestigial SO tools
WalkthroughAdds a new MCP-managed ScriptableObject workflow: design doc, Unity Editor tool to create/modify ScriptableObject assets, a Python server tool to forward requests, Unity editor tests and fixtures, integration tests, Unity project metadata, and a .gitignore entry for Temp artifacts. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as "Python Server"
participant Unity as "Unity Editor"
participant AssetDB as "Unity AssetDB"
rect `#EAF5FA`
Note over Client,Server: Create action (new feature)
Client->>Server: manage_scriptable_object(action="create", type_name, folder_path, asset_name, patches)
Server->>Server: validate & normalize params\n(parse JSON strings, coerce overwrite)
Server->>Unity: send_with_unity_instance / async_send_command_with_retry(params)
Unity->>Unity: check compiling/reloading state\nresolve typeName
Unity->>AssetDB: ensure folders exist\ncreate or overwrite asset
Unity->>Unity: apply patches via SerializedObject/SerializedProperty
Unity-->>Server: {guid, path, typeNameResolved, patchResults, warnings}
Server-->>Client: forward response
end
rect `#F9F0E6`
Note over Client,Server: Modify action
Client->>Server: manage_scriptable_object(action="modify", target, patches)
Server->>Server: validate target & patches
Server->>Unity: send_with_unity_instance / async_send_command_with_retry(params)
Unity->>Unity: resolve target (guid or path)
Unity->>Unity: apply patches (set, array_resize, object refs)
Unity-->>Server: {targetGuid/Path/Type, results}
Server-->>Client: forward response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 |
Greptile SummaryThis PR introduces a robust, unified ScriptableObject management tool that replaces previous implementations with a SerializedObject/SerializedProperty-based approach, fixing placement issues and enabling reliable patching of private fields, inherited fields, arrays, and object references. Key improvements:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant MCP as MCP Client
participant Server as Python Server
participant Unity as Unity Editor
participant SO as SerializedObject
participant AssetDB as AssetDatabase
Note over MCP,AssetDB: Create ScriptableObject Flow
MCP->>Server: manage_scriptable_object(action="create", type_name, folder_path, asset_name, patches)
Server->>Server: parse_json_payload(target, patches)
Server->>Server: coerce_bool(overwrite)
Server->>Unity: send_command("manage_scriptable_object", params)
Unity->>Unity: Check EditorApplication.isCompiling/isUpdating
alt Unity is compiling/reloading
Unity-->>Server: ErrorResponse("compiling_or_reloading")
Server-->>MCP: {success: false, hint: "retry"}
end
Unity->>Unity: TryNormalizeFolderPath(folderPath)
Note over Unity: Validate/reject Packages/, absolute, file:// paths<br/>Normalize backslashes, root under Assets/
Unity->>Unity: EnsureFolderExists(normalizedFolder)
Unity->>AssetDB: CreateFolder (recursively if needed)
Unity->>Unity: ResolveType(typeName)
Unity->>Unity: ScriptableObject.CreateInstance(resolvedType)
Unity->>AssetDB: CreateAsset(instance, finalPath)
Unity->>AssetDB: ImportAsset(finalPath)
alt patches provided
Unity->>SO: new SerializedObject(instance)
loop for each patch
Unity->>SO: FindProperty(propertyPath)
alt array_resize op
Unity->>SO: set arraySize or intValue
Unity->>SO: ApplyModifiedProperties()
Note over Unity,SO: Immediate apply for array resize
else set op
alt ObjectReference type
Unity->>AssetDB: GUIDToAssetPath or SanitizeAssetPath
Unity->>AssetDB: LoadAssetAtPath
Unity->>SO: set objectReferenceValue
else primitive/enum
Unity->>SO: set intValue/stringValue/boolValue/enumValueIndex
end
end
end
Unity->>SO: ApplyModifiedProperties()
end
Unity->>AssetDB: SetDirty(instance)
Unity->>AssetDB: SaveAssets()
Unity->>AssetDB: AssetPathToGUID(finalPath)
Unity-->>Server: SuccessResponse(guid, path, patchResults)
Server-->>MCP: {success: true, data: {guid, path, ...}}
Note over MCP,AssetDB: Modify ScriptableObject Flow
MCP->>Server: manage_scriptable_object(action="modify", target={guid|path}, patches)
Server->>Unity: send_command("manage_scriptable_object", params)
Unity->>Unity: TryResolveTarget(target)
alt by GUID
Unity->>AssetDB: GUIDToAssetPath(guid)
else by path
Unity->>Unity: SanitizeAssetPath(path)
end
Unity->>AssetDB: LoadAssetAtPath(resolvedPath)
Unity->>SO: new SerializedObject(target)
Unity->>SO: Update()
loop for each patch
Unity->>SO: FindProperty(propertyPath)
Unity->>Unity: ApplyPatch (same as create flow)
end
Unity->>SO: ApplyModifiedProperties()
Unity->>AssetDB: SetDirty(target)
Unity->>AssetDB: SaveAssets()
Unity-->>Server: SuccessResponse(targetGuid, targetPath, results)
Server-->>MCP: {success: true, data: {...}}
|
Greptile's behavior is changing!From now on, if a review finishes with no comments, we will not post an additional "statistics" comment to confirm that our review found nothing to comment on. However, you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs (1)
289-363: Consider clarifying array resize fallback logic (optional).The implementation correctly handles Unity's dual representation of array size (
.Array.sizesynthetic property vs.arraySizeon the array property itself). The fallback logic at lines 305-315 works but is complex.The current implementation passes all tests and handles the edge cases Unity's serialization system presents. If you want to improve clarity, consider adding more inline comments explaining each fallback branch.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.gitignoreFixscriptableobjecPlan.mdMCPForUnity/Editor/Tools/ManageScriptableObject.csMCPForUnity/Editor/Tools/ManageScriptableObject.cs.metaServer/src/services/tools/manage_scriptable_object.pyServer/tests/integration/test_manage_scriptable_object_tool.pyTestProjects/UnityMCPTests/Assets/Packages.metaTestProjects/UnityMCPTests/Assets/Temp.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/ManageScriptableObjectTestDefinition.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/ManageScriptableObjectTestDefinition.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/ManageScriptableObjectTestDefinitionBase.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/ManageScriptableObjectTestDefinitionBase.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs.metaTestProjects/UnityMCPTests/ProjectSettings/SceneTemplateSettings.json
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-04T00:02:45.818Z
Learnt from: dsarno
Repo: dsarno/unity-mcp PR: 60
File: TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/TestAsmdef.asmdef.meta:1-7
Timestamp: 2025-09-04T00:02:45.818Z
Learning: Unity assembly definition (.asmdef) files use name-based references by default, not GUID references. When an .asmdef file references another assembly, it uses the target assembly's name (from the "name" field) in the "references" array, and Unity automatically resolves this to the appropriate GUID at build time. Direct GUID references are not required for standard Unity assembly references.
Applied to files:
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/ManageScriptableObjectTestDefinitionBase.cs.meta
📚 Learning: 2025-09-04T00:02:45.818Z
Learnt from: dsarno
Repo: dsarno/unity-mcp PR: 60
File: TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/TestAsmdef.asmdef.meta:1-7
Timestamp: 2025-09-04T00:02:45.818Z
Learning: Unity assembly definition (.asmdef) files use name-based references by default, not GUID references. When an .asmdef file references another assembly, it uses the target assembly's name (from the "name" field) in the "references" array, and Unity automatically resolves this to the appropriate GUID at build time. Direct GUID references are not required for standard Unity assembly references. Name-based references are the standard and recommended approach.
Applied to files:
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/ManageScriptableObjectTestDefinitionBase.cs.meta
🧬 Code graph analysis (4)
Server/tests/integration/test_manage_scriptable_object_tool.py (2)
Server/tests/integration/test_helpers.py (2)
DummyContext(16-55)set_state(49-51)Server/src/services/tools/manage_scriptable_object.py (1)
manage_scriptable_object(27-72)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/ManageScriptableObjectTestDefinition.cs (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/ManageScriptableObjectTestDefinitionBase.cs (1)
ManageScriptableObjectTestDefinitionBase(6-10)MCPForUnity/Editor/Tools/ManageScriptableObject.cs (1)
List(213-265)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs (2)
MCPForUnity/Editor/Tools/JsonUtil.cs (2)
JsonUtil(6-29)CoerceJsonStringParameter(12-28)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-201)SanitizeAssetPath(20-34)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs (2)
MCPForUnity/Editor/Tools/ManageAsset.cs (2)
CreateAsset(163-302)CreateFolder(304-357)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-201)SanitizeAssetPath(20-34)
🪛 Ruff (0.14.10)
Server/tests/integration/test_manage_scriptable_object_tool.py
10-10: Unused function argument: kwargs
(ARG001)
46-46: Unused function argument: kwargs
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🔇 Additional comments (30)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures.meta (1)
1-8: LGTM! Standard Unity folder metadata.This is a properly formatted Unity
.metafile for the Fixtures folder. Unity automatically generates these files to track folder assets in the project, and the format follows Unity's standard conventions with appropriate default values.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (2)
35-35: LGTM! Clean capture of original state.Capturing the original platform service in
OneTimeSetUpensures that the pre-fixture state is preserved and can be restored after each test, preventing state leakage to other test fixtures.Also applies to: 44-44
59-68: LGTM! Proper test isolation without disrupting active MCP sessions.The new approach correctly addresses the issue where
MCPServiceLocator.Reset()would terminate active MCP connections during test execution. By restoring only the mutated platform service, the tests remain properly isolated while preserving any active MCP bridge.The null check is appropriately defensive, and the detailed comment clearly explains the rationale.
.gitignore (1)
37-38: LGTM. Clear comment explaining why temporary artifacts should be ignored, and the rule is appropriately scoped to the test project.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs.meta (1)
1-11: LGTM. Standard Unity MonoImporter metadata for the test script.TestProjects/UnityMCPTests/Assets/Packages.meta (1)
1-8: LGTM. Standard Unity folder metadata.TestProjects/UnityMCPTests/Assets/Temp.meta (1)
1-8: LGTM. Standard folder metadata; aligns with the .gitignore rule for this artifact location.TestProjects/UnityMCPTests/ProjectSettings/SceneTemplateSettings.json (1)
1-121: LGTM. Valid JSON configuration with expected structure for Unity scene template settings. The 23 dependency types provide comprehensive coverage for common asset types.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/ManageScriptableObjectTestDefinition.cs.meta (1)
1-11: LGTM. Standard Unity MonoImporter metadata for the test fixture.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/ManageScriptableObjectTestDefinitionBase.cs.meta (1)
1-11: LGTM. Standard Unity MonoImporter metadata for the test base class.MCPForUnity/Editor/Tools/ManageScriptableObject.cs.meta (1)
1-14: LGTM. Standard Unity MonoImporter metadata for the editor tool.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/ManageScriptableObjectTestDefinitionBase.cs (1)
1-14: LGTM! Clean test fixture design.The base fixture follows Unity best practices with a private
[SerializeField]field exposed via a read-only property. The comment about file naming is helpful for maintainability.Server/tests/integration/test_manage_scriptable_object_tool.py (3)
7-41: LGTM! Comprehensive create parameter forwarding test.The test thoroughly validates that create parameters are correctly parsed and forwarded, including boolean coercion for
overwriteand JSON string parsing forpatches.
43-70: LGTM! Good coverage of modify parameter forwarding.The test validates that modify parameters (target as JSON string, patches as list) are correctly parsed and forwarded. The target JSON string parsing is particularly important for LLM-generated payloads.
10-10: Static analysis false positive on unusedkwargs.The
kwargsparameter is required to match the signature of the mockedasync_send_command_with_retryfunction. This is a false positive from Ruff.Also applies to: 46-46
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/ManageScriptableObjectTestDefinition.cs (1)
1-27: LGTM! Well-structured test fixture.The fixture design effectively tests the tool's capabilities:
- Nested struct (ManageScriptableObjectNestedData) for testing nested property paths
- List for testing array resize and object reference assignment
- Private serialized fields exposed via read-only properties
The use of modern C# collection initialization (
new()) is appropriate.MCPForUnity/Editor/Tools/ManageScriptableObject.cs (6)
42-77: LGTM! Robust entry point with good error handling.The implementation handles several important cases:
- Returns retryable error during compilation/reload (lines 49-53)
- Tolerates JSON-string parameters for LLM compatibility (lines 56-57)
- Validates action with clear error messages and lists valid actions
The structured error responses with error codes facilitate client-side retry logic.
79-178: LGTM! Comprehensive create workflow.The create handler is thorough:
- Validates all required parameters with clear error messages
- Normalizes and validates folder paths (lines 101-109)
- Ensures folders exist with recursive creation (line 106)
- Resolves type and validates ScriptableObject inheritance (lines 111-115)
- Handles overwrite semantics correctly (lines 123-126)
- Applies patches after creation (lines 157-162)
- Properly persists changes (lines 164-165)
Exception handling during CreateInstance and CreateAsset provides clear diagnostics.
180-265: LGTM! Well-designed patch application.Key strengths:
- Target resolution supports both GUID and path (line 182)
- Validates patches parameter type (lines 188-196)
- Processes patches in order with per-patch error handling (lines 222-248)
- Critically important: array_resize changes are applied immediately (lines 250-254) so subsequent patches can resolve newly created array elements
- Batches remaining changes for efficiency (lines 257-262)
The per-patch result tracking ensures clients can identify which specific patches succeeded or failed.
365-478: LGTM! Robust type-safe property setting.The implementation handles Unity's SerializedProperty system correctly:
- Object references support GUID, path, and explicit null (lines 374-404)
- Type-safe setters for each SerializedPropertyType (lines 417-446)
- Enum handling accepts both integer index and case-insensitive string name (lines 455-478)
- Helper methods (TryGetInt/Float/Bool) handle type coercion and parsing
The error messages are descriptive and aid debugging.
606-653: LGTM! Excellent path validation and normalization.The path normalization includes important security and correctness checks:
- Rejects absolute paths and file:// URIs (lines 624-628)
- Rejects non-Assets roots like Packages/, ProjectSettings/, Library/ (lines 630-636)
- Normalizes backslashes and removes double slashes (lines 617-621)
- Automatically roots relative paths under Assets/ (line 651)
This prevents directory traversal and ensures all created assets are in safe, expected locations.
707-746: LGTM! Comprehensive type resolution with robust fallbacks.The type resolution strategy is thorough:
- Tries Type.GetType first (fast path, line 711)
- Scans assemblies with asm.GetType (line 714-725)
- Falls back to scanning all types by FullName (lines 728-743)
- Handles ReflectionTypeLoadException gracefully (line 732)
This ensures types can be found even in edge cases where standard lookup fails, which is important for Unity's dynamic assembly landscape.
FixscriptableobjecPlan.md (1)
1-206: Documentation provides excellent design context.This design document clearly outlines the tool's requirements, error codes, patch language, and acceptance criteria. It serves as useful reference material for understanding the implementation decisions in ManageScriptableObject.cs.
Server/src/services/tools/manage_scriptable_object.py (1)
1-75: LGTM! Clean server-side wrapper with good validation.The wrapper implementation is solid:
- Tolerates JSON-string payloads for complex objects (lines 42-44) - important for LLM compatibility
- Validates payload shapes with clear error messages (lines 46-50)
- Coerces
overwriteto boolean (line 57)- Removes None values to simplify Unity-side handling (line 63)
- Provides detailed type annotations with Annotated for tool documentation
The error handling ensures malformed requests are caught before reaching Unity.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs (6)
22-84: LGTM! Well-designed test lifecycle.The SetUp and TearDown ensure test isolation:
- Starts from clean slate (deletes TempRoot if it exists, lines 27-30)
- Creates test material assets with unique names (lines 37-47)
- TearDown performs best-effort cleanup (lines 50-84)
- Cleans up empty parent Temp folder (lines 73-81)
The use of unique GUIDs in material names prevents test interference.
86-124: LGTM! Comprehensive create + patch test.This test validates the core create workflow:
- Creates nested folders that don't exist (line 114)
- Places asset correctly under the target folder (line 115)
- Applies patches to private serialized field
displayName(line 121)- Applies patches to inherited field
baseNumber(line 122)- Applies patches to nested struct field
nested.note(line 123)The test covers the acceptance criteria from the design document.
126-189: LGTM! Thorough test of array and reference handling.This test validates complex patching scenarios:
- Resizes array using
.Array.sizepath (line 152)- Assigns object reference by GUID (lines 154-159)
- Assigns object reference by path (lines 161-166)
- Verifies per-patch results (lines 174-179)
- Verifies final persisted state (lines 181-188)
The combination of resize + reference assignment exercises the immediate-apply logic for array resizing.
191-219: LGTM! Good error case coverage.The test validates error handling for:
- Invalid action →
invalid_paramserror code- Type not found →
type_not_founderror code- Target not found →
target_not_founderror codeThe structured error codes enable client-side retry and error handling logic.
221-256: LGTM! Critical security validation tests.These tests ensure path validation prevents unauthorized file system access:
- Rejects Packages/ root (lines 224-233)
- Rejects absolute paths like /tmp/ (lines 235-244)
- Rejects file:// URIs (lines 246-255)
All cases correctly return
invalid_folder_patherror code.
258-278: LGTM! Path normalization verification.This test validates that:
- Backslashes are converted to forward slashes
- Relative paths are rooted under Assets/
- Double slashes are removed (line 277)
- Final path is correct (lines 275-276)
The test input deliberately includes multiple path issues to verify robust normalization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs (4)
30-40: Consider simplifying ValidActions set.Since
NormalizeAction(line 704) removes underscores/hyphens and the set usesOrdinalIgnoreCase, many entries are redundant. The set could be reduced to just the normalized forms:["create", "createso", "modify", "modifyso"].🔎 Proposed simplification
private static readonly HashSet<string> ValidActions = new(StringComparer.OrdinalIgnoreCase) { "create", - "createso", - "create_so", - "createSO", + "createso", "modify", - "modifyso", - "modify_so", - "modifySO", + "modifyso" };
157-165: Consider conditional SaveAssets call.
AssetDatabase.SaveAssets()is called unconditionally, even when no patches are applied. While this ensures the new asset is persisted, it might be unnecessary sinceCreateAsset+ImportAssettypically handle persistence. Consider making the save conditional onpatchesToken != null.🔎 Proposed optimization
if (patchesToken is JArray patches && patches.Count > 0) { var patchApply = ApplyPatches(instance, patches); patchResults = patchApply.results; warnings.AddRange(patchApply.warnings); + // ApplyPatches already calls SaveAssets if changes were made +} +else +{ + // Ensure new asset is saved even without patches + EditorUtility.SetDirty(instance); + AssetDatabase.SaveAssets(); } - -EditorUtility.SetDirty(instance); -AssetDatabase.SaveAssets();
426-462: Consider supporting additional SerializedPropertyType values.
TrySetValuecurrently supports Integer, Boolean, Float, String, and Enum, but common Unity types like Vector2/3/4, Color, Rect, Bounds, etc. are unsupported. This limits patching capabilities for many ScriptableObject fields.Example: Adding Vector3 support
case SerializedPropertyType.String: prop.stringValue = valueToken.Type == JTokenType.Null ? null : valueToken.ToString(); message = "Set string."; return true; +case SerializedPropertyType.Vector3: + if (valueToken is not JObject vec || + !TryGetFloat(vec["x"], out float x) || + !TryGetFloat(vec["y"], out float y) || + !TryGetFloat(vec["z"], out float z)) + { + message = "Expected object with x, y, z float fields."; return false; + } + prop.vector3Value = new Vector3(x, y, z); + message = "Set Vector3."; return true; + case SerializedPropertyType.Enum: return TrySetEnum(prop, valueToken, out message);Similar patterns can be added for Vector2, Vector4, Color, etc.
554-662: Consider extracting common path sanitization logic.Both
EnsureFolderExists(lines 564-568) andTryNormalizeFolderPath(lines 626-630) duplicate the slash normalization logic. Extracting to a helper method would improve maintainability.🔎 Proposed helper method
+private static string SanitizeSlashes(string path) +{ + if (string.IsNullOrWhiteSpace(path)) return path; + var s = path.Replace('\\', '/'); + while (s.IndexOf("//", StringComparison.Ordinal) >= 0) + { + s = s.Replace("//", "/", StringComparison.Ordinal); + } + return s; +} + private static bool EnsureFolderExists(string folderPath, out string error) { error = null; // ... - string sanitized = folderPath.Replace('\\', '/'); - while (sanitized.IndexOf("//", StringComparison.Ordinal) >= 0) - { - sanitized = sanitized.Replace("//", "/", StringComparison.Ordinal); - } + string sanitized = SanitizeSlashes(folderPath); // ... } private static bool TryNormalizeFolderPath(string folderPath, out string normalized, out string error) { // ... - var s = folderPath.Trim().Replace('\\', '/'); - while (s.IndexOf("//", StringComparison.Ordinal) >= 0) - { - s = s.Replace("//", "/", StringComparison.Ordinal); - } + var s = SanitizeSlashes(folderPath.Trim()); // ... }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs
🔇 Additional comments (10)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs (10)
20-21: LGTM: Manual registration is appropriate for MCP server integration.The
AutoRegister = falseattribute correctly indicates that this tool is registered manually via the Python server wrapper rather than through automatic discovery.
42-77: LGTM: Robust entry point with proper validation.The compilation state check, JSON coercion, action validation, and dispatch logic are all well-implemented. The retryable error code for transient Unity states is particularly good for MCP client resilience.
123-126: Verify overwrite behavior is acceptable for your use case.The asset deletion when
overwrite=trueis destructive and irreversible. Ensure that:
- Callers understand this behavior
- There are no references to the old asset that would be broken
- Version control or other backup mechanisms are in place if needed
180-211: LGTM: Clean modify implementation.The target resolution, patch validation, and application logic are well-structured with appropriate error handling.
213-265: LGTM: Well-designed patching with proper array resize handling.The immediate
ApplyModifiedPropertiesafter array resize (lines 249-254) is critical for ensuring subsequent patches can reference the resized array elements. The change tracking and conditional save logic are also excellent.
289-372: LGTM: Robust array resize with necessary fallback logic.The multiple fallback strategies (lines 307-372) correctly handle Unity's serialization quirks across versions. The detailed comments explain the rationale, and the implementation covers all edge cases including synthetic ".Array.size" properties and direct array properties.
464-487: LGTM: Flexible enum handling.Supporting both integer indices and string names with case-insensitive matching makes the API user-friendly and robust.
489-532: LGTM: Comprehensive target resolution.The dual GUID/path resolution with proper validation and error messages is well-implemented.
716-755: LGTM: Comprehensive type resolution with appropriate fallbacks.The multi-stage approach (Type.GetType → Assembly.GetType → FullName scan) is necessary for Unity's assembly structure. While the fallback scan (lines 737-752) is expensive, it's acceptable since type resolution happens infrequently and ensures comprehensive coverage.
534-552: LGTM: Robust helper methods.The JSON coercion with fallback warnings (lines 534-552) and the flexible Try* methods supporting multiple token types (lines 664-702) make the API resilient to various input formats.
Also applies to: 664-714
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs (2)
287-370: Consider documenting tested Unity versions for array resize logic.The array resize implementation includes sophisticated fallback logic to handle Unity version differences in how array properties are represented (direct
arraySizevs synthetic.Array.sizeproperty). While well-commented, this complexity could be fragile across Unity versions.Consider adding a comment documenting which Unity versions have been tested with this implementation, to help future maintainers understand the support matrix.
721-760: Type resolution is comprehensive but potentially slow.The type resolution includes three fallback strategies, with the final fallback (lines 742-757) iterating through all types in all assemblies. For large projects with many assemblies, this could be slow.
Consider caching resolved types if repeated lookups are expected, or log a performance warning when falling back to the full type scan. However, for single create operations, the current approach is acceptable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs
🔇 Additional comments (5)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs (5)
40-75: LGTM! Well-structured entry point with proper validation.The entry point correctly handles Unity's transient compilation state with a retry hint, validates parameters, and dispatches to the appropriate handler. The dual JSON coercion approach (object vs array) is appropriate for the different parameter types.
178-209: LGTM! Clean modification workflow with good error handling.The method properly resolves the target asset, validates the patches parameter, and returns detailed results including per-patch outcomes and warnings.
248-252: Good design: immediate application for array resizing.Applying array resize changes immediately via
ApplyModifiedProperties()ensures that subsequent patches referencing array elements (e.g.,items.Array.data[0]) will resolve correctly. This is essential for reliable patch ordering.
624-667: Excellent path validation and security boundaries.The folder path normalization properly rejects non-Assets paths (Packages/, absolute paths, file:// URLs, etc.) and consistently roots relative paths under Assets/. This prevents accidental or malicious writes outside the project's asset directory.
381-411: ObjectReference resolution is well-designed.The implementation supports both GUID and path resolution for object references, with proper null handling. The use of
AssetPathUtility.SanitizeAssetPathfor path-based resolution is consistent with the target resolution logic (line 511), ensuring uniform path handling throughout the tool.
…criptableObject tools
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs (2)
234-286: Consider disposing SerializedObject explicitly.While Unity's garbage collector typically handles
SerializedObjectcleanup, explicit disposal viausingstatement is a best practice to ensure timely resource release.🔎 Proposed refactor to dispose SerializedObject
-var so = new SerializedObject(target); -so.Update(); +using (var so = new SerializedObject(target)) +{ + so.Update(); -for (int i = 0; i < patches.Count; i++) -{ + for (int i = 0; i < patches.Count; i++) + { // ... existing patch logic ... -} + } -if (anyChanged) -{ - so.ApplyModifiedProperties(); - EditorUtility.SetDirty(target); - AssetDatabase.SaveAssets(); -} + if (anyChanged) + { + so.ApplyModifiedProperties(); + EditorUtility.SetDirty(target); + AssetDatabase.SaveAssets(); + } -return (results, warnings); + return (results, warnings); +}
447-500: Document ObjectReference support in the comment.The comment on line 452 lists supported types but omits
ObjectReference, which is actually supported (handled separately inApplySetlines 404-434). Including it would improve clarity.🔎 Proposed documentation update
-// Supported Types: Integer, Boolean, Float, String, Enum, Vector2, Vector3, Vector4, Color +// Supported Types: Integer, Boolean, Float, String, Enum, ObjectReference, Vector2, Vector3, Vector4, Color +// Note: ObjectReference is handled separately in ApplySet before calling TrySetValue
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs
🧰 Additional context used
🧬 Code graph analysis (1)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs (2)
MCPForUnity/Editor/Tools/JsonUtil.cs (2)
JsonUtil(6-29)CoerceJsonStringParameter(12-28)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-201)SanitizeAssetPath(20-34)
🔇 Additional comments (5)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs (5)
135-171: Past issues successfully resolved.The GUID-preserving overwrite logic has been properly implemented using
EditorUtility.CopySerializedwhen the existing asset type matches, addressing the previous concern about breaking references. The fallback to delete-and-recreate for type mismatches is appropriate.
40-75: LGTM: Well-structured entry point.The parameter validation, JSON string coercion, and retryable error handling for transient Unity states (
compiling_or_reloading) are well-designed for reliable MCP integration.
77-199: LGTM: Robust asset creation with proper validation.The parameter validation, folder normalization, type resolution, GUID-preserving overwrite, and optional patch application are well-coordinated. Error handling covers all failure paths.
664-707: LGTM: Secure path validation.The folder path normalization properly rejects non-project paths (absolute paths,
file:URIs,Packages/, etc.) and roots relative paths underAssets/as documented in the PR objectives. This prevents path traversal and ensures assets are created in valid locations.
879-918: LGTM: Comprehensive type resolution.The multi-stage type resolution (direct
Type.GetType, assembly scan, and fallback toFullNamematching withReflectionTypeLoadExceptionhandling) is thorough and appropriate for editor tools where performance is less critical than reliability.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs (1)
760-874: Inconsistent validation: object parsing accepts partial values, array parsing doesn't.The Vector2/3/4 and Color parsing helpers treat array and object inputs inconsistently:
- Array syntax (e.g.,
[1, 2]) requires all components to be present and valid.- Object syntax (e.g.,
{"x": 1}) accepts partial objects, filling missing components with zero (or 1 for alpha).This inconsistency could lead to silent errors when users provide incomplete objects expecting validation to fail.
Specific examples:
- Line 780:
TryGetVector2accepts{"x": 1}→Vector2(1, 0)- Lines 806-811:
TryGetVector3accepts{}→Vector3(0, 0, 0)(always returns true)- Lines 834-840:
TryGetVector4accepts{}→Vector4(0, 0, 0, 0)(always returns true)- Lines 866-871:
TryGetColoraccepts{}→Color(0, 0, 0, 1)(always returns true)Consider requiring all required components when parsing objects for clarity and consistency with array parsing.
🔎 Proposed fix to require all components
// TryGetVector2 object parsing if (token is JObject obj) { float x = 0, y = 0; bool hasX = TryGetFloat(obj["x"], out x); bool hasY = TryGetFloat(obj["y"], out y); - if (hasX || hasY) // partial okay? usually stricter is better, but allow flexible + if (hasX && hasY) // require both x and y { value = new Vector2(x, y); return true; } }Apply similar strictness to
TryGetVector3(require x, y, z),TryGetVector4(require x, y, z, w), andTryGetColor(require r, g, b; allow optional a with default 1).For Vector3:
if (token is JObject obj) { float x = 0, y = 0, z = 0; - TryGetFloat(obj["x"], out x); - TryGetFloat(obj["y"], out y); - TryGetFloat(obj["z"], out z); - value = new Vector3(x, y, z); - return true; + bool hasX = TryGetFloat(obj["x"], out x); + bool hasY = TryGetFloat(obj["y"], out y); + bool hasZ = TryGetFloat(obj["z"], out z); + if (hasX && hasY && hasZ) + { + value = new Vector3(x, y, z); + return true; + } } +return false;Apply analogous changes to
TryGetVector4andTryGetColor.
🧹 Nitpick comments (1)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs (1)
175-175: Clarify why AssetDatabase.ImportAsset is commented out.The commented
AssetDatabase.ImportAsset(finalPath)call may be intentional (sinceSaveAssets()is called later at line 195), but this should be documented to prevent confusion.
…ve broken SO logic from ManageAsset
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs (1)
676-719: Consider Windows absolute path detection.The folder path normalization rejects Unix-style absolute paths (
/...) and URI schemes (file:...), but doesn't explicitly check for Windows drive letters (e.g.,C:/...,D:\...). While Unity paths should always be project-relative, adding a check likeRegex.IsMatch(s, @"^[A-Za-z]:[/\\]")would be more defensive.🔎 Proposed enhancement
var s = SanitizeSlashes(folderPath.Trim()); // Reject obvious non-project/invalid roots. We only support Assets/ (and relative paths that will be rooted under Assets/). -if (s.StartsWith("/", StringComparison.Ordinal) || s.StartsWith("file:", StringComparison.OrdinalIgnoreCase)) +if (s.StartsWith("/", StringComparison.Ordinal) + || s.StartsWith("file:", StringComparison.OrdinalIgnoreCase) + || System.Text.RegularExpressions.Regex.IsMatch(s, @"^[A-Za-z]:[/\\]")) { error = "Folder path must be a project-relative path under Assets/."; return false; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Tools/ManageAsset.csMCPForUnity/Editor/Tools/ManageScriptableObject.csServer/src/services/tools/manage_asset.py
🧰 Additional context used
🧬 Code graph analysis (1)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs (1)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-201)SanitizeAssetPath(20-34)
🔇 Additional comments (7)
Server/src/services/tools/manage_asset.py (1)
25-25: LGTM! Clear documentation update.The docstring update appropriately directs users to the new dedicated
manage_scriptable_objecttool for ScriptableObject workflows, aligning with the broader refactoring described in the PR objectives.MCPForUnity/Editor/Tools/ManageScriptableObject.cs (4)
100-103: Good validation for path separators.The assetName validation correctly rejects path separators, addressing the concern from past reviews. This prevents unexpected nested paths or failures during asset creation.
141-178: Excellent GUID-preserving overwrite implementation.The code correctly uses
EditorUtility.CopySerializedto preserve the asset GUID during overwrite, preventing broken references. The explicit name restoration at line 156 fixes the "Main Object Name does not match filename" warning. Well done addressing the past review concern.
464-505: Comprehensive SerializedPropertyType coverage.The implementation now supports Integer, Boolean, Float, String, Enum, Vector2, Vector3, Vector4, and Color types, with clear comments documenting supported types. This addresses the past review concern about limited type support and provides a solid foundation for common use cases.
763-787: Strict Vector2 parsing implemented.The object parsing now requires both
xandycomponents (line 780 uses&&), providing consistent validation with the array parsing. This addresses the past review concern about overly permissive partial object parsing.MCPForUnity/Editor/Tools/ManageAsset.cs (2)
423-429: Clear deprecation guidance for ScriptableObject modification.The deprecation comments appropriately direct users to the new
manage_scriptable_objecttool while maintaining backward compatibility for simple property setting. This provides a smooth migration path.
356-412: Well-structured GameObject component modification flow.The new component modification logic properly uses
ComponentResolverfor type resolution, iterates through component properties, applies nested properties viaApplyObjectProperties, and logs helpful warnings for unresolved or missing components. Themodifiedflag tracking ensures assets are only saved when changes occur.
Summary
This PR makes ScriptableObject creation/patching reliable and repeatable when driven via MCP by:
manage_scriptable_object) backed by UnitySerializedObject/SerializedPropertypaths (instead of reflection).Motivation / problems addressed
Matches the failure modes called out in
FixscriptableobjecPlan.md:[SerializeField]fields.Array.size+.Array.data[i])Key changes
Unity Editor tool
MCPForUnity/Editor/Tools/ManageScriptableObject.csaction=create(create asset + optional patches)action=modify(patch existing asset by{guid|path})SerializedObject.FindProperty(propertyPath)and applies typed writes, including object reference assignment by GUID/path.Assets/Packages/, absolute paths, andfile://…withinvalid_folder_path.Python server wrapper
Server/src/services/tools/manage_scriptable_object.pyto expose the unified tool as a first-class MCP tool.Tests
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/…covering:[SerializeField]patching//in result)Server/tests/integration/test_manage_scriptable_object_tool.pyRepo hygiene
.gitignore: ignoreTestProjects/UnityMCPTests/Assets/Temp/so temporary test assets never get committed again.Server/build/lib/...) so they don’t appear in the tool list anymore.How to test
156/156 passed0)78 passed, 2 skipped, 7 xpassedNotes
manage_scriptable_object(create/modify + patches) using Unity property paths, matching the design direction inFixscriptableobjecPlan.md.Summary by CodeRabbit
New Features
Documentation
Chores
Tests
Behavior Change
✏️ Tip: You can customize this high-level summary in your review settings.