fix(ipc): decode explicit null results and stop silent null degradation - #287
Open
cyberspace-cs wants to merge 1 commit into
Open
cyberspace-cs wants to merge 1 commit into
cyberspace-cs wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a silent-failure channel in the IPC protocol where JSON
nullwas treated as "field absent" instead of a legal value.
1. Decoder: explicit
nullresults were rejected as ambiguousResponseFrame::deserializeand theFramevisitor modelled theresultfield asOption<Value>. serde collapses JSON"result": nullinto the same
Noneas a missing field, so a reply like{"id":"x","result":null}was rejected with a misleadingambiguous response: expected exactly one of result or error.That shape is not hypothetical: the daemon emits it through
serde_json::to_value(..).unwrap_or(Value::Null)(see below), andValue::Nullserialises back to"result": null— so the encoder anddecoder were asymmetric.
Both deserializers now parse the raw value
(
#[serde(default, deserialize_with)]) so an explicitnullresultdecodes to
Ok(Value::Null)while a missing field stays an error.2. Daemon: result serialisation failures silently degraded to
nullEvery
serde_json::to_value(..).unwrap_or(Value::Null)site silentlyturned a result serialisation failure — e.g. a payload nested deeper
than serde_json's 128-level recursion limit — into a JSON
nullresult, hiding the real failure from the CLI client and colliding with
legitimate null results.
All of these now route through
ok_value()/serialise_err()helpers that return a structured
protocol_errorwith the underlyingserde message. The upload/download param-staging paths (which
forwarded
nullparams to the extension on failure) now return thesame error instead.
Testing
cargo test -p bsk-protocol— 158 passed (4 new: explicit-nulldecode for
FrameandResponseFrame, serialise round-tripsymmetry, missing-field regression)
cargo test -p bsk --lib— 328 passed, 2 failed (both pre-existingenvironment failures in
skill_install::harnesson hosts with a realhermesinstall; unrelated to this change)cargo fmtclean on both crates