-
Notifications
You must be signed in to change notification settings - Fork 0
Fix ApplyTextEdits replacement index calculation and port probe handshake #18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ApplyTextEdits replacement index calculation and port probe handshake #18
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Summary
This PR addresses two critical fixes in the Unity MCP Bridge system. The first fix resolves a dangerous bug in the ApplyTextEdits
method's auto-upgrade logic within ManageScript.cs
. The original implementation had a flawed substring calculation that could cause IndexOutOfRangeException
or produce incorrect text replacements when converting text edits to structured replace_method
operations. The new implementation properly extracts the method text first, converts edit positions to relative coordinates within that method, clamps ranges to prevent out-of-bounds access, and then performs safe replacement operations.
The second fix updates the port discovery mechanism in port_discovery.py
to support the new framing protocol (FRAMING=1). The Unity MCP Bridge now sends greeting messages advertising framing capabilities, and the probe function has been enhanced to detect these greetings and handle both framed and unframed ping/pong exchanges. When FRAMING=1 is detected, messages are sent/received with 8-byte big-endian length prefixes, ensuring proper communication with modern Unity MCP servers while maintaining backward compatibility.
These changes integrate well with the existing codebase architecture - the ManageScript.cs
fix preserves the auto-upgrade functionality for text editing operations while making them crash-safe, and the port_discovery.py
enhancement extends the existing port probing mechanism to work with the new protocol framing system that appears to be part of a broader protocol evolution in this MCP implementation.
Important Files Changed
Click to expand file changes
Filename | Score | Overview |
---|---|---|
UnityMcpBridge/Editor/Tools/ManageScript.cs | 4/5 | Fixed critical substring calculation bug in ApplyTextEdits auto-upgrade logic |
UnityMcpBridge/UnityMcpServer~/src/port_discovery.py | 4/5 | Added FRAMING=1 protocol support to port discovery probe mechanism |
Confidence score: 4/5
- This PR is safe to merge with minimal risk as both fixes address specific, well-defined issues
- Score reflects solid bug fixes with clear improvements to system reliability and protocol compatibility
- Pay close attention to UnityMcpBridge/Editor/Tools/ManageScript.cs to ensure the new substring logic handles all edge cases correctly
Sequence Diagram
sequenceDiagram
participant User
participant MCP_Server as MCP Server
participant Port_Discovery as Port Discovery
participant Unity_Bridge as Unity Bridge
participant Unity_Editor as Unity Editor
participant Script_Manager as Script Manager
participant Validation as Validation System
User->>MCP_Server: "apply_text_edits request"
MCP_Server->>Port_Discovery: "discover_unity_port()"
Port_Discovery->>Port_Discovery: "list_candidate_files()"
Port_Discovery->>Unity_Bridge: "TCP connect + probe with FRAMING=1"
Unity_Bridge-->>Port_Discovery: "greeting with FRAMING=1"
Port_Discovery->>Unity_Bridge: "framed ping (8-byte header + payload)"
Unity_Bridge-->>Port_Discovery: "framed pong response"
Port_Discovery-->>MCP_Server: "validated port number"
MCP_Server->>Unity_Bridge: "forward apply_text_edits request"
Unity_Bridge->>Script_Manager: "HandleCommand(apply_text_edits)"
Script_Manager->>Script_Manager: "validate precondition_sha256"
Script_Manager->>Script_Manager: "convert edits to absolute indices"
alt single edit targeting method
Script_Manager->>Script_Manager: "detect method span for auto-upgrade"
Script_Manager->>Script_Manager: "compute replacement relative to method span"
Script_Manager->>Script_Manager: "upgrade to replace_method operation"
else multiple/other edits
Script_Manager->>Script_Manager: "apply text edits in reverse order"
end
Script_Manager->>Validation: "CheckBalancedDelimiters()"
Validation-->>Script_Manager: "validation result"
opt USE_ROSLYN defined
Script_Manager->>Validation: "Roslyn syntax validation"
Validation-->>Script_Manager: "syntax diagnostics"
end
Script_Manager->>Unity_Editor: "atomic file write with backup"
Script_Manager->>Unity_Editor: "ScheduleScriptRefresh()"
Unity_Editor->>Unity_Editor: "debounced asset import and compilation"
Script_Manager-->>Unity_Bridge: "success response with new SHA256"
Unity_Bridge-->>MCP_Server: "operation result"
MCP_Server-->>User: "edits applied successfully"
2 files reviewed, no comments
Summary
Testing
pytest -q
https://chatgpt.com/codex/tasks/task_e_68a296f62bcc8327addf3431c744a0da