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
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
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