From 9672931c89d60d3fb0f75e16f3ebfaee8889a7c7 Mon Sep 17 00:00:00 2001 From: JUN Date: Fri, 18 Sep 2026 18:57:54 +0900 Subject: [PATCH] fix(responses): compile the apply_patch body recognition accepted `resolveCodeModeHelperName` decides a code-mode `exec` body is an apply-patch call by reading `unwrapFreeformToolInput` under the wire tool name, which strips an outer Markdown fence and accepts that name's fallback fields (#4983). Compilation kept a narrower unwrap that saw only `input` and `patch`, so a body accepted through a fence or through a field like `content` reached `tools.apply_patch` still wrapped. The host then rejected the JSON text or the fence instead of applying the patch. Both halves now read one canonical body, produced by one unwrap under the name the body actually arrived on. That name is not always the helper: the name-based path arrives as `apply_patch`, whose fallback fields are `patch` and `content`, while the body-based path arrives as `exec`, whose fields are the code-carrying ones. `{"patch": ...}` is meaningful for the first and is correctly refused by the second, so reading it under `exec` would compile a body recognition had declined. Closes #5046. --- src/bridge/response-json.ts | 2 +- src/bridge/sse.ts | 2 +- src/responses/code-mode-helper-compat.ts | 40 ++++--- src/responses/custom-tool-compat.ts | 2 +- src/server/responses-custom-tool-repair.ts | 4 +- .../responses-code-mode-patch-compile.test.ts | 106 ++++++++++++++++++ 6 files changed, 136 insertions(+), 20 deletions(-) create mode 100644 tests/responses/responses-code-mode-patch-compile.test.ts diff --git a/src/bridge/response-json.ts b/src/bridge/response-json.ts index e9c466a45f8..cdc9cd1a02e 100644 --- a/src/bridge/response-json.ts +++ b/src/bridge/response-json.ts @@ -203,7 +203,7 @@ function buildResponseJSONWithBudget( ): string => { const helper = resolveCodeModeHelperName(codeModeHelperName, toolName, args, namespace, options?.declaredToolNames); return helper - ? compileCodeModeHelperInput(args, helper) + ? compileCodeModeHelperInput(args, helper, codeModeHelperName ?? toolName) : repairFreeformToolInput(args, toolName, namespace); }; const parseArgsObj = (args: string): Record => { diff --git a/src/bridge/sse.ts b/src/bridge/sse.ts index eda83f01f78..c11de670d48 100644 --- a/src/bridge/sse.ts +++ b/src/bridge/sse.ts @@ -145,7 +145,7 @@ export function bridgeToResponsesSSE( ): string => { const helper = resolveCodeModeHelperName(codeModeHelperName, toolName, args, namespace, options?.declaredToolNames); return helper - ? compileCodeModeHelperInput(args, helper) + ? compileCodeModeHelperInput(args, helper, codeModeHelperName ?? toolName) : repairFreeformToolInput(args, toolName, namespace); }; // Best-effort unwrap of a PARTIAL freeform arg buffer for live input streaming diff --git a/src/responses/code-mode-helper-compat.ts b/src/responses/code-mode-helper-compat.ts index 1ad74425d31..d390bb76cb4 100644 --- a/src/responses/code-mode-helper-compat.ts +++ b/src/responses/code-mode-helper-compat.ts @@ -9,33 +9,43 @@ function isPlainObject(value: unknown): value is Record { return !!value && typeof value === "object" && !Array.isArray(value); } -function unwrapPatchInput(value: string): string { - try { - const parsed: unknown = JSON.parse(value); - if (isPlainObject(parsed)) { - if (typeof parsed.input === "string") return parsed.input; - if (typeof parsed.patch === "string") return parsed.patch; - } - } catch { - // Native custom calls carry the patch body directly. - } - return value; -} - /** * Convert a nested Code Mode helper call into unified-exec JavaScript. * * Parsed values are serialized as data, never interpolated as source, so command and patch text * cannot escape the generated call. Invalid structured helper payloads are also passed as data so * nested-tool validation can reject them without evaluating provider text as JavaScript. + * + * `wireToolName` is the name the body actually arrived under, which is not always the helper: + * a provider that got the NAME right and the BODY wrong sends `exec`, and the apply-patch + * helper is inferred from the payload. Recognition and compilation must read ONE canonical + * body, so both unwrap under that same name — see the apply-patch branch below. It defaults to + * the helper name, which is correct for the name-based path where the wire name IS the helper. */ -export function compileCodeModeHelperInput(argumentsText: unknown, toolName: string): string { +export function compileCodeModeHelperInput( + argumentsText: unknown, + toolName: string, + wireToolName?: string, +): string { if (typeof argumentsText !== "string") return ""; const helperName = toolName.startsWith("default.") ? toolName.slice("default.".length) : toolName; if (helperName === "apply_patch") { - const patch = normalizeApplyPatchDelimiters(unwrapPatchInput(argumentsText)); + // `resolveCodeModeHelperName` decides this IS an apply-patch call by reading + // `unwrapFreeformToolInput(argumentsText, wireToolName)`, which strips an outer Markdown + // fence and accepts that name's fallback fields (#4983). Compiling from a narrower unwrap + // meant a body accepted through a fence or a fallback field reached `tools.apply_patch` + // still wrapped, so the host rejected the JSON or the fence instead of applying the patch + // (#5046). One unwrap, one body, one decision. + // + // The vocabulary is the WIRE name rather than the helper name on purpose. `{"patch": ...}` + // is an apply_patch wrapper and is not an `exec` fallback field, and the recognizer already + // declines it under `exec`; reading it here would compile a body that recognition rejected, + // which is exactly the drift a second, looser unwrap introduces. + const patch = normalizeApplyPatchDelimiters( + unwrapFreeformToolInput(argumentsText, wireToolName ?? helperName), + ); return `const result = await tools.apply_patch(${JSON.stringify(patch)});\ntext(result);`; } let parsed: unknown = argumentsText; diff --git a/src/responses/custom-tool-compat.ts b/src/responses/custom-tool-compat.ts index 3577a23256b..399dba6b2e2 100644 --- a/src/responses/custom-tool-compat.ts +++ b/src/responses/custom-tool-compat.ts @@ -326,7 +326,7 @@ export function restoreRoutedCustomCalls( id: customToolItemId(item.id), name: aliased ? targetName : item.name, input: helper - ? compileCodeModeHelperInput(sourceInput, helper) + ? compileCodeModeHelperInput(sourceInput, helper, aliased ? String(item.name) : targetName) : repairFreeformToolInput( sourceInput, targetName, diff --git a/src/server/responses-custom-tool-repair.ts b/src/server/responses-custom-tool-repair.ts index c9d8527ebc6..1cb167b9f4f 100644 --- a/src/server/responses-custom-tool-repair.ts +++ b/src/server/responses-custom-tool-repair.ts @@ -298,7 +298,7 @@ export function createRoutedCustomToolRestoreBlockRewrite( releaseCall(upstreamItemId); return [replaceSseDataPayload(block, JSON.stringify({ ...parsed, - input: helper ? compileCodeModeHelperInput(source, helper) : source, + input: helper ? compileCodeModeHelperInput(source, helper, name) : source, }))]; } if ( @@ -381,7 +381,7 @@ export function createRoutedCustomToolRestoreBlockRewrite( type: nextType, item_id: customToolItemId(upstreamItemId), input: helper - ? compileCodeModeHelperInput(source, helper) + ? compileCodeModeHelperInput(source, helper, itemName?.name ?? "") : unwrapRoutedCustomToolArguments(source, itemName?.name ?? "", itemName?.namespace), }; return [replaceSseDataPayload(replaceSseEventName(block, nextType), JSON.stringify(next))]; diff --git a/tests/responses/responses-code-mode-patch-compile.test.ts b/tests/responses/responses-code-mode-patch-compile.test.ts new file mode 100644 index 00000000000..aa12e7c63fd --- /dev/null +++ b/tests/responses/responses-code-mode-patch-compile.test.ts @@ -0,0 +1,106 @@ +import { describe, expect, test } from "bun:test"; +import { compileCodeModeHelperInput, resolveCodeModeHelperName } from "../../src/responses/code-mode-helper-compat"; +import { restoreRoutedCustomCallsInJson } from "../../src/responses/custom-tool-compat"; + +/** + * Recognition and compilation must read ONE canonical body (#5046). + * + * #4983 widened what counts as an apply-patch call under a code-mode `exec` catalog: an + * outer Markdown fence, and any single fallback field of that tool name. Recognition used + * the widened unwrap; compilation kept a narrower one that saw only `input` and `patch`. So + * a body accepted through a fence or through `content` reached `tools.apply_patch` still + * wrapped, and the host rejected the JSON text or the fence instead of applying the patch. + * + * These cases are written against the pair, not against either half, because the defect was + * that the two halves disagreed while each looked correct alone. + */ +const PATCH = "*** Begin Patch\n*** Update File: README.md\n@@\n-old\n+new\n*** End Patch"; +const EXPECTED = `const result = await tools.apply_patch(${JSON.stringify(PATCH)});\ntext(result);`; +const CODE_MODE = new Set(["exec"]); + +/** Exactly what every bridge call site does: recognize, then compile under the wire name. */ +function compileAsBridge(body: string, wireName = "exec", declared = CODE_MODE): string | undefined { + const helper = resolveCodeModeHelperName(undefined, wireName, body, undefined, declared); + return helper ? compileCodeModeHelperInput(body, helper, wireName) : undefined; +} + +describe("code-mode apply_patch compiles the body recognition accepted", () => { + test("every accepted exec fallback field compiles to the same raw patch", () => { + // The list is `FREEFORM_FALLBACK_KEYS.exec` plus the `input` wrapper. If a key is added + // there and not here, the two lists have drifted and the next reader should be told. + for (const key of ["input", "code", "script", "js", "javascript", "command", "cmd", "content"]) { + const body = JSON.stringify({ [key]: PATCH }); + expect({ key, source: compileAsBridge(body) }).toEqual({ key, source: EXPECTED }); + } + }); + + test("fenced and unfenced forms compile identically", () => { + const fenced = "```\n" + PATCH + "\n```"; + expect(compileAsBridge(PATCH)).toBe(EXPECTED); + expect(compileAsBridge(fenced)).toBe(EXPECTED); + expect(compileAsBridge(JSON.stringify({ input: fenced }))).toBe(EXPECTED); + expect(compileAsBridge("```diff\n" + PATCH + "\n```")).toBe(EXPECTED); + }); + + test("a native apply_patch call keeps its own vocabulary", () => { + // The name-based path arrives under `apply_patch`, whose fallback keys are `patch` and + // `content`. `{"patch": ...}` is meaningful there and is NOT an exec fallback field, so + // the two names deliberately answer differently — which is why the wire name, not the + // helper name, decides. + expect(compileCodeModeHelperInput(JSON.stringify({ patch: PATCH }), "apply_patch")).toBe(EXPECTED); + expect(compileCodeModeHelperInput(PATCH, "apply_patch")).toBe(EXPECTED); + expect(compileAsBridge(JSON.stringify({ patch: PATCH }))).toBeUndefined(); + }); + + test("a normal code-mode JavaScript body is left alone", () => { + for (const body of [ + 'const result = await tools.exec_command({ cmd: "ls" });\ntext(result);', + 'await tools.apply_patch("*** Begin Patch\\n*** Add File: a.txt\\n+hi\\n*** End Patch");', + JSON.stringify({ input: 'const a = 1;\ntext(a);' }), + ]) { + expect(resolveCodeModeHelperName(undefined, "exec", body, undefined, CODE_MODE)).toBeUndefined(); + } + }); + + test("a caller-defined exec outside a code-mode catalog is never reinterpreted", () => { + // Without a genuine code-mode catalog an `exec` that takes patch text is a legitimate + // caller tool, and handing it generated JavaScript would be the mis-route this repair + // exists to avoid. The widened unwrap must not change that. + for (const body of [PATCH, JSON.stringify({ content: PATCH }), "```\n" + PATCH + "\n```"]) { + expect(compileAsBridge(body, "exec", new Set(["shell"]))).toBeUndefined(); + // A catalog that also declares a legacy shell bridge is not code mode either, and no + // declared set at all is the plainest case of the same rule. + expect(compileAsBridge(body, "exec", new Set(["exec", "shell_command"]))).toBeUndefined(); + expect(resolveCodeModeHelperName(undefined, "exec", body, undefined, undefined)).toBeUndefined(); + } + }); + + test("the compiled helper survives the bridge, not only the recognizer", () => { + // The unit above proves the pair agrees. This proves the agreement reaches the item a + // client actually receives: before the fix this restored + // `tools.apply_patch("{\"content\":\"*** Begin Patch...\"}")`. + for (const key of ["content", "code", "input"]) { + const upstream = JSON.stringify({ + id: "resp_patch", + output: [{ + type: "function_call", + id: "fc_patch", + call_id: "call_patch", + name: "exec", + arguments: JSON.stringify({ [key]: PATCH }), + status: "completed", + }], + }); + const restored = JSON.parse(restoreRoutedCustomCallsInJson( + upstream, + new Set(["exec"]), + new Set(), + CODE_MODE, + )) as { output: Array> }; + expect({ key, item: restored.output[0] }).toMatchObject({ + key, + item: { type: "custom_tool_call", name: "exec", input: EXPECTED }, + }); + } + }); +});