-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(core): RequiresInput catch in Dispatcher.dispatch -> IncompleteResult; MRTR types (SEP-2322) #2062
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: fweinberger/s2-ext-slot
Are you sure you want to change the base?
feat(core): RequiresInput catch in Dispatcher.dispatch -> IncompleteResult; MRTR types (SEP-2322) #2062
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,8 @@ | ||
| import { SdkError, SdkErrorCode } from '../errors/sdkErrors.js'; | ||
| import type { | ||
| IncompleteResult, | ||
| InputRequest, | ||
| InputResponseRequestParams, | ||
| JSONRPCErrorResponse, | ||
| JSONRPCNotification, | ||
| JSONRPCRequest, | ||
|
|
@@ -46,6 +49,33 @@ export type DispatchFn = (req: JSONRPCRequest, env?: RequestEnv) => AsyncGenerat | |
| */ | ||
| export type DispatchMiddleware = (next: DispatchFn) => DispatchFn; | ||
|
|
||
| /** | ||
| * Thrown by a handler (typically via `ctx.mcpReq.send` when no backchannel is available) | ||
| * to signal that the request cannot complete without client input. {@linkcode Dispatcher.dispatch} | ||
| * catches this and yields a successful {@linkcode IncompleteResult} response (SEP-2322 | ||
| * Option E ephemeral path). The client services the {@linkcode RequiresInput.inputRequests} | ||
| * and retries with `params.inputResponses`. | ||
| */ | ||
| export class RequiresInput extends Error { | ||
| constructor( | ||
| readonly inputRequests: Record<string, InputRequest>, | ||
| readonly requestState?: string | ||
| ) { | ||
| // eslint-disable-next-line unicorn/no-array-sort -- toSorted() requires ES2023 lib; consumers may target ES2022 | ||
| super(`Client input required: ${Object.keys(inputRequests).sort().join(', ')}`); | ||
| this.name = 'RequiresInput'; | ||
| } | ||
|
|
||
| /** Convert to the wire result {@linkcode Dispatcher.dispatch} yields. */ | ||
| toIncompleteResult(): IncompleteResult { | ||
| return { | ||
| resultType: 'incomplete', | ||
| inputRequests: this.inputRequests, | ||
| ...(this.requestState !== undefined && { requestState: this.requestState }) | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Derives the handler return type for the 3-arg `setRequestHandler` form from its | ||
| * `result` schema, defaulting to {@linkcode Result} when no schema is supplied. | ||
|
|
@@ -158,13 +188,18 @@ export class Dispatcher<ContextT extends BaseContext = BaseContext> { | |
| throw new SdkError(SdkErrorCode.NotConnected, 'No outbound channel: ctx.mcpReq.send requires a connected peer'); | ||
| }); | ||
|
|
||
| // SEP-2322: lift inputResponses/requestState off the params if this is a retry round. | ||
| const mrtrParams = request.params as InputResponseRequestParams | undefined; | ||
|
Comment on lines
+191
to
+192
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 nit: Extended reasoning...What the cast does
const mrtrParams = request.params as InputResponseRequestParams | undefined;
// ...
inputResponses: mrtrParams?.inputResponses,
requestState: mrtrParams?.requestState,
Why upstream parsing doesn't cover itInbound messages do go through Step-by-step proof
This is exactly the 'no unsafe Codebase precedentThe repo already handles the analogous envelope extension differently: Impact and severityNit, not blocking. Any TypeError thrown in a handler is caught by Suggested fixEither (a) add inputResponses:
mrtrParams?.inputResponses != null && typeof mrtrParams.inputResponses === 'object'
? mrtrParams.inputResponses
: undefined,
requestState: typeof mrtrParams?.requestState === 'string' ? mrtrParams.requestState : undefined,Option (a) is more consistent with how |
||
|
|
||
| const base: BaseContext = { | ||
| sessionId: env.sessionId, | ||
| mcpReq: { | ||
| id: request.id, | ||
| method: request.method, | ||
| _meta: request.params?._meta, | ||
| signal: localAbort.signal, | ||
| inputResponses: mrtrParams?.inputResponses, | ||
| requestState: mrtrParams?.requestState, | ||
| send: (async (r: Request, schemaOrOptions?: unknown, maybeOptions?: RequestOptions) => { | ||
| const isSchema = schemaOrOptions != null && typeof schemaOrOptions === 'object' && '~standard' in schemaOrOptions; | ||
| const options = isSchema ? maybeOptions : (schemaOrOptions as RequestOptions | undefined); | ||
|
|
@@ -205,9 +240,16 @@ export class Dispatcher<ContextT extends BaseContext = BaseContext> { | |
| : { jsonrpc: '2.0', id: request.id, result }; | ||
| }, | ||
| error => { | ||
| final = localAbort.signal.aborted | ||
| ? errorResponse(request.id, ProtocolErrorCode.InternalError, 'Request cancelled').message | ||
| : toErrorResponse(request.id, error); | ||
| if (localAbort.signal.aborted) { | ||
| final = errorResponse(request.id, ProtocolErrorCode.InternalError, 'Request cancelled').message; | ||
| } else if (error instanceof RequiresInput) { | ||
| // SEP-2322 Option E: a handler that needs client input throws RequiresInput | ||
| // (typically via ctx.mcpReq.send with no backchannel). This is a *successful* | ||
| // result discriminated by resultType:'incomplete', not an error response. | ||
| final = { jsonrpc: '2.0', id: request.id, result: error.toIncompleteResult() }; | ||
| } else { | ||
| final = toErrorResponse(request.id, error); | ||
| } | ||
| } | ||
| ) | ||
| .finally(() => { | ||
|
|
@@ -289,7 +331,11 @@ export class Dispatcher<ContextT extends BaseContext = BaseContext> { | |
| const handler = maybeHandler as (params: unknown, ctx: ContextT) => Result | Promise<Result>; | ||
| stored = async (request, ctx) => { | ||
| const userParams = { ...((request.params ?? {}) as Record<string, unknown>) }; | ||
| // Protocol-envelope fields are stripped before user-schema validation so a strict | ||
| // schema does not reject SEP-2322 retry rounds. | ||
| delete userParams._meta; | ||
| delete userParams.inputResponses; | ||
| delete userParams.requestState; | ||
| const parsed = await validateStandardSchema(schemas.params, userParams); | ||
| if (!parsed.success) { | ||
| throw new ProtocolError(ProtocolErrorCode.InvalidParams, `Invalid params for ${method}: ${parsed.error}`); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 nit: this JSDoc (and the
IncompleteResultJSDoc in types.ts, and the inline catch comment below) saysctx.mcpReq.sendwith no backchannel throwsRequiresInput/emitsIncompleteResult, but the defaultsendfallback at L186-189 still throwsSdkError(NotConnected)→ JSON-RPC error response (the existing test 'ctx.mcpReq.send with no env.send throws SdkError(NotConnected)' confirms this, as does theRequestEnv.sendJSDoc in context.ts). That behavior depends on the not-yet-landed F4 ContinuationCompat adapter. SinceRequiresInput/IncompleteResultare new public exports, suggest dropping or qualifying the parenthetical (e.g. "when a continuation adapter suppliesenv.send") until F4 lands.Extended reasoning...
What the docs claim vs. what ships
This PR adds three pieces of prose that all claim
ctx.mcpReq.sendwith no backchannel produces aRequiresInput→IncompleteResult:RequiresInputclass JSDoc (dispatcher.ts:52-58): "Thrown by a handler (typically viactx.mcpReq.sendwhen no backchannel is available)"IncompleteResultJSDoc (types.ts:206-213): "throwing RequiresInput (or callingctx.mcpReq.sendwith no backchannel) emits it"But the actual default
sendfallback in_dispatchCore(dispatcher.ts:186-189) is unchanged by this PR — whenenv.sendis undefined it throwsnew SdkError(SdkErrorCode.NotConnected, 'No outbound channel: ...'). ThatSdkErroris not aRequiresInputinstance, so it falls through the new catch branch intotoErrorResponse(request.id, error)and yields a JSON-RPC error response, not anIncompleteResult.Confirmed by the unchanged test
The existing test
'ctx.mcpReq.send with no env.send throws SdkError(NotConnected)'(dispatcher.test.ts:91-107) is left intact by this diff and still asserts the actual behavior: the handler catches anSdkErrorwithcode === SdkErrorCode.NotConnected, and the dispatch output is aJSONRPCErrorResponsewhoseerror.messagematches/No outbound channel/. TheRequestEnv.sendJSDoc in context.ts likewise still says "When undefined,ctx.mcpReq.sendthrowsSdkErrorCode.NotConnected" — so the package now contains two contradictory JSDoc descriptions of the same code path.Step-by-step proof
Dispatcherand register a handler that callsawait ctx.mcpReq.send({ method: 'elicitation/create', params: {} }).d.dispatch(req)with noenv(soenv.sendisundefined)._dispatchCoreassignssend = env.send ?? (async () => { throw new SdkError(SdkErrorCode.NotConnected, ...) })(L186-189).send(...), which rejects withSdkError(NotConnected). The handler's promise rejects with that error..then(..., error => ...)arm,localAbort.signal.abortedis false anderror instanceof RequiresInputis false (it's anSdkError), so control falls tofinal = toErrorResponse(request.id, error).{ kind: 'response', message: { jsonrpc: '2.0', id, error: { code: ..., message: 'No outbound channel: ...' } } }— a JSON-RPC error, not{ result: { resultType: 'incomplete', ... } }.A grep of
packages/confirms no shippedenv.sendimplementation throwsRequiresInput: it's referenced only in dispatcher.ts (definition + catch), types.ts (JSDoc link), the test (which throws it manually from a handler body), and the public export.BackchannelCompat(landed in 3eebc7f) does not reference it, and shttpHandler.ts:63 still documents "ctx.mcpReq.send rejects with NotConnected on this path".Why it matters
RequiresInputandIncompleteResultare newly added to the public export surface (exports/public/index.ts:56 and theexport *of types.ts), so their JSDoc is consumer-facing. Per REVIEW.md § Documentation & Changesets — "prose that promises behavior the code no longer ships misleads consumers... Flag any claim the diff doesn't back" — this is exactly the recurring catch. The PR description itself states that F4 (ContinuationCompat) is the future PR that will wireenv.sendto throwRequiresInput; until that lands, the parenthetical describes behavior this commit does not implement.Suggested fix
Either drop the "via
ctx.mcpReq.sendwith no backchannel" parenthetical from all three locations until F4 lands, or qualify it to make the dependency explicit, e.g. "Thrown by a handler (or by a ContinuationCompat-suppliedenv.sendwhen no backchannel is available)". Nit severity since it's docs-only and the stacked series will resolve it shortly.