-
Notifications
You must be signed in to change notification settings - Fork 25
Streamable http transport fix #26
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
Conversation
- Implementation seems to work - Unit test is broken on client/server protocol issue
… to route request with stream endpoint to StreamableEndpoint, ensure using correct protocolVersion as default returned is 2024-11-05 which was not included streamble Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
| public String protocolVersion() { | ||
| // FIXME this should be done in sdk itself as returning "2024-11-05" | ||
| // looks wrong for streamable | ||
| // see https://github.com/modelcontextprotocol/java-sdk/pull/441 |
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.
modelcontextprotocol/java-sdk#441
will need changes with next release of the java-sdl
| "servers": { | ||
| "jenkins": { | ||
| "type": "http", | ||
| "url": "https://jenkins-host/mcp-server/streamable", |
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.
We should be using /mcp url since that's the standard url followed by other MCP servers at large, too. Thanks!
|
closing in favour of #28 |
fix implementation from PR #25