Repository navigation
Drop generated titles from tool input and output schemas #3757
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,7 +63,17 @@ def _returns_content(annotation: Any) -> bool: | |
| return False | ||
|
|
||
|
|
||
| class StrictJsonSchema(GenerateJsonSchema): | ||
| class UntitledJsonSchema(GenerateJsonSchema): | ||
| """A JSON schema generator that leaves out the titles pydantic derives from field names. | ||
|
|
||
| They only repeat the property key; titles set explicitly with `Field(title=...)` are kept. | ||
| """ | ||
|
|
||
| def field_title_should_be_set(self, schema: Any) -> bool: | ||
| return False | ||
|
|
||
|
|
||
| class StrictJsonSchema(UntitledJsonSchema): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Nit (optional): code outside the SDK that passes Why this was flaggedCondition: a caller outside this repo uses Verification: Triggers when code outside this repo passes or subclasses Finding ID: |
||
| """A JSON schema generator that raises exceptions instead of emitting warnings. | ||
|
|
||
| This is used to detect non-serializable types during schema generation. | ||
|
|
@@ -128,6 +138,9 @@ def model_post_init(self, context: Any, /) -> None: | |
| if self.output_model is not None and self.output_schema is None: | ||
| # StrictJsonSchema raises instead of warning, so an unserializable return type fails construction. | ||
| schema = self._output_adapter(self.output_model).json_schema(schema_generator=StrictJsonSchema) | ||
| if self.wrap_output: | ||
| # The `{"result": ...}` wrapper is ours, so its class-name title says nothing. | ||
| schema.pop("title", None) | ||
| self.output_schema = _inline_root_ref(schema) | ||
|
|
||
| def _output_adapter(self, output_model: Any) -> TypeAdapter[Any]: | ||
|
|
@@ -338,7 +351,7 @@ def func_metadata( | |
| field_metadata: list[Any] = [] | ||
|
|
||
| if param.annotation is inspect.Parameter.empty: | ||
| field_metadata.append(WithJsonSchema({"title": param.name, "type": "string"})) | ||
| field_metadata.append(WithJsonSchema({"type": "string"})) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 (optional) Unannotated parameters keep a title outside tools, and it changes from the parameter name to a pydantic title-cased one ( Why this was flaggedTrigger: a function with an unannotated parameter ( Verification: Triggered when a function with an unannotated parameter is registered as a resource template. Finding ID: |
||
| # Check if the parameter name conflicts with BaseModel attributes | ||
| # This is necessary because Pydantic warns about shadowing parent attributes | ||
| if hasattr(BaseModel, field_name) and callable(getattr(BaseModel, field_name)): | ||
|
|
@@ -508,7 +521,7 @@ def _create_output_model(original_annotation: Any, type_expr: Any, func_name: st | |
| if len(args) == 2 and args[0] is str: | ||
| # TODO: should we use the original annotation? We are losing any potential `Annotated` | ||
| # metadata for Pydantic here: | ||
| model = Annotated[type_expr, Field(title=f"{func_name}DictOutput")] | ||
| model = type_expr | ||
|
Comment on lines
522
to
+524
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 (optional) pre-existing: A tool returning Why this was flaggedTrigger: a tool declared Verification: pre-existing: triggers when a tool's return annotation is Finding ID: |
||
| else: | ||
| # dict with non-str keys needs wrapping | ||
| model = _create_wrapped_model(func_name, original_annotation) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 nit (optional): AGENTS.md makes the v2 public API a 2.x compatibility contract and says any change to observable behaviour is a design decision a maintainer makes explicitly: this drops generated
titlekeys from every MCPServer tool'stools/listinput and output schema, and changesStrictJsonSchemaandFuncMetadata.output_modelfordict[str, X]. Fix: record an explicit maintainer decision on the PR, or keep the current output as default and make untitled schemas opt-in, which covers the 6 sites listed. Same instruction at 6 sites (tools/base.py:107, tools/base.py:109, utilities/func_metadata.py:76, :143, :354, :524).Why this was flagged
Nothing fails at runtime, and validation is unaffected because
titleis an annotation keyword. What the instruction guards is the 2.x compatibility contract: after this change every MCPServer tool'stools/listentry loses the<func>Argumentsroot title, the<func>Outputroot title of wrapped results, the<func>DictOutputtitle ofdict[str, X]results, the parameter-name title of unannotated parameters, and all field-name-derived property titles. It bites for a 2.x user who upgrades and has a client or UI that displays or keys on those titles, a snapshot test of their own tool schemas, or code that relies onoutput_schema == Model.model_json_schema()(the diff itself had to rewritetest_basemodel_union_input_required_result_derives_model_schemabecause that equality no longer holds). Two importable classes also change behaviour:StrictJsonSchemanow suppresses field titles for anyone using it directly, andFuncMetadata.output_modelfordict[str, X]is the bare type rather thanAnnotated[..., Field(title=...)].Verification: Instruction: AGENTS.md (base 86ca12b), "Branching Model", second bullet, present as quoted: "v2 is released; its public API is a compatibility contract for the 2.x line. Removals, renames, or any change to an existing API's signature or observable behaviour (including ones softened by a
@ deprecatedshim) is a design decision a maintainer makes explicitly, and should generally be avoided." It binds library code under src/.Finding ID:
f-833dd265