fix: don't JSON pre-parse string args whose annotation only accepts str or None#3062
Closed
akminx wants to merge 1 commit into
Closed
fix: don't JSON pre-parse string args whose annotation only accepts str or None#3062akminx wants to merge 1 commit into
akminx wants to merge 1 commit into
Conversation
…tr or None pre_parse_json decided whether to json.loads a string argument with 'field_info.annotation is not str', which treats 'str | None' like a non-string annotation. A valid string value that happens to parse as a JSON object or array (e.g. a JSON-serialized template body) was silently replaced with a dict/list and then failed model validation. Only skip pre-parsing when every union member is str or None, so a plain string is already fully valid. Unions with non-str members such as 'str | list[str]' keep the existing stringified-JSON handling. Fixes modelcontextprotocol#3055
dfae0c7 to
fd53853
Compare
4 tasks
Author
|
Closing in favor of #3056, which predates this PR and I hadn't spotted before opening. Left a note there about the |
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.
Motivation and Context
Fixes #3055.
FuncMetadata.pre_parse_jsondecides whether tojson.loadsa string argument withfield_info.annotation is not str. That treatsstr | Nonelike a non-string annotation, so a valid string value that happens to parse as a JSON object/array (e.g. a JSON-serialized template body) is silently replaced with adict/listand then fails validation of the argument model withInput should be a valid string.How Has This Been Tested?
Added
test_optional_str_not_parsed_as_json, which fails onmainand passes with this change. The fulltest_func_metadata.pysuite passes (43 tests), andruff check/ruff formatare clean.Implementation notes
The issue suggested skipping pre-parsing for any union that includes
str, but that would change the documented behavior covered bytest_str_vs_list_str: forstr | list[str], a stringified JSON array is still expected to parse into a list (the Claude Desktop workaround this hook exists for). So the new check is narrower: pre-parsing is skipped only when every union member isstrorNone— in that case a plain string is already fully valid and parsing could only corrupt it. Unions with non-str members keep the existing behavior, andAnnotatedmembers are unwrapped before the check.Breaking Changes
None. Behavior changes only for parameters whose annotation is
str/None-only unions, where the previous behavior corrupted valid input.Types of changes
Checklist