fix(server): surface parse/validation transport errors via onerror in streamable HTTP#1684
Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
Quick follow-up: all required project CI checks are passing, and this PR is ready for code-owner review. One external review check ( |
|
Maintainer follow-up: all required CI jobs for this change are green, but the external Could someone with write access please re-run/cancel that check (or mark it non-blocking for this PR) and proceed with code-owner review when convenient? |
travisbreaks
left a comment
There was a problem hiding this comment.
Heads up: PR #1687 addresses the same root issue (#1395) with a broader approach. Instead of patching individual catch blocks, it moves the onerror call into createJsonErrorResponse() itself, which covers all ~15 call sites at once (not just the two parse failures patched here).
This PR has one advantage over #1687: it preserves the actual caught error object (error instanceof Error ? error : new Error(String(error))) rather than wrapping it in a new Error with the generic JSON-RPC message string. That fidelity matters for logging and observability.
If #1687 lands first, the two catch blocks you patched here would already have onerror coverage via the centralized path, making this PR a no-op. If this lands first, #1687 would need a rebase.
A couple of notes specific to this PR:
- Missing changeset (the changeset bot flagged this).
- The scope is intentionally narrow, covering only two of several silent error paths. Others (invalid Accept header, invalid Content-Type, session validation, protocol version checks) remain uncovered.
I'd suggest coordinating with the author of #1687 to pick one direction. The strongest outcome would be #1687's centralized architecture combined with your pattern of forwarding the real error object.
|
Thanks for the detailed review — this makes sense. I agree we should avoid parallel fixes for the same root issue. I’ll align with #1687’s centralized approach and avoid duplicating effort. My main concern to preserve is forwarding the original caught error object for observability. I’ll follow up on #1687 with that improvement + tests. |
|
Closing this in favor of #1687 to keep one canonical implementation path. I’ll contribute the remaining improvement there: preserving the original caught error object while using the centralized |
Summary
This PR fixes missing
onerrorpropagation inStreamableHTTPServerTransportwhen request parsing/validation fails.What changed
packages/server/src/server/streamableHttp.ts:this.onerror?.(...)whenreq.json()failsthis.onerror?.(...)whenJSONRPCMessageSchema.parse(...)failspackages/server/test/server/streamableHttp.test.tsto verify both paths triggeronerrorWhy
Previously, these transport-level errors could be handled in HTTP responses but were not reported through
onerror, making observability and custom error handling inconsistent.Validation
pnpm --filter @modelcontextprotocol/server test -- test/server/streamableHttp.test.tsCloses #1395