diff --git a/CLAUDE.md b/CLAUDE.md index ed00fdb..4b09cd6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -91,7 +91,9 @@ not checked), and the responses in MCP's JSON. `installMcp` in `Application.kt` then be a duplicate. What stays here is `KatcherMcpServer.register` (the five tools, `link_fix` the only one that writes), `CrashTrust`'s domain rules and `CrashAssessment`; only the hidden-character set comes from kore (`HiddenCharacters`). `McpEndpointTest` drives the whole module through Ktor's -test engine. +test engine; `McpWireFormatTest` holds the answers to MCP's JSON on the raw body — katcher's own +`Json` would drop `protocolVersion` and `ping`'s `result` — with a control on the bare SDK that must +come out different. ## Client crash-capture model (important, non-obvious) diff --git a/README.md b/README.md index 5778a57..187edb8 100644 --- a/README.md +++ b/README.md @@ -141,6 +141,22 @@ helm upgrade --install katcher ./charts/katcher --set mcp.token="$MCP_TOKEN" The chart wires it through a `Secret` and fills `MCP_ALLOWED_HOSTS` from `hostname`. +### A warning in the startup log + +With `MCP_TOKEN` set, the log at start carries a `WARN` line from the MCP SDK: +`ContentNegotiation is already installed. MCP requires json(McpJson) for correct JSON-RPC +serialization…`. It is expected, and it does not mean the MCP answers are malformed. + +The SDK prints it whenever the application already has ContentNegotiation, because Ktor does not let +it see which `Json` that is. Katcher's would matter: it pretty-prints and leaves out default values, +and an answer written with it would lose `protocolVersion` from `initialize` and the whole `result` +from `ping`. The endpoint writes its answers in MCP's own JSON before Katcher's ContentNegotiation +sees them. `McpWireFormatTest` holds that on the raw body: every answer of the assembled server — +both `initialize` versions, `ping`, the tool list, tool calls with and without a tool error, an +unknown tool and method, a batch, an unparseable body — is byte for byte what MCP's JSON writes. Its +control sends the same requests to the bare SDK under Katcher's JSON and requires them to come out +different, so a green run cannot mean the comparison catches nothing. + ### Behind a reverse proxy An MCP client is a machine and carries no browser session, so the forward-auth middleware diff --git a/server/src/commonTest/kotlin/io/github/youndie/katcher/mcp/McpEndpointTest.kt b/server/src/commonTest/kotlin/io/github/youndie/katcher/mcp/McpEndpointTest.kt index d38cd15..ae121f0 100644 --- a/server/src/commonTest/kotlin/io/github/youndie/katcher/mcp/McpEndpointTest.kt +++ b/server/src/commonTest/kotlin/io/github/youndie/katcher/mcp/McpEndpointTest.kt @@ -16,7 +16,6 @@ import io.ktor.http.contentType import io.ktor.http.encodedPath import io.ktor.server.testing.ApplicationTestBuilder import io.ktor.server.testing.testApplication -import io.modelcontextprotocol.kotlin.sdk.types.LATEST_PROTOCOL_VERSION import kotlinx.serialization.json.Json import kotlinx.serialization.json.JsonObject import kotlinx.serialization.json.boolean @@ -32,7 +31,8 @@ import kotlin.test.assertEquals * What the endpoint does with a token, a `Host` header or a protocol version is kore-mcp's, and kore * tests it. What is katcher's is the wiring — that the module installs the endpoint behind its own * ContentNegotiation and registers the five tools on it — and the one property katcher's own copy of - * that wiring got wrong: which requests the token check sees. + * that wiring got wrong: which requests the token check sees. Whether the answers leave in MCP's JSON + * despite that ContentNegotiation is [McpWireFormatTest]'s. * * A file database, for the reason [RepositoryTest] gives; nothing here reads it. */ @@ -59,8 +59,8 @@ class McpEndpointTest : RepositoryTest() { setBody(body) } - private fun initialize(version: String = "2025-06-18") = - """{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"$version",""" + + private fun initialize() = + """{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-06-18",""" + """"capabilities":{},"clientInfo":{"name":"test","version":"1"}}}""" private suspend fun HttpResponse.result(): JsonObject = @@ -101,22 +101,6 @@ class McpEndpointTest : RepositoryTest() { ) } - @Test - fun `an initialize asking for the latest protocol should get its version back`() = - testApplication { - // Given — katcher's ContentNegotiation omits defaults, and SDK 0.15.0 answers through it: - // without kore-mcp encoding the response itself, this is the field that goes missing and - // no current client connects. - katcher() - - // When - val response = rpc(initialize(LATEST_PROTOCOL_VERSION)) - - // Then - assertEquals(HttpStatusCode.OK, response.status) - assertEquals(LATEST_PROTOCOL_VERSION, response.result()["protocolVersion"]?.jsonPrimitive?.content) - } - /** * Ktor's router drops empty path segments and decodes each one, so every one of these reaches the * handler `/mcp` reaches, while the request's path string is something else. A check that decides diff --git a/server/src/commonTest/kotlin/io/github/youndie/katcher/mcp/McpWireFormatTest.kt b/server/src/commonTest/kotlin/io/github/youndie/katcher/mcp/McpWireFormatTest.kt new file mode 100644 index 0000000..8863926 --- /dev/null +++ b/server/src/commonTest/kotlin/io/github/youndie/katcher/mcp/McpWireFormatTest.kt @@ -0,0 +1,198 @@ +package io.github.youndie.katcher.mcp + +import io.github.youndie.katcher.KatcherProbes +import io.github.youndie.katcher.ServerConfig +import io.github.youndie.katcher.common +import io.github.youndie.katcher.data.RepositoryTest +import io.github.youndie.katcher.initDi +import io.github.youndie.katcher.module +import io.ktor.client.request.header +import io.ktor.client.request.post +import io.ktor.client.request.setBody +import io.ktor.client.statement.bodyAsText +import io.ktor.http.ContentType +import io.ktor.http.HttpHeaders +import io.ktor.http.HttpStatusCode +import io.ktor.http.contentType +import io.ktor.server.plugins.di.dependencies +import io.ktor.server.plugins.di.resolve +import io.ktor.server.testing.ApplicationTestBuilder +import io.ktor.server.testing.testApplication +import io.modelcontextprotocol.kotlin.sdk.server.Server +import io.modelcontextprotocol.kotlin.sdk.server.ServerOptions +import io.modelcontextprotocol.kotlin.sdk.server.mcpStatelessStreamableHttp +import io.modelcontextprotocol.kotlin.sdk.types.Implementation +import io.modelcontextprotocol.kotlin.sdk.types.JSONRPCMessage +import io.modelcontextprotocol.kotlin.sdk.types.LATEST_PROTOCOL_VERSION +import io.modelcontextprotocol.kotlin.sdk.types.McpJson +import io.modelcontextprotocol.kotlin.sdk.types.ServerCapabilities +import kotlinx.serialization.builtins.ListSerializer +import kotlinx.serialization.serializer +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * The warning katcher logs at start when `MCP_TOKEN` is set — `ContentNegotiation is already + * installed. MCP requires json(McpJson)…` — and the question behind it: do MCP's messages leave in + * katcher's JSON instead of the protocol's? SDK 0.15.0 prints it whenever the application already has + * ContentNegotiation, because Ktor's public API does not show which `Json` that is; it cannot look. + * + * For katcher the warning is not idle. Its `Json` (`common()`) pretty-prints and does not write + * defaults. One thing on the SDK's stateless transport goes through the application's + * ContentNegotiation — the answer to a `POST` in JSON mode — and under that `Json` an `initialize` + * answer loses `protocolVersion` (no current client connects) and a `ping` answer loses its whole + * `result` (no longer JSON-RPC). kore-mcp encodes that answer with `McpJson` on the transport's route + * before ContentNegotiation sees it; this test is what holds katcher's module to that. + * + * The measure is [mismatch], on the raw body: every answer must be exactly what `McpJson` writes for + * the message inside it. A different `Json` breaks the equality — a missing default, an extra `null`, + * whitespace. + */ +class McpWireFormatTest : RepositoryTest() { + private fun initialize(version: String) = + """{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"$version",""" + + """"capabilities":{},"clientInfo":{"name":"test","version":"1"}}}""" + + /** + * Requests the transport answers with a message: `initialize` with the latest protocol version + * (which is also the field's default) and with an older one, `ping` (whose whole `result` is a + * default), the tool list, a call without the optional fields, a call that ends in a tool error, + * a tool and a method that do not exist, a batch of two, and a body that does not parse at all. + */ + private val exchanges = + listOf( + Exchange("initialize latest", initialize(LATEST_PROTOCOL_VERSION)), + Exchange("initialize older", initialize("2025-06-18")), + Exchange("ping", """{"jsonrpc":"2.0","id":3,"method":"ping"}"""), + Exchange("tools/list", """{"jsonrpc":"2.0","id":4,"method":"tools/list"}"""), + Exchange( + "tools/call without arguments", + """{"jsonrpc":"2.0","id":5,"method":"tools/call","params":{"name":"list_apps"}}""", + ), + Exchange( + "tools/call ending in a tool error", + """{"jsonrpc":"2.0","id":6,"method":"tools/call","params":{"name":"get_crash_metadata",""" + + """"arguments":{"groupId":999}}}""", + ), + Exchange( + "tools/call of a tool that does not exist", + """{"jsonrpc":"2.0","id":7,"method":"tools/call","params":{"name":"no_such_tool"}}""", + ), + Exchange("method not found", """{"jsonrpc":"2.0","id":8,"method":"no/such/method"}"""), + Exchange( + "batch", + """[{"jsonrpc":"2.0","id":9,"method":"ping"},{"jsonrpc":"2.0","id":10,"method":"tools/list"}]""", + ), + Exchange("unparseable body", """{"jsonrpc":"2.0","id":""", HttpStatusCode.BadRequest), + ) + + @Test + fun `every MCP answer of the assembled server should be exactly what McpJson writes`() = + testApplication { + // Given — the module as `main` starts it: katcher's ContentNegotiation from `common()` + // first, then `installKoreMcp`, which is the order in which the SDK warns. + setupSchema() + application { module(db, CONFIG, KatcherProbes(db)) } + + // When + val mismatches = mismatches() + + // Then + assertEquals(emptyMap(), mismatches) + } + + /** + * POSITIVE CONTROL for the test above: katcher's ContentNegotiation, katcher's five tools and the + * same requests, but the transport installed by the bare SDK instead of kore-mcp, so the answer + * goes out through `common()`'s `Json`. Without it the green above could mean the measure catches + * nothing. + * + * Measured on SDK 0.15.0: nine of the ten answers differ — all but the unparseable body, which the + * SDK writes itself. With pretty-printing taken out of katcher's `Json` three still do: `initialize` + * with the latest version, `ping` and the batch that carries one, none of which `McpJson` can even + * read back. Under `McpJson` itself the same bare SDK differs on none. + * + * **If this turns red, the test above has stopped proving anything.** Either the SDK no longer + * answers through the application's ContentNegotiation — the startup warning will have gone too; + * drop this control and the README's paragraph on the warning — or `common()`'s `Json` now writes + * what `McpJson` writes, and the control needs a `Json` that does not. + */ + @Test + fun `control - the bare SDK should answer through katcher's Json`() = + testApplication { + // Given + setupSchema() + application { + common() + initDi(db, CONFIG) + val tools = KatcherMcpServer(dependencies.resolve(), dependencies.resolve(), dependencies.resolve()) + mcpStatelessStreamableHttp(path = "/mcp", enableDnsRebindingProtection = false) { + Server( + INFO, + ServerOptions(ServerCapabilities(tools = ServerCapabilities.Tools(listChanged = false))), + ).apply { tools.register(this) } + } + } + + // When + val mismatches = mismatches() + + // Then + assertTrue(mismatches.isNotEmpty(), "the bare SDK answered in McpJson under katcher's Json - see the KDoc") + } + + /** Each exchange of [exchanges] whose body did not match `McpJson`, with how it did not. */ + private suspend fun ApplicationTestBuilder.mismatches(): Map = + exchanges + .mapNotNull { exchange -> + val response = + client.post("/mcp") { + // Every real client sends one, and the module checks it against the allowlist. + header(HttpHeaders.Host, HOST) + header(HttpHeaders.Authorization, "Bearer $TOKEN") + header(HttpHeaders.Accept, "application/json, text/event-stream") + contentType(ContentType.Application.Json) + setBody(exchange.request) + } + val body = response.bodyAsText() + assertEquals(exchange.status, response.status, "${exchange.name}: $body") + mismatch(body)?.let { exchange.name to it } + }.toMap() + + private class Exchange( + val name: String, + val request: String, + val status: HttpStatusCode = HttpStatusCode.OK, + ) + + private companion object { + const val TOKEN = "secret" + const val HOST = "katcher.example.com" + + // Configured as the chart configures it: the token, and `MCP_ALLOWED_HOSTS` filled from `hostname`. + val CONFIG = ServerConfig(mcpToken = TOKEN, mcpAllowedHosts = listOf(HOST)) + val INFO = Implementation(name = "katcher", version = "0.1.0") + + val message = serializer() + val batch = ListSerializer(message) + + /** + * `null` if [body] is exactly what `McpJson` writes for the message (or batch) inside it; + * otherwise how it differs. Both the read and the write are `McpJson`'s: a missing field comes + * back as its default, an extra `null` is dropped, no whitespace is written, and the bytes + * part; a body that lost what tells the message type apart does not decode at all. + */ + fun mismatch(body: String): String? { + val rewritten = + runCatching { + if (body.startsWith("[")) { + McpJson.encodeToString(batch, McpJson.decodeFromString(batch, body)) + } else { + McpJson.encodeToString(message, McpJson.decodeFromString(message, body)) + } + }.getOrElse { return "not a JSON-RPC message: ${it.message}" } + return if (rewritten == body) null else "McpJson writes $rewritten, got $body" + } + } +}