Unity tests fork backup #103
Conversation
|
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. WalkthroughUpdated Claude NL/T workflow model settings and removed the MCP Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant FindTool as find_in_file
participant URI as _split_uri
participant Unity as Unity (manage_script)
participant Regex as Regex Engine
Caller->>FindTool: find_in_file(ctx, uri, pattern, ...)
FindTool->>URI: _split_uri(uri)
URI-->>FindTool: (name, directory)
FindTool->>Unity: read file (name, path)
Unity-->>FindTool: file contents (raw or base64)
alt Read success
FindTool->>Regex: compile(pattern, flags)
Regex-->>FindTool: compiled_regex
FindTool->>Regex: finditer(content)
Regex-->>FindTool: match objects
loop per match (<= max_results)
FindTool->>FindTool: compute line number, excerpt, indices
end
FindTool-->>Caller: { success: true, data: {matches, count, total_matches} }
else Read failure
FindTool-->>Caller: { success: false, error }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on January 7
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Server/src/services/tools/find_in_file.py (2)
72-72: Remove or implement the unusedproject_rootparameter.The
project_rootparameter is declared but never used in the function body. If it's not needed for current or planned functionality, consider removing it to simplify the API.
99-104: Catch specific exceptions instead of bareexcept.The bare
exceptclause catches all exceptions including system exits. Specify the expected exception types (e.g.,except (ValueError, TypeError, base64.Error)).Apply this diff:
- except Exception: + except (ValueError, TypeError, base64.Error): contents = contents or ""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/claude-nl-suite.yml(2 hunks)Server/src/services/tools/find_in_file.py(1 hunks)TestProjects/UnityMCPTests/Assets/Scripts/Editor.meta(0 hunks)
💤 Files with no reviewable changes (1)
- TestProjects/UnityMCPTests/Assets/Scripts/Editor.meta
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T21:31:02.469Z
Learnt from: dsarno
Repo: dsarno/unity-mcp PR: 56
File: .claude/prompts/nl-unity-suite-full.md:5-6
Timestamp: 2025-08-29T21:31:02.469Z
Learning: The Claude NL suite workflows (.github/workflows/claude-nl-suite.yml and .github/workflows/claude-nl-suite-mini.yml) allow "Bash" tools generally, including printf and echo operations, so prompts that reference Bash(printf:*) and Bash(echo:*) are consistent with the actual workflow permissions.
Applied to files:
.github/workflows/claude-nl-suite.yml
🪛 Ruff (0.14.8)
Server/src/services/tools/find_in_file.py
72-72: Unused function argument: project_root
(ARG001)
103-103: Do not catch blind exception: Exception
(BLE001)
123-123: Local variable matches is assigned to but never used
Remove assignment to unused variable matches
(F841)
124-124: Local variable lines is assigned to but never used
Remove assignment to unused variable lines
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: nl-suite
🔇 Additional comments (8)
Server/src/services/tools/find_in_file.py (5)
1-13: LGTM!The imports are well-organized and appropriate for the file search functionality.
15-64: LGTM!The URI parsing logic correctly handles multiple URI formats (unity://, file://, plain paths) with proper normalization for Windows drive letters, UNC paths, and Assets-relative path derivation.
109-121: LGTM!The regex compilation with error handling and flexible
ignore_caseparameter handling (supporting both boolean and string inputs) is well-implemented.
139-175: LGTM!The regex matching logic correctly:
- Searches the entire file content
- Calculates line numbers by counting newlines
- Extracts line excerpts using efficient string operations
- Respects the
max_resultslimit- Returns structured match information with line, content, match text, and indices
177-184: LGTM!The return structure clearly distinguishes between
count(number of results returned, capped bymax_results) andtotal_matches(total matches found), allowing callers to detect truncation..github/workflows/claude-nl-suite.yml (3)
446-447: LGTM!The updated redirect strategy correctly separates MCP communication (stdout) from logging (stderr), which is essential for stdio-based MCP transport.
647-648: LGTM!The model configuration now uses Claude Haiku as primary with Sonnet fallback, optimizing for speed while maintaining reliability. This is a sensible cost/performance trade-off for the NL pass.
681-682: LGTM!Consistent with the NL pass, the T pass now uses Claude Haiku with Sonnet fallback, maintaining a uniform model strategy across both test stages.
Greptile OverviewGreptile SummaryAdds regex-based Major changes:
Issues found:
Confidence Score: 4/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Client as MCP Client
participant Tool as find_in_file Tool
participant URI as _split_uri Helper
participant Unity as Unity Instance
participant Regex as Regex Engine
Client->>Tool: find_in_file(uri, pattern, options)
Tool->>URI: _split_uri(uri)
alt unity://path/Assets/...
URI->>URI: Extract Assets-relative path
else file://...
URI->>URI: URL decode & normalize
URI->>URI: Find Assets segment
else plain path
URI->>URI: Normalize separators
URI->>URI: Find Assets segment
end
URI-->>Tool: (name, directory)
Tool->>Unity: manage_script(action=read, name, path)
Unity-->>Tool: {success, data: {contents or contentsEncoded}}
alt contentsEncoded=true
Tool->>Tool: Base64 decode encodedContents
end
Tool->>Regex: compile(pattern, flags)
alt Invalid regex
Regex-->>Tool: re.error
Tool-->>Client: {success: false, message}
end
Regex->>Regex: finditer(contents)
loop For each match (up to max_results)
Regex->>Tool: Match object
Tool->>Tool: Calculate line number
Tool->>Tool: Extract line content
Tool->>Tool: Append to results
end
Tool-->>Client: {success: true, data: {matches, count, total_matches}}
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| matches = [] | ||
| lines = contents.splitlines() | ||
|
|
||
| # Helper to map index to line number | ||
| def get_line_number(index, content_lines): | ||
| # This is a bit slow for large files if we do it for every match, | ||
| # but robust. | ||
| # Better: iterate matches and count newlines? | ||
| # Or just search line by line? | ||
| # Searching line by line is safer for line-based results, but regex might span lines. | ||
| pass |
There was a problem hiding this comment.
style: Unused matches list and dead code in get_line_number function - should be removed
| matches = [] | |
| lines = contents.splitlines() | |
| # Helper to map index to line number | |
| def get_line_number(index, content_lines): | |
| # This is a bit slow for large files if we do it for every match, | |
| # but robust. | |
| # Better: iterate matches and count newlines? | |
| # Or just search line by line? | |
| # Searching line by line is safer for line-based results, but regex might span lines. | |
| pass | |
| # Search the whole content and map back to lines |
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/src/services/tools/find_in_file.py
Line: 123:133
Comment:
**style:** Unused `matches` list and dead code in `get_line_number` function - should be removed
```suggestion
# Search the whole content and map back to lines
```
How can I resolve this? If you propose a fix, please make it concise.| ctx: Context, | ||
| uri: Annotated[str, "The resource URI to search under Assets/ or file path form supported by read_resource"], | ||
| pattern: Annotated[str, "The regex pattern to search for"], | ||
| project_root: Annotated[str | None, "Optional project root path"] = None, |
There was a problem hiding this comment.
style: project_root parameter is defined but never used in the function
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/src/services/tools/find_in_file.py
Line: 72:72
Comment:
**style:** `project_root` parameter is defined but never used in the function
How can I resolve this? If you propose a fix, please make it concise.| if not contents and data.get("contentsEncoded"): | ||
| try: | ||
| contents = base64.b64decode(data.get("encodedContents", "").encode( | ||
| "utf-8")).decode("utf-8", "replace") | ||
| except Exception: | ||
| contents = contents or "" |
There was a problem hiding this comment.
logic: Logic checks contentsEncoded but decodes from encodedContents - field name mismatch will cause decoding to fail
| if not contents and data.get("contentsEncoded"): | |
| try: | |
| contents = base64.b64decode(data.get("encodedContents", "").encode( | |
| "utf-8")).decode("utf-8", "replace") | |
| except Exception: | |
| contents = contents or "" | |
| if not contents and data.get("contentsEncoded"): | |
| try: | |
| contents = base64.b64decode(data.get("contentsEncoded", "").encode( | |
| "utf-8")).decode("utf-8", "replace") | |
| except Exception: | |
| contents = contents or "" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/src/services/tools/find_in_file.py
Line: 99:104
Comment:
**logic:** Logic checks `contentsEncoded` but decodes from `encodedContents` - field name mismatch will cause decoding to fail
```suggestion
if not contents and data.get("contentsEncoded"):
try:
contents = base64.b64decode(data.get("contentsEncoded", "").encode(
"utf-8")).decode("utf-8", "replace")
except Exception:
contents = contents or ""
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Server/src/services/tools/find_in_file.py (1)
98-105: Fix fallback decoding for base64-encoded contents (contentsEncodedvsencodedContents).The guard checks
data.get("contentsEncoded")but the decode usesencodedContents, so valid encoded content will be ignored and you’ll end up searching an empty string or erroring despite a successful read.Align the keys and support either spelling:
- contents = data.get("contents") - if not contents and data.get("contentsEncoded"): - try: - contents = base64.b64decode(data.get("encodedContents", "").encode( - "utf-8")).decode("utf-8", "replace") - except (ValueError, TypeError, base64.binascii.Error): - contents = contents or "" + contents = data.get("contents") + if not contents: + encoded = data.get("contentsEncoded") or data.get("encodedContents") + if encoded: + try: + contents = base64.b64decode(encoded.encode("utf-8")).decode( + "utf-8", "replace" + ) + except (ValueError, TypeError, base64.binascii.Error): + contents = contents or ""This way you correctly use the encoded payload regardless of which field name Unity returns.
🧹 Nitpick comments (3)
Server/src/services/tools/find_in_file.py (1)
72-77: Unusedproject_rootparameter (stylistic only).
project_rootis unused and Ruff flags it (ARG001). Since you’re keeping it for interface consistency, consider renaming to_project_root(or using it in_split_urilogic later) to quiet linters without changing the public API..github/workflows/claude-nl-suite.yml (2)
439-448: Stderr-only redirect is correct; consider quotinguv_cmdfor safety.Redirecting only stderr to the debug log while leaving stdout for MCP framing is exactly what you want here. To be robust against paths with spaces, you might quote the command:
- exec {uv_cmd} "$@" 2>> "$LOG" + exec "{uv_cmd}" "$@" 2>> "$LOG"
817-825: Unify NL/T filename regexes across helpers (allowing optional dash everywhere).
id_from_filenamein the canonicalization and backfill steps now accept bothNL0_results.xml/T AandNL-0_results.xml/T-A_results.xml, but the JUnit assemblyid_from_filenamestill only matches the non-dashed form. That means dashed filenames rely on thesystem-outfallback instead of the filename, which is a bit fragile.Recommend making the JUnit helper match the same forms:
- def id_from_filename(p: Path): - n = p.name - m = re.match(r'NL(\d+)_results\.xml$', n, re.I) - if m: - return f"NL-{int(m.group(1))}" - m = re.match(r'T([A-J])_results\.xml$', n, re.I) - if m: - return f"T-{m.group(1).upper()}" - return None + def id_from_filename(p: Path): + n = p.name + m = re.match(r'NL-?(\d+)_results\.xml$', n, re.I) + if m: + return f"NL-{int(m.group(1))}" + m = re.match(r'T-?([A-J])_results\.xml$', n, re.I) + if m: + return f"T-{m.group(1).upper()}" + return Noneor, longer term, factor a single shared helper so these patterns can’t diverge.
Also applies to: 866-872, 941-949
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/claude-nl-suite.yml(4 hunks)Server/src/services/tools/find_in_file.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T21:31:02.469Z
Learnt from: dsarno
Repo: dsarno/unity-mcp PR: 56
File: .claude/prompts/nl-unity-suite-full.md:5-6
Timestamp: 2025-08-29T21:31:02.469Z
Learning: The Claude NL suite workflows (.github/workflows/claude-nl-suite.yml and .github/workflows/claude-nl-suite-mini.yml) allow "Bash" tools generally, including printf and echo operations, so prompts that reference Bash(printf:*) and Bash(echo:*) are consistent with the actual workflow permissions.
Applied to files:
.github/workflows/claude-nl-suite.yml
🪛 Ruff (0.14.8)
Server/src/services/tools/find_in_file.py
72-72: Unused function argument: project_root
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (2)
Server/src/services/tools/find_in_file.py (1)
110-173: Regex search and line/excerpt mapping look correct.Flags handling (including stringy
ignore_case),finditerusage, newline-based line calculation, andmax_resultscapping all look sound and should produce stable line numbers and excerpts even for multiline patterns..github/workflows/claude-nl-suite.yml (1)
647-648: Model IDs are valid; no action needed.The model IDs are legitimate Anthropic Claude 4.5 snapshots:
claude-haiku-4-5-20251001(Haiku 4.5, Oct 1, 2025 snapshot)claude-sonnet-4-5-20250929(Sonnet 4.5, Sep 29, 2025 snapshot)Both are supported by
anthropics/claude-code-base-action@beta, which accepts any valid Anthropic model ID. The setup using Haiku with Sonnet fallback is sound.
| contents = contents or "" | ||
|
|
||
| if contents is None: | ||
| return {"success": False, "message": "Could not read file content."} |
There was a problem hiding this comment.
Bug: Base64 decode failure silently returns empty results
When base64 decoding fails, the exception handler sets contents = contents or "", resulting in an empty string. The subsequent check if contents is None: doesn't catch this case since "" is not None. This causes the function to silently return success with zero matches instead of reporting a decode error. If contents was originally None and contentsEncoded is true but decoding fails, users get a false positive "no matches found" response rather than an error message.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Server/src/services/tools/find_in_file.py (1)
100-108: Base64 decode failure silently returns empty results instead of error.If
contentsisNoneinitially and decoding fails, line 105 setscontents = "". The check at line 107 (if contents is None) won't catch this, causing the function to proceed with empty content and return zero matches—a false positive instead of an error.if not contents and data.get("contentsEncoded") and data.get("encodedContents"): try: contents = base64.b64decode(data.get("encodedContents", "").encode( "utf-8")).decode("utf-8", "replace") - except (ValueError, TypeError, base64.binascii.Error): - contents = contents or "" - - if contents is None: - return {"success": False, "message": "Could not read file content."} + except (ValueError, TypeError, base64.binascii.Error) as e: + return {"success": False, "message": f"Failed to decode file content: {e}"} + + if not contents and contents != "": + return {"success": False, "message": "Could not read file content."}Alternatively, if empty files are valid and should return zero matches, keep the original flow but ensure the decode error is explicitly reported.
🧹 Nitpick comments (2)
Server/src/services/tools/find_in_file.py (2)
15-64: Extract_split_urito a shared module to avoid duplication.This function is duplicated verbatim from
manage_script.py. Extract it to a shared utility module (e.g.,services/tools/uri_utils.py) and import it in both files.# services/tools/uri_utils.py from urllib.parse import unquote, urlparse import os def split_uri(uri: str) -> tuple[str, str]: """Split an incoming URI or path into (name, directory) suitable for Unity.""" # ... existing implementation ...Then in both files:
from services.tools.uri_utils import split_uri
72-76: Prefix unused parameter with underscore to silence linter.The parameter is intentionally kept for interface consistency (as documented at line 76), but should be prefixed to signal it's unused.
- project_root: Annotated[str | None, "Optional project root path"] = None, + _project_root: Annotated[str | None, "Optional project root path (unused, kept for interface consistency)"] = None,Then remove the comment at line 76.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Server/src/services/tools/find_in_file.py(1 hunks)Server/src/services/tools/manage_script.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.8)
Server/src/services/tools/find_in_file.py
72-72: Unused function argument: project_root
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (3)
Server/src/services/tools/manage_script.py (1)
124-129: Stricter decoding gate looks correct.The condition now properly requires all three conditions: no existing
contents,contentsEncodedflag is truthy, ANDencodedContentsdata is present. This prevents decode attempts when there's no actual encoded data.Server/src/services/tools/find_in_file.py (2)
128-164: Regex search implementation looks correct.The line number calculation, excerpt extraction, and result capping are implemented correctly.
One minor consideration:
line_content.strip()at line 159 removes leading/trailing whitespace, which may make it harder to see exact indentation context. Consider keeping the original or offering a configurable option.
166-173: Return structure is well-designed.Including both
count(returned matches) andtotal_matches(all found) allows callers to detect truncation. This is a good UX pattern.
Note
Adds a new
find_in_fileregex search tool, updates workflows (stderr-only logging, Haiku primary with Sonnet fallback, repo-stats gating), and broadens NL/T fragment ID parsing; minor base64 decode guard and cleanup.Server/src/services/tools/find_in_file.py— regex search in files with line/position excerpts; supportsunity://,file://, and plain paths.manage_script.apply_text_editsonly decodes contents when bothcontentsEncodedandencodedContentsare present.stderrto log to preservestdoutfor MCP (.claude/run-unity-mcp.sh).claude-haiku-4-5-20251001with fallbackclaude-sonnet-4-5-20250929in NL/T passes.NL-?(\d+),T-?([A-J])).github.repository == 'CoplayDev/unity-mcp').TestProjects/UnityMCPTests/Assets/Scripts/Editor.meta.Written by Cursor Bugbot for commit 03f5cc7. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Chores
✏️ Tip: You can customize this high-level summary in your review settings.