Replace RootModel wrappers with type aliases and TypeAdapter validation - #3470
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. |
📚 Documentation preview
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding, I also checked the auth path in src/mcp/server/auth/handlers/authorize.py — the TypeAdapter(AnyUrl) swap raises the same ValidationError the existing handler catches, so redirect_uri validation behavior is unchanged — and the _wire_fields RootModel-branch removal in src/mcp/client/session.py, which is safe now that the TID251 ban (with the generated-files exemption removed) keeps RootModel rows out of the surface types.
Extended reasoning...
A finding on the FuncMetadata.output_model compatibility break is already posted inline, so this note only records what else was concretely examined and ruled out: the authorization handler's redirect_uri validation (exception type and catch site unchanged), the elicitation property-schema gate (same PrimitiveSchemaDefinition validation via adapter), and the client session's wire-field computation (the dropped RootModel-unwrapping branch is dead once no generated type is a RootModel, which the widened lint ban enforces). The remaining bulk of the diff is regenerated code plus generator changes, which the inline finding and a maintainer's judgment on the API-contract question should govern.
2 verified lower-impact observations (convention, logging or cleanup points) were not posted.
| # TODO: should we use the original annotation? We are losing any potential `Annotated` | ||
| # metadata for Pydantic here: | ||
| model = _create_dict_model(func_name, type_expr) | ||
| model = Annotated[type_expr, Field(title=f"{func_name}DictOutput")] |
There was a problem hiding this comment.
🔴 For dict[str, T]-returning tools, the documented public field FuncMetadata.output_model now holds an Annotated[...] alias instead of the BaseModel subclass it held on every released 2.x version, so existing user code calling meta.output_model.model_validate(...), model_json_schema() or issubclass(meta.output_model, BaseModel) crashes with AttributeError/TypeError after a routine minor upgrade — AGENTS.md says 2.x observable behaviour must not change even with shims. Fix: keep dict outputs class-shaped without RootModel (e.g. a thin wrapper exposing model_validate) or defer this to 3.0 behind a deprecation path; also update the now-stale docstring at line 317 ("synthesized model for ... dict[str, T]").
Extended reasoning...
The PR description flags this as a breaking change ("validate it with TypeAdapter") — sized against the repo contract, that note does not hold: AGENTS.md states the 2.x public API is a compatibility contract and observable-behaviour changes "should generally be avoided" even when softened by @ deprecated shims; there is no shim here. FuncMetadata is documented user-facing API (docs/servers/structured-output.md, updated in this PR, tells users to inspect output_model). On base, _create_dict_model (removed at old lines 625-638) returned a RootModel subclass named {func}DictOutput; func_metadata()'s docstring still promises "a synthesized model for ... dict[str, T] ... returns" (line 317-318). After merge, _create_output_model line 511 stores Annotated[dict[str, T], Field(title=...)], and the field annotation widened from type[Any] | None to Annotated[Any, ...] (line 123), so pyright no longer flags misuse either. Any 2.x consumer that introspects tool metadata (test harnesses, custom validation layers, tool registries re-deriving schemas) and calls a classmethod on…
Verification: normal — acknowledged in diff: the PR description's "Breaking Changes" section states "FuncMetadata.output_model for dictionary returns now holds a type annotation instead of a model class; validate it with TypeAdapter", so this is a declared trade-off put up for sign-off, not an oversight; conflicts with stated purpose: restoring a BaseModel class for root-level dict serialization would…
2 participants
Replace the reintroduced
RootModelwrappers with plain types and unions validated byTypeAdapter, following #1910. Update code generation and enforce Ruff'sTID251ban for both import paths, including generated files.How Has This Been Tested?
The full suite passes with 5,968 tests and 100% line and branch coverage;
strict-no-coverpasses. The focused suite passes all 605 tests on Python 3.10, and Ruff, Pyright, the lockfile check, and regeneration checks pass.Breaking Changes
Wire formats are preserved.
FuncMetadata.output_modelfor dictionary returns now holds a type annotation instead of a model class; validate it withTypeAdapter. TheAnyUrlModelauthorization helper is removed.AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.