Repository navigation
Conversation
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. |
|
No actionable comments were generated in the recent review. ℹ️ Recent review info
Walkthrough
Merge Risk:
|
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR. The refactoring cleanly extracts the WebSocket upgrade handler into a testable installResponsesWsUpgradeHandler function, solves the competing-listener race with a well-designed emit-level dispatcher, and includes comprehensive regression tests covering all routing scenarios.
PR Size: M
- Lines changed: 492 (457 additions, 35 deletions)
- Files changed: 4
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean (JS file; test types are correct)
- Documentation accuracy - Clean
- Test coverage - Thorough (8 test cases covering competing listeners, delegation, rejection, error paths, and end-to-end forwarding)
- Code clarity - Good
Automated review by Claude AI
b38b2bd to
d63ba84
Compare
Symptoms: Responses WebSocket connections can close immediately after a successful 101 handshake, causing clients to report "Connection reset without closing handshake" and repeatedly reconnect. Different worker initialization orders can produce alternating successful and failed connections. Cause: CCH and Next.js can both handle the same WebSocket upgrade event. After CCH completes the handshake, the Next.js listener can close the accepted socket. Solution: Route /v1/responses upgrades exclusively to CCH before invoking ordinary upgrade listeners. Preserve listener dispatch for other paths, the current worker's private HTTP loopback target, payload limits, and cleanup behavior.
d63ba84 to
35e5c2a
Compare
Responses WebSocket connections can close immediately after a successful 101 handshake when CCH and Next.js both handle the same upgrade event.
Route
/v1/responsesupgrades exclusively to CCH before ordinary upgrade listeners run. Preserve delegation of other upgrade paths, listener semantics, the private worker HTTP target, payload limits, and shutdown cleanup.Validation: The standalone branch based on upstream v0.9.8 passed production build, Biome check and autofix, TypeScript checking, and the full Vitest suite in an isolated environment: 9,442 tests passed and 13 skipped, using a 30-second per-test timeout. Regression tests cover competing listener order, upgrade delegation, and forwarding to the private worker target.


Confidence Score: 5/5
The PR appears safe to merge; no blocking issues were found.Summary
The PR gives CCH sole ownership of
/v1/responsesWebSocket upgrades, preventing competing listeners from closing an accepted connection.No actionable issues were found. No tests were run during this review.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[HTTP upgrade] --> B{Responses path?} B -->|Yes| C{WebSocket support available?} C -->|Yes| D[CCH handles upgrade exclusively] C -->|No| E[Destroy socket] B -->|No| F{Other upgrade listeners?} F -->|Yes| G[Call original emitter] F -->|No| EReviews (3) · Last reviewed commit: "fix(proxy): prevent Responses WebSocket ..." · Reviewed by Greptile