🐛 fix(core): return JSON-RPC protocol errors

lsp-server/src/core/dispatcher.cppm

@@ -233,13 +233,22 @@ namespace lsp::core
                                           protocol::ErrorCodes code,
                                           std::string_view message)
     {
-        protocol::ResponseMessage response;
+        protocol::LSPAny response_id(std::nullptr_t{});
-        response.id = std::move(id);
+        if (id)
-        response.error = protocol::ResponseError{
+        {
-            .jsonrpc = "2.0",
+            response_id = std::visit(
-            .code = static_cast<protocol::integer>(code),
+                [](const auto& value) -> protocol::LSPAny { return value; }, *id);
-            .message = std::string(message),
+        }
-            .data = std::nullopt,
+
+        protocol::LSPObject error{
+            { "code", static_cast<protocol::integer>(code) },
+            { "message", protocol::string(message) },
+        };
+
+        protocol::LSPObject response{
+            { "jsonrpc", protocol::string("2.0") },
+            { "id", std::move(response_id) },
+            { "error", std::move(error) },
         };
         return transform::Serialize(response).value();
     }

lsp-server/src/core/server.cppm

@@ -65,6 +65,9 @@ export namespace lsp::core
         void SendError(const protocol::RequestMessage& request,
                        protocol::ErrorCodes code,
                        std::string_view message);
+        void SendError(std::optional<protocol::RequestId> id,
+                       protocol::ErrorCodes code,
+                       std::string_view message);
 
         std::istream& input_;
         std::ostream& output_;
@@ -82,6 +85,32 @@ export namespace lsp::core
 
 namespace lsp::core
 {
+    namespace
+    {
+        std::optional<protocol::RequestId> ExtractRequestId(
+            const protocol::LSPObject& object)
+        {
+            const auto id = object.find("id");
+            if (id == object.end())
+                return std::nullopt;
+            if (id->second.Is<protocol::integer>())
+                return protocol::RequestId{ id->second.Get<protocol::integer>() };
+            if (id->second.Is<protocol::uinteger>())
+            {
+                const auto value = id->second.Get<protocol::uinteger>();
+                if (value <= static_cast<protocol::uinteger>(
+                                 std::numeric_limits<protocol::integer>::max()))
+                {
+                    return protocol::RequestId{ static_cast<protocol::integer>(value) };
+                }
+                return std::nullopt;
+            }
+            if (id->second.Is<protocol::string>())
+                return protocol::RequestId{ id->second.Get<protocol::string>() };
+            return std::nullopt;
+        }
+    }
+
     LspServer::LspServer(std::istream& input,
                          std::ostream& output,
                          ProviderRegistrar registrar,
@@ -190,13 +219,30 @@ namespace lsp::core
     void LspServer::HandleMessage(const std::string& raw_message)
     {
         auto any = transform::Deserialize<protocol::LSPAny>(raw_message);
-        if (!any || !any->Is<protocol::LSPObject>())
+        if (!any)
         {
-            spdlog::warn("Failed to parse message: {}", raw_message);
+            SendError(std::nullopt, protocol::ErrorCodes::ParseError, "Parse error");
+            return;
+        }
+        if (!any->Is<protocol::LSPObject>())
+        {
+            SendError(std::nullopt,
+                      protocol::ErrorCodes::InvalidRequest,
+                      "Invalid Request");
             return;
         }
 
         const auto& object = any->Get<protocol::LSPObject>();
+        const auto id = ExtractRequestId(object);
+        const auto jsonrpc = object.find("jsonrpc");
+        if (jsonrpc == object.end() ||
+            !jsonrpc->second.Is<protocol::string>() ||
+            jsonrpc->second.Get<protocol::string>() != "2.0")
+        {
+            SendError(id, protocol::ErrorCodes::InvalidRequest, "Invalid Request");
+            return;
+        }
+
         const bool has_id = object.contains("id");
         const bool has_method = object.contains("method");
         const bool has_result = object.contains("result");
@@ -204,15 +250,33 @@ namespace lsp::core
 
         if (has_method && has_id)
         {
+            const auto method = object.find("method");
+            if (!id || !method->second.Is<protocol::string>())
+            {
+                SendError(id, protocol::ErrorCodes::InvalidRequest, "Invalid Request");
+                return;
+            }
             if (auto request = transform::Deserialize<protocol::RequestMessage>(raw_message))
+            {
                 HandleRequest(*request);
+            }
             else
-                spdlog::warn("Failed to parse request message");
+            {
+                SendError(id, protocol::ErrorCodes::InvalidRequest, "Invalid Request");
+            }
             return;
         }
 
         if (has_method)
         {
+            const auto method = object.find("method");
+            if (!method->second.Is<protocol::string>())
+            {
+                SendError(std::nullopt,
+                          protocol::ErrorCodes::InvalidRequest,
+                          "Invalid Request");
+                return;
+            }
             if (auto notification =
                     transform::Deserialize<protocol::NotificationMessage>(raw_message))
             {
@@ -220,12 +284,14 @@ namespace lsp::core
             }
             else
             {
-                spdlog::warn("Failed to parse notification message");
+                SendError(std::nullopt,
+                          protocol::ErrorCodes::InvalidRequest,
+                          "Invalid Request");
             }
             return;
         }
 
-        if (has_id && (has_result || has_error))
+        if (has_id && (has_result != has_error))
         {
             if (auto response = transform::Deserialize<protocol::ResponseMessage>(raw_message))
                 HandleResponse(*response);
@@ -234,7 +300,13 @@ namespace lsp::core
             return;
         }
 
-        spdlog::warn("Unrecognized message: {}", raw_message);
+        if (has_result || has_error)
+        {
+            spdlog::warn("Ignoring invalid response-like message");
+            return;
+        }
+
+        SendError(id, protocol::ErrorCodes::InvalidRequest, "Invalid Request");
     }
 
     void LspServer::SendMessage(const std::string& message)
@@ -484,6 +556,13 @@ namespace lsp::core
                               protocol::ErrorCodes code,
                               std::string_view message)
     {
-        SendMessage(BuildErrorResponseMessage(request, code, message));
+        SendError(request.id, code, message);
+    }
+
+    void LspServer::SendError(std::optional<protocol::RequestId> id,
+                              protocol::ErrorCodes code,
+                              std::string_view message)
+    {
+        SendMessage(BuildErrorResponseMessage(std::move(id), code, message));
     }
 }

lsp-server/src/protocol/common/message.cppm

@@ -39,7 +39,6 @@ export namespace lsp::protocol
 
     struct ResponseError
     {
-        string jsonrpc = "2.0";
         integer code;
         string message;
         std::optional<LSPAny> data;

lsp-server/src/protocol/protocol.cppm

@@ -50,7 +50,7 @@ namespace glz
     struct meta<lsp::protocol::ResponseError>
     {
         using T = lsp::protocol::ResponseError;
-        static constexpr auto value = glz::object(&T::jsonrpc, &T::code, &T::message, &T::data);
+        static constexpr auto value = glz::object(&T::code, &T::message, &T::data);
     };
 
     template<>

lsp-server/test/test_core_server.py

@@ -14,6 +14,10 @@ def frame(message: dict) -> bytes:
     return f"Content-Length: {len(body)}\r\n\r\n".encode("ascii") + body
 
 
+def raw_frame(body: bytes) -> bytes:
+    return f"Content-Length: {len(body)}\r\n\r\n".encode("ascii") + body
+
+
 def read_messages(data: bytes) -> list[dict]:
     messages = []
     offset = 0
@@ -169,12 +173,48 @@ def assert_lifecycle(server: Path) -> None:
         client.kill()
 
 
+def assert_json_rpc_errors(server: Path) -> None:
+    payload = b"".join([
+        raw_frame(b'{"jsonrpc":"2.0",'),
+        raw_frame(b"[]"),
+        raw_frame(b'{"jsonrpc":"1.0","id":7,"method":"initialize"}'),
+        frame({
+            "jsonrpc": "2.0",
+            "id": 8,
+            "method": "initialize",
+            "params": {},
+        }),
+        frame({"jsonrpc": "2.0", "id": 9, "method": "missing/method"}),
+        frame({"jsonrpc": "2.0", "id": 10, "method": "shutdown"}),
+        frame({"jsonrpc": "2.0", "method": "exit"}),
+    ])
+    result = run_raw(server, payload)
+    if result.returncode != 0:
+        raise RuntimeError("JSON-RPC error sequence should shut down cleanly")
+
+    responses = read_messages(result.stdout)
+    if responses[0].get("id", "missing") is not None:
+        raise RuntimeError(
+            f"ParseError response should use a null id: {responses[0]!r}")
+    if responses[0].get("error", {}).get("code") != -32700:
+        raise RuntimeError("Malformed JSON should return ParseError")
+    if responses[1].get("id", "missing") is not None:
+        raise RuntimeError("InvalidRequest response should use a null id")
+    if responses[1].get("error", {}).get("code") != -32600:
+        raise RuntimeError("A non-object JSON value should return InvalidRequest")
+    if response_by_id(result.stdout, 7).get("error", {}).get("code") != -32600:
+        raise RuntimeError("jsonrpc other than 2.0 should return InvalidRequest")
+    if response_by_id(result.stdout, 9).get("error", {}).get("code") != -32601:
+        raise RuntimeError("Unknown request method should return MethodNotFound")
+
+
 def main() -> int:
     parser = argparse.ArgumentParser()
     parser.add_argument("--server", type=Path, required=True)
     args = parser.parse_args()
 
     assert_lifecycle(args.server)
+    assert_json_rpc_errors(args.server)
     return 0