mcp: send a tools changed notification when mcp routes change tools #1630
Conversation
dosubot
bot
added
the
size:XL
| m.baseURL = fmt.Sprintf("http://localhost:%d%s", env.EnvoyListenerPort(), path) | ||
|
|
||
| m.client = mcp.NewClient(&mcp.Implementation{Name: "demo-http-client", Version: "0.1.0"}, &mcp.ClientOptions{ | ||
| ToolListChangedHandler: func(_ context.Context, request *mcp.ToolListChangedRequest) { |
There was a problem hiding this comment.
From this point the change is not related to the issue. This adds tests that were missing to verify upstream tool change notification propagation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1630 +/- ##
==========================================
+ Coverage 83.22% 83.40% +0.17%
==========================================
Files 137 138 +1
Lines 12373 12433 +60
==========================================
+ Hits 10298 10370 +72
+ Misses 1445 1430 -15
- Partials 630 633 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/retest |
internal/mcpproxy/session.go
Outdated
| if heartbeatTicker != nil { | ||
| heartbeatTicker.Reset(heartbeatInterval) | ||
| } | ||
| case <-toolsChanged: |
There was a problem hiding this comment.
hmm. does this mean that toolsChanged shared across many notification streams/sessions but only one item will be sent by the config change? I think it will not work as expected i think when we have N streams
There was a problem hiding this comment.
i think instead of channel, we can use sync.Cond with multiple waiter
There was a problem hiding this comment.
oh, yes, good catch! I'll fix
There was a problem hiding this comment.
Done. PTAL.
I've opted for a silightly different approach:
- I've moved the config-related stuff to a
config.go(apologies for the noise). - I've created a changeSignaler interface, that still uses a channel, but instead of sending to it, it clsoes the channel to wake up all watchers. I think it's a bit cleaner than the
sync.Condin this case, because this allows us to use the existingselectloop we're using for upstream events or heartbeats.
Signed-off-by: Ignasi Barrera <[email protected]>
Signed-off-by: Ignasi Barrera <[email protected]>
nacx
force-pushed
the
tool-change-notify
branch
from
c5cac7c to
7422f1b
Compare
Signed-off-by: Ignasi Barrera <[email protected]>
| }, | ||
| } | ||
|
|
||
| // Start watcher goroutines to make sure all of them are notified |
There was a problem hiding this comment.
This is a new test added after the review comments to make sure we can notify N watchers
Signed-off-by: Ignasi Barrera <[email protected]>
3 participants
Description
Sends a tool list changed notification when the MCPRoutes change. We were already notifying when the upstream MCP servers changed the notifications, but now we're also notifying when the routes change or the filtered tools change (without upstream MCP server changes).
Related Issues/PRs (if applicable)
Fixes #1619
Special notes for reviewers (if applicable)
N/A