fix(images): Codex P2 follow-ups for image bridge (#424) - #528
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Keep runTurn adapters out of the unsupported web-search loop, honor xAI authMode, cap paid image calls, and close the remaining review gaps around usage, transport, tool-choice, and tests.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
I reviewed the current head (553e9afc), not the earlier automated-review snapshots. The auth-mode selection, hidden-iteration usage aggregation, runTurn dispatch/stall handling, tool-choice mapping, duplicate-tool replacement, paid-call cap, hosted size/quality defaults, configured deadlines, and provider fetch transport findings are addressed in this head. The focused image tests and typecheck pass when the mock-heavy files are run in their intended isolated processes.
One merge blocker remains: downloadImageToArtifact() resolves the provider-returned hostname in assertUrlResolvesPublic(), then calls global fetch(url), which performs a second independent DNS resolution. A rebinding host can pass the first public-address check and resolve to loopback/private/metadata space for the actual connection. The code comment currently records this as a residual, but this PR introduces the remote-download surface; it cannot rely on a check that is detached from the connected peer.
Please either pin the validated address for the actual HTTPS connection while retaining the original hostname/SNI, validate the connected peer, or constrain downloads to a defensible trusted-host contract. Add a regression where the validation lookup is public and the connection lookup changes to a blocked address. After that and resolution of the stale review threads, I can re-review. This is a valuable feature, but it is not ready to merge into dev with that boundary open.
Two read-only audit lenses returned 30 contradictions; the verified ones are applied here and recorded in 006_corrections.md. Factual corrections: - claude-code#1124 closed 2025-05-16, not 2025-08-10 - 13 open enhancement issues, not 11 - #529 is merged, so issue #42 phase 2 is done - #418 has no PR behind it; it was miscounted as in-flight - #528 does not contain #424's current head, so it needs a rebase - #527's retarget is manual and independent of #526 Framing corrections: - stale-needs-info.yml is absent from the default branch, so bundle C had no real deadline - the debug switch #543's reporter asked for already exists - whether #545 is our defect is answerable from code, not owner judgment - #491 is an OAuth credential change, so it is a security boundary rather than a warm-up; #533 and #447 keep their security classification too Structure: bundle G added for roadmap honesty, bundle F rescoped to the security boundary, #498 split out of bundle B, cycle order rebuilt, and the loop archetype corrected to decision-elicitation.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Re-reviewed current head 21c65c97. The DNS-rebinding blocker from my prior review is addressed correctly in principle: the code resolves once, keeps the original hostname for Host/SNI, and pins the validated address through a custom lookup. The focused artifact suite passes 28/28, typecheck passes, and the previous DNS thread is now outdated.
A new blocking regression is introduced by the pinned transport, though: pinnedHttpsGet() collects every data chunk into chunks and calls Buffer.concat(chunks) before it returns a Response. downloadImageToArtifact() applies MAX_DOWNLOAD_BYTES only while reading that already-buffered Response afterward. Therefore an untrusted image URL can make the process allocate an unbounded response body before the intended 50 MiB cap runs, defeating the memory-exhaustion protection this path previously had.
Please stream the IncomingMessage through to the existing bounded reader, or enforce the same hard cap inside pinnedHttpsGet() and destroy the request immediately when exceeded. Add a regression against the default pinned transport that exceeds the cap and proves it aborts before full buffering; the current injected pinnedDownload tests do not exercise this allocation path.
Process gates also remain: this head currently conflicts with current dev (CONFLICTING / DIRTY) and the new tip has not completed cross-platform CI. After the bounded transport fix, rebase/resolve against current dev, run CI on the mergeable SHA, and ping for re-review.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Keep runTurn adapters out of the unsupported web-search loop, honor xAI authMode, cap paid image calls, and close the remaining review gaps around usage, transport, tool-choice, and tests.
Address Ingwannu and CodeRabbit feedback on lidge-jun#528: pinnedHttpsGet returns a streaming body, enforces MAX_DOWNLOAD_BYTES mid-stream, honors lookup all:true, and times out idle peers. Drop unreachable localhost check after assessDestination. Add transport regressions.
21c65c9 to
319a96c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Reject non-global IPv6 peers, stop double-counting hidden usage, reset runTurn idle stalls on progress, reject empty/non-image downloads, backfill Cursor conversation ids from the image loop, and clarify Responses vs /images/generations activation in docs.
When a non-OpenAI model receives an image_generation hosted tool from Codex,
OpenAI's server-side execution is unavailable. This intercepts the tool call,
routes it to xAI Grok Imagine, materializes the image to ~/.opencodex/artifacts/,
and feeds the result back to the model — all inside a streaming SSE loop.
Architecture mirrors the proven web-search sidecar pattern:
- parser.ts: extract hosted image_generation tool, stash into parsed._imageGeneration
- plan.ts: decide whether to activate (xAI provider + token available, non-OpenAI route)
- loop.ts: agentic loop (max 3 rounds) — intercept image tool calls, fulfill via xAI,
inject results, re-call model, bridge to SSE
- synthetic-tool.ts: buildImageTool() injection + isImageGenName() detection
- xai-client.ts: xAI /images/generations and /images/edits API client
- fulfill.ts: execute single image call, materialize to disk (never throws)
- artifacts.ts: extended with downloadImageToArtifact() + magic byte format detection
New files:
src/images/{types,xai-client,synthetic-tool,fulfill,plan,loop,index}.ts
tests/images/{xai-client,plan,fulfill,synthetic-tool,loop}.test.ts
docs-site/src/content/docs/guides/image-bridge.md
Modified files:
src/types.ts: +_imageGeneration stash, +OcxTool.imageGeneration flag, +OcxImagesConfig fields
src/responses/parser.ts: extractHostedImageGeneration extraction
src/server/responses/core.ts: image bridge trigger point
src/images/artifacts.ts: +downloadImageToArtifact, +guessExtFromMagic, shared helpers
- Enforce HTTPS-only scheme in downloadImageToArtifact (reject ftp/http/gopher) - Fix dispatch order: image bridge defers to web-search when both eligible - Narrow buildImageTool description to generation-only (no 'edit' promise) - Remove URL-interpolating error from fulfill.ts console.warn - Add downloadImageToArtifact SSRF tests (http/ftp reject, https succeeds) - Add handler-activation regression tests (stream/400/dual-tool defer)
- Move web-search dispatch before runTurn so dual-tool turns reach web-search even on Cursor/runTurn adapters - Guard image bridge with !routedCompaction to prevent hijacking compaction requests - Fix IPv4-mapped IPv6 hex SSRF bypass in destination-policy (::ffff:7f00:1 now decoded and classified as loopback) - Add SSRF tests: private IP, gopher, IPv6 mapped (dotted+hex), redirect:error fail-closed - Add regression: runTurn dual-tool → web-search wins - Add regression: compaction request → image bridge skipped
Clamp maxRounds, honor images.timeoutMs, strip all image tool aliases on forced-final, return SSE headers before runTurn collect, preserve Cursor conversation ids across loop iterations, wire onUsage/logCtx and 429 key rotation, and keep parallel image calls in one assistant turn.
Keep runTurn adapters out of the unsupported web-search loop, honor xAI authMode, cap paid image calls, and close the remaining review gaps around usage, transport, tool-choice, and tests.
Cancel the in-flight runTurn when the image-bridge collect deadline fires, fire onAttemptSend at dispatch time, and fold accumulated usage into incomplete terminals as well as done.
Resolve once via resolvePublicAddresses, then connect with a custom https lookup that keeps SNI/Host on the original hostname so a later private answer cannot retarget the peer.
Address Ingwannu and CodeRabbit feedback on lidge-jun#528: pinnedHttpsGet returns a streaming body, enforces MAX_DOWNLOAD_BYTES mid-stream, honors lookup all:true, and times out idle peers. Drop unreachable localhost check after assessDestination. Add transport regressions.
Reject failed downloads before attaching a streaming body so unread 4xx/5xx payloads cannot keep the socket alive, and cancel custom-seam bodies on !ok.
CodeRabbit: /reference/providers/ is not a registered Starlight page.
Reject non-global IPv6 peers, stop double-counting hidden usage, reset runTurn idle stalls on progress, reject empty/non-image downloads, backfill Cursor conversation ids from the image loop, and clarify Responses vs /images/generations activation in docs.
On idle expiry abort the runTurn signal and close the event queue so the consumer finishes even when runTurn ignores cancellation and never settles. Add a never-settling regression.
Rebase onto dev and extend the storage cleanup harness timeout for injected satellite rollback tests that measure 6–13s on Windows runners.
21a0be6 to
34c6d85
Compare
Wait for worker spawn and holdAfterLoadMs before concurrent PUT so slow Windows CI worker startup does not load a disabled policy snapshot.
Resolve api-storage-policy.test.ts conflict: keep Windows CI worker spawn timing (blockMs 1500, sawRunning gate, 800ms hold margin).
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Superseded and landed as part of maintainer takeover #577 (merged to |
2 participants
Summary
6d6b252. Prefer merging feat(images): add Grok image bridge for non-OpenAI models #424 first, then this; until then, review the tip commit (full compare vsdevalso includes feat(images): add Grok image bridge for non-OpenAI models #424).Changes
images.maxRoundsto[0, 10](clampImageMaxRounds) so hand-edited10000/ fractional values cannot unbound paid xAI callsimages.timeoutMsincallXaiImages(via plan → fulfill)plan.toolNames, not onlyimageGeneration:truequeue.collect()on Cursor/runTurn(skip eager drain; heartbeat while collecting)_cursorConversationIdacross image-loop iterationsonUsage/logCtxfor image-bridge completions (parity with web-search)rotateProviderTransportOn429inside the image loopTest plan
bun test --isolate tests/images/(94 pass)bun run typecheckSummary by CodeRabbit
maxRounds,timeoutMs).image_gen.