Wire MCP client options in mcptool.Connect: server sampling handler and list_changed notifications - #659
Conversation
bbbf9ac to
bb1224f
Compare
This comment has been minimized.
This comment has been minimized.
bb1224f to
56e00f6
Compare
This comment has been minimized.
This comment has been minimized.
Connect passed a nil *mcp.ClientOptions to mcp.NewClient, so no client handlers were registered and two go-sdk capabilities were dead: the client advertised no sampling capability and rejected every server-initiated sampling/createMessage, and it ignored notifications/tools/list_changed and notifications/prompts/list_changed. Build a non-nil ClientOptions from additive functional options. WithSampling registers CreateMessageHandler: deny-by-default when no approver is supplied (mirrors the Python client's sampling_callback policy), clamp the requested MaxTokens, translate the request messages and system prompt via the existing content mapper, invoke the caller's chat client, and return the reply as an assistant CreateMessageResult with an endTurn stop reason. WithToolListChanged and WithPromptListChanged forward the notifications to a caller callback so a static tool set can be refreshed mid-session. Connect keeps its previous behavior when called with no options.
56e00f6 to
b6a7afd
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR fixes mcptool.Connect constructing the MCP client with nil options by introducing functional ConnectOptions so client-side MCP handlers (sampling and list-changed notifications) can be registered when desired, while preserving existing behavior when called with no options.
Changes:
- Add
ConnectOptionplumbing and wire non-nil*mcp.ClientOptionsintomcp.NewClientfrommcptool.Connect. - Implement opt-in sampling support via
WithSampling, including deny-by-default approval gating and token clamping. - Add tool/prompt list-changed notification callbacks (
WithToolListChanged,WithPromptListChanged) and extend tests to cover sampling + tool list refresh.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tool/mcptool/mcp.go | Adds ConnectOptions, sampling handler wiring, and list-changed notification handlers to the MCP client constructed by Connect. |
| tool/mcptool/mcp_test.go | Extends the in-memory harness to pass ConnectOptions and adds black-box tests for sampling and tools list-changed behavior. |
| select { | ||
| case <-changed: | ||
| case <-time.After(2 * time.Second): | ||
| t.Fatal("tool list changed callback did not fire") | ||
| } |
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Generated by Go API Consistency Review Agent · sonnet46 · 41.4 AIC · ⌖ 5.8 AIC · ⊞ 5.7K
Comments that could not be inline-anchored
tool/mcptool/mcp.go:31
Parity gap — missing per-session request cap (Python sampling_max_requests)
WithSampling clamps the token budget per request but has no upper bound on how many sampling requests a server may issue per session connection. Python enforces this with sampling_max_requests (default 25), resetting the counter on reconnect. The comment in _mcp.py calls this out explicitly:
# - ``_DEFAULT_SAMPLING_MAX_REQUESTS`` caps the number of sampling requests per
# session connection (the co…
</details>
|
PratikDhanave (@PratikDhanave) needs a rebase |
…le with new MCP SDK) Upstream bumped the MCP go-sdk to protocol 2026-07-28, where SEP-2322 forbids a server from sending sampling/createMessage while serving a request (it must return an InputRequests map for the multi-round-trip flow). The WithSampling option relied on the old single-shot CreateMessageHandler, which no longer works. Drop the sampling option and its helpers/tests for now, keeping the working ConnectOption plumbing and the tool/prompt list-changed notification handlers. Sampling can be re-added against the SEP-2322 InputRequests flow as a follow-up.
github-actions
Bot
added
area:tool
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by Go API Consistency Review Agent · sonnet46 · 82.3 AIC · ⌖ 5.29 AIC · ⊞ 6K
| }, clientOptions) | ||
| return client.Connect(ctx, transport, nil) | ||
| } | ||
|
|
There was a problem hiding this comment.
Parity gap: WithSampling is absent from this PR but is claimed in the description and is needed to match upstream.
The PR description states that WithSampling(client, approve, maxTokens) is included in this change, but mcp.go contains no such exported function. Upstream Python wires sampling with three guardrails that all interact:
| Guardrail | Python default | Go (this PR) |
|---|---|---|
| Deny by default | ✅ sampling_approval_callback=None → deny |
missing |
| Token cap | ✅ sampling_max_tokens=4096 |
missing |
| Per-session request limit | ✅ sampling_max_requests=25 |
missing |
Python source: python/packages/core/agent_framework/_mcp.py – lines _DEFAULT_SAMPLING_MAX_TOKENS = 4096 and _DEFAULT_SAMPLING_MAX_REQUESTS = 25, and the deny logic at the top of the sampling callback.
The Go SamplingChatClient interface and stubSamplingChatClient test helper are added in mcp_test.go but no exported WithSampling option and no test that exercises sampling (deny-by-default or approve-all) is present in the production file. Until WithSampling is added with at minimum the deny-by-default guard, Go callers connecting to an MCP server that issues sampling/createMessage will receive an MCP-SDK-level error rather than the policy-controlled denial that Python and the PR description both promise.
| gotTexts []string | ||
| } | ||
|
|
||
| func (s *stubSamplingChatClient) GetResponse(_ context.Context, messages []*message.Message, maxTokens int64) (*message.Message, error) { |
There was a problem hiding this comment.
Dead code: stubSamplingChatClient is unused in this PR.
The stubSamplingChatClient type and its GetResponse method are added here but no test in this PR exercises them — there is no WithSampling option to wire, so the stub is never called. Either the sampling tests were accidentally omitted from this PR, or this helper should be removed to keep the test file clean. If WithSampling will be added in a follow-up, the stub should be added with it rather than here.
github-actions
Bot
added
failed-auto-risk
github-actions
Bot
added
pending-auto-risk
Parity Review: Cross-SDK ConsistencyPR scope: Adds three exported public APIs to
Note: The PR description mentions Upstream parity mapping
AssessmentThe two notification-handler options ( No parity issues found for the APIs actually introduced in this PR. The parity-approved label added.
|
github-actions
Bot
added
the
parity-approved
github-actions
Bot
added
risk:medium
What
mcptool.Connectconstructed the MCP client with a nil*mcp.ClientOptions, so no client-side handlers were ever registered. Two go-sdk capabilities were therefore dead:ClientOptions.CreateMessageHandlerauto-advertises the sampling capability when non-nil and dispatchessampling/createMessage. With nil options the client advertised no sampling and rejected every server-initiatedcreateMessage, so an MCP server that delegates sub-reasoning to the host model could not work.ClientOptions.ToolListChangedHandler/PromptListChangedHandlerare dispatched fornotifications/tools/list_changedandnotifications/prompts/list_changed, but with nil options those notifications were dropped, so a server adding/removing tools mid-session could not refresh the caller's static tool set.Both gaps live in the same
Connectconstruction site. This change builds a non-nilClientOptionsfrom additive functional options and keepsConnectbehaving exactly as before when called with no options:WithSampling(client, approve, maxTokens)setsCreateMessageHandler. It denies by default when no approver is supplied, clamps the server-requestedMaxTokens, translates the request messages andSystemPromptthrough the existingmcpContentToAgentContentmapper, invokes the caller's chat client, and returns the reply as aCreateMessageResult{Role: assistant, StopReason: endTurn}built with the existingagentContentToMCPContentmapper.WithToolListChanged(cb)/WithPromptListChanged(cb)set the notification handlers to invoke the caller callback (typically re-runningmcptool.ListTools).Why (cross-SDK parity)
Python wires
ClientSession(sampling_callback=...)and denies sampling by default unless the host explicitly opts in; .NET wires a samplingIChatClienton the MCP client. The Go port previously exposed neither, so host-in-the-loop sampling and dynamic tool/prompt refresh were unreachable. The deny-by-default policy here mirrors the Python client so a server cannot silently borrow the host model.Tests
Added to the canonical
tool/mcptool/mcp_test.go, black-box via the in-memory transport already used by the suite:CreateMessage; withWithSamplingand no approver the call returns an error and the stub chat client is never invoked.MaxTokensis clamped, the system prompt and user message are translated in order, and the reply is returned as an assistant/endTurn result.WithToolListChangedfires the callback afterserver.AddTool, and a subsequentListToolsreturns the newly added tool.go build ./...,go vet ./tool/mcptool/..., andgo test -race ./tool/mcptool/...pass.Open design questions
SamplingChatClientinterface (GetResponse(ctx, []*message.Message, maxTokens) (*message.Message, error)) rather than binding to a concrete provider, to keepmcptooldecoupled. Would a tie-in to the agent/provider chat-client type be preferred instead?ClientOptionsfields left as follow-ups. Prompts (Wire MCP prompts (ListPrompts/GetPrompt) through mcptool as promised by its package doc #628), approval mode (Support MCP approval mode on hostedtool.MCPServer via Responses require_approval #616), and name normalization (Normalize remote MCP tool names to a valid function-name pattern before surfacing to providers #611) are handled separately.CreateMessageResult.Contentcarries a single content block; the handler uses the first non-nil content of the reply. Should multi-content replies be coalesced differently?