-
Notifications
You must be signed in to change notification settings - Fork 490
feat: Implement Streamable-HTTP Client Basic #168
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
Conversation
WalkthroughThis change introduces a new streamable HTTP transport mechanism for JSON-RPC communication within the client package. It adds the Changes
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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: 3
🧹 Nitpick comments (4)
client/http.go (1)
11-14
: Error message refers to “SSE” while constructing Streamable‑HTTP transportThe constructor returns “failed to create SSE transport”, but the surrounding code and PR terminology call it “streamable‑http”. This can confuse users when debugging.
- return nil, fmt.Errorf("failed to create SSE transport: %w", err) + return nil, fmt.Errorf("failed to create streamable‑http transport: %w", err)client/transport/streamable_http.go (2)
206-228
: Double‑closingresp.Body
& partial SSE detection
SendRequest
defersresp.Body.Close()
(line 179) but hands the same reader tohandleSSEResponse
, whosereadSSE
closes it again (line 286). Closing an already‑closed body is safe but signals oversight.Additionally, only exact
text/event-stream
is recognised. Apply the samestrings.HasPrefix
improvement as above to avoid false negatives.
314-330
: SSE parser drops messages with multiline data / no explicitevent:
The SSE spec allows:
- multiple
data:
lines per event (should be concatenated with “\n”)- events without an
event:
field (default event namemessage
)Current logic overrides
data
on every line and ignores events lackingevent:
– losing information.Suggest accumulating
data
across lines and treating emptyevent
as"message"
before dispatching.Do you want a patched parser implementation?
client/transport/streamable_http_test.go (1)
281-287
: Variableerrors
shadows stdlib package, harming readabilityInside the “MultipleRequests” sub‑test, the slice
errors := make([]error, …)
hides the importederrors
package, which may be needed later and confuses readers.
Rename the slice to e.g.errs
to avoid shadowing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/http.go
(1 hunks)client/transport/streamable_http.go
(1 hunks)client/transport/streamable_http_test.go
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
client/transport/streamable_http_test.go (2)
mcp/types.go (1)
JSONRPCNotification
(192-195)client/transport/streamable_http.go (1)
NewStreamableHTTP
(65-84)
client/transport/streamable_http.go (2)
client/client.go (1)
Client
(16-24)mcp/types.go (1)
JSONRPCNotification
(192-195)
🔇 Additional comments (2)
client/transport/streamable_http.go (1)
163-169
: 🛠️ Refactor suggestionContent‑Type equality check too strict; real servers usually append charset
Accept
/ server reply commonly containsapplication/json; charset=utf-8
.
A strict equality test will mis‑route such responses to the “unexpected content type” branch.- switch resp.Header.Get("Content-Type") { + ct := resp.Header.Get("Content-Type") + switch { + case strings.HasPrefix(ct, "application/json"):Remember to import
strings
.Likely an incorrect or invalid review comment.
client/transport/streamable_http_test.go (1)
205-224
:SendRequestWithTimeout
uses a pre‑canceled context – good, but assert typeThe assertion checks
errors.Is(err, context.Canceled)
which is correct, yet if future implementation wraps the error, this might break. Consider usingcontext.Cause
(Go 1.20+) orerrors.Is
onctx.Err()
for resilience.
No change required, just a note.
Nice, I wonder if we should add more documentation to the README about how to use this |
Nice work! Is the server-side implementation coming soon? |
I'm working on it, but very few minutes on it every day. So it will not be very quick. |
|
This PR implements the basic streamable-http transport for the client side.
This implementation follows the specification, including (but not limited to)
DELETE
requestThe features below are not supported:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests