-
Notifications
You must be signed in to change notification settings - Fork 29
feat: Add mcp transport protocol #345
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
base: client-transport-decouple
Are you sure you want to change the base?
Conversation
7e58222
to
78014d7
Compare
0d87f13
to
83030dc
Compare
/gcbrun |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a high level, I'm concerned with the type safety of this approach. Given we have such concrete types defined by MCP, we should be re-implementing those for each version and ensuring requests and responses match them correctly. As is, this method seems difficult to scale as MCP evolves over time.
An example of what this should look like is probably more then like protocol.py is defined here.
) | ||
|
||
|
||
class McpHttpTransport(ITransport): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't there be a different transport for each supported MCP version?
In this PR, the MCP protocol does not yet support Auth related functionality such as
Authorized Invocations
andAuthenticated Parameters
.Therefore, the default is still the Toolbox Protocol here.
Note: This PR is currently based on two separate PRs. The protocol is added in #348. This will be updated once the protocol object PR is merged.