mcp: implement sampling with tools#699
mcp: implement sampling with tools#699maciej-kisiel merged 6 commits intomodelcontextprotocol:mainfrom
Conversation
|
Good stuff @findleyr. |
findleyr
force-pushed
the
toolsampling
branch
from
7640e70 to
d3c9a52
Compare
Add support for tool use within sampling requests, as described in the MCP spec's sampling.tools capability. New content types: ToolUseContent and ToolResultContent for sampling messages. New capability types: SamplingCapabilities gains Tools and Context sub-fields, plus ToolChoice for controlling tool invocation. Following the TypeScript SDK's pattern, tool-enabled sampling uses separate types from basic sampling for backward compatibility: - CreateMessageWithToolsParams with SamplingMessageV2 (array content) - CreateMessageWithToolsResult (array content) - ServerSession.CreateMessageWithTools and ClientOptions.CreateMessageWithToolsHandler The basic CreateMessage/CreateMessageResult API is unchanged. Both paths share the same wire method (sampling/createMessage) and go through the method info table: the table uses the broader WithTools result type, and CreateMessage downconverts (erroring if multiple content blocks are returned). Setting CreateMessageWithToolsHandler infers the sampling.tools capability. It is a panic to set both CreateMessageHandler and CreateMessageWithToolsHandler.
findleyr
force-pushed
the
toolsampling
branch
from
d3c9a52 to
41908dd
Compare
|
If you skim over the tests, this change isn't actually that large. Unfortunately, we needed to add new APIs to work around the spec change, but what we've chosen is consistent with typescript. Notably, the new |
- Add CreateMessageWithToolsParams, SamplingMessageV2, and CreateMessageWithToolsResult types for tool-enabled sampling with array content support (parallel tool calls) - Add ServerSession.CreateMessageWithTools and ClientOptions.CreateMessageWithToolsHandler - Remove Tools/ToolChoice from CreateMessageParams (moved to WithTools) - Infer sampling.tools capability from CreateMessageWithToolsHandler - Panic if both CreateMessageHandler and CreateMessageWithToolsHandler are set - CreateMessage errors if client returns multiple content blocks - Reject JSON null in unmarshalContent; return non-nil empty slice for empty arrays - Remove tool_result from result allow-list (only valid in user messages) - Rename wireContent.ToolResultContent to NestedContent - Fix clone() to deep-copy Sampling sub-fields - Fix typo "maximyum" and doubled phrase in IncludeContext doc - Add rough_edges.src.md note for v2 unification
maciej-kisiel
left a comment
maciej-kisiel
left a comment
There was a problem hiding this comment.
Sharing the comments without looking at the test file to expedite the process. I will look at the remaining file soon.
| // Content holds the unstructured result of the tool call. | ||
| Content []Content | ||
| // StructuredContent holds an optional structured result as a JSON object. | ||
| StructuredContent any |
There was a problem hiding this comment.
For my own education: why the same logic as to ToolUseContent.Input doesn't apply here? They are both defined the same way in the specification.
There was a problem hiding this comment.
They serve different purposes: ToolUseContent.Input is a flat JSON object (tool arguments), so map[string]any is the natural representation — and we normalize nil to {} on marshal to satisfy the spec's required field. ToolResultContent.Content is a list of typed content blocks ([]Content), which goes through contentsFromWire for proper type dispatch. The spec defines them the same way structurally (both are required), but their Go representations differ because of the nested content semantics.
There was a problem hiding this comment.
My comment was more about ToolResultContent.StructuredContent and the differentiation between any and map[string]any. But after some thought, I think this is because we want to be able let the user put any result object here, that is marshalable to a JSON object, and not an intermediate data structure that would enforce correct schema. I just wonder if we ever enforce that this needs to be a JSON object vs. any other JSON value.
| // returning a [CreateMessageWithToolsResult] that supports array content | ||
| // (for parallel tool calls). Use this instead of [ServerSession.CreateMessage] | ||
| // when the request includes tools. | ||
| func (ss *ServerSession) CreateMessageWithTools(ctx context.Context, params *CreateMessageWithToolsParams) (*CreateMessageWithToolsResult, error) { |
There was a problem hiding this comment.
Should these check if the client has the capability?
There was a problem hiding this comment.
Good question. Neither CreateMessage nor CreateMessageWithTools currently validates capabilities, and checkInitialized is itself still a TODO. I think adding capability validation is worth doing, but it's broader scope — it should probably cover all capability-gated server→client requests consistently. Want me to file an issue for this?
There was a problem hiding this comment.
Please do. I just wonder, given this is a new handler that it would be easier to validate than to introduce a behavior change later on, but as you say, if we want to do it consistently we will need to face this challenge anyways.
maciej-kisiel
left a comment
maciej-kisiel
left a comment
There was a problem hiding this comment.
As promised, I took a thorough look at the tests.
I think we could improve their structure and placing. The main high level principle I would follow would be:
Introduce _MarshalJSON and _UnmarshalJSON (potentially table driven if there are multiple interesting cases) tests for each new protocol message and put those in appropriate _test.go files, corresponding to the files where they are defined.
I left more concrete comments how this principle would apply to the current proposal.
Fix capability override bug where Tools inference could overwrite manually set sampling capabilities. Annotate wireContent fields with their owning content types. Restructure tests: rename sampling_tools_test.go to sampling_test.go, move marshal/unmarshal tests to protocol_test.go as table-driven tests, fix test naming, and add realistic message history to error integration test.
Rather than silently dropping extra content blocks during the CreateMessageWithToolsParams→CreateMessageParams downconversion, return an error so callers learn to use CreateMessageWithToolsHandler.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks |
| // Content holds the unstructured result of the tool call. | ||
| Content []Content | ||
| // StructuredContent holds an optional structured result as a JSON object. | ||
| StructuredContent any |
There was a problem hiding this comment.
My comment was more about ToolResultContent.StructuredContent and the differentiation between any and map[string]any. But after some thought, I think this is because we want to be able let the user put any result object here, that is marshalable to a JSON object, and not an intermediate data structure that would enforce correct schema. I just wonder if we ever enforce that this needs to be a JSON object vs. any other JSON value.
4 participants
Summary
Add support for tool use within sampling requests, as described in the MCP 2025-11-25 spec's sampling.tools capability.
New types
Content types:
Capability types:
Tool-enabled sampling (parallel tool calls):
Design
Following the TypeScript SDK's pattern, tool-enabled sampling uses separate types from basic sampling to avoid breaking the existing API. The basic CreateMessage/CreateMessageResult path
is unchanged.
Both paths share the same wire method (sampling/createMessage) and go through the method info table. The table uses the broader CreateMessageWithToolsResult type internally;
CreateMessage downconverts (erroring if multiple content blocks are returned). This is documented as a rough edge to unify in v2.
Setting CreateMessageWithToolsHandler automatically infers the sampling.tools capability. It is a panic to set both CreateMessageHandler and CreateMessageWithToolsHandler.
References