Split responses and errors into separate message categories#1441
Conversation
jonathanhefner
left a comment
jonathanhefner
left a comment
There was a problem hiding this comment.
I think the schema here is in reference to the JSON-RPC response object (which encompasses both "regular" responses and error responses), whereas our schema.json models those a bit differently.
My instinct would be to change schema.json to more closely model the JSON-RPC spec, but I also think the changes in this PR improve clarity.
|
Good call, didn't notice that JSON-RPC and the schema differed. I think updating the schema to match JSON-RPC feels like the right approach here. |
pja-ant
force-pushed
the
split-response-error-categories
branch
from
d5632fa to
0509c42
Compare
|
Resolved comments (id for errors now optional, and updated response schema to be sum type over success + error response). |
- Separate "Responses" section for successful responses with result field - Separate "Errors" section for error responses with error field - Add schema anchor links to each message type description - Clarify that responses must have result, errors must have error field 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Simplified the message type hierarchy by combining successful responses and error responses into a single "Responses" section, as they are both responses to requests per JSON-RPC 2.0 specification. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…rror ID optional - Rename JSONRPCResponse to JSONRPCResponseSuccess - Rename JSONRPCError to JSONRPCResponseError - Make JSONRPCResponseError.id optional (for malformed requests) - Create JSONRPCResponse as union type: JSONRPCResponseSuccess | JSONRPCResponseError - Split Responses section in basic/index.mdx into Success Responses and Error Responses subsections - Regenerate schema.json and schema.mdx 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…ponseError to JSONRPCErrorResponse 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
pja-ant
force-pushed
the
split-response-error-categories
branch
from
5e1cda4 to
89dac20
Compare
Pull request was converted to draft
| * | ||
| * @category JSON-RPC | ||
| */ | ||
| export interface JSONRPCError { |
There was a problem hiding this comment.
FYI: I think this technically manifests as a breaking change in the SDKs - e.g. TypeScript SDK used to export this type but it was replaced by the union above.
Also JSONRPCResponse is now a different type (union of 2 types rather than specifically JSONRPCResultResponse. Had keep around the old types as @deprecated: modelcontextprotocol/typescript-sdk#1306
cc: @KKonstantinov
6 participants
I found it odd that the spec includes both success and error responses under the same Message type, but the schema has these as separate. I think keeping them separate works better so that (a) it matches the schema, and (b) you don't need awkward wording to explain what each type of responses needs to do.
A couple of other changes:
Motivation and Context
Tidying up docs.
How Has This Been Tested?
Breaking Changes
N/A
Types of changes
Checklist