Align script management responses and validation #13
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 enhances the script management system in Unity MCP Bridge by addressing several operational issues and improving robustness. The changes focus on aligning the system with CLI tooling expectations and Unity's asset management best practices.
The most significant change involves fixing the Roslyn validation logic, which was previously allowing validation errors to pass through at the Standard level - this was a known issue marked with a TODO comment that has now been formally addressed. The validation now properly gates on Standard+ level and fails operations when compilation errors are detected.
Other key improvements include: changing the default validation level from 'basic' to 'standard' to match CLI parity expectations, adding unity:// URIs to script read/update responses for better resource identification by external systems, implementing comprehensive backup file cleanup to prevent Unity from importing temporary .bak files as assets, and adding symlink guards to prevent editing of symlinked files which could cause security issues or file system confusion.
The refresh debounce system has been enhanced to handle multiple file operations more efficiently by coalescing paths and ensuring proper import scheduling. Additionally, immediate refresh operations are now moved to Unity's main thread using EditorApplication.delayCall to prevent threading violations that could cause editor instability.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| UnityMcpBridge/Editor/Tools/ManageScript.cs | 4/5 | Comprehensive script management improvements including validation fixes, backup cleanup, symlink guards, and thread-safe refresh operations |
Confidence score: 4/5
- This PR addresses critical bugs and improves system reliability with well-targeted fixes
- Score reflects solid improvements to validation logic and asset management, though the extensive changes require careful testing
- Pay close attention to the Roslyn validation changes and refresh debounce logic modifications
Sequence Diagram
sequenceDiagram
participant User
participant HandleCommand
participant ValidateScriptSyntax
participant Roslyn as Roslyn Compiler
participant FileSystem
participant RefreshDebounce
participant Unity as Unity Editor
User->>HandleCommand: "Script management request"
HandleCommand->>HandleCommand: "Extract and validate parameters"
alt create/update action
HandleCommand->>ValidateScriptSyntax: "Validate with Standard+ level"
ValidateScriptSyntax->>ValidateScriptSyntax: "Basic structure validation"
alt USE_ROSLYN defined and Standard+ level
ValidateScriptSyntax->>Roslyn: "Parse syntax tree and get diagnostics"
Roslyn-->>ValidateScriptSyntax: "Return errors/warnings"
alt has errors
ValidateScriptSyntax-->>HandleCommand: "Validation failed"
HandleCommand-->>User: "Error response with diagnostics"
end
end
ValidateScriptSyntax-->>HandleCommand: "Validation passed"
HandleCommand->>FileSystem: "Atomic write (.tmp → .bak → final)"
FileSystem->>FileSystem: "Clean up .bak files"
HandleCommand->>RefreshDebounce: "Schedule script refresh"
HandleCommand-->>User: "Success with unity:// URI"
RefreshDebounce->>Unity: "Debounced asset import and compilation"
end
alt apply_text_edits action
HandleCommand->>FileSystem: "Read current file and compute SHA256"
alt precondition check fails
HandleCommand-->>User: "stale_file error with SHA mismatch"
end
HandleCommand->>HandleCommand: "Apply edits with overlap validation"
alt USE_ROSLYN defined
HandleCommand->>Roslyn: "Parse and validate result"
alt syntax errors found
HandleCommand-->>User: "syntax_error with diagnostics"
end
HandleCommand->>Roslyn: "Optional formatting"
end
HandleCommand->>FileSystem: "Atomic write with backup cleanup"
HandleCommand->>RefreshDebounce: "Schedule refresh"
HandleCommand-->>User: "Success with applied count and new SHA256"
end
alt validate action
HandleCommand->>ValidateScriptSyntax: "Validate with specified level (default: standard)"
ValidateScriptSyntax-->>HandleCommand: "Diagnostics array"
HandleCommand-->>User: "Validation result with structured diagnostics"
end
1 file reviewed, no comments
![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 ((now - _last) < window) return; | ||
| if (_scheduled && (now - _last) < window) return; | ||
| _last = now; | ||
| _scheduled = true; |
There was a problem hiding this comment.
Bug: Debouncing Logic Fails to Prevent Race Conditions
The RefreshDebounce.Schedule method's debouncing logic is flawed. The _scheduled flag isn't synchronized, which can lead to race conditions and duplicate scheduling. Also, the _scheduled && (now - _last) < window condition allows new refreshes to be scheduled immediately after a debounced call completes, bypassing the time window and defeating the debouncing.
Summary
validatelevel to standard for CLI parityunity://URI in script read/update results.bakbackups, guard against symlinked edits, and run immediate refreshes on the main threadTesting
pytest -qhttps://chatgpt.com/codex/tasks/task_e_68a1eac13eac83279d6f077413ceabce