feat: implement support for SEP-1686 Tasks#755
feat: implement support for SEP-1686 Tasks#755LucaButBoring wants to merge 12 commits intomodelcontextprotocol:mainfrom
Conversation
4cf98e9 to
82ccfa1
Compare
|
Nice to have this in, the api seems quite the same as typescript sdk |
|
@LucaButBoring Thanks. I'll try to review the PR. In the meantime, please rebase against |
82ccfa1 to
8baba5e
Compare
|
Rebased - this is (unfortunately) a very large PR, but I recommend starting with the code related to the key design decisions, and there's also a new integration test ( |
| * @see McpSchema.CallToolResult | ||
| * @see McpSchema.CreateMessageResult | ||
| */ | ||
| public <T extends McpSchema.ServerTaskPayloadResult> T getTaskResult( |
There was a problem hiding this comment.
Shouldn't this be ClientTaskPayloadResult? Regardless, there is no current way to get the input_required request from the client for a task.
There was a problem hiding this comment.
This is the client, so when it calls tasks/result, it will only receive the appropriate server message for the task (currently only CallToolResult).
The input_required-bound request happens asynchronously relative to this but (with SSE) requires the client to invoke tasks/result first.
There was a problem hiding this comment.
There's a lengthy (and admittedly hard to parse) note on the input_required status in the spec explaining the side-channeling here: https://modelcontextprotocol.io/specification/2025-11-25/basic/utilities/tasks#input-required-status
There was a problem hiding this comment.
Why have the typeRef? There's only 1 possible result type.
There was a problem hiding this comment.
Mostly for consistency with the server version of this interface, and so that we don't need to make a breaking change if (I expect "when") we add more supported client->server methods for Tasks.
There was a problem hiding this comment.
Oh, I left in a comment about sampling here that isn't relevant which probably muddies the waters, I'll adjust that.
There was a problem hiding this comment.
Updated the client-side documentation, will tweak the server-side documentation in a bit.
There was a problem hiding this comment.
Updated documentation for:
- McpSyncClient.getTask
- McpSyncClient.getTaskResult
- McpAsyncClient.getTask
- McpAsyncClient.getTaskResult
- McpSyncServerExchange.getTask
- McpSyncServerExchange.getTaskResult
- McpAsyncServerExchange.getTask
- McpAsyncServerExchange.getTaskResult
| * @param resultTypeRef Type reference for deserializing the result. | ||
| * @return The task result. | ||
| */ | ||
| public <T extends McpSchema.ServerTaskPayloadResult> T getTaskResult(String taskId, TypeRef<T> resultTypeRef) { |
|
I'm testing with our server implementation. The protocol version in the initialize request is |
Will fix this.
That's expected for tool calls from the client - the client only declares a I'll update the documentation to try and make this more clear. |
I ran into an issue with this. I've been developing server support for tasks for a while now. I was testing with the current version of the MCP Inspector which reports protocol version |
|
The current version of the inspector doesn't support Tasks, that's being worked on here: modelcontextprotocol/inspector#1013 |
Yeah, I know. But it still sends protocol version |
Also added taskId convenience overloads.
|
It is, I'm testing my fix for that right now - looks like #733 added the constant for it but the corresponding constant for the latest version wasn't changed to it. |
|
@Randgalt That requires changing a lot of tests, so I'm considering making that change in a separate PR, is that fine? The protocol version change is technically needed for all of the 11/25 spec features, so I don't think this PR should include it (it should've been done independently first, really). For testing right now, you can declare the supported protocol versions when constructing the transport, like this: var transport = HttpClientStreamableHttpTransport // or WebClientStreamableHttpTransport
.builder("http://localhost:" + PORT) // use WebClient.builder().baseUrl() here for WebClientStreamableHttpTransport
.supportedProtocolVersions(List.of(ProtocolVersions.MCP_2025_11_25))
.build(); |
|
Opened #758 to bump the declared protocol version |
|
OK - I pulled both PRs and made a temp branch that has both of them and did some testing. I still don't understand how Here's my code: The above stays at |
|
I just tried with
That's it, we don't see the elicit result nor another |
| * @see McpSchema.ResultMessage | ||
| * @see McpSchema.ErrorMessage | ||
| */ | ||
| public List<McpSchema.ResponseMessage<McpSchema.CallToolResult>> callToolStream( |
There was a problem hiding this comment.
This method seems misnamed. It's not returning a stream but a list.
There was a problem hiding this comment.
Ah, I forgot about this - I was a bit on the fence about what the return type on this should be, actually. Conventionally, we convert all Flux<T> into List<T>, but following that causes these awkward cases (also in elicitation/sampling) where we have a *Stream method that doesn't actually return a Stream<T>.
I can change these to return a Stream<T> in the sync interfaces, I'm just not sure if that's a clear method contract for a synchronous method, since streams have an opaque execution contract.
I suppose I can make these into actual streams and just document the underlying behavior more clearly.
There was a problem hiding this comment.
Actually, Flux.toStream() blocks for each item emitted into the stream, hm. At any rate, I'll just make a stream and we can go from there.
|
Looks like elicitation/sampling aren't wired up to the message queue correctly, I'm adjusting the implementation/tests to address this and properly detect issues with it (this was also an oversight I made in TS at first but the API differences tripped me up when verifying it). Will update once that's resolved. |
|
Should be fixed, now - wired up the queue properly (and trimmed its API surface down a fair bit) and updated the tests to avoid internal blocking, which was masking the issue before |
It's still behaving somewhat as before. I call Then If I use |
mcp-core/src/main/java/io/modelcontextprotocol/client/McpClientFeatures.java
Outdated
Show resolved
Hide resolved
| @@ -1590,7 +2656,7 @@ public record CallToolResult( // @formatter:off | |||
| @JsonProperty("content") List<Content> content, | |||
| @JsonProperty("isError") Boolean isError, | |||
| @JsonProperty("structuredContent") Object structuredContent, | |||
| @JsonProperty("_meta") Map<String, Object> meta) implements Result { // @formatter:on | |||
| @JsonProperty("_meta") Map<String, Object> meta) implements ServerTaskPayloadResult { // @formatter:on | |||
There was a problem hiding this comment.
public record CallToolResult( // @formatter:off
@JsonProperty("content") List<Content> content,
@JsonProperty("isError") Boolean isError,
@JsonProperty("structuredContent") Object structuredContent,
@JsonProperty("task") Task task,
@JsonProperty("_meta") Map<String, Object> meta) implements Result, GetTaskPayloadResult {I think something like this would be better.
There was a problem hiding this comment.
You mean so that CallToolResult implicitly satisfies CreateTaskResult? It seems unintuitive for SDK consumers to have an ambiguous union like that in method signatures.
There was a problem hiding this comment.
Yes,I implemented as this, which is simpler
mcp-core/src/main/java/io/modelcontextprotocol/client/McpClientFeatures.java
Outdated
Show resolved
Hide resolved
|
@Randgalt I pushed a working client/server example to a separate branch (client; server) - how closely does this match your repro? Trying to figure out where our setups diverge so I can diagnose this effectively. |
This helps, thank you. I found one bug in our server and what I think is a bug in your client. We're expecting |
872aff7 to
f3aff3e
Compare
|
Updated both branches with the relevant fix - metadata added to all task-related results and notifications |
|
@LucaButBoring we're getting farther, but still not working completely. The elicit request/response loop seems to be working. But, once we receive the ElicitResult we don't get called again and the task times out. Here are the messages we receive: Our test code sees the Flux/Mono is impossible to debug - awful experience. I'll keep trying. |
|
@LucaButBoring IMO there seem to be a lot of assumptions in this PR. I did some more testing by calling |
|
OK - I think I've proven then issue @LucaButBoring I modified our server so that when the Per the docs as I read them, however, this is not correct. When the client gets the |
|
That is a valid flow, but not the only valid flow (rather, if the client disconnects polling still works). The relevant part here is from section 5.6:
|
Ok. If so, that’s unfortunate. I’ll see what can be done to improve the spec. But I’ll adjust our server accordingly and get back to you. |
|
Definitely room for discussion on this, and the incoming Transports WG proposals may very well change this - I'll be discussing this with them in the meeting tomorrow. |
|
OK - at long last everything is working. Thanks for all your work on this. |
|
@LucaButBoring I apologize, I promised I'd review this PR and started doing that but had to prioritize the conformance tests and had to take a pause. I will get back to it after I'm back from PTO in a week. My quick feedback for now is that the implementation is quite invasive and scattered in the core McpAsyncServer class and it would be a good opportunity to look at #578 to provide better encapsulation of this feature. I'll do a more thorough review later. |
|
There are at least a few reasons why it's as invasive as it is right now, off the top of my head:
With that said, I'm open to overhauling this entirely if it's possible to encapsulate this in a repository, I just don't currently see how that could be done without making invasive changes anyways (tasks don't exactly satisfy |
|
New test failure appears to be existing flakiness - I haven't yet been able to reproduce it. |
|
@chemicL The TS SDK is refactoring their implementation of Tasks in modelcontextprotocol/typescript-sdk#1449, and I think their new approach is slightly better about decoupling logic from the core message exchange - I'm going to try to refactor this to extract a |

Implements SEP-1686 (Tasks), from the latest specification release.
Motivation and Context
Tasks address several protocol gaps:
start_tool,get_status,get_resulttools; a single task-aware tool handles the full lifecycleUsage
Server: Defining a Task-Aware Tool
Server: Using TaskContext for Lifecycle Management
TaskSupportMode options:
REQUIRED(default): Must have task metadata; returns error otherwiseOPTIONAL: Works with or without task metadata; auto-polling shim provides backward compatibilityFORBIDDEN: No task support (regular tools)Client: Streaming API (Recommended)
Drop-in replacement for
callToolthat handles polling automatically:Client: Task-Based API (For Explicit Control)
For consumers who need custom polling behavior, cancellation logic, or batched task management:
Similar patterns exist for sampling (
createMessageStream/createMessageTask) and elicitation (createElicitationStream/createElicitationTask).Server: Bidirectional Task Flows
Servers can send task-augmented requests to clients, assuming the client has configured its own
TaskStore:Key Design Decisions
Experimental namespace - All task APIs are in
io.modelcontextprotocol.experimental.tasks, signaling that the API may change (matches TypeScript/Python SDKs)TaskStore abstraction - Interface for pluggable storage;
InMemoryTaskStoreprovided for development and testing. The originating request (e.g.,CallToolRequest) is stored alongside the task, so tool routing can be derived from stored context rather than maintained as separate mapping state.Auto-polling shim -
OPTIONALmode tools work transparently for non-task-aware clientsDefense-in-depth session isolation - Session ID required on all TaskStore operations; enforced at both server and storage layers to prevent cross-session task access
nullforsessionIdbypasses validation (single-tenant mode). This is used byMcpAsyncClientsince clients are inherently single-tenant - there's only one session, so cross-session isolation doesn't apply.How Has This Been Tested?
Breaking Changes
None
Types of changes
Checklist
Additional context
Closes #668
This PR also includes a tweak to how
202 Acceptedis handled by the client implementation, which was done to handle how the TypeScript server SDK configures its response headers when accepting JSON-RPC responses and notifications from the client - in particular, sendingInitializeNotificationproduced an exception in the Java SDK client before this, which made testing compatibility with the existing TS SDK's Tasks implementation rather difficult.