Align script management responses and validation #12
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 makes several important improvements to the Unity MCP script management system to align with CLI standards and fix critical validation bugs. The changes focus on four main areas:
-
Validation Level Standardization: Changes the default validation level from 'basic' to 'standard' to match CLI tool behavior, ensuring consistency across different interfaces.
-
URI Response Enhancement: Adds
unity://URI format to script read and update responses, providing proper resource identification that external tools can use to reference Unity scripts. -
Roslyn Validation Fix: Corrects a critical bug in the Roslyn validation logic where syntax errors could be ignored. The fix ensures validation only runs for Standard+ levels and properly fails when compilation errors are detected.
-
Refresh Debounce Improvement: Enhances the refresh debounce system to collect all affected paths from multiple rapid changes and ensures that new calls properly schedule asset imports, preventing missed updates during bulk operations.
These changes integrate well with the existing Unity MCP architecture, which serves as a bridge between Unity and Large Language Models via the Model Context Protocol. The script management functionality is a core component that allows AI tools to read, create, and modify Unity C# scripts, so improving its reliability and consistency is crucial for the overall system's effectiveness.
Important Files Changed
Files Modified
| Filename | Score | Overview |
|---|---|---|
| UnityMcpBridge/Editor/Tools/ManageScript.cs | 4/5 | Fixed critical Roslyn validation logic, added URI responses, changed default validation level, and improved refresh debounce system |
Confidence score: 4/5
- This PR is safe to merge with good confidence due to targeted fixes for known issues
- Score reflects solid bug fixes and consistency improvements, though the validation logic change affects core functionality
- Pay close attention to the Roslyn validation changes to ensure error handling works as expected
Sequence Diagram
sequenceDiagram
participant User
participant MCP as "MCP Server"
participant ManageScript as "ManageScript"
participant ValidateScriptSyntax as "ValidateScriptSyntax"
participant Roslyn as "Roslyn Compiler"
participant FileSystem as "File System"
participant AssetDatabase as "AssetDatabase"
participant RefreshDebounce as "RefreshDebounce"
User->>MCP: "manage_script request"
MCP->>ManageScript: "HandleCommand(params)"
alt action: create
ManageScript->>ManageScript: "TryResolveUnderAssets(path)"
ManageScript->>FileSystem: "Check if file exists"
FileSystem-->>ManageScript: "File not exists"
ManageScript->>ManageScript: "GenerateDefaultScriptContent()"
ManageScript->>ManageScript: "GetValidationLevelFromGUI()"
ManageScript->>ValidateScriptSyntax: "ValidateScriptSyntax(contents, level)"
ValidateScriptSyntax->>Roslyn: "Parse and analyze syntax"
Roslyn-->>ValidateScriptSyntax: "Diagnostics"
ValidateScriptSyntax-->>ManageScript: "isValid, errors"
alt validation fails
ManageScript-->>MCP: "Error response with validation errors"
else validation succeeds
ManageScript->>FileSystem: "Write file atomically"
ManageScript->>RefreshDebounce: "ScheduleScriptRefresh(relativePath)"
ManageScript-->>MCP: "Success response with unity:// URI"
end
else action: update
ManageScript->>FileSystem: "Check if file exists"
FileSystem-->>ManageScript: "File exists"
ManageScript->>ValidateScriptSyntax: "ValidateScriptSyntax(contents, level)"
ValidateScriptSyntax->>Roslyn: "Parse and analyze syntax"
Roslyn-->>ValidateScriptSyntax: "Diagnostics"
ValidateScriptSyntax-->>ManageScript: "isValid, errors"
alt validation fails
ManageScript-->>MCP: "Error response with validation errors"
else validation succeeds
ManageScript->>FileSystem: "Atomic file replace"
ManageScript->>RefreshDebounce: "ScheduleScriptRefresh(relativePath)"
ManageScript-->>MCP: "Success response with unity:// URI"
end
else action: apply_text_edits
ManageScript->>FileSystem: "Read original file"
FileSystem-->>ManageScript: "File contents"
ManageScript->>ManageScript: "Verify precondition SHA256"
ManageScript->>ManageScript: "Apply edits in reverse order"
ManageScript->>ManageScript: "CheckBalancedDelimiters()"
ManageScript->>Roslyn: "Parse and validate syntax"
Roslyn-->>ManageScript: "Syntax diagnostics"
alt syntax errors
ManageScript-->>MCP: "Error response with diagnostics"
else syntax valid
ManageScript->>FileSystem: "Atomic file write"
ManageScript->>RefreshDebounce: "ScheduleScriptRefresh(relativePath)"
ManageScript-->>MCP: "Success response with SHA256"
end
else action: validate
ManageScript->>FileSystem: "Read file"
FileSystem-->>ManageScript: "File contents"
ManageScript->>ValidateScriptSyntax: "ValidateScriptSyntax(contents, standard)"
ValidateScriptSyntax->>Roslyn: "Parse and analyze"
Roslyn-->>ValidateScriptSyntax: "Diagnostics"
ValidateScriptSyntax-->>ManageScript: "isValid, diagnostics"
ManageScript-->>MCP: "Response with diagnostic details"
else action: read
ManageScript->>FileSystem: "Read file"
FileSystem-->>ManageScript: "File contents"
ManageScript-->>MCP: "Success response with unity:// URI and contents"
else action: delete
ManageScript->>AssetDatabase: "MoveAssetToTrash(relativePath)"
AssetDatabase-->>ManageScript: "Success/failure"
AssetDatabase->>AssetDatabase: "Refresh()"
ManageScript-->>MCP: "Success response"
end
MCP-->>User: "Response"
Note over RefreshDebounce: "Debounced refresh triggers after 200ms"
RefreshDebounce->>AssetDatabase: "ImportAsset() for all pending paths"
RefreshDebounce->>AssetDatabase: "RequestScriptCompilation()"
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.
| string[] toImport; | ||
| lock (_lock) { toImport = _paths.ToArray(); _paths.Clear(); } | ||
| foreach (var p in toImport) | ||
| AssetDatabase.ImportAsset(p, ImportAssetOptions.ForceUpdate); |
There was a problem hiding this comment.
Bug: Race Condition in RefreshDebounce Method
The RefreshDebounce.Schedule method has a race condition where the _scheduled flag is accessed and modified without proper synchronization. This allows multiple threads to simultaneously pass the scheduling check, leading to redundant EditorApplication.delayCall callbacks being scheduled unnecessarily, which creates inefficiency.
1 participant
Summary
validatelevel to standard for CLI parityunity://URI in script read/update resultsTesting
pytest -qhttps://chatgpt.com/codex/tasks/task_e_68a1eac13eac83279d6f077413ceabce