fix(handshake): check node response envelope instead of blindly returning result#109
Open
Sentinel-Bluebuilder wants to merge 1 commit into
Conversation
…ning result
The node wraps every response as { success, result?, error? }. A failed
handshake (bad signature, session not active on-chain, node-side error) can
arrive as HTTP 200 with success:false and an error payload. The previous code
returned response.data.result unconditionally (with a comment literally
'supposing success: True and error doesn't exist'), so failures surfaced as
undefined further down the tunnel-setup path instead of a clear error.
Now handshake() throws a descriptive error (including the node's error code and
message when present) on success:false / error payload, and on a successful
response missing result. JSDoc @throws updated to match.
Closes the original intent of sentinel-official#34. The mistitled sentinel-official#75 promised this but never
implemented it (and re-introduced a separately-rejected uid change); this is the
clean, correctly-scoped version against src/utils.ts.
Sentinel-Bluebuilder
added a commit
to Sentinel-Bluebuilder/sentinel-js-sdk-1
that referenced
this pull request
Jun 12, 2026
…ning result blindly
nodeInfo() returned (response.data as NodeResponse).result without checking
the { success, result?, error? } envelope. On an HTTP-200 success:false
response the function returned undefined, which then crashed downstream in
node selection / connection setup with an unrelated error.
Mirror the envelope check from the handshake() fix (sentinel-official#109): throw a descriptive
error carrying the node's code + message on success:false / error payload, and
throw when result is missing. Sibling of sentinel-official#109 - same unchecked-result bug.
Verified: tsc --noEmit exit 0.
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.
What
handshake()insrc/utils.tsnow validates the node's response envelope before returning, instead of blindly returningresponse.data.result.Why
Every node response is wrapped as
NodeResponse { success: boolean, result?, error?: { code, message } }(seesrc/types.ts). A failed handshake — bad signature, session not active on-chain, or any node-side error — can come back as HTTP 200 withsuccess: falseand anerrorpayload. axios does not throw on that, so the old code:returned
undefined(the comment even admits it assumes the happy path). The failure then surfaced much later as an opaque crash during tunnel setup (result.addrsof undefined, etc.) instead of a clear, actionable error at the handshake boundary.Change
JSDoc
@throwsupdated to document the new behavior. No type changes needed —NodeResponse/NodeResponseErroralready model this exactly.Scope / relationship to #75
This is the clean, correctly-scoped implementation of the original error-checking request (#34). The earlier #75 was titled "add response error checking" but never touched the return path — it only flipped
uuid→uidin an example (a change already merged as wrong via #90 and rejected once in #78). #75 has been closed; this PR does what #34 actually asked for, againstsrc/utils.ts, and nothing else.Verification
tsc --noEmitpasses clean on this branch.Follow-up (not in this PR)
nodeInfo()(same file) has the identical pattern —return (response.data as NodeResponse).result as NodeInfowith nosuccess/errorcheck. Left out to keep this PR focused on the handshake path; happy to send a sibling PR applying the same guard there.