Skip to content

fix(codex): fall back to caller main during Pool cooldown - #4088

Merged
lidge-jun merged 3 commits into
devfrom
codex/pr3997-caller-main-cooldown
Sep 9, 2026
Merged

fix(codex): fall back to caller main during Pool cooldown#4088
lidge-jun merged 3 commits into
devfrom
codex/pr3997-caller-main-cooldown

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Sep 9, 2026

Copy link
Copy Markdown
Owner

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):

# PR Layer Review focus
2 this PR cooldown caller-main fallback auth-context cooldown branch
1 (PR-A, carried #4025) startup policy binding dependency only

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

  • Head: ddcf8b5, based on the parent branch tip (which is based on dev 8026405). The stacked base is CI-covered because ci.yml's pull_request trigger has no base filter.
  • Remote GitHub Actions on the exact head: runs started at publication; exact-head run URLs are recorded in this section when they conclude.
  • Cumulative integration proof on this branch tip (which contains both layers): ci.yml lane=all run dispatched after PR checks conclude; run URL recorded here.
  • Local suite, typecheck, build, and privacy scan: NOT RUN (local execution restricted in the preparing environment); the remote jobs provide this evidence.
  • Contributor's original evidence (historical, from fix(codex): fall back to caller main during Pool cooldown #3997, attributed): 71 tests / 286 assertions in the authentication-context suite plus 55 tests / 201 assertions of main hard-lock coverage on Windows, and a 26-job contributor CI run on the original head; superseded by the fresh remote runs above.
  • Read-only independent review of the carried diff (cherry-pick fidelity; credential boundary: the fallback cannot clear cooldown, persist caller credentials, bypass the 99% policy, or serve an exact-bound request): dispatched; verdicts are recorded in this section when they arrive.

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.

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 32b1e971f remains 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, 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 remains the fallback only when no comparable id exists on both sides.

reserve-availability exports those claims and its private userId delegates to them on 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. 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).

@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 9, 2026 06:45
@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)
  • ^dev$
  • ^preview$

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 1ba84f01-6e2f-4e5d-86c1-169c69e3ed65

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-09T06:50:16.610917Z ddcf8b5 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

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

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 58 / 80

이 PR은 Pool 모드에서 선택돼 있는 저장 계정이 쿨다운 중인데, 요청에는 이미 검증된 native Codex(메인) 로그인이 붙어 있을 때 생기는 빈틈을 메웁니다. 지금 dev(8026405, #4067 tip)의 resolveCodexAuthContext는 쿨다운 계정이 잡히고 recovery probe lease도 못 받으면 바로 CodexAccountCooldownError를 던집니다. 그런데 같은 파일이 이미 “상류(upstream)가 거절한 뒤 재시도” 경로에서는 검증된 호출자 메인으로 넘어가도록 resolveCallerOwnedMainContext()를 쓰고 있습니다. 그래서 한 번은 메인으로 살아나고, 바로 다음 새 요청은 보내기도 전에 쿨다운 에러로 죽는 앞뒤가 안 맞는 동작이 납니다. 이슈 #3996이 그 재현을 적어 두었고, 이 변경은 그 빈 가지에 기존 호출자-메인 해석기를 그대로 꽂습니다.

생산 코드 차이는 src/codex/auth-context.ts 일곱 줄뿐입니다. probe lease를 못 잡았을 때 requestScopedMainCredential이 있고 exact binding이 없고, 제외 대상이 메인 자체가 아니면 resolveCallerOwnedMainContext()로 돌아갑니다. 조건식은 위쪽 “선택 실패 후 재시도” 가지(대략 790–795행)와 같은 모양이라, 의도적으로 두 경로를 맞춰 둔 것입니다. 쿨다운을 지우지 않고, 호출자 자격 증명을 Pool 선택으로 저장하지 않으며, exact account binding은 그대로 막힙니다. #3973(수동 크레딧 리셋 후 쿨다운 정리)과도 겹치지 않습니다. 영문/한글 codex-integration 가이드에 그 동작을 짧게 적어 두었고, 테스트는 새 요청이 메인을 쓰는지와 main 99% 정책 경계(98.99 vs 99)를 같이 잡습니다.

베이스는 dev가 아니라 부모 브랜치 codex/pr4025-main-policy-binding입니다. 같은 내용의 포크 PR이 #3997로 열려 있고(intake 게이트), 부모 쪽 유지보수 스폰서 캐리가 #4085입니다. 이 PR은 그 위에 올린 스택 2층입니다. 부모 레이어가 startup policy binding이 끝날 때까지 resolveCallerOwnedMainContext()를 막아 두기 때문에, 이 쿨다운 fallback도 그 동안에는 기존 503 maintenance 응답으로 맞춰집니다. 인증/자격 증명 선택 경로라 MAINTAINERS.md상 보안 리뷰가 필요합니다. CI는 방금 올라가서 핵심 suite가 아직 pending이고, stacked base라 enforce-target은 실패한 상태입니다(부모 머지 후 dev로 retarget하면 자연히 풀리는 종류).

라인 900 - fixedAccountId === undefined 검사는 바로 위 exact-binding 쿨다운 가지에서 이미 막힌 뒤라 사실상 중복이다. 가독성용으로 남긴 건지, 위 790행과 글자 그대로 맞추려는 건지 한 줄만 확인하면 됩니다.
라인 900-902 - 조건이 선택-실패 fallback과 동일해 보이지만, 쿨다운 스냅샷/quotaScope probe 경로에서 requestScopedMainCredential이 false인 일반 Pool 요청은 여전히 쿨다운 에러를 냅니다. 의도한 범위가 “검증된 native 호출자만”인지 리뷰에서 한 번 더 짚을 만합니다.
경로 src/codex/auth-context.ts - 자격 증명 선택 변경이라 MAINTAINERS.md 보안 리뷰 체크가 머지 전 필수입니다. 쿨다운 clear / persist / 99% bypass / exact-bind bypass가 안 되는지는 본문 verification에 적혀 있으나, 독립 리뷰 결과가 아직 본문에 채워지지 않았습니다.
경로 #3997 / #4085 - 동일 주제의 포크 PR(#3997)과 부모 캐리(#4085)가 같이 열려 있습니다. 이 PR만 dev에 넣으면 안 되고, 머지 순서와 leftover 정리가 필요합니다.
경로 CI enforce-target - stacked base(codex/pr4025-main-policy-binding)를 향한 정상적인 실패로 보이며, 부모 착지 전 “빨강 = 제품 버그”로 읽으면 안 됩니다.

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

너의 추천
부모 #4085가 dev에 들어간 뒤 이 PR을 dev로 돌리고, 핵심 CI 초록 + 보안 리뷰 사인오프를 받은 다음 머지하세요. #3996은 본문이 이미 Closes로 연결합니다. 착지 후 #3997에는 Landed via #<landing> at <commit> 댓글, landed-via-maintainer 라벨, completed/superseded 클로즈를 이어서 처리하세요. 지금은 스택·보안·CI가 남아 있으니 바로 dev 단독 머지는 하지 마세요.

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/codex/auth-context.ts
@lidge-jun

Copy link
Copy Markdown
Owner Author

Review-finding disposition for this carried PR (payload identical to the original #3997 head 094e509):

  • P2 — caller identity may coincide with the cooled Pool account: a real design question for the original change, not a carry defect: when the native keyring login is also registered as a Pool account, the caller-main fallback can serve the same subscription that is cooling down. The fallback still enforces caller validation, model entitlement, and the main 99% policy, and it never touches Pool state; but it does not compare the caller identity against the cooled account. Deferred to the integration decision: either the original author adds the identity comparison with tests, or a maintainer follow-up does. Not silently expanded inside this carry.

@lidge-jun
lidge-jun force-pushed the codex/pr4025-main-policy-binding branch from ca36b53 to a79193f Compare September 9, 2026 08:14
@lidge-jun
lidge-jun force-pushed the codex/pr3997-caller-main-cooldown branch from ddcf8b5 to 65ba8ea Compare September 9, 2026 09:12
luvs01 and others added 2 commits September 9, 2026 19:05
Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
Carried-from: #3997 (094e509)
…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).
@lidge-jun
lidge-jun force-pushed the codex/pr3997-caller-main-cooldown branch from 65ba8ea to c3106e1 Compare September 9, 2026 10:22
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).
@luvs01

luvs01 commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

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 Ingwannu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Base automatically changed from codex/pr4025-main-policy-binding to dev September 9, 2026 10:56
@lidge-jun

Copy link
Copy Markdown
Owner Author

Maintainer integration into dev at exact head 00b96bba314c65d850835d78da200083782db13a. Current maintain/admin authority and outstanding maintainer review state were verified with the repository helper. Exact-head Cross-platform CI: https://github.com/lidge-jun/opencodex/actions/runs/34340681852 (success; conditional skipped jobs are not counted as passing tests). The owner explicitly requested immediate serial integration of the prepared PR set. Cumulative lane=all has not yet run; full validation will run on final dev after landing. Independent technical/security review and source carry verification were completed for this delivery. Local product tests/typecheck/build/install were NOT RUN under the owner restriction. Merge uses the original commits and preserves attribution.

@lidge-jun
lidge-jun merged commit 4a08c75 into dev Sep 9, 2026
35 checks passed
@lidge-jun
lidge-jun deleted the codex/pr3997-caller-main-cooldown branch September 9, 2026 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants