Skip to content

fix(codex): avoid reauthentication advice for rate-limited warmup - #4144

Draft
remorser58 wants to merge 1 commit into
lidge-jun:devfrom
remorser58:fix/codex-warmup-rate-limit-message
Draft

fix(codex): avoid reauthentication advice for rate-limited warmup#4144
remorser58 wants to merge 1 commit into
lidge-jun:devfrom
remorser58:fix/codex-warmup-rate-limit-message

Conversation

@remorser58

Copy link
Copy Markdown

Summary

HTTP 429 during Codex account warmup currently tells users to reauthenticate even though signing in again cannot reset a provider's rate or usage limits. Report this case as rate-limited, with guidance to retry later or after the account's usage limit resets.

  • Keep HTTP 429 on the warmup failure response, including when its bounded error-body read times out, and expose codex_warmup_rate_limited in OAuth login status.
  • Keep HTTP 401/403 on the existing authentication failure path. Preserve the failed-warmup gate, existing credentials during failed reauthentication, and secret-safe error reporting. No inference retry is added.
  • Add regression coverage for the public OAuth completion path and document the retry guidance.

Related to #3846 and #3848. This is an independent, narrowly scoped error-reporting fix; it preserves the current account-registration policy. The larger deferred-registration behavior in #3848 remains separate.

Verification

Verified on macOS using synthetic OAuth identities and upstream responses, with isolated test homes. No real account credentials or provider inference calls were used for this patch.

  • Failing-first OAuth regressions: 4 failed, then 4 passed. A separately added stalled-body 429 regression also failed before its one-line fix and passed afterwards.
  • bun run test -- tests/codex-integration/codex-auth-api.test.ts tests/codex-integration/codex-warmup.test.ts tests/codex-integration/warmup.test.ts tests/codex-integration/token-guardian.test.ts tests/codex-integration/codex-quota-auto-refresh.test.ts: 355 passed, 0 failed.
  • bun run typecheck: passed.
  • bun run privacy:scan: passed.
  • cd docs-site && bun run build: passed, 425 pages.
  • git diff --check: passed.
  • Real ephemeral localhost HTTP exercise of OAuth start/status: 4 passed, covering 429, 401, 403, and failed reauthentication with synthetic upstream responses.

The regressions cover bounded stalled-body cancellation, preserved HTTP 400 timeout behavior without fallback, rate-limit versus authentication classification, failure-code propagation, one warmup attempt, no raw upstream error-body disclosure, no new account persistence, and unchanged credentials after failed reauthentication.

The full repository suite, Windows/Linux execution, and live-account validation were not run. This PR is a draft under the contributor policy. Authentication-area maintainer sponsorship and the required upstream review remain necessary before merge.

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.

Review readiness checklist

  • All CI tests are green on my local testing.
  • I pushed my PR to the latest dev commit.
  • I resolved all correct Codex and CodeRabbit findings.
  • My PR is ready for review.

Classify HTTP 429 warmup failures separately and carry the machine-readable
failure code into OAuth login status so clients can distinguish rate limits.
Preserve a received 429 even when bounded error-body cleanup times out.

Constraint: Keep failed warmup from persisting or validating an account.
Rejected: Change quota-exhausted registration policy | Separate work in PR lidge-jun#3848.
Confidence: high
Scope-risk: narrow
Directive: Never expose raw upstream warmup error bodies.
Tested: 355 focused tests; TypeScript typecheck; privacy scan; 425-page docs build.
Not-tested: Full repository suite and Windows/Linux execution.
@github-actions github-actions Bot added the intake: hygiene-blocked Deterministic PR hygiene checks failed label Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

⚠️ Deterministic hygiene checks failed.

  • unsponsored_surface — This changes an authentication, workflow, release-automation, or dependency surface. MAINTAINERS.md requires security review for these; ask a maintainer to apply maintainer-sponsored once they have reviewed it. Paths: src/codex/auth-api.ts.

@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

⏳ DRAFT

  • hygiene: unsponsored_surface.

What to do

  • Fix unsponsored_surface — This changes an authentication, workflow, release-automation, or dependency surface. MAINTAINERS.md requires security review for these; ask a maintainer to apply maintainer-sponsored once they have reviewed it. Paths: src/codex/auth-api.ts.
  • Tick all four boxes in the PR description once you're done (currently 1/4).

Review readiness checklist

  • ⬜ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ⬜ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

1/4 boxes ticked.

This pull request was already a draft. Its draft status will be preserved after every issue above is resolved.
@remorser58 Tick the boxes once your local CI is green, your branch is on the latest dev commit, and every correct Codex and CodeRabbit finding is resolved.

@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 46 / 80

이 PR은 Codex 계정 warmup이 HTTP 429를 받았을 때, 지금도 “다시 로그인(Reauthenticate)하세요”로 안내하는 오답을 고치려는 좁은 패치입니다. 관련 이슈/PR은 #3846(등록이 warmup 성공에 묶임), #3848(쿼터 소진 시 등록 유예, 여전히 draft)이고, 작성자도 등록 정책 자체는 건드리지 않고 오류 분류·문구·상태 코드만 분리한다고 명시했습니다. base는 dev, 지금은 draft이며 intake: hygiene-blocked이고 CI에서 hygiene·enforce-target이 실패했습니다. 실패 이유는 unsponsored_surfacesrc/codex/auth-api.ts 인증 표면 변경이라 maintainer-sponsored가 필요합니다.

현재 dev HEAD의 verifyCodexAccountWarmup(src/codex/auth-api.ts)는 warmup이 무엇이든 실패하면 같은 JSON으로 codex_warmup_failed + “Reauthenticate…” + HTTP 401을 돌립니다. CodexWarmupErrorstatussrc/codex/warmup.ts에 이미 실려 있는데, 429도 401 재인증 경로로 뭉개집니다. 이 PR은 (1) err instanceof CodexWarmupError && err.status === 429일 때 codex_warmup_rate_limited와 재시도 안내·HTTP 429로 분기하고, (2) OAuth completion이 그 code를 login state에 실어 주며, (3) drainErrorBody에서 abort여도 이미 받은 429 status는 transport로 덮지 않게 하고, (4) 401/403은 기존 재인증 문구를 유지하며, (5) 계정 미지속·재인증 시 기존 credential 보존·업스트림 body 비노출을 테스트로 고정합니다. docs-site codex-integration.md에도 429는 재로그인이 한도가 아니라 재시도라는 한 문단을 추가합니다. CodexLoginStateRow.code 필드는 HEAD에 이미 있어서 스키마 확장이 아니라 값 전파에 가깝습니다.

방향은 맞고 범위도 작습니다. 사용자가 쿼터/레이트리밋인데 재로그인을 반복하게 만드는 UX는 실제 운영에서 자주 나오는 함정이고, #3848의 “등록 자체를 warmup 성공 전에도 허용”과는 축이 다릅니다. 다만 auth 표면이라 스폰서 없이 머지하면 안 되고, draft + hygiene-blocked 상태에서는 리뷰 큐에 올리면 안 됩니다. 테스트는 auth-api/warmup 쪽에 회귀를 잘 심었고, 작성자 로컬 355 focused green·typecheck·privacy·docs build를 적었습니다. 전체 스위트·Windows/Linux·실계정은 미실행이라 스폰서 전 최소 Cross-platform CI green은 필요합니다.

src/codex/auth-api.ts verifyCodexAccountWarmup - 429만 codex_warmup_rate_limited/HTTP 429로 분기하는 축은 타당. 401/403 문구 유지 확인됨.
src/codex/warmup.ts drainErrorBody - signal.aborted여도 res.status===429면 http_status 보존. stalled body 회귀 테스트가 이 한 줄을 지탱함.
OAuth login state code 전파 - CodexLoginStateRow.code는 HEAD에 이미 있음. GUI가 code를 아직 안 보여줘도 API/상태 소비자에게는 이득.
관련 #3848 - 등록 유예 정책과 독립. 이 PR에 유예 로직을 섞지 말 것(작성자 의도 유지).
CI - hygiene unsponsored_surface + enforce-target fail. maintainer-sponsored 전까지 ready 아님.

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

  • auth-api 표면 변경에 maintainer-sponsored를 줄지(보안 리뷰 범위)
  • 429를 login UI에 별도 카피로 노출할지, 일단 API code만으로 충분한지
  • #3848과 같은 열차에 태울지, 이 좁은 분류 수정만 먼저 넣을지

너의 추천
정책·방향은 유지한 채 draft로 두고, 메인테이너 스폰서 + hygiene/CI green 후에만 ready로 올리세요. #3848과 합치지 마세요. 머지 후보가 되면 warmup 429/401/403·reauth credential 보존 테스트만 스폰서가 한 번 더 보면 됩니다.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working intake: hygiene-blocked Deterministic PR hygiene checks failed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants