Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 21 additions & 14 deletions src/routing/identity-domains.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Comment on lines +43 to +44

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Identify the routing key as prompt_cache_key

The quoted routing statement describes prompt_cache_key, not API credentials. Placing it immediately after “two API keys” and referring only to “keys” makes it sound as though the provider documents API-key routing, recreating the source-attribution ambiguity this change is meant to fix. Name prompt_cache_key explicitly or remove this sentence, since its routing behavior supplies no evidence about cache sharing between two API credentials.

Useful? React with 👍 / 👎.

* 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
Expand Down Expand Up @@ -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.)
Expand Down Expand Up @@ -158,12 +163,14 @@ const PROVIDER_DOCUMENTED_DOMAINS: Record<string, {
evidence: "separates-and-shares",
},
cache: {
// Separation only. The documentation says caches are not shared across
// organizations or processing regions, and says in the same place that changing
// keys inside one organization does not guarantee a hit. So a different org or
// region is proven distinct, while same org and region is "unknown" -- claiming
// "shared" there would assert a warm prefix the provider explicitly refuses to
// promise, and the caller would pay for it by replaying a long prompt that misses.
// Separation only, and the asymmetry is in the source. The prompt-caching guide says
// "Caches are not shared across organizations and cannot be reused across regional
// processing boundaries", which settles a DIFFERENT org or region as distinct. It
// states no counterpart for an identical one: the guide never discusses two keys in
// one organization, and a key is documented to "influence routing" without
// guaranteeing a hit. Same org and region therefore stays "unknown" -- claiming
// "shared" would assert a warm prefix the provider never promised, and the caller
// would pay for the guess by replaying a long prompt that misses.
key: (ref) => ref.organizationId !== undefined && ref.region !== undefined
? `openai:org:${ref.organizationId}:region:${ref.region}`
: undefined,
Expand Down
74 changes: 74 additions & 0 deletions src/server/responses/account-change-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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<ApplyAccountChangeConversationStateScrubArgs, "body" | "bindingKey" | "servingAccountId" | "priorAccountId">,
): 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);
}
35 changes: 26 additions & 9 deletions src/server/responses/compact.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,9 @@ import {
import {
applyAccountChangeConversationStateScrub,
conversationStateBindingFromAuth,
accountChangeFileReferenceRefusal,
rememberServingConversationStateIssuer,
conversationCarriesUploadedFiles,
} from "./account-change-state";
import {
TokenRefreshError,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
12 changes: 12 additions & 0 deletions src/server/responses/core-codex-account.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import { bindRouteReasoningReplayScope } from "./core-replay";
import {
conversationStateBindingFromAuth,
applyAccountChangeConversationStateScrub,
conversationCarriesUploadedFiles,
} from "./account-change-state";
import {
recordAdapterReasoning,
Expand Down Expand Up @@ -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
Expand Down
9 changes: 9 additions & 0 deletions src/server/responses/request-prepare.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down Expand Up @@ -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,
Expand Down
7 changes: 4 additions & 3 deletions structure/catalog.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines +271 to +274

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Correct the remaining documented-denial claims

This now says the result follows from an absent promise, but src/routing/identity-domains.ts:23-24 still says the documentation “declines to promise the hit,” and tests/routing/routing-identity-domains.test.ts:80-82 still says changing API keys is “explicitly not guaranteed to hit.” A maintainer tracing the same-org classification will therefore still encounter the unsupported denial this commit is intended to remove; update those comments too so the contract has one consistent explanation.

AGENTS.md reference: structure/AGENTS.md:L61-L63

Useful? React with 👍 / 👎.

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
Expand Down
29 changes: 29 additions & 0 deletions structure/transports/responses.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
Loading