Web socket api refactor - #3704
Open
jmthomas wants to merge 9 commits into
Open
Conversation
The two clients had drifted in ten ways. Fixes three real bugs: python disconnect leaked the socket when the courtesy unsubscribe failed, add and remove defaulted to the import-time OPENC3_SCOPE so an api built with an explicit scope streamed from DEFAULT, and read_all crashed on len(None) when the socket closed before the empty-batch end marker. Python also now raises on missing auth env vars instead of returning None, normalizes empty and malformed frames to None, honors Script Runner stop via a sys.modules lookup that avoids an import cycle, accepts int nanoseconds as well as datetimes, tolerates a disconnect frame with no reason, and gains the missing SystemEventsWebSocketApi. Ruby normalizes generate_url to the ws/wss schemes, which python's websockets library requires. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collapse the timeout/auth keyword block repeated across 13 constructors into DEFAULT_OPTIONS plus **options forwarding, the 7 near-identical history_count event classes into a HistoryCountIdentifier mixin driven by a CHANNEL constant, and the two generate_url bodies into a shared cable_url helper. Extract write_command, parse_message/check_protocol_frame and stream_action so the command framing, frame parsing and action building each live in one place. Behavior is unchanged except that a typo'd option name now raises instead of being silently swallowed by **options, restoring what explicit keyword arguments used to catch. Adds direct tests for write, which is still public API but no longer has an internal caller now that write_action frames directly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3704 +/- ##
==========================================
+ Coverage 79.35% 79.60% +0.25%
==========================================
Files 885 885
Lines 65365 65364 -1
Branches 2591 2591
==========================================
+ Hits 51870 52033 +163
+ Misses 12826 12658 -168
- Partials 669 673 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the WebSocket API implementations (Ruby and Python) to reduce duplication, normalize URL generation, and tighten protocol/error-handling behavior while expanding unit test coverage to validate the new behaviors.
Changes:
- Centralized common WebSocket options handling, protocol frame checks, and command-frame writing logic.
- Normalized cable URL generation from environment variables (including ws/wss scheme normalization and OPENC3_DEVEL hostname behavior).
- Added extensive Ruby RSpec and Python unittest coverage for connect/disconnect, subscribe handshake, cooperative stop, identifier construction, and streaming read-all behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| openc3/spec/script/web_socket_api_spec.rb | Adds broad RSpec coverage using a fake stream and env helper to validate the refactored Ruby WebSocket API behavior. |
| openc3/python/test/script/test_web_socket_api.py | Adds comprehensive Python unit tests mirroring Ruby coverage for the refactored Python WebSocket API. |
| openc3/python/openc3/script/web_socket_api.py | Refactors Python WebSocket API: shared option validation, protocol handling, URL builder, streaming helpers, and safer disconnect behavior. |
| openc3/lib/openc3/script/web_socket_api.rb | Refactors Ruby WebSocket API: shared option validation, cable URL builder, protocol parsing/checking, streaming helper, and improved internal factoring. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Malformed frames and mid-stream closes were swallowed as end-of-stream, turning protocol errors into a silent exit 1 in cli_script_monitor and a nil-deref in the documented consumer loops. Let them surface instead. read_all now distinguishes a bounded query, where a missing end marker means a truncated result and must raise, from a realtime query, which is never sent one and simply keeps what it collected. Ruby's end_time is optional to match Python, and disconnect clears @subscribed even when the courtesy unsubscribe fails on a half-closed socket. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
Behavior changes (both Ruby and Python):
Python changes:
Ruby changes:
|
EmilyRagan
reviewed
Aug 12, 2026
ryan-pratt
reviewed
Aug 17, 2026
Comment on lines
+78
to
+88
| # Options every websocket api accepts, and their defaults. Subclasses | ||
| # forward **options rather than restating these. | ||
| DEFAULT_OPTIONS = { | ||
| "write_timeout": 10.0, | ||
| "read_timeout": 10.0, | ||
| "connect_timeout": 5.0, | ||
| "authentication": None, | ||
| } | ||
|
|
||
| def __init__(self, url, scope=OPENC3_SCOPE, **options): | ||
| """Create the WebsocketApi object |
Contributor
There was a problem hiding this comment.
just a note that this is a potential breaking change if people called it with positional args
Member
Author
There was a problem hiding this comment.
I went through the usage throughout our code and I don't see any use of positional args. I will call this out in the release notes.
|
jmthomas
requested review from
EmilyRagan and
ryan-pratt
and
a balanced review from Copilot
August 18, 2026 13:24
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
openc3/python/openc3/script/web_socket_api.py:525
Nonedoes not uniquely mean the socket closed:WebSocketClientStream.read()also returnsNonewhenever its read timeout expires (10 seconds by default). Consequently a quiet bounded query raises “closed” even while connected, and the new realtime mode exits after 10 seconds despite being documented as endless. Distinguish timeout from EOF (or explicitly configure and handle the stream timeout) before deciding whether to raise or break.
if batch is None:
# A bounded query must receive its end marker; a truncated
# result returned as if complete is worse than an error. A
# realtime query never gets one, so a close is an ordinary
# way for it to end.
if end_time is not None:
raise RuntimeError("WebSocket closed before end marker")
break
openc3/lib/openc3/script/web_socket_api.rb:468
- Making
end_timeoptional introduces a realtime mode, butself.newstill uses the 10-second defaultread_timeout. On a quiet realtime stream,api.readtherefore raisesTimeout::Errorafter 10 seconds, so this cannot stream endlessly as documented; the method-leveltimeoutalso cannot control that read. Configure the socket timeout for this mode and handle its expiration as the collection timeout rather than an error.
def self.read_all(items: nil, packets: nil, start_time: nil, end_time: nil, scope: nil, timeout: nil)
read_all_start_time = Time.now
data = []
self.new do |api|
api.add(items: items, packets: packets, start_time: start_time, end_time: end_time, scope: scope)
while true
batch = api.read
| "authentication": None, | ||
| } | ||
|
|
||
| def __init__(self, url, scope=OPENC3_SCOPE, **options): |
ryan-pratt
approved these changes
Aug 18, 2026
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.
The progression of commits is important. I first created more unit tests to verify existing functionality. Brought coverage to 100%. Then reconciled the Ruby and Python behavior. Then refactored to remove duplication in source. Then refactored to remove duplication in test.