Repository navigation
Conversation
schema_for_output used schemars' default deserialize contract, so an always-written Option or #[serde(default)] field was advertised as optional and a type accepting several forms advertised all of them. Tool results are serialised; inputs keep the deserialize contract. Closes modelcontextprotocol#1374
|
@connor4312 is on triage rotation this week and is the reviewer for this one. Labels come from the PR labeler. Please review the change. Automated triage. Labels are a first pass and the assigned maintainer has the final say. |
chrikrah
left a comment
There was a problem hiding this comment.
@atarutin this is mergeable at b1b6189. It fixes the example in #1374, and reverting the one line in schema_for_output fails the new test.
non-blocking: this changes what existing servers advertise. required grows on upgrade for a plain Option field and a #[serde(default)] field. A client that generates types from outputSchema sees those fields flip from optional to required. That is the intended correction, and the PR title carries it into the changelog.
non-blocking: #[tool(output_schema = schema_for_type::<T>())], as in test_json_schema_detection.rs:59, stays on the deserialize contract. That stays untouched, since an explicit override is the author saying what they mean.
$ cargo test -p rmcp --features client,server,macros,elicitation --lib --test test_json_schema_detection --test test_list_tools_result --test test_structured_output --test test_tool_builder_methods # head b1b6189, rustc 1.96.1
241 passed; 7 passed; 1 passed; 21 passed; 6 passed
# schema_for_output calling schema_for_type again, new tests kept
schema_for_output_describes_the_serialized_form ... FAILED
# struct { name: String, note: Option<String>, #[serde(default)] tags: Vec<String> }, schema_for_output::<T>()
# base 50fba5c "required":["name"]
# head b1b6189 "required":["name","note","tags"] # "tags" still carries "default":[]
$ cargo fmt --check # clean
# not run: clippy, the full --all-features suite (no pkg-config for openssl-sys here)
@connor4312, you're the assigned reviewer. The changelog line covers the required change. Does it also need a release note?
Closes #1374.
schema_for_outputnow generates withSchemaSettings::draft2020_12().for_serialize(). Tool results are values the server serialises, so their schema describes whatSerializewrites: anOptionwithoutskip_serializing_ifisrequired, and a type that accepts several forms advertises only the one it writes. Inputs (schema_for_type) keep the deserialize contract. There is no public API change, and for types whose two forms are equal the output schema is unchanged.Tests:
Optionisrequiredin an output schema, and a skipped one is not;Optionstays optional, and serde does accept the missing key.Ran:
cargo test -p rmcp --all-features, CI's rmcp clippy command (all features exceptlocal,-D warnings), andcargo +nightly fmt --check.