Conversation
PoolQuotaResult now carries the failure source observed while obtaining WHAM/quota or refreshing tokens, so poolAccountDto reports the actual cause instead of inferring it from overlapping booleans. Terminal WHAM 401s surface as quota_unauthorized and terminal refresh failures as refresh_failed.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
github-actions
Bot
added
the
intake: hygiene-blocked
|
⏳ DRAFT
What to do
Review readiness checklist
✅ 4/4 boxes ticked. This pull request was already a draft. Its draft status will be preserved after every issue above is resolved. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change records reauthentication failure sources in pool quota results, passes normalized reasons through account health projection, and exposes the resulting reason in account DTOs. Integration tests cover quota authorization and OAuth refresh failures. ChangesReauthentication reason propagation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Client
participant AccountsAPI
participant poolAccountDto
participant PoolQuotaProbe
participant OAuthTokenEndpoint
Client->>AccountsAPI: request account data
AccountsAPI->>poolAccountDto: build account DTO
poolAccountDto->>PoolQuotaProbe: obtain quota result
PoolQuotaProbe->>OAuthTokenEndpoint: refresh token when required
OAuthTokenEndpoint-->>PoolQuotaProbe: return refresh result
PoolQuotaProbe-->>poolAccountDto: return reauthReason
poolAccountDto-->>AccountsAPI: return account DTO reason
AccountsAPI-->>Client: return reauthReason
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
리뷰 · 우선순위 46 / 80이 PR은 풀 계정이 다시 로그인이 필요할 때, 그 이유를 응답에 적습니다. 예전 목록은 원인 세 개를 참/거짓 하나로 합친 뒤, 겹친 조건으로 이유를 추측했습니다. 사용량 조회가 401로 끝나도, 갱신 토큰이 거절돼도, 건강 상태는 늘 갱신 실패였습니다. 목록을 보는 사람은 다시 로그인해야 하는지, 사용 권한만 없는지 알 수 없었습니다. 이제는 조회가 실패한 자리에 이유를 남깁니다. 사용량 주소가 끝까지 401이면 src/codex/auth-api/pool-quota-probe.ts:404 - src/oauth/health.ts:216 - 저장된 검증이 401이나 403으로 실패하면 재인증이 필요하다고 올립니다. 243줄은 호출자가 이유를 안 주면 무조건 src/codex/auth-api/account-list.ts:85 - 타입 주석은 tests/codex-integration/codex-auth-api.test.ts:2093 - 두 테스트는 계정 JSON의 메인테이너의 판단이 필요한 지점
저장된 401/403 검증 실패를 이 PR에서 너의 추천 404줄은 이 댓글은 grok-bot이 작성했습니다 |
PoolQuotaResult now carries the failure source observed while obtaining WHAM/quota or refreshing tokens, so poolAccountDto reports the actual cause instead of inferring it from overlapping booleans. Terminal WHAM 401s surface as quota_unauthorized and terminal refresh failures as refresh_failed. Carried from #5191. The two regression cases are registered from tests/helpers/pool-reauth-cause.ts rather than written inline: the original placement grew tests/codex-integration/codex-auth-api.test.ts from 6522 to 6560 lines against a 6549-line file-size-baseline cap that only ever moves downward. Registering them through the same registerXCases seam the file already uses for two other helpers leaves it at 6525 with the assertions byte-identical. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
…e stored auth causes
|
Addressed the review findings in 8ba026a (pushed to fix/pool-reauth-failure-cause):
Verified: |
…ice (#5199) * fix(codex): preserve pool reauthentication failure causes PoolQuotaResult now carries the failure source observed while obtaining WHAM/quota or refreshing tokens, so poolAccountDto reports the actual cause instead of inferring it from overlapping booleans. Terminal WHAM 401s surface as quota_unauthorized and terminal refresh failures as refresh_failed. Carried from #5191. The two regression cases are registered from tests/helpers/pool-reauth-cause.ts rather than written inline: the original placement grew tests/codex-integration/codex-auth-api.test.ts from 6522 to 6560 lines against a 6549-line file-size-baseline cap that only ever moves downward. Registering them through the same registerXCases seam the file already uses for two other helpers leaves it at 6525 with the assertions byte-identical. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(server): stop a local catalog-sync warning from un-readying the proxy /readyz went to a terminal failed state on any nonempty warning from the post-startup Codex sync. A warning there describes artifacts written into the LOCAL Codex home, not the proxy's ability to serve: no catalog source so Codex keeps its native catalog, combos omitted from the catalog, a conversation-history relabel left to Codex's own writer, or a caught catalog-refresh exception after which config injection still runs and still reports its own ok. The sync continues past every one of them. #5181 is what that cost. A single-replica Kubernetes deployment lost its only Service endpoint while every non-Codex route stayed healthy, because the readiness probe read a degradation of a local file as a dead process. Switching the probe to /healthz restored service, which is the reporter's own evidence that the process was viable. ok and warning are separate fields in the sync result because they answer separate questions. ok is the sync's verdict on the essential work - the write admission and the config injection. The gate now follows ok alone. The narrowing is only to warning. A throw, a null outcome, and ok !== true stay terminal, so the service-home write refusal, the external-provider injection failure, the config/integrity preflight refusal and the final injection failure all still fail readiness. A throw and a null stay terminal for a different reason: neither is a classified outcome, so the gate cannot claim one. Readiness also still stays pending until the sync SETTLES, which is the race the gate was built for and which this does not touch. This is the boundary the Claude Code roster reconciliation already has - it delays the ready transition without being allowed to fail it - applied to the catalog sync. Scope note: the brief for this work held that the Codex pool's "needs reauthentication" warning was itself the terminal warning. It is not. That line comes from warnGatedNativeSuppressedOnce (src/codex/catalog/gated-native-warn.ts) and is a bare console.warn that never enters the structured result, and per-account pool refresh failures are caught and returned as null by model-entitlements.ts rather than propagating. The account-level fault that does reach the gate is a native MAIN credential refresh failing outside that per-account catch: it rejects the entitlement gather, surfaces as the catch-all "catalog sync skipped" warning, and permanently un-readies the process. That is the same class of blast radius the report describes, and it is closed here. Closes #5181 * fix(codex): scope model denial evidence to the credential generation An authenticated unsupported-model refusal is remembered for six hours under (account id, model id) with no credential generation. Re-auth of the same pool account keeps the internal id and increments the generation, and can swap the subscription underneath it — so the previous credential's refusal keeps steering routing away from an account that now has access. The account-wide forget that would have covered this (`model-entitlements.ts`) sits behind a condition requiring a previously cached roster with changed identity, so with no roster it never runs — which is exactly the state the flagship request path is usually in, since nothing on it refills the five-minute roster cache. Entries now carry the generation they were observed under, and: - the reader drops evidence whose generation is no longer live, so superseded evidence stops influencing the replacement without depending on the conditional forget; - a write from an older generation cannot overwrite newer evidence, so an in-flight generation-G refusal landing after G+1 is saved neither resurrects nor rewrites it; - a clear from an older generation cannot delete newer evidence, so a late generation-G success cannot re-admit an account the current credential has just been refused by. An equal generation still clears: that is the ordinary "this account just served this model" case. Evidence that cannot name a generation is dropped rather than attributed to whatever is current. Main-account contexts carry no pool generation, and already carry no account id, so nothing changes there. Cache-only routing, the six-hour TTL, the entry bound, positive-roster precedence and empty-filter restoration are all untouched. The liveness predicate is injected rather than imported so the store stays a leaf module; production wires `isCodexAccountGenerationLive`. Closes #4952 * fix(codex): keep main-pool denial evidence and fence stale writes before they evict Three defects in the carried #5092, found by adversarial review of the generation fences rather than by CI, which was green on all three. 1. main-pool evidence was silently discarded. Both production call sites pass `kind === "pool" ? generation : undefined`, and the carried code dropped any refusal that could not name a generation. A `main-pool` context - the stored main login taking part in rotation - has a real accountId and no POOL credential generation, because its credential lives in auth.json. So for that account #4906 reverted: the pool would re-send the model the login had just refused, on every request. A refusal with no generation is now ACCOUNT-scoped and the generation fences do not apply to it. This is the rule core-codex-account.ts already uses for the quota writer, where an absent credential generation skips the liveness check instead of discarding the write. Request-owned `main` is unaffected: its accountId is null and the existing guard returns before any of this. 2. The stale-write fence only rejected a late refusal when the same key already held newer evidence. With no entry for its own key the stale row was inserted, and at the 512-entry bound that insert evicts the oldest valid row - which no later read fence can restore, because the evidence is gone. The write now rejects a generation that is no longer live before touching the map at all, which closes the race the issue names rather than only its common case. 3. The read fence validated liveness for every non-expired entry before checking the model or the caller's exclusion set, and DELETED any row that came back not-live. Two problems. The issue asks for validation after the exclusion read fence, and an excluded account - a draining profile switch, or a request-owned credential - was causing a credential-store read on its behalf. And the predicate cannot tell "this account reauthenticated" from "the store could not be read": loadCodexAccountRecordStore catches a read failure and returns {}, so an unreadable store deleted valid evidence permanently. The reader now matches the model and applies the exclusion set first, and SKIPS a non-live row instead of deleting it. Skipping already produces the routing outcome on every read; deletion only saved memory in a bounded map, and it was the part that was unsafe. Superseded rows still leave by TTL, by eviction, and by the account-wide forget. Each liveness check also reloaded and reparsed the whole credential store per row, on the request path. The seam is now a factory that opens once per lookup over one loadCodexAccountRecordSnapshot, which is the shape that snapshot was added for, and it is not opened at all when no matching row carries a generation. Every constraint the issue sets is unchanged: cache-only routing with no added upstream fetch, the six-hour TTL, the 512-entry bound, positive-roster precedence, and empty-filter restoration. This remains an ordering preference, not an eligibility filter, and a live-generation refusal still denies. Co-authored-by: Abhishek Sharma <abhicse24@gmail.com> --------- Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> Co-authored-by: lidge-jun <lidge-jun@users.noreply.github.com> Co-authored-by: Abhishek Sharma <abhicse24@gmail.com>
추가 리뷰 · 우선순위 32 / 808ba026a는 지난 리뷰에서 말한 것 대부분을 고쳤습니다. 토큰 서버가 5xx를 주면 이제 재로그인을 요구하지 않습니다. 갱신 토큰이 거절되거나 만료된 경우만 src/codex/auth-api/account-list.ts:116 - 메모리에 재로그인 표시가 있으면 건강 이유로 무조건 src/codex/auth-api/pool-quota-probe.ts:410 - 메인테이너의 판단이 필요한 지점 검증 401/403이 메모리 표시보다 먼저여야 하는지 정해 주세요. 작성자는 표시가 있는 동안은 죽은 갱신을 이 조회에서도 디스크에 남길지 정해 주세요. 토큰 가디언은 이미 너의 추천 116줄은 메모리 표시만 이유일 때 이유를 넘기지 마세요. 그러면 253줄이 저장된 401과 403을 그대로 씁니다. 410줄에서 갱신이 이 댓글은 grok-bot이 작성했습니다 |
|
The change from this PR landed on It went in alongside the Closing because the content is on |
|
A note for the record: the #5199 squash (09698b9) carried this PR's original commit only — the review-fix commit pushed to this branch before the squash, and the follow-up scope recommended in the second review, did not land on dev. Opened #5222 as a follow-up carrying both, with the two reviewer-flagged decision points implemented per the reviewer's recommendation. |
2 participants
Motivation
poolAccountDtocollapses three independent reauthentication causes into oneneedsReauthboolean and then guesses the reason from overlapping booleans. When the pool quota probe observed a terminal token-refresh failure, the DTO reportedquota_unauthorized, and the health projection always claimedrefresh_failed— so a WHAM 401 and a dead refresh grant were indistinguishable to the operator who has to act on them.Description
PoolQuotaResultnow carriesreauthReason("refresh_failed" | "quota_unauthorized"), recorded where the probe observes the failure: terminal WHAM 401s on the initial response and the replay reportquota_unauthorized; terminalTokenRefreshErrors and terminal-fenced refresh lineages reportrefresh_failed.poolAccountDtoprefers the observedquotaResult.reauthReasonover boolean inference, and maps it intoprojectCodexAccountHealthso the persisted verdict reportsunauthorizedfor quota/missing-credential failures instead of alwaysrefresh_failed.handleCodexAuthAPI.Tests
bun test tests/codex-integration/codex-auth-api.test.ts tests/usage/quota-401-recovery-runtime.test.ts tests/oauth/oauth-health.test.ts— 374 pass, 0 failbun x tsc --noEmit— cleanReview readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
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.
Summary by CodeRabbit
New Features
Tests