From 8ef2880d7619e851d6a6efa30d2afeacb8b83bf0 Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 21:18:31 +0900 Subject: [PATCH 1/7] fix(codex): classify a refresh failure on the structured code, not terminal prose A transient token-endpoint failure could still quarantine a healthy account. The verdict at src/codex/account-store.ts matched "invalidated", "revoked" and "expired" anywhere in the combined code+description text, unconditionally, so a `server_error` whose description merely said "the token was revoked" was retired as terminal -- the false quarantine #2887 exists to prevent, reached through the description rather than through the code. The comment directly above it already required the exact structured code; only invalid_grant was actually held to it. A body that carries a structured `error` code is now classified by that code alone. The substring fallback survives only where no structured code exists at all -- an `error_description`-only body, or one the parser could not decode -- because there the prose is the only signal upstream gave us, and removing it would leave a genuinely dead grant retrying forever. Each terminal word is pinned with its own negative case in a new file: the existing coverage only pinned a description mentioning invalid_grant, which the exact-code check already handled, so the three words were unguarded. --- scripts/test-layout/layout.json | 1 + src/codex/account-store.ts | 22 ++- ...count-store-refresh-classification.test.ts | 152 ++++++++++++++++++ tests/fixtures/test-layout-expected.json | 1 + 4 files changed, 171 insertions(+), 5 deletions(-) create mode 100644 tests/codex-integration/codex-account-store-refresh-classification.test.ts diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index 283692139cc..42fed868ff1 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -427,6 +427,7 @@ "codex-account-mode-state.test.ts": "gui", "codex-account-namespaces.test.ts": "codex-integration", "codex-account-selection-preferences.test.ts": "codex-integration", + "codex-account-store-refresh-classification.test.ts": "codex-integration", "codex-account-store.test.ts": "codex-integration", "codex-account-unusable-reason.test.ts": "codex-integration", "codex-admission-primitives.test.ts": "codex-integration", diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index feaec9f5c0e..150f4b52759 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -1205,11 +1205,23 @@ async function resolveCodexToken( // Matched on the exact `error` CODE, not anywhere in the combined text: a transient // `server_error` whose description happens to mention invalid_grant would otherwise // retire a healthy account, which is the failure this whole change exists to remove. - const reason = errCodeExact === "invalid_grant" - || errCodeExact === "refresh_token_invalidated" - || errDesc.includes("invalidated") || errDesc.includes("revoked") ? "revoked" as const - : errCodeExact === "refresh_token_expired" - || errDesc.includes("expired") ? "expired" as const + // + // That rule binds the DESCRIPTION words too. "invalidated", "revoked" and "expired" read + // as terminal prose, but upstream puts arbitrary text there: a `server_error` whose + // description says "token was revoked" or "session expired" is still a 5xx blip, and + // retiring the account on it is exactly the false quarantine #2887 exists to prevent. + // So a body that carries a structured code is classified by that code ALONE. The + // substring fallback survives only where there is no structured code to read at all -- + // a description-only body, or one this parser could not decode -- because there the + // prose is the only signal upstream gave us. + const structuredCode = errCodeExact ? errCodeExact : undefined; + const proseIsOnlySignal = structuredCode === undefined; + const reason = structuredCode === "invalid_grant" + || structuredCode === "refresh_token_invalidated" + || (proseIsOnlySignal + && (errDesc.includes("invalidated") || errDesc.includes("revoked"))) ? "revoked" as const + : structuredCode === "refresh_token_expired" + || (proseIsOnlySignal && errDesc.includes("expired")) ? "expired" as const : "unknown" as const; throw new TokenRefreshError(reason, `Codex token refresh failed (${reason}); reauthenticate the account.`); } diff --git a/tests/codex-integration/codex-account-store-refresh-classification.test.ts b/tests/codex-integration/codex-account-store-refresh-classification.test.ts new file mode 100644 index 00000000000..934d9ca5e78 --- /dev/null +++ b/tests/codex-integration/codex-account-store-refresh-classification.test.ts @@ -0,0 +1,152 @@ +import { describe, expect, test, beforeEach, afterEach } from "bun:test"; +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { setAsyncIcaclsRunnerForTests, setIcaclsRunnerForTests } from "../../src/lib/windows-secret-acl"; +import { flushConfigDirHardeningForTests } from "../../src/config/paths"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; + +/** + * Refresh-failure classification: which upstream bodies are allowed to retire an account. + * + * The rule the runtime states is that a terminal verdict comes from the exact structured + * `error` code. The terminal WORDS were never held to it: "invalidated", "revoked" and + * "expired" were matched anywhere in the combined code+description text, so a transient + * `server_error` whose description happened to say "the token was revoked" retired a healthy + * account -- the false quarantine #2887 exists to prevent, reached through the description + * instead of through the code. + * + * Every terminal word is pinned here with its own negative case. These live in their own file + * rather than in codex-account-store.test.ts because that file is 1.8k lines and each case + * needs the same scratch-home isolation the parent file installs. + */ + +let TEST_DIR = ""; + +const ICACLS_OK = { success: true, exitCode: 0, timedOut: false, stdout: "" }; + +describe("codex refresh-failure classification", () => { + beforeEach(() => { + // Credential-store behavior, not Windows ACL behavior: stub both runners so hardening + // never spawns icacls.exe. + setIcaclsRunnerForTests(() => ICACLS_OK); + setAsyncIcaclsRunnerForTests(async () => ICACLS_OK); + TEST_DIR = mkdtempSync(join(tmpdir(), "ocx-codex-refresh-class-")); + process.env.OPENCODEX_HOME = TEST_DIR; + }); + + afterEach(async () => { + await flushConfigDirHardeningForTests(); + setIcaclsRunnerForTests(null); + setAsyncIcaclsRunnerForTests(null); + delete process.env.OPENCODEX_HOME; + if (TEST_DIR) removeTreeWithRetry(TEST_DIR); + TEST_DIR = ""; + }); + + /** Drive one forced refresh against a stubbed token endpoint and return the thrown reason. */ + async function classify(accountId: string, respond: () => Response): Promise { + const { forceRefreshCodexPoolToken, readCodexAccountRecord, saveCodexAccountCredential, TokenRefreshError } = + await import("../../src/codex/account-store"); + saveCodexAccountCredential(accountId, { + accessToken: "rejected", + refreshToken: `grant-${accountId}`, + expiresAt: Date.now() + 3600_000, + chatgptAccountId: "acc", + }); + const generation = readCodexAccountRecord(accountId)!.generation; + const originalFetch = globalThis.fetch; + globalThis.fetch = (async () => respond()) as typeof fetch; + try { + await forceRefreshCodexPoolToken(accountId, { + rejectedGeneration: generation, + rejectedAccessToken: "rejected", + }); + throw new Error("expected a TokenRefreshError"); + } catch (error) { + expect(error).toBeInstanceOf(TokenRefreshError); + return (error as InstanceType).reason; + } finally { + globalThis.fetch = originalFetch; + } + } + + // --- negative cases: a structured code that is not terminal wins over terminal prose --- + + test("a server_error whose description says the token was revoked stays transient", async () => { + const reason = await classify("prose-revoked", () => Response.json({ + error: "server_error", + error_description: "upstream reported the refresh token was revoked; retry shortly", + }, { status: 503 })); + expect(reason).toBe("unknown"); + }); + + test("a server_error whose description says the grant was invalidated stays transient", async () => { + const reason = await classify("prose-invalidated", () => Response.json({ + error: "server_error", + error_description: "a peer cache entry was invalidated while refreshing", + }, { status: 503 })); + expect(reason).toBe("unknown"); + }); + + test("a server_error whose description says the session expired stays transient", async () => { + const reason = await classify("prose-expired", () => Response.json({ + error: "server_error", + error_description: "the upstream session expired mid-request; try again", + }, { status: 503 })); + expect(reason).toBe("unknown"); + }); + + test("a nested error object with a transient code and terminal prose stays transient", async () => { + // The nested shape carries the code in `error.code`, and its `message` is the same + // free-text field: reading the message as proof is the same defect in the other shape. + const reason = await classify("nested-prose", () => Response.json({ + error: { + code: "server_error", + message: "Your session has expired and the token was revoked.", + type: "server_error", + param: null, + }, + }, { status: 503 })); + expect(reason).toBe("unknown"); + }); + + test("an unrelated OAuth code with terminal prose stays transient", async () => { + // `invalid_request` is a client-shape complaint, not a statement about the grant. + const reason = await classify("unrelated-code", () => Response.json({ + error: "invalid_request", + error_description: "refresh_token was invalidated by an unknown parameter", + }, { status: 400 })); + expect(reason).toBe("unknown"); + }); + + // --- positive cases: the exact codes still retire, and prose still speaks when alone --- + + test("the exact terminal codes still classify as terminal", async () => { + expect(await classify("code-invalid-grant", () => Response.json({ error: "invalid_grant" }, { status: 400 }))) + .toBe("revoked"); + expect(await classify("code-invalidated", () => Response.json({ + error: { code: "refresh_token_invalidated", message: "Your session has ended." }, + }, { status: 401 }))).toBe("revoked"); + expect(await classify("code-expired", () => Response.json({ + error: { code: "refresh_token_expired", message: "The refresh token has expired." }, + }, { status: 401 }))).toBe("expired"); + }); + + test("with no structured code at all the description is still the only signal there is", async () => { + // A body carrying only `error_description` gives the classifier nothing else to read, so + // the substring fallback survives exactly there -- removing it would regress the opposite + // direction and leave a genuinely dead grant retrying forever. + expect(await classify("desc-only-revoked", () => Response.json({ + error_description: "refresh token revoked", + }, { status: 400 }))).toBe("revoked"); + expect(await classify("desc-only-expired", () => Response.json({ + error_description: "refresh token expired", + }, { status: 400 }))).toBe("expired"); + }); + + test("an unparseable body carries no terminal evidence and stays transient", async () => { + const reason = await classify("unparseable", () => new Response("502 revoked", { status: 502 })); + expect(reason).toBe("unknown"); + }); +}); diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index c585d827cdc..658581a4fc9 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -261,6 +261,7 @@ "codex-account-mode-state.test.ts": "gui", "codex-account-namespaces.test.ts": "codex-integration", "codex-account-selection-preferences.test.ts": "codex-integration", + "codex-account-store-refresh-classification.test.ts": "codex-integration", "codex-account-store.test.ts": "codex-integration", "codex-account-unusable-reason.test.ts": "codex-integration", "codex-admission-primitives.test.ts": "codex-integration", From 5e9c64499b3a1230a34ac7a1245a26adc2aa6ebf Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 21:20:17 +0900 Subject: [PATCH 2/7] perf(codex): resolve credential identity once per denial pass, not per cache entry `cachedDeniedCodexAccountIdsForModel` is synchronous and on the request path for the flagship models, and it resolved a credential identity for every cached (account, client version) entry. `readCodexAccountRecord` reloads, reparses and renormalizes the whole `codex-accounts.json` per call, so with the cache at its documented budget -- 64 accounts at MODEL_ROSTER_CACHE_MAX, four versions each at MODEL_ROSTER_VERSIONS_PER_ACCOUNT_MAX -- one warm request could perform up to 256 synchronous full-store reads before it chose an account. The read is hoisted, not the check. `credentialIdentityResolver` applies the same prefix rule, the same tombstone and missing-credential rejection and the same `pool::` shape; it just loads each backing store at most once per pass, lazily, so an excluded account or an all-Direct pass still opens nothing. The loop has no suspension point, so a per-pass snapshot is no staler than per-entry reads and is strictly more coherent: a foreign writer landing mid-loop can no longer hand earlier entries one generation and later ones another. `loadCodexAccountRecordSnapshot` is the new account-store export this needs. `loadCodexAccountStore` could not serve it -- it drops tombstones and `generation`, which are exactly what the identity is made of. --- src/codex/account-store.ts | 17 ++++++++ src/codex/model-entitlements.ts | 73 ++++++++++++++++++++++++--------- 2 files changed, 71 insertions(+), 19 deletions(-) diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index 150f4b52759..60f58ffdc9d 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -268,6 +268,23 @@ export function readCodexAccountRecord(id: string): CodexAccountCredentialRecord return loadCodexAccountRecordStore()[id] ?? null; } +/** + * One store load, every record, for a caller that resolves MANY ids in a single synchronous pass. + * + * `readCodexAccountRecord` reloads, reparses and renormalizes the whole file per id. That is the + * right shape for one lookup and the wrong shape for a loop: the entitlement denial reader holds + * up to 64 accounts with four client versions each, so scoring one warm flagship request could + * perform up to 256 full-store reads on the request path. + * + * These are the same normalized records `readCodexAccountRecord` hands out, tombstones included, + * so the caller keeps its own `deletedAt` and `generation` checks instead of trusting a filtered + * view. That is the difference from `loadCodexAccountStore`, which drops both and cannot answer a + * question about credential generation. + */ +export function loadCodexAccountRecordSnapshot(): Readonly { + return loadCodexAccountRecordStore(); +} + const QUOTA_HISTORY_IDENTITY_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/; function validQuotaHistoryIdentity(value: unknown): value is string { diff --git a/src/codex/model-entitlements.ts b/src/codex/model-entitlements.ts index 15ea05cb793..575aa8defa0 100644 --- a/src/codex/model-entitlements.ts +++ b/src/codex/model-entitlements.ts @@ -1,8 +1,8 @@ import { createHash } from "node:crypto"; import { readBoundedResponseBody } from "../lib/bounded-body"; -import type { OcxConfig } from "../types"; +import type { CodexAccountCredentialRecord, OcxConfig } from "../types"; import { isSelectableCodexPoolAccount } from "./account-id"; -import { getValidCodexToken, readCodexAccountRecord } from "./account-store"; +import { getValidCodexToken, loadCodexAccountRecordSnapshot } from "./account-store"; import { getMainAccountToken, getValidMainAccountToken, @@ -523,17 +523,48 @@ function boundedCacheSet(accountId: string, value: CachedAccountModels): void { evictClass(accountId.startsWith(DIRECT_CALLER_ACCOUNT_PREFIX)); } +/** + * An identity resolver scoped to one caller's pass, reading each backing store at most once. + * + * The identity check itself is unchanged -- same prefix rule, same tombstone and missing-credential + * rejection, same `pool::` shape -- but the READ is hoisted. Per-id + * resolution reloads and reparses the whole `codex-accounts.json` every call, so a loop over cache + * entries paid one full-store read per entry: the denial reader admits 64 accounts with four client + * versions each, which is up to 256 synchronous reads to score a single warm flagship request. + * + * Both stores are read lazily, so a pass that touches only Direct callers, or only native main, + * still opens nothing it does not need. Neither backing read is memoized across passes: a resolver + * lives for one synchronous loop, and that loop has no suspension point, so nothing this process + * does can change the file underneath it. A snapshot is therefore not staler than per-entry reads + * would have been -- it is strictly more coherent, because a foreign writer landing mid-loop can no + * longer give the earlier entries one generation and the later ones another. + */ +function credentialIdentityResolver(): (accountId: string) => string | undefined { + let records: Readonly> | undefined; + let mainRead = false; + let mainIdentity: string | undefined; + return (accountId: string): string | undefined => { + if (accountId.startsWith(DIRECT_CALLER_ACCOUNT_PREFIX)) { + return `direct:${accountId.slice(DIRECT_CALLER_ACCOUNT_PREFIX.length)}`; + } + if (accountId === MAIN_CODEX_ACCOUNT_ID) { + if (!mainRead) { + const token = getMainAccountToken(); + mainIdentity = token ? `main:${token.chatgptAccountId}` : undefined; + mainRead = true; + } + return mainIdentity; + } + records ??= loadCodexAccountRecordSnapshot(); + const record = records[accountId]; + if (!record?.credential || record.deletedAt != null) return undefined; + return `pool:${record.generation}:${record.credential.chatgptAccountId}`; + }; +} + +/** Single-id resolution. Identical to one call through a fresh {@link credentialIdentityResolver}. */ function currentCredentialIdentity(accountId: string): string | undefined { - if (accountId.startsWith(DIRECT_CALLER_ACCOUNT_PREFIX)) { - return `direct:${accountId.slice(DIRECT_CALLER_ACCOUNT_PREFIX.length)}`; - } - if (accountId === MAIN_CODEX_ACCOUNT_ID) { - const token = getMainAccountToken(); - return token ? `main:${token.chatgptAccountId}` : undefined; - } - const record = readCodexAccountRecord(accountId); - if (!record?.credential || record.deletedAt != null) return undefined; - return `pool:${record.generation}:${record.credential.chatgptAccountId}`; + return credentialIdentityResolver()(accountId); } async function accountCredentialSnapshot( @@ -1214,16 +1245,20 @@ export function cachedDeniedCodexAccountIdsForModel( if (!modelId || !ENTITLEMENT_PREFERRED_NATIVE_OPENAI_MODELS.has(modelId)) return undefined; const denied = new Set(); const granted = new Set(); + // One resolver for the whole pass: the loop below runs once per cached (account, client version) + // entry, and resolving an identity per entry meant a full account-store read per entry. + const identityOf = credentialIdentityResolver(); for (const [key, entry] of accountModelsCache) { const accountId = accountIdOfCacheKey(key); // A forwarded Direct credential is one request's caller, never a pool candidate. if (accountId.startsWith(DIRECT_CALLER_ACCOUNT_PREFIX)) continue; - // The caller's read fence, honoured BEFORE `currentCredentialIdentity` below, because that - // is the read: for native main it resolves the physical stored token, once per cached client - // version. A request that is forbidden to read main -- a profile switch draining it, or a - // request-owned credential that owns no main state -- must not reread account storage just to - // score an ordering preference. Dropping the account leaves it UNKNOWN rather than denied, - // which is the same outcome as having no cached roster for it and changes no selection. + // The caller's read fence, honoured BEFORE `identityOf` below, because that is the read: for + // native main it resolves the physical stored token. A request that is forbidden to read main + // -- a profile switch draining it, or a request-owned credential that owns no main state -- + // must not reread account storage just to score an ordering preference. Dropping the account + // leaves it UNKNOWN rather than denied, which is the same outcome as having no cached roster + // for it and changes no selection. The resolver reads lazily for the same reason: an excluded + // account `continue`s here, so its store is never opened at all. if (options.excludeAccountIds?.has(accountId)) continue; if (entry.expiresAt <= now) continue; // A credential we can currently read AND that differs is proof the entry answers for a @@ -1231,7 +1266,7 @@ export function cachedDeniedCodexAccountIdsForModel( // one. An UNREADABLE credential is not proof of anything, and the same unknown-is-not-denied // discipline that governs rosters governs identities: it leaves the entry in place rather // than manufacturing a reason to ignore it. - const identity = currentCredentialIdentity(accountId); + const identity = identityOf(accountId); if (identity !== undefined && identity !== entry.credentialIdentity) continue; const state = codexModelEntitlementStateForRoster( entry.models, From 1a7882dc6b988d60227586a77c386c11076db87d Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 21:23:02 +0900 Subject: [PATCH 3/7] fix(test): drop a duplicate account-store import that cannot parse `tests/codex-integration/codex-model-entitlements.test.ts` imported `readCodexAccountRecord` and `saveCodexAccountCredential` twice, in two separate declarations. Duplicate imported bindings are an early SyntaxError under ESM and TS2300 under `tsc --noEmit`, so the file cannot load and its entire suite -- including the denial-reader coverage the same commit added -- never runs. Introduced by 89bdf5fa4a, whose message records that no local suite or typecheck was run. Removing the second declaration restores the file; the surviving multi-line import already provides both names. --- tests/codex-integration/codex-model-entitlements.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/codex-integration/codex-model-entitlements.test.ts b/tests/codex-integration/codex-model-entitlements.test.ts index 56ef19433a4..18598f0ee5a 100644 --- a/tests/codex-integration/codex-model-entitlements.test.ts +++ b/tests/codex-integration/codex-model-entitlements.test.ts @@ -37,7 +37,6 @@ import { import { clearCodexRuntimeResolveCache, loadPersistedCodexRuntime } from "../../src/codex/runtime"; import { ACCOUNT_GATED_NATIVE_OPENAI_MODELS } from "../../src/codex/catalog/native-models"; import upstreamModelsSnapshot from "../../src/codex/data/upstream-models.json"; -import { readCodexAccountRecord, saveCodexAccountCredential } from "../../src/codex/account-store"; import { installIsolatedCodexHome } from "../helpers/isolated-codex-home"; import { removeTreeWithRetry } from "../helpers/remove-tree"; import { repoPath } from "../helpers/repo-root"; From f586568b3177f2fc5c24f9571658d11b86e2d787 Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 21:23:03 +0900 Subject: [PATCH 4/7] test(codex): pin the denial pass to one account-store read The read count is invisible to every behavioral assertion, so nothing stopped it regressing back to one full-store reload per cache entry. These cases assert it directly by counting `readFileSync` against `codex-accounts.json` across one `cachedDeniedCodexAccountIdsForModel` call: six entries over three accounts cost one read, and a pass whose every entry is fenced out costs none, which is what proves the resolver stayed lazy rather than merely shared. The validation the count must not have bought is asserted beside it: a roster recorded under a superseded identity is still rejected, and an account with no stored record is still left unknown rather than dropped. New file rather than an addition to codex-model-entitlements.test.ts, which is already past 1.9k lines, and registered in both layout maps. --- scripts/test-layout/layout.json | 1 + ...ex-entitlement-identity-read-fence.test.ts | 151 ++++++++++++++++++ tests/fixtures/test-layout-expected.json | 1 + 3 files changed, 153 insertions(+) create mode 100644 tests/codex-integration/codex-entitlement-identity-read-fence.test.ts diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index 42fed868ff1..75b2d7ed415 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -460,6 +460,7 @@ "codex-cooldown-recovery.test.ts": "codex-integration", "codex-coordinator-doctor.test.ts": "codex-integration", "codex-desired-state.test.ts": "codex-integration", + "codex-entitlement-identity-read-fence.test.ts": "codex-integration", "codex-envkey-admission-substitution.test.ts": "codex-integration", "codex-exec-invocation.test.ts": "codex-integration", "codex-features-cache.test.ts": "codex-integration", diff --git a/tests/codex-integration/codex-entitlement-identity-read-fence.test.ts b/tests/codex-integration/codex-entitlement-identity-read-fence.test.ts new file mode 100644 index 00000000000..1c1cc8cb3ae --- /dev/null +++ b/tests/codex-integration/codex-entitlement-identity-read-fence.test.ts @@ -0,0 +1,151 @@ +import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; +import * as fs from "node:fs"; +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + cachedDeniedCodexAccountIdsForModel, + resetCodexModelEntitlementCacheForTests, + seedCodexModelEntitlementsForTests, +} from "../../src/codex/model-entitlements"; +import { readCodexAccountRecord, saveCodexAccountCredential } from "../../src/codex/account-store"; +import { setAsyncIcaclsRunnerForTests, setIcaclsRunnerForTests } from "../../src/lib/windows-secret-acl"; +import { flushConfigDirHardeningForTests } from "../../src/config/paths"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; + +/** + * How many times one denial pass reads account storage. + * + * `cachedDeniedCodexAccountIdsForModel` is synchronous and runs on the request path for the + * flagship models, and it validates every cached (account, client version) entry against the + * account's current credential. Resolving that identity per entry meant a full reload, reparse and + * renormalize of `codex-accounts.json` per entry: at the documented cache budget -- 64 accounts, + * four versions each -- one warm request could perform 256 synchronous full-store reads. + * + * These cases pin the read COUNT, which no behavioral assertion can see, alongside the validation + * the count must not have bought: a stale identity is still rejected, and an excluded account still + * opens nothing. + */ + +const ASTRA = "gpt-6-astra"; +const VERSION_A = "0.146.0"; +const VERSION_B = "0.147.0"; +const NOW = 1_800_000_000_000; + +let TEST_DIR = ""; + +const ICACLS_OK = { success: true, exitCode: 0, timedOut: false, stdout: "" }; + +/** Count reads of the pool account store, whoever performs them. */ +function countAccountStoreReads(): { reads: () => number; restore: () => void } { + const original = fs.readFileSync; + let reads = 0; + const spy = spyOn(fs, "readFileSync").mockImplementation(((...args: Parameters) => { + const target = args[0]; + if (typeof target === "string" && target.endsWith("codex-accounts.json")) reads += 1; + return original(...args); + }) as typeof fs.readFileSync); + return { reads: () => reads, restore: () => spy.mockRestore() }; +} + +/** Store a pool credential and return the identity string the reader will derive from it. */ +function storedIdentity(accountId: string): string { + saveCodexAccountCredential(accountId, { + accessToken: `access-${accountId}`, + refreshToken: `grant-${accountId}`, + expiresAt: NOW + 3600_000, + chatgptAccountId: `chatgpt-${accountId}`, + }); + const record = readCodexAccountRecord(accountId)!; + return `pool:${record.generation}:${record.credential!.chatgptAccountId}`; +} + +describe("the denial pass resolves credential identity once, not once per cache entry", () => { + beforeEach(() => { + setIcaclsRunnerForTests(() => ICACLS_OK); + setAsyncIcaclsRunnerForTests(async () => ICACLS_OK); + TEST_DIR = mkdtempSync(join(tmpdir(), "ocx-entitlement-read-fence-")); + process.env.OPENCODEX_HOME = TEST_DIR; + resetCodexModelEntitlementCacheForTests(); + }); + + afterEach(async () => { + await flushConfigDirHardeningForTests(); + setIcaclsRunnerForTests(null); + setAsyncIcaclsRunnerForTests(null); + delete process.env.OPENCODEX_HOME; + if (TEST_DIR) removeTreeWithRetry(TEST_DIR); + TEST_DIR = ""; + resetCodexModelEntitlementCacheForTests(); + }); + + test("six cache entries across three accounts cost one store read", () => { + // Each account carries two client versions, which is two cache entries and, before this, + // two full-store reads. + for (const accountId of ["pool-a", "pool-b", "pool-c"]) { + const identity = storedIdentity(accountId); + seedCodexModelEntitlementsForTests(accountId, ["gpt-5.5"], NOW, VERSION_A, identity); + seedCodexModelEntitlementsForTests(accountId, ["gpt-5.5"], NOW, VERSION_B, identity); + } + + const counter = countAccountStoreReads(); + try { + const denied = cachedDeniedCodexAccountIdsForModel(ASTRA, NOW); + // The answer is unchanged: every account's confirmed roster omits Astra. + expect([...(denied ?? [])].sort()).toEqual(["pool-a", "pool-b", "pool-c"]); + expect(counter.reads()).toBe(1); + } finally { + counter.restore(); + } + }); + + test("the snapshot does not weaken the identity check it answers from", () => { + // `stale` holds a roster recorded under a credential the account no longer has, so its denial + // is evidence about a different identity and must not count. `current` matches and must. + const staleIdentity = storedIdentity("stale"); + seedCodexModelEntitlementsForTests("stale", ["gpt-5.5"], NOW, VERSION_A, `${staleIdentity}-superseded`); + const currentIdentity = storedIdentity("current"); + seedCodexModelEntitlementsForTests("current", ["gpt-5.5"], NOW, VERSION_A, currentIdentity); + + const counter = countAccountStoreReads(); + try { + expect([...(cachedDeniedCodexAccountIdsForModel(ASTRA, NOW) ?? [])]).toEqual(["current"]); + expect(counter.reads()).toBe(1); + } finally { + counter.restore(); + } + }); + + test("an account with no stored record stays unknown rather than being rejected outright", () => { + // An UNREADABLE credential is not proof of anything. The store is read once and answers + // `undefined` for this id, which leaves the entry in place exactly as before. + seedCodexModelEntitlementsForTests("unstored", ["gpt-5.5"], NOW, VERSION_A, "test:unstored"); + storedIdentity("present-so-the-file-exists"); + + const counter = countAccountStoreReads(); + try { + expect([...(cachedDeniedCodexAccountIdsForModel(ASTRA, NOW) ?? [])]).toEqual(["unstored"]); + expect(counter.reads()).toBe(1); + } finally { + counter.restore(); + } + }); + + test("a pass whose every entry is excluded opens no store at all", () => { + // The resolver loads lazily for the same reason the fence is checked first: an excluded + // account must not cause a read it was excluded to prevent. + const identity = storedIdentity("fenced"); + seedCodexModelEntitlementsForTests("fenced", ["gpt-5.5"], NOW, VERSION_A, identity); + seedCodexModelEntitlementsForTests("fenced", ["gpt-5.5"], NOW, VERSION_B, identity); + + const counter = countAccountStoreReads(); + try { + expect(cachedDeniedCodexAccountIdsForModel(ASTRA, NOW, { + excludeAccountIds: new Set(["fenced"]), + })).toBeUndefined(); + expect(counter.reads()).toBe(0); + } finally { + counter.restore(); + } + }); +}); diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index 658581a4fc9..48d28b51002 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -294,6 +294,7 @@ "codex-cooldown-recovery.test.ts": "codex-integration", "codex-coordinator-doctor.test.ts": "codex-integration", "codex-desired-state.test.ts": "codex-integration", + "codex-entitlement-identity-read-fence.test.ts": "codex-integration", "codex-envkey-admission-substitution.test.ts": "codex-integration", "codex-exec-invocation.test.ts": "codex-integration", "codex-features-cache.test.ts": "codex-integration", From d515f732d9f149595850ad16c2cc6451fe0de2e7 Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 21:23:37 +0900 Subject: [PATCH 5/7] perf(codex): hoist the same identity read out of the other synchronous passes `ensureCodexEntitlementFreshness`'s prologue, `getCodexModelEntitlementStatus` and `isCodexModelEntitlementSnapshotCurrent` each resolve an identity per candidate inside one synchronous loop, which is the same full-store reload per iteration the denial pass had. They share the pass-scoped resolver now. The two remaining single-id call sites stay single-id deliberately: both live in async continuations after an upstream fetch, where the point of the read is that it is taken AFTER the await. A resolver there would answer from a snapshot taken before the suspension, which is the one place this hoist would weaken the check rather than leave it alone. `loadCodexAccountRecordSnapshot` names its record type through the imported `CodexAccountCredentialRecord` rather than the module-local `CodexAccountStore` alias, so the exported signature stays nameable under declaration checking. --- src/codex/account-store.ts | 2 +- src/codex/model-entitlements.ts | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index 60f58ffdc9d..f695884cf1e 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -281,7 +281,7 @@ export function readCodexAccountRecord(id: string): CodexAccountCredentialRecord * view. That is the difference from `loadCodexAccountStore`, which drops both and cannot answer a * question about credential generation. */ -export function loadCodexAccountRecordSnapshot(): Readonly { +export function loadCodexAccountRecordSnapshot(): Readonly> { return loadCodexAccountRecordStore(); } diff --git a/src/codex/model-entitlements.ts b/src/codex/model-entitlements.ts index 575aa8defa0..f52c9426231 100644 --- a/src/codex/model-entitlements.ts +++ b/src/codex/model-entitlements.ts @@ -960,8 +960,10 @@ export async function ensureCodexEntitlementFreshness( ); const candidates = normalizedCandidateAccountIds(config); const mutationEpoch = codexCredentialMutationEpoch(); + // Same hoist as the denial pass: this prologue is synchronous and reads once per candidate. + const identityOf = credentialIdentityResolver(); const identityEntries = candidates.map(accountId => ( - [accountId, currentCredentialIdentity(accountId) ?? null] as const + [accountId, identityOf(accountId) ?? null] as const )); const identityVector = new Map(identityEntries); const workset = candidates.filter(accountId => needsEntitlementRefresh( @@ -1014,8 +1016,9 @@ export function getCodexModelEntitlementStatus( clientVersion?: string | null, ): CodexModelEntitlementStatus { const version = resolveCodexEntitlementClientVersion(clientVersion); + const identityOf = credentialIdentityResolver(); const accounts = candidateAccountIds(config).flatMap(accountId => { - const credentialIdentity = currentCredentialIdentity(accountId); + const credentialIdentity = identityOf(accountId); return credentialIdentity ? [{ accountId, credentialIdentity }] : []; }); if (accounts.length === 0) return { status: "unavailable" }; @@ -1320,8 +1323,9 @@ export function cachedAvailableAccountGatedNativeModels( } export function isCodexModelEntitlementSnapshotCurrent(snapshot: CodexModelEntitlementSnapshot): boolean { + const identityOf = credentialIdentityResolver(); for (const [accountId, identity] of snapshot.credentialIdentities) { - if (currentCredentialIdentity(accountId) !== identity) return false; + if (identityOf(accountId) !== identity) return false; } return true; } From 00840c493f01eaa5c743925dc05e1551e8d7d917 Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 21:24:17 +0900 Subject: [PATCH 6/7] test(codex): type the read counter around readFileSync overloads `spyOn(fs, "readFileSync").mockImplementation` has to satisfy an overloaded target, which a single Parameters<>-typed signature does not. The counter only needs to observe the call and pass it through, so the pass-through is typed structurally and cast once. --- .../codex-entitlement-identity-read-fence.test.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/codex-integration/codex-entitlement-identity-read-fence.test.ts b/tests/codex-integration/codex-entitlement-identity-read-fence.test.ts index 1c1cc8cb3ae..6694a03d342 100644 --- a/tests/codex-integration/codex-entitlement-identity-read-fence.test.ts +++ b/tests/codex-integration/codex-entitlement-identity-read-fence.test.ts @@ -38,14 +38,17 @@ const ICACLS_OK = { success: true, exitCode: 0, timedOut: false, stdout: "" }; /** Count reads of the pool account store, whoever performs them. */ function countAccountStoreReads(): { reads: () => number; restore: () => void } { - const original = fs.readFileSync; + // `readFileSync` is heavily overloaded, so the pass-through is typed structurally and cast once + // rather than trying to satisfy every overload: this counts calls, it does not model the API. + const original = fs.readFileSync as (...args: unknown[]) => unknown; let reads = 0; - const spy = spyOn(fs, "readFileSync").mockImplementation(((...args: Parameters) => { + const spy = spyOn(fs, "readFileSync"); + spy.mockImplementation(((...args: unknown[]) => { const target = args[0]; if (typeof target === "string" && target.endsWith("codex-accounts.json")) reads += 1; return original(...args); - }) as typeof fs.readFileSync); - return { reads: () => reads, restore: () => spy.mockRestore() }; + }) as unknown as typeof fs.readFileSync); + return { reads: () => reads, restore: () => { spy.mockRestore(); } }; } /** Store a pool credential and return the identity string the reader will derive from it. */ From 9bfcb59b2653aa2a691c54486715a876e972b297 Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 21:33:01 +0900 Subject: [PATCH 7/7] fix(codex): keep the snapshot-current gate reading per account Adversarial review of the resolver hoist. `isCodexModelEntitlementSnapshotCurrent` is a fail-closed publication gate whose whole question is whether a snapshot is STILL current, so the freshest per-account answer is the point of the read. A pass-wide snapshot is a coherence win in the denial and candidate passes and a small weakening here: it could answer "current" for a later account from a record a concurrent reauth had already replaced. Reverted to per-id there and said why. The candidate-loop hoists stay. `ensureCodexEntitlementFreshness` explicitly captures its identity vector BEFORE the await and rechecks each id individually after it, and `getCodexModelEntitlementStatus` is a read-only management projection. The read counter now carries the in-process precedent for spying `node:fs` against an ESM named import -- codex-account-delete-atomicity.test.ts asserts a production read through the same seam -- and a calibration case, so the zero-expecting fence assertion can no longer pass by seeing nothing. --- src/codex/model-entitlements.ts | 8 +++++-- ...ex-entitlement-identity-read-fence.test.ts | 22 +++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/codex/model-entitlements.ts b/src/codex/model-entitlements.ts index f52c9426231..a770925f1d6 100644 --- a/src/codex/model-entitlements.ts +++ b/src/codex/model-entitlements.ts @@ -1323,9 +1323,13 @@ export function cachedAvailableAccountGatedNativeModels( } export function isCodexModelEntitlementSnapshotCurrent(snapshot: CodexModelEntitlementSnapshot): boolean { - const identityOf = credentialIdentityResolver(); for (const [accountId, identity] of snapshot.credentialIdentities) { - if (identityOf(accountId) !== identity) return false; + // Deliberately per-id, unlike the passes above. This is a fail-closed publication gate asking + // whether a snapshot is STILL current, so the freshest possible answer per account is the + // point of the read. A pass-wide snapshot would be a coherence win everywhere else and a + // small weakening here: it could answer "current" for a later account from a record a + // concurrent reauth had already replaced. + if (currentCredentialIdentity(accountId) !== identity) return false; } return true; } diff --git a/tests/codex-integration/codex-entitlement-identity-read-fence.test.ts b/tests/codex-integration/codex-entitlement-identity-read-fence.test.ts index 6694a03d342..d5daad4d20a 100644 --- a/tests/codex-integration/codex-entitlement-identity-read-fence.test.ts +++ b/tests/codex-integration/codex-entitlement-identity-read-fence.test.ts @@ -38,6 +38,13 @@ const ICACLS_OK = { success: true, exitCode: 0, timedOut: false, stdout: "" }; /** Count reads of the pool account store, whoever performs them. */ function countAccountStoreReads(): { reads: () => number; restore: () => void } { + // Spying the `node:fs` namespace DOES observe production code that binds `readFileSync` as an + // ESM named import, which is how `src/codex/account-store.ts` binds it. Established in-process + // precedent: codex-account-delete-atomicity.test.ts asserts `toHaveBeenLastCalledWith` against a + // read performed by production code, and codex-account-store.test.ts intercepts this module's own + // `statSync`/`fstatSync` the same way. The case that does NOT work is a spawned child holding its + // own `require("node:fs")` (codex-inject-integration.test.ts:146); nothing here spawns one, and + // the calibration case below fails loudly if that ever stops being true. // `readFileSync` is heavily overloaded, so the pass-through is typed structurally and cast once // rather than trying to satisfy every overload: this counts calls, it does not model the API. const original = fs.readFileSync as (...args: unknown[]) => unknown; @@ -151,4 +158,19 @@ describe("the denial pass resolves credential identity once, not once per cache counter.restore(); } }); + + test("the read counter observes production reads at all", () => { + // Calibration for the zero-expecting case above, which is indistinguishable from a counter + // that can see nothing. One `readCodexAccountRecord` is exactly one store read by + // construction, so this pins the oracle rather than the behavior under test. + storedIdentity("calibration"); + + const counter = countAccountStoreReads(); + try { + expect(readCodexAccountRecord("calibration")).not.toBeNull(); + expect(counter.reads()).toBe(1); + } finally { + counter.restore(); + } + }); });