Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions src/responses/code-mode-helper-compat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);`;
}
Expand All @@ -43,17 +46,33 @@ 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
) {
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") {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Add the missing co-author trailers

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 👍 / 👎.

// 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);`;
}

Expand Down
2 changes: 1 addition & 1 deletion src/server/responses-undeclared-tool-guard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
2 changes: 1 addition & 1 deletion src/server/responses/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down
22 changes: 17 additions & 5 deletions src/types/tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -71,7 +72,7 @@ export const CODE_MODE_EXEC_TOOL_NAME = "exec";
*
* Rewrites invented `default.<name>` prefixes back to a declared bare tool when that bare tool
* is declared and neither `default.<name>` nor `default__<name>` 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.
Expand All @@ -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)
Comment on lines +107 to +108

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject default helpers in flat-bridge catalogs

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 👍 / 👎.

&& !declared.has("default." + bare)
&& !declared.has("default__" + bare)
) {
candidate = bare;
}
}
if (!declared.has(CODE_MODE_EXEC_TOOL_NAME)) return candidate;
Expand Down
12 changes: 6 additions & 6 deletions structure/transports/responses.md
Original file line number Diff line number Diff line change
Expand Up @@ -268,12 +268,12 @@ recovery hint naming the broken rule; flat shell bridges and foreign MCP namespa
annotated, Responses and Kiro additionally require the request's verified code-mode catalog, Cursor
matches the exact `exec` name under its `opencodex-responses` provider without catalog context, and
Cursor's error classification and Kiro's whitespace and failed-wrapper grouping are unchanged. Both
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.
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 name-alias normalization in `src/responses/code-mode-helper-compat.ts` also
compiles `view_image` into the declared `exec` and surfaces its `image_url` through `image()`, 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.

> Decision record: [ADR-0040](../decisions/ADR-0040-responses-http-sse.md)

Expand Down
54 changes: 54 additions & 0 deletions tests/adapters/bridge-legacy-shell-normalization.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -106,4 +141,23 @@ describe("bridge normalizes code-mode helper names against the declared catalog"
expect(sse).toContain('"name":"exec_command"');
expect(sse).toContain('"arguments":"{\\"cmd\\":\\"ls\\"}"');
});

// #4171 review: the flat-bridge shape declares `exec` next to a bare `exec_command`, where
// `exec` may be an ordinary caller tool and nested `tools.*` helpers are not what it runs.
// A `view_image` call there must not be compiled into code-mode JavaScript.
test("a flat-bridge catalog never compiles view_image into code-mode exec", async () => {
const sse = await drain(bridgeToResponsesSSE(
toolTurn("view_image", '{"path":"/tmp/image.png"}'),
"deepseek-x",
undefined,
undefined,
undefined,
undefined,
50_000,
{ declaredToolNames: new Set(["exec", "exec_command", "view_image"]) },
));
expect(sse).toContain('"name":"view_image"');
expect(sse).not.toContain("tools.view_image");
expect(sse).not.toContain('"name":"exec"');
});
});
134 changes: 134 additions & 0 deletions tests/responses/legacy-shell-compat.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
Loading
Loading