Skip to content

test(remote): add coverage for the remote gateway server #288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cyberspace-cs wants to merge 2 commits into
base: main
Choose a base branch
from
Open

test(remote): add coverage for the remote gateway server #288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
57dd73f
fix(ipc): decode explicit null results and stop silent null degradation
cyberspace-cs Sep 19, 2026
d0cd44d
test(remote): add coverage for remote gateway server
cyberspace-cs Sep 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
166 changes: 138 additions & 28 deletions crates/bsk-cli/src/daemon/ipc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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,
Expand All @@ -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()
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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),
}
}
Expand All @@ -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),
}
}
Expand All @@ -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),
}
}
Expand All @@ -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),
}
}
Expand All @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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> {
Expand Down Expand Up @@ -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>
Expand Down Expand Up @@ -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)),
}
Expand Down Expand Up @@ -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!(
Expand All @@ -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)),
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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> {
Expand All @@ -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 -----
Expand Down Expand Up @@ -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:?}"),
}
}
}
Loading