From 57dd73f3c4d5f04290b2d227c0ecd7d3f2bf12ef Mon Sep 17 00:00:00 2001
From: cyberspace-cs <cyberspace-cs@users.noreply.github.com>
Date: Sat, 19 Sep 2026 19:06:09 +0800
Subject: [PATCH 1/3] fix(ipc): decode explicit null results and stop silent
 null degradation

The IPC protocol is JSON-RPC-style: a successful reply carries
"result" and a failed reply carries "error". Two defects made JSON
null a silent failure channel instead of a legal value.

1. Decoder (bsk-protocol): ResponseFrame::deserialize and the Frame
   visitor modelled the result field as Option<Value>, which serde
   collapses "result": null into the same None as a missing field.
   The daemon emits exactly that shape through its
   serde_json::to_value(..).unwrap_or(Value::Null) fallback, so a
   result serialisation failure surfaced on the CLI as a misleading
   "ambiguous response" / "expected result or error" decode error
   instead of a null result. Both deserializers now parse the raw
   value with #[serde(default, deserialize_with)] so an explicit
   null result decodes to Ok(Value::Null) while a missing field
   stays an error.

2. Daemon (bsk-cli): every serde_json::to_value(..).unwrap_or(Value::Null)
   site silently degraded a result serialisation failure (e.g. a
   payload nesting deeper than serde_json's recursion limit) to JSON
   null, hiding the real failure and colliding with legitimate null
   results. These now route through ok_value()/serialise_err()
   helpers that return a structured protocol_error instead, and the
   upload/download param staging paths return the same error rather
   than forwarding null params to the extension.

Adds unit tests on both sides: explicit-null decode for Frame and
ResponseFrame, serialise round-trip symmetry, missing-field
regression, and the daemon-side serialisation-failure path.
---
 CHANGELOG.md                     |  10 ++
 crates/bsk-cli/src/daemon/ipc.rs | 166 +++++++++++++++++++++++++------
 crates/bsk-protocol/src/frame.rs |  86 +++++++++++++++-
 3 files changed, 232 insertions(+), 30 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3d928347..b9c13516 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,6 +5,16 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 
 Starting from 0.2.0, CLI / Extension / DSH Plugin share the same version number.
 
+## [Unreleased]
+
+### Fixed
+
+- IPC protocol: an explicit `null` result (for example the daemon's
+  `serde_json::to_value(..).unwrap_or(Value::Null)` fallback) is now decoded as a
+  success body instead of being rejected as an ambiguous frame; the daemon no
+  longer silently degrades result serialisation failures to `null` and returns a
+  structured `protocol_error` instead
+
 ## [0.3.0] - 2026-09-16
 
 ### Added
diff --git a/crates/bsk-cli/src/daemon/ipc.rs b/crates/bsk-cli/src/daemon/ipc.rs
index 69daf58c..7847f3c0 100644
--- a/crates/bsk-cli/src/daemon/ipc.rs
+++ b/crates/bsk-cli/src/daemon/ipc.rs
@@ -156,7 +156,7 @@ pub fn default_ping_handler() -> RpcHandler {
             match method {
                 Method::SystemPing => {
                     let result = PingResult { pong: true };
-                    ResponseBody::Ok(serde_json::to_value(result).unwrap_or(Value::Null))
+                    ok_value(result)
                 }
                 other => ResponseBody::Err(RpcError {
                     code: ErrorCode::UnknownMethod,
@@ -178,11 +178,11 @@ pub fn system_handler(status: DaemonStatus) -> RpcHandler {
             match method {
                 Method::SystemPing => {
                     let result = PingResult { pong: true };
-                    ResponseBody::Ok(serde_json::to_value(result).unwrap_or(Value::Null))
+                    ok_value(result)
                 }
                 Method::SystemStatus => {
                     let result = status.snapshot();
-                    ResponseBody::Ok(serde_json::to_value(result).unwrap_or(Value::Null))
+                    ok_value(result)
                 }
                 other => ResponseBody::Err(RpcError {
                     code: ErrorCode::UnknownMethod,
@@ -230,7 +230,7 @@ pub fn full_handler(status: DaemonStatus, state: Arc<DaemonState>) -> RpcHandler
             let body = match method {
                 Method::SystemPing => {
                     let result = PingResult { pong: true };
-                    ResponseBody::Ok(serde_json::to_value(result).unwrap_or(Value::Null))
+                    ok_value(result)
                 }
                 Method::SystemStatus => match handle_status(&status, &state, params).await {
                     Ok(v) => ResponseBody::Ok(v),
@@ -437,7 +437,16 @@ async fn handle_tool_dispatch(
         for (file, path) in upload.files.iter_mut().zip(paths) {
             file.staged_path = Some(path.to_string_lossy().into_owned());
         }
-        params = serde_json::to_value(upload).unwrap_or(Value::Null);
+        params = match serde_json::to_value(&upload) {
+            Ok(v) => v,
+            Err(err) => {
+                return ResponseBody::Err(RpcError {
+                    code: ErrorCode::ProtocolError,
+                    message: format!("failed to serialise upload params: {err}"),
+                    data: None,
+                });
+            }
+        };
     } else if method == Method::ToolDownload {
         let mut download: DownloadParams = match serde_json::from_value(params) {
             Ok(v) => v,
@@ -450,7 +459,16 @@ async fn handle_tool_dispatch(
         download.browser_relative_dir = Some(staging.browser_relative_dir);
         download.max_byte_size = Some(super::file_transfer::MAX_TRANSFER_BYTES);
         download_transfer_id = Some(staging.transfer_id);
-        params = serde_json::to_value(download).unwrap_or(Value::Null);
+        params = match serde_json::to_value(&download) {
+            Ok(v) => v,
+            Err(err) => {
+                return ResponseBody::Err(RpcError {
+                    code: ErrorCode::ProtocolError,
+                    message: format!("failed to serialise download params: {err}"),
+                    data: None,
+                });
+            }
+        };
     }
     if let Some(audit_id) = audit_id
         && let Some(object) = params.as_object_mut()
@@ -505,7 +523,7 @@ async fn handle_tool_dispatch(
                 Ok(size) => {
                     result.byte_size = size;
                     result.transfer_id = Some(id);
-                    ResponseBody::Ok(serde_json::to_value(result).unwrap_or(Value::Null))
+                    ok_value(result)
                 }
                 Err(err) => {
                     state
@@ -544,7 +562,7 @@ fn handle_transfer_begin(state: &Arc<DaemonState>, params: Value) -> ResponseBod
         });
     }
     match state.transfers.begin_upload(p) {
-        Ok(v) => ResponseBody::Ok(serde_json::to_value(v).unwrap_or(Value::Null)),
+        Ok(v) => ok_value(v),
         Err(err) => ResponseBody::Err(err),
     }
 }
@@ -555,7 +573,7 @@ fn handle_transfer_chunk(state: &Arc<DaemonState>, params: Value) -> ResponseBod
         Err(err) => return ResponseBody::Err(invalid_params(err.to_string())),
     };
     match state.transfers.write_chunk(p) {
-        Ok(v) => ResponseBody::Ok(serde_json::to_value(v).unwrap_or(Value::Null)),
+        Ok(v) => ok_value(v),
         Err(err) => ResponseBody::Err(err),
     }
 }
@@ -566,7 +584,7 @@ fn handle_transfer_finish(state: &Arc<DaemonState>, params: Value) -> ResponseBo
         Err(err) => return ResponseBody::Err(invalid_params(err.to_string())),
     };
     match state.transfers.finish_upload(p) {
-        Ok(v) => ResponseBody::Ok(serde_json::to_value(v).unwrap_or(Value::Null)),
+        Ok(v) => ok_value(v),
         Err(err) => ResponseBody::Err(err),
     }
 }
@@ -577,7 +595,7 @@ fn handle_transfer_read(state: &Arc<DaemonState>, params: Value) -> ResponseBody
         Err(err) => return ResponseBody::Err(invalid_params(err.to_string())),
     };
     match state.transfers.read_chunk(p) {
-        Ok(v) => ResponseBody::Ok(serde_json::to_value(v).unwrap_or(Value::Null)),
+        Ok(v) => ok_value(v),
         Err(err) => ResponseBody::Err(err),
     }
 }
@@ -587,7 +605,7 @@ fn handle_transfer_release(state: &Arc<DaemonState>, params: Value) -> ResponseB
         Ok(v) => v,
         Err(err) => return ResponseBody::Err(invalid_params(err.to_string())),
     };
-    ResponseBody::Ok(serde_json::to_value(state.transfers.release(p)).unwrap_or(Value::Null))
+    ok_value(state.transfers.release(p))
 }
 
 fn invalid_params(message: impl Into<String>) -> RpcError {
@@ -598,6 +616,36 @@ fn invalid_params(message: impl Into<String>) -> RpcError {
     }
 }
 
+/// Serialise a handler result into a success body.
+///
+/// A serialisation failure (e.g. exceeding serde_json's recursion limit
+/// on a deeply nested payload) is surfaced as a `protocol_error` instead
+/// of silently degrading to JSON `null`. A `null` result would otherwise
+/// be indistinguishable from a method legitimately returning null and
+/// would hide the real failure from the CLI client; it also trips the
+/// wire decoder's "expected result or error" guard on peers that predate
+/// the explicit-null fix in `bsk-protocol/src/frame.rs`.
+fn ok_value(value: impl Serialize) -> ResponseBody {
+    match serde_json::to_value(&value) {
+        Ok(v) => ResponseBody::Ok(v),
+        Err(err) => ResponseBody::Err(RpcError {
+            code: ErrorCode::ProtocolError,
+            message: format!("failed to serialise handler result: {err}"),
+            data: None,
+        }),
+    }
+}
+
+/// Map a result-serialisation failure onto a structured `protocol_error`
+/// for handlers that return `Result<Value, RpcError>`.
+fn serialise_err(context: &str, err: serde_json::Error) -> RpcError {
+    RpcError {
+        code: ErrorCode::ProtocolError,
+        message: format!("failed to serialise {context}: {err}"),
+        data: None,
+    }
+}
+
 /// Upper bound on `tool.wait_ms` (5 minutes). Larger values are
 /// rejected as `invalid_params` so a buggy agent cannot wedge a
 /// daemon-side sleep beyond a reasonable window. The cap matches the
@@ -635,9 +683,7 @@ async fn handle_wait_ms(
         });
     }
     if params.duration_ms == 0 {
-        return ResponseBody::Ok(
-            serde_json::to_value(WaitMsResult { waited_ms: 0 }).unwrap_or(Value::Null),
-        );
+        return ok_value(WaitMsResult { waited_ms: 0 });
     }
     let guard = match registry.register(rpc_id) {
         Ok(guard) => guard,
@@ -652,10 +698,7 @@ async fn handle_wait_ms(
     let token = guard.token().clone();
     let result = tokio::select! {
         _ = tokio::time::sleep(Duration::from_millis(params.duration_ms)) => {
-            ResponseBody::Ok(
-                serde_json::to_value(WaitMsResult { waited_ms: params.duration_ms })
-                    .unwrap_or(Value::Null),
-            )
+            ok_value(WaitMsResult { waited_ms: params.duration_ms })
         }
         _ = token.cancelled() => {
             ResponseBody::Err(RpcError {
@@ -723,7 +766,7 @@ fn handle_cancel(state: &Arc<DaemonState>, params: Value) -> ResponseBody {
             );
         }
     }
-    ResponseBody::Ok(serde_json::to_value(CancelResult { cancelled }).unwrap_or(Value::Null))
+    ok_value(CancelResult { cancelled })
 }
 
 /// Test-only re-export of the cancel handler: the system-only handler
@@ -743,7 +786,7 @@ fn handle_cancel_with_registry_only(registry: &Arc<AbortRegistry>, params: Value
         }
     };
     let cancelled = registry.cancel(&params.rpc_id);
-    ResponseBody::Ok(serde_json::to_value(CancelResult { cancelled }).unwrap_or(Value::Null))
+    ok_value(CancelResult { cancelled })
 }
 
 fn tool_dispatch_timeout(params: &Value) -> Result<Duration, RpcError> {
@@ -872,7 +915,8 @@ async fn handle_status(
 ) -> Result<Value, RpcError> {
     let params: StatusParams = parse_params_or_default(params)?;
     maybe_wait_for_browser(state, params.wait_for_browser_ms).await;
-    Ok(serde_json::to_value(status.snapshot_with(state)).unwrap_or(Value::Null))
+    serde_json::to_value(status.snapshot_with(state))
+        .map_err(|err| serialise_err("status snapshot", err))
 }
 
 fn parse_params_or_default<T>(params: Value) -> Result<T, RpcError>
@@ -976,7 +1020,7 @@ async fn handle_session_start(
                 browser_instance_id: session.browser_id.0.clone(),
                 agent_window_id: session.agent_window_id,
             };
-            Ok(serde_json::to_value(result).unwrap_or(Value::Null))
+            serde_json::to_value(result).map_err(|err| serialise_err("session start result", err))
         }
         Err(err) => Err(map_start_error(err)),
     }
@@ -1084,7 +1128,7 @@ async fn handle_session_stop(
                 returned_tab_ids: stop.returned_tab_ids,
                 return_failures: stop.return_failures,
             };
-            Ok(serde_json::to_value(result).unwrap_or(Value::Null))
+            serde_json::to_value(result).map_err(|err| serialise_err("session stop result", err))
         }
         Err(StopSessionError::ReturnFailures(stop)) => {
             let message = format!(
@@ -1101,7 +1145,7 @@ async fn handle_session_stop(
                 returned_tab_ids: stop.returned_tab_ids,
                 return_failures: stop.return_failures,
             };
-            Ok(serde_json::to_value(result).unwrap_or(Value::Null))
+            serde_json::to_value(result).map_err(|err| serialise_err("session stop result", err))
         }
         Err(err) => Err(map_stop_error(err)),
     }
@@ -1225,7 +1269,7 @@ async fn handle_session_stop_all(
         returned_tab_ids,
         return_failures,
     };
-    Ok(serde_json::to_value(result).unwrap_or(Value::Null))
+    serde_json::to_value(result).map_err(|err| serialise_err("session stop_all result", err))
 }
 
 fn handle_session_list(state: &Arc<DaemonState>) -> ResponseBody {
@@ -1235,7 +1279,7 @@ fn handle_session_list(state: &Arc<DaemonState>) -> ResponseBody {
         .into_iter()
         .map(|s| s.status_entry())
         .collect();
-    ResponseBody::Ok(serde_json::to_value(SessionListResult { sessions }).unwrap_or(Value::Null))
+    ok_value(SessionListResult { sessions })
 }
 
 async fn handle_browser_list(state: &Arc<DaemonState>, params: Value) -> Result<Value, RpcError> {
@@ -1247,7 +1291,8 @@ async fn handle_browser_list(state: &Arc<DaemonState>, params: Value) -> Result<
     // ascending, with `instance_id` as a deterministic tiebreaker
     // (review I1).
     let browsers = snapshot_status_entries(&state.browsers, &state.sessions);
-    Ok(serde_json::to_value(BrowserListResult { browsers }).unwrap_or(Value::Null))
+    serde_json::to_value(BrowserListResult { browsers })
+        .map_err(|err| serialise_err("browser list result", err))
 }
 
 // ----- Test-helper IpcServer wrapper around the transport layer -----
@@ -2031,4 +2076,69 @@ mod tests {
         let _ = tx.send(());
         let _ = server.await;
     }
+
+    // A Serialize impl that always fails, standing in for a deeply
+    // nested payload that trips serde_json's recursion limit.
+    struct FailingSerialize;
+    impl serde::Serialize for FailingSerialize {
+        fn serialize<S>(&self, _serializer: S) -> Result<S::Ok, S::Error>
+        where
+            S: serde::Serializer,
+        {
+            Err(serde::ser::Error::custom("recursion limit exceeded"))
+        }
+    }
+
+    #[test]
+    fn ok_value_serialises_regular_result() {
+        match ok_value(PingResult { pong: true }) {
+            ResponseBody::Ok(v) => assert_eq!(v, serde_json::json!({ "pong": true })),
+            other => panic!("expected ok, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn ok_value_reports_protocol_error_on_failed_serialise() {
+        // A serialisation failure must surface as a structured
+        // protocol_error instead of silently degrading to JSON null.
+        match ok_value(FailingSerialize) {
+            ResponseBody::Err(err) => {
+                assert_eq!(err.code, ErrorCode::ProtocolError);
+                assert!(
+                    err.message.contains("failed to serialise"),
+                    "message: {}",
+                    err.message
+                );
+            }
+            other => panic!("expected protocol_error, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn serialise_err_builds_protocol_error_with_context() {
+        let err = serde_json::from_str::<serde_json::Value>("[").unwrap_err();
+        let rpc = serialise_err("status snapshot", err);
+        assert_eq!(rpc.code, ErrorCode::ProtocolError);
+        assert!(
+            rpc.message.contains("status snapshot"),
+            "message: {}",
+            rpc.message
+        );
+    }
+
+    #[test]
+    fn null_result_response_round_trips_over_frame_decode() {
+        // The daemon can legitimately emit an explicit null result;
+        // the CLI-side Frame decoder must treat it as a success body
+        // rather than an ambiguous frame (frame.rs regression guard).
+        let wire = r#"{"id":"rpc-null","result":null}"#;
+        let frame: Frame = serde_json::from_str(wire).expect("explicit null result decodes");
+        match frame {
+            Frame::Response(resp) => {
+                assert_eq!(resp.id, "rpc-null");
+                assert_eq!(resp.body, ResponseBody::Ok(serde_json::Value::Null));
+            }
+            other => panic!("expected response frame, got {other:?}"),
+        }
+    }
 }
diff --git a/crates/bsk-protocol/src/frame.rs b/crates/bsk-protocol/src/frame.rs
index b80eec9b..abb0de8f 100644
--- a/crates/bsk-protocol/src/frame.rs
+++ b/crates/bsk-protocol/src/frame.rs
@@ -94,6 +94,14 @@ impl<'de> Deserialize<'de> for ResponseFrame {
         #[derive(Deserialize)]
         struct Flat {
             id: RpcId,
+            // `#[serde(default, deserialize_with)]` keeps a present-but-null
+            // `result` distinct from an absent field: serde's plain
+            // `Option<Value>` collapses JSON `"result": null` (a legal
+            // JSON-RPC null result — see the daemon's
+            // `serde_json::to_value(..).unwrap_or(Value::Null)` fallback in
+            // `bsk-cli/src/daemon/ipc.rs`) into `None`, which would then
+            // misclassify an explicit null result as an ambiguous frame.
+            #[serde(default, deserialize_with = "de_result_field")]
             result: Option<serde_json::Value>,
             error: Option<RpcError>,
         }
@@ -114,6 +122,22 @@ impl<'de> Deserialize<'de> for ResponseFrame {
     }
 }
 
+/// Deserialise the `result` field of a response frame.
+///
+/// A plain `Option<serde_json::Value>` collapses JSON `null` into `None`
+/// (serde's `Option` treats `null` as "none"), which would make an
+/// explicit `{"result": null}` reply indistinguishable from a missing
+/// field and reject it as ambiguous. The daemon can legitimately emit an
+/// explicit null result (its `serde_json::to_value(..).unwrap_or(Value::Null)`
+/// fallback), so we parse the raw `Value` here and let the caller's match
+/// decide — a present-but-null result becomes `Some(Value::Null)`.
+fn de_result_field<'de, D>(deserializer: D) -> Result<Option<serde_json::Value>, D::Error>
+where
+    D: Deserializer<'de>,
+{
+    serde_json::Value::deserialize(deserializer).map(Some)
+}
+
 impl Serialize for Frame {
     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
     where
@@ -152,7 +176,11 @@ impl<'de> Visitor<'de> for FrameVisitor {
         let mut id = None::<RpcId>;
         let mut method = None::<Method>;
         let mut params = None::<serde_json::Value>;
-        let mut result = None::<serde_json::Value>;
+        // Outer `Option` = field presence, inner = the value. `"result":
+        // null` parses to `Some(None)` so an explicit null result is
+        // distinguishable from a missing field (see `ResponseFrame`'s
+        // `Flat` helper for the rationale).
+        let mut result = None::<Option<serde_json::Value>>;
         let mut error = None::<RpcError>;
         let mut event = None::<EventKind>;
         let mut payload = None::<serde_json::Value>;
@@ -222,10 +250,14 @@ impl<'de> Visitor<'de> for FrameVisitor {
 
         let id = id.ok_or_else(|| de::Error::missing_field("id"))?;
         match (result, error) {
-            (Some(v), None) => Ok(Frame::Response(ResponseFrame {
+            (Some(Some(v)), None) => Ok(Frame::Response(ResponseFrame {
                 id,
                 body: ResponseBody::Ok(v),
             })),
+            (Some(None), None) => Ok(Frame::Response(ResponseFrame {
+                id,
+                body: ResponseBody::Ok(serde_json::Value::Null),
+            })),
             (None, Some(e)) => Ok(Frame::Response(ResponseFrame {
                 id,
                 body: ResponseBody::Err(e),
@@ -280,4 +312,54 @@ mod tests {
             Some("sess-1"),
         );
     }
+
+    #[test]
+    fn explicit_null_result_decodes_as_ok_null() {
+        // The daemon may legitimately emit `"result": null` (its
+        // `serde_json::to_value(..).unwrap_or(Value::Null)` fallback in
+        // `bsk-cli/src/daemon/ipc.rs`). A plain `Option<Value>` would
+        // collapse the explicit null into "field absent" and reject the
+        // frame as ambiguous; the inner `Option` keeps the distinction.
+        let wire = serde_json::json!({ "id": "rpc-1", "result": null });
+        let frame: Frame = serde_json::from_value(wire).unwrap();
+        match frame {
+            Frame::Response(resp) => {
+                assert_eq!(resp.id, "rpc-1");
+                assert_eq!(resp.body, ResponseBody::Ok(serde_json::Value::Null));
+            }
+            other => panic!("expected response frame, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn explicit_null_result_response_frame_decodes_as_ok_null() {
+        // The dedicated `ResponseFrame` deserializer must agree with the
+        // generic `Frame` visitor on the same wire shape.
+        let wire = serde_json::json!({ "id": "rpc-2", "result": null });
+        let resp: ResponseFrame = serde_json::from_value(wire).unwrap();
+        assert_eq!(resp.id, "rpc-2");
+        assert_eq!(resp.body, ResponseBody::Ok(serde_json::Value::Null));
+    }
+
+    #[test]
+    fn null_result_round_trips_through_serialise() {
+        // Serialising `Ok(Value::Null)` emits `"result": null`; decoding
+        // that wire shape must produce the same body (symmetric protocol).
+        let frame = Frame::Response(ResponseFrame {
+            id: "rpc-3".into(),
+            body: ResponseBody::Ok(serde_json::Value::Null),
+        });
+        let v = serde_json::to_value(&frame).unwrap();
+        let back: Frame = serde_json::from_value(v).unwrap();
+        assert_eq!(back, frame);
+    }
+
+    #[test]
+    fn missing_result_and_error_still_rejected() {
+        // A response with neither `result` nor `error` remains ambiguous —
+        // only an *explicit* null result is a legal success payload.
+        let wire = serde_json::json!({ "id": "rpc-4" });
+        assert!(serde_json::from_value::<Frame>(wire.clone()).is_err());
+        assert!(serde_json::from_value::<ResponseFrame>(wire).is_err());
+    }
 }

From d85029294d88ea3dabf41ac61608b5f547294b00 Mon Sep 17 00:00:00 2001
From: Zhang GH <5351707+iuyo5678@users.noreply.github.com>
Date: Sun, 27 Sep 2026 13:37:41 +0800
Subject: [PATCH 2/3] fix(protocol): scope null result decoding to
 ResponseFrame

---
 CHANGELOG.md                     |   7 +-
 crates/bsk-cli/src/daemon/ipc.rs | 166 ++++++-------------------------
 crates/bsk-protocol/src/frame.rs | 116 +++++++++++----------
 3 files changed, 95 insertions(+), 194 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index b9c13516..62f49a1a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,11 +9,8 @@ Starting from 0.2.0, CLI / Extension / DSH Plugin share the same version number.
 
 ### Fixed
 
-- IPC protocol: an explicit `null` result (for example the daemon's
-  `serde_json::to_value(..).unwrap_or(Value::Null)` fallback) is now decoded as a
-  success body instead of being rejected as an ambiguous frame; the daemon no
-  longer silently degrades result serialisation failures to `null` and returns a
-  structured `protocol_error` instead
+- Protocol: preserve explicit `null` results when deserializing `ResponseFrame`,
+  restoring round-trip consistency while rejecting responses with both a result and an error.
 
 ## [0.3.0] - 2026-09-16
 
diff --git a/crates/bsk-cli/src/daemon/ipc.rs b/crates/bsk-cli/src/daemon/ipc.rs
index 7847f3c0..69daf58c 100644
--- a/crates/bsk-cli/src/daemon/ipc.rs
+++ b/crates/bsk-cli/src/daemon/ipc.rs
@@ -156,7 +156,7 @@ pub fn default_ping_handler() -> RpcHandler {
             match method {
                 Method::SystemPing => {
                     let result = PingResult { pong: true };
-                    ok_value(result)
+                    ResponseBody::Ok(serde_json::to_value(result).unwrap_or(Value::Null))
                 }
                 other => ResponseBody::Err(RpcError {
                     code: ErrorCode::UnknownMethod,
@@ -178,11 +178,11 @@ pub fn system_handler(status: DaemonStatus) -> RpcHandler {
             match method {
                 Method::SystemPing => {
                     let result = PingResult { pong: true };
-                    ok_value(result)
+                    ResponseBody::Ok(serde_json::to_value(result).unwrap_or(Value::Null))
                 }
                 Method::SystemStatus => {
                     let result = status.snapshot();
-                    ok_value(result)
+                    ResponseBody::Ok(serde_json::to_value(result).unwrap_or(Value::Null))
                 }
                 other => ResponseBody::Err(RpcError {
                     code: ErrorCode::UnknownMethod,
@@ -230,7 +230,7 @@ pub fn full_handler(status: DaemonStatus, state: Arc<DaemonState>) -> RpcHandler
             let body = match method {
                 Method::SystemPing => {
                     let result = PingResult { pong: true };
-                    ok_value(result)
+                    ResponseBody::Ok(serde_json::to_value(result).unwrap_or(Value::Null))
                 }
                 Method::SystemStatus => match handle_status(&status, &state, params).await {
                     Ok(v) => ResponseBody::Ok(v),
@@ -437,16 +437,7 @@ async fn handle_tool_dispatch(
         for (file, path) in upload.files.iter_mut().zip(paths) {
             file.staged_path = Some(path.to_string_lossy().into_owned());
         }
-        params = match serde_json::to_value(&upload) {
-            Ok(v) => v,
-            Err(err) => {
-                return ResponseBody::Err(RpcError {
-                    code: ErrorCode::ProtocolError,
-                    message: format!("failed to serialise upload params: {err}"),
-                    data: None,
-                });
-            }
-        };
+        params = serde_json::to_value(upload).unwrap_or(Value::Null);
     } else if method == Method::ToolDownload {
         let mut download: DownloadParams = match serde_json::from_value(params) {
             Ok(v) => v,
@@ -459,16 +450,7 @@ async fn handle_tool_dispatch(
         download.browser_relative_dir = Some(staging.browser_relative_dir);
         download.max_byte_size = Some(super::file_transfer::MAX_TRANSFER_BYTES);
         download_transfer_id = Some(staging.transfer_id);
-        params = match serde_json::to_value(&download) {
-            Ok(v) => v,
-            Err(err) => {
-                return ResponseBody::Err(RpcError {
-                    code: ErrorCode::ProtocolError,
-                    message: format!("failed to serialise download params: {err}"),
-                    data: None,
-                });
-            }
-        };
+        params = serde_json::to_value(download).unwrap_or(Value::Null);
     }
     if let Some(audit_id) = audit_id
         && let Some(object) = params.as_object_mut()
@@ -523,7 +505,7 @@ async fn handle_tool_dispatch(
                 Ok(size) => {
                     result.byte_size = size;
                     result.transfer_id = Some(id);
-                    ok_value(result)
+                    ResponseBody::Ok(serde_json::to_value(result).unwrap_or(Value::Null))
                 }
                 Err(err) => {
                     state
@@ -562,7 +544,7 @@ fn handle_transfer_begin(state: &Arc<DaemonState>, params: Value) -> ResponseBod
         });
     }
     match state.transfers.begin_upload(p) {
-        Ok(v) => ok_value(v),
+        Ok(v) => ResponseBody::Ok(serde_json::to_value(v).unwrap_or(Value::Null)),
         Err(err) => ResponseBody::Err(err),
     }
 }
@@ -573,7 +555,7 @@ fn handle_transfer_chunk(state: &Arc<DaemonState>, params: Value) -> ResponseBod
         Err(err) => return ResponseBody::Err(invalid_params(err.to_string())),
     };
     match state.transfers.write_chunk(p) {
-        Ok(v) => ok_value(v),
+        Ok(v) => ResponseBody::Ok(serde_json::to_value(v).unwrap_or(Value::Null)),
         Err(err) => ResponseBody::Err(err),
     }
 }
@@ -584,7 +566,7 @@ fn handle_transfer_finish(state: &Arc<DaemonState>, params: Value) -> ResponseBo
         Err(err) => return ResponseBody::Err(invalid_params(err.to_string())),
     };
     match state.transfers.finish_upload(p) {
-        Ok(v) => ok_value(v),
+        Ok(v) => ResponseBody::Ok(serde_json::to_value(v).unwrap_or(Value::Null)),
         Err(err) => ResponseBody::Err(err),
     }
 }
@@ -595,7 +577,7 @@ fn handle_transfer_read(state: &Arc<DaemonState>, params: Value) -> ResponseBody
         Err(err) => return ResponseBody::Err(invalid_params(err.to_string())),
     };
     match state.transfers.read_chunk(p) {
-        Ok(v) => ok_value(v),
+        Ok(v) => ResponseBody::Ok(serde_json::to_value(v).unwrap_or(Value::Null)),
         Err(err) => ResponseBody::Err(err),
     }
 }
@@ -605,7 +587,7 @@ fn handle_transfer_release(state: &Arc<DaemonState>, params: Value) -> ResponseB
         Ok(v) => v,
         Err(err) => return ResponseBody::Err(invalid_params(err.to_string())),
     };
-    ok_value(state.transfers.release(p))
+    ResponseBody::Ok(serde_json::to_value(state.transfers.release(p)).unwrap_or(Value::Null))
 }
 
 fn invalid_params(message: impl Into<String>) -> RpcError {
@@ -616,36 +598,6 @@ fn invalid_params(message: impl Into<String>) -> RpcError {
     }
 }
 
-/// Serialise a handler result into a success body.
-///
-/// A serialisation failure (e.g. exceeding serde_json's recursion limit
-/// on a deeply nested payload) is surfaced as a `protocol_error` instead
-/// of silently degrading to JSON `null`. A `null` result would otherwise
-/// be indistinguishable from a method legitimately returning null and
-/// would hide the real failure from the CLI client; it also trips the
-/// wire decoder's "expected result or error" guard on peers that predate
-/// the explicit-null fix in `bsk-protocol/src/frame.rs`.
-fn ok_value(value: impl Serialize) -> ResponseBody {
-    match serde_json::to_value(&value) {
-        Ok(v) => ResponseBody::Ok(v),
-        Err(err) => ResponseBody::Err(RpcError {
-            code: ErrorCode::ProtocolError,
-            message: format!("failed to serialise handler result: {err}"),
-            data: None,
-        }),
-    }
-}
-
-/// Map a result-serialisation failure onto a structured `protocol_error`
-/// for handlers that return `Result<Value, RpcError>`.
-fn serialise_err(context: &str, err: serde_json::Error) -> RpcError {
-    RpcError {
-        code: ErrorCode::ProtocolError,
-        message: format!("failed to serialise {context}: {err}"),
-        data: None,
-    }
-}
-
 /// Upper bound on `tool.wait_ms` (5 minutes). Larger values are
 /// rejected as `invalid_params` so a buggy agent cannot wedge a
 /// daemon-side sleep beyond a reasonable window. The cap matches the
@@ -683,7 +635,9 @@ async fn handle_wait_ms(
         });
     }
     if params.duration_ms == 0 {
-        return ok_value(WaitMsResult { waited_ms: 0 });
+        return ResponseBody::Ok(
+            serde_json::to_value(WaitMsResult { waited_ms: 0 }).unwrap_or(Value::Null),
+        );
     }
     let guard = match registry.register(rpc_id) {
         Ok(guard) => guard,
@@ -698,7 +652,10 @@ async fn handle_wait_ms(
     let token = guard.token().clone();
     let result = tokio::select! {
         _ = tokio::time::sleep(Duration::from_millis(params.duration_ms)) => {
-            ok_value(WaitMsResult { waited_ms: params.duration_ms })
+            ResponseBody::Ok(
+                serde_json::to_value(WaitMsResult { waited_ms: params.duration_ms })
+                    .unwrap_or(Value::Null),
+            )
         }
         _ = token.cancelled() => {
             ResponseBody::Err(RpcError {
@@ -766,7 +723,7 @@ fn handle_cancel(state: &Arc<DaemonState>, params: Value) -> ResponseBody {
             );
         }
     }
-    ok_value(CancelResult { cancelled })
+    ResponseBody::Ok(serde_json::to_value(CancelResult { cancelled }).unwrap_or(Value::Null))
 }
 
 /// Test-only re-export of the cancel handler: the system-only handler
@@ -786,7 +743,7 @@ fn handle_cancel_with_registry_only(registry: &Arc<AbortRegistry>, params: Value
         }
     };
     let cancelled = registry.cancel(&params.rpc_id);
-    ok_value(CancelResult { cancelled })
+    ResponseBody::Ok(serde_json::to_value(CancelResult { cancelled }).unwrap_or(Value::Null))
 }
 
 fn tool_dispatch_timeout(params: &Value) -> Result<Duration, RpcError> {
@@ -915,8 +872,7 @@ async fn handle_status(
 ) -> Result<Value, RpcError> {
     let params: StatusParams = parse_params_or_default(params)?;
     maybe_wait_for_browser(state, params.wait_for_browser_ms).await;
-    serde_json::to_value(status.snapshot_with(state))
-        .map_err(|err| serialise_err("status snapshot", err))
+    Ok(serde_json::to_value(status.snapshot_with(state)).unwrap_or(Value::Null))
 }
 
 fn parse_params_or_default<T>(params: Value) -> Result<T, RpcError>
@@ -1020,7 +976,7 @@ async fn handle_session_start(
                 browser_instance_id: session.browser_id.0.clone(),
                 agent_window_id: session.agent_window_id,
             };
-            serde_json::to_value(result).map_err(|err| serialise_err("session start result", err))
+            Ok(serde_json::to_value(result).unwrap_or(Value::Null))
         }
         Err(err) => Err(map_start_error(err)),
     }
@@ -1128,7 +1084,7 @@ async fn handle_session_stop(
                 returned_tab_ids: stop.returned_tab_ids,
                 return_failures: stop.return_failures,
             };
-            serde_json::to_value(result).map_err(|err| serialise_err("session stop result", err))
+            Ok(serde_json::to_value(result).unwrap_or(Value::Null))
         }
         Err(StopSessionError::ReturnFailures(stop)) => {
             let message = format!(
@@ -1145,7 +1101,7 @@ async fn handle_session_stop(
                 returned_tab_ids: stop.returned_tab_ids,
                 return_failures: stop.return_failures,
             };
-            serde_json::to_value(result).map_err(|err| serialise_err("session stop result", err))
+            Ok(serde_json::to_value(result).unwrap_or(Value::Null))
         }
         Err(err) => Err(map_stop_error(err)),
     }
@@ -1269,7 +1225,7 @@ async fn handle_session_stop_all(
         returned_tab_ids,
         return_failures,
     };
-    serde_json::to_value(result).map_err(|err| serialise_err("session stop_all result", err))
+    Ok(serde_json::to_value(result).unwrap_or(Value::Null))
 }
 
 fn handle_session_list(state: &Arc<DaemonState>) -> ResponseBody {
@@ -1279,7 +1235,7 @@ fn handle_session_list(state: &Arc<DaemonState>) -> ResponseBody {
         .into_iter()
         .map(|s| s.status_entry())
         .collect();
-    ok_value(SessionListResult { sessions })
+    ResponseBody::Ok(serde_json::to_value(SessionListResult { sessions }).unwrap_or(Value::Null))
 }
 
 async fn handle_browser_list(state: &Arc<DaemonState>, params: Value) -> Result<Value, RpcError> {
@@ -1291,8 +1247,7 @@ async fn handle_browser_list(state: &Arc<DaemonState>, params: Value) -> Result<
     // ascending, with `instance_id` as a deterministic tiebreaker
     // (review I1).
     let browsers = snapshot_status_entries(&state.browsers, &state.sessions);
-    serde_json::to_value(BrowserListResult { browsers })
-        .map_err(|err| serialise_err("browser list result", err))
+    Ok(serde_json::to_value(BrowserListResult { browsers }).unwrap_or(Value::Null))
 }
 
 // ----- Test-helper IpcServer wrapper around the transport layer -----
@@ -2076,69 +2031,4 @@ mod tests {
         let _ = tx.send(());
         let _ = server.await;
     }
-
-    // A Serialize impl that always fails, standing in for a deeply
-    // nested payload that trips serde_json's recursion limit.
-    struct FailingSerialize;
-    impl serde::Serialize for FailingSerialize {
-        fn serialize<S>(&self, _serializer: S) -> Result<S::Ok, S::Error>
-        where
-            S: serde::Serializer,
-        {
-            Err(serde::ser::Error::custom("recursion limit exceeded"))
-        }
-    }
-
-    #[test]
-    fn ok_value_serialises_regular_result() {
-        match ok_value(PingResult { pong: true }) {
-            ResponseBody::Ok(v) => assert_eq!(v, serde_json::json!({ "pong": true })),
-            other => panic!("expected ok, got {other:?}"),
-        }
-    }
-
-    #[test]
-    fn ok_value_reports_protocol_error_on_failed_serialise() {
-        // A serialisation failure must surface as a structured
-        // protocol_error instead of silently degrading to JSON null.
-        match ok_value(FailingSerialize) {
-            ResponseBody::Err(err) => {
-                assert_eq!(err.code, ErrorCode::ProtocolError);
-                assert!(
-                    err.message.contains("failed to serialise"),
-                    "message: {}",
-                    err.message
-                );
-            }
-            other => panic!("expected protocol_error, got {other:?}"),
-        }
-    }
-
-    #[test]
-    fn serialise_err_builds_protocol_error_with_context() {
-        let err = serde_json::from_str::<serde_json::Value>("[").unwrap_err();
-        let rpc = serialise_err("status snapshot", err);
-        assert_eq!(rpc.code, ErrorCode::ProtocolError);
-        assert!(
-            rpc.message.contains("status snapshot"),
-            "message: {}",
-            rpc.message
-        );
-    }
-
-    #[test]
-    fn null_result_response_round_trips_over_frame_decode() {
-        // The daemon can legitimately emit an explicit null result;
-        // the CLI-side Frame decoder must treat it as a success body
-        // rather than an ambiguous frame (frame.rs regression guard).
-        let wire = r#"{"id":"rpc-null","result":null}"#;
-        let frame: Frame = serde_json::from_str(wire).expect("explicit null result decodes");
-        match frame {
-            Frame::Response(resp) => {
-                assert_eq!(resp.id, "rpc-null");
-                assert_eq!(resp.body, ResponseBody::Ok(serde_json::Value::Null));
-            }
-            other => panic!("expected response frame, got {other:?}"),
-        }
-    }
 }
diff --git a/crates/bsk-protocol/src/frame.rs b/crates/bsk-protocol/src/frame.rs
index abb0de8f..b5d84a60 100644
--- a/crates/bsk-protocol/src/frame.rs
+++ b/crates/bsk-protocol/src/frame.rs
@@ -94,13 +94,6 @@ impl<'de> Deserialize<'de> for ResponseFrame {
         #[derive(Deserialize)]
         struct Flat {
             id: RpcId,
-            // `#[serde(default, deserialize_with)]` keeps a present-but-null
-            // `result` distinct from an absent field: serde's plain
-            // `Option<Value>` collapses JSON `"result": null` (a legal
-            // JSON-RPC null result — see the daemon's
-            // `serde_json::to_value(..).unwrap_or(Value::Null)` fallback in
-            // `bsk-cli/src/daemon/ipc.rs`) into `None`, which would then
-            // misclassify an explicit null result as an ambiguous frame.
             #[serde(default, deserialize_with = "de_result_field")]
             result: Option<serde_json::Value>,
             error: Option<RpcError>,
@@ -122,15 +115,8 @@ impl<'de> Deserialize<'de> for ResponseFrame {
     }
 }
 
-/// Deserialise the `result` field of a response frame.
-///
-/// A plain `Option<serde_json::Value>` collapses JSON `null` into `None`
-/// (serde's `Option` treats `null` as "none"), which would make an
-/// explicit `{"result": null}` reply indistinguishable from a missing
-/// field and reject it as ambiguous. The daemon can legitimately emit an
-/// explicit null result (its `serde_json::to_value(..).unwrap_or(Value::Null)`
-/// fallback), so we parse the raw `Value` here and let the caller's match
-/// decide — a present-but-null result becomes `Some(Value::Null)`.
+// Preserve field presence: an explicit null is Some(Value::Null), while
+// #[serde(default)] leaves a missing result as None.
 fn de_result_field<'de, D>(deserializer: D) -> Result<Option<serde_json::Value>, D::Error>
 where
     D: Deserializer<'de>,
@@ -176,11 +162,7 @@ impl<'de> Visitor<'de> for FrameVisitor {
         let mut id = None::<RpcId>;
         let mut method = None::<Method>;
         let mut params = None::<serde_json::Value>;
-        // Outer `Option` = field presence, inner = the value. `"result":
-        // null` parses to `Some(None)` so an explicit null result is
-        // distinguishable from a missing field (see `ResponseFrame`'s
-        // `Flat` helper for the rationale).
-        let mut result = None::<Option<serde_json::Value>>;
+        let mut result = None::<serde_json::Value>;
         let mut error = None::<RpcError>;
         let mut event = None::<EventKind>;
         let mut payload = None::<serde_json::Value>;
@@ -250,14 +232,10 @@ impl<'de> Visitor<'de> for FrameVisitor {
 
         let id = id.ok_or_else(|| de::Error::missing_field("id"))?;
         match (result, error) {
-            (Some(Some(v)), None) => Ok(Frame::Response(ResponseFrame {
+            (Some(v), None) => Ok(Frame::Response(ResponseFrame {
                 id,
                 body: ResponseBody::Ok(v),
             })),
-            (Some(None), None) => Ok(Frame::Response(ResponseFrame {
-                id,
-                body: ResponseBody::Ok(serde_json::Value::Null),
-            })),
             (None, Some(e)) => Ok(Frame::Response(ResponseFrame {
                 id,
                 body: ResponseBody::Err(e),
@@ -315,51 +293,87 @@ mod tests {
 
     #[test]
     fn explicit_null_result_decodes_as_ok_null() {
-        // The daemon may legitimately emit `"result": null` (its
-        // `serde_json::to_value(..).unwrap_or(Value::Null)` fallback in
-        // `bsk-cli/src/daemon/ipc.rs`). A plain `Option<Value>` would
-        // collapse the explicit null into "field absent" and reject the
-        // frame as ambiguous; the inner `Option` keeps the distinction.
         let wire = serde_json::json!({ "id": "rpc-1", "result": null });
         let frame: Frame = serde_json::from_value(wire).unwrap();
-        match frame {
-            Frame::Response(resp) => {
-                assert_eq!(resp.id, "rpc-1");
-                assert_eq!(resp.body, ResponseBody::Ok(serde_json::Value::Null));
-            }
-            other => panic!("expected response frame, got {other:?}"),
-        }
+        assert_eq!(
+            frame,
+            Frame::Response(ResponseFrame {
+                id: "rpc-1".into(),
+                body: ResponseBody::Ok(serde_json::Value::Null),
+            })
+        );
     }
 
     #[test]
     fn explicit_null_result_response_frame_decodes_as_ok_null() {
-        // The dedicated `ResponseFrame` deserializer must agree with the
-        // generic `Frame` visitor on the same wire shape.
-        let wire = serde_json::json!({ "id": "rpc-2", "result": null });
-        let resp: ResponseFrame = serde_json::from_value(wire).unwrap();
+        let wire = r#"{"id":"rpc-2","result":null}"#;
+        let resp: ResponseFrame = serde_json::from_str(wire).unwrap();
         assert_eq!(resp.id, "rpc-2");
         assert_eq!(resp.body, ResponseBody::Ok(serde_json::Value::Null));
     }
 
     #[test]
     fn null_result_round_trips_through_serialise() {
-        // Serialising `Ok(Value::Null)` emits `"result": null`; decoding
-        // that wire shape must produce the same body (symmetric protocol).
-        let frame = Frame::Response(ResponseFrame {
+        let response = ResponseFrame {
             id: "rpc-3".into(),
             body: ResponseBody::Ok(serde_json::Value::Null),
-        });
-        let v = serde_json::to_value(&frame).unwrap();
-        let back: Frame = serde_json::from_value(v).unwrap();
-        assert_eq!(back, frame);
+        };
+        let wire = serde_json::to_value(&response).unwrap();
+        assert_eq!(wire, serde_json::json!({ "id": "rpc-3", "result": null }));
+        let back: ResponseFrame = serde_json::from_value(wire.clone()).unwrap();
+        assert_eq!(back, response);
+        let frame: Frame = serde_json::from_value(wire).unwrap();
+        assert_eq!(frame, Frame::Response(response));
     }
 
     #[test]
     fn missing_result_and_error_still_rejected() {
-        // A response with neither `result` nor `error` remains ambiguous —
-        // only an *explicit* null result is a legal success payload.
         let wire = serde_json::json!({ "id": "rpc-4" });
         assert!(serde_json::from_value::<Frame>(wire.clone()).is_err());
         assert!(serde_json::from_value::<ResponseFrame>(wire).is_err());
     }
+
+    #[test]
+    fn result_and_error_together_rejected() {
+        for result in [serde_json::Value::Null, serde_json::json!({ "pong": true })] {
+            let wire = serde_json::json!({
+                "id": "rpc-5",
+                "result": result,
+                "error": { "code": "protocol_error", "message": "test error" },
+            });
+            assert!(serde_json::from_value::<Frame>(wire.clone()).is_err());
+            assert!(serde_json::from_value::<ResponseFrame>(wire).is_err());
+        }
+    }
+
+    #[test]
+    fn duplicate_result_including_null_rejected() {
+        for wire in [
+            r#"{"id":"rpc-6","result":null,"result":1}"#,
+            r#"{"id":"rpc-6","result":1,"result":null}"#,
+        ] {
+            assert!(serde_json::from_str::<Frame>(wire).is_err());
+            assert!(serde_json::from_str::<ResponseFrame>(wire).is_err());
+        }
+    }
+
+    #[test]
+    fn non_null_results_and_errors_decode_consistently() {
+        for wire in [
+            serde_json::json!({ "id": "rpc-7", "result": false }),
+            serde_json::json!({ "id": "rpc-7", "result": 0 }),
+            serde_json::json!({ "id": "rpc-7", "result": "" }),
+            serde_json::json!({ "id": "rpc-7", "result": [] }),
+            serde_json::json!({ "id": "rpc-7", "result": {} }),
+            serde_json::json!({
+                "id": "rpc-7",
+                "error": { "code": "protocol_error", "message": "test error" },
+            }),
+        ] {
+            let response: ResponseFrame = serde_json::from_value(wire.clone()).unwrap();
+            let frame: Frame = serde_json::from_value(wire.clone()).unwrap();
+            assert_eq!(frame, Frame::Response(response.clone()));
+            assert_eq!(serde_json::to_value(response).unwrap(), wire);
+        }
+    }
 }

From 5c0bb72be84953275977274d6874858091022d48 Mon Sep 17 00:00:00 2001
From: Zhang GH <5351707+iuyo5678@users.noreply.github.com>
Date: Sun, 27 Sep 2026 13:44:33 +0800
Subject: [PATCH 3/3] fix(protocol): reject null error fields in response
 frames

---
 crates/bsk-protocol/src/frame.rs | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/crates/bsk-protocol/src/frame.rs b/crates/bsk-protocol/src/frame.rs
index b5d84a60..9af29730 100644
--- a/crates/bsk-protocol/src/frame.rs
+++ b/crates/bsk-protocol/src/frame.rs
@@ -94,8 +94,9 @@ impl<'de> Deserialize<'de> for ResponseFrame {
         #[derive(Deserialize)]
         struct Flat {
             id: RpcId,
-            #[serde(default, deserialize_with = "de_result_field")]
+            #[serde(default, deserialize_with = "de_present_field")]
             result: Option<serde_json::Value>,
+            #[serde(default, deserialize_with = "de_present_field")]
             error: Option<RpcError>,
         }
 
@@ -115,13 +116,14 @@ impl<'de> Deserialize<'de> for ResponseFrame {
     }
 }
 
-// Preserve field presence: an explicit null is Some(Value::Null), while
-// #[serde(default)] leaves a missing result as None.
-fn de_result_field<'de, D>(deserializer: D) -> Result<Option<serde_json::Value>, D::Error>
+// Deserialize present fields as their actual type: Value accepts null, but
+// RpcError requires an error object. Missing fields use #[serde(default)].
+fn de_present_field<'de, D, T>(deserializer: D) -> Result<Option<T>, D::Error>
 where
     D: Deserializer<'de>,
+    T: Deserialize<'de>,
 {
-    serde_json::Value::deserialize(deserializer).map(Some)
+    T::deserialize(deserializer).map(Some)
 }
 
 impl Serialize for Frame {
@@ -357,6 +359,18 @@ mod tests {
         }
     }
 
+    #[test]
+    fn null_error_is_rejected() {
+        for wire in [
+            r#"{"id":"rpc-null-error","error":null}"#,
+            r#"{"id":"rpc-null-error","result":null,"error":null}"#,
+            r#"{"id":"rpc-null-error","result":true,"error":null}"#,
+        ] {
+            assert!(serde_json::from_str::<Frame>(wire).is_err());
+            assert!(serde_json::from_str::<ResponseFrame>(wire).is_err());
+        }
+    }
+
     #[test]
     fn non_null_results_and_errors_decode_consistently() {
         for wire in [