Skip to content

fix(responses): execute code-mode view_image through unified exec - #4455

Closed
jeongjin0 wants to merge 1 commit into
lidge-jun:devfrom
jeongjin0:codex/code-mode-view-image-helper
Closed

fix(responses): execute code-mode view_image through unified exec#4455
jeongjin0 wants to merge 1 commit into
lidge-jun:devfrom
jeongjin0:codex/code-mode-view-image-helper

Conversation

@jeongjin0

@jeongjin0 jeongjin0 commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Compile routed view_image and synthetic default.view_image calls through the bare code-mode exec tool, invoking tools.view_image(...) and surfacing image(result.image_url) instead of terminating the turn.
  • Preserve directly declared and namespaced tools, normalize the documented path aliases as data, and keep catalogs without bare exec fail-closed.
  • Update the Responses transport contract and add streaming, non-streaming, malformed-input, alias, and namespace regression coverage.

Closes #4412

This supersedes #4171 with the supported nested helper behavior.

Co-authored-by: Rafael Moreira rrmlima@gmail.com

Verification

  • bun test tests/responses/legacy-shell-compat.test.ts tests/adapters/bridge-legacy-shell-normalization.test.ts tests/responses/responses-undeclared-tool-guard.test.ts (121 pass)
  • bun run typecheck
  • bun run structure:check
  • bun run privacy:scan
  • git diff --check origin/dev...HEAD
  • A broader changed-suite run completed 20,789 tests successfully but reported 22 failures in unrelated Kiro, TOML, storage, audio, and provider-option tests; this draft remains open for repository CI.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: e11556ce-ca33-4e35-b3eb-d6c21ed6a7d7

📥 Commits

Reviewing files that changed from the base of the PR and between b41c63f and b09c5c2.

📒 Files selected for processing (3)
  • structure/transports/responses.md
  • tests/responses/legacy-shell-compat.test.ts
  • tests/responses/responses-undeclared-tool-guard.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The change adds view_image to code-mode helper normalization and compilation. Bare and default.view_image calls route through declared exec, invoke nested tools.view_image, and emit image or text output. Tests cover routing, aliases, malformed input, and fail-closed behavior.

Changes

Code-mode view_image compatibility

Layer / File(s) Summary
Helper-name normalization
src/types/tools.ts, src/server/responses-undeclared-tool-guard.ts, src/server/responses/core.ts
view_image is recognized as a code-mode helper. Synthetic default.view_image names normalize to exec when appropriate. Guard comments document the helper.
view_image compilation
src/responses/code-mode-helper-compat.ts, structure/transports/responses.md
Bare and default.-prefixed helper names compile to tools.view_image. file_path, file, and image_path map to path. Results with image_url use image(); other results use text().
Compatibility validation
tests/responses/legacy-shell-compat.test.ts, tests/adapters/bridge-legacy-shell-normalization.test.ts, tests/responses/responses-undeclared-tool-guard.test.ts
Tests cover routing, path aliases, output handling, malformed input, streaming and non-streaming calls, declaration requirements, and namespaced-tool refusal.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant Provider
  participant UndeclaredToolGuard
  participant ExecCompiler
  participant CodeModeTools
  Provider->>UndeclaredToolGuard: Emit view_image or default.view_image
  UndeclaredToolGuard->>ExecCompiler: Route through declared exec
  ExecCompiler->>CodeModeTools: Call tools.view_image with path
  CodeModeTools-->>ExecCompiler: Return image_url or host result
  ExecCompiler-->>Provider: Emit image(image_url) or text(result)
Loading

Suggested reviewers: lidge-jun

Merge Risk: ⚪ Minimal · up to b09c5

The requested view-image compatibility behavior is implemented with coverage for supported and rejected declaration paths. No actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #4412 coding requirements are met. In src/types/tools.ts, CODE_MODE_HELPER_TOOL_NAMES includes view_image, and normalizeDeclaredToolName strips an unclaimed default. prefix before it m…
Out of Scope Changes check ✅ Passed The changes stay within Issue #4412. Source changes in src/types/tools.ts and src/responses/code-mode-helper-compat.ts implement view_image code-mode normalization and compilation. Comment updat…
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: routing code-mode view_image calls through the unified exec tool.
Full details: Docstring Coverage

Explanation

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.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 13, 2026
@github-actions

github-actions Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (2/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 2/4).

Review readiness checklist

  • ⬜ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

2/4 boxes ticked.

This PR stays in draft until every box above is ticked.

@jeongjin0

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 70 / 80

설명

이 PR은 code-mode에서 모델이 view_image(또는 default.view_image)를 선언된 exec의 중첩 헬퍼로 냈을 때, undeclared로 턴을 끊지 않고 tools.view_image(...)image(result.image_url)로 실행하게 만듭니다. 이슈 #4412를 닫고, 예전 #4171지원되는 nested-helper 동작으로 대체한다고 명시했습니다. 지금 dev(HEAD 261bab915)의 src/types/tools.tsCODE_MODE_HELPER_TOOL_NAMESexec_command/shell_command/write_stdin/apply_patch만 있고 view_image는 없습니다. src/responses/code-mode-helper-compat.tscompileCodeModeHelperInputview_image 분기가 없어, 그 이름은 마지막 exec_command 경로로 떨어지거나 가드에 걸립니다. 보고된 “code-mode exec catalog rejects routed default.view_image”와 코드가 맞습니다.

변경은 네 겹입니다. (1) helper 목록에 view_image 추가, (2) default. 접두가 붙은 helper를 bare로 벗겨 exec로 정규화(#4412), (3) compile 경로에서 file_path/file/image_pathpath 별칭 정규화 후 tools.view_image 호출 스크립트 생성, (4) undeclared-tool 가드/core.ts 주석에 view_image를 exec nested 집합에 포함. 카탈로그에 bare exec가 없으면 여전히 fail-closed입니다. MCP가 스스로 view_image를 선언한 경우는 기존 규칙대로 helper 재작성 대상이 아닙니다. 테스트는 streaming/non-streaming, malformed, alias, namespace 회귀를 legacy-shell-compat·bridge-legacy-shell-normalization·responses-undeclared-tool-guard에 넣었고, structure/transports/responses.md도 한 줄 갱신했습니다.

dev 방향과도 잘 맞습니다. Responses code-mode 다리와 undeclared fail-closed를 유지한 채, Codex가 실제로 노출하는 nested helper만 허용하는 기존 패턴의 연장입니다. draft이고 readiness 체크리스트가 아직 비어 있으며, 작성자 말로는 넓은 suite에서 무관한 22실패가 있어 repository CI를 기다려야 합니다. #4171은 아직 OPEN이므로, 이 PR이 합쳐지면 원본은 landed/superseded로 닫는 게 맞습니다.

경로 src/responses/code-mode-helper-compat.ts view_image 분기 - path가 없고 별칭만 있으면 복사 후 별칭을 지웁니다. 별칭이 여러 개면 먼저 나온 키가 이깁니다. 의도된 데이터 정규화이지만, 동시에 file_pathimage_path가 다르면 조용히 한쪽만 남습니다. 테스트에 충돌 케이스가 있으면 더 안전합니다.

경로 src/types/tools.ts default. helper strip - declared.has(exec)이고 bare가 helper 목록에 있을 때만 default.view_imageview_image → 이후 exec로 갑니다. 논리는 #4176과 같으나, helper 목록이 늘어날수록 “default. 위조” 공격면이 exec 카탈로그에 묶여 있음을 주석/테스트로 계속 고정해야 합니다.

경로 src/server/responses-undeclared-tool-guard.ts / core.ts - bare exec alias를 MCP exec에 열어주지 않는 기존 가드를 유지한 채 주석만 view_image를 포함했습니다. 동작 변경이 주석과 일치하는지, addWireToolName이 exec에 대해 bare alias를 계속 막는지 회귀가 이미 있습니다. 좋습니다.

PR 본문 Verification - focused 120 pass는 설득력 있지만, 작성자가 기록한 무관 22실패 + draft 체크리스트 미완은 merge 전에 exact-head CI로 덮어써야 합니다.

메인테이너의 판단이 필요한 지점

  • exact-head CI가 초록일 때만 합칠지(MAINTAINERS.md 기본)
  • #4171을 이 PR merge 직후 Landed via #4455로 닫을지, 지금 duplicate로 미리 닫을지
  • view_image 별칭 충돌 시 첫 키 우선 vs 명시적 400
  • image() 없이 text(result)만 나오는 실패를 모델이 재시도하기 충분한지

너의 추천
CI가 초록이면 dev에 합치세요. #4412의 올바른 모양이고 #4171보다 nested-helper 계약에 가깝습니다. 합친 뒤 #4171#4412를 landed/completed로 정리하세요. types/config 분할 무관, close-don't-rebase 대상 아님. 지금은 draft이니 ready 표시 전에 체크리스트를 채우세요.

이 댓글은 grok-bot이 작성했습니다

@jeongjin0
jeongjin0 force-pushed the codex/code-mode-view-image-helper branch from b41c63f to b09c5c2 Compare September 13, 2026 04:38
@jeongjin0

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Restore routed view_image calls through the declared code-mode exec tool and surface the host image result while keeping undeclared and namespaced catalogs fail-closed.

Co-authored-by: Rafael Moreira <rrmlima@gmail.com>
@jeongjin0
jeongjin0 force-pushed the codex/code-mode-view-image-helper branch from b09c5c2 to 61b9aef Compare September 13, 2026 04:49
@jeongjin0

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

lidge-jun added a commit that referenced this pull request Sep 13, 2026
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.
@lidge-jun

Copy link
Copy Markdown
Owner

Closing as landed: executing code-mode view_image through unified exec is on dev via #4475, merged inside lane R's cumulative tip #4489 (merge commit 3f76ce4, verified as an ancestor of origin/dev).

Your authorship is preserved by a Co-authored-by trailer in the landed commit itself rather than only in the pull request body, so it counts on your contributor graph.

The carry folded the review findings already on this pull request. If you think something from this branch did not make it to dev, say so and I will reopen.

@lidge-jun lidge-jun closed this Sep 13, 2026
S0RYUASUKA pushed a commit to S0RYUASUKA/opencodex that referenced this pull request Sep 13, 2026
…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 lidge-jun#4455 by jeongjin0 and the overlapping four-file subset in lidge-jun#4171 by
rrmlima, both answering lidge-jun#4412.

Folds in the review findings recorded on lidge-jun#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants