Repository navigation
refactor(websocket): add SDK-owned Responses policy helpers - #4219
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. |
|
The new entry guards correctly prioritize the initiating exception over failures from There is one adjacent cleanup-failure boundary worth covering before the helpers are wired into the public connection managers. In both
A deterministic regression could inject:
The desired contract, if clearing is not guaranteed to be infallible, would be: attempt connection cleanup, retain the initiating exception, and avoid silently replacing the primary failure with a secondary cleanup error. A narrowly scoped If every production This is a source-level failure-path observation, not a reproduced production failure. It is particularly relevant because these private helpers are being qualified before their public-manager rollout. |
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed exact commit f5c59a0. The shared helpers preserve Responses connection policy, prepared headers/query options, reconnect and unsent-message behavior, progress accounting, and cleanup exception precedence. Existing cleanup comments address the feedback; the BaseException test intentionally checks cancellation/control-flow preservation. Coordinator and independent static review found no actionable issues. Hosted checks are successful or explicitly skipped. No candidate code or tests executed.
Add private SDK-owned helpers for Responses WebSocket setup, iteration, progress tracking, and cleanup. Focused sync and async tests cover prepared headers, argument collisions, generator controls, retry progress, and cleanup error precedence.
Existing public managers do not yet call these helpers. Rollout needs three steps: merge this helper change into public main, land compiler delegation, then regenerate and promote the bindings. The compiler must wait for the merged helper change; an open PR is insufficient. The first dependency needs no package release.
Validation: 585 focused WebSocket tests passed, including the 18 new helper cases. Ruff formatting and lint, plus ty 0.0.87 checks for both changed files, passed.