Add SEP-1577 sampling with tools support#1018
Conversation
Introduces parallel V2 sampling types (SamplingMessageV2, CreateMessageWithToolsRequest/Result, ToolUseContent, ToolResultContent, ToolChoice) alongside the existing V1 types, leaving the legacy wire format byte-identical. Servers call createMessageWithTools on the exchange; a version gate refuses multi-content or tools payloads when the negotiated protocol version is older than 2025-11-25. Clients opt in via samplingWithTools(handler) on the builder, which advertises sampling.tools in the capability handshake. StopReason gains TOOL_USE. Signed-off-by: Dariusz Jędrzejczyk <2554306+chemicL@users.noreply.github.com>
Kehrlann
left a comment
Kehrlann
left a comment
There was a problem hiding this comment.
There's a "MUST" about tool use / result balance that seems to be missing ; but I'm not fully sure where this should even be enforced. I guess this can be handled by the implementer?
Also, it'd be really neat to have at least one happy-path integration test.
| @JsonInclude(JsonInclude.Include.NON_ABSENT) | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public record Sampling() { | ||
| public record Sampling(@JsonProperty("tools") SamplingTools tools) { |
There was a problem hiding this comment.
SEP-1577 also defines a context property for sampling (schema)
is this ignored on purpose?
There was a problem hiding this comment.
Looking at this property more closely made me realize that it wasn't included because it might be removed.. Looking more closely I realized that Sampling itself is being deprecated and will be removed. Thank you for your careful review, but with the information from the upcoming spec I will close this PR...
| return Mono.error( | ||
| new IllegalStateException("Received sampling request, but no sampling handler is registered.")); | ||
| } | ||
| return this.samplingHandler.apply(v1Request).cast(Object.class); |
There was a problem hiding this comment.
If the incoming request has no tools or toolChoice (both of which are optional), then we route to v2. Otherwise, we route to v1.
A server could send a request with or without tools, but the constructor only allows us to define one of samplingWithToolsHandler / samplingHandler (gated with if (withTools) / else). So if the server sends a request without tools to a v2-enabled client, we'll throw IllegalStateException because we're missing samplingHandler
| if (this.clientCapabilities.sampling() == null) { | ||
| return Mono.error(new IllegalStateException("Client must be configured with sampling capabilities")); | ||
| } | ||
| if (this.clientCapabilities.sampling().tools() == null) { |
There was a problem hiding this comment.
If tools is null, users cannot use this method. But this is the only way to create multi-content sampling requests, even without tools.
| * {@link McpSchema#PROTOCOL_VERSION_SAMPLING_WITH_TOOLS}, or when the version is | ||
| * {@code null} (unknown — treated as pre-SEP-1577). | ||
| */ | ||
| private static boolean isOlderThanSamplingWithToolsVersion(String negotiatedVersion) { |
There was a problem hiding this comment.
I had to scratch my head a bit at the name.
Consider renaming to supportsSamplingWithTools ?
| * {@link io.modelcontextprotocol.server.McpAsyncServerExchange#createMessageWithTools} | ||
| * must have negotiated at least this version. | ||
| */ | ||
| public static final String PROTOCOL_VERSION_SAMPLING_WITH_TOOLS = "2025-11-25"; |
There was a problem hiding this comment.
I don't think we should hint at particular feature gates within the schema. The schema reference doesn't mention this kind of behavior , I'd prefer having it closer to the feature itself.
Consider renaming to PROTOCOL_2025_11_25 and keeping the "sampling with tools" concept in the server instead
| public ToolChoice { | ||
| Assert.hasText(mode, "mode must not be empty"); | ||
| } |
There was a problem hiding this comment.
- Validate that
modeis one of the allowed modes "toolChoice": {}is a param for a sampling request (schema), and this would fail deserializing it . From what I understand, it should default toautoin that case?
| */ | ||
| @JsonInclude(JsonInclude.Include.NON_ABSENT) | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public record SamplingMessageV2( // @formatter:off |
There was a problem hiding this comment.
The name is confusing, since the it does not match the spec, but I can't think of anything better. Consider documenting why we need this.
Is SamplingMessage v1 intended to be replaced by v2 in the future?
| @JsonProperty("role") Role role, | ||
| @JsonProperty("content") | ||
| @JsonFormat(with = JsonFormat.Feature.ACCEPT_SINGLE_VALUE_AS_ARRAY) | ||
| List<Content> content) { // @formatter:on |
| void setNegotiatedProtocolVersion(String version) { | ||
| this.negotiatedProtocolVersion.set(version); | ||
| } | ||
|
|
There was a problem hiding this comment.
This cannot change over the lifetime of the session, can it? Should we guard against it?
|
Thank you for the careful review @Kehrlann. In light of the discoveries described in #693 (comment) I am closing this PR. |
2 participants
Introduces parallel V2 sampling types (
SamplingMessageV2,CreateMessageWithToolsRequest/Result,ToolUseContent,ToolResultContent,ToolChoice) alongside the existing V1 types, leaving the legacy wire format byte-identical. Servers callcreateMessageWithToolson the exchange; a version gate refuses multi-content or tools payloads when the negotiated protocol version is older than 2025-11-25. Clients opt in viasamplingWithTools(handler)on the builder, which advertises sampling.tools in the capability handshake.StopReasongainsTOOL_USE.