Skip to content

fix(responses): exclude dropped chat reasoning from input admission - #5714

Closed
oocheol wants to merge 2 commits into
lidge-jun:devfrom
oocheol:codex/skip-dropped-openai-chat-reasoning-admission
Closed

oocheol wants to merge 2 commits into
lidge-jun:devfrom
oocheol:codex/skip-dropped-openai-chat-reasoning-admission

Conversation

@oocheol

@oocheol oocheol commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The pre-dispatch input gate counted replayed assistant thinking for every route, but openai-chat drops that history unless the model is in preserveReasoningContentModels. A long Codex thread could therefore get a local context_length_exceeded refusal despite sending a much smaller upstream request. The gate now uses the routed provider's same preservation rule for both direct and combo-target admission. Preserved openai-chat reasoning and other adapters still count it. Closes #5696.

Both direct and combo regressions verify that the long thinking string is absent from the dropped wire body and present when preserved, alongside the admission decisions. English/Korean architecture guidance and the Responses failover contract are updated.

Verification

Windows, Bun 1.4.0, isolated test homes:

$bun = '.\node_modules\@oven\bun-windows-x64\bin\bun.exe'
& $bun scripts/test.ts --parallel=1 ./tests/server/input-admission.test.ts ./tests/responses/responses-context-overflow.test.ts ./tests/adapters/openai/openai-chat-serialized-tool-call-think.test.ts
& $bun node_modules/typescript/bin/tsc --noEmit
& $bun scripts/structure-ssot.ts
& $bun scripts/privacy-scan.ts
git diff --check

Results: 51 pass, 0 fail; typecheck, structure, privacy, and diff checks passed. docs-site build passed (505 pages).

An initial parallel run of the same three files failed in responses-context-overflow.test.ts because multiple test servers in one process conflicted over the process-global spend-ledger home (SPEND_LEDGER_OWNER_HOME_CONFLICT); the complete serial rerun passed. Full-suite exception: the earlier --changed=dev run on this shared Windows host selected a large import-connected set and was stopped after more than four minutes without a result. It is not passing evidence. This bounded gate change was validated with the estimator, adapter, and request-level overflow suites; full-suite and cross-platform validation remain for CI.

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.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • Required local validation passed; commands, results, and any full-suite exception are documented.
  • I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
  • I resolved all correct Codex and CodeRabbit findings.
  • My PR is ready for review.

@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 9162dd18-f44a-4b65-b302-1e1e219805d3

📥 Commits

Reviewing files that changed from the base of the PR and between 3f55bc6 and ac697a7.

📒 Files selected for processing (2)
  • docs-site/src/content/docs/ko/reference/architecture.md
  • tests/server/input-admission.test.ts

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


📝 Walkthrough

Walkthrough

The admission estimator now follows the routed provider’s handling of replayed assistant thinking. It excludes thinking for openai-chat models that do not preserve reasoning content. Tests and documentation cover direct and combo admission behavior.

Changes

Input admission

Layer / File(s) Summary
Provider-aware estimation and validation
src/server/responses/input-admission.ts, tests/server/input-admission.test.ts, docs-site/src/content/docs/reference/architecture.md, docs-site/src/content/docs/ko/reference/architecture.md, structure/transports/responses-failover.md
estimateInputTokens accepts an optional provider and excludes assistant thinking when the adapter is openai-chat and the model is not in preserveReasoningContentModels. Direct and combo admission checks pass the provider. Tests cover dropped and preserved reasoning, including combo output headroom. The English and Korean architecture documentation and failover documentation describe the estimate behavior.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: lidge-jun

Merge Risk: ⚪ Minimal · up to ac697

The admission change is mergeable after normal checks; no actionable issue remains from this review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #5696 requires the pre-dispatch estimate to match the routed adapter's reasoning replay behavior for direct and combo admission. In src/server/responses/input-admission.ts, `estimateInputToken…
Out of Scope Changes check ✅ Passed The changed files remain connected to Issue #5696. docs-site/src/content/docs/reference/architecture.md and docs-site/src/content/docs/ko/reference/architecture.md document the same admission and …
Docstring Coverage ✅ Passed Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1 u…
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: excluding reasoning content that the chat adapter drops from input admission.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

@oocheol

oocheol commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@coderabbitai

coderabbitai Bot commented Sep 24, 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.

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

github-actions Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

✅ READY

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

Review readiness checklist

  • ✅ Required local validation passed; commands, results, and any full-suite exception are documented.
  • ✅ I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
  • ✅ 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

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 64 / 80

이 PR은 글을 모델에게 보내기 전에 "너무 길지 않은가"를 재는 문을 고칩니다.

긴 대화에는 모델이 예전에 속으로 한 생각(thinking)이 많이 쌓입니다. openai-chat으로 보내는데 그 모델 이름이 preserveReasoningContentModels에 없으면, 실제 전송은 그 생각을 빼고 보냅니다. 문은 지금까지 그 생각을 그대로 세었습니다. 그래서 보내는 글은 창에 들어가는데도, 로컬에서 "너무 길다"며 거절하는 일이 있었습니다. 이슈 #5696입니다.

이제는 그 목록에 없는 openai-chat 모델이면 생각을 세지 않습니다. 목록에 있는 모델과, openai-chat이 아닌 어댑터는 예전처럼 셉니다. 한 곳으로 바로 보내는 검사와, 콤보에서 다음 대상을 고르는 검사가 같은 규칙을 씁니다. 기준 브랜치는 dev입니다. 이 이슈를 다루는 다른 열린 PR은 없습니다.

docs-site/src/content/docs/ko/reference/architecture.md - 끝 문장이 일을 잘못 잇습니다. 잘못 거절을 막는 쪽은, 보내지 않는 생각을 계산에서 빼는 일입니다. 생각을 보존하는 모델을 계속 세는 일은 그 이유와 다른 문장으로 써야 합니다.

tests/server/input-admission.test.ts - 직접 경로 테스트는 전송 글에 reasoning_content라는 이름이 없는지만 확인합니다. 생각 글 3만 토큰이 다른 칸에 들어가도 통과합니다. 콤보 테스트는 거절 여부만 보고, 전송 글과 맞추지 않습니다.

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

이 PR은 아직 초안입니다. 준비 체크는 4개 중 0개입니다. 설명에는 관련 테스트 51개 통과와 타입 검사가 적혀 있습니다. 전체 테스트는 이 작업 컴퓨터에서 끝까지 돌리지 못했다고 본인이 적었습니다. 코드 방향과 별개로, 저장소 게이트는 체크를 채워야 초안을 풉니다.

계산은 어댑터와 같은 modelInList와 같은 parsed.modelId를 봅니다. 지금 호출부(request-prepare.ts)도 parsed.modelId를 넘기므로 맞습니다. 나중에 함수에 넘기는 모델 이름과 parsed.modelId가 달라지면 문과 전송이 다시 어긋날 수 있습니다.

너의 추천

고치는 방향은 맞습니다. 한국어 설명의 끝 문장을 나누고, 테스트가 긴 생각 문자열이 전송 본문에 없는지도 보면 됩니다. 그다음 준비 체크를 채우고 머지하면 됩니다.

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

@oocheol
oocheol force-pushed the codex/skip-dropped-openai-chat-reasoning-admission branch from 3f55bc6 to ac697a7 Compare September 24, 2026 01:40
@oocheol

oocheol commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 24, 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.

@github-actions
github-actions Bot marked this pull request as ready for review September 24, 2026 01:52
lidge-jun added a commit that referenced this pull request Sep 24, 2026
…, #5714, #5704, #5707) (#5738)

* fix(claude): bound Responses user to 64 chars for long metadata.user_id

Claude Code sends metadata.user_id as a ~186-char JSON string. It was copied
verbatim into the Responses 'user' field, which Azure OpenAI and other
OpenAI-compatible backends cap at 64 chars, so every Claude Code request
routed to Azure failed with 400 "Invalid 'user': string too long".

Keep short ids as-is and send the SHA-256 hex digest (already computed for
prompt_cache_key) when the id exceeds 64 chars.

Fixes #5705

Co-authored-by: Giulio Leone <giulioleone097@gmail.com>

* Fix plaintext V2 SSE responses with missing content type

Co-authored-by: Jerry WANG <jerrywang@Jerrys-MacBook-Pro-2.local>

* fix(responses): exclude dropped chat reasoning from input admission

* test(admission): verify reasoning payload matches gate decisions

Co-authored-by: 정우철 <oocheol@naver.com>

* Allow HTTP upstream for canonical ChatGPT provider

An explicit upstreamWebsocket: false now routes streaming canonical ChatGPT turns over HTTP/SSE before sending. This gives operators a supported escape from intermittent post-send WebSocket closes without replaying ambiguous turns. The default remains WebSocket and native WS controls are unavailable when HTTP is selected.

Verified: focused Responses/provider suites, typecheck, structure check, privacy scan, docs build. Changed-area suite rerun pending.
Co-authored-by: kosta <kosta963@gmail.com>

* test(claude): pin exact user hash and the 64/65-char boundary

Refs #5705

Co-authored-by: Giulio Leone <giulioleone097@gmail.com>

* docs(architecture): attribute the admission fix to excluding unsent thinking

Split the preserved-reasoning statement from the refusal rationale in the English and Korean paragraphs, as review of #5714 asked.

Refs #5696

Co-authored-by: 정우철 <oocheol@naver.com>

* fix(anthropic): count ping events as upstream liveness

Anthropic streams may include any number of ping events. The adapter only turned SSE comments into heartbeats, so a long silent thinking phase that pinged was cut off at stallTimeoutSec with upstream_stall_timeout. Named and data-only ping records now yield the same heartbeat.

Refs #5707

* fix(management): report upstreamWebsocket as configured in GET /api/providers

The row coerced an unset value to false. After the canonical ChatGPT opt-out, unset means upstream WebSocket and false means HTTP/SSE, so a save built from the row could turn WebSocket off. The row now omits the key when it is unset.

Co-authored-by: kosta <kosta963@gmail.com>

* docs(providers): describe the canonical ChatGPT upstreamWebsocket opt-out

The reference row, its seven translations, and the provider type and schema comments still said the canonical ChatGPT transport ignores upstreamWebsocket. It now selects HTTP/SSE when set to false. The locale rows were also behind the English row on the first-party-only restriction; they are retranslated from it.

Co-authored-by: kosta <kosta963@gmail.com>

* fix(responses): bound the plaintext V2 SSE prefix probe by stallTimeoutSec

The carried probe read the first chunk of an unlabeled body with no deadline, before the passthrough stall guard is attached. An upstream that sent headers but no body held the request and its host lease until the client gave up, and a failed read escaped the classifier. Each probe read now races a per-read inactivity window and one total budget, both stallTimeoutSec, plus the client abort signal. Timeout, abort, and read errors cancel the reader and return the existing unsupported-content-type 502. Docs now scope the recovery to missing or unrecognized non-JSON content types.

Follow-up to #5683 review (maintainer, Codex, CodeRabbit).

Co-authored-by: Jerry WANG <jerrywang@Jerrys-MacBook-Pro-2.local>

---------

Co-authored-by: Giulio Leone <giulioleone097@gmail.com>
Co-authored-by: Jerry WANG <jerrywang@Jerrys-MacBook-Pro-2.local>
Co-authored-by: 정우철 <oocheol@naver.com>
Co-authored-by: kosta <kosta963@gmail.com>
@lidge-jun

Copy link
Copy Markdown
Owner

Carried into bundle #5738, which is now on dev (squash-merged as df61bce) with a Co-authored-by trailer for you, so this PR is closing as landed. Thank you for the fix. If something from this branch did not make it in, the bundle description lists what was changed during the carry.

@lidge-jun lidge-jun closed this Sep 24, 2026
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.

2 participants