Skip to content

Fix pre_parse_json corrupting str | None values that look like JSON#3056

Open
vientooscuro wants to merge 1 commit into
modelcontextprotocol:mainfrom
vientooscuro:fix/pre-parse-json-optional-str
Open

Fix pre_parse_json corrupting str | None values that look like JSON#3056
vientooscuro wants to merge 1 commit into
modelcontextprotocol:mainfrom
vientooscuro:fix/pre-parse-json-optional-str

Conversation

@vientooscuro

Copy link
Copy Markdown

Summary

FuncMetadata.pre_parse_json() decided whether to json.loads() a string
argument based on field_info.annotation is not str. That check is True
for str | None as well (it's Optional[str], not literally str), so any
optional string parameter got the same "maybe this is JSON" treatment as a
list/dict parameter.

If a caller passes a valid string that also happens to parse as a JSON
object/array — e.g. a JSON-serialized payload passed as a plain string field
(block-based template body, serialized config, etc.) — the value silently
got replaced with a dict/list before the pydantic argument model was
validated, and validation then failed with Input should be a valid string
even though the caller's string was perfectly valid for that field.

Fixes #3055.

Fix

Added _is_optional_str(), which recognizes str | None (any union whose
only non-None member is str) and skips JSON pre-parsing for it, since no
member of such a union other than str could ever be produced by decoding
JSON — so pre-parsing that annotation can only corrupt, never help.

str | list[str] (and other unions with a real non-str member) keep the
existing behavior, since a JSON array/object there is meaningfully different
from the raw string and should still be pre-parsed — covered by the existing
test_str_vs_list_str test, which still passes.

Test plan

  • Added test_optional_str_is_never_json_pre_parsed covering dict-like,
    list-like, plain-string, and None values for a str | None param.
  • uv run pytest tests/server/mcpserver/test_func_metadata.py — 43 passed
  • uv run pytest tests/server/mcpserver (full suite) — 526 passed
  • uv run ruff check / ruff format --check on changed files

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No issues found across 2 files

Tip: cubic could auto-approve low-risk PRs like this, if it thinks it's safe to merge. Learn more

Re-trigger cubic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FuncMetadata.pre_parse_json mis-detects str | None as non-string and corrupts JSON-looking string arguments

1 participant