Skip to content

Conversation

@Kludex
Copy link
Member

@Kludex Kludex commented Jan 23, 2026

Now properly matching the specification.

Comment on lines 472 to 479
class GetTaskPayloadResult(Result):
"""The response to a tasks/result request.
The structure matches the result type of the original request.
For example, a tools/call task would return the CallToolResult structure.
"""

model_config = ConfigDict(extra="allow")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This matches the additionalProperties in the specification.


# TODO(Marcelo): The extra="allow" should be only on specific types e.g. `Meta`, not on the base class.
model_config = ConfigDict(extra="allow", alias_generator=to_camel, populate_by_name=True)
model_config = ConfigDict(alias_generator=to_camel, populate_by_name=True)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now properly done.

Copy link
Contributor

@maxisbey maxisbey Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(quick comment before I review) just to link related things, there's ongoing discussions around this here: modelcontextprotocol/modelcontextprotocol#2084

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we should have it just ignore extra fields. Wouldn't this cause errors if a newer server sends back extra stuff in an object?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ignoring extra fields now. That's what this PR is doing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 3 options: extra being allow, ignore or forbid. By default it's ignore - which is now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh I thought default was forbid

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

"httpx-sse>=0.4",
"pydantic>=2.12.0; python_version >= '3.14'",
"pydantic>=2.11.0; python_version < '3.14'",
"pydantic>=2.12.0",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to bump it for this to work because the extra="allow" behavior doesn't work properly in TypedDicts on previous versions. Given that we are forced to do it on 3.14, I think it makes sense to change this to have a clean state here.

@Kludex Kludex requested a review from maxisbey January 23, 2026 10:48
@claude
Copy link

claude bot commented Jan 23, 2026

Code Review - Issue 1: model_config overwrites parent configuration

File: src/mcp/types/_types.py (line 479)

Issue: The model_config in GetTaskPayloadResult completely replaces the parent configuration instead of extending it.

In Pydantic v2, when a child class defines model_config, it completely replaces the parent's config rather than merging. The parent class MCPModel defines:

model_config = ConfigDict(alias_generator=to_camel, populate_by_name=True)

See:

"""Base class for all MCP protocol types. Allows extra fields for forward compatibility."""
model_config = ConfigDict(alias_generator=to_camel, populate_by_name=True)

By setting model_config = ConfigDict(extra="allow") in GetTaskPayloadResult, you lose:

  • alias_generator=to_camel - converts Python snake_case to JSON camelCase
  • populate_by_name=True - allows using either the alias or field name

This breaks serialization consistency for this class compared to all other MCP types.

Fix: Merge the configs instead of replacing:

    model_config = ConfigDict(extra="allow", alias_generator=to_camel, populate_by_name=True)

@claude
Copy link

claude bot commented Jan 23, 2026

Code Review - Issue 2: Missing tests for behavior change

Reference: CLAUDE.md Testing Requirements

This PR removes extra="allow" from the base MCPModel class, which is a significant behavior change affecting ~60 model classes. The change impacts:

  1. Data validation: Models now reject/ignore extra fields instead of accepting them
  2. Forward compatibility: The docstring promises "Allows extra fields for forward compatibility" - this behavior is being removed
  3. Storage behavior: Extra fields are no longer preserved on model instances

According to CLAUDE.md:

New features require tests
Bug fixes require regression tests

No tests were added in this PR to verify:

  • GetTaskPayloadResult still correctly accepts extra fields
  • Other models properly handle extra fields per the new behavior
  • Forward compatibility scenarios work as intended
  • Existing functionality is preserved

Please add tests to cover this validation behavior change.

@claude
Copy link

claude bot commented Jan 23, 2026

Code Review - Issue 3: Breaking changes not documented in migration guide

Reference: CLAUDE.md Breaking Changes

This PR introduces two breaking changes that require migration documentation:

1. Removal of extra="allow" from MCPModel

Impact: All MCP protocol types will no longer accept extra fields by default (except GetTaskPayloadResult). This breaks:

  • Forward compatibility with future protocol versions that add fields
  • Any code relying on extra fields being preserved
  • The documented behavior in MCPModel's docstring

2. Pydantic version requirement increase (2.11.0 → 2.12.0)

Impact: Users currently on Pydantic 2.11.0 will be forced to upgrade, which could break their dependency graphs.

According to CLAUDE.md:

When making breaking changes, document them in docs/migration.md. Include:

  • What changed
  • Why it changed
  • How to migrate existing code

Please add a section to docs/migration.md documenting both breaking changes and providing migration guidance for users.

@Kludex
Copy link
Member Author

Kludex commented Jan 23, 2026

Ah, now I understand what the comment about: Allows extra fields for forward compatibility. means. But that doesn't matter, because if the field is not being handled, it can be just ignored. Isn't that the case?

@Kludex
Copy link
Member Author

Kludex commented Jan 23, 2026

Ah, now I understand what the comment about: Allows extra fields for forward compatibility. means. But that doesn't matter, because if the field is not being handled, it can be just ignored. Isn't that the case?

Answering my own question: I checked the C# SDK, and they just ignore the fields in older SDK versions, which makes completely sense since you can't handle those fields - but still doesn't error, which is what we want here!

@maxisbey
Copy link
Contributor

Ah, now I understand what the comment about: Allows extra fields for forward compatibility. means. But that doesn't matter, because if the field is not being handled, it can be just ignored. Isn't that the case?

Answering my own question: I checked the C# SDK, and they just ignore the fields in older SDK versions, which makes completely sense since you can't handle those fields - but still doesn't error, which is what we want here!

this follows the typescript approach as well, so sounds good to me too

@Kludex Kludex merged commit a1d330d into main Jan 23, 2026
40 checks passed
@Kludex Kludex deleted the fix-loose-extra branch January 23, 2026 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants