feat: Add session resource helper functions #617
Conversation
- Add ErrSessionDoesNotSupportResources error constant - Implement AddSessionResource() helper for adding single resource - Implement AddSessionResources() helper for batch resource operations - Implement DeleteSessionResources() helper for removing resources - Auto-register resource capabilities with listChanged=true for session resources - Send notifications/resources/list_changed when resources are modified - Match session tools helper pattern for consistency
WalkthroughAdds a new exported error and APIs on MCPServer to add/delete session-scoped resources (single and bulk), with validation, atomic updates, capability auto-enablement, and conditional async notifications to initialized sessions. Adds comprehensive tests, updates tests to use a SessionWithResources interface, and expands documentation for session resources and session tools. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (2)**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*_test.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (2)server/session_test.go (3)
server/session.go (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (5)
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. Comment |
![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
🧹 Nitpick comments (12)
www/docs/pages/servers/resources.mdx (5)
552-556: Align handler return types across this page (helpers reflect current API).Examples elsewhere on this page still show handlers returning *mcp.ReadResourceResult and using mcp.ResourceContent. The current API (as used here and in tests) returns []mcp.ResourceContents with concrete types like mcp.TextResourceContents. Update all code samples on this page for consistency to avoid confusing users.
567-577: Prefer the current handler signature and concrete contents type.This helper uses func(ctx, req) ([]mcp.ResourceContents, error) and mcp.TextResourceContents, which matches the server API. Ensure earlier sections (Static/Dynamic/Error Handling/Timeout) are updated to the same signature and types.
582-601: Batch helper sample looks good; consider a brief note on overrides.When multiple resources include the same URI, the last one wins (override). Add a one‑liner here mirroring the Important Notes bullet to set correct expectations.
609-637: Call out capability requirements when using the direct interface.Using SessionWithResources.SetSessionResources bypasses the helpers’ auto‑registration of resource capabilities. Add a note to either call server.WithResourceCapabilities(...) at server construction or invoke AddSessionResource(...) once to auto‑register before using the direct setter; otherwise, list_changed notifications won’t be emitted.
641-646: Clarify notification conditions and session init behavior.Tighten the bullets to match implementation and tests.
- - Notifications (`resources/list_changed`) are automatically sent when resources are added/removed - - The server automatically registers resource capabilities when session resources are first added - - Operations are thread-safe and can be called concurrently - - Resources are only available to initialized sessions unless explicitly added before initialization + - Notifications (`resources/list_changed`) are sent only for initialized sessions with `listChanged` enabled + - Resource capabilities are auto-registered (with `listChanged=true`) on the first session resource added via helpers + - Operations are thread-safe and can be called concurrently + - You can add resources before initialization (no notifications until initialized); clients will see them after initializationserver/session_resource_helpers_test.go (7)
47-54: Reduce flakiness: avoid tiny timeouts for channel notifications.Use assert.Eventually to wait for a notification instead of a 100ms select timeout.
- // Check that notification was sent - select { - case notification := <-sessionChan: - assert.Equal(t, "notifications/resources/list_changed", notification.Method) - case <-time.After(100 * time.Millisecond): - t.Error("Expected notification not received") - } + // Check that notification was sent + assert.Eventually(t, func() bool { return len(sessionChan) > 0 }, 2*time.Second, 25*time.Millisecond) + notification := <-sessionChan + assert.Equal(t, "notifications/resources/list_changed", notification.Method)
109-123: Same here: replace second timeout select with Eventually/drain.The 50ms window is brittle under load. Consider using len(sessionChan) to assert exactly one send occurred.
337-344: Use Eventually instead of a 100ms timeout.Same flakiness concern; prefer Eventually to wait for the post‑init notification.
391-417: Good table‑driven coverage; add an assertion for auto‑registered listChanged.Case “no resource capability auto-registers…” should also assert that listChanged was set to true after the first add.
- // If no capability was set, verify it was auto-registered - if server.capabilities.resources == nil { - // After first Add, capability should be auto-registered - assert.NotNil(t, server.capabilities.resources) - } + // If no capability was set, verify it was auto-registered with listChanged=true + if server.capabilities.resources == nil { + t.Fatalf("resources capability unexpectedly nil before add") + } + // After first Add, capability should be present and listChanged true + assert.NotNil(t, server.capabilities.resources) + assert.True(t, server.capabilities.resources.listChanged, "auto-registered resources capability should enable listChanged")
676-686: Don’t assert on error message text; it’s brittle.Match only presence of an error (and optionally method/message type), not the error string.
- // Operation should succeed despite notification failure - require.NoError(t, err) - - // Give some time for the notification timeout - time.Sleep(150 * time.Millisecond) - - // Verify error was logged - assert.NotNil(t, capturedError) - assert.Contains(t, capturedError.Error(), "blocked") + // Operation should succeed despite notification failure + require.NoError(t, err) + // Wait for the async send attempt to fail and be reported via hook + assert.Eventually(t, func() bool { return capturedError != nil }, 2*time.Second, 25*time.Millisecond)
509-603: Concurrency test is solid; optional polish.
- Nice avoidance of require from goroutines.
- Consider t.Parallel at the test level if isolation allows.
- Minor: rename local variable “server” to “srv” across tests to avoid package-name shadowing.
14-27: Reduce duplication with a small helper to create/register sessions.Multiple tests repeat identical session setup/registration. A helper improves readability and maintenance.
// test helper (place near top of file) func newSessionWithResources(t *testing.T, s *MCPServer, initialized bool, buf int) *sessionTestClientWithResources { t.Helper() ch := make(chan mcp.JSONRPCNotification, buf) sess := &sessionTestClientWithResources{ sessionID: "session-1", notificationChannel: ch, initialized: initialized, sessionResources: make(map[string]ServerResource), } require.NoError(t, s.RegisterSession(context.Background(), sess)) return sess }Then use: session := newSessionWithResources(t, server, true, 10)
Also applies to: 82-85, 169-176, 236-238, 293-308, 366-369, 486-505, 520-522
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/session_resource_helpers_test.go(1 hunks)www/docs/pages/servers/resources.mdx(2 hunks)www/docs/pages/servers/tools.mdx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- www/docs/pages/servers/tools.mdx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
server/session_resource_helpers_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
server/session_resource_helpers_test.go
🧬 Code graph analysis (1)
server/session_resource_helpers_test.go (4)
server/server.go (2)
WithResourceCapabilities(205-213)ServerResource(67-70)mcp/types.go (7)
JSONRPCNotification(335-338)ReadResourceRequest(612-616)ResourceContents(737-739)Params(180-180)ReadResourceParams(618-624)Resource(683-700)MCPMethod(15-15)server/errors.go (2)
ErrSessionNotFound(16-16)ErrSessionDoesNotSupportResources(20-20)server/hooks.go (2)
Hooks(94-121)OnErrorHookFunc(58-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: coverage
🔇 Additional comments (2)
server/session_resource_helpers_test.go (2)
196-203: Confirm semantics: should deleting a non‑existent resource emit list_changed?The PR summary says notifications are sent “when resources are modified.” Deleting a missing URI doesn’t modify state. If the implementation only notifies on actual changes, this assertion will fail intermittently. Either:
- Change the code to notify on no‑ops (document it), or
- Update the test to assert no notification on no‑op deletes.
Which behavior do we want?
264-277: LGTM on override verification.Good check that session resources supersede globals and that the session handler is invoked.
Use channel instead of shared variable to avoid race condition when capturing errors from goroutines in the notification error handling test
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
server/session_resource_helpers_test.go (6)
3-12: Reorder imports to match guideline groups (std → third‑party → local).Keeps goimports/consistency: stdlib, then testify, then local mcp package.
Apply:
import ( "context" "fmt" "testing" "time" - "github.com/mark3labs/mcp-go/mcp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/mark3labs/mcp-go/mcp" )As per coding guidelines.
651-664: Make notification error assertion robust; assert hook method instead of error string.String‑matching “channel” is brittle. Capture the method from OnError and assert it’s the expected notification.
-// Set up error tracking with a channel to avoid race conditions -errorChan := make(chan error, 1) +// Set up error tracking with channels to avoid race conditions +errorChan := make(chan error, 1) +methodChan := make(chan mcp.MCPMethod, 1) @@ server.hooks.OnError = []OnErrorHookFunc{ func(ctx context.Context, id any, method mcp.MCPMethod, message any, err error) { select { case errorChan <- err: default: // Channel already has an error, ignore subsequent ones } + select { + case methodChan <- method: + default: + } }, }And validate:
// Wait for the error to be logged select { case capturedError := <-errorChan: // Verify error was logged assert.NotNil(t, capturedError) - assert.Contains(t, capturedError.Error(), "channel") + select { + case m := <-methodChan: + assert.Equal(t, mcp.MCPMethod("notifications/resources/list_changed"), m) + case <-time.After(200 * time.Millisecond): + t.Error("Expected method to be captured by OnError") + } case <-time.After(200 * time.Millisecond): t.Error("Expected error was not logged") }Also applies to: 695-701
109-123: Reduce flakiness from short fixed timeouts; add small wait helpers.100ms/50ms can be tight on CI. Create helper(s) to wait/assert notifications with a configurable timeout and reuse.
Add near the top of the file:
func waitNotification(t *testing.T, ch <-chan mcp.JSONRPCNotification, d time.Duration) (mcp.JSONRPCNotification, bool) { t.Helper() select { case n := <-ch: return n, true case <-time.After(d): return mcp.JSONRPCNotification{}, false } }Then replace hand‑rolled selects with:
n, ok := waitNotification(t, sessionChan, time.Second) require.True(t, ok, "expected notification") assert.Equal(t, "notifications/resources/list_changed", n.Method)…and use the same pattern with require.False for “no notification” checks. Based on learnings.
Also applies to: 177-183, 309-316, 337-343, 448-455, 456-462
524-558: Tiny nit: use struct{} for done channel.Saves allocations and communicates intent (signal only).
-done := make(chan bool) +done := make(chan struct{}) @@ - done <- true + done <- struct{}{}And adjust receivers accordingly.
525-593: Renameerrorschannel to avoid confusion with the stdliberrorspackage.Improves clarity in test output and future imports.
-errors := make(chan error, 100) +errCh := make(chan error, 100) @@ - if err != nil { - errors <- err + if err != nil { + errCh <- err } @@ -close(errors) -for err := range errors { +close(errCh) +for err := range errCh { t.Errorf("Concurrent operation error: %v", err) }
599-601: Drop redundant assertion.len(x) is always ≥ 0. Keep the upper bound check.
-assert.GreaterOrEqual(t, len(sessionResources), 0) assert.LessOrEqual(t, len(sessionResources), 11) // 10 regular + 1 concurrent
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/session_resource_helpers_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
server/session_resource_helpers_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
server/session_resource_helpers_test.go
🧬 Code graph analysis (1)
server/session_resource_helpers_test.go (5)
server/server.go (2)
WithResourceCapabilities(205-213)ServerResource(67-70)mcp/types.go (6)
JSONRPCNotification(335-338)ReadResourceRequest(612-616)Params(180-180)ReadResourceParams(618-624)Resource(683-700)MCPMethod(15-15)mcp/resources.go (1)
NewResource(12-23)server/errors.go (2)
ErrSessionNotFound(16-16)ErrSessionDoesNotSupportResources(20-20)server/hooks.go (2)
Hooks(94-121)OnErrorHookFunc(58-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: coverage
- GitHub Check: test
🔇 Additional comments (2)
server/session_resource_helpers_test.go (2)
523-583: Concurrency test pattern looks solid.Good: no assert/require in goroutines; error funnel via channel; join via done signals; final checks done after all goroutines complete. This aligns with testify’s guidance on avoiding assertions in spawned goroutines.
Also applies to: 590-602
44-66: Tests pass reliably; verify with race detector locally.All targeted tests (
TestAddSessionResource,TestSessionResourcesConcurrency, etc.) pass consistently across multiple runs. The code exhibits good test practices: proper channel handling with timeouts (select/case), comprehensive coverage for single/batch operations, and concurrency testing.The race detector verification cannot be completed in this environment (requires CGO), so please run the stress test locally on your machine:
#!/bin/bash set -euo pipefail go test -race ./server -run 'Session(Resource|Resources)' -count=50 -timeout=5mThis ensures timing-sensitive operations in channels and shared state remain safe under concurrent execution.
- Capture pre-call resource count before AddSessionResource - Assert resource is present and count increased after call - Validate listChanged behavior matches expectations - Fix auto-registration check to properly verify capabilities
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
server/session_resource_helpers_test.go (3)
476-482: Auto-register capability assertion looks good.Post‑add capability presence and listChanged=true are verified correctly; resolves the prior unreachable check concern.
196-203: No-op delete must NOT emit list_changed.Deleting a nonexistent resource should be a no-op; don’t expect a notification. Update the test to assert no message is received.
-// Verify another notification was sent -select { -case notification := <-sessionChan: - assert.Equal(t, "notifications/resources/list_changed", notification.Method) -case <-time.After(100 * time.Millisecond): - t.Error("Expected notification not received") -} +// Verify NO notification was sent (no change) +select { +case <-sessionChan: + t.Error("Unexpected notification for no-op delete") +case <-time.After(150 * time.Millisecond): + // Expected: no notification +}
263-277: Validate merge/override via server list/read APIs, not session map only.This test claims to cover global+session merging but only inspects the session map. Also assert:
- Global-only resources appear in the session’s effective list.
- Overridden global resolves to the session handler via the server Read path.
Proposed additions (adjust API names if they differ):
// Get effective resources (global + session) -sessionResources := session.GetSessionResources() +sessionResources := session.GetSessionResources() // Session should have 2 session-specific resources assert.Len(t, sessionResources, 2) @@ // Test read operations use correct handlers contents, err := sessionResources["test://global1"].Handler(ctx, mcp.ReadResourceRequest{Params: mcp.ReadResourceParams{URI: "test://global1"}}) require.NoError(t, err) assert.Equal(t, "session override content", contents[0].(mcp.TextResourceContents).Text) + +// Also validate via server APIs (merge + override observable to the client) +listed, err := server.ListResources(ctx, session.SessionID()) +require.NoError(t, err) +uris := make(map[string]struct{}, len(listed)) +for _, r := range listed { + uris[r.URI] = struct{}{} +} +assert.Contains(t, uris, "test://global2", "global-only resource should be visible in session listing") + +read, err := server.ReadResource(ctx, session.SessionID(), mcp.ReadResourceParams{URI: "test://global1"}) +require.NoError(t, err) +require.NotEmpty(t, read) +if tc, ok := read[0].(mcp.TextResourceContents); ok { + assert.Equal(t, "session override content", tc.Text, "overridden global should resolve to session handler") +} else { + t.Fatalf("unexpected content type: %T", read[0]) +}To confirm API names before editing, run:
#!/bin/bash # Look for server list/read APIs rg -nP '(?s)^\s*func\s+\(\s*\*\s*MCPServer\s*\)\s+(ListResources|ReadResource)\s*\(' -g '**/*.go' -C2
🧹 Nitpick comments (4)
server/session_resource_helpers_test.go (4)
665-679: Make error-hook assertion robust: capture method + error, not error text.Matching on error substrings is brittle. Capture the method and assert it equals notifications/resources/list_changed; still assert an error was reported.
-// Set up error tracking with a channel to avoid race conditions -errorChan := make(chan error, 1) +// Set up error tracking with a channel to avoid race conditions +type errEvent struct { + method mcp.MCPMethod + err error +} +errorChan := make(chan errEvent, 1) @@ -server.hooks.OnError = []OnErrorHookFunc{ - func(ctx context.Context, id any, method mcp.MCPMethod, message any, err error) { - select { - case errorChan <- err: - default: - // Channel already has an error, ignore subsequent ones - } - }, -} +server.hooks.OnError = []OnErrorHookFunc{ + func(ctx context.Context, id any, method mcp.MCPMethod, message any, err error) { + select { + case errorChan <- errEvent{method: method, err: err}: + default: + // already captured one error + } + }, +} @@ -select { -case capturedError := <-errorChan: - // Verify error was logged - assert.NotNil(t, capturedError) - assert.Contains(t, capturedError.Error(), "channel") +select { +case ev := <-errorChan: + // Verify error was logged and the right notification path was attempted + assert.NotNil(t, ev.err) + assert.Equal(t, mcp.MCPMethod("notifications/resources/list_changed"), ev.method) case <-time.After(200 * time.Millisecond): t.Error("Expected error was not logged") }Also applies to: 709-716
47-53: Deflake notification waits: centralize timeouts and helpers.Repeated short timeouts risk flakes in CI. Add small helpers and use a single timeout value.
Add these helpers near the top of the file:
// test helpers const notifyWait = 200 * time.Millisecond func expectNotification(t *testing.T, ch <-chan mcp.JSONRPCNotification) mcp.JSONRPCNotification { t.Helper() select { case n := <-ch: return n case <-time.After(notifyWait): t.Fatalf("expected notification within %s", notifyWait) return mcp.JSONRPCNotification{} } } func expectNoNotification(t *testing.T, ch <-chan mcp.JSONRPCNotification) { t.Helper() select { case <-ch: t.Fatalf("unexpected notification") case <-time.After(notifyWait): } }Then, replace selects, e.g.:
-select { -case notification := <-sessionChan: - assert.Equal(t, "notifications/resources/list_changed", notification.Method) -case <-time.After(100 * time.Millisecond): - t.Error("Expected notification not received") -} +notification := expectNotification(t, sessionChan) +assert.Equal(t, "notifications/resources/list_changed", notification.Method)And:
-select { -case <-sessionChan: - t.Error("Unexpected additional notification received") -case <-time.After(50 * time.Millisecond): - // Expected: no more notifications -} +expectNoNotification(t, sessionChan)Also applies to: 109-116, 117-123, 177-183, 311-315, 339-343, 468-474
537-603: Optional: use sync.WaitGroup instead of a done channel trio.wg simplifies coordination and avoids boolean sends.
var wg sync.WaitGroup wg.Add(3) go func(){ defer wg.Done(); /* add loop */ }() go func(){ defer wg.Done(); /* delete loop */ }() go func(){ defer wg.Done(); /* add+delete loop */ }() wg.Wait()
48-51: Avoid magic strings for method names.Define a local const and use it everywhere to prevent typos.
const listChangedMethod = "notifications/resources/list_changed"Then replace string literals with listChangedMethod.
Also applies to: 112-113, 180-181, 340-341
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/session_resource_helpers_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
server/session_resource_helpers_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
server/session_resource_helpers_test.go
🧬 Code graph analysis (1)
server/session_resource_helpers_test.go (5)
server/server.go (2)
WithResourceCapabilities(205-213)ServerResource(67-70)mcp/types.go (6)
ReadResourceRequest(612-616)ResourceContents(737-739)Params(180-180)ReadResourceParams(618-624)Resource(683-700)MCPMethod(15-15)mcp/resources.go (1)
NewResource(12-23)server/errors.go (2)
ErrSessionNotFound(16-16)ErrSessionDoesNotSupportResources(20-20)server/hooks.go (2)
Hooks(94-121)OnErrorHookFunc(58-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: coverage
- GitHub Check: test
- Update test to assert no notification is sent when deleting non-existent resource - Replace expectation of notification with assertion that no notification occurs - Fail test if unexpected notification is received
- Exercise server's ListResources and ReadResource handlers with session context - Assert global resources appear in merged list - Verify session resource overrides global resource with same URI - Confirm ReadResource uses session handler for overridden resources - Test that non-overridden global resources still use global handlers
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/session_resource_helpers_test.go (1)
661-722: Consider more robust error assertion.The test correctly validates that operations succeed despite notification failures and that errors are logged via hooks. However, line 713 assumes the error message contains "channel", which may be implementation-dependent.
Consider this more robust assertion:
- assert.Contains(t, capturedError.Error(), "channel") + // Just verify an error was logged; the exact message is implementation-dependent + assert.Error(t, capturedError)Alternatively, if you want to be more specific, check for context timeout or use
errors.Iswith a known error type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/session_resource_helpers_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
server/session_resource_helpers_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
server/session_resource_helpers_test.go
🧬 Code graph analysis (1)
server/session_resource_helpers_test.go (5)
server/server.go (2)
WithResourceCapabilities(205-213)ServerResource(67-70)mcp/types.go (7)
JSONRPCNotification(335-338)ReadResourceRequest(612-616)ResourceContents(737-739)Params(180-180)ReadResourceParams(618-624)Resource(683-700)MCPMethod(15-15)mcp/resources.go (1)
NewResource(12-23)server/errors.go (2)
ErrSessionNotFound(16-16)ErrSessionDoesNotSupportResources(20-20)server/hooks.go (2)
Hooks(94-121)OnErrorHookFunc(58-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: coverage
- GitHub Check: test
🔇 Additional comments (9)
server/session_resource_helpers_test.go (9)
15-66: LGTM!The test correctly validates adding a single session resource, notification behavior, and handler execution.
68-149: LGTM!The test correctly validates batch addition (single notification), resource overwriting, and handler updates.
151-203: LGTM!The test correctly validates resource deletion and properly asserts that no notification is sent when deleting a non-existent resource (lines 196-202), addressing the previous review comment.
279-347: LGTM!The test correctly validates that notifications are not sent to uninitialized sessions and properly resume after initialization.
349-387: LGTM!The test correctly validates deletion behavior on uninitialized sessions (no notification, but operation succeeds).
389-485: LGTM!The test correctly validates capability-dependent notification behavior. The auto-registration assertion has been fixed (lines 435-482) by capturing pre-state and verifying post-state, addressing the previous review comment.
487-519: LGTM!The test correctly validates that operations on unregistered sessions return ErrSessionNotFound.
521-616: LGTM!The test correctly validates thread safety with proper goroutine synchronization. It follows testify best practices by collecting errors in channels and asserting in the main test goroutine, avoiding unsafe require/assert calls from worker goroutines.
Based on learnings
618-659: LGTM!The test correctly validates error handling when sessions do not support per-session resources, checking all three operations.
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 (1)
server/session_resource_helpers_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
server/session_resource_helpers_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
server/session_resource_helpers_test.go
🧬 Code graph analysis (1)
server/session_resource_helpers_test.go (5)
server/server.go (2)
WithResourceCapabilities(205-213)ServerResource(67-70)mcp/types.go (7)
JSONRPCNotification(335-338)ReadResourceRequest(612-616)ResourceContents(737-739)Params(180-180)ReadResourceParams(618-624)Resource(683-700)MCPMethod(15-15)mcp/resources.go (1)
NewResource(12-23)server/errors.go (2)
ErrSessionNotFound(16-16)ErrSessionDoesNotSupportResources(20-20)server/hooks.go (2)
Hooks(94-121)OnErrorHookFunc(58-58)
🪛 GitHub Actions: go
server/session_resource_helpers_test.go
[error] 199-199: Unexpected notification received when deleting non-existent resource.
🔇 Additional comments (1)
server/session_resource_helpers_test.go (1)
321-764: Comprehensive test coverage with proper patterns.The remaining test functions demonstrate excellent practices:
- Proper handling of session lifecycle states (uninitialized, unregistered)
- Capability-driven behavior testing with table-driven approach
- Thread-safety validation via concurrent goroutines with proper synchronization
- Error path coverage including incompatible session types
- Graceful degradation testing (notification failures don't break operations)
The tests follow Go testing best practices and coding guidelines.
- Modified DeleteSessionResources to track if resources were actually deleted - Only send list_changed notification if something was removed - Fixed race condition in TestStreamableHTTP_SessionWithResources by protecting shared state - Added sync.Once to prevent multiple WaitGroup.Done() calls
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/streamable_http_test.go (1)
858-858: Prefer interface assertion to avoid coupling to concrete session typeAssert SessionWithResources in the hook instead of *streamableHttpSession. Keeps the test resilient if the transport/session impl changes. The sync.Once guarding Done is good.
Apply this diff:
- var registeredSession SessionWithResources + var registeredSession SessionWithResources hooks := &Hooks{} var mu sync.Mutex var sessionRegistered sync.WaitGroup var sessionRegisteredOnce sync.Once sessionRegistered.Add(1) hooks.AddOnRegisterSession(func(ctx context.Context, session ClientSession) { - if s, ok := session.(*streamableHttpSession); ok { + if s, ok := session.(SessionWithResources); ok { mu.Lock() registeredSession = s mu.Unlock() sessionRegisteredOnce.Do(func() { sessionRegistered.Done() }) } })Also applies to: 862-862, 866-873
server/session.go (1)
513-529: Avoid unsynchronized reads of capabilities; use a tiny helper with RLockBoth conditions read s.capabilities.resources without locking. Use capabilitiesMu.RLock (or a small helper) to prevent data races under -race.
Apply these diffs:
- if session.Initialized() && s.capabilities.resources != nil && s.capabilities.resources.listChanged { + if session.Initialized() && s.resourcesListChangedEnabled() { // Send notification only to this session if err := s.SendNotificationToSpecificClient(sessionID, "notifications/resources/list_changed", nil); err != nil {- if actuallyDeleted && session.Initialized() && s.capabilities.resources != nil && s.capabilities.resources.listChanged { + if actuallyDeleted && session.Initialized() && s.resourcesListChangedEnabled() { // Send notification only to this session if err := s.SendNotificationToSpecificClient(sessionID, "notifications/resources/list_changed", nil); err != nil {Add this helper (outside these ranges):
// resourcesListChangedEnabled reports whether resources.listChanged notifications are enabled. func (s *MCPServer) resourcesListChangedEnabled() bool { s.capabilitiesMu.RLock() defer s.capabilitiesMu.RUnlock() return s.capabilities.resources != nil && s.capabilities.resources.listChanged }(Optional) Consider a parallel helper for tools to standardize capability reads across the codebase.
Also applies to: 580-596
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/session.go(1 hunks)server/streamable_http_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
server/streamable_http_test.goserver/session.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
server/streamable_http_test.go
🧬 Code graph analysis (2)
server/streamable_http_test.go (2)
server/session.go (2)
SessionWithResources(43-51)ClientSession(11-20)server/hooks.go (1)
Hooks(94-121)
server/session.go (3)
server/server.go (3)
MCPServer(142-169)ResourceHandlerFunc(34-34)ServerResource(67-70)server/errors.go (2)
ErrSessionNotFound(16-16)ErrSessionDoesNotSupportResources(20-20)server/hooks.go (1)
Hooks(94-121)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: coverage
- GitHub Check: test
🔇 Additional comments (3)
server/streamable_http_test.go (1)
961-966: LGTM: race-safe session capture for concurrent Set/GetLock-copying the interface then nil-guarding before invoking SetSessionResources/GetSessionResources avoids racy reads of the shared pointer in goroutines. Matches the thread-safety contract on the interface.
Also applies to: 970-975
server/session.go (2)
465-467: LGTM: thin wrapper aligns with tools helper patternSimple pass-through to AddSessionResources maintains API symmetry with tool helpers.
481-486: Helper verified with proper thread-safe lockingThe
implicitlyRegisterCapabilitieshelper exists atserver/server.go:558–571and correctly guardss.capabilitieswithcapabilitiesMu. It uses double-check locking (RLock for initial check, then Lock before calling the register function), ensuring the lambda insession.goexecutes atomically. No thread-safety issues.
…eSessionResources - Validate Resource.URI is non-empty and conforms to RFC 3986 using url.ParseRequestURI - Return formatted errors for invalid or empty URIs instead of silently inserting them - Skip SetSessionResources call in DeleteSessionResources when no resources were actually deleted - Add comprehensive test for session resource overriding global resources
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
1 participant
Description
Add helper functions for managing session-scoped resources, providing feature parity with session-scoped tools. This enables developers to dynamically manage resources per session with convenient helper functions instead of manual map management.
Type of Change
Checklist
MCP Spec Compliance
Additional Information
What's Changed
ErrSessionDoesNotSupportResourceserror constant for type assertion failuresAddSessionResource()helper for adding a single resource to a sessionAddSessionResources()helper for adding multiple resources to a sessionDeleteSessionResources()helper for removing resources from a sessionlistChanged=truewhen first session resource is addednotifications/resources/list_changednotifications when resources are modified (for initialized sessions with capability enabled)Design Decisions
AddSessionTool,AddSessionTools,DeleteSessionToolsfunctionslistChanged=truefor session resourcesTesting
-raceflagImpact
This is a purely additive change with no breaking changes. Existing code using manual resource management will continue to work unchanged.
Summary by CodeRabbit