Optimize capability and notification according to specification #184
Conversation
WalkthroughThis update introduces enhancements to both client and server components of the system, focusing on capability management and notification mechanisms. On the client side, the Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
client/client.go (1)
44-48:⚠️ Potential issueClientOptions are not being applied in NewClient.
The
optionsparameter is accepted but never applied to the client instance. The functional options won't have any effect.Apply this fix:
func NewClient(transport transport.Interface, options ...ClientOption) *Client { - return &Client{ + client := &Client{ transport: transport, } + + // Apply all provided options + for _, option := range options { + option(client) + } + + return client }
🧹 Nitpick comments (2)
client/client.go (1)
128-129: JSON field name may cause confusion.The field name in the struct is
Capabilitiesbut it's tagged withjson:"serverCapabilities". This mismatch between field name and JSON tag could cause confusion.Consider renaming the field to match its JSON tag for clarity:
params := struct { ProtocolVersion string `json:"protocolVersion"` ClientInfo mcp.Implementation `json:"clientInfo"` - Capabilities mcp.ClientCapabilities `json:"serverCapabilities"` + ServerCapabilities mcp.ClientCapabilities `json:"serverCapabilities"` }{ ProtocolVersion: request.Params.ProtocolVersion, ClientInfo: request.Params.ClientInfo, - Capabilities: request.Params.Capabilities, // Will be empty struct if not set + ServerCapabilities: request.Params.Capabilities, // Will be empty struct if not set }server/server.go (1)
465-470: Fix comment to reference prompts instead of resources.The comment incorrectly refers to "resources" when it should refer to "prompts".
- // When the list of available resources changes, servers that declared the listChanged capability SHOULD send a notification. + // When the list of available prompts changes, servers that declared the listChanged capability SHOULD send a notification.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/client.go(5 hunks)client/sse_test.go(1 hunks)mcp/types.go(2 hunks)server/server.go(6 hunks)server/server_test.go(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
server/server_test.go (3)
mcp/types.go (2)
MethodNotificationToolsListChanged(62-62)RESOURCE_NOT_FOUND(245-245)server/server.go (2)
NewMCPServer(375-401)WithToolCapabilities(351-358)examples/everything/main.go (1)
NewMCPServer(31-155)
client/client.go (2)
mcp/types.go (3)
JSONRPCNotification(206-209)ClientCapabilities(323-333)ServerCapabilities(338-361)client/transport/interface.go (1)
Interface(11-27)
server/server.go (1)
mcp/types.go (4)
MethodNotificationResourcesListChanged(52-52)MethodNotificationPromptsListChanged(58-58)MethodNotificationToolsListChanged(62-62)RESOURCE_NOT_FOUND(245-245)
🔇 Additional comments (15)
mcp/types.go (2)
49-63: Well-defined notification method constants.These new notification method constants provide a standardized way to handle notifications for resource, prompt, and tool list changes, which improves code maintainability and reduces the risk of typos in string literals.
243-246: Good addition of specific error code for resource not found.Adding a dedicated error code
RESOURCE_NOT_FOUNDprovides better semantics than using the more genericINVALID_PARAMSfor missing resources. This allows clients to handle this specific case more appropriately.client/sse_test.go (1)
14-14: Naming updated for consistency.The comment now correctly refers to "serverCapabilities" instead of just "capabilities", which aligns with the terminology changes in other files where client and server capabilities are now separate concepts.
server/server_test.go (3)
202-202: Good replacement of string literals with constants.Using the defined constants like
mcp.MethodNotificationToolsListChangedinstead of hardcoded strings makes the code more maintainable and less prone to errors.Also applies to: 244-245, 272-273, 297-299
864-864: Correctly updated error code for resource not found.Updating from
mcp.INVALID_PARAMSto the newmcp.RESOURCE_NOT_FOUNDerror code provides more specific error handling for missing resources.
315-315: Explicit capability configuration improves test clarity.Adding
WithToolCapabilities(true)explicitly in the test server creation clarifies that the tests expect tool capabilities to be enabled.client/client.go (4)
19-25: Client struct now separates client and server capabilities.Splitting the capabilities into
clientCapabilitiesandserverCapabilitiesfields provides a clearer separation of concerns. This is a good architectural improvement.
27-34: Well-implemented functional options pattern.The introduction of
ClientOptionandWithClientCapabilitiesfollows the functional options pattern, which is a clean way to configure the client.
145-146: Good use of comments for clarity.The comment clearly explains that the server capabilities are being stored, which enhances code readability.
420-428: Useful getter methods for capabilities.Adding getters for both client and server capabilities provides a clean interface for accessing these fields without exposing the internal structure directly.
server/server.go (5)
420-425: Conditional notification for resource list changes looks good.This enhancement properly implements capability-aware notifications for resource list changes, using the new constant from mcp/types.go. Notifications will only be sent to clients that have opted in through the capabilities system.
445-450: Capability-aware notification for resource templates implemented correctly.The implementation correctly sends notifications for resource template changes only when the capability is enabled, maintaining consistency with the resource notification pattern.
492-496: Proper implementation of conditional tool notifications.This change correctly updates the tool notification logic to be capability-aware, using the appropriate constant from mcp/types.go.
515-519: Consistent implementation of tool deletion notifications.The conditional notification for tool deletions matches the pattern used for tool additions, maintaining consistency throughout the codebase.
748-752: Improved error code specificity for resource not found.Changing the error code from INVALID_PARAMS to RESOURCE_NOT_FOUND provides a more accurate error representation, making the API more informative for clients. This aligns with the new error code defined in mcp/types.go.
dugenkui03
force-pushed
the
patch-server-capability
branch
from
023aab9 to
2e4f116
Compare
|
On my computer, all test run successfully |
dugenkui03
changed the title
dugenkui03
force-pushed
the
patch-server-capability
branch
2 times, most recently
from
cf4b6c8 to
792e666
Compare
dugenkui03
force-pushed
the
patch-server-capability
branch
from
792e666 to
8ea296f
Compare
|
This is great! Thanks for this! |
Summary by CodeRabbit