From 61b9aefc3829b37e002bef7ebc61b159de7e009b Mon Sep 17 00:00:00 2001 From: jeongjin Date: Sun, 13 Sep 2026 13:22:22 +0900 Subject: [PATCH] fix(responses): execute code-mode view_image helper 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 --- src/responses/code-mode-helper-compat.ts | 25 +++- src/server/responses-undeclared-tool-guard.ts | 2 +- src/server/responses/core.ts | 2 +- src/types/tools.ts | 22 ++- structure/transports/responses.md | 4 +- .../bridge-legacy-shell-normalization.test.ts | 35 +++++ tests/responses/legacy-shell-compat.test.ts | 134 ++++++++++++++++++ .../responses-undeclared-tool-guard.test.ts | 69 ++++++++- 8 files changed, 277 insertions(+), 16 deletions(-) diff --git a/src/responses/code-mode-helper-compat.ts b/src/responses/code-mode-helper-compat.ts index 726c5dcf19..756b78aa14 100644 --- a/src/responses/code-mode-helper-compat.ts +++ b/src/responses/code-mode-helper-compat.ts @@ -31,7 +31,10 @@ function unwrapPatchInput(value: string): string { */ export function compileCodeModeHelperInput(argumentsText: unknown, toolName: string): string { if (typeof argumentsText !== "string") return ""; - if (toolName === "apply_patch") { + const helperName = toolName.startsWith("default.") + ? toolName.slice("default.".length) + : toolName; + if (helperName === "apply_patch") { const patch = normalizeApplyPatchDelimiters(unwrapPatchInput(argumentsText)); return `const result = await tools.apply_patch(${JSON.stringify(patch)});\ntext(result);`; } @@ -43,7 +46,7 @@ export function compileCodeModeHelperInput(argumentsText: unknown, toolName: str } const args: unknown = isPlainObject(parsed) ? { ...parsed } : parsed; if ( - toolName === "shell_command" + helperName === "shell_command" && isPlainObject(args) && typeof args.command === "string" && args.cmd === undefined @@ -51,9 +54,25 @@ export function compileCodeModeHelperInput(argumentsText: unknown, toolName: str args.cmd = args.command; delete args.command; } - if (toolName === "write_stdin") { + if (helperName === "write_stdin") { return `const result = await tools.write_stdin(${JSON.stringify(args)});\ntext(result);`; } + if (helperName === "view_image") { + // Codex code-mode `exec` exposes `tools.view_image({path, detail?})`; the host answers + // with a custom_tool_call_output carrying `input_image`, which `image()` surfaces back + // to the model. Aliases map onto Codex's `path`/`detail`; anything else is passed as + // data so nested validation can reject it. + const viewArgs: unknown = isPlainObject(args) ? { ...args } : args; + if (isPlainObject(viewArgs)) { + for (const alias of ["file_path", "file", "image_path"]) { + if (typeof viewArgs.path !== "string" && typeof viewArgs[alias] === "string") { + viewArgs.path = viewArgs[alias]; + } + delete viewArgs[alias]; + } + } + return `const result = await tools.view_image(${JSON.stringify(viewArgs)});\nif (result && result.image_url) { image(result.image_url); } else { text(result); }`; + } return `const result = await tools.exec_command(${JSON.stringify(args)});\ntext(result);`; } diff --git a/src/server/responses-undeclared-tool-guard.ts b/src/server/responses-undeclared-tool-guard.ts index 69b445a713..58b2ce727c 100644 --- a/src/server/responses-undeclared-tool-guard.ts +++ b/src/server/responses-undeclared-tool-guard.ts @@ -106,7 +106,7 @@ function addWireToolName( } // `exec` is the one name that also switches on nested-helper normalization, so a bare alias // for a namespaced MCP tool would silently authorize `exec_command`/`shell_command`/ - // `apply_patch` the request never declared. Every other inner name keeps the bare alias. + // `apply_patch`/`view_image` the request never declared. Every other inner name keeps the bare alias. if (name !== CODE_MODE_EXEC_TOOL_NAME) names.add(name); } diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 6d89ff13ae..392c26ef63 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -5071,7 +5071,7 @@ async function handleResponsesInner( // `buildToolBridgeMaps` also aliases a namespaced tool under its bare name when the // caller's `tool_choice` selected it unambiguously, which the bridge needs to route the // call back. For `exec` alone that alias would also switch on nested-helper - // normalization and re-authorize `exec_command`/`shell_command`/`apply_patch`, so it is + // normalization and re-authorize `exec_command`/`shell_command`/`apply_patch`/`view_image`, so it is // admitted here only when the caller's own catalog declared a bare `exec`. Selecting an // MCP `exec` is not a declaration of the code-mode shell tool. if ( diff --git a/src/types/tools.ts b/src/types/tools.ts index 7a4cdc4ebb..fd80a4b100 100644 --- a/src/types/tools.ts +++ b/src/types/tools.ts @@ -47,16 +47,17 @@ export function dottedToolName(namespace: string | undefined, name: string): str * * Codex's code-mode shell tool is declared as `exec` (a freeform custom tool whose own * description mentions the nested `await tools.exec_command(...)` helper). Some routed providers - * echo that helper name as the tool-call name, emitting `exec_command`, `write_stdin`, or - * `apply_patch` instead of the declared `exec`. Accept these nested helper names only when the - * request catalog actually declares `exec` and does not itself declare the emitted name (an MCP - * server may legitimately advertise one under its own namespace). + * echo that helper name as the tool-call name, emitting `exec_command`, `write_stdin`, + * `apply_patch`, or `view_image` instead of the declared `exec`. Accept these nested helper names + * only when the request catalog actually declares `exec` and does not itself declare the emitted + * name (an MCP server may legitimately advertise one under its own namespace). */ const LEGACY_SHELL_BRIDGE_TOOL_NAMES = ["exec_command", "shell_command"] as const; const CODE_MODE_HELPER_TOOL_NAMES = [ ...LEGACY_SHELL_BRIDGE_TOOL_NAMES, "write_stdin", "apply_patch", + "view_image", ] as const; /** @@ -71,7 +72,7 @@ export const CODE_MODE_EXEC_TOOL_NAME = "exec"; * * Rewrites invented `default.` prefixes back to a declared bare tool when that bare tool * is declared and neither `default.` nor `default__` was explicitly declared (#4176). - * Also normalizes legacy helper names (`exec_command`, `shell_command`, `apply_patch`) to + * Also normalizes legacy helper names (`exec_command`, `shell_command`, `apply_patch`, `view_image`) to * `exec` when code-mode `exec` is declared in the request catalog. * * @param name - The tool name emitted on the wire by the provider. @@ -98,6 +99,17 @@ export function normalizeDeclaredToolName( && !declared.has("default__" + bare) ) { candidate = bare; + } else if ( + // Code mode never declares bare helper names; a provider that invents `default.` + // for one still means the nested helper. Strip the prefix so the helper list + // below can rewrite it to `exec` (#4412). + bare.length > 0 + && declared.has(CODE_MODE_EXEC_TOOL_NAME) + && (CODE_MODE_HELPER_TOOL_NAMES as readonly string[]).includes(bare) + && !declared.has("default." + bare) + && !declared.has("default__" + bare) + ) { + candidate = bare; } } if (!declared.has(CODE_MODE_EXEC_TOOL_NAME)) return candidate; diff --git a/structure/transports/responses.md b/structure/transports/responses.md index 96ef06d3c0..8ff2d1e31b 100644 --- a/structure/transports/responses.md +++ b/structure/transports/responses.md @@ -271,9 +271,7 @@ Cursor's error classification and Kiro's whitespace and failed-wrapper grouping halves live in `src/adapters/exec-tool-result-normalize.ts` so the pre-call and post-hoc wording cannot drift. This guidance and annotation change rewrites neither the model's JavaScript nor its patch payload; the existing name-alias delimiter -normalization in `src/responses/code-mode-helper-compat.ts` is unchanged, and the host still rejects a -malformed call exactly as before. Anthropic, Google, OpenAI-chat and command-code result paths -have no exec-result seam today and are not annotated. +normalization in `src/responses/code-mode-helper-compat.ts` remains separate from result annotation. That boundary maps a routed provider's bare or synthetic-`default.` `view_image` call onto the declared code-mode `exec`, invokes nested `tools.view_image`, and emits the returned `image_url` through `image()`. Explicit `path` wins; otherwise `file_path`, `file`, and `image_path` normalize in that order. Malformed input still reaches nested validation as data. Anthropic, Google, OpenAI-chat and command-code result paths have no exec-result seam today and are not annotated. > Decision record: [ADR-0040](../decisions/ADR-0040-responses-http-sse.md) diff --git a/tests/adapters/bridge-legacy-shell-normalization.test.ts b/tests/adapters/bridge-legacy-shell-normalization.test.ts index 0b4a94283c..9cdc2e1b88 100644 --- a/tests/adapters/bridge-legacy-shell-normalization.test.ts +++ b/tests/adapters/bridge-legacy-shell-normalization.test.ts @@ -97,6 +97,41 @@ describe("bridge normalizes code-mode helper names against the declared catalog" expect(sse).toContain("image.png"); }); + test("view_image is compiled through code-mode exec and surfaces the image", async () => { + const sse = await drain(bridgeToResponsesSSE( + toolTurn("view_image", '{"file_path":"/tmp/image.png","detail":"high"}'), + "fixture-model", + undefined, + new Set(["exec"]), + undefined, + undefined, + 50_000, + { declaredToolNames: new Set(["exec"]) }, + )); + expect(sse).not.toContain("undeclared client tool"); + expect(sse).toContain('"name":"exec"'); + expect(sse).toContain('await tools.view_image({\\"detail\\":\\"high\\",\\"path\\":\\"/tmp/image.png\\"})'); + expect(sse).toContain("image(result.image_url)"); + expect(sse).not.toContain("tools.exec_command"); + }); + + test("default.view_image is compiled through code-mode exec", async () => { + const sse = await drain(bridgeToResponsesSSE( + toolTurn("default.view_image", '{"path":"/tmp/image.png"}'), + "fixture-model", + undefined, + new Set(["exec"]), + undefined, + undefined, + 50_000, + { declaredToolNames: new Set(["exec"]) }, + )); + expect(sse).not.toContain("undeclared client tool"); + expect(sse).toContain('"name":"exec"'); + expect(sse).toContain("await tools.view_image"); + expect(sse).not.toContain("tools.exec_command"); + }); + test("a catalog that declares exec_command itself is never rewritten", async () => { const sse = await drain(bridgeToResponsesSSE( toolTurn("exec_command"), "deepseek-x", undefined, undefined, undefined, undefined, 50_000, diff --git a/tests/responses/legacy-shell-compat.test.ts b/tests/responses/legacy-shell-compat.test.ts index 6503c3e6dd..62438d7c53 100644 --- a/tests/responses/legacy-shell-compat.test.ts +++ b/tests/responses/legacy-shell-compat.test.ts @@ -112,4 +112,138 @@ describe("code-mode helper compatibility", () => { expect(received).toEqual(input === "[]" ? [] : input); } }); + + test("view_image compiles to tools.view_image and forwards image_url to image()", async () => { + const source = compileCodeModeHelperInput( + JSON.stringify({ path: "/tmp/shot.png", detail: "high" }), + "default.view_image", + ); + let received: unknown; + let surfaced: unknown; + const run = new AsyncFunction("tools", "text", "image", source); + + await run( + { + view_image: async (args: unknown) => { + received = args; + return { image_url: "data:image/png;base64,AAAA" }; + }, + }, + () => { throw new Error("image result leaked into text output"); }, + (value: unknown) => { surfaced = value; }, + ); + + expect(received).toEqual({ path: "/tmp/shot.png", detail: "high" }); + expect(surfaced).toBe("data:image/png;base64,AAAA"); + }); + + test("view_image maps file_path/file/image_path aliases onto path", async () => { + for (const alias of ["file_path", "file", "image_path"]) { + const source = compileCodeModeHelperInput( + JSON.stringify({ [alias]: "/tmp/alias.png" }), + "view_image", + ); + let received: unknown; + const run = new AsyncFunction("tools", "text", "image", source); + await run( + { + view_image: async (args: unknown) => { + received = args; + return {}; + }, + }, + () => {}, + () => {}, + ); + expect(received).toEqual({ path: "/tmp/alias.png" }); + } + }); + + test("view_image keeps explicit path precedence and removes provider aliases", async () => { + const source = compileCodeModeHelperInput( + JSON.stringify({ path: "/tmp/right.png", file_path: "/tmp/wrong.png", detail: "original" }), + "view_image", + ); + let received: unknown; + const run = new AsyncFunction("tools", "text", "image", source); + await run( + { + view_image: async (args: unknown) => { + received = args; + return {}; + }, + }, + () => {}, + () => {}, + ); + expect(received).toEqual({ path: "/tmp/right.png", detail: "original" }); + }); + + test("view_image aliases use deterministic precedence when providers send more than one", async () => { + const source = compileCodeModeHelperInput( + JSON.stringify({ + file_path: "/tmp/file-path.png", + file: "/tmp/file.png", + image_path: "/tmp/image-path.png", + }), + "view_image", + ); + let received: unknown; + const run = new AsyncFunction("tools", "text", "image", source); + await run( + { + view_image: async (args: unknown) => { + received = args; + return {}; + }, + }, + () => {}, + () => {}, + ); + + expect(received).toEqual({ path: "/tmp/file-path.png" }); + }); + + test("view_image without image_url still returns the host result", async () => { + const source = compileCodeModeHelperInput( + JSON.stringify({ path: "/tmp/missing.png" }), + "view_image", + ); + let surfaced = false; + let output: unknown; + const run = new AsyncFunction("tools", "text", "image", source); + await run( + { + view_image: async () => ({ error: "not found" }), + }, + (value: unknown) => { output = value; }, + () => { surfaced = true; }, + ); + expect(surfaced).toBe(false); + expect(output).toEqual({ error: "not found" }); + }); + + test("invalid view_image input remains data instead of becoming JavaScript", async () => { + const input = "{not-json`); throw new Error('escaped') //"; + let received: unknown; + const run = new AsyncFunction( + "tools", + "text", + "image", + compileCodeModeHelperInput(input, "view_image"), + ); + + await run( + { + view_image: async (args: unknown) => { + received = args; + return { error: "invalid input" }; + }, + }, + () => {}, + () => {}, + ); + + expect(received).toBe(input); + }); }); diff --git a/tests/responses/responses-undeclared-tool-guard.test.ts b/tests/responses/responses-undeclared-tool-guard.test.ts index 93d7f0f020..9ff5c593b7 100644 --- a/tests/responses/responses-undeclared-tool-guard.test.ts +++ b/tests/responses/responses-undeclared-tool-guard.test.ts @@ -134,7 +134,7 @@ describe("collectDeclaredWireToolNames", () => { test("withholds the bare alias when only a namespaced exec was declared", () => { // A bare `exec` in the declared set is not just a name: it switches on nested-helper // normalization, so aliasing a namespaced MCP `exec` under the bare name would authorize - // `exec_command`/`shell_command`/`apply_patch` this request never declared. + // `exec_command`/`shell_command`/`apply_patch`/`view_image` this request never declared. const names = collectDeclaredWireToolNames({ tools: [{ type: "namespace", name: "mcp", tools: [{ type: "function", name: "exec" }] }], }); @@ -848,6 +848,57 @@ describe("the reported turn, end to end through handleResponses", () => { expect(body.output[0]?.input).toContain("await tools.apply_patch"); }); + test("streaming: default.view_image is bridged through unified exec with image output", async () => { + const viewImageCall = { + type: "function_call", + id: "fc_view", + call_id: "call_view", + name: "default.view_image", + arguments: JSON.stringify({ image_path: "/tmp/image.png", detail: "original" }), + status: "completed", + }; + const response = await post(true, () => new Response([ + frame("response.output_item.added", { + output_index: 0, + item: { ...viewImageCall, arguments: "", status: "in_progress" }, + }), + frame("response.output_item.done", { output_index: 0, item: viewImageCall }), + frame("response.completed", { + response: { id: "resp_view", status: "completed", output: [viewImageCall] }, + }), + "data: [DONE]", + ].join("\n\n") + "\n\n", { headers: { "content-type": "text/event-stream" } })); + + const body = await response.text(); + expect(body).not.toContain("response.failed"); + expect(body).toContain('"name":"exec"'); + expect(body).toContain("await tools.view_image"); + expect(body).toContain('\\"path\\":\\"/tmp/image.png\\"'); + expect(body).toContain("image(result.image_url)"); + expect(body).not.toContain("tools.exec_command"); + }); + + test("non-streaming: bare view_image is bridged through unified exec", async () => { + const viewImageCall = { + type: "function_call", + id: "fc_view", + call_id: "call_view", + name: "view_image", + arguments: JSON.stringify({ path: "/tmp/image.png" }), + status: "completed", + }; + const response = await post(false, () => new Response( + JSON.stringify({ id: "resp_view", status: "completed", output: [viewImageCall] }), + { headers: { "content-type": "application/json" } }, + )); + + expect(response.status).toBe(200); + const body = await response.json() as { output: Array> }; + expect(body.output[0]).toMatchObject({ type: "custom_tool_call", name: "exec" }); + expect(String(body.output[0]?.input)).toContain("await tools.view_image"); + expect(String(body.output[0]?.input)).toContain("image(result.image_url)"); + }); + test("a declared exec call still completes normally", async () => { const execCall = { type: "function_call", @@ -2001,7 +2052,7 @@ describe("empty and absent tool catalogs", () => { namespace: "mcp__functions", }); - for (const name of ["apply_patch", "exec_command", "shell_command", "write_stdin"]) { + for (const name of ["apply_patch", "exec_command", "shell_command", "write_stdin", "view_image"]) { const refused = await post( false, tools, @@ -2099,6 +2150,18 @@ describe("undeclaredToolCallNameInResponse", () => { expect(undeclaredToolCallNameInResponse(response, new Set())).toBe("write_stdin"); }); + test("accepts view_image helper spellings only through a bare unified exec declaration", () => { + for (const name of ["view_image", "default.view_image"]) { + const response = { output: [{ type: "function_call", name }] }; + expect(undeclaredToolCallNameInResponse(response, new Set(["exec"]))).toBeUndefined(); + expect(undeclaredToolCallNameInResponse(response, new Set())).toBe(name); + expect(undeclaredToolCallNameInResponse(response, new Set(["exec_command"]))).toBe(name); + } + + const direct = { output: [{ type: "function_call", name: "view_image" }] }; + expect(undeclaredToolCallNameInResponse(direct, new Set(["view_image"]))).toBeUndefined(); + }); + test("never legacy-normalizes a namespaced shell bridge call", () => { // A namespaced call (e.g. an MCP server advertising its own exec_command) must be // matched by its full wire name only — never normalized to bare `exec`. @@ -2122,7 +2185,7 @@ describe("undeclaredToolCallNameInResponse", () => { tools: [{ type: "namespace", name: "mcp", tools: [{ type: "function", name: "exec" }] }], }); - for (const name of ["exec_command", "shell_command", "apply_patch", "write_stdin", "exec"]) { + for (const name of ["exec_command", "shell_command", "apply_patch", "write_stdin", "view_image", "exec"]) { expect(undeclaredToolCallNameInResponse( { output: [{ type: "function_call", name, call_id: "call_1" }] }, declared,