Add support for Session-specific resources #610
Conversation
WalkthroughAdds session-scoped resources and session-first resolution for resource reads/listing across SSE and StreamableHTTP sessions: new SessionWithResources interface and implementations, a sessionResources store wired into session lifecycle, handlers merge/sort session+global resources with pagination, tests and docs added, and go directive normalized to 1.23.0. (50 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
server/session_test.go (1)
104-111: Update the struct doc to mention resourcesThe leading comment still says this type implements
SessionWithTools, which is misleading now that it wraps resources. Please fix the wording so the doc matches the struct’s purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
go.mod(1 hunks)server/server.go(2 hunks)server/session.go(1 hunks)server/session_test.go(3 hunks)server/sse.go(3 hunks)www/docs/pages/servers/resources.mdx(1 hunks)www/docs/pages/servers/tools.mdx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
server/session.go (1)
server/server.go (1)
ServerResource(65-68)
server/sse.go (2)
server/server.go (1)
ServerResource(65-68)server/session.go (1)
SessionWithResources(43-51)
server/server.go (2)
server/session.go (2)
ClientSessionFromContext(86-91)SessionWithResources(43-51)mcp/types.go (2)
Resource(660-677)Params(177-177)
server/session_test.go (3)
mcp/types.go (7)
JSONRPCNotification(333-336)Resource(660-677)ReadResourceRequest(589-593)ResourceContents(714-716)TextResourceContents(718-728)TextResourceContents(730-730)Params(177-177)server/server.go (3)
ServerResource(65-68)NewMCPServer(334-362)WithToolCapabilities(310-317)server/session.go (2)
ClientSessionFromContext(86-91)SessionWithResources(43-51)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
server/session_test.go (1)
129-156: Consider aligning map copying approach with sessionTestClientWithTools.The implementation correctly uses
maps.Copyfor defensive copying (lines 139, 154). However,sessionTestClientWithToolsuses manual map copying (lines 80-83, 97-100).For consistency across test fixtures, consider either:
- Updating
sessionTestClientWithToolsto also usemaps.Copy, or- Using manual copying here to match the existing pattern
Both approaches are functionally correct, but consistency improves maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/server.go(2 hunks)server/session_test.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
server/server.go (2)
server/session.go (2)
ClientSessionFromContext(86-91)SessionWithResources(43-51)mcp/types.go (2)
Resource(660-677)Params(177-177)
server/session_test.go (4)
mcp/types.go (7)
JSONRPCNotification(333-336)Resource(660-677)ReadResourceRequest(589-593)ResourceContents(714-716)TextResourceContents(718-728)TextResourceContents(730-730)Params(177-177)server/server.go (3)
ServerResource(65-68)NewMCPServer(334-362)WithToolCapabilities(310-317)server/session.go (2)
SessionWithResources(43-51)ClientSessionFromContext(86-91)mcp/resources.go (1)
NewResource(12-23)
🔇 Additional comments (2)
server/server.go (2)
825-872: LGTM! Session resource merging correctly uses URI as key.The merge logic correctly:
- Sorts global resources deterministically by URI
- Merges session resources using URI as the map key (lines 852, 857)
- Re-sorts after merge to maintain consistent ordering
This addresses the concern from the previous review about using
resource.Nameinstead ofresource.URIas the map key. Session resources will now correctly override global resources by URI.
939-964: LGTM! Session-first resource lookup is correctly implemented.The handler correctly implements session-first resolution:
- Checks for session-specific resources first (lines 944-955)
- Falls back to global resources if not found (lines 958-964)
- Uses URI as the lookup key, which is correct per MCP protocol
This mirrors the existing session-first tool lookup pattern in
handleToolCalland maintains consistency with the protocol where resources are identified by URI and tools by name.
|
@coderabbitai generate docstrings |
✅ Actions performedDocstrings generation started. |
|
Caution No docstrings were generated. |
| module github.com/mark3labs/mcp-go | ||
|
|
||
| go 1.23 | ||
| go 1.23.0 |
There was a problem hiding this comment.
go failed to run on macos without adding in this patch version specifier 🤷
|
@coderabbitai generate docstrings |
✅ Actions performedDocstrings generation started. |
|
Caution No docstrings were generated. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/streamable_http_test.go (1)
726-859: Well-structured test that mirrors the SessionWithTools pattern.The test effectively validates the SessionWithResources implementation with proper coverage of:
- Session registration via hooks
- Basic set/get operations
- Thread-safety through concurrent access testing
- State verification after concurrent operations
Optional enhancement: Verify Handler preservation.
The concurrent test (lines 820-829) creates
ServerResourceentries without aHandler, similar to the tools test. While this is consistent, consider adding a test case that verifies Handlers are correctly preserved and retrieved, especially since resource handlers are a core part of the functionality.For example, add a verification after line 812:
// Verify handler is preserved testContents, err := resource.Handler(context.Background(), mcp.ReadResourceRequest{ Params: mcp.ReadResourceParams{URI: "file://test_resource"}, }) if err != nil { t.Errorf("Handler returned error: %v", err) } if len(testContents) != 1 { t.Errorf("Expected 1 content item, got %d", len(testContents)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/streamable_http_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/streamable_http_test.go (4)
server/hooks.go (1)
Hooks(94-121)server/server.go (3)
NewMCPServer(334-362)WithHooks(293-297)ServerResource(65-68)server/streamable_http.go (1)
NewTestStreamableHTTPServer(977-981)mcp/types.go (5)
Resource(660-677)ReadResourceRequest(589-593)ResourceContents(714-716)TextResourceContents(718-728)TextResourceContents(730-730)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/streamable_http_test.go (1)
855-989: Prefer URI keys in session resources map for clarity and consistencyServer resources are typically keyed by URI. In this test, keys like "test_resource" and "final_resource" are names, while the Resource.URI contains "file://...". Using URIs as keys avoids ambiguity and aligns with how resources are commonly keyed elsewhere. Consider this refactor:
@@ - resources := map[string]ServerResource{ - "test_resource": { + resources := map[string]ServerResource{ + "file://test_resource": { Resource: mcp.Resource{ URI: "file://test_resource", Name: "test_resource", Description: "A test resource", MIMEType: "text/plain", }, @@ - if len(retrievedResources) != 1 { + if len(retrievedResources) != 1 { t.Errorf("Expected 1 resource, got %d", len(retrievedResources)) } - if resource, exists := retrievedResources["test_resource"]; !exists { + if resource, exists := retrievedResources["file://test_resource"]; !exists { t.Error("Expected test_resource to exist") } else if resource.Resource.Name != "test_resource" { t.Errorf("Expected resource name test_resource, got %s", resource.Resource.Name) } @@ - resources := map[string]ServerResource{ - fmt.Sprintf("resource_%d", i): { + resources := map[string]ServerResource{ + fmt.Sprintf("file://resource_%d", i): { Resource: mcp.Resource{ URI: fmt.Sprintf("file://resource_%d", i), Name: fmt.Sprintf("resource_%d", i), Description: fmt.Sprintf("Resource %d", i), MIMEType: "text/plain", }, }, } @@ - finalResources := map[string]ServerResource{ - "final_resource": { + finalResources := map[string]ServerResource{ + "file://final_resource": { Resource: mcp.Resource{ URI: "file://final_resource", Name: "final_resource", Description: "Final Resource", MIMEType: "text/plain", }, }, } @@ - if _, exists := retrievedResources["final_resource"]; !exists { + if _, exists := retrievedResources["file://final_resource"]; !exists { t.Error("Expected final_resource to exist") }If the implementation intentionally allows arbitrary keys, consider at least asserting Resource.URI in retrieved values to reduce ambiguity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/streamable_http_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/streamable_http_test.go (6)
server/server.go (4)
NewMCPServer(334-362)WithResourceCapabilities(209-217)ServerResource(65-68)WithHooks(293-297)server/streamable_http.go (2)
NewTestStreamableHTTPServer(977-981)WithHTTPContextFunc(75-79)mcp/types.go (6)
Request(168-171)Resource(660-677)ReadResourceRequest(589-593)ResourceContents(714-716)TextResourceContents(718-728)TextResourceContents(730-730)server/session.go (3)
ClientSessionFromContext(86-91)SessionWithResources(43-51)ClientSession(11-20)server/constants.go (1)
HeaderKeySessionID(5-5)server/hooks.go (1)
Hooks(94-121)
🔇 Additional comments (1)
server/streamable_http_test.go (1)
1188-1194: Helper looks goodMatches postJSON pattern and sets session header. No issues.
|
@jaredly could you fix the merge conflicts in:
I will merge after that |
… session-resources
|
@ezynda3 thanks! |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
server/streamable_http_test.go (1)
649-719: Close responses, assert status, and strengthen the read assertionsThe earlier review feedback is still outstanding: both the list and read requests leak their response bodies, the read path never checks its HTTP status nor the JSON-RPC error field, and the follow-up call reuses JSON-RPC id
2, which makes the trace ambiguous. Please address all of these in one pass. For example:resp, err = postSessionJSON(testServer.URL, sessionID, listResourcesRequest) if err != nil { t.Fatalf("Failed to send list resources request: %v", err) } +defer resp.Body.Close() if resp.StatusCode != http.StatusOK { t.Errorf("Expected status 200, got %d", resp.StatusCode) } @@ -// List resources +// Read resource getResourceRequest := map[string]any{ "jsonrpc": "2.0", - "id": 2, + "id": 3, "method": "resources/read", "params": map[string]any{"uri": "file://test_resource"}, } resp, err = postSessionJSON(testServer.URL, sessionID, getResourceRequest) if err != nil { t.Fatalf("Failed to send list resources request: %v", err) } +if resp.StatusCode != http.StatusOK { + t.Fatalf("Expected status 200, got %d", resp.StatusCode) +} +defer resp.Body.Close() - bodyBytes, _ = io.ReadAll(resp.Body) + bodyBytes, err = io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("Failed to read response body: %v", err) + } var readResponse jsonRPCResponse if err := json.Unmarshal(bodyBytes, &readResponse); err != nil { t.Fatalf("Failed to unmarshal response: %v", err) } + if readResponse.Error != nil { + t.Fatalf("Unexpected error in read response: %+v", readResponse.Error) + } @@ if cmap["uri"] != "file://test_resource" { t.Errorf("Expected content URI file://test_resource, got %v", cmap["uri"]) } + if cmap["text"] != "test content" { + t.Errorf("Expected content text 'test content', got %v", cmap["text"]) + } + if cmap["mimeType"] != "text/plain" { + t.Errorf("Expected mimeType 'text/plain', got %v", cmap["mimeType"]) + }This keeps the test hygienic and verifies the handler output end-to-end.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/server.go(2 hunks)server/session.go(1 hunks)server/streamable_http.go(9 hunks)server/streamable_http_sampling_test.go(3 hunks)server/streamable_http_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/session.go
- server/streamable_http_sampling_test.go
🧰 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/server.goserver/streamable_http.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 (3)
server/streamable_http_test.go (5)
server/server.go (3)
NewMCPServer(335-363)WithResourceCapabilities(203-211)ServerResource(65-68)server/streamable_http.go (2)
NewTestStreamableHTTPServer(1132-1136)WithHTTPContextFunc(77-81)mcp/types.go (4)
Resource(683-700)ResourceContents(737-739)TextResourceContents(741-752)TextResourceContents(754-754)server/session.go (3)
ClientSessionFromContext(93-98)SessionWithResources(43-51)ClientSession(11-20)server/constants.go (1)
HeaderKeySessionID(5-5)
server/server.go (2)
server/session.go (2)
ClientSessionFromContext(93-98)SessionWithResources(43-51)mcp/types.go (1)
Resource(683-700)
server/streamable_http.go (2)
server/server.go (1)
ServerResource(65-68)server/session.go (2)
SessionWithTools(32-40)SessionWithResources(43-51)
⏰ 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
server/server.go
Outdated
| // Get all resource URIs for consistent ordering | ||
| resourceURIs := make([]string, 0, len(s.resources)) | ||
| for uri := range s.resources { | ||
| resourceURIs = append(resourceURIs, uri) | ||
| } | ||
|
|
||
| // Sort the resource URIs for consistent ordering | ||
| sort.Strings(resourceURIs) | ||
|
|
||
| // Add resources in sorted order | ||
| for _, uri := range resourceURIs { | ||
| resources = append(resources, s.resources[uri].resource) | ||
| } | ||
| s.resourcesMu.RUnlock() | ||
|
|
||
| // Sort the resources by name | ||
| sort.Slice(resources, func(i, j int) bool { | ||
| return resources[i].Name < resources[j].Name | ||
| }) | ||
| // Check if there are session-specific resources | ||
| session := ClientSessionFromContext(ctx) | ||
| if session != nil { | ||
| if sessionWithResources, ok := session.(SessionWithResources); ok { | ||
| if sessionResources := sessionWithResources.GetSessionResources(); sessionResources != nil { | ||
| // Override or add session-specific resources | ||
| // We need to create a map first to merge the resources properly | ||
| resourceMap := make(map[string]mcp.Resource) | ||
|
|
||
| // Add global resources first | ||
| for _, resource := range resources { | ||
| resourceMap[resource.URI] = resource | ||
| } | ||
|
|
||
| // Then override with session-specific resources | ||
| for uri, serverResource := range sessionResources { | ||
| resourceMap[uri] = serverResource.Resource | ||
| } | ||
|
|
||
| // Convert back to slice | ||
| resources = make([]mcp.Resource, 0, len(resourceMap)) | ||
| for _, resource := range resourceMap { | ||
| resources = append(resources, resource) | ||
| } | ||
|
|
||
| // Sort again to maintain consistent ordering | ||
| sort.Slice(resources, func(i, j int) bool { | ||
| return resources[i].URI < resources[j].URI | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Apply pagination |
There was a problem hiding this comment.
Restore name-based ordering before pagination
listByPagination binary-searches by GetName(), but we now hand it a slice sorted by URI. As soon as resource names and URIs diverge, pagination cursors jump or repeat rows. Please sort by name (with URI as a tie-breaker) after merging session resources so the ordering matches the pagination contract.
- // Sort again to maintain consistent ordering
- sort.Slice(resources, func(i, j int) bool {
- return resources[i].URI < resources[j].URI
- })
}
}
}
+ // Sort resources by name so pagination cursors (which serialize names) remain valid.
+ sort.Slice(resources, func(i, j int) bool {
+ if resources[i].Name == resources[j].Name {
+ return resources[i].URI < resources[j].URI
+ }
+ return resources[i].Name < resources[j].Name
+ })📝 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.
| // Get all resource URIs for consistent ordering | |
| resourceURIs := make([]string, 0, len(s.resources)) | |
| for uri := range s.resources { | |
| resourceURIs = append(resourceURIs, uri) | |
| } | |
| // Sort the resource URIs for consistent ordering | |
| sort.Strings(resourceURIs) | |
| // Add resources in sorted order | |
| for _, uri := range resourceURIs { | |
| resources = append(resources, s.resources[uri].resource) | |
| } | |
| s.resourcesMu.RUnlock() | |
| // Sort the resources by name | |
| sort.Slice(resources, func(i, j int) bool { | |
| return resources[i].Name < resources[j].Name | |
| }) | |
| // Check if there are session-specific resources | |
| session := ClientSessionFromContext(ctx) | |
| if session != nil { | |
| if sessionWithResources, ok := session.(SessionWithResources); ok { | |
| if sessionResources := sessionWithResources.GetSessionResources(); sessionResources != nil { | |
| // Override or add session-specific resources | |
| // We need to create a map first to merge the resources properly | |
| resourceMap := make(map[string]mcp.Resource) | |
| // Add global resources first | |
| for _, resource := range resources { | |
| resourceMap[resource.URI] = resource | |
| } | |
| // Then override with session-specific resources | |
| for uri, serverResource := range sessionResources { | |
| resourceMap[uri] = serverResource.Resource | |
| } | |
| // Convert back to slice | |
| resources = make([]mcp.Resource, 0, len(resourceMap)) | |
| for _, resource := range resourceMap { | |
| resources = append(resources, resource) | |
| } | |
| // Sort again to maintain consistent ordering | |
| sort.Slice(resources, func(i, j int) bool { | |
| return resources[i].URI < resources[j].URI | |
| }) | |
| } | |
| } | |
| } | |
| // Apply pagination | |
| // Convert back to slice | |
| resources = make([]mcp.Resource, 0, len(resourceMap)) | |
| for _, resource := range resourceMap { | |
| resources = append(resources, resource) | |
| } | |
| } | |
| } | |
| } | |
| // Sort resources by name so pagination cursors (which serialize names) remain valid. | |
| sort.Slice(resources, func(i, j int) bool { | |
| if resources[i].Name == resources[j].Name { | |
| return resources[i].URI < resources[j].URI | |
| } | |
| return resources[i].Name < resources[j].Name | |
| }) | |
| // Apply pagination |
🤖 Prompt for AI Agents
In server/server.go around lines 831 to 879, after merging session-specific
resources into the resources slice you must restore the name-based ordering
expected by listByPagination: sort the final resources slice by
resource.GetName() and use resource.URI as a deterministic tie-breaker (e.g.
compare names first, and if equal compare URIs) so the slice passed into
pagination is ordered by name then URI to prevent cursor jumps or duplicates.
|
@jaredly can you resolve those coderabbit issues? |
| listResult, ok := resp.Result.(mcp.ListResourcesResult) | ||
| _, ok = resp.Result.(mcp.ListResourcesResult) | ||
| assert.True(t, ok) | ||
| assert.NotNil(t, listResult.Resources) |
There was a problem hiding this comment.
this NotNil was erroring because the new listResources code returns a nil slice instead of a zero-length non-nil slice. My understanding is that there is not an important semantic different between the two, so I don't think it makes sense to assert non-nil here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/server_test.go (1)
448-449: Test coverage reduced: Resources field no longer validated.The test previously asserted that
listResult.Resourceswas non-nil. Now it only verifies the result type isListResourcesResultwithout checking the Resources field content. With session-scoped resources introduced in this PR, an empty resources list is likely acceptable in this basic test, but consider whether validation of the Resources field (even if empty) should be retained to ensure the field is properly initialized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/server.go(3 hunks)server/server_test.go(1 hunks)server/streamable_http.go(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/server.go
🧰 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/server_test.goserver/streamable_http.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/server_test.go
🧬 Code graph analysis (2)
server/server_test.go (1)
mcp/types.go (2)
Result(249-253)ListResourcesResult(591-594)
server/streamable_http.go (3)
server/server.go (2)
ServerResource(67-70)ServerTool(55-58)mcp/types.go (1)
JSONRPCNotification(335-338)server/session.go (3)
SessionWithTools(32-40)SessionWithResources(43-51)SessionWithLogging(23-29)
⏰ 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.go (3)
788-819: LGTM: Properly addresses previous concurrency concerns.The
sessionResourcesStoreimplementation correctly usesmaps.Copyto clone the resource map in bothget(lines 799-805) andset(lines 807-813) methods. This prevents callers from mutating shared state after locks are released, which was flagged as critical in previous reviews. The shallow copy is appropriate here sinceServerResourcecontains only value types (struct with strings) and a function pointer, both safe to share.Based on learnings (past review comments on lines 799-813 and 807-813 have been addressed).
835-845: Good consistency: sessionToolsStore also updated with cloning.The
sessionToolsStoremethods were also updated to usemaps.Copyfor cloning in bothgetandsetoperations, maintaining consistency with the newsessionResourcesStoreimplementation and preventing similar concurrency issues.
942-948: LGTM: Session resource methods follow established pattern.The
GetSessionResourcesandSetSessionResourcesmethods correctly delegate to the store's get/set methods, which handle cloning under lock. This mirrors the existingSessionWithToolsimplementation and ensures thread-safe access as required by theSessionWithResourcesinterface contract.As per coding guidelines (thread-safety requirements documented in server/session.go interface comments).
3 participants
Description
This mirrors the session-specific tools functionality. I based it off of the
SessionWithToolsimplementation.Type of Change
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores