Payload-safe paging for hierarchy/components + safer asset search + docs #106
Conversation
* Fix test teardown to avoid dropping MCP bridge 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). * Avoid leaking PlatformService in CodexConfigHelperTests 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.
WalkthroughThis PR removes the ScriptableObject design doc, adds paging/safety to scene and gameobject queries, injects a Dev Mode force-refresh flag into CLI/stdio/uvx commands and UI, improves multi-PID server discovery and graceful termination, exposes server diagnostics, adds coerce_int and input normalization, and includes related tests, docs, and dependency pinning. Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Unity Editor (UI)
participant Settings as Settings UI
participant Config as MCP Client Configurator
participant UVX as UVX / uvx CLI (stdio/CLI)
participant Server as Local MCP HTTP Server
participant ProcMgr as ServerManagementService
rect rgb(240,248,255)
Editor->>Settings: toggle Dev Mode
Settings->>Editor: persist EditorPrefs.DevModeForceServerRefresh
Settings->>Config: update configurator outputs/snippets (include devFlags)
end
rect rgb(245,245,220)
Editor->>UVX: StartLocalHttpServer request (includes devFlags if enabled)
UVX->>Server: launch command with args (--no-cache --refresh ...)?
Server-->>UVX: started / listening PIDs
end
rect rgb(240,255,240)
Editor->>ProcMgr: StopLocalHttpServer
ProcMgr->>ProcMgr: GetListeningProcessIdsForPort
ProcMgr->>ProcMgr: filter out Editor PID via GetCurrentProcessIdSafe & LooksLikeMcpServerProcess
ProcMgr->>ProcMgr: TerminateProcess(pid) (graceful → force)
ProcMgr-->>Editor: stop result per-PID
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (1)
155-173: Argument order for--no-cache --refreshis inconsistent across implementations.ConfigJsonBuilder places
--no-cache --refreshafter--from <url>, while McpClientConfiguratorBase.cs (line 430) and ServerManagementService.cs (line 463) both place them before--from. Although most CLI tools are order-agnostic, this inconsistency should be harmonized—either reorder the args in ConfigJsonBuilder to match the other two files, or verify that uvx accepts arguments in this order.
🧹 Nitpick comments (12)
Server/src/services/tools/manage_asset.py (1)
118-120: Consider narrowing the exception catch.The broad
except Exceptionon line 119 is overly defensive. Sincepathhas a type annotation ofstr, the most likely failure mode is receiving an unexpected type. Consider catching onlyAttributeErrororTypeErrorto allow unexpected errors to propagate while still handling the intended edge case.🔎 Proposed refinement
- try: - raw_path = (path or "").strip() - except Exception: - raw_path = "" + try: + raw_path = (path or "").strip() + except (AttributeError, TypeError): + # Handle case where path is not a string despite type annotation + raw_path = ""Based on static analysis hints (Ruff BLE001).
Server/src/main.py (1)
278-281: Replace en-dashes with hyphens for consistency.The strings use en-dash characters (
–, U+2013) instead of hyphen-minus (-, U+002D) in ranges like10–25. While visually similar, this could cause issues if the text is parsed or copied. As flagged by Ruff (RUF001).🔎 Proposed fix
- - Start with `include_properties=false` (metadata-only) and small `page_size` (e.g. **10–25**). - - Only request `include_properties=true` when needed; keep `page_size` small (e.g. **3–10**) to bound payloads. + - Start with `include_properties=false` (metadata-only) and small `page_size` (e.g. **10-25**). + - Only request `include_properties=true` when needed; keep `page_size` small (e.g. **3-10**) to bound payloads. - `manage_asset(action="search")`: - - Use paging (`page_size`, `page_number`) and keep `page_size` modest (e.g. **25–50**) to avoid token-heavy responses. + - Use paging (`page_size`, `page_number`) and keep `page_size` modest (e.g. **25-50**) to avoid token-heavy responses.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (1)
108-145: Consider asserting exact item count for consistency.Line 136 uses
Assert.IsTrue(items.Count > 0)while the first test assertsAssert.AreEqual(2, items.Count). For consistency and stronger coverage, consider asserting the exact expected count here as well (the GameObject has Transform + Rigidbody + BoxCollider = 3 components, so withpageSize=2you'd expect exactly 2).🔎 Proposed fix
var items = data["items"] as JArray; Assert.IsNotNull(items); - Assert.IsTrue(items.Count > 0); + Assert.AreEqual(2, items.Count, "Expected exactly pageSize items.");TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSceneHierarchyPagingTests.cs (1)
106-107: Minor: Remove trailing blank lines.Extra blank lines at end of file.
MCPForUnity/Editor/Tools/ManageScene.cs (1)
605-619: Potential performance concern with name-based resolution.When resolving by name, this iterates through all root objects and their entire child hierarchies via
GetComponentsInChildren<Transform>. For scenes with deep hierarchies, this could be slow.Consider adding a fast-path exit once a match is found, or document that ID-based resolution is preferred for performance-critical paths.
🔎 Suggested optimization
The current implementation already returns on first match, which is good. However,
GetComponentsInChildrenallocates an array of all transforms before iterating. For very large hierarchies, consider using a recursive search that can exit early:// Alternative: recursive early-exit search GameObject FindByNameRecursive(Transform parent, string targetName) { foreach (Transform child in parent) { if (child.gameObject.name == targetName) return child.gameObject; var found = FindByNameRecursive(child, targetName); if (found != null) return found; } return null; }Server/src/services/tools/manage_gameobject.py (1)
101-114: Duplicated_coerce_inthelper across multiple files.This exact helper function is now duplicated in
manage_asset.py,read_console.py,manage_scene.py, and here. Consider extracting it toservices/tools/utils.pyalongside the existingcoerce_boolfunction to follow DRY principles.The broad
Exceptioncatch (flagged by Ruff BLE001) is acceptable here given the defensive coercion intent—it ensures any unexpected input gracefully falls back to the default.Proposed refactor to utils.py
# In Server/src/services/tools/utils.py, add: def coerce_int(value: Any, default: int | None = None) -> int | None: """Attempt to coerce a loosely-typed value to an integer.""" if value is None: return default try: if isinstance(value, bool): return default if isinstance(value, int): return int(value) s = str(value).strip() if s.lower() in ("", "none", "null"): return default return int(float(s)) except Exception: return defaultServer/tests/integration/test_manage_gameobject_param_coercion.py (1)
69-72: Consider tightening assertions to verify coercion.The assertions accept both coerced (
25) and original ("25") values, which is flexible but doesn't verify that coercion actually happened. Based on the implementation inmanage_gameobject.py, numeric values should be coerced to integers (e.g.,page_sizeshould be25, not"25").If coercion is expected, the assertions could be stricter:
Stricter assertions (if coercion is verified)
- assert p["pageSize"] in (25, "25") - assert p["cursor"] in (50, "50") - assert p["maxComponents"] in (100, "100") - assert p["includeProperties"] in (True, "true") + assert p["pageSize"] == 25 + assert p["cursor"] == 50 + assert p["maxComponents"] == 100 + assert p["includeProperties"] is TrueMCPForUnity/Editor/Helpers/CodexConfigHelper.cs (2)
42-52: Duplicated dev-mode flag logic and empty catch blocks.The
devForceRefreshreading and args injection pattern is duplicated betweenBuildCodexServerBlockandCreateUnityMcpTable. Additionally, the empty catch block silently swallows anyEditorPrefsaccess issues.Consider extracting the flag reading to a helper method:
Proposed helper extraction
private static bool GetDevModeForceRefresh() { try { return EditorPrefs.GetBool(EditorPrefKeys.DevModeForceServerRefresh, false); } catch { return false; } } private static void AddDevModeArgs(TomlArray args, bool devForceRefresh) { if (devForceRefresh) { args.Add(new TomlString { Value = "--no-cache" }); args.Add(new TomlString { Value = "--refresh" }); } }
197-207: Same duplication as noted above.This block mirrors the logic in
BuildCodexServerBlock(lines 42-52). The suggested helper extraction would consolidate both occurrences.MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
544-548: Consider extracting the conditional to improve readability.The inline conditional in the interpolated string works correctly but is dense. Consider extracting to a local variable for clarity, similar to how it's done in Register().
🔎 Optional refactor for consistency
string gitUrl = AssetPathUtility.GetMcpServerGitUrl(); bool devForceRefresh = false; try { devForceRefresh = EditorPrefs.GetBool(EditorPrefKeys.DevModeForceServerRefresh, false); } catch { } +string devFlags = devForceRefresh ? "--no-cache --refresh " : string.Empty; return "# Register the MCP server with Claude Code:\n" + - $"claude mcp add --transport stdio UnityMCP -- \"{uvxPath}\" {(devForceRefresh ? "--no-cache --refresh " : string.Empty)}--from \"{gitUrl}\" mcp-for-unity\n\n" + + $"claude mcp add --transport stdio UnityMCP -- \"{uvxPath}\" {devFlags}--from \"{gitUrl}\" mcp-for-unity\n\n" +Server/src/services/tools/manage_scene.py (1)
44-57: Consider extracting coercion helpers to reduce duplication.The
_coerce_intfunction is duplicated betweenmanage_scene.pyandmanage_gameobject.py. Consider extracting to a shared utility module to improve maintainability.# In a new file like services/tools/coercion.py: def coerce_int(value, default=None): """Coerce a value to int, handling string/bool edge cases.""" if value is None: return default try: if isinstance(value, bool): return default if isinstance(value, int): return int(value) s = str(value).strip() if s.lower() in ("", "none", "null"): return default return int(float(s)) except Exception: return defaultMCPForUnity/Editor/Services/ServerManagementService.cs (1)
357-358: Minor: Windows process name check could miss edge cases.The check
combined.Contains("uv.exe")aftercombined.Contains("uvx")is partially redundant since "uvx" would match both "uvx" and "uvx.exe". However, explicitly checking for "uv.exe" is correct since "uv" alone might match unrelated strings.🔎 Consider tightening the Windows check
- return combined.Contains("python") || combined.Contains("uvx") || combined.Contains("uv.exe") || combined.Contains("uvx.exe"); + // Check for common MCP server process names + return combined.Contains("python.exe") || combined.Contains("python ") || + combined.Contains("uvx.exe") || combined.Contains("uv.exe");
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
FixscriptableobjecPlan.mdMCPForUnity/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/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.metadocs/README-DEV.md
💤 Files with no reviewable changes (1)
- FixscriptableobjecPlan.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T05:19:59.258Z
Learnt from: dsarno
Repo: dsarno/unity-mcp PR: 85
File: tests/test_telemetry_queue_worker.py:33-37
Timestamp: 2025-10-21T05:19:59.258Z
Learning: In the unity-mcp repository, pyproject.toml is located in MCPForUnity/UnityMcpServer~/src/, not at the repository root. When testing code that imports telemetry.py, the working directory should be changed to SRC (MCPForUnity/UnityMcpServer~/src/) so that telemetry.py's get_package_version() can find pyproject.toml via relative path.
Applied to files:
Server/src/services/tools/debug_request_context.py
🧬 Code graph analysis (17)
Server/tests/integration/test_manage_gameobject_param_coercion.py (3)
Server/tests/integration/test_manage_scene_paging_params.py (1)
fake_send(11-13)Server/src/services/tools/manage_gameobject.py (1)
manage_gameobject(16-268)Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (1)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
HandleCommand(42-253)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (1)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-49)
MCPForUnity/Editor/Tools/ManageScene.cs (1)
MCPForUnity/Editor/Tools/ManageGameObject.cs (6)
List(1632-1791)List(2639-2676)List(2699-2717)List(2722-2761)List(2769-2804)GameObject(1604-1627)
MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs (1)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-49)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (2)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-49)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
uvxPath(169-176)
Server/tests/integration/test_debug_request_context_diagnostics.py (1)
Server/src/services/tools/debug_request_context.py (1)
debug_request_context(16-80)
Server/src/services/tools/manage_scene.py (1)
Server/src/services/tools/manage_gameobject.py (1)
_coerce_int(101-114)
Server/src/services/tools/manage_gameobject.py (4)
Server/src/services/tools/manage_asset.py (1)
_coerce_int(93-106)Server/src/services/tools/manage_scene.py (1)
_coerce_int(44-57)Server/src/services/tools/read_console.py (1)
_coerce_int(61-74)Server/src/services/tools/utils.py (1)
coerce_bool(11-24)
Server/tests/integration/test_manage_scene_paging_params.py (2)
Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)Server/src/services/tools/manage_scene.py (1)
manage_scene(13-122)
Server/src/services/tools/manage_asset.py (1)
Server/tests/integration/test_helpers.py (1)
info(36-37)
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)
Server/tests/integration/test_tool_signatures_paging.py (2)
Server/src/services/tools/manage_scene.py (1)
manage_scene(13-122)Server/src/services/tools/manage_gameobject.py (1)
manage_gameobject(16-268)
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (3)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-49)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
uvxPath(169-176)MCPForUnity/Editor/External/Tommy.cs (1)
TomlString(168-193)
MCPForUnity/Editor/Services/ServerManagementService.cs (3)
MCPForUnity/Editor/Helpers/McpLog.cs (4)
McpLog(7-52)Info(37-41)Warn(43-46)Error(48-51)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-272)TryRun(161-227)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-49)
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (1)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-49)
🪛 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)
278-278: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
279-279: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
281-281: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
Server/src/services/tools/manage_scene.py
72-72: Do not catch blind exception: Exception
(BLE001)
Server/src/services/tools/manage_gameobject.py
113-113: 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/manage_asset.py
119-119: 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: Greptile Review
🔇 Additional comments (35)
Server/src/services/tools/manage_asset.py (2)
18-46: LGTM: Excellent documentation improvements for LLM guidance.The updated parameter descriptions and tool-level tips effectively steer models toward payload-safe patterns (paging, avoiding previews, using search_pattern). This aligns well with the PR's objective of preventing large-response crashes.
111-133: Smart normalization logic for LLM-generated inputs.This server-side guardrail effectively prevents common mistakes where models place Unity AssetDatabase queries (like
t:MonoScript) into thepathparameter instead ofsearch_pattern, which would trigger inefficient project-wide searches. The mapping ofasset_typetofilter_typeis also a sensible fallback. Logging at lines 126 and 132 provides good visibility for debugging.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSceneHierarchyPagingTests.cs.meta (1)
1-11: LGTM!Standard Unity-generated meta file for the new paging test suite.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (1)
35-36: LGTM!The test isolation for
DevModeForceServerRefreshfollows the established pattern used forGitUrlOverrideandUseHttpTransport. Correctly tracks both presence and value, ensures deterministicuvxargs ordering during tests, and properly restores the original state in teardown.Also applies to: 46-47, 58-60, 98-106
docs/README-DEV.md (2)
76-76: Good addition documenting the dev-mode behavior.Clear explanation of the
--no-cache --refreshflags and when to use them.
172-206: Well-documented paging defaults.The "Payload sizing & paging defaults" section provides actionable guidance for callers. The parameter descriptions with default values and clamping ranges align with the implementation in
ManageScene.cs.Server/src/main.py (1)
11-35: Reasonable startup safety guard.This workaround handles edge cases where tool signature evaluation via
eval()lacks commontypingnames in globals. The broad exception catch withpassis intentional here to avoid breaking startup—acceptable given the# pragma: no covermarker and defensive nature.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (1)
72-106: Good test coverage for default paging behavior.The test properly validates the paging contract:
includeProperties=falseby default, exactlypageSizeitems returned, and lightweight metadata without thepropertiesfield.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSceneHierarchyPagingTests.cs (1)
29-103: Comprehensive paging test covering key scenarios.Good coverage of:
- Root pagination with truncation detection
- Cursor-based continuation to fetch subsequent pages
- Children pagination via
parentinstance IDThe test validates the paging envelope structure (
scope,truncated,next_cursor,items.Count).MCPForUnity/Editor/Tools/ManageScene.cs (3)
29-37: Well-structured paging parameters.The
SceneCommandextension with paging fields and the corresponding parsing inToSceneCommandfollow the established patterns. Good support for both camelCase and snake_case parameter names.Also applies to: 74-84
502-564: Solid paging implementation with sensible defaults.The defaults are well-chosen for bounding response sizes:
pageSize=50(clamped 1-500)maxNodes=1000(clamped 1-5000)The response envelope with
scope,truncated,next_cursor, andtotalprovides clear paging metadata for callers.
624-660: Good summary structure for paged responses.The
BuildGameObjectSummarymethod returns useful metadata includingchildrenCursorandchildrenPageSizeDefaultto guide callers on fetching children. SettingchildrenTruncated=truewhenchildCount > 0is a reasonable signal that children exist but aren't inlined.Server/pyproject.toml (1)
33-33: [Your rewritten review comment text here]
[Exactly ONE classification tag]Server/src/services/tools/manage_gameobject.py (1)
221-224: LGTM!The paging parameters are correctly assembled and will be filtered by the
{k: v for k, v in params.items() if v is not None}comprehension at line 234, ensuring only valid values are sent to Unity.Server/tests/integration/test_manage_gameobject_param_coercion.py (1)
43-45: LGTM!The mock function correctly captures the params for assertion. The unused
cmdandkwargsarguments (flagged by Ruff ARG001) are expected for a mock that must match the signature ofasync_send_command_with_retry.MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.uxml (1)
45-50: LGTM!The Dev Mode UI block is well-structured with clear help text explaining the trade-off (slower startup but avoids stale cached builds). The toggle name
dev-mode-force-refresh-togglealigns with the C# binding inMcpSettingsSection.cs.Server/src/services/tools/debug_request_context.py (1)
57-61: LGTM!The server metadata block provides useful diagnostics for debugging. Using
list(sys.argv)creates a copy to prevent mutation, andget_package_version()has proper fallback handling. Since this is a debug endpoint, exposing runtime context is appropriate.MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
23-23: LGTM!The new
DevModeForceServerRefreshconstant follows the established naming convention and is logically placed with related override settings.Server/tests/integration/test_manage_scene_paging_params.py (1)
37-42: Same flexible assertion pattern as gameobject test.The assertions accept both coerced and original values. For
manage_scene.py, the coercion usesdefault=None, so failed coercions would exclude the param entirely. The assertions verify the params are present with expected values, which is sufficient for integration testing.MCPForUnity/Editor/Windows/Components/Settings/McpSettingsSection.cs (2)
157-161: LGTM!The callback correctly persists the toggle value to
EditorPrefsand invokesOnHttpServerCommandUpdateRequestedto trigger config regeneration. This follows the same pattern used for other settings likegitUrlOverride.
110-110: LGTM!The toggle is correctly initialized from
EditorPrefswith a safe default offalse, ensuring dev mode is opt-in.Server/tests/integration/test_tool_signatures_paging.py (1)
1-32: LGTM - Signature verification tests for paging parameters.These tests ensure the paging/safety parameters are present in the function signatures, providing regression protection if someone accidentally removes them. The tests are appropriately minimal for their purpose.
Server/tests/integration/test_debug_request_context_diagnostics.py (1)
4-26: LGTM - Test verifies server diagnostics are exposed.The test correctly validates that debug_request_context returns server metadata (version, cwd, argv). Using monkeypatch to stabilize the version value is a good practice for deterministic assertions.
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
426-430: LGTM - Dev mode refresh flags correctly integrated.The implementation properly reads the DevModeForceServerRefresh preference with a safe try/catch fallback and injects the
--no-cache --refreshflags into the stdio transport command when enabled.MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (1)
84-87: LGTM - EditorPrefs access with safe fallback.The try/catch pattern consistently guards against potential EditorPrefs exceptions with a sensible default of
false.MCPForUnity/Editor/Tools/ManageGameObject.cs (4)
183-212: LGTM - Robust coercion helpers for paging parameters.The
CoerceIntandCoerceBoollocal functions handle various input formats gracefully (null, boolean, empty string, "none", "null", numeric strings). This defensive parsing accommodates different client behaviors.
1250-1268: LGTM - Well-bounded paging with safety limits.The clamping of
pageSize(1-200),cursor(≥0), andmaxComponents(1-500) provides good guardrails. Building a stable component list upfront ensures consistent pagination across requests.
1273-1315: LGTM - Payload budget prevents response explosion.The 250KB payload budget (
maxPayloadChars) with early termination is a sensible safeguard whenincludeProperties=true. The per-component error handling ensures one serialization failure doesn't break the entire response.
1344-1357: LGTM - Compact metadata helper.
BuildComponentMetadatareturns essential information (typeName, instanceID, enabled) without deep serialization, supporting the metadata-first pattern whenincludeProperties=false.Server/src/services/tools/manage_scene.py (2)
59-74: LGTM - Comprehensive boolean coercion.The
_coerce_boolfunction handles a wide range of truthy/falsy string representations ("true", "1", "yes", "y", "on" and their opposites), which accommodates various client input formats.Regarding the static analysis hint about catching
Exceptionon line 72: this is acceptable here as the function is purely defensive and must never propagate errors—returningdefaultis the correct fallback behavior.
96-111: LGTM - Conditional param inclusion.Paging parameters are only added to the Unity command payload when they have non-None values after coercion, avoiding unnecessary parameter bloat.
MCPForUnity/Editor/Services/ServerManagementService.cs (4)
233-273: LGTM - Robust multi-PID termination with safety guards.The implementation correctly:
- Discovers all processes listening on the port
- Refuses to terminate the Unity Editor process
- Refuses to terminate processes that don't look like MCP servers
- Reports per-PID outcomes
This prevents accidental termination of unrelated services.
345-394: Well-designed process identification heuristics.
LooksLikeMcpServerProcessuses reasonable heuristics:
- Explicitly excludes Unity/Unity Hub processes
- Requires positive indicators (uv/uvx/python) AND MCP-related markers (mcp-for-unity, --transport http)
The empty catch on line 391 is acceptable for this defensive helper that must never throw.
396-427: LGTM - Graceful-then-forceful termination strategy.Trying graceful termination first (
kill -15/taskkill) before escalating to forceful (kill -9/taskkill /F) is the correct approach, allowing the process to clean up resources if possible.
458-464: LGTM - Dev mode flags integrated into server command.Consistent with other files, the DevModeForceServerRefresh preference is read with a safe try/catch and the
--no-cache --refreshflags are injected when enabled.
Greptile SummaryImplemented payload-safe paging defaults and LLM-mistake hardening to prevent Unity freeze/crash risks from large JSON responses. Key Changes:
Validation: Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant LLM as LLM/Agent
participant Server as Python Server
participant Unity as Unity C# Handler
Note over LLM,Unity: Paged Hierarchy Request (new flow)
LLM->>Server: manage_scene(action="get_hierarchy", page_size=50)
Server->>Server: Coerce page_size string/int (default: None → 50)
Server->>Unity: {"action": "get_hierarchy", "pageSize": 50}
Unity->>Unity: Clamp pageSize (50, max: 500)
Unity->>Unity: Fetch roots, slice [0:50]
Unity-->>Server: {items: [...], cursor: 0, next_cursor: "50", truncated: true, total: 100}
Server-->>LLM: Success + paged data
Note over LLM,Unity: Follow-up page request
LLM->>Server: manage_scene(action="get_hierarchy", page_size=50, cursor=50)
Server->>Unity: {"action": "get_hierarchy", "pageSize": 50, "cursor": 50}
Unity->>Unity: Slice [50:100]
Unity-->>Server: {items: [...], cursor: 50, next_cursor: null, truncated: false, total: 100}
Server-->>LLM: Success + remaining data
Note over LLM,Unity: Asset Search (hardened flow)
LLM->>Server: manage_asset(action="search", path="t:MonoScript")
Server->>Server: Detect "t:" prefix in path (LLM mistake)
Server->>Server: Normalize: search_pattern="t:MonoScript", path="Assets"
Server->>Unity: {"action": "search", "path": "Assets", "searchPattern": "t:MonoScript"}
Unity->>Unity: Bounded search in Assets folder
Unity-->>Server: {results: [...]}
Server-->>LLM: Success + safe results
Note over LLM,Unity: Component Properties (metadata-first)
LLM->>Server: manage_gameobject(action="get_components", target="Player", page_size=10)
Server->>Server: include_properties defaults to None (false)
Server->>Unity: {"action": "get_components", "target": "Player", "pageSize": 10}
Unity->>Unity: Return metadata only (no properties)
Unity-->>Server: {components: [{name, type}, ...], truncated: false}
Server-->>LLM: Lightweight component list
LLM->>Server: manage_gameobject(action="get_components", target="Player", page_size=3, include_properties=true)
Server->>Unity: {"action": "get_components", "includeProperties": true, "pageSize": 3}
Unity->>Unity: Return first 3 components with full properties
Unity-->>Server: {components: [{name, type, properties: {...}}, ...], next_cursor: "3"}
Server-->>LLM: Detailed properties (bounded)
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Server/src/services/tools/run_tests.py (1)
59-60: Consider removing the redundant wrapper.The
_coerce_intfunction now simply delegates tocoerce_intwithout adding any logic. You can callcoerce_intdirectly at line 74 and remove this wrapper.🔎 Proposed simplification
- # Coerce timeout defensively (string/float -> int) - def _coerce_int(value, default=None): - return coerce_int(value, default=default) - # Coerce string or list to list of strings def _coerce_string_list(value) -> list[str] | None:Then at line 74:
- ts = _coerce_int(timeout_seconds) + ts = coerce_int(timeout_seconds)MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
426-430: Consider extracting the dev-mode flag reading pattern.This pattern is duplicated at lines 544-546. Consider extracting a helper method similar to
CodexConfigHelper.GetDevModeForceRefresh()to reduce duplication and maintain consistency.🔎 Proposed helper extraction
Add a private helper method in
ClaudeCliMcpConfigurator:private static bool GetDevModeForceRefresh() { try { return EditorPrefs.GetBool(EditorPrefKeys.DevModeForceServerRefresh, false); } catch { return false; } }Then replace both occurrences:
-bool devForceRefresh = false; -try { devForceRefresh = EditorPrefs.GetBool(EditorPrefKeys.DevModeForceServerRefresh, false); } catch { } +bool devForceRefresh = GetDevModeForceRefresh();Server/src/services/tools/utils.py (1)
70-71: Redundantint()cast on already-int value.When
valueis already anint, callingint(value)is a no-op. You can returnvaluedirectly.🔎 Proposed fix
if isinstance(value, int): - return int(value) + return valueServer/src/services/tools/read_console.py (1)
42-53: Consider usingcoerce_boolfrom utils in a follow-up.There's a
coerce_boolfunction inutils.pythat could replace this local_coerce_bool. This is outside the scope of this PR but would complete the centralization effort.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.csMCPForUnity/Editor/Helpers/CodexConfigHelper.csMCPForUnity/Editor/Helpers/ConfigJsonBuilder.csServer/src/main.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_manage_gameobject_param_coercion.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSceneHierarchyPagingTests.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSceneHierarchyPagingTests.cs
- Server/src/services/tools/manage_gameobject.py
🧰 Additional context used
🧬 Code graph analysis (5)
Server/tests/integration/test_manage_gameobject_param_coercion.py (2)
Server/tests/integration/test_manage_scene_paging_params.py (1)
fake_send(11-13)Server/src/services/tools/manage_gameobject.py (1)
manage_gameobject(16-254)
Server/src/services/tools/manage_scene.py (3)
Server/src/services/tools/utils.py (1)
coerce_int(63-77)Server/src/services/tools/run_tests.py (1)
_coerce_int(59-60)Server/src/services/tools/read_console.py (1)
_coerce_bool(42-53)
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (3)
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)
Server/src/services/tools/run_tests.py (1)
Server/src/services/tools/utils.py (1)
coerce_int(63-77)
Server/src/services/tools/read_console.py (1)
Server/src/services/tools/utils.py (1)
coerce_int(63-77)
🪛 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/src/services/tools/manage_scene.py
61-61: Do not catch blind exception: Exception
(BLE001)
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: nl-suite
🔇 Additional comments (18)
Server/src/main.py (2)
11-35: LGTM - Startup safety guard for typing names.The workaround is well-commented and appropriately defensive. The bare
except Exception: passis intentional here as a startup safety guard that should never break server initialization.
271-283: Well-documented paging guidance.The payload sizing instructions provide clear, actionable recommendations for LLM callers to avoid large responses. The recommended page sizes align with the PR objectives.
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
544-549: LGTM - Dev flags correctly integrated into manual snippet.The generated CLI command now includes
--no-cache --refreshflags when dev mode is enabled, maintaining consistency with the Register path.Server/src/services/tools/utils.py (1)
63-77: Well-designed coercion utility.The function correctly handles edge cases: booleans (which are int subclasses in Python) return default, empty/null strings return default, and float-like strings are truncated to int. The bare
except Exceptionis acceptable for a defensive utility.Server/src/services/tools/read_console.py (1)
62-62: LGTM - Centralized integer coercion.The change correctly uses the shared
coerce_intutility for thecountparameter.MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (2)
20-38: Well-structured dev-mode helpers.The
GetDevModeForceRefresh()andAddDevModeArgs()methods cleanly encapsulate the dev-mode logic with appropriate null checks and error handling. This pattern is correctly reused in bothBuildCodexServerBlockandCreateUnityMcpTable.
62-67: LGTM - Dev flags correctly integrated.The dev-mode flags are prepended before
--fromand package name, maintaining correct uvx argument ordering.Server/tests/integration/test_manage_gameobject_param_coercion.py (2)
35-36: LGTM - Updated assertions reflect proper boolean coercion.The assertions now correctly expect
TrueandFalseboolean values rather than string representations, aligning with the centralizedcoerce_boolbehavior.
39-72: Good test coverage for paging parameter coercion.The new test validates that string inputs are correctly coerced to integers (
"25"→25,"50"→50,"100"→100) and booleans ("true"→True). The unusedcmdandkwargsparameters infake_sendare intentional to match the mocked function signature.MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (2)
84-87: LGTM - Dev mode flag integration.The pattern is consistent with other configurators. The try/catch guards against potential EditorPrefs access issues.
155-177: Well-documented argument construction.The comment clearly explains the purpose of
--no-cacheand--refreshflags. The argument ordering (dev flags →--from→ package →--transport) is consistent across all uvx builders.Server/src/services/tools/manage_asset.py (4)
12-12: LGTM: Centralized coercion utility imported.The
coerce_intimport from utils is used at lines 92-93 for consistent numeric parameter handling across the codebase.
18-22: LGTM: Documentation enhancements improve LLM guidance.The updated parameter descriptions and tool-level payload safety tips effectively guide callers toward safer patterns (paging, avoiding previews, using search_pattern with Assets scope).
Also applies to: 27-27, 35-39, 44-46
92-93: LGTM: Consistent numeric coercion.Using the centralized
coerce_intutility aligns with the project's pattern for handling loosely-typed numeric inputs.
95-117: Normalization logic is sound, but defensive error handling appears unnecessary.The normalization effectively prevents token-heavy project-wide searches by moving AssetDatabase queries (e.g.,
"t:MonoScript") frompathintosearch_pattern. However, the try-except block at lines 101-105 catchesAttributeErrorandTypeErroras defensive handling for non-stringpathvalues, despitepathbeing strictly annotated asstr(line 27). All test calls in the codebase pass proper string values ("Assets","Assets/Test.mat"), and there is no evidence of None or non-string values being passed at runtime.The defensive
(path or "")pattern and error handling are overly cautious but do not harm correctness. Ifpathtype safety is guaranteed by upstream validation, the try-except can be simplified or removed.Server/src/services/tools/manage_scene.py (3)
6-6: LGTM: Centralized coercion utility imported.The
coerce_intimport is appropriately used for numeric parameter coercion.
31-38: LGTM: Paging and safety parameters enhance payload control.The new parameters provide the necessary controls for bounded responses as described in the PR objectives.
87-100: LGTM: Conditional parameter inclusion is consistent.The pattern of conditionally including coerced paging/safety parameters aligns with the existing approach for other optional parameters.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Server/src/services/tools/manage_scene.py (2)
6-6: Excellent refactoring: centralized utilities imported.The import now includes
coerce_boolfrom the centralized utilities module, addressing the code duplication concern from the previous review.
44-51: Refactoring complete: all previous concerns resolved.This refactoring successfully addresses all three issues flagged in the previous review:
- ✓ Removed the redundant
_coerce_intwrapper—now callscoerce_intdirectly- ✓ Eliminated duplicate
_coerce_boollogic—now uses centralizedcoerce_boolfromutils.py- ✓ Removed local broad exception handling—delegated to centralized utilities
The code is now DRY, maintainable, and consistent with the project's coercion patterns.
Based on the resolution of past review comments.
🧹 Nitpick comments (2)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
553-557: Extract duplicated helper to a shared utility class.This method is duplicated verbatim in
CodexConfigHelper.cs(lines 19-29). Consider extracting it to a shared helper class (e.g.,EditorPrefsHelperorDevModeHelper) to follow the DRY principle and reduce maintenance burden.💡 Suggested refactor
Create a shared utility class:
// MCPForUnity/Editor/Helpers/EditorPrefsHelper.cs namespace MCPForUnity.Editor.Helpers { internal static class EditorPrefsHelper { internal static bool GetDevModeForceRefresh() { try { return EditorPrefs.GetBool(EditorPrefKeys.DevModeForceServerRefresh, false); } catch { return false; } } } }Then replace both occurrences with:
- private static bool GetDevModeForceRefresh() - { - try { return EditorPrefs.GetBool(EditorPrefKeys.DevModeForceServerRefresh, false); } - catch { return false; } - } + // Use shared helper: EditorPrefsHelper.GetDevModeForceRefresh()Update call sites at lines 426 and 542:
- bool devForceRefresh = GetDevModeForceRefresh(); + bool devForceRefresh = EditorPrefsHelper.GetDevModeForceRefresh();Server/src/services/tools/utils.py (1)
63-77: Consider narrowing the exception handler for better error visibility.The implementation is correct and handles the defensive coercion use case well. However, the broad
except Exceptioncould mask unexpected errors during development or debugging.🔎 Suggested refactor to catch specific exceptions
def coerce_int(value: Any, default: int | None = None) -> int | None: """Attempt to coerce a loosely-typed value to an integer.""" if value is None: return default try: if isinstance(value, bool): return default if isinstance(value, int): return value s = str(value).strip() if s.lower() in ("", "none", "null"): return default return int(float(s)) - except Exception: + except (ValueError, TypeError, AttributeError): return defaultThis narrows the catch to the specific exceptions that
int(),float(),str(), and.strip()would raise, improving debuggability without sacrificing robustness.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.csServer/src/services/tools/manage_scene.pyServer/src/services/tools/read_console.pyServer/src/services/tools/run_tests.pyServer/src/services/tools/utils.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Server/src/services/tools/run_tests.py
- Server/src/services/tools/read_console.py
🧰 Additional context used
🧬 Code graph analysis (2)
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)
🪛 Ruff (0.14.10)
Server/src/services/tools/utils.py
76-76: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (7)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (2)
426-428: LGTM - Dev mode flags correctly injected into stdio command.The conditional devFlags construction and injection logic is correct. The trailing space in the devFlags string ensures proper spacing in the final command.
542-546: Consistent manual snippet generation.The devFlags logic mirrors the Register() method implementation, ensuring consistency between automatic registration and manual instructions.
Server/src/services/tools/manage_scene.py (2)
31-38: LGTM: paging/safety parameters well-documented.The new parameters are properly typed and annotated with clear descriptions. The mix of
int | strannotations aligns with the defensive coercion strategy used throughout the codebase.
64-79: LGTM: conditional parameter passing is correct.The conditional logic correctly adds paging/safety parameters only when they are non-None after coercion. Note that
parent(line 66-67) is intentionally passed through without coercion since it can be either a string (name/path) or an integer (instanceID).TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptableObjectTests.cs (3)
6-6: LGTM!The
System.Threadingnamespace is correctly added to supportThread.Sleepin the new helper method.
26-26: Good addition for test stability.The two
WaitForUnityReady()calls each serve distinct purposes:
- Line 26: Ensures Unity is ready before test setup begins
- Line 50: Waits for
AssetDatabase.Refresh()to complete, as it can trigger asynchronous asset processingThis addresses the intermittent setup failures mentioned in the code comments and aligns with the test stability goals in the PR objectives.
Also applies to: 50-50
311-325: Well-implemented helper for EditMode test synchronization.The implementation is sound:
- Uses
Thread.Sleeppolling, which is a pragmatic choice for EditMode tests where Unity's compilation and asset processing run on background threads- 30-second timeout provides a reasonable safety net for compilation scenarios
- 50ms poll interval balances responsiveness with CPU usage
- Clear comments explain the purpose and context
While
Thread.Sleepis generally not ideal in Unity Editor code, it's acceptable here becauseEditorApplication.isCompilingandisUpdatingare updated by Unity's background systems, allowing the main thread to safely poll these flags.
* Fix test teardown to avoid dropping MCP bridge 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). * Avoid leaking PlatformService in CodexConfigHelperTests 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. * Fix SO MCP tooling: validate folder roots, normalize paths, expand tests; remove vestigial SO tools * Remove UnityMCPTests stress artifacts and ignore Assets/Temp * Ignore UnityMCPTests Assets/Temp only * Clarify array_resize fallback logic comments * Refactor: simplify action set and reuse slash sanitization * Enhance: preserve GUID on overwrite & support Vector/Color types in ScriptableObject tools * Fix: ensure asset name matches filename to suppress Unity warnings * Fix: resolve Unity warnings by ensuring asset name match and removing redundant import * Refactor: Validate assetName, strict object parsing for vectors, remove broken SO logic from ManageAsset * Hardening: reject Windows drive paths; clarify supported asset types * Delete FixscriptableobjecPlan.md * docs: Add manage_scriptable_object tool description to README
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
51-51: Well-placed addition to the tools list.The new
manage_scriptable_objectentry is clearly described, follows the established documentation format, and is positioned appropriately within the tools section.For future consideration: If this tool has complex parameters or usage patterns (e.g., specific SerializedObject property path syntax, constraints on asset types), consider expanding the main Tools section with a brief usage note or linking to detailed docs (e.g., a section in CUSTOM_TOOLS.md or a new docs/MANAGE_SCRIPTABLE_OBJECT.md). This could help LLMs and users understand constraints and best practices upfront—especially given the PR's focus on "payload-sizing and paging guidance" and "normalizing unsafe calls."
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.mdTestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs
🧰 Additional context used
🧬 Code graph analysis (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (2)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-49)MCPForUnity/Editor/Services/MCPServiceLocator.cs (2)
MCPServiceLocator(11-91)Register(40-62)
🔇 Additional comments (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (2)
35-36: LGTM! Clean DevModeForceServerRefresh handling.The DevModeForceServerRefresh flag handling follows the established pattern for EditorPrefs state management in this fixture. The SetUp comment clearly explains why forcing the flag to false is necessary (to ensure deterministic uvx argument ordering regardless of developer settings), and the OneTimeTearDown properly restores the original state.
Also applies to: 46-47, 58-60, 102-110
76-79: Good defensive fallback for null platform service.The else clause provides a sensible default by registering a new
PlatformService()when the original was null. This ensures subsequent tests/fixtures always have a valid platform service available, which is a reasonable design choice given the fixture's goal of minimizing disruption to active MCP sessions.
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=falseSummary by CodeRabbit
New Features
Documentation
Utilities
✏️ Tip: You can customize this high-level summary in your review settings.