fix(protocol): preserve explicit null results in ResponseFrame - #287
Merged
iuyo5678 merged 4 commits intoSep 27, 2026
Merged
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.
Collaborator
|
Could you narrow this PR to the ResponseFrame explicit-null decoding fix and its regression tests? |
iuyo5678
changed the title
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
2 participants
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
Directly deserializing
{"id":"x","result":null}asResponseFramepreviously rejected a valid success response, including one produced by serializingResponseBody::Ok(Value::Null). Preserve the presence of theresultfield so explicit null decodes successfully, while responses with neither field, both fields, or a null error are rejected.The change is scoped to
ResponseFramedecoding, regression tests, and the changelog. Tests cover explicit null, serialization round trips, missing and conflicting fields, duplicate result fields, invalid null errors, and compatibility withFrame, non-null results, and error responses.Validation
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --offline --locked -- -D warningscargo test --offline --locked -p bsk-protocol— 164 passedCARGO_NET_OFFLINE=true node scripts/check-crate-skill.mjscargo test --workspace --offline --locked— 771 passed, 0 failed, 1 ignored (macOS)5c0bb72.