Skip to content

Conversation

@dsarno
Copy link
Collaborator

@dsarno dsarno commented Oct 23, 2025

Summary

  • Widened Python tool schemas and added server-side coercion:
    • manage_asset.properties accepts dict or JSON string; numeric pagination accepts strings.
    • manage_gameobject.component_properties accepts dict or JSON string; booleans and vectors tolerate string inputs.
  • Added focused EditMode tests:
    • Material creation with properties (object + JSON-string paths).
    • GameObject modify with componentProperties as object and JSON string.
    • Verified serializer returns material data without instantiating in EditMode.
  • Fixed flakiness:
    • Unique, GUID-suffixed asset paths; aggressive cleanup in SetUp/TearDown.
    • Resolved type ambiguities (UnityEngine.Object).

Why

Eliminates frequent “invalid param” failures from FastMCP pre-validation and unblocks real-world LLM/automation clients that stringify inputs. Provides a test-backed baseline for material management via MCP.

Notes

  • Live verification included: JSON-string material create/assign, vector/boolean coercions, console pagination.
  • Follow-up (separate branch): extend ApplyMaterialProperties to support direct _Color/_Glossiness/_MainTex assignment.

Summary by CodeRabbit

  • New Features

    • Tools now accept JSON-formatted properties and component properties provided as strings; these are automatically parsed before use.
  • Bug Fixes

    • Port discovery updated to recognize an additional Unity bridge connection indicator.
  • Tests

    • Added editor test suites validating JSON parameter parsing, material creation, and component property assignment (with defensive handling when environment variations occur).

- MCPMaterialTests.cs: Tests for material creation, assignment, and data reading
- MCPParameterHandlingTests.cs: Tests for JSON parameter parsing issues
- SphereMaterialWorkflowTests.cs: Tests for complete sphere material workflow

These tests document the current issues with:
- JSON parameter parsing in manage_asset and manage_gameobject tools
- Material creation with properties
- Material assignment to GameObjects
- Material component data reading

All tests currently fail (Red phase of TDD) and serve as specifications
for what needs to be fixed in the MCP system.
- Removed redundant tests that verify working functionality (GameObjectSerializer, Unity APIs)
- Kept focused tests that document the real issue: MCP tool parameter validation
- Tests now clearly identify the root cause: JSON string parsing in MCP tools
- Tests specify exactly what needs to be fixed: parameter type flexibility

The issue is NOT in Unity APIs or serialization (which work fine),
but in MCP tool parameter validation being too strict.
- Update _try_probe_unity_mcp to recognize Unity bridge welcome message
- Unity bridge sends 'WELCOME UNITY-MCP' instead of JSON pong response
- Maintains backward compatibility with JSON pong format
- Fixes MCP server connection to Unity Editor
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 23, 2025

Walkthrough

Preprocesses tool parameters so JSON strings are coerced into structured objects in C# editor tools and Python server tools; updates port probe to accept a Unity bridge welcome message; adds NUnit EditMode tests, asset metadata, and package/project version updates.

Changes

Cohort / File(s) Summary
C# Editor Tools
MCPForUnity/Editor/Tools/ManageAsset.cs, MCPForUnity/Editor/Tools/ManageGameObject.cs, MCPForUnity/Editor/Tools/JsonUtil.cs
Adds internal JsonUtil.CoerceJsonStringParameter and calls it to parse stringified JSON parameters (e.g., properties, componentProperties) into JObject; parse failures are logged; no public signatures changed.
Python MCP Tools
MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py, MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py
Coerces JSON-string properties / component_properties into Python dicts using json.loads; logs or returns on invalid JSON; ensures dict when None.
Port Discovery
MCPForUnity/UnityMcpServer~/src/port_discovery.py
_try_probe_unity_mcp now recognizes a Unity bridge welcome message (WELCOME UNITY-MCP) in addition to the previous JSON "pong" pattern; docstring updated.
Tests — New
TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs, TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs
New NUnit EditMode tests validating JSON parameter parsing/coercion, material creation, material assignment via componentProperties, and renderer serialization behavior; include setup/teardown and cleanup.
Tests — Updates
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs
Replaces direct enum reference with Enum.TryParse and skips the test if the enum value is unavailable.
Project & Packages
TestProjects/UnityMCPTests/Packages/manifest.json, TestProjects/UnityMCPTests/ProjectSettings/ProjectVersion.txt, TestProjects/UnityMCPTests/ProjectSettings/Packages/com.unity.testtools.codecoverage/Settings.json
Adds [email protected], updates com.unity.timeline to 1.7.5, bumps Unity project version to 2022.3.6f1, and removes top-level m_Name/m_Path from codecoverage settings.
Asset Metadata
TestProjects/UnityMCPTests/Assets/Materials.meta, TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs.meta, TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs.meta
Updates Materials.meta GUID and adds MonoImporter metadata files for new test source files.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant EditorTool as Editor Tool (C#)
    participant ServerTool as Server Tool (Py)
    participant Parser as JSON Parser
    participant Logic as Existing Logic

    Client->>EditorTool: Command(payload with properties/componentProperties)
    rect #EBF5FF
    note right of EditorTool: NEW — pre-process parameter
    EditorTool->>Parser: If param is string -> parse JSON
    alt parse succeeds
        Parser->>EditorTool: JObject
        EditorTool->>EditorTool: replace param in @params
    else parse fails
        Parser->>EditorTool: log warning
        EditorTool->>EditorTool: keep original
    end
    end
    EditorTool->>Logic: Invoke action with processed params
    Logic->>Client: Return result

    Client->>ServerTool: Command(payload with properties/component_properties)
    rect #F7FBF3
    note right of ServerTool: NEW — coerce JSON strings to dict
    ServerTool->>Parser: If param is string -> json.loads
    alt parse succeeds
        Parser->>ServerTool: dict
        ServerTool->>ServerTool: replace param
    else parse fails
        Parser->>ServerTool: log / return error
    end
    end
    ServerTool->>Logic: Invoke action with processed params
    Logic->>Client: Return result
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • Scriptwonder
  • justinpbarnett

Poem

🐇 I nibbled JSON crumbs with care,
Turned strings to objects, tidy and fair.
Commands now parse with joyful hops,
Tests clap paws for the cleaner props.
🌿🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 15.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "Harden MCP tool parameter handling + add material workflow tests (TDD)" accurately reflects the two primary changes in the changeset. The first part captures the core infrastructure improvements: adding server-side coercion for JSON string parameters across manage_asset.properties and manage_gameobject.component_properties, plus the new JsonUtil utility. The second part correctly identifies the new test files (MCPToolParameterTests.cs and MaterialParameterToolTests.cs) that validate JSON-string parameter handling and material workflows. The title is specific and clear without vague terms, and a teammate scanning the git history would immediately understand that this PR adds robustness to parameter handling and introduces material-focused test coverage.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f9756ea and c004994.

📒 Files selected for processing (1)
  • TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (7)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)

69-82: Consider extracting JSON coercion to a shared helper method.

This JSON string coercion logic is duplicated in ManageAsset.cs (lines 66-79). Consider extracting to a shared utility method to reduce duplication and ensure consistent behavior:

private static void CoerceJsonStringParameter(JObject @params, string paramName)
{
    var token = @params[paramName];
    if (token != null && token.Type == JTokenType.String)
    {
        try
        {
            var parsed = JObject.Parse(token.ToString());
            @params[paramName] = parsed;
        }
        catch (Exception e)
        {
            Debug.LogWarning($"[MCP] Could not parse '{paramName}' JSON string: {e.Message}");
        }
    }
}

Then both files could call:

CoerceJsonStringParameter(@params, "componentProperties");
MCPForUnity/Editor/Tools/ManageAsset.cs (1)

66-79: JSON coercion mirrors ManageGameObject implementation.

This JSON string coercion for the properties parameter mirrors the identical pattern added to ManageGameObject.cs for componentProperties. See my earlier comment on ManageGameObject.cs (lines 69-82) regarding potential extraction to a shared helper method to reduce duplication.

The implementation itself is correct: parses JSON string to JObject, logs warning on failure, and continues execution gracefully.

TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs (2)

24-33: Extract common folder setup to SetUp method.

The folder creation logic is duplicated across all three tests (lines 24-33, 73-75, 124-126). Consider adding a [SetUp] method to handle this initialization, similar to the pattern used in MaterialParameterToolTests.cs.

Apply this pattern:

+    private const string TempDir = "Assets/Temp/MCPToolParameterTests";
+
+    [SetUp]
+    public void SetUp()
+    {
+        if (!AssetDatabase.IsValidFolder("Assets/Temp"))
+        {
+            AssetDatabase.CreateFolder("Assets", "Temp");
+        }
+        if (!AssetDatabase.IsValidFolder(TempDir))
+        {
+            AssetDatabase.CreateFolder("Assets/Temp", "MCPToolParameterTests");
+        }
+    }

Then remove the duplicated setup code from each test method.


62-62: Qualify UnityEngine.Object to avoid ambiguity.

Using UnityEngine.Object without qualification can be ambiguous with System.Object. While the code compiles correctly, explicit qualification improves clarity, especially in test contexts where multiple namespaces are in use.

Consider using the fully qualified name consistently, or add a using alias at the top:

using Object = UnityEngine.Object;

Also applies to: 116-116, 166-166

TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs (2)

70-74: Remove redundant cleanup—SetUp already handles this.

Lines 70-74 duplicate the cleanup logic already performed in SetUp (lines 30-34). This check is unnecessary and can be removed.

-        // Ensure a clean state if a previous run left the asset behind (uses _matPath now)
-        if (AssetDatabase.LoadAssetAtPath<UnityEngine.Object>(_matPath) != null)
-        {
-            AssetDatabase.DeleteAsset(_matPath);
-            AssetDatabase.Refresh();
-        }
         var createParams = new JObject

159-161: Consider splitting the long assertion for clarity.

The assertion on lines 159-161 checks four different property keys. Splitting this into separate assertions or adding a helper method would improve readability and make test failures more specific.

bool hasMaterialProperty = props.ContainsKey("sharedMaterial") || 
                          props.ContainsKey("material") || 
                          props.ContainsKey("sharedMaterials") || 
                          props.ContainsKey("materials");
Assert.IsTrue(hasMaterialProperty, "Serialized data should include material info.");
MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (1)

46-51: Clarify that string input must be valid JSON.

The annotation mentions the parameter accepts a string, but the documentation examples only show dict format. Add a note that string input must be valid JSON matching the dict structure.

-    component_properties: Annotated[dict[str, dict[str, Any]] | str,
+    component_properties: Annotated[dict[str, dict[str, Any]] | str,
                                     """Dictionary of component names to their properties to set. For example:
+                                    Can also be provided as a JSON string representation of the dict.
                                     `{"MyScript": {"otherObject": {"find": "Player", "method": "by_name"}}}` assigns GameObject
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 26eafbf and 1bdbeb8.

⛔ Files ignored due to path filters (1)
  • MCPForUnity/UnityMcpServer~/src/uv.lock is excluded by !**/*.lock
📒 Files selected for processing (13)
  • MCPForUnity/Editor/Tools/ManageAsset.cs (1 hunks)
  • MCPForUnity/Editor/Tools/ManageGameObject.cs (1 hunks)
  • MCPForUnity/UnityMcpServer~/src/port_discovery.py (1 hunks)
  • MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py (2 hunks)
  • MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (4 hunks)
  • TestProjects/UnityMCPTests/Assets/Materials.meta (1 hunks)
  • TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs (1 hunks)
  • TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs (1 hunks)
  • TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs (1 hunks)
  • TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs.meta (1 hunks)
  • TestProjects/UnityMCPTests/Packages/manifest.json (2 hunks)
  • TestProjects/UnityMCPTests/ProjectSettings/Packages/com.unity.testtools.codecoverage/Settings.json (0 hunks)
  • TestProjects/UnityMCPTests/ProjectSettings/ProjectVersion.txt (1 hunks)
💤 Files with no reviewable changes (1)
  • TestProjects/UnityMCPTests/ProjectSettings/Packages/com.unity.testtools.codecoverage/Settings.json
🧰 Additional context used
🧬 Code graph analysis (3)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs (2)
MCPForUnity/Editor/Tools/ManageGameObject.cs (4)
  • UnityEngine (2177-2274)
  • HandleCommand (42-202)
  • Color (2124-2137)
  • GameObject (1284-1307)
MCPForUnity/Editor/Tools/ManageAsset.cs (3)
  • CreateFolder (295-348)
  • HandleCommand (46-126)
  • DeleteAsset (510-538)
MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py (1)
tests/test_helpers.py (1)
  • info (3-4)
MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (1)
tests/test_helpers.py (1)
  • info (3-4)
🪛 Ruff (0.14.1)
MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py

42-42: Do not catch blind exception: Exception

(BLE001)

MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py

122-122: Do not catch blind exception: Exception

(BLE001)

🔇 Additional comments (9)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs (1)

137-142: Good defensive programming for optional enum value.

The runtime check for McpTypes.Trae availability is appropriate and makes the test resilient to package versions where this enum value may not exist. The selective application (only Trae has this check, not Windsurf/Kiro/etc.) correctly suggests Trae is version-dependent.

TestProjects/UnityMCPTests/Assets/Materials.meta (1)

2-2: LGTM - GUID regeneration.

The GUID update for the Materials folder metadata is acceptable. This is likely part of the test infrastructure reorganization mentioned in the PR.

TestProjects/UnityMCPTests/ProjectSettings/ProjectVersion.txt (1)

1-2: LGTM - Unity version upgrade to LTS release.

The upgrade to Unity 2022.3.6f1 (an LTS release) modernizes the test project infrastructure and aligns with the broader test enhancements in this PR.

MCPForUnity/UnityMcpServer~/src/port_discovery.py (1)

57-74: LGTM - Backward-compatible probe enhancement.

The updated probe logic correctly accepts both the new Unity bridge welcome message ("WELCOME UNITY-MCP") and the previous pong pattern, ensuring graceful migration without breaking existing deployments.

TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs.meta (1)

1-11: LGTM - Standard Unity test metadata.

This is standard Unity MonoImporter metadata for the new MaterialParameterToolTests.cs test file. The configuration is appropriate with default settings.

TestProjects/UnityMCPTests/Packages/manifest.json (1)

4-4: LGTM - Dependency updates support new test requirements.

The additions of:

  • com.unity.ai.navigation 1.1.4
  • com.unity.timeline upgrade to 1.7.5

are appropriate for the expanded test coverage and Unity version upgrade in this PR.

Also applies to: 13-13

TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs (1)

21-169: Test coverage looks good—JSON coercion paths are well exercised.

The three tests effectively validate:

  • JSON string properties in manage_asset (Test 1)
  • JSON string componentProperties in manage_gameobject (Test 2)
  • Combined workflows with both coercion paths (Test 3)

The use of GUID-suffixed paths ensures proper test isolation, and cleanup in finally blocks prevents resource leaks.

TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs (1)

17-59: Excellent SetUp/TearDown pattern with defensive cleanup.

The SetUp method's aggressive cleanup strategy (checking AssetDatabase, then hard-deleting files on disk) effectively prevents flakiness from leftover assets. The use of GUID-suffixed paths ensures test isolation.

MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (1)

1-1: JSON coercion feature aligns well with C# implementation.

The addition of JSON string support for component_properties provides client compatibility and mirrors the C# coercion logic in ManageGameObject.cs. The type annotation update and info logging are helpful for debugging.

Also applies to: 46-46, 117-123

…ix: fail fast on bad component_properties JSON\ntest: unify setup, avoid test-to-test calls\nchore: use relative MCP package path
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (1)

181-189: Fix the prefab_folder default or add validation to prevent None paths.

The issue is valid. At line 38–39, prefab_folder defaults to None. Line 177 filters None values from the params dict, but line 186 uses the local variable prefab_folder directly, bypassing that filter. When saveAsPrefab is True and prefabPath is omitted, constructing the path with a None folder produces "None/<name>.prefab", which is invalid.

Suggested fix: provide a safe default like "Assets/Prefabs" or validate that prefab_folder is a non-empty string before constructing the path:

-                # Use the provided prefab_folder (which has a default) and the name to construct the path
-                constructed_path = f"{prefab_folder}/{params['name']}.prefab"
+                # Use provided prefab_folder if valid; else fallback to a safe default
+                base_folder = prefab_folder if isinstance(prefab_folder, str) and prefab_folder else "Assets/Prefabs"
+                constructed_path = f"{base_folder}/{params['name']}.prefab"

Alternatively, require either prefab_path or a valid prefab_folder and return an error if neither is provided.

♻️ Duplicate comments (1)
MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (1)

118-124: Good: fail fast on invalid component_properties JSON (addresses prior review).

Switch to json.JSONDecodeError and returning an error is the right call; avoids forwarding invalid data to C#.

🧹 Nitpick comments (8)
MCPForUnity/Editor/Tools/JsonUtil.cs (1)

12-28: Make coercion safer and less noisy (guard + broader JSON exception).

Only attempt parse when value looks like an object (starts with '{'); catch JsonException to cover all Newtonsoft parse errors. Prevents warnings on arbitrary strings and whitespace.

 using Newtonsoft.Json.Linq;
+using Newtonsoft.Json;

 internal static void CoerceJsonStringParameter(JObject @params, string paramName)
 {
     if (@params == null || string.IsNullOrEmpty(paramName)) return;
     var token = @params[paramName];
     if (token != null && token.Type == JTokenType.String)
     {
         try
         {
-            var parsed = JObject.Parse(token.ToString());
+            var s = token.ToString()?.Trim();
+            if (string.IsNullOrEmpty(s) || s[0] != '{')
+            {
+                // Not an object JSON; skip to avoid noisy warnings.
+                return;
+            }
+            var parsed = JObject.Parse(s);
             @params[paramName] = parsed;
         }
-        catch (Newtonsoft.Json.JsonReaderException e)
+        catch (JsonException e)
         {
             Debug.LogWarning($"[MCP] Could not parse '{paramName}' JSON string: {e.Message}");
         }
     }
 }
TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs (4)

35-71: URP-safe color assertion.

URP Lit uses “_BaseColor”, not “_Color”. Check either to keep the test meaningful across pipelines.

-                if (mat.HasProperty("_Color"))
-                {
-                    Assert.AreEqual(Color.blue, mat.GetColor("_Color"));
-                }
+                var expected = Color.blue;
+                if (mat.HasProperty("_BaseColor"))
+                    Assert.AreEqual(expected, mat.GetColor("_BaseColor"));
+                else if (mat.HasProperty("_Color"))
+                    Assert.AreEqual(expected, mat.GetColor("_Color"));
+                // else: shader doesn't expose a color property; creation path is still validated.

96-111: Assert the material actually applied to the MeshRenderer.

Currently we only assert tool success. Verify the renderer’s sharedMaterial matches the created asset to cover end-to-end assignment.

                 var result = raw as JObject ?? JObject.FromObject(raw);
                 Assert.IsTrue(result.Value<bool>("success"), result.ToString());
+
+                // Stronger verification: material applied
+                var go = GameObject.Find("MCPParamTestSphere");
+                Assert.IsNotNull(go, "Expected GameObject to exist");
+                var mr = go.GetComponent<MeshRenderer>();
+                Assert.IsNotNull(mr, "Expected MeshRenderer on created primitive");
+                var createdMat = AssetDatabase.LoadAssetAtPath<Material>(matPath);
+                Assert.IsNotNull(createdMat, "Expected created material to exist");
+                Assert.AreEqual(createdMat, mr.sharedMaterial, "Material assignment via componentProperties failed");

Also applies to: 114-118


121-166: Reduce duplication via parameterized test or helper.

Tests 2 and 3 largely duplicate the same flow. Consider extracting helpers (create material, create sphere, assign material) or parameterizing JSON-string vs object paths to speed iteration and cut maintenance.


23-34: Optional: add TearDown to remove the Temp test folder.

Per-test cleanup is good; a class-level TearDown that deletes the TempDir (if empty) keeps the project tidy after local runs.

MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (3)

71-84: Boolean coercion is too permissive for unknown strings.

bool("foo") becomes True, which can flip flags unexpectedly. Prefer “unknown string -> default” instead of truthiness.

     def _coerce_bool(value, default=None):
         if value is None:
             return default
         if isinstance(value, bool):
             return value
         if isinstance(value, str):
             v = value.strip().lower()
             if v in ("true", "1", "yes", "on"):
                 return True
             if v in ("false", "0", "no", "off"):
                 return False
-        return bool(value)
+        return default

85-107: Vector coercion should drop invalid inputs instead of passing raw strings through.

Returning default=position propagates bad strings to C#, causing downstream parse issues. Return None on invalid.

-    def _coerce_vec(value, default=None):
+    def _coerce_vec(value, default=None):
         if value is None:
             return default
         import math
         def _to_vec3(parts):
             try:
                 vec = [float(parts[0]), float(parts[1]), float(parts[2])]
             except (ValueError, TypeError):
                 return default
             return vec if all(math.isfinite(n) for n in vec) else default
         if isinstance(value, list) and len(value) == 3:
             return _to_vec3(value)
         if isinstance(value, str):
             s = value.strip()
             # minimal tolerant parse for "[x,y,z]" or "x,y,z"
             if s.startswith("[") and s.endswith("]"):
                 s = s[1:-1]
             # support "x,y,z" and "x y z"
             parts = [p.strip() for p in (s.split(",") if "," in s else s.split())]
             if len(parts) == 3:
                 return _to_vec3(parts)
-        return default
+        return default
@@
-    position = _coerce_vec(position, default=position)
-    rotation = _coerce_vec(rotation, default=rotation)
-    scale = _coerce_vec(scale, default=scale)
+    position = _coerce_vec(position, default=None)
+    rotation = _coerce_vec(rotation, default=None)
+    scale = _coerce_vec(scale, default=None)

Also applies to: 108-110


150-177: Minor: normalize booleans to actual bools and prune empties before send.

You already coerce booleans; ensure only true bools cross the wire and remove empty strings to reduce C# side ambiguity. Non-blocking.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1bdbeb8 and dbd5cf6.

📒 Files selected for processing (7)
  • MCPForUnity/Editor/Tools/JsonUtil.cs (1 hunks)
  • MCPForUnity/Editor/Tools/ManageAsset.cs (1 hunks)
  • MCPForUnity/Editor/Tools/ManageGameObject.cs (1 hunks)
  • MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py (2 hunks)
  • MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (4 hunks)
  • TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs (1 hunks)
  • TestProjects/UnityMCPTests/Packages/manifest.json (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • MCPForUnity/Editor/Tools/ManageAsset.cs
  • MCPForUnity/Editor/Tools/ManageGameObject.cs
  • MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py
  • TestProjects/UnityMCPTests/Packages/manifest.json
🧰 Additional context used
🧬 Code graph analysis (3)
MCPForUnity/Editor/Tools/JsonUtil.cs (1)
MCPForUnity/Editor/Tools/ManageGameObject.cs (2)
  • UnityEngine (2165-2262)
  • Type (2269-2283)
MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (1)
tests/test_helpers.py (1)
  • info (3-4)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs (2)
MCPForUnity/Editor/Tools/ManageGameObject.cs (4)
  • UnityEngine (2165-2262)
  • HandleCommand (42-190)
  • Color (2112-2125)
  • GameObject (1272-1295)
MCPForUnity/Editor/Tools/ManageAsset.cs (3)
  • CreateFolder (283-336)
  • HandleCommand (46-114)
  • DeleteAsset (498-526)

Comment on lines +118 to +124
# Coerce 'component_properties' from JSON string to dict for client compatibility
if isinstance(component_properties, str):
try:
component_properties = json.loads(component_properties)
ctx.info("manage_gameobject: coerced component_properties from JSON string to dict")
except json.JSONDecodeError as e:
return {"success": False, "message": f"Invalid JSON in component_properties: {e}"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Also validate parsed type is a dict.

Parsing a valid JSON array/string would slip through; ensure the result is a dict before proceeding.

 if isinstance(component_properties, str):
     try:
         component_properties = json.loads(component_properties)
         ctx.info("manage_gameobject: coerced component_properties from JSON string to dict")
     except json.JSONDecodeError as e:
         return {"success": False, "message": f"Invalid JSON in component_properties: {e}"}
+    if component_properties is not None and not isinstance(component_properties, dict):
+        return {"success": False, "message": "component_properties must be a JSON object (dict)."}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Coerce 'component_properties' from JSON string to dict for client compatibility
if isinstance(component_properties, str):
try:
component_properties = json.loads(component_properties)
ctx.info("manage_gameobject: coerced component_properties from JSON string to dict")
except json.JSONDecodeError as e:
return {"success": False, "message": f"Invalid JSON in component_properties: {e}"}
# Coerce 'component_properties' from JSON string to dict for client compatibility
if isinstance(component_properties, str):
try:
component_properties = json.loads(component_properties)
ctx.info("manage_gameobject: coerced component_properties from JSON string to dict")
except json.JSONDecodeError as e:
return {"success": False, "message": f"Invalid JSON in component_properties: {e}"}
if component_properties is not None and not isinstance(component_properties, dict):
return {"success": False, "message": "component_properties must be a JSON object (dict)."}
🤖 Prompt for AI Agents
In MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py around lines 118
to 124, the code JSON-decodes component_properties but doesn't verify the
decoded type; after json.loads succeeds, check that component_properties is a
dict (isinstance(component_properties, dict)) and if it is not, return a failure
response (e.g., {"success": False, "message": "component_properties must be a
JSON object/dict"}) instead of proceeding; keep the existing JSONDecodeError
handling and log unchanged but add a distinct log/return for non-dict parsed
values.

@dsarno dsarno merged commit a0287af into CoplayDev:main Oct 24, 2025
1 check passed
@dsarno dsarno deleted the tdd/material-management-tests branch October 24, 2025 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant