diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index fad27db650..0c07fc9a7c 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -267,6 +267,7 @@ "catalog-cursor-search.test.ts": "codex-integration", "catalog-free-pricing-status.test.ts": "codex-integration", "catalog-full-picker-order.test.ts": "codex-integration", + "catalog-gated-native-suppression-reason.test.ts": "codex-integration", "catalog-go-exact-efforts.test.ts": "codex-integration", "catalog-hub-context-window.test.ts": "codex-integration", "catalog-input-modality-enum.test.ts": "codex-integration", @@ -1084,6 +1085,7 @@ "responses-parser-malformed-content.test.ts": "responses", "responses-parser.test.ts": "responses", "responses-pool-401-refresh.test.ts": "responses", + "responses-pool-refresh-attribution.test.ts": "responses", "responses-reasoning-summary-passthrough.test.ts": "responses", "responses-reasoning-summary-rewrite.test.ts": "responses", "responses-routed-web-search-fields.test.ts": "responses", diff --git a/src/codex/catalog/sync.ts b/src/codex/catalog/sync.ts index 087b659140..074d9ddef0 100644 --- a/src/codex/catalog/sync.ts +++ b/src/codex/catalog/sync.ts @@ -42,6 +42,8 @@ import { resolveCodexModelEntitlements, type CodexModelEntitlementSnapshot, } from "../model-entitlements"; +import { isAccountNeedsReauth } from "../account-runtime-state"; +import { codexAccountLogLabel, fallbackCodexAccountLogLabel } from "../account-label"; import { CODEX_CUSTOM_MODEL_CATALOG_KIND, CODEX_PROVIDER_MODEL_CATALOG_KIND, activeCodexModelsCachePath, applyCatalogMetadata, applyMultiAgentMode, applyNativeOpenAiContextOverride, applyRoutedCodexToolMode, catalogBackupPathFor, catalogHasRoutedEntries, catalogModelSlug, ensureStrictCatalogFields, findNativeTemplate, findSupportedNativeTemplate, isDefaultCatalogPath, isRoutedModelCompatibilityExcluded, legacyCatalogBackupPath, normalizeRoutedCatalogEntry, normalizeServiceTiers, readCatalog, readCatalogBackup, readCodexCatalogPath, readCodexCatalogPathForHome, readConfiguredAutoReviewModel, readNativeBaseline } from "./parsing"; @@ -1708,6 +1710,75 @@ export function finalizeAutoReviewModelOverride( return applyAutoReviewModelOverride(models, readConfiguredAutoReviewModel(), sourceModels); } +/** + * Why an account-gated native model stopped being offered, but only when the answer is one the + * operator can act on. + * + * Suppression is an omission: the row is never built, so there is no catalog entry for a reason + * to ride on and no downstream consumer that could explain it later. #4212's reporter watched + * their models disappear and reasonably concluded the proxy was broken, because every surface + * that changed said nothing about the account that caused it. + * + * Returns `undefined` for the ordinary case — an account that is simply not entitled to a gated + * model. That is the default state for most installations, it is not news, and warning about it + * on every sync would bury the one case that matters. A credential the operator must repair is + * the case that matters, so that is the only one this speaks up about. + * + * Accounts are named with the durable `p`-prefixed log label, the same identifier the dashboard + * shows, never the raw pool id or the email. + */ +export function gatedNativeReauthSuppressionReason(args: { + snapshot: CodexModelEntitlementSnapshot; + slug: string; + eligibleAccountIds?: ReadonlySet; + needsReauth: (accountId: string) => boolean; + label: (accountId: string) => string; +}): string | undefined { + const observed = [...args.snapshot.modelsByAccount.keys()] + .filter(accountId => !args.eligibleAccountIds || args.eligibleAccountIds.has(accountId)) + // Only accounts that could actually have served THIS model. An account upstream positively + // denied is not why the model is missing, and blaming it would send the operator to repair a + // credential that was never going to help. `unknown` has to stay in: an account whose roster + // could not be confirmed reports `unknown` rather than `granted`, and a credential stuck on + // a failed refresh is exactly that account. + .filter(accountId => ( + codexModelEntitlementStateForAccount(args.snapshot, accountId, args.slug) !== "denied" + )); + const stuck = observed.filter(accountId => args.needsReauth(accountId)); + if (stuck.length === 0) return undefined; + const names = stuck.map(accountId => args.label(accountId)).sort().join(", "); + return stuck.length === observed.length + ? `every Codex account that could serve it needs reauthentication (${names})` + : `${stuck.length} of ${observed.length} Codex accounts that could serve it need reauthentication (${names})`; +} + +/** Durable, operator-facing label for a pool account id; never the raw id or the email. */ +function gatedNativeAccountLabel(config: OcxConfig, accountId: string): string { + // Direct mode narrows eligibility to the native main credential, so this is the account most + // likely to be named here. `codexAuthContextLogLabel` calls it "main" everywhere else; hashing + // it into a `p`-prefixed digest would name the one account the operator cannot look up. + if (accountId === MAIN_CODEX_ACCOUNT_ID) return "main"; + const account = (config.codexAccounts ?? []).find(candidate => candidate.id === accountId); + return account ? codexAccountLogLabel(account) : fallbackCodexAccountLogLabel(accountId); +} + +const warnedGatedNativeSuppression = new Set(); + +/** Test seam: the warn-once memory is process-global, so a case needs to be able to clear it. */ +export function resetGatedNativeSuppressionWarningsForTests(): void { + warnedGatedNativeSuppression.clear(); +} + +function warnGatedNativeSuppressedOnce(slug: string, reason: string): void { + const signature = `${slug}\u0000${reason}`; + if (warnedGatedNativeSuppression.has(signature)) return; + warnedGatedNativeSuppression.add(signature); + console.warn( + `[opencodex] catalog sync: ${slug} is not being offered because ${reason}. ` + + "Sign in again to restore it.", + ); +} + /** * Mescla o catálogo retido com os modelos visíveis e as configurações atuais, * incluindo os nomes nativos. Tenta preservar o backup original e usa a permissão @@ -1777,6 +1848,20 @@ function writeRetainedCatalogSync({ const unavailableGatedNativeSlugs = new Set([...ACCOUNT_GATED_NATIVE_OPENAI_MODELS].filter(slug => ( !availableBareGatedNativeSlugs.has(slug) ))); + // #4212: this set is the whole record of a model vanishing, and it is a set of strings that + // nothing downstream ever asks a question of. Explain it here, while the entitlement snapshot + // that produced it is still in scope, because after this point the model is simply absent and + // no later surface can tell "never entitled" apart from "the account broke this morning". + for (const slug of unavailableGatedNativeSlugs) { + const reason = gatedNativeReauthSuppressionReason({ + snapshot: modelEntitlements, + slug, + eligibleAccountIds: bareEligibleAccountIds, + needsReauth: isAccountNeedsReauth, + label: accountId => gatedNativeAccountLabel(config, accountId), + }); + if (reason) warnGatedNativeSuppressedOnce(slug, reason); + } const suppressedBareNativeSlugs = new Set([ ...desktopAllowlistSuppressedNativeSlugs(config), ...unavailableGatedNativeSlugs, diff --git a/src/server/responses/compact.ts b/src/server/responses/compact.ts index 8add02babf..45bdb3fa3d 100644 --- a/src/server/responses/compact.ts +++ b/src/server/responses/compact.ts @@ -152,6 +152,7 @@ import { decodeRequestErrorResponse, handleResponses, preAuthUpstreamHostCircuitKey, + poolCredentialRefreshIncompleteResponse, upstreamHostCircuitOpenResponse, usesCodexForwardPoolAuth, } from "./core"; @@ -317,6 +318,13 @@ async function refreshPoolCompactContext(args: { authCtx: CodexAuthContext & { kind: "pool" }; provider: OcxProviderConfig; codexAccountMode?: CodexAccountMode; + /** + * Public selector for the account this refresh is for, when the request carried one. The + * caller has it and this function does not, because compact takes no `RouteResult` — which + * is the whole reason the refusal here used to be less specific than the one core returns + * for the identical failure. + */ + codexAccountNamespace?: string; substituteMainCredential: boolean; options: HandleResponsesCompactOptions; }): Promise< @@ -377,14 +385,15 @@ async function refreshPoolCompactContext(args: { if (isTerminalCompactPoolRefreshFailure(error)) { return { ok: false, quarantine: true, response: reauthResponse() }; } - const response = formatErrorResponse( - 503, - "server_busy", - "Codex credential refresh did not complete; retry this request", - ); - const headers = new Headers(response.headers); - headers.set("Retry-After", "1"); - return { ok: false, quarantine: false, response: new Response(response.body, { status: response.status, headers }) }; + return { + ok: false, + quarantine: false, + response: poolCredentialRefreshIncompleteResponse({ + authCtx, + config, + accountSelector: args.codexAccountNamespace, + }), + }; } } @@ -917,6 +926,7 @@ export async function handleResponsesCompact( authCtx: poolAuthCtx, provider: compactProvider, codexAccountMode: route.codexAccountMode, + codexAccountNamespace: route.codexAccountNamespace, substituteMainCredential, options, }) diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 425621814c..3f4cfe4345 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -2262,6 +2262,50 @@ function isTerminalPoolRefreshFailure(error: unknown): boolean { return error instanceof TokenRefreshError && (error.reason === "revoked" || error.reason === "expired"); } +/** + * The refusal an operator meets when a stored pool credential's forced refresh does not complete. + * + * A bare "retry this request" reads as a transient fault in the proxy, which is how #4212's + * reporter spent an afternoon concluding OpenCodex had broken while one of their own accounts was + * the thing that needed them. It stays a retryable 503 and stays non-quarantining, because the + * refresh genuinely may succeed and a token-endpoint 5xx must not retire a healthy account + * (#2887). What it adds is the account and the exit: when retrying stops helping, that account + * has to be signed in again. + * + * The label is a public account selector when the request carried one, otherwise the durable + * `p`-prefixed log label — never the raw pool id and never the email. Those are the identifiers + * `responses-compaction-routing.test.ts` and `codex-auth-context.test.ts` already assert must not + * reach an operator-facing surface, and an error body travels further than a log line, not less. + * When neither is resolvable the sentence degrades to "the selected Codex pool account" rather + * than naming something opaque, because a wrong name is worse than no name. + * + * The wording says "sign in to that account again" and deliberately does NOT say + * "reauthentication". `classifyError` runs `isAuthenticationMessage` before it reaches the + * `status === 503` arm, and that check is status-blind on the bare substring "authentication", + * which "reauthentication" contains. A body carrying that word is reclassified to + * `authentication_error` / `invalid_api_key` even though the HTTP status stays 503 — and Codex + * applies retry-after backoff only for `server_is_overloaded`, so the friendlier sentence would + * have quietly disabled the retry this refusal exists to ask for. `options.code` cannot buy the + * classification back; only the wording can. + */ +export function poolCredentialRefreshIncompleteResponse(args: { + authCtx: CodexAuthContext; + config: Pick; + accountSelector?: string; +}): Response { + const label = args.accountSelector ?? codexAuthContextLogLabel(args.authCtx, args.config); + const account = label ? `Codex pool account ${label}` : "the selected Codex pool account"; + const response = formatErrorResponse( + 503, + "server_busy", + `Codex credential refresh did not complete for ${account}; retry this request. ` + + "If it keeps failing, sign in to that account again.", + ); + const headers = new Headers(response.headers); + headers.set("Retry-After", "1"); + return new Response(response.body, { status: response.status, headers }); +} + /** * One forced refresh and one same-account rebuild for a stored pool credential that * upstream rejected with a pre-stream 401. `quarantine` distinguishes a dead grant, @@ -2332,14 +2376,15 @@ async function refreshPoolForwardAuth(args: { response: formatErrorResponse(401, "authentication_error", "Selected Codex account needs reauthentication"), }; } - const response = formatErrorResponse( - 503, - "server_busy", - "Codex credential refresh did not complete; retry this request", - ); - const headers = new Headers(response.headers); - headers.set("Retry-After", "1"); - return { ok: false, quarantine: false, response: new Response(response.body, { status: response.status, headers }) }; + return { + ok: false, + quarantine: false, + response: poolCredentialRefreshIncompleteResponse({ + authCtx, + config, + accountSelector: route.codexAccountNamespace, + }), + }; } } diff --git a/tests/codex-integration/catalog-gated-native-suppression-reason.test.ts b/tests/codex-integration/catalog-gated-native-suppression-reason.test.ts new file mode 100644 index 0000000000..3b24f70501 --- /dev/null +++ b/tests/codex-integration/catalog-gated-native-suppression-reason.test.ts @@ -0,0 +1,149 @@ +import { describe, expect, test } from "bun:test"; +import { gatedNativeReauthSuppressionReason } from "../../src/codex/catalog/sync"; +import type { CodexModelEntitlementSnapshot } from "../../src/codex/model-entitlements"; + +/** + * #4212: when the accounts backing an account-gated native model stop being usable, the model is + * omitted from the catalog. An omission has no row, so nothing downstream could later explain the + * disappearance — the model was simply gone, and the reporter concluded the proxy had broken. + * + * Two properties have to hold together, and they pull against each other. The explanation has to + * appear for the operator whose credential is the cause, and it has to stay silent for everyone + * else, because being unentitled to a gated model is the normal state of most installations and + * a line printed on every sync would bury the one that matters. + */ + +const SLUG = "gpt-daybreak-blue-latest"; + +interface AccountFixture { + id: string; + /** Observed roster for this account. Defaults to one that includes the gated model. */ + models?: string[]; + /** False leaves the roster unconfirmed, which is what a stuck credential looks like. */ + confirmed?: boolean; +} + +function snapshot(accounts: AccountFixture[]): CodexModelEntitlementSnapshot { + return { + modelsByAccount: new Map(accounts.map(account => [account.id, new Set(account.models ?? [SLUG])])), + clientVersionByAccount: new Map(), + confirmedAccountIds: new Set( + accounts.filter(account => account.confirmed !== false).map(account => account.id), + ), + credentialIdentities: new Map(), + }; +} + +const label = (accountId: string): string => `label-${accountId}`; +const nobodyNeedsReauth = (): boolean => false; +const everybodyNeedsReauth = (): boolean => true; + +describe("gated native suppression reason", () => { + test("stays silent when every account is healthy", () => { + expect(gatedNativeReauthSuppressionReason({ + snapshot: snapshot([{ id: "pool-a" }, { id: "pool-b" }]), + slug: SLUG, + needsReauth: nobodyNeedsReauth, + label, + })).toBeUndefined(); + }); + + test("stays silent when no account was observed at all", () => { + expect(gatedNativeReauthSuppressionReason({ + snapshot: snapshot([]), + slug: SLUG, + needsReauth: everybodyNeedsReauth, + label, + })).toBeUndefined(); + }); + + test("stays silent when the stuck account was never entitled to this model", () => { + // The ordinary install: a confirmed roster that simply does not list the gated model is a + // denial, so this account is not why the model is missing. Naming it would send the operator + // to repair a credential that was never going to produce the model. + expect(gatedNativeReauthSuppressionReason({ + snapshot: snapshot([{ id: "pool-a", models: [] }]), + slug: SLUG, + needsReauth: everybodyNeedsReauth, + label, + })).toBeUndefined(); + }); + + test("names an entitled account that is stuck", () => { + const reason = gatedNativeReauthSuppressionReason({ + snapshot: snapshot([{ id: "pool-a" }, { id: "pool-b" }]), + slug: SLUG, + needsReauth: everybodyNeedsReauth, + label, + }); + expect(reason).toContain("every Codex account that could serve it needs reauthentication"); + expect(reason).toContain("label-pool-a"); + expect(reason).toContain("label-pool-b"); + }); + + test("names an account whose roster could not be confirmed", () => { + // This is the reported shape. A credential stuck on a failed refresh cannot confirm its + // roster, so entitlement reads `unknown` rather than `granted` — the model disappears + // precisely because the evidence went missing, and that account must stay a candidate. + const reason = gatedNativeReauthSuppressionReason({ + snapshot: snapshot([{ id: "pool-a", models: [], confirmed: false }]), + slug: SLUG, + needsReauth: everybodyNeedsReauth, + label, + }); + expect(reason).toContain("every Codex account that could serve it needs reauthentication"); + expect(reason).toContain("label-pool-a"); + }); + + test("reports how many accounts are stuck when only some are", () => { + const reason = gatedNativeReauthSuppressionReason({ + snapshot: snapshot([{ id: "pool-a" }, { id: "pool-b" }]), + slug: SLUG, + needsReauth: accountId => accountId === "pool-a", + label, + }); + expect(reason).toContain("1 of 2 Codex accounts that could serve it need reauthentication"); + expect(reason).toContain("label-pool-a"); + expect(reason).not.toContain("label-pool-b"); + }); + + test("counts only accounts the caller considers eligible", () => { + // Direct mode narrows the eligible set to main. A broken pool account outside that set did + // not cause this omission. + expect(gatedNativeReauthSuppressionReason({ + snapshot: snapshot([{ id: "pool-a" }, { id: "main" }]), + slug: SLUG, + eligibleAccountIds: new Set(["main"]), + needsReauth: accountId => accountId === "pool-a", + label, + })).toBeUndefined(); + + const reason = gatedNativeReauthSuppressionReason({ + snapshot: snapshot([{ id: "pool-a" }, { id: "main" }]), + slug: SLUG, + eligibleAccountIds: new Set(["main"]), + needsReauth: accountId => accountId === "main", + label, + }); + expect(reason).toContain("every Codex account that could serve it needs reauthentication"); + expect(reason).toContain("label-main"); + }); + + test("orders names so the same failure produces the same sentence", () => { + // The warning is emitted once per distinct sentence, so an unstable order would re-warn + // about a situation that had not changed. + const forwards = gatedNativeReauthSuppressionReason({ + snapshot: snapshot([{ id: "pool-b" }, { id: "pool-a" }]), + slug: SLUG, + needsReauth: everybodyNeedsReauth, + label, + }); + const backwards = gatedNativeReauthSuppressionReason({ + snapshot: snapshot([{ id: "pool-a" }, { id: "pool-b" }]), + slug: SLUG, + needsReauth: everybodyNeedsReauth, + label, + }); + expect(forwards).toBe(backwards!); + }); +}); diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index e8e712c839..ed625c193b 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -102,6 +102,7 @@ "catalog-cursor-search.test.ts": "codex-integration", "catalog-free-pricing-status.test.ts": "codex-integration", "catalog-full-picker-order.test.ts": "codex-integration", + "catalog-gated-native-suppression-reason.test.ts": "codex-integration", "catalog-go-exact-efforts.test.ts": "codex-integration", "catalog-hub-context-window.test.ts": "codex-integration", "catalog-input-modality-enum.test.ts": "codex-integration", @@ -919,6 +920,7 @@ "responses-parser-malformed-content.test.ts": "responses", "responses-parser.test.ts": "responses", "responses-pool-401-refresh.test.ts": "responses", + "responses-pool-refresh-attribution.test.ts": "responses", "responses-reasoning-summary-passthrough.test.ts": "responses", "responses-reasoning-summary-rewrite.test.ts": "responses", "responses-routed-web-search-fields.test.ts": "responses", diff --git a/tests/responses/responses-pool-refresh-attribution.test.ts b/tests/responses/responses-pool-refresh-attribution.test.ts new file mode 100644 index 0000000000..e7cae0ae3e --- /dev/null +++ b/tests/responses/responses-pool-refresh-attribution.test.ts @@ -0,0 +1,124 @@ +import { describe, expect, test } from "bun:test"; +import { poolCredentialRefreshIncompleteResponse } from "../../src/server/responses/core"; +import type { CodexAuthContext } from "../../src/codex/auth-context"; +import type { OcxConfig } from "../../src/types"; + +/** + * #4212: a pool credential whose forced refresh does not complete used to refuse with + * "Codex credential refresh did not complete; retry this request" and nothing else. That reads + * as a fault in the proxy, so the reporter went looking for a bug in OpenCodex while one of + * their own accounts was the thing that needed them. + * + * These cases pin the two halves of the fix that can regress independently: the refusal names + * an account and states that reauthentication is the exit, and the name it uses is never an + * identifier this codebase treats as private. + */ + +const ACCOUNT_ID = "sensitive-pool-account-id"; +const ACCOUNT_EMAIL = "operator@example.test"; +const LOG_LABEL = "pa1b2c3"; + +function poolAuthCtx(): CodexAuthContext { + return { + kind: "pool", + accountId: ACCOUNT_ID, + writerGeneration: 1, + generation: 1, + accessToken: "access-token", + chatgptAccountId: "chatgpt-account-id", + }; +} + +function configWithAccount(): Pick { + return { + codexAccounts: [{ id: ACCOUNT_ID, email: ACCOUNT_EMAIL, logLabel: LOG_LABEL, isMain: false }], + }; +} + +async function errorPayload(response: Response): Promise<{ message: string; type: string; code: string }> { + const body = await response.json() as { error: { message: string; type: string; code: string } }; + return body.error; +} + +describe("pool credential refresh refusal attribution", () => { + test("names the selector the request actually used", async () => { + const response = poolCredentialRefreshIncompleteResponse({ + authCtx: poolAuthCtx(), + config: configWithAccount(), + accountSelector: "team", + }); + const error = await errorPayload(response); + expect(error.message).toContain("Codex pool account team"); + expect(error.message).toContain("sign in to that account again"); + // The selector the operator typed wins over the derived label: it is the name they can act on. + expect(error.message).not.toContain(LOG_LABEL); + }); + + test("falls back to the durable log label when the request carried no selector", async () => { + const response = poolCredentialRefreshIncompleteResponse({ + authCtx: poolAuthCtx(), + config: configWithAccount(), + }); + const error = await errorPayload(response); + expect(error.message).toContain(`Codex pool account ${LOG_LABEL}`); + expect(error.message).toContain("sign in to that account again"); + }); + + test("never puts the raw pool id or the account email in the refusal", async () => { + for (const accountSelector of [undefined, "team"]) { + const response = poolCredentialRefreshIncompleteResponse({ + authCtx: poolAuthCtx(), + config: configWithAccount(), + accountSelector, + }); + const error = await errorPayload(response); + expect(error.message).not.toContain(ACCOUNT_ID); + expect(error.message).not.toContain(ACCOUNT_EMAIL); + } + }); + + test("says nothing specific rather than naming something opaque", async () => { + const response = poolCredentialRefreshIncompleteResponse({ + authCtx: poolAuthCtx(), + config: { codexAccounts: [] }, + }); + const error = await errorPayload(response); + // An unresolvable account still gets the actionable half of the sentence. + expect(error.message).toContain("the selected Codex pool account"); + expect(error.message).toContain("sign in to that account again"); + expect(error.message).not.toContain(ACCOUNT_ID); + }); + + test("stays the retryable 503 contract it replaced", async () => { + const response = poolCredentialRefreshIncompleteResponse({ + authCtx: poolAuthCtx(), + config: configWithAccount(), + accountSelector: "team", + }); + expect(response.status).toBe(503); + expect(response.headers.get("Retry-After")).toBe("1"); + // Naming the account must not reclassify the refusal. Codex applies retry-after backoff only + // for server_is_overloaded, so a message-driven remap here would silently drop the retry. + const error = await errorPayload(response); + expect(error.type).toBe("server_error"); + expect(error.code).toBe("server_is_overloaded"); + expect(error.message).toContain("retry this request"); + }); + + test("keeps the word that would reclassify it out of the body", async () => { + // classifyError runs isAuthenticationMessage before it reaches the status === 503 arm, and + // that check is status-blind on the bare substring "authentication" — which "reauthentication" + // contains. Saying the friendlier word here turns a retryable overload into + // authentication_error / invalid_api_key and drops Codex's retry-after backoff, so this + // guards the wording rather than only the resulting code. + const response = poolCredentialRefreshIncompleteResponse({ + authCtx: poolAuthCtx(), + config: configWithAccount(), + accountSelector: "team", + }); + expect(response.status).toBe(503); + const error = await errorPayload(response); + expect(error.message.toLowerCase()).not.toContain("authentication"); + expect(error.message.toLowerCase()).not.toContain("unauthorized"); + }); +});