fix: accept JSON strings for list parameters in manage_gameobject and manage_texture #731
Conversation
… manage_texture Fixes the issue where Pydantic validation was rejecting JSON strings for list-type parameters. The following parameters now accept both JSON strings and native Python lists: - manage_gameobject.components_to_add - manage_gameobject.components_to_remove - manage_texture.palette Changes: - Update parameter type annotations from list[str] to list[str] | str - Add normalize_string_list() utility function for robust parsing - Improve _normalize_palette() with better JSON string handling - Remove unused imports (Union, List) The normalize functions handle various input formats: - None -> None - list/tuple of values -> validated list - JSON string -> parsed and validated - Invalid values -> descriptive error message Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Reviewer's GuideAllows JSON string inputs for list-type parameters in manage_gameobject and manage_texture by broadening type annotations and centralizing normalization/parsing logic, with improved validation and error reporting for string-list and palette parameters. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughAdds runtime normalization for string-or-list inputs: introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
_normalize_palette, the earlyisinstance(value, list)return now bypasses the subsequent per-color validation/normalization (including float-to-int handling), so list inputs will no longer be normalized as before; consider removing the early return or routing list inputs through_normalize_color_intto preserve the original behavior. _normalize_palettenow treats string inputs differently from list inputs (strings are fully normalized via_normalize_color_int, while lists are returned as-is), which can lead to inconsistent palette formats depending on how the same logical value is passed; it may be safer to ensure both code paths go through the same normalization logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_normalize_palette`, the early `isinstance(value, list)` return now bypasses the subsequent per-color validation/normalization (including float-to-int handling), so list inputs will no longer be normalized as before; consider removing the early return or routing list inputs through `_normalize_color_int` to preserve the original behavior.
- `_normalize_palette` now treats string inputs differently from list inputs (strings are fully normalized via `_normalize_color_int`, while lists are returned as-is), which can lead to inconsistent palette formats depending on how the same logical value is passed; it may be safer to ensure both code paths go through the same normalization logic.
## Individual Comments
### Comment 1
<location> `Server/src/services/tools/manage_texture.py:52-54` </location>
<code_context>
return None, None
- # Try parsing as string first
+ # Already a list - validate structure and return
+ if isinstance(value, list):
+ return value, None
+
+ # Try parsing as string (immediate parsing for string input)
</code_context>
<issue_to_address>
**issue (bug_risk):** List palettes now bypass `_normalize_color_int`, so unvalidated/unnormalized palettes can slip through.
Previously, list inputs flowed into the `normalized = []` block where each entry was passed through `_normalize_color_int`, enforcing structure and range. With the new `if isinstance(value, list): return value, None` early return, list palettes bypass that normalization and validation.
This can create inconsistent behavior between list palettes and JSON string palettes, and allow invalid color entries.
I’d suggest either removing this early return so lists go through the existing normalization, or refactoring so `_normalize_color_int` is applied in a shared path for both direct lists and parsed lists.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 174c634269
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Already a list - validate structure and return | ||
| if isinstance(value, list): | ||
| return value, None |
There was a problem hiding this comment.
This new early return bypasses the existing _normalize_color_int loop for any palette already passed as a Python list, so the most common non-string input path is no longer validated or converted to 0-255 ints. As a result, inputs like [[1.0, 0.0, 0.0, 1.0]] now flow through unchanged and are sent to Unity, where TextureOps.ParseColor32 converts components with ToObject<int>() (MCPForUnity/Editor/Helpers/TextureOps.cs), truncating normalized float colors and producing incorrect output instead of the prior conversion behavior.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Server/src/services/tools/manage_texture.py (1)
77-87:⚠️ Potential issue | 🟠 MajorDead code: this block is unreachable.
After the early returns for
None(line 49),list(line 52), andstr(line 57), execution can only reach here for non-list/non-str/non-None types. Theif not isinstance(value, list)at line 77 will always beTrue, so lines 80–87 (theforloop overvalue) are never executed.Once the list-handling fix above is applied, this entire block (lines 77–87) should be removed.
🤖 Fix all issues with AI agents
In `@Server/src/services/tools/manage_texture.py`:
- Around line 52-54: When `value` is a native list it currently returns
immediately and skips per-color normalization; change the branch that handles
isinstance(value, list) to mirror the JSON path: iterate over each element in
the list, call _normalize_color_int on each color entry (handling both 3- and
4-component colors), collect the normalized colors, validate shapes/types and
return the normalized list with None for error; then remove the now-dead legacy
validation block (the old list-validation logic around lines 77–87) since it's
redundant.
🧹 Nitpick comments (1)
Server/src/services/tools/manage_texture.py (1)
13-13:normalize_string_listis imported but unused in this file.The palette normalization uses
_normalize_palette, notnormalize_string_list. Consider removing the unused import.
There was a problem hiding this comment.
Pull request overview
This PR aims to make the Python MCP server tools more tolerant of MCP/LLM clients that send list-typed parameters as JSON strings, specifically for manage_gameobject component lists and manage_texture palettes.
Changes:
- Added
normalize_string_list()utility to accept/parse list-like inputs provided as JSON strings (or plain strings). - Updated
manage_gameobjectto acceptcomponents_to_add/components_to_removeaslist[str] | strand normalize them before sending to Unity. - Updated
manage_textureto acceptpaletteaslist[list[int|float]] | strand enhanced_normalize_palette()JSON-string handling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
Server/src/services/tools/utils.py |
Adds normalize_string_list() helper for list-of-strings parameters that may arrive as JSON strings. |
Server/src/services/tools/manage_gameobject.py |
Expands component list parameter types and normalizes them via normalize_string_list(). |
Server/src/services/tools/manage_texture.py |
Expands palette type to accept JSON strings and adjusts palette normalization logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Already a list - validate structure and return | ||
| if isinstance(value, list): | ||
| return value, None | ||
|
|
There was a problem hiding this comment.
_normalize_palette() now returns early for any list input without validating/normalizing entries to 0–255 int RGBA. This bypasses the normalization loop below and can send floats/invalid shapes through to Unity, regressing the documented behavior. Remove the early return and run the same per-color normalization/validation for list inputs as for parsed JSON strings.
| # Already a list - validate structure and return | |
| if isinstance(value, list): | |
| return value, None |
| from services.registry import mcp_for_unity_tool | ||
| from services.tools import get_unity_instance_from_context | ||
| from services.tools.utils import parse_json_payload, coerce_bool, coerce_int, normalize_color | ||
| from services.tools.utils import parse_json_payload, coerce_bool, coerce_int, normalize_color, normalize_string_list |
There was a problem hiding this comment.
normalize_string_list is imported but never used in this module. Please remove the unused import to avoid dead code and keep imports accurate.
| from services.tools.utils import parse_json_payload, coerce_bool, coerce_int, normalize_color, normalize_string_list | |
| from services.tools.utils import parse_json_payload, coerce_bool, coerce_int, normalize_color |
| palette: Annotated[list[list[int | float]] | str, | ||
| "Color palette as [[r,g,b,a], ...]. Accepts both 0-255 range or 0.0-1.0 normalized range"] | None = None, |
There was a problem hiding this comment.
The palette parameter now accepts JSON strings (and _normalize_palette behavior was adjusted), but the existing integration test suite for manage_texture doesn’t cover palette inputs. Add tests for palette as a JSON string and as a native list (including float 0.0–1.0 values) to ensure it normalizes to int RGBA and returns helpful errors for invalid shapes.
| # --- Normalize components_to_add and components_to_remove --- | ||
| components_to_add, add_error = normalize_string_list(components_to_add, "components_to_add") | ||
| if add_error: | ||
| return {"success": False, "message": add_error} | ||
|
|
||
| components_to_remove, remove_error = normalize_string_list(components_to_remove, "components_to_remove") | ||
| if remove_error: | ||
| return {"success": False, "message": remove_error} |
There was a problem hiding this comment.
components_to_add/components_to_remove now accept JSON strings and are normalized at runtime, but there are existing integration tests for manage_gameobject JSON parsing that don’t cover these parameters. Add tests that pass these fields as JSON strings (e.g. '["Rigidbody"]') and as plain strings, and assert the sent params contain a proper list of strings or a clear validation error.
Server/src/services/tools/utils.py
Outdated
| Normalize a string list parameter that might be a JSON string. | ||
|
|
||
| Handles various input formats from MCP clients/LLMs: | ||
| - None -> (None, None) | ||
| - list/tuple of strings -> (list, None) | ||
| - JSON string '["a", "b", "c"]' -> parsed and normalized |
There was a problem hiding this comment.
normalize_string_list() also treats a non-JSON plain string as a single-element list (see the parsed == value branch), but the docstring/input-format list doesn’t mention that behavior. Update the docstring to reflect the accepted plain-string format so callers know what to expect.
| Normalize a string list parameter that might be a JSON string. | |
| Handles various input formats from MCP clients/LLMs: | |
| - None -> (None, None) | |
| - list/tuple of strings -> (list, None) | |
| - JSON string '["a", "b", "c"]' -> parsed and normalized | |
| Normalize a string list parameter that might be a JSON string or plain string. | |
| Handles various input formats from MCP clients/LLMs: | |
| - None -> (None, None) | |
| - list/tuple of strings -> (list, None) | |
| - JSON string '["a", "b", "c"]' -> parsed and normalized | |
| - Plain non-JSON string "foo" -> treated as ["foo"] |
…anage_texture and utils
… input Fixes bug where malformed JSON arrays like '["BoxCollider",]' were silently treated as single-element list strings instead of returning validation errors. - Add check for JSON array syntax before parsing - Return error when input looks like JSON but fails to parse - Plain non-JSON strings still treated as single-element lists Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Thanks @whatevertogo, merged to beta! Solid fix for the JSON string edge cases. |
2 participants
Description
Fixes issue where Pydantic validation was rejecting JSON string inputs for list-type parameters in
manage_gameobjectandmanage_texturetools. Previously, when AI assistants or clients passed JSON strings like["Rigidbody", "BoxCollider"], Pydantic would fail validation before the parameter reached the tool function.Type of Change
Changes Made
Python Server Changes
Parameter Type Annotations
manage_gameobject.components_to_add:list[str]→list[str] | strmanage_gameobject.components_to_remove:list[str]→list[str] | strmanage_texture.palette:list[list[int | float]]→list[list[int | float]] | strNew Utility Function
normalize_string_list()inutils.pyto robustly handle list parametersImproved Error Handling
_normalize_palette()with better JSON string handling@field_validatordecorators (they don't work in non-Pydantic classes)C# Changes
Testing/Screenshots/Recordings
Comprehensive test suite created and executed with 100% pass rate:
Pydantic Type Validation
strtypelisttypenormalize_string_list() Function
["Rigidbody", "BoxCollider"]→['Rigidbody', 'BoxCollider']['SingleComponent']None_normalize_palette() Function
NoneTest script results: All 15 test cases passed
Documentation Updates
tools/UPDATE_DOCS_PROMPT.mdtools/UPDATE_DOCS.mdNote: The parameter descriptions in the type annotations already mention JSON string support, so no doc updates required for this fix.
Related Issues
Resolves issue where JSON string parameters were rejected by Pydantic validation.
Additional Notes
This change maintains full backward compatibility:
The fix uses the same pattern as existing parameters like
position,rotation, andscalewhich already accept JSON strings vialist[float] | dict[str, float] | strtype annotations.Summary by Sourcery
Allow manage_gameobject and manage_texture tools to accept JSON string inputs for list-style parameters and normalize them consistently.
Bug Fixes:
Enhancements:
Summary by CodeRabbit