-
Notifications
You must be signed in to change notification settings - Fork 348
feat: support sse customize httpClient #190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughA new client option function Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
client/transport/sse.go (2)
49-57
: Consider preserving default transport settingsWhile the proxy implementation works correctly, this approach replaces the entire transport with a new one that only has the proxy setting. Consider preserving other useful transport settings like connection pooling, timeouts, TLS configuration, etc.
func WithProxy(proxy httpproxy.Config) ClientOption { return func(sc *SSE) { - sc.httpClient.Transport = &http.Transport{ - Proxy: func(r *http.Request) (*url.URL, error) { - return proxy.ProxyFunc()(r.URL) - }, - } + // Get the existing transport or create a new one + transport := sc.httpClient.Transport + if transport == nil { + transport = http.DefaultTransport + } + + // Try to preserve existing settings if it's an http.Transport + if t, ok := transport.(*http.Transport); ok { + // Clone the transport and update the proxy + newTransport := t.Clone() + newTransport.Proxy = func(r *http.Request) (*url.URL, error) { + return proxy.ProxyFunc()(r.URL) + } + sc.httpClient.Transport = newTransport + } else { + // If it's not an http.Transport, create a new one + sc.httpClient.Transport = &http.Transport{ + Proxy: func(r *http.Request) (*url.URL, error) { + return proxy.ProxyFunc()(r.URL) + }, + } + } } }
59-63
: Add nil check for transportIt's good practice to validate that the provided transport is not nil to prevent potential nil pointer dereference issues later.
func WithTransport(trans http.RoundTripper) ClientOption { return func(sc *SSE) { + if trans == nil { + return + } sc.httpClient.Transport = trans } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sum
is excluded by!**/*.sum
📒 Files selected for processing (2)
client/transport/sse.go
(2 hunks)go.mod
(1 hunks)
🔇 Additional comments (3)
go.mod (1)
9-9
:✅ Verification successful
Verify dependency compatibility
The new dependencies on golang.org/x/net and golang.org/x/text are appropriate for implementing proxy functionality. However, ensure these specific versions exist and are compatible with your other dependencies.
Also applies to: 15-15
🌐 Web query:
Are golang.org/x/net v0.39.0 and golang.org/x/text v0.24.0 stable releases?
💡 Result:
Stability of golang.org/x/net v0.39.0 and golang.org/x/text v0.24.0
golang.org/x/net v0.39.0:
- The golang.org/x/net module uses semantic versioning. However, the go.mod version is still v0.x.x, meaning it has not reached major version v1. According to Go ecosystem conventions, a module is only considered formally "stable" after reaching v1.0.0[1][8].
- The package is widely used, and version tags (like v0.39.0) are regularly published and maintained, including security fixes[5]. But as the documentation notes, "When a project reaches major version v1 it is considered stable"[1][8].
- Thus, v0.39.0 is not officially marked as a "stable" release, though it is considered mature and is commonly used in production[1][8].
golang.org/x/text v0.24.0:
- Like x/net, golang.org/x/text also uses v0.x.x versioning and has not reached v1.0.0.
- The documentation for golang.org/x/text indicates that it is a widely used supplementary package, supports Unicode updates, and is maintained[6][9]. However, backward compatibility is not strictly guaranteed, and the project is not considered "stable" until it hits v1.0.0[6].
- Therefore, v0.24.0 is not an official "stable" release by Go module conventions, although it is actively used and supported[6][9].
Summary Table
Module Version Officially Stable? Actively Maintained Notes golang.org/x/net v0.39.0 No Yes Pre-v1, stability once v1.0.0 is reached[1][8] golang.org/x/text v0.24.0 No Yes Pre-v1, backward compatibility not guaranteed[6] Conclusion
Neither golang.org/x/net v0.39.0 nor golang.org/x/text v0.24.0 are officially "stable releases" according to Go's semantic versioning policy: both are pre-v1 modules. They are, however, mature, well-maintained, and widely used in production by the Go community[1][6][8]. If you require formal stability guarantees (such as strict semantic versioning and backward compatibility), you should wait for v1.0.0 releases. For practical purposes, current releases are considered reliable for most use cases as long as you review changelogs and test updates accordingly.
Citations:
- 1: https://pkg.go.dev/golang.org/x/net
- 2: https://github.com/golang/net
- 3: https://github.com/kashalls/external-dns-unifi-webhook/actions/runs/14392915841/workflow
- 4: https://sca.analysiscenter.veracode.com/vulnerability-database/security/denial-of-service-dos/go/sid-46227
- 5: https://groups.google.com/g/golang-announce
- 6: https://pkg.go.dev/golang.org/x/text
- 7: https://cloud.google.com/spanner/docs/release-notes
- 8: https://pkg.go.dev/golang.org/x/net/html
- 9: https://github.com/golang/text
- 10: https://deps.dev/go/golang.org%2Fx%2Fnet/v0.0.0-20191004110552-13f9640d40b9/versions
Dependency Stability Verification
Both golang.org/x/net v0.39.0 and golang.org/x/text v0.24.0 are pre‑v1 modules (not formally “stable” per Go’s semantic versioning), but they are mature, widely used, and actively maintained.
• golang.org/x/net v0.39.0: pre‑v1, regularly updated with security fixes
• golang.org/x/text v0.24.0: pre‑v1, supports Unicode updates but may introduce non‑backward‑compatible changes before v1.0.0No code changes required—just ensure your compatibility and integration tests pass when upgrading these dependencies.
client/transport/sse.go (2)
18-18
: Appropriate import for proxy functionalityThe addition of the httpproxy package import is appropriate for the new proxy functionality.
49-63
: Note on option orderingBe aware that WithProxy and WithTransport options will override each other if both are used. The last option applied will determine the final transport configuration. Consider documenting this behavior or providing a more comprehensive option that handles both settings.
@@ -1,15 +1,17 @@ | |||
module github.com/mark3labs/mcp-go | |||
|
|||
go 1.23 | |||
go 1.23.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Note on Go version specificity
Specifying the patch version (1.23.0) is more precise than just the minor version (1.23). However, ensure that this version aligns with your deployment targets, as Go 1.23 would be a future release as of April 2024.
🌐 Web query:
What is the latest stable version of Go currently available?
💡 Result:
Latest Stable Version of Go
The latest stable version of the Go programming language currently available is Go 1.24.2, which was released on April 1, 2025[1][9]. This version includes security and bug fixes for several packages including net/http
, crypto/tls
, go/types
, and others[1].
Go 1.24 is a major release, introducing notable changes such as full support for generic type aliases and several performance improvements in the runtime and toolchain[2][7]. The 1.24.2 release is the most recent minor revision, providing important security updates and bug fixes to ensure stability and security for developers[1][9].
Citations:
- 1: https://go.dev/doc/devel/release
- 2: https://tip.golang.org/doc/go1.24
- 3: https://www.geeksforgeeks.org/future-of-golang/
- 4: https://endoflife.date/go
- 5: https://ssojet.com/blog/unlocking-the-future-of-golang-trends-predictions-and-business-impact-in-2025/
- 6: https://github.com/golang/tools/releases
- 7: https://go.dev/blog/go1.24
- 8: https://antonz.org/which-go/
- 9: https://en.wikipedia.org/wiki/Go_(programming_language)
- 10: https://discussion.fedoraproject.org/t/fedora-doesnt-provide-latest-version-of-go/146930
Upgrade Go version in go.mod to the latest stable release
The go.mod
file currently specifies:
- File: go.mod
Line 3:go 1.23.0
As of April 2025, Go 1.24.2 is the latest stable release, which includes important security patches and performance improvements. Please update the Go version directive to align with supported deployment targets:
--- go.mod
@@ line 3
- go 1.23.0
+ go 1.24.2
📝 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.
go 1.23.0 | |
go 1.24.2 |
client/transport/sse.go
Outdated
@@ -45,6 +46,22 @@ func WithHeaders(headers map[string]string) ClientOption { | |||
} | |||
} | |||
|
|||
func WithProxy(proxy httpproxy.Config) ClientOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just expose a method to modify httpClient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would be in favor of that approach which is way more flexible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated, just expose a httpClient option
8735592
to
37dec5a
Compare
add support for customize SSE client with http transport and http proxy
Summary by CodeRabbit
New Features
Chores