fix(vision): sync captions into Responses passthrough - #1047
Conversation
📝 WalkthroughWalkthroughThe vision pipeline now synchronizes rewritten image content with native Responses request bodies. End-to-end tests cover captions for standard and tool-returned images, cache isolation, and placeholder fallback behavior. ChangesResponses vision synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f46fdca55
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/vision/index.ts`:
- Around line 308-311: Update the input-image normalization logic in the visible
transformation function so empty image_url values are omitted or replaced with
the existing omission marker instead of returning the original input_image part;
apply the same behavior when descriptions.length is zero. Add an end-to-end
assertion verifying the forwarded input contains no input_image parts for this
case, and flag the provider/adapter contract drift under src/**.
🪄 Autofix
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: 72f00de6-a5b6-42df-a4e9-c97936338e89
📒 Files selected for processing (2)
src/vision/index.tstests/vision-sidecar-e2e.test.ts
| if (value.type === "input_image" && typeof value.image_url === "string") { | ||
| if (nonEmptyImageUrlsOnly && value.image_url.length === 0) return value; | ||
| const description = descriptions[nextDescription++]; | ||
| return description === undefined ? value : { type: "input_text", text: description }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove empty input_image parts from _rawBody.
On Line 309, an empty image_url returns the original input_image part. A text-only Responses provider can still reject this unsupported part.
Remove the empty part or replace it with the omission marker. Apply the same result when descriptions.length === 0. Add an end-to-end assertion that forwarded input contains no input_image parts for this case.
As per path instructions, flag provider/adapter contract drift in src/**.
🤖 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/vision/index.ts` around lines 308 - 311, Update the input-image
normalization logic in the visible transformation function so empty image_url
values are omitted or replaced with the existing omission marker instead of
returning the original input_image part; apply the same behavior when
descriptions.length is zero. Add an end-to-end assertion verifying the forwarded
input contains no input_image parts for this case, and flag the provider/adapter
contract drift under src/**.
Source: Path instructions
…edger A fourth audit round found #812 -> #572 and #1047/#1002 -> #1024 still sitting in the literal-extraction table. Those three PR bodies contain no issue reference at all, so the table was attributing claims the authors never made. The associations are still probably correct - #812 does add an Apertis preset, #1047 and #1002 do edit the vision sidecar - but a ledger that mixes what was written with what was inferred cannot be acted on without re-deriving it, which defeats the point of writing it down.
|
Reviewed in a sweep of the The vision sidecar rewrites The test earns its place: it asserts against the recorded upstream body over actual HTTP passthrough, and covers tool-output images and the no-sidecar stripping case. It would fail before the fix, because the recorded body still contains the original image. Non-image raw items are preserved, which is the part I checked hardest — a Your remaining blocker is verification, not design. The PR is a draft, the head is 341 commits behind Worth flagging the neighbourhood: #1002 makes sidecar reasoning configurable and touches adjacent code. They are not in conflict as written, but whichever lands second should re-run the other's tests rather than assuming. |
2 participants
Summary
Root cause
The vision fallback rewrote the parsed
context.messages, but the OpenAI Responses passthrough adapter serializes_rawBody. Text-only Responses providers therefore still received the originalinput_imageeven after the sidecar produced a caption.Impact
Text-only models using the Responses wire now receive the vision caption instead of an unsupported raw image. Native vision-capable models remain unchanged.
Validation
bun run prepushSummary by CodeRabbit
Bug Fixes
Tests