[WRONG BRANCH] fix(combos): reserve output headroom before a combo fallback - #4750
Conversation
…[skip ci] A combo could route a large turn onto a fallback whose total context window cannot hold the input plus the output allowance the caller asked for. That target answers 200, emits a few hundred tokens and stops on finish_reason: length, which the Anthropic surface renders as "response exceeded the output token maximum" naming a limit the model never approached. Raising CLAUDE_CODE_MAX_OUTPUT_TOKENS only changes the number in that message. By the time it happens, output has committed and no later target may be tried. Admit a combo child against both budgets before dispatch. When the caller declared max_output_tokens, require estimated input <= input ceiling AND estimated input + min(declared output, target output ceiling) <= context window, and refuse locally with 413 input_admission_refused before any upstream bytes are sent. Combo policy already treats that local code as a safe hop, so the ladder selects a larger-context target without replaying committed output. The two budgets are checked separately on purpose. resolveInputCeiling already answers "how much input may this target take", and modelMaxInputTokens can tighten it below the window; charging the output reserve against that tightened number would count the reserve twice and skip a target that fits. The window is what input and output actually share, so the reserve belongs there. Reserving min(declared, target ceiling) rather than a fixed slice is what makes this catch the reported case: the common industry reservation of min(max_output, 20k) leaves 100k + 20k inside a 128k window, so the turn is admitted and fails upstream anyway. Canonical native slugs that the narrower pinned table does not carry now resolve their window from the generated in-tree bundle. That table gap is why the gate was completely inert on the route where this was observed. The bundle is compiled in, not a catalog read, so this adds no I/O, and explicit provider and operator caps may only narrow the result. It deliberately covers slugs retired from the picker, because a retired slug is still dispatchable when an operator names it explicitly in a combo target, which is exactly that configuration. Scope stays narrow. Direct and single-target requests keep the deliberately loose 2.5x pathological-input gate, because they have nowhere to hop. Compaction turns stay exempt. Unknown context and a caller that declared no output allowance both remain fail-open, so no limits are invented for custom providers. Closes #4664 Co-authored-by: RHODIZ IT <info.rhodiz@gmail.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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. |
리뷰 · 우선순위 76 / 80이 PR은 RHODIZSECURITY의 #4664를 현재 고침은 콤보 자식(
다만 머지 레인은 아직 막혀 있다. base가 라인/심볼로 보면 아래가 맞다. 라인 266-285 (input-admission.ts · checkComboTargetInputAdmission) - 창/입력 예산을 분리해 예약을 한 번만 센다. 콤보에서만 쓰는 계약이 tip 방향과 맞다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![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: bc648a2f84
ℹ️ 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".
| const configured = positive(modelRecordValue(provider.modelMaxOutputTokens, modelId)) | ||
| ?? positive(provider.defaultMaxOutputTokens); |
There was a problem hiding this comment.
modelMaxOutputTokens and defaultMaxOutputTokens are documented wire defaults, not capability limits (structure/config.md:212-217), and adapters such as openai-chat.ts:49-52 and anthropic.ts:963-967 let an explicit request override them. Thus, for example, a 128k-window combo target with an 80k input, a configured 32k default, and an explicit 64k output request is admitted after reserving only 32k, but the adapter sends 64k and recreates the context-overflow/truncation this change is intended to prevent. Reserve the full explicit allowance unless a genuine per-model capability ceiling is obtained through the canonical catalog/derivation path.
AGENTS.md reference: src/AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
…ws [skip ci] OPENAI_CODEX_PROVIDER_ID is the routing provider name, and its value is the string "openai". Using it to index the generated bundle therefore skipped the native Codex rows entirely and read the public API rows instead. The two agree on Spark's 128k window, so the case that motivated the fallback still resolved, but any slug where they differ would have taken the wrong window -- and gpt-5-codex-mini exists only in the native catalog, so it resolved nothing at all. Name the catalog keys explicitly and say in a comment why the provider id is not one of them. Co-authored-by: RHODIZ IT <info.rhodiz@gmail.com>
…llowance [skip ci] The no-declared-allowance row passed `undefined` as the second argument of a builder whose parameter has a default. A default parameter applies to an explicit `undefined`, so the row built a request carrying 64,000 max output tokens and then asserted that no output reserve was applied. It would have asserted the opposite of what it covers, and it would have done so by passing. Split the builder in two so the no-allowance case cannot silently acquire one.
The remembered model id is provider-reported and arrives on the response, so nothing upstream of the recall store bounds its length. Lane keys are already SHA-256 digests, which means the 256-lane cap bounded the number of entries but not the bytes those entries held. A long-running process could accumulate arbitrarily large remembered strings. Bound retention on two more axes: 1 KiB per remembered model id and 64 KiB in aggregate. The size test runs on code units before encoding, because a UTF-8 encoding is never smaller than its code-unit count, so the bound never pays the allocation it exists to prevent. Aggregate eviction drops the least recently written lane, which is the front of the map because every write re-inserts its own lane at the back. A single entry is capped far below the aggregate budget, so a write can never evict itself. Every removal now goes through one helper that releases the entry's bytes, so the counter cannot drift from the map through the read-time invalidation path, the reconciliation path, or a lane rewrite. An unretainable model id DECLINES the write rather than clearing the lane. That is the ordering-sensitive part. This callback carries a config generation, not a request order, so two accepted completions on one lane under the same generation can arrive out of order; a clearing branch would let the older one erase the newer selection. Declining matches how every other rejection in rememberComboForLane already returns, and leaves the established contract intact: an older response never overwrites or clears a newer one. Register the store for periodic expiry as well. The TTL was previously evaluated only on read or on a generation change, so a lane that is never read again held its entry until the process exited. Closes #4525 Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
fix(responses): bound combo recall model retention
|
Cascading the lane downward. This branch now carries the whole lane; its head tree matches the verified tip tree exactly. Evidence at the exact head 246d703 (tree
Chained-child stacks merge top-down, so this lands in the parent branch and cascades to Maintainer integration decision under MAINTAINERS.md / AGENTS.md, recorded with the exact-head evidence above. |
99749e1
into
codex/cf1-definite-context-overflow
⏳ DRAFT
What to do
Its title has been prefixed with |
github-actions
Bot
changed the title
1 participant
Summary
Carries #4664 by @RHODIZSECURITY onto current
dev, stacked on #4744. Base iscodex/cf1-definite-context-overflow; retarget todevonce the parent lands.A combo could route a large turn onto a fallback whose total context window cannot hold the input plus the output allowance the caller asked for. That target answers 200, emits a few hundred tokens, and stops on
finish_reason: length— which the Anthropic surface renders asClaude's response exceeded the 64000 output token maximum, naming a limit the model never approached. RaisingCLAUDE_CODE_MAX_OUTPUT_TOKENSonly changes the number in that message. By the time it happens output has committed, so no later target may be tried.A combo child is now admitted against both budgets before dispatch. When the caller declared
max_output_tokens,checkComboTargetInputAdmissionrequiresestimated input <= input ceilingandestimated input + min(declared output, target output ceiling) <= context window, and refuses locally with413 input_admission_refusedbefore any upstream bytes are sent. Combo policy already treats that local code as a safe hop, so the ladder selects a larger-context target without replaying committed output.Three things differ from the original patch:
resolveInputCeilinganswers "how much input may this target take", andmodelMaxInputTokenscan tighten it below the window. The original checkedinput + headroom <= ceiling, which charges the output reserve against an already-tightened input budget and would skip a target that fits. The window is what input and output actually share, so the two conditions are now evaluated separately against the budget each one belongs to.gpt-5.3-codex-sparkis inRETIRED_NATIVE_OPENAI_MODELSon currentdev. The fallback deliberately covers retired slugs, because a retired slug is still dispatchable when an operator names it explicitly in a combo target — which is precisely the configuration where the gate was inert. The comment and the test now say that.min(max_output, 20k); that leaves 100k + 20k inside a 128k window, so the reported turn would still be admitted and still fail upstream.min(declared, target ceiling)is what actually catches it.Scope stays narrow. Direct and single-target requests keep the deliberately loose 2.5x pathological-input gate, because they have nowhere to hop. Compaction turns stay exempt. Unknown context and a caller that declared no output allowance both remain fail-open, so no limits are invented for custom providers.
Verification
No local suite, no focused test file, no typecheck, no build and no dependency install was run — this lane is under an explicit owner instruction forbidding local execution. Evidence is static source reading plus hosted CI.
Static checks performed:
src/generated/model-metadata.ts:openaicarries["gpt-5.3-codex-spark",128000,32000,...]andopenai-codexcarries["gpt-5.3-codex-spark",128000,128000,...]. Output ceiling therefore resolves throughnativeOpenAiMaxOutputTokens, which reads theopenairow (32k), not theopenai-codexrow.NATIVE_OPENAI_MODELSand the capability-alias list (it appears only inRETIRED_NATIVE_OPENAI_MODELS,native-models.ts:181), soPINNED_NATIVE_CAPABILITY_ENTRIESdoes not hold it and the generated fallback is the path actually taken.resolveInputCeilingassertion intests/server/input-admission.test.tsagainst the refactor. The generated fallback is consulted only when the native lookup returns null, so the pinned 272k Sol window, the impostor-provider null, the routedprovider/modelnull and the explicit 50k override are all unchanged.tests/for assertions that depend on canonical native admission resolving nothing for a previously unknown slug; there are none.src/generated/model-metadata.tshas no imports at all, socore.ts → request-prepare.ts → input-admission.ts → generated/model-metadata.tscannot reachsrc/lab(tests/lab/core-lab-boundary.test.ts).input-admission.ts194 → 304,request-prepare.ts970 → 975, both far under the 2,000-line threshold and under the separateRESPONSES_CORE_MODULES2,000-line owner assertion.server-combo-failover-e2e.test.ts4,128 → 4,110 against its frozen 4,166 cap.structure/transports/responses.mdownssrc/server/perstructure/manifest.jsonand is updated; it is already ingrace.oversizeDocs, which is a boolean allowlist with no recorded size, so the addition does not trip the budget check.Hosted CI: this layer is not the lane tip, so its head commit carries
[skip ci]under the maintainer-approved tip-only policy. The gating run is on the tip PR of this stack.Checklist