Protocol framing #20
Protocol framing #20
Conversation
…ove blob stream tools; simplify tool registration - Python server: always consume handshake and negotiate framing on reconnects (prevents invalid framed length).\n- C#: strict FRAMING=1 handshake and NoDelay; debounce AssetDatabase/compilation.\n- Tools: keep manage_script + script edits; remove manage_script_stream and test tools from default registration.\n- Editor window: guard against auto retargeting IDE config.
…Exact, safe write framing; remove unused vars
… MCP edit ops; mitigate false 'no opening brace' errors and allow relaxed validation for text edits
…ority and server apply_text_edits/validate; add resources list/read; deprecate manage_script read/update/edit; remove stdout prints; tweak connection handshake logging
…ard-compatibility
…ard-compatibility-g252rq
…n-c#-and-python
…ow-in-byte-count
Add initial transport handshake tests with plan placeholders
…ests Convert skip decorators to xfail and harden framing tests
…-logging Add handshake fallback and logging checks
…ck; add uv -q for quieter stdio; MCP server: compatibility guards for capabilities/resource decorators and indentation fix; ManageScript: shadow var fix; robust mac config path.
…text edits; anchor aliasing and text-op conversion; immediate compile on NL/structured; add resource_tools (tail_lines, find_in_file); update test cases
…-response Fix ApplyTextEdits replacement index calculation and port probe handshake
…le-level-helper
|
Caution Review failedThe pull request is closed. WalkthroughAdds macOS config path support for Claude Desktop, introduces a new ManageEditor get_project_root action, significantly expands ManageScript with safe path resolution, edit/validate actions, and debounced refresh, enforces strict framed I/O in the Unity bridge and Python client, adds resource APIs and tool registrations, and introduces comprehensive tests and configs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client (Python)
participant Conn as UnityConnection
participant Unity as UnityMcpBridge (Editor)
Client->>Conn: connect()
Conn->>Unity: TCP connect
Unity-->>Conn: "WELCOME UNITY-MCP 1 FRAMING=1"
Conn->>Conn: enable framing
Client->>Conn: send_command_with_retry(type, params)
Conn->>Unity: [8-byte len][JSON]
Unity-->>Conn: [8-byte len][JSON response]
Conn-->>Client: parsed response
sequenceDiagram
participant Tool as script_apply_edits (server)
participant UTools as manage_script tools
participant Unity as ManageScript.cs
participant Editor as Unity Editor
Tool->>UTools: apply_text_edits(uri, edits, precondition)
UTools->>Unity: action=apply_text_edits (framed)
Unity->>Editor: validate, apply, atomic write, schedule refresh
Unity-->>UTools: {success, sha256, uri}
UTools-->>Tool: forwarded response
sequenceDiagram
participant Client as MCP Client
participant RTools as resource_tools
participant UEdit as ManageEditor.cs
Client->>RTools: list_resources(project=auto)
RTools->>UEdit: command get_project_root
UEdit-->>RTools: {projectRoot}
RTools->>RTools: resolve safe paths, glob *.cs under Assets
RTools-->>Client: {uris, count}
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (21)
✨ 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 protocol framing upgrade for the Unity MCP (Model Context Protocol) bridge system. The changes introduce a new binary framing protocol that replaces the previous text-based communication with 8-byte big-endian length headers for message boundaries. This addresses fundamental reliability issues where JSON parsing could fail with partial or malformed messages.
Key architectural changes include:
Protocol Layer: The core Unity-MCP TCP communication now uses strict framing with mandatory handshake (FRAMING=1) and timeout-aware I/O operations. All messages are prefixed with their exact length, eliminating message boundary ambiguity.
Script Management: Major refactoring of script editing capabilities with new granular tools (apply_text_edits, create_script, delete_script, validate_script) that support incremental editing rather than full-file replacement. This includes SHA256 precondition checking for concurrent modification protection and atomic file operations.
Cross-Platform Support: Enhanced macOS compatibility with proper configuration path handling for Claude Desktop and other MCP clients.
Resource Management: Addition of MCP resources API that allows AI assistants to discover and read Unity C# scripts directly through the protocol, with proper path sandboxing and security controls.
Logging Infrastructure: Comprehensive logging overhaul that ensures stdout remains clean for MCP protocol communication, with all diagnostic output redirected to stderr and optional file logging.
The changes maintain backward compatibility where possible while establishing a foundation for more reliable MCP communication. The new framed protocol supports large payload transfers (up to 64MB) with proper flow control and prevents the connection issues that could occur with the previous implementation.
Important Files Changed
File Changes Overview
| Filename | Score | Overview |
|---|---|---|
UnityMcpBridge/Editor/UnityMcpBridge.cs |
4/5 | Implements strict binary protocol framing with 8-byte headers and mandatory handshake |
UnityMcpBridge/Editor/Tools/ManageScript.cs |
4/5 | Major refactor adding transactional text editing, security hardening, and AST-based operations |
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py |
3/5 | Introduces framed communication with thread-safe I/O and handshake validation |
UnityMcpBridge/UnityMcpServer~/src/server.py |
4/5 | Adds MCP resources API and strict logging configuration for protocol compliance |
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py |
4/5 | New advanced script editing tool with natural language parsing and structured operations |
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py |
2/5 | Resource wrapper tools with potential security vulnerabilities in path handling |
UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs |
4/5 | Adds UV quiet flag and UTF-8 BOM handling to prevent protocol interference |
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py |
4/5 | Refactors to granular tools and deprecates monolithic update operations |
UnityMcpBridge/UnityMcpServer~/src/port_discovery.py |
4/5 | Implements framing detection in port discovery with protocol negotiation |
UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py |
3/5 | Updates tool registration with logging and new module imports |
test_unity_socket_framing.py |
4/5 | New comprehensive socket framing test with large payload validation |
tests/test_transport_framing.py |
4/5 | Transport layer framing tests with handshake and protocol enforcement |
tests/test_script_tools.py |
4/5 | Comprehensive script management testing with MCP dependency mocking |
tests/test_script_editing.py |
5/5 | Test framework stubs for script editing safety and performance features |
tests/test_logging_stdout.py |
4/5 | AST-based validation ensuring no stdout contamination in server code |
tests/test_resources_api.py |
4/5 | Security-focused test stubs for resource API path validation |
UnityMcpBridge/Editor/Tools/ManageEditor.cs |
4/5 | Adds get_project_root command for external tool navigation |
UnityMcpBridge/Editor/Data/McpClients.cs |
5/5 | Adds macOS configuration path for Claude Desktop cross-platform support |
UnityMcpBridge/Editor/Models/McpClient.cs |
5/5 | Adds macOS configuration path field to model definition |
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py |
5/5 | Trivial newline addition for POSIX compliance |
UnityMcpBridge/UnityMcpServer~/src/pyrightconfig.json |
5/5 | Python Language Server configuration for improved development experience |
Confidence score: 3/5
- This PR introduces significant architectural changes that improve protocol reliability but includes complex logic that could introduce subtle bugs
- Score reflects the extensive scope of changes across critical communication pathways and the introduction of new security-sensitive path handling logic
- Pay close attention to
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.pyfor path traversal vulnerabilities andunity_connection.pyfor handshake timeout edge cases
Sequence Diagram
sequenceDiagram
participant User
21 files reviewed, 13 comments
| string commandText = null; | ||
| bool usedFraming = true; | ||
|
|
||
| if (true) |
There was a problem hiding this comment.
style: Dead code: if (true) condition makes the branch always execute and usedFraming variable is always true
| if (true) | |
| // Enforced framed mode for this connection | |
| commandText = await ReadFrameAsUtf8Async(stream, FrameIOTimeoutMs); |
| { | ||
| throw new System.IO.IOException("Read timed out"); | ||
| } | ||
| int r = readTask.Result; |
There was a problem hiding this comment.
logic: Potential race condition: accessing readTask.Result without checking if the task completed successfully could throw if the read task faulted
| stderr_handler = logging.StreamHandler(stream=sys.stderr) | ||
| stderr_handler.setFormatter(logging.Formatter(config.log_format)) | ||
|
|
||
| handlers = [stderr_handler] |
There was a problem hiding this comment.
style: handlers variable is defined but never used
| handlers = [stderr_handler] | |
| stderr_handler = logging.StreamHandler(stream=sys.stderr) | |
| stderr_handler.setFormatter(logging.Formatter(config.log_format)) |
| # Read a small bounded amount looking for pong | ||
| chunks = [] | ||
| total = 0 | ||
| data = b"" |
There was a problem hiding this comment.
style: variable data is initialized but immediately overwritten in the loop - this initialization is redundant
| PROJECT_ROOT = Path(os.environ.get("UNITY_PROJECT_ROOT", Path.cwd())).resolve() | ||
| ASSETS_ROOT = (PROJECT_ROOT / "Assets").resolve() | ||
|
|
||
| def _resolve_safe_path_from_uri(uri: str) -> Path | None: |
There was a problem hiding this comment.
style: Union type syntax Path | None requires Python 3.10+, consider Optional[Path] for compatibility
| if payload_len == 0: | ||
| raise Exception("Invalid framed length: 0") |
There was a problem hiding this comment.
style: Zero-length payload rejection may be overly strict - some protocols allow zero-length messages as keepalives or control messages
| header = struct.pack('>Q', len(payload)) | ||
| self.sock.sendall(header) | ||
| self.sock.sendall(payload) |
There was a problem hiding this comment.
style: Sending header and payload in separate sendall() calls could cause partial writes in edge cases - consider combining into single send
| elif uri.startswith("file://"): | ||
| raw = uri[len("file://"):] |
There was a problem hiding this comment.
logic: Potential path traversal vulnerability with file:// URIs - they bypass the unity://path/ prefix validation and could reference arbitrary filesystem paths
| window = int(m.group(1)) | ||
| method = m.group(2) | ||
| # naive search for method header to get a line number | ||
| text_all = p.read_text(encoding="utf-8") |
There was a problem hiding this comment.
style: Reading entire file content twice (here and later) is inefficient for large files - consider reading once and reusing
| - ignore_case: case-insensitive by default | ||
| - max_results: cap results to avoid huge payloads | ||
| """ | ||
| import re |
There was a problem hiding this comment.
syntax: Redundant import re statement since re is already imported at module level
| import re | |
| try: |
1 participant
Summary by CodeRabbit
New Features
Chores
Tests
Style