Payload-safe paging for hierarchy/components + safer asset search + docs #490
Conversation
CodexConfigHelperTests was calling MCPServiceLocator.Reset() in TearDown, which disposes the active bridge/transport during MCP-driven test runs. Replace with restoring only the mutated service (IPlatformService).
Capture the original IPlatformService before this fixture runs and restore it in TearDown. This preserves the MCP connection safety fix (no MCPServiceLocator.Reset()) while avoiding global state leakage to subsequent tests.
…sts; remove vestigial SO tools
…criptableObject tools
… redundant import
…ve broken SO logic from ManageAsset
Reviewer's GuideImplements payload-safe paging and metadata-first responses for hierarchy and component queries, hardens asset search and process management, adds dev-mode cache-busting controls, centralizes Python-side coercion helpers, and updates tests/docs accordingly. Sequence diagram for paged manage_scene get_hierarchysequenceDiagram
actor LLMClient
participant MCPServerPython as MCPServerPython
participant ManageScenePy as manage_scene_py
participant UnityTransport as UnityTransport
participant ManageSceneCs as ManageScene_cs
LLMClient->>MCPServerPython: manage_scene(action=get_hierarchy, parent, page_size, cursor, ...)
MCPServerPython->>ManageScenePy: manage_scene(action, name, build_index, screenshot_file_name,
MCPServerPython-->>ManageScenePy: screenshot_super_size, parent, page_size, cursor,
MCPServerPython-->>ManageScenePy: max_nodes, max_depth, max_children_per_node, include_transform)
ManageScenePy->>ManageScenePy: coerce_int/coerce_bool on paging params
ManageScenePy->>UnityTransport: send_with_unity_instance(async_send_command_with_retry,
ManageScenePy-->>UnityTransport: "manage_scene", params{action,get_hierarchy+paging}
UnityTransport->>ManageSceneCs: HandleCommand(params)
ManageSceneCs->>ManageSceneCs: ToSceneCommand(JObject)
ManageSceneCs->>ManageSceneCs: GetSceneHierarchyPaged(SceneCommand)
ManageSceneCs->>ManageSceneCs: ResolveGameObject(parent)
alt parent is null
ManageSceneCs->>ManageSceneCs: nodes = activeScene.GetRootGameObjects()
else parent found
ManageSceneCs->>ManageSceneCs: nodes = parent.children
else parent not found
ManageSceneCs-->>UnityTransport: ErrorResponse("Parent GameObject not found")
UnityTransport-->>ManageScenePy: error
ManageScenePy-->>MCPServerPython: error
MCPServerPython-->>LLMClient: error
ManageSceneCs->>ManageSceneCs: return
end
ManageSceneCs->>ManageSceneCs: clamp pageSize,maxNodes,maxChildrenPerNode
ManageSceneCs->>ManageSceneCs: slice nodes[cursor:cursor+effectiveTake]
loop for each node in page
ManageSceneCs->>ManageSceneCs: BuildGameObjectSummary(go, includeTransform,...)
end
ManageSceneCs->>ManageSceneCs: compute next_cursor or null
ManageSceneCs-->>UnityTransport: SuccessResponse{scope,cursor,pageSize,next_cursor,truncated,total,items}
UnityTransport-->>ManageScenePy: response
ManageScenePy-->>MCPServerPython: response
MCPServerPython-->>LLMClient: paged hierarchy summary
LLMClient->>MCPServerPython: repeat with cursor=next_cursor until null
Sequence diagram for paged manage_gameobject get_components with metadata-first behaviorsequenceDiagram
actor LLMClient
participant MCPServerPython as MCPServerPython
participant ManageGameObjectPy as manage_gameobject_py
participant UnityTransport as UnityTransport
participant ManageGameObjectCs as ManageGameObject_cs
LLMClient->>MCPServerPython: manage_gameobject(action=get_components,
LLMClient-->>MCPServerPython: target,page_size,cursor,max_components,
LLMClient-->>MCPServerPython: include_properties=false)
MCPServerPython->>ManageGameObjectPy: manage_gameobject(...)
ManageGameObjectPy->>ManageGameObjectPy: coerce_int page_size,cursor,max_components
ManageGameObjectPy->>ManageGameObjectPy: coerce_bool include_properties
ManageGameObjectPy->>UnityTransport: send_with_unity_instance(async_send_command_with_retry,
ManageGameObjectPy-->>UnityTransport: "manage_gameobject", params{action=get_components+paging}
UnityTransport->>ManageGameObjectCs: HandleCommand(params)
ManageGameObjectCs->>ManageGameObjectCs: GetComponentsFromTarget(target,
ManageGameObjectCs-->>ManageGameObjectCs: searchMethod,includeNonPublicSerialized,
ManageGameObjectCs-->>ManageGameObjectCs: pageSize,cursor,maxComponents,includeProperties=false)
ManageGameObjectCs->>ManageGameObjectCs: clamp pageSize,maxComponents
ManageGameObjectCs->>ManageGameObjectCs: components = targetGo.GetComponents()
ManageGameObjectCs->>ManageGameObjectCs: slice components[cursor:cursor+effectiveTake]
loop for each component in page
alt include_properties == false
ManageGameObjectCs->>ManageGameObjectCs: BuildComponentMetadata(Component)
else include_properties == true
ManageGameObjectCs->>ManageGameObjectCs: serialize component via GameObjectSerializer
ManageGameObjectCs->>ManageGameObjectCs: enforce maxPayloadChars budget
end
end
ManageGameObjectCs->>ManageGameObjectCs: compute next_cursor or null
ManageGameObjectCs-->>UnityTransport: SuccessResponse{cursor,pageSize,next_cursor,truncated,total,includeProperties,items}
UnityTransport-->>ManageGameObjectPy: response
ManageGameObjectPy-->>MCPServerPython: response
MCPServerPython-->>LLMClient: paged component metadata
LLMClient->>MCPServerPython: repeat with cursor=next_cursor
Note over LLMClient,MCPServerPython: When full properties needed, set include_properties=true with smaller page_size
Updated class diagram for hierarchy, component paging, and dev-mode controlsclassDiagram
class SceneCommand {
string action
string name
string path
int? buildIndex
string fileName
int? superSize
JToken parent
int? pageSize
int? cursor
int? maxNodes
int? maxDepth
int? maxChildrenPerNode
bool? includeTransform
}
class ManageScene_cs {
+object HandleCommand(JObject params)
-object GetSceneHierarchyPaged(SceneCommand cmd)
-GameObject ResolveGameObject(JToken targetToken, Scene activeScene)
-object BuildGameObjectSummary(GameObject go, bool includeTransform, int maxChildrenPerNode)
-string GetGameObjectPath(GameObject go)
}
class ManageGameObject_cs {
+object HandleCommand(JObject params)
-object GetComponentsFromTarget(string target, string searchMethod, bool includeNonPublicSerialized, int pageSize, int cursor, int maxComponents, bool includeProperties)
-object BuildComponentMetadata(Component c)
}
class ServerManagementService {
+bool StopLocalHttpServer()
-List~int~ GetListeningProcessIdsForPort(int port)
-static int GetCurrentProcessIdSafe()
-bool LooksLikeMcpServerProcess(int pid)
-bool TerminateProcess(int pid)
+bool TryGetLocalHttpServerCommand(out string command, out string error)
}
class CodexConfigHelper {
+string BuildCodexServerBlock(string uvPath)
+TomlTable CreateUnityMcpTable(string uvPath)
-static bool GetDevModeForceRefresh()
-static void AddDevModeArgs(TomlArray args, bool devForceRefresh)
}
class ConfigJsonBuilder {
+void PopulateUnityNode(JObject unity, string uvPath, McpClient client)
-static IList~string~ BuildUvxArgs(string fromUrl, string packageName, bool devForceRefresh)
}
class McpClientConfiguratorBase {
-void Register()
+string GetManualSnippet()
-static bool GetDevModeForceRefresh()
}
class McpSettingsSection {
-Toggle devModeForceRefreshToggle
-void CacheUIElements()
-void InitializeUI()
-void RegisterCallbacks()
+void UpdatePathOverrides()
}
class EditorPrefKeys {
<<static>>
+string DevModeForceServerRefresh
}
class manage_scene_py {
+async_manage_scene(ctx, action, name, build_index, screenshot_file_name, screenshot_super_size, parent, page_size, cursor, max_nodes, max_depth, max_children_per_node, include_transform) dict
}
class manage_gameobject_py {
+async_manage_gameobject(ctx, action, target, search_in_children, search_inactive, includeNonPublicSerialized, page_size, cursor, max_components, include_properties, new_name, offset, component_name, component_properties) dict
}
class manage_asset_py {
+async_manage_asset(ctx, action, path, asset_type, properties, destination, generate_preview, search_pattern, filter_type, filter_date_after, page_size, page_number) dict
}
class utils_py {
+Any parse_json_payload(Any value)
+int~nullable~ coerce_int(Any value, int~nullable~ default)
}
SceneCommand <.. ManageScene_cs
ManageScene_cs <.. manage_scene_py
ManageGameObject_cs <.. manage_gameobject_py
manage_asset_py ..> utils_py
manage_scene_py ..> utils_py
manage_gameobject_py ..> utils_py
CodexConfigHelper ..> EditorPrefKeys
ConfigJsonBuilder ..> EditorPrefKeys
McpClientConfiguratorBase ..> EditorPrefKeys
McpSettingsSection ..> EditorPrefKeys
McpSettingsSection ..> CodexConfigHelper
ServerManagementService ..> EditorPrefKeys
Flow diagram for manage_asset search normalization and paging safetyflowchart TD
A_start[Start manage_asset
action=search] --> B_coerce[Coerce page_size,page_number
using coerce_int]
B_coerce --> C_pathRaw[Read raw path string]
C_pathRaw --> D_checkQuery{search_pattern is empty
AND path starts with t:}
D_checkQuery -->|Yes| E_normalizePath[Set search_pattern = path
Set path = Assets
Emit ctx.info normalization]
D_checkQuery -->|No| F_skipPath[Keep original path/search_pattern]
E_normalizePath --> G_filterType
F_skipPath --> G_filterType
G_filterType{filter_type is empty
AND asset_type is non-empty string} -->|Yes| H_mapFilter[Set filter_type = asset_type
Emit ctx.info mapping]
G_filterType -->|No| I_keepFilter[Keep filter_type]
H_mapFilter --> J_buildParams
I_keepFilter --> J_buildParams
J_buildParams[Build params_dict with
action,path,search_pattern,
filter_type,page_size,page_number,
generate_preview, etc.] --> K_sendUnity[send_with_unity_instance
async_send_command_with_retry
manage_asset C# handler]
K_sendUnity --> L_end[End]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis PR implements three major features: a dev-mode force-refresh mechanism for MCP server installation caching to enable faster iteration, safer multi-process termination logic to avoid killing the Unity Editor, and pagination support for scene hierarchy and component retrieval with configurable safety limits and optional property inclusion. Changes
Sequence DiagramssequenceDiagram
participant Client as MCP Client
participant Server as MCP Server<br/>(Python)
participant EditorService as Editor Service<br/>(Unity)
participant Database as Scene/Assets<br/>Database
rect rgba(100, 150, 200, 0.2)
note over Client,Server: Get Hierarchy with Paging
Client->>Server: manage_scene(action: get_hierarchy,<br/>pageSize: 10, cursor: 0)
Server->>EditorService: HandleCommand(get_hierarchy,<br/>pageSize, cursor, max_nodes)
EditorService->>Database: GetSceneHierarchyPaged()
Database->>EditorService: [Page of GameObjects]
EditorService->>EditorService: BuildGameObjectSummary<br/>for each item
EditorService->>Server: {items: [...], next_cursor, truncated}
Server->>Client: ✓ {items, next_cursor, truncated,<br/>pageSize, total}
end
rect rgba(150, 100, 200, 0.2)
note over Client,Server: Retrieve Next Page via Cursor
Client->>Server: manage_scene(action: get_hierarchy,<br/>pageSize: 10, cursor: <next_cursor>)
Server->>EditorService: HandleCommand(get_hierarchy,<br/>pageSize, cursor)
EditorService->>Database: GetSceneHierarchyPaged()
Database->>EditorService: [Next Page of GameObjects]
EditorService->>Server: {items: [...], next_cursor, truncated}
Server->>Client: ✓ {items, next_cursor, truncated}
end
rect rgba(200, 150, 100, 0.2)
note over Client,Server: Get Components with Properties
Client->>Server: manage_gameobject(action: get_components,<br/>target: "Player", includeProperties: true)
Server->>EditorService: HandleCommand(get_components,<br/>pageSize, includeProperties)
EditorService->>Database: GetComponentsFromTarget()
Database->>EditorService: [Components]
EditorService->>EditorService: BuildComponentMetadata<br/>+ properties (if enabled)
EditorService->>Server: {items: [...], truncated}
Server->>Client: ✓ {items, pageSize, total, truncated}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Several Python tools (
manage_scene,manage_gameobject,read_console,run_tests) now callcoerce_int/coerce_bool; double‑check that each module imports these fromservices.tools.utilsto avoid runtimeNameErrors. - In
GetComponentsFromTarget, the payload budgeting forincludeProperties=truecallsJToken.FromObject(...).ToString(...)for every component, which double‑serializes each item; consider a cheaper heuristic (e.g., capping item count or using a rough size estimate) to reduce per‑call overhead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several Python tools (`manage_scene`, `manage_gameobject`, `read_console`, `run_tests`) now call `coerce_int`/`coerce_bool`; double‑check that each module imports these from `services.tools.utils` to avoid runtime `NameError`s.
- In `GetComponentsFromTarget`, the payload budgeting for `includeProperties=true` calls `JToken.FromObject(...).ToString(...)` for every component, which double‑serializes each item; consider a cheaper heuristic (e.g., capping item count or using a rough size estimate) to reduce per‑call overhead.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Tools/ManageScene.cs:577-586` </location>
<code_context>
+ private static GameObject ResolveGameObject(JToken targetToken, Scene activeScene)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** GameObject.Find is not scoped to the provided Scene and may return objects from other loaded scenes.
In the path-based branch, `GameObject.Find(s)` ignores `activeScene` and searches all loaded scenes, so a path meant for `activeScene` could resolve to a same-path object in another scene. If you want to keep lookups scoped, consider traversing `activeScene.GetRootGameObjects()` (as in your name-based search) and only use `GameObject.Find` when cross-scene lookup is explicitly desired.
Suggested implementation:
```csharp
private static GameObject FindInSceneByPath(Scene scene, string path)
{
if (!scene.IsValid() || !scene.isLoaded || string.IsNullOrEmpty(path))
return null;
// Normalize to avoid leading/trailing slashes interfering with segment names
var trimmed = path.Trim().Trim('/');
if (string.IsNullOrEmpty(trimmed))
return null;
var segments = trimmed.Split('/');
if (segments.Length == 0)
return null;
// First segment must match a root GameObject in this scene
var roots = scene.GetRootGameObjects();
GameObject current = null;
foreach (var root in roots)
{
if (root != null && root.name == segments[0])
{
current = root;
break;
}
}
if (current == null)
return null;
// Walk down the hierarchy using Transform.Find scoped under the matched root
for (var i = 1; i < segments.Length && current != null; i++)
{
var childTransform = current.transform.Find(segments[i]);
current = childTransform != null ? childTransform.gameObject : null;
}
return current;
}
private static GameObject ResolveGameObject(JToken targetToken, Scene activeScene)
```
Inside `ResolveGameObject`, locate the branch that handles string/“path-based” targets (the one currently using `GameObject.Find(...)` to resolve a path string). Replace those `GameObject.Find(pathString)` calls with `FindInSceneByPath(activeScene, pathString)` to keep lookups scoped to `activeScene`. If you truly need cross-scene lookup in some cases, keep `GameObject.Find` only in those explicit branches (e.g., guarded by a flag or a specific token marker indicating cross-scene resolution).
</issue_to_address>
### Comment 2
<location> `MCPForUnity/Editor/Services/ServerManagementService.cs:282-287` </location>
<code_context>
}
- private int GetProcessIdForPort(int port)
+ private List<int> GetListeningProcessIdsForPort(int port)
{
+ var results = new List<int>();
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The netstat-based PID parsing on Windows doesn’t distinguish LISTENING entries from client connections.
On Windows, `netstat -ano | findstr :{port}` returns both LISTENING and ESTABLISHED (and other) entries. The current logic takes the PID from any matching line, so it can capture client processes instead of the actual listener, even though `LooksLikeMcpServerProcess` reduces this somewhat. Please restrict PID collection to LISTENING lines only (e.g., filter by state in code or add `findstr LISTENING`), so you only return PIDs for listening sockets on that port.
Suggested implementation:
```csharp
var parts = line.Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries);
// On Windows, netstat output includes multiple states (LISTENING, ESTABLISHED, etc.).
// We only want PIDs for LISTENING sockets on the given port.
if (parts.Length > 3
&& parts[3].Equals("LISTENING", StringComparison.OrdinalIgnoreCase)
&& int.TryParse(parts[parts.Length - 1], out int pid))
{
results.Add(pid);
}
}
```
If the netstat invocation is constructed elsewhere (e.g., as `"netstat -ano | findstr :{port}"`), you could further harden this by adding a `LISTENING` filter at the shell level, such as `"netstat -ano | findstr :{port} | findstr LISTENING"`. That change would need to be applied where the process start info/command line for netstat is built.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| private List<int> GetListeningProcessIdsForPort(int port) | ||
| { | ||
| var results = new List<int>(); | ||
| try | ||
| { | ||
| string stdout, stderr; |
There was a problem hiding this comment.
suggestion (bug_risk): The netstat-based PID parsing on Windows doesn’t distinguish LISTENING entries from client connections.
On Windows, netstat -ano | findstr :{port} returns both LISTENING and ESTABLISHED (and other) entries. The current logic takes the PID from any matching line, so it can capture client processes instead of the actual listener, even though LooksLikeMcpServerProcess reduces this somewhat. Please restrict PID collection to LISTENING lines only (e.g., filter by state in code or add findstr LISTENING), so you only return PIDs for listening sockets on that port.
Suggested implementation:
var parts = line.Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries);
// On Windows, netstat output includes multiple states (LISTENING, ESTABLISHED, etc.).
// We only want PIDs for LISTENING sockets on the given port.
if (parts.Length > 3
&& parts[3].Equals("LISTENING", StringComparison.OrdinalIgnoreCase)
&& int.TryParse(parts[parts.Length - 1], out int pid))
{
results.Add(pid);
}
}If the netstat invocation is constructed elsewhere (e.g., as "netstat -ano | findstr :{port}"), you could further harden this by adding a LISTENING filter at the shell level, such as "netstat -ano | findstr :{port} | findstr LISTENING". That change would need to be applied where the process start info/command line for netstat is built.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs (1)
311-325: Consider Unity-idiomatic alternatives to Thread.Sleep polling.The busy-wait pattern with
Thread.Sleep(50)is functional and appropriate for test code, but Unity Editor callbacks might be more idiomatic. The current implementation successfully addresses the test determinism issues mentioned in the PR objectives.Alternative approach using EditorApplication.update
If you want to avoid blocking the main thread with Thread.Sleep, consider using EditorApplication.update callbacks:
private static void WaitForUnityReady(double timeoutSeconds = 30.0) { double start = EditorApplication.timeSinceStartup; bool ready = false; System.Action checkReady = null; checkReady = () => { if (!EditorApplication.isCompiling && !EditorApplication.isUpdating) { ready = true; EditorApplication.update -= checkReady; } else if (EditorApplication.timeSinceStartup - start > timeoutSeconds) { EditorApplication.update -= checkReady; Assert.Fail($"Timed out waiting for Unity to finish compiling/updating (>{timeoutSeconds:0.0}s)."); } }; EditorApplication.update += checkReady; // Wait for callback to complete while (!ready && EditorApplication.timeSinceStartup - start <= timeoutSeconds) { Thread.Sleep(10); // Brief sleep to yield } }However, the current polling approach is simpler and works well for test scenarios.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (2)
72-106: Test logic is correct, but consider adding a clarity comment.The test adds 2 components (Rigidbody, BoxCollider) to a GameObject that already has a Transform component (3 total). With
pageSize=2, the assertion at line 99 expects exactly 2 items, which correctly validates paging behavior. However, this implicit assumption about the Transform component could be made more explicit with a comment.Optional: Add comment clarifying total component count
// Arrange testGameObject.AddComponent<Rigidbody>(); testGameObject.AddComponent<BoxCollider>(); + // Note: GameObject already has Transform, so total is 3 components
108-145: Consider strengthening the properties assertion.Line 144 uses a heuristic (
Properties().Count() > 2) to verify that properties are included. This could be fragile if component serialization changes. A more robust approach would be to directly check for a"properties"key in the first item.Recommended: Check for properties key directly
- // Heuristic: property-including payload should have more than just typeName/instanceID. - Assert.Greater(first.Properties().Count(), 2, "Expected richer component payload when includeProperties=true."); + // When includeProperties=true, the component should include a "properties" field. + Assert.IsNotNull(first["properties"], "Expected 'properties' field when includeProperties=true.");docs/README-DEV.md (1)
172-206: LGTM! Comprehensive paging documentation.The payload sizing and paging defaults are well-documented with clear practical recommendations. The documentation accurately reflects the implementation (page_size defaults, max limits, include_properties behavior).
Optional: Style improvement for "very large"
Line 174: Static analysis suggests replacing "very large" with a more specific term like "unbounded" or "extremely large" for stronger technical writing. However, "very large" is perfectly clear in this context.
Server/tests/integration/test_tool_signatures_paging.py (1)
1-32: LGTM! Good signature validation tests.These tests effectively prevent regression by ensuring paging parameters remain in the tool signatures. The approach using
inspect.signatureis appropriate for integration testing.Optional: Consider validating parameter types
The current tests only verify parameter presence. For stronger validation, you could also check parameter types and defaults:
def test_manage_scene_signature_includes_paging_params(): import services.tools.manage_scene as mod sig = inspect.signature(mod.manage_scene) params = sig.parameters # Check presence and types assert "page_size" in params assert params["page_size"].annotation in (int | str, "int | str") # depending on Python version assert params["page_size"].default is NoneHowever, this adds maintenance overhead and may be overkill for integration tests.
Server/src/services/tools/utils.py (1)
63-77: LGTM! Solid defensive coercion utility.The
coerce_intfunction correctly handles edge cases (None, booleans, empty strings) and provides sensible fallback behavior. The implementation mirrors the existingcoerce_boolpattern, which is good for consistency.Optional: Narrow exception handling
Line 76: Static analysis suggests avoiding bare
Exception. While the defensive catch-all is acceptable here (and consistent with the tolerant design), you could narrow it to specific exceptions for slightly better error visibility:- except Exception: + except (ValueError, TypeError, AttributeError): return defaultHowever, given this is a defensive utility meant to never raise, the current approach is also reasonable.
Server/src/services/tools/read_console.py (1)
9-9: LGTM! Good refactoring to shared utilities.Moving from local coercion helpers to centralized utilities in
utils.pyreduces code duplication and improves maintainability. The explicitdefault=Trueforinclude_stacktracemakes the intent clear.Also applies to: 42-42, 49-49
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (1)
9-9: Verify UnityEngine usage.The
using UnityEngine;directive was added, but it's not clearly used in the changed code. Consider removing if unused, or keep if required by UnityEditor dependencies.MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
425-429: Dev-mode CLI flags are composed correctly; consider centralizing the prefs helperThe
devFlagsstring ("--no-cache --refresh ") is correctly inserted before--fromin both the runtime args and manual snippet, and theGetDevModeForceRefreshhelper is defensively implemented aroundEditorPrefs.If you find yourself using this flag in more places (Codex helpers already have a similar helper), consider centralizing
GetDevModeForceRefreshin a shared utility to avoid drift in behavior, but the current implementation is functionally sound.Also applies to: 520-551, 553-557
MCPForUnity/Editor/Services/ServerManagementService.cs (1)
345-394: Solid heuristic with good safety defaults.The explicit rejection of Unity/UnityHub processes (lines 372-375) and the fail-safe
return falsedefault are good defensive measures.Minor: Line 357 has slight redundancy—
combined.Contains("uvx")already coversuvx.exe. Consider simplifying:-return combined.Contains("python") || combined.Contains("uvx") || combined.Contains("uv.exe") || combined.Contains("uvx.exe"); +return combined.Contains("python") || combined.Contains("uvx") || combined.Contains("uv.exe");
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.csMCPForUnity/Editor/Constants/EditorPrefKeys.csMCPForUnity/Editor/Helpers/CodexConfigHelper.csMCPForUnity/Editor/Helpers/ConfigJsonBuilder.csMCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Tools/ManageGameObject.csMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.csMCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.uxmlServer/pyproject.tomlServer/src/main.pyServer/src/services/tools/debug_request_context.pyServer/src/services/tools/manage_asset.pyServer/src/services/tools/manage_gameobject.pyServer/src/services/tools/manage_scene.pyServer/src/services/tools/read_console.pyServer/src/services/tools/run_tests.pyServer/src/services/tools/utils.pyServer/tests/integration/test_debug_request_context_diagnostics.pyServer/tests/integration/test_manage_gameobject_param_coercion.pyServer/tests/integration/test_manage_scene_paging_params.pyServer/tests/integration/test_tool_signatures_paging.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSceneHierarchyPagingTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSceneHierarchyPagingTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.csdocs/README-DEV.md
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Applied to files:
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.uxmldocs/README-DEV.md
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
Applied to files:
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.uxmldocs/README-DEV.md
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
Server/src/services/tools/run_tests.pydocs/README-DEV.mdServer/src/services/tools/read_console.pyServer/src/services/tools/manage_scene.pyServer/src/services/tools/manage_gameobject.py
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
Server/src/services/tools/run_tests.pydocs/README-DEV.mdMCPForUnity/Editor/Helpers/CodexConfigHelper.csServer/src/services/tools/manage_gameobject.pyMCPForUnity/Editor/Services/ServerManagementService.cs
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.csMCPForUnity/Editor/Helpers/CodexConfigHelper.csMCPForUnity/Editor/Tools/ManageGameObject.csMCPForUnity/Editor/Services/ServerManagementService.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
docs/README-DEV.mdMCPForUnity/Editor/Helpers/CodexConfigHelper.csMCPForUnity/Editor/Services/ServerManagementService.cs
🧬 Code graph analysis (15)
Server/src/services/tools/run_tests.py (1)
Server/src/services/tools/utils.py (1)
coerce_int(63-77)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (1)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
HandleCommand(42-253)
Server/tests/integration/test_manage_gameobject_param_coercion.py (2)
Server/src/services/tools/manage_gameobject.py (1)
manage_gameobject(16-254)Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (4)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
GetDevModeForceRefresh(553-557)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-49)MCPForUnity/Editor/External/Tommy.cs (1)
TomlString(168-193)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
uvxPath(169-176)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (1)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (2)
MCPServiceLocator(11-91)Register(40-62)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSceneHierarchyPagingTests.cs (1)
MCPForUnity/Editor/Tools/ManageScene.cs (1)
HandleCommand(90-191)
Server/src/services/tools/debug_request_context.py (1)
Server/src/core/telemetry.py (1)
get_package_version(66-80)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (3)
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (1)
GetDevModeForceRefresh(20-30)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
uvxPath(169-176)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-49)
Server/src/services/tools/manage_scene.py (1)
Server/src/services/tools/utils.py (2)
coerce_int(63-77)coerce_bool(11-24)
Server/tests/integration/test_debug_request_context_diagnostics.py (1)
Server/src/services/tools/debug_request_context.py (1)
debug_request_context(16-80)
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs (1)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-49)
MCPForUnity/Editor/Tools/ManageScene.cs (1)
MCPForUnity/Editor/Tools/ManageGameObject.cs (2)
Type(2557-2571)GameObject(1604-1627)
Server/tests/integration/test_tool_signatures_paging.py (2)
Server/src/services/tools/manage_scene.py (1)
manage_scene(14-90)Server/src/services/tools/manage_gameobject.py (1)
manage_gameobject(16-254)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
MCPForUnity/Editor/Helpers/GameObjectSerializer.cs (3)
JToken(537-556)GameObjectSerializer(17-557)GetComponentData(122-429)
Server/src/services/tools/manage_gameobject.py (1)
Server/src/services/tools/utils.py (3)
coerce_bool(11-24)parse_json_payload(27-60)coerce_int(63-77)
🪛 LanguageTool
docs/README-DEV.md
[style] ~174-~174: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ded) Some Unity tool calls can return very large JSON payloads (deep hierarchies, compo...
(EN_WEAK_ADJECTIVE)
🪛 Ruff (0.14.10)
Server/tests/integration/test_manage_gameobject_param_coercion.py
43-43: Unused function argument: cmd
(ARG001)
43-43: Unused function argument: kwargs
(ARG001)
Server/src/main.py
34-35: try-except-pass detected, consider logging the exception
(S110)
34-34: Do not catch blind exception: Exception
(BLE001)
Server/tests/integration/test_manage_scene_paging_params.py
11-11: Unused function argument: cmd
(ARG001)
11-11: Unused function argument: kwargs
(ARG001)
Server/src/services/tools/utils.py
76-76: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (35)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs (2)
6-6: LGTM!The
System.Threadingimport is necessary for theThread.Sleepcall in the newWaitForUnityReadyhelper method.
26-26: LGTM!The
WaitForUnityReady()calls are strategically placed to ensure Unity is in a stable state before and after asset operations, which improves test determinism.Also applies to: 50-50
Server/src/main.py (1)
272-283: Excellent documentation addition for payload safety.This guidance clearly communicates paging strategies and payload-sizing best practices to LLM clients, directly addressing the PR's goal of preventing large-response crashes and timeouts. The specific parameter recommendations (page sizes,
include_properties=falsefor initial queries,generate_preview=false) provide actionable defaults that will help models avoid overwhelming single responses.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSceneHierarchyPagingTests.cs.meta (1)
1-11: LGTM! Standard Unity metadata file.This is a standard Unity
.metafile for the new test file. No issues detected.docs/README-DEV.md (1)
76-76: LGTM! Clear dev mode documentation.The description accurately reflects the new
DevModeForceServerRefreshfeature and explains the trade-off (reliability vs. speed).MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
23-23: LGTM! Consistent constant addition.The new
DevModeForceServerRefreshkey follows the established naming convention and is properly namespaced with theMCPForUnity.prefix.Server/src/services/tools/debug_request_context.py (1)
1-80: LGTM! Useful diagnostic additions.The new
serverfield provides valuable debugging context (version, cwd, argv) without introducing security concerns. The implementation is clean and follows the existing pattern for other diagnostic fields.Server/src/services/tools/run_tests.py (2)
10-10: LGTM! Good refactoring to use shared utility.Importing
coerce_intfrom the shared utils module reduces code duplication and ensures consistent coercion behavior across tools.
70-72: LGTM! Clean replacement of local coercion logic.Line 70 correctly uses the shared
coerce_intutility, removing the need for the local_coerce_inthelper. The behavior is preserved while improving maintainability.MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.uxml (1)
45-50: LGTM! Well-structured dev mode UI addition.The new Dev Mode section follows existing UXML patterns and provides clear guidance about the performance tradeoff (slower startup for fresh builds).
Server/tests/integration/test_debug_request_context_diagnostics.py (1)
4-27: LGTM! Clean test coverage for server diagnostics.The test properly stubs dependencies and verifies the new server diagnostics payload structure.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (3)
35-36: LGTM! Proper test state management.The new fields track DevModeForceServerRefresh state correctly, following the established pattern for other EditorPrefs in this test class.
Also applies to: 46-47
58-60: Good defensive test setup.Forcing
DevModeForceServerRefreshto false ensures deterministic test behavior regardless of developer settings. The comment clearly explains why this matters for arg-order assertions.
76-79: LGTM! Complete test cleanup.The TearDown and OneTimeTearDown methods properly restore both the PlatformService and EditorPrefs state, preventing test pollution.
Also applies to: 102-110
Server/tests/integration/test_manage_gameobject_param_coercion.py (2)
35-36: LGTM! Stricter boolean type verification.The assertions now verify that string inputs like
"true"and"0"are properly coerced to booleanTrueandFalse, ensuring the coercion logic works correctly.
39-72: LGTM! Good coverage for paging parameter coercion.The test validates that paging-related parameters are correctly coerced from strings to their expected types (integers and booleans) and properly mapped to the Unity command payload.
Server/tests/integration/test_manage_scene_paging_params.py (1)
37-42: Verify lenient assertion style.The assertions here allow either integers or strings (e.g.,
p["pageSize"] in (10, "10")), which differs from the stricter type checking intest_manage_gameobject_param_coercion.py(Line 69:assert p["pageSize"] == 25).Should these assertions be strict to verify coercion actually occurred, or is this intentional to test that the function accepts both forms?
For consistency with the gameobject test, consider:
🔎 Proposed strict assertions
- assert p["pageSize"] in (10, "10") - assert p["cursor"] in (20, "20") - assert p["maxNodes"] in (1000, "1000") - assert p["maxDepth"] in (6, "6") - assert p["maxChildrenPerNode"] in (200, "200") - assert p["includeTransform"] in (True, "true") + assert p["pageSize"] == 10 + assert p["cursor"] == 20 + assert p["maxNodes"] == 1000 + assert p["maxDepth"] == 6 + assert p["maxChildrenPerNode"] == 200 + assert p["includeTransform"] is TrueMCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (2)
84-86: LGTM! Defensive EditorPrefs access.The try-catch ensures EditorPrefs failures don't break configuration generation, safely defaulting to
false(dev mode disabled).
155-177: LGTM! Clear dev mode flag implementation.The function correctly prepends cache-busting flags when dev mode is enabled, with helpful comments explaining the flag purposes and ordering consistency.
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (3)
20-30: LGTM! Consistent EditorPrefs access pattern.The implementation matches
ConfigJsonBuilder.csline 84-86, providing consistent defensive behavior across the codebase.
32-38: LGTM! Clean helper for TOML manipulation.The helper properly handles null checks and encapsulates the TOML-specific logic for adding dev mode flags.
62-67: LGTM! Consistent dev mode integration.Both stdio paths correctly retrieve the dev mode flag and apply the cache-busting arguments in the proper order (dev flags →
--from→ package name → transport flags), consistent withConfigJsonBuilder.cs.Also applies to: 212-217
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs (1)
33-91: Dev-mode refresh toggle wiring looks consistent and safeThe new
devModeForceRefreshToggleis correctly:
- Queried from UXML,
- Initialized from
EditorPrefKeys.DevModeForceServerRefresh,- Kept in sync in
UpdatePathOverrides, and- Propagated via
OnHttpServerCommandUpdateRequestedon changes.The only behavioral side effect is that
UpdatePathOverrideswill re-fire the value-changed callback when the stored value changes, which is harmless and keeps server args up to date. No changes needed from a correctness standpoint.Also applies to: 108-112, 157-161, 188-218
Server/src/services/tools/manage_asset.py (1)
12-13: Search normalization and paging coercion are robust and align with payload-safety goals
page_size/page_numbercoercion viacoerce_inttolerates string/float inputs while rejecting booleans, which matches the union type.- For
action=="search", movingt:filters frompathintosearch_patternand scopingpathto"Assets"should effectively prevent unintended full-project scans.- Mapping
asset_typeintofilter_typewhen the latter is unset is a good compatibility bridge for callers that overloadedasset_typefor search.No functional issues spotted in the new logic.
Also applies to: 17-23, 27-47, 92-118
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSceneHierarchyPagingTests.cs (1)
8-103: Hierarchy paging test is well-scoped and non-brittleThe test:
- Forces paging with
pageSize=10and assertsscope,truncated,next_cursor, and item counts.- Validates child paging via
parent(instance ID) with a smaller page size.- Cleans up all created objects in
TearDown.The coverage matches the new
GetSceneHierarchyPagedbehavior without overfitting to specific scene contents.Server/src/services/tools/manage_gameobject.py (1)
10-11: Paging and include_properties parameters are correctly coerced and forwarded
page_size,cursor, andmax_componentsare safely coerced withcoerce_int, and omitted on failure rather than sending bad types.include_propertiesis normalized viacoerce_bool, matching the C# side’s expectation of a strict bool.- The outgoing keys (
pageSize,cursor,maxComponents,includeProperties) align with the newGetComponentsFromTargetsignature, while remaining optional so older flows still behave as before.This is a clean, backwards-compatible extension of the tool surface.
Also applies to: 71-76, 132-147, 181-220
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
177-221: get_components paging and summary-first behavior are implemented correctly
- The
get_componentscase now:
- Accepts both
pageSize/page_sizeandmaxComponents/max_components, plusincludeProperties/include_properties.- Applies sensible defaults (25 page size, 0 cursor, 50 maxComponents) and clamps them inside
GetComponentsFromTarget, which guards against pathological payloads.GetComponentsFromTarget:
- Builds a stable component list once, paginates by cursor +
effectiveTake, and returns a structured payload (cursor,pageSize,next_cursor,truncated,total,items).- Returns lightweight metadata via
BuildComponentMetadatawhenincludeProperties == false, matching the “summary-first, deep-on-demand” design.- Enforces a rough 250k-character cap when
includePropertiesis true, stopping early and usingnext_cursorto allow further paging without ever throwing on oversized components.- Downgrades serialization errors per-component into error objects instead of failing the whole call.
Overall this is a solid hardening of
get_componentswith good defaults and safe failure modes.Also applies to: 1230-1342, 1344-1357
MCPForUnity/Editor/Tools/ManageScene.cs (2)
21-38: SceneCommand paging fields and routing to GetSceneHierarchyPaged are coherent and backward compatible
- New
SceneCommandfields (parent,pageSize,cursor,maxNodes,maxDepth,maxChildrenPerNode,includeTransform) are wired viaToSceneCommand, with:
- Numeric coercion through
BIand support for both camelCase and snake_case keys.- Boolean coercion for
includeTransformviaBB, tolerating"true","false","1","0", and empty strings.HandleCommandnow delegates"get_hierarchy"toGetSceneHierarchyPaged(cmd)but keeps other actions unchanged, so existing behavior is preserved outside hierarchy queries.This keeps the public contract stable while adding the new paging/safety surface.
Also applies to: 40-85, 148-176, 488-575
488-575: Hierarchy paging implementation is defensively bounded and matches the new tests
GetSceneHierarchyPaged:
- Validates that an active, loaded scene exists.
- Applies conservative, clamped defaults (
pageSizedefault 50,maxNodesdefault 1000) and computeseffectiveTake = min(pageSize, maxNodes)to cap both per-page and total work.- Distinguishes two scopes:
scope="roots"whenparentis null, usingGetRootGameObjects().scope="children"when a parent is supplied and resolved viaResolveGameObject, iterating only direct children.- Computes
cursor,next_cursor,truncated, andtotalconsistently with the paging tests.ResolveGameObjectsupports:
- Instance IDs via
EditorUtility.InstanceIDToObject,- Simple hierarchy paths via
GameObject.Find(path), and- Name searches (including inactive children) within the active scene.
BuildGameObjectSummary:
- Returns a compact node summary (name, instanceID, active flags, tag/layer/static, full
path, child count).- Omits inlined children by design, instead exposing
childrenCursorandchildrenPageSizeDefaultalongside achildrenTruncatedflag to encourage child-level paging.- Optionally includes local transform data when
includeTransformis true.The implementation is consistent with the Python-side
manage_scenepaging parameters and the new Unity tests.Also applies to: 577-622, 624-681
Server/src/services/tools/manage_scene.py (1)
6-8: Paging and safety parameters are cleanly plumbed through to the Unity side
- New arguments (
parent,page_size,cursor,max_nodes,max_depth,max_children_per_node,include_transform) are optional and normalized viacoerce_int/coerce_bool, preventing bad types from reaching C#.- The outgoing keys (
pageSize,cursor,maxNodes,maxDepth,maxChildrenPerNode,includeTransform) align withSceneCommandandGetSceneHierarchyPagedinManageScene.cs.- Existing behavior for non-
get_hierarchyactions is unaffected, since these fields are only added when non-None.The parameter handling is straightforward and maintains compatibility with earlier callers.
Also applies to: 31-38, 43-52, 53-79
MCPForUnity/Editor/Services/ServerManagementService.cs (5)
233-273: Good safety improvements for multi-PID handling.The guardrails preventing termination of the Unity Editor process and non-MCP-like processes are well-implemented. The logging is clear for debugging refused terminations.
282-337: LGTM!The platform-aware implementation is solid. Using
-nPflags on lsof avoids DNS lookups for faster/more reliable operation, and-sTCP:LISTENcorrectly filters to only listening sockets. Deduplication handles cases where the same PID may appear multiple times (e.g., IPv4/IPv6 bindings).
339-343: LGTM!Simple defensive helper with safe fallback. Returning -1 on failure correctly prevents false matches against valid PIDs.
396-427: Graceful-then-forceful termination pattern is appropriate.Minor note: On Unix,
kill -15returns success if the signal was sent, not if the process has terminated. The escalation to-9only triggers if the signal couldn't be sent (e.g., permission denied). For this use case, this is acceptable—if the old server doesn't terminate quickly, the new server will fail to bind and the user will see an error. Adding a wait-and-verify loop would add complexity for marginal benefit.
458-464: LGTM!Dev mode flag integration is clean. The safe fallback to
falseon exception and the correct placement of--no-cache --refreshbefore the package name follow uvx CLI conventions.
| dependencies = [ | ||
| "httpx>=0.27.2", | ||
| "fastmcp>=2.13.0,<2.13.2", | ||
| "fastmcp==2.14.1", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
fastmcp 2.14.1 changelog breaking changes release
💡 Result:
FastMCP 2.14.1 (released Dec 15, 2025) — summary of notable changes and breaking items:
-
Main features: sampling-with-tools (SEP-1577) and new ctx.sample_step(), result_type for structured outputs, AnthropicSamplingHandler + OpenAI handler promoted to production. [1][3]
-
Breaking / migration notes:
- 2.14 series removes several deprecated 2.x APIs (users should pin versions and audit for any APIs previously marked deprecated). [1][2]
- Earlier breaking changes called out in the changelog that still apply when upgrading (decorate-return behavior, websockets optional, server helper-method deprecations) — review your code for: decorators now return the objects they create (not the original function), websockets may need to be added as an extra dependency if you rely on it, and automatic server conversion helper methods are deprecated in favor of using the decorators directly. [2][4]
Sources:
[1] FastMCP Updates — FastMCP 2.14.1 (release notes).
[2] FastMCP Changelog (breaking changes and deprecations).
[3] PyPI fastmcp 2.14.1 upload metadata.
🌐 Web query:
fastmcp PyPI 2.14.1 version security
💡 Result:
Short answer: fastmcp 2.14.1 includes the security fixes — it is >= the releases that fixed the known FastMCP advisories, so it is not vulnerable to the disclosed issues affecting <2.13.0 and the MCP-SDK-related issue fixed in 2.14.0. See sources below.
Details and sources:
- PyPI release 2.14.1 (uploaded Dec 15, 2025). [1]
- Public advisories / CVEs: reflected XSS and auth “confused deputy” issues (CVE/GHSA) and a Windows command‑injection — all fixed in 2.13.0. [2][3][4]
- An MCP-SDK-related advisory that required FastMCP to require newer SDK versions was addressed in 2.14.0 (so 2.14.1 includes that fix). [5]
Recommendation: upgrade to (or keep) fastmcp >= 2.14.1 and review the FastMCP security advisories / changelog for any mitigation steps relevant to your deployment.
Sources:
[1] PyPI fastmcp 2.14.1 release info.
[2] NVD / CVE-2025-62800 (XSS advisory).
[3] NVD / CVE-2025-62801 (Windows command injection).
[4] FastMCP GitHub security advisories summary.
[5] FastMCP release notes / updates (2.14.0+).
🏁 Script executed:
# Check how fastmcp is used in the codebase to understand potential impact
find . -type f -name "*.py" | head -20 | xargs grep -l "fastmcp" 2>/dev/null | head -10Repository: CoplayDev/unity-mcp
Length of output: 246
🏁 Script executed:
# Check the pyproject.toml file to see the broader context
cat -n Server/pyproject.tomlRepository: CoplayDev/unity-mcp
Length of output: 2248
Audit code for fastmcp 2.14 breaking changes before merging.
The upgrade to fastmcp 2.14.1 is necessary for the new pagination and dev-mode features, and the version is secure with recent critical fixes. However, fastmcp 2.14 removes several deprecated 2.x APIs that require code review:
- Decorator return behavior: Decorators now return the objects they create (not the original function)
- Websockets handling: Websockets support is now optional—verify it's explicitly included if needed
- Server helper methods: Automatic server conversion helpers are deprecated; use decorators directly instead
Review Server/src/services/custom_tool_service.py, active_tool.py, and custom_tools.py for deprecated API usage. The exact pinning (==2.14.1) is appropriate here to ensure stability against future breaking changes; if a 2.14.2 patch is released, update manually if needed.
🤖 Prompt for AI Agents
In Server/pyproject.toml at line 33, we upgraded fastmcp to 2.14.1 which removes
deprecated 2.x APIs — audit Server/src/services/custom_tool_service.py,
active_tool.py, and custom_tools.py for three changes: (1) update any decorators
that expected the original function to be returned so they now accept and handle
the decorator returning the created object (adjust call sites and tests to use
the returned object instead of the original function), (2) ensure websocket
functionality is explicitly enabled/imported where used (add explicit websocket
dependency or import and feature flag where websocket handlers are registered),
and (3) replace any automatic server conversion helper calls with the new
decorator-based APIs (apply the provided decorators directly to handlers and
remove helper usage). Keep the exact pin to ==2.14.1 and run tests to confirm
behavior.
There was a problem hiding this comment.
We're good, 2.14.1 is the latest version and fixes issues in 2.14. Can upgrade later if we see fit. We were bitten by pinning a ">" for fastmcp version a few weeks back, when they introduced a change that broke all mcp tools everywhere.
There was a problem hiding this comment.
@dsarno, understood! Your exact pinning strategy makes perfect sense given the prior breakage from range pins. Sticking with ==2.14.1 will give you stability and control over when to upgrade—wise choice after being bitten once. Thanks for the context!
✏️ Learnings added
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 490
File: Server/pyproject.toml:33-33
Timestamp: 2025-12-29T04:54:17.743Z
Learning: For fastmcp dependency in Server/pyproject.toml: prefer exact version pinning (e.g., fastmcp==2.14.1) rather than range pins like >=2.13.0, due to past issues where fastmcp introduced breaking changes that affected all MCP tools.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| # Workaround for environments where tool signature evaluation runs with a globals | ||
| # dict that does not include common `typing` names (e.g. when annotations are strings | ||
| # and evaluated via `eval()` during schema generation). | ||
| # Making these names available in builtins avoids `NameError: Annotated/Literal/... is not defined`. | ||
| try: # pragma: no cover - startup safety guard | ||
| import builtins | ||
| import typing as _typing | ||
|
|
||
| _typing_names = ( | ||
| "Annotated", | ||
| "Literal", | ||
| "Any", | ||
| "Union", | ||
| "Optional", | ||
| "Dict", | ||
| "List", | ||
| "Tuple", | ||
| "Set", | ||
| "FrozenSet", | ||
| ) | ||
| for _name in _typing_names: | ||
| if not hasattr(builtins, _name) and hasattr(_typing, _name): | ||
| setattr(builtins, _name, getattr(_typing, _name)) # type: ignore[attr-defined] | ||
| except Exception: | ||
| pass | ||
|
|
There was a problem hiding this comment.
Reconsider the builtins patching approach—this is a significant anti-pattern.
Modifying builtins to inject typing names is a workaround that can cause unexpected side effects and maintenance issues:
- Global namespace pollution: All modules and third-party libraries will see these additions, potentially causing conflicts or unexpected behavior.
- Masks root cause: The real issue is likely that schema generation (Pydantic/FastMCP) lacks the proper evaluation context for annotations.
- Debugging complexity: Future developers will struggle to understand where these builtins come from.
Better alternatives:
- Use
from __future__ import annotationsat the top of tool definition modules to defer annotation evaluation - Ensure schema generation passes the correct
globalns/localnstotyping.get_type_hints()or equivalent - Import typing names in the modules where they're used, wrapped in
if TYPE_CHECKING:blocks when needed only for static analysis - If FastMCP schema generation is the issue, consider opening an issue upstream or providing a proper globals dict during schema extraction
Additionally, the static analysis tools correctly flag the bare except Exception: pass pattern—at minimum, log the exception for debugging.
🔎 Alternative approach: Use proper TYPE_CHECKING imports
Instead of patching builtins, ensure tool modules use proper imports:
from __future__ import annotations
from typing import TYPE_CHECKING
if TYPE_CHECKING:
from typing import Annotated, Literal, Any, Union, Optional, Dict, List, Tuple, Set, FrozenSetIf the issue is in schema generation, provide a proper evaluation namespace:
import typing
import sys
# When evaluating annotations, provide a proper namespace
eval_globals = {
**sys.modules[module_name].__dict__,
**vars(typing),
}
typing.get_type_hints(func, globalns=eval_globals)🧰 Tools
🪛 Ruff (0.14.10)
34-35: try-except-pass detected, consider logging the exception
(S110)
34-34: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In Server/src/main.py around lines 11 to 36, remove the builtins patch that
injects typing names (global namespace pollution and maintenance hazard) and
replace it with a proper fix: delete the try/except block that mutates builtins;
instead add from __future__ import annotations to modules that define
tools/schemas, import necessary typing symbols guarded by typing.TYPE_CHECKING
where needed, and ensure any schema-generation code calls
typing.get_type_hints(..., globalns=...) with a supplied evaluation namespace
(or adjust the schema extractor to provide it). If you must keep a fallback, log
the caught exception rather than using a bare except: pass and limit the
fallback scope to only the schema-evaluation call rather than mutating builtins.
|
🔥 |
2 participants
What changed
This PR hardens MCP-for-Unity against large-response crashes/timeouts by making “paged + summary-first” the default pattern and by guiding (and correcting) common LLM usage.
manage_scene(get_hierarchy): defaultpage_sizelowered to 50 (items per page), keeping payloads bounded when callers omit paging params.manage_scene(get_hierarchy)manage_gameobject(get_components)(metadata-first; small pages forinclude_properties=true)manage_asset(search)(page results; avoid previews)manage_asset(search)tool schema: updated parameter docs to steer models towardsearch_pattern+ paging and away from thumbnails.manage_asset(search)calls likepath="t:MonoScript"into a safer server-side filtered search (search_pattern="t:MonoScript",path="Assets"), preventing “invalid folder ⇒ search entire project” behavior.CodexConfigHelperTestsnow forceDevModeForceServerRefresh=falsein setup to avoid arg-order failures when dev mode is enabled in the editor.Why
We’ve seen Unity freeze/crash risk from single-shot, high-payload responses (deep hierarchies, component properties), and token blowups from broad asset searches. The goal is to reduce peak single-response size and make safe behavior the default for naive agents.
A/B validation (same
MCPStressTestscene)Using the same scene asset (
Assets/Scenes/MCPStressTest.unity/MCPStressTest.unityinUnityMCPTests), we captured raw JSON responses and measured bytes (wc -c):A (old, single-shot)
get_hierarchy: 30,715 bytesget_components(Heavy_00): 14,139 bytesB (new, paged + summary-first)
get_hierarchy(page_size=50): 3,825 bytesStressRoot_00,page_size=10): 2,530 bytesget_componentsmetadata-only pages (page_size=10): 767 / 769 / 596 bytesget_componentsproperties-on-demand pages (include_properties=true,page_size=3): 1,876 / 1,998 bytesResult: peak single-response size dropped from 30,715 → 3,825 bytes (~8× smaller), with
include_properties=truestaying bounded under ~2KB per page in the test.Testing
CodexConfigHelperTestscases; rerun via MCP: 4/4 passed.python3 -m compileallok; server unit tests (non-integration) passed (83 passed, 2 skipped, 7 xpassed).Notes / Follow-ups
manage_asset(search)is now more forgiving for LLMs, but callers should still prefer:path="Assets",search_pattern="t:MonoScript",page_size=25,page_number=1,generate_preview=falseShould fix #455 #488 #377
Summary by Sourcery
Introduce payload-safe paging defaults and dev-mode refresh controls across Unity MCP hierarchy, component, and asset tooling to reduce large-response risks and improve diagnostics.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.