-
Notifications
You must be signed in to change notification settings - Fork 746
feat: Add task related things to schema #671
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: main
Are you sure you want to change the base?
Conversation
|
I will try to implement this internal first, feel free to continue |
|
I would like to add modelcontextprotocol/modelcontextprotocol#1831 to this too |
|
Let's wait until the |
|
I'm ok to just drop it if time is not OK, and at the same time, I'm implementing it first internal too. |
|
Let's park this for now, then. |
|
@Kehrlann Just dropped. |
584e4a5 to
8544229
Compare
| @JsonProperty("content") List<Content> content, | ||
| @JsonProperty("isError") Boolean isError, | ||
| @JsonProperty("structuredContent") Object structuredContent, | ||
| @JsonProperty("task") Task task, |
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.
No | type in Java, so add the task here instead of GetTaskRequest | CallToolResult, an Either type will change much code, and Object loses type safety.
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.
I was adding a CallToolAuxResult and I found the JSONRPCResponse has a JSONRPCError, so I will follow the same design. it should be JSONRPCResponse | JSONRPCError, but the current shape seems pratical.
354d2e2 to
0ff73cb
Compare
Signed-off-by: He-Pin <[email protected]>
Signed-off-by: He-Pin <[email protected]>
Signed-off-by: He-Pin <[email protected]>
Signed-off-by: He-Pin <[email protected]>
Signed-off-by: He-Pin <[email protected]>
Signed-off-by: He-Pin <[email protected]>
Signed-off-by: He-Pin <[email protected]>
Signed-off-by: He-Pin <[email protected]>
Signed-off-by: He-Pin <[email protected]>
Signed-off-by: He-Pin <[email protected]>
Signed-off-by: He-Pin <[email protected]>
| */ | ||
| Map<String, Object> meta(); | ||
|
|
||
| sealed interface McpEvent extends McpMessage { |
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.
Where are these in the spec? I don't find them.
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.
My current implementation at work is Flux<McpMessage> callToolStream(CallToolRequest request) to support streaming responses. We also have callTool(CallToolRequest request) built upon this implementation.
I specifically plan to support streaming responses in the future. However, because my implementation at work is far more complex than a memory-based implementation, it's not yet ready for open-source development.
|
Update: Although my implementation at work is based on this, it includes database, some internal message queues, RPC, SSE, and webhook integration, so I don't expect to be able to contribute anything other than the schema for the next week or two. |
Signed-off-by: He-Pin <[email protected]>
Motivation and Context
ISSUE: modelcontextprotocol/modelcontextprotocol#1686
PR: modelcontextprotocol/modelcontextprotocol#1732
Issue in Java-sdk : #668
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context