diff --git a/devlog/_plan/260909_codex_credential_health_chain/000_plan.md b/devlog/_plan/260909_codex_credential_health_chain/000_plan.md new file mode 100644 index 0000000000..9c764929a8 --- /dev/null +++ b/devlog/_plan/260909_codex_credential_health_chain/000_plan.md @@ -0,0 +1,73 @@ +# Codex credential health chain (#4120, #3848, #3777) — plan + +## Reader summary + +Problem: a Codex pool credential whose OAuth grant was revoked upstream keeps +`lastCodexValidationStatus: "ok"` in `codex-accounts.json` and is presented as healthy for as long +as the install lives. Answer: a revoked/expired refresh grant is the strongest terminal evidence +available, so the guardian now persists that verdict on the account record instead of dropping it +into an in-memory backoff map, and the health projector reads it. What changes: an account with a +dead grant reports "Reauthentication required" on the dashboard, in `ocx status` and in +`ocx doctor`, and keeps reporting it across restarts until a re-login or a successful refresh +disproves it. + +## Loop spec + +- Loop archetype: satisfy-spec, three work-phases delivered as a bottom-up manual branch chain + (wp1 -> wp2 -> wp3), so this unit opens with the diff-level roadmap below and each decade doc is + revalidated at its own P. +- Trigger: maintainer directive to deliver #4120, shepherd #3848 and build #3777 as one chain. +- Goal: each layer is a non-draft, mergeable PR whose exact-head remote CI is green. +- Non-goals: no merging (the dispatching session owns merge order); no rebase of any layer unless + that session asks for one; no release or promotion; no default-on background warmup; and no + local product suite, typecheck, build, lint or install — the standing maintainer rule is that + remote CI on the PR's exact final head is the only gate, and every skipped local check is + recorded NOT RUN. +- Verifier: `.github/workflows/ci.yml` on `pull_request`; the `test` job selects + `tests/**` through the changes filter, so the appended regression rows in + `tests/codex-integration/` and `tests/oauth/` are in the selected set on Linux, macOS and + Windows. +- Stop condition: all three PRs non-draft with green exact-head CI, or a BLOCKED outcome naming + the blocker. wp1 must be able to land alone if wp2 stalls. +- Escalation: a required rebase, a merge conflict against `dev`, or any need to run a local suite + returns to the dispatching session rather than being resolved unilaterally. + +## Root cause (#4120, evidence) + +`guardianSweep`'s pool branch decides whether to sweep an account at +`src/oauth/token-guardian.ts:210-215`: + + const needsRefresh = cred.expiresAt <= nowMs + horizonMs; + const needsWarmup = opts.codexWarmupEnabled && (...); + if (!needsRefresh && !needsWarmup) continue; + +and decides what to persist on failure at `src/oauth/token-guardian.ts:238-241`: + + const permanent = err instanceof TokenRefreshError && (err.reason === "revoked" || err.reason === "expired"); + if (needsWarmup && !(err instanceof TokenRefreshError)) { + markCodexAccountValidationFailed(id, codexWarmupFailureReason(err)); + } + +`permanent` is computed and then used only to widen the in-memory backoff delay +(`recordFailure`, `:100-115`), which does not survive a restart and is not what any health +surface reads. The persisted-verdict branch requires `needsWarmup`, which is false in the default +configuration because `codexWarmupEnabled` defaults to `false` (`:86`), and it additionally +excludes every `TokenRefreshError`. So the one class of failure that proves the credential is dead +is the one class that never reaches the account record. + +The second half of the defect is on the read side: `src/oauth/health.ts` never consults the +validation metadata at all. `projectCodexAccountHealth` (`:196-210`) reads only the in-memory +reauth flag and the cooldown snapshot, so a record carrying a stale login-time `"ok"` projects +`{ status: "healthy" }`. + +## Work-phase roadmap + +| Phase | Doc | Layer | Base | +|---|---|---|---| +| wp1 | `010_wp1_terminal_verdict.md` | persist + project the terminal verdict (#4120) | `origin/dev` | +| wp2 | `020_wp2_quota_registration.md` | shepherd #3848 onto the chain | wp1 head | +| wp3 | `030_wp3_anthropic_plan.md` | Anthropic subscription tier (#3777) | wp2 head | + +The order is a dependency order, not an effort order: wp3 edits `src/cli/account-api.ts`, which +wp2 already rewrites, and wp2 touches the same account-store and guardian surfaces wp1 changes. +Each layer stands alone for review and carries its own tests. diff --git a/devlog/_plan/260909_codex_credential_health_chain/010_wp1_terminal_verdict.md b/devlog/_plan/260909_codex_credential_health_chain/010_wp1_terminal_verdict.md new file mode 100644 index 0000000000..20058f2c4d --- /dev/null +++ b/devlog/_plan/260909_codex_credential_health_chain/010_wp1_terminal_verdict.md @@ -0,0 +1,98 @@ +# wp1 — persist and project the terminal validation verdict (#4120) + +Class C4: OAuth credential handling, the credential store, and a health surface. + +## Decision 1 — an extra optional key, not a new status value + +The obvious shape for "this credential is dead" is a third value in the persisted status union +(`"ok" | "failed" | "revoked"`). It is unsafe here. `isCredentialRecord` +(`src/codex/account-store.ts:60`) admits only `undefined | "ok" | "failed"`; a record carrying an +unrecognized value fails that predicate, so `normalizeRecord` (`:74-88`) falls through to +`isCredential`, which also fails because a record has no top-level `accessToken`, and returns +`undefined`. `loadCodexAccountRecordStore` (`:96-101`) then silently omits the record. An +operator who writes a terminal verdict on 2.51 and rolls back to 2.50 would lose the whole +account entry, credential included. + +An additional optional key has the opposite property: `normalizeRecord` returns +`{ ...value, refreshGrantFingerprint }`, so an unknown key is carried through untouched by a build +that has never heard of it. So the record gains: + + lastCodexValidationTerminal?: boolean; + +## Decision 2 — the marker clears itself + +A terminal verdict that can only be set is worse than no verdict: one spurious `invalid_grant` +from upstream would brand a live account dead forever, since background warmup is off by default +and nothing else would revisit it. The marker therefore has exactly two ways to disappear, and both +are structural rather than remembered: + +- `markCodexAccountValidated` clears it explicitly, alongside the error string it already clears. +- Every credential write drops it for free. `saveCodexAccountCredential` (`:145`), + `saveCodexAccountCredentialIfGeneration` (`:216`) and + `commitRefreshedCodexCredentialWithAliases` (`:275`, `:303`) rebuild the record from an + explicit field list plus `preservedValidationMetadata` (`:121-128`) and never spread + `...current`. Keeping the new key out of that pick list is what makes a successful refresh or a + re-login erase the verdict, which is correct: a refresh that succeeds disproves "grant revoked". + +Those five sites plus the tombstone at `:324` are every record writer in the codebase — +`loadCodexAccountRecordStore` is module-private and no other module writes +`codex-accounts.json`. + +## Decision 3 — the generation fence + +`markCodexAccountValidationFailed` gains an options bag with `expectedGeneration` and +`terminal`, and returns whether it wrote. The guardian passes the generation it actually observed: +`record.generation` before the refresh, replaced by `token.generation` once a refresh has +committed, because a successful refresh bumps the generation and a warmup failure after it belongs +to the new credential. + +A failed refresh never follows a commit inside the same call: `resolveCodexToken` returns on the +freshness shortcut (`:717-721`), on same-grant adoption (`:465-475`) and on the CAS commit +(`:975-985`), and the `TokenRefreshError` throw (`:948`) is reached only from a `!res.ok` +token response with no prior write. If a different writer replaced the credential between the +guardian's read and the locked re-read, the fence declines to write. That is a deliberate false +negative: the failure cannot be attributed to the credential the sweep observed, and refusing to +write is always safer than branding a freshly installed credential dead. + +## Decision 4 — project onto the existing health member, with no GUI diff + +A terminal verdict maps to `{ status: "reauth_required", reason: "refresh_failed" }`, which +already exists in `OAuthAccountHealth` (`src/oauth/health.ts:14-18`). That is not a shortcut, it +is the accurate statement: only a re-login fixes a revoked grant, and `actionFor` (`:88-95`) +already attaches `CODEX_REAUTH_ACTION` — "reauthenticate via the dashboard Codex account pool" — +for the `codex` provider. + +Reusing it also means the dashboard needs no change at all. The GUI does not render the server's +`healthLabel`; it recomputes the badge from the `health` object through +`gui/src/oauth-health-display.ts`, so `reauth_required` already turns the row amber +(`codex-account-pool-cards.tsx:81,88`), prints "Reauthentication required" and shows the action. +A new warning reason would have required a GUI enum, nine i18n locales and a dashboard screenshot, +for strictly worse copy. + +`collectLocalCodexEntries` (`:265-282`) currently inlines a copy of the projector's body rather +than calling it, which is how the CLI path would have silently missed this fix. It is folded onto +`projectCodexAccountHealth` so the two cannot drift again. + +Precedence note: `projectOAuthAccountHealth` checks reauth before cooldown, so an account that is +both revoked and quota-cooled now reports reauth. That is the right order — telling an operator to +wait out a cooldown on a credential that will never work again is a false promise. + +## Out of scope + +Issue expectation 3 (revalidate stored pool credentials on a bounded schedule even with warmup +disabled) is declined here: it means a default-on inference probe, which this change is explicitly +not allowed to introduce. Showing `lastCodexValidatedAt` as a first-class dashboard column is also +deferred — it is a GUI change with no server-side defect behind it. + +## Verification + +Appended to existing test files, because a new test file additionally requires entries in +`scripts/test-layout/layout.json` and `tests/fixtures/test-layout-expected.json`: + +- `tests/codex-integration/token-guardian.test.ts` — a revoked grant persists + `failed` + terminal with no warmup enabled; a transient (`unknown`) refresh failure persists + nothing; a credential replaced mid-refresh is not clobbered. +- `tests/codex-integration/codex-account-store.test.ts` — the generation fence declines a stale + write, `markCodexAccountValidated` clears the marker, and a credential write drops it. +- `tests/oauth/oauth-health.test.ts` — a terminal record projects `reauth_required` with the + Codex reauth action, and an ordinary record still projects healthy. diff --git a/devlog/_plan/260909_codex_credential_health_chain/020_wp2_quota_registration.md b/devlog/_plan/260909_codex_credential_health_chain/020_wp2_quota_registration.md new file mode 100644 index 0000000000..dbd6364117 --- /dev/null +++ b/devlog/_plan/260909_codex_credential_health_chain/020_wp2_quota_registration.md @@ -0,0 +1,24 @@ +# wp2 — shepherd #3848 onto the chain (quota-exhausted registration, #3846) + +Author: @shaun0927 (Junghwan). This layer is carried, not reimplemented, so the original author's +`Co-authored-by` trailers are preserved on the branch commits — `missing_coauthor_credit` in +`.github/scripts/pr-carry-attribution.cjs` reads the trailer, and a sentence in a commit body is +read by nothing. + +Substance (unchanged from the original PR): a Codex account whose weekly allowance is exhausted +cannot complete the mandatory inference warmup, so registration fails outright. The change persists +it as validation-pending, keeps it out of routing and manual selection, and requires a human +dashboard "Refresh quotas" click to finish validation, because finishing it spends model quota. + +Work in this phase: + +- Retarget the PR base from `dev` onto the wp1 head branch. +- Resolve the conflict against current `dev`. This is a conflict inside this lane's own chain, + which is the one case the no-rebase rule does not cover; a cross-lane rebase still returns to the + dispatching session. +- Preserve the GUI evidence screenshots already in the description — the PR touches `gui/`, so + `missing_ui_screenshot` (`.github/scripts/pr-quality.cjs:531`) requires them. +- Restore repository hygiene: no vendored reference clones, no tracked gitlink, no security triage + under `devlog/` (`tests/ci-workflows/repo-hygiene.test.ts`). + +Class C4 — authentication, account store, guardian and GUI in one change. diff --git a/devlog/_plan/260909_codex_credential_health_chain/030_wp3_anthropic_plan.md b/devlog/_plan/260909_codex_credential_health_chain/030_wp3_anthropic_plan.md new file mode 100644 index 0000000000..56dc7093f0 --- /dev/null +++ b/devlog/_plan/260909_codex_credential_health_chain/030_wp3_anthropic_plan.md @@ -0,0 +1,23 @@ +# wp3 — expose the Anthropic subscription tier (#3777) + +The OpenAI provider already reports a per-account `plan` string, so a consumer can weight each +account's remaining quota by its tier. The Anthropic provider reports rich quota and no tier at +all, so a six-account Claude pool has no meaningful aggregate capacity number. + +Surface, bottom to top: + +- `src/providers/quota.ts` `fetchAnthropicUsageQuota` — read the subscription tier from a real + field in the upstream usage/billing response. +- `src/oauth/index.ts` — add `plan: string | null` to the OAuth account summary. +- `src/server/management/oauth-account-routes.ts` — carry it on the management DTO. +- `src/cli/account-api.ts` — carry it on the CLI DTO. This file is why the layer chains on wp2, + which already rewrites it. +- The Anthropic GUI rows. + +Hard constraint from the issue and from the maintainer: if the upstream response carries no tier +field, land `plan: null` plus documentation saying so. Do not infer a Max x5 / x20 mapping from +quota percentages — the issue reporter already established that percentages are normalized per +account and carry no tier information, so a guess would be indistinguishable from data. + +The explicit `null` matters: it lets a consumer tell "unknown tier" apart from "OpenCodex too old +to report one". diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index 952ac048b8..9a2c1737e0 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -58,7 +58,8 @@ function isCredentialRecord(value: unknown): value is CodexAccountCredentialReco && (value.replacedAt === undefined || typeof value.replacedAt === "number") && (value.lastCodexValidatedAt === undefined || typeof value.lastCodexValidatedAt === "number") && (value.lastCodexValidationStatus === undefined || value.lastCodexValidationStatus === "ok" || value.lastCodexValidationStatus === "failed") - && (value.lastCodexValidationError === undefined || typeof value.lastCodexValidationError === "string"); + && (value.lastCodexValidationError === undefined || typeof value.lastCodexValidationError === "string") + && (value.lastCodexValidationTerminal === undefined || typeof value.lastCodexValidationTerminal === "boolean"); } export function refreshGrantFingerprintForToken(refreshToken: string): string { @@ -118,6 +119,15 @@ function persistCredentialMutation(store: CodexAccountStore): void { advanceCodexCredentialMutationEpoch(); } +/** + * Validation metadata that survives a credential write. + * + * `lastCodexValidationTerminal` is deliberately NOT in this list. Every credential write — + * re-login, the CAS refresh commit, same-grant alias propagation — rebuilds the record from this + * pick list, so leaving the marker out is what makes a successful refresh or a re-authentication + * erase a terminal verdict. Both events disprove "the grant was revoked", and a verdict that + * could only ever be set would brand an account dead forever on one spurious `invalid_grant`. + */ function preservedValidationMetadata(record: CodexAccountCredentialRecord | undefined): Pick< CodexAccountCredentialRecord, "lastCodexValidatedAt" | "lastCodexValidationStatus" | "lastCodexValidationError" @@ -163,22 +173,53 @@ export function markCodexAccountValidated(id: string, atMs: number = Date.now()) lastCodexValidatedAt: atMs, lastCodexValidationStatus: "ok", lastCodexValidationError: undefined, + // A completed validation is the direct refutation of a terminal verdict, and this + // spread would otherwise carry the old marker forward. + lastCodexValidationTerminal: undefined, }; persist(store); }); } -export function markCodexAccountValidationFailed(id: string, reason: string): void { - withCredentialMutationLockSync(() => { +export interface MarkCodexAccountValidationFailedOptions { + /** + * Write only while the stored record is still at this generation. + * + * A validation attempt is not atomic with the store: an operator can re-authenticate the + * account, or another writer can commit a refresh, while a probe is still in flight. Without + * this fence the late failure lands on whatever credential happens to be there and brands a + * freshly installed one dead. Declining to write is the safe direction — the failure cannot be + * attributed to a credential the caller never observed. + */ + expectedGeneration?: number; + /** The grant itself is revoked or expired; only a re-login clears it. */ + terminal?: boolean; +} + +/** Returns whether the verdict was actually persisted (false when the fence declined it). */ +export function markCodexAccountValidationFailed( + id: string, + reason: string, + options: MarkCodexAccountValidationFailedOptions = {}, +): boolean { + return withCredentialMutationLockSync(() => { const store = loadCodexAccountRecordStore(); const current = store[id]; - if (!current || current.deletedAt != null || !current.credential) return; + if (!current || current.deletedAt != null || !current.credential) return false; + if (options.expectedGeneration !== undefined && current.generation !== options.expectedGeneration) { + return false; + } store[id] = { ...current, lastCodexValidationStatus: "failed", lastCodexValidationError: reason, + // Only ever set here. A transient failure must not clear a terminal marker set earlier, + // and it must not invent one either, so the flag is written only when the caller proves + // the grant is dead. + ...(options.terminal ? { lastCodexValidationTerminal: true } : {}), }; persist(store); + return true; }); } diff --git a/src/oauth/health.ts b/src/oauth/health.ts index 011ebd8f41..cf5fa8c35d 100644 --- a/src/oauth/health.ts +++ b/src/oauth/health.ts @@ -1,7 +1,7 @@ import { getCodexAccountHealthSnapshot, type CodexCooldownSource } from "../codex/routing"; import { getAnthropicAccountHealthSnapshot } from "./anthropic-routing"; import { isAccountNeedsReauth } from "../codex/account-runtime-state"; -import { getCodexAccountCredential, listCodexAccountIds } from "../codex/account-store"; +import { getCodexAccountCredential, listCodexAccountIds, readCodexAccountRecord } from "../codex/account-store"; import { MAIN_CODEX_ACCOUNT_ID } from "../codex/main-account"; import { readRuntimePort } from "../config/process-state"; import { LOCAL_MANAGEMENT_READ_PATHS } from "../lib/local-management-capability"; @@ -200,15 +200,38 @@ export function projectCodexAccountHealth(input: { }): OAuthAccountHealth { const now = input.now ?? Date.now(); const snap = getCodexAccountHealthSnapshot(input.accountId, now); + // A persisted terminal verdict outranks the in-memory reauth flag rather than duplicating it: + // the flag lives in this process and a revoked grant does not. Without this read, an account + // whose grant was revoked upstream keeps its login-time `lastCodexValidationStatus: "ok"` and + // every surface reports it healthy until someone tries to use it (#4120). Only a re-login + // clears the marker, so `reauth_required` is the accurate projection — and it is deliberately + // checked ahead of any cooldown, because telling an operator to wait out a rate limit on a + // credential that will never work again is a false promise. + const needsReauth = input.needsReauth || hasTerminalCodexValidationFailure(input.accountId); return projectOAuthAccountHealth({ - needsReauth: input.needsReauth, - reauthReason: input.needsReauth ? "refresh_failed" : undefined, + needsReauth, + reauthReason: needsReauth ? "refresh_failed" : undefined, cooldownUntilMs: snap?.cooldownUntil, cooldownReason: cooldownReasonFromSource(snap?.cooldownSource), now, }); } +/** + * True when the stored pool record carries a terminal validation verdict — the refresh grant was + * revoked or expired, so no retry recovers it. The main account has no record in the pool store, + * so it never matches. + */ +function hasTerminalCodexValidationFailure(accountId: string): boolean { + // The main account lives in the native Codex auth file, not the pool store, so a lookup could + // only ever miss — and each lookup re-reads and re-hardens the whole store file. + if (accountId === MAIN_CODEX_ACCOUNT_ID) return false; + const record = readCodexAccountRecord(accountId); + return record?.deletedAt == null + && record?.lastCodexValidationTerminal === true + && record?.lastCodexValidationStatus === "failed"; +} + /** * Incomplete credentials warning. Kiro may intentionally store an empty refresh * when authenticated via KIRO_ACCESS_TOKEN or a pasted access-only token, as long @@ -272,13 +295,11 @@ function collectLocalCodexEntries(now: number): OAuthHealthEntry[] { const hasPoolCredential = accountId !== MAIN_CODEX_ACCOUNT_ID && getCodexAccountCredential(accountId) !== null; if (!hasPoolCredential && !needsReauth && !snap) continue; - const health = projectOAuthAccountHealth({ - needsReauth, - reauthReason: needsReauth ? "refresh_failed" : undefined, - cooldownUntilMs: snap?.cooldownUntil, - cooldownReason: cooldownReasonFromSource(snap?.cooldownSource), - now, - }); + // Call the projector rather than inlining a second copy of it. This collector serves the CLI + // (`ocx status`, `ocx doctor`) while the dashboard DTO goes through projectCodexAccountHealth, + // and the duplicated body is exactly how the CLI would have kept reporting a revoked account + // as healthy after the dashboard stopped. + const health = projectCodexAccountHealth({ accountId, needsReauth, now }); pushEntry(entries, "codex", accountId, health); } return entries; diff --git a/src/oauth/token-guardian.ts b/src/oauth/token-guardian.ts index a84d68a7c2..378d13c0fd 100644 --- a/src/oauth/token-guardian.ts +++ b/src/oauth/token-guardian.ts @@ -215,9 +215,13 @@ export async function guardianSweep(nowMs: number = Date.now()): Promise { try { const token = await getValidCodexToken(id); + observedGeneration = token.generation; if (needsRefresh) result.refreshed.push(key); if (needsWarmup) { await warmCodexAccount({ @@ -235,11 +239,26 @@ export async function guardianSweep(nowMs: number = Date.now()): Promise { expect(JSON.stringify(record)).not.toContain("sensitive-access revoked"); }); + test("a generation-fenced validation failure is declined once the credential has been replaced", async () => { + const { + markCodexAccountValidated, + markCodexAccountValidationFailed, + readCodexAccountRecord, + saveCodexAccountCredential, + } = await import("../../src/codex/account-store"); + saveCodexAccountCredential("fenced", { accessToken: "a1", refreshToken: "r1", expiresAt: 1, chatgptAccountId: "acc" }); + markCodexAccountValidated("fenced", 1234); + const stale = readCodexAccountRecord("fenced")!.generation; + + // The operator re-authenticates while a probe of the previous credential is still in flight. + saveCodexAccountCredential("fenced", { accessToken: "a2", refreshToken: "r2", expiresAt: 2, chatgptAccountId: "acc" }); + + expect(markCodexAccountValidationFailed("fenced", "refresh_revoked", { + expectedGeneration: stale, + terminal: true, + })).toBe(false); + + const record = readCodexAccountRecord("fenced")!; + expect(record.lastCodexValidationStatus).toBe("ok"); + expect(record.lastCodexValidationTerminal).toBeUndefined(); + + // The same verdict against the CURRENT generation is accepted. + expect(markCodexAccountValidationFailed("fenced", "refresh_revoked", { + expectedGeneration: record.generation, + terminal: true, + })).toBe(true); + expect(readCodexAccountRecord("fenced")!.lastCodexValidationTerminal).toBe(true); + }); + + test("a terminal verdict is cleared by a completed validation and by any credential write", async () => { + const { + markCodexAccountValidated, + markCodexAccountValidationFailed, + readCodexAccountRecord, + saveCodexAccountCredential, + saveCodexAccountCredentialIfGeneration, + } = await import("../../src/codex/account-store"); + const dead = { accessToken: "dead", refreshToken: "dead-r", expiresAt: 1, chatgptAccountId: "acc" }; + const fresh = { accessToken: "fresh", refreshToken: "fresh-r", expiresAt: 2, chatgptAccountId: "acc" }; + const rotated = { accessToken: "rotated", refreshToken: "rotated-r", expiresAt: 3, chatgptAccountId: "acc" }; + + saveCodexAccountCredential("terminal", dead); + markCodexAccountValidationFailed("terminal", "refresh_revoked", { terminal: true }); + expect(readCodexAccountRecord("terminal")!.lastCodexValidationTerminal).toBe(true); + + // A transient failure afterwards must neither clear nor re-assert the terminal marker. + markCodexAccountValidationFailed("terminal", "http_status:500"); + expect(readCodexAccountRecord("terminal")!.lastCodexValidationTerminal).toBe(true); + + // A completed validation refutes it outright. + markCodexAccountValidated("terminal", 4321); + expect(readCodexAccountRecord("terminal")!.lastCodexValidationTerminal).toBeUndefined(); + + // So does a re-login: the verdict belonged to the grant that was replaced. + markCodexAccountValidationFailed("terminal", "refresh_revoked", { terminal: true }); + saveCodexAccountCredential("terminal", fresh); + expect(readCodexAccountRecord("terminal")!.lastCodexValidationTerminal).toBeUndefined(); + + // And so does a successful CAS refresh, which proves the grant is still alive. + markCodexAccountValidationFailed("terminal", "refresh_revoked", { terminal: true }); + const generation = readCodexAccountRecord("terminal")!.generation; + expect(saveCodexAccountCredentialIfGeneration("terminal", generation, rotated)).toBe(true); + const record = readCodexAccountRecord("terminal")!; + expect(record.lastCodexValidationTerminal).toBeUndefined(); + // The non-terminal half of the verdict is still preserved metadata. + expect(record.lastCodexValidationStatus).toBe("failed"); + }); + test("successful refresh returns bumped generation and persists rotated refresh token", async () => { const { getCodexAccountCredential, diff --git a/tests/codex-integration/token-guardian.test.ts b/tests/codex-integration/token-guardian.test.ts index 7c8e09ec85..b229872d8a 100644 --- a/tests/codex-integration/token-guardian.test.ts +++ b/tests/codex-integration/token-guardian.test.ts @@ -199,6 +199,85 @@ describe("token guardian", () => { expect(readCodexAccountRecord("acct-warm")?.lastCodexValidatedAt).toBeGreaterThan(Date.now() - 30_000); }); + // #4120: the pool branch used to compute `permanent` and then spend it only on the in-memory + // backoff delay. A revoked grant is the strongest terminal evidence available and was the one + // class excluded from the persisted verdict, so the record kept its login-time "ok" forever. + test("a revoked refresh grant persists a terminal verdict even with warmup disabled", async () => { + writeConfig({ + tokenGuardian: { enabled: true, tickSeconds: 60, leadSeconds: 60 }, + providers: { openai: { adapter: "openai-responses", baseUrl: "https://chatgpt.com/backend-api/codex", authMode: "forward", codexAccountMode: "pool", refreshPolicy: "proactive" } }, + }); + saveCodexAccountCredential("acct-revoked", { + accessToken: "old", refreshToken: "rt", expiresAt: Date.now() + 5_000, chatgptAccountId: "cg-1", + }); + markCodexAccountValidated("acct-revoked", Date.now() - 120_000); + globalThis.fetch = (async () => new Response(JSON.stringify({ error: "invalid_grant" }), { + status: 400, headers: { "content-type": "application/json" }, + })) as typeof fetch; + + const res = await guardianSweep(Date.now()); + + expect(res.failed).toContain("codex:acct-revoked"); + const record = readCodexAccountRecord("acct-revoked"); + expect(record?.lastCodexValidationStatus).toBe("failed"); + expect(record?.lastCodexValidationTerminal).toBe(true); + expect(record?.lastCodexValidationError).toBe("refresh_revoked"); + }); + + test("a transient refresh failure leaves the stored verdict untouched", async () => { + writeConfig({ + tokenGuardian: { enabled: true, tickSeconds: 60, leadSeconds: 60 }, + providers: { openai: { adapter: "openai-responses", baseUrl: "https://chatgpt.com/backend-api/codex", authMode: "forward", codexAccountMode: "pool", refreshPolicy: "proactive" } }, + }); + saveCodexAccountCredential("acct-transient", { + accessToken: "old", refreshToken: "rt", expiresAt: Date.now() + 5_000, chatgptAccountId: "cg-1", + }); + markCodexAccountValidated("acct-transient", Date.now() - 120_000); + globalThis.fetch = (async () => new Response(JSON.stringify({ error: "server_error" }), { + status: 500, headers: { "content-type": "application/json" }, + })) as typeof fetch; + + const res = await guardianSweep(Date.now()); + + expect(res.failed).toContain("codex:acct-transient"); + const record = readCodexAccountRecord("acct-transient"); + // An upstream blip is not evidence that the grant is dead, and warmup is off, so nothing + // about the recorded verdict may move. + expect(record?.lastCodexValidationStatus).toBe("ok"); + expect(record?.lastCodexValidationTerminal).toBeUndefined(); + }); + + test("a credential replaced mid-refresh is not branded by the previous credential's failure", async () => { + writeConfig({ + tokenGuardian: { enabled: true, tickSeconds: 60, leadSeconds: 60 }, + providers: { openai: { adapter: "openai-responses", baseUrl: "https://chatgpt.com/backend-api/codex", authMode: "forward", codexAccountMode: "pool", refreshPolicy: "proactive" } }, + }); + saveCodexAccountCredential("acct-replaced", { + accessToken: "old", refreshToken: "rt", expiresAt: Date.now() + 5_000, chatgptAccountId: "cg-1", + }); + markCodexAccountValidated("acct-replaced", Date.now() - 120_000); + const staleGeneration = readCodexAccountRecord("acct-replaced")!.generation; + + // Stand in for an operator re-authenticating the account while the sweep's refresh is in + // flight: the replacement lands before upstream answers with a dead grant. + globalThis.fetch = (async () => { + saveCodexAccountCredential("acct-replaced", { + accessToken: "reauthed", refreshToken: "rt-2", expiresAt: Date.now() + 3600_000, chatgptAccountId: "cg-1", + }); + return new Response(JSON.stringify({ error: "invalid_grant" }), { + status: 400, headers: { "content-type": "application/json" }, + }); + }) as typeof fetch; + + await guardianSweep(Date.now()); + + const record = readCodexAccountRecord("acct-replaced")!; + expect(record.generation).toBeGreaterThan(staleGeneration); + expect(record.credential?.accessToken).toBe("reauthed"); + expect(record.lastCodexValidationTerminal).toBeUndefined(); + expect(record.lastCodexValidationStatus).toBe("ok"); + }); + test("direct mode warms main only and never enumerates the added-account store", async () => { const accountStore = join(tmp, "ocx", "codex-accounts.json"); writeFileSync(accountStore, "invalid-added-store"); diff --git a/tests/oauth/oauth-health.test.ts b/tests/oauth/oauth-health.test.ts index 1f04fbc996..a38343b108 100644 --- a/tests/oauth/oauth-health.test.ts +++ b/tests/oauth/oauth-health.test.ts @@ -8,6 +8,7 @@ import { collectOAuthHealthEntries, collectOAuthHealthEntriesForCli, projectOAuthAccountHealth, + projectCodexAccountHealth, } from "../../src/oauth/health"; import { getAccountSet, markAccountNeedsReauth, saveCredential } from "../../src/oauth/store"; import { @@ -107,6 +108,71 @@ describe("projectOAuthAccountHealth", () => { }); }); +describe("projectCodexAccountHealth", () => { + /** + * The suite's beforeEach only creates the home root. Every other case in this file reads the + * Codex store, and a read of a missing file is a clean empty store; these cases WRITE it, and + * the credential mutation lock opens a SQLite file inside the config dir. + */ + function withPoolStoreDir(): void { + mkdirSync(join(tmp, "ocx"), { recursive: true }); + } + + test("a terminal validation verdict is projected as reauth_required", async () => { + withPoolStoreDir(); + const { markCodexAccountValidationFailed, readCodexAccountRecord, saveCodexAccountCredential } = + await import("../../src/codex/account-store"); + saveCodexAccountCredential("pool-revoked", { + accessToken: "a", refreshToken: "r", expiresAt: Date.now() + 3_600_000, chatgptAccountId: "cg", + }); + + // Before the verdict is recorded this is exactly the reported bug: a credential upstream has + // revoked still projects healthy (#4120). + expect(projectCodexAccountHealth({ accountId: "pool-revoked", needsReauth: false })) + .toEqual({ status: "healthy" }); + + markCodexAccountValidationFailed("pool-revoked", "refresh_revoked", { + expectedGeneration: readCodexAccountRecord("pool-revoked")!.generation, + terminal: true, + }); + + expect(projectCodexAccountHealth({ accountId: "pool-revoked", needsReauth: false })) + .toEqual({ status: "reauth_required", reason: "refresh_failed" }); + }); + + test("a non-terminal validation failure does not claim the credential is dead", async () => { + withPoolStoreDir(); + const { markCodexAccountValidationFailed, saveCodexAccountCredential } = + await import("../../src/codex/account-store"); + saveCodexAccountCredential("pool-warmup", { + accessToken: "a", refreshToken: "r", expiresAt: Date.now() + 3_600_000, chatgptAccountId: "cg", + }); + markCodexAccountValidationFailed("pool-warmup", "http_status:500"); + + expect(projectCodexAccountHealth({ accountId: "pool-warmup", needsReauth: false })) + .toEqual({ status: "healthy" }); + }); + + test("the CLI collector reports the terminal verdict too", async () => { + withPoolStoreDir(); + const { markCodexAccountValidationFailed, readCodexAccountRecord, saveCodexAccountCredential } = + await import("../../src/codex/account-store"); + saveCodexAccountCredential("pool-cli", { + accessToken: "a", refreshToken: "r", expiresAt: Date.now() + 3_600_000, chatgptAccountId: "cg", + }); + markCodexAccountValidationFailed("pool-cli", "refresh_expired", { + expectedGeneration: readCodexAccountRecord("pool-cli")!.generation, + terminal: true, + }); + + // collectLocalCodexEntries used to inline its own copy of the projector, which is how + // `ocx status`/`ocx doctor` would have kept calling this account healthy. + const entry = collectOAuthHealthEntries().find(e => e.provider === "codex" && e.accountId === "pool-cli"); + expect(entry?.health).toEqual({ status: "reauth_required", reason: "refresh_failed" }); + expect(entry?.action).toBe(CODEX_REAUTH_ACTION); + }); +}); + describe("collectOAuthHealthEntries", () => { test("projects needsReauth account with reauth action", async () => { await saveCredential("kimi", {