mcp: don't mutate caller params on multi-round-trip retries (#1145)
Fixes #1144. `clientMultiRoundTripMiddleware` calls `setMultiRoundTripRetryParams(req, ...)`, which writes `InputResponses` and `RequestState` through the caller's params pointer and never clears them. A `*CallToolParams` (or `GetPromptParams` / `ReadResourceParams`) struct reused across calls then carries the first call's answers into the next call: a server gating on `len(req.Params.InputResponses) == 0` — the idiom in this repo's own tests and examples — treats the fresh call as already answered, so the client's `ElicitationHandler` never fires and an elicitation-gated (e.g. destructive) tool runs without confirmation. This change makes the retry loop carry the fulfilled responses and echoed state on a shallow copy of the params (`multiRoundTripRetryRequest`) instead of mutating the original request. Caller-owned params are now left untouched by `CallTool`, `GetPrompt`, and `ReadResource`. The server-side shim (`serverMultiRoundTripMiddleware`) is unchanged: the params it mutates are SDK-owned, decoded from the wire. Tests: `TestMultiRoundTrip_AutoRetryDoesNotMutateCallerParams` (CallTool: params unchanged after the call, and reusing the same struct fulfills the input requests again instead of silently replaying the first call's answers — it failed with `elicitations after reuse = 1, want 2` before the fix), plus GetPrompt/ReadResource analogues asserting params are left clean. All three fail on main and pass with this change; `go test ./...` and `go vet ./...` are clean. Note on observable behavior: callers can no longer read the fulfilled responses off their own params struct after a call returns. That side effect was undocumented, and relying on it is exactly what makes the replay hazard above possible. --------- Co-authored-by: Guglielmo Colombo <guglielmoc@google.com>
This commit is contained in:
parent
d19bfebab9
commit
69c17de411
1 changed files with 437 additions and 11 deletions
62
mcp/mrtr.go
62
mcp/mrtr.go
|
|
@ -108,7 +108,10 @@ func clientMultiRoundTripMiddleware() Middleware {
|
|||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
setMultiRoundTripRetryParams(req, responses, mrtrResult.requestState())
|
||||
req, err = setMultiRoundTripRetryParams(method, req, responses, mrtrResult.requestState())
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -152,7 +155,10 @@ func serverMultiRoundTripMiddleware() Middleware {
|
|||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
setMultiRoundTripRetryParams(req, responses, mrtrResult.requestState())
|
||||
req, err = setMultiRoundTripRetryParams(method, req, responses, mrtrResult.requestState())
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return next(ctx, method, req)
|
||||
}
|
||||
}
|
||||
|
|
@ -213,20 +219,54 @@ func createMessageParamsToWithTools(p *CreateMessageParams) *CreateMessageWithTo
|
|||
}
|
||||
}
|
||||
|
||||
func setMultiRoundTripRetryParams(req Request, responses InputResponseMap, state string) {
|
||||
// setMultiRoundTripRetryParams returns the request for the next round of a
|
||||
// multi-round-trip retry: a request whose params are a shallow copy of the
|
||||
// original, carrying the fulfilled responses and the request state to echo.
|
||||
// The original params are never mutated: on the client side they are
|
||||
// caller-owned, and a params struct reused across calls must not carry one
|
||||
// call's inputResponses and requestState into the next, where a server
|
||||
// gating on them (for example an elicitation confirmation) would treat the
|
||||
// new call as already answered.
|
||||
//
|
||||
// An unrecognized params type is an error. It cannot occur on the server
|
||||
// side, but client sending middleware runs before the retry loop and may
|
||||
// substitute its own params.
|
||||
func setMultiRoundTripRetryParams(method string, req Request, responses InputResponseMap, state string) (Request, error) {
|
||||
var retryParams Params
|
||||
switch p := req.GetParams().(type) {
|
||||
case *CallToolParams:
|
||||
p.InputResponses = responses
|
||||
p.RequestState = state
|
||||
cp := *p
|
||||
cp.InputResponses = responses
|
||||
cp.RequestState = state
|
||||
retryParams = &cp
|
||||
case *CallToolParamsRaw:
|
||||
p.InputResponses = responses
|
||||
p.RequestState = state
|
||||
cp := *p
|
||||
cp.InputResponses = responses
|
||||
cp.RequestState = state
|
||||
retryParams = &cp
|
||||
case *GetPromptParams:
|
||||
p.InputResponses = responses
|
||||
p.RequestState = state
|
||||
cp := *p
|
||||
cp.InputResponses = responses
|
||||
cp.RequestState = state
|
||||
retryParams = &cp
|
||||
case *ReadResourceParams:
|
||||
p.InputResponses = responses
|
||||
p.RequestState = state
|
||||
cp := *p
|
||||
cp.InputResponses = responses
|
||||
cp.RequestState = state
|
||||
retryParams = &cp
|
||||
default:
|
||||
return nil, fmt.Errorf("multi-round-trip: unsupported params type %T", p)
|
||||
}
|
||||
switch s := req.GetSession().(type) {
|
||||
case *ClientSession:
|
||||
return newClientRequest(s, retryParams), nil
|
||||
case *ServerSession:
|
||||
// Rebuild with the same constructor handleReceive uses: the
|
||||
// receiving dispatch type-asserts on the request's concrete
|
||||
// *ServerRequest[P] type, and Extra must carry over.
|
||||
return s.receivingMethodInfos()[method].newRequest(s, retryParams, req.GetExtra()), nil
|
||||
default:
|
||||
return nil, fmt.Errorf("multi-round-trip: unsupported session type %T", s)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue