Merged
Conversation
Also updated unity-mcp prefab cli command to include modify command
Contributor
Reviewer's GuideAdds a new headless Sequence diagram for prefab modify CLI delete_child flowsequenceDiagram
actor User
participant CliPrefabModify as PrefabCliModifyCommand
participant ServerManagePrefabsPy as ManagePrefabsPythonTool
participant UnityInstance
participant ManagePrefabsCs as ManagePrefabsEditorTool
participant RemoveChildrenCs as RemoveChildrenHelper
User->>CliPrefabModify: unity_mcp prefab modify --delete-child Child1
CliPrefabModify->>CliPrefabModify: build params with deleteChild list
CliPrefabModify->>ServerManagePrefabsPy: manage_prefabs(action=modify_contents, delete_child=[Child1])
ServerManagePrefabsPy->>ServerManagePrefabsPy: normalize params
ServerManagePrefabsPy->>ServerManagePrefabsPy: map delete_child to deleteChild
ServerManagePrefabsPy->>UnityInstance: send manage_prefabs with deleteChild
UnityInstance->>ManagePrefabsCs: ApplyModificationsToPrefabObject(@params)
ManagePrefabsCs->>ManagePrefabsCs: read deleteChild or delete_child token
ManagePrefabsCs->>RemoveChildrenCs: RemoveChildren(deleteChildToken, targetGo, prefabRoot)
RemoveChildrenCs->>RemoveChildrenCs: normalize deleteChildToken to JArray
RemoveChildrenCs->>RemoveChildrenCs: loop over child paths
RemoveChildrenCs->>RemoveChildrenCs: targetGo.transform.Find(childPath)
RemoveChildrenCs-->>ManagePrefabsCs: removedCount, error
alt removal error
ManagePrefabsCs-->>UnityInstance: ErrorResponse
else successful removal
ManagePrefabsCs->>ManagePrefabsCs: set modified flag when removedCount > 0
ManagePrefabsCs-->>UnityInstance: success response
end
UnityInstance-->>ServerManagePrefabsPy: response
ServerManagePrefabsPy-->>CliPrefabModify: response
CliPrefabModify->>CliPrefabModify: format_output and print_success
CliPrefabModify-->>User: Modified prefab message
Updated class diagram for prefab modification and delete_child handlingclassDiagram
class PrefabCliCommands {
+modify(path, target, position, rotation, scale, name, tag, layer, active, parent, add_component, remove_component, set_property, delete_child, create_child)
-_parse_vector3(value) list~float~
-_parse_property(prop_str) tuple~string,string,Any~
}
class ManagePrefabsPythonTool {
+manage_prefabs(unity_instance, action, prefab_path, target, position, rotation, scale, name, tag, layer, set_active, parent, components_to_add, components_to_remove, create_child, delete_child, component_properties) dict~string,Any~
-normalize_child_params(child, index) tuple~dict~string,Any~,Error~
}
class ManagePrefabsCs {
+ApplyModificationsToPrefabObject(@params, targetGo, prefabRoot) (bool, ErrorResponse)
-CreateSingleChildInPrefab(child, targetGo, prefabRoot) (bool, ErrorResponse)
-RemoveChildren(deleteChildToken, targetGo, prefabRoot) (int, ErrorResponse)
}
class UnityInstance {
+send_with_unity_instance(async_send_command_with_retry, unity_instance, command, params) dict~string,Any~
}
PrefabCliCommands --> ManagePrefabsPythonTool : uses_manage_prefabs
ManagePrefabsPythonTool --> UnityInstance : sends_manage_prefabs_command
UnityInstance --> ManagePrefabsCs : invokes_editor_tool
ManagePrefabsCs --> ManagePrefabsCs : calls_RemoveChildren
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughAdds headless prefab child-deletion support: a CLI Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "Client (unity-mcp CLI)"
participant Server as "Server (manage_prefabs command)"
participant Unity as "Unity Editor (ManagePrefabs.cs)"
participant AssetDB as "AssetDatabase"
CLI->>Server: run `prefab modify` + params (prefabPath, deleteChild, target, ...)
Server->>Server: validate/parse options (_parse_vector3/_parse_property)
Server->>Unity: send request { action: "modify_contents", prefabPath, deleteChild, ... }
Unity->>Unity: ApplyModificationsToPrefabObject(payload)
Unity->>Unity: RemoveChildren(deleteChild) -> for each childPath: transform.Find(childPath)
Unity->>AssetDB: Save/Reload prefab
AssetDB-->>Unity: prefab reloaded
Unity-->>Server: response { success: true/false, error? }
Server-->>CLI: print result / exit code
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
4 tasks
![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:
- In
Server/src/cli/commands/prefab.py,_parse_propertyand themodifysignature useAnybut this module doesn’t appear to importAnyfromtyping, which will raise aNameErrorat runtime; add the appropriate import (e.g.from typing import Any) to fix this.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Server/src/cli/commands/prefab.py`, `_parse_property` and the `modify` signature use `Any` but this module doesn’t appear to import `Any` from `typing`, which will raise a `NameError` at runtime; add the appropriate import (e.g. `from typing import Any`) to fix this.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.
Actionable comments posted: 4
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_prefabs.py (1)
23-44:⚠️ Potential issue | 🟠 MajorSet an explicit MCP tool
groupin the decorator.Line 23 defines the tool with
@mcp_for_unity_tool(...)but omitsgroup, which violates the repository tool-registration rule.As per coding guidelines, MCP tool `group` parameter must be set to one of: `'core'`, `'vfx'`, `'animation'`, `'ui'`, `'scripting_ext'`, `'probuilder'`, or `'testing'`.🔧 Minimal fix
`@mcp_for_unity_tool`( + group="core", description=( "Manages Unity Prefab assets via headless operations (no UI, no prefab stages). " ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/services/tools/manage_prefabs.py` around lines 23 - 44, The mcp_for_unity_tool decorator on the Manage Prefabs tool is missing the required group parameter; update the `@mcp_for_unity_tool`(...) call to include group="<one_of_allowed_values>" (choose from 'core', 'vfx', 'animation', 'ui', 'scripting_ext', 'probuilder', 'testing') so the tool registers correctly; e.g., add group='core' to the decorator arguments alongside description and annotations (refer to the mcp_for_unity_tool decorator and ToolAnnotations(title="Manage Prefabs") in this file).
🧹 Nitpick comments (2)
Server/tests/test_cli.py (1)
728-735: Tighten failure assertions to avoid false positives.At Line 734, Line 786, and Line 794 the tests only assert non-zero exit codes. Please also assert expected validation text so failures are tied to the intended parser/validator branch.
✅ Suggested test hardening
def test_prefab_modify_invalid_create_child_json(self, runner, mock_unity_response): """Test prefab modify with invalid JSON for create-child.""" result = runner.invoke(cli, [ "prefab", "modify", "Assets/Prefabs/Player.prefab", "--create-child", "not valid json" ]) assert result.exit_code != 0 + assert "Invalid JSON for --create-child" in result.output @@ def test_prefab_modify_invalid_vector_non_numeric(self, runner, mock_unity_response): """Test prefab modify rejects non-numeric vector components.""" result = runner.invoke(cli, [ "prefab", "modify", "Assets/Prefabs/Player.prefab", "--position", "1,foo,3" ]) assert result.exit_code != 0 + assert "All components must be numeric" in result.output @@ def test_prefab_modify_invalid_vector_wrong_count(self, runner, mock_unity_response): """Test prefab modify rejects vectors with wrong component count.""" result = runner.invoke(cli, [ "prefab", "modify", "Assets/Prefabs/Player.prefab", "--position", "1,2" ]) assert result.exit_code != 0 + assert "Must be 'x,y,z' format" in result.outputAlso applies to: 780-794
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/tests/test_cli.py` around lines 728 - 735, The test test_prefab_modify_invalid_create_child_json currently only asserts a non-zero exit code, which can give false positives; update this test (and the other two failing cases around the same block) to also assert the specific validation error text from the CLI parser so failures map to the intended branch—after calling runner.invoke(cli, [...]) keep the existing assert result.exit_code != 0 and add an assertion like assert "Invalid JSON" in result.output (or the exact validator message your CLI emits, e.g., "Failed to parse --create-child" or "create-child: invalid JSON") to tightly couple the test to the parser/validator behavior; apply the same pattern to the other prefab modify tests in that group.Server/src/services/tools/manage_prefabs.py (1)
192-193: Add lightweight validation before forwardingdelete_child.At Line 192, any non-
Nonevalue is forwarded directly. A small guard for type/empty values would fail fast with consistent errors and avoid downstream ambiguity.🧩 Suggested validation
- if delete_child is not None: - params["deleteChild"] = delete_child + if delete_child is not None: + if isinstance(delete_child, str): + if not delete_child.strip(): + return {"success": False, "message": "delete_child cannot be empty."} + elif isinstance(delete_child, list): + if not delete_child: + return {"success": False, "message": "delete_child list cannot be empty."} + if any(not isinstance(p, str) or not p.strip() for p in delete_child): + return {"success": False, "message": "delete_child list must contain non-empty strings only."} + else: + return {"success": False, "message": "delete_child must be a string or list of strings."} + params["deleteChild"] = delete_child🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/services/tools/manage_prefabs.py` around lines 192 - 193, The code forwards any non-None delete_child into params without validation; update the block that handles delete_child in manage_prefabs.py to validate before assignment: ensure delete_child is the expected type (e.g., bool) or, if strings are accepted, that it's a non-empty string, otherwise raise a clear ValueError/TypeError or return an error; only set params["deleteChild"] = delete_child when the value passes this lightweight validation so downstream code receives a well-typed value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Server/src/cli/CLI_USAGE_GUIDE.md`:
- Around line 565-566: Add a one-line clarifying note near the `prefab modify`
examples stating that the `--position` option for `prefab modify` intentionally
accepts CSV vectors (e.g., "0,1,2") and is an exception to the guide’s global
"no commas for multi-value options" rule so readers know to use comma-separated
values only for `prefab modify --position`.
In `@Server/src/cli/commands/prefab.py`:
- Around line 270-291: The parser splits comp_prop into component and prop but
doesn't reject empty identifiers, allowing inputs like ".mass=5" or
"Rigidbody.=5"; after the split (component, prop = comp_prop.rsplit(".", 1)) and
before parsing values, validate both component.strip() and prop.strip() are
non-empty and raise a clear ValueError (or argparse.ArgumentTypeError if used as
an arg parser type) when either is empty so invalid `--set-property` entries are
rejected at parse time; update the error message to include the original
comp_prop for easier debugging.
- Around line 365-367: The try/except that parses create_child currently allows
any JSON value; after json.loads(create_child) in the block handling
create_child you must verify the result is a JSON object (Python dict) before
assigning to params["createChild"]; if the parsed value is not a dict, raise or
print a clear usage/error message and abort (same control flow as the existing
JSONDecodeError branch). Update the code around the json.loads(...) call (the
create_child variable handling) to perform isinstance(parsed, dict) and handle
non-object values as an invalid argument.
- Around line 257-260: The except block in _parse_vector3 currently re-raises
click.BadParameter losing the original ValueError context; modify the except to
capture the ValueError (e.g., except ValueError as e:) and re-raise
click.BadParameter(f"All components must be numeric, got: '{value}'") from e so
the original traceback is preserved; reference the _parse_vector3 function and
the variables parts and value when making the change.
---
Outside diff comments:
In `@Server/src/services/tools/manage_prefabs.py`:
- Around line 23-44: The mcp_for_unity_tool decorator on the Manage Prefabs tool
is missing the required group parameter; update the `@mcp_for_unity_tool`(...)
call to include group="<one_of_allowed_values>" (choose from 'core', 'vfx',
'animation', 'ui', 'scripting_ext', 'probuilder', 'testing') so the tool
registers correctly; e.g., add group='core' to the decorator arguments alongside
description and annotations (refer to the mcp_for_unity_tool decorator and
ToolAnnotations(title="Manage Prefabs") in this file).
---
Nitpick comments:
In `@Server/src/services/tools/manage_prefabs.py`:
- Around line 192-193: The code forwards any non-None delete_child into params
without validation; update the block that handles delete_child in
manage_prefabs.py to validate before assignment: ensure delete_child is the
expected type (e.g., bool) or, if strings are accepted, that it's a non-empty
string, otherwise raise a clear ValueError/TypeError or return an error; only
set params["deleteChild"] = delete_child when the value passes this lightweight
validation so downstream code receives a well-typed value.
In `@Server/tests/test_cli.py`:
- Around line 728-735: The test test_prefab_modify_invalid_create_child_json
currently only asserts a non-zero exit code, which can give false positives;
update this test (and the other two failing cases around the same block) to also
assert the specific validation error text from the CLI parser so failures map to
the intended branch—after calling runner.invoke(cli, [...]) keep the existing
assert result.exit_code != 0 and add an assertion like assert "Invalid JSON" in
result.output (or the exact validator message your CLI emits, e.g., "Failed to
parse --create-child" or "create-child: invalid JSON") to tightly couple the
test to the parser/validator behavior; apply the same pattern to the other
prefab modify tests in that group.
🪄 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
Run ID: 4b7b0206-c09e-4624-a370-3e59d1424318
📒 Files selected for processing (10)
MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.csServer/src/cli/CLI_USAGE_GUIDE.mdServer/src/cli/commands/prefab.pyServer/src/services/tools/manage_prefabs.pyServer/tests/test_cli.pyServer/tests/test_manage_prefabs.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsCrudTests.csdocs/guides/CLI_EXAMPLE.mddocs/guides/CLI_USAGE.mdunity-mcp-skill/references/tools-reference.md
Comment on lines
+565
to
+566
| unity-mcp prefab modify "Assets/Prefabs/Player.prefab" --target Weapon --position "0,1,2" | ||
| unity-mcp prefab modify "Assets/Prefabs/Player.prefab" --delete-child Child1 --delete-child "Turret/Barrel" |
Contributor
There was a problem hiding this comment.
Clarify the vector syntax exception for prefab modify.
Line 565 uses CSV ("0,1,2"), which conflicts with this guide’s earlier “no commas for multi-value options” rule. Add a one-line note that prefab modify --position intentionally accepts CSV (if that is expected), so users don’t misapply the global rule.
✏️ Suggested doc tweak
# Modify prefab contents (headless)
+# Note: `prefab modify --position` expects CSV string format (`"x,y,z"`), unlike gameobject multi-value options.
unity-mcp prefab modify "Assets/Prefabs/Player.prefab" --target Weapon --position "0,1,2"📝 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
| unity-mcp prefab modify "Assets/Prefabs/Player.prefab" --target Weapon --position "0,1,2" | |
| unity-mcp prefab modify "Assets/Prefabs/Player.prefab" --delete-child Child1 --delete-child "Turret/Barrel" | |
| # Modify prefab contents (headless) | |
| # Note: `prefab modify --position` expects CSV string format (`"x,y,z"`), unlike gameobject multi-value options. | |
| unity-mcp prefab modify "Assets/Prefabs/Player.prefab" --target Weapon --position "0,1,2" | |
| unity-mcp prefab modify "Assets/Prefabs/Player.prefab" --delete-child Child1 --delete-child "Turret/Barrel" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Server/src/cli/CLI_USAGE_GUIDE.md` around lines 565 - 566, Add a one-line
clarifying note near the `prefab modify` examples stating that the `--position`
option for `prefab modify` intentionally accepts CSV vectors (e.g., "0,1,2") and
is an exception to the guide’s global "no commas for multi-value options" rule
so readers know to use comma-separated values only for `prefab modify
--position`.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds support for deleting child GameObjects from prefabs via the manage_prefabs tool and exposes the new capability through the CLI, with accompanying documentation and test coverage.
Changes:
- Add
delete_childsupport tomanage_prefabs(Python ↔ Unity bridge) and Unity editor implementation. - Introduce
unity-mcp prefab modifyCLI command with options for delete/create child, transforms, component ops, and property setting. - Update docs and add/extend tests across Unity, server tool, and CLI layers.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| unity-mcp-skill/references/tools-reference.md | Documents delete_child usage in manage_prefabs examples. |
| docs/guides/CLI_USAGE.md | Adds prefab modify usage examples and lists modify under prefab commands. |
| docs/guides/CLI_EXAMPLE.md | Adds prefab modify examples (delete child, transforms, set-property). |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsCrudTests.cs | Adds Unity EditMode tests validating child deletion behavior and error handling. |
| Server/tests/test_manage_prefabs.py | Extends server tool tests to cover delete_child signature/forwarding/description. |
| Server/tests/test_cli.py | Adds CLI tests for prefab modify including --delete-child and other options. |
| Server/src/services/tools/manage_prefabs.py | Adds delete_child parameter and forwards it to Unity as deleteChild. |
| Server/src/cli/commands/prefab.py | Implements unity-mcp prefab modify command and parsing helpers. |
| Server/src/cli/CLI_USAGE_GUIDE.md | Documents prefab modify usage examples. |
| MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs | Implements deletion logic for child GameObjects in prefab modification flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1168
to
+1174
| foreach (var childToken in childrenToDelete) | ||
| { | ||
| string childPath = childToken.Type == JTokenType.String ? childToken.ToString() : childToken["name"]?.ToString(); | ||
| if (string.IsNullOrEmpty(childPath)) | ||
| { | ||
| return (removedCount, new ErrorResponse("'delete_child' must be a string or object with 'name' field.")); | ||
| } |
There was a problem hiding this comment.
childToken["name"] will throw at runtime when childToken is a JValue that is not a string (e.g., number/bool/null) because indexing into a non-container token is invalid in Newtonsoft. This can crash the command instead of returning a structured error. Fix by explicitly branching on token type (e.g., JValue string vs JObject) and returning an error for any unsupported token types without attempting ["name"] access.
Comment on lines
+1170
to
+1173
| string childPath = childToken.Type == JTokenType.String ? childToken.ToString() : childToken["name"]?.ToString(); | ||
| if (string.IsNullOrEmpty(childPath)) | ||
| { | ||
| return (removedCount, new ErrorResponse("'delete_child' must be a string or object with 'name' field.")); |
There was a problem hiding this comment.
The error message (and implementation) implies delete_child supports an “object with 'name' field”, but the server-side tool signature/docs describe delete_child as str | list[str] (paths/names only). To avoid confusing API consumers, either (a) remove support for object tokens here and enforce strings only, or (b) update the Python type hints + docs to explicitly allow object items and define the schema.
Suggested change
| string childPath = childToken.Type == JTokenType.String ? childToken.ToString() : childToken["name"]?.ToString(); | |
| if (string.IsNullOrEmpty(childPath)) | |
| { | |
| return (removedCount, new ErrorResponse("'delete_child' must be a string or object with 'name' field.")); | |
| if (childToken.Type != JTokenType.String) | |
| { | |
| return (removedCount, new ErrorResponse("'delete_child' must be a string or array of strings.")); | |
| } | |
| string childPath = childToken.ToString(); | |
| if (string.IsNullOrEmpty(childPath)) | |
| { | |
| return (removedCount, new ErrorResponse("'delete_child' must be a non-empty string or array of non-empty strings.")); |
Server/src/cli/commands/prefab.py
Outdated
Comment on lines
+271
to
+273
|
|
||
| val_str = val_str.strip() | ||
|
|
There was a problem hiding this comment.
These lines include trailing whitespace on otherwise blank lines, which commonly fails linters/formatters and creates noisy diffs. Remove the trailing spaces or run the repo formatter so blank lines are truly empty.
| parsed_value = int(val_str) | ||
| except ValueError: | ||
| parsed_value = val_str | ||
|
|
There was a problem hiding this comment.
These lines include trailing whitespace on otherwise blank lines, which commonly fails linters/formatters and creates noisy diffs. Remove the trailing spaces or run the repo formatter so blank lines are truly empty.
Comment on lines
+279
to
+289
| # Parse numbers | ||
| elif "." in val_str: | ||
| try: | ||
| parsed_value = float(val_str) | ||
| except ValueError: | ||
| parsed_value = val_str | ||
| else: | ||
| try: | ||
| parsed_value = int(val_str) | ||
| except ValueError: | ||
| parsed_value = val_str |
There was a problem hiding this comment.
Numeric parsing is currently keyed off presence of ".", which misclassifies valid numeric inputs like scientific notation (1e-3) as strings. If the intention is to accept common numeric formats from CLI, switch to a more robust parse order (e.g., try int for strict integers, else try float for broader numeric formats) so values like 1e-3 are forwarded as numbers instead of strings.
Suggested change
| # Parse numbers | |
| elif "." in val_str: | |
| try: | |
| parsed_value = float(val_str) | |
| except ValueError: | |
| parsed_value = val_str | |
| else: | |
| try: | |
| parsed_value = int(val_str) | |
| except ValueError: | |
| parsed_value = val_str | |
| else: | |
| # Parse numbers: try int first, then float; fall back to string | |
| parsed_value = val_str | |
| try: | |
| parsed_value = int(val_str) | |
| except ValueError: | |
| try: | |
| parsed_value = float(val_str) | |
| except ValueError: | |
| # Leave as original string if not numeric | |
| parsed_value = val_str |
Comment on lines
+192
to
+193
| if delete_child is not None: | ||
| params["deleteChild"] = delete_child |
There was a problem hiding this comment.
delete_child is forwarded without runtime validation/coercion. Given the Unity side expects string paths (or currently attempts to handle object tokens), passing a list with any non-string item can lead to Unity-side failures. Add a small normalization/validation step here (e.g., if list: ensure every item is a non-empty str, else return a clear error message) to fail fast and keep errors actionable.
Suggested change
| if delete_child is not None: | |
| params["deleteChild"] = delete_child | |
| def normalize_delete_child_paths(delete_child_val: Any) -> tuple[Any | None, str | None]: | |
| """ | |
| Validate and normalize the delete_child parameter. | |
| - Accepts either a single non-empty string path or a list of non-empty string paths. | |
| - Returns (normalized_value, None) on success, or (None, error_message) on failure. | |
| """ | |
| # Single path as string | |
| if isinstance(delete_child_val, str): | |
| if not delete_child_val.strip(): | |
| return None, "delete_child must be a non-empty string path" | |
| return delete_child_val, None | |
| # List of paths | |
| if isinstance(delete_child_val, list): | |
| for i, item in enumerate(delete_child_val): | |
| if not isinstance(item, str): | |
| return None, f"delete_child[{i}] must be a non-empty string path, got {type(item).__name__}" | |
| if not item.strip(): | |
| return None, f"delete_child[{i}] must be a non-empty string path" | |
| # Preserve original list type to avoid changing behavior for valid inputs | |
| return delete_child_val, None | |
| # Unsupported type | |
| return None, f"delete_child must be a string path or list of string paths, got {type(delete_child_val).__name__}" | |
| if delete_child is not None: | |
| normalized_delete_child, delete_child_err = normalize_delete_child_paths(delete_child) | |
| if delete_child_err: | |
| return {"success": False, "message": delete_child_err} | |
| params["deleteChild"] = normalized_delete_child |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs`:
- Around line 1176-1181: Normalize childPath against the prefab root before
calling Transform.Find: detect and remove a leading root-prefixed segment (the
prefab root name or the prefabRoot path segment produced by get_hierarchy) or
any leading '/' so that childPath is relative to targetGo; update the code
around Transform childToRemove = targetGo.transform.Find(childPath) (in the
deleteChild/remove-child logic) to strip the root prefix (e.g., if childPath
starts with $"{prefabRoot}/" or $"{targetGo.name}/" or "/") then call Find with
the trimmed path so root-prefixed paths from get_hierarchy resolve correctly.
- Around line 1170-1173: The code reads childToken["name"] without ensuring
childToken is an object, which throws for JValue/JArray; modify the deleteChild
handling so you first type-check childToken is a JObject (e.g., using "is
JObject") before attempting childToken["name"], and only fall back to treating
it as a string when childToken.Type == JTokenType.String; ensure you still
return the ErrorResponse with the same message when the token is neither a
string nor an object. Update the logic around childToken, childPath,
removedCount and the ErrorResponse return to use this safe type-check so invalid
tokens produce the intended error instead of an exception.
In `@Server/tests/test_cli.py`:
- Around line 790-804: Update the two tests to ensure they fail due to CLI
parsing (_parse_vector3) rather than downstream runtime: after calling
runner.invoke(cli, ...), assert the exit code is non-zero, assert the CLI output
contains the Click validation message (e.g. contains "Invalid value for
'--position'" or the specific error text produced by _parse_vector3), and assert
the mocked downstream call (use the mock used to stand in for the runtime call,
e.g. run_command or mock_unity_response) was not invoked by calling its
assert_not_called(); keep the existing test names
test_prefab_modify_invalid_vector_non_numeric and
test_prefab_modify_invalid_vector_wrong_count and only change their assertions.
🪄 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
Run ID: 47f6d2fb-c733-4c9c-95b9-713e9650855f
📒 Files selected for processing (3)
MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.csServer/src/cli/commands/prefab.pyServer/tests/test_cli.py
🚧 Files skipped from review as they are similar to previous changes (1)
- Server/src/cli/commands/prefab.py
Comment on lines
+1170
to
+1173
| string childPath = childToken.Type == JTokenType.String ? childToken.ToString() : childToken["name"]?.ToString(); | ||
| if (string.IsNullOrEmpty(childPath)) | ||
| { | ||
| return (removedCount, new ErrorResponse("'deleteChild'/'delete_child' entries must be a string or object with 'name' field.")); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Newtonsoft.Json.Linq, does accessing token["name"]on aJValueorJArray throw, and if so which exception type?
💡 Result:
Yes, accessing token["name"] on a JValue throws an exception. It throws InvalidOperationException with the message "Cannot access child value on Newtonsoft.Json.Linq.JValue." For JArray, accessing token["name"] (string key) throws InvalidOperationException or ArgumentException with a message like "Accessed array values with invalid key value: 'name'. Int32 array index expected." JArray's indexer expects integer indices for elements, not string property names.
Citations:
- 1: https://stackoverflow.com/questions/43161432/c-sharp-cannot-access-child-value-on-newtonsoft-json-linq-jvalue
- 2: https://stackoverflow.com/questions/35703278/accessed-jarray-values-with-invalid-key-value-in-different-json
- 3: https://github.com/JamesNK/Newtonsoft.Json/blob/master/Src/Newtonsoft.Json/Linq/JArray.cs
- 4: https://hatchjs.com/cannot-access-child-value-on-newtonsoft-json-linq-jvalue/
- 5: https://stackoverflow.com/questions/67614966/cannot-access-child-value-on-newtonsoft-json-linq-jvalue
- 6: https://stackoverflow.com/questions/5865309/cannot-access-child-value-on-newtonsoft-json-linq-jvalue
- 7: https://stackoverflow.com/questions/39110204/newtonsoft-json-linq-jarray-does-not-contain-a-definition-for
🏁 Script executed:
# Check the file and relevant context around lines 1170-1173
cat -n MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs | sed -n '1160,1185p'Repository: CoplayDev/unity-mcp
Length of output: 1411
🏁 Script executed:
# Search for exception handling in the method that contains this code
rg -A 20 "deleteChild" MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs | head -80Repository: CoplayDev/unity-mcp
Length of output: 2759
🏁 Script executed:
# Search for try-catch blocks and error handling around the RemoveChildren call
rg -B 5 -A 10 "RemoveChildren" MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs | grep -A 15 "var deleteResult"Repository: CoplayDev/unity-mcp
Length of output: 521
🏁 Script executed:
# Look for "Internal error" handling in the file
rg "Internal error" MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.csRepository: CoplayDev/unity-mcp
Length of output: 136
🏁 Script executed:
# Find the method that calls RemoveChildren to see overall exception handling
rg -B 30 "RemoveChildren\(deleteChildToken" MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs | head -50Repository: CoplayDev/unity-mcp
Length of output: 1395
🏁 Script executed:
# Find the context around "Internal error" to see which method has the try-catch
rg -B 10 "Internal error" MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs | head -40Repository: CoplayDev/unity-mcp
Length of output: 603
🏁 Script executed:
# Check if RemoveChildren has any try-catch or if it's only at caller level
rg -A 50 "private static.*RemoveChildren" MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs | head -70Repository: CoplayDev/unity-mcp
Length of output: 2162
Type-check deleteChild entries before reading ["name"].
For non-object entries like 123 or false, accessing childToken["name"] throws InvalidOperationException (on JValue) or ArgumentException (on JArray) before the validation check runs. This unhandled exception propagates to the top-level action handler, causing callers to receive Internal error instead of the intended error message. Use an is JObject type-check to safely handle the property access:
Safer token handling
- string childPath = childToken.Type == JTokenType.String ? childToken.ToString() : childToken["name"]?.ToString();
+ string childPath = childToken.Type == JTokenType.String
+ ? childToken.ToString()
+ : childToken is JObject childObject
+ ? childObject["name"]?.ToString()
+ : null;
if (string.IsNullOrEmpty(childPath))
{
return (removedCount, new ErrorResponse("'deleteChild'/'delete_child' entries must be a string or object with 'name' field."));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs` around lines 1170 - 1173,
The code reads childToken["name"] without ensuring childToken is an object,
which throws for JValue/JArray; modify the deleteChild handling so you first
type-check childToken is a JObject (e.g., using "is JObject") before attempting
childToken["name"], and only fall back to treating it as a string when
childToken.Type == JTokenType.String; ensure you still return the ErrorResponse
with the same message when the token is neither a string nor an object. Update
the logic around childToken, childPath, removedCount and the ErrorResponse
return to use this safe type-check so invalid tokens produce the intended error
instead of an exception.
Comment on lines
+1176
to
+1181
| // Find the child to remove | ||
| Transform childToRemove = targetGo.transform.Find(childPath); | ||
| if (childToRemove == null) | ||
| { | ||
| return (removedCount, new ErrorResponse($"Child '{childPath}' not found under '{targetGo.name}'.")); | ||
| } |
Contributor
There was a problem hiding this comment.
Accept the same root-prefixed paths that get_hierarchy returns.
The path values emitted by get_hierarchy are root-prefixed (for example, Player/Turret/Barrel), but targetGo.transform.Find(childPath) only resolves paths relative to targetGo. That means a caller can copy a valid hierarchy path out of this tool and still get Child ... not found here. Please normalize against prefabRoot/targetGo before calling Find so deleteChild uses the same path contract as target and parent.
Example normalization
- Transform childToRemove = targetGo.transform.Find(childPath);
+ string normalizedPath = childPath;
+ if (normalizedPath.StartsWith(prefabRoot.name + "/", StringComparison.Ordinal))
+ {
+ normalizedPath = normalizedPath.Substring(prefabRoot.name.Length + 1);
+ }
+ if (normalizedPath.StartsWith(targetGo.name + "/", StringComparison.Ordinal))
+ {
+ normalizedPath = normalizedPath.Substring(targetGo.name.Length + 1);
+ }
+
+ Transform childToRemove = targetGo.transform.Find(normalizedPath);
if (childToRemove == null)
{
return (removedCount, new ErrorResponse($"Child '{childPath}' not found under '{targetGo.name}'."));
}📝 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
| // Find the child to remove | |
| Transform childToRemove = targetGo.transform.Find(childPath); | |
| if (childToRemove == null) | |
| { | |
| return (removedCount, new ErrorResponse($"Child '{childPath}' not found under '{targetGo.name}'.")); | |
| } | |
| // Find the child to remove | |
| string normalizedPath = childPath; | |
| if (normalizedPath.StartsWith(prefabRoot.name + "/", StringComparison.Ordinal)) | |
| { | |
| normalizedPath = normalizedPath.Substring(prefabRoot.name.Length + 1); | |
| } | |
| if (normalizedPath.StartsWith(targetGo.name + "/", StringComparison.Ordinal)) | |
| { | |
| normalizedPath = normalizedPath.Substring(targetGo.name.Length + 1); | |
| } | |
| Transform childToRemove = targetGo.transform.Find(normalizedPath); | |
| if (childToRemove == null) | |
| { | |
| return (removedCount, new ErrorResponse($"Child '{childPath}' not found under '{targetGo.name}'.")); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs` around lines 1176 - 1181,
Normalize childPath against the prefab root before calling Transform.Find:
detect and remove a leading root-prefixed segment (the prefab root name or the
prefabRoot path segment produced by get_hierarchy) or any leading '/' so that
childPath is relative to targetGo; update the code around Transform
childToRemove = targetGo.transform.Find(childPath) (in the
deleteChild/remove-child logic) to strip the root prefix (e.g., if childPath
starts with $"{prefabRoot}/" or $"{targetGo.name}/" or "/") then call Find with
the trimmed path so root-prefixed paths from get_hierarchy resolve correctly.
Comment on lines
+790
to
+804
| def test_prefab_modify_invalid_vector_non_numeric(self, runner, mock_unity_response): | ||
| """Test prefab modify rejects non-numeric vector components.""" | ||
| result = runner.invoke(cli, [ | ||
| "prefab", "modify", "Assets/Prefabs/Player.prefab", | ||
| "--position", "1,foo,3" | ||
| ]) | ||
| assert result.exit_code != 0 | ||
|
|
||
| def test_prefab_modify_invalid_vector_wrong_count(self, runner, mock_unity_response): | ||
| """Test prefab modify rejects vectors with wrong component count.""" | ||
| result = runner.invoke(cli, [ | ||
| "prefab", "modify", "Assets/Prefabs/Player.prefab", | ||
| "--position", "1,2" | ||
| ]) | ||
| assert result.exit_code != 0 |
Contributor
There was a problem hiding this comment.
Make these vector tests prove parser rejection, not just any failure.
Lines 796 and 804 only assert exit_code != 0, and run_command is unmocked. If _parse_vector3 regresses and the command falls through to a connection/runtime error, both tests still pass. Please assert the Click error text and assert_not_called() so these only succeed on CLI-side validation failures.
One way to tighten the assertions
- result = runner.invoke(cli, [
- "prefab", "modify", "Assets/Prefabs/Player.prefab",
- "--position", "1,foo,3"
- ])
- assert result.exit_code != 0
+ with patch("cli.commands.prefab.run_command") as mock_run:
+ result = runner.invoke(cli, [
+ "prefab", "modify", "Assets/Prefabs/Player.prefab",
+ "--position", "1,foo,3"
+ ])
+ assert result.exit_code != 0
+ assert "All components must be numeric" in result.output
+ mock_run.assert_not_called()
- result = runner.invoke(cli, [
- "prefab", "modify", "Assets/Prefabs/Player.prefab",
- "--position", "1,2"
- ])
- assert result.exit_code != 0
+ with patch("cli.commands.prefab.run_command") as mock_run:
+ result = runner.invoke(cli, [
+ "prefab", "modify", "Assets/Prefabs/Player.prefab",
+ "--position", "1,2"
+ ])
+ assert result.exit_code != 0
+ assert "Must be 'x,y,z' format" in result.output
+ mock_run.assert_not_called()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Server/tests/test_cli.py` around lines 790 - 804, Update the two tests to
ensure they fail due to CLI parsing (_parse_vector3) rather than downstream
runtime: after calling runner.invoke(cli, ...), assert the exit code is
non-zero, assert the CLI output contains the Click validation message (e.g.
contains "Invalid value for '--position'" or the specific error text produced by
_parse_vector3), and assert the mocked downstream call (use the mock used to
stand in for the runtime call, e.g. run_command or mock_unity_response) was not
invoked by calling its assert_not_called(); keep the existing test names
test_prefab_modify_invalid_vector_non_numeric and
test_prefab_modify_invalid_vector_wrong_count and only change their assertions.
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
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
Add the delete_child command from the original #980, thanks to @darkdread.
Also update the CLI usage and doc
Summary by Sourcery
Add a headless
prefab modifycommand with support for deleting and creating child GameObjects, editing transforms and metadata, and setting component properties on prefabs.New Features:
prefab modifyCLI command to edit prefab contents without opening the Unity UI.manage_prefabstool.manage_prefabs.Enhancements:
manage_prefabstool to accept and forward adelete_childparameter to Unity and document it in the tool description and references.prefab modifyworkflow.modify_contents, including error reporting for invalid paths.Tests:
prefab modifyoptions for delete-child, transforms, properties, components, active state, naming, and validation of vector and JSON inputs.manage_prefabsexposes and forwards thedelete_childparameter correctly and keeps existing behaviors intact.Summary by CodeRabbit
New Features
Documentation
Tests