From d2779262d5d37ff72e6c77aa58dcd6d46293a8db Mon Sep 17 00:00:00 2001 From: JUN Date: Fri, 11 Sep 2026 10:23:11 +0900 Subject: [PATCH] pool: name the account when a refresh fails or its models vanish Two surfaces changed because one pooled account stopped being usable, and neither said so. The reporter in #4212 lost astra and sol through the proxy, found the proxy worked with ocx turned off, and concluded OpenCodex had broken. The real cause was a single account stuck on a failed credential refresh, which they eventually found themselves and then asked to be told about. The request-time refusal now names the account. refreshPoolForwardAuth and refreshPoolCompactContext both caught a non-terminal refresh failure and returned "Codex credential refresh did not complete; retry this request", which describes a transient server problem. 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 gains is the account and the exit: when retrying stops helping, that account has to be signed in again. The two call sites now share one helper, so the regular and compact contracts on this endpoint cannot drift the way they already had -- compact takes no RouteResult and so could not reach the public selector at all until its caller started passing it. The refusal says "sign in to that account again" rather than the more natural "needs reauthentication", and that is load-bearing. classifyError runs isAuthenticationMessage before it reaches the status === 503 arm, and that check is status-blind on the bare substring "authentication", which "reauthentication" contains. The friendlier wording reclassifies the body to authentication_error / invalid_api_key while the HTTP status stays 503, and Codex applies retry-after backoff only for server_is_overloaded -- so it would have quietly disabled the retry this refusal exists to ask for. A test pins the wording, not just the resulting code, because the next person to improve this sentence will not know. The name 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. When neither resolves, the sentence degrades to "the selected Codex pool account" rather than naming something opaque. The catalog drop now explains itself. A gated native model that no usable account backs is omitted from the catalog -- there is no row, so nothing downstream could attach a reason to it, and no later surface can tell "never entitled" apart from "the account broke this morning". The suppression site now says which accounts are stuck while the entitlement snapshot that produced the omission is still in scope. That explanation is deliberately narrow, in two ways. It is produced only when an account needs reauthentication, because being unentitled is the default state of most installations and explaining that on every sync would bury the case an operator can act on. And it considers only accounts that could have served the model in question: an account upstream positively denied is not the reason the model is missing, so naming it would send the operator to repair a credential that was never going to help. An unconfirmed roster stays a candidate, because that is exactly what a credential stuck on a failed refresh looks like. Catalog bytes are unchanged. The suppressed slugs are still suppressed, so the existing oracles that assert gated slugs stay absent from the written catalog keep asserting exactly that. Closes #4212 --- scripts/test-layout/layout.json | 2 + src/codex/catalog/sync.ts | 85 ++++++++++ src/server/responses/compact.ts | 26 ++- src/server/responses/core.ts | 61 ++++++- ...og-gated-native-suppression-reason.test.ts | 149 ++++++++++++++++++ tests/fixtures/test-layout-expected.json | 2 + ...responses-pool-refresh-attribution.test.ts | 124 +++++++++++++++ 7 files changed, 433 insertions(+), 16 deletions(-) create mode 100644 tests/codex-integration/catalog-gated-native-suppression-reason.test.ts create mode 100644 tests/responses/responses-pool-refresh-attribution.test.ts 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"); + }); +});