-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: Handle MCP server notification messages #2613
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
|
nice ... so this does subjectively give nice feedback. Need to think a bit on the design how to present it. I think both in GUI and CLI it should be some sort of spinner like activity - which just shows the message (subtle) which changes as new one comes in (instead of printing out). That would feel like a lot more fine grained progress, but the bones of it are here! really cool! |
|
@michaelneale I like it. Here's what it looks like as a spinner: spinner.movstill very informative of progress without taking more than a line of screen real estate |
|
Here's the example MCP server that emits log and progress messages: |
|
.bundle |
macOS ARM64 Desktop App (Apple Silicon)📱 Download macOS Desktop App (arm64, signed) Instructions: This link is provided by nightly.link and will work even if you're not logged into GitHub. |
baxen
left a comment
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.
This is a great feature!
We should make sure we test thoroughly with existing mcps and SSE version (i don't see any examples with SSE here) because so much surface area is updated. We'll likely need to update post switching to rmcp so i'm less concerned with backwards compatibility in the mcp crates here as long as integration tests are passing
| let output_str = output_task | ||
| .await | ||
| .map_err(|e| ToolError::ExecutionError(e.to_string()))? | ||
| .map_err(|e| ToolError::ExecutionError(e.to_string()))?; |
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.
nit: repeated map?
| T: TransportHandle + Send + Sync + 'static, | ||
| { | ||
| service: Mutex<S>, | ||
| service: Mutex<tower::timeout::Timeout<McpService<T>>>, |
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.
it's less important that we hardcode for now because we have rmcp to migrate to anyways, but why do we need to force the timeout wrapped service?
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.
Now that McpClient is built with a Transport instead of a Service, the service itself is really just an implementation detail for the McpClient. In fact we're really only using the tower::Service in order to use the timeout wrapper.
It could be made generic here again but makes things decently more complicated and since it isn't part of the public API it feels less useful to do so
4bae700 to
82921d7
Compare
|
thanks @baxen for review! I created an integration test that covers the new functionality over both SSE and STDIO: https://github.com/block/goose/blob/b896dee383268021b9bd4bce2897035a2f72afc7/crates/mcp-client/examples/integration_test.rs Also did a bunch of manual testing with some of the bundled MCPs: google drive, memory, developer of course. Everything LGTM. |
Co-authored-by: Michael Neale <michael.neale@gmail.com>
|
@jamadeo / @michaelneale please see this gh issue I just opened: #2767 This PR "broke" developer extension shell tool calls in goose cli. It seems it is not properly waiting for the tool output to complete (e.g., shell command output for an ls command) before responding to the LLM. The details are in the gh issue. Let me know if you have any questions over there. |
Co-authored-by: Michael Neale <michael.neale@gmail.com>


This adds handling for server-initiated notification messages. Currently it supports logs and progress messages. This involved a decent amount of work up and down the MCP service stack, so it's a hefty change. Notifications means the services need to support listening for more messages besides a single request response.
The change also adds a tiny bit of refactoring to support it:
Screen.Recording.2025-05-22.at.11.44.07.AM.mov
spinner.mov