fix: When setting Color/Rect properties using manage_components, both array and object formats are supported#1035
Open
Misaka-Mikoto-Tech wants to merge 7 commits intoCoplayDev:betafrom
Open
Conversation
…y required to use the object format rather than an array.
Contributor
Reviewer's GuideUpdates manage_components value handling to parse JSON string payloads for Unity struct-like properties and documents the expected object vs array formats in SKILL.md, with new integration tests covering Color/Rect and related Unity struct cases. Sequence diagram for manage_components value normalization and forwardingsequenceDiagram
actor MCPClient
participant Server as ServerTools
participant Manage as manage_components
participant Parser as parse_json_payload
participant Unity as UnityEngine
MCPClient->>Server: Request manage_components
Server->>Manage: manage_components(action, target, component_type, property, value)
Manage->>Manage: validate action/target/component_type/property
Manage->>Manage: validate props (props_error)
alt props_error exists
Manage-->>Server: {success: False, message: props_error}
Server-->>MCPClient: Error response
else no props_error
Manage->>Manage: check value is "[object Object]" or "undefined"
alt invalid sentinel string
Manage-->>Server: {success: False, message: invalid input}
Server-->>MCPClient: Error response
else valid value
Manage->>Parser: parse_json_payload(value)
Parser-->>Manage: normalized_value (object/array/native)
Manage->>Unity: call tool with normalized_value
Unity-->>Manage: execution result
Manage-->>Server: {success: True/False, message, data}
Server-->>MCPClient: Final response
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR enables JSON array parsing for property values in the MCP server's Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Server as MCP Server<br/>(manage_components)
participant Converter as PropertyConversion<br/>UnityTypeConverters
participant Unity as Unity Engine
Client->>Server: set_property value="[1.0, 2.0, 3.0]"<br/>(JSON array string)
Server->>Server: parse_json_payload(value)<br/>→ [1.0, 2.0, 3.0]
Server->>Unity: Send params with<br/>parsed list value
Unity->>Converter: ReadJson with JArray
Converter->>Converter: Extract r, g, b<br/>(alpha defaults to 1f)
Converter->>Unity: Return Color(r, g, b, 1f)
Client->>Server: set_property value="Player One"<br/>(plain string)
Server->>Server: parse_json_payload(value)<br/>→ "Player One" (unchanged)
Server->>Unity: Send params with<br/>string value
Unity->>Converter: ReadJson with JValue string
Converter->>Unity: Return string as-is
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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 left some high level feedback:
- The description says Color/Rect must use the object format, but SKILL.md language only says to "prefer" object form; consider tightening the docs to clearly reflect whether array input is still supported or now rejected.
- The unconditional call to parse_json_payload on any string value may change behavior for plain string properties; ensure parse_json_payload is conservative (e.g., only parses when the string is valid JSON for a structured type) so simple string values are not unintentionally transformed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The description says Color/Rect must use the object format, but SKILL.md language only says to "prefer" object form; consider tightening the docs to clearly reflect whether array input is still supported or now rejected.
- The unconditional call to parse_json_payload on any string value may change behavior for plain string properties; ensure parse_json_payload is conservative (e.g., only parses when the string is valid JSON for a structured type) so simple string values are not unintentionally transformed.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Server/tests/integration/test_manage_components.py (1)
193-261: Minor: Test uses mismatched component types for some properties.The parametrized test uses
component_type="Transform"for all cases, butcolorbelongs to renderers/lights andpixelRectbelongs to cameras. While this doesn't affect the test's purpose (verifying JSON parsing on the Python side), it may cause confusion or fail if the mock is ever replaced with a real Unity call.Consider using appropriate component types for realism:
Suggested improvement
`@pytest.mark.parametrize`( - ("property_name", "raw_value", "expected_value"), + ("component_type", "property_name", "raw_value", "expected_value"), [ ( + "Transform", "position", "[1.0, 2.0, 3.0]", [1.0, 2.0, 3.0], ), ( + "Transform", "localScale", '{"x": 2.0, "y": 3.0, "z": 4.0}', {"x": 2.0, "y": 3.0, "z": 4.0}, ), ( + "Transform", "rotation", '{"x": 0.0, "y": 0.0, "z": 0.0, "w": 1.0}', {"x": 0.0, "y": 0.0, "z": 0.0, "w": 1.0}, ), ( + "Light", "color", '{"r": 1.0, "g": 1.0, "b": 0.0, "a": 1.0}', {"r": 1.0, "g": 1.0, "b": 0.0, "a": 1.0}, ), ( + "Camera", "pixelRect", '{"x": 10.0, "y": 20.0, "width": 1920.0, "height": 1080.0}', {"x": 10.0, "y": 20.0, "width": 1920.0, "height": 1080.0}, ), ], ) async def test_manage_components_set_property_single_json_value_for_unity_structs( monkeypatch, + component_type, property_name, raw_value, expected_value, ): # ... update component_type=component_type in the call🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/tests/integration/test_manage_components.py` around lines 193 - 261, The test test_manage_components_set_property_single_json_value_for_unity_structs uses component_type="Transform" for all parameterized cases which is misleading (color and pixelRect belong to different components); update the manage_components invocation in that test to pass realistic component_type values per case (e.g., use a renderer or Light for "color" and a Camera for "pixelRect") so the parameterization matches actual Unity component semantics while keeping the same assertions; locate the call to manage_comp_mod.manage_components in the test and change the component_type argument based on the property_name parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Server/tests/integration/test_manage_components.py`:
- Around line 193-261: The test
test_manage_components_set_property_single_json_value_for_unity_structs uses
component_type="Transform" for all parameterized cases which is misleading
(color and pixelRect belong to different components); update the
manage_components invocation in that test to pass realistic component_type
values per case (e.g., use a renderer or Light for "color" and a Camera for
"pixelRect") so the parameterization matches actual Unity component semantics
while keeping the same assertions; locate the call to
manage_comp_mod.manage_components in the test and change the component_type
argument based on the property_name parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb527c16-6a31-494b-aa13-c107dc496c75
📒 Files selected for processing (3)
Server/src/services/tools/manage_components.pyServer/tests/integration/test_manage_components.pyunity-mcp-skill/SKILL.md
Misaka-Mikoto-Tech
changed the title
Color/Rect properties via manage_componentsColor/Rect properties via manage_components
Misaka-Mikoto-Tech
changed the title
Color/Rect properties via manage_componentsColor/Rect properties via manage_components
Author
|
I checked both points and agree the docs should be more explicit. I’ve updated SKILL.md so the wording is tighter on the manage_components path: Vector2 / Vector3 / Vector4 / Quaternion: array or object forms are acceptable On parse_json_payload: the current implementation is intentionally conservative. It only attempts parsing for strings that already look like structured/scalar JSON ({...}, [...], true, false, null, numeric literals), and otherwise returns the original string unchanged. That means ordinary plain string properties are preserved as strings, while JSON-stringified structured values from MCP clients can still be recovered before reaching Unity. I also added regression coverage around the manage_components JSON-string path for representative Unity struct types so this behavior is easier to verify going forward. |
…or `Color` when used within `manage_components`.
…lain_string_value_stays_string`
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Server/tests/integration/test_manage_components.py (1)
223-291: Add the missing object-form Vector3 case to match the stated contract.Line 263 says Vector3 accepts array or object JSON, but only array form is currently tested.
Suggested test matrix update
`@pytest.mark.parametrize`( ("property_name", "raw_value", "expected_value"), [ ( "position", "[1.0, 2.0, 3.0]", [1.0, 2.0, 3.0], ), + ( + "position", + '{"x": 1.0, "y": 2.0, "z": 3.0}', + {"x": 1.0, "y": 2.0, "z": 3.0}, + ), ( "localScale", '{"x": 2.0, "y": 3.0, "z": 4.0}', {"x": 2.0, "y": 3.0, "z": 4.0}, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/tests/integration/test_manage_components.py` around lines 223 - 291, The test test_manage_components_set_property_single_json_value_for_unity_structs is missing the object-form Vector3 case: add another parameter tuple for ("position", '{"x": 1.0, "y": 2.0, "z": 3.0}', [1.0, 2.0, 3.0]) to the parametrize list so manage_components is validated for both array and object JSON forms for Vector3; update the param list in the test function to include this new case (refer to the test name test_manage_components_set_property_single_json_value_for_unity_structs and the "position" property).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Server/tests/integration/test_manage_components.py`:
- Around line 223-291: The test
test_manage_components_set_property_single_json_value_for_unity_structs is
missing the object-form Vector3 case: add another parameter tuple for
("position", '{"x": 1.0, "y": 2.0, "z": 3.0}', [1.0, 2.0, 3.0]) to the
parametrize list so manage_components is validated for both array and object
JSON forms for Vector3; update the param list in the test function to include
this new case (refer to the test name
test_manage_components_set_property_single_json_value_for_unity_structs and the
"position" property).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2396ce5-0b64-43c9-bc12-8da748b97cc1
📒 Files selected for processing (1)
Server/tests/integration/test_manage_components.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs (1)
116-131: Consider defensive checks for non-numeric array elements.The direct
(float)arr[n]casts will throwInvalidCastExceptionif the JSON array contains non-numeric values (e.g.,[1.0, "red", 0.5]). While the current exception is informative enough for debugging, catching and re-throwing asJsonSerializationExceptionwith a clearer message would provide better diagnostics consistent with the error at line 124.This is a minor robustness concern since malformed input is already an error path.
♻️ Optional: wrap casts for clearer error messaging
public override Color ReadJson(JsonReader reader, Type objectType, Color existingValue, bool hasExistingValue, JsonSerializer serializer) { JToken token = JToken.Load(reader); if (token is JArray arr && arr.Count >= 3) { - float alpha = arr.Count >= 4 ? (float)arr[3] : 1f; - return new Color((float)arr[0], (float)arr[1], (float)arr[2], alpha); + try + { + float alpha = arr.Count >= 4 ? (float)arr[3] : 1f; + return new Color((float)arr[0], (float)arr[1], (float)arr[2], alpha); + } + catch (Exception ex) when (ex is InvalidCastException or FormatException) + { + throw new JsonSerializationException($"Cannot deserialize Color: array elements must be numeric. Array: '{token}'", ex); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs` around lines 116 - 131, The Color deserialization currently casts JArray elements directly (e.g., (float)arr[0]) which can throw InvalidCastException for non-numeric JSON; update the UnityTypeConverters Color deserialization block to validate/convert each array element safely: for arr[0..2] (and optional arr[3]) check token.Type (or use TryGetValue/Convert.ToSingle with try/catch) and if conversion fails throw a JsonSerializationException that includes the offending token and a clear message; ensure the same defensive conversion is applied to the JObject path (jo["r"], jo["g"], jo["b"], jo["a"]) so all numeric parsing errors surface as JsonSerializationException with contextual info.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/PropertyConversion_UnityStructArraySupport_Tests.cs (1)
12-46: Consider adding negative test cases for robustness.The current tests cover happy paths. Adding tests for error conditions would strengthen coverage and document expected failure behavior:
🧪 Suggested error case tests
[Test] public void ConvertToType_ColorArrayTooFewElements_Throws() { Assert.Throws<Newtonsoft.Json.JsonSerializationException>(() => PropertyConversion.ConvertToType( JArray.Parse("[1.0, 0.5]"), // Only 2 elements typeof(Color) ) ); } [Test] public void ConvertToType_RectArrayTooFewElements_Throws() { Assert.Throws<Newtonsoft.Json.JsonSerializationException>(() => PropertyConversion.ConvertToType( JArray.Parse("[10.0, 20.0, 30.0]"), // Only 3 elements typeof(Rect) ) ); } [Test] public void ConvertToType_ColorObject_StillWorks() { // Verify backward compatibility with object format var result = (Color)PropertyConversion.ConvertToType( JObject.Parse("{\"r\": 1.0, \"g\": 0.5, \"b\": 0.25, \"a\": 0.75}"), typeof(Color) ); Assert.AreEqual(new Color(1.0f, 0.5f, 0.25f, 0.75f), result); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/PropertyConversion_UnityStructArraySupport_Tests.cs` around lines 12 - 46, Add negative and compatibility tests for PropertyConversion.ConvertToType: add a test named ConvertToType_ColorArrayTooFewElements_Throws that calls PropertyConversion.ConvertToType with JArray.Parse("[1.0, 0.5]") and asserts it throws Newtonsoft.Json.JsonSerializationException; add ConvertToType_RectArrayTooFewElements_Throws that calls PropertyConversion.ConvertToType with JArray.Parse("[10.0, 20.0, 30.0]") and asserts the same exception for type Rect; and add ConvertToType_ColorObject_StillWorks that passes a JObject with r/g/b/a keys to PropertyConversion.ConvertToType and asserts it returns the expected Color—place these tests in PropertyConversion_UnityStructArraySupport_Tests alongside the existing tests to cover failure and object-format compatibility.Server/tests/integration/test_manage_components.py (1)
215-247: Consider adding edge case tests for scalar JSON strings.The test correctly verifies plain strings pass through unchanged. However, based on the
parse_json_payloadimplementation inServer/src/services/tools/utils.py(lines 29-62), strings like"true","false","null", and numeric strings like"42"will be coerced to their JSON primitive types.Consider adding test cases to document this behavior and guard against regressions:
🧪 Suggested additional test cases
`@pytest.mark.asyncio` `@pytest.mark.parametrize`( ("raw_value", "expected_value", "expected_type"), [ ("true", True, bool), ("false", False, bool), ("null", None, type(None)), ("42", 42, int), ("3.14", 3.14, float), ], ) async def test_manage_components_set_property_scalar_json_coercion( monkeypatch, raw_value, expected_value, expected_type, ): """Test scalar JSON strings are coerced to native Python types. This documents intentional behavior: strings that look like JSON scalars are parsed. If a literal string "true" is needed, upstream must quote it. """ captured = {} async def fake_send(cmd, params, **kwargs): captured["params"] = params return {"success": True, "data": {"instanceID": 12345}} monkeypatch.setattr( manage_comp_mod, "async_send_command_with_retry", fake_send, ) resp = await manage_comp_mod.manage_components( ctx=DummyContext(), action="set_property", target="TestObject", component_type="TestComponent", property="scalarProp", value=raw_value, ) assert resp.get("success") is True assert captured["params"]["value"] == expected_value if expected_value is not None: assert isinstance(captured["params"]["value"], expected_type)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/tests/integration/test_manage_components.py` around lines 215 - 247, Add parametric tests alongside test_manage_components_set_property_single_plain_string_value_stays_string to cover scalar JSON-string coercion by parse_json_payload: create a parametrize test (e.g., test_manage_components_set_property_scalar_json_coercion) that monkeypatches manage_components' async_send_command_with_retry (same fake_send pattern), calls manage_components with value set to raw_value values "true","false","null","42","3.14", and asserts resp success plus that captured["params"]["value"] equals the expected_value and has the expected_type where applicable; this ensures parse_json_payload's behavior (function parse_json_payload) is documented and guarded against regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs`:
- Around line 116-131: The Color deserialization currently casts JArray elements
directly (e.g., (float)arr[0]) which can throw InvalidCastException for
non-numeric JSON; update the UnityTypeConverters Color deserialization block to
validate/convert each array element safely: for arr[0..2] (and optional arr[3])
check token.Type (or use TryGetValue/Convert.ToSingle with try/catch) and if
conversion fails throw a JsonSerializationException that includes the offending
token and a clear message; ensure the same defensive conversion is applied to
the JObject path (jo["r"], jo["g"], jo["b"], jo["a"]) so all numeric parsing
errors surface as JsonSerializationException with contextual info.
In `@Server/tests/integration/test_manage_components.py`:
- Around line 215-247: Add parametric tests alongside
test_manage_components_set_property_single_plain_string_value_stays_string to
cover scalar JSON-string coercion by parse_json_payload: create a parametrize
test (e.g., test_manage_components_set_property_scalar_json_coercion) that
monkeypatches manage_components' async_send_command_with_retry (same fake_send
pattern), calls manage_components with value set to raw_value values
"true","false","null","42","3.14", and asserts resp success plus that
captured["params"]["value"] equals the expected_value and has the expected_type
where applicable; this ensures parse_json_payload's behavior (function
parse_json_payload) is documented and guarded against regressions.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/PropertyConversion_UnityStructArraySupport_Tests.cs`:
- Around line 12-46: Add negative and compatibility tests for
PropertyConversion.ConvertToType: add a test named
ConvertToType_ColorArrayTooFewElements_Throws that calls
PropertyConversion.ConvertToType with JArray.Parse("[1.0, 0.5]") and asserts it
throws Newtonsoft.Json.JsonSerializationException; add
ConvertToType_RectArrayTooFewElements_Throws that calls
PropertyConversion.ConvertToType with JArray.Parse("[10.0, 20.0, 30.0]") and
asserts the same exception for type Rect; and add
ConvertToType_ColorObject_StillWorks that passes a JObject with r/g/b/a keys to
PropertyConversion.ConvertToType and asserts it returns the expected Color—place
these tests in PropertyConversion_UnityStructArraySupport_Tests alongside the
existing tests to cover failure and object-format compatibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45460602-1d40-4da7-bd6d-4837efe738d7
📒 Files selected for processing (4)
MCPForUnity/Runtime/Serialization/UnityTypeConverters.csServer/tests/integration/test_manage_components.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/PropertyConversion_UnityStructArraySupport_Tests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/PropertyConversion_UnityStructArraySupport_Tests.cs.meta
✅ Files skipped from review due to trivial changes (1)
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/PropertyConversion_UnityStructArraySupport_Tests.cs.meta
Author
|
I used Codex to initiate multiple sessions; the AI sometimes passes |
Misaka-Mikoto-Tech
changed the title
Color/Rect properties via manage_componentsColor/Rect properties using manage_components, both array and object formats are supported
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
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.
Description
When setting Color/Rect properties using manage_components, both array and object formats are supported.
Type of Change
Save your change type
Changes Made
Updated the ### Colors section of SKILL.md to clarify that when manipulating
Color/Rect-type attributes viamanage_components, an object format is required rather than an array.Testing/Screenshots/Recordings
Before
After
Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdRelated Issues
Additional Notes
Summary by Sourcery
Normalize and validate JSON string property values passed to manage_components and document the expected formats for Unity struct-like properties.
Bug Fixes:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Improvements