Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs
Original file line number Diff line number Diff line change
Expand Up @@ -905,6 +905,21 @@ private static (bool modified, ErrorResponse error) ApplyModificationsToPrefabOb
}
}

// Delete child GameObjects (supports single string or array of paths/names)
JToken deleteChildToken = @params["deleteChild"] ?? @params["delete_child"];
if (deleteChildToken != null)
{
var deleteResult = RemoveChildren(deleteChildToken, targetGo, prefabRoot);
if (deleteResult.error != null)
{
return (false, deleteResult.error);
}
if (deleteResult.removedCount > 0)
{
modified = true;
}
}

// Set properties on existing components
JObject componentProperties = @params["componentProperties"] as JObject ?? @params["component_properties"] as JObject;
if (componentProperties != null && componentProperties.Count > 0)
Expand Down Expand Up @@ -1132,6 +1147,47 @@ private static (bool created, ErrorResponse error) CreateSingleChildInPrefab(JTo
return (true, null);
}

/// <summary>
/// Removes child GameObjects from a prefab.
/// </summary>
private static (int removedCount, ErrorResponse error) RemoveChildren(JToken deleteChildToken, GameObject targetGo, GameObject prefabRoot)
{
int removedCount = 0;

// Normalize to array
JArray childrenToDelete;
if (deleteChildToken is JArray arr)
{
childrenToDelete = arr;
}
else
{
childrenToDelete = new JArray { deleteChildToken };
}

foreach (var childToken in childrenToDelete)
{
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."));
Comment on lines +1170 to +1173
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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:


🏁 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 -80

Repository: 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.cs

Repository: 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 -50

Repository: 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 -40

Repository: 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 -70

Repository: 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.

}

// 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}'."));
}
Comment on lines +1176 to +1181
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


UnityEngine.Object.DestroyImmediate(childToRemove.gameObject);
removedCount++;
McpLog.Info($"[ManagePrefabs] Removed child '{childPath}' under '{targetGo.name}' in prefab.");
}

return (removedCount, null);
}

#endregion

#region Hierarchy Builder
Expand Down
9 changes: 9 additions & 0 deletions Server/src/cli/CLI_USAGE_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,15 @@ unity-mcp prefab save

# Close prefab stage
unity-mcp prefab close

# Modify prefab contents (headless)
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"
Comment on lines +565 to +566
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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`.

unity-mcp prefab modify "Assets/Prefabs/Player.prefab" --set-property "Rigidbody.mass=5"
unity-mcp prefab modify "Assets/Prefabs/Player.prefab" --add-component BoxCollider --remove-component SphereCollider
unity-mcp prefab modify "Assets/Prefabs/Player.prefab" --create-child '{"name":"Spawn","primitive_type":"Sphere"}'
unity-mcp prefab modify "Assets/Prefabs/Player.prefab" --name NewName --tag Player --layer UI
unity-mcp prefab modify "Assets/Prefabs/Player.prefab" --inactive
```

### UI Commands
Expand Down
130 changes: 130 additions & 0 deletions Server/src/cli/commands/prefab.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Prefab CLI commands."""

import json
import sys
import click
from typing import Optional, Any
Expand Down Expand Up @@ -246,3 +247,132 @@ def create(target: str, path: str, overwrite: bool, include_inactive: bool, unli
click.echo(format_output(result, config.format))
if result.get("success"):
print_success(f"Created prefab: {path}")


def _parse_vector3(value: str) -> list[float]:
"""Parse 'x,y,z' string to list of floats."""
parts = value.split(",")
if len(parts) != 3:
raise click.BadParameter("Must be 'x,y,z' format")
try:
return [float(p.strip()) for p in parts]
except ValueError as e:
raise click.BadParameter(f"All components must be numeric, got: '{value}'") from e


def _parse_property(prop_str: str) -> tuple[str, str, Any]:
"""Parse 'Component.prop=value' into (component, prop, value)."""
if "=" not in prop_str:
raise click.BadParameter("Must be 'Component.prop=value' format")
comp_prop, val_str = prop_str.split("=", 1)
if "." not in comp_prop:
raise click.BadParameter("Must be 'Component.prop=value' format")
component, prop = comp_prop.rsplit(".", 1)
if not component.strip() or not prop.strip():
raise click.BadParameter(f"Component and property must be non-empty in '{comp_prop}', expected 'Component.prop=value'")

val_str = val_str.strip()

# Parse booleans
if val_str.lower() == "true":
parsed_value: Any = True
elif val_str.lower() == "false":
parsed_value = False
# 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
Comment on lines +281 to +291
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot uses AI. Check for mistakes.

Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
return component.strip(), prop.strip(), parsed_value


@prefab.command("modify")
@click.argument("path")
@click.option("--target", "-t", help="Target object name/path within prefab (default: root)")
@click.option("--position", "-p", help="New local position as 'x,y,z'")
@click.option("--rotation", "-r", help="New local rotation as 'x,y,z'")
@click.option("--scale", "-s", help="New local scale as 'x,y,z'")
@click.option("--name", "-n", help="New name for target")
@click.option("--tag", help="New tag")
@click.option("--layer", help="New layer")
@click.option("--active/--inactive", default=None, help="Set active state")
@click.option("--parent", help="New parent object name/path")
@click.option("--add-component", multiple=True, help="Component type to add (repeatable)")
@click.option("--remove-component", multiple=True, help="Component type to remove (repeatable)")
@click.option("--set-property", multiple=True, help="Property as 'Component.prop=value' (repeatable)")
@click.option("--delete-child", multiple=True, help="Child name/path to remove (repeatable)")
@click.option("--create-child", help="JSON object for child creation")
@handle_unity_errors
def modify(path: str, target: Optional[str], position: Optional[str], rotation: Optional[str],
scale: Optional[str], name: Optional[str], tag: Optional[str], layer: Optional[str],
active: Optional[bool], parent: Optional[str], add_component: tuple, remove_component: tuple,
set_property: tuple, delete_child: tuple, create_child: Optional[str]):
"""Modify a prefab's contents (headless, no UI).

\b
Examples:
unity-mcp prefab modify "Assets/Prefabs/Player.prefab" --delete-child Child1
unity-mcp prefab modify "Assets/Prefabs/Player.prefab" --delete-child "Turret/Barrel" --delete-child Bullet
unity-mcp prefab modify "Assets/Prefabs/Player.prefab" --target Weapon --position "0,1,2"
unity-mcp prefab modify "Assets/Prefabs/Player.prefab" --set-property "Rigidbody.mass=5"
unity-mcp prefab modify "Assets/Prefabs/Player.prefab" --create-child '{"name":"Spawn","primitive_type":"Sphere"}'
"""
config = get_config()

params: dict[str, Any] = {
"action": "modify_contents",
"prefabPath": path,
}

if target:
params["target"] = target
if position:
params["position"] = _parse_vector3(position)
if rotation:
params["rotation"] = _parse_vector3(rotation)
if scale:
params["scale"] = _parse_vector3(scale)
if name:
params["name"] = name
if tag:
params["tag"] = tag
if layer:
params["layer"] = layer
if active is not None:
params["setActive"] = active
if parent:
params["parent"] = parent
if add_component:
params["componentsToAdd"] = list(add_component)
if remove_component:
params["componentsToRemove"] = list(remove_component)
if set_property:
component_properties: dict[str, dict[str, Any]] = {}
for prop in set_property:
comp, name_p, val = _parse_property(prop)
if comp not in component_properties:
component_properties[comp] = {}
component_properties[comp][name_p] = val
params["componentProperties"] = component_properties
if delete_child:
params["deleteChild"] = list(delete_child)
if create_child:
try:
parsed = json.loads(create_child)
except json.JSONDecodeError as e:
raise click.BadParameter(f"Invalid JSON for --create-child: {e}") from e
if not isinstance(parsed, dict):
raise click.BadParameter(f"--create-child must be a JSON object, got {type(parsed).__name__}")
params["createChild"] = parsed

result = run_command("manage_prefabs", params, config)
click.echo(format_output(result, config.format))
if result.get("success"):
print_success(f"Modified prefab: {path}")
7 changes: 7 additions & 0 deletions Server/src/services/tools/manage_prefabs.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@
"(single object or array for batch creation in one save). "
"Example: create_child=[{\"name\": \"Child1\", \"primitive_type\": \"Sphere\", \"position\": [1,0,0]}, "
"{\"name\": \"Nested\", \"source_prefab_path\": \"Assets/Prefabs/Bullet.prefab\", \"position\": [0,2,0]}]. "
"Use delete_child parameter to remove child GameObjects from the prefab "
"(single name/path or array of paths for batch deletion. "
"Example: delete_child=[\"Child1\", \"Child2/Grandchild\"]). "
"Use component_properties with modify_contents to set serialized fields on existing components "
"(e.g. component_properties={\"Rigidbody\": {\"mass\": 5.0}, \"MyScript\": {\"health\": 100}}). "
"Supports object references via {\"guid\": \"...\"}, {\"path\": \"Assets/...\"}, or {\"instanceID\": 123}. "
Expand Down Expand Up @@ -67,6 +70,7 @@ async def manage_prefabs(
components_to_add: Annotated[list[str], "Component types to add in modify_contents."] | None = None,
components_to_remove: Annotated[list[str], "Component types to remove in modify_contents."] | None = None,
create_child: Annotated[dict[str, Any] | list[dict[str, Any]], "Create child GameObject(s) in the prefab. Single object or array of objects, each with: name (required), parent (optional, defaults to target), source_prefab_path (optional: asset path to instantiate as nested prefab, e.g. 'Assets/Prefabs/Bullet.prefab'), primitive_type (optional: Cube, Sphere, Capsule, Cylinder, Plane, Quad), position, rotation, scale, components_to_add, tag, layer, set_active. source_prefab_path and primitive_type are mutually exclusive."] | None = None,
delete_child: Annotated[str | list[str], "Child name(s) or path(s) to remove from the prefab. Supports single string or array for batch deletion (e.g. 'Child1' or ['Child1', 'Child1/Grandchild'])."] | None = None,
component_properties: Annotated[dict[str, dict[str, Any]], "Set properties on existing components in modify_contents. Keys are component type names, values are dicts of property name to value. Example: {\"Rigidbody\": {\"mass\": 5.0}, \"MyScript\": {\"health\": 100}}. Supports object references via {\"guid\": \"...\"}, {\"path\": \"Assets/...\"}, or {\"instanceID\": 123}. For Sprite sub-assets: {\"guid\": \"...\", \"spriteName\": \"<name>\"}. Single-sprite textures auto-resolve."] | None = None,
) -> dict[str, Any]:
# Back-compat: map 'name' → 'target' for create_from_gameobject (Unity accepts both)
Expand Down Expand Up @@ -185,6 +189,9 @@ def normalize_child_params(child: Any, index: int | None = None) -> tuple[dict |
return {"success": False, "message": err}
params["createChild"] = child_params

if delete_child is not None:
params["deleteChild"] = delete_child
Comment on lines +192 to +193
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot uses AI. Check for mistakes.

# Send command to Unity
response = await send_with_unity_instance(
async_send_command_with_retry, unity_instance, "manage_prefabs", params
Expand Down
Loading