Repository navigation
refactor: share existing WebSocket send and open handling - #2947
Conversation
Castiron-Internal-PR: openai/openai-node-internal#306 Castiron-Source-SHA: 68a9ec540ee28500d40b6258020e04da6c6010ff Castiron-Public-Base-SHA: 4ca7bd6
Castiron custom codeEvaluated main: ✅ No new custom-code files detected. 51 mixed files remain; 5 existing customizations changed. Compared
46 existing customizations unchanged
6 more in the full report. A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 38682391927 --repo openai/openai-node \
--name castiron-custom-code-38682391927-1 --dir /tmp/castiron-custom-code-38682391927-1
git apply --stat /tmp/castiron-custom-code-38682391927-1/custom-code.patch
cat /tmp/castiron-custom-code-38682391927-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 4ca7bd61c05effec8d2ace880a000047da37f1f3 62553b2d647c0ead0ed629e929bb4ecdc44f6782
python3 scripts/castiron/custom_code_report.py report \
--base 4ca7bd61c05effec8d2ace880a000047da37f1f3 \
--head 62553b2d647c0ead0ed629e929bb4ecdc44f6782 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-62553b2d647c
cat /tmp/castiron-custom-code-62553b2d647c/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed all seven changed files at 62553b2d647c0ead0ed629e929bb4ecdc44f6782. No blocking findings.
The extraction preserves dynamic reads of the public socket, lazy reconnect-state checks, JSON/raw queue admission and error propagation, and the existing listener cleanup when waiting for open. The different failed-send policies are retained: Responses requeues the attempted item, while Live/forks/sideband discard that attempted item and retain never-attempted messages. The new compatibility tests cover reconnect-on-socket-read, socket replacement, and failed-write retention across all five variants. Public/protected signatures, authentication, reconnect policy, and package/runtime requirements are unchanged in this patch.
Validation: static source and surrounding-code review, plus git diff --check. I did not execute project code or run tests/builds locally. Exact-head hosted CI is green, including Node 22/24/26 tests, lint, builds, ecosystem tests, breaking-change detection, CodeQL, baseline consistency, and both trusted Castiron statuses.
2 participants
Why
Responses and Live repeat the same socket-open checks, sends, and queue flushing. Sharing these operations reduces duplication while preserving each API's current behavior.
High-level description
Extract four internal functions and use them from stable/beta Responses and Live's main, fork, and sideband connections. Reuse the existing queue and transport types.
Keep credential refresh, reconnect timing, event parsing, errors, and public/protected interfaces unchanged. Preserve the different failed-send policies: Responses retains the queued item; Live removes an attempted item. Compatibility tests also cover socket replacement and reconnects triggered while reading the socket.
Generator ownership, further lifecycle consolidation, and intentional behavior changes remain separate follow-ups. Realtime and Translation are unchanged.
Validation
Public head:
62553b2d647c0ead0ed629e929bb4ecdc44f6782, based on4ca7bd61c05effec8d2ace880a000047da37f1f3. Its complete source tree matches the locally tested commit.Hosted CI passed on this head. Local results above also cover package and compatibility checks; generated HTTP-resource tests were not rerun locally.