Skip to content

Upgrade types.py to 2025-03-26 protocol version #456

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

john0312
Copy link
Contributor

@john0312 john0312 commented Apr 8, 2025

This PR upgrades types.py to the 2025-03-26 protocol version.

Motivation and Context

This is needed to support batch JSON RPC #444

How Has This Been Tested?

All tests and type check passed.

Breaking Changes

None

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

@john0312 john0312 force-pushed the upgrade_protocol_20250326 branch from a5a4ad2 to a2e3bc7 Compare April 8, 2025 16:42
@john0312
Copy link
Contributor Author

john0312 commented Apr 8, 2025

The support for tool annotations #445 will probably benefit from this PR as well.

@HeetVekariya
Copy link

@john0312 PR seems good.

  • We do need to add tests for this annotation functionality same as added in the Swift SDK by @mattt .
  • I would like to add tests for it, as in when the PR got merged. If you are working on it, we can collaborate for the same.

@john0312
Copy link
Contributor Author

john0312 commented Apr 9, 2025

@HeetVekariya I'm not a maintainer, so it is not my call to make on who gets to work on tool annotation tests. In fact, I'm not even certain if my PR will get reviewed/merged.

@@ -705,15 +748,77 @@ class ListToolsRequest(PaginatedRequest[RequestParams | None, Literal["tools/lis
params: RequestParams | None = None


class ToolAnnotations(BaseModel):
"""
Copy link

Choose a reason for hiding this comment

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

To set the optional bool default values, I think you would do:

    readOnlyHint: bool = False
    destructiveHint: bool = True
    idempotentHint: bool = False
    openWorldHint: bool = True

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking through the rest of the code, the convention seems to be to set the default to None instead of False/True? I think the default convention makes sense because we want to tell if the other side specified True/False or left it absent.

Note that this PR probably conflicts with a later PR #482, in which the default is left as None as well.

Not sure if you've permission to merge, but if you do, it looks like PR #482 is likely to be merged, so maybe I'll leave the ToolAnnotations out so we can merge the rest?

@john0312 john0312 force-pushed the upgrade_protocol_20250326 branch from a2e3bc7 to d91ccdf Compare April 16, 2025 07:30
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.

3 participants