feat(responses): opt-in ceiling for oversized outbound passthrough bodies - #3196
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. |
|
✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec162d99b0
ℹ️ 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".
| if (clientRequestedStream) { | ||
| return streamingContextOverflowResponse( | ||
| parsed._responseModelId ?? parsed.modelId, | ||
| translatorBudget, | ||
| ); |
There was a problem hiding this comment.
When a canonical OpenAI forward request exceeds the configured ceiling, this context_length_exceeded response directs Codex into compaction, but canonical compaction remains on this passthrough path and carries the full input plus a compaction_trigger. The resulting compaction request therefore exceeds the same unconditional byte check and is refused again instead of shrinking the conversation. Handle _compactionRequest specially—either allowing a viable compaction path or returning a terminal refusal that does not instruct Codex to compact—and add focused canonical-compaction coverage.
AGENTS.md reference: AGENTS.md:L339-L342
Useful? React with 👍 / 👎.
리뷰 · 우선순위 67 / 80현재 이 PR은 만든 네이티브 Responses 패스스루 본문이 운영자가 정한 바이트보다 크면, 업스트림에 보내기 전에 프록시가 거절합니다. 키를 안 쓰거나 0이면 측정조차 하지 않고 지금과 똑같이 보냅니다. 통합 테스트는 설정이 없을 때 20 MiB 본문이 실제로 fetch 된다고 확인합니다. 원래 기여자 PR #3142는 #3142에 없던 두 가지를 고칩니다. 첫째, 저장/메인 풀 401 재전송은 본문을 다시 만들고 검사 없이 보냈습니다. HEAD 막는 자리는 패스스루 갈래 안에서 넷입니다. 처음 만든 본문, 본문은 #2511을 닫지 않습니다. #2511은 제공자별 이미지 축소와 오래된 것부터 잘라 내기입니다. 이 PR은 거절만 합니다. 원래 기여자 PR은 #3142입니다. 그 PR은 이미 grok-bot 리뷰가 있고, 이 댓글은 #3142에 다시 달지 않습니다. 이 PR이 랜딩한 뒤 leftover로 닫으면 됩니다. CI에서 src/server/responses/core.ts retryCodexPoolOnAlternateAccount - 본문과 문서가 다섯 군데를 지킨다고 했지만, 이 함수는 새 본문을 만들고 바로 보냅니다. refuseOversizedOutboundBody를 부르지 않습니다. 그 헬퍼는 패스스루 갈래 안에만 있어서 여기까지 닿지 않습니다. #3142는 이 자리를 막았습니다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
…dies Reimplements #3142 (thanks @olddonkey) with the guard off by default, and adds the rebuild site and refusal shape that version was missing. The measurement, local refusal, image diagnostics, body-observation release and probe-lease handling are that PR's work and are kept. Why default-off rather than the 15 MiB default: the only measured ceiling in this codebase belongs to the WebSocket transport, and the comment recording it says the same body still succeeds over HTTP SSE. #2473 acts on that by falling back to HTTP rather than refusing, and #2426 records an 18.2 MB HTTP 200. A 15 MiB default would therefore refuse turns that work today - on canonical ChatGPT as well as on Azure and custom Responses gateways whose limits were never measured at all. An unset proxy now measures nothing and sends exactly what it sends today. Two fixes beyond the original: - The stored/main pool 401 replay rebuilds its body and sent it unchecked. It is now guarded like every other build site; a replay is precisely when a grown payload reappears. - A streaming refusal returns terminal response.failed / context_length_exceeded instead of a JSON 413. Codex treats HTTP 413 as a retryable transport error and resends the same oversized body, which is the loop this feature exists to stop. That is the contract the upstream-413 path already uses (#3177). RequestLogContext gains a proxy-owned errorCode so a locally refused request is named as such instead of being classified from a status with no upstream message behind it. Refs #3142. Related to #2511, which asks for per-provider downscaling and pruning and is deliberately not implemented here.
ec162d9 to
67835c0
Compare
1 participant
Summary
Reimplements #3142 (thanks @olddonkey) with the guard off by default, and adds the rebuild site and refusal shape that version was missing. Adds an opt-in
maxUpstreamBodyBytesceiling for serialized native Responses passthrough bodies.The byte measurement, local refusal, image diagnostics, body-observation release and probe-lease handling in #3142 are that PR's work and are kept as-is.
Why default-off rather than a 15 MiB default
#3142 ships
config.maxUpstreamBodyBytes ?? 15 MiB, which changes behavior for users who configure nothing. The evidence does not support any implicit ceiling:So a 15 MiB default refuses turns that succeed today. That holds on canonical ChatGPT too, which is why this goes further than the review's requested canonical-only default — and it applies to Azure and custom Responses gateways, whose limits were never measured at all.
With this PR an unset proxy measures nothing and sends exactly what it sends today. An operator who has hit a real wall sets one integer and gets an actionable local error instead of an opaque upstream failure.
This also restores the shape #2511 asked for (opt-in, default-off), so the two stop contradicting each other.
Two fixes beyond #3142
The 401 replay was unguarded. The stored/main pool 401 replay rebuilds its body and sent it unchecked — a replay is precisely when a grown payload reappears. It is now guarded like every other build site. The guard runs at all five: initial build,
rebuildAndRefetch, OAuth-refresh rebuild, alternate-account retry, and the 401 pool replay.A streaming refusal must be terminal. #3142 returns
formatErrorResponse(413, ...). Codex treats an HTTP 413 as a retryable transport failure and resends the same oversized body — the reconnect loop this feature exists to stop. Streaming refusals now returnstreamingContextOverflowResponse(response.failed/context_length_exceeded,retryable: false), which is the contract the upstream-413 path already uses (#3177). Non-streaming turns keep the JSON 413, where it is the right shape.RequestLogContextgains a proxy-ownederrorCodeso a locally refused request is named as such rather than classified from a status with no upstream message behind it.Verification
bun run privacy:scanreports one pre-existing hit indevlog/_plan/260902_multiplatform_qa_and_gui/091_wp6_merge_outcome.md(from3b9cddf00, unrelated to this branch).Activation-grounded regressions:
response.failed/context_length_exceeded, not a retryable 4130admits without measuring; a normal body still reaches upstreamfunction_call_output, singular/plural message formsChecklist
bun x tsc --noEmitpassesdocs-site/dev, branched from currentdev(c87071400, which carries fix(responses): surface provider 413 as terminal context overflow #3177)Scope
Does not close #2511. That issue asks for per-provider image downscaling and oldest-first pruning — active remediation that mutates request content. This is refusal only. It should make #2511 easier: the measurement helper, the image walk and the rebuild-site inventory are the parts that issue would need, and the key names do not collide.