diff --git a/src/routing/identity-domains.ts b/src/routing/identity-domains.ts index 8d7431b1806..035653f800f 100644 --- a/src/routing/identity-domains.ts +++ b/src/routing/identity-domains.ts @@ -37,13 +37,16 @@ export type IdentityDomainProvenance = "operator-declared" | "provider-documente * What a domain key is evidence FOR, which is two facts rather than one. * * Proven SEPARATION and proven SHARING are different claims, and a provider routinely - * gives the first without the second. OpenAI documents that prompt caches are not shared - * across organizations or processing regions, and in the same breath documents that - * changing keys inside one organization does not guarantee a hit. So a different - * org-or-region key proves two domains, while an identical one proves nothing: a - * positive cache inference needs the provider to actually promise the hit, and here the - * provider declines to. Inferring "shared" from an equal key would be the same guess - * this module exists to refuse, only pointed the other way. + * gives the first without the second. OpenAI's prompt-caching guide states the separating + * half outright -- "Caches are not shared across organizations and cannot be reused across + * regional processing boundaries" -- and never states a sharing half at all. The page does + * not discuss two API keys inside one organization, and what it does say about keys is that + * they "influence routing; they do not pin requests to a machine or guarantee a cache hit." + * So a different org-or-region key proves two domains, while an identical one proves + * nothing. A positive cache inference needs the provider to promise the hit, and no such + * promise exists here -- the silence is the evidence, not a documented denial. Inferring + * "shared" from an equal key would be the same guess this module exists to refuse, only + * pointed the other way. * * "separates" therefore means two different keys are two different domains while two * identical keys stay "unknown". "separates-and-shares" means the same source also @@ -130,7 +133,9 @@ export interface DeclaredCredentialGroup { * then classifies "unknown" rather than extrapolating. * * - OpenAI: rate limits are per organization and project, with model groups sharing a - * limit; prompt caches are not shared across organizations or processing regions. + * limit ("Rate limits are defined at the organization level and at the project level, not + * user level", plus the documented shared limit across a model family); prompt caches are + * not shared across organizations or regional processing boundaries. * - Anthropic: prompt cache is isolated per workspace even inside one organization. * (Cache-read tokens are also excluded from input TPM there, which is quota * accounting, not domain shape, so it does not appear here.) @@ -158,12 +163,14 @@ const PROVIDER_DOCUMENTED_DOMAINS: Record ref.organizationId !== undefined && ref.region !== undefined ? `openai:org:${ref.organizationId}:region:${ref.region}` : undefined, diff --git a/src/server/responses/account-change-state.ts b/src/server/responses/account-change-state.ts index c2b362668bc..71a24e26fe5 100644 --- a/src/server/responses/account-change-state.ts +++ b/src/server/responses/account-change-state.ts @@ -17,9 +17,32 @@ import { } from "../../codex/routing"; import type { OcxParsedRequest } from "../../types"; import type { RequestLogContext } from "../request-log"; +import { formatErrorResponse } from "../../bridge/errors"; export type ConversationStateScrubReason = "account-change"; +/** + * Cause AND remedy, because this refusal does not clear itself. + * + * Dropping a `previous_response_id` costs one cold turn and the conversation continues. This is + * not that. The file reference lives in the conversation's history, so once pool rotation has + * moved a conversation carrying an attachment, every following turn presents the same reference + * and is refused the same way. A caller told only that the reference is invalid will send the + * same request back and watch the conversation appear dead, which is the one outcome a refusal + * is supposed to prevent. So the text says what happened, that it will keep happening, and the + * two things that actually end it. + * + * Names no account id, no file id, and no conversation id. + */ +export const ACCOUNT_CHANGE_FILE_SCOPE_MESSAGE = + "This conversation is now being served by a different account than the one its uploaded files " + + "were sent to, and an uploaded file can only be read by the account that received it. The " + + "request was not sent upstream, and no file was removed from it. Because the references stay " + + "in this conversation's history, later turns will be refused the same way until this is " + + "resolved: re-upload the files so they are issued by the account now serving this " + + "conversation, or start a new conversation for them. Sending the same request again " + + "unchanged will not clear it."; + export type PortabilityDenial = | "previous-response-id" | "provider-conversation-id" @@ -144,6 +167,21 @@ export function collectConversationStateCarriers(body: unknown): ConversationSta }; } +/** + * Does this body reference an uploaded file? + * + * Answerable from the body alone, which is what lets an alternate-account path ask BEFORE it + * resolves an alternate: the answer cannot depend on which account is chosen, because an + * uploaded file is readable only by the account it was sent to (#4710). + * + * `fileIds` is optional on the carrier type, so the emptiness test lives here rather than being + * rewritten at each caller. One of those rewrites already dereferenced it directly. + */ +export function conversationCarriesUploadedFiles(body: unknown): boolean { + const fileIds = collectConversationStateCarriers(body).fileIds; + return fileIds !== undefined && fileIds.length > 0; +} + /** * Drop account-bound continuation from a request body in place. Readable user @@ -231,3 +269,39 @@ export function applyAccountChangeConversationStateScrub( } return true; } + +/** + * Refuse an account change that would carry an uploaded-file reference to an account that + * cannot read it (#4710). + * + * The classifier has always called `file_id` account-bound, and the scrubber has always removed + * only `previous_response_id` and `conversation`. A body whose ONLY account-bound state was an + * uploaded file therefore reported nothing scrubbed and was replayed unchanged against the new + * account, which is the one case the safety fix was supposed to cover. + * + * Deleting the references is not the fix. A file reference is not continuation state the model + * can do without: it is content the caller attached, and silently dropping it answers a + * different question than the one that was asked, with no way for the caller to tell. Pinning + * the request to the issuing account is not available either — every call site resolves and + * materialises its credential before reaching here, and the retry sites are reached precisely + * because the issuing account just refused the request. + * + * So the move is refused, before dispatch, and the caller is told exactly what to do about it. + * A 400 rather than a 409: re-uploading is required, and a retryable status would invite the + * same request back unchanged. + */ +export function accountChangeFileReferenceRefusal( + args: Pick, +): Response | undefined { + const { body, bindingKey, servingAccountId, priorAccountId } = args; + if (!servingAccountId || !bindingKey) return undefined; + const issuer = peekConversationStateIssuer(bindingKey); + const accountChanged = (issuer != null && issuer !== servingAccountId) + || (priorAccountId != null && priorAccountId !== servingAccountId); + if (!accountChanged) return undefined; + // Checked against the carriers directly rather than through the portability verdict: that + // verdict reports the FIRST reason it finds, so a body carrying both a previous response id + // and a file reference reports only the former and the file would slip through the scrub. + if (!conversationCarriesUploadedFiles(body)) return undefined; + return formatErrorResponse(400, "invalid_request_error", ACCOUNT_CHANGE_FILE_SCOPE_MESSAGE); +} diff --git a/src/server/responses/compact.ts b/src/server/responses/compact.ts index 6b5b979d37c..c4ba12f5ef2 100644 --- a/src/server/responses/compact.ts +++ b/src/server/responses/compact.ts @@ -71,7 +71,9 @@ import { import { applyAccountChangeConversationStateScrub, conversationStateBindingFromAuth, + accountChangeFileReferenceRefusal, rememberServingConversationStateIssuer, + conversationCarriesUploadedFiles, } from "./account-change-state"; import { TokenRefreshError, @@ -789,6 +791,14 @@ export async function handleResponsesCompact( { const binding = conversationStateBindingFromAuth(authCtx, codexPoolAffinityKey(req.headers)); if (binding) { + // Refused rather than scrubbed: an uploaded file is content the caller attached, not + // continuation state the turn can do without. + const refusal = accountChangeFileReferenceRefusal({ + body: raw, + bindingKey: binding.bindingKey, + servingAccountId: binding.accountId, + }); + if (refusal) return refusal; applyAccountChangeConversationStateScrub({ body: raw, bindingKey: binding.bindingKey, @@ -1082,15 +1092,22 @@ export async function handleResponsesCompact( ].filter(Boolean); // Build the alternate COMPLETELY before cancelling the first body: if construction // throws, the first rejection is still intact and can be returned to the client. - const alternate = await resolveAlternateCompactContext({ - req, - admission, - config, - route, - selectedModelId, - excludeAccountId: authCtx.accountId, - turnAdmissionLease, - }); + // The same reasoning refuses an uploaded-file move here: no alternate can read a file the + // issuing account received, so which one is chosen is irrelevant and asking before the + // resolution costs nothing. It also reuses the guarantee the comment above depends on -- + // the first body is still uncancelled -- so the client gets the original rejection rather + // than an inaccessible-file error from account B (#4710). + const alternate = conversationCarriesUploadedFiles(raw) + ? undefined + : await resolveAlternateCompactContext({ + req, + admission, + config, + route, + selectedModelId, + excludeAccountId: authCtx.accountId, + turnAdmissionLease, + }); // Resolution can await a credential refresh, so the client may have gone away // while we were choosing B. Re-check before spending anything: recording A, // cancelling its body, and sending B are all observable side effects, and B's diff --git a/src/server/responses/core-codex-account.ts b/src/server/responses/core-codex-account.ts index 5d61768d424..290f9b8d3b2 100644 --- a/src/server/responses/core-codex-account.ts +++ b/src/server/responses/core-codex-account.ts @@ -60,6 +60,7 @@ import { bindRouteReasoningReplayScope } from "./core-replay"; import { conversationStateBindingFromAuth, applyAccountChangeConversationStateScrub, + conversationCarriesUploadedFiles, } from "./account-change-state"; import { recordAdapterReasoning, @@ -499,6 +500,17 @@ export async function retryCodexPoolOnAlternateAccount( recordUnmovedTransientOutcome(); return { kind: "no-alternate" }; } + // An uploaded file is readable only by the account it was sent to, so NO alternate can serve + // this body. Which account would be chosen does not change that, which is why this asks before + // the resolution rather than after it: refusing here reserves no send, cancels no response, and + // leaves the caller holding the first account's rejection to return unchanged (#4710). The + // initial-dispatch sites answer with a 400 instead, because there is no earlier response there + // to fall back to. A same-account replay -- the gated-model 400 ladder above -- is unaffected, + // since it never leaves the issuing account. + if (!retryAuthCtx && conversationCarriesUploadedFiles(parsed._rawBody)) { + recordUnmovedTransientOutcome(); + return { kind: "no-alternate" }; + } // An account move is the guarded profile's fourth send and draws the single shared // final-recovery reserve. Nothing bounded it per request before: `excludeAccountId` excludes // only the account that just failed, and the caller's recovery loop can return here after the diff --git a/src/server/responses/request-prepare.ts b/src/server/responses/request-prepare.ts index 81ffeb013f8..c5525e0f373 100644 --- a/src/server/responses/request-prepare.ts +++ b/src/server/responses/request-prepare.ts @@ -115,6 +115,7 @@ import { codexAuthContextLogLabel } from "../../codex/account-label"; import { conversationStateBindingFromAuth, applyAccountChangeConversationStateScrub, + accountChangeFileReferenceRefusal, } from "./account-change-state"; /** Parses, selects, and admits one request without changing the dispatch policy. */ @@ -921,6 +922,14 @@ export async function prepareResponsesRequest( { const binding = conversationStateBindingFromAuth(admissionState.authCtx, poolAffinityKey); if (binding) { + // Before the scrub, because a file reference is refused rather than removed and the + // refusal has to happen while there is still no dispatch to undo. + const refusal = accountChangeFileReferenceRefusal({ + body: parsed._rawBody, + bindingKey: binding.bindingKey, + servingAccountId: binding.accountId, + }); + if (refusal) return refusal; applyAccountChangeConversationStateScrub({ body: parsed._rawBody, parsed, diff --git a/structure/catalog.md b/structure/catalog.md index bf9d6e3751e..598b6a7e16e 100644 --- a/structure/catalog.md +++ b/structure/catalog.md @@ -268,9 +268,10 @@ Pool mode routes across main plus added Codex credentials. Key rules: Every domain carries `evidence` alongside its provenance: a rule that documents only that two credentials are in different domains never lets an equal key mean "shared". OpenAI's cache rule is the case that forces it — caches are documented as not shared across organizations or - processing regions, while changing keys inside one organization is documented as not - guaranteeing a hit, so a different org or region relates `distinct` and the same org and region - relates `unknown`. OpenAI quota, Anthropic workspace cache, and Azure deployment domains carry + regional processing boundaries, while no documentation states that two keys inside one + organization do share a cache, so a different org or region relates `distinct` and the same org + and region relates `unknown`. The absent promise is what withholds `shared` there, not a + documented denial. OpenAI quota, Anthropic workspace cache, and Azure deployment domains carry the sharing half as well and still relate `shared`. - **A declared credential group cannot mean two things** (`src/routing/identity-domains.ts`, `src/config.ts`). `credentialGroupIssues` is the one definition of a valid grouping: unique diff --git a/structure/transports/responses.md b/structure/transports/responses.md index 3281f73fc0d..4e84d2d8900 100644 --- a/structure/transports/responses.md +++ b/structure/transports/responses.md @@ -209,6 +209,35 @@ readable user text, and records `conversationStateScrub: "account-change"` on th without account identifiers. Once the new account issues its own state, later turns carry it normally. `canPortConversationState` is local until `src/routing/identity-domains.ts` lands. +### Uploaded files do not move between accounts + +An uploaded `file_id` has always been classified as account-bound, and the scrub has always +removed only `previous_response_id` and `conversation`. A body whose only account-bound state was +a file reference therefore reported nothing scrubbed and went to the new account unchanged. + +Deleting the reference is not the contract. A file reference is content the caller attached, not +continuation state the turn can do without, and dropping it silently answers a different question +than the one that was asked. `accountChangeFileReferenceRefusal` reads the carriers directly +rather than through the portability verdict, because that verdict reports the first reason it +finds: a body carrying both a previous response id and a file reference reports only the former, +and the file would slip through the scrub. + +The initial `/v1/responses` selection and the native compact dispatch answer HTTP 400, not a +retryable status, and the message names both the cause and the remedy. That message carries more +than the immediate failure on purpose: the reference stays in conversation history, so every later +turn is refused the same way until the files are re-uploaded under the serving account or the +conversation is restarted, and a caller told only that the reference is invalid would resend +unchanged and see a dead conversation. + +The alternate-account paths refuse the move instead of raising a status, because an earlier +response already exists to return. `conversationCarriesUploadedFiles` answers from the body alone, +so both the Responses retry helper and the compact retry ask before resolving an alternate: no +send is reserved, the first response is never cancelled, and the caller returns the original +upstream rejection. A same-account replay such as the gated-model 400 ladder is unaffected, and a +single-account install never reaches any of this because serving and issuing accounts cannot +differ. Pinning a file-carrying conversation to its issuing account is routing-affinity work and +is tracked separately. + > Decision record: [ADR-0039](../decisions/ADR-0039-responses-http-sse.md) ### Mixed-wire provider defaults diff --git a/tests/responses/responses-account-change-scrub.test.ts b/tests/responses/responses-account-change-scrub.test.ts index 4d81924c7f7..de300abacb3 100644 --- a/tests/responses/responses-account-change-scrub.test.ts +++ b/tests/responses/responses-account-change-scrub.test.ts @@ -3,6 +3,9 @@ import { applyAccountChangeConversationStateScrub, canPortConversationState, collectConversationStateCarriers, + accountChangeFileReferenceRefusal, + conversationCarriesUploadedFiles, + ACCOUNT_CHANGE_FILE_SCOPE_MESSAGE, } from "../../src/server/responses/account-change-state"; import { clearConversationStateIssuerMap, @@ -38,6 +41,84 @@ function turnBody(text = "keep this user turn") { }; } +/** + * An uploaded file is content the caller attached, not continuation state (#4710). + * + * The classifier has always called `file_id` account-bound and the scrubber has always removed + * only `previous_response_id` and `conversation`, so a body whose only account-bound state was + * a file reference reported nothing scrubbed and went to the new account unchanged. Deleting + * the reference instead would answer a different question than the one that was asked, with no + * way for the caller to tell, so the move is refused before dispatch. + */ +function fileOnlyBody() { + return { + model: "gpt-5.4", + input: [ + { type: "message", role: "user", content: [{ type: "input_file", file_id: "file_abc123" }] }, + ], + }; +} + +describe("an account change refuses uploaded-file references instead of dropping them", () => { + afterEach(() => { clearConversationStateIssuerMap(); }); + + test("a file-only body is refused when the serving account is not the issuer", () => { + rememberConversationStateIssuer(BINDING_KEY, "account-a"); + const body = fileOnlyBody(); + // The gap this closes: the scrub reports nothing to do, which used to mean "carry on". + expect(applyAccountChangeConversationStateScrub({ + body, bindingKey: BINDING_KEY, servingAccountId: "account-b", + })).toBe(false); + + const refusal = accountChangeFileReferenceRefusal({ + body, bindingKey: BINDING_KEY, servingAccountId: "account-b", + }); + expect(refusal?.status).toBe(400); + // The reference is left byte-for-byte intact: refusing is the contract, not scrubbing. + expect(collectConversationStateCarriers(body).fileIds).toEqual(["file_abc123"]); + }); + + test("the same body is served without complaint by its own issuer", () => { + rememberConversationStateIssuer(BINDING_KEY, "account-a"); + expect(accountChangeFileReferenceRefusal({ + body: fileOnlyBody(), bindingKey: BINDING_KEY, servingAccountId: "account-a", + })).toBeUndefined(); + }); + + test("an in-request move is refused on the prior account alone, with no remembered issuer", () => { + expect(accountChangeFileReferenceRefusal({ + body: fileOnlyBody(), + bindingKey: BINDING_KEY, + servingAccountId: "account-b", + priorAccountId: "account-a", + })?.status).toBe(400); + }); + + test("a file reference behind a previous_response_id is still found", () => { + rememberConversationStateIssuer(BINDING_KEY, "account-a"); + const body = { ...turnBody(), input: [...fileOnlyBody().input] } as Record; + body.previous_response_id = "resp_account_a"; + // The portability verdict reports only the FIRST reason it finds, so a body carrying both + // would have reported the response id and let the file through the scrub untouched. + expect(canPortConversationState(collectConversationStateCarriers(body))) + .toMatchObject({ portable: false, reason: "previous-response-id" }); + expect(accountChangeFileReferenceRefusal({ + body, bindingKey: BINDING_KEY, servingAccountId: "account-b", + })?.status).toBe(400); + }); + + test("a body with no file reference keeps the existing scrub-and-continue contract", () => { + rememberConversationStateIssuer(BINDING_KEY, "account-a"); + const body = turnBody() as Record; + expect(accountChangeFileReferenceRefusal({ + body, bindingKey: BINDING_KEY, servingAccountId: "account-b", + })).toBeUndefined(); + expect(applyAccountChangeConversationStateScrub({ + body, bindingKey: BINDING_KEY, servingAccountId: "account-b", + })).toBe(true); + }); +}); + function compactTurnBody(text = "keep this compact user turn") { return { model: "gpt-5.4", @@ -152,3 +233,75 @@ describe("Codex pool account-change conversation-state scrub", () => { }); }); +/** + * The alternate-account paths ask this question BEFORE they resolve an alternate (#4710). + * + * They cannot answer with a status the way the initial dispatch does, because an earlier + * response already exists and is what the caller returns. So they refuse the move instead, and + * the predicate they refuse on has to be answerable from the body alone -- no binding, no + * serving account, no issuer -- since none of those are known yet at that point. + */ +describe("uploaded-file detection answers before an alternate account is chosen (#4710)", () => { + function fileAttachment(id = "file_account_a") { + return { + type: "message", + role: "user", + content: [ + { type: "input_text", text: "what does this say?" }, + { type: "input_file", file_id: id }, + ], + }; + } + + test("a file-only body is detected from the body alone", () => { + expect(conversationCarriesUploadedFiles({ model: "gpt-5.4", input: [fileAttachment()] })).toBe(true); + }); + + test("a file behind a previous_response_id is still detected", () => { + // The portability verdict reports only the first reason it finds, so a predicate built on it + // would miss this body and let the retry sites move a file reference they already refused + // to move when it appeared alone. + expect(conversationCarriesUploadedFiles({ + model: "gpt-5.4", + previous_response_id: "resp_account_a", + input: [fileAttachment()], + })).toBe(true); + }); + + test("top-level file_id and file_ids shapes are both detected", () => { + expect(conversationCarriesUploadedFiles({ + input: [{ type: "message", role: "user", file_id: "file_1" }], + })).toBe(true); + expect(conversationCarriesUploadedFiles({ + input: [{ type: "message", role: "user", file_ids: ["file_2"] }], + })).toBe(true); + }); + + test("a body with no uploaded file lets the move proceed", () => { + // The retry sites must keep failing over for every ordinary body; a predicate that answered + // true too often would silently disable alternate-account recovery. + expect(conversationCarriesUploadedFiles(turnBody())).toBe(false); + expect(conversationCarriesUploadedFiles(compactTurnBody())).toBe(false); + expect(conversationCarriesUploadedFiles({ model: "gpt-5.4", input: [] })).toBe(false); + expect(conversationCarriesUploadedFiles({})).toBe(false); + expect(conversationCarriesUploadedFiles(undefined)).toBe(false); + expect(conversationCarriesUploadedFiles("not a body")).toBe(false); + }); + + test("an empty file_ids array is not a file reference", () => { + // fileIds is optional on the carrier type and always materialised as an array here, so the + // emptiness test has to live in one place rather than being rewritten per caller. + expect(conversationCarriesUploadedFiles({ + input: [{ type: "message", role: "user", file_ids: [] }], + })).toBe(false); + }); + + test("the refusal message names the cause, the persistence, and both remedies", () => { + // This refusal does not clear itself: the reference stays in history, so every later turn is + // refused again. A caller told only that the reference is invalid would resend unchanged. + expect(ACCOUNT_CHANGE_FILE_SCOPE_MESSAGE).toContain("later turns will be refused"); + expect(ACCOUNT_CHANGE_FILE_SCOPE_MESSAGE).toContain("re-upload the files"); + expect(ACCOUNT_CHANGE_FILE_SCOPE_MESSAGE).toContain("start a new conversation"); + expect(ACCOUNT_CHANGE_FILE_SCOPE_MESSAGE).toContain("no file was removed"); + }); +});