fix: use bufio.Scanner for stdio transport to avoid panic when stdio mcp server outputs a long line #464
Conversation
WalkthroughThe code replaces the use of Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related issues
Possibly related PRs
Suggested reviewers
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 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 (1)
client/transport/stdio.go (1)
170-181: Reading logic correctly implements Scanner pattern, but consider improving error handling.The implementation correctly follows the Scanner pattern:
Scan()returns false on EOF or errorErr()distinguishes between normal EOF and actual errorsText()provides the line content without newlineThis should indeed prevent the panic issue mentioned in the PR.
However, consider using a structured logger instead of
fmt.Printffor error reporting, as this is a library component:- fmt.Printf("Error reading response: %v\n", err) + // Consider using a structured logger here + fmt.Printf("Error reading response: %v\n", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/transport/stdio.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:16.959Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Learnt from: davidleitw
PR: mark3labs/mcp-go#451
File: mcp/tools.go:1192-1217
Timestamp: 2025-06-26T09:38:18.629Z
Learning: In mcp-go project, the maintainer prefers keeping builder pattern APIs simple without excessive validation for edge cases. The WithOutput* functions are designed to assume correct usage rather than defensive programming, following the principle of API simplicity over comprehensive validation.
🔇 Additional comments (3)
client/transport/stdio.go (3)
27-27: LGTM: Field type change is appropriate for the fix.The change from
*bufio.Readerto*bufio.Scanneraligns with the objective to prevent panics during line reading. Scanner provides more robust EOF handling compared to Reader.
42-42: LGTM: Constructor properly updated for Scanner.The initialization correctly uses
bufio.NewScanner(input)instead ofbufio.NewReader(input), maintaining consistency with the field type change.
117-117: LGTM: Subprocess initialization updated correctly.The
spawnCommandmethod properly initializes the stdout field withbufio.NewScanner(stdout), consistent with the type change and fixing approach.
…mcp server outputs a long line Change-Id: Iaaaf44f80d2e49f5275c5f6903c87dcb4dbb53a3
WingT
changed the title
|
I was under the impression that bufio.NewScanner was limited to 64kb which is why we use NewReader instead. @coderabbitai, am I wrong about this? |
|
Tip For best results, initiate chat on the files or code changes. @ezynda3 You’re right: bufio.Scanner defaults to a ~64 KiB max token size. It won’t panic, but Scan() will fail with ErrTooLong on longer lines unless you raise the limit via Scanner.Buffer. If we keep Scanner for stdio, we should call Buffer with a larger max (e.g., a few MiB or configurable) to handle very long lines safely. Otherwise, a streaming approach with bufio.Reader/json.Decoder can avoid a hard cap. Would you like me to propose a small patch that sets Scanner.Buffer with a sane default and a knob to tweak it? ✏️ Learnings added
🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@ezynda3 You’re right: bufio.Scanner defaults to a 64KB max token. It won’t panic, but it will return ErrTooLong for longer lines unless we raise the limit via Scanner.Buffer(initialBuf, maxToken). The current PR uses NewScanner without Buffer, so long lines >64KB will still fail (albeit gracefully). If we stick with Scanner, I suggest setting something like:
Alternatively, using bufio.Reader with ReadBytes('\n') and accumulating when ErrBufferFull occurs also handles arbitrarily long lines without the Scanner limit. Would you prefer I patch this PR to call Scanner.Buffer with a sane default (e.g., 16MiB) and make it configurable? 🏁 Script executed: Length of output: 2203 @ezynda3 You’re not wrong. bufio.Scanner defaults to a 64 KiB max token. Our script confirms this PR uses NewScanner in client/transport/stdio.go (Line 75, Line 183) and there’s no Scanner.Buffer anywhere in the repo, so very long lines would still hit ErrTooLong. If we keep Scanner, we should raise the limit. Minimal change: // In NewIO(...)
scanner := bufio.NewScanner(input)
buf := make([]byte, 256*1024) // 256 KiB initial buffer
scanner.Buffer(buf, 64*1024*1024) // 64 MiB max token
// scanner.Split(bufio.ScanLines) // optional; default is ScanLines
return &Stdio{
stdin: output,
stdout: scanner,
stderr: logging,
}
// In spawnCommand(...)
c.stdout = bufio.NewScanner(stdout)
buf := make([]byte, 256*1024)
c.stdout.Buffer(buf, 64*1024*1024)Alternatively, we can revert to bufio.Reader and accumulate on ErrBufferFull to avoid any hard cap. Preference? ✏️ Learnings added
🧠 Learnings used |
@coderabbitai Is there any way to keep bufio.Reader and avoid panics? |
Description
Fixes #462
Type of Change
Checklist
MCP Spec Compliance
Summary by CodeRabbit