Merged
Conversation
Contributor
Reviewer's GuideImplements a prefab material persistence fix, improves serialized int/long handling, adds a close_prefab_stage editor action, and updates MCP Unity skill/docs for new graphics, packages, scripting, and screenshot workflows. Sequence diagram for prefab creation with runtime material persistencesequenceDiagram
actor EditorUser
participant ManagePrefabs
participant PersistRuntimeMaterials
participant Renderer
participant AssetDatabase
participant PrefabStage as CreatePrefabAsset
EditorUser->>ManagePrefabs: manage_prefabs(action="create_from_gameobject", ...)
ManagePrefabs->>ManagePrefabs: Validate parameters and sourceObject
Note over ManagePrefabs: Step 7: Persist runtime-only materials
ManagePrefabs->>PersistRuntimeMaterials: PersistRuntimeMaterials(root, prefabPath)
PersistRuntimeMaterials->>Renderer: GetComponentsInChildren<Renderer>(includeInactive=true)
loop For each renderer
PersistRuntimeMaterials->>Renderer: Get sharedMaterials
loop For each material slot
PersistRuntimeMaterials->>Renderer: Check material instance and property block
alt Runtime instance or property block colors
PersistRuntimeMaterials->>AssetDatabase: AssetPathUtility.SanitizeAssetPath(matPath)
PersistRuntimeMaterials->>AssetDatabase: IsValidFolder(materialsFolder)
alt Folder missing
PersistRuntimeMaterials->>AssetDatabase: CreateFolder(parentDir, folderName)
end
PersistRuntimeMaterials->>AssetDatabase: LoadAssetAtPath<Material>(matPath)
alt Material asset missing
PersistRuntimeMaterials->>RenderPipeline as RenderPipelineUtility: ResolveShader("Standard")
PersistRuntimeMaterials->>AssetDatabase: CreateAsset(new Material(shader), matPath)
end
PersistRuntimeMaterials->>Renderer: Apply runtime properties to persisted material
PersistRuntimeMaterials->>Renderer: Replace sharedMaterials[slot] with persisted asset
end
end
PersistRuntimeMaterials->>Renderer: Undo.RecordObject(renderer, "Persist runtime materials for prefab")
PersistRuntimeMaterials->>Renderer: Clear property blocks
PersistRuntimeMaterials->>AssetDatabase: SaveAssets
end
PersistRuntimeMaterials-->>ManagePrefabs: (count, paths)
Note over ManagePrefabs,PrefabStage: Step 8: Create prefab asset
ManagePrefabs->>PrefabStage: CreatePrefabAsset(sourceObject, prefabPath, replaceExisting)
PrefabStage-->>ManagePrefabs: GameObject prefabInstance
alt Prefab creation succeeded
ManagePrefabs->>ManagePrefabs: Build SuccessResponse
ManagePrefabs-->>EditorUser: { wasCreated, wasReplaced, childCount, materialsPersisted }
else Prefab creation failed
ManagePrefabs-->>EditorUser: ErrorResponse("Failed to create prefab asset")
end
Sequence diagram for manage_editor close_prefab_stage tool from Python to C#sequenceDiagram
actor MCPClient
participant PythonTool as manage_editor.py
participant UnityTransport as send_with_unity_instance
participant CSharpTool as ManageEditor.HandleCommand
participant PrefabAPI as PrefabStageUtility/StageUtility
MCPClient->>PythonTool: manage_editor(action="close_prefab_stage")
PythonTool->>UnityTransport: async_send_command_with_retry("manage_editor", { action: close_prefab_stage })
UnityTransport->>CSharpTool: HandleCommand(params)
CSharpTool->>CSharpTool: Read action from ToolParams
CSharpTool->>PrefabAPI: GetCurrentPrefabStage()
alt In prefab stage
PrefabAPI-->>CSharpTool: prefabStage (with assetPath)
CSharpTool->>PrefabAPI: GoToMainStage()
CSharpTool-->>UnityTransport: SuccessResponse({ message, prefabPath })
else Not in prefab stage
PrefabAPI-->>CSharpTool: null
CSharpTool-->>UnityTransport: SuccessResponse({ message: "Not currently in prefab editing mode." })
end
UnityTransport-->>PythonTool: SuccessResponse
PythonTool-->>MCPClient: Result payload (message, optional prefabPath)
Class diagram for prefab material persistence and serialized int/long handlingclassDiagram
class ManagePrefabs {
+object HandleCommand(JObject @params)
-static object CreatePrefabFromGameObject(JObject @params)
-static (int count, List~string~ paths) PersistRuntimeMaterials(GameObject root, string prefabPath)
-static bool HasPropertyBlockColors(Renderer renderer, int slot)
-static void ApplyPropertyBlockToMaterial(Renderer renderer, int slot, Material mat)
}
class ParamCoercion {
+static int CoerceInt(JToken token, int defaultValue)
+static long CoerceLong(JToken token, long defaultValue)
+static bool CoerceBool(JToken token, bool defaultValue)
+static float CoerceFloat(JToken token, float defaultValue)
}
class GraphicsHelpers {
+static object ReadSerializedValue(SerializedProperty prop)
+static bool SetSerializedValue(SerializedProperty prop, JToken value)
}
class ComponentOps {
-static bool SetViaSerializedProperty(Component component, string propertyName, string normalizedName, JToken value, ref string error)
-static bool SetSerializedPropertyRecursive(SerializedProperty prop, JToken value, ref string error)
}
class ManageScriptableObject {
-static bool TrySetValueRecursive(SerializedProperty prop, JToken valueToken, ref string message)
}
class ManageEditor {
+object HandleCommand(JObject @params)
-static object ClosePrefabStage()
-static object DeployPackage()
-static object RestorePackage()
}
class UnityEditor_APIs {
<<utility>>
PrefabStageUtility
StageUtility
AssetDatabase
EditorUtility
Undo
}
ManagePrefabs --> UnityEditor_APIs : uses
ManagePrefabs ..> ParamCoercion : (indirect via materials)
GraphicsHelpers ..> ParamCoercion : uses
ComponentOps ..> ParamCoercion : uses
ManageScriptableObject ..> ParamCoercion : uses
ManageEditor --> UnityEditor_APIs : uses PrefabStageUtility, StageUtility
ManagePrefabs --> Renderer : scans and updates
ManagePrefabs --> Material : creates and persists
Flow diagram for manage_editor close_prefab_stage actionflowchart TD
Start["manage_editor(action=close_prefab_stage)"] --> GetStage["GetCurrentPrefabStage()
via PrefabStageUtility"]
GetStage -->|"prefabStage is null"| NotInStage["Return SuccessResponse
'Not currently in prefab editing mode.'"]
GetStage -->|"prefabStage not null"| ReadPath["prefabPath = prefabStage.assetPath"]
ReadPath --> ExitStage["StageUtility.GoToMainStage()
(exit prefab editing)"]
ExitStage --> ReturnSuccess["Return SuccessResponse
with prefabPath in data"]
GetStage --> Err["Exception thrown"]
Err --> ReturnError["Return ErrorResponse
'Error closing prefab stage: ...'"]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR updates docs and the Unity MCP skill, adds prefab/material persistence and editor actions, tightens integer/long coercion and SerializedProperty handling, changes screenshot tooling to camera-based calls, and adjusts script compilation/workflow behavior (auto-refresh + wait-for-compilation). It also removes wait_for_completion from manage_editor and adds close_prefab_stage. Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new integer handling in
ComponentOps.SetSerializedPropertyRecursiveandManageScriptableObject.TrySetValueRecursivenow treatsnull/missing values as hard errors; if some integer fields are meant to be optional or left unchanged, consider preserving the previous "no-op on null" behavior instead of failing the whole set. - In
ManageEditor.ClosePrefabStage, the catch block returns an error message but drops the original exception; consider logging the exception (e.g., viaMcpLog.Error) to aid debugging when prefab stage transitions fail. - The
EnsureEditorExtensionModehelper performs string-based injection into the UXML root tag; to avoid edge cases with XML declarations, comments, or unconventional formatting, consider switching to an XML-based approach (e.g.,XDocument) so the attribute is added in a more robust and schema-safe way.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new integer handling in `ComponentOps.SetSerializedPropertyRecursive` and `ManageScriptableObject.TrySetValueRecursive` now treats `null`/missing values as hard errors; if some integer fields are meant to be optional or left unchanged, consider preserving the previous "no-op on null" behavior instead of failing the whole set.
- In `ManageEditor.ClosePrefabStage`, the catch block returns an error message but drops the original exception; consider logging the exception (e.g., via `McpLog.Error`) to aid debugging when prefab stage transitions fail.
- The `EnsureEditorExtensionMode` helper performs string-based injection into the UXML root tag; to avoid edge cases with XML declarations, comments, or unconventional formatting, consider switching to an XML-based approach (e.g., `XDocument`) so the attribute is added in a more robust and schema-safe way.
## Individual Comments
### Comment 1
<location path="MCPForUnity/Editor/Helpers/ComponentOps.cs" line_range="543-552" />
<code_context>
prop.boolValue = ParamCoercion.CoerceBool(value, false);
return true;
case SerializedPropertyType.Integer:
- prop.intValue = ParamCoercion.CoerceInt(value, 0);
+ if (prop.type == "long")
+ prop.longValue = ParamCoercion.CoerceLong(value, 0);
</code_context>
<issue_to_address>
**issue (bug_risk):** Integer coercion now silently maps invalid inputs to 0 instead of surfacing an error.
The previous approach used `int.MinValue` as a sentinel and surfaced an error on parse failure. Now, anything non-null is passed to `CoerceInt`/`CoerceLong` with a default of 0, so values like "abc" or malformed numbers become 0 instead of failing. This can hide data issues and unintentionally overwrite values with 0. Please add a way to detect coercion failures (so you can distinguish a real 0 from a failed parse) and apply the same fix in `ManageScriptableObject.TrySetValueRecursive` for consistency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves Unity-side tooling correctness (prefab/material persistence and int64 serialized property handling), adds an editor control action for exiting Prefab Stage, and refreshes contributor/skill documentation to match current tool behavior and transport architecture.
Changes:
- Persist runtime-only materials to
.matassets during prefab creation so material appearance survives serialization. - Add
ParamCoercion.CoerceLong()and update serialized-property read/write helpers to properly handlelongfields (avoiding int32 truncation). - Add
manage_editor(action="close_prefab_stage"), remove the unusedwaitForCompletionparameter, and sync/refresh skill + architecture docs.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| unity-mcp-skill/references/workflows.md | Updates script workflows to wait on compilation via editor state; adds prefab instantiation guidance; adds batch discovery note. |
| unity-mcp-skill/references/tools-reference.md | Adds prefab instantiation example and documents close_prefab_stage. |
| unity-mcp-skill/SKILL.md | Updates best practices around script compilation waits and prefab instantiation guidance. |
| Server/src/services/tools/manage_editor.py | Adds close_prefab_stage action; removes wait_for_completion plumbing. |
| MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs | Adds PersistRuntimeMaterials() and surfaces materialsPersisted in the response. |
| MCPForUnity/Editor/Tools/ManageUI.cs | Injects editor-extension-mode for UXML writes; improves capture-in-progress error response payload. |
| MCPForUnity/Editor/Tools/ManageScriptableObject.cs | Writes longValue for long-typed integer properties using CoerceLong. |
| MCPForUnity/Editor/Tools/ManageEditor.cs | Adds close_prefab_stage action and removes unused waitForCompletion param. |
| MCPForUnity/Editor/Tools/Graphics/GraphicsHelpers.cs | Reads/writes long via prop.longValue when applicable. |
| MCPForUnity/Editor/Tools/GameObjects/ManageGameObject.cs | Updates prefab-asset error guidance to match current prefab tooling. |
| MCPForUnity/Editor/Helpers/ParamCoercion.cs | Adds CoerceLong() helper. |
| MCPForUnity/Editor/Helpers/ComponentOps.cs | Uses CoerceLong and adds ObjectReference readback verification. |
| CLAUDE.md | Updates transport diagram and expands repo architecture/tooling guidance. |
| .claude/skills/unity-mcp-skill/references/workflows.md | Syncs skill docs (graphics/packages/camera screenshot updates, prefab instantiation, batch discovery). |
| .claude/skills/unity-mcp-skill/references/tools-reference.md | Syncs tool reference additions for graphics/packages and close_prefab_stage. |
| .claude/skills/unity-mcp-skill/SKILL.md | Syncs skill best practices (camera screenshots, compilation waits, package section). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+1858
to
+1869
| if (contents.Contains("editor-extension-mode")) | ||
| return contents; | ||
|
|
||
| int idx = contents.IndexOf("<ui:UXML", StringComparison.Ordinal); | ||
| if (idx < 0) | ||
| idx = contents.IndexOf("<UXML", StringComparison.Ordinal); | ||
| if (idx < 0) | ||
| return contents; | ||
|
|
||
| int closeTag = contents.IndexOf('>', idx); | ||
| if (closeTag < 0) | ||
| return contents; |
| // Case 2: Material exists but is not a persistent asset (runtime instance). | ||
| bool isRuntimeInstance = mat != null && !EditorUtility.IsPersistent(mat); | ||
| bool isNullWithPropertyBlock = mat == null && HasPropertyBlockColors(renderer, slot); | ||
| bool isNullMaterial = mat == null && !isNullWithPropertyBlock; |
Comment on lines
+302
to
+306
| // Derive a unique asset path from the GameObject name and slot | ||
| string goName = renderer.gameObject.name.Replace(" ", "_"); | ||
| string suffix = slot > 0 ? $"_slot{slot}" : ""; | ||
| string matPath = $"{materialsFolder}/{goName}{suffix}_mat.mat"; | ||
| matPath = AssetPathUtility.SanitizeAssetPath(matPath); |
Comment on lines
+355
to
+359
| // Clear any property blocks now that the material is persisted | ||
| for (int slot = 0; slot < sharedMats.Length; slot++) | ||
| { | ||
| renderer.SetPropertyBlock(null, slot); | ||
| } |
| { | ||
| MaterialPropertyBlock block = new MaterialPropertyBlock(); | ||
| renderer.GetPropertyBlock(block, slot); | ||
| return !block.isEmpty; |
Comment on lines
926
to
+936
| case SerializedPropertyType.Integer: | ||
| // Use ParamCoercion for robust int parsing | ||
| int intVal = ParamCoercion.CoerceInt(valueToken, int.MinValue); | ||
| if (intVal == int.MinValue && valueToken?.Type != JTokenType.Integer) | ||
| if (valueToken == null || valueToken.Type == JTokenType.Null) | ||
| { | ||
| // Double-check: if it's actually int.MinValue or failed to parse | ||
| if (valueToken == null || valueToken.Type == JTokenType.Null || | ||
| (valueToken.Type == JTokenType.String && !int.TryParse(valueToken.ToString(), out _))) | ||
| { | ||
| message = "Expected integer value."; | ||
| return false; | ||
| } | ||
| message = "Expected integer value."; | ||
| return false; | ||
| } | ||
| prop.intValue = intVal; | ||
| message = "Set int."; | ||
| if (prop.type == "long") | ||
| prop.longValue = ParamCoercion.CoerceLong(valueToken, 0); | ||
| else | ||
| prop.intValue = ParamCoercion.CoerceInt(valueToken, 0); | ||
| message = prop.type == "long" ? "Set long." : "Set int."; |
Comment on lines
543
to
+552
| case SerializedPropertyType.Integer: | ||
| int intVal = ParamCoercion.CoerceInt(value, int.MinValue); | ||
| if (intVal == int.MinValue && value?.Type != JTokenType.Integer) | ||
| if (value == null || value.Type == JTokenType.Null) | ||
| { | ||
| if (value == null || value.Type == JTokenType.Null || | ||
| (value.Type == JTokenType.String && !int.TryParse(value.ToString(), out _))) | ||
| { | ||
| error = "Expected integer value."; | ||
| return false; | ||
| } | ||
| error = "Expected integer value."; | ||
| return false; | ||
| } | ||
| prop.intValue = intVal; | ||
| if (prop.type == "long") | ||
| prop.longValue = ParamCoercion.CoerceLong(value, 0); | ||
| else | ||
| prop.intValue = ParamCoercion.CoerceInt(value, 0); |
![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: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/unity-mcp-skill/references/workflows.md:
- Around line 119-121: Several wait loops poll the editor state using the wrong
key name "isCompiling" which should be "is_compiling"; update every occurrence
that reads mcpforunity://editor/state (notably the "manage_script update
auto-triggers import + compile" snippet and the other occurrences around the
noted ranges) to check "is_compiling" instead of "isCompiling" so the wait logic
reads the correct field; search for the symbol isCompiling and replace those
checks with is_compiling while ensuring the surrounding logic that reads
editor/state remains unchanged.
In @.claude/skills/unity-mcp-skill/SKILL.md:
- Line 198: Update the compile-wait instruction to use the documented
editor_state field name: replace the camelCase token "isCompiling" in the line
"Read mcpforunity://editor/state → wait until isCompiling == false" with the
snake_case "is_compiling" so it matches the editor_state schema and avoids
breaking integrations that expect editor_state.is_compiling.
In `@CLAUDE.md`:
- Line 40: Update the description of HTTP isolation to clearly distinguish
local/shared-server behavior from remote-hosted behavior: state that in HTTP
mode a multi-agent setup on a single shared Python server uses session isolation
via client_id for per-client sessions, and explicitly add that in remote-hosted
transport the identity is resolved from the API key to an authenticated user_id
which is used for tenant scoping; reference the existing phrase "Session
isolation via `client_id`" and add the clause about resolving API key to
`user_id` for remote-hosted scoping to remove ambiguity.
In `@MCPForUnity/Editor/Tools/ManageEditor.cs`:
- Around line 372-385: Before calling StageUtility.GoToMainStage() in
ClosePrefabStage(), check the current prefab stage for unsaved changes via
PrefabStageUtility.GetCurrentPrefabStage() and prefabStage.hasUnsavedChanges; if
true, deterministically resolve them by saving the scene with
EditorSceneManager.SaveScene(prefabStage.scene) (and handle/save failure) or
explicitly discard changes via the prefab stage API before calling
StageUtility.GoToMainStage(); update ClosePrefabStage to perform that check and
only switch stages after saving or discarding to avoid blocking in headless/CI
environments.
In `@MCPForUnity/Editor/Tools/ManageScriptableObject.cs`:
- Around line 927-937: The code currently coerces non-numeric JToken values
(e.g., "abc") to 0 via ParamCoercion.CoerceLong/CoerceInt causing silent
corruption; update the block that handles valueToken/prop.type to first validate
that valueToken represents a numeric value (JTokenType.Integer or Float) or a
numeric string that parses successfully, and if it does not, set message =
"Expected integer value." and return false; only call
ParamCoercion.CoerceLong/CoerceInt and assign prop.longValue/prop.intValue when
the token passes that numeric check, otherwise reject before coercion.
In `@MCPForUnity/Editor/Tools/ManageUI.cs`:
- Around line 184-185: The current use of
contents.Contains("editor-extension-mode") in EnsureEditorExtensionMode can
match tokens anywhere (comments/child nodes) and cause incorrect behavior;
update EnsureEditorExtensionMode (and callers that manipulate the contents
string) to only check and inject the attribute on the root UXML opening tag by
parsing or using a targeted regex that finds the first opening tag (e.g., match
the first "<...>" start tag) and then test/insert editor-extension-mode into
that tag rather than doing a global Contains on the entire contents string;
ensure the same scoped approach is applied to the other occurrences around lines
where contents is modified so the attribute is only considered/inserted on the
root element.
In `@MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs`:
- Around line 302-347: matPath currently uses only renderer.gameObject.name and
slot which causes different renderers with the same name to collide; change the
matPath generation in ManagePrefabs so it includes a stable unique identifier
for the prefab instance (e.g. the prefab asset path or prefab GUID or a
sanitized hierarchy path) along with renderer.gameObject.name and slot before
calling AssetPathUtility.SanitizeAssetPath. Locate the matPath build around the
variables goName, suffix and materialsFolder, obtain a prefab identifier via
PrefabUtility.GetPrefabAssetPathOfNearestInstanceRoot(renderer.gameObject) or
PrefabUtility.GetPrefabInstanceHandle/GetLocalIdentifierInFile and incorporate a
short hash or sanitized version of that identifier into the filename (or folder)
to ensure distinct persisted .mat files for different prefabs while still using
AssetPathUtility.SanitizeAssetPath on the final path.
- Around line 355-359: HasPropertyBlockColors() and
ApplyPropertyBlockToMaterial() currently treat any non-empty
MaterialPropertyBlock as a color candidate and then the code clears all slots on
the renderer (renderer.SetPropertyBlock(null, slot)), which wipes unrelated
overrides; narrow detection to only consider color properties and only clear the
specific slot after migration. Update HasPropertyBlockColors() to check for
presence of _BaseColor or _Color (use block.TryGetColor or block.HasProperty for
those names) instead of !block.isEmpty, change ApplyPropertyBlockToMaterial() to
remove/ignore only the color properties from the block and then call
renderer.SetPropertyBlock(null, slot) only for that same slot when the block no
longer contains any other properties; apply the same per-slot-only logic to the
similar block around the second occurrence (the 376-380 area) and stop clearing
every slot in sharedMats.
- Around line 295-300: The current check uses isNullWithPropertyBlock (which is
true only when mat == null) so renderers with a persistent shared material plus
a MaterialPropertyBlock color override are skipped; change the logic to detect
property-block colors regardless of mat: call HasPropertyBlockColors(renderer,
slot) into a boolean like hasPropertyBlock and replace the continue condition
with if (!isRuntimeInstance && !hasPropertyBlock) continue so property-block
tints on non-null shared materials are handled.
In `@Server/src/services/tools/manage_editor.py`:
- Line 13: The endpoint description in manage_editor.py still mentions boolean
argument handling ("Tip: pass booleans as true/false; ... 'true'/'false' are
accepted") even though the endpoint no longer accepts boolean inputs after
removing wait_for_completion; update the description string (the description
variable/docstring in this file) to remove the stale boolean-parameter guidance
and adjust the remaining sentence flow so it only documents current read-only
and modifying actions (telemetry_status, telemetry_ping, play, pause, stop,
set_active_tool, add_tag, remove_tag, add_layer, remove_layer,
close_prefab_stage, deploy_package, restore_package).
In `@unity-mcp-skill/references/workflows.md`:
- Around line 119-120: The wait snippets use a different field name
(isCompiling) than the rest of the workflow which reads
editor_state["is_compiling"]; update all wait examples (including the
manage_script update auto-triggers import+compile snippet and the other
occurrences mentioned) to reference editor_state["is_compiling"] instead of
isCompiling so they all use the same editor_state field; search for isCompiling
in the document and replace with editor_state["is_compiling"] in those wait
examples and ensure the surrounding code still references the editor_state
object consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb807129-236c-410f-a8d4-a38f683ff8c7
📒 Files selected for processing (16)
.claude/skills/unity-mcp-skill/SKILL.md.claude/skills/unity-mcp-skill/references/tools-reference.md.claude/skills/unity-mcp-skill/references/workflows.mdCLAUDE.mdMCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Helpers/ParamCoercion.csMCPForUnity/Editor/Tools/GameObjects/ManageGameObject.csMCPForUnity/Editor/Tools/Graphics/GraphicsHelpers.csMCPForUnity/Editor/Tools/ManageEditor.csMCPForUnity/Editor/Tools/ManageScriptableObject.csMCPForUnity/Editor/Tools/ManageUI.csMCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.csServer/src/services/tools/manage_editor.pyunity-mcp-skill/SKILL.mdunity-mcp-skill/references/tools-reference.mdunity-mcp-skill/references/workflows.md
| ### Transport Modes | ||
|
|
||
| - **Stdio**: Single-agent only. Separate Python process per client. Legacy TCP bridge to Unity. New connections stomp old ones. | ||
| - **HTTP**: Multi-agent ready. Single shared Python server. WebSocket hub at `/hub/plugin`. Session isolation via `client_id`. |
Contributor
There was a problem hiding this comment.
Clarify HTTP isolation mechanism (client_id vs authenticated user_id).
Line 40 currently says HTTP mode isolates sessions via client_id, but in remote-hosted mode the transport resolves identity from API key to user_id for scoping. Please update this line to avoid security/tenant-isolation ambiguity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` at line 40, Update the description of HTTP isolation to clearly
distinguish local/shared-server behavior from remote-hosted behavior: state that
in HTTP mode a multi-agent setup on a single shared Python server uses session
isolation via client_id for per-client sessions, and explicitly add that in
remote-hosted transport the identity is resolved from the API key to an
authenticated user_id which is used for tenant scoping; reference the existing
phrase "Session isolation via `client_id`" and add the clause about resolving
API key to `user_id` for remote-hosted scoping to remove ambiguity.
Comment on lines
+372
to
+385
| private static object ClosePrefabStage() | ||
| { | ||
| try | ||
| { | ||
| var prefabStage = PrefabStageUtility.GetCurrentPrefabStage(); | ||
| if (prefabStage == null) | ||
| { | ||
| return new SuccessResponse("Not currently in prefab editing mode."); | ||
| } | ||
|
|
||
| string prefabPath = prefabStage.assetPath; | ||
| StageUtility.GoToMainStage(); | ||
| return new SuccessResponse($"Exited prefab stage for '{prefabPath}'.", new { prefabPath }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Unity Editor scripting, what happens when StageUtility.GoToMainStage() is called while the current Prefab Stage has unsaved changes and Auto Save is disabled?
💡 Result:
When you call StageUtility.GoToMainStage() while you’re currently in a Prefab Stage that has unsaved changes and Auto Save is disabled, Unity tries to switch stages, but first asks permission to leave the current stage.
Concretely:
StageUtility.GoToMainStage()delegates the navigation toStageNavigationManager.instance.GoToMainStage(...). [1]- When switching stages, Unity calls
previousStage.AskUserToSaveModifiedStageBeforeSwitchingStage(). If that returns false, the stage switch is canceled and you remain in the Prefab Stage. [2] - Because a
PrefabStagereportshasUnsavedChangeswhen its preview scene is dirty, your Prefab Stage counts as “modified” in this situation. [3] - With Auto Save disabled, Unity won’t silently save on the way out; instead, the user is expected to decide (typically via a Save / Don’t Save / Cancel prompt) whether to save, discard, or cancel the navigation. Auto Save can also be entirely disallowed at the project level via
EditorSettings.prefabModeAllowAutoSave. [3][4]
Net effect: the editor will block the stage switch until the user chooses what to do; if the user cancels, GoToMainStage() does not take you back to the Main Stage. [2]
Sources
[1] UnityCsReference – StageUtility.GoToMainStage() implementation (calls StageNavigationManager.instance.GoToMainStage(...)). (github.com)
[2] UnityCsReference – StageNavigationManager.SwitchToStage: calls AskUserToSaveModifiedStageBeforeSwitchingStage() and cancels switching when it returns false. (github.com)
[3] UnityCsReference – PrefabStage.hasUnsavedChanges and PrefabStage.autoSave logic. (github.com)
[4] Unity Scripting API – EditorSettings.prefabModeAllowAutoSave (project-level ability to allow/disallow Prefab Mode Auto Save UI/behavior). (docs.unity3d.com)
Citations:
- 1: https://github.com/Unity-Technologies/UnityCsReference/blob/master/Editor/Mono/SceneManagement/StageManager/StageUtility.cs/
- 2: https://github.com/Unity-Technologies/UnityCsReference/blob/master/Editor/Mono/SceneManagement/StageManager/StageNavigationManager.cs
- 3: https://github.com/Unity-Technologies/UnityCsReference/blob/master/Editor/Mono/SceneManagement/StageManager/PrefabStage/PrefabStage.cs
- 4: https://docs.unity3d.com/ja/2021.1/ScriptReference/EditorSettings-prefabModeAllowAutoSave.html?utm_source=openai
Check for unsaved changes before calling StageUtility.GoToMainStage() in ClosePrefabStage().
StageUtility.GoToMainStage() will block on a user prompt if the prefab stage has unsaved changes and Auto Save is disabled. In headless/automated environments (CI/CD, command-line tools), this hangs indefinitely with no user to respond.
Before switching stages, check prefabStage.hasUnsavedChanges and either save the stage (using EditorSceneManager.SaveScene(prefabStage.scene)) or explicitly discard changes (via the prefab stage API) to ensure the action completes deterministically without user interaction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/ManageEditor.cs` around lines 372 - 385, Before
calling StageUtility.GoToMainStage() in ClosePrefabStage(), check the current
prefab stage for unsaved changes via PrefabStageUtility.GetCurrentPrefabStage()
and prefabStage.hasUnsavedChanges; if true, deterministically resolve them by
saving the scene with EditorSceneManager.SaveScene(prefabStage.scene) (and
handle/save failure) or explicitly discard changes via the prefab stage API
before calling StageUtility.GoToMainStage(); update ClosePrefabStage to perform
that check and only switch stages after saving or discarding to avoid blocking
in headless/CI environments.
Comment on lines
+295
to
+300
| bool isRuntimeInstance = mat != null && !EditorUtility.IsPersistent(mat); | ||
| bool isNullWithPropertyBlock = mat == null && HasPropertyBlockColors(renderer, slot); | ||
| bool isNullMaterial = mat == null && !isNullWithPropertyBlock; | ||
|
|
||
| if (!isRuntimeInstance && !isNullWithPropertyBlock) | ||
| continue; |
Contributor
There was a problem hiding this comment.
Property-block colors on normal shared materials are still skipped.
The property-block path only runs when mat == null. A renderer that still has a persistent shared material plus a MaterialPropertyBlock color override falls through continue, so those tints still won't survive prefab serialization.
🎯 Suggested fix
- bool isRuntimeInstance = mat != null && !EditorUtility.IsPersistent(mat);
- bool isNullWithPropertyBlock = mat == null && HasPropertyBlockColors(renderer, slot);
+ bool hasPropertyBlockColors = HasPropertyBlockColors(renderer, slot);
+ bool isRuntimeInstance = mat != null && !EditorUtility.IsPersistent(mat);
+ bool isPersistentMaterialWithPropertyBlock =
+ mat != null && EditorUtility.IsPersistent(mat) && hasPropertyBlockColors;
+ bool isNullWithPropertyBlock = mat == null && hasPropertyBlockColors;
bool isNullMaterial = mat == null && !isNullWithPropertyBlock;
- if (!isRuntimeInstance && !isNullWithPropertyBlock)
+ if (!isRuntimeInstance && !isNullWithPropertyBlock && !isPersistentMaterialWithPropertyBlock)
continue;📝 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
| bool isRuntimeInstance = mat != null && !EditorUtility.IsPersistent(mat); | |
| bool isNullWithPropertyBlock = mat == null && HasPropertyBlockColors(renderer, slot); | |
| bool isNullMaterial = mat == null && !isNullWithPropertyBlock; | |
| if (!isRuntimeInstance && !isNullWithPropertyBlock) | |
| continue; | |
| bool hasPropertyBlockColors = HasPropertyBlockColors(renderer, slot); | |
| bool isRuntimeInstance = mat != null && !EditorUtility.IsPersistent(mat); | |
| bool isPersistentMaterialWithPropertyBlock = | |
| mat != null && EditorUtility.IsPersistent(mat) && hasPropertyBlockColors; | |
| bool isNullWithPropertyBlock = mat == null && hasPropertyBlockColors; | |
| bool isNullMaterial = mat == null && !isNullWithPropertyBlock; | |
| if (!isRuntimeInstance && !isNullWithPropertyBlock && !isPersistentMaterialWithPropertyBlock) | |
| continue; |
🤖 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 295 - 300,
The current check uses isNullWithPropertyBlock (which is true only when mat ==
null) so renderers with a persistent shared material plus a
MaterialPropertyBlock color override are skipped; change the logic to detect
property-block colors regardless of mat: call HasPropertyBlockColors(renderer,
slot) into a boolean like hasPropertyBlock and replace the continue condition
with if (!isRuntimeInstance && !hasPropertyBlock) continue so property-block
tints on non-null shared materials are handled.
Comment on lines
+302
to
+347
| // Derive a unique asset path from the GameObject name and slot | ||
| string goName = renderer.gameObject.name.Replace(" ", "_"); | ||
| string suffix = slot > 0 ? $"_slot{slot}" : ""; | ||
| string matPath = $"{materialsFolder}/{goName}{suffix}_mat.mat"; | ||
| matPath = AssetPathUtility.SanitizeAssetPath(matPath); | ||
| if (matPath == null) | ||
| { | ||
| McpLog.Warn($"[ManagePrefabs] Could not build safe material path for '{renderer.gameObject.name}', skipping."); | ||
| continue; | ||
| } | ||
|
|
||
| // Ensure the Materials directory exists | ||
| if (!AssetDatabase.IsValidFolder(materialsFolder)) | ||
| { | ||
| string parentDir = Path.GetDirectoryName(materialsFolder).Replace("\\", "/"); | ||
| string folderName = Path.GetFileName(materialsFolder); | ||
| AssetDatabase.CreateFolder(parentDir, folderName); | ||
| } | ||
|
|
||
| Material persisted = AssetDatabase.LoadAssetAtPath<Material>(matPath); | ||
| if (persisted == null) | ||
| { | ||
| // Create a new material with the correct shader for the active pipeline | ||
| Shader shader = isRuntimeInstance && mat.shader != null | ||
| ? mat.shader | ||
| : RenderPipelineUtility.ResolveShader("Standard"); | ||
| persisted = new Material(shader); | ||
| AssetDatabase.CreateAsset(persisted, matPath); | ||
| } | ||
|
|
||
| // Copy properties from the runtime instance if available | ||
| if (isRuntimeInstance) | ||
| { | ||
| persisted.CopyPropertiesFromMaterial(mat); | ||
| EditorUtility.SetDirty(persisted); | ||
| } | ||
| else if (isNullWithPropertyBlock) | ||
| { | ||
| // Extract color from the property block and apply to the new material | ||
| ApplyPropertyBlockToMaterial(renderer, slot, persisted); | ||
| EditorUtility.SetDirty(persisted); | ||
| } | ||
|
|
||
| sharedMats[slot] = persisted; | ||
| changed = true; | ||
| persistedPaths.Add(matPath); |
Contributor
There was a problem hiding this comment.
Renderer-name collisions will merge distinct persisted materials.
matPath is based only on renderer.gameObject.name and slot. Two different renderers named Cube in separate branches resolve to the same asset path, so the later one reuses/overwrites the earlier .mat and both prefab children end up sharing the same persisted material.
🤖 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 302 - 347,
matPath currently uses only renderer.gameObject.name and slot which causes
different renderers with the same name to collide; change the matPath generation
in ManagePrefabs so it includes a stable unique identifier for the prefab
instance (e.g. the prefab asset path or prefab GUID or a sanitized hierarchy
path) along with renderer.gameObject.name and slot before calling
AssetPathUtility.SanitizeAssetPath. Locate the matPath build around the
variables goName, suffix and materialsFolder, obtain a prefab identifier via
PrefabUtility.GetPrefabAssetPathOfNearestInstanceRoot(renderer.gameObject) or
PrefabUtility.GetPrefabInstanceHandle/GetLocalIdentifierInFile and incorporate a
short hash or sanitized version of that identifier into the filename (or folder)
to ensure distinct persisted .mat files for different prefabs while still using
AssetPathUtility.SanitizeAssetPath on the final path.
Comment on lines
+355
to
+359
| // Clear any property blocks now that the material is persisted | ||
| for (int slot = 0; slot < sharedMats.Length; slot++) | ||
| { | ||
| renderer.SetPropertyBlock(null, slot); | ||
| } |
Contributor
There was a problem hiding this comment.
Don't clear every MaterialPropertyBlock after migrating one slot.
HasPropertyBlockColors() currently returns !block.isEmpty, but ApplyPropertyBlockToMaterial() only copies _BaseColor and _Color. After that, Lines 355-359 clear property blocks for every slot on the renderer, which drops unrelated float/vector/texture overrides and can wipe untouched slots.
🧹 Narrow the detection first
private static bool HasPropertyBlockColors(Renderer renderer, int slot)
{
MaterialPropertyBlock block = new MaterialPropertyBlock();
renderer.GetPropertyBlock(block, slot);
- return !block.isEmpty;
+ return block.HasColor("_BaseColor") || block.HasColor("_Color");
}Also applies to: 376-380
🤖 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 355 - 359,
HasPropertyBlockColors() and ApplyPropertyBlockToMaterial() currently treat any
non-empty MaterialPropertyBlock as a color candidate and then the code clears
all slots on the renderer (renderer.SetPropertyBlock(null, slot)), which wipes
unrelated overrides; narrow detection to only consider color properties and only
clear the specific slot after migration. Update HasPropertyBlockColors() to
check for presence of _BaseColor or _Color (use block.TryGetColor or
block.HasProperty for those names) instead of !block.isEmpty, change
ApplyPropertyBlockToMaterial() to remove/ignore only the color properties from
the block and then call renderer.SetPropertyBlock(null, slot) only for that same
slot when the block no longer contains any other properties; apply the same
per-slot-only logic to the similar block around the second occurrence (the
376-380 area) and stop clearing every slot in sharedMats.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
unity-mcp-skill/references/workflows.md (1)
119-120:⚠️ Potential issue | 🟠 MajorCorrect the editor_state field path for compilation status.
The wait instruction references
is_compilingat the root level, but the actual editor_state resource structure iscompilation.is_compiling(orcompilation.nin the serialized form). This mismatch will break wait loops.Based on the editor_state.py implementation (Server/src/services/resources/editor_state.py), the correct path is
editor_state["compilation"]["is_compiling"].📝 Suggested fix
-# Read mcpforunity://editor/state → wait until is_compiling == false +# Read mcpforunity://editor/state → wait until compilation.is_compiling == falseAlso applies to: 235-235, 291-292, 520-521
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@unity-mcp-skill/references/workflows.md` around lines 119 - 120, The wait logic reads the compilation flag from the wrong path; update all checks that reference is_compiling at the root to use the nested path editor_state["compilation"]["is_compiling"] (or its serialized form compilation.n) so the wait loops check the actual compilation status; search for occurrences of is_compiling in the workflow steps (e.g., the read of mcpforunity://editor/state) and replace them with the nested editor_state["compilation"]["is_compiling"] access..claude/skills/unity-mcp-skill/SKILL.md (2)
40-40:⚠️ Potential issue | 🟠 MajorCorrect the editor_state field path for compilation status.
Same issue as in workflows.md: the wait instruction references
is_compilingat the root level, but the actual structure iseditor_state["compilation"]["is_compiling"].📝 Suggested fix
-# Read mcpforunity://editor/state → wait until is_compiling == false +# Read mcpforunity://editor/state → wait until compilation.is_compiling == falseAlso applies to: 198-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/unity-mcp-skill/SKILL.md at line 40, The wait instruction currently reads mcpforunity://editor/state and checks editor_state.is_compiling at the root; change the condition to reference the nested compilation flag editor_state["compilation"]["is_compiling"] (or editor_state.compilation.is_compiling) so it waits until that value is false; update the same incorrect occurrence elsewhere in the file (the duplicate at the second location) to use the nested path as well.
177-177:⚠️ Potential issue | 🔴 CriticalCritical: Prefab instantiation API is documented incorrectly.
Same issue as in workflows.md: this line claims
manage_gameobject(action="create", prefab_path="...")instantiates prefabs, but theprefab_pathparameter is only used whensave_as_prefab=Trueto specify where to save a newly created GameObject as a prefab file.The documented API for prefab instantiation does not exist. Users following this guidance will encounter failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/unity-mcp-skill/SKILL.md at line 177, The docs incorrectly state that manage_gameobject(action="create", prefab_path="...") instantiates prefabs; update the SKILL.md entry to clarify that the prefab_path parameter is only used when save_as_prefab=True to specify where to save a newly created GameObject as a prefab file and that there is no current API to instantiate prefabs via manage_gameobject; remove the false instantiation example/claim, and optionally add a short note suggesting filing an enhancement if prefab-instantiation functionality is required..claude/skills/unity-mcp-skill/references/workflows.md (1)
119-120:⚠️ Potential issue | 🟠 MajorSame issue as in
unity-mcp-skill/references/workflows.md.This file appears to be a duplicate. The
is_compilingfield path issue flagged in the other workflows.md file also applies here.Also applies to: 235-235, 291-292, 520-521, 422-450 (prefab instantiation)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/unity-mcp-skill/references/workflows.md around lines 119 - 120, This file is a duplicate and repeats the same incorrect field path and prefab-instantiation logic found in the other workflows.md; remove or consolidate this duplicate and update the references so they use the correct state field path "is_compiling" from the canonical workflow (ensure reads from mcpforunity://editor/state reference the exact field name used elsewhere), and review the prefab instantiation steps (lines noted around the "prefab instantiation" block) to match the corrected implementation in the primary workflows.md; specifically fix the "manage_script update auto-triggers import + compile" step to wait on the correct is_compiling boolean and align prefab creation steps with the canonical sequence to avoid duplication and mismatched field names.
🧹 Nitpick comments (2)
MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs (1)
371-385: Duplicate helper: consider extracting to shared utility.This
EnsureAssetFolderExistsimplementation is identical to the one inManageMaterial.cs(lines 370-384). Consider extracting to a shared utility class likeAssetPathUtilityto eliminate duplication.🤖 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 371 - 385, The EnsureAssetFolderExists implementation is duplicated (also present in ManageMaterial.cs); extract it into a shared static utility (e.g., class AssetPathUtility with a public static EnsureAssetFolderExists or EnsureFolderExists method) and replace the local copies in ManagePrefabs.cs and ManageMaterial.cs to call that utility; update both files to remove the duplicated private method and reference the new shared method to centralize the folder-creation logic.MCPForUnity/Editor/Tools/ManageMaterial.cs (1)
370-384: Consider extracting shared helper to avoid duplication.This
EnsureAssetFolderExistsimplementation is duplicated inManagePrefabs.cs(lines 371-385). Consider extracting this to a shared utility class (e.g.,AssetPathUtility) to maintain DRY principles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/ManageMaterial.cs` around lines 370 - 384, Duplicate EnsureAssetFolderExists implementation found in ManageMaterial.cs and ManagePrefabs.cs; extract it to a single shared static helper to remove duplication. Create a new static class (e.g., AssetPathUtility) with a public/static method EnsureAssetFolderExists(string assetFolderPath) containing the existing folder-creation logic, keep method name the same to minimize call-site changes, and replace the local implementations in both ManageMaterial.cs (EnsureAssetFolderExists) and ManagePrefabs.cs with calls to AssetPathUtility.EnsureAssetFolderExists(assetFolderPath); ensure the helper lives in the same Editor namespace (or a common namespace) and adjust using statements if needed.
🤖 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/Helpers/ComponentOps.cs`:
- Around line 544-554: The code currently accepts JTokenType.Float as valid
integer input and falls back to long.TryParse on the token's ToString(),
allowing values like 1.2 to be coerced into integers; update the validation in
the method where this block lives to explicitly reject JTokenType.Float (i.e.,
treat floats as invalid for integer properties) so the branch that sets
prop.longValue or prop.intValue only runs for integral tokens or parseable
integer strings; keep the error = "Expected integer value." and return false for
float tokens and avoid relying on ToString() coercion, referencing the existing
symbols prop.type, prop.longValue, prop.intValue, and
ParamCoercion.CoerceLong/CoerceInt to locate and change the conditional.
In `@MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs`:
- Around line 368-371: The XML doc summary above EnsureAssetFolderExists is
incorrect (it describes checking a renderer's MaterialPropertyBlock); update the
XML documentation for the method EnsureAssetFolderExists to accurately describe
its behavior (creating or ensuring an asset folder exists at the given path,
creating intermediate folders as needed) and adjust params/returns remarks to
match the method signature and behavior so the documentation reflects the actual
purpose of EnsureAssetFolderExists.
In `@unity-mcp-skill/references/workflows.md`:
- Around line 422-450: The docs wrongly claim manage_gameobject(action="create",
prefab_path="...") will instantiate prefabs; fix this by either (A) updating
workflows.md to remove/replace those examples with the correct current flow
(explain that prefab_path in manage_gameobject.py is only used when
action=="create" AND save_as_prefab=True and that manage_prefabs has no
instantiate action), or (B) implement proper instantiation support: add an
"instantiate" or "create_from_prefab" action to manage_prefabs (or extend
manage_gameobject to handle prefab_path for create without save_as_prefab) and
update the docs accordingly; reference manage_gameobject.py (prefab_path and
save_as_prefab logic) and the manage_prefabs actions (create_from_gameobject,
get_info, get_hierarchy, modify_contents) so the change is made in the correct
place.
---
Duplicate comments:
In @.claude/skills/unity-mcp-skill/references/workflows.md:
- Around line 119-120: This file is a duplicate and repeats the same incorrect
field path and prefab-instantiation logic found in the other workflows.md;
remove or consolidate this duplicate and update the references so they use the
correct state field path "is_compiling" from the canonical workflow (ensure
reads from mcpforunity://editor/state reference the exact field name used
elsewhere), and review the prefab instantiation steps (lines noted around the
"prefab instantiation" block) to match the corrected implementation in the
primary workflows.md; specifically fix the "manage_script update auto-triggers
import + compile" step to wait on the correct is_compiling boolean and align
prefab creation steps with the canonical sequence to avoid duplication and
mismatched field names.
In @.claude/skills/unity-mcp-skill/SKILL.md:
- Line 40: The wait instruction currently reads mcpforunity://editor/state and
checks editor_state.is_compiling at the root; change the condition to reference
the nested compilation flag editor_state["compilation"]["is_compiling"] (or
editor_state.compilation.is_compiling) so it waits until that value is false;
update the same incorrect occurrence elsewhere in the file (the duplicate at the
second location) to use the nested path as well.
- Line 177: The docs incorrectly state that manage_gameobject(action="create",
prefab_path="...") instantiates prefabs; update the SKILL.md entry to clarify
that the prefab_path parameter is only used when save_as_prefab=True to specify
where to save a newly created GameObject as a prefab file and that there is no
current API to instantiate prefabs via manage_gameobject; remove the false
instantiation example/claim, and optionally add a short note suggesting filing
an enhancement if prefab-instantiation functionality is required.
In `@unity-mcp-skill/references/workflows.md`:
- Around line 119-120: The wait logic reads the compilation flag from the wrong
path; update all checks that reference is_compiling at the root to use the
nested path editor_state["compilation"]["is_compiling"] (or its serialized form
compilation.n) so the wait loops check the actual compilation status; search for
occurrences of is_compiling in the workflow steps (e.g., the read of
mcpforunity://editor/state) and replace them with the nested
editor_state["compilation"]["is_compiling"] access.
---
Nitpick comments:
In `@MCPForUnity/Editor/Tools/ManageMaterial.cs`:
- Around line 370-384: Duplicate EnsureAssetFolderExists implementation found in
ManageMaterial.cs and ManagePrefabs.cs; extract it to a single shared static
helper to remove duplication. Create a new static class (e.g., AssetPathUtility)
with a public/static method EnsureAssetFolderExists(string assetFolderPath)
containing the existing folder-creation logic, keep method name the same to
minimize call-site changes, and replace the local implementations in both
ManageMaterial.cs (EnsureAssetFolderExists) and ManagePrefabs.cs with calls to
AssetPathUtility.EnsureAssetFolderExists(assetFolderPath); ensure the helper
lives in the same Editor namespace (or a common namespace) and adjust using
statements if needed.
In `@MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs`:
- Around line 371-385: The EnsureAssetFolderExists implementation is duplicated
(also present in ManageMaterial.cs); extract it into a shared static utility
(e.g., class AssetPathUtility with a public static EnsureAssetFolderExists or
EnsureFolderExists method) and replace the local copies in ManagePrefabs.cs and
ManageMaterial.cs to call that utility; update both files to remove the
duplicated private method and reference the new shared method to centralize the
folder-creation logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a00f9adf-cced-416c-b70c-736362ca1a84
📒 Files selected for processing (12)
.claude/skills/unity-mcp-skill/SKILL.md.claude/skills/unity-mcp-skill/references/workflows.mdMCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Tools/ManageMaterial.csMCPForUnity/Editor/Tools/ManageScriptableObject.csMCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.csMCPForUnity/Editor/Windows/Components/Advanced/McpAdvancedSection.uxmlServer/src/cli/commands/material.pyServer/src/services/tools/manage_editor.pyServer/src/services/tools/manage_material.pyunity-mcp-skill/SKILL.mdunity-mcp-skill/references/workflows.md
✅ Files skipped from review due to trivial changes (1)
- Server/src/services/tools/manage_material.py
🚧 Files skipped from review as they are similar to previous changes (2)
- unity-mcp-skill/SKILL.md
- MCPForUnity/Editor/Tools/ManageScriptableObject.cs
Comment on lines
+544
to
+554
| if (value == null || value.Type == JTokenType.Null | ||
| || (value.Type != JTokenType.Integer && value.Type != JTokenType.Float | ||
| && !long.TryParse(value.ToString(), out _))) | ||
| { | ||
| if (value == null || value.Type == JTokenType.Null || | ||
| (value.Type == JTokenType.String && !int.TryParse(value.ToString(), out _))) | ||
| { | ||
| error = "Expected integer value."; | ||
| return false; | ||
| } | ||
| error = "Expected integer value."; | ||
| return false; | ||
| } | ||
| prop.intValue = intVal; | ||
| if (prop.type == "long") | ||
| prop.longValue = ParamCoercion.CoerceLong(value, 0); | ||
| else | ||
| prop.intValue = ParamCoercion.CoerceInt(value, 0); |
Contributor
There was a problem hiding this comment.
Reject non-integral floats for integer properties.
At Line 545-Line 546, any float is treated as valid integer input. Values like 1.2 can be silently coerced into an integer and overwrite serialized state unexpectedly.
🔧 Suggested fix
+using System.Globalization;
...
case SerializedPropertyType.Integer:
- if (value == null || value.Type == JTokenType.Null
- || (value.Type != JTokenType.Integer && value.Type != JTokenType.Float
- && !long.TryParse(value.ToString(), out _)))
+ if (value == null || value.Type == JTokenType.Null)
{
error = "Expected integer value.";
return false;
}
+ if (value.Type == JTokenType.Float)
+ {
+ double d = value.Value<double>();
+ if (Math.Abs(d % 1d) > double.Epsilon)
+ {
+ error = "Expected integer value.";
+ return false;
+ }
+ }
+ else if (value.Type != JTokenType.Integer
+ && !long.TryParse(value.ToString(), NumberStyles.Integer, CultureInfo.InvariantCulture, out _))
+ {
+ error = "Expected integer value.";
+ return false;
+ }
if (prop.type == "long")
prop.longValue = ParamCoercion.CoerceLong(value, 0);
else
prop.intValue = ParamCoercion.CoerceInt(value, 0);
return true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (value == null || value.Type == JTokenType.Null | |
| || (value.Type != JTokenType.Integer && value.Type != JTokenType.Float | |
| && !long.TryParse(value.ToString(), out _))) | |
| { | |
| if (value == null || value.Type == JTokenType.Null || | |
| (value.Type == JTokenType.String && !int.TryParse(value.ToString(), out _))) | |
| { | |
| error = "Expected integer value."; | |
| return false; | |
| } | |
| error = "Expected integer value."; | |
| return false; | |
| } | |
| prop.intValue = intVal; | |
| if (prop.type == "long") | |
| prop.longValue = ParamCoercion.CoerceLong(value, 0); | |
| else | |
| prop.intValue = ParamCoercion.CoerceInt(value, 0); | |
| if (value == null || value.Type == JTokenType.Null) | |
| { | |
| error = "Expected integer value."; | |
| return false; | |
| } | |
| if (value.Type == JTokenType.Float) | |
| { | |
| double d = value.Value<double>(); | |
| if (Math.Abs(d % 1d) > double.Epsilon) | |
| { | |
| error = "Expected integer value."; | |
| return false; | |
| } | |
| } | |
| else if (value.Type != JTokenType.Integer | |
| && !long.TryParse(value.ToString(), NumberStyles.Integer, CultureInfo.InvariantCulture, out _)) | |
| { | |
| error = "Expected integer value."; | |
| return false; | |
| } | |
| if (prop.type == "long") | |
| prop.longValue = ParamCoercion.CoerceLong(value, 0); | |
| else | |
| prop.intValue = ParamCoercion.CoerceInt(value, 0); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Helpers/ComponentOps.cs` around lines 544 - 554, The code
currently accepts JTokenType.Float as valid integer input and falls back to
long.TryParse on the token's ToString(), allowing values like 1.2 to be coerced
into integers; update the validation in the method where this block lives to
explicitly reject JTokenType.Float (i.e., treat floats as invalid for integer
properties) so the branch that sets prop.longValue or prop.intValue only runs
for integral tokens or parseable integer strings; keep the error = "Expected
integer value." and return false for float tokens and avoid relying on
ToString() coercion, referencing the existing symbols prop.type, prop.longValue,
prop.intValue, and ParamCoercion.CoerceLong/CoerceInt to locate and change the
conditional.
Comment on lines
+422
to
+450
| ### Instantiate Prefab in Scene | ||
|
|
||
| Use `manage_gameobject` (not `manage_prefabs`) to place prefab instances in the scene. | ||
|
|
||
| ```python | ||
| # Full path | ||
| manage_gameobject( | ||
| action="create", | ||
| name="Enemy_1", | ||
| prefab_path="Assets/Prefabs/Enemy.prefab", | ||
| position=[5, 0, 3], | ||
| parent="Enemies" | ||
| ) | ||
|
|
||
| # Smart lookup — just the prefab name works too | ||
| manage_gameobject(action="create", name="Enemy_2", prefab_path="Enemy", position=[10, 0, 3]) | ||
|
|
||
| # Batch-spawn multiple instances | ||
| batch_execute(commands=[ | ||
| {"tool": "manage_gameobject", "params": { | ||
| "action": "create", "name": f"Enemy_{i}", | ||
| "prefab_path": "Enemy", "position": [i * 3, 0, 0], "parent": "Enemies" | ||
| }} | ||
| for i in range(5) | ||
| ]) | ||
| ``` | ||
|
|
||
| > **Note:** `manage_prefabs` is for headless prefab editing (inspect, modify contents, create from GameObject). To *instantiate* a prefab into the scene, always use `manage_gameobject(action="create", prefab_path="...")`. | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify manage_gameobject does not support prefab instantiation via prefab_path
# Expected: prefab_path is only used with save_as_prefab, not for instantiation
echo "=== Checking manage_gameobject prefab_path usage ==="
rg -n -C5 'prefab_path' Server/src/services/tools/manage_gameobject.py
echo -e "\n=== Checking manage_prefabs available actions ==="
rg -n 'Literal\[' Server/src/services/tools/manage_prefabs.py | head -20Repository: CoplayDev/unity-mcp
Length of output: 2310
🏁 Script executed:
#!/bin/bash
# Get full context around prefab_path usage in manage_gameobject
echo "=== Full manage_gameobject create action logic ==="
sed -n '160,230p' Server/src/services/tools/manage_gameobject.py
echo -e "\n=== Check if prefab_path is used outside save_as_prefab condition ==="
rg -n -B10 -A10 'prefabPath' Server/src/services/tools/manage_gameobject.py | head -60Repository: CoplayDev/unity-mcp
Length of output: 6209
🏁 Script executed:
#!/bin/bash
# Search for how to actually instantiate prefabs in the codebase
echo "=== Search for instantiate related code ==="
rg -i 'instantiate' Server/src/ --type py -n
echo -e "\n=== Check C# implementation for prefab handling ==="
rg -i 'prefab' Assets/Scripts/ --type cs -l 2>/dev/null | head -5Repository: CoplayDev/unity-mcp
Length of output: 159
🏁 Script executed:
#!/bin/bash
# Get manage_prefabs full implementation to see available actions
echo "=== manage_prefabs full implementation ==="
cat Server/src/services/tools/manage_prefabs.py
echo -e "\n=== Check manage_prefabs function signature ==="
rg -n 'async def manage_prefabs' Server/src/services/tools/manage_prefabs.py -A 30Repository: CoplayDev/unity-mcp
Length of output: 14163
🏁 Script executed:
#!/bin/bash
# Search for C# prefab/instantiation code more broadly
echo "=== Search Assets for instantiate or prefab loading ==="
find Assets -name "*.cs" -type f -exec grep -l -i "instantiate\|prefab" {} \; 2>/dev/null | head -10Repository: CoplayDev/unity-mcp
Length of output: 118
Critical: Prefab instantiation method documented does not exist in the API.
The section at lines 422-450 documents using manage_gameobject(action="create", prefab_path="...") to instantiate prefabs into the scene, but this functionality does not exist.
In manage_gameobject.py, the prefab_path parameter is only processed when both action == "create" AND save_as_prefab=True (line 203). It is used to specify where to save a newly created GameObject as a prefab file—not to load or instantiate from an existing prefab.
Additionally, manage_prefabs has no instantiation capability. Its only available actions are: create_from_gameobject, get_info, get_hierarchy, and modify_contents—none of which instantiate prefabs into a scene.
Impact: Users following this documentation will encounter failures. There is currently no documented way to instantiate prefabs into a scene using these tools.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@unity-mcp-skill/references/workflows.md` around lines 422 - 450, The docs
wrongly claim manage_gameobject(action="create", prefab_path="...") will
instantiate prefabs; fix this by either (A) updating workflows.md to
remove/replace those examples with the correct current flow (explain that
prefab_path in manage_gameobject.py is only used when action=="create" AND
save_as_prefab=True and that manage_prefabs has no instantiate action), or (B)
implement proper instantiation support: add an "instantiate" or
"create_from_prefab" action to manage_prefabs (or extend manage_gameobject to
handle prefab_path for create without save_as_prefab) and update the docs
accordingly; reference manage_gameobject.py (prefab_path and save_as_prefab
logic) and the manage_prefabs actions (create_from_gameobject, get_info,
get_hierarchy, modify_contents) so the change is made in the correct place.
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.
Summary by Sourcery
Persist runtime materials when creating prefabs, improve serialized int/long handling and editor/prefab workflows, and expand MCP skill documentation for graphics, packages, and script compilation behavior.
Bug Fixes:
Enhancements:
Documentation:
Summary by CodeRabbit
New Features
Documentation
Bug Fixes