fix(jsonrpc): json-rpc spec mismatches

[brbrr]

User description

• "id": null is a request, not a notification. Request.ID and response.ID are now json.RawMessage; nil means the key was absent. A request with a null id gets a reply with "id": null. validID reads the raw bytes instead of going through reflection and json.Number.
• Trailing data after the request body is rejected. decodeOne reads past the single JSON value; whitespace is allowed, anything else is -32700 Parse error with a caret at the offending byte. Applies to single requests and batches.
• A handler returning a nil interface keeps the result key. The response stores json.RawMessage("null") so omitempty does not drop it.

---

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.38%. Comparing base (6388ef7) to head (3d30ed4).
⚠️ Report is 31 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4166      +/-   ##
==========================================
+ Coverage   80.25%   80.38%   +0.13%     
==========================================
  Files         502      504       +2     
  Lines       38318    38437     +119     
==========================================
+ Hits        30752    30899     +147     
+ Misses       7557     7529      -28     
  Partials        9        9              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Claude finished @brbrr's task in 37m 55s —— View job

---

Reviewing PR #4166

• Gather context (read changed files, diff)
• Review jsonrpc/server.go changes
• Review jsonrpc/pretty_error.go changes
• Review jsonrpc/server_test.go changes
• Post findings

Summary

The three fixes (null id vs. notification, trailing-data rejection, result: null preservation) are each correctly targeted and well covered by table-driven tests. validID reading raw bytes instead of going through reflect/json.Number is a nice simplification, and all callers already guard id != nil before calling it, so the id[0] indexing is safe. I confirmed json.RawMessage's UnmarshalJSON is invoked even for literal null, which is what makes the "absent vs. null" distinction actually work.

Important

• jsonrpc/server.go:557-581 (inline comment posted) — decodeOne's trailing-data check now fully drains the request body (one byte per Read call) on every request, even the common case with no trailing garbage. Previously json.Decoder stopped right after the parsed value. Combined with the HTTP transport's 10MB MaxBytesReader limit, a client can send a tiny valid request followed by megabytes of trailing whitespace and force a full-body drain that didn't exist before — a cheap amplification vector on an RPC endpoint that accepts untrusted input. Suggest bounding how much trailing whitespace is tolerated, or using Peek/Discard to skip runs of whitespace in bulk rather than one byte at a time.

No other correctness, concurrency, or resource-leak issues found; the trailingDataError offset math and the nilResult/null id handling check out against the test expectations.
(branch)

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 3d30ed4)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

⚠️ Risk level: Medium

📂 Priority files jsonrpc/server.go

🏅 Score: 75

🧪 PR contains tests

🔒 No security concerns identified

[github-actions]

Claude finished @brbrr's task in 2m 53s —— View job

---

Reviewing PR #4166 (new commits pushed)

• Gather context (diff since last review, read changed files)
• Check if prior feedback (chunked trailing-data scan) was addressed
• Review jsonrpc/server.go changes
• Review jsonrpc/pretty_error.go changes
• Review jsonrpc/server_test.go changes
• Post findings

Summary

The latest commit (3d30ed479, "scan trailing request data in chunks") addresses both points raised in the earlier automated reviews on decodeOne (jsonrpc/server.go:558-585):

• Refill overhead / amplification: decodeOne now reads into a [bufferSize]byte (128-byte) buffer instead of one byte at a time. For the HTTP transport's worst case (tiny valid request + ~10MB of trailing whitespace padding under MaxBytesReader), this cuts the number of Read syscalls by ~128x. The reviewer's own suggestion explicitly offered "bound the whitespace" or "scan in bulk instead of one byte per Read" as alternatives — this takes the latter, which is a reasonable, low-risk fix given scanning whitespace in memory is cheap once syscall overhead is removed.
• Spurious trailing-data error on (0, nil) reads: previously a conforming-but-unusual reader returning (0, nil) would leave a zero-valued b[0] that fell into the default case and raised a false parse error. With the chunked version, n is correctly 0 in that case and buf[:n] is empty, so the loop just reads again. This is now covered by TestHandleReaderTrailingData/empty_reads_are_not_trailing_data using the new stallReader + iotest.OneByteReader combo (jsonrpc/server_test.go:773-800), and the offset-across-chunk-boundaries case is covered by the second subtest.

Checked the offset bookkeeping (offset++ per whitespace byte, buf[:n] processed before checking err to correctly handle a Read that returns n > 0 together with io.EOF) — logic is correct and matches io.Reader contract guidance.

No remaining correctness or concurrency issues found. The only residual point (not a blocker, both earlier reviewers explicitly offered it as an acceptable trade-off): the handler still scans to EOF rather than capping total trailing bytes tolerated, so a malicious client can still force an up-to-~10MB in-memory scan per request — just a cheap one now (no syscall-per-byte cost, no amplification of socket round-trips). If this needs to be hardened further (e.g., a small fixed cap on trailing bytes before erroring), that would be a follow-up, not something blocking this PR.
(branch)

[github-actions]

Persistent review updated to latest commit 3d30ed4