feature: Support log/cancelled/progress notification #329
Conversation
## Walkthrough
This change introduces a new `RequestSession` parameter to all tool, resource, and prompt handler signatures, enabling handlers to send logging, progress, and cancellation notifications to clients. It adds a `RequestSession` struct and related methods, updates internal server logic, adjusts tests and examples, and updates documentation to reflect the new handler interface and session capabilities.
## Changes
| Files/Paths | Change Summary |
|-----------------------------------------------------------------------------|------------------------------------------------------------------------------------------------|
| README.md | Updated all handler signatures to include `RequestSession`; added documentation and examples for session-based notifications. |
| mcp/types.go, mcp/prompts.go | Added/updated method constants for notifications, extended request structs for metadata, removed notification structs, added log level filtering. |
| mcp/utils.go | Removed helper functions and type checks for now-removed notification structs. |
| server/request_session.go (new) | Introduced `RequestSession` struct with methods for sending log, progress, and cancellation notifications. |
| server/server.go | Updated handler function types and invocations to use `RequestSession`; adjusted notification sending and logging level handling. |
| server/typed_tools_handler_func.go (new), mcp/typed_tools.go (removed) | Moved typed tool handler logic to `server` package, updated to support `RequestSession`. |
| server/request_handler.go | Ensured context carries request ID for non-notification messages. |
| server/session.go, server/sse.go, server/stdio.go, server/streamable_http.go| Updated session interfaces and structs to support logger name and logging level. |
| server/session_test.go, server/server_test.go, server/resource_test.go, server/server_race_test.go, server/sse_test.go, server/stdio_test.go, server/streamable_http_test.go | Updated all handler/test function signatures to include `RequestSession`; added session logging methods to test clients. |
| client/inprocess_test.go, client/sse_test.go | Updated handler signatures in tests; added/verified notification handling in SSE client tests. |
| examples/custom_context/main.go, examples/dynamic_path/main.go, examples/everything/main.go, examples/typed_tools/main.go | Updated all handler signatures to include `RequestSession`. |
| server/typed_tools_handler_func_test.go | Updated test package, imports, and handler signatures to use `RequestSession`. |
| mcptest/mcptest_test.go | Updated test handler signature to include `RequestSession`. |
## Assessment against linked issues
| Objective | Addressed | Explanation |
|-------------------------------------------------------------------------------------------------------------------|-----------|-----------------------------------------------------------------------------|
| Pass session/capability object (like `RequestSession`) to all handler types to enable log/progress/cancel notification (#321) | ✅ | |
| Provide methods on session object for sending log, progress, and cancellation notifications (#321) | ✅ | |
| Update documentation and examples to show new handler signatures and notification usage (#321) | ✅ | |
## Assessment against linked issues: Out-of-scope changes
| Code Change | Explanation |
|------------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------|
| Removal of `TypedToolHandlerFunc` and `NewTypedToolHandler` from `mcp/typed_tools.go` | This refactoring moves typed tool handler logic to the `server` package rather than adding to #321 objectives directly. |
| Addition of `loggerName` fields and `GetLoggerName()` methods in various session structs | These changes enhance session logging capabilities but are not explicitly required by #321. |
| Changes to `levelToSeverity` map and `LoggingLevel.Allows` method | These additions improve log level filtering but are not explicitly mentioned in #321. |
## Possibly related PRs
- mark3labs/mcp-go#46: The main PR changes handler function signatures to add a `RequestSession` parameter and introduces the `RequestSession` struct with methods for sending notifications, while the retrieved PR focuses on replacing a global notification channel with per-session notification channels and session management interfaces; thus, although both involve session and notification handling, the main PR’s changes to handler signatures and `RequestSession` usage are related to the retrieved PR’s introduction of per-session notification handling and session interfaces.
## Suggested labels
`type: enhancement`, `area: mcp spec`
## Suggested reviewers
- pottekkat
- robert-jackson-gleanWarning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
✨ Finishing Touches
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
dugenkui03
force-pushed
the
support-RequestContext-with-logging-progress-cancellation
branch
from
619d10d to
5697131
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
mcp/types.go (1)
214-244:⚠️ Potential issueCritical: Data race detected in NotificationParams.UnmarshalJSON.
The pipeline detected a data race in this method. The issue occurs because:
- Maps are initialized without synchronization (lines 223-228)
- Multiple goroutines might unmarshal the same
NotificationParamsinstance concurrently- Map writes are not thread-safe
This could cause runtime panics or data corruption in production.
Consider one of these solutions:
Option 1: Make NotificationParams immutable during unmarshaling
func (p *NotificationParams) UnmarshalJSON(data []byte) error { // Create a map to hold all fields var m map[string]any if err := json.Unmarshal(data, &m); err != nil { return err } - // Initialize maps if they're nil - if p.Meta == nil { - p.Meta = make(map[string]any) - } - if p.AdditionalFields == nil { - p.AdditionalFields = make(map[string]any) - } + // Create new maps to avoid concurrent writes + meta := make(map[string]any) + additionalFields := make(map[string]any) // Process all fields for k, v := range m { if k == "_meta" { // Handle Meta field if meta, ok := v.(map[string]any); ok { - p.Meta = meta + meta = meta } } else { // Handle additional fields - p.AdditionalFields[k] = v + additionalFields[k] = v } } + + // Atomic assignment at the end + p.Meta = meta + p.AdditionalFields = additionalFields return nil }Option 2: Document that NotificationParams instances should not be shared between goroutines during unmarshaling
Add a comment above the struct:
// NotificationParams holds notification parameters. // Note: Instances of this type should not be shared between goroutines // during JSON unmarshaling as it may cause data races. type NotificationParams struct {🧰 Tools
🪛 GitHub Actions: go
[error] 218-239: Data race detected in NotificationParams.UnmarshalJSON method. Concurrent map writes and reads occur during JSON unmarshalling, causing race conditions.
client/sse_test.go (1)
45-108:⚠️ Potential issueAddress data race conditions in notification testing.
The pipeline failures indicate data race conditions in this test section. The issue likely stems from concurrent access to shared notification collections without proper synchronization.
The test modifies shared variables (
notificationNum,messageNotification,progressNotifications) from multiple goroutines without synchronization. Consider using proper synchronization mechanisms:+import "sync" +var ( + mu sync.Mutex + notificationNum int + messageNotification *mcp.JSONRPCNotification + progressNotifications []*mcp.JSONRPCNotification +) client.OnNotification(func(notification mcp.JSONRPCNotification) { + mu.Lock() + defer mu.Unlock() if notification.Method == string(mcp.MethodNotificationMessage) { messageNotification = ¬ification } else if notification.Method == string(mcp.MethodNotificationProgress) { progressNotifications = append(progressNotifications, ¬ification) } notificationNum += 1 })
🧹 Nitpick comments (4)
server/server.go (1)
335-335: Consistent type conversion for notification methods.All notification method calls correctly convert
MCPMethodtostring. While this is functionally correct, consider updatingSendNotificationToAllClientsto acceptMCPMethoddirectly to avoid these conversions throughout the codebase.Also applies to: 350-350, 382-382, 409-409, 429-429, 471-471, 498-498
client/sse_test.go (1)
601-601: Remove debug print statements.The
printlnstatements appear to be debug leftovers and should be removed from production test code.- println(notification.Method) - println(fmt.Sprintf("%v", result)) - println(notification.Method)Also applies to: 654-654, 690-690
README.md (1)
95-95: Fix markdown formatting issues.Static analysis detected formatting inconsistencies that should be addressed for better documentation quality.
- - [RequestContext](#RequestContext) + - [RequestContext](#requestcontext)Also, ensure consistent use of spaces instead of hard tabs throughout the code examples in the RequestContext section (lines 546-564).
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
95-95: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
95-95: Link fragments should be valid
Expected: #requestcontext; Actual: #RequestContext(MD051, link-fragments)
examples/everything/main.go (1)
389-432: Consider migrating to new RequestContext notification methods.The
handleLongRunningOperationToolfunction still uses the olderserver.SendNotificationToClientapproach. Consider updating it to use the newrequestContext.SendProgressNotificationmethod for consistency and better encapsulation.- server := server.ServerFromContext(ctx) - err := server.SendNotificationToClient( - ctx, - "notifications/progress", - map[string]any{ - "progress": i, - "total": int(steps), - "progressToken": progressToken, - "message": fmt.Sprintf("Server progress %v%%", int(float64(i)*100/steps)), - }, - ) + message := fmt.Sprintf("Server progress %v%%", int(float64(i)*100/steps)) + total := float64(steps) + err := requestContext.SendProgressNotification(ctx, float64(i), &total, &message)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
server/internal/gen/request_handler.go.tmplis excluded by!**/gen/**
📒 Files selected for processing (28)
README.md(12 hunks)client/inprocess_test.go(3 hunks)client/sse_test.go(6 hunks)examples/custom_context/main.go(1 hunks)examples/dynamic_path/main.go(1 hunks)examples/everything/main.go(10 hunks)examples/typed_tools/main.go(2 hunks)mcp/prompts.go(1 hunks)mcp/typed_tools.go(0 hunks)mcp/types.go(3 hunks)mcp/utils.go(0 hunks)mcptest/mcptest_test.go(1 hunks)server/request_context.go(1 hunks)server/request_handler.go(1 hunks)server/resource_test.go(5 hunks)server/server.go(15 hunks)server/server_race_test.go(7 hunks)server/server_test.go(17 hunks)server/session.go(1 hunks)server/session_test.go(9 hunks)server/sse.go(2 hunks)server/sse_test.go(3 hunks)server/stdio.go(2 hunks)server/stdio_test.go(1 hunks)server/streamable_http.go(3 hunks)server/streamable_http_test.go(2 hunks)server/typed_tools_handler_func.go(1 hunks)server/typed_tools_handler_func_test.go(14 hunks)
💤 Files with no reviewable changes (2)
- mcp/utils.go
- mcp/typed_tools.go
🧰 Additional context used
🧬 Code Graph Analysis (14)
server/request_handler.go (1)
mcp/types.go (1)
NewRequestId(259-261)
mcp/prompts.go (1)
mcp/types.go (1)
Meta(135-147)
examples/custom_context/main.go (1)
server/request_context.go (1)
RequestContext(13-17)
examples/dynamic_path/main.go (2)
mcp/tools.go (3)
NewTool(567-589)CallToolRequest(46-53)CallToolResult(36-43)server/request_context.go (1)
RequestContext(13-17)
server/server_race_test.go (5)
server/request_context.go (1)
RequestContext(13-17)mcp/prompts.go (3)
GetPromptRequest(20-31)GetPromptResult(35-40)Prompt(46-54)mcp/tools.go (3)
CallToolRequest(46-53)CallToolResult(36-43)Description(669-673)server/server.go (1)
ToolHandlerFunc(40-40)mcp/types.go (2)
ReadResourceRequest(526-538)ResourceContents(640-642)
mcptest/mcptest_test.go (2)
server/request_context.go (1)
RequestContext(13-17)mcp/tools.go (2)
CallToolRequest(46-53)CallToolResult(36-43)
server/stdio_test.go (2)
mcp/tools.go (3)
NewTool(567-589)CallToolRequest(46-53)CallToolResult(36-43)server/request_context.go (1)
RequestContext(13-17)
server/typed_tools_handler_func.go (3)
server/request_context.go (1)
RequestContext(13-17)mcp/tools.go (2)
CallToolRequest(46-53)CallToolResult(36-43)mcp/utils.go (1)
NewToolResultError(241-251)
server/sse_test.go (2)
mcp/tools.go (3)
NewTool(567-589)CallToolRequest(46-53)CallToolResult(36-43)server/request_context.go (1)
RequestContext(13-17)
server/streamable_http_test.go (2)
server/request_context.go (1)
RequestContext(13-17)mcp/tools.go (2)
CallToolRequest(46-53)CallToolResult(36-43)
server/streamable_http.go (1)
mcp/types.go (2)
LoggingLevel(685-685)LoggingLevelError(692-692)
server/typed_tools_handler_func_test.go (5)
server/request_context.go (1)
RequestContext(13-17)mcp/tools.go (2)
CallToolRequest(46-53)CallToolResult(36-43)mcp/utils.go (2)
NewToolResultText(175-184)NewToolResultError(241-251)server/typed_tools_handler_func.go (1)
NewTypedToolHandler(13-21)mcp/types.go (4)
Content(782-784)TextContent(788-793)TextContent(795-795)Params(177-177)
server/session_test.go (3)
mcp/types.go (2)
LoggingLevel(685-685)LoggingLevelError(692-692)server/request_context.go (1)
RequestContext(13-17)mcp/tools.go (3)
CallToolRequest(46-53)CallToolResult(36-43)NewTool(567-589)
examples/everything/main.go (1)
server/request_context.go (1)
RequestContext(13-17)
🪛 GitHub Actions: go
client/sse_test.go
[error] 457-539: Data race detected during execution of TestSSEMCPClient and its subtests. Multiple concurrent goroutines read and write shared variables without synchronization, causing race conditions. Key locations include lines 457-539 in sse_test.go and related code in client.go and transport/sse.go.
[error] 591-762: Data race detected during execution of TestSSEMCPClient.func14 and func15. Multiple concurrent goroutines read and write shared variables without synchronization, causing race conditions.
mcp/types.go
[error] 218-239: Data race detected in NotificationParams.UnmarshalJSON method. Concurrent map writes and reads occur during JSON unmarshalling, causing race conditions.
🪛 markdownlint-cli2 (0.17.2)
README.md
95-95: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
95-95: Link fragments should be valid
Expected: #requestcontext; Actual: #RequestContext
(MD051, link-fragments)
546-546: Hard tabs
Column: 1
(MD010, no-hard-tabs)
547-547: Hard tabs
Column: 1
(MD010, no-hard-tabs)
548-548: Hard tabs
Column: 1
(MD010, no-hard-tabs)
549-549: Hard tabs
Column: 1
(MD010, no-hard-tabs)
550-550: Hard tabs
Column: 1
(MD010, no-hard-tabs)
552-552: Hard tabs
Column: 1
(MD010, no-hard-tabs)
553-553: Hard tabs
Column: 1
(MD010, no-hard-tabs)
554-554: Hard tabs
Column: 1
(MD010, no-hard-tabs)
555-555: Hard tabs
Column: 1
(MD010, no-hard-tabs)
557-557: Hard tabs
Column: 1
(MD010, no-hard-tabs)
558-558: Hard tabs
Column: 1
(MD010, no-hard-tabs)
559-559: Hard tabs
Column: 1
(MD010, no-hard-tabs)
560-560: Hard tabs
Column: 1
(MD010, no-hard-tabs)
561-561: Hard tabs
Column: 1
(MD010, no-hard-tabs)
562-562: Hard tabs
Column: 1
(MD010, no-hard-tabs)
563-563: Hard tabs
Column: 1
(MD010, no-hard-tabs)
564-564: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (53)
server/request_handler.go (1)
57-59: LGTM: Proper request ID context handling.The else block correctly adds the request ID to the context for non-notification messages, which supports the RequestContext functionality for progress and logging notifications. The logic properly distinguishes between notifications (ID == nil) and regular requests.
mcp/prompts.go (1)
27-29: LGTM: Well-implemented metadata field for request context.The addition of the
Metafield toGetPromptRequest.Paramsis correctly implemented with:
- Appropriate pointer type for optional metadata
- Proper JSON tag with
omitempty- Clear documentation explaining its purpose
- Consistent with the broader RequestContext support for progress and logging notifications
examples/custom_context/main.go (1)
82-82: LGTM: Proper handler signature update.The addition of the
requestContext server.RequestContextparameter correctly updates the tool handler signature to support the new context functionality for logging, progress, and cancellation notifications. The parameter placement betweenctxandrequestfollows the established pattern across the codebase.server/resource_test.go (2)
31-31: LGTM: Test handler signatures updated correctly.The anonymous resource read handler functions have been properly updated to include the new
requestContext RequestContextparameter, maintaining consistency with the updated handler signature requirements throughout the codebase.Also applies to: 50-50, 111-111, 167-167
87-87: LGTM: Proper string conversion in assertion.The explicit conversion to string ensures type consistency in the assertion, which is a good practice when comparing constants to string values.
mcptest/mcptest_test.go (1)
53-53: LGTM! Handler signature correctly updated for RequestContext support.The function signature has been properly updated to include the new
RequestContextparameter, aligning with the broader refactoring to support logging, progress, and cancellation notifications. The parameter placement betweenctxandrequestis consistent with the pattern used across the codebase.server/sse.go (1)
79-82: LGTM! Logger name getter method correctly implemented.The
GetLoggerName()method properly returns the logger name field as expected by theClientSessioninterface.examples/dynamic_path/main.go (1)
22-24: LGTM! Tool handler signature correctly updated.The tool handler function signature has been properly updated to include the
RequestContextparameter, maintaining consistency with the new handler interface pattern. The function logic remains unchanged, which is appropriate for this signature update.server/session.go (1)
24-26: LGTM! Interface correctly extended with logger name support.The addition of
GetLoggerName() *stringto theClientSessioninterface properly supports the enhanced logging capabilities introduced in this PR. The method signature is consistent with the implementations in the session types.server/stdio.go (1)
105-107: LGTM: Method implementation is correct.The
GetLoggerName()method correctly returns the logger name pointer, following the expected interface pattern.examples/typed_tools/main.go (2)
76-76: LGTM: RequestContext parameter addition is correct.The function signature correctly includes the new
RequestContextparameter, enabling the handler to send logging, progress, and cancellation notifications to clients.
67-67:✅ Verification successful
Verify the typed tool handler package migration.
The typed tool handler functionality appears to have moved from
mcp.NewTypedToolHandlertoserver.NewTypedToolHandler. This is a breaking change that could affect other code using the old API.Please verify that all references to
mcp.NewTypedToolHandlerhave been updated across the codebase:
🏁 Script executed:
#!/bin/bash # Description: Search for any remaining references to the old mcp.NewTypedToolHandler # Expected: No occurrences should be found if migration is complete echo "Searching for old mcp.NewTypedToolHandler references..." rg "mcp\.NewTypedToolHandler" --type go echo -e "\nSearching for new server.NewTypedToolHandler references..." rg "server\.NewTypedToolHandler" --type goLength of output: 436
Package migration verified: all references to mcp.NewTypedToolHandler removed
- No occurrences of
mcp.NewTypedToolHandlerremain in the codebase.server.NewTypedToolHandleris used only inexamples/typed_tools/main.goas intended.This breaking change is fully applied and requires no further updates.
server/stdio_test.go (1)
143-143: LGTM: Test handler signature correctly updated.The anonymous tool handler function signature has been properly updated to include the
RequestContextparameter, maintaining consistency with the new handler interface.server/server_race_test.go (2)
42-42: LGTM: Consistent RequestContext parameter additions.All handler function signatures have been correctly updated to include the
RequestContextparameter, maintaining consistency across the test suite.Also applies to: 52-52, 63-63, 74-74, 105-105, 124-124, 172-172, 177-177
82-84: Excellent middleware pattern implementation.The middleware function correctly forwards the
RequestContextparameter to the next handler in the chain, demonstrating proper middleware composition with the new signature.server/streamable_http.go (1)
529-539: LGTM! Thread-safe logging level implementation.The atomic operations for
SetLogLevelandGetLogLevelare correctly implemented for thread safety. The fallback tomcp.LoggingLevelErrorwhen no level is set provides a sensible default.server/sse_test.go (3)
577-577: LGTM! Consistent signature update.The handler signature is correctly updated to include the
RequestContextparameter, aligning with the new API pattern. The handler logic remains unchanged, which is appropriate for this test.
1186-1188: LGTM! Consistent signature update.The handler signature is correctly updated to include the
RequestContextparameter. The implementation remains simple and appropriate for testing purposes.
1338-1338: LGTM! Consistent signature update.The handler signature is correctly updated to include the
RequestContextparameter, maintaining consistency across all test handlers.server/streamable_http_test.go (2)
42-42: LGTM! Consistent signature update.The
addSSEToolhandler signature is correctly updated to include theRequestContextparameter, aligning with the new API pattern throughout the codebase.
604-604: LGTM! Consistent signature update.The test tool handler signature is correctly updated to include the
RequestContextparameter, maintaining consistency with the broader refactoring.server/typed_tools_handler_func.go (2)
9-11: LGTM! Well-designed generic function type.The
TypedToolHandlerFunc[T any]generic type provides excellent type safety for tool handlers while maintaining compatibility with the newRequestContextparameter. The generic constraint[T any]is appropriate for flexible argument binding.
12-21: LGTM! Excellent typed handler wrapper implementation.The
NewTypedToolHandlerfunction provides a clean and type-safe way to handle tool arguments:
- Proper error handling with descriptive error messages using
mcp.NewToolResultError- Automatic argument binding via
request.BindArguments- Consistent signature that integrates well with the
RequestContextpattern- Clean separation of concerns between argument binding and handler logic
This implementation promotes type safety while maintaining flexibility and good error reporting.
client/inprocess_test.go (3)
30-30: LGTM! Handler signature updated correctly.The addition of
requestContext server.RequestContextparameter to the tool handler follows the correct pattern and aligns with the PR objective to support log/cancelled/progress notifications.
51-51: LGTM! Resource handler signature updated correctly.The RequestContext parameter addition is consistent with the tool handler change above.
73-73: LGTM! Prompt handler signature updated correctly.The RequestContext parameter addition maintains consistency across all handler types.
server/server_test.go (4)
163-163: LGTM! Handler signatures updated consistently.All tool handler function signatures have been correctly updated to include the
requestContext RequestContextparameter. The positioning afterctx context.Contextis consistent across all handlers.Also applies to: 168-168, 192-192, 197-197, 233-233, 238-238, 265-265, 271-271
204-204: LGTM! Method constant string conversions are correct.The explicit conversion of MCP method constants to strings using
string()is necessary for proper comparison and follows the updated type system where these constants are now typed.Also applies to: 246-246, 278-278, 279-279, 303-303, 305-305, 336-336, 846-846, 848-848, 901-901, 903-903, 905-905, 954-954, 956-956
525-533: LGTM! fakeSession interface implementation updated correctly.The addition of logging-related methods
SetLogLevel,GetLogLevel, andGetLoggerNameproperly implements the updatedClientSessioninterface with appropriate default values and no-op behavior for testing.
709-709: LGTM! Handler signatures consistently updated.All prompt, resource template, and tool handler signatures have been correctly updated to include the
RequestContextparameter in the proper position.Also applies to: 1124-1124, 1386-1386, 1473-1473, 1489-1489, 1635-1635, 1806-1806
server/typed_tools_handler_func_test.go (3)
1-1: LGTM! Package restructuring handled correctly.The change from
package mcptopackage serverand the addition of themcppackage import properly reflect the movement of typed tool handler functionality to the server package.Also applies to: 7-7
22-22: LGTM! Handler function signatures updated correctly.All typed handler function signatures have been consistently updated to include the
RequestContextparameter in the correct position, maintaining the expected signature pattern.Also applies to: 88-88, 174-174
23-23: LGTM! Type references properly qualified.All references to MCP types and utility functions are correctly prefixed with
mcp.to reflect the new import structure. The test calls properly passRequestContext{}as the required parameter.Also applies to: 30-30, 39-39, 44-44, 54-54, 66-66, 69-69, 73-73, 91-91, 104-104, 108-108, 111-111, 118-118, 127-127, 132-132, 141-141, 145-145, 177-177, 180-180, 214-214, 221-221, 243-243, 248-252, 269-269, 272-276, 298-298, 301-304
server/session_test.go (7)
45-54: LGTM! Test client logging methods properly implemented.The no-op implementations for
SetLogLevel, default error level forGetLogLevel, and nil return forGetLoggerNameare appropriate for test clients.
81-90: Consistent logging method implementations.The logging methods follow the same pattern as other test clients, maintaining consistency across test implementations.
162-171: Consistent test client implementation pattern.
208-210: Completes the SessionWithLogging interface implementation.Adding
GetLoggerNameensures this test client fully implements the updated interface.
224-228: Handler signature correctly updated with RequestContext.The function signature now includes the
RequestContextparameter as per the new API design.
274-274: Correct usage of empty RequestContext in test.Passing an empty
RequestContext{}is appropriate for this test scenario.
393-397: All handler signatures consistently updated.The tool handler function signatures have been uniformly updated to include the
RequestContextparameter.Also applies to: 598-601, 618-622
mcp/types.go (3)
59-92: Well-structured notification method constants with proper typing.The explicit
MCPMethodtyping improves type safety, and the new notification methods for logging, progress, and cancellation align perfectly with the PR objectives. The specification links in comments are helpful.
534-537: Meta field enables progress notifications for resource reads.Adding the
Metafield toReadResourceRequestparameters allows clients to request progress notifications, which is essential for the new notification features.
698-719: Well-implemented logging level filtering logic.The
Allowsmethod correctly implements severity-based filtering using a clear mapping of levels to numeric severities. The error handling for invalid levels is appropriate.server/server.go (4)
31-31: Handler function signatures properly updated with RequestContext.All handler function types now consistently include the
RequestContextparameter betweencontext.Contextand the request parameter, enabling rich client interaction capabilities.Also applies to: 34-34, 37-37, 40-40
218-232: Middleware correctly updated to support RequestContext.The
WithRecoverymiddleware properly forwards theRequestContextparameter while maintaining its panic recovery functionality.
731-732: RequestContext properly created and passed to handlers.The
RequestContextis consistently created with the server instance and request metadata, then passed to all handlers. This enables handlers to send progress, logging, and cancellation notifications.Also applies to: 738-738, 769-769, 851-852, 1002-1003
610-610: Simplified logging level setting aligns with interface changes.Directly calling
SetLogLevelonclientSessionis cleaner than type assertion and aligns with the updated interface design.server/request_context.go (3)
32-35: Good defensive programming practice.The method properly checks server capabilities before attempting to send notifications, preventing unnecessary operations when logging is not supported.
94-102: Robust error handling for request ID validation.The cancellation notification method properly validates the request ID existence and type before proceeding, which prevents runtime panics and provides clear error messages.
25-27:✅ Verification successful
Potential pointer handling issue in constructor.
The assignment
requestContext.progressToken = &requestParamMeta.ProgressTokenmay be incorrect depending on whetherProgressTokenis already a pointer type. IfProgressTokenis a pointer, this creates a double pointer which could cause issues when dereferencing in line 76.
🏁 Script executed:
#!/bin/bash # Description: Check the type definition of ProgressToken in mcp package to verify correct pointer handling # Search for ProgressToken type definition in mcp package ast-grep --pattern 'type $_ struct { $$$ ProgressToken $_ $$$ }' # Also search for ProgressToken field definitions rg -A 3 -B 1 "ProgressToken.*string\|ProgressToken.*\*"Length of output: 866
🏁 Script executed:
#!/bin/bash # Display the RequestContext struct and progressToken field definitions rg -n "type RequestContext" -A 5 server/request_context.go rg -n "progressToken" -A 5 server/request_context.goLength of output: 972
🏁 Script executed:
#!/bin/bash # Find the definition of ProgressToken in mcp/types.go rg -n "type ProgressToken" -A 5 mcp/types.goLength of output: 266
Pointer handling in RequestContext constructor is correct
The
ProgressTokentype is declared astype ProgressToken any(an interface alias), not a pointer. As a result, taking the address ofrequestParamMeta.ProgressTokenproduces a*mcp.ProgressTokenthat exactly matches theprogressToken *mcp.ProgressTokenfield onRequestContext. No double-pointer is introduced, and dereferencing*exchange.progressTokenat line 76 is safe.client/sse_test.go (1)
519-540: Good test coverage for notification filtering.The test properly verifies that logging notifications are filtered based on the client's log level, ensuring only error-level messages are received when the level is set to warning.
README.md (1)
534-567: Excellent documentation for new RequestContext feature.The documentation clearly explains the purpose and usage of RequestContext with practical examples. This will help developers understand how to use the notification capabilities effectively.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
546-546: Hard tabs
Column: 1(MD010, no-hard-tabs)
547-547: Hard tabs
Column: 1(MD010, no-hard-tabs)
548-548: Hard tabs
Column: 1(MD010, no-hard-tabs)
549-549: Hard tabs
Column: 1(MD010, no-hard-tabs)
550-550: Hard tabs
Column: 1(MD010, no-hard-tabs)
552-552: Hard tabs
Column: 1(MD010, no-hard-tabs)
553-553: Hard tabs
Column: 1(MD010, no-hard-tabs)
554-554: Hard tabs
Column: 1(MD010, no-hard-tabs)
555-555: Hard tabs
Column: 1(MD010, no-hard-tabs)
557-557: Hard tabs
Column: 1(MD010, no-hard-tabs)
558-558: Hard tabs
Column: 1(MD010, no-hard-tabs)
559-559: Hard tabs
Column: 1(MD010, no-hard-tabs)
560-560: Hard tabs
Column: 1(MD010, no-hard-tabs)
561-561: Hard tabs
Column: 1(MD010, no-hard-tabs)
562-562: Hard tabs
Column: 1(MD010, no-hard-tabs)
563-563: Hard tabs
Column: 1(MD010, no-hard-tabs)
564-564: Hard tabs
Column: 1(MD010, no-hard-tabs)
examples/everything/main.go (1)
194-194: Consistent handler signature updates.All handler function signatures have been properly updated to include the
requestContext server.RequestContextparameter. This maintains consistency across the codebase and enables the new notification capabilities.Also applies to: 208-208, 222-222, 260-260, 279-279, 318-318, 338-338, 360-360, 391-391, 457-457
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
client/sse_test.go (3)
71-109: Well-implemented notification testing handler with minor optimization opportunity.The handler correctly demonstrates both progress and logging notifications. The implementation properly:
- Sends progress notifications with start/end messages
- Tests log level filtering by sending both INFO and ERROR level messages
- Handles errors from notification sending
- Uses proper progress token handling
Consider extracting the common notification testing logic into a helper function to reduce code duplication across handlers:
+func sendTestNotifications(ctx context.Context, requestContext server.RequestContext, operation string) error { + totalProgressValue := float64(100) + startMessage := fmt.Sprintf("start %s", operation) + + if err := requestContext.SendProgressNotification(ctx, float64(0), &totalProgressValue, &startMessage); err != nil { + return err + } + + if err := requestContext.SendLoggingNotification(ctx, mcp.LoggingLevelInfo, map[string]any{ + "filtered_log_message": "will be filtered by log level", + }); err != nil { + return err + } + + if err := requestContext.SendLoggingNotification(ctx, mcp.LoggingLevelError, map[string]any{ + "log_message": "log message value", + }); err != nil { + return err + } + + endMessage := fmt.Sprintf("end %s", operation) + return requestContext.SendProgressNotification(ctx, float64(100), &totalProgressValue, &endMessage) +}
602-693: Comprehensive prompt notification test with debugging statement.The test properly verifies prompt notifications, but contains a debugging print statement that should be removed.
Remove the debugging print statement:
- println(fmt.Sprintf("%v", result))
464-477: Consider extracting common notification test setup.The notification handling setup is duplicated across multiple test cases. Consider extracting this into a helper function to reduce code duplication and improve maintainability.
+func setupNotificationHandling() (*sync.Mutex, *int, **mcp.JSONRPCNotification, *[]*mcp.JSONRPCNotification, func(mcp.JSONRPCNotification)) { + mu := &sync.Mutex{} + notificationNum := 0 + var messageNotification *mcp.JSONRPCNotification + progressNotifications := make([]*mcp.JSONRPCNotification, 0) + + handler := func(notification mcp.JSONRPCNotification) { + mu.Lock() + defer mu.Unlock() + if notification.Method == string(mcp.MethodNotificationMessage) { + messageNotification = ¬ification + } else if notification.Method == string(mcp.MethodNotificationProgress) { + progressNotifications = append(progressNotifications, ¬ification) + } + notificationNum++ + } + + return mu, ¬ificationNum, &messageNotification, &progressNotifications, handler +}Also applies to: 554-560, 608-621, 701-714
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/sse_test.go(6 hunks)server/sse.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/sse.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (8)
client/sse_test.go (8)
5-8: LGTM! Proper imports added for new functionality.The import additions are appropriate for the new notification testing functionality:
fmtfor formatted output in testsassertfrom testify for improved test assertionssyncfor mutex-based synchronization in concurrent notification handling
33-33: LGTM! Logging capability properly enabled.The server is correctly configured with logging capability using
server.WithLogging(), which is necessary for testing log notifications.
46-46: LGTM! Handler signatures updated to include RequestContext.The handler function signatures have been correctly updated to include the
server.RequestContextparameter, which enables sending notifications to clients. This aligns with the broader codebase changes mentioned in the AI summary.Also applies to: 59-59
110-151: Consistent notification implementation for prompt handler.The prompt handler follows the same notification pattern as the tool handler, which is good for consistency. The implementation correctly sends progress and logging notifications.
153-189: Consistent notification implementation for resource handler.The resource handler maintains the same notification pattern, ensuring consistent behavior across all handler types.
458-546: Comprehensive test case with proper synchronization.The test case properly:
- Uses mutex for thread-safe notification handling
- Sets up notification callback before starting operations
- Tests log level filtering (INFO filtered out, ERROR received)
- Verifies progress notifications with correct token and values
- Uses proper assertions for all notification fields
548-600: Good test case for verifying notification filtering.This test correctly verifies that when logging level is set to CRITICAL, lower-level notifications (INFO/ERROR) are filtered out, and progress notifications are not sent when no progress token is provided.
695-785: Well-structured resource notification test.The test case for resource notifications follows the same comprehensive pattern as the other notification tests, properly verifying all aspects of the notification system.
dugenkui03
force-pushed
the
support-RequestContext-with-logging-progress-cancellation
branch
from
8b0fffa to
f06b092
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
client/sse_test.go (1)
458-546:⚠️ Potential issueFix critical mutex issue - double lock detected.
The test has excellent notification coverage but contains a critical synchronization bug.
- mu.Lock() + mu.Unlock()This double lock at line 545 can cause deadlocks. The mutex should be unlocked to properly release the lock acquired at line 523.
🧹 Nitpick comments (1)
client/sse_test.go (1)
668-668: Remove debug println statement.The println statement should be removed from production test code.
- println(fmt.Sprintf("%v", result))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/sse_test.go(6 hunks)server/sse.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/sse.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (8)
client/sse_test.go (8)
5-8: LGTM - Appropriate imports for notification testing.The added imports support the new notification testing functionality with proper synchronization and assertions.
33-33: LGTM - Enable logging capabilities for notification testing.Adding
server.WithLogging()is necessary to support the logging notification features being tested.
46-46: LGTM - Handler signature updates align with new RequestContext API.The addition of
requestContext server.RequestContextparameter enables handlers to send notifications to clients, which is essential for the new feature.Also applies to: 59-59
71-109: LGTM - Comprehensive notification testing tool.The implementation provides excellent coverage of notification features:
- Progress notifications with proper progress tracking (0% → 100%)
- Logging notifications at different levels for filter testing
- Proper error handling for notification failures
This tool effectively validates the notification capabilities.
110-151: LGTM - Consistent notification pattern across operation types.The prompt handler implements the same notification pattern as the tool handler, ensuring comprehensive testing of notification support across all MCP operation types.
153-189: LGTM - Completes notification testing coverage.The resource handler implements consistent notification patterns, completing comprehensive test coverage across all MCP operation types (tools, prompts, resources).
548-600: LGTM - Important negative testing for notification filtering.This test ensures notifications are only sent when appropriate (with progress token) and demonstrates proper logging level filtering. The mutex usage is correct here.
695-785: LGTM - Completes comprehensive notification testing suite.This test validates notification support for resource operations, completing thorough coverage across all MCP operation types with proper assertions and synchronization.
dugenkui03
force-pushed
the
support-RequestContext-with-logging-progress-cancellation
branch
from
f06b092 to
2399b78
Compare
dugenkui03
force-pushed
the
support-RequestContext-with-logging-progress-cancellation
branch
from
2399b78 to
c5b90a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/request_session.go (1)
118-118: Track the TODO for future implementation.The TODO indicates plans to implement additional methods like 'roots/list'. Consider creating an issue to track this future work.
Would you like me to create a GitHub issue to track the implementation of additional RequestSession methods?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
README.md(12 hunks)client/inprocess_test.go(3 hunks)client/sse_test.go(6 hunks)examples/custom_context/main.go(1 hunks)examples/dynamic_path/main.go(1 hunks)examples/everything/main.go(10 hunks)examples/typed_tools/main.go(2 hunks)mcptest/mcptest_test.go(1 hunks)server/request_session.go(1 hunks)server/resource_test.go(5 hunks)server/server.go(15 hunks)server/server_race_test.go(7 hunks)server/server_test.go(17 hunks)server/session_test.go(9 hunks)server/sse.go(3 hunks)server/sse_test.go(3 hunks)server/stdio_test.go(1 hunks)server/streamable_http_test.go(2 hunks)server/typed_tools_handler_func.go(1 hunks)server/typed_tools_handler_func_test.go(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- examples/typed_tools/main.go
- server/resource_test.go
- examples/dynamic_path/main.go
- examples/custom_context/main.go
- server/typed_tools_handler_func.go
- server/sse.go
- mcptest/mcptest_test.go
- server/server_race_test.go
- server/sse_test.go
- server/stdio_test.go
- client/inprocess_test.go
- server/streamable_http_test.go
- server/server_test.go
- server/typed_tools_handler_func_test.go
- server/session_test.go
- server/server.go
- examples/everything/main.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
client/sse_test.go (4)
server/server.go (1)
WithLogging(265-269)server/request_session.go (1)
RequestSession(13-17)mcp/types.go (21)
LoggingLevelInfo(689-689)LoggingLevelError(692-692)Content(782-784)TextContent(788-793)TextContent(795-795)Resource(590-605)ReadResourceRequest(526-538)ResourceContents(640-642)TextResourceContents(644-652)TextResourceContents(654-654)JSONRPCNotification(333-336)MethodNotificationMessage(73-73)MethodNotificationProgress(77-77)InitializeRequest(384-393)Params(177-177)Implementation(463-466)SetLevelRequest(671-679)LoggingLevelWarning(691-691)Meta(135-147)ProgressToken(128-128)LoggingLevelCritical(693-693)client/sse.go (1)
NewSSEMCPClient(25-33)
🪛 markdownlint-cli2 (0.17.2)
README.md
95-95: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
95-95: Link fragments should be valid
Expected: #requestsession; Actual: #RequestSession
(MD051, link-fragments)
546-546: Hard tabs
Column: 1
(MD010, no-hard-tabs)
547-547: Hard tabs
Column: 1
(MD010, no-hard-tabs)
548-548: Hard tabs
Column: 1
(MD010, no-hard-tabs)
549-549: Hard tabs
Column: 1
(MD010, no-hard-tabs)
550-550: Hard tabs
Column: 1
(MD010, no-hard-tabs)
552-552: Hard tabs
Column: 1
(MD010, no-hard-tabs)
553-553: Hard tabs
Column: 1
(MD010, no-hard-tabs)
554-554: Hard tabs
Column: 1
(MD010, no-hard-tabs)
555-555: Hard tabs
Column: 1
(MD010, no-hard-tabs)
557-557: Hard tabs
Column: 1
(MD010, no-hard-tabs)
558-558: Hard tabs
Column: 1
(MD010, no-hard-tabs)
559-559: Hard tabs
Column: 1
(MD010, no-hard-tabs)
560-560: Hard tabs
Column: 1
(MD010, no-hard-tabs)
561-561: Hard tabs
Column: 1
(MD010, no-hard-tabs)
562-562: Hard tabs
Column: 1
(MD010, no-hard-tabs)
563-563: Hard tabs
Column: 1
(MD010, no-hard-tabs)
564-564: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (3)
server/request_session.go (1)
1-118: Well-structured implementation of RequestSession!The RequestSession provides a clean interface for sending notifications to clients. The capability checks and error handling are properly implemented.
client/sse_test.go (2)
70-108: Comprehensive test handlers for notification functionality!The handlers correctly demonstrate sending both progress and logging notifications at different log levels. Good test coverage for the RequestSession functionality.
Also applies to: 109-150, 152-188
457-546: Excellent test coverage for notification scenarios!The test cases thoroughly validate:
- Progress notifications with tokens
- Log level filtering
- Notification content and metadata
- Edge cases where notifications should not be sent
The use of mutexes for thread-safe notification collection is appropriate.
Also applies to: 548-600, 602-690, 692-782
dugenkui03
force-pushed
the
support-RequestContext-with-logging-progress-cancellation
branch
from
7257736 to
ef53de3
Compare
dugenkui03
force-pushed
the
support-RequestContext-with-logging-progress-cancellation
branch
from
ef53de3 to
51fbd1d
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
♻️ Duplicate comments (2)
README.md (2)
95-95: Fix anchor link case mismatch.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
95-95: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
95-95: Link fragments should be valid
Expected: #requestsession; Actual: #requestSession(MD051, link-fragments)
534-567: Fix formatting and typo issues in RequestSession section.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
546-546: Hard tabs
Column: 1(MD010, no-hard-tabs)
547-547: Hard tabs
Column: 1(MD010, no-hard-tabs)
548-548: Hard tabs
Column: 1(MD010, no-hard-tabs)
549-549: Hard tabs
Column: 1(MD010, no-hard-tabs)
550-550: Hard tabs
Column: 1(MD010, no-hard-tabs)
552-552: Hard tabs
Column: 1(MD010, no-hard-tabs)
553-553: Hard tabs
Column: 1(MD010, no-hard-tabs)
554-554: Hard tabs
Column: 1(MD010, no-hard-tabs)
555-555: Hard tabs
Column: 1(MD010, no-hard-tabs)
557-557: Hard tabs
Column: 1(MD010, no-hard-tabs)
558-558: Hard tabs
Column: 1(MD010, no-hard-tabs)
559-559: Hard tabs
Column: 1(MD010, no-hard-tabs)
560-560: Hard tabs
Column: 1(MD010, no-hard-tabs)
561-561: Hard tabs
Column: 1(MD010, no-hard-tabs)
562-562: Hard tabs
Column: 1(MD010, no-hard-tabs)
563-563: Hard tabs
Column: 1(MD010, no-hard-tabs)
564-564: Hard tabs
Column: 1(MD010, no-hard-tabs)
🧹 Nitpick comments (3)
client/sse_test.go (3)
70-108: Consider refactoring duplicated notification logic.The notification sending logic (progress + logging) is repeated across multiple handlers. Consider extracting this into a helper function to reduce code duplication.
Example helper function:
func sendTestNotifications(ctx context.Context, requestSession server.RequestSession, prefix string) error { totalProgressValue := float64(100) startMessage := fmt.Sprintf("start %s", prefix) if err := requestSession.SendProgressNotification(ctx, float64(0), &totalProgressValue, &startMessage); err != nil { return err } if err := requestSession.SendLoggingNotification(ctx, mcp.LoggingLevelInfo, map[string]any{ "filtered_log_message": "will be filtered by log level", }); err != nil { return err } if err := requestSession.SendLoggingNotification(ctx, mcp.LoggingLevelError, map[string]any{ "log_message": "log message value", }); err != nil { return err } endMessage := fmt.Sprintf("end %s", prefix) return requestSession.SendProgressNotification(ctx, float64(100), &totalProgressValue, &endMessage) }
457-546: Test implementation looks robust but could benefit from helper functions.The test correctly:
- Uses proper synchronization with mutex
- Sets up notification handlers before starting client
- Validates notification content and metadata
- Uses appropriate timeouts
However, this test pattern is repeated multiple times. Consider extracting common setup/assertion logic into helper functions.
Example helper functions:
func setupNotificationClient(t *testing.T, serverURL string) (*client.SSEMCPClient, *sync.Mutex, *int, **mcp.JSONRPCNotification, *[]*mcp.JSONRPCNotification) { client, err := NewSSEMCPClient(serverURL + "/sse") require.NoError(t, err) mu := &sync.Mutex{} notificationNum := 0 var messageNotification *mcp.JSONRPCNotification progressNotifications := make([]*mcp.JSONRPCNotification, 0) client.OnNotification(func(notification mcp.JSONRPCNotification) { mu.Lock() defer mu.Unlock() if notification.Method == string(mcp.MethodNotificationMessage) { messageNotification = ¬ification } else if notification.Method == string(mcp.MethodNotificationProgress) { progressNotifications = append(progressNotifications, ¬ification) } notificationNum++ }) return client, mu, ¬ificationNum, &messageNotification, &progressNotifications } func assertNotifications(t *testing.T, mu *sync.Mutex, notificationNum int, messageNotification *mcp.JSONRPCNotification, progressNotifications []*mcp.JSONRPCNotification, expectedStartMsg, expectedEndMsg string) { mu.Lock() defer mu.Unlock() assert.Equal(t, 3, notificationNum) assert.NotNil(t, messageNotification) assert.Equal(t, string(mcp.MethodNotificationMessage), messageNotification.Method) assert.Equal(t, "error", messageNotification.Params.AdditionalFields["level"]) assert.Equal(t, map[string]any{"log_message": "log message value"}, messageNotification.Params.AdditionalFields["data"]) assert.Len(t, progressNotifications, 2) // First progress notification assert.Equal(t, string(mcp.MethodNotificationProgress), progressNotifications[0].Method) assert.Equal(t, expectedStartMsg, progressNotifications[0].Params.AdditionalFields["message"]) assert.EqualValues(t, 0, progressNotifications[0].Params.AdditionalFields["progress"]) assert.Equal(t, "progress_token", progressNotifications[0].Params.AdditionalFields["progressToken"]) assert.EqualValues(t, 100, progressNotifications[0].Params.AdditionalFields["total"]) // Second progress notification assert.Equal(t, string(mcp.MethodNotificationProgress), progressNotifications[1].Method) assert.Equal(t, expectedEndMsg, progressNotifications[1].Params.AdditionalFields["message"]) assert.EqualValues(t, 100, progressNotifications[1].Params.AdditionalFields["progress"]) assert.Equal(t, "progress_token", progressNotifications[1].Params.AdditionalFields["progressToken"]) assert.EqualValues(t, 100, progressNotifications[1].Params.AdditionalFields["total"]) }
520-520: Consider reducing sleep duration for faster test execution.The 500ms sleep might be unnecessarily long for notification processing. Consider reducing to 100-200ms or using a more deterministic approach with channels/waitgroups.
-time.Sleep(time.Millisecond * 500) +time.Sleep(time.Millisecond * 100)Also applies to: 596-596, 666-666, 758-758
🛑 Comments failed to post (1)
README.md (1)
534-567: 🛠️ Refactor suggestion
Verify RequestSession example accuracy.
The example code references
testHeaderKeyandtestHeaderFuncKeywhich are not defined in the context of this example. This might confuse readers since these variables aren't available in a typical implementation.Consider updating the example to be more self-contained:
return &mcp.CallToolResult{ Content: []mcp.Content{ mcp.TextContent{ Type: "text", - Text: "context from header: " + ctx.Value(testHeaderKey).(string) + ", " + ctx.Value(testHeaderFuncKey).(string), + Text: "RequestSession notification example completed successfully", }, }, }, nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.return &mcp.CallToolResult{ Content: []mcp.Content{ mcp.TextContent{ Type: "text", Text: "RequestSession notification example completed successfully", }, }, }, nil🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
546-546: Hard tabs
Column: 1(MD010, no-hard-tabs)
547-547: Hard tabs
Column: 1(MD010, no-hard-tabs)
548-548: Hard tabs
Column: 1(MD010, no-hard-tabs)
549-549: Hard tabs
Column: 1(MD010, no-hard-tabs)
550-550: Hard tabs
Column: 1(MD010, no-hard-tabs)
552-552: Hard tabs
Column: 1(MD010, no-hard-tabs)
553-553: Hard tabs
Column: 1(MD010, no-hard-tabs)
554-554: Hard tabs
Column: 1(MD010, no-hard-tabs)
555-555: Hard tabs
Column: 1(MD010, no-hard-tabs)
557-557: Hard tabs
Column: 1(MD010, no-hard-tabs)
558-558: Hard tabs
Column: 1(MD010, no-hard-tabs)
559-559: Hard tabs
Column: 1(MD010, no-hard-tabs)
560-560: Hard tabs
Column: 1(MD010, no-hard-tabs)
561-561: Hard tabs
Column: 1(MD010, no-hard-tabs)
562-562: Hard tabs
Column: 1(MD010, no-hard-tabs)
563-563: Hard tabs
Column: 1(MD010, no-hard-tabs)
564-564: Hard tabs
Column: 1(MD010, no-hard-tabs)
🤖 Prompt for AI Agents
In README.md around lines 534 to 567, the example code for RequestSession uses undefined variables testHeaderKey and testHeaderFuncKey, which can confuse readers. Replace these variables with either defined constants or remove the context.Value calls to make the example self-contained and clear. Ensure the example compiles and runs independently without relying on external undefined variables.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
server/request_session.go (4)
13-15: Consider expanding the struct for future extensibilityThe struct currently only holds a progress token, but given the TODO comment about implementing other methods like 'roots/list', consider adding fields that might be needed for future session features.
29-32: Improve receiver variable naming for consistencyThe receiver variable
exchangedoesn't match the struct nameRequestSession. Consider usingrsorsessionfor better clarity.-func (exchange *RequestSession) IsLoggingNotificationSupported(ctx context.Context) bool { +func (rs *RequestSession) IsLoggingNotificationSupported(ctx context.Context) bool {
67-89: Good progress notification implementation with minor improvementsThe method correctly checks for progress token presence. Consider adding input validation and fixing the receiver variable name.
-func (exchange *RequestSession) SendProgressNotification(ctx context.Context, progress float64, total *float64, message *string) error { - if exchange.progressToken == nil { +func (rs *RequestSession) SendProgressNotification(ctx context.Context, progress float64, total *float64, message *string) error { + if progress < 0 { + return fmt.Errorf("progress cannot be negative") + } + if total != nil && *total < 0 { + return fmt.Errorf("total cannot be negative") + } + + if rs.progressToken == nil { return nil } params := map[string]any{ "progress": progress, - "progressToken": *exchange.progressToken, + "progressToken": *rs.progressToken, }
118-118: Address the TODO comment for completenessThe TODO comment suggests implementing additional methods like 'roots/list'. Consider creating issues to track these future enhancements or implement them as part of this PR if they're needed for the current feature.
Would you like me to help create a plan for implementing the additional methods mentioned in the TODO comment?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/request_session.go(1 hunks)server/server.go(15 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/server.go
🔇 Additional comments (4)
server/request_session.go (4)
1-7: LGTM: Clean package structure and importsThe package declaration and imports are well-organized and appropriate for the functionality.
9-9: Good use of unexported key type for contextUsing an unexported struct type as a context key is a Go best practice that prevents key collisions.
92-116:⚠️ Potential issueFix potential nil pointer dereference in reason parameter
The method has good error handling for request ID validation, but there's an issue with the reason parameter handling on line 107.
-func (exchange *RequestSession) SendCancellationNotification(ctx context.Context, reason *string) error { +func (rs *RequestSession) SendCancellationNotification(ctx context.Context, reason *string) error { requestIDRawValue := ctx.Value(requestIDKey{}) if requestIDRawValue == nil { return fmt.Errorf("invalid requestID") @@ -104,7 +104,7 @@ "requestId": requestID.Value(), } if reason != nil { - params["reason"] = reason + params["reason"] = *reason }Likely an incorrect or invalid review comment.
17-26:✅ Verification successful
Verify progress token pointer handling
The assignment on line 22 takes the address of
requestParamMeta.ProgressToken. Ensure this is correct based on theProgressTokenfield type in theMetastruct.
🏁 Script executed:
#!/bin/bash # Check the definition of ProgressToken in the Meta struct ast-grep --pattern $'type Meta struct { $$$ ProgressToken $_ $$$ }'Length of output: 810
🏁 Script executed:
#!/bin/bash # Locate the ProgressToken type definition rg -n "^type ProgressToken" -A3 -B3 mcp/types.go # Locate RequestSession and its progressToken field rg -n "^type RequestSession" -A5 server/request_session.go rg -n "progressToken" -C2 server/request_session.goLength of output: 1496
ProgressToken pointer assignment is correct
We’ve confirmed that
ProgressTokenis declared as a non‐pointer alias (type ProgressToken anyin mcp/types.go:128), so taking its address (&requestParamMeta.ProgressToken) produces a*mcp.ProgressTokenthat exactly matches theprogressToken *mcp.ProgressTokenfield inRequestSession. No changes are required.
|
@dugenkui03 Can you resolve the conflicts? |
@pottekkat conflicts are fixed |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mcp/prompts.go(1 hunks)mcp/types.go(3 hunks)server/server.go(15 hunks)server/server_test.go(17 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- mcp/prompts.go
- server/server.go
- server/server_test.go
🔇 Additional comments (3)
mcp/types.go (3)
59-69: LGTM: Good type safety improvement for existing notification constants.The explicit typing of existing notification method constants as
MCPMethodimproves type safety and consistency. This is a good refactoring practice.
71-93: LGTM: Well-documented new notification method constants.The three new notification method constants are properly documented with comprehensive comments and specification links. The naming follows the established pattern and the protocol semantics are clearly explained.
602-605: LGTM: Consistent metadata field addition.Adding the
Metafield toReadResourceParamsaligns with the protocol's metadata support and is consistent with similar additions mentioned in other parts of the codebase.
| var ( | ||
| levelToSeverity = func() map[LoggingLevel]int { | ||
| return map[LoggingLevel]int{ | ||
| LoggingLevelEmergency: 0, | ||
| LoggingLevelAlert: 1, | ||
| LoggingLevelCritical: 2, | ||
| LoggingLevelError: 3, | ||
| LoggingLevelWarning: 4, | ||
| LoggingLevelNotice: 5, | ||
| LoggingLevelInfo: 6, | ||
| LoggingLevelDebug: 7, | ||
| } | ||
| }() | ||
| ) | ||
|
|
||
| // Allows is a helper function that decides a message could be sent to client or not according to the logging level | ||
| func (subscribedLevel LoggingLevel) Allows(currentLevel LoggingLevel) (bool, error) { | ||
| if _, ok := levelToSeverity[currentLevel]; !ok { | ||
| return false, fmt.Errorf("illegal message logging level:%s", currentLevel) | ||
| } | ||
| return levelToSeverity[subscribedLevel] >= levelToSeverity[currentLevel], nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix potential bug in logging level validation.
The Allows method has a logical flaw: it only validates that currentLevel exists in the map but doesn't validate subscribedLevel. If an invalid subscribedLevel is passed, the map lookup returns 0 (Emergency severity), which would incorrectly allow all messages.
Apply this fix to validate both logging levels:
-func (subscribedLevel LoggingLevel) Allows(currentLevel LoggingLevel) (bool, error) {
- if _, ok := levelToSeverity[currentLevel]; !ok {
- return false, fmt.Errorf("illegal message logging level:%s", currentLevel)
- }
- return levelToSeverity[subscribedLevel] >= levelToSeverity[currentLevel], nil
-}
+func (subscribedLevel LoggingLevel) Allows(currentLevel LoggingLevel) (bool, error) {
+ if _, ok := levelToSeverity[currentLevel]; !ok {
+ return false, fmt.Errorf("illegal message logging level: %s", currentLevel)
+ }
+ if _, ok := levelToSeverity[subscribedLevel]; !ok {
+ return false, fmt.Errorf("illegal subscribed logging level: %s", subscribedLevel)
+ }
+ return levelToSeverity[subscribedLevel] >= levelToSeverity[currentLevel], nil
+}Note: I also fixed a minor spacing issue in the error message format.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var ( | |
| levelToSeverity = func() map[LoggingLevel]int { | |
| return map[LoggingLevel]int{ | |
| LoggingLevelEmergency: 0, | |
| LoggingLevelAlert: 1, | |
| LoggingLevelCritical: 2, | |
| LoggingLevelError: 3, | |
| LoggingLevelWarning: 4, | |
| LoggingLevelNotice: 5, | |
| LoggingLevelInfo: 6, | |
| LoggingLevelDebug: 7, | |
| } | |
| }() | |
| ) | |
| // Allows is a helper function that decides a message could be sent to client or not according to the logging level | |
| func (subscribedLevel LoggingLevel) Allows(currentLevel LoggingLevel) (bool, error) { | |
| if _, ok := levelToSeverity[currentLevel]; !ok { | |
| return false, fmt.Errorf("illegal message logging level:%s", currentLevel) | |
| } | |
| return levelToSeverity[subscribedLevel] >= levelToSeverity[currentLevel], nil | |
| } | |
| func (subscribedLevel LoggingLevel) Allows(currentLevel LoggingLevel) (bool, error) { | |
| if _, ok := levelToSeverity[currentLevel]; !ok { | |
| return false, fmt.Errorf("illegal message logging level: %s", currentLevel) | |
| } | |
| if _, ok := levelToSeverity[subscribedLevel]; !ok { | |
| return false, fmt.Errorf("illegal subscribed logging level: %s", subscribedLevel) | |
| } | |
| return levelToSeverity[subscribedLevel] >= levelToSeverity[currentLevel], nil | |
| } |
🤖 Prompt for AI Agents
In mcp/types.go around lines 790 to 812, the Allows method currently only
validates if currentLevel exists in the levelToSeverity map but does not
validate subscribedLevel, which can cause incorrect behavior if subscribedLevel
is invalid. To fix this, add a check to ensure subscribedLevel is also a valid
key in levelToSeverity before comparing severities. If subscribedLevel is
invalid, return an error similar to the currentLevel validation. Also, correct
the spacing in the error message format for consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
client/http_test.go (1)
32-59: 💡 Verification agent🧩 Analysis chain
Use the new RequestSession parameter for sending notifications.
The new
RequestSessionparameter is added but not utilized. The handler still uses the oldserver.SendNotificationToClientmethod instead of leveraging the session-based approach. This defeats the purpose of introducing the session parameter.Based on the PR objectives to support log/cancelled/progress notifications, consider updating the implementation to use the session parameter:
func( ctx context.Context, - requestionSession server.RequestSession, + requestSession server.RequestSession, request mcp.CallToolRequest, ) (*mcp.CallToolResult, error) { - server := server.ServerFromContext(ctx) - err := server.SendNotificationToClient( - ctx, - "notifications/progress", - map[string]any{ - "progress": 10, - "total": 10, - "progressToken": 0, - }, - ) + err := requestSession.SendProgress(10, 10) if err != nil { return nil, fmt.Errorf("failed to send notification: %w", err) }Let me verify the available methods on the
RequestSessionstruct:
🏁 Script executed:
#!/bin/bash # Description: Check the available methods on RequestSession struct # Expected: Methods for sending progress, log, and cancellation notifications ast-grep --pattern $'type RequestSession struct { $$$ }' ast-grep --pattern $'func ($_ *RequestSession) $FUNC_NAME($$$) $$$'Length of output: 4746
Replace direct Server.SendNotificationToClient with RequestSession.SendProgressNotification
The test handler still calls
ServerFromContext(...).SendNotificationToClientinstead of using the injectedRequestSession. Update it to:
- Rename the parameter from
requestionSession→requestSession.- Remove the
server := ServerFromContext(ctx)lookup and itsSendNotificationToClientcall.- Call
requestSession.SendProgressNotification(ctx, progress, &total, nil)to send progress via the session.client/http_test.go (around lines 32–59):
- func( - ctx context.Context, - requestionSession server.RequestSession, - request mcp.CallToolRequest, - ) (*mcp.CallToolResult, error) { - server := server.ServerFromContext(ctx) - err := server.SendNotificationToClient( - ctx, - "notifications/progress", - map[string]any{ - "progress": 10, - "total": 10, - "progressToken": 0, - }, - ) + func( + ctx context.Context, + requestSession server.RequestSession, + request mcp.CallToolRequest, + ) (*mcp.CallToolResult, error) { + total := 10.0 + err := requestSession.SendProgressNotification(ctx, 10, &total, nil) if err != nil { return nil, fmt.Errorf("failed to send notification: %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/http_test.go(1 hunks)server/session.go(1 hunks)server/streamable_http.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/session.go
- server/streamable_http.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
client/http_test.go (1)
server/request_session.go (1)
RequestSession(13-15)
There was a problem hiding this comment.
I think the higher level design is good as described in the linked issue.
But, SDK this is a huge breaking change requiring users to change all handlers.
Also, can we make the session capabilities available through the context itself? That would not require any change to existing users, will add the capability and is more idiomatic Go.
That seems like the more elegant approach. Maybe you have already considered this and can give more input as to why you think the current solution is better.
|
@pottekkat thanks for your review. I will convert this PR to draft first, and will reopen it once I have completed update. |
|
Ok @dugenkui03 Thank you for seeing this through. |
Description
Fixes #321.
The SDK currently supports sending any notification anywhere (without even worrying about any restrictions), so this PR is more like a sugar method.
Type of Change
Checklist
MCP Spec Compliance
Additional Information
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests
Refactor