Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/core/src/exports/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ export type {
} from '../../shared/protocol.js';
export { DEFAULT_REQUEST_TIMEOUT_MSEC } from '../../shared/protocol.js';

// SEP-2322 multi-round-trip request types (IncompleteResult/InputRequest/InputResponseRequestParams
// are exported via the `export * from types/types.js` below)
export { RequiresInput } from '../../shared/dispatcher.js';

// Task manager types (NOT TaskManager class itself — internal)
export type { RequestTaskStore, TaskContext, TaskManagerOptions, TaskRequestOptions } from '../../shared/taskManager.js';

Expand Down
13 changes: 13 additions & 0 deletions packages/core/src/shared/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,19 @@ export type BaseContext = {
* This is used by certain transports to correctly associate related messages.
*/
notify: (notification: Notification) => Promise<void>;

/**
* SEP-2322: client-supplied answers to a prior round's
* {@linkcode IncompleteResult.inputRequests}, keyed by the same opaque ids.
* Populated from `request.params.inputResponses` when present.
*/
inputResponses?: Record<string, Result>;

/**
* SEP-2322: opaque continuation token echoed from a prior round's
* {@linkcode IncompleteResult.requestState}.
*/
requestState?: string;
};

/**
Expand Down
52 changes: 49 additions & 3 deletions packages/core/src/shared/dispatcher.ts
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,
Expand Down Expand Up @@ -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`.
*/
Comment on lines +52 to +58
Copy link
Copy Markdown

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 IncompleteResult JSDoc in types.ts, and the inline catch comment below) says ctx.mcpReq.send with no backchannel throws RequiresInput/emits IncompleteResult, but the default send fallback at L186-189 still throws SdkError(NotConnected) → JSON-RPC error response (the existing test 'ctx.mcpReq.send with no env.send throws SdkError(NotConnected)' confirms this, as does the RequestEnv.send JSDoc in context.ts). That behavior depends on the not-yet-landed F4 ContinuationCompat adapter. Since RequiresInput/IncompleteResult are new public exports, suggest dropping or qualifying the parenthetical (e.g. "when a continuation adapter supplies env.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.send with no backchannel produces a RequiresInputIncompleteResult:

  1. The RequiresInput class JSDoc (dispatcher.ts:52-58): "Thrown by a handler (typically via ctx.mcpReq.send when no backchannel is available)"
  2. The inline catch comment (dispatcher.ts:246-248): "throws RequiresInput (typically via ctx.mcpReq.send with no backchannel)"
  3. The IncompleteResult JSDoc (types.ts:206-213): "throwing RequiresInput (or calling ctx.mcpReq.send with no backchannel) emits it"

But the actual default send fallback in _dispatchCore (dispatcher.ts:186-189) is unchanged by this PR — when env.send is undefined it throws new SdkError(SdkErrorCode.NotConnected, 'No outbound channel: ...'). That SdkError is not a RequiresInput instance, so it falls through the new catch branch into toErrorResponse(request.id, error) and yields a JSON-RPC error response, not an IncompleteResult.

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 an SdkError with code === SdkErrorCode.NotConnected, and the dispatch output is a JSONRPCErrorResponse whose error.message matches /No outbound channel/. The RequestEnv.send JSDoc in context.ts likewise still says "When undefined, ctx.mcpReq.send throws SdkErrorCode.NotConnected" — so the package now contains two contradictory JSDoc descriptions of the same code path.

Step-by-step proof

  1. Construct a Dispatcher and register a handler that calls await ctx.mcpReq.send({ method: 'elicitation/create', params: {} }).
  2. Call d.dispatch(req) with no env (so env.send is undefined).
  3. _dispatchCore assigns send = env.send ?? (async () => { throw new SdkError(SdkErrorCode.NotConnected, ...) }) (L186-189).
  4. The handler awaits send(...), which rejects with SdkError(NotConnected). The handler's promise rejects with that error.
  5. In the .then(..., error => ...) arm, localAbort.signal.aborted is false and error instanceof RequiresInput is false (it's an SdkError), so control falls to final = toErrorResponse(request.id, error).
  6. The generator yields { 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 shipped env.send implementation throws RequiresInput: 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

RequiresInput and IncompleteResult are newly added to the public export surface (exports/public/index.ts:56 and the export * 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 wire env.send to throw RequiresInput; until that lands, the parenthetical describes behavior this commit does not implement.

Suggested fix

Either drop the "via ctx.mcpReq.send with 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-supplied env.send when no backchannel is available)". Nit severity since it's docs-only and the stacked series will resolve it shortly.

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.
Expand Down Expand Up @@ -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
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit: request.params as InputResponseRequestParams launders unvalidated wire data into the strongly-typed ctx.mcpReq.inputResponses (Record<string, Result>) / ctx.mcpReq.requestState (string) fields — BaseRequestParamsSchema.loose() validates _meta but passes these keys through as unknown, so a client sending requestState: 123 lands a number in a string-typed public context field. REVIEW.md § Correctness flags 'no unsafe as assertions'; consider adding these fields to BaseRequestParamsSchema (matching the _meta / TaskAugmentedRequestParamsSchema precedent) or guarding with typeof before assignment. Impact is contained — any handler TypeError is caught and wrapped as a JSON-RPC error — so not blocking.

Extended reasoning...

What the cast does

_dispatchCore reads:

const mrtrParams = request.params as InputResponseRequestParams | undefined;
// ...
inputResponses: mrtrParams?.inputResponses,
requestState: mrtrParams?.requestState,

request is a JSONRPCRequest, whose params is Infer<typeof BaseRequestParamsSchema.loose()> — i.e. { _meta?: RequestMeta } & Record<string, unknown>. The as InputResponseRequestParams cast reinterprets that loose record as { inputResponses?: Record<string, Result>; requestState?: string } and the values are then assigned directly onto BaseContext.mcpReq, where context.ts declares them with those concrete types. There is no runtime check between the wire bytes and the typed public field.

Why upstream parsing doesn't cover it

Inbound messages do go through JSONRPCMessageSchema.parse() (via deserializeMessage / parseJSONRPCMessage), and JSONRPCRequestSchema spreads RequestSchema.shape, where params is BaseRequestParamsSchema.loose().optional() (schemas.ts:95). That schema explicitly defines _meta: RequestMetaSchema.optional(), so request.params._meta is Zod-validated at the wire boundary — a client sending _meta: 123 is rejected before reaching the dispatcher. But inputResponses and requestState are not in BaseRequestParamsSchema; .loose() admits them as catchall keys with runtime type unknown. So unlike _meta on the line above, these two assignments are backed by nothing but the cast.

Step-by-step proof

  1. Client sends {"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"t","arguments":{},"requestState":123,"inputResponses":"oops"}}.
  2. JSONRPCMessageSchema.parse() succeeds: params matches BaseRequestParamsSchema.loose() (no _meta violation; extra keys pass through unvalidated).
  3. _dispatchCore runs const mrtrParams = request.params as InputResponseRequestParams; mrtrParams.requestState is the number 123, mrtrParams.inputResponses is the string "oops".
  4. These are assigned to base.mcpReq.requestState / base.mcpReq.inputResponses, which TypeScript believes are string | undefined and Record<string, Result> | undefined.
  5. A handler that does ctx.mcpReq.requestState?.slice(0, 8) typechecks cleanly but throws TypeError: ctx.mcpReq.requestState.slice is not a function at runtime; likewise Object.entries(ctx.mcpReq.inputResponses ?? {}) would iterate string indices.

This is exactly the 'no unsafe as assertions' item in REVIEW.md § Correctness — a cast at a trust boundary that lies about the runtime type of wire data.

Codebase precedent

The repo already handles the analogous envelope extension differently: task is added to BaseRequestParamsSchema via TaskAugmentedRequestParamsSchema and read through the Zod-backed guard isTaskAugmentedRequestParams (guards.ts:90), not via a raw as. _meta is in the schema itself. The new inputResponses/requestState are the only envelope fields that reach BaseContext purely through a cast.

Impact and severity

Nit, not blocking. Any TypeError thrown in a handler is caught by _dispatchCore's .then(_, error => toErrorResponse(...)) arm and becomes a JSON-RPC InternalError response back to the same misbehaving client — no process crash, no cross-request corruption, no escalation. requestState is documented as an opaque token the server itself originated, and the primary consumer (F4 ContinuationCompat, not yet landed) will have to treat it as untrusted/tamperable regardless. The dispatcher itself never operates on these values; it only passes them through.

Suggested fix

Either (a) add inputResponses/requestState to BaseRequestParamsSchema so Zod validates them at the wire-parse boundary alongside _meta, or (b) add cheap inline guards before assignment:

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 _meta and task are handled and keeps the validation in one place.


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);
Expand Down Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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}`);
Expand Down
37 changes: 37 additions & 0 deletions packages/core/src/types/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,43 @@ export type TaskAugmentedRequestParams = Infer<typeof TaskAugmentedRequestParams
export type RequestMeta = Infer<typeof RequestMetaSchema>;
export type Notification = Infer<typeof NotificationSchema>;
export type Result = Infer<typeof ResultSchema>;

/**
* A single server-to-client request the handler needs answered before it can finish.
* Carried in {@linkcode IncompleteResult.inputRequests}, keyed by an opaque id the client
* echoes back in {@linkcode InputResponseRequestParams.inputResponses}. SEP-2322.
*/
export interface InputRequest {
method: string;
params?: Record<string, unknown>;
}

/**
* SEP-2322 multi-round-trip result discriminator. The server returns this when it cannot
* complete without client input (sampling, elicitation). The client services
* {@linkcode IncompleteResult.inputRequests | inputRequests} and retries the same request
* with {@linkcode InputResponseRequestParams.inputResponses | inputResponses}. Handlers
* normally do not return this directly; throwing {@linkcode RequiresInput}
* (or calling `ctx.mcpReq.send` with no backchannel) emits it.
*/
export interface IncompleteResult extends Result {
resultType: 'incomplete';
/** Server-initiated requests the client must answer, keyed by an opaque id. */
inputRequests?: Record<string, InputRequest>;
/** Opaque continuation token the client echoes back unchanged. */
requestState?: string;
}

/**
* Params shape for a SEP-2322 retry: the client adds `inputResponses` (keyed by the ids
* from {@linkcode IncompleteResult.inputRequests}) and echoes `requestState` alongside
* the original method-specific params.
*/
export interface InputResponseRequestParams {
inputResponses?: Record<string, Result>;
requestState?: string;
}

export type RequestId = Infer<typeof RequestIdSchema>;
export type JSONRPCRequest = Infer<typeof JSONRPCRequestSchema>;
export type JSONRPCNotification = Infer<typeof JSONRPCNotificationSchema>;
Expand Down
33 changes: 31 additions & 2 deletions packages/core/test/shared/dispatcher.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ import { z } from 'zod/v4';

import { SdkError, SdkErrorCode } from '../../src/errors/sdkErrors.js';
import type { DispatchOutput } from '../../src/shared/dispatcher.js';
import { Dispatcher } from '../../src/shared/dispatcher.js';
import type { JSONRPCErrorResponse, JSONRPCRequest, JSONRPCResultResponse, Result } from '../../src/types/index.js';
import { Dispatcher, RequiresInput } from '../../src/shared/dispatcher.js';
import type { IncompleteResult, JSONRPCErrorResponse, JSONRPCRequest, JSONRPCResultResponse, Result } from '../../src/types/index.js';
import { ProtocolError, ProtocolErrorCode } from '../../src/types/index.js';

const req = (method: string, params?: Record<string, unknown>, id = 1): JSONRPCRequest => ({ jsonrpc: '2.0', id, method, params });
Expand Down Expand Up @@ -320,3 +320,32 @@ describe('Dispatcher.setRequestHandler 3-arg (custom method + {params, result})'
expect(() => setNotif('acme/ping', () => {})).toThrow(/not a spec notification method/);
});
});

describe('RequiresInput → IncompleteResult (SEP-2322 Option E)', () => {
test('handler throwing RequiresInput yields a successful IncompleteResult, not an error', async () => {
const d = new Dispatcher();
d.setRequestHandler('ping', async () => {
throw new RequiresInput({ r0: { method: 'elicitation/create', params: {} } }, 'state-token-1');
});
const out = await collect(d.dispatch(req('ping')));
expect(out).toHaveLength(1);
expect(out[0]!.kind).toBe('response');
const msg = out[0]!.message as JSONRPCResultResponse;
expect('result' in msg).toBe(true);
const result = msg.result as IncompleteResult;
expect(result.resultType).toBe('incomplete');
expect(result.inputRequests).toEqual({ r0: { method: 'elicitation/create', params: {} } });
expect(result.requestState).toBe('state-token-1');
});

test('inputResponses/requestState are lifted onto ctx.mcpReq', async () => {
const d = new Dispatcher();
let seen: { inputResponses?: unknown; requestState?: unknown } = {};
d.setRequestHandler('ping', async (_r, ctx) => {
seen = { inputResponses: ctx.mcpReq.inputResponses, requestState: ctx.mcpReq.requestState };
return {};
});
await collect(d.dispatch(req('ping', { inputResponses: { r0: { ok: true } }, requestState: 's1' })));
expect(seen).toEqual({ inputResponses: { r0: { ok: true } }, requestState: 's1' });
});
});
Loading