Fix: Fix vector and color parameter validation to accept JSON string inputs#625
Merged
Scriptwonder merged 3 commits intoJan 27, 2026
Merged
Conversation
The Pydantic validation in FastMCP occurs before function execution, causing validation errors when clients pass string values like '[2, 2, 2]' for parameters typed as `list[float] | str`. Since the code already has normalization functions (_normalize_vector, _normalize_color) that handle string inputs, change the type annotations to `Any` to bypass Pydantic's strict validation. Affected parameters: - manage_gameobject: position, rotation, scale, offset - manage_material: color Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Change parameter types from `list[float]` to `list[float] | str` to accept both list and JSON string inputs (consistent with read_console/run_tests) - Modify _normalize_vector to return (value, error_message) tuple instead of silently returning None on invalid input - Add detailed error messages for invalid vector values This fixes the Pydantic validation error when clients pass string values like "[2, 2, 2]" for scale/position/rotation parameters. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
Reviewer's GuideAllows vector-like and color parameters to be passed as either lists or JSON strings, and centralizes detailed validation/error messaging in the vector normalization helper so manage_gameobject can return clear, user-facing errors instead of raw Pydantic validation failures. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughEnhanced error reporting and input flexibility for vector and color parameters across tools. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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
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 |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
_normalize_vectorerror messages say the value 'must be a list or JSON string' even though tuples and comma-separated strings are also accepted; consider aligning the messages with the actual accepted formats to avoid confusing users. - The repeated pattern for normalizing
position,rotation,scale, andoffset(call_normalize_vector, then immediately return on*_error) could be refactored into a small helper or loop to reduce duplication and make it easier to extend with new vector parameters.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_normalize_vector` error messages say the value 'must be a list or JSON string' even though tuples and comma-separated strings are also accepted; consider aligning the messages with the actual accepted formats to avoid confusing users.
- The repeated pattern for normalizing `position`, `rotation`, `scale`, and `offset` (call `_normalize_vector`, then immediately return on `*_error`) could be refactored into a small helper or loop to reduce duplication and make it easier to extend with new vector parameters.
## Individual Comments
### Comment 1
<location> `Server/src/services/tools/manage_gameobject.py:16-25` </location>
<code_context>
-def _normalize_vector(value: Any, default: Any = None) -> list[float] | None:
+def _normalize_vector(value: Any, param_name: str = "vector") -> tuple[list[float] | None, str | None]:
"""
Robustly normalize a vector parameter to [x, y, z] format.
</code_context>
<issue_to_address>
**suggestion:** Distinguish list/tuple inputs with incorrect length from entirely invalid types in error messages.
For non-string inputs, any list/tuple with length ≠ 3 falls through to the generic `return None, f"{param_name} must be a list or JSON string, got {type(value).__name__}"`, yielding misleading errors like `position must be a list or JSON string, got list` when the issue is the element count.
Consider handling this explicitly:
```python
if isinstance(value, (list, tuple)):
if len(value) != 3:
return None, f"{param_name} must have exactly 3 elements [x, y, z], got {len(value)}: {value}"
try:
vec = [float(value[0]), float(value[1]), float(value[2])]
...
```
so callers get a precise, actionable message when the length is wrong.
Suggested implementation:
```python
# If already a list/tuple, ensure it has exactly 3 elements and convert to floats
if isinstance(value, (list, tuple)):
if len(value) != 3:
return None, f"{param_name} must have exactly 3 elements [x, y, z], got {len(value)}: {value}"
```
This change assumes that the existing body of the `if` block continues to convert the three elements to floats and handle any conversion errors as before. No further changes are required unless other parts of the function explicitly depend on the earlier `and len(value) == 3` condition, in which case they should be updated to rely on the new early return for incorrect lengths.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+16
to
25
| def _normalize_vector(value: Any, param_name: str = "vector") -> tuple[list[float] | None, str | None]: | ||
| """ | ||
| Robustly normalize a vector parameter to [x, y, z] format. | ||
| Handles: list, tuple, JSON string, comma-separated string. | ||
| Returns None if parsing fails. | ||
| Returns (parsed_vector, error_message). If error_message is set, parsed_vector is None. | ||
| """ | ||
| if value is None: | ||
| return default | ||
| return None, None | ||
|
|
||
| # If already a list/tuple with 3 elements, convert to floats |
Contributor
There was a problem hiding this comment.
suggestion: Distinguish list/tuple inputs with incorrect length from entirely invalid types in error messages.
For non-string inputs, any list/tuple with length ≠ 3 falls through to the generic return None, f"{param_name} must be a list or JSON string, got {type(value).__name__}", yielding misleading errors like position must be a list or JSON string, got list when the issue is the element count.
Consider handling this explicitly:
if isinstance(value, (list, tuple)):
if len(value) != 3:
return None, f"{param_name} must have exactly 3 elements [x, y, z], got {len(value)}: {value}"
try:
vec = [float(value[0]), float(value[1]), float(value[2])]
...so callers get a precise, actionable message when the length is wrong.
Suggested implementation:
# If already a list/tuple, ensure it has exactly 3 elements and convert to floats
if isinstance(value, (list, tuple)):
if len(value) != 3:
return None, f"{param_name} must have exactly 3 elements [x, y, z], got {len(value)}: {value}"This change assumes that the existing body of the if block continues to convert the three elements to floats and handle any conversion errors as before. No further changes are required unless other parts of the function explicitly depend on the earlier and len(value) == 3 condition, in which case they should be updated to rely on the new early return for incorrect lengths.
Collaborator
|
Thanks! Looks good to me. I will add some fix of manage_components script in this PR and upload it together later. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes Pydantic validation error when clients pass vector/color parameters as JSON strings (e.g.,
"[2, 2, 2]").Previously, parameters like
scale,position,rotation, andcolorwere typed aslist[float], causing string inputs to be rejected at the Pydantic validation stage before reaching the normalization logic.Type of Change
Changes Made
manage_gameobject.py:
list[float]tolist[float] | strforposition,rotation,scale, andoffset_normalize_vector()to return(value, error_message)tuple instead of silently returningNoneon invalid inputmanage_material.py:
colorparameter type fromlist[float]tolist[float] | str_normalize_color()Testing/Screenshots/Recordings
Test Results
"[2, 2, 2]"{x:2, y:2, z:2}[1, 2, 3]"invalid""[1, 2]"Error Message Example
Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdRelated Issues
Fixes the validation error:
Input should be a valid list [type=list_type, input_value='[2, 2, 2]', input_type=str]Additional Notes
read_console.py:types: Annotated[list[Literal[...]] | str, ...]run_tests.py:test_names: Annotated[list[str] | str, ...]_normalize_component_properties()which returns(parsed_dict, error_message)Summary by Sourcery
Allow vector and color tool parameters to accept JSON strings and improve validation error reporting for game object management.
Bug Fixes:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.