feat: add request header #143
Conversation
WalkthroughA new Changes
Suggested reviewers
Warning 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 (8)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
🔇 Additional comments (3)
✨ 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 (
|
![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 (2)
server/stdio.go (1)
238-238: Header parameter now included in HandleMessage call.The code has been updated to pass an empty header map to match the new signature. This maintains compatibility while not providing any actual headers from the stdio interface.
Consider allowing headers to be customized in stdio mode by providing a configuration option similar to
WithStdioContextFunc. This could allow testing with headers or implementing specific behaviors based on predefined headers.-response := s.server.HandleMessage(ctx, map[string]string{}, rawMessage) +response := s.server.HandleMessage(ctx, s.getRequestHeaders(), rawMessage)With a corresponding function and field:
func (s *StdioServer) getRequestHeaders() map[string]string { if s.defaultHeaders != nil { return s.defaultHeaders } return map[string]string{} }server/sse.go (1)
357-360: Header extraction from HTTP request.The code correctly extracts headers from the HTTP request, but only uses the first value for each header key.
Consider handling multi-value headers more appropriately. HTTP headers can have multiple values, and currently, only the first one is used.
-for k, v := range r.Header { - header[k] = v[0] -} +// Option 1: Join multiple values (common approach) +for k, values := range r.Header { + header[k] = strings.Join(values, ", ") +} + +// OR Option 2: Use a multi-map if needed elsewhere in the codebase +// This would require changing the Header field type in Request struct
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
mcp/types.go(1 hunks)server/request_handler.go(10 hunks)server/server_test.go(18 hunks)server/sse.go(3 hunks)server/stdio.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/sse.go (2)
examples/custom_context/main.go (1)
MCPServer(100-102)server/server.go (1)
MCPServer(143-159)
🔇 Additional comments (22)
mcp/types.go (1)
89-91: Good addition of header support to the Request struct.The addition of the
Headerfield to theRequeststruct enables passing metadata via headers, which will be useful for implementing business logic in hooks functions as mentioned in the PR objectives.server/request_handler.go (3)
15-17: Good update to include header parameter in HandleMessage signature.This change appropriately adds a
header map[string]stringparameter to the method signature, allowing headers to be passed in for processing.
70-70: Properly initializing Request.Header from parameter.The header parameter is correctly assigned to the
request.Headerfield, ensuring it's available for the initialization flow.
90-90: Consistent header assignment across all request types.Good implementation - consistently applying the header parameter to all request types ensures headers are available throughout the application.
Also applies to: 116-116, 142-142, 168-168, 194-194, 220-220, 246-246, 272-272
server/sse.go (3)
56-64: Improved code formatting for struct fields.The reformatting of variable declarations in the
SSEServerstruct improves readability and consistency.
161-166: Consistent field formatting in constructor.The reformatting of the initialization values in the
NewSSEServerfunction matches the struct definition and improves readability.
362-362: Updated HandleMessage call with header parameter.The code correctly passes the extracted headers to the HandleMessage method, allowing request headers to propagate through the system.
server/server_test.go (15)
142-143: Good implementation of the new header parameterThe header parameter has been properly added to the
HandleMessagecall with an "Authorization" header, which aligns with the PR objective of adding request header support.
316-317: Consistent header implementation in the tools test casesThe header parameter has been properly implemented in both test message handling calls, maintaining consistency across the test case.
Also applies to: 337-338
425-426: Appropriate header implementation in message handling testsThe header parameter follows the same pattern used across the PR, ensuring consistent behavior testing.
465-469: Correctly updated the pagination test casesThe header parameter is appropriately inserted between the context and message parameters.
488-489: Properly updated notification handling testThe header parameter maintains the consistent pattern with the "Authorization" token.
568-569: Correctly updated client notification test caseThe header parameter is correctly implemented for the SendNotificationToClient test.
693-697: Properly updated prompt handling test casesThe header parameter follows the same implementation pattern used throughout the PR.
751-755: Correctly updated error handling testsThe header parameter has been properly integrated into the invalid message test cases.
876-880: Header parameter properly implemented in handler testsThe undefined handlers test cases have been correctly updated with the header parameter.
962-966: Successfully updated capability testsThe header parameter is consistently added to the HandleMessage calls in the capability test cases.
1043-1044: Proper header implementation in instruction testsThe header parameter has been correctly integrated into the instruction test cases.
1092-1097: Correctly updated resource template testsBoth HandleMessage calls in the resource template tests have been updated with the header parameter, maintaining consistency.
Also applies to: 1117-1119
1292-1298: All hook tests properly updatedAll HandleMessage calls in the WithHooks test cases have been correctly updated with the header parameter, ensuring comprehensive testing of the new functionality.
Also applies to: 1301-1305, 1311-1315, 1321-1328
1377-1385: Recovery test cases properly updatedThe header parameter has been correctly added to the HandleMessage call in the recovery test case.
142-1385: Overall implementation is consistent and completeThe PR effectively adds header support to all HandleMessage calls throughout the test file. The implementation consistently uses
map[string]string{"Authorization": "Bearer test"}as the header value, which provides good test coverage for the new header parameter. This change aligns perfectly with the PR objective of enhancing themcp.Requestwith header functionality for business logic in hooks.The implementation maintains the existing behavior of the tests while adding the new header functionality, which is an excellent approach.
🧰 Tools
🪛 golangci-lint (1.64.8)
342-342: S1040: type assertion to the same type: toolsList already has type mcp.JSONRPCMessage
(gosimple)
|
This PR enables the request headers received by the server launched in SSE to be read within the hook function. I have already resolved the conflicts and hope that it can be merged. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Hmm. It seems somewhat odd to have to have a headers for STDIO clients (conceptually headers don't seem to make sense in that context -- at least not to me).
RE: this
Add the request header to mcp.Request so that users can use the header to handle business logic in the hooks function.
I see a lot of wiring in the tests for adding headers arguments but I don't see a new test explicitly confirming that headers is set on the request in the hooks (e.g. the case you actually care about). Maybe I missed it on review? If not, would you mind adding a new test for this explicitly?
rwjblue-glean
added
the
status: needs submitter response
|
I added a testing. I retrieve the token from the header to filter different permissions and allow returning different tools lists. |
pottekkat
added
type: enhancement
|
Is there any update to this? I'd like to get Auth header to differentiate users for my remote servers, and I'd prefer not to use a fork to do so |
|
If the only concern is with the naming of the field (i.e Header) then can we close that by renaming it to something like "RequestMetadata" ? I do agree that "Header" is not apt as this is more of a protocol addition rather than transport. |
Add the request header to mcp.Request so that users can use the header to handle business logic in the hooks function.
Summary by CodeRabbit