feat(images): add Grok image bridge for non-OpenAI models - #424
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
fb94938 to
976da38
Compare
976da38 to
38d4112
Compare
38d4112 to
d4b9ac5
Compare
d4b9ac5 to
79da2f9
Compare
79da2f9 to
db48e7c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
db48e7c to
99fa843
Compare
This comment was marked as outdated.
This comment was marked as outdated.
통합(dev push 완료): #437 803807a, #460 74ddd96, #431 82a47db, #405 be16c1d, #468 후속 테스트 bcaf029. close: 통합 4건 + 통합 없이 1건(#459, 설계 충돌). #457은 OPEN 유지. 이슈 close 0건 — dev에서 고쳐졌는데 안 닫힌 이슈가 없었다. 무효화: WP4(#466)와 WP6(#467)은 동료가 직접 머지했고, 우리 분석이 지적한 결함을 동료 커밋이 같은 방향으로 해소했다. 보안 보류 5건에 신규 발견 게시: #429 재분류, #355 OAuth 토큰 목적지, #424 유료 브리지 기본 ON, #408 설치 락 해제, #403 4분할 요청. A-gate는 사이클마다 3~5라운드를 돌았다. 반복된 원인은 계획서에 테스트나 코드를 적으면서 실물 소스를 확인하지 않은 것이었다.
99fa843 to
b3f2b49
Compare
b3f2b49 to
eb7720e
Compare
- 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)
Replace lidge-jun#355's standalone artifacts.ts with lidge-jun#424's fuller version (adds downloadImageToArtifact, SSRF protection, HTTPS enforcement). Also bring in destination-policy.ts exports needed by the module. Removes lidge-jun#355's duplicate guessExtFromMagic — lidge-jun#424's version is the canonical one.
|
Re-reviewed PR #424 at The latest #424 commit fixes the Windows artifact-prune path handling and makes PR #528 already covers the earlier findings around Cursor conversation continuity, early SSE headers, stalled After accounting for #528, these concrete blockers still remain:
PR #528 currently diverges from the updated #424 branch, so it must be rebased onto the final #424 head before its CI result is meaningful and before merge. Current #424 exact-head workflows are still |
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.
Replace lidge-jun#355's standalone artifacts.ts with lidge-jun#424's fuller version (adds downloadImageToArtifact, SSRF protection, HTTPS enforcement). Also bring in destination-policy.ts exports needed by the module. Removes lidge-jun#355's duplicate guessExtFromMagic — lidge-jun#424's version is the canonical one.
- pruneOldArtifacts() caps artifacts/ to 200 files, deleting oldest by mtime - Runs after each successful write in materializeInlineImage + downloadImageToArtifact - Best-effort: errors caught and warned, never fails the write
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
- pruneOldArtifacts() deletes oldest files when count exceeds maxFiles - Default 200 files, configurable via images.artifactsKeepCount - Runs after each successful artifact write (both inline + download) - Add tests for prune logic and integration with materializeInlineImage - Document retention in image-bridge guide
- Use dirname/basename instead of lastIndexOf('/') for cross-platform paths
- Treat maxFiles<=0 as 'disable pruning' instead of deleting all files
- Add test verifying maxFiles=0 and maxFiles=-1 do not delete anything
guessExtFromMagic was checking only the first 4 bytes (\x89PNG) of the 8-byte PNG signature (89 50 4E 47 0D 0A 1A 0A), accepting malformed or truncated data as PNG.
85b3aa8 to
5ade264
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85b3aa8a59
ℹ️ 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 shouldLoop = split.calls.length > 0 && !split.hasRealToolCall && !forceFinal; | ||
| if (!shouldLoop) { | ||
| yield* replay(split.passthrough); | ||
| return; |
There was a problem hiding this comment.
Preserve mixed image and client tool calls
When a model emits an image call and a normal client tool call in the same response, scanEventsForImageCall has already removed the image events, but hasRealToolCall makes shouldLoop false, so replay exposes only the normal tool and the image is never fulfilled or reported as failed. This silently loses part of a valid parallel tool batch and can also leave stateful adapters such as Cursor waiting for the hidden call's result; preserve or fulfill the image call before returning the client calls and add a mixed-call regression test.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
| const events: AdapterEvent[] = []; | ||
| try { | ||
| const parse = prepared.responseAdapter.parseStream.bind(prepared.responseAdapter); | ||
| for await (const event of parseStreamWithProgress(prepared.response, parse, { | ||
| signal, | ||
| inactivityTimeoutMs: STALL_TIMEOUT_MS, | ||
| })) { | ||
| if (event.type === "heartbeat") yield event; | ||
| else events.push(event); |
There was a problem hiding this comment.
For every routed request whose catalog enables the image bridge—even when the model never invokes image generation—all semantic events are accumulated until a terminal event arrives. A faulty or adversarial provider can continuously emit deltas, thereby avoiding the inactivity timeout while growing this array without limit and eventually exhausting the Bun process; enforce an event/byte/output cap and cover the over-limit stream with a focused regression test.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
| export function guessExtFromMagic(bytes: Uint8Array): string { | ||
| const sig = Buffer.from(bytes.slice(0, 12)).toString("latin1"); | ||
| if (sig.startsWith("\x89PNG\r\n\x1a\n")) return "png"; | ||
| if (sig.startsWith("\xff\xd8\xff")) return "jpg"; | ||
| if (sig.startsWith("RIFF") && sig.slice(8, 12) === "WEBP") return "webp"; | ||
| if (sig.startsWith("GIF8")) return "gif"; | ||
| return "png"; |
There was a problem hiding this comment.
Reject payloads without a recognized image signature
When xAI or its CDN returns an empty body, HTML error page, or otherwise malformed bytes with a successful status, this fallback labels the payload as PNG; both materialization paths then write it and fulfillImageCall reports successful generation with a broken artifact path. Return an error for empty or unrecognized payloads instead of defaulting to PNG, with focused coverage for malformed inline and downloaded data.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
| export function pruneOldArtifacts(dir: string, maxFiles: number): void { | ||
| // A non-positive maxFiles disables pruning entirely (do not delete everything). | ||
| if (maxFiles <= 0) return; |
There was a problem hiding this comment.
Validate the artifact retention count
When an operator sets images.artifactsKeepCount to 0 or a negative value, the unvalidated config reaches this branch and disables pruning entirely, even though the option is documented as the maximum number of retained files. A value intended to retain no artifacts, or a simple bad hand edit, therefore removes the only disk-growth bound; require a positive integer or explicitly document and separate the unlimited-retention mode.
AGENTS.md reference: AGENTS.md:L96-L97
Useful? React with 👍 / 👎.
| path: primary, | ||
| files, | ||
| count: files.length, | ||
| markdown: ``, |
There was a problem hiding this comment.
Encode artifact paths before building Markdown
When OPENCODEX_HOME contains spaces, parentheses, or #, or on Windows where primary contains a drive prefix and backslashes, interpolating the filesystem path directly creates an invalid or misinterpreted Markdown image target. The generated file exists but models that return this supplied markdown field give users a broken image; preserve the raw path field while constructing the Markdown target from a properly escaped portable URI and cover Windows and reserved-character paths.
AGENTS.md reference: AGENTS.md:L36-L37
Useful? React with 👍 / 👎.
- 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
Replace lidge-jun#355's standalone artifacts.ts with lidge-jun#424's fuller version (adds downloadImageToArtifact, SSRF protection, HTTPS enforcement). Also bring in destination-policy.ts exports needed by the module. Removes lidge-jun#355's duplicate guessExtFromMagic — lidge-jun#424's version is the canonical one.
- pruneOldArtifacts() caps artifacts/ to 200 files, deleting oldest by mtime - Runs after each successful write in materializeInlineImage + downloadImageToArtifact - Best-effort: errors caught and warned, never fails the write
- 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
Replace lidge-jun#355's standalone artifacts.ts with lidge-jun#424's fuller version (adds downloadImageToArtifact, SSRF protection, HTTPS enforcement). Also bring in destination-policy.ts exports needed by the module. Removes lidge-jun#355's duplicate guessExtFromMagic — lidge-jun#424's version is the canonical one.
- pruneOldArtifacts() caps artifacts/ to 200 files, deleting oldest by mtime - Runs after each successful write in materializeInlineImage + downloadImageToArtifact - Best-effort: errors caught and warned, never fails the write
|
Maintainer takeover: landing vehicle is now #577 (Wibias:maintainer-takeover/424-feat-image-bridge). It rebases this work onto latest Huge thanks to @tizerluo — you will be credited for the original feature work when this lands. |
|
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 |
Summary
When Codex routes to a non-OpenAI model (Claude, Gemini, Grok, etc.), the
image_generationhosted tool stops working because it requires OpenAI's server-side execution. This PR adds an Image Bridge that:image_generationin the request's tools arrayimage_gen) the routed model can call/images/generations)~/.opencodex/artifacts/with magic-byte format detectionThe architecture mirrors the proven
src/web-search/sidecar pattern — same async-generator loop,bridgeToResponsesSSE, heartbeat, and abort-signal linkage.New Files
src/images/types.tsImageBridgePlan,ImageCallResult)src/images/synthetic-tool.tssrc/images/xai-client.ts/images/generationsand/images/editsclientsrc/images/fulfill.tssrc/images/plan.tssrc/images/loop.tssrc/images/index.tstests/images/*.test.tsdocs-site/.../image-bridge.mdModified Files
src/types.ts—_imageGenerationstash field,OcxTool.imageGenerationflag,OcxImagesConfigbridge fieldssrc/responses/parser.ts—extractHostedImageGeneration()+ stash into parsed resultsrc/server/responses/core.ts— image bridge trigger (after web-search block, before standard path)src/images/artifacts.ts— extended withdownloadImageToArtifact()+guessExtFromMagic()(shared helpers extracted)Verification
bun x tsc --noEmitpasses (0 errors)bun test tests/images/passes (53 tests, 0 failures)bun run privacy:scanpassesChecklist
Summary by CodeRabbit
image_generationon non-OpenAI providers using an interception/synthetic tool and bounded multi-round tool-call looping.