-
Notifications
You must be signed in to change notification settings - Fork 158
Add Server Streamable Http Transport #235
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: main
Are you sure you want to change the base?
Conversation
Hi @e5l @devcrocod @SeanChinJunKai , would you mind taking a look? |
Hi @devcrocod , would you mind taking a look when available? |
6820649
to
465f3dd
Compare
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.
Thank you @zshea for the PR
Would it be possible to add integration tests to make sure it actually works?
Also, id
is mandatory for JSONRPCRequest
, JSONRPCResponse
, so let's keep it non-nullable
@Serializable | ||
public class JSONRPCResponse( | ||
public val id: RequestId, | ||
public val id: RequestId?, |
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.
public val id: RequestId?, | |
public val id: RequestId? = null, |
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.
According to the spec, the id
is required field
} | ||
} | ||
|
||
@KtorDsl |
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.
Missing KDoc
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.
I tried your PR in practice in Stateless mode and brought back what I found.
this.response.status(status) | ||
this.respond( | ||
JSONRPCResponse( | ||
id = null, |
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.
Yom can use RequestId.StringId("server-error")
as in the Python SDK
|
||
val hasRequest = messages.any { it is JSONRPCRequest } | ||
if (!hasRequest) { | ||
call.respondNullable(status = HttpStatusCode.Accepted, message = null) |
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.
Perhaps because of the Accept
request header, this line returns 406 Not Acceptable
instead of 202
. MCP Inspector throws an error in log, and the Python client crashes because of this. For me, call.respondBytes(status = HttpStatusCode.Accepted, bytes = ByteArray(0))
helped.
block: RoutingContext.() -> Server, | ||
) { | ||
routing { | ||
post("/mcp") { |
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.
get
requests also need to be processed.
Hi @zshea Thanks for this PR. In relation to:
Was an issue created on ktor for this? |
One thing that's not clear to me
Looking at the spec the client MUST be able to accept a json response. Is this a bug in the client? From the spec:
|
I played around with this a bit.
|
Closes #79 #183
Implement server StreamableHttpTransport (both stateful and stateless)
This is a workable solution, tested with Claude Code
Caveat:
enableJsonResponse = true
. We can remove this limitation once the upstream has fixed this issue.StreamHttpTransport
assumes the SSE stream while the server assumes the JSON response.Motivation and Context
It is painful to support
/sse
+/message
endpoint on production. Hopefully we can close this issue with this PR.How Has This Been Tested?
It has been tested in my environment with Claude Code (1.0.89) MCP with both
mcpStreamableHttp
andmcpStatelessStreamableHttp
Breaking Changes
No, everything is backward compatible
Types of changes
Checklist
Additional context
It is workable with the following implementation:
But the response is always 200, even if we set the status explicitly. And the connection won't be closed with a 4xx or 5xx response. It breaks the MCP spec and can't be used directly. I think we need to create issues to upstream.