Repository navigation
Fix response result deserialization to use the original request method (#601) - #999
Conversation
modelcontextprotocol#601) An incoming JSON-RPC response's result was decoded by JSON shape alone, so a response whose payload matches another type's shape (e.g. a tasks/result payload shaped like CallToolResult) was completed with the wrong runtime type and failed at the erased generic cast in Protocol.request. - Capture the raw result JSON while decoding JSONRPCResponse on the wire path, keeping the public JSONRPCResponse API unchanged. - At the request/response correlation point, decode the raw result with the deserializer declared by the original request's method, falling back to the shape-decoded result for custom methods, programmatically constructed responses, and task-augmented calls returning CreateTaskResult. - Rewrite the SSE replay message id in the raw JSON before decoding in StreamableHttpClientTransport instead of copying the decoded message, so the raw result survives. - Results whose shape matches no known type now surface as GetTaskPayloadResult (the raw-payload result type) instead of failing the whole message decode.
![jetbrains-air[bot]](https://avatars.githubusercontent.com/in/3314082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for taking this on — the correlation-based direction is the right one; just one blocking item inline before this can go in.
Produced by Air Automations. Name: Code review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/409567ec-d3a7-40ac-ba95-8518ec577ac2?run=910be83c-9a31-4d4d-bf9e-b99237674527
| "Failed to deserialize the result of '${method.value}' as " + | ||
| "'${deserializer.descriptor.serialName}'; falling back to the shape-decoded result" | ||
| } | ||
| response.result |
There was a problem hiding this comment.
Blocking: this catch treats every failure to decode the method's declared result type as "must be a task-augmented response" and silently falls back to the shape-decoded value, with the real cause going to a debug log only. Together with the new GetTaskPayloadResult fallback in JSONRPCResponseWireSerializer, a malformed payload for any known method now reaches the caller as an unrelated cast error.
Failure scenario (ran both revisions to confirm): a server replies to tools/call with {"content":[{"type":"text"}]} (missing text). On main the decode surfaces MissingFieldException: Field 'text' is required for type with serial name '…TextContent'. On this branch the caller gets ClassCastException: GetTaskPayloadResult cannot be cast to CallToolResult, and nothing in the exception mentions the malformed payload. Same for tasks/get returning {"taskId":"x","status":"bogus-status"}: TaskStatus does not contain element with name 'bogus-status' becomes ClassCastException: GetTaskPayloadResult cannot be cast to GetTaskResult. So every server-side protocol violation or version skew now reports as a cast error naming a tasks type unrelated to the request.
Suggested fix: narrow the fallback to the one legitimate divergence instead of swallowing all of them — fall back only when response.result is CreateTaskResult (task augmentation), and otherwise throw e so request() completes exceptionally with the actual SerializationException. That keeps the fail-fast behavior this PR wants while preserving the diagnostic.
| * request's method (see https://github.com/modelcontextprotocol/kotlin-sdk/issues/601). | ||
| */ | ||
| @Transient | ||
| internal var rawResult: JsonElement? = null |
There was a problem hiding this comment.
rawResult is a @Transient body var on a public data class, so the generated public copy() silently drops it. The "never copy a decoded response" invariant the fix now depends on is enforced only by KDoc — no compiler check, no test.
Failure scenario: response.copy(id = …) on a wire-decoded response reverts to shape-based decoding, i.e. exactly the two call sites this PR had to rewrite in StreamableHttpClientTransport. Reproduced locally: copy() of a decoded tasks/result response whose payload is CallToolResult-shaped makes request<GetTaskPayloadResult> fail with ClassCastException, and the new tests do not catch it. The sibling gap in the same replay path (JSONRPCError ids are still not rewritten) is a plausible next place someone reaches for copy.
Suggested fix: make losing the raw JSON non-fatal rather than relying on the comment — in deserializeResult, when rawResult is null, re-encode response.result and decode that with the method's deserializer (the approach #913 took for directly constructed responses), or carry the raw element as a constructor property so copy() preserves it.
| } catch (e: SerializationException) { | ||
| (resultElement as? JsonObject)?.let(::GetTaskPayloadResult) ?: throw e | ||
| } |
There was a problem hiding this comment.
Both items from the last round are properly addressed — happy to approve this.
Produced by Air Automations. Name: Code review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/409567ec-d3a7-40ac-ba95-8518ec577ac2?run=87f81b78-dc05-4392-bd34-b1e11f8d142b
| * @param result The result for the copied response. | ||
| * @return A response carrying the selected public fields and matching wire data. | ||
| */ | ||
| public fun copy(id: RequestId = this.id, result: RequestResult = this.result): JSONRPCResponse = |
There was a problem hiding this comment.
The PR description no longer matches the code, and this repo squash-merges with squash_merge_commit_message: PR_BODY, so the description becomes the permanent commit message. The "How" section's last bullet still claims "Streamable HTTP replay rewrites the replay id in the raw JSON before decoding (decodeIncomingMessage) instead of copying an already shape-decoded message, at both SSE sites" — but StreamableHttpClientTransport.kt is not in the diff at all; both SSE sites still call msg.copy(id = replayMessageId) (lines 468 and 517) and this copy override is what actually carries the raw JSON across the id rewrite. The "Intentional behavior change" and "Tests" sections are stale too: for the 21 mapped methods a bad payload now surfaces as the real SerializationException rather than reaching the caller as a cast failure, and ProtocolResultDeserializationTest has 13 tests, not 7.
Failure scenario: someone bisecting a future regression in the SSE replay path lands on this squashed commit, reads that the id rewrite happens in the raw JSON via decodeIncomingMessage, and greps for a function and a transport change that never existed — while the real invariant (copy() preserves rawResult only while result is referentially unchanged) goes unmentioned in history.
Suggested fix: update the PR body before merge — replace the transport bullet with the JSONRPCResponse.copy() raw-JSON preservation mechanism, correct the behavior-change paragraph to say decode errors now propagate for known methods, and refresh the test counts.
44c0630
There was a problem hiding this comment.
Everything from the previous rounds checks out on this revision — happy to approve.
Produced by Air Automations. Name: Code review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/409567ec-d3a7-40ac-ba95-8518ec577ac2?run=73f10ee5-3aed-4c36-b193-dcbd1ffc5a0b
Closes #601.
Response results were selected by JSON shape before being correlated with their original request. Overlapping shapes could therefore produce the wrong runtime type and a ClassCastException, especially for tasks/result and tasks/get. The shared wire decoder now captures the original result JSON, and request correlation decodes it using the request method. Provisional shape checks, including those that throw IllegalArgumentException before serializer validation, cannot reject an arbitrary tasks/result object. Task-augmented requests recognize task envelopes before the normal result type, while still accepting normal responses when augmentation is ignored.
JSONRPCResponse.copy() preserves the captured JSON only when result is referentially unchanged; replacing result clears it. This makes the existing Streamable HTTP replay copies preserve the payload without changing the transport implementation. The public constructor, copy signature, value semantics, and serialized shape remain unchanged. Known-method decoding failures, including SerializationException for malformed typed fields, reach the caller. Custom or unknown methods retain shape-based validation and its original errors rather than accepting the deferred GetTaskPayloadResult fallback; responses constructed without captured wire JSON retain their prior behavior.
Verification on macOS arm64 with the existing JDK 21 environment:
The request-correlation approach follows the maintainer guidance on #913. Developed with AI assistance.