Repository navigation
Type generics for CallToolResult - Require 'structuredContent' if 'outputSchema' defined. - #859
KKonstantinov wants to merge 12 commits into
Conversation
…urn if outputSchema defined
|
Please review, but this could be further improved by not allowing "structuredContent" at all when "outputSchema" is defined. Currently, it'll only generate a typecheck error if "outputSchema" is defined but "structuredContent" is missing, but it will not error when "structuredContent" is returned and "outputSchema" wasn't defined in the first place. (e.g. if the user forgot by accident). cc @ihrpr |
cliffhall
left a comment
cliffhall
left a comment
There was a problem hiding this comment.
Suggestion for changes to mcpServerOutputSchema.ts example.
Would be good to see explicit tests for CallToolResultUnstructuredSchema and CallToolResultStructuredSchema.
| export type CallToolResult = Infer<typeof CallToolResultSchema>; | ||
| export type CallToolResultUnstructured = Infer<typeof CallToolResultUnstructuredSchema>; | ||
| export type CallToolResultStructured<OArgs extends ZodRawShape> = Infer<typeof CallToolResultStructuredSchema> & { | ||
| structuredContent: z.infer<z.ZodObject<OArgs, 'strip'>>; |
There was a problem hiding this comment.
Not sure what 'strip' adds here? (the actual runtime zod schema won't strip extra fields)
There was a problem hiding this comment.
Have removed it, frankly I don't recall why I had added it at the time of this PR, might have been related to the removal of passthrough from runtime but yes, just doing it at compile time doesn't help, removed.
| extra: RequestHandlerExtra<ServerRequest, ServerNotification>, | ||
| ) => CallToolResult | Promise<CallToolResult> | ||
| : (extra: RequestHandlerExtra<ServerRequest, ServerNotification>) => CallToolResult | Promise<CallToolResult>; | ||
| ) => OutputArgs extends ZodRawShape |
There was a problem hiding this comment.
Could you extract the return type to avoid this repetition?
export type ToolCallback<InputArgs extends undefined | ZodRawShape = undefined, OutputArgs extends undefined | ZodRawShape = undefined> =
InputArgs extends ZodRawShape
? (args: z.objectOutputType<InputArgs, ZodTypeAny>, extra: RequestHandlerExtra<ServerRequest, ServerNotification>) => CallToolResult<OutputArgs>
: (extra: RequestHandlerExtra<ServerRequest, ServerNotification>) => CallToolResult<OutputArgs>;
type CallToolResult<OutputArgs extends undefined | ZodRawShape = undefined> =
OutputArgs extends ZodRawShape
? CallToolResultStructured<OutputArgs> | Promise<CallToolResultStructured<OutputArgs>>
: OutputArgs extends undefined
? CallToolResultUnstructured | Promise<CallToolResultUnstructured>
: never;There was a problem hiding this comment.
Done. Have called it "CallToolResultByOutputArgsType" for a lack of a better name (open to suggestions..).
CallToolResult is already used and imported - that is the union type between structured and unstructured.
Unfortunately I don't think we can make CallToolResult generic in the core types.ts (although I very much wish we could) because it then won't match the non-generic type defined in https://github.com/modelcontextprotocol/modelcontextprotocol/blob/main/schema/2025-06-18/schema.ts
The reality is the CallToolResult in https://github.com/modelcontextprotocol/modelcontextprotocol/blob/main/schema/2025-06-18/schema.ts NOT being generic is a bit misleading, because it implies returning structuredContent even when there is no output schema defined.
| @@ -1165,13 +1167,21 @@ export class ResourceTemplate { | |||
| * - `content` if the tool does not have an outputSchema | |||
There was a problem hiding this comment.
nit: could you update this comment? (cf. https://modelcontextprotocol.io/specification/2025-06-18/server/tools#structured-content)
- `content`: if an outputSchema is defined, content *SHOULD* have the serialized JSON structuredContent in a text content for backwards compatibility
0845a57 to
c94ba4b
Compare
|
Tks for the comments @ochafik , had missed these last week, now addressed. |
ochafik
left a comment
ochafik
left a comment
There was a problem hiding this comment.
Thanks @KKonstantinov ! Two last nits, looks great otherwise!
…ame OArgs to OutputArgs
@ochafik sorry just coming back to this, comments are done! |
|
Hey @KKonstantinov wanna get this merged this week? |
4 participants
The structuredContent tool output is currently not utilizing Typescript's capabilities and the check is performed only runtime.
If outputSchema is defined, as per the implementation, structuredContent is required in the response.
Motivation and Context
Mitigating runtime-only handling and proper type checking when outputSchema is defined.
How Has This Been Tested?
The changes are in types only - the results of the PR can be observed in tests, //@ts-expect-error has to be added now when testing for tools defined with outputSchema and not returning structuredContent.
Breaking Changes
No need, only if they had a runtime error anyways will they get a type error.
Types of changes
Checklist