Implement SEP-1577: Sampling With Tools#628
Implement SEP-1577: Sampling With Tools#628alexhancock merged 2 commits intomodelcontextprotocol:mainfrom
Conversation
c2da172 to
f037e01
Compare
|
Hi @alexhancock, could you please take a look at this PR? Thanks! |
|
@DaleSeo Sorry it took me a bit. I'm glad to have support for this, but a little worried about the breaking change. Can you think of any alternatives that would keep compat, or make it a but lighter for updaters? Interested to discuss! |
95e640e to
73e60d7
Compare
|
Thanks for the feedback, @alexhancock! I've made a few updates to simplify the migration:
use std::convert::TryInto;
let content: Content = Content::text("hello");
let sampling_content: SamplingMessageContent = content.try_into()?;
// Before
SamplingMessage { role: Role::User, content: Content::text("hi") }
// After
SamplingMessage::user_text("hi")The type change is unavoidable to support tool use and result content types from SEP-1577, but I think the migration path is now reasonable. Most users can either use the new constructors or add Let me know if you have any other suggestions! |
alexhancock
left a comment
There was a problem hiding this comment.
Thanks @DaleSeo, approving but also leaving this list of recommendations.
Generated by Opus 4.5 comparing the diff to https://modelcontextprotocol.io/community/seps/1577--sampling-with-tools.md
## SEP-1577 Implementation Review - Recommended Changes
1. **Missing capability validation (MUST requirement)**: Add runtime validation to throw an error when `tools` or `toolChoice` are provided but `clientCapabilities.sampling.tools` is missing.
2. **Tool use/result balancing not enforced (MUST requirement)**: Add validation that every assistant message with `ToolUseContent` (id: X) is followed by a user message with `ToolResultContent` (toolUseId: X).
3. **Tool result content mixing not enforced**: Spec states "SamplingMessage with tool result content blocks MUST NOT contain other content types" - add validation for this.
4. **Role-content type mismatch possible**: `ToolUseContent` should only appear in assistant messages, `ToolResultContent` only in user messages. Currently not enforced - consider runtime validation.
5. **CreateMessageResult.role not constrained**: Spec says `role: "assistant"` (literal), but implementation allows any `Role`. Minor, but could be tightened.
6. **Dead code**: `AssistantMessageContent` and `UserMessageContent` enums in `content.rs` are defined but never used. Remove or use them.
The validations seem good to do as followups?
Closes #552
Motivation and Context
This PR implements SEP-1577: Sampling With Tools, enabling MCP servers to run agentic loops using the client's LLM while maintaining user supervision.
Key additions:
ToolChoice/ToolChoiceMode- Control tool selection behaviorToolUseContent/ToolResultContent- Tool calling content typesSamplingContent<T>- Single or array content wrapperSamplingMessageContent- Unified content enum withToolUseandToolResultvariantsSamplingCapability- Structured capability withtoolsandcontextsub-capabilitiesReference implementations:
How Has This Been Tested?
Breaking Changes
The type signature of
SamplingMessage.contentchanged fromContenttoSamplingContent<SamplingMessageContent>.Migration Made Easy
Convenience constructors (recommended):
Converting existing
Contentvalues:Note:
TryFromis used becauseContent::ResourceandContent::ResourceLinkvariants are not supported in sampling messages.Wire Format Compatibility
ClientCapabilities.sampling- Empty{}JSON still deserializes toSamplingCapabilityTypes of changes
Checklist
Additional context