fix: make component properties reachable on manage_gameobject create - #1298
Open
asavs wants to merge 1 commit into
Open
fix: make component properties reachable on manage_gameobject create#1298asavs wants to merge 1 commit into
asavs wants to merge 1 commit into
Conversation
Fixes CoplayDev#1297. At action:"create", component_properties was accepted and coerced by the C# dispatcher (ManageGameObject.cs) but only ever consumed by the "modify" handler, so it silently did nothing. Meanwhile the shape "create" already reads directly out of each componentsToAdd entry ({typeName, properties}) was rejected before it reached Unity, because the Python schema typed components_to_add as list[str]. - GameObjectComponentHelpers.cs: factor the componentProperties loop + error aggregation out of GameObjectModify.cs into a shared ApplyComponentProperties helper, so both actions apply it identically. - GameObjectCreate.cs: call the new helper after components are added, destroying the partially-created object and returning the error if any property fails to set (matching how component-add failures are handled). - GameObjectModify.cs: switch to the shared helper (behavior-preserving refactor, no functional change on the modify path). - manage_gameobject.py: widen components_to_add to accept {"typeName": ..., "properties": {...}} objects alongside plain strings, matching what GameObjectCreate.cs already reads. - Regenerated website/docs/reference/tools/core/manage_gameobject.md via tools/generate_docs_reference.py for the updated parameter docs. Tested: Server/tests/test_manage_gameobject.py exercises the Python contract end-to-end, including a real fastmcp/pydantic schema validation run of the issue's exact repro payloads (confirmed the pre-fix ValidationError reproduces on the unmodified file, and is gone after). Added TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ ManageGameObjectCreateTests.cs coverage for the C# side, but this was not run against a live Editor.
Contributor
📝 WalkthroughWalkthroughChangesGameObject component properties
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant manage_gameobject
participant GameObjectCreate
participant ApplyComponentProperties
Caller->>manage_gameobject: submit components_to_add and component_properties
manage_gameobject->>manage_gameobject: normalize and validate inputs
manage_gameobject->>GameObjectCreate: forward componentsToAdd and componentProperties
GameObjectCreate->>ApplyComponentProperties: apply properties after adding components
ApplyComponentProperties-->>GameObjectCreate: return modified state or errors
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Server/tests/test_manage_gameobject.py (1)
67-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest name doesn't match what it asserts.
test_tool_description_mentions_component_propertiesonly checks that a non-empty description exists (the comment acknowledges this). Consider renaming to something liketest_tool_registered_with_description, or asserting on the parameter annotation instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Server/tests/test_manage_gameobject.py` around lines 67 - 75, Rename test_tool_description_mentions_component_properties to reflect that it only verifies the manage_gameobject tool is registered with a non-empty description, such as test_tool_registered_with_description; leave the existing assertion behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs`:
- Around line 163-179: Update the componentProperties iteration around
SetComponentPropertiesInternal to explicitly reject entries whose prop.Value is
not a JObject. Add a descriptive validation error to componentErrors for each
unsupported value instead of skipping it, and ensure modified remains false for
rejected entries while preserving the existing success and error handling for
object values.
In `@Server/tests/test_manage_gameobject.py`:
- Around line 113-123: Update
test_invalid_component_properties_rejected_before_send to assert that "params"
is not present in mock_unity, matching the fixture’s captured request structure
and later tests. Keep the existing failure assertion unchanged.
---
Nitpick comments:
In `@Server/tests/test_manage_gameobject.py`:
- Around line 67-75: Rename test_tool_description_mentions_component_properties
to reflect that it only verifies the manage_gameobject tool is registered with a
non-empty description, such as test_tool_registered_with_description; leave the
existing assertion behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b4fa7b1-12fc-48d2-afdf-e0ee96ff6bc3
📒 Files selected for processing (7)
MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.csMCPForUnity/Editor/Tools/GameObjects/GameObjectCreate.csMCPForUnity/Editor/Tools/GameObjects/GameObjectModify.csServer/src/services/tools/manage_gameobject.pyServer/tests/test_manage_gameobject.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cswebsite/docs/reference/tools/core/manage_gameobject.md
Comment on lines
+163
to
+179
| foreach (var prop in componentPropertiesObj.Properties()) | ||
| { | ||
| string compName = prop.Name; | ||
| JObject propertiesToSet = prop.Value as JObject; | ||
| if (propertiesToSet != null) | ||
| { | ||
| var setResult = SetComponentPropertiesInternal(targetGo, compName, propertiesToSet); | ||
| if (setResult != null) | ||
| { | ||
| componentErrors.Add(setResult); | ||
| } | ||
| else | ||
| { | ||
| modified = true; | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Non-object property values are silently ignored.
If a componentProperties entry's value isn't a JSON object (e.g. {"BoxCollider": "size"}), the entry is skipped with no error and modified stays false, so create/modify report success while nothing was applied. The Python layer only validates the outer dict, so this reaches here. Reporting it keeps the "reject unsupported input explicitly" half of the contract.
🐛 Proposed fix
string compName = prop.Name;
JObject propertiesToSet = prop.Value as JObject;
- if (propertiesToSet != null)
- {
- var setResult = SetComponentPropertiesInternal(targetGo, compName, propertiesToSet);
- if (setResult != null)
- {
- componentErrors.Add(setResult);
- }
- else
- {
- modified = true;
- }
- }
+ if (propertiesToSet == null)
+ {
+ componentErrors.Add(new ErrorResponse(
+ $"componentProperties['{compName}'] must be an object of property names to values.",
+ new { errors = new[] { $"Invalid value for '{compName}': {prop.Value.Type}." } }));
+ continue;
+ }
+
+ var setResult = SetComponentPropertiesInternal(targetGo, compName, propertiesToSet);
+ if (setResult != null)
+ {
+ componentErrors.Add(setResult);
+ }
+ else
+ {
+ modified = true;
+ }📝 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
| foreach (var prop in componentPropertiesObj.Properties()) | |
| { | |
| string compName = prop.Name; | |
| JObject propertiesToSet = prop.Value as JObject; | |
| if (propertiesToSet != null) | |
| { | |
| var setResult = SetComponentPropertiesInternal(targetGo, compName, propertiesToSet); | |
| if (setResult != null) | |
| { | |
| componentErrors.Add(setResult); | |
| } | |
| else | |
| { | |
| modified = true; | |
| } | |
| } | |
| } | |
| foreach (var prop in componentPropertiesObj.Properties()) | |
| { | |
| string compName = prop.Name; | |
| JObject propertiesToSet = prop.Value as JObject; | |
| if (propertiesToSet == null) | |
| { | |
| componentErrors.Add(new ErrorResponse( | |
| $"componentProperties['{compName}'] must be an object of property names to values.", | |
| new { errors = new[] { $"Invalid value for '{compName}': {prop.Value.Type}." } })); | |
| continue; | |
| } | |
| var setResult = SetComponentPropertiesInternal(targetGo, compName, propertiesToSet); | |
| if (setResult != null) | |
| { | |
| componentErrors.Add(setResult); | |
| } | |
| else | |
| { | |
| modified = true; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs` around
lines 163 - 179, Update the componentProperties iteration around
SetComponentPropertiesInternal to explicitly reject entries whose prop.Value is
not a JObject. Add a descriptive validation error to componentErrors for each
unsupported value instead of skipping it, and ensure modified remains false for
rejected entries while preserving the existing success and error handling for
object values.
Comment on lines
+113
to
+123
| def test_invalid_component_properties_rejected_before_send(self, mock_unity): | ||
| result = asyncio.run( | ||
| manage_gameobject( | ||
| SimpleNamespace(), | ||
| action="create", | ||
| name="Probe", | ||
| component_properties="not json", | ||
| ) | ||
| ) | ||
| assert result["success"] is False | ||
| assert "component_properties" not in mock_unity |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assertion is vacuous — component_properties is never a key of mock_unity.
The fixture captures unity_instance, tool_name, params, so this passes even if the invalid payload were forwarded. Use the "params" not in mock_unity form the later tests use.
💚 Proposed fix
assert result["success"] is False
- assert "component_properties" not in mock_unity
+ assert "params" not in mock_unity📝 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
| def test_invalid_component_properties_rejected_before_send(self, mock_unity): | |
| result = asyncio.run( | |
| manage_gameobject( | |
| SimpleNamespace(), | |
| action="create", | |
| name="Probe", | |
| component_properties="not json", | |
| ) | |
| ) | |
| assert result["success"] is False | |
| assert "component_properties" not in mock_unity | |
| def test_invalid_component_properties_rejected_before_send(self, mock_unity): | |
| result = asyncio.run( | |
| manage_gameobject( | |
| SimpleNamespace(), | |
| action="create", | |
| name="Probe", | |
| component_properties="not json", | |
| ) | |
| ) | |
| assert result["success"] is False | |
| assert "params" not in mock_unity |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Server/tests/test_manage_gameobject.py` around lines 113 - 123, Update
test_invalid_component_properties_rejected_before_send to assert that "params"
is not present in mock_unity, matching the fixture’s captured request structure
and later tests. Keep the existing failure assertion unchanged.
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.
Fixes #1297
What was broken
manage_gameobjectataction: "create"had no reachable way to set component properties:component_propertieswas accepted by the tool, coerced by the C# dispatcher (ManageGameObject.cs:55-68runs for every action, not justmodify), and then never read anywhere on the create path. The call returned"success": trueand silently did nothing.createdoes read —{"typeName": ..., "properties": {...}}objects nested insidecomponents_to_add(GameObjectCreate.cs:250-253) — was rejected before it ever reached Unity, because the Python schema typedcomponents_to_addaslist[str] | str. Passing an object entry raised a PydanticValidationErrortwo ways (as a bad list item and as a bad bare string).So one argument was implemented but unreachable, and the other was reachable but discarded.
action: "modify"was unaffected.What changed and why
MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.csFactored the
componentPropertiesloop + per-component error aggregation that used to live only inGameObjectModify.csinto a newApplyComponentProperties(GameObject, JObject, out bool modified)helper.GameObjectModifyalready had all the logiccreateneeded; duplicating ~50 lines of error-aggregation code across two files felt worse than the one shared helper, so I pulled it out rather than copy-pasting (the file already duplicates the smallercomponentsToAddloop between create/modify — this block was long enough that duplication seemed like the wrong call here).MCPForUnity/Editor/Tools/GameObjects/GameObjectCreate.csCalls the new helper right after components are added. On failure, destroys the partially-created GameObject and returns the error — matching the existing behavior for a failed
AddComponentInternalcall a few lines above it, so a failed property set doesn't leave a half-configured object behind.MCPForUnity/Editor/Tools/GameObjects/GameObjectModify.csSwitched to the shared helper. Behavior-preserving refactor only — same aggregation, same error shape,
modifiedflips the same way it used to.Server/src/services/tools/manage_gameobject.pyWidened
components_to_add's type tolist[str | dict[str, Any]] | dict[str, Any] | str, with a new_normalize_components_to_add()that validates each dict entry has a stringtypeName(acceptstype_nametoo, for consistency with the snake_case-first Python parameter naming) and an optional dictproperties, matching whatGameObjectCreate.csalready reads. Plain string entries, JSON-string payloads, and a bare (non-list-wrapped) object all keep working, same tolerance pattern as the existingnormalize_string_list/normalize_propertieshelpers inutils.py. Also updated thecomponents_to_add/component_propertiesdocstrings to say both now work oncreate, and regeneratedwebsite/docs/reference/tools/core/manage_gameobject.mdviatools/generate_docs_reference.py(only that file changed).I did not add snake_case acceptance on the C# side (
componentPropertiesvscomponent_properties) — checked, and unlikeManagePrefabs.cs:940this tool's Python layer always sends camelCase over the wire (manage_gameobject.pybuildsparams["componentProperties"]/params["componentsToAdd"]unconditionally), so there's no snake_case form ever arriving in the JObject the C# side would need to check for.Tested
Python (ran, all green):
cd Server && uv sync --extra dev && uv run pytest tests/ -v— 1355 passed, 3 skipped (was 1336 passed, 3 skipped before this branch's new test file).Server/tests/test_manage_gameobject.py(19 new tests) covering:components_to_addaccepting plain strings, object entries, mixed lists, a bare object, JSON-string forms,type_name→typeNamenormalization, and rejection of malformed entries (missingtypeName, non-objectproperties, wrong entry type);component_propertiesforwarding oncreate.call_toolinvocation (not just calling the Python function directly, which would bypass Pydantic validation): built aFastMCPinstance, registeredmanage_gameobject, and called it with{"action": "create", "name": "Probe", "components_to_add": [{"typeName": "BoxCollider", "properties": {"size": [2,2,2]}}]}.git stashof just that file): raisespydantic.ValidationError: 2 validation errors for call[manage_gameobject]— the exact error quoted in the issue.componentsToAddintact as[{'typeName': 'BoxCollider', 'properties': {'size': [2, 2, 2]}}].manage_gameobject.mdchanged, with just the two touched parameter rows different.C# — reviewed, not run. I don't have a live Unity Editor session to run EditMode tests against from here. I added coverage to
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs(a "Component Add and Property Tests" region: property application viacomponent_propertieson a newly-added component, the{typeName, properties}object-entry form, a mixed string+objectcomponents_to_addlist, and the failure/cleanup path whencomponent_propertiestargets a component that was never added) following the file's existing conventions, and hand-tracedGameObjectComponentHelpers.ApplyComponentProperties/ the two call sites for brace/paren balance and control flow, but none of it has been compiled or executed inside the Editor. Please run the EditMode suite before merging.Related, out of scope
While reading
Server/src/cli/commands/gameobject.pyI noticed thegameobject createCLI command has a comment "Add components separately since componentsToAdd doesn't work" (line 180) and works around it by callingmanage_componentsin a follow-up round trip aftercreate. That looks like a related-but-distinct issue scoped to the CLI'screatecommand rather than the MCP tool schema this issue is about, so I left it alone — flagging here in case it's worth its own issue.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
manage_gameobjectguidance for component inputs and property application.