Support server returning only JSON on requests #299
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
beaulac
reviewed
Apr 9, 2025
9 tasks
* always send headers specified in requestInit option * avoid doubled onerror call * use for-await to iterate SSE stream * remove outdated comments * simplify requestId tracking * throw error when response Content-Type is out of spec
StreamableHTTPClientTransport cleanup / fixes
jerome3o-anthropic
requested changes
Apr 10, 2025
src/examples/README.md
Outdated
|
|
||
| ```bash | ||
| # Initialize the server and get the session ID from headers | ||
| SESSION_ID=$(curl -X POST -H "Content-Type: application/json" -H "Accept: application/json" \ |
There was a problem hiding this comment.
This doesn't work for me - I get:
❯ curl -X POST -H "Content-Type: application/json" -H "Accept: application/json" \
-d '{"jsonrpc":"2.0","method":"initialize","params":{"capabilities":{}},"id":"1"}' \
-i http://localhost:3000/mcp 2>&1
HTTP/1.1 406 Not Acceptable
X-Powered-By: Express
Date: Thu, 10 Apr 2025 10:50:08 GMT
Connection: keep-alive
Keep-Alive: timeout=5
Transfer-Encoding: chunked
{"jsonrpc":"2.0","error":{"code":-32000,"message":"Not Acceptable: Client must accept both application/json and text/event-stream"},"id":null}%
There was a problem hiding this comment.
This worked with the current implementation:
SESSION_ID=$(curl -X POST \
-H "Content-Type: application/json" \
-H "Accept: application/json" \
-H "Accept: text/event-stream" \
-d '{
"jsonrpc": "2.0",
"method": "initialize",
"params": {
"capabilities": {},
"protocolVersion": "2025-03-26",
"clientInfo": {
"name": "test",
"version": "1.0.0"
}
},
"id": "1"
}' \
-i http://localhost:3000/mcp 2>&1 | grep -i "mcp-session-id" | cut -d' ' -f2 | tr -d '\r')
echo "Session ID: $SESSION_ID"
src/examples/README.md
Outdated
Comment on lines
29
to
32
| curl -X POST -H "Content-Type: application/json" -H "Accept: application/json" \ | ||
| -H "mcp-session-id: $SESSION_ID" \ | ||
| -d '{"jsonrpc":"2.0","method":"mcp.call_tool","params":{"name":"greet","arguments":{"name":"World"}},"id":"2"}' \ | ||
| http://localhost:3000/mcp |
There was a problem hiding this comment.
mcp.call_tool isn't correct, also I needed to have the Accept: text/event-stream header here too
curl -X POST \
-H "Content-Type: application/json" \
-H "Accept: application/json" \
-H "Accept: text/event-stream" \
-H "mcp-session-id: $SESSION_ID" \
-d '{
"jsonrpc": "2.0",
"method": "tools/call",
"params": {
"name": "greet",
"arguments": {
"name": "World"
}
},
"id": "2"
}' \
http://localhost:3000/mcp
There was a problem hiding this comment.
remove before merging
There was a problem hiding this comment.
ugh, how this got it here!
| app.listen(PORT, () => { | ||
| console.log(`MCP Streamable HTTP Server with JSON responses listening on port ${PORT}`); | ||
| console.log(`Server is running. Press Ctrl+C to stop.`); | ||
| console.log(`Initialize with: curl -X POST -H "Content-Type: application/json" -H "Accept: application/json" -d '{"jsonrpc":"2.0","method":"initialize","params":{"capabilities":{}},"id":"1"}' http://localhost:${PORT}/mcp`); |
There was a problem hiding this comment.
looks like this may need the same treatment mentioned prior
src/server/streamableHttp.test.ts
Outdated
| await jsonResponseTransport.handleRequest(req, mockResponse); | ||
|
|
||
| // Wait for all promises to resolve | ||
| await new Promise(resolve => setTimeout(resolve, 50)); |
There was a problem hiding this comment.
[non-blocking nit] is there a better way to do this?
There was a problem hiding this comment.
seems like it would just slow down tests
src/server/streamableHttp.test.ts
Outdated
| it("should return JSON response for a single request", async () => { | ||
| const requestMessage: JSONRPCMessage = { | ||
| jsonrpc: "2.0", | ||
| method: "test", |
There was a problem hiding this comment.
[non-blocking question] should this use real MCP messages?
There was a problem hiding this comment.
yes, actually was thinking to use MCP server and rewrite tests, mocking request doesn't seem robust
src/server/streamableHttp.test.ts
Outdated
| await jsonResponseTransport.handleRequest(req, mockResponse); | ||
|
|
||
| // Wait for all promises to resolve - give it enough time | ||
| await new Promise(resolve => setTimeout(resolve, 100)); |
There was a problem hiding this comment.
we actually don't even need it, was trying to make the test work, but the issues was in completely different place
jerome3o-anthropic
approved these changes
Apr 10, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Spec states that servers must support either streaming or json response. Clients MUST support both.
We anticipate that TypeScript sdk will mostly be used with streaming, but for testing purpose adding support for returning JSON.
Returning JSON is more limited in functionality as we are loosing streaming tool notification/logging etc.
Example of a response calling to server that supports streaming:
Example of a response calling to server that support only JSON (loosing logging):
