Skip to content

Chore/pytest harness tests only #85

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dsarno wants to merge 13 commits into from
Closed

Chore/pytest harness tests only #85

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f8946c2
pytest: make harness MCPForUnity-only; remove UnityMcpBridge paths fr…
dsarno Oct 21, 2025
15bd3f3
Add missing meta for MaterialMeshInstantiationTests.cs (Assets)
dsarno Oct 21, 2025
a729600
bridge/tools/manage_script: fix missing unity_connection prefix in va…
dsarno Oct 21, 2025
b0edffd
tests: rename to script_apply_edits_module; extract DummyContext to t…
dsarno Oct 21, 2025
282f4d8
tests: import cleanup and helper extraction; telemetry: prefer plain …
dsarno Oct 21, 2025
2d50bf4
chore: remove unintended .wt-origin-main gitlink and ignore folder
dsarno Oct 21, 2025
386a6cc
tests: nit fixes (unused-arg stubs, import order, path-normalized ign…
dsarno Oct 21, 2025
8cdcda3
Add development dependencies to pyproject.toml
dsarno Oct 21, 2025
ddd3159
Revert "Update github-repo-stats.yml"
dsarno Oct 21, 2025
a607041
test: improve test clarity and modernize asyncio usage
dsarno Oct 21, 2025
3213d46
refactor: remove duplicate DummyContext definitions across test files
dsarno Oct 21, 2025
51750f0
chore: remove unused _load function from test_edit_strict_and_warning…
dsarno Oct 21, 2025
dcd6907
docs: add comment explaining CWD manipulation in telemetry test
dsarno Oct 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/github-repo-stats.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
name: github-repo-stats

on:
# schedule:
schedule:
# Run this once per day, towards the end of the day for keeping the most
# recent data point most meaningful (hours are interpreted in UTC).
#- cron: "0 23 * * *"
- cron: "0 23 * * *"
workflow_dispatch: # Allow for running this manually.

jobs:
Expand Down
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,5 @@ TestProjects/UnityMCPTests/Packages/packages-lock.json
# Backup artifacts
*.backup
*.backup.meta

.wt-origin-main/
6 changes: 6 additions & 0 deletions MCPForUnity/UnityMcpServer~/src/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@ dependencies = [
"tomli>=2.3.0",
]

[project.optional-dependencies]
dev = [
"pytest>=8.0.0",
"pytest-anyio>=0.6.0",
]

[build-system]
requires = ["setuptools>=64.0.0", "wheel"]
build-backend = "setuptools.build_meta"
Expand Down
16 changes: 10 additions & 6 deletions MCPForUnity/UnityMcpServer~/src/telemetry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,11 @@ def __init__(self):
"""
server_config = None
for modname in (
# Prefer plain module to respect test-time overrides and sys.path injection
"config",
"src.config",
Comment on lines +96 to +98
Copy link

@coderabbitai coderabbitai bot Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify that plain module names don't introduce unintended imports in production.

The addition of plain module names ("config", "src.config") before package-qualified paths enables test-time sys.path injection, which aligns with the test harness changes in this PR. However, these plain names could theoretically match unintended modules if sys.path is polluted in production environments.

Run the following script to verify that no unintended config or src.config modules exist in common Python paths that could shadow the intended module:

Based on learnings

🏁 Script executed:

#!/bin/bash
# Description: Check for potential module name collisions with "config" and "src.config"

# Search for config.py files that might be imported accidentally
echo "=== Searching for potential config.py collisions ==="
fd -t f "^config\.py$" --exclude "MCPForUnity/UnityMcpServer~/src/config.py"

# Check if src/config.py exists in other locations
echo -e "\n=== Searching for src/config.py in other locations ==="
fd -t f "^config\.py$" -p "src/" --exclude "MCPForUnity/UnityMcpServer~/src/config.py"

Length of output: 245

🏁 Script executed:

#!/bin/bash
# Inspect both config.py files to understand their relationship

echo "=== MCPForUnity config.py ==="
cat MCPForUnity/UnityMcpServer~/src/config.py 2>/dev/null || echo "Not found"

echo -e "\n=== UnityMcpBridge config.py ==="
cat UnityMcpBridge/UnityMcpServer~/src/config.py 2>/dev/null || echo "Not found"

echo -e "\n=== telemetry.py lines 90-130 for context ==="
sed -n '90,130p' MCPForUnity/UnityMcpServer~/src/telemetry.py 2>/dev/null || echo "Not found"

Length of output: 4782

🏁 Script executed:

#!/bin/bash
# Find and inspect _validated_endpoint method

echo "=== Searching for _validated_endpoint implementation ==="
rg -n "_validated_endpoint" MCPForUnity/UnityMcpServer~/src/telemetry.py -A 15

echo -e "\n=== Full __init__ context to understand endpoint assignment flow ==="
sed -n '88,135p' MCPForUnity/UnityMcpServer~/src/telemetry.py

Length of output: 5509

🏁 Script executed:

#!/bin/bash
# Get complete _validated_endpoint implementation

echo "=== Complete _validated_endpoint method ==="
sed -n '187,210p' MCPForUnity/UnityMcpServer~/src/telemetry.py

echo -e "\n=== Check if hardcoded default is accessible as constant ==="
rg -n "api-prod.coplay.dev" MCPForUnity/UnityMcpServer~/src/telemetry.py

Length of output: 1299

Address circular fallback in endpoint validation; verify module import resolution in production.

Two issues identified:

  1. Endpoint validation bypass (lines 120-126): When cfg_default is invalid, _validated_endpoint(default_ep, default_ep) fails validation but returns the same invalid value as fallback. The hardcoded default ("https://api-prod.coplay.dev/telemetry/events") should be the ultimate fallback instead. Replace line 126 with:

    self.endpoint = self._validated_endpoint(default_ep, "https://api-prod.coplay.dev/telemetry/events")

  2. Module import resolution (lines 96-98): Plain module names enable test-time sys.path injection, but UnityMcpBridge/UnityMcpServer~/src/config.py exists alongside the target module, creating collision risk. While fully-qualified fallbacks (lines 99-100) provide MCPForUnity-specific safety for production, verify that plain imports resolve to the intended module in both test and production environments. Consider adding debug logging to confirm which config module was loaded.

"MCPForUnity.UnityMcpServer~.src.config",
"MCPForUnity.UnityMcpServer.src.config",
"src.config",
"config",
):
try:
mod = importlib.import_module(modname)
Expand All @@ -116,10 +117,13 @@ def __init__(self):
server_config, "telemetry_endpoint", None)
default_ep = cfg_default or "https://api-prod.coplay.dev/telemetry/events"
self.default_endpoint = default_ep
self.endpoint = self._validated_endpoint(
os.environ.get("UNITY_MCP_TELEMETRY_ENDPOINT", default_ep),
default_ep,
)
# Prefer config default; allow explicit env override only when set
env_ep = os.environ.get("UNITY_MCP_TELEMETRY_ENDPOINT")
if env_ep is not None and env_ep != "":
self.endpoint = self._validated_endpoint(env_ep, default_ep)
else:
# Validate config-provided default as well to enforce scheme/host rules
self.endpoint = self._validated_endpoint(default_ep, default_ep)
Comment on lines +120 to +126
Copy link

@coderabbitai coderabbitai bot Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Validation fallback uses potentially invalid config value.

The endpoint validation logic has a flaw: when validating default_ep (line 126), it passes default_ep itself as the fallback. If the config file provides an invalid telemetry endpoint (e.g., wrong scheme, localhost, or malformed URL), the validation will fail but return the same invalid value as the fallback.

Attack scenario: A malicious or misconfigured config file sets telemetry_endpoint = "file:///tmp/steal-data". Without an env override, line 126 validates this but falls back to the same invalid value, bypassing the security checks in _validated_endpoint.

Apply this diff to use the hardcoded safe default as the fallback:

+        # Safe hardcoded fallback
+        SAFE_DEFAULT = "https://api-prod.coplay.dev/telemetry/events"
         # Telemetry endpoint (Cloud Run default; override via env)
         cfg_default = None if server_config is None else getattr(
             server_config, "telemetry_endpoint", None)
-        default_ep = cfg_default or "https://api-prod.coplay.dev/telemetry/events"
+        default_ep = cfg_default or SAFE_DEFAULT
         self.default_endpoint = default_ep
         # Prefer config default; allow explicit env override only when set
         env_ep = os.environ.get("UNITY_MCP_TELEMETRY_ENDPOINT")
         if env_ep is not None and env_ep != "":
-            self.endpoint = self._validated_endpoint(env_ep, default_ep)
+            self.endpoint = self._validated_endpoint(env_ep, SAFE_DEFAULT)
         else:
             # Validate config-provided default as well to enforce scheme/host rules
-            self.endpoint = self._validated_endpoint(default_ep, default_ep)
+            self.endpoint = self._validated_endpoint(default_ep, SAFE_DEFAULT)

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In MCPForUnity/UnityMcpServer~/src/telemetry.py around lines 120 to 126, the
call self._validated_endpoint(default_ep, default_ep) uses the config-provided
default as its own fallback; change the fallback to the hardcoded safe default
(not the possibly-malicious config value). Replace the second argument with the
project's hardcoded telemetry-safe constant (or the literal safe URL, e.g.,
"https://telemetry.unity.com") and ensure that constant is defined/imported;
this makes _validated_endpoint return the known-safe default when validation of
the config value fails.

try:
logger.info(
"Telemetry configured: endpoint=%s (default=%s), timeout_env=%s",
Expand Down
18 changes: 9 additions & 9 deletions MCPForUnity/UnityMcpServer~/src/tools/manage_script.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from mcp.server.fastmcp import FastMCP, Context

from registry import mcp_for_unity_tool
from unity_connection import send_command_with_retry
import unity_connection
Copy link

@greptile-apps greptile-apps bot Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: This import pattern differs from other tool files in the same directory (e.g., script_apply_edits.py, read_console.py, manage_editor.py) which still use from unity_connection import send_command_with_retry. While the module import works correctly and enables better test mocking, the inconsistency across the codebase could be confusing. Consider either:

  1. Applying this pattern to all tool files for consistency
  2. Documenting why manage_script.py needs this special import pattern
Prompt To Fix With AI 
This is a comment left during a code review.
Path: MCPForUnity/UnityMcpServer~/src/tools/manage_script.py
Line: 9:9

Comment:
**style:** This import pattern differs from other tool files in the same directory (e.g., `script_apply_edits.py`, `read_console.py`, `manage_editor.py`) which still use `from unity_connection import send_command_with_retry`. While the module import works correctly and enables better test mocking, the inconsistency across the codebase could be confusing. Consider either:
1. Applying this pattern to all tool files for consistency
2. Documenting why `manage_script.py` needs this special import pattern

How can I resolve this? If you propose a fix, please make it concise.



def _split_uri(uri: str) -> tuple[str, str]:
Expand Down Expand Up @@ -103,7 +103,7 @@ def _needs_normalization(arr: list[dict[str, Any]]) -> bool:
warnings: list[str] = []
if _needs_normalization(edits):
# Read file to support index->line/col conversion when needed
read_resp = send_command_with_retry("manage_script", {
read_resp = unity_connection.send_command_with_retry("manage_script", {
"action": "read",
"name": name,
"path": directory,
Expand Down Expand Up @@ -304,7 +304,7 @@ def _le(a: tuple[int, int], b: tuple[int, int]) -> bool:
"options": opts,
}
params = {k: v for k, v in params.items() if v is not None}
resp = send_command_with_retry("manage_script", params)
resp = unity_connection.send_command_with_retry("manage_script", params)
if isinstance(resp, dict):
data = resp.setdefault("data", {})
data.setdefault("normalizedEdits", normalized_edits)
Expand Down Expand Up @@ -336,7 +336,7 @@ def _flip_async():
st = _latest_status()
if st and st.get("reloading"):
return
send_command_with_retry(
unity_connection.send_command_with_retry(
"execute_menu_item",
{"menuPath": "MCP/Flip Reload Sentinel"},
max_retries=0,
Expand Down Expand Up @@ -386,7 +386,7 @@ def create_script(
contents.encode("utf-8")).decode("utf-8")
params["contentsEncoded"] = True
params = {k: v for k, v in params.items() if v is not None}
resp = send_command_with_retry("manage_script", params)
resp = unity_connection.send_command_with_retry("manage_script", params)
return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)}


Expand All @@ -401,7 +401,7 @@ def delete_script(
if not directory or directory.split("/")[0].lower() != "assets":
return {"success": False, "code": "path_outside_assets", "message": "URI must resolve under 'Assets/'."}
params = {"action": "delete", "name": name, "path": directory}
resp = send_command_with_retry("manage_script", params)
resp = unity_connection.send_command_with_retry("manage_script", params)
return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)}


Expand All @@ -426,7 +426,7 @@ def validate_script(
"path": directory,
"level": level,
}
resp = send_command_with_retry("manage_script", params)
resp = unity_connection.send_command_with_retry("manage_script", params)
if isinstance(resp, dict) and resp.get("success"):
diags = resp.get("data", {}).get("diagnostics", []) or []
warnings = sum(1 for d in diags if str(
Expand Down Expand Up @@ -473,7 +473,7 @@ def manage_script(

params = {k: v for k, v in params.items() if v is not None}

response = send_command_with_retry("manage_script", params)
response = unity_connection.send_command_with_retry("manage_script", params)

if isinstance(response, dict):
if response.get("success"):
Expand Down Expand Up @@ -541,7 +541,7 @@ def get_sha(
try:
name, directory = _split_uri(uri)
params = {"action": "get_sha", "name": name, "path": directory}
resp = send_command_with_retry("manage_script", params)
resp = unity_connection.send_command_with_retry("manage_script", params)
if isinstance(resp, dict) and resp.get("success"):
data = resp.get("data", {})
minimal = {"sha256": data.get(
Expand Down
2 changes: 1 addition & 1 deletion MCPForUnity/UnityMcpServer~/src/tools/read_console.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
)
def read_console(
ctx: Context,
action: Annotated[Literal['get', 'clear'], "Get or clear the Unity Editor console."],
action: Annotated[Literal['get', 'clear'], "Get or clear the Unity Editor console."] | None = None,
types: Annotated[list[Literal['error', 'warning',
'log', 'all']], "Message types to get"] | None = None,
count: Annotated[int, "Max messages to return"] | None = None,
Expand Down
8 changes: 8 additions & 0 deletions TestProjects/UnityMCPTests/Assets/Temp.meta
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions ...Projects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialMeshInstantiationTests.cs.meta
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions UnityMcpBridge/UnityMcpServer~/src/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
def pytest_ignore_collect(path, config):
# Avoid duplicate import mismatches between Bridge and MCPForUnity copies
p = str(path)
return p.endswith("test_telemetry.py")
18 changes: 9 additions & 9 deletions UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from mcp.server.fastmcp import FastMCP, Context

from registry import mcp_for_unity_tool
from unity_connection import send_command_with_retry
import unity_connection


def _split_uri(uri: str) -> tuple[str, str]:
Expand Down Expand Up @@ -103,7 +103,7 @@ def _needs_normalization(arr: list[dict[str, Any]]) -> bool:
warnings: list[str] = []
if _needs_normalization(edits):
# Read file to support index->line/col conversion when needed
read_resp = send_command_with_retry("manage_script", {
read_resp = unity_connection.send_command_with_retry("manage_script", {
"action": "read",
"name": name,
"path": directory,
Expand Down Expand Up @@ -304,7 +304,7 @@ def _le(a: tuple[int, int], b: tuple[int, int]) -> bool:
"options": opts,
}
params = {k: v for k, v in params.items() if v is not None}
resp = send_command_with_retry("manage_script", params)
resp = unity_connection.send_command_with_retry("manage_script", params)
if isinstance(resp, dict):
data = resp.setdefault("data", {})
data.setdefault("normalizedEdits", normalized_edits)
Expand Down Expand Up @@ -336,7 +336,7 @@ def _flip_async():
st = _latest_status()
if st and st.get("reloading"):
return
send_command_with_retry(
unity_connection.send_command_with_retry(
"execute_menu_item",
{"menuPath": "MCP/Flip Reload Sentinel"},
max_retries=0,
Expand Down Expand Up @@ -386,7 +386,7 @@ def create_script(
contents.encode("utf-8")).decode("utf-8")
params["contentsEncoded"] = True
params = {k: v for k, v in params.items() if v is not None}
resp = send_command_with_retry("manage_script", params)
resp = unity_connection.send_command_with_retry("manage_script", params)
return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)}


Expand All @@ -401,7 +401,7 @@ def delete_script(
if not directory or directory.split("/")[0].lower() != "assets":
return {"success": False, "code": "path_outside_assets", "message": "URI must resolve under 'Assets/'."}
params = {"action": "delete", "name": name, "path": directory}
resp = send_command_with_retry("manage_script", params)
resp = unity_connection.send_command_with_retry("manage_script", params)
return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)}


Expand All @@ -426,7 +426,7 @@ def validate_script(
"path": directory,
"level": level,
}
resp = send_command_with_retry("manage_script", params)
resp = unity_connection.send_command_with_retry("manage_script", params)
if isinstance(resp, dict) and resp.get("success"):
diags = resp.get("data", {}).get("diagnostics", []) or []
warnings = sum(1 for d in diags if str(
Expand Down Expand Up @@ -473,7 +473,7 @@ def manage_script(

params = {k: v for k, v in params.items() if v is not None}

response = send_command_with_retry("manage_script", params)
response = unity_connection.send_command_with_retry("manage_script", params)

if isinstance(response, dict):
if response.get("success"):
Expand Down Expand Up @@ -541,7 +541,7 @@ def get_sha(
try:
name, directory = _split_uri(uri)
params = {"action": "get_sha", "name": name, "path": directory}
resp = send_command_with_retry("manage_script", params)
resp = unity_connection.send_command_with_retry("manage_script", params)
if isinstance(resp, dict) and resp.get("success"):
data = resp.get("data", {})
minimal = {"sha256": data.get(
Expand Down
30 changes: 30 additions & 0 deletions docs/README-DEV.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,36 @@

Welcome to the MCP for Unity development environment! This directory contains tools and utilities to streamline MCP for Unity core development.

## 🛠️ Development Setup

### Installing Development Dependencies

To contribute or run tests, you need to install the development dependencies:

```bash
# Navigate to the server source directory
cd MCPForUnity/UnityMcpServer~/src

# Install the package in editable mode with dev dependencies
pip install -e .[dev]
```

This installs:
- **Runtime dependencies**: `httpx`, `mcp[cli]`, `pydantic`, `tomli`
- **Development dependencies**: `pytest`, `pytest-anyio`

### Running Tests

```bash
# From the repo root
pytest tests/ -v
```

Or if you prefer using Python module syntax:
```bash
python -m pytest tests/ -v
```

## 🚀 Available Development Features

### ✅ Development Deployment Scripts
Expand Down
4 changes: 4 additions & 0 deletions pytest.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
[pytest]
testpaths = tests
norecursedirs = UnityMcpBridge MCPForUnity

21 changes: 21 additions & 0 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,24 @@
os.environ.setdefault("DISABLE_TELEMETRY", "true")
os.environ.setdefault("UNITY_MCP_DISABLE_TELEMETRY", "true")
os.environ.setdefault("MCP_DISABLE_TELEMETRY", "true")

# Avoid collecting tests under the two 'src' package folders to prevent
# duplicate-package import conflicts (two different 'src' packages).
collect_ignore = [
"UnityMcpBridge/UnityMcpServer~/src",
"MCPForUnity/UnityMcpServer~/src",
]
collect_ignore_glob = [
"UnityMcpBridge/UnityMcpServer~/src/*",
"MCPForUnity/UnityMcpServer~/src/*",
]

def pytest_ignore_collect(path):
p = str(path)
norm = p.replace("\\", "/")
return (
"/UnityMcpBridge/UnityMcpServer~/src/" in norm
or "/MCPForUnity/UnityMcpServer~/src/" in norm
or norm.endswith("UnityMcpBridge/UnityMcpServer~/src")
or norm.endswith("MCPForUnity/UnityMcpServer~/src")
)
Loading