fix(openai-responses): round-trip image-gen namespace - #588
Conversation
Lower Codex's private image_gen namespace to safe API-key aliases, restore function calls in JSON and SSE responses, and encode replayed history without changing ChatGPT forward mode.
📝 WalkthroughWalkthroughAPI-key Responses image-generation tools now use upstream-safe aliases, while passthrough JSON and SSE responses restore Codex’s ChangesImage-generation Responses bridge
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Codex
participant OpenAIResponsesAdapter
participant ResponsesProvider
participant ResponsesPassthrough
Codex->>OpenAIResponsesAdapter: image_gen tool declaration
OpenAIResponsesAdapter->>ResponsesProvider: image_gen__imagegen alias
ResponsesProvider-->>ResponsesPassthrough: aliased function-call response
ResponsesPassthrough-->>Codex: restored image_gen namespace and name
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/codex-integration.md`:
- Around line 66-72: Update the translated codex-integration pages for ja, ko,
ru, and zh-cn to reflect the English paragraph’s API-key Responses behavior:
image_gen aliasing, removal of duplicate hosted image_generation, response
namespace restoration, and replay encoding. If a translation cannot be updated,
explicitly mark its corresponding content as stale rather than leaving it
contradictory; preserve the unchanged ChatGPT forward-mode behavior.
- Around line 67-69: Update the documentation wording around the Codex namespace
adapter to describe the generic image_gen__<inner-name> alias pattern instead of
hardcoding image_gen__imagegen, matching the adapter and related documentation.
Simplify “maps the returned function call back to” by removing the redundant
“returned” while preserving the explicit image_gen namespace behavior.
In `@src/adapters/openai-responses.ts`:
- Around line 631-664: Update src/adapters/openai-responses.ts lines 631-664 so
hosted image_generation removal depends on successful lowering by
flattenImageGenNamespace or an existing usable flat alias, not the broad
hasImageGenClientTool gate; preserve the hosted tool when no callable alias is
produced. Update tests/openai-responses-passthrough.test.ts lines 881-888 to
expect the hosted tool for an empty or malformed namespace, and revise
structure/04_transports-and-sidecars.md lines 67-68 to document removal only
when a usable image_gen__<inner-name> alias replaces it.
In `@src/server/responses-image-gen-repair.ts`:
- Around line 76-112: Extract the duplicated SSE parsing and ReadableStream
relay logic from nextSseBlock, sseDataPayload, replaceSseDataPayload, and the
corresponding relaySseWithResponsesItemIdRepair flow into a shared
relaySseWithPayloadRewrite utility. Have each repair module provide only its
payload rewrite callback, then update responses/core.ts to compose both rewrites
in one relay so each SSE event is parsed, transformed, and encoded only once.
In `@src/server/responses/core.ts`:
- Around line 1552-1554: Make the win32 eager-relay invariant explicit around
needsClientRewrite, winNoClientRewrite, and the eager branch: since
winNoClientRewrite guarantees no image-generation aliases, do not invoke
relaySseWithImageGenCallRestore on this path, or otherwise clearly document and
enforce that the eager body is passed through unchanged. Preserve client
rewriting for paths where aliases are present and avoid introducing a JS pull
wrapper on win32.
In `@structure/04_transports-and-sidecars.md`:
- Around line 63-70: Update the gate description near the “win32-no-repair only”
text to use the renamed needsClientRewrite predicate and describe the condition
as win32 with no client-facing rewrite, including neither image-gen aliases nor
item-id repair. Keep this document synchronized with the corresponding core.ts
and src/server/index.ts comments.
In `@tests/responses-image-gen-repair.test.ts`:
- Line 191: The test name in the “handleResponses restores client-facing SSE
calls on the Windows rewrite path” test overstates coverage because it does not
force or verify the win32 branch. Either rename the test to describe the
platform-neutral passthrough rewrite behavior, or explicitly mock/set
process.platform to win32 and assert the branch controlled by handleResponses
executes.
🪄 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: e3da1fb9-7174-4f11-bb14-19a307051177
📒 Files selected for processing (10)
docs-site/src/content/docs/guides/codex-integration.mdsrc/adapters/openai-responses.tssrc/server/index.tssrc/server/responses-image-gen-repair.tssrc/server/responses/core.tsstructure/04_transports-and-sidecars.mdtests/azure-adapter.test.tstests/openai-responses-passthrough.test.tstests/passthrough-abort.test.tstests/responses-image-gen-repair.test.ts
| function nextSseBlock(buffer: string): { block: string; delimiter: string; rest: string } | null { | ||
| const match = buffer.match(/\r?\n\r?\n/); | ||
| if (!match || match.index === undefined) return null; | ||
| return { | ||
| block: buffer.slice(0, match.index), | ||
| delimiter: match[0], | ||
| rest: buffer.slice(match.index + match[0].length), | ||
| }; | ||
| } | ||
|
|
||
| function sseDataPayload(block: string): string | null { | ||
| const data: string[] = []; | ||
| for (const line of block.split(/\r?\n/)) { | ||
| if (!line.startsWith("data:")) continue; | ||
| const value = line.slice(5); | ||
| data.push(value.startsWith(" ") ? value.slice(1) : value); | ||
| } | ||
| return data.length > 0 ? data.join("\n") : null; | ||
| } | ||
|
|
||
| function replaceSseDataPayload(block: string, payload: string): string { | ||
| const newline = block.includes("\r\n") ? "\r\n" : "\n"; | ||
| const lines = block.split(/\r?\n/); | ||
| const rewritten: string[] = []; | ||
| let replaced = false; | ||
| for (const line of lines) { | ||
| if (!line.startsWith("data:")) { | ||
| rewritten.push(line); | ||
| continue; | ||
| } | ||
| if (!replaced) { | ||
| rewritten.push(`data: ${payload}`); | ||
| replaced = true; | ||
| } | ||
| } | ||
| return replaced ? rewritten.join(newline) : block; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
nextSseBlock, sseDataPayload, and replaceSseDataPayload are verbatim duplicates of src/server/responses-item-id-repair.ts.
The three parsers here plus the ReadableStream shell at Lines 118-169 are byte-for-byte the same shape as relaySseWithResponsesItemIdRepair (src/server/responses-item-id-repair.ts Lines 224-275). Beyond the duplication, src/server/responses/core.ts Lines 1665-1668 now chains both wrappers on the same stream, so every SSE block is split, JSON.parsed, re-stringifyd, and re-encoded twice per event.
Suggested extraction: a shared relaySseWithPayloadRewrite(body, rewrite: (payload: string) => string) in a small sse-rewrite.ts, with both modules supplying only their rewrite callback. core.ts could then compose the two rewrites into a single pass.
🤖 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/server/responses-image-gen-repair.ts` around lines 76 - 112, Extract the
duplicated SSE parsing and ReadableStream relay logic from nextSseBlock,
sseDataPayload, replaceSseDataPayload, and the corresponding
relaySseWithResponsesItemIdRepair flow into a shared relaySseWithPayloadRewrite
utility. Have each repair module provide only its payload rewrite callback, then
update responses/core.ts to compose both rewrites in one relay so each SSE event
is parsed, transformed, and encoded only once.
There was a problem hiding this comment.
Confirmed: the SSE framing/relay shell is duplicated, and chaining both opt-in rewrites performs two passes. I am intentionally deferring that extraction from this correctness-focused PR because a true single-pass composition would also restructure the existing provider-local item-id repair path and broaden the regression surface. This thread is left unresolved for maintainer direction or a focused follow-up.
There was a problem hiding this comment.
@RailyW, understood. The duplication and double SSE transformation remain valid follow-up work, but deferring the shared relay extraction is reasonable for this correctness-focused change: consolidating src/server/responses-image-gen-repair.ts with the provider-local item-ID repair path in src/server/responses-item-id-repair.ts would alter a broader, stream-sensitive code path.
I’ll leave this finding unresolved for maintainer direction. Would you like me to open a dedicated GitHub issue for the single-pass SSE payload-rewrite utility, with #588 and this discussion linked as context?
Remove hosted image_generation only when a usable client alias replaces it, while retaining the hosted tool for malformed namespaces and replay-only calls.
Build the reverse map from explicitly declared dotted image-gen functions so safe upstream aliases return to Codex with an explicit namespace without guessing unrelated double-underscore names.
Keep the win32 eager branch free of client rewrite wrappers and align the source invariant and transport documentation with the no-client-rewrite gate.
Describe the integration test as the platform-neutral passthrough rewrite path while the separate source invariant continues to guard the win32 native relay branch.
Document the generic image_gen__<inner-name> contract and clarify that hosted image generation is removed only after a usable client alias replaces it.
Update the Japanese, Korean, Russian, and Simplified Chinese Codex integration guides with the API-key alias, response restoration, replay, and forward-mode compatibility contract.
Document the request-normalization and response-restoration helper contracts without changing the deferred SSE relay structure.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad020ca329
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/server/responses-image-gen-repair.ts`:
- Around line 13-26: Export declaredToolGroups from
src/server/responses-image-gen-repair.ts (lines 13-26) as the canonical
tool-group collector, then update src/adapters/openai-responses.ts (lines
644-652) to import and reuse it instead of duplicating the collection loop;
preserve the existing ordering and handling of body.tools and
input[].additional_tools.
🪄 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: db86ad9d-c85f-4da4-9ead-313fc1294896
📒 Files selected for processing (13)
docs-site/src/content/docs/guides/codex-integration.mddocs-site/src/content/docs/ja/guides/codex-integration.mddocs-site/src/content/docs/ko/guides/codex-integration.mddocs-site/src/content/docs/ru/guides/codex-integration.mddocs-site/src/content/docs/zh-cn/guides/codex-integration.mdsrc/adapters/openai-responses.tssrc/server/index.tssrc/server/responses-image-gen-repair.tssrc/server/responses/core.tsstructure/04_transports-and-sidecars.mdtests/openai-responses-passthrough.test.tstests/passthrough-abort.test.tstests/responses-image-gen-repair.test.ts
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Maintainer review verdict — approve to mergeReviewed head SecurityNo medium/high/critical issues. Alias maps are built only from the caller's declared CorrectnessNo blocking bugs found. Focused suite passed locally (70/70): CI already green on ubuntu/windows/macos + npm-global. Open CodeRabbit / Codex feedback
VerdictMerge. Optional follow-up issue for single-pass SSE payload rewrite composition; nothing else outstanding from Rabbit/Codex that should block. |
|
Thanks @RailyW — solid correctness work on the image-gen namespace round-trip, and appreciated the thorough follow-through on the review threads (hosted-tool gate, tool_choice, locales, win32 invariant). Squash-merging to |
Wibias
left a comment
There was a problem hiding this comment.
Maintainer approval after bug+security review. Mergeable; deferred SSE single-pass extraction is follow-up only.
|
Follow-up for the deferred single-pass SSE rewrite composition: https://github.com/lidge-jun/opencodex/pull/ |
Follow-up to #588: extract shared sse-payload-rewrite shell and compose image-gen restore with item-id repair in one parse/stringify pass instead of chaining two JS pull wrappers.
Summary
This is a follow-up to #492. That change prevents one hosted-tool collision by removing a duplicate
image_generationdeclaration, but API-key Responses providers can still reject Codex's privateimage_gennamespace itself. Replacing the namespace with a dotted function name is not sufficient either: public-compatible upstreams may enforce the flat^[a-zA-Z0-9_-]+$function-name pattern, while Codex dispatches namespaced tools through an explicitnamespacefield and does not reconstruct it by splitting a returned name. An outbound-only rewrite can therefore make chat succeed while the resulting image call fails as an unsupported client tool.Request-side normalization
openai-responsesproviders, including Azure's inherited keyed path. ChatGPTforwardmode keeps its native Responses Lite payload unchanged.image_gennamespace into public-compatibleimage_gen__<inner-name>function declarations while preserving the inner function schema, description, and strictness.body.toolsand Codex Desktop's nestedinput[].type = "additional_tools"containers, because an upstream validates their combined tool namespace.image_generationdeclaration only when a usable client alias replaces it. Malformed namespaces and replay-only calls keep the hosted fallback; equivalent usable aliases are deduplicated in stable container order.{ namespace: "image_gen", name: "<inner-name>" }calls, plus legacy dottedimage_gen.<inner-name>calls, back to the same safe wire alias before they return upstream.allowed_toolschoices to the same alias only when the matching callable declaration was successfully lowered, preventing declaration/selector mismatches without rewriting unrelated choices.Response-side restoration
image_gen__<inner-name>to{ namespace: "image_gen", name: "<inner-name>" }before Codex receives them. The SSE path covers incremental output-item events and the completed response snapshot, including frames split across transport chunks.previous_response_idhistory can be encoded and replayed without leaking the private namespace shape upstream.[DONE], and the existing Windows native-relay optimization whenever no client rewrite is required.Compatibility boundaries
image_generationkeeps that hosted tool./v1/images/generationsand/v1/images/editsrelay paths are unchanged.Verification
bun test tests/openai-responses-passthrough.test.ts tests/responses-image-gen-repair.test.ts tests/responses-tool-groups.test.ts tests/azure-adapter.test.ts tests/passthrough-abort.test.ts(70 passed, 0 failed, 201 assertions)bun node_modules/typescript/bin/tsc --noEmitbun scripts/privacy-scan.tsgit diff origin/dev...HEAD --checkcd docs-site && bun install --frozen-lockfile && bun run build(141 pages built)Checklist
Summary by CodeRabbit
image_gencompatibility for API-key Responses integrations via safe aliasing, normalization, and replay-safe function-call mapping (including legacy dotted formats).image_gen/hostedimage_generationconflict handling and ensured consistent naming restoration during passthrough relays.