fix(responses): finalize adopted WebSocket stage records - #4607
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe change makes response-local Codex WebSocket stage records update in place. Cancellation now aborts the upstream before usage persistence. Documentation defines the identity contract, and tests cover success, failure, isolation, and cancel-drain persistence. ChangesCodex WebSocket stage record identity
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The WebSocket telemetry changes preserve response-local records and finalize cancellation data before persistence, with focused coverage and successful current-head CI. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. (6 skipped: 6 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
리뷰 · 우선순위 67 / 80설명 이 PR은 #4191 계열 WebSocket stage 텔레메트리의 채택(adopt) 참조가 최종 스냅샷을 못 보는 구멍을 고칩니다. 현재 문제는 교환이 나중에 성공·실패 카운터를 다시 고침은 작습니다. 이미 레코드가 있으면 다만 diff 의 대부분이 코드가 아니라 structure 문서 연쇄 링크입니다. 14개 가까운 베이스로 쓴 스냅샷은 라인 (PR 브랜치 경로 경로 경로 검증/CI - 포커스 16 pass 는 설득력 있고, broad 메인테이너의 판단이 필요한 지점
너의 추천 코드 한 줄(제자리 갱신)과 참조 동일성 테스트는 머지 가치가 있습니다. 현재 tip 에 Exact-head CI 초록을 확인한 뒤 병합하세요. structure 대량 링크가 check 에 꼭 필요 없다면 줄이거나 후속 문서로 미루면 리뷰 부담이 줄어듭니다. ready 체크리스트가 비어 있으므로, 초록 나오기 전에는 merge 하지 마세요. 이 댓글은 grok-bot이 작성했습니다 |
|
@codex review |
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd3bbbf11c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Ingwannu
left a comment
There was a problem hiding this comment.
Approved on exact head 5227e2b2c709db1fe20329107ac5af9fc6e1c23f.
I traced both production changes through the response lifecycle. markCodexWsStage now preserves the response-local object already adopted by logging while replacing every required stage field in place; the isolation regression proves another Response record is untouched. In the cancellation path, AbortController.abort() synchronously finalizes the WebSocket stage before onClientCancel performs the usage write, remains idempotent with the later teardown abort, and is still gated behind cancelled && !sawTerminal(), so a real terminal retains priority.
The focused suite observes success, late socket failure, byte-limited cancel drain and persisted stage bytes, and the exact-head hosted ordinary/macOS matrix is green. This approval is for the reviewed head only and is not a merge action.
…-stage-telemetry-20260914
Summary
Finalize WebSocket stage records when a Responses request adopts the already-running upstream stream, including terminal outcome and timing rather than leaving the adopted stage open.
Current author verification
Published head
c529b1dd530c314837d93544cc61e6868de2b9bbincludes dev snapshotaa91958e3b050084e1edc07dcd66b05ef6eac604. The actual branch connected to this PR was read back after publication. This section replaces older head and validation claims; earlier CI results are historical evidence only.Review readiness checklist
The local-validation box refers to the explicit scope and exceptions above. Author readiness remains separate from approval, merge permission and future review findings.
Summary by CodeRabbit
Bug Fixes
Documentation