feature/Add new manage_scriptable_object tool #489
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
…criptableObject tools
… redundant import
…ve broken SO logic from ManageAsset
Contributor
Reviewer's GuideIntroduces a dedicated manage_scriptable_object workflow, deprecates ScriptableObject creation in the generic manage_asset tool, wires the new SO tool through the Python server, and adds focused Unity/Python tests plus minor test-harness and .gitignore hygiene. Sequence diagram for manage_scriptable_object create with patchessequenceDiagram
actor User
participant Client
participant PythonServer
participant ToolWrapper as manage_scriptable_object_py
participant UnityTransport
participant UnityEditor
participant CSHandler as ManageScriptableObject_cs
participant AssetDB
User->>Client: Request create ScriptableObject
Client->>PythonServer: Tool call manage_scriptable_object\n{action:create, type_name, folder_path, asset_name, overwrite, patches}
PythonServer->>ToolWrapper: Invoke tool function
ToolWrapper->>ToolWrapper: parse_json_payload(target, patches)\ncoerce_bool(overwrite)
ToolWrapper->>UnityTransport: send_with_unity_instance\ncommand: manage_scriptable_object\nparams: {action, typeName, folderPath, assetName, overwrite, patches}
UnityTransport->>UnityEditor: async_send_command_with_retry
UnityEditor->>CSHandler: HandleCommand(JObject params)
CSHandler->>CSHandler: NormalizeAction(action)\nJsonUtil.CoerceJsonStringParameter\nCoerceJsonStringArrayParameter
CSHandler->>CSHandler: HandleCreate(params)
CSHandler->>CSHandler: TryNormalizeFolderPath(folderPath)
CSHandler->>CSHandler: EnsureFolderExists(normalizedFolder)
CSHandler->>CSHandler: ResolveType(typeName)
CSHandler->>AssetDB: AssetDatabase.CreateAsset or overwrite path
AssetDB-->>CSHandler: guid, finalPath
CSHandler->>CSHandler: ApplyPatches(instance, patches)
CSHandler->>CSHandler: SerializedObject.Update\nApplyPatch per patch
CSHandler->>CSHandler: ApplyArrayResize / ApplySet\nTrySetValue, TrySetEnum
CSHandler->>AssetDB: ApplyModifiedProperties\nSaveAssets
CSHandler-->>UnityEditor: SuccessResponse{guid, path, patchResults, warnings}
UnityEditor-->>UnityTransport: response
UnityTransport-->>ToolWrapper: response
ToolWrapper-->>PythonServer: dict response
PythonServer-->>Client: tool result
Client-->>User: Created SO info
Class diagram for ManageScriptableObject C# tool and Python wrapperclassDiagram
class ManageScriptableObject {
<<static>>
- CodeCompilingOrReloading : string
- CodeInvalidParams : string
- CodeTypeNotFound : string
- CodeInvalidFolderPath : string
- CodeTargetNotFound : string
- CodeAssetCreateFailed : string
- ValidActions : HashSet~string~
+ HandleCommand(params : JObject) object
- HandleCreate(params : JObject) object
- HandleModify(params : JObject) object
- ApplyPatches(target : Object, patches : JArray) (List~object~, List~string~)
- ApplyPatch(so : SerializedObject, propertyPath : string, op : string, patchObj : JObject, changed : bool) object
- ApplyArrayResize(so : SerializedObject, propertyPath : string, patchObj : JObject, changed : bool) object
- ApplySet(so : SerializedObject, propertyPath : string, patchObj : JObject, changed : bool) object
- TrySetValue(prop : SerializedProperty, valueToken : JToken, message : string) bool
- TrySetEnum(prop : SerializedProperty, valueToken : JToken, message : string) bool
- TryResolveTarget(targetToken : JToken, target : Object, targetPath : string, targetGuid : string, error : object) bool
- CoerceJsonStringArrayParameter(params : JObject, paramName : string) void
- EnsureFolderExists(folderPath : string, error : string) bool
- SanitizeSlashes(path : string) string
- TryNormalizeFolderPath(folderPath : string, normalized : string, error : string) bool
- TryGetInt(token : JToken, value : int) bool
- TryGetFloat(token : JToken, value : float) bool
- TryGetBool(token : JToken, value : bool) bool
- TryGetVector2(token : JToken, value : Vector2) bool
- TryGetVector3(token : JToken, value : Vector3) bool
- TryGetVector4(token : JToken, value : Vector4) bool
- TryGetColor(token : JToken, value : Color) bool
- NormalizeAction(raw : string) string
- IsCreateAction(normalized : string) bool
- ResolveType(typeName : string) Type
}
class ErrorResponse {
+ ErrorResponse(code : string)
+ ErrorResponse(code : string, details : object)
}
class SuccessResponse {
+ SuccessResponse(message : string, payload : object)
}
class manage_scriptable_object_py {
+ manage_scriptable_object(ctx : Context, action : string, type_name : string, folder_path : string, asset_name : string, overwrite : bool, target : dict~string, Any~, patches : list~dict~) dict~string, Any~
- parse_json_payload(payload : Any) Any
- coerce_bool(value : Any, default : bool) bool
}
class UnityTransportModule {
+ send_with_unity_instance(sender : function, unity_instance : Any, command : string, params : dict~string, Any~) Any
+ async_send_command_with_retry(unity_instance : Any, command : string, params : dict~string, Any~) Any
}
class ToolsRegistry {
+ mcp_for_unity_tool(description : string)
+ get_unity_instance_from_context(ctx : Context) Any
}
ManageScriptableObject ..> ErrorResponse : uses
ManageScriptableObject ..> SuccessResponse : uses
manage_scriptable_object_py ..> ManageScriptableObject : calls_via_transport
manage_scriptable_object_py ..> UnityTransportModule : uses
manage_scriptable_object_py ..> ToolsRegistry : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe PR removes ScriptableObject creation from ManageAsset, adds a dedicated ManageScriptableObject C# editor tool with create/modify/patch support, exposes a Python wrapper (manage_scriptable_object), adds integration and Unity editor tests/fixtures, updates tests to avoid MCPServiceLocator.Reset(), and adds Unity metadata and .gitignore entries. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as Python Server
participant Unity as Unity Editor
participant AssetDB as AssetDatabase
participant SO as SerializedObject
rect rgb(240,248,255)
note over Client,Server: Create ScriptableObject flow
Client->>Server: manage_scriptable_object(action="create", params)
Server->>Server: validate & coerce params
Server->>Unity: send_with_unity_instance("manage_scriptable_object", params)
Unity->>Unity: validate typeName, normalize folderPath, ensure folders
Unity->>Unity: resolve ScriptableObject type
Unity->>AssetDB: Create or overwrite asset (preserve GUID if needed)
AssetDB->>Unity: asset ready
Unity->>SO: create SerializedObject(asset)
loop patches
Unity->>SO: apply patch (set / array_resize)
SO-->>Unity: per-patch result
end
Unity->>AssetDB: SaveAssets()
Unity-->>Server: {success, guid, path, type, patch_results}
Server-->>Client: response
end
sequenceDiagram
participant Client
participant Server as Python Server
participant Unity as Unity Editor
participant SO as SerializedObject
rect rgb(245,245,220)
note over Client,Server: Modify ScriptableObject flow
Client->>Server: manage_scriptable_object(action="modify", target, patches)
Server->>Server: validate target & patches
Server->>Unity: send_with_unity_instance("manage_scriptable_object", params)
Unity->>Unity: resolve asset by guid/path
Unity->>SO: load SerializedObject(asset)
loop patches
Unity->>SO: apply patch to SerializedProperty
SO-->>Unity: per-patch result
end
Unity->>Unity: SaveAssets()
Unity-->>Server: {success, target_metadata, patch_results}
Server-->>Client: response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ 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)
🔇 Additional comments (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
MCPForUnity/Editor/Tools/ManageScriptableObject.cs (3)
247-299:warningslist is initialized but never populated.The
warningslist (line 249) is returned in the tuple but is never added to within this method. This meanspatchApply.warningsat line 195 andwarningsat line 242 will always be empty lists. Consider either:
- Removing the unused warnings infrastructure if not needed
- Adding warnings for non-critical issues (e.g., type coercion, deprecated property paths)
417-447: Consider warning when object reference resolution fails.When both
refGuidandrefPathare provided but the asset cannot be loaded (lines 430-437),newRefremains null and the reference is silently cleared. Consider adding a warning to inform callers that the reference wasn't found:if (!string.IsNullOrEmpty(resolvedPath)) { newRef = AssetDatabase.LoadAssetAtPath<UnityEngine.Object>(resolvedPath); + // If path resolved but asset not found, could add warning here }This is a minor UX improvement for debugging failed patches.
677-722: Consider adding path traversal validation.The path validation rejects absolute paths and restricted roots, but doesn't explicitly handle
..traversal. A malicious path like"Assets/../Packages/Malicious"would pass theStartsWith("Assets/")check but could escape the Assets directory.While Unity's
AssetDatabase.CreateFoldermay reject such paths, explicit validation would provide defense-in-depth:🔎 Suggested enhancement
private static bool TryNormalizeFolderPath(string folderPath, out string normalized, out string error) { // ... existing code ... + // Reject path traversal attempts + if (s.Contains("..")) + { + error = "Folder path must not contain path traversal sequences."; + return false; + } + if (s.StartsWith("Packages/", StringComparison.OrdinalIgnoreCase)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs (2)
221-256: Consider adding a Windows drive path test case.The test covers Unix-style absolute paths (
/tmp/...) andfile://URIs, but the PR objectives mention rejecting Windows drive/absolute paths. Adding a test case likeC:\Users\...orD:\Projects\...would ensure cross-platform validation coverage.🔎 Suggested additional test case
Assert.IsFalse(badFileUri.Value<bool>("success")); Assert.AreEqual("invalid_folder_path", badFileUri.Value<string>("error")); + + var badWindowsDrive = ToJObject(ManageScriptableObject.HandleCommand(new JObject + { + ["action"] = "create", + ["typeName"] = typeof(ManageScriptableObjectTestDefinition).FullName, + ["folderPath"] = @"C:\Users\Test", + ["assetName"] = "BadFolder4", + ["overwrite"] = true, + })); + Assert.IsFalse(badWindowsDrive.Value<bool>("success")); + Assert.AreEqual("invalid_folder_path", badWindowsDrive.Value<string>("error")); }
258-278: Minor: Consider tracking_createdAssetPathfor consistency.This test creates an asset but doesn't set
_createdAssetPath, unlike other tests. While cleanup still works because the asset is underTempRoot, tracking it explicitly would maintain consistency and ensure cleanup if the folder structure changes.🔎 Suggested fix
var res = ToJObject(ManageScriptableObject.HandleCommand(create)); Assert.IsTrue(res.Value<bool>("success"), res.ToString()); - var path = res["data"]?["path"]?.ToString(); + _createdAssetPath = res["data"]?["path"]?.ToString(); + var path = _createdAssetPath; Assert.IsNotNull(path, "Expected path in response.");
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.gitignoreMCPForUnity/Editor/Tools/ManageAsset.csMCPForUnity/Editor/Tools/ManageScriptableObject.csMCPForUnity/Editor/Tools/ManageScriptableObject.cs.metaServer/src/services/tools/manage_asset.pyServer/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 (6)
📓 Common learnings
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
.gitignoreServer/src/services/tools/manage_scriptable_object.py
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
.gitignore
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
Server/src/services/tools/manage_scriptable_object.py
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Tools/ManageScriptableObject.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/ManageScriptableObjectTestDefinitionBase.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/ManageScriptableObjectTestDefinition.cs
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
Applied to files:
TestProjects/UnityMCPTests/Assets/Packages.meta
🧬 Code graph analysis (6)
Server/src/services/tools/manage_scriptable_object.py (3)
Server/src/services/tools/utils.py (2)
coerce_bool(11-24)parse_json_payload(27-60)Server/src/transport/unity_transport.py (1)
send_with_unity_instance(78-103)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(753-785)
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)
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(247-299)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (1)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (2)
MCPServiceLocator(11-91)Register(40-62)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs (3)
MCPForUnity/Editor/Tools/BatchExecute.cs (1)
JArray(153-161)MCPForUnity/Editor/Tools/ManageScriptableObject.cs (1)
HandleCommand(42-77)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: Sourcery review
🔇 Additional comments (36)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (2)
35-35: LGTM: Clean capture of original service state.The field addition and OneTimeSetUp capture correctly preserve the platform service state before this fixture's tests potentially mock it. This enables targeted restoration without disrupting other services.
Also applies to: 44-44
59-68: Excellent targeted fix with clear documentation.The refactor from
MCPServiceLocator.Reset()to selective service restoration directly addresses the issue of disrupting active MCP sessions during test execution. The approach is sound:
- Surgical restoration: Only restores the platform service that this fixture mutates, avoiding disposal of the bridge/transport manager that would kill active MCP connections.
- Proper lifecycle: Restoration in
TearDown(notOneTimeTearDown) ensures each test gets a clean slate while preventing test pollution.- Clear rationale: Comments explicitly document why this pattern is needed, helping future maintainers.
The defensive null check at Line 65 is fine, though
MCPServiceLocator.Platformnever returns null (lazy-initializes via??= new PlatformService())..gitignore (1)
37-38: LGTM!Good addition to prevent test artifacts from being committed. The pattern correctly targets the specific test output directory.
TestProjects/UnityMCPTests/Assets/Temp.meta (1)
1-8: LGTM!Standard Unity folder meta file. Tracking the meta while ignoring the folder contents is the correct approach to preserve folder structure and GUID stability.
TestProjects/UnityMCPTests/Assets/Packages.meta (1)
1-8: LGTM!Standard Unity folder meta file for the Packages directory.
TestProjects/UnityMCPTests/ProjectSettings/SceneTemplateSettings.json (1)
1-121: LGTM!Standard Unity-generated scene template settings file with default dependency type configurations.
MCPForUnity/Editor/Tools/ManageAsset.cs (2)
251-253: LGTM!Error message correctly updated to reflect that ScriptableObject creation has been removed from this tool. The supported types list (Folder, Material, PhysicsMaterial) now accurately reflects the tool's capabilities.
423-429: LGTM!Good migration strategy: the modification path is preserved for backward compatibility with a clear deprecation note directing users to
manage_scriptable_object. This allows existing workflows to continue functioning while encouraging adoption of the more robust serialized property-based approach.Server/tests/integration/test_manage_scriptable_object_tool.py (2)
7-40: LGTM!Comprehensive test for the create action covering parameter forwarding, JSON string parsing for patches, and boolean coercion for overwrite. The
**kwargsin the mock is intentionally matching the real function signature which accepts optional keyword arguments likeinstance_idandloop.
43-70: LGTM!Good coverage for the modify action, testing both JSON string target parsing and native list patches forwarding.
Server/src/services/tools/manage_scriptable_object.py (2)
1-22: LGTM!Clear docstring documenting the Unity-side handler, command name, and supported actions. Imports are minimal and appropriate.
24-72: LGTM!Well-structured tool implementation:
- Proper use of
Annotatedtypes for clear parameter documentation- Tolerant JSON parsing handles LLM stringification of complex objects
- Type validation with user-friendly error messages
- Correct snake_case to camelCase mapping for Unity params
- None filtering keeps Unity handler cleaner
- Response validation with fallback error handling
Server/src/services/tools/manage_asset.py (1)
24-25: LGTM!Good documentation update directing users to
manage_scriptable_objectfor ScriptableObject operations. This provides clear guidance in the tool's API surface.MCPForUnity/Editor/Tools/ManageScriptableObject.cs (8)
1-41: LGTM - Well-structured class setup with clear error codes.The constant error codes and valid actions set are well-defined. The use of
StringComparer.OrdinalIgnoreCasefor the action set is appropriate.
42-77: LGTM - Clean command routing with appropriate state checks.The handling of Unity's compile/reload state with a retryable hint is good for MCP clients. The action normalization and routing logic is clear.
142-172: Well-implemented GUID preservation for overwrites.The logic correctly handles three scenarios:
- Existing asset of same type → CopySerialized preserves GUID
- Existing asset of different type → Delete and recreate
- No existing asset → Standard creation
The asset name restoration at line 157 is a good fix for Unity warnings.
214-245: LGTM - Clean modify flow with proper validation.
460-538: LGTM - Comprehensive type handling with proper error messages.The switch covers common Unity property types, and the enum handling with both index and name lookup is flexible.
540-583: LGTM - Target resolution with proper fallback handling.The GUID computation at line 581 ensures the response always includes the target GUID even when resolved by path.
846-876: LGTM - Color parsing with sensible alpha defaults.The alpha channel correctly defaults to 1.0 when not provided or when parsing fails, which matches Unity's expected behavior for opaque colors.
890-929: LGTM - Robust type resolution with multiple fallback strategies.The three-pass type resolution (Type.GetType → assembly scan → FullName scan) handles various edge cases, and the
ReflectionTypeLoadExceptionhandling at line 915 is good defensive coding for assemblies with load issues.MCPForUnity/Editor/Tools/ManageScriptableObject.cs.meta (1)
1-14: LGTM - Standard Unity meta file.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/ManageScriptableObjectTestDefinition.cs.meta (1)
1-11: LGTM - Standard Unity meta file for test fixture.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/ManageScriptableObjectTestDefinitionBase.cs.meta (1)
1-11: LGTM - Standard Unity meta file for base fixture.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures.meta (1)
1-8: LGTM - Standard Unity folder meta file.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs.meta (1)
1-11: LGTM - Standard Unity meta file for test class.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/ManageScriptableObjectTestDefinitionBase.cs (1)
1-14: LGTM - Clean base test fixture.Follows Unity serialization best practices with a private
[SerializeField]field and public read-only accessor. The comment about filename matching class name is helpful for maintainers.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/ManageScriptableObjectTestDefinition.cs (1)
1-27: LGTM - Well-designed test fixture for comprehensive testing.The fixture covers key serialization scenarios:
- Inheritance (via base class)
- Nested structs (
ManageScriptableObjectNestedData)- Object reference lists (
List<Material>)IReadOnlyList<T>accessor provides good encapsulationThis enables thorough testing of the
ApplyPatchesfunctionality inManageScriptableObject.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs (8)
1-21: LGTM!Clean setup with appropriate imports and well-organized constants and fields for tracking test artifacts.
22-48: LGTM!Good test isolation with unique material names via GUIDs, and the shader fallback chain appropriately handles different render pipelines.
50-84: LGTM!Thorough best-effort cleanup that handles individual assets and folder hierarchies, with the parent folder cleanup being a nice touch for test hygiene.
86-124: LGTM!Comprehensive happy-path test covering folder creation, path normalization, and patch application across private fields, inherited fields, and nested struct properties.
126-189: LGTM!Excellent coverage of modify workflows including array resize and object reference assignment via both GUID and path, with per-patch result verification for better failure diagnostics.
191-219: LGTM!Good negative test coverage verifying specific error codes for invalid action, missing type, and missing target scenarios.
280-301: LGTM!Clean recursive folder creation using
AssetDatabase.CreateFolder, properly leveragingAssetPathUtility.SanitizeAssetPathfor consistent path handling.
303-307: LGTM!Simple and effective utility for normalizing command results to
JObject.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The
warningslist returned fromApplyPatchesis never populated, so you can either remove it from the API surface or start adding concrete warning cases (e.g., best-effort fallbacks inApplyArrayResize) to make it useful. TryResolveTargetcurrently accepts anyUnityEngine.Object, which meansmanage_scriptable_objectcan operate on non-ScriptableObject assets; consider either constraining this toScriptableObjector explicitly documenting that broader behavior.- In
ManageScriptableObject.HandleCreateyou callAssetDatabase.SaveAssets()after patching, whileApplyPatchesalso saves assets whenanyChangedis true; you could centralize the save call in one place to avoid redundant saves.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `warnings` list returned from `ApplyPatches` is never populated, so you can either remove it from the API surface or start adding concrete warning cases (e.g., best-effort fallbacks in `ApplyArrayResize`) to make it useful.
- `TryResolveTarget` currently accepts any `UnityEngine.Object`, which means `manage_scriptable_object` can operate on non-ScriptableObject assets; consider either constraining this to `ScriptableObject` or explicitly documenting that broader behavior.
- In `ManageScriptableObject.HandleCreate` you call `AssetDatabase.SaveAssets()` after patching, while `ApplyPatches` also saves assets when `anyChanged` is true; you could centralize the save call in one place to avoid redundant saves.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Tools/ManageScriptableObject.cs:301-321` </location>
<code_context>
+ changed = false;
+ try
+ {
+ string normalizedOp = op.Trim().ToLowerInvariant();
+
+ switch (normalizedOp)
+ {
+ case "array_resize":
+ return ApplyArrayResize(so, propertyPath, patchObj, out changed);
+ case "set":
+ default:
+ return ApplySet(so, propertyPath, patchObj, out changed);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Unknown operations silently fall back to `set`, which can hide caller errors.
In `ApplyPatch`, any op other than `array_resize` is treated as `set`, so a typo like `"op": "arrary_resize"` will silently act as `set` instead of failing. Consider validating `normalizedOp` against the allowed operations and returning a structured error for unsupported values instead of defaulting to `set`.
```suggestion
private static object ApplyPatch(SerializedObject so, string propertyPath, string op, JObject patchObj, out bool changed)
{
changed = false;
try
{
string normalizedOp = (op ?? string.Empty).Trim().ToLowerInvariant();
switch (normalizedOp)
{
case "array_resize":
return ApplyArrayResize(so, propertyPath, patchObj, out changed);
case "set":
return ApplySet(so, propertyPath, patchObj, out changed);
default:
return new
{
propertyPath,
op,
ok = false,
message = $"Unsupported op '{op}'. Supported ops are: 'set', 'array_resize'."
};
}
}
catch (Exception ex)
{
return new { propertyPath, op, ok = false, message = ex.Message };
}
}
```
</issue_to_address>
### Comment 2
<location> `MCPForUnity/Editor/Tools/ManageScriptableObject.cs:279-280` </location>
<code_context>
+ op = "set";
+ }
+
+ var patchResult = ApplyPatch(so, propertyPath, op, patchObj, out bool changed);
+ anyChanged |= changed;
+ results.Add(patchResult);
+
</code_context>
<issue_to_address>
**suggestion (performance):** The `changed` flag is set to `ok` in `TrySetValue`, which can trigger saves even when values are unchanged.
In `ApplyPatches`, `anyChanged` is derived from `changed`, which is set in `ApplySet` as `changed = ok;`. Because `TrySetValue` returns `ok` on successful parsing rather than on actual value changes, writing the same value still sets `changed = true`, causing `ApplyModifiedProperties`, `SetDirty`, and `SaveAssets` to run unnecessarily. To avoid redundant serialization/saves, compare the existing and new values for each supported property type and only set `changed = true` when they differ.
Suggested implementation:
```csharp
if (string.IsNullOrWhiteSpace(op))
{
op = "set";
}
var patchResult = ApplyPatch(so, propertyPath, op, patchObj, out bool changed);
anyChanged |= changed;
results.Add(patchResult);
namespace MCPForUnity.Editor.Tools
```
```csharp
private static object ApplySet(SerializedObject so, string propertyPath, JObject patchObj, out bool changed)
{
changed = false;
var valueToken = patchObj["value"];
if (valueToken == null)
{
return new { propertyPath, op = "set", ok = false, message = "Missing required field: value" };
}
var property = so.FindProperty(propertyPath);
if (property == null)
{
return new { propertyPath, op = "set", ok = false, message = $"Property not found: {propertyPath}" };
}
var ok = TrySetValue(property, valueToken, out bool valueChanged, out string message);
changed = valueChanged;
if (ok)
{
property.serializedObject.ApplyModifiedProperties();
return new { propertyPath, op = "set", ok = true };
}
return new { propertyPath, op = "set", ok = false, message };
}
```
```csharp
private static bool TrySetValue(SerializedProperty property, JToken valueToken, out bool changed, out string message)
{
message = null;
changed = false;
try
{
switch (property.propertyType)
{
case SerializedPropertyType.Integer:
{
int newValue = valueToken.Value<int>();
int oldValue = property.intValue;
if (oldValue == newValue)
return true;
property.intValue = newValue;
changed = true;
break;
}
case SerializedPropertyType.Boolean:
{
bool newValue = valueToken.Value<bool>();
bool oldValue = property.boolValue;
if (oldValue == newValue)
return true;
property.boolValue = newValue;
changed = true;
break;
}
case SerializedPropertyType.Float:
{
float newValue = valueToken.Value<float>();
float oldValue = property.floatValue;
if (Mathf.Approximately(oldValue, newValue))
return true;
property.floatValue = newValue;
changed = true;
break;
}
case SerializedPropertyType.String:
{
string newValue = valueToken.Value<string>();
string oldValue = property.stringValue;
if (string.Equals(oldValue, newValue, StringComparison.Ordinal))
return true;
property.stringValue = newValue;
changed = true;
break;
}
case SerializedPropertyType.ObjectReference:
{
// Delegate to existing logic; it should set the reference only when changed if possible.
// If not, this still avoids marking changes for failed parses.
bool ok = TrySetObjectReference(property, valueToken, out bool refChanged, out message);
changed = refChanged;
return ok;
}
case SerializedPropertyType.Enum:
{
// Delegate to existing enum logic and propagate the changed flag.
bool ok = TrySetEnum(property, valueToken, out bool enumChanged, out message);
changed = enumChanged;
return ok;
}
// Add similar comparison logic for other supported property types as needed.
default:
message = $"Unsupported property type: {property.propertyType}";
return false;
}
return true;
}
catch (Exception ex)
{
message = $"Failed to set value: {ex.Message}";
changed = false;
return false;
}
}
```
Because only a portion of the file is visible, you will also need to:
1. Update any existing calls to `TrySetValue` (other than the one in `ApplySet` already patched above) to use the new signature:
- From: `TrySetValue(property, valueToken, out string message)`
- To: `TrySetValue(property, valueToken, out bool changed, out string message)` (and either use or discard `changed` as appropriate).
2. Adjust the helper methods for complex types to match the new pattern:
- Change `TrySetObjectReference` to:
`private static bool TrySetObjectReference(SerializedProperty property, JToken valueToken, out bool changed, out string message)`
and implement it so `changed` is `true` only when the reference actually changes.
- Change `TrySetEnum` to:
`private static bool TrySetEnum(SerializedProperty property, JToken valueToken, out bool changed, out string message)`
and set `changed` only when the enum value differs from the current one.
3. If you support additional property types (e.g., `Vector2`, `Vector3`, `Color`, arrays, etc.), mirror the same “compare then assign” pattern inside the `switch (property.propertyType)` so `changed` only becomes `true` when the underlying value is different.
4. Ensure `using UnityEngine;` and `using System;` are present at the top of the file so `Mathf` and `StringComparison` compile correctly.
</issue_to_address>
### Comment 3
<location> `TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs:87` </location>
<code_context>
+ }
+
+ [Test]
+ public void Create_CreatesNestedFolders_PlacesAssetCorrectly_AndAppliesPatches()
+ {
+ var create = new JObject
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test that asserts GUID-preserving overwrite semantics for `overwrite=true`.
The `ManageScriptableObject` logic is supposed to overwrite an existing asset in-place (preserving its GUID) when `overwrite` is true and the existing type matches. Current tests cover folder creation and patching but not this GUID behavior. Please add or extend a test that:
1. Creates an asset at a known path.
2. Records its GUID.
3. Calls `create` again with the same `folderPath`/`assetName` and `overwrite=true`.
4. Asserts the path is unchanged and `AssetDatabase.AssetPathToGUID(path)` matches the original GUID.
This will exercise the GUID-preserving branch and guard against regressions in the overwrite flow.
Suggested implementation:
```csharp
AssetDatabase.Refresh();
}
[Test]
public void Create_WithOverwriteTrue_PreservesGuid_WhenExistingAssetTypeMatches()
{
// Arrange: ensure the nested folder exists
var folderSegments = NestedFolder.Split('/');
var currentPath = "Assets";
for (var i = 1; i < folderSegments.Length; i++)
{
var nextPath = $"{currentPath}/{folderSegments[i]}";
if (!AssetDatabase.IsValidFolder(nextPath))
{
AssetDatabase.CreateFolder(currentPath, folderSegments[i]);
}
currentPath = nextPath;
}
var assetPath = $"{NestedFolder}/My_Test_Def.asset";
// Create an initial asset at the target path
var originalAsset = ScriptableObject.CreateInstance<ManageScriptableObjectTestDefinition>();
originalAsset.displayName = "Original";
originalAsset.baseNumber = 1;
AssetDatabase.CreateAsset(originalAsset, assetPath);
AssetDatabase.SaveAssets();
AssetDatabase.Refresh();
var originalGuid = AssetDatabase.AssetPathToGUID(assetPath);
Assert.IsFalse(string.IsNullOrEmpty(originalGuid), "Expected initial asset to have a valid GUID.");
// Act: run the create command again with overwrite=true for the same path
var create = new JObject
{
["action"] = "create",
["typeName"] = typeof(ManageScriptableObjectTestDefinition).FullName,
["folderPath"] = NestedFolder,
["assetName"] = "My_Test_Def",
["overwrite"] = true,
["patches"] = new JArray
{
new JObject { ["propertyPath"] = "displayName", ["op"] = "set", ["value"] = "Overwritten" },
new JObject { ["propertyPath"] = "baseNumber", ["op"] = "set", ["value"] = 99 },
},
};
// This should hit the overwrite-in-place branch and preserve the existing GUID
ManageScriptableObject.Execute(create);
AssetDatabase.SaveAssets();
AssetDatabase.Refresh();
// Assert: same path, same GUID, updated data
var newGuid = AssetDatabase.AssetPathToGUID(assetPath);
Assert.AreEqual(originalGuid, newGuid, "GUID should be preserved when overwriting an existing asset of the same type.");
Assert.AreEqual(assetPath, AssetDatabase.GUIDToAssetPath(originalGuid), "Asset path should remain unchanged.");
var overwrittenAsset =
AssetDatabase.LoadAssetAtPath<ManageScriptableObjectTestDefinition>(assetPath);
Assert.IsNotNull(overwrittenAsset, "Overwritten asset should still be loadable at the same path.");
Assert.AreEqual("Overwritten", overwrittenAsset.displayName);
Assert.AreEqual(99, overwrittenAsset.baseNumber);
}
[Test]
public void Create_CreatesNestedFolders_PlacesAssetCorrectly_AndAppliesPatches()
```
1. The invocation `ManageScriptableObject.Execute(create);` should be updated to match whatever entry point the existing tests use to run the `ManageScriptableObject` logic (for example, `ManageScriptableObject.Run(create)`, `ManageScriptableObject.Handle(create)`, or similar). Use the same call signature and parameter shape as in the other tests in this fixture.
2. If `NestedFolder` already includes `"Assets"` or has a different format, adjust the folder creation logic accordingly so it does not attempt to create duplicate root segments.
3. Ensure the necessary `using` directives are present at the top of the file (they likely already are, but this test relies on `ScriptableObject`, `AssetDatabase`, `JObject`, and `JArray`).
</issue_to_address>
### Comment 4
<location> `TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs:127` </location>
<code_context>
+ }
+
+ [Test]
+ public void Modify_ArrayResize_ThenAssignObjectRefs_ByGuidAndByPath()
+ {
+ // Create base asset first with no patches.
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for clearing an object reference to null via patches.
`ApplySet` is designed to clear `ObjectReference` properties when `ref` is omitted and `value` is `null`, but this path isn’t currently tested. Please extend this test (or add a new one) to apply a patch like `{"propertyPath": "materials.Array.data[0]", "op": "set", "value": null}` after the initial assignments, and assert that `asset.Materials[0]` becomes `null` while `asset.Materials[1]` is unchanged. This will exercise the null-clearing branch.
Suggested implementation:
```csharp
var create = new JObject
{
["action"] = "create",
["typeName"] = typeof(ManageScriptableObjectTestDefinition).FullName,
["folderPath"] = TempRoot,
["assetName"] = "Modify_Target",
["overwrite"] = true
};
var createRes = ToJObject(ManageScriptableObject.HandleCommand(create));
Assert.IsTrue(createRes.Value<bool>("success"), createRes.ToString());
var createdAssetPath = createRes.Value<string>("assetPath");
Assert.IsFalse(string.IsNullOrWhiteSpace(createdAssetPath), "Expected assetPath in create response.");
// Create two material assets that we'll assign by guid and by path.
var material1Path = System.IO.Path.Combine(TempRoot, "Modify_ArrayResize_Material_1.mat").Replace('\\', '/');
var material2Path = System.IO.Path.Combine(TempRoot, "Modify_ArrayResize_Material_2.mat").Replace('\\', '/');
var shader = Shader.Find("Universal Render Pipeline/Lit") ?? Shader.Find("Standard");
Assert.IsNotNull(shader, "Expected to find at least one default shader for test materials.");
var material1 = new Material(shader);
var material2 = new Material(shader);
AssetDatabase.CreateAsset(material1, material1Path);
AssetDatabase.CreateAsset(material2, material2Path);
AssetDatabase.SaveAssets();
AssetDatabase.Refresh();
var material1Guid = AssetDatabase.AssetPathToGUID(material1Path);
Assert.IsFalse(string.IsNullOrWhiteSpace(material1Guid), "Expected GUID for first material.");
// First modify: resize materials array to 2, then assign by guid and by path.
var modifyInitial = new JObject
{
["action"] = "modify",
["assetPath"] = createdAssetPath,
["patches"] = new JArray
{
// Resize array
new JObject
{
["op"] = "set",
["propertyPath"] = "materials.Array.size",
["value"] = 2
},
// Assign element 0 by guid
new JObject
{
["op"] = "set",
["propertyPath"] = "materials.Array.data[0]",
["ref"] = new JObject
{
["guid"] = material1Guid
}
},
// Assign element 1 by path
new JObject
{
["op"] = "set",
["propertyPath"] = "materials.Array.data[1]",
["ref"] = new JObject
{
["assetPath"] = material2Path
}
}
}
};
var modifyInitialRes = ToJObject(ManageScriptableObject.HandleCommand(modifyInitial));
Assert.IsTrue(modifyInitialRes.Value<bool>("success"), modifyInitialRes.ToString());
// Verify both material references were applied.
var asset = AssetDatabase.LoadAssetAtPath<ManageScriptableObjectTestDefinition>(createdAssetPath);
Assert.IsNotNull(asset, "Modified asset should load as TestDefinition after initial modify.");
Assert.IsNotNull(asset!.Materials, "Materials array should not be null after resize.");
Assert.AreEqual(2, asset.Materials.Length, "Materials array size should be 2 after resize.");
Assert.IsNotNull(asset.Materials[0], "Materials[0] should be assigned by guid after initial modify.");
Assert.IsNotNull(asset.Materials[1], "Materials[1] should be assigned by path after initial modify.");
// Second modify: clear the first material reference by setting value:null and omitting ref.
var modifyClearRef = new JObject
{
["action"] = "modify",
["assetPath"] = createdAssetPath,
["patches"] = new JArray
{
new JObject
{
["op"] = "set",
["propertyPath"] = "materials.Array.data[0]",
// ApplySet should interpret value:null with no "ref" as "clear the ObjectReference".
["value"] = JValue.CreateNull()
}
}
};
var modifyClearRefRes = ToJObject(ManageScriptableObject.HandleCommand(modifyClearRef));
Assert.IsTrue(modifyClearRefRes.Value<bool>("success"), modifyClearRefRes.ToString());
// Reload and verify that only the first reference was cleared.
asset = AssetDatabase.LoadAssetAtPath<ManageScriptableObjectTestDefinition>(createdAssetPath);
Assert.IsNotNull(asset, "Modified asset should still be loadable after clearing reference.");
Assert.IsNotNull(asset!.Materials, "Materials array should still exist after clearing reference.");
Assert.AreEqual(2, asset.Materials.Length, "Materials array size should remain 2 after clearing reference.");
Assert.IsNull(asset.Materials[0], "Materials[0] should be cleared (null) when value:null and no ref are applied.");
Assert.IsNotNull(asset.Materials[1], "Materials[1] should remain unchanged when only Materials[0] is cleared.");
```
This edit assumes that:
1. `ManageScriptableObjectTestDefinition` has a `public Material[] Materials` (or similarly named) field/prop serialized under the path `"materials"`. If the backing field/property uses a different name, update `"materials.Array.*"` and `asset.Materials` accordingly.
2. `ToJObject`, `TempRoot`, and `ManageScriptableObject.HandleCommand` are already defined and used consistently elsewhere in the test file.
3. The original test body did not contain additional logic after the `Assert.IsTrue(createRes...)` line. If it did, you should merge any existing modify/verify logic with the new two-phase modify flow above (e.g., incorporate existing patches into `modifyInitial`).
</issue_to_address>
### Comment 5
<location> `Server/tests/integration/test_manage_scriptable_object_tool.py:7` </location>
<code_context>
+import services.tools.manage_scriptable_object as mod
+
+
+def test_manage_scriptable_object_forwards_create_params(monkeypatch):
+ captured = {}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add integration tests for invalid `target`/`patches` inputs in the Python wrapper.
Since the wrapper validates input via `parse_json_payload`, consider adding tests that cover malformed payloads:
- Non-JSON `target` string → expect `{"success": False, "message": ...}` describing the expected shape.
- Non-list `patches` (e.g., scalar or dict) → expect the "patches must be a list" error.
These will ensure the defensive behavior is exercised and preserved.
Suggested implementation:
```python
def test_manage_scriptable_object_forwards_create_params(monkeypatch):
captured = {}
async def fake_async_send(cmd, params, **kwargs):
captured["cmd"] = cmd
captured["params"] = params
def test_manage_scriptable_object_rejects_non_json_target(monkeypatch):
ctx = DummyContext()
async def noop_async_send(*_, **__):
return None
# Avoid any real side effects when the tool tries to send commands
monkeypatch.setattr(ctx, "async_send", noop_async_send)
result = asyncio.run(
mod.manage_scriptable_object(
ctx,
target="not valid json", # malformed JSON string
patches=[], # structurally valid for this test
)
)
assert isinstance(result, dict)
assert result.get("success") is False
# Message should explain the issue with the target / JSON shape
message = result.get("message", "")
assert message
assert "target" in message or "json" in message.lower()
def test_manage_scriptable_object_rejects_non_list_patches(monkeypatch):
ctx = DummyContext()
async def noop_async_send(*_, **__):
return None
monkeypatch.setattr(ctx, "async_send", noop_async_send)
# Valid JSON target, but invalid patches payloads
for invalid_patches in ({}, 123, "not-a-list", None):
result = asyncio.run(
mod.manage_scriptable_object(
ctx,
target="{}", # minimally valid JSON object
patches=invalid_patches,
)
)
assert isinstance(result, dict)
assert result.get("success") is False
# Message should reflect that patches must be a list
message = result.get("message", "")
assert message
assert "list" in message.lower()
```
These tests assume:
1. `manage_scriptable_object` is an `async` function taking `(context, target, patches)` and returning a `dict` with `success` and `message` keys. If the actual wrapper signature or return shape differs, update the calls and assertions accordingly.
2. The error message for malformed `target` mentions either "target" or "JSON", and for invalid `patches` includes "list". If your concrete error messages are more specific (e.g. `"patches must be a list"`), adjust the string checks to match them exactly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+301
to
+321
| private static object ApplyPatch(SerializedObject so, string propertyPath, string op, JObject patchObj, out bool changed) | ||
| { | ||
| changed = false; | ||
| try | ||
| { | ||
| string normalizedOp = op.Trim().ToLowerInvariant(); | ||
|
|
||
| switch (normalizedOp) | ||
| { | ||
| case "array_resize": | ||
| return ApplyArrayResize(so, propertyPath, patchObj, out changed); | ||
| case "set": | ||
| default: | ||
| return ApplySet(so, propertyPath, patchObj, out changed); | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| return new { propertyPath, op, ok = false, message = ex.Message }; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Unknown operations silently fall back to set, which can hide caller errors.
In ApplyPatch, any op other than array_resize is treated as set, so a typo like "op": "arrary_resize" will silently act as set instead of failing. Consider validating normalizedOp against the allowed operations and returning a structured error for unsupported values instead of defaulting to set.
Suggested change
| private static object ApplyPatch(SerializedObject so, string propertyPath, string op, JObject patchObj, out bool changed) | |
| { | |
| changed = false; | |
| try | |
| { | |
| string normalizedOp = op.Trim().ToLowerInvariant(); | |
| switch (normalizedOp) | |
| { | |
| case "array_resize": | |
| return ApplyArrayResize(so, propertyPath, patchObj, out changed); | |
| case "set": | |
| default: | |
| return ApplySet(so, propertyPath, patchObj, out changed); | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| return new { propertyPath, op, ok = false, message = ex.Message }; | |
| } | |
| } | |
| private static object ApplyPatch(SerializedObject so, string propertyPath, string op, JObject patchObj, out bool changed) | |
| { | |
| changed = false; | |
| try | |
| { | |
| string normalizedOp = (op ?? string.Empty).Trim().ToLowerInvariant(); | |
| switch (normalizedOp) | |
| { | |
| case "array_resize": | |
| return ApplyArrayResize(so, propertyPath, patchObj, out changed); | |
| case "set": | |
| return ApplySet(so, propertyPath, patchObj, out changed); | |
| default: | |
| return new | |
| { | |
| propertyPath, | |
| op, | |
| ok = false, | |
| message = $"Unsupported op '{op}'. Supported ops are: 'set', 'array_resize'." | |
| }; | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| return new { propertyPath, op, ok = false, message = ex.Message }; | |
| } | |
| } |
Comment on lines
+279
to
+280
| var patchResult = ApplyPatch(so, propertyPath, op, patchObj, out bool changed); | ||
| anyChanged |= changed; |
Contributor
There was a problem hiding this comment.
suggestion (performance): The changed flag is set to ok in TrySetValue, which can trigger saves even when values are unchanged.
In ApplyPatches, anyChanged is derived from changed, which is set in ApplySet as changed = ok;. Because TrySetValue returns ok on successful parsing rather than on actual value changes, writing the same value still sets changed = true, causing ApplyModifiedProperties, SetDirty, and SaveAssets to run unnecessarily. To avoid redundant serialization/saves, compare the existing and new values for each supported property type and only set changed = true when they differ.
Suggested implementation:
if (string.IsNullOrWhiteSpace(op))
{
op = "set";
}
var patchResult = ApplyPatch(so, propertyPath, op, patchObj, out bool changed);
anyChanged |= changed;
results.Add(patchResult);
namespace MCPForUnity.Editor.Tools private static object ApplySet(SerializedObject so, string propertyPath, JObject patchObj, out bool changed)
{
changed = false;
var valueToken = patchObj["value"];
if (valueToken == null)
{
return new { propertyPath, op = "set", ok = false, message = "Missing required field: value" };
}
var property = so.FindProperty(propertyPath);
if (property == null)
{
return new { propertyPath, op = "set", ok = false, message = $"Property not found: {propertyPath}" };
}
var ok = TrySetValue(property, valueToken, out bool valueChanged, out string message);
changed = valueChanged;
if (ok)
{
property.serializedObject.ApplyModifiedProperties();
return new { propertyPath, op = "set", ok = true };
}
return new { propertyPath, op = "set", ok = false, message };
} private static bool TrySetValue(SerializedProperty property, JToken valueToken, out bool changed, out string message)
{
message = null;
changed = false;
try
{
switch (property.propertyType)
{
case SerializedPropertyType.Integer:
{
int newValue = valueToken.Value<int>();
int oldValue = property.intValue;
if (oldValue == newValue)
return true;
property.intValue = newValue;
changed = true;
break;
}
case SerializedPropertyType.Boolean:
{
bool newValue = valueToken.Value<bool>();
bool oldValue = property.boolValue;
if (oldValue == newValue)
return true;
property.boolValue = newValue;
changed = true;
break;
}
case SerializedPropertyType.Float:
{
float newValue = valueToken.Value<float>();
float oldValue = property.floatValue;
if (Mathf.Approximately(oldValue, newValue))
return true;
property.floatValue = newValue;
changed = true;
break;
}
case SerializedPropertyType.String:
{
string newValue = valueToken.Value<string>();
string oldValue = property.stringValue;
if (string.Equals(oldValue, newValue, StringComparison.Ordinal))
return true;
property.stringValue = newValue;
changed = true;
break;
}
case SerializedPropertyType.ObjectReference:
{
// Delegate to existing logic; it should set the reference only when changed if possible.
// If not, this still avoids marking changes for failed parses.
bool ok = TrySetObjectReference(property, valueToken, out bool refChanged, out message);
changed = refChanged;
return ok;
}
case SerializedPropertyType.Enum:
{
// Delegate to existing enum logic and propagate the changed flag.
bool ok = TrySetEnum(property, valueToken, out bool enumChanged, out message);
changed = enumChanged;
return ok;
}
// Add similar comparison logic for other supported property types as needed.
default:
message = $"Unsupported property type: {property.propertyType}";
return false;
}
return true;
}
catch (Exception ex)
{
message = $"Failed to set value: {ex.Message}";
changed = false;
return false;
}
}Because only a portion of the file is visible, you will also need to:
- Update any existing calls to
TrySetValue(other than the one inApplySetalready patched above) to use the new signature:- From:
TrySetValue(property, valueToken, out string message) - To:
TrySetValue(property, valueToken, out bool changed, out string message)(and either use or discardchangedas appropriate).
- From:
- Adjust the helper methods for complex types to match the new pattern:
- Change
TrySetObjectReferenceto:
private static bool TrySetObjectReference(SerializedProperty property, JToken valueToken, out bool changed, out string message)
and implement it sochangedistrueonly when the reference actually changes. - Change
TrySetEnumto:
private static bool TrySetEnum(SerializedProperty property, JToken valueToken, out bool changed, out string message)
and setchangedonly when the enum value differs from the current one.
- Change
- If you support additional property types (e.g.,
Vector2,Vector3,Color, arrays, etc.), mirror the same “compare then assign” pattern inside theswitch (property.propertyType)sochangedonly becomestruewhen the underlying value is different. - Ensure
using UnityEngine;andusing System;are present at the top of the file soMathfandStringComparisoncompile correctly.
| } | ||
|
|
||
| [Test] | ||
| public void Create_CreatesNestedFolders_PlacesAssetCorrectly_AndAppliesPatches() |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider adding a test that asserts GUID-preserving overwrite semantics for overwrite=true.
The ManageScriptableObject logic is supposed to overwrite an existing asset in-place (preserving its GUID) when overwrite is true and the existing type matches. Current tests cover folder creation and patching but not this GUID behavior. Please add or extend a test that:
- Creates an asset at a known path.
- Records its GUID.
- Calls
createagain with the samefolderPath/assetNameandoverwrite=true. - Asserts the path is unchanged and
AssetDatabase.AssetPathToGUID(path)matches the original GUID.
This will exercise the GUID-preserving branch and guard against regressions in the overwrite flow.
Suggested implementation:
AssetDatabase.Refresh();
}
[Test]
public void Create_WithOverwriteTrue_PreservesGuid_WhenExistingAssetTypeMatches()
{
// Arrange: ensure the nested folder exists
var folderSegments = NestedFolder.Split('/');
var currentPath = "Assets";
for (var i = 1; i < folderSegments.Length; i++)
{
var nextPath = $"{currentPath}/{folderSegments[i]}";
if (!AssetDatabase.IsValidFolder(nextPath))
{
AssetDatabase.CreateFolder(currentPath, folderSegments[i]);
}
currentPath = nextPath;
}
var assetPath = $"{NestedFolder}/My_Test_Def.asset";
// Create an initial asset at the target path
var originalAsset = ScriptableObject.CreateInstance<ManageScriptableObjectTestDefinition>();
originalAsset.displayName = "Original";
originalAsset.baseNumber = 1;
AssetDatabase.CreateAsset(originalAsset, assetPath);
AssetDatabase.SaveAssets();
AssetDatabase.Refresh();
var originalGuid = AssetDatabase.AssetPathToGUID(assetPath);
Assert.IsFalse(string.IsNullOrEmpty(originalGuid), "Expected initial asset to have a valid GUID.");
// Act: run the create command again with overwrite=true for the same path
var create = new JObject
{
["action"] = "create",
["typeName"] = typeof(ManageScriptableObjectTestDefinition).FullName,
["folderPath"] = NestedFolder,
["assetName"] = "My_Test_Def",
["overwrite"] = true,
["patches"] = new JArray
{
new JObject { ["propertyPath"] = "displayName", ["op"] = "set", ["value"] = "Overwritten" },
new JObject { ["propertyPath"] = "baseNumber", ["op"] = "set", ["value"] = 99 },
},
};
// This should hit the overwrite-in-place branch and preserve the existing GUID
ManageScriptableObject.Execute(create);
AssetDatabase.SaveAssets();
AssetDatabase.Refresh();
// Assert: same path, same GUID, updated data
var newGuid = AssetDatabase.AssetPathToGUID(assetPath);
Assert.AreEqual(originalGuid, newGuid, "GUID should be preserved when overwriting an existing asset of the same type.");
Assert.AreEqual(assetPath, AssetDatabase.GUIDToAssetPath(originalGuid), "Asset path should remain unchanged.");
var overwrittenAsset =
AssetDatabase.LoadAssetAtPath<ManageScriptableObjectTestDefinition>(assetPath);
Assert.IsNotNull(overwrittenAsset, "Overwritten asset should still be loadable at the same path.");
Assert.AreEqual("Overwritten", overwrittenAsset.displayName);
Assert.AreEqual(99, overwrittenAsset.baseNumber);
}
[Test]
public void Create_CreatesNestedFolders_PlacesAssetCorrectly_AndAppliesPatches()- The invocation
ManageScriptableObject.Execute(create);should be updated to match whatever entry point the existing tests use to run theManageScriptableObjectlogic (for example,ManageScriptableObject.Run(create),ManageScriptableObject.Handle(create), or similar). Use the same call signature and parameter shape as in the other tests in this fixture. - If
NestedFolderalready includes"Assets"or has a different format, adjust the folder creation logic accordingly so it does not attempt to create duplicate root segments. - Ensure the necessary
usingdirectives are present at the top of the file (they likely already are, but this test relies onScriptableObject,AssetDatabase,JObject, andJArray).
| } | ||
|
|
||
| [Test] | ||
| public void Modify_ArrayResize_ThenAssignObjectRefs_ByGuidAndByPath() |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add coverage for clearing an object reference to null via patches.
ApplySet is designed to clear ObjectReference properties when ref is omitted and value is null, but this path isn’t currently tested. Please extend this test (or add a new one) to apply a patch like {"propertyPath": "materials.Array.data[0]", "op": "set", "value": null} after the initial assignments, and assert that asset.Materials[0] becomes null while asset.Materials[1] is unchanged. This will exercise the null-clearing branch.
Suggested implementation:
var create = new JObject
{
["action"] = "create",
["typeName"] = typeof(ManageScriptableObjectTestDefinition).FullName,
["folderPath"] = TempRoot,
["assetName"] = "Modify_Target",
["overwrite"] = true
};
var createRes = ToJObject(ManageScriptableObject.HandleCommand(create));
Assert.IsTrue(createRes.Value<bool>("success"), createRes.ToString());
var createdAssetPath = createRes.Value<string>("assetPath");
Assert.IsFalse(string.IsNullOrWhiteSpace(createdAssetPath), "Expected assetPath in create response.");
// Create two material assets that we'll assign by guid and by path.
var material1Path = System.IO.Path.Combine(TempRoot, "Modify_ArrayResize_Material_1.mat").Replace('\\', '/');
var material2Path = System.IO.Path.Combine(TempRoot, "Modify_ArrayResize_Material_2.mat").Replace('\\', '/');
var shader = Shader.Find("Universal Render Pipeline/Lit") ?? Shader.Find("Standard");
Assert.IsNotNull(shader, "Expected to find at least one default shader for test materials.");
var material1 = new Material(shader);
var material2 = new Material(shader);
AssetDatabase.CreateAsset(material1, material1Path);
AssetDatabase.CreateAsset(material2, material2Path);
AssetDatabase.SaveAssets();
AssetDatabase.Refresh();
var material1Guid = AssetDatabase.AssetPathToGUID(material1Path);
Assert.IsFalse(string.IsNullOrWhiteSpace(material1Guid), "Expected GUID for first material.");
// First modify: resize materials array to 2, then assign by guid and by path.
var modifyInitial = new JObject
{
["action"] = "modify",
["assetPath"] = createdAssetPath,
["patches"] = new JArray
{
// Resize array
new JObject
{
["op"] = "set",
["propertyPath"] = "materials.Array.size",
["value"] = 2
},
// Assign element 0 by guid
new JObject
{
["op"] = "set",
["propertyPath"] = "materials.Array.data[0]",
["ref"] = new JObject
{
["guid"] = material1Guid
}
},
// Assign element 1 by path
new JObject
{
["op"] = "set",
["propertyPath"] = "materials.Array.data[1]",
["ref"] = new JObject
{
["assetPath"] = material2Path
}
}
}
};
var modifyInitialRes = ToJObject(ManageScriptableObject.HandleCommand(modifyInitial));
Assert.IsTrue(modifyInitialRes.Value<bool>("success"), modifyInitialRes.ToString());
// Verify both material references were applied.
var asset = AssetDatabase.LoadAssetAtPath<ManageScriptableObjectTestDefinition>(createdAssetPath);
Assert.IsNotNull(asset, "Modified asset should load as TestDefinition after initial modify.");
Assert.IsNotNull(asset!.Materials, "Materials array should not be null after resize.");
Assert.AreEqual(2, asset.Materials.Length, "Materials array size should be 2 after resize.");
Assert.IsNotNull(asset.Materials[0], "Materials[0] should be assigned by guid after initial modify.");
Assert.IsNotNull(asset.Materials[1], "Materials[1] should be assigned by path after initial modify.");
// Second modify: clear the first material reference by setting value:null and omitting ref.
var modifyClearRef = new JObject
{
["action"] = "modify",
["assetPath"] = createdAssetPath,
["patches"] = new JArray
{
new JObject
{
["op"] = "set",
["propertyPath"] = "materials.Array.data[0]",
// ApplySet should interpret value:null with no "ref" as "clear the ObjectReference".
["value"] = JValue.CreateNull()
}
}
};
var modifyClearRefRes = ToJObject(ManageScriptableObject.HandleCommand(modifyClearRef));
Assert.IsTrue(modifyClearRefRes.Value<bool>("success"), modifyClearRefRes.ToString());
// Reload and verify that only the first reference was cleared.
asset = AssetDatabase.LoadAssetAtPath<ManageScriptableObjectTestDefinition>(createdAssetPath);
Assert.IsNotNull(asset, "Modified asset should still be loadable after clearing reference.");
Assert.IsNotNull(asset!.Materials, "Materials array should still exist after clearing reference.");
Assert.AreEqual(2, asset.Materials.Length, "Materials array size should remain 2 after clearing reference.");
Assert.IsNull(asset.Materials[0], "Materials[0] should be cleared (null) when value:null and no ref are applied.");
Assert.IsNotNull(asset.Materials[1], "Materials[1] should remain unchanged when only Materials[0] is cleared.");This edit assumes that:
ManageScriptableObjectTestDefinitionhas apublic Material[] Materials(or similarly named) field/prop serialized under the path"materials". If the backing field/property uses a different name, update"materials.Array.*"andasset.Materialsaccordingly.ToJObject,TempRoot, andManageScriptableObject.HandleCommandare already defined and used consistently elsewhere in the test file.- The original test body did not contain additional logic after the
Assert.IsTrue(createRes...)line. If it did, you should merge any existing modify/verify logic with the new two-phase modify flow above (e.g., incorporate existing patches intomodifyInitial).
| import services.tools.manage_scriptable_object as mod | ||
|
|
||
|
|
||
| def test_manage_scriptable_object_forwards_create_params(monkeypatch): |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add integration tests for invalid target/patches inputs in the Python wrapper.
Since the wrapper validates input via parse_json_payload, consider adding tests that cover malformed payloads:
- Non-JSON
targetstring → expect{"success": False, "message": ...}describing the expected shape. - Non-list
patches(e.g., scalar or dict) → expect the "patches must be a list" error.
These will ensure the defensive behavior is exercised and preserved.
Suggested implementation:
def test_manage_scriptable_object_forwards_create_params(monkeypatch):
captured = {}
async def fake_async_send(cmd, params, **kwargs):
captured["cmd"] = cmd
captured["params"] = params
def test_manage_scriptable_object_rejects_non_json_target(monkeypatch):
ctx = DummyContext()
async def noop_async_send(*_, **__):
return None
# Avoid any real side effects when the tool tries to send commands
monkeypatch.setattr(ctx, "async_send", noop_async_send)
result = asyncio.run(
mod.manage_scriptable_object(
ctx,
target="not valid json", # malformed JSON string
patches=[], # structurally valid for this test
)
)
assert isinstance(result, dict)
assert result.get("success") is False
# Message should explain the issue with the target / JSON shape
message = result.get("message", "")
assert message
assert "target" in message or "json" in message.lower()
def test_manage_scriptable_object_rejects_non_list_patches(monkeypatch):
ctx = DummyContext()
async def noop_async_send(*_, **__):
return None
monkeypatch.setattr(ctx, "async_send", noop_async_send)
# Valid JSON target, but invalid patches payloads
for invalid_patches in ({}, 123, "not-a-list", None):
result = asyncio.run(
mod.manage_scriptable_object(
ctx,
target="{}", # minimally valid JSON object
patches=invalid_patches,
)
)
assert isinstance(result, dict)
assert result.get("success") is False
# Message should reflect that patches must be a list
message = result.get("message", "")
assert message
assert "list" in message.lower()These tests assume:
manage_scriptable_objectis anasyncfunction taking(context, target, patches)and returning adictwithsuccessandmessagekeys. If the actual wrapper signature or return shape differs, update the calls and assertions accordingly.- The error message for malformed
targetmentions either "target" or "JSON", and for invalidpatchesincludes "list". If your concrete error messages are more specific (e.g."patches must be a list"), adjust the string checks to match them exactly.
This was referenced Jan 5, 2026
This was referenced Jan 20, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Before, MCP failed to create SOs well, with issues involving:
[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.Testing
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 Sourcery
Introduce a dedicated manage_scriptable_object workflow for reliable ScriptableObject creation and modification, and update surrounding tooling and tests to use and validate it.
New Features:
Enhancements:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.