feat(mcptoolset): surface elicitation and tool result _meta - #1168
QuentinBisson wants to merge 10 commits into
Conversation
karolpiotrowicz
left a comment
karolpiotrowicz
left a comment
There was a problem hiding this comment.
Thanks for this, @QuentinBisson! I know this is still a draft, but since it's linked to #1165 I took an early look — and it's already in really good shape, so I wanted to share some feedback now rather than make you wait. Appreciate the work you've put in here, especially the clean split into the two independent pieces (_meta passthrough + elicitation plumbing) and the in-memory-transport tests running real protocol traffic against a live server.
I pulled the branch and it holds up well: the full test suite passes, -race is clean on the package, and it's backward compatible (_meta is only added when present, and the two new Config fields are purely additive).
None of this is urgent given the draft status — just flagging it early so it's easy to fold in. One thing I'd want to resolve before it comes out of draft, plus a couple of doc clarifications and an optional design question:
1. ElicitationCompleteHandler set on its own advertises a capability the client can't service. New enters the capability branch on ElicitationHandler != nil || ElicitationCompleteHandler != nil, so setting only ElicitationCompleteHandler advertises form+URL elicitation while ElicitationHandler stays nil — and the SDK then rejects any incoming elicitation/create with "client does not support elicitation". Since a completion notification can't arrive unless an elicitation was created first, this combination is effectively unusable. I'd suggest failing fast in New (details inline).
2. Doc accuracy on _meta. The functionResponse comment says the metadata reaches "callbacks and the embedding application" — worth adding that it's also included in the function response returned to the model (and therefore persisted in session/traces). That's the intended behavior per #1165, but it's the most consequential consumer, so it's good to spell out.
3. (Optional / your call) Opt-in vs. always-on for _meta. Because the metadata now flows to the model for every server that emits it, users of meta-emitting servers will start seeing it surface without opting in. The reserved-key shape you chose is faithful to #1165 — just flagging the always-on aspect in case you'd prefer a flag or the OnToolResult hook that was also floated.
A few optional test/robustness follow-ups can wait until it's marked ready. Thanks again for driving this — happy to review again whenever you flip it out of draft.
| if cfg.ElicitationHandler != nil || cfg.ElicitationCompleteHandler != nil { | ||
| if cfg.Client != nil { | ||
| return nil, fmt.Errorf("mcptoolset: ElicitationHandler and ElicitationCompleteHandler cannot be combined with a custom Client; set them in the client's mcp.ClientOptions instead") | ||
| } |
There was a problem hiding this comment.
The capability branch is gated on either handler being set, but only ElicitationHandler can actually service an incoming elicitation/create. If a caller sets only ElicitationCompleteHandler, the client advertises form+URL elicitation yet the SDK rejects the request at runtime with "client does not support elicitation" — and a completion notification can't arrive without a prior elicitation anyway, so this config can't work.
Simplest fix is to fail fast in New, e.g. right after the custom-client check:
| } | |
| } | |
| if cfg.ElicitationHandler == nil { | |
| return nil, fmt.Errorf("mcptoolset: ElicitationCompleteHandler requires ElicitationHandler to be set") | |
| } |
(Alternatively, gate just the Capabilities.Elicitation advertisement on cfg.ElicitationHandler != nil — but since the complete-handler-only case is inert, erroring is clearer.)
| // functionResponse builds the function response map for a tool result. | ||
| // The result's _meta field is preserved under the "_meta" key, mirroring the | ||
| // raw MCP serialization, so that metadata attached by the server (e.g. auth | ||
| // challenges from MCP gateways) reaches callbacks and the embedding | ||
| // application instead of being silently dropped. |
There was a problem hiding this comment.
Small doc clarification: this map is the function response returned to the model, so _meta reaches the LLM (and is persisted to session/traces), not only "callbacks and the embedding application." Could you extend the comment to say so? It's the intended behavior per #1165, but the model being a consumer is the important part for anyone reading this later.
|
Thanks for the early look, @karolpiotrowicz, and for pulling the branch and running 1. 2. 3. Always-on vs opt-in for Both fixes are pushed. Marking ready for review. |
8231104 to
e3e14c6
Compare
karolpiotrowicz
left a comment
karolpiotrowicz
left a comment
There was a problem hiding this comment.
Both threads from the earlier round are properly resolved. The New guard is exactly the right shape and set_test.go:1069 pins it, the functionResponse doc now says plainly that the map reaches the model and is persisted to session and traces, and the _meta filtering added since is a real improvement over dropping the metadata wholesale. The second-label rule in isReservedMetaKey matches the 2026-07-28 spec exactly, com.example.mcp/ counter-example included, which is the part that is easy to get wrong.
One thing needs fixing before this goes in, and it comes from the reserved-tool-name check rather than from the _meta work.
A server-advertised tool name can take the whole agent down, and the operator cannot prevent it. The check at set.go:225-227 runs before the tool filter at set.go:234, so a ToolFilter that explicitly excludes the offending tool does not help. The error is not scoped to the toolset either: tools_processor.go:42-44 yields it and returns, which fails the whole invocation. So a remote MCP server that happens to expose a tool called transfer_to_agent disables every tool on the agent, with no configuration available to work around it.
// tool/mcptoolset/set_test.go
func TestReservedNameDefeatsToolFilter(t *testing.T) {
ts, err := mcptoolset.New(mcptoolset.Config{
Transport: srv(t, "get_weather", "transfer_to_agent"), // real mcp.Server, in-memory transport
ToolFilter: tool.StringPredicate([]string{"get_weather"}),
})
if err != nil {
t.Fatalf("New: %v", err)
}
inv := icontext.NewInvocationContext(t.Context(), icontext.InvocationContextParams{})
tools, err := ts.Tools(icontext.NewReadonlyContext(inv))
t.Logf("tools=%d err=%v", len(tools), err)
if err != nil {
t.Fatal("get_weather is unreachable even though transfer_to_agent was filtered out")
}
}$ go test -run TestReservedNameDefeatsToolFilter ./tool/mcptoolset/
tools=0 err=MCP server advertises tool "transfer_to_agent", a name reserved by the framework
get_weather is unreachable even though transfer_to_agent was filtered out
--- FAIL: TestReservedNameDefeatsToolFilter (0.00s)
Worth knowing while you decide how to handle it: on an agent that already has transfer targets, a server tool named transfer_to_agent was rejected before this change too, by the duplicate check in toolutils.go:47-49. The newly-broken cases are a single agent with no transfer targets meeting that name, and any agent meeting adk_request_credential or adk_request_confirmation. The first of those is the ordinary configuration, and it is also the one where there is nothing to hijack, so failing closed there costs availability without buying protection. Guarding the framework's dispatch names is worth doing, so this is about where the check sits rather than whether it should exist.
Two reserved-key cases slip through the _meta filter. The spec reserves four unprefixed keys — progressToken, traceparent, tracestate and baggage — as an explicit exception to the prefix rule, and tool.go:228-231 returns early for any key with no slash, so none of them are dropped. These are request-direction keys, so this bites when a server or gateway echoes them onto a result, and the effect is that protocol metadata lands in the function response and from there in the model's context and the session. Separately, tool.go:228 uses strings.LastIndex, but the spec terminates the prefix at the first slash and a key name may not contain one, so a key like io.modelcontextprotocol/a/b parses to a second label of modelcontextprotocol/a and is kept. That second one only admits malformed keys, so it matters much less than the first.
$ go test -run TestUnprefixedReservedKeys ./tool/mcptoolset/
isReservedMetaKey("progressToken") = false
isReservedMetaKey("traceparent") = false
isReservedMetaKey("tracestate") = false
isReservedMetaKey("baggage") = false
isReservedMetaKey("io.modelcontextprotocol/a/b") = false
isReservedMetaKey("io.modelcontextprotocol/serverInfo") = true
The table at meta_test.go:29-42 is otherwise a good regression net, and adding those cases to it would have caught both.
A successful result carrying only _meta still loses it. When the server returns no text content and no structured content, tool.go:160-162 returns a bare error, so the metadata reaches neither the response nor a ToolError. That line predates this PR, so it is not something the change broke, but a metadata-only auth challenge is close enough to the motivating case that it is worth deciding deliberately rather than leaving it as the one path that drops _meta.
A few smaller things, none of which need to block:
- The
ToolErrordoc could name the hook that can actually readMeta.tool.go:167-169is accurate as written, anderrors.Asdoes work in an error callback. It is just not obvious from the comment that the reachable caller isOnToolErrorCallbacksonllmagent.go:333, or thatbase_flow.go:1295-1297flattens the error to its message for the model, soMetais deliberately not model-visible the way success-path_metais. - The elicitation handler fields do not mention concurrency. One handler closure is shared by every tool on the toolset, and tool calls in a turn run on separate goroutines, so a handler can be entered concurrently. The docs at
set.go:157-182are detailed about the URL-mode blocking obligation, which makes the silence on concurrency read as "no obligation". RootsV2atset.go:78is deprecated as of 2026-07-28 (SEP-2577). The reason for setting it is sound and the field stays functional through the deprecation window, so this is only worth a comment noting why it is still used.ElicitationCompleteHandlerhas no happy-path test. The rejection case is covered, but nothing exercises both handlers set with a completion notification actually delivered, which is the behavior the field exists for.- The description is now missing some of the change. It does not mention the exported
ToolError, or thatToolscan fail on a reserved tool name, both of which are things a user of the package would want to find there.
The in-memory-transport tests running real protocol traffic against a live server continue to be the best part of this, and they are what made the _meta behavior straightforward to check.
|
Thank @karolpiotrowicz those are now addressed :) |
karolpiotrowicz
left a comment
karolpiotrowicz
left a comment
There was a problem hiding this comment.
All four items from the last round are properly fixed, and the fixes are the right shape rather than the minimum that would pass. The reserved-name check moved below the filter, the four unprefixed protocol keys and the first-slash prefix parse are both handled, the metadata-only result now surfaces instead of erroring, and the new test cases in meta_test.go are exactly the ones that would have caught the two _meta bugs. Standing up a raw JSON-RPC peer to exercise notifications/elicitation/complete because the SDK exposes no server API for it is more effort than this needed.
One new correctness bug, and two things worth knowing before merge.
The metadata-only branch keys on the wrong emptiness test. tool.go:160-167 enters the new branch when textResponse.Len() == 0, which is also true for a result whose content is non-text. So an image-only result that happens to carry server _meta now returns an empty output and reports success, while the identical result without _meta still returns an error. The image is dropped either way, so nothing new is lost, but the caller loses the signal that anything went wrong, and {"output": ""} is indistinguishable from a genuine empty string. Keying the branch on len(res.Content) == 0 restores the distinction.
$ go test -run TestMetaOnlyDivergence ./tool/mcptoolset/
image + unrelated _meta -> res=map[string]interface {}{"_meta":map[string]interface {}{"com.example/x":1}, "output":""} err=<nil>
image, no _meta -> res=map[string]interface {}(nil) err=no text content in tool response
The test that produces it
func TestMetaOnlyDivergence(t *testing.T) {
img := &mcp.ImageContent{Data: []byte{1, 2, 3}, MIMEType: "image/png"}
withMeta, err1 := runTool(t, func(ctx context.Context, r *mcp.CallToolRequest) (*mcp.CallToolResult, error) {
return &mcp.CallToolResult{Content: []mcp.Content{img}, Meta: mcp.Meta{"com.example/x": 1}}, nil
})
t.Logf("image + unrelated _meta -> res=%#v err=%v", withMeta, err1)
noMeta, err2 := runTool(t, func(ctx context.Context, r *mcp.CallToolRequest) (*mcp.CallToolResult, error) {
return &mcp.CallToolResult{Content: []mcp.Content{img}}, nil
})
t.Logf("image, no _meta -> res=%#v err=%v", noMeta, err2)
if err1 == nil && err2 != nil {
t.Errorf("an unrelated _meta key turns a hard error into a silent empty success")
}
}The new elicitation test passes with the feature disconnected. In elicitation_complete_test.go the raw server writes the elicitation request, then the completion notification, then the tools/call response, without ever reading the client's reply to the elicitation. The tool call therefore completes from the already-written response whether or not the completion handler ran. Deleting the ElicitationCompleteHandler line from the client options in set.go leaves go test -run TestElicitationCompleteHandler reporting ok, so the file does not currently guard the thing it was written for. Having the server block on reading the client's response to elicitID before answering the tools/call would make it real.
Two smaller things in the same file: the select on elicited has a default branch with no happens-before edge against the handler goroutine, so it can report "handler was never called" spuriously under load, and srv.elicitedURL() compares the server's own loginURL constant against itself while its message says it is checking what the handler received. Capturing req.Params.URL inside the handler, as TestElicitationHandler already does, would make that assertion mean what it says.
The deprecation notice points at an API that cannot express the exclusion. The comment at set.go:242 says excluding the tool by name keeps the rest of the toolset usable, and that is true for Config.ToolFilter. But that field is marked // Deprecated: use tool.FilterToolset instead, and filteredToolset.Tools calls the inner Tools() first and returns its error, so a caller who follows the deprecation gets no escape hatch. agentregistry constructs the toolset with no filter at all. This is much less serious than it looks, because set.Tools already fails the whole call on any ListTools error and always has, so a misbehaving server could do this before the change. It is worth resolving as an API-consistency matter rather than a vulnerability, and skipping the offending tool rather than erroring would close it for every caller at once.
A few smaller items, none of which need to block:
IsReservedToolNameis missing two names its own doc covers. It says "a function call name that the flow dispatches itself", butstop_streamingis intercepted before the tool lookup andtask_completedends a sequential-agent step, and neither is in the list.- There is no way to advertise form-only elicitation.
set.go:75-79always declares both modes, so a caller with a form-only handler will receive URL requests it cannot service and hit the retry behaviour the field doc describes. Worth a sentence in the doc if it is a deliberate simplification. - The concurrency note understates the sharing.
set.go:173-175scopes it to "a single turn", but one handler closure and one client session serve the toolset for the agent's whole lifetime, across every invocation. - The elicitation docs never say the URL is untrusted.
req.Params.URLarrives unprompted from the server and carries no scheme restriction, which matters because the natural implementation hands it to a browser. serverMetais computed twice on the metadata-only path, andunprefixedReservedMetaKeyswould read more naturally as aswitchinsideisReservedMetaKeythan as a package-level map.
The _meta filtering itself holds up well. It matches the second-label rule in the 2026-07-28 spec in both directions, including the com.example.mcp/ case that is easy to get backwards, and the malformed-key edges I could find (.mcp/x over-filtered, a..mcp/k under-filtered) are both unreachable for a conforming server and harmless either way, since a server wanting to reach the model can just use its own vendor prefix.
QuentinBisson
left a comment
QuentinBisson
left a comment
There was a problem hiding this comment.
Thanks, all three of the substantial items were real. Fixed in 898423a.
Metadata-only branch keyed on the wrong emptiness test. Correct, and the divergence was exactly as you reproduced it. The branch now keys on len(res.Content) == 0, so a non-text result fails with or without _meta. TestCallToolMeta gained a non-text result with server meta case and a non-text result without server meta case; reverting the fix makes the first one fail. serverMeta is now computed once, ahead of the error branch, and functionResponse takes the filtered map rather than the result.
The completion test passed with the feature disconnected. Also correct. The raw server now keeps the client's responses in a channel the serve loop does not consume, and callTool waits for the reply to the elicitation/create request before it answers the tools/call (with a bounded wait that answers a JSON-RPC error instead of hanging). Deleting ElicitationCompleteHandler from the client options now fails the test with the client never answered the elicitation, since the elicitation handler never returns. The select/default on the handler channel is gone: the handler pushes req.Params.URL and the assertion blocks on it, so it checks what the handler received rather than the server's own constant. srv.elicitedURL() is deleted.
Reserved names error out with no escape hatch. Taken as you suggest: Tools now logs and skips the offending tool instead of failing the whole call, which closes it for tool.FilterToolset and for agentregistry at the same time. TestToolsRejectsReservedToolName becomes TestToolsDropsReservedToolName, asserting the reserved tool is absent and the rest of the toolset is returned; the separate filter test is redundant now and is gone.
Smaller items:
IsReservedToolNamecoversstop_streamingandtask_completed, both behind new named constants thatbase_flow.gonow uses in place of the literals.Config.ElicitationHandlerdoc now says both modes are always advertised, that a form-only handler must go through a customClient'smcp.ClientOptions, thatParams.URLis untrusted and unrestricted in scheme, and that one handler and one session serve the toolset for its whole lifetime across every invocation, not for a single turn.unprefixedReservedMetaKeysis now aswitchinsideisReservedMetaKey.
go test -race -count=1 -shuffle=on ./tool/mcptoolset/ ./internal/llminternal/... ./agent/workflowagents/... is green, and golangci-lint run reports nothing new on the touched packages.
Add ElicitationHandler and ElicitationCompleteHandler to mcptoolset.Config, applied to the default MCP client so servers can use elicitation/create (including URL-mode elicitation per SEP-1036) during tool calls. The client declares both form and URL elicitation capabilities when a handler is set; combining the handlers with a custom Client is rejected since handlers must be configured on the client itself. Preserve the tool result _meta field in the function response map under the "_meta" key (mirroring raw MCP serialization) instead of dropping it, so server-attached metadata such as auth challenges reaches callbacks and the embedding application.
…rify _meta doc Fail fast in New when ElicitationCompleteHandler is set without ElicitationHandler: the client cannot service an elicitation/create and a completion notification cannot arrive without a prior elicitation, so the config is inert. Clarify that _meta in the function response reaches the model and is persisted to session and traces.
…rors Drop _meta keys in prefixes MCP reserves for itself (any prefix whose second label is "modelcontextprotocol" or "mcp") so protocol handshake metadata does not reach the model, and an absent "_meta" still means the server attached none of its own. Return ToolError from an errored tool result so metadata the server attached survives the error path. The error message is unchanged. Refuse a server tool whose name the flow dispatches itself, which would otherwise be invoked in place of the framework's own handling. Move the elicitation test to the InputRequests path, which protocol version 2026-07-28 requires in place of server-initiated elicitation/create while serving a request.
The reserved tool name check ran before the tool filter and failed the whole invocation, so a server tool named transfer_to_agent disabled every tool on the agent with no way to configure around it. It now runs after the filter. isReservedMetaKey missed the unprefixed protocol keys (progressToken, traceparent, tracestate, baggage) and split the prefix at the last slash instead of the first. A successful result carrying only _meta returned "no text content in tool response"; it now returns the metadata with an empty output. Also documents the concurrency contract of the elicitation handlers, the RootsV2 deprecation, and which callback can read ToolError.Meta, and adds a happy-path test for ElicitationCompleteHandler over a raw JSON-RPC server, since the SDK exports no server API for that notification.
…ools Keying the metadata-only branch on the assembled text length also matched a result whose content is non-text, so an image-only result carrying server _meta reported success with an empty output while the same result without _meta reported an error. An empty text response is now a success only when no non-text block was dropped, and the server metadata is computed once for every path. A tool whose name the framework dispatches itself is now dropped with a log line rather than failing the whole toolset: tool.FilterToolset returns the inner Tools() error, so a caller following the ToolFilter deprecation had no way to keep the rest of the toolset usable. IsReservedToolName also covers stop_streaming, which the live flow serves before the tool lookup, and task_completed, which sequentialagent injects. TestElicitationCompleteHandler passed with ElicitationCompleteHandler disconnected, because the raw server wrote the tools/call response without reading the client's reply to the elicitation. The server now waits for that reply, and the URL assertion reads what the handler received instead of the server's own constant.
898423a to
24b3259
Compare
|
Rebased onto main. One semantic note from the rebase: #1352 made an empty text result a success unless non-text blocks were dropped, so I took that guard and removed the separate metadata-only branch. A metadata-only result still surfaces with its |
karolpiotrowicz
left a comment
karolpiotrowicz
left a comment
There was a problem hiding this comment.
Round 1's findings are all closed. The reserved-tool-name check now runs after the operator's ToolFilter and drops just that tool instead of aborting the invocation, the four unprefixed reserved keys are matched, the prefix scan uses the first slash, and an empty-text success keeps its _meta. Thanks for recutting onto current main as well.
One new problem, in the rule that is the point of the feature. Two things need to be fixed before merge: the reserved-key rule has to match every label of the prefix except the last, and TestIsReservedMetaKey needs at least one row that fails under the rule as it stands.
isReservedMetaKey treats a key as reserved when the second label of its prefix is modelcontextprotocol or mcp. The specification's rule is that the marker may sit at any position that is not the last: "Any prefix beginning with zero or more valid labels, followed by modelcontextprotocol or mcp, followed by any valid label, is reserved for MCP use. For example: modelcontextprotocol.io/, mcp.dev/, api.modelcontextprotocol.org/, and tools.mcp.com/ are all reserved."
Two of those four examples fall through. Executed against the head:
modelcontextprotocol.io/x got=false want=true spec example 1
mcp.dev/x got=false want=true spec example 2
api.modelcontextprotocol.org/x got=true want=true spec example 3
tools.mcp.com/x got=true want=true spec example 4
a.b.modelcontextprotocol.c/x got=false want=true deeper prefix
a.b.mcp.d/x got=false want=true deeper prefix
A key that falls through is kept by serverMeta, set as response["_meta"], and travels out as the genai.FunctionResponse.Response — so it reaches the model and is persisted to the session event, which is what the filter exists to prevent.
What bounds this today: the Go SDK spells all five of its own reserved keys in reverse-DNS (io.modelcontextprotocol/serverInfo and siblings), and every one of those puts the marker at index 1, so nothing the SDK emits leaks. The exposure is to servers that use the specification's literal forms, which are equally valid.
Worth saying that you read the spec correctly in the harder direction: mcp/key and com.example.mcp/key both return false, and they should, because the marker has no following label. The rule is the mirror image of the right one rather than a careless one. Whatever shape the fix takes, it needs to keep that second property — a marker in the final position is still not reserved.
Two of the tests bless the defect rather than catching it, which is why it survived:
- Every
want: trueprefix row inTestIsReservedMetaKeyputs the marker at index 1, so the table is satisfied by the current rule and would stay green after a correct fix.tools.api.mcp.com/x,modelcontextprotocol.io/xandmcp.dev/xwould each discriminate. TestToolsDropsReservedToolNameconfigures noToolFilter, so moving the reserved check back in front of the filter — undoing the round-1 fix — leaves the suite green. A case with a filter that records the names it is offered would pin the ordering.
Smaller things:
- The "only non-text content" path returns a bare
fmt.Errorfand discards themetacomputed further up, which is asymmetric with theIsErrorpath you deliberately changed to carry it. A server attaching an auth challenge under its own prefix loses it exactly when the result is unrenderable. - The reserved-name drop is unconditional, but
transfer_to_agentis registered only when the agent has transfer targets andtask_completedonly bysequentialagentduringRunLive. A lonellmagentwith no sub-agents whose MCP server exposes a tool calledtransfer_to_agentloses a tool that works onmain, with only a log line. Either scope the check to names the framework actually registers for that invocation, or document the unconditional drop onConfig. _metaforwarding is on by default with no opt-out, so a server's own bookkeeping keys now reach the model and the session on every call. That is what the feature asks for, so I mention it rather than object.
CI has never run on this head — three check suites are sitting at action_required waiting on maintainer approval, so everything above comes from a local run.
Return UnsupportedContentError from the non-text-only path so the server metadata survives, matching the ToolError path. Document on Config that the reserved-tool-name drop is unconditional. Pin the names ToolFilter is offered, so the reserved-name check cannot move back in front of it, and add _meta rows that discriminate the second-label rule from a marker at any other position.
|
Three of the four are fixed. The blocking one rests on a superseded revision. The text you quote is 2025-06-18. It was replaced in 2025-11-25 and reads the same in 2026-07-28, the current revision:
That is what "Any label except the last" would un-reserve every key the SDK emits, since all six spell the marker last: Your three rows are Fixed:
|
karolpiotrowicz
left a comment
karolpiotrowicz
left a comment
There was a problem hiding this comment.
Nothing in the code blocks this, but the branch cannot be merged as it stands, and the reason needs a maintainer's decision rather than a rebase.
First, a correction I owe you. My last round called the reserved-_meta-prefix rule a bug, on the grounds that it missed modelcontextprotocol.io/ and mcp.dev/. That was wrong, and the four rows you added in this round's commit are right. The specification changed the rule between revisions: 2025-06-18 used a positional rule whose examples were the forward-DNS spellings, and both 2025-11-25 and 2026-07-28 replaced it with "any prefix where the second label is modelcontextprotocol or mcp", giving io.modelcontextprotocol/, dev.mcp/, org.modelcontextprotocol.api/ and com.mcp.tools/ as reserved and com.example.mcp/ as explicitly not. I was quoting the superseded revision. Your implementation matches the current one exactly, the pre-existing table rows are that revision's own example list, and the SDK you pin targets 2026-07-28. Sorry for the detour — the rows you added are a good regression guard and should stay.
The branch no longer merges, and neither obvious resolution compiles
#1401 landed on main 42 minutes after you pushed this round, so this is timing rather than anything you did. It replaced the "only non-text content" error with a renderer that formats every content type. On current main a tool returning only an image now yields {"output":"[MCP image: mimeType=\"image/png\", size=4 bytes]"} and no error, and droppedNonText no longer exists anywhere in the package.
The textual conflict in tool.go understates the problem, because main also added tool/mcptoolset/tool_test.go, which merges cleanly and so raises no conflict marker. I merged your head into main and tried both resolutions:
# keep this PR's tool.go
$ go vet ./tool/mcptoolset/
tool/mcptoolset/tool_test.go:713:14: undefined: hasMIMECharsetParameter
# keep main's tool.go
$ go vet ./tool/mcptoolset/
tool/mcptoolset/meta_test.go:59:13: undefined: isReservedMetaKey
tool/mcptoolset/meta_test.go:99:33: undefined: serverMeta
So neither side can simply win, and reconciling them is a design question rather than a merge chore: should a result carrying only non-text content fail loudly, as this PR does, or render a placeholder and report success, as main now does? Main's behaviour reports success while discarding the payload, which is exactly what your comment at tool.go:163-167 says it is there to prevent. Your own TODO(#1401) anticipated this, which is why I would rather you got an answer than guessed.
I would hold off reworking until a maintainer says which behaviour survives. If main's renderer stands, UnsupportedContentError and TestUnsupportedContentCarriesServerMeta have no code path left. If failing loudly stands, the renderer needs the guard back. Either way the _meta threading — serverMeta, ToolError, isReservedMetaKey — is orthogonal and survives, since main has none of it.
A scope question that is above both of us
This adds four exported symbols to tool/mcptoolset, and the reserved-_meta filter has no counterpart in adk-python: at its current main it returns the whole dumped result to the model (mcp_tool.py:636) and deliberately treats _meta as opaque server-owned JSON (mcp_tool.py:81-86). adk-go would be the only implementation that filters it.
I think filtering is the right instinct — a careless server can otherwise put arbitrary keys into the model's context and the persisted event. It is still a divergence from every sibling, so it wants a maintainer's sign-off before you spend more time on it. The elicitation half is straightforward parity, so that part is not in question (mcp_toolset.py:172).
Worth fixing whenever the rework happens
The godoc on the new type at tool.go:204-206 says the error means "every content block is of a type it does not convert", but the guard is textResponse.Len() == 0 && droppedNonText, so an empty TextContent alongside an ImageContent also triggers it even though that text block was converted. Your inline comment two dozen lines up already states the real rule correctly.
The causal clause in the Config doc at set.go:128-129 — "because a call to that name never reaches the tool" — holds only for stop_streaming, which the flow intercepts before the tool lookup. The other four reserved names resolve through toolsDict, so a server tool carrying one of them would have been reached. Dropping them is still right, and the sentence that follows already discloses the over-drop, so it is only the reason that needs rewording.
Three smaller ones, none blocking. The Meta field doc says keys are dropped when they sit "in prefixes the MCP protocol reserves", but four unprefixed keys are dropped too. The new type could carry ToolError's caveat that Meta never reaches the model, since the same is true of it. And one extra table row like {key: "net.mcp/hint", want: true} would pin the second-label rule itself rather than the four prefixes the spec happens to use as examples — I replaced the whole function body with a hardcoded list of those four prefixes and the package still passed.
Everything else is in good shape: build, vet, gofmt and the full suite are green, the API change is purely additive, and the filter-ordering assertion you added genuinely holds — inverting the order fails all five subtests.
|
|
||
| // UnsupportedContentError reports a tool result the toolset cannot render as a | ||
| // function response, because every content block is of a type it does not | ||
| // convert. Callers reach it with errors.As to read the metadata the server |
There was a problem hiding this comment.
This says the error means "every content block is of a type it does not convert", but the guard is textResponse.Len() == 0 && droppedNonText. A result of [TextContent{Text: ""}, ImageContent{...}] triggers it even though the text block was converted — it just contributed no text. The inline comment at lines 163-167 states the real rule correctly, so it is only this summary that is falsifiable.
No test covers that shape either: every row in the non-text table pairs a non-empty caption with the non-text block.
|
|
||
| // Meta holds the metadata the server attached to the result, without keys | ||
| // in prefixes the MCP protocol reserves for itself. It is nil when the | ||
| // server attached no metadata of its own. |
There was a problem hiding this comment.
"without keys in prefixes the MCP protocol reserves for itself" leaves out the four unprefixed keys isReservedMetaKey also drops — progressToken, traceparent, tracestate and baggage. A caller reading this would expect a bare traceparent to survive into Meta.
The same wording is on ToolError.Meta, so this is a two-site fix rather than something this commit introduced.
| // UnsupportedContentError reports a tool result the toolset cannot render as a | ||
| // function response, because every content block is of a type it does not | ||
| // convert. Callers reach it with errors.As to read the metadata the server | ||
| // attached to the result, which a plain error message cannot carry. |
There was a problem hiding this comment.
ToolError's doc just above spells out that the flow renders the error for the model as its message alone, so Meta never reaches the model. That is equally true here — both are returned as errors from Run, so neither goes through functionResponse — but only the older type says so. Worth copying the caveat across, since the natural reading of "callers reach it with errors.As to read the metadata" is that the metadata gets somewhere.
| // Config provides initial configuration for the MCP ToolSet. | ||
| // | ||
| // A server tool whose name the framework dispatches itself is dropped from the | ||
| // toolset and logged, because a call to that name never reaches the tool. The |
There was a problem hiding this comment.
"because a call to that name never reaches the tool" holds only for stop_streaming, which base_flow.go intercepts before the toolsDict lookup. transfer_to_agent, task_completed, adk_request_credential and adk_request_confirmation all resolve through that lookup, so with none of them registered a server tool carrying one of those names would have been reached and now is not.
The drop is still the right call and your next sentence already discloses the over-drop — it is the stated reason that is wrong, for four of the five names.
| {key: "modelcontextprotocol.io/key", want: false}, | ||
| {key: "mcp.dev/key", want: false}, | ||
| {key: "a.b.mcp.d/key", want: false}, | ||
| {key: "tools.api.mcp.com/key", want: false}, |
There was a problem hiding this comment.
These four rows are correct and worth keeping — they pin the current second-label rule against the superseded 2025-06-18 positional one, which is exactly the reading I got wrong last round.
One gap: every want: true prefix in the table is one of io.modelcontextprotocol/, dev.mcp/, org.modelcontextprotocol.api/ or com.mcp.tools/. I replaced the whole body of isReservedMetaKey with a hardcoded list of those four and the package still passed, even though the substitute wrongly forwards spec-reserved keys like net.mcp/hint and uk.modelcontextprotocol/x. A single row such as {key: "net.mcp/hint", want: true} closes it.
| var offered []string | ||
| ts, err := mcptoolset.New(mcptoolset.Config{ | ||
| Transport: clientTransport, | ||
| ToolFilter: func(ctx agent.ReadonlyContext, t tool.Tool) bool { |
There was a problem hiding this comment.
The closure parameter t tool.Tool shadows the subtest's t *testing.T from a few lines up. It compiles and does the right thing today because both types have a Name() method and you want the tool's — but a t.Errorf added inside this closure later would either target the wrong t or stop compiling. Renaming the parameter avoids that.
Dismissing this. The reserved-prefix finding it carries was based on a superseded revision of the MCP specification and has been withdrawn. Scope of the _meta filter is now with the maintainers.
|
@wolo-lab — flagging this one for a direction call before the author puts in more work. The elicitation half of this PR is parity with adk-python. The other half adds a reserved-key filter for One correction for the record. An earlier review here objected that the reserved-prefix rule missed the specification's own examples. That objection rested on the superseded 2025-06-18 revision of the MCP specification. The current revision reserves a prefix when its second label is Separately, the branch no longer merges. #1401 landed 42 minutes after this was pushed and the two touch the same file. Neither mechanical resolution compiles, so reconciling them is a design call as well. |
|
@karolpiotrowicz thanks for the correction on the Rather than wait on one direction call that covers two unrelated changes, would you prefer I split this into two PRs? 1. Elicitation only — On the scope question, this half is not a divergence from the siblings — it follows one. The same change shipped in adk-python as 2. Your own note that the |
…citation-meta Signed-off-by: QuentinBisson <quentin@giantswarm.io> # Conflicts: # tool/mcptoolset/tool.go
3 participants
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
_metais discarded, leaving no path for MCP server auth challenges (e.g. SEP-1036 URL elicitation) #1165Problem:
mcptoolsetflattens everyCallToolResultto{"output": ...}and drops the result's_metafield, and there is no way to handle elicitation requests: servers using URL-mode elicitation (SEP-1036, spec 2025-11-25) for out-of-band flows such as per-user OAuth challenges fail opaquely inside the toolset (client does not support "url" elicitation). go-sdk already ships the full client surface (ClientOptions.ElicitationHandler,ElicitationCompleteHandler,URLElicitationCapabilities), so this is purely mcptoolset plumbing.Solution:
Two independent pieces, as proposed in #1165:
Config.ElicitationHandlerandConfig.ElicitationCompleteHandler, applied to the default MCP client. Setting a handler declares both form and URL elicitation capabilities (the SDK's inferred capability covers form mode only;RootsV2preserves the default roots capability that settingCapabilitieswould otherwise disable). Combining the handlers with a customConfig.Clientreturns an error fromNew, since handlers must be configured in that client's ownmcp.ClientOptions._metapassthrough: when a tool result carriesMeta, it is included in the returned function response map under the"_meta"key, mirroring the raw MCP serialization. Results withoutMetaare unchanged.The
_metapassthrough drops the keys the MCP protocol reserves for itself (themodelcontextprotocol/mcpprefixes, plus the unprefixedprogressToken,traceparent,tracestateandbaggage), so only the server's own metadata reaches the model. A result that is marked as an error carries the same metadata on the exportedToolError, which anllmagent.Config.OnToolErrorCallbacksentry reads witherrors.As. A result with no content at all keeps its_metawith an empty output; a result whose content the toolset cannot render still fails, with or without_meta.Toolsdrops a server tool that advertises a name the framework dispatches itself, and logs it.IsReservedToolNamecoverstransfer_to_agent,adk_request_credential,adk_request_confirmation,stop_streaming(served by the live flow before the tool lookup) andtask_completed(injected bysequentialagent). Dropping rather than erroring is what keeps the rest of the toolset usable for every caller:tool.FilterToolsetreturns the innerTools()error, so a caller following theConfig.ToolFilterdeprecation has no escape hatch.I went with the reserved-key shape for
_metarather than anOnToolResulthook (the other option floated in #1165) because it is data-only and involves no new callback surface; happy to rework to the hook shape if preferred.Testing Plan
Unit Tests:
New tests in
tool/mcptoolset, all running real MCP protocol traffic over in-memory transports against a realmcp.Server:TestCallToolMeta: text, structured and content-free results with_metapreserved; no_metakey when the result has none; a non-text result renders and keeps its_meta.TestCallToolErrorCarriesServerMeta: an error result reaches the caller as a*ToolErrorcarrying the metadata.TestIsReservedMetaKey/TestServerMeta: reserved-key filtering, prefixed and unprefixed.TestElicitationHandler: server raises a URL-mode elicitation mid tool call; the configured handler receives the URL and the call completes.TestElicitationCompleteHandler: full URL-mode sequence. The server answers thetools/callonly after it reads the client's reply to the elicitation, and that reply arrives only once the completion handler releases the elicitation handler, so the test fails ifElicitationCompleteHandleris not wired. Its server is a raw JSON-RPC peer, because the SDK exports no server API fornotifications/elicitation/complete.TestToolsDropsReservedToolName: each reserved name is dropped and the rest of the toolset is returned.TestNewRejectsElicitationHandlerWithCustomClientandTestNewRejectsElicitationCompleteHandlerWithoutElicitationHandler: configuration errors fromNew.Manual End-to-End (E2E) Tests:
The elicitation test drives the full path end-to-end (real
mcp.Server→elicitation/createwithmode: "url"→ client handler → tool result), which is the same wire sequence an MCP gateway produces. We (Giant Swarm) also plan to run this against our MCP gateway (per-user OAuth to backends, challenges mapped to the A2Aauth-requiredtask state) and can report back on the PR.Checklist
Additional context
Python twin (elicitation callback only;
metaalready survives there): google/adk-python#6422 / google/adk-python#6423.