fix(codex): distinguish model availability from auth failure - #4480
Conversation
Carry #4381 from aad1d75 onto the current dev tip. When an adapter ends a turn with a recognized truncated stop reason, the streaming path already keeps an open function, custom, or tool-search call incomplete, and the error and explicit-incomplete terminals already close an in-flight provider web search as failed. The plain truncated `done` path did not: it closed the search as `completed`, so a client showed a finished search for a turn the provider cut short before any result arrived. That search takes the same failed status now. The classifier and bridge terminal tests gain the exact `max_output_tokens` stop-reason value, which the vocabulary and mapping cases previously omitted even though `src/responses/truncated-stop-reason.ts` classifies it. Without that case, dropping it from the classifier would silently let the bridge emit `response.completed` with an incomplete call or search attached. This is the remainder of #4312's tool-finalization scope after #4341 landed the open function, custom, and tool-search call handling upstream; nothing from that PR is duplicated here. Verification on this carry: bun test tests/adapters/bridge-nonstreaming-terminal.test.ts, bun run typecheck, bun run structure:check, bun run privacy:scan. Local full suite: NOT RUN. Hosted CI on the lane tip is the suite proof. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
Carry #4388 from 9518b52 onto the lane B chain above the #4381 carry. A hidden image/video iteration collected adapter events into an unbounded array. Both the runTurn emit path and ordinary stream collection kept every non-heartbeat event until the scanner ran, so an oversized tool argument or a long text stream grew one iteration without limit even though src/lib/translator-budget.ts already defines the turn and call-argument ceilings. Each iteration now owns an independent budget: 32 MiB of serialized retained events including array framing, and 2 MiB of UTF-8 arguments per open call, with surrogate pairs that straddle two deltas counted once. Overflow aborts the producer and surfaces translation_buffer_limit instead of a truncated success. Folded in the Codex review finding left on the source head, which was not addressed there. createAdapterEventQueue merges adjacent text and thinking deltas into chunks of up to 64 KiB while no reader is waiting, so a synchronous producer's token-granular deltas survive as a handful of strings. Charging each pre-merge envelope billed about 31 bytes for every one-character delta, so roughly 1 MiB of retained output crossed the 32 MiB turn limit and aborted a healthy turn. push now reports whether it merged the event into its buffered tail, and a merged delta is charged only for the payload it appended. The return value is additive: every other caller passes push as a void callback and is unaffected. That accounting is deliberately conservative in one place. JSON escaping is per character, so a merged delta costs its quoted form minus the quotes; a surrogate pair split across two deltas over-counts by eight bytes and never under-counts. Verification on this carry: bun test tests/images/loop.test.ts tests/adapters/run-turn-queue.test.ts (74 pass, 0 fail), bun run typecheck, bun run structure:check, bun run privacy:scan. The new coalescing regression was driven red against the unfixed emit path first, where it reproduced the spurious translation_buffer_limit response. Local full suite: NOT RUN. Hosted CI on the lane tip is the suite proof. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
Carry #4460 from eefa074 onto the lane B chain above the #4388 carry. Lane tip. Account-gated native model selection threw CodexPoolAuthenticationError whenever no configured account carried the entitlement, so Responses, Images, Live and Search all reported HTTP 401 authentication_error with code invalid_api_key. A healthy credential that simply lacks the model looked broken, and users re-logged in and rotated keys for a condition that needed a different model or a reopened quota window. CodexModelAvailabilityError now carries the reason. An entitlement no account holds is 400 invalid_request_error; a model whose capable accounts are all temporarily exhausted is 429 rate_limit_error with code rate_limit_exceeded and no Retry-After, since the reopening time is not known here. Real credential failures keep their 401. Two carry-specific changes on top of the source head. The source branch shipped devlog/_fin/260913_model_availability_errors. A _fin unit records work already visible in public git history, and this work has not landed, so it moves to devlog/_plan and gains the catch-order audit below. The maintainer review asked for one more pass over catches outside Responses, because CodexModelAvailabilityError extends CodexPoolAuthenticationError and a parent-first branch would fold 400 and 429 back into 401. The shared mapper and the Images, Live and Search surfaces all test the subclass first. context-history folds the parent straight to 401 and is left unchanged: it resolves with modelId "context_history", and every throw site is gated on ACCOUNT_GATED_NATIVE_OPENAI_MODELS membership, directly or through modelEligibleAccountIds, which is only populated for a gated model. A new assertion pins that membership so a future gated "context_history" cannot silently restore the invalid_api_key report. encrypted-payload and collaboration import the parent but never branch on it. The new test file is registered in scripts/test-layout/layout.json and tests/fixtures/test-layout-expected.json; the source head omitted both, which fails tests/test-layout-tooling.test.ts. This touches src/codex/auth-context.ts, so pr-hygiene reports unsponsored_surface and MAINTAINERS.md requires security review before it can leave draft. That is expected and unresolved here. Verification on this carry: bun test over tests/codex-integration/codex-model-availability-error.test.ts, tests/codex-integration/codex-auth-context.test.ts, tests/images/loop.test.ts, tests/adapters/run-turn-queue.test.ts and tests/adapters/bridge-nonstreaming-terminal.test.ts (205 pass, 0 fail), plus tests/test-layout.test.ts and tests/test-layout-tooling.test.ts (17 pass), bun run typecheck, bun run structure:check, bun run privacy:scan. Local full suite: NOT RUN. Hosted CI on this tip is the lane's suite proof. Co-authored-by: Shawn <77868633+AgenticLab-SH@users.noreply.github.com>
github-actions
Bot
added
the
intake: hygiene-blocked
|
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. |
✅ READY
Hygiene✅ Deterministic PR hygiene checks passed. |
|
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 |
리뷰 · 우선순위 75 / 80설명
우선순위 75는 오진 401이 지원 부담·장애 대응을 크게 만들고, 매핑이 명확하며 tip 캐리로 바로 넣을 수 있기 때문이다. types/config 분할과 무관하다.
심볼 원본 #4460 / context-history 예외 - 메인테이너가 같은 매핑을 history 표면에도 펼칠지는 후속 결정(본문도 그렇게 적음). 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
github-actions
Bot
removed
the
intake: hygiene-blocked
![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: 2fb74c151b
ℹ️ 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 unsupported = entitledAccountIds?.size === 0 && !mainModelGrantUnobserved; | ||
| throw new CodexModelAvailabilityError( | ||
| unsupported ? "unsupported" : "temporarily_unavailable", |
There was a problem hiding this comment.
When every model-roster lookup is unconfirmed—for example, because /models timed out—or credential snapshots return null, entitledAccountIds is empty even though no account has actually denied the model. This condition therefore labels the result unsupported, causing the new mapper to return a non-retryable 400; in the missing/failed-credential case it also violates the intended unchanged 401 authentication behavior. Classify unsupported only when the relevant account rosters are confirmed denials, and allow unknown or credential-failure states to retain their transient/authentication response.
Useful? React with 👍 / 👎.
This branch reports an unsupported account-gated model as 400 invalid_request_error instead of 401 invalid_api_key, and one local API auth case still pinned the old status. It was the only assertion that failed on the hosted suite: ubuntu shard 1/4 and macos 1/2 both failed on it and on nothing else. What the case exists to prove is that the request fails before upstream dispatch, which the empty dispatch list still shows. The status was incidental to that intent and the message assertion is unchanged. Verification: bun test tests/server/server-auth.test.ts (112 pass, 0 fail), bun run typecheck. Co-authored-by: Shawn <77868633+AgenticLab-SH@users.noreply.github.com>
1 participant
Summary
Carries #4460 by @AgenticLab-SH (source head
eefa07442, fork branchcodex/260913-model-capacity-errors-dev) as the tip of lane B in the contributor carry train.Account-gated native model selection threw
CodexPoolAuthenticationErrorwhenever no configured account carried the entitlement, so Responses, Images, Live and Search all answered HTTP 401authentication_errorwith codeinvalid_api_key. A perfectly healthy credential that simply lacks the model looked broken, and the natural user response — log in again, rotate the key — fixes nothing. The condition needs a different model or a reopened quota window.CodexModelAvailabilityErrornow carries the reason:invalid_api_keyinvalid_request_errorinvalid_api_keyrate_limit_error/rate_limit_exceededinvalid_api_keyThe 429 carries no
Retry-After, because the reopening time is not known at this layer; that is asserted rather than left implicit.Carry-specific changes
The
_findocument moved to_plan. The source branch shippeddevlog/_fin/260913_model_availability_errors/. A_finunit records work already visible in public git history and this work has not landed, so it now sits indevlog/_plan/and carries the audit below.The catch-order audit the maintainer review asked for.
CodexModelAvailabilityErrorextendsCodexPoolAuthenticationError, so anycatchtesting the parent first folds the new 400 and 429 straight back into 401. Results:images.ts,live.tsandsearch.tsall test the subclass before the parent. That order is the contract.src/server/context-history.tsfolds the parent to 401 and is left unchanged. It resolves withmodelId: "context_history", and everyCodexModelAvailabilityErrorthrow site is gated onACCOUNT_GATED_NATIVE_OPENAI_MODELSmembership — directly, or throughmodelEligibleAccountIds, which is only populated for a gated model. The subclass cannot reach that catch today, and a new assertion pins that membership so a future gated"context_history"cannot silently restore theinvalid_api_keyreport.encrypted-payload.tsandcollaboration.tsimport the parent but never branch on it.Whether context-history should adopt the same mapping outright is left as the maintainer decision it was raised as, not resolved inside a carry.
Test-layout registration.
tests/codex-integration/codex-model-availability-error.test.tsis new, and the source head registered it in neitherscripts/test-layout/layout.jsonnortests/fixtures/test-layout-expected.json. Both entries are added; without themtests/test-layout-tooling.test.tsfails, which I reproduced before fixing.Sponsorship
This touches
src/codex/auth-context.ts, sopr-hygienereportsunsponsored_surfaceandMAINTAINERS.mdrequires security review before the PR can leave draft. That is expected here and is not resolved by this carry.Lane
This is the tip of lane B. Its base is
codex/260913-carry-4388-media-bounds, which in turn sits oncodex/260913-carry-4381-truncated-search. The CI run on this tip is the suite proof for all three links; the two links below hold[skip ci]on their head commits deliberately.The branch is cumulative, so it contains both links below it. Both authors are credited on the tip for that reason.
Verification
bun testovertests/codex-integration/codex-model-availability-error.test.ts,tests/codex-integration/codex-auth-context.test.ts,tests/images/loop.test.ts,tests/adapters/run-turn-queue.test.ts,tests/adapters/bridge-nonstreaming-terminal.test.ts— 205 pass, 0 fail, 2879 assertions.bun test tests/test-layout.test.ts tests/test-layout-tooling.test.ts— 17 pass, 0 fail (reproduced red first).bun run typecheck— passed.bun run structure:check— passed.bun run privacy:scan— passed.Checklist
Co-authored-by: Shawn 77868633+AgenticLab-SH@users.noreply.github.com
Co-authored-by: luvs01 27862058+luvs01@users.noreply.github.com