fix(mcp): notify only the session whose compatibility shim transitioned - #108
Open
carldebilly wants to merge 1 commit into
Open
carldebilly wants to merge 1 commit into
carldebilly wants to merge 1 commit into
Conversation
Under DynamicToolCompatibilityMode.DiscoverAndCallShim, each initialize-era session answers its first tools/list with the discover_tools/call_tool pair, then signals tools/list_changed so it re-lists and gets the real catalog. The transition is connection-local — each session claims its own intro — but the signal cleared _toolListChanged, a handler-wide collection whose SDK fan-out notifies every attached session. Every other session got a tools/list_changed for a catalog that had not changed. The notification now goes to request.Server, the server of the request that made the transition. SignalToolListChanged lost its only caller and is removed. Its comment, about Clear() raising Changed on an empty collection, moves to SignalDiscoveryChanged, which still relies on that. TDD: the new test is red 3/3 before the fix — session B receives the notification session A's transition caused. A ping on B, sent after A's own notification arrives, is the ordering barrier: the SDK starts every session's send synchronously inside that one Clear(), so a notification bound for B is already queued ahead of the ping's response. B's own transition is the positive control that the handler observes one when it should. Refs #102
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #102.
Under
DynamicToolCompatibilityMode.DiscoverAndCallShim, an initialize-era session answers its firsttools/listwith thediscover_tools/call_toolpair. It then sendstools/list_changedso the client re-lists and gets the real catalog. That transition belongs to one connection, but the notification went through the handler-wide_toolListChangedcollection, and the SDK fans that collection out to every attached session. So each session's transition sent a spurioustools/list_changedto all the others.The notification is now sent to
request.Serveronly.SignalToolListChangedhas no callers left, so it is removed. Its comment aboutClear()raisingChangedmoves toSignalDiscoveryChanged, which still relies on that behaviour.Test plan
When_OneLegacySessionConsumesItsShimIntro_Then_NoOtherSessionIsNotifiedwas red 3/3 before the fix, because session B received the notification caused by session A's transition. The ordering barrier is apingon B sent after A's own notification arrives. The SDK starts every session's send synchronously inside the singleClear(), so anything bound for B is already queued ahead of the ping's response. B's own transition is the positive control.