feat: add initial script edit tests #3
Conversation
WalkthroughIntroduces precondition-aware, size-limited, and syntax-validated text edit application in Unity C# tooling; enhances create/delete/validate responses; adds Python tool endpoints for apply_text_edits, create, delete, validate with URI normalization; refines response decoding and error handling; adjusts a test script’s CLI parsing; adds targeted tests; minor formatting in manage_asset. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PyTools as Python Tools (manage_script.py)
participant Unity as Unity ManageScript.cs
participant FS as File System
Client->>PyTools: apply_text_edits(uri, edits, precondition_sha256?)
PyTools->>Unity: action=apply_text_edits (name, path, edits, precondition_sha256)
Unity->>Unity: Validate size, delimiters, optional Roslyn parse/format
Unity->>FS: Atomic write updated contents
Unity-->>PyTools: {applied/unchanged, sha256, uri, scheduledRefresh | error (stale_file/too_large/unbalanced_braces/syntax_error)}
PyTools-->>Client: Structured response
Client->>PyTools: delete_script(uri)
PyTools->>Unity: action=delete
Unity->>FS: MoveAssetToTrash
Unity-->>PyTools: {deleted: true}
PyTools-->>Client: Success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (1)
71-80: Normalize error payloads for consistency with manage_script responsesmanage_script.manage_script normalizes backend errors to use the "message" key. Here, manage_asset returns the raw dict (with "error" from the C# side). Consider normalizing "error" → "message" for consistency across tool endpoints.
Apply this diff to normalize the response shape:
# Use centralized async retry helper to avoid blocking the event loop result = await async_send_command_with_retry("manage_asset", params_dict, loop=loop) # Return the result obtained from Unity - return result if isinstance(result, dict) else {"success": False, "message": str(result)} + if isinstance(result, dict): + # Normalize error shape for consistency with manage_script + if not result.get("success") and "message" not in result and "error" in result: + result = {**result, "message": result["error"]} + return result + return {"success": False, "message": str(result)}UnityMcpBridge/Editor/Tools/ManageScript.cs (4)
506-513: Delimiter check message wordingThe status/message uses "unbalanced_braces", but the check validates braces, parentheses, and brackets. Consider renaming to "unbalanced_delimiters" to better reflect the check and reduce confusion downstream.
- string hint = $"unbalanced_braces at line {line}. Call resources/read for lines {startLine}-{endLine} and resend a smaller apply_text_edits that restores balance."; - return Response.Error(hint, new { status = "unbalanced_braces", line, expected = expected.ToString() }); + string hint = $"unbalanced_delimiters at line {line}. Call resources/read for lines {startLine}-{endLine} and resend a smaller apply_text_edits that restores balance."; + return Response.Error(hint, new { status = "unbalanced_delimiters", line, expected = expected.ToString() });
528-536: Dispose Roslyn workspace to avoid leaksAdhocWorkspace implements IDisposable. Use a using declaration and the imported Formatter symbol.
- var root = tree.GetRoot(); - var workspace = new AdhocWorkspace(); - root = Microsoft.CodeAnalysis.Formatting.Formatter.Format(root, workspace); - working = root.ToFullString(); + var root = tree.GetRoot(); + using var workspace = new AdhocWorkspace(); + root = Formatter.Format(root, workspace); + working = root.ToFullString();
571-589: CRLF handling in line/column indexer to avoid off-by-oneTryIndexFromLineCol treats only '\n' as newline. On CRLF files, '\r' is counted as a character and can skew column positions. Normalize CRLF as a single newline during indexing.
for (int i = 0; i <= text.Length; i++) { if (line == line1 && col == col1) { index = i; return true; } if (i == text.Length) break; char c = text[i]; - if (c == '\n') { line++; col = 1; } - else { col++; } + if (c == '\r') + { + // Normalize CRLF as a single newline + if (i + 1 < text.Length && text[i + 1] == '\n') i++; + line++; col = 1; + } + else if (c == '\n') { line++; col = 1; } + else { col++; } }
231-234: Update the “Unknown action” help textThe message omits newly supported actions (apply_text_edits, validate) and still references deprecated ones. Update to reflect the current surface to reduce confusion.
- return Response.Error( - $"Unknown action: '{action}'. Valid actions are: create, read, update, delete." - ); + return Response.Error( + $"Unknown action: '{action}'. Valid actions are: create, delete, apply_text_edits, validate." + );UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
1-8: Remove unused imports flagged by static analysisRuff reports unused imports: get_unity_connection, config, time. Trim them to keep the module clean.
-from mcp.server.fastmcp import FastMCP, Context -from typing import Dict, Any, List -from unity_connection import get_unity_connection, send_command_with_retry -from config import config -import time +from mcp.server.fastmcp import FastMCP, Context +from typing import Dict, Any, List +from unity_connection import send_command_with_retry import os import base64
25-44: Consider normalizing response shape across all tool endpointsapply_text_edits returns the raw backend dict, while manage_script wraps responses into a {"success","message","data"} shape (and decodes contents when present). For a consistent public API, consider applying the same normalization across apply_text_edits/create_script/delete_script/validate_script.
I can provide a small helper to wrap/normalize responses if you want to pursue this.
tests/test_script_tools.py (2)
1-6: Drop unused pytest importPer Ruff F401, pytest is imported but not used.
-import pytest
77-95: Test assumes sha256 at top-level; production returns it under dataYour fake send returns {"success": True, "sha256": "..."} and the assertion follows suit. In production, ManageScript.cs returns sha256 inside data for apply_text_edits. If you later wire this test to real responses, adjust to resp.get("data", {}).get("sha256") to avoid brittle expectations.
test_unity_socket_framing.py (2)
6-9: Optionally validate/clamp SIZE_MB to avoid zero/negative sizesIf a negative or zero value is provided, the filter becomes empty and can silently mask the intended stress scenario. A tiny guard keeps intent intact without affecting normal usage.
Apply this minimal change:
try: - SIZE_MB = int(sys.argv[1]) + SIZE_MB = max(1, int(sys.argv[1])) except (IndexError, ValueError): SIZE_MB = 5 # e.g., 5 or 10
64-67: Narrow exception handling for greeting readCatching Exception is overly broad and can hide programming errors. Limit to socket.timeout and OSError for clarity without changing behavior.
- try: - greeting = s.recv(256) - except Exception: - greeting = b"" + try: + greeting = s.recv(256) + except (socket.timeout, OSError): + greeting = b""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
UnityMcpBridge/Editor/Tools/ManageScript.cs(11 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(3 hunks)test_unity_socket_framing.py(1 hunks)tests/test_script_tools.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
get_unity_connection(291-307)send_command_with_retry(324-343)tests/test_script_tools.py (1)
tool(42-46)
UnityMcpBridge/Editor/Tools/ManageScript.cs (1)
UnityMcpBridge/Editor/Helpers/Response.cs (3)
Success(18-33)Response(10-58)Error(41-57)
🪛 Ruff (0.12.2)
tests/test_script_tools.py
5-5: pytest imported but unused
Remove unused import: pytest
(F401)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
3-3: unity_connection.get_unity_connection imported but unused
Remove unused import: unity_connection.get_unity_connection
(F401)
4-4: config.config imported but unused
Remove unused import: config.config
(F401)
5-5: time imported but unused
Remove unused import: time
(F401)
🔇 Additional comments (5)
UnityMcpBridge/Editor/Tools/ManageScript.cs (4)
195-200: Precondition SHA-256 passed through correctlyThreading precondition_sha256 from HandleCommand into ApplyTextEdits enables stale-file protection and aligns with the Python caller. LGTM.
453-458: Stale-file protection: good use of SHA-256 with clear diagnosticsPrecondition check returns a structured stale_file response with both expected and current SHA. This avoids silent overwrites and aids clients in sequencing edits. LGTM.
461-491: Edit payload size guard is appropriateTracking totalBytes and enforcing MaxEditPayloadBytes prevents heavy payloads from blocking the editor thread and guides clients to split edits. LGTM.
552-564: Enhanced success payload on apply_text_edits is well-structuredIncluding applied, sha256, uri, and scheduledRefresh provides clients everything needed for subsequent preconditioned edits and UI updates. LGTM.
test_unity_socket_framing.py (1)
6-9: Import-safe CLI parsing: good fix for pytest argv contaminationCatching both IndexError and ValueError prevents import-time failures under pytest when argv contains non-integer flags. This makes the helper safe to import in test runs.
1 participant
Summary
Testing
python -m pytest -qhttps://chatgpt.com/codex/tasks/task_e_68a01cf305748327920ba5ce80d0b854
Summary by CodeRabbit