feat(server/sse): Add support for dynamic base paths #214
Conversation
WalkthroughThis change introduces dynamic base path support for the SSE server in the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsserver/sse.go (1)🔇 Additional comments (8)
✨ 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:
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: 4
🧹 Nitpick comments (7)
examples/dynamic_path/main.go (2)
26-35: Avoid hard-coding the outbound baseURL – derive it from the chosen listen address instead
WithBaseURL("http://localhost:8080")will break if the user changes the-addrflag (:9090,0.0.0.0:8081, etc.).
Deriving the value once the flag is parsed prevents “wrong endpoint” bugs in copy-pasted examples.- server.WithBaseURL("http://localhost:8080"), + server.WithBaseURL(fmt.Sprintf("http://localhost%s", addr)),(If TLS might be enabled in the future, you could inspect
addror add a separate-base-urlflag.)
38-40: Router pattern requires Go 1.22 – add a build tag or explicit comment
ServeMuxpatterns that contain{tenant}only compile/run on Go 1.22+.
A short doc-comment or a//go:build go1.22guard will save users on earlier versions from a confusing 404.server/sse.go (3)
331-333: Ensure the initial endpoint event always ends with\n\n
fmt.Fprintf(w, "event: endpoint\ndata: %s\r\n\r\n", endpoint)mixes\r\nand\n.
For consistency with the rest of the stream (eventQueueuses\n\n), prefer\n\n. No functional bug but helps downstream parsers.- fmt.Fprintf(w, "event: endpoint\ndata: %s\r\n\r\n", endpoint) + fmt.Fprintf(w, "event: endpoint\ndata: %s\n\n", endpoint)
478-483: Return errors instead of panicking for library callers
panichere abruptly terminates the program if library users accidentally call the wrong helper.
A safer API is to return an explicit error so callers can decide.-func (s *SSEServer) CompleteSseEndpoint() string { - if s.dynamicBasePathFunc != nil { - panic("CompleteSseEndpoint cannot be used with WithDynamicBasePath. Use dynamic path logic in your router.") - } - return s.baseURL + s.basePath + s.sseEndpoint -} +func (s *SSEServer) CompleteSseEndpoint() (string, error) { + if s.dynamicBasePathFunc != nil { + return "", fmt.Errorf("CompleteSseEndpoint cannot be used together with WithDynamicBasePath") + } + return s.baseURL + s.basePath + s.sseEndpoint, nil +}(Change callers accordingly.)
Panics are still appropriate inside tests, but exported APIs should favour explicit errors.
567-569: ServeHTTP panic could be downgraded to 500 for robustnessIf someone accidentally wires the whole server under a dynamic path, the entire process panics. Returning
http.StatusInternalServerError(or a log +500) avoids bringing down shared servers.server/sse_test.go (2)
946-968:SSEHandlerRequiresDynamicBasePathsilently ignores earlier panic expectationThe first two calls (
_ = sseServer.SSEHandler()) should be wrapped inrequire.NotPanicsto document the intention; otherwise a panic would skip the rest of the test and the later recover logic never triggers, leading to false positives.
1009-1044: Duplicate code for static vs dynamic endpoint assertions – factor helperFour almost-identical blocks differ only by constructor options. Extracting a table-driven sub-test improves readability and future maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/dynamic_path/main.go(1 hunks)server/sse.go(9 hunks)server/sse_test.go(1 hunks)
robert-jackson-glean
force-pushed
the
rwjblue/push-ysoppsqxnkmy
branch
from
887b314 to
15e1a5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
server/sse_test.go (1)
453-469: Improved SSE frame reading with buffered reader loopExcellent change that addresses the previous review feedback about potential truncation issues. Using a buffered reader loop ensures the complete SSE frame is captured reliably, even on slower systems where the frame might be split across multiple TCP packets.
🧹 Nitpick comments (1)
server/sse_test.go (1)
982-1020: Replace simple buffer read with buffered reader for consistencyWhile this test case correctly verifies dynamic base path with full URL configuration, it uses a simple buffered read instead of the more robust buffered reader loop introduced earlier in the file.
Consider replacing the simple buffer read with a buffered reader loop for consistency and to prevent potential SSE frame truncation issues:
- buf := make([]byte, 1024) - n, err := resp.Body.Read(buf) - if err != nil { - t.Fatalf("Failed to read SSE response: %v", err) - } - endpointEvent := string(buf[:n]) + reader := bufio.NewReader(resp.Body) + var endpointEvent strings.Builder + for { + line, err := reader.ReadString('\n') + if err != nil { + t.Fatalf("Failed to read SSE response: %v", err) + } + endpointEvent.WriteString(line) + if line == "\n" || line == "\r\n" { + break // End of SSE frame + } + } + endpointEventStr := endpointEvent.String()Then update the subsequent code to use
endpointEventStrinstead ofendpointEvent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/dynamic_path/main.go(1 hunks)server/sse.go(9 hunks)server/sse_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/dynamic_path/main.go
- server/sse.go
🔇 Additional comments (4)
server/sse_test.go (4)
865-957: Well-structured test for dynamic path mountingThis test case thoroughly verifies the new dynamic base path functionality, correctly testing both the SSE endpoint connection and JSON-RPC message communication with tenant-specific paths.
Note that the test uses Go 1.22+
PathValue()function, which should be mentioned in documentation as a dependency requirement.
958-980: Important safety check for dynamic path configurationGood test ensuring that the API enforces correct usage patterns - SSEHandler and MessageHandler work correctly in both modes, while ServeHTTP properly panics when configured with dynamic paths to prevent misuse.
1022-1057: Comprehensive URL generation test for all configuration combinationsGreat test case that verifies the URL generation logic for all combinations of static vs dynamic paths, with and without base URL, and with full URL usage enabled or disabled. This ensures consistent behavior across all configuration patterns.
1059-1070: Important API contract enforcementGood test verifying that CompleteSseEndpoint correctly panics when used with dynamic base paths, enforcing the API contract and preventing misuse.
robert-jackson-glean
force-pushed
the
rwjblue/push-ysoppsqxnkmy
branch
from
15e1a5e to
252d7d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/sse.go (1)
368-371: Potential edge case in path concatenationWhile path sanitization in
WithDynamicBasePathprevents most issues, the direct concatenation ofbasePath + s.messageEndpointmight still produce unexpected results ifs.messageEndpointdoesn't begin with a slash.Consider using a more robust path joining approach:
- endpointPath := basePath + s.messageEndpoint + // Ensure messageEndpoint starts with a slash for proper concatenation + messageEndpoint := s.messageEndpoint + if !strings.HasPrefix(messageEndpoint, "/") { + messageEndpoint = "/" + messageEndpoint + } + endpointPath := basePath + messageEndpoint
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/dynamic_path/main.go(1 hunks)server/sse.go(9 hunks)server/sse_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/dynamic_path/main.go
- server/sse_test.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: robert-jackson-glean
PR: mark3labs/mcp-go#214
File: server/sse.go:0-0
Timestamp: 2025-04-28T00:14:49.237Z
Learning: The SSE server in mcp-go implements path sanitization within the `WithDynamicBasePath` function that ensures the dynamic base path starts with "/" and has no trailing "/" to prevent double slashes in URL construction.
server/sse.go (1)
Learnt from: robert-jackson-glean
PR: mark3labs/mcp-go#214
File: server/sse.go:0-0
Timestamp: 2025-04-28T00:14:49.237Z
Learning: The SSE server in mcp-go implements path sanitization within the `WithDynamicBasePath` function that ensures the dynamic base path starts with "/" and has no trailing "/" to prevent double slashes in URL construction.
🔇 Additional comments (9)
server/sse.go (9)
36-41: Well-defined type for dynamic base path handlingThe new
DynamicBasePathFunctype is clearly documented and serves an important purpose in enabling multi-tenant and dynamic routing scenarios.
79-80: LGTM - Clear field documentationThe field comment clearly explains the purpose of this function pointer for determining dynamic base paths.
120-136: Good implementation with path sanitizationThe implementation of
WithDynamicBasePathis robust with proper path sanitization that ensures consistent path formats (adding leading slash when missing and removing trailing slashes). This prevents URL construction issues.
359-374: Clean implementation of dynamic path support in endpoint generationThe updated
GetMessageEndpointForClientmethod properly handles both static and dynamic base paths with a clear implementation. The method intelligently applies the dynamic base path function when available.
485-488: Good defensive programming with clear error guidanceThe panic with a clear error message helps prevent misuse of the API by enforcing the correct usage pattern when using dynamic base paths.
501-503: Consistent API enforcement patternConsistent with the implementation in
CompleteSseEndpoint, ensuring that these methods can't be misused with dynamic base paths.
515-571: Excellent handler abstractions with comprehensive documentationThe new
SSEHandlerandMessageHandlermethods are well-designed and thoroughly documented with example usage. They provide clean abstractions for integrating with various HTTP routers and supporting dynamic path segments.
575-577: Strong API consistency enforcementThe panic in
ServeHTTPproperly prevents API misuse and clearly guides users toward the correct approach for dynamic path scenarios.
339-340: LGTM - Updated handleSSE to support dynamic pathsProperly updated to pass the request to
GetMessageEndpointForClient, enabling dynamic base path resolution.
robert-jackson-glean
force-pushed
the
rwjblue/push-ysoppsqxnkmy
branch
from
252d7d8 to
a643f22
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/sse.go (1)
364-366: Consider additional validation for dynamic path usageWhile you check if
dynamicBasePathFuncis non-nil, there's no validation that the returned path is non-empty. An empty path could lead to URL construction issues.if s.dynamicBasePathFunc != nil { basePath = s.dynamicBasePathFunc(r, sessionID) + if basePath == "" { + // Log warning or use default + basePath = "/" + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/dynamic_path/main.go(1 hunks)server/sse.go(9 hunks)server/sse_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/dynamic_path/main.go
- server/sse_test.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: robert-jackson-glean
PR: mark3labs/mcp-go#214
File: server/sse.go:0-0
Timestamp: 2025-04-28T00:14:49.237Z
Learning: The SSE server in mcp-go implements path sanitization within the `WithDynamicBasePath` function that ensures the dynamic base path starts with "/" and has no trailing "/" to prevent double slashes in URL construction.
server/sse.go (1)
Learnt from: robert-jackson-glean
PR: mark3labs/mcp-go#214
File: server/sse.go:0-0
Timestamp: 2025-04-28T00:14:49.237Z
Learning: The SSE server in mcp-go implements path sanitization within the `WithDynamicBasePath` function that ensures the dynamic base path starts with "/" and has no trailing "/" to prevent double slashes in URL construction.
🔇 Additional comments (8)
server/sse.go (8)
36-41: Well-documented type for dynamic base path generationGood definition with clear documentation explaining the purpose and expected behavior of the function type.
79-81: Clean field addition with descriptive commentThe new field for storing the dynamic base path function is well-commented and properly placed in the struct.
120-136: Excellent path sanitization implementationThe implementation correctly normalizes paths by ensuring they start with a slash and don't end with one, preventing potential URL construction issues. This is consistent with the learning from a previous review about preventing double slashes.
359-374: Clean implementation of dynamic endpoint generationThe method correctly handles both static and dynamic paths, with proper fallback and consistent URL construction. The implementation is straightforward and maintains backward compatibility.
485-489: Good enforcement of usage patternsAppropriate panic messages when attempting to use static methods with dynamic paths, guiding users toward the correct pattern. This helps prevent subtle bugs that could arise from mixing the two approaches.
Also applies to: 501-504
515-542: Well-documented handler for SSE endpointThe SSEHandler method has excellent documentation with clear examples that show how to use it with dynamic paths. The examples are particularly helpful for showing the integration with routers.
544-571: Well-documented handler for message endpointSimilar to the SSE handler, the documentation is thorough with relevant examples. The consistent approach between both handlers makes the API intuitive to use.
575-577: Clear enforcement of the correct usage patternThe panic message explicitly directs users to the appropriate methods for dynamic path scenarios, preventing misuse of the API.
This change introduces the ability to mount SSE endpoints at dynamic
paths with variable segments (e.g., `/api/{tenant}/sse`) by adding a new
`WithDynamicBasePath` option and related functionality. This enables
advanced use cases such as multi-tenant architectures or integration
with routers that support path parameters.
Key Features:
* DynamicBasePathFunc: New function type and option
(WithDynamicBasePath) to generate the SSE server's base path
dynamically per request/session.
* Flexible Routing: New SSEHandler() and MessageHandler() methods allow
mounting handlers at arbitrary or dynamic paths using any router
(e.g., net/http, chi, gorilla/mux).
* Endpoint Generation: GetMessageEndpointForClient now supports both
static and dynamic path modes, and correctly generates full URLs when
configured.
* Example: Added examples/dynamic_path/main.go demonstrating dynamic
path mounting and usage.
```go
mcpServer := mcp.NewMCPServer("dynamic-path-example", "1.0.0")
sseServer := mcp.NewSSEServer(
mcpServer,
mcp.WithDynamicBasePath(func(r *http.Request, sessionID string) string {
tenant := r.PathValue("tenant")
return "/api/" + tenant
}),
mcp.WithBaseURL("http://localhost:8080"),
)
mux := http.NewServeMux()
mux.Handle("/api/{tenant}/sse", sseServer.SSEHandler())
mux.Handle("/api/{tenant}/message", sseServer.MessageHandler())
```
robert-jackson-glean
force-pushed
the
rwjblue/push-ysoppsqxnkmy
branch
from
a643f22 to
86cebe8
Compare
| @@ -447,6 +483,9 @@ func (s *SSEServer) GetUrlPath(input string) (string, error) { | |||
| } | |||
|
|
|||
| func (s *SSEServer) CompleteSseEndpoint() string { | |||
| if s.dynamicBasePathFunc != nil { | |||
| panic("CompleteSseEndpoint cannot be used with WithDynamicBasePath. Use dynamic path logic in your router.") | |||
There was a problem hiding this comment.
Curious what the runtime implications of a panic are here. Specifically, if the DynamicBasePathFunc is not implemented correctly (e.g., returning invalid paths or omitting required segments), it could lead to broken endpoints or runtime errors. Do we think that's the correct approach?
There was a problem hiding this comment.
Good point! The spirit of what I'm going for here is basically to error early and loudly if you've misconfigured things with this new feature, but I totally agree that intentionally panicking may not be the best approach for consumers.
I'll update each of the panic scenarios that I added to instead follow the standard go error pattern.
This change introduces the ability to mount SSE endpoints at dynamic paths with variable segments (e.g.,
/api/{tenant}/sse) by adding a newWithDynamicBasePathoption and related functionality. This enables advanced use cases such as multi-tenant architectures or integration with routers that support path parameters.Key Features:
This is an alternate to #121, the key differences are:
Summary by CodeRabbit
New Features
Bug Fixes
Tests