Skip to content

Conversation

dak2
Copy link
Contributor

@dak2 dak2 commented Aug 24, 2025

Motivation and Context

If the server accepts JSON-RPC notifications and responses, it must return an HTTP status code of 202 without a body.

refs

How Has This Been Tested?

  • Check existing and added test cases for this case.
  • Confirm the server returns 202 when the client sends a notification or response.

Breaking Changes

There is a possibility that, if users depend on status code 200.

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

Additional context

Follow up: #111

@atesgoral
Copy link
Contributor

atesgoral commented Aug 29, 2025

On the tests: I don't see anywhere in either the MCP or the JSON-RPC spec where a result or error property is used in a request with an id. These are response properties. Requests have method and params.

@dak2 dak2 force-pushed the handle-202 branch 2 times, most recently from 07ee3d6 to ae627e9 Compare September 1, 2025 22:25
@dak2
Copy link
Contributor Author

dak2 commented Sep 1, 2025

@atesgoral
Thanks for your review.

As you pointed out, it seems I misunderstood. I've deleted the test case I added.

Also, I believe this case is already covered by an existing test in test/mcp/server/transports/streamable_http_transport_test.rb (the case where a server returns 202 when a JSON-RPC notification request object is passed, and 200 when a JSON-RPC regular request object is passed), so I haven't added any specific tests.

Could you please review it again?

@koic
Copy link
Member

koic commented Sep 1, 2025

It seems odd that there is no reproduction test. Without a reproduction case for this issue, wouldn't it be difficult to notice if a regression occurs?

@dak2
Copy link
Contributor Author

dak2 commented Sep 1, 2025

@koic

Thanks for your checking!

I believe the following three points need to be verified for this requirement:

  1. When receiving a JSON-RPC notification object, return a 202 status code without a response body.
  2. When receiving a JSON-RPC response object, return a 202 status code without a response body.
  3. Do not return a 202 when receiving a regular JSON-RPC request object.

Regarding the second point, as atesgoral pointed out, since it's a response object, it doesn't get passed as a request in the first place, so I don't believe it needs to be verified in testing.

For case 1, see lines

test "POST notifications/initialized returns 202 with no body" do
# Create a session first (optional for notification, but keep consistent with flow)
init_request = create_rack_request(
"POST",
"/",
{ "CONTENT_TYPE" => "application/json" },
{ jsonrpc: "2.0", method: "initialize", id: "init" }.to_json,
)
init_response = @transport.handle_request(init_request)
session_id = init_response[1]["Mcp-Session-Id"]
notif_request = create_rack_request(
"POST",
"/",
{
"CONTENT_TYPE" => "application/json",
"HTTP_MCP_SESSION_ID" => session_id,
},
{ jsonrpc: "2.0", method: MCP::Methods::NOTIFICATIONS_INITIALIZED }.to_json,
)
response = @transport.handle_request(notif_request)
assert_equal 202, response[0]
assert_empty(response[1])
assert_empty(response[2])
end
, and case 3 is
test "handles POST request with valid JSON-RPC message" do
# First create a session
init_request = create_rack_request(
"POST",
"/",
{ "CONTENT_TYPE" => "application/json" },
{ jsonrpc: "2.0", method: "initialize", id: "init" }.to_json,
)
init_response = @transport.handle_request(init_request)
session_id = init_response[1]["Mcp-Session-Id"]
# Now make the ping request with the session ID
request = create_rack_request(
"POST",
"/",
{
"CONTENT_TYPE" => "application/json",
"HTTP_MCP_SESSION_ID" => session_id,
},
{ jsonrpc: "2.0", method: "ping", id: "123" }.to_json,
)
response = @transport.handle_request(request)
assert_equal 200, response[0]
assert_equal({ "Content-Type" => "application/json" }, response[1])
body = JSON.parse(response[2][0])
assert_equal "2.0", body["jsonrpc"]
assert_equal "123", body["id"]
assert_empty(body["result"])
end

I believe these cases is already covered by above test cases, so I don't think we need to write additional tests.
What do you think about this?

@topherbullock
Copy link
Contributor

topherbullock commented Sep 19, 2025

  1. If the input consists solely of (any number of) JSON-RPC responses or notifications:
  • If the server accepts the input, the server MUST return HTTP status code 202 Accepted with no body.

https://modelcontextprotocol.io/specification/2025-03-26/basic/transports#sending-messages-to-the-server

this doesn't exactly implement the spec, since only Notification in JSON-RPC is "a Request object without an "id" member". the "Responses" type includes an id field but does not have a method

instead of checking !(body["id"] && body["method"]) , I'd prefer specific response?(body) notification?(body) checks to make the logic more clear that 202 is expected if input is only notification/response

@dak2 dak2 force-pushed the handle-202 branch 3 times, most recently from af739dc to c4e717a Compare September 21, 2025 13:50
@dak2
Copy link
Contributor Author

dak2 commented Sep 21, 2025

@topherbullock

Thanks for the comment!!
I agree with you. It appears that my implementation logic was incorrect.
I separated the logic for the response and the notification.
I'd appreciate it if you could review it.

@dak2
Copy link
Contributor Author

dak2 commented Sep 21, 2025

@koic

FYI: Sorry, I misunderstood. I've added a new test case for when the JSON-RPC request object is passed as input.

end

def response?(body)
body["jsonrpc"] == "2.0" && body["id"] && !body["method"]
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't the condition body["jsonrpc"] == "2.0" be a validation that applies more broadly, rather than only to notification? and response??

Copy link
Member

Choose a reason for hiding this comment

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

Hm, doesn't this duplicate the validation in json_rpc_handler?
https://github.com/Shopify/json-rpc-handler/blob/v0.1.1/lib/json_rpc_handler.rb#L68

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@koic

Thank you for checking it!

As you pointed out, the jsonrpc validation is already handled by the json_rpc_handler, so it's unnecessary. I overlooked that.

Deleted.
https://github.com/modelcontextprotocol/ruby-sdk/compare/9631b57dd374c74ed67809ac9e962d1fa84c6c62..2aee943b1067f3105498ae3ab0e51b83f7bc1eb6

end

def notification?(body)
!body["id"] && body["method"]
Copy link
Member

Choose a reason for hiding this comment

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

Nits, the values of body["method"] doesn't return to be a boolean. Since these is a predicate method, it would be better to return a boolean using something like !!.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the change you suggested because I thought it was a more accurate approach.
https://github.com/modelcontextprotocol/ruby-sdk/compare/2aee943b1067f3105498ae3ab0e51b83f7bc1eb6..cd7a54a5c9110035a530dddb1607329ddf4e78a9

Thank you for your review!

…fications and responses

If the server accepts JSON-RPC notifications and responses, it must return an HTTP status code of 202 without a body.

refs
- https://modelcontextprotocol.io/specification/2025-03-26/basic/transports#sending-messages-to-the-server
- https://www.jsonrpc.org/specification

Follow up: modelcontextprotocol#111
@koic koic merged commit 99b57a3 into modelcontextprotocol:main Sep 27, 2025
5 checks passed
@koic
Copy link
Member

koic commented Sep 27, 2025

Thanks!

@dak2 dak2 deleted the handle-202 branch September 28, 2025 02:18
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