From ec9d3157bb3d4413db982fd84eb3bb3d85301eab Mon Sep 17 00:00:00 2001 From: Dallin Romney Date: Thu, 24 Jul 2025 19:06:42 +0700 Subject: [PATCH 1/2] fix: system message tools array args for search and replace tool --- .../definitions/searchAndReplaceInFile.ts | 2 +- .../systemMessageTools/parseSystemToolCall.ts | 21 ++++++++++- .../parseSystemToolCall.vitest.ts | 32 ++++++++++++++++ manual-testing-sandbox/test.js | 37 ------------------- 4 files changed, 53 insertions(+), 39 deletions(-) diff --git a/core/tools/definitions/searchAndReplaceInFile.ts b/core/tools/definitions/searchAndReplaceInFile.ts index aa12e7c72..96dac65da 100644 --- a/core/tools/definitions/searchAndReplaceInFile.ts +++ b/core/tools/definitions/searchAndReplaceInFile.ts @@ -104,7 +104,7 @@ ${SEARCH_AND_REPLACE_RULES} [ ["filepath", "path/to/file.ts"], [ - "diff", + "diffs", `[ "------- SEARCH [exact content to find] diff --git a/core/tools/systemMessageTools/parseSystemToolCall.ts b/core/tools/systemMessageTools/parseSystemToolCall.ts index 74dc6f470..0300654d9 100644 --- a/core/tools/systemMessageTools/parseSystemToolCall.ts +++ b/core/tools/systemMessageTools/parseSystemToolCall.ts @@ -87,12 +87,31 @@ export function handleToolCallBuffer( if (isNewLine) { const isEndArgTag = line.match(/end_?arg/i); if (isEndArgTag) { - const trimmedValue = state.currentArgLines.join("").trim(); + let trimmedValue = state.currentArgLines.join("").trim(); state.currentArgLines.length = 0; state.processedArgNames.add(state.currentArgName); state.currentArgName = undefined; try { + if ( + trimmedValue.startsWith("[") || + trimmedValue.startsWith("{") + ) { + trimmedValue = trimmedValue.replace( + /"((?:\\[\s\S]|[^"\\])*?)"/g, + (match) => { + const content = match.slice(1, -1); + // Replace unescaped newlines + return ( + '"' + + content + .replace(/([^\\])\n/g, "$1\\n") + .replace(/^\n/g, "\\n") + + '"' + ); + }, + ); + } const parsed = JSON.parse(trimmedValue); const stringifiedArg = JSON.stringify(parsed); return createDelta("", stringifiedArg, state.toolCallId); diff --git a/core/tools/systemMessageTools/parseSystemToolCall.vitest.ts b/core/tools/systemMessageTools/parseSystemToolCall.vitest.ts index 436e3fa6e..2aeac2ce8 100644 --- a/core/tools/systemMessageTools/parseSystemToolCall.vitest.ts +++ b/core/tools/systemMessageTools/parseSystemToolCall.vitest.ts @@ -266,4 +266,36 @@ describe("handleToolCallBuffer", () => { }); expect(state.done).toBe(true); }); + + it("handles JSON array args", () => { + state.currentLineIndex = 3; + state.currentArgName = "test_arg"; + state.currentArgLines = [ + "[\n", + '"------- SEARCH\n', + " subtract(number) {\n", + " return this;\n", + " }\n", + "=======\n", + " subtract(number) {\n", + " this -= number\n", + " return this;\n", + " }\n", + '+++++++ REPLACE"\n', + "]\n", + ]; + + handleToolCallBuffer("END_ARG", state); + const newLineResult = handleToolCallBuffer("\n", state); + + expect(newLineResult).toEqual({ + type: "function", + function: { + name: "", + arguments: + '["------- SEARCH\\n subtract(number) {\\n return this;\\n }\\n=======\\n subtract(number) {\\n this -= number\\n return this;\\n }\\n+++++++ REPLACE"]', + }, + id: expect.any(String), + }); + }); }); diff --git a/manual-testing-sandbox/test.js b/manual-testing-sandbox/test.js index 0e60af4e0..d2920d6af 100644 --- a/manual-testing-sandbox/test.js +++ b/manual-testing-sandbox/test.js @@ -1,50 +1,21 @@ -/** - * Calculator class for performing basic arithmetic operations - * Uses method chaining to allow consecutive operations - */ class Calculator { - /** - * Initialize calculator with result set to 0 - */ constructor() { this.result = 0; } - /** - * Add a number to the current result - * @param {number} number - The number to add - * @returns {Calculator} - Returns this instance for method chaining - */ add(number) { this.result += number; return this; } - - /** - * Subtract a number from the current result - * @param {number} number - The number to subtract - * @returns {Calculator} - Returns this instance for method chaining - */ subtract(number) { return this; } - /** - * Multiply the current result by a number - * @param {number} number - The number to multiply by - * @returns {Calculator} - Returns this instance for method chaining - */ multiply(number) { this.result *= number; return this; } - /** - * Divide the current result by a number - * @param {number} number - The number to divide by - * @returns {Calculator} - Returns this instance for method chaining - * @throws {Error} - Throws error if attempting to divide by zero - */ divide(number) { if (number === 0) { throw new Error("Cannot divide by zero"); @@ -53,18 +24,10 @@ class Calculator { return this; } - /** - * Get the current result value - * @returns {number} - The current result - */ getResult() { return this.result; } - /** - * Reset the result to 0 - * @returns {Calculator} - Returns this instance for method chaining - */ reset() { this.result = 0; return this; From 3adf4b62a7019936f578cdcd1d72015827858b24 Mon Sep 17 00:00:00 2001 From: Dallin Romney Date: Thu, 24 Jul 2025 19:12:21 +0700 Subject: [PATCH 2/2] chore: revert duplicate fix logic for search and replace array args --- .../complexToolCall.vitest.ts | 304 ------------------ gui/src/util/clientTools/searchReplaceImpl.ts | 45 +-- 2 files changed, 4 insertions(+), 345 deletions(-) delete mode 100644 core/tools/systemMessageTools/complexToolCall.vitest.ts diff --git a/core/tools/systemMessageTools/complexToolCall.vitest.ts b/core/tools/systemMessageTools/complexToolCall.vitest.ts deleted file mode 100644 index 954427ee2..000000000 --- a/core/tools/systemMessageTools/complexToolCall.vitest.ts +++ /dev/null @@ -1,304 +0,0 @@ -import { beforeEach, describe, expect, it } from "vitest"; -import { AssistantChatMessage, ChatMessage, PromptLog } from "../.."; -import { interceptSystemToolCalls } from "./interceptSystemToolCalls"; - -describe("interceptSystemToolCalls - Complex Tool Call Test", () => { - let abortController: AbortController; - - beforeEach(() => { - abortController = new AbortController(); - }); - - const createAsyncGenerator = async function* ( - messages: ChatMessage[][], - ): AsyncGenerator { - for (const messageGroup of messages) { - yield messageGroup; - } - return undefined; - }; - - function isAssistantMessageWithToolCalls( - message: ChatMessage, - ): message is AssistantChatMessage { - return ( - message.role === "assistant" && - "toolCalls" in message && - message.toolCalls !== undefined && - message.toolCalls.length > 0 - ); - } - - async function collectAllResults( - generator: AsyncGenerator, - ): Promise { - const results: ChatMessage[][] = []; - while (true) { - const result = await generator.next(); - if (result.done) break; - if (result.value) results.push(result.value); - } - return results; - } - - it("handles complex multi-line tool arguments with nested content and special characters", async () => { - // This test case is based on the example.txt which has complex nested content - // that includes JSON arrays, code blocks, escape characters, and the word "assistant" - const complexDiffContent = `I'll add an exp method to the Calculator class in test.js. This method will raise the current result to the power of a given number. - -\`\`\`tool -TOOL_NAME: search_and_replace_in_file -BEGIN_ARG: filepath -test.js -END_ARG -BEGIN_ARG: diff -[ -"------- SEARCH - /** - * Divide the current result by a number - * @param {number} number - The number to divide by - * @ -assistant -returns {Calculator} - Returns this instance for method chaining - * @throws {Error} - Throws error if attempting to divide by zero - */ - divide(number) { - if (number === 0) { - throw new Error(\"Cannot divide by zero\"); - } - this.result /= number; - return this; - } - -======= - /** - * Divide the current result by a number - * @param {number} number - The number to -assistant - divide by - * @returns {Calculator} - Returns this instance for method chaining - * @throws {Error} - Throws error if attempting to divide by zero - */ - divide(number) { - if (number === 0) { - throw new Error(\"Cannot divide by zero\"); - } - this.result /= number; - return this; - } - - /** - * Raise the current result to the power of a number - * -assistant - @param {number} number - The exponent to raise to - * @returns {Calculator} - Returns this instance for method chaining - */ - exp(number) { - this.result = Math.pow(this.result, number); - return this; - } - -+++++++ REPLACE" -] -END_ARG -\`\`\``; - - const messages: ChatMessage[][] = [ - [ - { - role: "assistant", - content: - "I'll add an exp method to the Calculator class in test.js.\n\n", - }, - ], - [{ role: "assistant", content: "```tool\n" }], - [ - { - role: "assistant", - content: "TOOL_NAME: search_and_replace_in_file\n", - }, - ], - [{ role: "assistant", content: "BEGIN_ARG: filepath\n" }], - [{ role: "assistant", content: "test.js\n" }], - [{ role: "assistant", content: "END_ARG\n" }], - [{ role: "assistant", content: "BEGIN_ARG: diffs\n" }], - [{ role: "assistant", content: complexDiffContent }], - [{ role: "assistant", content: "\nEND_ARG\n" }], - [{ role: "assistant", content: "```" }], - ]; - - const generator = interceptSystemToolCalls( - createAsyncGenerator(messages), - abortController, - ); - - const allResults = await collectAllResults(generator); - - // Should have processed intro text - expect(allResults[0]).toEqual([ - { - role: "assistant", - content: [ - { - type: "text", - text: "I'll add an exp method to the Calculator class in test.js.", - }, - ], - }, - ]); - - // Should have detected and parsed the tool call - const toolCallResults = allResults.filter( - (r): r is AssistantChatMessage[] => - r && r[0] && isAssistantMessageWithToolCalls(r[0]), - ); - - expect(toolCallResults.length).toBeGreaterThan(0); - - // Tool name should be correct - const toolNameResult = toolCallResults.find( - (r) => - r[0].toolCalls?.[0]?.function?.name === "search_and_replace_in_file", - ); - expect(toolNameResult).toBeTruthy(); - - // Reconstruct the full arguments from all deltas - let fullArgs = ""; - for (const toolResult of toolCallResults) { - fullArgs += toolResult[0].toolCalls?.[0]?.function?.arguments || ""; - } - - console.log("Full args reconstructed:", fullArgs); - - // Should be valid JSON - expect(() => JSON.parse(fullArgs)).not.toThrow(); - - const parsed = JSON.parse(fullArgs); - expect(parsed.filepath).toBe("test.js"); - expect(parsed.diffs).toBeTruthy(); - - // The diffs should contain the complex content including special markers - const diffContent1 = - typeof parsed.diffs === "string" - ? parsed.diffs - : JSON.stringify(parsed.diffs); - expect(diffContent1).toContain("SEARCH"); - expect(diffContent1).toContain("REPLACE"); - expect(diffContent1).toContain("Cannot divide by zero"); - expect(diffContent1).toContain("assistant"); // Word should be preserved in content - }); - - it("handles arguments with embedded JSON and special escape sequences", async () => { - // Test with JSON array containing embedded quotes and backslashes - const jsonArrayContent = `["first item", "second \\"quoted\\" item", "third\\nitem"]`; - - const messages: ChatMessage[][] = [ - [{ role: "assistant", content: "```tool\n" }], - [{ role: "assistant", content: "TOOL_NAME: test_tool\n" }], - [{ role: "assistant", content: "BEGIN_ARG: json_array\n" }], - [{ role: "assistant", content: jsonArrayContent }], - [{ role: "assistant", content: "\nEND_ARG\n" }], - [{ role: "assistant", content: "```" }], - ]; - - const generator = interceptSystemToolCalls( - createAsyncGenerator(messages), - abortController, - ); - - const allResults = await collectAllResults(generator); - - // Should have parsed the tool call - const toolCallResults = allResults.filter( - (r): r is AssistantChatMessage[] => - r && r[0] && isAssistantMessageWithToolCalls(r[0]), - ); - - expect(toolCallResults.length).toBeGreaterThan(0); - - // Reconstruct the full arguments - let fullArgs = ""; - for (const toolResult of toolCallResults) { - fullArgs += toolResult[0].toolCalls?.[0]?.function?.arguments || ""; - } - - // Should be valid JSON - expect(() => JSON.parse(fullArgs)).not.toThrow(); - - const parsed = JSON.parse(fullArgs); - expect(parsed.json_array).toBeTruthy(); - - // The parsed array should be correct - const arrayValue = - typeof parsed.json_array === "string" - ? JSON.parse(parsed.json_array) - : parsed.json_array; - - expect(Array.isArray(arrayValue)).toBe(true); - expect(arrayValue).toHaveLength(3); - expect(arrayValue[0]).toBe("first item"); - expect(arrayValue[1]).toBe('second "quoted" item'); - expect(arrayValue[2]).toBe("third\nitem"); - }); - - it("simulates the exact user scenario that causes parsing issues", async () => { - // This simulates the exact scenario from example.txt that was causing issues - const userProblemDiff = `[ -"------- SEARCH - /** - * Divide the current result by a number - * @param {number} number - The number to divide by - * @returns {Calculator} - Returns this instance for method chaining - * @throws {Error} - Throws error if attempting to divide by zero - */ - divide(number) { - if (number === 0) { - throw new Error(\"Cannot divide by zero\"); - } - this.result /= number; - return this; - } -======= - /** - * Divide the current result by a number - * @param {number} number - The number to divide by - * @returns {Calculator} - Returns this instance for method chaining - * @throws {Error} - Throws error if attempting to divide by zero - */ - divide(number) { - if (number === 0) { - throw new Error(\"Cannot divide by zero\"); - } - this.result /= number; - return this; - } - - /** - * Raise the current result to the power of a number - * @param {number} number - The exponent to raise to - * @returns {Calculator} - Returns this instance for method chaining - */ - exp(number) { - this.result = Math.pow(this.result, number); - return this; - } -+++++++ REPLACE" -]`; - - // Test that the JSON string can be properly parsed and used - let parsed; - expect(() => { - parsed = JSON.parse(userProblemDiff); - }).not.toThrow(); - - expect(Array.isArray(parsed)).toBe(true); - expect(parsed!.length).toBe(1); - - const diffContent = parsed![0]; - expect(diffContent).toContain("------- SEARCH"); - expect(diffContent).toContain("======="); - expect(diffContent).toContain("+++++++ REPLACE"); - expect(diffContent).toContain("Cannot divide by zero"); - }); -}); diff --git a/gui/src/util/clientTools/searchReplaceImpl.ts b/gui/src/util/clientTools/searchReplaceImpl.ts index bdd04a5e7..1fcf649e5 100644 --- a/gui/src/util/clientTools/searchReplaceImpl.ts +++ b/gui/src/util/clientTools/searchReplaceImpl.ts @@ -10,45 +10,13 @@ export const searchReplaceToolImpl: ClientToolImpl = async ( toolCallId, extras, ) => { - const { filepath } = args; - let { diffs } = args; - - // Handle legacy parameter name and system message tools edge cases - if (!diffs && (args as any).diff) { - diffs = (args as any).diff; - } - - if (!diffs) { - throw new Error("Missing required parameter: diffs (or diff)"); - } + const { filepath, diffs } = args; const state = extras.getState(); const allowAnonymousTelemetry = state.config.config.allowAnonymousTelemetry; const streamId = uuid(); - // Handle the case where system message tools provide diffs as a JSON string - // instead of an actual array (due to JSON parsing/stringifying in system message tools) - let processedDiffs: string[]; - if (typeof diffs === "string") { - try { - const parsed = JSON.parse(diffs); - if (Array.isArray(parsed)) { - processedDiffs = parsed; - } else { - processedDiffs = [diffs]; - } - } catch (e) { - processedDiffs = [diffs]; - } - } else if (Array.isArray(diffs)) { - processedDiffs = diffs; - } else { - throw new Error( - "diffs parameter must be an array of strings or a JSON string representing an array", - ); - } - // Resolve the file path const resolvedFilepath = await resolveRelativePathInDir( filepath, @@ -60,16 +28,11 @@ export const searchReplaceToolImpl: ClientToolImpl = async ( // Parse all search/replace blocks from all diff strings const allBlocks = []; - for (let diffIndex = 0; diffIndex < processedDiffs.length; diffIndex++) { - const blocks = parseAllSearchReplaceBlocks(processedDiffs[diffIndex]); + for (let diffIndex = 0; diffIndex < diffs.length; diffIndex++) { + const blocks = parseAllSearchReplaceBlocks(diffs[diffIndex]); if (blocks.length === 0) { throw new Error( - `No complete search/replace blocks found in diff ${diffIndex + 1}. Each diff must contain at least one complete SEARCH/REPLACE block with the format: -------- SEARCH -[content to find] -======= -[replacement content] -+++++++ REPLACE`, + `No complete search/replace blocks found in diff ${diffIndex + 1}`, ); } allBlocks.push(...blocks);