feat(images): Grok image bridge (maintainer takeover of #424) - #577
Conversation
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.
…n#424) Fold lidge-jun#424 retention onto the lidge-jun#528 tip and clear remaining review blockers: hosted-only activation, type-only tool_choice, API-key-only xAI Images auth, ordered multi-block thinking replay, and post-batch artifact prune.
|
@codex review |
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR adds an opt-in Image Bridge for hosted Responses image-generation tools on non-OpenAI routes. It calls xAI image APIs, securely stores generated artifacts, continues streamed conversations through bounded loops, integrates dispatch priority with web search, and documents configuration and limitations. ChangesImage Bridge
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant handleResponses
participant runWithImageBridge
participant fulfillImageCall
participant xAI
Client->>handleResponses: POST /v1/responses with image_generation
handleResponses->>runWithImageBridge: activate synthetic image bridge
runWithImageBridge->>fulfillImageCall: intercept image_gen call
fulfillImageCall->>xAI: POST /images/generations or /images/edits
xAI-->>fulfillImageCall: return image data
fulfillImageCall-->>runWithImageBridge: return local artifact path
runWithImageBridge-->>Client: stream continuation SSE
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
WHATWG URL expands forms like 127.1 / 2130706433 to 127.0.0.1 before destination classification; lock that rejection path with regression tests.
|
Security review (maintainer): Image-bridge scope looked over for SSRF/DNS pin, credential mode, One candidate medium (non-canonical IPv4 like @codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
opencodex/src/responses/parser.ts
Line 190 in ee7f783
Drop the hosted image_gen alias when the bridge is inactive
For a named hosted alias such as {type: "image_gen", name: "image_gen"}, extractHostedImageGeneration recognizes the entry, but buildTools excludes only image_generation. When the bridge is disabled or cannot obtain an xAI key, this generic branch converts the hosted entry into a client-executed function; the routed model can then return a function call that Codex never declared as an executable function. Exclude both hosted image types from generic tool conversion and add an inactive-bridge parser/handler regression.
AGENTS.md reference: AGENTS.md:L93-L95
ℹ️ 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".
| const prepareIterationEvents = async function* (forceFinal: boolean): AsyncGenerator<AdapterEvent, IterationResponse> { | ||
| const iterParsed: OcxParsedRequest = { | ||
| ...parsed, stream: true, | ||
| context: { ...parsed.context, messages, tools: forceFinal ? toolsNoImage : allTools }, |
There was a problem hiding this comment.
When the client sends tool_choice: {type: "image_generation"}, the parser maps it to {name: "image_gen"} and every loop iteration preserves that choice. On a forced-final iteration (maxRounds: 0 or after the round cap), this line removes image_gen from the tools but leaves the named choice intact; when the usual Codex tools remain, OpenAI-chat and Anthropic serialize a tool choice naming an undeclared tool and the upstream rejects the request instead of returning a final answer. Clear or remap the image-specific tool choice whenever forceFinal is true, with a focused forced-choice regression test.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
| }, | ||
| }; | ||
|
|
||
| const req = https.request(optionsHttps, (res: IncomingMessage) => { |
There was a problem hiding this comment.
When a user relies on config.proxy or HTTPS_PROXY because direct internet egress is blocked, this direct node:https request bypasses the proxy environment that applyProxyEnv explicitly configures for Bun fetch. xAI responses containing hosted URLs therefore reach the Images API successfully but fail during artifact download, producing image generation returned no usable images. Keep the DNS-pinning guarantee while using a proxy-aware transport, and cover the configured-proxy path in the image download tests.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
| pruneArtifacts(plan.artifactsKeepCount); | ||
| const retained = files.filter((p) => existsSync(p)); |
There was a problem hiding this comment.
With concurrent image turns and a tight artifactsKeepCount, this global prune races with other fulfillments: one call can delete another call's newly written artifact before its existsSync filter, or immediately after that filter but before the path is returned. For example, two simultaneous calls with a keep count of 1 can make one successful xAI generation report failure or return a dangling path. Serialize write/prune/filter operations or exclude all in-flight batches from pruning, and add a concurrent retention regression test.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
| // Eagerly acquire only the FIRST iteration's final headers so connect/header/HTTP failures remain | ||
| // non-2xx JSON. Skip for runTurn adapters: their "headers" are synthetic, and awaiting | ||
| // queue.collect() before returning SSE starves clients of headers/heartbeats on slow first turns. | ||
| const skipEagerDrain = !!adapter.runTurn; |
There was a problem hiding this comment.
For an image-enabled combo target backed by a runTurn adapter such as Cursor, skipping the eager drain returns an HTTP 200 SSE response before the adapter has emitted anything. handleComboResponses consequently commits that target as successful even when its first meaningful event is an immediate error or the adapter ends empty, so the client receives a failed stream instead of falling back to the next combo target; the normal runTurn branch explicitly calls preflightAdapterEvents for this case. Preserve early headers for ordinary requests, but preflight image-loop runTurn attempts when comboAttempt is set and add the corresponding fallback regression.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
| yield { | ||
| type: "error", | ||
| message: e instanceof LoopError ? e.message : (e instanceof Error ? e.message : String(e)), | ||
| ...(e instanceof LoopError ? { status: e.status } : {}), | ||
| }; |
There was a problem hiding this comment.
When at least one image round has been discarded and a later routed-model iteration fails or stalls, hiddenUsage already contains the earlier model consumption, but this catch emits an error without attaching that usage. bridgeToResponsesSSE only invokes onUsage for an error carrying event.usage, so request logs report the failed turn as unreported/zero and omit all tokens consumed before the failure. Attach the accumulated usage to the terminal error, merging any available partial usage, and cover a successful image round followed by a failed iteration.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
| path: primary, | ||
| files: retained, | ||
| count: retained.length, | ||
| markdown: ``, |
There was a problem hiding this comment.
On Windows, primary is an absolute path such as C:\Users\name\.opencodex\artifacts\img.png; embedding it directly as a Markdown destination makes backslashes act as escapes and leaves the drive/path in a form renderers do not resolve. Because the synthetic tool is hidden, the downstream model commonly repeats this supplied markdown field as the only user-visible image, yielding a broken image even though the artifact exists. Keep path and files as native filesystem paths, but construct markdown from an escaped file: URI and add a Windows-path test.
AGENTS.md reference: AGENTS.md:L36-L37
Useful? React with 👍 / 👎.
| const artifactsKeepCount = | ||
| typeof keepRaw === "number" && Number.isFinite(keepRaw) ? Math.floor(keepRaw) : undefined; | ||
| return { | ||
| provider: found.provider, |
There was a problem hiding this comment.
When the configured xAI provider has an apiKeyPool, the plan keeps only the current provider/key snapshot and discards found.name. If that active key receives a 429, callXaiImages exposes the status but fulfillment converts it to an ordinary tool error, so the bridge repeatedly uses the same exhausted key and never invokes the repository's rotateProviderTransportOn429 path that other provider requests use. Carry the provider name through the plan and rotate/retry boundedly on xAI 429 responses, with a multi-key image-fulfillment regression.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
| if (!resp.ok) { | ||
| const err = new Error("xAI images API returned " + resp.status) as Error & { status: number }; | ||
| err.status = resp.status; | ||
| throw err; |
There was a problem hiding this comment.
When the xAI Images endpoint returns a non-2xx response, this branch throws without consuming or cancelling resp.body. A rate-limit or upstream failure with a slow or large body therefore leaves its connection active until the 60-second abort deadline; repeated image calls and concurrent turns can accumulate these unread sockets even though the failures were already handled. Cancel the body before throwing and extend the non-2xx test to verify cancellation.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
| internalAbort.abort("client closed responses stream"); | ||
| }, 2_000, | ||
| { | ||
| responseId: "", |
There was a problem hiding this comment.
This unconditionally assigns the empty response ID even for ordinary HTTP requests, unlike the normal bridge path where empty IDs are limited to the WebSocket compatibility option. onCompletedResponse then stores continuation state under "", while previousResponseProviderState rejects a falsy response ID, so Cursor conversation state recorded after an image turn is unreachable and every image response also overwrites the same useless cache entry. Generate a normal ID for HTTP image turns and propagate forceEmptyResponseId only for the WebSocket path, with a continuation regression.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
Same class of Windows CI flake as the injected satellite rollback cases — the threads-table fail-closed path can exceed bun's default 5s harness budget.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/guides/image-bridge.md`:
- Around line 86-95: Update the “Streaming only” limitation in the image-bridge
documentation to describe the “Responses tool-event streaming path” instead of
intercepting the SSE response stream. Preserve the existing claim that stream:
false requests are rejected with a 400 error.
In `@src/images/artifacts.ts`:
- Around line 63-72: Make the stat pass in pruneArtifacts resilient per entry:
catch statSync failures for individual files, warn as appropriate, and skip only
those entries instead of aborting the entire entries.map operation. Preserve
successfully collected stats so pruning still enforces the configured cap on
remaining artifacts, including when fulfillImageCall batches overlap.
- Around line 365-371: Update downloadImageToArtifact and materializeInlineImage
to reserve bytes synchronously on the shared ImageBudget before any await or
validation, rejecting when the reservation would exceed
MAX_DECODED_BYTES_PER_RESPONSE. Refund each reservation whenever the operation
fails, including directory/IO errors and rejected non-image payloads, while
retaining the charge on success.
In `@src/images/fulfill.ts`:
- Around line 40-52: Validate the selected imageUrl in the fulfillment flow
before calling callXaiImages, reusing assertUrlResolvesPublic() and the existing
destination-policy behavior; reject unsafe or disallowed URLs without
dispatching the edit request, while preserving the current fallback between
obj.image_url, obj.image, and undefined.
In `@src/images/loop.ts`:
- Around line 467-471: Update the validation around terminalIndexes to
distinguish an incorrect terminal-event count from a correctly counted terminal
event that is not last. Preserve the existing LoopError behavior, but make the
message describe the misplaced-terminal condition without reporting a misleading
count; use terminalIndexes and events to identify the two cases.
In `@src/images/plan.ts`:
- Around line 16-29: Update findXaiProvider and the planImageBridge flow to
evaluate every enabled xAI provider matching the known names or hostnames
instead of returning the first hostname match. Select the first matching
provider that can produce a usable API token, skipping OAuth-only or keyless
entries so earlier config order cannot shadow later credentials; preserve the
existing undefined result when none are credentialed.
In `@src/images/xai-client.ts`:
- Around line 39-54: Update mapSizeToAspectRatio to reject zero or otherwise
non-positive parsed width and height before calculating the ratio, returning
undefined so malformed sizes are dropped instead of defaulting to
XAI_ASPECT_RATIOS[0]. Preserve the existing nearest-ratio mapping for valid
positive dimensions.
- Around line 103-107: Update the non-2xx branch in the xAI response handling to
cancel the response body and release its reader before constructing and throwing
the status error. Preserve the existing body-free error message and status
assignment, and keep the success-path cleanup unchanged.
- Around line 109-132: Reduce MAX_RESPONSE_BYTES in the response-reading logic
to a substantially smaller cap that remains above the maximum valid response
produced by the downstream materializeInlineImage artifact limits. Keep the
existing byte-count enforcement and error behavior, and update the nearby
comment so it accurately describes the tighter memory protection.
In `@src/lib/destination-policy.ts`:
- Around line 254-261: Derive ipKind from isIP(address) before trusting the
resolver-reported family in the address loop, using family only when the literal
cannot determine a valid IPv4 or IPv6 kind. Update the classification and
publicAddresses logic around the ipKind calculation so IPv6 literals cannot
reach classifyIpv4, while preserving the existing rejection and address-family
fallback behavior.
In `@src/responses/parser.ts`:
- Around line 621-624: Update the `_webSearch` detection flow in the relevant
parser logic to inspect the same merged tool sources as
`extractHostedImageGeneration`, including `loadedToolSpecs` and `data.tools`.
Preserve the existing handling for top-level tools while ensuring hosted
`web_search` surfaced through `additional_tools` is recognized consistently.
- Around line 111-114: Update mapToolChoice() so the hosted
image_generation/image_gen rewrite to IMAGE_GEN_TOOL_NAME occurs only when the
image bridge is available, matching buildTools() behavior. Otherwise preserve a
valid non-image tool choice, or remove IMAGE_GEN_TOOL_NAME from
options.toolChoice in the responses core flow when no bridge is armed.
In `@src/server/responses/core.ts`:
- Around line 1552-1561: Extract the duplicated image-tool filtering predicate
into a shared exported helper near the image-tool utilities, such as
withoutImageTools, accepting the tools and image plan toolNames. Update both the
forced-final path in parsed.context.tools and the toolsNoImage construction in
the image loop to call this helper, preserving the current filtering behavior
and avoiding separate predicates.
In `@tests/images/artifacts-prune.test.ts`:
- Around line 69-90: Update the “writing >keepCount images then pruning keeps
newest” test to assign deterministic, strictly increasing mtimes to each
materialized artifact using the existing utimesSync approach from the earlier
unit test. Remove the 5 ms sleep and stamp each path after creation, preserving
the existing pruning and per-index assertions.
In `@tests/images/artifacts-ssrf.test.ts`:
- Around line 86-127: Replace every lookupMock.mockClear() cleanup in the
resolvePublicAddresses tests with resetLookup(), ensuring both call history and
mocked implementations are reset after each test. Preserve the declared default
async empty-result behavior so tests remain isolated and fail closed regardless
of execution order.
In `@tests/images/loop.test.ts`:
- Around line 212-250: Add a focused regression test alongside the existing
parallel image-call test that generates six parallel calls across two rounds,
with maxRounds set to 2. Capture toolResult messages containing the
budget-exhausted error, verify exactly 12 attempts minus
MAX_IMAGE_CALLS_PER_TURN are refused, and assert the error includes the
configured per-turn maximum; import and reference MAX_IMAGE_CALLS_PER_TURN from
the image loop.
In `@tests/images/pinned-https-get.test.ts`:
- Around line 57-108: Extend the test “lookup honors scalar and { all: true }
callback shapes” to invoke capturedLookup with the callback in the second
argument, covering the typeof lookupOptions === "function" branch in
pinnedHttpsGet. Assert the callback receives no error and the pinned address and
family, while preserving the existing scalar and all-address assertions.
- Around line 48-54: Add cleanup for the repeated node:https mocks by
registering afterAll to call mock.restoreModule("node:https") in this test
suite, preserving the existing requestMock setup and preventing the final stub
from leaking into later tests.
In `@tests/images/plan.test.ts`:
- Around line 145-168: Translate the four Chinese comments in the tests around
“baseUrl is pinned to registry regardless of config override” and “custom-named
provider with api.x.ai baseUrl does NOT get built-in OAuth token” into clear
English. Preserve the existing assertions and test behavior unchanged, retaining
the security intent about baseUrl pinning and preventing OAuth fallback for
custom-named providers.
- Around line 29-32: Update the image planner tests around makeConfig to expose
artifactsKeepCount and add focused coverage for finite, zero, and negative
values, verifying the planner’s handling in src/images/plan.ts. Remove any
maxRounds-related test or configuration from this file, since it belongs to the
image loop. Delete the unused OAuth mock and tokenResult plumbing because the
planner does not import the OAuth module.
In `@tests/images/synthetic-tool.test.ts`:
- Around line 22-78: Add tests in the extractHostedImageGeneration suite
covering a hosted tool with type "image_gen" and a hosted entry with an explicit
name distinct from its type. Assert activation succeeds and toolNames contains
the expected preferred name, preserving coverage for both accepted hosted forms
and the name-over-type behavior.
In `@tests/images/xai-client.test.ts`:
- Around line 87-101: Make the timeout assertion in the “timeoutMs composes a
deadline that aborts the fetch signal” test tolerant of event-loop delays by
polling until seenSignal.aborted becomes true, returning immediately when it
fires and using a bounded fallback timeout to avoid hanging. Preserve the
existing signal setup and assert that the signal is ultimately aborted.
In `@tests/images/z-fulfill.test.ts`:
- Around line 63-74: Register the existing reset function with the test
framework using beforeEach(reset) so all module-level mock state is restored
before every test. Then remove the redundant per-test reset() calls while
preserving each test’s existing setup and assertions.
In `@tests/images/z-handler-activation.test.ts`:
- Around line 82-102: Update the src/web-search/index mock to spread the actual
module exports before applying the test-specific overrides, preserving all
unmocked APIs. Reset mockWsPlan in the test setup before each test, and remove
the redundant per-test reset and cleanup-only restoration once setup guarantees
isolation. Keep the existing web-search behavior and useRunTurnAdapter
restoration unchanged.
In `@tests/responses-parser.test.ts`:
- Around line 84-94: Add an assertion to the test around parseRequest that
verifies parsed.context.tools contains the tool named by
parsed.options.toolChoice, specifically ensuring the image_gen synthetic tool is
present alongside the mapped tool choice. Update the implementation if needed so
this invariant holds before the bridge arms, and preserve the existing
image-generation parsing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6cd7ba67-3ebf-437a-b0c0-06be1372c27e
📒 Files selected for processing (26)
docs-site/src/content/docs/guides/codex-integration.mddocs-site/src/content/docs/guides/image-bridge.mdsrc/images/artifacts.tssrc/images/fulfill.tssrc/images/index.tssrc/images/loop.tssrc/images/plan.tssrc/images/synthetic-tool.tssrc/images/types.tssrc/images/xai-client.tssrc/lib/destination-policy.tssrc/responses/parser.tssrc/server/responses/core.tssrc/types.tstests/destination-policy-resolved.test.tstests/images/artifacts-prune.test.tstests/images/artifacts-ssrf.test.tstests/images/loop.test.tstests/images/pinned-https-get.test.tstests/images/plan.test.tstests/images/synthetic-tool.test.tstests/images/xai-client.test.tstests/images/z-fulfill.test.tstests/images/z-handler-activation.test.tstests/responses-parser.test.tstests/storage-cleanup.test.ts
| ## Limitations | ||
|
|
||
| - **Only xAI Grok Imagine is supported.** DALL-E and other image providers may be added later. | ||
| - **Web search takes priority** on adapters that support the web-search sidecar loop. If both web | ||
| search and image generation are requested in the same turn, web-search runs and image | ||
| generation is skipped. Cursor/`runTurn` adapters cannot use that sidecar today, so the image | ||
| bridge may still run for those dual-tool turns. | ||
| - **xAI costs apply.** Image generation via xAI requires an active xAI subscription or API credits. | ||
| - **Streaming only.** The bridge works by intercepting the SSE response stream; requests with | ||
| `stream: false` are rejected with a 400 error. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# What does the Responses handler actually do for stream:false + hosted image_generation?
ast-grep outline src/server/responses/core.ts --items all --match 'Image|image'
rg -nP -C6 'runWithImageBridge|imageBridge|planImageBridge' src/server/responses/core.ts
rg -nP -C4 'stream' src/images/plan.ts src/images/loop.ts | rg -nP -C4 '400|stream'Repository: lidge-jun/opencodex
Length of output: 15374
Clarify the streaming-path wording. In docs-site/src/content/docs/guides/image-bridge.md:94-95, keep the stream: false → 400 claim, but rephrase the lead-in to “Responses tool-event streaming path” instead of “intercepting the SSE response stream”; src/server/responses/core.ts:1545-1549 rejects non-streaming image-bridge requests with 400 image bridge requires stream=true, and src/images/loop.ts:287-290 shows runTurn adapters replay queued events into the bridge rather than exposing raw SSE.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs-site/src/content/docs/guides/image-bridge.md` around lines 86 - 95,
Update the “Streaming only” limitation in the image-bridge documentation to
describe the “Responses tool-event streaming path” instead of intercepting the
SSE response stream. Preserve the existing claim that stream: false requests are
rejected with a 400 error.
Source: Path instructions
| let stats: Array<{ name: string; mtime: number }>; | ||
| try { | ||
| stats = entries.map(name => { | ||
| const st = statSync(join(dir, name)); | ||
| return { name, mtime: st.mtimeMs }; | ||
| }); | ||
| } catch (e) { | ||
| console.warn(`[images] prune: could not stat files in ${dir}:`, e instanceof Error ? e.message : e); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Make the stat pass per-entry resilient — one vanished file currently disables the whole prune.
entries.map at lines 65-68 runs inside a single try block, so a single statSync ENOENT (a concurrent prune from a parallel image batch, or the user deleting an artifact) aborts the entire retention pass at line 71 and leaves the directory over the configured cap until the next fulfilled call. Since fulfillImageCall (src/images/fulfill.ts) calls pruneArtifacts after every batch and batches can overlap, this is a reachable path. Skipping unstattable entries preserves the documented best-effort contract while still enforcing the cap on everything that is still there.
♻️ Proposed fix: skip unstattable entries instead of aborting
- let stats: Array<{ name: string; mtime: number }>;
- try {
- stats = entries.map(name => {
- const st = statSync(join(dir, name));
- return { name, mtime: st.mtimeMs };
- });
- } catch (e) {
- console.warn(`[images] prune: could not stat files in ${dir}:`, e instanceof Error ? e.message : e);
- return;
- }
+ const stats: Array<{ name: string; mtime: number }> = [];
+ for (const name of entries) {
+ try {
+ const st = statSync(join(dir, name));
+ if (st.isFile()) stats.push({ name, mtime: st.mtimeMs });
+ } catch (e) {
+ // A file removed concurrently must not abort the whole retention pass.
+ console.warn(`[images] prune: could not stat ${name}:`, e instanceof Error ? e.message : e);
+ }
+ }
+ if (stats.length <= maxFiles) return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let stats: Array<{ name: string; mtime: number }>; | |
| try { | |
| stats = entries.map(name => { | |
| const st = statSync(join(dir, name)); | |
| return { name, mtime: st.mtimeMs }; | |
| }); | |
| } catch (e) { | |
| console.warn(`[images] prune: could not stat files in ${dir}:`, e instanceof Error ? e.message : e); | |
| return; | |
| } | |
| const stats: Array<{ name: string; mtime: number }> = []; | |
| for (const name of entries) { | |
| try { | |
| const st = statSync(join(dir, name)); | |
| if (st.isFile()) stats.push({ name, mtime: st.mtimeMs }); | |
| } catch (e) { | |
| // A file removed concurrently must not abort the whole retention pass. | |
| console.warn(`[images] prune: could not stat ${name}:`, e instanceof Error ? e.message : e); | |
| } | |
| } | |
| if (stats.length <= maxFiles) return; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/images/artifacts.ts` around lines 63 - 72, Make the stat pass in
pruneArtifacts resilient per entry: catch statSync failures for individual
files, warn as appropriate, and skip only those entries instead of aborting the
entire entries.map operation. Preserve successfully collected stats so pruning
still enforces the configured cap on remaining artifacts, including when
fulfillImageCall batches overlap.
| const imageUrl = | ||
| typeof obj.image_url === "string" ? obj.image_url : typeof obj.image === "string" ? obj.image : undefined; | ||
| const size = typeof obj.size === "string" ? obj.size : plan.defaultSize; | ||
| const quality = typeof obj.quality === "string" ? obj.quality : plan.defaultQuality; | ||
|
|
||
| let result; | ||
| try { | ||
| result = await callXaiImages( | ||
| { prompt, model: plan.model, n, imageUrl, size, quality }, | ||
| plan.auth, | ||
| signal, | ||
| plan.timeoutMs, | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm a reusable destination/scheme policy exists and that the edits path never calls it.
fd -t f 'destination-policy*' -x cat -n {}
echo '--- exported policy helpers ---'
fd -t f 'destination-policy*' --exec ast-grep outline {} --items all
echo '--- who validates image URLs? ---'
rg -nP --type=ts -C3 '\b(assert|check|validate|is)\w*(Destination|Url|Host|Ip)\w*\s*\(' src | head -60
echo '--- any validation on the inbound edits URL? ---'
rg -nP --type=ts -C4 '\bimage_url\b' srcRepository: lidge-jun/opencodex
Length of output: 41262
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/images/fulfill.ts ---'
cat -n src/images/fulfill.ts | sed -n '1,140p'
echo '--- src/images/xai-client.ts ---'
cat -n src/images/xai-client.ts | sed -n '1,180p'
echo '--- src/lib/destination-policy.ts (relevant exports) ---'
cat -n src/lib/destination-policy.ts | sed -n '200,280p'Repository: lidge-jun/opencodex
Length of output: 13390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- image-path URL guards ---'
rg -n --type=ts 'resolvePublicAddresses|assertUrlResolvesPublic|assessUrlDestination|destination-policy|imageUrl' src/images src/lib src/chat src/responses src/adapters | sed -n '1,200p'
echo '--- image bridge types ---'
cat -n src/images/types.ts | sed -n '1,220p'
echo '--- any image URL validation in fulfill or artifacts ---'
rg -n --type=ts 'assertUrlResolvesPublic|resolvePublicAddresses|downloadImageToArtifact|image_url|imageUrl' src/images | sed -n '1,200p'Repository: lidge-jun/opencodex
Length of output: 5488
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/images/artifacts.ts (relevant section) ---'
cat -n src/images/artifacts.ts | sed -n '280,360p'Repository: lidge-jun/opencodex
Length of output: 3997
Validate image_url before forwarding edit requests
src/images/fulfill.ts:40-48 takes obj.image_url / obj.image from the tool call and sends it straight to callXaiImages(), which places it in the /images/edits body in src/images/xai-client.ts:70-84. That bypasses the same HTTPS/DNS destination checks used for provider-returned URLs in src/images/artifacts.ts:311-325 via resolvePublicAddresses() in src/lib/destination-policy.ts:222-270. Reject unsafe edit URLs here before dispatching the request, ideally by reusing assertUrlResolvesPublic() so both directions enforce the same policy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/images/fulfill.ts` around lines 40 - 52, Validate the selected imageUrl
in the fulfillment flow before calling callXaiImages, reusing
assertUrlResolvesPublic() and the existing destination-policy behavior; reject
unsafe or disallowed URLs without dispatching the edit request, while preserving
the current fallback between obj.image_url, obj.image, and undefined.
Source: Path instructions
| const terminalIndexes = events.flatMap((event, index) => | ||
| event.type === "done" || event.type === "incomplete" || event.type === "error" ? [index] : []); | ||
| if (terminalIndexes.length !== 1 || terminalIndexes[0] !== events.length - 1) { | ||
| throw new LoopError(502, `Image-bridge adapter stream protocol error: expected one final terminal event, received ${terminalIndexes.length}`); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
The protocol-error message contradicts itself when the terminal event is merely misplaced.
The guard rejects two different conditions — wrong terminal-event count, and a terminal event that isn't last — but interpolates only terminalIndexes.length. A stream ending with done followed by a stray text_delta produces received 1, i.e. "expected one final terminal event, received 1", which sends the reader looking for a counting bug that doesn't exist. Distinguish the two cases.
♻️ Proposed refactor
- if (terminalIndexes.length !== 1 || terminalIndexes[0] !== events.length - 1) {
- throw new LoopError(502, `Image-bridge adapter stream protocol error: expected one final terminal event, received ${terminalIndexes.length}`);
- }
+ if (terminalIndexes.length !== 1) {
+ throw new LoopError(502, `Image-bridge adapter stream protocol error: expected exactly one terminal event, received ${terminalIndexes.length}`);
+ }
+ if (terminalIndexes[0] !== events.length - 1) {
+ throw new LoopError(502, `Image-bridge adapter stream protocol error: terminal event at index ${terminalIndexes[0]} of ${events.length} events is not last`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const terminalIndexes = events.flatMap((event, index) => | |
| event.type === "done" || event.type === "incomplete" || event.type === "error" ? [index] : []); | |
| if (terminalIndexes.length !== 1 || terminalIndexes[0] !== events.length - 1) { | |
| throw new LoopError(502, `Image-bridge adapter stream protocol error: expected one final terminal event, received ${terminalIndexes.length}`); | |
| } | |
| const terminalIndexes = events.flatMap((event, index) => | |
| event.type === "done" || event.type === "incomplete" || event.type === "error" ? [index] : []); | |
| if (terminalIndexes.length !== 1) { | |
| throw new LoopError(502, `Image-bridge adapter stream protocol error: expected exactly one terminal event, received ${terminalIndexes.length}`); | |
| } | |
| if (terminalIndexes[0] !== events.length - 1) { | |
| throw new LoopError(502, `Image-bridge adapter stream protocol error: terminal event at index ${terminalIndexes[0]} of ${events.length} events is not last`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/images/loop.ts` around lines 467 - 471, Update the validation around
terminalIndexes to distinguish an incorrect terminal-event count from a
correctly counted terminal event that is not last. Preserve the existing
LoopError behavior, but make the message describe the misplaced-terminal
condition without reporting a misleading count; use terminalIndexes and events
to identify the two cases.
| test("timeoutMs composes a deadline that aborts the fetch signal", async () => { | ||
| let seenSignal: AbortSignal | undefined; | ||
| globalThis.fetch = (async (_input, init) => { | ||
| seenSignal = init?.signal; | ||
| return new Response(JSON.stringify({ data: [{ b64_json: "dGVzdA==" }] }), { | ||
| status: 200, | ||
| headers: { "content-type": "application/json" }, | ||
| }); | ||
| }) as typeof fetch; | ||
| await callXaiImages({ prompt: "x" }, AUTH, undefined, 50); | ||
| expect(seenSignal).toBeDefined(); | ||
| expect(seenSignal!.aborted).toBe(false); | ||
| await new Promise(resolve => setTimeout(resolve, 60)); | ||
| expect(seenSignal!.aborted).toBe(true); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
10 ms of slack between the 50 ms deadline and the 60 ms wait will flake in CI.
Line 96 arms a 50 ms AbortSignal.timeout, Line 99 waits 60 ms, and Line 100 asserts the signal has fired. Any event-loop stall or timer coalescing beyond 10 ms — routine on loaded shared runners, and the PR notes cross-platform CI is still pending — makes this fail with no product defect. Widen the gap, or better, poll until the signal flips so the test is fast in the common case and tolerant under load.
💚 Proposed fix: poll for the abort instead of racing a fixed sleep
await callXaiImages({ prompt: "x" }, AUTH, undefined, 50);
expect(seenSignal).toBeDefined();
expect(seenSignal!.aborted).toBe(false);
- await new Promise(resolve => setTimeout(resolve, 60));
+ const deadline = Date.now() + 2_000;
+ while (!seenSignal!.aborted && Date.now() < deadline) {
+ await new Promise(resolve => setTimeout(resolve, 10));
+ }
expect(seenSignal!.aborted).toBe(true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("timeoutMs composes a deadline that aborts the fetch signal", async () => { | |
| let seenSignal: AbortSignal | undefined; | |
| globalThis.fetch = (async (_input, init) => { | |
| seenSignal = init?.signal; | |
| return new Response(JSON.stringify({ data: [{ b64_json: "dGVzdA==" }] }), { | |
| status: 200, | |
| headers: { "content-type": "application/json" }, | |
| }); | |
| }) as typeof fetch; | |
| await callXaiImages({ prompt: "x" }, AUTH, undefined, 50); | |
| expect(seenSignal).toBeDefined(); | |
| expect(seenSignal!.aborted).toBe(false); | |
| await new Promise(resolve => setTimeout(resolve, 60)); | |
| expect(seenSignal!.aborted).toBe(true); | |
| }); | |
| test("timeoutMs composes a deadline that aborts the fetch signal", async () => { | |
| let seenSignal: AbortSignal | undefined; | |
| globalThis.fetch = (async (_input, init) => { | |
| seenSignal = init?.signal; | |
| return new Response(JSON.stringify({ data: [{ b64_json: "dGVzdA==" }] }), { | |
| status: 200, | |
| headers: { "content-type": "application/json" }, | |
| }); | |
| }) as typeof fetch; | |
| await callXaiImages({ prompt: "x" }, AUTH, undefined, 50); | |
| expect(seenSignal).toBeDefined(); | |
| expect(seenSignal!.aborted).toBe(false); | |
| const deadline = Date.now() + 2_000; | |
| while (!seenSignal!.aborted && Date.now() < deadline) { | |
| await new Promise(resolve => setTimeout(resolve, 10)); | |
| } | |
| expect(seenSignal!.aborted).toBe(true); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/images/xai-client.test.ts` around lines 87 - 101, Make the timeout
assertion in the “timeoutMs composes a deadline that aborts the fetch signal”
test tolerant of event-loop delays by polling until seenSignal.aborted becomes
true, returning immediately when it fires and using a bounded fallback timeout
to avoid hanging. Preserve the existing signal setup and assert that the signal
is ultimately aborted.
| function reset(): void { | ||
| xaiResult = { images: [{ b64_json: "dGVzdA==" }] }; | ||
| xaiError = null; | ||
| xaiCalls.length = 0; | ||
| capturedTimeoutMs = undefined; | ||
| matIdx = 0; | ||
| dlIdx = 0; | ||
| pruneCalls = 0; | ||
| pruneImpl = () => { pruneCalls++; }; | ||
| materializeFn = async (i) => touchArtifact(`img-${i}.png`); | ||
| downloadFn = async (i) => touchArtifact(`dl-${i}.png`); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Wire reset() into beforeEach so state leakage can't silently reappear.
All of the mock state on Lines 36-46 is module-level and mutable, and isolation depends entirely on every test remembering reset() as its first statement (Lines 78, 88, 98, 105, 112, 122, 130, 138, 146, 155, 180, 192, 201, 210, 225). The first test that forgets inherits the previous test's materializeFn — e.g. the throwing stub from Line 139 — and fails for a reason unrelated to its own assertions. A single beforeEach(reset) makes the invariant structural instead of a convention.
♻️ Proposed refactor
+import { afterAll, beforeAll, beforeEach, describe, expect, mock, test } from "bun:test"; function reset(): void {
xaiResult = { images: [{ b64_json: "dGVzdA==" }] };
xaiError = null;
xaiCalls.length = 0;
capturedTimeoutMs = undefined;
matIdx = 0;
dlIdx = 0;
pruneCalls = 0;
pruneImpl = () => { pruneCalls++; };
materializeFn = async (i) => touchArtifact(`img-${i}.png`);
downloadFn = async (i) => touchArtifact(`dl-${i}.png`);
}
+
+beforeEach(reset);The per-test reset() calls then become redundant and can be dropped.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function reset(): void { | |
| xaiResult = { images: [{ b64_json: "dGVzdA==" }] }; | |
| xaiError = null; | |
| xaiCalls.length = 0; | |
| capturedTimeoutMs = undefined; | |
| matIdx = 0; | |
| dlIdx = 0; | |
| pruneCalls = 0; | |
| pruneImpl = () => { pruneCalls++; }; | |
| materializeFn = async (i) => touchArtifact(`img-${i}.png`); | |
| downloadFn = async (i) => touchArtifact(`dl-${i}.png`); | |
| } | |
| function reset(): void { | |
| xaiResult = { images: [{ b64_json: "dGVzdA==" }] }; | |
| xaiError = null; | |
| xaiCalls.length = 0; | |
| capturedTimeoutMs = undefined; | |
| matIdx = 0; | |
| dlIdx = 0; | |
| pruneCalls = 0; | |
| pruneImpl = () => { pruneCalls++; }; | |
| materializeFn = async (i) => touchArtifact(`img-${i}.png`); | |
| downloadFn = async (i) => touchArtifact(`dl-${i}.png`); | |
| } | |
| beforeEach(reset); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/images/z-fulfill.test.ts` around lines 63 - 74, Register the existing
reset function with the test framework using beforeEach(reset) so all
module-level mock state is restored before every test. Then remove the redundant
per-test reset() calls while preserving each test’s existing setup and
assertions.
| mock.module("../../src/web-search/index", () => ({ | ||
| buildWebSearchTool: () => ({ name: "web_search", parameters: { type: "object", properties: {} } }), | ||
| WEB_SEARCH_TOOL_NAME: "web_search", | ||
| extractHostedWebSearch: (tools: unknown[]) => { | ||
| if (!Array.isArray(tools)) return undefined; | ||
| for (const t of tools) { | ||
| if (t && typeof t === "object" && (t as Record<string, unknown>).type === "web_search") { | ||
| return { search_context_size: "medium" }; | ||
| } | ||
| } | ||
| return undefined; | ||
| }, | ||
| runWithWebSearch: async () => { | ||
| webSearchRun = true; | ||
| return new Response("data: {\"type\":\"done\"}\n\n", { | ||
| status: 200, headers: { "content-type": "text/event-stream" }, | ||
| }); | ||
| }, | ||
| planWebSearch: () => mockWsPlan, | ||
| shouldResolveOpenAiWebSearchSidecar: () => false, | ||
| })); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
This mock.module replaces all of src/web-search/index instead of spreading the real module — unlike the two mocks above it.
Lines 44-46 and 71-73 both do { ...actualModule, override }, but Lines 82-102 hand-build the entire surface. Every other export of src/web-search/index becomes undefined for the whole test process, so the day someone imports one more symbol from that module in src/server/responses/core.ts (or in anything it pulls in), this suite fails with an opaque x is not a function that points nowhere near the real cause.
Separately: Line 188 sets mockWsPlan = { backend: "openai" } and the finally on Lines 195-197 restores only useRunTurnAdapter, so the plan leaks past that test. It happens to be harmless today because Line 203 reassigns it, but a beforeEach reset would make that independent of test order.
♻️ Proposed fix
- mock.module("../../src/web-search/index", () => ({
+ const actualWebSearch = await import("../../src/web-search/index");
+ mock.module("../../src/web-search/index", () => ({
+ ...actualWebSearch,
buildWebSearchTool: () => ({ name: "web_search", parameters: { type: "object", properties: {} } }),+beforeEach(() => {
+ imageBridgeRun = false;
+ webSearchRun = false;
+ runTurnCalled = false;
+ useRunTurnAdapter = false;
+ mockWsPlan = undefined;
+});(then the per-test reset lines and the finally blocks can go away)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mock.module("../../src/web-search/index", () => ({ | |
| buildWebSearchTool: () => ({ name: "web_search", parameters: { type: "object", properties: {} } }), | |
| WEB_SEARCH_TOOL_NAME: "web_search", | |
| extractHostedWebSearch: (tools: unknown[]) => { | |
| if (!Array.isArray(tools)) return undefined; | |
| for (const t of tools) { | |
| if (t && typeof t === "object" && (t as Record<string, unknown>).type === "web_search") { | |
| return { search_context_size: "medium" }; | |
| } | |
| } | |
| return undefined; | |
| }, | |
| runWithWebSearch: async () => { | |
| webSearchRun = true; | |
| return new Response("data: {\"type\":\"done\"}\n\n", { | |
| status: 200, headers: { "content-type": "text/event-stream" }, | |
| }); | |
| }, | |
| planWebSearch: () => mockWsPlan, | |
| shouldResolveOpenAiWebSearchSidecar: () => false, | |
| })); | |
| const actualWebSearch = await import("../../src/web-search/index"); | |
| mock.module("../../src/web-search/index", () => ({ | |
| ...actualWebSearch, | |
| buildWebSearchTool: () => ({ name: "web_search", parameters: { type: "object", properties: {} } }), | |
| WEB_SEARCH_TOOL_NAME: "web_search", | |
| extractHostedWebSearch: (tools: unknown[]) => { | |
| if (!Array.isArray(tools)) return undefined; | |
| for (const t of tools) { | |
| if (t && typeof t === "object" && (t as Record<string, unknown>).type === "web_search") { | |
| return { search_context_size: "medium" }; | |
| } | |
| } | |
| return undefined; | |
| }, | |
| runWithWebSearch: async () => { | |
| webSearchRun = true; | |
| return new Response("data: {\"type\":\"done\"}\n\n", { | |
| status: 200, headers: { "content-type": "text/event-stream" }, | |
| }); | |
| }, | |
| planWebSearch: () => mockWsPlan, | |
| shouldResolveOpenAiWebSearchSidecar: () => false, | |
| })); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/images/z-handler-activation.test.ts` around lines 82 - 102, Update the
src/web-search/index mock to spread the actual module exports before applying
the test-specific overrides, preserving all unmocked APIs. Reset mockWsPlan in
the test setup before each test, and remove the redundant per-test reset and
cleanup-only restoration once setup guarantees isolation. Keep the existing
web-search behavior and useRunTurnAdapter restoration unchanged.
Clear forced image tool_choice on final pass, keep HTTP response IDs unless WS empty-id is requested, attach hiddenUsage on loop errors, serialize artifact retention, emit file: Markdown URIs, and cancel xAI error bodies.
|
Pushed Codex tip fixes:
Deferred as follow-ups (larger / lower urgency): proxy-aware pinned downloads, combo @codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/images/fulfill.ts`:
- Around line 12-24: Update the fulfillment flow around retainAfterBatch so each
batch’s artifact materialization/write, prune, and path filtering executes
within the shared retentionTail lock; keep the xAI request outside this critical
section. Ensure concurrent fulfillments with keepCount 1 cannot prune another
batch’s newly written files before that batch filters them, and add a focused
interleaved regression test under tests/.
In `@tests/images/loop.test.ts`:
- Around line 151-173: Extend the forced-final tests around runWithImageBridge
to cover toolChoice.allowedTools cases: preserve non-image tools when mixed with
image tools, and fall back to "auto" when only image tools are allowed. Add
focused cases matching the behavior implemented in the relevant src flow, while
retaining the existing named image tool_choice coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c4e9db8-b3fc-4649-8a8c-e99ff33ecfea
📒 Files selected for processing (7)
src/images/fulfill.tssrc/images/loop.tssrc/images/xai-client.tssrc/server/responses/core.tstests/images/loop.test.tstests/images/xai-client.test.tstests/images/z-fulfill.test.ts
Prefer isIP(address) over resolver-reported family, and charge the shared per-response ImageBudget atomically before any await.
|
Maintainer landing note: CI green on tip Huge thanks again to @tizerluo — full credit for the original image-bridge feature work remains yours. |
Reconcile with lidge-jun#355 Gemini inline artifact helpers: keep DNS-pinned downloads and HTTP artifact serving APIs in a shared artifacts module.
…akes These suites measured 5.6–7.5s on windows-latest under the default 5s harness budget after the lidge-jun#355 merge; give them the same 20s ceiling used elsewhere.
|
Landed via maintainer takeover #577 (squash-merged to @tizerluo — thank you so much for this PR. The Grok image bridge is a big contribution, and you retain full credit for the original feature work. The takeover only folded review/security hardening (#528 + maintainer follow-ups) and rebased onto latest |
1 participant
Summary
Maintainer takeover of #424 (Grok image bridge) that folds in stacked #528 and clears remaining review blockers on latest
dev.Credit: original feature work by @tizerluo on #424. This PR is the maintainer landing vehicle after rebasing, conflict resolution, and hardening.
Included
artifactsKeepCount) with post-batch prune (no mid-batch path wipe),<=0disables prune, Windows-safe paths, full PNG signature checkfunctiontools namedimage_genno longer arm the bridge)tool_choice: { type: "image_generation" }maps to required syntheticimage_genimage_gendistinctionCloses #424
Closes #528
Test plan
bun test --isolate tests/images/ tests/responses-parser.test.tsbun run typecheckSummary by CodeRabbit
New Features
Documentation
image_genbehaves versus the Responses-based bridge flow.Bug Fixes