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 }, + }); + } + }); +});