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..f2fbf9e581 --- /dev/null +++ b/devlog/_plan/260829_bugpr_lane_h_residual_issues/160_issue_2886_entitlement_client_version.md @@ -0,0 +1,220 @@ +# 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 → 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` +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. + +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. + +`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'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, +specifically not returning `invalid model visibility target`. Named mutation: derive +`supportedNative` from `nativeModelRows` again. + +## 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, +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..6dd58c172c 100644 --- a/src/codex/model-entitlements.ts +++ b/src/codex/model-entitlements.ts @@ -10,13 +10,206 @@ import { type NativeMainRefreshDependencies, } 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_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; + // 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 function deriveGatedClientVersionFloor( + rows: ReadonlyArray>, + gatedSlugs: ReadonlySet = ACCOUNT_GATED_NATIVE_OPENAI_MODELS, +): string | null { + const floors = rows + .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)); + return floors.reduce( + (best, candidate) => (best === null || compareClientVersions(candidate, best) > 0 ? candidate : best), + null, + ); +} + +/** + * 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 { + 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; +} + +/** + * 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. + * 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. + * + * 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, + options: { readonly bypassRuntimeMemo?: boolean; readonly now?: number } = {}, +): string { + if (isUsableCodexClientVersion(inbound)) return inbound.trim(); + // 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; +} 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; epoch: number; version: string | null } | null = null; + +function readRuntimeVersion( + loadRuntime: () => { selectedVersion?: string | null } | null, +): string | null { + try { + const value = loadRuntime()?.selectedVersion; + return isUsableCodexClientVersion(value) ? value.trim() : null; + } catch { + // An unreadable or malformed runtime file is an absent version, not a failure worth + // propagating into entitlement resolution. + return null; + } +} + +function memoizedPersistedRuntimeVersion( + loadRuntime: () => { selectedVersion?: string | null } | null, + now: number, +): string | null { + 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, epoch, 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; 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; + +/** + * 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 { @@ -29,6 +222,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 +241,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; @@ -56,6 +266,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. * @@ -67,23 +293,38 @@ 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); + + // 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(isDirect(accountId)); + evictClass(accountId.startsWith(DIRECT_CALLER_ACCOUNT_PREFIX)); } function currentCredentialIdentity(accountId: string): string | undefined { @@ -150,6 +391,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 +401,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 +416,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 +424,7 @@ async function fetchAccountModels( } catch { return { credentialIdentity: credential.credentialIdentity, + clientVersion, expiresAt: now + MODEL_ROSTER_FAILURE_TTL_MS, models: new Set(), confirmed: false, @@ -212,18 +456,34 @@ async function modelsForCredential( credential: CodexModelEntitlementCredentialSnapshot, fetcher: typeof fetch, 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 && 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) + + // 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) { boundedCacheSet(credential.accountId, result); @@ -269,6 +529,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; @@ -278,7 +542,7 @@ export async function resolveCodexModelEntitlements( .filter((value): value is CodexModelEntitlementCredentialSnapshot => value !== null); 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 +555,17 @@ 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); const result = await modelsForCredential( credential, options.fetcher ?? fetch, options.now ?? Date.now(), + clientVersion, ); return result.confirmed && result.models.has(modelId); } @@ -331,11 +597,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) + (!eligibleAccountIds || eligibleAccountIds.has(accountIdOfCacheKey(accountId))) + && !accountIdOfCacheKey(accountId).startsWith(DIRECT_CALLER_ACCOUNT_PREFIX) + && (version === null || entry.clientVersion === version) && entry.confirmed && entry.expiresAt > now && entry.models.has(modelId) @@ -351,21 +628,43 @@ 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; +} + +/** + * 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[], 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/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/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/claude-models-discovery.test.ts b/tests/claude-models-discovery.test.ts index 0fd20b156f..3729ab76c9 100644 --- a/tests/claude-models-discovery.test.ts +++ b/tests/claude-models-discovery.test.ts @@ -514,3 +514,51 @@ 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; + + // 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); + // 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; + if (server) await server.stop(true); + } +}); diff --git a/tests/codex-model-entitlements.test.ts b/tests/codex-model-entitlements.test.ts index ca4e631da4..832d8d3049 100644 --- a/tests/codex-model-entitlements.test.ts +++ b/tests/codex-model-entitlements.test.ts @@ -2,15 +2,24 @@ import { beforeEach, describe, expect, test } from "bun:test"; import { availableAccountGatedNativeModels, cachedAvailableAccountGatedNativeModels, + deriveGatedClientVersionFloor, entitledCodexAccountIdsForModel, + GATED_MODEL_CLIENT_VERSION_FLOOR, isDirectCallerEntitledToCodexModel, + isUsableCodexClientVersion, + memoizeRuntimeVersionForTests, resetCodexModelEntitlementCacheForTests, + resolveCodexEntitlementClientVersion, resolveCodexModelEntitlements, seedCodexModelEntitlementsForTests, 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"; +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 +53,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 +68,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 +84,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 +109,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 +143,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 +159,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 +186,444 @@ 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 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")], + // 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 of the first two tiers unusable: no inbound version, no selected runtime. + loadPersistedRuntime: () => null, + }); + + // 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([SOL, TERRA, LUNA]); + expect(snapshot.modelsByAccount.has("main")).toBe(true); + }); + + 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. + // 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(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(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" }; + }; + // 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_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. + 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); + }); + + 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 () => { + // 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]); + }); + + // 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. 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: inverted, now: 1_000, clientVersion: "0.150.0", + })).toBe(true); + expect(await isDirectCallerEntitledToCodexModel(directHeaders("tok-race"), SOL, { + 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 () => { + // `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); + }); +}); 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";