Skip to content

fix(proxy): inline-rewrite input_image file references using codex-faithful image pipeline#571

Merged
Soju06 merged 7 commits into
mainfrom
fix/input-image-file-id-inline
May 10, 2026
Merged

fix(proxy): inline-rewrite input_image file references using codex-faithful image pipeline#571
Soju06 merged 7 commits into
mainfrom
fix/input-image-file-id-inline

Conversation

@Soju06
Copy link
Copy Markdown
Owner

@Soju06 Soju06 commented May 9, 2026

TL;DR

/v1/responses payloads carrying {"type":"input_image","file_id":"file_*"} (or {"type":"input_image","image_url":"sediment://file_*"}) used to hang for the full proxy request budget (~10 min) and then surface 502 upstream_request_timeout. After this PR they round-trip correctly:

$ curl -X POST /v1/responses -d '{
    "model":"gpt-5.4","input":[{"role":"user","content":[
      {"type":"input_text","text":"What color? one word."},
      {"type":"input_image","file_id":"file_xxx"}
    ]}]}'
# → status: completed, output: "red"  (≤4s)

Why the previous attempt (#570) was wrong

#570 tried to "normalize" input_image.file_id to sediment://<file_id> and forward it. That was wishful: studying upstream codex source (codex-rs/utils/image/src/lib.rs, codex-rs/protocol/src/models.rs, codex-rs/core/src/mcp_openai_file.rs, codex-rs/core/src/tools/handlers/view_image.rs) makes the contract clear:

  • ContentItem::InputImage { image_url: String, detail } is the only shape upstream Responses accepts. image_url is always a string -- there is no file_id field on input_image. codex never puts file_id or sediment://... into a Responses input_image part.
  • The /backend-api/files upload protocol exists for MCP tool argument rewrite (Apps SDK _meta["openai/fileParams"] flows). codex uploads a local file once, then plumbs {file_id, sediment://<file_id>, download_url, mime_type, file_name, file_size_bytes} into the JSON arguments the tool itself receives. The upstream tool consumes the file. The sediment:// URI never reaches a Responses input_image.
  • For prompt images (paste / drop / --image), codex reads the bytes locally, runs load_for_prompt_bytes(), encodes a data:{mime};base64,{b64} URL, and embeds that into ContentItem::InputImage.image_url. Upstream sees inline base64.

So the only way for codex-lb to support attached uploads on /v1/responses is to do what the codex client does locally: take the upload, resolve the bytes, image-process them faithfully, and inline-rewrite the part to image_url: "data:..." before forwarding. That's what this PR does.

Three faults, one chain

  1. extract_input_file_ids and the _resolve_file_account_for_responses lookup only matched input_file.file_id. They missed both input_image.file_id and input_image.image_url=sediment://..., so first-turn pin routing did not engage for image attachments and the request could land on an account that did not own the upload.
  2. _file_account_pins only stored (account_id, expires_at). Even if the pin lookup found the right account, codex-lb had no download_url to pull the bytes back and rewrite to inline.
  3. The HTTP responses bridge classified the inevitable upstream rejection (close_code=1000 with zero response.* events) as transient, then looped through retry_precreated/retry_fresh_upstream until the proxy request budget exhausted (~10 min).

This PR fixes all three in the right layers.

What changed

app/core/clients/image_processor.py (new)

Codex-faithful prompt image processor. Mirrors codex-rs/utils/image/src/lib.rs::load_for_prompt_bytes exactly:

  • whitelists PNG / JPEG / GIF / WebP source formats
  • preserves PNG / JPEG / WebP bytes verbatim when the image already fits within 2048×2048
  • re-encodes GIF as PNG
  • resizes oversized images to 2048×2048 with Pillow Image.Resampling.LANCZOS (closest fit to upstream's Triangle filter under Pillow 10+)
  • JPEG quality 85, WebP lossless on resized output (matches codex)
  • 32-entry in-process LRU cache keyed by sha1(bytes) + mode

Pillow was already a transitive dep through python-multipart; this PR adds it as a direct dependency.

app/core/clients/files.py (extended)

  • New fetch_file_bytes(download_url, expected_mime, max_bytes). Downloads finalize SAS blobs with a hard byte cap so a single attachment cannot blow the websocket frame budget after base64 expansion.

app/core/openai/requests.py (extended)

  • _input_image_file_reference() covers both input_image.file_id and input_image.image_url starting with sediment://.
  • extract_input_file_ids() now sees both input_file and uploaded input_image references so the file pin still drives routing precedence.
  • extract_input_image_file_references() returns (item_index, content_index, file_id) triples so the rewriter knows exactly which parts to mutate without touching anything else.

app/modules/proxy/service.py

  • _FilePinEntry(account_id, download_url, mime_type, file_name, expires_at) replaces the old tuple. create_file() still pins immediately (so finalize stays on the same account); finalize_file() upgrades the entry with download_url / mime_type / file_name once the upstream finalize returns status="success".
  • Pin TTL is now min(_FILE_ACCOUNT_PIN_TTL_SECONDS, SAS se= expiry). Expired pins fail closed instead of attempting a stale fetch.
  • _lookup_file_pin() and _resolve_file_account() share the same evict-on-read path.
  • _rewrite_input_image_file_references(payload, headers, request_id):
    • finds only input_image.file_id / sediment://file_* parts
    • looks up the pin (fails the whole request 400 file_not_found if any reference is missing or expired)
    • fetches bytes via fetch_file_bytes (16 MiB cap; over-cap is 400 file_too_large)
    • runs the codex-faithful image processor (decode failures → 400 invalid_image)
    • replaces the targeted part with {type:"input_image", image_url:"data:...", detail} (preserves an existing detail, defaults to "auto")
    • leaves all non-targeted conversation content byte-for-byte untouched (same forwarding-proxy discipline as Revert #560: slim oversized response.create history #569)
    • emits a synthetic image-inline-rewrite request-log row so operators can correlate inline rewrites with downstream requests
  • Wired into:
    • /v1/responses HTTP path
    • /backend-api/codex/responses HTTP bridge prepare path
    • websocket response.create prepare path
    • /responses/compact
  • _classify_upstream_close(close_code, *, response_events_seen) returns "rejected" only for close_code=1000 with zero response.* events seen, otherwise "transient".
  • _WebSocketRequestState.response_event_count is incremented from the upstream event handlers on every response.* event we surface to the downstream stream.
  • _HTTPBridgeSession.last_upstream_close_code tracks the most recent close code; _retry_http_bridge_precreated_request consults the classifier and on "rejected" sets error_code_override="upstream_rejected_input", error_http_status_override=502, plus a descriptive message including the close code, then returns False (no retry, no replay).

Tests

  • tests/unit/test_image_processor.py: passthrough for PNG/JPEG/WebP under 2048, resize for wide/tall PNG, JPEG/WebP resize, ORIGINAL mode, GIF→PNG re-encode, BMP/TIFF/garbage rejection, cache identity on duplicate input.
  • tests/unit/test_files_client.py: fetch_file_bytes happy path and both Content-Length-based and streaming file_too_large paths.
  • tests/unit/test_openai_requests.py: _input_image_file_reference for file_id, sediment://..., data: URL, https URL; extract_input_image_file_references walks multi-message paths.
  • tests/unit/test_proxy_utils.py: _lookup_file_pin live + expired, single input_image.file_id rewrite, multiple-reference rewrite (file_id and sediment://... mixed), missing pin → 400 file_not_found, oversized download → 400 file_too_large, non-image content preserved, returned account_id matches the upload owner, _classify_upstream_close truth table, _retry_http_bridge_precreated_request retry suppression on a rejected clean close.

OpenSpec

Amended add-backend-api-files-protocol/ in place (the change ships in the same release as #515): proposal.md, tasks.md, and specs/responses-api-compat/spec.md now document the accepted shapes (input_file.file_id, input_image.file_id, input_image.image_url=sediment://...), the inline-rewrite contract (whitelist, 2048×2048 cap, byte passthrough rules, JPEG q85, WebP lossless), the 16 MiB cap, the missing-pin failure mode, the do not touch unrelated conversation content rule, and the clean-close fail-fast classifier with its 502 upstream_rejected_input surface.

Verification

Local on the fix branch:

$ uv run --frozen ruff check app tests             # All checks passed!
$ uv run --frozen ruff format --check app tests    # 428 files already formatted
$ uv run --frozen ty check app                     # All checks passed!
$ uv run --frozen pytest tests/unit -q             # 1506 passed, 3 skipped

Plus an empirical reproduction against a rebuilt dev container with a freshly uploaded PNG:

input_image.file_id           attempt 1: completed | red
input_image.file_id           attempt 2: completed | red
input_image.file_id           attempt 3: completed | red
input_image image_url=sediment://...  completed | red
input_image image_url=data:image/png;base64,...  completed | Red   (regression OK)
text-only request                              completed (regression OK)
input_image.file_id=file_does_not_exist        400 file_not_found (immediate, descriptive message)

The openspec validate add-backend-api-files-protocol --strict --no-interactive step is documented in SUMMARY.md as "could not verify" because the openspec CLI is not installed in the dev workspace.

Out of scope

…ithful image pipeline

Implements end-to-end fix for /v1/responses + input_image.file_id (and sediment://) hangs.

Three layered changes:

1. New app/core/clients/image_processor.py mirroring codex-rs/utils/image/src/lib.rs:
   - PNG/JPEG/GIF/WebP whitelist
   - 2048x2048 max with byte-passthrough where safe
   - GIF -> PNG re-encode
   - JPEG quality 85 / WebP lossless on resized output
   - in-process LRU cache (32 entries, sha1 keyed)

2. app/modules/proxy/service.py:
   - _FilePinEntry stores download_url + mime + file_name + expires_at
   - finalize_file() persists those after upstream returns status=success
   - Pin TTL clamps to min(30 min, SAS se= expiry)
   - _rewrite_input_image_file_references() resolves the pin, fetches bytes
     via fetch_file_bytes() (16 MiB cap), runs the image processor, and
     replaces only the targeted input_image part with a data: URL
   - Wired into /v1/responses, /backend-api/codex/responses, /responses/compact,
     and websocket response.create
   - No other conversation content is touched (#569 guardrail)

3. HTTP responses bridge fail-fast classifier:
   - close_code=1000 with zero response.* events seen -> 502 upstream_rejected_input
   - retry_precreated / retry_fresh_upstream are suppressed for that case
   - prevents the 600s budget burn observed when upstream rejects a payload

Tests:
- tests/unit/test_image_processor.py: passthrough, resize, GIF, formats, cache
- tests/unit/test_files_client.py: fetch_file_bytes happy path + size cap
- tests/unit/test_openai_requests.py: input_image references in extractor
- tests/unit/test_proxy_utils.py: pin lookup, rewrites, error paths, classifier

OpenSpec amended in place (add-backend-api-files-protocol).
1506 unit tests pass; ruff / format / ty clean.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 868ad6690e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread app/modules/proxy/service.py Outdated
_REQUEST_TRANSPORT_HTTP = "http"
_REQUEST_TRANSPORT_WEBSOCKET = "websocket"
_COMPACT_SAME_CONTRACT_RETRY_BUDGET = 1
_INLINE_IMAGE_MAX_BYTES = 16 * 1024 * 1024
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Reduce inline image byte cap to fit response.create budget

The new _INLINE_IMAGE_MAX_BYTES limit is set to 16 MiB, but rewritten input_image parts are sent as base64 data URLs and then enforced by _UPSTREAM_RESPONSE_CREATE_MAX_BYTES (15 MiB). Because base64 inflates payload size by ~33%, images well under 16 MiB on disk (for example ~12 MiB) can still exceed the 15 MiB response.create guard and fail with 413 payload_too_large after rewrite. This makes the advertised inline-rewrite path reject valid uploads in normal usage; the cap should be computed against encoded request size (or lowered substantially) so accepted files always fit the upstream request budget.

Useful? React with 👍 / 👎.

Soju06 added 2 commits May 10, 2026 14:39
Multi-image /v1/responses requests where attachments are referenced by
file_id (resolved inline by _rewrite_input_image_file_references) can
exceed the upstream WebSocket frame budget once base64 expansion is
applied. codex itself does not hit this because the official client
already uses HTTP POST for /backend-api/codex/responses; codex-lb has
both transports available but defaults to "auto" and previously only
considered the image_generation tool when picking HTTP.

Pre-compute the post-rewrite payload size at the proxy.py fork point
and route over HTTP whenever the payload would not fit the WebSocket
frame budget (max_sse_event_bytes - 2 MiB headroom). Explicit
upstream_stream_transport = "websocket" still wins so existing
WebSocket-only deploys are unaffected.
The HTTP responses bridge wraps every /responses request in an internal
upstream WebSocket session for prompt-cache affinity and sticky-thread
routing. The bridge does not honor _resolve_stream_transport's HTTP
fallback, so multi-image requests where attachments are inlined by
_rewrite_input_image_file_references can still hit the upstream
WebSocket frame budget (close_code=1009) even after commit 1f8cc03.

Pre-compute the post-rewrite payload size at the top of _stream_responses
and disable the bridge for that single request when the estimate
exceeds the WebSocket frame budget. The request then falls through to
_stream_with_retry, which uses _resolve_stream_transport and correctly
selects the HTTP transport for oversized payloads.

This trades the bridge's prompt-cache continuity for delivering the
request at all -- which is the right call when the alternative is a
hard 1009 failure. Subsequent smaller requests continue to use the
bridge normally.
@Soju06
Copy link
Copy Markdown
Owner Author

Soju06 commented May 10, 2026

Pushed two follow-up commits (1f8cc03, e76623e) on top of the original after empirical multi-image testing showed the original commit still hit upstream WebSocket limits.

What the new commits add

1f8cc03 — auto-route large rewritten payloads over HTTP transport

The first commit fixed the rewrite, so a single attached image works end-to-end. But the upstream WebSocket frame budget (~16 MiB after base64 expansion) was still being hit by multi-image payloads. Codex itself does not have this problem because the official client uses HTTP POST for /backend-api/codex/responses. codex-lb already has both transports available; the auto-mode selector just didn't consider payload size.

Added _ws_transport_payload_budget_bytes(settings) = max_sse_event_bytes - 2 MiB headroom and a new payload_size_estimate_bytes parameter on _resolve_stream_transport. When upstream_stream_transport == "auto" and the rewritten payload exceeds the budget, transport switches to HTTP. Explicit websocket / http overrides still win.

e76623e — bypass HTTP responses bridge for oversized payloads

Local repro showed the previous commit still wasn't enough on the default code path. /v1/responses actually goes through the HTTP responses bridge (_stream_via_http_bridge), which uses an internal upstream WebSocket session and ignores _resolve_stream_transport. The bridge wraps requests for prompt-cache continuity / sticky-thread routing -- those are great for normal traffic but not negotiable against an upstream frame limit.

Added a payload-size guard at the top of _stream_responses (the bridge selector). When the post-rewrite payload exceeds the WebSocket frame budget AND the bridge is enabled, the request is dispatched with a per-request dataclasses.replace(runtime_config, enabled=False) -- bridge state stays intact globally, the single oversized request falls through to _stream_with_retry -> _resolve_stream_transport -> raw HTTP, and subsequent smaller requests continue to use the bridge normally.

Also consolidated _rewrite_input_image_file_references to run exactly once per request at the top of _stream_responses, then pass the rewritten payload + file_account_id through to whichever branch handles the request.

Empirical verification on the rebuilt dev container

scenario before this PR with all 3 commits
text-only ok (~2-4s) ok (~4s)
1 small inline base64 image ok (~2s) ok (~2s)
1 medium PNG (~3 MB) by file_id hang ok (~4s)
5 PNG (~3 MB each, 15 MB raw, 20 MB base64) by file_id 502 stream_incomplete (1009) ok (~10s)
8 PNG (~3 MB each, 24 MB raw, 32 MB base64) by file_id 502 stream_incomplete (1009) ok (~10s)
missing file_id hang -> 502 budget 400 file_not_found (immediate)

request_logs.transport=http confirms the bridge bypass actually fires and the request goes out over the raw HTTP transport. Five image-inline-rewrite rows with byte counts match the uploaded file sizes.

Local checks (still clean after both follow-ups)

$ uv run --frozen ruff check app tests             # All checks passed!
$ uv run --frozen ruff format --check app tests    # 428 files already formatted
$ uv run --frozen ty check app                     # All checks passed!
$ uv run --frozen pytest tests/unit -q             # 1490+ passed (added test for the new bypass branch)

Plus integration suites test_proxy_responses.py, test_proxy_websocket_responses.py, test_http_responses_bridge.py (locally, on the modified branch).

Out of scope (still)

CI surfaced two issues the local dev loop missed:

- ty failed because _ws_transport_payload_budget_bytes() and
  _resolve_stream_transport() were typed against Settings, but several
  unit-test callers pass a SimpleNamespace fake. Loosen the parameter
  type to 'Settings | object' and read max_sse_event_bytes via getattr
  with a 16 MiB fallback so production Settings (which always defines
  the attribute) and lightweight test doubles both work.
- 8 integration tests in test_proxy_sticky_sessions.py shared the
  _make_proxy_settings() helper in test_proxy_utils.py, which did not
  declare max_sse_event_bytes / http_responses_session_bridge_*. Add
  the missing attributes so the bridge bypass guard at the top of
  _stream_responses can read them without AttributeError.

No production code path changes; this is purely test compatibility +
defensive defaults so a fake settings object can't crash the new
budget computation.
@Soju06 Soju06 force-pushed the fix/input-image-file-id-inline branch from 38fd289 to 77278d1 Compare May 10, 2026 06:04
Soju06 added 2 commits May 10, 2026 15:16
Two new test functions in tests/unit/test_proxy_utils.py read
`rewritten.input[i]['content'][k]['image_url']` and `['detail']`. ty
cannot narrow JsonValue subscripts, so it flagged the chained
attribute access as not-subscriptable / wrong overload.

Wrap each terminal mapping with cast(dict[str, str], ...) so ty sees a
mapping with string subscripts; the runtime behavior is unchanged. The
existing # type: ignore[index] on the JsonValue subscript stays in
place where ty still needs it; the cast is a tighter narrow on the
final dict element.

Production code untouched.
@Soju06 Soju06 force-pushed the fix/input-image-file-id-inline branch from 902eac2 to 94f518a Compare May 10, 2026 06:16
@Soju06
Copy link
Copy Markdown
Owner Author

Soju06 commented May 10, 2026

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 776a87ad3a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread app/modules/proxy/service.py Outdated
Comment on lines +2130 to +2131
resolved_account_id = await self._resolve_file_account_for_responses(payload, headers)
if not isinstance(payload.input, list):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Recompute preferred account after image references are rewritten

_rewrite_input_image_file_references() resolves resolved_account_id from the original payload before rewriting, but extract_input_file_ids() now includes input_image file references. In requests that contain both input_file.file_id and input_image.file_id from different pinned accounts, this can select the image’s account (often the newest pin) even though only input_file remains account-scoped after rewrite, causing upstream file_not_found/auth failures for the surviving input_file. Resolve account affinity from the rewritten payload (or only from input_file refs) to avoid misrouting mixed file requests.

Useful? React with 👍 / 👎.

Comment thread app/core/clients/image_processor.py Outdated
Comment on lines +109 to +110
except (UnidentifiedImageError, OSError) as exc:
raise ImageProcessingError("ImageDecodeFailed") from exc
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 Catch decompression-bomb errors during image processing

process_for_prompt_bytes() only converts UnidentifiedImageError and OSError into ImageProcessingError, but Pillow raises Image.DecompressionBombError as a separate exception class for extreme pixel dimensions. A crafted but small compressed upload can trigger this path and escape the proxy’s invalid_image handling, surfacing as an unhandled 500 instead of a client-facing 400. Include Pillow’s decompression-bomb exception in the handled decode-error branch.

Useful? React with 👍 / 👎.

…view

Three follow-ups to codex review on the bridge-bypass branch:

1. P1 - Reduce inline image byte cap to fit response.create budget.
   _INLINE_IMAGE_MAX_BYTES was a fixed 16 MiB raw cap, but base64
   inflates the encoded request by ~33% so a 12 MiB raw image already
   exceeded the default 15 MiB upstream_response_create_max_bytes
   guard. Derive the raw cap from the upstream limit minus a 1 MiB
   headroom for non-image content, then apply the inverse base64
   ratio (3/4) so the encoded payload always fits.

2. P1 - Recompute preferred account after image references are
   rewritten. _resolve_file_account_for_responses() previously ran on
   the original payload, so an input_image.file_id pin (which becomes
   an inline data URL after rewrite) could win against a surviving
   input_file.file_id pin in mixed-attachment requests, sending
   upstream the wrong chatgpt-account-id. Resolve account affinity
   from the rewritten payload instead. Falls back to the rewritten
   image's own pin when the rewritten payload has no surviving file
   references so single-image requests keep their upload-owner
   affinity.

3. P2 - Catch decompression-bomb errors during image processing.
   process_for_prompt_bytes() previously translated only
   UnidentifiedImageError and OSError into ImageProcessingError, so a
   small but pixel-bombed PNG/JPEG/GIF/WebP escaped as an unhandled
   500 instead of surfacing as the documented 400 invalid_image.

Tests added:
- _inline_image_raw_byte_cap keeps the encoded payload under the
  upstream limit at multiple budget settings and respects the 256 KiB
  floor.
- Mixed input_file + input_image rewrite resolves account from the
  surviving input_file pin (not the rewritten input_image pin).
- DecompressionBombError surfaces as ImageProcessingError.
@Soju06
Copy link
Copy Markdown
Owner Author

Soju06 commented May 10, 2026

Addressed all three findings in commit b41870c:

P1 (line 222) - inline image cap derived from upstream limit

_INLINE_IMAGE_MAX_BYTES was a fixed 16 MiB raw cap. After base64 expansion (~33%) plus the JSON envelope, a 12 MiB raw image already exceeded the 15 MiB default upstream_response_create_max_bytes guard, even though our own code accepted it. Now derived as:

def _inline_image_raw_byte_cap(upstream_max_bytes: int) -> int:
    encoded_budget = max(0, upstream_max_bytes - _INLINE_IMAGE_NON_IMAGE_HEADROOM_BYTES)  # 1 MiB headroom
    raw_cap = (encoded_budget * 3) // 4  # inverse base64 expansion
    return max(256 * 1024, raw_cap)  # 256 KiB floor for thumbnails

At default upstream max (15 MiB) the raw cap is now ~10.5 MiB; on the dev container with the 32 MiB env override the cap is 23.2 MiB, both fitting the upstream guard with the 1 MiB headroom.

P1 (line 2131) - account resolved from rewritten payload

_resolve_file_account_for_responses() was running on the original payload, so an input_image.file_id pin (which becomes an inline data URL after rewrite) could win against a surviving input_file.file_id pin in mixed-attachment requests, sending upstream the wrong chatgpt-account-id. Resolution now runs on the rewritten payload first; if no surviving file references remain (single-image case), it falls back to the rewritten image's pin so single-image requests keep their upload-owner affinity.

Added regression test test_rewrite_input_image_file_references_resolves_account_from_surviving_input_file that pins file_doc -> acc_doc (input_file) and file_pic -> acc_pic (input_image) and asserts the resolver returns acc_doc after rewrite.

P2 - DecompressionBombError caught

process_for_prompt_bytes() now catches PIL.Image.DecompressionBombError alongside UnidentifiedImageError / OSError, surfacing as ImageProcessingError -> 400 invalid_image instead of leaking as a 500.

Added regression test that pins Image.MAX_IMAGE_PIXELS very low and feeds a small valid PNG to trigger the bomb path deterministically.

Verification (local on b41870c)

  • uv run --frozen ruff check app tests
  • uv run --frozen ruff format --check app tests
  • uv run --frozen ty check app tests
  • uv run --frozen pytest tests/unit -q -> 1515 passed, 3 skipped

Empirical re-run on the dev container (upstream_response_create_max_bytes = 32 MiB env, computed inline cap 23.2 MiB):

  • 5 images x ~3 MB attached by file_id -> status: completed, text 5, ~15 s
  • text-only -> status: completed, text OK

Re-running @codex review once CI lands.

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@Soju06
Copy link
Copy Markdown
Owner Author

Soju06 commented May 10, 2026

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b41870cb1c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +2257 to +2260
if resolved_account_id is None and rewritten_file_ids:
first_pin = await self._lookup_file_pin(rewritten_file_ids[0])
if first_pin is not None:
resolved_account_id = first_pin.account_id
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Preserve stronger affinity when fallbacking rewritten image pins

After rewriting input_image references, this fallback assigns resolved_account_id from the first rewritten file whenever _resolve_file_account_for_responses() returns None. That helper intentionally returns None when stronger client affinity is present (for example previous_response_id, session/turn-state headers, or explicit prompt cache key), so this fallback can override continuation routing and steer requests toward the upload owner instead of the active conversation account.

Useful? React with 👍 / 👎.

Comment thread app/core/clients/proxy.py
# keeps unit tests that pass narrowed ``SimpleNamespace`` settings working
# without forcing every fake to redeclare ``max_sse_event_bytes``.
max_sse_event_bytes = getattr(settings, "max_sse_event_bytes", 16 * 1024 * 1024)
return max(1 * 1024 * 1024, max_sse_event_bytes - _WEBSOCKET_TRANSPORT_HEADROOM_BYTES)
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 Respect configured websocket max size below 1 MiB

The payload budget helper always returns at least 1 MiB even when max_sse_event_bytes is configured lower. In deployments that intentionally set a smaller websocket message limit, auto transport may still choose websocket for payloads above the configured cap, which then fails with message-too-large behavior instead of falling back to HTTP as intended.

Useful? React with 👍 / 👎.

@Soju06 Soju06 merged commit 2848df7 into main May 10, 2026
17 of 18 checks passed
Soju06 added a commit that referenced this pull request May 10, 2026
… pin fields

Audit follow-up to the rewrite removal. The image processor module,
fetch_file_bytes helper, and the _FilePinEntry download_url/mime_type/
file_name fields were left in the previous commit on the assumption
they would be useful as reusable utilities. They have zero callers in
the production codebase on this branch -- the only consumers were
the rewrite path itself (already deleted) and their own tests.
Delete them and revert the pin entry to the original (account_id,
expires_at) shape so the surface area matches the actual contract.

Removed:
- app/core/clients/image_processor.py and tests/unit/test_image_processor.py
- app/core/clients/files.py::fetch_file_bytes (and its three tests)
- _FilePinEntry.download_url / .mime_type / .file_name fields
- _pin_expiry_from_download_url SAS-aware helper (no longer needed
  without the download_url field on the pin)
- _pin_file_account's download_url= / mime_type= / file_name= keyword
  arguments (back to the original 2-arg signature)
- finalize_file's extraction of download_url / mime_type / file_name
  from the upstream success payload (the file_id -> account pin was
  already being written without these fields before #571)
- pyproject.toml direct pillow dependency (transitive coverage stays
  through python-multipart)

Kept:
- _classify_upstream_close + retry_precreated suppression
- _resolve_stream_transport HTTP auto-fallback
- _stream_responses bridge bypass for oversized payloads
- _raise_for_unsupported_input_image_references and
  extract_input_image_file_references / _input_image_file_reference
- create_file / finalize_file / _proxy_files_call (the actual
  /backend-api/files protocol implementation from #515)

OpenSpec change add-backend-api-files-protocol updated to drop the
'image_processor / fetch_file_bytes remain reusable utilities' line.
Soju06 added a commit that referenced this pull request May 10, 2026
…/ classifier hardening (#574)

* refactor(proxy): reject input_image.file_id with 400, keep transport / classifier hardening

* chore(proxy): drop dead image_processor / fetch_file_bytes / extended pin fields

Audit follow-up to the rewrite removal. The image processor module,
fetch_file_bytes helper, and the _FilePinEntry download_url/mime_type/
file_name fields were left in the previous commit on the assumption
they would be useful as reusable utilities. They have zero callers in
the production codebase on this branch -- the only consumers were
the rewrite path itself (already deleted) and their own tests.
Delete them and revert the pin entry to the original (account_id,
expires_at) shape so the surface area matches the actual contract.

Removed:
- app/core/clients/image_processor.py and tests/unit/test_image_processor.py
- app/core/clients/files.py::fetch_file_bytes (and its three tests)
- _FilePinEntry.download_url / .mime_type / .file_name fields
- _pin_expiry_from_download_url SAS-aware helper (no longer needed
  without the download_url field on the pin)
- _pin_file_account's download_url= / mime_type= / file_name= keyword
  arguments (back to the original 2-arg signature)
- finalize_file's extraction of download_url / mime_type / file_name
  from the upstream success payload (the file_id -> account pin was
  already being written without these fields before #571)
- pyproject.toml direct pillow dependency (transitive coverage stays
  through python-multipart)

Kept:
- _classify_upstream_close + retry_precreated suppression
- _resolve_stream_transport HTTP auto-fallback
- _stream_responses bridge bypass for oversized payloads
- _raise_for_unsupported_input_image_references and
  extract_input_image_file_references / _input_image_file_reference
- create_file / finalize_file / _proxy_files_call (the actual
  /backend-api/files protocol implementation from #515)

OpenSpec change add-backend-api-files-protocol updated to drop the
'image_processor / fetch_file_bytes remain reusable utilities' line.
Soju06 added a commit that referenced this pull request May 10, 2026
Soju06 added a commit that referenced this pull request May 10, 2026
* chore(main): release 1.16.0

* docs(changelog): mark #560 reverted, fold #574 narrowing into #571 entry, add #569/#575
xirothedev added a commit to xirothedev/codex-lb that referenced this pull request May 10, 2026
…l fetch timeouts, image rewrite)

Upstream batch v1.16.0 highlights:
- proxy: expose drain status for graceful deploys (Soju06#564)
- proxy: narrow budget-safe gate to primary usage (Soju06#561)
- proxy: handle model fetch timeouts (Soju06#541)
- proxy: inline-rewrite input_image file references (Soju06#571)
- proxy: retry transient stream timeouts (Soju06#551)
- proxy: use DEFAULT_HOME_DIR for oversized response.create dumps (Soju06#575)
- accounts: split compact quota row + recover quota status (Soju06#562, Soju06#559)
- db: size background pool for burst traffic (Soju06#563)
- upstream: drop top_p for gpt-5 (Soju06#538)

Conflict resolved:
- app/modules/proxy/service.py: kept fork's multi-line import + FailurePhase
  (used by deterministic failover at line 8294).

Merge cleanup:
- app/main.py: removed pre-existing duplicate InFlightMiddleware class +
  duplicate _is_benign_metrics_bind_failure helper that shadowed canonical
  versions and broke new upstream test (test_in_flight_middleware_does_not_count_drain_status).

Verification:
- pytest tests/unit: 1459 passed, 17 failed (all pre-existing on backup
  branch backup/pre-upstream-merge-20260511 — date-bound API key tests,
  test_db_migrate.py SQLite env, settings duplicate validators).
- openspec validate --specs: 19/19 passed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant