Skip to content

fix(chat): preserve completion semantics in JSON-to-SSE fallback - #3779

Closed
Ingwannu wants to merge 1 commit into
devfrom
ingw/fix-chat-json-sse-parity
Closed

fix(chat): preserve completion semantics in JSON-to-SSE fallback#3779
Ingwannu wants to merge 1 commit into
devfrom
ingw/fix-chat-json-sse-parity

Conversation

@Ingwannu

@Ingwannu Ingwannu commented Sep 6, 2026

Copy link
Copy Markdown
Owner

Delivered — superseded by merged work

Verified in dev at 5759d9ea2f1e7281cdc01eb9628f2e0a123fb59c: #3803 (ac4a7659fd).

Original contribution: fix(chat): preserve completion semantics in JSON-to-SSE fallback, by @Ingwannu.

JSON-to-stream tools, reasoning, usage and finish-reason preservation, extended across both fallback paths.

The original PR is closed as superseded; its contribution remains credited in the landed history.

Attribution strengthened by #3811, merged as cf9f662190c4c6770697c45c870941509cc98f9c. See CREDITS.md for the source-to-landing attribution record.

Summary

Closes #3770.

  • Repair only the inbound Chat streaming fallback used when the Responses pipeline returns complete JSON. The existing converter retains tool calls, reasoning and incomplete status, but the old emitter discarded them and always reported stop.
  • Reuse the converted message: emit indexed function calls, reasoning/text (and refusal when already supplied by the converter), and its actual finish reason. Preserve usage, the single terminal marker, and the existing translation-budget lifecycle.
  • No routing, authentication, retry, native Chat passthrough, or real upstream-SSE translator changes. This does not implement [Bug]: Responses-to-Chat translation silently drops refusal text in streaming and JSON responses #3767's earlier refusal conversion. JSON fallback remains buffered delivery, not upstream token streaming.
  • Add actual-handler regressions and update the protocol reference and transport decision record. Built on dev at bd1cda99c.

Verification

Using Bun 1.4.0, fresh temporary HOME/OPENCODEX_HOME/CODEX_HOME directories, and a local synthetic upstream only:

  • Before the runtime fix: all four defect cases failed (single/parallel tools, reasoning, and length).
  • bun test ./tests/responses/chat-json-sse-fallback.test.ts ./tests/test-layout.test.ts ./tests/test-layout-tooling.test.ts: 24 pass, 0 fail, 629 assertions. Seven handler cases also cover ordinary text, empty output, cancellation, single upstream dispatch, one terminal marker, usage, and budget release.
  • bun node_modules/typescript/bin/tsc --noEmit: passed.
  • bun scripts/privacy-scan.ts: passed.
  • Docs: bun node_modules/astro/bin/astro.mjs build: 425 pages built successfully. Existing chunk-size and missing 404-content warnings remain.
  • git diff --check: passed.
  • Full suite, hosted exact-head CI, and Windows/macOS execution are not completed. Kept Draft pending those gates and independent maintainer review. No production configuration or daemon was changed.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. No such boundary is changed; fixtures are synthetic and privacy scan passes.

@lidge-jun 쉽게 요약하면, 스트리밍을 요청했는데 내부 응답이 JSON으로 돌아오는 경우에만 도구 호출과 추론 내용이 빠지고 종료 사유가 무조건 정상 종료로 바뀌던 문제입니다. 기존 변환 결과를 그대로 전달하게 고쳤고, 실제 HTTP 핸들러 테스트로 수정 전 실패와 수정 후 통과를 확인했습니다. #3767과는 분리했습니다. 아직 전체·플랫폼 검증 전이라 Draft로 두며, 병합하지 않았습니다.

@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 72 / 80

이 PR은 #3770을 닫으려는 Draft 수정입니다. Chat 클라이언트가 stream: true인데 내부 Responses 파이프라인이 완성 JSON을 돌려주는 “JSON-to-SSE 폴백”에서, 이미 변환된 completion의 도구 호출·reasoning·실제 finish_reason을 버리고 텍스트만 흘린 뒤 종료를 항상 stop으로 찍던 구멍을 막습니다. HEAD bd1cda99csrc/server/chat-completions.ts L458–L477을 보면, responsesJsonToChatCompletion으로 만든 completion에서 message.content만 꺼내 프레임을 만들고 마지막에 finish_reason: "stop"을 하드코딩합니다. 그런데 같은 변환기(src/chat/outbound.ts L563–)는 이미 tool_calls, reasoning_content, incomplete→length를 채웁니다. 즉 의미는 변환 결과에 있는데 스트리밍 폴백 emitter만 버리는 상태입니다. PR은 변환된 message를 재사용해 indexed tool_calls·reasoning/text(그리고 변환기가 이미 준 refusal이 있으면 그것)·실제 finish_reason을 내고, usage·단일 terminal·[DONE]·translation-budget 생명주기는 유지합니다. 라우팅/인증/재시도/네이티브 Chat/실제 upstream SSE 번역기는 건드리지 않고, #3767 refusal 변환 본편도 범위 밖으로 명시한 점이 release-244 기준으로 좋습니다.

변경 표면은 작습니다. chat-completions.ts 폴백 블록, 핸들러 회귀 tests/responses/chat-json-sse-fallback.test.ts, 레이아웃 등록, proxy-formats·structure/04_transports-and-sidecars.md 결정 기록입니다. 로컬 검증(해당 테스트 24 pass, tsc, privacy-scan, astro build) 서술은 구체적이고, Draft로 전체·크로스플랫폼 CI를 기다리는 태도도 맞습니다. 다만 HEAD 변환기 자체는 아직 refusal content part를 모으지 않으므로, PR 본문의 “refusal when already supplied by the converter”는 지금 converter가 refusal을 넣기 전까지는 사실상 no-op입니다. #3767과 섞지 않은 선택은 유지하되, 그 문장은 “converter가 채운 경우에만 전달”로 읽는 게 맞습니다. types/config 분할과 무관하고, #3770 전용 수리로 머지 가치가 큽니다.

라인 src/server/chat-completions.ts 폴백(HEAD L458–477) - content만 복사하고 finish_reason을 stop으로 고정하던 경로를 변환 message 재사용으로 고침(이 PR의 핵심)
경로 src/chat/outbound.ts responsesJsonToChatCompletion - tool/reasoning/length는 이미 있음. refusal part 수집은 아직 없어 PR이 말하는 refusal 전달은 converter 확장 전까지 비활성
경로 새 chat-json-sse-fallback.test.ts - 단일/병렬 tool_calls, reasoning, length, 일반/빈 completion, budget release를 핸들러 수준으로 고정한 점은 좋음
경로 문서 structure/04_transports-and-sidecars.md - “buffered delivery이지 token streaming이 아님”을 Decision Log에 박은 것이 범위 오해를 막음
경로 Draft/CI - 작성자 말대로 전체 스위트·Windows/macOS exact-head는 아직. 머지 전 그 게이트를 보는 게 맞음
경로 #3767 - refusal 본편과 분리한 것은 유지. 이 PR에 refusal 변환을 끼워 넣지 말 것

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

  • Draft를 Ready로 올리기 전 최소 게이트: 이 PR 테스트+tsc만으로 충분한지, Cross-platform CI green을 필수으로 볼지
  • refusal을 #3767이 converter에 넣기 전까지 폴백 delta 문서/코드에서 빼 혼동을 줄일지
  • JSON 폴백이 도구 호출을 한 덩어리 delta로 내는 것이 Chat 클라이언트 호환에 충분한지(일부 클라이언트는 스트리밍 index 증분을 기대)
  • #3770 close 조건을 이 PR 머지로 확정할지, 실측 클라이언트(특정 IDE/CLI) 스모크 후 close할지

너의 추천
방향 승인입니다. CI(특히 Cross-platform) green 확인하고 Draft 해제한 뒤 dev에 머지하세요. 범위 확대해서 #3767을 끌어오지 마세요. 머지 후 #3770은 landed 코멘트와 함께 닫으면 됩니다. refusal 문구는 converter가 실제로 공급할 때까지 “전달만 한다”고 짧게 정리해 두면 리뷰 오해가 줄어듭니다.

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

@lidge-jun

Copy link
Copy Markdown
Owner

Landed via #3803 at ac4a7659fd833ad89c525497abd7847640c30b53.

@lidge-jun lidge-jun added the landed-via-maintainer Original PR closed after landing via a maintainer merge train label Sep 6, 2026
@lidge-jun lidge-jun closed this Sep 6, 2026
everton-dgn pushed a commit to everton-dgn/opencodex that referenced this pull request Sep 6, 2026
Carry lidge-jun#3779 and cover incomplete terminal precedence and bounded serialization.

Co-authored-by: Ingwannu <ingwannu@users.noreply.github.com>
@lidge-jun
lidge-jun deleted the ingw/fix-chat-json-sse-parity branch September 9, 2026 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working landed-via-maintainer Original PR closed after landing via a maintainer merge train

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants