GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Replace RootModel wrappers with type aliases and TypeAdapter validation by Kludex · Pull Request #3470 · modelcontextprotocol/python-sdk Conversation
Member
Replace the reintroduced How Has This Been Tested?The full suite passes with 5,968 tests and 100% line and branch coverage; Breaking ChangesWire formats are preserved. AI DisclaimerThis PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
|
There was an error while loading. Please reload this page.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
📚 Documentation preview
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 11 files
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| # 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.
The reason will be displayed to describe this comment to others. Learn more.
🔴 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]").
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…
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.