fix: remove vestigial find/component params from manage_gameobject#693
Open
HivemindMinion wants to merge 2 commits intoCoplayDev:betafrom
Open
fix: remove vestigial find/component params from manage_gameobject#693HivemindMinion wants to merge 2 commits intoCoplayDev:betafrom
HivemindMinion wants to merge 2 commits intoCoplayDev:betafrom
Conversation
The manage_gameobject tool retained parameters (search_term, find_all, search_in_children, search_inactive, component_name, page_size, cursor, max_components, include_properties, includeNonPublicSerialized) from when it was a monolith tool. These params now belong to the separate find_gameobjects tool, manage_components tool, and gameobject_components resource. Their presence on manage_gameobject caused LLMs to hallucinate non-existent actions (e.g. action="find", action="get_components"), leading to ~46 failed tool calls in a single session. Also tightens search_method Literal to only values relevant for target resolution (by_id, by_name, by_path) and improves the tool description to explicitly cross-reference the correct tools and resources. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideRefactors the manage_gameobject MCP tool to strictly handle only GameObject CRUD/duplication, removing vestigial find/component/paging parameters, tightening search semantics, updating the tool description, and adjusting tests to assert the slimmer signature. Sequence diagram for correct tool selection for GameObject operationssequenceDiagram
actor LLM
participant ToolRouter
participant manage_gameobject
participant find_gameobjects
participant manage_components
participant gameobject_components_resource
LLM->>ToolRouter: Request to CREATE/MODIFY/DELETE/DUPLICATE/MOVE GameObject
ToolRouter->>manage_gameobject: manage_gameobject(action, target, search_method, ...)
manage_gameobject-->>ToolRouter: Result(success, message, data)
ToolRouter-->>LLM: Tool response
LLM->>ToolRouter: Request to FIND GameObjects
ToolRouter->>find_gameobjects: find_gameobjects(search_term, search_method, find_all, ...)
find_gameobjects-->>ToolRouter: Result(list_of_gameobjects)
ToolRouter-->>LLM: Tool response
LLM->>ToolRouter: Request to MANAGE COMPONENTS
ToolRouter->>manage_components: manage_components(action, target, component_name, ...)
manage_components-->>ToolRouter: Result(success, message)
ToolRouter-->>LLM: Tool response
LLM->>ToolRouter: Request to READ component data
ToolRouter->>gameobject_components_resource: GET /scene/gameobject/{id}/components
gameobject_components_resource-->>ToolRouter: Component data
ToolRouter-->>LLM: Resource response
Class diagram for updated GameObject management toolsclassDiagram
class ManageGameObjectTool {
+action: Literal_create_modify_delete_duplicate_move_relative
+target: str
+search_method: Literal_by_id_by_name_by_path
+name: str
+tag: str
+parent: str
+world_space: bool
+position: list_float_or_dict_or_str
+rotation: list_float_or_dict_or_str
+scale: list_float_or_dict_or_str
+components_to_add: list_str
+components_to_remove: list_str
+primitive_type: str
+save_as_prefab: bool
+prefab_path: str
+set_active: bool
+layer: int
+component_properties: dict
+new_name: str
+offset: list_float_or_dict_or_str
+copy_position_rotation: bool
+copy_parent: bool
+execute()
}
class FindGameObjectsTool {
+search_term: str
+search_method: Literal_by_id_by_name_by_path_by_tag_by_layer_by_component
+find_all: bool
+search_in_children: bool
+search_inactive: bool
+execute()
}
class ManageComponentsTool {
+action: Literal_add_component_remove_component_set_property
+target: str
+component_name: str
+includeNonPublicSerialized: bool
+page_size: int
+cursor: int
+max_components: int
+include_properties: bool
+execute()
}
class GameObjectComponentsResource {
+uri_template: mcpforunity_scene_gameobject_id_components
+GET(id)
}
ManageGameObjectTool ..> FindGameObjectsTool : delegates_finding_to
ManageGameObjectTool ..> ManageComponentsTool : delegates_component_mutation_to
ManageComponentsTool ..> GameObjectComponentsResource : reads_component_data_from
FindGameObjectsTool ..> GameObjectComponentsResource : may_read_component_data_from
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughmanage_gameobject's public API was simplified: legacy search, component-management, and paging parameters were removed from the function signature and payload assembly; parameter normalization and related validation logic were trimmed and the tool description was expanded to reference dedicated find/component tools. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `Server/src/services/tools/manage_gameobject.py:60-61` </location>
<code_context>
- search_method: Annotated[Literal["by_id", "by_name", "by_path", "by_tag", "by_layer", "by_component"],
- "How to find objects. Used with 'find' and some 'target' lookups."] | None = None,
+ "GameObject identifier by name, path, or instance ID for modify/delete/duplicate actions"] | None = None,
+ search_method: Annotated[Literal["by_id", "by_name", "by_path"],
+ "How to resolve 'target'. Defaults to by_name."] | None = None,
name: Annotated[str,
- "GameObject name for 'create' (initial name) and 'modify' (rename) actions ONLY. For 'find' action, use 'search_term' instead."] | None = None,
</code_context>
<issue_to_address>
**suggestion:** The docstring claims a default of 'by_name' but the parameter itself defaults to None.
Since `search_method` is `Literal[...] | None` with a default of `None`, consider either normalizing `None` to `'by_name'` at the start of this function, or updating the docstring to say that `None` is treated as `'by_name'` by the underlying logic. This keeps the signature and documentation aligned and avoids confusion for readers.
Suggested implementation:
```python
search_method: Annotated[Literal["by_id", "by_name", "by_path"],
"How to resolve 'target'. None (the default) is treated as 'by_name'."] | None = None,
```
This change assumes the underlying implementation already treats `search_method is None` as equivalent to `"by_name"`. If it doesn't, you should also add a normalization near the start of the relevant function body, e.g.:
```python
if search_method is None:
search_method = "by_name"
```
and update any type hints/usages accordingly if `search_method` needs to be non-optional within the function body.
</issue_to_address>
### Comment 2
<location> `Server/src/services/tools/manage_gameobject.py:58-59` </location>
<code_context>
action: Annotated[Literal["create", "modify", "delete", "duplicate",
"move_relative"], "Action to perform on GameObject."] | None = None,
target: Annotated[str,
- "GameObject identifier by name or path for modify/delete/component actions"] | None = None,
- search_method: Annotated[Literal["by_id", "by_name", "by_path", "by_tag", "by_layer", "by_component"],
- "How to find objects. Used with 'find' and some 'target' lookups."] | None = None,
+ "GameObject identifier by name, path, or instance ID for modify/delete/duplicate actions"] | None = None,
+ search_method: Annotated[Literal["by_id", "by_name", "by_path"],
+ "How to resolve 'target'. Defaults to by_name."] | None = None,
</code_context>
<issue_to_address>
**question (bug_risk):** Clarify how instance ID targets interact with `search_method` when it is omitted.
Since `target` can now be an instance ID but `search_method` defaults to `by_name` when omitted, please clarify or adjust how resolution works in that case. If the backend auto-detects IDs by format, documenting that is enough; otherwise, consider normalizing or requiring `search_method` when `target` looks like an instance ID to avoid mis-resolving objects.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1 task
![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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Server/src/services/tools/manage_gameobject.py (1)
183-195:⚠️ Potential issue | 🟡 MinorPre-existing:
prefab_foldercan beNonewhen constructing the default prefab path.Not introduced by this PR, but worth noting: if
save_as_prefab=Trueand noprefab_pathis provided, Line 188 evaluatesf"{prefab_folder}/{params['name']}.prefab"— whenprefab_folderisNone, this produces"None/SomeName.prefab". Previously there may have been a default value for this parameter. Consider adding a fallback:Suggested guard
if "prefabPath" not in params: if "name" not in params or not params["name"]: return {"success": False, "message": "Cannot create default prefab path: 'name' parameter is missing."} - constructed_path = f"{prefab_folder}/{params['name']}.prefab" + folder = prefab_folder or "Assets/Prefabs" + constructed_path = f"{folder}/{params['name']}.prefab"
Expand search_method to include by_tag, by_layer, by_component and clarify that Unity infers the method when omitted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Server/src/services/tools/manage_gameobject.py`:
- Around line 60-64: The search_method Literal in manage_gameobject currently
still lists ["by_id","by_name","by_path","by_tag","by_layer","by_component"]
contrary to the PR intent; update the Annotated Literal for search_method in the
manage_gameobject signature to only include ["by_id","by_name","by_path"]
(matching the tightened schema and moving all find-style searches to
find_gameobjects), and ensure any related docstring text describing inference
(the string after Literal) is adjusted if needed; if the C# side truly still
expects the other tokens, confirm that interoperability before removing,
otherwise remove the extra values from the Literal to prevent presenting
unsupported options to callers/LLMs.
🧹 Nitpick comments (1)
Server/src/services/tools/manage_gameobject.py (1)
186-193: Pre-existing:prefab_foldercan beNone, producing path"None/name.prefab".When
save_as_prefabisTrueandprefab_pathis not provided, Line 191 uses the rawprefab_folderparameter in an f-string. Sinceprefab_folderdefaults toNone, this constructs"None/SomeName.prefab"— a bogus path that would likely fail on the C# side.Not introduced by this PR, but it's in the changed vicinity and worth a follow-up fix.
Suggested guard
if "prefabPath" not in params: if "name" not in params or not params["name"]: return {"success": False, "message": "Cannot create default prefab path: 'name' parameter is missing."} + if prefab_folder is None: + return {"success": False, "message": "Cannot create default prefab path: 'prefab_folder' parameter is missing."} # Use the provided prefab_folder (which has a default) and the name to construct the path constructed_path = f"{prefab_folder}/{params['name']}.prefab"
Comment on lines
+60
to
+64
| search_method: Annotated[ | ||
| Literal["by_id", "by_name", "by_path", "by_tag", "by_layer", "by_component"], | ||
| "How to resolve 'target'. If omitted, Unity infers: instance ID -> by_id, " | ||
| "path (contains '/') -> by_path, otherwise by_name." | ||
| ] | None = None, |
Contributor
There was a problem hiding this comment.
search_method Literal still includes the values the PR intended to remove.
The PR description states the Literal was tightened to ["by_id", "by_name", "by_path"], but the code retains all six values including "by_tag", "by_layer", and "by_component". If manage_gameobject no longer supports find-style searches (those moved to find_gameobjects), keeping these options in the schema contradicts the goal of reducing LLM hallucination of unsupported actions.
Please confirm whether this is intentional (perhaps the C# side still handles these for target resolution) or an oversight.
If these should be removed per the PR objective:
search_method: Annotated[
- Literal["by_id", "by_name", "by_path", "by_tag", "by_layer", "by_component"],
+ Literal["by_id", "by_name", "by_path"],
"How to resolve 'target'. If omitted, Unity infers: instance ID -> by_id, "
"path (contains '/') -> by_path, otherwise by_name."
] | None = None,🤖 Prompt for AI Agents
In `@Server/src/services/tools/manage_gameobject.py` around lines 60 - 64, The
search_method Literal in manage_gameobject currently still lists
["by_id","by_name","by_path","by_tag","by_layer","by_component"] contrary to the
PR intent; update the Annotated Literal for search_method in the
manage_gameobject signature to only include ["by_id","by_name","by_path"]
(matching the tightened schema and moving all find-style searches to
find_gameobjects), and ensure any related docstring text describing inference
(the string after Literal) is adjusted if needed; if the C# side truly still
expects the other tokens, confirm that interoperability before removing,
otherwise remove the extra values from the Literal to prevent presenting
unsupported options to callers/LLMs.
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
3 participants
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
The
manage_gameobjecttool retained 10 parameters from when it was a monolith tool that handled finding, components, and CRUD in one function. These params now belong to separate, dedicated tools and resources:search_term,find_all,search_in_children,search_inactive→find_gameobjectstoolcomponent_name,page_size,cursor,max_components,include_properties,includeNonPublicSerialized→manage_componentstool /gameobject_componentsresourceTheir presence on
manage_gameobjectcaused LLMs to hallucinate non-existent actions (e.g.,action="find",action="get_components"), leading to ~46 failed tool calls in a single session.Type of Change
Changes Made
manage_gameobjectthat belong tofind_gameobjects,manage_components, and thegameobject_componentsresourcesearch_methodLiteral from["by_id", "by_name", "by_path", "by_tag", "by_layer", "by_component"]to["by_id", "by_name", "by_path"](the find-only methods belong onfind_gameobjects)coerce_bool/coerce_intcalls for removed params, unusedjson/coerce_intimports, deadsearch_termvalidation blockTesting/Screenshots/Recordings
All 563 Python tests pass (
uv run --extra dev pytest tests/ -v). No C# changes.Impact analysis: The 4 "dead" params (
find_all,search_in_children,max_components,includeNonPublicSerialized) were never wired to C# handlers — they were passed through but ignored. The other 6 have exact equivalents on dedicated tools/resources. No functionality is lost.Documentation Updates
manage_gameobjecttool still exists with the same name; only its parameter list was trimmed. The tool description was updated inline.Related Issues
Addresses LLM action hallucination caused by bloated tool schemas — the most common failure mode reported in AI-driven Unity sessions.
Additional Notes
The root cause: when an LLM sees parameters like
search_termandcomponent_nameon a tool, it infers matching actions should exist. By removing these vestigial params, the tool schema now accurately represents only whatmanage_gameobjectcan do (create/modify/delete/duplicate/move_relative), pushing LLMs toward the correct dedicated tools.Summary by Sourcery
Simplify the manage_gameobject tool schema to reflect only GameObject CRUD operations and direct callers to dedicated tools/resources for find and component operations.
Enhancements:
Tests:
Summary by CodeRabbit
Refactor
Tests