Skip to content

fix(responses): finalize adopted WebSocket stage records - #4607

Open
luvs01 wants to merge 3 commits into
lidge-jun:devfrom
luvs01:agent/ws-stage-telemetry-20260914
Open

luvs01 wants to merge 3 commits into
lidge-jun:devfrom
luvs01:agent/ws-stage-telemetry-20260914

Conversation

@luvs01

@luvs01 luvs01 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

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 c529b1dd530c314837d93544cc61e6868de2b9bb includes dev snapshot aa91958e3b050084e1edc07dcd66b05ef6eac604. 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.

  • WebSocket stage coverage: 93 pass, 461 assertions across three files.
  • Typecheck and structure ownership checks passed on the integrated source. Tests were scoped to the changed contracts; this does not claim that every platform matrix was repeated locally. New file-size ratchet conflicts, where present, were fixed by extracting coherent source or test modules, without raising the baseline or removing assertions.
  • Full hosted CI was not multiplied across unchanged implementation merges. The focused integration evidence above and applicable repository checks define this author-validation scope; skipped or unexecuted matrices are not passing results.

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.

  • All CI tests are green on my local testing.
  • I pushed my PR to the latest dev commit.
  • I resolved all correct Codex and CodeRabbit findings.
  • My PR is ready for review.

Summary by CodeRabbit

  • Bug Fixes

    • Improved WebSocket response telemetry so stage records retain their identity while receiving final success or failure details.
    • Kept telemetry isolated between separate responses.
    • Improved reporting of frame counts, relayed events, request bytes, and connection close codes.
    • Ensured canceled connections finalize transport telemetry and persist complete usage details.
  • Documentation

    • Clarified stage record identity and stream-buffer accounting expectations across transport, runtime, provider, client, and operational documentation.

@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: ec597869-21b3-488c-8028-367aa19a65f6

📥 Commits

Reviewing files that changed from the base of the PR and between 5227e2b and c529b1d.

📒 Files selected for processing (6)
  • structure/catalog.md
  • structure/gui-and-management-api.md
  • structure/providers/xai-grok.md
  • structure/runtime.md
  • structure/subagents.md
  • structure/transports/responses.md

Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The 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.

Changes

Codex WebSocket stage record identity

Layer / File(s) Summary
Stage record contract
structure/transports/responses.md:464, structure/adapters/registry.md:6, structure/catalog.md:6, structure/clients/claude-desktop.md:9, structure/data-planes/images.md:10, structure/data-planes/inbound-compat.md:50, structure/gui-and-management-api.md:4, structure/ops/service-and-sidecars.md:9, structure/providers/xai-grok.md:10, structure/runtime.md:10, structure/subagents.md:28, structure/transports/byte-accounting.md:5, structure/transports/inventory.md:9, structure/transports/streaming-health.md:13
The documentation defines fresh stage snapshots, in-place updates for the same response-local record, and separate records for separate responses. Related documents link to this contract.
In-place stage updates
src/server/responses/codex-ws-wire.ts:111-115
markCodexWsStage merges a new stage record into an existing response entry with Object.assign. It uses set only when the response has no existing entry.
Cancellation telemetry finalization
src/server/relay-eager.ts:469-470
The cancellation path aborts the upstream before it invokes onClientCancel.
Identity and terminal-state tests
tests/responses/ws-failure-stage.test.ts:2-6, tests/responses/ws-failure-stage.test.ts:312-460
Tests verify stable references, response isolation, successful and failed terminal counters, close-code and byte recording, cancel-drain usage persistence, callback behavior, and upstream abortion.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to c529b

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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: finalizing adopted WebSocket stage records for Responses telemetry.
Full details: Docstring Coverage

Explanation

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.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 14, 2026
@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

  • ✅ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ✅ My PR is ready for review.

4/4 boxes ticked.

This pull request has been marked Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers notified: @lidge-jun @Ingwannu

@luvs01

luvs01 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 67 / 80

설명

이 PR은 #4191 계열 WebSocket stage 텔레메트리의 채택(adopt) 참조가 최종 스냅샷을 못 보는 구멍을 고칩니다. 현재 dev HEAD 는 ab6fd697c (#4606) 입니다. src/server/responses/codex-ws-wire.tsmarkCodexWsStage 는 지금도 WeakMap.set 만 합니다. handleResponsesadoptCodexWsStage(약 5380–5387줄) 는 Response 가 커밋되는 순간 readCodexWsStage(response) 로 객체를 읽어 logCtx.activeAttempt.codexWsStage 에 같은 참조를 붙입니다.

문제는 교환이 나중에 성공·실패 카운터를 다시 markCodexWsStage 할 때, 맵 엔트리만 새 객체로 갈아끼운다는 점입니다. attempt 가 들고 있는 옛 참조는 그대로라 close code·실패 바이트·최종 프레임 수가 usage/attempt 쪽에 안 보입니다. 본문이 말한 “텔레메트리 정확도만, 재전송·자격증명·스키마 변경 없음”과 맞습니다.

고침은 작습니다. 이미 레코드가 있으면 Object.assign(current, record) 로 제자리 갱신하고, 없으면 예전처럼 set 합니다. 테스트는 tests/responses/ws-failure-stage.test.ts 에서 (1) 한 Response 의 채택 참조가 최종 스냅샷과 같은 객체인지, (2) 다른 Response 레코드는 건드리지 않는지, (3) 성공/늦은 close 경로에서 커밋 전에 잡은 참조가 터미널 뒤에도 채워지는지를 고정합니다. 방향이 옳고, 현재 dev 의 adopt 퍼널과도 맞습니다.

다만 diff 의 대부분이 코드가 아니라 structure 문서 연쇄 링크입니다. 14개 가까운 structure/**/*.md 에 같은 한 문장(“Response-attached WebSocket telemetry follows the stage record identity contract”)을 붙였습니다. structure:check 소유자 링크 요구를 맞추려는 의도일 수 있지만, 리뷰·충돌 면에서는 소음이 큽니다. 본문 검증도 솔직합니다. 포커스 스위트는 초록인데 test:changed 는 900초 타임아웃(exit 124)으로 끝났고, Exact-head CI 는 대기/미완으로 읽힙니다. 체크리스트에도 “ready for review / all CI green” 이 비어 있습니다.

베이스로 쓴 스냅샷은 b8d90ba3a (#4604) 이고, 그 사이 dev 에는 #4605·#4606 이 올라왔습니다. 이 파일들(codex-ws-wire.ts / ws-failure-stage 테스트)과 직접 겹칠 확률은 낮아 보이며 GitHub 상 MERGEABLE 입니다. 그래도 Exact-head 를 현재 tip 기준으로 다시 돌리는 편이 안전합니다. types.ts/config.ts 분할과 무관하고, #4608 send-budget 과도 경로가 다릅니다.

라인 (PR 브랜치 markCodexWsStage Object.assign 분기) - 채택 참조가 최종 필드를 보게 하는 최소 수정입니다. 부분 스냅샷이 오면 이전 필드를 덮어쓰므로, 호출부가 “매번 완전한 fresh snapshot” 을 넣는다는 계약을 지켜야 합니다. 본문·structure 서술이 그 계약을 이미 말하고 있습니다.

경로 tests/responses/ws-failure-stage.test.ts - 참조 동일성(toBe(firstAdopted))과 응답 간 격리를 같이 단언한 점이 좋습니다. 성공 경로에서 커밋 전 adopt → 이후 completed 이벤트로 카운터 채움 순서도 실제 handleResponses 퍼널과 맞습니다.

경로 structure/*.md 다수 - 동일 문장 대량 추가는 소유자 링크 요구 충족용으로 보이지만, 핵심 계약 문장은 structure/transports/responses.md 한곳에 두고 나머지는 기존 byte-accounting 문장만 유지하는 편이 diff 가 읽기 쉽습니다. 필수인지 structure:check 실패 로그로 한 번만 확인하면 좋습니다.

경로 검증/CI - 포커스 16 pass 는 설득력 있고, broad test:changed 실패·타임아웃은 미귀속으로 남아 있습니다. merge 조건은 Exact-head CI 초록으로 두는 것이 맞습니다.

메인테이너의 판단이 필요한 지점

  • Exact-head CI(현재 tip ab6fd697c 기준 rebase/재실행 포함) 초록을 merge 게이트로 둘지 (추천: 예)
  • structure 문서 14파일 동일 문장을 이 PR에 유지할지, responses.md 중심으로 줄일지
  • Object.assign 이 부분 갱신을 허용할지, 호출부가 항상 전체 스냅샷만 넣는 계약을 테스트로 더 못 박을지
  • broad test:changed 타임아웃 로그의 미귀속 실패를 블로커로 볼지, 포커스+Exact-head 만으로 충분할지
  • types.ts/config.ts 분할과 무관 — close-don't-rebase 대상 아님

너의 추천

코드 한 줄(제자리 갱신)과 참조 동일성 테스트는 머지 가치가 있습니다. 현재 tip 에 Exact-head CI 초록을 확인한 뒤 병합하세요. structure 대량 링크가 check 에 꼭 필요 없다면 줄이거나 후속 문서로 미루면 리뷰 부담이 줄어듭니다. ready 체크리스트가 비어 있으므로, 초록 나오기 전에는 merge 하지 마세요.

이 댓글은 grok-bot이 작성했습니다

@luvs01

luvs01 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-15T01:19:33.651601Z c529b1d New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/server/responses/codex-ws-wire.ts
@luvs01

luvs01 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@luvs01

luvs01 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: 5227e2b2c7

ℹ️ 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".

@luvs01
luvs01 marked this pull request as ready for review September 14, 2026 09:54

@Ingwannu Ingwannu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions
github-actions Bot marked this pull request as draft September 15, 2026 01:17
@github-actions
github-actions Bot marked this pull request as ready for review September 15, 2026 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working review-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants