Merge pull request #281 from koic/client_raise_on_error_response

Fix client methods silently swallowing JSON-RPC error responses

Author: koic

lib/mcp/client.rb

@@ -6,6 +6,27 @@
 
 module MCP
   class Client
+    class ServerError < StandardError
+      attr_reader :code, :data
+
+      def initialize(message, code:, data: nil)
+        super(message)
+        @code = code
+        @data = data
+      end
+    end
+
+    class RequestHandlerError < StandardError
+      attr_reader :error_type, :original_error, :request
+
+      def initialize(message, request, error_type: :internal_error, original_error: nil)
+        super(message)
+        @request = request
+        @error_type = error_type
+        @original_error = original_error
+      end
+    end
+
     # Initializes a new MCP::Client instance.
     #
     # @param transport [Object] The transport object to use for communication with the server.
@@ -33,11 +54,7 @@ def initialize(transport:)
     #     puts tool.name
     #   end
     def tools
-      response = transport.send_request(request: {
-        jsonrpc: JsonRpcHandler::Version::V2_0,
-        id: request_id,
-        method: "tools/list",
-      })
+      response = request(method: "tools/list")
 
       response.dig("result", "tools")&.map do |tool|
         Tool.new(
@@ -53,11 +70,7 @@ def tools
     #
     # @return [Array<Hash>] An array of available resources.
     def resources
-      response = transport.send_request(request: {
-        jsonrpc: JsonRpcHandler::Version::V2_0,
-        id: request_id,
-        method: "resources/list",
-      })
+      response = request(method: "resources/list")
 
       response.dig("result", "resources") || []
     end
@@ -67,11 +80,7 @@ def resources
     #
     # @return [Array<Hash>] An array of available resource templates.
     def resource_templates
-      response = transport.send_request(request: {
-        jsonrpc: JsonRpcHandler::Version::V2_0,
-        id: request_id,
-        method: "resources/templates/list",
-      })
+      response = request(method: "resources/templates/list")
 
       response.dig("result", "resourceTemplates") || []
     end
@@ -81,11 +90,7 @@ def resource_templates
     #
     # @return [Array<Hash>] An array of available prompts.
     def prompts
-      response = transport.send_request(request: {
-        jsonrpc: JsonRpcHandler::Version::V2_0,
-        id: request_id,
-        method: "prompts/list",
-      })
+      response = request(method: "prompts/list")
 
       response.dig("result", "prompts") || []
     end
@@ -119,25 +124,15 @@ def call_tool(name: nil, tool: nil, arguments: nil, progress_token: nil)
         params[:_meta] = { progressToken: progress_token }
       end
 
-      transport.send_request(request: {
-        jsonrpc: JsonRpcHandler::Version::V2_0,
-        id: request_id,
-        method: "tools/call",
-        params: params,
-      })
+      request(method: "tools/call", params: params)
     end
 
     # Reads a resource from the server by URI and returns the contents.
     #
     # @param uri [String] The URI of the resource to read.
     # @return [Array<Hash>] An array of resource contents (text or blob).
     def read_resource(uri:)
-      response = transport.send_request(request: {
-        jsonrpc: JsonRpcHandler::Version::V2_0,
-        id: request_id,
-        method: "resources/read",
-        params: { uri: uri },
-      })
+      response = request(method: "resources/read", params: { uri: uri })
 
       response.dig("result", "contents") || []
     end
@@ -147,31 +142,34 @@ def read_resource(uri:)
     # @param name [String] The name of the prompt to get.
     # @return [Hash] A hash containing the prompt details.
     def get_prompt(name:)
-      response = transport.send_request(request: {
-        jsonrpc: JsonRpcHandler::Version::V2_0,
-        id: request_id,
-        method: "prompts/get",
-        params: { name: name },
-      })
+      response = request(method: "prompts/get", params: { name: name })
 
       response.fetch("result", {})
     end
 
     private
 
-    def request_id
-      SecureRandom.uuid
-    end
+    def request(method:, params: nil)
+      request_body = {
+        jsonrpc: JsonRpcHandler::Version::V2_0,
+        id: request_id,
+        method: method,
+      }
+      request_body[:params] = params if params
 
-    class RequestHandlerError < StandardError
-      attr_reader :error_type, :original_error, :request
+      response = transport.send_request(request: request_body)
 
-      def initialize(message, request, error_type: :internal_error, original_error: nil)
-        super(message)
-        @request = request
-        @error_type = error_type
-        @original_error = original_error
+      # Guard with `is_a?(Hash)` because custom transports may return non-Hash values.
+      if response.is_a?(Hash) && response.key?("error")
+        error = response["error"]
+        raise ServerError.new(error["message"], code: error["code"], data: error["data"])
       end
+
+      response
+    end
+
+    def request_id
+      SecureRandom.uuid
     end
   end
 end

test/mcp/client_test.rb

@@ -369,5 +369,92 @@ def test_call_tool_omits_meta_when_no_progress_token
       client = Client.new(transport: transport)
       client.call_tool(tool: tool, arguments: arguments)
     end
+
+    def test_tools_raises_server_error_on_error_response
+      transport = mock
+      mock_response = { "error" => { "code" => -32_601, "message" => "Method not found" } }
+
+      transport.expects(:send_request).returns(mock_response).once
+
+      client = Client.new(transport: transport)
+      error = assert_raises(Client::ServerError) { client.tools }
+      assert_equal(-32_601, error.code)
+      assert_equal("Method not found", error.message)
+    end
+
+    def test_resources_raises_server_error_on_error_response
+      transport = mock
+      mock_response = { "error" => { "code" => -32_602, "message" => "Invalid params" } }
+
+      transport.expects(:send_request).returns(mock_response).once
+
+      client = Client.new(transport: transport)
+      error = assert_raises(Client::ServerError) { client.resources }
+      assert_equal(-32_602, error.code)
+    end
+
+    def test_read_resource_raises_server_error_on_error_response
+      transport = mock
+      mock_response = { "error" => { "code" => -32_602, "message" => "Resource not found" } }
+
+      transport.expects(:send_request).returns(mock_response).once
+
+      client = Client.new(transport: transport)
+      assert_raises(Client::ServerError) { client.read_resource(uri: "file:///missing") }
+    end
+
+    def test_get_prompt_raises_server_error_on_error_response
+      transport = mock
+      mock_response = { "error" => { "code" => -32_602, "message" => "Prompt not found" } }
+
+      transport.expects(:send_request).returns(mock_response).once
+
+      client = Client.new(transport: transport)
+      assert_raises(Client::ServerError) { client.get_prompt(name: "missing") }
+    end
+
+    def test_prompts_raises_server_error_on_error_response
+      transport = mock
+      mock_response = { "error" => { "code" => -32_601, "message" => "Method not found" } }
+
+      transport.expects(:send_request).returns(mock_response).once
+
+      client = Client.new(transport: transport)
+      assert_raises(Client::ServerError) { client.prompts }
+    end
+
+    def test_resource_templates_raises_server_error_on_error_response
+      transport = mock
+      mock_response = { "error" => { "code" => -32_601, "message" => "Method not found" } }
+
+      transport.expects(:send_request).returns(mock_response).once
+
+      client = Client.new(transport: transport)
+      assert_raises(Client::ServerError) { client.resource_templates }
+    end
+
+    def test_call_tool_raises_server_error_on_error_response
+      transport = mock
+      mock_response = { "error" => { "code" => -32_602, "message" => "Tool not found" } }
+
+      transport.expects(:send_request).returns(mock_response).once
+
+      client = Client.new(transport: transport)
+      error = assert_raises(Client::ServerError) { client.call_tool(name: "missing") }
+      assert_equal(-32_602, error.code)
+    end
+
+    def test_server_error_includes_data_field
+      transport = mock
+      mock_response = {
+        "error" => { "code" => -32_603, "message" => "Internal error", "data" => "extra details" },
+      }
+
+      transport.expects(:send_request).returns(mock_response).once
+
+      client = Client.new(transport: transport)
+      error = assert_raises(Client::ServerError) { client.tools }
+      assert_equal("extra details", error.data)
+    end
   end
 end