Skip to content

Conversation

@dsarno
Copy link
Owner

@dsarno dsarno commented Aug 16, 2025

Summary

  • keep deprecated manage_script read/update/edit actions as compatibility aliases with warnings
  • handle CRLF newlines correctly when mapping line/column to index
  • flag IDE config mismatches when auto-manage is disabled
  • support explicit 'basic', 'standard', 'strict', and 'comprehensive' validation levels
  • cap framed response length in socket test
  • pin Python 3.11 version for pyright

Testing

  • pytest -q

https://chatgpt.com/codex/tasks/task_e_68a083d83a108327a0cb602fdeb3b1a7

@dsarno dsarno merged commit 031f5d7 into protocol-framing Aug 16, 2025
1 check passed
@coderabbitai
Copy link

coderabbitai bot commented Aug 16, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/update-managescript-for-backward-compatibility-g252rq

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@greptile-apps greptile-apps bot left a 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 introduces several backward compatibility improvements and safety enhancements to the Unity MCP project. The primary focus is on restoring deprecated manage_script actions (read, update, edit) as compatibility aliases while maintaining deprecation warnings to guide users toward newer alternatives. The changes also expand validation level support to include 'standard' and 'comprehensive' options alongside existing 'basic' and 'strict' levels.

Key technical improvements include better CRLF newline handling when mapping line/column positions to string indices - a critical fix for Windows environments where text editors use different line ending conventions. The PR also enhances IDE configuration mismatch detection by ensuring these issues are surfaced even when auto-manage is disabled, improving troubleshooting visibility.

For testing infrastructure, the PR adds a 128MB safety cap for framed response lengths in socket tests to prevent memory exhaustion from corrupted protocol frames. Additionally, Python 3.11 is now explicitly pinned in the Pyright configuration to ensure consistent type checking across different development environments.

These changes integrate well with the existing codebase by preserving API compatibility while gradually encouraging migration to preferred methods. The validation level expansion fits naturally into the existing switch-based validation system in ManageScript.cs.

Important Files Changed

Files Modified (4 files)
Filename Score Overview
UnityMcpBridge/Editor/Tools/ManageScript.cs 4/5 Restores deprecated actions as compatibility aliases with warnings and improves CRLF newline handling
UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs 5/5 Adds IDE configuration mismatch detection when auto-manage is disabled
UnityMcpBridge/UnityMcpServer~/src/pyrightconfig.json 5/5 Pins Python 3.11 version for consistent type checking across environments
test_unity_socket_framing.py 4/5 Adds 128MB safety cap for framed response lengths to prevent memory issues

Confidence score: 4/5

  • This PR is relatively safe to merge with some areas requiring attention due to backward compatibility changes
  • Score reflects solid safety improvements and defensive programming, but complexity in ManageScript.cs changes requires careful review
  • Pay close attention to ManageScript.cs for potential edge cases in CRLF handling and deprecated action routing

Sequence Diagram

sequenceDiagram
    participant User
    participant MCP_Client as MCP Client
    participant Unity_Bridge as Unity Bridge
    participant ManageScript as ManageScript Handler
    participant ValidationEngine as Validation Engine
    participant FileSystem as File System
    participant EditorWindow as Editor Window

    User->>MCP_Client: "Request script operation"
    MCP_Client->>Unity_Bridge: "Send manage_script command"
    
    alt Framing Protocol Check
        Unity_Bridge->>Unity_Bridge: "Check response length limits"
        Note over Unity_Bridge: Cap framed response to prevent overflow
    end
    
    Unity_Bridge->>ManageScript: "HandleCommand(params)"
    ManageScript->>ManageScript: "Extract action, name, path, contents"
    ManageScript->>ManageScript: "Validate parameters"
    ManageScript->>ManageScript: "TryResolveUnderAssets(path)"
    
    alt Legacy Action Warning
        Note over ManageScript: If action is 'read', 'update', or 'edit'
        ManageScript->>ManageScript: "Log deprecation warning"
        ManageScript->>ManageScript: "Continue with backward compatibility"
    end
    
    alt Action: validate
        ManageScript->>ManageScript: "Parse validation level (basic/standard/strict/comprehensive)"
        ManageScript->>FileSystem: "Read script file"
        FileSystem-->>ManageScript: "File contents"
        ManageScript->>ValidationEngine: "ValidateScriptSyntax(contents, level)"
        
        alt Roslyn Available
            ValidationEngine->>ValidationEngine: "Use Roslyn compiler services"
        else Fallback
            ValidationEngine->>ValidationEngine: "Use basic structure validation"
        end
        
        ValidationEngine-->>ManageScript: "Validation results with diagnostics"
        ManageScript-->>Unity_Bridge: "Formatted diagnostics response"
    end
    
    alt Action: apply_text_edits
        ManageScript->>FileSystem: "Read current file"
        FileSystem-->>ManageScript: "Current contents"
        ManageScript->>ManageScript: "Check precondition SHA256"
        ManageScript->>ManageScript: "Convert line/col to indices (handle CRLF)"
        ManageScript->>ManageScript: "Validate edit ranges don't overlap"
        ManageScript->>ManageScript: "Apply edits from back to front"
        ManageScript->>ManageScript: "Check balanced delimiters"
        
        opt Roslyn Validation
            ManageScript->>ValidationEngine: "Parse and validate syntax"
            ValidationEngine-->>ManageScript: "Syntax check results"
        end
        
        ManageScript->>FileSystem: "Atomic write with temp file"
        ManageScript->>ManageScript: "Schedule asset refresh"
    end
    
    alt Action: create/update (legacy)
        ManageScript->>EditorWindow: "Get validation level from GUI settings"
        EditorWindow-->>ManageScript: "Current validation level"
        ManageScript->>ValidationEngine: "ValidateScriptSyntax(contents, level)"
        ValidationEngine-->>ManageScript: "Validation results"
        
        alt Validation Failed
            ManageScript-->>Unity_Bridge: "Error response with details"
        else Validation Passed
            ManageScript->>FileSystem: "Write script file atomically"
            ManageScript->>ManageScript: "Schedule debounced refresh"
            ManageScript-->>Unity_Bridge: "Success response"
        end
    end
    
    Unity_Bridge->>Unity_Bridge: "Apply framing safety check"
    Unity_Bridge-->>MCP_Client: "Return response (framed if protocol supports)"
    MCP_Client-->>User: "Operation result"
Loading

4 files reviewed, no comments

Edit Code Review Bot Settings | Greptile

@dsarno dsarno deleted the codex/update-managescript-for-backward-compatibility-g252rq branch August 19, 2025 09:57
dsarno pushed a commit that referenced this pull request Nov 29, 2025
* First pass at MCP client refactor

* Restore original text instructions

Well most of them, I modified a few

* Move configurators to their own folder

It's less clusterd

* Remvoe override for Windsurf because we no longer need to use it

* Add Antigravity configs

Works like Windsurf, but it sucks ass

* Add some docs for properties

* Add comprehensive MCP client configurators documentation

* Add missing imports (#7)

* Handle Linux paths when unregistering CLI commands

* Construct a JSON error in a much more secure fashion
dsarno pushed a commit that referenced this pull request Nov 29, 2025
* First pass at MCP client refactor

* Restore original text instructions

Well most of them, I modified a few

* Move configurators to their own folder

It's less clusterd

* Remvoe override for Windsurf because we no longer need to use it

* Add Antigravity configs

Works like Windsurf, but it sucks ass

* Add some docs for properties

* Add comprehensive MCP client configurators documentation

* Add missing imports (#7)

* Handle Linux paths when unregistering CLI commands

* Construct a JSON error in a much more secure fashion

* Fix stdio auto-reconnect after domain reloads

We mirror what we've done with the HTTP/websocket connection

We also ensure the states from the stdio/HTTP connections are handled separately. Things now work as expected

* Fix ActiveMode to return resolved transport mode instead of preferred mode

The ActiveMode property now calls ResolvePreferredMode() to return the actual active transport mode rather than just the preferred mode setting.

* Minor improvements for stdio bridge

- Consolidated the !useHttp && isRunning checks into a single shouldResume flag.
- Wrapped the fire-and-forget StopAsync in a continuation that logs faults (matching the HTTP handler pattern).
- Wrapped StartAsync in a continuation that logs failures and only triggers the health check on success.

* Refactor TransportManager to use switch expressions and improve error handling

- Replace if-else chains with switch expressions for better readability and exhaustiveness checking
- Add GetClient() helper method to centralize client retrieval logic
- Wrap StopAsync in try-catch to log failures when stopping a failed transport
- Use client.TransportName instead of mode.ToString() for consistent naming in error messages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants