design: mrtr implementation proposal#950
Conversation
yarolegovich
force-pushed
the
yarolegovich/mrtr
branch
from
8fa6e4f to
ec8b204
Compare
|
I think the main question we should ask ourselves is how we would design the API for this if there was no backwards compatibility requirement. If we have an approach that will likely be chosen for v2, sometimes it may make sense to move toward its direction, even if it has some disadvantages if they would disappear with v2 release. |
| ``` | ||
|
|
||
| `InputRequests` and `RequestState` fields are added directly to `CallToolResult`, `GetPromptResult`, and `ReadResourceResult` as exported. | ||
| Result type discriminator (completed, input_required) is unexported so that SDK users don't need to set it to the correct constant in addition to setting either `Content` or `InputRequests`. Handler execution result is validated and augmented before marshaling: |
There was a problem hiding this comment.
How would the error be propagated to the tool developer?
There was a problem hiding this comment.
That's a good question. My instinct was to treat it similarly to json.Marshal failure on the write path, but there's however an interesting discrepancy in how it's handled depending on where in the stack it occurs:
- Here it's returned as an
fmt.Errorf -> jsonrpc errorto clients. - Here it's just written to an error log through onInternalError handler. The result is not returned to clients.
(1) exposes some implementation details but is more "visible" and is likely harmless. My vote would be a combination - log an error and return a jsonrpc server error to clients.
|
|
||
| An unexported middleware is installed in the client, which similarly to `urlElicitationMiddleware` will automatically invoke handlers for the corresponding methods on incomplete results and retry the original request. `ClientOptions` will be extended with configuration knobs: | ||
| ```go | ||
| type MRTROptions struct { |
There was a problem hiding this comment.
I'm not sure I would introduce these knobs from the start. If the user need is real, someone will come to use and ask about it. We can always add them later in a backwards compatible manner.
There was a problem hiding this comment.
Don't object. We can start with a reasonable default of max retries and add configurability on request
|
|
||
| // RequireInput constructs a tool call, prompt or resource result with input requests set. | ||
| // mrtrResult provides methods for setting private fields on these types. | ||
| func RequireInput[T any, TP interface { *T; mrtrResult }](r InputRequiredResult) TP { ... } |
There was a problem hiding this comment.
We can always introduce this even with exported fields, as a "util" that guarantees correctness. But likely not from the start.
There was a problem hiding this comment.
imo at least with the current data model and resultType auto-populated the utility would be very shallow. I would not be considering it unless we get an inflow of issues where people try to set InputRequests together with Content or other fields
|
|
||
| ### Protocol version bridging | ||
|
|
||
| When a handler returns an input-required result to an old client the SDK could bridge by invoking `ServerSession.Elicit`/`CreateMessage`/`ListRoots` on the `ServerSession` and re-invoking the handler with the collected `inputResponses`: |
There was a problem hiding this comment.
I think this behavior would be valuable. Without it, the server creators are not incentivized to use the new MRTR format because migrating would mean they will stop working with all older clients. It would slow down the adoption of the new protocol version.
There was a problem hiding this comment.
The question is whether we should build it from the start or add later if there's a demand for it?
I don't think my proposal would change significantly if we didn't need backward compatibility. I would want a sealed interface return type for If we make the client API return a sealed interface as well the "base case" becomes overly verbose - all clients need to unpack result to a specific type and handle it accordingly, even though most of the time retry is auto-handled by the middleware. Now this breaks the proposed client side "manual handling" where if you disable MRTR through options you can just inspect |
2 participants
This PR presents a proposal for implementing Multi Round-Trip Requests (MRTR) as defined in SEP-2322.
In the new protocol version servers can't initiate requests to clients, but when a server requires additional input for completing
tools/call,prompts/get, orresources/readit can return an incomplete result along with a set ofinputRequests. The client fulfills them locally and retries the same call withinputResponsesattached.