-
Notifications
You must be signed in to change notification settings - Fork 0
Feat/nl edits ai planner prep #21
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
Conversation
…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
…xt ops; preview+confirm for regex; safe_script_edit wrapper; immediate compile & verification; header-safe anchors
…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.
…add SHA precondition and immediate refresh; keep verification slice; minor test and uv.lock updates
…cription with canonical fields & examples; alias/wrapper normalization; machine-parsable validation hints; auto applyMode=sequential for mixed insert+replace; echo normalizedEdits
…edit'; add routing='text' for pure text; echo normalizedEdits; C#: include 'code' in error payloads
…ontext access might be invalid' on forks
…enrich spec examples; add routing metadata; add standalone long test script
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 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 introduces comprehensive preparations for AI-powered natural language editing capabilities in the Unity MCP Bridge system. The changes establish a robust foundation for Claude AI and other language models to perform intelligent script editing operations within Unity projects.
The PR significantly enhances cross-platform support by adding proper macOS configuration paths for Claude Desktop (~/Library/Application Support/Claude/claude_desktop_config.json
), addressing a critical gap in platform compatibility. The system now properly detects and configures MCP clients across Windows, Linux, and macOS environments.
A major architectural shift introduces structured edit operations with anchor-based editing modes (anchor_insert
, anchor_delete
, anchor_replace
) that enable pattern-based code modifications. The script editing system now features auto-upgrade logic that intelligently routes text edits to structured operations when method-level changes are detected, along with mandatory SHA256 precondition validation to prevent edit conflicts and file corruption.
The server infrastructure gains MCP version compatibility guards and introduces a comprehensive script editing specification resource (unity://spec/script-edits
) that serves as documentation for AI clients. New resource wrapper tools provide MCP primitives for clients without native resource support, including natural language parsing capabilities for requests like "show me the last 50 lines around the Update method".
For testing and validation, the PR adds a comprehensive Claude AI test specification with structured categories (NL-0 through NL-4 for natural language tests, T-A through T-J for technical tests) and includes a large Unity test script (longUnityScript-claudeTest.cs
) with over 2000 lines designed specifically for AI editing validation. The test suite transitions from skip markers to xfail markers, making tests discoverable for AI-assisted development while maintaining CI stability.
Security enhancements include header guards preventing edits before 'using' directives, path safety measures with project boundary enforcement, thread synchronization for concurrent operations, and atomic file writing with BOM handling to prevent JSON parsing issues. The system also adds immediate vs debounced refresh modes for different editing scenarios and improved error handling with machine-parsable error codes.
Important Files Changed
Click to expand file changes
Filename | Score | Overview |
---|---|---|
UnityMcpBridge/Editor/Data/McpClients.cs |
5/5 | Added macOS configuration path support for Claude Desktop client |
tests/test_logging_stdout.py |
4/5 | Refactored stdout detection from regex to AST parsing for better accuracy |
ClaudeTests/longUnityScript-claudeTest.cs |
5/5 | Added comprehensive 2000+ line Unity test script for AI editing validation |
UnityMcpBridge/UnityMcpServer~/src/server.py |
4/5 | Enhanced server with MCP version compatibility and script editing specification |
UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py |
4/5 | Added resource tools registration for broader IDE compatibility |
UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs |
4/5 | Improved configuration system with BOM handling and OS-specific paths |
.claude/prompts/nl-unity-suite.md |
4/5 | Added comprehensive test specification for Claude AI natural language editing |
tests/test_script_editing.py |
5/5 | Changed test markers from skip to xfail for AI-assisted development |
UnityMcpBridge/Editor/Helpers/Response.cs |
4/5 | Added machine-parsable error codes while maintaining backward compatibility |
UnityMcpBridge/Editor/Tools/ManageScript.cs |
4/5 | Major enhancement with anchor-based editing modes and validation improvements |
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py |
3/5 | New resource wrapper tools with natural language parsing capabilities |
tests/test_transport_framing.py |
4/5 | Improved test robustness with better timeout handling and byte reading |
UnityMcpBridge/Editor/Tools/ManageEditor.cs |
4/5 | Added get_project_root command for AI clients to understand project structure |
tests/test_resources_api.py |
5/5 | Converted test placeholders from skip to xfail for AI development readiness |
.github/workflows/claude-nl-suite.yml |
2/5 | Added GitHub Actions workflow for Claude AI integration with path and security concerns |
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py |
2/5 | Major refactor removing NL processing and adding complex edit routing logic |
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py |
4/5 | Enhanced connection handling with thread safety and protocol flexibility |
UnityMcpBridge/Editor/Models/McpClient.cs |
4/5 | Added macConfigPath field for proper macOS client configuration support |
Confidence score: 3/5
- This PR introduces significant architectural changes for AI-powered editing that require careful review due to the complexity and scope of modifications
- Score reflects concerns about hardcoded paths in GitHub workflow, complex edit routing logic that could be brittle, and potential security implications of natural language parsing
- Pay close attention to
.github/workflows/claude-nl-suite.yml
,UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py
, andUnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py
for path validation and security considerations
Sequence Diagram
sequenceDiagram
participant User
participant GitHub as "GitHub Actions"
participant Claude as "Claude AI"
participant UnityMCP as "Unity MCP Server"
participant Unity as "Unity Editor"
participant TestFile as "longUnityScript-claudeTest.cs"
User->>GitHub: "Trigger workflow_dispatch"
GitHub->>GitHub: "Checkout repository"
GitHub->>GitHub: "Install Python + uv dependencies"
GitHub->>Claude: "Initialize Claude with MCP tools"
Claude->>UnityMCP: "Connect to Unity MCP Server"
UnityMCP->>Unity: "Establish bridge connection"
Note over Claude: Begin NL-0 through T-J test suite
Claude->>TestFile: "Read last 120 lines (NL-0)"
TestFile-->>Claude: "File content window"
Claude->>TestFile: "Show 40 lines around Update method"
TestFile-->>Claude: "Method context window"
Claude->>UnityMCP: "script_apply_edits: replace HasTarget method"
UnityMCP->>Unity: "manage_script with structured edits"
Unity->>TestFile: "Replace method with block-bodied version"
Unity-->>UnityMCP: "Success response"
UnityMCP-->>Claude: "Edit confirmation"
Claude->>UnityMCP: "script_apply_edits: insert PrintSeries method"
UnityMCP->>Unity: "manage_script with insert_method op"
Unity->>TestFile: "Insert new method after GetCurrentTarget"
Unity-->>UnityMCP: "Success response"
UnityMCP-->>Claude: "Insert confirmation"
Claude->>TestFile: "Verify inserted method exists"
TestFile-->>Claude: "Method verification"
Claude->>UnityMCP: "script_apply_edits: delete PrintSeries method"
UnityMCP->>Unity: "manage_script with delete_method op"
Unity->>TestFile: "Remove PrintSeries method"
Unity-->>UnityMCP: "Success response"
UnityMCP-->>Claude: "Delete confirmation"
Claude->>UnityMCP: "apply_text_edits with precondition_sha256"
UnityMCP->>Unity: "Atomic text edits with validation"
Unity->>TestFile: "Apply precise line/column edits"
Unity->>Unity: "Validate syntax and balance"
Unity-->>UnityMCP: "Success with new SHA256"
UnityMCP-->>Claude: "Edit applied successfully"
Claude->>Claude: "Generate JUnit XML report"
Claude->>GitHub: "Write reports/claude-nl-tests.xml"
Claude->>GitHub: "Write reports/claude-nl-tests.md"
GitHub->>GitHub: "Upload JUnit artifacts"
GitHub->>GitHub: "Annotate PR with test results"
opt Optional Unity Compilation
GitHub->>Unity: "Run Unity test runner for compilation"
Unity-->>GitHub: "Compilation results"
end
GitHub->>GitHub: "Clean working tree (discard temp edits)"
GitHub-->>User: "Test results and annotations"
18 files reviewed, 13 comments
return {"mimeType": "text/plain", "text": text, "metadata": {"sha256": sha}} | ||
except Exception as e: | ||
return {"mimeType": "text/plain", "text": f"Error reading resource: {e}"} | ||
if hasattr(mcp, "resource") and hasattr(getattr(mcp, "resource"), "list"): |
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.
logic: The nested hasattr check on getattr(mcp, "resource")
could potentially raise AttributeError if mcp.resource doesn't exist
}) | ||
return assets | ||
|
||
if hasattr(mcp, "resource") and hasattr(getattr(mcp, "resource"), "read"): |
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.
logic: Same potential AttributeError issue with nested hasattr check
/// <param name="data">Optional additional data (e.g., error details) to include.</param> | ||
/// <returns>An object representing the error response.</returns> | ||
public static object Error(string errorMessage, object data = null) | ||
public static object Error(string errorCodeOrMessage, object data = null) |
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.
syntax: Parameter name changed from errorMessage
to errorCodeOrMessage
but XML documentation still references 'errorMessage'
public static object Error(string errorCodeOrMessage, object data = null) | |
/// <param name="errorCodeOrMessage">A message describing the error.</param> |
if (string.IsNullOrEmpty(preconditionSha256)) | ||
return Response.Error("precondition_required", new { status = "precondition_required", current_sha256 = currentSha }); |
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.
logic: Breaking change: precondition SHA256 is now mandatory for all text edits. This will break existing clients that don't provide preconditions.
// Attempt auto-upgrade: if a single edit targets a method header/body, re-route as structured replace_method | ||
if (spans.Count == 1) | ||
{ | ||
var sp = spans[0]; | ||
// Heuristic: around the start of the edit, try to match a method header in original | ||
int searchStart = Math.Max(0, sp.start - 200); | ||
int searchEnd = Math.Min(original.Length, sp.start + 200); | ||
string slice = original.Substring(searchStart, searchEnd - searchStart); | ||
var rx = new System.Text.RegularExpressions.Regex(@"(?m)^[\t ]*(?:\[[^\]]+\][\t ]*)*[\t ]*(?:public|private|protected|internal|static|virtual|override|sealed|async|extern|unsafe|new|partial)[\s\S]*?\b([A-Za-z_][A-Za-z0-9_]*)\s*\("); | ||
var mh = rx.Match(slice); | ||
if (mh.Success) | ||
{ | ||
string methodName = mh.Groups[1].Value; | ||
// Find class span containing the edit | ||
if (TryComputeClassSpan(original, name, null, out var clsStart, out var clsLen, out _)) | ||
{ | ||
if (TryComputeMethodSpan(original, clsStart, clsLen, methodName, null, null, null, out var mStart, out var mLen, out _)) | ||
{ | ||
// If the edit overlaps the method span significantly, treat as replace_method | ||
if (sp.start <= mStart + 2 && sp.end >= mStart + 1) | ||
{ | ||
var structEdits = new JArray(); | ||
var op = new JObject | ||
{ | ||
["mode"] = "replace_method", | ||
["className"] = name, | ||
["methodName"] = methodName, | ||
["replacement"] = original.Remove(sp.start, sp.end - sp.start).Insert(sp.start, sp.text ?? string.Empty).Substring(mStart, (sp.text ?? string.Empty).Length + (sp.start - mStart) + (mLen - (sp.end - mStart))) | ||
}; | ||
structEdits.Add(op); | ||
// Reuse structured path | ||
return EditScript(fullPath, relativePath, name, structEdits, new JObject{ ["refresh"] = "immediate", ["validate"] = "standard" }); | ||
} | ||
} | ||
} | ||
} | ||
} |
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.
logic: Complex heuristic logic for auto-upgrading text edits to structured edits. The method detection regex and span overlap logic could misclassify legitimate edits as method replacements.
deadline = time.time() + 0.5 | ||
while time.time() < deadline: | ||
r, _, _ = select.select([conn], [], [], 0.05) | ||
if r: | ||
conn.close() | ||
sock.close() | ||
return |
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.
style: Consider using conn.settimeout()
instead of manual deadline checking for cleaner timeout handling
if [ -f UnityMcpServer/requirements.txt ]; then | ||
uv pip install -r UnityMcpServer/requirements.txt | ||
fi |
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.
logic: Path 'UnityMcpServer/requirements.txt' doesn't exist in repository. Should likely be 'UnityMcpBridge/UnityMcpServer~/requirements.txt' or similar
STRUCT = {"replace_class","delete_class","replace_method","delete_method","insert_method","anchor_insert","anchor_delete","anchor_replace"} | ||
TEXT = {"prepend","append","replace_range","regex_replace","anchor_insert"} |
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.
logic: anchor_insert
appears in both STRUCT and TEXT sets, which could cause inconsistent routing behavior
"command": "python", | ||
"args": ["UnityMcpServer/src/server.py"] |
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.
logic: Path 'UnityMcpServer/src/server.py' is invalid. The actual server path appears to be different based on repository structure
# If we have a mixed batch (TEXT + STRUCT), apply text first with precondition, then structured | ||
if mixed: | ||
text_edits = [e for e in edits or [] if (e.get("op") or "").lower() in TEXT] | ||
struct_edits = [e for e in edits or [] if (e.get("op") or "").lower() in STRUCT and (e.get("op") or "").lower() not in {"anchor_insert"}] |
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.
logic: Logic excludes anchor_insert
from struct_edits but it's defined as a structured operation above - potential inconsistency
@greptileai could you give me some advice about splitting this into three different PRS – – one for framing, one for the MCP configuration improvements, and one for the script editing features |
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.
Looking at this large PR, I can definitely help you split it into three focused PRs. Here's my recommended breakdown:
PR #1: Transport Framing Improvements
Focus: Socket communication reliability and framing protocol enhancements
Files to include:
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py
(the threading locks, framing validation, timeout improvements)tests/test_transport_framing.py
(the test infrastructure improvements)tests/test_logging_stdout.py
(AST-based print statement detection - this is infrastructure)
Key changes:
- Thread-safe socket operations with
_io_lock
- Improved framing protocol validation
- Better timeout handling during retries
- Enhanced error handling for socket operations
PR #2: MCP Configuration & Client Management
Focus: Cross-platform config handling, Claude Desktop support, and UI improvements
Files to include:
UnityMcpBridge/Editor/Data/McpClients.cs
(macOS config path addition)UnityMcpBridge/Editor/Models/McpClient.cs
(macConfigPath field)UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs
(BOM handling, atomic writes, cross-platform paths)UnityMcpBridge/Editor/Helpers/Response.cs
(error code improvements)UnityMcpBridge/Editor/Tools/ManageEditor.cs
(get_project_root action)
Key changes:
- Proper macOS support for Claude Desktop configuration
- UTF-8 BOM removal and atomic config writing
- Better error reporting with machine-readable codes
- Cross-platform path handling improvements
PR #3: Script Editing Features & NL Testing Infrastructure
Focus: Advanced script editing capabilities and the new testing framework
Files to include:
.claude/prompts/nl-unity-suite.md
(the comprehensive test specification).github/workflows/claude-nl-suite.yml
(CI workflow for NL testing)ClaudeTests/longUnityScript-claudeTest.cs
(test target file)UnityMcpBridge/Editor/Tools/ManageScript.cs
(all the advanced editing features)UnityMcpBridge/UnityMcpServer~/src/server.py
(spec resource serving)UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py
(resource tools registration)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py
(major overhaul)UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py
(new file)UnityMcpBridge/UnityMcpServer~/src/uv.lock
(version bump)tests/test_resources_api.py
,tests/test_script_editing.py
(test placeholders)
Key changes:
- Structured script editing with
replace_method
,insert_method
, etc. - Header guards and validation improvements
- Auto-upgrade from text edits to structured edits
- Comprehensive alias handling for field names
- New resource tools for clients without MCP resources support
- Natural language testing framework with JUnit output
Migration Strategy:
- Start with PR #1 (Framing) - This is the foundation and has the least dependencies
- Follow with PR #2 (Configuration) - This builds on the improved error handling from PR #1
- Finish with PR #3 (Script Editing) - This is the largest and benefits from the stability improvements in the previous PRs
Benefits of This Split:
- Easier review: Each PR has a clear, focused scope
- Safer rollback: If issues arise, you can revert individual features
- Parallel development: Different aspects can be worked on independently
- Better testing: Each PR can be thoroughly tested in isolation
- Clearer history: Git history will show logical progression of features
The current PR is quite ambitious - splitting it this way will make it much more manageable for both development and review!
@greptileai which files will have most conflicts / dependencies to untangle ? |
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.
Looking at this large diff, the files with the most conflicts and dependencies to untangle are:
1. UnityMcpBridge/Editor/Tools/ManageScript.cs (Most Complex)
- Massive changes: 200+ lines of new code added
- Complex dependencies:
- New header guard logic
- Auto-upgrade detection for method edits
- New anchor operations (anchor_insert, anchor_delete, anchor_replace)
- Parameter signature parsing changes
- Refresh mode handling (immediate vs debounced)
- Precondition requirement enforcement
- Breaking changes: Now requires
precondition_sha256
for all text edits - Cross-cutting concerns: Affects validation, compilation pipeline, and editor refresh
2. UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (High Complexity)
- Complete rewrite: 400+ lines of new normalization logic
- Complex aliasing system: Handles multiple field name variations
- Routing logic: Decides between structured/text/mixed edit paths
- Breaking changes: Removes natural language parsing entirely
- Dependencies: Tightly coupled with Unity's ManageScript.cs changes
3. UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs (Medium-High)
- File I/O changes: New atomic write logic with BOM handling
- Platform-specific paths: macOS vs Linux config path logic
- JSON validation: New verification steps
- Command line changes: Added
-q
flag that could break existing workflows
4. UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (Medium)
- Threading: New I/O lock for socket operations
- Protocol changes: Enhanced framing negotiation
- Error handling: New timeout and connection logic
- Backward compatibility: Optional framing requirement
Conflict Risk Assessment:
Highest Risk:
- ManageScript.cs ↔ manage_script_edits.py (tightly coupled protocol changes)
- Any existing code calling script editing APIs (breaking
precondition_sha256
requirement)
Medium Risk:
- UnityMcpEditorWindow.cs config writing vs existing config readers
- unity_connection.py framing changes vs existing clients
- New Response.cs error format vs existing error handlers
Dependencies to Untangle:
- Script editing protocol: The C# and Python sides must be deployed together
- Config file format: BOM removal might break existing parsers
- MCP framing: Connection negotiation changes affect all tool communication
- Error response format: New
code
field might break existing error handling
Recommendation: Deploy the script editing changes (ManageScript.cs + manage_script_edits.py) as a single atomic unit, and test the config file changes thoroughly on macOS systems.
…ed-named-value-errors
…ed-named-value-errors-1zr4mk
|
||
|
||
def main() -> None: | ||
print("DEBUG: mcp_source main invoked") |
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.
No description provided.