fix(codex): persist a terminal validation verdict for a revoked pool grant - #4140
Conversation
…grant A Codex pool credential whose OAuth grant was revoked upstream kept lastCodexValidationStatus: "ok" in codex-accounts.json and was reported as healthy for as long as the install lived, while every request using it 401'd. guardianSweep's pool branch already classified the failure -- it computed `permanent` for a revoked/expired TokenRefreshError -- but spent it only on widening an in-memory backoff delay. The persisted-verdict branch next to it required `needsWarmup`, which is false in the default configuration, and additionally excluded every TokenRefreshError, so the one class of failure that proves the credential is dead was the one class that never reached the record. Persist that verdict instead, independently of needsWarmup, and add a lastCodexValidationTerminal marker so a dead grant is distinguishable from a transient warmup failure. The write is fenced on the generation the sweep actually observed, so a credential replaced mid-refresh is never branded by the previous credential's failure. The marker clears itself in both directions that disprove it: markCodexAccountValidated clears it explicitly, and every credential write drops it because the record is rebuilt from preservedValidationMetadata, which deliberately omits it. A refresh that succeeds disproves "the grant was revoked", so one spurious invalid_grant cannot brand a live account dead forever. On the read side, projectCodexAccountHealth now reads that verdict and reports reauth_required/refresh_failed -- the accurate statement, since only a re-login recovers a revoked grant, and the existing union member already carries the Codex reauth action. collectLocalCodexEntries is folded onto the same projector rather than keeping its inlined copy, which is how the CLI would otherwise have kept reporting the account healthy after the dashboard stopped. Background warmup stays opt-in; no default-on probe is introduced. Closes #4120
Diff-level roadmap for the three-layer chain (#4120 -> #3848 -> #3777), with the wp1 design decisions recorded: why the terminal marker is an extra optional key rather than a new status value, why it clears itself on every credential write, why the generation fence declines rather than clobbers, and why the dashboard fix is a server-side projection onto the existing reauth_required member.
|
Warning Review limit reachedNext included review available in 36 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (11)
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 |
리뷰 · 우선순위 70 / 80이 PR은 Codex 풀 계정에서 업스트림 OAuth grant가 이미 폐기됐는데도 현재 이 PR의 쓰기는 터미널 읽기는 터미널 판정을 기존 union 멤버
메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
✅ Deterministic PR hygiene checks passed. |
Brings the branch onto dev 71a0c30, which now contains the terminal validation verdict from lidge-jun#4140. The overlapping hunks in account-store, token-guardian and health were already reconciled in aeb86cb, so dev merges clean here. The 26 branch commits are authored by DaedalGames with an email (noreply@daedalgames.github.io) that is not linked to a GitHub account, so GitHub renders them as unlinked and they would credit nobody on the contributor graph. These trailers carry the credit explicitly, using addresses GitHub can resolve. Co-authored-by: DaedalGames <daedal@daedal.games> Co-authored-by: shaun0927 <70629228+shaun0927@users.noreply.github.com>
1 participant
Summary
Closes #4120.
A Codex pool credential whose OAuth grant was revoked upstream kept
lastCodexValidationStatus: "ok"incodex-accounts.jsonand was presented as healthy indefinitely, while every request using it returned401 token_revoked. The reporter found 6 of 12 accounts imported from such a store were dead on arrival.guardianSweep's pool branch already classified the failure — it computedpermanentfor a revoked/expiredTokenRefreshError— but spent it only on widening an in-memory backoff delay, which no health surface reads and no restart survives. The persisted-verdict branch beside it requiredneedsWarmup(false in the default configuration, sincecodexWarmupEnableddefaults off) and additionally excluded everyTokenRefreshError. So the one class of failure that proves a credential is dead was the one class that never reached the record.Write side. A terminal
TokenRefreshErrornow persistslastCodexValidationStatus: "failed"regardless ofneedsWarmup, plus a newlastCodexValidationTerminalmarker that distinguishes a dead grant from a transient warmup failure. Background warmup stays opt-in — no default-on probe is introduced, so issue expectation 3 is deliberately not implemented.The marker clears itself, which is what keeps one spurious
invalid_grantfrom branding a live account dead forever:markCodexAccountValidatedclears it explicitly, and every credential write drops it because those writers rebuild the record frompreservedValidationMetadata, which deliberately omits it. A refresh that succeeds disproves "the grant was revoked".Generation fence.
markCodexAccountValidationFailedtakes an optionalexpectedGenerationand returns whether it wrote. The sweep passes the generation it actually observed —record.generationbefore the refresh,token.generationonce a refresh has committed. If another writer replaced the credential mid-flight the write is declined rather than applied: the failure cannot be attributed to a credential the sweep never observed, and declining is always safer than branding a freshly installed one dead.Read side.
projectCodexAccountHealthnow reads that verdict and reportsreauth_required/refresh_failed. That is the accurate projection rather than a convenient one: only a re-login recovers a revoked grant, and the existing union member already attachesCODEX_REAUTH_ACTION("reauthenticate via the dashboard Codex account pool").collectLocalCodexEntrieskept an inlined copy of the projector instead of calling it, which is precisely howocx statusandocx doctorwould have gone on reporting the account healthy after the dashboard stopped; it is folded onto the shared projector so the two cannot drift again.Two design notes a reviewer may want to challenge
Why a separate key instead of a third
lastCodexValidationStatusvalue.isCredentialRecordadmits only"ok" | "failed". A record carrying an unrecognized status fails that predicate,normalizeRecordthen failsisCredentialand returnsundefined, andloadCodexAccountRecordStoresilently omits the record — so an operator who wrote a"revoked"status and then rolled back to an earlier version would lose the whole account entry, credential included. An unknown extra key is spread through untouched instead.Why there is no
gui/diff. The dashboard does not render the server'shealthLabel; it recomputes the badge from thehealthobject throughgui/src/oauth-health-display.ts. Reusingreauth_requiredtherefore already turns the account row amber, prints "Reauthentication required" and surfaces the action, with no GUI change. A new warning reason would have cost a GUI enum, nine i18n locales and a screenshot, for strictly worse copy. ShowinglastCodexValidatedAtas a first-class dashboard column (the optional half of issue expectation 2) is left as a separate GUI change.Verification
bun run test, nobun run test:changed, nobun run typecheck, nobun run build:gui, nobun run lint:gui, nobun install. The exact-head remote CI on this PR is the gate. This PR was also pushed with--no-verifyunder the same instruction.scripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json):tests/codex-integration/token-guardian.test.ts— a revoked grant persistsfailed+ terminal with warmup disabled; a transient (unknown) refresh failure leaves the recorded verdict untouched; a credential replaced mid-refresh is not branded by the previous credential's failure.tests/codex-integration/codex-account-store.test.ts— the generation fence declines a stale write and accepts a current one; a terminal verdict is cleared by a completed validation, by a re-login, and by a successful CAS refresh, while a transient failure neither clears nor invents it.tests/oauth/oauth-health.test.ts— a healthy record still projectshealthy, a terminal record projectsreauth_required, a non-terminal failure does not, and the CLI collector reports the terminal verdict with the Codex reauth action.devlog/_plan/260909_codex_credential_health_chain/.Checklist
devlog/_plan/.)refresh_revoked/refresh_expiredtoken derived from the error'sreasondiscriminator and never from response text, so no token or upstream body can reach the store or a log; no default is turned on, and background warmup remains opt-in; the new write is strictly narrowing (an account can only move from healthy to needs-reauth), so a failure of this code cannot admit a credential that was previously excluded.