fix(stdio): skip non-JSON lines in ReadBuffer#1762
Conversation
ReadBuffer.readMessage() now loops past lines that throw SyntaxError (e.g., debug output from tsx/nodemon writing to stdout) instead of propagating them to onerror. Lines that parse as JSON but fail JSONRPC schema validation still throw, so genuinely malformed messages still surface. Keeps the existing Buffer-based architecture to preserve UTF-8 correctness across chunk boundaries and the deserializeMessage(string) public signature. Fixes #700 Closes #1225 Co-authored-by: Sahar Shemesh <sahar.shemesh@zoominfo.com>
🦋 Changeset detectedLatest commit: 577fe05 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-tested fix that cleanly skips non-JSON lines while preserving schema validation errors for callers.
Extended reasoning...
Overview
This PR modifies ReadBuffer.readMessage() in packages/core/src/shared/stdio.ts to silently skip lines that throw SyntaxError (from JSON.parse) while still propagating Zod schema validation errors. The implementation converts the existing single-pass if block into a while loop with a targeted try/catch. A changeset and 7 new tests are included.
Security risks
None. The change only affects how non-parseable lines are handled in the stdio read buffer. It does not touch authentication, permissions, cryptographic operations, or external network calls. The narrowly-scoped instanceof SyntaxError check ensures only JSON parse failures are swallowed — schema validation errors still surface via onerror.
Level of scrutiny
This is a low-risk bug fix. The change is small (net +18 lines of logic), touches only one function in one file, maintains the existing JSONRPCMessage | null contract, and does not alter the deserializeMessage signature. Both callers (StdioServerTransport.processReadBuffer and StdioClientTransport.processReadBuffer) use the same while/try/catch/onerror pattern that remains fully compatible. The tests cover the key scenarios: non-JSON prefix/interleaved lines, incomplete JSON preservation, unbalanced braces, invalid-JSON-like lines, whitespace tolerance, and schema-validation-still-throws.
Other factors
No outstanding reviewer comments. The PR cleanly supersedes a previous more invasive attempt (#1225) that had UTF-8 chunk-boundary bugs and a breaking API change. This approach is minimal and conservative — it keeps the existing Buffer-based architecture and just adds the skip logic. The changeset is correctly typed as a patch release.
bhosmer-ant
left a comment
bhosmer-ant
left a comment
There was a problem hiding this comment.
LGTM. The SyntaxError-only catch is the right discriminator — distinguishes "not JSON at all" (skip) from "JSON but not JSON-RPC" (throw, surfaces via onerror). Verified the caller loop in both client and server stdio.ts handles the continue/return null contract correctly.
One tradeoff worth being aware of (noted in the changeset): a server emitting genuinely truncated JSON ({"jsonrpc":"2.0","id":1,"resul + newline) now gets silently dropped instead of surfacing. Unlikely in practice since JSON.stringify()+'\n' is a single write, and schema-validation failures still throw.
Nit: consider adding a test for consecutive empty lines (\n\n) — JSON.parse('') throws SyntaxError so they're skipped correctly, but explicit coverage would be nice.
Good idea, added a test for this! |
There was a problem hiding this comment.
LGTM. Clean, minimal fix with thorough test coverage.
Extended reasoning...
Overview
This PR modifies a single method (ReadBuffer.readMessage()) in packages/core/src/shared/stdio.ts to silently skip non-JSON lines by wrapping the parsing logic in a while loop with a SyntaxError-specific catch. It adds 8 new tests and a changeset file. The deserializeMessage signature is unchanged.
Security risks
None. The change only affects how non-JSON lines are handled in the stdio buffer — they are skipped instead of throwing. Schema validation errors (Zod) still propagate, so malformed JSON-RPC messages continue to surface. No auth, crypto, or permissions code is touched.
Level of scrutiny
Low scrutiny warranted. This is a small, focused bug fix in a utility class. The SyntaxError-only catch is the correct discriminator between "not JSON at all" (skip) and "valid JSON but invalid JSON-RPC" (throw). Both callers (client/stdio.ts:193 and server/stdio.ts:67) already call readMessage() in a while(true) loop with try/catch, so the null/throw contract is fully compatible.
Other factors
- 8 new tests cover empty lines, non-JSON prefixes, interleaved debug output, incomplete JSON preservation, unbalanced braces, invalid-JSON-like lines, whitespace tolerance, and schema validation still throwing.
- The CODEOWNERS file has a default
*catch-all for the typescript-sdk team (no specific owner for stdio files beyond the default). - The author addressed the earlier review nit about adding a consecutive-empty-lines test.
- No outstanding reviewer comments remain unaddressed.
bhosmer-ant
left a comment
bhosmer-ant
left a comment
There was a problem hiding this comment.
Empty-line test added — thanks! LGTM.
There was a problem hiding this comment.
LGTM, clean bug fix with good test coverage.
Extended reasoning...
Overview
This PR modifies ReadBuffer.readMessage() in packages/core/src/shared/stdio.ts to skip non-JSON lines (catching only SyntaxError) instead of propagating them. It adds 8 tests and a changeset file. Total scope: ~30 lines of logic change plus ~80 lines of tests across 3 files.
Security risks
None. The change only affects error handling for malformed input on stdio. Schema validation errors (Zod) still propagate, so valid-JSON-but-invalid-JSON-RPC messages continue to surface via onerror. No auth, crypto, or permissions code is touched.
Level of scrutiny
Low scrutiny warranted. This is a focused bug fix in a non-security-critical parsing layer. The change is mechanical — wrapping existing logic in a while loop with a targeted SyntaxError catch. Both callers (client/stdio.ts and server/stdio.ts) already use a while(true) + try/catch loop around readMessage(), so the contract (JSONRPCMessage | null, throws on schema errors) is fully preserved.
Other factors
- The author addressed the previous review nit (empty lines test was added).
- No outstanding reviewer comments remain.
- No CODEOWNERS file conflicts visible.
- The approach is explicitly simpler than the superseded PR #1225, avoiding the UTF-8 chunk-boundary bug and signature-breaking changes from that PR.
- No bugs were found by the automated bug hunting system.
2 participants
ReadBuffer.readMessage()now silently skips lines that throwSyntaxError(e.g., debug output from hot-reload tools) instead of propagating them toonerror. Lines that parse as JSON but fail JSONRPC schema validation still throw.Motivation and Context
Fixes #700. When running an MCP server under
tsx watchornodemon, the hot-reload tool writes lines likeGracefully restarting...to stdout, which the stdio transport tries to parse as JSON-RPC. This triggersSyntaxError→onerroron every reload, generating noise in telemetry and logs.This supersedes #1225 with a simpler implementation. That PR rewrote
ReadBufferto a string-array architecture, which introduced a UTF-8 chunk-boundary corruption bug and a breaking change to the exporteddeserializeMessagesignature. This PR keeps the existing Buffer-based architecture and just adds a loop + try/catch inreadMessage().Test cases adapted from #1225 — thanks @saharis9988 for the thorough coverage.
How Has This Been Tested?
packages/core/test/shared/stdio.test.tscovering: non-JSON prefix lines, interleaved non-JSON, incomplete JSON preservation, unbalanced braces, invalid-JSON-like lines, whitespace tolerance, and schema-validation-still-throwspnpm --filter @modelcontextprotocol/core test— 461 tests passBreaking Changes
None.
deserializeMessage(line: string)signature unchanged. Callers ofreadMessage()see the sameJSONRPCMessage | nullcontract; they just no longer getSyntaxErrorthrown for non-JSON lines.Types of changes
Checklist
Additional context
Per review feedback on #1225: only
SyntaxError(fromJSON.parse) is caught and skipped. Zod schema validation errors fromJSONRPCMessageSchema.parsestill propagate, so genuinely malformed JSON-RPC messages continue to surface viaonerror.