[robfrank]

What does this PR do?

Makes every malformed JSON-RPC member on the MCP endpoint produce a JSON-RPC error envelope instead of failing the request.

The defaulting JSON accessors (getString(name, default), getJSONObject(name, default)) fall back only when the member is absent or null. A member that is present but of another JSON type raises out of the read instead. Four such reads sat above the try that was meant to answer for them:

Read	Was	Now
method in dispatch	HTTP 500, no body	-32600
params in dispatch	HTTP 500, no body	-32600
name / arguments in toolsCall	HTTP 500, no body	-32602
uri in resourcesRead	HTTP 500, no body	-32602
name / arguments in promptsGet	-32603	-32602

The dispatch pair is the worst of them: their enclosing try carries only a finally (the DatabaseContext cleanup) and no catch, so the raise left the dispatcher entirely and the transport turned it into a bodiless HTTP 500. That affects every method, ping included, not only the two that read an arguments member.

method and params are members of the request object, so a wrong shape there is -32600 Invalid Request. uri, name and arguments live inside params, so those are -32602 Invalid params.

Motivation

Closes #5585. Found while reviewing #5584, which fixed the same defect for prompts/get only; that fix was deliberately scoped to the surface that PR introduced, and this one covers the sites that predate it.

Additional Notes

Two exception types, not one. Gson raises IllegalStateException when getAsString() or getAsJsonObject() meets an array of the wrong arity, but UnsupportedOperationException when getAsString() meets an object. The prompts/get fix in #5584 caught only the first, which is why {"name": {}} there answered -32603. Every catch here covers both.

A Gson coercion worth knowing about. A single-element array coerces: {"method": ["ping"]} yields the string "ping" and dispatches normally. The tests therefore use {} rather than a one-element array, with a comment saying why - the obvious payload silently tests nothing.

Checklist

• I have run the build using mvn clean package command
• My unit tests cover both failure and success scenarios

Five tests added to MCPTransportConformanceTest, which already owns envelope-level conformance (invalid ids, notifications, batching). All five were confirmed to fail before the fix: four with expected: 200 but was: 500, and the prompts/get one with expected: -32602 but was: -32603 after temporarily reverting that catch.

MCP*Test is 265 tests, 0 failures, on a mvn clean build.

🤖 Generated with Claude Code

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

🟢 Coverage 100.00% diff coverage · -6.21% coverage variation

Metric	Results
Coverage variation	✅ -6.21% coverage variation
Diff coverage	✅ 100.00% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (bd0ba0d)	149149	111895	75.02%
Head commit (655b2a6)	174085 (+24936)	119791 (+7896)	68.81% (-6.21%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#5619)	16	16	100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Review

Solid, tightly-scoped fix with excellent commit prose and tests that were confirmed to fail beforehand. The -32600 vs -32602 split by member ownership (request object vs params) is correct per JSON-RPC 2.0, the dual IllegalStateException | UnsupportedOperationException catches match Gson's actual behavior (getAsJsonObject/multi-element-array -> IllegalStateException; getAsString on an object -> UnsupportedOperationException), and calling out the single-element-array coercion in the test comment is the kind of detail that keeps a "test" from silently testing nothing. Nice.

I verified all the params/request member reads (MCPDispatcher.java:213-214, 278, 313-314, 338-339) are now guarded. One gap remains, though.

One unguarded JSON-RPC member left, same defect class

isResponse runs as the first statement in dispatch (line 171), above every guard added here, and reads a member without protection:

```java
// line 554
return "2.0".equals(message.getString("jsonrpc", null))
&& message.has("result") != message.has("error");
```

getString(name, null) falls back only when the member is absent or null (JSONObject.java:236-240), so a jsonrpc member that is present but of another JSON type raises straight out of getAsString() - exactly the failure mode this PR set out to eliminate. It's reachable: isResponse only requires no method member plus a valid id before it reaches line 554, so a payload like

```json
{"jsonrpc": {}, "id": 1}
```

throws UnsupportedOperationException out of dispatch, above the guarded try, and the transport turns it into the same bodiless HTTP 500 the PR description calls "the worst of them." A multi-element array ("jsonrpc": ["2.0","x"]) does the same via IllegalStateException.

Given the PR's stated goal - "every malformed JSON-RPC member on the MCP endpoint produce a JSON-RPC error envelope instead of failing the request" - this is an incomplete-fix gap rather than a regression, but it's the identical bug on the jsonrpc member. Since isResponse runs before id is even extracted, the cleanest fix is probably to make it type-tolerant (e.g. message.opt("jsonrpc") and compare, so a non-string simply isn't a response) rather than wrapping it in another try. A one-line test alongside the five added here would lock it down.

Minor (non-blocking)

• In the combined guards (dispatch, toolsCall), when only one of the two members is malformed the message names both ("'name' must be a string and 'arguments' an object"). Harmless, just slightly less precise than the single-member messages.
• The single-element-array coercion ({"method":["ping"]} -> dispatches ping) is a real, if minor, JSON-RPC spec deviation. Correctly out of scope here and well-documented, but worth a follow-up issue so it isn't forgotten.

Overall: correct, well-tested, and a clear net improvement. I'd suggest folding the isResponse jsonrpc read into this PR since it's the same class and sits on the same hot path, but that's a judgment call.

[robfrank]

isResponse reading jsonrpc: confirmed and fixed

Reproduced before fixing: {"jsonrpc":{},"id":1} returned HTTP 500, exactly as described. The probe runs as the first statement in dispatch, so it sits above every guard the PR added, and it is genuinely the one read that cannot be placed under one - it decides whether a reply is owed at all, so there is no envelope to answer with yet.

Fixed in 655b2a676, using the opt approach suggested:

return "2.0".equals(message.opt("jsonrpc"))
    && message.has("result") != message.has("error");

opt maps any JSON shape to an object rather than demanding a string, so a jsonrpc that is not the string "2.0" simply means the payload is not a response, and it falls through to normal dispatch. Note this also tightens one case that previously passed: a numeric {"jsonrpc": 2.0, ...} used to be coerced to "2.0" and treated as a response; it no longer is, which matches JSON-RPC requiring the literal string.

A test covers both raising shapes, the object and the multi-element array, since those go through different Gson exceptions:

for (final String jsonrpc : new String[] { "{}", "[\"2.0\",\"x\"]" }) { ... }

Confirmed failing before the fix with [payload with jsonrpc={}] but was: 500. MCP*Test is now 266 tests, 0 failures.

Single-element array coercion: filed as #5620

Filed rather than folded in, agreeing with your read that it is a different defect. Worth recording there: the arity of the array decides the outcome, which is not a distinction JSON-RPC makes. ["ping"] executes, ["ping","x"] is rejected.

The issue also notes that fixing it properly means type-checking the member (opt(name) instanceof String) rather than catching what the accessor raises - which would make the -32600/-32602 guards in this PR redundant on the string-typed members. So the two are better done together than layered, and this PR is the wrong place for it.

Combined guard messages: leaving as-is

Declining this one. Splitting "'name' must be a string and 'arguments' an object" into per-member messages means a separate try block per member at three sites, roughly doubling the guard code. The payload that triggers it is a client bug, both candidate members are named, and the JSON-RPC code already says which half of the request is at fault (-32600 for a request member, -32602 for a params member). The precision gained does not carry that much weight.

Happy to reverse this if you would rather have exact messages.

[claude]

Review

Nicely scoped fix with a clear write-up, and the tests are thoughtful - especially calling out the single-element-array coercion so the malformed path is actually exercised rather than silently coerced into a valid call. I verified the exception-type reasoning against JSONObject/Gson: getAsString() on an object throws UnsupportedOperationException, on a wrong-arity array throws IllegalStateException, so each catch (IllegalStateException | UnsupportedOperationException) is the right pair. The isResponse switch to opt(...) is also a genuine improvement (and cleaner).

One gap: set_server_setting with a non-string key still reaches HTTP 500

The PR aims for every malformed JSON-RPC member to produce an envelope, but a surviving path remains in toolsCall. After the new guard reads arguments as a JSONObject, the log line calls formatArgs(toolName, args) at MCPDispatcher.java:344-345, OUTSIDE the try that starts at line 347. formatArgs does:

final boolean maskValue = "set_server_setting".equals(toolName) && isHiddenSetting(args.getString("key", null));

getString("key", null) falls back only when key is absent/null. If key is present but not a string, getAsString() raises UnsupportedOperationException (object) or IllegalStateException (array), which escapes formatArgs -> escapes toolsCall -> passes through dispatch finally-only try -> bodiless HTTP 500. Exactly the defect this PR removes elsewhere.

Repro (tool name must be exactly set_server_setting, since that gates the getString):

{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"set_server_setting","arguments":{"key":{}}}}

Suggested fix - read key type-tolerantly so the log helper can never raise:

final Object rawKey = args.opt("key");
final boolean maskValue = "set_server_setting".equals(toolName) && rawKey instanceof String s && isHiddenSetting(s);

Worth a sixth test alongside the others (tools/call set_server_setting with key={} -> -32602, not 500).

Minor

The two-member guards report a single combined message ("method must be a string and params an object") even when only one member is malformed. Fine for a JSON-RPC error string, just slightly less precise than the per-member messages in resourcesRead. Not worth changing.

Otherwise looks good - correct, well-tested, no performance or security concerns introduced.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.00%. Comparing base (bd0ba0d) to head (655b2a6).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5619      +/-   ##
============================================
+ Coverage     66.24%   67.00%   +0.75%     
============================================
  Files          1764     1718      -46     
  Lines        149149   142093    -7056     
  Branches      31683    30521    -1162     
============================================
- Hits          98807    95208    -3599     
+ Misses        37298    34211    -3087     
+ Partials      13044    12674     -370     

☔ 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.