From 7f0527f708fcb0ceaf560f80805f48f3f5d84c9c Mon Sep 17 00:00:00 2001
From: David Sarno <david@lighthaus.us>
Date: Mon, 8 Sep 2025 20:25:07 -0700
Subject: [PATCH] chore: apply CodeRabbit suggestions

- 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
---
 .../UnityMcpServer~/src/telemetry.py          | 35 ++++++++++++++++---
 .../src/tools/execute_menu_item.py            |  2 +-
 .../src/tools/manage_gameobject.py            |  4 +--
 3 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py
index b701e2a..3141d1b 100644
--- a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py
+++ b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py
@@ -13,6 +13,7 @@ import sys
 import platform
 import logging
 from enum import Enum
+from urllib.parse import urlparse
 from dataclasses import dataclass, asdict
 from typing import Optional, Dict, Any, List
 from pathlib import Path
@@ -64,9 +65,11 @@ class TelemetryConfig:
         self.enabled = not self._is_disabled()
         
         # Telemetry endpoint (Cloud Run default; override via env)
-        self.endpoint = os.environ.get(
-            "UNITY_MCP_TELEMETRY_ENDPOINT",
-            "https://unity-mcp-telemetry-375728817078.us-central1.run.app/telemetry/events"
+        default_ep = "https://unity-mcp-telemetry-375728817078.us-central1.run.app/telemetry/events"
+        self.default_endpoint = default_ep
+        self.endpoint = self._validated_endpoint(
+            os.environ.get("UNITY_MCP_TELEMETRY_ENDPOINT", default_ep),
+            default_ep,
         )
         
         # Local storage for UUID and milestones
@@ -109,6 +112,25 @@ class TelemetryConfig:
         data_dir.mkdir(parents=True, exist_ok=True)
         return data_dir
 
+    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:
+            logger.debug(
+                f"Invalid telemetry endpoint '{candidate}', using default. Error: {e}",
+                exc_info=True,
+            )
+            return fallback
+
 class TelemetryCollector:
     """Main telemetry collection class"""
     
@@ -227,7 +249,9 @@ class TelemetryCollector:
             # Prefer httpx when available; otherwise fall back to urllib
             if httpx:
                 with httpx.Client(timeout=self.config.timeout) as client:
-                    response = client.post(self.config.endpoint, json=payload)
+                    # Re-validate endpoint at send time to handle dynamic changes
+                    endpoint = self.config._validated_endpoint(self.config.endpoint, self.config.default_endpoint)
+                    response = client.post(endpoint, json=payload)
                     if response.status_code == 200:
                         logger.debug(f"Telemetry sent: {record.record_type}")
                     else:
@@ -236,8 +260,9 @@ class TelemetryCollector:
                 import urllib.request
                 import urllib.error
                 data_bytes = json.dumps(payload).encode("utf-8")
+                endpoint = self.config._validated_endpoint(self.config.endpoint, self.config.default_endpoint)
                 req = urllib.request.Request(
-                    self.config.endpoint,
+                    endpoint,
                     data=data_bytes,
                     headers={"Content-Type": "application/json"},
                     method="POST",
diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py b/UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py
index 27fa51a..c21ecb8 100644
--- a/UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py
+++ b/UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py
@@ -14,7 +14,7 @@ def register_execute_menu_item_tools(mcp: FastMCP):
 
     @mcp.tool()
     @telemetry_tool("execute_menu_item")
-    async def execute_menu_item(
+    def execute_menu_item(
         ctx: Any,
         menu_path: str,
         action: str = 'execute',
diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py
index b96a8bb..a2ffe0e 100644
--- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py
+++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py
@@ -122,9 +122,9 @@ def register_manage_gameobject_tools(mcp: FastMCP):
                     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"}
-            # Ensure prefab_folder itself isn't sent if prefabPath was constructed or provided
+            # Ensure prefabFolder itself isn't sent if prefabPath was constructed or provided
             # The C# side only needs the final prefabPath
-            params.pop("prefab_folder", None) 
+            params.pop("prefabFolder", None) 
             # --------------------------------
             
             # Use centralized retry helper