Added optional telemetry #264
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a privacy-first telemetry system across the Python server and Unity Editor bridge, instruments many MCP tools with a telemetry decorator, adds docs and tests, and improves bridge robustness (timeouts, I/O diagnostics). Changes are additive and include opt-out mechanisms and persistent identifiers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UnityEditor as Unity Editor
participant TelemetryHelper
participant PyBridge as Local Python Bridge
participant TelemetrySvc as telemetry.py
participant Endpoint as Telemetry Endpoint
User->>UnityEditor: Start Auto-Connect
UnityEditor->>TelemetryHelper: RecordBridgeStartup()
TelemetryHelper-->>PyBridge: Send telemetry (non-blocking)
PyBridge->>TelemetrySvc: record_telemetry(STARTUP)
TelemetrySvc->>TelemetrySvc: persist UUID/milestones, enqueue record
TelemetrySvc-->>Endpoint: HTTPS POST /telemetry/events (async, short timeout)
UnityEditor->>PyBridge: Bridge connection attempt
alt Connected
PyBridge->>TelemetrySvc: record_telemetry(UNITY_CONNECTION: connected)
else Failed
PyBridge->>TelemetrySvc: record_telemetry(UNITY_CONNECTION: failed, error)
end
sequenceDiagram
autonumber
participant Client as MCP Client
participant Decorator as telemetry_decorator
participant Tool as MCP Tool
participant TelemetrySvc as telemetry.py
participant Endpoint as Telemetry Endpoint
Client->>Decorator: Invoke decorated tool(args)
Decorator->>Tool: Call wrapped function
alt Success
Decorator->>TelemetrySvc: record_milestone(FIRST_TOOL_USAGE / specific)
Decorator->>TelemetrySvc: record_tool_usage(tool, success=true, duration)
else Exception
Decorator->>TelemetrySvc: record_tool_usage(tool, success=false, error)
Decorator-->>Client: propagate exception
end
TelemetrySvc-->>Endpoint: Async HTTPS POST
Decorator-->>Client: return result or re-raise
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (29)
README.md (1)
341-343: Fix markdown list style to satisfy lint (MD004).Use asterisks instead of dashes for unordered list items for consistency with the rest of the doc.
- - **🔒 Anonymous**: Random UUIDs only, no personal data - - **🚫 Easy opt-out**: Set `DISABLE_TELEMETRY=true` environment variable - - **📖 Transparent**: See [TELEMETRY.md](TELEMETRY.md) for full details + * **🔒 Anonymous**: Random UUIDs only, no personal data + * **🚫 Easy opt-out**: Set `DISABLE_TELEMETRY=true` environment variable + * **📖 Transparent**: See [TELEMETRY.md](TELEMETRY.md) for full detailsUnityMcpBridge/Editor/Helpers/TelemetryHelper.cs.meta (1)
1-11: Meta file looks fine; add trailing newline.Unity prefers newline at EOF; minor hygiene.
UnityMcpBridge/UnityMcpServer~/src/config.py (1)
37-39: Consider environment overrides for enable/endpoint.Let admins disable telemetry or change the endpoint without code changes.
Add below (outside the shown lines), after creating
config:# At top: import os def _env_true(key: str) -> bool: v = os.getenv(key) return bool(v) and v.strip().lower() in ("1", "true", "yes", "on") # After: config = ServerConfig() if _env_true("DISABLE_TELEMETRY") or _env_true("UNITY_MCP_DISABLE_TELEMETRY") or _env_true("MCP_DISABLE_TELEMETRY"): config.telemetry_enabled = False endpoint = os.getenv("UNITY_MCP_TELEMETRY_ENDPOINT") if endpoint: config.telemetry_endpoint = endpointTELEMETRY.md (2)
80-85: Resolve “retry” vs “no retries” contradiction; tighten wording.Currently it says background retry and also “no retries.” Clarify as best-effort, no retries, with a 10‑second timeout.
- - **Retry**: Background thread with graceful failure - - **Timeout**: 10 second timeout, no retries on failure + - **Delivery**: Background thread (best-effort), no retries on failure + - **Timeout**: 10-second timeout
70-79: Clarify which side writes local files vs EditorPrefs.Avoid confusion about where the UUID lives.
Files created: - `customer_uuid.txt`: Anonymous identifier - `milestones.json`: One-time events tracker + + Note: In the Unity Editor, the anonymous identifier is stored in EditorPrefs + (`MCPForUnity.CustomerUUID`) rather than a file. The files above are created by + the Python server component.UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs (5)
13-15: Avoid magic number (200) for error truncation.Introduce a named constant and reuse it.
- private const string TELEMETRY_DISABLED_KEY = "MCPForUnity.TelemetryDisabled"; - private const string CUSTOMER_UUID_KEY = "MCPForUnity.CustomerUUID"; + private const string TELEMETRY_DISABLED_KEY = "MCPForUnity.TelemetryDisabled"; + private const string CUSTOMER_UUID_KEY = "MCPForUnity.CustomerUUID"; + private const int MAX_ERROR_LEN = 200;
135-139: Use the named constant for truncation.- data["error"] = error.Substring(0, Math.Min(200, error.Length)); + data["error"] = error.Substring(0, Math.Min(MAX_ERROR_LEN, error.Length));
155-158: Use the named constant for truncation (duplicate site).- data["error"] = error.Substring(0, Math.Min(200, error.Length)); + data["error"] = error.Substring(0, Math.Min(MAX_ERROR_LEN, error.Length));
118-122: Avoid hardcoding bridge version.Source it from assembly/package metadata to prevent drift. Example (outside this hunk):
var bridgeVersion = typeof(MCPForUnityBridge).Assembly.GetName().Version?.ToString() ?? "unknown";
176-186: Deduplicate debug flag logic across classes.
IsDebugEnabled()here and inMCPForUnityBridgeare identical; centralize to one helper.UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
80-82: Record success path too (symmetry and funnel completion).You record failures; consider also emitting a success event when StartAutoConnect completes without exception so success/failure rates are measurable.
Example:
Start(); isAutoConnectMode = true; // Record telemetry for bridge startup TelemetryHelper.RecordBridgeStartup(); + TelemetryHelper.RecordBridgeConnection(true);If “connection” is intended to mean Unity↔Python handshake, move the success emit to the actual handshake point instead.
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (6)
119-136: Narrow exception handling in _load_persistent_data (ruff BLE001).Catching Exception broadly hides real issues and trips BLE001. Handle expected I/O/JSON errors explicitly.
- except Exception as e: + except (OSError, PermissionError, json.JSONDecodeError) as e: logger.warning(f"Failed to load telemetry data: {e}") self._customer_uuid = str(uuid.uuid4()) self._milestones = {}
137-143: Narrow exception handling in _save_milestones (ruff BLE001).Only catch file system errors.
- except Exception as e: + except (OSError, PermissionError) as e: logger.warning(f"Failed to save milestones: {e}")
199-229: Avoid blind catch in _send_telemetry; catch transport/OS errors.This keeps non-blocking behavior while satisfying BLE001. Also keep debug logging.
- except Exception as e: + except (httpx.HTTPError, OSError, ValueError) as e: # Never let telemetry errors interfere with app functionality logger.debug(f"Telemetry send failed: {e}")
208-210: Prefer platform.system() for clearer OS signal.os.name collapses macOS/Linux to "posix". Use platform.system() to distinguish ("Windows", "Linux", "Darwin").
+import platform ... - "platform": os.name + "platform": platform.system()
182-198: Thread-per-event may scale poorly; consider a single worker with a queue.Burst telemetry will create many threads. A background worker consuming a Queue preserves non-blocking semantics with bounded concurrency.
If interested, I can provide a minimal refactor with a daemon worker and queue.
208-209: Avoid hardcoding version in multiple places."3.0.2" appears here and in server.py. Centralize (e.g., version.py or a CONFIG constant) to prevent drift.
I can draft a small version.py and wire both call sites to it.
UnityMcpBridge/UnityMcpServer~/src/server.py (2)
28-37: Use clear semantics for timestamps vs durations; keep version centralized.
- If “startup_time” is a timestamp, name it accordingly and use time.time(); use a separate monotonic clock for durations.
- Consider centralizing "3.0.2".
- start_time = time.time() - record_telemetry(RecordType.STARTUP, { - "server_version": "3.0.2", - "startup_time": start_time - }) + started_at = time.time() + start_clk = time.perf_counter() + record_telemetry(RecordType.STARTUP, { + "server_version": "3.0.2", # TODO: import from a shared version constant + "started_at": started_at + })
42-47: Compute connection_time_ms using a monotonic clock.time.perf_counter() is robust against wall-clock changes.
- record_telemetry(RecordType.UNITY_CONNECTION, { + record_telemetry(RecordType.UNITY_CONNECTION, { "status": "connected", - "connection_time_ms": (time.time() - start_time) * 1000 + "connection_time_ms": (time.perf_counter() - start_clk) * 1000 })UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py (4)
1-1: Shebang vs. execution bitEither make the file executable in git (chmod +x) or drop the shebang and run with
python. Keeping both mismatched triggers linters.
55-57: Avoid using private attribute_customer_uuidAccessing private state is brittle. Don’t print identifiers; just confirm initialization.
Apply this diff:
- collector = get_telemetry() - print(f"✅ Telemetry collector initialized (UUID: {collector._customer_uuid[:8]}...)") + collector = get_telemetry() + print("✅ Telemetry collector initialized")
109-114: Replace ambiguous glyphs with ASCII“ℹ️” can trip linters and some terminals. Prefer ASCII for test output.
Apply this diff:
- print("ℹ️ UUID file will be created on first use") + print("Info: UUID file will be created on first use") @@ - print("ℹ️ Milestones file will be created on first milestone") + print("Info: Milestones file will be created on first milestone")
39-41: Overly broad exception handlingCatching Exception everywhere hides actionable failures and triggers BLE001. Narrow where feasible (e.g., Base64Error, OSError, ImportError), or at least log exception types. For a lightweight harness this is optional.
Also applies to: 49-51, 57-59, 118-120, 144-146
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (4)
8-17: Tighten telemetry import shim (remove unused and hush linter)
- Drop unused
record_milestone, MilestoneTypeimport here; the decorator module handles those.- Rename unused
tool_namein the fallback to_tool_name.HAS_TELEMETRYisn’t used; remove or prefix with underscore.Apply this diff:
try: - from telemetry_decorator import telemetry_tool - from telemetry import record_milestone, MilestoneType - HAS_TELEMETRY = True + from telemetry_decorator import telemetry_tool except ImportError: - HAS_TELEMETRY = False - def telemetry_tool(tool_name: str): + def telemetry_tool(_tool_name: str): def decorator(func): return func return decorator
118-133: Base64 decode keying: keep key names consistentYou gate on
contentsEncodedthen readencodedContents, which matches other sites below—good. If the server ever returnscontentsonly, your fallback setscontents = contents or ""in the except. Consider an explicit else branch for clarity.
286-304: Unused imports/vars in debug preview
difflib,lines, andprevaren’t used. Trim to avoid noise.Apply this diff:
- try: - import difflib - # Apply locally to preview final result - lines = [] - # Build an indexable original from a read if we normalized from read; otherwise skip - prev = "" + try: # We cannot guarantee file contents here without a read; return normalized spans only return {
93-101: Telemetry coverage choiceYou instrumented apply_text_edits, create_script, and manage_script—nice. Consider decorating delete_script and validate_script too for visibility, or confirm intentional omission.
Also applies to: 360-368, 439-448
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
15-16: Use a monotonic clock for duration
perf_counter()avoids issues from wall-clock changes.Apply this diff:
- start_time = time.time() + start_time = time.perf_counter() @@ - duration_ms = (time.time() - start_time) * 1000 + duration_ms = (time.perf_counter() - start_time) * 1000Also applies to: 38-39
39-39: Make telemetry emission failure-proofWrap
record_tool_usageto ensure telemetry can’t raise in finally.Apply this diff:
- record_tool_usage(tool_name, success, duration_ms, error) + try: + record_tool_usage(tool_name, success, duration_ms, error) + except Exception: + pass
📜 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 (11)
README.md(1 hunks)TELEMETRY.md(1 hunks)UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs(1 hunks)UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs.meta(1 hunks)UnityMcpBridge/Editor/MCPForUnityBridge.cs(1 hunks)UnityMcpBridge/UnityMcpServer~/src/config.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/server.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/telemetry.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py (1)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (7)
get_telemetry(233-238)record_telemetry(240-244)record_milestone(144-168)record_milestone(246-248)RecordType(27-36)MilestoneType(38-46)is_telemetry_enabled(287-289)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (4)
record_tool_usage(250-261)record_milestone(144-168)record_milestone(246-248)MilestoneType(38-46)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
telemetry_tool(14-17)decorator(15-16)
UnityMcpBridge/UnityMcpServer~/src/server.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (5)
record_telemetry(240-244)record_milestone(144-168)record_milestone(246-248)RecordType(27-36)MilestoneType(38-46)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
get_unity_connection(351-371)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs (3)
TelemetryHelper(11-187)RecordBridgeStartup(116-123)RecordBridgeConnection(128-141)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
telemetry_tool(10-42)decorator(12-41)UnityMcpBridge/UnityMcpServer~/src/telemetry.py (3)
record_milestone(144-168)record_milestone(246-248)MilestoneType(38-46)
UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs (1)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (3)
IsDebugEnabled(42-45)MCPForUnityBridge(105-126)IsAutoConnectMode(57-57)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py
1-1: Shebang is present but file is not executable
(EXE001)
39-39: Do not catch blind exception: Exception
(BLE001)
49-49: Do not catch blind exception: Exception
(BLE001)
57-57: Do not catch blind exception: Exception
(BLE001)
109-109: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
114-114: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
116-116: Consider moving this statement to an else block
(TRY300)
118-118: Do not catch blind exception: Exception
(BLE001)
144-144: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py
32-32: Consider moving this statement to an else block
(TRY300)
UnityMcpBridge/UnityMcpServer~/src/server.py
48-48: Do not catch blind exception: Exception
(BLE001)
49-49: Use explicit conversion flag
Replace with conversion flag
(RUF010)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
14-14: Unused function argument: tool_name
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py
132-132: Do not catch blind exception: Exception
(BLE001)
141-141: Do not catch blind exception: Exception
(BLE001)
226-226: Do not catch blind exception: Exception
(BLE001)
🪛 LanguageTool
TELEMETRY.md
[grammar] ~5-~5: There might be a mistake here.
Context: ... privacy practices. ## 🔒 Privacy First - Anonymous: We use randomly generated U...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...e documented here ## 📊 What We Collect ### Usage Analytics - Tool Usage: Which ...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ... 📊 What We Collect ### Usage Analytics - Tool Usage: Which MCP tools you use (m...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...xecution times and success/failure rates - System Info: Unity version, platform (...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...latform (Windows/Mac/Linux), MCP version - Milestones: First-time usage events (f...
(QB_NEW_EN)
[grammar] ~20-~20: There might be a mistake here.
Context: ...ol use, etc.) ### Technical Diagnostics - Connection Events: Bridge startup/conn...
(QB_NEW_EN)
[grammar] ~21-~21: There might be a mistake here.
Context: ...idge startup/connection success/failures - Error Reports: Anonymized error messag...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ... error messages (truncated to 200 chars) - Server Health: Startup time, connectio...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ...n latency ### What We DON'T Collect - ❌ Your code or script contents - ❌ Proje...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...Collect - ❌ Your code or script contents - ❌ Project names, file names, or paths - ...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ... - ❌ Project names, file names, or paths - ❌ Personal information or identifiers - ...
(QB_NEW_EN)
[grammar] ~28-~28: There might be a mistake here.
Context: ... - ❌ Personal information or identifiers - ❌ Sensitive project data - ❌ IP addresse...
(QB_NEW_EN)
[grammar] ~29-~29: There might be a mistake here.
Context: ...r identifiers - ❌ Sensitive project data - ❌ IP addresses (beyond what's needed for...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...for HTTP requests) ## 🚫 How to Opt Out ### Method 1: Environment Variable (Recommen...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...od 1: Environment Variable (Recommended) Set any of these environment variables t...
(QB_NEW_EN)
[grammar] ~48-~48: There might be a mistake here.
Context: ...### Method 2: Unity Editor (Coming Soon) In Unity Editor: `Window > MCP for Unity...
(QB_NEW_EN)
[grammar] ~51-~51: There might be a mistake here.
Context: ... Telemetry` ### Method 3: Manual Config Add to your MCP client config: ```json {...
(QB_NEW_EN)
[grammar] ~61-~61: There might be a mistake here.
Context: ... } } ``` ## 🔧 Technical Implementation ### Architecture - Python Server: Core t...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ...chnical Implementation ### Architecture - Python Server: Core telemetry collecti...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ...re telemetry collection and transmission - Unity Bridge: Local event collection f...
(QB_NEW_EN)
[grammar] ~65-~65: There might be a mistake here.
Context: ...Local event collection from Unity Editor - Anonymous UUIDs: Generated per-install...
(QB_NEW_EN)
[grammar] ~66-~66: There might be a mistake here.
Context: ...per-installation for aggregate analytics - Thread-safe: Non-blocking background t...
(QB_NEW_EN)
[grammar] ~67-~67: There might be a mistake here.
Context: ...**: Non-blocking background transmission - Fail-safe: Errors never interrupt your...
(QB_NEW_EN)
[grammar] ~70-~70: There might be a mistake here.
Context: ...nterrupt your workflow ### Data Storage Telemetry data is stored locally in: - *...
(QB_NEW_EN)
[grammar] ~71-~71: There might be a mistake here.
Context: ...age Telemetry data is stored locally in: - Windows: %APPDATA%\UnityMCP\ - **mac...
(QB_NEW_EN)
[grammar] ~72-72: There might be a mistake here./Library/Application Support/...
Context: ...ta is stored locally in: - Windows: %APPDATA%\UnityMCP\ - macOS: `
(QB_NEW_EN)
[grammar] ~73-~73: There might be a mistake here.
Context: ...s**: %APPDATA%\UnityMCP\ - macOS: ~/Library/Application Support/UnityMCP/ - Linux: ~/.local/share/UnityMCP/ Fil...
(QB_NEW_EN)
[grammar] ~76-~76: There might be a mistake here.
Context: .../.local/share/UnityMCP/ Files created: -customer_uuid.txt: Anonymous identifier - milestones.jso...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ...customer_uuid.txt: Anonymous identifier - milestones.json`: One-time events tracker ### Data Tran...
(QB_NEW_EN)
[grammar] ~80-~80: There might be a mistake here.
Context: ...me events tracker ### Data Transmission - Endpoint: `https://telemetry.coplay.de...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ... ### Data Transmission - Endpoint: https://telemetry.coplay.dev/unity-mcp/anonymous - Method: HTTPS POST with JSON payload -...
(QB_NEW_EN)
[grammar] ~82-~82: There might be a mistake here.
Context: ...Method: HTTPS POST with JSON payload - Retry: Background thread with graceful...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ... Background thread with graceful failure - Timeout: 10 second timeout, no retries...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ...s on failure ## 📈 How We Use This Data ### Product Improvement - Feature Usage:...
(QB_NEW_EN)
[grammar] ~88-~88: There might be a mistake here.
Context: ...e Use This Data ### Product Improvement - Feature Usage: Understand which tools ...
(QB_NEW_EN)
[grammar] ~94-~94: There might be a mistake here.
Context: ...ompatibility ### Development Priorities - Roadmap: Focus development on most-use...
(QB_NEW_EN)
[grammar] ~100-~100: There might be a mistake here.
Context: ... problematic areas ### What We Don't Do - ❌ Sell data to third parties - ❌ Use dat...
(QB_NEW_EN)
[grammar] ~101-~101: There might be a mistake here.
Context: ... Don't Do - ❌ Sell data to third parties - ❌ Use data for advertising/marketing - ❌...
(QB_NEW_EN)
[grammar] ~102-~102: There might be a mistake here.
Context: ...s - ❌ Use data for advertising/marketing - ❌ Track individual developers - ❌ Store ...
(QB_NEW_EN)
[grammar] ~103-~103: There might be a mistake here.
Context: ...arketing - ❌ Track individual developers - ❌ Store sensitive project information #...
(QB_NEW_EN)
[grammar] ~106-~106: There might be a mistake here.
Context: ...oject information ## 🛠️ For Developers ### Testing Telemetry ```bash cd UnityMcpBri...
(QB_NEW_EN)
[grammar] ~134-~134: There might be a mistake here.
Context: ...abled") ``` ## 📋 Data Retention Policy - Aggregated Data: Retained indefinitely...
(QB_NEW_EN)
[grammar] ~136-~136: There might be a mistake here.
Context: ...tained indefinitely for product insights - Raw Events: Automatically purged after...
(QB_NEW_EN)
[grammar] ~137-~137: There might be a mistake here.
Context: ...ts**: Automatically purged after 90 days - Personal Data: None collected, so none...
(QB_NEW_EN)
[grammar] ~138-~138: There might be a mistake here.
Context: ...Data**: None collected, so none to purge - Opt-out: Immediate - no data sent afte...
(QB_NEW_EN)
[grammar] ~141-~141: There might be a mistake here.
Context: ...opting out ## 🤝 Contact & Transparency - Questions: [Discord Community](https:/...
(QB_NEW_EN)
[grammar] ~143-~143: There might be a mistake here.
Context: ...ncy - Questions: Discord Community - Issues: [GitHub Issues](https://github...
(QB_NEW_EN)
[grammar] ~144-~144: There might be a mistake here.
Context: ...y4p8KfzrN4) - Issues: GitHub Issues - Privacy Concerns: Create a GitHub issu...
(QB_NEW_EN)
[grammar] ~148-~148: There might be a mistake here.
Context: ...epository ## 📊 Example Telemetry Event Here's what a typical telemetry event lo...
(QB_NEW_EN)
[grammar] ~168-~168: There might be a mistake here.
Context: ... "duration_ms": 42.5 } } ``` Notice: - ✅ Anonymous UUID (randomly generated) - ...
(QB_NEW_EN)
[grammar] ~169-~169: There might be a mistake here.
Context: ... - ✅ Anonymous UUID (randomly generated) - ✅ Tool performance metrics - ✅ Success...
(QB_NEW_EN)
[grammar] ~170-~170: There might be a mistake here.
Context: ... generated) - ✅ Tool performance metrics - ✅ Success/failure tracking - ❌ No code c...
(QB_NEW_EN)
[grammar] ~171-~171: There might be a mistake here.
Context: ...e metrics - ✅ Success/failure tracking - ❌ No code content - ❌ No project informa...
(QB_NEW_EN)
[grammar] ~172-~172: There might be a mistake here.
Context: ...ess/failure tracking - ❌ No code content - ❌ No project information - ❌ No personal...
(QB_NEW_EN)
[grammar] ~173-~173: There might be a mistake here.
Context: ... code content - ❌ No project information - ❌ No personal data --- *Unity MCP Tele...
(QB_NEW_EN)
README.md
[grammar] ~337-~337: There might be a mistake here.
Context: ... branch. --- ## 📊 Telemetry & Privacy Unity MCP includes **privacy-focused, an...
(QB_NEW_EN)
[grammar] ~341-~341: There might be a mistake here.
Context: ...s**: Random UUIDs only, no personal data - 🚫 Easy opt-out: Set `DISABLE_TELEMETR...
(QB_NEW_EN)
[grammar] ~342-~342: There might be a mistake here.
Context: ...BLE_TELEMETRY=true` environment variable - 📖 Transparent: See [TELEMETRY.md](TEL...
(QB_NEW_EN)
[grammar] ~343-~343: There might be a mistake here.
Context: ...METRY.md](TELEMETRY.md) for full details Your privacy matters to us. All telemetr...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
README.md
341-341: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
342-342: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
343-343: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
🔇 Additional comments (4)
README.md (1)
337-346: No duplicate “Telemetry & Privacy” section found. Only a single header exists in README.md.TELEMETRY.md (1)
34-46: Ensure opt-out vars match implementation.Docs list
DISABLE_TELEMETRY,UNITY_MCP_DISABLE_TELEMETRY, andMCP_DISABLE_TELEMETRY. Editor code currently ignoresMCP_DISABLE_TELEMETRY. Align either docs or code (see suggestion in TelemetryHelper.cs).UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py (1)
67-69: Env var toggle semantics verified — telemetry.py’s_is_disabledchecksos.environ.get(var, "").lower()against (“true”, “1”, “yes”, “on”), so settingDISABLE_TELEMETRY = "true"reliably disables telemetry and the test will not flake.UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
471-520: Legacy ‘update’ routing: handle positional over named ‘action’Upstream callers may pass arguments positionally. Your decorator looks at kwargs; routing here is fine, but ensure telemetry milestone logic also handles positional ‘action’. See decorator comment.
|
|
||
| // Record telemetry for bridge startup | ||
| TelemetryHelper.RecordBridgeStartup(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix IsAutoConnectMode property vs. method mismatch (compile break).
TelemetryHelper.RecordBridgeStartup references MCPForUnityBridge.IsAutoConnectMode as a property, but here it’s implemented as a method. This will not compile.
Apply in TelemetryHelper.cs (safer, non-breaking to existing call sites):
- ["auto_connect"] = MCPForUnityBridge.IsAutoConnectMode
+ ["auto_connect"] = MCPForUnityBridge.IsAutoConnectMode()Alternatively (riskier if there are existing call sites), convert this to a property in MCPForUnityBridge:
- public static bool IsAutoConnectMode() => isAutoConnectMode;
+ public static bool IsAutoConnectMode => isAutoConnectMode;📝 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.
| // Record telemetry for bridge startup | |
| TelemetryHelper.RecordBridgeStartup(); | |
| } | |
| // File: TelemetryHelper.cs | |
| public static void RecordBridgeStartup() | |
| { | |
| // ... other telemetry fields ... | |
| var data = new Dictionary<string, object> | |
| { | |
| ["version"] = Application.version, | |
| ["auto_connect"] = MCPForUnityBridge.IsAutoConnectMode() | |
| }; | |
| TelemetryManager.Send("bridge_startup", data); | |
| } |
🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/MCPForUnityBridge.cs around lines 72-75,
RecordBridgeStartup calls MCPForUnityBridge.IsAutoConnectMode as a property but
that identifier is implemented as a method; update TelemetryHelper instead to
call the method form: locate TelemetryHelper.RecordBridgeStartup in
TelemetryHelper.cs and change any references from
MCPForUnityBridge.IsAutoConnectMode to MCPForUnityBridge.IsAutoConnectMode()
(ensuring correct static/instance access and visibility), or if you prefer the
other option, convert MCPForUnityBridge.IsAutoConnectMode() into a public bool
property named IsAutoConnectMode so existing property-style call sites remain
valid.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (8)
64-66: Make endpoint overridable via env for dev/testHardcoding production makes local/dev testing harder. Allow an env override.
Apply:
- # Telemetry endpoint - hardcoded to Coplay production API - self.endpoint = "https://api-prod.coplay.dev/telemetry/events" + # Telemetry endpoint (env override: UNITY_MCP_TELEMETRY_ENDPOINT) + self.endpoint = os.environ.get( + "UNITY_MCP_TELEMETRY_ENDPOINT", + "https://api-prod.coplay.dev/telemetry/events", + )
110-116: Avoid creating local UUID/files when telemetry is disabledCurrently we always load/write persistent data even if opted out. Gate it to align with privacy expectations.
Apply:
def __init__(self): self.config = TelemetryConfig() self._customer_uuid: Optional[str] = None self._milestones: Dict[str, Dict[str, Any]] = {} - self._load_persistent_data() + self._lock = threading.Lock() + if self.config.enabled: + self._load_persistent_data()
91-101: Use platform.system() for OS detection (simpler, clearer macOS path)Avoid os.uname() branching. This also aligns with platform names you document.
Apply:
- if os.name == 'nt': # Windows - base_dir = Path(os.environ.get('APPDATA', Path.home() / 'AppData' / 'Roaming')) - elif os.name == 'posix': # macOS/Linux - if 'darwin' in os.uname().sysname.lower(): # macOS - base_dir = Path.home() / 'Library' / 'Application Support' - else: # Linux - base_dir = Path(os.environ.get('XDG_DATA_HOME', Path.home() / '.local' / 'share')) + system = __import__("platform").system() + if system == "Windows": + base_dir = Path(os.environ.get("APPDATA", Path.home() / "AppData" / "Roaming")) + elif system == "Darwin": # macOS + base_dir = Path.home() / "Library" / "Application Support" + else: # Linux/Unix + base_dir = Path(os.environ.get("XDG_DATA_HOME", Path.home() / ".local" / "share"))
116-133: Narrow exception handling for file I/O and include tracebacks at debug levelCatching bare Exception hides real issues (Ruff BLE001). Limit to OSError/JSON errors and include traceback.
Apply:
- except Exception as e: - logger.warning(f"Failed to load telemetry data: {e}") + except (OSError, json.JSONDecodeError): + logger.debug("Failed to load telemetry data", exc_info=True) self._customer_uuid = str(uuid.uuid4()) self._milestones = {}
134-140: Same here: narrow exception class on saveApply:
- except Exception as e: - logger.warning(f"Failed to save milestones: {e}") + except OSError: + logger.debug("Failed to save milestones", exc_info=True)
196-207: Report platform via platform.system() and accept any 2xx as success; make version configurableThis aligns payload with docs and avoids false negatives on 201/204.
Apply:
+VERSION = os.environ.get("UNITY_MCP_VERSION", "3.0.2") @@ - "version": "3.0.2", # Unity MCP version - "platform": os.name + "version": VERSION, # Unity MCP version + "platform": __import__("platform").system() @@ - if response.status_code == 200: + if response.is_success: logger.debug(f"Telemetry sent: {record.record_type}") else: logger.debug(f"Telemetry failed: HTTP {response.status_code}")Also applies to: 218-221
167-195: Consider a single worker thread + queue instead of a thread per eventCurrent approach risks thread proliferation under bursty telemetry. A tiny queue-based worker keeps it bounded.
If helpful, I can send a follow-up patch introducing queue.Queue with one daemon worker and a graceful shutdown hook.
272-277: Unify error truncation to 200 chars to match docsDocs say 200 chars; here it’s 500.
Apply:
- "error": str(error)[:500] # Limit error message length + "error": str(error)[:200] # Limit error message lengthTELEMETRY.md (5)
12-19: Platform naming: docs say Windows/Mac/Linux, example shows “posix”After switching payload to platform.system(), platform will be “Windows”, “Darwin”, or “Linux”. Update wording accordingly.
Apply:
-- **System Info**: Unity version, platform (Windows/Mac/Linux), MCP version +- **System Info**: Unity version, platform (Windows/macOS/Linux), MCP version
80-85: Clarify “Retry” vs “no retries”The bullet label “Retry” conflicts with “no retries on failure.” Rename to “Transport” and add the new endpoint override.
Apply:
-**Retry**: Background thread with graceful failure -**Timeout**: 10 second timeout, no retries on failure +**Transport**: Background thread with graceful failure (no retries) +**Timeout**: 10 second timeout +**Config override**: Set `UNITY_MCP_TELEMETRY_ENDPOINT` to point at a custom endpoint (e.g., staging)
152-166: Keep example consistent with payload (platform and error truncation)Update example to reflect platform.system() and 200-char error truncation elsewhere in the doc.
Apply:
- "platform": "posix", + "platform": "Windows",
134-140: Optional: note version configurability for reproducible telemetry in test runsIf you accept the
UNITY_MCP_VERSIONenv var change, mention it here.Proposed addition:
-**Opt-out**: Immediate - no data sent after opting out +**Opt-out**: Immediate - no data sent after opting out +**Version override (testing)**: Set `UNITY_MCP_VERSION` to pin the reported MCP version during tests
61-69: “Thread-safe” claim needs nuanceMilestone writes weren’t thread-safe; once you add a lock (see code review), this stays accurate.
📜 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)
TELEMETRY.md(1 hunks)UnityMcpBridge/UnityMcpServer~/src/config.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/telemetry.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- UnityMcpBridge/UnityMcpServer~/src/config.py
🧰 Additional context used
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py
129-129: Do not catch blind exception: Exception
(BLE001)
138-138: Do not catch blind exception: Exception
(BLE001)
223-223: Do not catch blind exception: Exception
(BLE001)
🪛 LanguageTool
TELEMETRY.md
[grammar] ~5-~5: There might be a mistake here.
Context: ... privacy practices. ## 🔒 Privacy First - Anonymous: We use randomly generated U...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...e documented here ## 📊 What We Collect ### Usage Analytics - Tool Usage: Which ...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ... 📊 What We Collect ### Usage Analytics - Tool Usage: Which MCP tools you use (m...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...xecution times and success/failure rates - System Info: Unity version, platform (...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...latform (Windows/Mac/Linux), MCP version - Milestones: First-time usage events (f...
(QB_NEW_EN)
[grammar] ~20-~20: There might be a mistake here.
Context: ...ol use, etc.) ### Technical Diagnostics - Connection Events: Bridge startup/conn...
(QB_NEW_EN)
[grammar] ~21-~21: There might be a mistake here.
Context: ...idge startup/connection success/failures - Error Reports: Anonymized error messag...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ... error messages (truncated to 200 chars) - Server Health: Startup time, connectio...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ...n latency ### What We DON'T Collect - ❌ Your code or script contents - ❌ Proje...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...Collect - ❌ Your code or script contents - ❌ Project names, file names, or paths - ...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ... - ❌ Project names, file names, or paths - ❌ Personal information or identifiers - ...
(QB_NEW_EN)
[grammar] ~28-~28: There might be a mistake here.
Context: ... - ❌ Personal information or identifiers - ❌ Sensitive project data - ❌ IP addresse...
(QB_NEW_EN)
[grammar] ~29-~29: There might be a mistake here.
Context: ...r identifiers - ❌ Sensitive project data - ❌ IP addresses (beyond what's needed for...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...for HTTP requests) ## 🚫 How to Opt Out ### Method 1: Environment Variable (Recommen...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...od 1: Environment Variable (Recommended) Set any of these environment variables t...
(QB_NEW_EN)
[grammar] ~48-~48: There might be a mistake here.
Context: ...### Method 2: Unity Editor (Coming Soon) In Unity Editor: `Window > MCP for Unity...
(QB_NEW_EN)
[grammar] ~51-~51: There might be a mistake here.
Context: ... Telemetry` ### Method 3: Manual Config Add to your MCP client config: ```json {...
(QB_NEW_EN)
[grammar] ~61-~61: There might be a mistake here.
Context: ... } } ``` ## 🔧 Technical Implementation ### Architecture - Python Server: Core t...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ...chnical Implementation ### Architecture - Python Server: Core telemetry collecti...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ...re telemetry collection and transmission - Unity Bridge: Local event collection f...
(QB_NEW_EN)
[grammar] ~65-~65: There might be a mistake here.
Context: ...Local event collection from Unity Editor - Anonymous UUIDs: Generated per-install...
(QB_NEW_EN)
[grammar] ~66-~66: There might be a mistake here.
Context: ...per-installation for aggregate analytics - Thread-safe: Non-blocking background t...
(QB_NEW_EN)
[grammar] ~67-~67: There might be a mistake here.
Context: ...**: Non-blocking background transmission - Fail-safe: Errors never interrupt your...
(QB_NEW_EN)
[grammar] ~70-~70: There might be a mistake here.
Context: ...nterrupt your workflow ### Data Storage Telemetry data is stored locally in: - *...
(QB_NEW_EN)
[grammar] ~71-~71: There might be a mistake here.
Context: ...age Telemetry data is stored locally in: - Windows: %APPDATA%\UnityMCP\ - **mac...
(QB_NEW_EN)
[grammar] ~72-72: There might be a mistake here./Library/Application Support/...
Context: ...ta is stored locally in: - Windows: %APPDATA%\UnityMCP\ - macOS: `
(QB_NEW_EN)
[grammar] ~73-~73: There might be a mistake here.
Context: ...s**: %APPDATA%\UnityMCP\ - macOS: ~/Library/Application Support/UnityMCP/ - Linux: ~/.local/share/UnityMCP/ Fil...
(QB_NEW_EN)
[grammar] ~76-~76: There might be a mistake here.
Context: .../.local/share/UnityMCP/ Files created: -customer_uuid.txt: Anonymous identifier - milestones.jso...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ...customer_uuid.txt: Anonymous identifier - milestones.json`: One-time events tracker ### Data Tran...
(QB_NEW_EN)
[grammar] ~80-~80: There might be a mistake here.
Context: ...me events tracker ### Data Transmission - Endpoint: `https://api-prod.coplay.dev...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ... ### Data Transmission - Endpoint: https://api-prod.coplay.dev/telemetry/events - Method: HTTPS POST with JSON payload -...
(QB_NEW_EN)
[grammar] ~82-~82: There might be a mistake here.
Context: ...Method: HTTPS POST with JSON payload - Retry: Background thread with graceful...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ... Background thread with graceful failure - Timeout: 10 second timeout, no retries...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ...s on failure ## 📈 How We Use This Data ### Product Improvement - Feature Usage:...
(QB_NEW_EN)
[grammar] ~88-~88: There might be a mistake here.
Context: ...e Use This Data ### Product Improvement - Feature Usage: Understand which tools ...
(QB_NEW_EN)
[grammar] ~94-~94: There might be a mistake here.
Context: ...ompatibility ### Development Priorities - Roadmap: Focus development on most-use...
(QB_NEW_EN)
[grammar] ~100-~100: There might be a mistake here.
Context: ... problematic areas ### What We Don't Do - ❌ Sell data to third parties - ❌ Use dat...
(QB_NEW_EN)
[grammar] ~101-~101: There might be a mistake here.
Context: ... Don't Do - ❌ Sell data to third parties - ❌ Use data for advertising/marketing - ❌...
(QB_NEW_EN)
[grammar] ~102-~102: There might be a mistake here.
Context: ...s - ❌ Use data for advertising/marketing - ❌ Track individual developers - ❌ Store ...
(QB_NEW_EN)
[grammar] ~103-~103: There might be a mistake here.
Context: ...arketing - ❌ Track individual developers - ❌ Store sensitive project information #...
(QB_NEW_EN)
[grammar] ~106-~106: There might be a mistake here.
Context: ...oject information ## 🛠️ For Developers ### Testing Telemetry ```bash cd UnityMcpBri...
(QB_NEW_EN)
[grammar] ~134-~134: There might be a mistake here.
Context: ...abled") ``` ## 📋 Data Retention Policy - Aggregated Data: Retained indefinitely...
(QB_NEW_EN)
[grammar] ~136-~136: There might be a mistake here.
Context: ...tained indefinitely for product insights - Raw Events: Automatically purged after...
(QB_NEW_EN)
[grammar] ~137-~137: There might be a mistake here.
Context: ...ts**: Automatically purged after 90 days - Personal Data: None collected, so none...
(QB_NEW_EN)
[grammar] ~138-~138: There might be a mistake here.
Context: ...Data**: None collected, so none to purge - Opt-out: Immediate - no data sent afte...
(QB_NEW_EN)
[grammar] ~141-~141: There might be a mistake here.
Context: ...opting out ## 🤝 Contact & Transparency - Questions: [Discord Community](https:/...
(QB_NEW_EN)
[grammar] ~143-~143: There might be a mistake here.
Context: ...ncy - Questions: Discord Community - Issues: [GitHub Issues](https://github...
(QB_NEW_EN)
[grammar] ~144-~144: There might be a mistake here.
Context: ...y4p8KfzrN4) - Issues: GitHub Issues - Privacy Concerns: Create a GitHub issu...
(QB_NEW_EN)
[grammar] ~148-~148: There might be a mistake here.
Context: ...epository ## 📊 Example Telemetry Event Here's what a typical telemetry event lo...
(QB_NEW_EN)
[grammar] ~168-~168: There might be a mistake here.
Context: ... "duration_ms": 42.5 } } ``` Notice: - ✅ Anonymous UUID (randomly generated) - ...
(QB_NEW_EN)
[grammar] ~169-~169: There might be a mistake here.
Context: ... - ✅ Anonymous UUID (randomly generated) - ✅ Tool performance metrics - ✅ Success...
(QB_NEW_EN)
[grammar] ~170-~170: There might be a mistake here.
Context: ... generated) - ✅ Tool performance metrics - ✅ Success/failure tracking - ❌ No code c...
(QB_NEW_EN)
[grammar] ~171-~171: There might be a mistake here.
Context: ...e metrics - ✅ Success/failure tracking - ❌ No code content - ❌ No project informa...
(QB_NEW_EN)
[grammar] ~172-~172: There might be a mistake here.
Context: ...ess/failure tracking - ❌ No code content - ❌ No project information - ❌ No personal...
(QB_NEW_EN)
[grammar] ~173-~173: There might be a mistake here.
Context: ... code content - ❌ No project information - ❌ No personal data --- *Unity MCP Tele...
(QB_NEW_EN)
🔇 Additional comments (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (2)
1-17: Overall: solid, privacy-conscious design with non-blocking transportNice job keeping telemetry optional, anonymous, and failure-safe. API surface is clean and easy to integrate.
284-286: Non-blocking status check should not create local files when disabled (covered by earlier change)Once gating _load_persistent_data by enabled, calling is_telemetry_enabled won’t create UUID/data files. Good.
…emetry * commit '3e83f993bfe632034bf7302d4319e3cd16353eb8': Improved ci prompt testing suite (#270) chore: bump version to 3.3.2 Fix: Unity Editor reload crash + debug-noise reduction (#266) Revise README for improved clarity and organization docs: install uv via official installer (curl/winget) Update README.md docs: fix Windows uv path to use WinGet shim, keep macOS AppSupport symlink path docs: update README.md with improved installation paths, documentation, and logo fix: Update README installation paths to match ServerInstaller.cs
… decorator; add endpoint env override + urllib fallback; enrich OS fields; fix TelemetryHelper invocation
- Add server-side integer coercion for numeric parameters in all tools - Fix parameter type validation issues (read_resource, find_in_file, read_console, manage_scene, manage_asset) - Add proper tool descriptions with ctx parameter documentation - Fix Context type annotations (use Context instead of Any for ctx) - All tools now accept flexible numeric inputs (strings, floats) and coerce to integers - Telemetry system working with all tool_execution events captured in BigQuery - Remove invalid parameter type warnings from client-side validation
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
README.md (1)
319-321: Fix path separators for macOS/Linux examples.Backslashes break on Unix paths; use forward slashes.
- - **macOS:** `~/Library/AppSupport/UnityMCP/UnityMcpServer\src` - - **Linux:** `~/.local/share/UnityMCP/UnityMcpServer\src` + - **macOS:** `~/Library/AppSupport/UnityMCP/UnityMcpServer/src` + - **Linux:** `~/.local/share/UnityMCP/UnityMcpServer/src`UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py (1)
125-128: Bug: wrong key popped;prefab_foldervsprefabFolder.The intended key isn’t removed, so C# receives an unexpected field.
- params.pop("prefab_folder", None) + params.pop("prefabFolder", None)UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py (1)
17-22: Avoid blocking the event loop: make the tool sync or use non-blocking I/O.
send_command_with_retryis synchronous (uses time.sleep). Wrapping it in anasync defwill block the loop when awaited via the telemetry decorator.- async def execute_menu_item( + def execute_menu_item( ctx: Any, menu_path: str, action: str = 'execute', parameters: Dict[str, Any] = None, ) -> Dict[str, Any]:
♻️ Duplicate comments (5)
UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs (2)
23-41: HonorMCP_DISABLE_TELEMETRYand use robust truthy parsing.Aligns with protocol-wide opt-out and avoids culture allocations from
ToLower().- var envDisable = Environment.GetEnvironmentVariable("DISABLE_TELEMETRY"); - if (!string.IsNullOrEmpty(envDisable) && - (envDisable.ToLower() == "true" || envDisable == "1")) + var envDisable = Environment.GetEnvironmentVariable("DISABLE_TELEMETRY"); + if (!string.IsNullOrEmpty(envDisable) && + (envDisable.Equals("true", StringComparison.OrdinalIgnoreCase) || envDisable == "1")) { return false; } var unityMcpDisable = Environment.GetEnvironmentVariable("UNITY_MCP_DISABLE_TELEMETRY"); - if (!string.IsNullOrEmpty(unityMcpDisable) && - (unityMcpDisable.ToLower() == "true" || unityMcpDisable == "1")) + if (!string.IsNullOrEmpty(unityMcpDisable) && + (unityMcpDisable.Equals("true", StringComparison.OrdinalIgnoreCase) || unityMcpDisable == "1")) { return false; } + + var mcpDisable = Environment.GetEnvironmentVariable("MCP_DISABLE_TELEMETRY"); + if (!string.IsNullOrEmpty(mcpDisable) && + (mcpDisable.Equals("true", StringComparison.OrdinalIgnoreCase) || mcpDisable == "1")) + { + return false; + }
163-174: Make sending pluggable; call registered sender if set.Enables the bridge to forward telemetry to the Python server.
- private static void SendTelemetryToPythonServer(Dictionary<string, object> telemetryData) + private static volatile Action<Dictionary<string, object>> s_sender; + + public static void RegisterTelemetrySender(Action<Dictionary<string, object>> sender) + { + System.Threading.Interlocked.Exchange(ref s_sender, sender); + } + + private static void SendTelemetryToPythonServer(Dictionary<string, object> telemetryData) { - // This would integrate with the existing bridge communication system - // For now, we'll just log it when debug is enabled - if (IsDebugEnabled()) - { - Debug.Log($"<b><color=#2EA3FF>MCP-TELEMETRY</color></b>: {telemetryData["event_type"]}"); - } - - // TODO: Integrate with MCPForUnityBridge command system - // We would send this as a special telemetry command to the Python server + var sender = s_sender; + if (sender != null) + { + try { sender(telemetryData); return; } + catch (Exception ex) + { + if (IsDebugEnabled()) Debug.LogWarning($"Telemetry sender error: {ex.Message}"); + } + } + if (IsDebugEnabled()) + { + Debug.Log($"<b><color=#2EA3FF>MCP-TELEMETRY</color></b>: {telemetryData["event_type"]}"); + } }UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
29-35: Make milestone checks robust to positional args; never let telemetry affect tool behavior.Handle positional
action(e.g.,(ctx, action, ...)) and guard milestone recording.- result = func(*args, **kwargs) - success = True - if tool_name == "manage_script" and kwargs.get("action") == "create": - record_milestone(MilestoneType.FIRST_SCRIPT_CREATION) - elif tool_name.startswith("manage_scene"): - record_milestone(MilestoneType.FIRST_SCENE_MODIFICATION) - record_milestone(MilestoneType.FIRST_TOOL_USAGE) + result = func(*args, **kwargs) + success = True + action_val = kwargs.get("action") + if action_val is None and len(args) >= 2: + action_val = args[1] + try: + if tool_name == "manage_script" and action_val == "create": + record_milestone(MilestoneType.FIRST_SCRIPT_CREATION) + elif tool_name.startswith("manage_scene"): + record_milestone(MilestoneType.FIRST_SCENE_MODIFICATION) + record_milestone(MilestoneType.FIRST_TOOL_USAGE) + except Exception: + pass return result
54-60: Async path parity: apply the same positional ‘action’ handling and safety.- result = await func(*args, **kwargs) - success = True - if tool_name == "manage_script" and kwargs.get("action") == "create": - record_milestone(MilestoneType.FIRST_SCRIPT_CREATION) - elif tool_name.startswith("manage_scene"): - record_milestone(MilestoneType.FIRST_SCENE_MODIFICATION) - record_milestone(MilestoneType.FIRST_TOOL_USAGE) + result = await func(*args, **kwargs) + success = True + action_val = kwargs.get("action") + if action_val is None and len(args) >= 2: + action_val = args[1] + try: + if tool_name == "manage_script" and action_val == "create": + record_milestone(MilestoneType.FIRST_SCRIPT_CREATION) + elif tool_name.startswith("manage_scene"): + record_milestone(MilestoneType.FIRST_SCENE_MODIFICATION) + record_milestone(MilestoneType.FIRST_TOOL_USAGE) + except Exception: + pass return resultUnityMcpBridge/UnityMcpServer~/src/telemetry.py (1)
115-121: Protect milestone writes with a lock to prevent double-recording under concurrencyConcurrent calls can pass the contains-check and both write. Guard the check-then-set and file save with a lock; invoke
record()outside the critical section.Apply:
class TelemetryCollector: @@ def __init__(self): self.config = TelemetryConfig() self._customer_uuid: Optional[str] = None self._milestones: Dict[str, Dict[str, Any]] = {} + self._lock = threading.Lock() self._load_persistent_data() @@ def record_milestone(self, milestone: MilestoneType, data: Optional[Dict[str, Any]] = None) -> bool: """Record a milestone event, returns True if this is the first occurrence""" if not self.config.enabled: return False - - milestone_key = milestone.value - if milestone_key in self._milestones: - return False # Already recorded - - milestone_data = { - "timestamp": time.time(), - "data": data or {} - } - - self._milestones[milestone_key] = milestone_data - self._save_milestones() + milestone_key = milestone.value + with self._lock: + if milestone_key in self._milestones: + return False # Already recorded + milestone_data = { + "timestamp": time.time(), + "data": data or {} + } + self._milestones[milestone_key] = milestone_data + self._save_milestones() # Also send as telemetry record self.record( record_type=RecordType.USAGE, data={"milestone": milestone_key, **(data or {})}, milestone=milestone ) return TrueAlso applies to: 146-171
🧹 Nitpick comments (38)
README.md (2)
299-301: Standardize list style to match markdownlint (MD004).Switch dashes to asterisks per repository style.
-- **🔒 Anonymous**: Random UUIDs only, no personal data -- **🚫 Easy opt-out**: Set `DISABLE_TELEMETRY=true` environment variable -- **📖 Transparent**: See [TELEMETRY.md](TELEMETRY.md) for full details +* **🔒 Anonymous**: Random UUIDs only, no personal data +* **🚫 Easy opt-out**: Set `DISABLE_TELEMETRY=true` environment variable +* **📖 Transparent**: See [TELEMETRY.md](TELEMETRY.md) for full details
295-304: Document all supported opt-out environment variables.README mentions only
DISABLE_TELEMETRY, but the Editor code also honorsUNITY_MCP_DISABLE_TELEMETRYand should additionally honorMCP_DISABLE_TELEMETRY(protocol-wide). Add them here for consistency.- - **🚫 Easy opt-out**: Set `DISABLE_TELEMETRY=true` environment variable + - **🚫 Easy opt-out**: Set one of `DISABLE_TELEMETRY=true`, `UNITY_MCP_DISABLE_TELEMETRY=true`, or `MCP_DISABLE_TELEMETRY=true`UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs (2)
38-41: Guard EditorPrefs read.Mirror your defensive style used in IsDebugEnabled().
- return !UnityEditor.EditorPrefs.GetBool(TELEMETRY_DISABLED_KEY, false); + try { return !UnityEditor.EditorPrefs.GetBool(TELEMETRY_DISABLED_KEY, false); } + catch { return true; }
120-122: Avoid hardcodingbridge_version.Source from assembly/package metadata to prevent drift.
- ["bridge_version"] = "3.0.2", + ["bridge_version"] = GetBridgeVersion(),Add this helper in the same class (outside this hunk):
private static string GetBridgeVersion() { try { var asm = typeof(TelemetryHelper).Assembly; var infoAttr = (System.Reflection.AssemblyInformationalVersionAttribute) Attribute.GetCustomAttribute(asm, typeof(System.Reflection.AssemblyInformationalVersionAttribute)); return infoAttr?.InformationalVersion ?? asm.GetName().Version?.ToString() ?? "unknown"; } catch { return "unknown"; } }UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py (3)
15-16: Silence unused context warning.Rename to
_ctx(keeps signature shape without Ruff ARG001).- ctx: Any, + _ctx: Any,
1-6: Trim unused imports.Remove unused
Context,get_unity_connection,config, andtime.-from mcp.server.fastmcp import FastMCP, Context -from typing import Dict, Any, List -from unity_connection import get_unity_connection, send_command_with_retry -from config import config -import time +from mcp.server.fastmcp import FastMCP +from typing import Dict, Any, List +from unity_connection import send_command_with_retry
85-111: Match docs: default includeNonPublicSerialized=True for get_components.Docstring promises True by default; enforce when not provided.
- params = { + if action == "get_components" and includeNonPublicSerialized is None: + includeNonPublicSerialized = True + + params = {UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py (2)
3-3: Drop unused import to avoid dead code.
get_unity_connectionisn’t used here.-from unity_connection import get_unity_connection, send_command_with_retry +from unity_connection import send_command_with_retry
16-22: Silence unused ctx argument (Ruff ARG001).FastMCP requires
ctx, but it’s not used. Explicitly discard it.def manage_shader( - ctx: Any, + ctx: Any, action: str, name: str, path: str, contents: str, ) -> Dict[str, Any]: @@ - try: + # ctx is unused in this tool + del ctx + try:UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py (1)
31-36: Silence unused ctx argument (Ruff ARG001).- """Executes a Unity Editor menu item via its path (e.g., "File/Save Project").""" + """Executes a Unity Editor menu item via its path (e.g., "File/Save Project").""" + del ctxUnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (2)
34-51: Deduplicate _coerce_int: reuse the shared helper from resource_tools.This local copy diverges riskily from the shared version and was flagged for broad excepts. Prefer importing the canonical helper.
-from telemetry_decorator import telemetry_tool +from telemetry_decorator import telemetry_tool +from tools.resource_tools import _coerce_int @@ - # Coerce numeric inputs defensively - def _coerce_int(value, default=None): - if value is None: - return default - try: - if isinstance(value, bool): - return default - if isinstance(value, int): - return int(value) - s = str(value).strip() - if s.lower() in ("", "none", "null"): - return default - return int(float(s)) - except Exception: - return default - - build_index = _coerce_int(build_index, default=0) + build_index = _coerce_int(build_index, default=0)
15-20: Silence unused ctx argument (Ruff ARG001).- def manage_scene( - ctx: Any, + def manage_scene( + ctx: Any, action: str, name: str, path: str, build_index: Any, ) -> Dict[str, Any]: @@ - try: + # ctx is unused in this tool + del ctx + try:Also applies to: 33-36
UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py (2)
1-6: Remove unused imports.-from mcp.server.fastmcp import FastMCP, Context -import time +from mcp.server.fastmcp import FastMCP, Context from typing import Dict, Any -from unity_connection import get_unity_connection, send_command_with_retry -from config import config +from unity_connection import send_command_with_retry
37-43: Normalize telemetry_status response shape for consistency.Other tools return data under a
datakey.- if action == "telemetry_status": - return {"success": True, "telemetry_enabled": is_telemetry_enabled()} + if action == "telemetry_status": + return {"success": True, "data": {"telemetry_enabled": is_telemetry_enabled()}}UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
14-17: Silence unused parameter in telemetry fallback.Minor lint fix in the no-op decorator to avoid ARG001 when telemetry isn’t present.
- def telemetry_tool(tool_name: str): + def telemetry_tool(_: str): def decorator(func): return func return decorator
93-99: Silence unused ctx arguments across tools.All these tool functions currently ignore
ctx. Discard explicitly to quiet linters and make intent clear.def apply_text_edits( - ctx: Context, + ctx: Context, uri: str, @@ - ) -> Dict[str, Any]: + ) -> Dict[str, Any]: + del ctx """Apply small text edits to a C# script identified by URI."""Repeat the same
del ctxfirst statement for:
- create_script
- delete_script
- validate_script
- manage_script
- manage_script_capabilities
- get_sha
Also applies to: 360-367, 401-404, 416-420, 441-450, 602-604 </blockquote></details> <details> <summary>UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)</summary><blockquote> `55-72`: **Guard replace_range against out-of-bounds columns.** Clamping columns to line length avoids misaddressed slices on short lines during local apply. ```diff 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 index_of(line: int, col: int) -> int: + if line <= len(lines): + line_text = lines[line - 1] + # col is 1-based; allow EOF position at len+1 + col_clamped = max(1, min(col, len(line_text) + 1)) + return sum(len(l) for l in lines[: line - 1]) + (col_clamped - 1) + return sum(len(l) for l in lines)
351-359: Silence unused ctx argument (Ruff ARG001).- def script_apply_edits( + def script_apply_edits( ctx: Context, name: str, path: str, edits: List[Dict[str, Any]], options: Optional[Dict[str, Any]] = None, script_type: str = "MonoBehaviour", namespace: str = "", ) -> Dict[str, Any]: - # Normalize locator first so downstream calls target the correct script file. + del ctx + # Normalize locator first so downstream calls target the correct script file.UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py (6)
4-4: Use Optional[...] in type hints and import itPEP 484 discourages implicit Optional. Also add Optional to imports.
-from typing import List, Dict, Any +from typing import List, Dict, Any, Optional @@ - def read_console( + def read_console( ctx: Context, - action: str = None, - types: List[str] = None, + action: Optional[str] = None, + types: Optional[List[str]] = None, count: Any = None, - filter_text: str = None, - since_timestamp: str = None, - format: str = None, - include_stacktrace: bool = None + filter_text: Optional[str] = None, + since_timestamp: Optional[str] = None, + format: Optional[str] = None, + include_stacktrace: Optional[bool] = None ) -> Dict[str, Any]:Also applies to: 16-25
17-17: Silence unused ctx (if keeping it for signature compatibility)Either rename to
_ctxor add a no-op reference to avoid ARG001.- ctx: Context, + _ctx: Context,
55-70: Deduplicate _coerce_int across toolsThis helper is repeated in multiple modules with slight differences. Prefer a single shared implementation to avoid drift.
Option A (quick): import the existing helper from resource_tools.
- # Coerce count defensively (string/float -> int) - def _coerce_int(value, default=None): - if value is None: - return default - try: - if isinstance(value, bool): - return default - if isinstance(value, int): - return int(value) - s = str(value).strip() - if s.lower() in ("", "none", "null"): - return default - return int(float(s)) - except Exception: - return default + # Coerce count defensively (string/float -> int) + from tools.resource_tools import _coerce_int # reuse shared helperIf the import path differs in your layout, adjust accordingly or extract a small utils module (e.g., utils/coercion.py) and import from there.
85-90: Remove dead code around 'count' reinsertionThe dict-comp explicitly keeps 'count' even when None, so the subsequent reinsertion is unreachable.
- # Add count back if it was None, explicitly sending null might be important for C# logic - if 'count' not in params_dict: - params_dict['count'] = None
45-49: Normalize inputs: validate action and lower-case typesAvoid unexpected casing and invalid actions leaking downstream.
action = action if action is not None else 'get' - types = types if types is not None else ['error', 'warning', 'log'] + types = types if types is not None else ['error', 'warning', 'log'] + if types is not None: + types = [str(t).lower() for t in types] @@ - "action": action, + "action": action if action in ("get", "clear") else "get",Also applies to: 75-82
5-5: Drop unused imports
timeandconfigare unused here.-import time @@ -from config import configAlso applies to: 8-8
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (5)
20-20: Silence unused ctx (if required by the tool signature)Rename to
_ctxto satisfy linters.- ctx: Any, + _ctx: Any,
56-71: Consolidate _coerce_int with shared helperAvoid duplicated logic across tools; also narrow the exception type if you keep it local.
Option A (reuse):
- def _coerce_int(value, default=None): - if value is None: - return default - try: - if isinstance(value, bool): - return default - if isinstance(value, int): - return int(value) - s = str(value).strip() - if s.lower() in ("", "none", "null"): - return default - return int(float(s)) - except Exception: - return default + from tools.resource_tools import _coerce_intOption B (narrow exceptions):
- except Exception: + except (TypeError, ValueError, OverflowError): return default
9-10: Remove unused imports
configandtimeare not used.-from config import config -import time
96-96: Avoid unused variable assignmentEstablishing the connection for side effects is fine; just drop the unused binding.
- connection = get_unity_connection() + get_unity_connection()
30-31: Optional: clarify pagination typesIf these are intended to be integers, prefer
Optional[int]in the signature to capture intent.- page_size: Any = None, - page_number: Any = None + page_size: Optional[int] = None, + page_number: Optional[int] = NoneUnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (4)
141-148: list_resources: inputs and bounds look solid; one small nitLimit coercion + Assets-only enforcement are good. You can simplify the break condition since
minimum=1already guarantees ≥1.- if len(matches) >= max(1, limit_int): + if len(matches) >= limit_int: breakAlso applies to: 168-187
203-214: read_resource: precedence and safety LGTM; consider large-file guardrailsFull-text reads are fine for code files, but can spike memory on large assets. Optionally cap
head_bytesand tail/line windows to a sane upper bound (e.g., 1–2 MB or ~10k lines).- if head_bytes and head_bytes > 0: + MAX_BYTES = 2_000_000 + if head_bytes and head_bytes > 0: - raw = p.read_bytes()[: head_bytes] + raw = p.read_bytes()[: min(head_bytes, MAX_BYTES)] @@ - if tail_lines is not None and tail_lines > 0: + if tail_lines is not None and tail_lines > 0: lines = text.splitlines() - n = max(0, tail_lines) + n = max(0, min(tail_lines, 10_000)) text = "\n".join(lines[-n:]) - elif start_line is not None and line_count is not None and line_count >= 0: + elif start_line is not None and line_count is not None and line_count >= 0: lines = text.splitlines() s = max(0, start_line - 1) - e = min(len(lines), s + line_count) + e = min(len(lines), s + min(line_count, 10_000)) text = "\n".join(lines[s:e])Also applies to: 321-346
351-358: find_in_file: guard against regex DoS and excessive payloadsUntrusted patterns can backtrack badly. You already cap results; add simple guards on pattern size and line length to reduce risk.
- async def find_in_file( + async def find_in_file( uri: str, pattern: str, @@ - rx = re.compile(pattern, flags) + if len(pattern) > 2000: + return {"success": False, "error": "Pattern too long"} + rx = re.compile(pattern, flags) @@ - if rx.search(line): + if len(line) > 20000: + continue + if rx.search(line): results.append({"line": i, "text": line})Also applies to: 379-386
65-66: Catching bare Exception is acceptable here but consider narrowingWhere feasible, catch specific exceptions (e.g., OSError, UnicodeDecodeError, re.error) to avoid masking unrelated issues, while still returning structured errors.
Also applies to: 95-96, 194-196, 346-347, 389-391
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (5)
139-145: Narrow exception and keep stacktrace when saving milestonesCatching
Exceptionhides programming errors and obscures diagnostics.Apply:
def _save_milestones(self): """Save milestones to disk""" - try: - self.config.milestones_file.write_text(json.dumps(self._milestones, indent=2)) - except Exception as e: - logger.warning(f"Failed to save milestones: {e}") + try: + self.config.milestones_file.write_text(json.dumps(self._milestones, indent=2), encoding="utf-8") + except OSError as e: + logger.debug(f"Failed to save milestones: {e}", exc_info=True)
219-222: Avoid hardcoding version; read from config/envEmbedding "3.0.2" risks drift. Make it configurable and reuse.
Apply:
@@ class TelemetryConfig: - # Session tracking + # Version and session tracking + self.version = os.environ.get("UNITY_MCP_VERSION", "3.0.2") self.session_id = str(uuid.uuid4()) @@ def _send_telemetry(self, record: TelemetryRecord): - "version": "3.0.2", # Unity MCP version + "version": self.config.version, # Unity MCP version "platform": _platform, "source": _source,Also applies to: 80-83
191-197: Unbounded thread-per-event can exhaust resources; prefer a single worker with a bounded queueHigh-volume emitters will spawn many threads. Consider a background worker and a bounded
queue.Queue, dropping when full.Minimal sketch (illustrative):
+import queue @@ - current_context = contextvars.copy_context() - thread = threading.Thread( - target=lambda: current_context.run(self._send_telemetry, record), - daemon=True - ) - thread.start() + try: + self._queue.put_nowait((contextvars.copy_context(), record)) + except queue.Full: + logger.debug("Telemetry queue full; dropping %s", record.record_type)And initialize one worker:
# in __init__ self._queue: "queue.Queue[tuple[contextvars.Context, TelemetryRecord]]" = queue.Queue(maxsize=1024) threading.Thread(target=self._worker_loop, daemon=True).start() def _worker_loop(self): while True: ctx, rec = self._queue.get() try: ctx.run(self._send_telemetry, rec) finally: self._queue.task_done()
254-257: Preserve traceback on send failuresKeep stack traces for debugging while staying non-fatal.
Apply:
- except Exception as e: - # Never let telemetry errors interfere with app functionality - logger.debug(f"Telemetry send failed: {e}") + except Exception as e: + # Never let telemetry errors interfere with app functionality + logger.debug(f"Telemetry send failed: {e}", exc_info=True)
96-111: Simplify macOS detection
os.uname()is fine on POSIX, butsys.platform == "darwin"is clearer and avoids an unnecessary syscall.Apply:
- elif os.name == 'posix': # macOS/Linux - if 'darwin' in os.uname().sysname.lower(): # macOS + elif os.name == 'posix': # macOS/Linux + if sys.platform == 'darwin': # macOS base_dir = Path.home() / 'Library' / 'Application Support' else: # Linux base_dir = Path(os.environ.get('XDG_DATA_HOME', Path.home() / '.local' / 'share'))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
README.md(1 hunks)UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs(1 hunks)UnityMcpBridge/Editor/MCPForUnityBridge.cs(1 hunks)UnityMcpBridge/UnityMcpServer~/src/telemetry.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(8 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- UnityMcpBridge/Editor/MCPForUnityBridge.cs
🧰 Additional context used
🧬 Code graph analysis (12)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py (2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
telemetry_tool(14-17)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(15-69)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(15-69)UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (1)
_coerce_int(20-42)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py (1)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(15-69)
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/telemetry_decorator.py (1)
telemetry_tool(15-69)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (4)
record_tool_usage(279-290)record_milestone(146-170)record_milestone(275-277)MilestoneType(40-48)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
telemetry_tool(14-17)decorator(15-16)
UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py (2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
telemetry_tool(14-17)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(15-69)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(15-69)UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (1)
_coerce_int(20-42)
UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs (1)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (3)
IsDebugEnabled(47-50)MCPForUnityBridge(110-131)IsAutoConnectMode(62-62)
UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py (5)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(15-69)UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (1)
_coerce_int(57-70)UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (1)
_coerce_int(35-48)UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (1)
_coerce_int(20-42)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(388-407)
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(15-69)UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (1)
_coerce_int(57-70)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
telemetry_tool(15-69)decorator(17-68)UnityMcpBridge/UnityMcpServer~/src/telemetry.py (3)
record_milestone(146-170)record_milestone(275-277)MilestoneType(40-48)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(15-69)UnityMcpBridge/UnityMcpServer~/src/telemetry.py (2)
is_telemetry_enabled(316-318)record_tool_usage(279-290)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py
17-17: Unused function argument: ctx
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py
15-15: Unused function argument: ctx
(ARG001)
47-47: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py
15-15: Unused function argument: ctx
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py
352-352: Unused function argument: ctx
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py
35-35: Consider moving this statement to an else block
(TRY300)
60-60: Consider moving this statement to an else block
(TRY300)
UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py
18-18: Unused function argument: ctx
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py
20-20: Unused function argument: ctx
(ARG001)
69-69: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py
17-17: Unused function argument: ctx
(ARG001)
18-18: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
19-19: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
68-68: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py
40-40: Consider moving this statement to an else block
(TRY300)
41-41: Do not catch blind exception: Exception
(BLE001)
143-143: Unused function argument: ctx
(ARG001)
206-206: Unused function argument: ctx
(ARG001)
345-345: Consider moving this statement to an else block
(TRY300)
346-346: Do not catch blind exception: Exception
(BLE001)
354-354: Unused function argument: ctx
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
14-14: Unused function argument: tool_name
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py
134-134: Do not catch blind exception: Exception
(BLE001)
143-143: Do not catch blind exception: Exception
(BLE001)
239-244: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
246-246: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
254-254: Do not catch blind exception: Exception
(BLE001)
🪛 markdownlint-cli2 (0.17.2)
README.md
299-299: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
300-300: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
301-301: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
🔇 Additional comments (5)
UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs (1)
118-123: LGTM: fixed method invocation forIsAutoConnectMode().This resolves the prior compile-time issue.
UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py (1)
14-16: Decorator order looks correctTelemetry wraps the function first, then the MCP decorator wraps telemetry. Invocation should be captured as intended.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (1)
17-19: Decorator order and async usage look goodTelemetry + async tool wiring is consistent with the rest of the repo.
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (2)
20-42: Good reusable integer coercion helperCovers common edge cases (bools, “null”-likes, float strings) and supports a minimum clamp.
16-16: Telemetry integration looks consistentDecorators applied for list_resources, read_resource, and find_in_file are consistent with the shared telemetry_decorator behavior.
Also applies to: 141-141, 203-203, 350-351
…rt/end positions with 1-based exclusive endCol\n- read_resource: metadata-only default + lengthBytes; selection returns text\n- read_console: strip stacktrace when include_stacktrace=false\n- validate_script: summary counts; get_sha: minimal fields\n- silence stdout in test_telemetry helper
There was a problem hiding this comment.
Actionable comments posted: 2
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/tools/resource_tools.py (1)
179-187: Respect pattern filter; don’t hard-force .cs.The hard suffix check blocks valid non-.cs patterns even when caller specifies a different glob. Rely on pattern alone (default already “*.cs”).
- # Enforce .cs extension regardless of provided pattern - if p.suffix.lower() != ".cs": - continue - if pattern and not fnmatch.fnmatch(p.name, pattern): - continue + # Filter by provided glob (default: *.cs) + if pattern and not fnmatch.fnmatch(p.name, pattern): + continue
♻️ Duplicate comments (2)
UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py (1)
67-90: Restore DISABLE_TELEMETRY after test (prevent state leak).Wrap env change in try/finally and restore original value after the reload and checks.
- # Set environment variable to disable telemetry - os.environ["DISABLE_TELEMETRY"] = "true" - - # Re-import to get fresh config + prev = os.environ.get("DISABLE_TELEMETRY") + os.environ["DISABLE_TELEMETRY"] = "true" + try: + # Re-import to get fresh config import importlib import telemetry importlib.reload(telemetry) - - from telemetry import is_telemetry_enabled, record_telemetry, RecordType - - _ = is_telemetry_enabled() - - if not is_telemetry_enabled(): - pass - - # Test that records are ignored when disabled - record_telemetry(RecordType.USAGE, {"test": "should_be_ignored"}) - pass - - return True - else: - pass - return False + from telemetry import is_telemetry_enabled, record_telemetry, RecordType + _ = is_telemetry_enabled() + if not is_telemetry_enabled(): + record_telemetry(RecordType.USAGE, {"test": "should_be_ignored"}) + return True + else: + return False + finally: + if prev is None: + os.environ.pop("DISABLE_TELEMETRY", None) + else: + os.environ["DISABLE_TELEMETRY"] = prevUnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
574-579: Fix description: remove nonexistent random_string arg.Doc mentions a parameter the function doesn’t accept.
- @mcp.tool(description=( - "Get manage_script capabilities (supported ops, limits, and guards).\n\n" - "Args:\n- random_string: required parameter (any string value)\n\n" - "Returns:\n- ops: list of supported structured ops\n- text_ops: list of supported text ops\n- max_edit_payload_bytes: server edit payload cap\n- guards: header/using guard enabled flag\n" - )) + @mcp.tool(description=( + "Get manage_script capabilities (supported ops, limits, and guards).\n\n" + "Returns:\n- ops: list of supported structured ops\n- text_ops: list of supported text ops\n- max_edit_payload_bytes: server edit payload cap\n- guards: header/using guard enabled flag\n" + ))
🧹 Nitpick comments (20)
UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py (4)
103-114: Make storage test assert something real.Currently it always “passes.” Assert files exist (or fail) to validate persistence.
- # Check if files exist - if collector.config.uuid_file.exists(): - pass - else: - pass - - if collector.config.milestones_file.exists(): - pass - else: - pass - - return True + # Check that expected files were created by collector init + uuid_ok = collector.config.uuid_file.exists() + milestones_ok = collector.config.milestones_file.exists() + return bool(uuid_ok and milestones_ok)
25-27: Avoid blind excepts and unused exception vars.Either log the exception or drop the binding to satisfy linters; prefer narrower exceptions where possible.
- except ImportError as e: + except ImportError: # Silent failure path for tests return FalseExample logging pattern you can replicate elsewhere:
- except Exception as e: - # Silent failure path for tests - return False + except Exception as exc: + print(f"test_telemetry_basic error: {exc}", file=sys.stderr) + return FalseAlso applies to: 39-41, 49-51, 57-59, 116-118, 141-143
1-1: Remove shebang or make file executable.Shebang is unnecessary for a test module and triggers EXE001; remove it or chmod +x.
15-61: Strengthen basic test with minimal assertions.Use explicit checks (e.g., assert VERSION record didn’t raise; FIRST_STARTUP returns a bool) to catch regressions early.
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (6)
391-403: Return all matches per line and include an excerpt.Current code reports only the first match per line and no excerpt. Use finditer and attach the line snippet.
- for i, line in enumerate(lines, start=1): - m = rx.search(line) - if m: - start_col = m.start() + 1 # 1-based - end_col = m.end() + 1 # 1-based, end exclusive - results.append({ - "startLine": i, - "startCol": start_col, - "endLine": i, - "endCol": end_col, - }) - if max_results_int and len(results) >= max_results_int: - break + for i, line in enumerate(lines, start=1): + for m in rx.finditer(line): + results.append({ + "startLine": i, + "startCol": m.start() + 1, # 1-based + "endLine": i, + "endCol": m.end() + 1, # 1-based, end exclusive + "lineExcerpt": line, + }) + if max_results_int and len(results) >= max_results_int: + break + if max_results_int and len(results) >= max_results_int: + break
363-364: Use plain bool for ignore_case.Optional[bool] is misleading here since None is not special-cased.
- ignore_case: Optional[bool] = True, + ignore_case: bool = True,
20-43: Deduplicate _coerce_int across tools.This helper exists in multiple modules; move to a shared utils to avoid drift.
197-204: Doc: clarify selection precedence and default.After reverting default-text behavior, document: precedence head_bytes > tail_lines > start_line/line_count; default returns full text up to CAP with truncation flag.
354-356: Avoid broad except; preserve traceback for debugging.Consider logging or returning a code along with error; or narrow the exception type.
135-167: list_resources limit semantics vs. “always include spec”.Appended “unity://spec/script-edits” may exceed limit; if that’s intentional, consider documenting that it’s excluded from the cap, or include it only when under cap.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
614-618: Align get_sha response with description (include fields when available).Preserve sha256/lengthBytes and pass through optional fields if present.
- if isinstance(resp, dict) and resp.get("success"): - data = resp.get("data", {}) - minimal = {"sha256": data.get("sha256"), "lengthBytes": data.get("lengthBytes")} - return {"success": True, "data": minimal} + if isinstance(resp, dict) and resp.get("success"): + data = resp.get("data", {}) + out = { + "sha256": data.get("sha256"), + "lengthBytes": data.get("lengthBytes"), + } + # Pass through if provided by server + for k in ("lastModifiedUtc", "uri", "path"): + v = data.get(k) + if v is not None: + out[k] = v + return {"success": True, "data": out}
14-17: Silence ARG001 in fallback decorator.Rename unused parameter or annotate to satisfy ruff.
- def telemetry_tool(tool_name: str): + def telemetry_tool(_tool_name: str): # noqa: ARG001 def decorator(func): return func return decoratorUnityMcpBridge/UnityMcpServer~/src/tools/read_console.py (8)
4-9: Tidy imports; add typing for new annotations; drop unused imports.Removes unused time/config/get_unity_connection; prepares for Optional/Union and logging.
-from typing import List, Dict, Any -import time +from typing import Any, Dict, List, Optional, Union +import logging from mcp.server.fastmcp import FastMCP, Context -from unity_connection import get_unity_connection, send_command_with_retry -from config import config +from unity_connection import send_command_with_retry from telemetry_decorator import telemetry_toolAdd a module logger (place after imports):
_log = logging.getLogger(__name__)
16-25: Use explicit Optional annotations and refine count typing; silence unused ctx.Aligns with Ruff RUF013/ARG001 and communicates accepted input types.
- def read_console( - ctx: Context, - action: str = None, - types: List[str] = None, - count: Any = None, - filter_text: str = None, - since_timestamp: str = None, - format: str = None, - include_stacktrace: bool = None - ) -> Dict[str, Any]: + def read_console( + ctx: Context, # noqa: ARG001 + action: Optional[str] = None, + types: Optional[List[str]] = None, + count: Optional[Union[int, float, str]] = None, + filter_text: Optional[str] = None, + since_timestamp: Optional[str] = None, + format: Optional[str] = None, + include_stacktrace: Optional[bool] = None, + ) -> Dict[str, Any]:
42-44: Remove unused connection acquisition; it creates side effects and work.get_unity_connection() is called inside send_command_with_retry, so this local call/variable is redundant.
- # Get the connection instance - bridge = get_unity_connection() + # Connection is created on demand inside send_command_with_retry.
51-54: Normalize types input and support 'all'.Accept a comma-separated string and expand 'all' to the full set to match docstring.
if isinstance(action, str): action = action.lower() + # Normalize types if passed as string or 'all' + if isinstance(types, str): + types = [t.strip() for t in types.split(",") if t.strip()] + if types and "all" in types: + types = ["error", "warning", "log"]
55-70: _coerce_int: clamp negatives and narrow exception; add typing.Prevents negative counts and avoids blind except.
- def _coerce_int(value, default=None): + def _coerce_int(value: Any, default: int | None = None) -> int | None: if value is None: return default try: if isinstance(value, bool): return default if isinstance(value, int): return int(value) s = str(value).strip() if s.lower() in ("", "none", "null"): return default - return int(float(s)) - except Exception: + n = int(float(s)) + return n if n >= 0 else default + except (ValueError, TypeError): return default
84-90: Simplify None-pruning and avoid redundant “add back” step.The comprehension already guarantees presence/absence; the follow-up check is currently redundant.
- # Remove None values unless it's 'count' (as None might mean 'all') - params_dict = {k: v for k, v in params_dict.items() if v is not None or k == 'count'} - - # Add count back if it was None, explicitly sending null might be important for C# logic - if 'count' not in params_dict: - params_dict['count'] = None + # Remove None values; ensure 'count' key is explicit (may be None to signal 'all') + params_dict = {k: v for k, v in params_dict.items() if v is not None} + if "count" not in params_dict: + params_dict["count"] = None
93-101: Don’t swallow exceptions silently; log at debug and narrow errors.Keeps logs clean but debuggable, and satisfies BLE001/S110.
- if isinstance(resp, dict) and resp.get("success") and not include_stacktrace: + if isinstance(resp, dict) and resp.get("success") and not include_stacktrace: # Strip stacktrace fields from returned lines if present try: lines = resp.get("data", {}).get("lines", []) for line in lines: if isinstance(line, dict) and "stacktrace" in line: line.pop("stacktrace", None) - except Exception: - pass + except (KeyError, TypeError, AttributeError) as e: + _log.debug("read_console: failed to strip stacktrace: %s", e)Add at top of file if not present:
_log = logging.getLogger(__name__)
80-80: Nit: avoid shadowing built-in name 'format'.Not urgent, but consider local alias (e.g., output_format) when building params to improve readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(10 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (4)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(15-69)UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py (1)
_coerce_int(56-69)UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (1)
_coerce_int(57-70)UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (1)
_coerce_int(35-48)
UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py (1)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (7)
get_telemetry(262-267)record_telemetry(269-273)record_milestone(146-170)record_milestone(275-277)RecordType(29-38)MilestoneType(40-48)is_telemetry_enabled(316-318)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
telemetry_tool(15-69)decorator(17-68)UnityMcpBridge/UnityMcpServer~/src/telemetry.py (3)
record_milestone(146-170)record_milestone(275-277)MilestoneType(40-48)
UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(15-69)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(388-407)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py
40-40: Consider moving this statement to an else block
(TRY300)
41-41: Do not catch blind exception: Exception
(BLE001)
143-143: Unused function argument: ctx
(ARG001)
206-206: Unused function argument: ctx
(ARG001)
354-354: Do not catch blind exception: Exception
(BLE001)
362-362: Unused function argument: ctx
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py
1-1: Shebang is present but file is not executable
(EXE001)
25-25: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
39-39: Do not catch blind exception: Exception
(BLE001)
39-39: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
49-49: Do not catch blind exception: Exception
(BLE001)
49-49: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
57-57: Do not catch blind exception: Exception
(BLE001)
57-57: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
114-114: Consider moving this statement to an else block
(TRY300)
116-116: Do not catch blind exception: Exception
(BLE001)
116-116: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
141-141: Do not catch blind exception: Exception
(BLE001)
141-141: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
14-14: Unused function argument: tool_name
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py
17-17: Unused function argument: ctx
(ARG001)
18-18: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
19-19: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
68-68: Do not catch blind exception: Exception
(BLE001)
100-101: try-except-pass detected, consider logging the exception
(S110)
100-100: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (10)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (6)
8-18: Guarded telemetry import looks good.No-op decorator fallback keeps runtime stable when telemetry is unavailable.
72-92: Excellent user guidance in apply_text_edits description.Clear, actionable cautions and workflow steps.
473-527: Legacy ‘update’ routing is thoughtful; payload cap check is a nice touch.Good migration path and guardrails.
417-426: Validate allowed levels strictly.Input validation is solid and user-friendly.
1-71: _split_uri normalization looks robust.Covers file://, unity://path, UNC, and Assets-relative paths; good normalization.
433-438: No downstream consumers ofdiagnosticsin production code — tests have been updated to consume only counts and the only other reference is theprune_tool_results.pysummarizer; it’s safe to remove the raw diagnostics array.UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py (4)
9-9: Telemetry import looks good.Importing the shared decorator keeps instrumentation consistent across tools.
47-49: Default changes: verify compatibility.Changing defaults to types=['error','warning','log'] and format='detailed' can alter existing clients’ behavior. Confirm no callers rely on the previous narrower set or 'json' default; if unsure, consider keeping prior defaults or gating via config.
91-93: LGTM: centralizing retries via send_command_with_retry.Matches the established Unity reload handling pattern and keeps tool code lean.
14-16: Ensure telemetry_tool is the outermost decorator
Swap onread_consoleto:- @mcp.tool() - @telemetry_tool("read_console") + @telemetry_tool("read_console") + @mcp.tool()Review all other
@mcp.tool()usages to apply the same ordering for consistent latency capture.
- README path separators (todo in separate doc commit) - manage_gameobject: pop prefabFolder not prefab_folder - execute_menu_item: make sync to avoid blocking event loop - telemetry: validate endpoint scheme (allow http/https only) and re-validate at send time
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (2)
143-160: Split UUID vs. milestones loading; avoid resetting UUID on milestones parse failureA malformed milestones.json currently regenerates the customer UUID and swallows root-cause details. Load UUID and milestones independently, narrow exceptions, and preserve stack traces.
Apply:
def _load_persistent_data(self): - """Load UUID and milestones from disk""" - try: - # Load customer UUID - if self.config.uuid_file.exists(): - self._customer_uuid = self.config.uuid_file.read_text().strip() - else: - self._customer_uuid = str(uuid.uuid4()) - self.config.uuid_file.write_text(self._customer_uuid) - - # Load milestones - if self.config.milestones_file.exists(): - self._milestones = json.loads(self.config.milestones_file.read_text()) - except Exception as e: - logger.warning(f"Failed to load telemetry data: {e}") - self._customer_uuid = str(uuid.uuid4()) - self._milestones = {} + """Load UUID and milestones from disk""" + # Load customer UUID + try: + if self.config.uuid_file.exists(): + self._customer_uuid = self.config.uuid_file.read_text(encoding="utf-8").strip() or str(uuid.uuid4()) + else: + self._customer_uuid = str(uuid.uuid4()) + try: + self.config.uuid_file.write_text(self._customer_uuid, encoding="utf-8") + if os.name == "posix": + os.chmod(self.config.uuid_file, 0o600) + except OSError as e: + logger.debug(f"Failed to persist customer UUID: {e}", exc_info=True) + except OSError as e: + logger.debug(f"Failed to load customer UUID: {e}", exc_info=True) + self._customer_uuid = str(uuid.uuid4()) + + # Load milestones (failure here must not affect UUID) + try: + if self.config.milestones_file.exists(): + content = self.config.milestones_file.read_text(encoding="utf-8") + self._milestones = json.loads(content) or {} + if not isinstance(self._milestones, dict): + self._milestones = {} + except (OSError, json.JSONDecodeError, ValueError) as e: + logger.debug(f"Failed to load milestones: {e}", exc_info=True) + self._milestones = {}
137-142: Protect milestone writes with a lock to prevent double-recording under concurrencyTwo threads can record the same “first” milestone. Guard the check+write with a lock.
class TelemetryCollector: @@ def __init__(self): self.config = TelemetryConfig() self._customer_uuid: Optional[str] = None self._milestones: Dict[str, Dict[str, Any]] = {} + self._lock = threading.Lock() self._load_persistent_data() @@ def record_milestone(self, milestone: MilestoneType, data: Optional[Dict[str, Any]] = None) -> bool: @@ - milestone_key = milestone.value - if milestone_key in self._milestones: - return False # Already recorded - - milestone_data = { - "timestamp": time.time(), - "data": data or {} - } - - self._milestones[milestone_key] = milestone_data - self._save_milestones() + milestone_key = milestone.value + with self._lock: + if milestone_key in self._milestones: + return False # Already recorded + milestone_data = { + "timestamp": time.time(), + "data": data or {}, + } + self._milestones[milestone_key] = milestone_data + self._save_milestones()Also applies to: 168-191
🧹 Nitpick comments (7)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (4)
161-166: Narrow exception type and write with UTF-8; include trace for diagnosticsUse UTF-8 and catch only I/O errors; prefer debug with stack.
def _save_milestones(self): @@ - try: - self.config.milestones_file.write_text(json.dumps(self._milestones, indent=2)) - except Exception as e: - logger.warning(f"Failed to save milestones: {e}") + try: + self.config.milestones_file.write_text( + json.dumps(self._milestones, indent=2), + encoding="utf-8", + ) + except OSError as e: + logger.debug(f"Failed to save milestones: {e}", exc_info=True)
249-259: Minor: prefer HAS_HTTPX flag and include exception trace on send failureSmall robustness/readability tweaks.
- if httpx: + if HAS_HTTPX: with httpx.Client(timeout=self.config.timeout) as client: @@ - except Exception as e: + except Exception as e: # Never let telemetry errors interfere with app functionality - logger.debug(f"Telemetry send failed: {e}") + logger.debug(f"Telemetry send failed: {e}", exc_info=True)Also applies to: 279-282
241-244: Avoid hard-coding versionReading the MCP version from a single source (e.g., config or package metadata) prevents drift.
287-293: Optional: thread-safe singleton initVery low risk, but a module-level lock avoids double-initialization under a thundering herd.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py (2)
115-125: Validate prefabPath to prevent traversal and enforce Assets/ prefixTighten path rules when constructing/accepting prefab paths.
if action == "create" and params.get("saveAsPrefab"): # Check if 'saveAsPrefab' is explicitly True in params if "prefabPath" not in params: if "name" not in params or not params["name"]: return {"success": False, "message": "Cannot create default prefab path: 'name' parameter is missing."} # Use the provided prefab_folder (which has a default) and the name to construct the path constructed_path = f"{prefab_folder}/{params['name']}.prefab" # Ensure clean path separators (Unity prefers '/') params["prefabPath"] = constructed_path.replace("\\", "/") elif not params["prefabPath"].lower().endswith(".prefab"): return {"success": False, "message": f"Invalid prefab_path: '{params['prefabPath']}' must end with .prefab"} + # Enforce 'Assets/' prefix and no traversal + normalized = params["prefabPath"].replace("\\", "/") + if not normalized.startswith("Assets/") or ".." in normalized or normalized.startswith("/"): + return {"success": False, "message": "Invalid prefabPath. Must start with 'Assets/' and contain no path traversal."} + params["prefabPath"] = normalized
15-16: ctx is unusedIf allowed by the MCP API, consider
_ctx: Anyto silence ARG001; otherwise keep as-is.UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py (1)
18-18: ctx is unusedIf permissible, rename to
_ctx: Anyto appease linters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(15-69)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
telemetry_tool(14-17)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(15-69)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
telemetry_tool(14-17)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py
122-122: Abstract raise to an inner function
(TRY301)
122-122: Avoid specifying long messages outside the exception class
(TRY003)
125-125: Abstract raise to an inner function
(TRY301)
125-125: Avoid specifying long messages outside the exception class
(TRY003)
126-126: Consider moving this statement to an else block
(TRY300)
127-127: Do not catch blind exception: Exception
(BLE001)
156-156: Do not catch blind exception: Exception
(BLE001)
165-165: Do not catch blind exception: Exception
(BLE001)
264-269: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
271-271: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
279-279: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py
18-18: Unused function argument: ctx
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py
15-15: Unused function argument: ctx
(ARG001)
🔇 Additional comments (5)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (1)
115-133: Good: endpoint scheme validation blocks non-HTTP schemesThe
_validated_endpointhelper correctly constrains schemes and falls back safely. This addresses SSRF/local-file risks flagged earlier.UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py (2)
12-16: Decorator order is correct
@telemetry_toolwraps the function before registration via@mcp.tool(), ensuring telemetry covers tool execution.
125-128: Confirm C# contract for prefabFolder omissionYou always drop
prefabFolder. Verify the C# handler never expects it whensaveAsPrefabis False or for other actions.UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py (2)
15-17: Decorator order is correct
@telemetry_toolwraps before registration, so telemetry captures execution.
15-23: No changes needed: FastMCP supports both sync and async tool functions (mcpcat.io, the-pocket.github.io)
…oad\n\n- TelemetryConfig reads config.telemetry_enabled/endpoint, env can override\n- Validate endpoint scheme; revalidate on send\n- Split UUID/milestones load error handling\n- Add tests for config precedence, scheme validation, UUID preservation\n- validate_script: optional include_diagnostics with documented behavior
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
604-608: Docstring/return mismatch for get_sha.Description promises more fields than returned. Align docs or return the extra fields.
- @mcp.tool(description=( - "Get SHA256 and metadata for a Unity C# script without returning file contents.\n\n" - "Args: uri (unity://path/Assets/... or file://... or Assets/...).\n" - "Returns: {sha256, lengthBytes, lastModifiedUtc, uri, path}." - )) + @mcp.tool(description=( + "Get SHA256 and basic metadata for a Unity C# script without returning file contents.\n\n" + "Args: uri (unity://path/Assets/... or file://... or Assets/...).\n" + "Returns: {sha256, lengthBytes}." + ))If you prefer returning the richer shape, I can adjust the implementation to include lastModifiedUtc/uri/path when available.
Also applies to: 618-620
♻️ Duplicate comments (1)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (1)
198-223: Protect milestone writes with a lock (fix race/double-recording).Concurrent calls can observe empty _milestones and both write, causing duplicate "first" milestones and racy file writes.
Apply:
@@ class TelemetryCollector: @@ def __init__(self): self.config = TelemetryConfig() self._customer_uuid: Optional[str] = None self._milestones: Dict[str, Dict[str, Any]] = {} + self._lock = threading.Lock() self._load_persistent_data() @@ def record_milestone(self, milestone: MilestoneType, data: Optional[Dict[str, Any]] = None) -> bool: """Record a milestone event, returns True if this is the first occurrence""" if not self.config.enabled: return False - - milestone_key = milestone.value - if milestone_key in self._milestones: - return False # Already recorded - - milestone_data = { - "timestamp": time.time(), - "data": data or {} - } - - self._milestones[milestone_key] = milestone_data - self._save_milestones() + milestone_key = milestone.value + with self._lock: + if milestone_key in self._milestones: + return False # Already recorded + milestone_data = { + "timestamp": time.time(), + "data": data or {}, + } + self._milestones[milestone_key] = milestone_data + self._save_milestones()Also applies to: 156-161
🧹 Nitpick comments (6)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (4)
243-249: Avoid unbounded thread creation; use a bounded worker/queue.Spawning a thread per record can overwhelm processes under bursty load; prefer a single background worker with Queue or a ThreadPoolExecutor.
If helpful, I can provide a minimal Queue+worker patch.
309-313: Preserve stack traces on send failures.Add exc_info to debug log to aid investigation while remaining non-fatal.
- logger.debug(f"Telemetry send failed: {e}") + logger.debug(f"Telemetry send failed: {e}", exc_info=True)
317-323: Make get_telemetry initialization thread-safe.Minor, but two threads could construct two collectors. Guard with a module-level lock.
# Global telemetry instance _telemetry_collector: Optional[TelemetryCollector] = None +_telemetry_lock = threading.Lock() def get_telemetry() -> TelemetryCollector: """Get the global telemetry collector instance""" global _telemetry_collector - if _telemetry_collector is None: - _telemetry_collector = TelemetryCollector() + if _telemetry_collector is None: + with _telemetry_lock: + if _telemetry_collector is None: + _telemetry_collector = TelemetryCollector() return _telemetry_collectorAlso applies to: 314-316
265-274: Avoid hardcoded version; source from a single place.Hardcoding "3.0.2" risks drift. Import from a version module or server config.
I can wire this to UnityMcpServer version or a version constant on request.
tests/test_telemetry_endpoint_validation.py (1)
1-12: Optional: add a positive http/https acceptance case.Add a test that a valid https URL is accepted unchanged to complement the rejection test.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
8-17: Silence linter for no-op decorator argument.Rename unused param to underscore to satisfy ARG001.
- def telemetry_tool(tool_name: str): + def telemetry_tool(_tool_name: str): def decorator(func): return func return decorator
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(10 hunks)tests/test_telemetry_endpoint_validation.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_telemetry_endpoint_validation.py (1)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (1)
TelemetryCollector(153-311)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
telemetry_tool(15-69)decorator(17-68)UnityMcpBridge/UnityMcpServer~/src/telemetry.py (3)
record_milestone(198-222)record_milestone(330-332)MilestoneType(42-50)
🪛 Ruff (0.12.2)
tests/test_telemetry_endpoint_validation.py
14-14: Use of assert detected
(S101)
30-30: Use of assert detected
(S101)
36-36: Use of assert detected
(S101)
55-55: Use of assert detected
(S101)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
14-14: Unused function argument: tool_name
(ARG001)
419-419: Unused function argument: ctx
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py
78-79: try-except-continue detected, consider logging the exception
(S112)
78-78: Do not catch blind exception: Exception
(BLE001)
141-141: Abstract raise to an inner function
(TRY301)
141-141: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Abstract raise to an inner function
(TRY301)
144-144: Avoid specifying long messages outside the exception class
(TRY003)
145-145: Consider moving this statement to an else block
(TRY300)
146-146: Do not catch blind exception: Exception
(BLE001)
195-195: Do not catch blind exception: Exception
(BLE001)
294-299: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
301-301: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
309-309: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (5)
tests/test_telemetry_endpoint_validation.py (3)
4-15: LGTM: endpoint scheme validation fallback is covered.Good isolation via XDG_DATA_HOME; verifies file:// is rejected and default used.
16-39: Nice precedence test (config vs env).Covers expected override order and restores config in finally.
40-56: UUID continuity test is on point.Confirms malformed milestones don’t reset UUID after the loader refactor.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
93-101: Telemetry decorator application looks good.Non-invasive, guarded, and keeps tool signature intact.
436-440: validate_script: useful summary with optional full diagnostics.Clear contract; severity filters look correct.
…reads version file; locks for milestones
There was a problem hiding this comment.
Actionable comments posted: 2
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/tools/manage_script.py (1)
372-381: Fix path traversal: validate normalized path and derive name/dir from it.Current checks use unnormalized directory and can allow Assets/../ escapes. Normalize first, then validate and use normalized components.
- name = os.path.splitext(os.path.basename(path))[0] - directory = os.path.dirname(path) - # Local validation to avoid round-trips on obviously bad input - norm_path = os.path.normpath((path or "").replace("\\", "/")).replace("\\", "/") - if not directory or directory.split("/")[0].lower() != "assets": + # Local validation to avoid round-trips on obviously bad input + norm_path = os.path.normpath((path or "").replace("\\", "/")).replace("\\", "/") + # Recompute components from normalized path to prevent traversal tricks + name = os.path.splitext(os.path.basename(norm_path))[0] + directory = os.path.dirname(norm_path) + if not directory or directory.split("/")[0].lower() != "assets": return {"success": False, "code": "path_outside_assets", "message": f"path must be under 'Assets/'; got '{path}'."} - if ".." in norm_path.split("/") or norm_path.startswith("/"): + if ".." in norm_path.split("/") or norm_path.startswith("/"): return {"success": False, "code": "bad_path", "message": "path must not contain traversal or be absolute."}
♻️ Duplicate comments (1)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (1)
192-202: Milestone writes are now serialized and persisted safely — nice fixLocking around writes and narrowing exception handling address concurrency and diagnostics concerns.
Also applies to: 203-218
🧹 Nitpick comments (19)
UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs (3)
25-46: Use culture-invariant checks for env vars; simplify boolean parsing.Avoid ToLower() and use OrdinalIgnoreCase for correctness and micro-alloc savings.
- var envDisable = Environment.GetEnvironmentVariable("DISABLE_TELEMETRY"); - if (!string.IsNullOrEmpty(envDisable) && - (envDisable.ToLower() == "true" || envDisable == "1")) + var envDisable = Environment.GetEnvironmentVariable("DISABLE_TELEMETRY"); + if (!string.IsNullOrEmpty(envDisable) && + (envDisable.Equals("true", StringComparison.OrdinalIgnoreCase) || envDisable == "1")) { return false; } var unityMcpDisable = Environment.GetEnvironmentVariable("UNITY_MCP_DISABLE_TELEMETRY"); - if (!string.IsNullOrEmpty(unityMcpDisable) && - (unityMcpDisable.ToLower() == "true" || unityMcpDisable == "1")) + if (!string.IsNullOrEmpty(unityMcpDisable) && + (unityMcpDisable.Equals("true", StringComparison.OrdinalIgnoreCase) || unityMcpDisable == "1")) { return false; }
47-49: Guard EditorPrefs access so telemetry never breaks editor workflows.Wrap EditorPrefs calls with try/catch and safe fallbacks, mirroring IsDebugEnabled().
- return !UnityEditor.EditorPrefs.GetBool(TELEMETRY_DISABLED_KEY, false); + try { return !UnityEditor.EditorPrefs.GetBool(TELEMETRY_DISABLED_KEY, false); } + catch { return true; } @@ - var uuid = UnityEditor.EditorPrefs.GetString(CUSTOMER_UUID_KEY, ""); - if (string.IsNullOrEmpty(uuid)) - { - uuid = System.Guid.NewGuid().ToString(); - UnityEditor.EditorPrefs.SetString(CUSTOMER_UUID_KEY, uuid); - } - return uuid; + try + { + var uuid = UnityEditor.EditorPrefs.GetString(CUSTOMER_UUID_KEY, ""); + if (string.IsNullOrEmpty(uuid)) + { + uuid = Guid.NewGuid().ToString(); + UnityEditor.EditorPrefs.SetString(CUSTOMER_UUID_KEY, uuid); + } + return uuid; + } + catch + { + return Guid.NewGuid().ToString(); + } @@ - UnityEditor.EditorPrefs.SetBool(TELEMETRY_DISABLED_KEY, true); + try { UnityEditor.EditorPrefs.SetBool(TELEMETRY_DISABLED_KEY, true); } catch { /* ignore */ } @@ - UnityEditor.EditorPrefs.SetBool(TELEMETRY_DISABLED_KEY, false); + try { UnityEditor.EditorPrefs.SetBool(TELEMETRY_DISABLED_KEY, false); } catch { /* ignore */ }Also applies to: 55-64, 69-80
137-139: Avoid hard-coded bridge_version; source from a single authority.3.0.2 here vs server-version.txt (Python side) risks drift. Consider wiring a shared version constant or passing version from the server when registering the sender.
Would you like a follow-up patch to surface a BridgeVersion from MCPForUnityBridge (or read server-version.txt once and cache)?
UnityMcpBridge/UnityMcpServer~/src/server.py (4)
52-53: Use perf_counter for durations (consistent, monotonic).Compute connection_time_ms using the same monotonic clock used on failures.
- "connection_time_ms": (time.time() - start_time) * 1000 + "connection_time_ms": (time.perf_counter() - start_clk) * 1000
35-36: Narrow exception when reading version file.Catching OSError/FileNotFoundError is clearer and satisfies BLE001.
- except Exception: + except OSError: server_version = "unknown"
65-65: Document broad catch or suppress BLE001.If you intend to isolate unexpected startup errors to keep the server alive, annotate the catch.
- except Exception as e: + except Exception as e: # noqa: BLE001 - isolate unexpected startup errors, record telemetry, continue
1-1: Remove unused imports.Image and List are unused.
-from mcp.server.fastmcp import FastMCP, Context, Image +from mcp.server.fastmcp import FastMCP, Context @@ -from typing import AsyncIterator, Dict, Any, List +from typing import AsyncIterator, Dict, AnyAlso applies to: 5-5
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (3)
8-17: Silence ARG001 in telemetry fallback by marking the arg unused.Minor linter noise when telemetry is unavailable.
-try: +try: from telemetry_decorator import telemetry_tool from telemetry import record_milestone, MilestoneType HAS_TELEMETRY = True except ImportError: HAS_TELEMETRY = False - def telemetry_tool(tool_name: str): + def telemetry_tool(_tool_name: str): def decorator(func): return func return decorator
288-291: Remove unused import in debug preview.difflib isn’t used.
- try: - import difflib + try: # Apply locally to preview final result lines = []
419-420: Optionally mark ctx as used to satisfy linters.If keeping ctx for signature parity, assign to underscore or rename to _ctx.
- def validate_script( - ctx: Context, uri: str, level: str = "basic", include_diagnostics: bool = False + def validate_script( + ctx: Context, uri: str, level: str = "basic", include_diagnostics: bool = False ) -> Dict[str, Any]: + _ = ctx # keep signature; silence ARG001UnityMcpBridge/UnityMcpServer~/src/telemetry.py (9)
17-28: Prune unused imports and drop unused HAS_HTTPX flagRemove
asdict,List, and the unusedHAS_HTTPXvariable to keep imports clean.-from dataclasses import dataclass, asdict -from typing import Optional, Dict, Any, List +from dataclasses import dataclass +from typing import Optional, Dict, Any @@ try: import httpx - HAS_HTTPX = True except ImportError: httpx = None # type: ignore - HAS_HTTPX = False
66-80: Narrow exception scope when probing server config and log missesCatching all exceptions hides genuine errors (e.g., attribute errors in config). Catch
ImportErroronly and log at debug for traceability.for modname in ( @@ - try: - mod = importlib.import_module(modname) - server_config = getattr(mod, "config", None) - if server_config is not None: - break - except Exception: - continue + try: + mod = importlib.import_module(modname) + server_config = getattr(mod, "config", None) + if server_config is not None: + break + except ImportError as e: + logger.debug(f"Config import failed for {modname}: {e}") + continue
118-133: Harden permissions of the telemetry data directory on POSIXSet
0700on the directory (in addition to file0600) to reduce leakage via world-readable dirs.data_dir = base_dir / 'UnityMCP' - data_dir.mkdir(parents=True, exist_ok=True) + data_dir.mkdir(parents=True, exist_ok=True) + if os.name == "posix": + try: + os.chmod(data_dir, 0o700) + except OSError: + logger.debug("Failed to chmod data_dir to 0700", exc_info=True) return data_dir
134-152: Tighten endpoint validation: HTTPS everywhere; HTTP only for localhostPermit
http://only for loopback to avoid downgrade/mitm in production; also narrow the catch block.- def _validated_endpoint(self, candidate: str, fallback: str) -> str: + def _validated_endpoint(self, candidate: str, fallback: str) -> str: """Validate telemetry endpoint URL scheme; allow only http/https. Falls back to the provided default on error. """ - try: - parsed = urlparse(candidate) - if parsed.scheme not in ("https", "http"): - raise ValueError(f"Unsupported scheme: {parsed.scheme}") - # Basic sanity: require network location and path - if not parsed.netloc: - raise ValueError("Missing netloc in endpoint") - return candidate - except Exception as e: + try: + parsed = urlparse(candidate) + if parsed.scheme == "https": + pass + elif parsed.scheme == "http": + if parsed.hostname not in {"localhost", "127.0.0.1", "::1"}: + raise ValueError("Plain HTTP only allowed for localhost") + else: + raise ValueError(f"Unsupported scheme: {parsed.scheme}") + if not parsed.netloc: + raise ValueError("Missing netloc in endpoint") + return candidate + except ValueError as e: logger.debug( f"Invalid telemetry endpoint '{candidate}', using default. Error: {e}", exc_info=True, ) return fallback
166-179: Validate UUID format when loading; regenerate and persist if invalidPrevents arbitrary/empty content in
customer_uuid.txt.- if self.config.uuid_file.exists(): - self._customer_uuid = self.config.uuid_file.read_text(encoding="utf-8").strip() or str(uuid.uuid4()) + if self.config.uuid_file.exists(): + raw = self.config.uuid_file.read_text(encoding="utf-8").strip() + try: + self._customer_uuid = str(uuid.UUID(raw)) + except Exception: + self._customer_uuid = str(uuid.uuid4()) + try: + self.config.uuid_file.write_text(self._customer_uuid, encoding="utf-8") + if os.name == "posix": + os.chmod(self.config.uuid_file, 0o600) + except OSError as e: + logger.debug(f"Failed to persist regenerated UUID: {e}", exc_info=True)
283-292: Treat all 2xx as success for httpx path (aligns with urllib path)Consistency and fewer false negatives on 201/204 etc.
- if response.status_code == 200: + if 200 <= response.status_code < 300: logger.debug(f"Telemetry sent: {record.record_type}") else: logger.debug(f"Telemetry failed: HTTP {response.status_code}")
312-315: Include stack trace on send failures for diagnosisKeep failures non-fatal but logged with context.
- except Exception as e: + except Exception as e: # Never let telemetry errors interfere with app functionality - logger.debug(f"Telemetry send failed: {e}") + logger.debug(f"Telemetry send failed: {e}", exc_info=True)
317-326: Make singleton initialization thread-safePrevent double-initialization under concurrent access.
_telemetry_collector: Optional[TelemetryCollector] = None +_collector_lock = threading.Lock() @@ def get_telemetry() -> TelemetryCollector: """Get the global telemetry collector instance""" global _telemetry_collector - if _telemetry_collector is None: - _telemetry_collector = TelemetryCollector() + if _telemetry_collector is None: + with _collector_lock: + if _telemetry_collector is None: + _telemetry_collector = TelemetryCollector() return _telemetry_collector
268-277: Use dynamic version resolution instead of hard-coding
server-version.txt (src/server-version.txt) contains3.3.0, so the hard-coded"3.0.2"is out of date. In telemetry.py, replace:- "version": "3.0.2", + "version": _resolve_version(),and add the
_resolve_version()helper (at top-level) to read from importlib.metadata or server-version.txt as originally proposed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs(1 hunks)UnityMcpBridge/UnityMcpServer~/src/server-version.txt(1 hunks)UnityMcpBridge/UnityMcpServer~/src/server.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/telemetry.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(10 hunks)tests/conftest.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/conftest.py
- UnityMcpBridge/UnityMcpServer~/src/server-version.txt
🧰 Additional context used
🧬 Code graph analysis (3)
UnityMcpBridge/UnityMcpServer~/src/server.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (5)
record_telemetry(327-331)record_milestone(203-225)record_milestone(333-335)RecordType(31-40)MilestoneType(42-50)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
get_unity_connection(351-371)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
telemetry_tool(15-69)decorator(17-68)UnityMcpBridge/UnityMcpServer~/src/telemetry.py (3)
record_milestone(203-225)record_milestone(333-335)MilestoneType(42-50)
UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs (1)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (3)
IsDebugEnabled(47-50)MCPForUnityBridge(110-131)IsAutoConnectMode(62-62)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/server.py
35-35: Do not catch blind exception: Exception
(BLE001)
65-65: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
14-14: Unused function argument: tool_name
(ARG001)
419-419: Unused function argument: ctx
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py
78-79: try-except-continue detected, consider logging the exception
(S112)
78-78: Do not catch blind exception: Exception
(BLE001)
141-141: Abstract raise to an inner function
(TRY301)
141-141: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Abstract raise to an inner function
(TRY301)
144-144: Avoid specifying long messages outside the exception class
(TRY003)
145-145: Consider moving this statement to an else block
(TRY300)
146-146: Do not catch blind exception: Exception
(BLE001)
297-302: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
304-304: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
312-312: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
434-441: Return shape looks good; counts and optional diagnostics are clear.Nice balance of summary vs. detail controlled by include_diagnostics.
- Logging to stderr with force; quiet httpx/urllib3 - Async lifespan fix; defer telemetry in first second - Bounded telemetry queue with single worker - Reduce initial Unity connect timeout to 1s - Keep server_version in file
…int to Cloud Run; add unit test for backpressure
…otating file logs; Cloud Run endpoint in config; minor robustness in scene tool
There was a problem hiding this comment.
Actionable comments posted: 9
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/tools/manage_scene.py (1)
68-69: Telemetry success may be misreported due to swallowed exceptions.Catching all exceptions here prevents the telemetry decorator from seeing failures; it will record
success=Trueeven when you return{"success": False, ...}. Either let exceptions propagate or teach the decorator to infer success from the returned dict.Option A (propagate): remove the try/except; allow the decorator to record failures via exceptions, and let the MCP layer shape errors.
Option B (keep shaping, preferred): update the decorator to treat
{"success": False}as a failure:diff --git a/UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py b/UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py @@ def telemetry_tool(tool_name: str): - result = func(*args, **kwargs) - success = True + result = func(*args, **kwargs) + success = True + # Treat conventional tool payloads with explicit success=False as failures + if isinstance(result, dict) and result.get("success") is False: + success = False if tool_name == "manage_script" and kwargs.get("action") == "create": record_milestone(MilestoneType.FIRST_SCRIPT_CREATION) @@ - result = await func(*args, **kwargs) - success = True + result = await func(*args, **kwargs) + success = True + if isinstance(result, dict) and result.get("success") is False: + success = FalseIf you must keep the broad catch here, at least silence the lint until Option B lands:
- except Exception as e: + except Exception as e: # noqa: BLE001
♻️ Duplicate comments (1)
UnityMcpBridge/UnityMcpServer~/src/config.py (1)
37-39: Avoid duplicating telemetry config; wire or remove these fields.It looks like TelemetryCollector/TelemetryConfig manage
enabledand the endpoint internally. If these ServerConfig fields aren’t read there, this is dead config and can drift.Actions:
- Either plumb ServerConfig.telemetry_* into TelemetryConfig construction, or remove them and keep a single source of truth (TelemetryConfig + env overrides).
🧹 Nitpick comments (20)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (4)
14-20: Silence ARG001 or make intent explicit.
ctxis required by the tool signature but unused. Either rename to_ctxor add an inline ignore.Apply:
- ctx: Context, + ctx: Context, # noqa: ARG001
19-19: Tighten typing for better schema and editor help.Use
Optional[int]instead ofAnyto preserve a numeric contract while still allowing coercion.Apply:
- build_index: Any, + build_index: Optional[int] = None,And update imports:
-from typing import Dict, Any +from typing import Dict, Any, Optional
34-49: DRY: reuse the shared coercion helper from tools/resource_tools.This local
_coerce_intduplicates the existing implementation (which also supports clamping). Import and reuse the shared helper instead.Apply (remove the local helper):
- # Coerce numeric inputs defensively - def _coerce_int(value, default=None): - if value is None: - return default - try: - if isinstance(value, bool): - return default - if isinstance(value, int): - return int(value) - s = str(value).strip() - if s.lower() in ("", "none", "null"): - return default - return int(float(s)) - except Exception: - return defaultAdd import at top (adjust path if package layout differs):
from tools.resource_tools import _coerce_int
50-50: Clamp build index to non-negative.When using the shared helper, clamp to
minimum=0to avoid accidental negatives.- build_index = _coerce_int(build_index, default=0) + build_index = _coerce_int(build_index, default=0, minimum=0)UnityMcpBridge/Editor/MCPForUnityBridge.cs (3)
79-80: Record startup telemetry — consider also logging first successful connection.
Startup event is good. To close the funnel, consider emitting a single success event (once) after the first successful client handshake to complement the failure path below.Would you like me to wire a one-time TelemetryHelper.RecordBridgeConnection(true) after the handshake write succeeds in HandleClientAsync?
113-115: Main-thread ID capture — add a defensive fallback.
Edge case: if mainThreadId were ever 0, InvokeOnMainThreadWithTimeout could block the editor waiting on delayCall. I proposed a guard in that method below to avoid hangs.
545-576: Per-command timeout path: add machine-readable error codes.
Enrich errors with a stable code to simplify client handling.Apply this diff:
- var timeoutResponse = new - { - status = "error", - error = $"Command processing timed out after {FrameIOTimeoutMs} ms", - }; + var timeoutResponse = new + { + status = "error", + error = $"Command processing timed out after {FrameIOTimeoutMs} ms", + code = "timeout", + }; ... - var errorResponse = new - { - status = "error", - error = ex.Message, - }; + var errorResponse = new + { + status = "error", + error = ex.Message, + code = "internal_error", + };UnityMcpBridge/UnityMcpServer~/src/config.py (1)
18-18: 1s default connection timeout is aggressive—verify end-to-end and fix the misleading comment.
- 1.0s may spuriously fail on cold/slow editors or CI; confirm retries/backoff cover this.
- Comment says “retries use shorter timeouts,” which reads contradictory. If that’s not true, update it.
Proposed comment tweak:
- connection_timeout: float = 1.0 # short initial timeout; retries use shorter timeouts + connection_timeout: float = 1.0 # short connect/IO timeout; retries/backoff handle slow startstests/test_telemetry_queue_worker.py (3)
41-41: Drop unused fixture to satisfy linters.
monkeypatchisn’t used.Apply:
-def test_telemetry_queue_backpressure_and_single_worker(monkeypatch, caplog): +def test_telemetry_queue_backpressure_and_single_worker(caplog):
53-57: Silence ARG001 and make the slow sender explicit.Rename unused params to underscore-prefixed to appease Ruff; behavior unchanged.
- def slow_send(self, rec): + def slow_send(_self, _rec): time.sleep(0.05)
65-66: Deflake: relax the timing threshold.80 ms for 50 calls can be tight on CI. Bump to reduce flakes.
- assert elapsed_ms < 80.0 + assert elapsed_ms < 250.0UnityMcpBridge/UnityMcpServer~/src/server.py (1)
22-40: Rotate-file logging path is macOS-specific; prefer platform-aware dirs and avoid swallow-all except.Current path under
~/Library/Application Support/...won’t exist on Linux/Windows. Also the bareexcept: passhides useful diagnostics.-try: - import os as _os - _log_dir = _os.path.join(_os.path.expanduser("~/Library/Application Support/UnityMCP"), "Logs") - _os.makedirs(_log_dir, exist_ok=True) +try: + from pathlib import Path + import platform + base = Path.home() + if platform.system() == "Darwin": + log_root = base / "Library" / "Application Support" / "UnityMCP" + elif platform.system() == "Windows": + log_root = Path(os.environ.get("LOCALAPPDATA", base)) / "UnityMCP" + else: + log_root = base / ".local" / "share" / "UnityMCP" + _log_dir = log_root / "Logs" + _log_dir.mkdir(parents=True, exist_ok=True) - _file_path = _os.path.join(_log_dir, "unity_mcp_server.log") + _file_path = str(_log_dir / "unity_mcp_server.log") @@ - logger.addHandler(_fh) -except Exception: - # Never let logging setup break startup - pass + logger.addHandler(_fh) +except Exception as e: + # Never let logging setup break startup + logger.debug("File logging disabled: %s", e)UnityMcpBridge/UnityMcpServer~/src/telemetry.py (8)
201-206: Harden milestones file permissions (POSIX).The UUID file is chmod 600; do the same for milestones.
self.config.milestones_file.write_text( json.dumps(self._milestones, indent=2), encoding="utf-8", ) + if os.name == "posix": + os.chmod(self.config.milestones_file, 0o600)
148-153: Narrow exception in _validated_endpoint.Catching ValueError is sufficient here; avoids “blind except” warnings.
- except Exception as e: + except ValueError as e: logger.debug( f"Invalid telemetry endpoint '{candidate}', using default. Error: {e}", exc_info=True, ) return fallback
75-82: Log config import errors at debug instead of silent continue.Helps diagnose misconfig without polluting logs.
- except Exception: - continue + except Exception as e: + logger.debug("Failed to import telemetry config from %s: %s", modname, e, exc_info=True) + continue
267-269: Scope suppression to ValueError around task_done.task_done only raises ValueError on underflow; suppressing all Exceptions can mask bugs.
- with contextlib.suppress(Exception): + with contextlib.suppress(ValueError): self._queue.task_done()
336-341: Guard singleton creation against races.Two threads calling get_telemetry() simultaneously can create two instances.
_telemetry_collector: Optional[TelemetryCollector] = None +_collector_lock = threading.Lock() def get_telemetry() -> TelemetryCollector: """Get the global telemetry collector instance""" - global _telemetry_collector - if _telemetry_collector is None: - _telemetry_collector = TelemetryCollector() + global _telemetry_collector + if _telemetry_collector is None: + with _collector_lock: + if _telemetry_collector is None: + _telemetry_collector = TelemetryCollector() return _telemetry_collector
258-266: Provide a graceful shutdown for the worker.Daemon threads exit abruptly; add a stop signal and join for clean teardown (tests, short-lived CLIs).
Example patch:
class TelemetryCollector: @@ - self._worker: threading.Thread = threading.Thread(target=self._worker_loop, daemon=True) + self._stop = threading.Event() + self._worker: threading.Thread = threading.Thread(target=self._worker_loop, daemon=True) self._worker.start() @@ def _worker_loop(self): """Background worker that serializes telemetry sends.""" - while True: - ctx, rec = self._queue.get() + while True: + ctx, rec = self._queue.get() + if rec is None or self._stop.is_set(): + self._queue.task_done() + break try: ctx.run(self._send_telemetry, rec) @@ with contextlib.suppress(ValueError): self._queue.task_done() + + def shutdown(self, timeout: float = 2.0): + """Signal the worker to stop and wait briefly.""" + self._stop.set() + try: + self._queue.put_nowait((contextvars.copy_context(), None)) # sentinel + except Exception: + pass + self._worker.join(timeout=timeout)
284-293: Avoid hard-coded version; resolve from config or package metadata.Prevents drift when releases bump.
Example:
from importlib.metadata import PackageNotFoundError, version as pkg_version def _resolve_version(default: str = "unknown") -> str: try: return pkg_version("unity-mcp") except PackageNotFoundError: return os.environ.get("UNITY_MCP_VERSION", default)Then set payload["version"] = _resolve_version().
280-283: Ensure JSON-serializable payload data.Non-serializable values in data will raise during httpx JSON encoding; coerce with default=str.
def _json_safe(obj: Any) -> Any: return json.loads(json.dumps(obj, default=str)) # ... enriched_data = _json_safe(dict(record.data or {}))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
UnityMcpBridge/Editor/MCPForUnityBridge.cs(6 hunks)UnityMcpBridge/UnityMcpServer~/src/config.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/server.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/server_version.txt(1 hunks)UnityMcpBridge/UnityMcpServer~/src/telemetry.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py(2 hunks)tests/test_telemetry_queue_worker.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- UnityMcpBridge/UnityMcpServer~/src/server_version.txt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
PR: CoplayDev/unity-mcp#260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
Applied to files:
UnityMcpBridge/Editor/MCPForUnityBridge.cs
🧬 Code graph analysis (4)
tests/test_telemetry_queue_worker.py (1)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (4)
TelemetryCollector(155-330)_send_telemetry(270-330)record(233-256)RecordType(33-42)
UnityMcpBridge/UnityMcpServer~/src/server.py (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
get_unity_connection(351-371)UnityConnection(30-346)UnityMcpBridge/UnityMcpServer~/src/telemetry.py (5)
record_telemetry(343-347)record_milestone(209-231)record_milestone(349-351)RecordType(33-42)MilestoneType(44-52)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (3)
UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs (3)
TelemetryHelper(11-217)RecordBridgeStartup(133-140)RecordBridgeConnection(145-158)UnityMcpBridge/Editor/Helpers/Response.cs (2)
Response(10-61)Error(41-60)UnityMcpBridge/Editor/Tools/ManageScene.cs (2)
ManageScene(17-425)HandleCommand(22-112)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(15-69)UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (1)
_coerce_int(20-42)
🪛 Ruff (0.12.2)
tests/test_telemetry_queue_worker.py
41-41: Unused function argument: monkeypatch
(ARG001)
53-53: Unused function argument: self
(ARG001)
53-53: Unused function argument: rec
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py
80-81: try-except-continue detected, consider logging the exception
(S112)
80-80: Do not catch blind exception: Exception
(BLE001)
143-143: Abstract raise to an inner function
(TRY301)
143-143: Avoid specifying long messages outside the exception class
(TRY003)
146-146: Abstract raise to an inner function
(TRY301)
146-146: Avoid specifying long messages outside the exception class
(TRY003)
147-147: Consider moving this statement to an else block
(TRY300)
148-148: Do not catch blind exception: Exception
(BLE001)
264-264: Do not catch blind exception: Exception
(BLE001)
313-318: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
320-320: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
328-328: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/server.py
32-34: try-except-pass detected, consider logging the exception
(S110)
32-32: Do not catch blind exception: Exception
(BLE001)
39-40: try-except-pass detected, consider logging the exception
(S110)
39-39: Do not catch blind exception: Exception
(BLE001)
61-61: Do not catch blind exception: Exception
(BLE001)
100-100: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py
15-15: Unused function argument: ctx
(ARG001)
47-47: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (9)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (3)
7-7: Telemetry import — LGTM.Importing the decorator here is appropriate.
64-66: Approve normalization — consistent with other tools
manage_scene now mirrors manage_gameobject/manage_editor/manage_shader by returning only{success, message, data}; no callers or tests inspect additional raw keys.
13-13: Decorators are applied in the intended order and consistently across the repo. All tool functions (e.g. execute_menu_item, manage_shader, manage_gameobject, manage_asset, read_console, find_in_file) use@mcp.tool()outside of@telemetry_tool, andmanage_scene.pymatches this convention.UnityMcpBridge/Editor/MCPForUnityBridge.cs (2)
41-41: LGTM on main-thread tracking field.
Field addition is appropriate for subsequent main-thread checks.
86-87: LGTM on connection-failure telemetry.
Error message is already truncated in TelemetryHelper; non-blocking and safe.UnityMcpBridge/UnityMcpServer~/src/server.py (1)
90-91: Nice: narrowed exception and parameterized logging.Catching
ConnectionErrorexplicitly and using lazy-arg logging is the right move.UnityMcpBridge/UnityMcpServer~/src/telemetry.py (3)
238-240: LGTM: non-blocking early return when disabled.This keeps telemetry inert under opt-out.
251-256: LGTM: bounded queue with drop-on-backpressure.Prevents thread explosion and sheds load safely.
169-197: LGTM: separated UUID and milestones loading with safer error handling.Good separation avoids UUID churn on malformed milestones.
…timeouts; fix milestone lock reentrancy
…gs; keep direct write path
…'action'; add tests for keyword/positional extraction
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
91-94: Fix property vs. method access mismatch.The telemetry call attempts to access
IsAutoConnectModeas a property, but Line 76 defines it as a method.Apply this fix in TelemetryHelper.cs:
- ["auto_connect"] = MCPForUnityBridge.IsAutoConnectMode + ["auto_connect"] = MCPForUnityBridge.IsAutoConnectMode()UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
26-31: Introspection fallback already handles positional ‘action’; also add noqa for BLE001 if linting.Apply:
- except Exception: + except Exception: # noqa: BLE001 - best-effort extraction sub_action = NoneAlso applies to: 60-66
39-44: Telemetry must never affect tool behavior; guard milestone recording and handle positional action.Use the already-derived sub_action, and wrap milestone calls.
Apply:
- success = True - if tool_name == "manage_script" and kwargs.get("action") == "create": - record_milestone(MilestoneType.FIRST_SCRIPT_CREATION) - elif tool_name.startswith("manage_scene"): - record_milestone(MilestoneType.FIRST_SCENE_MODIFICATION) - record_milestone(MilestoneType.FIRST_TOOL_USAGE) + success = True + action_val = sub_action or kwargs.get("action") + try: + if tool_name == "manage_script" and action_val == "create": + record_milestone(MilestoneType.FIRST_SCRIPT_CREATION) + elif tool_name.startswith("manage_scene"): + record_milestone(MilestoneType.FIRST_SCENE_MODIFICATION) + record_milestone(MilestoneType.FIRST_TOOL_USAGE) + except Exception: + _log.debug("milestone emit failed", exc_info=True)UnityMcpBridge/UnityMcpServer~/src/telemetry.py (1)
170-175: Start worker after loading persistent data.Prevents first events from using customer_uuid="unknown".
Apply:
- self._worker: threading.Thread = threading.Thread(target=self._worker_loop, daemon=True) - self._worker.start() - self._load_persistent_data() + self._load_persistent_data() + self._worker: threading.Thread = threading.Thread(target=self._worker_loop, daemon=True) + self._worker.start()
🧹 Nitpick comments (13)
UnityMcpBridge/Editor/Tools/ManageScene.cs (1)
53-53: Consider adjusting log verbosity for production readiness.The diagnostic logging statements are well-placed for debugging, but the frequency of these logs in the scene hierarchy operations might be excessive in production. Consider:
- Using a more granular logging level system
- Batching related log messages
- Implementing a sampling strategy for high-frequency operations
Also applies to: 110-110, 133-140, 335-337, 357-357, 392-394, 402-404, 407-412, 416-416
UnityMcpBridge/Editor/MCPForUnityBridge.cs (2)
1088-1103: Consider extracting timeout value to a named constant.The
HandleManageScenemethod usesFrameIOTimeoutMsfor both the main thread invocation and error message. Consider making this timeout configurable per-operation type since scene operations might need different timeouts than other operations.+ private const int SceneOperationTimeoutMs = FrameIOTimeoutMs; // Or make it configurable private static object HandleManageScene(JObject paramsObject) { try { if (IsDebugEnabled()) Debug.Log("[MCP] manage_scene: dispatching to main thread"); var sw = System.Diagnostics.Stopwatch.StartNew(); - var r = InvokeOnMainThreadWithTimeout(() => ManageScene.HandleCommand(paramsObject), FrameIOTimeoutMs); + var r = InvokeOnMainThreadWithTimeout(() => ManageScene.HandleCommand(paramsObject), SceneOperationTimeoutMs); sw.Stop(); if (IsDebugEnabled()) Debug.Log($"[MCP] manage_scene: completed in {sw.ElapsedMilliseconds} ms"); return r ?? Response.Error("manage_scene returned null (timeout or error)"); } catch (Exception ex) { return Response.Error($"manage_scene dispatch error: {ex.Message}"); } }
129-156: Rename and constrain the IO diagnostics writer thread
- Rename the thread (and related identifiers) to reflect its logger-only role (e.g. “MCP-IOLogger”)
- Add a comment clarifying that actual writes occur via WriteFrameAsync in the client loop
- Wrap the diagnostics loop in a debug-only conditional to eliminate unnecessary overhead in release builds
tests/test_telemetry_subaction.py (1)
15-15: Remove or document unused test function parameters.The mock functions have unused parameters that trigger static analysis warnings. While these are intentional for matching the expected signature, consider either:
- Prefixing with underscore to indicate intentionally unused:
_duration_ms- Adding a comment explaining why they're present
- def fake_record_tool_usage(tool_name, success, duration_ms, error, sub_action=None): + def fake_record_tool_usage(tool_name, success, duration_ms, error, sub_action=None): + # All parameters needed to match actual signature, only some are captured for testing captured["tool_name"] = tool_name captured["success"] = success captured["error"] = error captured["sub_action"] = sub_actionAlso applies to: 44-44, 67-67
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (4)
15-20: Silence unused ctx arg to satisfy Ruff.Rename to _ctx (or add a noqa) to avoid ARG001 without changing behavior.
Apply:
- def manage_scene( - ctx: Context, + def manage_scene( + _ctx: Context,
26-27: Docstring default for path conflicts with implementation.Docstring says default “Assets/”, but default is empty and omitted from params. Align docs or set the default.
Option A (docs):
- path: Asset path for scene operations (default: "Assets/"). + path: Asset path for scene operations (if provided).Option B (code):
- path: str = "", + path: str = "Assets/",
34-51: De-duplicate _coerce_int; reuse shared helper.This local helper diverges from resource_tools._coerce_int (e.g., minimum clamp). Import and reuse for consistency.
Apply:
-from typing import Dict, Any +from typing import Dict, Any +from tools.resource_tools import _coerce_int # reuse shared coercion @@ - # Coerce numeric inputs defensively - def _coerce_int(value, default=None): - if value is None: - return default - try: - if isinstance(value, bool): - return default - if isinstance(value, int): - return int(value) - s = str(value).strip() - if s.lower() in ("", "none", "null"): - return default - return int(float(s)) - except Exception: - return default - - coerced_build_index = _coerce_int(build_index, default=None) + coerced_build_index = _coerce_int(build_index, default=None, minimum=0)
68-69: Catching broad Exception is intentional here—document it to satisfy linter.Mark with noqa so Ruff BLE001 doesn’t fight this guardrail.
Apply:
- except Exception as e: + except Exception as e: # noqa: BLE001 - tool entrypoint must not raiseUnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
41-42: Optional: only flag scene “modification” for mutating actions.Avoid recording FIRST_SCENE_MODIFICATION for read-only ops like get_hierarchy.
Example:
mutating = {"create", "save", "add_to_build", "remove_from_build", "set_active"} if tool_name.startswith("manage_scene") and (action_val in mutating): record_milestone(MilestoneType.FIRST_SCENE_MODIFICATION)Also applies to: 75-76
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (4)
304-314: Treat any 2xx as success; lower success logs to debug.Reduces noise and counts 201/204 as OK.
Apply:
- if response.status_code == 200: - logger.info(f"Telemetry sent: {record.record_type}") + if 200 <= response.status_code < 300: + logger.debug(f"Telemetry sent: {record.record_type}") else: logger.warning(f"Telemetry failed: HTTP {response.status_code}") @@ - if 200 <= resp.getcode() < 300: - logger.info(f"Telemetry sent (urllib): {record.record_type}") + if 200 <= resp.getcode() < 300: + logger.debug(f"Telemetry sent (urllib): {record.record_type}")Also applies to: 326-329
79-85: Log config import failures at debug level (Ruff S112).Aids diagnostics without spamming.
Apply:
- except Exception: - continue + except Exception as e: # noqa: BLE001 + logger.debug("Config import failed for %s: %s", modname, e, exc_info=True) + continue
205-214: Set restrictive perms for milestones on POSIX (parity with UUID file).Apply:
try: self.config.milestones_file.write_text( json.dumps(self._milestones, indent=2), encoding="utf-8", ) + if os.name == "posix": + os.chmod(self.config.milestones_file, 0o600)
342-347: Optional: make singleton initialization thread-safe.Rare but possible double-init under concurrent import/use.
Apply:
-_telemetry_collector: Optional[TelemetryCollector] = None +_telemetry_collector: Optional[TelemetryCollector] = None +_telemetry_lock = threading.Lock() @@ - if _telemetry_collector is None: - _telemetry_collector = TelemetryCollector() + if _telemetry_collector is None: + with _telemetry_lock: + if _telemetry_collector is None: + _telemetry_collector = TelemetryCollector()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
UnityMcpBridge/Editor/MCPForUnityBridge.cs(9 hunks)UnityMcpBridge/Editor/Tools/ManageScene.cs(6 hunks)UnityMcpBridge/UnityMcpServer~/src/telemetry.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py(2 hunks)tests/test_telemetry_subaction.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
PR: CoplayDev/unity-mcp#260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
Applied to files:
UnityMcpBridge/Editor/MCPForUnityBridge.cs
🧬 Code graph analysis (5)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (4)
record_tool_usage(359-385)record_milestone(215-237)record_milestone(355-357)MilestoneType(48-56)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
telemetry_tool(14-17)decorator(15-16)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (4)
UnityMcpBridge/Editor/Helpers/McpLog.cs (2)
McpLog(6-30)Info(15-19)UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs (4)
TelemetryHelper(11-217)RecordBridgeStartup(133-140)RecordBridgeConnection(145-158)IsDebugEnabled(206-216)UnityMcpBridge/Editor/Tools/ManageScene.cs (2)
ManageScene(17-473)HandleCommand(51-150)UnityMcpBridge/Editor/Helpers/Response.cs (2)
Response(10-61)Error(41-60)
UnityMcpBridge/Editor/Tools/ManageScene.cs (2)
UnityMcpBridge/Editor/Helpers/McpLog.cs (2)
McpLog(6-30)Info(15-19)UnityMcpBridge/Editor/Helpers/Response.cs (3)
Error(41-60)Response(10-61)Success(18-33)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(15-87)UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (1)
_coerce_int(20-42)
tests/test_telemetry_subaction.py (1)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(15-87)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py
30-30: Do not catch blind exception: Exception
(BLE001)
44-44: Consider moving this statement to an else block
(TRY300)
64-64: Do not catch blind exception: Exception
(BLE001)
78-78: Consider moving this statement to an else block
(TRY300)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py
84-85: try-except-continue detected, consider logging the exception
(S112)
84-84: Do not catch blind exception: Exception
(BLE001)
108-108: Do not catch blind exception: Exception
(BLE001)
150-150: Abstract raise to an inner function
(TRY301)
150-150: Avoid specifying long messages outside the exception class
(TRY003)
153-153: Abstract raise to an inner function
(TRY301)
153-153: Avoid specifying long messages outside the exception class
(TRY003)
154-154: Consider moving this statement to an else block
(TRY300)
155-155: Do not catch blind exception: Exception
(BLE001)
270-270: Do not catch blind exception: Exception
(BLE001)
319-324: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
326-326: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
334-334: Do not catch blind exception: Exception
(BLE001)
378-378: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py
15-15: Unused function argument: ctx
(ARG001)
47-47: Do not catch blind exception: Exception
(BLE001)
tests/test_telemetry_subaction.py
15-15: Unused function argument: duration_ms
(ARG001)
23-23: Unused lambda argument: a
(ARG005)
23-23: Unused lambda argument: k
(ARG005)
26-26: Unused function argument: ctx
(ARG001)
26-26: Unused function argument: action
(ARG001)
44-44: Unused function argument: success
(ARG001)
44-44: Unused function argument: duration_ms
(ARG001)
44-44: Unused function argument: error
(ARG001)
49-49: Unused lambda argument: a
(ARG005)
49-49: Unused lambda argument: k
(ARG005)
52-52: Unused function argument: ctx
(ARG001)
52-52: Unused function argument: action
(ARG001)
52-52: Unused function argument: name
(ARG001)
67-67: Unused function argument: success
(ARG001)
67-67: Unused function argument: duration_ms
(ARG001)
67-67: Unused function argument: error
(ARG001)
72-72: Unused lambda argument: a
(ARG005)
72-72: Unused lambda argument: k
(ARG005)
75-75: Unused function argument: ctx
(ARG001)
75-75: Unused function argument: name
(ARG001)
🔇 Additional comments (8)
UnityMcpBridge/Editor/Tools/ManageScene.cs (2)
19-46: LGTM! Well-structured scene command parser with robust type handling.The
SceneCommandclass andToSceneCommandmethod provide clean, centralized parameter parsing. The nullablebuildIndexhandling with comprehensive type coercion is particularly thorough.
54-58: Good defensive programming with the command extraction pattern.The use of
ToSceneCommandfor parameter extraction provides consistent handling and prevents null reference exceptions through defensive initialization.UnityMcpBridge/Editor/MCPForUnityBridge.cs (3)
931-936: Good defensive check for uninitialized mainThreadId.The guard against
mainThreadId == 0prevents potential editor hangs, which is a critical safety measure.
973-975: LGTM! Proper exception throwing for timeout handling.Throwing an exception instead of returning Response.Error ensures proper error envelope generation in the catch block.
587-619: Excellent timeout handling with structured error responses.The implementation properly handles command processing timeouts and returns well-structured JSON error responses, preventing indefinite client blocks.
tests/test_telemetry_subaction.py (1)
1-82: Well-structured telemetry decorator tests with good coverage.The tests effectively validate sub-action extraction from both keyword and positional arguments, and correctly handle the absence of sub-actions. The use of monkeypatching to isolate the decorator behavior is appropriate.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py (2)
12-14: Decorator order is correct.Telemetry wraps the tool function before MCP registration; sub_action extraction will work.
52-59: Confirm JSON key namingbuildIndexvsbuild_index. Unity’s scene‐management endpoint will only pick up the exact property name, so verify in the Unity code or docs which form is expected and adjust here to avoid silent failures. For example, you can search this bridge with a correct regex:rg -nE 'build(Index|_index)' UnityMcpBridge -g '!**/dist/**' -g '!**/build/**'
…/conn telemetry; writer IO logs; manage_scene tolerant params; test worker wake
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (1)
84-86: Consider logging config import failures at debug level.While the broad exception handling is appropriate here (config loading should be resilient), adding debug logging would help with troubleshooting configuration issues.
for modname in ( "UnityMcpBridge.UnityMcpServer~.src.config", "UnityMcpBridge.UnityMcpServer.src.config", "src.config", "config", ): try: mod = importlib.import_module(modname) server_config = getattr(mod, "config", None) if server_config is not None: break - except Exception: - continue + except Exception as e: + logger.debug(f"Failed to import config from {modname}: {e}") + continuetests/test_telemetry_queue_worker.py (2)
41-41: Remove unused monkeypatch parameter.The
monkeypatchfixture parameter is not used in the test.-def test_telemetry_queue_backpressure_and_single_worker(monkeypatch, caplog): +def test_telemetry_queue_backpressure_and_single_worker(caplog):
57-58: Remove unused parameters from slow_send function.The
selfandrecparameters are not used within the function.- def slow_send(self, rec): + def slow_send(_self, _rec): time.sleep(0.05)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs(1 hunks)UnityMcpBridge/UnityMcpServer~/src/server.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/telemetry.py(1 hunks)tests/test_telemetry_queue_worker.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs (1)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (7)
System(679-724)System(726-730)System(732-751)System(753-770)IsDebugEnabled(61-64)MCPForUnityBridge(124-176)IsAutoConnectMode(76-76)
UnityMcpBridge/UnityMcpServer~/src/server.py (3)
UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (1)
register_all_tools(15-30)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
get_unity_connection(351-371)UnityConnection(30-346)UnityMcpBridge/UnityMcpServer~/src/telemetry.py (5)
record_telemetry(350-354)record_milestone(216-238)record_milestone(356-358)RecordType(37-46)MilestoneType(48-56)
tests/test_telemetry_queue_worker.py (1)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (4)
TelemetryCollector(162-337)record(240-262)RecordType(37-46)_send_telemetry(277-337)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py
84-85: try-except-continue detected, consider logging the exception
(S112)
84-84: Do not catch blind exception: Exception
(BLE001)
108-108: Do not catch blind exception: Exception
(BLE001)
150-150: Abstract raise to an inner function
(TRY301)
150-150: Avoid specifying long messages outside the exception class
(TRY003)
153-153: Abstract raise to an inner function
(TRY301)
153-153: Avoid specifying long messages outside the exception class
(TRY003)
154-154: Consider moving this statement to an else block
(TRY300)
155-155: Do not catch blind exception: Exception
(BLE001)
271-271: Do not catch blind exception: Exception
(BLE001)
320-325: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
327-327: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
335-335: Do not catch blind exception: Exception
(BLE001)
379-379: Do not catch blind exception: Exception
(BLE001)
UnityMcpBridge/UnityMcpServer~/src/server.py
32-34: try-except-pass detected, consider logging the exception
(S110)
32-32: Do not catch blind exception: Exception
(BLE001)
39-40: try-except-pass detected, consider logging the exception
(S110)
39-39: Do not catch blind exception: Exception
(BLE001)
61-61: Do not catch blind exception: Exception
(BLE001)
72-72: Do not catch blind exception: Exception
(BLE001)
104-104: Undefined name e
(F821)
108-108: Do not catch blind exception: Exception
(BLE001)
116-116: Undefined name e
(F821)
tests/test_telemetry_queue_worker.py
41-41: Unused function argument: monkeypatch
(ARG001)
57-57: Unused function argument: self
(ARG001)
57-57: Unused function argument: rec
(ARG001)
🔇 Additional comments (11)
UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs (1)
1-224: LGTM! The telemetry implementation follows privacy-first principles with comprehensive opt-out support.The implementation correctly handles multiple opt-out mechanisms, persistent UUID storage, and robust error handling. The thread-safe registration mechanism and fallback logging are well implemented.
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (1)
1-224: LGTM! Privacy-focused telemetry implementation with robust architecture.This is a well-designed telemetry system that prioritizes privacy with multiple opt-out mechanisms, persistent data handling, and robust error recovery. The implementation correctly uses a bounded queue with a single background worker to prevent blocking operations.
tests/test_telemetry_queue_worker.py (1)
41-82: LGTM! Test validates telemetry queue backpressure and worker behavior.The test effectively validates that telemetry operations are non-blocking under backpressure and that only a single worker thread processes events. The dynamic module loading approach and queue replacement technique are clever solutions for testing internal behavior.
UnityMcpBridge/UnityMcpServer~/src/server.py (8)
3-4: LGTM! Added proper imports for new logging and telemetry functionality.The imports support the new rotating file logging and telemetry features.
16-19: LGTM! Proper console logging configuration.Setting
stream=Noneandforce=Truecorrectly ensures logging goes to stderr and overrides any existing stdout handlers, which is essential for MCP stdio communication.
22-41: LGTM! Robust rotating file logging setup.The implementation properly handles cross-platform paths, creates necessary directories, and gracefully handles setup failures without breaking startup. The rotating file handler configuration is appropriate for log management.
42-44: LGTM! Proper telemetry import ordering.Importing telemetry after logging configuration ensures telemetry logs use the correct configuration.
54-75: LGTM! Proper deferred telemetry implementation.The implementation correctly defers telemetry emission by 1 second to avoid interfering with stdio handshake, addresses the previous review feedback about immediate timing checks.
76-93: LGTM! Optional Unity startup connection with proper telemetry.The environment variable control for skipping startup connection is useful for testing scenarios, and the deferred telemetry recording correctly captures connection success.
94-107: LGTM! Proper ConnectionError handling with telemetry.The implementation correctly catches the specific
ConnectionErrorexception and uses parameterized logging as suggested in previous reviews.
147-159: LGTM! Improved asset creation strategy documentation.The updated formatting with real newlines improves readability of the strategy text.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (3)
54-57: Finally-guard forrecord_tool_usagelooks good and matches prior feedbackTelemetry errors are contained; original function result/exception is preserved.
39-47: Action extraction via bound signature correctly handles positional argsThis addresses the earlier concern about positional
actionand mirrors logic in both branches.Also applies to: 80-88
75-88: Async branch mirrors sync milestones and guards—niceParity achieved with logging, milestones, and isolation.
🧹 Nitpick comments (5)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (5)
20-21: Use a monotonic clock for duration measurementsPrefer
time.perf_counter()to avoid wall-clock adjustments skewing durations.- start_time = time.time() + start_time = time.perf_counter() ... - duration_ms = (time.time() - start_time) * 1000 + duration_ms = (time.perf_counter() - start_time) * 1000 ... - start_time = time.time() + start_time = time.perf_counter() ... - duration_ms = (time.time() - start_time) * 1000 + duration_ms = (time.perf_counter() - start_time) * 1000Also applies to: 53-54, 61-63, 94-95
17-19: Avoid per-call signature introspection; compute once at decoration time
inspect.signatureeach call is relatively expensive. Compute once and reuse; keep the broad guard for isolation.def telemetry_tool(tool_name: str): """Decorator to add telemetry tracking to MCP tools""" def decorator(func: Callable) -> Callable: + # Precompute signature once to reduce per-call overhead + try: + sig = inspect.signature(func) + except Exception: # noqa: BLE001 - telemetry isolation + sig = None @functools.wraps(func) def _sync_wrapper(*args, **kwargs) -> Any: start_time = time.time() success = False error = None # Extract sub-action (e.g., 'get_hierarchy') from bound args when available sub_action = None try: - sig = inspect.signature(func) - bound = sig.bind_partial(*args, **kwargs) - bound.apply_defaults() - sub_action = bound.arguments.get("action") - except Exception: + if sig is not None and "action" in sig.parameters: + bound = sig.bind_partial(*args, **kwargs) + bound.apply_defaults() + sub_action = bound.arguments.get("action") + except Exception: # noqa: BLE001 - telemetry isolation sub_action = None @@ async def _async_wrapper(*args, **kwargs) -> Any: start_time = time.time() success = False error = None # Extract sub-action (e.g., 'get_hierarchy') from bound args when available sub_action = None try: - sig = inspect.signature(func) - bound = sig.bind_partial(*args, **kwargs) - bound.apply_defaults() - sub_action = bound.arguments.get("action") - except Exception: + if sig is not None and "action" in sig.parameters: + bound = sig.bind_partial(*args, **kwargs) + bound.apply_defaults() + sub_action = bound.arguments.get("action") + except Exception: # noqa: BLE001 - telemetry isolation sub_action = NoneAlso applies to: 23-31, 65-73
32-49: Satisfy TRY300: return inelseblock; keep telemetry isolation with explicit commentsPurely structural to appease Ruff; no behavior change.
- try: + try: global _decorator_log_count if _decorator_log_count < 10: _log.info(f"telemetry_decorator sync: tool={tool_name}") _decorator_log_count += 1 result = func(*args, **kwargs) success = True action_val = sub_action or kwargs.get("action") - try: + try: if tool_name == "manage_script" and action_val == "create": record_milestone(MilestoneType.FIRST_SCRIPT_CREATION) elif tool_name.startswith("manage_scene"): record_milestone(MilestoneType.FIRST_SCENE_MODIFICATION) record_milestone(MilestoneType.FIRST_TOOL_USAGE) - except Exception: + except Exception: # noqa: BLE001 - telemetry isolation _log.debug("milestone emit failed", exc_info=True) - return result - except Exception as e: + except Exception as e: error = str(e) raise + else: + return result finally: duration_ms = (time.time() - start_time) * 1000 - try: + try: record_tool_usage(tool_name, success, duration_ms, error, sub_action=sub_action) - except Exception: + except Exception: # noqa: BLE001 - telemetry isolation _log.debug("record_tool_usage failed", exc_info=True) @@ - try: + try: global _decorator_log_count if _decorator_log_count < 10: _log.info(f"telemetry_decorator async: tool={tool_name}") _decorator_log_count += 1 result = await func(*args, **kwargs) success = True action_val = sub_action or kwargs.get("action") - try: + try: if tool_name == "manage_script" and action_val == "create": record_milestone(MilestoneType.FIRST_SCRIPT_CREATION) elif tool_name.startswith("manage_scene"): record_milestone(MilestoneType.FIRST_SCENE_MODIFICATION) record_milestone(MilestoneType.FIRST_TOOL_USAGE) - except Exception: + except Exception: # noqa: BLE001 - telemetry isolation _log.debug("milestone emit failed", exc_info=True) - return result - except Exception as e: + except Exception as e: error = str(e) raise + else: + return result finally: duration_ms = (time.time() - start_time) * 1000 - try: + try: record_tool_usage(tool_name, success, duration_ms, error, sub_action=sub_action) - except Exception: + except Exception: # noqa: BLE001 - telemetry isolation _log.debug("record_tool_usage failed", exc_info=True)Also applies to: 74-91
39-47: Deduplicate milestone emission into a tiny helperKeeps both branches identical and clearer; makes it harder to regress async/sync parity.
- action_val = sub_action or kwargs.get("action") - try: - if tool_name == "manage_script" and action_val == "create": - record_milestone(MilestoneType.FIRST_SCRIPT_CREATION) - elif tool_name.startswith("manage_scene"): - record_milestone(MilestoneType.FIRST_SCENE_MODIFICATION) - record_milestone(MilestoneType.FIRST_TOOL_USAGE) - except Exception: - _log.debug("milestone emit failed", exc_info=True) + action_val = sub_action or kwargs.get("action") + _emit_milestones(action_val) @@ - action_val = sub_action or kwargs.get("action") - try: - if tool_name == "manage_script" and action_val == "create": - record_milestone(MilestoneType.FIRST_SCRIPT_CREATION) - elif tool_name.startswith("manage_scene"): - record_milestone(MilestoneType.FIRST_SCENE_MODIFICATION) - record_milestone(MilestoneType.FIRST_TOOL_USAGE) - except Exception: - _log.debug("milestone emit failed", exc_info=True) + action_val = sub_action or kwargs.get("action") + _emit_milestones(action_val)Add inside
decoratorbefore the wrappers:+ def _emit_milestones(action_val: str | None) -> None: + try: + if tool_name == "manage_script" and action_val == "create": + record_milestone(MilestoneType.FIRST_SCRIPT_CREATION) + elif tool_name.startswith("manage_scene"): + record_milestone(MilestoneType.FIRST_SCENE_MODIFICATION) + record_milestone(MilestoneType.FIRST_TOOL_USAGE) + except Exception: # noqa: BLE001 - telemetry isolation + _log.debug("milestone emit failed", exc_info=True)Also applies to: 80-88
30-31: Intentional broad exception handling: annotate to appease Ruff BLE001These catches are deliberate to keep telemetry non-intrusive. Add
# noqa: BLE001and a short rationale.-except Exception: +except Exception: # noqa: BLE001 - telemetry must not affect tool behavior ... -except Exception: +except Exception: # noqa: BLE001 - telemetry must not affect tool behavior ... -except Exception: +except Exception: # noqa: BLE001 - telemetry must not affect tool behavior ... -except Exception: +except Exception: # noqa: BLE001 - telemetry must not affect tool behavior ... -except Exception: +except Exception: # noqa: BLE001 - telemetry must not affect tool behavior ... -except Exception: +except Exception: # noqa: BLE001 - telemetry must not affect tool behaviorAlso applies to: 46-47, 56-57, 71-73, 87-88, 97-98
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
telemetry_tool(14-17)decorator(15-16)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py
30-30: Do not catch blind exception: Exception
(BLE001)
46-46: Do not catch blind exception: Exception
(BLE001)
48-48: Consider moving this statement to an else block
(TRY300)
56-56: Do not catch blind exception: Exception
(BLE001)
71-71: Do not catch blind exception: Exception
(BLE001)
87-87: Do not catch blind exception: Exception
(BLE001)
89-89: Consider moving this statement to an else block
(TRY300)
97-97: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py (1)
1-16: Use relative import for local telemetry module
Replace the absolute importfrom telemetry import record_tool_usage, record_milestone, MilestoneTypewith
from .telemetry import record_tool_usage, record_milestone, MilestoneTypeto ensure you’re importing the local module and avoid collisions with external packages; verify your packaging setup (setup.py/pyproject.toml) correctly defines
srcas a package root.
| import inspect | ||
| import logging | ||
| from typing import Callable, Any | ||
| from telemetry import record_tool_usage, record_milestone, MilestoneType |
There was a problem hiding this comment.
Resolve potential module shadowing and ensure tools use this decorator
If any tool modules define their own telemetry_tool (e.g., manage_script.py snippet), this file’s decorator won’t be used. Replace those stubs with imports of this implementation to ensure telemetry actually fires.
Proposed fix in tools/manage_script.py (adjust import style per package layout):
-def telemetry_tool(tool_name: str):
- def decorator(func):
- return func
- return decorator
+from telemetry_decorator import telemetry_tool
+# or, if packaged:
+# from .telemetry_decorator import telemetry_toolCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py around line 10,
there is a risk that tool modules define their own telemetry_tool stubs which
shadow this implementation and prevent telemetry from firing; to fix, remove any
local telemetry_tool stubs in tool modules (e.g., tools/manage_script.py) and
replace them with an explicit import of this module's decorator (use the correct
package-relative path such as from UnityMcpServer.src.telemetry_decorator import
telemetry_tool or adjust to your repo layout), ensure the tool functions are
decorated with the imported telemetry_tool, and verify no duplicate
telemetry_tool definitions remain across the codebase.
…rtup diagnostics, and normalize logging - config: point telemetry_endpoint to Cloud Run default - telemetry: log effective endpoint/timeout; reject localhost endpoints - server: telemetry logger at normal level with rotating file; default timeout=5s when unset
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
UnityMcpBridge/UnityMcpServer~/src/server.py (1)
125-137: Fix undefined variable bug in exception handler lambdas.The lambda functions capture the variable
eby reference, butegoes out of scope when the except block ends. This will cause an undefined variable error when the timer executes.except Exception as e: logger.warning("Unexpected error connecting to Unity on startup: %s", e) _unity_connection = None + error_msg = str(e)[:200] import threading as _t _t.Timer(1.0, lambda: record_telemetry( RecordType.UNITY_CONNECTION, { "status": "failed", - "error": str(e)[:200], + "error": error_msg, "connection_time_ms": (time.perf_counter() - start_clk) * 1000, } )).start()The same fix should be applied to the ConnectionError handler at lines 111-124:
except ConnectionError as e: logger.warning("Could not connect to Unity on startup: %s", e) _unity_connection = None + error_msg = str(e)[:200] # Record connection failure (deferred) import threading as _t _t.Timer(1.0, lambda: record_telemetry( RecordType.UNITY_CONNECTION, { "status": "failed", - "error": str(e)[:200], + "error": error_msg, "connection_time_ms": (time.perf_counter() - start_clk) * 1000, } )).start()
🧹 Nitpick comments (3)
UnityMcpBridge/UnityMcpServer~/src/server.py (1)
22-42: Improve exception handling for logging setup by adding specific error logging.While failing silently for logging setup is acceptable to avoid breaking startup, consider logging the specific exception when logging setup fails for debugging purposes.
# Also write logs to a rotating file so logs are available when launched via stdio try: import os as _os _log_dir = _os.path.join(_os.path.expanduser("~/Library/Application Support/UnityMCP"), "Logs") _os.makedirs(_log_dir, exist_ok=True) _file_path = _os.path.join(_log_dir, "unity_mcp_server.log") _fh = RotatingFileHandler(_file_path, maxBytes=512*1024, backupCount=2, encoding="utf-8") _fh.setFormatter(logging.Formatter(config.log_format)) _fh.setLevel(getattr(logging, config.log_level)) logger.addHandler(_fh) # Also route telemetry logger to the same rotating file and normal level try: tlog = logging.getLogger("unity-mcp-telemetry") tlog.setLevel(getattr(logging, config.log_level)) tlog.addHandler(_fh) - except Exception: - # Never let logging setup break startup - pass -except Exception: - # Never let logging setup break startup - pass + except Exception as e: + # Never let logging setup break startup + print(f"Failed to setup telemetry logger: {e}", file=sys.stderr) +except Exception as e: + # Never let logging setup break startup + print(f"Failed to setup rotating log file: {e}", file=sys.stderr)UnityMcpBridge/UnityMcpServer~/src/telemetry.py (2)
79-85: Consider logging the exception for debugging config module import failures.The current implementation silently continues on exception when importing config modules. While this is acceptable for resilience, consider logging the exception for debugging purposes.
for modname in ( "UnityMcpBridge.UnityMcpServer~.src.config", "UnityMcpBridge.UnityMcpServer.src.config", "src.config", "config", ): try: mod = importlib.import_module(modname) server_config = getattr(mod, "config", None) if server_config is not None: break - except Exception: + except Exception as e: + logger.debug(f"Failed to import config module {modname}: {e}") continue
186-186: Consider using RLock to prevent potential deadlock.The class uses
threading.Lockbut therecord_milestonemethod acquires this lock then calls_save_milestoneswhich appears to be designed to be called with the lock held. While the current implementation is correct (caller holds lock), usingRLockwould make the code more maintainable and prevent future issues.- self._lock: threading.Lock = threading.Lock() + self._lock: threading.RLock = threading.RLock()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
UnityMcpBridge/UnityMcpServer~/src/config.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/server.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/telemetry.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- UnityMcpBridge/UnityMcpServer~/src/config.py
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/UnityMcpServer~/src/server.py (2)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
get_unity_connection(351-371)UnityConnection(30-346)UnityMcpBridge/UnityMcpServer~/src/telemetry.py (5)
record_telemetry(367-371)record_milestone(233-255)record_milestone(373-375)RecordType(37-46)MilestoneType(48-56)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/server.py
37-39: try-except-pass detected, consider logging the exception
(S110)
37-37: Do not catch blind exception: Exception
(BLE001)
40-42: try-except-pass detected, consider logging the exception
(S110)
40-40: Do not catch blind exception: Exception
(BLE001)
47-48: try-except-pass detected, consider logging the exception
(S110)
47-47: Do not catch blind exception: Exception
(BLE001)
58-59: try-except-pass detected, consider logging the exception
(S110)
58-58: Do not catch blind exception: Exception
(BLE001)
78-78: Do not catch blind exception: Exception
(BLE001)
89-89: Do not catch blind exception: Exception
(BLE001)
121-121: Undefined name e
(F821)
125-125: Do not catch blind exception: Exception
(BLE001)
133-133: Undefined name e
(F821)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py
84-85: try-except-continue detected, consider logging the exception
(S112)
84-84: Do not catch blind exception: Exception
(BLE001)
106-107: try-except-pass detected, consider logging the exception
(S110)
106-106: Do not catch blind exception: Exception
(BLE001)
117-117: Do not catch blind exception: Exception
(BLE001)
121-122: try-except-pass detected, consider logging the exception
(S110)
121-121: Do not catch blind exception: Exception
(BLE001)
163-163: Abstract raise to an inner function
(TRY301)
163-163: Avoid specifying long messages outside the exception class
(TRY003)
166-166: Abstract raise to an inner function
(TRY301)
166-166: Avoid specifying long messages outside the exception class
(TRY003)
170-170: Abstract raise to an inner function
(TRY301)
170-170: Avoid specifying long messages outside the exception class
(TRY003)
171-171: Consider moving this statement to an else block
(TRY300)
172-172: Do not catch blind exception: Exception
(BLE001)
288-288: Do not catch blind exception: Exception
(BLE001)
337-342: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
344-344: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
352-352: Do not catch blind exception: Exception
(BLE001)
396-396: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (7)
UnityMcpBridge/UnityMcpServer~/src/server.py (2)
17-18: LGTM! Good practice using force=True to ensure proper handler replacement.The addition of
stream=Noneandforce=Trueensures that the logging configuration properly replaces any existing handlers and avoids conflicts with MCP stdio handshake.
164-176: LGTM! Good improvement using real newline formatting.The switch from escaped sequences to actual newlines improves readability of the asset creation strategy text.
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (5)
1-67: LGTM! Well-structured telemetry system with privacy-first design.The telemetry system is well-designed with proper privacy considerations:
- Fire-and-forget pattern with background worker
- Non-blocking operations
- Bounded queue to prevent resource exhaustion
- Clear documentation of intent and behavior
156-177: Excellent endpoint validation with proper security checks.The endpoint validation is comprehensive and secure:
- Validates URL scheme (http/https only)
- Rejects localhost/loopback addresses
- Validates presence of network location
- Falls back safely on invalid values
281-292: LGTM! Robust worker loop implementation.The worker loop is well-implemented:
- Proper exception handling to keep the worker running
- Uses
task_done()for queue management- Suppresses exceptions in cleanup to ensure reliability
377-403: LGTM! Comprehensive tool usage recording with proper error handling.The
record_tool_usagefunction is well-implemented with:
- Proper parameter documentation
- Safe handling of sub_action conversion
- Error message truncation to prevent excessive data
- Resilient error handling
323-350: No action needed: endpoint validation is lightweight and incurs negligible overhead at telemetry send time.
The_validated_endpointmethod performs only in-memory URL parsing and sanity checks (no I/O) and is invoked twice per send, which is trivial compared to network latency; caching isn’t required.
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests