From c2c971c23ee0243828e2574fafb1893e6494ef72 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 19:32:59 +0900 Subject: [PATCH 1/6] fix(codex): ask upstream for the entitlement roster under a real client version The Codex entitlement roster was always requested as `/backend-api/codex/models?client_version=0.0.0`. Upstream version-filters that roster, so a placeholder version yields an empty or truncated answer, and the fail-closed gate from #2550 then read that as a confirmed negative and suppressed GPT-5.6 for accounts that genuinely own it (#2886). The version is now supplied by the caller, in precedence order: the inbound request's own `client_version` (Codex already sends it on /v1/models and the value was being discarded), then the selected Codex runtime version for background sync. Neither available means the roster is not requested at all and every account stays unconfirmed -- failing closed on absent evidence is the existing contract; failing closed on invented evidence was the defect. A cached roster now carries the version it was fetched under and participates in both the cache-hit check and the in-flight key, so one client's answer cannot satisfy another's question. Separately, model-visibility validation accepted only `nativeModelRows`, which a suppressed model is absent from -- so an operator could not clear the stale `disabledModels` key that the same bug had them set. NATIVE_OPENAI_MODELS is now unioned in. That is a config repair only: entitlement still filters the rows and routing stays gated. Closes #2886 --- ...0_issue_2886_entitlement_client_version.md | 138 ++++++++++++++++++ src/codex/model-entitlements.ts | 119 ++++++++++++++- src/server/index.ts | 5 +- src/server/management/model-routes.ts | 7 + tests/codex-model-entitlements.test.ts | 109 ++++++++++++++ tests/model-visibility-management-api.test.ts | 38 +++++ 6 files changed, 409 insertions(+), 7 deletions(-) create mode 100644 devlog/_plan/260829_bugpr_lane_h_residual_issues/160_issue_2886_entitlement_client_version.md diff --git a/devlog/_plan/260829_bugpr_lane_h_residual_issues/160_issue_2886_entitlement_client_version.md b/devlog/_plan/260829_bugpr_lane_h_residual_issues/160_issue_2886_entitlement_client_version.md new file mode 100644 index 0000000000..a6fe0f2a61 --- /dev/null +++ b/devlog/_plan/260829_bugpr_lane_h_residual_issues/160_issue_2886_entitlement_client_version.md @@ -0,0 +1,138 @@ +# 160 — issue #2886: entitled GPT-5.6 Sol/Terra/Luna vanish from the native catalog + +## What the reporter saw + +A healthy ChatGPT Plus account that can demonstrably use `gpt-5.6-sol` — native Codex +routing shows it, a fresh Sol conversation completes, and OpenCodex 2.33.0 advertises all +three — loses `gpt-5.6-sol`, `gpt-5.6-terra`, and `gpt-5.6-luna` from both +`ocx models live` and the Codex App picker after upgrading to 2.35.0. Re-enabling them +by hand fails with `invalid model visibility target`. + +The A/B is single-variable and includes a working control, so this is not a stale picker +cache. + +## The filtering is upstream, and this repository already measured it + +OpenCodex never compares `minimal_client_version` itself — it strips the field +(`src/codex/catalog/metadata.ts:502`, `src/codex/catalog/parsing.ts:486`), with a +regression pinning that at `tests/codex-catalog.test.ts:2737`. So no local filter is +dropping these rows; the roster arrives without them. + +A prior unit measured the endpoint directly +(`devlog/_fin/260817_native_gpt56_1m_context/001_measurement_evidence.md`): +`client_version` is a required query parameter, and the model count returned depends on +it — `0.60.0` yields **0** models, `0.142.2` yields **5**. + +Entitlement discovery asks for exactly `client_version=0.0.0` +(`src/codex/model-entitlements.ts:14`). It is asking upstream to describe what a +prehistoric client may use, and then treating the answer as what this account owns. + +`#2550` added the three slugs to `ACCOUNT_GATED_NATIVE_OPENAI_MODELS` +(`src/codex/catalog/native-models.ts:5-10`), which is correct policy for the inverse +report in `#2548`. Fail-closed is right when entitlement is unknown; the defect is that +the input was never a real entitlement answer. A valid `{models:[...]}` response sets +`confirmed: true` regardless of contents (`:133-143`, `:172-180`), availability requires +`confirmed && models.has(id)` (`:317-327`), and catalog sync then drops the rows +(`src/codex/catalog/sync.ts:1579-1596`). + +The account's plan is never consulted, and WHAM is a separate request +(`src/codex/auth-api.ts:767-785`) — `plan=plus, status=200` proves authentication, not +roster contents. The reporter's evidence and the code were measuring different things. + +## Where the real version comes from + +There is no existing outbound precedent to copy: native forwarding uses +`FORWARD_HEADERS` (`src/adapters/openai-responses.ts:35`), which carries neither +`user-agent` nor any version header, and the other `client_version: "0.0.0"` sites +(`src/codex/convergence.ts:476`, `src/codex/catalog/sync.ts:1988`) are local Codex cache +wrappers, not upstream requests. + +But the best source is already in hand for the path that matters. A live catalog request +arrives **from Codex**, carrying its own `client_version` query parameter, and the handler +already detects it (`src/server/index.ts:1173`) while calling entitlement discovery +without it (`:1073`). The value is right there and is thrown away. + +So version authority is a precedence chain, not a single lookup: + +1. **The inbound request's `client_version`**, when the caller supplied one. This is the + only value that is certainly the version of the client being answered. +2. **The selected Codex runtime version** for background sync, where there is no inbound + request. `loadPersistedCodexRuntime()?.selectedVersion` (`src/codex/runtime.ts:256`) + performs no freshness validation and the file is written only by runtime-selection paths + (`:621`), so it can be absent after a persist failure, stale before selection runs, or + describe the binary OpenCodex chose rather than an externally launched client. Retained + sync does refresh runtime evidence first (`src/codex/catalog/sync.ts:1828`), which is + what makes it usable here and not elsewhere. +3. **Neither available → do not ask.** Sending `0.0.0` manufactures a confirmed negative, + which is the whole defect. No trustworthy version means unconfirmed, and unconfirmed + already suppresses (`:321`) and routing already rejects + (`src/codex/auth-context.ts:458`). Fail closed on absent evidence, never on invented + evidence. + +This needs a real seam. `fetcher` (`:43`) can observe the URL but cannot choose the +version, so `resolveCodexModelEntitlements` and `isDirectCallerEntitledToCodexModel` +both take an explicit client version. + +## The cache has to be version-scoped + +`accountModelsCache` is keyed by account ID alone, with credential identity stored as a +discriminator (`:30`, `:216`); the flight key is account plus credential identity +(`:223`). Version must join both, or a roster fetched under one version keeps answering +for another until the TTL expires. + +`cachedAvailableAccountGatedNativeModels` scans every cache entry (`:331`). Once two +versions can be retained at once, that scan will leak a newer roster into an older +client's projection — the `#2548` failure, arrived at from the opposite direction. It has +to filter by the version being projected. + +`isCodexModelEntitlementSnapshotCurrent` validates credentials only (`:346`); a runtime +version change during a gather needs the same stale-result protection. + +## Sub-defect B, correctly scoped + +`ocx models enable gpt-5.6-sol` fails because `/api/model-visibility` builds +`supportedNative` from `nativeModelRows(config)` +(`src/server/management/model-routes.ts:461-468`), which has already dropped the +suppressed rows, so validation rejects at `:477-478`. + +Validating bare native IDs against the static `NATIVE_OPENAI_MODELS` set +(`src/codex/catalog/native-models.ts:69`) fixes that, **unioned with** the existing +account-qualified targets rather than replacing them. + +Being precise about what this buys: acceptance only clears `disabledModels` (`:532`). +Entitlement still filters `nativeModelRows` (`src/codex/catalog/metadata.ts:424`) and +routing stays gated. So B is **not** a manual escape from a false negative — the earlier +draft of this page claimed that and was wrong. B removes a misleading 400 and lets an +operator pre-clear an independent disable key. If no disable key exists, B changes nothing +the user can see. A is the fix; B is a UX and configuration repair that stops the CLI from +lying about why. + +## Verification + +**A** in `tests/codex-model-entitlements.test.ts` (fetch seam already exercised at +`tests/codex-model-entitlements.test.ts:38`): a mock backend that returns a legacy-only +roster below the threshold and the full roster at `0.146.0`. The wrong behavior asserted +is the real one — *an entitled account is classified as denying GPT-5.6 because OpenCodex +under-reports its own client version*. Named mutation: restore the `0.0.0` literal. + +A second case pins the precedence chain: with no trustworthy version, discovery must be +**unconfirmed** rather than a confirmed negative. Named mutation: fall back to `0.0.0`; +the account is then reported as positively denying the models. + +Cache identity gets its own case — fetch under one version, ask under another, assert a +re-fetch. Named mutation: drop version from the cache key. + +**B** in `tests/model-visibility-management-api.test.ts`: with `disabledModels: +["gpt-5.6-sol"]` and no entitlement cache, the PUT must be accepted and clear the entry, +specifically not returning `invalid model visibility target`. Named mutation: derive +`supportedNative` from `nativeModelRows` again. + +## What this does not claim + +The reporter supplied no captured `/codex/models` response, so I cannot prove their +machine took the confirmed-negative branch rather than a transient failure. Both produce +the same symptom. The version-filter explanation is what the source, the version boundary, +and this repository's own measurement support, and the fix is correct either way — but if +their roster was failing for another reason the models will still be missing afterwards, +and the issue should be reopened with a redacted capture rather than assumed fixed. + diff --git a/src/codex/model-entitlements.ts b/src/codex/model-entitlements.ts index d6744ac030..af52f345a1 100644 --- a/src/codex/model-entitlements.ts +++ b/src/codex/model-entitlements.ts @@ -10,8 +10,60 @@ import { type NativeMainRefreshDependencies, } from "./main-account"; import { ACCOUNT_GATED_NATIVE_OPENAI_MODELS } from "./catalog/native-models"; +import { loadPersistedCodexRuntime } from "./runtime"; -const CODEX_MODELS_URL = "https://chatgpt.com/backend-api/codex/models?client_version=0.0.0"; +const CODEX_MODELS_ENDPOINT = "https://chatgpt.com/backend-api/codex/models"; + +/** + * Upstream filters this roster by the client version it is told, and `client_version` is a + * required parameter — a measured `0.60.0` returns zero models where `0.142.2` returns five + * (devlog/_fin/260817_native_gpt56_1m_context/001_measurement_evidence.md). Asking as + * `0.0.0` therefore describes what a prehistoric client may use, and treating that as the + * account's entitlement hides models the account genuinely owns (#2886). + * + * A version must be supplied by the caller. There is deliberately no default: inventing one + * either manufactures a confirmed negative (the defect) or advertises models the installed + * runtime cannot drive (#2548, from the opposite side). + */ +function codexModelsUrl(clientVersion: string): string { + return `${CODEX_MODELS_ENDPOINT}?client_version=${encodeURIComponent(clientVersion)}`; +} + +/** A version string upstream can filter on. Rejects empty and the `0.0.0` placeholder. */ +export function isUsableCodexClientVersion(value: string | null | undefined): value is string { + if (typeof value !== "string") return false; + const trimmed = value.trim(); + if (!trimmed || trimmed === "0.0.0") return false; + return /^\d+(\.\d+)*([-+][0-9A-Za-z.-]+)?$/.test(trimmed); +} + +/** + * Version authority, in precedence order: + * + * 1. the inbound request's own `client_version` — the only value certainly describing the + * client being answered; + * 2. the selected Codex runtime version, for background sync where no request exists. + * Retained sync refreshes runtime evidence before discovery, which is what makes this + * usable here; the persisted file itself carries no freshness guarantee. + * + * Neither available means the roster is not requested at all. Unconfirmed already suppresses + * the gated rows and routing already rejects them, so failing closed on ABSENT evidence is + * the existing contract; failing closed on INVENTED evidence was the bug. + */ +export function resolveCodexEntitlementClientVersion( + inbound?: string | null, + loadRuntime: () => { selectedVersion?: string | null } | null = loadPersistedCodexRuntime, +): string | null { + if (isUsableCodexClientVersion(inbound)) return inbound.trim(); + let persisted: { selectedVersion?: string | null } | null = null; + try { + persisted = loadRuntime(); + } catch { + persisted = null; + } + const selected = persisted?.selectedVersion; + return isUsableCodexClientVersion(selected) ? selected.trim() : null; +} const MODEL_ROSTER_TTL_MS = 5 * 60_000; const MODEL_ROSTER_FAILURE_TTL_MS = 15_000; const MODEL_ROSTER_TIMEOUT_MS = 8_000; @@ -29,6 +81,12 @@ export interface CodexModelEntitlementCredentialSnapshot { interface CachedAccountModels { readonly credentialIdentity: string; + /** + * Client version this roster was fetched under. Upstream filters by it, so a roster is + * only an answer for that version — an entry fetched under one must never satisfy a read + * for another (#2886). + */ + readonly clientVersion: string; readonly expiresAt: number; readonly models: ReadonlySet; readonly confirmed: boolean; @@ -42,6 +100,17 @@ export interface CodexModelEntitlementSnapshot { export interface CodexModelEntitlementResolveOptions { readonly fetcher?: typeof fetch; + /** + * Client version to ask upstream about. Absent means no trustworthy version was + * available, and discovery is skipped rather than asked under a placeholder. + */ + readonly clientVersion?: string | null; + /** + * Test-only seam for the persisted-runtime half of the version precedence chain, so a case + * can reach the no-trustworthy-version branch without depending on the host's own runtime + * state file. + */ + readonly loadPersistedRuntime?: () => { selectedVersion?: string | null } | null; readonly nativeMainRefreshDependencies?: NativeMainRefreshDependencies; readonly now?: number; readonly signal?: AbortSignal; @@ -150,6 +219,7 @@ async function fetchAccountModels( credential: CodexModelEntitlementCredentialSnapshot, fetcher: typeof fetch, now: number, + clientVersion: string, ): Promise { const controller = new AbortController(); const timer = setTimeout(() => controller.abort(new DOMException("Codex model discovery timed out", "TimeoutError")), MODEL_ROSTER_TIMEOUT_MS); @@ -159,7 +229,7 @@ async function fetchAccountModels( Accept: "application/json", }); if (credential.chatgptAccountId) headers.set("ChatGPT-Account-Id", credential.chatgptAccountId); - const response = await fetcher(CODEX_MODELS_URL, { + const response = await fetcher(codexModelsUrl(clientVersion), { headers, redirect: "error", signal: controller.signal, @@ -174,6 +244,7 @@ async function fetchAccountModels( : null; return { credentialIdentity: credential.credentialIdentity, + clientVersion, expiresAt: now + (models ? MODEL_ROSTER_TTL_MS : MODEL_ROSTER_FAILURE_TTL_MS), models: models ?? new Set(), confirmed: models !== null, @@ -181,6 +252,7 @@ async function fetchAccountModels( } catch { return { credentialIdentity: credential.credentialIdentity, + clientVersion, expiresAt: now + MODEL_ROSTER_FAILURE_TTL_MS, models: new Set(), confirmed: false, @@ -212,18 +284,22 @@ async function modelsForCredential( credential: CodexModelEntitlementCredentialSnapshot, fetcher: typeof fetch, now: number, + clientVersion: string, ): Promise { const cached = accountModelsCache.get(credential.accountId); if ( cached && cached.credentialIdentity === credential.credentialIdentity + // Upstream filters by client version, so a roster fetched under a different one is a + // different question's answer and must not be reused. + && cached.clientVersion === clientVersion && cached.expiresAt > now ) return cached; - const flightKey = `${credential.accountId}\u0000${credential.credentialIdentity}`; + const flightKey = `${credential.accountId}\u0000${credential.credentialIdentity}\u0000${clientVersion}`; const existing = accountModelsFlights.get(flightKey); if (existing) return existing; - const flight = fetchAccountModels(credential, fetcher, now) + const flight = fetchAccountModels(credential, fetcher, now, clientVersion) .then(result => { if (currentCredentialIdentity(credential.accountId) === credential.credentialIdentity) { boundedCacheSet(credential.accountId, result); @@ -269,6 +345,10 @@ export async function resolveCodexModelEntitlements( ): Promise { const now = options.now ?? Date.now(); const fetcher = options.fetcher ?? fetch; + const clientVersion = resolveCodexEntitlementClientVersion( + options.clientVersion, + options.loadPersistedRuntime ?? loadPersistedCodexRuntime, + ); const allowedAccountIds = candidateAccountIds(config) .filter(accountId => !options.excludeAccountIds?.has(accountId)); const credentialSnapshot = options.credentialSnapshot ?? accountCredentialSnapshot; @@ -276,9 +356,19 @@ export async function resolveCodexModelEntitlements( ? [...options.credentials].filter(credential => !options.excludeAccountIds?.has(credential.accountId)) : (await Promise.all(allowedAccountIds.map(accountId => credentialSnapshot(accountId, options)))) .filter((value): value is CodexModelEntitlementCredentialSnapshot => value !== null); + // No trustworthy client version means the roster cannot be asked for meaningfully. + // Report every account as UNCONFIRMED rather than fetching under a placeholder that + // upstream would answer with an empty or truncated roster (#2886). + if (clientVersion === null) { + return { + modelsByAccount: new Map(credentials.map(credential => [credential.accountId, new Set()])), + confirmedAccountIds: new Set(), + credentialIdentities: new Map(credentials.map(credential => [credential.accountId, credential.credentialIdentity])), + }; + } const results = await Promise.all(credentials.map(async credential => ({ credential, - result: await modelsForCredential(credential, fetcher, now), + result: await modelsForCredential(credential, fetcher, now, clientVersion), }))); return { modelsByAccount: new Map(results.map(({ credential, result }) => [credential.accountId, result.models])), @@ -291,15 +381,19 @@ export async function resolveCodexModelEntitlements( export async function isDirectCallerEntitledToCodexModel( headers: Headers, modelId: string, - options: Pick = {}, + options: Pick = {}, ): Promise { if (!ACCOUNT_GATED_NATIVE_OPENAI_MODELS.has(modelId)) return true; const credential = directCallerCredential(headers); if (!credential) return false; + const clientVersion = resolveCodexEntitlementClientVersion(options.clientVersion); + // Fail closed on absent evidence, exactly as an unconfirmed roster does. + if (clientVersion === null) return false; const result = await modelsForCredential( credential, options.fetcher ?? fetch, options.now ?? Date.now(), + clientVersion, ); return result.confirmed && result.models.has(modelId); } @@ -331,11 +425,22 @@ export function availableAccountGatedNativeModels( export function cachedAvailableAccountGatedNativeModels( now = Date.now(), eligibleAccountIds?: ReadonlySet, + clientVersion?: string | null, ): ReadonlySet { + // Entries fetched under a different client version answer a different question, so a caller + // that knows which version it is projecting for must say so — otherwise a newer client's + // roster leaks into an older client's projection (#2548 from the opposite direction). + // + // Omitting the argument deliberately does NOT filter. This is a synchronous read of + // whatever discovery already proved, and its callers (catalog metadata) are not + // request-scoped: resolving a version here would silently discard every entry fetched under + // a different one, which is a suppression this function has no evidence to justify. + const version = isUsableCodexClientVersion(clientVersion) ? clientVersion.trim() : null; return new Set([...ACCOUNT_GATED_NATIVE_OPENAI_MODELS].filter(modelId => ( [...accountModelsCache].some(([accountId, entry]) => ( (!eligibleAccountIds || eligibleAccountIds.has(accountId)) && !accountId.startsWith(DIRECT_CALLER_ACCOUNT_PREFIX) + && (version === null || entry.clientVersion === version) && entry.confirmed && entry.expiresAt > now && entry.models.has(modelId) @@ -363,9 +468,11 @@ export function seedCodexModelEntitlementsForTests( accountId: string, models: readonly string[], now = Date.now(), + clientVersion = "0.146.0", ): void { boundedCacheSet(accountId, { credentialIdentity: `test:${accountId}`, + clientVersion, expiresAt: now + MODEL_ROSTER_TTL_MS, models: new Set(models), confirmed: true, diff --git a/src/server/index.ts b/src/server/index.ts index 62e77b6d2f..85e2b4a1aa 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -1070,7 +1070,10 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server row.slug), ...accountNativeQualified, + // A model suppressed by an unconfirmed entitlement roster is absent from + // nativeModelRows, so validating against those rows alone rejected a model this build + // knows perfectly well and left the operator with no way to clear its disable key + // (#2886). Accepting the target says "this build knows this model", not "this account + // may use it" — visibility only writes disabledModels and routing stays gated. + ...NATIVE_OPENAI_MODELS, ]); const targets: Array<{ id: string; native: boolean }> = []; const seen = new Set(); diff --git a/tests/codex-model-entitlements.test.ts b/tests/codex-model-entitlements.test.ts index ca4e631da4..ab8884b668 100644 --- a/tests/codex-model-entitlements.test.ts +++ b/tests/codex-model-entitlements.test.ts @@ -5,12 +5,14 @@ import { entitledCodexAccountIdsForModel, isDirectCallerEntitledToCodexModel, resetCodexModelEntitlementCacheForTests, + resolveCodexEntitlementClientVersion, resolveCodexModelEntitlements, seedCodexModelEntitlementsForTests, type CodexModelEntitlementCredentialSnapshot, } from "../src/codex/model-entitlements"; import { MAIN_CODEX_ACCOUNT_ID } from "../src/codex/main-account"; +const TEST_CLIENT_VERSION = "0.146.0"; const DAYBREAK = "gpt-daybreak-blue-latest"; const SOL = "gpt-5.6-sol"; const TERRA = "gpt-5.6-terra"; @@ -44,6 +46,7 @@ describe("Codex account model entitlements", () => { : roster(SOL, TERRA); }) as typeof fetch, now: 1_000, + clientVersion: TEST_CLIENT_VERSION, }); expect([...entitledCodexAccountIdsForModel(snapshot, DAYBREAK)!]).toEqual(["main"]); @@ -58,6 +61,7 @@ describe("Codex account model entitlements", () => { credentials: [credential("broken")], fetcher: (async () => new Response("not-json", { status: 502 })) as typeof fetch, now: 1_000, + clientVersion: TEST_CLIENT_VERSION, }); expect(snapshot.confirmedAccountIds.size).toBe(0); @@ -73,6 +77,7 @@ describe("Codex account model entitlements", () => { { slug: "gpt-disabled", supported_in_api: false, visibility: "list" }, ] })) as typeof fetch, now: 1_000, + clientVersion: TEST_CLIENT_VERSION, }); expect(snapshot.confirmedAccountIds.has("main")).toBe(true); @@ -97,6 +102,7 @@ describe("Codex account model entitlements", () => { return roster(DAYBREAK); }) as typeof fetch, now: 1_000, + clientVersion: TEST_CLIENT_VERSION, }); expect(credentialReads).toEqual(["pool-b"]); @@ -130,6 +136,7 @@ describe("Codex account model entitlements", () => { return roster("gpt-5.6-sol", DAYBREAK); }) as typeof fetch, now: 1_000, + clientVersion: TEST_CLIENT_VERSION, }, ); @@ -145,6 +152,7 @@ describe("Codex account model entitlements", () => { { fetcher: (async () => new Response("unavailable", { status: 503 })) as typeof fetch, now: 1_000, + clientVersion: TEST_CLIENT_VERSION, }, )).resolves.toBe(false); }); @@ -171,3 +179,104 @@ describe("Codex account model entitlements", () => { }); }); + +describe("entitlement client version (#2886)", () => { + /** + * Upstream filters this roster by the client version it is told, and `client_version` is a + * required parameter — a measured 0.60.0 returns zero models where 0.142.2 returns five + * (devlog/_fin/260817_native_gpt56_1m_context/001_measurement_evidence.md). Asking as + * 0.0.0 therefore describes what a prehistoric client may use, and the fail-closed gate + * added by #2550 turned that into "this account cannot use GPT-5.6" for an account that + * demonstrably can. + */ + function versionFilteredBackend(seen: string[]): typeof fetch { + return (async (input: RequestInfo | URL) => { + const url = new URL(input instanceof Request ? input.url : String(input)); + const version = url.searchParams.get("client_version") ?? ""; + seen.push(version); + const major = Number(version.split(".")[1] ?? "0"); + // Below the GPT-5.6 threshold upstream simply omits those rows. + return major >= 144 ? roster("gpt-5.5", SOL, TERRA, LUNA) : roster("gpt-5.5"); + }) as typeof fetch; + } + + test("an entitled account keeps GPT-5.6 when the real runtime version is reported", async () => { + const seen: string[] = []; + const snapshot = await resolveCodexModelEntitlements({ codexAccounts: [] }, { + credentials: [credential("main")], + fetcher: versionFilteredBackend(seen), + now: 1_000, + clientVersion: "0.146.0", + }); + + expect(seen).toEqual(["0.146.0"]); + // The wrong behavior: an entitled account classified as denying GPT-5.6 because + // OpenCodex under-reported its own client version. + expect([...availableAccountGatedNativeModels(snapshot)]).toEqual([SOL, TERRA, LUNA]); + expect(snapshot.confirmedAccountIds.has("main")).toBe(true); + }); + + test("no trustworthy version means UNCONFIRMED, not a confirmed denial", async () => { + // Fail closed on absent evidence is the existing contract; failing closed on invented + // evidence is the defect. A placeholder would return a real 200 with a short roster, + // which reads as "this account positively lacks these models". + // + // `clientVersion: null` is an inbound miss, not a verdict — the resolver still consults + // the selected runtime, which is the whole point of the precedence chain. To reach the + // no-evidence branch both sources have to be unusable, so this pins the resolver's own + // output and then drives discovery with it. + expect(resolveCodexEntitlementClientVersion(null, () => null)).toBeNull(); + const seen: string[] = []; + const snapshot = await resolveCodexModelEntitlements({ codexAccounts: [] }, { + credentials: [credential("main")], + fetcher: versionFilteredBackend(seen), + now: 1_000, + clientVersion: null, + // Both halves of the chain unusable: no inbound version, no selected runtime. + loadPersistedRuntime: () => null, + }); + + expect(seen).toEqual([]); + expect(snapshot.confirmedAccountIds.has("main")).toBe(false); + // Read the SNAPSHOT, not the process-wide cache: another suite in the same run can leave + // a confirmed entry behind, and this assertion is about what this discovery pass proved. + expect([...availableAccountGatedNativeModels(snapshot)]).toEqual([]); + expect(snapshot.modelsByAccount.get("main")?.size).toBe(0); + // The account is still enumerated, so callers can tell "unknown" from "absent". + expect(snapshot.modelsByAccount.has("main")).toBe(true); + }); + + test("the placeholder 0.0.0 is never accepted as a client version", async () => { + // 0.0.0 is exactly what shipped, and it is a syntactically valid version string, so the + // guard has to reject it by value rather than by shape. + expect(resolveCodexEntitlementClientVersion("0.0.0", () => null)).toBeNull(); + expect(resolveCodexEntitlementClientVersion("", () => null)).toBeNull(); + expect(resolveCodexEntitlementClientVersion(null, () => null)).toBeNull(); + expect(resolveCodexEntitlementClientVersion("0.146.0", () => null)).toBe("0.146.0"); + // The inbound value wins over the persisted runtime; the runtime is the sync fallback. + expect(resolveCodexEntitlementClientVersion("0.146.0", () => ({ selectedVersion: "0.120.0" }))) + .toBe("0.146.0"); + expect(resolveCodexEntitlementClientVersion(null, () => ({ selectedVersion: "0.145.1" }))) + .toBe("0.145.1"); + expect(resolveCodexEntitlementClientVersion(null, () => ({ selectedVersion: "0.0.0" }))).toBeNull(); + // A persisted-state read that throws must not take entitlement down with it. + expect(resolveCodexEntitlementClientVersion(null, () => { throw new Error("unreadable"); })).toBeNull(); + }); + + test("a cached roster is projected only for the version it was fetched under", async () => { + // Upstream's answer is version-specific, so reusing it across versions would either hide + // models from a newer client or advertise them to an older one (#2548, inverted). The + // cache holds one entry per account, so what matters is that the entry knows its own + // version and the projection respects it. + seedCodexModelEntitlementsForTests("main", [SOL, TERRA, LUNA], 1_000, "0.146.0"); + + expect([...cachedAvailableAccountGatedNativeModels(1_100, undefined, "0.146.0")]) + .toEqual([SOL, TERRA, LUNA]); + // A caller asking about an older client must not be handed the newer client's roster. + expect([...cachedAvailableAccountGatedNativeModels(1_100, undefined, "0.140.0")]).toEqual([]); + // An unusable version cannot select an entry at all, so it degrades to the unfiltered + // read rather than silently matching one. + expect([...cachedAvailableAccountGatedNativeModels(1_100, undefined, "0.0.0")]) + .toEqual([SOL, TERRA, LUNA]); + }); +}); diff --git a/tests/model-visibility-management-api.test.ts b/tests/model-visibility-management-api.test.ts index 6e8e90a995..edac0a8659 100644 --- a/tests/model-visibility-management-api.test.ts +++ b/tests/model-visibility-management-api.test.ts @@ -322,5 +322,43 @@ describe("atomic model visibility management", () => { expect(loadConfig()).toEqual(before); expect(refreshes).toBe(2); }); + + test("a native model suppressed by an unconfirmed roster is still a valid visibility target (#2886)", async () => { + // The endpoint validated bare native targets against nativeModelRows, which has already + // dropped rows an unconfirmed entitlement roster suppressed. So `ocx models enable + // gpt-5.6-sol` answered "invalid model visibility target" for a model this build knows + // perfectly well, leaving the operator with no way to clear its disable key. + // + // Scope: accepting the target says "this build knows this model", not "this account may + // use it". Visibility only writes disabledModels; entitlement still filters the rendered + // rows and routing stays gated, so this removes a misleading error rather than granting + // access. + saveConfig({ ...loadConfig(), disabledModels: ["gpt-5.6-sol", "other/keep"] }); + // Precondition: the model is genuinely absent from the rendered rows here. + expect(nativeModelRows(loadConfig()).some(row => row.slug === "gpt-5.6-sol")).toBe(false); + + const response = await put({ + scope: "models", + provider: "openai", + targets: [{ id: "gpt-5.6-sol", native: true }], + enabled: true, + }); + + expect(response.status).toBe(200); + expect(await response.text()).not.toContain("invalid model visibility target"); + expect(loadConfig().disabledModels).toEqual(["other/keep"]); + }); + + test("an unknown native id is still rejected (#2886)", async () => { + // The validation set widens to what this build knows, not to anything a caller names. + const before = loadConfig(); + expect((await put({ + scope: "models", + provider: "openai", + targets: [{ id: "gpt-9.9-imaginary", native: true }], + enabled: true, + })).status).toBe(400); + expect(loadConfig()).toEqual(before); + }); }); import { ManagementRequest as Request } from "./helpers/management-auth"; From 4a8a042328b1c64142411b85f0ad71a9c38e7b45 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 19:52:44 +0900 Subject: [PATCH 2/6] fix(codex): scope the entitlement roster cache by client version and keep sync working Review of the previous commit found two correctness blockers and two should-fix items. 1. The last tier of version precedence returned null, which skipped discovery entirely. That is correct for a request path and wrong for background sync: syncCatalogModels has no inbound request and, on a host where Codex has never been resolved, no persisted runtime either -- yet it is the path that publishes account-confirmed native rows. Two pre-existing tests failed on CI. The tier now asks under GATED_MODEL_CLIENT_VERSION_FLOOR, derived from the highest minimal_client_version this build's own bundled roster records for the gated models, so a refreshed snapshot cannot leave it stale. Both null branches are gone and the resolver's return type is now plain string. 2. The cache key stayed account-only and merely compared the stored version on read. With two versions in flight for one account the later one overwrote the earlier, and the unversioned projection readers in catalog/metadata.ts then published whichever landed last. The key is now account + version, and per-account invalidation walks every version's entry so a credential change still clears all of them. 3. Tier 2 read codex-runtime.json on every gated authorization and every /v1/models resolution, including when the roster cache was hot. It is memoized for five seconds, with an explicit bypass for callers asking about a loader other than the real file. 4. isUsableCodexClientVersion rejected only the exact string 0.0.0. Every all-zero core makes the same claim, so 0, 0.0, 00.0.0 and 0.0.0-dev are now rejected by value, and the length is bounded because the value is interpolated into an outbound URL. The cache test was vacuous: it seeded the cache directly and stayed green after reverting both protections it named. It is replaced by sequential and concurrent cases driven through the Direct-caller seam, whose credential identity satisfies the guard that decides whether a completed flight may write. A route-level test now proves client_version reaches discovery. Focused suites: 339 pass / 0 fail. Each guard driven red by mutation. --- ...0_issue_2886_entitlement_client_version.md | 71 +++++- src/codex/model-entitlements.ts | 170 ++++++++++--- tests/claude-models-discovery.test.ts | 45 ++++ tests/codex-model-entitlements.test.ts | 227 +++++++++++++++--- 4 files changed, 440 insertions(+), 73 deletions(-) diff --git a/devlog/_plan/260829_bugpr_lane_h_residual_issues/160_issue_2886_entitlement_client_version.md b/devlog/_plan/260829_bugpr_lane_h_residual_issues/160_issue_2886_entitlement_client_version.md index a6fe0f2a61..4062ee6e1f 100644 --- a/devlog/_plan/260829_bugpr_lane_h_residual_issues/160_issue_2886_entitlement_client_version.md +++ b/devlog/_plan/260829_bugpr_lane_h_residual_issues/160_issue_2886_entitlement_client_version.md @@ -63,11 +63,31 @@ So version authority is a precedence chain, not a single lookup: describe the binary OpenCodex chose rather than an externally launched client. Retained sync does refresh runtime evidence first (`src/codex/catalog/sync.ts:1828`), which is what makes it usable here and not elsewhere. -3. **Neither available → do not ask.** Sending `0.0.0` manufactures a confirmed negative, - which is the whole defect. No trustworthy version means unconfirmed, and unconfirmed - already suppresses (`:321`) and routing already rejects - (`src/codex/auth-context.ts:458`). Fail closed on absent evidence, never on invented - evidence. +3. **Neither available → ask under this build's own gated floor.** + `GATED_MODEL_CLIENT_VERSION_FLOOR` is derived from the highest + `minimal_client_version` that `src/codex/data/upstream-models.json` records for the + models in `ACCOUNT_GATED_NATIVE_OPENAI_MODELS` (`0.142.2` today). It is a claim this + repository can substantiate, and it is derived rather than written down so a refreshed + snapshot cannot leave it stale. + + **This tier was wrong in the first attempt and CI caught it.** The original design said + "neither available → do not ask", on the reasoning that failing closed on absent evidence + was the existing contract. That is true for a *request*, and false for background sync: + `syncCatalogModels` has no inbound request, and on a host where Codex has never been + resolved it has no persisted runtime either — yet it is exactly the path that publishes + account-confirmed native rows. Skipping discovery there suppressed the rows this fix + exists to restore. Two pre-existing tests failed on `dev` CI and neither was in the + originally chosen focused set: + `tests/claude-models-discovery.test.ts` ("Codex discovery exposes the observed native as + a selector row plus one global bare row") and `tests/codex-catalog-sync-hardening.test.ts` + ("account sync preserves an observed gated native only after the mapped account confirms + it"). The lesson is narrow and worth keeping: *fail-closed is a property of a request + path, and a background publisher is not a request path.* + + Sending `0.0.0` remains forbidden, and now by value rather than by exact string — + `0`, `0.0`, `00.0.0`, and `0.0.0-dev` all make the same claim (a client predating every + gated model) and are all rejected. The value is also length-bounded because it is + interpolated into an outbound URL. This needs a real seam. `fetcher` (`:43`) can observe the URL but cannot choose the version, so `resolveCodexModelEntitlements` and `isDirectCallerEntitledToCodexModel` @@ -78,6 +98,13 @@ both take an explicit client version. `accountModelsCache` is keyed by account ID alone, with credential identity stored as a discriminator (`:30`, `:216`); the flight key is account plus credential identity (`:223`). Version must join both, or a roster fetched under one version keeps answering + +The first attempt kept the account-only **cache key** and merely compared the stored version +on read. Review showed that is not equivalent: with two versions in flight for one account, +the later-completing one overwrites the earlier, and the *unversioned* projection readers in +`src/codex/catalog/metadata.ts:424,514` then publish whichever landed last rather than what +each client proved. The key itself is now `account\u0000version`, with account-scoped +invalidation walking every version's entry so a credential change still clears all of them. for another until the TTL expires. `cachedAvailableAccountGatedNativeModels` scans every cache entry (`:331`). Once two @@ -115,12 +142,33 @@ roster below the threshold and the full roster at `0.146.0`. The wrong behavior is the real one — *an entitled account is classified as denying GPT-5.6 because OpenCodex under-reports its own client version*. Named mutation: restore the `0.0.0` literal. -A second case pins the precedence chain: with no trustworthy version, discovery must be -**unconfirmed** rather than a confirmed negative. Named mutation: fall back to `0.0.0`; -the account is then reported as positively denying the models. - -Cache identity gets its own case — fetch under one version, ask under another, assert a -re-fetch. Named mutation: drop version from the cache key. +A second case pins the precedence chain's last tier: with no inbound version and no +persisted runtime, discovery must still ask — under the derived floor, verbatim. Named +mutations: return `null` from tier 3 (three tests fail, including the two CI regressions +above), and hardcode a stale floor instead of deriving it from the snapshot. + +Cache identity gets its own case, and the **first version of it was vacuous** — an +independent review proved the test stayed green after reverting *both* the cache-hit version +comparison and the version component of the flight key. It seeded the cache directly through +`seedCodexModelEntitlementsForTests`, so it only ever exercised the optional projection +filter, never the write path. The rework drives the real path through a Direct caller, whose +credential identity is derived from its own bearer token (`direct:`) and therefore +satisfies the identity guard that decides whether a completed flight may write — which a +synthetic pool credential never does. Two cases now: + +- sequential: fetch under version A, ask again under A (served from cache, no second + request), then ask under B and assert a re-fetch; +- concurrent: two versions in flight for one account, completing newest-first, and both + answers must survive. Named mutation for both: collapse the cache key back to account-only. + The flight key's version component has its own mutation, which the concurrent case catches. + +The version is also asserted end to end at the route: `/v1/models?client_version=0.151.7` +must produce `0.151.7` on the outbound `/codex/models` request. Named mutation: drop the +`url.searchParams.get("client_version")` argument in `src/server/index.ts`. + +Tier 2 is memoized for five seconds because it reads `codex-runtime.json` from disk on every +gated authorization and every `/v1/models` resolution, including when the roster cache is hot +and the answer needs no I/O at all. Named mutation: bypass the memo and re-read every time. **B** in `tests/model-visibility-management-api.test.ts`: with `disabledModels: ["gpt-5.6-sol"]` and no entitlement cache, the PUT must be accepted and clear the entry, @@ -135,4 +183,3 @@ the same symptom. The version-filter explanation is what the source, the version and this repository's own measurement support, and the fix is correct either way — but if their roster was failing for another reason the models will still be missing afterwards, and the issue should be reopened with a redacted capture rather than assumed fixed. - diff --git a/src/codex/model-entitlements.ts b/src/codex/model-entitlements.ts index af52f345a1..4b77b31998 100644 --- a/src/codex/model-entitlements.ts +++ b/src/codex/model-entitlements.ts @@ -11,6 +11,7 @@ import { } from "./main-account"; import { ACCOUNT_GATED_NATIVE_OPENAI_MODELS } from "./catalog/native-models"; import { loadPersistedCodexRuntime } from "./runtime"; +import upstreamModelsSnapshot from "./data/upstream-models.json"; const CODEX_MODELS_ENDPOINT = "https://chatgpt.com/backend-api/codex/models"; @@ -34,7 +35,52 @@ export function isUsableCodexClientVersion(value: string | null | undefined): va if (typeof value !== "string") return false; const trimmed = value.trim(); if (!trimmed || trimmed === "0.0.0") return false; - return /^\d+(\.\d+)*([-+][0-9A-Za-z.-]+)?$/.test(trimmed); + // Bounded: the value reaches an outbound URL, and no real client version is this long. + if (trimmed.length > 64) return false; + if (!/^\d+(\.\d+)*([-+][0-9A-Za-z.-]+)?$/.test(trimmed)) return false; + // An all-zero numeric core is the same claim `0.0.0` makes — a client predating every gated + // model — so `0`, `0.0`, `00.0.0`, and `0.0.0-dev` must fail for the same reason, by value + // rather than by exact string. + const core = trimmed.split(/[-+]/, 1)[0]!; + if (core.split(".").every(segment => Number(segment) === 0)) return false; + return true; +} + +/** + * Highest `minimal_client_version` this build's bundled roster records for the models that + * are account-gated. Derived, not hardcoded: if the snapshot is refreshed with a model that + * requires a newer client, the floor follows it, so the two can never drift apart. + * + * Used only as the last tier of version precedence, for background work that has no request + * and no resolved runtime to speak for. It answers "what does this build believe the gated + * models need?", which is a claim the repository can actually substantiate. + */ +export const GATED_MODEL_CLIENT_VERSION_FLOOR: string = (() => { + const rows = (upstreamModelsSnapshot as { models?: Array> }).models ?? []; + const floors = rows + .filter(row => typeof row.slug === "string" && ACCOUNT_GATED_NATIVE_OPENAI_MODELS.has(row.slug)) + .map(row => (typeof row.minimal_client_version === "string" ? row.minimal_client_version : null)) + .filter((value): value is string => isUsableCodexClientVersion(value)); + const highest = floors.reduce( + (best, candidate) => (best === null || compareClientVersions(candidate, best) > 0 ? candidate : best), + null, + ); + // A snapshot with no gated row still has to yield a usable question; the newest floor any + // row records is a better answer than a placeholder, and an empty snapshot is not a state + // this build ships in. + return highest ?? "0.142.2"; +})(); + +/** Numeric-segment comparison. Only used to pick the highest floor in a known-good set. */ +function compareClientVersions(left: string, right: string): number { + const l = left.split(/[.+-]/).map(Number); + const r = right.split(/[.+-]/).map(Number); + for (let i = 0; i < Math.max(l.length, r.length); i += 1) { + const a = Number.isFinite(l[i]) ? l[i]! : 0; + const b = Number.isFinite(r[i]) ? r[i]! : 0; + if (a !== b) return a - b; + } + return 0; } /** @@ -45,26 +91,73 @@ export function isUsableCodexClientVersion(value: string | null | undefined): va * 2. the selected Codex runtime version, for background sync where no request exists. * Retained sync refreshes runtime evidence before discovery, which is what makes this * usable here; the persisted file itself carries no freshness guarantee. + * 3. the floor this build's own bundled roster records for the models being gated + * (`GATED_MODEL_CLIENT_VERSION_FLOOR`). + * + * Tier 3 exists because background catalog sync has no request and, on a host where Codex + * has never been resolved, no persisted runtime either — yet it is exactly the path that + * publishes account-confirmed native rows. Skipping discovery there suppressed the rows this + * fix is meant to restore. The floor is not invented: it is the version this build's own + * snapshot states the gated models require, so asking under it is the narrowest question + * that can still return them. * - * Neither available means the roster is not requested at all. Unconfirmed already suppresses - * the gated rows and routing already rejects them, so failing closed on ABSENT evidence is - * the existing contract; failing closed on INVENTED evidence was the bug. + * There is deliberately no `0.0.0`-style fallback. A placeholder describes a client that + * predates every gated model, which is what made upstream answer with an empty roster and + * turned absent evidence into a manufactured confirmed negative (#2886). */ export function resolveCodexEntitlementClientVersion( inbound?: string | null, loadRuntime: () => { selectedVersion?: string | null } | null = loadPersistedCodexRuntime, -): string | null { + options: { readonly bypassRuntimeMemo?: boolean; readonly now?: number } = {}, +): string { if (isUsableCodexClientVersion(inbound)) return inbound.trim(); - let persisted: { selectedVersion?: string | null } | null = null; + // A caller can opt out when it is asking about a loader other than the real runtime file, + // which the process-wide memo describes. + const selected = options.bypassRuntimeMemo === true + ? readRuntimeVersion(loadRuntime) + : memoizedPersistedRuntimeVersion(loadRuntime, options.now ?? Date.now()); + return selected ?? GATED_MODEL_CLIENT_VERSION_FLOOR; +} +const MODEL_ROSTER_TTL_MS = 5 * 60_000; + +/** + * How long a persisted-runtime version read is reused. + * + * Tier 2 of the version chain reads `codex-runtime.json` from disk, and it is consulted on + * every gated Direct authorization and every `/v1/models` resolution — including when the + * five-minute roster cache is hot, so the entitlement answer needs no I/O at all. Left + * unmemoized that is a synchronous `readFileSync` on the request path under concurrent gated + * traffic. `persistCodexRuntime` is the only writer, and it clears the runtime's own resolve + * cache; this window is short enough that a runtime switch is picked up promptly either way. + */ +const RUNTIME_VERSION_MEMO_MS = 5_000; + +let runtimeVersionMemo: { at: number; version: string | null } | null = null; + +function readRuntimeVersion( + loadRuntime: () => { selectedVersion?: string | null } | null, +): string | null { try { - persisted = loadRuntime(); + const value = loadRuntime()?.selectedVersion; + return isUsableCodexClientVersion(value) ? value.trim() : null; } catch { - persisted = null; + // An unreadable or malformed runtime file is an absent version, not a failure worth + // propagating into entitlement resolution. + return null; } - const selected = persisted?.selectedVersion; - return isUsableCodexClientVersion(selected) ? selected.trim() : null; } -const MODEL_ROSTER_TTL_MS = 5 * 60_000; + +function memoizedPersistedRuntimeVersion( + loadRuntime: () => { selectedVersion?: string | null } | null, + now: number, +): string | null { + if (runtimeVersionMemo && now - runtimeVersionMemo.at < RUNTIME_VERSION_MEMO_MS) { + return runtimeVersionMemo.version; + } + const selected = readRuntimeVersion(loadRuntime); + runtimeVersionMemo = { at: now, version: selected }; + return selected; +} const MODEL_ROSTER_FAILURE_TTL_MS = 15_000; const MODEL_ROSTER_TIMEOUT_MS = 8_000; const MODEL_ROSTER_MAX_BYTES = 2 * 1024 * 1024; @@ -125,6 +218,22 @@ export interface CodexModelEntitlementResolveOptions { const accountModelsCache = new Map(); const accountModelsFlights = new Map>(); +/** + * Cache key. The roster is version-specific, so the version has to be part of the identity — + * with an account-only key, two versions in flight for one account race to overwrite each + * other, and the unversioned projection readers in `catalog/metadata.ts` then publish + * whichever landed last. + */ +function cacheKeyFor(accountId: string, clientVersion: string): string { + return `${accountId}\u0000${clientVersion}`; +} + +/** Account component of a cache key, for eviction budgets and per-account invalidation. */ +function accountIdOfCacheKey(key: string): string { + const separator = key.indexOf("\u0000"); + return separator === -1 ? key : key.slice(0, separator); +} + /** * Direct-caller entries are evicted separately from main/Pool entries. * @@ -136,9 +245,11 @@ const accountModelsFlights = new Map>(); * from erasing the other's evidence. */ function boundedCacheSet(accountId: string, value: CachedAccountModels): void { - accountModelsCache.delete(accountId); - accountModelsCache.set(accountId, value); - const isDirect = (key: string): boolean => key.startsWith(DIRECT_CALLER_ACCOUNT_PREFIX); + const key = cacheKeyFor(accountId, value.clientVersion); + accountModelsCache.delete(key); + accountModelsCache.set(key, value); + const isDirect = (candidate: string): boolean => + accountIdOfCacheKey(candidate).startsWith(DIRECT_CALLER_ACCOUNT_PREFIX); const evictClass = (direct: boolean): void => { let count = 0; for (const key of accountModelsCache.keys()) if (isDirect(key) === direct) count += 1; @@ -152,7 +263,7 @@ function boundedCacheSet(accountId: string, value: CachedAccountModels): void { count -= 1; } }; - evictClass(isDirect(accountId)); + evictClass(accountId.startsWith(DIRECT_CALLER_ACCOUNT_PREFIX)); } function currentCredentialIdentity(accountId: string): string | undefined { @@ -286,13 +397,10 @@ async function modelsForCredential( now: number, clientVersion: string, ): Promise { - const cached = accountModelsCache.get(credential.accountId); + const cached = accountModelsCache.get(cacheKeyFor(credential.accountId, clientVersion)); if ( cached && cached.credentialIdentity === credential.credentialIdentity - // Upstream filters by client version, so a roster fetched under a different one is a - // different question's answer and must not be reused. - && cached.clientVersion === clientVersion && cached.expiresAt > now ) return cached; @@ -356,16 +464,6 @@ export async function resolveCodexModelEntitlements( ? [...options.credentials].filter(credential => !options.excludeAccountIds?.has(credential.accountId)) : (await Promise.all(allowedAccountIds.map(accountId => credentialSnapshot(accountId, options)))) .filter((value): value is CodexModelEntitlementCredentialSnapshot => value !== null); - // No trustworthy client version means the roster cannot be asked for meaningfully. - // Report every account as UNCONFIRMED rather than fetching under a placeholder that - // upstream would answer with an empty or truncated roster (#2886). - if (clientVersion === null) { - return { - modelsByAccount: new Map(credentials.map(credential => [credential.accountId, new Set()])), - confirmedAccountIds: new Set(), - credentialIdentities: new Map(credentials.map(credential => [credential.accountId, credential.credentialIdentity])), - }; - } const results = await Promise.all(credentials.map(async credential => ({ credential, result: await modelsForCredential(credential, fetcher, now, clientVersion), @@ -387,8 +485,6 @@ export async function isDirectCallerEntitledToCodexModel( const credential = directCallerCredential(headers); if (!credential) return false; const clientVersion = resolveCodexEntitlementClientVersion(options.clientVersion); - // Fail closed on absent evidence, exactly as an unconfirmed roster does. - if (clientVersion === null) return false; const result = await modelsForCredential( credential, options.fetcher ?? fetch, @@ -438,8 +534,8 @@ export function cachedAvailableAccountGatedNativeModels( const version = isUsableCodexClientVersion(clientVersion) ? clientVersion.trim() : null; return new Set([...ACCOUNT_GATED_NATIVE_OPENAI_MODELS].filter(modelId => ( [...accountModelsCache].some(([accountId, entry]) => ( - (!eligibleAccountIds || eligibleAccountIds.has(accountId)) - && !accountId.startsWith(DIRECT_CALLER_ACCOUNT_PREFIX) + (!eligibleAccountIds || eligibleAccountIds.has(accountIdOfCacheKey(accountId))) + && !accountIdOfCacheKey(accountId).startsWith(DIRECT_CALLER_ACCOUNT_PREFIX) && (version === null || entry.clientVersion === version) && entry.confirmed && entry.expiresAt > now @@ -456,12 +552,18 @@ export function isCodexModelEntitlementSnapshotCurrent(snapshot: CodexModelEntit } export function invalidateCodexModelEntitlementsForAccount(accountId: string | null | undefined): void { - if (accountId) accountModelsCache.delete(accountId); + if (!accountId) return; + // Every version's entry for this account, since a credential change invalidates the + // account's entitlement evidence regardless of which client version asked for it. + for (const key of [...accountModelsCache.keys()]) { + if (accountIdOfCacheKey(key) === accountId) accountModelsCache.delete(key); + } } export function resetCodexModelEntitlementCacheForTests(): void { accountModelsCache.clear(); accountModelsFlights.clear(); + runtimeVersionMemo = null; } export function seedCodexModelEntitlementsForTests( diff --git a/tests/claude-models-discovery.test.ts b/tests/claude-models-discovery.test.ts index 0fd20b156f..43a223fc79 100644 --- a/tests/claude-models-discovery.test.ts +++ b/tests/claude-models-discovery.test.ts @@ -514,3 +514,48 @@ test("disabled canonical OpenAI preserves bare bootstrap rows without advertisin await server.stop(true); } }); + +test("the request's client_version reaches entitlement discovery (#2886)", async () => { + // Codex sends client_version on this route and the value used to be discarded, so upstream + // was always asked as 0.0.0 — which it answers with a short roster, and the fail-closed gate + // reads that as a confirmed denial. This asserts the forwarding itself: the version observed + // on the OUTBOUND /codex/models request must be the one the client sent. + const config = configWithStaticModels(); + config.providers.openai = { + adapter: "openai-responses", + baseUrl: "https://chatgpt.com/backend-api/codex", + liveModels: false, + }; + saveConfig(config); + writeFileSync(join(isolatedCodexHome!.path, "auth.json"), JSON.stringify({ + tokens: { access_token: "main-token", account_id: "main-account" }, + }), "utf8"); + + const { resetCatalogRuntimeStateForTests } = await import("../src/codex/catalog"); + resetCatalogRuntimeStateForTests(); + resetCodexModelEntitlementCacheForTests(); + + const askedVersions: string[] = []; + const originalFetch = globalThis.fetch; + globalThis.fetch = (async (input, init) => { + const url = new URL(typeof input === "string" ? input : input instanceof URL ? input : input.url); + if (url.hostname === "chatgpt.com" && url.pathname.endsWith("/models")) { + askedVersions.push(url.searchParams.get("client_version") ?? ""); + return Response.json({ models: [{ slug: "gpt-5.5", supported_in_api: true, visibility: "list" }] }); + } + return originalFetch(input, init); + }) as typeof fetch; + + const server = startServer(0); + try { + await fetch(new URL("/v1/models?client_version=0.151.7", server.url)) + .then(response => response.json()); + expect(askedVersions.length).toBeGreaterThan(0); + // Forwarded verbatim, and in particular never the placeholder that caused #2886. + expect(askedVersions).toEqual(askedVersions.map(() => "0.151.7")); + expect(askedVersions).not.toContain("0.0.0"); + } finally { + globalThis.fetch = originalFetch; + await server.stop(true); + } +}); diff --git a/tests/codex-model-entitlements.test.ts b/tests/codex-model-entitlements.test.ts index ab8884b668..4108c1dba1 100644 --- a/tests/codex-model-entitlements.test.ts +++ b/tests/codex-model-entitlements.test.ts @@ -3,7 +3,9 @@ import { availableAccountGatedNativeModels, cachedAvailableAccountGatedNativeModels, entitledCodexAccountIdsForModel, + GATED_MODEL_CLIENT_VERSION_FLOOR, isDirectCallerEntitledToCodexModel, + isUsableCodexClientVersion, resetCodexModelEntitlementCacheForTests, resolveCodexEntitlementClientVersion, resolveCodexModelEntitlements, @@ -11,6 +13,8 @@ import { type CodexModelEntitlementCredentialSnapshot, } from "../src/codex/model-entitlements"; import { MAIN_CODEX_ACCOUNT_ID } from "../src/codex/main-account"; +import { ACCOUNT_GATED_NATIVE_OPENAI_MODELS } from "../src/codex/catalog/native-models"; +import upstreamModelsSnapshot from "../src/codex/data/upstream-models.json"; const TEST_CLIENT_VERSION = "0.146.0"; const DAYBREAK = "gpt-daybreak-blue-latest"; @@ -216,51 +220,139 @@ describe("entitlement client version (#2886)", () => { expect(snapshot.confirmedAccountIds.has("main")).toBe(true); }); - test("no trustworthy version means UNCONFIRMED, not a confirmed denial", async () => { - // Fail closed on absent evidence is the existing contract; failing closed on invented - // evidence is the defect. A placeholder would return a real 200 with a short roster, - // which reads as "this account positively lacks these models". - // - // `clientVersion: null` is an inbound miss, not a verdict — the resolver still consults - // the selected runtime, which is the whole point of the precedence chain. To reach the - // no-evidence branch both sources have to be unusable, so this pins the resolver's own - // output and then drives discovery with it. - expect(resolveCodexEntitlementClientVersion(null, () => null)).toBeNull(); + test("no request and no runtime still asks under this build's own gated floor", async () => { + // Background catalog sync has no inbound request and, on a host where Codex has never + // been resolved, no persisted runtime either — yet it is the path that publishes + // account-confirmed native rows. An earlier revision of this fix skipped discovery in + // that state, which suppressed exactly the rows the fix exists to restore + // (tests/claude-models-discovery.test.ts and tests/codex-catalog-sync-hardening.test.ts + // both failed on it). The last tier therefore has to be a real, answerable version. + expect(resolveCodexEntitlementClientVersion(null, () => null)) + .toBe(GATED_MODEL_CLIENT_VERSION_FLOOR); const seen: string[] = []; const snapshot = await resolveCodexModelEntitlements({ codexAccounts: [] }, { credentials: [credential("main")], - fetcher: versionFilteredBackend(seen), + // Gates exactly at the version the bundled snapshot declares for the gated models, so + // this asserts the floor is *sufficient* to return them rather than re-testing the + // arbitrary threshold the other backend uses. + fetcher: (async (input: RequestInfo | URL) => { + const url = new URL(input instanceof Request ? input.url : String(input)); + const version = url.searchParams.get("client_version") ?? ""; + seen.push(version); + const minor = Number(version.split(".")[1] ?? "0"); + return minor >= 142 ? roster("gpt-5.5", SOL, TERRA, LUNA) : roster("gpt-5.5"); + }) as typeof fetch, now: 1_000, clientVersion: null, - // Both halves of the chain unusable: no inbound version, no selected runtime. + // Both of the first two tiers unusable: no inbound version, no selected runtime. loadPersistedRuntime: () => null, }); - expect(seen).toEqual([]); - expect(snapshot.confirmedAccountIds.has("main")).toBe(false); + // The floor is asked verbatim — not `0.0.0`, and not skipped. + expect(seen).toEqual([GATED_MODEL_CLIENT_VERSION_FLOOR]); + expect(snapshot.confirmedAccountIds.has("main")).toBe(true); // Read the SNAPSHOT, not the process-wide cache: another suite in the same run can leave // a confirmed entry behind, and this assertion is about what this discovery pass proved. - expect([...availableAccountGatedNativeModels(snapshot)]).toEqual([]); - expect(snapshot.modelsByAccount.get("main")?.size).toBe(0); - // The account is still enumerated, so callers can tell "unknown" from "absent". + expect([...availableAccountGatedNativeModels(snapshot)]).toEqual([SOL, TERRA, LUNA]); expect(snapshot.modelsByAccount.has("main")).toBe(true); }); + test("the gated floor is derived from the bundled roster, not written by hand", () => { + // If the snapshot is refreshed with a model requiring a newer client, the floor must + // follow it; a hand-copied constant would silently under-ask forever. + const rows = (upstreamModelsSnapshot as { models?: Array> }).models ?? []; + const gatedFloors = rows + .filter(row => typeof row.slug === "string" && ACCOUNT_GATED_NATIVE_OPENAI_MODELS.has(row.slug)) + .map(row => row.minimal_client_version) + .filter((value): value is string => typeof value === "string"); + expect(gatedFloors.length).toBeGreaterThan(0); + expect(gatedFloors).toContain(GATED_MODEL_CLIENT_VERSION_FLOOR); + // Highest, so no gated model is asked for under a version that cannot return it. + for (const floor of gatedFloors) { + const asNumbers = (value: string) => value.split(/[.+-]/).map(Number); + const a = asNumbers(floor); + const b = asNumbers(GATED_MODEL_CLIENT_VERSION_FLOOR); + for (let i = 0; i < Math.max(a.length, b.length); i += 1) { + const left = Number.isFinite(a[i]) ? a[i]! : 0; + const right = Number.isFinite(b[i]) ? b[i]! : 0; + if (left !== right) { + expect(left).toBeLessThan(right); + break; + } + } + } + // And it is never the placeholder that caused #2886. + expect(GATED_MODEL_CLIENT_VERSION_FLOOR).not.toBe("0.0.0"); + }); + test("the placeholder 0.0.0 is never accepted as a client version", async () => { // 0.0.0 is exactly what shipped, and it is a syntactically valid version string, so the // guard has to reject it by value rather than by shape. - expect(resolveCodexEntitlementClientVersion("0.0.0", () => null)).toBeNull(); - expect(resolveCodexEntitlementClientVersion("", () => null)).toBeNull(); - expect(resolveCodexEntitlementClientVersion(null, () => null)).toBeNull(); - expect(resolveCodexEntitlementClientVersion("0.146.0", () => null)).toBe("0.146.0"); + // Rejected by value means "does not win the precedence chain": each of these falls + // through to the derived floor rather than being asked upstream verbatim. + // Every assertion here is about a SUPPLIED loader, so each bypasses the process memo that + // describes the real runtime file — otherwise one case's cached read answers the next. + const ask = (inbound: string | null, load: () => { selectedVersion?: string | null } | null) => + resolveCodexEntitlementClientVersion(inbound, load, { bypassRuntimeMemo: true }); + expect(ask("0.0.0", () => null)) + .toBe(GATED_MODEL_CLIENT_VERSION_FLOOR); + expect(ask("", () => null)) + .toBe(GATED_MODEL_CLIENT_VERSION_FLOOR); + expect(ask(null, () => null)) + .toBe(GATED_MODEL_CLIENT_VERSION_FLOOR); + expect(ask("0.146.0", () => null)).toBe("0.146.0"); // The inbound value wins over the persisted runtime; the runtime is the sync fallback. - expect(resolveCodexEntitlementClientVersion("0.146.0", () => ({ selectedVersion: "0.120.0" }))) - .toBe("0.146.0"); - expect(resolveCodexEntitlementClientVersion(null, () => ({ selectedVersion: "0.145.1" }))) - .toBe("0.145.1"); - expect(resolveCodexEntitlementClientVersion(null, () => ({ selectedVersion: "0.0.0" }))).toBeNull(); + expect(ask("0.146.0", () => ({ selectedVersion: "0.120.0" }))).toBe("0.146.0"); + expect(ask(null, () => ({ selectedVersion: "0.145.1" }))).toBe("0.145.1"); + // A persisted `0.0.0` is the same placeholder and must not be preferred over the floor. + expect(ask(null, () => ({ selectedVersion: "0.0.0" }))) + .toBe(GATED_MODEL_CLIENT_VERSION_FLOOR); // A persisted-state read that throws must not take entitlement down with it. - expect(resolveCodexEntitlementClientVersion(null, () => { throw new Error("unreadable"); })).toBeNull(); + expect(ask(null, () => { throw new Error("unreadable"); })) + .toBe(GATED_MODEL_CLIENT_VERSION_FLOOR); + // isUsableCodexClientVersion is the by-value guard the chain relies on. + expect(isUsableCodexClientVersion("0.0.0")).toBe(false); + expect(isUsableCodexClientVersion("0.142.2")).toBe(true); + // Every spelling of an all-zero core makes the same claim `0.0.0` does, so rejecting only + // the exact string would leave the defect reachable through a variant. + for (const zeroish of ["0", "0.0", "00.0.0", "0.0.0-dev", "0.0.0.0", " 0.0.0 "]) { + expect(isUsableCodexClientVersion(zeroish)).toBe(false); + expect(ask(zeroish, () => null)).toBe(GATED_MODEL_CLIENT_VERSION_FLOOR); + } + // Bounded, because the value is interpolated into an outbound URL. + expect(isUsableCodexClientVersion(`0.${"9".repeat(120)}`)).toBe(false); + // A leading-zero segment with a nonzero core is still a real version. + expect(isUsableCodexClientVersion("00.142.2")).toBe(true); + }); + + test("the persisted runtime version is not re-read from disk on every resolution", () => { + // Tier 2 reads codex-runtime.json, and it is consulted on every gated Direct authorization + // and every /v1/models resolution — including when the roster cache is hot and the answer + // needs no I/O. Without a memo that is a synchronous readFileSync on the request path. + let reads = 0; + const loader = () => { + reads += 1; + return { selectedVersion: "0.147.3" }; + }; + + expect(resolveCodexEntitlementClientVersion(null, loader, { now: 1_000 })).toBe("0.147.3"); + expect(resolveCodexEntitlementClientVersion(null, loader, { now: 1_200 })).toBe("0.147.3"); + expect(resolveCodexEntitlementClientVersion(null, loader, { now: 3_000 })).toBe("0.147.3"); + // Three resolutions inside the memo window, one read. + expect(reads).toBe(1); + + // Past the window the file is consulted again, so a runtime switch is still picked up. + expect(resolveCodexEntitlementClientVersion(null, loader, { now: 20_000 })).toBe("0.147.3"); + expect(reads).toBe(2); + + // An inbound version short-circuits before tier 2, so no read happens at all. + expect(resolveCodexEntitlementClientVersion("0.150.0", loader, { now: 40_000 })).toBe("0.150.0"); + expect(reads).toBe(2); + + // The bypass is what lets a caller ask about a loader other than the real runtime file. + expect(resolveCodexEntitlementClientVersion(null, () => ({ selectedVersion: "0.149.9" }), { + bypassRuntimeMemo: true, + })).toBe("0.149.9"); }); test("a cached roster is projected only for the version it was fetched under", async () => { @@ -279,4 +371,85 @@ describe("entitlement client version (#2886)", () => { expect([...cachedAvailableAccountGatedNativeModels(1_100, undefined, "0.0.0")]) .toEqual([SOL, TERRA, LUNA]); }); + + // The projection test above seeds the cache directly, so it cannot see the cache-hit key or + // the in-flight key — both survived being reverted while it stayed green. These two drive + // the real write path instead. A Direct caller's credential identity is derived from its own + // bearer token (`direct:`), so it satisfies the identity guard that decides whether a + // completed flight is allowed to write, which a synthetic pool credential never does. + function directHeaders(token: string): Headers { + return new Headers({ authorization: `Bearer ${token}`, "chatgpt-account-id": "acct-1" }); + } + + test("a roster fetched under one version is refetched for another, not reused", async () => { + const asked: string[] = []; + const backend = (async (input: RequestInfo | URL) => { + const url = new URL(input instanceof Request ? input.url : String(input)); + asked.push(url.searchParams.get("client_version") ?? ""); + return roster(SOL); + }) as typeof fetch; + + // Same account, same credential, same instant — only the version differs. + expect(await isDirectCallerEntitledToCodexModel(directHeaders("tok-refetch"), SOL, { + fetcher: backend, now: 1_000, clientVersion: "0.146.0", + })).toBe(true); + // Second ask under the SAME version is served from cache: no new request. + expect(await isDirectCallerEntitledToCodexModel(directHeaders("tok-refetch"), SOL, { + fetcher: backend, now: 1_000, clientVersion: "0.146.0", + })).toBe(true); + expect(asked).toEqual(["0.146.0"]); + + // A different version is a different question and must reach upstream again, even though + // the entry is still well within its TTL. + expect(await isDirectCallerEntitledToCodexModel(directHeaders("tok-refetch"), SOL, { + fetcher: backend, now: 1_000, clientVersion: "0.150.0", + })).toBe(true); + expect(asked).toEqual(["0.146.0", "0.150.0"]); + }); + + test("two versions in flight for one account do not overwrite each other's evidence", async () => { + // The failure this pins: with an account-only cache key, the LATER-completing version + // overwrites the earlier one, and the unversioned projection readers in catalog/metadata + // then publish whichever landed last rather than what each client actually proved. + const release: Array<() => void> = []; + const backend = (async (input: RequestInfo | URL) => { + const url = new URL(input instanceof Request ? input.url : String(input)); + const version = url.searchParams.get("client_version") ?? ""; + // The newer client is entitled; the older one is not. + const body = version === "0.150.0" ? roster(SOL, TERRA) : roster("gpt-5.5"); + await new Promise(resolve => release.push(resolve)); + return body; + }) as typeof fetch; + + const newer = isDirectCallerEntitledToCodexModel(directHeaders("tok-race"), SOL, { + fetcher: backend, now: 1_000, clientVersion: "0.150.0", + }); + const older = isDirectCallerEntitledToCodexModel(directHeaders("tok-race"), SOL, { + fetcher: backend, now: 1_000, clientVersion: "0.140.0", + }); + // Let both requests reach the backend, then complete the NEWER one first so the older, + // model-less roster is the last write. + while (release.length < 2) await new Promise(resolve => setTimeout(resolve, 0)); + release[0]!(); + release[1]!(); + + expect(await newer).toBe(true); + expect(await older).toBe(false); + + // Each version's evidence survives independently: the late, empty roster did not erase + // the newer client's confirmation. + expect([...cachedAvailableAccountGatedNativeModels(1_100, undefined, "0.150.0")]).toEqual([]); + // Direct entries are excluded from the CATALOG projection by design, so assert through the + // entitlement check itself — both answers must still be served from cache, unchanged. + expect(await isDirectCallerEntitledToCodexModel(directHeaders("tok-race"), SOL, { + fetcher: (async () => { throw new Error("must be served from cache"); }) as typeof fetch, + now: 1_000, + clientVersion: "0.150.0", + })).toBe(true); + expect(await isDirectCallerEntitledToCodexModel(directHeaders("tok-race"), SOL, { + fetcher: (async () => { throw new Error("must be served from cache"); }) as typeof fetch, + now: 1_000, + clientVersion: "0.140.0", + })).toBe(false); + }); }); From 8ce98df293f23622113383a2ebd7c814f74591ef Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 19:58:22 +0900 Subject: [PATCH 3/6] fix(codex): bound entitlement cache growth along the new client-version axis Putting client_version in the cache key introduced a cardinality knob reachable from an inbound request. With the previous flat per-key budget, one caller cycling client_version filled its whole eviction class and pushed unrelated accounts' confirmed grants out. That is the fail-closed catalog flapping the two-class budget was introduced to prevent, reached along a new axis; a probe showed a victim entry inside its TTL being refetched after 90 versions from one other caller. Two bounds now apply on write. An account keeps at most 4 versions, so a caller cycling versions evicts only its own older entries. The class budget then counts distinct accounts rather than keys, so one account's versions cannot spend another's share, and the account just written is never the victim. One of the two tests added here initially passed vacuously. Built on a synthetic pool credential, it never stored anything: currentCredentialIdentity rejects such a credential, so the completed flight is not allowed to write and the assertion held with an empty cache. Both tests now drive the Direct-caller path, whose credential identity is derived from its own bearer token and therefore satisfies that guard. A third test pins the account-counting budget, which the first two could not distinguish from key-counting. Focused suites: 342 pass / 0 fail. Mutations: drop the per-account bound, and count keys instead of accounts -- each turns a distinct test red. --- src/codex/model-entitlements.ts | 44 +++++++++++--- tests/codex-model-entitlements.test.ts | 83 ++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 9 deletions(-) diff --git a/src/codex/model-entitlements.ts b/src/codex/model-entitlements.ts index 4b77b31998..66d2414273 100644 --- a/src/codex/model-entitlements.ts +++ b/src/codex/model-entitlements.ts @@ -162,6 +162,19 @@ const MODEL_ROSTER_FAILURE_TTL_MS = 15_000; const MODEL_ROSTER_TIMEOUT_MS = 8_000; const MODEL_ROSTER_MAX_BYTES = 2 * 1024 * 1024; const MODEL_ROSTER_CACHE_MAX = 64; + +/** + * Versions retained per account. + * + * `client_version` arrives on the inbound `/v1/models` request, so making it part of the cache + * key handed callers a knob on key cardinality. With a flat per-key budget, one account cycling + * 64 versions filled its whole class and pushed other accounts' confirmed grants out — the same + * fail-closed catalog flapping the two-class budget exists to prevent, reached by a different + * axis. A handful of versions per account is all any real deployment needs (a client, a runtime, + * the floor), and the budget below counts ACCOUNTS so one noisy account cannot spend another's + * share. + */ +const MODEL_ROSTER_VERSIONS_PER_ACCOUNT_MAX = 4; const DIRECT_CALLER_ACCOUNT_PREFIX = "__direct_codex__:"; export interface CodexModelEntitlementCredentialSnapshot { @@ -250,17 +263,30 @@ function boundedCacheSet(accountId: string, value: CachedAccountModels): void { accountModelsCache.set(key, value); const isDirect = (candidate: string): boolean => accountIdOfCacheKey(candidate).startsWith(DIRECT_CALLER_ACCOUNT_PREFIX); + + // First, bound THIS account's versions, so a caller cycling client_version evicts only its + // own older entries and never reaches another account's evidence. + const ownKeys = [...accountModelsCache.keys()].filter(k => accountIdOfCacheKey(k) === accountId); + for (const stale of ownKeys.slice(0, Math.max(0, ownKeys.length - MODEL_ROSTER_VERSIONS_PER_ACCOUNT_MAX))) { + accountModelsCache.delete(stale); + } + + // Then bound the class by DISTINCT ACCOUNTS. Counting keys would let one account's versions + // consume the budget; counting accounts keeps each account's share independent of how many + // versions any other account is using. const evictClass = (direct: boolean): void => { - let count = 0; - for (const key of accountModelsCache.keys()) if (isDirect(key) === direct) count += 1; - while (count > MODEL_ROSTER_CACHE_MAX) { - let oldest: string | undefined; - for (const key of accountModelsCache.keys()) { - if (isDirect(key) === direct) { oldest = key; break; } + const accountsInOrder: string[] = []; + for (const key of accountModelsCache.keys()) { + if (isDirect(key) !== direct) continue; + const account = accountIdOfCacheKey(key); + if (!accountsInOrder.includes(account)) accountsInOrder.push(account); + } + // Never evict the account just written, even if it is the least-recently-inserted. + for (const victim of accountsInOrder.slice(0, Math.max(0, accountsInOrder.length - MODEL_ROSTER_CACHE_MAX))) { + if (victim === accountId) continue; + for (const key of [...accountModelsCache.keys()]) { + if (accountIdOfCacheKey(key) === victim) accountModelsCache.delete(key); } - if (oldest === undefined) break; - accountModelsCache.delete(oldest); - count -= 1; } }; evictClass(accountId.startsWith(DIRECT_CALLER_ACCOUNT_PREFIX)); diff --git a/tests/codex-model-entitlements.test.ts b/tests/codex-model-entitlements.test.ts index 4108c1dba1..b55a8475ed 100644 --- a/tests/codex-model-entitlements.test.ts +++ b/tests/codex-model-entitlements.test.ts @@ -452,4 +452,87 @@ describe("entitlement client version (#2886)", () => { clientVersion: "0.140.0", })).toBe(false); }); + + test("one caller cycling client_version cannot evict another account's evidence", async () => { + // `client_version` arrives on the inbound request, so making it part of the cache key handed + // callers a knob on key cardinality. With a flat per-key budget, ONE caller cycling versions + // filled its whole eviction class and pushed unrelated accounts' confirmed grants out — the + // fail-closed catalog flapping the two-class budget exists to prevent, reached by a new axis. + // + // Asserted through the Direct path on purpose: a synthetic pool credential never satisfies + // `currentCredentialIdentity`, so a resolver call with one writes NOTHING to the cache and an + // eviction test built on it passes without ever storing an entry. (That mistake was made and + // caught here: the first version of this test was vacuous for exactly that reason.) + let fetches = 0; + const backend = (async () => { fetches += 1; return roster(SOL); }) as typeof fetch; + const ask = (token: string, version: string) => isDirectCallerEntitledToCodexModel( + directHeaders(token), + SOL, + { fetcher: backend, now: 1_000, clientVersion: version }, + ); + + // The victim's entry is genuinely cached: a second identical ask does not refetch. + expect(await ask("tok-victim", "0.146.0")).toBe(true); + const afterVictim = fetches; + expect(await ask("tok-victim", "0.146.0")).toBe(true); + expect(fetches).toBe(afterVictim); + + // One noisy caller, far more distinct versions than the per-class account budget. + for (let i = 0; i < 90; i += 1) await ask("tok-noisy", `0.${150 + i}.0`); + + // The victim is still inside its TTL, so this must be a cache hit, not a refetch. + const beforeRecheck = fetches; + expect(await ask("tok-victim", "0.146.0")).toBe(true); + expect(fetches).toBe(beforeRecheck); + }); + + test("a single account retains only a bounded number of versions", async () => { + // The per-account bound is what makes the class budget safe. Without it, one account's + // versions grow without limit inside its own class. + let fetches = 0; + const backend = (async () => { fetches += 1; return roster(SOL); }) as typeof fetch; + const ask = (version: string) => isDirectCallerEntitledToCodexModel( + directHeaders("tok-bounded"), + SOL, + { fetcher: backend, now: 1_000, clientVersion: version }, + ); + + for (let i = 0; i < 10; i += 1) await ask(`0.${200 + i}.0`); + expect(fetches).toBe(10); + + // The most recent version is still cached. + const afterFill = fetches; + expect(await ask("0.209.0")).toBe(true); + expect(fetches).toBe(afterFill); + + // The oldest has been dropped, so it costs a refetch rather than living forever. + expect(await ask("0.200.0")).toBe(true); + expect(fetches).toBe(afterFill + 1); + }); + + test("the class budget counts accounts, not cached keys", async () => { + // The documented budget is 64 ACCOUNTS per class. Counting keys instead would silently divide + // that by the per-account version bound, so a deployment well inside the intended limit would + // start losing evidence: 20 accounts holding 4 versions each is 80 keys but only 20 accounts. + let fetches = 0; + const backend = (async () => { fetches += 1; return roster(SOL); }) as typeof fetch; + const ask = (token: string, version: string) => isDirectCallerEntitledToCodexModel( + directHeaders(token), + SOL, + { fetcher: backend, now: 1_000, clientVersion: version }, + ); + + expect(await ask("tok-first", "0.146.0")).toBe(true); + + // Twenty further accounts, each using the full per-account version allowance. + for (let account = 0; account < 20; account += 1) { + for (let v = 0; v < 4; v += 1) await ask(`tok-${account}`, `0.${300 + v}.0`); + } + + // Far more than 64 keys are now live, but far fewer than 64 accounts, so the first account's + // entry must still be served from cache. + const before = fetches; + expect(await ask("tok-first", "0.146.0")).toBe(true); + expect(fetches).toBe(before); + }); }); From 8e69cfffd5bed725db7ca3a1d464de747b1322b1 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 20:10:49 +0900 Subject: [PATCH 4/6] fix(codex): fence the runtime-version memo on an epoch and bound in-flight roster requests Second review round. Three real problems, and one test that overclaimed. The five-second memo was fenced on time alone, so a runtime switch inside the window kept answering under the replaced version. That is not merely a late answer: background sync can commit the resulting roster to disk, and a newer-to-older switch would confirm models the older client cannot drive. runtime.ts now exposes codexRuntimeStateEpoch(), which its own invalidation path already bumps as it writes, and the memo is fenced on that as well as time. A supplied loader is also auto-bypassed now rather than relying on the caller passing a flag, since forgetting it would silently answer one loader's question from another's read. The flight map was unbounded. Distinct client_version values miss the flight key by design, so a caller cycling versions could open arbitrarily many concurrent upstream requests, each holding an eight-second timer. Concurrency is now bounded per account; over the bound the answer is unconfirmed, which is the same fail-closed result a discovery failure produces. The floor-derivation test was vacuous: it compared the constant against the shipped snapshot and reimplemented the comparator, so replacing the whole derivation with the literal that snapshot happens to contain left it green. The derivation is extracted as deriveGatedClientVersionFloor and tested on independent fixtures instead. The empty case is now explicit rather than implied: gpt-daybreak-blue-latest has no minimal_client_version row at all, so the derivation returning null is a state this build can genuinely reach, and the fallback behind it is named. The concurrent test's negative half used a throwing fetcher, which production converts into an unconfirmed roster -- also false, so it could not distinguish a cache hit from a refetch. It now counts requests and inverts the answer any refetch would give. Focused suites: 344 pass / 0 fail. New mutations: return the first floor instead of the highest, drop the epoch fence, and remove the flight bound -- each turns its own test red. --- src/codex/model-entitlements.ts | 94 ++++++++++--- src/codex/runtime.ts | 12 ++ tests/codex-model-entitlements.test.ts | 177 +++++++++++++++++++------ 3 files changed, 225 insertions(+), 58 deletions(-) diff --git a/src/codex/model-entitlements.ts b/src/codex/model-entitlements.ts index 66d2414273..6dd58c172c 100644 --- a/src/codex/model-entitlements.ts +++ b/src/codex/model-entitlements.ts @@ -11,6 +11,7 @@ import { } from "./main-account"; import { ACCOUNT_GATED_NATIVE_OPENAI_MODELS } from "./catalog/native-models"; import { loadPersistedCodexRuntime } from "./runtime"; +import { codexRuntimeStateEpoch } from "./runtime"; import upstreamModelsSnapshot from "./data/upstream-models.json"; const CODEX_MODELS_ENDPOINT = "https://chatgpt.com/backend-api/codex/models"; @@ -55,21 +56,34 @@ export function isUsableCodexClientVersion(value: string | null | undefined): va * and no resolved runtime to speak for. It answers "what does this build believe the gated * models need?", which is a claim the repository can actually substantiate. */ -export const GATED_MODEL_CLIENT_VERSION_FLOOR: string = (() => { - const rows = (upstreamModelsSnapshot as { models?: Array> }).models ?? []; +export function deriveGatedClientVersionFloor( + rows: ReadonlyArray>, + gatedSlugs: ReadonlySet = ACCOUNT_GATED_NATIVE_OPENAI_MODELS, +): string | null { const floors = rows - .filter(row => typeof row.slug === "string" && ACCOUNT_GATED_NATIVE_OPENAI_MODELS.has(row.slug)) + .filter(row => typeof row.slug === "string" && gatedSlugs.has(row.slug)) .map(row => (typeof row.minimal_client_version === "string" ? row.minimal_client_version : null)) .filter((value): value is string => isUsableCodexClientVersion(value)); - const highest = floors.reduce( + return floors.reduce( (best, candidate) => (best === null || compareClientVersions(candidate, best) > 0 ? candidate : best), null, ); - // A snapshot with no gated row still has to yield a usable question; the newest floor any - // row records is a better answer than a placeholder, and an empty snapshot is not a state - // this build ships in. - return highest ?? "0.142.2"; -})(); +} + +/** + * Fallback when the snapshot records no usable gated floor. + * + * Not every gated slug carries a `minimal_client_version` — `gpt-daybreak-blue-latest` has no + * row in the current snapshot at all — so the derivation can legitimately come back empty as the + * gated set changes. This is the last resort behind it, and it is still a version upstream can + * filter on rather than the placeholder that caused #2886. + */ +const GATED_MODEL_CLIENT_VERSION_FLOOR_FALLBACK = "0.142.2"; + +export const GATED_MODEL_CLIENT_VERSION_FLOOR: string = + deriveGatedClientVersionFloor( + (upstreamModelsSnapshot as { models?: Array> }).models ?? [], + ) ?? GATED_MODEL_CLIENT_VERSION_FLOOR_FALLBACK; /** Numeric-segment comparison. Only used to pick the highest floor in a known-good set. */ function compareClientVersions(left: string, right: string): number { @@ -111,9 +125,11 @@ export function resolveCodexEntitlementClientVersion( options: { readonly bypassRuntimeMemo?: boolean; readonly now?: number } = {}, ): string { if (isUsableCodexClientVersion(inbound)) return inbound.trim(); - // A caller can opt out when it is asking about a loader other than the real runtime file, - // which the process-wide memo describes. - const selected = options.bypassRuntimeMemo === true + // The memo describes the real runtime file, so a caller supplying a different loader is asking + // a different question and must not be answered from it. Detected rather than left to the + // caller, because forgetting the flag would silently cross-answer. + const bypass = options.bypassRuntimeMemo === true || loadRuntime !== loadPersistedCodexRuntime; + const selected = bypass ? readRuntimeVersion(loadRuntime) : memoizedPersistedRuntimeVersion(loadRuntime, options.now ?? Date.now()); return selected ?? GATED_MODEL_CLIENT_VERSION_FLOOR; @@ -132,7 +148,7 @@ const MODEL_ROSTER_TTL_MS = 5 * 60_000; */ const RUNTIME_VERSION_MEMO_MS = 5_000; -let runtimeVersionMemo: { at: number; version: string | null } | null = null; +let runtimeVersionMemo: { at: number; epoch: number; version: string | null } | null = null; function readRuntimeVersion( loadRuntime: () => { selectedVersion?: string | null } | null, @@ -151,11 +167,18 @@ function memoizedPersistedRuntimeVersion( loadRuntime: () => { selectedVersion?: string | null } | null, now: number, ): string | null { - if (runtimeVersionMemo && now - runtimeVersionMemo.at < RUNTIME_VERSION_MEMO_MS) { + const epoch = codexRuntimeStateEpoch(); + // Time bounds staleness; the epoch makes a runtime switch invalidate this immediately, so the + // window can never answer under the version that was just replaced. + if ( + runtimeVersionMemo + && runtimeVersionMemo.epoch === epoch + && now - runtimeVersionMemo.at < RUNTIME_VERSION_MEMO_MS + ) { return runtimeVersionMemo.version; } const selected = readRuntimeVersion(loadRuntime); - runtimeVersionMemo = { at: now, version: selected }; + runtimeVersionMemo = { at: now, epoch, version: selected }; return selected; } const MODEL_ROSTER_FAILURE_TTL_MS = 15_000; @@ -175,6 +198,18 @@ const MODEL_ROSTER_CACHE_MAX = 64; * share. */ const MODEL_ROSTER_VERSIONS_PER_ACCOUNT_MAX = 4; + +/** + * Concurrent roster requests allowed per account. + * + * The cache is bounded on write, but an in-flight request is not a cache entry: distinct + * `client_version` values miss the flight key by design, so a caller cycling versions could open + * arbitrarily many concurrent upstream requests, each holding an eight-second timer. This bounds + * the concurrency itself. Exceeding it is reported as unconfirmed — the same fail-closed answer a + * discovery failure produces, and cheaper than either queueing or serving another version's + * roster. + */ +const MODEL_ROSTER_FLIGHTS_PER_ACCOUNT_MAX = 4; const DIRECT_CALLER_ACCOUNT_PREFIX = "__direct_codex__:"; export interface CodexModelEntitlementCredentialSnapshot { @@ -433,6 +468,21 @@ async function modelsForCredential( const flightKey = `${credential.accountId}\u0000${credential.credentialIdentity}\u0000${clientVersion}`; const existing = accountModelsFlights.get(flightKey); if (existing) return existing; + + // Bound concurrency per account before opening another upstream request. + let liveForAccount = 0; + for (const key of accountModelsFlights.keys()) { + if (accountIdOfCacheKey(key) === credential.accountId) liveForAccount += 1; + } + if (liveForAccount >= MODEL_ROSTER_FLIGHTS_PER_ACCOUNT_MAX) { + return { + credentialIdentity: credential.credentialIdentity, + clientVersion, + expiresAt: now, + models: new Set(), + confirmed: false, + }; + } const flight = fetchAccountModels(credential, fetcher, now, clientVersion) .then(result => { if (currentCredentialIdentity(credential.accountId) === credential.credentialIdentity) { @@ -592,6 +642,20 @@ export function resetCodexModelEntitlementCacheForTests(): void { runtimeVersionMemo = null; } +/** + * Test-only seam for the memoized tier-2 read. + * + * The memo is deliberately reachable only through the DEFAULT loader (a supplied loader is + * auto-bypassed, so it cannot be cross-answered), which leaves no way to observe memo behavior + * from a test without either touching the real state file or exposing this. + */ +export function memoizeRuntimeVersionForTests( + loadRuntime: () => { selectedVersion?: string | null } | null, + now: number, +): string | null { + return memoizedPersistedRuntimeVersion(loadRuntime, now); +} + export function seedCodexModelEntitlementsForTests( accountId: string, models: readonly string[], diff --git a/src/codex/runtime.ts b/src/codex/runtime.ts index 9680cc4db0..51150e6aa7 100644 --- a/src/codex/runtime.ts +++ b/src/codex/runtime.ts @@ -435,6 +435,18 @@ export type CodexRuntimeProcessCachePeek = let resolveCacheEpoch = 0; let resolveCache: ResolveCacheMemo | null = null; +/** + * Bumped whenever persisted runtime state is replaced or process authority is cleared. + * + * Consumers that memoize anything derived from `codex-runtime.json` — entitlement's client + * version, for one — read this instead of a timestamp, so a runtime switch invalidates them at + * the moment of the write rather than after a delay window. Exposed because the alternative was + * a time-based guess that could answer under the previous version after the file had changed. + */ +export function codexRuntimeStateEpoch(): number { + return resolveCacheEpoch; +} + function publishResolveCache(key: string, at: number, value: ResolveCodexRuntimeResult): void { const epoch = ++resolveCacheEpoch; resolveCache = { diff --git a/tests/codex-model-entitlements.test.ts b/tests/codex-model-entitlements.test.ts index b55a8475ed..832d8d3049 100644 --- a/tests/codex-model-entitlements.test.ts +++ b/tests/codex-model-entitlements.test.ts @@ -2,10 +2,12 @@ import { beforeEach, describe, expect, test } from "bun:test"; import { availableAccountGatedNativeModels, cachedAvailableAccountGatedNativeModels, + deriveGatedClientVersionFloor, entitledCodexAccountIdsForModel, GATED_MODEL_CLIENT_VERSION_FLOOR, isDirectCallerEntitledToCodexModel, isUsableCodexClientVersion, + memoizeRuntimeVersionForTests, resetCodexModelEntitlementCacheForTests, resolveCodexEntitlementClientVersion, resolveCodexModelEntitlements, @@ -13,6 +15,7 @@ import { type CodexModelEntitlementCredentialSnapshot, } from "../src/codex/model-entitlements"; import { MAIN_CODEX_ACCOUNT_ID } from "../src/codex/main-account"; +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"; @@ -257,34 +260,81 @@ describe("entitlement client version (#2886)", () => { expect(snapshot.modelsByAccount.has("main")).toBe(true); }); - test("the gated floor is derived from the bundled roster, not written by hand", () => { - // If the snapshot is refreshed with a model requiring a newer client, the floor must - // follow it; a hand-copied constant would silently under-ask forever. - const rows = (upstreamModelsSnapshot as { models?: Array> }).models ?? []; - const gatedFloors = rows - .filter(row => typeof row.slug === "string" && ACCOUNT_GATED_NATIVE_OPENAI_MODELS.has(row.slug)) - .map(row => row.minimal_client_version) - .filter((value): value is string => typeof value === "string"); - expect(gatedFloors.length).toBeGreaterThan(0); - expect(gatedFloors).toContain(GATED_MODEL_CLIENT_VERSION_FLOOR); - // Highest, so no gated model is asked for under a version that cannot return it. - for (const floor of gatedFloors) { - const asNumbers = (value: string) => value.split(/[.+-]/).map(Number); - const a = asNumbers(floor); - const b = asNumbers(GATED_MODEL_CLIENT_VERSION_FLOOR); - for (let i = 0; i < Math.max(a.length, b.length); i += 1) { - const left = Number.isFinite(a[i]) ? a[i]! : 0; - const right = Number.isFinite(b[i]) ? b[i]! : 0; - if (left !== right) { - expect(left).toBeLessThan(right); - break; - } - } - } - // And it is never the placeholder that caused #2886. + test("the gated floor derivation picks the highest usable gated version", () => { + // Asserted on INDEPENDENT fixtures, not the shipped snapshot. An earlier version of this test + // compared the constant against the bundled data and reimplemented the comparator, so it + // stayed green even if the whole derivation were replaced by the literal the fixture happens + // to contain — vacuous in exactly the way that matters. + const gated = new Set(["a", "b", "c"]); + const derive = (rows: Array>) => deriveGatedClientVersionFloor(rows, gated); + + // Highest wins, and ordering in the input does not matter. + expect(derive([ + { slug: "a", minimal_client_version: "0.98.0" }, + { slug: "b", minimal_client_version: "0.142.2" }, + { slug: "c", minimal_client_version: "0.124.0" }, + ])).toBe("0.142.2"); + expect(derive([ + { slug: "b", minimal_client_version: "0.142.2" }, + { slug: "a", minimal_client_version: "0.98.0" }, + ])).toBe("0.142.2"); + // Numeric comparison, not lexicographic: "0.98.0" must not beat "0.142.2". + expect(derive([ + { slug: "a", minimal_client_version: "0.9.0" }, + { slug: "b", minimal_client_version: "0.10.0" }, + ])).toBe("0.10.0"); + + // Non-gated rows are ignored even when they record a higher floor. + expect(derive([ + { slug: "a", minimal_client_version: "0.100.0" }, + { slug: "unrelated", minimal_client_version: "9.9.9" }, + ])).toBe("0.100.0"); + + // Unusable and missing values are skipped rather than selected. + expect(derive([ + { slug: "a", minimal_client_version: "0.0.0" }, + { slug: "b", minimal_client_version: "" }, + { slug: "c", minimal_client_version: "0.130.0" }, + ])).toBe("0.130.0"); + expect(derive([{ slug: "a" }, { slug: "b", minimal_client_version: 5 }])).toBeNull(); + expect(derive([])).toBeNull(); + + // And the shipped constant is a real, filterable version — never the #2886 placeholder. + expect(isUsableCodexClientVersion(GATED_MODEL_CLIENT_VERSION_FLOOR)).toBe(true); expect(GATED_MODEL_CLIENT_VERSION_FLOOR).not.toBe("0.0.0"); }); + test("concurrent roster requests for one account are bounded", async () => { + // Distinct client_version values miss the flight key by design, so without a bound a caller + // cycling versions could open arbitrarily many concurrent upstream requests, each holding an + // 8s timer. Over the bound the answer is unconfirmed — the same fail-closed result a discovery + // failure gives. + let opened = 0; + const gate: Array<() => void> = []; + const backend = (async () => { + opened += 1; + await new Promise(resolve => gate.push(resolve)); + return roster(SOL); + }) as typeof fetch; + + const asks = Array.from({ length: 12 }, (_, i) => isDirectCallerEntitledToCodexModel( + directHeaders("tok-flights"), + SOL, + { fetcher: backend, now: 1_000, clientVersion: `0.${400 + i}.0` }, + )); + + // Give the admitted flights a turn to reach the backend, then release them. + while (gate.length < 4) await new Promise(resolve => setTimeout(resolve, 0)); + for (const release of gate) release(); + const results = await Promise.all(asks); + + // At most the bound reached upstream; the rest were refused without a request. + expect(opened).toBeLessThanOrEqual(4); + // The refused ones are unconfirmed, not confirmed-denied by a bad roster. + expect(results.filter(Boolean).length).toBeGreaterThan(0); + expect(results.filter(Boolean).length).toBeLessThanOrEqual(4); + }); + test("the placeholder 0.0.0 is never accepted as a client version", async () => { // 0.0.0 is exactly what shipped, and it is a syntactically valid version string, so the // guard has to reject it by value rather than by shape. @@ -334,25 +384,58 @@ describe("entitlement client version (#2886)", () => { reads += 1; return { selectedVersion: "0.147.3" }; }; - + // A SUPPLIED loader is auto-bypassed — the memo describes the real runtime file, so answering + // a different loader from it would cross-answer. Each call must therefore read. expect(resolveCodexEntitlementClientVersion(null, loader, { now: 1_000 })).toBe("0.147.3"); - expect(resolveCodexEntitlementClientVersion(null, loader, { now: 1_200 })).toBe("0.147.3"); - expect(resolveCodexEntitlementClientVersion(null, loader, { now: 3_000 })).toBe("0.147.3"); - // Three resolutions inside the memo window, one read. - expect(reads).toBe(1); + expect(resolveCodexEntitlementClientVersion(null, loader, { now: 1_100 })).toBe("0.147.3"); + expect(reads).toBe(2); + + // The memo applies to the DEFAULT loader, which is the one on the request path. Count reads + // of the real state file through the seam runtime.ts exposes for it. + let defaultReads = 0; + const countingDefault = () => { + defaultReads += 1; + return loadPersistedCodexRuntime(); + }; + // Establish the memo, then assert three further resolutions inside the window are free. + memoizeRuntimeVersionForTests(countingDefault, 1_000); + expect(defaultReads).toBe(1); + memoizeRuntimeVersionForTests(countingDefault, 1_200); + memoizeRuntimeVersionForTests(countingDefault, 3_000); + expect(defaultReads).toBe(1); // Past the window the file is consulted again, so a runtime switch is still picked up. - expect(resolveCodexEntitlementClientVersion(null, loader, { now: 20_000 })).toBe("0.147.3"); - expect(reads).toBe(2); + memoizeRuntimeVersionForTests(countingDefault, 20_000); + expect(defaultReads).toBe(2); // An inbound version short-circuits before tier 2, so no read happens at all. expect(resolveCodexEntitlementClientVersion("0.150.0", loader, { now: 40_000 })).toBe("0.150.0"); expect(reads).toBe(2); + }); - // The bypass is what lets a caller ask about a loader other than the real runtime file. - expect(resolveCodexEntitlementClientVersion(null, () => ({ selectedVersion: "0.149.9" }), { - bypassRuntimeMemo: true, - })).toBe("0.149.9"); + test("persisting a new runtime invalidates the memoized version immediately", () => { + // A five-second staleness window is not merely a late answer: background sync can commit the + // wrong roster to disk inside it. A newer->older switch would confirm models the older client + // cannot drive; older->newer would deny models the account owns. The memo is therefore fenced + // on the runtime module's own epoch, which persistCodexRuntime bumps as it writes. + let version = "0.147.3"; + let reads = 0; + const loader = () => { + reads += 1; + return { selectedVersion: version }; + }; + + expect(memoizeRuntimeVersionForTests(loader, 1_000)).toBe("0.147.3"); + expect(reads).toBe(1); + // Same epoch, inside the window: memoized. + expect(memoizeRuntimeVersionForTests(loader, 1_100)).toBe("0.147.3"); + expect(reads).toBe(1); + + // The runtime is replaced. Even well inside the time window, the next read must see it. + version = "0.120.0"; + clearCodexRuntimeResolveCache(); + expect(memoizeRuntimeVersionForTests(loader, 1_200)).toBe("0.120.0"); + expect(reads).toBe(2); }); test("a cached roster is projected only for the version it was fetched under", async () => { @@ -440,17 +523,25 @@ describe("entitlement client version (#2886)", () => { // the newer client's confirmation. expect([...cachedAvailableAccountGatedNativeModels(1_100, undefined, "0.150.0")]).toEqual([]); // Direct entries are excluded from the CATALOG projection by design, so assert through the - // entitlement check itself — both answers must still be served from cache, unchanged. + // entitlement check itself. A THROWING fetcher would be useless for the negative case: + // production converts a failed fetch into an unconfirmed roster, which is also `false`, so it + // could not tell a cache hit from a refetch. Count requests, and have any refetch return the + // OPPOSITE answer, so serving from cache is the only way each assertion can hold. + let refetches = 0; + const inverted = (async (input: RequestInfo | URL) => { + refetches += 1; + const url = new URL(input instanceof Request ? input.url : String(input)); + // Inverted on purpose: 0.150.0 would become denied, 0.140.0 would become entitled. + return url.searchParams.get("client_version") === "0.150.0" ? roster("gpt-5.5") : roster(SOL); + }) as typeof fetch; + expect(await isDirectCallerEntitledToCodexModel(directHeaders("tok-race"), SOL, { - fetcher: (async () => { throw new Error("must be served from cache"); }) as typeof fetch, - now: 1_000, - clientVersion: "0.150.0", + fetcher: inverted, now: 1_000, clientVersion: "0.150.0", })).toBe(true); expect(await isDirectCallerEntitledToCodexModel(directHeaders("tok-race"), SOL, { - fetcher: (async () => { throw new Error("must be served from cache"); }) as typeof fetch, - now: 1_000, - clientVersion: "0.140.0", + fetcher: inverted, now: 1_000, clientVersion: "0.140.0", })).toBe(false); + expect(refetches).toBe(0); }); test("one caller cycling client_version cannot evict another account's evidence", async () => { From 3fc5df12e8970b65c5f2787ab5b392e6f3e68185 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 20:13:41 +0900 Subject: [PATCH 5/6] docs(devlog): record why the entitlement floor is a probe, not client-compat evidence --- ...0_issue_2886_entitlement_client_version.md | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/devlog/_plan/260829_bugpr_lane_h_residual_issues/160_issue_2886_entitlement_client_version.md b/devlog/_plan/260829_bugpr_lane_h_residual_issues/160_issue_2886_entitlement_client_version.md index 4062ee6e1f..9c59d7e6c9 100644 --- a/devlog/_plan/260829_bugpr_lane_h_residual_issues/160_issue_2886_entitlement_client_version.md +++ b/devlog/_plan/260829_bugpr_lane_h_residual_issues/160_issue_2886_entitlement_client_version.md @@ -177,6 +177,41 @@ specifically not returning `invalid model visibility target`. Named mutation: de ## What this does not claim +### The floor is an entitlement probe, not client-compatibility evidence + +An independent review called this a blocker: tier 3 asks under `0.142.2`, which is a *model +requirement*, not evidence of the installed client's version, so an entitled account can have +gated rows published into a catalog that an older externally launched Codex cannot drive — the +#2548 direction. The reasoning is sound and the risk is real. The fix is still the floor, for +four reasons that the code and the existing tests support: + +1. **The suggested alternative contradicts `dev`.** "Refuse or defer the durable catalog write + when no client version is available" is what returning `null` did, and two tests already on + `dev` fail under it: `tests/claude-models-discovery.test.ts` and + `tests/codex-catalog-sync-hardening.test.ts` ("account sync preserves an observed gated native + only after the mapped account confirms it"). Those tests encode the intended behavior — a + background sync *should* confirm entitlement and publish. A change that contradicts them is a + separate, deliberate decision, not a fix to this bug. + +2. **Tier 2 already handles the known-old-client case correctly.** If a Codex runtime has been + resolved and it is older than the gated models require, tier 2 supplies *that* version, upstream + returns no gated rows, and they stay suppressed — which is exactly right. Tier 3 is reached only + when no runtime has ever been resolved, so there is no known client to be wrong about. + +3. **The floor is the narrowest probe that can work.** It is the lowest version under which the + gated models can be returned at all. Asking under it cannot manufacture a confirmation: an + unentitled account still comes back without the rows. + +4. **Client-compatibility filtering has never existed here.** No code path in `src/` consults + `minimal_client_version`; both catalog sites delete it (`catalog/parsing.ts:486`, + `catalog/metadata.ts:502`). The proxy has never enforced client-version compatibility, so this + change does not remove a guard — it leaves a pre-existing gap where it was. + +The tradeoff, stated plainly: the failure this accepts is a model appearing for a client too old to +drive it, which surfaces as an upstream error on use. The failure it fixes is an entitled account on +a current client silently losing GPT-5.6 — the reported bug. Adding a real client-compatibility +filter is worth doing, and it is its own unit of work with its own decision about those two tests. + The reporter supplied no captured `/codex/models` response, so I cannot prove their machine took the confirmed-negative branch rather than a transient failure. Both produce the same symptom. The version-filter explanation is what the source, the version boundary, From 7427fa10efc8a796aeb6dbcd508f91c7a6359246 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 20:26:33 +0900 Subject: [PATCH 6/6] test(codex): restore mocked fetch when server startup fails The route-level client_version test installed a globalThis.fetch mock before entering the try block, so a throw from startServer would have left it installed for every later test in the file. The server is now constructed inside the try and stopped only if construction succeeded. Also repairs a sentence in the devlog that an inserted paragraph had split in two. Both from CodeRabbit review. Its third finding, bounding concurrent version-specific roster flights, is already implemented in 8e69cfffd (MODEL_ROSTER_FLIGHTS_PER_ACCOUNT_MAX). --- .../160_issue_2886_entitlement_client_version.md | 2 +- tests/claude-models-discovery.test.ts | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/devlog/_plan/260829_bugpr_lane_h_residual_issues/160_issue_2886_entitlement_client_version.md b/devlog/_plan/260829_bugpr_lane_h_residual_issues/160_issue_2886_entitlement_client_version.md index 9c59d7e6c9..f2fbf9e581 100644 --- a/devlog/_plan/260829_bugpr_lane_h_residual_issues/160_issue_2886_entitlement_client_version.md +++ b/devlog/_plan/260829_bugpr_lane_h_residual_issues/160_issue_2886_entitlement_client_version.md @@ -98,6 +98,7 @@ both take an explicit client version. `accountModelsCache` is keyed by account ID alone, with credential identity stored as a discriminator (`:30`, `:216`); the flight key is account plus credential identity (`:223`). Version must join both, or a roster fetched under one version keeps answering +for another until the TTL expires. The first attempt kept the account-only **cache key** and merely compared the stored version on read. Review showed that is not equivalent: with two versions in flight for one account, @@ -105,7 +106,6 @@ the later-completing one overwrites the earlier, and the *unversioned* projectio `src/codex/catalog/metadata.ts:424,514` then publish whichever landed last rather than what each client proved. The key itself is now `account\u0000version`, with account-scoped invalidation walking every version's entry so a credential change still clears all of them. -for another until the TTL expires. `cachedAvailableAccountGatedNativeModels` scans every cache entry (`:331`). Once two versions can be retained at once, that scan will leak a newer roster into an older diff --git a/tests/claude-models-discovery.test.ts b/tests/claude-models-discovery.test.ts index 43a223fc79..3729ab76c9 100644 --- a/tests/claude-models-discovery.test.ts +++ b/tests/claude-models-discovery.test.ts @@ -546,8 +546,11 @@ test("the request's client_version reaches entitlement discovery (#2886)", async return originalFetch(input, init); }) as typeof fetch; - const server = startServer(0); + // Started INSIDE the try: if startServer throws, the mocked global fetch must still be + // restored, or every later test in this file inherits it. + let server: ReturnType | null = null; try { + server = startServer(0); await fetch(new URL("/v1/models?client_version=0.151.7", server.url)) .then(response => response.json()); expect(askedVersions.length).toBeGreaterThan(0); @@ -556,6 +559,6 @@ test("the request's client_version reaches entitlement discovery (#2886)", async expect(askedVersions).not.toContain("0.0.0"); } finally { globalThis.fetch = originalFetch; - await server.stop(true); + if (server) await server.stop(true); } });