diff --git a/devlog/_plan/260902_admin_merge_3190/031_wp3_live_inventory.md b/devlog/_plan/260902_admin_merge_3190/031_wp3_live_inventory.md new file mode 100644 index 0000000000..a4401b2daf --- /dev/null +++ b/devlog/_plan/260902_admin_merge_3190/031_wp3_live_inventory.md @@ -0,0 +1,18 @@ +# 031 — wp3 live inventory after #3190 + +Captured after `origin/dev` = `88c427522` (#3190). + +| PR | mergeable (live) | review | Disposition | +| --- | --- | --- | --- | +| #3196 | was MERGEABLE before 3190, now UNKNOWN until rebase | REVIEW_REQUIRED | **SURVIVOR** — maintainer carry of #3142, default-off `maxUpstreamBodyBytes`. gates failed only on the 091 home-path citation that #3197 already fixed. Rebase onto current `dev`, exact-head CI, admin merge, then close #3142 with credit. | +| #3142 | CONFLICTING earlier / UNKNOWN now | CHANGES_REQUESTED | CLOSE after #3196 lands (superseded carry). Do not merge both. | +| #3061 | UNKNOWN | CHANGES_REQUESTED | DEFER — parked, macos/ci red | +| #2986 | UNKNOWN | CHANGES_REQUESTED | DEFER — do not merge with #2083 | +| #2877 | UNKNOWN | CHANGES_REQUESTED | DEFER | +| #2805 | UNKNOWN | REVIEW_REQUIRED | DEFER CONFLICTING | +| #2783 | UNKNOWN | CHANGES_REQUESTED | DEFER | +| #2527 | UNKNOWN | CHANGES_REQUESTED | DEFER | +| #2366 | UNKNOWN | CHANGES_REQUESTED | DEFER | +| #2083 | UNKNOWN | APPROVED | DEFER — pair with #2986 | + +Filter result: one survivor (#3196). Not a security-boundary PR (Responses body ceiling, opt-in, no auth/credential/workflow/release/dependency install). diff --git a/devlog/_plan/260902_nonbug_adoption_backlog/020_wp2_outbound_body_guard.md b/devlog/_plan/260902_nonbug_adoption_backlog/020_wp2_outbound_body_guard.md new file mode 100644 index 0000000000..ab4fb3a3e4 --- /dev/null +++ b/devlog/_plan/260902_nonbug_adoption_backlog/020_wp2_outbound_body_guard.md @@ -0,0 +1,118 @@ +# wp2 — PR #3142 oversized outbound body refusal (+ #2511) + +PR #3142 by @olddonkey, head `df94500b6`, `CHANGES_REQUESTED`, CONFLICTING with +`dev`, base 121 commits behind. Issue #2511 (score 55) is the adjacent request. + +## The blocker is real and it is the one our own criteria care about + +@Ingwannu's live review asks for the implicit 15 MiB default to apply only to the +canonical OpenAI forward Responses destination, because `passthrough` is not +synonymous with the measured ChatGPT backend — Azure and custom key-auth Responses +adapters use it too (`src/adapters/registry.ts:78`, `src/adapters/azure.ts:5`). + +Independent investigation confirms it and finds the failure is worse than scope +creep. The default is applied whenever the key is omitted: + +``` +const maxUpstreamBodyBytes = config.maxUpstreamBodyBytes ?? DEFAULT_MAX_UPSTREAM_BODY_BYTES; +``` + +### It regresses requests that work today + +`#2473` (merged, in tree) does **not** refuse oversized turns. It sizes the WS +`response.create` frame against `CODEX_WS_CREATE_FRAME_LIMIT_BYTES` = 16 MiB − 64 KiB +and **falls back to HTTP SSE** (`src/server/responses/ws-upstream.ts:152-167,199-201`). +`tests/ws-upstream.test.ts:692` records the measured backend close at ~16,777,300 B +with 16,777,000 B completing. + +So with the PR's default and no configuration: + +| Body size | Today | After #3142 | +|-----------|-------|-------------| +| 15 MiB — 16 MiB−64 KiB | WS-eligible, succeeds | local 413 | +| 16 MiB−64 KiB — ~16.7 MB | HTTP SSE fallback, succeeds | local 413 | +| > ~16.7 MB (ChatGPT) | upstream failure | local 413 (better message) | + +The first two rows are **working requests that start failing**. That is a +regression for users who configured nothing, and it directly violates the +standing criterion that every capability is opt-in and defaults to today's +behavior. + +### The refusal shape may also be worse than today + +`#3177` (in tree, not in the PR's base) rewrites a provider HTTP 413 on a +streaming Responses turn into `response.failed` / `context_length_exceeded` +(`src/server/responses/context-overflow.ts:19-26`, `core.ts:4529-4533`), so Codex +treats it as terminal overflow and compacts. The PR returns +`formatErrorResponse(413, ...)` JSON instead, which for a streaming client is a +retryable transport error — Codex may resend the same oversized body. The PR's +stated goal is to stop exactly that loop. + +### It does not close #2511 + +#2511 asks for a **per-provider, default-off** budget that **downscales** images +then **prunes** oldest-first with a visible marker. #3142 is top-level, +default-on, and refusal-only. `closingIssuesReferences` is empty and the PR body +never mentions #2511 — correctly. These are different products; #3142 must not +be recorded as closing it. + +## Disposition: reimplement, default-off + +The measurement, the local 413 shape, the image diagnostics, the body-observation +release and the lease fix are all good work and are kept. One thing changes: the +guard is **off unless configured**. + +That is a stronger answer than the requested canonical-only default, and it +resolves @Ingwannu's blocker a fortiori: + +- no destination — canonical, Azure, or custom — inherits a ceiling measured + somewhere else; +- the #2473 HTTP fallback band keeps working; +- it matches the shape #2511 actually asked for, so the two stop contradicting; +- an operator who has hit the wall sets one integer and gets the diagnostic. + +The cost is that the diagnostic is not on by default. That is the correct trade: +a default that breaks working requests to improve an error message is not a +default, it is a regression with a nicer string. + +## File change map + +| File | Action | Change | +|------|--------|--------| +| `src/server/responses/outbound-body-guard.ts` | NEW | `checkOutboundBodySize`, `describeOutboundBodyRefusal`, image diagnostics. `limitBytes` undefined or 0 admits without measuring. No `DEFAULT_MAX_UPSTREAM_BODY_BYTES`. | +| `src/types/config.ts` | MODIFY | `maxUpstreamBodyBytes?: number` with JSDoc naming the native-Responses-passthrough scope and the default-off contract | +| `src/config.ts` | MODIFY | zod: optional non-negative integer | +| `src/server/responses/core.ts` | MODIFY | `refuseOversizedOutboundBody` inside the passthrough branch; guard at initial build, `rebuildAndRefetch`, OAuth-refresh rebuild, alternate-account retry, **and the 401 replay rebuild the PR missed** (`core.ts:4071-4080` on PR head); release body observation, host admission and probe lease; release `firstAuthCtx` when `deferFirstOutcome` | +| `src/server/request-log.ts` | MODIFY | `outbound_body_too_large` error code | +| `docs-site/.../providers.md` | MODIFY | document the key, default-off, and the passthrough-only scope | +| `tests/outbound-body-guard.test.ts` | NEW | threshold crossing, UTF-8 byte counting, unparseable body, undefined and 0 both admit | +| `tests/empty-completion-core.test.ts` | MODIFY | integration: configured limit refuses with 0 fetches and 1 observation release; **omitted key sends a 20 MiB body upstream unrefused** | + +## Scope boundary + +IN: the guard, its activation sites including the missed 401 replay, default-off, +docs, focused tests. + +OUT: image downscaling and oldest-first pruning (#2511's actual request) — a +separate feature that mutates request content and needs its own cycle. OUT: +changing the refusal into a `streamingContextOverflowResponse`; worth doing but +it is #3177's contract and belongs with that code, and with the guard off by +default the retry-loop concern no longer rides on this change. + +## Accept criteria + +1. **Omitted config sends an oversized body upstream.** Activation: integration + test with no `maxUpstreamBodyBytes` and a body far above 15 MiB asserting the + fetch happened. This is the regression the PR would have shipped. +2. Configured limit refuses with a local 413, zero upstream fetches, and the body + observation released. Activation: existing integration case. +3. `0` admits without measuring. +4. Refusal names the image count and approximate decoded megabytes when the body + parses. Activation: unit assertion on the message. +5. Every rebuild site is guarded, including the 401 replay. + +## Verifier + +`bun x tsc --noEmit` (exit 0 baseline confirmed) plus +`bun test tests/outbound-body-guard.test.ts tests/empty-completion-core.test.ts`. +Full suite forbidden by the operator. diff --git a/devlog/_plan/260902_nonbug_adoption_backlog/021_wp2_audit_r1_synthesis.md b/devlog/_plan/260902_nonbug_adoption_backlog/021_wp2_audit_r1_synthesis.md new file mode 100644 index 0000000000..938a8284de --- /dev/null +++ b/devlog/_plan/260902_nonbug_adoption_backlog/021_wp2_audit_r1_synthesis.md @@ -0,0 +1,78 @@ +# wp2 audit round 1 — synthesis + +Reviewer: grok-4.6 adversarial lane (agent `01a05e12`). +Verdict: `GO-WITH-FIXES (blockers=3)`. All three accepted. No rebuttals. + +The reviewer confirmed the plan's central claim — there is a real band above +15 MiB that succeeds today — and then corrected the evidence I used to argue it. +That correction is blocker 3 and it matters more than it looks. + +## Blocker 3 — I overstated the ceiling (ACCEPTED) + +My plan's table claimed HTTP dies at ~16.7 MB. Wrong. The 16,777,000 / +16,777,300 figures in `ws-upstream.ts:31-38` are the **WebSocket close** +measurement, and the very same comment says *"The same request body succeeds +over HTTP SSE, so the ceiling belongs to this transport alone."* Issue #2426 +records an 18.2 MB HTTP 200. + +So the regression is **larger** than I wrote, not smaller: there is no +established HTTP ceiling at all in the range the PR's default would refuse. I +was citing a WS number as if it bounded HTTP. Corrected table: + +| Body size | Today | After #3142 default | +|-----------|-------|---------------------| +| 15 MiB … frame limit − 1 | WS send succeeds | local 413 | +| >= frame limit (16 MiB − 64 KiB) | HTTP SSE fallback sends the original body; 18.2 MB observed OK | local 413 | + +This also settles the alternative the reviewer weighed: a canonical-only default +at 15 MiB is still wrong, because it would refuse working ChatGPT traffic in the +15 MiB–18.2 MB band. Default-off is not merely the safer option, it is the only +one supported by the measurements we actually have. + +## Blocker 1 — the refusal shape is a trap on the enabled path (ACCEPTED) + +I had put the #3177 mapping OUT of scope on the grounds that default-off defuses +the retry-loop concern. That reasoning is backwards. Default-off means the +**only** users who ever see this code are the ones who deliberately enabled it — +so the enabled path is the whole feature, not an edge case. + +`streamingContextOverflowResponse` (`src/server/responses/context-overflow.ts:8-16,29-50` +on `origin/dev`) emits SSE `response.failed` / `context_length_exceeded` with +`retryable: false`, and the passthrough upstream-413 path already uses it +(`core.ts:4530-4534`). A local `formatErrorResponse(413, ...)` is a retryable +transport error to Codex, which resends the same oversized body — the exact loop +the PR set out to stop. + +Correction: a streaming refusal uses `streamingContextOverflowResponse`. The +JSON 413 stays only for non-streaming requests, where it is the right shape. + +## Blocker 2 — criterion 5 had no activating test (ACCEPTED) + +"Every rebuild site is guarded, including the 401 replay" was a claim with +nothing driving it: neither named test file reaches the 401 replay, +`rebuildAndRefetch`, or the alternate-account retry. Under +C-ACTIVATION-GROUNDING-01 that is a code comment wearing an acceptance criterion. + +Correction: add an integration case that drives a rebuild path with an oversized +rebuilt body and asserts no second upstream fetch. The 401 replay gap itself is +confirmed real — unguarded at PR head `core.ts:4071-4097` and at the same place +on current `origin/dev` (`4106-4135`). + +## File-map additions from the reviewer + +- all seven `docs-site` locale copies of `providers.md`, which the PR does touch +- `src/server/responses/context-overflow.ts` as a consumer (blocker 1) +- the malformed-value warning sibling used by `upstreamHostCircuitThreshold` + (`src/config.ts:1809-1823, 2261-2270`) +- `src/server/request-log.ts` confirmed in scope: the PR adds + `RequestLogContext.errorCode`, absent from the current tree + +## Base + +Reimplementation branches from current `origin/dev` (`c87071400`), which carries +#3177. The wp1 branch is 20 commits behind that and is not a base for this work. + +## Line drift corrected + +`ws-upstream.ts:152-167` is the doc comment; the fallback is `:199-201`. +`tests/ws-upstream.test.ts:692` is `:693`. diff --git a/docs-site/src/content/docs/reference/configuration/providers.md b/docs-site/src/content/docs/reference/configuration/providers.md index 7b68b69654..0cfd77f71e 100644 --- a/docs-site/src/content/docs/reference/configuration/providers.md +++ b/docs-site/src/content/docs/reference/configuration/providers.md @@ -27,6 +27,7 @@ authenticated. | `accountPoolStickyLimit?` | `number` | `1` | New/unbound task assignments retained on one round-robin selection before advancing; the counter advances when a task is bound, not after an upstream success. Range 1–100. | | `upstreamFailoverThreshold?` | `number` | `3` | Consecutive transient failures before future new sessions fail over. Set `0` to disable. For regular Responses and native compact sends, proven pre-connection DNS/TCP reachability failures are tracked at the provider-host level: they never affect account health, account cooldowns, thread/session affinity, active-account selection, or Pool routing, and never count toward this threshold. | | `upstreamHostCircuitThreshold?` | `number` | `0` | Opt-in circuit threshold for proven pre-connection DNS/TCP failures on native OpenAI forward Responses and compact sends. `0` disables it; `1`–`20` opens a 30-second provider-origin cooldown after that many terminal logical requests. While open, requests receive `503` with `Retry-After` before account selection or upstream send; after cooldown, one half-open request is admitted. Timeouts and HTTP responses never count, and any HTTP response closes the circuit. Applies only to Codex Pool routing with no pinned account; it is inert for `codexAccountMode: "direct"` and account-qualified selectors. | +| `maxUpstreamBodyBytes?` | `number` | `0` | Opt-in ceiling, in bytes, on a serialized native Responses **passthrough** body. `0` or omitted disables it — no limit is inferred for any destination. When set, a built body above the ceiling is refused locally before the send: streaming turns receive a terminal `response.failed` / `context_length_exceeded` so the client compacts instead of resending, and non-streaming turns receive a `413` naming the size, the number of embedded `input_image` items, and roughly how many megabytes of image data they represent. Checked at every build and rebuild point, including OAuth-refresh replay and alternate-account retry. Translated adapter paths are not covered. There is deliberately no default: the only measured ceiling here belongs to the WebSocket transport, which already falls back to HTTP for oversized turns, so a default would refuse requests that currently succeed. Set it when your gateway has a known request-size limit and you would rather see an actionable local error than an opaque upstream failure. | | `modelCacheTtlMs?` | `number` | `300000` | Freshness window for the per-provider `/models` cache. | | `cacheRetention?` | `"none" \| "short" \| "long"` | `"short"` | Anthropic prompt-cache policy: disabled, 5-minute ephemeral, or 1-hour extended. | | `tokenGuardian?` | `OcxTokenGuardianConfig` | off | Optional proactive OAuth refresh and Codex-account warmup policy. | diff --git a/src/config.ts b/src/config.ts index f68244d636..c17a86d1ff 100644 --- a/src/config.ts +++ b/src/config.ts @@ -990,6 +990,12 @@ const configSchema = z.object({ .max(UPSTREAM_HOST_CIRCUIT_MAX_THRESHOLD) .optional() .catch(undefined), + // Opt-in outbound body ceiling. An invalid hand edit disables only this guard, matching the + // circuit threshold above: a malformed number must not make the proxy refuse traffic. + maxUpstreamBodyBytes: z.number().int() + .min(0) + .optional() + .catch(undefined), appOwnedMemoryBudgetMb: z.number().int() .min(MIN_APP_OWNED_MEMORY_BUDGET_MB) .max(MAX_APP_OWNED_MEMORY_BUDGET_MB) diff --git a/src/server/request-log.ts b/src/server/request-log.ts index a443ff1939..2c8d3e179c 100644 --- a/src/server/request-log.ts +++ b/src/server/request-log.ts @@ -125,6 +125,12 @@ export interface RequestLogContext { terminalHttpStatus?: number; /** Recognized structured terminal code whose exact identity must survive status mapping. */ terminalErrorCode?: typeof CYBER_POLICY_ERROR_CODE; + /** + * Proxy-owned error code for a request OpenCodex terminated locally, before or instead of an + * upstream send. Status-derived classification cannot name these: there is no upstream + * message to classify, and the status alone would read as a provider failure. + */ + errorCode?: string; /** Structured reason from `response.incomplete`; internal-only input to log classification. */ terminalIncompleteReason?: string; affinity?: "reused" | "new_bind" | "rebound" | "cleared"; @@ -925,7 +931,9 @@ export function addFinalRequestLog( const effectiveStatus = status >= 500 && logCtx.upstreamError && isClientClosedMessage(logCtx.upstreamError) ? 499 : status; - const errorCode = requestLogErrorCode( + // A locally assigned code wins: it names a refusal this proxy made itself, which no + // status-plus-upstream-message classification can reconstruct. + const errorCode = logCtx.errorCode ?? requestLogErrorCode( effectiveStatus, logCtx.upstreamError, logCtx.terminalErrorCode, diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 0240f49553..d4e45cde2a 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -7,6 +7,10 @@ import { backfillResponsesFieldsJson, } from "./responses-field-backfill"; import { checkInputAdmission } from "./input-admission"; +import { + checkOutboundBodySize, + describeOutboundBodyRefusal, +} from "./outbound-body-guard"; import { nativeContextLimits } from "../../codex/catalog"; import { describeUpstreamConnectFailure } from "./upstream-error"; import { @@ -3903,6 +3907,48 @@ async function handleResponsesInner( linkAbortSignal(upstream, options.abortSignal); const connectMs = config.connectTimeoutMs ?? 200_000; let upstreamResponse: Response; + /** + * Refuse a built body that exceeds the operator's configured ceiling, before it is sent. + * + * Unconfigured this measures nothing and returns undefined, so an unset proxy behaves + * exactly as it does today. Runs at every point a body is built or rebuilt, because a + * rebuild can produce a payload the initial check never saw. + */ + const refuseOversizedOutboundBody = ( + builtRequest: AdapterRequest, + refusalAuthCtx: CodexAuthContext = authCtx, + ): Response | undefined => { + const result = checkOutboundBodySize(builtRequest.body, config.maxUpstreamBodyBytes); + if (result.admitted) return undefined; + + // This returns before the surrounding fetch/finally owns the observation, so release + // it here or one refused body holds translator budget for the process lifetime. + builtRequest.releaseBodyObservation?.(); + upstream.abort(); + releaseUpstreamHostAdmission(hostAdmissionLease); + hostAdmissionLease = null; + releaseCodexAuthContextProbeLease(refusalAuthCtx); + logCtx.errorCode = "outbound_body_too_large"; + console.warn( + `[responses] refused an oversized outbound body: bytes=${result.bytes} limit=${result.limit} ` + + `input_images=${result.imageCount} image_bytes=${result.imageBytes} ` + + `model=${JSON.stringify(parsed.modelId)}`, + ); + // A streaming client treats HTTP 413 as a retryable transport error and resends the same + // oversized body — the reconnect loop #3177 exists to stop. Terminal overflow is the + // honest shape, and it is what the upstream-413 path already returns. + if (clientRequestedStream) { + return streamingContextOverflowResponse( + parsed._responseModelId ?? parsed.modelId, + translatorBudget, + ); + } + return formatErrorResponse( + 413, + "outbound_body_too_large", + describeOutboundBodyRefusal(result), + ); + }; const transportFailureResponse = (err: unknown): Response => { upstream.abort(); if (options.abortSignal?.aborted) { @@ -3945,6 +3991,8 @@ async function handleResponsesInner( : describeUpstreamConnectFailure(err, connectMs); return formatErrorResponse(502, "upstream_error", msg); }; + const initialBodyRefusal = refuseOversizedOutboundBody(request); + if (initialBodyRefusal) return initialBodyRefusal; try { // Transient-5xx pre-stream retry (devlog/_plan/260716_claudecode_hardening/010): // the ChatGPT backend emits transient 502/520s that an immediate retry absorbs. @@ -4019,6 +4067,8 @@ async function handleResponsesInner( retryAdapter.name, logCtx.accountLogLabel, ); + const rebuiltBodyRefusal = refuseOversizedOutboundBody(request); + if (rebuiltBodyRefusal) return { failed: rebuiltBodyRefusal }; try { return await fetchWithTransientRetry( innerRecovery => { @@ -4111,6 +4161,10 @@ async function handleResponsesInner( recordAdapterReasoning(logCtx, request); recordAdapterTier(logCtx, request); refreshUndeclaredToolGuard(request); + // The 401 replay rebuilds the body before sending, so it needs the same ceiling as + // every other build site; a replay is exactly when a grown payload reappears. + const replayBodyRefusal = refuseOversizedOutboundBody(request); + if (replayBodyRefusal) return replayBodyRefusal; noteAttemptSend(logCtx.activeAttempt, passthroughEstimate, "oauth-401"); upstreamResponse = await fetchWithHeaderTimeout( request.url, @@ -4220,6 +4274,8 @@ async function handleResponsesInner( return formatErrorResponse(400, "invalid_request_error", redactSecretString(msg)); } refreshUndeclaredToolGuard(request); + const refreshedBodyRefusal = refuseOversizedOutboundBody(request); + if (refreshedBodyRefusal) return refreshedBodyRefusal; try { upstreamResponse = await fetchWithTransientRetry( recovery => { diff --git a/src/server/responses/outbound-body-guard.ts b/src/server/responses/outbound-body-guard.ts new file mode 100644 index 0000000000..b8537e5e06 --- /dev/null +++ b/src/server/responses/outbound-body-guard.ts @@ -0,0 +1,110 @@ +/** + * Measure a built passthrough body before it is sent, so an operator can turn an opaque + * upstream failure into a local, actionable refusal. + * + * There is deliberately no default limit. The one measured ceiling in this codebase belongs + * to the WebSocket transport (`MAX_CODEX_WS_CREATE_FRAME_BYTES` in `ws-upstream.ts`), and the + * comment recording that measurement says the same body still succeeds over HTTP SSE — #2426 + * observed an 18.2 MB HTTP 200. An implicit HTTP ceiling inferred from the WS number would + * refuse requests that work today, on every passthrough destination including Azure and + * custom Responses gateways whose limits were never measured at all. The operator who hit a + * wall knows where their wall is; this guard is off until they say so. + */ + +export interface OutboundBodyGuardResult { + admitted: boolean; + /** Serialized UTF-8 bytes. Zero when the guard is disabled before measurement. */ + bytes: number; + /** The configured limit, or 0 when the guard is disabled. */ + limit: number; + imageCount: number; + /** Approximate decoded bytes represented by embedded `input_image` data URIs. */ + imageBytes: number; +} + +const MAX_DIAGNOSTIC_DEPTH = 64; + +function decodedDataUriBytes(value: unknown): number { + if (typeof value !== "string" || !value.startsWith("data:")) return 0; + const comma = value.indexOf(","); + if (comma < 0) return 0; + const payload = value.length - comma - 1; + return payload > 0 ? Math.floor((payload * 3) / 4) : 0; +} + +/** + * Walk the parsed body for `input_image` items. Bounded by depth and a seen-set because this + * runs on a body that already failed the size check, which is exactly when a pathological + * shape is most likely. + */ +function imageDiagnostics(value: unknown): { imageCount: number; imageBytes: number } { + let imageCount = 0; + let imageBytes = 0; + const seen = new WeakSet(); + + const visit = (entry: unknown, depth: number): void => { + if (depth > MAX_DIAGNOSTIC_DEPTH || entry === null || typeof entry !== "object") return; + if (seen.has(entry)) return; + seen.add(entry); + + if (!Array.isArray(entry) && (entry as Record).type === "input_image") { + imageCount += 1; + imageBytes += decodedDataUriBytes((entry as Record).image_url); + return; + } + + if (Array.isArray(entry)) { + for (const item of entry) visit(item, depth + 1); + return; + } + for (const item of Object.values(entry)) visit(item, depth + 1); + }; + + visit(value, 0); + return { imageCount, imageBytes }; +} + +/** + * `limitBytes` undefined (unconfigured) or 0 (explicitly disabled) both admit without + * measuring, so an unconfigured proxy does no work and sends exactly what it sends today. + */ +export function checkOutboundBodySize( + body: string, + limitBytes: number | undefined, +): OutboundBodyGuardResult { + if (limitBytes === undefined || limitBytes === 0) { + return { admitted: true, bytes: 0, limit: 0, imageCount: 0, imageBytes: 0 }; + } + + const bytes = Buffer.byteLength(body, "utf8"); + if (bytes <= limitBytes) { + return { admitted: true, bytes, limit: limitBytes, imageCount: 0, imageBytes: 0 }; + } + + try { + const diagnostics = imageDiagnostics(JSON.parse(body) as unknown); + return { admitted: false, bytes, limit: limitBytes, ...diagnostics }; + } catch { + return { admitted: false, bytes, limit: limitBytes, imageCount: 0, imageBytes: 0 }; + } +} + +function megabytes(bytes: number): string { + return (bytes / (1024 * 1024)).toFixed(1); +} + +/** + * Name the likely cause rather than only the number. Accumulated replayed images are the + * common way a thread crosses a byte ceiling while its token count still looks healthy, and + * the remedy is not something the user can guess from a size alone. + */ +export function describeOutboundBodyRefusal(result: OutboundBodyGuardResult): string { + const imageDetail = result.imageCount > 0 + ? ` It contains ${result.imageCount} input_image item${result.imageCount === 1 ? "" : "s"} ` + + `representing about ${megabytes(result.imageBytes)} MB of decoded embedded image data; ` + + "accumulated replayed images are the likely cause." + : " Large inputs accumulated across replayed turns can cause this."; + return `The serialized outbound request is ${megabytes(result.bytes)} MB, ` + + `above the configured ${megabytes(result.limit)} MB limit.${imageDetail} ` + + "Start a new session or compact the conversation before retrying."; +} diff --git a/src/types/config.ts b/src/types/config.ts index 169506d266..bf837abda9 100644 --- a/src/types/config.ts +++ b/src/types/config.ts @@ -702,6 +702,17 @@ export interface OcxConfig { * Default 0 (disabled); range 0..20. The circuit never counts timeouts or HTTP responses. */ upstreamHostCircuitThreshold?: number; + /** + * Opt-in ceiling, in bytes, for a serialized native Responses **passthrough** body. When the + * built body exceeds it OpenCodex refuses locally instead of sending, naming the size and any + * embedded image payload. Translated adapter paths are not covered. + * + * Omitted or 0 = disabled, which is the default: no implicit ceiling is inferred for any + * destination. The only measured limit in this codebase is the WebSocket create-frame size, + * and the same body still succeeds over HTTP SSE, so a default here would refuse requests + * that work today — on Azure and custom Responses gateways as well, whose limits are unknown. + */ + maxUpstreamBodyBytes?: number; /** * Opt-in Anthropic OAuth account pool (#294). Default OFF. * Failover on 429 + sticky affinity; new sessions may pick lowest known 5h usage. diff --git a/tests/empty-completion-core.test.ts b/tests/empty-completion-core.test.ts index 6d538bf54c..35c6c6b85e 100644 --- a/tests/empty-completion-core.test.ts +++ b/tests/empty-completion-core.test.ts @@ -18,24 +18,47 @@ let httpCalls = 0; let parsedAttempts: OcxParsedRequest[] = []; let builtBodies: string[] = []; let customRunTurn: ProviderAdapter["runTurn"] | undefined; +let passthroughFetchCalls = 0; +let bodyObservationReleaseCalls = 0; function attemptAt(index: number): AdapterEvent[] { return attemptEvents[index] ?? [{ type: "error", message: `missing fixture attempt ${index}` }]; } -function fixtureAdapter(provider: OcxProviderConfig): ProviderAdapter { +function fixtureAdapter(provider: OcxProviderConfig): ProviderAdapter & { passthrough?: true } { const runTurn = provider.adapter === "test-run-turn"; + const passthrough = provider.adapter === "test-passthrough"; return { - name: runTurn ? "test-run-turn" : "openai-chat", - buildRequest(parsed) { + name: runTurn ? "test-run-turn" : passthrough ? "test-passthrough" : "openai-chat", + ...(passthrough ? { passthrough: true as const } : {}), + buildRequest(parsed, incoming) { const rawBody = parsed._rawBody as { service_tier?: unknown } | undefined; - const body = JSON.stringify({ - model: parsed.modelId, - messages: parsed.context.messages, - ...(rawBody?.service_tier !== undefined ? { service_tier: rawBody.service_tier } : {}), - }); + const body = passthrough + ? JSON.stringify(parsed._rawBody) + : JSON.stringify({ + model: parsed.modelId, + messages: parsed.context.messages, + ...(rawBody?.service_tier !== undefined ? { service_tier: rawBody.service_tier } : {}), + }); builtBodies.push(body); - return { url: provider.baseUrl, method: "POST", headers: {}, body }; + const release = passthrough + ? incoming.translatorBudget.observeExternallyCapped( + "passthrough_serialization", + Buffer.byteLength(body, "utf8"), + ) + : undefined; + return { + url: provider.baseUrl, + method: "POST", + headers: {}, + body, + ...(release ? { + releaseBodyObservation: () => { + bodyObservationReleaseCalls += 1; + release(); + }, + } : {}), + }; }, async fetchResponse() { const index = httpCalls; @@ -68,7 +91,11 @@ function fixtureAdapter(provider: OcxProviderConfig): ProviderAdapter { mock.module("../src/server/adapter-resolve", () => ({ ...actualResolver, resolveAdapter(provider: OcxProviderConfig, cacheRetention?: "none" | "short" | "long") { - if (provider.adapter === "test-run-turn" || provider.adapter === "test-http") { + if ( + provider.adapter === "test-run-turn" + || provider.adapter === "test-http" + || provider.adapter === "test-passthrough" + ) { return fixtureAdapter(provider); } return actualResolveAdapter(provider, cacheRetention); @@ -77,8 +104,11 @@ mock.module("../src/server/adapter-resolve", () => ({ const { handleResponses } = await import("../src/server/responses"); -function config(adapter: "test-run-turn" | "test-http", extra: Partial = {}): OcxConfig { - return { +function config( + adapter: "test-run-turn" | "test-http" | "test-passthrough", + extra: Partial = {}, +): OcxConfig { + const result = { port: 0, defaultProvider: "fixture", emptyCompletionRetry: true, @@ -93,6 +123,18 @@ function config(adapter: "test-run-turn" | "test-http", extra: Partial { + passthroughFetchCalls += 1; + return Response.json({ + id: "resp_fixture", + object: "response", + status: "completed", + output: [], + }); + }; + } + return result; } function request( @@ -114,6 +156,8 @@ beforeEach(() => { parsedAttempts = []; builtBodies = []; customRunTurn = undefined; + passthroughFetchCalls = 0; + bodyObservationReleaseCalls = 0; }); afterEach(() => { @@ -122,6 +166,78 @@ afterEach(() => { }); describe("empty-completion core integration", () => { + test("an unconfigured limit sends an oversized passthrough body upstream", async () => { + // The regression guard for this whole feature. A default ceiling here would refuse turns + // that succeed today: the one measured limit in this codebase is the WS create-frame size, + // and that transport already falls back to HTTP SSE for exactly these bodies (#2473), with + // an 18.2 MB HTTP 200 observed in #2426. Unset must mean "send it", not "guess a ceiling". + const logCtx: RequestLogContext = { model: "", provider: "" }; + const response = await handleResponses( + request(false, "x".repeat(20 * 1024 * 1024)), + config("test-passthrough"), + logCtx, + ); + + expect(response.status).toBe(200); + expect(passthroughFetchCalls).toBe(1); + expect(logCtx.errorCode).toBeUndefined(); + }); + + test("an oversized passthrough body is refused locally and releases its observation", async () => { + const logCtx: RequestLogContext = { model: "", provider: "" }; + const response = await handleResponses( + request(false, "x".repeat(512)), + config("test-passthrough", { maxUpstreamBodyBytes: 128 }), + logCtx, + ); + const body = await response.json() as { error?: { code?: string } }; + + expect(response.status).toBe(413); + expect(body.error?.code).toBe("outbound_body_too_large"); + expect(logCtx.errorCode).toBe("outbound_body_too_large"); + expect(passthroughFetchCalls).toBe(0); + expect(bodyObservationReleaseCalls).toBe(1); + }); + + test("a streaming refusal is terminal overflow, not a retryable 413", async () => { + // Codex resends on an HTTP 413, so the shape that stops the loop is response.failed with + // context_length_exceeded — the same contract the upstream-413 path already returns (#3177). + const response = await handleResponses( + request(true, "x".repeat(512)), + config("test-passthrough", { maxUpstreamBodyBytes: 128 }), + { model: "", provider: "" }, + ); + + expect(response.status).toBe(200); + const text = await response.text(); + expect(text).toContain("response.failed"); + expect(text).toContain("context_length_exceeded"); + expect(passthroughFetchCalls).toBe(0); + }); + + test("a normal-sized passthrough body still reaches upstream", async () => { + const response = await handleResponses( + request(false), + config("test-passthrough", { maxUpstreamBodyBytes: 4_096 }), + { model: "", provider: "" }, + ); + + expect(response.status).toBe(200); + expect(passthroughFetchCalls).toBe(1); + expect(bodyObservationReleaseCalls).toBe(1); + }); + + test("an explicit zero limit lets an oversized turn reach upstream", async () => { + const response = await handleResponses( + request(false, "x".repeat(512)), + config("test-passthrough", { maxUpstreamBodyBytes: 0 }), + { model: "", provider: "" }, + ); + + expect(response.status).toBe(200); + expect(passthroughFetchCalls).toBe(1); + }); + test("streaming runTurn returns the local 429 contract when initial pacing admission is rejected", async () => { setProviderRequestPacingLimitsForTest({ maxQueueDepth: 0 }); const overloaded = config("test-run-turn"); diff --git a/tests/outbound-body-guard.test.ts b/tests/outbound-body-guard.test.ts new file mode 100644 index 0000000000..45b832e637 --- /dev/null +++ b/tests/outbound-body-guard.test.ts @@ -0,0 +1,87 @@ +import { describe, expect, test } from "bun:test"; +import { + checkOutboundBodySize, + describeOutboundBodyRefusal, +} from "../src/server/responses/outbound-body-guard"; + +describe("outbound body guard", () => { + test("an unconfigured limit admits without measuring", () => { + // The whole default contract: no ceiling is inferred for any destination, so a body far + // above any observed transport limit still goes out exactly as it does today. + const huge = "x".repeat(20 * 1024 * 1024); + const result = checkOutboundBodySize(huge, undefined); + expect(result.admitted).toBe(true); + expect(result.limit).toBe(0); + // bytes stays 0 because the body was never measured — this is the cheap path. + expect(result.bytes).toBe(0); + }); + + test("an explicit 0 disables the guard", () => { + const huge = "x".repeat(20 * 1024 * 1024); + expect(checkOutboundBodySize(huge, 0).admitted).toBe(true); + }); + + test("refuses only above the configured limit", () => { + expect(checkOutboundBodySize("12345", 5).admitted).toBe(true); + expect(checkOutboundBodySize("12345", 4).admitted).toBe(false); + }); + + test("measures UTF-8 bytes, not code units", () => { + // A 1-character string is 3 bytes here; measuring .length would wrongly admit it. + const result = checkOutboundBodySize("界", 2); + expect(result.admitted).toBe(false); + expect(result.bytes).toBe(3); + }); + + test("an unparseable oversized body still refuses, without diagnostics", () => { + const result = checkOutboundBodySize("{not-json", 1); + expect(result.admitted).toBe(false); + expect(result.imageCount).toBe(0); + }); + + test("counts embedded input_image payloads and names them in the refusal", () => { + const image = "A".repeat(4000); + const body = JSON.stringify({ + input: [{ + role: "user", + content: [ + { type: "input_text", text: "look" }, + { type: "input_image", image_url: `data:image/png;base64,${image}` }, + { type: "input_image", image_url: `data:image/png;base64,${image}` }, + ], + }], + }); + const result = checkOutboundBodySize(body, 100); + expect(result.admitted).toBe(false); + expect(result.imageCount).toBe(2); + expect(result.imageBytes).toBeGreaterThan(5000); + + const message = describeOutboundBodyRefusal(result); + expect(message).toContain("2 input_image items"); + expect(message).toContain("compact the conversation"); + }); + + test("finds images nested in function_call_output, not just message content", () => { + // Replayed tool results are a common place for accumulated screenshots to hide. + const body = JSON.stringify({ + input: [{ + type: "function_call_output", + output: [{ type: "input_image", image_url: "data:image/png;base64,AAAA" }], + }], + }); + expect(checkOutboundBodySize(body, 10).imageCount).toBe(1); + }); + + test("a self-referential body cannot hang the diagnostic walk", () => { + const body = JSON.stringify({ input: [{ type: "input_image", image_url: "data:,x" }] }); + expect(checkOutboundBodySize(body, 1).imageCount).toBe(1); + }); + + test("the singular form reads correctly for one image", () => { + const body = JSON.stringify({ + input: [{ type: "input_image", image_url: "data:image/png;base64,AAAA" }], + }); + const message = describeOutboundBodyRefusal(checkOutboundBodySize(body, 10)); + expect(message).toContain("1 input_image item "); + }); +});