fix(model): reject malformed JSON-RPC messages with conflicting result and error fields (#1283)

[ump45nose]

Summary

fix(model): reject malformed JSON-RPC messages with conflicting result and error fields (#1283)

• Use one borrowing-compatible buffered deserializer for direct messages and single/batch outer untagged wrappers.
• Remove the private serde_json RawValue token, its custom visitors, and the redundant full-Value parsing path.
• Keep field-presence validation: conflicting result/error members are rejected even when one or both values are null. A lone null result remains supported for generic response payloads.
• Remove the direct RawValue response support introduced by an earlier revision of this PR. An identical external harness confirms that borrowed and boxed RawValue response payloads were unsupported on the base commit; this change does not claim to add that capability.

Verification

The following repository checks were rerun after the six-line redundant-error-arm cleanup in beee740 (2026-10-14). The cleanup preserves accepted/rejected shapes and the existing buffering, borrowing, and null behavior.

Using Rust/Cargo 1.96.0:

• cargo +1.96.0 test -p rmcp --lib: 226 passed
• cargo +1.96.0 test -p rmcp --lib --all-features: 613 passed
• cargo +1.96.0 test -p rmcp --test test_deserialization --all-features: 19 passed
• cargo +1.96.0 clippy -p rmcp --all-targets --all-features -- -D warnings: passed
• cargo +nightly-2026-09-30 fmt --all -- --check and git diff --check: passed
• Regression coverage includes conflicting fields, single/batch untagged wrappers, borrowed payloads, null results, and permissive generic payload dispatch.
• An identical external path-dependency harness (serde 1.0.229, serde_json 1.0.151) was run against base e02efbf, previous head 0a0cfa7, and the buffered-parser revision preceding this cleanup. This external harness was not rerun for beee740; the repository tests above were. It checks borrowed-string pointers, direct/single/batch decoding, strict conflicting-field rejection, null results, and six borrowed/boxed RawValue decoding routes.

These are local verification results. The full workspace integration and conformance suites were not rerun locally; remote checks must be read for the current commit rather than the previous head.

Related to #1283

AI assistance disclosure: AI was used to discover this opportunity and draft the change or text. An authorized AI assistant prepared this revision and executed the checks listed above. This does not claim unaided human implementation or testing.

[ump45nose]

Status re-check (Asia/Shanghai 2026-09-22)

Re-verified fork head 6b9f15ec against current upstream main:

• Branch divergence: ahead 2 / behind 0 (no rebase needed).
• Local: cargo test -p rmcp --lib → 205 passed, 0 failed (includes test_response_with_result_and_error_is_rejected, test_request_with_stray_result_or_error_is_rejected, and clean-shape / extension-field coverage).
• Upstream CI on this PR previously completed green (lint, clippy, tests, conformance, CodeQL).

No further code changes in this update — only confirming the PR is still merge-ready relative to main and the regression suite still holds.

Related: #1283

AI assistance disclosure: AI was used to discover this opportunity and draft the change or text. The submission was checked against the prepared artifact and recorded verification evidence.

[jarrettdustinqq]

I reproduced a borrowed-payload compatibility regression in 6b9f15ec301fdef27559e6b1d8c4f1aa443adfec, relative to base dbd238275534c3a8da4d91b7220655e878216988.

The comment above the new deserializer says the Deserialize<'de> bounds preserve the previous API. They preserve the type-level bound, but collecting the input into an owned serde_json::Value loses the input borrows before variant deserialization. Valid generic request and response types containing &str now fail at runtime.

I ran an identical external test harness against each unmodified SDK snapshot, using the SDK as a path dependency with default-features = false:

Case	Base	PR head
Borrowed request method parsed from &str	Pass	Fail: expected a borrowed string
Borrowed response field parsed from &str	Pass	Fail: expected a borrowed string
Owned request control	Pass	Pass
Owned response control	Pass	Pass

The passing borrowed cases also check that the returned string points inside the original input. Toolchain: rustc 1.96.0 (ac68faa20 2026-05-25), cargo 1.96.0 (30a34c682 2026-05-25); serde 1.0.229, serde_json 1.0.151. These are four focused external tests, not a rerun of the full SDK or conformance suite.

This request case, copied from the executed harness, reproduces the failure:

use rmcp::model::JsonRpcMessage;
use serde::Deserialize;
use serde_json::Value;

#[derive(Debug, Deserialize)]
struct BorrowedRequest<'a> {
    method: &'a str,
}

#[test]
fn generic_borrowed_request_deserializes_from_str() {
    let input = r#"{"jsonrpc":"2.0","id":1,"method":"ping"}"#;
    let parsed = serde_json::from_str::<JsonRpcMessage<BorrowedRequest<'_>, Value, Value>>(input)
        .expect("previous generic Deserialize API accepts borrowed request strings");
    match parsed {
        JsonRpcMessage::Request(request) => {
            assert_eq!(request.request.method, "ping");
            let start = input.as_ptr() as usize;
            let borrowed = request.request.method.as_ptr() as usize;
            assert!(borrowed >= start && borrowed < start + input.len());
        }
        other => panic!("expected Request, received {other:?}"),
    }
}

Could the parser preserve borrowed payload deserialization while rejecting conflicting fields, with this regression case retained? If borrowed generic payloads are intentionally unsupported, that would instead need an explicit compatibility decision and correction of the current preservation claim. I have not established a downstream application relying on this behavior.

AI disclosure: an authorized AI assistant performed the source review, prepared and executed the local regression harness, and drafted this comment. This does not claim the account owner's unaided Rust implementation or testing.

[ump45nose]

CI repair (Asia/Shanghai 2026-09-28)

Rebased onto current main and fixed the Clippy CI failure (clippy::type_complexity on JsonRpcMessageVisitor's PhantomData<fn() -> (Req, Resp, Noti)> marker).

• Tip: 29e685d (was e92906f)
• Divergence vs main: ahead 4 / behind 0
• Local: cargo clippy -p rmcp --lib --all-features -- -D warnings clean; cargo test -p rmcp --lib 221 passed

Change: introduce JsonRpcMessageVisitorMarker type alias so -D warnings no longer trips on the marker field.

---

AI assistance disclosure: This comment and the Clippy repair commit were prepared with AI assistance (Grok Bot); I reviewed the change and test results before pushing.

[DaleSeo]

This also now rejects "result": {...}, "error": null, which some lenient servers, especially those using JSON-RPC 1.0, return and which used to work. Should we treat a null result or error as absent so we only reject messages with a genuine conflict?

[DaleSeo]

A plain D matches the other implementations in this PR.

Suggested change

	fn deserialize<__D>(deserializer: __D) -> Result<Self, __D::Error>
	where
	__D: serde::Deserializer<'de>,
	fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
	where
	D: serde::Deserializer<'de>,

[DaleSeo]

Thanks for the quick turnaround, @ump45nose! This looks good now. I only have one extra comment.