Skip to content

feat(remote_device_bridge): ITransport with TCP and WebSocket - #1322

Open
jkim89 wants to merge 5 commits into
jonghyunk/pr01-wire-codecfrom
jonghyunk/pr03-websocket-transport
Open

feat(remote_device_bridge): ITransport with TCP and WebSocket#1322
jkim89 wants to merge 5 commits into
jonghyunk/pr01-wire-codecfrom
jonghyunk/pr03-websocket-transport

Conversation

@jkim89

@jkim89 jkim89 commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Transport layer for the remote plugin session: an ITransport seam with TCP and WebSocket implementations.

What changes

  • ITransport (send/receive complete wire frames)
  • TcpSocket / TcpTransport (POSIX, timeouts, buffer sizes)
  • WebSocketTransport (one frame per RFC 6455 binary message, no TLS — terminate wss:// in a proxy)
  • parse_remote() / connect_transport() for --remote=<host:port> or ws://host:port[/path]
  • Diagnostics go through isaaccapture::Logger

What does not change

Wire format, plugins, runtime behavior. The planned separate TCP-seam PR is folded in here because it has no tests of its own.

Testing

test_websocket_transport.cpp (5 cases: URL parsing, every length class, control frames/fragments, bad handshakes, connect_transport). Fresh build: ctest 271/271.

Depends on

#1321

@coderabbitai

coderabbitai Bot commented Oct 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@jkim89 jkim89 mentioned this pull request Oct 22, 2026
Open
@jkim89
jkim89 requested a review from tengw-nv October 23, 2026 21:31
@jkim89
jkim89 marked this pull request as ready for review October 27, 2026 22:48
@jkim89
jkim89 force-pushed the jonghyunk/pr01-wire-codec branch from fe5f90f to 6452f88 Compare October 30, 2026 19:12
@jkim89
jkim89 force-pushed the jonghyunk/pr03-websocket-transport branch from 7ea56d3 to 2aaf4b2 Compare October 30, 2026 19:12
jkim89 and others added 5 commits October 30, 2026 14:34
@jkim89
feat(remote_device_bridge): add ITransport, TCP and WebSocket transports
2fbc483 
Sessions talk to the bridge through ITransport, which sends and receives
complete wire frames. TcpTransport frames them over one TCP connection
(TcpSocket owns the POSIX socket, timeouts and buffer sizes);
WebSocketTransport carries exactly one frame per RFC 6455 binary message
(no TLS; terminate wss:// in a proxy). parse_remote()
and connect_transport() turn --remote=<host:port> or
ws://host:port[/path] into a connected transport. The TCP seam has no
test file of its own, so it ships with the WebSocket transport and its
tests.

Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
@jkim89 @claude
rebase onto pr00 review
9c0cb2a 
pr01's codec became remote_plugin_protocol (protocol_frame.hpp /
protocol_payloads.hpp): the transports include protocol_frame.hpp and use
protocol::kHeaderSize, protocol::decode_header and protocol::DecodeError
through a namespace alias. The session CMake and AGENTS.md conflicts are
resolved onto the split file names.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
@jkim89 @claude
review round 1 (pr03)
2818cb3 
Applied the fact-checked (VERIFIED) findings on code this branch adds:
- TcpSocket: setsockopt failures (TCP_NODELAY, SO_SNDTIMEO, SO_REUSEADDR)
  throw with errno; connect/listen attempts throw their reason instead of
  bool + out-param, including a failed socket(); addrinfo is held by a
  unique_ptr; parse_endpoint accepts [v6]:port and bare v6 hosts and
  rejects trailing garbage ("80x"); EAGAIN branches that a blocking socket
  cannot hit are gone; the once-per-process buffer-size log is dropped
  (and with it the log_bridge link).
- TcpTransport moves to tcp_transport.{hpp,cpp}; parse_remote and
  connect_transport move to remote_address.{hpp,cpp}; RemoteAddress
  carries std::optional ws_path instead of bool + path.
- accept() reports the peer through TcpSocket::accept, so the two
  transports' accept functions no longer copy each other.
- One kMidMessageStall and one kConnectTimeout in transport.hpp.
- WebSocket: named header-bit constants; protocol violations (unmasked
  client frame, RSV bits, oversized control frame, text/unknown opcodes)
  are Malformed, not Closed; the client upgrade error says what failed;
  the Host value is named host_header.
- Threads is linked by the test that spawns them, not the library.
- Tests: TcpTransport::receive cases, invalid/IPv6 endpoint cases,
  wait_ready drives the server handshake, kHeaderSize instead of 24.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
@jkim89 @claude
review round 2 (pr03)
42e4fa1 
Applied the fact-checked (VERIFIED) findings on code this branch adds:
- TcpSocket::recv_all returns ReceiveResult (its Ok/Timeout/Closed subset),
  so TcpTransport no longer maps one enum onto the other; peer_name() is
  private; a failed connect/listen reports every address's error.
- parse_endpoint moves to remote_address.hpp with parse_remote, and parses
  the port with std::from_chars; the Host header brackets IPv6 hosts.
- WebSocketTransport: an explicit server constructor and a client
  connect() factory replace the role flag plus client-only defaults.
- One accept_transport<T> helper instead of two copies of accept.
- WebSocket: reserved control opcodes are Malformed; starts_with instead
  of rfind(..., 0).
- TcpTransport::receive drops a check recv_all already covers; kSendTimeout
  says why it is independent of the other 3 s constants.
- Tests: oversized and reserved-opcode WebSocket cases.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
@jkim89 @claude
review round 3 (pr03)
506f44a 
Applied the fact-checked (VERIFIED) findings on code this branch adds:
- WebSocketTransport's client role is a constructor that performs the
  upgrade and throws, instead of a connect() factory plus a second step.
- A failed PONG send in receive() reports Closed instead of being ignored.
- send_all drops a check for a zero-byte send() of a non-empty buffer.
- parse_remote/parse_endpoint and TcpTransport tests move to
  test_remote_address.cpp and test_tcp_transport.cpp.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
@jkim89
jkim89 force-pushed the jonghyunk/pr01-wire-codec branch from 6452f88 to c43a9c9 Compare October 30, 2026 21:42
@jkim89
jkim89 force-pushed the jonghyunk/pr03-websocket-transport branch from 2aaf4b2 to 506f44a Compare October 30, 2026 21:42
@jkim89

jkim89 commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Update (Oct 30): rebased onto #1320/#1321; no functional change beyond the rebase. Run through the review skill locally (3 rounds) before pushing.

This branch was successfully deployed

1 active deployment
dev — 506f44ac Deployed Oct 30, 2026 by jkim89 via publish-wheel #6345
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tengw-nv tengw-nv Awaiting requested review from tengw-nv

Assignees

@jkim89 jkim89

Labels

NVIDIA

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jkim89 @ivany-nv