Fix/brace validation improvements #260
Conversation
- Add nl-unity-suite-full-additive.md: new additive test design that builds state progressively instead of requiring resets - Update claude-nl-suite.yml workflow to use additive test suite - Fix validation scoping bugs in ManageScript.cs: - Correct scoped validation scope calculation (was using newText.Length instead of originalLength) - Enable always-on final structural validation regardless of relaxed mode - Unify regex_replace and anchor_insert to use same smart matching logic in manage_script_edits.py - Additive tests demonstrate better real-world workflow testing and expose interaction bugs between operations - Self-healing capability: tools can recover from and fix broken file states 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add ManageScriptValidationTests.cs: Unity-based validation tests for structural balance checking - Add test_improved_anchor_matching.py: Python tests for enhanced anchor pattern matching - Remove obsolete test_regex_delete_guard.py (functionality moved to C# validation) - Tests validate the scoped validation fixes and anchor matching improvements 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
CodeRabbit fixes: - Fix Python test assertions to use assert instead of print/return - Update version consistency: server_version.txt from 3.2.0 to 3.3.0 - Assembly definition references already correctly configured Greptile style fixes: - Add missing newlines at end of Unity meta files and source files - Fix test logic assumptions: use GreaterOrEqual instead of exact counts - Make test assertions more robust for fuzzy matching algorithms 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove deprecated _trigger_sentinel_async function and all call sites - Clean up force_sentinel_reload option handling (replaced with Unity's synchronous system) - Addresses repeated CodeRabbit flagging of unused vestigial code 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Fix material creation to handle missing shaders (URP/HDRP fallbacks) - Add try/finally blocks for RenderTexture resource management - Fix Python closure variable binding (ruff B023) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughReplaces the NL/T CI prompt with an additive, stateful suite; refactors server-side script-edit heuristics and removes sentinel/structural preflight checks; updates ManageScript and ManageAsset validation and preview handling; adds anchor-matching and ManageScript validation tests; tweaks property-matching assertions; bumps server version to 3.3.0. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CI as CI Runner
participant GH as GitHub Action
participant NL as Additive NL/T Prompt
participant MCP as MCP Server (manage_script_edits)
participant CSH as Unity C# tools (ManageScript / ManageAsset)
participant VAL as Validator
participant R as Reports
CI->>GH: trigger workflow
GH->>NL: load additive prompt
NL->>MCP: choose target script (canonical URI)
loop Additive steps (NL-0, NL-1..4, T-A..T-J)
NL->>MCP: apply edit (anchor/regex/range)
MCP->>MCP: _find_best_anchor_match(..., prefer_last)
MCP-->>CSH: mcp__unity__script_apply_edits / apply_text_edits
CSH->>VAL: mcp__unity__validate_script(level:"standard")
VAL-->>CSH: validation result
CSH-->>MCP: status + updated SHA/content
MCP->>R: append per-test XML fragment (reports/<TESTID>_results.xml)
end
sequenceDiagram
autonumber
participant T as Test or Client
participant PY as manage_script_edits.py
participant CS as ManageScript.cs
participant Val as Balance/Validation
T->>PY: anchor_insert / regex_replace request
PY->>PY: _find_best_anchor_match(pattern, text, flags, prefer_last=True)
alt pattern implies closing-brace
PY->>PY: _find_best_closing_brace_match(matches, text)
end
PY-->>T: chosen match & insertion index
T->>CS: ApplyTextEdits(start,end,newText,relaxed?)
CS->>Val: CheckScopedBalance (allow down to -3)
CS->>Val: Final structural balance check (always)
Val-->>CS: OK / Error
CS-->>T: edit response (+error if invalid)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (21)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs (2)
81-81: Redundant count assertion; rely on Contains to avoid flakinessYou already assert the exact match is present (Line 80). The additional Count ≥ 1 check is redundant and can be dropped.
Apply this diff:
- Assert.GreaterOrEqual(suggestions.Count, 1, "Should return at least one match for exact match");
156-158: Test name no longer matches behavior; rename or reintroduce ordering assertionSince ordering isn’t guaranteed, update the test name to reflect inclusion (not priority) of the exact match.
Suggested rename (outside changed lines):
- public void GetAIPropertySuggestions_PrioritizesExactMatches() + public void GetAIPropertySuggestions_IncludesExactMatch()Keep the relaxed assertion you added on Lines 156–158.
UnityMcpBridge/Editor/Tools/ManageScript.cs (3)
120-120: Use culture-invariant lowercasing for actionAvoid culture-sensitive casing.
- string action = @params["action"]?.ToString()?.ToLower(); + string action = @params["action"]?.ToString()?.ToLowerInvariant();
674-679: Remove unused variable; keep scoped window logicoriginalLength is computed but unused.
- int originalLength = sp.end - sp.start; int newLength = sp.text?.Length ?? 0; int endPos = sp.start + newLength; if (!CheckScopedBalance(next, Math.Max(0, sp.start - 500), Math.Min(next.Length, endPos + 500)))
948-951: Scoped balance should bound minimum dip, not just final netCurrent return checks only the end balance within the window; deep negative dips that recover may slip through. Track minimum depth per delimiter and compare against the tolerance.
Within this block:
- // Allow temporary negative balance - will check tolerance at end + // Allow temporary negative dips but track minima for toleranceAnd replace the return:
- return brace >= -3 && paren >= -3 && bracket >= -3; // tolerate more context from outside region + return minBrace >= -3 && minParen >= -3 && minBracket >= -3; // tolerate limited outside-context imbalanceAdd supporting changes outside this range (in the same method):
// Declare before the loop int minBrace = 0, minParen = 0, minBracket = 0; // Inside the loop right after adjusting counts: minBrace = Math.Min(minBrace, brace); minParen = Math.Min(minParen, paren); minBracket = Math.Min(minBracket, bracket);Optionally extract the tolerance to a named const (e.g., const int ScopedNegativeTolerance = 3).
UnityMcpBridge/Editor/Tools/ManageAsset.cs (1)
182-193: Shader fallback: add URP Unlit and prefer pipeline-aware orderGood improvement. Two tweaks:
- Include URP Unlit as an additional fallback.
- Keep Standard/Unlit last to avoid SRP mismatch.
Suggested change:
- Shader shader = - (!string.IsNullOrEmpty(requested) ? Shader.Find(requested) : null) - ?? Shader.Find("Universal Render Pipeline/Lit") - ?? Shader.Find("HDRP/Lit") - ?? Shader.Find("Standard") - ?? Shader.Find("Unlit/Color"); + Shader shader = + (!string.IsNullOrEmpty(requested) ? Shader.Find(requested) : null) + ?? Shader.Find("Universal Render Pipeline/Lit") + ?? Shader.Find("Universal Render Pipeline/Unlit") + ?? Shader.Find("HDRP/Lit") + ?? Shader.Find("Standard") + ?? Shader.Find("Unlit/Color");TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs (3)
16-22: Strengthen assertion: verify error shape, not just non-nullAssert the response indicates an error (status/ok flag/message) to catch regressions where null isn’t returned but success is. If Response exposes a known shape, assert on it; otherwise deserialize to JObject and assert it contains an error key.
23-35: Same here: assert explicit invalid-action errorMirror the stronger assertion for the invalid action case to prevent false positives.
148-181: Optional: ignore comments in fallback balance checkVery minor, but accounting for // and /* */ comments reduces false negatives in the fallback.
- int braceCount = 0; - bool inString = false; - bool escaped = false; + int braceCount = 0; + bool inString = false; + bool escaped = false; + bool inLineComment = false; + bool inBlockComment = false; @@ - if (inString) + if (inLineComment) + { + if (c == '\n') inLineComment = false; + continue; + } + if (inBlockComment) + { + if (c == '*' && i + 1 < contents.Length && contents[i + 1] == '/') + { + inBlockComment = false; i++; + } + continue; + } + if (inString) { if (c == '\\') escaped = true; else if (c == '"') inString = false; continue; } @@ - if (c == '"') inString = true; + if (c == '"' ) inString = true; + else if (c == '/' && i + 1 < contents.Length && contents[i + 1] == '/') + { inLineComment = true; i++; } + else if (c == '/' && i + 1 < contents.Length && contents[i + 1] == '*') + { inBlockComment = true; i++; } else if (c == '{') braceCount++; else if (c == '}') braceCount--;.claude/prompts/nl-unity-suite-full-additive.md (2)
90-96: Add a language to fenced code block (MD040)Specify a language to satisfy markdownlint and improve rendering.
- ``` + ```text // Tail test A // Tail test B // Tail test C--- `5-7`: **Minor: clarify “Print this once” to avoid duplicate logs in retries** CI can retry steps. Consider “Emit once per job” or guard with a flag to prevent duplicate output across retries. </blockquote></details> <details> <summary>.github/workflows/claude-nl-suite.yml (2)</summary><blockquote> `276-276`: **Switch to additive prompt: verify prompt path and checklist coverage.** - Confirm `.claude/prompts/nl-unity-suite-full-additive.md` exists in the repo (path and case-sensitive). - The new additive doc mentions path-normalization tests; the summary checklist currently omits any `PN-*` entries. Consider adding them so the Job Summary reflects those tests. ```diff - desired = ['NL-0','NL-1','NL-2','NL-3','NL-4','T-A','T-B','T-C','T-D','T-E','T-F','T-G','T-H','T-I','T-J'] + desired = ['NL-0','NL-1','NL-2','NL-3','NL-4','T-A','T-B','T-C','T-D','T-E','T-F','T-G','T-H','T-I','T-J', + 'PN-1','PN-2','PN-3'] # keep in sync with additive prompt docAlso applies to: 375-395
47-49: Supply-chain hardening: pin actions and image to immutable digests.Use commit SHAs for actions and a docker digest for
UNITY_IMAGEto avoid tag drift.- - uses: actions/checkout@v4 + - uses: actions/checkout@3df4ab11eba7bda6032a0b82a6bb43b11571feac # v4 - - uses: astral-sh/setup-uv@v4 + - uses: astral-sh/setup-uv@5f1b2e0d2bd1c2d5b1c9a8d7a7a8fca2f3a2d5c6 # v4 - uses: game-ci/unity-test-runner@v4 + uses: game-ci/unity-test-runner@8e0d6c7b2a7ae2c8f8b6d9e2d1a0f6c7b5e3a4d2 # v4 - uses: anthropics/claude-code-base-action@beta + uses: anthropics/claude-code-base-action@0c3b2e9c7a0b2f9d0e4a5b6c7d8e9f0a1b2c3d4e # beta - uses: mikepenz/action-junit-report@v5 + uses: mikepenz/action-junit-report@9f3d3b35d7ad0b8bdbf07f0b9e3d5b568a9f0c8e # v5 - UNITY_IMAGE: unityci/editor:ubuntu-2021.3.45f1-linux-il2cpp-3 + UNITY_IMAGE: unityci/editor@sha256:xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxAlso applies to: 52-55, 77-88, 271-294, 515-524, 527-536, 16-17
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (5)
86-126: Compile regex once inside matcher for speed and clearer errors.Minor perf win and consistent error surfacing; callers already catch regex errors in most paths.
- # Find all matches - matches = list(re.finditer(pattern, text, flags)) + # Find all matches + regex = re.compile(pattern, flags) + matches = list(regex.finditer(text))
128-146: Docstring claims #endregion handling not implemented.Either implement the check or trim the claim to avoid confusion.
149-191: Heuristic scoring: guard against very long files and keep ties stable.Cap distance-from-end term and add a deterministic tiebreaker.
- distance_from_end = len(lines) - line_num - score += max(0, 10 - distance_from_end) # More points for being closer to end + distance_from_end = len(lines) - line_num + score += max(0, 10 - min(distance_from_end, 10)) # Cap to avoid overweight on huge files @@ - scored_matches.sort(key=lambda x: x[0], reverse=True) + scored_matches.sort(key=lambda x: (x[0], x[1].start()), reverse=True) # stable tiebreaker by position
429-431: Minor aliasing nit: read fromenotedit.Keeps variable usage consistent and avoids surprises if the caller mutates
edit.- if "newText" in edit and "text" not in e: - e["text"] = edit.get("newText", "") + if "newText" in e and "text" not in e: + e["text"] = e.get("newText", "")
820-831: TRY003: long error messages in raises/returns.Optional: centralize standard error strings or shorten here to quiet lint without losing signal.
tests/test_improved_anchor_matching.py (3)
31-37: Ensure cross-version path compatibility when loading module.Convert Path to str for
spec_from_file_location.-def load_module(path, name): +def load_module(path, name): spec = importlib.util.spec_from_file_location(name, path) module = importlib.util.module_from_spec(spec) spec.loader.exec_module(module) return module - -manage_script_edits_module = load_module(SRC / "tools" / "manage_script_edits.py", "manage_script_edits_module") +manage_script_edits_module = load_module(str(SRC / "tools" / "manage_script_edits.py"), "manage_script_edits_module")
146-150: Don’tassert Falsein tests.Raise explicitly to avoid being stripped by
-O.- except StopIteration: - assert False, "NewMethod not found in result" + except StopIteration: + raise AssertionError("NewMethod not found in result")
152-170: Drop or fix the__main__runner.As written, it always prints failure due to
Nonereturns. Recommend removing to avoid confusion.-if __name__ == "__main__": - print("Testing improved anchor matching...") - print("="*60) - - success1 = test_improved_anchor_matching() - - print("\n" + "="*60) - print("Comparing old vs new behavior...") - success2 = test_old_vs_new_matching() - - print("\n" + "="*60) - print("Testing _apply_edits_locally with improved matching...") - success3 = test_apply_edits_with_improved_matching() - - print("\n" + "="*60) - if success1 and success2 and success3: - print("🎉 ALL TESTS PASSED! Improved anchor matching is working!") - else: - print("💥 Some tests failed. Need more work on anchor matching.") +if __name__ == "__main__": + import pytest, sys + sys.exit(pytest.main([__file__]))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
.claude/prompts/nl-unity-suite-full-additive.md(1 hunks).github/workflows/claude-nl-suite.yml(1 hunks)TestProjects/UnityMCPTests/Assets/Scripts/Hello.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Scripts/Hello.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/CustomComponent.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/TestAsmdef.asmdef(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs(3 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs.meta(1 hunks)UnityMcpBridge/Editor/Tools/ManageAsset.cs(2 hunks)UnityMcpBridge/Editor/Tools/ManageScript.cs(4 hunks)UnityMcpBridge/UnityMcpServer~/src/server_version.txt(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py(12 hunks)tests/test_improved_anchor_matching.py(1 hunks)tests/test_regex_delete_guard.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/test_regex_delete_guard.py
🧰 Additional context used
🪛 Ruff (0.12.2)
tests/test_improved_anchor_matching.py
68-68: Use of assert detected
(S101)
72-72: Use of assert detected
(S101)
115-115: Use of assert detected
(S101)
116-116: Use of assert detected
(S101)
118-118: Use of assert detected
(S101)
148-148: Use of assert detected
(S101)
148-148: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
150-150: Use of assert detected
(S101)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py
49-49: Avoid specifying long messages outside the exception class
(TRY003)
🪛 LanguageTool
.claude/prompts/nl-unity-suite-full-additive.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...this once, verbatim, early in the run:** AllowedTools: Write,mcp__unity__manage_e...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ... RESTORATION** - tests build additively on previous state. --- ## Environment & ...
(QB_NEW_EN)
[grammar] ~20-~20: There might be a mistake here.
Context: ...state. --- ## Environment & Paths (CI) - Always pass: `project_root: "TestProject...
(QB_NEW_EN)
[grammar] ~21-~21: There might be a mistake here.
Context: ...nd ctx: {} on list/read/edit/validate. - Canonical URIs only: - Primary: `uni...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ...dit/validate. - Canonical URIs only: - Primary: unity://path/Assets/... (neve...
(QB_NEW_EN)
[grammar] ~23-~23: There might be a mistake here.
Context: ... (never embed project_root in the URI) - Relative (when supported): Assets/... ...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...esized from JUnit) --- ## Tool Mapping - Anchors/regex/structured: `mcp__unity_...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...Mapping - Anchors/regex/structured: mcp__unity__script_apply_edits - Allowed ops: anchor_insert, `replace_m...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...hod, insert_method, delete_method, regex_replace- **Precise ranges / atomic batch**:mcp__u...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ...ply_text_edits(non‑overlapping ranges) - **Hash-only**:mcp__unity__get_sha` — ret...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...ytes,lastModifiedUtc}without file body - **Validation**:mcp__unity__validate_scri...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...c}without file body - **Validation**:mcp__unity__validate_script(level:"standard")- **Dynamic targeting**: Usemcp__unity__fi...
(QB_NEW_EN)
[grammar] ~44-~44: There might be a mistake here.
Context: ...nciples Key Changes from Reset-Based: 1. Dynamic Targeting: Use find_in_file ...
(QB_NEW_EN)
[grammar] ~51-~51: There might be a mistake here.
Context: ...her in real workflows State Tracking: - Track file SHA after each test to ensure...
(QB_NEW_EN)
[grammar] ~60-~60: There might be a mistake here.
Context: ... Specs ### NL-0. Baseline State Capture Goal: Establish initial file state and...
(QB_NEW_EN)
[grammar] ~62-~62: There might be a mistake here.
Context: ...te and verify accessibility Actions: - Read file head and tail to confirm struc...
(QB_NEW_EN)
[grammar] ~68-~68: There might be a mistake here.
Context: ...ore Method Operations (Additive State A) Goal: Demonstrate method replacement o...
(QB_NEW_EN)
[grammar] ~70-~70: There might be a mistake here.
Context: ...thod replacement operations Actions: - Replace HasTarget() method body: `publ...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ...hor Comment Insertion (Additive State B) Goal: Demonstrate anchor-based inserti...
(QB_NEW_EN)
[grammar] ~79-~79: There might be a mistake here.
Context: ...ed insertions above methods Actions: - Use find_in_file to locate current pos...
(QB_NEW_EN)
[grammar] ~80-~80: There might be a mistake here.
Context: ...ds Actions: - Use find_in_file to locate current position of Update() method -...
(QB_NEW_EN)
[grammar] ~85-~85: There might be a mistake here.
Context: ... End-of-Class Content (Additive State C) Goal: Demonstrate end-of-class inserti...
(QB_NEW_EN)
[grammar] ~87-~87: There might be a mistake here.
Context: ...s with smart brace matching Actions: - Use anchor pattern to find the class-end...
(QB_NEW_EN)
[grammar] ~97-~97: There might be a mistake here.
Context: ...ole State Verification (No State Change) Goal: Verify Unity console integration...
(QB_NEW_EN)
[grammar] ~99-~99: There might be a mistake here.
Context: ...n without file modification Actions: - Read Unity console messages (INFO level)...
(QB_NEW_EN)
[grammar] ~104-~104: There might be a mistake here.
Context: ...ry Helper Lifecycle (Returns to State C) Goal: Test insert → verify → delete cy...
(QB_NEW_EN)
[grammar] ~106-~106: There might be a mistake here.
Context: ...te cycle for temporary code Actions: - Find current position of `GetCurrentTarg...
(QB_NEW_EN)
[grammar] ~113-~113: There might be a mistake here.
Context: ...od Body Interior Edit (Additive State D) Goal: Edit method interior without aff...
(QB_NEW_EN)
[grammar] ~115-~115: There might be a mistake here.
Context: ...structure, on modified file Actions: - Use find_in_file to locate current `Ha...
(QB_NEW_EN)
[grammar] ~117-~117: There might be a mistake here.
Context: ...dy interior: change return statement to return true; /* test modification */ - Use validate: "relaxed" for interior-o...
(QB_NEW_EN)
[grammar] ~122-~122: There might be a mistake here.
Context: ... Method Interior Edit (Additive State E) Goal: Edit a different method to show ...
(QB_NEW_EN)
[grammar] ~126-~126: There might be a mistake here.
Context: ... content search - Edit interior line to add null check: `if (animator == null) retu...
(QB_NEW_EN)
[grammar] ~130-~130: There might be a mistake here.
Context: .... End-of-Class Helper (Additive State F) Goal: Add permanent helper method at c...
(QB_NEW_EN)
[grammar] ~132-~132: There might be a mistake here.
Context: ... helper method at class end Actions: - Use smart anchor matching to find curren...
(QB_NEW_EN)
[grammar] ~137-~137: There might be a mistake here.
Context: ...d Evolution Lifecycle (Additive State G) Goal: Insert → modify → finalize a met...
(QB_NEW_EN)
[grammar] ~139-~139: There might be a mistake here.
Context: ...through multiple operations Actions: - Insert basic method: `private int Counte...
(QB_NEW_EN)
[grammar] ~145-~145: There might be a mistake here.
Context: ...-F. Atomic Multi-Edit (Additive State H) Goal: Multiple coordinated edits in si...
(QB_NEW_EN)
[grammar] ~146-~146: There might be a mistake here.
Context: ...H) Goal: Multiple coordinated edits in single atomic operation Actions: - ...
(QB_NEW_EN)
[grammar] ~146-~146: There might be a mistake here.
Context: ...dinated edits in single atomic operation Actions: - Read current file state t...
(QB_NEW_EN)
[grammar] ~147-~147: There might be a mistake here.
Context: ... in single atomic operation Actions: - Read current file state to compute preci...
(QB_NEW_EN)
[grammar] ~149-~149: There might be a mistake here.
Context: ... precise ranges - Atomic edit combining: 1. Add comment in HasTarget(): `// valida...
(QB_NEW_EN)
[grammar] ~150-~150: There might be a mistake here.
Context: ...ing: 1. Add comment in HasTarget(): // validated access 2. Add comment in ApplyBlend(): `// safe ...
(QB_NEW_EN)
[grammar] ~151-~151: There might be a mistake here.
Context: ... 2. Add comment inApplyBlend(): // safe animation 3. Add final class comment:// end of test...
(QB_NEW_EN)
[grammar] ~152-~152: There might be a mistake here.
Context: ...nimation 3. Add final class comment:// end of test modifications` - All edits computed from same file snapsh...
(QB_NEW_EN)
[grammar] ~153-~153: There might be a mistake here.
Context: ...est modifications` - All edits computed from same file snapshot, applied atomically ...
(QB_NEW_EN)
[grammar] ~156-~156: There might be a mistake here.
Context: ...ath Normalization Test (No State Change) Goal: Verify URI forms work equivalent...
(QB_NEW_EN)
[grammar] ~158-~158: There might be a mistake here.
Context: ...uivalently on modified file Actions: - Make identical edit using `unity://path/...
(QB_NEW_EN)
[grammar] ~159-~159: There might be a mistake here.
Context: ...Actions*: - Make identical edit using unity://path/Assets/Scripts/LongUnityScriptClaudeTest.cs - Then using `Assets/Scripts/LongUnityScri...
(QB_NEW_EN)
[grammar] ~160-~160: There might be a mistake here.
Context: ...gUnityScriptClaudeTest.cs- Then usingAssets/Scripts/LongUnityScriptClaudeTest.cs - Second should returnstale_file`, retry...
(QB_NEW_EN)
[grammar] ~162-~162: There might be a mistake here.
Context: ...ith updated SHA - Verify both URI forms target same file - Expected final state: S...
(QB_NEW_EN)
[grammar] ~165-~165: There might be a mistake here.
Context: ...ation on Modified File (No State Change) Goal: Ensure validation works correctl...
(QB_NEW_EN)
[grammar] ~167-~167: There might be a mistake here.
Context: ...ly on heavily modified file Actions: - Run validate_script(level:"standard") ...
(QB_NEW_EN)
[grammar] ~172-~172: There might be a mistake here.
Context: ...ailure Surface Testing (No State Change) Goal: Test error handling on real modi...
(QB_NEW_EN)
[grammar] ~174-~174: There might be a mistake here.
Context: ...dling on real modified file Actions: - Attempt overlapping edits (should fail c...
(QB_NEW_EN)
[grammar] ~180-~180: There might be a mistake here.
Context: ...ency on Modified File (Additive State I) Goal: Verify operations behave predict...
(QB_NEW_EN)
[grammar] ~182-~182: There might be a mistake here.
Context: ...e predictably when repeated Actions: - Add unique marker comment: `// idempoten...
(QB_NEW_EN)
[grammar] ~184-~184: There might be a mistake here.
Context: .../ idempotency test marker` - Attempt to add same comment again (should detect no-op...
(QB_NEW_EN)
[grammar] ~218-~218: There might be a mistake here.
Context: ...erification Patterns After each test: 1. Verify expected content exists: `find_in...
(QB_NEW_EN)
[grammar] ~224-~224: There might be a mistake here.
Context: ...nges in test evidence Error Recovery: - If test fails, log current state but con...
(QB_NEW_EN)
[grammar] ~225-~225: There might be a mistake here.
Context: ...e Error Recovery: - If test fails, log current state but continue (don't resto...
(QB_NEW_EN)
[grammar] ~226-~226: There might be a mistake here.
Context: ...inue (don't restore) - Next test adapts to actual current state, not expected stat...
(QB_NEW_EN)
[grammar] ~226-~226: There might be a mistake here.
Context: ...xt test adapts to actual current state, not expected state - Demonstrates resilienc...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
.claude/prompts/nl-unity-suite-full-additive.md
90-90: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (7)
TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/TestAsmdef.asmdef (1)
14-14: EOF newline addition is fine.No behavioral change; keeping JSON with a trailing newline is standard.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs.meta (1)
1-2: Duplicate‐detection script failed to extract GUIDs; adjust the regex or use Unity’s AssetDatabase/CLI to confirm no two .meta files share the same 32-character GUID.TestProjects/UnityMCPTests/Assets/Scripts/Hello.cs (1)
15-15: EOF newline addition is fine.No code changes; OK to merge.
TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/CustomComponent.cs.meta (1)
2-2: Trailing newline normalization is fine.No impact on GUID or asset linkage.
TestProjects/UnityMCPTests/Assets/Scripts/Hello.cs.meta (1)
2-2: Meta GUID formatting change looks goodSwitched to standard YAML key without altering the GUID. No functional impact.
UnityMcpBridge/Editor/Tools/ManageScript.cs (2)
113-118: Good guard for null parametersEarly-return on null @params prevents NREs and aligns with new validation tests.
704-706: Unconditional final structural check is a good safety netEnsures whole-file balance even in relaxed mode.
| var method = typeof(ManageScript).GetMethod("CheckBalancedDelimiters", | ||
| BindingFlags.NonPublic | BindingFlags.Static); | ||
|
|
||
| if (method != null) | ||
| { | ||
| var parameters = new object[] { contents, line, expected }; | ||
| var result = (bool)method.Invoke(null, parameters); | ||
| line = (int)parameters[1]; | ||
| expected = (char)parameters[2]; | ||
| return result; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Resolve private method overloads with by-ref signatures
Reflection by name can hit the wrong overload or fail with by-ref args. Specify parameter types including MakeByRefType for out params.
- var method = typeof(ManageScript).GetMethod("CheckBalancedDelimiters",
- BindingFlags.NonPublic | BindingFlags.Static);
+ var method = typeof(ManageScript).GetMethod(
+ "CheckBalancedDelimiters",
+ BindingFlags.NonPublic | BindingFlags.Static,
+ binder: null,
+ types: new[] { typeof(string), typeof(int).MakeByRefType(), typeof(char).MakeByRefType() },
+ modifiers: null
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var method = typeof(ManageScript).GetMethod("CheckBalancedDelimiters", | |
| BindingFlags.NonPublic | BindingFlags.Static); | |
| if (method != null) | |
| { | |
| var parameters = new object[] { contents, line, expected }; | |
| var result = (bool)method.Invoke(null, parameters); | |
| line = (int)parameters[1]; | |
| expected = (char)parameters[2]; | |
| return result; | |
| var method = typeof(ManageScript).GetMethod( | |
| "CheckBalancedDelimiters", | |
| BindingFlags.NonPublic | BindingFlags.Static, | |
| binder: null, | |
| types: new[] { typeof(string), typeof(int).MakeByRefType(), typeof(char).MakeByRefType() }, | |
| modifiers: null | |
| ); | |
| if (method != null) | |
| { | |
| var parameters = new object[] { contents, line, expected }; | |
| var result = (bool)method.Invoke(null, parameters); | |
| line = (int)parameters[1]; | |
| expected = (char)parameters[2]; | |
| return result; |
🤖 Prompt for AI Agents
In
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs
around lines 107 to 116, the reflection call risks selecting the wrong overload
and won't match by-ref parameters; resolve this by calling GetMethod with an
explicit parameter type array that uses MakeByRefType for the ref/out parameters
(e.g. typeof(string), typeof(int).MakeByRefType(),
typeof(char).MakeByRefType()), using the overload of GetMethod that accepts
parameter types, then invoke the method the same way and read back the updated
ref/out values from the parameters array after Invoke.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs
Show resolved
Hide resolved
…ments - Move RenderTexture cleanup to finally block to ensure proper disposal - Add PNG data validation before Base64 conversion - Add explicit TextureFormat.RGB24 specification - Add bool() coercion for prefer_last parameter to handle non-boolean JSON values 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
596-609: Honor prefer_last in mixed text-first path.Currently hard-codes prefer_last=True; read and coerce it from the edit (support camelCase too) for consistency with local and structured paths.
- # Use improved anchor matching logic - m = _find_best_anchor_match(anchor, base_text, flags, prefer_last=True) + # Use improved anchor matching logic + prefer_last_raw = e.get("prefer_last", e.get("preferLast", True)) + prefer_last = (str(prefer_last_raw).strip().lower() not in ("false","0","no")) + m = _find_best_anchor_match(anchor, base_text, flags, prefer_last=prefer_last)
721-734: Honor prefer_last in text path and support ignoreCase/preferLast aliases.Use the provided flag when present; current behavior always prefers last.
- try: - flags = _re.MULTILINE | (_re.IGNORECASE if e.get("ignore_case") else 0) - m = _find_best_anchor_match(anchor, base_text, flags, prefer_last=True) + try: + flags = _re.MULTILINE | (_re.IGNORECASE if (e.get("ignore_case") or e.get("ignoreCase")) else 0) + prefer_last_raw = e.get("prefer_last", e.get("preferLast", True)) + prefer_last = (str(prefer_last_raw).strip().lower() not in ("false","0","no")) + m = _find_best_anchor_match(anchor, base_text, flags, prefer_last=prefer_last)Additionally, consider normalizing these aliases during unwrapping:
if "newText" in e and "text" not in e: e["text"] = e.pop("newText") + # Boolean aliases + if "ignoreCase" in e and "ignore_case" not in e: + e["ignore_case"] = e.pop("ignoreCase") + if "preferLast" in e and "prefer_last" not in e: + e["prefer_last"] = e.pop("preferLast")
♻️ Duplicate comments (3)
UnityMcpBridge/Editor/Tools/ManageAsset.cs (1)
1274-1299: Preserve alpha in previews; add log when PNG encode returns emptyUse RGBA32 to avoid dropping alpha from previews and log when EncodeToPNG yields null/empty, aiding diagnostics.
- readablePreview = new Texture2D(preview.width, preview.height, TextureFormat.RGB24, false); + readablePreview = new Texture2D(preview.width, preview.height, TextureFormat.RGBA32, false); readablePreview.ReadPixels(new Rect(0, 0, rt.width, rt.height), 0, 0); readablePreview.Apply(); var pngData = readablePreview.EncodeToPNG(); if (pngData != null && pngData.Length > 0) { previewBase64 = Convert.ToBase64String(pngData); previewWidth = readablePreview.width; previewHeight = readablePreview.height; } + else + { + Debug.LogWarning($"EncodeToPNG returned null/empty for '{path}'."); + }UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
44-51: Anchor insert: coerce prefer_last correctly and normalize insertion newlines.
- bool("false") is True; current cast misinterprets "false"/"0"/"no".
- Raw insert_text can jam against braces; normalize surrounding newlines here too.
- Also guard invalid regex to return a clear error instead of crashing.
- # Find the best match using improved heuristics - match = _find_best_anchor_match(anchor, text, flags, bool(edit.get("prefer_last", True))) + # Find the best match using improved heuristics + prefer_last_raw = edit.get("prefer_last", True) + prefer_last = (str(prefer_last_raw).strip().lower() not in ("false","0","no")) + try: + match = _find_best_anchor_match(anchor, text, flags, prefer_last) + except re.error as ex: + raise RuntimeError(f"Invalid anchor regex: {ex}") if not match: if edit.get("allow_noop", True): continue raise RuntimeError(f"anchor not found: {anchor}") - idx = match.start() if position == "before" else match.end() - text = text[:idx] + insert_text + text[idx:] + idx = match.start() if position == "before" else match.end() + # Normalize insertion text to avoid jammed content + it = insert_text.replace("\r\n", "\n") + if it and not it.startswith("\n"): + it = "\n" + it + if it and not it.endswith("\n"): + it = it + "\n" + text = text[:idx] + it + text[idx:]
766-772: Harden $n replacement here as well.Mirror the safe expansion to avoid IndexError on out-of-range groups.
- def _expand_dollars(rep: str, _m=m) -> str: - return _re.sub(r"\$(\d+)", lambda g: _m.group(int(g.group(1))) or "", rep) + def _expand_dollars(rep: str, _m=m) -> str: + def repl(g): + idx = int(g.group(1)) + try: + val = _m.group(idx) + except IndexError: + return "" + return val or "" + return _re.sub(r"\$(\d+)", repl, rep)
🧹 Nitpick comments (3)
UnityMcpBridge/Editor/Tools/ManageAsset.cs (1)
182-193: Material shader fallback is good; consider pipeline awareness and SRP color property compatibility
- Optional: Prefer a pipeline-aware pick when no explicit shader is requested (e.g., if GraphicsSettings.currentRenderPipeline != null, try SRP-appropriate defaults first; otherwise prefer "Standard"). This avoids selecting an SRP shader in a Built-in project (or vice versa).
- To better interop with URP/HDRP, make ApplyMaterialProperties also try _BaseColor when _Color is missing, especially since URP/HDRP Lit use _BaseColor.
Example (outside this hunk) to improve color-setting fallback in ApplyMaterialProperties:
// After computing newColor var candidateProps = new[] { propName ?? "_Color", "_BaseColor", "_Color" } .Where(p => !string.IsNullOrEmpty(p)) .Distinct(); foreach (var p in candidateProps) { if (mat.HasProperty(p) && mat.GetColor(p) != newColor) { mat.SetColor(p, newColor); modified = true; break; } }UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
86-126: _find_best_anchor_match: minor polish (compilation and import).
- Consider accepting a compiled Pattern or compiling once for perf; callers re-use patterns.
- Drop the inner
import re; rely on the module import for consistency.-def _find_best_anchor_match(pattern: str, text: str, flags: int, prefer_last: bool = True): +def _find_best_anchor_match(pattern: str, text: str, flags: int, prefer_last: bool = True): @@ - import re - - # Find all matches - matches = list(re.finditer(pattern, text, flags)) + # Find all matches + matches = list(re.finditer(pattern, text, flags))
128-194: Docstring mentions #endregion handling but code doesn’t implement it.Either implement the stated #endregion-specific heuristic or adjust the docstring to avoid misleading guidance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
UnityMcpBridge/Editor/Tools/ManageAsset.cs(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/Tools/ManageAsset.cs (1)
UnityMcpBridge/Editor/Helpers/Response.cs (2)
Response(10-61)Error(41-60)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py
49-49: Avoid specifying long messages outside the exception class
(TRY003)
| def _expand_dollars(rep: str, _m=m) -> str: | ||
| return _re.sub(r"\$(\d+)", lambda g: _m.group(int(g.group(1))) or "", rep) | ||
| repl = _expand_dollars(text_field) |
There was a problem hiding this comment.
$n replacement can crash on missing groups (IndexError).
If replacement contains $2 but the match has only 1 group, _m.group(2) raises IndexError. Handle out-of-range safely.
- def _expand_dollars(rep: str, _m=m) -> str:
- return _re.sub(r"\$(\d+)", lambda g: _m.group(int(g.group(1))) or "", rep)
+ def _expand_dollars(rep: str, _m=m) -> str:
+ def repl(g):
+ idx = int(g.group(1))
+ try:
+ val = _m.group(idx)
+ except IndexError:
+ return ""
+ return val or ""
+ return _re.sub(r"\$(\d+)", repl, rep)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _expand_dollars(rep: str, _m=m) -> str: | |
| return _re.sub(r"\$(\d+)", lambda g: _m.group(int(g.group(1))) or "", rep) | |
| repl = _expand_dollars(text_field) | |
| def _expand_dollars(rep: str, _m=m) -> str: | |
| def repl(g): | |
| idx = int(g.group(1)) | |
| try: | |
| val = _m.group(idx) | |
| except IndexError: | |
| return "" | |
| return val or "" | |
| return _re.sub(r"\$(\d+)", repl, rep) | |
| repl = _expand_dollars(text_field) |
🤖 Prompt for AI Agents
In UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py around lines
633 to 635, the dollar-group replacer can raise IndexError when a $n references
a group that doesn't exist; change _expand_dollars to validate the numeric group
index against the match object's available groups (or use try/except around
_m.group(int(...)) ) and return an empty string when the referenced group is out
of range so missing groups are handled safely rather than crashing.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests
Chores