Omit absent JSON-RPC ids instead of emitting null - #6068
Merged
Conversation
MCP narrows base JSON-RPC 2.0 for error responses: schema/2025-11-25 types them as `id?: RequestId` where RequestId = string | number, so null is not representable and an absent id must be encoded by omitting the key. Base JSON-RPC 2.0 section 5 says the id "MUST be Null" when it cannot be determined; MCP deliberately overrides that. This is enforced, not advisory. The reference TypeScript SDK gates every inbound message through JSONRPCErrorResponseSchema, a .strict() object with `id: RequestIdSchema.optional()`, using the throwing parse(). An "id":null body therefore makes a conformant client throw inside its transport before the application sees the error. For session-not-found that is especially costly: clients use code -32001 to trigger automatic session recovery, so they lose recovery, not merely an error message. Add session.HasJSONRPCID as the single definition of an absent id. A plain nil check is insufficient because the transparent proxy threads the incoming id through as raw bytes: json.RawMessage(nil) and a RawMessage holding literal null are both non-nil interfaces that marshal to null. A zero id still round-trips - the predicate tests nil-ness, not zero-ness. Part of #6038. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Apply the rule established in the previous commit to the three hand-built error envelopes in pkg/mcp: classification errors, batch rejections, and filtered tool calls. Each dropped the raw request id straight into the map, so an absent id marshaled to "id":null and made a spec-strict client throw inside its transport. classificationErrorBody is the second site reached with raw bytes -- the transparent proxy threads a json.RawMessage through it -- so it uses session.HasJSONRPCID rather than a nil check. Two tests decoded the id into a tagged struct and asserted Nil, which holds whether the key is absent or null, so they could not observe this bug at all; one of them passed with the bug deliberately reintroduced. Both now decode into a map and assert key absence. Where an id is present, the tests assert verbatim echo against the wire bytes, since a map decode coerces a numeric id to float64 and would drop the guarantee pinned in #5945. Part of #6038. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two sites, with different exposure - worth distinguishing. pkg/ratelimit is a reachable bug. rateLimitHandler gates only on the method being tools/call and has no notification guard, so a well-formed tools/call notification that trips the limiter reaches writeRateLimited with a nil id and emits "id":null at HTTP 429 - precisely the payload that makes a spec-strict client throw inside its transport. A new handler-level test drives the middleware with a notification and pins the key's absence; the pre-existing coverage only exercised the body builder, so it could not observe this. pkg/vmcp/server is defense-in-depth. Tracing every call site of writeModernError and writeModernDenied shows all of them downstream of dispatchModern's `parsed.ID == nil` guard, which answers 202 before any error envelope is built, and vMCP ids arrive from jsonrpc2.ID.Raw() as nil, int64 or string - never raw bytes. So the predicate is always true there today. The writers take `id any`, so guarding them is still right, but no client was receiving a null id from this path. writeModernResult deliberately keeps emitting id unconditionally: MCP requires it on a result response, so omitting would be as wrong as null. A test pins that, since the obvious future refactor is to make it match its siblings. Part of #6038. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The last two hand-built envelopes, neither of which needs the shared
predicate.
The origin middleware's 403 hardcoded "id": nil, so the key is simply
deleted: the request is rejected on its Origin header before any
JSON-RPC body is parsed, so there is no id to echo. This is the case
the MCP transport spec names explicitly when it says the body "has no
id" - it calls out the Origin check by name.
writeSSEErrorEvent holds a typed jsonrpc2.ID, so it gates on
id.IsValid() rather than HasJSONRPCID. That is not merely stylistic:
the shared predicate takes `any`, and a jsonrpc2.ID zero value is a
non-nil interface holding an empty struct, so it would report the id
present and emit "id":{} - neither absent nor a valid RequestId.
HasJSONRPCID now documents that.
The origin test decoded the id into a tagged struct and asserted Nil,
so it could not observe this bug at all; it now decodes into a map and
pins the exact body. The new SSE test asserts the data: framing as well
as the payload, since a bare JSON object in a text/event-stream is
silently discarded by the client (#6037, a neighbouring writer).
Part of #6038.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jhrozek
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
aponcedeleonch,
blkt,
jerm-dro,
rdimitrov,
reyortiz3 and
tgrunnagle
as code owners
July 27, 2026 21:56
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6068 +/- ##
=======================================
Coverage 72.20% 72.20%
=======================================
Files 721 721
Lines 75062 75097 +35
=======================================
+ Hits 54198 54225 +27
- Misses 17003 17006 +3
- Partials 3861 3866 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ChrisJBurns
approved these changes
Jul 27, 2026
ChrisJBurns
left a comment
ChrisJBurns
left a comment
Collaborator
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: mcp-protocol-reviewer, golang-arch-reviewer, test-coverage-reviewer, code-quality-completeness
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| F1 | Missing call-site comment: id.IsValid() vs HasJSONRPCID in writeSSEErrorEvent |
8/10 | LOW | Suggestion |
| F2 | notAcceptableBody deferred fabricated id has no inline TODO |
7/10 | LOW | Suggestion |
Overall
This is a well-motivated, correctly implemented protocol compliance fix. All seven sites now conditionally omit the "id" key using HasJSONRPCID, the predicate correctly handles every wire-realistic id type (Go nil, json.RawMessage(nil), json.RawMessage("null"), json.RawMessage("0"), string, int, and float64 — including zero), and the test suite expansion uses the right map-decode technique to distinguish key absence from present-null throughout.
Two low-severity suggestions follow. Neither is blocking. The call-site comment on writeSSEErrorEvent's id.IsValid() guard (F1) is a future-maintainer safety net: without it, a refactor that "standardizes" that call to session.HasJSONRPCID would silently regress to "id":{} for zero-value IDs — worse than the original bug. The notAcceptableBody TODO (F2) makes the known deferred issue grep-findable without relying solely on the PR description.
The PR's own notes are unusually thorough. The documented deferrals (pkg/authz/handleUnauthorized, the fabricated "server-error" id) are reasonable and correctly scoped. The zero-id preservation is tested explicitly and held by multiple test rows. writeModernResult correctly continues to emit id unconditionally — the result-response schema requires it, unlike the error path — and the new comment pinning that distinction will prevent the obvious future refactor from going wrong.
Additional Findings (not in diff)
[F2 — LOW] notAcceptableBody at pkg/vmcp/server/server.go emits a fabricated "id":"server-error" string. This is correctly deferred (it is a different class of violation from this PR's scope), but the source has no inline TODO comment tracking it. Adding a // TODO: fabricated "server-error" id is non-conforming; fix in separate PR at the notAcceptableBody declaration would make the deferred state grep-findable.
Generated with Claude Code
github-actions
Bot
added
size/L
github-actions
Bot
added
size/L
github-actions
Bot
added
size/L
ChrisJBurns
approved these changes
Jul 28, 2026
11 tasks
9 tasks
jhrozek
added a commit
that referenced
this pull request
Jul 28, 2026
External review of the two preceding commits found no correctness regressions but surfaced a comment that would have led a future contributor to loosen a security guard, and a test hole that let a real regression pass the whole suite. The comment: objectCarriesResult was documented as reporting a non-null "result" field. RawMessage's UnmarshalJSON runs even for a JSON null, so probe.Result holds the 4 bytes "null" whenever the key exists at all and an explicit null counts as carrying a result. That is the fail-closed direction and is deliberate, but anyone adding a null check to match the old wording would have widened the smuggling surface. Say what the code does and why. The test hole: every StructuralRoundTrip row asserts byte-identity for a body needing no filtering, and every bypass test used LF bodies, so nothing exercised a filtered event on a CRLF stream. Hardcoding the replacement line's terminator as "\n" passed the entire suite. TestResponseFilteringWriter_SSE_FilteredEventTerminators covers that and the unterminated trailing event, which also pins the deliberate choice not to fabricate a terminator. Also: carriesResult now has direct coverage, including the null case above and the two deliberate false results (an undecodable prefix, and a nested array we do not recurse into) so neither gets "fixed" into a regression. Three verb-free fmt.Errorf calls become errors.New, the duplicated result probe is extracted, and the cedar-authorizer and request boilerplate repeated across ten SSE tests moves into helpers that assert the parsed request actually reached the context. Deliberately unchanged: the JSON-RPC error code stays 500 (#6039), and an absent id stays omitted rather than null, matching #6068. Refs #6037 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jhrozek
added a commit
that referenced
this pull request
Jul 28, 2026
External review of the two preceding commits found no correctness regressions but surfaced a comment that would have led a future contributor to loosen a security guard, and a test hole that let a real regression pass the whole suite. The comment: objectCarriesResult was documented as reporting a non-null "result" field. RawMessage's UnmarshalJSON runs even for a JSON null, so probe.Result holds the 4 bytes "null" whenever the key exists at all and an explicit null counts as carrying a result. That is the fail-closed direction and is deliberate, but anyone adding a null check to match the old wording would have widened the smuggling surface. Say what the code does and why. The test hole: every StructuralRoundTrip row asserts byte-identity for a body needing no filtering, and every bypass test used LF bodies, so nothing exercised a filtered event on a CRLF stream. Hardcoding the replacement line's terminator as "\n" passed the entire suite. TestResponseFilteringWriter_SSE_FilteredEventTerminators covers that and the unterminated trailing event, which also pins the deliberate choice not to fabricate a terminator. Also: carriesResult now has direct coverage, including the null case above and the two deliberate false results (an undecodable prefix, and a nested array we do not recurse into) so neither gets "fixed" into a regression. Three verb-free fmt.Errorf calls become errors.New, the duplicated result probe is extracted, and the cedar-authorizer and request boilerplate repeated across ten SSE tests moves into helpers that assert the parsed request actually reached the context. Deliberately unchanged: the JSON-RPC error code stays 500 (#6039), and an absent id stays omitted rather than null, matching #6068. Refs #6037 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jhrozek
added a commit
that referenced
this pull request
Jul 28, 2026
* Parse SSE responses per event, not per line The response filter processed a text/event-stream body line by line, but SSE semantics are per event: within one event all "data:" field values concatenate, and a blank line is structural. Processing lines meant we parsed the body differently than the client does, and every such disagreement is a way to smuggle an unfiltered list past the filter (#5257). Five were reachable: - A payload split across two "data:" lines. Each half failed to decode on its own, so both passed through and the client reassembled the full list. - Two JSON-RPC messages as two "data:" fields of one event. Assembling produced two concatenated values, which json.Unmarshal rejects as trailing data, so carriesResult missed it. - Mixed line terminators. SSE permits CR, LF and CRLF interchangeably within one stream; sniffing one convention for the whole body left entire events buried inside what we treated as a single line. - A leading UTF-8 BOM. Clients strip it per the WHATWG decode algorithm before parsing lines, so our "data:" prefix match failed where theirs succeeded. - A Response carrying both error and result. filterListResponse returned early on any error, and the result went out untouched. The reference client's schema strips the stray error and accepts the result. Scan (line, terminator) pairs and group them into events, assemble each event's data payload, then decode and filter once. Every line is re-emitted with the terminator it was scanned with, so the body is reproduced structurally and the old separator reconciliation — which wrote back at most one separator however many it dropped, collapsing multi-event bodies — is deleted rather than patched. A frame that fails closed now carries a correlatable JSON-RPC error instead of an empty data buffer, which per WHATWG is never dispatched and left the client hanging on its own timeout. At the SSE framing layer, every payload a strict client can parse and correlate as this request's Response is now assembled and decoded before filtering. The residual pass-through paths carry only payloads no strict parser accepts: a split inside a string literal leaves a raw control character in a JSON string, which the client's own parser rejects. So the gap is closed by construction rather than by luck. The per-method result-shape fallbacks in filterToolsResponse and filterFindToolResponse are a separate matter, untouched here. Refs #6037 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Tighten SSE filter comments and close test gaps External review of the two preceding commits found no correctness regressions but surfaced a comment that would have led a future contributor to loosen a security guard, and a test hole that let a real regression pass the whole suite. The comment: objectCarriesResult was documented as reporting a non-null "result" field. RawMessage's UnmarshalJSON runs even for a JSON null, so probe.Result holds the 4 bytes "null" whenever the key exists at all and an explicit null counts as carrying a result. That is the fail-closed direction and is deliberate, but anyone adding a null check to match the old wording would have widened the smuggling surface. Say what the code does and why. The test hole: every StructuralRoundTrip row asserts byte-identity for a body needing no filtering, and every bypass test used LF bodies, so nothing exercised a filtered event on a CRLF stream. Hardcoding the replacement line's terminator as "\n" passed the entire suite. TestResponseFilteringWriter_SSE_FilteredEventTerminators covers that and the unterminated trailing event, which also pins the deliberate choice not to fabricate a terminator. Also: carriesResult now has direct coverage, including the null case above and the two deliberate false results (an undecodable prefix, and a nested array we do not recurse into) so neither gets "fixed" into a regression. Three verb-free fmt.Errorf calls become errors.New, the duplicated result probe is extracted, and the cedar-authorizer and request boilerplate repeated across ten SSE tests moves into helpers that assert the parsed request actually reached the context. Deliberately unchanged: the JSON-RPC error code stays 500 (#6039), and an absent id stays omitted rather than null, matching #6068. Refs #6037 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Close two SSE filter bypasses found in review Both are regressions the per-event rewrite introduced, found by review on #6088 and reproduced before fixing. Assembling the event's data values left leading whitespace on the payload. Go's JSON scanner treats only SP, TAB, CR and LF as whitespace, but unicode.IsSpace also covers U+000B, U+000C, U+0085, U+00A0, U+1680 and U+3000 — and the go-sdk client trims its data buffer with exactly bytes.TrimSpace. So a payload led by any of those six was rejected by both DecodeMessage and carriesResult while the client parsed it happily, and the event fell through to the undecodable branch with the unfiltered list intact. Trim on assembly instead, which aligns us with the client's own normalization: all six now filter correctly rather than merely failing closed. The second is a coverage regression rather than a new hole. Processing per event means one malformed data value makes the whole event skip filtering, where the old per-line loop filtered each line independently. A result-bearing value sitting after a malformed one in the same event therefore passed through raw. Probe each data value on its own before conceding the event needs no filtering. This only ever turns a pass-through into a fail-closed envelope, never the reverse, so it cannot reintroduce the split-payload bypass the rewrite closed. Tests for both, plus the two the review asked for: a strict WHATWG-grammar SSE parser so the error-path assertions test delivery rather than bytes — conformant payloads that no client dispatches are the whole of #6037 — and the fail-closed invariant for the rest of the stream after a mid-stream failure, which nothing pinned before. That last test asserts an authorized tool survives the second event, not just that the denied one is absent. Absence alone would also hold if the loop truncated the stream, which is the regression the test exists to catch; verified against that mutation specifically. Refs #6037 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Match the client's SSE line model, not the spec's Panel review on #6088 found the rewrite parses less than the client that consumes these streams, and where the two disagree an upstream can put a list through the gap. go-sdk's scanEvents — used by ToolHive's own vMCP and mcpcompat stacks — reads lines with ReadBytes('\n'), so a lone CR is not a terminator, and it applies TrimSpace to each data value rather than to the assembled buffer. The scanner here did the opposite on both counts, following the WHATWG grammar instead. A bare CR inside a data payload therefore split a line for us and not for the client: our second piece lost its "data:" prefix, the assembled payload was truncated, nothing decoded, and the body went out verbatim with the list intact. main filtered that body, so this was a regression against it. Separately, one of the six whitespace bytes Go's JSON scanner rejects but unicode.IsSpace accepts, sitting at an interior value boundary, survived our single whole-buffer trim while the client stripped it per value. main leaked that one too. Scan on LF only and trim each value before joining. That parses a superset of what we saw before, so it can only decode and filter more, never less. A browser EventSource does treat a lone CR as a terminator and will mis-frame a pass-through body carrying one — but identically to how it would mis-frame the backend's own bytes, since the output stays byte-preserving, so the worst case is an unparseable payload rather than a disclosure. Under LF-only scanning a CRLF line's CR lives in the line text, not the terminator, so replacing a data line would have silently downgraded it to bare LF. The CR is re-attached when a line is replaced. The test oracle had the same bug as the code: it reimplemented WHATWG line splitting, so the suite agreed with the implementation by construction and could not have caught any of this. It now models the go-sdk client, which is the reading that decides whether a payload reaches a caller. Also from the review: the fail-closed envelope inherited the offending event's name, and go-sdk skips events not named "message", so on a named event the #6037 hang survived the fix that was meant to end it. And an upstream error carrying an explicit "result":null lost its real code to our generic envelope; a null result cannot carry a list, so that check now exempts it. Three comments asserted things about go-sdk that reading it disproves, including the one that made the trim granularity look safe. Corrected. Refs #6037 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Seven hand-built JSON-RPC error envelopes dropped the raw request id straight into a
map[string]any, so an absent id marshaled to"id":null. MCP does not allow that, and the reference client enforces it.schema/2025-11-25types themid?: RequestIdwhereRequestId = string | number.nullis not in the union, and the key is optional only on the error response — it stays required onJSONRPCResultResponse. Base JSON-RPC 2.0 §5 says the id "MUST be Null" when it cannot be determined; MCP deliberately overrides that, because its schema cannot express null. Both transport pages say the body "has noid", once specifically for the 403 Origin-check case. So an absent id is encoded by omitting the key.JSONRPCErrorResponseSchemais a.strict()object withid: RequestIdSchema.optional()— which admitsundefinedbut notnull— and it gates every inbound message throughparse(), the throwing variant, in all five client transports. An"id":nullbody makes a conformant client throw inside its transport before the application ever sees the error. For session-not-found that is especially costly: clients use code-32001to trigger automatic session recovery, so they lose recovery, not merely an error message.rateLimitHandlergates only on the method beingtools/calland has no notification guard, so a rate-limitedtools/callnotification reachedwriteRateLimitedwith a nil id and emitted"id":nullat HTTP 429. This was verified end-to-end through the realParsingMiddleware. The issue did not identify this site as reachable; it surfaced during review.session.HasJSONRPCIDis the single definition of an absent id. Two sites receive the id as raw bytes threaded from the transparent proxy, where bothjson.RawMessage("null")andjson.RawMessage(nil)are non-nil interfaces that marshal tonull. A zero id still round-trips — the predicate tests nil-ness, not zero-ness.writeSSEErrorEventholds a typedjsonrpc2.IDand usesid.IsValid(): passing that struct through theany-typed predicate would report it present and emit"id":{}, which is neither absent nor a validRequestId.HasJSONRPCIDnow documents that.Nil, which holds whether the key is absent or null. One of them passed with the bug deliberately reintroduced. All now decode into a map and assert key absence. Where an id is present, assertions check verbatim echo against the wire bytes, since a map decode coerces a numeric id tofloat64and would drop the guarantee pinned in Transparent proxy: session-not-found 404 does not echo the request JSON-RPC id #5945.Fixes #6038
Type of change
Test plan
task test-e2e)task lint-fix)Every touched package is green, including under
-race:pkg/transport/session,pkg/mcp,pkg/ratelimit,pkg/vmcp/server,pkg/transport/middleware/origin,pkg/transport/proxy/streamable,pkg/transport/proxy/transparent.task testcould not be scoped — the Taskfile'stesttarget runs the whole suite overgo list ./...with no package filter — so the underlyinggo test -count=1invocation was used per package. CI runs the real thing. Linting was not run locally; leaving it to CI.Every new and flipped assertion was proven to be a real regression guard by mutation: the production change was reverted or inverted, the test confirmed to fail, then restored. This was not ceremony — it caught two tests that passed with the bug present (
TestParsingMiddlewareRejectsBatch, and the pre-existing origin test), and confirmed that swapping test fixtures fromfloat64toint64had not silently rendered the zero-id rows inert.Manual testing: the reachable rate-limit path was driven end-to-end through the real
ParsingMiddlewarewith atools/callnotification, confirming429 {"error":{...},"id":null,"jsonrpc":"2.0"}before the fix and the same body without theidkey after.E2E: compile-verified only (
go vet ./test/e2e/...). The batch-rejection assertion instdio_proxy_over_streamable_http_mcp_server_test.gowas updated but needs a built binary and live containers to run.API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Changes
pkg/transport/session/jsonrpc_errors.goHasJSONRPCID;NotFoundBodyomits the key; doc rewritten (#6031 had asserted null was required)pkg/mcp/classification_response.goclassificationErrorBody: conditional id; fallback literalpkg/mcp/tool_filter.gofilteredToolCallErrorBody: samepkg/mcp/batch.gopkg/ratelimit/middleware.gorateLimitedBody: conditional id;fmt.Sprintffallbackpkg/vmcp/server/modern_envelope.gowriteModernError/writeModernDenied: conditional id;writeModernResultdeliberately unchanged, with a comment saying whypkg/transport/middleware/origin/origin.gowriteForbidden:"id": nildeleted outright; fallback literal and docpkg/transport/proxy/streamable/streamable_proxy.gowriteSSEErrorEvent: gates onid.IsValid()pkg/transport/proxy/transparent/transparent_proxy.gorequestIDdoc still claimed JSON-RPC requires a null idDoes this introduce a user-facing change?
Yes. Error responses from session-not-found, classification failures, batch rejection, filtered tool calls, rate limiting, the vMCP Modern path, the origin 403, and SSE error events now omit the
idkey when there is no id to echo, instead of sending"id":null. Spec-strict MCP clients can parse these responses at all rather than throwing in their transport layer; in particular,-32001-driven automatic session recovery starts working for them. Clients that tolerated the non-conformant body are unaffected — a present id is still echoed verbatim, includingid: 0.Special notes for reviewers
On the two halves of the vMCP/rate-limit commit. These have different exposure and the commit message says so.
pkg/ratelimitfixes a path a real client can hit.pkg/vmcp/serveris defense-in-depth on a path that is currently unreachable: every call site ofwriteModernError/writeModernDeniedsits downstream ofdispatchModern'sparsed.ID == nilguard, which answers 202 before any error envelope is built, and vMCP ids arrive fromjsonrpc2.ID.Raw()asnil | int64 | string, never raw bytes. The writers takeid any, so guarding them is still right — but no client was receiving a null id from that path, and the PR should not be read as claiming otherwise.Scope is wider than the issue. #6038 named three sites; there are seven. The extra four are
pkg/mcp/tool_filter.go,pkg/transport/middleware/origin/origin.go,pkg/vmcp/server/modern_envelope.go, andwriteSSEErrorEvent. The origin one is arguably the most clear-cut of all of them: it hardcoded"id": nil, and its 403 is the case the transport spec names explicitly.Deliberately untouched:
pkg/vmcp/server/server.go:1372hardcodes"id":"server-error"— a fabricated id, arguably worse than an absent one since a client may correlate it against a real request. Separate issue.writeModernResultstill always emitsid. A result response requires one; omitting there would be as wrong as null. A test pins this, because the obvious future refactor is to make it match its siblings.pkg/authz/handleUnauthorizedemits{"Result":null,...,"ID":{}}— nojsonrpcfield, capitalized keys, id discarded. Not a Error responses emit "id":null, which the TS SDK client rejects #6038 violation (it never writes"id":null), and open PR Emit conformant JSON-RPC on denial and error paths #6055 already fixes exactly those lines.text/event-streamwith nodata:prefix) and Clean up JSON-RPC error codes and leaked error text on denial paths #6039 (HTTP statuses used as JSON-RPC error codes) remain open. The newwriteSSEErrorEventtest asserts thedata:framing specifically so this change cannot drift into the Filter-failure errors on the SSE path are silently discarded #6037 failure mode.No shared envelope builder, deliberately. Only the id predicate is shared. PR #6055 designed and rejected a full envelope helper; that reasoning still holds — the maps genuinely differ (rate-limit carries
data, vMCP setsCache-Control, statuses vary). Sharing just the predicate is what the two raw-bytes sites actually needed.Where I would want the most scrutiny: whether
sessionis the right home forHasJSONRPCID. It was chosen because that package already owns this envelope shape and is a near-leaf every affected package already imports, so it adds no new dependency edges. But a generic JSON-RPC predicate living in a package namedsessionis a naming wart, and a smalljsonrpcerrpackage would read better at the cost of one new import path.Generated with Claude Code