fix(codex): fall back to caller main during Pool cooldown - #4088
Conversation
|
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. |
리뷰 · 우선순위 58 / 80이 PR은 Pool 모드에서 선택돼 있는 저장 계정이 쿨다운 중인데, 요청에는 이미 검증된 native Codex(메인) 로그인이 붙어 있을 때 생기는 빈틈을 메웁니다. 지금 생산 코드 차이는 베이스는 라인 900 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddcf8b5f9b
ℹ️ 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".
|
Review-finding disposition for this carried PR (payload identical to the original #3997 head 094e509):
|
ca36b53 to
a79193f
Compare
ddcf8b5 to
65ba8ea
Compare
…allback The early-cooldown fallback could serve the very subscription that is cooling down when the caller's keyring login is also registered as a Pool account. Guard it with a fail-closed identity ladder: a distinct workspace account id always passes; the exact materialized bearer+account tuple or a matching (account id, email) pair for a rotated token denies; a different email on a shared workspace account id is a distinct team member and passes; an unreadable identity fails closed. No physical-main read, no caller persistence, no cooldown or probe-lease changes. Same-email personal/business coexistence over-denies during the cooldown — the safe direction. Review-finding correction on the carried #3997 (maintainer commit).
65ba8ea to
c3106e1
Compare
The cooldown guard compared the workspace account id, then the exact bearer material, then the configured email. Email is the wrong last word on identity in both directions: a caller whose token carries no email claim was refused even when its user id proved it was a different member of the same workspace, and the cooled subscription's own rotated token was admitted whenever the recorded email had since changed, which let it skip its own cooldown. Compare the native ChatGPT user identity the reserve path already trusts (chatgpt_user_id then user_id under the OpenAI auth namespace, never sub) whenever both the caller bearer and the cooled account's stored credential carry one: the same id denies across email changes and token rotation, a different id allows, and a credential whose own two encodings disagree identifies nobody and fails closed. Email stays the fallback when no comparable id exists on both sides. reserve-availability now exports those claims; its private userId keeps the raw precedence, so an empty or non-string chatgpt_user_id still blocks the user_id fallback exactly as before and the reserve path is unchanged. Review-finding correction on the carried #3997 (maintainer commit).
c3106e1 to
00b96bb
Compare
|
I have switched follow-up tracking to this maintainer stack and added hold notices to #4025 and #3997. Their source branches remain at 6c1387d and 094e509, respectively, so there will be no competing pushes there. The original change payloads are preserved in the carried commits: all nine #4025 paths and all five #3997 paths retain the same additions/deletions in order, excluding rebase context and line numbers. The original author attribution is retained. This verifies the source transfer; it does not claim the later maintainer corrections are identical to the originals or have been independently re-reviewed here. I am tracking #4085 at 87efd10 with its successful current-head CI, and this child at 00b96bb with its current run queued. The new identity/read-boundary corrections remain owned by this stack's review and integration flow. After actual dev integration is verified, I will close #4025 for the startup-binding layer and #3997/#3996 for the cooldown fallback layer as appropriate. Until then the originals remain held and open. |
Ingwannu
left a comment
There was a problem hiding this comment.
Reviewed 00b96bb specifically against parent 87efd10. Exact-head repository CI 34340681852 has now completed successfully.
The new cooldown branch goes through resolveCallerOwnedMainContext, preserving model-entitlement and main-policy checks. It requires an independently usable caller, does not clear the selected Pool cooldown or persist the caller as the selected account, and excludes exact account bindings. I checked both sides of the shared-workspace case: a different stable user can be distinguished, while the same stable user after token/email rotation stays blocked; conflicting user claims and otherwise ambiguous identities fail closed. The reserve helper extraction keeps its original raw-claim precedence. Tests also retain the 98.99%/99% main-policy boundary with physical-main reads forbidden.
No new code defect found. This is a reviewed child delta, not permission to land ahead of #4085: keep the parent's ownership/Windows verification hold, then retarget/revalidate the composed head on dev. The author's freeze on #3997/#4025 is the right coordination path; no duplicate rewrite or premature source-issue closure is needed.
|
Maintainer integration into dev at exact head |
Summary
Closes #3996.
Carried from #3997 (author: @luvs01) to give the change a maintainer-sponsored same-repo head; the original fork PR is intake-blocked on
unsponsored_surface, an intake-hygiene gate rather than a product failure. Source commit: 094e509. This diff is the original change rebased onto the parent layer with no semantic edits.A stored Pool account's cooldown can reject a fresh native request before it considers that request's eligible main credential. The existing post-upstream-rejection path can use the same credential, leaving successive requests with inconsistent behavior.
Use the existing caller-owned-main resolver when the selected stored account is cooling down and no recovery probe is available. The production change is seven lines and preserves caller/model validation, main quota policy, exact account bindings, Pool selection and cooldown state. English/Korean integration guidance describes the behavior.
This is independent of manual reset-credit reconciliation (#3973): it spends no credit and clears no cooldown. It also preserves the existing post-upstream retry path rather than adding another upstream retry loop.
In this chained tree the fallback enters
resolveCallerOwnedMainContext(), which the parent layer fences while the startup policy binding is pending — so the cooldown fallback also returns the existing 503 maintenance response during that initialization, matching the parent layer's contract.Stack (merge bottom-up):
Depends on the parent PR. Review this PR's diff only. After the parent lands, this PR is retargeted to
dev.Security review: this change touches src/codex/auth-context.ts (credential selection) and requires explicit security review per MAINTAINERS.md before merge.
Co-authored-by: luvs01 27862058+luvs01@users.noreply.github.com
Verification
dev8026405). The stacked base is CI-covered because ci.yml's pull_request trigger has no base filter.ci.ymllane=all run dispatched after PR checks conclude; run URL recorded here.Checklist
Carry + review correction (2026-09-09 update)
This branch was cascaded onto the corrected parent (PR #4085 head 11f575e) and gained one maintainer corrective commit answering the review finding on the carried change:
65ba8ea24— the early-cooldown caller-main fallback no longer serves the subscription that is cooling down when the caller's keyring login is also registered as a Pool account. The guard is a fail-closed identity ladder: distinct workspace account id passes; exact materialized bearer+account tuple or matching (account id, email) pair (rotated token) denies; a different email on a shared workspace account id is a distinct team member and passes; an unreadable identity fails closed. No physical-main read, no caller persistence, no cooldown or probe-lease changes. Known residual (fail-safe): coexisting personal/business registrations with the same email and account id are denied while cooled.The carried commit
32b1e971fremains a payload-exact cherry-pick of the original #3997 head 094e509 (author luvs01 preserved). Remote CI at the new exact head 65ba8ea: 25 checks pass, 2 dispatch-only skips, 0 fail (run https://github.com/lidge-jun/opencodex/actions/runs/34333454278), CodeRabbit included. The earlier head ddcf8b5 was fully green (25 pass / 0 fail, run https://github.com/lidge-jun/opencodex/actions/runs/34320502156) and cumulative lane=all on it succeeded (run https://github.com/lidge-jun/opencodex/actions/runs/34321628628); both predate the corrective commits.Identity correction (head 00b96bb)
The first corrective commit kept the cooled subscription out of its own fallback but still decided identity by the configured email once the workspace account id matched. That was wrong in both directions: a caller whose token carried no email claim was refused even when its user id proved it was a different member of the same workspace, and the cooled subscription's own rotated token was admitted whenever the recorded email had changed, which let it skip its own cooldown.
The guard now compares the native ChatGPT user identity the reserve path already trusts —
chatgpt_user_id, thenuser_id, under the OpenAI auth namespace, neversub— whenever both the caller bearer and the cooled account's stored credential carry one. The same id denies across email changes and token rotation, a different id allows, and a credential whose own two encodings disagree identifies nobody and fails closed. Email remains the fallback only when no comparable id exists on both sides.reserve-availabilityexports those claims and its privateuserIddelegates to them on the raw precedence, so an empty or non-stringchatgpt_user_idstill blocks theuser_idfallback exactly as before and the reserve path is unchanged. Everything else the earlier guard preserved is preserved: opaque bearers fail closed, the exact materialized bearer plus account tuple denies, a distinct workspace account id allows, main-in-rotation uses the observed-main equality tag, and exact bindings, the probe-lease arm, Direct and main-pin never reach this rung. No physical main credential is read and no Pool state is written.Three resolver regressions cover the contract, including one chosen so it fails against the pre-fix guard rather than passing either way, plus claim-level unit tests that pin the raw precedence and the conflict flag.
Remote CI at 00b96bb: 25 checks pass, 2 dispatch-only skips, 0 fail (run 34340681852). Local suite, typecheck and build: NOT RUN (local execution restricted in the preparing environment).