From 5c74f76138422021dad8ab5b2c01f872e6eb7caf Mon Sep 17 00:00:00 2001 From: JUN Date: Thu, 10 Sep 2026 00:16:28 +0900 Subject: [PATCH 1/2] fix(codex): persist a terminal validation verdict for a revoked pool 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 --- src/codex/account-store.ts | 49 +++++++++++- src/oauth/health.ts | 41 +++++++--- src/oauth/token-guardian.ts | 27 ++++++- src/types/accounts.ts | 16 ++++ .../codex-account-store.test.ts | 70 ++++++++++++++++ .../codex-integration/token-guardian.test.ts | 79 +++++++++++++++++++ tests/oauth/oauth-health.test.ts | 66 ++++++++++++++++ 7 files changed, 330 insertions(+), 18 deletions(-) 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", { From 2156fbe7fc6f1f880388c944932aaf7111c53aa6 Mon Sep 17 00:00:00 2001 From: JUN Date: Thu, 10 Sep 2026 00:16:34 +0900 Subject: [PATCH 2/2] devlog: open the codex credential health chain unit 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. --- .../000_plan.md | 73 ++++++++++++++ .../010_wp1_terminal_verdict.md | 98 +++++++++++++++++++ .../020_wp2_quota_registration.md | 24 +++++ .../030_wp3_anthropic_plan.md | 23 +++++ 4 files changed, 218 insertions(+) create mode 100644 devlog/_plan/260909_codex_credential_health_chain/000_plan.md create mode 100644 devlog/_plan/260909_codex_credential_health_chain/010_wp1_terminal_verdict.md create mode 100644 devlog/_plan/260909_codex_credential_health_chain/020_wp2_quota_registration.md create mode 100644 devlog/_plan/260909_codex_credential_health_chain/030_wp3_anthropic_plan.md 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".