Repository navigation
Treat typing.Dict[str, X] return annotations like dict[str, X] - #3756
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. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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: 9d07811d75
ℹ️ 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".
| elif (origin := get_origin(type_expr)) is not None: | ||
| if origin is dict: | ||
| args = get_args(type_expr) | ||
| if len(args) == 2 and args[0] is str: |
There was a problem hiding this comment.
For existing tools annotated with typing.Dict[str, X], this changes both the advertised output schema and returned structured content from {"result": {...}} to the dictionary itself. However, no documentation accompanies this user-visible change. Update the existing docs/servers/structured-output.md page to explain that the legacy spelling now behaves like dict[str, X], including the changed wrapping behavior for existing consumers. The repository explicitly requires relevant documentation updates in the same change.
AGENTS.md reference: AGENTS.md:L144-L148
Useful? React with 👍 / 👎.
| assert func_metadata(func_typing_dict).output_schema == { | ||
| "type": "object", | ||
| "additionalProperties": {"type": "string"}, | ||
| "title": "func_typing_dictDictOutput", | ||
| } |
There was a problem hiding this comment.
Both new schema assertions compare SDK-produced transformations against handwritten dictionary literals. The test-quality guide's Assertions section requires full-object snapshot(...) equality for transformations and requires generating those snapshots with --inline-snapshot=fix, rather than hand-editing expected literals. Convert the new typing.Dict and NewType expectations to generated snapshots instead of copying the legacy assertion pattern; AGENTS.md explicitly makes that guide applicable when reviewing tests.
AGENTS.md reference: AGENTS.md:L68-L69
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
One optional note from this repository's REVIEW.md or CLAUDE.md checks was not posted as a comment, over this review's limit for such notes; it is on this commit's check card.
| origin = get_origin(type_expr) | ||
|
|
||
| # Handle parameterized generics (list[str], typing.Dict[str, int], Union[str, int], etc.) | ||
| elif (origin := get_origin(type_expr)) is not None: |
There was a problem hiding this comment.
🟡 (optional) Tools returning Annotated[typing.Dict[str, X], Field(...)] lose their pydantic metadata after merging: constraints stop being enforced and descriptions leave output_schema. The dict branch now reached at func_metadata.py:502 builds the model from the stripped type_expr at :508, not from original_annotation. Fix: build the str-keyed dict model from the original annotation, e.g. Annotated[original_annotation, Field(title=...)], so metadata survives for both typing.Dict and builtin dict. The loss is pre-existing for dict[str, X] (see the TODO at :506); this change widens it to the typing.Dict spelling.
Why this was flagged
A tool annotated -> Annotated[typing.Dict[str, int], Field(min_length=1, description="scores")] reaches _create_output_model from func_metadata.py:443 with type_expr = typing.Dict[str, int] and original_annotation still carrying the Field. get_origin is dict, so :502-508 now returns Annotated[type_expr, Field(title=f"{func_name}DictOutput")], which discards the user's Field. On the base, typing.Dict[...] failed isinstance(type_expr, GenericAlias) and fell to the catch-all _create_wrapped_model(func_name, original_annotation), so the schema's result property carried minProperties/description and convert_result (:218) rejected an empty dict. After merging, the schema has neither and an empty dict validates. No safeguard applies: tests/server/mcpserver/test_func_metadata.py:1471 (test_preserves_pydantic_metadata) only covers Annotated[int, Field(gt=1)].
Verification: Triggers when a tool's return annotation is Annotated[typing.Dict[str, X], Field(...)]. In src/mcp/server/mcpserver/utilities/func_metadata.py, on the base typing.Dict[str, int] reached the final else: _create_wrapped_model(func_name, original_annotation). On the head, line 502 now matches and line 508 is built from the stripped type_expr, so the user's Field is gone.
Finding ID: f-f90d2307
There was a problem hiding this comment.
I reviewed the updated PR and found no bugs; the follow-up commit adds the docs note and the structured_content assertions and drops the # pragma: no cover markers. A maintainer look is still worthwhile because this deliberately changes the wire shape for existing -> typing.Dict[str, X] tools on the stable line.
What was reviewed:
- Traced
_create_output_modelinsrc/mcp/server/mcpserver/utilities/func_metadata.pyfor annotations that newly enter theget_origin(...) is not Nonebranch:typing.List/Tuple,Union/Optional/X | Y,Literal,typing.Mapping, subscripted user generics andtyping.Dictwith non-strkeys all still end up wrapped, as before. - Bare
typing.Dictnow has origindictbutget_argsreturns(), so it fails thelen(args) == 2check and stays wrapped;NewTypeand unsubscripted type aliases have no origin and still reach the catch-all. - No other use of the removed
GenericAliasimport remains in the module. - The second commit touches only tests and docs, so I could not confirm from the code whether the other reviewer's inline comment on
func_metadata.pywas acted on.
Extended reasoning...
The change is three files (+33/-8): one branch condition in _create_output_model, two test cases and a one-line docs parenthetical, touching no auth, injection or data-exposure surface. By reading the code, the only observable behaviour change is that typing.Dict[str, X] returns are no longer wrapped in {"result": ...}; every other annotation that newly takes the generic branch is wrapped exactly as before. That is still a change to published output_schema and structured_content for existing servers, which AGENTS.md treats as an explicit maintainer decision on the 2.x line. An inline comment from another automated reviewer on func_metadata.py predates a follow-up commit that did not touch that file, so it may still be open. No CODEOWNERS file exists in the checkout, and nothing here was executed — the conclusions come from reading the code only.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
📚 Documentation preview
|
Fixes #3745
-> typing.Dict[str, X]now gets the same output schema andstructuredContentas-> dict[str, X], instead of being wrapped in{"result": ...}.Motivation and Context
_create_output_modeldetected generics withisinstance(type_expr, types.GenericAlias).typing.Dict[str, X]is atyping._GenericAlias, so it fell through to the catch-all branch and got wrapped. RewritingDicttodict(e.g. ruff UP006) therefore silently changed a tool's output on the wire.The branch now checks
get_origin(type_expr) is not None.How Has This Been Tested?
typing.Dict[str, str]case totest_structured_output_dict_str_types; it fails without the fix.NewTypecase totest_structured_output_generic_types, which is now what reaches the catch-all branch.func_metadata.pyat 100% coverage,strict-no-cover, ruff and pyright pass; test file passes on 3.11, 3.13 and 3.14.Breaking Changes
Tools annotated
-> typing.Dict[str, X]stop wrapping their result in{"result": ...}, matching the builtin spelling. Union, Optional, Literal andtyping.List/Tuplenow take the generic branch instead of the catch-all, but are still wrapped as before.Types of changes
Checklist
help wanted, or I'm a maintainer)Additional context
Related: #3725 (bare
dict/list/tuple), which touches the same function.AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.