Chore/pytest harness tests only #85
Conversation
…om tests; route tools.manage_script via unity_connection for reliable monkeypatching; fix ctx usage; all tests green (39 pass, 5 skip, 7 xpass)
|
Warning Rate limit exceeded@dsarno has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (11)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR refactors command dispatch pathways in manage_script tools to route through the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes The PR exhibits consistent refactoring patterns across test files (tool registration, patching, context injection) but spans 20+ files with modest structural complexity. While individual changes follow predictable patterns, the breadth, coordination across disparate test utilities, and correctness of registry-based tool discovery warrant careful verification of consistency and interaction between dynamically loaded modules. Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Summary
Refactored pytest test harness to use MCPForUnity-only paths and improved monkeypatching reliability by changing import pattern in manage_script.py.
Key changes:
- Changed
manage_script.pyimport fromfrom unity_connection import send_command_with_retrytoimport unity_connectionin both MCPForUnity and UnityMcpBridge directories - Updated all 8
send_command_with_retry()call sites to useunity_connection.send_command_with_retry() - Refactored 14 test files to patch
unity_connection.send_command_with_retryat module level instead of attempting to patch imported functions - Updated tests to use decorator-based tool registration via registry pattern instead of direct registration
- Added
DummyContextmocks across all test files for proper context handling - Fixed stale file references:
manage_script_edits.py→script_apply_edits.py - Added missing Unity meta file for
MaterialMeshInstantiationTests.cs - Tests now passing: 39 pass, 5 skip, 7 xpass
Why this matters:
Python's import system creates independent bindings when using from X import Y. Monkeypatching the original module doesn't affect already-imported function references. The module import pattern (import unity_connection) ensures tests can reliably mock the connection layer.
Confidence Score: 4/5
- This PR is safe to merge with minimal risk - it's a refactoring of test infrastructure with working tests
- The changes are well-contained test infrastructure improvements. All tests are passing (39 pass, 5 skip, 7 xpass). The production code change (import pattern) is functionally equivalent and improves testability. However, there's an inconsistency: only
manage_script.pyuses the module import pattern while 8+ other tool files still use direct function imports, which could be confusing for future maintainers. - MCPForUnity/UnityMcpServer~/src/tools/manage_script.py - consider applying the module import pattern consistently across all tool files
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| MCPForUnity/UnityMcpServer~/src/tools/manage_script.py | 4/5 | Changed import from from unity_connection import send_command_with_retry to import unity_connection for better test monkeypatching; all 8 function calls updated to unity_connection.send_command_with_retry() |
| UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py | 4/5 | Identical import refactoring as MCPForUnity version; changed from function import to module import pattern |
| tests/test_edit_normalization_and_noop.py | 4/5 | Updated to patch unity_connection.send_command_with_retry at module level; replaced outdated manage_script_edits.py references with script_apply_edits.py; added DummyContext mock |
| tests/test_edit_strict_and_warnings.py | 4/5 | Updated monkeypatch strategy to patch unity_connection.send_command_with_retry instead of direct function import; added DummyContext mock; uses decorator-based tool registration |
| tests/test_manage_script_uri.py | 4/5 | Updated all tests to monkeypatch unity_connection.send_command_with_retry; switched from direct registration to decorator-based registry pattern; added DummyContext |
Sequence Diagram
sequenceDiagram
participant Test as Test Suite
participant Monkeypatch as pytest.monkeypatch
participant UnityConn as unity_connection
participant ManageScript as manage_script.py
participant Unity as Unity Editor
Note over Test,Unity: Before PR: Direct function import
Test->>Monkeypatch: patch manage_script.send_command_with_retry
Test->>ManageScript: call tool function
ManageScript->>ManageScript: send_command_with_retry() [patched]
Note over ManageScript: ❌ Patch doesn't work reliably
Note over Test,Unity: After PR: Module import pattern
Test->>Monkeypatch: patch unity_connection.send_command_with_retry
Test->>ManageScript: call tool function
ManageScript->>UnityConn: unity_connection.send_command_with_retry()
UnityConn->>UnityConn: [patched in tests]
Note over UnityConn: ✅ Patch works reliably
Note over Test,Unity: Production flow (unchanged)
ManageScript->>UnityConn: unity_connection.send_command_with_retry()
UnityConn->>Unity: send command
Unity-->>UnityConn: response
UnityConn-->>ManageScript: return response
ManageScript-->>Test: return result
16 files reviewed, 1 comment
|
|
||
| from registry import mcp_for_unity_tool | ||
| from unity_connection import send_command_with_retry | ||
| import unity_connection |
There was a problem hiding this 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:
- Applying this pattern to all tool files for consistency
- Documenting why
manage_script.pyneeds 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.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
429-429: Missing unity_connection prefix causes runtime error.Line 429 calls
send_command_with_retrydirectly without theunity_connection.prefix, while all other call sites in the file correctly useunity_connection.send_command_with_retry. This will cause a NameError at runtime.Apply this diff:
- resp = send_command_with_retry("manage_script", params) + resp = unity_connection.send_command_with_retry("manage_script", params)tests/test_manage_script_uri.py (1)
1-1: Tidy up unused symbols and fix lint on test doubles
- Line 1: manage_script alias is unused; remove the import to avoid F401 noise.
- Line 76: manage_script_module variable is unused; import without alias (for side‑effects) or remove.
- Lines 57-59: DummyCtx is unused; remove it.
- Lines 150-153: fake_send doesn’t use cmd; rename to _cmd to satisfy Ruff ARG001.
Apply these diffs:
- import tools.manage_script as manage_script # type: ignore + # import triggers registration elsewhere; no direct use here @@ - import tools.manage_script as manage_script_module + import tools.manage_script # trigger decorator registration @@ -class DummyCtx: # FastMCP Context placeholder - pass +# (removed unused DummyCtx) @@ - def fake_send(cmd, params): + def fake_send(_cmd, params): captured['params'] = params return {"success": True, "message": "ok"}Also applies to: 76-76, 57-59, 150-153
♻️ Duplicate comments (1)
tests/test_edit_normalization_and_noop.py (1)
50-60: Extract DummyContext to a shared test utility module.This is the same
DummyContextclass duplicated fromtests/test_resources_api.pyand other test files. Consolidate into a shared location.
🧹 Nitpick comments (16)
tests/test_telemetry_endpoint_validation.py (1)
10-17: Consider extracting duplicated setup logic.The sys.path manipulation and module import pattern is repeated across three test functions. Extracting this to a pytest fixture or helper function would improve maintainability.
Consider creating a fixture:
import pytest @pytest.fixture def telemetry_module(): import sys import pathlib ROOT = pathlib.Path(__file__).resolve().parents[1] SRC = ROOT / "MCPForUnity" / "UnityMcpServer~" / "src" sys.path.insert(0, str(SRC)) telemetry = importlib.import_module("telemetry") importlib.reload(telemetry) return telemetryThen use it in tests:
def test_endpoint_rejects_non_http(tmp_path, monkeypatch, telemetry_module): monkeypatch.setenv("XDG_DATA_HOME", str(tmp_path)) monkeypatch.setenv("UNITY_MCP_TELEMETRY_ENDPOINT", "file:///etc/passwd") tc = telemetry_module.TelemetryCollector() assert tc.config.endpoint == tc.config.default_endpointAlso applies to: 32-38, 60-67
tests/test_validate_script_summary.py (2)
65-75: Avoid double registration: prefer a single import pathYou already load manage_script via importlib earlier (Lines 36-38) and then import tools.manage_script here to trigger registration. This can register tools twice in the global registry. Prefer only the tools.manage_script import and drop the earlier importlib-based load for determinism.
Apply this removal outside the shown hunk:
- manage_script = _load_module( - SRC / "tools" / "manage_script.py", "manage_script_mod")
51-61: Consolidate DummyContext into shared fixture to reduce duplicationDummyContext is duplicated across 10 test modules (test_manage_script_uri.py, test_validate_script_summary.py, test_script_tools.py, test_read_resource_minimal.py, test_resources_api.py, test_read_console_truncate.py, test_get_sha.py, test_find_in_file_minimal.py, test_edit_strict_and_warnings.py, test_edit_normalization_and_noop.py). Move the identical class to tests/conftest.py and import it across all test modules to follow DRY principles and reduce maintenance overhead.
tests/test_manage_script_uri.py (1)
81-85: Narrow the tool filter to reduce accidental matchesThe substring 'script' is broad and may capture unintended tools. Consider using an explicit allow-list like {'apply_text_edits','create_script','delete_script','validate_script','get_sha','manage_script'} to keep registration precise.
tests/test_edit_strict_and_warnings.py (3)
50-57: Use a single import path to register toolsYou both import via importlib (_load at Line 37) and standard import here to trigger registration. Drop the earlier importlib load to avoid duplicated registry entries and unpredictable ordering.
58-65: Centralize DummyContextMove DummyContext to tests/conftest.py and reuse across the suite to eliminate duplication.
72-75: Quiet unused fake_send parametersThe test doubles don’t use cmd; rename to _cmd for clarity and to satisfy linters.
Apply:
- def fake_send(cmd, params): + def fake_send(_cmd, params): @@ - def fake_send(cmd, params): + def fake_send(_cmd, params):Also applies to: 98-100
tests/test_get_sha.py (2)
52-62: Deduplicate DummyContextSame suggestion as other files: extract to a shared location (tests/conftest.py).
66-76: Clarify variable naming and avoid shadowingtools = get_registered_tools() can be confusing alongside the tools package; use registered_tools to improve readability. Also, as in other files, avoid double registration by removing the earlier importlib load.
Apply:
- from registry import get_registered_tools - tools = get_registered_tools() + from registry import get_registered_tools + registered_tools = get_registered_tools() @@ - for tool_info in tools: + for tool_info in registered_tools:tests/test_script_tools.py (3)
56-66: Extract DummyContext to a shared helperSame DRY recommendation: define once in tests/conftest.py and reuse.
70-80: Factor out shared tool‑registration helpersetup_manage_script and setup_manage_asset share nearly identical registry wiring with different filters. Consider a small helper like setup_tools(filter_pred) to remove duplication; keep explicit allow‑lists for determinism.
Also applies to: 85-95
209-212: Prefer pytest-asyncio over asyncio.run for async testMark the test as async and use pytest-asyncio to avoid nesting event loops and to integrate with pytest’s async lifecycle.
Example:
- async def run(): - resp = await manage_asset( - DummyContext(), - action="modify", - path="Assets/Prefabs/Player.prefab", - properties={"hp": 100}, - ) - ... - asyncio.run(run()) + import pytest + @pytest.mark.asyncio + async def test_manage_asset_prefab_modify_request(monkeypatch): + tools = setup_manage_asset() + manage_asset = tools["manage_asset"] + ... + resp = await manage_asset(DummyContext(), action="modify", path="Assets/Prefabs/Player.prefab", properties={"hp": 100}) + ...tests/test_resources_api.py (2)
2-7: Remove duplicate pytest import.The
pytestmodule is imported twice (lines 2 and 7), which is redundant.Apply this diff:
-import pytest - - import sys from pathlib import Path import pytest
48-51: Consider more precise tool name matching.The substring matching pattern
if any(keyword in tool_name for keyword in [...])could match unintended tools. For example, a tool named"prefix_find_in_file_suffix"would match when searching for"find_in_file".Consider using exact name matching or name suffix/prefix checks:
- if any(keyword in tool_name for keyword in ['find_in_file', 'list_resources', 'read_resource']): + if tool_name in ['find_in_file', 'list_resources', 'read_resource']: mcp._tools[tool_name] = tool_info['func']Or if tools have a consistent naming pattern:
if any(tool_name.endswith(keyword) for keyword in ['find_in_file', 'list_resources', 'read_resource']):tests/test_edit_normalization_and_noop.py (2)
70-73: Substring matching with 'script' is overly broad.The keyword
'script'will match any tool name containing "script" anywhere in the name (e.g.,"describe_script","typescript_tool","postscript_handler"). This is more fragile than the matching intest_resources_api.py.Consider using more specific patterns or exact matching:
- if any(keyword in tool_name for keyword in ['script', 'apply_text', 'create_script', 'delete_script', 'validate_script', 'get_sha']): + # Match only specific tool names we need + script_tools = {'manage_script', 'apply_text_edits', 'create_script', 'delete_script', 'validate_script', 'get_sha'} + if tool_name in script_tools or any(tool_name.endswith(suffix) for suffix in ['_script', '_sha']): mcp.tools[tool_name] = tool_info['func']
143-152: Consider extracting duplicate tool discovery logic.This code block duplicates the registry-based tool discovery pattern from
setup_tools(). If this pattern is used frequently, consider creating a helper function.def load_tools_from_registry(module_name, keywords): """Load tools from registry matching given keywords.""" mcp = DummyMCP() __import__(module_name) from registry import get_registered_tools for tool_info in get_registered_tools(): tool_name = tool_info['name'] if any(keyword in tool_name for keyword in keywords): mcp.tools[tool_name] = tool_info['func'] return mcp.toolsThen use:
tools_struct_dict = load_tools_from_registry('tools.script_apply_edits', ['script_apply', 'apply_edits'])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
MCPForUnity/UnityMcpServer~/src/tools/manage_script.py(9 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialMeshInstantiationTests.cs.meta(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(8 hunks)tests/test_edit_normalization_and_noop.py(7 hunks)tests/test_edit_strict_and_warnings.py(3 hunks)tests/test_find_in_file_minimal.py(2 hunks)tests/test_get_sha.py(2 hunks)tests/test_improved_anchor_matching.py(1 hunks)tests/test_manage_script_uri.py(2 hunks)tests/test_read_console_truncate.py(3 hunks)tests/test_read_resource_minimal.py(2 hunks)tests/test_resources_api.py(5 hunks)tests/test_script_tools.py(6 hunks)tests/test_telemetry_endpoint_validation.py(3 hunks)tests/test_telemetry_subaction.py(1 hunks)tests/test_validate_script_summary.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
tests/test_script_tools.py (3)
tests/test_edit_normalization_and_noop.py (3)
info(52-53)error(58-59)DummyMCP(42-47)MCPForUnity/UnityMcpServer~/src/tools/manage_script.py (1)
manage_script(443-500)UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (1)
manage_asset(15-83)
tests/test_get_sha.py (2)
tests/test_edit_normalization_and_noop.py (3)
info(52-53)error(58-59)DummyMCP(42-47)MCPForUnity/UnityMcpServer~/src/tools/manage_script.py (2)
manage_script(443-500)get_sha(536-552)
tests/test_read_resource_minimal.py (1)
tests/test_find_in_file_minimal.py (6)
DummyContext(24-33)info(26-27)warning(29-30)error(32-33)resource_tools(37-49)DummyMCP(13-21)
tests/test_find_in_file_minimal.py (1)
tests/test_read_resource_minimal.py (3)
info(43-44)resource_tools(54-66)DummyMCP(30-38)
tests/test_edit_normalization_and_noop.py (3)
tests/test_edit_strict_and_warnings.py (6)
DummyContext(58-64)info(59-60)warning(61-62)error(63-64)setup_tools(48-57)DummyMCP(40-45)tests/test_find_in_file_minimal.py (5)
DummyContext(24-33)info(26-27)warning(29-30)error(32-33)DummyMCP(13-21)MCPForUnity/UnityMcpServer~/src/tools/manage_script.py (1)
manage_script(443-500)
tests/test_manage_script_uri.py (2)
tests/test_edit_normalization_and_noop.py (5)
DummyContext(50-59)info(52-53)warning(55-56)error(58-59)DummyMCP(42-47)MCPForUnity/UnityMcpServer~/src/tools/manage_script.py (1)
manage_script(443-500)
tests/test_edit_strict_and_warnings.py (2)
MCPForUnity/UnityMcpServer~/src/tools/manage_script.py (1)
manage_script(443-500)tests/test_edit_normalization_and_noop.py (4)
DummyContext(50-59)info(52-53)warning(55-56)error(58-59)
tests/test_read_console_truncate.py (2)
tests/test_edit_normalization_and_noop.py (6)
DummyContext(50-59)info(52-53)warning(55-56)error(58-59)setup_tools(62-74)DummyMCP(42-47)UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py (1)
read_console(14-87)
tests/test_resources_api.py (2)
tests/test_find_in_file_minimal.py (4)
info(26-27)error(32-33)resource_tools(37-49)DummyMCP(13-21)MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py (1)
list_resources(137-186)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
tests/test_validate_script_summary.py (2)
tests/test_edit_normalization_and_noop.py (5)
info(52-53)warning(55-56)error(58-59)setup_tools(62-74)DummyMCP(42-47)MCPForUnity/UnityMcpServer~/src/tools/manage_script.py (2)
manage_script(443-500)validate_script(409-439)
🪛 Ruff (0.14.1)
tests/test_manage_script_uri.py
150-150: Unused function argument: cmd
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (20)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialMeshInstantiationTests.cs.meta (1)
1-11: Verified as standard auto-generated Unity metadata.This .meta file is auto-generated by Unity's asset database and follows the standard format for test assets. No review action needed.
tests/test_telemetry_subaction.py (1)
4-13: LGTM!The shift from fully-qualified module path to dynamic sys.path-based import is consistent with the broader PR refactoring pattern.
tests/test_find_in_file_minimal.py (2)
24-34: LGTM!The DummyContext mock provides a clean way to satisfy context requirements in tests without side effects.
39-49: LGTM!The registry-based dynamic tool discovery aligns with the PR's standardization objectives and improves test maintainability.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
9-9: LGTM!The module-level import of unity_connection centralizes the command dispatch pathway as intended by the PR.
tests/test_read_resource_minimal.py (1)
41-51: LGTM!The DummyContext introduction and registry-based tool discovery follow the established pattern from other test files and support the PR's standardization goals.
Also applies to: 56-66, 82-82
tests/test_read_console_truncate.py (3)
51-61: LGTM!DummyContext addition is consistent with the test infrastructure standardization across the PR.
63-75: LGTM!The registry-based tool discovery replaces static registrations and aligns with the dynamic loading pattern introduced throughout the test suite.
92-96: LGTM!The patches now correctly target the tools.read_console module, and the addition of DummyContext and action parameter makes the tests more robust and aligned with the actual function signatures.
Also applies to: 119-123
MCPForUnity/UnityMcpServer~/src/tools/manage_script.py (1)
9-9: LGTM!All command dispatch calls correctly route through
unity_connection.send_command_with_retry, successfully centralizing the communication pathway as intended by the PR refactoring.Also applies to: 106-106, 307-307, 339-339, 389-389, 404-404, 429-429, 476-476, 544-544
tests/test_validate_script_summary.py (1)
94-101: Right patch target for command dispatch — LGTMPatching unity_connection.send_command_with_retry is correct given manage_script references it via the module. The context injection with DummyContext aligns with the tool signature.
tests/test_manage_script_uri.py (1)
97-101: Unified patching approach — LGTMPatching unity_connection.send_command_with_retry at the import site is consistent and robust across tests. No need to patch tools.manage_script directly anymore.
Also applies to: 133-137, 155-159
tests/test_edit_strict_and_warnings.py (1)
76-78: Patch target correctness — LGTMPatching unity_connection.send_command_with_retry is the right abstraction point after the refactor.
Also applies to: 101-103
tests/test_get_sha.py (1)
90-96: Routing and assertions — LGTMPatching the central unity_connection function and asserting minimalized response shape from get_sha are correct and aligned with manage_script behavior.
tests/test_script_tools.py (1)
108-113: Patch sites — LGTM
- Managing script edits: patch unity_connection.send_command_with_retry.
- Managing assets: patch tools.manage_asset.async_send_command_with_retry directly.
Both targets match their respective call sites.Also applies to: 133-138, 160-165
tests/test_resources_api.py (2)
75-82: Excellent test isolation with explicit event loop management.Creating a dedicated event loop for each test and ensuring proper cleanup with
try/finallyis a best practice that prevents test interference.
95-102: Consistent event loop management applied.The same best practice pattern is correctly applied here as in the previous test.
tests/test_edit_normalization_and_noop.py (3)
86-91: Centralized patching approach aligns with PR objectives.Patching
unity_connection.send_command_with_retryat the module level is the correct approach and aligns with the PR's goal of routing all command dispatches through theunity_connectionmodule.
97-97: Context-aware test calls improve accuracy.Passing
DummyContext()instead ofNoneensures the code under test receives a context object with the expected interface, making tests more representative of production behavior.Also applies to: 115-115, 133-133, 172-172
112-114: Consistent patching strategy maintained throughout tests.All test functions consistently patch
unity_connection.send_command_with_retry, maintaining a unified approach to mocking Unity command dispatch.Also applies to: 127-131, 162-165
…lidate_script; tests: tidy manage_script_uri unused symbols and arg names
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_manage_script_uri.py (1)
72-84: Consider more robust tool filtering.The keyword-based substring matching on line 82 could be fragile:
- False positives if unrelated tools contain these keywords
- Requires updating keywords when tool names change
- Not explicit about which tools are needed
Consider one of these alternatives:
Option 1: Explicit tool name list
- for tool_info in registered_tools: - tool_name = tool_info['name'] - if any(keyword in tool_name for keyword in ['script', 'apply_text', 'create_script', 'delete_script', 'validate_script', 'get_sha']): - mcp.tools[tool_name] = tool_info['func'] + required_tools = ['apply_text_edits', 'create_script', 'delete_script', 'validate_script', 'manage_script', 'get_sha'] + for tool_info in registered_tools: + tool_name = tool_info['name'] + if tool_name in required_tools: + mcp.tools[tool_name] = tool_info['func']Option 2: Registry tags/categories
If the registry supports tagging, filter by category instead of name matching.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py(9 hunks)tests/test_manage_script_uri.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(416-436)
tests/test_manage_script_uri.py (2)
tests/test_find_in_file_minimal.py (5)
DummyContext(24-33)info(26-27)warning(29-30)error(32-33)DummyMCP(13-21)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
manage_script(443-500)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (6)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (2)
9-9: LGTM: Clean refactor to module-level import.This change from a direct function import to a module import makes the code easier to test by providing a clear patching target at the module level.
106-106: LGTM: All call sites consistently updated.All 8 call sites across the file (in apply_text_edits, create_script, delete_script, validate_script, manage_script, and get_sha) have been consistently updated to use the module-qualified call pattern. No functional changes detected.
Also applies to: 307-307, 339-339, 389-389, 404-404, 429-429, 476-476, 544-544
tests/test_manage_script_uri.py (4)
1-1: LGTM: Helpful documentation comment.The comment clarifies the side-effect nature of the import, which is valuable for understanding the registry-based approach.
57-69: LGTM: Standardized test context.Good cleanup removing the unused DummyCtx and adding DummyContext that matches the pattern from test_find_in_file_minimal.py. This standardization improves test consistency.
87-109: LGTM: Test correctly aligned with production changes.The test now patches
unity_connection.send_command_with_retryinstead of the module-level import, which correctly matches the refactored production code. The use of DummyContext is also consistent with the standardized approach.
111-164: LGTM: Consistent test updates across all test cases.All three test functions (test_split_uri_unity_path, test_split_uri_file_urls, test_split_uri_plain_path) have been consistently updated to use the registry-based tool discovery and patch the correct unity_connection module. The test logic and assertions remain unchanged.
…ests/test_helpers and import; add telemetry stubs in tests to avoid pyproject I/O
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_edit_strict_and_warnings.py (1)
58-59: Remove redundant manual module load; risks duplicate registration._manual load introduces a second module instance (“manage_script_mod3”) and can double-register tools via decorators. It’s unused and may cause flakiness.
-manage_script = _load(SRC / "tools" / "manage_script.py", "manage_script_mod3") -tests/test_resources_api.py (1)
2-7: Remove duplicate pytest import.Two imports exist; keep one.
-import pytest - - -import sys +import sys from pathlib import Path -import pytest +import pytest
♻️ Duplicate comments (1)
tests/test_resources_api.py (1)
48-49: DRY win: DummyContext is now shared.Previous duplication is resolved by importing from tests.test_helpers. Good change.
🧹 Nitpick comments (11)
tests/test_helpers.py (1)
1-10: Make DummyContext capture messages and host shared stub installers.Capturing messages aids assertions; centralizing telemetry/mcp stubs reduces duplication across tests.
Apply to DummyContext:
-class DummyContext: - """Mock context object for testing""" - def info(self, message): - pass - - def warning(self, message): - pass - - def error(self, message): - pass +class DummyContext: + """Mock context object for testing.""" + def __init__(self): + self.infos: list[str] = [] + self.warnings: list[str] = [] + self.errors: list[str] = [] + + def info(self, message): + self.infos.append(str(message)) + + def warning(self, message): + self.warnings.append(str(message)) + + def error(self, message): + self.errors.append(str(message))Optionally add shared installers below (new code):
# Shared helpers to stub external modules in tests def install_telemetry_stubs(): import sys, types telemetry = types.ModuleType("telemetry") def _noop(*_args, **_kwargs): pass class MilestoneType: pass telemetry.record_resource_usage = _noop telemetry.record_tool_usage = _noop telemetry.record_milestone = _noop telemetry.MilestoneType = MilestoneType telemetry.get_package_version = lambda: "0.0.0" sys.modules["telemetry"] = telemetry telemetry_decorator = types.ModuleType("telemetry_decorator") def telemetry_tool(*_args, **_kwargs): def _wrap(fn): return fn return _wrap telemetry_decorator.telemetry_tool = telemetry_tool sys.modules["telemetry_decorator"] = telemetry_decorator def install_mcp_fastmcp_stubs(): import sys, types mcp_pkg = types.ModuleType("mcp") server_pkg = types.ModuleType("mcp.server") fastmcp_pkg = types.ModuleType("mcp.server.fastmcp") class _Dummy: pass fastmcp_pkg.FastMCP = _Dummy fastmcp_pkg.Context = _Dummy server_pkg.fastmcp = fastmcp_pkg mcp_pkg.server = server_pkg sys.modules["mcp"] = mcp_pkg sys.modules["mcp.server"] = server_pkg sys.modules["mcp.server.fastmcp"] = fastmcp_pkgtests/test_find_in_file_minimal.py (3)
14-26: Tighten stubs: avoid ARG001 warnings and ensure override.
- Rename unused args to “*_”/“**__”.
- Assign into sys.modules unconditionally to prevent leaking the real telemetry on reordered runs.
-telemetry = types.ModuleType("telemetry") -def _noop(*args, **kwargs): +telemetry = types.ModuleType("telemetry") +def _noop(*_args, **_kwargs): pass @@ -telemetry.get_package_version = lambda: "0.0.0" -sys.modules.setdefault("telemetry", telemetry) +telemetry.get_package_version = lambda: "0.0.0" +sys.modules["telemetry"] = telemetry @@ -telemetry_decorator = types.ModuleType("telemetry_decorator") -def telemetry_tool(*dargs, **dkwargs): +telemetry_decorator = types.ModuleType("telemetry_decorator") +def telemetry_tool(*_args, **_kwargs): def _wrap(fn): return fn return _wrap -telemetry_decorator.telemetry_tool = telemetry_tool -sys.modules.setdefault("telemetry_decorator", telemetry_decorator) +telemetry_decorator.telemetry_tool = telemetry_tool +sys.modules["telemetry_decorator"] = telemetry_decoratorAlso applies to: 27-34
53-60: Avoid shadowing the tools package for clarity.Rename local “tools” list from the registry.
- from registry import get_registered_tools - tools = get_registered_tools() + from registry import get_registered_tools + registered_tools = get_registered_tools() @@ - for tool_info in tools: + for tool_info in registered_tools:
70-77: Simplify event loop usage with asyncio.run.Use asyncio.run for readability and fewer moving parts.
-loop = asyncio.new_event_loop() -try: - resp = loop.run_until_complete( - find_in_file(uri="unity://path/Assets/A.txt", - pattern="world", ctx=DummyContext(), project_root=str(proj)) - ) -finally: - loop.close() +resp = asyncio.run( + find_in_file( + uri="unity://path/Assets/A.txt", + pattern="world", + ctx=DummyContext(), + project_root=str(proj), + ) +)tests/test_edit_strict_and_warnings.py (4)
11-23: Harden stubs (unused args + unconditional injection).Same rationale as other tests: avoid ARG001 and prevent accidental real imports.
-telemetry = types.ModuleType("telemetry") -def _noop(*args, **kwargs): +telemetry = types.ModuleType("telemetry") +def _noop(*_args, **_kwargs): pass @@ -telemetry.get_package_version = lambda: "0.0.0" -sys.modules.setdefault("telemetry", telemetry) +telemetry.get_package_version = lambda: "0.0.0" +sys.modules["telemetry"] = telemetry @@ -telemetry_decorator = types.ModuleType("telemetry_decorator") -def telemetry_tool(*dargs, **dkwargs): +telemetry_decorator = types.ModuleType("telemetry_decorator") +def telemetry_tool(*_args, **_kwargs): def _wrap(fn): return fn return _wrap -telemetry_decorator.telemetry_tool = telemetry_tool -sys.modules.setdefault("telemetry_decorator", telemetry_decorator) +telemetry_decorator.telemetry_tool = telemetry_tool +sys.modules["telemetry_decorator"] = telemetry_decoratorAlso applies to: 24-31
32-49: Centralize fastmcp stubs; avoid setdefault.Use a shared helper (e.g., tests.test_helpers.install_mcp_fastmcp_stubs()) and assign into sys.modules to guarantee the stub wins regardless of import order.
Example:
from tests.test_helpers import install_mcp_fastmcp_stubs install_mcp_fastmcp_stubs()
71-78: Small cleanup in setup_tools.
- Keep only registry import; rename local to avoid shadowing.
- import tools.manage_script - from registry import get_registered_tools - for tool_info in get_registered_tools(): + import tools.manage_script # triggers decorator registration + from registry import get_registered_tools + for tool_info in get_registered_tools(): name = tool_info['name'] if any(k in name for k in ['script', 'apply_text', 'create_script', 'delete_script', 'validate_script', 'get_sha']): mcp.tools[name] = tool_info['func']
96-97: Prefer keyword for ctx for readability.Make context explicit at call sites.
-resp = apply_edits(DummyContext(), uri="unity://path/Assets/Scripts/F.cs", +resp = apply_edits(ctx=DummyContext(), uri="unity://path/Assets/Scripts/F.cs", @@ -resp = apply_edits(DummyContext(), uri="unity://path/Assets/Scripts/F.cs", +resp = apply_edits(ctx=DummyContext(), uri="unity://path/Assets/Scripts/F.cs",Also applies to: 120-121
tests/test_resources_api.py (3)
15-27: Same stub improvements as elsewhere (ARG001 + unconditional injection).Align with other tests.
-def _noop(*args, **kwargs): +def _noop(*_args, **_kwargs): pass @@ -telemetry.get_package_version = lambda: "0.0.0" -sys.modules.setdefault("telemetry", telemetry) +telemetry.get_package_version = lambda: "0.0.0" +sys.modules["telemetry"] = telemetry @@ -def telemetry_tool(*dargs, **dkwargs): +def telemetry_tool(*_args, **_kwargs): def _wrap(fn): return fn return _wrap -telemetry_decorator.telemetry_tool = telemetry_tool -sys.modules.setdefault("telemetry_decorator", telemetry_decorator) +telemetry_decorator.telemetry_tool = telemetry_tool +sys.modules["telemetry_decorator"] = telemetry_decoratorAlso applies to: 28-35
57-64: Avoid shadowing the tools package name.Rename local variable from the registry for clarity.
- from registry import get_registered_tools - tools = get_registered_tools() + from registry import get_registered_tools + registered_tools = get_registered_tools() @@ - for tool_info in tools: + for tool_info in registered_tools:
87-95: Use asyncio.run in tests to simplify loop handling.Fewer lines and no manual close needed.
- import asyncio - loop = asyncio.new_event_loop() - try: - resp = loop.run_until_complete( - list_resources(ctx=DummyContext(), pattern="*.cs", under="Assets", - limit=50, project_root=str(proj)) - ) - finally: - loop.close() + import asyncio + resp = asyncio.run( + list_resources( + ctx=DummyContext(), pattern="*.cs", under="Assets", limit=50, project_root=str(proj) + ) + ) @@ - import asyncio - loop = asyncio.new_event_loop() - try: - resp = loop.run_until_complete( - list_resources(ctx=DummyContext(), pattern="*.cs", under="..", - limit=10, project_root=str(proj)) - ) - finally: - loop.close() + import asyncio + resp = asyncio.run( + list_resources( + ctx=DummyContext(), pattern="*.cs", under="..", limit=10, project_root=str(proj) + ) + )Also applies to: 107-115
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/test_edit_strict_and_warnings.py(4 hunks)tests/test_find_in_file_minimal.py(3 hunks)tests/test_helpers.py(1 hunks)tests/test_improved_anchor_matching.py(4 hunks)tests/test_resources_api.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_improved_anchor_matching.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_find_in_file_minimal.py (1)
tests/test_helpers.py (1)
DummyContext(1-10)
tests/test_edit_strict_and_warnings.py (2)
MCPForUnity/UnityMcpServer~/src/tools/manage_script.py (1)
manage_script(443-500)tests/test_helpers.py (1)
DummyContext(1-10)
tests/test_resources_api.py (4)
MCPForUnity/UnityMcpServer~/src/telemetry.py (6)
MilestoneType(65-73)record_resource_usage(450-468)record_tool_usage(421-447)record_milestone(267-289)record_milestone(416-418)get_package_version(39-46)MCPForUnity/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)tests/test_helpers.py (1)
DummyContext(1-10)MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py (1)
list_resources(137-186)
🪛 Ruff (0.14.1)
tests/test_find_in_file_minimal.py
28-28: Unused function argument: dargs
(ARG001)
28-28: Unused function argument: dkwargs
(ARG001)
tests/test_edit_strict_and_warnings.py
25-25: Unused function argument: dargs
(ARG001)
25-25: Unused function argument: dkwargs
(ARG001)
tests/test_resources_api.py
29-29: Unused function argument: dargs
(ARG001)
29-29: Unused function argument: dkwargs
(ARG001)
…config and opt-in env override; test stubs and CWD fixes; exclude Bridge from pytest discovery
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
tests/test_resources_api.py (1)
25-28: Consider suppressing the unused argument warning.The unused
dargsanddkwargsparameters are intentional for the no-op decorator stub. You can suppress the static analysis warning by prefixing with underscores or adding a suppression comment.Apply this diff to suppress the warning:
-def telemetry_tool(*dargs, **dkwargs): +def telemetry_tool(*_args, **_kwargs): def _wrap(fn): return fn return _wraptests/test_edit_strict_and_warnings.py (3)
76-76: Consider moving import to the top of the file.The import is placed after function definitions, which is non-idiomatic. Moving it to the top (around line 10) would improve readability and follow Python conventions.
Apply this diff to move the import:
+from tests.test_helpers import DummyContext + ROOT = pathlib.Path(__file__).resolve().parents[1] SRC = ROOT / "MCPForUnity" / "UnityMcpServer~" / "src" sys.path.insert(0, str(SRC))And remove line 76:
return mcp.tools -from tests.test_helpers import DummyContext
87-88: Import placement within test function is non-idiomatic.While functional, importing
unity_connectioninside the test function (line 87) is unusual. This pattern may be necessary to ensure the module is imported after stubs are set up, but consider if it could be moved to the module level after the stub setup (after line 48).If the import doesn't need to wait until test execution, consider moving it to the top:
sys.modules.setdefault("mcp.server", server_pkg) sys.modules.setdefault("mcp.server.fastmcp", fastmcp_pkg) +import unity_connectionThen remove the import from lines 87 and 112.
66-75: Consider making tool selection more explicit rather than keyword-based.The registry module is properly implemented and
get_registered_tools()works correctly. However, the substring matching on line 73 is fragile: using the broad keyword'script'could unintentionally include new tools added tomanage_script.py(e.g.,download_script,upload_script).The current approach works because the only matching tool is the intended
manage_script, but a more maintainable solution would explicitly specify tool names rather than filtering by substring keywords, or define the expected tool list as a configuration constant.MCPForUnity/UnityMcpServer~/src/telemetry.py (1)
96-101: Config import search order: confirm no unintended shadowing.Preferring plain
config/src.configis fine for tests, but could import an unrelated top‑levelconfigmodule in user envs. Consider scoping via a known base path or env var (e.g., UNITY_MCP_CONFIG_DIR) withspec_from_file_locationto reduce ambiguity.tests/test_telemetry_queue_worker.py (2)
63-68: Queue swap relies on suppressed task_done; acceptable but brittle.Reassigning
collector._queuemid‑loop can mismatchget()vstask_done(). It works because exceptions are suppressed, but it’s fragile. If feasible, expose queue size as a constructor arg or factory for tests to avoid swapping live queues.
77-84: Tight timing threshold may flake on CI.50 enqueues + logging can exceed 80 ms under load. Consider a looser bound or asserting non‑blocking behavior via log drops/backpressure only.
Apply:
- assert elapsed_ms < 80.0 + assert elapsed_ms < 200.0tests/test_telemetry_subaction.py (2)
6-12: Guard sys.path mutation to avoid duplicate inserts; optional cleanup.Insert only if absent, and consider removing after import to limit cross‑test effects.
Apply:
- sys.path.insert(0, str(SRC)) + if str(SRC) not in sys.path: + sys.path.insert(0, str(SRC))
15-27: Minimal telemetry stub: OK; add cleanup to reduce bleed‑through.After importing
telemetry_decorator, you cansys.modules.pop("telemetry", None)to avoid accidental reuse by other tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.wt-origin-main(1 hunks)MCPForUnity/UnityMcpServer~/src/telemetry.py(2 hunks)TestProjects/UnityMCPTests/Assets/Temp.meta(1 hunks)UnityMcpBridge/UnityMcpServer~/src/conftest.py(1 hunks)pytest.ini(1 hunks)tests/conftest.py(1 hunks)tests/test_edit_strict_and_warnings.py(4 hunks)tests/test_resources_api.py(4 hunks)tests/test_telemetry_endpoint_validation.py(3 hunks)tests/test_telemetry_queue_worker.py(3 hunks)tests/test_telemetry_subaction.py(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- .wt-origin-main
- pytest.ini
- TestProjects/UnityMCPTests/Assets/Temp.meta
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_telemetry_endpoint_validation.py
🧰 Additional context used
🧬 Code graph analysis (6)
tests/conftest.py (1)
UnityMcpBridge/UnityMcpServer~/src/conftest.py (1)
pytest_ignore_collect(1-4)
tests/test_edit_strict_and_warnings.py (4)
tests/test_resources_api.py (3)
_noop(13-14)MilestoneType(15-16)telemetry_tool(25-28)tests/test_find_in_file_minimal.py (3)
_noop(16-17)MilestoneType(18-19)telemetry_tool(28-31)MCPForUnity/UnityMcpServer~/src/tools/manage_script.py (1)
manage_script(443-500)tests/test_helpers.py (1)
DummyContext(1-10)
tests/test_telemetry_subaction.py (1)
MCPForUnity/UnityMcpServer~/src/telemetry.py (5)
MilestoneType(65-73)record_tool_usage(424-450)record_milestone(270-292)record_milestone(419-421)get_package_version(39-46)
UnityMcpBridge/UnityMcpServer~/src/conftest.py (1)
tests/conftest.py (1)
pytest_ignore_collect(20-27)
tests/test_resources_api.py (2)
tests/test_helpers.py (1)
DummyContext(1-10)MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py (1)
list_resources(137-186)
tests/test_telemetry_queue_worker.py (2)
MCPForUnity/UnityMcpServer~/src/telemetry.py (1)
get_package_version(39-46)UnityMcpBridge/UnityMcpServer~/src/telemetry.py (1)
get_package_version(37-44)
🪛 Ruff (0.14.1)
tests/conftest.py
20-20: Unused function argument: config
(ARG001)
tests/test_edit_strict_and_warnings.py
25-25: Unused function argument: dargs
(ARG001)
25-25: Unused function argument: dkwargs
(ARG001)
UnityMcpBridge/UnityMcpServer~/src/conftest.py
1-1: Unused function argument: config
(ARG001)
tests/test_resources_api.py
25-25: Unused function argument: dargs
(ARG001)
25-25: Unused function argument: dkwargs
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (9)
UnityMcpBridge/UnityMcpServer~/src/conftest.py (1)
1-4: No changes required; implementation is correct as-is.The verification confirms there is only one
test_telemetry.pyfile in the directory tree (UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py). The broad match pattern in the implementation is therefore appropriate and unambiguous—there is no risk of unintended file skips. The unusedconfigparameter is correctly retained as it is required by pytest's hook signature.tests/test_resources_api.py (4)
44-44: Excellent refactor—addresses duplication concern from previous review.Importing
DummyContextfrom the sharedtests.test_helpersmodule eliminates the code duplication flagged in the past review comments. This follows the DRY principle.
47-60: Registry-based tool discovery looks correct.The fixture properly imports the tools module to trigger decorator registration, then filters tools from the registry by name. The keyword-based filtering on line 58 is appropriate for test isolation, though it relies on stable naming conventions.
83-90: LGTM—proper async test pattern.The explicit event loop creation and cleanup in a try/finally block is the correct pattern for testing async functions. The use of
DummyContext()aligns with the broader test refactoring.
103-110: LGTM—consistent async handling.The event loop handling here mirrors the pattern in the first test and correctly ensures cleanup via the finally block.
tests/test_edit_strict_and_warnings.py (3)
11-30: LGTM! Telemetry stubs align with test patterns.The telemetry stub modules correctly avoid file I/O during imports. The unused parameters flagged by static analysis in the
telemetry_tooldecorator (line 25) are intentional for a no-op stub and can be safely ignored.
93-94: LGTM! DummyContext usage aligns with context-aware tool invocation.The test correctly passes
DummyContext()as the first argument toapply_edits, which supports the new context-aware tool calling pattern.
117-118: LGTM! Consistent with context-aware pattern.The test correctly uses
DummyContext()and the updated monkeypatch target, consistent with the changes in the first test.MCPForUnity/UnityMcpServer~/src/telemetry.py (1)
120-125: Env override semantics improved.Treating empty env as “unset” avoids surprising fallbacks. Looks good.
…ore hook); telemetry: validate config endpoint; read_console: action optional
- Add [project.optional-dependencies] section with dev group - Include pytest>=8.0.0 and pytest-anyio>=0.6.0 - Add Development Setup section to README-DEV.md with installation and testing instructions
This reverts commit 8ae595d.
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 (1)
tests/test_edit_strict_and_warnings.py (1)
53-57: Remove unused_loadfunction.The
_loadfunction is no longer used after refactoring to dynamic tool registration insetup_tools. This is dead code that should be removed.Apply this diff to remove the unused function:
-def _load(path: pathlib.Path, name: str): - spec = importlib.util.spec_from_file_location(name, path) - mod = importlib.util.module_from_spec(spec) - spec.loader.exec_module(mod) - return mod - -
🧹 Nitpick comments (5)
MCPForUnity/UnityMcpServer~/src/pyproject.toml (1)
14-18: Consider tightening version constraints for reproducible builds.The optional dev dependencies use lower-bound versioning only, which allows drift to potentially incompatible future releases. For reproducible test environments, consider adding upper bounds or pinned versions.
Additionally, verify that pytest and pytest-anyio are the complete testing toolkit needed. The AI summary mentions mocking patterns (
DummyContext, patchingunity_connection); ensure any required testing utilities (e.g., pytest-mock) are included, or confirm they are satisfied transitively.You can verify compatibility and inspect transitive dependencies by running:
#!/bin/bash # Description: Verify pytest and pytest-anyio compatibility and inspect transitive dependencies. # Show dependency tree for the dev group pip index versions pytest pytest-anyio | grep -A 5 "Available versions" # Check if a test configuration exists find . -name "pytest.ini" -o -name "pyproject.toml" -o -name "conftest.py" | head -10tests/conftest.py (1)
11-18: Consider removing redundant collection filters.Three separate mechanisms exclude the same directories:
collect_ignore,collect_ignore_glob, andpytest_ignore_collect. The hook at lines 20-28 already normalizes paths and covers all cases. Maintaining multiple filters increases the risk of drift.Apply this diff to simplify:
-# 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/*", -] -The
pytest_ignore_collecthook below provides the same coverage with better path normalization.tests/test_edit_strict_and_warnings.py (1)
73-76: Fragile tool filtering logic.The substring matching on line 75 has maintenance concerns:
'script'matches too broadly (any tool with "script" in the name)- Redundant conditions (
'create_script'already matches'script')- Brittle if tool names change
Consider using an explicit whitelist of exact tool names or a more precise pattern (e.g., prefix/suffix matching).
Example with explicit whitelist:
- for tool_info in get_registered_tools(): - name = tool_info['name'] - if any(k in name for k in ['script', 'apply_text', 'create_script', 'delete_script', 'validate_script', 'get_sha']): - mcp.tools[name] = tool_info['func'] + allowed_tools = { + 'apply_text_edits', 'create_script', 'delete_script', + 'validate_script', 'get_sha' + } + for tool_info in get_registered_tools(): + name = tool_info['name'] + if name in allowed_tools: + mcp.tools[name] = tool_info['func']tests/test_resources_api.py (2)
50-59: Consider verifying that required tools are registered.The keyword-based filtering is convenient but brittle: if tool names change or the registry doesn't contain expected tools, tests may fail silently with KeyError later.
Add verification after the filtering loop:
for tool_info in tools: tool_name = tool_info['name'] if any(keyword in tool_name for keyword in ['find_in_file', 'list_resources', 'read_resource']): mcp._tools[tool_name] = tool_info['func'] + + # Verify required tools are present + required_tools = ['list_resources'] + for required in required_tools: + if required not in mcp._tools: + raise RuntimeError(f"Required tool '{required}' not found in registry") + return mcp._tools
83-90: Consider using pytest-asyncio for cleaner async test handling.Manual event loop creation and cleanup is verbose and error-prone. The
pytest-asynciolibrary provides fixtures and markers that handle this automatically.Install pytest-asyncio and refactor to:
+import pytest + +@pytest.mark.asyncio +async def test_resource_list_filters_and_rejects_traversal(resource_tools, tmp_path, monkeypatch): # ... setup code ... list_resources = resource_tools["list_resources"] - import asyncio - loop = asyncio.new_event_loop() - try: - resp = loop.run_until_complete( - list_resources(ctx=DummyContext(), pattern="*.cs", under="Assets", - limit=50, project_root=str(proj)) - ) - finally: - loop.close() + resp = await list_resources( + ctx=DummyContext(), pattern="*.cs", under="Assets", + limit=50, project_root=str(proj) + )Apply the same pattern to
test_resource_list_rejects_outside_paths(lines 103-110).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitignore(1 hunks)MCPForUnity/UnityMcpServer~/src/pyproject.toml(1 hunks)MCPForUnity/UnityMcpServer~/src/telemetry.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/read_console.py(1 hunks)docs/README-DEV.md(1 hunks)tests/conftest.py(1 hunks)tests/test_edit_strict_and_warnings.py(4 hunks)tests/test_resources_api.py(4 hunks)tests/test_telemetry_queue_worker.py(4 hunks)tests/test_telemetry_subaction.py(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- docs/README-DEV.md
- tests/test_telemetry_subaction.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_telemetry_queue_worker.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T05:19:59.247Z
Learnt from: dsarno
PR: dsarno/unity-mcp#85
File: tests/test_telemetry_queue_worker.py:33-37
Timestamp: 2025-10-21T05:19:59.247Z
Learning: In the unity-mcp repository, pyproject.toml is located in MCPForUnity/UnityMcpServer~/src/, not at the repository root. When testing code that imports telemetry.py, the working directory should be changed to SRC (MCPForUnity/UnityMcpServer~/src/) so that telemetry.py's get_package_version() can find pyproject.toml via relative path.
Applied to files:
MCPForUnity/UnityMcpServer~/src/telemetry.py
🧬 Code graph analysis (3)
tests/test_resources_api.py (4)
MCPForUnity/UnityMcpServer~/src/telemetry.py (5)
MilestoneType(65-73)record_tool_usage(425-451)record_milestone(271-293)record_milestone(420-422)get_package_version(39-46)MCPForUnity/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)tests/test_helpers.py (1)
DummyContext(1-10)MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py (1)
list_resources(137-186)
tests/test_edit_strict_and_warnings.py (3)
tests/test_helpers.py (1)
DummyContext(1-10)tests/test_edit_normalization_and_noop.py (4)
DummyContext(50-59)fake_send(82-84)fake_send(125-126)fake_send(156-160)tests/test_resources_api.py (4)
_noop(13-14)MilestoneType(15-16)telemetry_tool(25-28)_wrap(26-27)
tests/conftest.py (1)
UnityMcpBridge/UnityMcpServer~/src/conftest.py (1)
pytest_ignore_collect(1-4)
🪛 Ruff (0.14.1)
tests/test_edit_strict_and_warnings.py
27-27: Unused function argument: dargs
(ARG001)
27-27: Unused function argument: dkwargs
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (10)
MCPForUnity/UnityMcpServer~/src/tools/read_console.py (1)
14-30: LGTM! Makingactionoptional improves API ergonomics.The change makes
actionoptional with a sensible default of'get', simplifying the most common use case. The implementation correctly handles the default on line 30, and the change is fully backward compatible.tests/conftest.py (1)
20-28: Path normalization looks good; previous feedback addressed.The implementation correctly normalizes path separators for cross-platform compatibility and removes the unused
configparameter. The substring and endswith checks reliably exclude the intended directories.Based on past review comments.
tests/test_edit_strict_and_warnings.py (6)
6-6: LGTM!The import of
DummyContextis correct and aligns with the test infrastructure changes to support proper context propagation.
13-32: LGTM! Telemetry stubbing is correctly implemented.The stub modules properly isolate tests from file I/O. The static analysis warning about unused parameters in
telemetry_tool(line 27) is a false positive—a noop decorator stub must accept any arguments.
88-89: LGTM! Patching correctly updated.The switch to patching
unity_connection.send_command_with_retryaligns with the PR objective to route commands through the unified connection module.
94-94: LGTM! Context propagation added.Passing
DummyContext()instead ofNoneproperly supports the context propagation pattern introduced in this refactoring.
113-114: LGTM! Consistent patching approach.The patching strategy is consistent with the first test and properly routes through
unity_connection.
118-118: LGTM! Context propagation consistent.Consistent use of
DummyContext()across both test functions.tests/test_resources_api.py (2)
11-30: LGTM! Stub telemetry modules prevent file I/O during imports.The stub implementations correctly mirror the actual telemetry module interfaces while providing no-op behavior suitable for testing. The use of
sys.modules.setdefaultis appropriately defensive.
44-44: Excellent! This addresses the previous review feedback.Importing
DummyContextfrom the sharedtest_helpersmodule eliminates the code duplication flagged in the previous review.
| # Prefer plain module to respect test-time overrides and sys.path injection | ||
| "config", | ||
| "src.config", |
There was a problem hiding this comment.
🧩 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.pyLength 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.pyLength of output: 1299
Address circular fallback in endpoint validation; verify module import resolution in production.
Two issues identified:
-
Endpoint validation bypass (lines 120-126): When
cfg_defaultis 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")
-
Module import resolution (lines 96-98): Plain module names enable test-time sys.path injection, but
UnityMcpBridge/UnityMcpServer~/src/config.pyexists 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.
| # 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) |
There was a problem hiding this comment.
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.
| # locate server src dynamically to avoid hardcoded layout assumptions | ||
| ROOT = Path(__file__).resolve().parents[1] | ||
| candidates = [ | ||
| ROOT / "MCPForUnity" / "UnityMcpServer~" / "src", | ||
| ROOT / "UnityMcpServer~" / "src", | ||
| ] | ||
| SRC = next((p for p in candidates if p.exists()), None) | ||
| if SRC is None: | ||
| searched = "\n".join(str(p) for p in candidates) | ||
| pytest.skip( | ||
| "MCP for Unity server source not found. Tried:\n" + searched, | ||
| allow_module_level=True, | ||
| ) | ||
| SRC = ROOT / "MCPForUnity" / "UnityMcpServer~" / "src" |
There was a problem hiding this comment.
Update the comment to match the implementation.
The comment on line 6 states "locate server src dynamically" but line 8 uses a hardcoded path construction. This is inconsistent.
Apply this diff to fix the comment:
-# locate server src dynamically to avoid hardcoded layout assumptions
+# Resolve server src path relative to test file
ROOT = Path(__file__).resolve().parents[1]
SRC = ROOT / "MCPForUnity" / "UnityMcpServer~" / "src"📝 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.
| # locate server src dynamically to avoid hardcoded layout assumptions | |
| ROOT = Path(__file__).resolve().parents[1] | |
| candidates = [ | |
| ROOT / "MCPForUnity" / "UnityMcpServer~" / "src", | |
| ROOT / "UnityMcpServer~" / "src", | |
| ] | |
| SRC = next((p for p in candidates if p.exists()), None) | |
| if SRC is None: | |
| searched = "\n".join(str(p) for p in candidates) | |
| pytest.skip( | |
| "MCP for Unity server source not found. Tried:\n" + searched, | |
| allow_module_level=True, | |
| ) | |
| SRC = ROOT / "MCPForUnity" / "UnityMcpServer~" / "src" | |
| # Resolve server src path relative to test file | |
| ROOT = Path(__file__).resolve().parents[1] | |
| SRC = ROOT / "MCPForUnity" / "UnityMcpServer~" / "src" |
🤖 Prompt for AI Agents
In tests/test_resources_api.py around lines 6 to 8, the comment saying "locate
server src dynamically to avoid hardcoded layout assumptions" is inaccurate
because the code constructs a specific hardcoded path; update the comment to
reflect that the path is being constructed explicitly (or indicate that it
targets the expected project layout), e.g., replace the dynamic wording with a
comment stating the test locates the server src using the expected project
layout and constructs the path accordingly.
- Add explanation for 200ms timeout in backpressure test - Replace manual event loop creation with asyncio.run() - Add assertion message with actual elapsed time for easier debugging
Replace 7 duplicate DummyContext class definitions with imports from tests.test_helpers. This follows DRY principles and ensures consistency across the test suite.
…s.py Dead code cleanup - function was no longer used after refactoring to dynamic tool registration.
Clarify why os.chdir() is necessary: telemetry.py calls get_package_version() at module load time, which reads pyproject.toml using a relative path. Acknowledges the fragility while explaining why it's currently required.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores