Framed transport + Claude‑friendly edit tools + live Unity NL test framework #42 #56
Conversation
…x optional + project_root fallback; workflows: set UNITY_PROJECT_ROOT for CI
…rompts: mini NL/T clarifications
…ver: accept relative UNITY_PROJECT_ROOT and bare spec URI
…to avoid absolute paths
…w batch-mode bridge via UNITY_MCP_ALLOW_BATCH
… readiness timeout
…ss from container
…e readiness; fix YAML
… validation; explicit status dir + license env
…; ready to run happy path
…ense; create dirs before start
…th shared mounts; mount .config and .cache too
… probe python; stabilize licensing flow
… validated; fix indentation
…n MCP server with python; tighten secret scoping
…L); readiness logs+probe; Claude gating via outputs
…icensing and MCP/Claude wiring
…ch/new Regex) and keep CultureInvariant/Multiline options; reduces risk of catastrophic backtracking stalls.
…NotFoundError when writing MD_OUT.
![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
♻️ Duplicate comments (6)
.claude/prompts/nl-unity-suite-full.md (1)
95-101: Fix FS‑write rules contradiction; explicitly allow Write + revert helper only.Current text says “only … revert.sh” but earlier requires Write to emit JUnit fragments. Tighten and clarify.
- - Bash(printf|echo) must write to stdout only. Do not use shell redirection, here‑docs, or `tee` to create/modify files. The only allowed FS mutation is via `scripts/nlt-revert.sh`. + - Only these may modify the filesystem: + 1) Write → `reports/*_results.xml` (per path rules above) + 2) `scripts/nlt-revert.sh` → snapshot/restore + - Do not use shell redirection, here‑docs, or `tee`..github/workflows/claude-nl-suite.yml (3)
148-176: Broaden secret redaction in logs (emails, api keys, key=value, etc.).-docker logs -f unity-mcp 2>&1 | sed -E 's/((serial|license|password|token)[^[:space:]]*)/[REDACTED]/ig' & LOGPID=$! +docker logs -f unity-mcp 2>&1 | sed -E 's/([[:alnum:]_]*(serial|license|password|token|key|secret|api[-_]?key|email|username|user|uid)[[:alnum:]_]*[[:space:]]*[:=][[:space:]]*)[^[:space:]]+/\1[REDACTED]/ig' & LOGPID=$! @@ -docker logs --tail 200 unity-mcp | sed -E 's/((serial|license|password|token)[^[:space:]]*)/[REDACTED]/ig' || true +docker logs --tail 200 unity-mcp | sed -E 's/([[:alnum:]_]*(serial|license|password|token|key|secret|api[-_]?key|email|username|user|uid)[[:alnum:]_]*[[:space:]]*[:=][[:space:]]*)[^[:space:]]+/\1[REDACTED]/ig' || true @@ -docker logs --tail 400 unity-mcp | sed -E 's/((serial|license|password|token)[^[:space:]]*)/[REDACTED]/ig' || true +docker logs --tail 400 unity-mcp | sed -E 's/([[:alnum:]_]*(serial|license|password|token|key|secret|api[-_]?key|email|username|user|uid)[[:alnum:]_]*[[:space:]]*[:=][[:space:]]*)[^[:space:]]+/\1[REDACTED]/ig' || trueAlso applies to: 540-540
19-27: Expose job outputs (paths) for downstream workflows.nl-suite: if: github.event_name == 'workflow_dispatch' runs-on: ubuntu-latest timeout-minutes: 60 + outputs: + junit_path: ${{ steps.expose_paths.outputs.junit_out }} + md_path: ${{ steps.expose_paths.outputs.md_out }} @@ - name: Prepare reports and dirs run: | set -eux rm -f reports/*.xml reports/*.md || true mkdir -p reports reports/_snapshots scripts + + - name: Expose output paths + id: expose_paths + run: | + echo "junit_out=${JUNIT_OUT}" >> "$GITHUB_OUTPUT" + echo "md_out=${MD_OUT}" >> "$GITHUB_OUTPUT"Also applies to: 202-208
223-257: Lock snapshot paths to reports/_snapshots and add hygiene checks.Prevents path traversal/accidental overwrites; enforce scope and sane filenames.
cat > scripts/nlt-revert.sh <<'BASH' #!/usr/bin/env bash set -euo pipefail sub="${1:-}"; target_rel="${2:-}"; snap="${3:-}" WS="${GITHUB_WORKSPACE:-$PWD}" ROOT="$WS/TestProjects/UnityMCPTests" t_abs="$(realpath -m "$WS/$target_rel")" - s_abs="$(realpath -m "$WS/$snap")" + SNAP_ROOT="$WS/reports/_snapshots" + s_abs="$(realpath -m "$WS/$snap")" if [[ "$t_abs" != "$ROOT/Assets/"* ]]; then echo "refuse: target outside allowed scope: $t_abs" >&2; exit 2 fi + # Enforce snapshot scope and filename hygiene + if [[ "$s_abs" != "$SNAP_ROOT/"* ]]; then + echo "refuse: snapshot path outside reports/_snapshots: $s_abs" >&2; exit 2 + fi + if printf '%s' "$s_abs" | grep -q '[[:cntrl:]]'; then + echo "refuse: control chars in path" >&2; exit 2 + fi + rel_snap="${s_abs#$SNAP_ROOT/}"; IFS='/' read -r -a segs <<< "$rel_snap" + for seg in "${segs[@]}"; do + [[ -z "$seg" || "$seg" == ".." || ${#seg} -gt 100 || ! "$seg" =~ ^[A-Za-z0-9._-]+$ ]] && { echo "refuse: bad segment: $seg" >&2; exit 2; } + done + (( ${#s_abs} <= 255 )) || { echo "refuse: snapshot path too long" >&2; exit 2; } mkdir -p "$(dirname "$s_abs")"UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
490-499: regex_replace only replaces first match and ignores count/ignore_case (mixed + text paths).This will silently under-apply edits and diverge from preview semantics. Honor count (0 = all) and ignore_case, and emit multiple spans in order.
Apply:
@@ - elif opx == "regex_replace": - pattern = e.get("pattern") or "" - m = _re.search(pattern, current_text, _re.MULTILINE) - if not m: - continue - sl, sc = line_col_from_index(m.start()) - el, ec = line_col_from_index(m.end()) - at_edits.append({"startLine": sl, "startCol": sc, "endLine": el, "endCol": ec, "newText": text_field}) - current_text = current_text[:m.start()] + text_field + current_text[m.end():] + elif opx == "regex_replace": + pattern = e.get("pattern") or "" + count = int(e.get("count", 0) or 0) # 0 = all + flags = _re.MULTILINE | (_re.IGNORECASE if e.get("ignore_case") else 0) + base = current_text + matches = list(_re.finditer(pattern, base, flags)) + if not matches: + continue + if count > 0: + matches = matches[:count] + # Compute spans against the same snapshot; positions are relative to precondition + for m in matches: + sl, sc = line_col_from_index(m.start()) + el, ec = line_col_from_index(m.end()) + at_edits.append({"startLine": sl, "startCol": sc, "endLine": el, "endCol": ec, "newText": text_field}) @@ - elif op == "regex_replace": - pattern = e.get("pattern") or "" - repl = text_field - flags = _re.MULTILINE | (_re.IGNORECASE if e.get("ignore_case") else 0) - m = _re.search(pattern, current_text, flags) - if not m: - continue - sl, sc = line_col_from_index(m.start()) - el, ec = line_col_from_index(m.end()) - at_edits.append({ - "startLine": sl, - "startCol": sc, - "endLine": el, - "endCol": ec, - "newText": repl - }) - current_text = current_text[:m.start()] + repl + current_text[m.end():] + elif op == "regex_replace": + pattern = e.get("pattern") or "" + repl = text_field + count = int(e.get("count", 0) or 0) # 0 = all + flags = _re.MULTILINE | (_re.IGNORECASE if e.get("ignore_case") else 0) + base = current_text + matches = list(_re.finditer(pattern, base, flags)) + if not matches: + continue + if count > 0: + matches = matches[:count] + for m in matches: + sl, sc = line_col_from_index(m.start()) + el, ec = line_col_from_index(m.end()) + at_edits.append({"startLine": sl, "startCol": sc, "endLine": el, "endCol": ec, "newText": repl})Also applies to: 603-620
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
79-90: Guard early: ensure path resolves under Assets/ and validate edit schema before sending.Saves a round‑trip and yields clearer errors.
name, directory = _split_uri(uri) + if not directory or directory.split("/")[0].lower() != "assets": + return {"success": False, "code": "path_outside_assets", "message": f"URI must resolve under 'Assets/' (got '{directory or '<root>'}')."} + # Basic schema/range checks + required = {"startLine","startCol","endLine","endCol","newText"} + for i, e in enumerate(edits or []): + if not isinstance(e, dict) or not required.issubset(e): + return {"success": False, "code": "bad_edits", "message": f"edits[{i}] missing required keys"} + if (e["startLine"] < 1 or e["startCol"] < 1 or + e["endLine"] < e["startLine"] or + (e["endLine"] == e["startLine"] and e["endCol"] < e["startCol"])): + return {"success": False, "code": "bad_edits", "message": f"edits[{i}] has invalid range"}
🧹 Nitpick comments (6)
.github/workflows/claude-nl-suite.yml (1)
295-341: JUnit assembly: avoid regex fallback on XML; log parse errors instead.- except Exception: - txt = Path(frag).read_text(encoding='utf-8', errors='replace') - for m in re.findall(r'<testcase[\\s\\S]*?</testcase>', txt, flags=re.DOTALL): - try: suite.append(ET.fromstring(m)); added += 1 - except Exception: pass + except Exception as e: + print(f"[warn] failed to parse {frag}: {e}")UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (3)
444-445: Be tolerant decoding base64 contents.Unity may return encoded contents for non‑UTF8 clean files; decode with errors='replace' to avoid hard failures.
- contents = base64.b64decode(data["encodedContents"]).decode("utf-8") + contents = base64.b64decode(data["encodedContents"]).decode("utf-8", "replace")
60-66: Minor: avoid ambiguous variable name in closure.Rename generator variable to silence E741 and improve readability.
- def index_of(line: int, col: int) -> int: - if line <= len(lines): - return sum(len(l) for l in lines[: line - 1]) + (col - 1) + def index_of(line: int, col: int) -> int: + if line <= len(lines): + return sum(len(seg) for seg in lines[: line - 1]) + (col - 1) return sum(len(l) for l in lines)
652-665: Preview path returns success False when confirm=false; consider code=preview_required.Optional UX: return a machine‑parsable code to let callers branch cleanly.
- return _with_norm({"success": False, "message": "Preview diff; set options.confirm=true to apply.", "data": {"diff": "\n".join(diff)}}, normalized_for_echo, routing="text") + return _with_norm({"success": False, "code": "preview_required", "message": "Preview diff; set options.confirm=true to apply.", "data": {"diff": "\n".join(diff)}}, normalized_for_echo, routing="text")UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
53-56: POSIX absolute paths: dropping the leading “/” is surprising.Consider preserving the leading slash and letting downstream Assets/ guard reject out‑of‑project paths; current behavior rewrites “/tmp” → “tmp”.
- if effective_path.startswith("/"): - effective_path = effective_path[1:] + # Preserve POSIX absolute paths; downstream guards decide validity.
312-321: Keep capabilities in sync with editor limits.Editor currently caps apply_text_edits at 15 KiB (see ManageScript.cs). capabilities returns 256 KiB, which will mislead clients.
Align the advertised limit with the editor constant (or raise the editor limit). Do you want me to open a follow‑up to plumb this via a single source of truth?
📜 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 (6)
.claude/prompts/nl-unity-suite-full.md(1 hunks).github/workflows/claude-nl-suite.yml(1 hunks)UnityMcpBridge/Editor/Tools/ManageScript.cs(14 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py(1 hunks)claude-chunk.md(0 hunks)
💤 Files with no reviewable changes (1)
- claude-chunk.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T21:31:02.427Z
Learnt from: dsarno
PR: dsarno/unity-mcp#56
File: .claude/prompts/nl-unity-suite-full.md:5-6
Timestamp: 2025-08-29T21:31:02.427Z
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:
.claude/prompts/nl-unity-suite-full.md
🧬 Code graph analysis (3)
UnityMcpBridge/Editor/Tools/ManageScript.cs (2)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
Start(226-331)UnityMcpBridge/Editor/Helpers/Response.cs (3)
Response(10-61)Error(41-60)Success(18-33)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (4)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(378-397)tests/test_manage_script_uri.py (1)
tool(46-50)tests/test_resources_api.py (1)
tool(29-33)tests/test_script_tools.py (1)
tool(43-47)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (3)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(378-397)tests/test_manage_script_uri.py (1)
tool(46-50)tests/test_script_tools.py (1)
tool(43-47)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
73-73: Unused function argument: ctx
(ARG001)
97-97: Unused function argument: ctx
(ARG001)
135-135: Unused function argument: ctx
(ARG001)
151-151: Unused function argument: ctx
(ARG001)
175-175: Unused function argument: ctx
(ARG001)
247-248: try-except-pass detected, consider logging the exception
(S110)
247-247: Do not catch blind exception: Exception
(BLE001)
254-254: Do not catch blind exception: Exception
(BLE001)
295-295: Do not catch blind exception: Exception
(BLE001)
298-298: Use explicit conversion flag
Replace with conversion flag
(RUF010)
305-305: Unused function argument: ctx
(ARG001)
316-321: Consider moving this statement to an else block
(TRY300)
322-322: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py
23-25: Avoid specifying long messages outside the exception class
(TRY003)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
59-59: Avoid specifying long messages outside the exception class
(TRY003)
61-61: Function definition does not bind loop variable lines
(B023)
62-62: Ambiguous variable name: l
(E741)
62-62: Function definition does not bind loop variable lines
(B023)
63-63: Ambiguous variable name: l
(E741)
63-63: Function definition does not bind loop variable lines
(B023)
77-77: Avoid specifying long messages outside the exception class
(TRY003)
207-207: Unused function argument: ctx
(ARG001)
282-282: Comment contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF003)
282-282: Comment contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF003)
530-530: Do not catch blind exception: Exception
(BLE001)
649-649: Do not catch blind exception: Exception
(BLE001)
660-660: Consider [*diff[:800], "... (diff truncated) ..."] instead of concatenation
Replace with [*diff[:800], "... (diff truncated) ..."]
(RUF005)
664-664: Do not catch blind exception: Exception
(BLE001)
669-669: Do not catch blind exception: Exception
(BLE001)
680-680: Consider [*diff[:2000], "... (diff truncated) ..."] instead of concatenation
Replace with [*diff[:2000], "... (diff truncated) ..."]
(RUF005)
🪛 YAMLlint (1.37.1)
.github/workflows/claude-nl-suite.yml
[error] 5-5: trailing spaces
(trailing-spaces)
[error] 9-9: trailing spaces
(trailing-spaces)
[error] 13-13: trailing spaces
(trailing-spaces)
[error] 18-18: trailing spaces
(trailing-spaces)
[warning] 21-21: wrong indentation: expected 8 but found 6
(indentation)
[warning] 25-25: wrong indentation: expected 10 but found 8
(indentation)
[warning] 26-26: too many spaces after colon
(colons)
[error] 27-27: trailing spaces
(trailing-spaces)
[warning] 30-30: wrong indentation: expected 10 but found 8
(indentation)
[warning] 33-33: wrong indentation: expected 14 but found 12
(indentation)
[warning] 33-33: too many spaces after colon
(colons)
[warning] 34-34: too many spaces after colon
(colons)
[warning] 36-36: too many spaces after colon
(colons)
[error] 46-46: trailing spaces
(trailing-spaces)
[warning] 49-49: wrong indentation: expected 14 but found 12
(indentation)
[error] 50-50: trailing spaces
(trailing-spaces)
[warning] 54-54: wrong indentation: expected 14 but found 12
(indentation)
[error] 55-55: trailing spaces
(trailing-spaces)
[error] 73-73: trailing spaces
(trailing-spaces)
[warning] 79-79: wrong indentation: expected 14 but found 12
(indentation)
[warning] 79-79: too many spaces after colon
(colons)
[warning] 80-80: too many spaces after colon
(colons)
[warning] 82-82: too many spaces after colon
(colons)
[warning] 84-84: wrong indentation: expected 14 but found 12
(indentation)
[error] 88-88: trailing spaces
(trailing-spaces)
[error] 95-95: trailing spaces
(trailing-spaces)
[error] 102-102: trailing spaces
(trailing-spaces)
[warning] 107-107: wrong indentation: expected 14 but found 12
(indentation)
[warning] 107-107: too many spaces after colon
(colons)
[warning] 109-109: too many spaces after colon
(colons)
[error] 139-139: trailing spaces
(trailing-spaces)
[error] 179-179: trailing spaces
(trailing-spaces)
[error] 201-201: trailing spaces
(trailing-spaces)
[error] 208-208: trailing spaces
(trailing-spaces)
[error] 221-221: trailing spaces
(trailing-spaces)
[error] 267-267: trailing spaces
(trailing-spaces)
[error] 268-268: trailing spaces
(trailing-spaces)
[warning] 275-275: wrong indentation: expected 14 but found 12
(indentation)
[error] 293-293: trailing spaces
(trailing-spaces)
[error] 342-342: trailing spaces
(trailing-spaces)
[error] 442-442: trailing spaces
(trailing-spaces)
[error] 454-454: trailing spaces
(trailing-spaces)
[error] 466-466: trailing spaces
(trailing-spaces)
[error] 479-479: trailing spaces
(trailing-spaces)
[error] 496-496: trailing spaces
(trailing-spaces)
[error] 512-512: trailing spaces
(trailing-spaces)
[warning] 517-517: wrong indentation: expected 14 but found 12
(indentation)
[error] 523-523: trailing spaces
(trailing-spaces)
[warning] 528-528: wrong indentation: expected 14 but found 12
(indentation)
[error] 535-535: trailing spaces
(trailing-spaces)
[error] 542-542: no new line character at the end of file
(new-line-at-end-of-file)
[error] 542-542: trailing spaces
(trailing-spaces)
🪛 LanguageTool
.claude/prompts/nl-unity-suite-full.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...this once, verbatim, early in the run:** AllowedTools: Write,Bash(printf:*),Bash(...
(QB_NEW_EN)
[grammar] ~20-~20: There might be a mistake here.
Context: ...write. --- ## 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] ~24-~24: There might be a mistake here.
Context: ...the URI) - Relative (when supported): Assets/... - File paths for the helper script are wor...
(QB_NEW_EN)
[grammar] ~30-~30: There might be a mistake here.
Context: ...it-nl-suite.md(synthesized from JUnit) - Helper script:scripts/nlt-revert.sh` (...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ...(snapshot/restore) --- ## Tool Mapping - Anchors/regex/structured: `mcp__unity_...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...Mapping - Anchors/regex/structured: mcp__unity__script_apply_edits - Allowed ops: anchor_insert, `replace_r...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...overlapping ranges within a single call) - Precise ranges / atomic batch: `mcp__u...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...ply_text_edits(non‑overlapping ranges) - **Validation**:mcp__unity__validate_scri...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...n‑overlapping ranges) - Validation: mcp__unity__validate_script(level:"standard") - Reporting: Write small XML fragments...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...rting**: Write small XML fragments to reports/*_results.xml - Editor state/flush: `mcp__unity__manag...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...r(use sparingly; no project mutations) - **Console readback**:mcp__unity__read_co...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...ot assert in place of validate_script) - Snapshot/Restore: `Bash(scripts/nlt-re...
(QB_NEW_EN)
[grammar] ~43-~43: There might be a mistake here.
Context: ...lidate_script) - **Snapshot/Restore**: Bash(scripts/nlt-revert.sh:*) - Forscript_apply_edits: use name` + w...
(QB_NEW_EN)
[grammar] ~70-~70: There might be a mistake here.
Context: ...(T‑A/T‑E) - Do NOT use anchor_replace. - Use script_apply_edits with: ```json {...
(QB_NEW_EN)
[grammar] ~79-~79: There might be a mistake here.
Context: ...the method. # T‑B (replace method body) - Use mcp__unity__apply_text_edits with ...
(QB_NEW_EN)
[grammar] ~80-~80: There might be a mistake here.
Context: ... strictly inside the HasTarget braces. - Compute start/end from a fresh `read_res...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ...h read_resource at test start. Do not edit signature or header. - On `{status:"sta...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ... start. Do not edit signature or header. - On {status:"stale_file"} retry once wi...
(QB_NEW_EN)
[grammar] ~82-~82: There might be a mistake here.
Context: ...hash; if absent, re-read once and retry. - On bad_request: write the testcase wit...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ...ure>…, restore, and continue to next test. - On missing_field`: FALL B...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ...e>, restore, and continue to next test. - On missing_field`: FALL BACK per above;...
(QB_NEW_EN)
[style] ~84-~84: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..., restore, and continue to next test. - On missing_field: FALL BACK per above; i...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~91-~91: There might be a mistake here.
Context: ...tem-out>. - If content contains ]]>`, split CDATA: ...
(QB_NEW_EN)
[grammar] ~141-~141: There might be a mistake here.
Context: ...fore any further edits in the same test. - Prefer anchors (script_apply_edits) fo...
(QB_NEW_EN)
[grammar] ~144-~144: There might be a mistake here.
Context: ...rrors (timeout, EOF, connection closed):** - Write reports/<TESTID>_results.xml wit...
(QB_NEW_EN)
[grammar] ~145-~145: There might be a mistake here.
Context: ...<error> node capturing the error text. - Run the OS restore via `scripts/nlt-reve...
(QB_NEW_EN)
[grammar] ~150-~150: There might be a mistake here.
Context: ... evidence, and end with VERDICT: FAIL. - Run scripts/nlt-revert.sh restore ... ...
(QB_NEW_EN)
[grammar] ~154-~154: There might be a mistake here.
Context: ...T-E, T-F, T-G, T-H, T-I, T-J (15 total). - Before NL-1..T-J: Bash(scripts/nlt-rever...
(QB_NEW_EN)
[grammar] ~155-~155: There might be a mistake here.
Context: ...) IF the baseline exists; skip for NL-0. - NL-0 must include the PLAN line (len=15)...
(QB_NEW_EN)
[grammar] ~163-~163: There might be a mistake here.
Context: ...verify; delete PrintSeries(); restore. - NL‑2. Anchor comment — Insert `// Build ...
(QB_NEW_EN)
[grammar] ~164-~164: There might be a mistake here.
Context: ...bove public void Update(...); restore. - NL‑3. End‑of‑class — Insert `// Tail tes...
(QB_NEW_EN)
[grammar] ~165-~165: There might be a mistake here.
Context: ...— Insert // Tail test A/B/C (3 lines) before final brace; restore. - NL‑4. Compile t...
(QB_NEW_EN)
[grammar] ~165-~165: There might be a mistake here.
Context: ...` (3 lines) before final brace; restore. - NL‑4. Compile trigger — Record INFO only...
(QB_NEW_EN)
[grammar] ~168-~168: There might be a mistake here.
Context: ...fy; delete via regex_replace; restore. ### T‑B. Replace body — Single `replace_rang...
(QB_NEW_EN)
[grammar] ~169-~169: There might be a mistake here.
Context: ...lace_rangeinsideHasTarget`; restore. ### T‑C. Header/region preservation — Edit i...
(QB_NEW_EN)
[grammar] ~170-~170: There might be a mistake here.
Context: ...reserve signature/docs/regions; restore. ### T‑D. End‑of‑class (anchor) — Insert help...
(QB_NEW_EN)
[grammar] ~171-~171: There might be a mistake here.
Context: ...per before final brace; remove; restore. ### T‑E. Lifecycle — Insert → update → delet...
(QB_NEW_EN)
[grammar] ~172-~172: There might be a mistake here.
Context: ...rt → update → delete via regex; restore. ### T‑F. Atomic batch — One `mcp__unity__app...
(QB_NEW_EN)
[grammar] ~173-~173: There might be a mistake here.
Context: ...pply_text_edits` call (text ranges only) - Compute all three edits from the **same ...
(QB_NEW_EN)
[grammar] ~174-~174: There might be a mistake here.
Context: ...hree edits from the same fresh read: 1) Two small interior replace_range tweak...
(QB_NEW_EN)
[grammar] ~175-~175: There might be a mistake here.
Context: ...o small interior replace_range tweaks. 2) One end‑of‑class insertion: find the...
(QB_NEW_EN)
[grammar] ~176-~176: There might be a mistake here.
Context: ...eplacement` to the 3‑line comment block. - Send all three ranges in one call, s...
(QB_NEW_EN)
[grammar] ~178-~178: There might be a mistake here.
Context: ...…`, restore, and continue. - T‑G. Path normalization — Make the same ...
(QB_NEW_EN)
[grammar] ~183-~183: There might be a mistake here.
Context: ...nterior tweak (or none), then MUST call: mcp__unity__validate_script(level:"stand...
(QB_NEW_EN)
[grammar] ~184-~184: There might be a mistake here.
Context: ...unity__validate_script(level:"standard") - Write the validator output to system-out...
(QB_NEW_EN)
[grammar] ~195-~195: There might be a mistake here.
Context: ...a tiny legit edit with pre_sha; success. * Attempt another edit reusing the OLD pre...
(QB_NEW_EN)
[grammar] ~198-~198: There might be a mistake here.
Context: ..., restore. - (3) USING_GUARD (optional): * Attempt a 1-line insert above the first ...
(QB_NEW_EN)
[grammar] ~204-~204: There might be a mistake here.
Context: ...aseline. - Repeat a replace_range twice (second call may be noop). Validate stand...
(QB_NEW_EN)
[grammar] ~204-~204: There might be a mistake here.
Context: ... a replace_range twice (second call may be noop). Validate standard after each. - ...
(QB_NEW_EN)
[grammar] ~204-~204: There might be a mistake here.
Context: ..._range twice (second call may be noop). Validate standard after each. - Insert or ensure...
(QB_NEW_EN)
[grammar] ~204-~204: There might be a mistake here.
Context: ... be noop). Validate standard after each. - Insert or ensure a tiny comment, then de...
(QB_NEW_EN)
[grammar] ~205-~205: There might be a mistake here.
Context: ...re a tiny comment, then delete it twice (second delete may be noop). - Restore an...
(QB_NEW_EN)
[grammar] ~205-~205: There might be a mistake here.
Context: ...then delete it twice (second delete may be noop). - Restore and PASS unless an err...
(QB_NEW_EN)
[grammar] ~205-~205: There might be a mistake here.
Context: ...te it twice (second delete may be noop). - Restore and PASS unless an error/structu...
(QB_NEW_EN)
[grammar] ~211-~211: There might be a mistake here.
Context: ...uses are non‑fatal; record and continue. - End each testcase <system-out> with `V...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
.claude/prompts/nl-unity-suite-full.md
92-92: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
102-102: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
168-168: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
169-169: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
170-170: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
171-171: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
172-172: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
174-174: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
177-177: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
178-178: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
179-179: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 0
(MD005, list-indent)
190-190: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
191-191: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
192-192: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
194-194: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
195-195: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
196-196: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
197-197: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
199-199: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
200-200: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
⏰ 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 (7)
.claude/prompts/nl-unity-suite-full.md (4)
51-57: Regex anchor upgraded for CRLF — looks good.
73-76: Multiline flag on deletion regex — good catch.
154-156: Restore guard before NL‑0 — correct.
5-7: RemoveBash(printf:*)andBash(echo:*)from AllowedTools in.claude/prompts/nl-unity-suite-full.md. They’re not listed underallowed_toolsin.github/workflows/claude-nl-suite.ymland must align.⛔ Skipped due to learnings
Learnt from: dsarno PR: dsarno/unity-mcp#56 File: .claude/prompts/nl-unity-suite-full.md:5-6 Timestamp: 2025-08-29T21:31:02.427Z 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.UnityMcpBridge/Editor/Tools/ManageScript.cs (3)
717-725: Great: return sha256 in apply_text_edits response.This enables precondition chaining on the client. Thanks.
1280-1291: Structured edits now include sha256 and schedule refresh sensibly.Looks good and symmetric with text‑edits flow.
229-236: All Regex calls include timeouts
Confirmed everyRegex.Match,Regex.IsMatch, andnew Regexinvocation in ManageScript.cs uses the timeout overload.
| private const int MaxEditPayloadBytes = 15 * 1024; | ||
|
|
||
| private static object ApplyTextEdits( |
There was a problem hiding this comment.
Raise edit payload cap (15 KiB is too low and inconsistent with server tooling).
This will reject routine edits and contradicts capabilities. Bump to 256 KiB (or read from config).
- private const int MaxEditPayloadBytes = 15 * 1024;
+ private const int MaxEditPayloadBytes = 256 * 1024;📝 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.
| private const int MaxEditPayloadBytes = 15 * 1024; | |
| private static object ApplyTextEdits( | |
| private const int MaxEditPayloadBytes = 256 * 1024; | |
| private static object ApplyTextEdits( |
🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/Tools/ManageScript.cs around lines 456 to 458, the
MaxEditPayloadBytes constant is set to 15 * 1024 which is too low; update it to
256 * 1024 (256 KiB) or, preferably, make it configurable by reading a value
from configuration with a sensible fallback of 256 * 1024; change the constant
to either the new literal or replace it with a config read + fallback and ensure
any uses respect the new value.
| 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) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a timeout to auto‑upgrade header regex.
This regex can hang on adversarial input without a timeout.
- 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 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*\(",
+ RegexOptions.CultureInvariant | RegexOptions.Multiline,
+ TimeSpan.FromMilliseconds(500)
+ );📝 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 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) | |
| 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*\(", | |
| RegexOptions.CultureInvariant | RegexOptions.Multiline, | |
| TimeSpan.FromMilliseconds(500) | |
| ); | |
| var mh = rx.Match(slice); | |
| if (mh.Success) |
🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/Tools/ManageScript.cs around lines 557 to 559, the
header-detection regex can hang on adversarial input because it’s created
without a match timeout; update the code to construct or invoke the regex with a
reasonable timeout (e.g. 250–1000 ms) using the Regex constructor overload or
Match overload that accepts a TimeSpan and pass a TimeSpan timeout value, so any
pathological input will abort instead of hanging the process.
| "options": {"refresh": "immediate", "validate": (options or {}).get("validate", "standard")} | ||
| } | ||
| resp_text = send_command_with_retry("manage_script", params_text) | ||
| if not (isinstance(resp_text, dict) and resp_text.get("success")): | ||
| return _with_norm(resp_text if isinstance(resp_text, dict) else {"success": False, "message": str(resp_text)}, normalized_for_echo, routing="mixed/text-first") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure sequential application on server to match locally computed spans.
Mixed-path text edits are computed sequentially but are sent without options.applyMode. Force sequential to avoid offset issues.
- "options": {"refresh": "immediate", "validate": (options or {}).get("validate", "standard")}
+ "options": {"refresh": "immediate",
+ "validate": (options or {}).get("validate", "standard"),
+ "applyMode": "sequential"}📝 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.
| "options": {"refresh": "immediate", "validate": (options or {}).get("validate", "standard")} | |
| } | |
| resp_text = send_command_with_retry("manage_script", params_text) | |
| if not (isinstance(resp_text, dict) and resp_text.get("success")): | |
| return _with_norm(resp_text if isinstance(resp_text, dict) else {"success": False, "message": str(resp_text)}, normalized_for_echo, routing="mixed/text-first") | |
| "options": { | |
| "refresh": "immediate", | |
| "validate": (options or {}).get("validate", "standard"), | |
| "applyMode": "sequential" | |
| } | |
| } | |
| resp_text = send_command_with_retry("manage_script", params_text) | |
| if not (isinstance(resp_text, dict) and resp_text.get("success")): | |
| return _with_norm( | |
| resp_text if isinstance(resp_text, dict) else {"success": False, "message": str(resp_text)}, | |
| normalized_for_echo, | |
| routing="mixed/text-first" | |
| ) |
🤖 Prompt for AI Agents
In UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py around lines
525 to 529, the manage_script request is sent without setting options.applyMode
so mixed-path text edits computed locally (which are sequential) can be applied
out of order on the server; update the params_text (or options within it) to
include options.applyMode = "sequential" (ensure options exists or merge into
params) before calling send_command_with_retry so the server applies edits in
sequential mode and offsets remain correct.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
UnityMcpBridge/Editor/Tools/ManageScript.cs (2)
557-559: Add a timeout to the auto-upgrade header regex.
Reintroduces a no-timeout Regex that can hang on pathological inputs. Use a bounded constructor.- 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 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*\(", + System.Text.RegularExpressions.RegexOptions.CultureInvariant | System.Text.RegularExpressions.RegexOptions.Multiline, + TimeSpan.FromMilliseconds(250) + );
880-905: Check ancestor symlinks for structured edits as well (consistency + safety).Only the target file is checked here, unlike ApplyTextEdits which walks ancestors. Mirror the stricter check to avoid escape via symlinked parents.
- // Refuse edits if the target is a symlink + // Refuse edits if the target or any ancestor is a symlink try { - var attrs = File.GetAttributes(fullPath); - if ((attrs & FileAttributes.ReparsePoint) != 0) - return Response.Error("Refusing to edit a symlinked script path."); + var di = new DirectoryInfo(Path.GetDirectoryName(fullPath) ?? ""); + while (di != null && !string.Equals(di.FullName.Replace('\\','/'), Application.dataPath.Replace('\\','/'), StringComparison.OrdinalIgnoreCase)) + { + if (di.Exists && (di.Attributes & FileAttributes.ReparsePoint) != 0) + return Response.Error("Refusing to edit a symlinked script path."); + di = di.Parent; + } } catch { // ignore failures checking attributes and proceed }
🧹 Nitpick comments (3)
UnityMcpBridge/Editor/Tools/ManageScript.cs (3)
453-458: Bump or make edit payload limit configurable (15 KiB is too low).Routine refactors will exceed 15 KiB; align with transport/server caps or allow EditorPrefs override.
- private const int MaxEditPayloadBytes = 15 * 1024; + private const int MaxEditPayloadBytes = 256 * 1024; // consider EditorPrefs override if needed
528-547: Header guard may block legitimate using edits; add an explicit override.The guard prevents top-of-file edits (license headers/usings). Consider an opt-in (e.g., options.allowUsingHeaderEdits=true) restricted to trusted callers.
1113-1201: Tighten anchor_ regex options and timeouts.*Use CultureInvariant and a smaller timeout to keep the editor responsive under adversarial anchors.
- var rx = new Regex(anchor, RegexOptions.Multiline, TimeSpan.FromSeconds(2)); + var rx = new Regex(anchor, RegexOptions.CultureInvariant | RegexOptions.Multiline, TimeSpan.FromMilliseconds(250)); @@ - var rx = new Regex(anchor, RegexOptions.Multiline, TimeSpan.FromSeconds(2)); + var rx = new Regex(anchor, RegexOptions.CultureInvariant | RegexOptions.Multiline, TimeSpan.FromMilliseconds(250)); @@ - var rx = new Regex(anchor, RegexOptions.Multiline, TimeSpan.FromSeconds(2)); + var rx = new Regex(anchor, RegexOptions.CultureInvariant | RegexOptions.Multiline, TimeSpan.FromMilliseconds(250));
📜 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 (1)
UnityMcpBridge/Editor/Tools/ManageScript.cs(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/Tools/ManageScript.cs (1)
UnityMcpBridge/Editor/Helpers/Response.cs (3)
Response(10-61)Error(41-60)Success(18-33)
⏰ 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 (6)
UnityMcpBridge/Editor/Tools/ManageScript.cs (6)
150-151: LGTM: name validation hardened with timeout.
Timeout + CultureInvariant is appropriate for editor paths.
226-244: LGTM: validation handler with bounded regex and structured diags.
Bounded regex + severity parsing looks solid.
313-335: LGTM: atomic create without BOM and deferred refresh.
Safe write pattern and scheduling look good.
671-726: LGTM: atomic write + selectable refresh + returned SHA.
Good idempotency signal and editor responsiveness.
1491-1665: LGTM: method-span detection with bounded regex and robust brace walk.
Good handling of generics, constraints, and expression-bodied members.
2420-2486: LGTM: debounced import/compile with coalescing.
Thread-safe set + tick-based debounce looks solid.
| /// <summary> | ||
| /// Resolves a directory under Assets/, preventing traversal and escaping. | ||
| /// Returns fullPathDir on disk and canonical 'Assets/...' relative path. | ||
| /// </summary> | ||
| private static bool TryResolveUnderAssets(string relDir, out string fullPathDir, out string relPathSafe) | ||
| { | ||
| string assets = Application.dataPath.Replace('\\', '/'); | ||
|
|
||
| // Normalize caller path: allow both "Scripts/..." and "Assets/Scripts/..." | ||
| string rel = (relDir ?? "Scripts").Replace('\\', '/').Trim(); | ||
| if (string.IsNullOrEmpty(rel)) rel = "Scripts"; | ||
| if (rel.StartsWith("Assets/", StringComparison.OrdinalIgnoreCase)) rel = rel.Substring(7); | ||
| rel = rel.TrimStart('/'); | ||
|
|
||
| string targetDir = Path.Combine(assets, rel).Replace('\\', '/'); | ||
| string full = Path.GetFullPath(targetDir).Replace('\\', '/'); | ||
|
|
||
| bool underAssets = full.StartsWith(assets + "/", StringComparison.OrdinalIgnoreCase) | ||
| || string.Equals(full, assets, StringComparison.OrdinalIgnoreCase); | ||
| if (!underAssets) | ||
| { | ||
| fullPathDir = null; | ||
| relPathSafe = null; | ||
| return false; | ||
| } | ||
|
|
||
| // Best-effort symlink guard: if the directory OR ANY ANCESTOR (up to Assets/) is a reparse point/symlink, reject | ||
| try | ||
| { | ||
| var di = new DirectoryInfo(full); | ||
| while (di != null) | ||
| { | ||
| if (di.Exists && (di.Attributes & FileAttributes.ReparsePoint) != 0) | ||
| { | ||
| fullPathDir = null; | ||
| relPathSafe = null; | ||
| return false; | ||
| } | ||
| var atAssets = string.Equals( | ||
| di.FullName.Replace('\\','/'), | ||
| assets, | ||
| StringComparison.OrdinalIgnoreCase | ||
| ); | ||
| if (atAssets) break; | ||
| di = di.Parent; | ||
| } | ||
| } | ||
| catch { /* best effort; proceed */ } | ||
|
|
||
| fullPathDir = full; | ||
| string tail = full.Length > assets.Length ? full.Substring(assets.Length).TrimStart('/') : string.Empty; | ||
| relPathSafe = ("Assets/" + tail).TrimEnd('/'); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fail closed on symlink-attribute check errors in path resolution.
If attribute probing throws, the code proceeds, weakening the guard. Default-deny keeps edits confined under Assets/ even when FS state is ambiguous.
- catch { /* best effort; proceed */ }
+ catch
+ {
+ fullPathDir = null;
+ relPathSafe = null;
+ return false; // fail closed if we can't verify ancestors
+ }📝 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.
| /// <summary> | |
| /// Resolves a directory under Assets/, preventing traversal and escaping. | |
| /// Returns fullPathDir on disk and canonical 'Assets/...' relative path. | |
| /// </summary> | |
| private static bool TryResolveUnderAssets(string relDir, out string fullPathDir, out string relPathSafe) | |
| { | |
| string assets = Application.dataPath.Replace('\\', '/'); | |
| // Normalize caller path: allow both "Scripts/..." and "Assets/Scripts/..." | |
| string rel = (relDir ?? "Scripts").Replace('\\', '/').Trim(); | |
| if (string.IsNullOrEmpty(rel)) rel = "Scripts"; | |
| if (rel.StartsWith("Assets/", StringComparison.OrdinalIgnoreCase)) rel = rel.Substring(7); | |
| rel = rel.TrimStart('/'); | |
| string targetDir = Path.Combine(assets, rel).Replace('\\', '/'); | |
| string full = Path.GetFullPath(targetDir).Replace('\\', '/'); | |
| bool underAssets = full.StartsWith(assets + "/", StringComparison.OrdinalIgnoreCase) | |
| || string.Equals(full, assets, StringComparison.OrdinalIgnoreCase); | |
| if (!underAssets) | |
| { | |
| fullPathDir = null; | |
| relPathSafe = null; | |
| return false; | |
| } | |
| // Best-effort symlink guard: if the directory OR ANY ANCESTOR (up to Assets/) is a reparse point/symlink, reject | |
| try | |
| { | |
| var di = new DirectoryInfo(full); | |
| while (di != null) | |
| { | |
| if (di.Exists && (di.Attributes & FileAttributes.ReparsePoint) != 0) | |
| { | |
| fullPathDir = null; | |
| relPathSafe = null; | |
| return false; | |
| } | |
| var atAssets = string.Equals( | |
| di.FullName.Replace('\\','/'), | |
| assets, | |
| StringComparison.OrdinalIgnoreCase | |
| ); | |
| if (atAssets) break; | |
| di = di.Parent; | |
| } | |
| } | |
| catch { /* best effort; proceed */ } | |
| fullPathDir = full; | |
| string tail = full.Length > assets.Length ? full.Substring(assets.Length).TrimStart('/') : string.Empty; | |
| relPathSafe = ("Assets/" + tail).TrimEnd('/'); | |
| return true; | |
| } | |
| /// <summary> | |
| /// Resolves a directory under Assets/, preventing traversal and escaping. | |
| /// Returns fullPathDir on disk and canonical 'Assets/...' relative path. | |
| /// </summary> | |
| private static bool TryResolveUnderAssets(string relDir, out string fullPathDir, out string relPathSafe) | |
| { | |
| string assets = Application.dataPath.Replace('\\', '/'); | |
| // Normalize caller path: allow both "Scripts/..." and "Assets/Scripts/..." | |
| string rel = (relDir ?? "Scripts").Replace('\\', '/').Trim(); | |
| if (string.IsNullOrEmpty(rel)) rel = "Scripts"; | |
| if (rel.StartsWith("Assets/", StringComparison.OrdinalIgnoreCase)) rel = rel.Substring(7); | |
| rel = rel.TrimStart('/'); | |
| string targetDir = Path.Combine(assets, rel).Replace('\\', '/'); | |
| string full = Path.GetFullPath(targetDir).Replace('\\', '/'); | |
| bool underAssets = full.StartsWith(assets + "/", StringComparison.OrdinalIgnoreCase) | |
| || string.Equals(full, assets, StringComparison.OrdinalIgnoreCase); | |
| if (!underAssets) | |
| { | |
| fullPathDir = null; | |
| relPathSafe = null; | |
| return false; | |
| } | |
| // Best-effort symlink guard: if the directory OR ANY ANCESTOR (up to Assets/) is a reparse point/symlink, reject | |
| try | |
| { | |
| var di = new DirectoryInfo(full); | |
| while (di != null) | |
| { | |
| if (di.Exists && (di.Attributes & FileAttributes.ReparsePoint) != 0) | |
| { | |
| fullPathDir = null; | |
| relPathSafe = null; | |
| return false; | |
| } | |
| var atAssets = string.Equals( | |
| di.FullName.Replace('\\','/'), | |
| assets, | |
| StringComparison.OrdinalIgnoreCase | |
| ); | |
| if (atAssets) break; | |
| di = di.Parent; | |
| } | |
| } | |
| catch | |
| { | |
| fullPathDir = null; | |
| relPathSafe = null; | |
| return false; // fail closed if we can't verify ancestors | |
| } | |
| fullPathDir = full; | |
| string tail = full.Length > assets.Length ? full.Substring(assets.Length).TrimStart('/') : string.Empty; | |
| relPathSafe = ("Assets/" + tail).TrimEnd('/'); | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/Tools/ManageScript.cs around lines 54 to 107, the
symlink/attribute check currently swallows exceptions in the try/catch and
continues (weakening the safety guard); change the catch so the function fails
closed: on any exception from DirectoryInfo/attribute probing set fullPathDir =
null, relPathSafe = null and return false (optionally log the exception), rather
than ignoring it, so ambiguous filesystem states reject the path.
…xt conversion and translate to \g<n> in local apply; keeps previews and actual edits consistent.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (5)
405-412: Route anchor_insert via text-edits, not structured.anchor_insert is currently included in STRUCT and structured_kinds, so it will be sent down the structured "edit" path and skipped by the apply_text_edits conversion. This re-introduces the misrouting flagged earlier and breaks parity with the text-only/mixed conversion logic you added below. Remove anchor_insert from STRUCT/structured_kinds and add it to TEXT so anchor_insert consistently uses apply_text_edits with preconditions and sequential semantics.
- STRUCT = {"replace_class","delete_class","replace_method","delete_method","insert_method","anchor_delete","anchor_replace","anchor_insert"} - TEXT = {"prepend","append","replace_range","regex_replace"} + STRUCT = {"replace_class","delete_class","replace_method","delete_method","insert_method","anchor_delete","anchor_replace"} + TEXT = {"prepend","append","replace_range","regex_replace","anchor_insert"} @@ - structured_kinds = {"replace_class","delete_class","replace_method","delete_method","insert_method","anchor_insert"} + structured_kinds = STRUCT # keep a single source of truthAlso applies to: 561-563
520-533: Mixed-path text apply: set options.applyMode="sequential" to match locally mutated offsets.You compute spans against a locally mutated snapshot (current_text), but send them without applyMode. Unity defaults to atomic ordering, which can misapply later spans. Add applyMode="sequential".
- params_text: Dict[str, Any] = { + params_text: Dict[str, Any] = { "action": "apply_text_edits", "name": name, "path": path, "namespace": namespace, "scriptType": script_type, "edits": at_edits, "precondition_sha256": sha, - "options": {"refresh": "immediate", "validate": (options or {}).get("validate", "standard")} + "options": { + "refresh": "immediate", + "validate": (options or {}).get("validate", "standard"), + "applyMode": "sequential" + } }
580-585: anchor_insert (text-only conversion) should respect ignore_case.Your mixed-path handler honors ignore_case; the text-only path does not.
- if op == "anchor_insert": + if op == "anchor_insert": anchor = e.get("anchor") or "" position = (e.get("position") or "before").lower() - m = _re.search(anchor, current_text, _re.MULTILINE) + flags = _re.MULTILINE | (_re.IGNORECASE if e.get("ignore_case") else 0) + m = _re.search(anchor, current_text, flags)
609-625: regex_replace (text-only): only first match; ignores count. Match parity with mixed path.Implement sequential multi-match with count and ignore_case to keep conversion consistent.
- elif op == "regex_replace": - pattern = e.get("pattern") or "" - repl = text_field - flags = _re.MULTILINE | (_re.IGNORECASE if e.get("ignore_case") else 0) - m = _re.search(pattern, current_text, flags) - if not m: - continue - sl, sc = line_col_from_index(m.start()) - el, ec = line_col_from_index(m.end()) - at_edits.append({ - "startLine": sl, - "startCol": sc, - "endLine": el, - "endCol": ec, - "newText": repl - }) - current_text = current_text[:m.start()] + repl + current_text[m.end():] + elif op == "regex_replace": + pattern = e.get("pattern") or "" + flags = _re.MULTILINE | (_re.IGNORECASE if e.get("ignore_case") else 0) + count = int(e.get("count", 0) or 0) # 0 = all + replaced = 0 + while True: + m = _re.search(pattern, current_text, flags) + if not m: + break + sl, sc = line_col_from_index(m.start()) + el, ec = line_col_from_index(m.end()) + at_edits.append({"startLine": sl, "startCol": sc, "endLine": el, "endCol": ec, "newText": text_field}) + current_text = current_text[:m.start()] + text_field + current_text[m.end():] + replaced += 1 + if count > 0 and replaced >= count: + break
492-505: regex_replace in mixed batches: only first match; ignores count/ignore_case sequencing.Support multiple matches with count=0/all and ignore_case, and compute spans sequentially against the evolving text to keep positions consistent.
- elif opx == "regex_replace": - pattern = e.get("pattern") or "" - m = _re.search(pattern, current_text, _re.MULTILINE) - if not m: - continue - # Expand $1, $2... in replacement using this match - def _expand_dollars(rep: str) -> str: - return _re.sub(r"\$(\d+)", lambda g: m.group(int(g.group(1))) or "", rep) - repl = _expand_dollars(text_field) - sl, sc = line_col_from_index(m.start()) - el, ec = line_col_from_index(m.end()) - at_edits.append({"startLine": sl, "startCol": sc, "endLine": el, "endCol": ec, "newText": repl}) - current_text = current_text[:m.start()] + repl + current_text[m.end():] + elif opx == "regex_replace": + pattern = e.get("pattern") or "" + count = int(e.get("count", 0) or 0) # 0 = all + flags = _re.MULTILINE | (_re.IGNORECASE if e.get("ignore_case") else 0) + replaced = 0 + while True: + m = _re.search(pattern, current_text, flags) + if not m: + break + # Expand $1.. using this match + def _expand_dollars(rep: str) -> str: + return _re.sub(r"\$(\d+)", lambda g: m.group(int(g.group(1))) or "", rep) + repl = _expand_dollars(text_field) + sl, sc = line_col_from_index(m.start()) + el, ec = line_col_from_index(m.end()) + at_edits.append({"startLine": sl, "startCol": sc, "endLine": el, "endCol": ec, "newText": repl}) + current_text = current_text[:m.start()] + repl + current_text[m.end():] + replaced += 1 + if count > 0 and replaced >= count: + break
🧹 Nitpick comments (5)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (5)
109-118: Normalize Windows-style paths and stray prefixes.Replace backslashes with slashes during normalization so file://C:... and Assets... resolve consistently.
- n = (name or "").strip() - p = (path or "").strip() + n = (name or "").strip().replace("\\", "/") + p = (path or "").strip().replace("\\", "/") @@ - def strip_prefix(s: str) -> str: + def strip_prefix(s: str) -> str: + s = s.replace("\\", "/") if s.startswith("unity://path/"): return s[len("unity://path/"):] if s.startswith("file://"): return s[len("file://"):] return s
217-221: Silence unused ctx param without changing the tool signature.Avoid linter noise while keeping Context available for future diagnostics.
def script_apply_edits( ctx: Context, name: str, path: str, edits: List[Dict[str, Any]], @@ - ) -> Dict[str, Any]: + ) -> Dict[str, Any]: + _ = ctx # intentionally unused; reserved for future tracing/telemetry
536-537: Narrow overly broad exception catches.Catching bare Exception makes failures opaque. Limit to expected error types (regex, value, key) and surface messages.
- except Exception as e: + except (re.error, ValueError, KeyError) as e: return _with_norm({"success": False, "message": f"Text edit conversion failed: {e}"}, normalized_for_echo, routing="mixed/text-first") @@ - except Exception as e: + except (re.error, ValueError, KeyError) as e: return _with_norm({"success": False, "code": "conversion_failed", "message": f"Edit conversion failed: {e}"}, normalized_for_echo, routing="text") @@ - except Exception as e: + except (re.error, ValueError, KeyError) as e: return _with_norm({"success": False, "code": "preview_failed", "message": f"Preview failed: {e}"}, normalized_for_echo, routing="text") @@ - except Exception as e: + except (re.error, ValueError, KeyError) as e: return {"success": False, "message": f"Edit application failed: {e}"}Also applies to: 655-656, 670-671, 675-676
284-296: Replace non-breaking hyphen in comment with ASCII hyphen.Avoid RUF003 and copy/paste gremlins in docs.
- # CI compatibility (T‑A/T‑E): + # CI compatibility (T-A/T-E):
665-666: Micro: build truncated diff lists with literal unpacking.Satisfies RUF005 and is a touch cleaner.
- if len(diff) > 800: - diff = diff[:800] + ["... (diff truncated) ..."] + if len(diff) > 800: + diff = [*diff[:800], "... (diff truncated) ..."] @@ - if len(diff) > 2000: - diff = diff[:2000] + ["... (diff truncated) ..."] + if len(diff) > 2000: + diff = [*diff[:2000], "... (diff truncated) ..."]Also applies to: 685-686
📜 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 (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (3)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(378-397)tests/test_manage_script_uri.py (1)
tool(46-50)tests/test_script_tools.py (1)
tool(43-47)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py
23-25: Avoid specifying long messages outside the exception class
(TRY003)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
59-59: Avoid specifying long messages outside the exception class
(TRY003)
61-61: Function definition does not bind loop variable lines
(B023)
62-62: Ambiguous variable name: l
(E741)
62-62: Function definition does not bind loop variable lines
(B023)
63-63: Ambiguous variable name: l
(E741)
63-63: Function definition does not bind loop variable lines
(B023)
79-79: Avoid specifying long messages outside the exception class
(TRY003)
209-209: Unused function argument: ctx
(ARG001)
284-284: Comment contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF003)
284-284: Comment contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF003)
499-499: Function definition does not bind loop variable m
(B023)
536-536: Do not catch blind exception: Exception
(BLE001)
655-655: Do not catch blind exception: Exception
(BLE001)
666-666: Consider [*diff[:800], "... (diff truncated) ..."] instead of concatenation
Replace with [*diff[:800], "... (diff truncated) ..."]
(RUF005)
670-670: Do not catch blind exception: Exception
(BLE001)
675-675: Do not catch blind exception: Exception
(BLE001)
686-686: Consider [*diff[:2000], "... (diff truncated) ..."] instead of concatenation
Replace with [*diff[:2000], "... (diff truncated) ..."]
(RUF005)
⏰ 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 (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
413-431: Structured path LGTM, with a caveat.Deterministic ordering and immediate refresh are correct. Please confirm Unity’s manage_script "edit" handler fully supports anchor_* ops; if not, anchor ops should always go via apply_text_edits (see earlier routing fix).
| elif op == "replace_range": | ||
| start_line = int(edit.get("startLine", 1)) | ||
| start_col = int(edit.get("startCol", 1)) | ||
| end_line = int(edit.get("endLine", start_line)) | ||
| end_col = int(edit.get("endCol", 1)) | ||
| replacement = edit.get("text", "") | ||
| lines = text.splitlines(keepends=True) | ||
| max_line = len(lines) + 1 # 1-based, exclusive end | ||
| if (start_line < 1 or end_line < start_line or end_line > max_line | ||
| or start_col < 1 or end_col < 1): | ||
| raise RuntimeError("replace_range out of bounds") | ||
| def index_of(line: int, col: int) -> int: | ||
| if line <= len(lines): | ||
| return sum(len(l) for l in lines[: line - 1]) + (col - 1) | ||
| return sum(len(l) for l in lines) | ||
| a = index_of(start_line, start_col) | ||
| b = index_of(end_line, end_col) | ||
| text = text[:a] + replacement + text[b:] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
replace_range: clamp columns to line bounds; current index_of can overshoot.
If startCol/endCol exceed a line’s visible length, index_of can jump past the intended line, producing surprising replacements. Clamp to [1, visible_len(line)+1] and compute offsets accordingly.
- def index_of(line: int, col: int) -> int:
- if line <= len(lines):
- return sum(len(l) for l in lines[: line - 1]) + (col - 1)
- return sum(len(l) for l in lines)
+ def _visible_len(s: str) -> int:
+ return len(s[:-1]) if s.endswith("\n") else len(s)
+ def index_of(line: int, col: int) -> int:
+ if line <= len(lines):
+ base = sum(len(x) for x in lines[: line - 1])
+ max_col = _visible_len(lines[line - 1]) + 1 # 1-based, allow EOL insertion
+ c = max(1, min(col, max_col))
+ return base + (c - 1)
+ return sum(len(x) for x in lines)📝 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.
| elif op == "replace_range": | |
| start_line = int(edit.get("startLine", 1)) | |
| start_col = int(edit.get("startCol", 1)) | |
| end_line = int(edit.get("endLine", start_line)) | |
| end_col = int(edit.get("endCol", 1)) | |
| replacement = edit.get("text", "") | |
| lines = text.splitlines(keepends=True) | |
| max_line = len(lines) + 1 # 1-based, exclusive end | |
| if (start_line < 1 or end_line < start_line or end_line > max_line | |
| or start_col < 1 or end_col < 1): | |
| raise RuntimeError("replace_range out of bounds") | |
| def index_of(line: int, col: int) -> int: | |
| if line <= len(lines): | |
| return sum(len(l) for l in lines[: line - 1]) + (col - 1) | |
| return sum(len(l) for l in lines) | |
| a = index_of(start_line, start_col) | |
| b = index_of(end_line, end_col) | |
| text = text[:a] + replacement + text[b:] | |
| elif op == "replace_range": | |
| start_line = int(edit.get("startLine", 1)) | |
| start_col = int(edit.get("startCol", 1)) | |
| end_line = int(edit.get("endLine", start_line)) | |
| end_col = int(edit.get("endCol", 1)) | |
| replacement = edit.get("text", "") | |
| lines = text.splitlines(keepends=True) | |
| max_line = len(lines) + 1 # 1-based, exclusive end | |
| if (start_line < 1 or end_line < start_line or end_line > max_line | |
| or start_col < 1 or end_col < 1): | |
| raise RuntimeError("replace_range out of bounds") | |
| def _visible_len(s: str) -> int: | |
| return len(s[:-1]) if s.endswith("\n") else len(s) | |
| def index_of(line: int, col: int) -> int: | |
| if line <= len(lines): | |
| base = sum(len(x) for x in lines[: line - 1]) | |
| max_col = _visible_len(lines[line - 1]) + 1 # 1-based, allow EOL insertion | |
| c = max(1, min(col, max_col)) | |
| return base + (c - 1) | |
| return sum(len(x) for x in lines) | |
| a = index_of(start_line, start_col) | |
| b = index_of(end_line, end_col) | |
| text = text[:a] + replacement + text[b:] |
🧰 Tools
🪛 Ruff (0.12.2)
59-59: Avoid specifying long messages outside the exception class
(TRY003)
61-61: Function definition does not bind loop variable lines
(B023)
62-62: Ambiguous variable name: l
(E741)
62-62: Function definition does not bind loop variable lines
(B023)
63-63: Ambiguous variable name: l
(E741)
63-63: Function definition does not bind loop variable lines
(B023)
…nes in Python conversion paths; C# path ensures trailing newline and skips duplicate insertion within class.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
UnityMcpBridge/Editor/Tools/ManageScript.cs (4)
549-612: Gate auto-upgrade (text→structured) behind an option to avoid surprising edits.Auto-upgrade can mask intended simple edits. Add an opt-in flag (e.g., options.autoUpgrade=true) and default to current behavior for back-compat.
If you want, I can provide a small follow-up patch wiring HandleCommand → ApplyTextEdits(optionsAutoUpgrade).
557-559: Add timeout/options to auto-upgrade header regex (prevent UI hangs).This regex lacks an explicit timeout; pathological inputs can stall the editor. This was flagged before; please harden it.
- 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 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*\(", + System.Text.RegularExpressions.RegexOptions.CultureInvariant | System.Text.RegularExpressions.RegexOptions.Multiline, + TimeSpan.FromMilliseconds(250) + );#!/bin/bash # List Regex constructions lacking a TimeSpan timeout in this file rg -nP -C1 --type cs $'new\\s+Regex\\s*\\(' UnityMcpBridge/Editor/Tools/ManageScript.cs rg -nP -C1 --type cs $'Regex\\.(Match|IsMatch)\\(' UnityMcpBridge/Editor/Tools/ManageScript.cs
80-107: Fail closed on symlink-attribute probe errors in TryResolveUnderAssets (path safety).If attribute probing throws, the guard is bypassed. Reject on exception to prevent symlink-based escapes.
- } - catch { /* best effort; proceed */ } + } + catch + { + // Fail closed if we cannot verify ancestors + fullPathDir = null; + relPathSafe = null; + return false; + }
892-905: Check ancestor symlinks (not just the file) in EditScript and fail closed on probe errors.Mirror the ancestor-walk used in ApplyTextEdits; don’t proceed on exceptions.
- // Refuse edits if the target is a symlink + // Refuse edits if the target or any ancestor is a symlink try { - var attrs = File.GetAttributes(fullPath); - if ((attrs & FileAttributes.ReparsePoint) != 0) - return Response.Error("Refusing to edit a symlinked script path."); + var di = new DirectoryInfo(Path.GetDirectoryName(fullPath) ?? ""); + var assetsRoot = Application.dataPath.Replace('\\','/'); + while (di != null && !string.Equals(di.FullName.Replace('\\','/'), assetsRoot, StringComparison.OrdinalIgnoreCase)) + { + if (di.Exists && (di.Attributes & FileAttributes.ReparsePoint) != 0) + return Response.Error("Refusing to edit a symlinked script path."); + di = di.Parent; + } } - catch - { - // ignore failures checking attributes and proceed - } + catch + { + return Response.Error("Refusing to edit a symlinked script path (attributes check failed)."); + }UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (4)
405-412: anchor_insert routing is inconsistent across sets; confirm intended path.It’s classified as STRUCT here but also converted to text-edits elsewhere. Pick one to avoid divergent behavior (recommended: treat anchor_* as text-edits).
- STRUCT = {"replace_class","delete_class","replace_method","delete_method","insert_method","anchor_delete","anchor_replace","anchor_insert"} - TEXT = {"prepend","append","replace_range","regex_replace"} + STRUCT = {"replace_class","delete_class","replace_method","delete_method","insert_method"} + TEXT = {"prepend","append","replace_range","regex_replace","anchor_insert","anchor_delete","anchor_replace"} @@ - structured_kinds = {"replace_class","delete_class","replace_method","delete_method","insert_method","anchor_insert"} + structured_kinds = {"replace_class","delete_class","replace_method","delete_method","insert_method"}Also applies to: 567-569
537-538: Mixed path: send applyMode=sequential to match locally computed offsets.Without this, server may apply atomically and misalign later spans.
- "options": {"refresh": "immediate", "validate": (options or {}).get("validate", "standard")} + "options": {"refresh": "immediate", + "validate": (options or {}).get("validate", "standard"), + "applyMode": "sequential"}
49-66: replace_range: clamp columns to line bounds; fix closure capture and E741.Out-of-range columns can jump into the next line; also fix late-binding of
linesand ambiguousl.elif op == "replace_range": @@ - def index_of(line: int, col: int) -> int: - if line <= len(lines): - return sum(len(l) for l in lines[: line - 1]) + (col - 1) - return sum(len(l) for l in lines) + def _visible_len(s: str) -> int: + return len(s[:-1]) if s.endswith("\n") else len(s) + def index_of(line: int, col: int, lines_snapshot=lines) -> int: + if line <= len(lines_snapshot): + base = sum(len(seg) for seg in lines_snapshot[: line - 1]) + max_col = _visible_len(lines_snapshot[line - 1]) + 1 # [1..EOL+1] + c = max(1, min(col, max_col)) + return base + (c - 1) + return sum(len(seg) for seg in lines_snapshot)
498-510: regex_replace (mixed path): honors only first match, ignores count/ignore_case.Apply sequentially with flags and $n expansion; produce multiple spans.
- elif opx == "regex_replace": - pattern = e.get("pattern") or "" - m = _re.search(pattern, current_text, _re.MULTILINE) - if not m: - continue - # Expand $1, $2... in replacement using this match - def _expand_dollars(rep: str) -> str: - return _re.sub(r"\$(\d+)", lambda g: m.group(int(g.group(1))) or "", rep) - repl = _expand_dollars(text_field) - sl, sc = line_col_from_index(m.start()) - el, ec = line_col_from_index(m.end()) - at_edits.append({"startLine": sl, "startCol": sc, "endLine": el, "endCol": ec, "newText": repl}) - current_text = current_text[:m.start()] + repl + current_text[m.end():] + elif opx == "regex_replace": + pattern = e.get("pattern") or "" + count = int(e.get("count", 0) or 0) # 0 = all + flags = _re.MULTILINE | (_re.IGNORECASE if e.get("ignore_case") else 0) + applied, pos = 0, 0 + while True: + m = _re.search(pattern, current_text[pos:], flags) + if not m or (count > 0 and applied >= count): + break + start = pos + m.start() + end = pos + m.end() + def _expand_dollars(rep: str) -> str: + return _re.sub(r"\$(\d+)", lambda g: m.group(int(g.group(1))) or "", rep) + repl = _expand_dollars(text_field) + sl, sc = line_col_from_index(start) + el, ec = line_col_from_index(end) + at_edits.append({"startLine": sl, "startCol": sc, "endLine": el, "endCol": ec, "newText": repl}) + current_text = current_text[:start] + repl + current_text[end:] + pos = start + len(repl) + applied += 1
🧹 Nitpick comments (8)
UnityMcpBridge/Editor/Tools/ManageScript.cs (3)
453-457: Raise edit payload cap or make it configurable.15 KiB will block routine refactors and contradicts the transport’s much larger frame cap. Suggest 256 KiB with EditorPrefs override.
- private const int MaxEditPayloadBytes = 15 * 1024; + private const int MaxEditPayloadBytes = 256 * 1024; // consider EditorPrefs override, e.g. "MCPForUnity_MaxEditBytes"
734-767: Support one-past-EOL positions in TryIndexFromLineCol (common edit APIs).Edits often use endCol = lineLength+1. Current scan can reject such endpoints. Map beyond-EOL to line end.
- private static bool TryIndexFromLineCol(string text, int line1, int col1, out int index) - { - // 1-based line/col to absolute index (0-based), col positions are counted in code points - int line = 1, col = 1; - for (int i = 0; i <= text.Length; i++) - { - if (line == line1 && col == col1) - { - index = i; - return true; - } - if (i == text.Length) break; - char c = text[i]; - if (c == '\r') - { - // Treat CRLF as a single newline; skip the LF if present - if (i + 1 < text.Length && text[i + 1] == '\n') - i++; - line++; - col = 1; - } - else if (c == '\n') - { - line++; - col = 1; - } - else - { - col++; - } - } - index = -1; - return false; - } + private static bool TryIndexFromLineCol(string text, int line1, int col1, out int index) + { + if (line1 < 1 || col1 < 1) { index = -1; return false; } + int line = 1, i = 0, len = text.Length; + // Advance to the requested line + while (line < line1 && i < len) + { + char c = text[i++]; + if (c == '\r') { if (i < len && text[i] == '\n') i++; line++; } + else if (c == '\n') { line++; } + } + if (line != line1) { index = -1; return false; } + int lineStart = i; + // Find end of this line + while (i < len && text[i] != '\r' && text[i] != '\n') i++; + int lineEnd = i; // position of newline or len + // Clamp col to [1, lineLength+1] + int offset = Math.Max(0, Math.Min(col1 - 1, lineEnd - lineStart)); + index = lineStart + offset; + return true; + }
2434-2501: Debounced refresh looks solid; small nit: add max-age guard.Consider a max wait (e.g., 2s) to avoid starvation under a constant stream.
Example: if (DateTime.UtcNow - _lastRequest > TimeSpan.FromSeconds(2)) force a run.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (5)
37-49: Local preview: normalize newline padding for anchor_insert to match server conversion.Prevents “jammed” insertions and makes preview diffs consistent.
- elif op == "anchor_insert": + elif op == "anchor_insert": anchor = edit.get("anchor", "") position = (edit.get("position") or "before").lower() insert_text = edit.get("text", "") flags = re.MULTILINE | (re.IGNORECASE if edit.get("ignore_case") else 0) m = re.search(anchor, text, flags) if not m: if edit.get("allow_noop", True): continue raise RuntimeError(f"anchor not found: {anchor}") idx = m.start() if position == "before" else m.end() - text = text[:idx] + insert_text + text[idx:] + it = insert_text + 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:]
209-216: Silence unusedctx.Avoids Ruff ARG001; keeps signature intact.
def script_apply_edits( ctx: Context, name: str, path: str, edits: List[Dict[str, Any]], options: Dict[str, Any] | None = None, script_type: str = "MonoBehaviour", namespace: str = "", ) -> Dict[str, Any]: - # Normalize locator first so downstream calls target the correct script file. + _ = ctx # context unused + # Normalize locator first so downstream calls target the correct script file.Also applies to: 217-219
542-544: Narrow overly broad exception handling; include error codes.Catching bare Exception hinders diagnostics; prefer specific exceptions or attach type info.
- except Exception as e: - return _with_norm({"success": False, "message": f"Text edit conversion failed: {e}"}, normalized_for_echo, routing="mixed/text-first") + except (re.error, ValueError, RuntimeError) as e: + return _with_norm({"success": False, "code": e.__class__.__name__, "message": str(e)}, normalized_for_echo, routing="mixed/text-first") @@ - except Exception as e: - return _with_norm({"success": False, "code": "conversion_failed", "message": f"Edit conversion failed: {e}"}, normalized_for_echo, routing="text") + except (re.error, ValueError, RuntimeError) as e: + return _with_norm({"success": False, "code": e.__class__.__name__, "message": str(e)}, normalized_for_echo, routing="text") @@ - except Exception as e: - return {"success": False, "message": f"Edit application failed: {e}"} + except (re.error, ValueError, RuntimeError) as e: + return {"success": False, "code": e.__class__.__name__, "message": str(e)}Also applies to: 666-668, 681-687
284-284: Replace non-breaking hyphen with ASCII hyphen in comment.Prevents RUF003 and invisible diffs.
- # CI compatibility (T‑A/T‑E): + # CI compatibility (T-A/T-E):
453-561: Potential race between text-edits and structured edits.Text edits use a precondition SHA; structured ‘edit’ call doesn’t. If supported, pass a precondition/version to
editor re-read and re-validate before applying.
📜 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/ManageScript.cs(14 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (3)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(378-397)tests/test_manage_script_uri.py (1)
tool(46-50)tests/test_script_tools.py (1)
tool(43-47)
UnityMcpBridge/Editor/Tools/ManageScript.cs (2)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
Start(226-331)UnityMcpBridge/Editor/Helpers/Response.cs (3)
Response(10-61)Error(41-60)Success(18-33)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py
23-25: Avoid specifying long messages outside the exception class
(TRY003)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
59-59: Avoid specifying long messages outside the exception class
(TRY003)
61-61: Function definition does not bind loop variable lines
(B023)
62-62: Ambiguous variable name: l
(E741)
62-62: Function definition does not bind loop variable lines
(B023)
63-63: Ambiguous variable name: l
(E741)
63-63: Function definition does not bind loop variable lines
(B023)
79-79: Avoid specifying long messages outside the exception class
(TRY003)
209-209: Unused function argument: ctx
(ARG001)
284-284: Comment contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF003)
284-284: Comment contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF003)
505-505: Function definition does not bind loop variable m
(B023)
542-542: Do not catch blind exception: Exception
(BLE001)
666-666: Do not catch blind exception: Exception
(BLE001)
677-677: Consider [*diff[:800], "... (diff truncated) ..."] instead of concatenation
Replace with [*diff[:800], "... (diff truncated) ..."]
(RUF005)
681-681: Do not catch blind exception: Exception
(BLE001)
686-686: Do not catch blind exception: Exception
(BLE001)
697-697: Consider [*diff[:2000], "... (diff truncated) ..."] instead of concatenation
Replace with [*diff[:2000], "... (diff truncated) ..."]
(RUF005)
⏰ 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)
UnityMcpBridge/Editor/Tools/ManageScript.cs (3)
229-236: Nice: diagnostics regex now has a timeout.Prevents stalls on bad inputs.
311-336: LGTM: atomic create without BOM + deferred refresh.Matches update/apply paths and avoids spurious BOM diffs.
669-726: LGTM: atomic write, refresh control, and SHA returned from ApplyTextEdits.Good for client precondition chaining and CI determinism.
| if (!File.Exists(fullPath)) | ||
| return Response.Error($"Script not found at '{relativePath}'."); | ||
| // Refuse edits if the target or any ancestor is a symlink | ||
| try | ||
| { | ||
| var di = new DirectoryInfo(Path.GetDirectoryName(fullPath) ?? ""); | ||
| while (di != null && !string.Equals(di.FullName.Replace('\\','/'), Application.dataPath.Replace('\\','/'), StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| if (di.Exists && (di.Attributes & FileAttributes.ReparsePoint) != 0) | ||
| return Response.Error("Refusing to edit a symlinked script path."); | ||
| di = di.Parent; | ||
| } | ||
| } | ||
| catch | ||
| { | ||
| // If checking attributes fails, proceed without the symlink guard | ||
| } | ||
| if (edits == null || edits.Count == 0) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fail closed on symlink check errors for ApplyTextEdits.
You walk ancestors (good), but exceptions currently fall through and disable the guard.
- }
- catch
- {
- // If checking attributes fails, proceed without the symlink guard
- }
+ }
+ catch
+ {
+ return Response.Error("Refusing to edit a symlinked script path (attributes check failed).");
+ }📝 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.
| if (!File.Exists(fullPath)) | |
| return Response.Error($"Script not found at '{relativePath}'."); | |
| // Refuse edits if the target or any ancestor is a symlink | |
| try | |
| { | |
| var di = new DirectoryInfo(Path.GetDirectoryName(fullPath) ?? ""); | |
| while (di != null && !string.Equals(di.FullName.Replace('\\','/'), Application.dataPath.Replace('\\','/'), StringComparison.OrdinalIgnoreCase)) | |
| { | |
| if (di.Exists && (di.Attributes & FileAttributes.ReparsePoint) != 0) | |
| return Response.Error("Refusing to edit a symlinked script path."); | |
| di = di.Parent; | |
| } | |
| } | |
| catch | |
| { | |
| // If checking attributes fails, proceed without the symlink guard | |
| } | |
| if (edits == null || edits.Count == 0) | |
| if (!File.Exists(fullPath)) | |
| return Response.Error($"Script not found at '{relativePath}'."); | |
| // Refuse edits if the target or any ancestor is a symlink | |
| try | |
| { | |
| var di = new DirectoryInfo(Path.GetDirectoryName(fullPath) ?? ""); | |
| while (di != null && !string.Equals(di.FullName.Replace('\\','/'), Application.dataPath.Replace('\\','/'), StringComparison.OrdinalIgnoreCase)) | |
| { | |
| if (di.Exists && (di.Attributes & FileAttributes.ReparsePoint) != 0) | |
| return Response.Error("Refusing to edit a symlinked script path."); | |
| di = di.Parent; | |
| } | |
| } | |
| catch | |
| { | |
| return Response.Error("Refusing to edit a symlinked script path (attributes check failed)."); | |
| } | |
| if (edits == null || edits.Count == 0) |
🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/Tools/ManageScript.cs around lines 466 to 483, the
symlink-guard currently swallows all exceptions and disables the guard; change
the behavior to fail-closed by returning an error when the symlink attribute
check throws. Replace the empty catch with a catch that returns a Response.Error
(with a concise message like "Refusing to edit script: symlink check failed") so
any exception during ancestor attribute inspection prevents edits; keep the
existing ancestor-walking logic and only alter the catch to return the error.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py
Outdated
Show resolved
Hide resolved
…reflight+strict; no-op evidence in C#; update NL suite prompt; add unit tests
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (17)
UnityMcpBridge/Editor/Tools/ManageScript.cs (4)
481-486: Raise MaxEditPayloadBytes (or make it configurable).15 KiB is too low for routine edits and contradicts the transport’s larger frame cap. Suggest 256 KiB or a config-based value.
- private const int MaxEditPayloadBytes = 15 * 1024; + private const int MaxEditPayloadBytes = 256 * 1024;
80-107: Fail closed on symlink-attribute errors in TryResolveUnderAssets.If attribute probing throws, the guard is disabled, enabling path-escape via symlinked ancestors. Fail closed.
} - catch { /* best effort; proceed */ } + catch + { + fullPathDir = null; + relPathSafe = null; + return false; // fail closed if we can't verify ancestors + }
494-511: Fail closed on ApplyTextEdits symlink guard errors.Proceeding on exception weakens the protection; return an error instead.
} - catch - { - // If checking attributes fails, proceed without the symlink guard - } + catch + { + return Response.Error("Refusing to edit a symlinked script path (attributes check failed)."); + }
585-587: Add timeout/options to auto‑upgrade header regex.Construct the regex with a small match timeout to avoid UI stalls on pathological inputs.
- 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 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*\(", + System.Text.RegularExpressions.RegexOptions.CultureInvariant | System.Text.RegularExpressions.RegexOptions.Multiline, + TimeSpan.FromMilliseconds(250) + );UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (6)
526-539: Include options.applyMode="sequential" for text spans (mixed path).Server applies atomic by default; send sequential to preserve locally computed offsets.
- params_text: Dict[str, Any] = { + params_text: Dict[str, Any] = { "action": "apply_text_edits", "name": name, "path": path, "namespace": namespace, "scriptType": script_type, "edits": at_edits, "precondition_sha256": sha, - "options": {"refresh": "immediate", "validate": (options or {}).get("validate", "standard")} + "options": {"refresh": "immediate", + "validate": (options or {}).get("validate", "standard"), + "applyMode": "sequential"} }
582-607: Text path: add ignore_case + accept 'replacement' alias for anchor_insert.Parity with other branches; improves UX and consistency.
- # aliasing for text field - text_field = e.get("text") or e.get("insert") or e.get("content") or "" + # aliasing for text field + text_field = e.get("text") or e.get("insert") or e.get("content") or e.get("replacement") or "" @@ - if op == "anchor_insert": + if op == "anchor_insert": anchor = e.get("anchor") or "" position = (e.get("position") or "after").lower() - m = _re.search(anchor, current_text, _re.MULTILINE) + flags = _re.MULTILINE | (_re.IGNORECASE if e.get("ignore_case") else 0) + m = _re.search(anchor, current_text, flags)
579-607: Text path: support prepend/append conversion to spans.Avoid falling back to whole‑file update when only text ops are provided.
for e in edits or []: op = (e.get("op") or e.get("operation") or e.get("type") or e.get("mode") or "").strip().lower() # aliasing for text field text_field = e.get("text") or e.get("insert") or e.get("content") or "" if op == "anchor_insert": ... + elif op in ("prepend","append"): + if op == "prepend": + sl, sc = 1, 1 + at_edits.append({"startLine": sl, "startCol": sc, "endLine": sl, "endCol": sc, "newText": text_field}) + current_text = text_field + current_text + else: + eof_idx = len(current_text) + sl, sc = line_col_from_index(eof_idx) + new_text = ("" if current_text.endswith("\n") else "\n") + text_field + at_edits.append({"startLine": sl, "startCol": sc, "endLine": sl, "endCol": sc, "newText": new_text}) + current_text = current_text + new_text elif op == "replace_range":
49-66: Clamp columns to visible line bounds in replace_range.Current index math can overshoot for cols > line length; clamp to [1, len(line)+1].
- def index_of(line: int, col: int) -> int: - if line <= len(lines): - return sum(len(l) for l in lines[: line - 1]) + (col - 1) - return sum(len(l) for l in lines) + def _visible_len(s: str) -> int: + return len(s[:-1]) if s.endswith("\n") else len(s) + def index_of(line: int, col: int) -> int: + if line <= len(lines): + base = sum(len(x) for x in lines[: line - 1]) + max_col = _visible_len(lines[line - 1]) + 1 # allow EOL insertion + c = max(1, min(col, max_col)) + return base + (c - 1) + return sum(len(x) for x in lines)
498-541: regex_replace (mixed path): honor ignore_case/count and handle multiple matches.Currently first-match only and ignores flags/count. Apply sequentially with $n expansion per match.
- elif opx == "regex_replace": - pattern = e.get("pattern") or "" - m = _re.search(pattern, current_text, _re.MULTILINE) - if not m: - continue - # Expand $1, $2... in replacement using this match - def _expand_dollars(rep: str) -> str: - return _re.sub(r"\$(\d+)", lambda g: m.group(int(g.group(1))) or "", rep) - repl = _expand_dollars(text_field) - sl, sc = line_col_from_index(m.start()) - el, ec = line_col_from_index(m.end()) - at_edits.append({"startLine": sl, "startCol": sc, "endLine": el, "endCol": ec, "newText": repl}) - current_text = current_text[:m.start()] + repl + current_text[m.end():] + elif opx == "regex_replace": + pattern = e.get("pattern") or "" + flags = _re.MULTILINE | (_re.IGNORECASE if e.get("ignore_case") else 0) + count = int(e.get("count", 0) or 0) # 0 = all + applied, pos = 0, 0 + while True: + m = _re.search(pattern, current_text[pos:], flags) + if not m or (count > 0 and applied >= count): + break + start = pos + m.start() + end = pos + m.end() + repl = _re.sub(r"\$(\d+)", lambda g: m.group(int(g.group(1))) or "", text_field) + sl, sc = line_col_from_index(start) + el, ec = line_col_from_index(end) + at_edits.append({"startLine": sl, "startCol": sc, "endLine": el, "endCol": ec, "newText": repl}) + current_text = current_text[:start] + repl + current_text[end:] + pos = start + len(repl) + applied += 1
620-641: Text path: regex_replace should support multiple matches, count, and $n per match.Current code replaces only the first match.
- elif op == "regex_replace": - pattern = e.get("pattern") or "" - repl = text_field - flags = _re.MULTILINE | (_re.IGNORECASE if e.get("ignore_case") else 0) - m = _re.search(pattern, current_text, flags) - if not m: - continue - # Expand $1, $2... backrefs in replacement using the first match (consistent with mixed-path behavior) - def _expand_dollars(rep: str) -> str: - return _re.sub(r"\$(\d+)", lambda g: m.group(int(g.group(1))) or "", rep) - repl_expanded = _expand_dollars(repl) - sl, sc = line_col_from_index(m.start()) - el, ec = line_col_from_index(m.end()) - at_edits.append({ - "startLine": sl, - "startCol": sc, - "endLine": el, - "endCol": ec, - "newText": repl_expanded - }) - current_text = current_text[:m.start()] + repl_expanded + current_text[m.end():] + elif op == "regex_replace": + pattern = e.get("pattern") or "" + repl = text_field + count = int(e.get("count", 0) or 0) # 0 = all + flags = _re.MULTILINE | (_re.IGNORECASE if e.get("ignore_case") else 0) + applied, pos = 0, 0 + while True: + m = _re.search(pattern, current_text[pos:], flags) + if not m or (count > 0 and applied >= count): + break + start = pos + m.start() + end = pos + m.end() + repl_expanded = _re.sub(r"\$(\d+)", lambda g: m.group(int(g.group(1))) or "", repl) + sl, sc = line_col_from_index(start) + el, ec = line_col_from_index(end) + at_edits.append({"startLine": sl, "startCol": sc, "endLine": el, "endCol": ec, "newText": repl_expanded}) + current_text = current_text[:start] + repl_expanded + current_text[end:] + pos = start + len(repl_expanded) + applied += 1UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
53-56: Regression: don’t strip POSIX leading “/” outside Assets.This reintroduces the bug we previously discussed; it turns absolute paths into relative ones and breaks guards/routing. Remove the slash-stripping.
- # For POSIX absolute paths outside Assets, drop the leading '/' - # to return a clean relative-like directory (e.g., '/tmp' -> 'tmp'). - if effective_path.startswith("/"): - effective_path = effective_path[1:]
62-71: apply_text_edits lacks the promised Assets/ guard.Docs say the path “must resolve under Assets/,” but no check is enforced. Add the guard to prevent server-side rejection or unsafe paths.
"""Apply small text edits to a C# script identified by URI.""" - name, directory = _split_uri(uri) + name, directory = _split_uri(uri) + if not directory or directory.split("/")[0].lower() != "assets": + return { + "success": False, + "code": "path_outside_assets", + "message": f"URI must resolve under 'Assets/'; got '{directory or '<root>'}'." + }Also applies to: 80-81
.claude/prompts/nl-unity-suite-full.md (2)
5-7: Sync AllowedTools with workflow (drop Bash printf/echo).The workflow only allows Bash(scripts/nlt-revert.sh:*). Listing Bash(printf|echo) here will cause tool-denied failures.
Apply:
-AllowedTools: Write,Bash(printf:*),Bash(echo:*),Bash(scripts/nlt-revert.sh:*),mcp__unity__manage_editor,mcp__unity__list_resources,mcp__unity__read_resource,mcp__unity__apply_text_edits,mcp__unity__script_apply_edits,mcp__unity__validate_script,mcp__unity__find_in_file,mcp__unity__read_console,mcp__unity__get_sha +AllowedTools: Write,Bash(scripts/nlt-revert.sh:*),mcp__unity__manage_editor,mcp__unity__list_resources,mcp__unity__read_resource,mcp__unity__apply_text_edits,mcp__unity__script_apply_edits,mcp__unity__validate_script,mcp__unity__find_in_file,mcp__unity__read_console,mcp__unity__get_shaTo confirm repo-wide consistency, run:
#!/bin/bash rg -nP '^AllowedTools:' .claude/prompts -n rg -nP 'allowed_tools:' .github/workflows/claude-nl-suite*.yml -n -A5
96-108: Fix filesystem-mutation rules (allow Write + revert helper, drop printf/echo note).Current text forbids all FS mutations except revert, but reporting requires Write. Also, printf/echo aren’t in AllowedTools after syncing.
- **Never** open/patch `$JUNIT_OUT` or `$MD_OUT`; CI merges fragments and synthesizes Markdown. - Write destinations must match: `^reports/[A-Za-z0-9._-]+_results\.xml$` - Snapshot files must live under `reports/_snapshots/` - Reject absolute paths and any path containing `..` - Reject control characters and line breaks in filenames; enforce UTF‑8 - Cap basename length to ≤64 chars; cap any path segment to ≤100 and total path length to ≤255 - - Bash(printf|echo) must write to stdout only. Do not use shell redirection, here‑docs, or `tee` to create/modify files. The only allowed FS mutation is via `scripts/nlt-revert.sh`. + - Only two actions may modify the filesystem: + 1) Write → `reports/*_results.xml` (per rules above) + 2) `Bash(scripts/nlt-revert.sh:*)` → snapshot/restore + - No shell redirection / here‑docs / tee anywhere..github/workflows/claude-nl-suite.yml (3)
20-27: Expose junit/md paths as job outputs (optional).Useful for downstream jobs without re-parsing env.
nl-suite: if: github.event_name == 'workflow_dispatch' runs-on: ubuntu-latest timeout-minutes: 60 + outputs: + junit_path: ${{ steps.expose_paths.outputs.junit_out }} + md_path: ${{ steps.expose_paths.outputs.md_out }} env: JUNIT_OUT: reports/junit-nl-suite.xml MD_OUT: reports/junit-nl-suite.md @@ - name: Prepare reports and dirs run: | set -eux rm -f reports/*.xml reports/*.md || true mkdir -p reports reports/_snapshots scripts + +- name: Expose output paths + id: expose_paths + run: | + echo "junit_out=${JUNIT_OUT}" >> "$GITHUB_OUTPUT" + echo "md_out=${MD_OUT}" >> "$GITHUB_OUTPUT"
223-257: Harden snapshot helper against path traversal and bad filenames.Agents pass the snapshot path; current script allows arbitrary destinations.
cat > scripts/nlt-revert.sh <<'BASH' #!/usr/bin/env bash set -euo pipefail sub="${1:-}"; target_rel="${2:-}"; snap="${3:-}" WS="${GITHUB_WORKSPACE:-$PWD}" ROOT="$WS/TestProjects/UnityMCPTests" t_abs="$(realpath -m "$WS/$target_rel")" -s_abs="$(realpath -m "$WS/$snap")" +SNAP_ROOT="$WS/reports/_snapshots" +case "$snap" in + /*|*'..'* ) echo "refuse: invalid snapshot path input" >&2; exit 2;; +esac +s_abs="$(realpath -m "$SNAP_ROOT/$snap")" if [[ "$t_abs" != "$ROOT/Assets/"* ]]; then echo "refuse: target outside allowed scope: $t_abs" >&2; exit 2 fi +if [[ "$s_abs" != "$SNAP_ROOT/"* ]]; then + echo "refuse: snapshot path outside reports/_snapshots: $s_abs" >&2; exit 2 +fi +# basic hygiene: control chars, segment length/charset +if printf '%s' "$s_abs" | grep -q '[[:cntrl:]]'; then echo "refuse: control chars in path" >&2; exit 2; fi +rel="${s_abs#$SNAP_ROOT/}"; IFS='/' read -r -a segs <<< "$rel" +for seg in "${segs[@]}"; do [[ -z "$seg" || ${#seg} -gt 100 || ! "$seg" =~ ^[A-Za-z0-9._-]+$ ]] && { echo "refuse: bad segment: $seg" >&2; exit 2; }; done +(( ${#s_abs} <= 255 )) || { echo "refuse: path too long" >&2; exit 2; } mkdir -p "$(dirname "$s_abs")" case "$sub" in snapshot)
279-290: Align workflowallowed_toolswith prompt declarations
.github/workflows/claude-nl-suite.ymlhas removedBash(printf:*)andBash(echo:*), but.claude/prompts/nl-unity-suite-full.mdstill declares them in itsAllowedTools:line—either re-add them to the workflow or drop them from the prompt. Apply the same update to the mini prompt (.claude/prompts/nl-unity-claude-tests-mini.md).
🧹 Nitpick comments (13)
tests/test_get_sha.py (1)
55-74: Strengthen assertions to verify payload shape.Also assert sha256 presence to catch regressions in routing/data plumbing.
resp = get_sha(None, uri="unity://path/Assets/Scripts/A.cs") assert captured["cmd"] == "manage_script" assert captured["params"]["action"] == "get_sha" assert captured["params"]["name"] == "A" assert captured["params"]["path"].endswith("Assets/Scripts") assert resp["success"] is True + assert resp.get("data", {}).get("sha256") == "abc"tests/test_edit_normalization_and_noop.py (1)
66-76: Verify index‑pair normalization result.Add an assertion to ensure [0,0] maps to (1,1) in the outgoing payload.
apply(None, uri="unity://path/Assets/Scripts/F.cs", edits=edits, precondition_sha256="x") - # last call is apply_text_edits + # last call is apply_text_edits; verify normalized coordinates + p = calls[-1] + e = p["edits"][0] + assert e["startLine"] == 1 and e["startCol"] == 1 + assert e["endLine"] == 1 and e["endCol"] == 1UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
405-412: Avoid duplicating op classification sets.Define STRUCT once and reuse to prevent drift (note anchor_insert appears in both paths today).
- STRUCT = {"replace_class","delete_class","replace_method","delete_method","insert_method","anchor_delete","anchor_replace","anchor_insert"} + STRUCT = {"replace_class","delete_class","replace_method","delete_method","insert_method","anchor_delete","anchor_replace","anchor_insert"} TEXT = {"prepend","append","replace_range","regex_replace"} @@ - structured_kinds = {"replace_class","delete_class","replace_method","delete_method","insert_method","anchor_insert"} + structured_kinds = STRUCTAlso applies to: 567-569
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (6)
15-22: Update _split_uri docstring to match current behavior (don’t claim we strip leading slashes).The implementation preserves POSIX leading slashes and supports UNC hosts; the docstring still says “strip host and leading slashes.” Please align the docs to avoid confusion.
- - file://... → percent-decode, normalize, strip host and leading slashes, - then, if any 'Assets' segment exists, return path relative to that 'Assets' root. + - file://... → percent-decode and normalize; preserve leading slashes and + include UNC hosts (//server/share). If any 'Assets' segment exists, + return the path relative to that 'Assets' root.
104-110: Decode only when contents is None; add robust base64 decode.Avoid decoding when contents == "" and tolerate bad/missing encoded payloads.
- 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 Exception: - contents = contents or "" + contents = data.get("contents") + if contents is None and data.get("contentsEncoded"): + try: + b64 = (data.get("encodedContents") or "").encode("utf-8") + contents = base64.b64decode(b64).decode("utf-8", "replace") + except Exception: + contents = ""
459-466: Harden decode on read responses and keep shapes consistent.Add errors='replace' and guard missing keys.
- decoded_contents = base64.b64decode(response["data"]["encodedContents"]).decode('utf-8') + b64 = (response["data"].get("encodedContents") or "").encode("utf-8") + decoded_contents = base64.b64decode(b64).decode("utf-8", "replace")
476-480: Lint: prefer explicit conversion flag and avoid bare Exception.Ruff RUF010 suggests {e!s}. Also narrow or at least bind the exception.
- except Exception as e: + except Exception as e: return { "success": False, - "message": f"Python error managing script: {str(e)}", + "message": f"Python error managing script: {e!s}", }
73-79: Silence ARG001 (unused ctx) without changing signatures.Delete or bind ctx at function start to keep FastMCP signatures stable.
def apply_text_edits( ctx: Context, uri: str, edits: List[Dict[str, Any]], precondition_sha256: str | None = None, strict: bool | None = None, ) -> Dict[str, Any]: + del ctx @@ def create_script( ctx: Context, path: str, contents: str = "", script_type: str | None = None, namespace: str | None = None, ) -> Dict[str, Any]: + del ctx @@ - def delete_script(ctx: Context, uri: str) -> Dict[str, Any]: + def delete_script(ctx: Context, uri: str) -> Dict[str, Any]: + del ctx @@ - def validate_script( - ctx: Context, uri: str, level: str = "basic" - ) -> Dict[str, Any]: + def validate_script(ctx: Context, uri: str, level: str = "basic") -> Dict[str, Any]: + del ctx @@ def manage_script( ctx: Context, action: str, name: str, path: str, contents: str = "", script_type: str | None = None, namespace: str | None = None, ) -> Dict[str, Any]: + del ctx @@ - def manage_script_capabilities(ctx: Context) -> Dict[str, Any]: + def manage_script_capabilities(ctx: Context) -> Dict[str, Any]: + del ctx @@ - def get_sha(ctx: Context, uri: str) -> Dict[str, Any]: + def get_sha(ctx: Context, uri: str) -> Dict[str, Any]: + del ctxAlso applies to: 278-283, 316-316, 332-334, 355-363, 486-486, 513-514
135-136: Replace silent try/except/pass with debug logging.Swallowing these makes normalization failures opaque; log at debug to aid triage without spamming.
- except Exception: - pass + except Exception as e: + # non-fatal normalization issue; continue with best effort + import logging as _log + _log.getLogger(__name__).debug("edit normalization warning: %r", e)Also applies to: 145-146, 178-179, 199-200, 208-209, 232-234
.claude/prompts/nl-unity-suite-full.md (1)
186-189: Status token formatting consistency.Use
{status:"stale_file"}(not{stale_file}) to match earlier sections and server schema.- - Expect all‑or‑nothing semantics; on `{status:"overlap"}` or `{status:"bad_request"}`, write the testcase fragment with `<failure>…</failure>`, **restore**, and continue. - - T‑G. Path normalization — Make the same edit with `unity://path/Assets/...` then `Assets/...`. Without refreshing `precondition_sha256`, the second attempt returns `{stale_file}`; retry with the server-provided hash to confirm both forms resolve to the same file. + - Expect all‑or‑nothing semantics; on `{status:"overlap"}` or `{status:"bad_request"}`, write the testcase fragment with `<failure>…</failure>`, **restore**, and continue. + - T‑G. Path normalization — Make the same edit with `unity://path/Assets/...` then `Assets/...`. Without refreshing `precondition_sha256`, the second attempt returns `{status:"stale_file"}`; retry with the server-provided hash to confirm both forms resolve to the same file..github/workflows/claude-nl-suite.yml (3)
148-176: Broaden log redaction (cover key=value, emails, more keywords).Extend sed to mask common secret patterns in streamed and tail logs.
-docker logs -f unity-mcp 2>&1 | sed -E 's/((serial|license|password|token)[^[:space:]]*)/[REDACTED]/ig' & LOGPID=$! +docker logs -f unity-mcp 2>&1 | sed -E 's/([[:alnum:]_]*(serial|license|password|token|key|secret|api[-_]?key|email|username|user|uid)[[:alnum:]_]*[[:space:]]*[:=][[:space:]]*)[^[:space:]]+/\1[REDACTED]/ig' & LOGPID=$! @@ -docker logs --tail 200 unity-mcp | sed -E 's/((serial|license|password|token)[^[:space:]]*)/[REDACTED]/ig' || true +docker logs --tail 200 unity-mcp | sed -E 's/([[:alnum:]_]*(serial|license|password|token|key|secret|api[-_]?key|email|username|user|uid)[[:alnum:]_]*[[:space:]]*[:=][[:space:]]*)[^[:space:]]+/\1[REDACTED]/ig' || true @@ -docker logs --tail 400 unity-mcp | sed -E 's/((serial|license|password|token)[^[:space:]]*)/[REDACTED]/ig' || true +docker logs --tail 400 unity-mcp | sed -E 's/([[:alnum:]_]*(serial|license|password|token|key|secret|api[-_]?key|email|username|user|uid)[[:alnum:]_]*[[:space:]]*[:=][[:space:]]*)[^[:space:]]+/\1[REDACTED]/ig' || trueAlso applies to: 538-542
296-343: Fragile XML fallback via regex; prefer XML parsing only.Regex fallback can misparse edge cases. Consider ignoring malformed fragments or using a tolerant XML parser.
1-543: YAML hygiene (non-functional).Trailing spaces, inconsistent indentation, and missing EOF newline flagged by linters. Not breaking, but easy to tidy.
📜 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 (8)
.claude/prompts/nl-unity-suite-full.md(1 hunks).github/workflows/claude-nl-suite.yml(1 hunks)UnityMcpBridge/Editor/Tools/ManageScript.cs(14 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py(1 hunks)tests/test_edit_normalization_and_noop.py(1 hunks)tests/test_edit_strict_and_warnings.py(1 hunks)tests/test_get_sha.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T21:31:02.427Z
Learnt from: dsarno
PR: dsarno/unity-mcp#56
File: .claude/prompts/nl-unity-suite-full.md:5-6
Timestamp: 2025-08-29T21:31:02.427Z
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:
.claude/prompts/nl-unity-suite-full.md.github/workflows/claude-nl-suite.yml
🧬 Code graph analysis (6)
tests/test_edit_normalization_and_noop.py (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
manage_script(355-480)register_manage_script_tools(9-521)
tests/test_get_sha.py (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (3)
manage_script(355-480)register_manage_script_tools(9-521)get_sha(513-521)
tests/test_edit_strict_and_warnings.py (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
manage_script(355-480)register_manage_script_tools(9-521)
UnityMcpBridge/Editor/Tools/ManageScript.cs (1)
UnityMcpBridge/Editor/Helpers/Response.cs (3)
Response(10-61)Error(41-60)Success(18-33)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(378-397)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
line_col_from_index(459-463)line_col_from_index(572-577)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(378-397)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
line_col_from_index(112-120)
🪛 Ruff (0.12.2)
tests/test_edit_normalization_and_noop.py
15-15: Multiple statements on one line (colon)
(E701)
35-35: Unused method argument: args
(ARG002)
35-35: Unused method argument: kwargs
(ARG002)
36-36: Multiple statements on one line (semicolon)
(E702)
50-50: Unused function argument: cmd
(ARG001)
64-64: Use of assert detected
(S101)
70-70: Unused function argument: cmd
(ARG001)
83-83: Unused function argument: cmd
(ARG001)
83-83: Unused function argument: params
(ARG001)
88-88: Use of assert detected
(S101)
89-89: Use of assert detected
(S101)
tests/test_get_sha.py
42-42: Unused method argument: args
(ARG002)
42-42: Unused method argument: kwargs
(ARG002)
69-69: Use of assert detected
(S101)
70-70: Use of assert detected
(S101)
71-71: Use of assert detected
(S101)
72-72: Use of assert detected
(S101)
73-73: Use of assert detected
(S101)
tests/test_edit_strict_and_warnings.py
15-15: Multiple statements on one line (colon)
(E701)
37-37: Unused method argument: args
(ARG002)
37-37: Unused method argument: kwargs
(ARG002)
38-38: Multiple statements on one line (semicolon)
(E702)
52-52: Unused function argument: cmd
(ARG001)
52-52: Unused function argument: params
(ARG001)
62-62: Use of assert detected
(S101)
64-64: Use of assert detected
(S101)
65-65: Use of assert detected
(S101)
67-67: Use of assert detected
(S101)
74-74: Unused function argument: cmd
(ARG001)
74-74: Unused function argument: params
(ARG001)
81-81: Use of assert detected
(S101)
82-82: Use of assert detected
(S101)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
73-73: Unused function argument: ctx
(ARG001)
108-108: Do not catch blind exception: Exception
(BLE001)
135-136: try-except-pass detected, consider logging the exception
(S110)
135-135: Do not catch blind exception: Exception
(BLE001)
145-146: try-except-pass detected, consider logging the exception
(S110)
145-145: Do not catch blind exception: Exception
(BLE001)
178-179: try-except-pass detected, consider logging the exception
(S110)
178-178: Do not catch blind exception: Exception
(BLE001)
199-200: try-except-pass detected, consider logging the exception
(S110)
199-199: Do not catch blind exception: Exception
(BLE001)
208-209: try-except-pass detected, consider logging the exception
(S110)
208-208: Do not catch blind exception: Exception
(BLE001)
232-234: try-except-pass detected, consider logging the exception
(S110)
232-232: Do not catch blind exception: Exception
(BLE001)
278-278: Unused function argument: ctx
(ARG001)
316-316: Unused function argument: ctx
(ARG001)
332-332: Unused function argument: ctx
(ARG001)
356-356: Unused function argument: ctx
(ARG001)
428-429: try-except-pass detected, consider logging the exception
(S110)
428-428: Do not catch blind exception: Exception
(BLE001)
435-435: Do not catch blind exception: Exception
(BLE001)
476-476: Do not catch blind exception: Exception
(BLE001)
479-479: Use explicit conversion flag
Replace with conversion flag
(RUF010)
486-486: Unused function argument: ctx
(ARG001)
498-504: Consider moving this statement to an else block
(TRY300)
505-505: Do not catch blind exception: Exception
(BLE001)
513-513: Unused function argument: ctx
(ARG001)
520-520: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py
23-25: Avoid specifying long messages outside the exception class
(TRY003)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
59-59: Avoid specifying long messages outside the exception class
(TRY003)
61-61: Function definition does not bind loop variable lines
(B023)
62-62: Ambiguous variable name: l
(E741)
62-62: Function definition does not bind loop variable lines
(B023)
63-63: Ambiguous variable name: l
(E741)
63-63: Function definition does not bind loop variable lines
(B023)
79-79: Avoid specifying long messages outside the exception class
(TRY003)
209-209: Unused function argument: ctx
(ARG001)
284-284: Comment contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF003)
284-284: Comment contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF003)
505-505: Function definition does not bind loop variable m
(B023)
542-542: Do not catch blind exception: Exception
(BLE001)
629-629: Function definition does not bind loop variable m
(B023)
670-670: Do not catch blind exception: Exception
(BLE001)
681-681: Consider [*diff[:800], "... (diff truncated) ..."] instead of concatenation
Replace with [*diff[:800], "... (diff truncated) ..."]
(RUF005)
685-685: Do not catch blind exception: Exception
(BLE001)
690-690: Do not catch blind exception: Exception
(BLE001)
709-709: Consider [*diff[:2000], "... (diff truncated) ..."] instead of concatenation
Replace with [*diff[:2000], "... (diff truncated) ..."]
(RUF005)
🪛 LanguageTool
.claude/prompts/nl-unity-suite-full.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...this once, verbatim, early in the run:** AllowedTools: Write,Bash(printf:*),Bash(...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...ole,mcp__unity__get_sha --- ## Mission 1) Pick target file (prefer): - `unity:/...
(QB_NEW_EN)
[grammar] ~20-~20: There might be a mistake here.
Context: ...write. --- ## 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] ~24-~24: There might be a mistake here.
Context: ...the URI) - Relative (when supported): Assets/... - File paths for the helper script are wor...
(QB_NEW_EN)
[grammar] ~30-~30: There might be a mistake here.
Context: ...it-nl-suite.md(synthesized from JUnit) - Helper script:scripts/nlt-revert.sh` (...
(QB_NEW_EN)
[grammar] ~71-~71: There might be a mistake here.
Context: ...(T‑A/T‑E) - Do NOT use anchor_replace. - Use script_apply_edits with: ```json {...
(QB_NEW_EN)
[grammar] ~80-~80: There might be a mistake here.
Context: ...the method. # T‑B (replace method body) - Use mcp__unity__apply_text_edits with ...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ... strictly inside the HasTarget braces. - Compute start/end from a fresh `read_res...
(QB_NEW_EN)
[grammar] ~82-~82: There might be a mistake here.
Context: ...h read_resource at test start. Do not edit signature or header. - On `{status:"sta...
(QB_NEW_EN)
[grammar] ~82-~82: There might be a mistake here.
Context: ... start. Do not edit signature or header. - On {status:"stale_file"} retry once wi...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ...hash; if absent, re-read once and retry. - On bad_request: write the testcase wit...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ...ure>…, restore, and continue to next test. - On missing_field`: FALL B...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ...e>, restore, and continue to next test. - On missing_field`: FALL BACK per above;...
(QB_NEW_EN)
[style] ~85-~85: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..., restore, and continue to next test. - On missing_field: FALL BACK per above; i...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~88-~88: There might be a mistake here.
Context: ...y. Span formats for apply_text_edits: - Prefer LSP ranges (0‑based): `{ "range":...
(QB_NEW_EN)
[grammar] ~89-~89: There might be a mistake here.
Context: ..._edits: - Prefer LSP ranges (0‑based): { "range": { "start": {"line": L, "character": C}, "end": {…} }, "newText": "…" }- Explicit fields are 1‑based:{ "startLi...
(QB_NEW_EN)
[grammar] ~90-~90: There might be a mistake here.
Context: ..."newText": "…" }- Explicit fields are 1‑based:{ "startLine": L1, "startCol": C1, "endLine": L2, "endCol": C2, "newText": "…" }` - SDK preflights overlap after normalizati...
(QB_NEW_EN)
[grammar] ~91-~91: There might be a mistake here.
Context: ..."}with conflicts and no file mutation. - Optional debug: passstrict:true` to re...
(QB_NEW_EN)
[grammar] ~98-~98: There might be a mistake here.
Context: ...tem-out>. - If content contains ]]>`, split CDATA: ...
(QB_NEW_EN)
[grammar] ~149-~149: There might be a mistake here.
Context: ...fore any further edits in the same test. - Prefer anchors (script_apply_edits) fo...
(QB_NEW_EN)
[grammar] ~152-~152: There might be a mistake here.
Context: ...rrors (timeout, EOF, connection closed):** - Write reports/<TESTID>_results.xml wit...
(QB_NEW_EN)
[grammar] ~153-~153: There might be a mistake here.
Context: ...<error> node capturing the error text. - Run the OS restore via `scripts/nlt-reve...
(QB_NEW_EN)
[grammar] ~158-~158: There might be a mistake here.
Context: ... evidence, and end with VERDICT: FAIL. - Run scripts/nlt-revert.sh restore ... ...
(QB_NEW_EN)
[grammar] ~162-~162: There might be a mistake here.
Context: ...T-E, T-F, T-G, T-H, T-I, T-J (15 total). - Before NL-1..T-J: Bash(scripts/nlt-rever...
(QB_NEW_EN)
[grammar] ~163-~163: There might be a mistake here.
Context: ...) IF the baseline exists; skip for NL-0. - NL-0 must include the PLAN line (len=15)...
(QB_NEW_EN)
[grammar] ~171-~171: There might be a mistake here.
Context: ...verify; delete PrintSeries(); restore. - NL‑2. Anchor comment — Insert `// Build ...
(QB_NEW_EN)
[grammar] ~172-~172: There might be a mistake here.
Context: ...bove public void Update(...); restore. - NL‑3. End‑of‑class — Insert `// Tail tes...
(QB_NEW_EN)
[grammar] ~173-~173: There might be a mistake here.
Context: ...— Insert // Tail test A/B/C (3 lines) before final brace; restore. - NL‑4. Compile t...
(QB_NEW_EN)
[grammar] ~173-~173: There might be a mistake here.
Context: ...` (3 lines) before final brace; restore. - NL‑4. Compile trigger — Record INFO only...
(QB_NEW_EN)
[grammar] ~176-~176: There might be a mistake here.
Context: ...fy; delete via regex_replace; restore. ### T‑B. Replace body — Single `replace_rang...
(QB_NEW_EN)
[grammar] ~177-~177: There might be a mistake here.
Context: ...lace_rangeinsideHasTarget`; restore. ### T‑C. Header/region preservation — Edit i...
(QB_NEW_EN)
[grammar] ~178-~178: There might be a mistake here.
Context: ...reserve signature/docs/regions; restore. ### T‑D. End‑of‑class (anchor) — Insert help...
(QB_NEW_EN)
[grammar] ~179-~179: There might be a mistake here.
Context: ...per before final brace; remove; restore. ### T‑E. Lifecycle — Insert → update → delet...
(QB_NEW_EN)
[grammar] ~180-~180: There might be a mistake here.
Context: ...rt → update → delete via regex; restore. ### T‑F. Atomic batch — One `mcp__unity__app...
(QB_NEW_EN)
[grammar] ~181-~181: There might be a mistake here.
Context: ...pply_text_edits` call (text ranges only) - Compute all three edits from the **same ...
(QB_NEW_EN)
[grammar] ~182-~182: There might be a mistake here.
Context: ...hree edits from the same fresh read: 1) Two small interior replace_range tweak...
(QB_NEW_EN)
[grammar] ~183-~183: There might be a mistake here.
Context: ...o small interior replace_range tweaks. 2) One end‑of‑class insertion: find the...
(QB_NEW_EN)
[grammar] ~184-~184: There might be a mistake here.
Context: ...eplacement` to the 3‑line comment block. - Send all three ranges in one call, s...
(QB_NEW_EN)
[grammar] ~186-~186: There might be a mistake here.
Context: ...…`, restore, and continue. - T‑G. Path normalization — Make the same ...
(QB_NEW_EN)
[grammar] ~191-~191: There might be a mistake here.
Context: ...nterior tweak (or none), then MUST call: mcp__unity__validate_script(level:"stand...
(QB_NEW_EN)
[grammar] ~192-~192: There might be a mistake here.
Context: ...unity__validate_script(level:"standard") - Write the validator output to system-out...
(QB_NEW_EN)
[grammar] ~199-~199: There might be a mistake here.
Context: ... both spans come from the same snapshot. * Single mcp__unity__apply_text_edits call...
(QB_NEW_EN)
[grammar] ~204-~204: There might be a mistake here.
Context: ...a tiny legit edit with pre_sha; success. * Attempt another edit reusing the OLD pre...
(QB_NEW_EN)
[grammar] ~207-~207: There might be a mistake here.
Context: ..., restore. - (3) USING_GUARD (optional): * Attempt a 1-line insert above the first ...
(QB_NEW_EN)
[grammar] ~213-~213: There might be a mistake here.
Context: ...aseline. - Repeat a replace_range twice (second call may be noop). Validate stand...
(QB_NEW_EN)
[grammar] ~213-~213: There might be a mistake here.
Context: ... a replace_range twice (second call may be noop). Validate standard after each. - ...
(QB_NEW_EN)
[grammar] ~213-~213: There might be a mistake here.
Context: ..._range twice (second call may be noop). Validate standard after each. - Insert or ensure...
(QB_NEW_EN)
[grammar] ~213-~213: There might be a mistake here.
Context: ... be noop). Validate standard after each. - Insert or ensure a tiny comment, then de...
(QB_NEW_EN)
[grammar] ~214-~214: There might be a mistake here.
Context: ...re a tiny comment, then delete it twice (second delete may be noop). - Restore an...
(QB_NEW_EN)
[grammar] ~214-~214: There might be a mistake here.
Context: ...then delete it twice (second delete may be noop). - Restore and PASS unless an err...
(QB_NEW_EN)
[grammar] ~214-~214: There might be a mistake here.
Context: ...te it twice (second delete may be noop). - Restore and PASS unless an error/structu...
(QB_NEW_EN)
[grammar] ~220-~220: There might be a mistake here.
Context: ...uses are non‑fatal; record and continue. - End each testcase <system-out> with `V...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
.claude/prompts/nl-unity-suite-full.md
99-99: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
109-109: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
176-176: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
177-177: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
178-178: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
179-179: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
180-180: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
182-182: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
185-185: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
186-186: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
187-187: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 0
(MD005, list-indent)
198-198: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
199-199: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
200-200: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
201-201: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
203-203: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
204-204: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
205-205: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
206-206: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
208-208: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
209-209: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
🪛 YAMLlint (1.37.1)
.github/workflows/claude-nl-suite.yml
[error] 5-5: trailing spaces
(trailing-spaces)
[error] 9-9: trailing spaces
(trailing-spaces)
[error] 13-13: trailing spaces
(trailing-spaces)
[error] 18-18: trailing spaces
(trailing-spaces)
[warning] 21-21: wrong indentation: expected 8 but found 6
(indentation)
[warning] 25-25: wrong indentation: expected 10 but found 8
(indentation)
[warning] 26-26: too many spaces after colon
(colons)
[error] 27-27: trailing spaces
(trailing-spaces)
[warning] 30-30: wrong indentation: expected 10 but found 8
(indentation)
[warning] 33-33: wrong indentation: expected 14 but found 12
(indentation)
[warning] 33-33: too many spaces after colon
(colons)
[warning] 34-34: too many spaces after colon
(colons)
[warning] 36-36: too many spaces after colon
(colons)
[error] 46-46: trailing spaces
(trailing-spaces)
[warning] 49-49: wrong indentation: expected 14 but found 12
(indentation)
[error] 50-50: trailing spaces
(trailing-spaces)
[warning] 54-54: wrong indentation: expected 14 but found 12
(indentation)
[error] 55-55: trailing spaces
(trailing-spaces)
[error] 73-73: trailing spaces
(trailing-spaces)
[warning] 79-79: wrong indentation: expected 14 but found 12
(indentation)
[warning] 79-79: too many spaces after colon
(colons)
[warning] 80-80: too many spaces after colon
(colons)
[warning] 82-82: too many spaces after colon
(colons)
[warning] 84-84: wrong indentation: expected 14 but found 12
(indentation)
[error] 88-88: trailing spaces
(trailing-spaces)
[error] 95-95: trailing spaces
(trailing-spaces)
[error] 102-102: trailing spaces
(trailing-spaces)
[warning] 107-107: wrong indentation: expected 14 but found 12
(indentation)
[warning] 107-107: too many spaces after colon
(colons)
[warning] 109-109: too many spaces after colon
(colons)
[error] 139-139: trailing spaces
(trailing-spaces)
[error] 179-179: trailing spaces
(trailing-spaces)
[error] 201-201: trailing spaces
(trailing-spaces)
[error] 208-208: trailing spaces
(trailing-spaces)
[error] 221-221: trailing spaces
(trailing-spaces)
[error] 267-267: trailing spaces
(trailing-spaces)
[error] 268-268: trailing spaces
(trailing-spaces)
[warning] 275-275: wrong indentation: expected 14 but found 12
(indentation)
[error] 294-294: trailing spaces
(trailing-spaces)
[error] 343-343: trailing spaces
(trailing-spaces)
[error] 443-443: trailing spaces
(trailing-spaces)
[error] 455-455: trailing spaces
(trailing-spaces)
[error] 467-467: trailing spaces
(trailing-spaces)
[error] 480-480: trailing spaces
(trailing-spaces)
[error] 497-497: trailing spaces
(trailing-spaces)
[error] 513-513: trailing spaces
(trailing-spaces)
[warning] 518-518: wrong indentation: expected 14 but found 12
(indentation)
[error] 524-524: trailing spaces
(trailing-spaces)
[warning] 529-529: wrong indentation: expected 14 but found 12
(indentation)
[error] 536-536: trailing spaces
(trailing-spaces)
[error] 543-543: no new line character at the end of file
(new-line-at-end-of-file)
[error] 543-543: trailing spaces
(trailing-spaces)
⏰ 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: nl-suite
- GitHub Check: Cursor Bugbot
🔇 Additional comments (11)
UnityMcpBridge/Editor/Tools/ManageScript.cs (4)
339-364: Atomic create without BOM + delayed refresh — LGTM.No BOM, atomic write, and deferring heavy work off the reply path look solid.
437-474: Atomic update without BOM + delayed refresh — LGTM.Safe replace with cleanup and post‑reply refresh is correct.
229-239: Diagnostics parsing with timeout — LGTM.Regex hardened with timeout; structured mapping to severity/line is good.
250-277: get_sha response shape — LGTM.Includes sha256, byte length, and ISO timestamp; sensible fallbacks.
tests/test_edit_normalization_and_noop.py (1)
79-90: No‑op propagation — LGTM.Test correctly checks Unity’s no_op signal and evidence passthrough.
tests/test_edit_strict_and_warnings.py (2)
48-68: Zero‑based normalization warning — LGTM.Covers warning emission and normalizedEdits echo.
70-83: Strict mode error on zero‑based fields — LGTM.Validates code path and error code.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (4)
112-121: Index→line/col mapping looks correct (1-based).Matches the server’s mapping strategy; no change needed.
214-249: Nice: preflight overlap detection on non-zero-length ranges.Good guard to fail fast on conflicting edits.
285-296: create_script validation is solid.Assets/ guard, traversal/absolute checks, name and .cs enforcement all look good.
383-434: No action required: EOF semantics are correct — the server’s replace_range implementation defines max_line = len(lines)+1 and maps any endLine > len(lines) to the file-end index, so using endLine = N+1, endCol = 1 properly targets EOF.
| except socket.timeout as e: | ||
| logger.warning("Socket timeout during framed receive") | ||
| raise TimeoutError("Timeout receiving Unity response") from e | ||
| except Exception as e: |
There was a problem hiding this comment.
Bug: Zero-Length Frame Handling Mismatch
The C# server and Python client handle zero-length frames inconsistently. The C# side returns an empty string, treating them as valid responses, but the Python client interprets them as heartbeats and keeps waiting for more data. This mismatch can cause the Python client to hang indefinitely when the C# server sends a zero-length response.
Additional Locations (1)
There was a problem hiding this comment.
Looking at the code and the bug description, the issue is clear: there's a mismatch between how the C# server and Python client handle zero-length frames.
The Python client correctly treats zero-length frames as heartbeats and continues waiting for actual data (lines 133-141), but it has a fallback that returns an empty response after reaching thresholds. However, the C# server apparently sends zero-length frames as actual responses rather than heartbeats.
Here's my suggestion to resolve this inconsistency:
| except Exception as e: | |
| except socket.timeout as e: | |
| logger.warning("Socket timeout during framed receive") | |
| raise TimeoutError("Timeout receiving Unity response") from e | |
| except Exception as e: | |
| logger.error(f"Error during framed receive: {str(e)}") | |
| raise |
Wait, let me provide a more comprehensive fix. The issue is in the heartbeat handling logic. We need to distinguish between legitimate zero-length responses from the C# server and actual heartbeats. Here's the proper fix:
| except Exception as e: | |
| except Exception as e: | |
| logger.error(f"Error during framed receive: {str(e)}") | |
| # If we received zero-length frames that might be legitimate responses from C# server, | |
| # return empty response immediately instead of treating as heartbeats | |
| if "heartbeat threshold reached" in str(e).lower(): | |
| logger.debug("Treating accumulated heartbeats as empty response from C# server") | |
| return b"" | |
| raise |
Actually, the real fix should be in the heartbeat handling logic above. The issue is that the Python client should not wait indefinitely for heartbeats when the C# server sends a zero-length frame as a legitimate response. The current code is mostly correct, but we should reduce the heartbeat threshold and timeout to be more responsive:
Recommended approach: Modify the C# server to not send zero-length frames as responses, OR modify the Python client to be more aggressive about treating zero-length frames as legitimate empty responses. Since you're asking about the Python side, I'd suggest reducing the heartbeat tolerance and making it return empty responses faster when receiving zero-length frames from the C# server.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
314-336: Backoff variable computed but unused; apply jitter to backoff.Current sleep uses only jitter; incorporate exponential backoff.
- # Base exponential backoff - backoff = base_backoff * (2 ** attempt) - # Decorrelated jitter multiplier - jitter = random.uniform(0.1, 0.3) + # Base exponential backoff + backoff = base_backoff * (2 ** attempt) + # Decorrelated jitter multiplier + jitter = random.uniform(0.5, 1.5) @@ - sleep_s = min(cap, jitter * (2 ** attempt)) + sleep_s = min(cap, backoff * jitter)
♻️ Duplicate comments (2)
tests/test_transport_framing.py (2)
160-199: Zero‑length heartbeat behavior validated. Resolved prior mismatch.Client now skips length=0 frames and reads the next payload; aligns with server semantics.
86-109: Pre‑handshake disconnect race addressed.Previous review noted timeout/poll mismatch; your tighter 0.15/0.01 loop mitigates false disconnects.
🧹 Nitpick comments (14)
tests/test_transport_framing.py (4)
86-109: Use monotonic clock for deadlines in pre-handshake poll.Avoid time jumps affecting disconnect race checks.
- deadline = time.time() + 0.15 # short, reduces race with legitimate clients - while time.time() < deadline: + deadline = time.monotonic() + 0.15 # short, reduces race with legitimate clients + while time.monotonic() < deadline:
32-34: Enable SO_REUSEADDR for flaky/rapid CI retries.Prevents “address already in use” between fast binds.
- sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)Also applies to: 80-82
63-64: Avoid blind except; keep minimal diagnostics.Tests can swallow expected errors, but make the exceptions explicit or log at debug.
- except Exception: - pass + except (OSError, socket.timeout): + # Expected in shutdown/teardown; silence in tests + passOptionally add at top of file (outside hunk):
import logging logger = logging.getLogger(__name__)Also applies to: 68-69, 100-100
184-186: Expand one‑liners; fix E701.Improves readability and appeases linters.
- try: conn.close() - except Exception: pass + try: + conn.close() + except OSError: + passUnityMcpBridge/UnityMcpServer~/src/unity_connection.py (10)
94-102: Prefer logger.exception and narrow close errors.Avoid blind except; capture traceback for failures.
- except Exception as e: - logger.error(f"Failed to connect to Unity: {str(e)}") - try: - if self.sock: - self.sock.close() - except Exception: - pass + except Exception: + logger.exception("Failed to connect to Unity") + try: + if self.sock: + self.sock.close() + except OSError: + logger.debug("Error closing socket after connect failure", exc_info=True) self.sock = None return False
141-145: Use logger.exception on framed receive failures.Keeps stack traces.
- except Exception as e: - logger.error(f"Error during framed receive: {str(e)}") - raise + except Exception: + logger.exception("Error during framed receive") + raise
153-154: Raise ConnectionError on EOF before data.Use semantic exception.
- raise Exception("Connection closed before receiving data") + raise ConnectionError("Connection closed before receiving data")
193-195: Raise TimeoutError instead of generic Exception.Aligns with framed path.
- except socket.timeout: - logger.warning("Socket timeout during receive") - raise Exception("Timeout receiving Unity response") + except socket.timeout: + logger.warning("Socket timeout during receive") + raise TimeoutError("Timeout receiving Unity response")
254-259: Avoid logging payload preview; risk of secret leakage.Prefer size + mode only, or behind a config flag.
- logger.debug( - "send %d bytes; mode=%s; head=%s", - len(payload), - mode, - (payload[:32]).decode('utf-8', 'ignore'), - ) + logger.debug("send %d bytes; mode=%s", len(payload), mode)
261-264: Coalesce framed send and validate outgoing size.Fewer syscalls and symmetric max enforcement.
- if self.use_framing: - header = struct.pack('>Q', len(payload)) - self.sock.sendall(header) - self.sock.sendall(payload) + if self.use_framing: + if len(payload) > FRAMED_MAX: + raise ValueError(f"Outgoing payload too large: {len(payload)} > {FRAMED_MAX}") + frame = struct.pack('>Q', len(payload)) + payload + self.sock.sendall(frame)
221-223: Remove stale last_short_timeout; simplify short-timeout handling.The state variable is never set; local restore is sufficient.
- last_short_timeout = None + # (removed) last_short_timeout @@ - restore_timeout = None - if attempt > 0 and last_short_timeout is None: - restore_timeout = self.sock.gettimeout() - self.sock.settimeout(1.0) + restore_timeout = None + if attempt > 0: + restore_timeout = self.sock.gettimeout() + self.sock.settimeout(1.0) @@ - if restore_timeout is not None: - self.sock.settimeout(restore_timeout) - last_short_timeout = None + if restore_timeout is not None: + self.sock.settimeout(restore_timeout)Also applies to: 269-280
239-241: Raise ConnectionError on connect failure in send path.Use specific exception instead of generic.
- if not self.sock and not self.connect(): - raise Exception("Could not connect to Unity") + if not self.sock and not self.connect(): + raise ConnectionError("Could not connect to Unity")
183-184: Demote verbose receive log to debug.Prevents log spam on large responses.
- logger.info(f"Received complete response ({len(data)} bytes)") + logger.debug("Received complete response (%d bytes)", len(data))
169-178: Replace manual in-place JSON “content” unescaping with proper parsing. Manual string surgery risks corrupting valid JSON; instead,json.loads(decoded_data)into an object, ifcontentis a JSON string thenjson.loadsthat and assign back, thenjson.dumps()the result. Legacy (unframed) mode remains enabled inunity_connection.py(self.use_framing = False) and is covered bytests/test_transport_framing.py.
📜 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/UnityMcpServer~/src/unity_connection.py(6 hunks)tests/test_transport_framing.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_transport_framing.py (1)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (4)
_read_exact(114-121)connect(44-102)receive_full_response(123-197)disconnect(104-112)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
UnityMcpBridge/UnityMcpServer~/src/port_discovery.py (2)
PortDiscovery(24-155)discover_unity_port(93-134)tests/test_transport_framing.py (1)
_read_exact(47-54)
🪛 Ruff (0.12.2)
tests/test_transport_framing.py
63-64: try-except-pass detected, consider logging the exception
(S110)
63-63: Do not catch blind exception: Exception
(BLE001)
68-69: try-except-pass detected, consider logging the exception
(S110)
68-68: Do not catch blind exception: Exception
(BLE001)
100-100: Do not catch blind exception: Exception
(BLE001)
124-124: Use of assert detected
(S101)
125-125: Use of assert detected
(S101)
132-132: Use of assert detected
(S101)
133-133: Use of assert detected
(S101)
137-137: Use of assert detected
(S101)
151-151: Use of assert detected
(S101)
184-184: Multiple statements on one line (colon)
(E701)
185-185: try-except-pass detected, consider logging the exception
(S110)
185-185: Do not catch blind exception: Exception
(BLE001)
185-185: Multiple statements on one line (colon)
(E701)
193-193: Use of assert detected
(S101)
196-196: Use of assert detected
(S101)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py
87-87: Avoid specifying long messages outside the exception class
(TRY003)
93-93: Consider moving this statement to an else block
(TRY300)
94-94: Do not catch blind exception: Exception
(BLE001)
95-95: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
95-95: Use explicit conversion flag
Replace with conversion flag
(RUF010)
99-100: try-except-pass detected, consider logging the exception
(S110)
99-99: Do not catch blind exception: Exception
(BLE001)
119-119: Avoid specifying long messages outside the exception class
(TRY003)
135-135: Abstract raise to an inner function
(TRY301)
135-135: Avoid specifying long messages outside the exception class
(TRY003)
141-141: Avoid specifying long messages outside the exception class
(TRY003)
143-143: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
143-143: Use explicit conversion flag
Replace with conversion flag
(RUF010)
241-241: Abstract raise to an inner function
(TRY301)
241-241: Create your own exception
(TRY002)
241-241: Avoid specifying long messages outside the exception class
(TRY003)
361-361: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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)
tests/test_transport_framing.py (1)
121-126: Handshake negative path looks correct.connect() must fail and leave no socket on non‑framed greeting.
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
353-363: Thread-safe global connection init looks good.Double-checked locking around the singleton prevents duplicate sockets.
…t; add robust tests; prompt uses delete_method for temp helper
…-length response with C#; update test
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
.claude/prompts/nl-unity-suite-full.md (2)
121-135: Unexecutable fallback: agents can’t “restore using baseline bytes”Agents don’t have a tool to read baseline bytes; instruct a deterministic error path instead.
- - If the helper fails: fall back once to a guarded full‑file restore using the baseline bytes; then continue. + - If the helper fails: write the testcase with an `<error>` node (include the helper stderr/output), then continue to the next test. Do not attempt a byte‑level restore.
96-103: Fix contradiction: allow Write tool as a sanctioned FS mutator; keep Bash stdout-onlyCurrently says only the revert helper may modify the FS, but earlier requires writing per-test XML via Write. Make the rule explicit and consistent.
- - Bash(printf|echo) must write to stdout only. Do not use shell redirection, here‑docs, or `tee` to create/modify files. The only allowed FS mutation is via `scripts/nlt-revert.sh`. + - Bash(printf|echo) must write to stdout only. Do not use shell redirection, here‑docs, or `tee`. + - Only these may modify the filesystem: + 1) Write → `reports/*_results.xml` (per path rules above) + 2) `scripts/nlt-revert.sh` → snapshot/restoreUnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (6)
617-619: Unify structured_kinds with STRUCT; do not include anchor_insert.Keeps routing rules consistent across branches.
- structured_kinds = {"replace_class","delete_class","replace_method","delete_method","insert_method","anchor_insert"} + structured_kinds = STRUCT
633-641: anchor_insert (text path): add replacement alias and ignore_case.Parity with other branches improves robustness.
- # aliasing for text field - text_field = e.get("text") or e.get("insert") or e.get("content") or "" + # aliasing for text field + text_field = e.get("text") or e.get("insert") or e.get("content") or e.get("replacement") or "" @@ - if op == "anchor_insert": + if op == "anchor_insert": anchor = e.get("anchor") or "" position = (e.get("position") or "after").lower() - m = _re.search(anchor, current_text, _re.MULTILINE) + flags = _re.MULTILINE | (_re.IGNORECASE if e.get("ignore_case") else 0) + m = _re.search(anchor, current_text, flags)
456-462: anchor_insert misclassified as STRUCT; route via text path instead.Keeping anchor_insert in STRUCT bypasses apply_text_edits guards and causes inconsistent handling. Remove it from STRUCT and add to TEXT.
- STRUCT = {"replace_class","delete_class","replace_method","delete_method","insert_method","anchor_delete","anchor_replace","anchor_insert"} - TEXT = {"prepend","append","replace_range","regex_replace"} + STRUCT = {"replace_class","delete_class","replace_method","delete_method","insert_method","anchor_delete","anchor_replace"} + TEXT = {"prepend","append","replace_range","regex_replace","anchor_insert"}
549-561: regex_replace (mixed path): honor ignore_case and count; support multiple matches.Only the first match is handled, ignoring flags/count; can under-apply and desync subsequent anchors.
- elif opx == "regex_replace": - pattern = e.get("pattern") or "" - m = _re.search(pattern, current_text, _re.MULTILINE) - if not m: - continue - # Expand $1, $2... in replacement using this match - def _expand_dollars(rep: str) -> str: - return _re.sub(r"\$(\d+)", lambda g: m.group(int(g.group(1))) or "", rep) - repl = _expand_dollars(text_field) - sl, sc = line_col_from_index(m.start()) - el, ec = line_col_from_index(m.end()) - at_edits.append({"startLine": sl, "startCol": sc, "endLine": el, "endCol": ec, "newText": repl}) - current_text = current_text[:m.start()] + repl + current_text[m.end():] + elif opx == "regex_replace": + pattern = e.get("pattern") or "" + count = int(e.get("count", 0) or 0) # 0 = all + flags = _re.MULTILINE | (_re.IGNORECASE if e.get("ignore_case") else 0) + matches = list(_re.finditer(pattern, current_text, flags)) + if not matches: + continue + if count > 0: + matches = matches[:count] + offset = 0 + for m in matches: + def _expand_dollars(rep: str) -> str: + return _re.sub(r"\$(\d+)", lambda g: m.group(int(g.group(1))) or "", rep) + repl = _expand_dollars(text_field) + start = m.start() + offset + end = m.end() + offset + sl, sc = line_col_from_index(start) + el, ec = line_col_from_index(end) + at_edits.append({"startLine": sl, "startCol": sc, "endLine": el, "endCol": ec, "newText": repl}) + current_text = current_text[:start] + repl + current_text[end:] + offset += len(repl) - (m.end() - m.start())
588-589: Apply text edits sequentially on server to match locally computed spans.Without options.applyMode="sequential", positions are interpreted atomically against the precondition snapshot, risking misapplies.
- "options": {"refresh": "immediate", "validate": (options or {}).get("validate", "standard")} + "options": { + "refresh": "immediate", + "validate": (options or {}).get("validate", "standard"), + "applyMode": "sequential" + }
671-701: regex_replace (text path): first-match only; add multi-match with $n expansion and structural guard.Convert all non-overlapping matches honoring count/ignore_case, expanding $n per match, and reject edits that unbalance structure.
- elif op == "regex_replace": - pattern = e.get("pattern") or "" - repl = text_field - flags = _re.MULTILINE | (_re.IGNORECASE if e.get("ignore_case") else 0) - m = _re.search(pattern, current_text, flags) - if not m: - continue - # Expand $1, $2... backrefs in replacement using the first match (consistent with mixed-path behavior) - def _expand_dollars(rep: str) -> str: - return _re.sub(r"\$(\d+)", lambda g: m.group(int(g.group(1))) or "", rep) - repl_expanded = _expand_dollars(repl) - # Preview structural balance after replacement; refuse destructive deletes - preview = current_text[:m.start()] + repl_expanded + current_text[m.end():] - if not _is_structurally_balanced(preview): - return _with_norm({ - "success": False, - "code": "validation_failed", - "message": "regex_replace would unbalance braces/parentheses; prefer delete_method", - "data": {"status": "validation_failed", "normalizedEdits": normalized_for_echo, "hint": "Use script_apply_edits delete_method for method removal"} - }, normalized_for_echo, routing="text") - sl, sc = line_col_from_index(m.start()) - el, ec = line_col_from_index(m.end()) - at_edits.append({ - "startLine": sl, - "startCol": sc, - "endLine": el, - "endCol": ec, - "newText": repl_expanded - }) - current_text = preview + elif op == "regex_replace": + pattern = e.get("pattern") or "" + repl = text_field + count = int(e.get("count", 0) or 0) # 0 = all + flags = _re.MULTILINE | (_re.IGNORECASE if e.get("ignore_case") else 0) + applied, pos = 0, 0 + while True: + m = _re.search(pattern, current_text[pos:], flags) + if not m or (count > 0 and applied >= count): + break + start = pos + m.start() + end = pos + m.end() + repl_expanded = _re.sub(r"\$(\d+)", lambda g: m.group(int(g.group(1))) or "", repl) + # Structural guard after each tentative replacement + preview_text = current_text[:start] + repl_expanded + current_text[end:] + if not _is_structurally_balanced(preview_text): + return _with_norm({ + "success": False, + "code": "validation_failed", + "message": "regex_replace would unbalance braces/parentheses; prefer delete_method", + "data": {"status": "validation_failed", "normalizedEdits": normalized_for_echo, "hint": "Use script_apply_edits delete_method for method removal"} + }, normalized_for_echo, routing="text") + sl, sc = line_col_from_index(start) + el, ec = line_col_from_index(end) + at_edits.append({"startLine": sl, "startCol": sc, "endLine": el, "endCol": ec, "newText": repl_expanded}) + current_text = preview_text + pos = start + len(repl_expanded) + applied += 1
🧹 Nitpick comments (9)
.claude/prompts/nl-unity-suite-full.md (4)
20-22: Include get_sha in the “Always pass” clause for consistency
mcp__unity__get_shalikely shares the same context contract; call it out to avoid ambiguity.-- Always pass: `project_root: "TestProjects/UnityMCPTests"` and `ctx: {}` on list/read/edit/validate. +- Always pass: `project_root: "TestProjects/UnityMCPTests"` and `ctx: {}` on list/read/edit/validate/get_sha.
59-63: Specify range format in fallback step“Insert before that index with one non‑overlapping range” is ambiguous. Tie it to LSP/explicit ranges as defined below.
- - Insert the three comment lines immediately before that index with one non‑overlapping range. + - Insert the three comment lines immediately before that brace using a zero‑width LSP range (or explicit 1‑based range) at that position.
83-88: Verifystrict:trueis a supported parameterIf
strictisn’t in the schema, this will yieldmissing_field/bad_request. Either drop it or document actual debug flags supported by the server.
91-101: Minor markdown lint fixes (optional)Indentation and emphasis-as-heading nits flagged by linters (MD007/MD036). Not blocking but easy to clean.
tests/test_regex_delete_guard.py (4)
37-41: Clean up DummyMCP decorator; avoid semicolons and unused args.Use conventional formatting and underscore unused parameters to satisfy linters.
-class DummyMCP: - def __init__(self): self.tools = {} - def tool(self, *args, **kwargs): - def deco(fn): self.tools[fn.__name__] = fn; return fn - return deco +class DummyMCP: + def __init__(self): + self.tools = {} + + def tool(self, *_args, **_kwargs): + def deco(fn): + self.tools[fn.__name__] = fn + return fn + return deco
26-30: Harden dynamic loader against None spec/loader.Guard against importlib returning None to avoid AttributeError in edge cases.
def _load(path: pathlib.Path, name: str): - spec = importlib.util.spec_from_file_location(name, path) - mod = importlib.util.module_from_spec(spec) - spec.loader.exec_module(mod) + spec = importlib.util.spec_from_file_location(name, path) + if spec is None or spec.loader is None: + raise ImportError(f"Cannot load module {name} from {path}") + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) return mod
49-87: Assert no write occurred in failure case.Track calls and assert only the initial read happened; strengthens the regression signal.
- def fake_send(cmd, params): + calls = [] + def fake_send(cmd, params): + calls.append((cmd, params.get("action"))) # Only the initial read should be invoked; provide contents if cmd == "manage_script" and params.get("action") == "read": return {"success": True, "data": {"contents": contents}} # If preflight failed as intended, no write should be attempted; return a marker if called return {"success": True, "message": "SHOULD_NOT_WRITE"} @@ assert "delete_method" in (data.get("hint") or "") + # Ensure no write attempts were made + assert calls.count(("manage_script", "read")) == 1 + assert all(a != "apply_text_edits" and a != "edit" for (_, a) in calls)
1-1: Silence Bandit S101 for test assertions.If Bandit runs on tests, add a file-level ignore to avoid noisy S101 warnings.
+# ruff: noqa: S101 import sysUnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
60-66: replace_range local preview: clamp columns to visible line length.Current index_of can overshoot EOL; clamp to [1, len(line)+1] to match LSP semantics. Also avoid ambiguous loop var.
- def index_of(line: int, col: int) -> int: - if line <= len(lines): - return sum(len(l) for l in lines[: line - 1]) + (col - 1) - return sum(len(l) for l in lines) + def _visible_len(s: str) -> int: + return len(s[:-1]) if s.endswith("\n") else len(s) + def index_of(line: int, col: int) -> int: + if line <= len(lines): + base = sum(len(x) for x in lines[: line - 1]) + max_col = _visible_len(lines[line - 1]) + 1 # allow EOL insertion + c = max(1, min(col, max_col)) + return base + (c - 1) + return sum(len(x) for x in lines)
📜 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 (4)
.claude/prompts/nl-unity-suite-full.md(1 hunks)UnityMcpBridge/UnityMcpServer~/src/server_version.txt(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py(1 hunks)tests/test_regex_delete_guard.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- UnityMcpBridge/UnityMcpServer~/src/server_version.txt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T21:31:02.427Z
Learnt from: dsarno
PR: dsarno/unity-mcp#56
File: .claude/prompts/nl-unity-suite-full.md:5-6
Timestamp: 2025-08-29T21:31:02.427Z
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:
.claude/prompts/nl-unity-suite-full.md
🧬 Code graph analysis (2)
tests/test_regex_delete_guard.py (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
register_manage_script_edits_tools(234-815)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(380-399)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
line_col_from_index(112-120)
🪛 Ruff (0.12.2)
tests/test_regex_delete_guard.py
16-16: Multiple statements on one line (colon)
(E701)
38-38: Unused method argument: args
(ARG002)
38-38: Unused method argument: kwargs
(ARG002)
39-39: Multiple statements on one line (semicolon)
(E702)
80-80: Use of assert detected
(S101)
81-81: Use of assert detected
(S101)
82-82: Use of assert detected
(S101)
84-84: Use of assert detected
(S101)
86-86: Use of assert detected
(S101)
147-147: Use of assert detected
(S101)
149-149: Use of assert detected
(S101)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py
23-25: Avoid specifying long messages outside the exception class
(TRY003)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
59-59: Avoid specifying long messages outside the exception class
(TRY003)
61-61: Function definition does not bind loop variable lines
(B023)
62-62: Ambiguous variable name: l
(E741)
62-62: Function definition does not bind loop variable lines
(B023)
63-63: Ambiguous variable name: l
(E741)
63-63: Function definition does not bind loop variable lines
(B023)
79-79: Avoid specifying long messages outside the exception class
(TRY003)
131-131: Multiple statements on one line (semicolon)
(E702)
131-131: Multiple statements on one line (semicolon)
(E702)
131-131: Multiple statements on one line (semicolon)
(E702)
133-133: Multiple statements on one line (semicolon)
(E702)
133-133: Multiple statements on one line (semicolon)
(E702)
133-133: Multiple statements on one line (semicolon)
(E702)
134-134: Multiple statements on one line (colon)
(E701)
135-135: Multiple statements on one line (colon)
(E701)
136-136: Multiple statements on one line (colon)
(E701)
137-137: Multiple statements on one line (colon)
(E701)
138-138: Multiple statements on one line (colon)
(E701)
139-139: Multiple statements on one line (colon)
(E701)
260-260: Unused function argument: ctx
(ARG001)
335-335: Comment contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF003)
335-335: Comment contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF003)
556-556: Function definition does not bind loop variable m
(B023)
593-593: Do not catch blind exception: Exception
(BLE001)
680-680: Function definition does not bind loop variable m
(B023)
730-730: Do not catch blind exception: Exception
(BLE001)
741-741: Consider [*diff[:800], "... (diff truncated) ..."] instead of concatenation
Replace with [*diff[:800], "... (diff truncated) ..."]
(RUF005)
745-745: Do not catch blind exception: Exception
(BLE001)
750-750: Do not catch blind exception: Exception
(BLE001)
769-769: Consider [*diff[:2000], "... (diff truncated) ..."] instead of concatenation
Replace with [*diff[:2000], "... (diff truncated) ..."]
(RUF005)
🪛 LanguageTool
.claude/prompts/nl-unity-suite-full.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...this once, verbatim, early in the run:** AllowedTools: Write,Bash(printf:*),Bash(...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...ole,mcp__unity__get_sha --- ## Mission 1) Pick target file (prefer): - `unity:/...
(QB_NEW_EN)
[grammar] ~20-~20: There might be a mistake here.
Context: ...write. --- ## 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] ~24-~24: There might be a mistake here.
Context: ...the URI) - Relative (when supported): Assets/... - File paths for the helper script are wor...
(QB_NEW_EN)
[grammar] ~30-~30: There might be a mistake here.
Context: ...it-nl-suite.md(synthesized from JUnit) - Helper script:scripts/nlt-revert.sh` (...
(QB_NEW_EN)
[grammar] ~71-~71: There might be a mistake here.
Context: ...er (T‑A/T‑E) - Prefer structured delete: - Use script_apply_edits with `{ "op":"d...
(QB_NEW_EN)
[grammar] ~72-~72: There might be a mistake here.
Context: ...tSeries" }(or__TempHelper` for T‑A). - If structured delete is unavailable, fal...
(QB_NEW_EN)
[grammar] ~75-~75: There might be a mistake here.
Context: ...ex deletes. # T‑B (replace method body) - Use mcp__unity__apply_text_edits with ...
(QB_NEW_EN)
[grammar] ~76-~76: There might be a mistake here.
Context: ... strictly inside the HasTarget braces. - Compute start/end from a fresh `read_res...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ...h read_resource at test start. Do not edit signature or header. - On `{status:"sta...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ... start. Do not edit signature or header. - On {status:"stale_file"} retry once wi...
(QB_NEW_EN)
[grammar] ~78-~78: There might be a mistake here.
Context: ...hash; if absent, re-read once and retry. - On bad_request: write the testcase wit...
(QB_NEW_EN)
[grammar] ~79-~79: There might be a mistake here.
Context: ...ure>…, restore, and continue to next test. - On missing_field`: FALL B...
(QB_NEW_EN)
[grammar] ~79-~79: There might be a mistake here.
Context: ...e>, restore, and continue to next test. - On missing_field`: FALL BACK per above;...
(QB_NEW_EN)
[style] ~80-~80: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..., restore, and continue to next test. - On missing_field: FALL BACK per above; i...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~83-~83: There might be a mistake here.
Context: ...y. Span formats for apply_text_edits: - Prefer LSP ranges (0‑based): `{ "range":...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ..._edits: - Prefer LSP ranges (0‑based): { "range": { "start": {"line": L, "character": C}, "end": {…} }, "newText": "…" }- Explicit fields are 1‑based:{ "startLi...
(QB_NEW_EN)
[grammar] ~85-~85: There might be a mistake here.
Context: ..."newText": "…" }- Explicit fields are 1‑based:{ "startLine": L1, "startCol": C1, "endLine": L2, "endCol": C2, "newText": "…" }` - SDK preflights overlap after normalizati...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ..."}with conflicts and no file mutation. - Optional debug: passstrict:true` to re...
(QB_NEW_EN)
[grammar] ~93-~93: There might be a mistake here.
Context: ...tem-out>. - If content contains ]]>`, split CDATA: ...
(QB_NEW_EN)
[grammar] ~144-~144: There might be a mistake here.
Context: ...fore any further edits in the same test. - Prefer anchors (script_apply_edits) fo...
(QB_NEW_EN)
[grammar] ~147-~147: There might be a mistake here.
Context: ...rrors (timeout, EOF, connection closed):** - Write reports/<TESTID>_results.xml wit...
(QB_NEW_EN)
[grammar] ~148-~148: There might be a mistake here.
Context: ...<error> node capturing the error text. - Run the OS restore via `scripts/nlt-reve...
(QB_NEW_EN)
[grammar] ~153-~153: There might be a mistake here.
Context: ... evidence, and end with VERDICT: FAIL. - Run scripts/nlt-revert.sh restore ... ...
(QB_NEW_EN)
[grammar] ~157-~157: There might be a mistake here.
Context: ...T-E, T-F, T-G, T-H, T-I, T-J (15 total). - Before NL-1..T-J: Bash(scripts/nlt-rever...
(QB_NEW_EN)
[grammar] ~158-~158: There might be a mistake here.
Context: ...) IF the baseline exists; skip for NL-0. - NL-0 must include the PLAN line (len=15)...
(QB_NEW_EN)
[grammar] ~166-~166: There might be a mistake here.
Context: ...verify; delete PrintSeries(); restore. - NL‑2. Anchor comment — Insert `// Build ...
(QB_NEW_EN)
[grammar] ~167-~167: There might be a mistake here.
Context: ...bove public void Update(...); restore. - NL‑3. End‑of‑class — Insert `// Tail tes...
(QB_NEW_EN)
[grammar] ~168-~168: There might be a mistake here.
Context: ...— Insert // Tail test A/B/C (3 lines) before final brace; restore. - NL‑4. Compile t...
(QB_NEW_EN)
[grammar] ~168-~168: There might be a mistake here.
Context: ...` (3 lines) before final brace; restore. - NL‑4. Compile trigger — Record INFO only...
(QB_NEW_EN)
[grammar] ~171-~171: There might be a mistake here.
Context: ...fy; delete via regex_replace; restore. ### T‑B. Replace body — Single `replace_rang...
(QB_NEW_EN)
[grammar] ~172-~172: There might be a mistake here.
Context: ...lace_rangeinsideHasTarget`; restore. ### T‑C. Header/region preservation — Edit i...
(QB_NEW_EN)
[grammar] ~173-~173: There might be a mistake here.
Context: ...reserve signature/docs/regions; restore. ### T‑D. End‑of‑class (anchor) — Insert help...
(QB_NEW_EN)
[grammar] ~174-~174: There might be a mistake here.
Context: ...per before final brace; remove; restore. ### T‑E. Lifecycle — Insert → update → delet...
(QB_NEW_EN)
[grammar] ~175-~175: There might be a mistake here.
Context: ...rt → update → delete via regex; restore. ### T‑F. Atomic batch — One `mcp__unity__app...
(QB_NEW_EN)
[grammar] ~176-~176: There might be a mistake here.
Context: ...pply_text_edits` call (text ranges only) - Compute all three edits from the **same ...
(QB_NEW_EN)
[grammar] ~177-~177: There might be a mistake here.
Context: ...hree edits from the same fresh read: 1) Two small interior replace_range tweak...
(QB_NEW_EN)
[grammar] ~178-~178: There might be a mistake here.
Context: ...o small interior replace_range tweaks. 2) One end‑of‑class insertion: find the...
(QB_NEW_EN)
[grammar] ~179-~179: There might be a mistake here.
Context: ...eplacement` to the 3‑line comment block. - Send all three ranges in one call, s...
(QB_NEW_EN)
[grammar] ~181-~181: There might be a mistake here.
Context: ...…`, restore, and continue. - T‑G. Path normalization — Make the same ...
(QB_NEW_EN)
[grammar] ~186-~186: There might be a mistake here.
Context: ...nterior tweak (or none), then MUST call: mcp__unity__validate_script(level:"stand...
(QB_NEW_EN)
[grammar] ~187-~187: There might be a mistake here.
Context: ...unity__validate_script(level:"standard") - Write the validator output to system-out...
(QB_NEW_EN)
[grammar] ~194-~194: There might be a mistake here.
Context: ... both spans come from the same snapshot. * Single mcp__unity__apply_text_edits call...
(QB_NEW_EN)
[grammar] ~199-~199: There might be a mistake here.
Context: ...a tiny legit edit with pre_sha; success. * Attempt another edit reusing the OLD pre...
(QB_NEW_EN)
[grammar] ~203-~203: There might be a mistake here.
Context: ...### Per‑test error handling and recovery - For each test (NL‑0..T‑J), use a try/fin...
(QB_NEW_EN)
[grammar] ~204-~204: There might be a mistake here.
Context: ... (NL‑0..T‑J), use a try/finally pattern: - try: run the test steps; always write `r...
(QB_NEW_EN)
[grammar] ~207-~207: There might be a mistake here.
Context: ... - On any transport/JSON/tool exception: - catch and write a <testcase> fragment ...
(QB_NEW_EN)
[grammar] ~208-~208: There might be a mistake here.
Context: ...message), then proceed to the next test. - After NL‑4 completes, proceed directly t...
(QB_NEW_EN)
[grammar] ~209-~209: There might be a mistake here.
Context: ...lidator warnings (do not abort the run). - (3) USING_GUARD (optional): * Attempt ...
(QB_NEW_EN)
[grammar] ~210-~210: There might be a mistake here.
Context: ... the run). - (3) USING_GUARD (optional): * Attempt a 1-line insert above the first ...
(QB_NEW_EN)
[grammar] ~216-~216: There might be a mistake here.
Context: ...aseline. - Repeat a replace_range twice (second call may be noop). Validate stand...
(QB_NEW_EN)
[grammar] ~216-~216: There might be a mistake here.
Context: ... a replace_range twice (second call may be noop). Validate standard after each. - ...
(QB_NEW_EN)
[grammar] ~216-~216: There might be a mistake here.
Context: ..._range twice (second call may be noop). Validate standard after each. - Insert or ensure...
(QB_NEW_EN)
[grammar] ~216-~216: There might be a mistake here.
Context: ... be noop). Validate standard after each. - Insert or ensure a tiny comment, then de...
(QB_NEW_EN)
[grammar] ~217-~217: There might be a mistake here.
Context: ...re a tiny comment, then delete it twice (second delete may be noop). - Restore an...
(QB_NEW_EN)
[grammar] ~217-~217: There might be a mistake here.
Context: ...then delete it twice (second delete may be noop). - Restore and PASS unless an err...
(QB_NEW_EN)
[grammar] ~217-~217: There might be a mistake here.
Context: ...te it twice (second delete may be noop). - Restore and PASS unless an error/structu...
(QB_NEW_EN)
[grammar] ~223-~223: There might be a mistake here.
Context: ...uses are non‑fatal; record and continue. - End each testcase <system-out> with `V...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
.claude/prompts/nl-unity-suite-full.md
94-94: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
104-104: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
171-171: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
172-172: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
173-173: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
174-174: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
175-175: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
177-177: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
180-180: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
181-181: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
182-182: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 0
(MD005, list-indent)
193-193: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
194-194: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
195-195: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
196-196: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
198-198: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
199-199: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
200-200: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
201-201: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
211-211: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
212-212: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
⏰ 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
| ### T‑F. Atomic batch — One `mcp__unity__apply_text_edits` call (text ranges only) | ||
| - Compute all three edits from the **same fresh read**: | ||
| 1) Two small interior `replace_range` tweaks. | ||
| 2) One **end‑of‑class insertion**: find the **index of the final `}`** for the class; create a zero‑width range `[idx, idx)` and set `replacement` to the 3‑line comment block. | ||
| - Send all three ranges in **one call**, sorted **descending by start index** to avoid offset drift. | ||
| - Expect all‑or‑nothing semantics; on `{status:"overlap"}` or `{status:"bad_request"}`, write the testcase fragment with `<failure>…</failure>`, **restore**, and continue. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use LSP/explicit ranges; avoid index-based spans in T‑F
apply_text_edits here uses “[idx, idx)” offsets, which aren’t part of the documented span formats below. Specify LSP or explicit line/col for the end‑of‑class insertion.
-### T‑F. Atomic batch — One `mcp__unity__apply_text_edits` call (text ranges only)
- - Compute all three edits from the **same fresh read**:
- 1) Two small interior `replace_range` tweaks.
- 2) One **end‑of‑class insertion**: find the **index of the final `}`** for the class; create a zero‑width range `[idx, idx)` and set `replacement` to the 3‑line comment block.
- - Send all three ranges in **one call**, sorted **descending by start index** to avoid offset drift.
+### T‑F. Atomic batch — One `mcp__unity__apply_text_edits` call (text ranges only)
+ - Compute all three edits from the **same fresh read**:
+ 1) Two small interior `replace_range` tweaks.
+ 2) One **end‑of‑class insertion**: locate the final `}` and compute its LSP position; create a zero‑width LSP range with `start == end` at that position and set `newText` to the 3‑line comment block.
+ - Send all three non‑overlapping ranges in **one call** (ordering not required); expect all‑or‑nothing semantics.📝 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.
| ### T‑F. Atomic batch — One `mcp__unity__apply_text_edits` call (text ranges only) | |
| - Compute all three edits from the **same fresh read**: | |
| 1) Two small interior `replace_range` tweaks. | |
| 2) One **end‑of‑class insertion**: find the **index of the final `}`** for the class; create a zero‑width range `[idx, idx)` and set `replacement` to the 3‑line comment block. | |
| - Send all three ranges in **one call**, sorted **descending by start index** to avoid offset drift. | |
| - Expect all‑or‑nothing semantics; on `{status:"overlap"}` or `{status:"bad_request"}`, write the testcase fragment with `<failure>…</failure>`, **restore**, and continue. | |
| ### T-F. Atomic batch — One `mcp__unity__apply_text_edits` call (text ranges only) | |
| - Compute all three edits from the **same fresh read**: | |
| 1) Two small interior `replace_range` tweaks. | |
| 2) One **end-of-class insertion**: locate the final `}` and compute its LSP position; create a zero-width LSP range with `start == end` at that position and set `newText` to the 3-line comment block. | |
| - Send all three non-overlapping ranges in **one call** (ordering not required); expect all-or-nothing semantics. |
🧰 Tools
🪛 LanguageTool
[grammar] ~176-~176: There might be a mistake here.
Context: ...pply_text_edits` call (text ranges only) - Compute all three edits from the **same ...
(QB_NEW_EN)
[grammar] ~177-~177: There might be a mistake here.
Context: ...hree edits from the same fresh read: 1) Two small interior replace_range tweak...
(QB_NEW_EN)
[grammar] ~178-~178: There might be a mistake here.
Context: ...o small interior replace_range tweaks. 2) One end‑of‑class insertion: find the...
(QB_NEW_EN)
[grammar] ~179-~179: There might be a mistake here.
Context: ...eplacement` to the 3‑line comment block. - Send all three ranges in one call, s...
(QB_NEW_EN)
[grammar] ~181-~181: There might be a mistake here.
Context: ...…`, restore, and continue. - T‑G. Path normalization — Make the same ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
177-177: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
180-180: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
181-181: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🤖 Prompt for AI Agents
In .claude/prompts/nl-unity-suite-full.md around lines 176–181, the guidance
currently uses index‑based “[idx, idx)” offsets for the end‑of‑class insertion
which are not part of the documented span formats; change the insertion to use
an LSP range or explicit line/column coordinates instead. Update the text to
describe how to compute the final class closing brace position as a line/column
(e.g., find the line and column of the last '}' in the fresh file read),
construct a zero‑length LSP range at that line/column, and set the replacement
to the 3‑line comment block; keep the batching rules (send all three edits in
one call sorted descending by start position) and the error handling behavior
unchanged. Ensure examples and any sample payloads use the documented
LSP/line‑col span format rather than numeric index offsets.
| elif op == "anchor_insert": | ||
| anchor = edit.get("anchor", "") | ||
| position = (edit.get("position") or "before").lower() | ||
| insert_text = edit.get("text", "") | ||
| flags = re.MULTILINE | (re.IGNORECASE if edit.get("ignore_case") else 0) | ||
| m = re.search(anchor, text, flags) | ||
| if not m: | ||
| if edit.get("allow_noop", True): | ||
| continue | ||
| raise RuntimeError(f"anchor not found: {anchor}") | ||
| idx = m.start() if position == "before" else m.end() | ||
| text = text[:idx] + insert_text + text[idx:] | ||
| elif op == "replace_range": |
There was a problem hiding this comment.
anchor_insert: default position inconsistent with rest of file; also normalize newlines.
Elsewhere you default to position='after' and add newline padding. Local apply currently uses 'before' and no padding, causing preview/runtime drift.
- elif op == "anchor_insert":
- anchor = edit.get("anchor", "")
- position = (edit.get("position") or "before").lower()
- insert_text = edit.get("text", "")
- flags = re.MULTILINE | (re.IGNORECASE if edit.get("ignore_case") else 0)
- m = re.search(anchor, text, flags)
+ elif op == "anchor_insert":
+ anchor = edit.get("anchor", "")
+ position = (edit.get("position") or "after").lower()
+ insert_text = edit.get("text", "")
+ flags = re.MULTILINE | (re.IGNORECASE if edit.get("ignore_case") else 0)
+ m = re.search(anchor, text, flags)
if not m:
if edit.get("allow_noop", True):
continue
raise RuntimeError(f"anchor not found: {anchor}")
idx = m.start() if position == "before" else m.end()
- text = text[:idx] + insert_text + text[idx:]
+ # Normalize insertion to avoid jammed methods
+ insert_text_norm = insert_text
+ if insert_text_norm and not insert_text_norm.startswith("\n"):
+ insert_text_norm = "\n" + insert_text_norm
+ if insert_text_norm and not insert_text_norm.endswith("\n"):
+ insert_text_norm = insert_text_norm + "\n"
+ text = text[:idx] + insert_text_norm + text[idx:]📝 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.
| elif op == "anchor_insert": | |
| anchor = edit.get("anchor", "") | |
| position = (edit.get("position") or "before").lower() | |
| insert_text = edit.get("text", "") | |
| flags = re.MULTILINE | (re.IGNORECASE if edit.get("ignore_case") else 0) | |
| m = re.search(anchor, text, flags) | |
| if not m: | |
| if edit.get("allow_noop", True): | |
| continue | |
| raise RuntimeError(f"anchor not found: {anchor}") | |
| idx = m.start() if position == "before" else m.end() | |
| text = text[:idx] + insert_text + text[idx:] | |
| elif op == "replace_range": | |
| elif op == "anchor_insert": | |
| anchor = edit.get("anchor", "") | |
| position = (edit.get("position") or "after").lower() | |
| insert_text = edit.get("text", "") | |
| flags = re.MULTILINE | (re.IGNORECASE if edit.get("ignore_case") else 0) | |
| m = re.search(anchor, text, flags) | |
| if not m: | |
| if edit.get("allow_noop", True): | |
| continue | |
| raise RuntimeError(f"anchor not found: {anchor}") | |
| idx = m.start() if position == "before" else m.end() | |
| # Normalize insertion to avoid jammed methods | |
| insert_text_norm = insert_text | |
| if insert_text_norm and not insert_text_norm.startswith("\n"): | |
| insert_text_norm = "\n" + insert_text_norm | |
| if insert_text_norm and not insert_text_norm.endswith("\n"): | |
| insert_text_norm = insert_text_norm + "\n" | |
| text = text[:idx] + insert_text_norm + text[idx:] | |
| elif op == "replace_range": |
🧰 Tools
🪛 Ruff (0.12.2)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
There was a problem hiding this comment.
Actionable comments posted: 0
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/unity_connection.py (2)
318-345: Backoff bug: computedbackoffis unused; actual sleep ignoresbase_backoffThis yields much smaller-than-intended delays and defeats the base backoff knob.
- # Base exponential backoff - backoff = base_backoff * (2 ** attempt) - # Decorrelated jitter multiplier - jitter = random.uniform(0.1, 0.3) + # Base exponential backoff + backoff = base_backoff * (2 ** attempt) + # Jitter multiplier (±50%) + jitter = random.uniform(0.5, 1.5) ... - sleep_s = min(cap, jitter * (2 ** attempt)) + sleep_s = min(cap, backoff * jitter)
154-205: Normalize exception types in legacy (unframed) receive for consistent retry policyRaising generic Exception masks timeouts as “slow” errors; use ConnectionError/TimeoutError like the framed path so fast_error classification works.
- if not chunks: - raise Exception("Connection closed before receiving data") + if not chunks: + raise ConnectionError("Connection closed before receiving data") ... - except socket.timeout: - logger.warning("Socket timeout during receive") - raise Exception("Timeout receiving Unity response") + except socket.timeout: + logger.warning("Socket timeout during receive") + raise TimeoutError("Timeout receiving Unity response") - except Exception as e: - logger.error(f"Error during receive: {str(e)}") + except Exception as e: + logger.exception("Error during receive: %s", str(e)) raise
♻️ Duplicate comments (2)
tests/test_transport_framing.py (1)
86-109: Use monotonic clock for the pre-handshake poll windowMonotonic avoids time jumps; keeps the short poll window semantics.
- deadline = time.time() + 0.15 # short, reduces race with legitimate clients - while time.time() < deadline: + deadline = time.monotonic() + 0.15 # short, reduces race with legitimate clients + while time.monotonic() < deadline:This addresses earlier feedback about the polling window/race; the tighter loop + monotonic improves robustness.
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
262-267: Don’t log payload previews; risk of secret leakageThe head snippet can include API keys or PII. Keep size/mode, drop content.
- with contextlib.suppress(Exception): - logger.debug( - "send %d bytes; mode=%s; head=%s", - len(payload), - mode, - (payload[:32]).decode('utf-8', 'ignore'), - ) + with contextlib.suppress(Exception): + logger.debug("send %d bytes; mode=%s", len(payload), mode)Earlier reviews flagged this; this change removes the leak vector while retaining useful telemetry.
🧹 Nitpick comments (5)
UnityMcpBridge/UnityMcpServer~/src/config.py (1)
17-23: Expose handshake/connect settings in ServerConfigApply:
# Connection settings connection_timeout: float = 60.0 # default steady-state timeout; retries use shorter timeouts buffer_size: int = 16 * 1024 * 1024 # 16MB buffer + # Handshake/connect settings + require_framing: bool = True + handshake_timeout: float = 1.0 # seconds + connect_timeout: float = 1.0 # seconds (short, separate from steady-state receive) # Framed receive behavior framed_receive_timeout: float = 2.0 # max seconds to wait while consuming heartbeats only max_heartbeat_frames: int = 16 # cap heartbeat frames consumed before giving uptests/test_transport_framing.py (2)
32-36: Reduce flakiness: enable SO_REUSEADDR on test serversPrevents bind failures under TIME_WAIT during fast CI reruns.
sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) sock.bind(("127.0.0.1", 0))
170-187: Style: avoid single-line try/except; keep close in finallyMinor readability fix; helps static analyzers.
- finally: - try: conn.close() - except Exception: pass - sock.close() + finally: + try: + conn.close() + except Exception: + pass + sock.close()UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
229-288: Remove dead state:last_short_timeoutis never set; simplify retry-timeout handlingUse only the local
restore_timeoutvariable.- last_short_timeout = None + # (unused: last_short_timeout removed; rely on local restore_timeout) ... - if attempt > 0 and last_short_timeout is None: + if attempt > 0: restore_timeout = self.sock.gettimeout() self.sock.settimeout(1.0) ... - last_short_timeout = None
190-193: Lower log level for per-response message to reduce noiseSuccess-path JSON size logs fit better at debug.
- logger.info(f"Received complete response ({len(data)} bytes)") + logger.debug("Received complete response (%d bytes)", len(data))
📜 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 (3)
UnityMcpBridge/UnityMcpServer~/src/config.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py(6 hunks)tests/test_transport_framing.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
UnityMcpBridge/UnityMcpServer~/src/port_discovery.py (2)
PortDiscovery(24-155)discover_unity_port(93-134)tests/test_transport_framing.py (1)
_read_exact(47-54)
tests/test_transport_framing.py (1)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (5)
UnityConnection(30-346)_read_exact(114-121)connect(44-102)receive_full_response(123-205)disconnect(104-112)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py
87-87: Avoid specifying long messages outside the exception class
(TRY003)
93-93: Consider moving this statement to an else block
(TRY300)
94-94: Do not catch blind exception: Exception
(BLE001)
95-95: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
95-95: Use explicit conversion flag
Replace with conversion flag
(RUF010)
99-100: try-except-pass detected, consider logging the exception
(S110)
99-99: Do not catch blind exception: Exception
(BLE001)
119-119: Avoid specifying long messages outside the exception class
(TRY003)
143-143: Abstract raise to an inner function
(TRY301)
143-143: Avoid specifying long messages outside the exception class
(TRY003)
149-149: Avoid specifying long messages outside the exception class
(TRY003)
151-151: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
151-151: Use explicit conversion flag
Replace with conversion flag
(RUF010)
249-249: Abstract raise to an inner function
(TRY301)
249-249: Create your own exception
(TRY002)
249-249: Avoid specifying long messages outside the exception class
(TRY003)
369-369: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_transport_framing.py
63-64: try-except-pass detected, consider logging the exception
(S110)
63-63: Do not catch blind exception: Exception
(BLE001)
68-69: try-except-pass detected, consider logging the exception
(S110)
68-68: Do not catch blind exception: Exception
(BLE001)
100-100: Do not catch blind exception: Exception
(BLE001)
124-124: Use of assert detected
(S101)
125-125: Use of assert detected
(S101)
132-132: Use of assert detected
(S101)
133-133: Use of assert detected
(S101)
137-137: Use of assert detected
(S101)
151-151: Use of assert detected
(S101)
184-184: Multiple statements on one line (colon)
(E701)
185-185: try-except-pass detected, consider logging the exception
(S110)
185-185: Do not catch blind exception: Exception
(BLE001)
185-185: Multiple statements on one line (colon)
(E701)
193-193: Use of assert detected
(S101)
196-196: Use of assert detected
(S101)
⏰ 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: Cursor Bugbot
🔇 Additional comments (4)
UnityMcpBridge/UnityMcpServer~/src/config.py (1)
20-22: Heartbeat/framed-receive knobs look good; align with usage in unity_connectionThese defaults match the client logic (deadline + max frames) and are safe.
tests/test_transport_framing.py (1)
160-197: Nice: explicit zero-length heartbeat test aligns with client’s bounded-skip behaviorConfirms the client returns either payload or empty after threshold/timeout. Looks good.
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
60-93: Handshake is robust and strict; plain-text advisory to legacy peers is correctLoop-until-newline with a monotonic deadline, FRAMING=1 enforcement, and advisory text (not framed) are all on point.
125-147: Framed receive: bounded heartbeat consumption matches server semanticsZero-length frames treated as heartbeats with both a frame-count cap and a deadline; returning b"" after threshold is consistent with tests. Good.
…for structured ops; forward options on apply_text_edits; add validate=relaxed support and scoped checks; update NL/T prompt; add tests for options forwarding, relaxed mode, and atomic batches
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (11)
.claude/prompts/nl-unity-suite-full.md (4)
75-79: Drop unsupported op delete_method; use supported ops for deletion
delete_methodisn’t in the allowed op set (anchor_insert,replace_range,regex_replace). Keep instructions executable with supported ops.-# Delete the temporary helper (T‑A/T‑E) -- Prefer structured delete: - - Use `script_apply_edits` with `{ "op":"delete_method", "className":"LongUnityScriptClaudeTest", "methodName":"PrintSeries" }` (or `__TempHelper` for T‑A). -- If structured delete is unavailable, fall back to `apply_text_edits` with a single `replace_range` spanning the exact method block (bounds computed from a fresh read); avoid whole‑file regex deletes. +# Delete the temporary helper (T‑A/T‑E) +- Prefer `regex_replace` targeting the exact method block, or `apply_text_edits` with a single `replace_range` spanning the method (bounds from a fresh read); avoid whole‑file regex deletes.Optional concrete regex (engine supports inline flags):
{"op":"regex_replace","pattern":"(?ms)^\\s*private\\s+int\\s+__TempHelper\\s*\\(.*?\\)\\s*=>\\s*.*?;\\s*\\r?\\n","replacement":""}
182-187: T‑F uses index-based spans; switch to LSP positions for portability and clarityIndices drift and aren’t part of the documented span formats. Use a zero‑length LSP range at the final brace position; ordering need not be enforced.
-### T‑F. Atomic batch — One `mcp__unity__apply_text_edits` call (text ranges only) - - Compute all three edits from the **same fresh read**: - 1) Two small interior `replace_range` tweaks. - 2) One **end‑of‑class insertion**: find the **index of the final `}`** for the class; create a zero‑width range `[idx, idx)` and set `replacement` to the 3‑line comment block. - - Send all three ranges in **one call**, sorted **descending by start index** to avoid offset drift. +### T‑F. Atomic batch — One `mcp__unity__apply_text_edits` call (text ranges only) + - Compute all three edits from the **same fresh read**: + 1) Two small interior `replace_range` tweaks. + 2) One **end‑of‑class insertion**: locate the final `}` and compute its LSP position; create a zero‑width LSP range with `start == end` at that position and set `newText` to the 3‑line comment block. + - Send all three non‑overlapping ranges in **one call** (ordering not required); expect all‑or‑nothing semantics.
140-141: Unexecutable fallback path: agent cannot “restore using baseline bytes”Agents don’t have a tool to read baseline bytes. Fail deterministically and continue.
-- If the helper fails: fall back once to a guarded full‑file restore using the baseline bytes; then continue. +- If the helper fails: write the testcase with an `<error>` (restore failure), then continue to the next test.
108-109: Resolve contradiction: reporting requires Write, but text says only revert may mutate FSThis will mislead agents and break JUnit fragment writes. Allow exactly Write (to results files) and the revert helper; keep Bash stdout-only.
- - Bash(printf|echo) must write to stdout only. Do not use shell redirection, here‑docs, or `tee` to create/modify files. The only allowed FS mutation is via `scripts/nlt-revert.sh`. + - Bash(printf|echo) must write to stdout only. Do not use shell redirection, here‑docs, or `tee`. + - Only these may modify the filesystem: + 1) Write → `reports/*_results.xml` (per path rules above) + 2) `scripts/nlt-revert.sh` → snapshot/restoreUnityMcpBridge/Editor/Tools/ManageScript.cs (4)
496-513: Symlink guard should fail closed when attributes cannot be checked.Don’t silently drop the guard on exceptions.
- } - catch - { - // If checking attributes fails, proceed without the symlink guard - } + } + catch + { + return Response.Error("Refusing to edit a symlinked script path (attributes check failed)."); + }
485-486: Raise edit payload cap to 256 KiB (aligns with client/tools and prevents routine rejections).Current 15 KiB is too low and contradicts manage_script_capabilities (256 KiB).
- private const int MaxEditPayloadBytes = 15 * 1024; + private const int MaxEditPayloadBytes = 256 * 1024;
80-107: Fail closed on symlink-attribute check errors in TryResolveUnderAssets.Proceeding on exceptions weakens the guard; reject when attribute probing fails.
- } - catch { /* best effort; proceed */ } + } + catch + { + fullPathDir = null; + relPathSafe = null; + return false; // fail closed if we can't verify ancestors + }
587-589: Add a timeout to the auto‑upgrade header regex to avoid UI hangs.Construct Regex with a small timeout like 250–500 ms.
- 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 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*\(", + System.Text.RegularExpressions.RegexOptions.CultureInvariant | System.Text.RegularExpressions.RegexOptions.Multiline, + TimeSpan.FromMilliseconds(250) + );UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
37-49: Align anchor_insert local-preview semantics with server: default to 'after' and pad with newlines.Prevents preview/runtime drift.
- elif op == "anchor_insert": - anchor = edit.get("anchor", "") - position = (edit.get("position") or "before").lower() - insert_text = edit.get("text", "") + elif op == "anchor_insert": + anchor = edit.get("anchor", "") + position = (edit.get("position") or "after").lower() + insert_text = edit.get("text", "") flags = re.MULTILINE | (re.IGNORECASE if edit.get("ignore_case") else 0) m = re.search(anchor, text, flags) if not m: if edit.get("allow_noop", True): continue raise RuntimeError(f"anchor not found: {anchor}") idx = m.start() if position == "before" else m.end() - text = text[:idx] + insert_text + text[idx:] + insert_text_norm = insert_text + if insert_text_norm and not insert_text_norm.startswith("\n"): + insert_text_norm = "\n" + insert_text_norm + if insert_text_norm and not insert_text_norm.endswith("\n"): + insert_text_norm = insert_text_norm + "\n" + text = text[:idx] + insert_text_norm + text[idx:]
49-66: Clamp replace_range columns to visible line bounds to avoid overshooting indices.Prevents surprising replacements when columns exceed line length.
- def index_of(line: int, col: int) -> int: - if line <= len(lines): - return sum(len(l) for l in lines[: line - 1]) + (col - 1) - return sum(len(l) for l in lines) + def _visible_len(s: str) -> int: + return len(s[:-1]) if s.endswith("\n") else len(s) + def index_of(line: int, col: int) -> int: + if line <= len(lines): + base = sum(len(x) for x in lines[: line - 1]) + max_col = _visible_len(lines[line - 1]) + 1 # allow EOL insertion + c = max(1, min(col, max_col)) + return base + (c - 1) + return sum(len(x) for x in lines)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
52-57: Do not strip leading “/” on POSIX absolute paths.Keep absolute paths intact; higher-level guards decide validity.
- if effective_path.startswith("/"): - effective_path = effective_path[1:] + // Keep POSIX absolute paths as-is; downstream guards enforce 'Assets/'.
🧹 Nitpick comments (7)
.claude/prompts/nl-unity-suite-full.md (1)
97-107: Minor Markdown lint/consistency nitsTighten list indentation and list style to reduce MD lint noise; remove trailing punctuation in headings.
Also applies to: 110-110, 183-188, 199-202, 217-218
UnityMcpBridge/Editor/Tools/ManageScript.cs (1)
754-783: Include scheduledRefresh in ApplyTextEdits response for parity with create/update/edit.Expose whether refresh was scheduled vs forced.
return Response.Success( $"Applied {spans.Count} text edit(s) to '{relativePath}'.", new { uri = $"unity://path/{relativePath}", path = relativePath, editsApplied = spans.Count, - sha256 = newSha + sha256 = newSha, + scheduledRefresh = !immediate } );tests/test_edit_normalization_and_noop.py (1)
67-80: Assert normalized coordinates for the index-pair case to close the loop.The test currently doesn’t verify the final apply payload.
- apply(None, uri="unity://path/Assets/Scripts/F.cs", edits=edits, precondition_sha256="x") - # last call is apply_text_edits - + apply(None, uri="unity://path/Assets/Scripts/F.cs", edits=edits, precondition_sha256="x") + # last call is apply_text_edits + last = calls[-1] + e2 = last["edits"][0] + assert e2["startLine"] == 1 and e2["startCol"] == 1 + assert e2["endLine"] == 1 and e2["endCol"] == 1tests/test_script_tools.py (1)
126-139: Prefer pytest.mark.asyncio over asyncio.run for async test integration.Improves runtime integration and reporting in pytest.
- asyncio.run(run()) + import pytest + @pytest.mark.asyncio + async def _case(): + await run() + asyncio.run(_case())UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
668-697: Optional: support count and ignore_case in regex_replace conversion (text path) and expand $n per match.Brings parity with local preview and mixed-path logic.
- elif op == "regex_replace": - pattern = e.get("pattern") or "" - repl = text_field - flags = _re.MULTILINE | (_re.IGNORECASE if e.get("ignore_case") else 0) - m = _re.search(pattern, base_text, flags) - if not m: - continue - # Expand $1, $2... backrefs in replacement using the first match (consistent with mixed-path behavior) - def _expand_dollars(rep: str) -> str: - return _re.sub(r"\$(\d+)", lambda g: m.group(int(g.group(1))) or "", rep) - repl_expanded = _expand_dollars(repl) - # Preview structural balance after replacement; refuse destructive deletes - preview = base_text[:m.start()] + repl_expanded + base_text[m.end():] - if not _is_structurally_balanced(preview): - return _with_norm({ - "success": False, - "code": "validation_failed", - "message": "regex_replace would unbalance braces/parentheses; prefer delete_method", - "data": {"status": "validation_failed", "normalizedEdits": normalized_for_echo, "hint": "Use script_apply_edits delete_method for method removal"} - }, normalized_for_echo, routing="text") - sl, sc = line_col_from_index(m.start()) - el, ec = line_col_from_index(m.end()) - at_edits.append({ - "startLine": sl, - "startCol": sc, - "endLine": el, - "endCol": ec, - "newText": repl_expanded - }) + elif op == "regex_replace": + pattern = e.get("pattern") or "" + repl = text_field + count = int(e.get("count", 0) or 0) # 0 = all + flags = _re.MULTILINE | (_re.IGNORECASE if e.get("ignore_case") else 0) + matches = list(_re.finditer(pattern, base_text, flags)) + if not matches: + continue + if count > 0: + matches = matches[:count] + for m in matches: + def _expand_dollars(rep: str) -> str: + return _re.sub(r"\$(\d+)", lambda g: m.group(int(g.group(1))) or "", rep) + repl_expanded = _expand_dollars(repl) + preview = base_text[:m.start()] + repl_expanded + base_text[m.end():] + if not _is_structurally_balanced(preview): + return _with_norm({"success": False, "code": "validation_failed", + "message": "regex_replace would unbalance braces/parentheses; prefer delete_method", + "data": {"status": "validation_failed", "normalizedEdits": normalized_for_echo}}, + normalized_for_echo, routing="text") + sl, sc = line_col_from_index(m.start()) + el, ec = line_col_from_index(m.end()) + at_edits.append({"startLine": sl, "startCol": sc, "endLine": el, "endCol": ec, "newText": repl_expanded})UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
81-92: Add an Assets/ guard early to avoid futile round-trips for invalid URIs.Return a structured failure if directory doesn’t resolve under Assets/.
name, directory = _split_uri(uri) + if not directory or directory.split("/")[0].lower() != "assets": + return {"success": False, "code": "path_outside_assets", "message": "URI must resolve under 'Assets/'."}
516-523: Apply the same Assets/ guard to get_sha for consistency with other tools.Prevents out-of-project calls.
- try: - name, directory = _split_uri(uri) - params = {"action": "get_sha", "name": name, "path": directory} + try: + name, directory = _split_uri(uri) + if not directory or directory.split("/")[0].lower() != "assets": + return {"success": False, "code": "path_outside_assets", "message": "URI must resolve under 'Assets/'."} + params = {"action": "get_sha", "name": name, "path": directory}
📜 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 (6)
.claude/prompts/nl-unity-suite-full.md(1 hunks)UnityMcpBridge/Editor/Tools/ManageScript.cs(14 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py(1 hunks)tests/test_edit_normalization_and_noop.py(1 hunks)tests/test_script_tools.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T21:31:02.427Z
Learnt from: dsarno
PR: dsarno/unity-mcp#56
File: .claude/prompts/nl-unity-suite-full.md:5-6
Timestamp: 2025-08-29T21:31:02.427Z
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:
.claude/prompts/nl-unity-suite-full.md
🧬 Code graph analysis (5)
tests/test_edit_normalization_and_noop.py (4)
tests/test_script_tools.py (2)
DummyMCP(39-47)tool(43-47)tests/test_edit_strict_and_warnings.py (4)
_load(25-29)DummyMCP(35-39)tool(37-39)deco(38-38)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
manage_script(358-483)register_manage_script_tools(9-524)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
register_manage_script_edits_tools(234-812)
UnityMcpBridge/Editor/Tools/ManageScript.cs (1)
UnityMcpBridge/Editor/Helpers/Response.cs (3)
Response(10-61)Error(41-60)Success(18-33)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (3)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(388-407)tests/test_manage_script_uri.py (1)
tool(46-50)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
line_col_from_index(508-512)line_col_from_index(621-626)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(388-407)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
line_col_from_index(113-121)
tests/test_script_tools.py (3)
tests/test_manage_script_uri.py (2)
DummyMCP(42-50)tool(46-50)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
register_manage_script_tools(9-524)UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (2)
register_manage_asset_tools(12-79)manage_asset(16-79)
🪛 Ruff (0.12.2)
tests/test_edit_normalization_and_noop.py
15-15: Multiple statements on one line (colon)
(E701)
36-36: Unused method argument: args
(ARG002)
36-36: Unused method argument: kwargs
(ARG002)
37-37: Multiple statements on one line (semicolon)
(E702)
51-51: Unused function argument: cmd
(ARG001)
65-65: Use of assert detected
(S101)
71-71: Unused function argument: cmd
(ARG001)
84-84: Unused function argument: cmd
(ARG001)
84-84: Unused function argument: params
(ARG001)
89-89: Use of assert detected
(S101)
90-90: Use of assert detected
(S101)
96-96: Multiple statements on one line (semicolon)
(E702)
99-99: Unused function argument: cmd
(ARG001)
111-111: Use of assert detected
(S101)
114-114: Use of assert detected
(S101)
115-115: Use of assert detected
(S101)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
73-73: Unused function argument: ctx
(ARG001)
109-109: Do not catch blind exception: Exception
(BLE001)
136-137: try-except-pass detected, consider logging the exception
(S110)
136-136: Do not catch blind exception: Exception
(BLE001)
146-147: try-except-pass detected, consider logging the exception
(S110)
146-146: Do not catch blind exception: Exception
(BLE001)
179-180: try-except-pass detected, consider logging the exception
(S110)
179-179: Do not catch blind exception: Exception
(BLE001)
200-201: try-except-pass detected, consider logging the exception
(S110)
200-200: Do not catch blind exception: Exception
(BLE001)
209-210: try-except-pass detected, consider logging the exception
(S110)
209-209: Do not catch blind exception: Exception
(BLE001)
233-235: try-except-pass detected, consider logging the exception
(S110)
233-233: Do not catch blind exception: Exception
(BLE001)
281-281: Unused function argument: ctx
(ARG001)
319-319: Unused function argument: ctx
(ARG001)
335-335: Unused function argument: ctx
(ARG001)
359-359: Unused function argument: ctx
(ARG001)
431-432: try-except-pass detected, consider logging the exception
(S110)
431-431: Do not catch blind exception: Exception
(BLE001)
438-438: Do not catch blind exception: Exception
(BLE001)
479-479: Do not catch blind exception: Exception
(BLE001)
482-482: Use explicit conversion flag
Replace with conversion flag
(RUF010)
489-489: Unused function argument: ctx
(ARG001)
501-507: Consider moving this statement to an else block
(TRY300)
508-508: Do not catch blind exception: Exception
(BLE001)
516-516: Unused function argument: ctx
(ARG001)
523-523: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py
23-25: Avoid specifying long messages outside the exception class
(TRY003)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
59-59: Avoid specifying long messages outside the exception class
(TRY003)
61-61: Function definition does not bind loop variable lines
(B023)
62-62: Ambiguous variable name: l
(E741)
62-62: Function definition does not bind loop variable lines
(B023)
63-63: Ambiguous variable name: l
(E741)
63-63: Function definition does not bind loop variable lines
(B023)
79-79: Avoid specifying long messages outside the exception class
(TRY003)
131-131: Multiple statements on one line (semicolon)
(E702)
131-131: Multiple statements on one line (semicolon)
(E702)
131-131: Multiple statements on one line (semicolon)
(E702)
133-133: Multiple statements on one line (semicolon)
(E702)
133-133: Multiple statements on one line (semicolon)
(E702)
133-133: Multiple statements on one line (semicolon)
(E702)
134-134: Multiple statements on one line (colon)
(E701)
135-135: Multiple statements on one line (colon)
(E701)
136-136: Multiple statements on one line (colon)
(E701)
137-137: Multiple statements on one line (colon)
(E701)
138-138: Multiple statements on one line (colon)
(E701)
139-139: Multiple statements on one line (colon)
(E701)
260-260: Unused function argument: ctx
(ARG001)
335-335: Comment contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF003)
335-335: Comment contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF003)
554-554: Function definition does not bind loop variable m
(B023)
591-591: Do not catch blind exception: Exception
(BLE001)
677-677: Function definition does not bind loop variable m
(B023)
727-727: Do not catch blind exception: Exception
(BLE001)
738-738: Consider [*diff[:800], "... (diff truncated) ..."] instead of concatenation
Replace with [*diff[:800], "... (diff truncated) ..."]
(RUF005)
742-742: Do not catch blind exception: Exception
(BLE001)
747-747: Do not catch blind exception: Exception
(BLE001)
766-766: Consider [*diff[:2000], "... (diff truncated) ..."] instead of concatenation
Replace with [*diff[:2000], "... (diff truncated) ..."]
(RUF005)
tests/test_script_tools.py
43-43: Unused method argument: args
(ARG002)
43-43: Unused method argument: kwargs
(ARG002)
73-73: Use of assert detected
(S101)
74-74: Use of assert detected
(S101)
75-75: Use of assert detected
(S101)
76-76: Use of assert detected
(S101)
83-83: Unused function argument: cmd
(ARG001)
94-94: Use of assert detected
(S101)
95-95: Use of assert detected
(S101)
103-103: Unused function argument: cmd
(ARG001)
111-111: Use of assert detected
(S101)
118-118: Unused function argument: loop
(ARG001)
133-133: Use of assert detected
(S101)
134-134: Use of assert detected
(S101)
135-135: Use of assert detected
(S101)
136-136: Use of assert detected
(S101)
137-137: Use of assert detected
(S101)
🪛 LanguageTool
.claude/prompts/nl-unity-suite-full.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...this once, verbatim, early in the run:** AllowedTools: Write,Bash(printf:*),Bash(...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...ole,mcp__unity__get_sha --- ## Mission 1) Pick target file (prefer): - `unity:/...
(QB_NEW_EN)
[grammar] ~20-~20: There might be a mistake here.
Context: ...write. --- ## 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] ~24-~24: There might be a mistake here.
Context: ...the URI) - Relative (when supported): Assets/... - File paths for the helper script are wor...
(QB_NEW_EN)
[grammar] ~30-~30: There might be a mistake here.
Context: ...it-nl-suite.md(synthesized from JUnit) - Helper script:scripts/nlt-revert.sh` (...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ...(snapshot/restore) --- ## Tool Mapping - Anchors/regex/structured: `mcp__unity_...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...Mapping - Anchors/regex/structured: mcp__unity__script_apply_edits - Allowed ops: anchor_insert, `replace_r...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...overlapping ranges within a single call) - Precise ranges / atomic batch: `mcp__u...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...esh read and sent atomically by default. - Prefer options.applyMode:"atomic" when...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...ns; for single‑span, sequential is fine. - Hash-only: mcp__unity__get_sha — ret...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...c}without file body - **Validation**:mcp__unity__validate_script(level:"standard") - For edits, you may passoptions.validat...
(QB_NEW_EN)
[grammar] ~44-~44: There might be a mistake here.
Context: ...lt): full‑file delimiter balance checks. - relaxed: scoped checks for interior, non‑struct...
(QB_NEW_EN)
[grammar] ~45-~45: Ensure spelling is correct
Context: ... relaxed: scoped checks for interior, non‑structural text edits; do not use for header/signa...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~45-~45: There might be a mistake here.
Context: ...header/signature/brace‑touching changes. - Reporting: Write small XML fragments...
(QB_NEW_EN)
[grammar] ~76-~76: There might be a mistake here.
Context: ...er (T‑A/T‑E) - Prefer structured delete: - Use script_apply_edits with `{ "op":"d...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ...tSeries" }(or__TempHelper` for T‑A). - If structured delete is unavailable, fal...
(QB_NEW_EN)
[grammar] ~80-~80: There might be a mistake here.
Context: ...ex deletes. # T‑B (replace method body) - Use mcp__unity__apply_text_edits with ...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ... strictly inside the HasTarget braces. - Compute start/end from a fresh `read_res...
(QB_NEW_EN)
[grammar] ~82-~82: There might be a mistake here.
Context: ...h read_resource at test start. Do not edit signature or header. - On `{status:"sta...
(QB_NEW_EN)
[grammar] ~82-~82: There might be a mistake here.
Context: ... start. Do not edit signature or header. - On {status:"stale_file"} retry once wi...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ...hash; if absent, re-read once and retry. - On bad_request: write the testcase wit...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ...ure>…, restore, and continue to next test. - On missing_field`: FALL B...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ...e>, restore, and continue to next test. - On missing_field`: FALL BACK per above;...
(QB_NEW_EN)
[style] ~85-~85: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..., restore, and continue to next test. - On missing_field: FALL BACK per above; i...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~88-~88: There might be a mistake here.
Context: ...y. Span formats for apply_text_edits: - Prefer LSP ranges (0‑based): `{ "range":...
(QB_NEW_EN)
[grammar] ~89-~89: There might be a mistake here.
Context: ..._edits: - Prefer LSP ranges (0‑based): { "range": { "start": {"line": L, "character": C}, "end": {…} }, "newText": "…" }- Explicit fields are 1‑based:{ "startLi...
(QB_NEW_EN)
[grammar] ~90-~90: There might be a mistake here.
Context: ..."newText": "…" }- Explicit fields are 1‑based:{ "startLine": L1, "startCol": C1, "endLine": L2, "endCol": C2, "newText": "…" }` - SDK preflights overlap after normalizati...
(QB_NEW_EN)
[grammar] ~91-~91: There might be a mistake here.
Context: ..."}with conflicts and no file mutation. - Optional debug: passstrict:true` to re...
(QB_NEW_EN)
[grammar] ~99-~99: There might be a mistake here.
Context: ...tem-out>. - If content contains ]]>`, split CDATA: ...
(QB_NEW_EN)
[grammar] ~150-~150: There might be a mistake here.
Context: ...fore any further edits in the same test. - Prefer anchors (script_apply_edits) fo...
(QB_NEW_EN)
[grammar] ~153-~153: There might be a mistake here.
Context: ...rrors (timeout, EOF, connection closed):** - Write reports/<TESTID>_results.xml wit...
(QB_NEW_EN)
[grammar] ~154-~154: There might be a mistake here.
Context: ...<error> node capturing the error text. - Run the OS restore via `scripts/nlt-reve...
(QB_NEW_EN)
[grammar] ~159-~159: There might be a mistake here.
Context: ... evidence, and end with VERDICT: FAIL. - Run scripts/nlt-revert.sh restore ... ...
(QB_NEW_EN)
[grammar] ~163-~163: There might be a mistake here.
Context: ...T-E, T-F, T-G, T-H, T-I, T-J (15 total). - Before NL-1..T-J: Bash(scripts/nlt-rever...
(QB_NEW_EN)
[grammar] ~164-~164: There might be a mistake here.
Context: ...) IF the baseline exists; skip for NL-0. - NL-0 must include the PLAN line (len=15)...
(QB_NEW_EN)
[grammar] ~172-~172: There might be a mistake here.
Context: ...verify; delete PrintSeries(); restore. - NL‑2. Anchor comment — Insert `// Build ...
(QB_NEW_EN)
[grammar] ~173-~173: There might be a mistake here.
Context: ...bove public void Update(...); restore. - NL‑3. End‑of‑class — Insert `// Tail tes...
(QB_NEW_EN)
[grammar] ~174-~174: There might be a mistake here.
Context: ...— Insert // Tail test A/B/C (3 lines) before final brace; restore. - NL‑4. Compile t...
(QB_NEW_EN)
[grammar] ~174-~174: There might be a mistake here.
Context: ...` (3 lines) before final brace; restore. - NL‑4. Compile trigger — Record INFO only...
(QB_NEW_EN)
[grammar] ~177-~177: There might be a mistake here.
Context: ...fy; delete via regex_replace; restore. ### T‑B. Replace body — Single `replace_rang...
(QB_NEW_EN)
[grammar] ~178-~178: There might be a mistake here.
Context: ...lace_rangeinsideHasTarget`; restore. ### T‑C. Header/region preservation — Edit i...
(QB_NEW_EN)
[grammar] ~179-~179: There might be a mistake here.
Context: ...reserve signature/docs/regions; restore. ### T‑D. End‑of‑class (anchor) — Insert help...
(QB_NEW_EN)
[grammar] ~180-~180: There might be a mistake here.
Context: ...per before final brace; remove; restore. ### T‑E. Lifecycle — Insert → update → delet...
(QB_NEW_EN)
[grammar] ~181-~181: There might be a mistake here.
Context: ...rt → update → delete via regex; restore. ### T‑F. Atomic batch — One `mcp__unity__app...
(QB_NEW_EN)
[grammar] ~182-~182: There might be a mistake here.
Context: ...pply_text_edits` call (text ranges only) - Compute all three edits from the **same ...
(QB_NEW_EN)
[grammar] ~183-~183: There might be a mistake here.
Context: ...hree edits from the same fresh read: 1) Two small interior replace_range tweak...
(QB_NEW_EN)
[grammar] ~184-~184: There might be a mistake here.
Context: ...o small interior replace_range tweaks. 2) One end‑of‑class insertion: find the...
(QB_NEW_EN)
[grammar] ~185-~185: There might be a mistake here.
Context: ...eplacement` to the 3‑line comment block. - Send all three ranges in one call, s...
(QB_NEW_EN)
[grammar] ~187-~187: There might be a mistake here.
Context: ...…`, restore, and continue. - T‑G. Path normalization — Make the same ...
(QB_NEW_EN)
[grammar] ~192-~192: There might be a mistake here.
Context: ...nterior tweak (or none), then MUST call: mcp__unity__validate_script(level:"stand...
(QB_NEW_EN)
[grammar] ~193-~193: There might be a mistake here.
Context: ...unity__validate_script(level:"standard") - Write the validator output to system-out...
(QB_NEW_EN)
[grammar] ~200-~200: There might be a mistake here.
Context: ... both spans come from the same snapshot. * Single mcp__unity__apply_text_edits call...
(QB_NEW_EN)
[grammar] ~205-~205: There might be a mistake here.
Context: ...a tiny legit edit with pre_sha; success. * Attempt another edit reusing the OLD pre...
(QB_NEW_EN)
[grammar] ~209-~209: There might be a mistake here.
Context: ...### Per‑test error handling and recovery - For each test (NL‑0..T‑J), use a try/fin...
(QB_NEW_EN)
[grammar] ~210-~210: There might be a mistake here.
Context: ... (NL‑0..T‑J), use a try/finally pattern: - try: run the test steps; always write `r...
(QB_NEW_EN)
[grammar] ~213-~213: There might be a mistake here.
Context: ... - On any transport/JSON/tool exception: - catch and write a <testcase> fragment ...
(QB_NEW_EN)
[grammar] ~214-~214: There might be a mistake here.
Context: ...message), then proceed to the next test. - After NL‑4 completes, proceed directly t...
(QB_NEW_EN)
[grammar] ~215-~215: There might be a mistake here.
Context: ...lidator warnings (do not abort the run). - (3) USING_GUARD (optional): * Attempt ...
(QB_NEW_EN)
[grammar] ~216-~216: There might be a mistake here.
Context: ... the run). - (3) USING_GUARD (optional): * Attempt a 1-line insert above the first ...
(QB_NEW_EN)
[grammar] ~222-~222: There might be a mistake here.
Context: ...aseline. - Repeat a replace_range twice (second call may be noop). Validate stand...
(QB_NEW_EN)
[grammar] ~222-~222: There might be a mistake here.
Context: ... a replace_range twice (second call may be noop). Validate standard after each. - ...
(QB_NEW_EN)
[grammar] ~222-~222: There might be a mistake here.
Context: ..._range twice (second call may be noop). Validate standard after each. - Insert or ensure...
(QB_NEW_EN)
[grammar] ~222-~222: There might be a mistake here.
Context: ... be noop). Validate standard after each. - Insert or ensure a tiny comment, then de...
(QB_NEW_EN)
[grammar] ~223-~223: There might be a mistake here.
Context: ...re a tiny comment, then delete it twice (second delete may be noop). - Restore an...
(QB_NEW_EN)
[grammar] ~223-~223: There might be a mistake here.
Context: ...then delete it twice (second delete may be noop). - Restore and PASS unless an err...
(QB_NEW_EN)
[grammar] ~223-~223: There might be a mistake here.
Context: ...te it twice (second delete may be noop). - Restore and PASS unless an error/structu...
(QB_NEW_EN)
[grammar] ~229-~229: There might be a mistake here.
Context: ...uses are non‑fatal; record and continue. - End each testcase <system-out> with `V...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
.claude/prompts/nl-unity-suite-full.md
100-100: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
110-110: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
177-177: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
178-178: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
179-179: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
180-180: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
181-181: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
183-183: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
186-186: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
187-187: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
188-188: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 0
(MD005, list-indent)
199-199: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
200-200: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
201-201: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
202-202: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
204-204: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
205-205: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
206-206: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
207-207: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
217-217: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
218-218: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
⏰ 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: nl-suite
- GitHub Check: Cursor Bugbot
| - **Hash-only**: `mcp__unity__get_sha` — returns `{sha256,lengthBytes,lastModifiedUtc}` without file body | ||
| - **Validation**: `mcp__unity__validate_script(level:"standard")` |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify mcp__unity__get_sha exists and returned shape matches doc
Prevent prompt/tool drift before merge.
🏁 Script executed:
#!/bin/bash
# Validate the tool is implemented and returns the documented fields.
set -euo pipefail
# Find tool declaration/registration and handlers
rg -nP 'mcp__unity__get_sha' -C3
rg -nP '\b(get_sha|getSha|GetSha|GetSHA)\b' -C3 --type cs --type ts --type rs
# Inspect schema: expect fields sha256,lengthBytes,lastModifiedUtc
rg -nP 'sha256|lengthBytes|lastModifiedUtc' -n -C2 --type cs --type ts --type rsLength of output: 977
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the Python implementation of mcp__unity__get_sha
rg -nP 'def\s+mcp__unity__get_sha' -C3 --type py
# Search for any references or aliases of get_sha in Python tools
rg -nP 'unity__get_sha' -C3 --type py
# Check tool registration or declaration calls
rg -nP 'register_tool\(|tool_registry' -C3 --type py
# Inspect returned schema fields in the handler
rg -nP 'sha256|lengthBytes|lastModifiedUtc' -C3 --type pyLength of output: 70
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate manage_script.py
FILE=$(fd --type f manage_script.py | head -n1)
echo "Inspecting $FILE"
# Print relevant section around tool definitions (~lines 240-300)
sed -n '240,300p' "$FILE"
# List all Python functions for mcp__unity__
rg -nP 'def\s+mcp__unity__' -C3 --type py
# Search for schema field mentions in Python
rg -nP 'sha256|lengthBytes|lastModifiedUtc' -C3 --type pyLength of output: 3697
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any reference to mcp__unity__ across the repo
rg -nP 'mcp__unity__' -C3
# Search for all get_sha occurrences in code
rg -nP 'get_sha' -C3
# List Python files related to Unity in tools directory
find UnityMcpBridge/UnityMcpServer~/src/tools -type f -name '*unity*.py'
# List all mcp.tool-decorated functions in Python
rg -nP '@mcp\.tool' -C1 --type pyLength of output: 10520
Update mcp__unity__get_sha return shape in prompt
Replace “returns {sha256,lengthBytes,lastModifiedUtc}” with “returns {sha256,lengthBytes,lastModifiedUtc,uri,path}” in .claude/prompts/nl-unity-suite-full.md lines 41–42.
🧰 Tools
🪛 LanguageTool
[grammar] ~42-~42: There might be a mistake here.
Context: ...c}without file body - **Validation**:mcp__unity__validate_script(level:"standard") - For edits, you may passoptions.validat...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In .claude/prompts/nl-unity-suite-full.md around lines 41 to 42, update the
mcp__unity__get_sha return shape in the prompt: change the text that says
“returns {sha256,lengthBytes,lastModifiedUtc}” to “returns
{sha256,lengthBytes,lastModifiedUtc,uri,path}” so the prompt reflects the new
fields; ensure spacing, punctuation, and backticks remain consistent with
surrounding markdown.
…=atomic for T-F; emphasize always writing testcase and restoring on errors
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
.claude/prompts/nl-unity-suite-full.md (4)
75-79: Remove unsupported opdelete_method; use supported deletes.The toolset exposes anchor_insert/replace_range/regex_replace;
delete_methodis not implemented and will mislead agents.-# Delete the temporary helper (T‑A/T‑E) -- Prefer structured delete: - - Use `script_apply_edits` with `{ "op":"delete_method", "className":"LongUnityScriptClaudeTest", "methodName":"PrintSeries" }` (or `__TempHelper` for T‑A). -- If structured delete is unavailable, fall back to `apply_text_edits` with a single `replace_range` spanning the exact method block (bounds computed from a fresh read); avoid whole‑file regex deletes. +# Delete the temporary helper (T‑A/T‑E) +- Prefer `apply_text_edits` with a single `replace_range` spanning the exact method block (bounds from a fresh read), or `script_apply_edits` with a targeted `regex_replace`. +- Avoid whole‑file regex deletes.
108-108: Fix contradiction: allow Write (reports) + revert as the only FS mutations.Current line forbids all FS changes except revert, but reporting requires Write.
- - Bash(printf|echo) must write to stdout only. Do not use shell redirection, here‑docs, or `tee` to create/modify files. The only allowed FS mutation is via `scripts/nlt-revert.sh`. + - Bash(printf|echo) must write to stdout only. Do not use shell redirection, here‑docs, or `tee`. + - Only these may modify the filesystem: + 1) Write → `reports/*_results.xml` (per path rules above) + 2) `scripts/nlt-revert.sh` → snapshot/restore
140-141: Unexecutable fallback: agents can’t “restore from baseline bytes.”Replace with deterministic error reporting + continue.
-- If the helper fails: fall back once to a guarded full‑file restore using the baseline bytes; then continue. +- If the helper fails: write a testcase with `<error>` (restore failure), then continue to the next test.
184-191: Use LSP zero‑length range; remove index math and unnecessary ordering.Index offsets drift and contradict your “ordering not required” note. Align with “Span formats”.
-### T‑F. Atomic batch — One `mcp__unity__apply_text_edits` call (text ranges only) - - Compute all three edits from the **same fresh read**: - 1) Two small interior `replace_range` tweaks. - 2) One **end‑of‑class insertion**: find the **index of the final `}`** for the class; create a zero‑width range `[idx, idx)` and set `replacement` to the 3‑line comment block. - - Send all three ranges in **one call**, sorted **descending by start index** to avoid offset drift. +### T‑F. Atomic batch — One `mcp__unity__apply_text_edits` call (text ranges only) + - Compute all three edits from the **same fresh read**: + 1) Two small interior `replace_range` tweaks. + 2) One **end‑of‑class insertion**: locate the final `}` and compute its LSP position; create a zero‑length LSP range (`start == end`) there and set `newText` to the 3‑line comment block. + - Send all three non‑overlapping ranges in **one call**; ordering not required when using LSP coordinates.UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
523-531: Add Assets/ guard to get_sha to match other tool functions.Prevents futile round-trips for out-of-project paths.
def get_sha(ctx: Context, uri: str) -> Dict[str, Any]: """Return SHA256 and basic metadata for a script.""" try: name, directory = _split_uri(uri) + if not directory or directory.split("/")[0].lower() != "assets": + return {"success": False, "code": "path_outside_assets", "message": "URI must resolve under 'Assets/'."} params = {"action": "get_sha", "name": name, "path": directory} resp = send_command_with_retry("manage_script", params) return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)}
81-87: Add Assets/ guard to apply_text_edits (consistency and early rejection).Other tools validate project scope; apply_text_edits should too.
name, directory = _split_uri(uri) + # Guard: must resolve under Assets/ + if not directory or directory.split("/")[0].lower() != "assets": + return { + "success": False, + "code": "path_outside_assets", + "message": f"URI must resolve under 'Assets/'; got '{directory or '<root>'}'.", + }
🧹 Nitpick comments (8)
.claude/prompts/nl-unity-suite-full.md (2)
64-68: Prefer LSP coordinates over index wording in the fallback.Improve precision and keep consistency with “Span formats” below.
- - Find the last `}` in the file (class closing brace) by scanning from end. - - Insert the three comment lines immediately before that index with one non‑overlapping range. + - Find the last `}` in the file (class closing brace). + - Compute its LSP position and insert the three comment lines at that zero‑width position with one non‑overlapping range.
185-190: Markdown list indentation/style nits (MD004/MD007).Fix unordered-list indentation and consistent dash style to appease linters.
tests/test_script_tools.py (4)
5-5: Remove unused import to appease Ruff.
pytestisn’t referenced directly; themonkeypatchfixture doesn’t require an explicit import.-import pytest
43-47: Silence ARG002 by underscoring unused decorator args.These parameters aren’t used; underscore them to satisfy linters.
- def tool(self, *args, **kwargs): # accept decorator kwargs like description + def tool(self, *_args, **_kwargs): # accept decorator kwargs like description
83-86: Rename unused parameters in fakes to underscore.Tidies ARG001/ARG0018 without changing behavior.
- def fake_send(cmd, params): + def fake_send(_cmd, params): calls.append(params) return {"success": True, "sha256": f"hash{len(calls)}"}- def fake_send(cmd, params): + def fake_send(_cmd, params): captured["params"] = params return {"success": True}- def fake_send(cmd, params): + def fake_send(_cmd, params): captured["params"] = params return {"success": True}- async def fake_async(cmd, params, loop=None): + async def fake_async(cmd, params, _loop=None): captured["cmd"] = cmd captured["params"] = params return {"success": True}Also applies to: 103-106, 119-122, 138-141
13-28: Avoid global sys.modules pollution in tests.Consider moving the MCP stubs into a fixture that registers/unregisters modules via monkeypatch to keep test isolation.
If helpful, I can provide a fixture-based version.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
52-56: Don’t strip leading “/” from POSIX absolute paths.Let guards decide validity; altering the path can surprise callers and complicate diagnostics.
- effective_path = assets_rel if assets_rel else norm - # For POSIX absolute paths outside Assets, drop the leading '/' - # to return a clean relative-like directory (e.g., '/tmp' -> 'tmp'). - if effective_path.startswith("/"): - effective_path = effective_path[1:] + effective_path = assets_rel if assets_rel else norm
112-121: Consider de-duplicating line_col_from_index via manage_script_edits.Same logic exists in manage_script_edits; centralizing reduces drift.
I can wire a shared helper if desired.
📜 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 (3)
.claude/prompts/nl-unity-suite-full.md(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(2 hunks)tests/test_script_tools.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T21:31:02.427Z
Learnt from: dsarno
PR: dsarno/unity-mcp#56
File: .claude/prompts/nl-unity-suite-full.md:5-6
Timestamp: 2025-08-29T21:31:02.427Z
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:
.claude/prompts/nl-unity-suite-full.md
🧬 Code graph analysis (2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (3)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(388-407)tests/test_manage_script_uri.py (1)
tool(46-50)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
line_col_from_index(508-512)line_col_from_index(621-626)
tests/test_script_tools.py (4)
tests/test_edit_normalization_and_noop.py (3)
_Dummy(15-15)DummyMCP(34-38)tool(36-38)tests/test_manage_script_uri.py (3)
_Dummy(28-28)DummyMCP(42-50)tool(46-50)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
register_manage_script_tools(9-531)UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (2)
register_manage_asset_tools(12-79)manage_asset(16-79)
🪛 LanguageTool
.claude/prompts/nl-unity-suite-full.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...this once, verbatim, early in the run:** AllowedTools: Write,Bash(printf:*),Bash(...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...ole,mcp__unity__get_sha --- ## Mission 1) Pick target file (prefer): - `unity:/...
(QB_NEW_EN)
[grammar] ~20-~20: There might be a mistake here.
Context: ...write. --- ## 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] ~24-~24: There might be a mistake here.
Context: ...the URI) - Relative (when supported): Assets/... - File paths for the helper script are wor...
(QB_NEW_EN)
[grammar] ~30-~30: There might be a mistake here.
Context: ...it-nl-suite.md(synthesized from JUnit) - Helper script:scripts/nlt-revert.sh` (...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ...(snapshot/restore) --- ## Tool Mapping - Anchors/regex/structured: `mcp__unity_...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...Mapping - Anchors/regex/structured: mcp__unity__script_apply_edits - Allowed ops: anchor_insert, `replace_r...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...overlapping ranges within a single call) - Precise ranges / atomic batch: `mcp__u...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...esh read and sent atomically by default. - Prefer options.applyMode:"atomic" when...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...ns; for single‑span, sequential is fine. - Hash-only: mcp__unity__get_sha — ret...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...c}without file body - **Validation**:mcp__unity__validate_script(level:"standard") - For edits, you may passoptions.validat...
(QB_NEW_EN)
[grammar] ~44-~44: There might be a mistake here.
Context: ...lt): full‑file delimiter balance checks. - relaxed: scoped checks for interior, non‑struct...
(QB_NEW_EN)
[grammar] ~45-~45: Ensure spelling is correct
Context: ... relaxed: scoped checks for interior, non‑structural text edits; do not use for header/signa...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~45-~45: There might be a mistake here.
Context: ...header/signature/brace‑touching changes. - Reporting: Write small XML fragments...
(QB_NEW_EN)
[grammar] ~76-~76: There might be a mistake here.
Context: ...er (T‑A/T‑E) - Prefer structured delete: - Use script_apply_edits with `{ "op":"d...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ...tSeries" }(or__TempHelper` for T‑A). - If structured delete is unavailable, fal...
(QB_NEW_EN)
[grammar] ~80-~80: There might be a mistake here.
Context: ...ex deletes. # T‑B (replace method body) - Use mcp__unity__apply_text_edits with ...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ... strictly inside the HasTarget braces. - Compute start/end from a fresh `read_res...
(QB_NEW_EN)
[grammar] ~82-~82: There might be a mistake here.
Context: ...h read_resource at test start. Do not edit signature or header. - On `{status:"sta...
(QB_NEW_EN)
[grammar] ~82-~82: There might be a mistake here.
Context: ... start. Do not edit signature or header. - On {status:"stale_file"} retry once wi...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ...hash; if absent, re-read once and retry. - On bad_request: write the testcase wit...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ...ure>…, restore, and continue to next test. - On missing_field`: FALL B...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ...e>, restore, and continue to next test. - On missing_field`: FALL BACK per above;...
(QB_NEW_EN)
[style] ~85-~85: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..., restore, and continue to next test. - On missing_field: FALL BACK per above; i...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~88-~88: There might be a mistake here.
Context: ...y. Span formats for apply_text_edits: - Prefer LSP ranges (0‑based): `{ "range":...
(QB_NEW_EN)
[grammar] ~89-~89: There might be a mistake here.
Context: ..._edits: - Prefer LSP ranges (0‑based): { "range": { "start": {"line": L, "character": C}, "end": {…} }, "newText": "…" }- Explicit fields are 1‑based:{ "startLi...
(QB_NEW_EN)
[grammar] ~90-~90: There might be a mistake here.
Context: ..."newText": "…" }- Explicit fields are 1‑based:{ "startLine": L1, "startCol": C1, "endLine": L2, "endCol": C2, "newText": "…" }` - SDK preflights overlap after normalizati...
(QB_NEW_EN)
[grammar] ~91-~91: There might be a mistake here.
Context: ..."}with conflicts and no file mutation. - Optional debug: passstrict:true` to re...
(QB_NEW_EN)
[grammar] ~99-~99: There might be a mistake here.
Context: ...tem-out>. - If content contains ]]>`, split CDATA: ...
(QB_NEW_EN)
[grammar] ~150-~150: There might be a mistake here.
Context: ...fore any further edits in the same test. - Prefer anchors (script_apply_edits) fo...
(QB_NEW_EN)
[grammar] ~153-~153: There might be a mistake here.
Context: ...rrors (timeout, EOF, connection closed):** - Write reports/<TESTID>_results.xml wit...
(QB_NEW_EN)
[grammar] ~154-~154: There might be a mistake here.
Context: ...<error> node capturing the error text. - Run the OS restore via `scripts/nlt-reve...
(QB_NEW_EN)
[grammar] ~159-~159: There might be a mistake here.
Context: ... evidence, and end with VERDICT: FAIL. - Run scripts/nlt-revert.sh restore ... ...
(QB_NEW_EN)
[grammar] ~163-~163: There might be a mistake here.
Context: ...T-E, T-F, T-G, T-H, T-I, T-J (15 total). - Before NL-1..T-J: Bash(scripts/nlt-rever...
(QB_NEW_EN)
[grammar] ~164-~164: There might be a mistake here.
Context: ...) IF the baseline exists; skip for NL-0. - NL-0 must include the PLAN line (len=15)...
(QB_NEW_EN)
[grammar] ~172-~172: There might be a mistake here.
Context: ...verify; delete PrintSeries(); restore. - NL‑2. Anchor comment — Insert `// Build ...
(QB_NEW_EN)
[grammar] ~173-~173: There might be a mistake here.
Context: ...bove public void Update(...); restore. - NL‑3. End‑of‑class — Insert `// Tail tes...
(QB_NEW_EN)
[grammar] ~174-~174: There might be a mistake here.
Context: ...— Insert // Tail test A/B/C (3 lines) before final brace; restore. - NL‑4. Compile t...
(QB_NEW_EN)
[grammar] ~174-~174: There might be a mistake here.
Context: ...` (3 lines) before final brace; restore. - NL‑4. Compile trigger — Record INFO only...
(QB_NEW_EN)
[grammar] ~177-~177: There might be a mistake here.
Context: ...fy; delete via regex_replace; restore. ### T‑B. Replace body — Single `replace_rang...
(QB_NEW_EN)
[grammar] ~178-~178: There might be a mistake here.
Context: ...lace_rangeinsideHasTarget`; restore. - Options: pass {"validate":"relaxed"} for...
(QB_NEW_EN)
[grammar] ~179-~179: There might be a mistake here.
Context: ...:"relaxed"} for interior one-line edits. ### T‑C. Header/region preservation — Edit i...
(QB_NEW_EN)
[grammar] ~180-~180: There might be a mistake here.
Context: ...reserve signature/docs/regions; restore. - Options: pass {"validate":"relaxed"} for...
(QB_NEW_EN)
[grammar] ~181-~181: There might be a mistake here.
Context: ...:"relaxed"} for interior one-line edits. ### T‑D. End‑of‑class (anchor) — Insert help...
(QB_NEW_EN)
[grammar] ~182-~182: There might be a mistake here.
Context: ...per before final brace; remove; restore. ### T‑E. Lifecycle — Insert → update → delet...
(QB_NEW_EN)
[grammar] ~183-~183: There might be a mistake here.
Context: ...rt → update → delete via regex; restore. ### T‑F. Atomic batch — One `mcp__unity__app...
(QB_NEW_EN)
[grammar] ~184-~184: There might be a mistake here.
Context: ...pply_text_edits` call (text ranges only) - Compute all three edits from the **same ...
(QB_NEW_EN)
[grammar] ~185-~185: There might be a mistake here.
Context: ...hree edits from the same fresh read: 1) Two small interior replace_range tweak...
(QB_NEW_EN)
[grammar] ~186-~186: There might be a mistake here.
Context: ...o small interior replace_range tweaks. 2) One end‑of‑class insertion: find the...
(QB_NEW_EN)
[grammar] ~187-~187: There might be a mistake here.
Context: ...eplacement` to the 3‑line comment block. - Send all three ranges in one call, s...
(QB_NEW_EN)
[grammar] ~189-~189: There might be a mistake here.
Context: ...…`, restore, and continue. - Options: pass {"applyMode":"atomic"} to ...
(QB_NEW_EN)
[grammar] ~190-~190: There might be a mistake here.
Context: ...de":"atomic"} to enforce all‑or‑nothing. - T‑G. Path normalization — Make the same ...
(QB_NEW_EN)
[grammar] ~195-~195: There might be a mistake here.
Context: ...nterior tweak (or none), then MUST call: mcp__unity__validate_script(level:"stand...
(QB_NEW_EN)
[grammar] ~196-~196: There might be a mistake here.
Context: ...unity__validate_script(level:"standard") - Write the validator output to system-out...
(QB_NEW_EN)
[grammar] ~203-~203: There might be a mistake here.
Context: ... both spans come from the same snapshot. * Single mcp__unity__apply_text_edits call...
(QB_NEW_EN)
[grammar] ~208-~208: There might be a mistake here.
Context: ...a tiny legit edit with pre_sha; success. * Attempt another edit reusing the OLD pre...
(QB_NEW_EN)
[grammar] ~212-~212: There might be a mistake here.
Context: ...### Per‑test error handling and recovery - For each test (NL‑0..T‑J), use a try/fin...
(QB_NEW_EN)
[grammar] ~213-~213: There might be a mistake here.
Context: ... (NL‑0..T‑J), use a try/finally pattern: - Always write a testcase fragment and per...
(QB_NEW_EN)
[grammar] ~214-~214: There might be a mistake here.
Context: ..., even when tools return error payloads. - try: run the test steps; always write `r...
(QB_NEW_EN)
[grammar] ~218-~218: There might be a mistake here.
Context: ...message), then proceed to the next test. - After NL‑4 completes, proceed directly t...
(QB_NEW_EN)
[grammar] ~219-~219: There might be a mistake here.
Context: ...lidator warnings (do not abort the run). - (3) USING_GUARD (optional): * Attempt ...
(QB_NEW_EN)
[grammar] ~220-~220: There might be a mistake here.
Context: ... the run). - (3) USING_GUARD (optional): * Attempt a 1-line insert above the first ...
(QB_NEW_EN)
[grammar] ~226-~226: There might be a mistake here.
Context: ...aseline. - Repeat a replace_range twice (second call may be noop). Validate stand...
(QB_NEW_EN)
[grammar] ~226-~226: There might be a mistake here.
Context: ... a replace_range twice (second call may be noop). Validate standard after each. - ...
(QB_NEW_EN)
[grammar] ~226-~226: There might be a mistake here.
Context: ..._range twice (second call may be noop). Validate standard after each. - Insert or ensure...
(QB_NEW_EN)
[grammar] ~226-~226: There might be a mistake here.
Context: ... be noop). Validate standard after each. - Insert or ensure a tiny comment, then de...
(QB_NEW_EN)
[grammar] ~227-~227: There might be a mistake here.
Context: ...re a tiny comment, then delete it twice (second delete may be noop). - Restore an...
(QB_NEW_EN)
[grammar] ~227-~227: There might be a mistake here.
Context: ...then delete it twice (second delete may be noop). - Restore and PASS unless an err...
(QB_NEW_EN)
[grammar] ~227-~227: There might be a mistake here.
Context: ...te it twice (second delete may be noop). - Restore and PASS unless an error/structu...
(QB_NEW_EN)
[grammar] ~233-~233: There might be a mistake here.
Context: ...uses are non‑fatal; record and continue. - End each testcase <system-out> with `V...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
.claude/prompts/nl-unity-suite-full.md
100-100: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
110-110: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
177-177: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
178-178: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
180-180: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
182-182: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
183-183: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
185-185: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
188-188: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
189-189: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
190-190: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
191-191: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 0
(MD005, list-indent)
202-202: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
203-203: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
204-204: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
205-205: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
207-207: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
208-208: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
209-209: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
210-210: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
221-221: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
222-222: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
73-73: Unused function argument: ctx
(ARG001)
109-109: Do not catch blind exception: Exception
(BLE001)
136-137: try-except-pass detected, consider logging the exception
(S110)
136-136: Do not catch blind exception: Exception
(BLE001)
146-147: try-except-pass detected, consider logging the exception
(S110)
146-146: Do not catch blind exception: Exception
(BLE001)
179-180: try-except-pass detected, consider logging the exception
(S110)
179-179: Do not catch blind exception: Exception
(BLE001)
200-201: try-except-pass detected, consider logging the exception
(S110)
200-200: Do not catch blind exception: Exception
(BLE001)
209-210: try-except-pass detected, consider logging the exception
(S110)
209-209: Do not catch blind exception: Exception
(BLE001)
233-235: try-except-pass detected, consider logging the exception
(S110)
233-233: Do not catch blind exception: Exception
(BLE001)
261-262: try-except-pass detected, consider logging the exception
(S110)
261-261: Do not catch blind exception: Exception
(BLE001)
288-288: Unused function argument: ctx
(ARG001)
326-326: Unused function argument: ctx
(ARG001)
342-342: Unused function argument: ctx
(ARG001)
366-366: Unused function argument: ctx
(ARG001)
438-439: try-except-pass detected, consider logging the exception
(S110)
438-438: Do not catch blind exception: Exception
(BLE001)
445-445: Do not catch blind exception: Exception
(BLE001)
486-486: Do not catch blind exception: Exception
(BLE001)
489-489: Use explicit conversion flag
Replace with conversion flag
(RUF010)
496-496: Unused function argument: ctx
(ARG001)
508-514: Consider moving this statement to an else block
(TRY300)
515-515: Do not catch blind exception: Exception
(BLE001)
523-523: Unused function argument: ctx
(ARG001)
530-530: Do not catch blind exception: Exception
(BLE001)
tests/test_script_tools.py
43-43: Unused method argument: args
(ARG002)
43-43: Unused method argument: kwargs
(ARG002)
73-73: Use of assert detected
(S101)
74-74: Use of assert detected
(S101)
75-75: Use of assert detected
(S101)
76-76: Use of assert detected
(S101)
83-83: Unused function argument: cmd
(ARG001)
94-94: Use of assert detected
(S101)
95-95: Use of assert detected
(S101)
103-103: Unused function argument: cmd
(ARG001)
111-111: Use of assert detected
(S101)
119-119: Unused function argument: cmd
(ARG001)
131-131: Use of assert detected
(S101)
138-138: Unused function argument: loop
(ARG001)
153-153: Use of assert detected
(S101)
154-154: Use of assert detected
(S101)
155-155: Use of assert detected
(S101)
156-156: Use of assert detected
(S101)
157-157: Use of assert detected
(S101)
⏰ 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 (6)
.claude/prompts/nl-unity-suite-full.md (4)
6-6: AllowedTools line matches workflow allowances.Retain Bash(printf|echo) in the list; stdout-only constraints are enforced later. Noting we used your retrieved learning to confirm this alignment.
41-42: Update get_sha return shape (schema drift check).Docs list only three fields; recent handlers also return uri/path. Please verify and reflect the full shape.
Apply this diff if accurate:
-**Hash-only**: `mcp__unity__get_sha` — returns `{sha256,lengthBytes,lastModifiedUtc}` without file body +**Hash-only**: `mcp__unity__get_sha` — returns `{sha256,lengthBytes,lastModifiedUtc,uri,path}` without file body
58-63: Brace-anchor regex looks correct and CRLF-safe.Solid choice: multiline + optional CR handles Windows line endings; captures and reinserts the closing brace.
103-107: Path rules are well‑hardened.Good defense-in-depth: regex whitelist, snapshots dir, traversal/controls/length caps.
tests/test_script_tools.py (1)
39-58: Good lightweight MCP stubbing and tool registration harness.This keeps tests hermetic and fast; nice.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
469-485: Response handling looks solid.Decoding encodedContents and normalizing the data shape is clean and caller-friendly.
| for e in edits or []: | ||
| e2 = dict(e) | ||
| has_all = all(k in e2 for k in ("startLine","startCol","endLine","endCol")) | ||
| if has_all: | ||
| zero_based = False | ||
| for k in ("startLine","startCol","endLine","endCol"): | ||
| try: | ||
| if int(e2.get(k, 1)) < 1: | ||
| zero_based = True | ||
| except Exception: | ||
| pass | ||
| if zero_based: | ||
| if strict: | ||
| return {"success": False, "code": "zero_based_explicit_fields", "message": "Explicit line/col fields are 1-based; received zero-based.", "data": {"normalizedEdits": [e2]}} | ||
| for k in ("startLine","startCol","endLine","endCol"): | ||
| try: | ||
| if int(e2.get(k, 1)) < 1: | ||
| e2[k] = 1 | ||
| except Exception: | ||
| pass | ||
| if "zero_based_explicit_fields_normalized" not in warnings: | ||
| warnings.append("zero_based_explicit_fields_normalized") | ||
| normalized_edits.append(e2) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure newText is present for explicit-form edits.
If callers omit newText (or use text), we currently pass an invalid edit downstream.
for e in edits or []:
e2 = dict(e)
+ # Require newText; map from 'text' if provided
+ if "newText" not in e2:
+ if "text" in e2:
+ e2["newText"] = e2.pop("text")
+ else:
+ return {
+ "success": False,
+ "code": "missing_field",
+ "message": "apply_text_edits requires 'newText' for explicit edits.",
+ "data": {"got": e2},
+ }📝 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.
| for e in edits or []: | |
| e2 = dict(e) | |
| has_all = all(k in e2 for k in ("startLine","startCol","endLine","endCol")) | |
| if has_all: | |
| zero_based = False | |
| for k in ("startLine","startCol","endLine","endCol"): | |
| try: | |
| if int(e2.get(k, 1)) < 1: | |
| zero_based = True | |
| except Exception: | |
| pass | |
| if zero_based: | |
| if strict: | |
| return {"success": False, "code": "zero_based_explicit_fields", "message": "Explicit line/col fields are 1-based; received zero-based.", "data": {"normalizedEdits": [e2]}} | |
| for k in ("startLine","startCol","endLine","endCol"): | |
| try: | |
| if int(e2.get(k, 1)) < 1: | |
| e2[k] = 1 | |
| except Exception: | |
| pass | |
| if "zero_based_explicit_fields_normalized" not in warnings: | |
| warnings.append("zero_based_explicit_fields_normalized") | |
| normalized_edits.append(e2) | |
| for e in edits or []: | |
| e2 = dict(e) | |
| # Require newText; map from 'text' if provided | |
| if "newText" not in e2: | |
| if "text" in e2: | |
| e2["newText"] = e2.pop("text") | |
| else: | |
| return { | |
| "success": False, | |
| "code": "missing_field", | |
| "message": "apply_text_edits requires 'newText' for explicit edits.", | |
| "data": {"got": e2}, | |
| } | |
| has_all = all(k in e2 for k in ("startLine","startCol","endLine","endCol")) | |
| if has_all: | |
| zero_based = False | |
| for k in ("startLine","startCol","endLine","endCol"): | |
| try: | |
| if int(e2.get(k, 1)) < 1: | |
| zero_based = True | |
| except Exception: | |
| pass | |
| if zero_based: | |
| if strict: | |
| return {"success": False, "code": "zero_based_explicit_fields", "message": "Explicit line/col fields are 1-based; received zero-based.", "data": {"normalizedEdits": [e2]}} | |
| for k in ("startLine","startCol","endLine","endCol"): | |
| try: | |
| if int(e2.get(k, 1)) < 1: | |
| e2[k] = 1 | |
| except Exception: | |
| pass | |
| if "zero_based_explicit_fields_normalized" not in warnings: | |
| warnings.append("zero_based_explicit_fields_normalized") | |
| normalized_edits.append(e2) |
🧰 Tools
🪛 Ruff (0.12.2)
200-201: try-except-pass detected, consider logging the exception
(S110)
200-200: Do not catch blind exception: Exception
(BLE001)
209-210: try-except-pass detected, consider logging the exception
(S110)
209-209: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py around lines 191 to
214, the loop normalizes explicit-form edits but doesn't ensure each edit has a
newText field, allowing invalid edits to be passed downstream; update the
normalization to validate and normalize newText: if "newText" is missing but
"text" exists, copy text -> newText; if neither exists then, when strict is true
return an error like {"success": False, "code":
"missing_newText_explicit_fields", "message": "Explicit edits must include
newText.", "data": {"normalizedEdits": [e2]}}; if strict is false set
e2["newText"] = "" (or other safe default) and add a one-time warning tag such
as "missing_newText_normalized"; perform this check/normalization before
appending e2 to normalized_edits and preserve the existing zero-based handling
and warnings logic.
…indows; early regex compile with hints; debug_preview for apply_text_edits
| if heartbeat_count >= getattr(config, 'max_heartbeat_frames', 16) or time.monotonic() > deadline: | ||
| # Treat as empty successful response to match C# server behavior | ||
| logger.debug("Heartbeat threshold reached; returning empty response") | ||
| return b"" |
There was a problem hiding this comment.
Bug: Zero-Length Payload Handling Mismatch
There's an inconsistency in how zero-length framed payloads are handled. The C# code immediately returns an empty string for these frames, but the Python code treats them as heartbeats and continues waiting for more data. This divergence can cause clients to hang or time out when expecting immediate empty responses.
TL;DR
This PR hardens the transport with explicit framing + timeouts/retries, upgrades the editing surface for Claude (micro‑edits and resource read/search), and adds an end‑to‑end natural‑language (NL) test harness that drives a live Unity instance from CI and reports JUnit/Markdown artifacts suitable for Actions. (+6,962/−741 over 47 files.) ([GitHub]1)
Why this matters
Highlights (compared to
main)1) Robust framing of messages (with testing)
FRAMING=1handshake; non‑blocking script writes; TCPNoDelay; timeouts and retries on reads/writes. Caps frames at 64 MiB and rejects zero‑length frames. ([GitHub]1)test_unity_socket_framing.py. ([GitHub]1)2) Claude‑friendly editing tools + resource reading
apply_text_edits(with precondition hashes and atomic multi‑edit batches) andscript_apply_editsfor structured C# edits;validate_scriptto catch syntax issues before writes. ([GitHub]2)list_resources,read_resource, andfind_in_fileenable read‑first plans and targeted diffs. ([GitHub]3)manage_scriptpaths stay for compatibility while read/update/edit flavors are de‑emphasized. ([GitHub]1)3) New end‑to‑end NL testing CI (client prompts → Unity → back)
…manage_editor, …list_resources, …read_resource, …apply_text_edits, …script_apply_edits, …validate_script, …find_in_file+ minimal Bash). ([GitHub]3)FRAMING=1preamble before tests proceed. ([GitHub]1)reports/junit-for-actions.xml) and a Markdown summary; usesmikepenz/action-junit-report@v5. ([GitHub]3)What changed (selected)
Transport & bridge
ReadExactwith timeouts/retries; 64 MiB frame cap; rejects zero‑length frames; non‑blocking script writes;NoDelayon sockets. ([GitHub]1)Server & tools
apply_text_edits/script_apply_edits; addlist_resources/read_resource; improve method‑span parsing and validation flows to reduce false negatives. ([GitHub]1)Tests
CI
.github/workflows/claude-nl-suite.ymlswitched to a mini NL prompt; honorsJUNIT_OUT/MD_OUT; normalizes toreports/junit-for-actions.xml; publishes via action‑junit‑report v5; adds summary to the job page. ([GitHub]3)How to verify
A) In CI (recommended)
Run “Claude NL Test Suite (mini)” on this branch. (Requires an
ANTHROPIC_API_KEYin your GH secrets)Confirm the steps:
FRAMING=1and the suite proceeds. ([GitHub]1)reports/junit-for-actions.xmlis published and annotated by the JUnit action. ([GitHub]3)B) Locally (quick check)
test_unity_socket_framing.pyassertions for boundary and preamble checks. ([GitHub]2)apply_text_editsorscript_apply_editson a small C# file, then callvalidate_scriptandread_resourceto confirm read‑write‑re‑read flows. ([GitHub]2)Backwards compatibility
manage_scriptremains available; the NL suite and docs steer tooling toward micro‑edits and resource‑first reads. No public API breaks are expected for existing automations. ([GitHub]1)Risks
FRAMING=1and proper lengths. The NL suite explicitly verifies this path. ([GitHub]1)Follow‑ups
Change map (at a glance)
Summary by CodeRabbit
New Features
CI / Workflows
Documentation
Tests
Chores