Standardize how we define MCP tools #292
Conversation
WalkthroughReplaces the editor's manual ExecuteCommand switch with registry-based dispatch; adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Bridge as Unity Editor Bridge
participant Registry as CommandRegistry
participant Handler as Command Handler
Client->>Bridge: ExecuteCommand(command)
Bridge->>Registry: GetHandler(command.type)
alt handler found
Registry-->>Bridge: handlerFunc
Bridge->>Handler: Invoke(paramsObject)
Handler-->>Bridge: JSON response
Bridge-->>Client: Response JSON
else missing / error
Registry-->>Bridge: None / throws
Bridge-->>Client: Error JSON
end
sequenceDiagram
autonumber
actor MCPClient
participant Tool as MCP Tool
participant Conn as UnityConnection
participant Unity as Unity Editor
MCPClient->>Tool: call(tool, annotated params)
Tool->>Tool: normalize & filter params
Tool->>Conn: send_command_with_retry(type, params)
Conn->>Unity: framed request
Unity-->>Conn: framed response
Conn-->>Tool: parsed result
Tool-->>MCPClient: dict result (success/error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
| $"Unknown or unsupported command type: {command.type}" | ||
| ), | ||
| }; | ||
| object result = CommandRegistry.GetHandler(command.type)(paramsObject); |
There was a problem hiding this comment.
This was removed in an earlier PR, it probably re-appeared by a bad merge conflict resolution
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
UnityMcpBridge/UnityMcpServer~/src/port_discovery.py (1)
65-69: Fix probe payload to match Unity bridge protocolThe Unity bridge speaks newline-delimited JSON (
UnityConnectionalways writesjson.dumps(...)+b"\n"). Sending the bare bytesb"ping"never produces a response—the C# side keeps waiting for a newline—so_try_probe_unity_mcpalways returnsFalse. That forces discovery to fall back to the “first seen” port, which can be stale or belong to another project, defeating the resilience we’re trying to add. Please emit the same framed JSON ping we use elsewhere.- s.sendall(b"ping") + s.sendall(b'{"command":"ping"}\n')UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py (1)
47-55: Restore Unity connection handling or drop it
get_unity_connection()is still invoked here, but the import was removed. As soon as the tool runs, this will raise aNameErrorand the call never reachessend_command_with_retry. Either re-import the symbol or, since the retry helper already manages the connection, just delete this block. The latter keeps the code lean and avoids the unused variable too.- # Get the connection instance - bridge = get_unity_connection()
🧹 Nitpick comments (11)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
17-39: Avoid per-call inspect.signature; precompute action param once.This decorates hot paths; binding the signature on every call is unnecessary. Cache the action parameter position at decoration time and read from args/kwargs.
Apply:
def telemetry_tool(tool_name: str): """Decorator to add telemetry tracking to MCP tools""" def decorator(func: Callable) -> Callable: + # Precompute 'action' param position to avoid per-call introspection + try: + _sig = inspect.signature(func) + _param_names = tuple(_sig.parameters.keys()) + _action_idx = _param_names.index("action") if "action" in _param_names else None + except Exception: + _action_idx = None + + def _extract_sub_action(args, kwargs): + if "action" in kwargs: + return kwargs.get("action") + if _action_idx is not None and len(args) > _action_idx: + return args[_action_idx] + return None @@ - def _sync_wrapper(*args, **kwargs) -> Any: + def _sync_wrapper(*args, **kwargs) -> Any: start_time = time.time() success = False error = None # Extract sub-action (e.g., 'get_hierarchy') from bound args when available - sub_action = None - try: - sig = inspect.signature(func) - bound = sig.bind_partial(*args, **kwargs) - bound.apply_defaults() - sub_action = bound.arguments.get("action") - except Exception: - sub_action = None + sub_action = _extract_sub_action(args, kwargs) @@ - async def _async_wrapper(*args, **kwargs) -> Any: + async def _async_wrapper(*args, **kwargs) -> Any: start_time = time.time() success = False error = None # Extract sub-action (e.g., 'get_hierarchy') from bound args when available - sub_action = None - try: - sig = inspect.signature(func) - bound = sig.bind_partial(*args, **kwargs) - bound.apply_defaults() - sub_action = bound.arguments.get("action") - except Exception: - sub_action = None + sub_action = _extract_sub_action(args, kwargs)Also applies to: 63-83
46-51: Minor: tighten milestone scene check or document prefix contract.Using startswith("manage_scene") is fine; if tool names change (e.g., "scene"), milestones could drift. Optionally constrain to exact name or a documented prefix convention.
Also applies to: 89-94
UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
1071-1086: Remove now-unused special-case manage_scene handler.
HandleManageSceneappears unused after registry dispatch. Consider removing to reduce dead code and stale behavior drift.UnityMcpBridge/UnityMcpServer~/src/tools/manage_prefabs.py (3)
16-22: Restore explicit enum description for action.“Operation” is less helpful to clients than listing allowed values. Include them in the annotation text to align with FastMCP’s parameter metadata guidance.
Apply:
- ], "Operation"], + ], "Operation: one of open_stage | close_stage | save_open_stage | create_from_gameobject"],
24-34: Optional: restrict mode via Literal for clearer contracts.If only InIsolation is supported, encode it in the type to help LLMs and clients.
Example:
- mode: Annotated[str | None, - "Optional prefab stage mode (only 'InIsolation' is currently supported)"] = None, + mode: Annotated[Literal["InIsolation"] | None, + "Prefab stage mode (currently only 'InIsolation')"] = None,
35-61: Consider lightweight precondition checks for action-specific args.E.g., require target and prefab_path for create_from_gameobject; return a friendly error before bridging. Keeps Unity logs cleaner without adopting full Pydantic.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (4)
359-369: Add Annotated parameter descriptions to align with FastMCP metadata.This tool lacks Annotated[...] metadata for params; adding them improves discoverability and standardization.
Apply minimal updates:
-from typing import Dict, Any, List, Tuple, Optional +from typing import Dict, Any, List, Tuple, Optional, Annotated @@ - def script_apply_edits( - ctx: Context, - name: str, - path: str, - edits: List[Dict[str, Any]], - options: Optional[Dict[str, Any]] = None, - script_type: str = "MonoBehaviour", - namespace: str = "", - ) -> Dict[str, Any]: + def script_apply_edits( + ctx: Context, + name: Annotated[str, "Script name (with or without .cs)"], + path: Annotated[str, "Directory under Assets or full Assets/.../File.cs"], + edits: Annotated[List[Dict[str, Any]], "Structured/text edits to apply"], + options: Annotated[Optional[Dict[str, Any]], "Options: validate, refresh, preview, confirm"] = None, + script_type: Annotated[str, "Unity script type (e.g., MonoBehaviour)"] = "MonoBehaviour", + namespace: Annotated[str, "Namespace (optional)"] = "", + ) -> Dict[str, Any]:
669-677: Broad excepts are acceptable here; consider narrowing where feasible.Error mapping is user-facing. If practical, catch regex compilation errors explicitly (ValueError/re.error) to improve diagnostics and keep BLE001 quiet.
Also applies to: 822-835
871-888: Guard oversized diffs for preview to keep responses small.You already truncate; consider emitting total-changes metadata to help clients decide to fetch more.
883-890: Consistency nit: return shapes.Some error returns use {"success": False, "message": ...}, others include "code". Consider adding code consistently for easier client handling.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (1)
101-103: Remove the unusedconnectionassignmentCalling
get_unity_connection()purely for its side effect is fine, but assigning it toconnectionand never using it trips lint and adds noise. Dropping the assignment (or reusing the object) keeps the intent clear.- connection = get_unity_connection() + get_unity_connection()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
UnityMcpBridge/Editor/MCPForUnityBridge.cs(1 hunks)UnityMcpBridge/Editor/Tools/CommandRegistry.cs(2 hunks)UnityMcpBridge/Editor/Tools/ManageScript.cs(9 hunks)UnityMcpBridge/UnityMcpServer~/src/Dockerfile(1 hunks)UnityMcpBridge/UnityMcpServer~/src/__init__.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/config.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/port_discovery.py(6 hunks)UnityMcpBridge/UnityMcpServer~/src/pyproject.toml(1 hunks)UnityMcpBridge/UnityMcpServer~/src/reload_sentinel.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/server.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/telemetry.py(17 hunks)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py(5 hunks)UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py(4 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py(4 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_prefabs.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(21 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py(26 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py(5 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py(9 hunks)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py(18 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
PR: CoplayDev/unity-mcp#260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
Applied to files:
UnityMcpBridge/UnityMcpServer~/src/DockerfileUnityMcpBridge/UnityMcpServer~/src/pyproject.tomlUnityMcpBridge/UnityMcpServer~/src/__init__.py
🧬 Code graph analysis (15)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (4)
record_tool_usage(403-429)record_milestone(249-271)record_milestone(398-400)MilestoneType(49-57)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
UnityMcpBridge/Editor/Tools/CommandRegistry.cs (1)
CommandRegistry(12-49)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/Editor/Tools/CommandRegistry.cs (1)
UnityMcpBridge/Editor/Tools/Prefabs/ManagePrefabs.cs (1)
ManagePrefabs(12-273)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_prefabs.py (1)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py (3)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/telemetry.py (2)
is_telemetry_enabled(458-460)record_tool_usage(403-429)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (3)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
get_unity_connection(378-399)async_send_command_with_retry(439-452)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)tests/test_manage_script_uri.py (1)
tool(46-50)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
telemetry_tool(17-107)decorator(19-106)
UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py (1)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (7)
get_telemetry(383-388)record_telemetry(391-395)record_milestone(249-271)record_milestone(398-400)RecordType(37-46)MilestoneType(49-57)is_telemetry_enabled(458-460)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/Editor/Tools/ManageScript.cs (1)
UnityMcpBridge/Editor/Helpers/Response.cs (3)
Response(10-61)Error(41-60)Success(18-33)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
UnityMcpBridge/UnityMcpServer~/src/port_discovery.py (1)
PortDiscovery(24-160)
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (3)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (1)
_coerce_int(62-75)
🪛 Ruff (0.13.1)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py
103-103: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/port_discovery.py
127-127: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py
67-67: Avoid specifying long messages outside the exception class
(TRY003)
88-89: Avoid specifying long messages outside the exception class
(TRY003)
681-681: Do not catch blind exception: Exception
(BLE001)
833-833: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py
15-15: Unused function argument: ctx
(ARG001)
58-58: Use explicit conversion flag
Replace with conversion flag
(RUF010)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py
16-16: Unused function argument: ctx
(ARG001)
17-18: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
19-19: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
22-22: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
23-23: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
24-24: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
57-57: Use explicit conversion flag
Replace with conversion flag
(RUF010)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py
19-19: Unused function argument: ctx
(ARG001)
101-101: Local variable connection is assigned to but never used
Remove assignment to unused variable connection
(F841)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py
16-16: Unused function argument: ctx
(ARG001)
60-60: Do not catch blind exception: Exception
(BLE001)
62-62: Use explicit conversion flag
Replace with conversion flag
(RUF010)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
16-16: Unused function argument: tool_name
(ARG001)
92-92: Unused function argument: ctx
(ARG001)
169-170: try-except-pass detected, consider logging the exception
(S110)
169-169: Do not catch blind exception: Exception
(BLE001)
234-235: try-except-pass detected, consider logging the exception
(S110)
234-234: Do not catch blind exception: Exception
(BLE001)
370-370: Unused function argument: ctx
(ARG001)
407-407: Unused function argument: ctx
(ARG001)
421-421: Unused function argument: ctx
(ARG001)
454-454: Unused function argument: ctx
(ARG001)
514-515: try-except-pass detected, consider logging the exception
(S110)
514-514: Do not catch blind exception: Exception
(BLE001)
577-577: Unused function argument: ctx
(ARG001)
602-602: Unused function argument: ctx
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py
25-25: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
57-57: Do not catch blind exception: Exception
(BLE001)
57-57: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
117-117: Consider moving this statement to an else block
(TRY300)
119-119: Do not catch blind exception: Exception
(BLE001)
119-119: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py
124-124: Do not catch blind exception: Exception
(BLE001)
128-129: try-except-pass detected, consider logging the exception
(S110)
128-128: Do not catch blind exception: Exception
(BLE001)
179-180: Abstract raise to an inner function
(TRY301)
179-180: Avoid specifying long messages outside the exception class
(TRY003)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py
15-15: Unused function argument: ctx
(ARG001)
17-17: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
19-19: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
21-21: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
23-23: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
25-25: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
27-27: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
29-29: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
31-31: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
33-33: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
35-35: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
39-39: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
43-43: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
45-45: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
46-46: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
48-48: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
55-55: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
63-63: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
66-66: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
126-126: Use explicit conversion flag
Replace with conversion flag
(RUF010)
UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py
18-18: Unused function argument: ctx
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py
94-95: Avoid specifying long messages outside the exception class
(TRY003)
128-129: Avoid specifying long messages outside the exception class
(TRY003)
154-155: Abstract raise to an inner function
(TRY301)
154-155: Avoid specifying long messages outside the exception class
(TRY003)
174-175: Abstract raise to an inner function
(TRY301)
174-175: Create your own exception
(TRY002)
174-175: Avoid specifying long messages outside the exception class
(TRY003)
208-208: Consider moving this statement to an else block
(TRY300)
212-212: Do not catch blind exception: Exception
(BLE001)
214-214: Use explicit conversion flag
Replace with conversion flag
(RUF010)
320-320: Abstract raise to an inner function
(TRY301)
320-320: Create your own exception
(TRY002)
396-397: Avoid specifying long messages outside the exception class
(TRY003)
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py
142-142: Unused function argument: ctx
(ARG001)
194-194: Unused function argument: ctx
(ARG001)
353-353: Unused function argument: ctx
(ARG001)
🔇 Additional comments (2)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
1042-1046: Registry dispatch verified
All expected handlers, includingmanage_prefabs, are registered in CommandRegistry.UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (1)
18-33: register_all_tools invocation and metadata usage verified: function is only called once and all @mcp.tool definitions include descriptions and Annotated parameters.
| if isinstance(response, dict) and response.get("success"): | ||
| # If the response contains base64 encoded content, decode it | ||
| if response.get("data", {}).get("contentsEncoded"): | ||
| decoded_contents = base64.b64decode(response["data"]["encodedContents"]).decode('utf-8') | ||
| decoded_contents = base64.b64decode( | ||
| response["data"]["encodedContents"]).decode('utf-8') | ||
| response["data"]["contents"] = decoded_contents | ||
| del response["data"]["encodedContents"] | ||
| del response["data"]["contentsEncoded"] | ||
|
|
||
| return {"success": True, "message": response.get("message", "Operation successful."), "data": response.get("data")} |
There was a problem hiding this comment.
Normalize data before probing for encoded shader contents
When Unity returns a success payload whose data field is either null or a non-dict (several tools already send strings here), calling .get(...) on it raises an AttributeError. That propagates into the catch block, so the caller only sees "Python error managing shader: '...' " instead of the original Unity response. Guard the branch to run only when data is actually a mapping, and fall back gracefully otherwise.
- if response.get("data", {}).get("contentsEncoded"):
- decoded_contents = base64.b64decode(
- response["data"]["encodedContents"]).decode('utf-8')
- response["data"]["contents"] = decoded_contents
- del response["data"]["encodedContents"]
- del response["data"]["contentsEncoded"]
+ data = response.get("data")
+ if isinstance(data, dict) and data.get("contentsEncoded"):
+ encoded = data.get("encodedContents")
+ if encoded:
+ decoded_contents = base64.b64decode(encoded).decode('utf-8')
+ data = {**data, "contents": decoded_contents}
+ data.pop("encodedContents", None)
+ data.pop("contentsEncoded", None)
+ response["data"] = data📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if isinstance(response, dict) and response.get("success"): | |
| # If the response contains base64 encoded content, decode it | |
| if response.get("data", {}).get("contentsEncoded"): | |
| decoded_contents = base64.b64decode(response["data"]["encodedContents"]).decode('utf-8') | |
| decoded_contents = base64.b64decode( | |
| response["data"]["encodedContents"]).decode('utf-8') | |
| response["data"]["contents"] = decoded_contents | |
| del response["data"]["encodedContents"] | |
| del response["data"]["contentsEncoded"] | |
| return {"success": True, "message": response.get("message", "Operation successful."), "data": response.get("data")} | |
| if isinstance(response, dict) and response.get("success"): | |
| # If the response contains base64 encoded content, decode it | |
| data = response.get("data") | |
| if isinstance(data, dict) and data.get("contentsEncoded"): | |
| encoded = data.get("encodedContents") | |
| if encoded: | |
| decoded_contents = base64.b64decode(encoded).decode('utf-8') | |
| data = {**data, "contents": decoded_contents} | |
| data.pop("encodedContents", None) | |
| data.pop("contentsEncoded", None) | |
| response["data"] = data | |
| return {"success": True, "message": response.get("message", "Operation successful."), "data": response.get("data")} |
There was a problem hiding this comment.
Ignoring, I only made a formatting change, the logic can be revisited later
|
Will test, respond to feedback and release tomorrow |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (1)
174-181: Let callers list non-script assets.Hard-coding the
.cssuffix here defeats thepatternparameter—pattern="*.shader"(or any non-.csglob) will always return an empty list even though the annotation and description promise configurability. That regresses existing capability to browse other asset types. Please drop the forced suffix check and let the glob drive the filter, defaulting to*.csonly when the caller doesn’t supply one.- # Enforce .cs extension regardless of provided pattern - if p.suffix.lower() != ".cs": - continue - if pattern and not fnmatch.fnmatch(p.name, pattern): + glob = pattern or "*.cs" + if not fnmatch.fnmatch(p.name, glob): continueUnityMcpBridge/UnityMcpServer~/src/tools/read_console.py (1)
47-55: Restore theget_unity_connectionimport or drop the unused call.
get_unity_connectionis no longer imported, yet Line 48 still calls it. This will raiseNameErrorat runtime before we ever build params. Either re-importget_unity_connectionor, since the result is unused andsend_command_with_retryalready acquires the connection internally, just remove the call entirely. I recommend the latter to avoid redundant work.- # Get the connection instance - bridge = get_unity_connection() - # Set defaults if values are None action = action if action is not None else 'get' types = types if types is not None else ['error', 'warning', 'log']
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py(4 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_prefabs.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(21 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py(27 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py(5 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
get_unity_connection(378-399)async_send_command_with_retry(439-452)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py (3)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/telemetry.py (2)
is_telemetry_enabled(458-460)record_tool_usage(403-429)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
telemetry_tool(16-19)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (3)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
telemetry_tool(17-107)decorator(19-106)tests/test_script_tools.py (2)
decorator(44-46)tool(43-47)
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (3)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (1)
_coerce_int(62-75)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_prefabs.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
🪛 Ruff (0.13.1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py
19-19: Unused function argument: ctx
(ARG001)
101-101: Local variable connection is assigned to but never used
Remove assignment to unused variable connection
(F841)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py
16-16: Unused function argument: ctx
(ARG001)
17-18: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
19-19: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
22-22: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
23-23: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
24-24: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
57-57: Use explicit conversion flag
Replace with conversion flag
(RUF010)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py
15-15: Unused function argument: ctx
(ARG001)
17-17: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
19-19: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
21-21: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
23-23: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
25-25: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
27-27: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
29-29: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
31-31: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
33-33: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
35-35: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
39-39: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
43-43: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
45-45: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
46-46: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
48-48: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
55-55: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
63-63: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
66-66: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
126-126: Use explicit conversion flag
Replace with conversion flag
(RUF010)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py
15-15: Unused function argument: ctx
(ARG001)
58-58: Use explicit conversion flag
Replace with conversion flag
(RUF010)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py
19-19: Unused function argument: ctx
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py
16-16: Unused function argument: ctx
(ARG001)
60-60: Do not catch blind exception: Exception
(BLE001)
62-62: Use explicit conversion flag
Replace with conversion flag
(RUF010)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py
67-67: Avoid specifying long messages outside the exception class
(TRY003)
88-89: Avoid specifying long messages outside the exception class
(TRY003)
681-681: Do not catch blind exception: Exception
(BLE001)
833-833: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py
18-18: Unused function argument: ctx
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
16-16: Unused function argument: tool_name
(ARG001)
92-92: Unused function argument: ctx
(ARG001)
169-170: try-except-pass detected, consider logging the exception
(S110)
169-169: Do not catch blind exception: Exception
(BLE001)
234-235: try-except-pass detected, consider logging the exception
(S110)
234-234: Do not catch blind exception: Exception
(BLE001)
370-370: Unused function argument: ctx
(ARG001)
407-407: Unused function argument: ctx
(ARG001)
421-421: Unused function argument: ctx
(ARG001)
454-454: Unused function argument: ctx
(ARG001)
514-515: try-except-pass detected, consider logging the exception
(S110)
514-514: Do not catch blind exception: Exception
(BLE001)
577-577: Unused function argument: ctx
(ARG001)
602-602: Unused function argument: ctx
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py
142-142: Unused function argument: ctx
(ARG001)
194-194: Unused function argument: ctx
(ARG001)
353-353: Unused function argument: ctx
(ARG001)
🔇 Additional comments (5)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
455-463: Re-include'update'in the advertised action Literal.With
'update'removed from theLiteral, FastMCP will now reject legacymanage_scriptupdate calls before they can reach the in-function migration path, breaking the compatibility you’re preserving. Please add'update'back (mark it deprecated in the description if desired) so existing clients keep working.- action: Annotated[Literal['create', 'read', 'delete'], "Operations"], + action: Annotated[ + Literal['create', 'read', 'update', 'delete'], + "Operations (legacy 'update' is deprecated—prefer apply_text_edits)" + ],UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
606-611: Fix encoded payload fallback.Line 610 still requires both
contentsEncodedandencodedContents, so payloads that only send the encoded string keep returning “No contents returned from Unity read.” Please decode whenever either encoded field is present (preferencodedContents), guarding for non-string values.- contents = data.get("contents") - if contents is None and data.get("contentsEncoded") and data.get("encodedContents"): - contents = base64.b64decode( - data["encodedContents"]).decode("utf-8") + contents = data.get("contents") + if contents is None: + encoded_val = data.get("encodedContents") + if isinstance(encoded_val, str): + contents = base64.b64decode(encoded_val).decode("utf-8") + elif isinstance(data.get("contentsEncoded"), str): + contents = base64.b64decode( + data["contentsEncoded"]).decode("utf-8")
761-763: Removeanchor_insertfromstructured_kinds.Line 762 keeps
anchor_insertin the structured set, so the text-conversion branch that advertises anchor support never runs. Dropping it here lets pure anchor inserts reach the text-edits path as intended.- structured_kinds = {"replace_class", "delete_class", - "replace_method", "delete_method", "insert_method", "anchor_insert"} + structured_kinds = {"replace_class", "delete_class", + "replace_method", "delete_method", "insert_method"}UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (1)
17-20: Revertname/pathto optional parameters.Requiring
nameandpathmakes previously valid calls likemanage_scene(action="get_hierarchy")raiseTypeErrorbefore our logic runs, breaking existing clients and telemetry (see earlier review). These values still behave as optional inside the function, so restore their defaults.- name: Annotated[str, "Scene name (no extension) for create/load/save"], - path: Annotated[str, "Asset path for scene operations (default: 'Assets/')"], + name: Annotated[str | None, "Scene name (no extension) for create/load/save"] = None, + path: Annotated[str | None, "Asset path for scene operations (default: 'Assets/')"] = None,UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py (1)
48-55: Guardresponse["data"]before decoding shader payloads.If Unity replies with
{success: True, data: None}(or any non-dict),response.get("data", {}).get("contentsEncoded")raisesAttributeError, which dumps us into the catch block and hides the real Unity response—exactly the failure called out in the earlier review. Please normalizedatato a dict before touching it so we don't crash on valid responses.- if response.get("data", {}).get("contentsEncoded"): - decoded_contents = base64.b64decode( - response["data"]["encodedContents"]).decode('utf-8') - response["data"]["contents"] = decoded_contents - del response["data"]["encodedContents"] - del response["data"]["contentsEncoded"] + data = response.get("data") + if isinstance(data, dict) and data.get("contentsEncoded"): + encoded = data.get("encodedContents") + if encoded: + decoded_contents = base64.b64decode(encoded).decode('utf-8') + data["contents"] = decoded_contents + data.pop("encodedContents", None) + data.pop("contentsEncoded", None) + response["data"] = data
There was a problem hiding this comment.
Whitespace changes
| { "read_console", ReadConsole.HandleCommand }, | ||
| { "manage_menu_item", ManageMenuItem.HandleCommand }, | ||
| { "manage_shader", ManageShader.HandleCommand}, | ||
| { "manage_prefabs", ManagePrefabs.HandleCommand}, |
There was a problem hiding this comment.
Issue with earlier merge, this should have been in
There was a problem hiding this comment.
Whitespace changes
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (1)
54-57: Preserve full Unity payload on successWhen Unity responds with
success: True, we now drop every other key exceptmessageanddata. Existing actions likeget_hierarchyandget_build_settingsreturn structured payloads (e.g.,hierarchy,scenes) outside ofdata, and they lose that information after this change. Please keep returning the original dict (or merge in the friendly message without discarding other fields) so downstream clients continue to receive the full payload.Apply this diff to retain the original response while still exposing the friendly message:
- if isinstance(response, dict) and response.get("success"): - return {"success": True, "message": response.get("message", "Scene operation successful."), "data": response.get("data")} - return response if isinstance(response, dict) else {"success": False, "message": str(response)} + if isinstance(response, dict): + if response.get("success"): + response.setdefault("message", "Scene operation successful.") + return response + return {"success": False, "message": str(response)}
🧹 Nitpick comments (4)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py (4)
17-26: Optional parameter typing is correct; consider polishing the action’s description.
- Using Annotated[T] | None for optionals resolves implicit Optional (RUF013) and ensures correct FastMCP schema. Good.
- Minor: “Operations” is vague; a clearer description (e.g., “Operation to perform”) helps clients render better tooltips.
Suggested change:
- "get_active_tool", "get_selection", "get_prefab_stage", "set_active_tool", "add_tag", "remove_tag", "get_tags", "add_layer", "remove_layer", "get_layers"], "Operations"], + "get_active_tool", "get_selection", "get_prefab_stage", "set_active_tool", "add_tag", "remove_tag", "get_tags", "add_layer", "remove_layer", "get_layers"], "Operation to perform"],
36-47: Pre‑validate companion params for clearer client errors.Returning early on missing companion args gives immediate, actionable feedback instead of a round‑trip error from Unity.
if action == "telemetry_ping": record_tool_usage("diagnostic_ping", True, 1.0, None) return {"success": True, "message": "telemetry ping queued"} - # Prepare parameters, removing None values + # Validate required companion parameters for certain actions + if action == "set_active_tool" and not tool_name: + return {"success": False, "message": "tool_name is required when action is 'set_active_tool'."} + if action in {"add_tag", "remove_tag"} and not tag_name: + return {"success": False, "message": "tag_name is required when action is 'add_tag' or 'remove_tag'."} + if action in {"add_layer", "remove_layer"} and not layer_name: + return {"success": False, "message": "layer_name is required when action is 'add_layer' or 'remove_layer'."} + # Prepare parameters, removing None values
16-16: Rename unused ctx param or suppress to satisfy Ruff ARG001.Change the param name to underscore‑prefixed:
- ctx: Context, + _ctx: Context,Alternatively, keep the name and add a no‑op reference before use (less clean):
_ = ctx.Based on static analysis hints
59-59: Use f‑string conversion flag to satisfy RUF010.- return {"success": False, "message": f"Python error managing editor: {str(e)}"} + return {"success": False, "message": f"Python error managing editor: {e!s}"}Based on static analysis hints
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (4)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
get_unity_connection(378-399)async_send_command_with_retry(439-452)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)tests/test_manage_script_uri.py (1)
tool(46-50)tests/test_script_tools.py (1)
tool(43-47)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
🪛 Ruff (0.13.1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py
19-19: Unused function argument: ctx
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py
16-16: Unused function argument: ctx
(ARG001)
59-59: Use explicit conversion flag
Replace with conversion flag
(RUF010)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py
15-15: Unused function argument: ctx
(ARG001)
60-60: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py (1)
1-1: Adopts FastMCP conventions and centralized retry — looks good.
- Typed imports, named tool decorator with description, camelCase payload keys (toolName/layerName), and None‑filtering all align with FastMCP + Unity payload expectations.
Also applies to: 3-3, 7-7, 13-13, 40-42, 49-49
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py (1)
20-20: Prefer descriptive annotation over generic "Operations".While the typing change is correct, the description "Operations" is too generic. Consider using a more specific description that explains what these operations do.
Apply this diff for better clarity:
- action: Annotated[Literal["execute", "list", "exists"], "Operations"], + action: Annotated[Literal["execute", "list", "exists"], "Menu operations: execute a menu item, list available items, or check if an item exists"],UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py (2)
17-18: Consider more descriptive parameter descriptions.While the typing is correct, some parameter descriptions could be more specific about their purpose and usage context.
Apply this diff for better clarity:
- action: Annotated[Literal["telemetry_status", "telemetry_ping", "play", "pause", "stop", "get_state", "get_project_root", "get_windows", - "get_active_tool", "get_selection", "get_prefab_stage", "set_active_tool", "add_tag", "remove_tag", "get_tags", "add_layer", "remove_layer", "get_layers"], "Operations"], + action: Annotated[Literal["telemetry_status", "telemetry_ping", "play", "pause", "stop", "get_state", "get_project_root", "get_windows", + "get_active_tool", "get_selection", "get_prefab_stage", "set_active_tool", "add_tag", "remove_tag", "get_tags", "add_layer", "remove_layer", "get_layers"], "Editor operations: control playmode, manage tools/tags/layers, query editor state"],
60-60: Use explicit conversion for string formatting.The static analysis correctly identifies that
str()should be replaced with a more explicit conversion.Apply this diff based on static analysis hints:
- return {"success": False, "message": f"Python error managing editor: {str(e)}"} + return {"success": False, "message": f"Python error managing editor: {e!s}"}UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py (1)
63-63: Use explicit conversion for string formatting.Same static analysis issue as in manage_editor.py.
Apply this diff based on static analysis hints:
- return {"success": False, "message": f"Python error managing shader: {str(e)}"} + return {"success": False, "message": f"Python error managing shader: {e!s}"}UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (4)
21-22: Remove redundant Optional in type annotation.
build_indexis annotated asint | NoneinsideAnnotatedand then unioned with| Noneagain. Drop the inner| Noneto simplify the type.- build_index: Annotated[int | None, - "Build index for load/build settings actions"] | None = None, + build_index: Annotated[int, + "Build index for load/build settings actions"] | None = None,
19-20: Fix misleading default inpathdescription.The help text claims a default of
'Assets/', but the parameter defaults toNoneand no fallback is applied. Update the description (or implement the default).- path: Annotated[str, - "Asset path for scene operations (default: 'Assets/')"] | None = None, + path: Annotated[str, + "Asset path for scene operations (e.g., 'Assets/')"] | None = None,
45-50: Prefer explicit None checks when building params.Using truthiness skips empty strings unintentionally. If empty strings are meaningful, check
is not Noneinstead.- if name: + if name is not None: params["name"] = name - if path: + if path is not None: params["path"] = path
61-61: Ruff RUF010: use explicit conversion flag in f-string.Avoid
str(e)inside f-strings; prefer{e!s}.Based on static analysis hints.
- return {"success": False, "message": f"Python error managing scene: {str(e)}"} + return {"success": False, "message": f"Python error managing scene: {e!s}"}UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py (2)
26-30: Avoid shadowing the built-informat.Keep the public parameter name for MCP compatibility but use a local
output_formatinternally.- format: Annotated[Literal['plain', 'detailed', - 'json'], "Output format"] | None = None, + format: Annotated[Literal['plain', 'detailed', + 'json'], "Output format"] | None = None, include_stacktrace: Annotated[bool, "Include stack traces in output"] | None = None ) -> dict[str, Any]: ctx.info(f"Processing read_console: {action}") @@ - format = format if format is not None else 'detailed' + output_format = format if format is not None else 'detailed' include_stacktrace = include_stacktrace if include_stacktrace is not None else True @@ - "format": format.lower() if isinstance(format, str) else format, + "format": output_format.lower() if isinstance(output_format, str) else output_format,Also applies to: 38-39, 70-71
19-20: Surface default directly in the signature for better MCP metadata.You already default
actionto'get'at runtime. Expose that in the signature so FastMCP metadata shows the default.- action: Annotated[Literal['get', 'clear'], "Operations"], + action: Annotated[Literal['get', 'clear'], "Operations"] = 'get',Please confirm that FastMCP surfaces this default as expected in your client UIs.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (1)
39-43: Validate required inputs forcreate.When
action == "create",asset_typemust be provided. Fail fast with a clear message before calling Unity.# Ensure properties is a dict if None if properties is None: properties = {} + # Validate required inputs early + if isinstance(action, str) and action.lower() == "create" and not asset_type: + return {"success": False, "message": "asset_type is required when action is 'create'."}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py(4 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_prefabs.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(20 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py(28 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py(4 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- UnityMcpBridge/UnityMcpServer~/src/tools/manage_prefabs.py
🧰 Additional context used
🧬 Code graph analysis (9)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
telemetry_tool(16-19)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py (3)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)tests/test_find_in_file_minimal.py (1)
tool(18-22)
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (5)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (1)
_coerce_int(44-57)tests/test_manage_script_uri.py (1)
tool(46-50)tests/test_resources_api.py (1)
tool(29-33)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
telemetry_tool(17-107)decorator(19-106)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (3)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
telemetry_tool(16-19)line_col_from_index(136-144)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (3)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
async_send_command_with_retry(439-452)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)tests/test_resources_api.py (1)
tool(29-33)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
🪛 Ruff (0.13.1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py
60-60: Use explicit conversion flag
Replace with conversion flag
(RUF010)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py
61-61: Use explicit conversion flag
Replace with conversion flag
(RUF010)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py
17-17: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
19-19: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
21-21: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
23-23: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
25-25: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
27-27: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
29-29: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
31-31: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
33-33: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
35-35: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
39-39: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
43-43: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
45-45: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
46-46: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
48-48: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
55-55: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
63-63: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
66-66: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
127-127: Use explicit conversion flag
Replace with conversion flag
(RUF010)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
16-16: Unused function argument: tool_name
(ARG001)
169-170: try-except-pass detected, consider logging the exception
(S110)
169-169: Do not catch blind exception: Exception
(BLE001)
234-235: try-except-pass detected, consider logging the exception
(S110)
234-234: Do not catch blind exception: Exception
(BLE001)
518-519: try-except-pass detected, consider logging the exception
(S110)
518-518: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py
67-67: Avoid specifying long messages outside the exception class
(TRY003)
88-89: Avoid specifying long messages outside the exception class
(TRY003)
682-682: Do not catch blind exception: Exception
(BLE001)
834-834: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py
61-61: Do not catch blind exception: Exception
(BLE001)
63-63: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (13)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py (2)
16-16: LGTM! Tool standardization successfully implemented.The addition of the explicit tool name aligns with FastMCP's documented best practices for tool registration and improves MCP client compatibility.
28-28: LGTM! Consistent logging implementation.The logging statement follows the pattern established across other tools in this PR.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (4)
24-28: LGTM! Improved error handling with clear operational guidance.The changes enhance the error handling by enforcing required
opfields and providing explicit guidance on allowed operations. The RuntimeError with specific allowed operations helps users understand exactly what they can do.
51-54: LGTM! Better error handling for anchor matching failures.The change from silent failure to explicit RuntimeError when anchor insertion cannot be matched provides clearer feedback to users about why their operations failed.
310-358: LGTM! Comprehensive tool documentation following FastMCP standards.The detailed description with examples, canonical field documentation, and proper formatting provides excellent guidance for users. This follows FastMCP's documentation recommendations for tool parameter metadata.
369-369: LGTM! Consistent logging pattern.The addition of context logging follows the established pattern across all tools in this PR.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py (1)
13-18: LGTM! Tool standardization successfully implemented.The explicit tool name and improved type annotations align with FastMCP's documented best practices. The PR comment noting the correct form
Annotated[str, ""] | Nonefor optional parameters is properly applied.UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py (2)
13-22: LGTM! Tool standardization successfully implemented.The explicit tool name, comprehensive description, and proper type annotations with optional parameters align well with FastMCP's documented best practices.
51-56: Verify data structure before accessing encoded contents.The past review comment correctly identified a potential AttributeError when
datais not a dict. This could mask the original Unity response with a Python error message.Run the following script to check how the
datafield is used across the codebase:#!/bin/bash # Description: Analyze how response data is handled in other tool files # Search for similar data.get patterns that might have the same issue rg -n -A3 -B3 'response\.get\("data".*\.get\(' --type=py # Look for safe data handling patterns in other tools rg -n -A5 -B2 'isinstance.*dict.*and.*data\.get' --type=pyUnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (2)
3-3: LGTM! Proper type annotation added.The addition of the FastMCP import and type annotation for the
mcpparameter improves type safety and documentation.
20-35: LGTM! Comprehensive tool registration with proper logging.The expanded registration function properly imports and registers all tool modules, with clear logging to track the initialization process. This centralizes the tool registration while maintaining good observability.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (1)
62-76: LGTM on param marshalling and async retry usage.Clean mapping to C# keys, sensible None-stripping, and correct use of the async retry helper.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
458-525: Keep legacy'update'action discoverable.By narrowing
actiontoLiteral['create', 'read', 'delete'], FastMCP clients will now reject'update'calls before they reach the compatibility shim below, breaking the very fallback we’re preserving. Please restore'update'in the advertised enum (marking it deprecated in the metadata if desired) so existing scripts continue to function.- action: Annotated[Literal['create', 'read', 'delete'], "Operations"], + action: Annotated[Literal['create', 'read', 'delete', 'update'], "Operations (note: 'update' is legacy and routes to apply_text_edits)"],
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
681-682: Consider logging regex compilation errors for debugging.While catching generic exceptions for regex compilation is acceptable to provide user-friendly error messages, consider logging these for debugging purposes.
except Exception as ex: + import logging + logging.debug(f"Regex compilation failed: {ex}", exc_info=True) return _with_norm(_err("bad_regex", f"Invalid regex pattern: {ex}", normalized=normalized_for_echo, routing="mixed/text-first", extra={"hint": "Escape special chars or prefer structured delete for methods."}), normalized_for_echo, routing="mixed/text-first")Also applies to: 832-833
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(20 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py(31 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (3)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
telemetry_tool(17-107)decorator(19-106)tests/test_script_tools.py (2)
decorator(44-46)tool(43-47)
🪛 Ruff (0.13.1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py
68-68: Avoid specifying long messages outside the exception class
(TRY003)
89-90: Avoid specifying long messages outside the exception class
(TRY003)
681-681: Do not catch blind exception: Exception
(BLE001)
832-832: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
16-16: Unused function argument: tool_name
(ARG001)
169-170: try-except-pass detected, consider logging the exception
(S110)
169-169: Do not catch blind exception: Exception
(BLE001)
234-235: try-except-pass detected, consider logging the exception
(S110)
234-234: Do not catch blind exception: Exception
(BLE001)
518-519: try-except-pass detected, consider logging the exception
(S110)
518-518: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (9)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (4)
329-361: LGTM! Well-structured sentinel reload implementation.The optional sentinel reload feature is cleanly implemented with proper error handling, daemon threads to avoid blocking, and clear status file checking. The approach of reading the latest status file to check if Unity is already reloading before flipping the sentinel is thoughtful.
91-115: LGTM! Comprehensive normalization for LSP-style edits.The normalization logic for accepting various edit formats (LSP-style ranges, index ranges, and standard line/col) is thorough and will improve compatibility with different clients. The careful handling of 0-based vs 1-based coordinates is appropriate.
241-279: LGTM! Robust overlap detection for edit ranges.The implementation correctly identifies overlapping edit ranges using proper position comparison logic. This preflight validation will prevent corruption from conflicting edits.
456-458: Include 'update' in the action Literal to preserve backward compatibility.The function still handles the legacy
'update'action (lines 471-528) but doesn't advertise it in the type annotation. FastMCP clients will reject'update'invocations before they reach your migration logic.Based on learnings from previous reviews
- action: Annotated[Literal['create', 'read', 'delete'], "Operations"], + action: Annotated[Literal['create', 'read', 'delete', 'update'], "Operations"],UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (5)
125-133: LGTM! Smart heuristic for finding class-ending braces.The enhanced anchor matching logic with indentation-based scoring and context awareness is excellent. This will significantly improve the accuracy of finding the correct closing brace for class-level insertions, avoiding common pitfalls with method braces.
310-357: LGTM! Comprehensive tool documentation with clear examples.The tool description provides excellent guidance on best practices, canonical field names, and concrete examples. This will help users understand how to properly use the structured editing capabilities.
376-465: LGTM! Thorough field aliasing for backward compatibility.The normalization logic handles a wide variety of field aliases and formats, making the tool more resilient to different client implementations. The automatic upgrade from
anchor_insertwith method anchors toinsert_methodis particularly clever.
607-612: Logic for decoding encoded contents may skip valid payloads.The current condition requires both
contentsEncodedANDencodedContentsto be present. This could skip valid responses where onlyencodedContentsis provided.Based on learnings
- if contents is None and data.get("contentsEncoded") and data.get("encodedContents"): - contents = base64.b64decode( - data["encodedContents"]).decode("utf-8") + if contents is None: + encoded_val = data.get("encodedContents") + if isinstance(encoded_val, str): + contents = base64.b64decode(encoded_val).decode("utf-8")
759-761: Remove 'anchor_insert' from structured_kinds to enable text conversion path.The code comment indicates the text path should handle
anchor_insert, but including it instructured_kindsprevents that path from executing.Based on learnings
- structured_kinds = {"replace_class", "delete_class", - "replace_method", "delete_method", "insert_method", "anchor_insert"} + structured_kinds = {"replace_class", "delete_class", + "replace_method", "delete_method", "insert_method"}
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 (2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py (1)
104-111: Handle missingprefab_folderbefore building default prefab pathWhen
save_as_prefabis true and the caller omitsprefab_folder, the string interpolation here producesNone/<name>.prefab. That bogus path is still sent to Unity and will either fail or create an unexpected folder, defeating the “automatic default path” behavior. Guard the construction so we either have a real folder value or return an explicit error.- constructed_path = f"{prefab_folder}/{params['name']}.prefab" - # Ensure clean path separators (Unity prefers '/') - params["prefabPath"] = constructed_path.replace("\\", "/") + if not prefab_folder: + return {"success": False, "message": "Cannot create default prefab path: 'prefab_folder' parameter is missing."} + sanitized_folder = prefab_folder.replace("\\", "/").rstrip("/") + constructed_path = f"{sanitized_folder}/{params['name']}.prefab" + params["prefabPath"] = constructed_pathUnityMcpBridge/UnityMcpServer~/src/tools/manage_prefabs.py (1)
35-51: Await the asyncctx.infocall (convert tool to async).
Context.infois an async method; in a synchronous tool this call just creates an un-awaited coroutine, which raises aRuntimeWarningand never delivers the log to the client.(fastmcp.wiki) Please make the handlerasyncand await both the log call and the blocking bridge request (e.g., viaasyncio.to_thread) so the MCP runtime stays happy.+import asyncio from typing import Annotated, Any, Literal @@ - def manage_prefabs( + async def manage_prefabs( @@ - ctx.info(f"Processing manage_prefabs: {action}") + await ctx.info(f"Processing manage_prefabs: {action}") @@ - response = send_command_with_retry("manage_prefabs", params) + response = await asyncio.to_thread( + send_command_with_retry, + "manage_prefabs", + params, + )
🧹 Nitpick comments (10)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py (1)
128-128: Let Ruff format the exception messageRuff (RUF010) flags
str(e)inside an f-string. Switching to the conversion flag keeps the same output and satisfies the linter.- return {"success": False, "message": f"Python error managing GameObject: {str(e)}"} + return {"success": False, "message": f"Python error managing GameObject: {e!s}"}UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (1)
61-61: Use explicit string conversion flag in error message.Ruff raised RUF010 here; prefer the
!sformat specifier instead of callingstr()inside the f-string.- return {"success": False, "message": f"Python error managing scene: {str(e)}"} + return {"success": False, "message": f"Python error managing scene: {e!s}"}UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (8)
76-89: Condense tool description to align with “concise and precise” objective.The current doc is verbose. Shorten while keeping key guidance.
- @mcp.tool(name="apply_text_edits", description=( - """Apply small text edits to a C# script identified by URI. - IMPORTANT: This tool replaces EXACT character positions. Always verify content at target lines/columns BEFORE editing! - RECOMMENDED WORKFLOW: - 1. First call resources/read with start_line/line_count to verify exact content - 2. Count columns carefully (or use find_in_file to locate patterns) - 3. Apply your edit with precise coordinates - 4. Consider script_apply_edits with anchors for safer pattern-based replacements - Notes: - - For method/class operations, use script_apply_edits (safer, structured edits) - - For pattern-based replacements, consider anchor operations in script_apply_edits - - Lines, columns are 1-indexed - - Tabs count as 1 column""" - )) + @mcp.tool( + name="apply_text_edits", + description=( + "Apply explicit text edits to a C# script by URI (lines/cols are 1‑indexed). " + "Accepts explicit ranges or normalizes LSP/index ranges. " + "Prefer script_apply_edits for structured/anchor edits. " + "Use precondition_sha256 to guard concurrent changes." + ), + )
128-133: Harden encoded-content decoding and avoid double-decode pitfalls.Gate on presence of
encodedContentsand keepcontentsif available.- if not contents and data.get("contentsEncoded"): - try: - contents = base64.b64decode(data.get("encodedContents", "").encode( - "utf-8")).decode("utf-8", "replace") + if not contents and data.get("contentsEncoded"): + enc = data.get("encodedContents") + try: + if isinstance(enc, str) and enc: + contents = base64.b64decode(enc).decode("utf-8", "replace") except Exception: contents = contents or ""
155-173: Replace silent exception swallowing with low-noise debug logging.The normalization branches suppress errors that hinder troubleshooting.
- except Exception: - pass + except Exception as e: + ctx.info(f"apply_text_edits: failed zero-based check: {e}") ... - except Exception: - pass + except Exception as e: + ctx.info(f"apply_text_edits: normalization clamp failed: {e}")Note: keep to
infoor a gated debug to avoid log spam.Also applies to: 221-239
405-416: Minor wording fix: “delete” vs “edit” in parameter description.The
uridescription says “edit”; update to “delete”.- uri: Annotated[str, "URI of the script to delete under Assets/ directory, unity://path/Assets/... or file://... or Assets/..."] + uri: Annotated[str, "URI of the script to delete (unity://path/Assets/..., file://..., or Assets/...)"]
459-466: Unify Annotated metadata to a single concise description per param.Multiple strings in
Annotatedcan render oddly across clients; keep one description.- name: Annotated[str, "Script name (no .cs extension)", "Name of the script to create"], - path: Annotated[str, "Asset path (default: 'Assets/')", "Path under Assets/ to create the script at, e.g., 'Assets/Scripts/My.cs'"], - contents: Annotated[str, "Contents of the script to create", - "C# code for 'create'/'update'"] | None = None, - script_type: Annotated[str, "Script type (e.g., 'C#')", - "Type hint (e.g., 'MonoBehaviour')"] | None = None, + name: Annotated[str, "Script name (no .cs extension)"], + path: Annotated[str, "Asset path under Assets/, e.g., 'Assets/Scripts/My.cs'"], + contents: Annotated[str, "C# code (base64 when creating; deprecated for 'update')"] | None = None, + script_type: Annotated[str, "Script type (e.g., 'C#' or 'MonoBehaviour')"] | None = None,
575-601: Expose limits and ops: addapply_text_editssemantics to text ops and echo server cap.If the server’s cap differs or is configurable, fetch and echo it to avoid drift.
- max_edit_payload_bytes = 256 * 1024 + max_edit_payload_bytes = 256 * 1024 # keep in sync with server guards = {"using_guard": True} extras = {"get_sha": True}If the server exposes this via a command, consider retrieving it once and caching.
603-609: Minor wording fix: “edit” vs “hash/metadata” in parameter description.- uri: Annotated[str, "URI of the script to edit under Assets/ directory, unity://path/Assets/... or file://... or Assets/..."] + uri: Annotated[str, "URI of the script (unity://path/Assets/..., file://..., or Assets/...) to query for SHA/size"]
38-39: Test UNC paths & single-percent-decode in _split_uri
Coverage missing for UNC share URIs (e.g.unity://path/\\server\share\…) and double-percent cases; ensure you only call percent-decode once and add explicit tests in tests/ for UNC+Assets detection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py(4 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_prefabs.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(20 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_prefabs.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (3)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
telemetry_tool(17-107)decorator(19-106)tests/test_manage_script_uri.py (1)
tool(46-50)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
telemetry_tool(16-19)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
🪛 Ruff (0.13.1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
16-16: Unused function argument: tool_name
(ARG001)
169-170: try-except-pass detected, consider logging the exception
(S110)
169-169: Do not catch blind exception: Exception
(BLE001)
234-235: try-except-pass detected, consider logging the exception
(S110)
234-234: Do not catch blind exception: Exception
(BLE001)
517-518: try-except-pass detected, consider logging the exception
(S110)
517-517: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py
61-61: Use explicit conversion flag
Replace with conversion flag
(RUF010)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py
128-128: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (6)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (6)
16-19: Rename unused decorator arg to satisfy linter and signal intent.
tool_nameis unused in the fallback; rename to_tool_name.- def telemetry_tool(tool_name: str): + def telemetry_tool(_tool_name: str): def decorator(func): return func return decorator
367-390: Good: strict path validation under Assets and extension checks.
420-429: LGTM: Literal for level with server-side guard + summary counts.
503-517: Nice: legacy 'update' routed to whole‑file edit with precondition and payload guard.Please confirm the server’s payload cap; if it differs from 256 KiB, mirror the exact value in both this guard and
manage_script_capabilities.
611-619: Good: minimal result surface for get_sha.
458-466: Restore deprecated 'update' in the advertised enum to preserve compatibility.Clients will reject 'update' before it can be migrated; include it in
Literaland mark deprecated.- action: Annotated[Literal['create', 'read', 'delete'], "Operations"], + action: Annotated[ + Literal['create', 'read', 'update', 'delete'], + "Operations ('update' is deprecated and routed to apply_text_edits)" + ],
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (2)
17-22: Preserve empty-string values and tighten metadata wording.
- Using truthiness checks will drop valid empty strings; switch to explicit
is not Nonechecks.- Minor copy edits in parameter metadata improve clarity and FastMCP UI rendering.
Apply this diff:
- name: Annotated[str, - "Scene name. Not required get_active/get_build_settings"] | None = None, + name: Annotated[str, + "Scene name (no extension). Not required for 'get_active' or 'get_build_settings'."] | None = None, - path: Annotated[str, - "Asset path for scene operations (default: 'Assets/')"] | None = None, + path: Annotated[str, + "Asset path for scene operations (default handled by C# if omitted)"] | None = None, @@ - if name: + if name is not None: params["name"] = name - if path: + if path is not None: params["path"] = path if coerced_build_index is not None: params["buildIndex"] = coerced_build_indexAlso applies to: 45-51
17-22: Docs vs behavior:'Assets/'default is only in the description.The code doesn’t enforce a default path; it relies on the C# side. Either:
- keep it as-is but clarify the description (done in diff above), or
- set a Python-side default when
path is None.If you prefer a Python default:
path: Annotated[str, - "Asset path for scene operations (default: 'Assets/')"] | None = None, + "Asset path for scene operations (default: 'Assets/')"] | None = None, @@ - if path is not None: + if path is None: + params["path"] = "Assets/" + else: params["path"] = pathUnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py (2)
27-37: Validate required inputs by action before calling Unity.Avoid round-trips and clearer errors when
menu_pathis missing forexecute/exists.) -> dict[str, Any]: ctx.info(f"Processing manage_menu_item: {action}") - # Prepare parameters for the C# handler + # Validate action-specific requirements + if action in ("execute", "exists") and not menu_path: + return {"success": False, "message": "Parameter 'menu_path' is required for 'execute' and 'exists'."} + + # Prepare parameters for the C# handler params_dict: dict[str, Any] = {
25-26: Consider defaulting refresh to False.FastMCP will display this as optional already; giving it a default avoids sending
None.- refresh: Annotated[bool, - "Optional flag to force refresh of the menu cache when listing"] | None = None, + refresh: Annotated[bool, + "Optional flag to force refresh of the menu cache when listing"] = False,UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py (2)
31-37: Add targeted validation for action-dependent parameters.Preempt server-side errors and provide actionable messages to clients.
try: # Diagnostics: quick telemetry checks if action == "telemetry_status": return {"success": True, "telemetry_enabled": is_telemetry_enabled()} if action == "telemetry_ping": record_tool_usage("diagnostic_ping", True, 1.0, None) return {"success": True, "message": "telemetry ping queued"} + # Input validation for specific actions + if action == "set_active_tool" and not tool_name: + return {"success": False, "message": "Parameter 'tool_name' is required for 'set_active_tool'."} + if action in ("add_tag", "remove_tag") and not tag_name: + return {"success": False, "message": "Parameter 'tag_name' is required for 'add_tag'/'remove_tag'."} + if action in ("add_layer", "remove_layer") and not layer_name: + return {"success": False, "message": "Parameter 'layer_name' is required for 'add_layer'/'remove_layer'."}
17-19: Optional: align action case handling.Other tools lowercase
actionbefore sending; here you pass it through. It’s fine since Literals are lower-case, but normalizing can prevent future drift.- action: Annotated[Literal["telemetry_status", "telemetry_ping", "play", "pause", "stop", "get_state", "get_project_root", "get_windows", + action: Annotated[Literal["telemetry_status", "telemetry_ping", "play", "pause", "stop", "get_state", "get_project_root", "get_windows", "get_active_tool", "get_selection", "get_prefab_stage", "set_active_tool", "add_tag", "remove_tag", "get_tags", "add_layer", "remove_layer", "get_layers"], "Get and update the Unity Editor state."],And just before building
params:- params = { + normalized_action = action.lower() + params = { - "action": action, + "action": normalized_action,UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py (2)
33-37: Avoid shadowing built-informatwith a local variable.Keep the parameter name (for compatibility), but use a different local variable to improve clarity.
- format = format if format is not None else 'detailed' + output_format = format if format is not None else 'detailed' @@ - "format": format.lower() if isinstance(format, str) else format, + "format": output_format.lower() if isinstance(output_format, str) else output_format,Also applies to: 67-69
19-31: Type hint consistency: action isn’t optional but you handle None.Either keep the defensive code or reflect it in the type to help FastMCP generate accurate metadata.
Option A (keep as-is): remove the None fallback.
Option B (reflect optionality):
- action: Annotated[Literal['get', 'clear'], "Get or clear the Unity Editor console."], + action: Annotated[Literal['get', 'clear'], "Get or clear the Unity Editor console."] | None = None,UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (2)
38-42: Add action-specific input validation to reduce Unity round-trips.Validate required parameters for common actions and return clear messages.
ctx.info(f"Processing manage_asset: {action}") # Ensure properties is a dict if None if properties is None: properties = {} + # Action-specific validations + lowered = action.lower() + if lowered == "create" and not asset_type: + return {"success": False, "message": "Parameter 'asset_type' is required for 'create'."} + if lowered == "modify" and not properties: + return {"success": False, "message": "Parameter 'properties' is required for 'modify'."} + if lowered in ("duplicate", "move", "rename") and not destination: + return {"success": False, "message": "Parameter 'destination' is required for 'duplicate'/'move'/'rename'."} + @@ - params_dict = { - "action": action.lower(), + params_dict = { + "action": lowered, "path": path, "assetType": asset_type, "properties": properties, "destination": destination,Also applies to: 62-79
43-61: Duplicate_coerce_intacross tools — consider a shared helper.This helper appears in multiple modules (assets/scenes/console). Extracting to a small utility (e.g.,
utils/typing.py) reduces drift.I can open a follow-up PR to introduce
coerce_int(value, default=None)and update call sites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py(4 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_prefabs.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(20 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- UnityMcpBridge/UnityMcpServer~/src/tools/manage_prefabs.py
🧰 Additional context used
🧬 Code graph analysis (8)
UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (3)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
async_send_command_with_retry(439-452)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)tests/test_resources_api.py (1)
tool(29-33)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
telemetry_tool(17-107)decorator(19-106)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py (3)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/telemetry.py (2)
is_telemetry_enabled(458-460)record_tool_usage(403-429)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py (1)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
async_send_command_with_retry(439-452)
🪛 Ruff (0.13.1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py
61-61: Do not catch blind exception: Exception
(BLE001)
63-63: Use explicit conversion flag
Replace with conversion flag
(RUF010)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
16-16: Unused function argument: tool_name
(ARG001)
169-170: try-except-pass detected, consider logging the exception
(S110)
169-169: Do not catch blind exception: Exception
(BLE001)
234-235: try-except-pass detected, consider logging the exception
(S110)
234-234: Do not catch blind exception: Exception
(BLE001)
517-518: try-except-pass detected, consider logging the exception
(S110)
517-517: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py
60-60: Use explicit conversion flag
Replace with conversion flag
(RUF010)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py
61-61: Use explicit conversion flag
Replace with conversion flag
(RUF010)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py
128-128: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (15)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py (1)
49-56: Guard against non-dictdatabefore decoding encoded contentsUnity sometimes returns
dataasNoneor even a plain string on success. Calling.get(...)on that blows up with anAttributeError, which your broadexcept Exceptionthen wraps as"Python error managing shader: ...", masking the real Unity response. We already flagged this earlier; please normalizedatato a mapping before decoding.- if response.get("data", {}).get("contentsEncoded"): - decoded_contents = base64.b64decode( - response["data"]["encodedContents"]).decode('utf-8') - response["data"]["contents"] = decoded_contents - del response["data"]["encodedContents"] - del response["data"]["contentsEncoded"] + data = response.get("data") + if isinstance(data, dict) and data.get("contentsEncoded"): + encoded = data.get("encodedContents") + if encoded: + decoded_contents = base64.b64decode( + encoded).decode("utf-8") + data = {**data, "contents": decoded_contents} + data.pop("encodedContents", None) + data.pop("contentsEncoded", None) + response["data"] = dataUnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py (1)
16-16: Restore explicit action guidance for FastMCP UI.Reintroduce concrete values in the action description so clients see allowed options.
- @mcp.tool(name="manage_menu_item", description="Manage Unity menu items (execute/list/exists). If you're not sure what menu item to use, use the 'list' action to find it before using 'execute'.") + @mcp.tool(name="manage_menu_item", description="Manage Unity menu items. Actions: 'execute', 'list', 'exists'. Use 'list' to discover items before 'execute'.") @@ - action: Annotated[Literal["execute", "list", "exists"], "Read and execute Unity menu items."], + action: Annotated[ + Literal["execute", "list", "exists"], + "Operation to perform: 'execute', 'list', or 'exists'", + ],Also applies to: 20-26
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (1)
28-30: Nice metadata fix forgenerate_preview.Clearer description aligns with the tool’s behavior and FastMCP guidance.
UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py (1)
58-59: Approve read_console changes; verify emitted FastMCP schema
- Defensive int coercion, explicit
count=None, and optional stacktrace stripping are all solid.- Diff the generated FastMCP schema file (e.g.
schema.json) to confirm theread_consoletool entry reflects these parameter definitions. Apply the same check for lines 71–78 and 81–90.UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (7)
458-458: Restore 'update' action to maintain backward compatibility.The PR narrows the action Literal to
{'create', 'read', 'delete'}but the function implementation still supports legacy 'update' routing (lines 470-527). FastMCP uses Annotated with explicit parameter descriptions and Literal constraints are surfaced to clients, so removing 'update' from the advertised enum will cause MCP clients to reject 'update' invocations before they reach the migration logic.Based on past review comments
- action: Annotated[Literal['create', 'read', 'delete'], "Perform CRUD operations on C# scripts."], + action: Annotated[Literal['create', 'read', 'delete', 'update'], "Perform CRUD operations on C# scripts."],
76-89: LGTM: Proper FastMCP tool definition with comprehensive parameter metadata.The tool decorator now includes an explicit name and detailed description, and all parameters use Annotated with descriptive metadata. This aligns with FastMCP's recommended approach of using Annotated for parameter descriptions and providing metadata that helps MCP clients understand tool capabilities.
367-375: LGTM: Tool definition follows FastMCP standards.The create_script tool properly uses the explicit name decorator parameter and Annotated types for comprehensive parameter documentation, following FastMCP's parameter metadata guidelines.
405-410: LGTM: Consistent tool metadata pattern.The delete_script tool correctly implements the standardized approach with explicit tool name and Annotated parameter descriptions.
420-429: LGTM: Proper use of Literal for validation level constraints.The validate_script tool correctly uses
Literal['basic', 'standard']to constrain validation levels, which FastMCP will surface to clients for proper validation.
575-581: LGTM: Well-documented capabilities tool.The manage_script_capabilities tool provides comprehensive metadata about supported operations and includes proper Context parameter usage, following FastMCP's Context injection pattern.
603-608: LGTM: Clean utility tool implementation.The get_sha tool follows the established pattern with proper Context usage and parameter documentation.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py (4)
17-68: Fix type annotations to includeNonefor nullable parameters.All parameters with
= Nonedefaults must have their Annotated types include| Noneto comply with PEP 484. FastMCP supports standard Python type annotations including union types, and these nullable annotations are required for proper type checking.Based on past review comments
- target: Annotated[str, + target: Annotated[str | None, "GameObject identifier by name or path for modify/delete/component actions"] | None = None, - search_method: Annotated[str, + search_method: Annotated[str | None, "How to find objects ('by_name', 'by_id', 'by_path', etc.). Used with 'find' and some 'target' lookups."] | None = None, - name: Annotated[str, + name: Annotated[str | None, "GameObject name - used for both 'create' (initial name) and 'modify' (rename)"] | None = None,Apply this pattern to all remaining nullable parameters in the function signature.
12-16: LGTM: Tool standardization follows FastMCP guidelines.The tool decorator correctly specifies an explicit name and comprehensive description, and the action parameter uses Literal with descriptive Annotated metadata. This aligns with FastMCP's parameter metadata approach for better MCP client compatibility.
70-70: LGTM: Proper Context usage.The Context parameter is properly used for logging, following FastMCP's Context.info() logging pattern.
102-116: LGTM: Robust prefab path handling.The prefab path construction and validation logic properly handles edge cases and provides clear error messages. The cleanup of
prefabFolderfrom the final params prevents duplicate information from being sent to Unity.
|
|
||
| except Exception as e: | ||
| return {"success": False, "message": f"Python error managing editor: {str(e)}"} | ||
| return {"success": False, "message": f"Python error managing editor: {str(e)}"} |
There was a problem hiding this comment.
Ruff RUF010: use explicit conversion flag in f-string.
Replace str(e) with {e!s}.
Based on static analysis hints.
- return {"success": False, "message": f"Python error managing editor: {str(e)}"}
+ return {"success": False, "message": f"Python error managing editor: {e!s}"}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return {"success": False, "message": f"Python error managing editor: {str(e)}"} | |
| return {"success": False, "message": f"Python error managing editor: {e!s}"} |
🧰 Tools
🪛 Ruff (0.13.1)
60-60: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py at line 60, the
f-string uses str(e) which triggers Ruff RUF010; replace the explicit str() call
with the f-string conversion flag by using {e!s} so the exception is converted
to string via the f-string conversion mechanism (i.e., change f"...{str(e)}" to
f"...{e!s}").
There was a problem hiding this comment.
@msanatan Understood, no problem! The current code works perfectly fine as-is. The str(e) approach is clear and readable.
|
|
||
| except Exception as e: | ||
| return {"success": False, "message": f"Python error managing scene: {str(e)}"} | ||
| return {"success": False, "message": f"Python error managing scene: {str(e)}"} |
There was a problem hiding this comment.
Ruff RUF010: use explicit conversion flag in f-string.
Replace str(e) with {e!s} to satisfy the linter.
Based on static analysis hints.
- return {"success": False, "message": f"Python error managing scene: {str(e)}"}
+ return {"success": False, "message": f"Python error managing scene: {e!s}"}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return {"success": False, "message": f"Python error managing scene: {str(e)}"} | |
| return {"success": False, "message": f"Python error managing scene: {e!s}"} |
🧰 Tools
🪛 Ruff (0.13.1)
61-61: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py around line 61, the
f-string uses str(e) which triggers Ruff RUF010; replace the explicit call with
the f-string conversion flag by changing f"Python error managing scene:
{str(e)}" to f"Python error managing scene: {e!s}" (update any equivalent
occurrences in the file to use !s as well).
There was a problem hiding this comment.
@msanatan, understood! I'll leave that f-string formatting as-is for now.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(18 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
telemetry_tool(17-107)decorator(19-106)
🪛 Ruff (0.13.1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
16-16: Unused function argument: tool_name
(ARG001)
169-170: try-except-pass detected, consider logging the exception
(S110)
169-169: Do not catch blind exception: Exception
(BLE001)
234-235: try-except-pass detected, consider logging the exception
(S110)
234-234: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
458-466: Restore legacyupdateaction in the LiteralLine 458 still advertises only
{'create','read','delete'}even though the router keeps a compatibility path for'update'. FastMCP clients surface the Literal choices directly, so existing'update'calls (e.g., from Windsurf/VS Code) will now be rejected before they reach the migration logic. Please re-add'update'—marking it deprecated in the metadata if you like—so we don’t break legacy scripts.- action: Annotated[Literal['create', 'read', 'delete'], "Perform CRUD operations on C# scripts."], + action: Annotated[Literal['create', 'read', 'update', 'delete'], "Perform CRUD operations on C# scripts (note: 'update' is deprecated, prefer apply_text_edits/script_apply_edits)."],
| for k in ("startLine", "startCol", "endLine", "endCol"): | ||
| try: | ||
| if int(e2.get(k, 1)) < 1: | ||
| e2[k] = 1 | ||
| except Exception: | ||
| pass | ||
| warnings.append("zero_based_explicit_fields_normalized") | ||
| warnings.append( | ||
| "zero_based_explicit_fields_normalized") | ||
| normalized_edits.append(e2) |
There was a problem hiding this comment.
Fix zero-based normalization for explicit edits
Line 165 (and the mirrored branch around Line 231) clamps zero-based coordinates to 1, so any edit targeting line/column > 1 lands on the wrong position instead of being shifted by +1. Please translate every explicit coordinate by +1 when zero_based is detected to preserve the caller’s intent.
- for k in ("startLine", "startCol", "endLine", "endCol"):
- try:
- if int(e2.get(k, 1)) < 1:
- e2[k] = 1
- except Exception:
- pass
+ for k in ("startLine", "startCol", "endLine", "endCol"):
+ try:
+ e2[k] = int(e2.get(k, 0)) + 1
+ except Exception:
+ pass
@@
- for k in ("startLine", "startCol", "endLine", "endCol"):
- try:
- if int(e2.get(k, 1)) < 1:
- e2[k] = 1
- except Exception:
- pass
+ for k in ("startLine", "startCol", "endLine", "endCol"):
+ try:
+ e2[k] = int(e2.get(k, 0)) + 1
+ except Exception:
+ passAlso applies to: 231-238
🧰 Tools
🪛 Ruff (0.13.1)
169-170: try-except-pass detected, consider logging the exception
(S110)
169-169: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py around lines
165-173 (and the mirrored branch at 231-238), the code currently forces any
explicit coordinate less than 1 to 1 instead of translating zero-based
coordinates; update both blocks to detect when the edit is zero_based and
translate every explicit coordinate by +1 while still clamping to a minimum of
1. Concretely: for each key in ("startLine","startCol","endLine","endCol") parse
the value as an int, if the edit is zero_based add 1 to the parsed value, then
if the result is < 1 set it to 1, and finally write the adjusted integer back to
e2[k]; preserve the existing try/except structure and keep the same warnings and
appending to normalized_edits.
There was a problem hiding this comment.
Ignoring for now, I only changed the whitespace
There was a problem hiding this comment.
@msanatan, that makes perfect sense! Since this PR is focused on standardizing MCP tool definitions and you only made whitespace changes to this section, deferring the pre-existing logic issue is the right approach. The zero-based coordinate normalization bug can be addressed in a separate, focused PR when you're ready to tackle it.
(\ /)
( ._.)
o_(")(") *understanding nod*
|
Lots of good feedback, and tested a good bit. Merging in |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
152-173: Zero-based explicit coords are clamped, not translated (+1); edits land at wrong positions.When explicit fields are zero-based, the code sets all <1 values to 1 instead of translating to 1-based. This corrupts target ranges. Translate all four coords by +1 (then clamp) in both branches.
Apply this diff in both blocks:
- for k in ("startLine", "startCol", "endLine", "endCol"): - try: - if int(e2.get(k, 1)) < 1: - e2[k] = 1 - except Exception: - pass + for k in ("startLine", "startCol", "endLine", "endCol"): + try: + v = int(e2.get(k, 0)) + e2[k] = max(1, v + 1) + except Exception: + passAlso consider logging instead of bare
except ...: passto aid debugging.Also applies to: 217-239
🧹 Nitpick comments (2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
459-465: Consolidate Annotated metadata strings per param (cleaner for FastMCP parsers).Multiple string metadata entries may be ignored by some clients. Prefer a single descriptive string.
- name: Annotated[str, "Script name (no .cs extension)", "Name of the script to create"], - path: Annotated[str, "Asset path (default: 'Assets/')", "Path under Assets/ to create the script at, e.g., 'Assets/Scripts/My.cs'"], - contents: Annotated[str, "Contents of the script to create", - "C# code for 'create'/'update'"] | None = None, - script_type: Annotated[str, "Script type (e.g., 'C#')", - "Type hint (e.g., 'MonoBehaviour')"] | None = None, + name: Annotated[str, "Script name without .cs; for create, becomes the new file's base name."], + path: Annotated[str, "Assets-relative path. Example: Assets/Scripts/My.cs"], + contents: Annotated[str, "C# contents for create/update (plain text; encoded for transport internally)."] | None = None, + script_type: Annotated[str, "Script type (e.g., 'C#') or hint (e.g., 'MonoBehaviour')."] | None = None,
294-307: Remove unused difflib import or return a minimal diff preview.
diffliband locals are unused; keep it lean or compute a small unified diff if needed.- import difflib - # Apply locally to preview final result - lines = [] - # Build an indexable original from a read if we normalized from read; otherwise skip - prev = "" - # We cannot guarantee file contents here without a read; return normalized spans only + # Return normalized spans only (no local diff build)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(18 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
telemetry_tool(17-107)decorator(19-106)
🪛 Ruff (0.13.1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
16-16: Unused function argument: tool_name
(ARG001)
169-170: try-except-pass detected, consider logging the exception
(S110)
169-169: Do not catch blind exception: Exception
(BLE001)
234-235: try-except-pass detected, consider logging the exception
(S110)
234-234: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (5)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (5)
76-89: Good FastMCP metadata and UX notes for apply_text_edits.Clear description, step-by-step guidance, and 1-indexing callout are on point.
380-390: Path hygiene looks solid.Good normalization and guardrails against traversal/absolute paths; ensures under Assets/.
515-544: Capability surface is concise and consistent.Clear ops/text_ops and max payload; extras advertise get_sha for clients. LGTM.
547-566: Minimal SHA endpoint is useful and safe.Returns only sha256/lengthBytes; good for optimistic concurrency without content fetch.
454-466: Restore 'update' in the action Literal to keep legacy calls working.Function still routes legacy update flows, but the enum excludes it, so FastMCP clients will reject those requests pre-dispatch.
- action: Annotated[Literal['create', 'read', 'delete'], "Perform CRUD operations on C# scripts."], + action: Annotated[Literal['create', 'read', 'update', 'delete'], "Perform CRUD operations on C# scripts. Note: 'update' is deprecated but retained for backward compatibility."],#!/bin/bash # Find any in-repo callers still sending 'update' rg -nP -C2 "['\"]action['\"]\s*:\s*['\"]update['\"]"
* refactor: migrate command routing to use CommandRegistry lookup instead of switch statement * style: improve code formatting and indentation consistency * refactor: clean up imports and type hints across tool modules * Revert "feat: Implement Asset Store Compliance for Unity MCP Bridge" This reverts commit 2fca7fc. * Revert "feat(asset-store): implement post-installation prompt system for Asset Store compliance" This reverts commit ab25a71. * chore: upgrade mcp[cli] dependency from 1.4.1 to 1.15.0 * style: fix formatting and whitespace in Python server files * Remove description, probably a Python versionn change * feat: add type hints and parameter descriptions to Unity MCP tools * docs: improve shader management tool parameter descriptions and types * refactor: add type annotations and improve documentation for script management tools * refactor: improve type annotations and documentation in manage_scene tool * refactor: add type annotations and improve parameter descriptions across MCP tools * feat: add explicit name parameters to all MCP tool decorators * refactor: remove unused Unity connection instance in manage_asset_tools * chore: update type hints in manage_editor function parameters for better clarity * feat: make name and path parameters optional for scene management operations * refactor: remove unused get_unity_connection import from manage_asset.py * chore: rename Operation parameter annotation to Operations for consistency * feat: add logging to MCP clients for tool actions across MCP server components * chore: add FastMCP type hint to register_all_tools parameter * style: reformat docstring in apply_text_edits tool to use multiline string syntax * refactor: update type hints from Dict/List/Tuple/Optional to modern Python syntax * refactor: clean up imports and add type annotations to script editing tools * refactor: update type hints to use | None syntax for optional parameters * Minor fixes * docs: improve tool descriptions with clearer action explanations * refactor: remove legacy update action migration code from manage_script.py * style: replace em dashes with regular hyphens in tool descriptions [skip ci] * refactor: convert manage_script_capabilities docstring to multiline format [skip ci]
We're just following FastMCP's docs, minus using Pydantic to validate data entry. Closes #289
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores