v1 stdio buffer limit#2239
Conversation
|
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks — the latest commit addresses my earlier concern by plumbing maxBufferSize through StdioServerParameters and the StdioServerTransport constructor, and I didn't find any new bugs. I'm leaving this for a human since the default 10 MB cap is still a behavior change for existing users with large single-message payloads (they'll now see the transport error and close unless they opt into a higher limit), so a maintainer should confirm the default and whether this needs a changeset/breaking-change note.
Extended reasoning...
Overview
This PR adds a size limit to the stdio ReadBuffer (src/shared/stdio.ts) with a 10 MB default, surfaces overflow as an onerror + transport close in both StdioClientTransport and StdioServerTransport, and exposes a maxBufferSize option via StdioServerParameters and a new options parameter on the server transport constructor. Tests cover the shared buffer behavior and both transports' overflow paths.
Changes since my last review
My previous review flagged that maxBufferSize was dead code because neither transport passed it through. The follow-up commit (9981dc5) addresses this: the client transport now constructs new ReadBuffer({ maxBufferSize: server.maxBufferSize }) and the server transport accepts an options object. The implementation looks correct — processReadBuffer() drains complete messages after each append, so the cap effectively bounds a single in-flight message line, and overflow clears the buffer before throwing.
Security risks
The change is defensive in nature (bounds unbounded memory growth from a misbehaving or malicious peer writing a huge line without a newline). No injection, auth, or data-exposure surface is introduced. The main risk is availability/compatibility, not security: a legitimate >10 MB single JSON-RPC message (e.g. a large base64 tool result) that previously worked will now tear down the transport unless the consumer raises the limit.
Level of scrutiny
This touches the core stdio transport of a widely used SDK and changes default runtime behavior for existing users, so it warrants maintainer-level scrutiny even though the diff is small and the implementation is sound. The choice of the 10 MB default, whether the failure mode (close the whole transport vs. drop the message) is the desired policy, and whether this should ship with a changeset / be called out as a breaking change are product decisions for a human maintainer — the PR description and changeset are currently empty.
Other factors
Test coverage for the new behavior is good across shared, client, and server layers. One minor note: the limit is checked against the total buffered size before draining, so a single data chunk containing several complete messages that together exceed the cap would also trip it — unlikely with real pipe chunk sizes, but worth being aware of.
2 participants
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context