-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Implement SEP-1577 - Sampling With Tools #1101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
commit: |
- Use structuredContent instead of content for plain object results in ToolResultContent tests - Use camelCase toolChoice instead of snake_case tool_choice in CreateMessageRequest test 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add type checks for ToolChoice, ToolUseContent, ToolResultContent, SamplingMessageContentBlock - Update expected spec types count from 119 to 123 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Changes:
- Remove `disable_parallel_tool_use` from ToolChoiceSchema (not in MCP spec)
- Remove unnecessary `.passthrough()` from ToolChoiceSchema
- Change CreateMessageResultSchema.role from z.literal("assistant") to
z.enum(["user", "assistant"]) to match spec's SamplingMessage.role
- Update spec type count from 123 to 127 (4 new sampling tool types)
- Fix test accessing .type on content union (could be array)
- Add test for CreateMessageResult with array content
- Remove test expecting user role to fail (spec allows both roles)
Note: 7 type compatibility errors remain due to upstream spec issue
where ToolUseContent.input and ToolResultContent.structuredContent use
`object` type instead of `{ [key: string]: unknown }`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
05f64f6 to
a39bd78
Compare
|
Hi @ochafik - I went ahead and rebased and reconciled a few schema updates ("user"|"assistant" type relaxation, etc.), hope that's ok 😬 Note: a handful of related errors remains due to a schema inconsistency that needs to be fixed in the spec before this will pass (due to spec.types.ts intake) - it's something I should've caught in my review, apologies. Below is Claude's (exhaustive) rundown of the situation, happy to talk through it if useful! Upstream Spec Fix Required: SEP-1577 Type InconsistencyProblemThe TypeScript SDK has 7 type compatibility errors between Root CauseIn Affected Fields
Why This MattersIn TypeScript:
These are not mutually assignable, causing cascade failures through all types that contain
Existing Precedent
// In schema/draft/schema.ts (existing, correct)
export interface CallToolResult extends Result {
structuredContent?: { [key: string]: unknown }; // ✓ correct
// ...
}Proposed FixIn // ToolUseContent - line ~1529
export interface ToolUseContent {
// ...
input: object; // ← change this
// ...
}
// ToolResultContent - line ~1568
export interface ToolResultContent {
// ...
structuredContent?: object; // ← change this
// ...
}To: // ToolUseContent
export interface ToolUseContent {
// ...
input: { [key: string]: unknown };
// ...
}
// ToolResultContent
export interface ToolResultContent {
// ...
structuredContent?: { [key: string]: unknown };
// ...
}Commit ReferenceThis inconsistency was introduced in commit After FixOnce the upstream spec is fixed:
|
|
Thanks @bhosmer-ant, sent modelcontextprotocol/modelcontextprotocol#1842 for review to fix the spec types. |
bhosmer-ant
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Implement types from modelcontextprotocol/modelcontextprotocol#1577 (accepted)
Spec PR: modelcontextprotocol/modelcontextprotocol#1796 (merged)
cc/ @bhosmer-ant
Follow up PRs may include more code from #991:
Motivation and Context
Allows using tools in sampling.
How Has This Been Tested?
#991 has examples + backfill support
Breaking Changes
Yes: any code that uses the result of sampling will now need to check whether
CreateMessageResult.contentis an array or not (if the server is on a previous spec version, the content returned cannot be an array, but the static types now allow both)Types of changes
Checklist
Additional context