fix(mcp): reject a server-initiated request as a streamable-HTTP response - #1054
fix(mcp): reject a server-initiated request as a streamable-HTTP response#1054chiliec wants to merge 2 commits into
Conversation
…onse networkClient.request matched the decoded POST body to the in-flight call on id alone, so a server that answered tools/call with a method-bearing frame produced an empty, error-free result. Apply the same isRequestOrNotification guard the SSE paths already use and return a protocol error naming the method. Fixes Gitlawb#1050 Signed-off-by: Vladimir Babin <vovababin@gmail.com>
Greptile SummaryThis PR prevents a server-initiated JSON-RPC request from being accepted as the response to an in-flight streamable-HTTP call.
Confidence Score: 4/5The PR appears safe to merge; the only identified concern is a non-blocking diagnostic accuracy issue. The protocol guard correctly prevents method-bearing frames from being accepted as responses, and the regression test reaches the intended behavior. The error wording is misleading only when the method-bearing frame is a notification rather than a request. Files Needing Attention: internal/mcp/network_client.go
|
| Filename | Overview |
|---|---|
| internal/mcp/network_client.go | Rejects method-bearing plain HTTP response frames correctly, although the new diagnostic inaccurately labels notifications as requests. |
| internal/mcp/network_client_test.go | Adds a focused regression test that exercises the public CallTool path and verifies the unexpected method is reported. |
Reviews (1): Last reviewed commit: "fix(mcp): reject a server-initiated requ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if message.isRequestOrNotification() { | ||
| return fmt.Errorf("MCP %s expected a response from server %s, got %q request", method, client.server.Name, message.Method) | ||
| } |
There was a problem hiding this comment.
A method-bearing notification without an ID also satisfies isRequestOrNotification(), but this error always calls the frame a “request.” When a server returns a notification where an HTTP response is expected, callers receive a misleading protocol diagnostic. Describe it as a “request or notification,” or more generally as a method-bearing message.
| if message.isRequestOrNotification() { | |
| return fmt.Errorf("MCP %s expected a response from server %s, got %q request", method, client.server.Name, message.Method) | |
| } | |
| if message.isRequestOrNotification() { | |
| return fmt.Errorf("MCP %s expected a response from server %s, got %q request or notification", method, client.server.Name, message.Method) | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Good catch — a method-bearing frame with no id is a notification, not a request. Reworded to expected a response from server X, got method "roots/list" instead in 3dc67b9 so it stays accurate for both.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. WalkthroughThe streamable-HTTP MCP client now rejects server-initiated requests or notifications when a response is expected. A regression test verifies that a ChangesMCP response validation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to The client now returns a protocol error instead of accepting a server request as a tool response. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
A method-bearing frame without an id is a notification, so the error should not call it a request. Signed-off-by: Vladimir Babin <vovababin@gmail.com>
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving. This is exactly the fix #1050 asked for, and the test is the one I hoped someone would write.
The guard goes in before the id match, so a method-bearing frame is rejected on its own terms rather than falling through to an id comparison that happens to fail, and the error names the unexpected method, which is what makes it diagnosable. Reusing isRequestOrNotification() means this path and the stdio path now answer the same question the same way, which was the point of filing it separately rather than leaving it inside the stdio issue.
Verified rather than read. The test passes, and removing the three added lines reproduces the exact symptom I described in the issue:
CallTool() error = nil, result = CallToolResult{Content:nil, IsError:false}
An error-free empty tool result, which is the thing that makes this hard to trace back from a user report. So the guard is load-bearing rather than decorative. internal/mcp is green here, and CI came back 9 of 9 after I released the fork gate.
Your commit message is also more accurate than my issue text: describing the frame by its method rather than calling it a request is the better framing, since a notification hits the same path.
2 participants
Summary
networkClient.request(streamable HTTP) matched the decoded POST body to the in-flight call on id alone. A server that answers atools/callPOST with a method-bearing frame such as{"jsonrpc":"2.0","id":1,"method":"roots/list","params":{}}therefore returnederr = nilwith an empty result, so the caller saw a tool that returned nothing instead of a protocol error.This applies the same
isRequestOrNotification()guard thatdeliverEventMessageanddecodeSSERPCMessagealready use on the SSE paths, before the id match, and returns an error naming the unexpected method:Regression test
TestNetworkClientRejectsServerInitiatedRequestAsResponsedrives anhttptestserver that answerstools/callwith the frame from the issue and assertsCallToolreturns an error mentioning"roots/list". Same shape asTestStdioClientIgnoresServerInitiatedRequestsInPendingResponses, adapted to the HTTP client.Linked issue
Fixes #1050
Checklist
issue-approvedlabel.go build ./...,go vet ./..., andgo test ./...pass locally.gofmtclean.-racewhere relevant).Validation
Go 1.26.6 (from
go.mod), Linux.network_client.gochange stashed, the new test fails withCallTool() error = nil, result = mcp.CallToolResult{Content:[]mcp.Content(nil), IsError:false}, want protocol error; with it restored, it passes.go test ./internal/mcp/... -count=1→ ok;go test ./internal/mcp/ -race -count=1→ ok.go build ./...,go vet ./...,make fmt-check,git diff HEAD --check→ clean.go test ./...: every package passes exceptinternal/sandbox,internal/tools,internal/tui, which fail identically on a cleanmaincheckout in my environment (the clone lives under/tmp, and those tests treat/tmpas a write root). Unrelated to this diff.Summary by CodeRabbit