Repository navigation
Drop generated titles from tool input and output schemas - #3757
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e282569400
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
📚 Documentation preview
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 24 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| if param.annotation is inspect.Parameter.empty: | ||
| field_metadata.append(WithJsonSchema({"title": param.name, "type": "string"})) | ||
| field_metadata.append(WithJsonSchema({"type": "string"})) |
There was a problem hiding this comment.
🟡 (optional) Unannotated parameters keep a title outside tools, and it changes from the parameter name to a pydantic title-cased one (x becomes X). func_metadata.py:354 drops the explicit "title": param.name. Only base.py:107 passes UntitledJsonSchema. The default generator at resources/templates.py:156 and prompts/base.py:130 then derives its own title. Fix: make every arg_model schema call agree, either by passing schema_generator=UntitledJsonSchema at those two sites or by keeping the name title for non-tool callers.
Why this was flagged
Trigger: a function with an unannotated parameter (def f(city): ..., or a lambda) registered as a resource template or prompt, or any caller of func_metadata(fn).arg_model.model_json_schema(). On the base, func_metadata.py:354 attached WithJsonSchema({"title": param.name, "type": "string"}), so the property was {"title": "city", "type": "string"}. Now it attaches {"type": "string"} and the default pydantic generator fills in a derived title. The diff's own test updates show the result: tests/server/mcpserver/test_func_metadata.py:314-315 go from "title": "x" to "title": "X". ResourceTemplate.parameters (resources/templates.py:116) is built at templates.py:156 with no schema_generator, so its stored schema changes value instead of losing the title. The PR text says this title is dropped; that holds only for tools. No guard applies: the field itself is public output.
Verification: Triggered when a function with an unannotated parameter is registered as a resource template. src/mcp/server/mcpserver/utilities/func_metadata.py:354 now attaches WithJsonSchema({"type": "string"}). Only src/mcp/server/mcpserver/tools/base.py:107 passes schema_generator=UntitledJsonSchema. So outside tools the title is replaced by a title-cased one, not dropped.
Finding ID: f-3d8f4ff9
| return False | ||
|
|
||
|
|
||
| class StrictJsonSchema(UntitledJsonSchema): |
There was a problem hiding this comment.
🟡 Nit (optional): code outside the SDK that passes StrictJsonSchema as a schema generator silently loses field titles after upgrading within 2.x. func_metadata.py:76 rebases this existing, API-reference-documented class onto UntitledJsonSchema. On the base branch it only turned schema warnings into errors. Fix: scope the title drop to tool schemas without changing the existing class, e.g. keep StrictJsonSchema(GenerateJsonSchema) and have func_metadata.py:140 use a private subclass of both. Tool output stays as this PR intends, so this is not the opt-in fix for the wire change.
Why this was flagged
Condition: a caller outside this repo uses mcp.server.mcpserver.utilities.func_metadata.StrictJsonSchema directly, e.g. Model.model_json_schema(schema_generator=StrictJsonSchema), or subclasses it. Such callers cannot be enumerated from this checkout; the only in-repo use is…
Verification: Triggers when code outside this repo passes or subclasses StrictJsonSchema directly. Base: class StrictJsonSchema(GenerateJsonSchema) overrode only emit_warning. Head, src/mcp/server/mcpserver/utilities/func_metadata.py:76: class StrictJsonSchema(UntitledJsonSchema), so it now also drops every field-name-derived title. The docstring (lines 77-80) is unchanged.
Finding ID: f-2795f25f
| # 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 |
There was a problem hiding this comment.
🟡 (optional) pre-existing: A tool returning Annotated[dict[str, T], Field(title=...)] still loses its explicit title, although this change promises explicit titles are kept. func_metadata.py:524 builds the model from type_expr, which has the Annotated metadata stripped. The title and any description never reach outputSchema. Fix: build the dict[str, T] output model from original_annotation so Field(...) metadata survives, as it already does for wrapped returns. The base overwrote the title with <func>DictOutput; now the schema has no title at all.
Why this was flagged
Trigger: a tool declared -> Annotated[dict[str, float], Field(title="Temperatures", description="By city")], registered through Tool.from_function (src/mcp/server/mcpserver/tools/base.py:102), which calls func_metadata and then _create_output_model (func_metadata.py:457). inspect_annotation(...).type at func_metadata.py:390 strips Annotated, and line 524 sets model = type_expr. The schema derived at func_metadata.py:140 is {"additionalProperties": {"type": "number"}, "type": "object"} with neither the title nor the description. The base lost the same metadata: it wrapped type_expr in Field(title=f"{func_name}DictOutput"), so this is not a regression, and the TODO at func_metadata.py:522-523 names the loss. The wrapped path passes original_annotation to _create_wrapped_model (func_metadata.py:531), which is why Field metadata survives there. No safeguard applies: the new test at tests/server/mcpserver/test_tool_manager.py:910 only checks a BaseModel return.
Verification: pre-existing: triggers when a tool's return annotation is Annotated[dict[str, T], Field(title=..., description=...)]. For dict[str, T] (src/mcp/server/mcpserver/utilities/func_metadata.py lines 519-524) the model is model = type_expr, so the Field(...) metadata held only in original_annotation is discarded. The base line was model = Annotated[type_expr, Field(title=f"{func_name}DictOutput")], also built from the stripped type_expr.
Finding ID: f-fe6a2e07
| assert manager.get_tool("test_func") is None | ||
|
|
||
|
|
||
| def test_tool_schemas_keep_only_titles_set_explicitly(): |
There was a problem hiding this comment.
🟡 nit (optional): The new test has no docstring, so a later failure gives no hint whether the pinned schemas are spec-mandated or SDK-defined. The repo's test-quality guide asks for a 1-2 sentence behaviour docstring that states provenance. Fix: add a docstring under tests/server/mcpserver/test_tool_manager.py:910 saying this is SDK-defined behaviour: generated titles are dropped and explicit Field(title=...) titles are kept. [also at: tests/server/mcpserver/test_tool_manager.py:931 - nit: The new test falls short of the repo's test-quality bar, so a later failure is harder to triage. test_tool_schemas_keep_only_titles_set_explicitly has no docstring, compares hand-written dict literals, and gives the never-invoked find_book a ... body. Fix: add a 1-2 sentence docstring… No runtime…]
Why this was flagged
There is no runtime trigger; this is a convention check on a test the diff adds. test_tool_schemas_keep_only_titles_set_explicitly at tests/server/mcpserver/test_tool_manager.py:910 goes straight from the def line to class Book(BaseModel): at line 911. .claude/skills/test-quality/SKILL.md:16-19 says "Docstrings: 1-2 sentences of behaviour, honest about provenance" and calls provenance "the triage key when the test later fails". The base branch does not have this test, so the gap is new. The same guide (SKILL.md:106-107) asks that registered-but-never-invoked bodies be raise NotImplementedError; find_book at line 915 uses .... No safeguard enforces either rule: ruff selects only D212 from pydocstyle (pyproject.toml:214).
Verification: tests/server/mcpserver/test_tool_manager.py:910-911 is added by this diff and goes straight from def test_tool_schemas_keep_only_titles_set_explicitly(): to class Book(BaseModel): with no docstring. The test pins SDK-defined schema output and does not say so. Nothing breaks: the test passes and its name already describes the behaviour.
Finding ID: f-60ff036b
| parameters = func_arg_metadata.arg_model.model_json_schema(by_alias=True, schema_generator=UntitledJsonSchema) | ||
| # The arguments model is generated, so its class-name title says nothing. | ||
| parameters.pop("title", None) |
There was a problem hiding this comment.
🟡 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 title keys from every MCPServer tool's tools/list input and output schema, and changes StrictJsonSchema and FuncMetadata.output_model for dict[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 title is an annotation keyword. What the instruction guards is the 2.x compatibility contract: after this change every MCPServer tool's tools/list entry loses the <func>Arguments root title, the <func>Output root title of wrapped results, the <func>DictOutput title of dict[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 on output_schema == Model.model_json_schema() (the diff itself had to rewrite test_basemodel_union_input_required_result_derives_model_schema because that equality no longer holds). Two importable classes also change behaviour: StrictJsonSchema now suppresses field titles for anyone using it directly, and FuncMetadata.output_model for dict[str, X] is the bare type rather than Annotated[..., 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 @ deprecated shim) is a design decision a maintainer makes explicitly, and should generally be avoided." It binds library code under src/.
Finding ID: f-833dd265
| assert manager.get_tool("test_func") is None | ||
|
|
||
|
|
||
| def test_tool_schemas_keep_only_titles_set_explicitly(): |
There was a problem hiding this comment.
🟡 nit (optional): AGENTS.md requires type hints for all code: the new top-level test test_tool_schemas_keep_only_titles_set_explicitly() is declared with no return annotation. Fix: annotate newly added functions with their return type, here -> None, as the tests/docs_src tests already do.
Why this was flagged
Nothing fails at runtime and pyright still passes, since it infers the return type. The instruction guards consistent, fully annotated code; it bites only as a convention gap on the one function this diff adds. Sized honestly: this is small, and every other test in tests/server/mcpserver/test_tool_manager.py and tests/server/mcpserver/tools/test_base.py also omits -> None, while the tests under tests/docs_src/ include it. The nested helper find_book and the new UntitledJsonSchema.field_title_should_be_set in src/ are fully annotated.
Verification: Instruction: AGENTS.md at base 86ca12b, section "## Code Quality", reads verbatim "- Type hints required for all code"; it is repo-root scoped and has no carve-out for tests. Code: tests/server/mcpserver/test_tool_manager.py:910, added by this diff, is def test_tool_schemas_keep_only_titles_set_explicitly(): with no return annotation. The function takes no parameters, so it carries no type hint at all. For the maintainer: nothing fails at runtime and pyright infers the return type.
Finding ID: f-b0366653
MCPServertools no longer publish the titles pydantic and the SDK generate ininputSchemaandoutputSchema. Titles set explicitly withField(title=...)are kept.Motivation and Context
Every tool schema carried titles that only repeat information already there:
{ "type": "object", "properties": { "query": {"title": "Query", "type": "string"}, "limit": {"default": 10, "title": "Limit", "type": "integer"} }, "required": ["query"], "title": "search_booksArguments" }inputSchemais what clients hand to the model, so these are tokens on every tool, every turn, with no information. Now:{ "type": "object", "properties": { "query": {"type": "string"}, "limit": {"default": 10, "type": "integer"} }, "required": ["query"] }What is dropped:
"Query","Result"), viaGenerateJsonSchema.field_title_should_be_set. This only governs the derived title, so an explicitField(title=...)still lands in the schema.<func>Argumentsroot title ofinputSchema, the<func>Outputroot title of a wrapped{"result": ...}output, and the<func>DictOutputtitle ofdict[str, X]outputs. These name models the SDK creates internally; theDictOutputone only existed to keep the schema identical when itsRootModelwas removed in Replace RootModel wrappers with type aliases and TypeAdapter validation #3470.What is kept: explicit
Field(title=...)titles, and the class-name titles of user types ("title": "Book"for a-> Booktool, and in$defs).How Has This Been Tested?
--inline-snapshot=fix, literal dicts edited by hand).test_tool_schemas_keep_only_titles_set_explicitly, which checks that explicit titles survive on both the input and output side.strict-no-coverand pyright clean.Breaking Changes
No code changes needed, but
tools/listoutput changes: the titles above disappear frominputSchemaandoutputSchema. Validation is unaffected, sincetitleis an annotation keyword. A client that displayed or keyed on those generated titles would see them gone.Types of changes
Checklist
help wanted, or I'm a maintainer)Additional context
The JSON examples in
docs/are updated to the new output. Translations underi18n/are not touched.This overlaps with #3756 in
test_structured_output_dict_str_types: whichever lands second needs to drop thefunc_typing_dictDictOutputtitle from that test.AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.