fix: JSON material property handling + tests (manage_asset) #90
Conversation
…ercion and end-to-end; update test project manifest and ProjectVersion
…ly; resolve _BaseMap/_MainTex automatically and apply when missing name
|
Warning Rate limit exceeded@dsarno has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
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. WalkthroughAdded JSON-string coercion and action-name validation; extended material property resolution to be case-insensitive and alias-aware; added GameObject component property application; improved texture binding, logging, and dirty/save handling; expanded C# and Python tests; adjusted test package reference and Unity editor version. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ManageAsset
participant Parser
participant Resolver
participant AssetStore
Client->>ManageAsset: HandleCommand(action, path, properties)
ManageAsset->>ManageAsset: validate action against ValidActions
alt properties is JSON string
ManageAsset->>Parser: parse JSON string
Parser-->>ManageAsset: parsed object / parse-error
end
ManageAsset->>Resolver: classify keys (shader/float/color/texture) & resolve names
alt target is GameObject
ManageAsset->>AssetStore: locate GameObject & components
AssetStore-->>ManageAsset: component list
ManageAsset->>Resolver: resolve component types & nested properties
Resolver->>AssetStore: apply component property updates (per-component)
else target is Material
ManageAsset->>Resolver: map aliases -> shader property names
Resolver->>AssetStore: apply colors/floats/textures (candidate props)
else
ManageAsset->>AssetStore: apply top-level properties
end
ManageAsset->>AssetStore: mark dirty & save
AssetStore-->>ManageAsset: save result
ManageAsset-->>Client: return success / no-op / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR enhances the ManageAsset tool with flexible JSON material property handling, adding support for friendly property names (metallic, smoothness, albedo) and cross-pipeline compatibility. The implementation now accepts JSON strings as parameters, handles case-insensitive structured texture blocks, and automatically resolves property names across URP/HDRP/Standard pipelines.
Key Changes:
- JSON string coercion at entry point (lines 66-79) converts string properties to JObject
- Friendly name aliasing:
metallic→_Metallic,smoothness→_Smoothness/_Glossiness,albedo→_BaseMap/_MainTex - Case-insensitive structured texture blocks with automatic
_BaseMap/_MainTexresolution when name omitted - Direct property assignment maintained for backward compatibility
- Comprehensive C# tests added covering JSON parsing, float properties, texture assignment, and 10-scenario end-to-end flow
- Python unit tests for JSON parsing logic (standalone and integration)
Minor Issues:
- Debug log left in modify action (line 92) - should be removed before merge
- Unity editor version downgraded from 2022.3.6f1 to 2021.3.45f2 - verify compatibility
Confidence Score: 4/5
- This PR is safe to merge with minor cleanup needed (debug log removal).
- Score reflects solid implementation with comprehensive test coverage across C# and Python. The changes are backward-compatible and additive. Minor concerns include a debug log in production code and Unity version downgrade. No logical errors or breaking changes detected.
- ManageAsset.cs needs debug log removed (line 92). Verify Unity 2021.3.45f2 compatibility doesn't break newer features.
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| MCPForUnity/Editor/Tools/ManageAsset.cs | 4/5 | Enhanced material property handling with JSON parsing, friendly name aliases, case-insensitive texture blocks, and cross-pipeline property resolution. Debug log added to modify action. Good error handling throughout. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs | 4/5 | Added comprehensive test coverage for JSON parsing, float properties, texture assignment, and end-to-end scenarios. Tests validate multi-pipeline shader support and graceful error handling. Contains 10-scenario end-to-end test. |
| tests/test_manage_asset_json_parsing.py | 3/5 | Integration tests for manage_asset JSON parsing with mocked Unity connection. Tests reference tools.manage_asset module which may not exist in this codebase - needs verification. |
| TestProjects/UnityMCPTests/ProjectSettings/ProjectVersion.txt | 3/5 | Unity editor version downgraded from 2022.3.6f1 to 2021.3.45f2 - could indicate compatibility concerns or testing environment change. |
Sequence Diagram
sequenceDiagram
participant Client as MCP Client
participant Server as Python MCP Server
participant Unity as Unity Editor (C#)
participant Asset as Material Asset
Client->>Server: manage_asset(action="create", properties="{...}")
Server->>Server: Parse JSON string to dict
Server->>Unity: Send command with properties dict
Unity->>Unity: HandleCommand - Coerce JSON string
Note over Unity: Line 66-79: Parse properties<br/>from JSON string to JObject
Unity->>Unity: CreateAsset/ModifyAsset
Unity->>Unity: ApplyMaterialProperties
Note over Unity: ResolvePropertyName()<br/>metallic → _Metallic<br/>smoothness → _Smoothness<br/>albedo → _BaseMap/_MainTex
alt Structured Texture Block
Unity->>Unity: Find "texture" key (case-insensitive)
Unity->>Unity: Extract name/Name and path/Path
Unity->>Unity: Resolve _BaseMap/_MainTex if name empty
Unity->>Asset: SetTexture(resolvedName, texture)
end
alt Direct Property Assignment
Unity->>Unity: Iterate non-reserved keys
Unity->>Unity: ResolvePropertyName (aliases + case)
alt Color Array
Unity->>Asset: SetColor(property, [r,g,b,a])
else Float
Unity->>Asset: SetFloat(property, value)
else Texture String
Unity->>Asset: SetTexture(property, path)
end
end
Unity-->>Server: Return success response
Server-->>Client: Return result
6 files reviewed, 2 comments
| m_EditorVersion: 2021.3.45f2 | ||
| m_EditorVersionWithRevision: 2021.3.45f2 (88f88f591b2e) |
There was a problem hiding this comment.
style: Unity version downgraded from 2022.3.6f1 to 2021.3.45f2 - check if this downgrade affects compatibility with newer Unity features used in the codebase.
Prompt To Fix With AI
This is a comment left during a code review.
Path: TestProjects/UnityMCPTests/ProjectSettings/ProjectVersion.txt
Line: 1:2
Comment:
**style:** Unity version downgraded from 2022.3.6f1 to 2021.3.45f2 - check if this downgrade affects compatibility with newer Unity features used in the codebase.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Should work, will test with ci.
There was a problem hiding this comment.
Pull Request Overview
This PR improves material property handling in the ManageAsset tool by adding friendly-name/property aliasing and case-insensitive structured texture block support, while maintaining backward compatibility with direct property assignment.
Key changes:
- Added friendly-name aliasing for material properties (
metallic→_Metallic,smoothness↔_Smoothness/_Glossiness,albedo↔_BaseMap/_MainTex) - Implemented case-insensitive texture block parsing with automatic property name resolution
- Added comprehensive test coverage including JSON string parsing tests and end-to-end property handling scenarios
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_manage_asset_json_parsing.py | New Python test file validating JSON string parameter parsing and invalid JSON handling |
| tests/test_json_parsing_simple.py | New Python test file for core JSON parsing logic without MCP dependencies |
| TestProjects/UnityMCPTests/ProjectSettings/ProjectVersion.txt | Updated Unity editor version for test project |
| TestProjects/UnityMCPTests/Packages/manifest.json | Changed MCP package reference from git URL to local file path |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs | Added helper for shader validation, comprehensive tests for JSON parsing, float properties, texture assignment, and end-to-end scenarios |
| MCPForUnity/Editor/Tools/ManageAsset.cs | Enhanced texture property assignment with case-insensitive parsing and friendly-name aliasing support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
TestProjects/UnityMCPTests/ProjectSettings/ProjectVersion.txt (1)
1-2: Unity Editor version downgrade — verify CI/tooling alignment.Moving tests to 2021.3.45f2 can break CI if runners still use 2022.3.x and may affect package compatibility. Confirm:
- CI uses 2021.3.45f2 for this project path.
- Packages in Packages/manifest.json remain compatible with 2021.3.45f2.
- Local dev README/workflow reflects this version.
TestProjects/UnityMCPTests/Packages/manifest.json (2)
3-3: Local package path may fail in CI."file:../../../MCPForUnity" assumes a specific repo layout. Ensure CI checks out this path relative to the test project, or gate with an env-based override (e.g., use file: locally and a pinned Git URL in CI).
10-10: Avoid nonessential Git-based IDE packages in test manifests.Git-sourced com.unity.ide.windsurf can add network flakiness. Consider removing from the test project or pinning to a commit for deterministic CI.
Suggested removal:
- "com.unity.ide.windsurf": "https://github.com/Asuta/com.unity.ide.windsurf.git",tests/test_json_parsing_simple.py (1)
14-20: Minor: structure try/except/else to satisfy TRY300.Pure style; keeps behavior identical.
- if isinstance(properties, str): - try: - parsed = json.loads(properties) - return parsed, "success" - except json.JSONDecodeError as e: - return properties, f"failed to parse: {e}" - return properties, "no_parsing_needed" + if isinstance(properties, str): + try: + parsed = json.loads(properties) + except json.JSONDecodeError as e: + return properties, f"failed to parse: {e}" + else: + return parsed, "success" + return properties, "no_parsing_needed"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
MCPForUnity/Editor/Tools/ManageAsset.cs(3 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs(6 hunks)TestProjects/UnityMCPTests/Packages/manifest.json(1 hunks)TestProjects/UnityMCPTests/ProjectSettings/ProjectVersion.txt(1 hunks)tests/test_json_parsing_simple.py(1 hunks)tests/test_manage_asset_json_parsing.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs (2)
MCPForUnity/Editor/Tools/ManageGameObject.cs (4)
GameObject(1284-1307)HandleCommand(42-202)Color(2124-2137)UnityEngine(2177-2274)MCPForUnity/Editor/Tools/ManageAsset.cs (3)
HandleCommand(46-128)DeleteAsset(512-540)CreateFolder(297-350)
tests/test_manage_asset_json_parsing.py (1)
tests/test_helpers.py (2)
info(3-4)warning(6-7)
🪛 Ruff (0.14.1)
tests/test_json_parsing_simple.py
17-17: Consider moving this statement to an else block
(TRY300)
tests/test_manage_asset_json_parsing.py
60-60: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
🔇 Additional comments (7)
tests/test_manage_asset_json_parsing.py (1)
18-20: Confirm whether ctx.info/ctx.warning are awaited.These are AsyncMocks. If manage_asset calls them without awaiting, Python can emit “coroutine was never awaited” warnings. Please verify and switch to Mock (sync) or
awaitthe calls accordingly.TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs (4)
22-32: Helper looks good.Accepts URP/HDRP/Standard/Unlit and guards nulls. No changes needed.
65-71: Creation flow assertions LGTM.Material load and shader checks are solid; optional color check already guarded by HasProperty.
125-135: GameObject assignment verification LGTM.Good end-to-end validation of material assignment and shader across renderer.
186-190: Shader assertion after JSON coercion LGTM.Confirms created material shader; matches cross-pipeline helper.
MCPForUnity/Editor/Tools/ManageAsset.cs (2)
66-79: JSON string coercion to JObject looks goodCovers the core need and warns on parse failure; no behavior regressions noted.
1068-1081: Add test for structured texture block with "albedo" friendly nameThe alias table properly maps "albedo" to ["_BaseMap", "_MainTex"]. However, existing texture assignment tests only cover direct property assignment (e.g.,
["_BaseMap"] = path), not structured texture blocks. Add a test case with"texture": {"name":"albedo","path":"Assets/..."}to verify the friendly name works within structured inputs.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs
Show resolved
Hide resolved
TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs
Show resolved
Hide resolved
TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs
Show resolved
Hide resolved
| # Test with invalid JSON string | ||
| result = await manage_asset( | ||
| ctx=ctx, | ||
| action="create", | ||
| path="Assets/Test.mat", | ||
| asset_type="Material", | ||
| properties='{"invalid": json, "missing": quotes}' | ||
| ) | ||
|
|
||
| # Verify warning was logged | ||
| ctx.warning.assert_called() | ||
| assert "failed to parse properties JSON string" in str(ctx.warning.call_args) | ||
|
|
There was a problem hiding this comment.
Fix unused variable and strengthen assertion (Ruff F841).
Use the result to check success (or drop the assignment).
- result = await manage_asset(
+ result = await manage_asset(
ctx=ctx,
action="create",
path="Assets/Test.mat",
asset_type="Material",
properties='{"invalid": json, "missing": quotes}'
)
@@
- ctx.warning.assert_called()
- assert "failed to parse properties JSON string" in str(ctx.warning.call_args)
+ ctx.warning.assert_called()
+ assert "failed to parse properties JSON string" in str(ctx.warning.call_args)
+ assert result.get("success") is TrueCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.1)
60-60: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
🤖 Prompt for AI Agents
In tests/test_manage_asset_json_parsing.py around lines 59 to 71, the test
currently assigns the return value of manage_asset to "result" but never uses it
and only checks ctx.warning; either assert something about result (e.g., that it
indicates failure/None/False depending on manage_asset contract) or drop the
assignment entirely. Also strengthen the warning assertion by checking the
warning call arguments explicitly (e.g., assert any("failed to parse properties
JSON string" in str(arg) for call in ctx.warning.call_args_list for arg in call)
or use ctx.warning.assert_called_with(...) with the expected message fragment)
so the test both uses the result or removes the unused variable and verifies the
exact warning contents.
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
MCPForUnity/Editor/Tools/ManageAsset.cs (1)
993-1054: Refactor structured texture block to support friendly aliases and remove goto.This block doesn't honor friendly aliases (
albedo,metallic,smoothness) because it bypassesResolvePropertyName(lines 1064–1088). The PR objectives promise alias support for these names, but the structured texture path won't map"albedo"to_BaseMap/_MainTex.Additionally, the
goto AfterTexturepattern reduces readability.Refactor to call
ResolvePropertyNameand remove the goto/label:// Example: Set texture property (case-insensitive key and subkeys) { JObject texProps = null; var direct = properties.Property("texture"); if (direct != null && direct.Value is JObject t0) texProps = t0; if (texProps == null) { - var ci = properties.Properties().FirstOrDefault(p => string.Equals(p.Name, "texture", StringComparison.OrdinalIgnoreCase)); + var ci = properties.Properties().FirstOrDefault( + p => string.Equals(p.Name, "texture", StringComparison.OrdinalIgnoreCase)); if (ci != null && ci.Value is JObject t1) texProps = t1; } - if (texProps == null) goto AfterTexture; - - string propName = (texProps["name"] ?? texProps["Name"])?.ToString(); - string texPath = (texProps["path"] ?? texProps["Path"])?.ToString(); - if (!string.IsNullOrEmpty(texPath)) + if (texProps != null) { - Texture newTex = AssetDatabase.LoadAssetAtPath<Texture>( - AssetPathUtility.SanitizeAssetPath(texPath) - ); - if (newTex == null) - { - Debug.LogWarning($"Texture not found at path: {texPath}"); - } - else + string rawName = (texProps["name"] ?? texProps["Name"])?.ToString(); + string texPath = (texProps["path"] ?? texProps["Path"])?.ToString(); + if (!string.IsNullOrEmpty(texPath)) { - // Resolve candidate property names across pipelines - string[] candidates; - string lower = (propName ?? string.Empty).ToLowerInvariant(); - if (string.IsNullOrEmpty(propName)) - { - candidates = new[] { "_BaseMap", "_MainTex" }; - } - else if (lower == "_maintex") - { - candidates = new[] { "_MainTex", "_BaseMap" }; - } - else if (lower == "_basemap") + var newTex = AssetDatabase.LoadAssetAtPath<Texture>( + AssetPathUtility.SanitizeAssetPath(texPath)); + if (newTex == null) { - candidates = new[] { "_BaseMap", "_MainTex" }; + Debug.LogWarning($"Texture not found at path: {texPath}"); } else { - candidates = new[] { propName }; - } - - foreach (var candidate in candidates) - { - if (mat.HasProperty(candidate)) + // Reuse ResolvePropertyName so friendly aliases (albedo, etc.) work here too + string targetProp = string.IsNullOrEmpty(rawName) + ? ResolvePropertyName("_BaseMap") + : ResolvePropertyName(rawName); + if (!string.IsNullOrEmpty(targetProp) && + mat.HasProperty(targetProp) && + mat.GetTexture(targetProp) != newTex) { - if (mat.GetTexture(candidate) != newTex) - { - mat.SetTexture(candidate, newTex); - modified = true; - } - // Stop after first applicable candidate - break; + mat.SetTexture(targetProp, newTex); + modified = true; } } } } } -AfterTexture:Based on PR objectives.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Tools/ManageAsset.cs(3 hunks)
🔇 Additional comments (1)
MCPForUnity/Editor/Tools/ManageAsset.cs (1)
1064-1088: LGTM! Friendly alias resolution implemented correctly.The
ResolvePropertyNamehelper properly maps friendly names (metallic,smoothness,albedo) to their shader property equivalents across URP/Standard pipelines, with appropriate fallback logic.
…red texture (supports 'albedo'); tests: self-sufficient texture asset and _BaseColor/_Color guards; python: assert success in invalid JSON case
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
MCPForUnity/Editor/Tools/ManageAsset.cs (1)
91-93: Critical: Remove duplicate return statement.Line 93 duplicates the return on line 92, making it unreachable dead code. This will cause compiler errors or warnings.
Apply this diff:
case "modify": var properties = @params["properties"] as JObject; return ModifyAsset(path, properties); - return ModifyAsset(path, properties);
🧹 Nitpick comments (1)
tests/test_manage_asset_json_parsing.py (1)
68-71: Strengthen the warning assertion.The current assertion converts
call_argsto a string and checks for a substring, which is fragile. Access the actual warning message directly for a more precise check.Apply this diff:
- # Verify warning was logged - ctx.warning.assert_called() - assert "failed to parse properties JSON string" in str(ctx.warning.call_args) - assert result.get("success") is True + # Verify warning was logged with expected message + ctx.warning.assert_called_once() + warning_message = ctx.warning.call_args[0][0] + assert "failed to parse properties JSON string" in warning_message.lower() + assert result.get("success") is True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Tools/ManageAsset.cs(3 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs(6 hunks)tests/test_manage_asset_json_parsing.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs (2)
MCPForUnity/Editor/Tools/ManageGameObject.cs (4)
GameObject(1284-1307)HandleCommand(42-202)Color(2124-2137)UnityEngine(2177-2274)MCPForUnity/Editor/Tools/ManageAsset.cs (4)
HandleCommand(46-128)DeleteAsset(512-540)CreateFolder(297-350)CreateAsset(164-295)
tests/test_manage_asset_json_parsing.py (1)
tests/test_helpers.py (2)
info(3-4)warning(6-7)
🔇 Additional comments (8)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs (6)
22-32: LGTM!The helper centralizes shader validation across multiple tests, improving maintainability. The hardcoded list of acceptable shaders ensures tests remain stable across different pipeline configurations.
200-238: LGTM!The test properly validates JSON string parsing for material creation with unique paths, shader validation via the helper, and guarded color assertions. Cleanup is handled correctly.
293-325: LGTM!The test correctly validates graceful handling of invalid JSON strings using
LogAssert.Expectwith a regex pattern. The acceptance of either success (with defaults) or failure is a reasonable design choice for error resilience.
327-384: LGTM!The test comprehensively validates float property handling (metallic) for both create and modify operations. Tolerance-based assertions (
1e-3f) are appropriate for float comparisons, and the test structure is solid.
386-464: LGTM!The test now addresses the past review concern by creating the texture dynamically if missing (lines 418-432), making it self-sufficient. The verification logic handles both
_BaseMapand_MainTexproperties, ensuring pipeline resilience.
466-697: Excellent comprehensive test coverage.This end-to-end test validates all 10 property handling scenarios in a single flow, including JSON parsing, friendly aliases, structured blocks, texture assignment, pipeline switching, and invalid JSON handling. Past review concerns about color assertion resilience are fully addressed with
HasPropertyguards (lines 503-508, 522-527) and tolerance-based comparisons (line 665).MCPForUnity/Editor/Tools/ManageAsset.cs (2)
993-1032: LGTM! Texture handling improved with case-insensitive keys and alias support.The refactored texture block now:
- Accepts the
"texture"key case-insensitively (lines 996-1003)- Supports case-insensitive
"name"/"Name"and"path"/"Path"subkeys (lines 1006-1007)- Defaults to
"_BaseMap"when name is omitted (line 1019)- Reuses
ResolvePropertyNameto honor friendly aliases like"albedo"(line 1020)- Eliminates the goto/label pattern for cleaner flow
This aligns with the PR objectives for structured texture support.
1046-1060: LGTM! Friendly-name aliases implemented as described in PR objectives.The extended property resolver now maps:
"metallic"→"_Metallic"(line 1056)"smoothness"→"_Smoothness"/"_Glossiness"(line 1057)"albedo"→"_BaseMap"/"_MainTex"(line 1058)This enables user-friendly property names while maintaining compatibility with various shader pipelines (URP/HDRP/Standard).
…valid-json case tolerant; ensure prefab modify test patches transport correctly
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
MCPForUnity/Editor/Tools/ManageAsset.cs (2)
912-938: Use ResolvePropertyName for color to support _Color/_BaseColor across pipelines.Without the resolver, “color” defaults to
_Colorand may miss URP’s_BaseColor. Apply the mapping in both object and array paths.- string propName = colorProps["name"]?.ToString() ?? "_Color"; // Default main color + string propName = colorProps["name"]?.ToString() ?? "_Color"; // Default main color + propName = ResolvePropertyName(propName); @@ - string propName = "_Color"; + string propName = ResolvePropertyName("_Color");Also applies to: 939-965
967-991: Apply alias resolver for structured float (“metallic”, “smoothness”).
float.nameisn’t resolved, so friendly names won’t map (e.g., “metallic” → “_Metallic”). Resolve before HasProperty/SetFloat.- string propName = floatProps["name"]?.ToString(); + string propName = ResolvePropertyName(floatProps["name"]?.ToString());
🧹 Nitpick comments (2)
tests/test_manage_asset_json_parsing.py (1)
22-25: Silence Ruff ARG001 by underscoring unused parameters in fakes.These test doubles don’t use their inputs. Prefix them with “_” to avoid warnings without changing behavior.
- async def fake_async(cmd, params, loop=None): + async def fake_async(_cmd, _params, _loop=None): return {"success": True, "message": "Asset created successfully", "data": {"path": "Assets/Test.mat"}} @@ - async def fake_async(cmd, params, loop=None): + async def fake_async(_cmd, _params, _loop=None): return {"success": True, "message": "Asset created successfully"} @@ - async def fake_async(cmd, params, loop=None): + async def fake_async(_cmd, _params, _loop=None): return {"success": True, "message": "Asset created successfully"} @@ - async def fake_async(cmd, params, loop=None): + async def fake_async(_cmd, _params, _loop=None): return {"success": True, "message": "Asset created successfully"} @@ - def fake_send(cmd, params): + def fake_send(_cmd, _params): return {"success": True, "message": "GameObject created successfully"}Also applies to: 49-52, 72-75, 97-100, 127-130
tests/test_script_tools.py (1)
181-187: Simplify patching—module-level setattr is enough here.The extra
__globals__injection is brittle. Prefer only the module-level patch; keep the globals override only if you’ve seen aliasing flakiness locally.- # Patch both at the module and at the function closure location - monkeypatch.setattr(tools_manage_asset, - "async_send_command_with_retry", fake_async) - # Also patch the globals of the function object (handles dynamically loaded module alias) - manage_asset.__globals__["async_send_command_with_retry"] = fake_async + # Patch the async helper where the tool resolves it + monkeypatch.setattr(tools_manage_asset, "async_send_command_with_retry", fake_async)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Tools/ManageAsset.cs(3 hunks)tests/test_manage_asset_json_parsing.py(1 hunks)tests/test_script_tools.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_script_tools.py (1)
MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py (1)
manage_asset(16-92)
🪛 Ruff (0.14.1)
tests/test_manage_asset_json_parsing.py
22-22: Unused function argument: cmd
(ARG001)
22-22: Unused function argument: params
(ARG001)
22-22: Unused function argument: loop
(ARG001)
49-49: Unused function argument: cmd
(ARG001)
49-49: Unused function argument: params
(ARG001)
49-49: Unused function argument: loop
(ARG001)
72-72: Unused function argument: cmd
(ARG001)
72-72: Unused function argument: params
(ARG001)
72-72: Unused function argument: loop
(ARG001)
97-97: Unused function argument: cmd
(ARG001)
97-97: Unused function argument: params
(ARG001)
97-97: Unused function argument: loop
(ARG001)
127-127: Unused function argument: cmd
(ARG001)
127-127: Unused function argument: params
(ARG001)
🔇 Additional comments (1)
MCPForUnity/Editor/Tools/ManageAsset.cs (1)
992-1031: Structured texture block aligns with goals (case-insensitive, aliases, default fallback).Reuses the resolver, supports Name/Path, and falls back to _BaseMap→_MainTex. Looks good.
If you intended “albedo” as an alias for textures here too, it’s already covered via
ResolvePropertyName("albedo")returning_BaseMap/_MainTex. Please confirm with a quick test payload like:
{"texture": {"name": "albedo", "path": "Assets/.."}}.
…ayDev#349) * feat: add JSON property handling for materials; add tests for JSON coercion and end-to-end; update test project manifest and ProjectVersion * fix(manage_asset): support structured texture blocks case-insensitively; resolve _BaseMap/_MainTex automatically and apply when missing name * Update MCPForUnity/Editor/Tools/ManageAsset.cs Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> * refactor(manage_asset): remove goto; reuse alias resolver for structured texture (supports 'albedo'); tests: self-sufficient texture asset and _BaseColor/_Color guards; python: assert success in invalid JSON case * chore(manage_asset): remove duplicate return in modify case * tests: fix mocks/patching for manage_asset/manage_gameobject; make invalid-json case tolerant; ensure prefab modify test patches transport correctly * ci: allow manual dispatch for Unity EditMode tests (workflow_dispatch) --------- Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
ManageAsset:metallic→_Metallic,smoothness↔_Smoothness/_Glossiness,albedo↔_BaseMap/_MainTex"texture"key regardless of casing"name"/"Name"and"path"/"Path"_BaseMap/_MainTexautomatically{"_BaseMap": "Assets/.../Tex.asset"}Tests
MCPToolParameterTests:_Metalliccreate/modify testMisc
TestProjects/UnityMCPTests/Packages/manifest.jsonProjectSettings/ProjectVersion.txtfor test projectResult
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores