Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions devlog/_plan/260909_codex_credential_health_chain/000_plan.md
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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".
49 changes: 45 additions & 4 deletions src/codex/account-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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;
});
}

Expand Down
Loading
Loading