Repository navigation
fix(server): validate sampling tool-use history - #3049
Adebayo120 wants to merge 3 commits into
Conversation
Reject sampling histories with assistant tool_use blocks that lack matching tool_result blocks in the next user message. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🦋 Changeset detectedLatest commit: 2d52e31 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
claude
Bot
added
the
v2
tlq5l
left a comment
tlq5l
left a comment
There was a problem hiding this comment.
Ran the server unit tests (577 pass), server.test.ts in test/integration (57 pass), scenarios/sampling.test.ts and coverage.test.ts in test/e2e, and check for server, integration and e2e; all clean. I also sent a few histories through createMessage over InMemoryTransport on main and on this branch: an interrupted tool_use in the middle followed by a valid pair, a trailing tool_use, and a tool_use in a user message are accepted on main and rejected here, while two valid rounds and text + tool_use in one assistant message are still accepted. That matches the balance rule in the 2025-11-25 sampling spec.
| const toolUses = content.filter((block): block is ToolUseContent => block.type === 'tool_use'); | ||
| const toolResults = content.filter((block): block is ToolResultContent => block.type === 'tool_result'); | ||
|
|
||
| if (toolUses.length > 0 && message.role !== 'assistant') { |
There was a problem hiding this comment.
These two role checks are new rejections that the description and changeset don't mention, and neither error message is asserted anywhere in the tests. Could you add a createMessage validation case for each (e.g. [user tool_use, user tool_result] and [user text, assistant tool_result]) and a sentence in the changeset?
There was a problem hiding this comment.
Added separate createMessage validation tests for tool_use in a user message and tool_result in an assistant message, and updated the changeset to mention both rejections. The server integration tests pass (59/59). Thanks for catching this.
Document and test the sampling message role checks raised in review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This branch has not been deployed
2 participants
While integrating sampling, I noticed the server would forward a history containing an assistant
tool_usewithout its matching usertool_result.Reject these incomplete histories before sending the sampling request, and add regression coverage for missing or interrupted results.