Skip to content

Commit a0287af

Browse files
authored
Harden MCP tool parameter handling + add material workflow tests (TDD) (#343)
* Add TDD tests for MCP material management issues - 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. * Refine TDD tests to focus on actual MCP tool parameter parsing issues - 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. * Fix port discovery protocol mismatch - 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 * Resolve merge: unify manage_gameobject param coercion and schema widening * Tests: add MaterialParameterToolTests; merge port probe fix; widen tool schemas for JSON-string params * refactor: extract JSON coercion helper; docs + exception narrowing\nfix: fail fast on bad component_properties JSON\ntest: unify setup, avoid test-to-test calls\nchore: use relative MCP package path * chore(tests): track MCPToolParameterTests.cs.meta to keep GUID stable * test: decouple MaterialParameterToolTests with helpers (no inter-test calls)
1 parent 15bf793 commit a0287af

File tree

15 files changed

+546
-20
lines changed

15 files changed

+546
-20
lines changed
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
using Newtonsoft.Json.Linq;
2+
using UnityEngine;
3+
4+
namespace MCPForUnity.Editor.Tools
5+
{
6+
internal static class JsonUtil
7+
{
8+
/// <summary>
9+
/// If @params[paramName] is a JSON string, parse it to a JObject in-place.
10+
/// Logs a warning on parse failure and leaves the original value.
11+
/// </summary>
12+
internal static void CoerceJsonStringParameter(JObject @params, string paramName)
13+
{
14+
if (@params == null || string.IsNullOrEmpty(paramName)) return;
15+
var token = @params[paramName];
16+
if (token != null && token.Type == JTokenType.String)
17+
{
18+
try
19+
{
20+
var parsed = JObject.Parse(token.ToString());
21+
@params[paramName] = parsed;
22+
}
23+
catch (Newtonsoft.Json.JsonReaderException e)
24+
{
25+
Debug.LogWarning($"[MCP] Could not parse '{paramName}' JSON string: {e.Message}");
26+
}
27+
}
28+
}
29+
}
30+
}
31+
32+

MCPForUnity/Editor/Tools/ManageAsset.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,9 @@ public static object HandleCommand(JObject @params)
6363
// Common parameters
6464
string path = @params["path"]?.ToString();
6565

66+
// Coerce string JSON to JObject for 'properties' if provided as a JSON string
67+
JsonUtil.CoerceJsonStringParameter(@params, "properties");
68+
6669
try
6770
{
6871
switch (action)

MCPForUnity/Editor/Tools/ManageGameObject.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,9 @@ public static object HandleCommand(JObject @params)
6666
bool includeNonPublicSerialized = @params["includeNonPublicSerialized"]?.ToObject<bool>() ?? true; // Default to true
6767
// --- End add parameter ---
6868

69+
// Coerce string JSON to JObject for 'componentProperties' if provided as a JSON string
70+
JsonUtil.CoerceJsonStringParameter(@params, "componentProperties");
71+
6972
// --- Prefab Redirection Check ---
7073
string targetPath =
7174
targetToken?.Type == JTokenType.String ? targetToken.ToString() : null;

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
Defines the manage_asset tool for interacting with Unity assets.
33
"""
44
import asyncio
5+
import json
56
from typing import Annotated, Any, Literal
67

78
from fastmcp import Context
@@ -33,6 +34,14 @@ async def manage_asset(
3334
page_number: Annotated[int | float | str, "Page number for pagination"] | None = None
3435
) -> dict[str, Any]:
3536
ctx.info(f"Processing manage_asset: {action}")
37+
# Coerce 'properties' from JSON string to dict for client compatibility
38+
if isinstance(properties, str):
39+
try:
40+
properties = json.loads(properties)
41+
ctx.info("manage_asset: coerced properties from JSON string to dict")
42+
except json.JSONDecodeError as e:
43+
ctx.warn(f"manage_asset: failed to parse properties JSON string: {e}")
44+
# Leave properties as-is; Unity side may handle defaults
3645
# Ensure properties is a dict if None
3746
if properties is None:
3847
properties = {}

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

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import json
12
from typing import Annotated, Any, Literal
23

34
from fastmcp import Context
@@ -42,8 +43,9 @@ def manage_gameobject(
4243
layer: Annotated[str, "Layer name"] | None = None,
4344
components_to_remove: Annotated[list[str],
4445
"List of component names to remove"] | None = None,
45-
component_properties: Annotated[dict[str, dict[str, Any]],
46+
component_properties: Annotated[dict[str, dict[str, Any]] | str,
4647
"""Dictionary of component names to their properties to set. For example:
48+
Can also be provided as a JSON string representation of the dict.
4749
`{"MyScript": {"otherObject": {"find": "Player", "method": "by_name"}}}` assigns GameObject
4850
`{"MyScript": {"playerHealth": {"find": "Player", "component": "HealthComponent"}}}` assigns Component
4951
Example set nested property:
@@ -65,7 +67,7 @@ def manage_gameobject(
6567
"Controls whether serialization of private [SerializeField] fields is included (accepts true/false or 'true'/'false')"] | None = None,
6668
) -> dict[str, Any]:
6769
ctx.info(f"Processing manage_gameobject: {action}")
68-
70+
6971
# Coercers to tolerate stringified booleans and vectors
7072
def _coerce_bool(value, default=None):
7173
if value is None:
@@ -113,6 +115,13 @@ def _to_vec3(parts):
113115
search_inactive = _coerce_bool(search_inactive)
114116
includeNonPublicSerialized = _coerce_bool(includeNonPublicSerialized)
115117

118+
# Coerce 'component_properties' from JSON string to dict for client compatibility
119+
if isinstance(component_properties, str):
120+
try:
121+
component_properties = json.loads(component_properties)
122+
ctx.info("manage_gameobject: coerced component_properties from JSON string to dict")
123+
except json.JSONDecodeError as e:
124+
return {"success": False, "message": f"Invalid JSON in component_properties: {e}"}
116125
try:
117126
# Map tag to search_term when search_method is by_tag for backward compatibility
118127
if action == "find" and search_method == "by_tag" and tag is not None and search_term is None:

MCPForUnity/UnityMcpServer~/src/uv.lock

Lines changed: 88 additions & 11 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

TestProjects/UnityMCPTests/Assets/Editor.meta renamed to TestProjects/UnityMCPTests/Assets/Materials.meta

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,12 @@ public void DoesNotAddEnvOrDisabled_ForTrae()
134134
var configPath = Path.Combine(_tempRoot, "trae.json");
135135
WriteInitialConfig(configPath, isVSCode: false, command: _fakeUvPath, directory: "/old/path");
136136

137-
var client = new McpClient { name = "Trae", mcpType = McpTypes.Trae };
137+
if (!Enum.TryParse<McpTypes>("Trae", out var traeValue))
138+
{
139+
Assert.Ignore("McpTypes.Trae not available in this package version; skipping test.");
140+
}
141+
142+
var client = new McpClient { name = "Trae", mcpType = traeValue };
138143
InvokeWriteToConfig(configPath, client);
139144

140145
var root = JObject.Parse(File.ReadAllText(configPath));

0 commit comments

Comments
 (0)