Refactor framed I/O and harden server paths #8
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements a comprehensive refactor of the Unity MCP (Model Context Protocol) Bridge focusing on protocol framing improvements, security hardening, and code consolidation. The changes span both the C# Unity Bridge and Python MCP server components.
Core Protocol Improvements:
The main enhancement centralizes framed socket I/O logic in UnityMcpBridge.cs by extracting duplicate read/write operations into reusable methods ReadFrameAsUtf8Async and WriteFrameAsync. This eliminates code duplication that existed across the HandleClientAsync method while adding robust frame size validation, including overflow checks for int.MaxValue. The framing protocol maintains backward compatibility while enforcing consistent length validation.
Security and Path Management:
Significant security improvements were made to path handling in the Python server. The new _resolve_safe_path_from_uri() function prevents directory traversal attacks by validating that all resolved paths remain within the project root using pathlib's relative_to() method. Environment variable support for UNITY_PROJECT_ROOT provides deployment flexibility while maintaining security boundaries.
Protocol Communication Fixes:
A critical fix addresses stdout interference in the MCP protocol communication. Tool registration now uses proper logging instead of print statements, preventing corruption of JSON message exchange between the server and AI clients. This is essential for MCP protocol compliance.
Development Workflow Improvements:
Script management tools were streamlined by deprecating the 'update' operation in favor of more granular apply_text_edits, aligning with modern LSP-style editing patterns. The edit bounds checking logic was enhanced to properly handle end-of-file append operations while maintaining safety.
Connection Robustness:
Handshake error handling was improved in the Unity connection layer, now sending properly framed error messages when FRAMING=1 requirements aren't met, along with better socket cleanup to prevent resource leaks.
Important Files Changed
File Changes Summary
| Filename | Score | Overview |
|---|---|---|
| UnityMcpBridge/Editor/UnityMcpBridge.cs | 4/5 | Centralizes framed I/O logic into reusable methods and adds robust frame size validation |
| UnityMcpBridge/UnityMcpServer~/src/server.py | 4/5 | Implements secure path resolution with directory traversal protection and environment variable support |
| UnityMcpBridge/UnityMcpServer~/src/tools/init.py | 5/5 | Replaces print statements with proper logging to prevent stdout interference in MCP protocol |
| UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py | 4/5 | Deprecates 'update' operation and simplifies content handling logic |
| UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py | 4/5 | Improves bounds checking logic for end-of-file append operations |
| UnityMcpBridge/UnityMcpServer~/src/unity_connection.py | 4/5 | Enhances handshake error handling with framed error responses and socket cleanup |
Confidence score: 4/5
- This PR is generally safe to merge with good architectural improvements and security enhancements
- Score reflects solid refactoring with proper error handling, though some changes modify core protocol behavior
- Pay close attention to the path resolution changes in server.py and truthiness logic changes in manage_script.py
Sequence Diagram
sequenceDiagram
participant User
participant Client as "MCP Client"
participant Server as "Python Server"
participant Unity as "Unity Bridge"
User->>Client: "Send MCP command"
Client->>Server: "Tool call with parameters"
Server->>Server: "Validate parameters"
Server->>Unity: "Connect via TCP socket"
Unity->>Server: "WELCOME UNITY-MCP 1 FRAMING=1"
Server->>Unity: "Send framed command with length header"
Unity->>Unity: "Parse JSON command"
Unity->>Unity: "Execute command via tool handlers"
Unity->>Server: "Send framed response with results"
Server->>Client: "Return structured response"
Client->>User: "Display results"
6 files reviewed, 2 comments
| "scriptType": script_type, | ||
| } | ||
| if contents is not None: | ||
| if contents: |
There was a problem hiding this comment.
logic: Changing from if contents is not None: to if contents: means empty strings will now be treated as falsy. This could break existing functionality if empty script creation was previously supported.
| # Base64 encode the contents if they exist to avoid JSON escaping issues | ||
| if contents is not None: | ||
| if action in ['create', 'update']: | ||
| if contents: |
There was a problem hiding this comment.
logic: Same truthiness change here - verify that empty string contents for non-create actions should be ignored rather than processed.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bugbot free trial expires on August 31, 2025
Learn more in the Cursor dashboard.
| if contents is not None: | ||
| if action in ['create', 'update']: | ||
| if contents: | ||
| if action == 'create': |
There was a problem hiding this comment.
Bug: Empty Scripts Fail Base64 Encoding
The if contents: condition now treats empty strings as falsy, preventing base64 encoding and sending of explicitly empty script content. This breaks creating scripts with intentionally empty content, as Unity may expect the encodedContents field even for empty strings. This impacts both create_script and manage_script.
Additional Locations (1)
| header = self._read_exact(sock, 8) | ||
| payload_len = struct.unpack('>Q', header)[0] | ||
| if payload_len == 0 or payload_len > (64 * 1024 * 1024): | ||
| if payload_len > (64 * 1024 * 1024): |
There was a problem hiding this comment.
Bug: Protocol Mismatch: Zero-Length Messages
The Python side now permits zero-length framed messages, but the Unity C# side still rejects them. This creates a protocol mismatch, leading to communication failures when empty frames are sent.
Additional Locations (1)
* refactor: Split ParseColorOrDefault into two overloads and change default to Color.white * Auto-format Python code * Remove unused Python module * Refactored VFX functionality into multiple files Tested everything, works like a charm * Rename ManageVfx folder to just Vfx We know what it's managing * Clean up whitespace on plugin tools and resources * Make ManageGameObject less of a monolith by splitting it out into different files * Remove obsolete FindObjectByInstruction method We also update the namespace for ManageVFX * refactor: Consolidate editor state resources into single canonical implementation Merged EditorStateV2 into EditorState, making get_editor_state the canonical resource. Updated Unity C# to use EditorStateCache directly. Enhanced Python implementation with advice/staleness enrichment, external changes detection, and instance ID inference. Removed duplicate EditorStateV2 resource and legacy fallback mapping. * Validate editor state with Pydantic models in both C# and Python Added strongly-typed Pydantic models for EditorStateV2 schema in Python and corresponding C# classes with JsonProperty attributes. Updated C# to serialize using typed classes instead of anonymous objects. Python now validates the editor state payload before returning it, catching schema mismatches early. * Consolidate run_tests and run_tests_async into single async implementation Merged run_tests_async into run_tests, making async job-based execution the default behavior. Removed synchronous blocking test execution. Updated RunTests.cs to start test jobs immediately and return job_id for polling. Changed TestJobManager methods to internal visibility. Updated README to reflect single run_tests_async tool. Python implementation now uses async job pattern exclusively. * Validate test job responses with Pydantic models in Python * Change resources URI from unity:// to mcpforunity:// It should reduce conflicts with other Unity MCPs that users try, and to comply with Unity's requests regarding use of their company and product name * Update README with all tools + better listing for resources * Update other references to resources * Updated translated doc - unfortunately I cannot verify * Update the Chinese translation of the dev docks * Change menu item from Setup Window to Local Setup Window We now differentiate whether it's HTTP local or remote * Fix URIs for menu items and tests * Shouldn't have removed it * Minor edits from CodeRabbit feedback * Don't use reflection which takes longer * Fix failing python tests * Add serialization helpers for ParticleSystem curves and MinMaxCurve types Added SerializeAnimationCurve and SerializeMinMaxCurve helper methods to properly serialize Unity's curve types. Updated GetInfo to use these helpers for startLifetime, startSpeed, startSize, gravityModifier, and rateOverTime instead of only reading constant values. * Use ctx param * Update Server/src/services/tools/run_tests.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Minor fixes * Rename anything EditorStateV2 to just EditorState It's the default, there's no old version * Make infer_single_instance_id public by removing underscore prefix * Fix Python tests, again * Replace AI generated .meta files with actual Unity ones * ## Pre-Launch Enhancements: Testing Infrastructure & Tool Improvements (#8) * Add local test harness for fast developer iteration Scripts for running the NL/T/GO test suites locally against a GUI Unity Editor, complementing the CI workflows in .github/workflows/. Benefits: - 10-100x faster than CI (no Docker startup) - Real-time Unity console debugging - Single test execution for rapid iteration - Auto-detects HTTP vs stdio transport Usage: ./scripts/local-test/setup.sh # One-time setup ./scripts/local-test/quick-test.sh NL-0 # Run single test ./scripts/local-test/run-nl-suite-local.sh # Full suite See scripts/local-test/README.md for details. Also updated .gitignore to: - Allow scripts/local-test/ to be tracked - Ignore generated artifacts (reports/*.xml, .claude/local/, .unity-mcp/) * Fix issue CoplayDev#525: Save dirty scenes for all test modes Move SaveDirtyScenesIfNeeded() call outside the PlayMode conditional so EditMode tests don't get blocked by Unity's "Save Scene" modal dialog. This prevents MCP from timing out when running EditMode tests with unsaved scene changes. * fix: add missing FAST_FAIL_TIMEOUT constant in PluginHub The FAST_FAIL_TIMEOUT class attribute was referenced on line 149 but never defined, causing AttributeError on every ping attempt. This error was silently caught by the broad 'except Exception' handler, causing all fast-fail commands (read_console, get_editor_state, ping) to fail after 6 seconds of retries with 'ping not answered' error. Added FAST_FAIL_TIMEOUT = 10 to define a 10-second timeout for fast-fail commands, matching the intent of the existing fast-fail infrastructure. * feat(ScriptableObject): enhance dry-run validation for AnimationCurve and Quaternion Dry-run validation now validates value formats, not just property existence: - AnimationCurve: Validates structure ({keys:[...]} or direct array), checks each keyframe is an object, validates numeric fields (time, value, inSlope, outSlope, inWeight, outWeight) and integer fields (weightedMode) - Quaternion: Validates array length (3 for Euler, 4 for raw) or object structure ({x,y,z,w} or {euler:[x,y,z]}), ensures all components are numeric Refactored shared validation helpers into appropriate locations: - ParamCoercion: IsNumericToken, ValidateNumericField, ValidateIntegerField - VectorParsing: ValidateAnimationCurveFormat, ValidateQuaternionFormat Added comprehensive XML documentation clarifying keyframe field defaults (all default to 0 except as noted). Added 5 new dry-run validation tests covering valid and invalid formats for both AnimationCurve and Quaternion properties. * test: fix integration tests after merge - test_refresh_unity_retry_recovery: Mock now handles both refresh_unity and get_editor_state commands (refresh_unity internally calls get_editor_state when wait_for_ready=True) - test_run_tests_async_forwards_params: Mock response now includes required 'mode' field for RunTestsStartResponse Pydantic validation - test_get_test_job_forwards_job_id: Updated to handle GetTestJobResponse as Pydantic model instead of dict (use model_dump() for assertions) * Update warning message to apply to all test modes Follow-up to PR CoplayDev#527: Since SaveDirtyScenesIfNeeded() now runs for all test modes, update the warning message to say 'tests' instead of 'PlayMode tests'. * feat(run_tests): add wait_timeout to get_test_job to avoid client loop detection When polling for test completion, MCP clients like Cursor can detect the repeated get_test_job calls as 'looping' and terminate the agent. Added wait_timeout parameter that makes the server wait internally for tests to complete (polling Unity every 2s) before returning. This dramatically reduces client-side tool calls from 10-20 down to 1-2, avoiding loop detection. Usage: get_test_job(job_id='xxx', wait_timeout=30) - Returns immediately if tests complete within timeout - Returns current status if timeout expires (client can call again) - Recommended: 30-60 seconds * fix: use Pydantic attribute access in test_run_tests_async for merge compatibility * revert: remove local test harness - will be submitted in separate PR --------- Co-authored-by: Scott Jennings <scott.jennings+CIGINT@cloudimperiumgames.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: dsarno <david@lighthaus.us> Co-authored-by: Scott Jennings <scott.jennings+CIGINT@cloudimperiumgames.com>
Summary
Testing
pytest -qhttps://chatgpt.com/codex/tasks/task_e_68a0e9a504208327bdbeb2a5e75223fc