fix(responses): execute code-mode view_image through unified exec - #4475
Conversation
…ip ci] A routed provider that echoes the nested helper name emitted `view_image` instead of the declared code-mode `exec`, and the undeclared-tool guard failed the turn. `view_image` now joins the helper names the guard admits behind a bare `exec` declaration, and the compiler turns the call into `await tools.view_image(...)`, surfacing the returned `image_url` through `image()` and falling back to `text()` when the host returns no image. The `default.view_image` spelling some providers invent is stripped to the bare helper first. Explicit `path` wins over the `file_path`, `file` and `image_path` aliases, in that order. Carries #4455 by jeongjin0 and the overlapping four-file subset in #4171 by rrmlima, both answering #4412. Folds in the review findings recorded on #4171: the compat path executes the helper rather than emitting a text-only stub, it asserts nothing about `view_image` being unavailable in code mode, and the composition cases the review named are covered end to end — a namespaced `view_image` keeps its full wire name, a flat-bridge catalog that declares `exec` beside a bare `exec_command` is never rewritten, and malformed arguments still reach nested validation as data. Co-authored-by: Jeongjin Shin <80797980+jeongjin0@users.noreply.github.com> Co-authored-by: rrmlima <137737127+rrmlima@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change adds ChangesCode-mode
Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant RoutedProvider
participant ResponsesGuard
participant normalizeDeclaredToolName
participant compileCodeModeHelperInput
participant CodeModeExec
RoutedProvider->>ResponsesGuard: Emit default.view_image
ResponsesGuard->>normalizeDeclaredToolName: Check declared tool catalog
normalizeDeclaredToolName-->>ResponsesGuard: Resolve to exec when code mode is declared
ResponsesGuard->>compileCodeModeHelperInput: Compile view_image arguments
compileCodeModeHelperInput->>CodeModeExec: Run tools.view_image(...)
CodeModeExec-->>compileCodeModeHelperInput: Return image_url
compileCodeModeHelperInput-->>RoutedProvider: Emit image(image_url)
Possibly related PRs
Merge Risk: ⚪ Minimal · up to The routed view_image compatibility paths and guard behavior are covered without a concrete unresolved merge risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation Most changes support Full details: Docstring CoverageExplanation Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 7 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 9d8e46c257
ℹ️ 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".
| && declared.has(CODE_MODE_EXEC_TOOL_NAME) | ||
| && (CODE_MODE_HELPER_TOOL_NAMES as readonly string[]).includes(bare) |
There was a problem hiding this comment.
When the catalog declares exec alongside exec_command or shell_command (the flat-bridge shape), this new branch strips default.apply_patch because apply_patch is in the helper list. The subsequent unconditional candidate === "apply_patch" check then rewrites it to exec before the flat-bridge guard runs, so an undeclared provider call is authorized and converted into JavaScript for an ordinary exec tool. Gate this prefix branch with declaresCodeModeExec, or perform the flat-bridge refusal before the apply_patch rewrite.
Useful? React with 👍 / 👎.
| if (helperName === "write_stdin") { | ||
| return `const result = await tools.write_stdin(${JSON.stringify(args)});\ntext(result);`; | ||
| } | ||
| if (helperName === "view_image") { |
There was a problem hiding this comment.
The commit message explicitly says this carries #4455 by @jeongjin0 and part of #4171 by @rrmlima, but the commit contains no Co-authored-by trailers despite claiming that both authors were credited that way. The prose attribution does not register either contributor; add proper trailers to the PR description or a follow-up branch commit so they survive the squash.
AGENTS.md reference: AGENTS.md:L288-L292
Useful? React with 👍 / 👎.
리뷰 · 우선순위 72 / 80설명
우선순위 72는 실제 502 사용자 경로를 막고, helper 허용 규칙을 기존 exec 계약 안에 넣으며, tip과 겹침이 적기 때문이다. types/config 분할 캠페인과는 무관하다.
심볼 원본 #4455 / #4412(default. 접두) - 캐리가 원본+접두 수정까지 묶었는지 머지 전 한 번 확인. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Lane R of the contributor carry train, the serialized responses/core lane: code-mode view_image through unified exec (#4455 by jeongjin0, also carrying the duplicate #4171 by rrmlima), routed effort ladders from models.dev with a refused-rung replay (#4409 by yxr1995-maker), and web-search continuations bound to the serving API key (#4387 by luvs01). Cross-platform CI run 34746891233 concluded success on 2c28886, the exact head merged here, and it covers every link because the lane is cumulative. #4475 and #4488 carry no ci check of their own; their head commits carry [skip ci] by design, under the owner-authorized tip-only CI economy for this batch. The fourth planned link, #4086 by Eleven-is-cool, is not here because it is already on dev as d6723f7 with its own Co-authored-by trailer. The lane attempted the carry first and found a modify/delete conflict on structure/04_transports-and-sidecars.md, which the #4276 SSOT restructure had removed; the landed version is a superset of the branch. All four source authors are credited by Co-authored-by trailers in the landed commits.
1 participant
Summary
view_imageinstead of the declaredexec, and the undeclared-tool guard failed the whole turn with a 502.view_imagenow joins the helper names the guard admits behind a bareexecdeclaration.await tools.view_image(...)and surfaces the returnedimage_urlthroughimage(), falling back totext()when the host returns no image. It executes the helper rather than acknowledging the call with a text-only stub.default.view_imagespelling some providers invent is stripped to the bare helper first, matching the existingdefault.handling for the other helpers.path: an explicitpathwins, thenfile_path,fileandimage_pathin that order. Anything else is passed through as data so nested validation can reject it.Co-authored-bytrailers on the branch commit.Closes #4412
Review findings folded in
#4171 carries a
CHANGES_REQUESTEDreview. Its findings are addressed here rather than carried forward:tools.view_imageand returns the image, instead of accepting an undeclared call and emitting onlytext().view_imageis universally unavailable in code mode, and no host capability is invented.view_image, a namespacedview_image, a flat-bridge catalog that declaresexecbeside a bareexec_command, and malformed arguments.dev(d0cbfffdd), not the stale merge base the review flagged.Verification
bun test tests/adapters/bridge-legacy-shell-normalization.test.ts tests/responses/legacy-shell-compat.test.ts tests/responses/responses-undeclared-tool-guard.test.ts— 123 pass, 0 fail.bun run typecheck— clean.bun run structure:check— passed.structure/transports/responses.mdsits exactly at its 600-line budget ondev, so the paragraph documenting this seam was rewrapped in place to stay net-neutral rather than pushing the doc over.bun run privacy:scan— passed.[skip ci].Checklist
The guard question is the security-relevant one here, and it is unchanged in shape:
view_imageis admitted only when the request catalog declares a bareexec. A namespacedexecdeclaration still authorizes nothing, and a catalog that declaresview_imageor a legacy shell bridge name itself is never rewritten.Summary by CodeRabbit
New Features
view_imagetool in code-mode execution.Bug Fixes
Documentation
Tests