feat: Prefab Feature Updates #611
Conversation
- Add get_info: retrieve prefab metadata (GUID, type, components, child count, variant info) - Add get_hierarchy: get prefab internal structure with pagination support - Add list_prefabs: search prefabs in project with optional name filtering - Extract PrefabUtilityHelper class for reusable prefab utility methods - Update Python tool descriptions and parameter documentation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace PrefabUtility.SaveAsPrefabAsset (for creating new prefabs) with EditorSceneManager.SaveScene to properly save stage modifications. This fixes the issue where component additions were lost after closing the prefab stage. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add pagination constants (DefaultPageSize, MaxPageSize) - Extract SaveAndRefreshStage helper to reduce duplication - Change all user-facing messages to English - Add REQUIRED_PARAMS validation in Python - Split path parameter into prefab_path and folder_path for clarity - Improve error handling with specific exception types Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…and new unlinking option
…r prefab stage saving
Reviewer's GuideExtends Unity prefab management with new read APIs, safer write flows, and better server orchestration, including a critical fix to prefab stage saving and new hierarchy pagination support. Sequence diagram for create_from_gameobject prefab workflowsequenceDiagram
actor User
participant Client as ToolClient
participant PyTool as manage_prefabs_py
participant UnityTransport
participant UnityEditor
participant CSHandler as ManagePrefabs_cs
User->>Client: Invoke manage_prefabs(action=create_from_gameobject,...)
Client->>PyTool: manage_prefabs(request)
PyTool->>PyTool: Validate required params
PyTool->>PyTool: preflight(wait_for_no_compile, refresh_if_dirty)
PyTool->>UnityTransport: send_with_unity_instance("manage_prefabs", params)
UnityTransport->>UnityEditor: async_send_command_with_retry("manage_prefabs", params)
UnityEditor->>CSHandler: HandleCommand(JObject params)
CSHandler->>CSHandler: switch(action=create_from_gameobject)
CSHandler->>CSHandler: ValidateCreatePrefabParams(params)
alt invalid params
CSHandler-->>UnityEditor: ErrorResponse
else valid params
CSHandler->>CSHandler: FindSceneObjectByName(target, searchInactive)
alt target not found
CSHandler-->>UnityEditor: ErrorResponse
else target found
CSHandler->>CSHandler: ValidateSourceObjectForPrefab(sourceObject, unlinkIfInstance, allowOverwrite)
alt invalid sourceObject
CSHandler-->>UnityEditor: ErrorResponse
else valid sourceObject
opt unlinkIfInstance and shouldUnlink
CSHandler->>UnityEditor: PrefabUtility.GetOutermostPrefabInstanceRoot
CSHandler->>UnityEditor: PrefabUtility.UnpackPrefabInstance
end
CSHandler->>CSHandler: EnsureAssetDirectoryExists(finalPath)
CSHandler->>CSHandler: CreatePrefabAsset(sourceObject, finalPath, allowOverwrite)
CSHandler->>UnityEditor: AssetDatabase.SaveAssets
CSHandler->>UnityEditor: AssetDatabase.Refresh
CSHandler->>UnityEditor: Selection.activeGameObject = result
CSHandler-->>UnityEditor: SuccessResponse(data: prefabPath, instanceId,...)
end
end
end
UnityEditor-->>UnityTransport: response
UnityTransport-->>PyTool: response
PyTool->>PyTool: Normalize response (ensure success field)
PyTool-->>Client: result
Client-->>User: Show operation result
Class diagram for updated prefab management utilitiesclassDiagram
class ManagePrefabs {
<<static>>
-const string ACTION_OPEN_STAGE
-const string ACTION_CLOSE_STAGE
-const string ACTION_SAVE_OPEN_STAGE
-const string ACTION_CREATE_FROM_GAMEOBJECT
-const string ACTION_GET_INFO
-const string ACTION_GET_HIERARCHY
-const string SupportedActions
-const int DefaultPageSize
-const int MaxPageSize
+HandleCommand(JObject params) object
-OpenStage(JObject params) object
-CloseStage(JObject params) object
-SaveOpenStage() object
-SaveAndRefreshStage(PrefabStage stage) void
-SaveStagePrefab(PrefabStage stage) void
-ValidatePrefabStageForSave(PrefabStage stage) bool
-CreatePrefabFromGameObject(JObject params) object
-ValidateCreatePrefabParams(JObject params) (bool isValid, string errorMessage, string targetName, string finalPath, bool includeInactive, bool replaceExisting, bool unlinkIfInstance)
-ValidateSourceObjectForPrefab(GameObject sourceObject, bool unlinkIfInstance, bool replaceExisting) (bool isValid, string errorMessage, bool shouldUnlink, string existingPrefabPath)
-CreatePrefabAsset(GameObject sourceObject, string path, bool replaceExisting) GameObject
-EnsureAssetDirectoryExists(string assetPath) void
-FindSceneObjectByName(string name, bool includeInactive) GameObject
-GetInfo(JObject params) object
-GetHierarchy(JObject params) object
-BuildHierarchyItems(Transform root) List~object~
-BuildHierarchyItemsRecursive(Transform transform, string parentPath, List~object~ items) void
-SerializeStage(PrefabStage stage) object
}
class PrefabUtilityHelper {
<<static>>
+GetPrefabGUID(string assetPath) string
+GetVariantInfo(GameObject prefabAsset) (bool isVariant, string parentPath, string parentGuid)
+GetComponentTypeNames(GameObject obj) List~string~
+CountChildrenRecursive(Transform transform) int
+GetNestedPrefabPath(GameObject gameObject) string
}
class PaginationRequest {
+PageSize int
+Cursor int
+FromParams(JObject params, int defaultPageSize) PaginationRequest
}
ManagePrefabs ..> PrefabUtilityHelper : uses
ManagePrefabs ..> PaginationRequest : uses
ManagePrefabs ..> PrefabStage : uses
ManagePrefabs ..> GameObject : uses
ManagePrefabs ..> Transform : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds nested-prefab utilities and asset-path validation to editor helpers; expands ManagePrefabs with get_info/get_hierarchy, robust open/save/close/create flows and hierarchy builders; server manage_prefabs gains preflight, expanded actions/params and normalization; CLI adds --unlink-if-instance and --force flags; marks scenes dirty after GameObject edits. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Server as MCP Server
participant Tool as manage_prefabs.py
participant Unity as Unity Editor
participant Handler as ManagePrefabs.cs
Client->>Server: POST manage_prefabs(action=get_hierarchy, prefabPath, flags)
Server->>Tool: assemble params & run preflight
Tool->>Unity: preflight check
Unity-->>Tool: preflight result
Tool->>Unity: send command "get_hierarchy" with params
Unity->>Handler: HandleCommand("get_hierarchy")
Handler->>Handler: Load prefab contents & BuildHierarchyItemsRecursive
Handler-->>Unity: HierarchyResponse (items, totalCount)
Unity-->>Tool: raw response
Tool->>Tool: normalize/validate response (ensure success)
Tool-->>Server: standardized dict
Server-->>Client: return hierarchy result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
SaveAndRefreshStageyou callAssetDatabase.SaveAssets()both insideSaveStagePrefaband again immediately after, which is redundant; consider centralizing the save in one place to avoid unnecessary extra writes. - In
GetHierarchy,cursoris used directly to computestartIndex/endIndexwithout clamping to 0; if a negative cursor is passed in,Skip(startIndex)will throw, so it would be safer toMathf.Max(cursor, 0)before using it. - The
prefabPathargument passed throughBuildHierarchyItemsandBuildHierarchyItemsRecursiveis never used; if you don't need it, removing this parameter from the methods and calls would simplify the hierarchy-building helpers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `SaveAndRefreshStage` you call `AssetDatabase.SaveAssets()` both inside `SaveStagePrefab` and again immediately after, which is redundant; consider centralizing the save in one place to avoid unnecessary extra writes.
- In `GetHierarchy`, `cursor` is used directly to compute `startIndex`/`endIndex` without clamping to 0; if a negative cursor is passed in, `Skip(startIndex)` will throw, so it would be safer to `Mathf.Max(cursor, 0)` before using it.
- The `prefabPath` argument passed through `BuildHierarchyItems` and `BuildHierarchyItemsRecursive` is never used; if you don't need it, removing this parameter from the methods and calls would simplify the hierarchy-building helpers.
## Individual Comments
### Comment 1
<location> `Server/src/services/tools/manage_prefabs.py:121-132` </location>
<code_context>
+
unity_instance = get_unity_instance_from_context(ctx)
+ # Preflight check for operations to ensure Unity is ready
try:
+ gate = await preflight(ctx, wait_for_no_compile=True, refresh_if_dirty=True)
+ if gate is not None:
+ return gate.model_dump()
</code_context>
<issue_to_address>
**suggestion (performance):** Consider skipping or relaxing preflight for read‑only prefab operations.
Using `preflight(..., wait_for_no_compile=True, refresh_if_dirty=True)` for all requests adds unnecessary latency and asset refreshes for read-only calls like `get_info` and `get_hierarchy`. Consider only running this heavier preflight for write operations (`open_stage`, `close_stage`, `save_open_stage`, `create_from_gameobject`) and either skipping it or using a lighter version for reads to keep those calls more responsive.
```suggestion
unity_instance = get_unity_instance_from_context(ctx)
# Preflight check for operations to ensure Unity is ready
# Use heavier preflight only for write operations to avoid unnecessary latency on read-only calls
try:
write_actions = {
"open_stage",
"close_stage",
"save_open_stage",
"create_from_gameobject",
}
if action in write_actions:
# For write operations, ensure Unity is not compiling and refresh dirty assets
gate = await preflight(ctx, wait_for_no_compile=True, refresh_if_dirty=True)
else:
# For read-only operations, use a lighter preflight to keep calls responsive
gate = await preflight(ctx, wait_for_no_compile=False, refresh_if_dirty=False)
if gate is not None:
return gate.model_dump()
except Exception as exc:
return {
"success": False,
"message": f"Unity preflight check failed: {exc}"
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR enhances prefab management with two new read operations (get_info, get_hierarchy) and critical bug fix for saving prefab stages, plus new parameters for better workflow control.
Changes:
- Fixed
save_open_stageto use correct Unity API (SetDirty+SaveAssetsinstead ofSaveAsPrefabAsset) - Added
get_infoandget_hierarchyactions for reading prefab metadata and structure - Added
unlink_if_instance,allow_overwrite, andsearch_inactiveparameters for enhanced prefab creation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| Server/src/services/tools/manage_prefabs.py | Updated tool with new actions, parameters, validation, pagination support, and preflight checks |
| MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs | Fixed critical save bug, added get_info/get_hierarchy actions, enhanced validation with tuple-based methods, improved error handling |
| MCPForUnity/Editor/Helpers/PrefabUtilityHelper.cs | New helper class providing utility methods for GUID retrieval, variant info, component inspection, and nested prefab paths |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs`:
- Around line 287-297: UnpackPrefabInstance is being called on sourceObject
which may be a child of a prefab instance; change the unlink logic in
ManagePrefabs (where objectValidation.shouldUnlink is checked) to first get the
instance root via PrefabUtility.GetOutermostPrefabInstanceRoot(sourceObject) (or
GetNearestPrefabInstanceRoot for nested cases), validate the returned root is
non-null, and call PrefabUtility.UnpackPrefabInstance(instanceRoot,
PrefabUnpackMode.Completely, InteractionMode.AutomatedAction) instead of
unpacking sourceObject; keep the existing try/catch and logging but reference
the instance root name in the log and error message.
In `@Server/src/services/tools/manage_prefabs.py`:
- Around line 111-119: The current required-parameter check in manage_prefabs.py
(using REQUIRED_PARAMS -> required, param_name, param_value =
locals().get(param_name)) only rejects None but allows blank/whitespace strings;
update the conditional to treat empty or whitespace-only strings as missing as
well (e.g., if param_value is None or (isinstance(param_value, str) and
param_value.strip() == "") ) and return the same error object so blank params
are rejected up front.
- Around line 182-190: The code treats non-dict MCPResponse objects as
unexpected, hiding useful error details; update the handling in the
response-return logic (the block that currently checks isinstance(response,
dict)) to first normalize any MCPResponse-like object by checking for a
model_dump() method and calling response = response.model_dump() when present,
then continue the existing dict check so fields like error and hint are
preserved; ensure you reference this change in the function/method that calls
async_send_command_with_retry and performs the isinstance(response, dict) check
so the normalized dict is returned instead of the generic unexpected-type
message.
🧹 Nitpick comments (4)
Server/src/services/tools/manage_prefabs.py (1)
135-138: Validate pagination bounds and scope them to get_hierarchy.
The docstring states a max of 500, but inputs aren’t validated, and pagination params are sent for non‑hierarchy actions. Consider validating and only attaching them forget_hierarchy.♻️ Proposed refactor
- coerced_page_size = coerce_int(page_size, default=None) - coerced_cursor = coerce_int(cursor, default=None) + coerced_page_size = coerce_int(page_size, default=None) + coerced_cursor = coerce_int(cursor, default=None) + if action == "get_hierarchy": + if coerced_page_size is not None and not (1 <= coerced_page_size <= 500): + return { + "success": False, + "message": "page_size must be between 1 and 500." + } + if coerced_cursor is not None and coerced_cursor < 0: + return { + "success": False, + "message": "cursor must be >= 0." + } @@ - if coerced_page_size is not None: + if action == "get_hierarchy" and coerced_page_size is not None: params["pageSize"] = coerced_page_size @@ - if coerced_cursor is not None: + if action == "get_hierarchy" and coerced_cursor is not None: params["cursor"] = coerced_cursorAlso applies to: 170-176
MCPForUnity/Editor/Helpers/PrefabUtilityHelper.cs (1)
18-33: Normalize empty GUID to null for invalid paths.
AssetDatabase.AssetPathToGUIDcan return an empty string for invalid paths; returningnullkeeps the method contract consistent and avoids treating empty GUIDs as valid.♻️ Suggested tweak
- return AssetDatabase.AssetPathToGUID(assetPath); + var guid = AssetDatabase.AssetPathToGUID(assetPath); + return string.IsNullOrEmpty(guid) ? null : guid;MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs (2)
161-173: Avoid duplicateAssetDatabase.SaveAssetscalls.
SaveStagePrefabalready persists assets;SaveAndRefreshStageinvokesSaveAssetsagain, adding redundant IO. Consider keeping the save call in one place.♻️ Suggested tweak
- // Save all assets to ensure changes persist to disk - AssetDatabase.SaveAssets(); - McpLog.Info($"[ManagePrefabs] Successfully saved prefab '{stage.assetPath}'.");Also applies to: 195-203
540-595: Pagination still builds the full hierarchy in memory.
BuildHierarchyItemswalks the entire tree before slicing, which can negate pagination benefits for large prefabs. Consider streaming traversal with early exit, or counting separately while only materializing the requested window.Also applies to: 604-648
|
When MCP first emerged, I had the idea of building a Unity–MCP integration myself. However, due to academic commitments and limitations in my technical foundation at the time, I wasn’t able to implement an MCP system on my own. Seeing your project was a genuinely pleasant surprise for me, and I hope to help make it even better. |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- PrefabUtilityHelper.GetPrefabGUID() documents that it returns null for invalid paths, but AssetDatabase.AssetPathToGUID() returns an empty string on failure, so the behavior and XML doc are inconsistent; consider normalizing the return value or updating the comment.
- GetHierarchy currently builds a complete in-memory list of all hierarchy items before applying pagination, which may be expensive for very large prefabs; consider streaming the traversal with index bounds to only materialize the requested page.
- The Python manage_prefabs preflight check runs for all actions, including read-only get_info/get_hierarchy, which may add unnecessary latency; consider skipping or relaxing preflight for non-mutating operations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- PrefabUtilityHelper.GetPrefabGUID() documents that it returns null for invalid paths, but AssetDatabase.AssetPathToGUID() returns an empty string on failure, so the behavior and XML doc are inconsistent; consider normalizing the return value or updating the comment.
- GetHierarchy currently builds a complete in-memory list of all hierarchy items before applying pagination, which may be expensive for very large prefabs; consider streaming the traversal with index bounds to only materialize the requested page.
- The Python manage_prefabs preflight check runs for all actions, including read-only get_info/get_hierarchy, which may add unnecessary latency; consider skipping or relaxing preflight for non-mutating operations.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs:375-376` </location>
<code_context>
+ /// <summary>
+ /// Validates source object can be converted to prefab.
+ /// </summary>
+ private static (bool isValid, string errorMessage, bool shouldUnlink, string existingPrefabPath)
+ ValidateSourceObjectForPrefab(GameObject sourceObject, bool unlinkIfInstance, bool replaceExisting)
+ {
+ // Check if this is a Prefab Asset (the .prefab file itself in the editor)
</code_context>
<issue_to_address>
**suggestion:** The replaceExisting parameter in ValidateSourceObjectForPrefab is unused and can be removed or leveraged.
The `replaceExisting` parameter isn’t used anywhere in this method, which can obscure intent for future maintainers. Please either remove it (and update the call sites) or implement logic that relies on it, e.g., when validating behavior for overwriting existing prefabs.
Suggested implementation:
```csharp
/// <summary>
/// Validates source object can be converted to prefab.
/// </summary>
private static (bool isValid, string errorMessage, bool shouldUnlink, string existingPrefabPath)
ValidateSourceObjectForPrefab(GameObject sourceObject, bool unlinkIfInstance)
{
```
The method signature change requires updating all call sites in `ManagePrefabs.cs` (and any other files) where `ValidateSourceObjectForPrefab` is invoked. Specifically:
1. Remove the `replaceExisting` argument from each call, e.g. change:
`ValidateSourceObjectForPrefab(sourceObject, unlinkIfInstance, replaceExisting)` to `ValidateSourceObjectForPrefab(sourceObject, unlinkIfInstance)`.
2. Ensure there are no remaining references to the removed `replaceExisting` parameter in this method (currently there are none) or in associated documentation/comments.
</issue_to_address>
### Comment 2
<location> `Server/src/services/tools/manage_prefabs.py:126-127` </location>
<code_context>
+ # Preflight check for operations to ensure Unity is ready
try:
+ gate = await preflight(ctx, wait_for_no_compile=True, refresh_if_dirty=True)
+ if gate is not None:
+ return gate.model_dump()
+ except Exception as exc:
</code_context>
<issue_to_address>
**suggestion (performance):** Running a full preflight (no-compile + refresh_if_dirty) for read-only actions may be unnecessarily heavy.
This preflight configuration is applied uniformly to all actions, including read-only ones like `get_info` and `get_hierarchy`. For these non-destructive calls, forcing a refresh and waiting for compilation can add avoidable latency and load, especially when invoked often.
Consider using a lighter preflight for read operations (e.g., skipping `refresh_if_dirty` and/or `wait_for_no_compile`) while retaining the stricter behavior for write operations (`open_stage`, `create_from_gameobject`, `save_open_stage`, `close_stage`).
Suggested implementation:
```python
# Preflight check for operations to ensure Unity is ready
try:
# Use stricter preflight for write operations, lighter for read-only ones
write_actions = {
"open_stage",
"create_from_gameobject",
"save_open_stage",
"close_stage",
}
if action in write_actions:
gate = await preflight(ctx, wait_for_no_compile=True, refresh_if_dirty=True)
else:
# For read-only or unknown actions, avoid forcing a refresh/compile to reduce latency
gate = await preflight(ctx, wait_for_no_compile=False, refresh_if_dirty=False)
if gate is not None:
return gate.model_dump()
except Exception as exc:
return {
"success": False,
"message": f"Unity preflight check failed: {exc}"
}
```
- If this module already defines action constants/enums elsewhere (e.g., `ACTION_OPEN_STAGE`, etc.), replace the string literals in `write_actions` with those constants to stay consistent with the existing conventions.
- If there are other mutating actions besides `open_stage`, `create_from_gameobject`, `save_open_stage`, and `close_stage`, add them to the `write_actions` set so they continue to use the stricter preflight.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…isting parameter and simplify validation logic
- Fix searchInactive not working correctly for child objects - Improve error message accuracy for object not found - Use Application.dataPath for reliable directory path resolution
|
I've tested all the parameters of the manage_prefabs function using Claude Code. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs (1)
85-90: Missing null check afterSanitizeAssetPath.
SanitizeAssetPathreturnsnullwhen path traversal is detected (as added in AssetPathUtility.cs). PassingnulltoLoadAssetAtPathwill returnnull, which triggers a misleading error message ("No prefab asset found") instead of indicating a security issue.🐛 Proposed fix
string sanitizedPath = AssetPathUtility.SanitizeAssetPath(prefabPath); + if (sanitizedPath == null) + { + return new ErrorResponse($"Invalid prefab path: '{prefabPath}'."); + } GameObject prefabAsset = AssetDatabase.LoadAssetAtPath<GameObject>(sanitizedPath);
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs`:
- Around line 512-517: Sanitized path can be null when
AssetPathUtility.SanitizeAssetPath detects path traversal; before calling
AssetDatabase.LoadAssetAtPath in ManagePrefabs.cs (the block using
sanitizedPath, prefabAsset and ErrorResponse), add a null check for
sanitizedPath and return an ErrorResponse explaining the path was invalid or
contained traversal (similar to OpenStage handling). Ensure the check occurs
immediately after calling AssetPathUtility.SanitizeAssetPath(prefabPath) to
avoid the misleading "No prefab asset found" message.
- Around line 353-363: SanitizeAssetPath can return null which causes a
NullReferenceException when calling sanitizedPath.EndsWith; in ManagePrefabs
(ManagePrefabs.cs) capture the result of
AssetPathUtility.SanitizeAssetPath(requestedPath), check for null (or empty)
before calling EndsWith and return the same failure tuple with a clear message
if null is returned, and only then append ".prefab" and validate the "Assets/"
prefix so methods like the code path using sanitizedPath (variable) are safe.
- Around line 553-565: AssetPathUtility.SanitizeAssetPath(prefabPath) can return
null/empty and you must guard before calling PrefabUtility.LoadPrefabContents;
add a null/empty check for sanitizedPath (after calling
AssetPathUtility.SanitizeAssetPath) and return an ErrorResponse with a clear
message if sanitizedPath is null or empty so
PrefabUtility.LoadPrefabContents(sanitizedPath) is never called with a null
argument.
🧹 Nitpick comments (3)
Server/src/cli/commands/prefab.py (1)
116-122: Minor style inconsistency: extraneous blank line between decorator and function.Line 121 introduces an empty line between the
@click.optiondecorator and the function definition, which is inconsistent with the rest of this file and other CLI command files (e.g.,scene.py,asset.py,gameobject.py).Suggested fix
`@click.option`( "--unlink-if-instance", is_flag=True, help="Unlink from existing prefab before creating new one." ) - def create(target: str, path: str, overwrite: bool, include_inactive: bool, unlink_if_instance: bool):MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
57-94: Validation logic looks correct but has minor inconsistency withSanitizeAssetPath.The invalid character set
{ ':', '*', '?', '"', '<', '>', '|' }is appropriate for cross-platform compatibility. However, note thatSanitizeAssetPathdoesn't reject these characters—it only handles traversal and prefix. This means a path could passSanitizeAssetPathbut failIsValidAssetPath.If both methods are intended to be used together (sanitize then validate), this is fine. If they might be used independently, consider aligning the character validation.
♻️ Optional: Extract shared validation logic
+ private static readonly char[] InvalidPathChars = { ':', '*', '?', '"', '<', '>', '|' }; + + private static bool ContainsInvalidChars(string path) + { + foreach (char c in InvalidPathChars) + { + if (path.IndexOf(c) >= 0) return true; + } + return false; + }Then use in both methods if desired.
MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs (1)
411-429:replaceExistingparameter doesn't affect actual behavior.
SaveAsPrefabAssetAndConnectwill overwrite an existing prefab file regardless of thereplaceExistingflag. The flag only affects the log message. The actual overwrite protection is handled earlier at lines 273-277 viaGenerateUniqueAssetPath. This is fine but slightly confusing—consider renaming or documenting this clearly.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs`:
- Around line 356-374: Sanitized prefab paths currently aren't validated for
invalid filename characters before we call EnsureAssetDirectoryExists, causing
potential exceptions; after you set sanitizedPath (and after appending ".prefab"
if needed) call AssetPathUtility.IsValidAssetPath(sanitizedPath) and, if it
returns false, return a failure tuple with a clear message (similar format to
existing returns) instead of continuing, so EnsureAssetDirectoryExists and
subsequent code never receive an invalid path; reference the sanitizedPath
variable, AssetPathUtility.SanitizeAssetPath, AssetPathUtility.IsValidAssetPath,
and EnsureAssetDirectoryExists when making the change.
- Around line 275-328: The wasReplaced flag is currently derived from
objectValidation.existingPrefabPath which reflects source linking, not whether
an asset already existed at finalPath; update the logic so you determine
pre-existence of the target asset (e.g. call
AssetDatabase.LoadAssetAtPath<UnityEngine.Object>(finalPath) before creating the
prefab or reuse the earlier check that generated a unique path) and set
wasReplaced = replaceExisting && (target asset existed prior to creation).
Adjust the SuccessResponse payload construction (where wasReplaced is set) to
reference that pre-existence boolean so overwrites are reported correctly.
- Around line 344-378: The boolean options are only read from camelCase keys and
miss snake_case fallbacks, causing silent failures; update the three reads to
probe both variants from the `@params` dictionary (same pattern used for
target/name): for includeInactive use `@params`["searchInactive"] ??
`@params`["search_inactive"], for replaceExisting use `@params`["allowOverwrite"] ??
`@params`["allow_overwrite"], and for unlinkIfInstance use
`@params`["unlinkIfInstance"] ?? `@params`["unlink_if_instance"], then call
ToObject<bool>() and keep the ?? false default so callers using either naming
convention are accepted.
| string targetName = @params["target"]?.ToString() ?? @params["name"]?.ToString(); | ||
| if (string.IsNullOrEmpty(targetName)) | ||
| { | ||
| return (false, "'target' parameter is required for create_from_gameobject.", null, null, false, false, false); | ||
| } | ||
|
|
||
| string requestedPath = @params["prefabPath"]?.ToString(); | ||
| if (string.IsNullOrWhiteSpace(requestedPath)) | ||
| { | ||
| return (false, "'prefabPath' parameter is required for create_from_gameobject.", targetName, null, false, false, false); | ||
| } | ||
|
|
||
| string sanitizedPath = AssetPathUtility.SanitizeAssetPath(requestedPath); | ||
| if (sanitizedPath == null) | ||
| { | ||
| return (false, $"Invalid prefab path (path traversal detected): '{requestedPath}'", targetName, null, false, false, false); | ||
| } | ||
| if (string.IsNullOrEmpty(sanitizedPath)) | ||
| { | ||
| return (false, $"Invalid prefab path '{requestedPath}'. Path cannot be empty.", targetName, null, false, false, false); | ||
| } | ||
| if (!sanitizedPath.EndsWith(".prefab", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| sanitizedPath += ".prefab"; | ||
| } | ||
|
|
||
| // Validate path is within Assets folder | ||
| if (!sanitizedPath.StartsWith("Assets/", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return (false, $"Prefab path must be within the Assets folder. Got: '{sanitizedPath}'", targetName, null, false, false, false); | ||
| } | ||
|
|
||
| bool includeInactive = @params["searchInactive"]?.ToObject<bool>() ?? false; | ||
| bool replaceExisting = @params["allowOverwrite"]?.ToObject<bool>() ?? false; | ||
| bool unlinkIfInstance = @params["unlinkIfInstance"]?.ToObject<bool>() ?? false; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file structure and read the specific section
cd MCPForUnity/Editor/Tools/Prefabs && wc -l ManagePrefabs.csRepository: CoplayDev/unity-mcp
Length of output: 83
🏁 Script executed:
# Read the area around lines 344-378 to understand context better
sed -n '330,385p' MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.csRepository: CoplayDev/unity-mcp
Length of output: 2752
🏁 Script executed:
# Search for all parameter naming conventions across the codebase
rg -i 'searchInactive|search_inactive|allowOverwrite|allow_overwrite|unlinkIfInstance|unlink_if_instance' --type cs -C 2Repository: CoplayDev/unity-mcp
Length of output: 11908
🏁 Script executed:
# Look for parameter normalization or case conversion logic
rg -i 'ToLower|normali|camelcase|snake_case|convert.*case' MCPForUnity/Editor --type cs -B 2 -A 2Repository: CoplayDev/unity-mcp
Length of output: 50375
🏁 Script executed:
# Check how `@params` is typically used and where it comes from
rg -n '@params\[' MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs | head -20Repository: CoplayDev/unity-mcp
Length of output: 1029
🏁 Script executed:
# Check if create_from_gameobject goes through BatchExecute
rg -n "create_from_gameobject|create_prefab" MCPForUnity/Editor --type cs -B 2 -A 2Repository: CoplayDev/unity-mcp
Length of output: 1656
🏁 Script executed:
# Check how commands are routed and if normalization applies
rg -n "CreateFromGameObject|CreatePrefab" MCPForUnity/Editor --type cs -B 2 -A 2Repository: CoplayDev/unity-mcp
Length of output: 2963
🏁 Script executed:
# Examine BatchExecute.NormalizeParameterKeys to understand when it's applied
sed -n '1,100p' MCPForUnity/Editor/Tools/BatchExecute.cs | head -80Repository: CoplayDev/unity-mcp
Length of output: 3067
🏁 Script executed:
# Check the ManagePrefabs command handler entry point
sed -n '1,100p' MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs | head -80Repository: CoplayDev/unity-mcp
Length of output: 3240
🏁 Script executed:
# Search for tool method dispatch to see if normalization applies universally
rg -n "HandleCommand|NormalizeParameterKeys" MCPForUnity/Editor/Tools --type cs -C 1Repository: CoplayDev/unity-mcp
Length of output: 6599
🏁 Script executed:
# Check CommandRegistry to understand how it invokes HandleCommand
sed -n '140,170p' MCPForUnity/Editor/Tools/CommandRegistry.csRepository: CoplayDev/unity-mcp
Length of output: 1207
🏁 Script executed:
# Look for direct invocation of ManagePrefabs.HandleCommand to see if normalization applies
rg -n "ManagePrefabs" MCPForUnity/Editor --type cs -B 1 -A 1Repository: CoplayDev/unity-mcp
Length of output: 3855
🏁 Script executed:
# Check if there's a central parameter normalization layer outside of BatchExecute
rg -n "NormalizeParameterKeys\|normalize.*param" MCPForUnity/Editor --type csRepository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
# Search for NormalizeParameterKeys usage
rg -n "NormalizeParameterKeys" MCPForUnity/Editor --type cs -B 1 -A 1Repository: CoplayDev/unity-mcp
Length of output: 830
🏁 Script executed:
# Check if there's a global command handler that normalizes params before routing
rg -n "ExecuteCommand\|InvokeCommand\|CommandRegistry" MCPForUnity/Editor --type cs | head -30Repository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
# Look at how commands are actually invoked from the MCP server level
fd -t f "\.cs$" MCPForUnity/Editor | xargs rg -l "HandleCommand.*@params" | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && head -50 {}'Repository: CoplayDev/unity-mcp
Length of output: 9490
🏁 Script executed:
# Check for documentation or comments about parameter naming convention
rg -n "parameter.*case|camelCase|snake_case|naming.*convention" MCPForUnity/Editor --type cs -B 2 -A 2 | head -50Repository: CoplayDev/unity-mcp
Length of output: 4010
🏁 Script executed:
# Look at the comment in ManageAsset more carefully
sed -n '47,55p' MCPForUnity/Editor/Tools/ManageAsset.csRepository: CoplayDev/unity-mcp
Length of output: 400
🏁 Script executed:
# Check if there's any server-side logic that normalizes before sending to editor
fd -t f "\.py$|\.ts$|\.js$" | xargs rg -l "camelCase\|snake_case\|normali" 2>/dev/null | head -5Repository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
# Verify the complete pattern in FindGameObjects for comparison
sed -n '25,35p' MCPForUnity/Editor/Tools/FindGameObjects.csRepository: CoplayDev/unity-mcp
Length of output: 558
Add fallback parameter names for consistency with codebase patterns.
Other tools (FindGameObjects, ManageAsset, Pagination) explicitly support both camelCase and snake_case parameter names for defensive handling. ManagePrefabs uses fallback for target/name but not for searchInactive, allowOverwrite, or unlinkIfInstance—causing silent failures when callers use snake_case variants (e.g., search_inactive). Match the established pattern by accepting both naming conventions.
♻️ Suggested compatibility fix
- bool includeInactive = `@params`["searchInactive"]?.ToObject<bool>() ?? false;
- bool replaceExisting = `@params`["allowOverwrite"]?.ToObject<bool>() ?? false;
- bool unlinkIfInstance = `@params`["unlinkIfInstance"]?.ToObject<bool>() ?? false;
+ bool includeInactive = `@params`["searchInactive"]?.ToObject<bool>()
+ ?? `@params`["search_inactive"]?.ToObject<bool>() ?? false;
+ bool replaceExisting = `@params`["allowOverwrite"]?.ToObject<bool>()
+ ?? `@params`["allow_overwrite"]?.ToObject<bool>() ?? false;
+ bool unlinkIfInstance = `@params`["unlinkIfInstance"]?.ToObject<bool>()
+ ?? `@params`["unlink_if_instance"]?.ToObject<bool>() ?? false;🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs` around lines 344 - 378,
The boolean options are only read from camelCase keys and miss snake_case
fallbacks, causing silent failures; update the three reads to probe both
variants from the `@params` dictionary (same pattern used for target/name): for
includeInactive use `@params`["searchInactive"] ?? `@params`["search_inactive"], for
replaceExisting use `@params`["allowOverwrite"] ?? `@params`["allow_overwrite"], and
for unlinkIfInstance use `@params`["unlinkIfInstance"] ??
`@params`["unlink_if_instance"], then call ToObject<bool>() and keep the ?? false
default so callers using either naming convention are accepted.
| string sanitizedPath = AssetPathUtility.SanitizeAssetPath(requestedPath); | ||
| if (sanitizedPath == null) | ||
| { | ||
| return (false, $"Invalid prefab path (path traversal detected): '{requestedPath}'", targetName, null, false, false, false); | ||
| } | ||
| if (string.IsNullOrEmpty(sanitizedPath)) | ||
| { | ||
| return (false, $"Invalid prefab path '{requestedPath}'. Path cannot be empty.", targetName, null, false, false, false); | ||
| } | ||
| if (!sanitizedPath.EndsWith(".prefab", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| sanitizedPath += ".prefab"; | ||
| } | ||
|
|
||
| // Validate path is within Assets folder | ||
| if (!sanitizedPath.StartsWith("Assets/", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return (false, $"Prefab path must be within the Assets folder. Got: '{sanitizedPath}'", targetName, null, false, false, false); | ||
| } |
There was a problem hiding this comment.
Validate sanitized prefab paths against invalid characters.
SanitizeAssetPath doesn’t reject invalid filename chars, so EnsureAssetDirectoryExists can throw and bubble as an internal error. Consider using IsValidAssetPath after appending the extension.
🐛 Proposed fix
if (!sanitizedPath.EndsWith(".prefab", StringComparison.OrdinalIgnoreCase))
{
sanitizedPath += ".prefab";
}
+
+ if (!AssetPathUtility.IsValidAssetPath(sanitizedPath))
+ {
+ return (false, $"Invalid prefab path '{requestedPath}'.", targetName, null, false, false, false);
+ }🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs` around lines 356 - 374,
Sanitized prefab paths currently aren't validated for invalid filename
characters before we call EnsureAssetDirectoryExists, causing potential
exceptions; after you set sanitizedPath (and after appending ".prefab" if
needed) call AssetPathUtility.IsValidAssetPath(sanitizedPath) and, if it returns
false, return a failure tuple with a clear message (similar format to existing
returns) instead of continuing, so EnsureAssetDirectoryExists and subsequent
code never receive an invalid path; reference the sanitizedPath variable,
AssetPathUtility.SanitizeAssetPath, AssetPathUtility.IsValidAssetPath, and
EnsureAssetDirectoryExists when making the change.
…atevertogo/unity-mcp into fix/last-child-pseudo-class-warnings
- Add force parameter to save_open_stage for automated workflows where isDirty may not be correctly set - Use PrefabUtility.SaveAsPrefabAsset for dialog-free saving - Mark prefab stage scene dirty when modifying GameObjects in prefab mode - Skip save when no changes and force=false (prevents false dirty flag) The force parameter ensures reliable saving in CI/automation scenarios where Unity dirty tracking may be inconsistent with programmatic changes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
I made a PR for your branch fix a couple of things -- take a look what you think, and if you're okay with that, accept the PR and we can merge this in. Thanks for your good work here. |
|
could you hold on a moment? I realized that cli(prefab.py) also need some corresponding adjustments. I’ll make those quick changes to the CLI after I merge this, and then we can merge everything into beta
|
…aram fix: improve prefab stage save for automated workflows
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs`:
- Around line 242-253: The code always marks the active scene dirty which misses
modified GameObjects in non-active (additive) scenes; update the logic after
EditorUtility.SetDirty(targetGo) to, when not in a prefab stage, use the target
GameObject's scene (targetGo.scene) if it is valid and loaded instead of
unconditionally calling EditorSceneManager.GetActiveScene(); call
EditorSceneManager.MarkSceneDirty(targetGo.scene) when targetGo.scene.IsValid()
(fall back to GetActiveScene() only if the target scene is invalid).
| EditorUtility.SetDirty(targetGo); | ||
|
|
||
| // Mark the appropriate scene as dirty (handles both regular scenes and prefab stages) | ||
| var prefabStage = PrefabStageUtility.GetCurrentPrefabStage(); | ||
| if (prefabStage != null) | ||
| { | ||
| EditorSceneManager.MarkSceneDirty(prefabStage.scene); | ||
| } | ||
| else | ||
| { | ||
| EditorSceneManager.MarkSceneDirty(EditorSceneManager.GetActiveScene()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n MCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs | head -300Repository: CoplayDev/unity-mcp
Length of output: 12509
🏁 Script executed:
find . -name "*.cs" -type f | xargs grep -l "ManageGameObjectCommon" | head -5Repository: CoplayDev/unity-mcp
Length of output: 9166
🏁 Script executed:
cat -n MCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.csRepository: CoplayDev/unity-mcp
Length of output: 11621
🏁 Script executed:
find . -name "GameObjectLookup.cs" -type fRepository: CoplayDev/unity-mcp
Length of output: 111
🏁 Script executed:
cat -n MCPForUnity/Editor/Helpers/GameObjectLookup.csRepository: CoplayDev/unity-mcp
Length of output: 16010
Mark the GameObject's actual scene dirty, not always the active scene.
When searching by component (by_component), the target object can be found in any loaded scene via FindObjectsByType/FindObjectsOfType. Marking only the active scene leaves modified GameObjects in additive scenes unmarked, risking lost changes. Use targetGo.scene when valid.
🐛 Proposed fix
- EditorSceneManager.MarkSceneDirty(EditorSceneManager.GetActiveScene());
+ var sceneToDirty = targetGo.scene.IsValid()
+ ? targetGo.scene
+ : EditorSceneManager.GetActiveScene();
+ EditorSceneManager.MarkSceneDirty(sceneToDirty);📝 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.
| EditorUtility.SetDirty(targetGo); | |
| // Mark the appropriate scene as dirty (handles both regular scenes and prefab stages) | |
| var prefabStage = PrefabStageUtility.GetCurrentPrefabStage(); | |
| if (prefabStage != null) | |
| { | |
| EditorSceneManager.MarkSceneDirty(prefabStage.scene); | |
| } | |
| else | |
| { | |
| EditorSceneManager.MarkSceneDirty(EditorSceneManager.GetActiveScene()); | |
| } | |
| EditorUtility.SetDirty(targetGo); | |
| // Mark the appropriate scene as dirty (handles both regular scenes and prefab stages) | |
| var prefabStage = PrefabStageUtility.GetCurrentPrefabStage(); | |
| if (prefabStage != null) | |
| { | |
| EditorSceneManager.MarkSceneDirty(prefabStage.scene); | |
| } | |
| else | |
| { | |
| var sceneToDirty = targetGo.scene.IsValid() | |
| ? targetGo.scene | |
| : EditorSceneManager.GetActiveScene(); | |
| EditorSceneManager.MarkSceneDirty(sceneToDirty); | |
| } |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs` around lines 242 -
253, The code always marks the active scene dirty which misses modified
GameObjects in non-active (additive) scenes; update the logic after
EditorUtility.SetDirty(targetGo) to, when not in a prefab stage, use the target
GameObject's scene (targetGo.scene) if it is valid and loaded instead of
unconditionally calling EditorSceneManager.GetActiveScene(); call
EditorSceneManager.MarkSceneDirty(targetGo.scene) when targetGo.scene.IsValid()
(fall back to GetActiveScene() only if the target scene is invalid).
|
The last thing I would suggest is adding a small test suite to the edit mode test folder for testing the prefab tooling and improvements the PR makes. Maybe like 5-10 tests that show it working, CRUD on prefabs, that the save dialog doesn't get things stuck, etc. I can do it later or you can try if you have time. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Server/src/cli/commands/prefab.py`:
- Around line 183-192: The current loop that prints nested prefab info accesses
item['prefab']['assetPath'] directly and can KeyError if assetPath is missing;
update the prefab handling in the compact branch (where response_data, data,
items, show_prefab_info are used) to access the prefab dict with .get('prefab',
{}) and then use .get('assetPath') (providing a safe fallback string or empty
value) before formatting prefab_info, so nested roots without assetPath won't
raise and will print a sensible fallback.
| if compact and response_data.get("success") and response_data.get("data"): | ||
| data = response_data["data"] | ||
| items = data.get("items", []) | ||
| for item in items: | ||
| indent = " " * item.get("path", "").count("/") | ||
| prefab_info = "" | ||
| if show_prefab_info and item.get("prefab", {}).get("isNestedRoot"): | ||
| prefab_info = f" [nested: {item['prefab']['assetPath']}]" | ||
| click.echo(f"{indent}{item.get('name')}{prefab_info}") | ||
| click.echo(f"\nTotal: {data.get('total', 0)} objects") |
There was a problem hiding this comment.
Guard against missing assetPath in nested prefab info.
item['prefab']['assetPath'] can raise KeyError if the backend omits the field for a nested root. Safer to use .get(...) on the prefab dict.
🛠️ Suggested fix
- if show_prefab_info and item.get("prefab", {}).get("isNestedRoot"):
- prefab_info = f" [nested: {item['prefab']['assetPath']}]"
+ prefab = item.get("prefab", {})
+ if show_prefab_info and prefab.get("isNestedRoot"):
+ prefab_info = f" [nested: {prefab.get('assetPath', 'N/A')}]"📝 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.
| if compact and response_data.get("success") and response_data.get("data"): | |
| data = response_data["data"] | |
| items = data.get("items", []) | |
| for item in items: | |
| indent = " " * item.get("path", "").count("/") | |
| prefab_info = "" | |
| if show_prefab_info and item.get("prefab", {}).get("isNestedRoot"): | |
| prefab_info = f" [nested: {item['prefab']['assetPath']}]" | |
| click.echo(f"{indent}{item.get('name')}{prefab_info}") | |
| click.echo(f"\nTotal: {data.get('total', 0)} objects") | |
| if compact and response_data.get("success") and response_data.get("data"): | |
| data = response_data["data"] | |
| items = data.get("items", []) | |
| for item in items: | |
| indent = " " * item.get("path", "").count("/") | |
| prefab_info = "" | |
| prefab = item.get("prefab", {}) | |
| if show_prefab_info and prefab.get("isNestedRoot"): | |
| prefab_info = f" [nested: {prefab.get('assetPath', 'N/A')}]" | |
| click.echo(f"{indent}{item.get('name')}{prefab_info}") | |
| click.echo(f"\nTotal: {data.get('total', 0)} objects") |
🤖 Prompt for AI Agents
In `@Server/src/cli/commands/prefab.py` around lines 183 - 192, The current loop
that prints nested prefab info accesses item['prefab']['assetPath'] directly and
can KeyError if assetPath is missing; update the prefab handling in the compact
branch (where response_data, data, items, show_prefab_info are used) to access
the prefab dict with .get('prefab', {}) and then use .get('assetPath')
(providing a safe fallback string or empty value) before formatting prefab_info,
so nested roots without assetPath won't raise and will print a sensible
fallback.
…ure dirty state tracking
|
Done. Added ManagePrefabsCrudTests.cs You can check it out and run the tests.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsCrudTests.cs`:
- Line 82: The local variable "instance" in ManagePrefabsCrudTests is declared
as "GameObject instance = null" but never used; remove that declaration and also
remove the corresponding cleanup reference that attempts to destroy or set
"instance" (the no-op cleanup in the test teardown method) so there is no unused
variable or redundant cleanup call.
♻️ Duplicate comments (2)
MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs (2)
391-393: Add fallback parameter names for snake_case compatibility.Other tools in the codebase (FindGameObjects, ManageAsset) support both camelCase and snake_case parameter names. These boolean options only read camelCase, causing silent failures when callers use
search_inactive,allow_overwrite, orunlink_if_instance.♻️ Suggested fix
- bool includeInactive = `@params`["searchInactive"]?.ToObject<bool>() ?? false; - bool replaceExisting = `@params`["allowOverwrite"]?.ToObject<bool>() ?? false; - bool unlinkIfInstance = `@params`["unlinkIfInstance"]?.ToObject<bool>() ?? false; + bool includeInactive = `@params`["searchInactive"]?.ToObject<bool>() + ?? `@params`["search_inactive"]?.ToObject<bool>() ?? false; + bool replaceExisting = `@params`["allowOverwrite"]?.ToObject<bool>() + ?? `@params`["allow_overwrite"]?.ToObject<bool>() ?? false; + bool unlinkIfInstance = `@params`["unlinkIfInstance"]?.ToObject<bool>() + ?? `@params`["unlink_if_instance"]?.ToObject<bool>() ?? false;
380-389: Consider validating the final path withIsValidAssetPath.After appending the
.prefabextension, the path isn't validated for invalid filename characters.SanitizeAssetPathdoesn't reject characters like*,?, etc., which could causeEnsureAssetDirectoryExiststo throw an unclear error.♻️ Suggested fix
if (!sanitizedPath.EndsWith(".prefab", StringComparison.OrdinalIgnoreCase)) { sanitizedPath += ".prefab"; } + if (!AssetPathUtility.IsValidAssetPath(sanitizedPath)) + { + return (false, $"Invalid prefab path '{requestedPath}'.", targetName, null, false, false, false); + } + // Validate path is within Assets folder
🧹 Nitpick comments (2)
MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs (1)
200-221: Consider optimizing the save operations.Marking all children dirty (lines 200-207) ensures changes are captured but may be slow for large prefabs. Additionally,
AssetDatabase.SaveAssets()(line 220) afterSaveAsPrefabAsset(line 212) is partially redundant sinceSaveAsPrefabAssetalready persists the prefab. The explicit calls do ensure database synchronization for automated workflows, so this is acceptable if reliability is prioritized over performance.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsCrudTests.cs (1)
445-451: Minor: Redundant cleanup path.
parentPathreturned fromCreateTestPrefab("ParentPrefab")is the same asPath.Combine(TempDirectory, "ParentPrefab.prefab"), making line 448 redundant.♻️ Suggested fix
finally { SafeDeleteAsset(parentPath); - SafeDeleteAsset(Path.Combine(TempDirectory, "ParentPrefab.prefab").Replace('\\', '/')); SafeDeleteAsset(Path.Combine(TempDirectory, "ChildPrefab.prefab").Replace('\\', '/')); SafeDeleteAsset(Path.Combine(TempDirectory, "NestedContainer.prefab").Replace('\\', '/')); }
| // Create an initial prefab | ||
| string initialPrefabPath = Path.Combine(TempDirectory, "Original.prefab").Replace('\\', '/'); | ||
| GameObject sourceObject = new GameObject("SourceObject"); | ||
| GameObject instance = null; |
There was a problem hiding this comment.
Remove unused variable instance.
The variable instance is declared but never assigned a value other than null, making the cleanup on line 125 a no-op.
♻️ Suggested fix
string initialPrefabPath = Path.Combine(TempDirectory, "Original.prefab").Replace('\\', '/');
GameObject sourceObject = new GameObject("SourceObject");
- GameObject instance = null;And remove line 125:
finally
{
SafeDeleteAsset(initialPrefabPath);
SafeDeleteAsset(Path.Combine(TempDirectory, "NewFromLinked.prefab").Replace('\\', '/'));
if (sourceObject != null) UnityEngine.Object.DestroyImmediate(sourceObject, true);
- if (instance != null) UnityEngine.Object.DestroyImmediate(instance, true);
}🤖 Prompt for AI Agents
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsCrudTests.cs`
at line 82, The local variable "instance" in ManagePrefabsCrudTests is declared
as "GameObject instance = null" but never used; remove that declaration and also
remove the corresponding cleanup reference that attempts to destroy or set
"instance" (the no-op cleanup in the test teardown method) so there is no unused
variable or redundant cleanup call.
|
Bug fix after testing:Added AssetDatabase.Refresh() to ensure changes take effect immediately after saving. |
|
Nice work @whatevertogo -- this is a great addition. Thanks for adding the tests and fixes. |
|
just tested the prefab edit, the prefab save tool seems broken. save_open_stage returns success: true but the prefab is not actually saved (the * in the Unity Editor indicates unsaved changes, and isDirty: true in the response also hints at this). |
|
The save_open_stage function returns success: true and the prefab is being saved to disk, but Unity Editor still shows * (unsaved indicator) and isDirty: true in the response. This appears to be a Unity Editor UI state synchronization issue—the save operation works, but the Prefab Stage's dirty state isn't being cleared properly. This seems related to the SaveAsPrefabAsset approach that was added. Could you take a look? Perhaps we could revert to the previous script version that i wrote was working? @dsarno, i believe we must use this(Fixed issue where component additions were lost after closing prefab stage (d0cb1ba, 943e7ad)
|
|
@cyanxwh Good catch, thanks. I actually reworked the Prefab tools to work headlessly (i.e. without a stage). The Unity API lets you do everything you need to do with prefabs without the stage -- and the stage is what causes the unwanted save dialog issues. See: #635. You can test it from our |
@whatevertogo I don't think we need it -- if you're able to try my newer "headless" prefab approach, you'll see it works with no stage and no save issues. |
Got it, thanks @dsarno.Looking forward to the headless prefab approach—it sounds like the ideal solution. Let me know when it's ready! |
|
@whatevertogo It's already live -- see this comment: #611 (comment) |
Nice work! I'll give it a try. |
3 participants
Description
This PR implements comprehensive enhancements to the prefab management system in MCP for Unity, focusing on
simplification, improved validation, and better developer experience. The changes streamline prefab operations, add
nested prefab support, and fix several critical bugs related to prefab stage handling.
Type of Change
Changes Made
Core Enhancements
Nested Prefab Support (d0c7135)
Path Validation & Security (14e3847, 72de29d)
New Utility Class (228f9fb)
Bug Fixes
pattern
Code Quality Improvements
New Features
(5859fec)
Testing/Screenshots/Recordings
All changes tested with Unity Editor including:
Documentation Updates
Related Issues
#97
Additional Notes
All changes follow Unity's recommended practices for prefab stage operations in Unity 2021.3+
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.