From 175244ab89880f1bf6ccd26d837d88c638cf45c1 Mon Sep 17 00:00:00 2001 From: marfrit Date: Tue, 11 Aug 2026 14:21:41 +0200 Subject: [PATCH] =?UTF-8?q?lmcp:=20review=20fixes=201,=204=20and=206=20?= =?UTF-8?q?=E2=80=94=20legacy=20routing=20that=20survives=20the=20transpor?= =?UTF-8?q?t?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From @deus, bundle sha256 f38898d3…, verified byte-identical on arrival; lmcp.lua sha256 57fb2413…, the checksum quoted in the handover. Only lmcp.lua changes; versions.lua and envelope.lua are byte-identical to what is already on this branch. luac -p passes on all three. What the three fixes do, checked in the file rather than taken on trust — the conformance suites are HTTP-only and structurally cannot see finding 1: #1 handle_request now DERIVES _legacy when it arrives unset, instead of relying on the HTTP transport to have pre-set it. Previously req._legacy was written in exactly one place (_dispatch_post) and read in three, and run_stdio calls handle_request directly — so on stdio initialize, ping and logging/setLevel fell through to -32601 Method not found. That is the first message any client sends, and it worked unconditionally on master. #6 An empty protocol version now counts as "none announced" rather than as an unknown one. Router and versions.check had disagreed: check("") returns true by rule a, while the router excluded "" from the legacy set and sent it down the new path to be rejected by the envelope. #4 Legacy replies go through jsonrpc_result_legacy, which does NOT inject resultType. The claim that these methods "keep the old shape" was not true on the wire before: legacy ping answered {"resultType":"complete"} where master answered {}. Still open from the same review, deliberately not in this commit: #2 (the server-initiated SSE machinery is unreachable — nothing sets sess.sse_conn since the GET branch was removed), #3 (GET/DELETE answer 405 to legacy clients too, because _legacy is consulted at method dispatch and not at HTTP verb routing), #5 (routing accepts a header-only or _meta-only announcement, envelope.check then demands both), #7 (-32021 mapped but never produced, -32600 unmapped). Still NOT on master. --- lmcp.lua | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/lmcp.lua b/lmcp.lua index 5c52653..bb3a18c 100644 --- a/lmcp.lua +++ b/lmcp.lua @@ -487,6 +487,15 @@ local function jsonrpc_result(id, result) return json.encode({ jsonrpc = JSONRPC, id = id, result = copy }) end +-- Legacy (2025-06-18) result encoder: the old server's jsonrpc_result did +-- NOT inject resultType -- that field belongs to the 2026-07-28 result +-- envelope (review finding #4: the legacy branches must keep the old wire +-- shape; a strict old client that validates ping's empty result would +-- break on a foreign resultType). +local function jsonrpc_result_legacy(id, result) + return json.encode({ jsonrpc = JSONRPC, id = id, result = result }) +end + local function jsonrpc_error(id, code, message, data) -- `data` is optional and only set when present: all existing callers -- pass three arguments and do not change. @@ -566,6 +575,19 @@ function lmcp:handle_request(req) return nil end + -- Dual-protocol routing, transport-agnostic (review fix #1): the HTTP + -- path pre-sets _announced/_legacy in _dispatch_post (it can see the + -- MCP-Protocol-Version header); stdio reaches handle_request directly + -- with NO headers, so derive here from _meta only. Empty string counts + -- as "no version announced" (versions.check rule a, review fix #6). + if req._legacy == nil then + local meta_v = ((req.params or {})._meta or {}) + ["io.modelcontextprotocol/protocolVersion"] + if meta_v == "" then meta_v = nil end + req._announced = meta_v + req._legacy = (meta_v == nil or meta_v == "2025-06-18") + end + -- Legacy surface (2025-06-18), reachable only when the router announced -- the old version or none at all (E2: no version = old). SEP-2575 removed -- these from 2026-07-28; on the NEW path they fall through to the default @@ -577,7 +599,7 @@ function lmcp:handle_request(req) local p = req.params or {} self._client_caps = p.capabilities or {} self._client_info = p.clientInfo or {} - return jsonrpc_result(id, { + return jsonrpc_result_legacy(id, { protocolVersion = MCP_VERSION, capabilities = self:_capabilities(), serverInfo = { @@ -586,7 +608,7 @@ function lmcp:handle_request(req) }, }) elseif req._legacy and method == "ping" then - return jsonrpc_result(id, json.empty_object) + return jsonrpc_result_legacy(id, json.empty_object) elseif req._legacy and method == "logging/setLevel" then local lvl = (req.params or {}).level if type(lvl) ~= "string" or not LOG_LEVELS[lvl] then @@ -594,7 +616,7 @@ function lmcp:handle_request(req) "level must be one of: debug, info, notice, warning, error, critical, alert, emergency") end self._log_level = lvl - return jsonrpc_result(id, json.empty_object) + return jsonrpc_result_legacy(id, json.empty_object) end -- MCP 2026-07-28, /server/discover: "Servers MUST implement it." @@ -1170,9 +1192,13 @@ local function _dispatch_post(self, conn) -- MCP-Protocol-Version header OR _meta.protocolVersion (E2: either source -- is authoritative). Both present and differing -> -32020, checked BEFORE -- routing so a mixed announcement never reaches either protocol path. + -- An empty string counts as "no version" (versions.check rule a; review + -- fix #6) — it is treated as absent, exactly like a missing channel. local header_v = conn.headers['mcp-protocol-version'] + if header_v == "" then header_v = nil end local meta_v = ((rpc_req.params or {})._meta or {}) ["io.modelcontextprotocol/protocolVersion"] + if meta_v == "" then meta_v = nil end if header_v and meta_v and header_v ~= meta_v then return _build_http_response( _error_status[-32020] or "400 Bad Request",