Harden MCP tool parameter handling to eliminate “invalid param” errors #339
Conversation
- Update pyproject.toml to use fastmcp>=2.12.5 instead of mcp[cli] - Replace all imports from mcp.server.fastmcp to fastmcp - Maintain MCP protocol compliance with mcp>=1.16.0 - All 15 files updated with new import statements - Server and tools registration working with FastMCP 2.0
…et search/read_resource/run_tests) for better LLM compatibility
…logs correct version
…itor, and manage_gameobject - read_console: accept int|str for count parameter with coercion - manage_editor: accept bool|str for wait_for_completion with coercion - manage_gameobject: accept bool|str for all boolean parameters with coercion - All tools now handle string parameters gracefully and convert to proper types internally
Adds fastmcp as explicit dependency for FastMCP 2.0 migration. Relaxes mcp version constraint to support broader compatibility. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Removes stub mcp modules from test files that were conflicting with the real mcp and fastmcp packages now installed as dependencies. Adds tests/__init__.py to make tests a proper Python package. This fixes test collection errors after migrating to FastMCP 2.0. Test results: 40 passed, 7 xpassed, 5 skipped, 1 failed (pre-existing) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Updates all remaining files to use `from fastmcp import` instead of the old `from mcp.server.fastmcp import` path. Changes: - server.py: Update FastMCP import - tools/__init__.py: Update FastMCP import - resources/__init__.py: Update FastMCP import - tools/manage_script.py, read_console.py, resource_tools.py: Update imports - test stubs: Update to stub `fastmcp` instead of `mcp.server.fastmcp` Addresses PR review feedback about incomplete migration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughUpdates dependencies and package versions, replaces imports from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Client/CLI
participant Tool as MCP Tool
participant Coerce as Coercion Helpers
participant RPC as send_command_with_retry
note over Coerce: New helpers: _coerce_bool, _coerce_int, vector parser
Caller->>Tool: invoke tool with raw params (strings/nums)
Tool->>Coerce: send raw params for normalization
Coerce->>Coerce: parse/convert strings → bool/int/list
Coerce-->>Tool: return normalized params
Tool->>RPC: send_command_with_retry(params with coerced types)
RPC-->>Tool: response
Tool-->>Caller: normalized response (success / structured error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
MCPForUnity/UnityMcpServer~/src/tools/read_console.py (1)
92-100: Add check for "stackTrace" (camelCase) to avoid silently failing to strip traces.The Python code checks only for
"stacktrace"(lowercase) at line 97–98, but the C# backend constructs responses with propertystackTrace(camelCase). When serialized via System.Text.Json (the standard .NET serializer), this becomes JSON key"stackTrace". The tests pass because mock fixtures use lowercase, masking the real issue. Wheninclude_stacktrace=False, traces from actual backend responses won't be removed.Update line 97 to check both variants:
if isinstance(line, dict) and ("stacktrace" in line or "stackTrace" in line): line.pop("stacktrace", None) line.pop("stackTrace", None)MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (2)
135-135: Add action-specific required-parameter checks.Avoids opaque failures from the Unity side when required fields are absent.
# Prepare parameters, removing None values + # Additional action-specific validations + if action == "get_component" and not component_name: + return {"success": False, "message": "'get_component' requires 'component_name'."} + if action == "set_component_property" and not component_properties: + return {"success": False, "message": "'set_component_property' requires 'component_properties'."} + if action == "add_component" and not (components_to_add or component_name): + return {"success": False, "message": "'add_component' requires 'components_to_add' or 'component_name'."} + if action == "remove_component" and not (components_to_remove or component_name): + return {"success": False, "message": "'remove_component' requires 'components_to_remove' or 'component_name'."}
165-176: Critical: prefab_folder defaults to None, creating malformed paths like 'None/.prefab'.The function parameter
prefab_folderhas a default value ofNone(line 43). When the code constructs the path at line 171 usingf"{prefab_folder}/{params['name']}.prefab", a None value gets converted to the string"None", resulting in invalid Unity paths. Additionally, theparamsdictionary filters out None values, but the code uses the raw function parameter instead, bypassing any validation.Add explicit validation before path construction:
- # Check if 'saveAsPrefab' is explicitly True in params - if action == "create" and params.get("saveAsPrefab"): + # Check if 'saveAsPrefab' is explicitly True in params + if action == "create" and params.get("saveAsPrefab") is True: if "prefabPath" not in params: - if "name" not in params or not params["name"]: - return {"success": False, "message": "Cannot create default prefab path: 'name' parameter is missing."} - # Use the provided prefab_folder (which has a default) and the name to construct the path - constructed_path = f"{prefab_folder}/{params['name']}.prefab" + if not params.get("name"): + return {"success": False, "message": "Cannot create prefab: 'name' is required when 'save_as_prefab' is true and 'prefab_path' is not provided."} + if not prefab_folder: + return {"success": False, "message": "Cannot create prefab: provide either 'prefab_path' or 'prefab_folder'."} + # Use the provided prefab_folder and the name to construct the path + constructed_path = f"{prefab_folder}/{params['name']}.prefab" # Ensure clean path separators (Unity prefers '/') params["prefabPath"] = constructed_path.replace("\\", "/")
🧹 Nitpick comments (15)
MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py (1)
369-381: Consider consolidating duplicate_coerce_boolhelpers.The same
_coerce_boollogic appears in multiple files (read_console.py, manage_editor.py, manage_gameobject.py per relevant snippets). Extracting this to a shared utility module would reduce duplication and ensure consistent coercion behavior across tools.Apply this pattern:
Create a new shared utility module (e.g.,
src/utils/type_coercion.py):def coerce_bool(value, default=None): """Coerce various inputs to bool, handling stringified booleans.""" if value is None: return default if isinstance(value, bool): return value if isinstance(value, str): v = value.strip().lower() if v in ("true", "1", "yes", "on"): return True if v in ("false", "0", "no", "off"): return False return bool(value)Then import and use across all tool modules.
MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py (1)
41-57: Consider consolidating duplicate_coerce_intlogic.The
_coerce_inthelper is duplicated across files—there's a module-level version in resource_tools.py (lines 20-42) and this inline version. Extracting coercion utilities to a shared module (e.g.,src/utils/type_coercion.py) would reduce duplication and centralize type-handling logic.tests/test_manage_gameobject_param_coercion.py (2)
7-9: Avoid global sys.path mutation in tests.Use monkeypatch.syspath_prepend within the test to keep isolation and avoid cross‑test interference.
Apply:
@@ -ROOT = pathlib.Path(__file__).resolve().parents[1] -SRC = ROOT / "MCPForUnity" / "UnityMcpServer~" / "src" -sys.path.insert(0, str(SRC)) +ROOT = pathlib.Path(__file__).resolve().parents[1] +SRC = ROOT / "MCPForUnity" / "UnityMcpServer~" / "src" @@ -def test_manage_gameobject_boolean_and_tag_mapping(monkeypatch): +def test_manage_gameobject_boolean_and_tag_mapping(monkeypatch): # Import with SRC as CWD to satisfy telemetry import side effects _prev = os.getcwd() os.chdir(str(SRC)) try: + # Prepend SRC to sys.path for this test only + monkeypatch.syspath_prepend(str(SRC)) manage_go_mod = _load_module(SRC / "tools" / "manage_gameobject.py", "manage_go_mod") finally: os.chdir(_prev)Also applies to: 35-41
46-49: Rename unusedcmdparameter to silence Ruff ARG001.The
cmdparameter infake_send()is defined but never used in the function body. Rename it to_cmdto follow Python conventions and indicate intentional non-use:- def fake_send(cmd, params): + def fake_send(_cmd, params): captured["params"] = params return {"success": True, "data": {}}MCPForUnity/UnityMcpServer~/src/tools/run_tests.py (2)
51-66: DRY: reuse the shared int coercer.Local _coerce_int duplicates logic used elsewhere. Import the central helper from resource_tools for consistency and maintenance.
-from pydantic import BaseModel, Field +from pydantic import BaseModel, Field @@ -from models import MCPResponse +from models import MCPResponse @@ -from registry import mcp_for_unity_tool +from registry import mcp_for_unity_tool from unity_connection import async_send_command_with_retry +from tools.resource_tools import _coerce_int @@ - # Coerce timeout defensively (string/float -> int) - def _coerce_int(value, default=None): - if value is None: - return default - try: - if isinstance(value, bool): - return default - if isinstance(value, int): - return int(value) - s = str(value).strip() - if s.lower() in ("", "none", "null"): - return default - return int(float(s)) - except Exception: - return default - params: dict[str, Any] = {"mode": mode} ts = _coerce_int(timeout_seconds) if ts is not None: params["timeoutSeconds"] = tsAlso applies to: 68-70
72-74: Normalize return type to RunTestsResponse.Ensure the function always returns a RunTestsResponse instance.
- return RunTestsResponse(**response) if isinstance(response, dict) else response + return RunTestsResponse(**response) if isinstance(response, dict) else RunTestsResponse(success=False, message=str(response))MCPForUnity/UnityMcpServer~/src/tools/read_console.py (1)
33-46: Deduplicate coercers: import shared helpers.Use the centralized _coerce_bool and _coerce_int from resource_tools for consistency.
-from unity_connection import send_command_with_retry +from unity_connection import send_command_with_retry +from tools.resource_tools import _coerce_bool, _coerce_int @@ - # Coerce booleans defensively (strings like 'true'/'false') - def _coerce_bool(value, default=None): - if value is None: - return default - if isinstance(value, bool): - return value - if isinstance(value, str): - v = value.strip().lower() - if v in ("true", "1", "yes", "on"): - return True - if v in ("false", "0", "no", "off"): - return False - return bool(value) - - include_stacktrace = _coerce_bool(include_stacktrace, True) + include_stacktrace = _coerce_bool(include_stacktrace, True) @@ - # Coerce count defensively (string/float -> int) - def _coerce_int(value, default=None): - if value is None: - return default - try: - if isinstance(value, bool): - return default - if isinstance(value, int): - return int(value) - s = str(value).strip() - if s.lower() in ("", "none", "null"): - return default - return int(float(s)) - except Exception: - return default - count = _coerce_int(count)Also applies to: 54-67
MCPForUnity/UnityMcpServer~/src/tools/manage_scene.py (1)
22-36: Reuse shared int coercer to avoid drift.Align with other tools by importing _coerce_int from resource_tools.
from registry import mcp_for_unity_tool from unity_connection import send_command_with_retry +from tools.resource_tools import _coerce_int @@ - # Coerce numeric inputs defensively - def _coerce_int(value, default=None): - if value is None: - return default - try: - if isinstance(value, bool): - return default - if isinstance(value, int): - return int(value) - s = str(value).strip() - if s.lower() in ("", "none", "null"): - return default - return int(float(s)) - except Exception: - return default + # Coerce numeric inputs defensivelyMCPForUnity/UnityMcpServer~/src/tools/manage_editor.py (1)
27-41: Deduplicate boolean coercion.Import and use the shared _coerce_bool from resource_tools for consistency.
from telemetry import is_telemetry_enabled, record_tool_usage from unity_connection import send_command_with_retry +from tools.resource_tools import _coerce_bool @@ - # Coerce boolean parameters defensively to tolerate 'true'/'false' strings - def _coerce_bool(value, default=None): - if value is None: - return default - if isinstance(value, bool): - return value - if isinstance(value, str): - v = value.strip().lower() - if v in ("true", "1", "yes", "on"): # common truthy strings - return True - if v in ("false", "0", "no", "off"): - return False - return bool(value) - wait_for_completion = _coerce_bool(wait_for_completion)MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (6)
9-10: Tighten the user-facing description to match actual accepted formats.Explicitly list accepted boolean synonyms and note that bare
"x,y,z"vectors are accepted.- description="Manage GameObjects. For booleans, send true/false; if your client only sends strings, 'true'/'false' are accepted. Vectors may be [x,y,z] or a string like '[x,y,z]'. For 'get_components', the `data` field contains a dictionary of component names and their serialized properties. For 'get_component', specify 'component_name' to retrieve only that component's serialized data." + description="Manage GameObjects. For booleans, send true/false; if your client only sends strings, 'true'/'false' (and '1'/'0'/'yes'/'no'/'on'/'off') are accepted. Vectors may be [x,y,z], 'x,y,z', or a string like '[x,y,z]'. For 'get_components', the `data` field contains a dictionary of component names and their serialized properties. For 'get_component', specify 'component_name' to retrieve only that component's serialized data."
61-63: Clarify thatcomponent_nameis also used byget_component.Prevents misuse and reduces round-trips.
- component_name: Annotated[str, - "Component name for 'add_component' and 'remove_component' actions"] | None = None, + component_name: Annotated[str, + "Component name for 'add_component', 'remove_component', and 'get_component' actions"] | None = None,
69-82: Avoid surprising truthiness on unknown strings; align bool coercion across tools.
return bool(value)turns any non-empty unrecognized string (e.g., "maybe") into True. Prefer strict parsing and fallback todefaultto avoid accidental True. Also, this helper duplicates similar ones in read_console/resource_tools/manage_editor — consolidate to a shared utility to keep behavior consistent.- def _coerce_bool(value, default=None): + def _coerce_bool(value, default=None): if value is None: return default if isinstance(value, bool): return value + if isinstance(value, (int, float)): + return bool(value) if isinstance(value, str): v = value.strip().lower() if v in ("true", "1", "yes", "on"): return True if v in ("false", "0", "no", "off"): return False - return bool(value) + # Unrecognized value; avoid truthy surprises + return defaultAlso consider moving this helper to a shared module and importing it here, read_console.py, resource_tools.py, and manage_editor.py for a single source of truth. Based on relevant code snippets.
104-106: UseNonefallback for unparsable vectors instead of echoing invalid input.Echoing the original string/list back to C# will likely fail harder. Omitting the param lets the server use defaults.
- position = _coerce_vec(position, default=position) - rotation = _coerce_vec(rotation, default=rotation) - scale = _coerce_vec(scale, default=scale) + position = _coerce_vec(position) + rotation = _coerce_vec(rotation) + scale = _coerce_vec(scale)
190-191: Intentional broad catch: add a Ruff suppression or narrow exceptions.If broad catching is desired to always return a structured MCP error, add a localized suppression.
- except Exception as e: - return {"success": False, "message": f"Python error managing GameObject: {str(e)}"} + except Exception as e: # noqa: BLE001 - return structured error to MCP clients + return {"success": False, "message": f"Python error managing GameObject: {str(e)}"}
69-103: Centralize coercers across tools modules to eliminate duplication.Verification confirms
_coerce_boolis duplicated identically across 4 files (read_console.py, resource_tools.py, manage_editor.py, manage_gameobject.py). The tools directory already has__init__.py, making acoercion.pymodule feasible and straightforward to import. Consider consolidating both_coerce_booland_coerce_vecto a shared utility module to reduce maintenance burden and prevent future drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
MCPForUnity/UnityMcpServer~/src/pyproject.toml(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/__init__.py(1 hunks)MCPForUnity/UnityMcpServer~/src/server.py(1 hunks)MCPForUnity/UnityMcpServer~/src/server_version.txt(1 hunks)MCPForUnity/UnityMcpServer~/src/tools/__init__.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tools/execute_menu_item.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_editor.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py(3 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_prefabs.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_scene.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_script.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_shader.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/read_console.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py(4 hunks)MCPForUnity/UnityMcpServer~/src/tools/run_tests.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/script_apply_edits.py(1 hunks)MCPForUnity/package.json(1 hunks)tests/__init__.py(1 hunks)tests/test_edit_normalization_and_noop.py(0 hunks)tests/test_get_sha.py(0 hunks)tests/test_improved_anchor_matching.py(0 hunks)tests/test_manage_asset_param_coercion.py(1 hunks)tests/test_manage_gameobject_param_coercion.py(1 hunks)tests/test_manage_script_uri.py(2 hunks)tests/test_read_console_truncate.py(0 hunks)tests/test_script_tools.py(0 hunks)tests/test_validate_script_summary.py(0 hunks)
💤 Files with no reviewable changes (6)
- tests/test_validate_script_summary.py
- tests/test_edit_normalization_and_noop.py
- tests/test_read_console_truncate.py
- tests/test_improved_anchor_matching.py
- tests/test_script_tools.py
- tests/test_get_sha.py
🧰 Additional context used
🧬 Code graph analysis (7)
MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py (3)
MCPForUnity/UnityMcpServer~/src/tools/read_console.py (1)
_coerce_bool(34-45)MCPForUnity/UnityMcpServer~/src/tools/manage_editor.py (1)
_coerce_bool(28-39)MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (1)
_coerce_bool(70-81)
tests/test_manage_asset_param_coercion.py (2)
tests/test_helpers.py (1)
DummyContext(1-10)MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py (1)
manage_asset(15-83)
MCPForUnity/UnityMcpServer~/src/tools/manage_editor.py (3)
MCPForUnity/UnityMcpServer~/src/tools/read_console.py (1)
_coerce_bool(34-45)MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py (1)
_coerce_bool(369-380)MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (1)
_coerce_bool(70-81)
MCPForUnity/UnityMcpServer~/src/tools/read_console.py (3)
MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py (1)
_coerce_bool(369-380)MCPForUnity/UnityMcpServer~/src/tools/manage_editor.py (1)
_coerce_bool(28-39)MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (1)
_coerce_bool(70-81)
tests/test_manage_gameobject_param_coercion.py (2)
tests/test_helpers.py (1)
DummyContext(1-10)MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (1)
manage_gameobject(11-191)
MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (3)
MCPForUnity/UnityMcpServer~/src/tools/read_console.py (1)
_coerce_bool(34-45)MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py (1)
_coerce_bool(369-380)MCPForUnity/UnityMcpServer~/src/tools/manage_editor.py (1)
_coerce_bool(28-39)
MCPForUnity/UnityMcpServer~/src/tools/run_tests.py (4)
MCPForUnity/UnityMcpServer~/src/tools/read_console.py (1)
_coerce_int(54-67)MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py (1)
_coerce_int(20-42)MCPForUnity/UnityMcpServer~/src/tools/manage_scene.py (1)
_coerce_int(22-35)MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py (1)
_coerce_int(41-54)
🪛 Ruff (0.14.1)
tests/test_manage_asset_param_coercion.py
47-47: Unused function argument: cmd
(ARG001)
47-47: Unused function argument: loop
(ARG001)
tests/test_manage_gameobject_param_coercion.py
46-46: Unused function argument: cmd
(ARG001)
MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py
89-89: Do not catch blind exception: Exception
(BLE001)
100-100: Do not catch blind exception: Exception
(BLE001)
MCPForUnity/UnityMcpServer~/src/tools/run_tests.py
64-64: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (23)
MCPForUnity/UnityMcpServer~/src/resources/__init__.py (1)
7-7: LGTM! Import path updated consistently.The migration from
mcp.server.fastmcptofastmcpaligns with the dependency changes inpyproject.toml.tests/__init__.py (1)
1-1: LGTM! Test package initialization.This file properly designates the tests directory as a package, enabling cleaner test module imports and supporting the removal of complex in-test stubs.
MCPForUnity/package.json (1)
3-3: LGTM! Version bump to force client schema refresh.The version update to 6.2.2 is consistent across the project and will trigger client-side schema updates for the parameter type widening changes.
MCPForUnity/UnityMcpServer~/src/server_version.txt (1)
1-1: LGTM! Version synchronized.Server version correctly updated to 6.2.2, matching the package.json version.
MCPForUnity/UnityMcpServer~/src/pyproject.toml (2)
3-3: LGTM! Version bump coordinated.Project version correctly updated to 6.2.2, consistent with other configuration files.
9-10: Dependency versions verified and compatible.All original review concerns are validated:
- fastmcp 2.12.5 is published and available on PyPI
- mcp 1.16.0 is published and available on PyPI
- No documented breaking changes exist between mcp 1.16.0 and 1.17.0; the 1.17.0 release contains only minor features and fixes
- fastmcp 2.12.5 is fully MCP-compliant and interoperable with MCP clients and SDKs
- No security vulnerabilities present for these versions
The constraint
mcp>=1.16.0is compatible withfastmcp>=2.12.5.tests/test_manage_script_uri.py (1)
24-34: LGTM! Test infrastructure simplified.The removal of nested module stubs (
mcp.server.fastmcp) in favor of a singlefastmcpstub is a clean improvement that aligns with the import path migration. This simplification makes the test setup more maintainable.MCPForUnity/UnityMcpServer~/src/tools/script_apply_edits.py (1)
6-6: LGTM! Import path migrated.The Context import correctly updated to use
fastmcp, consistent with the dependency changes and import migration across the codebase.MCPForUnity/UnityMcpServer~/src/tools/manage_script.py (1)
6-6: LGTM! Import path migrated successfully.Both
FastMCPandContextare now correctly imported fromfastmcp, completing the import path migration for this module.MCPForUnity/UnityMcpServer~/src/tools/execute_menu_item.py (1)
6-6: LGTM: Import path modernized.The migration from
mcp.server.fastmcptofastmcpaligns with the broader dependency update across the codebase.MCPForUnity/UnityMcpServer~/src/tools/manage_prefabs.py (1)
3-3: LGTM: Import path updated.Consistent with the project-wide migration to the
fastmcppackage.MCPForUnity/UnityMcpServer~/src/tools/__init__.py (1)
7-7: LGTM: Import aligned with new package structure.The switch to
fastmcpis consistent across all tool modules.MCPForUnity/UnityMcpServer~/src/server.py (1)
2-2: LGTM: Core server updated to use fastmcp.The import change is straightforward and matches the dependency migration.
tests/test_manage_asset_param_coercion.py (1)
36-66: LGTM: Test correctly validates string-to-int coercion.The test successfully verifies that
page_size="50"andpage_number="2"are coerced to integers 50 and 2 before being passed to the Unity bridge.Note: The static analysis warnings about unused parameters
cmdandloopin the mock function are false positives—these parameters are required for signature compatibility with the realasync_send_command_with_retry.MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py (3)
14-14: LGTM: Import updated.Consistent with the project-wide migration to
fastmcp.
193-200: LGTM: Type signatures broadened for client flexibility.Accepting
int | float | strfor numeric parameters allows clients that stringify values to pass validation, with server-side coercion (lines 312-315) handling the conversion.
354-354: LGTM: Boolean parameter widened to accept strings.This allows clients to pass
"true"/"false"strings, which are coerced to booleans at line 381.MCPForUnity/UnityMcpServer~/src/tools/manage_shader.py (1)
4-4: LGTM: Import path updated.Consistent with the dependency migration to
fastmcp.MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py (2)
7-7: LGTM: Import modernized.Aligns with the project-wide switch to
fastmcp.
32-33: LGTM: Pagination parameters widened for client compatibility.The type signatures now accept
int | float | str, enabling clients that stringify numeric values to pass schema validation. Server-side coercion (lines 56-57) ensures the values are normalized to integers before transmission to Unity.MCPForUnity/UnityMcpServer~/src/tools/manage_editor.py (1)
16-17: LGTM on schema widening for booleans.Accepting bool or string for wait_for_completion matches the PR goal and keeps native types working.
MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (2)
3-3: Import path migration looks correct.Switch to
fastmcp.Contextmatches the PR’s dependency shift.
116-134: Good guardrails for 'find' vs 'name' usage.Clear, actionable messages prevent common misuse.
| timeout_seconds: Annotated[str, Field( | ||
| description="Optional timeout in seconds for the Unity test run (string, e.g. '30')")] | None = None, | ||
| ) -> RunTestsResponse: |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Broaden timeout_seconds schema to accept both int and str.
Current schema (str only) can reject native ints at pre‑validation, contradicting “native types remain supported.”
- timeout_seconds: Annotated[str, Field(
- description="Optional timeout in seconds for the Unity test run (string, e.g. '30')")] | None = None,
+ timeout_seconds: Annotated[int | str, Field(
+ description="Optional timeout in seconds for the Unity test run (accepts int or string, e.g., 30 or '30')")] | None = None,📝 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.
| timeout_seconds: Annotated[str, Field( | |
| description="Optional timeout in seconds for the Unity test run (string, e.g. '30')")] | None = None, | |
| ) -> RunTestsResponse: | |
| timeout_seconds: Annotated[int | str, Field( | |
| description="Optional timeout in seconds for the Unity test run (accepts int or string, e.g., 30 or '30')")] | None = None, | |
| ) -> RunTestsResponse: |
🤖 Prompt for AI Agents
In MCPForUnity/UnityMcpServer~/src/tools/run_tests.py around lines 46 to 48, the
timeout_seconds parameter is annotated as Annotated[str, Field(...)] | None
which rejects native ints before validation; change the annotation to accept
both int and str (e.g., Annotated[int | str, Field(...)] | None or
Annotated[Union[int, str], Field(...)] | None), update the Field description to
reflect both types, and ensure necessary typing imports are present so native
integers are accepted and validated.
Parameter Type Improvements: - Broaden count in read_console.py to accept int | str - Broaden build_index in manage_scene.py to accept int | str - Harden vector parsing in manage_gameobject.py with NaN/Inf checks - Add whitespace-delimited vector support (e.g., "1 2 3") - Narrow exception handling from Exception to (ValueError, TypeError) Test Improvements: - Harden _load_module in test files with spec/loader validation - Fix test_manage_gameobject_boolean_and_tag_mapping by mapping tag→search_term Bug Fixes: - Fix syntax error in manage_shader.py (remove stray 'x') Version: Bump to 6.2.3 All tests pass: 41 passed, 5 skipped, 7 xpassed
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
MCPForUnity/UnityMcpServer~/src/tools/manage_scene.py (1)
16-17: Past review concern addressed: build_index now accepts both int and str.The parameter type has been widened to
int | stras suggested in the previous review, eliminating "invalid param" errors for clients that pass native integers.MCPForUnity/UnityMcpServer~/src/tools/read_console.py (1)
19-19: Past review concern addressed: count now accepts both int and str.The parameter type has been widened to
int | stras suggested in the previous review, allowing clients to pass either native integers or quoted strings.MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (1)
83-104: Past review concern fully addressed: vector parsing is now hardened with finite checks and flexible delimiter support.The
_coerce_vechelper now:
- Uses narrow exception handling (ValueError, TypeError instead of blind Exception)
- Validates components with
math.isfiniteto reject NaN/Inf- Supports both comma-delimited (
"x,y,z") and whitespace-delimited ("x y z") string formatsThis implementation matches the recommendations from the previous review.
🧹 Nitpick comments (5)
tests/test_telemetry_queue_worker.py (1)
39-45: Consider extracting the _load_module helper to test_helpers to eliminate duplication.This identical
_load_modulehelper with the ImportError guard appears in at least five test files (test_telemetry_queue_worker.py, test_get_sha.py, test_validate_script_summary.py, test_manage_asset_param_coercion.py, test_read_console_truncate.py). Moving it totests/test_helpers.pywould reduce maintenance burden and ensure consistent error handling across the test suite.MCPForUnity/UnityMcpServer~/src/tools/manage_scene.py (1)
21-35: Consider extracting _coerce_int to a shared utility module to eliminate duplication.This
_coerce_inthelper appears duplicated across multiple tool modules (at minimum: manage_scene.py, read_console.py, and likely manage_asset.py based on the relevant snippets). Extracting it to a shared utility module (e.g.,src/utils/param_coercion.py) would reduce duplication and ensure consistent coercion behavior across all tools.MCPForUnity/UnityMcpServer~/src/tools/read_console.py (1)
34-45: Consider extracting _coerce_bool to a shared utility module to eliminate duplication.This
_coerce_boolhelper is duplicated across multiple tool modules (at minimum: read_console.py, manage_gameobject.py, manage_editor.py, and resource_tools.py based on relevant snippets). Extracting it to a shared utility module (e.g.,src/utils/param_coercion.py) alongside_coerce_intwould centralize coercion logic and ensure consistency.tests/test_manage_gameobject_param_coercion.py (2)
48-50: Remove unused parameter for clarity.The
cmdparameter is captured but never used in the function body.Apply this diff:
- def fake_send(cmd, params): + def fake_send(_cmd, params): captured["params"] = params return {"success": True, "data": {}}
66-69: Tighten assertions to verify actual coercion.The coercion functions in manage_gameobject.py convert
find_all="true"toTrueandsearch_inactive="0"toFalsebefore callingsend_command_with_retry. The current flexible assertions (orconditions) allow both stringified and native forms, but the captured params should contain only the coerced boolean values. Stricter assertions would better validate that coercion is working correctly and catch potential regressions.Apply this diff:
- # ensure tag mapped to searchTerm and booleans passed through; C# side coerces true/false already + # ensure tag mapped to searchTerm and booleans are coerced to native Python bool assert captured["params"]["searchTerm"] == "Player" - assert captured["params"]["findAll"] == "true" or captured["params"]["findAll"] is True - assert captured["params"]["searchInactive"] in ("0", False, 0) + assert captured["params"]["findAll"] is True + assert captured["params"]["searchInactive"] is False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
MCPForUnity/UnityMcpServer~/src/server_version.txt(1 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py(3 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_scene.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_shader.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tools/read_console.py(1 hunks)MCPForUnity/package.json(1 hunks)tests/test_get_sha.py(1 hunks)tests/test_manage_asset_param_coercion.py(1 hunks)tests/test_manage_gameobject_param_coercion.py(1 hunks)tests/test_read_console_truncate.py(1 hunks)tests/test_telemetry_queue_worker.py(1 hunks)tests/test_validate_script_summary.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- MCPForUnity/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- MCPForUnity/UnityMcpServer~/src/server_version.txt
- MCPForUnity/UnityMcpServer~/src/tools/manage_shader.py
🧰 Additional context used
🧬 Code graph analysis (4)
MCPForUnity/UnityMcpServer~/src/tools/read_console.py (3)
MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (1)
_coerce_bool(70-81)MCPForUnity/UnityMcpServer~/src/tools/manage_editor.py (1)
_coerce_bool(28-39)MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py (1)
_coerce_bool(369-380)
MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (4)
tests/test_helpers.py (1)
info(3-4)MCPForUnity/UnityMcpServer~/src/tools/read_console.py (1)
_coerce_bool(34-45)MCPForUnity/UnityMcpServer~/src/tools/manage_editor.py (1)
_coerce_bool(28-39)MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py (1)
_coerce_bool(369-380)
tests/test_manage_asset_param_coercion.py (2)
tests/test_helpers.py (1)
DummyContext(1-10)MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py (1)
manage_asset(15-83)
tests/test_manage_gameobject_param_coercion.py (2)
tests/test_helpers.py (1)
DummyContext(1-10)MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (1)
manage_gameobject(11-197)
🪛 Ruff (0.14.1)
tests/test_validate_script_summary.py
13-13: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_telemetry_queue_worker.py
42-42: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_get_sha.py
14-14: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_manage_asset_param_coercion.py
16-16: Avoid specifying long messages outside the exception class
(TRY003)
49-49: Unused function argument: cmd
(ARG001)
49-49: Unused function argument: loop
(ARG001)
tests/test_manage_gameobject_param_coercion.py
15-15: Avoid specifying long messages outside the exception class
(TRY003)
48-48: Unused function argument: cmd
(ARG001)
tests/test_read_console_truncate.py
13-13: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (7)
tests/test_manage_asset_param_coercion.py (1)
38-68: LGTM! Test correctly validates string-to-int coercion for pagination parameters.The test properly exercises the coercion logic by passing string values for
page_sizeandpage_number, then verifying they're converted to integers in the captured parameters.tests/test_read_console_truncate.py (1)
52-100: LGTM! Tests correctly validate the include_stacktrace parameter and stacktrace field filtering.Both tests properly exercise the
include_stacktraceparameter: the first verifies default behavior (stacktraces included), and the second confirms stacktraces are stripped wheninclude_stacktrace=False.MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (1)
117-119: LGTM! Backward compatibility mapping ensures tag parameter works with by_tag search method.The logic correctly maps
tagtosearch_termwhensearch_methodis"by_tag"andsearch_termis not explicitly provided, maintaining backward compatibility without requiring breaking changes to existing client code.tests/test_manage_gameobject_param_coercion.py (4)
1-10: LGTM: Path setup is correct.The imports and path configuration properly prepare the test environment to load the source modules.
12-18: LGTM: ImportError guard in place.The function now safely handles missing spec or loader scenarios. Previous concern resolved.
21-31: LGTM: FastMCP stubbing is appropriate.The stub setup correctly isolates the test from real MCP dependencies.
54-62: Tag→searchTerm mapping now works correctly.The test correctly validates that
tag="Player"maps tosearchTermwhensearch_method="by_tag". The mapping logic implemented in manage_gameobject.py (lines 121-122 of the relevant snippets) resolves the previous concern.
1 participant
Summary
This branch widens python tool schemas and adds server-side coercion so clients that stringify numbers/booleans/vectors no longer fail pre-validation. Behavior is backwards compatible; native types still work.
Dependencies
Added FastMCP as an explicit dependency and standardized imports to fastmcp; aligned mcp sdk package version to FastMCP’s constraints.
Key changes
resources/read(start_line, line_count, head_bytes, tail_lines),manage_asset.search(page_size, page_number),run_tests(timeout_seconds),manage_scene(build_index).read_console(include_stacktrace),resources/find_in_file(ignore_case),manage_editor(wait_for_completion),manage_gameobject(find_all, search_inactive, search_in_children, save_as_prefab, set_active, includeNonPublicSerialized).manage_gameobject(position, rotation, scale).manage_shadersignature (stray character afterctx: Context).Compatibility
Fixes #300
Summary by CodeRabbit
Chores
New Features
Tests